Jonathan <git.jonathan.bressat@xxxxxxxxx> writes:
> Because with this merge still fail for unstaged file that has the same
> content, because unstaged file are not exactly treated the same way.
Correct. If you want to do this correctly, you'd need to make sure
that you'd clobber untracked files ONLY when you are not losing any
information.
And even with that, I think some existing users will be hurt with
this change in a huge way. They may have untracked change locally
because they are not quite done with it yet, and somebody else
throws a pull request at them that has the same change as the local
modification.
They make a trial merge, look at the result, and discard it because
there are also unwanted changes in the branch they pulled into.
$ git pull $URL $branch ;# responding to the pull request
... examine the result, finding it unsatisfactory ...
$ git reset --hard ORIG_HEAD
... now we are back to where we started; well not really ...
Now, without this change, "git pull" used to stop until they stashed
away the untracked change safely. But with this change, "git pull"
will succeed, and then "reset --hard" will discard it together with
other changes that came to us from $URL/$branch. They lost their
local, uncommitted change.
And "but you can pull the equivalent out of $URL/$branch" is not a
good excuse. They may not notice the lossage long after having
dealt with this pull request (there are busy people who are handling
many pull requests from many people) and they have been relying on
"git pull" that never clobbers their local uncommitted changes. And
when they noticed the lossage, they may not even remember which one
of pull requests happened to have an identical change as their local
change to cause this lossage, simply because "git pull" that used to
stop just continued without a noise.
So, I am not sure if this is really a good idea to begin with. It
certainly would make it slightly simpler in a trivial case, but it
surely looks like a dangerous behaviour change, especially if it is
done unconditionally.
> Our patch broke some test in t6436-merge-overwrite.sh so we think that
> we need to modify those tests to make them follow the patch.
Wait. Isn't it backwards? The existing tests _may_ be casting an
undesirable current behaviour in stone, but most of the time it is
protecting existing user's expectations. If you have an untracked
file, you can rest assured that they won't be clobbered by a merge.
So we'd need to think twice and carefully examine if it makes sense
to update the expectations. I haven't read the change to the tests,
so I cannot tell which case it is.
Thanks.
