{"thread":{"id":"39526","subject":"[PATCH v2 0/2] make commit --verbose work with --no-status","startedAt":"2015-06-04T17:56:29Z","lastAt":"2015-06-05T17:13:16Z","messageCount":9,"participants":["Tay Ray Chuan","Junio C Hamano"],"isPatch":true,"patchVersion":2,"patchTotal":2},"messages":[{"id":"262969","messageId":"1433440591-30917-1-git-send-email-rctay89@gmail.com","threadId":"39526","inReplyTo":null,"subject":"[PATCH v2 0/2] make commit --verbose work with --no-status","fromName":"Tay Ray Chuan","fromEmail":"rctay89@gmail.com","sentAt":"2015-06-04T17:56:29Z","receivedAt":"2015-06-04T17:56:29Z","isPatch":true,"sender":{"key":"rctay89@gmail.com","avatar":"https://avatars.githubusercontent.com/u/61553?v=4"},"body":"When running git-commit`, --verbose appends a diff to the prepared\nmessage, while --no-status omits git-status output; thus, one would\nexpect --verbose --no-status to give a commit message with a diff of\nthe commit without git-status output.\n\nHowever, this is not what happens - the prepared commit message body is\nempty, entirely. (Needless to say, no diff is appended.) This patch\nseries attempts to make this work, as one would expect.\n\n[PATCH 1/2] extract setting of wt_status.commitable flag out of\nwt_status_print_updated()\n\nCurrently, the wt_status.commitable flag is set only when we call\nwt_status_print(). * Extract this logic, so that we don't have to go\nthrough the git-status output code just to set the flag.\n\n* In fact, it is not set when --short or --porcelain are used. This\n  series does not attempt to fix the bug, though it should be trivial to\n  do so with this patch. See 9cbcc2a (demonstrate git-commit --dry-run\n  exit code behaviour, Feb 22 2014).\n\n[PATCH 2/2] make commit --verbose work with --no-status\n\nActual work here.\n\n-- \n2.0.0.581.g64f2558\n"},{"id":"262971","messageId":"1433440591-30917-2-git-send-email-rctay89@gmail.com","threadId":"39526","inReplyTo":"1433440591-30917-1-git-send-email-rctay89@gmail.com","subject":"[PATCH v2 1/2] extract setting of wt_status.commitable flag out of wt_status_print_updated()","fromName":"Tay Ray Chuan","fromEmail":"rctay89@gmail.com","sentAt":"2015-06-04T17:56:30Z","receivedAt":"2015-06-04T17:56:30Z","isPatch":true,"sender":{"key":"rctay89@gmail.com","avatar":"https://avatars.githubusercontent.com/u/61553?v=4"},"body":"It may not be obvious from its name that wt_status_print_updated() that\nit also sets wt_status.commitable, which affects commit functionality.\nExtract this out into a separate function for improved clarity, though\nat the expense of executing another loop.\n\nSigned-off-by: Tay Ray Chuan <rctay89@gmail.com>\n---\n\nChanged since v1\n- move call to _mark_committable() to match original control-flow\n\n  Originally, our placement of the call perhaps befitted aesthetics /\n  logical grouping. But perhaps it is a better idea to match the original\n  control flow to dispel any suspicion that this patch changed behaviour\n  unintendedly.\n\n wt-status.c | 17 ++++++++++++++++-\n 1 file changed, 16 insertions(+), 1 deletion(-)\n\ndiff --git a/wt-status.c b/wt-status.c\nindex c56c78f..87550ae 100644\n--- a/wt-status.c\n+++ b/wt-status.c\n@@ -626,6 +626,21 @@ void wt_status_collect(struct wt_status *s)\n \twt_status_collect_untracked(s);\n }\n \n+void wt_status_mark_commitable(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->index_status ||\n+\t\t    d->index_status == DIFF_STATUS_UNMERGED)\n+\t\t\tcontinue;\n+\t\ts->commitable = 1;\n+\t\tbreak;\n+\t}\n+}\n+\n static void wt_status_print_unmerged(struct wt_status *s)\n {\n \tint shown_header = 0;\n@@ -664,7 +679,6 @@ static void wt_status_print_updated(struct wt_status *s)\n \t\t\tcontinue;\n \t\tif (!shown_header) {\n \t\t\twt_status_print_cached_header(s);\n-\t\t\ts->commitable = 1;\n \t\t\tshown_header = 1;\n \t\t}\n \t\twt_status_print_change_data(s, WT_STATUS_UPDATED, it);\n@@ -1360,6 +1374,7 @@ void wt_status_print(struct wt_status *s)\n \n \twt_status_print_updated(s);\n \twt_status_print_unmerged(s);\n+\twt_status_mark_commitable(s);\n \twt_status_print_changed(s);\n \tif (s->submodule_summary &&\n \t    (!s->ignore_submodule_arg ||\n-- \n2.0.0.581.g64f2558\n"},{"id":"262970","messageId":"1433440591-30917-3-git-send-email-rctay89@gmail.com","threadId":"39526","inReplyTo":"1433440591-30917-2-git-send-email-rctay89@gmail.com","subject":"[PATCH v2 2/2] make commit --verbose work with --no-status","fromName":"Tay Ray Chuan","fromEmail":"rctay89@gmail.com","sentAt":"2015-06-04T17:56:31Z","receivedAt":"2015-06-04T17:56:31Z","isPatch":true,"sender":{"key":"rctay89@gmail.com","avatar":"https://avatars.githubusercontent.com/u/61553?v=4"},"body":"When running git-commit`, --verbose appends a diff to the prepared\nmessage, while --no-status omits git-status output; thus, one would\nexpect --verbose --no-status to give a commit message with a diff of\nthe commit without git-status output.\n\nHowever, this is not what happens - the prepared commit message body is\nempty, entirely. (Needless to say, no diff is appended.) This is because\ninternally the git-status machinery is used to provide both the diff and\nstatus output, but this machinery is skipped over entirely due to\n--no-status.\n\nWe introduce a new status_format, STATUS_FORMAT_DIFFONLY, which triggers\nthe setting of the commitable flag, and the printing of the diff. This\nis set only by git-commit, and when it detects that --verbose and\n--no-status have been used.\n\nSigned-off-by: Tay Ray Chuan <rctay89@gmail.com>\n---\n\nChanged since v1: adopted peff's suggestion in\n20140224083312.GB32594@sigill.intra.peff.net, added tests.\n\n builtin/commit.c          | 12 +++++++++++-\n t/t7507-commit-verbose.sh | 34 +++++++++++++++++++++++++++++++++-\n wt-status.c               |  2 +-\n wt-status.h               |  3 +++\n 4 files changed, 48 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex 254477f..d752899 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -144,6 +144,7 @@ static enum status_format {\n \tSTATUS_FORMAT_LONG,\n \tSTATUS_FORMAT_SHORT,\n \tSTATUS_FORMAT_PORCELAIN,\n+\tSTATUS_FORMAT_DIFFONLY,\n \n \tSTATUS_FORMAT_UNSPECIFIED\n } status_format = STATUS_FORMAT_UNSPECIFIED;\n@@ -510,6 +511,10 @@ static int run_status(FILE *fp, const char *index_file, const char *prefix, int\n \tcase STATUS_FORMAT_UNSPECIFIED:\n \t\tdie(\"BUG: finalize_deferred_config() should have been called\");\n \t\tbreak;\n+\tcase STATUS_FORMAT_DIFFONLY:\n+\t\twt_status_mark_commitable(s);\n+\t\twt_status_print_verbose(s);\n+\t\tbreak;\n \tcase STATUS_FORMAT_NONE:\n \tcase STATUS_FORMAT_LONG:\n \t\twt_status_print(s);\n@@ -1213,7 +1218,12 @@ static int parse_and_validate_options(int argc, const char *argv[],\n \tif (all && argc > 0)\n \t\tdie(_(\"Paths with -a does not make sense.\"));\n \n-\tif (status_format != STATUS_FORMAT_NONE)\n+\tif (status_format == STATUS_FORMAT_NONE) {\n+\t\tif (verbose && !include_status) {\n+\t\t\tinclude_status = 1;\n+\t\t\tstatus_format = STATUS_FORMAT_DIFFONLY;\n+\t\t}\n+\t} else\n \t\tdry_run = 1;\n \n \treturn argc;\ndiff --git a/t/t7507-commit-verbose.sh b/t/t7507-commit-verbose.sh\nindex 2ddf28c..9027dd4 100755\n--- a/t/t7507-commit-verbose.sh\n+++ b/t/t7507-commit-verbose.sh\n@@ -26,7 +26,39 @@ test_expect_success 'initial commit shows verbose diff' '\n \tgit commit --amend -v\n '\n \n-test_expect_success 'second commit' '\n+test_expect_success '--verbose appends diff' '\n+\tcat >expected <<-\\EOF &&\n+\t# ------------------------ >8 ------------------------\n+\t# Do not touch the line above.\n+\t# Everything below will be removed.\n+\tdiff --git a/file b/file\n+\tindex d95f3ad..94ab063 100644\n+\t--- a/file\n+\t+++ b/file\n+\t@@ -1 +1,2 @@\n+\t content\n+\t+content content\n+\tEOF\n+\tcat >editor <<-\\EOF &&\n+\t#!/bin/sh\n+\tawk \"/^# -+ >8 -+$/ { p=1 } p\" \"$1\" >actual\n+\techo commit > \"$1\"\n+\tEOF\n+\tchmod 755 editor &&\n+\techo content content >> file &&\n+\tgit add file &&\n+\ttest_tick &&\n+\tEDITOR=./editor git commit --verbose &&\n+\ttest_cmp expected actual\n+'\n+\n+test_expect_success '--verbose --no-status appends diff' '\n+\tgit reset --soft HEAD^ &&\n+\tEDITOR=./editor git commit --verbose --no-status &&\n+\ttest_cmp expected actual\n+'\n+\n+test_expect_success 'commit' '\n \techo content modified >file &&\n \tgit add file &&\n \tgit commit -F message\ndiff --git a/wt-status.c b/wt-status.c\nindex 87550ae..c4f7e48 100644\n--- a/wt-status.c\n+++ b/wt-status.c\n@@ -857,7 +857,7 @@ void wt_status_add_cut_line(FILE *fp)\n \tstrbuf_release(&buf);\n }\n \n-static void wt_status_print_verbose(struct wt_status *s)\n+void wt_status_print_verbose(struct wt_status *s)\n {\n \tstruct rev_info rev;\n \tstruct setup_revision_opt opt;\ndiff --git a/wt-status.h b/wt-status.h\nindex e0a99f7..4388296 100644\n--- a/wt-status.h\n+++ b/wt-status.h\n@@ -98,8 +98,11 @@ void wt_status_add_cut_line(FILE *fp);\n void wt_status_prepare(struct wt_status *s);\n void wt_status_print(struct wt_status *s);\n void wt_status_collect(struct wt_status *s);\n+void wt_status_mark_commitable(struct wt_status *state);\n void wt_status_get_state(struct wt_status_state *state, int get_detached_from);\n \n+void wt_status_print_verbose(struct wt_status *s);\n+\n void wt_shortstatus_print(struct wt_status *s);\n void wt_porcelain_print(struct wt_status *s);\n \n-- \n2.0.0.581.g64f2558\n"},{"id":"262973","messageId":"xmqqd21buxla.fsf@gitster.dls.corp.google.com","threadId":"39526","inReplyTo":"1433440591-30917-1-git-send-email-rctay89@gmail.com","subject":"Re: [PATCH v2 0/2] make commit --verbose work with --no-status","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-06-04T18:39:45Z","receivedAt":"2015-06-04T18:39:45Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Tay Ray Chuan <rctay89@gmail.com> writes:\n\n> When running git-commit`, --verbose appends a diff to the prepared\n> message, while --no-status omits git-status output.\n\nThe --verbose option is called --verbose and not --diff or --patch\nfor a reason, though.  The default is to show extra information as\ncomments, and verbose tells us to make that extra information more\nverbose.  We call that extra information \"status\", so it is natural\nfor \"--no-status\" to drop that extra information.\n\n> ; thus, one would\n> expect --verbose --no-status to give a commit message with a diff of\n> the commit without git-status output.\n>\n> However, this is not what happens\n\nAnd for a good reason, I would think.\n"},{"id":"262980","messageId":"xmqq8ubzupi8.fsf@gitster.dls.corp.google.com","threadId":"39526","inReplyTo":"1433440591-30917-2-git-send-email-rctay89@gmail.com","subject":"Re: [PATCH v2 1/2] extract setting of wt_status.commitable flag out of wt_status_print_updated()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-06-04T21:34:23Z","receivedAt":"2015-06-04T21:34:23Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Tay Ray Chuan <rctay89@gmail.com> writes:\n\n> It may not be obvious from its name that wt_status_print_updated() that\n> it also sets wt_status.commitable, which affects commit functionality.\n> Extract this out into a separate function for improved clarity, though\n> at the expense of executing another loop.\n\nMakes sense.\n\n> @@ -1360,6 +1374,7 @@ void wt_status_print(struct wt_status *s)\n>  \n>  \twt_status_print_updated(s);\n>  \twt_status_print_unmerged(s);\n> +\twt_status_mark_commitable(s);\n>  \twt_status_print_changed(s);\n>  \tif (s->submodule_summary &&\n>  \t    (!s->ignore_submodule_arg ||\n\nAs this is the only callsite of _updated(), we can be assured that\nthe conversion would not change the behaviour.\n\nBut I am not sure the placement of the new call is sensible.  The\nstandard pattern used in the wt-status infrastructure is to first\ncollect the information and then make output based on what was\ncollected.  Because the value of this patch is to separte the \"is it\ncommittable?\" information gathering step out of the output step,\nshouldn't the call be made a lot earlier than these sequence of\nwt_status_print_blah() calls?\n\nI am wondering if the flipping of the \"is it committable?\" bit\nbelongs to wt_status_collect().  It could be that some other crufty\nchecks that wt_status_print() have accumulated over time might be\nbetter moved to the \"collect\" phase, but that is a separate topic.\n"},{"id":"263010","messageId":"CALUzUxqeadRii1o0-yo=QaZCqoAzGk+aVq=y1-11dJvK=em0qw@mail.gmail.com","threadId":"39526","inReplyTo":"xmqqd21buxla.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v2 0/2] make commit --verbose work with --no-status","fromName":"Tay Ray Chuan","fromEmail":"rctay89@gmail.com","sentAt":"2015-06-05T12:40:01Z","receivedAt":"2015-06-05T12:40:01Z","isPatch":true,"sender":{"key":"rctay89@gmail.com","avatar":"https://avatars.githubusercontent.com/u/61553?v=4"},"body":"Hi Junio,\n\nOn Fri, Jun 5, 2015 at 2:39 AM, Junio C Hamano <gitster@pobox.com> wrote:\n>\n> Tay Ray Chuan <rctay89@gmail.com> writes:\n>\n> > When running git-commit`, --verbose appends a diff to the prepared\n> > message, while --no-status omits git-status output.\n>\n> The --verbose option is called --verbose and not --diff or --patch\n> for a reason, though.  The default is to show extra information as\n> comments, and verbose tells us to make that extra information more\n> verbose.  We call that extra information \"status\", so it is natural\n> for \"--no-status\" to drop that extra information.\n\nThanks for the explanation. Now I can appreciate why git-commit works this way.\n\nWould it be a good idea to have a --diff-only option to include diff,\nbut not status output? Or perhaps a --diff option, while leaving it to\nthe user to specify if status output is to be included with\n--no-status, which would open the doors for mixing and matching status\nformatting control, eg. with --short.\n\n-- \nCheers,\nRay Chuan\n"},{"id":"263025","messageId":"xmqqbnguta69.fsf@gitster.dls.corp.google.com","threadId":"39526","inReplyTo":"CALUzUxqeadRii1o0-yo=QaZCqoAzGk+aVq=y1-11dJvK=em0qw@mail.gmail.com","subject":"Re: [PATCH v2 0/2] make commit --verbose work with --no-status","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-06-05T16:03:10Z","receivedAt":"2015-06-05T16:03:10Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Tay Ray Chuan <rctay89@gmail.com> writes:\n\n> Would it be a good idea to have a --diff-only option to include diff,\n> but not status output? Or perhaps a --diff option, while leaving it to\n> the user to specify if status output is to be included with\n> --no-status, which would open the doors for mixing and matching status\n> formatting control, eg. with --short.\n\nThe name \"--diff-only\" does not sound right, as people would wonder\nwhat should happen when you give \"--status --diff-only\".\n\nPerhaps you would need to do some careful thinking, similar to what\nwe did when deciding the \"diff\" and \"log\" options.\n\nWe originally had \"--patch\" and then \"--patch-with-stat\" to \"diff\"\nand \"log\", but soon after that people found that \"show only stat\nwithout the patch text\" is a useful thing to do.  We retrofitted the\ncommand line parser to take \"--patch\" and \"--stat\" as orthogonal but\ninter-related options, which was a successful conversion that did\nnot break backward compatibility (These days people would not even\nknow that these strangely combined forms \"--patch-with-stat\" and\n\"--patch-with-raw\" even exist).\n\nAll of the above assumes that showing only the patch and not other\nhints to help situation awareness while making a commit is a useful\nthing in the first place.  I am undecided on that point myself.\n\nThanks.\n"},{"id":"263037","messageId":"CALUzUxrFvE-SDW0q2P08vR7rc4GHdpm24y7dk+kUdyGGwmqwOQ@mail.gmail.com","threadId":"39526","inReplyTo":"xmqqbnguta69.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v2 0/2] make commit --verbose work with --no-status","fromName":"Tay Ray Chuan","fromEmail":"rctay89@gmail.com","sentAt":"2015-06-05T16:48:35Z","receivedAt":"2015-06-05T16:48:35Z","isPatch":true,"sender":{"key":"rctay89@gmail.com","avatar":"https://avatars.githubusercontent.com/u/61553?v=4"},"body":"On Sat, Jun 6, 2015 at 12:03 AM, Junio C Hamano <gitster@pobox.com> wrote:\n> Tay Ray Chuan <rctay89@gmail.com> writes:\n>\n>> Would it be a good idea to have a --diff-only option to include diff,\n>> but not status output? Or perhaps a --diff option, while leaving it to\n>> the user to specify if status output is to be included with\n>> --no-status, which would open the doors for mixing and matching status\n>> formatting control, eg. with --short.\n>\n> The name \"--diff-only\" does not sound right, as people would wonder\n> what should happen when you give \"--status --diff-only\".\n>\n> Perhaps you would need to do some careful thinking, similar to what\n> we did when deciding the \"diff\" and \"log\" options.\n>\n> We originally had \"--patch\" and then \"--patch-with-stat\" to \"diff\"\n> and \"log\", but soon after that people found that \"show only stat\n> without the patch text\" is a useful thing to do.  We retrofitted the\n> command line parser to take \"--patch\" and \"--stat\" as orthogonal but\n> inter-related options, which was a successful conversion that did\n> not break backward compatibility (These days people would not even\n> know that these strangely combined forms \"--patch-with-stat\" and\n> \"--patch-with-raw\" even exist).\n>\n> All of the above assumes that showing only the patch and not other\n> hints to help situation awareness while making a commit is a useful\n> thing in the first place.  I am undecided on that point myself.\n\nHmm, perhaps such functionality should be off-loaded to a third-party\nwrapper. (I'd not be surprised if most wrappers already have this.)\n\n-- \nCheers,\nRay Chuan\n"},{"id":"263042","messageId":"xmqqlhfyrscz.fsf@gitster.dls.corp.google.com","threadId":"39526","inReplyTo":"CALUzUxrFvE-SDW0q2P08vR7rc4GHdpm24y7dk+kUdyGGwmqwOQ@mail.gmail.com","subject":"Re: [PATCH v2 0/2] make commit --verbose work with --no-status","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-06-05T17:13:16Z","receivedAt":"2015-06-05T17:13:16Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Tay Ray Chuan <rctay89@gmail.com> writes:\n\n>> All of the above assumes that showing only the patch and not other\n>> hints to help situation awareness while making a commit is a useful\n>> thing in the first place.  I am undecided on that point myself.\n>\n> Hmm, perhaps such functionality should be off-loaded to a third-party\n> wrapper. (I'd not be surprised if most wrappers already have this.)\n\nIf you believe that parenthesised comment to be true, then that\nwould be a sign that such a feature is desirable, no?  Substantiate\nit and let's relieve the third-party wrappers of that burden, then.\n"}]}