Merge branch 'sg/commit-graph-validate'

The code to write commit-graph over given commit object names has
been made a bit more robust.

* sg/commit-graph-validate:
  commit-graph: error out on invalid commit oids in 'write --stdin-commits'
  commit-graph: turn a group of write-related macro flags into an enum
  t5318-commit-graph: use 'test_expect_code'

diff --git a/builtin/commit-graph.c b/builtin/commit-graph.c
index 38027b83d9d8329a1dc2e47b236a985e4ce71060..57863619b71dc9d33ff864b2ce39c6e3a7d6559b 100644 (file)
--- a/builtin/commit-graph.c
+++ b/builtin/commit-graph.c
@@ -154,7 +154,7 @@ static int graph_write(int argc, const char **argv)
        struct string_list *commit_hex = NULL;
        struct string_list lines;
        int result = 0;
-       unsigned int flags = COMMIT_GRAPH_PROGRESS;
+       enum commit_graph_write_flags flags = COMMIT_GRAPH_WRITE_PROGRESS;
 
        static struct option builtin_commit_graph_write_options[] = {
                OPT_STRING(0, "object-dir", &opts.obj_dir,
@@ -192,9 +192,9 @@ static int graph_write(int argc, const char **argv)
        if (!opts.obj_dir)
                opts.obj_dir = get_object_directory();
        if (opts.append)
-               flags |= COMMIT_GRAPH_APPEND;
+               flags |= COMMIT_GRAPH_WRITE_APPEND;
        if (opts.split)
-               flags |= COMMIT_GRAPH_SPLIT;
+               flags |= COMMIT_GRAPH_WRITE_SPLIT;
 
        read_replace_refs = 0;
 
@@ -213,8 +213,10 @@ static int graph_write(int argc, const char **argv)
 
                if (opts.stdin_packs)
                        pack_indexes = &lines;
-               if (opts.stdin_commits)
+               if (opts.stdin_commits) {
                        commit_hex = &lines;
+                       flags |= COMMIT_GRAPH_WRITE_CHECK_OIDS;
+               }
 
                UNLEAK(buf);
        }

diff --git a/builtin/gc.c b/builtin/gc.c
index c18efadda53e54f0e80dbd16737e2d40f47fa16f..305fb0f45af3bdfc32e15bcc18acdcaa6af205cb 100644 (file)
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -687,7 +687,7 @@ int cmd_gc(int argc, const char **argv, const char *prefix)
 
        if (gc_write_commit_graph &&
            write_commit_graph_reachable(get_object_directory(),
-                                        !quiet && !daemonized ? COMMIT_GRAPH_PROGRESS : 0,
+                                        !quiet && !daemonized ? COMMIT_GRAPH_WRITE_PROGRESS : 0,
                                         NULL))
                return 1;
 

diff --git a/commit-graph.c b/commit-graph.c
index fe954ab5f845e65bc5cad987308aeb6598f2fd24..f2888c203b677a8a0091f7f63129efe282899172 100644 (file)
--- a/commit-graph.c
+++ b/commit-graph.c
@@ -783,7 +783,8 @@ struct write_commit_graph_context {
 
        unsigned append:1,
                 report_progress:1,
-                split:1;
+                split:1,
+                check_oids:1;
 
        const struct split_commit_graph_opts *split_opts;
 };
@@ -1134,7 +1135,8 @@ static int add_ref_to_list(const char *refname,
        return 0;
 }
 
-int write_commit_graph_reachable(const char *obj_dir, unsigned int flags,
+int write_commit_graph_reachable(const char *obj_dir,
+                                enum commit_graph_write_flags flags,
                                 const struct split_commit_graph_opts *split_opts)
 {
        struct string_list list = STRING_LIST_INIT_DUP;
@@ -1193,8 +1195,8 @@ static int fill_oids_from_packs(struct write_commit_graph_context *ctx,
        return 0;
 }
 
-static void fill_oids_from_commit_hex(struct write_commit_graph_context *ctx,
-                                     struct string_list *commit_hex)
+static int fill_oids_from_commit_hex(struct write_commit_graph_context *ctx,
+                                    struct string_list *commit_hex)
 {
        uint32_t i;
        struct strbuf progress_title = STRBUF_INIT;
@@ -1215,20 +1217,21 @@ static void fill_oids_from_commit_hex(struct write_commit_graph_context *ctx,
                struct commit *result;
 
                display_progress(ctx->progress, i + 1);
-               if (commit_hex->items[i].string &&
-                   parse_oid_hex(commit_hex->items[i].string, &oid, &end))
-                       continue;
-
-               result = lookup_commit_reference_gently(ctx->r, &oid, 1);
-
-               if (result) {
+               if (!parse_oid_hex(commit_hex->items[i].string, &oid, &end) &&
+                   (result = lookup_commit_reference_gently(ctx->r, &oid, 1))) {
                        ALLOC_GROW(ctx->oids.list, ctx->oids.nr + 1, ctx->oids.alloc);
                        oidcpy(&ctx->oids.list[ctx->oids.nr], &(result->object.oid));
                        ctx->oids.nr++;
+               } else if (ctx->check_oids) {
+                       error(_("invalid commit object id: %s"),
+                           commit_hex->items[i].string);
+                       return -1;
                }
        }
        stop_progress(&ctx->progress);
        strbuf_release(&progress_title);
+
+       return 0;
 }
 
 static void fill_oids_from_all_packs(struct write_commit_graph_context *ctx)
@@ -1752,7 +1755,7 @@ out:
 int write_commit_graph(const char *obj_dir,
                       struct string_list *pack_indexes,
                       struct string_list *commit_hex,
-                      unsigned int flags,
+                      enum commit_graph_write_flags flags,
                       const struct split_commit_graph_opts *split_opts)
 {
        struct write_commit_graph_context *ctx;
@@ -1773,9 +1776,10 @@ int write_commit_graph(const char *obj_dir,
        if (len && ctx->obj_dir[len - 1] == '/')
                ctx->obj_dir[len - 1] = 0;
 
-       ctx->append = flags & COMMIT_GRAPH_APPEND ? 1 : 0;
-       ctx->report_progress = flags & COMMIT_GRAPH_PROGRESS ? 1 : 0;
-       ctx->split = flags & COMMIT_GRAPH_SPLIT ? 1 : 0;
+       ctx->append = flags & COMMIT_GRAPH_WRITE_APPEND ? 1 : 0;
+       ctx->report_progress = flags & COMMIT_GRAPH_WRITE_PROGRESS ? 1 : 0;
+       ctx->split = flags & COMMIT_GRAPH_WRITE_SPLIT ? 1 : 0;
+       ctx->check_oids = flags & COMMIT_GRAPH_WRITE_CHECK_OIDS ? 1 : 0;
        ctx->split_opts = split_opts;
 
        if (ctx->split) {
@@ -1830,8 +1834,10 @@ int write_commit_graph(const char *obj_dir,
                        goto cleanup;
        }
 
-       if (commit_hex)
-               fill_oids_from_commit_hex(ctx, commit_hex);
+       if (commit_hex) {
+               if ((res = fill_oids_from_commit_hex(ctx, commit_hex)))
+                       goto cleanup;
+       }
 
        if (!pack_indexes && !commit_hex)
                fill_oids_from_all_packs(ctx);

diff --git a/commit-graph.h b/commit-graph.h
index df9a3b20e4abc7d388acab1cc85546aafa8345a3..486e64e591d476635c86e58518ccb3392c507e0c 100644 (file)
--- a/commit-graph.h
+++ b/commit-graph.h
@@ -71,9 +71,13 @@ struct commit_graph *parse_commit_graph(void *graph_map, int fd,
  */
 int generation_numbers_enabled(struct repository *r);
 
-#define COMMIT_GRAPH_APPEND     (1 << 0)
-#define COMMIT_GRAPH_PROGRESS   (1 << 1)
-#define COMMIT_GRAPH_SPLIT      (1 << 2)
+enum commit_graph_write_flags {
+       COMMIT_GRAPH_WRITE_APPEND     = (1 << 0),
+       COMMIT_GRAPH_WRITE_PROGRESS   = (1 << 1),
+       COMMIT_GRAPH_WRITE_SPLIT      = (1 << 2),
+       /* Make sure that each OID in the input is a valid commit OID. */
+       COMMIT_GRAPH_WRITE_CHECK_OIDS = (1 << 3)
+};
 
 struct split_commit_graph_opts {
        int size_multiple;
@@ -87,12 +91,13 @@ struct split_commit_graph_opts {
  * 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, unsigned int flags,
+int write_commit_graph_reachable(const char *obj_dir,
+                                enum commit_graph_write_flags flags,
                                 const struct split_commit_graph_opts *split_opts);
 int write_commit_graph(const char *obj_dir,
                       struct string_list *pack_indexes,
                       struct string_list *commit_hex,
-                      unsigned int flags,
+                      enum commit_graph_write_flags flags,
                       const struct split_commit_graph_opts *split_opts);
 
 #define COMMIT_GRAPH_VERIFY_SHALLOW    (1 << 0)

diff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh
index 22cb9d66430410f726e821e906fa79587f43c3e8..ab3eccf0fafa3f8e9158b6347a544a8b4202b7ce 100755 (executable)
--- a/t/t5318-commit-graph.sh
+++ b/t/t5318-commit-graph.sh
@@ -23,11 +23,10 @@ test_expect_success 'write graph with no packs' '
        test_path_is_missing info/commit-graph
 '
 
-test_expect_success 'close with correct error on bad input' '
+test_expect_success 'exit with correct error on bad input to --stdin-packs' '
        cd "$TRASH_DIRECTORY/full" &&
        echo doesnotexist >in &&
-       { git commit-graph write --stdin-packs <in 2>stderr; ret=$?; } &&
-       test "$ret" = 1 &&
+       test_expect_code 1 git commit-graph write --stdin-packs <in 2>stderr &&
        test_i18ngrep "error adding pack" stderr
 '
 
@@ -41,6 +40,15 @@ test_expect_success 'create commits and repack' '
        git repack
 '
 
+test_expect_success 'exit with correct error on bad input to --stdin-commits' '
+       cd "$TRASH_DIRECTORY/full" &&
+       echo HEAD | test_expect_code 1 git commit-graph write --stdin-commits 2>stderr &&
+       test_i18ngrep "invalid commit object id" stderr &&
+       # valid tree OID, but not a commit OID
+       git rev-parse HEAD^{tree} | test_expect_code 1 git commit-graph write --stdin-commits 2>stderr &&
+       test_i18ngrep "invalid commit object id" stderr
+'
+
 graph_git_two_modes() {
        git -c core.commitGraph=true $1 >output
        git -c core.commitGraph=false $1 >expect