threads / patch / 35928

patch, 3 partsmake commit --verbose work with --no-status

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

## tl;dr

8 messages between Feb 21, 2014 and Feb 24, 2014. Diffs are folded; open one to read it.

replies: 7people: 2as markdown or json

Tay Ray Chuan· Feb 21, 2014, 19:09 UTC · lore

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
Tay Ray Chuan· Feb 21, 2014, 19:09 UTC · re: Tay Ray Chuan · lore

[PATCH 1/3] rename STATUS_FORMAT_NONE to STATUS_FORMAT_DEFAULT

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(-)
Show changes to builtin/commit.c +8 −8
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
Tay Ray Chuan· Feb 21, 2014, 19:09 UTC · re: Tay Ray Chuan · lore

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

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(-)
Show changes to wt-status.c +16 −1
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
Tay Ray Chuan· Feb 21, 2014, 19:09 UTC · re: Tay Ray Chuan · lore

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

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(-)
Show changes to 3 files +17 −2

builtin/commit.c, wt-status.c, wt-status.h

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
Jeff King· Feb 22, 2014, 08:31 UTC · re: Tay Ray Chuan · lore

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

On Sat, Feb 22, 2014 at 03:09:22AM +0800, Tay Ray Chuan wrote:
Show 12 quoted lines
> @@ -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
Tay Ray Chuan· Feb 24, 2014, 04:24 UTC · re: Jeff King · lore

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

On Sat, Feb 22, 2014 at 4:31 PM, Jeff King <peff@peff.net> wrote:
Show 19 quoted lines
> 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--
Show changes to diff +4 −1
@@ -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;
Jeff King· Feb 24, 2014, 08:33 UTC · re: Tay Ray Chuan · lore

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

On Mon, Feb 24, 2014 at 12:24:42PM +0800, Tay Ray Chuan wrote:
Show 21 quoted lines
> > 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
Jeff King· Feb 22, 2014, 08:24 UTC · re: Tay Ray Chuan · lore

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

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

← back to recent threads