[PATCH v2 0/2] make commit --verbose work with --no-status

[PATCH v2 0/2] make commit --verbose work with --no-status

[Tay Ray Chuan]

When running git-commit`, --verbose appends a diff to the prepared
message, while --no-status omits git-status output; thus, one would
expect --verbose --no-status to give a commit message with a diff of
the commit without git-status output.

However, this is not what happens - the prepared commit message body is
empty, entirely. (Needless to say, no diff is appended.) This patch
series attempts to make this work, as one would expect.

[PATCH 1/2] extract setting of wt_status.commitable flag out of
wt_status_print_updated()

Currently, the wt_status.commitable flag is set only when we call
wt_status_print(). * Extract this logic, so that we don't have to go
through the git-status output code just to set the flag.

* In fact, it is not set when --short or --porcelain are used. This
  series does not attempt to fix the bug, though it should be trivial to
  do so with this patch. See 9cbcc2a (demonstrate git-commit --dry-run
  exit code behaviour, Feb 22 2014).

[PATCH 2/2] make commit --verbose work with --no-status

Actual work here.

-- 
2.0.0.581.g64f2558

[PATCH v2 1/2] extract setting of wt_status.commitable flag out of wt_status_print_updated()

[Tay Ray Chuan]

It may not be obvious from its name that wt_status_print_updated() that
it also sets wt_status.commitable, which affects commit functionality.
Extract this out into a separate function for improved clarity, though
at the expense of executing another loop.

Signed-off-by: Tay Ray Chuan <rctay89@gmail.com>
---

Changed since v1
- move call to _mark_committable() to match original control-flow

  Originally, our placement of the call perhaps befitted aesthetics /
  logical grouping. But perhaps it is a better idea to match the original
  control flow to dispel any suspicion that this patch changed behaviour
  unintendedly.

 wt-status.c | 17 ++++++++++++++++-
 1 file changed, 16 insertions(+), 1 deletion(-)

diff --git a/wt-status.c b/wt-status.c
index c56c78f..87550ae 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -626,6 +626,21 @@ void wt_status_collect(struct wt_status *s)
 	wt_status_collect_untracked(s);
 }
 
+void wt_status_mark_commitable(struct wt_status *s)
+{
+	int i;
+
+	for (i = 0; i < s->change.nr; i++) {
+		struct wt_status_change_data *d;
+		d = s->change.items[i].util;
+		if (!d->index_status ||
+		    d->index_status == DIFF_STATUS_UNMERGED)
+			continue;
+		s->commitable = 1;
+		break;
+	}
+}
+
 static void wt_status_print_unmerged(struct wt_status *s)
 {
 	int shown_header = 0;
@@ -664,7 +679,6 @@ static void wt_status_print_updated(struct wt_status *s)
 			continue;
 		if (!shown_header) {
 			wt_status_print_cached_header(s);
-			s->commitable = 1;
 			shown_header = 1;
 		}
 		wt_status_print_change_data(s, WT_STATUS_UPDATED, it);
@@ -1360,6 +1374,7 @@ void wt_status_print(struct wt_status *s)
 
 	wt_status_print_updated(s);
 	wt_status_print_unmerged(s);
+	wt_status_mark_commitable(s);
 	wt_status_print_changed(s);
 	if (s->submodule_summary &&
 	    (!s->ignore_submodule_arg ||
-- 
2.0.0.581.g64f2558

[PATCH v2 2/2] make commit --verbose work with --no-status

[Tay Ray Chuan]

When running git-commit`, --verbose appends a diff to the prepared
message, while --no-status omits git-status output; thus, one would
expect --verbose --no-status to give a commit message with a diff of
the commit without git-status output.

However, this is not what happens - the prepared commit message body is
empty, entirely. (Needless to say, no diff is appended.) This is because
internally the git-status machinery is used to provide both the diff and
status output, but this machinery is skipped over entirely due to
--no-status.

We introduce a new status_format, STATUS_FORMAT_DIFFONLY, which triggers
the setting of the commitable flag, and the printing of the diff. This
is set only by git-commit, and when it detects that --verbose and
--no-status have been used.

Signed-off-by: Tay Ray Chuan <rctay89@gmail.com>
---

Changed since v1: adopted peff's suggestion in
20140224083312.GB32594@sigill.intra.peff.net, added tests.

 builtin/commit.c          | 12 +++++++++++-
 t/t7507-commit-verbose.sh | 34 +++++++++++++++++++++++++++++++++-
 wt-status.c               |  2 +-
 wt-status.h               |  3 +++
 4 files changed, 48 insertions(+), 3 deletions(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index 254477f..d752899 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -144,6 +144,7 @@ static enum status_format {
 	STATUS_FORMAT_LONG,
 	STATUS_FORMAT_SHORT,
 	STATUS_FORMAT_PORCELAIN,
+	STATUS_FORMAT_DIFFONLY,
 
 	STATUS_FORMAT_UNSPECIFIED
 } status_format = STATUS_FORMAT_UNSPECIFIED;
@@ -510,6 +511,10 @@ static int run_status(FILE *fp, const char *index_file, const char *prefix, int
 	case STATUS_FORMAT_UNSPECIFIED:
 		die("BUG: finalize_deferred_config() should have been called");
 		break;
+	case STATUS_FORMAT_DIFFONLY:
+		wt_status_mark_commitable(s);
+		wt_status_print_verbose(s);
+		break;
 	case STATUS_FORMAT_NONE:
 	case STATUS_FORMAT_LONG:
 		wt_status_print(s);
@@ -1213,7 +1218,12 @@ static int parse_and_validate_options(int argc, const char *argv[],
 	if (all && argc > 0)
 		die(_("Paths with -a does not make sense."));
 
-	if (status_format != STATUS_FORMAT_NONE)
+	if (status_format == STATUS_FORMAT_NONE) {
+		if (verbose && !include_status) {
+			include_status = 1;
+			status_format = STATUS_FORMAT_DIFFONLY;
+		}
+	} else
 		dry_run = 1;
 
 	return argc;
diff --git a/t/t7507-commit-verbose.sh b/t/t7507-commit-verbose.sh
index 2ddf28c..9027dd4 100755
--- a/t/t7507-commit-verbose.sh
+++ b/t/t7507-commit-verbose.sh
@@ -26,7 +26,39 @@ test_expect_success 'initial commit shows verbose diff' '
 	git commit --amend -v
 '
 
-test_expect_success 'second commit' '
+test_expect_success '--verbose appends diff' '
+	cat >expected <<-\EOF &&
+	# ------------------------ >8 ------------------------
+	# Do not touch the line above.
+	# Everything below will be removed.
+	diff --git a/file b/file
+	index d95f3ad..94ab063 100644
+	--- a/file
+	+++ b/file
+	@@ -1 +1,2 @@
+	 content
+	+content content
+	EOF
+	cat >editor <<-\EOF &&
+	#!/bin/sh
+	awk "/^# -+ >8 -+$/ { p=1 } p" "$1" >actual
+	echo commit > "$1"
+	EOF
+	chmod 755 editor &&
+	echo content content >> file &&
+	git add file &&
+	test_tick &&
+	EDITOR=./editor git commit --verbose &&
+	test_cmp expected actual
+'
+
+test_expect_success '--verbose --no-status appends diff' '
+	git reset --soft HEAD^ &&
+	EDITOR=./editor git commit --verbose --no-status &&
+	test_cmp expected actual
+'
+
+test_expect_success 'commit' '
 	echo content modified >file &&
 	git add file &&
 	git commit -F message
diff --git a/wt-status.c b/wt-status.c
index 87550ae..c4f7e48 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -857,7 +857,7 @@ void wt_status_add_cut_line(FILE *fp)
 	strbuf_release(&buf);
 }
 
-static void wt_status_print_verbose(struct wt_status *s)
+void wt_status_print_verbose(struct wt_status *s)
 {
 	struct rev_info rev;
 	struct setup_revision_opt opt;
diff --git a/wt-status.h b/wt-status.h
index e0a99f7..4388296 100644
--- a/wt-status.h
+++ b/wt-status.h
@@ -98,8 +98,11 @@ void wt_status_add_cut_line(FILE *fp);
 void wt_status_prepare(struct wt_status *s);
 void wt_status_print(struct wt_status *s);
 void wt_status_collect(struct wt_status *s);
+void wt_status_mark_commitable(struct wt_status *state);
 void wt_status_get_state(struct wt_status_state *state, int get_detached_from);
 
+void wt_status_print_verbose(struct wt_status *s);
+
 void wt_shortstatus_print(struct wt_status *s);
 void wt_porcelain_print(struct wt_status *s);
 
-- 
2.0.0.581.g64f2558

Re: [PATCH v2 1/2] extract setting of wt_status.commitable flag out of wt_status_print_updated()

[Junio C Hamano]

Tay Ray Chuan <rctay89@gmail.com> writes:

> It may not be obvious from its name that wt_status_print_updated() that
> it also sets wt_status.commitable, which affects commit functionality.
> Extract this out into a separate function for improved clarity, though
> at the expense of executing another loop.

Makes sense.

> @@ -1360,6 +1374,7 @@ void wt_status_print(struct wt_status *s)
>  
>  	wt_status_print_updated(s);
>  	wt_status_print_unmerged(s);
> +	wt_status_mark_commitable(s);
>  	wt_status_print_changed(s);
>  	if (s->submodule_summary &&
>  	    (!s->ignore_submodule_arg ||

As this is the only callsite of _updated(), we can be assured that
the conversion would not change the behaviour.

But I am not sure the placement of the new call is sensible.  The
standard pattern used in the wt-status infrastructure is to first
collect the information and then make output based on what was
collected.  Because the value of this patch is to separte the "is it
committable?" information gathering step out of the output step,
shouldn't the call be made a lot earlier than these sequence of
wt_status_print_blah() calls?

I am wondering if the flipping of the "is it committable?" bit
belongs to wt_status_collect().  It could be that some other crufty
checks that wt_status_print() have accumulated over time might be
better moved to the "collect" phase, but that is a separate topic.

Re: [PATCH v2 0/2] make commit --verbose work with --no-status

[Junio C Hamano]

Tay Ray Chuan <rctay89@gmail.com> writes:

> When running git-commit`, --verbose appends a diff to the prepared
> message, while --no-status omits git-status output.

The --verbose option is called --verbose and not --diff or --patch
for a reason, though.  The default is to show extra information as
comments, and verbose tells us to make that extra information more
verbose.  We call that extra information "status", so it is natural
for "--no-status" to drop that extra information.

> ; thus, one would
> expect --verbose --no-status to give a commit message with a diff of
> the commit without git-status output.
>
> However, this is not what happens

And for a good reason, I would think.

Re: [PATCH v2 0/2] make commit --verbose work with --no-status

[Tay Ray Chuan]

Hi Junio,

On Fri, Jun 5, 2015 at 2:39 AM, Junio C Hamano <gitster@pobox.com> wrote:
>
> Tay Ray Chuan <rctay89@gmail.com> writes:
>
> > When running git-commit`, --verbose appends a diff to the prepared
> > message, while --no-status omits git-status output.
>
> The --verbose option is called --verbose and not --diff or --patch
> for a reason, though.  The default is to show extra information as
> comments, and verbose tells us to make that extra information more
> verbose.  We call that extra information "status", so it is natural
> for "--no-status" to drop that extra information.

Thanks for the explanation. Now I can appreciate why git-commit works this way.

Would it be a good idea to have a --diff-only option to include diff,
but not status output? Or perhaps a --diff option, while leaving it to
the user to specify if status output is to be included with
--no-status, which would open the doors for mixing and matching status
formatting control, eg. with --short.

-- 
Cheers,
Ray Chuan

Re: [PATCH v2 0/2] make commit --verbose work with --no-status

[Junio C Hamano]

Tay Ray Chuan <rctay89@gmail.com> writes:

> Would it be a good idea to have a --diff-only option to include diff,
> but not status output? Or perhaps a --diff option, while leaving it to
> the user to specify if status output is to be included with
> --no-status, which would open the doors for mixing and matching status
> formatting control, eg. with --short.

The name "--diff-only" does not sound right, as people would wonder
what should happen when you give "--status --diff-only".

Perhaps you would need to do some careful thinking, similar to what
we did when deciding the "diff" and "log" options.

We originally had "--patch" and then "--patch-with-stat" to "diff"
and "log", but soon after that people found that "show only stat
without the patch text" is a useful thing to do.  We retrofitted the
command line parser to take "--patch" and "--stat" as orthogonal but
inter-related options, which was a successful conversion that did
not break backward compatibility (These days people would not even
know that these strangely combined forms "--patch-with-stat" and
"--patch-with-raw" even exist).

All of the above assumes that showing only the patch and not other
hints to help situation awareness while making a commit is a useful
thing in the first place.  I am undecided on that point myself.

Thanks.

Re: [PATCH v2 0/2] make commit --verbose work with --no-status

[Tay Ray Chuan]

On Sat, Jun 6, 2015 at 12:03 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Tay Ray Chuan <rctay89@gmail.com> writes:
>
>> Would it be a good idea to have a --diff-only option to include diff,
>> but not status output? Or perhaps a --diff option, while leaving it to
>> the user to specify if status output is to be included with
>> --no-status, which would open the doors for mixing and matching status
>> formatting control, eg. with --short.
>
> The name "--diff-only" does not sound right, as people would wonder
> what should happen when you give "--status --diff-only".
>
> Perhaps you would need to do some careful thinking, similar to what
> we did when deciding the "diff" and "log" options.
>
> We originally had "--patch" and then "--patch-with-stat" to "diff"
> and "log", but soon after that people found that "show only stat
> without the patch text" is a useful thing to do.  We retrofitted the
> command line parser to take "--patch" and "--stat" as orthogonal but
> inter-related options, which was a successful conversion that did
> not break backward compatibility (These days people would not even
> know that these strangely combined forms "--patch-with-stat" and
> "--patch-with-raw" even exist).
>
> All of the above assumes that showing only the patch and not other
> hints to help situation awareness while making a commit is a useful
> thing in the first place.  I am undecided on that point myself.

Hmm, perhaps such functionality should be off-loaded to a third-party
wrapper. (I'd not be surprised if most wrappers already have this.)

-- 
Cheers,
Ray Chuan

Re: [PATCH v2 0/2] make commit --verbose work with --no-status

[Junio C Hamano]

Tay Ray Chuan <rctay89@gmail.com> writes:

>> All of the above assumes that showing only the patch and not other
>> hints to help situation awareness while making a commit is a useful
>> thing in the first place.  I am undecided on that point myself.
>
> Hmm, perhaps such functionality should be off-loaded to a third-party
> wrapper. (I'd not be surprised if most wrappers already have this.)

If you believe that parenthesised comment to be true, then that
would be a sign that such a feature is desirable, no?  Substantiate
it and let's relieve the third-party wrappers of that burden, then.