{"thread":{"id":"10972","subject":"[PATCH v2] builtin-commit: Include the diff in the commit message when verbose.","startedAt":"2007-11-22T02:54:49Z","lastAt":"2007-11-26T15:21:15Z","messageCount":5,"participants":["Kristian Høgsberg","Johannes Schindelin","Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":2,"patchTotal":null},"messages":[{"id":"60597","messageId":"1195700089-8326-1-git-send-email-krh@redhat.com","threadId":"10972","inReplyTo":null,"subject":"[PATCH v2] builtin-commit: Include the diff in the commit message when verbose.","fromName":"Kristian Høgsberg","fromEmail":"krh@redhat.com","sentAt":"2007-11-22T02:54:49Z","receivedAt":"2007-11-22T02:54:49Z","isPatch":true,"sender":{"key":"krh@redhat.com","avatar":"https://gravatar.com/avatar/763dee6f9594ac474f725b137a39565792928e583ddf59b32befc2907409027e?d=mp&s=160"},"body":"run_diff_index() and the entire diff machinery is hard coded to output\nto stdout, so just redirect that and restore it when done.\n\nSigned-off-by: Kristian Høgsberg \n---\n\nThinking about this, the dup-dance really belongs in wt-status.c since\nthat interface promises that it can redirect to a FILE *. Also the\nfflush()'es in the earlier patch could go wrong if stdout ends up flushing\nduring run_diff_index() and fp has unflushed data.\n\n builtin-commit.c | 8 +++++++-\n wt-status.c | 16 ++++++++++++++++\n 2 files changed, 23 insertions(+), 1 deletions(-)\n\ndiff --git a/builtin-commit.c b/builtin-commit.c\nindex e8893f8..39764ae 100644\n--- a/builtin-commit.c\n+++ b/builtin-commit.c\n@@ -662,7 +662,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix)\n \tint header_len;\n \tstruct strbuf sb;\n \tconst char *index_file, *reflog_msg;\n-\tchar *nl;\n+\tchar *nl, *p;\n \tunsigned char commit_sha1[20];\n \tstruct ref_lock *ref_lock;\n \n@@ -758,6 +758,12 @@ int cmd_commit(int argc, const char **argv, const char *prefix)\n \t\trollback_index_files();\n \t\texit(1);\n \t}\n+\n+\t/* Truncate the message just before the diff, if any. */\n+\tp = strstr(sb.buf, \"\\ndiff --git a/\");\n+\tif (p != NULL)\n+\t\tstrbuf_setlen(&sb, p - sb.buf);\n+\n \tstripspace(&sb, 1);\n \tif (sb.len < header_len || message_is_empty(&sb, header_len)) {\n \t\trollback_index_files();\ndiff --git a/wt-status.c b/wt-status.c\nindex d3c10b8..0e0439f 100644\n--- a/wt-status.c\n+++ b/wt-status.c\n@@ -315,12 +315,28 @@ static void wt_status_print_untracked(struct wt_status *s)\n static void wt_status_print_verbose(struct wt_status *s)\n {\n \tstruct rev_info rev;\n+\tint saved_stdout;\n+\n+\tfflush(s->fp);\n+\n+\t/* Sigh, the entire diff machinery is hardcoded to output to\n+\t * stdout. Do the dup-dance...*/\n+\tsaved_stdout = dup(STDOUT_FILENO);\n+\tif (saved_stdout < 0 ||dup2(fileno(s->fp), STDOUT_FILENO) < 0)\n+\t\tdie(\"couldn't redirect stdout\\n\");\n+\n \tinit_revisions(&rev, NULL);\n \tsetup_revisions(0, NULL, &rev, s->reference);\n \trev.diffopt.output_format |= DIFF_FORMAT_PATCH;\n \trev.diffopt.detect_rename = 1;\n \twt_read_cache(s);\n \trun_diff_index(&rev, 1);\n+\n+\tfflush(stdout);\n+\n+\tif (dup2(saved_stdout, STDOUT_FILENO) < 0)\n+\t\tdie(\"couldn't restore stdout\\n\");\n+\tclose(saved_stdout);\n }\n \n void wt_status_print(struct wt_status *s)\n-- \n1.5.3.4\n"},{"id":"60644","messageId":"Pine.LNX.4.64.0711221049350.27959@racer.site","threadId":"10972","inReplyTo":"1195700089-8326-1-git-send-email-krh@redhat.com","subject":"Re: [PATCH v2] builtin-commit: Include the diff in the commit message when verbose.","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2007-11-22T10:52:04Z","receivedAt":"2007-11-22T10:52:04Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Wed, 21 Nov 2007, Kristian Høgsberg wrote:\n\n> @@ -758,6 +758,12 @@ int cmd_commit(int argc, const char **argv, const char *prefix)\n> \t\trollback_index_files();\n> \t\texit(1);\n> \t}\n> +\n> +\t/* Truncate the message just before the diff, if any. */\n> +\tp = strstr(sb.buf, \"\\ndiff --git a/\");\n> +\tif (p != NULL)\n> +\t\tstrbuf_setlen(&sb, p - sb.buf);\n> +\n\nIs this related to the change in wt_status? If so, wouldn't we want to \nsuppress the diff, instead of generating it, and then killing it later?\n\nBesides, you'd want to leave the \\n there: strbuf_setlen(&sb, p + 1 - \nsb.buf);\n\n> +\t/* Sigh, the entire diff machinery is hardcoded to output to\n> +\t * stdout. Do the dup-dance...*/\n\nI wonder how much effort it would be to change that. Not that it would \nhelp too much, since we want the output in a strbuf anyway.\n\nCiao,\nDscho"},{"id":"60652","messageId":"20071122111314.GE10389@sigill.intra.peff.net","threadId":"10972","inReplyTo":"Pine.LNX.4.64.0711221049350.27959@racer.site","subject":"Re: [PATCH v2] builtin-commit: Include the diff in the commit message when verbose.","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2007-11-22T11:13:14Z","receivedAt":"2007-11-22T11:13:14Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Nov 22, 2007 at 10:52:04AM +0000, Johannes Schindelin wrote:\n\n> > +\t/* Sigh, the entire diff machinery is hardcoded to output to\n> > +\t * stdout. Do the dup-dance...*/\n> \n> I wonder how much effort it would be to change that. Not that it would \n> help too much, since we want the output in a strbuf anyway.\n\nAn \"emit\"-type callback would be nice and elegant. The biggest trick is\nthat there are a lot of formatting calls, so you'd need your callback\nsignature to be a variadic function. But it's probably do-able.\n\n-Peff\n"},{"id":"60698","messageId":"7vzlx63xey.fsf@gitster.siamese.dyndns.org","threadId":"10972","inReplyTo":"Pine.LNX.4.64.0711221049350.27959@racer.site","subject":"Re: [PATCH v2] builtin-commit: Include the diff in the commit message when verbose.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2007-11-22T19:14:29Z","receivedAt":"2007-11-22T19:14:29Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> On Wed, 21 Nov 2007, Kristian Høgsberg wrote:\n>\n>> +\n>> +\t/* Truncate the message just before the diff, if any. */\n>> +\tp = strstr(sb.buf, \"\\ndiff --git a/\");\n>> +\tif (p != NULL)\n>> +\t\tstrbuf_setlen(&sb, p - sb.buf);\n>> +\n>\n> Is this related to the change in wt_status? If so, wouldn't we want to \n> suppress the diff, instead of generating it, and then killing it later?\n\nThis corresponds to the sed script near l.545 in git-commit.sh.\n\nI've been wondering if it would be better not to have this logic\nbut instead \"git commit -v\" to show the diff text prefixed with\n'# ' to make it a proper comment, by the way.\n\n> Besides, you'd want to leave the \\n there: strbuf_setlen(&sb, p + 1 - \n> sb.buf);\n\nYup.\n\n>> +\t/* Sigh, the entire diff machinery is hardcoded to output to\n>> +\t * stdout. Do the dup-dance...*/\n>\n> I wonder how much effort it would be to change that. Not that it would \n> help too much, since we want the output in a strbuf anyway.\n\nThe codepath is preparing COMMIT_EDITMSG file for the user to\nedit (or to the standard output, if this is \"git status -v\").\n\nI do not know about \"strbuf\" part; resulting patch text could be\nlarge and in most cases the callers would rather flush it to\noutside (either pipe or file) as soon as possible than having to\nkeep it all in core.\n\nI was trying to do the stdout -> FILE *fp conversion of diff.c\nlast night but dropped it halfway, after finding one puts()\nmisconversion and fixing it. The changes should mostly be\nstraightforward but the result felt ugly.\n"},{"id":"60938","messageId":"1196090475.2875.10.camel@hinata.boston.redhat.com","threadId":"10972","inReplyTo":"7vzlx63xey.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH v2] builtin-commit: Include the diff in the commit message when verbose.","fromName":"Kristian Høgsberg","fromEmail":"krh@redhat.com","sentAt":"2007-11-26T15:21:15Z","receivedAt":"2007-11-26T15:21:15Z","isPatch":true,"sender":{"key":"krh@redhat.com","avatar":"https://gravatar.com/avatar/763dee6f9594ac474f725b137a39565792928e583ddf59b32befc2907409027e?d=mp&s=160"},"body":"\nOn Thu, 2007-11-22 at 11:14 -0800, Junio C Hamano wrote:\n> Johannes Schindelin writes:\n> \n> > On Wed, 21 Nov 2007, Kristian Høgsberg wrote:\n> >\n> >> +\n> >> +\t/* Truncate the message just before the diff, if any. */\n> >> +\tp = strstr(sb.buf, \"\\ndiff --git a/\");\n> >> +\tif (p != NULL)\n> >> +\t\tstrbuf_setlen(&sb, p - sb.buf);\n> >> +\n> >\n> > Is this related to the change in wt_status? If so, wouldn't we want to \n> > suppress the diff, instead of generating it, and then killing it later?\n> \n> This corresponds to the sed script near l.545 in git-commit.sh.\n> \n> I've been wondering if it would be better not to have this logic\n> but instead \"git commit -v\" to show the diff text prefixed with\n> '# ' to make it a proper comment, by the way.\n\nYeah, that would be nicer... I think the best way to do this is to do a\nformatting callback for the diff machinery as Jeff suggests, which can\nthen prefix '# ' and write it to a FILE *.\n\n> > Besides, you'd want to leave the \\n there: strbuf_setlen(&sb, p + 1 - \n> > sb.buf);\n> \n> Yup.\n\nRight, off-by-one. Effectively it doesn't make a difference, since\nthere will always be a comment line above the diff, When stripspace\nremoves the comments it fixes up the end-of-line problem. Patch below.\n\ncheers,\nKristian\n\n\n>From 58eac54a00d3eb6a311c6fb4faa67eb831c60e01 Mon Sep 17 00:00:00 2001\nFrom: =?utf-8?q?Kristian=20H=C3=B8gsberg?= \nDate: Mon, 26 Nov 2007 10:16:08 -0500\nSubject: [PATCH] Fix off-by-one error when truncating the diff out of the commit message.\nMIME-Version: 1.0\nContent-Type: text/plain; charset=utf-8\nContent-Transfer-Encoding: 8bit\n\nSigned-off-by: Kristian Høgsberg \n---\n builtin-commit.c | 2 +-\n 1 files changed, 1 insertions(+), 1 deletions(-)\n\ndiff --git a/builtin-commit.c b/builtin-commit.c\nindex 45e51b1..330f778 100644\n--- a/builtin-commit.c\n+++ b/builtin-commit.c\n@@ -762,7 +762,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix)\n \t/* Truncate the message just before the diff, if any. */\n \tp = strstr(sb.buf, \"\\ndiff --git a/\");\n \tif (p != NULL)\n-\t\tstrbuf_setlen(&sb, p - sb.buf);\n+\t\tstrbuf_setlen(&sb, p - sb.buf + 1);\n \n \tstripspace(&sb, 1);\n \tif (sb.len < header_len || message_is_empty(&sb, header_len)) {\n-- \n1.5.3.4.206.g58ba4\n"}]}