threads / patch / 40138

patchdescribe: make '--always' fallback work after '--exact-match' failed

Subject: [PATCH] describe: make '--always' fallback work after '--exact-match' failed

## tl;dr

9 messages between Aug 20, 2015 and Aug 24, 2015. Diffs are folded; open one to read it.

replies: 8people: 2as markdown or json

SZEDER Gábor· Aug 20, 2015, 12:13 UTC · lore

'git describe [...] --always' should always show the unique abbreviated object name as a fallback when the given commit cannot be described with the given set of options, see da2478dbb0 (describe --always: fall back to showing an abbreviated object name, 2008-03-02).

However, this is not the case when the combination '--exact-match --always' is given and the commit cannot be described, because in such cases 'git describe' errors out, as if '--always' were not given at all.

Respect '--always' and print the unique abbreviated object name instead of erroring out when the commit cannot be described with '--exact-match --always'.

Signed-off-by: SZEDER Gábor <szeder@ira.uka.de>
---
 builtin/describe.c  | 2 +-
 t/t6120-describe.sh | 2 ++
 2 files changed, 3 insertions(+), 1 deletion(-)
Show changes to 2 files +3 −1

builtin/describe.c, t/t6120-describe.sh

diff --git a/builtin/describe.c b/builtin/describe.c
index a36c829e57..ce36032b1f 100644
--- a/builtin/describe.c
+++ b/builtin/describe.c
@@ -266,7 +266,7 @@ static void describe(const char *arg, int last_one)
 		return;
 	}
 
-	if (!max_candidates)
+	if (!always && !max_candidates)
 		die(_("no tag exactly matches '%s'"), sha1_to_hex(cmit->object.sha1));
 	if (debug)
 		fprintf(stderr, _("searching to describe %s\n"), arg);
diff --git a/t/t6120-describe.sh b/t/t6120-describe.sh
index c0e5b2a627..57d50156bb 100755
--- a/t/t6120-describe.sh
+++ b/t/t6120-describe.sh
@@ -113,6 +113,8 @@ check_describe A-3-* --long HEAD^^2
 check_describe c-7-* --tags
 check_describe e-3-* --first-parent --tags
 
+check_describe $(git rev-parse --short HEAD) --exact-match --always HEAD
+
 : >err.expect
 check_describe A --all A^0
 test_expect_success 'no warning was displayed for A' '

-- 
2.5.0.343.g6f38143
Junio C Hamano· Aug 20, 2015, 18:39 UTC · re: SZEDER Gábor · lore

Re: [PATCH] describe: make '--always' fallback work after '--exact-match' failed

SZEDER Gábor <szeder@ira.uka.de> writes:
Show 14 quoted lines
> 'git describe [...] --always' should always show the unique abbreviated
> object name as a fallback when the given commit cannot be described with
> the given set of options, see da2478dbb0 (describe --always: fall back
> to showing an abbreviated object name, 2008-03-02).
>
> However, this is not the case when the combination '--exact-match
> --always' is given and the commit cannot be described, because in such
> cases 'git describe' errors out, as if '--always' were not given at all.
>
> Respect '--always' and print the unique abbreviated object name instead
> of erroring out when the commit cannot be described with '--exact-match
> --always'.
>
> Signed-off-by: SZEDER Gábor <szeder@ira.uka.de>

Well, that can be argued both ways. Your patch introduces a regression, as "--exact-match" is an instruction to error out when no tag exactly matches, and you deliberately break that.

My knee-jerk reaction is that the most sensible way forward is to make --exact-match and --always mutually incompatible.

SZEDER Gábor· Aug 21, 2015, 11:40 UTC · re: Junio C Hamano · lore

Re: [PATCH] describe: make '--always' fallback work after '--exact-match' failed

Quoting Junio C Hamano <gitster@pobox.com>:
Show 20 quoted lines
> SZEDER Gábor <szeder@ira.uka.de> writes:
>
>> 'git describe [...] --always' should always show the unique abbreviated
>> object name as a fallback when the given commit cannot be described with
>> the given set of options, see da2478dbb0 (describe --always: fall back
>> to showing an abbreviated object name, 2008-03-02).
>>
>> However, this is not the case when the combination '--exact-match
>> --always' is given and the commit cannot be described, because in such
>> cases 'git describe' errors out, as if '--always' were not given at all.
>>
>> Respect '--always' and print the unique abbreviated object name instead
>> of erroring out when the commit cannot be described with '--exact-match
>> --always'.
>>
>> Signed-off-by: SZEDER Gábor <szeder@ira.uka.de>
>
> Well, that can be argued both ways.  Your patch introduces a
> regression, as "--exact-match" is an instruction to error out when
> no tag exactly matches, and you deliberately break that.

This patch doesn't break '--exact-match', in fact doesn't modify it at all when it's on its own or combined with other options, but it makes '--exact-match --always' finally work.

'git describe' errors out by default if it can't describe the given commit. So if a user wants an exact match or an error otherwise, then he should not give '--always' at all, because that's the instruction to not error out but give the abbreviated object name instead.

Why should '--exact-match' be any different from the other options that tell 'git describe' what to use for the description? Why should '--always' not work with '--exact-match', when it works in the other cases?

Consider '--contains': it should find a tag that comes after the given commit or error out if such a tag doesn't exist. Now, in current git.git:

   $ git describe --contains master
   fatal: cannot describe 'ff86faf2fa02bc21933c9e1dcc75c8d81b3e104a'
   $ git describe --contains --always master
   ff86faf2fa

Or the default behavior without any options: it should find a tag reachable from the given commit or error out, but what if we pass in a commit before the first tag? It recommends '--always':

   $ git describe e83c516
   fatal: No tags can describe 'e83c5163316f89bfbde7d9ab23ca2e25604af290'.
   Try --always, or create some tags.
   $ git describe --always e83c516
   e83c516331
> My knee-jerk reaction is that the most sensible way forward is to
> make --exact-match and --always mutually incompatible.

That would be wrong, it's perfectly valid to ask for an exactly matching tag or, if there is no such tag, the abbreviated object name as a fallback.

Junio C Hamano· Aug 21, 2015, 15:55 UTC · re: SZEDER Gábor · lore

Re: [PATCH] describe: make '--always' fallback work after '--exact-match' failed

SZEDER Gábor <szeder@ira.uka.de> writes:
Show 7 quoted lines
> Quoting Junio C Hamano <gitster@pobox.com>:
>
>> Well, that can be argued both ways.
> ...
>
> 'git describe' errors out by default if it can't describe the given
> commit.

Yes. "describe" always fails when it cannot produce an acceptable description.

And "--always" and "--exact-match" are two incompatible ways for users to change what is considered an acceptable description.

 - With "--exact-match" and "--always", the definition of
   "acceptable" changes: by default, only a name based on some
   anchor point is acceptable.
   - "--always" loosens the definition and also allows an
     abbreviated commit object name as acceptable.
   - "--exact-match" tightens the definition.  In addition to "only
     a name based on some anchor point is acceptable", it requires
     that "based on" to be "0 distance from the anchor point".  The
     help text says succinctly: "*ONLY* output exact matches".

If you allow a request with "--exact-match" to show something that is not an exact match, that no longer is "--exact-match". Allowing to mix "--always" to that option breaks it, as what the command does no longer is to "only outputs exact matches".

 - One option "--exact-match" says that it is wrong to show anything
   but exact matches.
 - The other option "--always" says it is willing to show a name
   that is not an exact match.

These are competing goals. You give preference to the latter, but that is not the only valid point of view.

And that is why I said it can be argued both ways. I think these two are fundamentally incompatible.

[Footnote]
*1* Other parts of "'describe' only considers a name based on some
    anchor point as acceptable." are also modified by various
    options:
    - With "--contains", the definition of "based on" changes
      direction: a commit may be described as somewhere ahead of an
      anchor point by default, but can be described as somewhere behind
      of an anchor point.
    - With "--all" and "--tags", the definition of "anchor point"
      changes: by default, only annotated tags are possible anchor
      points.
SZEDER Gábor· Aug 21, 2015, 14:50 UTC · re: SZEDER Gábor · lore

[PATCH] describe --contains: default to HEAD when no commit-ish is given

'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

Unlike other 'git describe' options, the '--contains' code path is implemented by calling 'name-rev' with a bunch of options plus all the commit-ishes that were passed to 'git describe'. If no commit-ish was present, then 'name-rev' got invoked with none, which then leads to the behavior illustrated above.

Porcelain commands usually default to HEAD when no commit-ish is given, and 'git describe' already does so in all other cases, so it should do so with '--contains' as well.

Pass HEAD to 'name-rev' when no commit-ish is given on the command line to make '--contains' behave consistently with other 'git describe' options.

'git describe's short help already indicates that the commit-ish is optional, but the synopsis in the man page doesn't, so update it accordingly as well.

Signed-off-by: SZEDER Gábor <szeder@ira.uka.de>
---
 Documentation/git-describe.txt |  4 ++--
 builtin/describe.c             | 11 +++++++----
 t/t6120-describe.sh            |  8 ++++++++
 3 files changed, 17 insertions(+), 6 deletions(-)
Show changes to 3 files +17 −6

Documentation/git-describe.txt, builtin/describe.c, t/t6120-describe.sh

diff --git a/Documentation/git-describe.txt b/Documentation/git-describe.txt
index e045fc73f8..c8f28c8c86 100644
--- a/Documentation/git-describe.txt
+++ b/Documentation/git-describe.txt
@@ -9,7 +9,7 @@ git-describe - Describe a commit using the most recent tag reachable from it
 SYNOPSIS
 --------
 [verse]
-'git describe' [--all] [--tags] [--contains] [--abbrev=<n>] <commit-ish>...
+'git describe' [--all] [--tags] [--contains] [--abbrev=<n>] [<commit-ish>...]
 'git describe' [--all] [--tags] [--contains] [--abbrev=<n>] --dirty[=<mark>]
 
 DESCRIPTION
@@ -27,7 +27,7 @@ see the -a and -s options to linkgit:git-tag[1].
 OPTIONS
 -------
 <commit-ish>...::
-	Commit-ish object names to describe.
+	Commit-ish object names to describe.  Defaults to HEAD if omitted.
 
 --dirty[=<mark>]::
 	Describe the working tree.
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)
+			while (*argv) {
+				argv_array_push(&args, *argv);
+				argv++;
+			}
+		else
+			argv_array_push(&args, "HEAD");
 		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' '

-- 
2.5.0.416.g84b07b4
Junio C Hamano· Aug 21, 2015, 16:30 UTC · re: SZEDER Gábor · lore

Re: [PATCH] describe --contains: default to HEAD when no commit-ish is given

SZEDER Gábor <szeder@ira.uka.de> writes:
Show 6 quoted lines
> '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.
Show 13 quoted lines
> 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.

Show 6 quoted lines
> +			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);
>  	}
Show 19 quoted lines
>  
> 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' '
SZEDER Gábor· Aug 24, 2015, 16:14 UTC · re: Junio C Hamano · lore

Re: [PATCH] describe --contains: default to HEAD when no commit-ish is given

Quoting Junio C Hamano <gitster@pobox.com>:
Show 15 quoted lines
>> @@ -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.

Exactly. parse-opts removes all --options from argv as it processes them, barfs at --unknown-options, so all what remains must be treated as a commit-ish. And if nothing is left, well, then there was none given.

Show 29 quoted lines
>> +			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?

Indeed, I didn't notice argv_array_pushv() being added, log tells me it happened quite recently. I suppose with both branches becoming a one-liner the order of them can remain what it was in the patch, this sparing the negation from 'if (!argc)'.

v2 comes in a minute.
Gábor
SZEDER Gábor· Aug 24, 2015, 16:15 UTC · re: SZEDER Gábor · lore

[PATCH v2] describe --contains: default to HEAD when no commit-ish is given

'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

Unlike other 'git describe' options, the '--contains' code path is implemented by calling 'name-rev' with a bunch of options plus all the commit-ishes that were passed to 'git describe'. If no commit-ish was present, then 'name-rev' got invoked with none, which then leads to the behavior illustrated above.

Porcelain commands usually default to HEAD when no commit-ish is given, and 'git describe' already does so in all other cases, so it should do so with '--contains' as well.

Pass HEAD to 'name-rev' when no commit-ish is given on the command line to make '--contains' behave consistently with other 'git describe' options. While at it, use argv_array_pushv() instead of the loop to pass commit-ishes to 'git name-rev'.

'git describe's short help already indicates that the commit-ish is optional, but the synopsis in the man page doesn't, so update it accordingly as well.

Signed-off-by: SZEDER Gábor <szeder@ira.uka.de>
---
 Documentation/git-describe.txt | 4 ++--
 builtin/describe.c             | 8 ++++----
 t/t6120-describe.sh            | 8 ++++++++
 3 files changed, 14 insertions(+), 6 deletions(-)
Show changes to 3 files +14 −6

Documentation/git-describe.txt, builtin/describe.c, t/t6120-describe.sh

diff --git a/Documentation/git-describe.txt b/Documentation/git-describe.txt
index e045fc73f8..c8f28c8c86 100644
--- a/Documentation/git-describe.txt
+++ b/Documentation/git-describe.txt
@@ -9,7 +9,7 @@ git-describe - Describe a commit using the most recent tag reachable from it
 SYNOPSIS
 --------
 [verse]
-'git describe' [--all] [--tags] [--contains] [--abbrev=<n>] <commit-ish>...
+'git describe' [--all] [--tags] [--contains] [--abbrev=<n>] [<commit-ish>...]
 'git describe' [--all] [--tags] [--contains] [--abbrev=<n>] --dirty[=<mark>]
 
 DESCRIPTION
@@ -27,7 +27,7 @@ see the -a and -s options to linkgit:git-tag[1].
 OPTIONS
 -------
 <commit-ish>...::
-	Commit-ish object names to describe.
+	Commit-ish object names to describe.  Defaults to HEAD if omitted.
 
 --dirty[=<mark>]::
 	Describe the working tree.
diff --git a/builtin/describe.c b/builtin/describe.c
index ce36032b1f..9dadfb58c8 100644
--- a/builtin/describe.c
+++ b/builtin/describe.c
@@ -443,10 +443,10 @@ 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)
+			argv_array_pushv(&args, argv);
+		else
+			argv_array_push(&args, "HEAD");
 		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' '

-- 
2.5.0.418.gdd37a9b
Junio C Hamano· Aug 24, 2015, 18:47 UTC · re: SZEDER Gábor · lore

Re: [PATCH] describe --contains: default to HEAD when no commit-ish is given

SZEDER Gábor <szeder@ira.uka.de> writes:
Show 27 quoted lines
>> 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?
>
> Indeed, I didn't notice argv_array_pushv() being added, log tells me
> it happened quite recently.  I suppose with both branches becoming a
> one-liner the order of them can remain what it was in the patch,
> this sparing the negation from 'if (!argc)'.

Another reason to favor the way the code is illustrated for educational purposes above is it is easier to see exceptional case first, i.e. "if we have nothing then we do this special thing, but otherwise we do the normal thing".

But that is a much weaker preference than the preference to "fatter else"; I could go either way.

> v2 comes in a minute.
Thanks.

← back to recent threads