Giter Site home page Giter Site logo

Comments (3)

dscho avatar dscho commented on August 24, 2024

The complexity of this project stems from the fact that the am rebase backend (which is currently the only one that supports the --whitespace=fix option) merely passes this option through to git apply, which works very differently from the merge backend.

In the merge backend, we ultimately have to follow the code path of what we do for --ignore-whitespace, i.e. we

A viable approach to add --whitespace=fix support here would be to add a new XDF_* flag that allows to transmogrify the lines ("records" in xdiff speak) in a fashion similar to XDF_IGNORE_WHITESPACE_CHANGE, but via a callback. This callback would then be used to fix the pre-image as well as the post-image. And that callback would need to call ws_fix_copy() much in the same way as apply.c does.

One particularly tricky aspect about trying to do --whitespace=fix in cherry-picks (which rebase essentially executes in a loop) is that in contrast to the apply command, xmerge works on two diffs. Essentially, cherry-picking a commit C boils down to combining the diffs C^..HEAD and C^..C. And only the post-image lines (without context lines!) of C^..C need to be transformed via ws_fix_copy()!

That is very different from what apply has to do: it needs to be able to match pre-image lines the may, or may not, violate the white-space rules, against post-image lines that again may, or may not, violate the white-space rules, and therefore apply needs to process both pre-image and post-image via ws_fix_copy(), even if only the fixed + lines will make it into the final result.

However, we still face a bit of a problem in case C^..C has a context line that has white-space issues and that has been fixed in HEAD: xmerge would mark those as conflicts, even if there should not be any conflict. The same issue applies to pre-image lines in C^..C which had white-space issues that were fixed in C^..HEAD.

So essentially, before we generate conflicts for overlapping line ranges, we have to be careful to treat white-space fixes in C^..HEAD as non-conflicting changes and quietly appending the white-space fixes via xdl_append_merge()/xdl_cleanup_merge() instead. We can do this by calling xdl_do_diff() on the respective xscr1 line range while applying ws_fix_copy() to the pre-image.

Not exactly trivial, right?

So now that I scared the heck out of most, for those readers who made it this far, here is an alternative to consider: apply ws_fix_copy() in xdl_recmatch() and in xdl_hash_record_with_whitespace() for both generated diffs, much like --ignore-whitespace does. And then also when copying the non-conflicting changes (here and here). For large files, it might be too expensive. But then, it might be okay, and much simpler to implement?

After this is implemented, we still have a little bit more to do to actually finish this ticket up: we have to cope with WS_BLANK_AT_EOF. But that will be substantially easier than supporting the rest of --whitespace=fix in the merge rebase backend...

from git.

phil-blain avatar phil-blain commented on August 24, 2024

Hi Dscho, nice write-up :) I had a feeling that when I wrote this:

I recently learned that 'git rebase --whitespace=fix' exists, which is also
great but since it uses the apply backend, it can't be used with --update-refs.
I understand this, and the fact that adding '--whitespace=fix' to the merge
backend would be a big task; this is not what this email is about.

" a big ask" was an understatement. With what you outline above, I understand more what it entails.

A few questions:

  • this would not be specific to --interactive (as you write in the description), but more to the merge backend in general. Why not title the ticket "rebase: allow using --whitespace=fix with the merge backend" then?
  • should this be only --whitespace=fix, or should other --whitespace=* values also be implemented ? I agree fix is arguably the most useful...

Also, I thought of this:

And that callback would need to call ws_fix_copy() much in the same way as apply.c does.

ws_fix_copy needs a whitespace_rule, so the callback will also need to call ws.c::whitespace_rule to get the relevant rule for the path.

from git.

dscho avatar dscho commented on August 24, 2024
  • Why not title the ticket "rebase: allow using --whitespace=fix with the merge backend" then?

I tried to be mindful of the motivation. And honestly, to me it sounds much more enticing to teach the interactive rebase to fix white-space issues than to teach the merge rebase backend (which most Git users do not even know exists and is used by default if git rebase <commit> is called).

  • should this be only --whitespace=fix, or should other --whitespace=* values also be implemented ? I agree fix is arguably the most useful...

Indeed. And I don't want to make the big ask of supporting git rebase -i --whitespace=fix even bigger. The --whitespace=* ask can easily be separated because, quite frankly, it is a separate concern.

And that callback would need to call ws_fix_copy() much in the same way as apply.c does.

ws_fix_copy needs a whitespace_rule, so the callback will also need to call ws.c::whitespace_rule to get the relevant rule for the path.

What the callback will actually need is a pointer to a cb_data that contains the whitespace rules, among other things. It's not only those rules, after all.

from git.

Related Issues (20)

Recommend Projects

  • React photo React

    A declarative, efficient, and flexible JavaScript library for building user interfaces.

  • Vue.js photo Vue.js

    🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.

  • Typescript photo Typescript

    TypeScript is a superset of JavaScript that compiles to clean JavaScript output.

  • TensorFlow photo TensorFlow

    An Open Source Machine Learning Framework for Everyone

  • Django photo Django

    The Web framework for perfectionists with deadlines.

  • D3 photo D3

    Bring data to life with SVG, Canvas and HTML. 📊📈🎉

Recommend Topics

  • javascript

    JavaScript (JS) is a lightweight interpreted programming language with first-class functions.

  • web

    Some thing interesting about web. New door for the world.

  • server

    A server is a program made to process requests and deliver data to clients.

  • Machine learning

    Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.

  • Game

    Some thing interesting about game, make everyone happy.

Recommend Org

  • Facebook photo Facebook

    We are working to build community through open source technology. NB: members must have two-factor auth.

  • Microsoft photo Microsoft

    Open source projects and samples from Microsoft.

  • Google photo Google

    Google ❤️ Open Source for everyone.

  • D3 photo D3

    Data-Driven Documents codes.