git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Junio C Hamano <gitster@pobox.com>
To: Stefan Beller <sbeller@google.com>
Cc: jrnieder@gmail.com, git@vger.kernel.org, mfick@codeaurora.org
Subject: Re: [PATCH v2] builtin/describe: introduce --broken flag
Date: Wed, 22 Mar 2017 10:21:37 -0700	[thread overview]
Message-ID: <xmqqinn1mdlq.fsf@gitster.mtv.corp.google.com> (raw)
In-Reply-To: <20170321225718.18633-1-sbeller@google.com> (Stefan Beller's message of "Tue, 21 Mar 2017 15:57:18 -0700")

Stefan Beller <sbeller@google.com> writes:

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

  reply	other threads:[~2017-03-22 17:22 UTC|newest]

Thread overview: 15+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-03-21  0:11 [PATCH 0/3] git-describe deals gracefully with broken submodules Stefan Beller
2017-03-21  0:11 ` [PATCH 1/3] submodule.c: port is_submodule_modified to use porcelain 2 Stefan Beller
2017-03-21  0:11 ` [PATCH 2/3] revision machinery: gentle submodule errors Stefan Beller
2017-03-21  0:11 ` [PATCH 3/3] builtin/describe: introduce --submodule-error-as-dirty flag Stefan Beller
2017-03-21  6:28 ` [PATCH 0/3] git-describe deals gracefully with broken submodules Junio C Hamano
2017-03-21  6:54   ` Junio C Hamano
2017-03-21 18:51     ` [PATCH] builtin/describe: introduce --broken flag Stefan Beller
2017-03-21 21:51       ` Junio C Hamano
2017-03-21 22:27         ` Stefan Beller
2017-03-21 22:41           ` Junio C Hamano
2017-03-21 22:50             ` Stefan Beller
2017-03-21 22:57               ` [PATCH v2] " Stefan Beller
2017-03-22 17:21                 ` Junio C Hamano [this message]
2017-03-22 21:50                 ` Jakub Narębski
2017-03-21 17:46   ` [PATCH 0/3] git-describe deals gracefully with broken submodules Stefan Beller

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=xmqqinn1mdlq.fsf@gitster.mtv.corp.google.com \
    --to=gitster@pobox.com \
    --cc=git@vger.kernel.org \
    --cc=jrnieder@gmail.com \
    --cc=mfick@codeaurora.org \
    --cc=sbeller@google.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).