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

From: Junio C Hamano <gitster@pobox.com>
Date: Nov 28, 2016, 18:42 UTC
Message-ID: <xmqqmvgjjvk5.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <alpine.DEB.2.20.1611261348420.117539@virtualbox>

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

Show 20 quoted lines

> On Sat, 26 Nov 2016, Johannes Schindelin wrote:
>
>> 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.