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

From: SZEDER Gábor <szeder.dev@gmail.com>
Date: Feb 1, 2018, 23:48 UTC
Message-ID: <20180201234805.7506-1-szeder.dev@gmail.com>
In-Reply-To: <1517348383-112294-6-git-send-email-dstolee@microsoft.com>

Show 25 quoted lines

> 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.

Show 12 quoted lines

> +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

Show 18 quoted lines

> 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.

Show 17 quoted lines

> +
> +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.

Show 66 quoted lines

> +    '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