{"thread":{"id":"27879","subject":"A few questions about git-reset's reflog messages","startedAt":"2011-07-21T19:28:30Z","lastAt":"2011-07-22T20:57:21Z","messageCount":3,"participants":["Ori Avtalion","Jeff King","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"171813","messageId":"4E287DDE.8020108@avtalion.name","threadId":"27879","inReplyTo":null,"subject":"A few questions about git-reset's reflog messages","fromName":"Ori Avtalion","fromEmail":"ori@avtalion.name","sentAt":"2011-07-21T19:28:30Z","receivedAt":"2011-07-21T19:28:30Z","isPatch":false,"sender":{"key":"ori@avtalion.name","avatar":"https://avatars.githubusercontent.com/u/28355?v=4"},"body":"Hi,\n\nI noticed an inconsistency with the reset command's reflog messages.\n\nThe command:\n    g reset <tree-ish>\n\nPrints this reflog message:\n\n    <tree-ish>: updating HEAD\n\nUsually, actual lines from \"git reflog\" are:\n640a027 HEAD@{0}: HEAD~1: updating HEAD\n0657539 HEAD@{1}: 0657539: updating HEAD\n\nThis feels redundant and not very informative.\n\nIs there any reason to print the tree-ish in the command? The 'raw' sha1\nis already recorded in the reflog.\n\nWhy does the message not mention 'reset' in the beginning like (most?)\nother commands?\n\nI dug into builtin/reset.c to try and improve it, and came across a few\nodd things, that I'd appreciate if someone would clarify:\n\n* There is code to set a \"updating ORIG_HEAD\" reflog message, but I\ncan't trigger it. What use-case causes it?\n\n* The part of the reflog message before the colon is composed by\nargs_to_str() which prints all of the arguments after the opts. This\nseems redundant as the only form of 'reset' that updates the reflog is\none with a single '<commit>' argument after the options. What is there\nfor args_to_str to loop over?\n\nThanks,\nOri\n"},{"id":"171843","messageId":"20110722161222.GA20700@sigill.intra.peff.net","threadId":"27879","inReplyTo":"4E287DDE.8020108@avtalion.name","subject":"Re: A few questions about git-reset's reflog messages","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-07-22T16:12:23Z","receivedAt":"2011-07-22T16:12:23Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jul 21, 2011 at 10:28:30PM +0300, Ori Avtalion wrote:\n\n> I noticed an inconsistency with the reset command's reflog messages.\n> \n> The command:\n>     g reset <tree-ish>\n> \n> Prints this reflog message:\n> \n>     <tree-ish>: updating HEAD\n> \n> Usually, actual lines from \"git reflog\" are:\n> 640a027 HEAD@{0}: HEAD~1: updating HEAD\n> 0657539 HEAD@{1}: 0657539: updating HEAD\n> \n> This feels redundant and not very informative.\n> \n> Is there any reason to print the tree-ish in the command? The 'raw' sha1\n> is already recorded in the reflog.\n> \n> Why does the message not mention 'reset' in the beginning like (most?)\n> other commands?\n\nI posted this patch recently:\n\n  http://article.gmane.org/gmane.comp.version-control.git/174471\n\nbut it didn't get any comment, probably because we were in release\nfreeze. Here's a repost:\n\n-- >8 --\nSubject: [PATCH] reset: give better reflog messages\n\nThe reset command creates its reflog entry from argv.\nHowever, it does so after having run parse_options, which\nmeans the only thing left in argv is any non-option\narguments. Thus you would end up with confusing reflog\nentries like:\n\n  $ git reset --hard HEAD^\n  $ git reset --soft HEAD@{1}\n  $ git log -2 -g --oneline\n  8e46cad HEAD@{0}: HEAD@{1}: updating HEAD\n  1eb9486 HEAD@{1}: HEAD^: updating HEAD\n\nHowever, we must also consider that some scripts may set\nGIT_REFLOG_ACTION before calling reset, and we need to show\ntheir reflog action (with our text appended). For example:\n\n  rebase -i (squash): updating HEAD\n\nOn top of that, we also set the ORIG_HEAD reflog action\n(even though it doesn't generally exist). In that case, the\nreset argument is somewhat meaningless, as it has nothing to\ndo with what's in ORIG_HEAD.\n\nThis patch changes the reset reflog code to show:\n\n  $GIT_REFLOG_ACTION: updating {HEAD,ORIG_HEAD}\n\nas before, but only if GIT_REFLOG_ACTION is set. Otherwise,\nshow:\n\n   reset: moving to $rev\n\nfor HEAD, and:\n\n   reset: updating ORIG_HEAD\n\nfor ORIG_HEAD (this is still somewhat superfluous, since we\nare in the ORIG_HEAD reflog, obviously, but at least we now\nmention which command was used to update it).\n\nWhile we're at it, we can clean up the code a bit:\n\n  1. Use strbufs to make the message.\n\n  1. Use the \"rev\" parameter instead of showing all options.\n     This makes more sense, since it is the only thing\n     impacting the writing of the ref.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin/reset.c        |   49 +++++++++++++++--------------------------------\n t/t1412-reflog-loop.sh |    8 +++---\n 2 files changed, 20 insertions(+), 37 deletions(-)\n\ndiff --git a/builtin/reset.c b/builtin/reset.c\nindex 98bca04..27b3426 100644\n--- a/builtin/reset.c\n+++ b/builtin/reset.c\n@@ -33,25 +33,6 @@ static const char *reset_type_names[] = {\n \tN_(\"mixed\"), N_(\"soft\"), N_(\"hard\"), N_(\"merge\"), N_(\"keep\"), NULL\n };\n \n-static char *args_to_str(const char **argv)\n-{\n-\tchar *buf = NULL;\n-\tunsigned long len, space = 0, nr = 0;\n-\n-\tfor (; *argv; argv++) {\n-\t\tlen = strlen(*argv);\n-\t\tALLOC_GROW(buf, nr + 1 + len, space);\n-\t\tif (nr)\n-\t\t\tbuf[nr++] = ' ';\n-\t\tmemcpy(buf + nr, *argv, len);\n-\t\tnr += len;\n-\t}\n-\tALLOC_GROW(buf, nr + 1, space);\n-\tbuf[nr] = '\\0';\n-\n-\treturn buf;\n-}\n-\n static inline int is_merge(void)\n {\n \treturn !access(git_path(\"MERGE_HEAD\"), F_OK);\n@@ -215,14 +196,18 @@ static int read_from_tree(const char *prefix, const char **argv,\n \treturn update_index_refresh(index_fd, lock, refresh_flags);\n }\n \n-static void prepend_reflog_action(const char *action, char *buf, size_t size)\n+static void set_reflog_message(struct strbuf *sb, const char *action,\n+\t\t\t       const char *rev)\n {\n-\tconst char *sep = \": \";\n \tconst char *rla = getenv(\"GIT_REFLOG_ACTION\");\n-\tif (!rla)\n-\t\trla = sep = \"\";\n-\tif (snprintf(buf, size, \"%s%s%s\", rla, sep, action) >= size)\n-\t\twarning(_(\"Reflog action message too long: %.*s...\"), 50, buf);\n+\n+\tstrbuf_reset(sb);\n+\tif (rla)\n+\t\tstrbuf_addf(sb, \"%s: %s\", rla, action);\n+\telse if (rev)\n+\t\tstrbuf_addf(sb, \"reset: moving to %s\", rev);\n+\telse\n+\t\tstrbuf_addf(sb, \"reset: %s\", action);\n }\n \n static void die_if_unmerged_cache(int reset_type)\n@@ -241,7 +226,7 @@ int cmd_reset(int argc, const char **argv, const char *prefix)\n \tunsigned char sha1[20], *orig = NULL, sha1_orig[20],\n \t\t\t\t*old_orig = NULL, sha1_old_orig[20];\n \tstruct commit *commit;\n-\tchar *reflog_action, msg[1024];\n+\tstruct strbuf msg = STRBUF_INIT;\n \tconst struct option options[] = {\n \t\tOPT__QUIET(&quiet, \"be quiet, only report errors\"),\n \t\tOPT_SET_INT(0, \"mixed\", &reset_type,\n@@ -261,8 +246,6 @@ int cmd_reset(int argc, const char **argv, const char *prefix)\n \n \targc = parse_options(argc, argv, prefix, options, git_reset_usage,\n \t\t\t\t\t\tPARSE_OPT_KEEP_DASHDASH);\n-\treflog_action = args_to_str(argv);\n-\tsetenv(\"GIT_REFLOG_ACTION\", reflog_action, 0);\n \n \t/*\n \t * Possible arguments are:\n@@ -357,13 +340,13 @@ int cmd_reset(int argc, const char **argv, const char *prefix)\n \t\told_orig = sha1_old_orig;\n \tif (!get_sha1(\"HEAD\", sha1_orig)) {\n \t\torig = sha1_orig;\n-\t\tprepend_reflog_action(\"updating ORIG_HEAD\", msg, sizeof(msg));\n-\t\tupdate_ref(msg, \"ORIG_HEAD\", orig, old_orig, 0, MSG_ON_ERR);\n+\t\tset_reflog_message(&msg, \"updating ORIG_HEAD\", NULL);\n+\t\tupdate_ref(msg.buf, \"ORIG_HEAD\", orig, old_orig, 0, MSG_ON_ERR);\n \t}\n \telse if (old_orig)\n \t\tdelete_ref(\"ORIG_HEAD\", old_orig, 0);\n-\tprepend_reflog_action(\"updating HEAD\", msg, sizeof(msg));\n-\tupdate_ref_status = update_ref(msg, \"HEAD\", sha1, orig, 0, MSG_ON_ERR);\n+\tset_reflog_message(&msg, \"updating HEAD\", rev);\n+\tupdate_ref_status = update_ref(msg.buf, \"HEAD\", sha1, orig, 0, MSG_ON_ERR);\n \n \tswitch (reset_type) {\n \tcase HARD:\n@@ -380,7 +363,7 @@ int cmd_reset(int argc, const char **argv, const char *prefix)\n \n \tremove_branch_state();\n \n-\tfree(reflog_action);\n+\tstrbuf_release(&msg);\n \n \treturn update_ref_status;\n }\ndiff --git a/t/t1412-reflog-loop.sh b/t/t1412-reflog-loop.sh\nindex 7f519e5..647d888 100755\n--- a/t/t1412-reflog-loop.sh\n+++ b/t/t1412-reflog-loop.sh\n@@ -21,10 +21,10 @@ test_expect_success 'setup reflog with alternating commits' '\n \n test_expect_success 'reflog shows all entries' '\n \tcat >expect <<-\\EOF\n-\t\ttopic@{0} two: updating HEAD\n-\t\ttopic@{1} one: updating HEAD\n-\t\ttopic@{2} two: updating HEAD\n-\t\ttopic@{3} one: updating HEAD\n+\t\ttopic@{0} reset: moving to two\n+\t\ttopic@{1} reset: moving to one\n+\t\ttopic@{2} reset: moving to two\n+\t\ttopic@{3} reset: moving to one\n \t\ttopic@{4} branch: Created from HEAD\n \tEOF\n \tgit log -g --format=\"%gd %gs\" topic >actual &&\n-- \n1.7.6.rc1.12.g65e2\n"},{"id":"171870","messageId":"7vzkk6vupw.fsf@alter.siamese.dyndns.org","threadId":"27879","inReplyTo":"20110722161222.GA20700@sigill.intra.peff.net","subject":"Re: A few questions about git-reset's reflog messages","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-07-22T20:57:21Z","receivedAt":"2011-07-22T20:57:21Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> Subject: [PATCH] reset: give better reflog messages\n> ...\n> This patch changes the reset reflog code to show:\n>\n>   $GIT_REFLOG_ACTION: updating {HEAD,ORIG_HEAD}\n>\n> as before, but only if GIT_REFLOG_ACTION is set. Otherwise,\n> show:\n>\n>    reset: moving to $rev\n>\n> for HEAD, and:\n>\n>    reset: updating ORIG_HEAD\n>\n> for ORIG_HEAD (this is still somewhat superfluous, since we\n> are in the ORIG_HEAD reflog, obviously, but at least we now\n> mention which command was used to update it).\n\nLooks sensible; thanks.\n"}]}