Merge branch 'jk/read-in-full'
authorJunio C Hamano <gitster@pobox.com>
Tue, 3 Oct 2017 06:42:49 +0000 (15:42 +0900)
committerJunio C Hamano <gitster@pobox.com>
Tue, 3 Oct 2017 06:42:49 +0000 (15:42 +0900)
Code clean-up to prevent future mistakes by copying and pasting
code that checks the result of read_in_full() function.

* jk/read-in-full:
worktree: check the result of read_in_full()
worktree: use xsize_t to access file size
distinguish error versus short read from read_in_full()
avoid looking at errno for short read_in_full() returns
prefer "!=" when checking read_in_full() result
notes-merge: drop dead zero-write code
files-backend: prefer "0" for write_in_full() error check

1  2 
packfile.c
refs/files-backend.c
sha1_file.c
diff --combined packfile.c
index f69a5c8d607af191fa8ba04e84da8f02c40823ef,263efb7151a4c4b1ef2dbc96d53b0636ff8ce287..eab7542487a194ecd25708f906e66c40e1bed336
@@@ -40,7 -40,9 +40,7 @@@ static unsigned int pack_max_fds
  static size_t peak_pack_mapped;
  static size_t pack_mapped;
  struct packed_git *packed_git;
 -
 -static struct mru packed_git_mru_storage;
 -struct mru *packed_git_mru = &packed_git_mru_storage;
 +struct mru packed_git_mru;
  
  #define SZ_FMT PRIuMAX
  static inline uintmax_t sz_fmt(size_t s) { return s; }
@@@ -442,6 -444,7 +442,7 @@@ static int open_packed_git_1(struct pac
        unsigned char sha1[20];
        unsigned char *idx_sha1;
        long fd_flag;
+       ssize_t read_result;
  
        if (!p->index_data && open_pack_index(p))
                return error("packfile %s index unavailable", p->pack_name);
                return error("cannot set FD_CLOEXEC");
  
        /* Verify we recognize this pack file format. */
-       if (read_in_full(p->pack_fd, &hdr, sizeof(hdr)) != sizeof(hdr))
+       read_result = read_in_full(p->pack_fd, &hdr, sizeof(hdr));
+       if (read_result < 0)
+               return error_errno("error reading from %s", p->pack_name);
+       if (read_result != sizeof(hdr))
                return error("file %s is far too short to be a packfile", p->pack_name);
        if (hdr.hdr_signature != htonl(PACK_SIGNATURE))
                return error("file %s is not a GIT packfile", p->pack_name);
                             p->num_objects);
        if (lseek(p->pack_fd, p->pack_size - sizeof(sha1), SEEK_SET) == -1)
                return error("end of packfile %s is unavailable", p->pack_name);
-       if (read_in_full(p->pack_fd, sha1, sizeof(sha1)) != sizeof(sha1))
+       read_result = read_in_full(p->pack_fd, sha1, sizeof(sha1));
+       if (read_result < 0)
+               return error_errno("error reading from %s", p->pack_name);
+       if (read_result != sizeof(sha1))
                return error("packfile %s signature is unavailable", p->pack_name);
        idx_sha1 = ((unsigned char *)p->index_data) + p->index_size - 40;
        if (hashcmp(sha1, idx_sha1))
@@@ -859,9 -868,9 +866,9 @@@ static void prepare_packed_git_mru(void
  {
        struct packed_git *p;
  
 -      mru_clear(packed_git_mru);
 +      mru_clear(&packed_git_mru);
        for (p = packed_git; p; p = p->next)
 -              mru_append(packed_git_mru, p);
 +              mru_append(&packed_git_mru, p);
  }
  
  static int prepare_packed_git_run_once = 0;
@@@ -1830,9 -1839,9 +1837,9 @@@ int find_pack_entry(const unsigned cha
        if (!packed_git)
                return 0;
  
 -      for (p = packed_git_mru->head; p; p = p->next) {
 +      for (p = packed_git_mru.head; p; p = p->next) {
                if (fill_pack_entry(sha1, e, p->item)) {
 -                      mru_mark(packed_git_mru, p);
 +                      mru_mark(&packed_git_mru, p);
                        return 1;
                }
        }
diff --combined refs/files-backend.c
index 38d16a13a88ac99c5c8d1aeffbe85a74ab3ef345,e35c64c001d98582f72945b3af9ab3bc5261273b..4b46cd2e26dbcb782d58bcf4a7bc0c0494b715a4
@@@ -1258,9 -1258,9 +1258,9 @@@ static int commit_ref_update(struct fil
                             const struct object_id *oid, const char *logmsg,
                             struct strbuf *err);
  
 -static int files_rename_ref(struct ref_store *ref_store,
 +static int files_copy_or_rename_ref(struct ref_store *ref_store,
                            const char *oldrefname, const char *newrefname,
 -                          const char *logmsg)
 +                          const char *logmsg, int copy)
  {
        struct files_ref_store *refs =
                files_downcast(ref_store, REF_STORE_WRITE, "rename_ref");
        }
  
        if (flag & REF_ISSYMREF) {
 -              ret = error("refname %s is a symbolic ref, renaming it is not supported",
 -                          oldrefname);
 +              if (copy)
 +                      ret = error("refname %s is a symbolic ref, copying it is not supported",
 +                                  oldrefname);
 +              else
 +                      ret = error("refname %s is a symbolic ref, renaming it is not supported",
 +                                  oldrefname);
                goto out;
        }
        if (!refs_rename_ref_available(&refs->base, oldrefname, newrefname)) {
                goto out;
        }
  
 -      if (log && rename(sb_oldref.buf, tmp_renamed_log.buf)) {
 +      if (!copy && log && rename(sb_oldref.buf, tmp_renamed_log.buf)) {
                ret = error("unable to move logfile logs/%s to logs/"TMP_RENAMED_LOG": %s",
                            oldrefname, strerror(errno));
                goto out;
        }
  
 -      if (refs_delete_ref(&refs->base, logmsg, oldrefname,
 +      if (copy && log && copy_file(tmp_renamed_log.buf, sb_oldref.buf, 0644)) {
 +              ret = error("unable to copy logfile logs/%s to logs/"TMP_RENAMED_LOG": %s",
 +                          oldrefname, strerror(errno));
 +              goto out;
 +      }
 +
 +      if (!copy && refs_delete_ref(&refs->base, logmsg, oldrefname,
                            orig_oid.hash, REF_NODEREF)) {
                error("unable to delete old %s", oldrefname);
                goto rollback;
         * the safety anyway; we want to delete the reference whatever
         * its current value.
         */
 -      if (!refs_read_ref_full(&refs->base, newrefname,
 +      if (!copy && !refs_read_ref_full(&refs->base, newrefname,
                                RESOLVE_REF_READING | RESOLVE_REF_NO_RECURSE,
                                oid.hash, NULL) &&
            refs_delete_ref(&refs->base, NULL, newrefname,
        lock = lock_ref_sha1_basic(refs, newrefname, NULL, NULL, NULL,
                                   REF_NODEREF, NULL, &err);
        if (!lock) {
 -              error("unable to rename '%s' to '%s': %s", oldrefname, newrefname, err.buf);
 +              if (copy)
 +                      error("unable to copy '%s' to '%s': %s", oldrefname, newrefname, err.buf);
 +              else
 +                      error("unable to rename '%s' to '%s': %s", oldrefname, newrefname, err.buf);
                strbuf_release(&err);
                goto rollback;
        }
        return ret;
  }
  
 +static int files_rename_ref(struct ref_store *ref_store,
 +                          const char *oldrefname, const char *newrefname,
 +                          const char *logmsg)
 +{
 +      return files_copy_or_rename_ref(ref_store, oldrefname,
 +                               newrefname, logmsg, 0);
 +}
 +
 +static int files_copy_ref(struct ref_store *ref_store,
 +                          const char *oldrefname, const char *newrefname,
 +                          const char *logmsg)
 +{
 +      return files_copy_or_rename_ref(ref_store, oldrefname,
 +                               newrefname, logmsg, 1);
 +}
 +
  static int close_ref_gently(struct ref_lock *lock)
  {
        if (close_lock_file_gently(&lock->lk))
@@@ -1705,12 -1676,13 +1705,12 @@@ static int commit_ref_update(struct fil
                 * check with HEAD only which should cover 99% of all usage
                 * scenarios (even 100% of the default ones).
                 */
 -              struct object_id head_oid;
                int head_flag;
                const char *head_ref;
  
                head_ref = refs_resolve_ref_unsafe(&refs->base, "HEAD",
                                                   RESOLVE_REF_READING,
 -                                                 head_oid.hash, &head_flag);
 +                                                 NULL, &head_flag);
                if (head_ref && (head_flag & REF_ISSYMREF) &&
                    !strcmp(head_ref, lock->ref_name)) {
                        struct strbuf log_err = STRBUF_INIT;
@@@ -3035,7 -3007,7 +3035,7 @@@ static int files_reflog_expire(struct r
                } else if (update &&
                           (write_in_full(get_lock_file_fd(&lock->lk),
                                oid_to_hex(&cb.last_kept_oid), GIT_SHA1_HEXSZ) < 0 ||
-                           write_str_in_full(get_lock_file_fd(&lock->lk), "\n") < 1 ||
+                           write_str_in_full(get_lock_file_fd(&lock->lk), "\n") < 0 ||
                            close_ref_gently(lock) < 0)) {
                        status |= error("couldn't write %s",
                                        get_lock_file_path(&lock->lk));
@@@ -3093,7 -3065,6 +3093,7 @@@ struct ref_storage_be refs_be_files = 
        files_create_symref,
        files_delete_refs,
        files_rename_ref,
 +      files_copy_ref,
  
        files_ref_iterator_begin,
        files_read_raw_ref,
diff --combined sha1_file.c
index 5a2014811fd0a42aa74b36bf3a470e471968be6f,11346cf6d83e69cfb39ab059de8cae6f6c982c64..09ad64ce555e0b908ce0d9f22219026a2d37ae0c
@@@ -398,7 -398,7 +398,7 @@@ static const char *parse_alt_odb_entry(
        return end;
  }
  
 -static void link_alt_odb_entries(const char *alt, int len, int sep,
 +static void link_alt_odb_entries(const char *alt, int sep,
                                 const char *relative_base, int depth)
  {
        struct strbuf objdirbuf = STRBUF_INIT;
  
  static void read_info_alternates(const char * relative_base, int depth)
  {
 -      char *map;
 -      size_t mapsz;
 -      struct stat st;
        char *path;
 -      int fd;
 +      struct strbuf buf = STRBUF_INIT;
  
        path = xstrfmt("%s/info/alternates", relative_base);
 -      fd = git_open(path);
 -      free(path);
 -      if (fd < 0)
 -              return;
 -      if (fstat(fd, &st) || (st.st_size == 0)) {
 -              close(fd);
 +      if (strbuf_read_file(&buf, path, 1024) < 0) {
 +              warn_on_fopen_errors(path);
 +              free(path);
                return;
        }
 -      mapsz = xsize_t(st.st_size);
 -      map = xmmap(NULL, mapsz, PROT_READ, MAP_PRIVATE, fd, 0);
 -      close(fd);
  
 -      link_alt_odb_entries(map, mapsz, '\n', relative_base, depth);
 -
 -      munmap(map, mapsz);
 +      link_alt_odb_entries(buf.buf, '\n', relative_base, depth);
 +      strbuf_release(&buf);
 +      free(path);
  }
  
  struct alternate_object_database *alloc_alt_odb(const char *dir)
@@@ -494,7 -503,7 +494,7 @@@ void add_to_alternates_file(const char 
                if (commit_lock_file(lock))
                        die_errno("unable to move new alternates file into place");
                if (alt_odb_tail)
 -                      link_alt_odb_entries(reference, strlen(reference), '\n', NULL, 0);
 +                      link_alt_odb_entries(reference, '\n', NULL, 0);
        }
        free(alts);
  }
@@@ -507,7 -516,7 +507,7 @@@ void add_to_alternates_memory(const cha
         */
        prepare_alt_odb();
  
 -      link_alt_odb_entries(reference, strlen(reference), '\n', NULL, 0);
 +      link_alt_odb_entries(reference, '\n', NULL, 0);
  }
  
  /*
@@@ -610,7 -619,7 +610,7 @@@ void prepare_alt_odb(void
        if (!alt) alt = "";
  
        alt_odb_tail = &alt_odb_list;
 -      link_alt_odb_entries(alt, strlen(alt), PATH_SEP, NULL, 0);
 +      link_alt_odb_entries(alt, PATH_SEP, NULL, 0);
  
        read_info_alternates(get_object_directory(), 0);
  }
@@@ -1748,10 -1757,15 +1748,15 @@@ static int index_core(unsigned char *sh
                ret = index_mem(sha1, "", size, type, path, flags);
        } else if (size <= SMALL_FILE_SIZE) {
                char *buf = xmalloc(size);
-               if (size == read_in_full(fd, buf, size))
-                       ret = index_mem(sha1, buf, size, type, path, flags);
+               ssize_t read_result = read_in_full(fd, buf, size);
+               if (read_result < 0)
+                       ret = error_errno("read error while indexing %s",
+                                         path ? path : "<unknown>");
+               else if (read_result != size)
+                       ret = error("short read while indexing %s",
+                                   path ? path : "<unknown>");
                else
-                       ret = error_errno("short read");
+                       ret = index_mem(sha1, buf, size, type, path, flags);
                free(buf);
        } else {
                void *buf = xmmap(NULL, size, PROT_READ, MAP_PRIVATE, fd, 0);