Re: [PATCH] sha1-name.c: for ":/", find detached HEAD commits
> > Does for_each_ref() iterate over per-worktree refs (like "bisect", > > perhaps)? > > No. To be clear: it iterates only over the per-worktree refs for the current worktree. So, if you mark an unreachable commit as "bad" with bisect, then that commit is reachable (and found by ":/") in the enclosing worktree only. With this patch, ":/" now behaves the same way with HEADs, which makes sense to me. Agreed with the above discussion. I can add a test for this.
Re: [PATCH] sha1-name.c: for ":/", find detached HEAD commits
On Wed, Jul 11, 2018 at 7:20 PM Junio C Hamano wrote: > > Duy Nguyen writes: > > >> 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, ); > >> + head_ref(handle_one_ref, ); > > > > When multiple worktrees are used, should we consider all HEADs or just > > current worktree's HEAD? > > Does for_each_ref() iterate over per-worktree refs (like "bisect", > perhaps)? No. > If so, then looking in different worktree's HEADs would > make sense, and otherwise not. > > I would think that the whole point of detaching HEAD in a separate > worktree is that you can avoid exposing the work you do while > detached to other worktrees by doing so, so from that point of view, > I would probably prefer :/ not to look into other worktrees, but > that is not a very strong preference. Yeah at least for me worktrees are still quite isolated. Occasionally I need to peek in a worktree from another one (e.g. if I want to run tests on current HEAD but do not want to wait for it to finish, I'll make a new worktree, checking out the same head and run tests there) but it's really rare. So yeah maybe it's best to stick to current worktree's HEAD only. At least until someone finds a good case for it. > If peeking all over the place > is easier to implement, then a minor information leaking is not > something I'd lose sleep over. > > Thanks for bringing up an interesting issue. -- Duy
Re: [PATCH] sha1-name.c: for ":/", find detached HEAD commits
Duy Nguyen writes: >> 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, ); >> + head_ref(handle_one_ref, ); > > When multiple worktrees are used, should we consider all HEADs or just > current worktree's HEAD? Does for_each_ref() iterate over per-worktree refs (like "bisect", perhaps)? If so, then looking in different worktree's HEADs would make sense, and otherwise not. I would think that the whole point of detaching HEAD in a separate worktree is that you can avoid exposing the work you do while detached to other worktrees by doing so, so from that point of view, I would probably prefer :/ not to look into other worktrees, but that is not a very strong preference. If peeking all over the place is easier to implement, then a minor information leaking is not something I'd lose sleep over. Thanks for bringing up an interesting issue.
Re: [PATCH] sha1-name.c: for ":/", find detached HEAD commits
On Wed, Jul 11, 2018 at 05:45:09PM +0200, Duy Nguyen wrote: > > 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, ); > > + head_ref(handle_one_ref, ); > > When multiple worktrees are used, should we consider all HEADs or just > current worktree's HEAD? > > This is the latter case, if we should go with the former (I don't > know, it's a genuine question) then you need one more call: > other_head_refs(). Oof, I didn't even think about other worktrees. I'd probably say "yes", as we consider them reachable (and really the intent of this feature seems to be "find it in any reachable commit"). -Peff
Re: [PATCH] sha1-name.c: for ":/", find detached HEAD commits
Jeff King writes: > On Tue, Jul 10, 2018 at 11:18:22PM -0700, William Chargin wrote: > >> > Also, I am not sure if "or from HEAD" is even needed when we say >> > "from ANY ref" already, as we count things like HEAD as part of the >> > ref namespace. >> >> My two cents: with the docs as is, I wasn't sure whether HEAD was >> intended to count as a ref for this purpose. The gitglossary man page >> defines a ref as a "name that begins with refs/" (seemingly excluding >> HEAD), though it later says that HEAD is a "special-purpose ref". In my >> opinion, the change adds clarity without any particular downside---but >> I'm happy to revert it if you'd prefer. I'd also be happy to change the >> wording to something like "any ref, including HEAD" if we want to >> emphasize that HEAD really is a ref. > > FWIW, I think the clarification to include HEAD is helpful here, since > it took me a few minutes of thinking to decide whether the current > behavior was a bug or just a subtlety. Your "including HEAD" suggestion > seems like the best route to me. But I can live with it either way. > >> After reaching consensus on the change to the docs, should I send in a >> [PATCH v2] In-Reply-To this thread? > > Yes. > >> Peff, should I add your >> Signed-off-by to the commit message, or is that not how things are done? > > Yes, you can add in any sign-offs that have been explicitly given. It's > normal to order them chronologically, too (so mine would come first, > then yours, showing that the patch flowed through me to you; Junio will > add his at the end). Thanks, agreed 100% and I have nothing more to add.
Re: [PATCH] sha1-name.c: for ":/", find detached HEAD commits
On Tue, Jul 10, 2018 at 5:44 PM 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.
Cool!
>
> Signed-off-by: William Chargin
> Based-on-patch-by: Jeff King
> ---
> Documentation/revisions.txt | 10 +-
> sha1-name.c | 1 +
> t/t4208-log-magic-pathspec.sh | 14 ++
> 3 files changed, 20 insertions(+), 5 deletions(-)
>
> diff --git a/Documentation/revisions.txt b/Documentation/revisions.txt
> index 7d1bd4409..aa56907fd 100644
> --- a/Documentation/revisions.txt
> +++ b/Documentation/revisions.txt
> @@ -181,11 +181,11 @@ existing tag object.
>the '' before '{caret}'.
>
> ':/', e.g. ':/fix nasty bug'::
> - 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
> - commit message. To match messages starting with a string, one can use
> + 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
> + or from 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
>literal '!' character, followed by 'foo'. Any other sequence beginning with
> 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, );
> + head_ref(handle_one_ref, );
When multiple worktrees are used, should we consider all HEADs or just
current worktree's HEAD?
This is the latter case, if we should go with the former (I don't
know, it's a genuine question) then you need one more call:
other_head_refs().
> commit_list_sort_by_date();
> 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..41b9f3eba 100755
> --- a/t/t4208-log-magic-pathspec.sh
> +++ b/t/t4208-log-magic-pathspec.sh
> @@ -25,6 +25,20 @@ 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 &&
> + 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 -- :/a" should not be ambiguous' '
> git log -- :/a
> '
> --
> 2.18.0.130.g61d0c9dcf
>
--
Duy
Re: [PATCH] sha1-name.c: for ":/", find detached HEAD commits
On Tue, Jul 10, 2018 at 11:18:22PM -0700, William Chargin wrote: > > Also, I am not sure if "or from HEAD" is even needed when we say > > "from ANY ref" already, as we count things like HEAD as part of the > > ref namespace. > > My two cents: with the docs as is, I wasn't sure whether HEAD was > intended to count as a ref for this purpose. The gitglossary man page > defines a ref as a "name that begins with refs/" (seemingly excluding > HEAD), though it later says that HEAD is a "special-purpose ref". In my > opinion, the change adds clarity without any particular downside---but > I'm happy to revert it if you'd prefer. I'd also be happy to change the > wording to something like "any ref, including HEAD" if we want to > emphasize that HEAD really is a ref. FWIW, I think the clarification to include HEAD is helpful here, since it took me a few minutes of thinking to decide whether the current behavior was a bug or just a subtlety. Your "including HEAD" suggestion seems like the best route to me. But I can live with it either way. > After reaching consensus on the change to the docs, should I send in a > [PATCH v2] In-Reply-To this thread? Yes. > Peff, should I add your > Signed-off-by to the commit message, or is that not how things are done? Yes, you can add in any sign-offs that have been explicitly given. It's normal to order them chronologically, too (so mine would come first, then yours, showing that the patch flowed through me to you; Junio will add his at the end). -Peff
Re: [PATCH] sha1-name.c: for ":/", find detached HEAD commits
> we care about reachability only from the detached HEAD here, so this > must _not_ use test_commit, which creates an extra tag. Right. I can add a comment to that effect. > Please avoid unnecessary reflowing of earlier lines of the paragrpah > when the only change is to insert "or from HEAD" in the middle of its > fourth line. Sure: I'll make that change. (My intent was to incrementally clean up an area already under change, but I'm happy to instead make only the minimal change.) > Also, I am not sure if "or from HEAD" is even needed when we say > "from ANY ref" already, as we count things like HEAD as part of the > ref namespace. My two cents: with the docs as is, I wasn't sure whether HEAD was intended to count as a ref for this purpose. The gitglossary man page defines a ref as a "name that begins with refs/" (seemingly excluding HEAD), though it later says that HEAD is a "special-purpose ref". In my opinion, the change adds clarity without any particular downside---but I'm happy to revert it if you'd prefer. I'd also be happy to change the wording to something like "any ref, including HEAD" if we want to emphasize that HEAD really is a ref. > We are interested in seeing that ":/string" is understood as a valid > object name, and this is not limited to "log" at all. Indeed. I was a bit surprised for the same reason (I expected the tests to be using `rev-parse`), but I agree that it's probably best to keep the existing structure to minimize churn. --- After reaching consensus on the change to the docs, should I send in a [PATCH v2] In-Reply-To this thread? Peff, should I add your Signed-off-by to the commit message, or is that not how things are done? Best, WC
Re: [PATCH] sha1-name.c: for ":/", find detached HEAD commits
William Chargin writes:
> 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.
>
> Signed-off-by: William Chargin
> Based-on-patch-by: Jeff King
> ---
> Documentation/revisions.txt | 10 +-
> sha1-name.c | 1 +
> t/t4208-log-magic-pathspec.sh | 14 ++
> 3 files changed, 20 insertions(+), 5 deletions(-)
>
> diff --git a/Documentation/revisions.txt b/Documentation/revisions.txt
> index 7d1bd4409..aa56907fd 100644
> --- a/Documentation/revisions.txt
> +++ b/Documentation/revisions.txt
> @@ -181,11 +181,11 @@ existing tag object.
>the '' before '{caret}'.
>
> ':/', e.g. ':/fix nasty bug'::
> - 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
> - commit message. To match messages starting with a string, one can use
> + 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
> + or from HEAD. The regular expression can match any part of the commit
> + message. To match messages starting with a string, one can use
Please avoid unnecessary reflowing of earlier lines of the paragrpah
when the only change is to insert "or from HEAD" in the middle of its
fourth line. That wastes reviewers time (and typically there are
more readers than there are writers). If you did this instead:
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 or from HEAD.
The regular expression can match any part of the
commit message. To match messages starting with a string, one can use
the resulting patch would have been much easier to follow.
Also, I am not sure if "or from HEAD" is even needed when we say
"from ANY ref" already, as we count things like HEAD as part of the
ref namespace. It may only feel necessary for the person who made
the change to call handle_one_ref() not only with for_each_ref() but
also with head_ref()---in other words, I am wondering if it was
merely a bug to ignore HEAD in the code when the documentation said
"from any ref" already. If that is the case, then the above change
is not necessary.
> 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, );
> + head_ref(handle_one_ref, );
Makes sense.
> commit_list_sort_by_date();
> 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..41b9f3eba 100755
> --- a/t/t4208-log-magic-pathspec.sh
> +++ b/t/t4208-log-magic-pathspec.sh
> @@ -25,6 +25,20 @@ 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 &&
> + 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 --
> +'
> +
OK.
I found these tests (including the existing ones) misleading, by the
way. We are interested in seeing that ":/string" is understood as a
valid object name, and this is not limited to "log" at all. Instead
of "git log", perhaps "git rev-parse --verify :/detached" may have
been more to the point of what we are actually trying to see.
But that is a fault of having test for ":/" in "magic pathspec"
test script; correcting that mistake is totally outside of the scope
of this fix.
Thanks.
Re: [PATCH] sha1-name.c: for ":/", find detached HEAD commits
On Tue, Jul 10, 2018 at 08:41:06AM -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. > > Signed-off-by: William Chargin > Based-on-patch-by: Jeff King Thanks, this looks perfect! Just to be clear on licensing, I'll add my: Signed-off-by: Jeff King > diff --git a/t/t4208-log-magic-pathspec.sh b/t/t4208-log-magic-pathspec.sh > index 62f335b2d..41b9f3eba 100755 > --- a/t/t4208-log-magic-pathspec.sh > +++ b/t/t4208-log-magic-pathspec.sh > @@ -25,6 +25,20 @@ 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 && > + test_tick && > + git commit --allow-empty -m detached && > + test_tick && > + git commit --allow-empty -m something-else && > + git log :/detached -- > +' At first I wondered if you could just use test_commit here (instead of a manual test_tick and commit). But we care about reachability only from the detached HEAD here, so this must _not_ use test_commit, which creates an extra tag. It could be worth mentioning that in a comment (since somebody who later refactors to make that change would not realize the problem, as it would simply cause a broken test to return a false success). But... > +test_expect_success '"git log :/detached -- " should not find an orphaned > commit' ' > + test_must_fail git log :/detached -- > +' This follow-up test _would_ notice such a breakage. Very nicely done. -Peff
