{"thread":{"id":"33742","subject":"[PATCH v2 00/11] sha1_name: improvements","startedAt":"2013-05-07T21:55:00Z","lastAt":"2013-05-08T22:06:16Z","messageCount":26,"participants":["Felipe Contreras","Junio C Hamano","Ramkumar Ramachandra"],"isPatch":true,"patchVersion":2,"patchTotal":11},"messages":[{"id":"216645","messageId":"1367963711-8722-1-git-send-email-felipe.contreras@gmail.com","threadId":"33742","inReplyTo":null,"subject":"[PATCH v2 00/11] sha1_name: improvements","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2013-05-07T21:55:00Z","receivedAt":"2013-05-07T21:55:00Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Hi,\n\nWhile trying to add support for the @ shortcut lots of cleanups arised. Here\nthey are in a single series.\n\nFelipe Contreras (7):\n tests: at-combinations: simplify setup\n tests: at-combinations: check ref names directly\n tests: at-combinations: improve nonsense()\n sha1_name: remove no-op\n sha1_name: remove unnecessary braces\n sha1_name: avoid Yoda conditions\n sha1_name: reorganize get_sha1_basic()\n\nRamkumar Ramachandra (4):\n tests: at-combinations: increase coverage\n tests: at-combinations: @{N} versus HEAD@{N}\n sha1_name: don't waste cycles in the @-parsing loop\n sha1_name: check @{-N} errors sooner\n\n sha1_name.c | 42 +++++++++++++++++++---------------\n t/t1508-at-combinations.sh | 56 +++++++++++++++++++++++++++++++++-------------\n 2 files changed, 64 insertions(+), 34 deletions(-)\n\n-- \n1.8.3.rc0.401.g45bba44\n"},{"id":"216646","messageId":"1367963711-8722-2-git-send-email-felipe.contreras@gmail.com","threadId":"33742","inReplyTo":"1367963711-8722-1-git-send-email-felipe.contreras@gmail.com","subject":"[PATCH v2 01/11] tests: at-combinations: simplify setup","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2013-05-07T21:55:01Z","receivedAt":"2013-05-07T21:55:01Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"The test is setting up an upstream branch, but there's a much simpler\nway of doing that: git branch -u.\n\nSigned-off-by: Ramkumar Ramachandra \nSigned-off-by: Felipe Contreras \n---\n t/t1508-at-combinations.sh | 6 ++----\n 1 file changed, 2 insertions(+), 4 deletions(-)\n\ndiff --git a/t/t1508-at-combinations.sh b/t/t1508-at-combinations.sh\nindex d5d6244..46e3f16 100755\n--- a/t/t1508-at-combinations.sh\n+++ b/t/t1508-at-combinations.sh\n@@ -31,10 +31,8 @@ test_expect_success 'setup' '\n \tgit checkout -b new-branch &&\n \ttest_commit new-one &&\n \ttest_commit new-two &&\n-\tgit config branch.old-branch.remote . &&\n-\tgit config branch.old-branch.merge refs/heads/master &&\n-\tgit config branch.new-branch.remote . &&\n-\tgit config branch.new-branch.merge refs/heads/upstream-branch\n+\tgit branch -u master old-branch &&\n+\tgit branch -u upstream-branch new-branch\n '\n \n check HEAD new-two\n-- \n1.8.3.rc0.401.g45bba44\n"},{"id":"216647","messageId":"1367963711-8722-3-git-send-email-felipe.contreras@gmail.com","threadId":"33742","inReplyTo":"1367963711-8722-1-git-send-email-felipe.contreras@gmail.com","subject":"[PATCH v2 02/11] tests: at-combinations: check ref names directly","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2013-05-07T21:55:02Z","receivedAt":"2013-05-07T21:55:02Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Some committishes might point to the same commit, but through a\ndifferent ref, that's why it's better to check directly for the ref,\nrather than the commit message.\n\nWe can do that by calling rev-parse --symbolic-full-name, and to\ndifferentiate the old from the new behavior we add an extra argument to\nthe check() helper.\n\nSigned-off-by: Ramkumar Ramachandra \nSigned-off-by: Felipe Contreras \n---\n t/t1508-at-combinations.sh | 27 ++++++++++++++++-----------\n 1 file changed, 16 insertions(+), 11 deletions(-)\n\ndiff --git a/t/t1508-at-combinations.sh b/t/t1508-at-combinations.sh\nindex 46e3f16..bd2d2fe 100755\n--- a/t/t1508-at-combinations.sh\n+++ b/t/t1508-at-combinations.sh\n@@ -4,9 +4,14 @@ test_description='test various @{X} syntax combinations together'\n . ./test-lib.sh\n \n check() {\n-test_expect_${3:-success} \"$1 = $2\" \"\n-\techo '$2' >expect &&\n-\tgit log -1 --format=%s '$1' >actual &&\n+test_expect_${4:-success} \"$1 = $3\" \"\n+\tif [ '$2' == 'commit' ]; then\n+\t\techo '$3' >expect &&\n+\t\tgit log -1 --format=%s '$1' >actual\n+\telse\n+\t\techo '$3' >expect &&\n+\t\tgit rev-parse --symbolic-full-name '$1' >actual\n+\tfi &&\n \ttest_cmp expect actual\n \"\n }\n@@ -35,14 +40,14 @@ test_expect_success 'setup' '\n \tgit branch -u upstream-branch new-branch\n '\n \n-check HEAD new-two\n-check \"@{1}\" new-one\n-check \"@{-1}\" old-two\n-check \"@{-1}@{1}\" old-one\n-check \"@{u}\" upstream-two\n-check \"@{u}@{1}\" upstream-one\n-check \"@{-1}@{u}\" master-two\n-check \"@{-1}@{u}@{1}\" master-one\n+check HEAD ref refs/heads/new-branch\n+check \"@{1}\" commit new-one\n+check \"@{-1}\" ref refs/heads/old-branch\n+check \"@{-1}@{1}\" commit old-one\n+check \"@{u}\" ref refs/heads/upstream-branch\n+check \"@{u}@{1}\" commit upstream-one\n+check \"@{-1}@{u}\" ref refs/heads/master\n+check \"@{-1}@{u}@{1}\" commit master-one\n nonsense \"@{u}@{-1}\"\n nonsense \"@{1}@{u}\"\n \n-- \n1.8.3.rc0.401.g45bba44\n"},{"id":"216648","messageId":"1367963711-8722-4-git-send-email-felipe.contreras@gmail.com","threadId":"33742","inReplyTo":"1367963711-8722-1-git-send-email-felipe.contreras@gmail.com","subject":"[PATCH v2 03/11] tests: at-combinations: improve nonsense()","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2013-05-07T21:55:03Z","receivedAt":"2013-05-07T21:55:03Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"In some circumstances 'git log' might fail, but not because the @\nparsing failed. For example: 'git rev-parse' might succeed and return a\nbad object, and then 'git log' would fail.\n\nThe layer we want to test is revision parsing, so let's test that\ndirectly.\n\nSigned-off-by: Ramkumar Ramachandra \nSigned-off-by: Felipe Contreras \n---\n t/t1508-at-combinations.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/t/t1508-at-combinations.sh b/t/t1508-at-combinations.sh\nindex bd2d2fe..2ea735e 100755\n--- a/t/t1508-at-combinations.sh\n+++ b/t/t1508-at-combinations.sh\n@@ -17,7 +17,7 @@ test_expect_${4:-success} \"$1 = $3\" \"\n }\n nonsense() {\n test_expect_${2:-success} \"$1 is nonsensical\" \"\n-\ttest_must_fail git log -1 '$1'\n+\ttest_must_fail git rev-parse '$1'\n \"\n }\n fail() {\n-- \n1.8.3.rc0.401.g45bba44\n"},{"id":"216649","messageId":"1367963711-8722-5-git-send-email-felipe.contreras@gmail.com","threadId":"33742","inReplyTo":"1367963711-8722-1-git-send-email-felipe.contreras@gmail.com","subject":"[PATCH v2 04/11] tests: at-combinations: increase coverage","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2013-05-07T21:55:04Z","receivedAt":"2013-05-07T21:55:04Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"From: Ramkumar Ramachandra \n\nAdd more tests exercising documented functionality.\n\n[fc: commit message and extra tests]\n\nSigned-off-by: Ramkumar Ramachandra \nSigned-off-by: Felipe Contreras \n---\n t/t1508-at-combinations.sh | 8 ++++++++\n 1 file changed, 8 insertions(+)\n\ndiff --git a/t/t1508-at-combinations.sh b/t/t1508-at-combinations.sh\nindex 2ea735e..58cd1fe 100755\n--- a/t/t1508-at-combinations.sh\n+++ b/t/t1508-at-combinations.sh\n@@ -42,13 +42,21 @@ test_expect_success 'setup' '\n \n check HEAD ref refs/heads/new-branch\n check \"@{1}\" commit new-one\n+check \"HEAD@{1}\" commit new-one\n+check \"@{now}\" commit new-two\n+check \"HEAD@{now}\" commit new-two\n check \"@{-1}\" ref refs/heads/old-branch\n+check \"@{-1}@{0}\" commit old-two\n check \"@{-1}@{1}\" commit old-one\n check \"@{u}\" ref refs/heads/upstream-branch\n+check \"HEAD@{u}\" ref refs/heads/upstream-branch\n check \"@{u}@{1}\" commit upstream-one\n check \"@{-1}@{u}\" ref refs/heads/master\n check \"@{-1}@{u}@{1}\" commit master-one\n nonsense \"@{u}@{-1}\"\n+nonsense \"@{0}@{0}\"\n nonsense \"@{1}@{u}\"\n+nonsense \"HEAD@{-1}\"\n+nonsense \"@{-1}@{-1}\"\n \n test_done\n-- \n1.8.3.rc0.401.g45bba44\n"},{"id":"216650","messageId":"1367963711-8722-6-git-send-email-felipe.contreras@gmail.com","threadId":"33742","inReplyTo":"1367963711-8722-1-git-send-email-felipe.contreras@gmail.com","subject":"[PATCH v2 05/11] tests: at-combinations: @{N} versus HEAD@{N}","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2013-05-07T21:55:05Z","receivedAt":"2013-05-07T21:55:05Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"From: Ramkumar Ramachandra \n\nAll the tests so far check that @{N} is the same as HEAD@{N} (for\npositive N). However, this is not always the case; write a couple of\ntests for this.\n\n[fc: simplify some wording]\n\nSigned-off-by: Ramkumar Ramachandra \nSigned-off-by: Felipe Contreras \n---\n t/t1508-at-combinations.sh | 13 +++++++++++++\n 1 file changed, 13 insertions(+)\n\ndiff --git a/t/t1508-at-combinations.sh b/t/t1508-at-combinations.sh\nindex 58cd1fe..13b0372 100755\n--- a/t/t1508-at-combinations.sh\n+++ b/t/t1508-at-combinations.sh\n@@ -59,4 +59,17 @@ nonsense \"@{1}@{u}\"\n nonsense \"HEAD@{-1}\"\n nonsense \"@{-1}@{-1}\"\n \n+# @{N} versus HEAD@{N}\n+\n+check \"HEAD@{3}\" commit old-two\n+nonsense \"@{3}\"\n+\n+test_expect_success 'switch to old-branch' '\n+\tgit checkout old-branch\n+'\n+\n+check HEAD ref refs/heads/old-branch\n+check \"HEAD@{1}\" commit new-two\n+check \"@{1}\" commit old-one\n+\n test_done\n-- \n1.8.3.rc0.401.g45bba44\n"},{"id":"216651","messageId":"1367963711-8722-7-git-send-email-felipe.contreras@gmail.com","threadId":"33742","inReplyTo":"1367963711-8722-1-git-send-email-felipe.contreras@gmail.com","subject":"[PATCH v2 06/11] sha1_name: remove no-op","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2013-05-07T21:55:06Z","receivedAt":"2013-05-07T21:55:06Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"'at' is always 0, since we can reach this point only if\n!len && reflog_len, and len=at when reflog is assigned.\n\nSigned-off-by: Felipe Contreras \n---\n sha1_name.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/sha1_name.c b/sha1_name.c\nindex 3820f28..01e49a9 100644\n--- a/sha1_name.c\n+++ b/sha1_name.c\n@@ -464,7 +464,7 @@ static int get_sha1_basic(const char *str, int len, unsigned char *sha1)\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\tret = interpret_branch_name(str, &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-- \n1.8.3.rc0.401.g45bba44\n"},{"id":"216653","messageId":"1367963711-8722-8-git-send-email-felipe.contreras@gmail.com","threadId":"33742","inReplyTo":"1367963711-8722-1-git-send-email-felipe.contreras@gmail.com","subject":"[PATCH v2 07/11] sha1_name: remove unnecessary braces","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2013-05-07T21:55:07Z","receivedAt":"2013-05-07T21:55:07Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Signed-off-by: Felipe Contreras \n---\n sha1_name.c | 5 ++---\n 1 file changed, 2 insertions(+), 3 deletions(-)\n\ndiff --git a/sha1_name.c b/sha1_name.c\nindex 01e49a9..6530ddd 100644\n--- a/sha1_name.c\n+++ b/sha1_name.c\n@@ -465,12 +465,11 @@ static int get_sha1_basic(const char *str, int len, unsigned char *sha1)\n \t\tint ret;\n \t\t/* try the @{-N} syntax for n-th checkout */\n \t\tret = interpret_branch_name(str, &buf);\n-\t\tif (ret > 0) {\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\telse if (ret == 0)\n \t\t\treturn -1;\n-\t\t}\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-- \n1.8.3.rc0.401.g45bba44\n"},{"id":"216652","messageId":"1367963711-8722-9-git-send-email-felipe.contreras@gmail.com","threadId":"33742","inReplyTo":"1367963711-8722-1-git-send-email-felipe.contreras@gmail.com","subject":"[PATCH v2 08/11] sha1_name: avoid Yoda conditions","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2013-05-07T21:55:08Z","receivedAt":"2013-05-07T21:55:08Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Speak in reverse we shall not; compare variable with constant, not\nconstant with variable.\n\nSigned-off-by: Felipe Contreras \n---\n sha1_name.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/sha1_name.c b/sha1_name.c\nindex 6530ddd..93c4e8c 100644\n--- a/sha1_name.c\n+++ b/sha1_name.c\n@@ -996,9 +996,9 @@ int interpret_branch_name(const char *name, struct strbuf *buf)\n \n \tif (!len)\n \t\treturn len; /* syntax Ok, not enough switches */\n-\tif (0 < len && len == namelen)\n+\tif (len > 0 && len == namelen)\n \t\treturn len; /* consumed all */\n-\telse if (0 < len) {\n+\telse if (len > 0) {\n \t\t/* we have extra data, which might need further processing */\n \t\tstruct strbuf tmp = STRBUF_INIT;\n \t\tint used = buf->len;\n-- \n1.8.3.rc0.401.g45bba44\n"},{"id":"216654","messageId":"1367963711-8722-10-git-send-email-felipe.contreras@gmail.com","threadId":"33742","inReplyTo":"1367963711-8722-1-git-send-email-felipe.contreras@gmail.com","subject":"[PATCH v2 09/11] sha1_name: don't waste cycles in the @-parsing loop","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2013-05-07T21:55:09Z","receivedAt":"2013-05-07T21:55:09Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"From: Ramkumar Ramachandra \n\nThe @-parsing loop unnecessarily checks for the sequence \"@{\" from len -\n2 unnecessarily. We can safely check from len - 4.\n\n[fc: remove cruft]\n\nSigned-off-by: Felipe Contreras \n---\n sha1_name.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/sha1_name.c b/sha1_name.c\nindex 93c4e8c..0acc6e0 100644\n--- a/sha1_name.c\n+++ b/sha1_name.c\n@@ -445,7 +445,7 @@ static int get_sha1_basic(const char *str, int len, unsigned char *sha1)\n \t/* basic@{time or number or -number} format to query ref-log */\n \treflog_len = at = 0;\n \tif (len && str[len-1] == '}') {\n-\t\tfor (at = len-2; at >= 0; at--) {\n+\t\tfor (at = len-4; at >= 0; at--) {\n \t\t\tif (str[at] == '@' && str[at+1] == '{') {\n \t\t\t\tif (!upstream_mark(str + at, len - at)) {\n \t\t\t\t\treflog_len = (len-1) - (at+2);\n-- \n1.8.3.rc0.401.g45bba44\n"},{"id":"216655","messageId":"1367963711-8722-11-git-send-email-felipe.contreras@gmail.com","threadId":"33742","inReplyTo":"1367963711-8722-1-git-send-email-felipe.contreras@gmail.com","subject":"[PATCH v2 10/11] sha1_name: reorganize get_sha1_basic()","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2013-05-07T21:55:10Z","receivedAt":"2013-05-07T21:55:10Z","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 | 30 +++++++++++++++++++-----------\n 1 file changed, 19 insertions(+), 11 deletions(-)\n\ndiff --git a/sha1_name.c b/sha1_name.c\nindex 0acc6e0..c512c69 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-4; 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,19 +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, &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\telse 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":"216656","messageId":"1367963711-8722-12-git-send-email-felipe.contreras@gmail.com","threadId":"33742","inReplyTo":"1367963711-8722-1-git-send-email-felipe.contreras@gmail.com","subject":"[PATCH v2 11/11] sha1_name: check @{-N} errors sooner","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2013-05-07T21:55:11Z","receivedAt":"2013-05-07T21:55:11Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"From: Ramkumar Ramachandra \n\nIt's trivial to check for them in the @{N} parsing loop.\n\n[fc: style]\n\nSigned-off-by: Felipe Contreras \n---\n sha1_name.c | 9 ++++-----\n 1 file changed, 4 insertions(+), 5 deletions(-)\n\ndiff --git a/sha1_name.c b/sha1_name.c\nindex c512c69..db965af 100644\n--- a/sha1_name.c\n+++ b/sha1_name.c\n@@ -448,7 +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-4; 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\tif (str[at+2] == '-') {\n+\t\t\t\t\tif (at != 0)\n+\t\t\t\t\t\t/* @{-N} not at start */\n+\t\t\t\t\t\treturn -1;\n \t\t\t\t\tnth_prior = 1;\n \t\t\t\t\tcontinue;\n \t\t\t\t}\n@@ -497,10 +500,6 @@ static int get_sha1_basic(const char *str, int len, unsigned char *sha1)\n \t\tunsigned long co_time;\n \t\tint co_tz, co_cnt;\n \n-\t\t/* a @{-N} placed anywhere except the start is an error */\n-\t\tif (str[at+2] == '-')\n-\t\t\treturn -1;\n-\n \t\t/* Is it asking for N-th entry, or approxidate? */\n \t\tfor (i = nth = 0; 0 <= nth && i < reflog_len; i++) {\n \t\t\tchar ch = str[at+2+i];\n-- \n1.8.3.rc0.401.g45bba44\n"},{"id":"216662","messageId":"CAMP44s3-vVUB4VnZP4uBMLAbviV+BMTqDcbO_TxkX+5RE6cnSg@mail.gmail.com","threadId":"33742","inReplyTo":"1367963711-8722-1-git-send-email-felipe.contreras@gmail.com","subject":"Re: [PATCH v2 00/11] sha1_name: improvements","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2013-05-07T22:11:56Z","receivedAt":"2013-05-07T22:11:56Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Tue, May 7, 2013 at 4:55 PM, Felipe Contreras\n wrote:\n> While trying to add support for the @ shortcut lots of cleanups arised. Here\n> they are in a single series.\n>\n> Felipe Contreras (7):\n> tests: at-combinations: simplify setup\n> tests: at-combinations: check ref names directly\n> tests: at-combinations: improve nonsense()\n> sha1_name: remove no-op\n> sha1_name: remove unnecessary braces\n> sha1_name: avoid Yoda conditions\n> sha1_name: reorganize get_sha1_basic()\n>\n> Ramkumar Ramachandra (4):\n> tests: at-combinations: increase coverage\n> tests: at-combinations: @{N} versus HEAD@{N}\n> sha1_name: don't waste cycles in the @-parsing loop\n> sha1_name: check @{-N} errors sooner\n\nWhen merging this series to the @ shortcut one, there will be\nconflicts, this is how I propose fixing them:\n\n return len; /* syntax Ok, not enough switches */\n- if (0 < len && len == namelen)\n+ if (len > 0 && len == namelen)\n return len; /* consumed all */\n- else if (0 < len)\n...\n++ else if (len > 0)\n + return reinterpret(name, namelen, len, buf);\n\n- check \"@\" new-two\n- check \"@@{u}\" upstream-two\n...\n++check \"@\" ref refs/heads/new-branch\n++check \"@@{u}\" ref refs/heads/upstream-branch\n\nIf that creates some kind of problem I would rather throw away this\nseries rather than the other one.\n\nCheers.\n\n-- \nFelipe Contreras\n"},{"id":"216680","messageId":"7vfvxyggmb.fsf@alter.siamese.dyndns.org","threadId":"33742","inReplyTo":"1367963711-8722-3-git-send-email-felipe.contreras@gmail.com","subject":"Re: [PATCH v2 02/11] tests: at-combinations: check ref names directly","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-05-08T05:55:40Z","receivedAt":"2013-05-08T05:55:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Felipe Contreras writes:\n\n> Some committishes might point to the same commit, but through a\n> different ref, that's why it's better to check directly for the ref,\n> rather than the commit message.\n>\n> We can do that by calling rev-parse --symbolic-full-name, and to\n> differentiate the old from the new behavior we add an extra argument to\n> the check() helper.\n>\n> Signed-off-by: Ramkumar Ramachandra \n> Signed-off-by: Felipe Contreras \n> ---\n\nIt is signed-off by Ram first but who is the author? You, or him?\n\n> t/t1508-at-combinations.sh | 27 ++++++++++++++++-----------\n> 1 file changed, 16 insertions(+), 11 deletions(-)\n>\n> diff --git a/t/t1508-at-combinations.sh b/t/t1508-at-combinations.sh\n> index 46e3f16..bd2d2fe 100755\n> --- a/t/t1508-at-combinations.sh\n> +++ b/t/t1508-at-combinations.sh\n> @@ -4,9 +4,14 @@ test_description='test various @{X} syntax combinations together'\n> . ./test-lib.sh\n> \n> check() {\n> -test_expect_${3:-success} \"$1 = $2\" \"\n> -\techo '$2' >expect &&\n> -\tgit log -1 --format=%s '$1' >actual &&\n> +test_expect_${4:-success} \"$1 = $3\" \"\n> +\tif [ '$2' == 'commit' ]; then\n> +\t\techo '$3' >expect &&\n> +\t\tgit log -1 --format=%s '$1' >actual\n> +\telse\n> +\t\techo '$3' >expect &&\n> +\t\tgit rev-parse --symbolic-full-name '$1' >actual\n> +\tfi &&\n\nMove the echo outside of if, and match the overall style.\n\n\techo '$3' >expect &&\n if test '$2' = commit\n\tthen\n\t\tgit log ...\n\telse\n\t\tgit rev-parse ...\n\tfi &&\n\n\n> \ttest_cmp expect actual\n> \"\n> }\n> @@ -35,14 +40,14 @@ test_expect_success 'setup' '\n> \tgit branch -u upstream-branch new-branch\n> '\n> \n> -check HEAD new-two\n> -check \"@{1}\" new-one\n> -check \"@{-1}\" old-two\n> -check \"@{-1}@{1}\" old-one\n> -check \"@{u}\" upstream-two\n> -check \"@{u}@{1}\" upstream-one\n> -check \"@{-1}@{u}\" master-two\n> -check \"@{-1}@{u}@{1}\" master-one\n> +check HEAD ref refs/heads/new-branch\n> +check \"@{1}\" commit new-one\n> +check \"@{-1}\" ref refs/heads/old-branch\n> +check \"@{-1}@{1}\" commit old-one\n> +check \"@{u}\" ref refs/heads/upstream-branch\n> +check \"@{u}@{1}\" commit upstream-one\n> +check \"@{-1}@{u}\" ref refs/heads/master\n> +check \"@{-1}@{u}@{1}\" commit master-one\n> nonsense \"@{u}@{-1}\"\n> nonsense \"@{1}@{u}\"\n"},{"id":"216681","messageId":"7v8v3qgglz.fsf@alter.siamese.dyndns.org","threadId":"33742","inReplyTo":"1367963711-8722-4-git-send-email-felipe.contreras@gmail.com","subject":"Re: [PATCH v2 03/11] tests: at-combinations: improve nonsense()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-05-08T05:55:52Z","receivedAt":"2013-05-08T05:55:52Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Felipe Contreras writes:\n\n> In some circumstances 'git log' might fail, but not because the @\n> parsing failed. For example: 'git rev-parse' might succeed and return a\n> bad object, and then 'git log' would fail.\n>\n> The layer we want to test is revision parsing, so let's test that\n> directly.\n\nHmph, but\n\n\tgit rev-parse Makefile\n\nwould happily succeed if there happens to be Makefile in the\ndirectory.\n\nAre we expecting that they are always object names? If that is the\ncase, perhaps\n\n\tgit rev-parse --verify \"$1\"\n\nwould express the intention better.\n\n> Signed-off-by: Ramkumar Ramachandra \n> Signed-off-by: Felipe Contreras \n> ---\n> t/t1508-at-combinations.sh | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/t/t1508-at-combinations.sh b/t/t1508-at-combinations.sh\n> index bd2d2fe..2ea735e 100755\n> --- a/t/t1508-at-combinations.sh\n> +++ b/t/t1508-at-combinations.sh\n> @@ -17,7 +17,7 @@ test_expect_${4:-success} \"$1 = $3\" \"\n> }\n> nonsense() {\n> test_expect_${2:-success} \"$1 is nonsensical\" \"\n> -\ttest_must_fail git log -1 '$1'\n> +\ttest_must_fail git rev-parse '$1'\n> \"\n> }\n> fail() {\n"},{"id":"216682","messageId":"7v1u9igglk.fsf@alter.siamese.dyndns.org","threadId":"33742","inReplyTo":"CAMP44s3-vVUB4VnZP4uBMLAbviV+BMTqDcbO_TxkX+5RE6cnSg@mail.gmail.com","subject":"Re: [PATCH v2 00/11] sha1_name: improvements","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-05-08T05:56:07Z","receivedAt":"2013-05-08T05:56:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Felipe Contreras writes:\n\n> When merging this series to the @ shortcut one, there will be\n> conflicts, this is how I propose fixing them:\n>\n> return len; /* syntax Ok, not enough switches */\n> - if (0 < len && len == namelen)\n> + if (len > 0 && len == namelen)\n> return len; /* consumed all */\n> - else if (0 < len)\n> ...\n> ++ else if (len > 0)\n> + return reinterpret(name, namelen, len, buf);\n>\n> - check \"@\" new-two\n> - check \"@@{u}\" upstream-two\n> ...\n> ++check \"@\" ref refs/heads/new-branch\n> ++check \"@@{u}\" ref refs/heads/upstream-branch\n\nThe resolution for the tests wrapper that acquired an extra\nparameter matches what I did locally. Thanks for a merge sanity\ncheck.\n\nI didn't see any conflicts on the sha1_name.c side, but I applied\nthe Yoda thing slightly differently to result in a slightly more\nstreamlined codeflow:\n\n\tif (!len) {\n\t\treturn len;\n\t} else if (len > 0) {\n\t\tif (len == namelen)\n \treturn len;\n\t\telse\n\t\t\treturn reinterpret(...);\n\t}\n\nwhich I think shows the choices better.\n\nAlthough I haven't looked at the largest one (10/11) carefully,\neverything else looked quite straightforward and readable.\n\nI am not very happy about how $ parameters are quoted in t1508,\nbut that suboptimal quoting were there before this series, and I'd\nconsider it outside of the scope for now.\n\nWill queue. Thanks.\n"},{"id":"216683","messageId":"CAMP44s1fjB_d9enPYdGX1Ycez2PYjG0iss9-PUsb+A+E5hMvEw@mail.gmail.com","threadId":"33742","inReplyTo":"7vfvxyggmb.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v2 02/11] tests: at-combinations: check ref names directly","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2013-05-08T06:03:42Z","receivedAt":"2013-05-08T06:03:42Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Wed, May 8, 2013 at 12:55 AM, Junio C Hamano wrote:\n> Felipe Contreras writes:\n>\n>> Some committishes might point to the same commit, but through a\n>> different ref, that's why it's better to check directly for the ref,\n>> rather than the commit message.\n>>\n>> We can do that by calling rev-parse --symbolic-full-name, and to\n>> differentiate the old from the new behavior we add an extra argument to\n>> the check() helper.\n>>\n>> Signed-off-by: Ramkumar Ramachandra \n>> Signed-off-by: Felipe Contreras \n>> ---\n>\n> It is signed-off by Ram first but who is the author? You, or him?\n\nI am, but he sent the patch first.\n\n>> t/t1508-at-combinations.sh | 27 ++++++++++++++++-----------\n>> 1 file changed, 16 insertions(+), 11 deletions(-)\n>>\n>> diff --git a/t/t1508-at-combinations.sh b/t/t1508-at-combinations.sh\n>> index 46e3f16..bd2d2fe 100755\n>> --- a/t/t1508-at-combinations.sh\n>> +++ b/t/t1508-at-combinations.sh\n>> @@ -4,9 +4,14 @@ test_description='test various @{X} syntax combinations together'\n>> . ./test-lib.sh\n>>\n>> check() {\n>> -test_expect_${3:-success} \"$1 = $2\" \"\n>> - echo '$2' >expect &&\n>> - git log -1 --format=%s '$1' >actual &&\n>> +test_expect_${4:-success} \"$1 = $3\" \"\n>> + if [ '$2' == 'commit' ]; then\n>> + echo '$3' >expect &&\n>> + git log -1 --format=%s '$1' >actual\n>> + else\n>> + echo '$3' >expect &&\n>> + git rev-parse --symbolic-full-name '$1' >actual\n>> + fi &&\n>\n> Move the echo outside of if, and match the overall style.\n\nRight.\n\n-- \nFelipe Contreras\n"},{"id":"216684","messageId":"CAMP44s2S53kUvp3LcYcXdzq=bwP-kG0uZmG+s-u_6mEEjABEBA@mail.gmail.com","threadId":"33742","inReplyTo":"7v8v3qgglz.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v2 03/11] tests: at-combinations: improve nonsense()","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2013-05-08T06:49:54Z","receivedAt":"2013-05-08T06:49:54Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Wed, May 8, 2013 at 12:55 AM, Junio C Hamano wrote:\n> Felipe Contreras writes:\n>\n>> In some circumstances 'git log' might fail, but not because the @\n>> parsing failed. For example: 'git rev-parse' might succeed and return a\n>> bad object, and then 'git log' would fail.\n>>\n>> The layer we want to test is revision parsing, so let's test that\n>> directly.\n>\n> Hmph, but\n>\n> git rev-parse Makefile\n>\n> would happily succeed if there happens to be Makefile in the\n> directory.\n>\n> Are we expecting that they are always object names? If that is the\n> case, perhaps\n>\n> git rev-parse --verify \"$1\"\n>\n> would express the intention better.\n\nProbably, although the same would fail before this patch.\n\n-- \nFelipe Contreras\n"},{"id":"216689","messageId":"CALkWK0ktoQg5pd-tSiP=wn-3JGv-N7OHK_FkNjPx5HycdjBsKw@mail.gmail.com","threadId":"33742","inReplyTo":"1367963711-8722-11-git-send-email-felipe.contreras@gmail.com","subject":"Re: [PATCH v2 10/11] sha1_name: reorganize get_sha1_basic()","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2013-05-08T07:39:03Z","receivedAt":"2013-05-08T07:39:03Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Felipe Contreras wrote:\n> ---\n> sha1_name.c | 30 +++++++++++++++++++-----------\n> 1 file changed, 19 insertions(+), 11 deletions(-)\n\nHow has this changed since my eyeballing of the previous version? An\ninter-diff would be nice: having spent a significant amount of time\nlooking at this area, I can confirm that the patch is Correct.\n"},{"id":"216690","messageId":"CAMP44s0yWXHAPAE3vmFpSpVyHi+SeHJEXBS_y4sJw2eP7H-ADQ@mail.gmail.com","threadId":"33742","inReplyTo":"CALkWK0ktoQg5pd-tSiP=wn-3JGv-N7OHK_FkNjPx5HycdjBsKw@mail.gmail.com","subject":"Re: [PATCH v2 10/11] sha1_name: reorganize get_sha1_basic()","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2013-05-08T07:42:13Z","receivedAt":"2013-05-08T07:42:13Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Wed, May 8, 2013 at 2:39 AM, Ramkumar Ramachandra wrote:\n> Felipe Contreras wrote:\n>> ---\n>> sha1_name.c | 30 +++++++++++++++++++-----------\n>> 1 file changed, 19 insertions(+), 11 deletions(-)\n>\n> How has this changed since my eyeballing of the previous version? An\n> inter-diff would be nice: having spent a significant amount of time\n> looking at this area, I can confirm that the patch is Correct.\n\nYou mean this compared to v4? It hasn't changed.\n\n-- \nFelipe Contreras\n"},{"id":"216693","messageId":"CALkWK0=WRNF_anMFTux0KUr6NUnPKGCKB3RcC-uaSkCu56fhOw@mail.gmail.com","threadId":"33742","inReplyTo":"7v1u9igglk.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v2 00/11] sha1_name: improvements","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2013-05-08T10:44:02Z","receivedAt":"2013-05-08T10:44:02Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Junio C Hamano wrote:\n> Will queue. Thanks.\n\nNit: you might want to add my s-o-b on patches 73027d and b018e8 queued in pu.\n"},{"id":"216720","messageId":"7vbo8lfi8y.fsf@alter.siamese.dyndns.org","threadId":"33742","inReplyTo":"1367963711-8722-11-git-send-email-felipe.contreras@gmail.com","subject":"Re: [PATCH v2 10/11] sha1_name: reorganize get_sha1_basic()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-05-08T18:18:05Z","receivedAt":"2013-05-08T18:18:05Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Felipe Contreras writes:\n\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> ...\n\ns/weather/whether/;\n\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-4; 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\nDoes this have to be inside the loop?\n\n> @@ -460,19 +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, &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\telse 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\nEarlier, if @{-N} resolved to a detached head, we just fed it to\nget_sha1_1(). If it resolved to a concrete refname, we also fed it\nto get_sha1_1(). We ended up calling ourselves again and did the\nright thing either way.\n\nThe new code bypasses the recursive call when we get a detached head\nback, because we know that calling get_sha1_1() with the 40-hex will\neventually take us back to this codepath, and immediately return\nwhen it sees get_sha1_hex() succeeds.\n\nWhat happens when str @{-N} leaves a concrete refname in buf.buf?\nThe branch name is lost with strbuf_release(), and then where do we\ngo from here? Continuing down from here would run dwim_ref/log on\nstr which is still @{-N}, no?\n\nAhh, OK, the new code will now let dwim_ref/log to process @{-N}\nagain (the log message hints this but it wasn't all that clear), so\neven though we already learned the branch name in buf here and\ndiscard it, we will eventually discover the same information later.\n\nThat is somewhat contrived, and I am not so sure if that is a good\nreorganization.\n\nAlso, a few points this patch highlights in the code before the\nchange:\n\n - If we were on a branch with 40-hex name at nth prior checkout,\n would we mistake it as being detached at the commit?\n\n - If we were on a branch 'foo' at nth prior checkout, would our\n previous get_sha1_1() have made us mistake it as referring to a\n tag 'foo' with the same name if it exists?\n\nThe former needs a fix to either writing of reflog or reading by\ninterpret_nth_prior_checkout() so that we can tell these cases apart\nmore reliably. Then the latter can be solved by splicing\nrefs/heads/ in front of the branch name before recursively calling\nget_sha1_1() in the original code (and similar fix can be\nforward-ported to this patch).\n\nIncidentally, I think if we prefix refs/heads/ in front and feed\nthat to dwim_ref/log, we would also avoid running @{-N} twice (which\nI suspect might be more expensive than a simple recursion, as it\nneeds to go through the reflog file), which may be a nice side\neffect of such a fix we would get for free.\n\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"},{"id":"216721","messageId":"7v7gj9fh56.fsf@alter.siamese.dyndns.org","threadId":"33742","inReplyTo":"7vbo8lfi8y.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v2 10/11] sha1_name: reorganize get_sha1_basic()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-05-08T18:41:57Z","receivedAt":"2013-05-08T18:41:57Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Also, a few points this patch highlights in the code before the\n> change:\n>\n> - If we were on a branch with 40-hex name at nth prior checkout,\n> would we mistake it as being detached at the commit?\n>\n> - If we were on a branch 'foo' at nth prior checkout, would our\n> previous get_sha1_1() have made us mistake it as referring to a\n> tag 'foo' with the same name if it exists?\n>\n> The former needs a fix to either writing of reflog or reading by\n> interpret_nth_prior_checkout() so that we can tell these cases apart\n> more reliably. Then the latter can be solved by splicing\n> refs/heads/ in front of the branch name before recursively calling\n> get_sha1_1() in the original code (and similar fix can be\n> forward-ported to this patch).\n>\n> Incidentally, I think if we prefix refs/heads/ in front and feed\n> that to dwim_ref/log, we would also avoid running @{-N} twice (which\n> I suspect might be more expensive than a simple recursion, as it\n> needs to go through the reflog file), which may be a nice side\n> effect of such a fix we would get for free.\n\nHere is the first step (i.e. more reliable interpret_nth_prior).\n\nI looked at all the existing callers of interpret_branch_name() and\nit appears to me that most of them currently assume they are getting\na branch name, because they want to work on a ref, and some of them\ndo not care, because they are working on a revision. For the\nformer, they can (and should) error out instead of relying on not\nhaving refs/heads/$detached_SHA1 that will prevent them from working\non a ref which is what they currently do, if they had the \"detached\"\ninformation. For the latter, if we give \"detached\" information,\nthey can either prefix \"refs/heads/\" (if the result is \"not\ndetached\") to call resolve_ref(), or call get_sha1_hex (if the\nresult is \"detached\"), which would be the solution for the second\nissue I noticed in the message I am replying to.\n\nThe next step on top of this patch may be to expose the \"detached\"\nbit up in the API chain to let callers of interpret_branch_name() to\nknow about the distinction.\n\n sha1_name.c | 12 ++++++++++--\n 1 file changed, 10 insertions(+), 2 deletions(-)\n\ndiff --git a/sha1_name.c b/sha1_name.c\nindex 3820f28..3dd6667 100644\n--- a/sha1_name.c\n+++ b/sha1_name.c\n@@ -862,6 +862,7 @@ static int get_sha1_oneline(const char *prefix, unsigned char *sha1,\n struct grab_nth_branch_switch_cbdata {\n \tint remaining;\n \tstruct strbuf buf;\n+\tint detached;\n };\n \n static int grab_nth_branch_switch(unsigned char *osha1, unsigned char *nsha1,\n@@ -880,9 +881,14 @@ static int grab_nth_branch_switch(unsigned char *osha1, unsigned char *nsha1,\n \tif (!match || !target)\n \t\treturn 0;\n \tif (--(cb->remaining) == 0) {\n+\t\tunsigned char sha1[20];\n+\n \t\tlen = target - match;\n \t\tstrbuf_reset(&cb->buf);\n \t\tstrbuf_add(&cb->buf, match, len);\n+\t\tcb->detached = (len == 40 &&\n+\t\t\t\t!get_sha1_hex(match, sha1) &&\n+\t\t\t\t!hashcmp(osha1, sha1));\n \t\treturn 1; /* we are done */\n \t}\n \treturn 0;\n@@ -892,7 +898,7 @@ static int grab_nth_branch_switch(unsigned char *osha1, unsigned char *nsha1,\n * Parse @{-N} syntax, return the number of characters parsed\n * if successful; otherwise signal an error with negative value.\n */\n-static int interpret_nth_prior_checkout(const char *name, struct strbuf *buf)\n+static int interpret_nth_prior_checkout(const char *name, struct strbuf *buf, int *detached)\n {\n \tlong nth;\n \tint retval;\n@@ -917,6 +923,8 @@ static int interpret_nth_prior_checkout(const char *name, struct strbuf *buf)\n \tif (0 < for_each_reflog_ent_reverse(\"HEAD\", grab_nth_branch_switch, &cb)) {\n \t\tstrbuf_reset(buf);\n \t\tstrbuf_add(buf, cb.buf.buf, cb.buf.len);\n+\t\tif (detached)\n+\t\t\t*detached = cb.detached;\n \t\tretval = brace - name + 1;\n \t}\n \n@@ -992,7 +1000,7 @@ int interpret_branch_name(const char *name, struct strbuf *buf)\n \tchar *cp;\n \tstruct branch *upstream;\n \tint namelen = strlen(name);\n-\tint len = interpret_nth_prior_checkout(name, buf);\n+\tint len = interpret_nth_prior_checkout(name, buf, NULL);\n \tint tmp_len;\n \n \tif (!len)\n"},{"id":"216725","messageId":"CAMP44s38eJP6WRQTQMDRqo-AXb7-YE1ZS-tJ7NK_QRwgHB3Obw@mail.gmail.com","threadId":"33742","inReplyTo":"7vbo8lfi8y.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v2 10/11] sha1_name: reorganize get_sha1_basic()","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2013-05-08T20:39:25Z","receivedAt":"2013-05-08T20:39:25Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Wed, May 8, 2013 at 1:18 PM, Junio C Hamano wrote:\n> Felipe Contreras writes:\n>\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>> ...\n>\n> s/weather/whether/;\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-4; 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> Does this have to be inside the loop?\n\nYes, the whole purpose is to avoid reflog_len to be set.\n\n>> @@ -460,19 +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>> struct strbuf buf = STRBUF_INIT;\n>> - int ret;\n>> - /* try the @{-N} syntax for n-th checkout */\n>> - ret = interpret_branch_name(str, &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>> + }\n>\n> Earlier, if @{-N} resolved to a detached head, we just fed it to\n> get_sha1_1(). If it resolved to a concrete refname, we also fed it\n> to get_sha1_1(). We ended up calling ourselves again and did the\n> right thing either way.\n>\n> The new code bypasses the recursive call when we get a detached head\n> back, because we know that calling get_sha1_1() with the 40-hex will\n> eventually take us back to this codepath, and immediately return\n> when it sees get_sha1_hex() succeeds.\n>\n> What happens when str @{-N} leaves a concrete refname in buf.buf?\n> The branch name is lost with strbuf_release(), and then where do we\n> go from here? Continuing down from here would run dwim_ref/log on\n> str which is still @{-N}, no?\n>\n> Ahh, OK, the new code will now let dwim_ref/log to process @{-N}\n> again (the log message hints this but it wasn't all that clear),\n\nI thought it was clear we would let dwim_{ref,log} do the job:\n\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---\n\n> That is somewhat contrived, and I am not so sure if that is a good\n> reorganization.\n\nBut much less contrived than before, because the code that deals with\n@{-N} is in one place, instead of sprinkled all over as many\ncorner-cases, and there's no recursion.\n\n> Also, a few points this patch highlights in the code before the\n> change:\n>\n> - If we were on a branch with 40-hex name at nth prior checkout,\n> would we mistake it as being detached at the commit?\n>\n> - If we were on a branch 'foo' at nth prior checkout, would our\n> previous get_sha1_1() have made us mistake it as referring to a\n> tag 'foo' with the same name if it exists?\n\nI don't know, but I suspect there's no change after this patch.\n\n-- \nFelipe Contreras\n"},{"id":"216729","messageId":"7vsj1xcf81.fsf@alter.siamese.dyndns.org","threadId":"33742","inReplyTo":"CAMP44s38eJP6WRQTQMDRqo-AXb7-YE1ZS-tJ7NK_QRwgHB3Obw@mail.gmail.com","subject":"Re: [PATCH v2 10/11] sha1_name: reorganize get_sha1_basic()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-05-08T21:51:42Z","receivedAt":"2013-05-08T21:51:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Felipe Contreras writes:\n\n> On Wed, May 8, 2013 at 1:18 PM, Junio C Hamano wrote:\n>> Felipe Contreras writes:\n>>\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>>> ...\n>>\n>> s/weather/whether/;\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-4; 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>> Does this have to be inside the loop?\n>\n> Yes, the whole purpose is to avoid reflog_len to be set.\n\nWhat I meant was the \"@{-\" check, which happens only at==0.\n\n\tif (!memcmp(str, \"@{-\", 3) && len > 3)\n\t\tnth_prior = 1;\n\telse\n\t\tfor (at = len - 4; at; at--) {\n\t\t\t... look for and break at the first \"@{\" ...\n\t\t}\n\nor something.\n\n>> Ahh, OK, the new code will now let dwim_ref/log to process @{-N}\n>> again (the log message hints this but it wasn't all that clear),\n>\n> I thought it was clear we would let dwim_{ref,log} do the job:\n\nYes, the reason I did not immediately think of that is because I\nknew @{-N} was expensive (need to read reflog backwards) and didn't\nthink anybody would redo the code to deliberately do that twice ;-)\n\n>> Also, a few points this patch highlights in the code before the\n>> change:\n>>\n>> - If we were on a branch with 40-hex name at nth prior checkout,\n>> would we mistake it as being detached at the commit?\n>>\n>> - If we were on a branch 'foo' at nth prior checkout, would our\n>> previous get_sha1_1() have made us mistake it as referring to a\n>> tag 'foo' with the same name if it exists?\n>\n> I don't know, but I suspect there's no change after this patch.\n\nYes, didn't I say \"the code before the change\" above?\n\nThese two correctness issues look more important issues to me, with\nor without the restructure patch (in other words, they are\nindependent).\n"},{"id":"216730","messageId":"CAMP44s0_h0gLyknHytdTeoA3E4e4hYYUP-_PKjJE9jWbUaYuPg@mail.gmail.com","threadId":"33742","inReplyTo":"7vsj1xcf81.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v2 10/11] sha1_name: reorganize get_sha1_basic()","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2013-05-08T22:06:16Z","receivedAt":"2013-05-08T22:06:16Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Wed, May 8, 2013 at 4:51 PM, Junio C Hamano wrote:\n> Felipe Contreras writes:\n>\n>> On Wed, May 8, 2013 at 1:18 PM, Junio C Hamano wrote:\n>>> Felipe Contreras writes:\n>>>\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>>>> ...\n>>>\n>>> s/weather/whether/;\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-4; 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>>> Does this have to be inside the loop?\n>>\n>> Yes, the whole purpose is to avoid reflog_len to be set.\n>\n> What I meant was the \"@{-\" check, which happens only at==0.\n>\n> if (!memcmp(str, \"@{-\", 3) && len > 3)\n> nth_prior = 1;\n> else\n> for (at = len - 4; at; at--) {\n> ... look for and break at the first \"@{\" ...\n> }\n>\n> or something.\n\nThat's doable, but would screw up the next patch.\n\n>>> Ahh, OK, the new code will now let dwim_ref/log to process @{-N}\n>>> again (the log message hints this but it wasn't all that clear),\n>>\n>> I thought it was clear we would let dwim_{ref,log} do the job:\n>\n> Yes, the reason I did not immediately think of that is because I\n> knew @{-N} was expensive (need to read reflog backwards) and didn't\n> think anybody would redo the code to deliberately do that twice ;-)\n\nBut that's what the commit message said.\n\n>>> Also, a few points this patch highlights in the code before the\n>>> change:\n>>>\n>>> - If we were on a branch with 40-hex name at nth prior checkout,\n>>> would we mistake it as being detached at the commit?\n>>>\n>>> - If we were on a branch 'foo' at nth prior checkout, would our\n>>> previous get_sha1_1() have made us mistake it as referring to a\n>>> tag 'foo' with the same name if it exists?\n>>\n>> I don't know, but I suspect there's no change after this patch.\n>\n> Yes, didn't I say \"the code before the change\" above?\n>\n> These two correctness issues look more important issues to me, with\n> or without the restructure patch (in other words, they are\n> independent).\n\nRight, if you are interested in correctness, you might want to try\n@{-1}{0}; it resolves to @{-1} currently, and it fails correctly with\nmy patch.\n\nCheers.\n\n-- \nFelipe Contreras\n"}]}