Re: [PATCH v2 05/14] commit-graph: implement git-commit-graph --write

>> Teach git-commit-graph to write graph files. Create new test script to verify
>> this command succeeds without failure.
>>
>> Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
>> ---
>>   Documentation/git-commit-graph.txt | 18 +++++++
>>   builtin/commit-graph.c             | 30 ++++++++++++
>>   t/t5318-commit-graph.sh            | 96 ++++++++++++++++++++++++++++++++++++++
>>   3 files changed, 144 insertions(+)
>>   create mode 100755 t/t5318-commit-graph.sh
>>
>> diff --git a/Documentation/git-commit-graph.txt b/Documentation/git-commit-graph.txt
>> index c8ea548dfb..3f3790d9a8 100644
>> --- a/Documentation/git-commit-graph.txt
>> +++ b/Documentation/git-commit-graph.txt
>> @@ -5,3 +5,21 @@ NAME
>>   ----
>>   git-commit-graph - Write and verify Git commit graphs (.graph files)
>>   
>> +
>> +SYNOPSIS
>> +--------
>> +[verse]
>> +'git commit-graph' --write <options> [--pack-dir <pack_dir>]
>> +
> What do these options do and what is the command's output?  IOW, an
> 'OPTIONS' section would be nice.
>
>> +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/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh
>> new file mode 100755
>> index 0000000000..6bcd1cc264
>> --- /dev/null
>> +++ b/t/t5318-commit-graph.sh
>> @@ -0,0 +1,96 @@
>> +#!/bin/sh
>> +
>> +test_description='commit graph'
>> +. ./test-lib.sh
>> +
>> +test_expect_success 'setup full repo' \
>> +    'rm -rf .git &&
>> +     mkdir full &&
>> +     cd full &&
>> +     git init &&
>> +     git config core.commitgraph true &&
>> +     git config pack.threads 1 &&
> Does this pack.threads=1 make a difference?
>
>> +     packdir=".git/objects/pack"'
> We tend to put single quotes around tests like this:
>
>    test_expect_success 'setup full repo' '
>          do-this &&
>          check-that
>    '
>
> This is not a mere style nit: those newlines before and after the test
> block make the test's output with '--verbose-log' slightly more
> readable.
>
> Furthermore, we prefer tabs for indentation.

> Finally, 'cd'-ing around such that it affects subsequent tests is
> usually frowned upon.  However, in this particular case (going into
> one repo, doing a bunch of tests there, then going into another repo,
> and doing another bunch of tests) I think it's better than changing
> directory in a subshell in every single test.
>
>> +
>> +test_expect_success 'write graph with no packs' \
>> +    'git commit-graph --write --pack-dir .'
>> +
>> +test_expect_success 'create commits and repack' \
>> +    'for i in $(test_seq 5)
>> +     do
>> +        echo $i >$i.txt &&
>> +        git add $i.txt &&
>> +        git commit -m "commit $i" &&
>> +        git branch commits/$i
>> +     done &&
>> +     git repack'
>> +
>> +test_expect_success 'write graph' \
>> +    'graph1=$(git commit-graph --write) &&
>> +     test_path_is_file ${packdir}/graph-${graph1}.graph'
> Style nit:  those {} around the variable names are unnecessary, but I
> see you use them a lot.
>
>> +
>> +t_expect_success 'Add more commits' \
> This must be test_expect_success.
>
>> +    'git reset --hard commits/3 &&
>> +     for i in $(test_seq 6 10)
>> +     do
>> +        echo $i >$i.txt &&
>> +        git add $i.txt &&
>> +        git commit -m "commit $i" &&
>> +        git branch commits/$i
>> +     done &&
>> +     git reset --hard commits/3 &&
>> +     for i in $(test_seq 11 15)
>> +     do
>> +        echo $i >$i.txt &&
>> +        git add $i.txt &&
>> +        git commit -m "commit $i" &&
>> +        git branch commits/$i
>> +     done &&
>> +     git reset --hard commits/7 &&
>> +     git merge commits/11 &&
>> +     git branch merge/1 &&
>> +     git reset --hard commits/8 &&
>> +     git merge commits/12 &&
>> +     git branch merge/2 &&
>> +     git reset --hard commits/5 &&
>> +     git merge commits/10 commits/15 &&
>> +     git branch merge/3 &&
>> +     git repack'
>> +
>> +# Current graph structure:
>> +#
>> +#      M3
>> +#     / |\_____
>> +#    / 10      15
>> +#   /   |      |
>> +#  /    9 M2   14
>> +# |     |/  \  |
>> +# |     8 M1 | 13
>> +# |     |/ | \_|
>> +# 5     7  |   12
>> +# |     |   \__|
>> +# 4     6      11
>> +# |____/______/
>> +# 3
>> +# |
>> +# 2
>> +# |
>> +# 1
>> +
>> +test_expect_success 'write graph with merges' \
>> +    'graph2=$(git commit-graph --write) &&
>> +     test_path_is_file ${packdir}/graph-${graph2}.graph'
>> +
>> +test_expect_success 'setup bare repo' \
>> +    'cd .. &&
>> +     git clone --bare full bare &&
>> +     cd bare &&
>> +     git config core.graph true &&
>> +     git config pack.threads 1 &&
>> +     baredir="objects/pack"'
>> +
>> +test_expect_success 'write graph in bare repo' \
>> +    'graphbare=$(git commit-graph --write) &&
>> +     test_path_is_file ${baredir}/graph-${graphbare}.graph'
>> +
>> +test_done
>> -- 
>> 2.16.0.15.g9c3cf44.dirty
>