git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Samuel Lijin <sxlijin@gmail.com>
To: git@vger.kernel.org
Cc: Samuel Lijin <sxlijin@gmail.com>
Subject: [PATCH v4 4/4] commit: fix exit code when doing a dry run
Date: Sun, 22 Jul 2018 22:09:03 -0400	[thread overview]
Message-ID: <20180723020903.22435-5-sxlijin@gmail.com> (raw)
In-Reply-To: <20180715110807.25544-1-sxlijin@gmail.com>

In wt-status.c, the s->committable bit is set only in the call tree of
wt_longstatus_print(), and it is not always set correctly. This means
that in normal cases, if there are changes to be committed, or if there
is a merge in progress and all conflicts have been resolved, `--dry-run`
and `--long` return the correct exit code but `--short` and
`--porcelain` do not, even though all four flags imply dry run.
Moreover, if there is a merge in progress and some but not all conflicts
have been resolved, `--short` and `--porcelain` only return the correct
exit code by coincidence (because the codepaths they follow never touch
the committable bit), whereas `--dry-run` and `--long` return the wrong
exit code.

Teach wt_status_collect() to set s->committable correctly (if a merge is
in progress, committable should be set iff there are no unmerged
changes; otherwise, committable should be set iff there are changes in
the index) so that all four flags which imply dry runs return the
correct exit code in the above described situations and mark the
documenting tests as fixed.

Use the index_status field in the wt_status_change_data structs in
has_unmerged() to determine whether or not there are unmerged paths,
instead of the stagemask field, to improve readability.

Also stop setting s->committable in wt_longstatus_print_updated() and
show_merge_in_progress(), and const-ify wt_status_state in the method
signatures in those callpaths.

Signed-off-by: Samuel Lijin <sxlijin@gmail.com>
---
 t/t7501-commit.sh | 12 +++----
 wt-status.c       | 80 +++++++++++++++++++++++++++++------------------
 2 files changed, 55 insertions(+), 37 deletions(-)

diff --git a/t/t7501-commit.sh b/t/t7501-commit.sh
index e49dfd0a2..6dba526e6 100755
--- a/t/t7501-commit.sh
+++ b/t/t7501-commit.sh
@@ -99,12 +99,12 @@ test_expect_success '--dry-run with stuff to commit returns ok' '
 	git commit -m next -a --dry-run
 '
 
-test_expect_failure '--short with stuff to commit returns ok' '
+test_expect_success '--short with stuff to commit returns ok' '
 	echo bongo bongo bongo >>file &&
 	git commit -m next -a --short
 '
 
-test_expect_failure '--porcelain with stuff to commit returns ok' '
+test_expect_success '--porcelain with stuff to commit returns ok' '
 	echo bongo bongo bongo >>file &&
 	git commit -m next -a --porcelain
 '
@@ -714,7 +714,7 @@ test_expect_success '--long with only unresolved merge conflicts' '
 	test_cmp expected actual
 '
 
-test_expect_failure '--dry-run with resolved and unresolved merge conflicts' '
+test_expect_success '--dry-run with resolved and unresolved merge conflicts' '
 	git reset --hard commit-2 &&
 	test_must_fail git merge --no-commit commit-1 &&
 	echo "resolve one merge conflict" >test-file1 &&
@@ -747,7 +747,7 @@ test_expect_success '--porcelain with resolved and unresolved merge conflicts' '
 	test_cmp expected actual
 '
 
-test_expect_failure '--long with resolved and unresolved merge conflicts' '
+test_expect_success '--long with resolved and unresolved merge conflicts' '
 	git reset --hard commit-2 &&
 	test_must_fail git merge --no-commit commit-1 &&
 	echo "resolve one merge conflict" >test-file1 &&
@@ -769,7 +769,7 @@ test_expect_success '--dry-run with only resolved merge conflicts' '
 	test_cmp expected actual
 '
 
-test_expect_failure '--short with only resolved merge conflicts' '
+test_expect_success '--short with only resolved merge conflicts' '
 	git reset --hard commit-2 &&
 	test_must_fail git merge --no-commit commit-1 &&
 	echo "resolve all merge conflicts" | tee test-file1 test-file2 &&
@@ -780,7 +780,7 @@ test_expect_failure '--short with only resolved merge conflicts' '
 	test_cmp expected actual
 '
 
-test_expect_failure '--porcelain with only resolved merge conflicts' '
+test_expect_success '--porcelain with only resolved merge conflicts' '
 	git reset --hard commit-2 &&
 	test_must_fail git merge --no-commit commit-1 &&
 	echo "resolve all merge conflicts" | tee test-file1 test-file2 &&
diff --git a/wt-status.c b/wt-status.c
index af83fae68..fc239f61c 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -724,6 +724,38 @@ static void wt_status_collect_untracked(struct wt_status *s)
 		s->untracked_in_ms = (getnanotime() - t_begin) / 1000000;
 }
 
+static int has_unmerged(const struct wt_status *s)
+{
+	int i;
+
+	for (i = 0; i < s->change.nr; i++) {
+		struct wt_status_change_data *d = (s->change.items[i]).util;
+		if (d->index_status == DIFF_STATUS_UNMERGED)
+			return 1;
+	}
+	return 0;
+}
+
+static void wt_status_mark_committable(
+		struct wt_status *s, const struct wt_status_state *state)
+{
+	int i;
+
+	if (state->merge_in_progress) {
+		s->committable = !has_unmerged(s);
+		return;
+	}
+
+	for (i = 0; i < s->change.nr; i++) {
+		struct wt_status_change_data *d = (s->change.items[i]).util;
+
+		if (d->index_status) {
+			s->committable = 1;
+			return;
+		}
+	}
+}
+
 void wt_status_collect(struct wt_status *s, const struct wt_status_state *state)
 {
 	wt_status_collect_changes_worktree(s);
@@ -734,6 +766,8 @@ void wt_status_collect(struct wt_status *s, const struct wt_status_state *state)
 		wt_status_collect_changes_index(s);
 
 	wt_status_collect_untracked(s);
+
+	wt_status_mark_committable(s, state);
 }
 
 static void wt_longstatus_print_unmerged(const struct wt_status *s)
@@ -759,28 +793,27 @@ static void wt_longstatus_print_unmerged(const struct wt_status *s)
 
 }
 
-static void wt_longstatus_print_updated(struct wt_status *s)
+static void wt_longstatus_print_updated(const struct wt_status *s)
 {
-	int shown_header = 0;
 	int i;
 
+	if (!s->committable)
+		return;
+
+	wt_longstatus_print_cached_header(s);
+
 	for (i = 0; i < s->change.nr; i++) {
 		struct wt_status_change_data *d;
 		struct string_list_item *it;
 		it = &(s->change.items[i]);
 		d = it->util;
-		if (!d->index_status ||
-		    d->index_status == DIFF_STATUS_UNMERGED)
-			continue;
-		if (!shown_header) {
-			wt_longstatus_print_cached_header(s);
-			s->committable = 1;
-			shown_header = 1;
+		if (d->index_status &&
+		    d->index_status != DIFF_STATUS_UNMERGED) {
+			wt_longstatus_print_change_data(s, WT_STATUS_UPDATED, it);
 		}
-		wt_longstatus_print_change_data(s, WT_STATUS_UPDATED, it);
 	}
-	if (shown_header)
-		wt_longstatus_print_trailer(s);
+
+	wt_longstatus_print_trailer(s);
 }
 
 /*
@@ -1064,21 +1097,7 @@ static void wt_longstatus_print_tracking(const struct wt_status *s)
 	strbuf_release(&sb);
 }
 
-static int has_unmerged(const 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->stagemask)
-			return 1;
-	}
-	return 0;
-}
-
-static void show_merge_in_progress(struct wt_status *s,
-				const struct wt_status_state *state,
+static void show_merge_in_progress(const struct wt_status *s,
 				const char *color)
 {
 	if (has_unmerged(s)) {
@@ -1090,7 +1109,6 @@ static void show_merge_in_progress(struct wt_status *s,
 					 _("  (use \"git merge --abort\" to abort the merge)"));
 		}
 	} else {
-		s-> committable = 1;
 		status_printf_ln(s, color,
 			_("All conflicts fixed but you are still merging."));
 		if (s->hints)
@@ -1584,12 +1602,12 @@ void wt_status_clear_state(struct wt_status_state *state)
 	free(state->detached_from);
 }
 
-static void wt_longstatus_print_state(struct wt_status *s,
+static void wt_longstatus_print_state(const struct wt_status *s,
 				      const struct wt_status_state *state)
 {
 	const char *state_color = color(WT_STATUS_HEADER, s);
 	if (state->merge_in_progress)
-		show_merge_in_progress(s, state, state_color);
+		show_merge_in_progress(s, state_color);
 	else if (state->am_in_progress)
 		show_am_in_progress(s, state, state_color);
 	else if (state->rebase_in_progress || state->rebase_interactive_in_progress)
@@ -1602,7 +1620,7 @@ static void wt_longstatus_print_state(struct wt_status *s,
 		show_bisect_in_progress(s, state, state_color);
 }
 
-static void wt_longstatus_print(struct wt_status *s, const struct wt_status_state *state)
+static void wt_longstatus_print(const struct wt_status *s, const struct wt_status_state *state)
 {
 	const char *branch_color = color(WT_STATUS_ONBRANCH, s);
 	const char *branch_status_color = color(WT_STATUS_HEADER, s);
-- 
2.18.0


  parent reply	other threads:[~2018-07-29  0:43 UTC|newest]

Thread overview: 26+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-04-18  3:06 [PATCH 0/2] Fix --short and --porcelain options for commit Samuel Lijin
2018-04-18  3:06 ` [PATCH 1/2] commit: fix --short and --porcelain Samuel Lijin
2018-04-18 18:38   ` Martin Ågren
     [not found]     ` <CAJZjrdW3X8eaSit85otKV2HvHmu0NDGcnnnrtxHME03q=eWW-Q@mail.gmail.com>
2018-04-19  3:55       ` Samuel Lijin
2018-04-20  7:08   ` Eric Sunshine
2018-04-18  3:06 ` [PATCH 2/2] wt-status: const-ify all printf helper methods Samuel Lijin
2018-04-26  9:25 ` [PATCH v2 0/2] Fix --short and --porcelain options for commit Samuel Lijin
2018-07-15 11:08   ` [PATCH v3 0/3] Fix --short/--porcelain options for git commit Samuel Lijin
2018-07-23  2:08     ` [PATCH v4 0/4] Rerolling patch series to fix t7501 Samuel Lijin
2018-07-30 22:15       ` Junio C Hamano
2018-07-23  2:09     ` [PATCH v4 1/4] t7501: add coverage for flags which imply dry runs Samuel Lijin
2018-07-23  2:09     ` [PATCH v4 2/4] wt-status: rename commitable to committable Samuel Lijin
2018-07-23  2:09     ` [PATCH v4 3/4] wt-status: teach wt_status_collect about merges in progress Samuel Lijin
2018-07-23  2:09     ` Samuel Lijin [this message]
2018-07-15 11:08   ` [PATCH v3 1/3] t7501: add merge conflict tests for dry run Samuel Lijin
2018-07-17 17:05     ` Junio C Hamano
2018-07-17 17:45       ` Junio C Hamano
2018-07-15 11:08   ` [PATCH v3 2/3] wt-status: teach wt_status_collect about merges in progress Samuel Lijin
2018-07-17 17:15     ` Junio C Hamano
2018-07-15 11:08   ` [PATCH v3 3/3] commit: fix exit code for --short/--porcelain Samuel Lijin
2018-07-17 17:33     ` Junio C Hamano
2018-07-19  9:31       ` Samuel Lijin
2018-04-26  9:25 ` [PATCH v2 1/2] commit: fix --short and --porcelain options Samuel Lijin
2018-05-02  5:50   ` Junio C Hamano
2018-05-02 15:52     ` Samuel Lijin
2018-04-26  9:25 ` [PATCH v2 2/2] wt-status: const-ify all printf helper methods Samuel Lijin

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20180723020903.22435-5-sxlijin@gmail.com \
    --to=sxlijin@gmail.com \
    --cc=git@vger.kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).