| Message ID | 4e85c6f7e40e7d6a8d93574645d65971b7cfa4f8.1581486293.git.me@ttaylorr.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | builtin/commit-graph.c: new split/merge options | expand |
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.
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.
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.
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
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
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
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(-)