Re: [PATCH 03/14] packed-graph: create git-graph builtin

From: Derrick Stolee <stolee@gmail.com>
Date: Jan 26, 2018, 13:13 UTC
Message-ID: <a29fc66e-8063-d134-b6d2-4be38de161f4@gmail.com>
In-Reply-To: <CAGZ79ka61UCXWAP4X8CTdCSiPUBnsO1_-wNSkGmV0VjZyAqcpw@mail.gmail.com>

On 1/25/2018 4:45 PM, Stefan Beller wrote:

Show 11 quoted lines

> On Thu, Jan 25, 2018 at 6:02 AM, Derrick Stolee <stolee@gmail.com> wrote:
>> Teach Git the 'graph' builtin that will be used for writing and
>> reading packed graph files. The current implementation is mostly
>> empty, except for a check that the core.graph setting is enabled
>> and a '--pack-dir' option.
> I wonder if this builtin should not respect the boolean core graph,
> as this new builtin commands' whole existence
> is to deal with these new files?
>
> As you assume this builtin as a plumbing command, I would
> expect it to pay less attention to config rather than more.

My thought was to alert the caller "This graph isn't going to be good for anything!" and fail quickly before doing work. You do have a good point, and I think we can remove that condition here. When we integrate with other commands ('repack', 'fetch', 'clone') we will want a different setting that signals automatically writing the graph and we don't want those to fail because they are not aware of a second config setting.

Show 13 quoted lines

>
>> @@ -408,6 +408,7 @@ static struct cmd_struct commands[] = {
>>          { "fsck-objects", cmd_fsck, RUN_SETUP },
>>          { "gc", cmd_gc, RUN_SETUP },
>>          { "get-tar-commit-id", cmd_get_tar_commit_id },
>> +       { "graph", cmd_graph, RUN_SETUP_GENTLY },
> Why gently, though?
>
>  From reading the docs (and assumptions on further patches)
> we'd want to abort if there is no .git dir to be found?
>
> Or is a future patch having manual logic? (e.g. if pack-dir is
> given, the command may be invoked from outside a git dir)

You are right. I inherited this from my MIDX patch which can operate on a list of IDX files without a .git folder. The commit graph operations need an ODB.

Thanks, -Stolee