On 6/7/2019 5:57 PM, Junio C Hamano wrote:
> "Derrick Stolee via GitGitGadget" <gitgitgadget@xxxxxxxxx> writes:
>
>> - N_("git commit-graph write [--object-dir <objdir>] [--append] [--reachable|--stdin-packs|--stdin-commits]"),
>> + N_("git commit-graph write [--object-dir <objdir>] [--append|--split] [--reachable|--stdin-packs|--stdin-commits]"),
>
> Not a comment on the essential part of this feature, but are append
> and split meant to be mutually exclusive?
I think the more correct thing to say is that `--split` implies `--append`:
As we write a new tip graph, we do not discard any commits from the lower
layers, even if we merge them together.
However, the interaction between the two features is not currently
guarded well. If both `--split` and `--append` are specified, we
will allocate a larger initial array to store commit ids and then
scan the existing commit-graph for commits.
Likely the following diff would be the correct place to guard against
a combined set of options, since these are passed to the method from
other builtins. This block is inside an if (ctx->split) condition.
diff --git a/commit-graph.c b/commit-graph.c
index 8842f93910..9cf5296e4e 100644
--- a/commit-graph.c
+++ b/commit-graph.c
@@ -1756,6 +1756,13 @@ int write_commit_graph(const char *obj_dir,
struct commit_graph *g;
prepare_commit_graph(ctx->r);
+ /*
+ * Writing a split graph only adds commits, implying
+ * a similar behavior to 'append' without scanning the
+ * existing commit-graph files.
+ */
+ ctx->append = 0;
+
g = ctx->r->objects->commit_graph;
while (g) {
> One thing that is somewhat curious is that this commit itself does
> not do much that would affect the correctness of how GRAPH_SPLIT
> works, as the actual machinery was introduced in the previous step
> and this step merely makes it accessible from the outside. So I had
> to look at the previous step to see if the internal machinery had
> some safety valve to catch the combination and flag it as an error
> or something like that, but if I am not mistaken, there is nothing
> that prevents both from being specified.
There is nothing that stops them both from being specified, so this
usage describes a strictness that is not enforced. I can change this
to be "[--append] [--split]" if that is preferred.
Thanks,
-Stolee
