{"thread":{"id":"47739","subject":"[RFC/PATCH] reset --hard: make use of the pretty machinery","startedAt":"2018-02-01T20:57:25Z","lastAt":"2018-02-02T20:17:02Z","messageCount":3,"participants":["Thomas Gummerer","Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"338048","messageId":"20180201205721.30087-1-t.gummerer@gmail.com","threadId":"47739","inReplyTo":null,"subject":"[RFC/PATCH] reset --hard: make use of the pretty machinery","fromName":"Thomas Gummerer","fromEmail":"t.gummerer@gmail.com","sentAt":"2018-02-01T20:57:21Z","receivedAt":"2018-02-01T20:57:25Z","isPatch":true,"sender":{"key":"t.gummerer@gmail.com","avatar":"https://avatars.githubusercontent.com/u/191004?v=4"},"body":"reset --hard currently uses its own logic for printing the first line of\nthe commit message in its output.  Instead of just using the first line,\nuse the pretty machinery to create the output.\n\nIn addition to the easier to follow code, this makes the output more\nconsistent with other commands that print the title of the commit, such\nas 'git commit --oneline' or 'git checkout', which both use\n'pp_commit_easy()' with the CMIT_FMT_ONELINE modifier.\n\nIt is a slight change of the output if the second line of the commit\nmessage is not a blank line, i.e. if the commit message is\n\n    foo\n    bar\n\npreviously we would print \"HEAD is now at 000000 foo\", while after\nthis change we print \"HEAD is now at 000000 foo bar\", same as 'git log\n--oneline' shows \"000000 foo bar\".\n\nSo this does make the output more consistent with other commands, and\n'reset' is a porcelain command, so nobody should be parsing the output\nin scripts.\n\nThe current behaviour dates back to 0e5a7faa3a (\"Make \"git reset\" a\nbuiltin.\", 2007-09-11), so I assume (without digging into the old\ncodebase too much) that the logic was implemented because there was\nno convenience function such as 'pp_commit_easy' that would do this\nalready.\n\nSigned-off-by: Thomas Gummerer <t.gummerer@gmail.com>\n---\n\nSending this as RFC/PATCH, as I'm not 100% sure this change in\nbehaviour is acceptable, and that I'm not missing some other edge\ncase, but I noticed this while trying to find out how this message\ngets constructed, and just using the pretty machinery seems much\nsimpler, and more consisent\n\n builtin/reset.c | 28 ++++++++++------------------\n 1 file changed, 10 insertions(+), 18 deletions(-)\n\ndiff --git a/builtin/reset.c b/builtin/reset.c\nindex e15f595799..5da0f75de9 100644\n--- a/builtin/reset.c\n+++ b/builtin/reset.c\n@@ -106,24 +106,16 @@ static int reset_index(const struct object_id *oid, int reset_type, int quiet)\n \n static void print_new_head_line(struct commit *commit)\n {\n-\tconst char *hex, *body;\n-\tconst char *msg;\n-\n-\thex = find_unique_abbrev(commit->object.oid.hash, DEFAULT_ABBREV);\n-\tprintf(_(\"HEAD is now at %s\"), hex);\n-\tmsg = logmsg_reencode(commit, NULL, get_log_output_encoding());\n-\tbody = strstr(msg, \"\\n\\n\");\n-\tif (body) {\n-\t\tconst char *eol;\n-\t\tsize_t len;\n-\t\tbody = skip_blank_lines(body + 2);\n-\t\teol = strchr(body, '\\n');\n-\t\tlen = eol ? eol - body : strlen(body);\n-\t\tprintf(\" %.*s\\n\", (int) len, body);\n-\t}\n-\telse\n-\t\tprintf(\"\\n\");\n-\tunuse_commit_buffer(commit, msg);\n+\tstruct strbuf buf = STRBUF_INIT;\n+\n+\tprintf(_(\"HEAD is now at %s\"),\n+\t\tfind_unique_abbrev(commit->object.oid.hash, DEFAULT_ABBREV));\n+\n+\tpp_commit_easy(CMIT_FMT_ONELINE, commit, &buf);\n+\tif (buf.len > 0)\n+\t\tprintf(\" %s\", buf.buf);\n+\tputchar('\\n');\n+\tstrbuf_release(&buf);\n }\n \n static void update_index_from_diff(struct diff_queue_struct *q,\n-- \n2.16.1.101.gde0f0111ea\n\n"},{"id":"338080","messageId":"20180202071518.GB16533@sigill.intra.peff.net","threadId":"47739","inReplyTo":"20180201205721.30087-1-t.gummerer@gmail.com","subject":"Re: [RFC/PATCH] reset --hard: make use of the pretty machinery","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-02-02T07:15:18Z","receivedAt":"2018-02-02T07:15:25Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Feb 01, 2018 at 08:57:21PM +0000, Thomas Gummerer wrote:\n\n> It is a slight change of the output if the second line of the commit\n> message is not a blank line, i.e. if the commit message is\n> \n>     foo\n>     bar\n> \n> previously we would print \"HEAD is now at 000000 foo\", while after\n> this change we print \"HEAD is now at 000000 foo bar\", same as 'git log\n> --oneline' shows \"000000 foo bar\".\n> \n> So this does make the output more consistent with other commands, and\n> 'reset' is a porcelain command, so nobody should be parsing the output\n> in scripts.\n\nYeah, I'd say it's definitely fine to change the (already translated!)\nstderr output of git-reset. I agree that it's an improvement to be\nconsistent with other commands here.\n\n> The current behaviour dates back to 0e5a7faa3a (\"Make \"git reset\" a\n> builtin.\", 2007-09-11), so I assume (without digging into the old\n> codebase too much) that the logic was implemented because there was\n> no convenience function such as 'pp_commit_easy' that would do this\n> already.\n\nYes, there used to be quite a lot of ad hoc parsing of commit objects,\nbut these days we have safer and more readable alternatives.  Even\nwithout the visible behavior change, I think it would be worth doing\nthis patch just as a code cleanup.\n\n> +\tstruct strbuf buf = STRBUF_INIT;\n> +\n> +\tprintf(_(\"HEAD is now at %s\"),\n> +\t\tfind_unique_abbrev(commit->object.oid.hash, DEFAULT_ABBREV));\n> +\n> +\tpp_commit_easy(CMIT_FMT_ONELINE, commit, &buf);\n> +\tif (buf.len > 0)\n> +\t\tprintf(\" %s\", buf.buf);\n> +\tputchar('\\n');\n> +\tstrbuf_release(&buf);\n\nI was disappointed you still had to call find_unique_abbrev(). It seems\nlike we ought to be able to do this in a single formatting call. But\nCMIT_FMT_ONELINE is just about the subject, and doesn't include the hash\nat all. There's no equivalent without turning to the user-format code.\n\nYou can do:\n\n  struct pretty_print_context ctx = {0, DEFAULT_ABBREV};\n  format_commit_message(commit, _(\"HEAD is now at %H %s\"), &buf, &ctx);\n  puts(buf.buf);\n\nbut I was annoyed at having to say \"DEFAULT_ABBREV\". It seems like that\nought to be the default.\n\nAnyway, I'm fine with your patch as-is. I just needed a little\nformatting code-golf in my day.\n\n-Peff\n"},{"id":"338117","messageId":"xmqqa7wrcfk9.fsf@gitster-ct.c.googlers.com","threadId":"47739","inReplyTo":"20180201205721.30087-1-t.gummerer@gmail.com","subject":"Re: [RFC/PATCH] reset --hard: make use of the pretty machinery","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-02-02T20:16:54Z","receivedAt":"2018-02-02T20:17:02Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Thomas Gummerer <t.gummerer@gmail.com> writes:\n\n> In addition to the easier to follow code, this makes the output more\n> consistent with other commands that print the title of the commit, such\n> as 'git commit --oneline' or 'git checkout', which both use\n> 'pp_commit_easy()' with the CMIT_FMT_ONELINE modifier.\n\nYes, this is absolutely the right thing to do.\n\n> +\n> +\tpp_commit_easy(CMIT_FMT_ONELINE, commit, &buf);\n> +\tif (buf.len > 0)\n> +\t\tprintf(\" %s\", buf.buf);\n> +\tputchar('\\n');\n> +\tstrbuf_release(&buf);\n>  }\n>  \n>  static void update_index_from_diff(struct diff_queue_struct *q,\n\nThanks, will queue.\n"}]}