* [PATCH v3 3/3] builtin/commit-graph.c: support '--input=none' [not found] <4e85c6f7e40e7d6a8d93574645d65971b7cfa4f8.1581486293.git.me@ttaylorr.com/> @ 2020-03-03 23:27 ` Taylor Blau 2020-03-23 20:12 ` [PATCH v4] builtin/commit-graph.c: support '--input=graphed' Taylor Blau 0 siblings, 1 reply; 10+ messages in thread From: Taylor Blau @ 2020-03-03 23:27 UTC (permalink / raw) To: git; +Cc: peff, dstolee, gitster, martin.agren In the previous commit, we introduced '--split=<no-merge|merge-all>', and alluded to the fact that '--split=merge-all' would be useful for callers who wish to always trigger a merge of an incremental chain. There is a problem with the above approach, which is that there is no way to specify to the commit-graph builtin that a caller only wants to include commits already in the graph. One can specify '--input=append' to include all commits in the existing graphs (that haven't since been deleted from the object store), but the absence of '--input=stdin-{commits,packs}' causes the builtin to call 'fill_oids_from_all_packs()'. Passing '--input=reachable' (as in 'git commit-graph write --split=merge-all --input=reachable --input=append') works around this issue by making '--input=reachable' effectively a no-op, but this can be prohibitively expensive in large repositories, making it an undesirable choice for some users. Teach '--input=none' as an option to behave as if '--input=append' were given, but to consider no other sources in addition. This, in conjunction with the option introduced in the previous patch offers the convenient way to force the commit-graph machinery to condense a chain of incrementals without requiring any new commits: $ git commit-graph write --split=merge-all --input=none Signed-off-by: Taylor Blau <me@ttaylorr.com> --- Documentation/git-commit-graph.txt | 8 +++++++- builtin/commit-graph.c | 11 ++++++++--- commit-graph.c | 6 ++++-- commit-graph.h | 3 ++- t/t5324-split-commit-graph.sh | 26 ++++++++++++++++++++++++++ 5 files changed, 47 insertions(+), 7 deletions(-) diff --git a/Documentation/git-commit-graph.txt b/Documentation/git-commit-graph.txt index 0a320cccdd..b210cef52f 100644 --- a/Documentation/git-commit-graph.txt +++ b/Documentation/git-commit-graph.txt @@ -39,7 +39,7 @@ COMMANDS -------- 'write':: -Write a commit-graph file based on the commits found in packfiles. +Write a commit-graph file based on the specified sources of input: + With the `--input=stdin-packs` option, generate the new commit graph by walking objects only in the specified pack-indexes. (Cannot be combined @@ -57,6 +57,12 @@ walking commits starting at all refs. (Cannot be combined with With the `--input=append` option, include all commits that are present in the existing commit-graph file. + +With the `--input=none` option, behave as if `--input=append` were +given, but do not walk other packs to find additional commits. ++ +If none of the above options are given, then generate the new +commit-graph by walking over all pack-indexes. ++ With the `--split[=<strategy>]` option, write the commit-graph as a chain of multiple commit-graph files stored in `<dir>/info/commit-graphs`. Commit-graph layers are merged based on the diff --git a/builtin/commit-graph.c b/builtin/commit-graph.c index 0ff25896d0..a71af88815 100644 --- a/builtin/commit-graph.c +++ b/builtin/commit-graph.c @@ -11,7 +11,7 @@ static char const * const builtin_commit_graph_usage[] = { N_("git commit-graph verify [--object-dir <objdir>] [--shallow] [--[no-]progress]"), N_("git commit-graph write [--object-dir <objdir>] " "[--split[=<strategy>]] " - "[--input=<reachable|stdin-packs|stdin-commits|append>] " + "[--input=<reachable|stdin-packs|stdin-commits|append|none>] " "[--[no-]progress] <split options>"), NULL }; @@ -24,7 +24,7 @@ static const char * const builtin_commit_graph_verify_usage[] = { static const char * const builtin_commit_graph_write_usage[] = { N_("git commit-graph write [--object-dir <objdir>] " "[--split[=<strategy>]] " - "[--input=<reachable|stdin-packs|stdin-commits|append>] " + "[--input=<reachable|stdin-packs|stdin-commits|append|none>] " "[--[no-]progress] <split options>"), NULL }; @@ -33,7 +33,8 @@ enum commit_graph_input { COMMIT_GRAPH_INPUT_REACHABLE = (1 << 1), COMMIT_GRAPH_INPUT_STDIN_PACKS = (1 << 2), COMMIT_GRAPH_INPUT_STDIN_COMMITS = (1 << 3), - COMMIT_GRAPH_INPUT_APPEND = (1 << 4) + COMMIT_GRAPH_INPUT_APPEND = (1 << 4), + COMMIT_GRAPH_INPUT_NONE = (1 << 5) }; static struct opts_commit_graph { @@ -80,6 +81,8 @@ static int option_parse_input(const struct option *opt, const char *arg, *to |= COMMIT_GRAPH_INPUT_STDIN_COMMITS; else if (!strcmp(arg, "append")) *to |= COMMIT_GRAPH_INPUT_APPEND; + else if (!strcmp(arg, "none")) + *to |= (COMMIT_GRAPH_INPUT_APPEND | COMMIT_GRAPH_INPUT_NONE); else die(_("unrecognized --input source, %s"), arg); return 0; @@ -225,6 +228,8 @@ static int graph_write(int argc, const char **argv) opts.obj_dir = get_object_directory(); if (opts.input & COMMIT_GRAPH_INPUT_APPEND) flags |= COMMIT_GRAPH_WRITE_APPEND; + if (opts.input & COMMIT_GRAPH_INPUT_NONE) + flags |= COMMIT_GRAPH_WRITE_NO_INPUT; if (opts.split) flags |= COMMIT_GRAPH_WRITE_SPLIT; if (opts.progress) diff --git a/commit-graph.c b/commit-graph.c index 3a5cb23cd7..417b7eac9c 100644 --- a/commit-graph.c +++ b/commit-graph.c @@ -788,7 +788,8 @@ struct write_commit_graph_context { unsigned append:1, report_progress:1, split:1, - check_oids:1; + check_oids:1, + no_input:1; const struct split_commit_graph_opts *split_opts; }; @@ -1785,6 +1786,7 @@ int write_commit_graph(struct object_directory *odb, ctx->split = flags & COMMIT_GRAPH_WRITE_SPLIT ? 1 : 0; ctx->check_oids = flags & COMMIT_GRAPH_WRITE_CHECK_OIDS ? 1 : 0; ctx->split_opts = split_opts; + ctx->no_input = flags & COMMIT_GRAPH_WRITE_NO_INPUT ? 1 : 0; if (ctx->split) { struct commit_graph *g; @@ -1843,7 +1845,7 @@ int write_commit_graph(struct object_directory *odb, goto cleanup; } - if (!pack_indexes && !commit_hex) + if (!ctx->no_input && !pack_indexes && !commit_hex) fill_oids_from_all_packs(ctx); close_reachable(ctx); diff --git a/commit-graph.h b/commit-graph.h index 65a7d2edae..df7f3f5961 100644 --- a/commit-graph.h +++ b/commit-graph.h @@ -79,7 +79,8 @@ enum commit_graph_write_flags { COMMIT_GRAPH_WRITE_PROGRESS = (1 << 1), COMMIT_GRAPH_WRITE_SPLIT = (1 << 2), /* Make sure that each OID in the input is a valid commit OID. */ - COMMIT_GRAPH_WRITE_CHECK_OIDS = (1 << 3) + COMMIT_GRAPH_WRITE_CHECK_OIDS = (1 << 3), + COMMIT_GRAPH_WRITE_NO_INPUT = (1 << 4) }; enum commit_graph_split_flags { diff --git a/t/t5324-split-commit-graph.sh b/t/t5324-split-commit-graph.sh index 6894106727..7614f3915b 100755 --- a/t/t5324-split-commit-graph.sh +++ b/t/t5324-split-commit-graph.sh @@ -376,4 +376,30 @@ test_expect_success '--split=no-merge always writes an incremental' ' test_line_count = 2 $graphdir/commit-graph-chain ' +test_expect_success '--split=no-merge, --input=none writes nothing' ' + test_when_finished rm -rf a graphs.before graphs.after && + rm -rf $graphdir && + git reset --hard commits/2 && + git rev-list -1 HEAD~1 >a && + git commit-graph write --split=no-merge --input=stdin-commits <a && + ls $graphdir/graph-*.graph >graphs.before && + test_line_count = 1 $graphdir/commit-graph-chain && + git commit-graph write --split --input=none && + ls $graphdir/graph-*.graph >graphs.after && + test_cmp graphs.before graphs.after +' + +test_expect_success '--split=merge-all, --input=none merges the chain' ' + test_when_finished rm -rf a b && + rm -rf $graphdir && + git reset --hard commits/2 && + git rev-list -1 HEAD~1 >a && + git rev-list -1 HEAD >b && + git commit-graph write --split=no-merge --input=stdin-commits <a && + git commit-graph write --split=no-merge --input=stdin-commits <b && + test_line_count = 2 $graphdir/commit-graph-chain && + git commit-graph write --split=merge-all --input=none && + test_line_count = 1 $graphdir/commit-graph-chain +' + test_done -- 2.24.1.2723.g1e7b7fcc88 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH v4] builtin/commit-graph.c: support '--input=graphed' 2020-03-03 23:27 ` [PATCH v3 3/3] builtin/commit-graph.c: support '--input=none' Taylor Blau @ 2020-03-23 20:12 ` Taylor Blau 2020-03-27 9:13 ` Jeff King 2020-04-01 22:49 ` SZEDER Gábor 0 siblings, 2 replies; 10+ messages in thread From: Taylor Blau @ 2020-03-23 20:12 UTC (permalink / raw) To: git; +Cc: dstolee, gitster, martin.agren, peff, szeder.dev Hi, In response to some discussion in [1], here is another idea instead of '--input=none' that may make things a little clearer. Since it had been a long time, I reminded myself that '--input=none' means "--input=append, but don't look at packs as is usually the default". In [1], Gabor suggested that we could call this '--input=exists' or '--input=existing', but I think that 'graphed' may be clearer, since it is closer to "only _graphed_ commits". Another option would be to call this '--input=only-graphed', but I think that may be overly verbose for what we're going for here. Let me know what you think. [1]: https://lore.kernel.org/git/20200322110424.GC2224@szeder.dev/ --- 8< --- In the previous commit, we introduced '--split=<no-merge|merge-all>', and alluded to the fact that '--split=merge-all' would be useful for callers who wish to always trigger a merge of an incremental chain. There is a problem with the above approach, which is that there is no way to specify to the commit-graph builtin that a caller only wants to include commits already in the graph. One can specify '--input=append' to include all commits in the existing graphs, but the absence of '--input=stdin-{commits,packs}' causes the builtin to call 'fill_oids_from_all_packs()'. Passing '--input=reachable' (as in 'git commit-graph write --split=merge-all --input=reachable --input=append') works around this issue by making '--input=reachable' effectively a no-op, but this can be prohibitively expensive in large repositories, making it an undesirable choice for some users. Teach '--input=graphed' as an option to behave as if '--input=append' were given, but to consider no other sources in addition. This, in conjunction with the option introduced in the previous patch offers the convenient way to force the commit-graph machinery to condense a chain of incrementals without requiring any new commits: $ git commit-graph write --split=merge-all --input=graphed Signed-off-by: Taylor Blau <me@ttaylorr.com> --- Documentation/git-commit-graph.txt | 8 +++++++- builtin/commit-graph.c | 11 ++++++++--- commit-graph.c | 6 ++++-- commit-graph.h | 3 ++- t/t5324-split-commit-graph.sh | 26 ++++++++++++++++++++++++++ 5 files changed, 47 insertions(+), 7 deletions(-) diff --git a/Documentation/git-commit-graph.txt b/Documentation/git-commit-graph.txt index 0a320cccdd..4d8fbbe8ff 100644 --- a/Documentation/git-commit-graph.txt +++ b/Documentation/git-commit-graph.txt @@ -39,7 +39,7 @@ COMMANDS -------- 'write':: -Write a commit-graph file based on the commits found in packfiles. +Write a commit-graph file based on the specified sources of input: + With the `--input=stdin-packs` option, generate the new commit graph by walking objects only in the specified pack-indexes. (Cannot be combined @@ -57,6 +57,12 @@ walking commits starting at all refs. (Cannot be combined with With the `--input=append` option, include all commits that are present in the existing commit-graph file. + +With the `--input=graphed` option, behave as if `--input=append` were +given, but do not walk other packs to find additional commits. ++ +If none of the above options are given, then generate the new +commit-graph by walking over all pack-indexes. ++ With the `--split[=<strategy>]` option, write the commit-graph as a chain of multiple commit-graph files stored in `<dir>/info/commit-graphs`. Commit-graph layers are merged based on the diff --git a/builtin/commit-graph.c b/builtin/commit-graph.c index 0ff25896d0..dfb6c554ac 100644 --- a/builtin/commit-graph.c +++ b/builtin/commit-graph.c @@ -11,7 +11,7 @@ static char const * const builtin_commit_graph_usage[] = { N_("git commit-graph verify [--object-dir <objdir>] [--shallow] [--[no-]progress]"), N_("git commit-graph write [--object-dir <objdir>] " "[--split[=<strategy>]] " - "[--input=<reachable|stdin-packs|stdin-commits|append>] " + "[--input=<reachable|stdin-packs|stdin-commits|append|graphed>] " "[--[no-]progress] <split options>"), NULL }; @@ -24,7 +24,7 @@ static const char * const builtin_commit_graph_verify_usage[] = { static const char * const builtin_commit_graph_write_usage[] = { N_("git commit-graph write [--object-dir <objdir>] " "[--split[=<strategy>]] " - "[--input=<reachable|stdin-packs|stdin-commits|append>] " + "[--input=<reachable|stdin-packs|stdin-commits|append|graphed>] " "[--[no-]progress] <split options>"), NULL }; @@ -33,7 +33,8 @@ enum commit_graph_input { COMMIT_GRAPH_INPUT_REACHABLE = (1 << 1), COMMIT_GRAPH_INPUT_STDIN_PACKS = (1 << 2), COMMIT_GRAPH_INPUT_STDIN_COMMITS = (1 << 3), - COMMIT_GRAPH_INPUT_APPEND = (1 << 4) + COMMIT_GRAPH_INPUT_APPEND = (1 << 4), + COMMIT_GRAPH_INPUT_GRAPHED = (1 << 5) }; static struct opts_commit_graph { @@ -80,6 +81,8 @@ static int option_parse_input(const struct option *opt, const char *arg, *to |= COMMIT_GRAPH_INPUT_STDIN_COMMITS; else if (!strcmp(arg, "append")) *to |= COMMIT_GRAPH_INPUT_APPEND; + else if (!strcmp(arg, "graphed")) + *to |= (COMMIT_GRAPH_INPUT_APPEND | COMMIT_GRAPH_INPUT_GRAPHED); else die(_("unrecognized --input source, %s"), arg); return 0; @@ -225,6 +228,8 @@ static int graph_write(int argc, const char **argv) opts.obj_dir = get_object_directory(); if (opts.input & COMMIT_GRAPH_INPUT_APPEND) flags |= COMMIT_GRAPH_WRITE_APPEND; + if (opts.input & COMMIT_GRAPH_INPUT_GRAPHED) + flags |= COMMIT_GRAPH_WRITE_NO_INPUT; if (opts.split) flags |= COMMIT_GRAPH_WRITE_SPLIT; if (opts.progress) diff --git a/commit-graph.c b/commit-graph.c index c5a8ea244b..3da52847e4 100644 --- a/commit-graph.c +++ b/commit-graph.c @@ -788,7 +788,8 @@ struct write_commit_graph_context { unsigned append:1, report_progress:1, split:1, - check_oids:1; + check_oids:1, + no_input:1; const struct split_commit_graph_opts *split_opts; }; @@ -1781,6 +1782,7 @@ int write_commit_graph(struct object_directory *odb, ctx->split = flags & COMMIT_GRAPH_WRITE_SPLIT ? 1 : 0; ctx->check_oids = flags & COMMIT_GRAPH_WRITE_CHECK_OIDS ? 1 : 0; ctx->split_opts = split_opts; + ctx->no_input = flags & COMMIT_GRAPH_WRITE_NO_INPUT ? 1 : 0; if (ctx->split) { struct commit_graph *g; @@ -1839,7 +1841,7 @@ int write_commit_graph(struct object_directory *odb, goto cleanup; } - if (!pack_indexes && !commit_hex) + if (!ctx->no_input && !pack_indexes && !commit_hex) fill_oids_from_all_packs(ctx); close_reachable(ctx); diff --git a/commit-graph.h b/commit-graph.h index 65a7d2edae..df7f3f5961 100644 --- a/commit-graph.h +++ b/commit-graph.h @@ -79,7 +79,8 @@ enum commit_graph_write_flags { COMMIT_GRAPH_WRITE_PROGRESS = (1 << 1), COMMIT_GRAPH_WRITE_SPLIT = (1 << 2), /* Make sure that each OID in the input is a valid commit OID. */ - COMMIT_GRAPH_WRITE_CHECK_OIDS = (1 << 3) + COMMIT_GRAPH_WRITE_CHECK_OIDS = (1 << 3), + COMMIT_GRAPH_WRITE_NO_INPUT = (1 << 4) }; enum commit_graph_split_flags { diff --git a/t/t5324-split-commit-graph.sh b/t/t5324-split-commit-graph.sh index 6894106727..6dda4c1f1c 100755 --- a/t/t5324-split-commit-graph.sh +++ b/t/t5324-split-commit-graph.sh @@ -376,4 +376,30 @@ test_expect_success '--split=no-merge always writes an incremental' ' test_line_count = 2 $graphdir/commit-graph-chain ' +test_expect_success '--split=no-merge, --input=graphed writes nothing' ' + test_when_finished rm -rf a graphs.before graphs.after && + rm -rf $graphdir && + git reset --hard commits/2 && + git rev-list -1 HEAD~1 >a && + git commit-graph write --split=no-merge --input=stdin-commits <a && + ls $graphdir/graph-*.graph >graphs.before && + test_line_count = 1 $graphdir/commit-graph-chain && + git commit-graph write --split --input=graphed && + ls $graphdir/graph-*.graph >graphs.after && + test_cmp graphs.before graphs.after +' + +test_expect_success '--split=merge-all, --input=graphed merges the chain' ' + test_when_finished rm -rf a b && + rm -rf $graphdir && + git reset --hard commits/2 && + git rev-list -1 HEAD~1 >a && + git rev-list -1 HEAD >b && + git commit-graph write --split=no-merge --input=stdin-commits <a && + git commit-graph write --split=no-merge --input=stdin-commits <b && + test_line_count = 2 $graphdir/commit-graph-chain && + git commit-graph write --split=merge-all --input=graphed && + test_line_count = 1 $graphdir/commit-graph-chain +' + test_done -- 2.26.0.rc2.311.g2e49f7a131 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH v4] builtin/commit-graph.c: support '--input=graphed' 2020-03-23 20:12 ` [PATCH v4] builtin/commit-graph.c: support '--input=graphed' Taylor Blau @ 2020-03-27 9:13 ` Jeff King 2020-04-01 22:49 ` SZEDER Gábor 1 sibling, 0 replies; 10+ messages in thread From: Jeff King @ 2020-03-27 9:13 UTC (permalink / raw) To: Taylor Blau; +Cc: git, dstolee, gitster, martin.agren, szeder.dev On Mon, Mar 23, 2020 at 02:12:19PM -0600, Taylor Blau wrote: > Hi, > > In response to some discussion in [1], here is another idea instead of > '--input=none' that may make things a little clearer. Since it had been > a long time, I reminded myself that '--input=none' means > "--input=append, but don't look at packs as is usually the default". > > In [1], Gabor suggested that we could call this '--input=exists' or > '--input=existing', but I think that 'graphed' may be clearer, since > it is closer to "only _graphed_ commits". Yeah, this name is much more clear to me than "none" (and would have prevented some mild confusion I had when experimenting the other day). > Another option would be to call this '--input=only-graphed', but I think > that may be overly verbose for what we're going for here. Agreed. -Peff ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v4] builtin/commit-graph.c: support '--input=graphed' 2020-03-23 20:12 ` [PATCH v4] builtin/commit-graph.c: support '--input=graphed' Taylor Blau 2020-03-27 9:13 ` Jeff King @ 2020-04-01 22:49 ` SZEDER Gábor 1 sibling, 0 replies; 10+ messages in thread From: SZEDER Gábor @ 2020-04-01 22:49 UTC (permalink / raw) To: Taylor Blau; +Cc: git, dstolee, gitster, martin.agren, peff On Mon, Mar 23, 2020 at 02:12:19PM -0600, Taylor Blau wrote: > Hi, > > In response to some discussion in [1], here is another idea instead of > '--input=none' that may make things a little clearer. Since it had been > a long time, I reminded myself that '--input=none' means > "--input=append, but don't look at packs as is usually the default". > > In [1], Gabor suggested that we could call this '--input=exists' or > '--input=existing', but I think that 'graphed' may be clearer, since > it is closer to "only _graphed_ commits". > > Another option would be to call this '--input=only-graphed', but I think > that may be overly verbose for what we're going for here. > > Let me know what you think. > > [1]: https://lore.kernel.org/git/20200322110424.GC2224@szeder.dev/ > > --- 8< --- > > In the previous commit, we introduced '--split=<no-merge|merge-all>', > and alluded to the fact that '--split=merge-all' would be useful for > callers who wish to always trigger a merge of an incremental chain. > > There is a problem with the above approach, which is that there is no > way to specify to the commit-graph builtin that a caller only wants to > include commits already in the graph. One can specify '--input=append' > to include all commits in the existing graphs, but the absence of > '--input=stdin-{commits,packs}' causes the builtin to call > 'fill_oids_from_all_packs()'. > > Passing '--input=reachable' (as in 'git commit-graph write > --split=merge-all --input=reachable --input=append') works around this > issue by making '--input=reachable' effectively a no-op, but this can be > prohibitively expensive in large repositories, making it an undesirable > choice for some users. > > Teach '--input=graphed' as an option to behave as if '--input=append' were > given, but to consider no other sources in addition. > > This, in conjunction with the option introduced in the previous patch > offers the convenient way to force the commit-graph machinery to > condense a chain of incrementals without requiring any new commits: > > $ git commit-graph write --split=merge-all --input=graphed > > Signed-off-by: Taylor Blau <me@ttaylorr.com> > --- > Documentation/git-commit-graph.txt | 8 +++++++- > builtin/commit-graph.c | 11 ++++++++--- > commit-graph.c | 6 ++++-- > commit-graph.h | 3 ++- > t/t5324-split-commit-graph.sh | 26 ++++++++++++++++++++++++++ > 5 files changed, 47 insertions(+), 7 deletions(-) > > diff --git a/Documentation/git-commit-graph.txt b/Documentation/git-commit-graph.txt > index 0a320cccdd..4d8fbbe8ff 100644 > --- a/Documentation/git-commit-graph.txt > +++ b/Documentation/git-commit-graph.txt > @@ -39,7 +39,7 @@ COMMANDS > -------- > 'write':: > > -Write a commit-graph file based on the commits found in packfiles. > +Write a commit-graph file based on the specified sources of input: > + > With the `--input=stdin-packs` option, generate the new commit graph by > walking objects only in the specified pack-indexes. (Cannot be combined > @@ -57,6 +57,12 @@ walking commits starting at all refs. (Cannot be combined with > With the `--input=append` option, include all commits that are present > in the existing commit-graph file. > + > +With the `--input=graphed` option, behave as if `--input=append` were > +given, but do not walk other packs to find additional commits. s/walk/scan/ would be more fitting, I think. In an earlier version of these patches I asked for clarification about what happens with expired commits that are still included in the commit-graph... and I do remember that you replied to that, but, unfortunately, not what your reply was. And after reading this log message and the documentation update it's still not clear to me. > ++ > +If none of the above options are given, then generate the new > +commit-graph by walking over all pack-indexes. s/walking/scanning/ > ++ > With the `--split[=<strategy>]` option, write the commit-graph as a > chain of multiple commit-graph files stored in > `<dir>/info/commit-graphs`. Commit-graph layers are merged based on the ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 0/3] builtin/commit-graph.c: new split/merge options
@ 2020-01-31 0:28 Taylor Blau
2020-02-12 5:47 ` [PATCH v3 " Taylor Blau
0 siblings, 1 reply; 10+ messages in thread
From: Taylor Blau @ 2020-01-31 0:28 UTC (permalink / raw)
To: git; +Cc: peff, dstolee, gitster
Hi,
Here are another few patches that came out of working on GitHub's
deployment of incremental commit-graphs. These three patches introduce
two new options: '--split[=<merge-all|no-merge>]' and
'--input=<source>'.
The former controls whether or not commit-graph's split machinery should
either write an incremental commit graph, squash the chain of
incrementals, or defer to the other options.
(This comes from GitHub's desire to have more fine-grained control over
the commit-graph chain's behavior. We run short jobs after every push
that we would like to limit the running time of, and hence we do not
want to ever merge a long chain of incrementals unless we specifically
opt into that.)
The latter of the two new options does two things:
* It cleans up the many options that specify input sources (e.g.,
'--stdin-commits', '--stdin-packs', '--reachable' and so on) under
one unifying name.
* It allows us to introduce a new argument '--input=none', to prevent
walking each packfile when neither '--stdin-commits' nor
'--stdin-packs' was given.
Together, these have the combined effect of being able to write the
following two new invocations:
$ git commit-graph write --split=merge-all --input=none
$ git commit-graph write --split=no-merge --input=stdin-packs
to (1) merge the chain, and (2) write a single new incremental.
Thanks in advance for your review, as always.
Taylor Blau (3):
builtin/commit-graph.c: support '--split[=<strategy>]'
builtin/commit-graph.c: introduce '--input=<source>'
builtin/commit-graph.c: support '--input=none'
Documentation/git-commit-graph.txt | 55 ++++++++------
builtin/commit-graph.c | 113 +++++++++++++++++++++++------
commit-graph.c | 25 ++++---
commit-graph.h | 10 ++-
t/t5318-commit-graph.sh | 46 ++++++------
t/t5324-split-commit-graph.sh | 85 +++++++++++++++++-----
6 files changed, 239 insertions(+), 95 deletions(-)
--
2.25.0.dirty
^ permalink raw reply [flat|nested] 10+ messages in thread* [PATCH v3 0/3] builtin/commit-graph.c: new split/merge options 2020-01-31 0:28 [PATCH 0/3] builtin/commit-graph.c: new split/merge options Taylor Blau @ 2020-02-12 5:47 ` Taylor Blau 2020-02-12 5:47 ` [PATCH v3 3/3] builtin/commit-graph.c: support '--input=none' Taylor Blau 0 siblings, 1 reply; 10+ messages in thread From: Taylor Blau @ 2020-02-12 5:47 UTC (permalink / raw) To: git; +Cc: peff, dstolee, gitster, martin.agren Hi everybody, Attached is what I anticipate/hope to be the final reroll of my series to add new arguments to the 'git commit-graph write --split' flag to allow callers to force or prohibit the commit-graph machinery to merge multiple commit-graph layers. I was keeping an eye out for more discussion about whether or not these flags were acceptable by reviewers. Martin Ågren and Derrick Stolee have both chimed in that they seem OK. Since there hasn't been much more discussion in this thread, I replayed this series on top of 'tb/commit-graph-use-odb' (which was itself rebased on 'master'). I picked up a couple of ASCIIDoc changes along the way, and a range-diff is included below. Thanks again. Taylor Blau (3): builtin/commit-graph.c: support '--split[=<strategy>]' builtin/commit-graph.c: introduce '--input=<source>' builtin/commit-graph.c: support '--input=none' Documentation/git-commit-graph.txt | 51 ++++++++----- builtin/commit-graph.c | 115 +++++++++++++++++++++++------ commit-graph.c | 28 ++++--- commit-graph.h | 10 ++- t/t5318-commit-graph.sh | 4 +- t/t5324-split-commit-graph.sh | 53 ++++++++++++- 6 files changed, 205 insertions(+), 56 deletions(-) Range-diff against v2: 1: 6428dac6e5 ! 1: e1635a0e34 builtin/commit-graph.c: support '--split[=<strategy>]' @@ Documentation/git-commit-graph.txt: or `--stdin-packs`.) +strategy and other splitting options. The new commits not already in the +commit-graph are added in a new "tip" file. This file is merged with the +existing file if the following merge conditions are met: +++ +* If `--split=merge-always` is specified, then a merge is always +conducted, and the remaining options are ignored. Conversely, if +`--split=no-merge` is specified, a merge is never performed, and the 2: c7ba70e19d = 2: 655fe63076 builtin/commit-graph.c: introduce '--input=<source>' 3: 7d6a608acd ! 3: 4e85c6f7e4 builtin/commit-graph.c: support '--input=none' @@ Documentation/git-commit-graph.txt: walking commits starting at all refs. (Canno + +With the `--input=none` option, behave as if `--input=append` were +given, but do not walk other packs to find additional commits. -+ +++ +If none of the above options are given, then generate the new +commit-graph by walking over all pack-indexes. ++ -- 2.25.0.119.gaa12b7378b ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH v3 3/3] builtin/commit-graph.c: support '--input=none' 2020-02-12 5:47 ` [PATCH v3 " Taylor Blau @ 2020-02-12 5:47 ` Taylor Blau 2020-02-13 11:39 ` SZEDER Gábor 2020-02-13 12:31 ` SZEDER Gábor 0 siblings, 2 replies; 10+ messages in thread From: Taylor Blau @ 2020-02-12 5:47 UTC (permalink / raw) To: git; +Cc: peff, dstolee, gitster, martin.agren In the previous commit, we introduced '--split=<no-merge|merge-all>', and alluded to the fact that '--split=merge-all' would be useful for callers who wish to always trigger a merge of an incremental chain. There is a problem with the above approach, which is that there is no way to specify to the commit-graph builtin that a caller only wants to include commits already in the graph. One can specify '--input=append' to include all commits in the existing graphs, but the absence of '--input=stdin-{commits,packs}' causes the builtin to call 'fill_oids_from_all_packs()'. Passing '--input=reachable' (as in 'git commit-graph write --split=merge-all --input=reachable --input=append') works around this issue by making '--input=reachable' effectively a no-op, but this can be prohibitively expensive in large repositories, making it an undesirable choice for some users. Teach '--input=none' as an option to behave as if '--input=append' were given, but to consider no other sources in addition. This, in conjunction with the option introduced in the previous patch offers the convenient way to force the commit-graph machinery to condense a chain of incrementals without requiring any new commits: $ git commit-graph write --split=merge-all --input=none Signed-off-by: Taylor Blau <me@ttaylorr.com> --- Documentation/git-commit-graph.txt | 8 +++++++- builtin/commit-graph.c | 11 ++++++++--- commit-graph.c | 6 ++++-- commit-graph.h | 3 ++- t/t5324-split-commit-graph.sh | 26 ++++++++++++++++++++++++++ 5 files changed, 47 insertions(+), 7 deletions(-) diff --git a/Documentation/git-commit-graph.txt b/Documentation/git-commit-graph.txt index 0a320cccdd..b210cef52f 100644 --- a/Documentation/git-commit-graph.txt +++ b/Documentation/git-commit-graph.txt @@ -39,7 +39,7 @@ COMMANDS -------- 'write':: -Write a commit-graph file based on the commits found in packfiles. +Write a commit-graph file based on the specified sources of input: + With the `--input=stdin-packs` option, generate the new commit graph by walking objects only in the specified pack-indexes. (Cannot be combined @@ -57,6 +57,12 @@ walking commits starting at all refs. (Cannot be combined with With the `--input=append` option, include all commits that are present in the existing commit-graph file. + +With the `--input=none` option, behave as if `--input=append` were +given, but do not walk other packs to find additional commits. ++ +If none of the above options are given, then generate the new +commit-graph by walking over all pack-indexes. ++ With the `--split[=<strategy>]` option, write the commit-graph as a chain of multiple commit-graph files stored in `<dir>/info/commit-graphs`. Commit-graph layers are merged based on the diff --git a/builtin/commit-graph.c b/builtin/commit-graph.c index 0ff25896d0..a71af88815 100644 --- a/builtin/commit-graph.c +++ b/builtin/commit-graph.c @@ -11,7 +11,7 @@ static char const * const builtin_commit_graph_usage[] = { N_("git commit-graph verify [--object-dir <objdir>] [--shallow] [--[no-]progress]"), N_("git commit-graph write [--object-dir <objdir>] " "[--split[=<strategy>]] " - "[--input=<reachable|stdin-packs|stdin-commits|append>] " + "[--input=<reachable|stdin-packs|stdin-commits|append|none>] " "[--[no-]progress] <split options>"), NULL }; @@ -24,7 +24,7 @@ static const char * const builtin_commit_graph_verify_usage[] = { static const char * const builtin_commit_graph_write_usage[] = { N_("git commit-graph write [--object-dir <objdir>] " "[--split[=<strategy>]] " - "[--input=<reachable|stdin-packs|stdin-commits|append>] " + "[--input=<reachable|stdin-packs|stdin-commits|append|none>] " "[--[no-]progress] <split options>"), NULL }; @@ -33,7 +33,8 @@ enum commit_graph_input { COMMIT_GRAPH_INPUT_REACHABLE = (1 << 1), COMMIT_GRAPH_INPUT_STDIN_PACKS = (1 << 2), COMMIT_GRAPH_INPUT_STDIN_COMMITS = (1 << 3), - COMMIT_GRAPH_INPUT_APPEND = (1 << 4) + COMMIT_GRAPH_INPUT_APPEND = (1 << 4), + COMMIT_GRAPH_INPUT_NONE = (1 << 5) }; static struct opts_commit_graph { @@ -80,6 +81,8 @@ static int option_parse_input(const struct option *opt, const char *arg, *to |= COMMIT_GRAPH_INPUT_STDIN_COMMITS; else if (!strcmp(arg, "append")) *to |= COMMIT_GRAPH_INPUT_APPEND; + else if (!strcmp(arg, "none")) + *to |= (COMMIT_GRAPH_INPUT_APPEND | COMMIT_GRAPH_INPUT_NONE); else die(_("unrecognized --input source, %s"), arg); return 0; @@ -225,6 +228,8 @@ static int graph_write(int argc, const char **argv) opts.obj_dir = get_object_directory(); if (opts.input & COMMIT_GRAPH_INPUT_APPEND) flags |= COMMIT_GRAPH_WRITE_APPEND; + if (opts.input & COMMIT_GRAPH_INPUT_NONE) + flags |= COMMIT_GRAPH_WRITE_NO_INPUT; if (opts.split) flags |= COMMIT_GRAPH_WRITE_SPLIT; if (opts.progress) diff --git a/commit-graph.c b/commit-graph.c index 3a5cb23cd7..417b7eac9c 100644 --- a/commit-graph.c +++ b/commit-graph.c @@ -788,7 +788,8 @@ struct write_commit_graph_context { unsigned append:1, report_progress:1, split:1, - check_oids:1; + check_oids:1, + no_input:1; const struct split_commit_graph_opts *split_opts; }; @@ -1785,6 +1786,7 @@ int write_commit_graph(struct object_directory *odb, ctx->split = flags & COMMIT_GRAPH_WRITE_SPLIT ? 1 : 0; ctx->check_oids = flags & COMMIT_GRAPH_WRITE_CHECK_OIDS ? 1 : 0; ctx->split_opts = split_opts; + ctx->no_input = flags & COMMIT_GRAPH_WRITE_NO_INPUT ? 1 : 0; if (ctx->split) { struct commit_graph *g; @@ -1843,7 +1845,7 @@ int write_commit_graph(struct object_directory *odb, goto cleanup; } - if (!pack_indexes && !commit_hex) + if (!ctx->no_input && !pack_indexes && !commit_hex) fill_oids_from_all_packs(ctx); close_reachable(ctx); diff --git a/commit-graph.h b/commit-graph.h index 65a7d2edae..df7f3f5961 100644 --- a/commit-graph.h +++ b/commit-graph.h @@ -79,7 +79,8 @@ enum commit_graph_write_flags { COMMIT_GRAPH_WRITE_PROGRESS = (1 << 1), COMMIT_GRAPH_WRITE_SPLIT = (1 << 2), /* Make sure that each OID in the input is a valid commit OID. */ - COMMIT_GRAPH_WRITE_CHECK_OIDS = (1 << 3) + COMMIT_GRAPH_WRITE_CHECK_OIDS = (1 << 3), + COMMIT_GRAPH_WRITE_NO_INPUT = (1 << 4) }; enum commit_graph_split_flags { diff --git a/t/t5324-split-commit-graph.sh b/t/t5324-split-commit-graph.sh index 6894106727..7614f3915b 100755 --- a/t/t5324-split-commit-graph.sh +++ b/t/t5324-split-commit-graph.sh @@ -376,4 +376,30 @@ test_expect_success '--split=no-merge always writes an incremental' ' test_line_count = 2 $graphdir/commit-graph-chain ' +test_expect_success '--split=no-merge, --input=none writes nothing' ' + test_when_finished rm -rf a graphs.before graphs.after && + rm -rf $graphdir && + git reset --hard commits/2 && + git rev-list -1 HEAD~1 >a && + git commit-graph write --split=no-merge --input=stdin-commits <a && + ls $graphdir/graph-*.graph >graphs.before && + test_line_count = 1 $graphdir/commit-graph-chain && + git commit-graph write --split --input=none && + ls $graphdir/graph-*.graph >graphs.after && + test_cmp graphs.before graphs.after +' + +test_expect_success '--split=merge-all, --input=none merges the chain' ' + test_when_finished rm -rf a b && + rm -rf $graphdir && + git reset --hard commits/2 && + git rev-list -1 HEAD~1 >a && + git rev-list -1 HEAD >b && + git commit-graph write --split=no-merge --input=stdin-commits <a && + git commit-graph write --split=no-merge --input=stdin-commits <b && + test_line_count = 2 $graphdir/commit-graph-chain && + git commit-graph write --split=merge-all --input=none && + test_line_count = 1 $graphdir/commit-graph-chain +' + test_done -- 2.25.0.119.gaa12b7378b ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH v3 3/3] builtin/commit-graph.c: support '--input=none' 2020-02-12 5:47 ` [PATCH v3 3/3] builtin/commit-graph.c: support '--input=none' Taylor Blau @ 2020-02-13 11:39 ` SZEDER Gábor 2020-02-13 12:31 ` SZEDER Gábor 1 sibling, 0 replies; 10+ messages in thread From: SZEDER Gábor @ 2020-02-13 11:39 UTC (permalink / raw) To: Taylor Blau; +Cc: git, peff, dstolee, gitster, martin.agren On Tue, Feb 11, 2020 at 09:47:57PM -0800, Taylor Blau wrote: > In the previous commit, we introduced '--split=<no-merge|merge-all>', Nit: the previous commit introduced '--input=<source>'. > and alluded to the fact that '--split=merge-all' would be useful for > callers who wish to always trigger a merge of an incremental chain. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v3 3/3] builtin/commit-graph.c: support '--input=none' 2020-02-12 5:47 ` [PATCH v3 3/3] builtin/commit-graph.c: support '--input=none' Taylor Blau 2020-02-13 11:39 ` SZEDER Gábor @ 2020-02-13 12:31 ` SZEDER Gábor 2020-02-13 16:08 ` Junio C Hamano 2020-02-13 17:56 ` Taylor Blau 1 sibling, 2 replies; 10+ messages in thread From: SZEDER Gábor @ 2020-02-13 12:31 UTC (permalink / raw) To: Taylor Blau; +Cc: git, peff, dstolee, gitster, martin.agren On Tue, Feb 11, 2020 at 09:47:57PM -0800, Taylor Blau wrote: > In the previous commit, we introduced '--split=<no-merge|merge-all>', > and alluded to the fact that '--split=merge-all' would be useful for > callers who wish to always trigger a merge of an incremental chain. > > There is a problem with the above approach, which is that there is no > way to specify to the commit-graph builtin that a caller only wants to > include commits already in the graph. I'd like clarification on a detail here. Is it only about not adding any new commits, or about keeping all existing commits as well? IOW, do you want to: - include only commits already existing in the commit-graph, without adding any new commits, but remove any commits that do not exist in the object database anymore. or: - include _all_ commits already existing in the commit-graph, even those that don't exist anymore in the object database, without adding any new commits. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v3 3/3] builtin/commit-graph.c: support '--input=none' 2020-02-13 12:31 ` SZEDER Gábor @ 2020-02-13 16:08 ` Junio C Hamano 2020-02-13 17:58 ` Taylor Blau 2020-02-13 17:56 ` Taylor Blau 1 sibling, 1 reply; 10+ messages in thread From: Junio C Hamano @ 2020-02-13 16:08 UTC (permalink / raw) To: SZEDER Gábor; +Cc: Taylor Blau, git, peff, dstolee, martin.agren SZEDER Gábor <szeder.dev@gmail.com> writes: > On Tue, Feb 11, 2020 at 09:47:57PM -0800, Taylor Blau wrote: >> In the previous commit, we introduced '--split=<no-merge|merge-all>', >> and alluded to the fact that '--split=merge-all' would be useful for >> callers who wish to always trigger a merge of an incremental chain. >> >> There is a problem with the above approach, which is that there is no >> way to specify to the commit-graph builtin that a caller only wants to >> include commits already in the graph. > > I'd like clarification on a detail here. Is it only about not adding > any new commits, or about keeping all existing commits as well? IOW, > do you want to: > > - include only commits already existing in the commit-graph, without > adding any new commits, but remove any commits that do not exist > in the object database anymore. > > or: > > - include _all_ commits already existing in the commit-graph, even > those that don't exist anymore in the object database, without > adding any new commits. FWIW, I read it as the former, but now you brought it up, it can be read either way. Thanks for good review comments, as always. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v3 3/3] builtin/commit-graph.c: support '--input=none' 2020-02-13 16:08 ` Junio C Hamano @ 2020-02-13 17:58 ` Taylor Blau 0 siblings, 0 replies; 10+ messages in thread From: Taylor Blau @ 2020-02-13 17:58 UTC (permalink / raw) To: Junio C Hamano Cc: SZEDER Gábor, Taylor Blau, git, peff, dstolee, martin.agren On Thu, Feb 13, 2020 at 08:08:15AM -0800, Junio C Hamano wrote: > SZEDER Gábor <szeder.dev@gmail.com> writes: > > > On Tue, Feb 11, 2020 at 09:47:57PM -0800, Taylor Blau wrote: > >> In the previous commit, we introduced '--split=<no-merge|merge-all>', > >> and alluded to the fact that '--split=merge-all' would be useful for > >> callers who wish to always trigger a merge of an incremental chain. > >> > >> There is a problem with the above approach, which is that there is no > >> way to specify to the commit-graph builtin that a caller only wants to > >> include commits already in the graph. > > > > I'd like clarification on a detail here. Is it only about not adding > > any new commits, or about keeping all existing commits as well? IOW, > > do you want to: > > > > - include only commits already existing in the commit-graph, without > > adding any new commits, but remove any commits that do not exist > > in the object database anymore. > > > > or: > > > > - include _all_ commits already existing in the commit-graph, even > > those that don't exist anymore in the object database, without > > adding any new commits. > > FWIW, I read it as the former, but now you brought it up, it can be > read either way. It was intended as the former, but I share both of your feelings that it could be read either way. I amended the commit message to clarify by adding: (and haven't since been deleted from the object store) as a parenthetical after "already in the graph...". > Thanks for good review comments, as always. Yes, indeed: thank very much for your thoughtful feedback. Thanks, Taylor ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v3 3/3] builtin/commit-graph.c: support '--input=none' 2020-02-13 12:31 ` SZEDER Gábor 2020-02-13 16:08 ` Junio C Hamano @ 2020-02-13 17:56 ` Taylor Blau 1 sibling, 0 replies; 10+ messages in thread From: Taylor Blau @ 2020-02-13 17:56 UTC (permalink / raw) To: SZEDER Gábor; +Cc: Taylor Blau, git, peff, dstolee, gitster, martin.agren On Thu, Feb 13, 2020 at 01:31:29PM +0100, SZEDER Gábor wrote: > On Tue, Feb 11, 2020 at 09:47:57PM -0800, Taylor Blau wrote: > > In the previous commit, we introduced '--split=<no-merge|merge-all>', > > and alluded to the fact that '--split=merge-all' would be useful for > > callers who wish to always trigger a merge of an incremental chain. > > > > There is a problem with the above approach, which is that there is no > > way to specify to the commit-graph builtin that a caller only wants to > > include commits already in the graph. > > I'd like clarification on a detail here. Is it only about not adding > any new commits, or about keeping all existing commits as well? IOW, > do you want to: > > - include only commits already existing in the commit-graph, without > adding any new commits, but remove any commits that do not exist > in the object database anymore. This one, since the commit-graph machinery will drop any commits (no matter what input/source/mode you specify) that no longer exist in the object store. > or: > > - include _all_ commits already existing in the commit-graph, even > those that don't exist anymore in the object database, without > adding any new commits. Thanks, Taylor ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2020-04-01 22:49 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <4e85c6f7e40e7d6a8d93574645d65971b7cfa4f8.1581486293.git.me@ttaylorr.com/>
2020-03-03 23:27 ` [PATCH v3 3/3] builtin/commit-graph.c: support '--input=none' Taylor Blau
2020-03-23 20:12 ` [PATCH v4] builtin/commit-graph.c: support '--input=graphed' Taylor Blau
2020-03-27 9:13 ` Jeff King
2020-04-01 22:49 ` SZEDER Gábor
2020-01-31 0:28 [PATCH 0/3] builtin/commit-graph.c: new split/merge options Taylor Blau
2020-02-12 5:47 ` [PATCH v3 " Taylor Blau
2020-02-12 5:47 ` [PATCH v3 3/3] builtin/commit-graph.c: support '--input=none' Taylor Blau
2020-02-13 11:39 ` SZEDER Gábor
2020-02-13 12:31 ` SZEDER Gábor
2020-02-13 16:08 ` Junio C Hamano
2020-02-13 17:58 ` Taylor Blau
2020-02-13 17:56 ` Taylor Blau
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).