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

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):
  Change tests from expecting to fail to expecting success.
  Add test for commit --dry-run --short.
  wt-status.c: Set the commitable flag in the collect phase.

 t/t7501-commit.sh | 14 ++++++++++++--
 wt-status.c       |  6 +++++-
 2 files changed, 17 insertions(+), 3 deletions(-)

-- 
2.18.0

Two tests were written which showed failure cases when passing
--procelain or --short.

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

Signed-off-by: Stephen P. Smith <redacted>
---
 t/t7501-commit.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/t/t7501-commit.sh b/t/t7501-commit.sh
index 4cae92804..810d4cea7 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
 '

-- 
2.18.0

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

The test demonstrated that the setting of the commitable flag was
broken as was found durning an earlier patch review.

Signed-off-by: Stephen P. Smith <redacted>
---
 t/t7501-commit.sh | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/t/t7501-commit.sh b/t/t7501-commit.sh
index 810d4cea7..fc69da816 100755
--- a/t/t7501-commit.sh
+++ b/t/t7501-commit.sh

@@ -682,4 +682,14 @@ test_expect_success '--dry-run with conflicts fixed from a merge' '
 	git commit -m "conflicts fixed from merge."
 '
 
+test_expect_success '--dry-run --short with conflicts fixed from a merge' '
+	# setup two branches with conflicting information
+	# in the same file, resolve the conflict,
+	# call commit with --dry-run --short
+	rm -f test-file &&
+	touch testfile &&
+	git add test-file &&
+	git commit --dry-run --short
+'
+
 test_done

-- 
2.18.0

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.

Since the set of the flag had been done in wt_longstatus_print_updated,
set the flag in wt_status_collect_updated_cb.

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

Leave the setting of the commitable flag in show_merge_in_progress. If
a check for merged commits is moved to the collect phase then other
--dry-run tests fail.

Signed-off-by: Stephen P. Smith <redacted>
---
 wt-status.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/wt-status.c b/wt-status.c
index 5ffab6101..d50798425 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;
 		}
 	}
 }

@@ -773,7 +778,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);

-- 
2.18.0

On Fri, Aug 31 2018, Stephen P. Smith wrote:

This commit is not OK and needs to be folded into later commits. It
makes the test suite fail until (presumably, haven't reviewed the rest)
a later commit. The tests must always pass, otherwise someone bisecting
will trip up over this commit.

On Fri, Aug 31 2018, Stephen P. Smith wrote:

Ditto my comment on 1/3 on this. I.e. this changes the failing tests in
this series from 2 to 3.

On Fri, Aug 31 2018, Stephen P. Smith wrote:

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.

What update is this? I.e. git.git commit id? See the "or this invocation
of `git show`" part of SubmittingPatches for how to quote it in the
commit message.

This looks sensible, but I'm not familiar with the status code.

Structurally, re: my comment on 1/3 and 2/3, it would make sense to make
this a two-part series. In 1/2 you add the test you're adding in 2/3 as
a test_expect_failure test, and in 2/2 (this commit) you tweak all the
"test_expect_failure" that now pass to "test_expect_success".

On Friday, August 31, 2018 9:26:02 AM MST Junio C Hamano wrote:

Ævar Arnfjörð Bjarmason [off-list ref] writes:

quoted

Ditto my comment on 1/3 on this. I.e. this changes the failing tests in
this series from 2 to 3.

Correct.  Thanks for helping Stephen on this topic.

I wasn't sure how this situation was normally handled.  I will update when I 
re-roll changes for wt-status.c.

On Friday, August 31, 2018 9:54:50 AM MST Junio C Hamano wrote:

Ævar Arnfjörð Bjarmason [off-list ref] writes:

quoted

quoted

Leave the setting of the commitable flag in show_merge_in_progress. If
a check for merged commits is moved to the collect phase then other
--dry-run tests fail.

"Some tests fail" is not a good explanation why you keep the setting
of commitable bit in the "show" codepath.  The test coverage may not
be thorough, and the tests that fail might be expecting wrong things.

I didn't figure it was, but I haven't yet figured out how to explain what what 
I saw last evening.   I wanted to send something out to get feedback from 
someone who may know the code far better than I.

The change in this patch makes the internal "diff-index" invocation
responsible for setting the commitable bit.  This is better for non
merge commits than the current code because the current code only
sets the commitable bit when longstatus is asked for (and the code
to show the longstatus detects changes to be committed), so the
short-form will not have chance to set the bit, but the internal
"diff-index" is what determines if the resulting commit would have
difference relative to the HEAD, so it is a better place to make
that decision.

Merge commits need to be allowed even when the resulting commit
records a tree that is identical to that of the current HEAD
(i.e. we merged a side branch, but we already had all the necessary
changes done on it).  So it is insufficient to let "diff-index"
invocation to set the committable bit.  Is that why "other --dry-run
tests fail"?  What I am getting at is to have a reasonable "because
..."  to explain why "other --dry-run tests fail" after it, to make
it clear to the readers that the failure is not because tests are
checking wrong things but because a specific condition 

thatwt_status_collect(), isYes

expeted from the code gets violated if we change the code in
show_merge_in_progress() function.

Agreed.  I'm just green at this code base, and so don't quite know what I 
should see as opposed to what I do see.

That brings us to another point.  Is there a case where we want to
see s->commitable bit set correctly but we do not make any call to
show_merge_in_progress() function?  It is conceivable to have a new
"git commit --dry-run --quiet [[--] <pathspec>]" mode that is
totally silent but reports if what we have is committable with the
exit status, and for that we would not want to call any show_*
functions.  That leads me to suspect that ideally we would want to
see wt_status_collect_changes_index() to be the one that is setting
the commitable bit.  Or even its caller wt_status_collect(), which
would give us a better chance of being correct even during the
initial commit.  For the "during merge" case, we would need to say
something like

	if (state->merge_in_progress && !has_unmerged(s))
		s->commitable = 1;

I placed the following  in wt_status_collect() last evening, and received 
errors from three early tests in 7501-commit.sh.   Thanks for a hint.

	if (!has_unmerged(s))
		s->commitable = 1;

Maybe the missing first condition was what I needed.

Which leads me to asking:  Do you want a preparatory patch moving 
has_unmerged() further up in the file before adding a reference to 
has_unmerged() in wt_status_collect().  

but the "state" thing is passed around only among the "print/show"
level of functions in the current code.  We might need to merge that
into the wt_status structure to pass it down to the "collect" phase
at the lower level before/while doing so.  I dunno.

Would you explain what you  are thinking for passing moving the "stat" think 
into wt_status.    I haven't figured out how the "collect" sequence, relates 
to the "print/show" squence.

Thanks for working on this.

I decided sometime back to work on something I didn't know using a process I 
don't normally use to broaden my experience.   I'm enjoying this and hope you 
don't mind lots of questions from someone new.

sps

On Thu, Aug 30, 2018 at 10:39:20PM -0700, Stephen P. Smith wrote:

I think the last line of the comment is unnecessary: it doesn't say
anything that is not obvious from the test's last line.

+	rm -f test-file &&
+	touch testfile &&

That filename should be 'test-file', i.e. with a dash, shouldn't it?

Anyway, if you want to truncate the file, then please use '>test-file'
instead of 'rm' and 'touch'.

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