:/regex syntax picks stash entries over regular commits

:/regex syntax picks stash entries over regular commits

[Nude F. Ninja]

What did you do before the bug happened? (Steps to reproduce your issue)
I ran git stash push, which created the stash entry "On main: dark
mode". Then I committed changes before noticing an oversight with the
previous commit. I wrote the fix and ran git commit --fixup :/dark

What did you expect to happen? (Expected behavior)
The fixup commit should have referenced the previous commit.

What happened instead? (Actual behavior)
It referenced the stash entry.

What's different between what you expected and what actually happened?
I cannot autosquash the commit that refers to the stash entry. I
suggest adding an option to prefer (older) regular commits over
(newer) stash entries when specifying a commit with the :/regex
syntax. In any case I think it's a bug for the --fixup option to
readily produce a commit that does not work with the --autosquash
option which it seems it was designed for.


[System Info]
git version:
git version 2.39.3 (Apple Git-146)

Re: :/regex syntax picks stash entries over regular commits

[Junio C Hamano]

"Nude F. Ninja" <nudefninja@gmail.com> writes:

> What did you do before the bug happened? (Steps to reproduce your issue)
> I ran git stash push, which created the stash entry "On main: dark
> mode". Then I committed changes before noticing an oversight with the
> previous commit. I wrote the fix and ran git commit --fixup :/dark

It is natural that there are multiple commits that match the pattern
you give in your repository.

One trick I learned that is effective is to explicitly state where
to start searches, e.g. "--fixup 'HEAD^{/dark mode}'", which would
be very much in line to what --fixup wants to do.  The commit to be
fixed up in a later rebase session by definition must be an ancestor
of the current HEAD.

Re: :/regex syntax picks stash entries over regular commits

[Jeff King]

On Sun, Mar 31, 2024 at 04:06:35PM -0700, Junio C Hamano wrote:

> "Nude F. Ninja" <nudefninja@gmail.com> writes:
> 
> > What did you do before the bug happened? (Steps to reproduce your issue)
> > I ran git stash push, which created the stash entry "On main: dark
> > mode". Then I committed changes before noticing an oversight with the
> > previous commit. I wrote the fix and ran git commit --fixup :/dark
> 
> It is natural that there are multiple commits that match the pattern
> you give in your repository.
> 
> One trick I learned that is effective is to explicitly state where
> to start searches, e.g. "--fixup 'HEAD^{/dark mode}'", which would
> be very much in line to what --fixup wants to do.  The commit to be
> fixed up in a later rebase session by definition must be an ancestor
> of the current HEAD.

Yeah, the "traverse all commits" aspect of ":/" is well known and
confusing, and why we introduced the rev ^{/} syntax. But I still wonder
if it would be better to limit ":/" to something more sensible. Finding
"refs/stash" or "refs/notes/*" is downright confusing (stash especially
because we don't walk the reflog, so it sees only stash@{0}, and not the
others!).

It would be pretty easy to do the equivalent of "--branches --tags
--remotes":

diff --git a/object-name.c b/object-name.c
index 523af6f64f..5285903f78 100644
--- a/object-name.c
+++ b/object-name.c
@@ -2002,7 +2002,9 @@ static enum get_oid_result get_oid_with_context_1(struct repository *repo,
 
 			cb.repo = repo;
 			cb.list = &list;
-			refs_for_each_ref(get_main_ref_store(repo), handle_one_ref, &cb);
+			refs_for_each_ref_in(get_main_ref_store(repo), "refs/heads/", handle_one_ref, &cb);
+			refs_for_each_ref_in(get_main_ref_store(repo), "refs/tags/", handle_one_ref, &cb);
+			refs_for_each_ref_in(get_main_ref_store(repo), "refs/remotes/", handle_one_ref, &cb);
 			refs_head_ref(get_main_ref_store(repo), handle_one_ref, &cb);
 			commit_list_sort_by_date(&list);
 			return get_oid_oneline(repo, name + 2, oid, list);

Or alternatively to skip known-confusing parts of the namespace like
refs/stash.

I dunno. I have long ago written off :/ as useless, so maybe trying to
make it slightly less confusing is a fool's errand. Maybe we'd be better
off putting a note in its documentation that rev^{/} is more likely to
do what you want.

-Peff