Re: [PATCH] add_to_index(): convert forgotten HASH_RENORMALIZE check

["Torsten Bögershausen" <tboegi@web.de>]

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@sigill.intra.peff.net/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@gmail.com>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
>  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 &&