commit-graph: return with errors during write

The write_commit_graph() method uses die() to report failure and
exit when confronted with an unexpected condition. This use of
die() in a library function is incorrect and is now replaced by
error() statements and an int return type. Return zero on success
and a negative value on failure.

Now that we use 'goto cleanup' to jump to the terminal condition
on an error, we have new paths that could lead to uninitialized
values. New initializers are added to correct for this.

The builtins 'commit-graph', 'gc', and 'commit' call these methods,
so update them to check the return value. Test that 'git commit-graph
write' returns a proper error code when hitting a failure condition
in write_commit_graph().

Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

diff --git a/builtin/commit-graph.c b/builtin/commit-graph.c
index 537fdfd0f0759e8cce2303af5a197ae77b2b879b..2a1c4d701fcb50605b46c441dd16729f8b7c6605 100644 (file)
--- a/builtin/commit-graph.c
+++ b/builtin/commit-graph.c
@@ -141,6 +141,7 @@ static int graph_write(int argc, const char **argv)
        struct string_list *pack_indexes = NULL;
        struct string_list *commit_hex = NULL;
        struct string_list lines;
+       int result = 0;
 
        static struct option builtin_commit_graph_write_options[] = {
                OPT_STRING(0, "object-dir", &opts.obj_dir,
@@ -168,10 +169,8 @@ static int graph_write(int argc, const char **argv)
 
        read_replace_refs = 0;
 
-       if (opts.reachable) {
-               write_commit_graph_reachable(opts.obj_dir, opts.append, 1);
-               return 0;
-       }
+       if (opts.reachable)
+               return write_commit_graph_reachable(opts.obj_dir, opts.append, 1);
 
        string_list_init(&lines, 0);
        if (opts.stdin_packs || opts.stdin_commits) {
@@ -188,14 +187,15 @@ static int graph_write(int argc, const char **argv)
                UNLEAK(buf);
        }
 
-       write_commit_graph(opts.obj_dir,
-                          pack_indexes,
-                          commit_hex,
-                          opts.append,
-                          1);
+       if (write_commit_graph(opts.obj_dir,
+                              pack_indexes,
+                              commit_hex,
+                              opts.append,
+                              1))
+               result = 1;
 
        UNLEAK(lines);
-       return 0;
+       return result;
 }
 
 int cmd_commit_graph(int argc, const char **argv, const char *prefix)

diff --git a/builtin/commit.c b/builtin/commit.c
index 2986553d5ffb97f0798e4d4eb921073439fb9a92..b9ea7222fae4d6caf0f7ab6f975179922f5e2293 100644 (file)
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -1669,8 +1669,9 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
                      "new_index file. Check that disk is not full and quota is\n"
                      "not exceeded, and then \"git reset HEAD\" to recover."));
 
-       if (git_env_bool(GIT_TEST_COMMIT_GRAPH, 0))
-               write_commit_graph_reachable(get_object_directory(), 0, 0);
+       if (git_env_bool(GIT_TEST_COMMIT_GRAPH, 0) &&
+           write_commit_graph_reachable(get_object_directory(), 0, 0))
+               return 1;
 
        repo_rerere(the_repository, 0);
        run_command_v_opt(argv_gc_auto, RUN_GIT_CMD);

diff --git a/builtin/gc.c b/builtin/gc.c
index 020f725acc40f413c49f812ea0e6aac0153d097c..3984addf73c48a53afb3f8546836733acb77f53e 100644 (file)
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -664,9 +664,10 @@ int cmd_gc(int argc, const char **argv, const char *prefix)
                clean_pack_garbage();
        }
 
-       if (gc_write_commit_graph)
-               write_commit_graph_reachable(get_object_directory(), 0,
-                                            !quiet && !daemonized);
+       if (gc_write_commit_graph &&
+           write_commit_graph_reachable(get_object_directory(), 0,
+                                        !quiet && !daemonized))
+               return 1;
 
        if (auto_gc && too_many_loose_objects())
                warning(_("There are too many unreachable loose objects; "

diff --git a/commit-graph.c b/commit-graph.c
index 66865acbd7489df4f85ee2454fe05a9b346e7aa7..1b58d1da149ae80c971326d0069e17477d5022c2 100644 (file)
--- a/commit-graph.c
+++ b/commit-graph.c
@@ -851,27 +851,30 @@ static int add_ref_to_list(const char *refname,
        return 0;
 }
 
-void write_commit_graph_reachable(const char *obj_dir, int append,
-                                 int report_progress)
+int write_commit_graph_reachable(const char *obj_dir, int append,
+                                int report_progress)
 {
        struct string_list list = STRING_LIST_INIT_DUP;
+       int result;
 
        for_each_ref(add_ref_to_list, &list);
-       write_commit_graph(obj_dir, NULL, &list, append, report_progress);
+       result = write_commit_graph(obj_dir, NULL, &list,
+                                   append, report_progress);
 
        string_list_clear(&list, 0);
+       return result;
 }
 
-void write_commit_graph(const char *obj_dir,
-                       struct string_list *pack_indexes,
-                       struct string_list *commit_hex,
-                       int append, int report_progress)
+int write_commit_graph(const char *obj_dir,
+                      struct string_list *pack_indexes,
+                      struct string_list *commit_hex,
+                      int append, int report_progress)
 {
        struct packed_oid_list oids;
        struct packed_commit_list commits;
        struct hashfile *f;
        uint32_t i, count_distinct = 0;
-       char *graph_name;
+       char *graph_name = NULL;
        struct lock_file lk = LOCK_INIT;
        uint32_t chunk_ids[5];
        uint64_t chunk_offsets[5];
@@ -883,15 +886,17 @@ void write_commit_graph(const char *obj_dir,
        uint64_t progress_cnt = 0;
        struct strbuf progress_title = STRBUF_INIT;
        unsigned long approx_nr_objects;
+       int res = 0;
 
        if (!commit_graph_compatible(the_repository))
-               return;
+               return 0;
 
        oids.nr = 0;
        approx_nr_objects = approximate_object_count();
        oids.alloc = approx_nr_objects / 32;
        oids.progress = NULL;
        oids.progress_done = 0;
+       commits.list = NULL;
 
        if (append) {
                prepare_commit_graph_one(the_repository, obj_dir);
@@ -932,10 +937,16 @@ void write_commit_graph(const char *obj_dir,
                        strbuf_setlen(&packname, dirlen);
                        strbuf_addstr(&packname, pack_indexes->items[i].string);
                        p = add_packed_git(packname.buf, packname.len, 1);
-                       if (!p)
-                               die(_("error adding pack %s"), packname.buf);
-                       if (open_pack_index(p))
-                               die(_("error opening index for %s"), packname.buf);
+                       if (!p) {
+                               error(_("error adding pack %s"), packname.buf);
+                               res = -1;
+                               goto cleanup;
+                       }
+                       if (open_pack_index(p)) {
+                               error(_("error opening index for %s"), packname.buf);
+                               res = -1;
+                               goto cleanup;
+                       }
                        for_each_object_in_pack(p, add_packed_commits, &oids,
                                                FOR_EACH_OBJECT_PACK_ORDER);
                        close_pack(p);
@@ -1006,8 +1017,11 @@ void write_commit_graph(const char *obj_dir,
        }
        stop_progress(&progress);
 
-       if (count_distinct >= GRAPH_EDGE_LAST_MASK)
-               die(_("the commit graph format cannot write %d commits"), count_distinct);
+       if (count_distinct >= GRAPH_EDGE_LAST_MASK) {
+               error(_("the commit graph format cannot write %d commits"), count_distinct);
+               res = -1;
+               goto cleanup;
+       }
 
        commits.nr = 0;
        commits.alloc = count_distinct;
@@ -1039,16 +1053,21 @@ void write_commit_graph(const char *obj_dir,
        num_chunks = num_extra_edges ? 4 : 3;
        stop_progress(&progress);
 
-       if (commits.nr >= GRAPH_EDGE_LAST_MASK)
-               die(_("too many commits to write graph"));
+       if (commits.nr >= GRAPH_EDGE_LAST_MASK) {
+               error(_("too many commits to write graph"));
+               res = -1;
+               goto cleanup;
+       }
 
        compute_generation_numbers(&commits, report_progress);
 
        graph_name = get_commit_graph_filename(obj_dir);
        if (safe_create_leading_directories(graph_name)) {
                UNLEAK(graph_name);
-               die_errno(_("unable to create leading directories of %s"),
-                         graph_name);
+               error(_("unable to create leading directories of %s"),
+                       graph_name);
+               res = -1;
+               goto cleanup;
        }
 
        hold_lock_file_for_update(&lk, graph_name, LOCK_DIE_ON_ERROR);
@@ -1107,9 +1126,12 @@ void write_commit_graph(const char *obj_dir,
        finalize_hashfile(f, NULL, CSUM_HASH_IN_STREAM | CSUM_FSYNC);
        commit_lock_file(&lk);
 
+cleanup:
        free(graph_name);
        free(commits.list);
        free(oids.list);
+
+       return res;
 }
 
 #define VERIFY_COMMIT_GRAPH_ERROR_HASH 2

diff --git a/commit-graph.h b/commit-graph.h
index 7dfb8c896fc35f633c73221ec639ca9c425338ab..869717ca19f19294e28ca01096a1ba0e51e2e0a1 100644 (file)
--- a/commit-graph.h
+++ b/commit-graph.h
@@ -65,12 +65,18 @@ struct commit_graph *parse_commit_graph(void *graph_map, int fd,
  */
 int generation_numbers_enabled(struct repository *r);
 
-void write_commit_graph_reachable(const char *obj_dir, int append,
+/*
+ * The write_commit_graph* methods return zero on success
+ * and a negative value on failure. Note that if the repository
+ * is not compatible with the commit-graph feature, then the
+ * methods will return 0 without writing a commit-graph.
+ */
+int write_commit_graph_reachable(const char *obj_dir, int append,
                                  int report_progress);
-void write_commit_graph(const char *obj_dir,
-                       struct string_list *pack_indexes,
-                       struct string_list *commit_hex,
-                       int append, int report_progress);
+int write_commit_graph(const char *obj_dir,
+                      struct string_list *pack_indexes,
+                      struct string_list *commit_hex,
+                      int append, int report_progress);
 
 int verify_commit_graph(struct repository *r, struct commit_graph *g);
 

diff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh
index e80c1cac02b26239c510765cdd8fb28dc9bafba1..3b6fd0d72848ff182c4a73a4626fae4af06fb023 100755 (executable)
--- a/t/t5318-commit-graph.sh
+++ b/t/t5318-commit-graph.sh
@@ -23,6 +23,14 @@ test_expect_success 'write graph with no packs' '
        test_path_is_file info/commit-graph
 '
 
+test_expect_success 'close with correct error on bad input' '
+       cd "$TRASH_DIRECTORY/full" &&
+       echo doesnotexist >in &&
+       { git commit-graph write --stdin-packs <in 2>stderr; ret=$?; } &&
+       test "$ret" = 1 &&
+       test_i18ngrep "error adding pack" stderr
+'
+
 test_expect_success 'create commits and repack' '
        cd "$TRASH_DIRECTORY/full" &&
        for i in $(test_seq 3)