Re: [PATCH] add_to_index(): convert forgotten HASH_RENORMALIZE check
On Wed, Feb 06, 2019 at 09:00:22PM -0500, Jeff King wrote:
Show 65 quoted lines
> 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 &&
Johannes Schindelin
