Elijah Newren <newren@xxxxxxxxx> 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.
