Re: [PATCH v2 07/14] commit-graph: implement git-commit-graph --update-head
From: SZEDER Gábor <hidden>
Date: 2018-02-02 01:36:11
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?
quoted hunk ↗ jump to hunk
Teach git-commit-graph to write 'graph_head' upon writing a new commit graph file. Signed-off-by: Derrick Stolee <redacted> --- 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...
+--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.
quoted hunk ↗ jump to hunk
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.
+}
+
static int graph_write(void)
{
struct object_id *graph_hash = construct_commit_graph(opts.pack_dir);First the new graph file is written ...
+ if (opts.update_head) + update_head_file(opts.pack_dir, graph_hash);
... and then the new graph head, good. There could be a race if it were the other way around.
quoted hunk ↗ jump to hunk
+ if (graph_hash) printf("%s\n", oid_to_hex(graph_hash));@@ -83,6 +109,8 @@ int cmd_commit_graph(int argc, const char **argv, const char *prefix) N_("read graph file")), OPT_BOOL('w', "write", &opts.write, N_("write commit graph file")), + OPT_BOOL('u', "update-head", &opts.update_head, + N_("update graph-head to written graph file")), { OPTION_STRING, 'H', "graph-hash", &opts.graph_hash, N_("hash"), N_("A hash for a specific graph file in the pack-dir."),@@ -109,10 +137,14 @@ int cmd_commit_graph(int argc, const char **argv, const char *prefix) opts.pack_dir = strbuf_detach(&path, NULL); } + opts.has_existing = !!get_graph_head_hash(opts.pack_dir, &opts.old_graph_hash); + if (opts.read) return graph_read(); if (opts.write) return graph_write(); + if (opts.has_existing) + printf("%s\n", oid_to_hex(&opts.old_graph_hash));
It seems that a command like 'git commit-graph --read --update-head' succeeds and '--update-head' has no effect. I think it should error out. 'git commit-graph --update-head' doesn't complain, either. Would it be more appropriate to have 'read' and 'write' subcommands instead of '--read' and '--write' options? Then parse-options alone would take care of a command line like 'git commit-graph read --update-index' and error out because of unrecognized option.
quoted hunk ↗ jump to hunk
return 0; }diff --git a/commit-graph.c b/commit-graph.c index 622a650259..764e016ddb 100644 --- a/commit-graph.c +++ b/commit-graph.c@@ -35,6 +35,31 @@ #define GRAPH_MIN_SIZE (GRAPH_CHUNKLOOKUP_SIZE + GRAPH_FANOUT_SIZE + \ GRAPH_OID_LEN + sizeof(struct commit_graph_header)) +struct object_id *get_graph_head_hash(const char *pack_dir, struct object_id *hash) +{ + struct strbuf head_filename = STRBUF_INIT; + char hex[GIT_MAX_HEXSZ + 1]; + FILE *f; + + strbuf_addstr(&head_filename, pack_dir); + strbuf_addstr(&head_filename, "/graph-head"); + + f = fopen(head_filename.buf, "r"); + strbuf_release(&head_filename); + + if (!f) + return 0; + + if (!fgets(hex, sizeof(hex), f)) + die("failed to read graph-head"); + + fclose(f); + + if (get_oid_hex(hex, hash)) + return 0; + return hash; +} + char* get_commit_graph_filename_hash(const char *pack_dir, struct object_id *hash) {diff --git a/commit-graph.h b/commit-graph.h index e046ae575c..43eb0aec84 100644 --- a/commit-graph.h +++ b/commit-graph.h@@ -4,6 +4,8 @@ #include "git-compat-util.h" #include "commit.h" +extern struct object_id *get_graph_head_hash(const char *pack_dir, + struct object_id *hash); extern char* get_commit_graph_filename_hash(const char *pack_dir, struct object_id *hash);diff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh index da565624e3..d1a23bcdaf 100755 --- a/t/t5318-commit-graph.sh +++ b/t/t5318-commit-graph.sh@@ -13,7 +13,8 @@ test_expect_success 'setup full repo' \ packdir=".git/objects/pack"' test_expect_success 'write graph with no packs' \ - 'git commit-graph --write --pack-dir .' + 'git commit-graph --write --pack-dir . && + test_path_is_missing graph-head' test_expect_success 'create commits and repack' \ 'for i in $(test_seq 5)@@ -37,6 +38,7 @@ EOF test_expect_success 'write graph' \ 'graph1=$(git commit-graph --write) && test_path_is_file ${packdir}/graph-${graph1}.graph && + test_path_is_missing ${packdir}/graph-head && git commit-graph --read --graph-hash=${graph1} >output && _graph_read_expect "5" "${packdir}" && cmp expect output'@@ -90,8 +92,11 @@ test_expect_success 'Add more commits' \ # 1 test_expect_success 'write graph with merges' \ - 'graph2=$(git commit-graph --write) && + 'graph2=$(git commit-graph --write --update-head) && test_path_is_file ${packdir}/graph-${graph2}.graph && + test_path_is_file ${packdir}/graph-head && + echo ${graph2} >expect && + cmp -n 40 expect ${packdir}/graph-head &&
This check is fishy, and that '-n 40' will need adjustment once we migrate to a longer hash function. I presume you used it, because 'graph-head' contains only 40 hexdigits without a trailing newline, but 'expect' created with 'echo' does contain a newline as well, right? Then this would be better instead: printf $graph2 >expect && test_cmp expect $packdir/graph-head &&
quoted hunk ↗ jump to hunk
git commit-graph --read --graph-hash=${graph2} >output && _graph_read_expect "18" "${packdir}" && cmp expect output'@@ -107,6 +112,9 @@ test_expect_success 'setup bare repo' \ test_expect_success 'write graph in bare repo' \ 'graphbare=$(git commit-graph --write) && test_path_is_file ${baredir}/graph-${graphbare}.graph && + test_path_is_file ${baredir}/graph-head && + echo ${graphbare} >expect && + cmp -n 40 expect ${baredir}/graph-head &&
Likewise.
git commit-graph --read --graph-hash=${graphbare} >output &&
_graph_read_expect "18" "${baredir}" &&
cmp expect output'
--
2.16.0.15.g9c3cf44.dirty