From: Derrick Stolee Date: Mon, 05 Feb 2018 21:01:31 GMT Subject: Re: [PATCH v2 07/14] commit-graph: implement git-commit-graph --update-head Message-ID: <316c100b-2c27-83e3-53d8-46b545f0353e@gmail.com> In-Reply-To: <20180202013552.24931-1-szeder.dev@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 >> --- >> 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 [--pack-dir ] >> 'git commit-graph' --read [--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-.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 /graph-head. >> ++ >> +------------------------------------------------ >> +$ git commit-graph --pack-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 ]"), >> N_("git commit-graph --read [--graph-hash=]"), >> - N_("git commit-graph --write [--pack-dir ]"), >> + N_("git commit-graph --write [--pack-dir ] [--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-.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.