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

Ignore fixup or squash commits in fixup_candidates_lines #65

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

guludo
Copy link
Contributor

@guludo guludo commented Sep 22, 2022

Fixup or squash commits are already ignored in fixup_candidates_files, do that in fixup_candidates_lines as well.

Fixup or squash commits are already ignored in fixup_candidates_files,
do that in fixup_candidates_lines as well.
@keis
Copy link
Owner

keis commented Sep 27, 2022

Hey, thanks for the pull request!

I need to think about this one a bit and haven't really had the time but I'll try to write down my thoughts.

First, this doesn't have a comparable effect to what is done in fixup_candidates_file. That function is pretty simple and takes the git log excluding fixups and provides the first result, and crucially still outputs a candidate even if the first commit is a filtered. This in contrast gets the candidates and the filters after.

It's possible the commit range passed to blame could be tweaked to get a "non-fixup" suggestion but that's probably pretty complex. For one it would have to consider changes to line numbers in those fixup commits that are skipped.

As the effect of the filter is pretty much equivalent to git-fixup | grep -v fixup the code could be simplified and avoid that extra rev-list

If you're changing the same line again as a previous fixup in a way it kinda makes sense to think of as a fixup to a fixup, but I guess git rebase doesn't handle that situation. And is filtering the candidate really helping? I imagine in that situation you would get nothing instead.

and a final btw after 5 years maybe it's now time to use --invert-grep #34

Sorry for the big train of thought dump, let me know what you think. If you have any examples of scenarios where you would want this is also good.

@guludo
Copy link
Contributor Author

guludo commented Oct 9, 2022

Hi, @keis. Thank you for your comments.

In my use case, I had multiple fixes for a single commit (and possibly others for other commits as well). I wanted to do them incrementally, so I could review them before performing the rebase of my feature branch. As such, showing fixup commits as candidates did not have any use for me. While piping the output to a grep commit would be reasonable, for me it wouldn't be so convenient as I am using the menu feature.

@guludo
Copy link
Contributor Author

guludo commented Oct 11, 2022

I took a second look at the code and, indeed, filtering after is somewhat awkward. Although it might work in some cases, it might not produce the desired effect in other cases.

Maybe a better fix here would be to lookup the original commit for each fixup commit found? That could be done by finding the first commit in the revision range that matches the title without the fixup! or squash! prefix.

@keis
Copy link
Owner

keis commented Dec 7, 2022

Hey, sorry for the long delay, responding to this got lost in my TODO list somewhere.

I like the idea of looking up the original commit for fixup commits that would be suggested

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