{"thread":{"id":"33702","subject":"[PATCH v2] sha1_name: reorganize get_sha1_basic()","startedAt":"2013-05-02T17:48:39Z","lastAt":"2013-05-02T19:59:29Z","messageCount":7,"participants":["Felipe Contreras","Ramkumar Ramachandra"],"isPatch":true,"patchVersion":2,"patchTotal":null},"messages":[{"id":"216293","messageId":"1367516919-4735-1-git-send-email-felipe.contreras@gmail.com","threadId":"33702","inReplyTo":null,"subject":"[PATCH v2] sha1_name: reorganize get_sha1_basic()","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2013-05-02T17:48:39Z","receivedAt":"2013-05-02T17:48:39Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Through the years the functionality to handle @{-N} and @{u} has moved\naround the code, and as a result, code that once made sense, doesn't any\nmore.\n\nThere is no need to call this function recursively with the branch of\n@{-N} substituted because dwim_{ref,log} already replaces it.\n\nHowever, there's one corner-case where @{-N} resolves to a detached\nHEAD, in which case we wouldn't get any ref back.\n\nSo we parse the nth-prior manually, and deal with it depending on\nweather it's a SHA-1, or a ref.\n\nSigned-off-by: Felipe Contreras \n---\n sha1_name.c | 29 ++++++++++++++++++-----------\n 1 file changed, 18 insertions(+), 11 deletions(-)\n\ndiff --git a/sha1_name.c b/sha1_name.c\nindex 3820f28..6428001 100644\n--- a/sha1_name.c\n+++ b/sha1_name.c\n@@ -431,13 +431,14 @@ static inline int upstream_mark(const char *string, int len)\n }\n \n static int get_sha1_1(const char *name, int len, unsigned char *sha1, unsigned lookup_flags);\n+static int interpret_nth_prior_checkout(const char *name, struct strbuf *buf);\n \n static int get_sha1_basic(const char *str, int len, unsigned char *sha1)\n {\n \tstatic const char *warn_msg = \"refname '%.*s' is ambiguous.\";\n \tchar *real_ref = NULL;\n \tint refs_found = 0;\n-\tint at, reflog_len;\n+\tint at, reflog_len, nth_prior = 0;\n \n \tif (len == 40 && !get_sha1_hex(str, sha1))\n \t\treturn 0;\n@@ -447,6 +448,10 @@ static int get_sha1_basic(const char *str, int len, unsigned char *sha1)\n \tif (len && str[len-1] == '}') {\n \t\tfor (at = len-2; at >= 0; at--) {\n \t\t\tif (str[at] == '@' && str[at+1] == '{') {\n+\t\t\t\tif (at == 0 && str[2] == '-') {\n+\t\t\t\t\tnth_prior = 1;\n+\t\t\t\t\tcontinue;\n+\t\t\t\t}\n \t\t\t\tif (!upstream_mark(str + at, len - at)) {\n \t\t\t\t\treflog_len = (len-1) - (at+2);\n \t\t\t\t\tlen = at;\n@@ -460,20 +465,22 @@ static int get_sha1_basic(const char *str, int len, unsigned char *sha1)\n \tif (len && ambiguous_path(str, len))\n \t\treturn -1;\n \n-\tif (!len && reflog_len) {\n+\tif (nth_prior) {\n \t\tstruct strbuf buf = STRBUF_INIT;\n-\t\tint ret;\n-\t\t/* try the @{-N} syntax for n-th checkout */\n-\t\tret = interpret_branch_name(str+at, &buf);\n-\t\tif (ret > 0) {\n-\t\t\t/* substitute this branch name and restart */\n-\t\t\treturn get_sha1_1(buf.buf, buf.len, sha1, 0);\n-\t\t} else if (ret == 0) {\n-\t\t\treturn -1;\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\tstrbuf_release(&buf);\n+\t\t\tif (detached)\n+\t\t\t\treturn 0;\n \t\t}\n+\t}\n+\n+\tif (!len && reflog_len)\n \t\t/* allow \"@{...}\" to mean the current branch reflog */\n \t\trefs_found = dwim_ref(\"HEAD\", 4, sha1, &real_ref);\n-\t} else if (reflog_len)\n+\telse if (reflog_len)\n \t\trefs_found = dwim_log(str, len, sha1, &real_ref);\n \telse\n \t\trefs_found = dwim_ref(str, len, sha1, &real_ref);\n-- \n1.8.3.rc0.401.g45bba44\n"},{"id":"216296","messageId":"CAMP44s1mm-uEnVQSwN5ORy8kqe9q7w_PKGYgOhKHcXbbkAtkUQ@mail.gmail.com","threadId":"33702","inReplyTo":"1367516919-4735-1-git-send-email-felipe.contreras@gmail.com","subject":"Re: [PATCH v2] sha1_name: reorganize get_sha1_basic()","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2013-05-02T18:02:48Z","receivedAt":"2013-05-02T18:02:48Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Thu, May 2, 2013 at 12:48 PM, Felipe Contreras\n wrote:\n> Through the years the functionality to handle @{-N} and @{u} has moved\n> around the code, and as a result, code that once made sense, doesn't any\n> more.\n>\n> There is no need to call this function recursively with the branch of\n> @{-N} substituted because dwim_{ref,log} already replaces it.\n>\n> However, there's one corner-case where @{-N} resolves to a detached\n> HEAD, in which case we wouldn't get any ref back.\n>\n> So we parse the nth-prior manually, and deal with it depending on\n> weather it's a SHA-1, or a ref.\n\nForgot again: Inspired by a patch from Ramkumar Ramachandra.\n\n-- \nFelipe Contreras\n"},{"id":"216300","messageId":"CALkWK0k4YisL0Nc6906FRsx7556UVqy_DD9Wo=1j4OWMkwqK-A@mail.gmail.com","threadId":"33702","inReplyTo":"1367516919-4735-1-git-send-email-felipe.contreras@gmail.com","subject":"Re: [PATCH v2] sha1_name: reorganize get_sha1_basic()","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2013-05-02T18:55:01Z","receivedAt":"2013-05-02T18:55:01Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Felipe Contreras wrote:\n> [...]\n\nOkay, you used nth_prior in this one.\n\n> There is no need to call this function recursively with the branch of\n> @{-N} substituted because dwim_{ref,log} already replaces it.\n\nI figured that the recursion is because dwim_{ref,log} didn't exist\nwhen this was written.\n\n> However, there's one corner-case where @{-N} resolves to a detached\n> HEAD, in which case we wouldn't get any ref back.\n>\n> So we parse the nth-prior manually, and deal with it depending on\n> weather it's a SHA-1, or a ref.\n\nRight. _This_ is the special case, which the old logic didn't quite\nconvey. The end-user version of this is: 'git checkout -' won't bring\nyou back to the branch if you said git checkout HEAD~1 earlier.\n\n> diff --git a/sha1_name.c b/sha1_name.c\n> index 3820f28..6428001 100644\n> --- a/sha1_name.c\n> +++ b/sha1_name.c\n> @@ -431,13 +431,14 @@ static inline int upstream_mark(const char *string, int len)\n> }\n>\n> static int get_sha1_1(const char *name, int len, unsigned char *sha1, unsigned lookup_flags);\n> +static int interpret_nth_prior_checkout(const char *name, struct strbuf *buf);\n\nIt didn't strike me to use interpret_nth_prior_checkout() directly. I\nwas still stuck at interpret_branch_name() returning a positive value.\n\n> static int get_sha1_basic(const char *str, int len, unsigned char *sha1)\n> {\n> static const char *warn_msg = \"refname '%.*s' is ambiguous.\";\n> char *real_ref = NULL;\n> int refs_found = 0;\n> - int at, reflog_len;\n> + int at, reflog_len, nth_prior = 0;\n>\n> if (len == 40 && !get_sha1_hex(str, sha1))\n> return 0;\n> @@ -447,6 +448,10 @@ static int get_sha1_basic(const char *str, int len, unsigned char *sha1)\n> if (len && str[len-1] == '}') {\n> for (at = len-2; at >= 0; at--) {\n> if (str[at] == '@' && str[at+1] == '{') {\n> + if (at == 0 && str[2] == '-') {\n> + nth_prior = 1;\n> + continue;\n> + }\n\nLooking at this closely once again.\nYou've already hit the beginning. What are you continuing? Take the\nexample of a compound expression with @{-\n\n@{-1}@{0}\n ^ at is here\n \"@{-\" is not matched\n\n@{-1}@{0}\n^ at is here\n\"@{-\" is matched\nWhat's to continue? at is already at 0\n\nOn another note, I think you've fixed a bug: @{-1}{0} was parsing to\nthe same value as @{-1}@{0} before your patch.\n\n> @@ -460,20 +465,22 @@ static int get_sha1_basic(const char *str, int len, unsigned char *sha1)\n> if (len && ambiguous_path(str, len))\n> return -1;\n>\n> - if (!len && reflog_len) {\n> + if (nth_prior) {\n\nnth_prior makes this much cleaner overall.\n\n> struct strbuf buf = STRBUF_INIT;\n> - int ret;\n> - /* try the @{-N} syntax for n-th checkout */\n> - ret = interpret_branch_name(str+at, &buf);\n> - if (ret > 0) {\n> - /* substitute this branch name and restart */\n> - return get_sha1_1(buf.buf, buf.len, sha1, 0);\n> - } else if (ret == 0) {\n> - return -1;\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> + strbuf_release(&buf);\n> + if (detached)\n> + return 0;\n\nNeat. I'd set reflog_len to zero and made sure that the last part of\nthe function wouldn't be executed. How did you get away without\nsetting refs_found to 1 though?\n\n> }\n> + }\n> +\n> + if (!len && reflog_len)\n> /* allow \"@{...}\" to mean the current branch reflog */\n> refs_found = dwim_ref(\"HEAD\", 4, sha1, &real_ref);\n\nI got this part wrong too: I said dwim_log() instead of dwim_ref().\n\n> - } else if (reflog_len)\n> + else if (reflog_len)\n> refs_found = dwim_log(str, len, sha1, &real_ref);\n> else\n> refs_found = dwim_ref(str, len, sha1, &real_ref);\n> --\n> 1.8.3.rc0.401.g45bba44\n>\n"},{"id":"216303","messageId":"CAMP44s3UTwgcxQ0Y89UtjBAw46ps_2EOGqZ=OG499G_uYL0DCA@mail.gmail.com","threadId":"33702","inReplyTo":"CALkWK0k4YisL0Nc6906FRsx7556UVqy_DD9Wo=1j4OWMkwqK-A@mail.gmail.com","subject":"Re: [PATCH v2] sha1_name: reorganize get_sha1_basic()","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2013-05-02T19:10:18Z","receivedAt":"2013-05-02T19:10:18Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Thu, May 2, 2013 at 1:55 PM, Ramkumar Ramachandra wrote:\n> Felipe Contreras wrote:\n>> [...]\n>\n> Okay, you used nth_prior in this one.\n>\n>> There is no need to call this function recursively with the branch of\n>> @{-N} substituted because dwim_{ref,log} already replaces it.\n>\n> I figured that the recursion is because dwim_{ref,log} didn't exist\n> when this was written.\n\nThey did, but they were not substituting the branches.\n\n>> @@ -447,6 +448,10 @@ static int get_sha1_basic(const char *str, int len, unsigned char *sha1)\n>> if (len && str[len-1] == '}') {\n>> for (at = len-2; at >= 0; at--) {\n>> if (str[at] == '@' && str[at+1] == '{') {\n>> + if (at == 0 && str[2] == '-') {\n>> + nth_prior = 1;\n>> + continue;\n>> + }\n>\n> Looking at this closely once again.\n> You've already hit the beginning. What are you continuing? Take the\n> example of a compound expression with @{-\n\nYeah, we could break, but I would prefer the break to happen naturally\nwhen in the for loop check.\n\n> On another note, I think you've fixed a bug: @{-1}{0} was parsing to\n> the same value as @{-1}@{0} before your patch.\n\nYeap.\n\n>> @@ -460,20 +465,22 @@ static int get_sha1_basic(const char *str, int len, unsigned char *sha1)\n>> if (len && ambiguous_path(str, len))\n>> return -1;\n>>\n>> - if (!len && reflog_len) {\n>> + if (nth_prior) {\n>\n> nth_prior makes this much cleaner overall.\n>\n>> struct strbuf buf = STRBUF_INIT;\n>> - int ret;\n>> - /* try the @{-N} syntax for n-th checkout */\n>> - ret = interpret_branch_name(str+at, &buf);\n>> - if (ret > 0) {\n>> - /* substitute this branch name and restart */\n>> - return get_sha1_1(buf.buf, buf.len, sha1, 0);\n>> - } else if (ret == 0) {\n>> - return -1;\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>> + strbuf_release(&buf);\n>> + if (detached)\n>> + return 0;\n>\n> Neat. I'd set reflog_len to zero and made sure that the last part of\n> the function wouldn't be executed. How did you get away without\n> setting refs_found to 1 though?\n\nThe rest of the code is not executed, there's no need if @{-N}\nevaluates to a SHA-1. There's no ref to dwim, and there's no reflog\nanyway. We just fetch the SHA-1 and return.\n\n>> }\n>> + }\n>> +\n>> + if (!len && reflog_len)\n>> /* allow \"@{...}\" to mean the current branch reflog */\n>> refs_found = dwim_ref(\"HEAD\", 4, sha1, &real_ref);\n>\n> I got this part wrong too: I said dwim_log() instead of dwim_ref().\n\nFortunately we are not changing the code this time, which is the best\nway to make sure that the behavior doesn't change.\n\nIt took me a long time to play with alternatives and find a clean\nsolution with minimal changes that is easy to understand, but I think\nthis code does the trick.\n\nCheers.\n\n-- \nFelipe Contreras\n"},{"id":"216304","messageId":"CALkWK0=1qxWWRT9ic_4uVg_ZF_5T0_ogpWwTHotqxWqsi9GOnA@mail.gmail.com","threadId":"33702","inReplyTo":"CAMP44s3UTwgcxQ0Y89UtjBAw46ps_2EOGqZ=OG499G_uYL0DCA@mail.gmail.com","subject":"Re: [PATCH v2] sha1_name: reorganize get_sha1_basic()","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2013-05-02T19:35:17Z","receivedAt":"2013-05-02T19:35:17Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Felipe Contreras wrote:\n>> Looking at this closely once again.\n>> You've already hit the beginning. What are you continuing? Take the\n>> example of a compound expression with @{-\n>\n> Yeah, we could break, but I would prefer the break to happen naturally\n> when in the for loop check.\n\nThis is followed by a condition on upstream_mark: just change that\nfrom if/else if/ and there's a break; at the end anyway.\n\nThe continue is misleading and should be removed.\n\n>> On another note, I think you've fixed a bug: @{-1}{0} was parsing to\n>> the same value as @{-1}@{0} before your patch.\n>\n> Yeap.\n\nWrite a note about it in the commit message atleast? I found it to be\na very non-trivial conclusion.\n\n>>> + if (interpret_nth_prior_checkout(str, &buf) > 0) {\n>>> + detached = (buf.len == 40 && !get_sha1_hex(buf.buf, sha1));\n>>> + strbuf_release(&buf);\n>>> + if (detached)\n>>> + return 0;\n>>\n>> Neat. I'd set reflog_len to zero and made sure that the last part of\n>> the function wouldn't be executed. How did you get away without\n>> setting refs_found to 1 though?\n>\n> The rest of the code is not executed, there's no need if @{-N}\n> evaluates to a SHA-1. There's no ref to dwim, and there's no reflog\n> anyway. We just fetch the SHA-1 and return.\n\nObviously the return 0 breaks out of the function. I meant what\nhappens if it's not detached. I'll answer it myself: you have a\nstring that's either not 40-characters, or doesn't resolve to a valid\nobject. You haven't found anything yet. Now, you'll be going down\nthe reflog_len codepath and calling dwim_log() to set the refs_found.\n"},{"id":"216310","messageId":"1367524078-13266-1-git-send-email-artagnon@gmail.com","threadId":"33702","inReplyTo":"CALkWK0=1qxWWRT9ic_4uVg_ZF_5T0_ogpWwTHotqxWqsi9GOnA@mail.gmail.com","subject":"Re: [PATCH v2] sha1_name: reorganize get_sha1_basic()","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2013-05-02T19:47:58Z","receivedAt":"2013-05-02T19:47:58Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"A small suggestion. Squash this in if you like; optionally submit it\nas a separate part.\n\ndiff --git a/sha1_name.c b/sha1_name.c\nindex 6428001..109ab41 100644\n--- a/sha1_name.c\n+++ b/sha1_name.c\n@@ -448,11 +448,12 @@ static int get_sha1_basic(const char *str, int len, unsigned char *sha1)\n if (len && str[len-1] == '}') {\n for (at = len-2; at >= 0; at--) {\n if (str[at] == '@' && str[at+1] == '{') {\n- if (at == 0 && str[2] == '-') {\n- nth_prior = 1;\n- continue;\n- }\n- if (!upstream_mark(str + at, len - at)) {\n+ if (str[at+2] == '-') {\n+ if (at != 0)\n+ return -1;\n+ else\n+ nth_prior = 1;\n+ } else if (!upstream_mark(str + at, len - at)) {\n reflog_len = (len-1) - (at+2);\n len = at;\n }\n@@ -497,10 +498,6 @@ static int get_sha1_basic(const char *str, int len, unsigned char *sha1)\n unsigned long co_time;\n int co_tz, co_cnt;\n \n- /* a @{-N} placed anywhere except the start is an error */\n- if (str[at+2] == '-')\n- return -1;\n-\n /* Is it asking for N-th entry, or approxidate? */\n for (i = nth = 0; 0 <= nth && i < reflog_len; i++) {\n char ch = str[at+2+i];\n"},{"id":"216311","messageId":"CAMP44s3-TM1PW6qKAFRf=Sdojw3RNscM_VJZfBy3C7xHZst_Wg@mail.gmail.com","threadId":"33702","inReplyTo":"1367524078-13266-1-git-send-email-artagnon@gmail.com","subject":"Re: [PATCH v2] sha1_name: reorganize get_sha1_basic()","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2013-05-02T19:59:29Z","receivedAt":"2013-05-02T19:59:29Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Thu, May 2, 2013 at 2:47 PM, Ramkumar Ramachandra wrote:\n> A small suggestion. Squash this in if you like; optionally submit it\n> as a separate part.\n>\n> diff --git a/sha1_name.c b/sha1_name.c\n> index 6428001..109ab41 100644\n> --- a/sha1_name.c\n> +++ b/sha1_name.c\n> @@ -448,11 +448,12 @@ static int get_sha1_basic(const char *str, int len, unsigned char *sha1)\n> if (len && str[len-1] == '}') {\n> for (at = len-2; at >= 0; at--) {\n> if (str[at] == '@' && str[at+1] == '{') {\n> - if (at == 0 && str[2] == '-') {\n> - nth_prior = 1;\n> - continue;\n> - }\n> - if (!upstream_mark(str + at, len - at)) {\n> + if (str[at+2] == '-') {\n> + if (at != 0)\n> + return -1;\n> + else\n> + nth_prior = 1;\n> + } else if (!upstream_mark(str + at, len - at)) {\n\nGenerally I don't like this style, it's prone to multiple indentation levels.\n\n if (str[at] == '@' && str[at+1] == '{') {\n- if (at == 0 && str[2] == '-') {\n+ if (str[at+2] == '-') {\n+ if (at != 0)\n+ /* @{-N} not at start */\n+ return -1;\n nth_prior = 1;\n continue;\n }\n\nOtherwise makes sense, but I would do it as a separate patch.\n\n-- \nFelipe Contreras\n"}]}