Usability improvement request: git show revision -- file

PATCH In review Piotr Krukowiecki 5 participants 29 messages Jun 15, 2016

lore

Share & tools

lore

Piotr Krukowiecki

author

2016-06-15 22:50 UTC #0 lore raw

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.

Signature · 1 line

Piotr Krukowiecki

Michael J Gruber

2016-06-15 22:50 UTC #1 lore raw

Piotr Krukowiecki venit, vidit, dixit 31.03.2011 08:45:

Quoted 4 lines

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

Quoted 8 lines

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

Quoted 6 lines

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? ;)

Quoted 8 lines

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

Michael J Gruber

2016-06-15 22:50 UTC #2 lore raw

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

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 <[email protected]>

---
 t/t1506-rev-parse-diagnosis.sh |   17 ++++++++++++-----
 1 files changed, 12 insertions(+), 5 deletions(-)

t/t1506-rev-parse-diagnosis.sh +12 -5

index 9f8adb1..f9cb202 100755
@@ -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
 '

Signature · 1 line

1.7.4.2.668.gba03a4

Michael J Gruber

2016-06-15 22:50 UTC #3 lore raw

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

In reply to 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 <[email protected]>

---
 sha1_name.c                    |   11 +++++++----
 t/t1506-rev-parse-diagnosis.sh |    2 +-
 2 files changed, 8 insertions(+), 5 deletions(-)

sha1_name.c +7 -4

index faea58d..69cd6c8 100644
@@ -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))

t/t1506-rev-parse-diagnosis.sh +1 -1

index f9cb202..4a6396f 100755
@@ -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
 }

Signature · 1 line

1.7.4.2.668.gba03a4

Junio C Hamano

2016-06-15 22:50 UTC #4 lore raw

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

Quoted 2 lines · Michael J Gruber

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?

Michael J Gruber

2016-06-15 22:50 UTC #5 lore raw

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

Junio C Hamano venit, vidit, dixit 31.03.2011 21:26:

Quoted 8 lines

Quoted 2 lines · Michael J Gruber
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

Junio C Hamano

2016-06-15 22:50 UTC #6 lore raw

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

Quoted 11 lines · Michael J Gruber

Junio C Hamano venit, vidit, dixit 31.03.2011 21:26:
Quoted 8 lines
Quoted 2 lines · Michael J Gruber
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?

Michael J Gruber

2016-06-15 22:50 UTC #7 lore raw

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

In reply to 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 <[email protected]>

---
 builtin/log.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

builtin/log.c +1 -0

index 9db43ed..ea0ddb4 100644
@@ -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;

Signature · 1 line

1.7.4.2.668.gba03a4

Johannes Sixt

2016-06-15 22:50 UTC #8 lore raw

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

Am 3/31/2011 11:17, schrieb Michael J Gruber:

Quoted 9 lines

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

Michael J Gruber

2016-06-15 22:50 UTC #9 lore raw

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

Johannes Sixt venit, vidit, dixit 31.03.2011 12:18:

Quoted 6 lines

Am 3/31/2011 11:17, schrieb Michael J Gruber:
Quoted 5 lines
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.

Quoted 7 lines

Quoted 4 lines
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

Johannes Sixt

2016-06-15 22:50 UTC #10 lore raw

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

Am 3/31/2011 12:58, schrieb Michael J Gruber:

Quoted 15 lines

Johannes Sixt venit, vidit, dixit 31.03.2011 12:18:
Quoted 6 lines
Am 3/31/2011 11:17, schrieb Michael J Gruber:
Quoted 5 lines
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.
Quoted 7 lines
Quoted 4 lines
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?

Quoted 8 lines

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

Michael J Gruber

2016-06-15 22:50 UTC #11 lore raw

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

Johannes Sixt venit, vidit, dixit 31.03.2011 13:42:

Quoted 25 lines

Am 3/31/2011 12:58, schrieb Michael J Gruber:
Quoted 15 lines
Johannes Sixt venit, vidit, dixit 31.03.2011 12:18:
Quoted 6 lines
Am 3/31/2011 11:17, schrieb Michael J Gruber:
Quoted 5 lines
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.
Quoted 7 lines
Quoted 4 lines
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?
Quoted 8 lines
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".

Quoted 7 lines

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

Nguyen Thai Ngoc Duy

2016-06-15 22:50 UTC #12 lore raw

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

In reply to Michael J Gruber

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

Quoted 2 lines · Michael J Gruber

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

Quoted 8 lines

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?

Quoted 3 lines

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.

Signature · 1 line

Duy

Michael J Gruber

2016-06-15 22:50 UTC #13 lore raw

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

Nguyen Thai Ngoc Duy venit, vidit, dixit 31.03.2011 14:50:

Quoted 2 lines

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

Quoted 12 lines

Quoted 2 lines · Michael J Gruber
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".
Quoted 4 lines
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.

Quoted 6 lines

Quoted 4 lines
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.

Quoted 6 lines

Quoted 3 lines
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

Junio C Hamano

2016-06-15 22:50 UTC #14 lore raw

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

Quoted 5 lines · Michael J Gruber

Quoted 1 line
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.

Michael J Gruber

2016-06-15 22:50 UTC #15 lore raw

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

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 <[email protected]>

---
 builtin/log.c |   21 +++++++++++++++------
 1 files changed, 15 insertions(+), 6 deletions(-)

builtin/log.c +15 -6

index 9db43ed..f585209 100644
@@ -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.

Signature · 1 line

1.7.4.2.668.gba03a4

Michael J Gruber

2016-06-15 22:50 UTC #16 lore raw

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

In reply to Junio C Hamano

Junio C Hamano venit, vidit, dixit 31.03.2011 21:23:

Quoted 4 lines

Quoted 2 lines · Michael J Gruber
Quoted 1 line
Tests please?

Quoted 8 lines

Quoted 3 lines
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

Michael J Gruber

2016-06-15 22:50 UTC #17 lore raw

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

In reply to Junio C Hamano

"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 <[email protected]>

---
 t/t1411-reflog-show.sh |   18 ++++++++++++++++++
 1 files changed, 18 insertions(+), 0 deletions(-)

t/t1411-reflog-show.sh +18 -0

index ba25ff3..88dc6a7 100755
@@ -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 <[email protected]>)
+Reflog message: commit (initial): one
+Author: A U Thor <[email protected]>
+
+    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 <[email protected]>)
 Reflog message: commit (initial): one

Signature · 1 line

1.7.4.2.668.gba03a4

Michael J Gruber

2016-06-15 22:50 UTC #18 lore raw

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

In reply to Junio C Hamano

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 <[email protected]>

---
 builtin/log.c          |    9 ++-------
 t/t1411-reflog-show.sh |    2 +-
 2 files changed, 3 insertions(+), 8 deletions(-)

builtin/log.c +2 -7

index f585209..916019c 100644
@@ -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);
 }

t/t1411-reflog-show.sh +1 -1

index 88dc6a7..caa687b 100755
@@ -41,7 +41,7 @@ Author: A U Thor <[email protected]>
 
     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
 '

Signature · 1 line

1.7.4.2.668.gba03a4

Michael J Gruber

2016-06-15 22:50 UTC #19 lore raw

cover reflog, show and command line overrides

In reply to Junio C Hamano

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

Signature · 1 line

1.7.4.2.668.gba03a4

Michael J Gruber

2016-06-15 22:50 UTC #20 lore raw

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

In reply to Junio C Hamano

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 <[email protected]>

---
 builtin/log.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

builtin/log.c +2 -0

index 916019c..474a76d 100644
@@ -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;

Signature · 1 line

1.7.4.2.668.gba03a4

Junio C Hamano

2016-06-15 22:50 UTC #21 lore raw

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

Quoted 4 lines · 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.

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

Quoted 6 lines

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.

Quoted 18 lines

Signed-off-by Michael J Gruber <[email protected]>

---
 builtin/log.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

builtin/log.c +2 -0

index 916019c..474a76d 100644
@@ -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;

Junio C Hamano

2016-06-15 22:50 UTC #22 lore raw

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

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 <[email protected]>

Signed-off-by Junio C Hamano <[email protected]>

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.

Michael J Gruber

2016-06-15 22:50 UTC #23 lore raw

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

Junio C Hamano venit, vidit, dixit 02.04.2011 00:59:

Quoted 5 lines

I attempted to rewrite the log message in a bit more objective voice like this:
builtin/show: do not prune by pathspec

Quoted 7 lines

"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 ;)

Quoted 5 lines

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.

Quoted 8 lines

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 <[email protected]>
Signed-off-by Junio C Hamano <[email protected]>
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...

Quoted 7 lines

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

Quoted 9 lines

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

Junio C Hamano

2016-06-15 22:50 UTC #24 lore raw

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

In reply to Michael J Gruber

builtin/log.c int cmd_show(int argc, const char **argv, const char *prefix) · line 424

Quoted 10 lines · Michael J Gruber

builtin/log.c +2 -0

index 916019c..474a76d 100644
@@ -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.

Michael J Gruber

2016-06-15 22:50 UTC #25 lore raw

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

Junio C Hamano venit, vidit, dixit 04.04.2011 23:49:

Quoted 28 lines

builtin/log.c int cmd_show(int argc, const char **argv, const char *prefix) · line 424
Quoted 10 lines · Michael J Gruber
builtin/log.c +2 -0
index 916019c..474a76d 100644
@@ -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;

Quoted 7 lines

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

Quoted 21 lines

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

Quoted 5 lines

    $ 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

Nguyen Thai Ngoc Duy

2016-06-15 22:50 UTC #26 lore raw

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

In reply to Michael J Gruber

Quoted 5 lines · Michael J Gruber

Quoted 1 line
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.

Quoted 11 lines

Quoted 6 lines
Quoted 3 lines
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 :)

Signature · 1 line

Duy

Michael J Gruber

2016-06-15 22:50 UTC #27 lore raw

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

Nguyen Thai Ngoc Duy venit, vidit, dixit 31.03.2011 15:35:

Quoted 17 lines

Quoted 5 lines · Michael J Gruber
Quoted 1 line
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.
Quoted 7 lines
Quoted 6 lines
Quoted 3 lines
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.

Quoted 7 lines

Quoted 4 lines
"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

Piotr Krukowiecki

author

2016-06-15 22:50 UTC #28 lore raw

In reply to Michael J Gruber

Quoted 8 lines · Michael J Gruber

Piotr Krukowiecki venit, vidit, dixit 31.03.2011 08:45:
Quoted 7 lines
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.

Quoted 8 lines

Quoted 5 lines
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.

Quoted 1 line

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

I see - that makes sense too.

Quoted 1 line

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

Not sure what you mean by that?

Quoted 8 lines

Quoted 6 lines
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.

Quoted 8 lines

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

Quoted 5 lines

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.

Signature · 1 line

Piotr Krukowiecki