Re: [PATCH] 3-way merge with file move fails when diff.renames = copies
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Nov 10, 2008, 23:49 UTC
- Message-ID
- <7v63mv3zww.fsf@gitster.siamese.dyndns.org>
- In-Reply-To
- <1226355970-2542-1-git-send-email-ddkilzer@kilzer.net>
"David D. Kilzer" <ddkilzer@kilzer.net> writes:
Show 33 quoted lines
> With diff.renames = copies, a 3-way merge (e.g. "git rebase") would
> fail with the following error:
>
> fatal: mode change for <file>, which is not in current HEAD
> Repository lacks necessary blobs to fall back on 3-way merge.
> Cannot fall back to three-way merge.
> Patch failed at 0001.
>
> The bug is a logic error added in ece7b749, which attempts to find
> an sha1 for a patch with no index line in build_fake_ancestor().
> Instead of failing unless an sha1 is found for both the old file and
> the new file, a failure should only be reported if neither the old
> file nor the new file is found.
>
> Signed-off-by: David D. Kilzer <ddkilzer@kilzer.net>
> ---
> builtin-apply.c | 2 +-
> t/t3400-rebase.sh | 17 +++++++++++++++++
> 2 files changed, 18 insertions(+), 1 deletions(-)
>
> diff --git a/builtin-apply.c b/builtin-apply.c
> index 4c4d1e1..cfeb6cc 100644
> --- a/builtin-apply.c
> +++ b/builtin-apply.c
> @@ -2573,7 +2573,7 @@ static void build_fake_ancestor(struct patch *list, const char *filename)
> else if (get_sha1(patch->old_sha1_prefix, sha1))
> /* git diff has no index line for mode/type changes */
> if (!patch->lines_added && !patch->lines_deleted) {
> - if (get_current_sha1(patch->new_name, sha1) ||
> + if (get_current_sha1(patch->new_name, sha1) &&
> get_current_sha1(patch->old_name, sha1))
> die("mode change for %s, which is not "
> "in current HEAD", name);Hmm.
The logic introduced by the blamed commit makes the --index-info unreliable (I'd rather see it fail reliably if it does not have enough information rather than pretending everything is Ok), and I think the patch makes it slightly more so.
If new_name that is not related at all to old_name happens to exist in the current tree you are applying the patch to, you can grab the contents of the unrelated file as the preimage and try to merge the changes in.
When running --index-info for the purpose of "am -3" (hence rebase), the expectation is that the tree you are applying the changes to is _similar_ to the preimage of the change, i.e. old_name. Shouldn't missing old_name be treated as a fatal condition? new_name does not have to even exist because otherwise you cannot accept a patch that creates the path.
Wouldn't this be a better patch, I wonder...
builtin-apply.c | 3 +-- 1 files changed, 1 insertions(+), 2 deletions(-)
diff --git i/builtin-apply.c w/builtin-apply.c index 4c4d1e1..7de70e9 100644 --- i/builtin-apply.c +++ w/builtin-apply.c @@ -2573,8 +2573,7 @@ static void build_fake_ancestor(struct patch *list, const char *filename) else if (get_sha1(patch->old_sha1_prefix, sha1)) /* git diff has no index line for mode/type changes */ if (!patch->lines_added && !patch->lines_deleted) { - if (get_current_sha1(patch->new_name, sha1) || - get_current_sha1(patch->old_name, sha1)) + if (get_current_sha1(patch->old_name, sha1)) die("mode change for %s, which is not " "in current HEAD", name); sha1_ptr = sha1;