On Wed, Feb 06, 2019 at 09:00:22PM -0500, Jeff King wrote: > On Wed, Feb 06, 2019 at 11:42:43AM +0100, SZEDER Gábor wrote: > > > I reported this and Peff looked into it on the way to Git Merge, but > > not working solution yet. > > > > https://public-inbox.org/git/20190129225121.GD1895@xxxxxxxxxxxxxxxxxxxxx/T/#u > > Oof. Well, now I know why my attempts to fix the test failed. It was not > my new test that was failing at all, but rather the existing test. Which > implies that I severely bungled the actual code change. > > Armed with that knowledge, it was pretty easy to find said bungling. The > fix is below. > > Junio, this should go on top of jk/add-ignore-errors-bit-assignment-fix > as soon as possible, as the regression is already in master. And I'll go > find a brown paper bag. ;) > > -- >8 -- > Subject: [PATCH] add_to_index(): convert forgotten HASH_RENORMALIZE check > > Commit 9e5da3d055 (add: use separate ADD_CACHE_RENORMALIZE flag, > 2019-01-17) switched out using HASH_RENORMALIZE in our flags field for a > new ADD_CACHE_RENORMALIZE flag. However, it forgot to convert one of the > checks for HASH_RENORMALIZE into the new flag, which totally broke "git > add --renormalize". > > To make matters even more confusing, the resulting code would racily > pass the tests! The forgotten check was responsible for defeating the > up-to-date check of the index entry. That meant that "git add > --renormalize" would refuse to renormalize things that appeared > stat-clean. But most of the time the test commands run fast enough that > the file mtime is the same as the index mtime. And thus we err on the > conservative side and re-hash the file, which is what "--renormalize" > would have wanted. > > But if you're unlucky and cross that one-second boundary between writing > the file and writing the index (which is more likely to happen on a slow > or heavily-loaded system), then the file appears stat-clean. And > "--renormalize" would effectively do nothing. > > The fix is straightforward: convert this check to use the right flag. > > Noticed-by: SZEDER Gábor <szeder.dev@xxxxxxxxx> > Signed-off-by: Jeff King <peff@xxxxxxxx> > --- > read-cache.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/read-cache.c b/read-cache.c > index 9783c493a3..accc059951 100644 > --- a/read-cache.c > +++ b/read-cache.c > @@ -746,7 +746,7 @@ int add_to_index(struct index_state *istate, const char *path, struct stat *st, > if (ignore_case) { > adjust_dirname_case(istate, ce->name); > } > - if (!(flags & HASH_RENORMALIZE)) { > + if (!(flags & ADD_CACHE_RENORMALIZE)) { > alias = index_file_exists(istate, ce->name, > ce_namelen(ce), ignore_case); > if (alias && > -- > 2.20.1.1122.g2972e48916 > Ack, of course. And trying to answer an older question: >>>The reason appears to be wrong bit mask usage >>>#define ADD_CACHE_IGNORE_ERRORS 4 >>>and >>>#define HASH_RENORMALIZE 4 What if we had renamed "flags" like this ? diff --git a/read-cache.c b/read-cache.c index 5096e395ee..f93d291613 100644 --- a/read-cache.c +++ b/read-cache.c @@ -696,20 +696,20 @@ void set_object_name_for_intent_to_add_entry(struct cache_entry *ce) oidcpy(&ce->oid, &oid); } -int add_to_index(struct index_state *istate, const char *path, struct stat *st, int flags) +int add_to_index(struct index_state *istate, const char *path, struct stat *st, int add_cache_flags) { int namelen, was_same; mode_t st_mode = st->st_mode; struct cache_entry *ce, *alias = NULL; unsigned ce_option = CE_MATCH_IGNORE_VALID|CE_MATCH_IGNORE_SKIP_WORKTREE|CE_MATCH_RACY_IS_DIRTY; - int verbose = flags & (ADD_CACHE_VERBOSE | ADD_CACHE_PRETEND); - int pretend = flags & ADD_CACHE_PRETEND; - int intent_only = flags & ADD_CACHE_INTENT; + int verbose = add_cache_flags & (ADD_CACHE_VERBOSE | ADD_CACHE_PRETEND); + int pretend = add_cache_flags & ADD_CACHE_PRETEND; + int intent_only = add_cache_flags & ADD_CACHE_INTENT; int add_option = (ADD_CACHE_OK_TO_ADD|ADD_CACHE_OK_TO_REPLACE| (intent_only ? ADD_CACHE_NEW_ONLY : 0)); int hash_flags = HASH_WRITE_OBJECT; - if (flags & ADD_CACHE_RENORMALIZE) + if (add_cache_flags & ADD_CACHE_RENORMALIZE) hash_flags |= HASH_RENORMALIZE; if (!S_ISREG(st_mode) && !S_ISLNK(st_mode) && !S_ISDIR(st_mode)) @@ -750,7 +750,7 @@ int add_to_index(struct index_state *istate, const char *path, struct stat *st, if (ignore_case) { adjust_dirname_case(istate, ce->name); } - if (!(flags & HASH_RENORMALIZE)) { + if (!(add_cache_flags & HASH_RENORMALIZE)) { alias = index_file_exists(istate, ce->name, ce_namelen(ce), ignore_case); if (alias &&
