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

[PATCH 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):
  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

[PATCH 1/3] Change tests from expecting to fail to expecting success.

[Stephen P. Smith]

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 <ischis2@cox.net>
---
 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

Re: [PATCH 1/3] Change tests from expecting to fail to expecting success.

[Ævar Arnfjörð Bjarmason]

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

> 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

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.

[PATCH 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 demonstrated that the setting of the commitable flag was
broken as was found durning an earlier patch review.

Signed-off-by: Stephen P. Smith <ischis2@cox.net>
---
 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

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

[Ævar Arnfjörð Bjarmason]

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

> 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 <ischis2@cox.net>
> ---
>  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

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

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

[Junio C Hamano]

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

> On Fri, Aug 31 2018, Stephen P. Smith wrote:
>
>> 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 <ischis2@cox.net>
>> ---
>>  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
>
> 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.

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

[Stephen & Linda Smith]

On Friday, August 31, 2018 9:26:02 AM MST Junio C Hamano wrote:
> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
> > 
> > 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.

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

[SZEDER Gábor]

On Thu, Aug 30, 2018 at 10:39:20PM -0700, Stephen P. Smith wrote:
> 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

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
> 

[PATCH 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.

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 <ischis2@cox.net>
---
 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

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

[Ævar Arnfjörð Bjarmason]

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.

> 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 <ischis2@cox.net>
> ---
>  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);

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

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

[Junio C Hamano]

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

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

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

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 that is
expeted from the code gets violated if we change the code in
show_merge_in_progress() function.

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;

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.

Thanks for working on this.

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

[Stephen & Linda Smith]

On Friday, August 31, 2018 9:54:50 AM MST Junio C Hamano wrote:
> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
> >> 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