* [PATCH 0/3] wt-status.c: commitable flag @ 2018-08-31 5:39 Stephen P. Smith 2018-08-31 5:39 ` [PATCH 1/3] Change tests from expecting to fail to expecting success Stephen P. Smith ` (2 more replies) 0 siblings, 3 replies; 12+ messages in thread From: Stephen P. Smith @ 2018-08-31 5:39 UTC (permalink / raw) To: Junio C Hamano; +Cc: Git Mailing List, 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH 1/3] Change tests from expecting to fail to expecting success. 2018-08-31 5:39 [PATCH 0/3] wt-status.c: commitable flag Stephen P. Smith @ 2018-08-31 5:39 ` Stephen P. Smith 2018-08-31 7:17 ` Ævar Arnfjörð Bjarmason 2018-08-31 5:39 ` [PATCH 2/3] Add test for commit --dry-run --short Stephen P. Smith 2018-08-31 5:39 ` [PATCH 3/3] wt-status.c: Set the commitable flag in the collect phase Stephen P. Smith 2 siblings, 1 reply; 12+ messages in thread From: Stephen P. Smith @ 2018-08-31 5:39 UTC (permalink / raw) To: Junio C Hamano; +Cc: Git Mailing List, 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 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH 1/3] Change tests from expecting to fail to expecting success. 2018-08-31 5:39 ` [PATCH 1/3] Change tests from expecting to fail to expecting success Stephen P. Smith @ 2018-08-31 7:17 ` Ævar Arnfjörð Bjarmason 0 siblings, 0 replies; 12+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2018-08-31 7:17 UTC (permalink / raw) To: Stephen P. Smith; +Cc: Junio C Hamano, Git Mailing List 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. ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH 2/3] Add test for commit --dry-run --short. 2018-08-31 5:39 [PATCH 0/3] wt-status.c: commitable flag Stephen P. Smith 2018-08-31 5:39 ` [PATCH 1/3] Change tests from expecting to fail to expecting success Stephen P. Smith @ 2018-08-31 5:39 ` Stephen P. Smith 2018-08-31 7:18 ` Ævar Arnfjörð Bjarmason 2018-09-01 11:55 ` SZEDER Gábor 2018-08-31 5:39 ` [PATCH 3/3] wt-status.c: Set the commitable flag in the collect phase Stephen P. Smith 2 siblings, 2 replies; 12+ messages in thread From: Stephen P. Smith @ 2018-08-31 5:39 UTC (permalink / raw) To: Junio C Hamano; +Cc: Git Mailing List, 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 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH 2/3] Add test for commit --dry-run --short. 2018-08-31 5:39 ` [PATCH 2/3] Add test for commit --dry-run --short Stephen P. Smith @ 2018-08-31 7:18 ` Ævar Arnfjörð Bjarmason 2018-08-31 16:26 ` Junio C Hamano 2018-08-31 17:58 ` Stephen & Linda Smith 2018-09-01 11:55 ` SZEDER Gábor 1 sibling, 2 replies; 12+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2018-08-31 7:18 UTC (permalink / raw) To: Stephen P. Smith; +Cc: Junio C Hamano, Git Mailing List 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. ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 2/3] Add test for commit --dry-run --short. 2018-08-31 7:18 ` Ævar Arnfjörð Bjarmason @ 2018-08-31 16:26 ` Junio C Hamano 2018-08-31 17:58 ` Stephen & Linda Smith 1 sibling, 0 replies; 12+ messages in thread From: Junio C Hamano @ 2018-08-31 16:26 UTC (permalink / raw) To: Ævar Arnfjörð Bjarmason; +Cc: Stephen P. Smith, Git Mailing List Æ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. ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 2/3] Add test for commit --dry-run --short. 2018-08-31 7:18 ` Ævar Arnfjörð Bjarmason 2018-08-31 16:26 ` Junio C Hamano @ 2018-08-31 17:58 ` Stephen & Linda Smith 1 sibling, 0 replies; 12+ messages in thread From: Stephen & Linda Smith @ 2018-08-31 17:58 UTC (permalink / raw) To: Junio C Hamano; +Cc: Ævar Arnfjörð Bjarmason, Git Mailing List 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. ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 2/3] Add test for commit --dry-run --short. 2018-08-31 5:39 ` [PATCH 2/3] Add test for commit --dry-run --short Stephen P. Smith 2018-08-31 7:18 ` Ævar Arnfjörð Bjarmason @ 2018-09-01 11:55 ` SZEDER Gábor 1 sibling, 0 replies; 12+ messages in thread From: SZEDER Gábor @ 2018-09-01 11:55 UTC (permalink / raw) To: Stephen P. Smith; +Cc: Junio C Hamano, Git Mailing List 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 > ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH 3/3] wt-status.c: Set the commitable flag in the collect phase. 2018-08-31 5:39 [PATCH 0/3] wt-status.c: commitable flag Stephen P. Smith 2018-08-31 5:39 ` [PATCH 1/3] Change tests from expecting to fail to expecting success Stephen P. Smith 2018-08-31 5:39 ` [PATCH 2/3] Add test for commit --dry-run --short Stephen P. Smith @ 2018-08-31 5:39 ` Stephen P. Smith 2018-08-31 7:23 ` Ævar Arnfjörð Bjarmason 2 siblings, 1 reply; 12+ messages in thread From: Stephen P. Smith @ 2018-08-31 5:39 UTC (permalink / raw) To: Junio C Hamano; +Cc: Git Mailing List, 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 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH 3/3] wt-status.c: Set the commitable flag in the collect phase. 2018-08-31 5:39 ` [PATCH 3/3] wt-status.c: Set the commitable flag in the collect phase Stephen P. Smith @ 2018-08-31 7:23 ` Ævar Arnfjörð Bjarmason 2018-08-31 16:54 ` Junio C Hamano 2018-08-31 18:13 ` Stephen & Linda Smith 0 siblings, 2 replies; 12+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2018-08-31 7:23 UTC (permalink / raw) To: Stephen P. Smith; +Cc: Junio C Hamano, Git Mailing List 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". ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 3/3] wt-status.c: Set the commitable flag in the collect phase. 2018-08-31 7:23 ` Ævar Arnfjörð Bjarmason @ 2018-08-31 16:54 ` Junio C Hamano 2018-08-31 18:13 ` Stephen & Linda Smith 1 sibling, 0 replies; 12+ messages in thread From: Junio C Hamano @ 2018-08-31 16:54 UTC (permalink / raw) To: Ævar Arnfjörð Bjarmason; +Cc: Stephen P. Smith, Git Mailing List Æ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. ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 3/3] wt-status.c: Set the commitable flag in the collect phase. 2018-08-31 7:23 ` Ævar Arnfjörð Bjarmason 2018-08-31 16:54 ` Junio C Hamano @ 2018-08-31 18:13 ` Stephen & Linda Smith 1 sibling, 0 replies; 12+ messages in thread From: Stephen & Linda Smith @ 2018-08-31 18:13 UTC (permalink / raw) To: Junio C Hamano; +Cc: Ævar Arnfjörð Bjarmason, Git Mailing List 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2018-09-01 11:55 UTC | newest] Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-08-31 5:39 [PATCH 0/3] wt-status.c: commitable flag Stephen P. Smith 2018-08-31 5:39 ` [PATCH 1/3] Change tests from expecting to fail to expecting success Stephen P. Smith 2018-08-31 7:17 ` Ævar Arnfjörð Bjarmason 2018-08-31 5:39 ` [PATCH 2/3] Add test for commit --dry-run --short Stephen P. Smith 2018-08-31 7:18 ` Ævar Arnfjörð Bjarmason 2018-08-31 16:26 ` Junio C Hamano 2018-08-31 17:58 ` Stephen & Linda Smith 2018-09-01 11:55 ` SZEDER Gábor 2018-08-31 5:39 ` [PATCH 3/3] wt-status.c: Set the commitable flag in the collect phase Stephen P. Smith 2018-08-31 7:23 ` Ævar Arnfjörð Bjarmason 2018-08-31 16:54 ` Junio C Hamano 2018-08-31 18:13 ` Stephen & Linda Smith
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).