Re: [PATCH v2 2/2] Avoid a segmentation fault with renaming merges

Johannes Schindelin [off-list ref] writes:

On Sat, 26 Nov 2016, Johannes Schindelin wrote:

quoted

diff --git a/merge-recursive.c b/merge-recursive.c
index 9041c2f149..609061f58a 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c

@@ -235,6 +235,8 @@ static int add_cacheinfo(struct merge_options *o,
 		struct cache_entry *nce;
 
 		nce = refresh_cache_entry(ce, CE_MATCH_REFRESH | CE_MATCH_IGNORE_MISSING);
+		if (!nce)
+			return err(o, _("addinfo: '%s' is not up-to-date"), path);
 		if (nce != ce)
 			ret = add_cache_entry(nce, options);
 	}

BTW I was not quite sure why we need to refresh the cache entry here, and
1335d76e45 (merge: avoid "safer crlf" during recording of merge results,
2016-07-08) has a commit message for which I need some time to wrap my
head around.

This callsite used to call make_cache_entry() with CE_MATCH_REFRESH,
which creates a new cache entry, calls refresh_cache_ent, and
returns the cache entry"; the log message attempts to explain why we
avoid passing CE_MATCH_REFRESH and instead first add it as a merged
entry and then refresh it (and re-add it if ce got changed).  We
used to leave the old (possibly conflicted) entries for the same
path in the index while refreshing the new cache entry, which has
correctly converted result, and the old entries got in the way,
attempting a wrong eol conversion and declaring that the new entry
out-of-date.  By adding the correctly converted result as a merged
entry, which gets rid of the old entries, the refresh operation will
not be corrupted by them.

Also, an error here may be overkill. Maybe we should simply change the "if
(nce != ce)" to an "if (nce && nce != ce)" here, as a locally-modified
file will give a nicer message later, anyway.

Looking at the commit you blamed, what happened in this case before
that change was that

 (1) make_cache_entry() would have called refresh_cache_entry() with
     CE_MATCH_REFRESH and returned a NULL;

 (2) merge-recursive.c::add_cacheinfo() noticed NULL and did

     return error(_("addinfo_cache failed for path '%s'"), path)

But the updated code forgot that refresh_cache_entry() could return
NULL.  So 1335d76e45 ("merge: avoid "safer crlf" during recording of
merge results", 2016-07-08) was not a faithful rewrite.

So I agree that your patch is the right fix; using the old message
lost by mistake in 1335d76e45 may have made it more clear that this
is a fix for a misconversion in that commit, though.

In any case, this does not seem like a new regression (1/2 applied
on v2.10.0 dies the same way), so I am inclined to queue these two
but not ship in the upcoming release for now.

Thanks.