* [PATCH v2 1/3] rev-parse: correctly diagnose revision errors before "--"
2013-12-06 22:05 ` [PATCH v2 0/3] rev-parse and "--" Jeff King
@ 2013-12-06 22:05 ` Jeff King
2013-12-06 23:34 ` Jonathan Nieder
2013-12-06 22:07 ` [PATCH v2 2/3] rev-parse: be more careful with munging arguments Jeff King
` (3 subsequent siblings)
4 siblings, 1 reply; 28+ messages in thread
From: Jeff King @ 2013-12-06 22:05 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Duy Nguyen, Git Mailing List
Rev-parse understands that a "--" may separate revisions and
filenames, and that anything after the "--" is taken as-is.
However, it does not understand that anything before the
token must be a revision (which is the usual rule
implemented by the setup_revisions parser).
Since rev-parse prefers revisions to files when parsing
before the "--", we end up with the correct result (if such
an argument is a revision, we parse it as one, and if it is
not, it is an error either way). However, we misdiagnose
the errors:
$ git rev-parse foobar -- >/dev/null
fatal: ambiguous argument 'foobar': unknown revision or path not in the working tree.
Use '--' to separate paths from revisions, like this:
'git <command> [<revision>...] -- [<file>...]'
$ >foobar
$ git rev-parse foobar -- >/dev/null
fatal: bad flag '--' used after filename
In both cases, we should know that the real error is that
"foobar" is meant to be a revision, but could not be
resolved.
Signed-off-by: Jeff King <peff@peff.net>
---
builtin/rev-parse.c | 10 ++++++++++
t/t1506-rev-parse-diagnosis.sh | 24 ++++++++++++++++++++++++
2 files changed, 34 insertions(+)
diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c
index 1d9ecaf..bb694ab 100644
--- a/builtin/rev-parse.c
+++ b/builtin/rev-parse.c
@@ -488,6 +488,7 @@ N_("git rev-parse --parseopt [options] -- [<args>...]\n"
int cmd_rev_parse(int argc, const char **argv, const char *prefix)
{
int i, as_is = 0, verify = 0, quiet = 0, revs_count = 0, type = 0;
+ int has_dashdash = 0;
int output_prefix = 0;
unsigned char sha1[20];
const char *name = NULL;
@@ -501,6 +502,13 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)
if (argc > 1 && !strcmp("-h", argv[1]))
usage(builtin_rev_parse_usage);
+ for (i = 1; i < argc; i++) {
+ if (!strcmp(argv[i], "--")) {
+ has_dashdash = 1;
+ break;
+ }
+ }
+
prefix = setup_git_directory();
git_config(git_default_config, NULL);
for (i = 1; i < argc; i++) {
@@ -788,6 +796,8 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)
}
if (verify)
die_no_single_rev(quiet);
+ if (has_dashdash)
+ die("bad revision '%s'", arg);
as_is = 1;
if (!show_file(arg, output_prefix))
continue;
diff --git a/t/t1506-rev-parse-diagnosis.sh b/t/t1506-rev-parse-diagnosis.sh
index f950c10..613d9bf 100755
--- a/t/t1506-rev-parse-diagnosis.sh
+++ b/t/t1506-rev-parse-diagnosis.sh
@@ -196,4 +196,28 @@ test_expect_success 'dotdot is not an empty set' '
test_cmp expect actual
'
+test_expect_success 'arg before dashdash must be a revision (missing)' '
+ test_must_fail git rev-parse foobar -- 2>stderr &&
+ test_i18ngrep "bad revision" stderr
+'
+
+test_expect_success 'arg before dashdash must be a revision (file)' '
+ >foobar &&
+ test_must_fail git rev-parse foobar -- 2>stderr &&
+ test_i18ngrep "bad revision" stderr
+'
+
+test_expect_success 'arg before dashdash must be a revision (ambiguous)' '
+ >foobar &&
+ git update-ref refs/heads/foobar HEAD &&
+ {
+ # we do not want to use rev-parse here, because
+ # we are testing it
+ cat .git/refs/heads/foobar &&
+ printf "%s\n" --
+ } >expect &&
+ git rev-parse foobar -- >actual &&
+ test_cmp expect actual
+'
+
test_done
--
1.8.5.524.g6743da6
^ permalink raw reply related [flat|nested] 28+ messages in thread* Re: [PATCH v2 1/3] rev-parse: correctly diagnose revision errors before "--"
2013-12-06 22:05 ` [PATCH v2 1/3] rev-parse: correctly diagnose revision errors before "--" Jeff King
@ 2013-12-06 23:34 ` Jonathan Nieder
0 siblings, 0 replies; 28+ messages in thread
From: Jonathan Nieder @ 2013-12-06 23:34 UTC (permalink / raw)
To: Jeff King; +Cc: Junio C Hamano, Duy Nguyen, Git Mailing List
Jeff King wrote:
> Since rev-parse prefers revisions to files when parsing
> before the "--", we end up with the correct result (if such
> an argument is a revision, we parse it as one, and if it is
> not, it is an error either way). However, we misdiagnose
> the errors:
>
> $ git rev-parse foobar -- >/dev/null
> fatal: ambiguous argument 'foobar': unknown revision or path not in the working tree.
> Use '--' to separate paths from revisions, like this:
> 'git <command> [<revision>...] -- [<file>...]'
>
> $ >foobar
> $ git rev-parse foobar -- >/dev/null
> fatal: bad flag '--' used after filename
>
> In both cases, we should know that the real error is that
> "foobar" is meant to be a revision, but could not be
> resolved.
Neat.
[...]
> --- a/builtin/rev-parse.c
> +++ b/builtin/rev-parse.c
> @@ -488,6 +488,7 @@ N_("git rev-parse --parseopt [options] -- [<args>...]\n"
> int cmd_rev_parse(int argc, const char **argv, const char *prefix)
> {
> int i, as_is = 0, verify = 0, quiet = 0, revs_count = 0, type = 0;
> + int has_dashdash = 0;
> int output_prefix = 0;
> unsigned char sha1[20];
> const char *name = NULL;
> @@ -501,6 +502,13 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)
> if (argc > 1 && !strcmp("-h", argv[1]))
> usage(builtin_rev_parse_usage);
>
> + for (i = 1; i < argc; i++) {
> + if (!strcmp(argv[i], "--")) {
> + has_dashdash = 1;
> + break;
> + }
> + }
> +
> prefix = setup_git_directory();
> git_config(git_default_config, NULL);
> for (i = 1; i < argc; i++) {
> @@ -788,6 +796,8 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)
> }
> if (verify)
> die_no_single_rev(quiet);
> + if (has_dashdash)
> + die("bad revision '%s'", arg);
> as_is = 1;
Yep, this is the "fall back to looking for a file" part of rev-parse,
so erroring out if there as a dashdash coming is the right thing to
do. And a quick code search for "rev-parse.*\ --\ " reveals that
most callers would simply not be affected by this.
Thanks for a pleasant patch.
For what it's worth,
Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>
^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH v2 2/3] rev-parse: be more careful with munging arguments
2013-12-06 22:05 ` [PATCH v2 0/3] rev-parse and "--" Jeff King
2013-12-06 22:05 ` [PATCH v2 1/3] rev-parse: correctly diagnose revision errors before "--" Jeff King
@ 2013-12-06 22:07 ` Jeff King
2013-12-07 0:04 ` Jonathan Nieder
2013-12-09 21:33 ` Eric Sunshine
2013-12-06 22:08 ` [PATCH v2 3/3] rev-parse: diagnose ambiguous revision/filename arguments Jeff King
` (2 subsequent siblings)
4 siblings, 2 replies; 28+ messages in thread
From: Jeff King @ 2013-12-06 22:07 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Duy Nguyen, Git Mailing List
When rev-parse looks at whether an argument like "foo..bar"
or "foobar^@" is a difference or parent-shorthand, it
internally munges the arguments so that it can pass the
individual rev arguments to get_sha1. However, we do not
consistently un-munge the result.
For cases where we do not match (e.g., "doesnotexist..HEAD"),
we wouuld then want to try to treat the argument as a
filename. try_difference gets this right, and always
unmunges in this case. However, try_parent_shorthand never
unmunges, leading to incorrect error messages, or even
incorrect results:
$ git rev-parse foobar^@
foobar
fatal: ambiguous argument 'foobar': unknown revision or path not in the working tree.
Use '--' to separate paths from revisions, like this:
'git <command> [<revision>...] -- [<file>...]'
$ >foobar
$ git rev-parse foobar^@
foobar
For cases where we do match, neither function unmunges. This
does not currently matter, since we are done with the
argument. However, a future patch will do further
processing, and this prepares for it. In addition, it's
simply a confusing interface for some cases to modify the
const argument, and others not to.
Signed-off-by: Jeff King <peff@peff.net>
---
As an aside, this is one of those places where C's string functions do
gross things with const. We modify a const argument to the function
without any casts or warnings, because the return value from strstr
implicitly drops the const. Time for us all to move to C++ with
templated strstr, right?
builtin/rev-parse.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c
index bb694ab..a60fcd3 100644
--- a/builtin/rev-parse.c
+++ b/builtin/rev-parse.c
@@ -286,6 +286,7 @@ static int try_difference(const char *arg)
exclude = n;
}
}
+ *dotdot = '.';
return 1;
}
*dotdot = '.';
@@ -309,8 +310,10 @@ static int try_parent_shorthands(const char *arg)
return 0;
*dotdot = 0;
- if (get_sha1_committish(arg, sha1))
+ if (get_sha1_committish(arg, sha1)) {
+ *dotdot = '^';
return 0;
+ }
if (!parents_only)
show_rev(NORMAL, sha1, arg);
@@ -319,6 +322,7 @@ static int try_parent_shorthands(const char *arg)
show_rev(parents_only ? NORMAL : REVERSED,
parents->item->object.sha1, arg);
+ *dotdot = '^';
return 1;
}
--
1.8.5.524.g6743da6
^ permalink raw reply related [flat|nested] 28+ messages in thread* Re: [PATCH v2 2/3] rev-parse: be more careful with munging arguments
2013-12-06 22:07 ` [PATCH v2 2/3] rev-parse: be more careful with munging arguments Jeff King
@ 2013-12-07 0:04 ` Jonathan Nieder
2013-12-09 21:33 ` Eric Sunshine
1 sibling, 0 replies; 28+ messages in thread
From: Jonathan Nieder @ 2013-12-07 0:04 UTC (permalink / raw)
To: Jeff King; +Cc: Junio C Hamano, Duy Nguyen, Git Mailing List
Jeff King wrote:
> As an aside, this is one of those places where C's string functions do
> gross things with const.
Yes, yuck.
The fundamental grossness is that argv is semantically char **
(assuming this doesn't segfault) but passed around as const char **.
I wonder why we don't use the same trick in git_extract_argv0_path and
when handling STRIP_EXTENSION in git.c.
Alternatively, try_difference could create a new buffer for its own
use. Originally (v0.99~273, 2005-06-13) this dotdot handling code was
pretty simple:
for (i = 1; i < argc; i++) {
char *arg = argv[i];
...
dotdot = strstr(arg, "..");
if (dotdot) {
unsigned char end[20];
char *n = dotdot+2;
*dotdot = 0;
if (!get_sha1(arg, sha1)) {
if (!*n)
n = "HEAD";
if (!get_sha1(n, end)) {
printf("%s\n", sha1_to_hex(end));
printf("^%s\n", sha1_to_hex(sha1));
continue;
}
}
*dotdot = '.';
}
printf("%s\n", arg);
}
The 'continue' without making the argument right again was justified
because we were done with the argument.
Now it is still not much more complicated --- the function is
logically just trying to parse the argument into a "this" and "next"
part and print them.
Something like the following (untested). Or if the allocation churn
is too much, it could use a static strbuf.
diff --git i/builtin/rev-parse.c w/builtin/rev-parse.c
index c76b89d..98262b3 100644
--- i/builtin/rev-parse.c
+++ w/builtin/rev-parse.c
@@ -234,37 +234,35 @@ static int try_difference(const char *arg)
unsigned char sha1[20];
unsigned char end[20];
const char *next;
- const char *this;
+ struct strbuf this = STRBUF_INIT;
int symmetric;
- static const char head_by_default[] = "HEAD";
if (!(dotdot = strstr(arg, "..")))
return 0;
next = dotdot + 2;
- this = arg;
symmetric = (*next == '.');
-
- *dotdot = 0;
next += symmetric;
- if (!*next)
- next = head_by_default;
- if (dotdot == arg)
- this = head_by_default;
-
- if (this == head_by_default && next == head_by_default &&
- !symmetric) {
+ if (!*next && dotdot == arg && !symmetric)
/*
* Just ".."? That is not a range but the
* pathspec for the parent directory.
*/
- *dotdot = '.';
return 0;
+
+ strbuf_add(&this, arg, dotdot - arg);
+
+ if (!*next)
+ next = "HEAD";
+ if (dotdot == arg) {
+ strbuf_reset(&this);
+ strbuf_addstr(&this, "HEAD");
}
- if (!get_sha1_committish(this, sha1) && !get_sha1_committish(next, end)) {
+ if (!get_sha1_committish(this.buf, sha1)
+ && !get_sha1_committish(next, end)) {
show_rev(NORMAL, end, next);
- show_rev(symmetric ? NORMAL : REVERSED, sha1, this);
+ show_rev(symmetric ? NORMAL : REVERSED, sha1, this.buf);
if (symmetric) {
struct commit_list *exclude;
struct commit *a, *b;
@@ -279,9 +277,10 @@ static int try_difference(const char *arg)
exclude = n;
}
}
+ strbuf_release(&this);
return 1;
}
- *dotdot = '.';
+ strbuf_release(&this);
return 0;
}
^ permalink raw reply related [flat|nested] 28+ messages in thread* Re: [PATCH v2 2/3] rev-parse: be more careful with munging arguments
2013-12-06 22:07 ` [PATCH v2 2/3] rev-parse: be more careful with munging arguments Jeff King
2013-12-07 0:04 ` Jonathan Nieder
@ 2013-12-09 21:33 ` Eric Sunshine
1 sibling, 0 replies; 28+ messages in thread
From: Eric Sunshine @ 2013-12-09 21:33 UTC (permalink / raw)
To: Jeff King; +Cc: Junio C Hamano, Duy Nguyen, Git Mailing List
On Fri, Dec 6, 2013 at 5:07 PM, Jeff King <peff@peff.net> wrote:
> When rev-parse looks at whether an argument like "foo..bar"
> or "foobar^@" is a difference or parent-shorthand, it
> internally munges the arguments so that it can pass the
> individual rev arguments to get_sha1. However, we do not
> consistently un-munge the result.
>
> For cases where we do not match (e.g., "doesnotexist..HEAD"),
> we wouuld then want to try to treat the argument as a
s/wouuld/would/
> filename. try_difference gets this right, and always
> unmunges in this case. However, try_parent_shorthand never
> unmunges, leading to incorrect error messages, or even
> incorrect results:
>
> $ git rev-parse foobar^@
> foobar
> fatal: ambiguous argument 'foobar': unknown revision or path not in the working tree.
> Use '--' to separate paths from revisions, like this:
> 'git <command> [<revision>...] -- [<file>...]'
>
> $ >foobar
> $ git rev-parse foobar^@
> foobar
>
> For cases where we do match, neither function unmunges. This
> does not currently matter, since we are done with the
> argument. However, a future patch will do further
> processing, and this prepares for it. In addition, it's
> simply a confusing interface for some cases to modify the
> const argument, and others not to.
>
> Signed-off-by: Jeff King <peff@peff.net>
^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH v2 3/3] rev-parse: diagnose ambiguous revision/filename arguments
2013-12-06 22:05 ` [PATCH v2 0/3] rev-parse and "--" Jeff King
2013-12-06 22:05 ` [PATCH v2 1/3] rev-parse: correctly diagnose revision errors before "--" Jeff King
2013-12-06 22:07 ` [PATCH v2 2/3] rev-parse: be more careful with munging arguments Jeff King
@ 2013-12-06 22:08 ` Jeff King
2013-12-06 23:25 ` [PATCH v2 0/3] rev-parse and "--" Jonathan Nieder
2013-12-09 19:05 ` Junio C Hamano
4 siblings, 0 replies; 28+ messages in thread
From: Jeff King @ 2013-12-06 22:08 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Duy Nguyen, Git Mailing List
If you have both a file and a branch named "foo", running:
git log foo
will complain. We should do the same in rev-parse, and
demand that it be disambiguated with:
git rev-parse foo --
or
git rev-parse -- foo
Signed-off-by: Jeff King <peff@peff.net>
---
builtin/rev-parse.c | 12 ++++++++----
t/t1506-rev-parse-diagnosis.sh | 22 ++++++++++++++++++++++
2 files changed, 30 insertions(+), 4 deletions(-)
diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c
index a60fcd3..a3a58bf 100644
--- a/builtin/rev-parse.c
+++ b/builtin/rev-parse.c
@@ -781,10 +781,11 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)
}
/* Not a flag argument */
- if (try_difference(arg))
- continue;
- if (try_parent_shorthands(arg))
+ if (try_difference(arg) || try_parent_shorthands(arg)) {
+ if (!has_dashdash)
+ verify_non_filename(prefix, arg);
continue;
+ }
name = arg;
type = NORMAL;
if (*arg == '^') {
@@ -794,8 +795,11 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)
if (!get_sha1(name, sha1)) {
if (verify)
revs_count++;
- else
+ else {
show_rev(type, sha1, name);
+ if (!has_dashdash)
+ verify_non_filename(prefix, arg);
+ }
continue;
}
if (verify)
diff --git a/t/t1506-rev-parse-diagnosis.sh b/t/t1506-rev-parse-diagnosis.sh
index 613d9bf..dc1f1dc 100755
--- a/t/t1506-rev-parse-diagnosis.sh
+++ b/t/t1506-rev-parse-diagnosis.sh
@@ -220,4 +220,26 @@ test_expect_success 'arg before dashdash must be a revision (ambiguous)' '
test_cmp expect actual
'
+test_expect_success 'ambiguous arg without dashdash (normal)' '
+ >foobar &&
+ git update-ref refs/heads/foobar HEAD &&
+ test_must_fail git rev-parse foobar 2>stderr &&
+ test_i18ngrep ambiguous stderr
+'
+
+test_expect_success 'ambiguous arg without dashdash (difference)' '
+ >one..two &&
+ git update-ref refs/heads/one HEAD &&
+ git update-ref refs/heads/two HEAD &&
+ test_must_fail git rev-parse one..two 2>stderr &&
+ test_i18ngrep ambiguous stderr
+'
+
+test_expect_success 'ambiguous arg without dashdash (parents)' '
+ >"foobar^@" &&
+ git update-ref refs/heads/foobar HEAD &&
+ test_must_fail git rev-parse foobar^@ 2>stderr &&
+ test_i18ngrep ambiguous stderr
+'
+
test_done
--
1.8.5.524.g6743da6
^ permalink raw reply related [flat|nested] 28+ messages in thread* Re: [PATCH v2 0/3] rev-parse and "--"
2013-12-06 22:05 ` [PATCH v2 0/3] rev-parse and "--" Jeff King
` (2 preceding siblings ...)
2013-12-06 22:08 ` [PATCH v2 3/3] rev-parse: diagnose ambiguous revision/filename arguments Jeff King
@ 2013-12-06 23:25 ` Jonathan Nieder
2013-12-06 23:30 ` Jeff King
2013-12-09 19:05 ` Junio C Hamano
4 siblings, 1 reply; 28+ messages in thread
From: Jonathan Nieder @ 2013-12-06 23:25 UTC (permalink / raw)
To: Jeff King; +Cc: Junio C Hamano, Duy Nguyen, Git Mailing List
Jeff King wrote:
> Patch 3 is the revised version of this patch which notices ambiguity.
> However, I'm having second thoughts on it. I think it's the right thing
> to do if you want to help people build something like "git log"
> themselves. But it does mean that we are breaking somebody who does:
>
> echo foo >HEAD
> commit=$(git rev-parse HEAD)
>
> I'm tempted to say that people who did that are stupid and wrong (and
> ugly, too). They should probably be using "--verify" in this case. But
> it has been that way for a long time, and there are two instances in our
> test scripts that are broken by the patch.
Wouldn't the same thing happen for $(git rev-parse HEAD^..HEAD) when
there is a 'HEAD^..HEAD' file, too? --verify doesn't support that.
I think it's a nice idea to provide this functionality for people who
need it, which would mean protected by an option (--check-ambiguity or
something). I don't think it's a good change to do it
unconditionally.
My two cents,
Jonathan
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: [PATCH v2 0/3] rev-parse and "--"
2013-12-06 23:25 ` [PATCH v2 0/3] rev-parse and "--" Jonathan Nieder
@ 2013-12-06 23:30 ` Jeff King
0 siblings, 0 replies; 28+ messages in thread
From: Jeff King @ 2013-12-06 23:30 UTC (permalink / raw)
To: Jonathan Nieder; +Cc: Junio C Hamano, Duy Nguyen, Git Mailing List
On Fri, Dec 06, 2013 at 03:25:56PM -0800, Jonathan Nieder wrote:
> > commit=$(git rev-parse HEAD)
> >
> > I'm tempted to say that people who did that are stupid and wrong (and
> > ugly, too). They should probably be using "--verify" in this case. But
> > it has been that way for a long time, and there are two instances in our
> > test scripts that are broken by the patch.
>
> Wouldn't the same thing happen for $(git rev-parse HEAD^..HEAD) when
> there is a 'HEAD^..HEAD' file, too? --verify doesn't support that.
True, but they can use "rev-parse HEAD^..HEAD --", which is probably a
better alternative to --verify.
> I think it's a nice idea to provide this functionality for people who
> need it, which would mean protected by an option (--check-ambiguity or
> something). I don't think it's a good change to do it
> unconditionally.
I don't think it's worth making it conditional. Literally nobody has
asked for it. I was mostly trying to harmonize "rev-parse" and the rest
of the revision parser. If that's not worth doing, I'd just as soon drop
it (and people using "--" properly already get the right behavior).
-Peff
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v2 0/3] rev-parse and "--"
2013-12-06 22:05 ` [PATCH v2 0/3] rev-parse and "--" Jeff King
` (3 preceding siblings ...)
2013-12-06 23:25 ` [PATCH v2 0/3] rev-parse and "--" Jonathan Nieder
@ 2013-12-09 19:05 ` Junio C Hamano
2013-12-09 19:12 ` Jonathan Nieder
4 siblings, 1 reply; 28+ messages in thread
From: Junio C Hamano @ 2013-12-09 19:05 UTC (permalink / raw)
To: Jeff King; +Cc: Duy Nguyen, Git Mailing List
Jeff King <peff@peff.net> writes:
> Is it better for "rev-parse" to be more careful, and to behave more like
> the rest of git? Or is better to be historically compatible?
>
> One thing to note is that "git rev-parse HEAD" is slightly broken there
> already. Because "git rev-parse $some_branch" may do very different
> things than what the caller expects if $some_branch does not exist (but
> there is a file with the same name). So maybe we are doing a favor by
> calling out the problem; if they want a rev, they should be using
> "--verify" (or "--").
I tend to agree with the reasoning in the last sentence. Let's cook
it for a while and see what happens.
Thanks.
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: [PATCH v2 0/3] rev-parse and "--"
2013-12-09 19:05 ` Junio C Hamano
@ 2013-12-09 19:12 ` Jonathan Nieder
2013-12-09 19:23 ` Jonathan Nieder
2013-12-09 20:48 ` Junio C Hamano
0 siblings, 2 replies; 28+ messages in thread
From: Jonathan Nieder @ 2013-12-09 19:12 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jeff King, Duy Nguyen, Git Mailing List
Junio C Hamano wrote:
>> So maybe we are doing a favor by
>> calling out the problem; if they want a rev, they should be using
>> "--verify" (or "--").
>
> I tend to agree with the reasoning in the last sentence. Let's cook
> it for a while and see what happens.
Isn't this essentially breaking a contract that would have been relied
on by any script that used "git rev-parse HEAD~3..HEAD"? Worse, it's
breaking that contract in a way that no one would notice until they
are asked to manipulate a worktree with a file named 'HEAD~3..HEAD'
--- in other words, the breakage it introduces is painfully subtle.
I agree that "git rev-parse HEAD" is better written as "git rev-parse
--verify HEAD" and hence not so much worth worrying about, but I don't
find it easy to believe that people should have anticipated this
change and added a trailing "--" to more complex rev-parse
expressions.
So to be clear, I think unless it is protected by a new option, this
is a bad idea.
Hope that helps,
Jonathan
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v2 0/3] rev-parse and "--"
2013-12-09 19:12 ` Jonathan Nieder
@ 2013-12-09 19:23 ` Jonathan Nieder
2013-12-09 20:48 ` Junio C Hamano
1 sibling, 0 replies; 28+ messages in thread
From: Jonathan Nieder @ 2013-12-09 19:23 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jeff King, Duy Nguyen, Git Mailing List
Jonathan Nieder wrote:
> Isn't this essentially breaking a contract
To clarify, if there were some strong motivation --- e.g. if this
were resolving some security problem --- then I would be all for
breaking compatibility in this way. What's confusing to me is that I
just don't see the motivation. Why wouldn't someone that wants this
functionality be able to pass a flag for it?
Is the idea that without a flag it's easier to teach or something?
Hoping that clarifies,
Jonathan
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v2 0/3] rev-parse and "--"
2013-12-09 19:12 ` Jonathan Nieder
2013-12-09 19:23 ` Jonathan Nieder
@ 2013-12-09 20:48 ` Junio C Hamano
2013-12-09 20:56 ` Jonathan Nieder
1 sibling, 1 reply; 28+ messages in thread
From: Junio C Hamano @ 2013-12-09 20:48 UTC (permalink / raw)
To: Jonathan Nieder; +Cc: Jeff King, Duy Nguyen, Git Mailing List
Jonathan Nieder <jrnieder@gmail.com> writes:
> Junio C Hamano wrote:
>
>>> So maybe we are doing a favor by
>>> calling out the problem; if they want a rev, they should be using
>>> "--verify" (or "--").
>>
>> I tend to agree with the reasoning in the last sentence. Let's cook
>> it for a while and see what happens.
>
> Isn't this essentially breaking a contract that would have been relied
> on by any script that used "git rev-parse HEAD~3..HEAD"? Worse, it's
> breaking that contract in a way that no one would notice until they
> are asked to manipulate a worktree with a file named 'HEAD~3..HEAD'
> --- in other words, the breakage it introduces is painfully subtle.
>
> I agree that "git rev-parse HEAD" is better written as "git rev-parse
> --verify HEAD" and hence not so much worth worrying about,
I do not share the "with --verify is better hence no problem"
reasoning. My "not so much worth worrying about" comes solely from
a hunch that nobody has "HEAD~3..HEAD" in their working directory,
and if somebody has one, then they must be using "--verify" (or a
clarifying "--"), because their "git log" and whatever they use "git
rev-parse HEAD~3..HEAD" for would behave very differently otherwise.
So it is not merely "--verify is better"---in a situation where the
backward incompatibility matters, I doubt the existing behaves
sensibly in the first place.
But if we cook it for a while, I suspect that we will find more and
more breakages of expectations in the existing scripts in and out of
the tree; in general, I hate _any_ change, and I do not mind
dropping it ;-).
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v2 0/3] rev-parse and "--"
2013-12-09 20:48 ` Junio C Hamano
@ 2013-12-09 20:56 ` Jonathan Nieder
2013-12-09 21:10 ` Junio C Hamano
0 siblings, 1 reply; 28+ messages in thread
From: Jonathan Nieder @ 2013-12-09 20:56 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jeff King, Duy Nguyen, Git Mailing List
Junio C Hamano wrote:
> I do not share the "with --verify is better hence no problem"
> reasoning. My "not so much worth worrying about" comes solely from
> a hunch that nobody has "HEAD~3..HEAD" in their working directory,
That's what makes it a problem. This change makes it very easy to
make a general-purpose script that breaks in an edge case that the
script's author is not likely to run into. Then as soon as someone
adds a file with such a name to the test data in their repo, their
favorite general-purpose repo munger just breaks.
If we wanted to forbid git from tracking files named HEAD~3..HEAD
altogether, that would be a different story.
> and if somebody has one, then they must be using "--verify" (or a
> clarifying "--"), because their "git log" and whatever they use "git
> rev-parse HEAD~3..HEAD" for would behave very differently otherwise.
Isn't protecting against this kind of thing the reason we ask authors
of general-purpose scripts to use "simple, do what I say and not what
I mean" plumbing commands?
Another relevant detail is that using rev-parse with "--" is more
painful than without, since it includes the "--" in its output.
Without this change, it seems much more likely to me that someone
would do
git rev-parse <commits> |
while read commit
do
...
done
than
git rev-parse <commits> -- |
while read commit
do
if test "$commit" = "--"
then
continue
fi
...
done
> So it is not merely "--verify is better"---in a situation where the
> backward incompatibility matters, I doubt the existing behaves
> sensibly in the first place.
What in the former of the above two loops is broken?
> But if we cook it for a while, I suspect that we will find more and
> more breakages of expectations in the existing scripts in and out of
> the tree;
Alas, probably no, because nobody has "HEAD~3..HEAD" in their working
directory. That's exactly the problem --- it creates an edge case
that nobody is likely to test until the once-in-a-few-years moment
when they need it.
Jonathan
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v2 0/3] rev-parse and "--"
2013-12-09 20:56 ` Jonathan Nieder
@ 2013-12-09 21:10 ` Junio C Hamano
0 siblings, 0 replies; 28+ messages in thread
From: Junio C Hamano @ 2013-12-09 21:10 UTC (permalink / raw)
To: Jonathan Nieder; +Cc: Jeff King, Duy Nguyen, Git Mailing List
Jonathan Nieder <jrnieder@gmail.com> writes:
>> But if we cook it for a while, I suspect that we will find more and
>> more breakages of expectations in the existing scripts in and out of
>> the tree;
>
> Alas, probably no, because nobody has "HEAD~3..HEAD" in their working
> directory. That's exactly the problem --- it creates an edge case
> that nobody is likely to test...
I didn't mean to say that running and encoutering the issue is the
only way to find "more breakges of expectations", by the way. I am
still on the fence, but leaning towards not merging it too hastily.
^ permalink raw reply [flat|nested] 28+ messages in thread