Thomas Gummerer <t.gummerer@xxxxxxxxx> writes: > My analysis is in the commit message below. > > --->8--- > Subject: [PATCH] entry: fix up to date marking > > write_entry always marks cache entries up to date when > state->refresh_cache is set. This is however not always accurate, > if core.autocrlf is set in the config, a file with cr and lf line > endings exists and the file attribute is set to text or crlf in the > gitattributes. > > Most notably this makes t0025 flaky. When calling deleting the files > that will be adjusted through the automated crlf handling, and then > calling `git read-tree --reset -u HEAD`, this leads to a race between > git read-tree and the filesystem. The test currently only passes > most of the time, because the filesystem usually isn't synced between > the call to unpack_trees() and write_locked_index(). > > Currently the sequence of 1) remove files with cr and lf as line > endings, 2) `git read-tree --reset -u HEAD` 3) checking the status of > the changed files succeeds, because the index and the files are written > at the same time, so they have the same mtime. Thus when reading the > index the next time, the files are recognized as racy, and the actual > contents on the disk are checked for changes. > > If the index and the files have different mtimes however, the entry is > written to the index as up to date because of the flag set in > entry.c:write_entry(), and the contents on the filesystem are not > actually checked again, because the stat data in the index matches. > > The failures in t0025 can be consistently reproduced by introducing a > call to sync() between the call to unpack_trees() and > write_index_locked(). > > Instead of blindly marking and entry up to date in write_entry(), check > if the contents may change on disk first, and strip the CE_UPTODATE flag > in that case. Because the flag is not set, the cache entry will go > through the racy check when writing the index the first time, and > smudged if appropriate. Sorry, but I am confused by all of the above. We write the thing out with write_entry(), possibly applying smudge filters and eol conversion to the "git" representation to create the "working tree" representation in this codepath, right? The resulting file matches what the user's configuration told us to produce. Until the working tree file is changed by somebody after the above happens, we shouldn't have to check the contents of the file to see if there is a difference. By definition, that has to match the contents expected to be there by Git. The only case I can think of that the above does not hold is when the smuge/clean and the eol conversion are not a correct round-trip operation pairs, but that would be a misconfiguration. Otherwise, we'd be _always_ comparing the contents without relying on the cached stat info for any paths whose in-core and working tree representations are different, not just those that are configured with misbehaving smudge/clean pair, no? Puzzled... In this case, my hunch says that the patch is correct, your analysis also is and it is only me who is missing some crucial bits in the analysis and getting confused. Enlightenment, please? > > This fixes the flaky test as well as the underlying problem. > > Signed-off-by: Thomas Gummerer <t.gummerer@xxxxxxxxx> > --- > entry.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/entry.c b/entry.c > index 582c400..102fdfa 100644 > --- a/entry.c > +++ b/entry.c > @@ -214,6 +214,8 @@ finish: > if (!fstat_done) > lstat(ce->name, &st); > fill_stat_cache_info(ce, &st); > + if (would_convert_to_git(ce->name)) > + ce->ce_flags &= ~CE_UPTODATE; > ce->ce_flags |= CE_UPDATE_IN_BASE; > state->istate->cache_changed |= CE_ENTRY_CHANGED; > } > -- > 2.7.0.75.g3ee1e0f.dirty -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html