* correct git merge behavior or corner case?
@ 2009-04-19 22:40 Tuncer Ayaz
2009-04-20 3:19 ` Shawn O. Pearce
2009-04-20 9:49 ` Johannes Schindelin
0 siblings, 2 replies; 19+ messages in thread
From: Tuncer Ayaz @ 2009-04-19 22:40 UTC (permalink / raw)
To: git
I have stumbled upon the following blog post via one of
the news aggegrators and wondered whether the behavior
is correct and expected or he's expecting something wrong
or doing something wrong.
I cannot see a wrong usage pattern from what he has written.
http://blog.teksol.info/2009/04/15/beware-of-gits-content-tracking.html
as the author hasn't posted here after a couple of days
I decided to take his question here for at least understanding
what behavior he is experiencing.
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: correct git merge behavior or corner case?
2009-04-19 22:40 correct git merge behavior or corner case? Tuncer Ayaz
@ 2009-04-20 3:19 ` Shawn O. Pearce
2009-04-20 9:49 ` Johannes Schindelin
1 sibling, 0 replies; 19+ messages in thread
From: Shawn O. Pearce @ 2009-04-20 3:19 UTC (permalink / raw)
To: Tuncer Ayaz; +Cc: git
Tuncer Ayaz <tuncer.ayaz@gmail.com> wrote:
> I have stumbled upon the following blog post via one of
> the news aggegrators and wondered whether the behavior
> is correct and expected or he's expecting something wrong
> or doing something wrong.
>
> I cannot see a wrong usage pattern from what he has written.
>
> http://blog.teksol.info/2009/04/15/beware-of-gits-content-tracking.html
>
> as the author hasn't posted here after a couple of days
> I decided to take his question here for at least understanding
> what behavior he is experiencing.
Well, the author of the blog post gave us *no* information about
what he did, or what he expected. He's just showing us a diff,
which we're not even sure how he created, and then complaining
about Git not doing something he wanted.
--
Shawn.
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: correct git merge behavior or corner case?
2009-04-19 22:40 correct git merge behavior or corner case? Tuncer Ayaz
2009-04-20 3:19 ` Shawn O. Pearce
@ 2009-04-20 9:49 ` Johannes Schindelin
2009-04-20 10:10 ` Anders Melchiorsen
1 sibling, 1 reply; 19+ messages in thread
From: Johannes Schindelin @ 2009-04-20 9:49 UTC (permalink / raw)
To: Tuncer Ayaz; +Cc: git
Hi,
On Mon, 20 Apr 2009, Tuncer Ayaz wrote:
> I have stumbled upon the following blog post via one of the news
> aggegrators and wondered whether the behavior is correct and expected or
> he's expecting something wrong or doing something wrong.
>
> I cannot see a wrong usage pattern from what he has written.
>
> http://blog.teksol.info/2009/04/15/beware-of-gits-content-tracking.html
>
> as the author hasn't posted here after a couple of days I decided to
> take his question here for at least understanding what behavior he is
> experiencing.
First, a blog posting is a lousy place to post complaints. That just
decreased my confidence level in the competence of Francois.
Second, he does not state what he expects to see instead. And for the
love of God, I cannot see what should be wrong in the output he gets.
Another few notches down.
Ciao,
Dscho
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: correct git merge behavior or corner case?
2009-04-20 9:49 ` Johannes Schindelin
@ 2009-04-20 10:10 ` Anders Melchiorsen
2009-04-21 2:44 ` Jeff King
0 siblings, 1 reply; 19+ messages in thread
From: Anders Melchiorsen @ 2009-04-20 10:10 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: Tuncer Ayaz, git
Johannes Schindelin wrote:
> Second, he does not state what he expects to see instead. And for the
> love of God, I cannot see what should be wrong in the output he gets.
> Another few notches down.
I think that I managed to recreate what he is describing.
#!/bin/bash
cd $(mktemp -d repo.XXXXX)
git init
touch date
git add date
git commit -memptydate
git branch parallel
touch LICENSE
git add LICENSE
git commit -mLICENSE
git checkout parallel
date >date
git add date
git commit -mdate
git checkout master
git rm date
git commit -mnodate
git merge parallel
cat LICENSE
^ permalink raw reply [flat|nested] 19+ messages in thread
* correct git merge behavior or corner case?
@ 2009-04-20 14:50 François Beausoleil
2009-04-21 19:27 ` Jeff King
0 siblings, 1 reply; 19+ messages in thread
From: François Beausoleil @ 2009-04-20 14:50 UTC (permalink / raw)
To: git
Hi all,
I'm the author of http://blog.teksol.info/2009/04/15/beware-of-gits-content-tracking.html
In http://article.gmane.org/gmane.comp.version-control.git/116954,
Johannes was very critical of me, so here's the full story.
I cloned the repository and branched master to do some integration
work. On the integration branch, I created an empty file. At one
point, I merged from master and had some conflicts. While resolving
the conflicts, I happened to notice a file that didn't exist on master
had been modified.
Full transcript follows:
$ git checkout -b merge-tracking-reproduce
5b8520419833635c61bdfb9abbbdc086df512388 # integration branch
$ git merge 5ed60ecabd585972003084e07401f598d00f5a1d # master branch
$ git status
# On branch merge-tracking-reproduce
# Changes to be committed:
# (use "git reset HEAD <file>..." to unstage)
#
...
# modified: vendor/plugins/acts_as_money/LICENSE
#
# Changed but not updated:
# (use "git add <file>..." to update what will be committed)
#
... a couple of conflicts, they don't matter
$ git diff --cached vendor/plugins/acts_as_money/LICENSE
diff --git a/vendor/plugins/acts_as_money/LICENSE b/vendor/plugins/
acts_as_money/LICENSE
index e69de29..a273c73 100644
--- a/vendor/plugins/acts_as_money/LICENSE
+++ b/vendor/plugins/acts_as_money/LICENSE
@@ -0,0 +1,4 @@
+one:
+ user: active
+ name: name
+ description: description
$ git log 5b8520419833635c61bdfb9abbbdc086df512388 -- vendor/plugins/
acts_as_money/LICENSE # on integration branch
commit c25787637b6ac5d8f0783cf1688c9ce6fb135659
Author: François Beausoleil <francois@teksol.info>
Date: Thu Apr 9 14:15:03 2009 -0400
Added Quirky dependencies
$ git log 5ed60ecabd585972003084e07401f598d00f5a1d -- vendor/plugins/
acts_as_money/LICENSE # on master branch
# empty, no file by that name on master
$ git log "-Sdescription: description"
5b8520419833635c61bdfb9abbbdc086df512388 # on integration branch
$ git log "-Sdescription: description"
5ed60ecabd585972003084e07401f598d00f5a1d # on master branch
commit 36d2a29d0c3c3a2a51db5f3be9faea7cea7ff3c1
Author: Michael Lacy <mike@kluster.com>
Date: Tue Apr 14 10:16:50 2009 -0400
quirky voting
$ git diff 36d2a29d0c3c3a2a51db5f3be9faea7cea7ff3c1^..
36d2a29d0c3c3a2a51db5f3be9faea7cea7ff3c1 -- vendor/gems/thoughtbot-
shoulda-2.10.1/test/fixtures/products.yml
diff --git a/vendor/gems/thoughtbot-shoulda-2.10.1/test/fixtures/
products.yml b/vendor/gems/thoughtbot-shoulda-2.10.1/test/fixtures/
index e69de29..a273c73 100644
--- a/vendor/gems/thoughtbot-shoulda-2.10.1/test/fixtures/products.yml
+++ b/vendor/gems/thoughtbot-shoulda-2.10.1/test/fixtures/products.yml
@@ -0,0 +1,4 @@
+one:
+ user: active
+ name: name
+ description: description
Regarding Anders reproduction recipe, no file was deleted. I'm trying
to write a reproduction script, but haven't managed to reproduce it
just yet. The steps I *think* happened are thus:
cd $(mktemp -d repo.XXXXXX)
git init
touch README
git add README
git commit --message "First commit"
git checkout -b integration
mkdir -p vendor/a
touch vendor/a/LICENSE
git add vendor/a/LICENSE
git commit --message "Adding LICENSE"
git checkout master
mkdir -p vendor/b
touch vendor/b/COPYING
git add vendor/b/COPYING
git commit --message "Adding COPYING"
echo "adding some content" > vendor/b/COPYING
git commit --all --message "Updated COPYING"
git checkout integration
git merge master
git diff HEAD^..HEAD
But the bug isn't reproduced with these steps. If you want access to
the repository, please contact me privately and I'll give you access.
Johannes, you were right: I should have found the right avenue for
posting this question and done so. If not immediately, at least the
next day. It was late, I had other things to do, I just decided to
post a quick note. Call me lazy, yes. Incompetent, that's going a
bit too far. I hope my competence can be redeemed in your eyes.
Have a nice day!
--
François Beausoleil
http://blog.teksol.info/
http://piston.rubyforge.org/
^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: correct git merge behavior or corner case?
2009-04-20 10:10 ` Anders Melchiorsen
@ 2009-04-21 2:44 ` Jeff King
2009-04-21 2:51 ` Jeff King
2009-04-21 3:09 ` Junio C Hamano
0 siblings, 2 replies; 19+ messages in thread
From: Jeff King @ 2009-04-21 2:44 UTC (permalink / raw)
To: Anders Melchiorsen; +Cc: Johannes Schindelin, Tuncer Ayaz, git
On Mon, Apr 20, 2009 at 12:10:35PM +0200, Anders Melchiorsen wrote:
> I think that I managed to recreate what he is describing.
>
>
> #!/bin/bash
>
> cd $(mktemp -d repo.XXXXX)
>
> git init
>
> touch date
> git add date
> git commit -memptydate
>
> git branch parallel
>
> touch LICENSE
> git add LICENSE
> git commit -mLICENSE
>
> git checkout parallel
> date >date
> git add date
> git commit -mdate
>
> git checkout master
> git rm date
> git commit -mnodate
>
> git merge parallel
>
> cat LICENSE
So basically one branch removes a file and adds an identical file under
a different name, while the other branch modifies the original file. Git
detects it as a rename, and applies the change from the second branch to
the newly added file instead of generating a conflict.
This is _exactly_ what git's rename detection is designed to do. Yes, it
seems horribly confusing in this toy example, but that is because it is
a toy example: both 'date' and 'LICENSE' are empty files. But with real
files, if a source file has actual content but is deleted, there is a
new filename with the identical or near-identical content, and the patch
applies to the new content without conflicts, then applying it there is
probably exactly what you want.
The only complaint I have in that example is that there is nothing
indicating to the user that the patch was applied to a renamed version.
The output I get is:
$ git merge parallel
Merge made by recursive.
LICENSE | 1 +
1 files changed, 1 insertions(+), 0 deletions(-)
Perhaps a note indicating that it applied changes for "date" to
"LICENSE" would be helpful.
-Peff
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: correct git merge behavior or corner case?
2009-04-21 2:44 ` Jeff King
@ 2009-04-21 2:51 ` Jeff King
2009-04-21 3:09 ` Junio C Hamano
1 sibling, 0 replies; 19+ messages in thread
From: Jeff King @ 2009-04-21 2:51 UTC (permalink / raw)
To: Anders Melchiorsen; +Cc: Johannes Schindelin, Tuncer Ayaz, git
On Mon, Apr 20, 2009 at 10:44:33PM -0400, Jeff King wrote:
> This is _exactly_ what git's rename detection is designed to do. Yes, it
> seems horribly confusing in this toy example, but that is because it is
> a toy example: both 'date' and 'LICENSE' are empty files. But with real
> files, if a source file has actual content but is deleted, there is a
> new filename with the identical or near-identical content, and the patch
> applies to the new content without conflicts, then applying it there is
> probably exactly what you want.
Looking back over the blog post, it seems that the original question was
not about a toy example, but what looks like some boilerplate that
involved empty files.
Maybe git should refuse to detect exact renames between empty files.
That is easy enough to special-case, and would help people who have
these sorts of boilerplate hierarchies. It would mean that we fail to
automatically resolve something like:
$ touch foo && git add foo && git commit -m boilerplate
$ git branch other
$ echo content >foo && git commit -m 'fill in boilerplate'
$ git checkout other
$ git mv foo bar && git commit -m reorganize
$ git merge master
But the failure case is actually quite reasonable. We just mark it as a
conflict, which is of course trivial for the user to resolve because the
ancestor, by definition, had nothing in it.
-Peff
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: correct git merge behavior or corner case?
2009-04-21 2:44 ` Jeff King
2009-04-21 2:51 ` Jeff King
@ 2009-04-21 3:09 ` Junio C Hamano
2009-04-21 8:48 ` Sverre Rabbelier
1 sibling, 1 reply; 19+ messages in thread
From: Junio C Hamano @ 2009-04-21 3:09 UTC (permalink / raw)
To: Jeff King; +Cc: Anders Melchiorsen, Johannes Schindelin, Tuncer Ayaz, git
Jeff King <peff@peff.net> writes:
> So basically one branch removes a file and adds an identical file under
> a different name, while the other branch modifies the original file. Git
> detects it as a rename, and applies the change from the second branch to
> the newly added file instead of generating a conflict.
>
> This is _exactly_ what git's rename detection is designed to do. Yes, it
> seems horribly confusing in this toy example, but that is because it is
> a toy example: both 'date' and 'LICENSE' are empty files. But with real
> files, if a source file has actual content but is deleted, there is a
> new filename with the identical or near-identical content, and the patch
> applies to the new content without conflicts, then applying it there is
> probably exactly what you want.
I had to briefly wonder what the fallout would be if we begin special-
casing empty blobs excluded even from exact renames. We effectively do
not consider fuzzy renames for blobs smaller than certain threshold, and
sane projects would not have an empty file tracked anyway, so...
A much lessor impact change would be to keep the diffcore-rename as-is, so
that it does detect exact renames between a pair of empty files, but
special case it in merge-recursive. I think I like the latter approach
better.
In any case, here is what the damage would look like...
diffcore-rename.c | 13 +++++++++----
1 files changed, 9 insertions(+), 4 deletions(-)
diff --git a/diffcore-rename.c b/diffcore-rename.c
index 0b0d6b8..dc1f159 100644
--- a/diffcore-rename.c
+++ b/diffcore-rename.c
@@ -59,11 +59,14 @@ static struct diff_rename_src {
} *rename_src;
static int rename_src_nr, rename_src_alloc;
-static struct diff_rename_src *register_rename_src(struct diff_filespec *one,
- unsigned short score)
+static void register_rename_src(struct diff_filespec *one,
+ unsigned short score)
{
int first, last;
+ if (is_empty_blob_sha1(one->sha1))
+ return;
+
first = 0;
last = rename_src_nr;
while (last > first) {
@@ -71,7 +74,7 @@ static struct diff_rename_src *register_rename_src(struct diff_filespec *one,
struct diff_rename_src *src = &(rename_src[next]);
int cmp = strcmp(one->path, src->one->path);
if (!cmp)
- return src;
+ return;
if (cmp < 0) {
last = next;
continue;
@@ -91,7 +94,7 @@ static struct diff_rename_src *register_rename_src(struct diff_filespec *one,
(rename_src_nr - first - 1) * sizeof(*rename_src));
rename_src[first].one = one;
rename_src[first].score = score;
- return &(rename_src[first]);
+ return;
}
static int basename_same(struct diff_filespec *src, struct diff_filespec *dst)
@@ -436,6 +439,8 @@ void diffcore_rename(struct diff_options *options)
else if (options->single_follow &&
strcmp(options->single_follow, p->two->path))
continue; /* not interested */
+ else if (is_empty_blob_sha1(p->two->sha1))
+ continue; /* not interested */
else
locate_rename_dst(p->two, 1);
}
^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: correct git merge behavior or corner case?
2009-04-21 3:09 ` Junio C Hamano
@ 2009-04-21 8:48 ` Sverre Rabbelier
2009-04-21 8:56 ` Johannes Schindelin
0 siblings, 1 reply; 19+ messages in thread
From: Sverre Rabbelier @ 2009-04-21 8:48 UTC (permalink / raw)
To: Junio C Hamano
Cc: Jeff King, Anders Melchiorsen, Johannes Schindelin, Tuncer Ayaz,
git
Heya,
On Tue, Apr 21, 2009 at 05:09, Junio C Hamano <gitster@pobox.com> wrote:
> and sane projects would not have an empty file tracked anyway, so...
Except python projects that are full of empty __init__.py files.... no?
--
Cheers,
Sverre Rabbelier
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: correct git merge behavior or corner case?
2009-04-21 8:48 ` Sverre Rabbelier
@ 2009-04-21 8:56 ` Johannes Schindelin
2009-04-21 9:00 ` Sverre Rabbelier
2009-04-21 9:01 ` Johannes Schindelin
0 siblings, 2 replies; 19+ messages in thread
From: Johannes Schindelin @ 2009-04-21 8:56 UTC (permalink / raw)
To: Sverre Rabbelier
Cc: Junio C Hamano, Jeff King, Anders Melchiorsen, Tuncer Ayaz, git
Hi,
On Tue, 21 Apr 2009, Sverre Rabbelier wrote:
> On Tue, Apr 21, 2009 at 05:09, Junio C Hamano <gitster@pobox.com> wrote:
> > and sane projects would not have an empty file tracked anyway, so...
>
> Except python projects that are full of empty __init__.py files.... no?
But they would be a good example why we do _not_ want rename detection
there.
I actually agree with Junio, though, that we want this special handling of
empty files only in merge-recursive.
Ciao,
Dscho
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: correct git merge behavior or corner case?
2009-04-21 8:56 ` Johannes Schindelin
@ 2009-04-21 9:00 ` Sverre Rabbelier
2009-04-21 9:04 ` Johannes Schindelin
2009-04-21 9:01 ` Johannes Schindelin
1 sibling, 1 reply; 19+ messages in thread
From: Sverre Rabbelier @ 2009-04-21 9:00 UTC (permalink / raw)
To: Johannes Schindelin
Cc: Junio C Hamano, Jeff King, Anders Melchiorsen, Tuncer Ayaz, git
Heya,
On Tue, Apr 21, 2009 at 10:56, Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
> But they would be a good example why we do _not_ want rename detection
> there.
Yes, I would agree that they are indeed a good example. The other day
I moved a directory in a python project (containing several
sub-directories and as such several empty __init__.py files), and it
renamed them rather wrongly :P.
The diff looked something like:
Renamed root/old/a/__init__.py to root/new/b/__init__.py
Renamed root/old/b/__init__.py to root/new/c/__init__.py
Renamed root/old/c/__init__.py to root/new/a/__init__.py
While of course, a better result would have been:
Renamed root/old/a/__init__.py to root/new/a/__init__.py
Renamed root/old/b/__init__.py to root/new/b/__init__.py
Renamed root/old/c/__init__.py to root/new/c/__init__.py
--
Cheers,
Sverre Rabbelier
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: correct git merge behavior or corner case?
2009-04-21 8:56 ` Johannes Schindelin
2009-04-21 9:00 ` Sverre Rabbelier
@ 2009-04-21 9:01 ` Johannes Schindelin
2009-04-21 17:27 ` Michał Kiedrowicz
2009-04-21 17:54 ` Michał Kiedrowicz
1 sibling, 2 replies; 19+ messages in thread
From: Johannes Schindelin @ 2009-04-21 9:01 UTC (permalink / raw)
To: Sverre Rabbelier
Cc: Junio C Hamano, Jeff King, Anders Melchiorsen, Tuncer Ayaz, git
Hi,
On Tue, 21 Apr 2009, Johannes Schindelin wrote:
> I actually agree with Junio, though, that we want this special handling
> of empty files only in merge-recursive.
And this _might_ be enough (not even compile-tested due to lack of time;
the OP did not provide the test as a proper patch):
---
merge-recursive.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/merge-recursive.c b/merge-recursive.c
index 774bacd..b7ea3cd 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -343,7 +343,7 @@ static struct string_list *get_renames(struct merge_options *o,
struct string_list_item *item;
struct rename *re;
struct diff_filepair *pair = diff_queued_diff.queue[i];
- if (pair->status != 'R') {
+ if (pair->status != 'R' || !re->pair->one->size) {
diff_free_filepair(pair);
continue;
}
^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: correct git merge behavior or corner case?
2009-04-21 9:00 ` Sverre Rabbelier
@ 2009-04-21 9:04 ` Johannes Schindelin
0 siblings, 0 replies; 19+ messages in thread
From: Johannes Schindelin @ 2009-04-21 9:04 UTC (permalink / raw)
To: Sverre Rabbelier
Cc: Junio C Hamano, Jeff King, Anders Melchiorsen, Tuncer Ayaz, git
Hi,
On Tue, 21 Apr 2009, Sverre Rabbelier wrote:
> On Tue, Apr 21, 2009 at 10:56, Johannes Schindelin
> <Johannes.Schindelin@gmx.de> wrote:
> > But they would be a good example why we do _not_ want rename detection
> > there.
>
> Yes, I would agree that they are indeed a good example. The other day
> I moved a directory in a python project (containing several
> sub-directories and as such several empty __init__.py files), and it
> renamed them rather wrongly :P.
>
> The diff looked something like:
> Renamed root/old/a/__init__.py to root/new/b/__init__.py
> Renamed root/old/b/__init__.py to root/new/c/__init__.py
> Renamed root/old/c/__init__.py to root/new/a/__init__.py
>
> While of course, a better result would have been:
> Renamed root/old/a/__init__.py to root/new/a/__init__.py
> Renamed root/old/b/__init__.py to root/new/b/__init__.py
> Renamed root/old/c/__init__.py to root/new/c/__init__.py
Heh, at some point I had a patch in my tree (dunno if it still there) to
use Levenshtein for rename detection, that should have helped...
Ciao,
Dscho
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: correct git merge behavior or corner case?
2009-04-21 9:01 ` Johannes Schindelin
@ 2009-04-21 17:27 ` Michał Kiedrowicz
2009-04-21 17:54 ` Michał Kiedrowicz
1 sibling, 0 replies; 19+ messages in thread
From: Michał Kiedrowicz @ 2009-04-21 17:27 UTC (permalink / raw)
To: Johannes Schindelin
Cc: Sverre Rabbelier, Junio C Hamano, Jeff King, Anders Melchiorsen,
Tuncer Ayaz, git
Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
> Hi,
>
> On Tue, 21 Apr 2009, Johannes Schindelin wrote:
>
> > I actually agree with Junio, though, that we want this special
> > handling of empty files only in merge-recursive.
>
> And this _might_ be enough (not even compile-tested due to lack of
> time; the OP did not provide the test as a proper patch):
>
> ---
>
> merge-recursive.c | 2 +-
> 1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/merge-recursive.c b/merge-recursive.c
> index 774bacd..b7ea3cd 100644
> --- a/merge-recursive.c
> +++ b/merge-recursive.c
> @@ -343,7 +343,7 @@ static struct string_list *get_renames(struct merge_options *o, struct string_list_item *item;
> struct rename *re;
> struct diff_filepair *pair = diff_queued_diff.queue[i];
> - if (pair->status != 'R') {
> + if (pair->status != 'R' || !re->pair->one->size) {
> diff_free_filepair(pair);
> continue;
> }
This doesn't work for me (actually, it segfaults, "re" has just been
declared). However, removing "re->" solves the problem.
---
merge-recursive.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/merge-recursive.c b/merge-recursive.c
index d6f0582..9c2a77f 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -343,7 +343,7 @@ static struct string_list *get_renames(struct merge_options *o,
struct string_list_item *item;
struct rename *re;
struct diff_filepair *pair = diff_queued_diff.queue[i];
- if (pair->status != 'R') {
+ if (pair->status != 'R' || !pair->one->size) {
diff_free_filepair(pair);
continue;
}
^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: correct git merge behavior or corner case?
2009-04-21 9:01 ` Johannes Schindelin
2009-04-21 17:27 ` Michał Kiedrowicz
@ 2009-04-21 17:54 ` Michał Kiedrowicz
2009-04-21 18:05 ` Jeff King
1 sibling, 1 reply; 19+ messages in thread
From: Michał Kiedrowicz @ 2009-04-21 17:54 UTC (permalink / raw)
To: Johannes Schindelin
Cc: Sverre Rabbelier, Junio C Hamano, Jeff King, Anders Melchiorsen,
Tuncer Ayaz, git
Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
> Hi,
>
> On Tue, 21 Apr 2009, Johannes Schindelin wrote:
>
> > I actually agree with Junio, though, that we want this special
> > handling of empty files only in merge-recursive.
>
> And this _might_ be enough (not even compile-tested due to lack of
> time; the OP did not provide the test as a proper patch):
>
> ---
>
> merge-recursive.c | 2 +-
> 1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/merge-recursive.c b/merge-recursive.c
> index 774bacd..b7ea3cd 100644
> --- a/merge-recursive.c
> +++ b/merge-recursive.c
> @@ -343,7 +343,7 @@ static struct string_list *get_renames(struct
> merge_options *o, struct string_list_item *item;
> struct rename *re;
> struct diff_filepair *pair =
> diff_queued_diff.queue[i];
> - if (pair->status != 'R') {
> + if (pair->status != 'R' || !re->pair->one->size) {
> diff_free_filepair(pair);
> continue;
> }
> --
And here is a test case:
---
t/t6035-merge-suspected-rename.sh | 33 +++++++++++++++++++++++++++++++++
1 files changed, 33 insertions(+), 0 deletions(-)
diff --git a/t/t6035-merge-suspected-rename.sh b/t/t6035-merge-suspected-rename.sh
new file mode 100755
index 0000000..81615fd
--- /dev/null
+++ b/t/t6035-merge-suspected-rename.sh
@@ -0,0 +1,33 @@
+#!/bin/sh
+
+test_description='Merge-recursive merging suspected rename'
+. ./test-lib.sh
+
+test_expect_success 'setup' '
+ echo "hello" > date &&
+ git add date &&
+ git commit -m initial &&
+
+ git branch parallel &&
+
+ echo "hello" > LICENSE &&
+ cp LICENSE LICENSE-copy &&
+ git add LICENSE &&
+ git commit -m LICENSE &&
+
+ git rm date &&
+ git commit -m removed &&
+
+ git checkout parallel &&
+ date > date &&
+ git add date &&
+ git commit -m date
+'
+
+test_expect_success 'merge' '
+ git checkout master &&
+ test_must_fail git merge parallel &&
+ test_cmp LICENSE LICENSE-copy
+'
+
+test_done
^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: correct git merge behavior or corner case?
2009-04-21 17:54 ` Michał Kiedrowicz
@ 2009-04-21 18:05 ` Jeff King
2009-04-21 18:47 ` Michał Kiedrowicz
0 siblings, 1 reply; 19+ messages in thread
From: Jeff King @ 2009-04-21 18:05 UTC (permalink / raw)
To: Michał Kiedrowicz
Cc: Johannes Schindelin, Sverre Rabbelier, Junio C Hamano,
Anders Melchiorsen, Tuncer Ayaz, git
On Tue, Apr 21, 2009 at 07:54:34PM +0200, Michał Kiedrowicz wrote:
> And here is a test case:
> [...]
> + echo "hello" > date &&
> + git add date &&
> + git commit -m initial &&
> +
> + git branch parallel &&
> +
> + echo "hello" > LICENSE &&
> + cp LICENSE LICENSE-copy &&
> + git add LICENSE &&
> + git commit -m LICENSE &&
I thought the point was about _empty_ renames. This is a small but
non-zero rename.
-Peff
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: correct git merge behavior or corner case?
2009-04-21 18:05 ` Jeff King
@ 2009-04-21 18:47 ` Michał Kiedrowicz
2009-04-21 19:11 ` Jeff King
0 siblings, 1 reply; 19+ messages in thread
From: Michał Kiedrowicz @ 2009-04-21 18:47 UTC (permalink / raw)
To: Jeff King
Cc: Johannes Schindelin, Sverre Rabbelier, Junio C Hamano,
Anders Melchiorsen, Tuncer Ayaz, git
Jeff King <peff@peff.net> wrote:
> On Tue, Apr 21, 2009 at 07:54:34PM +0200, Michał Kiedrowicz wrote:
>
> > And here is a test case:
> > [...]
> > + echo "hello" > date &&
> > + git add date &&
> > + git commit -m initial &&
> > +
> > + git branch parallel &&
> > +
> > + echo "hello" > LICENSE &&
> > + cp LICENSE LICENSE-copy &&
> > + git add LICENSE &&
> > + git commit -m LICENSE &&
>
> I thought the point was about _empty_ renames. This is a small but
> non-zero rename.
>
> -Peff
Yes, you are right. But change these echos to touch and you'll see that
this bug happens if date and LICENSE are the same, not necessary empty.
Michał Kiedrowicz
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: correct git merge behavior or corner case?
2009-04-21 18:47 ` Michał Kiedrowicz
@ 2009-04-21 19:11 ` Jeff King
0 siblings, 0 replies; 19+ messages in thread
From: Jeff King @ 2009-04-21 19:11 UTC (permalink / raw)
To: Michał Kiedrowicz
Cc: Johannes Schindelin, Sverre Rabbelier, Junio C Hamano,
Anders Melchiorsen, Tuncer Ayaz, git
On Tue, Apr 21, 2009 at 08:47:01PM +0200, Michał Kiedrowicz wrote:
> > I thought the point was about _empty_ renames. This is a small but
> > non-zero rename.
>
> Yes, you are right. But change these echos to touch and you'll see that
> this bug happens if date and LICENSE are the same, not necessary empty.
Right, because it is not exactly a bug, as I explained here:
http://article.gmane.org/gmane.comp.version-control.git/117078
I say "not exactly a bug", because it is working as intended by the
software authors, but because the intended behavior is to make a
heuristic guess (some content moved from one filename to another, so we
guess that a patch against the original content should actually go
against the new location), the guess may not follow the user's
expectations.
It is easy to see how the heuristic can be fooled in a toy example. But
what we really care about is whether and how the heuristic is fooled in
the real world. In the real world, there seems to be some non-trivial
probability of removing an empty file, adding a new one elsewhere, and
then merging with somebody who touched the empty file. So it may be
worth improving the heuristic for this special case, especially because
the harm done in a false negative is relatively small.
But what is the probability of doing the same thing to a file that has
non-trivial contents? I would guess it is much less likely, and by
special-casing it as a conflict, you have a much higher chance of
bothering users who were relying on actual rename detection for their
non-trivial case[1]. Of course, I don't have actual numbers, so I'm just
guessing.
So my point is that while both are perhaps a failing of the heuristic,
only one is going to be worth tweaking the heuristic for. So that is the
one that should be included in the test case, since it is how somebody
implementing a proposed tweak can test their tweak.
-Peff
[1] On a side note, this got me thinking about how git handles rename
detection during merging. One of the things I like about git is that it
tries to be very stupid: if something is questionable during a merge, it
calls attention to it and makes it _easy_ for the user to access the
versions and resolve (and I love mergetool for this). But renames are
not like this: either they happen during auto-conflict-resolution, or
they don't. I wonder if it might be a better strategy to barf on
conflicts due removed files that could be resolved by questionable
renames, but have a post-merge "renametool" that shows the user
potential renames and lets them interactively specify the resolution.
But maybe that would just be annoying, since 99% of the time, the rename
detection gets it right.
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: correct git merge behavior or corner case?
2009-04-20 14:50 François Beausoleil
@ 2009-04-21 19:27 ` Jeff King
0 siblings, 0 replies; 19+ messages in thread
From: Jeff King @ 2009-04-21 19:27 UTC (permalink / raw)
To: François Beausoleil; +Cc: git
On Mon, Apr 20, 2009 at 10:50:25AM -0400, François Beausoleil wrote:
> Regarding Anders reproduction recipe, no file was deleted. I'm trying to
> write a reproduction script, but haven't managed to reproduce it just yet.
> The steps I *think* happened are thus:
If there is no deletion, then that seems like it might actually be a
bug.
> But the bug isn't reproduced with these steps. If you want access to the
> repository, please contact me privately and I'll give you access.
I can take a look at it if you want to mail me the details off-list.
> Johannes, you were right: I should have found the right avenue for
> posting this question and done so. If not immediately, at least the next
> day. It was late, I had other things to do, I just decided to post a
> quick note. Call me lazy, yes. Incompetent, that's going a bit too far.
> I hope my competence can be redeemed in your eyes.
I think it is OK to complain about and discuss git on your blog. That's
what it's there for. But I do think you are much more likely to get
developer attention and get your problem _fixed_ if you send a message
to the mailing list. So I don't think posting on a blog makes you
incompetent; but complaining afterwards that git developers didn't read
your blog and fix your problem might. ;)
-Peff
^ permalink raw reply [flat|nested] 19+ messages in thread
end of thread, other threads:[~2009-04-21 19:29 UTC | newest]
Thread overview: 19+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2009-04-19 22:40 correct git merge behavior or corner case? Tuncer Ayaz
2009-04-20 3:19 ` Shawn O. Pearce
2009-04-20 9:49 ` Johannes Schindelin
2009-04-20 10:10 ` Anders Melchiorsen
2009-04-21 2:44 ` Jeff King
2009-04-21 2:51 ` Jeff King
2009-04-21 3:09 ` Junio C Hamano
2009-04-21 8:48 ` Sverre Rabbelier
2009-04-21 8:56 ` Johannes Schindelin
2009-04-21 9:00 ` Sverre Rabbelier
2009-04-21 9:04 ` Johannes Schindelin
2009-04-21 9:01 ` Johannes Schindelin
2009-04-21 17:27 ` Michał Kiedrowicz
2009-04-21 17:54 ` Michał Kiedrowicz
2009-04-21 18:05 ` Jeff King
2009-04-21 18:47 ` Michał Kiedrowicz
2009-04-21 19:11 ` Jeff King
-- strict thread matches above, loose matches on Subject: below --
2009-04-20 14:50 François Beausoleil
2009-04-21 19:27 ` Jeff King
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 NNTP newsgroup(s).