Usability improvement request: git show revision -- file

Usability improvement request: git show revision -- file

[Piotr Krukowiecki]

Hi,

if there's existing way to do this please tell me.

There's this file "src/subdir/file". I'm in the "src" directory and want to
see the "file" at specific revision.

Knowing about git show I'd expect something like this to work:

   $ git show master -- subdir/file

But it shows nothing (no output, no warning). Following also does not
work as expected:

   $ git show master:subdir/file
   fatal: Path 'src/subdir/file' exists, but not 'subdir/file'.
   Did you mean 'master:src/subdir/file'?

Of course following works:

   $ git show master:src/subdir/file

but it's not very convenient to have to specify full path, and it's not what
you would expect given that most other commands accept "-- relativepath"
syntax.


-- 
Piotr Krukowiecki

Re: Usability improvement request: git show revision -- file

[Michael J Gruber]

Piotr Krukowiecki venit, vidit, dixit 31.03.2011 08:45:
> Hi,
> 
> if there's existing way to do this please tell me.
> 
> There's this file "src/subdir/file". I'm in the "src" directory and want to
> see the "file" at specific revision.
> 
> Knowing about git show I'd expect something like this to work:
> 
>    $ git show master -- subdir/file
> 
> But it shows nothing (no output, no warning).

...because you are asking git to show the commit master, filtered by
subdir/file, and if that file is not changed there, the commit is not
selected. If it is changed there, it shows you only the diff affecting
that file. (We might want to change this to filter the diff only.)

> Following also does not
> work as expected:
> 
>    $ git show master:subdir/file
>    fatal: Path 'src/subdir/file' exists, but not 'subdir/file'.
>    Did you mean 'master:src/subdir/file'?

But git is really understanding about your situation, isn't it? ;)

> 
> Of course following works:
> 
>    $ git show master:src/subdir/file
> 
> but it's not very convenient to have to specify full path, and it's not what
> you would expect given that most other commands accept "-- relativepath"
> syntax.

It's not the command in this case, but the "commit:pathspec" syntax, and
for every command which understands it, it is relative to root (i.e.
expects a full path). But we do have a syntax for relative:

git show master:./subdir/file

Cheers,
Michael

[PATCH 1/3] t1506: factor out test for "Did you mean..."

[Michael J Gruber]

With the current code, it's a "'"'"'" jungle, and we test only 1 line of
the 2 line response. Factor out and test both.

Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
---
 t/t1506-rev-parse-diagnosis.sh |   17 ++++++++++++-----
 1 files changed, 12 insertions(+), 5 deletions(-)

diff --git a/t/t1506-rev-parse-diagnosis.sh b/t/t1506-rev-parse-diagnosis.sh
index 9f8adb1..f9cb202 100755
--- a/t/t1506-rev-parse-diagnosis.sh
+++ b/t/t1506-rev-parse-diagnosis.sh
@@ -6,6 +6,13 @@ exec </dev/null
 
 . ./test-lib.sh
 
+test_did_you_mean ()
+{
+	printf "fatal: Path '$2$3' $4, but not ${5:-'$3'}.\n" >expected &&
+	printf "Did you mean '$1:$2$3'?\n" >>expected &&
+	test_cmp expected error
+}
+
 HASH_file=
 
 test_expect_success 'set up basic repo' '
@@ -106,7 +113,7 @@ test_expect_success 'incorrect file in sha1:path' '
 	grep "fatal: Path '"'"'index-only.txt'"'"' exists on disk, but not in '"'"'HEAD'"'"'." error &&
 	(cd subdir &&
 	 test_must_fail git rev-parse HEAD:file2.txt 2> error &&
-	 grep "Did you mean '"'"'HEAD:subdir/file2.txt'"'"'?" error )
+	 test_did_you_mean HEAD subdir/ file2.txt exists )
 '
 
 test_expect_success 'incorrect file in :path and :N:path' '
@@ -115,14 +122,14 @@ test_expect_success 'incorrect file in :path and :N:path' '
 	test_must_fail git rev-parse :1:nothing.txt 2> error &&
 	grep "Path '"'"'nothing.txt'"'"' does not exist (neither on disk nor in the index)." error &&
 	test_must_fail git rev-parse :1:file.txt 2> error &&
-	grep "Did you mean '"'"':0:file.txt'"'"'?" error &&
+	test_did_you_mean ":0" "" file.txt "is in the index" "at stage 1" &&
 	(cd subdir &&
 	 test_must_fail git rev-parse :1:file.txt 2> error &&
-	 grep "Did you mean '"'"':0:file.txt'"'"'?" error &&
+	 test_did_you_mean ":0" "" file.txt "is in the index" "at stage 1" &&
 	 test_must_fail git rev-parse :file2.txt 2> error &&
-	 grep "Did you mean '"'"':0:subdir/file2.txt'"'"'?" error &&
+	 test_did_you_mean ":0" subdir/ file2.txt "is in the index" &&
 	 test_must_fail git rev-parse :2:file2.txt 2> error &&
-	 grep "Did you mean '"'"':0:subdir/file2.txt'"'"'?" error) &&
+	 test_did_you_mean :0 subdir/ file2.txt "is in the index") &&
 	test_must_fail git rev-parse :disk-only.txt 2> error &&
 	grep "fatal: Path '"'"'disk-only.txt'"'"' exists on disk, but not in the index." error
 '
-- 
1.7.4.2.668.gba03a4

[PATCH 2/3] sha1_name: Suggest commit:./file for path in subdir

[Michael J Gruber]

Currently, the "Did you mean..." message suggests "commit:fullpath"
only. Extend this to show the more convenient "commit:./file" form also.

Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
---
 sha1_name.c                    |   11 +++++++----
 t/t1506-rev-parse-diagnosis.sh |    2 +-
 2 files changed, 8 insertions(+), 5 deletions(-)

diff --git a/sha1_name.c b/sha1_name.c
index faea58d..69cd6c8 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -1012,11 +1012,13 @@ static void diagnose_invalid_sha1_path(const char *prefix,
 		if (!get_tree_entry(tree_sha1, fullname,
 				    sha1, &mode)) {
 			die("Path '%s' exists, but not '%s'.\n"
-			    "Did you mean '%s:%s'?",
+			    "Did you mean '%s:%s' aka '%s:./%s'?",
 			    fullname,
 			    filename,
 			    object_name,
-			    fullname);
+			    fullname,
+			    object_name,
+			    filename);
 		}
 		die("Path '%s' does not exist in '%s'",
 		    filename, object_name);
@@ -1065,9 +1067,10 @@ static void diagnose_invalid_index_path(int stage,
 		if (ce_namelen(ce) == fullnamelen &&
 		    !memcmp(ce->name, fullname, fullnamelen))
 			die("Path '%s' is in the index, but not '%s'.\n"
-			    "Did you mean ':%d:%s'?",
+			    "Did you mean ':%d:%s' aka ':%d:./%s'?",
 			    fullname, filename,
-			    ce_stage(ce), fullname);
+			    ce_stage(ce), fullname,
+			    ce_stage(ce), filename);
 	}
 
 	if (!lstat(filename, &st))
diff --git a/t/t1506-rev-parse-diagnosis.sh b/t/t1506-rev-parse-diagnosis.sh
index f9cb202..4a6396f 100755
--- a/t/t1506-rev-parse-diagnosis.sh
+++ b/t/t1506-rev-parse-diagnosis.sh
@@ -9,7 +9,7 @@ exec </dev/null
 test_did_you_mean ()
 {
 	printf "fatal: Path '$2$3' $4, but not ${5:-'$3'}.\n" >expected &&
-	printf "Did you mean '$1:$2$3'?\n" >>expected &&
+	printf "Did you mean '$1:$2$3'${2:+ aka '$1:./$3'}?\n" >>expected &&
 	test_cmp expected error
 }
 
-- 
1.7.4.2.668.gba03a4

[RFC/PATCH 3/3] builtin/show.c: do not prune by pathspec

[Michael J Gruber]

By design, "git show commit -- path" is not "git show commit:path", and
there is no reason to change that. But "git show commit -- path" simply
returns nothing at all "most of the time" because it prunes by pathspec
even though it does not walk commits. This is pretty useless.

So, turn off pruning (but keep diff limiting of course) so that "git
show commit -- path" shows the commit message and the diff that the
commit introduces to path (filtered by path); only the diff will be
empty "most of the time".

As an intended side effect, users mistaking "git show commit -- path"
for "git show commit:path" are automatically reminded that they asked
git to show a commit, not a blob.

Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
---
 builtin/log.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/builtin/log.c b/builtin/log.c
index 9db43ed..ea0ddb4 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -411,6 +411,7 @@ int cmd_show(int argc, const char **argv, const char *prefix)
 	opt.def = "HEAD";
 	opt.tweak = show_rev_tweak_rev;
 	cmd_log_init(argc, argv, prefix, &rev, &opt);
+	rev.prune = 0;
 
 	count = rev.pending.nr;
 	objects = rev.pending.objects;
-- 
1.7.4.2.668.gba03a4

Re: [RFC/PATCH 3/3] builtin/show.c: do not prune by pathspec

[Johannes Sixt]

Am 3/31/2011 11:17, schrieb Michael J Gruber:
> By design, "git show commit -- path" is not "git show commit:path", and
> there is no reason to change that. But "git show commit -- path" simply
> returns nothing at all "most of the time" because it prunes by pathspec
> even though it does not walk commits. This is pretty useless.
> 
> So, turn off pruning (but keep diff limiting of course) so that "git
> show commit -- path" shows the commit message and the diff that the
> commit introduces to path (filtered by path); only the diff will be
> empty "most of the time".

How does this interfere with git show --walk commit -- path? Will it now
show all commits instead of just those that changed path?

-- Hannes

Re: [RFC/PATCH 3/3] builtin/show.c: do not prune by pathspec

[Michael J Gruber]

Johannes Sixt venit, vidit, dixit 31.03.2011 12:18:
> Am 3/31/2011 11:17, schrieb Michael J Gruber:
>> By design, "git show commit -- path" is not "git show commit:path", and
>> there is no reason to change that. But "git show commit -- path" simply
>> returns nothing at all "most of the time" because it prunes by pathspec
>> even though it does not walk commits. This is pretty useless.
>>
>> So, turn off pruning (but keep diff limiting of course) so that "git
>> show commit -- path" shows the commit message and the diff that the
>> commit introduces to path (filtered by path); only the diff will be
>> empty "most of the time".
> 
> How does this interfere with git show --walk commit -- path? Will it now
> show all commits instead of just those that changed path?

Hmpft,

git show --walk origin/master
fatal: unrecognized argument: --walk

No, that is without my patch ;)

In other words: "ENOPARSEOPTS in revision.c", there is no "--walk".

Michael

Re: [RFC/PATCH 3/3] builtin/show.c: do not prune by pathspec

[Johannes Sixt]

Am 3/31/2011 12:58, schrieb Michael J Gruber:
> Johannes Sixt venit, vidit, dixit 31.03.2011 12:18:
>> Am 3/31/2011 11:17, schrieb Michael J Gruber:
>>> By design, "git show commit -- path" is not "git show commit:path", and
>>> there is no reason to change that. But "git show commit -- path" simply
>>> returns nothing at all "most of the time" because it prunes by pathspec
>>> even though it does not walk commits. This is pretty useless.
>>>
>>> So, turn off pruning (but keep diff limiting of course) so that "git
>>> show commit -- path" shows the commit message and the diff that the
>>> commit introduces to path (filtered by path); only the diff will be
>>> empty "most of the time".
>>
>> How does this interfere with git show --walk commit -- path? Will it now
>> show all commits instead of just those that changed path?
> 
> Hmpft,
> 
> git show --walk origin/master
> fatal: unrecognized argument: --walk
> 
> No, that is without my patch ;)
> 
> In other words: "ENOPARSEOPTS in revision.c", there is no "--walk".

8-( Oops sorry, and thanks for doing research that I should have done. At
first, I thought it's named --walk because we have --no-walk. But the name
is --do-walk.

My question is then:

How does this interfere with git show --do-walk commit -- path?

-- Hannes

Re: [RFC/PATCH 3/3] builtin/show.c: do not prune by pathspec

[Michael J Gruber]

Johannes Sixt venit, vidit, dixit 31.03.2011 13:42:
> Am 3/31/2011 12:58, schrieb Michael J Gruber:
>> Johannes Sixt venit, vidit, dixit 31.03.2011 12:18:
>>> Am 3/31/2011 11:17, schrieb Michael J Gruber:
>>>> By design, "git show commit -- path" is not "git show commit:path", and
>>>> there is no reason to change that. But "git show commit -- path" simply
>>>> returns nothing at all "most of the time" because it prunes by pathspec
>>>> even though it does not walk commits. This is pretty useless.
>>>>
>>>> So, turn off pruning (but keep diff limiting of course) so that "git
>>>> show commit -- path" shows the commit message and the diff that the
>>>> commit introduces to path (filtered by path); only the diff will be
>>>> empty "most of the time".
>>>
>>> How does this interfere with git show --walk commit -- path? Will it now
>>> show all commits instead of just those that changed path?
>>
>> Hmpft,
>>
>> git show --walk origin/master
>> fatal: unrecognized argument: --walk
>>
>> No, that is without my patch ;)
>>
>> In other words: "ENOPARSEOPTS in revision.c", there is no "--walk".
> 
> 8-( Oops sorry, and thanks for doing research that I should have done. At
> first, I thought it's named --walk because we have --no-walk. But the name
> is --do-walk.
> 
> My question is then:
> 
> How does this interfere with git show --do-walk commit -- path?

Uh, didn't know that one. If I had thought we have a consistent
interface before...

So, yes, it changes that case also. Note that nothing in git-show[1]
hints at the fact that one can use rev-list arguments, especially that
one could walk the commits, so it is undocumented behavior which would
change.

I would even say that quite generally it is difficult to find cases
where prune and no_walk make sense together. One may construe a user who
feeds a list of commits to "rev-list --no-walk" and uses it as a filter,
returning only those commits which touch a given pathspec. But, I'm not
suggesting to change "rev-list --no-walk -- pathspec" or log.

But I do think that "show" would benefit from a better default. An
alternative would be to imply --sparse (i.e. dense=0).

BTW: If you look at cmd_log_reflog() you see that already for other
log-like commands we have the problem that we have to override the
log-centric defaults from cmd_log_init(), which means ignoring certain
user specified rev-list and diff-core options!

Michael

Re: [RFC/PATCH 3/3] builtin/show.c: do not prune by pathspec

[Nguyen Thai Ngoc Duy]

I suggest add "commit" to the subject: do not prune commit by
pathspec. Diff limiting is another kind of pruning and I was confused.

On Thu, Mar 31, 2011 at 4:17 PM, Michael J Gruber
<git@drmicha.warpmail.net> wrote:
> By design, "git show commit -- path" is not "git show commit:path", and
> there is no reason to change that.

Even more true, now that "path" can be "*.sh".

> But "git show commit -- path" simply
> returns nothing at all "most of the time" because it prunes by pathspec
> even though it does not walk commits. This is pretty useless.
>
> So, turn off pruning (but keep diff limiting of course) so that "git
> show commit -- path" shows the commit message and the diff that the
> commit introduces to path (filtered by path); only the diff will be
> empty "most of the time".

Tests please?

> As an intended side effect, users mistaking "git show commit -- path"
> for "git show commit:path" are automatically reminded that they asked
> git to show a commit, not a blob.

Nor a tree. I don't really see how "git show commit:path" and "git how
commit -- path" are relevant for it to be mentioned here.
-- 
Duy

Re: [RFC/PATCH 3/3] builtin/show.c: do not prune by pathspec

[Michael J Gruber]

Nguyen Thai Ngoc Duy venit, vidit, dixit 31.03.2011 14:50:
> I suggest add "commit" to the subject: do not prune commit by
> pathspec. Diff limiting is another kind of pruning and I was confused.

I know that under the name "diff limiting" only (that may be my ignorance).

> 
> On Thu, Mar 31, 2011 at 4:17 PM, Michael J Gruber
> <git@drmicha.warpmail.net> wrote:
>> By design, "git show commit -- path" is not "git show commit:path", and
>> there is no reason to change that.
> 
> Even more true, now that "path" can be "*.sh".
> 
>> But "git show commit -- path" simply
>> returns nothing at all "most of the time" because it prunes by pathspec
>> even though it does not walk commits. This is pretty useless.
>>
>> So, turn off pruning (but keep diff limiting of course) so that "git
>> show commit -- path" shows the commit message and the diff that the
>> commit introduces to path (filtered by path); only the diff will be
>> empty "most of the time".
> 
> Tests please?

Heck, we don't have any to begin with, and this is marked RFC. Given our
usual reluctance to change even undocumented behavior I'm not going to
bother with tests for an RFC.

>> As an intended side effect, users mistaking "git show commit -- path"
>> for "git show commit:path" are automatically reminded that they asked
>> git to show a commit, not a blob.
> 
> Nor a tree. I don't really see how "git show commit:path" and "git how
> commit -- path" are relevant for it to be mentioned here.

"git show commit:path" is relevant because that is what the OP was
trying to do, and "git show commit:path" is relevant because that is
what the OP tried and confused him because there was no output at all.
Not to mention that this is the command this patch is about.

Someone needs a coffee, me thinks ;)

Michael

Re: [RFC/PATCH 3/3] builtin/show.c: do not prune by pathspec

[Nguyen Thai Ngoc Duy]

On Thu, Mar 31, 2011 at 8:26 PM, Michael J Gruber
<git@drmicha.warpmail.net> wrote:
>> Tests please?
>
> Heck, we don't have any to begin with, and this is marked RFC. Given our
> usual reluctance to change even undocumented behavior I'm not going to
> bother with tests for an RFC.

Ugh.. I missed the "RFC" part on the subject. Sorry.

>>> As an intended side effect, users mistaking "git show commit -- path"
>>> for "git show commit:path" are automatically reminded that they asked
>>> git to show a commit, not a blob.
>>
>> Nor a tree. I don't really see how "git show commit:path" and "git how
>> commit -- path" are relevant for it to be mentioned here.
>
> "git show commit:path" is relevant because that is what the OP was
> trying to do, and "git show commit:path" is relevant because that is
> what the OP tried and confused him because there was no output at all.
> Not to mention that this is the command this patch is about.

Thanks. I see I also missed the original thread somewhere. Someone
needs a sleep, me thinks :)
-- 
Duy

Re: [RFC/PATCH 3/3] builtin/show.c: do not prune by pathspec

[Michael J Gruber]

Nguyen Thai Ngoc Duy venit, vidit, dixit 31.03.2011 15:35:
> On Thu, Mar 31, 2011 at 8:26 PM, Michael J Gruber
> <git@drmicha.warpmail.net> wrote:
>>> Tests please?
>>
>> Heck, we don't have any to begin with, and this is marked RFC. Given our
>> usual reluctance to change even undocumented behavior I'm not going to
>> bother with tests for an RFC.
> 
> Ugh.. I missed the "RFC" part on the subject. Sorry.
> 
>>>> As an intended side effect, users mistaking "git show commit -- path"
>>>> for "git show commit:path" are automatically reminded that they asked
>>>> git to show a commit, not a blob.
>>>
>>> Nor a tree. I don't really see how "git show commit:path" and "git how
>>> commit -- path" are relevant for it to be mentioned here.
>>
>> "git show commit:path" is relevant because that is what the OP was
>> trying to do, and "git show commit:path" is relevant because that is
>> what the OP tried and confused him because there was no output at all.
>> Not to mention that this is the command this patch is about.
> 
> Thanks. I see I also missed the original thread somewhere. Someone
> needs a sleep, me thinks :)

I'm trying with the coffee that I had suggested (seems to be working),
but I guess that decision (between coffee and sleep) depends on the
timezone also :)

Michael

Re: [RFC/PATCH 3/3] builtin/show.c: do not prune by pathspec

[Junio C Hamano]

Michael J Gruber <git@drmicha.warpmail.net> writes:

>> Tests please?
>
> Heck, we don't have any to begin with, and this is marked RFC. Given our
> usual reluctance to change even undocumented behavior I'm not going to
> bother with tests for an RFC.

Quite the contrary, a well written test is a concise and readable way to
illustrate what behaviour the proposed change is making, and helps judging
if it is going in a good direction.  So if it is an RFC, a test would help
very much, especially if there isn't any in the area currently.

Re: [PATCH 2/3] sha1_name: Suggest commit:./file for path in subdir

[Junio C Hamano]

Michael J Gruber <git@drmicha.warpmail.net> writes:

> Currently, the "Did you mean..." message suggests "commit:fullpath"
> only. Extend this to show the more convenient "commit:./file" form also.

If we were to do this, I suspect that with non-empty prefix we should only
show "./$file" form for brevity without aka.  This is a end-user facing
message and not meant to help scripts, no?

Re: Usability improvement request: git show revision -- file

[Piotr Krukowiecki]

On Thu, Mar 31, 2011 at 9:50 AM, Michael J Gruber
<git@drmicha.warpmail.net> wrote:
> Piotr Krukowiecki venit, vidit, dixit 31.03.2011 08:45:
>> Hi,
>>
>> if there's existing way to do this please tell me.
>>
>> There's this file "src/subdir/file". I'm in the "src" directory and want to
>> see the "file" at specific revision.
>>
>> Knowing about git show I'd expect something like this to work:
>>
>>    $ git show master -- subdir/file
>>
>> But it shows nothing (no output, no warning).
>
> ...because you are asking git to show the commit master, filtered by
> subdir/file, and if that file is not changed there, the commit is not

It wasn't - I was doing git-blame and wanted to see file as it was before
the blamed commit.


> selected. If it is changed there, it shows you only the diff affecting

I see - that makes sense too.


> that file. (We might want to change this to filter the diff only.)

Not sure what you mean by that?


>> Following also does not
>> work as expected:
>>
>>    $ git show master:subdir/file
>>    fatal: Path 'src/subdir/file' exists, but not 'subdir/file'.
>>    Did you mean 'master:src/subdir/file'?
>
> But git is really understanding about your situation, isn't it? ;)

Yeah, but this looks like a workround for a common problem -
people specify relative path a lot, so a warning was added.


>> Of course following works:
>>
>>    $ git show master:src/subdir/file
>>
>> but it's not very convenient to have to specify full path, and it's not what
>> you would expect given that most other commands accept "-- relativepath"
>> syntax.
>
> It's not the command in this case, but the "commit:pathspec" syntax, and
> for every command which understands it, it is relative to root (i.e.
> expects a full path). But we do have a syntax for relative:
>
> git show master:./subdir/file

Thanks, I'll try to remember. I don't have a better idea for solving
this problem.


-- 
Piotr Krukowiecki

Re: [RFC/PATCH 3/3] builtin/show.c: do not prune by pathspec

[Michael J Gruber]

Junio C Hamano venit, vidit, dixit 31.03.2011 21:23:
> Michael J Gruber <git@drmicha.warpmail.net> writes:
> 
>>> Tests please?
>>
>> Heck, we don't have any to begin with, and this is marked RFC. Given our
>> usual reluctance to change even undocumented behavior I'm not going to
>> bother with tests for an RFC.
> 
> Quite the contrary, a well written test is a concise and readable way to
> illustrate what behaviour the proposed change is making, and helps judging
> if it is going in a good direction.  So if it is an RFC, a test would help
> very much, especially if there isn't any in the area currently.

While that may be true in some cases (e.g., providing sample output) I
don't think the commit message to 3/3 leaves anything open that a test
could clarify.

Michael

Re: [PATCH 2/3] sha1_name: Suggest commit:./file for path in subdir

[Michael J Gruber]

Junio C Hamano venit, vidit, dixit 31.03.2011 21:26:
> Michael J Gruber <git@drmicha.warpmail.net> writes:
> 
>> Currently, the "Did you mean..." message suggests "commit:fullpath"
>> only. Extend this to show the more convenient "commit:./file" form also.
> 
> If we were to do this, I suspect that with non-empty prefix we should only
> show "./$file" form for brevity without aka.  This is a end-user facing
> message and not meant to help scripts, no?

ENOPARSE

Do you mean:

- replace the old "commit:fullpath" with "commit:./file" or

- show the new form only without "aka" (but with the old form) or

- show literal "commit:./$file"?

I guess you meant the first one. But I left in both forms on purpose:
Saying only "commit:./file" does not explain what it means, the "./"
part is easy to miss, and the user may not even be aware to be in a
subdir. Listing both does not take much space and explains everything.

Michael

[PATCH 0/4] reflog, show and command line overrides

[Michael J Gruber]

While thinking about how to redo 3/3 (show: do not prune by pathspec) I
noticed a somehow related reflog problem, which overrides some command line
options.

So, here is some refactoring, a test exposing the reflog problem, and a fix for
reflog (the new 1/4 through 3/4). Those 3 should be general good cleanup.

It turned out that the refactoring does not help with the show problem, but I
changed the old 3/3 so that we change the pruning by commits only when the user
has not requested to walk with show (the new 4/4). No time for new test now, sorry.

The old 1/3 and 2/3 ("Did you mean...") are not impacted (and not resent). They
make for independent good UI cleanup also (and were related thematically only,
not technically).

Michael J Gruber (4):
  builtin/log.c: separate default and setup of cmd_log_init()
  t/t1411: test reflog with formats
  reflog: fix overriding of command line options
  builtin/show: do not prune by pathspec

 builtin/log.c          |   32 +++++++++++++++++++-------------
 t/t1411-reflog-show.sh |   18 ++++++++++++++++++
 2 files changed, 37 insertions(+), 13 deletions(-)

-- 
1.7.4.2.668.gba03a4

[PATCH 1/4] builtin/log.c: separate default and setup of cmd_log_init()

[Michael J Gruber]

cmd_log_init() sets up some default rev options and then calls
setup_revisions(), so that a caller cannot set up own defaults: Either
they get overriden by cmd_log_init() (if set before) or they override
the command line (if set after). We even complain about this in a
comment to cmd_log_reflog().

Therefore, separate the two steps so that one can still call
cmd_log_init() or, alternatively, cmd_log_init_defaults() followed by
cmd_log_init_finish() (and set defaults in between).

No functional change so far.

Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
---
 builtin/log.c |   21 +++++++++++++++------
 1 files changed, 15 insertions(+), 6 deletions(-)

diff --git a/builtin/log.c b/builtin/log.c
index 9db43ed..f585209 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -49,13 +49,8 @@ static int parse_decoration_style(const char *var, const char *value)
 	return -1;
 }
 
-static void cmd_log_init(int argc, const char **argv, const char *prefix,
-			 struct rev_info *rev, struct setup_revision_opt *opt)
+static void cmd_log_init_defaults(struct rev_info *rev)
 {
-	int i;
-	int decoration_given = 0;
-	struct userformat_want w;
-
 	rev->abbrev = DEFAULT_ABBREV;
 	rev->commit_format = CMIT_FMT_DEFAULT;
 	if (fmt_pretty)
@@ -68,7 +63,14 @@ static void cmd_log_init(int argc, const char **argv, const char *prefix,
 
 	if (default_date_mode)
 		rev->date_mode = parse_date_format(default_date_mode);
+}
 
+static void cmd_log_init_finish(int argc, const char **argv, const char *prefix,
+			 struct rev_info *rev, struct setup_revision_opt *opt)
+{
+	int i;
+	int decoration_given = 0;
+	struct userformat_want w;
 	/*
 	 * Check for -h before setup_revisions(), or "git log -h" will
 	 * fail when run without a git directory.
@@ -128,6 +130,13 @@ static void cmd_log_init(int argc, const char **argv, const char *prefix,
 	setup_pager();
 }
 
+static void cmd_log_init(int argc, const char **argv, const char *prefix,
+			 struct rev_info *rev, struct setup_revision_opt *opt)
+{
+	cmd_log_init_defaults(rev);
+	cmd_log_init_finish(argc, argv, prefix, rev, opt);
+}
+
 /*
  * This gives a rough estimate for how many commits we
  * will print out in the list.
-- 
1.7.4.2.668.gba03a4

[PATCH 2/4] t/t1411: test reflog with formats

[Michael J Gruber]

"git reflog --format=short" does not work because "reflog" overrides the
format option. This is documented in code. Document this by a test
(known failure) also.

Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
---
 t/t1411-reflog-show.sh |   18 ++++++++++++++++++
 1 files changed, 18 insertions(+), 0 deletions(-)

diff --git a/t/t1411-reflog-show.sh b/t/t1411-reflog-show.sh
index ba25ff3..88dc6a7 100755
--- a/t/t1411-reflog-show.sh
+++ b/t/t1411-reflog-show.sh
@@ -28,6 +28,24 @@ test_expect_success 'oneline reflog format' '
 	test_cmp expect actual
 '
 
+test_expect_success 'reflog default format' '
+	git reflog -1 >actual &&
+	test_cmp expect actual
+'
+
+cat >expect <<'EOF'
+commit e46513e
+Reflog: HEAD@{0} (C O Mitter <committer@example.com>)
+Reflog message: commit (initial): one
+Author: A U Thor <author@example.com>
+
+    one
+EOF
+test_expect_failure 'override reflog default format' '
+	git reflog --format=short -1 >actual &&
+	test_cmp expect actual
+'
+
 cat >expect <<'EOF'
 Reflog: HEAD@{Thu Apr 7 15:13:13 2005 -0700} (C O Mitter <committer@example.com>)
 Reflog message: commit (initial): one
-- 
1.7.4.2.668.gba03a4

[PATCH 3/4] reflog: fix overriding of command line options

[Michael J Gruber]

Currently, "git reflog" overrides some command line options such as
"--format".

Fix this by using the new 2-phase version of cmd_log_init().

Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
---
 builtin/log.c          |    9 ++-------
 t/t1411-reflog-show.sh |    2 +-
 2 files changed, 3 insertions(+), 8 deletions(-)

diff --git a/builtin/log.c b/builtin/log.c
index f585209..916019c 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -495,16 +495,11 @@ int cmd_log_reflog(int argc, const char **argv, const char *prefix)
 	rev.verbose_header = 1;
 	memset(&opt, 0, sizeof(opt));
 	opt.def = "HEAD";
-	cmd_log_init(argc, argv, prefix, &rev, &opt);
-
-	/*
-	 * This means that we override whatever commit format the user gave
-	 * on the cmd line.  Sad, but cmd_log_init() currently doesn't
-	 * allow us to set a different default.
-	 */
+	cmd_log_init_defaults(&rev);
 	rev.commit_format = CMIT_FMT_ONELINE;
 	rev.use_terminator = 1;
 	rev.always_show_header = 1;
+	cmd_log_init_finish(argc, argv, prefix, &rev, &opt);
 
 	return cmd_log_walk(&rev);
 }
diff --git a/t/t1411-reflog-show.sh b/t/t1411-reflog-show.sh
index 88dc6a7..caa687b 100755
--- a/t/t1411-reflog-show.sh
+++ b/t/t1411-reflog-show.sh
@@ -41,7 +41,7 @@ Author: A U Thor <author@example.com>
 
     one
 EOF
-test_expect_failure 'override reflog default format' '
+test_expect_success 'override reflog default format' '
 	git reflog --format=short -1 >actual &&
 	test_cmp expect actual
 '
-- 
1.7.4.2.668.gba03a4

[PATCH 4/4] builtin/show: do not prune by pathspec

[Michael J Gruber]

By design, "git show commit -- path" is not "git show commit:path", and
there is no reason to change that. But "git show commit -- path" simply
returns nothing at all "most of the time" because it prunes by pathspec
even though it does not walk commits. This is pretty useless.

So, turn off commit pruning (but keep diff limiting of course) so that
"git show commit -- path" shows the commit message and the diff that the
commit introduces to path (filtered by path); only the diff will be
empty "most of the time".

As an intended side effect, users mistaking "git show commit -- path"
for "git show commit:path" are automatically reminded that they asked
git to show a commit, not a blob.

In case the user has specified "--do-walk", assume they want the old
behaviour (prune by default).

Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
---
 builtin/log.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/builtin/log.c b/builtin/log.c
index 916019c..474a76d 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -420,6 +420,8 @@ int cmd_show(int argc, const char **argv, const char *prefix)
 	opt.def = "HEAD";
 	opt.tweak = show_rev_tweak_rev;
 	cmd_log_init(argc, argv, prefix, &rev, &opt);
+	if (rev.no_walk)
+		rev.prune = 0;
 
 	count = rev.pending.nr;
 	objects = rev.pending.objects;
-- 
1.7.4.2.668.gba03a4

Re: [PATCH 2/3] sha1_name: Suggest commit:./file for path in subdir

[Junio C Hamano]

Michael J Gruber <git@drmicha.warpmail.net> writes:

> Junio C Hamano venit, vidit, dixit 31.03.2011 21:26:
>> Michael J Gruber <git@drmicha.warpmail.net> writes:
>> 
>>> Currently, the "Did you mean..." message suggests "commit:fullpath"
>>> only. Extend this to show the more convenient "commit:./file" form also.
>> 
>> If we were to do this, I suspect that with non-empty prefix we should only
>> show "./$file" form for brevity without aka.  This is a end-user facing
>> message and not meant to help scripts, no?
>
> ENOPARSE

Sorry.  What I meant was to show only "$commit:./$file" (with $commit and
$file substituted appropriately so that the user can cut&paste from the
output) when we issue this advice in a subdirectory.

This is meant to be a direct and immediate advice to the user who typed it
from the command line to tell what he might meant.  It is beneficial to
teach the user that there are two different ways to spell it, and that in
different contexts one of these two ways may be easier to use over the
other way, but this is not a place to do so.

We know the context and what the user wanted to do (iow, the command is
run in a subdirectory, and we already guessed that the user probably
wanted to name the path in that subdirectory), so we already know between
two forms, ./$path_around_here form, not the $prefix/$path_around_here
form, is more appropriate, no?

Re: [PATCH 4/4] builtin/show: do not prune by pathspec

[Junio C Hamano]

Michael J Gruber <git@drmicha.warpmail.net> writes:

> By design, "git show commit -- path" is not "git show commit:path", and
> there is no reason to change that. But "git show commit -- path" simply
> returns nothing at all "most of the time" because it prunes by pathspec
> even though it does not walk commits. This is pretty useless.

Hmm, I'm very tempted to suggest throwing it into the "don't do it then"
basket.

> As an intended side effect, users mistaking "git show commit -- path"
> for "git show commit:path" are automatically reminded that they asked
> git to show a commit, not a blob.
>
> In case the user has specified "--do-walk", assume they want the old
> behaviour (prune by default).

Compared to "--do-walk", "git show HEAD~5.." would be a much more common
way to trigger it, and is a more appropriate justification why you made
the non-pruning conditional.

> Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
> ---
>  builtin/log.c |    2 ++
>  1 files changed, 2 insertions(+), 0 deletions(-)
>
> diff --git a/builtin/log.c b/builtin/log.c
> index 916019c..474a76d 100644
> --- a/builtin/log.c
> +++ b/builtin/log.c
> @@ -420,6 +420,8 @@ int cmd_show(int argc, const char **argv, const char *prefix)
>  	opt.def = "HEAD";
>  	opt.tweak = show_rev_tweak_rev;
>  	cmd_log_init(argc, argv, prefix, &rev, &opt);
> +	if (rev.no_walk)
> +		rev.prune = 0;
>  
>  	count = rev.pending.nr;
>  	objects = rev.pending.objects;

Re: [PATCH 4/4] builtin/show: do not prune by pathspec

[Junio C Hamano]

I attempted to rewrite the log message in a bit more objective voice like
this:

    builtin/show: do not prune by pathspec
    
    "git show $commit -- $path" does not show anything for a commit that does
    not change the $path.  While this may be technically correct, it is somewhat
    unexpected from the end user's point of view.
    
    Unless "show" is used as "log -p", e.g. "git show HEAD~5..", it makes more
    sense to show at least the log message for commits, even they are
    uninteresting with respect to $path.
    
    Turn off commit pruning (but keep diff limiting of course) so that the
    command shows the log message and the diff that the commit introduces to
    the path.  The diff part may be empty for a given commit that does not
    touch the path.
    
    As an intended side effect, users mistaking "git show commit -- path"
    for "git show commit:path" are automatically reminded that they asked
    git to show a commit, not a blob.
    
    Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
    Signed-off-by: Junio C Hamano <gitster@pobox.com>

which made me realize that this change does regress for no-walk case.

What if you (or your homegrown tool or alias) are feeding a list of
candidate commits that may have touched the path, without walking, and are
expecting them to be filtered?

    $ git show A B C D -- path

We used to get a nice output of "git show C -- path" in such a case but
now the output will be cluttered with the log message from a commit that
is totally uninteresting with respect to the given path.

I really wanted to like this patch, because I _very much_ liked the
"intended" ;-) side effect.

I am torn.

Re: [PATCH 4/4] builtin/show: do not prune by pathspec

[Michael J Gruber]

Junio C Hamano venit, vidit, dixit 02.04.2011 00:59:
> I attempted to rewrite the log message in a bit more objective voice like
> this:
> 
>     builtin/show: do not prune by pathspec
>     
>     "git show $commit -- $path" does not show anything for a commit that does
>     not change the $path.  While this may be technically correct, it is somewhat
>     unexpected from the end user's point of view.
>     
>     Unless "show" is used as "log -p", e.g. "git show HEAD~5..", it makes more
>     sense to show at least the log message for commits, even they are
>     uninteresting with respect to $path.

Definitely more diplomatic ;)

>     Turn off commit pruning (but keep diff limiting of course) so that the
>     command shows the log message and the diff that the commit introduces to
>     the path.  The diff part may be empty for a given commit that does not
>     touch the path.
>     
>     As an intended side effect, users mistaking "git show commit -- path"
>     for "git show commit:path" are automatically reminded that they asked
>     git to show a commit, not a blob.
>     
>     Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
>     Signed-off-by: Junio C Hamano <gitster@pobox.com>
> 
> which made me realize that this change does regress for no-walk case.

Well, it's an intended change of behaviour, so the term "regress" is
diplomatically problematic...

> 
> What if you (or your homegrown tool or alias) are feeding a list of
> candidate commits that may have touched the path, without walking, and are
> expecting them to be filtered?
> 
>     $ git show A B C D -- path
> 
> We used to get a nice output of "git show C -- path" in such a case but
> now the output will be cluttered with the log message from a commit that
> is totally uninteresting with respect to the given path.
> 
> I really wanted to like this patch, because I _very much_ liked the
> "intended" ;-) side effect.
> 
> I am torn.
> 

I would probably discard the "tool" aspect (for a porcelain). FWIW, we
never even documented that "git show" takes pathspec arguments, and only
experienced users know that "git show" has anything to do with "git log"
and friends (we mention only the relation to diff-tree).

That being said, we could detect in addition whether there is only one
positive rev (and no walk being done) and turn off pruning only in that
case. That would still catch users' (mis)expectation about "show rev --
file" but not impact anyone using it as a filter on more than 1 rev.
(Personally, I find the distinction between walk and no-walk modes clearer.)

Michael

Re: [PATCH 4/4] builtin/show: do not prune by pathspec

[Junio C Hamano]

Michael J Gruber <git@drmicha.warpmail.net> writes:

> diff --git a/builtin/log.c b/builtin/log.c
> index 916019c..474a76d 100644
> --- a/builtin/log.c
> +++ b/builtin/log.c
> @@ -420,6 +420,8 @@ int cmd_show(int argc, const char **argv, const char *prefix)
>  	opt.def = "HEAD";
>  	opt.tweak = show_rev_tweak_rev;
>  	cmd_log_init(argc, argv, prefix, &rev, &opt);
> +	if (rev.no_walk)
> +		rev.prune = 0;

This is not your fault, but I am somewhat disgusted by the reason why

    $ git show master..next [ -- Documentation ]

works by "walking" the history.  It takes completely different codepath
from "git log" with the same set of arguments.

 * first log_init() grabs '^master' and 'next' into the rev.pending object
   array;

 * we pop '^master' first, make it the only object in the rev.pending
   object array, and let cmd_log_walk() call prepare_revision_walk() on it
   to limit the list.  Since we don't have any positive ref, we get
   nothing;

 * then we pop 'next', make it again _the only_ object in the rev.pending
   object array; prepare_revision_walk() on the same codepath now limits
   the list to exclude what is reachable from 'master', only because we
   have processed '^master'.

Yikes.  In other words, the reason the current code works is only by
accident.

This is a tangent, but we _might_ want to (perhaps at git 2.0) update the
revision parsing machinery so that we can clear the object flags more
easily and have

    $ git log A..B C..D [ -- pathspec ]

compute the union of (commits reachable from B but not from A) and
(commits reachable from D but not from C).  I say we _might_ because we
would still want to compute what the current code computes in some cases,
and we may be able to express it as "^A ^C B D", but I am not sure if we
want to go that route and end up with a general set operation (with unions
and intersections, perhaps even using parentheses to express precedence).

More general set operation is certainly doable, and at that point it
probably does not matter that we cannot stream the output as we are doing
something complex (IOW, we would be revs->limited, giving up the latency),
but I don't know if it is useful in the first place to support such
general case.  Because the most complex set operations that I run every
day is

    $ git log ^master $topic1 $topic2 $topic3... -- $pathspec

and I don't recall cases in which I wished A..B C..D showed commits
within A..B that are reachable from C.

Re: [PATCH 4/4] builtin/show: do not prune by pathspec

[Michael J Gruber]

Junio C Hamano venit, vidit, dixit 04.04.2011 23:49:
> Michael J Gruber <git@drmicha.warpmail.net> writes:
> 
>> diff --git a/builtin/log.c b/builtin/log.c
>> index 916019c..474a76d 100644
>> --- a/builtin/log.c
>> +++ b/builtin/log.c
>> @@ -420,6 +420,8 @@ int cmd_show(int argc, const char **argv, const char *prefix)
>>  	opt.def = "HEAD";
>>  	opt.tweak = show_rev_tweak_rev;
>>  	cmd_log_init(argc, argv, prefix, &rev, &opt);
>> +	if (rev.no_walk)
>> +		rev.prune = 0;
> 
> This is not your fault, but I am somewhat disgusted by the reason why
> 
>     $ git show master..next [ -- Documentation ]
> 
> works by "walking" the history.  It takes completely different codepath
> from "git log" with the same set of arguments.
> 
>  * first log_init() grabs '^master' and 'next' into the rev.pending object
>    array;
> 
>  * we pop '^master' first, make it the only object in the rev.pending
>    object array, and let cmd_log_walk() call prepare_revision_walk() on it
>    to limit the list.  Since we don't have any positive ref, we get
>    nothing;
> 
>  * then we pop 'next', make it again _the only_ object in the rev.pending
>    object array; prepare_revision_walk() on the same codepath now limits
>    the list to exclude what is reachable from 'master', only because we
>    have processed '^master'.
> 
> Yikes.  In other words, the reason the current code works is only by
> accident.

That is super ugly, yes.

> 
> This is a tangent, but we _might_ want to (perhaps at git 2.0) update the
> revision parsing machinery so that we can clear the object flags more
> easily and have
> 
>     $ git log A..B C..D [ -- pathspec ]
> 
> compute the union of (commits reachable from B but not from A) and
> (commits reachable from D but not from C).  I say we _might_ because we
> would still want to compute what the current code computes in some cases,
> and we may be able to express it as "^A ^C B D", but I am not sure if we
> want to go that route and end up with a general set operation (with unions
> and intersections, perhaps even using parentheses to express precedence).
> 
> More general set operation is certainly doable, and at that point it
> probably does not matter that we cannot stream the output as we are doing
> something complex (IOW, we would be revs->limited, giving up the latency),
> but I don't know if it is useful in the first place to support such
> general case.  Because the most complex set operations that I run every
> day is
> 
>     $ git log ^master $topic1 $topic2 $topic3... -- $pathspec
> 
> and I don't recall cases in which I wished A..B C..D showed commits
> within A..B that are reachable from C.
> 

We seem to promise that one should think in terms of sets, and this
breaks with "A..B" and "C...D" unless one always thinks of them in the
resolved form "^A B" and "^E C D" (with E=$(git merge-base C D)).

I also think that "A...B C" treats C as the right of that symmetric
range (because anything non-left we treat as right), and "A...B ^C"
probably interacts badly with --cherry-pick (though I haven't tried).

Also, "A...B C...D" would be a candidate for union. Lot's to do for
after 1.7.5...

Michael