Re: [PATCH v2] builtin/describe: introduce --broken flag

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 22, 2017, 17:21 UTC
Message-ID: <xmqqinn1mdlq.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <20170321225718.18633-1-sbeller@google.com>

Stefan Beller <sbeller@google.com> writes:

Show 19 quoted lines

> git-describe tells you the version number you're at, or errors out, e.g.
> when you run it outside of a repository, which may happen when downloading
> a tar ball instead of using git to obtain the source code.
>
> To keep this property of only erroring out, when not in a repository,
> severe (submodule) errors must be downgraded to reporting them gently
> instead of having git-describe error out completely.
>
> To achieve that a flag '--broken' is introduced, which is in the same
> vein as '--dirty' but uses an actual child process to check for dirtiness.
> When that child dies unexpectedly, we'll append '-broken' instead of
> '-dirty'.
>
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
>  Documentation/git-describe.txt | 11 +++++++---
>  builtin/describe.c             | 47 ++++++++++++++++++++++++++++++++++--------
>  t/t6120-describe.sh            | 20 ++++++++++++++++++
>  3 files changed, 66 insertions(+), 12 deletions(-)

I admit that my suggestion to use pushv() was done without trying it out myself, but I do agree that the result looks better, especially because the "dirty" (not "broken") side does not even have to use a separate argv_array to prepare the command line in the first place (which I failed to spot exactly because I didn't try it myself ;-).

We probably _could_ use cp.argv to point at the diff_index_args() without doing pushv(&cp.args), and that would save a bit of allocation, but it would not matter in practice as this is not a performance critical codepath.

Thanks.

Show 29 quoted lines

> diff --git a/t/t6120-describe.sh b/t/t6120-describe.sh
> index 167491fd5b..16952e44fc 100755
> --- a/t/t6120-describe.sh
> +++ b/t/t6120-describe.sh
> @@ -233,4 +233,24 @@ test_expect_success 'describe --contains and --no-match' '
>  	test_cmp expect actual
>  '
>  
> +test_expect_success 'setup and absorb a submodule' '
> +	test_create_repo sub1 &&
> +	test_commit -C sub1 initial &&
> +	git submodule add ./sub1 &&
> +	git submodule absorbgitdirs &&
> +	git commit -a -m "add submodule" &&
> +	git describe --dirty >expect &&
> +	git describe --broken >out &&
> +	test_cmp expect out
> +'
> +
> +test_expect_success 'describe chokes on severly broken submodules' '
> +	mv .git/modules/sub1/ .git/modules/sub_moved &&
> +	test_must_fail git describe --dirty
> +'
> +test_expect_success 'describe ignoring a borken submodule' '
> +	git describe --broken >out &&
> +	grep broken out
> +'
> +
>  test_done