threads / discuss / 61240

:/regex syntax picks stash entries over regular commits

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

## tl;dr

3 messages between Mar 31, 2024 and Apr 1, 2024.

replies: 2people: 3as markdown or json

Nude F. Ninja· Mar 31, 2024, 22:13 UTC · lore

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)

Junio C Hamano· Mar 31, 2024, 23:06 UTC · re: Nude F. Ninja · lore

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

"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.

Jeff King· Apr 1, 2024, 02:44 UTC · re: Junio C Hamano · lore

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

On Sun, Mar 31, 2024 at 04:06:35PM -0700, Junio C Hamano wrote:
Show 15 quoted lines
> "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

← back to recent threads