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

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

[Tay Ray Chuan]

One would expect 'git commit --verbose --no-status' to give a commit
message with a diff of the commit, sans the output of git-status.
However, this does not work currently; the commit message body is
entirely empty (diff is absent as well). This patch series attempts to
make this work, as one would expect.

[PATCH 1/3] rename STATUS_FORMAT_NONE to STATUS_FORMAT_DEFAULT

We first rename STATUS_FORMAT_NONE to *_DEFAULT, so that we can use it
to mean "no status output at all".

[PATCH 2/3] 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.

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

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

Actual work here.

-- 
1.9.0.291.g027825b

[PATCH 1/3] rename STATUS_FORMAT_NONE to STATUS_FORMAT_DEFAULT

[Tay Ray Chuan]

Cc: Jeff King <peff@peff.net>

In f3f47a1 (status: add --long output format option), STATUS_FORMAT_NONE
was introduced, meaning "the user did not specify anything". Rename this
to *_DEFAULT to better indicate its meaning.

This paves the way for _NONE to really mean "no status".

Signed-off-by: Tay Ray Chuan <rctay89@gmail.com>
---
 builtin/commit.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index 3783bca..2e86b76 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -125,7 +125,7 @@ static const char *only_include_assumed;
 static struct strbuf message = STRBUF_INIT;
 
 static enum status_format {
-	STATUS_FORMAT_NONE = 0,
+	STATUS_FORMAT_DEFAULT = 0,
 	STATUS_FORMAT_LONG,
 	STATUS_FORMAT_SHORT,
 	STATUS_FORMAT_PORCELAIN,
@@ -487,7 +487,7 @@ 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_NONE:
+	case STATUS_FORMAT_DEFAULT:
 	case STATUS_FORMAT_LONG:
 		wt_status_print(s);
 		break;
@@ -1028,7 +1028,7 @@ static void finalize_deferred_config(struct wt_status *s)
 				   !s->null_termination);
 
 	if (s->null_termination) {
-		if (status_format == STATUS_FORMAT_NONE ||
+		if (status_format == STATUS_FORMAT_DEFAULT ||
 		    status_format == STATUS_FORMAT_UNSPECIFIED)
 			status_format = STATUS_FORMAT_PORCELAIN;
 		else if (status_format == STATUS_FORMAT_LONG)
@@ -1038,7 +1038,7 @@ static void finalize_deferred_config(struct wt_status *s)
 	if (use_deferred_config && status_format == STATUS_FORMAT_UNSPECIFIED)
 		status_format = status_deferred_config.status_format;
 	if (status_format == STATUS_FORMAT_UNSPECIFIED)
-		status_format = STATUS_FORMAT_NONE;
+		status_format = STATUS_FORMAT_DEFAULT;
 
 	if (use_deferred_config && s->show_branch < 0)
 		s->show_branch = status_deferred_config.show_branch;
@@ -1141,7 +1141,7 @@ 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_DEFAULT)
 		dry_run = 1;
 
 	return argc;
@@ -1197,7 +1197,7 @@ static int git_status_config(const char *k, const char *v, void *cb)
 		if (git_config_bool(k, v))
 			status_deferred_config.status_format = STATUS_FORMAT_SHORT;
 		else
-			status_deferred_config.status_format = STATUS_FORMAT_NONE;
+			status_deferred_config.status_format = STATUS_FORMAT_DEFAULT;
 		return 0;
 	}
 	if (!strcmp(k, "status.branch")) {
@@ -1314,7 +1314,7 @@ int cmd_status(int argc, const char **argv, const char *prefix)
 	case STATUS_FORMAT_UNSPECIFIED:
 		die("BUG: finalize_deferred_config() should have been called");
 		break;
-	case STATUS_FORMAT_NONE:
+	case STATUS_FORMAT_DEFAULT:
 	case STATUS_FORMAT_LONG:
 		s.verbose = verbose;
 		s.ignore_submodule_arg = ignore_submodule_arg;
@@ -1522,7 +1522,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
 		usage_with_options(builtin_commit_usage, builtin_commit_options);
 
 	status_init_config(&s, git_commit_config);
-	status_format = STATUS_FORMAT_NONE; /* Ignore status.short */
+	status_format = STATUS_FORMAT_DEFAULT; /* Ignore status.short */
 	s.colopts = 0;
 
 	if (get_sha1("HEAD", sha1))
-- 
1.9.0.291.g027825b

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

[Tay Ray Chuan]

At first glance, wt_status_print_updated() appears to be doing a bunch
of printing (as its name implies), and it may not be immediately obvious
that it also sets the vital wt_status.commitable flag. Extract this out
into a separate function; it is hoped that the improved clarity to
future Git contributors would outweigh the performance penalty.

Signed-off-by: Tay Ray Chuan <rctay89@gmail.com>
---
 wt-status.c | 17 ++++++++++++++++-
 1 file changed, 16 insertions(+), 1 deletion(-)

diff --git a/wt-status.c b/wt-status.c
index a452407..9b0189c 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -589,6 +589,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;
@@ -627,7 +642,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);
@@ -1309,6 +1323,7 @@ void wt_status_print(struct wt_status *s)
 		status_printf_ln(s, color(WT_STATUS_HEADER, s), "");
 	}
 
+	wt_status_mark_commitable(s);
 	wt_status_print_updated(s);
 	wt_status_print_unmerged(s);
 	wt_status_print_changed(s);
-- 
1.9.0.291.g027825b

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

[Tay Ray Chuan]

One would expect 'git commit --verbose --no-status' to give a commit
message with a diff of the commit, sans the output of git-status.
However, this does not work currently; the commit message body is
entirely empty (diff is absent as well). This is because internally the
status machinery is used to provide both the diff and status output, but
it is skipped due to --no-status.

We introduce a new status_format, STATUS_FORMAT_NONE. Thus, we still
call run_status(), but produce nothing in place of the usual git-status
output. status_format is set only when git-commit is passed i)
--verbose, and ii) --no-status.

Signed-off-by: Tay Ray Chuan <rctay89@gmail.com>
---
 builtin/commit.c | 14 +++++++++++++-
 wt-status.c      |  2 +-
 wt-status.h      |  3 +++
 3 files changed, 17 insertions(+), 2 deletions(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index 2e86b76..fca6a6b 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -126,6 +126,7 @@ static struct strbuf message = STRBUF_INIT;
 
 static enum status_format {
 	STATUS_FORMAT_DEFAULT = 0,
+	STATUS_FORMAT_NONE,
 	STATUS_FORMAT_LONG,
 	STATUS_FORMAT_SHORT,
 	STATUS_FORMAT_PORCELAIN,
@@ -478,6 +479,10 @@ static int run_status(FILE *fp, const char *index_file, const char *prefix, int
 	wt_status_collect(s);
 
 	switch (status_format) {
+	case STATUS_FORMAT_NONE:
+		wt_status_mark_commitable(s);
+		wt_status_print_verbose(s);
+		break;
 	case STATUS_FORMAT_SHORT:
 		wt_shortstatus_print(s);
 		break;
@@ -1141,7 +1146,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_DEFAULT)
+	if (verbose && !include_status) {
+		include_status = 1;
+		status_format = STATUS_FORMAT_NONE;
+	}
+
+	if (status_format != STATUS_FORMAT_DEFAULT && !verbose)
 		dry_run = 1;
 
 	return argc;
@@ -1305,6 +1315,8 @@ int cmd_status(int argc, const char **argv, const char *prefix)
 		s.prefix = prefix;
 
 	switch (status_format) {
+	case STATUS_FORMAT_NONE:
+		break;
 	case STATUS_FORMAT_SHORT:
 		wt_shortstatus_print(&s);
 		break;
diff --git a/wt-status.c b/wt-status.c
index 9b0189c..e90fd0f 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -822,7 +822,7 @@ void wt_status_truncate_message_at_cut_line(struct strbuf *buf)
 	strbuf_release(&pattern);
 }
 
-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 30a4812..5c89f23 100644
--- a/wt-status.h
+++ b/wt-status.h
@@ -95,8 +95,11 @@ void wt_status_truncate_message_at_cut_line(struct strbuf *);
 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);
 
-- 
1.9.0.291.g027825b

Re: [PATCH 1/3] rename STATUS_FORMAT_NONE to STATUS_FORMAT_DEFAULT

[Jeff King]

On Sat, Feb 22, 2014 at 03:09:20AM +0800, Tay Ray Chuan wrote:

> In f3f47a1 (status: add --long output format option), STATUS_FORMAT_NONE
> was introduced, meaning "the user did not specify anything". Rename this
> to *_DEFAULT to better indicate its meaning.

Hmm. We later introduced STATUS_FORMAT_UNSPECIFIED in 84b4202d. It seems
like that is the same thing as the _DEFAULT you are proposing here. Can
we collapse them into a single value?

-Peff

Re: [PATCH 3/3] make commit --verbose work with --no-status

[Jeff King]

On Sat, Feb 22, 2014 at 03:09:22AM +0800, Tay Ray Chuan wrote:

> @@ -1141,7 +1146,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_DEFAULT)
> +	if (verbose && !include_status) {
> +		include_status = 1;
> +		status_format = STATUS_FORMAT_NONE;
> +	}
> +
> +	if (status_format != STATUS_FORMAT_DEFAULT && !verbose)
>  		dry_run = 1;

What happens here when there is an alternate status format _and_
--verbose is used? If I say "git commit --porcelain" it should imply
--dry-run. But "git commit --porcelain --verbose" no longer does so
after your patch.

-Peff

Re: [PATCH 3/3] make commit --verbose work with --no-status

[Tay Ray Chuan]

On Sat, Feb 22, 2014 at 4:31 PM, Jeff King <peff@peff.net> wrote:
> On Sat, Feb 22, 2014 at 03:09:22AM +0800, Tay Ray Chuan wrote:
>
>> @@ -1141,7 +1146,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_DEFAULT)
>> +     if (verbose && !include_status) {
>> +             include_status = 1;
>> +             status_format = STATUS_FORMAT_NONE;
>> +     }
>> +
>> +     if (status_format != STATUS_FORMAT_DEFAULT && !verbose)
>>               dry_run = 1;
>
> What happens here when there is an alternate status format _and_
> --verbose is used? If I say "git commit --porcelain" it should imply
> --dry-run. But "git commit --porcelain --verbose" no longer does so
> after your patch.

I should have just left the --dry-run inference alone, like this.

-->8--

@@ -1141,7 +1146,12 @@ static int parse_and_validate_options
        if (all && argc > 0)
                die(_("Paths with -a does not make sense."));

-       if (status_format != STATUS_FORMAT_DEFAULT)
+       if (verbose && !include_status) {
+               include_status = 1;
+               status_format = STATUS_FORMAT_NONE;
+       } else if (status_format != STATUS_FORMAT_DEFAULT)
                dry_run = 1;

        return argc;

Re: [PATCH 3/3] make commit --verbose work with --no-status

[Jeff King]

On Mon, Feb 24, 2014 at 12:24:42PM +0800, Tay Ray Chuan wrote:

> > What happens here when there is an alternate status format _and_
> > --verbose is used? If I say "git commit --porcelain" it should imply
> > --dry-run. But "git commit --porcelain --verbose" no longer does so
> > after your patch.
> 
> I should have just left the --dry-run inference alone, like this.
> 
> -->8--
> 
> @@ -1141,7 +1146,12 @@ static int parse_and_validate_options
>         if (all && argc > 0)
>                 die(_("Paths with -a does not make sense."));
> 
> -       if (status_format != STATUS_FORMAT_DEFAULT)
> +       if (verbose && !include_status) {
> +               include_status = 1;
> +               status_format = STATUS_FORMAT_NONE;
> +       } else if (status_format != STATUS_FORMAT_DEFAULT)
>                 dry_run = 1;
> 
>         return argc;

Hrm, not quite, because the way include_status works is weird. If I turn
it off in the config, like this:

  git config commit.status false

then asking explicitly for a status format should still dry-run and show
it:

  git commit --porcelain

IOW, include_status is only respected when we are generating the actual
commit. So I think you need something more like:

  if (status_format == STATUS_FORMAT_DEFAULT) {
          if (verbose && !include_status) {
                  include_status = 1;
                  status_format = STATUS_FORMAT_NONE;
          }
  } else
          dry_run = 1;

-Peff