git-absorb icon indicating copy to clipboard operation
git-absorb copied to clipboard

Fixes up a commit adjacent to the line being changed

Open nickolay opened this issue 5 years ago • 1 comments

After fixing #6, I noticed another issue with the testcase from that issue.

In the testcase the commit to absorb is removing all instances of #[cfg(feature = "rsa_keygen")] and there is an intermediate commit adding a function with that attribute:

83      }
84   
85      #[cfg(feature = "rsa_keygen")]
86   +  pub fn limbs_even_add_one(r: &mut [Limb]) {
     +      // ...
91   +  }
92   +  
93   +  #[cfg(feature = "rsa_keygen")]
94      pub fn limbs_count_low_zero_bits(v: &[Limb]) -> u16 {

git-absorb decides to fix up this intermediate commit with both the hunk removing the cfg attribute at line 85 and at line 93, while arguably line 85 removal doesn't belong to the intermediate commit, as line 85 wasn't added by the commit.

It's clear why git-absorb does it, and if it produced a fixup for an older commit introducing line 85, that fixup would conflict with the intermediate commit. Still, git-absorb could:

  • Keep the removal of line 85 in the index
  • Warn about this
  • It's also possible rewrite the history to remove line 85 from an older commit, if we don't limit ourselves to producing fixup commits on top of existing history, but that's probably out of scope for git-absorb?

nickolay avatar Jul 14 '19 21:07 nickolay

hmm... a very interesting problem. sounds like the desired behavior here would "don't absorb this hunk into this commit, if doing so would widen the range of lines affected by the commit".

i'd probably put this behavior and the current behavior behind flags (strawperson names: --conservative for this, --aggressive for the existing behavior). the default behavior would be the more conservative option, since you can always rerun with more aggressive behavior afterwards. it would leave the hunk in the index while printing an appropriate log line.

i also wonder if this case can be handled better by hg-absorb's linelog-based strategy, but that's a much larger and more complicated consideration

tummychow avatar Jul 14 '19 22:07 tummychow