Creates wrong commits

[est31]

Do the following

git clone https://github.com/est31/ring
cd ring
git checkout git_absorb_bug
git reset HEAD^
git add .
git absorb

Then you'll get a bunch of okay commits but some commits will be totally wrong and change stuff that actually wasn't supposed to be changed. Edit: also, thanks for creating git-absorb, it's a wonderful idea to have this for git as well!

[tummychow]

Yeah that's a bug alright. Looks like the blob in apply_hunk_to_tree has some off by one errors (specific to one-sided hunks that contain only additions or only deletions?). I'll hack up a fix over the weekend probably.

…

On Thu, Dec 13, 2018, 17:30 est31 ***@***.*** wrote: Do the following git clone https://github.com/est31/ringcd ring git checkout git_absorb_bug git reset HEAD^ git add . git absorb Then you'll get a bunch of okay commits but some commits will be totally wrong and change stuff that actually wasn't supposed to be changed. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#6>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AEFieXHu-ietzSLDle6jJHsFCk_bZIsPks5u4v82gaJpZM4ZSujV> .

[est31]

Thanks! Optimally it would have a sanity check to compare the index before and after it ran. If everything went right, the two would be the same. I'll file another issue for that.

[nickolay]

There seem to be two problems here:

(To make sense of line numbers, you have to reproduce the bug and look at the created commits while reading this. I had to remove the "foreign author" check in order to reproduce.)

1. The first hunk in every file (e.g. limb.rs in the testcase) - and from the looks of it, any hunk with removals? - is applied incorrectly due to an off-by-one error: instead of removing 1 line starting with line 77, two lines starting with 78 are removed.
   • It seems that the line numbers in hunks are 1-based, so skip_past_nth should use .nth(n-1), which will return the index of the n-th newline character, so that skip_past_nth's return value is the index of the first character of the n+1-th line (where n is 1-based line number).

2. If there's more than one hunk per file, the 2nd and the following hunks apply at incorrect locations. In the testcase the first hunk of limb.rs was -77,1 +76,0 and the second hunk was -85,1 +83,0, where 77 and 85 are the line numbers in the same (HEAD) version.
   When we create the fixup commit for the second hunk, its parent is the first fixup commit, so the line numbers should be adjusted.
   Instead the second fixup commit is created as -85,1 (assuming the first problem is patched) in the file adjusted by the first fixup, thus removing a wrong line.
   • Looks like iterating over a file's hunks in reverse order ('hunk: for index_hunk in index_patch.hunks.iter().rev()) fixes this; I assume but haven't checked that hunks are sorted by their position in the file.

@tummychow does this make sense? If so, I can whip up a PR.

I don't see existing tests for the code in lib.rs, and it seems hard to unit test it. Would it be OK to use shell scripts invoking git for the tests, for now?

[nickolay]

Looks like iterating over a file's hunks in reverse order fixes this

Alas, this breaks multiple additions. The other idea I had is to restart the absorption after creating each fixup commit - this works with the testcases I posted in #14, but not with my real tree, and I have yet to figure out why.

[tummychow]

The first hunk in every file (e.g. limb.rs in the testcase) - and from the looks of it, any hunk with removals? - is applied incorrectly due to an off-by-one error: instead of removing 1 line starting with line 77, two lines starting with 78 are removed.

yep, this sounds about right to me

If there's more than one hunk per file, the 2nd and the following hunks apply at incorrect locations. In the testcase the first hunk of limb.rs was -77,1 +76,0 and the second hunk was -85,1 +83,0, where 77 and 85 are the line numbers in the same (HEAD) version.
When we create the fixup commit for the second hunk, its parent is the first fixup commit, so the line numbers should be adjusted.

i think this makes sense to me. rough guess of the fix: each time we apply a hunk in a file, all the subsequent hunks in that file should be offset by the net lines that the applied hunk added or removed. if it winds up being tricky, you're welcome to leave that out and just fix the off-by-one errors

I don't see existing tests for the code in lib.rs, and it seems hard to unit test it. Would it be OK to use shell scripts invoking git for the tests, for now?

let's not write tests that exec git, that introduces all kinds of new ways that tests can flake or fail. libgit can make test environments when we need a real repo to run against - see stack.rs for an example of what this looks like. it's admittedly rather boilerplate heavy, so don't feel obligated to write tests for this

[nickolay]

Thanks for the reply!

rough guess of the fix: each time we apply a hunk in a file

..or skip it -- that was the remaining problem I was seeing.

..all the subsequent hunks in that file should be offset by the net lines that the applied hunk added or removed.

Yep, that sounds right. I think this could be implemented by commuting with all the preceding hunks from the same file, before trying to commute with the actual commits. nope, it has to be done like you suggested.

you're welcome to leave that out and just fix the off-by-one errors

The off-by-one fix is trivial, feel free to land it without waiting for a PR from me, and please let me know if you have time/inclination to work on the other problem, otherwise I'll try to finish this.

let's not write tests that exec git, that introduces all kinds of new ways that tests can flake or fail. libgit can make test environments when we need a real repo to run against - see stack.rs

I understand that running git is sub-optimal, but I can't reuse the boilerplate from the tests in stack.rs, so I'll use my git-based tests for now.

[tummychow]

please let me know if you have time/inclination to work on the other problem, otherwise I'll try to finish this.

yeah i'm probably not gonna touch it in the immediate future, go for it

[nickolay]

@tummychow please check PR #14.

[est31]

Thanks for the fixes @nickolay !