{"thread":{"id":"49263","subject":"[PATCH 0/3] wt-status.c: commitable flag","startedAt":"2018-08-31T05:39:39Z","lastAt":"2018-09-01T11:55:48Z","messageCount":12,"participants":["Stephen P. Smith","Ævar Arnfjörð Bjarmason","Junio C Hamano","Stephen & Linda Smith","SZEDER Gábor"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"357062","messageId":"20180831053921.8083-1-ischis2@cox.net","threadId":"49263","inReplyTo":null,"subject":"[PATCH 0/3] wt-status.c: commitable flag","fromName":"Stephen P. Smith","fromEmail":"ischis2@cox.net","sentAt":"2018-08-31T05:39:18Z","receivedAt":"2018-08-31T05:39:39Z","isPatch":true,"sender":{"key":"ishchis2@gmail.com","avatar":null},"body":"A couple of years ago, during a patch review Junio found that the\ncommitable bit as implemented in wt-status.c was broken.\n\nStephen P. Smith (3):\n Change tests from expecting to fail to expecting success.\n Add test for commit --dry-run --short.\n wt-status.c: Set the commitable flag in the collect phase.\n\n t/t7501-commit.sh | 14 ++++++++++++--\n wt-status.c | 6 +++++-\n 2 files changed, 17 insertions(+), 3 deletions(-)\n\n-- \n2.18.0\n\n"},{"id":"357063","messageId":"20180831053921.8083-2-ischis2@cox.net","threadId":"49263","inReplyTo":"20180831053921.8083-1-ischis2@cox.net","subject":"[PATCH 1/3] Change tests from expecting to fail to expecting success.","fromName":"Stephen P. Smith","fromEmail":"ischis2@cox.net","sentAt":"2018-08-31T05:39:19Z","receivedAt":"2018-08-31T05:39:51Z","isPatch":true,"sender":{"key":"ishchis2@gmail.com","avatar":null},"body":"Two tests were written which showed failure cases when passing\n--procelain or --short.\n\nChange the test to expect success since updates to the wt-status\nbroken code section is being fixed.\n\nSigned-off-by: Stephen P. Smith \n---\n t/t7501-commit.sh | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/t/t7501-commit.sh b/t/t7501-commit.sh\nindex 4cae92804..810d4cea7 100755\n--- a/t/t7501-commit.sh\n+++ b/t/t7501-commit.sh\n@@ -99,12 +99,12 @@ test_expect_success '--dry-run with stuff to commit returns ok' '\n \tgit commit -m next -a --dry-run\n '\n \n-test_expect_failure '--short with stuff to commit returns ok' '\n+test_expect_success '--short with stuff to commit returns ok' '\n \techo bongo bongo bongo >>file &&\n \tgit commit -m next -a --short\n '\n \n-test_expect_failure '--porcelain with stuff to commit returns ok' '\n+test_expect_success '--porcelain with stuff to commit returns ok' '\n \techo bongo bongo bongo >>file &&\n \tgit commit -m next -a --porcelain\n '\n-- \n2.18.0\n\n"},{"id":"357064","messageId":"20180831053921.8083-3-ischis2@cox.net","threadId":"49263","inReplyTo":"20180831053921.8083-1-ischis2@cox.net","subject":"[PATCH 2/3] Add test for commit --dry-run --short.","fromName":"Stephen P. Smith","fromEmail":"ischis2@cox.net","sentAt":"2018-08-31T05:39:20Z","receivedAt":"2018-08-31T05:39:51Z","isPatch":true,"sender":{"key":"ishchis2@gmail.com","avatar":null},"body":"Add test for commit with --dry-run --short for a new file of zero\nlength.\n\nThe test demonstrated that the setting of the commitable flag was\nbroken as was found durning an earlier patch review.\n\nSigned-off-by: Stephen P. Smith \n---\n t/t7501-commit.sh | 10 ++++++++++\n 1 file changed, 10 insertions(+)\n\ndiff --git a/t/t7501-commit.sh b/t/t7501-commit.sh\nindex 810d4cea7..fc69da816 100755\n--- a/t/t7501-commit.sh\n+++ b/t/t7501-commit.sh\n@@ -682,4 +682,14 @@ test_expect_success '--dry-run with conflicts fixed from a merge' '\n \tgit commit -m \"conflicts fixed from merge.\"\n '\n \n+test_expect_success '--dry-run --short with conflicts fixed from a merge' '\n+\t# setup two branches with conflicting information\n+\t# in the same file, resolve the conflict,\n+\t# call commit with --dry-run --short\n+\trm -f test-file &&\n+\ttouch testfile &&\n+\tgit add test-file &&\n+\tgit commit --dry-run --short\n+'\n+\n test_done\n-- \n2.18.0\n\n"},{"id":"357065","messageId":"20180831053921.8083-4-ischis2@cox.net","threadId":"49263","inReplyTo":"20180831053921.8083-1-ischis2@cox.net","subject":"[PATCH 3/3] wt-status.c: Set the commitable flag in the collect phase.","fromName":"Stephen P. Smith","fromEmail":"ischis2@cox.net","sentAt":"2018-08-31T05:39:21Z","receivedAt":"2018-08-31T05:39:51Z","isPatch":true,"sender":{"key":"ishchis2@gmail.com","avatar":null},"body":"In an update to fix a bug with \"commit --dry-run\" it was found that\nthe commitable flag was broken. The update was, at the time,\naccepted as it was better than the previous version.\n\nSince the set of the flag had been done in wt_longstatus_print_updated,\nset the flag in wt_status_collect_updated_cb.\n\nSet the commitable flag in wt_status_collect_changes_initial to keep\nfrom introducing a rebase regression.\n\nLeave the setting of the commitable flag in show_merge_in_progress. If\na check for merged commits is moved to the collect phase then other\n--dry-run tests fail.\n\nSigned-off-by: Stephen P. Smith \n---\n wt-status.c | 6 +++++-\n 1 file changed, 5 insertions(+), 1 deletion(-)\n\ndiff --git a/wt-status.c b/wt-status.c\nindex 5ffab6101..d50798425 100644\n--- a/wt-status.c\n+++ b/wt-status.c\n@@ -540,10 +540,12 @@ static void wt_status_collect_updated_cb(struct diff_queue_struct *q,\n \t\t\t/* Leave {mode,oid}_head zero for an add. */\n \t\t\td->mode_index = p->two->mode;\n \t\t\toidcpy(&d->oid_index, &p->two->oid);\n+\t\t\ts->commitable = 1;\n \t\t\tbreak;\n \t\tcase DIFF_STATUS_DELETED:\n \t\t\td->mode_head = p->one->mode;\n \t\t\toidcpy(&d->oid_head, &p->one->oid);\n+\t\t\ts->commitable = 1;\n \t\t\t/* Leave {mode,oid}_index zero for a delete. */\n \t\t\tbreak;\n \n@@ -561,6 +563,7 @@ static void wt_status_collect_updated_cb(struct diff_queue_struct *q,\n \t\t\td->mode_index = p->two->mode;\n \t\t\toidcpy(&d->oid_head, &p->one->oid);\n \t\t\toidcpy(&d->oid_index, &p->two->oid);\n+\t\t\ts->commitable = 1;\n \t\t\tbreak;\n \t\tcase DIFF_STATUS_UNMERGED:\n \t\t\td->stagemask = unmerged_mask(p->two->path);\n@@ -665,11 +668,13 @@ static void wt_status_collect_changes_initial(struct wt_status *s)\n \t\t\t * code will output the stage values directly and not use the\n \t\t\t * values in these fields.\n \t\t\t */\n+\t\t\ts->commitable = 1;\n \t\t} else {\n \t\t\td->index_status = DIFF_STATUS_ADDED;\n \t\t\t/* Leave {mode,oid}_head zero for adds. */\n \t\t\td->mode_index = ce->ce_mode;\n \t\t\toidcpy(&d->oid_index, &ce->oid);\n+\t\t\ts->commitable = 1;\n \t\t}\n \t}\n }\n@@ -773,7 +778,6 @@ static void wt_longstatus_print_updated(struct wt_status *s)\n \t\t\tcontinue;\n \t\tif (!shown_header) {\n \t\t\twt_longstatus_print_cached_header(s);\n-\t\t\ts->commitable = 1;\n \t\t\tshown_header = 1;\n \t\t}\n \t\twt_longstatus_print_change_data(s, WT_STATUS_UPDATED, it);\n-- \n2.18.0\n\n"},{"id":"357070","messageId":"87d0tzc8e4.fsf@evledraar.gmail.com","threadId":"49263","inReplyTo":"20180831053921.8083-2-ischis2@cox.net","subject":"Re: [PATCH 1/3] Change tests from expecting to fail to expecting success.","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-08-31T07:17:23Z","receivedAt":"2018-08-31T07:17:27Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Fri, Aug 31 2018, Stephen P. Smith wrote:\n\n> diff --git a/t/t7501-commit.sh b/t/t7501-commit.sh\n> index 4cae92804..810d4cea7 100755\n> --- a/t/t7501-commit.sh\n> +++ b/t/t7501-commit.sh\n> @@ -99,12 +99,12 @@ test_expect_success '--dry-run with stuff to commit returns ok' '\n> \tgit commit -m next -a --dry-run\n> '\n>\n> -test_expect_failure '--short with stuff to commit returns ok' '\n> +test_expect_success '--short with stuff to commit returns ok' '\n> \techo bongo bongo bongo >>file &&\n> \tgit commit -m next -a --short\n> '\n>\n> -test_expect_failure '--porcelain with stuff to commit returns ok' '\n> +test_expect_success '--porcelain with stuff to commit returns ok' '\n> \techo bongo bongo bongo >>file &&\n> \tgit commit -m next -a --porcelain\n\nThis commit is not OK and needs to be folded into later commits. It\nmakes the test suite fail until (presumably, haven't reviewed the rest)\na later commit. The tests must always pass, otherwise someone bisecting\nwill trip up over this commit.\n"},{"id":"357071","messageId":"87bm9jc8cq.fsf@evledraar.gmail.com","threadId":"49263","inReplyTo":"20180831053921.8083-3-ischis2@cox.net","subject":"Re: [PATCH 2/3] Add test for commit --dry-run --short.","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-08-31T07:18:13Z","receivedAt":"2018-08-31T07:18:18Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Fri, Aug 31 2018, Stephen P. Smith wrote:\n\n> Add test for commit with --dry-run --short for a new file of zero\n> length.\n>\n> The test demonstrated that the setting of the commitable flag was\n> broken as was found durning an earlier patch review.\n>\n> Signed-off-by: Stephen P. Smith \n> ---\n> t/t7501-commit.sh | 10 ++++++++++\n> 1 file changed, 10 insertions(+)\n>\n> diff --git a/t/t7501-commit.sh b/t/t7501-commit.sh\n> index 810d4cea7..fc69da816 100755\n> --- a/t/t7501-commit.sh\n> +++ b/t/t7501-commit.sh\n> @@ -682,4 +682,14 @@ test_expect_success '--dry-run with conflicts fixed from a merge' '\n> \tgit commit -m \"conflicts fixed from merge.\"\n> '\n>\n> +test_expect_success '--dry-run --short with conflicts fixed from a merge' '\n> +\t# setup two branches with conflicting information\n> +\t# in the same file, resolve the conflict,\n> +\t# call commit with --dry-run --short\n> +\trm -f test-file &&\n> +\ttouch testfile &&\n> +\tgit add test-file &&\n> +\tgit commit --dry-run --short\n> +'\n> +\n> test_done\n\nDitto my comment on 1/3 on this. I.e. this changes the failing tests in\nthis series from 2 to 3.\n"},{"id":"357072","messageId":"87a7p3c83g.fsf@evledraar.gmail.com","threadId":"49263","inReplyTo":"20180831053921.8083-4-ischis2@cox.net","subject":"Re: [PATCH 3/3] wt-status.c: Set the commitable flag in the collect phase.","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-08-31T07:23:47Z","receivedAt":"2018-08-31T07:23:52Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Fri, Aug 31 2018, Stephen P. Smith wrote:\n\n> In an update to fix a bug with \"commit --dry-run\" it was found that\n> the commitable flag was broken. The update was, at the time,\n> accepted as it was better than the previous version.\n\nWhat update is this? I.e. git.git commit id? See the \"or this invocation\nof `git show`\" part of SubmittingPatches for how to quote it in the\ncommit message.\n\n> Since the set of the flag had been done in wt_longstatus_print_updated,\n> set the flag in wt_status_collect_updated_cb.\n>\n> Set the commitable flag in wt_status_collect_changes_initial to keep\n> from introducing a rebase regression.\n>\n> Leave the setting of the commitable flag in show_merge_in_progress. If\n> a check for merged commits is moved to the collect phase then other\n> --dry-run tests fail.\n>\n> Signed-off-by: Stephen P. Smith \n> ---\n> wt-status.c | 6 +++++-\n> 1 file changed, 5 insertions(+), 1 deletion(-)\n>\n> diff --git a/wt-status.c b/wt-status.c\n> index 5ffab6101..d50798425 100644\n> --- a/wt-status.c\n> +++ b/wt-status.c\n> @@ -540,10 +540,12 @@ static void wt_status_collect_updated_cb(struct diff_queue_struct *q,\n> \t\t\t/* Leave {mode,oid}_head zero for an add. */\n> \t\t\td->mode_index = p->two->mode;\n> \t\t\toidcpy(&d->oid_index, &p->two->oid);\n> +\t\t\ts->commitable = 1;\n> \t\t\tbreak;\n> \t\tcase DIFF_STATUS_DELETED:\n> \t\t\td->mode_head = p->one->mode;\n> \t\t\toidcpy(&d->oid_head, &p->one->oid);\n> +\t\t\ts->commitable = 1;\n> \t\t\t/* Leave {mode,oid}_index zero for a delete. */\n> \t\t\tbreak;\n>\n> @@ -561,6 +563,7 @@ static void wt_status_collect_updated_cb(struct diff_queue_struct *q,\n> \t\t\td->mode_index = p->two->mode;\n> \t\t\toidcpy(&d->oid_head, &p->one->oid);\n> \t\t\toidcpy(&d->oid_index, &p->two->oid);\n> +\t\t\ts->commitable = 1;\n> \t\t\tbreak;\n> \t\tcase DIFF_STATUS_UNMERGED:\n> \t\t\td->stagemask = unmerged_mask(p->two->path);\n> @@ -665,11 +668,13 @@ static void wt_status_collect_changes_initial(struct wt_status *s)\n> \t\t\t * code will output the stage values directly and not use the\n> \t\t\t * values in these fields.\n> \t\t\t */\n> +\t\t\ts->commitable = 1;\n> \t\t} else {\n> \t\t\td->index_status = DIFF_STATUS_ADDED;\n> \t\t\t/* Leave {mode,oid}_head zero for adds. */\n> \t\t\td->mode_index = ce->ce_mode;\n> \t\t\toidcpy(&d->oid_index, &ce->oid);\n> +\t\t\ts->commitable = 1;\n> \t\t}\n> \t}\n> }\n> @@ -773,7 +778,6 @@ static void wt_longstatus_print_updated(struct wt_status *s)\n> \t\t\tcontinue;\n> \t\tif (!shown_header) {\n> \t\t\twt_longstatus_print_cached_header(s);\n> -\t\t\ts->commitable = 1;\n> \t\t\tshown_header = 1;\n> \t\t}\n> \t\twt_longstatus_print_change_data(s, WT_STATUS_UPDATED, it);\n\nThis looks sensible, but I'm not familiar with the status code.\n\nStructurally, re: my comment on 1/3 and 2/3, it would make sense to make\nthis a two-part series. In 1/2 you add the test you're adding in 2/3 as\na test_expect_failure test, and in 2/2 (this commit) you tweak all the\n\"test_expect_failure\" that now pass to \"test_expect_success\".\n"},{"id":"357090","messageId":"xmqq8t4m33l1.fsf@gitster-ct.c.googlers.com","threadId":"49263","inReplyTo":"87bm9jc8cq.fsf@evledraar.gmail.com","subject":"Re: [PATCH 2/3] Add test for commit --dry-run --short.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-08-31T16:26:02Z","receivedAt":"2018-08-31T16:26:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> On Fri, Aug 31 2018, Stephen P. Smith wrote:\n>\n>> Add test for commit with --dry-run --short for a new file of zero\n>> length.\n>>\n>> The test demonstrated that the setting of the commitable flag was\n>> broken as was found durning an earlier patch review.\n>>\n>> Signed-off-by: Stephen P. Smith \n>> ---\n>> t/t7501-commit.sh | 10 ++++++++++\n>> 1 file changed, 10 insertions(+)\n>>\n>> diff --git a/t/t7501-commit.sh b/t/t7501-commit.sh\n>> index 810d4cea7..fc69da816 100755\n>> --- a/t/t7501-commit.sh\n>> +++ b/t/t7501-commit.sh\n>> @@ -682,4 +682,14 @@ test_expect_success '--dry-run with conflicts fixed from a merge' '\n>> \tgit commit -m \"conflicts fixed from merge.\"\n>> '\n>>\n>> +test_expect_success '--dry-run --short with conflicts fixed from a merge' '\n>> +\t# setup two branches with conflicting information\n>> +\t# in the same file, resolve the conflict,\n>> +\t# call commit with --dry-run --short\n>> +\trm -f test-file &&\n>> +\ttouch testfile &&\n>> +\tgit add test-file &&\n>> +\tgit commit --dry-run --short\n>> +'\n>> +\n>> test_done\n>\n> Ditto my comment on 1/3 on this. I.e. this changes the failing tests in\n> this series from 2 to 3.\n\nCorrect. Thanks for helping Stephen on this topic.\n"},{"id":"357101","messageId":"xmqq4lfa3291.fsf@gitster-ct.c.googlers.com","threadId":"49263","inReplyTo":"87a7p3c83g.fsf@evledraar.gmail.com","subject":"Re: [PATCH 3/3] wt-status.c: Set the commitable flag in the collect phase.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-08-31T16:54:50Z","receivedAt":"2018-08-31T16:54:55Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> On Fri, Aug 31 2018, Stephen P. Smith wrote:\n>\n>> In an update to fix a bug with \"commit --dry-run\" it was found that\n>> the commitable flag was broken. The update was, at the time,\n>> accepted as it was better than the previous version.\n>\n> What update is this? I.e. git.git commit id? See the \"or this invocation\n> of `git show`\" part of SubmittingPatches for how to quote it in the\n> commit message.\n>\n>> Since the set of the flag had been done in wt_longstatus_print_updated,\n>> set the flag in wt_status_collect_updated_cb.\n>>\n>> Set the commitable flag in wt_status_collect_changes_initial to keep\n>> from introducing a rebase regression.\n>>\n>> Leave the setting of the commitable flag in show_merge_in_progress. If\n>> a check for merged commits is moved to the collect phase then other\n>> --dry-run tests fail.\n\n\"Some tests fail\" is not a good explanation why you keep the setting\nof commitable bit in the \"show\" codepath. The test coverage may not\nbe thorough, and the tests that fail might be expecting wrong things.\n\nThe change in this patch makes the internal \"diff-index\" invocation\nresponsible for setting the commitable bit. This is better for non\nmerge commits than the current code because the current code only\nsets the commitable bit when longstatus is asked for (and the code\nto show the longstatus detects changes to be committed), so the\nshort-form will not have chance to set the bit, but the internal\n\"diff-index\" is what determines if the resulting commit would have\ndifference relative to the HEAD, so it is a better place to make\nthat decision.\n\nMerge commits need to be allowed even when the resulting commit\nrecords a tree that is identical to that of the current HEAD\n(i.e. we merged a side branch, but we already had all the necessary\nchanges done on it). So it is insufficient to let \"diff-index\"\ninvocation to set the committable bit. Is that why \"other --dry-run\ntests fail\"? What I am getting at is to have a reasonable \"because\n...\" to explain why \"other --dry-run tests fail\" after it, to make\nit clear to the readers that the failure is not because tests are\nchecking wrong things but because a specific condition that is\nexpeted from the code gets violated if we change the code in\nshow_merge_in_progress() function.\n\nThat brings us to another point. Is there a case where we want to\nsee s->commitable bit set correctly but we do not make any call to\nshow_merge_in_progress() function? It is conceivable to have a new\n\"git commit --dry-run --quiet [[--] ]\" mode that is\ntotally silent but reports if what we have is committable with the\nexit status, and for that we would not want to call any show_*\nfunctions. That leads me to suspect that ideally we would want to\nsee wt_status_collect_changes_index() to be the one that is setting\nthe commitable bit. Or even its caller wt_status_collect(), which\nwould give us a better chance of being correct even during the\ninitial commit. For the \"during merge\" case, we would need to say\nsomething like\n\n\tif (state->merge_in_progress && !has_unmerged(s))\n\t\ts->commitable = 1;\n\nbut the \"state\" thing is passed around only among the \"print/show\"\nlevel of functions in the current code. We might need to merge that\ninto the wt_status structure to pass it down to the \"collect\" phase\nat the lower level before/while doing so. I dunno.\n\nThanks for working on this.\n\n\n\n\n"},{"id":"357105","messageId":"3509994.v5Zsi5rRYD@thunderbird","threadId":"49263","inReplyTo":"87bm9jc8cq.fsf@evledraar.gmail.com","subject":"Re: [PATCH 2/3] Add test for commit --dry-run --short.","fromName":"Stephen & Linda Smith","fromEmail":"ischis2@cox.net","sentAt":"2018-08-31T17:58:48Z","receivedAt":"2018-08-31T17:58:54Z","isPatch":true,"sender":{"key":"ishchis2@gmail.com","avatar":null},"body":"On Friday, August 31, 2018 9:26:02 AM MST Junio C Hamano wrote:\n> Ævar Arnfjörð Bjarmason writes:\n> > \n> > Ditto my comment on 1/3 on this. I.e. this changes the failing tests in\n> > this series from 2 to 3.\n> \n> Correct. Thanks for helping Stephen on this topic.\n\nI wasn't sure how this situation was normally handled. I will update when I \nre-roll changes for wt-status.c.\n\n\n\n\n"},{"id":"357106","messageId":"1863304.0psXOz24iy@thunderbird","threadId":"49263","inReplyTo":"87a7p3c83g.fsf@evledraar.gmail.com","subject":"Re: [PATCH 3/3] wt-status.c: Set the commitable flag in the collect phase.","fromName":"Stephen & Linda Smith","fromEmail":"ischis2@cox.net","sentAt":"2018-08-31T18:13:24Z","receivedAt":"2018-08-31T18:13:28Z","isPatch":true,"sender":{"key":"ishchis2@gmail.com","avatar":null},"body":"On Friday, August 31, 2018 9:54:50 AM MST Junio C Hamano wrote:\n> Ævar Arnfjörð Bjarmason writes:\n> >> Leave the setting of the commitable flag in show_merge_in_progress. If\n> >> a check for merged commits is moved to the collect phase then other\n> >> --dry-run tests fail.\n> \n> \"Some tests fail\" is not a good explanation why you keep the setting\n> of commitable bit in the \"show\" codepath. The test coverage may not\n> be thorough, and the tests that fail might be expecting wrong things.\n> \nI didn't figure it was, but I haven't yet figured out how to explain what what \nI saw last evening. I wanted to send something out to get feedback from \nsomeone who may know the code far better than I.\n\n> The change in this patch makes the internal \"diff-index\" invocation\n> responsible for setting the commitable bit. This is better for non\n> merge commits than the current code because the current code only\n> sets the commitable bit when longstatus is asked for (and the code\n> to show the longstatus detects changes to be committed), so the\n> short-form will not have chance to set the bit, but the internal\n> \"diff-index\" is what determines if the resulting commit would have\n> difference relative to the HEAD, so it is a better place to make\n> that decision.\n> \n> Merge commits need to be allowed even when the resulting commit\n> records a tree that is identical to that of the current HEAD\n> (i.e. we merged a side branch, but we already had all the necessary\n> changes done on it). So it is insufficient to let \"diff-index\"\n> invocation to set the committable bit. Is that why \"other --dry-run\n> tests fail\"? What I am getting at is to have a reasonable \"because\n> ...\" to explain why \"other --dry-run tests fail\" after it, to make\n> it clear to the readers that the failure is not because tests are\n> checking wrong things but because a specific condition \nthatwt_status_collect(), isYes\n> expeted from the code gets violated if we change the code in\n> show_merge_in_progress() function.\nAgreed. I'm just green at this code base, and so don't quite know what I \nshould see as opposed to what I do see.\n\n> \n> That brings us to another point. Is there a case where we want to\n> see s->commitable bit set correctly but we do not make any call to\n> show_merge_in_progress() function? It is conceivable to have a new\n> \"git commit --dry-run --quiet [[--] ]\" mode that is\n> totally silent but reports if what we have is committable with the\n> exit status, and for that we would not want to call any show_*\n> functions. That leads me to suspect that ideally we would want to\n> see wt_status_collect_changes_index() to be the one that is setting\n> the commitable bit. Or even its caller wt_status_collect(), which\n> would give us a better chance of being correct even during the\n> initial commit. For the \"during merge\" case, we would need to say\n> something like\n> \n> \tif (state->merge_in_progress && !has_unmerged(s))\n> \t\ts->commitable = 1;\n> \nI placed the following in wt_status_collect() last evening, and received \nerrors from three early tests in 7501-commit.sh. Thanks for a hint.\n\n\tif (!has_unmerged(s))\n\t\ts->commitable = 1;\n\nMaybe the missing first condition was what I needed.\n\nWhich leads me to asking: Do you want a preparatory patch moving \nhas_unmerged() further up in the file before adding a reference to \nhas_unmerged() in wt_status_collect(). \n\n> but the \"state\" thing is passed around only among the \"print/show\"\n> level of functions in the current code. We might need to merge that\n> into the wt_status structure to pass it down to the \"collect\" phase\n> at the lower level before/while doing so. I dunno.\nWould you explain what you are thinking for passing moving the \"stat\" think \ninto wt_status. I haven't figured out how the \"collect\" sequence, relates \nto the \"print/show\" squence.\n\n> \n> Thanks for working on this.\nI decided sometime back to work on something I didn't know using a process I \ndon't normally use to broaden my experience. I'm enjoying this and hope you \ndon't mind lots of questions from someone new.\n\nsps\n\n\n\n"},{"id":"357157","messageId":"20180901115521.GA18536@localhost","threadId":"49263","inReplyTo":"20180831053921.8083-3-ischis2@cox.net","subject":"Re: [PATCH 2/3] Add test for commit --dry-run --short.","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2018-09-01T11:55:21Z","receivedAt":"2018-09-01T11:55:48Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Thu, Aug 30, 2018 at 10:39:20PM -0700, Stephen P. Smith wrote:\n> diff --git a/t/t7501-commit.sh b/t/t7501-commit.sh\n> index 810d4cea7..fc69da816 100755\n> --- a/t/t7501-commit.sh\n> +++ b/t/t7501-commit.sh\n> @@ -682,4 +682,14 @@ test_expect_success '--dry-run with conflicts fixed from a merge' '\n> \tgit commit -m \"conflicts fixed from merge.\"\n> '\n> \n> +test_expect_success '--dry-run --short with conflicts fixed from a merge' '\n> +\t# setup two branches with conflicting information\n> +\t# in the same file, resolve the conflict,\n> +\t# call commit with --dry-run --short\n\nI think the last line of the comment is unnecessary: it doesn't say\nanything that is not obvious from the test's last line.\n\n> +\trm -f test-file &&\n> +\ttouch testfile &&\n\nThat filename should be 'test-file', i.e. with a dash, shouldn't it?\n\nAnyway, if you want to truncate the file, then please use '>test-file'\ninstead of 'rm' and 'touch'.\n\n> +\tgit add test-file &&\n> +\tgit commit --dry-run --short\n> +'\n> +\n> test_done\n> -- \n> 2.18.0\n> \n"}]}