{"thread":{"id":"51589","subject":"[PATCH 0/3] commit-graph: error out on invalid commit oids in 'write --stdin-commits'","startedAt":"2019-08-05T08:02:54Z","lastAt":"2020-04-14T02:04:16Z","messageCount":19,"participants":["SZEDER Gábor","Derrick Stolee","Jeff King","Taylor Blau","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"379927","messageId":"20190805080240.30892-1-szeder.dev@gmail.com","threadId":"51589","inReplyTo":null,"subject":"[PATCH 0/3] commit-graph: error out on invalid commit oids in 'write --stdin-commits'","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2019-08-05T08:02:37Z","receivedAt":"2019-08-05T08:02:54Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"While 'git commit-graph write --stdin-commits' expects commit object\nids as input, it accepts and silently skips over any invalid commit\nobject ids, and still exits with success:\n\n $ echo not-a-commit-oid | git commit-graph write --stdin-commits\n $ echo $?\n $ ls -l .git/objects/info/commit-graph\n ls: cannot access '.git/objects/info/commit-graph': No such file or directory\n\nThe last patch in this series fixes this issue, with a bit of\npreparatory refactoring in the second and a while-at-it cleanup in the\nfirst patches.\n\nSZEDER Gábor (3):\n t5318-commit-graph: use 'test_expect_code'\n commit-graph: turn a group of write-related macro flags into an enum\n commit-graph: error out on invalid commit oids in 'write\n --stdin-commits'\n\n builtin/commit-graph.c | 10 ++++++----\n builtin/gc.c | 2 +-\n commit-graph.c | 40 +++++++++++++++++++++++-----------------\n commit-graph.h | 15 ++++++++++-----\n t/t5318-commit-graph.sh | 14 +++++++++++---\n 5 files changed, 51 insertions(+), 30 deletions(-)\n\n-- \n2.23.0.rc1.309.g896d8c5f5f\n\n"},{"id":"379928","messageId":"20190805080240.30892-2-szeder.dev@gmail.com","threadId":"51589","inReplyTo":"20190805080240.30892-1-szeder.dev@gmail.com","subject":"[PATCH 1/3] t5318-commit-graph: use 'test_expect_code'","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2019-08-05T08:02:38Z","receivedAt":"2019-08-05T08:02:55Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"In 't5318-commit-graph.sh' the test 'close with correct error on bad\ninput' manually verifies the exit code of a 'git commit-graph write'\ncommand.\n\nUse 'test_expect_code' instead.\n\nSigned-off-by: SZEDER Gábor \n---\n t/t5318-commit-graph.sh | 3 +--\n 1 file changed, 1 insertion(+), 2 deletions(-)\n\ndiff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh\nindex 22cb9d6643..4391007f4c 100755\n--- a/t/t5318-commit-graph.sh\n+++ b/t/t5318-commit-graph.sh\n@@ -26,8 +26,7 @@ test_expect_success 'write graph with no packs' '\n test_expect_success 'close with correct error on bad input' '\n \tcd \"$TRASH_DIRECTORY/full\" &&\n \techo doesnotexist >in &&\n-\t{ git commit-graph write --stdin-packs stderr; ret=$?; } &&\n-\ttest \"$ret\" = 1 &&\n+\ttest_expect_code 1 git commit-graph write --stdin-packs stderr &&\n \ttest_i18ngrep \"error adding pack\" stderr\n '\n \n-- \n2.23.0.rc1.309.g896d8c5f5f\n\n"},{"id":"379929","messageId":"20190805080240.30892-3-szeder.dev@gmail.com","threadId":"51589","inReplyTo":"20190805080240.30892-1-szeder.dev@gmail.com","subject":"[PATCH 2/3] commit-graph: turn a group of write-related macro flags into an enum","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2019-08-05T08:02:39Z","receivedAt":"2019-08-05T08:02:56Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"Signed-off-by: SZEDER Gábor \n---\n builtin/commit-graph.c | 6 +++---\n builtin/gc.c | 2 +-\n commit-graph.c | 11 ++++++-----\n commit-graph.h | 13 ++++++++-----\n 4 files changed, 18 insertions(+), 14 deletions(-)\n\ndiff --git a/builtin/commit-graph.c b/builtin/commit-graph.c\nindex 38027b83d9..64eccde314 100644\n--- a/builtin/commit-graph.c\n+++ b/builtin/commit-graph.c\n@@ -154,7 +154,7 @@ static int graph_write(int argc, const char **argv)\n \tstruct string_list *commit_hex = NULL;\n \tstruct string_list lines;\n \tint result = 0;\n-\tunsigned int flags = COMMIT_GRAPH_PROGRESS;\n+\tenum commit_graph_write_flags flags = COMMIT_GRAPH_WRITE_PROGRESS;\n \n \tstatic struct option builtin_commit_graph_write_options[] = {\n \t\tOPT_STRING(0, \"object-dir\", &opts.obj_dir,\n@@ -192,9 +192,9 @@ static int graph_write(int argc, const char **argv)\n \tif (!opts.obj_dir)\n \t\topts.obj_dir = get_object_directory();\n \tif (opts.append)\n-\t\tflags |= COMMIT_GRAPH_APPEND;\n+\t\tflags |= COMMIT_GRAPH_WRITE_APPEND;\n \tif (opts.split)\n-\t\tflags |= COMMIT_GRAPH_SPLIT;\n+\t\tflags |= COMMIT_GRAPH_WRITE_SPLIT;\n \n \tread_replace_refs = 0;\n \ndiff --git a/builtin/gc.c b/builtin/gc.c\nindex c18efadda5..305fb0f45a 100644\n--- a/builtin/gc.c\n+++ b/builtin/gc.c\n@@ -687,7 +687,7 @@ int cmd_gc(int argc, const char **argv, const char *prefix)\n \n \tif (gc_write_commit_graph &&\n \t write_commit_graph_reachable(get_object_directory(),\n-\t\t\t\t\t !quiet && !daemonized ? COMMIT_GRAPH_PROGRESS : 0,\n+\t\t\t\t\t !quiet && !daemonized ? COMMIT_GRAPH_WRITE_PROGRESS : 0,\n \t\t\t\t\t NULL))\n \t\treturn 1;\n \ndiff --git a/commit-graph.c b/commit-graph.c\nindex b3c4de79b6..04324a4648 100644\n--- a/commit-graph.c\n+++ b/commit-graph.c\n@@ -1133,7 +1133,8 @@ static int add_ref_to_list(const char *refname,\n \treturn 0;\n }\n \n-int write_commit_graph_reachable(const char *obj_dir, unsigned int flags,\n+int write_commit_graph_reachable(const char *obj_dir,\n+\t\t\t\t enum commit_graph_write_flags flags,\n \t\t\t\t const struct split_commit_graph_opts *split_opts)\n {\n \tstruct string_list list = STRING_LIST_INIT_DUP;\n@@ -1750,7 +1751,7 @@ static void expire_commit_graphs(struct write_commit_graph_context *ctx)\n int write_commit_graph(const char *obj_dir,\n \t\t struct string_list *pack_indexes,\n \t\t struct string_list *commit_hex,\n-\t\t unsigned int flags,\n+\t\t enum commit_graph_write_flags flags,\n \t\t const struct split_commit_graph_opts *split_opts)\n {\n \tstruct write_commit_graph_context *ctx;\n@@ -1771,9 +1772,9 @@ int write_commit_graph(const char *obj_dir,\n \tif (len && ctx->obj_dir[len - 1] == '/')\n \t\tctx->obj_dir[len - 1] = 0;\n \n-\tctx->append = flags & COMMIT_GRAPH_APPEND ? 1 : 0;\n-\tctx->report_progress = flags & COMMIT_GRAPH_PROGRESS ? 1 : 0;\n-\tctx->split = flags & COMMIT_GRAPH_SPLIT ? 1 : 0;\n+\tctx->append = flags & COMMIT_GRAPH_WRITE_APPEND ? 1 : 0;\n+\tctx->report_progress = flags & COMMIT_GRAPH_WRITE_PROGRESS ? 1 : 0;\n+\tctx->split = flags & COMMIT_GRAPH_WRITE_SPLIT ? 1 : 0;\n \tctx->split_opts = split_opts;\n \n \tif (ctx->split) {\ndiff --git a/commit-graph.h b/commit-graph.h\nindex df9a3b20e4..ae4db87fb5 100644\n--- a/commit-graph.h\n+++ b/commit-graph.h\n@@ -71,9 +71,11 @@ struct commit_graph *parse_commit_graph(void *graph_map, int fd,\n */\n int generation_numbers_enabled(struct repository *r);\n \n-#define COMMIT_GRAPH_APPEND (1 << 0)\n-#define COMMIT_GRAPH_PROGRESS (1 << 1)\n-#define COMMIT_GRAPH_SPLIT (1 << 2)\n+enum commit_graph_write_flags {\n+\tCOMMIT_GRAPH_WRITE_APPEND = (1 << 0),\n+\tCOMMIT_GRAPH_WRITE_PROGRESS = (1 << 1),\n+\tCOMMIT_GRAPH_WRITE_SPLIT = (1 << 2)\n+};\n \n struct split_commit_graph_opts {\n \tint size_multiple;\n@@ -87,12 +89,13 @@ struct split_commit_graph_opts {\n * is not compatible with the commit-graph feature, then the\n * methods will return 0 without writing a commit-graph.\n */\n-int write_commit_graph_reachable(const char *obj_dir, unsigned int flags,\n+int write_commit_graph_reachable(const char *obj_dir,\n+\t\t\t\t enum commit_graph_write_flags flags,\n \t\t\t\t const struct split_commit_graph_opts *split_opts);\n int write_commit_graph(const char *obj_dir,\n \t\t struct string_list *pack_indexes,\n \t\t struct string_list *commit_hex,\n-\t\t unsigned int flags,\n+\t\t enum commit_graph_write_flags flags,\n \t\t const struct split_commit_graph_opts *split_opts);\n \n #define COMMIT_GRAPH_VERIFY_SHALLOW\t(1 << 0)\n-- \n2.23.0.rc1.309.g896d8c5f5f\n\n"},{"id":"379930","messageId":"20190805080240.30892-4-szeder.dev@gmail.com","threadId":"51589","inReplyTo":"20190805080240.30892-1-szeder.dev@gmail.com","subject":"[PATCH 3/3] commit-graph: error out on invalid commit oids in 'write --stdin-commits'","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2019-08-05T08:02:40Z","receivedAt":"2019-08-05T08:02:58Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"While 'git commit-graph write --stdin-commits' expects commit object\nids as input, it accepts and silently skips over any invalid commit\nobject ids, and still exits with success:\n\n # nonsense\n $ echo not-a-commit-oid | git commit-graph write --stdin-commits\n $ echo $?\n 0\n # sometimes I forgot that refs are not good...\n $ echo HEAD | git commit-graph write --stdin-commits\n $ echo $?\n 0\n # valid tree OID, but not a commit OID\n $ git rev-parse HEAD^{tree} | git commit-graph write --stdin-commits\n $ echo $?\n 0\n $ ls -l .git/objects/info/commit-graph\n ls: cannot access '.git/objects/info/commit-graph': No such file or directory\n\nCheck that all input records are indeed valid commit object ids and\nreturn with error otherwise, the same way '--stdin-packs' handles\ninvalid input; see e103f7276f (commit-graph: return with errors during\nwrite, 2019-06-12).\n\nNote that it should only return with error when encountering an\ninvalid commit object id coming from standard input. However,\n'--reachable' uses the same code path to process object ids pointed to\nby all refs, and that includes tag object ids as well, which should\nstill be skipped over. Therefore add a new flag to 'enum\ncommit_graph_write_flags' and a corresponding field to 'struct\nwrite_commit_graph_context', so we can differentiate between those two\ncases.\n\nSigned-off-by: SZEDER Gábor \n---\n builtin/commit-graph.c | 4 +++-\n commit-graph.c | 29 +++++++++++++++++------------\n commit-graph.h | 4 +++-\n t/t5318-commit-graph.sh | 11 ++++++++++-\n 4 files changed, 33 insertions(+), 15 deletions(-)\n\ndiff --git a/builtin/commit-graph.c b/builtin/commit-graph.c\nindex 64eccde314..57863619b7 100644\n--- a/builtin/commit-graph.c\n+++ b/builtin/commit-graph.c\n@@ -213,8 +213,10 @@ static int graph_write(int argc, const char **argv)\n \n \t\tif (opts.stdin_packs)\n \t\t\tpack_indexes = &lines;\n-\t\tif (opts.stdin_commits)\n+\t\tif (opts.stdin_commits) {\n \t\t\tcommit_hex = &lines;\n+\t\t\tflags |= COMMIT_GRAPH_WRITE_CHECK_OIDS;\n+\t\t}\n \n \t\tUNLEAK(buf);\n \t}\ndiff --git a/commit-graph.c b/commit-graph.c\nindex 04324a4648..821900675b 100644\n--- a/commit-graph.c\n+++ b/commit-graph.c\n@@ -782,7 +782,8 @@ struct write_commit_graph_context {\n \n \tunsigned append:1,\n \t\t report_progress:1,\n-\t\t split:1;\n+\t\t split:1,\n+\t\t check_oids:1;\n \n \tconst struct split_commit_graph_opts *split_opts;\n };\n@@ -1193,8 +1194,8 @@ static int fill_oids_from_packs(struct write_commit_graph_context *ctx,\n \treturn 0;\n }\n \n-static void fill_oids_from_commit_hex(struct write_commit_graph_context *ctx,\n-\t\t\t\t struct string_list *commit_hex)\n+static int fill_oids_from_commit_hex(struct write_commit_graph_context *ctx,\n+\t\t\t\t struct string_list *commit_hex)\n {\n \tuint32_t i;\n \tstruct strbuf progress_title = STRBUF_INIT;\n@@ -1215,20 +1216,21 @@ static void fill_oids_from_commit_hex(struct write_commit_graph_context *ctx,\n \t\tstruct commit *result;\n \n \t\tdisplay_progress(ctx->progress, i + 1);\n-\t\tif (commit_hex->items[i].string &&\n-\t\t parse_oid_hex(commit_hex->items[i].string, &oid, &end))\n-\t\t\tcontinue;\n-\n-\t\tresult = lookup_commit_reference_gently(ctx->r, &oid, 1);\n-\n-\t\tif (result) {\n+\t\tif (!parse_oid_hex(commit_hex->items[i].string, &oid, &end) &&\n+\t\t (result = lookup_commit_reference_gently(ctx->r, &oid, 1))) {\n \t\t\tALLOC_GROW(ctx->oids.list, ctx->oids.nr + 1, ctx->oids.alloc);\n \t\t\toidcpy(&ctx->oids.list[ctx->oids.nr], &(result->object.oid));\n \t\t\tctx->oids.nr++;\n+\t\t} else if (ctx->check_oids) {\n+\t\t\terror(_(\"invalid commit object id: %s\"),\n+\t\t\t commit_hex->items[i].string);\n+\t\t\treturn -1;\n \t\t}\n \t}\n \tstop_progress(&ctx->progress);\n \tstrbuf_release(&progress_title);\n+\n+\treturn 0;\n }\n \n static void fill_oids_from_all_packs(struct write_commit_graph_context *ctx)\n@@ -1775,6 +1777,7 @@ int write_commit_graph(const char *obj_dir,\n \tctx->append = flags & COMMIT_GRAPH_WRITE_APPEND ? 1 : 0;\n \tctx->report_progress = flags & COMMIT_GRAPH_WRITE_PROGRESS ? 1 : 0;\n \tctx->split = flags & COMMIT_GRAPH_WRITE_SPLIT ? 1 : 0;\n+\tctx->check_oids = flags & COMMIT_GRAPH_WRITE_CHECK_OIDS ? 1 : 0;\n \tctx->split_opts = split_opts;\n \n \tif (ctx->split) {\n@@ -1829,8 +1832,10 @@ int write_commit_graph(const char *obj_dir,\n \t\t\tgoto cleanup;\n \t}\n \n-\tif (commit_hex)\n-\t\tfill_oids_from_commit_hex(ctx, commit_hex);\n+\tif (commit_hex) {\n+\t\tif ((res = fill_oids_from_commit_hex(ctx, commit_hex)))\n+\t\t\tgoto cleanup;\n+\t}\n \n \tif (!pack_indexes && !commit_hex)\n \t\tfill_oids_from_all_packs(ctx);\ndiff --git a/commit-graph.h b/commit-graph.h\nindex ae4db87fb5..486e64e591 100644\n--- a/commit-graph.h\n+++ b/commit-graph.h\n@@ -74,7 +74,9 @@ int generation_numbers_enabled(struct repository *r);\n enum commit_graph_write_flags {\n \tCOMMIT_GRAPH_WRITE_APPEND = (1 << 0),\n \tCOMMIT_GRAPH_WRITE_PROGRESS = (1 << 1),\n-\tCOMMIT_GRAPH_WRITE_SPLIT = (1 << 2)\n+\tCOMMIT_GRAPH_WRITE_SPLIT = (1 << 2),\n+\t/* Make sure that each OID in the input is a valid commit OID. */\n+\tCOMMIT_GRAPH_WRITE_CHECK_OIDS = (1 << 3)\n };\n \n struct split_commit_graph_opts {\ndiff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh\nindex 4391007f4c..ab3eccf0fa 100755\n--- a/t/t5318-commit-graph.sh\n+++ b/t/t5318-commit-graph.sh\n@@ -23,7 +23,7 @@ test_expect_success 'write graph with no packs' '\n \ttest_path_is_missing info/commit-graph\n '\n \n-test_expect_success 'close with correct error on bad input' '\n+test_expect_success 'exit with correct error on bad input to --stdin-packs' '\n \tcd \"$TRASH_DIRECTORY/full\" &&\n \techo doesnotexist >in &&\n \ttest_expect_code 1 git commit-graph write --stdin-packs stderr &&\n@@ -40,6 +40,15 @@ test_expect_success 'create commits and repack' '\n \tgit repack\n '\n \n+test_expect_success 'exit with correct error on bad input to --stdin-commits' '\n+\tcd \"$TRASH_DIRECTORY/full\" &&\n+\techo HEAD | test_expect_code 1 git commit-graph write --stdin-commits 2>stderr &&\n+\ttest_i18ngrep \"invalid commit object id\" stderr &&\n+\t# valid tree OID, but not a commit OID\n+\tgit rev-parse HEAD^{tree} | test_expect_code 1 git commit-graph write --stdin-commits 2>stderr &&\n+\ttest_i18ngrep \"invalid commit object id\" stderr\n+'\n+\n graph_git_two_modes() {\n \tgit -c core.commitGraph=true $1 >output\n \tgit -c core.commitGraph=false $1 >expect\n-- \n2.23.0.rc1.309.g896d8c5f5f\n\n"},{"id":"379932","messageId":"20190805101407.GB20404@szeder.dev","threadId":"51589","inReplyTo":"20190805080240.30892-1-szeder.dev@gmail.com","subject":"Re: [PATCH 0/3] commit-graph: error out on invalid commit oids in 'write --stdin-commits'","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2019-08-05T10:14:07Z","receivedAt":"2019-08-05T10:14:13Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Mon, Aug 05, 2019 at 10:02:37AM +0200, SZEDER Gábor wrote:\n> While 'git commit-graph write --stdin-commits' expects commit object\n> ids as input, it accepts and silently skips over any invalid commit\n> object ids, and still exits with success:\n> \n> $ echo not-a-commit-oid | git commit-graph write --stdin-commits\n> $ echo $?\n\nOh, messed up the copy-paste; this prints 0.\n\n> $ ls -l .git/objects/info/commit-graph\n> ls: cannot access '.git/objects/info/commit-graph': No such file or directory\n"},{"id":"379936","messageId":"ecdd6c01-5ee2-6869-4eb3-1eb45f4c335c@gmail.com","threadId":"51589","inReplyTo":"20190805080240.30892-4-szeder.dev@gmail.com","subject":"Re: [PATCH 3/3] commit-graph: error out on invalid commit oids in 'write --stdin-commits'","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2019-08-05T13:57:19Z","receivedAt":"2019-08-05T13:57:23Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 8/5/2019 4:02 AM, SZEDER Gábor wrote:\n> While 'git commit-graph write --stdin-commits' expects commit object\n> ids as input, it accepts and silently skips over any invalid commit\n> object ids, and still exits with success:\n> \n> # nonsense\n> $ echo not-a-commit-oid | git commit-graph write --stdin-commits\n> $ echo $?\n> 0\n> # sometimes I forgot that refs are not good...\n> $ echo HEAD | git commit-graph write --stdin-commits\n> $ echo $?\n> 0\n> # valid tree OID, but not a commit OID\n> $ git rev-parse HEAD^{tree} | git commit-graph write --stdin-commits\n> $ echo $?\n> 0\n> $ ls -l .git/objects/info/commit-graph\n> ls: cannot access '.git/objects/info/commit-graph': No such file or directory\n> \n> Check that all input records are indeed valid commit object ids and\n> return with error otherwise, the same way '--stdin-packs' handles\n> invalid input; see e103f7276f (commit-graph: return with errors during\n> write, 2019-06-12).\n\nConsistency is good. We should definitely make these modes match.\n\n> Note that it should only return with error when encountering an\n> invalid commit object id coming from standard input. However,\n> '--reachable' uses the same code path to process object ids pointed to\n> by all refs, and that includes tag object ids as well, which should\n> still be skipped over. Therefore add a new flag to 'enum\n> commit_graph_write_flags' and a corresponding field to 'struct\n> write_commit_graph_context', so we can differentiate between those two\n> cases.\n\nThank you for the care here.\n\n[snip]\n> @@ -1215,20 +1216,21 @@ static void fill_oids_from_commit_hex(struct write_commit_graph_context *ctx,\n> \t\tstruct commit *result;\n> \n> \t\tdisplay_progress(ctx->progress, i + 1);\n> -\t\tif (commit_hex->items[i].string &&\n> -\t\t parse_oid_hex(commit_hex->items[i].string, &oid, &end))\n> -\t\t\tcontinue;\n> -\n> -\t\tresult = lookup_commit_reference_gently(ctx->r, &oid, 1);\n> -\n> -\t\tif (result) {\n> +\t\tif (!parse_oid_hex(commit_hex->items[i].string, &oid, &end) &&\n> +\t\t (result = lookup_commit_reference_gently(ctx->r, &oid, 1))) {\n> \t\t\tALLOC_GROW(ctx->oids.list, ctx->oids.nr + 1, ctx->oids.alloc);\n> \t\t\toidcpy(&ctx->oids.list[ctx->oids.nr], &(result->object.oid));\n> \t\t\tctx->oids.nr++;\n> +\t\t} else if (ctx->check_oids) {\n> +\t\t\terror(_(\"invalid commit object id: %s\"),\n> +\t\t\t commit_hex->items[i].string);\n> +\t\t\treturn -1;\n> \t\t}\n> \t}\n> \tstop_progress(&ctx->progress);\n> \tstrbuf_release(&progress_title);\n> +\n> +\treturn 0;\n> }\n\nThis is the critical bit. I notice that you are not checking commit_hex->items[i].string\nfor NULL, but it should never be NULL here anyway.\n\n> @@ -1775,6 +1777,7 @@ int write_commit_graph(const char *obj_dir,\n> \tctx->append = flags & COMMIT_GRAPH_WRITE_APPEND ? 1 : 0;\n> \tctx->report_progress = flags & COMMIT_GRAPH_WRITE_PROGRESS ? 1 : 0;\n> \tctx->split = flags & COMMIT_GRAPH_WRITE_SPLIT ? 1 : 0;\n> +\tctx->check_oids = flags & COMMIT_GRAPH_WRITE_CHECK_OIDS ? 1 : 0;\n> \tctx->split_opts = split_opts;\n\nUsing the enum for the function and the bitfield for internal logic matches the\nexisting pattern. Thanks.\n\n> @@ -1829,8 +1832,10 @@ int write_commit_graph(const char *obj_dir,\n> \t\t\tgoto cleanup;\n> \t}\n> \n> -\tif (commit_hex)\n> -\t\tfill_oids_from_commit_hex(ctx, commit_hex);\n> +\tif (commit_hex) {\n> +\t\tif ((res = fill_oids_from_commit_hex(ctx, commit_hex)))\n> +\t\t\tgoto cleanup;\n> +\t}\n\nAnd this links the low-level error to a return code.\n\nThanks for this! The changes here look good and justify the two cleanup\npatches.\n\n-Stolee\n"},{"id":"379943","messageId":"20190805175704.GC20404@szeder.dev","threadId":"51589","inReplyTo":"ecdd6c01-5ee2-6869-4eb3-1eb45f4c335c@gmail.com","subject":"Re: [PATCH 3/3] commit-graph: error out on invalid commit oids in 'write --stdin-commits'","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2019-08-05T17:57:04Z","receivedAt":"2019-08-05T17:57:11Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Mon, Aug 05, 2019 at 09:57:19AM -0400, Derrick Stolee wrote:\n> On 8/5/2019 4:02 AM, SZEDER Gábor wrote:\n> > While 'git commit-graph write --stdin-commits' expects commit object\n> > ids as input, it accepts and silently skips over any invalid commit\n> > object ids, and still exits with success:\n> > \n> > # nonsense\n> > $ echo not-a-commit-oid | git commit-graph write --stdin-commits\n> > $ echo $?\n> > 0\n> > # sometimes I forgot that refs are not good...\n> > $ echo HEAD | git commit-graph write --stdin-commits\n> > $ echo $?\n> > 0\n> > # valid tree OID, but not a commit OID\n> > $ git rev-parse HEAD^{tree} | git commit-graph write --stdin-commits\n> > $ echo $?\n> > 0\n> > $ ls -l .git/objects/info/commit-graph\n> > ls: cannot access '.git/objects/info/commit-graph': No such file or directory\n> > \n> > Check that all input records are indeed valid commit object ids and\n> > return with error otherwise, the same way '--stdin-packs' handles\n> > invalid input; see e103f7276f (commit-graph: return with errors during\n> > write, 2019-06-12).\n> \n> Consistency is good. We should definitely make these modes match.\n\nI was also wondering whether it would be worth accepting refs as well,\neither as DWIMery or only when a '--revs' option is given (similar to\n'git pack-objects --revs'). Dunno, I'm a bit hesitant about always\naccepting refs as a DWIMery, this is plumbing after all. And I don't\nreally care whether I correct my bogus command by replacing 'echo'\nwith 'git rev-parse' or by adding a '--revs' argument; the important\nthing is that the command should tell me that I gave it junk. And\nthat would be a new feature, while this patch is a bugfix IMO.\n\n> > Note that it should only return with error when encountering an\n> > invalid commit object id coming from standard input. However,\n> > '--reachable' uses the same code path to process object ids pointed to\n> > by all refs, and that includes tag object ids as well, which should\n> > still be skipped over. Therefore add a new flag to 'enum\n> > commit_graph_write_flags' and a corresponding field to 'struct\n> > write_commit_graph_context', so we can differentiate between those two\n> > cases.\n> \n> Thank you for the care here.\n\nWell, to be honest, I wasn't careful... :) but running the test suite\nwith GIT_TEST_COMMIT_GRAPH=1 resulted in about a dozen failed test\nscripts that traced back to this.\n\n"},{"id":"394681","messageId":"20200403183057.GA659224@coredump.intra.peff.net","threadId":"51589","inReplyTo":"20190805080240.30892-4-szeder.dev@gmail.com","subject":"Re: [PATCH 3/3] commit-graph: error out on invalid commit oids in 'write --stdin-commits'","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-04-03T18:30:57Z","receivedAt":"2020-04-03T18:30:59Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Aug 05, 2019 at 10:02:40AM +0200, SZEDER Gábor wrote:\n\n> While 'git commit-graph write --stdin-commits' expects commit object\n> ids as input, it accepts and silently skips over any invalid commit\n> object ids, and still exits with success:\n> \n> # nonsense\n> $ echo not-a-commit-oid | git commit-graph write --stdin-commits\n> $ echo $?\n> 0\n> # sometimes I forgot that refs are not good...\n> $ echo HEAD | git commit-graph write --stdin-commits\n> $ echo $?\n> 0\n> # valid tree OID, but not a commit OID\n> $ git rev-parse HEAD^{tree} | git commit-graph write --stdin-commits\n> $ echo $?\n> 0\n> $ ls -l .git/objects/info/commit-graph\n> ls: cannot access '.git/objects/info/commit-graph': No such file or directory\n> \n> Check that all input records are indeed valid commit object ids and\n> return with error otherwise, the same way '--stdin-packs' handles\n> invalid input; see e103f7276f (commit-graph: return with errors during\n> write, 2019-06-12).\n\nCan you explain more why the old behavior is a problem? For reasons (see\nbelow), we want to do something like:\n\n git for-each-ref --format='%(objectname)' |\n git commit-graph write --stdin-commits\n\nIn v2.23 and earlier, that worked exactly like --reachable, but now it\nwill blow up if there are any refs that point to a non-commit (e.g., a\ntag of a blob).\n\nIt can be worked around by asking for %(objecttype) and %(*objecttype)\nand grepping the result, but that's awkward and much less efficient\n(especially if you have a lot of annotated tags, as we may have to open\nand parse each one).\n\nNow obviously you could just use --reachable for the code above. But\nhere are two plausible cases where you might not want to do that:\n\n - you're limiting the graph to only a subset of refs (e.g., you want to\n graph refs/heads/ and refs/tags, but not refs/some-other-weird-area/).\n\n - you're generating an incremental graph update. You know somehow that\n a few refs were updated, and you want to feed those tips to generate\n the incremental, but not the rest of the refs (not because it would\n be wrong to do so, but in the name of keeping it O(size of change)\n and not O(number of refs in the repo).\n\nThe latter is the actual case that bit us. I suppose one could do\nsomething like:\n\n git rev-list --no-walk On Mon, Aug 05, 2019 at 10:02:40AM +0200, SZEDER Gábor wrote:\n>\n> > While 'git commit-graph write --stdin-commits' expects commit object\n> > ids as input, it accepts and silently skips over any invalid commit\n> > object ids, and still exits with success:\n> >\n> > # nonsense\n> > $ echo not-a-commit-oid | git commit-graph write --stdin-commits\n> > $ echo $?\n> > 0\n> > # sometimes I forgot that refs are not good...\n> > $ echo HEAD | git commit-graph write --stdin-commits\n> > $ echo $?\n> > 0\n> > # valid tree OID, but not a commit OID\n> > $ git rev-parse HEAD^{tree} | git commit-graph write --stdin-commits\n> > $ echo $?\n> > 0\n> > $ ls -l .git/objects/info/commit-graph\n> > ls: cannot access '.git/objects/info/commit-graph': No such file or directory\n> >\n> > Check that all input records are indeed valid commit object ids and\n> > return with error otherwise, the same way '--stdin-packs' handles\n> > invalid input; see e103f7276f (commit-graph: return with errors during\n> > write, 2019-06-12).\n>\n> Can you explain more why the old behavior is a problem? For reasons (see\n> below), we want to do something like:\n>\n> git for-each-ref --format='%(objectname)' |\n> git commit-graph write --stdin-commits\n>\n> In v2.23 and earlier, that worked exactly like --reachable, but now it\n> will blow up if there are any refs that point to a non-commit (e.g., a\n> tag of a blob).\n>\n> It can be worked around by asking for %(objecttype) and %(*objecttype)\n> and grepping the result, but that's awkward and much less efficient\n> (especially if you have a lot of annotated tags, as we may have to open\n> and parse each one).\n>\n> Now obviously you could just use --reachable for the code above. But\n> here are two plausible cases where you might not want to do that:\n>\n> - you're limiting the graph to only a subset of refs (e.g., you want to\n> graph refs/heads/ and refs/tags, but not refs/some-other-weird-area/).\n>\n> - you're generating an incremental graph update. You know somehow that\n> a few refs were updated, and you want to feed those tips to generate\n> the incremental, but not the rest of the refs (not because it would\n> be wrong to do so, but in the name of keeping it O(size of change)\n> and not O(number of refs in the repo).\n>\n> The latter is the actual case that bit us. I suppose one could do\n> something like:\n>\n> git rev-list --no-walk git commit-graph write --stdin-commits\n>\n> to use rev-list as a filter, but that feels kind of baroque.\n>\n> Normally I'm in favor of more error checking instead of less, but in\n> this case it feels like it's putting scripted use at a disadvantage\n> versus the internal code (e.g., the auto-write for git-fetch uses the\n> \"--reachable\" semantics for its internal invocation).\n\nFor what it's worth, (and in case it wasn't obvious) this came about\nbecause we feed '--stdin-commits' at GitHub, and observed exactly this\nerror case. I wasn't sure what approach would be more palatable, so I\nprepared both in my fork at https://github.com/ttaylorr/git:\n\n - Branch 'tb/commit-graph-dont-check-oids' drops this checking\n entirely.\n\n - Branch 'tb/commit-graph-check-oids-option' adds a\n '--[no-]check-oids', in case that this is generally desirable\n behavior, by offering an opt-out of this OID checking.\n\nPlease let me know if you find either of these to be good options, and\nI'll happily send one of them to the list. Thanks.\n\n> -Peff\n>\n> PS As an aside, I think the internal git-fetch write could benefit from\n> this same trick: feed the set of newly-stored ref tips to the\n> commit-graph machinery, rather than using for_each_ref().\n\nThanks,\nTaylor\n"},{"id":"394690","messageId":"20200403193842.GA7859@szeder.dev","threadId":"51589","inReplyTo":"20200403184933.GA57202@syl.local","subject":"Re: [PATCH 3/3] commit-graph: error out on invalid commit oids in 'write --stdin-commits'","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2020-04-03T19:38:42Z","receivedAt":"2020-04-03T19:38:51Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Fri, Apr 03, 2020 at 12:49:33PM -0600, Taylor Blau wrote:\n> On Fri, Apr 03, 2020 at 02:30:57PM -0400, Jeff King wrote:\n> > On Mon, Aug 05, 2019 at 10:02:40AM +0200, SZEDER Gábor wrote:\n> >\n> > > While 'git commit-graph write --stdin-commits' expects commit object\n> > > ids as input, it accepts and silently skips over any invalid commit\n> > > object ids, and still exits with success:\n> > >\n> > > # nonsense\n> > > $ echo not-a-commit-oid | git commit-graph write --stdin-commits\n> > > $ echo $?\n> > > 0\n> > > # sometimes I forgot that refs are not good...\n> > > $ echo HEAD | git commit-graph write --stdin-commits\n> > > $ echo $?\n> > > 0\n> > > # valid tree OID, but not a commit OID\n> > > $ git rev-parse HEAD^{tree} | git commit-graph write --stdin-commits\n> > > $ echo $?\n> > > 0\n> > > $ ls -l .git/objects/info/commit-graph\n> > > ls: cannot access '.git/objects/info/commit-graph': No such file or directory\n> > >\n> > > Check that all input records are indeed valid commit object ids and\n> > > return with error otherwise, the same way '--stdin-packs' handles\n> > > invalid input; see e103f7276f (commit-graph: return with errors during\n> > > write, 2019-06-12).\n> >\n> > Can you explain more why the old behavior is a problem?\n\nBecause when I do:\n\n # sometimes I forgot that refs are not good...\n $ echo HEAD | git commit-graph write --stdin-commits\n\nthen I get _nothing_: neither an error, nor a commit-graph.\n\n> > For reasons (see\n> > below), we want to do something like:\n> >\n> > git for-each-ref --format='%(objectname)' |\n> > git commit-graph write --stdin-commits\n> >\n> > In v2.23 and earlier, that worked exactly like --reachable, but now it\n> > will blow up if there are any refs that point to a non-commit (e.g., a\n> > tag of a blob).\n> >\n> > It can be worked around by asking for %(objecttype) and %(*objecttype)\n> > and grepping the result, but that's awkward and much less efficient\n> > (especially if you have a lot of annotated tags, as we may have to open\n> > and parse each one).\n> >\n> > Now obviously you could just use --reachable for the code above. But\n> > here are two plausible cases where you might not want to do that:\n> >\n> > - you're limiting the graph to only a subset of refs (e.g., you want to\n> > graph refs/heads/ and refs/tags, but not refs/some-other-weird-area/).\n> >\n> > - you're generating an incremental graph update. You know somehow that\n> > a few refs were updated, and you want to feed those tips to generate\n> > the incremental, but not the rest of the refs (not because it would\n> > be wrong to do so, but in the name of keeping it O(size of change)\n> > and not O(number of refs in the repo).\n> >\n> > The latter is the actual case that bit us. I suppose one could do\n> > something like:\n> >\n> > git rev-list --no-walk > git commit-graph write --stdin-commits\n> >\n> > to use rev-list as a filter, but that feels kind of baroque.\n> >\n> > Normally I'm in favor of more error checking instead of less, but in\n> > this case it feels like it's putting scripted use at a disadvantage\n> > versus the internal code (e.g., the auto-write for git-fetch uses the\n> > \"--reachable\" semantics for its internal invocation).\n> \n> For what it's worth, (and in case it wasn't obvious) this came about\n> because we feed '--stdin-commits' at GitHub, and observed exactly this\n> error case. I wasn't sure what approach would be more palatable, so I\n> prepared both in my fork at https://github.com/ttaylorr/git:\n> \n> - Branch 'tb/commit-graph-dont-check-oids' drops this checking\n> entirely.\n\nOh, no :)\n\n> - Branch 'tb/commit-graph-check-oids-option' adds a\n> '--[no-]check-oids', in case that this is generally desirable\n> behavior, by offering an opt-out of this OID checking.\n> \n> Please let me know if you find either of these to be good options, and\n> I'll happily send one of them to the list. Thanks.\n\nOr introduce 'git commit-graph write --stdin-refs'? Or teach\n'--stdin-commits' to DWIM and accept and parse refs? Though the\nquestion still remains what to do with refs that can't be peeled back\nto commits\n\n> > -Peff\n> >\n> > PS As an aside, I think the internal git-fetch write could benefit from\n> > this same trick: feed the set of newly-stored ref tips to the\n> > commit-graph machinery, rather than using for_each_ref().\n> \n> Thanks,\n> Taylor\n"},{"id":"394691","messageId":"xmqqd08oz70o.fsf@gitster.c.googlers.com","threadId":"51589","inReplyTo":"20200403184933.GA57202@syl.local","subject":"Re: [PATCH 3/3] commit-graph: error out on invalid commit oids in 'write --stdin-commits'","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-04-03T19:47:03Z","receivedAt":"2020-04-03T19:47:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Taylor Blau writes:\n\n> On Fri, Apr 03, 2020 at 02:30:57PM -0400, Jeff King wrote:\n>> On Mon, Aug 05, 2019 at 10:02:40AM +0200, SZEDER Gábor wrote:\n>>\n>> > Check that all input records are indeed valid commit object ids and\n>> > return with error otherwise, the same way '--stdin-packs' handles\n>> > invalid input; see e103f7276f (commit-graph: return with errors during\n>> > write, 2019-06-12).\n>>\n>> Can you explain more why the old behavior is a problem? For reasons (see\n>> below), we want to do something like:\n>>\n>> git for-each-ref --format='%(objectname)' |\n>> git commit-graph write --stdin-commits\n>> ...\n>> - you're generating an incremental graph update. You know somehow that\n>> a few refs were updated, and you want to feed those tips to generate\n>> the incremental, but not the rest of the refs (not because it would\n>> be wrong to do so, but in the name of keeping it O(size of change)\n>> and not O(number of refs in the repo).\n>> ...\n>> Normally I'm in favor of more error checking instead of less, but in\n>> this case it feels like it's putting scripted use at a disadvantage\n>> versus the internal code (e.g., the auto-write for git-fetch uses the\n>> \"--reachable\" semantics for its internal invocation).\n\nI think the \"incremental from the tip of refs\" is a valid and useful\nuse case. I am not sure if the rationale given in the original to\ncompare the (stricter) check done here and what e103f7276f did\n(which does not seem to get any input, valid or invalid, from the\nend users) was a meaningful comparison, and regardless of Gábor's\nanswer to Peff's question, I think we should have an easy way to let\nthe machinery itself filter non-commit, so \"--[no-]check-oids\" that\noptionally turns the stricter checking off would be an easy way out.\nI do not have a strong opinion on which way the default should be\n(at least not yet), but given that this was already in two major\nreleases ago, I'd assume that stricter-by-default-with-escape-hatch\nwould be the way to go (at least for now).\n\n> For what it's worth, (and in case it wasn't obvious) this came about\n> because we feed '--stdin-commits' at GitHub, and observed exactly this\n> error case. I wasn't sure what approach would be more palatable, so I\n> prepared both in my fork at https://github.com/ttaylorr/git:\n>\n> - Branch 'tb/commit-graph-dont-check-oids' drops this checking\n> entirely.\n>\n> - Branch 'tb/commit-graph-check-oids-option' adds a\n> '--[no-]check-oids', in case that this is generally desirable\n> behavior, by offering an opt-out of this OID checking.\n\nThanks.\n"},{"id":"394692","messageId":"20200403195103.GA663412@coredump.intra.peff.net","threadId":"51589","inReplyTo":"20200403193842.GA7859@szeder.dev","subject":"Re: [PATCH 3/3] commit-graph: error out on invalid commit oids in 'write --stdin-commits'","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-04-03T19:51:03Z","receivedAt":"2020-04-03T19:51:07Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Apr 03, 2020 at 09:38:42PM +0200, SZEDER Gábor wrote:\n\n> > > Can you explain more why the old behavior is a problem?\n> \n> Because when I do:\n> \n> # sometimes I forgot that refs are not good...\n> $ echo HEAD | git commit-graph write --stdin-commits\n> \n> then I get _nothing_: neither an error, nor a commit-graph.\n\nOK, that makes more sense: it's an input format error, because we only\ntake hex oids.\n\nDo you care about complaining about:\n\n git rev-parse HEAD^{tree} | git commit-graph write --stdin-commits\n\n? That's the case that's much more interesting, I think.\n\n> Or introduce 'git commit-graph write --stdin-refs'? Or teach\n> '--stdin-commits' to DWIM and accept and parse refs? Though the\n> question still remains what to do with refs that can't be peeled back\n> to commits\n\nRight. I think there are two orthogonal questions:\n\n - whether to resolve arbitrary names to objects and how to handle such\n input if we don't\n\n - what to do with an oid (whether given as hex or resolved from a\n name) that isn't a commit-ish\n\n-Peff\n"},{"id":"394695","messageId":"20200403195545.GA58262@syl.local","threadId":"51589","inReplyTo":"20200403193842.GA7859@szeder.dev","subject":"Re: [PATCH 3/3] commit-graph: error out on invalid commit oids in 'write --stdin-commits'","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-04-03T19:55:45Z","receivedAt":"2020-04-03T19:55:52Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Fri, Apr 03, 2020 at 09:38:42PM +0200, SZEDER Gábor wrote:\n> On Fri, Apr 03, 2020 at 12:49:33PM -0600, Taylor Blau wrote:\n> > On Fri, Apr 03, 2020 at 02:30:57PM -0400, Jeff King wrote:\n> > > On Mon, Aug 05, 2019 at 10:02:40AM +0200, SZEDER Gábor wrote:\n> > >\n> > > > While 'git commit-graph write --stdin-commits' expects commit object\n> > > > ids as input, it accepts and silently skips over any invalid commit\n> > > > object ids, and still exits with success:\n> > > >\n> > > > # nonsense\n> > > > $ echo not-a-commit-oid | git commit-graph write --stdin-commits\n> > > > $ echo $?\n> > > > 0\n> > > > # sometimes I forgot that refs are not good...\n> > > > $ echo HEAD | git commit-graph write --stdin-commits\n> > > > $ echo $?\n> > > > 0\n> > > > # valid tree OID, but not a commit OID\n> > > > $ git rev-parse HEAD^{tree} | git commit-graph write --stdin-commits\n> > > > $ echo $?\n> > > > 0\n> > > > $ ls -l .git/objects/info/commit-graph\n> > > > ls: cannot access '.git/objects/info/commit-graph': No such file or directory\n> > > >\n> > > > Check that all input records are indeed valid commit object ids and\n> > > > return with error otherwise, the same way '--stdin-packs' handles\n> > > > invalid input; see e103f7276f (commit-graph: return with errors during\n> > > > write, 2019-06-12).\n> > >\n> > > Can you explain more why the old behavior is a problem?\n>\n> Because when I do:\n>\n> # sometimes I forgot that refs are not good...\n> $ echo HEAD | git commit-graph write --stdin-commits\n>\n> then I get _nothing_: neither an error, nor a commit-graph.\n>\n> > > For reasons (see\n> > > below), we want to do something like:\n> > >\n> > > git for-each-ref --format='%(objectname)' |\n> > > git commit-graph write --stdin-commits\n> > >\n> > > In v2.23 and earlier, that worked exactly like --reachable, but now it\n> > > will blow up if there are any refs that point to a non-commit (e.g., a\n> > > tag of a blob).\n> > >\n> > > It can be worked around by asking for %(objecttype) and %(*objecttype)\n> > > and grepping the result, but that's awkward and much less efficient\n> > > (especially if you have a lot of annotated tags, as we may have to open\n> > > and parse each one).\n> > >\n> > > Now obviously you could just use --reachable for the code above. But\n> > > here are two plausible cases where you might not want to do that:\n> > >\n> > > - you're limiting the graph to only a subset of refs (e.g., you want to\n> > > graph refs/heads/ and refs/tags, but not refs/some-other-weird-area/).\n> > >\n> > > - you're generating an incremental graph update. You know somehow that\n> > > a few refs were updated, and you want to feed those tips to generate\n> > > the incremental, but not the rest of the refs (not because it would\n> > > be wrong to do so, but in the name of keeping it O(size of change)\n> > > and not O(number of refs in the repo).\n> > >\n> > > The latter is the actual case that bit us. I suppose one could do\n> > > something like:\n> > >\n> > > git rev-list --no-walk > > git commit-graph write --stdin-commits\n> > >\n> > > to use rev-list as a filter, but that feels kind of baroque.\n> > >\n> > > Normally I'm in favor of more error checking instead of less, but in\n> > > this case it feels like it's putting scripted use at a disadvantage\n> > > versus the internal code (e.g., the auto-write for git-fetch uses the\n> > > \"--reachable\" semantics for its internal invocation).\n> >\n> > For what it's worth, (and in case it wasn't obvious) this came about\n> > because we feed '--stdin-commits' at GitHub, and observed exactly this\n> > error case. I wasn't sure what approach would be more palatable, so I\n> > prepared both in my fork at https://github.com/ttaylorr/git:\n> >\n> > - Branch 'tb/commit-graph-dont-check-oids' drops this checking\n> > entirely.\n>\n> Oh, no :)\n\nHeh, I figured as much when Peff pointed me towards this thread ;-).\n\n> > - Branch 'tb/commit-graph-check-oids-option' adds a\n> > '--[no-]check-oids', in case that this is generally desirable\n> > behavior, by offering an opt-out of this OID checking.\n> >\n> > Please let me know if you find either of these to be good options, and\n> > I'll happily send one of them to the list. Thanks.\n>\n> Or introduce 'git commit-graph write --stdin-refs'? Or teach\n> '--stdin-commits' to DWIM and accept and parse refs? Though the\n> question still remains what to do with refs that can't be peeled back\n> to commits\n\nHow would '--stdin-refs' behave differently? I assume that it'd handle\ngoing from 'refs/heads/blah' to an OID, but I think that's only kicking\nthe problem a little further down the road. Of course, you note the same\nproblem which is: what do we do when we can't resolve down to a commit\nobject.\n\nI think that it seems the '--[no-]check-oids' may be the best of both\nworlds, which allows for (mostly ;)) easy scripting around 'git\nfor-each-ref'.\n\n> > > -Peff\n> > >\n> > > PS As an aside, I think the internal git-fetch write could benefit from\n> > > this same trick: feed the set of newly-stored ref tips to the\n> > > commit-graph machinery, rather than using for_each_ref().\n> >\n> > Thanks,\n> > Taylor\nThanks,\nTaylor\n"},{"id":"394697","messageId":"20200403195736.GB58262@syl.local","threadId":"51589","inReplyTo":"xmqqd08oz70o.fsf@gitster.c.googlers.com","subject":"Re: [PATCH 3/3] commit-graph: error out on invalid commit oids in 'write --stdin-commits'","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-04-03T19:57:36Z","receivedAt":"2020-04-03T19:57:41Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Fri, Apr 03, 2020 at 12:47:03PM -0700, Junio C Hamano wrote:\n> Taylor Blau writes:\n>\n> > On Fri, Apr 03, 2020 at 02:30:57PM -0400, Jeff King wrote:\n> >> On Mon, Aug 05, 2019 at 10:02:40AM +0200, SZEDER Gábor wrote:\n> >>\n> >> > Check that all input records are indeed valid commit object ids and\n> >> > return with error otherwise, the same way '--stdin-packs' handles\n> >> > invalid input; see e103f7276f (commit-graph: return with errors during\n> >> > write, 2019-06-12).\n> >>\n> >> Can you explain more why the old behavior is a problem? For reasons (see\n> >> below), we want to do something like:\n> >>\n> >> git for-each-ref --format='%(objectname)' |\n> >> git commit-graph write --stdin-commits\n> >> ...\n> >> - you're generating an incremental graph update. You know somehow that\n> >> a few refs were updated, and you want to feed those tips to generate\n> >> the incremental, but not the rest of the refs (not because it would\n> >> be wrong to do so, but in the name of keeping it O(size of change)\n> >> and not O(number of refs in the repo).\n> >> ...\n> >> Normally I'm in favor of more error checking instead of less, but in\n> >> this case it feels like it's putting scripted use at a disadvantage\n> >> versus the internal code (e.g., the auto-write for git-fetch uses the\n> >> \"--reachable\" semantics for its internal invocation).\n>\n> I think the \"incremental from the tip of refs\" is a valid and useful\n> use case. I am not sure if the rationale given in the original to\n> compare the (stricter) check done here and what e103f7276f did\n> (which does not seem to get any input, valid or invalid, from the\n> end users) was a meaningful comparison, and regardless of Gábor's\n> answer to Peff's question, I think we should have an easy way to let\n> the machinery itself filter non-commit, so \"--[no-]check-oids\" that\n> optionally turns the stricter checking off would be an easy way out.\n\nThanks. I think that this is probably my preference, too. I'll send this\nas a patch to the list shortly...\n\n> I do not have a strong opinion on which way the default should be\n> (at least not yet), but given that this was already in two major\n> releases ago, I'd assume that stricter-by-default-with-escape-hatch\n> would be the way to go (at least for now).\n\nYeah. I think the nice thing about those patches is that we don't have\nto decide that right away.\n\n> > For what it's worth, (and in case it wasn't obvious) this came about\n> > because we feed '--stdin-commits' at GitHub, and observed exactly this\n> > error case. I wasn't sure what approach would be more palatable, so I\n> > prepared both in my fork at https://github.com/ttaylorr/git:\n> >\n> > - Branch 'tb/commit-graph-dont-check-oids' drops this checking\n> > entirely.\n> >\n> > - Branch 'tb/commit-graph-check-oids-option' adds a\n> > '--[no-]check-oids', in case that this is generally desirable\n> > behavior, by offering an opt-out of this OID checking.\n>\n> Thanks.\n\nLikewise :).\n\nThanks,\nTaylor\n"},{"id":"394700","messageId":"20200403204013.GC7859@szeder.dev","threadId":"51589","inReplyTo":"20200403195103.GA663412@coredump.intra.peff.net","subject":"Re: [PATCH 3/3] commit-graph: error out on invalid commit oids in 'write --stdin-commits'","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2020-04-03T20:40:13Z","receivedAt":"2020-04-03T20:40:22Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Fri, Apr 03, 2020 at 03:51:03PM -0400, Jeff King wrote:\n> On Fri, Apr 03, 2020 at 09:38:42PM +0200, SZEDER Gábor wrote:\n> \n> > > > Can you explain more why the old behavior is a problem?\n> > \n> > Because when I do:\n> > \n> > # sometimes I forgot that refs are not good...\n> > $ echo HEAD | git commit-graph write --stdin-commits\n> > \n> > then I get _nothing_: neither an error, nor a commit-graph.\n> \n> OK, that makes more sense: it's an input format error, because we only\n> take hex oids.\n> \n> Do you care about complaining about:\n> \n> git rev-parse HEAD^{tree} | git commit-graph write --stdin-commits\n> \n> ? That's the case that's much more interesting, I think.\n\nHm, are you trying to go in the direction where '--stdin-commits'\nwould keep erroring out on any non-full-hex-oid, but would accept and\nsilently ignore any hex oids that are not commits (perhaps even when\nthere is no such object, dunno)? I think that would support the use\ncases you mentioned, while it would still save me when I do the 'echo\n' thing (somehow I regularly do that, remember doing it the day\nbefore yesterday!).\n\nI only mentioned the ^{tree} form in the commit message for the sake\nof completeness, i.e. to show various cases where the user would get\nneither error nor commit-graph.\n\n> > Or introduce 'git commit-graph write --stdin-refs'? Or teach\n> > '--stdin-commits' to DWIM and accept and parse refs? Though the\n> > question still remains what to do with refs that can't be peeled back\n> > to commits\n> \n> Right. I think there are two orthogonal questions:\n> \n> - whether to resolve arbitrary names to objects and how to handle such\n> input if we don't\n> \n> - what to do with an oid (whether given as hex or resolved from a\n> name) that isn't a commit-ish\n> \n> -Peff\n"},{"id":"394727","messageId":"20200403231021.GA672258@coredump.intra.peff.net","threadId":"51589","inReplyTo":"20200403204013.GC7859@szeder.dev","subject":"Re: [PATCH 3/3] commit-graph: error out on invalid commit oids in 'write --stdin-commits'","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-04-03T23:10:21Z","receivedAt":"2020-04-03T23:10:24Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Apr 03, 2020 at 10:40:13PM +0200, SZEDER Gábor wrote:\n\n> > Do you care about complaining about:\n> > \n> > git rev-parse HEAD^{tree} | git commit-graph write --stdin-commits\n> > \n> > ? That's the case that's much more interesting, I think.\n> \n> Hm, are you trying to go in the direction where '--stdin-commits'\n> would keep erroring out on any non-full-hex-oid, but would accept and\n> silently ignore any hex oids that are not commits (perhaps even when\n> there is no such object, dunno)? I think that would support the use\n> cases you mentioned, while it would still save me when I do the 'echo\n> ' thing (somehow I regularly do that, remember doing it the day\n> before yesterday!).\n\nYes, exactly. The case you care about and the case I care about are\ndifferent ones, so there's no inherent conflict between them.\n\n-Peff\n"},{"id":"395370","messageId":"20200413193934.GC63249@syl.local","threadId":"51589","inReplyTo":"20200403231021.GA672258@coredump.intra.peff.net","subject":"Re: [PATCH 3/3] commit-graph: error out on invalid commit oids in 'write --stdin-commits'","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-04-13T19:39:34Z","receivedAt":"2020-04-13T19:39:40Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Fri, Apr 03, 2020 at 07:10:21PM -0400, Jeff King wrote:\n> On Fri, Apr 03, 2020 at 10:40:13PM +0200, SZEDER Gábor wrote:\n>\n> > > Do you care about complaining about:\n> > >\n> > > git rev-parse HEAD^{tree} | git commit-graph write --stdin-commits\n> > >\n> > > ? That's the case that's much more interesting, I think.\n> >\n> > Hm, are you trying to go in the direction where '--stdin-commits'\n> > would keep erroring out on any non-full-hex-oid, but would accept and\n> > silently ignore any hex oids that are not commits (perhaps even when\n> > there is no such object, dunno)? I think that would support the use\n> > cases you mentioned, while it would still save me when I do the 'echo\n> > ' thing (somehow I regularly do that, remember doing it the day\n> > before yesterday!).\n>\n> Yes, exactly. The case you care about and the case I care about are\n> different ones, so there's no inherent conflict between them.\n\nI was looking back again at this today, and I think we need something\nmore or less like the following on top. I'll send it out later today or\nearly tomorrow...\n\ndiff --git a/commit-graph.c b/commit-graph.c\nindex 2d436907cd..58e7890590 100644\n--- a/commit-graph.c\n+++ b/commit-graph.c\n@@ -1221,17 +1221,24 @@ static int fill_oids_from_commit_hex(struct write_commit_graph_context *ctx,\n commit_hex->nr);\n }\n for (i = 0; i < commit_hex->nr; i++) {\n+ int ret;\n const char *end;\n struct object_id oid;\n struct commit *result;\n\n display_progress(ctx->progress, i + 1);\n- if (!parse_oid_hex(commit_hex->items[i].string, &oid, &end) &&\n- (result = lookup_commit_reference_gently(ctx->r, &oid, 1))) {\n+\n+ ret = parse_oid_hex(commit_hex->items[i].string, &oid, &end);\n+ if (!ret) {\n+ result = lookup_commit_reference_gently(ctx->r, &oid, 1);\n+ if (result) {\n ALLOC_GROW(ctx->oids.list, ctx->oids.nr + 1, ctx->oids.alloc);\n oidcpy(&ctx->oids.list[ctx->oids.nr], &(result->object.oid));\n ctx->oids.nr++;\n- } else if (ctx->check_oids) {\n+ }\n+ }\n+\n+ if (ret || (ctx->check_oids && !result)) {\n error(_(\"invalid commit object id: %s\"),\n commit_hex->items[i].string);\n return -1;\n\n> -Peff\n\nThanks,\nTaylor\n"},{"id":"395371","messageId":"20200413212506.GA28407@coredump.intra.peff.net","threadId":"51589","inReplyTo":"20200413193934.GC63249@syl.local","subject":"Re: [PATCH 3/3] commit-graph: error out on invalid commit oids in 'write --stdin-commits'","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-04-13T21:25:06Z","receivedAt":"2020-04-13T21:25:12Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Apr 13, 2020 at 01:39:34PM -0600, Taylor Blau wrote:\n\n> > > Hm, are you trying to go in the direction where '--stdin-commits'\n> > > would keep erroring out on any non-full-hex-oid, but would accept and\n> > > silently ignore any hex oids that are not commits (perhaps even when\n> > > there is no such object, dunno)? I think that would support the use\n> > > cases you mentioned, while it would still save me when I do the 'echo\n> > > ' thing (somehow I regularly do that, remember doing it the day\n> > > before yesterday!).\n> >\n> > Yes, exactly. The case you care about and the case I care about are\n> > different ones, so there's no inherent conflict between them.\n> \n> I was looking back again at this today, and I think we need something\n> more or less like the following on top. I'll send it out later today or\n> early tomorrow...\n\nYes, the behavior there looks fine to me. Though you may want to catch\nthe parse_oid_hex() separately and return its own error message. Telling\nthe user \"I don't understand non-hex object names\" instead of just\n\"invalid commit object\" may be useful. I think it would also make the\nflow of the function easier to follow.\n\nIf we were writing from scratch, I'd actually suggest that\nbuiltin/commit-graph.c do parse_oid_hex() call as we read lines, and\nthen commit-graph could just be working with an oid_array or oidset,\nwhich would reduce overall memory usage. I don't know if that would\ncause other complications, but it could be worth looking into.\n\n> + if (ret || (ctx->check_oids && !result)) {\n> error(_(\"invalid commit object id: %s\"),\n> commit_hex->items[i].string);\n> return -1;\n\nWe could also take this a step further and just ditch check_oids\nentirely (under the assumption that nobody really wants it; they just\nwanted to catch bad names in the first place).\n\n-Peff\n"},{"id":"395388","messageId":"20200414020410.GC71440@syl.local","threadId":"51589","inReplyTo":"20200413212506.GA28407@coredump.intra.peff.net","subject":"Re: [PATCH 3/3] commit-graph: error out on invalid commit oids in 'write --stdin-commits'","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-04-14T02:04:10Z","receivedAt":"2020-04-14T02:04:16Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Mon, Apr 13, 2020 at 05:25:06PM -0400, Jeff King wrote:\n> On Mon, Apr 13, 2020 at 01:39:34PM -0600, Taylor Blau wrote:\n>\n> > > > Hm, are you trying to go in the direction where '--stdin-commits'\n> > > > would keep erroring out on any non-full-hex-oid, but would accept and\n> > > > silently ignore any hex oids that are not commits (perhaps even when\n> > > > there is no such object, dunno)? I think that would support the use\n> > > > cases you mentioned, while it would still save me when I do the 'echo\n> > > > ' thing (somehow I regularly do that, remember doing it the day\n> > > > before yesterday!).\n> > >\n> > > Yes, exactly. The case you care about and the case I care about are\n> > > different ones, so there's no inherent conflict between them.\n> >\n> > I was looking back again at this today, and I think we need something\n> > more or less like the following on top. I'll send it out later today or\n> > early tomorrow...\n>\n> Yes, the behavior there looks fine to me. Though you may want to catch\n> the parse_oid_hex() separately and return its own error message. Telling\n> the user \"I don't understand non-hex object names\" instead of just\n> \"invalid commit object\" may be useful. I think it would also make the\n> flow of the function easier to follow.\n>\n> If we were writing from scratch, I'd actually suggest that\n> builtin/commit-graph.c do parse_oid_hex() call as we read lines, and\n> then commit-graph could just be working with an oid_array or oidset,\n> which would reduce overall memory usage. I don't know if that would\n> cause other complications, but it could be worth looking into.\n\nIt actually improved the situation quite a bit, so thanks for the\nsuggestion. In addition refactoring it was quite a lot of fun. The\nsecond-order benefit was that it moves these two failure modes into\nseparate parts of the code.\n\nConverting the user input to an OID (and thus dealing with input like\n\"HEAD\", \"refs/tags/foo\", and etc.) is a separate part from turning those\nOIDs into 'struct commit *'s. So the \"non-OID input\" and \"this OID\ndoesn't refer to a commit\" checks can be moved into the builtin and\nlibrary machinery separately, which is quite handy.\n\nThe patch is too large to send here inline (there's a lot of noise from\nrenaming 'commit_hex' to 'commits'), but I'll include it in my\ncommit-graphs backlog series shortly.\n\n> > + if (ret || (ctx->check_oids && !result)) {\n> > error(_(\"invalid commit object id: %s\"),\n> > commit_hex->items[i].string);\n> > return -1;\n>\n> We could also take this a step further and just ditch check_oids\n> entirely (under the assumption that nobody really wants it; they just\n> wanted to catch bad names in the first place).\n\nI'd rather let this shake out in a discussion of the patch once I send\nit, since I'm not sure how people feel in general.\n\n> -Peff\n\nThanks,\nTaylor\n"}]}