Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Characterize base search behaviour #154

Closed

Conversation

blairconrad
Copy link
Contributor

While working on #145 I had trouble writing a test and ended up discovering what looked like undocumented behaviour: branches interrupt the search for a base commit (if not explicitly specified).
So I added some tests and updated the documentation.

Builds on my proposed refactoring around the configuration and logger objects, because it was convenient for me, but can be reworked.

@blairconrad
Copy link
Contributor Author

I think this has hit me "in real life" and I didn't realize it at the time! I often create alternative branches for experimentation. Recently I found that git-absorb didn't find a commit to fixup. I added a --base and moved on, as I was just trying to get work done.

Out of curiosity, why stop searching when you hit a branch? People's workflows differ, but I'd be content to fixup past branches in many (all?) cases.

Assuming the behaviour stays (and I'm not suggesting you remove or change it), might it be useful to emit a warning (similar to the "end of stack reached" one) if a branch halts the search?
Of course, this would probably require rework. I'd imagine walking the parents from HEAD and checking individually if they are also a branch, rather than hiding all branch heads, but I've not explored to see how feasible this is, even if it's desirable.

Thanks for listening.

@tummychow
Copy link
Owner

could you rebase the pr over master to eliminate the other bits? i'll review after that

as for why it works this way: git-absorb has to guess what commit you would counterfactually rebase over when choosing the base. git is not opinionated about how you specify this, and different workflows can differ drastically.

  • some people will set their push and pull remote to a unique branch, and they just manually git rebase master whenever they want to go over it
  • some people will set their push remote to be specific to their branch but set the pull remote to be the branch's final destination, so then they can git pull with no arguments to update their branch
  • some people will set the push and pull remote to both be the destination branch, and just git push origin HEAD whenever they have to push
  • some people like to do stacked-pr workflows (especially in non-branch-based systems like gerrit) where you have multiple non-master branches chaining off each other, each of which intends to be merged back into master in sequence

in the first place, how do you even know what the repo's "primary" branch is? master? main? trunk? what if you're in a project that has long-lived release maintenance branches? remember, HEAD's upstream might not be any of these. you have to exclude ancestors of some non-HEAD branches or otherwise you would be grabbing the entire history every time. the current behavior is essentially the most conservative possible thing - it only takes commits that are unique to your current branch, the rationale being you probably don't want to rebase into commits that are shared elsewhere.

this is also why your logging idea doesn't make sense - for most users, the stack search stops because it reaches a commit that's an ancestor of master. the warning would get printed every time. (and remember, you don't know what "master" is called in whatever repo you're in.) there's no good balance between "principle of least surprise" and "don't make noise in normal operation". i would be open to config options to change how the default is inferred, except that most possible implementations are themselves nonobvious and ambiguous (eg: https://github.com/tummychow/git-scripts/blob/master/git-gpr-python). it's much easier to just make the user specify a base, as is also required by the actual rebase command

Even with the force flag, absorbing changes that sit on top of
a detached commit that is also pointed to by another branch
means the staged changes are skipped and nothing is committed.

Hiding commits pointed to by a branch other than HEAD was
added in 17ec779, and a
detached head doesn't have a branch name, so the commit is
hidden.
Absorbing changes that sit on top of a commit that is also pointed
to by another branch means the staged changes are skipped and nothing
is committed.

Hiding commits pointed to by a branch other than HEAD was
added in 17ec779.
So long as the detached head isn't pointed to by another branch,
the --force flag will cause staged changes to be committed.
@blairconrad blairconrad force-pushed the characterize-behaviour branch from 9a2990f to 11e47a8 Compare February 23, 2025 19:56
@blairconrad
Copy link
Contributor Author

Rebased.

Thanks for the explanation of the search-stopping behaviour. I appreciate the time you put into it.
I will have to reread to fully understand all the ramifications, but for sure one workflow I wasn't factoring in Gerrit (which I'd never heard of).

this is also why your logging idea doesn't make sense - for most users, the stack search stops because it reaches a commit that's an ancestor of master. the warning would get printed every time

Ah, yes. Thanks. I see how it wouldn't work as I imagined it. Perhaps something not unlike the "WARN Could not find a commit to fix up, use --base to increase the search range." that's triggered _only when a commit can't be made for a given hunk. Just thinking out loud. I'll go away with my thoughts and either come back with something that's a little better-developed or will forget about it.

@blairconrad
Copy link
Contributor Author

blairconrad commented Feb 24, 2025

Ah. Things are coming a little more clear. I think the words I added to the docs are insufficient. The hiding of heads also hides descendants of HEAD as well. In fact, if I read things correctly, that's the whole point of #9.

Dropping this PR for now. I've given the commits that were worth keeping to #155. Thanks.

@blairconrad blairconrad deleted the characterize-behaviour branch February 27, 2025 12:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants