{"thread":{"id":"53223","subject":"[PATCH 0/7] commit-graph: split strategies, '--[no-]check-oids'","startedAt":"2020-04-14T04:04:06Z","lastAt":"2020-05-04T22:16:51Z","messageCount":19,"participants":["Taylor Blau","SZEDER Gábor","Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":7},"messages":[{"id":"395389","messageId":"cover.1586836700.git.me@ttaylorr.com","threadId":"53223","inReplyTo":null,"subject":"[PATCH 0/7] commit-graph: split strategies, '--[no-]check-oids'","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-04-14T04:03:59Z","receivedAt":"2020-04-14T04:04:06Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"Hi,\n\nThis is a reworked version of a couple of series [1, 2] that add various\noptions to the 'git commit-graph write' builtin. I had developed [1, 2]\nwhile deploying incremental commit-graphs at GitHub, and so there was a\nlot in flux about what options we needed and so on.\n\nNow that the dust has settled, I cleaned up all of the patches generated\nduring that time into a grab-bag series that reflects what we ended up\nsticking with.\n\nNamely, I got rid of the '--input=' options ('--input=none' was\nconfusing at best, and we ended up not needing it), as well as\n'--split=merge-all'. We used to use this to roll up all of the\nincremental graphs generated during pushes into one big length-1 chain\nevery N pushes. But, we ended up abandoning this, too, in favor of\n'--split=replace', which is described in the fourth patch. I'm not sure\nif anyone will be sad to see '--split=merge-all' go, so if you are,\nplease let me know and I'll drag that patch back in.\n\nThanks for all of the review that I've received so far, and thanks in\nadvance for your review on these patches.\n\n[1]: https://lore.kernel.org/git/cover.1580430057.git.me@ttaylorr.com/\n[2]: https://lore.kernel.org/git/cover.1584762087.git.me@ttaylorr.com/\n\nTaylor Blau (7):\n t/helper/test-read-graph.c: support commit-graph chains\n builtin/commit-graph.c: support for '--split[=]'\n builtin/commit-graph.c: introduce split strategy 'no-merge'\n builtin/commit-graph.c: introduce split strategy 'replace'\n oidset: introduce 'oidset_size'\n commit-graph.h: replace 'commit_hex' with 'commits'\n commit-graph.c: introduce '--[no-]check-oids'\n\n Documentation/git-commit-graph.txt | 22 +++--\n builtin/commit-graph.c | 59 +++++++++++--\n commit-graph.c | 129 +++++++++++++++++++----------\n commit-graph.h | 10 ++-\n oidset.c | 5 ++\n oidset.h | 5 ++\n t/helper/test-read-graph.c | 13 +--\n t/t5318-commit-graph.sh | 30 ++++++-\n t/t5324-split-commit-graph.sh | 30 +++++++\n 9 files changed, 230 insertions(+), 73 deletions(-)\n\n--\n2.26.0.106.g9fadedd637\n"},{"id":"395390","messageId":"0eb2c610283689675450b1e4e2c66147f1500cf1.1586836700.git.me@ttaylorr.com","threadId":"53223","inReplyTo":"cover.1586836700.git.me@ttaylorr.com","subject":"[PATCH 1/7] t/helper/test-read-graph.c: support commit-graph chains","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-04-14T04:04:04Z","receivedAt":"2020-04-14T04:04:09Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"In 61df89c8e5 (commit-graph: don't early exit(1) on e.g. \"git status\",\n2019-03-25), the former 'load_commit_graph_one' was refactored into\n'open_commit_graph' and 'load_commit_graph_one_fd_st' as a means of\navoiding an early-exit from non-library code.\n\nHowever, 'load_commit_graph_one' does not support commit-graph chains,\nand hence the 'read-graph' test tool does not work with them.\n\nReplace 'load_commit_graph_one' with 'read_commit_graph_one' in order to\nsupport commit-graph chains. In the spirit of 61df89c8e5,\n'read_commit_graph_one' does not ever 'die()', making it a suitable\nreplacement here.\n\nSigned-off-by: Taylor Blau \n---\n t/helper/test-read-graph.c | 13 +------------\n 1 file changed, 1 insertion(+), 12 deletions(-)\n\ndiff --git a/t/helper/test-read-graph.c b/t/helper/test-read-graph.c\nindex f8a461767c..4846040363 100644\n--- a/t/helper/test-read-graph.c\n+++ b/t/helper/test-read-graph.c\n@@ -7,26 +7,15 @@\n int cmd__read_graph(int argc, const char **argv)\n {\n \tstruct commit_graph *graph = NULL;\n-\tchar *graph_name;\n-\tint open_ok;\n-\tint fd;\n-\tstruct stat st;\n \tstruct object_directory *odb;\n \n \tsetup_git_directory();\n \todb = the_repository->objects->odb;\n \n-\tgraph_name = get_commit_graph_filename(odb);\n-\n-\topen_ok = open_commit_graph(graph_name, &fd, &st);\n-\tif (!open_ok)\n-\t\tdie_errno(_(\"Could not open commit-graph '%s'\"), graph_name);\n-\n-\tgraph = load_commit_graph_one_fd_st(fd, &st, odb);\n+\tgraph = read_commit_graph_one(the_repository, odb);\n \tif (!graph)\n \t\treturn 1;\n \n-\tFREE_AND_NULL(graph_name);\n \n \tprintf(\"header: %08x %d %d %d %d\\n\",\n \t\tntohl(*(uint32_t*)graph->data),\n-- \n2.26.0.106.g9fadedd637\n\n"},{"id":"395391","messageId":"e70c66be882e004a527df3da4d130a26356883e0.1586836700.git.me@ttaylorr.com","threadId":"53223","inReplyTo":"cover.1586836700.git.me@ttaylorr.com","subject":"[PATCH 2/7] builtin/commit-graph.c: support for '--split[=]'","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-04-14T04:04:08Z","receivedAt":"2020-04-14T04:04:17Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"With '--split', the commit-graph machinery writes new commits in another\nincremental commit-graph which is part of the existing chain, and\noptionally decides to condense the chain into a single commit-graph.\nThis is done to ensure that the asymptotic behavior of looking up a\ncommit in an incremental chain is not dominated by the number of\nincrementals in that chain. It can be controlled by the '--max-commits'\nand '--size-multiple' options.\n\nIn the next two commits, we will introduce additional splitting\nstrategies that can exert additional control over:\n\n - when a split commit-graph is and isn't written, and\n\n - when the existing commit-graph chain is discarded completely and\n replaced with another graph\n\nTo prepare for this, make '--split' take an optional strategy (as in\n'--split[=]'), and add a new enum to describe which strategy\nis being used. For now, no strategies are given, and the only enumerated\nvalue is 'COMMIT_GRAPH_SPLIT_UNSPECIFIED', indicating the absence of a\nstrategy.\n\nSigned-off-by: Taylor Blau \n---\n Documentation/git-commit-graph.txt | 11 ++++++-----\n builtin/commit-graph.c | 26 ++++++++++++++++++++++----\n commit-graph.h | 5 +++++\n 3 files changed, 33 insertions(+), 9 deletions(-)\n\ndiff --git a/Documentation/git-commit-graph.txt b/Documentation/git-commit-graph.txt\nindex 28d1fee505..10d757c5cc 100644\n--- a/Documentation/git-commit-graph.txt\n+++ b/Documentation/git-commit-graph.txt\n@@ -57,11 +57,12 @@ or `--stdin-packs`.)\n With the `--append` option, include all commits that are present in the\n existing commit-graph file.\n +\n-With the `--split` option, write the commit-graph as a chain of multiple\n-commit-graph files stored in `/info/commit-graphs`. The new commits\n-not already in the commit-graph are added in a new \"tip\" file. This file\n-is merged with the existing file if the following merge conditions are\n-met:\n+With the `--split[=]` option, write the commit-graph as a\n+chain of multiple commit-graph files stored in\n+`/info/commit-graphs`. Commit-graph layers are merged based on the\n+strategy and other splitting options. The new commits not already in the\n+commit-graph are added in a new \"tip\" file. This file is merged with the\n+existing file if the following merge conditions are met:\n +\n * If `--size-multiple=` is not specified, let `X` equal 2. If the new\n tip file would have `N` commits and the previous tip has `M` commits and\ndiff --git a/builtin/commit-graph.c b/builtin/commit-graph.c\nindex d1ab6625f6..342094fc77 100644\n--- a/builtin/commit-graph.c\n+++ b/builtin/commit-graph.c\n@@ -9,7 +9,9 @@\n \n static char const * const builtin_commit_graph_usage[] = {\n \tN_(\"git commit-graph verify [--object-dir ] [--shallow] [--[no-]progress]\"),\n-\tN_(\"git commit-graph write [--object-dir ] [--append|--split] [--reachable|--stdin-packs|--stdin-commits] [--[no-]progress] \"),\n+\tN_(\"git commit-graph write [--object-dir ] [--append] \"\n+\t \"[--split[=]] [--reachable|--stdin-packs|--stdin-commits] \"\n+\t \"[--[no-]progress] \"),\n \tNULL\n };\n \n@@ -19,7 +21,9 @@ static const char * const builtin_commit_graph_verify_usage[] = {\n };\n \n static const char * const builtin_commit_graph_write_usage[] = {\n-\tN_(\"git commit-graph write [--object-dir ] [--append|--split] [--reachable|--stdin-packs|--stdin-commits] [--[no-]progress] \"),\n+\tN_(\"git commit-graph write [--object-dir ] [--append] \"\n+\t \"[--split[=]] [--reachable|--stdin-packs|--stdin-commits] \"\n+\t \"[--[no-]progress] \"),\n \tNULL\n };\n \n@@ -114,6 +118,18 @@ static int graph_verify(int argc, const char **argv)\n extern int read_replace_refs;\n static struct split_commit_graph_opts split_opts;\n \n+static int write_option_parse_split(const struct option *opt, const char *arg,\n+\t\t\t\t int unset)\n+{\n+\topts.split = 1;\n+\tif (!arg)\n+\t\treturn 0;\n+\n+\tdie(_(\"unrecognized --split argument, %s\"), arg);\n+\n+\treturn 0;\n+}\n+\n static int graph_write(int argc, const char **argv)\n {\n \tstruct string_list *pack_indexes = NULL;\n@@ -136,8 +152,10 @@ static int graph_write(int argc, const char **argv)\n \t\tOPT_BOOL(0, \"append\", &opts.append,\n \t\t\tN_(\"include all commits already in the commit-graph file\")),\n \t\tOPT_BOOL(0, \"progress\", &opts.progress, N_(\"force progress reporting\")),\n-\t\tOPT_BOOL(0, \"split\", &opts.split,\n-\t\t\tN_(\"allow writing an incremental commit-graph file\")),\n+\t\tOPT_CALLBACK_F(0, \"split\", &split_opts.flags, NULL,\n+\t\t\tN_(\"allow writing an incremental commit-graph file\"),\n+\t\t\tPARSE_OPT_OPTARG | PARSE_OPT_NONEG,\n+\t\t\twrite_option_parse_split),\n \t\tOPT_INTEGER(0, \"max-commits\", &split_opts.max_commits,\n \t\t\tN_(\"maximum number of commits in a non-base split commit-graph\")),\n \t\tOPT_INTEGER(0, \"size-multiple\", &split_opts.size_multiple,\ndiff --git a/commit-graph.h b/commit-graph.h\nindex e87a6f6360..e799008ff4 100644\n--- a/commit-graph.h\n+++ b/commit-graph.h\n@@ -82,10 +82,15 @@ enum commit_graph_write_flags {\n \tCOMMIT_GRAPH_WRITE_CHECK_OIDS = (1 << 3)\n };\n \n+enum commit_graph_split_flags {\n+\tCOMMIT_GRAPH_SPLIT_UNSPECIFIED = 0\n+};\n+\n struct split_commit_graph_opts {\n \tint size_multiple;\n \tint max_commits;\n \ttimestamp_t expire_time;\n+\tenum commit_graph_split_flags flags;\n };\n \n /*\n-- \n2.26.0.106.g9fadedd637\n\n"},{"id":"395392","messageId":"2741858596ce9cd48aa0e3d126310435166d37ab.1586836700.git.me@ttaylorr.com","threadId":"53223","inReplyTo":"cover.1586836700.git.me@ttaylorr.com","subject":"[PATCH 3/7] builtin/commit-graph.c: introduce split strategy 'no-merge'","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-04-14T04:04:12Z","receivedAt":"2020-04-14T04:04:18Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"In the previous commit, we laid the groundwork for supporting different\nsplitting strategies. In this commit, we introduce the first splitting\nstrategy: 'no-merge'.\n\nPassing '--split=no-merge' is useful for callers which wish to write a\nnew incremental commit-graph, but do not want to spend effort condensing\nthe incremental chain [1]. Previously, this was possible by passing\n'--size-multiple=0', but this no longer the case following 63020f175f\n(commit-graph: prefer default size_mult when given zero, 2020-01-02).\n\nWhen '--split=no-merge' is given, the commit-graph machinery will never\ncondense an existing chain, and it will always write a new incremental.\n\n[1]: This might occur when, for example, a server administrator running\nsome program after each push may want to ensure that each job runs\nproportional in time to the size of the push, and does not \"jump\" when\nthe commit-graph machinery decides to trigger a merge.\n\nSigned-off-by: Taylor Blau \n---\n Documentation/git-commit-graph.txt | 5 +++++\n builtin/commit-graph.c | 7 ++++++-\n commit-graph.c | 19 ++++++++++++-------\n commit-graph.h | 3 ++-\n t/t5324-split-commit-graph.sh | 11 +++++++++++\n 5 files changed, 36 insertions(+), 9 deletions(-)\n\ndiff --git a/Documentation/git-commit-graph.txt b/Documentation/git-commit-graph.txt\nindex 10d757c5cc..a4c4a641e5 100644\n--- a/Documentation/git-commit-graph.txt\n+++ b/Documentation/git-commit-graph.txt\n@@ -63,6 +63,11 @@ chain of multiple commit-graph files stored in\n strategy and other splitting options. The new commits not already in the\n commit-graph are added in a new \"tip\" file. This file is merged with the\n existing file if the following merge conditions are met:\n+* If `--split=no-merge` is specified, a merge is never performed, and\n+the remaining options are ignored. A bare `--split` defers to the\n+remaining options. (Note that merging a chain of commit graphs replaces\n+the existing chain with a length-1 chain where the first and only\n+incremental holds the entire graph).\n +\n * If `--size-multiple=` is not specified, let `X` equal 2. If the new\n tip file would have `N` commits and the previous tip has `M` commits and\ndiff --git a/builtin/commit-graph.c b/builtin/commit-graph.c\nindex 342094fc77..42182ed71c 100644\n--- a/builtin/commit-graph.c\n+++ b/builtin/commit-graph.c\n@@ -121,11 +121,16 @@ static struct split_commit_graph_opts split_opts;\n static int write_option_parse_split(const struct option *opt, const char *arg,\n \t\t\t\t int unset)\n {\n+\tenum commit_graph_split_flags *flags = opt->value;\n+\n \topts.split = 1;\n \tif (!arg)\n \t\treturn 0;\n \n-\tdie(_(\"unrecognized --split argument, %s\"), arg);\n+\tif (!strcmp(arg, \"no-merge\"))\n+\t\t*flags = COMMIT_GRAPH_SPLIT_MERGE_PROHIBITED;\n+\telse\n+\t\tdie(_(\"unrecognized --split argument, %s\"), arg);\n \n \treturn 0;\n }\ndiff --git a/commit-graph.c b/commit-graph.c\nindex f013a84e29..af3fe20bb5 100644\n--- a/commit-graph.c\n+++ b/commit-graph.c\n@@ -1529,6 +1529,7 @@ static void split_graph_merge_strategy(struct write_commit_graph_context *ctx)\n {\n \tstruct commit_graph *g;\n \tuint32_t num_commits;\n+\tenum commit_graph_split_flags flags = COMMIT_GRAPH_SPLIT_UNSPECIFIED;\n \tuint32_t i;\n \n \tint max_commits = 0;\n@@ -1539,21 +1540,25 @@ static void split_graph_merge_strategy(struct write_commit_graph_context *ctx)\n \n \t\tif (ctx->split_opts->size_multiple)\n \t\t\tsize_mult = ctx->split_opts->size_multiple;\n+\n+\t\tflags = ctx->split_opts->flags;\n \t}\n \n \tg = ctx->r->objects->commit_graph;\n \tnum_commits = ctx->commits.nr;\n \tctx->num_commit_graphs_after = ctx->num_commit_graphs_before + 1;\n \n-\twhile (g && (g->num_commits <= size_mult * num_commits ||\n-\t\t (max_commits && num_commits > max_commits))) {\n-\t\tif (g->odb != ctx->odb)\n-\t\t\tbreak;\n+\tif (flags != COMMIT_GRAPH_SPLIT_MERGE_PROHIBITED) {\n+\t\twhile (g && (g->num_commits <= size_mult * num_commits ||\n+\t\t\t (max_commits && num_commits > max_commits))) {\n+\t\t\tif (g->odb != ctx->odb)\n+\t\t\t\tbreak;\n \n-\t\tnum_commits += g->num_commits;\n-\t\tg = g->base_graph;\n+\t\t\tnum_commits += g->num_commits;\n+\t\t\tg = g->base_graph;\n \n-\t\tctx->num_commit_graphs_after--;\n+\t\t\tctx->num_commit_graphs_after--;\n+\t\t}\n \t}\n \n \tctx->new_base_graph = g;\ndiff --git a/commit-graph.h b/commit-graph.h\nindex e799008ff4..8752afb88d 100644\n--- a/commit-graph.h\n+++ b/commit-graph.h\n@@ -83,7 +83,8 @@ enum commit_graph_write_flags {\n };\n \n enum commit_graph_split_flags {\n-\tCOMMIT_GRAPH_SPLIT_UNSPECIFIED = 0\n+\tCOMMIT_GRAPH_SPLIT_UNSPECIFIED = 0,\n+\tCOMMIT_GRAPH_SPLIT_MERGE_PROHIBITED = 1\n };\n \n struct split_commit_graph_opts {\ndiff --git a/t/t5324-split-commit-graph.sh b/t/t5324-split-commit-graph.sh\nindex 53b2e6b455..1438d63f24 100755\n--- a/t/t5324-split-commit-graph.sh\n+++ b/t/t5324-split-commit-graph.sh\n@@ -351,4 +351,15 @@ test_expect_success 'split across alternate where alternate is not split' '\n \ttest_cmp commit-graph .git/objects/info/commit-graph\n '\n \n+test_expect_success '--split=no-merge always writes an incremental' '\n+\ttest_when_finished rm -rf a b &&\n+\trm -rf $graphdir $infodir/commit-graph &&\n+\tgit reset --hard commits/2 &&\n+\tgit rev-list HEAD~1 >a &&\n+\tgit rev-list HEAD >b &&\n+\tgit commit-graph write --split --stdin-commits \n---\n Documentation/git-commit-graph.txt | 7 ++--\n builtin/commit-graph.c | 2 ++\n commit-graph.c | 53 ++++++++++++++++++++++--------\n commit-graph.h | 3 +-\n t/t5324-split-commit-graph.sh | 19 +++++++++++\n 5 files changed, 66 insertions(+), 18 deletions(-)\n\ndiff --git a/Documentation/git-commit-graph.txt b/Documentation/git-commit-graph.txt\nindex a4c4a641e5..46f7f7c573 100644\n--- a/Documentation/git-commit-graph.txt\n+++ b/Documentation/git-commit-graph.txt\n@@ -64,9 +64,10 @@ strategy and other splitting options. The new commits not already in the\n commit-graph are added in a new \"tip\" file. This file is merged with the\n existing file if the following merge conditions are met:\n * If `--split=no-merge` is specified, a merge is never performed, and\n-the remaining options are ignored. A bare `--split` defers to the\n-remaining options. (Note that merging a chain of commit graphs replaces\n-the existing chain with a length-1 chain where the first and only\n+the remaining options are ignored. `--split=replace` overwrites the\n+existing chain with a new one. A bare `--split` defers to the remaining\n+options. (Note that merging a chain of commit graphs replaces the\n+existing chain with a length-1 chain where the first and only\n incremental holds the entire graph).\n +\n * If `--size-multiple=` is not specified, let `X` equal 2. If the new\ndiff --git a/builtin/commit-graph.c b/builtin/commit-graph.c\nindex 42182ed71c..922f876bfa 100644\n--- a/builtin/commit-graph.c\n+++ b/builtin/commit-graph.c\n@@ -129,6 +129,8 @@ static int write_option_parse_split(const struct option *opt, const char *arg,\n \n \tif (!strcmp(arg, \"no-merge\"))\n \t\t*flags = COMMIT_GRAPH_SPLIT_MERGE_PROHIBITED;\n+\telse if (!strcmp(arg, \"replace\"))\n+\t\t*flags = COMMIT_GRAPH_SPLIT_REPLACE;\n \telse\n \t\tdie(_(\"unrecognized --split argument, %s\"), arg);\n \ndiff --git a/commit-graph.c b/commit-graph.c\nindex af3fe20bb5..c598508d7f 100644\n--- a/commit-graph.c\n+++ b/commit-graph.c\n@@ -866,7 +866,7 @@ static void write_graph_chunk_data(struct hashfile *f, int hash_len,\n \n \t\t\tif (edge_value >= 0)\n \t\t\t\tedge_value += ctx->new_num_commits_in_base;\n-\t\t\telse {\n+\t\t\telse if (ctx->new_base_graph) {\n \t\t\t\tuint32_t pos;\n \t\t\t\tif (find_commit_in_graph(parent->item,\n \t\t\t\t\t\t\t ctx->new_base_graph,\n@@ -897,7 +897,7 @@ static void write_graph_chunk_data(struct hashfile *f, int hash_len,\n \n \t\t\tif (edge_value >= 0)\n \t\t\t\tedge_value += ctx->new_num_commits_in_base;\n-\t\t\telse {\n+\t\t\telse if (ctx->new_base_graph) {\n \t\t\t\tuint32_t pos;\n \t\t\t\tif (find_commit_in_graph(parent->item,\n \t\t\t\t\t\t\t ctx->new_base_graph,\n@@ -964,7 +964,7 @@ static void write_graph_chunk_extra_edges(struct hashfile *f,\n \n \t\t\tif (edge_value >= 0)\n \t\t\t\tedge_value += ctx->new_num_commits_in_base;\n-\t\t\telse {\n+\t\t\telse if (ctx->new_base_graph) {\n \t\t\t\tuint32_t pos;\n \t\t\t\tif (find_commit_in_graph(parent->item,\n \t\t\t\t\t\t\t ctx->new_base_graph,\n@@ -1037,6 +1037,8 @@ static void close_reachable(struct write_commit_graph_context *ctx)\n {\n \tint i;\n \tstruct commit *commit;\n+\tenum commit_graph_split_flags flags = ctx->split_opts ?\n+\t\tctx->split_opts->flags : COMMIT_GRAPH_SPLIT_UNSPECIFIED;\n \n \tif (ctx->report_progress)\n \t\tctx->progress = start_delayed_progress(\n@@ -1066,8 +1068,9 @@ static void close_reachable(struct write_commit_graph_context *ctx)\n \t\tif (!commit)\n \t\t\tcontinue;\n \t\tif (ctx->split) {\n-\t\t\tif (!parse_commit(commit) &&\n-\t\t\t commit->graph_pos == COMMIT_NOT_FROM_GRAPH)\n+\t\t\tif ((!parse_commit(commit) &&\n+\t\t\t commit->graph_pos == COMMIT_NOT_FROM_GRAPH) ||\n+\t\t\t flags == COMMIT_GRAPH_SPLIT_REPLACE)\n \t\t\t\tadd_missing_parents(ctx, commit);\n \t\t} else if (!parse_commit_no_graph(commit))\n \t\t\tadd_missing_parents(ctx, commit);\n@@ -1287,6 +1290,8 @@ static uint32_t count_distinct_commits(struct write_commit_graph_context *ctx)\n static void copy_oids_to_commits(struct write_commit_graph_context *ctx)\n {\n \tuint32_t i;\n+\tenum commit_graph_split_flags flags = ctx->split_opts ?\n+\t\tctx->split_opts->flags : COMMIT_GRAPH_SPLIT_UNSPECIFIED;\n \n \tctx->num_extra_edges = 0;\n \tif (ctx->report_progress)\n@@ -1303,11 +1308,14 @@ static void copy_oids_to_commits(struct write_commit_graph_context *ctx)\n \t\tALLOC_GROW(ctx->commits.list, ctx->commits.nr + 1, ctx->commits.alloc);\n \t\tctx->commits.list[ctx->commits.nr] = lookup_commit(ctx->r, &ctx->oids.list[i]);\n \n-\t\tif (ctx->split &&\n+\t\tif (ctx->split && flags != COMMIT_GRAPH_SPLIT_REPLACE &&\n \t\t ctx->commits.list[ctx->commits.nr]->graph_pos != COMMIT_NOT_FROM_GRAPH)\n \t\t\tcontinue;\n \n-\t\tparse_commit_no_graph(ctx->commits.list[ctx->commits.nr]);\n+\t\tif (ctx->split && flags == COMMIT_GRAPH_SPLIT_REPLACE)\n+\t\t\tparse_commit(ctx->commits.list[ctx->commits.nr]);\n+\t\telse\n+\t\t\tparse_commit_no_graph(ctx->commits.list[ctx->commits.nr]);\n \n \t\tnum_parents = commit_list_count(ctx->commits.list[ctx->commits.nr]->parents);\n \t\tif (num_parents > 2)\n@@ -1488,8 +1496,12 @@ static int write_commit_graph_file(struct write_commit_graph_context *ctx)\n \t\t}\n \n \t\tif (ctx->base_graph_name) {\n-\t\t\tconst char *dest = ctx->commit_graph_filenames_after[\n-\t\t\t\t\t\tctx->num_commit_graphs_after - 2];\n+\t\t\tconst char *dest;\n+\t\t\tint idx = ctx->num_commit_graphs_after - 1;\n+\t\t\tif (ctx->num_commit_graphs_after > 1)\n+\t\t\t\tidx--;\n+\n+\t\t\tdest = ctx->commit_graph_filenames_after[idx];\n \n \t\t\tif (strcmp(ctx->base_graph_name, dest)) {\n \t\t\t\tresult = rename(ctx->base_graph_name, dest);\n@@ -1546,9 +1558,13 @@ static void split_graph_merge_strategy(struct write_commit_graph_context *ctx)\n \n \tg = ctx->r->objects->commit_graph;\n \tnum_commits = ctx->commits.nr;\n-\tctx->num_commit_graphs_after = ctx->num_commit_graphs_before + 1;\n+\tif (flags == COMMIT_GRAPH_SPLIT_REPLACE)\n+\t\tctx->num_commit_graphs_after = 1;\n+\telse\n+\t\tctx->num_commit_graphs_after = ctx->num_commit_graphs_before + 1;\n \n-\tif (flags != COMMIT_GRAPH_SPLIT_MERGE_PROHIBITED) {\n+\tif (flags != COMMIT_GRAPH_SPLIT_MERGE_PROHIBITED &&\n+\t flags != COMMIT_GRAPH_SPLIT_REPLACE) {\n \t\twhile (g && (g->num_commits <= size_mult * num_commits ||\n \t\t\t (max_commits && num_commits > max_commits))) {\n \t\t\tif (g->odb != ctx->odb)\n@@ -1561,7 +1577,11 @@ static void split_graph_merge_strategy(struct write_commit_graph_context *ctx)\n \t\t}\n \t}\n \n-\tctx->new_base_graph = g;\n+\tif (flags != COMMIT_GRAPH_SPLIT_REPLACE)\n+\t\tctx->new_base_graph = g;\n+\telse if (ctx->num_commit_graphs_after != 1)\n+\t\tBUG(\"split_graph_merge_strategy: num_commit_graphs_after \"\n+\t\t \"should be 1 with --split=replace\");\n \n \tif (ctx->num_commit_graphs_after == 2) {\n \t\tchar *old_graph_name = get_commit_graph_filename(g->odb);\n@@ -1768,6 +1788,7 @@ int write_commit_graph(struct object_directory *odb,\n \tstruct write_commit_graph_context *ctx;\n \tuint32_t i, count_distinct = 0;\n \tint res = 0;\n+\tint replace = 0;\n \n \tif (!commit_graph_compatible(the_repository))\n \t\treturn 0;\n@@ -1802,6 +1823,9 @@ int write_commit_graph(struct object_directory *odb,\n \t\t\t\tg = g->base_graph;\n \t\t\t}\n \t\t}\n+\n+\t\tif (ctx->split_opts)\n+\t\t\treplace = ctx->split_opts->flags & COMMIT_GRAPH_SPLIT_REPLACE;\n \t}\n \n \tctx->approx_nr_objects = approximate_object_count();\n@@ -1862,13 +1886,14 @@ int write_commit_graph(struct object_directory *odb,\n \t\tgoto cleanup;\n \t}\n \n-\tif (!ctx->commits.nr)\n+\tif (!ctx->commits.nr && !replace)\n \t\tgoto cleanup;\n \n \tif (ctx->split) {\n \t\tsplit_graph_merge_strategy(ctx);\n \n-\t\tmerge_commit_graphs(ctx);\n+\t\tif (!replace)\n+\t\t\tmerge_commit_graphs(ctx);\n \t} else\n \t\tctx->num_commit_graphs_after = 1;\n \ndiff --git a/commit-graph.h b/commit-graph.h\nindex 8752afb88d..718433d79b 100644\n--- a/commit-graph.h\n+++ b/commit-graph.h\n@@ -84,7 +84,8 @@ enum commit_graph_write_flags {\n \n enum commit_graph_split_flags {\n \tCOMMIT_GRAPH_SPLIT_UNSPECIFIED = 0,\n-\tCOMMIT_GRAPH_SPLIT_MERGE_PROHIBITED = 1\n+\tCOMMIT_GRAPH_SPLIT_MERGE_PROHIBITED = 1,\n+\tCOMMIT_GRAPH_SPLIT_REPLACE = 2\n };\n \n struct split_commit_graph_opts {\ndiff --git a/t/t5324-split-commit-graph.sh b/t/t5324-split-commit-graph.sh\nindex 1438d63f24..e5d8d64170 100755\n--- a/t/t5324-split-commit-graph.sh\n+++ b/t/t5324-split-commit-graph.sh\n@@ -362,4 +362,23 @@ test_expect_success '--split=no-merge always writes an incremental' '\n \ttest_line_count = 2 $graphdir/commit-graph-chain\n '\n \n+test_expect_success '--split=replace replaces the chain' '\n+\trm -rf $graphdir $infodir/commit-graph &&\n+\tgit reset --hard commits/3 &&\n+\tgit rev-list -1 HEAD~2 >a &&\n+\tgit rev-list -1 HEAD~1 >b &&\n+\tgit rev-list -1 HEAD >c &&\n+\tgit commit-graph write --split=no-merge --stdin-commits graph-files &&\n+\ttest_line_count = 1 graph-files &&\n+\tverify_chain_files_exist $graphdir &&\n+\tgraph_read_expect 2\n+'\n+\n test_done\n-- \n2.26.0.106.g9fadedd637\n\n"},{"id":"395394","messageId":"4a9798ba6f3aab081dff5e40e1d91568cf4ba23b.1586836700.git.me@ttaylorr.com","threadId":"53223","inReplyTo":"cover.1586836700.git.me@ttaylorr.com","subject":"[PATCH 5/7] oidset: introduce 'oidset_size'","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-04-14T04:04:22Z","receivedAt":"2020-04-14T04:04:27Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"Occasionally, it may be useful for callers to know the number of object\nIDs in an oidset. Right now, the only way to compute this is to call\n'kh_size' on the internal 'kh_set_oid_t'.\n\nSimilar to how we wrap other 'kh_*' functions over the 'oidset' type,\nlet's allow callers to compute this value by introducing 'oidset_size'.\n\nWe will add its first caller in the subsequent commit.\n\nSigned-off-by: Taylor Blau \n---\n oidset.c | 5 +++++\n oidset.h | 5 +++++\n 2 files changed, 10 insertions(+)\n\ndiff --git a/oidset.c b/oidset.c\nindex f63ce818f6..15d4e18c37 100644\n--- a/oidset.c\n+++ b/oidset.c\n@@ -36,6 +36,11 @@ void oidset_clear(struct oidset *set)\n \toidset_init(set, 0);\n }\n \n+int oidset_size(struct oidset *set)\n+{\n+\treturn kh_size(&set->set);\n+}\n+\n void oidset_parse_file(struct oidset *set, const char *path)\n {\n \tFILE *fp;\ndiff --git a/oidset.h b/oidset.h\nindex d8a106b127..8261c36b91 100644\n--- a/oidset.h\n+++ b/oidset.h\n@@ -54,6 +54,11 @@ int oidset_insert(struct oidset *set, const struct object_id *oid);\n */\n int oidset_remove(struct oidset *set, const struct object_id *oid);\n \n+/**\n+ * Returns the number of oids in the set.\n+ */\n+int oidset_size(struct oidset *set);\n+\n /**\n * Remove all entries from the oidset, freeing any resources associated with\n * it.\n-- \n2.26.0.106.g9fadedd637\n\n"},{"id":"395395","messageId":"811a2ece05f2d576bf2b3d6429c537800649b27b.1586836700.git.me@ttaylorr.com","threadId":"53223","inReplyTo":"cover.1586836700.git.me@ttaylorr.com","subject":"[PATCH 6/7] commit-graph.h: replace 'commit_hex' with 'commits'","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-04-14T04:04:25Z","receivedAt":"2020-04-14T04:04:30Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"The 'write_commit_graph()' function takes in either a string list of\npack indices, or a string list of hexadecimal commit OIDs. These\ncorrespond to the '--stdin-packs' and '--stdin-commits' mode(s) from\n'git commit-graph write'.\n\nUsing a string_list of hexadecimal commit IDs is not the most efficient\nuse of memory, since we can instead use the 'struct oidset', which is\nmore well-suited for this case.\n\nThis has another benefit which will become apparent in the following\ncommit. This is that we are about to disambiguate the kinds of errors we\nproduce with '--stdin-commits' into \"non-hex input\" and \"hex-input, but\nreferring to a non-commit object\". By having 'write_commit_graph' take\nin a 'struct oidset *' of commits, we place the burden on the caller (in\nthis case, the builtin) to handle the first case, and the commit-graph\nmachinery can handle the second case.\n\nSuggested-by: Jeff King \nSigned-off-by: Taylor Blau \n---\n builtin/commit-graph.c | 19 ++++++++++---\n commit-graph.c | 59 +++++++++++++++++++++++------------------\n commit-graph.h | 3 ++-\n t/t5318-commit-graph.sh | 2 +-\n 4 files changed, 52 insertions(+), 31 deletions(-)\n\ndiff --git a/builtin/commit-graph.c b/builtin/commit-graph.c\nindex 922f876bfa..c69716aa7e 100644\n--- a/builtin/commit-graph.c\n+++ b/builtin/commit-graph.c\n@@ -140,7 +140,7 @@ static int write_option_parse_split(const struct option *opt, const char *arg,\n static int graph_write(int argc, const char **argv)\n {\n \tstruct string_list *pack_indexes = NULL;\n-\tstruct string_list *commit_hex = NULL;\n+\tstruct oidset commits = OIDSET_INIT;\n \tstruct object_directory *odb = NULL;\n \tstruct string_list lines;\n \tint result = 0;\n@@ -213,7 +213,20 @@ static int graph_write(int argc, const char **argv)\n \t\tif (opts.stdin_packs)\n \t\t\tpack_indexes = &lines;\n \t\tif (opts.stdin_commits) {\n-\t\t\tcommit_hex = &lines;\n+\t\t\tstruct string_list_item *item;\n+\t\t\toidset_init(&commits, lines.nr);\n+\t\t\tfor_each_string_list_item(item, &lines) {\n+\t\t\t\tstruct object_id oid;\n+\t\t\t\tconst char *end;\n+\n+\t\t\t\tif (parse_oid_hex(item->string, &oid, &end)) {\n+\t\t\t\t\terror(_(\"unexpected non-hex object ID: \"\n+\t\t\t\t\t\t\"%s\"), item->string);\n+\t\t\t\t\treturn 1;\n+\t\t\t\t}\n+\n+\t\t\t\toidset_insert(&commits, &oid);\n+\t\t\t}\n \t\t\tflags |= COMMIT_GRAPH_WRITE_CHECK_OIDS;\n \t\t}\n \n@@ -222,7 +235,7 @@ static int graph_write(int argc, const char **argv)\n \n \tif (write_commit_graph(odb,\n \t\t\t pack_indexes,\n-\t\t\t commit_hex,\n+\t\t\t opts.stdin_commits ? &commits : NULL,\n \t\t\t flags,\n \t\t\t &split_opts))\n \t\tresult = 1;\ndiff --git a/commit-graph.c b/commit-graph.c\nindex c598508d7f..f60346baee 100644\n--- a/commit-graph.c\n+++ b/commit-graph.c\n@@ -1136,13 +1136,13 @@ static void compute_generation_numbers(struct write_commit_graph_context *ctx)\n \tstop_progress(&ctx->progress);\n }\n \n-static int add_ref_to_list(const char *refname,\n-\t\t\t const struct object_id *oid,\n-\t\t\t int flags, void *cb_data)\n+static int add_ref_to_set(const char *refname,\n+\t\t\t const struct object_id *oid,\n+\t\t\t int flags, void *cb_data)\n {\n-\tstruct string_list *list = (struct string_list *)cb_data;\n+\tstruct oidset *commits = (struct oidset *)cb_data;\n \n-\tstring_list_append(list, oid_to_hex(oid));\n+\toidset_insert(commits, oid);\n \treturn 0;\n }\n \n@@ -1150,14 +1150,14 @@ int write_commit_graph_reachable(struct object_directory *odb,\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+\tstruct oidset commits = OIDSET_INIT;\n \tint result;\n \n-\tfor_each_ref(add_ref_to_list, &list);\n-\tresult = write_commit_graph(odb, NULL, &list,\n+\tfor_each_ref(add_ref_to_set, &commits);\n+\tresult = write_commit_graph(odb, NULL, &commits,\n \t\t\t\t flags, split_opts);\n \n-\tstring_list_clear(&list, 0);\n+\toidset_clear(&commits);\n \treturn result;\n }\n \n@@ -1206,39 +1206,46 @@ static int fill_oids_from_packs(struct write_commit_graph_context *ctx,\n \treturn 0;\n }\n \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+static int fill_oids_from_commits(struct write_commit_graph_context *ctx,\n+\t\t\t\t struct oidset *commits)\n {\n-\tuint32_t i;\n+\tuint32_t i = 0;\n \tstruct strbuf progress_title = STRBUF_INIT;\n+\tstruct oidset_iter iter;\n+\tstruct object_id *oid;\n+\n+\tif (!oidset_size(commits))\n+\t\treturn 0;\n \n \tif (ctx->report_progress) {\n \t\tstrbuf_addf(&progress_title,\n \t\t\t Q_(\"Finding commits for commit graph from %d ref\",\n \t\t\t \"Finding commits for commit graph from %d refs\",\n-\t\t\t commit_hex->nr),\n-\t\t\t commit_hex->nr);\n+\t\t\t oidset_size(commits)),\n+\t\t\t oidset_size(commits));\n \t\tctx->progress = start_delayed_progress(\n \t\t\t\t\tprogress_title.buf,\n-\t\t\t\t\tcommit_hex->nr);\n+\t\t\t\t\toidset_size(commits));\n \t}\n-\tfor (i = 0; i < commit_hex->nr; i++) {\n-\t\tconst char *end;\n-\t\tstruct object_id oid;\n+\n+\toidset_iter_init(commits, &iter);\n+\twhile ((oid = oidset_iter_next(&iter))) {\n \t\tstruct commit *result;\n \n-\t\tdisplay_progress(ctx->progress, i + 1);\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\tdisplay_progress(ctx->progress, ++i);\n+\n+\t\tresult = lookup_commit_reference_gently(ctx->r, oid, 1);\n+\t\tif (result) {\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\t oid_to_hex(oid));\n \t\t\treturn -1;\n \t\t}\n \t}\n+\n \tstop_progress(&ctx->progress);\n \tstrbuf_release(&progress_title);\n \n@@ -1781,7 +1788,7 @@ static void expire_commit_graphs(struct write_commit_graph_context *ctx)\n \n int write_commit_graph(struct object_directory *odb,\n \t\t struct string_list *pack_indexes,\n-\t\t struct string_list *commit_hex,\n+\t\t struct oidset *commits,\n \t\t enum commit_graph_write_flags flags,\n \t\t const struct split_commit_graph_opts *split_opts)\n {\n@@ -1857,12 +1864,12 @@ int write_commit_graph(struct object_directory *odb,\n \t\t\tgoto cleanup;\n \t}\n \n-\tif (commit_hex) {\n-\t\tif ((res = fill_oids_from_commit_hex(ctx, commit_hex)))\n+\tif (commits) {\n+\t\tif ((res = fill_oids_from_commits(ctx, commits)))\n \t\t\tgoto cleanup;\n \t}\n \n-\tif (!pack_indexes && !commit_hex)\n+\tif (!pack_indexes && !commits)\n \t\tfill_oids_from_all_packs(ctx);\n \n \tclose_reachable(ctx);\ndiff --git a/commit-graph.h b/commit-graph.h\nindex 718433d79b..98ef121924 100644\n--- a/commit-graph.h\n+++ b/commit-graph.h\n@@ -6,6 +6,7 @@\n #include \"string-list.h\"\n #include \"cache.h\"\n #include \"object-store.h\"\n+#include \"oidset.h\"\n \n #define GIT_TEST_COMMIT_GRAPH \"GIT_TEST_COMMIT_GRAPH\"\n #define GIT_TEST_COMMIT_GRAPH_DIE_ON_LOAD \"GIT_TEST_COMMIT_GRAPH_DIE_ON_LOAD\"\n@@ -106,7 +107,7 @@ int write_commit_graph_reachable(struct object_directory *odb,\n \t\t\t\t const struct split_commit_graph_opts *split_opts);\n int write_commit_graph(struct object_directory *odb,\n \t\t struct string_list *pack_indexes,\n-\t\t struct string_list *commit_hex,\n+\t\t struct oidset *commits,\n \t\t enum commit_graph_write_flags flags,\n \t\t const struct split_commit_graph_opts *split_opts);\n \ndiff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh\nindex 9bf920ae17..e874a12696 100755\n--- a/t/t5318-commit-graph.sh\n+++ b/t/t5318-commit-graph.sh\n@@ -43,7 +43,7 @@ test_expect_success 'create commits and repack' '\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+\ttest_i18ngrep \"unexpected non-hex object ID: HEAD\" 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-- \n2.26.0.106.g9fadedd637\n\n"},{"id":"395396","messageId":"1ff42f4c3d568dd25889d2808cda3edf38a36cb9.1586836700.git.me@ttaylorr.com","threadId":"53223","inReplyTo":"cover.1586836700.git.me@ttaylorr.com","subject":"[PATCH 7/7] commit-graph.c: introduce '--[no-]check-oids'","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-04-14T04:04:29Z","receivedAt":"2020-04-14T04:04:34Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"When operating on a stream of commit OIDs on stdin, 'git commit-graph\nwrite' checks that each OID refers to an object that is indeed a commit.\nThis is convenient to make sure that the given input is well-formed, but\ncan sometimes be undesirable.\n\nFor example, server operators may wish to feed the refnames that were\nupdated during a push to 'git commit-graph write --input=stdin-commits',\nand silently discard refs that don't point at commits. This can be done\nby combing the output of 'git for-each-ref' with '--format\n%(*objecttype)', but this requires opening up a potentially large number\nof objects. Instead, it is more convenient to feed the updated refs to\nthe commit-graph machinery, and let it throw out refs that don't point\nto commits.\n\nIntroduce '--[no-]check-oids' to make such a behavior possible. With\n'--check-oids' (the default behavior to retain backwards compatibility),\n'git commit-graph write' will barf on a non-commit line in its input.\nWith 'no-check-oids', such lines will be silently ignored, making the\nabove possible by specifying this option.\n\nNo matter which is supplied, 'git commit-graph write' retains the\nbehavior from the previous commit of rejecting non-OID inputs like\n\"HEAD\" and \"refs/heads/foo\" as before.\n\nSigned-off-by: Taylor Blau \n---\n Documentation/git-commit-graph.txt | 5 +++++\n builtin/commit-graph.c | 11 ++++++++---\n t/t5318-commit-graph.sh | 28 ++++++++++++++++++++++++++++\n 3 files changed, 41 insertions(+), 3 deletions(-)\n\ndiff --git a/Documentation/git-commit-graph.txt b/Documentation/git-commit-graph.txt\nindex 46f7f7c573..91e8027b86 100644\n--- a/Documentation/git-commit-graph.txt\n+++ b/Documentation/git-commit-graph.txt\n@@ -82,6 +82,11 @@ tip with the previous tip.\n Finally, if `--expire-time=` is not specified, let `datetime`\n be the current time. After writing the split commit-graph, delete all\n unused commit-graph whose modified times are older than `datetime`.\n++\n+The `--[no-]check-oids` option decides whether or not OIDs are required\n+to be commits. By default, `--check-oids` is implied, generating an\n+error on non-commit objects. If `--no-check-oids` is given, non-commits\n+are silently discarded.\n \n 'verify'::\n \ndiff --git a/builtin/commit-graph.c b/builtin/commit-graph.c\nindex c69716aa7e..2d0a8e822a 100644\n--- a/builtin/commit-graph.c\n+++ b/builtin/commit-graph.c\n@@ -11,7 +11,7 @@ static char const * const builtin_commit_graph_usage[] = {\n \tN_(\"git commit-graph verify [--object-dir ] [--shallow] [--[no-]progress]\"),\n \tN_(\"git commit-graph write [--object-dir ] [--append] \"\n \t \"[--split[=]] [--reachable|--stdin-packs|--stdin-commits] \"\n-\t \"[--[no-]progress] \"),\n+\t \"[--[no-]progress] [--[no-]check-oids] \"),\n \tNULL\n };\n \n@@ -23,7 +23,7 @@ static const char * const builtin_commit_graph_verify_usage[] = {\n static const char * const builtin_commit_graph_write_usage[] = {\n \tN_(\"git commit-graph write [--object-dir ] [--append] \"\n \t \"[--split[=]] [--reachable|--stdin-packs|--stdin-commits] \"\n-\t \"[--[no-]progress] \"),\n+\t \"[--[no-]progress] [--[no-]check-oids] \"),\n \tNULL\n };\n \n@@ -36,6 +36,7 @@ static struct opts_commit_graph {\n \tint split;\n \tint shallow;\n \tint progress;\n+\tint check_oids;\n } opts;\n \n static struct object_directory *find_odb(struct repository *r,\n@@ -163,6 +164,8 @@ static int graph_write(int argc, const char **argv)\n \t\t\tN_(\"allow writing an incremental commit-graph file\"),\n \t\t\tPARSE_OPT_OPTARG | PARSE_OPT_NONEG,\n \t\t\twrite_option_parse_split),\n+\t\tOPT_BOOL(0, \"check-oids\", &opts.check_oids,\n+\t\t\tN_(\"require OIDs to be commits\")),\n \t\tOPT_INTEGER(0, \"max-commits\", &split_opts.max_commits,\n \t\t\tN_(\"maximum number of commits in a non-base split commit-graph\")),\n \t\tOPT_INTEGER(0, \"size-multiple\", &split_opts.size_multiple,\n@@ -173,6 +176,7 @@ static int graph_write(int argc, const char **argv)\n \t};\n \n \topts.progress = isatty(2);\n+\topts.check_oids = 1;\n \tsplit_opts.size_multiple = 2;\n \tsplit_opts.max_commits = 0;\n \tsplit_opts.expire_time = 0;\n@@ -227,7 +231,8 @@ static int graph_write(int argc, const char **argv)\n \n \t\t\t\toidset_insert(&commits, &oid);\n \t\t\t}\n-\t\t\tflags |= COMMIT_GRAPH_WRITE_CHECK_OIDS;\n+\t\t\tif (opts.check_oids)\n+\t\t\t\tflags |= COMMIT_GRAPH_WRITE_CHECK_OIDS;\n \t\t}\n \n \t\tUNLEAK(buf);\ndiff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh\nindex e874a12696..2cbd301abe 100755\n--- a/t/t5318-commit-graph.sh\n+++ b/t/t5318-commit-graph.sh\n@@ -49,6 +49,34 @@ test_expect_success 'exit with correct error on bad input to --stdin-commits' '\n \ttest_i18ngrep \"invalid commit object id\" stderr\n '\n \n+graph_expect_commits() {\n+\ttest-tool read-graph >got\n+\tif ! grep \"num_commits: $1\" got\n+\tthen\n+\t\techo \"graph_expect_commits: expected $1 commit(s), got:\"\n+\t\tcat got\n+\t\tfalse\n+\tfi\n+}\n+\n+test_expect_success 'ignores non-commit OIDs to --input=stdin-commits with --no-check-oids' '\n+\ttest_when_finished rm -rf \"$objdir/info/commit-graph\" &&\n+\tcd \"$TRASH_DIRECTORY/full\" &&\n+\t# write a graph to ensure layers are/are not added appropriately\n+\tgit rev-parse HEAD~1 >base &&\n+\tgit commit-graph write --stdin-commits bad &&\n+\ttest_expect_code 1 git commit-graph write --stdin-commits err &&\n+\ttest_i18ngrep \"invalid commit object id\" err &&\n+\tgraph_expect_commits 2 &&\n+\t# update with valid commit OID, ignore tree OID\n+\tgit rev-parse HEAD HEAD^{tree} >in &&\n+\tgit commit-graph write --stdin-commits --no-check-oids output\n \tgit -c core.commitGraph=false $1 >expect\n-- \n2.26.0.106.g9fadedd637\n"},{"id":"395471","messageId":"20200415042930.GA11703@syl.local","threadId":"53223","inReplyTo":"1ff42f4c3d568dd25889d2808cda3edf38a36cb9.1586836700.git.me@ttaylorr.com","subject":"Re: [PATCH 7/7] commit-graph.c: introduce '--[no-]check-oids'","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-04-15T04:29:30Z","receivedAt":"2020-04-15T04:29:37Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"Whoops. I sent the wrong version of this patch. It should be the below:\n\n--- >8 ---\n\nSubject: [PATCH] shallow.c: use 'reset_repository_shallow' when appropriate\n\nIn bd0b42aed3 (fetch-pack: do not take shallow lock unnecessarily,\n2019-01-10), the author noted that 'is_repository_shallow' produces\nvisible side-effect(s) by setting 'is_shallow' and 'shallow_stat'.\n\nThis is a problem for e.g., fetching with '--update-shallow' in a\nshallow repsoitory with 'fetch.writeCommitGraph' enabled, since the\nupdate to '.git/shallow' will cause Git to think that the repository\n*isn't* shallow when it is, thereby circumventing the commit-graph\ncompatability check.\n\nThis causes problems in shallow repositories with at least shallow refs\nthat have at least one ancestor (since the client won't have those\nobject(s), and therefore can't take the reachability closure over\ncommits to be written to the commit-graph).\n\nAddress this by introducing 'reset_repository_shallow()', and calling it\nwhen the shallow file is updated, forcing 'is_repository_shallow' to\nre-evaluate whether the repository is still shallow after fetching in\nthe above scenario.\n\nSigned-off-by: Taylor Blau \n---\n commit.h | 1 +\n fetch-pack.c | 1 +\n shallow.c | 15 ++++++++-------\n 3 files changed, 10 insertions(+), 7 deletions(-)\n\ndiff --git a/commit.h b/commit.h\nindex 008a0fa4a0..ee1ba139d4 100644\n--- a/commit.h\n+++ b/commit.h\n@@ -251,6 +251,7 @@ int register_shallow(struct repository *r, const struct object_id *oid);\n int unregister_shallow(const struct object_id *oid);\n int for_each_commit_graft(each_commit_graft_fn, void *);\n int is_repository_shallow(struct repository *r);\n+void reset_repository_shallow(struct repository *r);\n struct commit_list *get_shallow_commits(struct object_array *heads,\n \t\t\t\t\tint depth, int shallow_flag, int not_shallow_flag);\n struct commit_list *get_shallow_commits_by_rev_list(\ndiff --git a/fetch-pack.c b/fetch-pack.c\nindex 1734a573b0..051902ef6d 100644\n--- a/fetch-pack.c\n+++ b/fetch-pack.c\n@@ -1630,6 +1630,7 @@ static void update_shallow(struct fetch_pack_args *args,\n \t\tif (*alternate_shallow_file == '\\0') { /* --unshallow */\n \t\t\tunlink_or_warn(git_path_shallow(the_repository));\n \t\t\trollback_lock_file(&shallow_lock);\n+\t\t\treset_repository_shallow(the_repository);\n \t\t} else\n \t\t\tcommit_lock_file(&shallow_lock);\n \t\talternate_shallow_file = NULL;\ndiff --git a/shallow.c b/shallow.c\nindex 7fd04afed1..fac383dec9 100644\n--- a/shallow.c\n+++ b/shallow.c\n@@ -40,13 +40,6 @@ int register_shallow(struct repository *r, const struct object_id *oid)\n\n int is_repository_shallow(struct repository *r)\n {\n-\t/*\n-\t * NEEDSWORK: This function updates\n-\t * r->parsed_objects->{is_shallow,shallow_stat} as a side effect but\n-\t * there is no corresponding function to clear them when the shallow\n-\t * file is updated.\n-\t */\n-\n \tFILE *fp;\n \tchar buf[1024];\n \tconst char *path = r->parsed_objects->alternate_shallow_file;\n@@ -79,6 +72,12 @@ int is_repository_shallow(struct repository *r)\n \treturn r->parsed_objects->is_shallow;\n }\n\n+void reset_repository_shallow(struct repository *r)\n+{\n+\tr->parsed_objects->is_shallow = -1;\n+\tstat_validity_clear(r->parsed_objects->shallow_stat);\n+}\n+\n /*\n * TODO: use \"int\" elemtype instead of \"int *\" when/if commit-slab\n * supports a \"valid\" flag.\n@@ -362,6 +361,7 @@ void setup_alternate_shallow(struct lock_file *shallow_lock,\n \t\t * shallow file\".\n \t\t */\n \t\t*alternate_shallow_file = \"\";\n+\treset_repository_shallow(the_repository);\n \tstrbuf_release(&sb);\n }\n\n@@ -411,6 +411,7 @@ void prune_shallow(unsigned options)\n \t\t\tdie_errno(\"failed to write to %s\",\n \t\t\t\t get_lock_file_path(&shallow_lock));\n \t\tcommit_lock_file(&shallow_lock);\n+\t\treset_repository_shallow(the_repository);\n \t} else {\n \t\tunlink(git_path_shallow(the_repository));\n \t\trollback_lock_file(&shallow_lock);\n--\n2.26.0.106.g9fadedd637\n"},{"id":"395472","messageId":"20200415043137.GA12136@syl.local","threadId":"53223","inReplyTo":"20200415042930.GA11703@syl.local","subject":"Re: [PATCH 7/7] commit-graph.c: introduce '--[no-]check-oids'","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-04-15T04:31:37Z","receivedAt":"2020-04-15T04:31:44Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Tue, Apr 14, 2020 at 10:29:30PM -0600, Taylor Blau wrote:\n> Whoops. I sent the wrong version of this patch. It should be the below:\n\nDouble whoops. I was on the wrong branch, and hit send too early. *This*\nis the version of the patch that I meant to send ;).\n\n--- >8 ---\n\nSubject: [PATCH] commit-graph.c: introduce '--[no-]check-oids'\n\nWhen operating on a stream of commit OIDs on stdin, 'git commit-graph\nwrite' checks that each OID refers to an object that is indeed a commit.\nThis is convenient to make sure that the given input is well-formed, but\ncan sometimes be undesirable.\n\nFor example, server operators may wish to feed the refnames that were\nupdated during a push to 'git commit-graph write --input=stdin-commits',\nand silently discard refs that don't point at commits. This can be done\nby combing the output of 'git for-each-ref' with '--format\n%(*objecttype)', but this requires opening up a potentially large number\nof objects. Instead, it is more convenient to feed the updated refs to\nthe commit-graph machinery, and let it throw out refs that don't point\nto commits.\n\nIntroduce '--[no-]check-oids' to make such a behavior possible. With\n'--check-oids' (the default behavior to retain backwards compatibility),\n'git commit-graph write' will barf on a non-commit line in its input.\nWith 'no-check-oids', such lines will be silently ignored, making the\nabove possible by specifying this option.\n\nNo matter which is supplied, 'git commit-graph write' retains the\nbehavior from the previous commit of rejecting non-OID inputs like\n\"HEAD\" and \"refs/heads/foo\" as before.\n\nSigned-off-by: Taylor Blau \n---\n Documentation/git-commit-graph.txt | 5 +++++\n builtin/commit-graph.c | 11 ++++++++---\n commit-graph.c | 2 +-\n t/t5318-commit-graph.sh | 28 ++++++++++++++++++++++++++++\n 4 files changed, 42 insertions(+), 4 deletions(-)\n\ndiff --git a/Documentation/git-commit-graph.txt b/Documentation/git-commit-graph.txt\nindex 46f7f7c573..91e8027b86 100644\n--- a/Documentation/git-commit-graph.txt\n+++ b/Documentation/git-commit-graph.txt\n@@ -82,6 +82,11 @@ tip with the previous tip.\n Finally, if `--expire-time=` is not specified, let `datetime`\n be the current time. After writing the split commit-graph, delete all\n unused commit-graph whose modified times are older than `datetime`.\n++\n+The `--[no-]check-oids` option decides whether or not OIDs are required\n+to be commits. By default, `--check-oids` is implied, generating an\n+error on non-commit objects. If `--no-check-oids` is given, non-commits\n+are silently discarded.\n\n 'verify'::\n\ndiff --git a/builtin/commit-graph.c b/builtin/commit-graph.c\nindex c69716aa7e..2d0a8e822a 100644\n--- a/builtin/commit-graph.c\n+++ b/builtin/commit-graph.c\n@@ -11,7 +11,7 @@ static char const * const builtin_commit_graph_usage[] = {\n \tN_(\"git commit-graph verify [--object-dir ] [--shallow] [--[no-]progress]\"),\n \tN_(\"git commit-graph write [--object-dir ] [--append] \"\n \t \"[--split[=]] [--reachable|--stdin-packs|--stdin-commits] \"\n-\t \"[--[no-]progress] \"),\n+\t \"[--[no-]progress] [--[no-]check-oids] \"),\n \tNULL\n };\n\n@@ -23,7 +23,7 @@ static const char * const builtin_commit_graph_verify_usage[] = {\n static const char * const builtin_commit_graph_write_usage[] = {\n \tN_(\"git commit-graph write [--object-dir ] [--append] \"\n \t \"[--split[=]] [--reachable|--stdin-packs|--stdin-commits] \"\n-\t \"[--[no-]progress] \"),\n+\t \"[--[no-]progress] [--[no-]check-oids] \"),\n \tNULL\n };\n\n@@ -36,6 +36,7 @@ static struct opts_commit_graph {\n \tint split;\n \tint shallow;\n \tint progress;\n+\tint check_oids;\n } opts;\n\n static struct object_directory *find_odb(struct repository *r,\n@@ -163,6 +164,8 @@ static int graph_write(int argc, const char **argv)\n \t\t\tN_(\"allow writing an incremental commit-graph file\"),\n \t\t\tPARSE_OPT_OPTARG | PARSE_OPT_NONEG,\n \t\t\twrite_option_parse_split),\n+\t\tOPT_BOOL(0, \"check-oids\", &opts.check_oids,\n+\t\t\tN_(\"require OIDs to be commits\")),\n \t\tOPT_INTEGER(0, \"max-commits\", &split_opts.max_commits,\n \t\t\tN_(\"maximum number of commits in a non-base split commit-graph\")),\n \t\tOPT_INTEGER(0, \"size-multiple\", &split_opts.size_multiple,\n@@ -173,6 +176,7 @@ static int graph_write(int argc, const char **argv)\n \t};\n\n \topts.progress = isatty(2);\n+\topts.check_oids = 1;\n \tsplit_opts.size_multiple = 2;\n \tsplit_opts.max_commits = 0;\n \tsplit_opts.expire_time = 0;\n@@ -227,7 +231,8 @@ static int graph_write(int argc, const char **argv)\n\n \t\t\t\toidset_insert(&commits, &oid);\n \t\t\t}\n-\t\t\tflags |= COMMIT_GRAPH_WRITE_CHECK_OIDS;\n+\t\t\tif (opts.check_oids)\n+\t\t\t\tflags |= COMMIT_GRAPH_WRITE_CHECK_OIDS;\n \t\t}\n\n \t\tUNLEAK(buf);\ndiff --git a/commit-graph.c b/commit-graph.c\nindex f60346baee..b8737f0ce9 100644\n--- a/commit-graph.c\n+++ b/commit-graph.c\n@@ -145,7 +145,7 @@ static int verify_commit_graph_lite(struct commit_graph *g)\n \t *\n \t * There should only be very basic checks here to ensure that\n \t * we don't e.g. segfault in fill_commit_in_graph(), but\n-\t * because this is a very hot codepath nothing that e.g. loops\n+\t e because this is a very hot codepath nothing that e.g. loops\n \t * over g->num_commits, or runs a checksum on the commit-graph\n \t * itself.\n \t */\ndiff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh\nindex e874a12696..7960cefa1b 100755\n--- a/t/t5318-commit-graph.sh\n+++ b/t/t5318-commit-graph.sh\n@@ -49,6 +49,34 @@ test_expect_success 'exit with correct error on bad input to --stdin-commits' '\n \ttest_i18ngrep \"invalid commit object id\" stderr\n '\n\n+graph_expect_commits() {\n+\ttest-tool read-graph >got\n+\tif ! grep \"num_commits: $1\" got\n+\tthen\n+\t\techo \"graph_expect_commits: expected $1 commit(s), got:\"\n+\t\tcat got\n+\t\tfalse\n+\tfi\n+}\n+\n+test_expect_success 'ignores non-commit OIDs to --input=stdin-commits with --no-check-oids' '\n+\ttest_when_finished rm -rf \"$objdir/info/commit-graph\" &&\n+\tcd \"$TRASH_DIRECTORY/full\" &&\n+\t# write a graph to ensure layers are/are not added appropriately\n+\tgit rev-parse HEAD~1 >base &&\n+\tgit commit-graph write --stdin-commits bad &&\n+\ttest_expect_code 1 git commit-graph write --stdin-commits err &&\n+\ttest_i18ngrep \"unexpected non-hex object ID: HEAD\" err &&\n+\tgraph_expect_commits 2 &&\n+\t# update with valid commit OID, ignore tree OID\n+\tgit rev-parse HEAD HEAD^{tree} >in &&\n+\tgit commit-graph write --stdin-commits --no-check-oids output\n \tgit -c core.commitGraph=false $1 >expect\n--\n2.26.0.106.g9fadedd637\n\n"},{"id":"395895","messageId":"20200422105536.GB3063@szeder.dev","threadId":"53223","inReplyTo":"20200415043137.GA12136@syl.local","subject":"Re: [PATCH 7/7] commit-graph.c: introduce '--[no-]check-oids'","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2020-04-22T10:55:36Z","receivedAt":"2020-04-22T10:55:45Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Tue, Apr 14, 2020 at 10:31:37PM -0600, Taylor Blau wrote:\n> On Tue, Apr 14, 2020 at 10:29:30PM -0600, Taylor Blau wrote:\n> > Whoops. I sent the wrong version of this patch. It should be the below:\n> \n> Double whoops. I was on the wrong branch, and hit send too early. *This*\n> is the version of the patch that I meant to send ;).\n> \n> --- >8 ---\n> \n> Subject: [PATCH] commit-graph.c: introduce '--[no-]check-oids'\n> \n> When operating on a stream of commit OIDs on stdin, 'git commit-graph\n> write' checks that each OID refers to an object that is indeed a commit.\n> This is convenient to make sure that the given input is well-formed, but\n> can sometimes be undesirable.\n> \n> For example, server operators may wish to feed the refnames that were\n\ns/the refnames/full commit object IDs pointed to by refs/\n\nor something similar.\n\n> updated during a push to 'git commit-graph write --input=stdin-commits',\n> and silently discard refs that don't point at commits.\n\ns/refs//\n\n> This can be done\n> by combing the output of 'git for-each-ref' with '--format\n> %(*objecttype)', but this requires opening up a potentially large number\n> of objects. Instead, it is more convenient to feed the updated refs to\n\ns/refs/.../\n\n> the commit-graph machinery, and let it throw out refs that don't point\n\ns/refs/.../\n\n> to commits.\n> \n> Introduce '--[no-]check-oids' to make such a behavior possible. With\n> '--check-oids' (the default behavior to retain backwards compatibility),\n> 'git commit-graph write' will barf on a non-commit line in its input.\n> With 'no-check-oids', such lines will be silently ignored, making the\n\ns/no-check-oids/--no-check-oids/\n\n> above possible by specifying this option.\n> \n> No matter which is supplied, 'git commit-graph write' retains the\n> behavior from the previous commit of rejecting non-OID inputs like\n> \"HEAD\" and \"refs/heads/foo\" as before.\n\nSee? :) This is why all those s/// are necessary.\n\n> Signed-off-by: Taylor Blau \n> ---\n> Documentation/git-commit-graph.txt | 5 +++++\n> builtin/commit-graph.c | 11 ++++++++---\n> commit-graph.c | 2 +-\n> t/t5318-commit-graph.sh | 28 ++++++++++++++++++++++++++++\n> 4 files changed, 42 insertions(+), 4 deletions(-)\n> \n> diff --git a/Documentation/git-commit-graph.txt b/Documentation/git-commit-graph.txt\n> index 46f7f7c573..91e8027b86 100644\n> --- a/Documentation/git-commit-graph.txt\n> +++ b/Documentation/git-commit-graph.txt\n> @@ -82,6 +82,11 @@ tip with the previous tip.\n> Finally, if `--expire-time=` is not specified, let `datetime`\n> be the current time. After writing the split commit-graph, delete all\n> unused commit-graph whose modified times are older than `datetime`.\n> ++\n> +The `--[no-]check-oids` option decides whether or not OIDs are required\n> +to be commits. By default, `--check-oids` is implied, generating an\n> +error on non-commit objects. If `--no-check-oids` is given, non-commits\n> +are silently discarded.\n\nWhat happens with OIDs of tags, in particular with OIDs of tags that\ncan be peeled down to commit objects? According to (my (too\npedantic?) interpretation of) the above description they will trigger\nan error with '--check-oids' or will be ignored with\n'--no-check-oids'. The implementation, however, accepts those oids\nand peels them down to commit objects; I think this is the right\nbehaviour.\n\nWhat happens with OIDs that name non-existing objects?\n\n> 'verify'::\n> \n> diff --git a/builtin/commit-graph.c b/builtin/commit-graph.c\n> index c69716aa7e..2d0a8e822a 100644\n> --- a/builtin/commit-graph.c\n> +++ b/builtin/commit-graph.c\n> @@ -11,7 +11,7 @@ static char const * const builtin_commit_graph_usage[] = {\n> \tN_(\"git commit-graph verify [--object-dir ] [--shallow] [--[no-]progress]\"),\n> \tN_(\"git commit-graph write [--object-dir ] [--append] \"\n> \t \"[--split[=]] [--reachable|--stdin-packs|--stdin-commits] \"\n> -\t \"[--[no-]progress] \"),\n> +\t \"[--[no-]progress] [--[no-]check-oids] \"),\n> \tNULL\n> };\n> \n> @@ -23,7 +23,7 @@ static const char * const builtin_commit_graph_verify_usage[] = {\n> static const char * const builtin_commit_graph_write_usage[] = {\n> \tN_(\"git commit-graph write [--object-dir ] [--append] \"\n> \t \"[--split[=]] [--reachable|--stdin-packs|--stdin-commits] \"\n> -\t \"[--[no-]progress] \"),\n> +\t \"[--[no-]progress] [--[no-]check-oids] \"),\n> \tNULL\n> };\n> \n> @@ -36,6 +36,7 @@ static struct opts_commit_graph {\n> \tint split;\n> \tint shallow;\n> \tint progress;\n> +\tint check_oids;\n> } opts;\n> \n> static struct object_directory *find_odb(struct repository *r,\n> @@ -163,6 +164,8 @@ static int graph_write(int argc, const char **argv)\n> \t\t\tN_(\"allow writing an incremental commit-graph file\"),\n> \t\t\tPARSE_OPT_OPTARG | PARSE_OPT_NONEG,\n> \t\t\twrite_option_parse_split),\n> +\t\tOPT_BOOL(0, \"check-oids\", &opts.check_oids,\n> +\t\t\tN_(\"require OIDs to be commits\")),\n> \t\tOPT_INTEGER(0, \"max-commits\", &split_opts.max_commits,\n> \t\t\tN_(\"maximum number of commits in a non-base split commit-graph\")),\n> \t\tOPT_INTEGER(0, \"size-multiple\", &split_opts.size_multiple,\n> @@ -173,6 +176,7 @@ static int graph_write(int argc, const char **argv)\n> \t};\n> \n> \topts.progress = isatty(2);\n> +\topts.check_oids = 1;\n> \tsplit_opts.size_multiple = 2;\n> \tsplit_opts.max_commits = 0;\n> \tsplit_opts.expire_time = 0;\n> @@ -227,7 +231,8 @@ static int graph_write(int argc, const char **argv)\n> \n> \t\t\t\toidset_insert(&commits, &oid);\n> \t\t\t}\n> -\t\t\tflags |= COMMIT_GRAPH_WRITE_CHECK_OIDS;\n> +\t\t\tif (opts.check_oids)\n> +\t\t\t\tflags |= COMMIT_GRAPH_WRITE_CHECK_OIDS;\n> \t\t}\n> \n> \t\tUNLEAK(buf);\n> diff --git a/commit-graph.c b/commit-graph.c\n> index f60346baee..b8737f0ce9 100644\n> --- a/commit-graph.c\n> +++ b/commit-graph.c\n> @@ -145,7 +145,7 @@ static int verify_commit_graph_lite(struct commit_graph *g)\n> \t *\n> \t * There should only be very basic checks here to ensure that\n> \t * we don't e.g. segfault in fill_commit_in_graph(), but\n> -\t * because this is a very hot codepath nothing that e.g. loops\n> +\t e because this is a very hot codepath nothing that e.g. loops\n\nBogus hunk, perhaps?\n\n> \t * over g->num_commits, or runs a checksum on the commit-graph\n> \t * itself.\n> \t */\n> diff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh\n> index e874a12696..7960cefa1b 100755\n> --- a/t/t5318-commit-graph.sh\n> +++ b/t/t5318-commit-graph.sh\n> @@ -49,6 +49,34 @@ test_expect_success 'exit with correct error on bad input to --stdin-commits' '\n> \ttest_i18ngrep \"invalid commit object id\" stderr\n> '\n> \n> +graph_expect_commits() {\n> +\ttest-tool read-graph >got\n> +\tif ! grep \"num_commits: $1\" got\n> +\tthen\n> +\t\techo \"graph_expect_commits: expected $1 commit(s), got:\"\n> +\t\tcat got\n> +\t\tfalse\n> +\tfi\n> +}\n> +\n> +test_expect_success 'ignores non-commit OIDs to --input=stdin-commits with --no-check-oids' '\n> +\ttest_when_finished rm -rf \"$objdir/info/commit-graph\" &&\n> +\tcd \"$TRASH_DIRECTORY/full\" &&\n> +\t# write a graph to ensure layers are/are not added appropriately\n> +\tgit rev-parse HEAD~1 >base &&\n> +\tgit commit-graph write --stdin-commits +\tgraph_expect_commits 2 &&\n> +\t# bad input is rejected\n> +\techo HEAD >bad &&\n> +\ttest_expect_code 1 git commit-graph write --stdin-commits err &&\n> +\ttest_i18ngrep \"unexpected non-hex object ID: HEAD\" err &&\n> +\tgraph_expect_commits 2 &&\n> +\t# update with valid commit OID, ignore tree OID\n> +\tgit rev-parse HEAD HEAD^{tree} >in &&\n> +\tgit commit-graph write --stdin-commits --no-check-oids +\tgraph_expect_commits 3\n> +'\n> +\n> graph_git_two_modes() {\n> \tgit -c core.commitGraph=true $1 >output\n> \tgit -c core.commitGraph=false $1 >expect\n> --\n> 2.26.0.106.g9fadedd637\n> \n"},{"id":"395964","messageId":"20200422233930.GB19100@syl.local","threadId":"53223","inReplyTo":"20200422105536.GB3063@szeder.dev","subject":"Re: [PATCH 7/7] commit-graph.c: introduce '--[no-]check-oids'","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-04-22T23:39:30Z","receivedAt":"2020-04-22T23:39:35Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Wed, Apr 22, 2020 at 12:55:36PM +0200, SZEDER Gábor wrote:\n> On Tue, Apr 14, 2020 at 10:31:37PM -0600, Taylor Blau wrote:\n> > On Tue, Apr 14, 2020 at 10:29:30PM -0600, Taylor Blau wrote:\n> > > Whoops. I sent the wrong version of this patch. It should be the below:\n> >\n> > Double whoops. I was on the wrong branch, and hit send too early. *This*\n> > is the version of the patch that I meant to send ;).\n> >\n> > --- >8 ---\n> >\n> > Subject: [PATCH] commit-graph.c: introduce '--[no-]check-oids'\n> >\n> > When operating on a stream of commit OIDs on stdin, 'git commit-graph\n> > write' checks that each OID refers to an object that is indeed a commit.\n> > This is convenient to make sure that the given input is well-formed, but\n> > can sometimes be undesirable.\n> >\n> > For example, server operators may wish to feed the refnames that were\n>\n> s/the refnames/full commit object IDs pointed to by refs/\n>\n> or something similar.\n>\n> > updated during a push to 'git commit-graph write --input=stdin-commits',\n> > and silently discard refs that don't point at commits.\n>\n> s/refs//\n>\n> > This can be done\n> > by combing the output of 'git for-each-ref' with '--format\n> > %(*objecttype)', but this requires opening up a potentially large number\n> > of objects. Instead, it is more convenient to feed the updated refs to\n>\n> s/refs/.../\n>\n> > the commit-graph machinery, and let it throw out refs that don't point\n>\n> s/refs/.../\n>\n> > to commits.\n> >\n> > Introduce '--[no-]check-oids' to make such a behavior possible. With\n> > '--check-oids' (the default behavior to retain backwards compatibility),\n> > 'git commit-graph write' will barf on a non-commit line in its input.\n> > With 'no-check-oids', such lines will be silently ignored, making the\n>\n> s/no-check-oids/--no-check-oids/\n>\n> > above possible by specifying this option.\n> >\n> > No matter which is supplied, 'git commit-graph write' retains the\n> > behavior from the previous commit of rejecting non-OID inputs like\n> > \"HEAD\" and \"refs/heads/foo\" as before.\n>\n> See? :) This is why all those s/// are necessary.\n\nAh :). All good points; thanks for your suggestions.\n\n> > Signed-off-by: Taylor Blau \n> > ---\n> > Documentation/git-commit-graph.txt | 5 +++++\n> > builtin/commit-graph.c | 11 ++++++++---\n> > commit-graph.c | 2 +-\n> > t/t5318-commit-graph.sh | 28 ++++++++++++++++++++++++++++\n> > 4 files changed, 42 insertions(+), 4 deletions(-)\n> >\n> > diff --git a/Documentation/git-commit-graph.txt b/Documentation/git-commit-graph.txt\n> > index 46f7f7c573..91e8027b86 100644\n> > --- a/Documentation/git-commit-graph.txt\n> > +++ b/Documentation/git-commit-graph.txt\n> > @@ -82,6 +82,11 @@ tip with the previous tip.\n> > Finally, if `--expire-time=` is not specified, let `datetime`\n> > be the current time. After writing the split commit-graph, delete all\n> > unused commit-graph whose modified times are older than `datetime`.\n> > ++\n> > +The `--[no-]check-oids` option decides whether or not OIDs are required\n> > +to be commits. By default, `--check-oids` is implied, generating an\n> > +error on non-commit objects. If `--no-check-oids` is given, non-commits\n> > +are silently discarded.\n>\n> What happens with OIDs of tags, in particular with OIDs of tags that\n> can be peeled down to commit objects? According to (my (too\n> pedantic?) interpretation of) the above description they will trigger\n> an error with '--check-oids' or will be ignored with\n> '--no-check-oids'. The implementation, however, accepts those oids\n> and peels them down to commit objects; I think this is the right\n> behaviour.\n\nThat's right, and certainly merits a mention in the documentation. I've\nadded that...\n\n> What happens with OIDs that name non-existing objects?\n\n...these are silently discarded. I think that you could make a\ncompelling argument in either direction on this one, but I'm slightly\nswayed towards \"discard these, too\", since '--no-check-oids' is\nliterally saying \"don't check these\".\n\nI guess that pushes us into the territory of whether or not \"check\" is\nthe right verb. \"verify\"? \"scrutinize\" :)? Do you have any thoughts\nhere?\n\nIf you're otherwise satisfied with this series, here's the updated\npatch.\n\n-- >8 --\n\nSubject: [PATCH] commit-graph.c: introduce '--[no-]check-oids'\n\nWhen operating on a stream of commit OIDs on stdin, 'git commit-graph\nwrite' checks that each OID refers to an object that is indeed a commit.\nThis is convenient to make sure that the given input is well-formed, but\ncan sometimes be undesirable.\n\nFor example, server operators may wish to feed the full commit object\nIDs pointed to by refs that were updated during a push to 'git\ncommit-graph write --input=stdin-commits', and silently discard any\ninput that doesn't point at a commit. This can be done by combing the\noutput of 'git for-each-ref' with '--format %(*objecttype)', but this\nrequires opening up a potentially large number of objects. Instead, it\nis more convenient to feed the updated object IDs to the commit-graph\nmachinery, and let it throw out whatever remains. to commits.\n\nIntroduce '--[no-]check-oids' to make such a behavior possible. With\n'--check-oids' (the default behavior to retain backwards compatibility),\n'git commit-graph write' will barf on a non-commit line in its input.\nWith '--no-check-oids', such lines will be silently ignored, making the\nabove possible by specifying this option.\n\nNo matter which is supplied, 'git commit-graph write' retains the\nbehavior from the previous commit of rejecting non-OID inputs like\n\"HEAD\" and \"refs/heads/foo\" as before.\n\nSigned-off-by: Taylor Blau \n---\n Documentation/git-commit-graph.txt | 6 ++++++\n builtin/commit-graph.c | 11 ++++++++---\n t/t5318-commit-graph.sh | 28 ++++++++++++++++++++++++++++\n 3 files changed, 42 insertions(+), 3 deletions(-)\n\ndiff --git a/Documentation/git-commit-graph.txt b/Documentation/git-commit-graph.txt\nindex 46f7f7c573..6bdbe42766 100644\n--- a/Documentation/git-commit-graph.txt\n+++ b/Documentation/git-commit-graph.txt\n@@ -82,6 +82,12 @@ tip with the previous tip.\n Finally, if `--expire-time=` is not specified, let `datetime`\n be the current time. After writing the split commit-graph, delete all\n unused commit-graph whose modified times are older than `datetime`.\n++\n+The `--[no-]check-oids` option decides whether or not OIDs are required\n+to be commits. By default, `--check-oids` is implied, generating an\n+error on non-commit objects. If `--no-check-oids` is given, non-commits\n+and non-existent objects are silently discarded. In either case, tags\n+are peeled down to the object they reference.\n\n 'verify'::\n\ndiff --git a/builtin/commit-graph.c b/builtin/commit-graph.c\nindex c69716aa7e..2d0a8e822a 100644\n--- a/builtin/commit-graph.c\n+++ b/builtin/commit-graph.c\n@@ -11,7 +11,7 @@ static char const * const builtin_commit_graph_usage[] = {\n \tN_(\"git commit-graph verify [--object-dir ] [--shallow] [--[no-]progress]\"),\n \tN_(\"git commit-graph write [--object-dir ] [--append] \"\n \t \"[--split[=]] [--reachable|--stdin-packs|--stdin-commits] \"\n-\t \"[--[no-]progress] \"),\n+\t \"[--[no-]progress] [--[no-]check-oids] \"),\n \tNULL\n };\n\n@@ -23,7 +23,7 @@ static const char * const builtin_commit_graph_verify_usage[] = {\n static const char * const builtin_commit_graph_write_usage[] = {\n \tN_(\"git commit-graph write [--object-dir ] [--append] \"\n \t \"[--split[=]] [--reachable|--stdin-packs|--stdin-commits] \"\n-\t \"[--[no-]progress] \"),\n+\t \"[--[no-]progress] [--[no-]check-oids] \"),\n \tNULL\n };\n\n@@ -36,6 +36,7 @@ static struct opts_commit_graph {\n \tint split;\n \tint shallow;\n \tint progress;\n+\tint check_oids;\n } opts;\n\n static struct object_directory *find_odb(struct repository *r,\n@@ -163,6 +164,8 @@ static int graph_write(int argc, const char **argv)\n \t\t\tN_(\"allow writing an incremental commit-graph file\"),\n \t\t\tPARSE_OPT_OPTARG | PARSE_OPT_NONEG,\n \t\t\twrite_option_parse_split),\n+\t\tOPT_BOOL(0, \"check-oids\", &opts.check_oids,\n+\t\t\tN_(\"require OIDs to be commits\")),\n \t\tOPT_INTEGER(0, \"max-commits\", &split_opts.max_commits,\n \t\t\tN_(\"maximum number of commits in a non-base split commit-graph\")),\n \t\tOPT_INTEGER(0, \"size-multiple\", &split_opts.size_multiple,\n@@ -173,6 +176,7 @@ static int graph_write(int argc, const char **argv)\n \t};\n\n \topts.progress = isatty(2);\n+\topts.check_oids = 1;\n \tsplit_opts.size_multiple = 2;\n \tsplit_opts.max_commits = 0;\n \tsplit_opts.expire_time = 0;\n@@ -227,7 +231,8 @@ static int graph_write(int argc, const char **argv)\n\n \t\t\t\toidset_insert(&commits, &oid);\n \t\t\t}\n-\t\t\tflags |= COMMIT_GRAPH_WRITE_CHECK_OIDS;\n+\t\t\tif (opts.check_oids)\n+\t\t\t\tflags |= COMMIT_GRAPH_WRITE_CHECK_OIDS;\n \t\t}\n\n \t\tUNLEAK(buf);\ndiff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh\nindex e874a12696..7960cefa1b 100755\n--- a/t/t5318-commit-graph.sh\n+++ b/t/t5318-commit-graph.sh\n@@ -49,6 +49,34 @@ test_expect_success 'exit with correct error on bad input to --stdin-commits' '\n \ttest_i18ngrep \"invalid commit object id\" stderr\n '\n\n+graph_expect_commits() {\n+\ttest-tool read-graph >got\n+\tif ! grep \"num_commits: $1\" got\n+\tthen\n+\t\techo \"graph_expect_commits: expected $1 commit(s), got:\"\n+\t\tcat got\n+\t\tfalse\n+\tfi\n+}\n+\n+test_expect_success 'ignores non-commit OIDs to --input=stdin-commits with --no-check-oids' '\n+\ttest_when_finished rm -rf \"$objdir/info/commit-graph\" &&\n+\tcd \"$TRASH_DIRECTORY/full\" &&\n+\t# write a graph to ensure layers are/are not added appropriately\n+\tgit rev-parse HEAD~1 >base &&\n+\tgit commit-graph write --stdin-commits bad &&\n+\ttest_expect_code 1 git commit-graph write --stdin-commits err &&\n+\ttest_i18ngrep \"unexpected non-hex object ID: HEAD\" err &&\n+\tgraph_expect_commits 2 &&\n+\t# update with valid commit OID, ignore tree OID\n+\tgit rev-parse HEAD HEAD^{tree} >in &&\n+\tgit commit-graph write --stdin-commits --no-check-oids output\n \tgit -c core.commitGraph=false $1 >expect\n--\n2.26.0.113.ge9739cdccc\n\n"},{"id":"396120","messageId":"20200424105957.GB5925@szeder.dev","threadId":"53223","inReplyTo":"20200422233930.GB19100@syl.local","subject":"Re: [PATCH 7/7] commit-graph.c: introduce '--[no-]check-oids'","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2020-04-24T10:59:57Z","receivedAt":"2020-04-24T11:00:04Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Wed, Apr 22, 2020 at 05:39:30PM -0600, Taylor Blau wrote:\n> > > diff --git a/Documentation/git-commit-graph.txt b/Documentation/git-commit-graph.txt\n> > > index 46f7f7c573..91e8027b86 100644\n> > > --- a/Documentation/git-commit-graph.txt\n> > > +++ b/Documentation/git-commit-graph.txt\n> > > @@ -82,6 +82,11 @@ tip with the previous tip.\n> > > Finally, if `--expire-time=` is not specified, let `datetime`\n> > > be the current time. After writing the split commit-graph, delete all\n> > > unused commit-graph whose modified times are older than `datetime`.\n> > > ++\n> > > +The `--[no-]check-oids` option decides whether or not OIDs are required\n> > > +to be commits. By default, `--check-oids` is implied, generating an\n> > > +error on non-commit objects. If `--no-check-oids` is given, non-commits\n> > > +are silently discarded.\n> >\n> > What happens with OIDs of tags, in particular with OIDs of tags that\n> > can be peeled down to commit objects? According to (my (too\n> > pedantic?) interpretation of) the above description they will trigger\n> > an error with '--check-oids' or will be ignored with\n> > '--no-check-oids'. The implementation, however, accepts those oids\n> > and peels them down to commit objects; I think this is the right\n> > behaviour.\n> \n> That's right, and certainly merits a mention in the documentation. I've\n> added that...\n> \n> > What happens with OIDs that name non-existing objects?\n> \n> ...these are silently discarded. I think that you could make a\n> compelling argument in either direction on this one, but I'm slightly\n> swayed towards \"discard these, too\", since '--no-check-oids' is\n> literally saying \"don't check these\".\n\nI don't want to argue either way, but I'd argue for making a conscious\ndecision that is justified in the commit message and documented in the\ndocs.\n\nSo, the option is '--stdin-commits' or '--input=stdin-commits', but\nit's not only about commits. Now, allowing OIDs of tags pointing to\ncommits and peeling them makes obvious sense, because we want commits\nreachable from those tags included in the commit-graph file. Allowing\nOIDs of tags pointing to non-commits and silently ignoring them makes\nsense (to better support your 'git f-e-r ... | git c-g write ...' use\ncase), though it's not that obvious (after all I managed to overlook\nit back then, that's why we are now here discussing these\n'--check-oids' patches).\n\nBut I'm not sure about silently ignoring OIDs pointing to non-existent\nobjects, because those might be a sign of some issues in whatever is\ngenerating the list of objects to be fed to 'git c-g write'. E.g. there\ncould be a race between 'git for-each-ref' listing OIDs and some other\nprocesses pruning them. Does this worth worrying about? Dunno...\nbut at least let's think it through, and record in the commit message\nwhy we made that decision, whatever that decision might be.\n\n> I guess that pushes us into the territory of whether or not \"check\" is\n> the right verb. \"verify\"? \n\nOh, I didn't think about this, but now that you mention it we have\n'--verify' in 'git rev-parse', 'git tag' and elsewhere, and we have\n'verify-commit', 'verify-path' and 'verify-tag' commands. So\n'--verify-oids' might be more consistent. I kind of like the 'oids'\nsuffix in the option name, though I don't know what else we might want\nto verify in this command in the future...\n\n> \"scrutinize\" :)?\n\nHuhh, erm, no ;)\n\n> If you're otherwise satisfied with this series, here's the updated\n> patch.\n\nI haven't yet looked closely at the rest of the series... The\ndocumentation update in the updated patch below looks good to me,\nthanks.\n\n> -- >8 --\n> \n> Subject: [PATCH] commit-graph.c: introduce '--[no-]check-oids'\n> \n> When operating on a stream of commit OIDs on stdin, 'git commit-graph\n> write' checks that each OID refers to an object that is indeed a commit.\n> This is convenient to make sure that the given input is well-formed, but\n> can sometimes be undesirable.\n> \n> For example, server operators may wish to feed the full commit object\n> IDs pointed to by refs that were updated during a push to 'git\n> commit-graph write --input=stdin-commits', and silently discard any\n> input that doesn't point at a commit. This can be done by combing the\n> output of 'git for-each-ref' with '--format %(*objecttype)', but this\n> requires opening up a potentially large number of objects. Instead, it\n> is more convenient to feed the updated object IDs to the commit-graph\n> machinery, and let it throw out whatever remains. to commits.\n\nEither the bulk of a sentence is missing, or there is a stray(?) \"to\ncommits.\" at the end of this line.\n\n> Introduce '--[no-]check-oids' to make such a behavior possible. With\n> '--check-oids' (the default behavior to retain backwards compatibility),\n> 'git commit-graph write' will barf on a non-commit line in its input.\n> With '--no-check-oids', such lines will be silently ignored, making the\n> above possible by specifying this option.\n> \n> No matter which is supplied, 'git commit-graph write' retains the\n> behavior from the previous commit of rejecting non-OID inputs like\n> \"HEAD\" and \"refs/heads/foo\" as before.\n> \n> Signed-off-by: Taylor Blau \n> ---\n> Documentation/git-commit-graph.txt | 6 ++++++\n> builtin/commit-graph.c | 11 ++++++++---\n> t/t5318-commit-graph.sh | 28 ++++++++++++++++++++++++++++\n> 3 files changed, 42 insertions(+), 3 deletions(-)\n> \n> diff --git a/Documentation/git-commit-graph.txt b/Documentation/git-commit-graph.txt\n> index 46f7f7c573..6bdbe42766 100644\n> --- a/Documentation/git-commit-graph.txt\n> +++ b/Documentation/git-commit-graph.txt\n> @@ -82,6 +82,12 @@ tip with the previous tip.\n> Finally, if `--expire-time=` is not specified, let `datetime`\n> be the current time. After writing the split commit-graph, delete all\n> unused commit-graph whose modified times are older than `datetime`.\n> ++\n> +The `--[no-]check-oids` option decides whether or not OIDs are required\n> +to be commits. By default, `--check-oids` is implied, generating an\n> +error on non-commit objects. If `--no-check-oids` is given, non-commits\n> +and non-existent objects are silently discarded. In either case, tags\n> +are peeled down to the object they reference.\n"},{"id":"396798","messageId":"20200501223848.GH41612@syl.local","threadId":"53223","inReplyTo":"20200424105957.GB5925@szeder.dev","subject":"Re: [PATCH 7/7] commit-graph.c: introduce '--[no-]check-oids'","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-05-01T22:38:48Z","receivedAt":"2020-05-01T22:38:52Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Fri, Apr 24, 2020 at 12:59:57PM +0200, SZEDER Gábor wrote:\n> On Wed, Apr 22, 2020 at 05:39:30PM -0600, Taylor Blau wrote:\n> > > > diff --git a/Documentation/git-commit-graph.txt b/Documentation/git-commit-graph.txt\n> > > > index 46f7f7c573..91e8027b86 100644\n> > > > --- a/Documentation/git-commit-graph.txt\n> > > > +++ b/Documentation/git-commit-graph.txt\n> > > > @@ -82,6 +82,11 @@ tip with the previous tip.\n> > > > Finally, if `--expire-time=` is not specified, let `datetime`\n> > > > be the current time. After writing the split commit-graph, delete all\n> > > > unused commit-graph whose modified times are older than `datetime`.\n> > > > ++\n> > > > +The `--[no-]check-oids` option decides whether or not OIDs are required\n> > > > +to be commits. By default, `--check-oids` is implied, generating an\n> > > > +error on non-commit objects. If `--no-check-oids` is given, non-commits\n> > > > +are silently discarded.\n> > >\n> > > What happens with OIDs of tags, in particular with OIDs of tags that\n> > > can be peeled down to commit objects? According to (my (too\n> > > pedantic?) interpretation of) the above description they will trigger\n> > > an error with '--check-oids' or will be ignored with\n> > > '--no-check-oids'. The implementation, however, accepts those oids\n> > > and peels them down to commit objects; I think this is the right\n> > > behaviour.\n> >\n> > That's right, and certainly merits a mention in the documentation. I've\n> > added that...\n> >\n> > > What happens with OIDs that name non-existing objects?\n> >\n> > ...these are silently discarded. I think that you could make a\n> > compelling argument in either direction on this one, but I'm slightly\n> > swayed towards \"discard these, too\", since '--no-check-oids' is\n> > literally saying \"don't check these\".\n>\n> I don't want to argue either way, but I'd argue for making a conscious\n> decision that is justified in the commit message and documented in the\n> docs.\n\nMe either, I very much welcome your consistently thoughtful replies\n(even if my extreme delay in responding to this one would suggest\notherwise... ;)).\n\n> So, the option is '--stdin-commits' or '--input=stdin-commits', but\n> it's not only about commits. Now, allowing OIDs of tags pointing to\n> commits and peeling them makes obvious sense, because we want commits\n> reachable from those tags included in the commit-graph file. Allowing\n> OIDs of tags pointing to non-commits and silently ignoring them makes\n> sense (to better support your 'git f-e-r ... | git c-g write ...' use\n> case), though it's not that obvious (after all I managed to overlook\n> it back then, that's why we are now here discussing these\n> '--check-oids' patches).\n>\n> But I'm not sure about silently ignoring OIDs pointing to non-existent\n> objects, because those might be a sign of some issues in whatever is\n> generating the list of objects to be fed to 'git c-g write'. E.g. there\n> could be a race between 'git for-each-ref' listing OIDs and some other\n> processes pruning them. Does this worth worrying about? Dunno...\n> but at least let's think it through, and record in the commit message\n> why we made that decision, whatever that decision might be.\n\nYeah, I think that the most reasonable behavior is definitely that we\nshould complain about non-existent objects over 'git commit-graph write\n--stdin-commits' no matter if '--[no-]check-oids' is given or not.\n\nBut, let's step back for a minute. What are we actually hoping to\naccomplish with '--check-oids'? I wrote this patch because I wanted a\nway to support 'git for-each-ref' piping into 'git commit-graph write'\nwithout having to juggle which tags peel down to commits and which\ndon't.\n\nNow, I figured that it would be unreasonable to change the default\nbehavior of 'git commit-graph write --stdin-commits' (which is to\ncomplain and error out in this circumstance), so I added\n'--no-check-oids' as a way to avoid that behavior for callers that want\nthat.\n\nBut, are there ever any callers that *wouldn't* want this behavior? As\nfar as I can tell, probably not. We're only going to be permitting\n*more* inputs to 'git commit-graph write', and I seriously doubt that\nanybody is depending on the above behavior. (Of course, if that's not\nthe case, I'd love for somebody to speak up here and we can continue\nthe course on this patch).\n\nSo, I propose the following:\n\n * We drop the idea of '--[no-]{check,verify}-oids', and always\n silently ignore non-commit inputs, retaining the existing behavior\n of always complaining about things that aren't valid hex OIDs, such\n as \"HEAD\".\n\n * We always error out on missing or corrupt commit OIDs, including\n valid OIDs that don't resolve to any object, or resolve to a tag\n that can't be fully peeled.\n\nDoes that seem reasonable?\n\n> > I guess that pushes us into the territory of whether or not \"check\" is\n> > the right verb. \"verify\"?\n>\n> Oh, I didn't think about this, but now that you mention it we have\n> '--verify' in 'git rev-parse', 'git tag' and elsewhere, and we have\n> 'verify-commit', 'verify-path' and 'verify-tag' commands. So\n> '--verify-oids' might be more consistent. I kind of like the 'oids'\n> suffix in the option name, though I don't know what else we might want\n> to verify in this command in the future...\n>\n> > \"scrutinize\" :)?\n>\n> Huhh, erm, no ;)\n>\n> > If you're otherwise satisfied with this series, here's the updated\n> > patch.\n>\n> I haven't yet looked closely at the rest of the series... The\n> documentation update in the updated patch below looks good to me,\n> thanks.\n>\n> > -- >8 --\n> >\n> > Subject: [PATCH] commit-graph.c: introduce '--[no-]check-oids'\n> >\n> > When operating on a stream of commit OIDs on stdin, 'git commit-graph\n> > write' checks that each OID refers to an object that is indeed a commit.\n> > This is convenient to make sure that the given input is well-formed, but\n> > can sometimes be undesirable.\n> >\n> > For example, server operators may wish to feed the full commit object\n> > IDs pointed to by refs that were updated during a push to 'git\n> > commit-graph write --input=stdin-commits', and silently discard any\n> > input that doesn't point at a commit. This can be done by combing the\n> > output of 'git for-each-ref' with '--format %(*objecttype)', but this\n> > requires opening up a potentially large number of objects. Instead, it\n> > is more convenient to feed the updated object IDs to the commit-graph\n> > machinery, and let it throw out whatever remains. to commits.\n>\n> Either the bulk of a sentence is missing, or there is a stray(?) \"to\n> commits.\" at the end of this line.\n>\n> > Introduce '--[no-]check-oids' to make such a behavior possible. With\n> > '--check-oids' (the default behavior to retain backwards compatibility),\n> > 'git commit-graph write' will barf on a non-commit line in its input.\n> > With '--no-check-oids', such lines will be silently ignored, making the\n> > above possible by specifying this option.\n> >\n> > No matter which is supplied, 'git commit-graph write' retains the\n> > behavior from the previous commit of rejecting non-OID inputs like\n> > \"HEAD\" and \"refs/heads/foo\" as before.\n> >\n> > Signed-off-by: Taylor Blau \n> > ---\n> > Documentation/git-commit-graph.txt | 6 ++++++\n> > builtin/commit-graph.c | 11 ++++++++---\n> > t/t5318-commit-graph.sh | 28 ++++++++++++++++++++++++++++\n> > 3 files changed, 42 insertions(+), 3 deletions(-)\n> >\n> > diff --git a/Documentation/git-commit-graph.txt b/Documentation/git-commit-graph.txt\n> > index 46f7f7c573..6bdbe42766 100644\n> > --- a/Documentation/git-commit-graph.txt\n> > +++ b/Documentation/git-commit-graph.txt\n> > @@ -82,6 +82,12 @@ tip with the previous tip.\n> > Finally, if `--expire-time=` is not specified, let `datetime`\n> > be the current time. After writing the split commit-graph, delete all\n> > unused commit-graph whose modified times are older than `datetime`.\n> > ++\n> > +The `--[no-]check-oids` option decides whether or not OIDs are required\n> > +to be commits. By default, `--check-oids` is implied, generating an\n> > +error on non-commit objects. If `--no-check-oids` is given, non-commits\n> > +and non-existent objects are silently discarded. In either case, tags\n> > +are peeled down to the object they reference.\n\nThanks,\nTaylor\n"},{"id":"396864","messageId":"20200503094005.GD170902@coredump.intra.peff.net","threadId":"53223","inReplyTo":"20200501223848.GH41612@syl.local","subject":"Re: [PATCH 7/7] commit-graph.c: introduce '--[no-]check-oids'","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-05-03T09:40:05Z","receivedAt":"2020-05-03T09:40:08Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, May 01, 2020 at 04:38:48PM -0600, Taylor Blau wrote:\n\n> But, are there ever any callers that *wouldn't* want this behavior? As\n> far as I can tell, probably not. We're only going to be permitting\n> *more* inputs to 'git commit-graph write', and I seriously doubt that\n> anybody is depending on the above behavior. (Of course, if that's not\n> the case, I'd love for somebody to speak up here and we can continue\n> the course on this patch).\n> \n> So, I propose the following:\n> \n> * We drop the idea of '--[no-]{check,verify}-oids', and always\n> silently ignore non-commit inputs, retaining the existing behavior\n> of always complaining about things that aren't valid hex OIDs, such\n> as \"HEAD\".\n> \n> * We always error out on missing or corrupt commit OIDs, including\n> valid OIDs that don't resolve to any object, or resolve to a tag\n> that can't be fully peeled.\n> \n> Does that seem reasonable?\n\nFWIW, I think that is the best direction. If anybody is depending on the\n\"commit-graph write will complain about non-commits\" behavior, they\ncould only be doing so for a few versions; prior to v2.24.0 we did not.\n\n-Peff\n"},{"id":"396875","messageId":"xmqq5zddj6us.fsf@gitster.c.googlers.com","threadId":"53223","inReplyTo":"20200503094005.GD170902@coredump.intra.peff.net","subject":"Re: [PATCH 7/7] commit-graph.c: introduce '--[no-]check-oids'","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-05-03T16:55:39Z","receivedAt":"2020-05-03T16:55:49Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Fri, May 01, 2020 at 04:38:48PM -0600, Taylor Blau wrote:\n> ...\n>> So, I propose the following:\n>> \n>> * We drop the idea of '--[no-]{check,verify}-oids', and always\n>> silently ignore non-commit inputs, retaining the existing behavior\n>> of always complaining about things that aren't valid hex OIDs, such\n>> as \"HEAD\".\n>> \n>> * We always error out on missing or corrupt commit OIDs, including\n>> valid OIDs that don't resolve to any object, or resolve to a tag\n>> that can't be fully peeled.\n>> \n>> Does that seem reasonable?\n>\n> FWIW, I think that is the best direction. If anybody is depending on the\n> \"commit-graph write will complain about non-commits\" behavior, they\n> could only be doing so for a few versions; prior to v2.24.0 we did not.\n\nIf we had it for the past 180 days or so, that's not like \" people\nhave seen it for only a brief time\", but working it around shouldn't\nbe too difficult---they need to validate the input they feed to the\ncommand themselves (or do they need to do more?).\n\nThanks.\n\n"},{"id":"396905","messageId":"20200504145937.GA11373@coredump.intra.peff.net","threadId":"53223","inReplyTo":"xmqq5zddj6us.fsf@gitster.c.googlers.com","subject":"Re: [PATCH 7/7] commit-graph.c: introduce '--[no-]check-oids'","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-05-04T14:59:37Z","receivedAt":"2020-05-04T14:59:39Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, May 03, 2020 at 09:55:39AM -0700, Junio C Hamano wrote:\n\n> >> Does that seem reasonable?\n> >\n> > FWIW, I think that is the best direction. If anybody is depending on the\n> > \"commit-graph write will complain about non-commits\" behavior, they\n> > could only be doing so for a few versions; prior to v2.24.0 we did not.\n> \n> If we had it for the past 180 days or so, that's not like \" people\n> have seen it for only a brief time\", but working it around shouldn't\n> be too difficult---they need to validate the input they feed to the\n> command themselves (or do they need to do more?).\n\nYeah, my point wasn't so much that it was brief as that we've had it\nboth ways, and nobody was complaining about it before v2.24.0 (the\ntype-restriction change came as a side effect of another tightening).\n\nBut yeah, if somebody really wants that validation, they can do it\nthemselves with \"cat-file --batch-check\". Or even for-each-ref directly:\n\n git for-each-ref --format='%(objectname) %(objecttype) %(*objecttype)' |\n awk '/commit/ { print $1 }' |\n git commit-graph write --stdin-commits\n\nIf you're using --stdin-commits, you're presumably processing the input\nanyway (since otherwise you'd just be using --reachable).\n\nI suppose you could argue the other way, too (that the user could be\nfiltering out non-commits). But so far we have one data point in either\ndirection, and it wants the more forgiving behavior. :)\n\n-Peff\n"},{"id":"396925","messageId":"xmqqv9lbhdee.fsf@gitster.c.googlers.com","threadId":"53223","inReplyTo":"20200504145937.GA11373@coredump.intra.peff.net","subject":"Re: [PATCH 7/7] commit-graph.c: introduce '--[no-]check-oids'","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-05-04T16:29:29Z","receivedAt":"2020-05-04T16:29:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Sun, May 03, 2020 at 09:55:39AM -0700, Junio C Hamano wrote:\n>\n>> >> Does that seem reasonable?\n>> >\n>> > FWIW, I think that is the best direction. If anybody is depending on the\n>> > \"commit-graph write will complain about non-commits\" behavior, they\n>> > could only be doing so for a few versions; prior to v2.24.0 we did not.\n>> \n>> If we had it for the past 180 days or so, that's not like \" people\n>> have seen it for only a brief time\", but working it around shouldn't\n>> be too difficult---they need to validate the input they feed to the\n>> command themselves (or do they need to do more?).\n>\n> Yeah, my point wasn't so much that it was brief as that we've had it\n> both ways, and nobody was complaining about it before v2.24.0 (the\n> type-restriction change came as a side effect of another tightening).\n>\n> But yeah, if somebody really wants that validation, they can do it\n> themselves with \"cat-file --batch-check\". Or even for-each-ref directly:\n>\n> git for-each-ref --format='%(objectname) %(objecttype) %(*objecttype)' |\n> awk '/commit/ { print $1 }' |\n> git commit-graph write --stdin-commits\n>\n> If you're using --stdin-commits, you're presumably processing the input\n> anyway (since otherwise you'd just be using --reachable).\n>\n> I suppose you could argue the other way, too (that the user could be\n> filtering out non-commits). But so far we have one data point in either\n> direction, and it wants the more forgiving behavior. :)\n\nYup. I agree that Taylor outlined the best direction going forward.\n\nThanks.\n"},{"id":"397003","messageId":"20200504221647.GH45250@syl.local","threadId":"53223","inReplyTo":"xmqqv9lbhdee.fsf@gitster.c.googlers.com","subject":"Re: [PATCH 7/7] commit-graph.c: introduce '--[no-]check-oids'","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-05-04T22:16:47Z","receivedAt":"2020-05-04T22:16:51Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Mon, May 04, 2020 at 09:29:29AM -0700, Junio C Hamano wrote:\n> Jeff King writes:\n>\n> > On Sun, May 03, 2020 at 09:55:39AM -0700, Junio C Hamano wrote:\n> >\n> >> >> Does that seem reasonable?\n> >> >\n> >> > FWIW, I think that is the best direction. If anybody is depending on the\n> >> > \"commit-graph write will complain about non-commits\" behavior, they\n> >> > could only be doing so for a few versions; prior to v2.24.0 we did not.\n> >>\n> >> If we had it for the past 180 days or so, that's not like \" people\n> >> have seen it for only a brief time\", but working it around shouldn't\n> >> be too difficult---they need to validate the input they feed to the\n> >> command themselves (or do they need to do more?).\n> >\n> > Yeah, my point wasn't so much that it was brief as that we've had it\n> > both ways, and nobody was complaining about it before v2.24.0 (the\n> > type-restriction change came as a side effect of another tightening).\n> >\n> > But yeah, if somebody really wants that validation, they can do it\n> > themselves with \"cat-file --batch-check\". Or even for-each-ref directly:\n> >\n> > git for-each-ref --format='%(objectname) %(objecttype) %(*objecttype)' |\n> > awk '/commit/ { print $1 }' |\n> > git commit-graph write --stdin-commits\n> >\n> > If you're using --stdin-commits, you're presumably processing the input\n> > anyway (since otherwise you'd just be using --reachable).\n> >\n> > I suppose you could argue the other way, too (that the user could be\n> > filtering out non-commits). But so far we have one data point in either\n> > direction, and it wants the more forgiving behavior. :)\n>\n> Yup. I agree that Taylor outlined the best direction going forward.\n\nThanks, both. I'll send a series to address this shortly. (I've been\nsitting on it in ttaylorr/git for a couple of days over the weekend,\nbut a few other things today have gotten in the way of me sending it to\nthe list.)\n\n> Thanks.\n\nThanks,\nTaylor\n"}]}