Re: [PATCH v4 07/13] commit-graph: implement --set-latest

[Derrick Stolee]

On 2/22/2018 1:31 PM, Junio C Hamano wrote:

Derrick Stolee  writes:


  static struct opts_commit_graph {
const char *obj_dir;
const char *graph_file;
+   int set_latest;
  } opts;
...
@@ -89,6 +106,8 @@ static int graph_write(int argc, const char **argv)
{ OPTION_STRING, 'o', "object-dir", _dir,
N_("dir"),
N_("The object directory to store the graph") },
+   OPT_BOOL('u', "set-latest", _latest,
+   N_("update graph-head to written graph file")),
OPT_END(),
};
  
@@ -102,6 +121,9 @@ static int graph_write(int argc, const char **argv)

graph_name = write_commit_graph(opts.obj_dir);
  
  	if (graph_name) {

+   if (opts.set_latest)
+   set_latest_file(opts.obj_dir, graph_name);
+

This feels like a very strange API from potential caller's point of
view.  Because you have to decide that you are going to mark it as
the latest one upfront before actually writing the graph file, if
you forget to pass --set-latest, you have to know how to manually
mark the file as latest anyway.  I would understand if it were one
of the following:

  (1) whenever a new commit graph file is written in the
  objects/info/ directory, always mark it as the latest (drop
  --set-latest option altogether); or

  (2) make set-latest command that takes a name of an existing graph
  file in the objects/info/ directory, and sets the latest
  pointer to point at it (make it separate from 'write' command).

though.


Perhaps the 'write' subcommand should be replaced with 'replace' which 
does the following:


1. Write a new commit graph based on the starting commits (from all 
packs, from specified packs, from OIDs).

2. Update 'graph-latest' to point to that new file.
3. Delete all "expired" commit graph files.

(Hence, we would drop the "--set-latest" and "--delete-expired" options.)

Due to the concerns with concurrency, I really don't want to split these 
operations into independent processes that consumers need to call in 
series. Since this sequence of events is the only real interaction we 
expect (for now), this interface will simplify the design.


The biggest reason I didn't design it like this in the first place is 
that the behavior changes as the patch develops.


Thanks,
-Stolee

Re: [PATCH v4 07/13] commit-graph: implement --set-latest

[Junio C Hamano]

Derrick Stolee  writes:

>  static struct opts_commit_graph {
>   const char *obj_dir;
>   const char *graph_file;
> + int set_latest;
>  } opts;
> ...
> @@ -89,6 +106,8 @@ static int graph_write(int argc, const char **argv)
>   { OPTION_STRING, 'o', "object-dir", _dir,
>   N_("dir"),
>   N_("The object directory to store the graph") },
> + OPT_BOOL('u', "set-latest", _latest,
> + N_("update graph-head to written graph file")),
>   OPT_END(),
>   };
>  
> @@ -102,6 +121,9 @@ static int graph_write(int argc, const char **argv)
>   graph_name = write_commit_graph(opts.obj_dir);
>  
>   if (graph_name) {
> + if (opts.set_latest)
> + set_latest_file(opts.obj_dir, graph_name);
> +

This feels like a very strange API from potential caller's point of
view.  Because you have to decide that you are going to mark it as
the latest one upfront before actually writing the graph file, if
you forget to pass --set-latest, you have to know how to manually
mark the file as latest anyway.  I would understand if it were one
of the following:

 (1) whenever a new commit graph file is written in the
 objects/info/ directory, always mark it as the latest (drop
 --set-latest option altogether); or

 (2) make set-latest command that takes a name of an existing graph
 file in the objects/info/ directory, and sets the latest
 pointer to point at it (make it separate from 'write' command).

though.

[PATCH v4 07/13] commit-graph: implement --set-latest

[Derrick Stolee]

It is possible to have multiple commit graph files in a directory, but
only one is important at a time.

Use a 'graph-latest' file to point to the important file. Teach
git-commit-graph to write 'graph-latest' when given the "--set-latest"
option. Using this 'graph-latest' file is more robust than relying on
directory scanning and modified times.

Signed-off-by: Derrick Stolee 
---
 Documentation/git-commit-graph.txt | 10 ++
 builtin/commit-graph.c | 26 --
 commit-graph.c |  7 +++
 commit-graph.h |  2 ++
 t/t5318-commit-graph.sh| 24 +++-
 5 files changed, 62 insertions(+), 7 deletions(-)

diff --git a/Documentation/git-commit-graph.txt 
b/Documentation/git-commit-graph.txt
index 6d26e56..dc948c5 100644
--- a/Documentation/git-commit-graph.txt
+++ b/Documentation/git-commit-graph.txt
@@ -34,6 +34,9 @@ COMMANDS
 Write a commit graph file based on the commits found in packfiles.
 Includes all commits from the existing commit graph file. Outputs the
 resulting filename.
++
+With `--set-latest` option, update the graph-latest file to point
+to the written graph file.
 
 'read'::
 
@@ -53,6 +56,13 @@ EXAMPLES
 $ git commit-graph write
 
 
+* Write a graph file for the packed commits in your local .git folder
+* and update graph-latest.
++
+
+$ git commit-graph write --set-latest
+
+
 * Read basic information from a graph file.
 +
 
diff --git a/builtin/commit-graph.c b/builtin/commit-graph.c
index 28cd097..bf86172 100644
--- a/builtin/commit-graph.c
+++ b/builtin/commit-graph.c
@@ -8,7 +8,7 @@
 static char const * const builtin_commit_graph_usage[] = {
N_("git commit-graph [--object-dir ]"),
N_("git commit-graph read [--object-dir ] [--file=]"),
-   N_("git commit-graph write [--object-dir ]"),
+   N_("git commit-graph write [--object-dir ] [--set-latest]"),
NULL
 };
 
@@ -18,13 +18,14 @@ static const char * const builtin_commit_graph_read_usage[] 
= {
 };
 
 static const char * const builtin_commit_graph_write_usage[] = {
-   N_("git commit-graph write [--object-dir ]"),
+   N_("git commit-graph write [--object-dir ] [--set-latest]"),
NULL
 };
 
 static struct opts_commit_graph {
const char *obj_dir;
const char *graph_file;
+   int set_latest;
 } opts;
 
 static int graph_read(int argc, const char **argv)
@@ -81,6 +82,22 @@ static int graph_read(int argc, const char **argv)
return 0;
 }
 
+static void set_latest_file(const char *obj_dir, const char *graph_file)
+{
+   int fd;
+   struct lock_file lk = LOCK_INIT;
+   char *latest_fname = get_graph_latest_filename(obj_dir);
+
+   fd = hold_lock_file_for_update(, latest_fname, LOCK_DIE_ON_ERROR);
+   FREE_AND_NULL(latest_fname);
+
+   if (fd < 0)
+   die_errno("unable to open graph-head");
+
+   write_in_full(fd, graph_file, strlen(graph_file));
+   commit_lock_file();
+}
+
 static int graph_write(int argc, const char **argv)
 {
char *graph_name;
@@ -89,6 +106,8 @@ static int graph_write(int argc, const char **argv)
{ OPTION_STRING, 'o', "object-dir", _dir,
N_("dir"),
N_("The object directory to store the graph") },
+   OPT_BOOL('u', "set-latest", _latest,
+   N_("update graph-head to written graph file")),
OPT_END(),
};
 
@@ -102,6 +121,9 @@ static int graph_write(int argc, const char **argv)
graph_name = write_commit_graph(opts.obj_dir);
 
if (graph_name) {
+   if (opts.set_latest)
+   set_latest_file(opts.obj_dir, graph_name);
+
printf("%s\n", graph_name);
FREE_AND_NULL(graph_name);
}
diff --git a/commit-graph.c b/commit-graph.c
index 2a8594f..5ee0805 100644
--- a/commit-graph.c
+++ b/commit-graph.c
@@ -38,6 +38,13 @@
 #define GRAPH_MIN_SIZE (GRAPH_CHUNKLOOKUP_SIZE + GRAPH_FANOUT_SIZE + \
GRAPH_OID_LEN + 8)
 
+char *get_graph_latest_filename(const char *obj_dir)
+{
+   struct strbuf fname = STRBUF_INIT;
+   strbuf_addf(, "%s/info/graph-latest", obj_dir);
+   return strbuf_detach(, 0);
+}
+
 static struct commit_graph *alloc_commit_graph(void)
 {
struct commit_graph *g = xmalloc(sizeof(*g));
diff --git a/commit-graph.h b/commit-graph.h
index 9093b97..ae24b3a 100644
--- a/commit-graph.h
+++ b/commit-graph.h
@@ -3,6 +3,8 @@
 
 #include "git-compat-util.h"
 
+extern char *get_graph_latest_filename(const char *obj_dir);
+
 struct commit_graph {
int graph_fd;
 
diff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh
index 893fa24..cad9d90 100755
---