Re: [PATCH 2/2] describe: Exclude --all --match=PATTERN
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
From 66f985b2510c870e62e313732de4b6709894b074 Mon Sep 17 00:00:00 2001
From: Greg Price pr...@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
Greg Price pr...@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. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] describe: Exclude --all --match=PATTERN
On Sun, Mar 03, 2013 at 01:15:21PM -0800, Junio C Hamano wrote: Greg Price pr...@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 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] describe: Exclude --all --match=PATTERN
Junio C Hamano gits...@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;
}
--
To
Re: [PATCH 2/2] describe: Exclude --all --match=PATTERN
Greg Price pr...@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 pr...@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;
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 2/2] describe: Exclude --all --match=PATTERN
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 pr...@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
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
