[PATCH 2/2] describe: Exclude --all --match=PATTERN

[PATCH 2/2] describe: Exclude --all --match=PATTERN

[Greg Price]

Currently when --all is passed, the effect of --match is only
to demote non-matching tags to be treated like non-tags.  This
is puzzling behavior and not consistent with the documentation,
especially with the suggested usage of avoiding information leaks.
The combination of --all and --match is an oxymoron anyway, so
just forbid it.

Signed-off-by: Greg Price <price@mit.edu>
---
This should be applied after the preceding patch; I mistakenly omitted
the '1/2' in its subject line.

 Documentation/git-describe.txt | 3 ++-
 builtin/describe.c             | 3 +++
 2 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/Documentation/git-describe.txt b/Documentation/git-describe.txt
index 711040d..fd5d8f2 100644
--- a/Documentation/git-describe.txt
+++ b/Documentation/git-describe.txt
@@ -83,7 +83,8 @@ OPTIONS
 --match <pattern>::
 	Only consider tags matching the given `glob(7)` pattern,
 	excluding the "refs/tags/" prefix.  This can be used to avoid
-	leaking private tags from the repository.
+	leaking private tags from the repository.  This option is
+	incompatible with `--all`.
 
 --always::
 	Show uniquely abbreviated commit object as fallback.
diff --git a/builtin/describe.c b/builtin/describe.c
index 04c185b..90a72af 100644
--- a/builtin/describe.c
+++ b/builtin/describe.c
@@ -435,6 +435,9 @@ int cmd_describe(int argc, const char **argv, const char *prefix)
 	if (longformat && abbrev == 0)
 		die(_("--long is incompatible with --abbrev=0"));
 
+	if (pattern && all)
+		die(_("--match is incompatible with --all"));
+
 	if (contains) {
 		const char **args = xmalloc((7 + argc) * sizeof(char *));
 		int i = 0;
-- 
1.7.11.3

Re: [PATCH 2/2] describe: Exclude --all --match=PATTERN

[Junio C Hamano]

Greg Price <price@MIT.EDU> writes:

> Currently when --all is passed, the effect of --match is only
> to demote non-matching tags to be treated like non-tags.  This
> is puzzling behavior and not consistent with the documentation,
> especially with the suggested usage of avoiding information leaks.
> The combination of --all and --match is an oxymoron anyway, so
> just forbid it.

I am not sure if this is (1) "behaviour is sometimes useful in
narrow cases but is not explained well", (2) "behaviour does not
make sense in any situation", or (3) "the combination can make sense
if corrected, but the current behaviour is buggy".  If it is (2) or
(3), I think it makes sense to forbid the combination. Also, if it
is (3), we should later come up with an improved behaviour and then
re-enable the combination.

Without "--all" the command considers only the annotated tags to
base the descripion on, and with "--all", a ref that is not
annotated tags can be used as a base, but with a lower priority (if
an annotated tag can describe a given commit, that tag is used).

So naïvely I would expect "--all" and "--match" to base the
description on refs that match the pattern without limiting the
choice of base to annotated tags, and refs that do not match the
given pattern should not appear even as the last resort.  It appears
to me that the current situation is (3).

Will queue and cook in 'next'; thanks.

>
> Signed-off-by: Greg Price <price@mit.edu>
> ---
> This should be applied after the preceding patch; I mistakenly omitted
> the '1/2' in its subject line.
>
>  Documentation/git-describe.txt | 3 ++-
>  builtin/describe.c             | 3 +++
>  2 files changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/git-describe.txt b/Documentation/git-describe.txt
> index 711040d..fd5d8f2 100644
> --- a/Documentation/git-describe.txt
> +++ b/Documentation/git-describe.txt
> @@ -83,7 +83,8 @@ OPTIONS
>  --match <pattern>::
>  	Only consider tags matching the given `glob(7)` pattern,
>  	excluding the "refs/tags/" prefix.  This can be used to avoid
> -	leaking private tags from the repository.
> +	leaking private tags from the repository.  This option is
> +	incompatible with `--all`.
>  
>  --always::
>  	Show uniquely abbreviated commit object as fallback.
> diff --git a/builtin/describe.c b/builtin/describe.c
> index 04c185b..90a72af 100644
> --- a/builtin/describe.c
> +++ b/builtin/describe.c
> @@ -435,6 +435,9 @@ int cmd_describe(int argc, const char **argv, const char *prefix)
>  	if (longformat && abbrev == 0)
>  		die(_("--long is incompatible with --abbrev=0"));
>  
> +	if (pattern && all)
> +		die(_("--match is incompatible with --all"));
> +
>  	if (contains) {
>  		const char **args = xmalloc((7 + argc) * sizeof(char *));
>  		int i = 0;

Re: [PATCH 2/2] describe: Exclude --all --match=PATTERN

[Junio C Hamano]

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

> I am not sure if this is (1) "behaviour is sometimes useful in
> narrow cases but is not explained well", (2) "behaviour does not
> make sense in any situation", or (3) "the combination can make sense
> if corrected, but the current behaviour is buggy".  If it is (2) or
> (3), I think it makes sense to forbid the combination. Also, if it
> is (3), we should later come up with an improved behaviour and then
> re-enable the combination.
>
> Without "--all" the command considers only the annotated tags to
> base the descripion on, and with "--all", a ref that is not
> annotated tags can be used as a base, but with a lower priority (if
> an annotated tag can describe a given commit, that tag is used).
>
> So naïvely I would expect "--all" and "--match" to base the
> description on refs that match the pattern without limiting the
> choice of base to annotated tags, and refs that do not match the
> given pattern should not appear even as the last resort.  It appears
> to me that the current situation is (3).
>
> Will queue and cook in 'next'; thanks.

A fix to the broken semantics may look like this.  There are a few
points to note:

 * The local variable names "is_tag" and "might_be_tag" were
   inconsistent with the rest of the program, where the global
   variable "tags" is used to mean "the user gave --tags to allow
   lightweight ones to be used".  By that definition of the tag, a
   ref under refs/tags/ *is* a tag, and a ref that peels to a
   different object is an annotated tag.  These two variable names
   have been fixed.

 * The function returns early for a ref outside refs/tags/ when
   "--all" is not given with or without this patch.  At the end of
   the function, it also returned when (!all && !prio), but prio
   becomes zero only when the ref is outside refs/tags/ (or the tag
   does not match the pattern) in the original code.  With this
   patch, we reject refs outside refs/tags/ early when "--all" is
   not given, so the last-minute check before add_to_known_names()
   becomes unnecessary (hence removed).

 * If somebody is crazy enough to have an annotated tag under
   refs/heads/, the code would treat it as an annotated tag and
   assign prio==2 to it, with or without this patch.  We may want to
   tighten this further by checking with is_tag, but this patch does
   not do anything about it; I wanted it to focus on only one bug,
   i.e. interaction between "--all" and "--match=<pattern>".

 * When "--tags" is not given, we still give an unannotated tag to
   add_to_known_names(), only to issue a hint when the given commit
   is not describable with annotated tags but it could be described
   if "--tags" were given.  I think this is optimizing for the wrong
   case, and wasting resources.


 builtin/describe.c | 41 ++++++++++++++++++++---------------------
 1 file changed, 20 insertions(+), 21 deletions(-)

diff --git a/builtin/describe.c b/builtin/describe.c
index 04c185b..b2b740d 100644
--- a/builtin/describe.c
+++ b/builtin/describe.c
@@ -137,40 +137,39 @@ static void add_to_known_names(const char *path,
 
 static int get_name(const char *path, const unsigned char *sha1, int flag, void *cb_data)
 {
-	int might_be_tag = !prefixcmp(path, "refs/tags/");
+	int is_tag = !prefixcmp(path, "refs/tags/");
 	unsigned char peeled[20];
-	int is_tag, prio;
+	int is_annotated, prio;
 
-	if (!all && !might_be_tag)
+	/* Reject anything outside refs/tags/ unless --all */
+	if (!all && !is_tag)
 		return 0;
 
+	/* Accept only tags that match the pattern, if given */
+	if (pattern && (!is_tag || fnmatch(pattern, path + 10, 0)))
+		return 0;
+
+	/* Is it annotated? */
 	if (!peel_ref(path, peeled)) {
-		is_tag = !!hashcmp(sha1, peeled);
+		is_annotated = !!hashcmp(sha1, peeled);
 	} else {
 		hashcpy(peeled, sha1);
-		is_tag = 0;
+		is_annotated = 0;
 	}
 
-	/* If --all, then any refs are used.
-	 * If --tags, then any tags are used.
-	 * Otherwise only annotated tags are used.
+	/*
+	 * By default, we only use annotated tags, but with --tags
+	 * we fall back to lightweight ones (even without --tags,
+	 * we still remember lightweight ones, only to give hints
+	 * in an error message).  --all allows any refs to be used.
 	 */
-	if (might_be_tag) {
-		if (is_tag)
-			prio = 2;
-		else
-			prio = 1;
-
-		if (pattern && fnmatch(pattern, path + 10, 0))
-			prio = 0;
-	}
+	if (is_annotated)
+		prio = 2;
+	else if (is_tag)
+		prio = 1;
 	else
 		prio = 0;
 
-	if (!all) {
-		if (!prio)
-			return 0;
-	}
 	add_to_known_names(all ? path + 5 : path + 10, peeled, prio, sha1);
 	return 0;
 }

Re: [PATCH 2/2] describe: Exclude --all --match=PATTERN

[Greg Price]

[-- Attachment #1: Type: text/plain, Size: 1877 bytes --]

On Wed, Feb 27, 2013 at 12:20:07PM -0800, Junio C Hamano wrote:
> Without "--all" the command considers only the annotated tags to
> base the descripion on, and with "--all", a ref that is not
> annotated tags can be used as a base, but with a lower priority (if
> an annotated tag can describe a given commit, that tag is used).
> 
> So naïvely I would expect "--all" and "--match" to base the
> description on refs that match the pattern without limiting the
> choice of base to annotated tags, and refs that do not match the
> given pattern should not appear even as the last resort.  It appears
> to me that the current situation is (3).

Hmm.  It seems to me that "--all" says two things:

 (a) allow unannotated (rather than only annotated)

 (b) allow refs of any name (rather than only tags)

With "--match", particularly because the pattern always refers only to
tags, (b) is obliterated, and your proposed semantics are (a) plus a
sort of inverse of (b):

 (c) allow only refs matching the pattern

which is what "--match" means alone.  But if what we are going for is
(a) and (c), then we don't need "--all" for (a) -- we can get
precisely that with "--tags".  So these semantics make "--all --match=PAT"
equivalent to "--tags --match=PAT".

Given that, I think the user is better off if we reject "--all
--match" with an error message -- and perhaps the error message should
advise them to use "--tags" instead.  Otherwise we have "--all"
telling us (b) as well as (a), and "--match" countermanding (b) and
going precisely the other direction to (c).  If the user has written
that by hand, then they may be confused, and if the command line was
generated, perhaps called from a script, then I fear a bug in the
script is likely, what with the conflicting expectations expressed by
"--all" and "--match".

Patch below to suggest "--tags" in the error message.

Greg

[-- Attachment #2: 0001-describe-Better-error-message-on-all-match.patch --]
[-- Type: text/x-diff, Size: 1090 bytes --]

>From 66f985b2510c870e62e313732de4b6709894b074 Mon Sep 17 00:00:00 2001
From: Greg Price <price@mit.edu>
Date: Sun, 3 Mar 2013 12:19:57 -0800
Subject: [PATCH] describe: Better error message on --all --match

The reason they conflict is that --all means
 (a) allow unannotated, and
 (b) allow refs of any name (rather than only tags),
while --match contradicts (b) and goes further to
 (c) allow only tags matching pattern.

If what the user wants is (a) and (c), they can use --tags to get (a)
without (b).
---
 builtin/describe.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/builtin/describe.c b/builtin/describe.c
index 2ef3f10..6581c40 100644
--- a/builtin/describe.c
+++ b/builtin/describe.c
@@ -436,7 +436,7 @@ int cmd_describe(int argc, const char **argv, const char *prefix)
 		die(_("--long is incompatible with --abbrev=0"));
 
 	if (pattern && all)
-		die(_("--match is incompatible with --all"));
+		die(_("--all conflicts with --match; do you mean --tags?"));
 
 	if (contains) {
 		const char **args = xmalloc((7 + argc) * sizeof(char *));
-- 
1.7.11.3

Re: [PATCH 2/2] describe: Exclude --all --match=PATTERN

[Junio C Hamano]

Greg Price <price@MIT.EDU> writes:

> On Wed, Feb 27, 2013 at 12:20:07PM -0800, Junio C Hamano wrote:
>> Without "--all" the command considers only the annotated tags to
>> base the descripion on, and with "--all", a ref that is not
>> annotated tags can be used as a base, but with a lower priority (if
>> an annotated tag can describe a given commit, that tag is used).
>> 
>> So naïvely I would expect "--all" and "--match" to base the
>> description on refs that match the pattern without limiting the
>> choice of base to annotated tags, and refs that do not match the
>> given pattern should not appear even as the last resort.  It appears
>> to me that the current situation is (3).
>
> Hmm.  It seems to me that "--all" says two things:
>
>  (a) allow unannotated (rather than only annotated)
>
>  (b) allow refs of any name (rather than only tags)
>
> With "--match", particularly because the pattern always refers only to
> tags, (b) is obliterated, and your proposed semantics are (a) plus a
> sort of inverse of (b):
>
>  (c) allow only refs matching the pattern

I would think it is more like "only (a), without changing the
documented semantics of what '--all' and '--match' are by adding (b)
or (c)".

I do not think in the longer term it is wrong per-se to change the
semantics of "--match" from the documented "Only consider tags
matching the pattern" to "Only consider refs matching the pattern",
and such a change can and should be made as a separate patch
"describe: loosen --match to allow any ref, not just tags" on top of
the patch I sent which was meant to be bugfix-only.

Re: [PATCH 2/2] describe: Exclude --all --match=PATTERN

[Greg Price]

On Sun, Mar 03, 2013 at 01:15:21PM -0800, Junio C Hamano wrote:
> Greg Price <price@MIT.EDU> writes:
> > It seems to me that "--all" says two things:
> >
> >  (a) allow unannotated (rather than only annotated)
> >
> >  (b) allow refs of any name (rather than only tags)
> >
> > With "--match", particularly because the pattern always refers only to
> > tags, (b) is obliterated, and your proposed semantics are (a) plus a
> > sort of inverse of (b):
> >
> >  (c) allow only refs matching the pattern
> 
> I would think it is more like "only (a), without changing the
> documented semantics of what '--all' and '--match' are by adding (b)
> or (c)".

Perhaps I'm confused somehow?  I believe (a) and (b) together are the
documented semantics of what '--all' is.  And I believe (c), which
contradicts (b) and indeed goes in the opposite direction, is the
documented semantics of what '--match' is.

Certainly we could choose to resolve the conflict by saying that
'--match' overrides '--all', so that '--all' plus '--match' means
(a)+(c).  I believe that's what you suggested.

I think it would be preferable to recognize the conflict and let the
user sort out what they actually mean, because if they (or their
script) gave these options together then I think there's a substantial
likelihood that they are confused or their script is buggy.  If they
mean (a)+(c), they can get it more clearly with '--tag --match'.



> I do not think in the longer term it is wrong per-se to change the
> semantics of "--match" from the documented "Only consider tags
> matching the pattern" to "Only consider refs matching the pattern",
> and such a change can and should be made as a separate patch
> "describe: loosen --match to allow any ref, not just tags" on top of
> the patch I sent which was meant to be bugfix-only.

Yeah, that could be useful.  What form of pattern would you suggest
that the new '--match' accept?  The obvious, and unambiguous, form of
pattern is glob patterns on the full ref name, as with 'for-each-ref'.
Those are a little unwieldy for interactive use, but are perfect for a
script.  And probably 'describe --match' itself already makes the most
sense in a scripted context, as for interactive use one can go into
'gitk' or 'git log --oneline --decorate' or the like and find out the
same information plus more detail.

Particularly when handling both tags and other refs, I can also
imagine wanting to specify a disjunction of several patterns.  So we
might want to accept the option several times cumulatively.

I'd worry about changing the semantics of existing 'describe --match'
invocations in people's scripts, etc.  Perhaps we'd give it a new name
like '--match-ref'.  Or we could say something like, if it starts with
"refs/" it's a pattern on the whole refname, else it's a pattern on
just the tag name, and accept that if someone has a ref called
"refs/tags/refs/foo" and was finding it with 'describe --match' then
that will break until they edit the pattern.

Cheers,
Greg