{"thread":{"id":"34151","subject":"[PATCH 0/2] Slightly prettier reflog message from checkout","startedAt":"2013-06-15T17:38:24Z","lastAt":"2013-06-18T14:27:57Z","messageCount":10,"participants":["Ramkumar Ramachandra","Junio C Hamano","Phil Hord"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"220953","messageId":"1371317906-3991-1-git-send-email-artagnon@gmail.com","threadId":"34151","inReplyTo":null,"subject":"[PATCH 0/2] Slightly prettier reflog message from checkout","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2013-06-15T17:38:24Z","receivedAt":"2013-06-15T17:38:24Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"[1/2] is important.  [2/2] is a minor prettification, that wouldn't\nhave been possible without [1/2].\n\nThanks.\n\nRamkumar Ramachandra (2):\n  sha1_name: stop hard-coding 40-character hex checks\n  checkout: do not write full sha1 to reflog\n\n builtin/checkout.c | 2 +-\n sha1_name.c        | 6 +++---\n 2 files changed, 4 insertions(+), 4 deletions(-)\n\n-- \n1.8.3.1.438.g96d34e8\n"},{"id":"220954","messageId":"1371317906-3991-2-git-send-email-artagnon@gmail.com","threadId":"34151","inReplyTo":"1371317906-3991-1-git-send-email-artagnon@gmail.com","subject":"[PATCH 1/2] sha1_name: stop hard-coding 40-character hex checks","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2013-06-15T17:38:25Z","receivedAt":"2013-06-15T17:38:25Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"In two places, get_sha1_basic() assumes that strings are possibly sha1\nhexes if they are 40 characters long, and calls get_sha1_hex() in these\ntwo cases.  This 40-character check is ugly and wrong: there is nothing\npreventing a revision or branch name from being exactly 40 characters.\nReplace it with a call to the more robust get_short_sha1().\n\nSigned-off-by: Ramkumar Ramachandra <artagnon@gmail.com>\n---\n sha1_name.c | 6 +++---\n 1 file changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/sha1_name.c b/sha1_name.c\nindex 90419ef..d862af3 100644\n--- a/sha1_name.c\n+++ b/sha1_name.c\n@@ -451,7 +451,7 @@ static int get_sha1_basic(const char *str, int len, unsigned char *sha1)\n \tint refs_found = 0;\n \tint at, reflog_len, nth_prior = 0;\n \n-\tif (len == 40 && !get_sha1_hex(str, sha1)) {\n+\tif (!get_short_sha1(str, strlen(str), sha1, GET_SHA1_QUIETLY)) {\n \t\trefs_found = dwim_ref(str, len, tmp_sha1, &real_ref);\n \t\tif (refs_found > 0 && warn_ambiguous_refs) {\n \t\t\twarning(warn_msg, len, str);\n@@ -492,9 +492,9 @@ static int get_sha1_basic(const char *str, int len, unsigned char *sha1)\n \t\tint detached;\n \n \t\tif (interpret_nth_prior_checkout(str, &buf) > 0) {\n-\t\t\tdetached = (buf.len == 40 && !get_sha1_hex(buf.buf, sha1));\n+\t\t\tdetached = get_short_sha1(buf.buf, buf.len, sha1, GET_SHA1_QUIETLY);\n \t\t\tstrbuf_release(&buf);\n-\t\t\tif (detached)\n+\t\t\tif (detached != SHORT_NAME_NOT_FOUND)\n \t\t\t\treturn 0;\n \t\t}\n \t}\n-- \n1.8.3.1.438.g96d34e8\n"},{"id":"220955","messageId":"1371317906-3991-3-git-send-email-artagnon@gmail.com","threadId":"34151","inReplyTo":"1371317906-3991-1-git-send-email-artagnon@gmail.com","subject":"[PATCH 2/2] checkout: do not write full sha1 to reflog","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2013-06-15T17:38:26Z","receivedAt":"2013-06-15T17:38:26Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"A snippet from a typical git session may look like this:\n\n  $ git checkout @~3\n  ...\n  $ git checkout -\n\nThe reflog entries corresponding to the two checkouts look like:\n\n  f855138: checkout: moving from bdff0e3a374617dce784f801b97500d9ba2e4705 to co-reflog\n  bdff0e3: checkout: moving from co-reflog to HEAD~3\n\nThere is no need to write the full SHA-1 to the user-visible reflog; use\nfind_unique_abbrev() to shorten the first line like:\n\n  f855138: checkout: moving from bdff0e3 to co-reflog\n\nSigned-off-by: Ramkumar Ramachandra <artagnon@gmail.com>\n---\n builtin/checkout.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/builtin/checkout.c b/builtin/checkout.c\nindex f5b50e5..41b1929 100644\n--- a/builtin/checkout.c\n+++ b/builtin/checkout.c\n@@ -619,7 +619,7 @@ static void update_refs_for_switch(const struct checkout_opts *opts,\n \n \told_desc = old->name;\n \tif (!old_desc && old->commit)\n-\t\told_desc = sha1_to_hex(old->commit->object.sha1);\n+\t\told_desc = find_unique_abbrev(old->commit->object.sha1, DEFAULT_ABBREV);\n \tstrbuf_addf(&msg, \"checkout: moving from %s to %s\",\n \t\t    old_desc ? old_desc : \"(invalid)\", new->name);\n \n-- \n1.8.3.1.438.g96d34e8\n"},{"id":"220956","messageId":"CALkWK0=s_oza=2mUmAFhLZCbFzW4ZsW+ohkxOthmu0+iGhKLSw@mail.gmail.com","threadId":"34151","inReplyTo":"1371317906-3991-1-git-send-email-artagnon@gmail.com","subject":"Re: [PATCH 0/2] Slightly prettier reflog message from checkout","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2013-06-15T17:42:03Z","receivedAt":"2013-06-15T17:42:03Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Ramkumar Ramachandra wrote:\n> [1/2] is important.  [2/2] is a minor prettification, that wouldn't\n> have been possible without [1/2].\n\nI forgot to mention: some tests fail, and I'm investigating.  This is\nan early preview.\n"},{"id":"220972","messageId":"7vmwqqkftv.fsf@alter.siamese.dyndns.org","threadId":"34151","inReplyTo":"1371317906-3991-1-git-send-email-artagnon@gmail.com","subject":"Re: [PATCH 0/2] Slightly prettier reflog message from checkout","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-06-16T01:24:44Z","receivedAt":"2013-06-16T01:24:44Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ramkumar Ramachandra <artagnon@gmail.com> writes:\n\n> [1/2] is important.  [2/2] is a minor prettification, that wouldn't\n> have been possible without [1/2].\n>\n> Thanks.\n>\n> Ramkumar Ramachandra (2):\n>   sha1_name: stop hard-coding 40-character hex checks\n>   checkout: do not write full sha1 to reflog\n>\n>  builtin/checkout.c | 2 +-\n>  sha1_name.c        | 6 +++---\n>  2 files changed, 4 insertions(+), 4 deletions(-)\n\nI view the two codepaths touched by these patches the other way\naround.\n\nAn abbreviated unique SHA-1 you have today may not be unique\ntomorrow.  There is no reason to deliberately lose information\n(e.g. by using \"Then, instead of the absolute minimum, let's record\na bit more bytes\" heuristics) when we record. The reflog recording\ncode in checkout writes full 40-characters on purpose and there is\nno reason not to do so (i.e. the codepath that is the topic of 2/2).\n\nThat is a more important design decision between the two codepaths.\n\nOnce we accept that design principle of not losing information when\nwe do not have to, it naturally follows that the writing side should\nwrite full 40-hex, and also the reading side (i.e. the codepath that\nis the topic of 1/2) should make sure that it reads 40-hex and\nnothing else.  This also reduces the risk of a funny branch name\nthat consists only of [0-9a-f] getting mistaken as an object name,\nbut that is not the primary point.\n\nSo I am fairly strongly negative on both changes.\n"},{"id":"220994","messageId":"CALkWK0ki9C9OL36b3j14C-mVMPy07Uj5FXcrfMZJs_g3zBRC9A@mail.gmail.com","threadId":"34151","inReplyTo":"7vmwqqkftv.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 0/2] Slightly prettier reflog message from checkout","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2013-06-16T09:21:52Z","receivedAt":"2013-06-16T09:21:52Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Junio C Hamano wrote:\n> I view the two codepaths touched by these patches the other way\n> around.\n\nI see.  Thanks for the early feedback.  I have some doubts.\n\n> An abbreviated unique SHA-1 you have today may not be unique\n> tomorrow.  There is no reason to deliberately lose information\n> (e.g. by using \"Then, instead of the absolute minimum, let's record\n> a bit more bytes\" heuristics) when we record. The reflog recording\n> code in checkout writes full 40-characters on purpose and there is\n> no reason not to do so (i.e. the codepath that is the topic of 2/2).\n\nWhen did we guarantee that the messages written by the reflog are invariant?\n\n  $ git checkout @^\n  $ git reflog | head -n 1\n  b1d94f2 HEAD@{2 seconds ago}: checkout: moving from checkout-dash to HEAD^\n\nWhat does HEAD^ even mean?  What guarantees that checkout-dash will\nnot be something else tomorrow?  If you want invariance, isn't that\nwhat the first field is for (b1d94f2)?  As I understand it, the\nmessages are purely to convey end-user information about the\nbreadcrumb trail: they were later made semi-semantic (like the @{-N}\nparser using them).\n\n> Once we accept that design principle of not losing information when\n> we do not have to, it naturally follows that the writing side should\n> write full 40-hex, and also the reading side (i.e. the codepath that\n> is the topic of 1/2) should make sure that it reads 40-hex and\n> nothing else.  This also reduces the risk of a funny branch name\n> that consists only of [0-9a-f] getting mistaken as an object name,\n> but that is not the primary point.\n\nAs I already explained, I don't know what information loss you're\ntalking about.  And yes, I noticed advice.object_name_warning.\n\n> So I am fairly strongly negative on both changes.\n\nOkay, but please explain it for me.\n"},{"id":"220998","messageId":"CABURp0qs7uPkczmS8EC-iNgCVh2eTaeCKsh7NwVuS4j92jMn+A@mail.gmail.com","threadId":"34151","inReplyTo":"1371317906-3991-2-git-send-email-artagnon@gmail.com","subject":"Re: [PATCH 1/2] sha1_name: stop hard-coding 40-character hex checks","fromName":"Phil Hord","fromEmail":"phil.hord@gmail.com","sentAt":"2013-06-16T13:28:20Z","receivedAt":"2013-06-16T13:28:20Z","isPatch":true,"sender":{"key":"phil.hord@gmail.com","avatar":"https://avatars.githubusercontent.com/u/123908?v=4"},"body":"On Sat, Jun 15, 2013 at 1:38 PM, Ramkumar Ramachandra\n<artagnon@gmail.com> wrote:\n> In two places, get_sha1_basic() assumes that strings are possibly sha1\n> hexes if they are 40 characters long, and calls get_sha1_hex() in these\n> two cases.  This 40-character check is ugly and wrong: there is nothing\n> preventing a revision or branch name from being exactly 40 characters.\n> Replace it with a call to the more robust get_short_sha1().\n\nI share your disdain for the bare '40's in the code.  But I think this\ncode is less clear than the previous version with the magic number.\n\n>\n> Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>\n> ---\n>  sha1_name.c | 6 +++---\n>  1 file changed, 3 insertions(+), 3 deletions(-)\n>\n> diff --git a/sha1_name.c b/sha1_name.c\n> index 90419ef..d862af3 100644\n> --- a/sha1_name.c\n> +++ b/sha1_name.c\n> @@ -451,7 +451,7 @@ static int get_sha1_basic(const char *str, int len, unsigned char *sha1)\n>         int refs_found = 0;\n>         int at, reflog_len, nth_prior = 0;\n>\n> -       if (len == 40 && !get_sha1_hex(str, sha1)) {\n> +       if (!get_short_sha1(str, strlen(str), sha1, GET_SHA1_QUIETLY)) {\n\nUse len instead of strlen(str) here.  It's faster and more correct.\n\nBut also get_short_sha1 is much heavier than get_sha1_hex and does not\nseem appropriate here.\n\n>                 refs_found = dwim_ref(str, len, tmp_sha1, &real_ref);\n>                 if (refs_found > 0 && warn_ambiguous_refs) {\n>                         warning(warn_msg, len, str);\n> @@ -492,9 +492,9 @@ static int get_sha1_basic(const char *str, int len, unsigned char *sha1)\n>                 int detached;\n>\n>                 if (interpret_nth_prior_checkout(str, &buf) > 0) {\n> -                       detached = (buf.len == 40 && !get_sha1_hex(buf.buf, sha1));\n> +                       detached = get_short_sha1(buf.buf, buf.len, sha1, GET_SHA1_QUIETLY);\n>                         strbuf_release(&buf);\n> -                       if (detached)\n> +                       if (detached != SHORT_NAME_NOT_FOUND)\n\nThe semantic meaning of 'detached' seems less clear now if you have to\ncompare against an enumerated constant to determine the result.  But\nalso, I do not see why you have to test '!= SHORT_NAME_NOT_FOUND' here\nbut you did not have to in the other instance.\n\nI think it would be improved if you did this comparison in the\nassignment of detached so 'detached' could keep its original boolean\nmeaning.\n\nBut anyway, having looked inside get_short_sha1, it really does seem\nto do much more than you want here.\n\n>                                 return 0;\n>                 }\n>         }\n> --\n> 1.8.3.1.438.g96d34e8\n>\n> --\n> To unsubscribe from this list: send the line \"unsubscribe git\" in\n> the body of a message to majordomo@vger.kernel.org\n> More majordomo info at  http://vger.kernel.org/majordomo-info.html\n"},{"id":"221026","messageId":"7vtxkxigrb.fsf@alter.siamese.dyndns.org","threadId":"34151","inReplyTo":"CALkWK0ki9C9OL36b3j14C-mVMPy07Uj5FXcrfMZJs_g3zBRC9A@mail.gmail.com","subject":"Re: [PATCH 0/2] Slightly prettier reflog message from checkout","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-06-17T02:59:52Z","receivedAt":"2013-06-17T02:59:52Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ramkumar Ramachandra <artagnon@gmail.com> writes:\n\n> Junio C Hamano wrote:\n>> I view the two codepaths touched by these patches the other way\n>> around.\n>\n> I see.  Thanks for the early feedback.  I have some doubts.\n>\n>> An abbreviated unique SHA-1 you have today may not be unique\n>> tomorrow.\n\n> When did we guarantee that the messages written by the reflog are invariant?\n\nThat is not the point.  From the proposed log message for your 2/2:\n\n  f855138: checkout: moving from bdff0e3a374617dce784f801b97500d9ba2e4705 to co-reflog\n  f855138: checkout: moving from bdff0e3 to co-reflog\n\nThe \"bdff0e3\" may be a unique abbreviation to identify the commit\nbdff0e3a374617dce784f801b97500d9ba2e4705 when the reflog entry was\nwritten.  But the latter can become meaningless once you have an\nunrelated commit in your history that shares the prefix.\n\nThat is the \"deliberate loss of information\" I saw in the proposal.\nRecording full 40-hex does not have to worry about that issue; you\ndo not even have to argue \"but in this case we do not even have to\nhave unique SHA-1, nobody uses it\" vs. \"some other codepaths you are\nnot aware of may want to take advantage and start using it\".  In\nother words, we will have one less thing we have to worry about.\n"},{"id":"221151","messageId":"CALkWK0=_Ac+dQV23xiXty3peWdhn2iFzgSPb7LYDFmoCzYhxQQ@mail.gmail.com","threadId":"34151","inReplyTo":"CABURp0qs7uPkczmS8EC-iNgCVh2eTaeCKsh7NwVuS4j92jMn+A@mail.gmail.com","subject":"Re: [PATCH 1/2] sha1_name: stop hard-coding 40-character hex checks","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2013-06-18T07:03:22Z","receivedAt":"2013-06-18T07:03:22Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Phil Hord wrote:\n> I share your disdain for the bare '40's in the code.  But I think this\n> code is less clear than the previous version with the magic number.\n\nPlease read the cover-letter: I was just toying around to see if this\nwas a good idea, and Junio points out that it's not.\n"},{"id":"221225","messageId":"7vehbza3yq.fsf@alter.siamese.dyndns.org","threadId":"34151","inReplyTo":"CALkWK0=_Ac+dQV23xiXty3peWdhn2iFzgSPb7LYDFmoCzYhxQQ@mail.gmail.com","subject":"Re: [PATCH 1/2] sha1_name: stop hard-coding 40-character hex checks","fromName":"Junio C Hamano","fromEmail":"junio@pobox.com","sentAt":"2013-06-18T14:27:57Z","receivedAt":"2013-06-18T14:27:57Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ramkumar Ramachandra <artagnon@gmail.com> writes:\n\n> Phil Hord wrote:\n>> I share your disdain for the bare '40's in the code.  But I think this\n>> code is less clear than the previous version with the magic number.\n>\n> Please read the cover-letter: \n\nWhich was...\n\n>> [1/2] is important.  [2/2] is a minor prettification, that wouldn't\n>> have been possible without [1/2].\n\n...and I do not find how the above is connected with\n\n> I was just toying around to see if this\n> was a good idea,...\n\n...this in any way.  Puzzled.\n"}]}