Re: [PATCH] rev-parse: rev-parse: add --is-shallow-repository

From: Jonathan Nieder <jrnieder@gmail.com>
Date: Sep 19, 2017, 02:33 UTC
Message-ID: <20170919023349.GA175206@aiede.mtv.corp.google.com>
In-Reply-To: <20170918170429.8328-1-oystwa@gmail.com>

Hi,

Øystein Walle wrote:

> Running `git fetch --unshallow` on a repo that is not in fact shallow
> produces a fatal error message.

Hm, can you say more about the context? From a certain point of view, it might make sense for that command to succeed instead: if the repo is already unshallow, then why should't "fetch --unshallow" complain instead of declaring victory?

Show 9 quoted lines

>                                 Add a helper to rev-parse that scripters
> can use to determine whether a repo is shallow or not.
>
> Signed-off-by: Øystein Walle <oystwa@gmail.com>
> ---
>  Documentation/git-rev-parse.txt |  3 +++
>  builtin/rev-parse.c             |  5 +++++
>  t/t1500-rev-parse.sh            | 15 +++++++++++++++
>  3 files changed, 23 insertions(+)

Regardless, this new rev-parse --is-shallow helper looks like a good feature.

[...]

Show 11 quoted lines

> --- a/builtin/rev-parse.c
> +++ b/builtin/rev-parse.c
> @@ -868,6 +868,11 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)
>  						: "false");
>  				continue;
>  			}
> +			if (!strcmp(arg, "--is-shallow-repository")) {
> +				printf("%s\n", is_repository_shallow() ? "true"
> +						: "false");
> +				continue;
> +			}

The implementation is straightforward and correct.

[...]

> --- a/t/t1500-rev-parse.sh
> +++ b/t/t1500-rev-parse.sh

Thanks for writing tests. \o/

Show 5 quoted lines

> @@ -116,6 +116,21 @@ test_expect_success 'git-path inside sub-dir' '
>  	test_cmp expect actual
>  '
>  
> +test_expect_success 'git-path shallow repository' '

What does git-path mean here? I wonder if it's a copy/paste error. Did you mean something like

 test_expect_success 'rev-parse --is-shallow-repository in shallow repo' '

Show 9 quoted lines

> +	test_commit test_commit &&
> +	echo true >expect &&
> +	git clone --depth 1 --no-local . shallow &&
> +	test_when_finished "rm -rf shallow" &&
> +	git -C shallow rev-parse --is-shallow-repository >actual &&
> +	test_cmp expect actual
> +'
> +
> +test_expect_success 'git-path notshallow repository' '

Likewise: should this be

 test_expect_success 'rev-parse --is-shallow-repository in non-shallow repo' '

Show 6 quoted lines

> +	echo false >expect &&
> +	git rev-parse --is-shallow-repository >actual &&
> +	test_cmp expect actual
> +'
> +
>  test_expect_success 'showing the superproject correctly' '

With the two tweaks mentioned above,
Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>

Thanks.