{"thread":{"id":"49267","subject":"[PATCH v2 0/3] wt-status.c: commitable flag","startedAt":"2018-09-01T23:53:01Z","lastAt":"2018-09-02T05:17:47Z","messageCount":7,"participants":["Stephen P. Smith","Eric Sunshine","Stephen & Linda Smith"],"isPatch":true,"patchVersion":2,"patchTotal":3},"messages":[{"id":"357167","messageId":"20180901235256.4260-1-ischis2@cox.net","threadId":"49267","inReplyTo":null,"subject":"[PATCH v2 0/3] wt-status.c: commitable flag","fromName":"Stephen P. Smith","fromEmail":"ischis2@cox.net","sentAt":"2018-09-01T23:52:53Z","receivedAt":"2018-09-01T23:53:01Z","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  Move has_unmerged earlier in the file.\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 | 12 ++++++++++--\n wt-status.c       | 39 ++++++++++++++++++++++++---------------\n 2 files changed, 34 insertions(+), 17 deletions(-)\n\n-- \n2.18.0\n\n"},{"id":"357168","messageId":"20180901235256.4260-2-ischis2@cox.net","threadId":"49267","inReplyTo":"20180901235256.4260-1-ischis2@cox.net","subject":"[PATCH v2 1/3] wt-status.c: Move has_unmerged earlier in the file.","fromName":"Stephen P. Smith","fromEmail":"ischis2@cox.net","sentAt":"2018-09-01T23:52:54Z","receivedAt":"2018-09-01T23:54:17Z","isPatch":true,"sender":{"key":"ishchis2@gmail.com","avatar":null},"body":"Move has_unmerged up in the file so that has_unmerged can be called in\nwt_status_collect where we need to place a merge check.\n\nSigned-off-by: Stephen P. Smith <ischis2@cox.net>\n---\n wt-status.c | 26 +++++++++++++-------------\n 1 file changed, 13 insertions(+), 13 deletions(-)\n\ndiff --git a/wt-status.c b/wt-status.c\nindex 5ffab6101..180faf6ba 100644\n--- a/wt-status.c\n+++ b/wt-status.c\n@@ -724,6 +724,19 @@ static void wt_status_collect_untracked(struct wt_status *s)\n \t\ts->untracked_in_ms = (getnanotime() - t_begin) / 1000000;\n }\n \n+static int has_unmerged(struct wt_status *s)\n+{\n+\tint i;\n+\n+\tfor (i = 0; i < s->change.nr; i++) {\n+\t\tstruct wt_status_change_data *d;\n+\t\td = s->change.items[i].util;\n+\t\tif (d->stagemask)\n+\t\t\treturn 1;\n+\t}\n+\treturn 0;\n+}\n+\n void wt_status_collect(struct wt_status *s)\n {\n \twt_status_collect_changes_worktree(s);\n@@ -1063,19 +1076,6 @@ static void wt_longstatus_print_tracking(struct wt_status *s)\n \tstrbuf_release(&sb);\n }\n \n-static int has_unmerged(struct wt_status *s)\n-{\n-\tint i;\n-\n-\tfor (i = 0; i < s->change.nr; i++) {\n-\t\tstruct wt_status_change_data *d;\n-\t\td = s->change.items[i].util;\n-\t\tif (d->stagemask)\n-\t\t\treturn 1;\n-\t}\n-\treturn 0;\n-}\n-\n static void show_merge_in_progress(struct wt_status *s,\n \t\t\t\tstruct wt_status_state *state,\n \t\t\t\tconst char *color)\n-- \n2.18.0\n\n"},{"id":"357169","messageId":"20180901235256.4260-3-ischis2@cox.net","threadId":"49267","inReplyTo":"20180901235256.4260-1-ischis2@cox.net","subject":"[PATCH v2 2/3] Add test for commit --dry-run --short.","fromName":"Stephen P. Smith","fromEmail":"ischis2@cox.net","sentAt":"2018-09-01T23:52:55Z","receivedAt":"2018-09-01T23:54:17Z","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 demonstrates that the setting of the commitable flag is\nbroken.\n\nSigned-off-by: Stephen P. Smith <ischis2@cox.net>\n---\n t/t7501-commit.sh | 8 ++++++++\n 1 file changed, 8 insertions(+)\n\ndiff --git a/t/t7501-commit.sh b/t/t7501-commit.sh\nindex 4cae92804..5812dc2f8 100755\n--- a/t/t7501-commit.sh\n+++ b/t/t7501-commit.sh\n@@ -682,4 +682,12 @@ test_expect_success '--dry-run with conflicts fixed from a merge' '\n \tgit commit -m \"conflicts fixed from merge.\"\n '\n \n+test_expect_failure '--dry-run --short' '\n+\t# setup two branches with conflicting information\n+\t# in the same file, resolve the conflict\n+\t>test-file &&\n+\tgit add test-file &&\n+\tgit commit --dry-run --short\n+'\n+\n test_done\n-- \n2.18.0\n\n"},{"id":"357170","messageId":"20180901235256.4260-4-ischis2@cox.net","threadId":"49267","inReplyTo":"20180901235256.4260-1-ischis2@cox.net","subject":"[PATCH v2 3/3] wt-status.c: Set the commitable flag in the collect phase.","fromName":"Stephen P. Smith","fromEmail":"ischis2@cox.net","sentAt":"2018-09-01T23:52:56Z","receivedAt":"2018-09-01T23:54:17Z","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, accepted\nas it was better than the previous version. [1]\n\nSince the setting of the commitable flag had been done in\nwt_longstatus_print_updated, move it to wt_status_collect_updated_cb.\n\nSet the commitable flag in wt_status_collect_changes_initial to keep\nfrom introducing a rebase regression.\n\nInstead of setting the commitable flag in show_merge_in_progress, in\nwt_status_cllect check for a merge that has not been committed. If\npresent then set the commitable flag.\n\nChange the tests to expect success since updates to the wt-status\nbroken code section is being fixed.\n\n[1] https://public-inbox.org/git/xmqqr3gcj9i5.fsf@gitster.mtv.corp.google.com/\n\nSigned-off-by: Stephen P. Smith <ischis2@cox.net>\n---\n t/t7501-commit.sh |  6 +++---\n wt-status.c       | 13 +++++++++++--\n 2 files changed, 14 insertions(+), 5 deletions(-)\n\ndiff --git a/t/t7501-commit.sh b/t/t7501-commit.sh\nindex 5812dc2f8..4cda088cc 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@@ -682,7 +682,7 @@ test_expect_success '--dry-run with conflicts fixed from a merge' '\n \tgit commit -m \"conflicts fixed from merge.\"\n '\n \n-test_expect_failure '--dry-run --short' '\n+test_expect_success '--dry-run --short' '\n \t# setup two branches with conflicting information\n \t# in the same file, resolve the conflict\n \t>test-file &&\ndiff --git a/wt-status.c b/wt-status.c\nindex 180faf6ba..578328979 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@@ -739,6 +744,7 @@ static int has_unmerged(struct wt_status *s)\n \n void wt_status_collect(struct wt_status *s)\n {\n+\tstruct wt_status_state state;\n \twt_status_collect_changes_worktree(s);\n \n \tif (s->is_initial)\n@@ -746,6 +752,11 @@ void wt_status_collect(struct wt_status *s)\n \telse\n \t\twt_status_collect_changes_index(s);\n \twt_status_collect_untracked(s);\n+\n+\tmemset(&state, 0, sizeof(state));\n+\twt_status_get_state(&state, s->branch && !strcmp(s->branch, \"HEAD\"));\n+\tif (state.merge_in_progress && !has_unmerged(s))\n+\t\ts->commitable = 1;\n }\n \n static void wt_longstatus_print_unmerged(struct wt_status *s)\n@@ -786,7 +797,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@@ -1089,7 +1099,6 @@ static void show_merge_in_progress(struct wt_status *s,\n \t\t\t\t\t _(\"  (use \\\"git merge --abort\\\" to abort the merge)\"));\n \t\t}\n \t} else {\n-\t\ts-> commitable = 1;\n \t\tstatus_printf_ln(s, color,\n \t\t\t_(\"All conflicts fixed but you are still merging.\"));\n \t\tif (s->hints)\n-- \n2.18.0\n\n"},{"id":"357172","messageId":"CAPig+cTzBTA4hSkM9ZtRVARZEwXMv1o9GnPMrBGimt-g=ExmHw@mail.gmail.com","threadId":"49267","inReplyTo":"20180901235256.4260-3-ischis2@cox.net","subject":"Re: [PATCH v2 2/3] Add test for commit --dry-run --short.","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2018-09-02T02:18:34Z","receivedAt":"2018-09-02T02:18:47Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sat, Sep 1, 2018 at 7:53 PM Stephen P. Smith <ischis2@cox.net> wrote:\n> Add test for commit --dry-run --short.\n\nThe first line of a commit message typically mentions the area or\nmodule being touched. It's also customary not to capitalize the first\nword and to omit the final period. So, for instance:\n\n    t7501: add test of \"--dry-run --short\" setting 'committable' flag\n\n> Add test for commit with --dry-run --short for a new file of zero\n> length.\n>\n> The test demonstrates that the setting of the commitable flag is\n> broken.\n\ns/commitable/committable/\n\n> Signed-off-by: Stephen P. Smith <ischis2@cox.net>\n> ---\n> diff --git a/t/t7501-commit.sh b/t/t7501-commit.sh\n> @@ -682,4 +682,12 @@ test_expect_success '--dry-run with conflicts fixed from a merge' '\n> +test_expect_failure '--dry-run --short' '\n> +       # setup two branches with conflicting information\n> +       # in the same file, resolve the conflict\n\nWhat is this comment all about? It doesn't seem to have any relation\nto what the test itself is actually doing. (I see that it was copied\nfrom an earlier test in this script, but that doesn't help me\nunderstand what it is trying to say.)\n\n> +       >test-file &&\n> +       git add test-file &&\n> +       git commit --dry-run --short\n> +'\n"},{"id":"357173","messageId":"CAPig+cSnFE9G+09LucqTQXM7nKOXPkJ45CR-hkaA+5y9JdaNHw@mail.gmail.com","threadId":"49267","inReplyTo":"CAPig+cTzBTA4hSkM9ZtRVARZEwXMv1o9GnPMrBGimt-g=ExmHw@mail.gmail.com","subject":"Re: [PATCH v2 2/3] Add test for commit --dry-run --short.","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2018-09-02T02:22:42Z","receivedAt":"2018-09-02T02:22:54Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sat, Sep 1, 2018 at 10:18 PM Eric Sunshine <sunshine@sunshineco.com> wrote:\n> On Sat, Sep 1, 2018 at 7:53 PM Stephen P. Smith <ischis2@cox.net> wrote:\n> > The test demonstrates that the setting of the commitable flag is\n> > broken.\n>\n> s/commitable/committable/\n\nLooking at patch 3/3, I see that this misspelling exists in the code\nitself, so I guess my recommended spelling correction isn't needed. It\nmight make sense, though, to quote this word in the commit message to\nmake it more clear that that is the literal spelling in the code. That\nis:\n\n   ...demonstrates that the setting of the 'commitable' flag...\n\n(Not itself worth a re-roll.)\n"},{"id":"357175","messageId":"3033920.bsSLEtmIa1@thunderbird","threadId":"49267","inReplyTo":"20180901235256.4260-3-ischis2@cox.net","subject":"Re: [PATCH v2 2/3] Add test for commit --dry-run --short.","fromName":"Stephen & Linda Smith","fromEmail":"ischis2@cox.net","sentAt":"2018-09-02T05:17:43Z","receivedAt":"2018-09-02T05:17:47Z","isPatch":true,"sender":{"key":"ishchis2@gmail.com","avatar":null},"body":"On Saturday, September 1, 2018 7:18:34 PM MST Eric Sunshine wrote:\n> > diff --git a/t/t7501-commit.sh b/t/t7501-commit.sh\n> > @@ -682,4 +682,12 @@ test_expect_success '--dry-run with conflicts fixed\n> > from a merge' ' +test_expect_failure '--dry-run --short' '\n> > +       # setup two branches with conflicting information\n> > +       # in the same file, resolve the conflict\n> \n> What is this comment all about? It doesn't seem to have any relation\n> to what the test itself is actually doing. (I see that it was copied\n> from an earlier test in this script, but that doesn't help me\n> understand what it is trying to say.)\n\nAgreed.   \n\nI saw your other email about not being worth a re-roll, but I've made the \nchange locally in case Junio wants me to do so.  \n\nAdditionally if there are other comments I can wrap them into a single set.\n\n\n\n"}]}