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

Michael J Gruber [off-list ref] writes:

quoted

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.

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

Michael J Gruber [off-list ref] writes:

quoted

quoted

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

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

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 <redacted>
---
 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

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 <redacted>
---
 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

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

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 <redacted>
---
 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