From: Anton Trunov <anton.a.trunov@gmail.com>
To: Junio C Hamano <gitster@pobox.com>
Cc: git@vger.kernel.org, jrnieder@gmail.com, tboegi@web.de,
sunshine@sunshineco.com, charles@hashpling.org,
Johannes.Schindelin@gmx.de
Subject: Re: [PATCH] xmerge.c: fix xdl_merge to conform with the manual
Date: Wed, 04 Mar 2015 12:43:12 +0300 [thread overview]
Message-ID: <54F6D3B0.60600@gmail.com> (raw)
In-Reply-To: <xmqqzj7takks.fsf@gitster.dls.corp.google.com>
On 03/03/15 23:32, Junio C Hamano wrote:
> Anton Trunov <anton.a.trunov@gmail.com> writes:
>
>> The git-merge manual says that the ignore-space-change,
>> ignore-all-space, ignore-space-at-eol options preserve our version
>> if their version only introduces whitespace changes to a line.
>>
>> So far if there is whitespace-only changes to both sides
>> in *all* lines their version will be used.
>
> I am having hard time understanding the last sentence, especially
> the "So far" part. Do you mean "With the current code, if ours and
> theirs change whitespaces on all lines, we take theirs"?
By "so far" I mean "until now, but not including it", i.e. the code
before applying the patch.
> I find the description in the documentation is a bit hard to read.
>
> * If 'their' version only introduces whitespace changes to a line,
> 'our' version is used;
>
> * If 'our' version introduces whitespace changes but 'their'
> version includes a substantial change, 'their' version is used;
>
> * Otherwise, the merge proceeds in the usual way.
>
> And it is unclear if your reading is correct to me. In your "So
> far" scenario, 'our' version does introduce whitespace changes and
> 'their' version does quite a bit of damage to the file (after all,
> they both change *all* lines, right?). It does not seem too wrong
> to invoke the second clause above and take 'theirs', at least to me.
Let me elaborate on this a bit.
It doesn't matter if all lines are changed or not.
The point is if all the changes in all the *changed* lines are trivial
(non-whitespace), i.e. there is no one line with substantial change on
both sides, then we just through away their version and keep our
whitespace changes.
We are talking here about non-so-probable corner-case of trivial changes
in our and their versions, perhaps an uncoordinated tabs-vs-space clean-up.
So I think I should add "changed lines" to the commit message.
For the code version before applying this patch the following scenario
will take place if "git merge -Xignore-all-space remote" gets executed.
base file:
1st line
2nd line
master file:
1st line
2nd line with substantial change
remote file:
1st line
2nd line
merge result file:
1st line
2nd line with substantial change
So essentially it does what "git merge -s ours remote" does in case if
all their changes are trivial.
This seems like reasonable solution to me: we _are_ trying to ignore
whitespace changes and we are explicit about it.
But, in the scenario with trivial changes everywhere we get a completely
different result:
base file:
1st line
2nd line
master file:
1st line
2nd line
remote file:
1st line
2nd line
merge result file:
1st line
2nd line
In my opinion if we respect the principle of least astonishment this
behavior should be fixed to:
merge result file:
1st line
2nd line
Exactly so does this patch.
> It is an entirely different matter if the behaviour the document
> describes is sane, and I didn't ask "git log" what the reasoning
> behind that second point is, but my guess is that a change made by
> them being "substantial" is a sign that it is a whitespace cleanup
> change and we should take the cleanup in such a case, perhaps?
If we want to take in their clean-up why would we use the
-Xignore-space-change option in the first place?
It looks like we're explicitly saying "we don't want any changes that
are whitespace-only", right?
And if we introduced some cleanup too what should we do when the
cleanups conflict? (exactly our case)
As far as I am concerned one should either manually resolve that kind of
conflicts without using the -Xignore-... options or just
git merge -X theirs remote.
next prev parent reply other threads:[~2015-03-04 9:42 UTC|newest]
Thread overview: 13+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-03-03 17:37 [PATCH] xmerge.c: fix xdl_merge to conform with the manual Anton Trunov
2015-03-03 20:17 ` Torsten Bögershausen
2015-03-04 7:07 ` Eric Sunshine
2015-03-04 9:55 ` Anton Trunov
2015-03-04 10:01 ` Eric Sunshine
2015-03-04 9:59 ` Anton Trunov
2015-03-03 20:32 ` Junio C Hamano
2015-03-04 9:43 ` Anton Trunov [this message]
2015-03-04 20:01 ` Junio C Hamano
2015-03-05 9:50 ` Anton Trunov
2015-03-06 8:02 ` Anton Trunov
2015-03-08 8:06 ` Junio C Hamano
2015-03-09 10:07 ` Anton Trunov
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=54F6D3B0.60600@gmail.com \
--to=anton.a.trunov@gmail.com \
--cc=Johannes.Schindelin@gmx.de \
--cc=charles@hashpling.org \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
--cc=jrnieder@gmail.com \
--cc=sunshine@sunshineco.com \
--cc=tboegi@web.de \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).