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

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

[SZEDER Gábor]

'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(-)

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

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

[Junio C Hamano]

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.

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

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

[SZEDER Gábor]

Quoting Junio C Hamano <gitster@pobox.com>:

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

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

[Junio C Hamano]

SZEDER Gábor <szeder@ira.uka.de> writes:

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

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

[SZEDER Gábor]

'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(-)

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

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

[Junio C Hamano]

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

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

[SZEDER Gábor]

Quoting Junio C Hamano <gitster@pobox.com>:

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

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

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

[SZEDER Gábor]

'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(-)

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

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

[Junio C Hamano]

SZEDER Gábor <szeder@ira.uka.de> writes:

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