Re: [PATCH v2] sha1-name.c: for ":/", find detached HEAD commits
> Junio typically applies bugfixes as close to the bug-source as possible, > which allows them to be merged-up into various releases (rather than > cherry-picked, which would be required if built on top of 'master'). > > Ideally this is directly on top of the commit that introduced the bug, > though for an ancient bug like this, it's not worth the effort. It looks > like he applied it on the 2.16 maint branch, which predates e5e5e0883. > When it's merged up, the resolution will handle the rename (probably > even automatically due to Git's rename detection). That makes sense. Thanks for the explanation. > Great. Please come back anytime. :) Will do! Best, WC
Re: [PATCH v2] sha1-name.c: for ":/", find detached HEAD commits
On Thu, Jul 12, 2018 at 01:01:23PM -0700, William Chargin wrote: > Contents look good to me. I don't understand why the file name in your > patch is sha1_name.c as opposed to sha1-name.c (I see e5e5e0883 from > 2018-04-10, but that sounds pretty old), but I trust that whatever > you're doing there is correct. Junio typically applies bugfixes as close to the bug-source as possible, which allows them to be merged-up into various releases (rather than cherry-picked, which would be required if built on top of 'master'). Ideally this is directly on top of the commit that introduced the bug, though for an ancient bug like this, it's not worth the effort. It looks like he applied it on the 2.16 maint branch, which predates e5e5e0883. When it's merged up, the resolution will handle the rename (probably even automatically due to Git's rename detection). > > Thanks for working on this. > > You're quite welcome. Thanks to you, Peff, and Duy for your help. This > was a pleasant experience for me as a new contributor. :-) Great. Please come back anytime. :) -Peff
Re: [PATCH v2] sha1-name.c: for ":/", find detached HEAD commits
Contents look good to me. I don't understand why the file name in your patch is sha1_name.c as opposed to sha1-name.c (I see e5e5e0883 from 2018-04-10, but that sounds pretty old), but I trust that whatever you're doing there is correct. > Thanks for working on this. You're quite welcome. Thanks to you, Peff, and Duy for your help. This was a pleasant experience for me as a new contributor. :-)
Re: [PATCH v2] sha1-name.c: for ":/", find detached HEAD commits
William Chargin writes: >> As we discussed during the review on v1, ":/" >> is *NOT* pathspec (that is why having these tests in t4208 is wrong >> but we are following existing mistakes). > > Ah, I understand the terminology better now. Thanks. I'll change the > commit message wording to use "extended SHA-1s" instead of "pathspecs". > >> Now you have Peff's sign-off for the one-liner code change, the last >> one is redundant. > > Okay: I'll remove the "Based-on-patch-by" line. > >> Other than the above two nits, the patch looks good. I would have >> broken the line after "including HEAD.", but the slightly-long line >> is also OK. > > Happy to change this while making the above edits. :-) Let's save a round-trip. Here is what I'd queue for now, and you can just say "looks good" if you agree with the result, or send an updated version ;-). Thanks for working on this. -- >8 -- From: William Chargin Date: Wed, 11 Jul 2018 22:49:09 -0700 Subject: [PATCH] sha1-name.c: for ":/", find detached HEAD commits This patch broadens the set of commits matched by ":/" to include commits reachable from HEAD but not any named ref. This avoids surprising behavior when working with a detached HEAD and trying to refer to a commit that was recently created and only exists within the detached state. If multiple worktrees exist, only the current worktree's HEAD is considered reachable. This is consistent with the existing behavior for other per-worktree refs: e.g., bisect refs are considered reachable, but only within the relevant worktree. Signed-off-by: Jeff King Signed-off-by: William Chargin Signed-off-by: Junio C Hamano --- Documentation/revisions.txt | 3 ++- sha1_name.c | 1 + t/t4208-log-magic-pathspec.sh | 26 ++ 3 files changed, 29 insertions(+), 1 deletion(-) diff --git a/Documentation/revisions.txt b/Documentation/revisions.txt index dfcc49c72c..0845c3f590 100644 --- a/Documentation/revisions.txt +++ b/Documentation/revisions.txt @@ -180,7 +180,8 @@ existing tag object. A colon, followed by a slash, followed by a text, names a commit whose commit message matches the specified regular expression. This name returns the youngest matching commit which is - reachable from any ref. The regular expression can match any part of the + reachable from any ref, including HEAD. + The regular expression can match any part of the commit message. To match messages starting with a string, one can use e.g. ':/^foo'. The special sequence ':/!' is reserved for modifiers to what is matched. ':/!-foo' performs a negative match, while ':/!!foo' matches a diff --git a/sha1_name.c b/sha1_name.c index 611c7d24dd..19f66713e1 100644 --- a/sha1_name.c +++ b/sha1_name.c @@ -1649,6 +1649,7 @@ static int get_oid_with_context_1(const char *name, struct commit_list *list = NULL; for_each_ref(handle_one_ref, &list); + head_ref(handle_one_ref, &list); commit_list_sort_by_date(&list); return get_oid_oneline(name + 2, oid, list); } diff --git a/t/t4208-log-magic-pathspec.sh b/t/t4208-log-magic-pathspec.sh index a1705f70cf..69643d101d 100755 --- a/t/t4208-log-magic-pathspec.sh +++ b/t/t4208-log-magic-pathspec.sh @@ -25,6 +25,32 @@ test_expect_success '"git log :/a -- " should not be ambiguous' ' git log :/a -- ' +test_expect_success '"git log :/detached -- " should find a commit only in HEAD' ' + test_when_finished "git checkout master" && + git checkout --detach && + # Must manually call `test_tick` instead of using `test_commit`, + # because the latter additionally creates a tag, which would make + # the commit reachable not only via HEAD. + test_tick && + git commit --allow-empty -m detached && + test_tick && + git commit --allow-empty -m something-else && + git log :/detached -- +' + +test_expect_success '"git log :/detached -- " should not find an orphaned commit' ' + test_must_fail git log :/detached -- +' + +test_expect_success '"git log :/detached -- " should find HEAD only of own worktree' ' + git worktree add other-tree HEAD && + git -C other-tree checkout --detach && + test_tick && + git -C other-tree commit --allow-empty -m other-detached && + git -C other-tree log :/other-detached -- && + test_must_fail git log :/other-detached -- +' + test_expect_success '"git log -- :/a" should not be ambiguous' ' git log -- :/a ' -- 2.18.0-129-ge3331758f1
Re: [PATCH v2] sha1-name.c: for ":/", find detached HEAD commits
> As we discussed during the review on v1, ":/" > is *NOT* pathspec (that is why having these tests in t4208 is wrong > but we are following existing mistakes). Ah, I understand the terminology better now. Thanks. I'll change the commit message wording to use "extended SHA-1s" instead of "pathspecs". > Now you have Peff's sign-off for the one-liner code change, the last > one is redundant. Okay: I'll remove the "Based-on-patch-by" line. > Other than the above two nits, the patch looks good. I would have > broken the line after "including HEAD.", but the slightly-long line > is also OK. Happy to change this while making the above edits. :-) Best, WC
Re: [PATCH v2] sha1-name.c: for ":/", find detached HEAD commits
William Chargin writes: > This patch broadens the set of commits matched by ":/" pathspecs to As we discussed during the review on v1, ":/" is *NOT* pathspec (that is why having these tests in t4208 is wrong but we are following existing mistakes). It is a way to specify a commit object name (unfortunately "extended SHA-1" is the phrase we often call the various ways to name an object that are implemented in sha1-name.c). > include commits reachable from HEAD but not any named ref. This avoids > surprising behavior when working with a detached HEAD and trying to > refer to a commit that was recently created and only exists within the > detached state. > > If multiple worktrees exist, only the current worktree's HEAD is > considered reachable. This is consistent with the existing behavior for > other per-worktree refs: e.g., bisect refs are considered reachable, but > only within the relevant worktree. > > Signed-off-by: Jeff King > Signed-off-by: William Chargin > Based-on-patch-by: Jeff King Now you have Peff's sign-off for the one-liner code change, the last one is redundant. > --- Other than the above two nits, the patch looks good. I would have broken the line after "including HEAD.", but the slightly-long line is also OK. > Documentation/revisions.txt | 2 +- > sha1-name.c | 1 + > t/t4208-log-magic-pathspec.sh | 26 ++ > 3 files changed, 28 insertions(+), 1 deletion(-) > > diff --git a/Documentation/revisions.txt b/Documentation/revisions.txt > index 7d1bd4409..aa9579eba 100644 > --- a/Documentation/revisions.txt > +++ b/Documentation/revisions.txt > @@ -184,7 +184,7 @@ existing tag object. >A colon, followed by a slash, followed by a text, names >a commit whose commit message matches the specified regular expression. >This name returns the youngest matching commit which is > - reachable from any ref. The regular expression can match any part of the > + reachable from any ref, including HEAD. The regular expression can match > any part of the >commit message. To match messages starting with a string, one can use >e.g. ':/^foo'. The special sequence ':/!' is reserved for modifiers to what >is matched. ':/!-foo' performs a negative match, while ':/!!foo' matches a > diff --git a/sha1-name.c b/sha1-name.c > index 60d9ef3c7..641ca12f9 100644 > --- a/sha1-name.c > +++ b/sha1-name.c > @@ -1650,6 +1650,7 @@ static int get_oid_with_context_1(const char *name, > struct commit_list *list = NULL; > > for_each_ref(handle_one_ref, &list); > + head_ref(handle_one_ref, &list); > commit_list_sort_by_date(&list); > return get_oid_oneline(name + 2, oid, list); > } > diff --git a/t/t4208-log-magic-pathspec.sh b/t/t4208-log-magic-pathspec.sh > index 62f335b2d..4c8f3b8e1 100755 > --- a/t/t4208-log-magic-pathspec.sh > +++ b/t/t4208-log-magic-pathspec.sh > @@ -25,6 +25,32 @@ test_expect_success '"git log :/a -- " should not be > ambiguous' ' > git log :/a -- > ' > > +test_expect_success '"git log :/detached -- " should find a commit only in > HEAD' ' > + test_when_finished "git checkout master" && > + git checkout --detach && > + # Must manually call `test_tick` instead of using `test_commit`, > + # because the latter additionally creates a tag, which would make > + # the commit reachable not only via HEAD. > + test_tick && > + git commit --allow-empty -m detached && > + test_tick && > + git commit --allow-empty -m something-else && > + git log :/detached -- > +' > + > +test_expect_success '"git log :/detached -- " should not find an orphaned > commit' ' > + test_must_fail git log :/detached -- > +' > + > +test_expect_success '"git log :/detached -- " should find HEAD only of own > worktree' ' > + git worktree add other-tree HEAD && > + git -C other-tree checkout --detach && > + test_tick && > + git -C other-tree commit --allow-empty -m other-detached && > + git -C other-tree log :/other-detached -- && > + test_must_fail git log :/other-detached -- > +' > + > test_expect_success '"git log -- :/a" should not be ambiguous' ' > git log -- :/a > '
Re: [PATCH v2] sha1-name.c: for ":/", find detached HEAD commits
On Wed, Jul 11, 2018 at 10:49:09PM -0700, William Chargin wrote: > This patch broadens the set of commits matched by ":/" pathspecs to > include commits reachable from HEAD but not any named ref. This avoids > surprising behavior when working with a detached HEAD and trying to > refer to a commit that was recently created and only exists within the > detached state. > > If multiple worktrees exist, only the current worktree's HEAD is > considered reachable. This is consistent with the existing behavior for > other per-worktree refs: e.g., bisect refs are considered reachable, but > only within the relevant worktree. > > Signed-off-by: Jeff King > Signed-off-by: William Chargin > Based-on-patch-by: Jeff King > --- > Documentation/revisions.txt | 2 +- > sha1-name.c | 1 + > t/t4208-log-magic-pathspec.sh | 26 ++ > 3 files changed, 28 insertions(+), 1 deletion(-) This one looks good to me. Thanks. -Peff
[PATCH v2] sha1-name.c: for ":/", find detached HEAD commits
This patch broadens the set of commits matched by ":/" pathspecs to include commits reachable from HEAD but not any named ref. This avoids surprising behavior when working with a detached HEAD and trying to refer to a commit that was recently created and only exists within the detached state. If multiple worktrees exist, only the current worktree's HEAD is considered reachable. This is consistent with the existing behavior for other per-worktree refs: e.g., bisect refs are considered reachable, but only within the relevant worktree. Signed-off-by: Jeff King Signed-off-by: William Chargin Based-on-patch-by: Jeff King --- Documentation/revisions.txt | 2 +- sha1-name.c | 1 + t/t4208-log-magic-pathspec.sh | 26 ++ 3 files changed, 28 insertions(+), 1 deletion(-) diff --git a/Documentation/revisions.txt b/Documentation/revisions.txt index 7d1bd4409..aa9579eba 100644 --- a/Documentation/revisions.txt +++ b/Documentation/revisions.txt @@ -184,7 +184,7 @@ existing tag object. A colon, followed by a slash, followed by a text, names a commit whose commit message matches the specified regular expression. This name returns the youngest matching commit which is - reachable from any ref. The regular expression can match any part of the + reachable from any ref, including HEAD. The regular expression can match any part of the commit message. To match messages starting with a string, one can use e.g. ':/^foo'. The special sequence ':/!' is reserved for modifiers to what is matched. ':/!-foo' performs a negative match, while ':/!!foo' matches a diff --git a/sha1-name.c b/sha1-name.c index 60d9ef3c7..641ca12f9 100644 --- a/sha1-name.c +++ b/sha1-name.c @@ -1650,6 +1650,7 @@ static int get_oid_with_context_1(const char *name, struct commit_list *list = NULL; for_each_ref(handle_one_ref, &list); + head_ref(handle_one_ref, &list); commit_list_sort_by_date(&list); return get_oid_oneline(name + 2, oid, list); } diff --git a/t/t4208-log-magic-pathspec.sh b/t/t4208-log-magic-pathspec.sh index 62f335b2d..4c8f3b8e1 100755 --- a/t/t4208-log-magic-pathspec.sh +++ b/t/t4208-log-magic-pathspec.sh @@ -25,6 +25,32 @@ test_expect_success '"git log :/a -- " should not be ambiguous' ' git log :/a -- ' +test_expect_success '"git log :/detached -- " should find a commit only in HEAD' ' + test_when_finished "git checkout master" && + git checkout --detach && + # Must manually call `test_tick` instead of using `test_commit`, + # because the latter additionally creates a tag, which would make + # the commit reachable not only via HEAD. + test_tick && + git commit --allow-empty -m detached && + test_tick && + git commit --allow-empty -m something-else && + git log :/detached -- +' + +test_expect_success '"git log :/detached -- " should not find an orphaned commit' ' + test_must_fail git log :/detached -- +' + +test_expect_success '"git log :/detached -- " should find HEAD only of own worktree' ' + git worktree add other-tree HEAD && + git -C other-tree checkout --detach && + test_tick && + git -C other-tree commit --allow-empty -m other-detached && + git -C other-tree log :/other-detached -- && + test_must_fail git log :/other-detached -- +' + test_expect_success '"git log -- :/a" should not be ambiguous' ' git log -- :/a ' -- 2.18.0.130.g84cda7581
