From: Junio C Hamano <gitster@pobox•com>
To: "SZEDER Gábor" <szeder@ira•uka.de>
Cc: git@vger•kernel.org
Subject: Re: [PATCH] describe --contains: default to HEAD when no commit-ish is given
Date: Fri, 21 Aug 2015 09:30:40 -0700 [thread overview]
Message-ID: <xmqqoai07gen.fsf@gitster.dls.corp.google.com> (raw)
In-Reply-To: <1440168632-15412-1-git-send-email-szeder@ira.uka.de> ("SZEDER Gábor"'s message of "Fri, 21 Aug 2015 16:50:32 +0200")
SZEDER Gábor <szeder@ira•uka.de> writes:
> 'git describe --contains' doesn't default to HEAD when no commit is
> given, and it doesn't produce any output, not even an error:
>
> ~/src/git ((v2.5.0))$ ./git describe --contains
> ~/src/git ((v2.5.0))$ ./git describe --contains HEAD
> v2.5.0^0
Good spotting. I think defaulting to HEAD is a good move.
> diff --git a/builtin/describe.c b/builtin/describe.c
> index ce36032b1f..0e31ac5cb9 100644
> --- a/builtin/describe.c
> +++ b/builtin/describe.c
> @@ -443,10 +443,13 @@ int cmd_describe(int argc, const char **argv, const char *prefix)
> if (pattern)
> argv_array_pushf(&args, "--refs=refs/tags/%s", pattern);
> }
> - while (*argv) {
> - argv_array_push(&args, *argv);
> - argv++;
> - }
> + if (argc)
"What would this code do to 'describe --all --contains'?" was my
knee-jerk reaction, but the options are all parsed by this code and
nothing is delegated to name-rev, so 'if (!argc)' here is truly the
lack of any revisions to be described, which is good.
> + while (*argv) {
> + argv_array_push(&args, *argv);
> + argv++;
> + }
> + else
> + argv_array_push(&args, "HEAD");
By the way, I usually prefer a fatter 'else' clause when everything
else is equal, i.e.
if (!argc)
argv_array_push(&args, "HEAD"); /* default to HEAD */
else {
while (*argv) {
...
}
}
because it is easy to miss tiny else-clause while reading code, but
it is harder to miss tiny then-clause. In this case, however, the
while loop can be replaced with argv_array_pushv() these days, so
perhaps
if (!argc)
argv_array_push(&args, "HEAD"); /* default to HEAD ... */
else
argv_array_pushv(&args, argv); /* or relay what we got */
or something?
> return cmd_name_rev(args.argc, args.argv, prefix);
> }
>
> diff --git a/t/t6120-describe.sh b/t/t6120-describe.sh
> index 57d50156bb..bf52a0a1cc 100755
> --- a/t/t6120-describe.sh
> +++ b/t/t6120-describe.sh
> @@ -115,6 +115,14 @@ check_describe e-3-* --first-parent --tags
>
> check_describe $(git rev-parse --short HEAD) --exact-match --always HEAD
>
> +test_expect_success 'describe --contains defaults to HEAD without commit-ish' '
> + echo "A^0" >expect &&
> + git checkout A &&
> + test_when_finished "git checkout -" &&
> + git describe --contains >actual &&
> + test_cmp expect actual
> +'
> : >err.expect
> check_describe A --all A^0
> test_expect_success 'no warning was displayed for A' '
next prev parent reply other threads:[~2015-08-21 16:30 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-08-20 12:13 [PATCH] describe: make '--always' fallback work after '--exact-match' failed SZEDER Gábor
2015-08-20 18:39 ` Junio C Hamano
2015-08-21 11:40 ` SZEDER Gábor
2015-08-21 15:55 ` Junio C Hamano
2015-08-21 14:50 ` [PATCH] describe --contains: default to HEAD when no commit-ish is given SZEDER Gábor
2015-08-21 16:30 ` Junio C Hamano [this message]
2015-08-24 16:14 ` SZEDER Gábor
2015-08-24 16:15 ` [PATCH v2] " SZEDER Gábor
2015-08-24 18:47 ` [PATCH] " Junio C Hamano
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
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=xmqqoai07gen.fsf@gitster.dls.corp.google.com \
--to=gitster@pobox$(echo .)com \
--cc=git@vger$(echo .)kernel.org \
--cc=szeder@ira$(echo .)uka.de \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox