Re: [PATCH v2] rev-list: add option for --pretty=format without header

[Christian Couder <christian.couder@gmail.com>]

On Sun, Jul 11, 2021 at 11:55 PM brian m. carlson
<sandals@crustytoothpaste.net> wrote:
>
> In general, we encourage users to use plumbing commands, like git
> rev-list, over porcelain commands, like git log, when scripting.
> However, git rev-list has one glaring problem that prevents it from
> being used in certain cases: when --pretty is used with a custom format,
> it always prints out a line containing "commit" and the object ID.  This
> makes it unsuitable for many scripting needs, and forces users to use
> git log instead.
>
> While we can't change this behavior for backwards compatibility, we can
> add an option to suppress this behavior, so let's do so, and call it
> "--no-commit-header".  Additionally, add the corresponding positive
> option to switch it back on.
>
> Note that this option doesn't affect the built-in formats, only custom
> formats.  This is exactly the same behavior as users already have from
> git log and is what most users will be used to.
>
> Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
> ---
> Changes from v1:
> * Ensure this applies only to custom formats.
> * Fix the issue with --abbrev-commit mentioned by Junio.
> * Add tests for additional cases, including built-in formats.
> * Update documentation to reflect this data.

Thanks! This version mostly looks good to me!

> diff --git a/t/t6006-rev-list-format.sh b/t/t6006-rev-list-format.sh
> index 35a2f62392..41d0ca00b1 100755
> --- a/t/t6006-rev-list-format.sh
> +++ b/t/t6006-rev-list-format.sh
> @@ -41,22 +41,59 @@ test_expect_success 'setup' '
>         echo "$added_iso88591" | git commit -F - &&
>         head1=$(git rev-parse --verify HEAD) &&
>         head1_short=$(git rev-parse --verify --short $head1) &&
> +       head1_short4=$(git rev-parse --verify --short=4 $head1) &&
>         tree1=$(git rev-parse --verify HEAD:) &&
>         tree1_short=$(git rev-parse --verify --short $tree1) &&
>         echo "$changed" > foo &&
>         echo "$changed_iso88591" | git commit -a -F - &&
>         head2=$(git rev-parse --verify HEAD) &&
>         head2_short=$(git rev-parse --verify --short $head2) &&
> +       head2_short4=$(git rev-parse --verify --short=4 $head2) &&
>         tree2=$(git rev-parse --verify HEAD:) &&
>         tree2_short=$(git rev-parse --verify --short $tree2) &&
>         git config --unset i18n.commitEncoding
>  '
>
> -# usage: test_format name format_string [failure] <expected_output
> +# usage: test_format [argument...] name format_string [failure] <expected_output

Usually we use "option" instead of "argument" for the flags starting
with "-" or "--" before the required parameter. For example:

$ git rev-list -h
usage: git rev-list [OPTION] <commit-id>... [ -- paths... ]
...

(Yeah, I agree that [OPTION] is not very consistent with what other
commands show, which is usually "[<options>]".)

>  test_format () {
> +       local args=

Here...

> +       while true
> +       do
> +               case "$1" in
> +               --*)
> +                       args="$args $1"

...here...

> +                       shift;;
> +               *)
> +                       break;;
> +               esac
> +       done
>         cat >expect.$1
>         test_expect_${3:-success} "format $1" "
> -               git rev-list --pretty=format:'$2' main >output.$1 &&
> +               git rev-list $args --pretty=format:'$2' main >output.$1 &&

...and here "opts" might be better than "args".

> +               test_cmp expect.$1 output.$1
> +       "
> +}
> +
> +# usage: test_pretty [argument...] name format_name [failure] <expected_output

Here...

> +test_pretty () {
> +       local args=

...and in this function too.

> +       while true
> +       do
> +               case "$1" in
> +               --*)
> +                       args="$args $1"
> +                       shift;;
> +               *)
> +                       break;;
> +               esac
> +       done
> +       cat >expect.$1
> +       test_expect_${3:-success} "pretty $1 (without --no-commit-header)" "
> +               git rev-list $args --pretty='$2' main >output.$1 &&
> +               test_cmp expect.$1 output.$1
> +       "
> +       test_expect_${3:-success} "pretty $1 (with --no-commit-header)" "
> +               git rev-list $args --no-commit-header --pretty='$2' main >output.$1 &&
>                 test_cmp expect.$1 output.$1
>         "
>  }

[...]

> +test_pretty oneline oneline <<EOF
> +$head2 $changed
> +$head1 $added
> +EOF
> +
> +test_pretty short short <<EOF

test_pretty() accepts options like --no-commit-header, but it's only
used without any option. So I wonder if you forgot to add a few tests
with some options.

> +commit $head2
> +Author: $GIT_AUTHOR_NAME <$GIT_AUTHOR_EMAIL>
> +
> +    $changed
> +
> +commit $head1
> +Author: $GIT_AUTHOR_NAME <$GIT_AUTHOR_EMAIL>
> +
> +    $added
> +
> +EOF
> +
>  test_expect_success 'basic colors' '
>         cat >expect <<-EOF &&
>         commit $head2