Jeff King <peff@xxxxxxxx> writes: > On Tue, Aug 15, 2017 at 04:23:50AM +0000, Kevin Willford wrote: > >> > This read_cache_from() should be a noop, right, because it immediately >> > sees istate->initialized is set? So it shouldn't matter that it is not >> > in the conditional with discard_cache(). Though if its only purpose is >> > to re-read the just-discarded contents, perhaps it makes sense to put it >> > there for readability. >> >> I thought about that and didn't know if there were cases when this would be called >> and the cache has not been loaded. It didn't look like it since it is only called from >> cmd_commit and prepare_index is called before it. Also if in the future this call would >> be made when it had not read the index yet so thought it was safest just to leave >> this as always being called since it is basically a noop if the istate->initialized is set. > > Yeah, I agree it might be safer as you have it. And hopefully the > comment above the discard would point anybody in the right direction. I agree that it would not hurt to have it outside the conditional, because read_cache_from() is a no-op when it is already populated. I however do not think it is sensible to call prepare_to_commit() without having populated the in-core index---in the function, nobody conditionally reads the in-core index, expecting that the caller might not have prepared the in-core index, when we need to do the wt-status thing, for example. >> Also based on this commit >> https://github.com/git/git/commit/2888605c649ccd423232161186d72c0e6c458a48 >> it looked like the discard_cache was added independent of the read_cache_from call, >> which made me think that the two were not tied together. That one comes from the first commit of the C reimplementation, f5bbc322 ("Port git commit to C.", 2007-11-08). If I am reading the old code correctly, read_cache_from() is called for entirely different reasons. Back in the old code, when doing "commit --patch", prepare_index() called interactive_add(), which would return to us after updating the index file in the filesystem. The caller of prepare_index(), which was cmd_commit(), needed to read that off of the filesystem hence the call is there. When not doing "--patch", prepare_index() called read_cache(), so the read_cache_from() there was a no-op. 2888605c ("builtin-commit: fix partial-commit support", 2007-11-18) was to fix the mistake of not re-reading the index from the file at that point for non "--patch" cases. Having read_cache_from() that is most of the time no-op was not sufficient and needed additional discard_cache() there.
