Skip to content

Comments

Don't sort hunks based on dest_commit.#123

Merged
tummychow merged 1 commit intotummychow:masterfrom
rbartlensky:rb/off-by-one
Mar 9, 2025
Merged

Don't sort hunks based on dest_commit.#123
tummychow merged 1 commit intotummychow:masterfrom
rbartlensky:rb/off-by-one

Conversation

@rbartlensky
Copy link
Contributor

Hey!

I kept running into #116 and I finally had some time to sit down and bisect with a small repro I had available. I haven't tested with the repro from #116 but I believe it is the same issue.

I hope I'll have some time in the next {weeks|months} to open a PR to fix the now broken --one-fixup-per-commit feature (not 100% broken, but it doesn't work as advertised in some cases) and add some tests to catch such issues in the future.

@tummychow
Copy link
Owner

ah, thanks for your investigation. probably we have to commute the hunks to correct their line offsets while sorting them...

@rbartlensky
Copy link
Contributor Author

rbartlensky commented Oct 19, 2024

ah, thanks for your investigation. probably we have to commute the hunks to correct their line offsets while sorting them...

Another idea I had the other day was to change the main loop from:

for patch in patches {
  for hunk in hunks {
    for commit in stack {
    }
  }
}

to

for commit in stack {
  for patch in patches {
    for hunk in hunks {
    }
  }
}

and that way we can keep the same logic, and in the end the fixups will be sorted already by commit. However, it's easier said than done, since I have to move a lot of code around, which might introduce even more bugs if I'm not careful. Moreover I'm not familiar enough with the code to know whether changing the ordering might have other side-effects that I'm not seeing.

@rbartlensky
Copy link
Contributor Author

@tummychow could we merge this one to fix the sorting bug for the default case where someone is not using --one-fixup-per-commit? Since the bug happens with or without that flag I think it might be worth merging and leaving one-fixup-per-commit slightly buggy in some cases

@tummychow
Copy link
Owner

i would be willing to merge this even though it's not a complete fix, but it was failing ci. the commit is too old now so i can't rerun gh actions. do you want to rebase it and figure out what was failing?

Because each hunk needs to be displaced based on the previously
applied hunk, sorting hunks will change the order in which they are
applied, therefore undoing all our previous calculations.

This fixes tummychow#116, but breaks the --one-fixup-per-commit feature in
cases where there is an odd hunk in between some hunks that could've
been merged together.
@rbartlensky
Copy link
Contributor Author

i would be willing to merge this even though it's not a complete fix, but it was failing ci. the commit is too old now so i can't rerun gh actions. do you want to rebase it and figure out what was failing?

I rebased and pushed. Hopefully all is well now :)

@tummychow tummychow merged commit ec62a15 into tummychow:master Mar 9, 2025
5 checks passed
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