{"thread":{"id":"48852","subject":"[PATCH] sha1-name.c: for \":/\", find detached HEAD commits","startedAt":"2018-07-10T15:42:22Z","lastAt":"2018-07-13T19:40:47Z","messageCount":19,"participants":["William Chargin","Jeff King","Junio C Hamano","Duy Nguyen"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"352117","messageId":"20180710154106.5356-1-wchargin@gmail.com","threadId":"48852","inReplyTo":null,"subject":"[PATCH] sha1-name.c: for \":/\", find detached HEAD commits","fromName":"William Chargin","fromEmail":"wchargin@gmail.com","sentAt":"2018-07-10T15:41:06Z","receivedAt":"2018-07-10T15:42:22Z","isPatch":true,"sender":{"key":"wchargin@gmail.com","avatar":"https://avatars.githubusercontent.com/u/4317806?v=4"},"body":"This patch broadens the set of commits matched by \":/\" pathspecs to\ninclude commits reachable from HEAD but not any named ref. This avoids\nsurprising behavior when working with a detached HEAD and trying to\nrefer to a commit that was recently created and only exists within the\ndetached state.\n\nSigned-off-by: William Chargin \nBased-on-patch-by: Jeff King \n---\n Documentation/revisions.txt | 10 +++++-----\n sha1-name.c | 1 +\n t/t4208-log-magic-pathspec.sh | 14 ++++++++++++++\n 3 files changed, 20 insertions(+), 5 deletions(-)\n\ndiff --git a/Documentation/revisions.txt b/Documentation/revisions.txt\nindex 7d1bd4409..aa56907fd 100644\n--- a/Documentation/revisions.txt\n+++ b/Documentation/revisions.txt\n@@ -181,11 +181,11 @@ existing tag object.\n the '' before '{caret}'.\n \n ':/', e.g. ':/fix nasty bug'::\n- A colon, followed by a slash, followed by a text, names\n- a commit whose commit message matches the specified regular expression.\n- This name returns the youngest matching commit which is\n- reachable from any ref. The regular expression can match any part of the\n- commit message. To match messages starting with a string, one can use\n+ A colon, followed by a slash, followed by a text, names a commit whose\n+ commit message matches the specified regular expression. This name\n+ returns the youngest matching commit which is reachable from any ref\n+ or from HEAD. The regular expression can match any part of the commit\n+ message. To match messages starting with a string, one can use\n e.g. ':/^foo'. The special sequence ':/!' is reserved for modifiers to what\n is matched. ':/!-foo' performs a negative match, while ':/!!foo' matches a\n literal '!' character, followed by 'foo'. Any other sequence beginning with\ndiff --git a/sha1-name.c b/sha1-name.c\nindex 60d9ef3c7..641ca12f9 100644\n--- a/sha1-name.c\n+++ b/sha1-name.c\n@@ -1650,6 +1650,7 @@ static int get_oid_with_context_1(const char *name,\n \t\t\tstruct commit_list *list = NULL;\n \n \t\t\tfor_each_ref(handle_one_ref, &list);\n+\t\t\thead_ref(handle_one_ref, &list);\n \t\t\tcommit_list_sort_by_date(&list);\n \t\t\treturn get_oid_oneline(name + 2, oid, list);\n \t\t}\ndiff --git a/t/t4208-log-magic-pathspec.sh b/t/t4208-log-magic-pathspec.sh\nindex 62f335b2d..41b9f3eba 100755\n--- a/t/t4208-log-magic-pathspec.sh\n+++ b/t/t4208-log-magic-pathspec.sh\n@@ -25,6 +25,20 @@ test_expect_success '\"git log :/a -- \" should not be ambiguous' '\n \tgit log :/a --\n '\n \n+test_expect_success '\"git log :/detached -- \" should find a commit only in HEAD' '\n+\ttest_when_finished \"git checkout master\" &&\n+\tgit checkout --detach &&\n+\ttest_tick &&\n+\tgit commit --allow-empty -m detached &&\n+\ttest_tick &&\n+\tgit commit --allow-empty -m something-else &&\n+\tgit log :/detached --\n+'\n+\n+test_expect_success '\"git log :/detached -- \" should not find an orphaned commit' '\n+\ttest_must_fail git log :/detached --\n+'\n+\n test_expect_success '\"git log -- :/a\" should not be ambiguous' '\n \tgit log -- :/a\n '\n-- \n2.18.0.130.g61d0c9dcf\n\n"},{"id":"352150","messageId":"20180710175447.GB25861@sigill.intra.peff.net","threadId":"48852","inReplyTo":"20180710154106.5356-1-wchargin@gmail.com","subject":"Re: [PATCH] sha1-name.c: for \":/\", find detached HEAD commits","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-07-10T17:54:47Z","receivedAt":"2018-07-10T18:21:31Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jul 10, 2018 at 08:41:06AM -0700, William Chargin wrote:\n\n> This patch broadens the set of commits matched by \":/\" pathspecs to\n> include commits reachable from HEAD but not any named ref. This avoids\n> surprising behavior when working with a detached HEAD and trying to\n> refer to a commit that was recently created and only exists within the\n> detached state.\n> \n> Signed-off-by: William Chargin \n> Based-on-patch-by: Jeff King \n\nThanks, this looks perfect!\n\nJust to be clear on licensing, I'll add my:\n\n Signed-off-by: Jeff King \n\n> diff --git a/t/t4208-log-magic-pathspec.sh b/t/t4208-log-magic-pathspec.sh\n> index 62f335b2d..41b9f3eba 100755\n> --- a/t/t4208-log-magic-pathspec.sh\n> +++ b/t/t4208-log-magic-pathspec.sh\n> @@ -25,6 +25,20 @@ test_expect_success '\"git log :/a -- \" should not be ambiguous' '\n> \tgit log :/a --\n> '\n> \n> +test_expect_success '\"git log :/detached -- \" should find a commit only in HEAD' '\n> +\ttest_when_finished \"git checkout master\" &&\n> +\tgit checkout --detach &&\n> +\ttest_tick &&\n> +\tgit commit --allow-empty -m detached &&\n> +\ttest_tick &&\n> +\tgit commit --allow-empty -m something-else &&\n> +\tgit log :/detached --\n> +'\n\nAt first I wondered if you could just use test_commit here (instead of a\nmanual test_tick and commit). But we care about reachability only from\nthe detached HEAD here, so this must _not_ use test_commit, which\ncreates an extra tag.\n\nIt could be worth mentioning that in a comment (since somebody who later\nrefactors to make that change would not realize the problem, as it would\nsimply cause a broken test to return a false success). But...\n\n> +test_expect_success '\"git log :/detached -- \" should not find an orphaned commit' '\n> +\ttest_must_fail git log :/detached --\n> +'\n\nThis follow-up test _would_ notice such a breakage. Very nicely done.\n\n-Peff\n"},{"id":"352166","messageId":"xmqqwou26hcv.fsf@gitster-ct.c.googlers.com","threadId":"48852","inReplyTo":"20180710154106.5356-1-wchargin@gmail.com","subject":"Re: [PATCH] sha1-name.c: for \":/\", find detached HEAD commits","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-07-10T19:06:08Z","receivedAt":"2018-07-10T19:06:14Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"William Chargin writes:\n\n> This patch broadens the set of commits matched by \":/\" pathspecs to\n> include commits reachable from HEAD but not any named ref. This avoids\n> surprising behavior when working with a detached HEAD and trying to\n> refer to a commit that was recently created and only exists within the\n> detached state.\n>\n> Signed-off-by: William Chargin \n> Based-on-patch-by: Jeff King \n> ---\n> Documentation/revisions.txt | 10 +++++-----\n> sha1-name.c | 1 +\n> t/t4208-log-magic-pathspec.sh | 14 ++++++++++++++\n> 3 files changed, 20 insertions(+), 5 deletions(-)\n>\n> diff --git a/Documentation/revisions.txt b/Documentation/revisions.txt\n> index 7d1bd4409..aa56907fd 100644\n> --- a/Documentation/revisions.txt\n> +++ b/Documentation/revisions.txt\n> @@ -181,11 +181,11 @@ existing tag object.\n> the '' before '{caret}'.\n> \n> ':/', e.g. ':/fix nasty bug'::\n> - A colon, followed by a slash, followed by a text, names\n> - a commit whose commit message matches the specified regular expression.\n> - This name returns the youngest matching commit which is\n> - reachable from any ref. The regular expression can match any part of the\n> - commit message. To match messages starting with a string, one can use\n> + A colon, followed by a slash, followed by a text, names a commit whose\n> + commit message matches the specified regular expression. This name\n> + returns the youngest matching commit which is reachable from any ref\n> + or from HEAD. The regular expression can match any part of the commit\n> + message. To match messages starting with a string, one can use\n\nPlease avoid unnecessary reflowing of earlier lines of the paragrpah\nwhen the only change is to insert \"or from HEAD\" in the middle of its\nfourth line. That wastes reviewers time (and typically there are\nmore readers than there are writers). If you did this instead:\n\n A colon, followed by a slash, followed by a text, names\n a commit whose commit message matches the specified regular expression.\n This name returns the youngest matching commit which is\n reachable from any ref or from HEAD.\n The regular expression can match any part of the\n commit message. To match messages starting with a string, one can use\n\nthe resulting patch would have been much easier to follow.\n\nAlso, I am not sure if \"or from HEAD\" is even needed when we say\n\"from ANY ref\" already, as we count things like HEAD as part of the\nref namespace. It may only feel necessary for the person who made\nthe change to call handle_one_ref() not only with for_each_ref() but\nalso with head_ref()---in other words, I am wondering if it was\nmerely a bug to ignore HEAD in the code when the documentation said\n\"from any ref\" already. If that is the case, then the above change\nis not necessary.\n\n> diff --git a/sha1-name.c b/sha1-name.c\n> index 60d9ef3c7..641ca12f9 100644\n> --- a/sha1-name.c\n> +++ b/sha1-name.c\n> @@ -1650,6 +1650,7 @@ static int get_oid_with_context_1(const char *name,\n> \t\t\tstruct commit_list *list = NULL;\n> \n> \t\t\tfor_each_ref(handle_one_ref, &list);\n> +\t\t\thead_ref(handle_one_ref, &list);\n\nMakes sense.\n\n> \t\t\tcommit_list_sort_by_date(&list);\n> \t\t\treturn get_oid_oneline(name + 2, oid, list);\n> \t\t}\n> diff --git a/t/t4208-log-magic-pathspec.sh b/t/t4208-log-magic-pathspec.sh\n> index 62f335b2d..41b9f3eba 100755\n> --- a/t/t4208-log-magic-pathspec.sh\n> +++ b/t/t4208-log-magic-pathspec.sh\n> @@ -25,6 +25,20 @@ test_expect_success '\"git log :/a -- \" should not be ambiguous' '\n> \tgit log :/a --\n> '\n> \n> +test_expect_success '\"git log :/detached -- \" should find a commit only in HEAD' '\n> +\ttest_when_finished \"git checkout master\" &&\n> +\tgit checkout --detach &&\n> +\ttest_tick &&\n> +\tgit commit --allow-empty -m detached &&\n> +\ttest_tick &&\n> +\tgit commit --allow-empty -m something-else &&\n> +\tgit log :/detached --\n> +'\n> +\n> +test_expect_success '\"git log :/detached -- \" should not find an orphaned commit' '\n> +\ttest_must_fail git log :/detached --\n> +'\n> +\n\nOK.\n\nI found these tests (including the existing ones) misleading, by the\nway. We are interested in seeing that \":/string\" is understood as a\nvalid object name, and this is not limited to \"log\" at all. Instead\nof \"git log\", perhaps \"git rev-parse --verify :/detached\" may have\nbeen more to the point of what we are actually trying to see.\n\nBut that is a fault of having test for \":/\" in \"magic pathspec\"\ntest script; correcting that mistake is totally outside of the scope\nof this fix.\n\nThanks.\n"},{"id":"352201","messageId":"CAFW+GMAP6hwU1frm-FUVqx4pokpSQajWz7N6mGtMC-EO+01m2Q@mail.gmail.com","threadId":"48852","inReplyTo":"xmqqwou26hcv.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH] sha1-name.c: for \":/\", find detached HEAD commits","fromName":"William Chargin","fromEmail":"wchargin@gmail.com","sentAt":"2018-07-11T06:18:22Z","receivedAt":"2018-07-11T06:24:44Z","isPatch":true,"sender":{"key":"wchargin@gmail.com","avatar":"https://avatars.githubusercontent.com/u/4317806?v=4"},"body":"> we care about reachability only from the detached HEAD here, so this\n> must _not_ use test_commit, which creates an extra tag.\n\nRight. I can add a comment to that effect.\n\n> Please avoid unnecessary reflowing of earlier lines of the paragrpah\n> when the only change is to insert \"or from HEAD\" in the middle of its\n> fourth line.\n\nSure: I'll make that change. (My intent was to incrementally clean up an\narea already under change, but I'm happy to instead make only the\nminimal change.)\n\n> Also, I am not sure if \"or from HEAD\" is even needed when we say\n> \"from ANY ref\" already, as we count things like HEAD as part of the\n> ref namespace.\n\nMy two cents: with the docs as is, I wasn't sure whether HEAD was\nintended to count as a ref for this purpose. The gitglossary man page\ndefines a ref as a \"name that begins with refs/\" (seemingly excluding\nHEAD), though it later says that HEAD is a \"special-purpose ref\". In my\nopinion, the change adds clarity without any particular downside---but\nI'm happy to revert it if you'd prefer. I'd also be happy to change the\nwording to something like \"any ref, including HEAD\" if we want to\nemphasize that HEAD really is a ref.\n\n> We are interested in seeing that \":/string\" is understood as a valid\n> object name, and this is not limited to \"log\" at all.\n\nIndeed. I was a bit surprised for the same reason (I expected the tests\nto be using `rev-parse`), but I agree that it's probably best to keep\nthe existing structure to minimize churn.\n\n---\n\nAfter reaching consensus on the change to the docs, should I send in a\n[PATCH v2] In-Reply-To this thread? Peff, should I add your\nSigned-off-by to the commit message, or is that not how things are done?\n\nBest,\nWC\n"},{"id":"352230","messageId":"20180711123213.GA23835@sigill.intra.peff.net","threadId":"48852","inReplyTo":"CAFW+GMAP6hwU1frm-FUVqx4pokpSQajWz7N6mGtMC-EO+01m2Q@mail.gmail.com","subject":"Re: [PATCH] sha1-name.c: for \":/\", find detached HEAD commits","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-07-11T12:32:13Z","receivedAt":"2018-07-11T12:32:17Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jul 10, 2018 at 11:18:22PM -0700, William Chargin wrote:\n\n> > Also, I am not sure if \"or from HEAD\" is even needed when we say\n> > \"from ANY ref\" already, as we count things like HEAD as part of the\n> > ref namespace.\n> \n> My two cents: with the docs as is, I wasn't sure whether HEAD was\n> intended to count as a ref for this purpose. The gitglossary man page\n> defines a ref as a \"name that begins with refs/\" (seemingly excluding\n> HEAD), though it later says that HEAD is a \"special-purpose ref\". In my\n> opinion, the change adds clarity without any particular downside---but\n> I'm happy to revert it if you'd prefer. I'd also be happy to change the\n> wording to something like \"any ref, including HEAD\" if we want to\n> emphasize that HEAD really is a ref.\n\nFWIW, I think the clarification to include HEAD is helpful here, since\nit took me a few minutes of thinking to decide whether the current\nbehavior was a bug or just a subtlety. Your \"including HEAD\" suggestion\nseems like the best route to me. But I can live with it either way.\n\n> After reaching consensus on the change to the docs, should I send in a\n> [PATCH v2] In-Reply-To this thread?\n\nYes.\n\n> Peff, should I add your\n> Signed-off-by to the commit message, or is that not how things are done?\n\nYes, you can add in any sign-offs that have been explicitly given. It's\nnormal to order them chronologically, too (so mine would come first,\nthen yours, showing that the patch flowed through me to you; Junio will\nadd his at the end).\n\n-Peff\n"},{"id":"352261","messageId":"CACsJy8CBJvpKAAtVjrdcuO06WossxcMmqwmSRL9Shr7g+qVySA@mail.gmail.com","threadId":"48852","inReplyTo":"20180710154106.5356-1-wchargin@gmail.com","subject":"Re: [PATCH] sha1-name.c: for \":/\", find detached HEAD commits","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2018-07-11T15:45:09Z","receivedAt":"2018-07-11T15:45:46Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Tue, Jul 10, 2018 at 5:44 PM William Chargin wrote:\n>\n> This patch broadens the set of commits matched by \":/\" pathspecs to\n> include commits reachable from HEAD but not any named ref. This avoids\n> surprising behavior when working with a detached HEAD and trying to\n> refer to a commit that was recently created and only exists within the\n> detached state.\n\nCool!\n\n>\n> Signed-off-by: William Chargin \n> Based-on-patch-by: Jeff King \n> ---\n> Documentation/revisions.txt | 10 +++++-----\n> sha1-name.c | 1 +\n> t/t4208-log-magic-pathspec.sh | 14 ++++++++++++++\n> 3 files changed, 20 insertions(+), 5 deletions(-)\n>\n> diff --git a/Documentation/revisions.txt b/Documentation/revisions.txt\n> index 7d1bd4409..aa56907fd 100644\n> --- a/Documentation/revisions.txt\n> +++ b/Documentation/revisions.txt\n> @@ -181,11 +181,11 @@ existing tag object.\n> the '' before '{caret}'.\n>\n> ':/', e.g. ':/fix nasty bug'::\n> - A colon, followed by a slash, followed by a text, names\n> - a commit whose commit message matches the specified regular expression.\n> - This name returns the youngest matching commit which is\n> - reachable from any ref. The regular expression can match any part of the\n> - commit message. To match messages starting with a string, one can use\n> + A colon, followed by a slash, followed by a text, names a commit whose\n> + commit message matches the specified regular expression. This name\n> + returns the youngest matching commit which is reachable from any ref\n> + or from HEAD. The regular expression can match any part of the commit\n> + message. To match messages starting with a string, one can use\n> e.g. ':/^foo'. The special sequence ':/!' is reserved for modifiers to what\n> is matched. ':/!-foo' performs a negative match, while ':/!!foo' matches a\n> literal '!' character, followed by 'foo'. Any other sequence beginning with\n> diff --git a/sha1-name.c b/sha1-name.c\n> index 60d9ef3c7..641ca12f9 100644\n> --- a/sha1-name.c\n> +++ b/sha1-name.c\n> @@ -1650,6 +1650,7 @@ static int get_oid_with_context_1(const char *name,\n> struct commit_list *list = NULL;\n>\n> for_each_ref(handle_one_ref, &list);\n> + head_ref(handle_one_ref, &list);\n\nWhen multiple worktrees are used, should we consider all HEADs or just\ncurrent worktree's HEAD?\n\nThis is the latter case, if we should go with the former (I don't\nknow, it's a genuine question) then you need one more call:\nother_head_refs().\n\n> commit_list_sort_by_date(&list);\n> return get_oid_oneline(name + 2, oid, list);\n> }\n> diff --git a/t/t4208-log-magic-pathspec.sh b/t/t4208-log-magic-pathspec.sh\n> index 62f335b2d..41b9f3eba 100755\n> --- a/t/t4208-log-magic-pathspec.sh\n> +++ b/t/t4208-log-magic-pathspec.sh\n> @@ -25,6 +25,20 @@ test_expect_success '\"git log :/a -- \" should not be ambiguous' '\n> git log :/a --\n> '\n>\n> +test_expect_success '\"git log :/detached -- \" should find a commit only in HEAD' '\n> + test_when_finished \"git checkout master\" &&\n> + git checkout --detach &&\n> + test_tick &&\n> + git commit --allow-empty -m detached &&\n> + test_tick &&\n> + git commit --allow-empty -m something-else &&\n> + git log :/detached --\n> +'\n> +\n> +test_expect_success '\"git log :/detached -- \" should not find an orphaned commit' '\n> + test_must_fail git log :/detached --\n> +'\n> +\n> test_expect_success '\"git log -- :/a\" should not be ambiguous' '\n> git log -- :/a\n> '\n> --\n> 2.18.0.130.g61d0c9dcf\n>\n\n\n-- \nDuy\n"},{"id":"352267","messageId":"xmqq601l4toz.fsf@gitster-ct.c.googlers.com","threadId":"48852","inReplyTo":"20180711123213.GA23835@sigill.intra.peff.net","subject":"Re: [PATCH] sha1-name.c: for \":/\", find detached HEAD commits","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-07-11T16:34:52Z","receivedAt":"2018-07-11T16:34:57Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Tue, Jul 10, 2018 at 11:18:22PM -0700, William Chargin wrote:\n>\n>> > Also, I am not sure if \"or from HEAD\" is even needed when we say\n>> > \"from ANY ref\" already, as we count things like HEAD as part of the\n>> > ref namespace.\n>> \n>> My two cents: with the docs as is, I wasn't sure whether HEAD was\n>> intended to count as a ref for this purpose. The gitglossary man page\n>> defines a ref as a \"name that begins with refs/\" (seemingly excluding\n>> HEAD), though it later says that HEAD is a \"special-purpose ref\". In my\n>> opinion, the change adds clarity without any particular downside---but\n>> I'm happy to revert it if you'd prefer. I'd also be happy to change the\n>> wording to something like \"any ref, including HEAD\" if we want to\n>> emphasize that HEAD really is a ref.\n>\n> FWIW, I think the clarification to include HEAD is helpful here, since\n> it took me a few minutes of thinking to decide whether the current\n> behavior was a bug or just a subtlety. Your \"including HEAD\" suggestion\n> seems like the best route to me. But I can live with it either way.\n>\n>> After reaching consensus on the change to the docs, should I send in a\n>> [PATCH v2] In-Reply-To this thread?\n>\n> Yes.\n>\n>> Peff, should I add your\n>> Signed-off-by to the commit message, or is that not how things are done?\n>\n> Yes, you can add in any sign-offs that have been explicitly given. It's\n> normal to order them chronologically, too (so mine would come first,\n> then yours, showing that the patch flowed through me to you; Junio will\n> add his at the end).\n\nThanks, agreed 100% and I have nothing more to add.\n"},{"id":"352271","messageId":"20180711164041.GB20988@sigill.intra.peff.net","threadId":"48852","inReplyTo":"CACsJy8CBJvpKAAtVjrdcuO06WossxcMmqwmSRL9Shr7g+qVySA@mail.gmail.com","subject":"Re: [PATCH] sha1-name.c: for \":/\", find detached HEAD commits","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-07-11T16:40:41Z","receivedAt":"2018-07-11T16:40:44Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jul 11, 2018 at 05:45:09PM +0200, Duy Nguyen wrote:\n\n> > diff --git a/sha1-name.c b/sha1-name.c\n> > index 60d9ef3c7..641ca12f9 100644\n> > --- a/sha1-name.c\n> > +++ b/sha1-name.c\n> > @@ -1650,6 +1650,7 @@ static int get_oid_with_context_1(const char *name,\n> > struct commit_list *list = NULL;\n> >\n> > for_each_ref(handle_one_ref, &list);\n> > + head_ref(handle_one_ref, &list);\n> \n> When multiple worktrees are used, should we consider all HEADs or just\n> current worktree's HEAD?\n> \n> This is the latter case, if we should go with the former (I don't\n> know, it's a genuine question) then you need one more call:\n> other_head_refs().\n\nOof, I didn't even think about other worktrees. I'd probably say \"yes\",\nas we consider them reachable (and really the intent of this feature\nseems to be \"find it in any reachable commit\").\n\n-Peff\n"},{"id":"352275","messageId":"xmqqfu0p3czt.fsf@gitster-ct.c.googlers.com","threadId":"48852","inReplyTo":"CACsJy8CBJvpKAAtVjrdcuO06WossxcMmqwmSRL9Shr7g+qVySA@mail.gmail.com","subject":"Re: [PATCH] sha1-name.c: for \":/\", find detached HEAD commits","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-07-11T17:20:54Z","receivedAt":"2018-07-11T17:20:59Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Duy Nguyen writes:\n\n>> diff --git a/sha1-name.c b/sha1-name.c\n>> index 60d9ef3c7..641ca12f9 100644\n>> --- a/sha1-name.c\n>> +++ b/sha1-name.c\n>> @@ -1650,6 +1650,7 @@ static int get_oid_with_context_1(const char *name,\n>> struct commit_list *list = NULL;\n>>\n>> for_each_ref(handle_one_ref, &list);\n>> + head_ref(handle_one_ref, &list);\n>\n> When multiple worktrees are used, should we consider all HEADs or just\n> current worktree's HEAD?\n\nDoes for_each_ref() iterate over per-worktree refs (like \"bisect\",\nperhaps)? If so, then looking in different worktree's HEADs would\nmake sense, and otherwise not.\n\nI would think that the whole point of detaching HEAD in a separate\nworktree is that you can avoid exposing the work you do while\ndetached to other worktrees by doing so, so from that point of view,\nI would probably prefer :/ not to look into other worktrees, but\nthat is not a very strong preference. If peeking all over the place\nis easier to implement, then a minor information leaking is not\nsomething I'd lose sleep over.\n\nThanks for bringing up an interesting issue.\n\n\n"},{"id":"352279","messageId":"CACsJy8AMwKHUyUe-4-MKLn1D3x-+iiFW-Wdjj+HeX_JZ=MkO+w@mail.gmail.com","threadId":"48852","inReplyTo":"xmqqfu0p3czt.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH] sha1-name.c: for \":/\", find detached HEAD commits","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2018-07-11T17:37:58Z","receivedAt":"2018-07-11T17:38:27Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Wed, Jul 11, 2018 at 7:20 PM Junio C Hamano wrote:\n>\n> Duy Nguyen writes:\n>\n> >> diff --git a/sha1-name.c b/sha1-name.c\n> >> index 60d9ef3c7..641ca12f9 100644\n> >> --- a/sha1-name.c\n> >> +++ b/sha1-name.c\n> >> @@ -1650,6 +1650,7 @@ static int get_oid_with_context_1(const char *name,\n> >> struct commit_list *list = NULL;\n> >>\n> >> for_each_ref(handle_one_ref, &list);\n> >> + head_ref(handle_one_ref, &list);\n> >\n> > When multiple worktrees are used, should we consider all HEADs or just\n> > current worktree's HEAD?\n>\n> Does for_each_ref() iterate over per-worktree refs (like \"bisect\",\n> perhaps)?\n\nNo.\n\n> If so, then looking in different worktree's HEADs would\n> make sense, and otherwise not.\n>\n> I would think that the whole point of detaching HEAD in a separate\n> worktree is that you can avoid exposing the work you do while\n> detached to other worktrees by doing so, so from that point of view,\n> I would probably prefer :/ not to look into other worktrees, but\n> that is not a very strong preference.\n\nYeah at least for me worktrees are still quite isolated. Occasionally\nI need to peek in a worktree from another one (e.g. if I want to run\ntests on current HEAD but do not want to wait for it to finish, I'll\nmake a new worktree, checking out the same head and run tests there)\nbut it's really rare. So yeah maybe it's best to stick to current\nworktree's HEAD only. At least until someone finds a good case for it.\n\n> If peeking all over the place\n> is easier to implement, then a minor information leaking is not\n> something I'd lose sleep over.\n>\n> Thanks for bringing up an interesting issue.\n-- \nDuy\n"},{"id":"352281","messageId":"CAFW+GMD62V=o4hoEYKVteBZHzqBtquzLzTv2WXiSPZf3ZhOpeg@mail.gmail.com","threadId":"48852","inReplyTo":"CACsJy8AMwKHUyUe-4-MKLn1D3x-+iiFW-Wdjj+HeX_JZ=MkO+w@mail.gmail.com","subject":"Re: [PATCH] sha1-name.c: for \":/\", find detached HEAD commits","fromName":"William Chargin","fromEmail":"wchargin@gmail.com","sentAt":"2018-07-11T17:49:18Z","receivedAt":"2018-07-11T17:49:40Z","isPatch":true,"sender":{"key":"wchargin@gmail.com","avatar":"https://avatars.githubusercontent.com/u/4317806?v=4"},"body":"> > Does for_each_ref() iterate over per-worktree refs (like \"bisect\",\n> > perhaps)?\n>\n> No.\n\nTo be clear: it iterates only over the per-worktree refs for the current\nworktree. So, if you mark an unreachable commit as \"bad\" with bisect,\nthen that commit is reachable (and found by \":/\") in the enclosing\nworktree only.\n\nWith this patch, \":/\" now behaves the same way with HEADs, which makes\nsense to me. Agreed with the above discussion.\n\nI can add a test for this.\n"},{"id":"352322","messageId":"20180712054909.29077-1-wchargin@gmail.com","threadId":"48852","inReplyTo":"CAFW+GMD62V=o4hoEYKVteBZHzqBtquzLzTv2WXiSPZf3ZhOpeg@mail.gmail.com","subject":"[PATCH v2] sha1-name.c: for \":/\", find detached HEAD commits","fromName":"William Chargin","fromEmail":"wchargin@gmail.com","sentAt":"2018-07-12T05:49:09Z","receivedAt":"2018-07-12T05:49:38Z","isPatch":true,"sender":{"key":"wchargin@gmail.com","avatar":"https://avatars.githubusercontent.com/u/4317806?v=4"},"body":"This patch broadens the set of commits matched by \":/\" pathspecs to\ninclude commits reachable from HEAD but not any named ref. This avoids\nsurprising behavior when working with a detached HEAD and trying to\nrefer to a commit that was recently created and only exists within the\ndetached state.\n\nIf multiple worktrees exist, only the current worktree's HEAD is\nconsidered reachable. This is consistent with the existing behavior for\nother per-worktree refs: e.g., bisect refs are considered reachable, but\nonly within the relevant worktree.\n\nSigned-off-by: Jeff King \nSigned-off-by: William Chargin \nBased-on-patch-by: Jeff King \n---\n Documentation/revisions.txt | 2 +-\n sha1-name.c | 1 +\n t/t4208-log-magic-pathspec.sh | 26 ++++++++++++++++++++++++++\n 3 files changed, 28 insertions(+), 1 deletion(-)\n\ndiff --git a/Documentation/revisions.txt b/Documentation/revisions.txt\nindex 7d1bd4409..aa9579eba 100644\n--- a/Documentation/revisions.txt\n+++ b/Documentation/revisions.txt\n@@ -184,7 +184,7 @@ existing tag object.\n A colon, followed by a slash, followed by a text, names\n a commit whose commit message matches the specified regular expression.\n This name returns the youngest matching commit which is\n- reachable from any ref. The regular expression can match any part of the\n+ reachable from any ref, including HEAD. The regular expression can match any part of the\n commit message. To match messages starting with a string, one can use\n e.g. ':/^foo'. The special sequence ':/!' is reserved for modifiers to what\n is matched. ':/!-foo' performs a negative match, while ':/!!foo' matches a\ndiff --git a/sha1-name.c b/sha1-name.c\nindex 60d9ef3c7..641ca12f9 100644\n--- a/sha1-name.c\n+++ b/sha1-name.c\n@@ -1650,6 +1650,7 @@ static int get_oid_with_context_1(const char *name,\n \t\t\tstruct commit_list *list = NULL;\n \n \t\t\tfor_each_ref(handle_one_ref, &list);\n+\t\t\thead_ref(handle_one_ref, &list);\n \t\t\tcommit_list_sort_by_date(&list);\n \t\t\treturn get_oid_oneline(name + 2, oid, list);\n \t\t}\ndiff --git a/t/t4208-log-magic-pathspec.sh b/t/t4208-log-magic-pathspec.sh\nindex 62f335b2d..4c8f3b8e1 100755\n--- a/t/t4208-log-magic-pathspec.sh\n+++ b/t/t4208-log-magic-pathspec.sh\n@@ -25,6 +25,32 @@ test_expect_success '\"git log :/a -- \" should not be ambiguous' '\n \tgit log :/a --\n '\n \n+test_expect_success '\"git log :/detached -- \" should find a commit only in HEAD' '\n+\ttest_when_finished \"git checkout master\" &&\n+\tgit checkout --detach &&\n+\t# Must manually call `test_tick` instead of using `test_commit`,\n+\t# because the latter additionally creates a tag, which would make\n+\t# the commit reachable not only via HEAD.\n+\ttest_tick &&\n+\tgit commit --allow-empty -m detached &&\n+\ttest_tick &&\n+\tgit commit --allow-empty -m something-else &&\n+\tgit log :/detached --\n+'\n+\n+test_expect_success '\"git log :/detached -- \" should not find an orphaned commit' '\n+\ttest_must_fail git log :/detached --\n+'\n+\n+test_expect_success '\"git log :/detached -- \" should find HEAD only of own worktree' '\n+\tgit worktree add other-tree HEAD &&\n+\tgit -C other-tree checkout --detach &&\n+\ttest_tick &&\n+\tgit -C other-tree commit --allow-empty -m other-detached &&\n+\tgit -C other-tree log :/other-detached -- &&\n+\ttest_must_fail git log :/other-detached --\n+'\n+\n test_expect_success '\"git log -- :/a\" should not be ambiguous' '\n \tgit log -- :/a\n '\n-- \n2.18.0.130.g84cda7581\n\n"},{"id":"352353","messageId":"20180712154920.GA6380@sigill.intra.peff.net","threadId":"48852","inReplyTo":"20180712054909.29077-1-wchargin@gmail.com","subject":"Re: [PATCH v2] sha1-name.c: for \":/\", find detached HEAD commits","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-07-12T15:49:20Z","receivedAt":"2018-07-12T15:49:24Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jul 11, 2018 at 10:49:09PM -0700, William Chargin wrote:\n\n> This patch broadens the set of commits matched by \":/\" pathspecs to\n> include commits reachable from HEAD but not any named ref. This avoids\n> surprising behavior when working with a detached HEAD and trying to\n> refer to a commit that was recently created and only exists within the\n> detached state.\n> \n> If multiple worktrees exist, only the current worktree's HEAD is\n> considered reachable. This is consistent with the existing behavior for\n> other per-worktree refs: e.g., bisect refs are considered reachable, but\n> only within the relevant worktree.\n> \n> Signed-off-by: Jeff King \n> Signed-off-by: William Chargin \n> Based-on-patch-by: Jeff King \n> ---\n> Documentation/revisions.txt | 2 +-\n> sha1-name.c | 1 +\n> t/t4208-log-magic-pathspec.sh | 26 ++++++++++++++++++++++++++\n> 3 files changed, 28 insertions(+), 1 deletion(-)\n\nThis one looks good to me. Thanks.\n\n-Peff\n"},{"id":"352357","messageId":"xmqqva9kzbhi.fsf@gitster-ct.c.googlers.com","threadId":"48852","inReplyTo":"20180712054909.29077-1-wchargin@gmail.com","subject":"Re: [PATCH v2] sha1-name.c: for \":/\", find detached HEAD commits","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-07-12T16:04:41Z","receivedAt":"2018-07-12T16:04:45Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"William Chargin writes:\n\n> This patch broadens the set of commits matched by \":/\" pathspecs to\n\nAs we discussed during the review on v1, \":/\"\nis *NOT* pathspec (that is why having these tests in t4208 is wrong\nbut we are following existing mistakes). It is a way to specify a\ncommit object name (unfortunately \"extended SHA-1\" is the phrase we\noften call the various ways to name an object that are implemented\nin sha1-name.c).\n\n> include commits reachable from HEAD but not any named ref. This avoids\n> surprising behavior when working with a detached HEAD and trying to\n> refer to a commit that was recently created and only exists within the\n> detached state.\n>\n> If multiple worktrees exist, only the current worktree's HEAD is\n> considered reachable. This is consistent with the existing behavior for\n> other per-worktree refs: e.g., bisect refs are considered reachable, but\n> only within the relevant worktree.\n>\n> Signed-off-by: Jeff King \n> Signed-off-by: William Chargin \n> Based-on-patch-by: Jeff King \n\nNow you have Peff's sign-off for the one-liner code change, the last\none is redundant.\n\n> ---\n\nOther than the above two nits, the patch looks good. I would have\nbroken the line after \"including HEAD.\", but the slightly-long line\nis also OK.\n\n> Documentation/revisions.txt | 2 +-\n> sha1-name.c | 1 +\n> t/t4208-log-magic-pathspec.sh | 26 ++++++++++++++++++++++++++\n> 3 files changed, 28 insertions(+), 1 deletion(-)\n>\n> diff --git a/Documentation/revisions.txt b/Documentation/revisions.txt\n> index 7d1bd4409..aa9579eba 100644\n> --- a/Documentation/revisions.txt\n> +++ b/Documentation/revisions.txt\n> @@ -184,7 +184,7 @@ existing tag object.\n> A colon, followed by a slash, followed by a text, names\n> a commit whose commit message matches the specified regular expression.\n> This name returns the youngest matching commit which is\n> - reachable from any ref. The regular expression can match any part of the\n> + reachable from any ref, including HEAD. The regular expression can match any part of the\n> commit message. To match messages starting with a string, one can use\n> e.g. ':/^foo'. The special sequence ':/!' is reserved for modifiers to what\n> is matched. ':/!-foo' performs a negative match, while ':/!!foo' matches a\n> diff --git a/sha1-name.c b/sha1-name.c\n> index 60d9ef3c7..641ca12f9 100644\n> --- a/sha1-name.c\n> +++ b/sha1-name.c\n> @@ -1650,6 +1650,7 @@ static int get_oid_with_context_1(const char *name,\n> \t\t\tstruct commit_list *list = NULL;\n> \n> \t\t\tfor_each_ref(handle_one_ref, &list);\n> +\t\t\thead_ref(handle_one_ref, &list);\n> \t\t\tcommit_list_sort_by_date(&list);\n> \t\t\treturn get_oid_oneline(name + 2, oid, list);\n> \t\t}\n> diff --git a/t/t4208-log-magic-pathspec.sh b/t/t4208-log-magic-pathspec.sh\n> index 62f335b2d..4c8f3b8e1 100755\n> --- a/t/t4208-log-magic-pathspec.sh\n> +++ b/t/t4208-log-magic-pathspec.sh\n> @@ -25,6 +25,32 @@ test_expect_success '\"git log :/a -- \" should not be ambiguous' '\n> \tgit log :/a --\n> '\n> \n> +test_expect_success '\"git log :/detached -- \" should find a commit only in HEAD' '\n> +\ttest_when_finished \"git checkout master\" &&\n> +\tgit checkout --detach &&\n> +\t# Must manually call `test_tick` instead of using `test_commit`,\n> +\t# because the latter additionally creates a tag, which would make\n> +\t# the commit reachable not only via HEAD.\n> +\ttest_tick &&\n> +\tgit commit --allow-empty -m detached &&\n> +\ttest_tick &&\n> +\tgit commit --allow-empty -m something-else &&\n> +\tgit log :/detached --\n> +'\n> +\n> +test_expect_success '\"git log :/detached -- \" should not find an orphaned commit' '\n> +\ttest_must_fail git log :/detached --\n> +'\n> +\n> +test_expect_success '\"git log :/detached -- \" should find HEAD only of own worktree' '\n> +\tgit worktree add other-tree HEAD &&\n> +\tgit -C other-tree checkout --detach &&\n> +\ttest_tick &&\n> +\tgit -C other-tree commit --allow-empty -m other-detached &&\n> +\tgit -C other-tree log :/other-detached -- &&\n> +\ttest_must_fail git log :/other-detached --\n> +'\n> +\n> test_expect_success '\"git log -- :/a\" should not be ambiguous' '\n> \tgit log -- :/a\n> '\n"},{"id":"352359","messageId":"CAFW+GMAp2bA2=_BZ2S0HLO2x2aLE01zXigHrY3QtCmWxRuyAtA@mail.gmail.com","threadId":"48852","inReplyTo":"xmqqva9kzbhi.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v2] sha1-name.c: for \":/\", find detached HEAD commits","fromName":"William Chargin","fromEmail":"wchargin@gmail.com","sentAt":"2018-07-12T16:14:10Z","receivedAt":"2018-07-12T16:14:33Z","isPatch":true,"sender":{"key":"wchargin@gmail.com","avatar":"https://avatars.githubusercontent.com/u/4317806?v=4"},"body":"> As we discussed during the review on v1, \":/\"\n> is *NOT* pathspec (that is why having these tests in t4208 is wrong\n> but we are following existing mistakes).\n\nAh, I understand the terminology better now. Thanks. I'll change the\ncommit message wording to use \"extended SHA-1s\" instead of \"pathspecs\".\n\n> Now you have Peff's sign-off for the one-liner code change, the last\n> one is redundant.\n\nOkay: I'll remove the \"Based-on-patch-by\" line.\n\n> Other than the above two nits, the patch looks good. I would have\n> broken the line after \"including HEAD.\", but the slightly-long line\n> is also OK.\n\nHappy to change this while making the above edits. :-)\n\nBest,\nWC\n"},{"id":"352388","messageId":"xmqqva9kw9ru.fsf@gitster-ct.c.googlers.com","threadId":"48852","inReplyTo":"CAFW+GMAp2bA2=_BZ2S0HLO2x2aLE01zXigHrY3QtCmWxRuyAtA@mail.gmail.com","subject":"Re: [PATCH v2] sha1-name.c: for \":/\", find detached HEAD commits","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-07-12T19:09:57Z","receivedAt":"2018-07-12T19:10:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"William Chargin writes:\n\n>> As we discussed during the review on v1, \":/\"\n>> is *NOT* pathspec (that is why having these tests in t4208 is wrong\n>> but we are following existing mistakes).\n>\n> Ah, I understand the terminology better now. Thanks. I'll change the\n> commit message wording to use \"extended SHA-1s\" instead of \"pathspecs\".\n>\n>> Now you have Peff's sign-off for the one-liner code change, the last\n>> one is redundant.\n>\n> Okay: I'll remove the \"Based-on-patch-by\" line.\n>\n>> Other than the above two nits, the patch looks good. I would have\n>> broken the line after \"including HEAD.\", but the slightly-long line\n>> is also OK.\n>\n> Happy to change this while making the above edits. :-)\n\nLet's save a round-trip. Here is what I'd queue for now, and you\ncan just say \"looks good\" if you agree with the result, or send an\nupdated version ;-).\n\nThanks for working on this.\n\n-- >8 --\nFrom: William Chargin \nDate: Wed, 11 Jul 2018 22:49:09 -0700\nSubject: [PATCH] sha1-name.c: for \":/\", find detached HEAD commits\n\nThis patch broadens the set of commits matched by \":/\" to\ninclude commits reachable from HEAD but not any named ref. This avoids\nsurprising behavior when working with a detached HEAD and trying to\nrefer to a commit that was recently created and only exists within the\ndetached state.\n\nIf multiple worktrees exist, only the current worktree's HEAD is\nconsidered reachable. This is consistent with the existing behavior for\nother per-worktree refs: e.g., bisect refs are considered reachable, but\nonly within the relevant worktree.\n\nSigned-off-by: Jeff King \nSigned-off-by: William Chargin \nSigned-off-by: Junio C Hamano \n---\n Documentation/revisions.txt | 3 ++-\n sha1_name.c | 1 +\n t/t4208-log-magic-pathspec.sh | 26 ++++++++++++++++++++++++++\n 3 files changed, 29 insertions(+), 1 deletion(-)\n\ndiff --git a/Documentation/revisions.txt b/Documentation/revisions.txt\nindex dfcc49c72c..0845c3f590 100644\n--- a/Documentation/revisions.txt\n+++ b/Documentation/revisions.txt\n@@ -180,7 +180,8 @@ existing tag object.\n A colon, followed by a slash, followed by a text, names\n a commit whose commit message matches the specified regular expression.\n This name returns the youngest matching commit which is\n- reachable from any ref. The regular expression can match any part of the\n+ reachable from any ref, including HEAD.\n+ The regular expression can match any part of the\n commit message. To match messages starting with a string, one can use\n e.g. ':/^foo'. The special sequence ':/!' is reserved for modifiers to what\n is matched. ':/!-foo' performs a negative match, while ':/!!foo' matches a\ndiff --git a/sha1_name.c b/sha1_name.c\nindex 611c7d24dd..19f66713e1 100644\n--- a/sha1_name.c\n+++ b/sha1_name.c\n@@ -1649,6 +1649,7 @@ static int get_oid_with_context_1(const char *name,\n \t\t\tstruct commit_list *list = NULL;\n \n \t\t\tfor_each_ref(handle_one_ref, &list);\n+\t\t\thead_ref(handle_one_ref, &list);\n \t\t\tcommit_list_sort_by_date(&list);\n \t\t\treturn get_oid_oneline(name + 2, oid, list);\n \t\t}\ndiff --git a/t/t4208-log-magic-pathspec.sh b/t/t4208-log-magic-pathspec.sh\nindex a1705f70cf..69643d101d 100755\n--- a/t/t4208-log-magic-pathspec.sh\n+++ b/t/t4208-log-magic-pathspec.sh\n@@ -25,6 +25,32 @@ test_expect_success '\"git log :/a -- \" should not be ambiguous' '\n \tgit log :/a --\n '\n \n+test_expect_success '\"git log :/detached -- \" should find a commit only in HEAD' '\n+\ttest_when_finished \"git checkout master\" &&\n+\tgit checkout --detach &&\n+\t# Must manually call `test_tick` instead of using `test_commit`,\n+\t# because the latter additionally creates a tag, which would make\n+\t# the commit reachable not only via HEAD.\n+\ttest_tick &&\n+\tgit commit --allow-empty -m detached &&\n+\ttest_tick &&\n+\tgit commit --allow-empty -m something-else &&\n+\tgit log :/detached --\n+'\n+\n+test_expect_success '\"git log :/detached -- \" should not find an orphaned commit' '\n+\ttest_must_fail git log :/detached --\n+'\n+\n+test_expect_success '\"git log :/detached -- \" should find HEAD only of own worktree' '\n+\tgit worktree add other-tree HEAD &&\n+\tgit -C other-tree checkout --detach &&\n+\ttest_tick &&\n+\tgit -C other-tree commit --allow-empty -m other-detached &&\n+\tgit -C other-tree log :/other-detached -- &&\n+\ttest_must_fail git log :/other-detached --\n+'\n+\n test_expect_success '\"git log -- :/a\" should not be ambiguous' '\n \tgit log -- :/a\n '\n-- \n2.18.0-129-ge3331758f1\n\n"},{"id":"352425","messageId":"CAFW+GMByLBbj6oDu-ERhN-bFO__Tj_M752-OYLYa7=z5DF0Ckg@mail.gmail.com","threadId":"48852","inReplyTo":"xmqqva9kw9ru.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v2] sha1-name.c: for \":/\", find detached HEAD commits","fromName":"William Chargin","fromEmail":"wchargin@gmail.com","sentAt":"2018-07-12T20:01:23Z","receivedAt":"2018-07-12T20:01:45Z","isPatch":true,"sender":{"key":"wchargin@gmail.com","avatar":"https://avatars.githubusercontent.com/u/4317806?v=4"},"body":"Contents look good to me. I don't understand why the file name in your\npatch is sha1_name.c as opposed to sha1-name.c (I see e5e5e0883 from\n2018-04-10, but that sounds pretty old), but I trust that whatever\nyou're doing there is correct.\n\n> Thanks for working on this.\n\nYou're quite welcome. Thanks to you, Peff, and Duy for your help. This\nwas a pleasant experience for me as a new contributor. :-)\n"},{"id":"352521","messageId":"20180713192441.GA10354@sigill.intra.peff.net","threadId":"48852","inReplyTo":"CAFW+GMByLBbj6oDu-ERhN-bFO__Tj_M752-OYLYa7=z5DF0Ckg@mail.gmail.com","subject":"Re: [PATCH v2] sha1-name.c: for \":/\", find detached HEAD commits","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-07-13T19:24:42Z","receivedAt":"2018-07-13T19:24:45Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jul 12, 2018 at 01:01:23PM -0700, William Chargin wrote:\n\n> Contents look good to me. I don't understand why the file name in your\n> patch is sha1_name.c as opposed to sha1-name.c (I see e5e5e0883 from\n> 2018-04-10, but that sounds pretty old), but I trust that whatever\n> you're doing there is correct.\n\nJunio typically applies bugfixes as close to the bug-source as possible,\nwhich allows them to be merged-up into various releases (rather than\ncherry-picked, which would be required if built on top of 'master').\n\nIdeally this is directly on top of the commit that introduced the bug,\nthough for an ancient bug like this, it's not worth the effort. It looks\nlike he applied it on the 2.16 maint branch, which predates e5e5e0883.\nWhen it's merged up, the resolution will handle the rename (probably\neven automatically due to Git's rename detection).\n\n> > Thanks for working on this.\n> \n> You're quite welcome. Thanks to you, Peff, and Duy for your help. This\n> was a pleasant experience for me as a new contributor. :-)\n\nGreat. Please come back anytime. :)\n\n-Peff\n"},{"id":"352526","messageId":"CAFW+GMDtA=u1y4ncLUf25CSTymv-DAM3n5a+JSkT-XxNeH9PBA@mail.gmail.com","threadId":"48852","inReplyTo":"20180713192441.GA10354@sigill.intra.peff.net","subject":"Re: [PATCH v2] sha1-name.c: for \":/\", find detached HEAD commits","fromName":"William Chargin","fromEmail":"wchargin@gmail.com","sentAt":"2018-07-13T19:40:23Z","receivedAt":"2018-07-13T19:40:47Z","isPatch":true,"sender":{"key":"wchargin@gmail.com","avatar":"https://avatars.githubusercontent.com/u/4317806?v=4"},"body":"> Junio typically applies bugfixes as close to the bug-source as possible,\n> which allows them to be merged-up into various releases (rather than\n> cherry-picked, which would be required if built on top of 'master').\n>\n> Ideally this is directly on top of the commit that introduced the bug,\n> though for an ancient bug like this, it's not worth the effort. It looks\n> like he applied it on the 2.16 maint branch, which predates e5e5e0883.\n> When it's merged up, the resolution will handle the rename (probably\n> even automatically due to Git's rename detection).\n\nThat makes sense. Thanks for the explanation.\n\n> Great. Please come back anytime. :)\n\nWill do!\n\nBest,\nWC\n"}]}