Re: [PATCH v2 07/14] commit-graph: implement git-commit-graph --update-head

[Derrick Stolee <stolee@gmail.com>]

On 2/1/2018 8:35 PM, SZEDER Gábor wrote:
>> It is possible to have multiple commit graph files in a pack directory,
>> but only one is important at a time. Use a 'graph_head' file to point
>> to the important file.
> This implies that all those other files are ignored, right?

Yes. We do not use directory listings to find graph files.

>
>> Teach git-commit-graph to write 'graph_head' upon
>> writing a new commit graph file.
>>
>> Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
>> ---
>>   Documentation/git-commit-graph.txt | 34 ++++++++++++++++++++++++++++++++++
>>   builtin/commit-graph.c             | 38 +++++++++++++++++++++++++++++++++++---
>>   commit-graph.c                     | 25 +++++++++++++++++++++++++
>>   commit-graph.h                     |  2 ++
>>   t/t5318-commit-graph.sh            | 12 ++++++++++--
>>   5 files changed, 106 insertions(+), 5 deletions(-)
>>
>> diff --git a/Documentation/git-commit-graph.txt b/Documentation/git-commit-graph.txt
>> index 09aeaf6c82..99ced16ddc 100644
>> --- a/Documentation/git-commit-graph.txt
>> +++ b/Documentation/git-commit-graph.txt
>> @@ -12,15 +12,49 @@ SYNOPSIS
>>   'git commit-graph' --write <options> [--pack-dir <pack_dir>]
>>   'git commit-graph' --read <options> [--pack-dir <pack_dir>]
>>   
>> +OPTIONS
>> +-------
> Oh, look, the 'OPTIONS' section I missed in the previous patches! ;)
>
> This should be split up and squashed into the previous patches where
> the individual --options are first mentioned.
>
>> +--pack-dir::
>> +	Use given directory for the location of packfiles, graph-head,
>> +	and graph files.
>> +
>> +--read::
>> +	Read a graph file given by the graph-head file and output basic
>> +	details about the graph file. (Cannot be combined with --write.)
>  From the output of 'git commit-graph --read' it seems that it's not a
> generally useful option to the user.  Perhaps it should be mentioned
> that it's only intended as a debugging aid?  Or maybe it doesn't
> really matter, because eventually this command will become irrelevant,
> as other commands (clone, fetch, gc) will invoke it automagically...

I'll add some wording to make this clear.

>
>> +--graph-id::
>> +	When used with --read, consider the graph file graph-<oid>.graph.
>> +
>> +--write::
>> +	Write a new graph file to the pack directory. (Cannot be combined
>> +	with --read.)
> I think this should also mention that it prints the generated graph
> file's checksum.
>
>> +
>> +--update-head::
>> +	When used with --write, update the graph-head file to point to
>> +	the written graph file.
> So it should be used with '--write', noted.
>
>>   EXAMPLES
>>   --------
>>   
>> +* Output the hash of the graph file pointed to by <dir>/graph-head.
>> ++
>> +------------------------------------------------
>> +$ git commit-graph --pack-dir=<dir>
>> +------------------------------------------------
>> +
>>   * Write a commit graph file for the packed commits in your local .git folder.
>>   +
>>   ------------------------------------------------
>>   $ git commit-graph --write
>>   ------------------------------------------------
>>   
>> +* Write a graph file for the packed commits in your local .git folder,
>> +* and update graph-head.
>> ++
>> +------------------------------------------------
>> +$ git commit-graph --write --update-head
>> +------------------------------------------------
>> +
>>   * Read basic information from a graph file.
>>   +
>>   ------------------------------------------------
>> diff --git a/builtin/commit-graph.c b/builtin/commit-graph.c
>> index 218740b1f8..d73cbc907d 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 [--pack-dir <packdir>]"),
>>   	N_("git commit-graph --read [--graph-hash=<hash>]"),
>> -	N_("git commit-graph --write [--pack-dir <packdir>]"),
>> +	N_("git commit-graph --write [--pack-dir <packdir>] [--update-head]"),
>>   	NULL
>>   };
>>   
>> @@ -20,6 +20,9 @@ static struct opts_commit_graph {
>>   	int read;
>>   	const char *graph_hash;
>>   	int write;
>> +	int update_head;
>> +	int has_existing;
>> +	struct object_id old_graph_hash;
>>   } opts;
>>   
>>   static int graph_read(void)
>> @@ -30,8 +33,8 @@ static int graph_read(void)
>>   
>>   	if (opts.graph_hash && strlen(opts.graph_hash) == GIT_MAX_HEXSZ)
>>   		get_oid_hex(opts.graph_hash, &graph_hash);
>> -	else
>> -		die("no graph hash specified");
>> +	else if (!get_graph_head_hash(opts.pack_dir, &graph_hash))
>> +		die("no graph-head exists");
>>   
>>   	graph_file = get_commit_graph_filename_hash(opts.pack_dir, &graph_hash);
>>   	graph = load_commit_graph_one(graph_file, opts.pack_dir);
>> @@ -62,10 +65,33 @@ static int graph_read(void)
>>   	return 0;
>>   }
>>   
>> +static void update_head_file(const char *pack_dir, const struct object_id *graph_hash)
>> +{
>> +	struct strbuf head_path = STRBUF_INIT;
>> +	int fd;
>> +	struct lock_file lk = LOCK_INIT;
>> +
>> +	strbuf_addstr(&head_path, pack_dir);
>> +	strbuf_addstr(&head_path, "/");
>> +	strbuf_addstr(&head_path, "graph-head");
> strbuf_addstr(&head_path, "/graph-head"); ?
>
>> +
>> +	fd = hold_lock_file_for_update(&lk, head_path.buf, LOCK_DIE_ON_ERROR);
>> +	strbuf_release(&head_path);
>> +
>> +	if (fd < 0)
>> +		die_errno("unable to open graph-head");
>> +
>> +	write_in_full(fd, oid_to_hex(graph_hash), GIT_MAX_HEXSZ);
>> +	commit_lock_file(&lk);
> The new graph-head file will be writable.  All other files in
> .git/objects/pack are created read-only, including graph files.  Just
> pointing it out, but I don't think it's a bit deal; other than
> consistency with the permissions of other files I don't have any
> argument for making it read-only.

I don't have strong opinions on the permissions difference, except that 
a graph-<hash>.graph file should not change contents (or the hash will 
be wrong) but a user or external tool could change the graph-head 
contents to point to a different file. I can't think of a case where 
that is important.