Comments (3)
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
- specify a merge option called
ignore-space-change
- which passes the
XDF_IGNORE_WHITESPACE_CHANGE
flag to xmerge - xmerge then uses this flag in the two calls to
xdl_do_diff()
that generate the two diffs that need to be merged. XDF_IGNORE_WHITESPACE_CHANGE
is then interpreted inxdl_recmatch()
, and inxdl_hash_record_with_whitespace()
, both of which are called repeatedly fromxdl_do_diff()
.
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.
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 themerge
backend" then? - should this be only
--whitespace=fix
, or should other--whitespace=*
values also be implemented ? I agreefix
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.
- 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 agreefix
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 awhitespace_rule
, so the callback will also need to callws.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)
- CI failure on the Git `win build` for `xterm-256color` HOT 1
- [FeatureRequest][git config]Add support for key names with underscores HOT 6
- Drop support for obsolete MSys environments HOT 8
- rebase: do not try to `fixup` a skipped commit HOT 9
- git clone with basic auth in url directly returns authentication failure after 401 received under some git versions HOT 1
- Admin
- gitk doesn't work on macOS Monterey HOT 1
- Make `check-whitespace` failures more helpful HOT 4
- Reimplement `git subtree` as a built-in HOT 14
- `git apply --unsafe-paths` only works in certain cases! HOT 3
- Unexpected behavior of partial --help flag HOT 2
- This issue tracker is for project ideas, *not for bug reports* HOT 5
- U
- Document `AUTO_MERGE` HOT 4
- Make `blame.ignoreRevsFile .git-blame-ignore-revs` the default HOT 1
- More data should be dumped in case of protocol error HOT 4
- [BUG] "docs/.vuepress/.temp" Can`t be gnore HOT 1
- Is it possible to consider adding a `--confirm/-c` option to `git restore` HOT 1
- Teach `git apply` to respect `core.fileMode` HOT 13
Recommend Projects
-
React
A declarative, efficient, and flexible JavaScript library for building user interfaces.
-
Vue.js
🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
-
Typescript
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
-
TensorFlow
An Open Source Machine Learning Framework for Everyone
-
Django
The Web framework for perfectionists with deadlines.
-
Laravel
A PHP framework for web artisans
-
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.
-
Visualization
Some thing interesting about visualization, use data art
-
Game
Some thing interesting about game, make everyone happy.
Recommend Org
-
Facebook
We are working to build community through open source technology. NB: members must have two-factor auth.
-
Microsoft
Open source projects and samples from Microsoft.
-
Google
Google ❤️ Open Source for everyone.
-
Alibaba
Alibaba Open Source for everyone
-
D3
Data-Driven Documents codes.
-
Tencent
China tencent open source team.
from git.