Re: [PATCH v4 2/2] merge-recursive: fix the refresh logic in update_file_flags

[Junio C Hamano <gitster@pobox.com>]

Elijah Newren <newren@gmail.com> writes:

>> Hmph, !.call_depth would avoid resetting update_wd to 0, so the only
>> difference this patch makes is when the caller of this helper passed
>> (update_wd == 0) during the outermost merge.  We did not tell
>> add_cacheinfo() to refresh, and refresh_cache_entry() was not
>> called.  But the new code forces refresh to happen for normal
>> entries.  The proposed log message explains that a refresh is needed
>> for a new cache entry, but if I am reading the code correctly, this
>> function is called with !update_wd from two places, one of which is
>> the "Adding %s" /* do not overwrite ... */ the log message mentions.
>>
>> But the other one?  When both sides added identically, we do have an
>> up-to-date result on our side already, so shouldn't we avoid forcing
>> update_wd in that case?
>
> This change doesn't force update_wd (write out a new file, also
> implies refreshing is needed), this only forces refreshing (check
> stat-related fields of existing file).
>
>> I do not think passing refresh==1 in that case will produce an
>> incorrect result, but doesn't it force an unnecessary refreshing?
>>
>> Puzzled.
>
> It does force a refreshing, and it is a necessary one based on
> merge-recursive's design.  You can verify by putting an "exit 1" right
> ...
> So, this was a bug all along for BOTH cases, we just didn't notice before.

Ah, thanks.  It was just me getting a wrong impression from the
proposed log message that only the other one needed refresh; if both
sides need a refresh, then the change is absolutely correct.

Thanks for clearing my puzzlement.  Will queue.