Re: [PATCH v4 05/13] commit-graph: implement 'git-commit-graph write'

[Junio C Hamano]

Derrick Stolee  writes:

> +static int graph_write(int argc, const char **argv)
> +{
> + ...
> + graph_name = write_commit_graph(opts.obj_dir);
> +
> + if (graph_name) {
> + printf("%s\n", graph_name);
> + FREE_AND_NULL(graph_name);
> + }
> +
> + return 0;
> +}

After successfully writing a graph file out, write_commit_graph()
signals that fact by returning a non-NULL pointer, so that this
caller can report the filename to the end user.  This caller
protects itself from a NULL return, presumably because the callee
uses it to signal an error when writing the graph file out?  

Is it OK to lose that 1-bit of information, or should we have more like

if (graph_name) {
printf;
return 0;
} else {
return -1;
}

>  int cmd_commit_graph(int argc, const char **argv, const char *prefix)
>  {
>   static struct option builtin_commit_graph_options[] = {
> - { OPTION_STRING, 'p', "object-dir", _dir,
> + { OPTION_STRING, 'o', "object-dir", _dir,
>   N_("dir"),
>   N_("The object directory to store the graph") },
>   OPT_END(),

The same comment for a no-op patch from an earlier step applies
here, and we have another one that we saw above in graph_write().

> @@ -31,6 +67,11 @@ int cmd_commit_graph(int argc, const char **argv, const 
> char *prefix)
>builtin_commit_graph_usage,
>PARSE_OPT_STOP_AT_NON_OPTION);
>  
> + if (argc > 0) {
> + if (!strcmp(argv[0], "write"))
> + return graph_write(argc, argv);

And if we fix "graph_write" to report an error with negative return,
this needs to become something like

return !!graph_write(argc, argv);

as we do not want to return a negative value to be passed via
run_builtin() to exit(3) in handle_builtin().

> diff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh
> new file mode 100755
> index 000..6a5e93c
> --- /dev/null
> +++ b/t/t5318-commit-graph.sh
> @@ -0,0 +1,119 @@
> +#!/bin/sh
> +
> +test_description='commit graph'
> +. ./test-lib.sh
> +
> +test_expect_success 'setup full repo' '
> + rm -rf .git &&

I am perfectly OK with creating a separate subdirectory called
'full' in the trash directory given by the test framework, but
unless absolutely necessary I'd rather not to see "rm -rf", 
especially on ".git", in our test scripts.  People can screw up
doing various things (like copying and pasting).

> + mkdir full &&
> + cd full &&
> + git init &&
> + objdir=".git/objects"
> +'

And I absolutely do not want to see "cd full" that leaves and stays
in the subdirectory after this step is done.  

Imagine what happens if any earlier step fails before doing "cd
full", causing this "setup full" step to report failure, and then
the test goes on to the next step?  We will not be in "full" and
worse yet because we do not have "$TRASH_DIRECTORY/.git" (you
removed it), the "git commit-graph write --object-dir" command we
end up doing next will see the git source repository as the
repository it is working on.  Never risk trashing our source
repository with your test.  That is why we give you $TRASH_DIRECTORY
to play in.  Make use of it when you can.

I'd make this step just a single

git init full

and then the next one

git -C full commit-graph write --object-dir .

In later tests that have multi-step things, I'd instead make them

(
cd full &&
... whatever you do  &&
... in that separate  &&
... 'full' repository
)

if I were writing this test *and* if I really wanted to do things
inside $TRASH_DIRECTORY/full/.git repository.  I am not convinced
yet about the latter.  I know that it will make certain things
simpler to use a separate /full hierarchy (e.g. cleaning up, having
another unrelated test repository, etc.) while making other things
more cumbersome (e.g. you need to be careful when you "cd" and the
easiest way to do so is to ( do things in a subshell )).  I just do
not know what the trade-off would look like in this particular case.

A simple rule of thumb I try to follow is not to change $(pwd) for
the process that runs these test_expect_success shell functions.

> +
> +test_expect_success 'write graph with no packs' '
> + git commit-graph write --object-dir .
> +'
> +
> +test_expect_success 'create commits and repack' '
> + for i in $(test_seq 3)
> + do
> + test_commit $i &&
> + git branch commits/$i
> + done &&
> + git repack
> +'

[PATCH v4 05/13] commit-graph: implement 'git-commit-graph write'

[Derrick Stolee]

Teach git-commit-graph to write graph files. Create new test script to verify
this command succeeds without failure.

Signed-off-by: Derrick Stolee 
---
 Documentation/git-commit-graph.txt |  40 +
 builtin/commit-graph.c |  43 +-
 t/t5318-commit-graph.sh| 119 +
 3 files changed, 201 insertions(+), 1 deletion(-)
 create mode 100755 t/t5318-commit-graph.sh

diff --git a/Documentation/git-commit-graph.txt 
b/Documentation/git-commit-graph.txt
index e1c3078..c3f222f 100644
--- a/Documentation/git-commit-graph.txt
+++ b/Documentation/git-commit-graph.txt
@@ -5,6 +5,46 @@ NAME
 
 git-commit-graph - Write and verify Git commit graphs (.graph files)
 
+
+SYNOPSIS
+
+[verse]
+'git commit-graph write'  [--object-dir ]
+
+
+DESCRIPTION
+---
+
+Manage the serialized commit graph file.
+
+
+OPTIONS
+---
+--object-dir::
+   Use given directory for the location of packfiles and graph files.
+   The graph files will be in /info and the packfiles are expected
+   to be in /pack.
+
+
+COMMANDS
+
+'write'::
+
+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.
+
+
+EXAMPLES
+
+
+* Write a commit graph file for the packed commits in your local .git folder.
++
+
+$ git commit-graph write
+
+
+
 GIT
 ---
 Part of the linkgit:git[1] suite
diff --git a/builtin/commit-graph.c b/builtin/commit-graph.c
index 98110bb..a51d87b 100644
--- a/builtin/commit-graph.c
+++ b/builtin/commit-graph.c
@@ -1,9 +1,18 @@
 #include "builtin.h"
 #include "config.h"
+#include "dir.h"
+#include "lockfile.h"
 #include "parse-options.h"
+#include "commit-graph.h"
 
 static char const * const builtin_commit_graph_usage[] = {
N_("git commit-graph [--object-dir ]"),
+   N_("git commit-graph write [--object-dir ]"),
+   NULL
+};
+
+static const char * const builtin_commit_graph_write_usage[] = {
+   N_("git commit-graph write [--object-dir ]"),
NULL
 };
 
@@ -11,11 +20,38 @@ static struct opts_commit_graph {
const char *obj_dir;
 } opts;
 
+static int graph_write(int argc, const char **argv)
+{
+   char *graph_name;
+
+   static struct option builtin_commit_graph_write_options[] = {
+   { OPTION_STRING, 'o', "object-dir", _dir,
+   N_("dir"),
+   N_("The object directory to store the graph") },
+   OPT_END(),
+   };
+
+   argc = parse_options(argc, argv, NULL,
+builtin_commit_graph_write_options,
+builtin_commit_graph_write_usage, 0);
+
+   if (!opts.obj_dir)
+   opts.obj_dir = get_object_directory();
+
+   graph_name = write_commit_graph(opts.obj_dir);
+
+   if (graph_name) {
+   printf("%s\n", graph_name);
+   FREE_AND_NULL(graph_name);
+   }
+
+   return 0;
+}
 
 int cmd_commit_graph(int argc, const char **argv, const char *prefix)
 {
static struct option builtin_commit_graph_options[] = {
-   { OPTION_STRING, 'p', "object-dir", _dir,
+   { OPTION_STRING, 'o', "object-dir", _dir,
N_("dir"),
N_("The object directory to store the graph") },
OPT_END(),
@@ -31,6 +67,11 @@ int cmd_commit_graph(int argc, const char **argv, const char 
*prefix)
 builtin_commit_graph_usage,
 PARSE_OPT_STOP_AT_NON_OPTION);
 
+   if (argc > 0) {
+   if (!strcmp(argv[0], "write"))
+   return graph_write(argc, argv);
+   }
+
usage_with_options(builtin_commit_graph_usage,
   builtin_commit_graph_options);
 }
diff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh
new file mode 100755
index 000..6a5e93c
--- /dev/null
+++ b/t/t5318-commit-graph.sh
@@ -0,0 +1,119 @@
+#!/bin/sh
+
+test_description='commit graph'
+. ./test-lib.sh
+
+test_expect_success 'setup full repo' '
+   rm -rf .git &&
+   mkdir full &&
+   cd full &&
+   git init &&
+   objdir=".git/objects"
+'
+
+test_expect_success 'write graph with no packs' '
+   git commit-graph write --object-dir .
+'
+
+test_expect_success 'create commits and repack' '
+   for i in $(test_seq 3)
+   do
+   test_commit $i &&
+   git branch commits/$i
+   done &&
+   git repack
+'
+
+test_expect_success 'write graph' '
+   graph1=$(git commit-graph write) &&
+   test_path_is_file $objdir/info/$graph1
+'
+
+test_expect_success 'Add more commits' '
+   git reset --hard commits/1 &&
+   for i in $(test_seq 4 5)
+   do
+   test_commit $i &&
+