[PATCH v2 0/3] wt-status.c: commitable flag

[PATCH v2 0/3] wt-status.c: commitable flag

[Stephen P. Smith]

A couple of years ago, during a patch review Junio found that the
commitable bit as implemented in wt-status.c was broken.

Stephen P. Smith (3):
  Move has_unmerged earlier in the file.
  Add test for commit --dry-run --short.
  wt-status.c: Set the commitable flag in the collect phase.

 t/t7501-commit.sh | 12 ++++++++++--
 wt-status.c       | 39 ++++++++++++++++++++++++---------------
 2 files changed, 34 insertions(+), 17 deletions(-)

-- 
2.18.0

[PATCH v2 1/3] wt-status.c: Move has_unmerged earlier in the file.

[Stephen P. Smith]

Move has_unmerged up in the file so that has_unmerged can be called in
wt_status_collect where we need to place a merge check.

Signed-off-by: Stephen P. Smith <ischis2@cox.net>
---
 wt-status.c | 26 +++++++++++++-------------
 1 file changed, 13 insertions(+), 13 deletions(-)

diff --git a/wt-status.c b/wt-status.c
index 5ffab6101..180faf6ba 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -724,6 +724,19 @@ static void wt_status_collect_untracked(struct wt_status *s)
 		s->untracked_in_ms = (getnanotime() - t_begin) / 1000000;
 }
 
+static int has_unmerged(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;
+}
+
 void wt_status_collect(struct wt_status *s)
 {
 	wt_status_collect_changes_worktree(s);
@@ -1063,19 +1076,6 @@ static void wt_longstatus_print_tracking(struct wt_status *s)
 	strbuf_release(&sb);
 }
 
-static int has_unmerged(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,
 				struct wt_status_state *state,
 				const char *color)
-- 
2.18.0

[PATCH v2 2/3] Add test for commit --dry-run --short.

[Stephen P. Smith]

Add test for commit with --dry-run --short for a new file of zero
length.

The test demonstrates that the setting of the commitable flag is
broken.

Signed-off-by: Stephen P. Smith <ischis2@cox.net>
---
 t/t7501-commit.sh | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/t/t7501-commit.sh b/t/t7501-commit.sh
index 4cae92804..5812dc2f8 100755
--- a/t/t7501-commit.sh
+++ b/t/t7501-commit.sh
@@ -682,4 +682,12 @@ test_expect_success '--dry-run with conflicts fixed from a merge' '
 	git commit -m "conflicts fixed from merge."
 '
 
+test_expect_failure '--dry-run --short' '
+	# setup two branches with conflicting information
+	# in the same file, resolve the conflict
+	>test-file &&
+	git add test-file &&
+	git commit --dry-run --short
+'
+
 test_done
-- 
2.18.0

Re: [PATCH v2 2/3] Add test for commit --dry-run --short.

[Eric Sunshine]

On Sat, Sep 1, 2018 at 7:53 PM Stephen P. Smith <ischis2@cox.net> wrote:
> Add test for commit --dry-run --short.

The first line of a commit message typically mentions the area or
module being touched. It's also customary not to capitalize the first
word and to omit the final period. So, for instance:

    t7501: add test of "--dry-run --short" setting 'committable' flag

> Add test for commit with --dry-run --short for a new file of zero
> length.
>
> The test demonstrates that the setting of the commitable flag is
> broken.

s/commitable/committable/

> Signed-off-by: Stephen P. Smith <ischis2@cox.net>
> ---
> diff --git a/t/t7501-commit.sh b/t/t7501-commit.sh
> @@ -682,4 +682,12 @@ test_expect_success '--dry-run with conflicts fixed from a merge' '
> +test_expect_failure '--dry-run --short' '
> +       # setup two branches with conflicting information
> +       # in the same file, resolve the conflict

What is this comment all about? It doesn't seem to have any relation
to what the test itself is actually doing. (I see that it was copied
from an earlier test in this script, but that doesn't help me
understand what it is trying to say.)

> +       >test-file &&
> +       git add test-file &&
> +       git commit --dry-run --short
> +'

Re: [PATCH v2 2/3] Add test for commit --dry-run --short.

[Eric Sunshine]

On Sat, Sep 1, 2018 at 10:18 PM Eric Sunshine <sunshine@sunshineco.com> wrote:
> On Sat, Sep 1, 2018 at 7:53 PM Stephen P. Smith <ischis2@cox.net> wrote:
> > The test demonstrates that the setting of the commitable flag is
> > broken.
>
> s/commitable/committable/

Looking at patch 3/3, I see that this misspelling exists in the code
itself, so I guess my recommended spelling correction isn't needed. It
might make sense, though, to quote this word in the commit message to
make it more clear that that is the literal spelling in the code. That
is:

   ...demonstrates that the setting of the 'commitable' flag...

(Not itself worth a re-roll.)

Re: [PATCH v2 2/3] Add test for commit --dry-run --short.

[Stephen & Linda Smith]

On Saturday, September 1, 2018 7:18:34 PM MST Eric Sunshine wrote:
> > diff --git a/t/t7501-commit.sh b/t/t7501-commit.sh
> > @@ -682,4 +682,12 @@ test_expect_success '--dry-run with conflicts fixed
> > from a merge' ' +test_expect_failure '--dry-run --short' '
> > +       # setup two branches with conflicting information
> > +       # in the same file, resolve the conflict
> 
> What is this comment all about? It doesn't seem to have any relation
> to what the test itself is actually doing. (I see that it was copied
> from an earlier test in this script, but that doesn't help me
> understand what it is trying to say.)

Agreed.   

I saw your other email about not being worth a re-roll, but I've made the 
change locally in case Junio wants me to do so.  

Additionally if there are other comments I can wrap them into a single set.

[PATCH v2 3/3] wt-status.c: Set the commitable flag in the collect phase.

[Stephen P. Smith]

In an update to fix a bug with "commit --dry-run" it was found that
the commitable flag was broken. The update was, at the time, accepted
as it was better than the previous version. [1]

Since the setting of the commitable flag had been done in
wt_longstatus_print_updated, move it to wt_status_collect_updated_cb.

Set the commitable flag in wt_status_collect_changes_initial to keep
from introducing a rebase regression.

Instead of setting the commitable flag in show_merge_in_progress, in
wt_status_cllect check for a merge that has not been committed. If
present then set the commitable flag.

Change the tests to expect success since updates to the wt-status
broken code section is being fixed.

[1] https://public-inbox.org/git/xmqqr3gcj9i5.fsf@gitster.mtv.corp.google.com/

Signed-off-by: Stephen P. Smith <ischis2@cox.net>
---
 t/t7501-commit.sh |  6 +++---
 wt-status.c       | 13 +++++++++++--
 2 files changed, 14 insertions(+), 5 deletions(-)

diff --git a/t/t7501-commit.sh b/t/t7501-commit.sh
index 5812dc2f8..4cda088cc 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
 '
@@ -682,7 +682,7 @@ test_expect_success '--dry-run with conflicts fixed from a merge' '
 	git commit -m "conflicts fixed from merge."
 '
 
-test_expect_failure '--dry-run --short' '
+test_expect_success '--dry-run --short' '
 	# setup two branches with conflicting information
 	# in the same file, resolve the conflict
 	>test-file &&
diff --git a/wt-status.c b/wt-status.c
index 180faf6ba..578328979 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -540,10 +540,12 @@ static void wt_status_collect_updated_cb(struct diff_queue_struct *q,
 			/* Leave {mode,oid}_head zero for an add. */
 			d->mode_index = p->two->mode;
 			oidcpy(&d->oid_index, &p->two->oid);
+			s->commitable = 1;
 			break;
 		case DIFF_STATUS_DELETED:
 			d->mode_head = p->one->mode;
 			oidcpy(&d->oid_head, &p->one->oid);
+			s->commitable = 1;
 			/* Leave {mode,oid}_index zero for a delete. */
 			break;
 
@@ -561,6 +563,7 @@ static void wt_status_collect_updated_cb(struct diff_queue_struct *q,
 			d->mode_index = p->two->mode;
 			oidcpy(&d->oid_head, &p->one->oid);
 			oidcpy(&d->oid_index, &p->two->oid);
+			s->commitable = 1;
 			break;
 		case DIFF_STATUS_UNMERGED:
 			d->stagemask = unmerged_mask(p->two->path);
@@ -665,11 +668,13 @@ static void wt_status_collect_changes_initial(struct wt_status *s)
 			 * code will output the stage values directly and not use the
 			 * values in these fields.
 			 */
+			s->commitable = 1;
 		} else {
 			d->index_status = DIFF_STATUS_ADDED;
 			/* Leave {mode,oid}_head zero for adds. */
 			d->mode_index = ce->ce_mode;
 			oidcpy(&d->oid_index, &ce->oid);
+			s->commitable = 1;
 		}
 	}
 }
@@ -739,6 +744,7 @@ static int has_unmerged(struct wt_status *s)
 
 void wt_status_collect(struct wt_status *s)
 {
+	struct wt_status_state state;
 	wt_status_collect_changes_worktree(s);
 
 	if (s->is_initial)
@@ -746,6 +752,11 @@ void wt_status_collect(struct wt_status *s)
 	else
 		wt_status_collect_changes_index(s);
 	wt_status_collect_untracked(s);
+
+	memset(&state, 0, sizeof(state));
+	wt_status_get_state(&state, s->branch && !strcmp(s->branch, "HEAD"));
+	if (state.merge_in_progress && !has_unmerged(s))
+		s->commitable = 1;
 }
 
 static void wt_longstatus_print_unmerged(struct wt_status *s)
@@ -786,7 +797,6 @@ static void wt_longstatus_print_updated(struct wt_status *s)
 			continue;
 		if (!shown_header) {
 			wt_longstatus_print_cached_header(s);
-			s->commitable = 1;
 			shown_header = 1;
 		}
 		wt_longstatus_print_change_data(s, WT_STATUS_UPDATED, it);
@@ -1089,7 +1099,6 @@ static void show_merge_in_progress(struct wt_status *s,
 					 _("  (use \"git merge --abort\" to abort the merge)"));
 		}
 	} else {
-		s-> commitable = 1;
 		status_printf_ln(s, color,
 			_("All conflicts fixed but you are still merging."));
 		if (s->hints)
-- 
2.18.0