* Re: git add —ignore-errors causes --renormalize
2019-01-17 16:27 ` Jeff King
@ 2019-01-17 16:45 ` Jeff King
2019-01-19 17:08 ` Torsten Bögershausen
2019-01-29 21:35 ` SZEDER Gábor
2 siblings, 0 replies; 6+ messages in thread
From: Jeff King @ 2019-01-17 16:45 UTC (permalink / raw)
To: Dmitriy Smirnov
Cc: Torsten Bögershausen, git, Kirill Likhodedov,
Aleksey Pivovarov
On Thu, Jan 17, 2019 at 11:27:11AM -0500, Jeff King wrote:
> -- >8 --
> Subject: [PATCH] add: use separate ADD_CACHE_RENORMALIZE flag
>
> Commit 9472935d81 (add: introduce "--renormalize", 2017-11-16) taught
> git-add to pass HASH_RENORMALIZE to add_to_index(), which then passes
> the flag along to index_path(). However, the flags taken by
> add_to_index() and the ones taken by index_path() are distinct
> namespaces. We cannot take HASH_* flags in add_to_index(), because they
> overlap with the ADD_CACHE_* flags we already take (in this case,
> HASH_RENORMALIZE conflicts with ADD_CACHE_IGNORE_ERRORS).
>
> We can solve this by adding a new ADD_CACHE_RENORMALIZE flag, and using
> it to set HASH_RENORMALIZE within add_to_index(). In order to make it
> clear that these two flags come from distinct sets, let's also change
> the name "newflags" in the function to "hash_flags".
By the way, I wondered if there was a good way for the compiler to help
us find an error like this. There's no type-checking here, since all of
the flags are "int", with the values #define macros. Could enums give us
better safety?
My experiments suggest no, since the compiler is pretty loose about what
it will allow, even with -Wenum-compare. In particular, I think it's
happy to allow bitwise-AND even against tags from other enums. But if
somebody can figure out a way to make it work, I'm all ears. :)
A more drastic option is to replace the flag int with a struct
containing a bitfield. I.e., something like:
struct add_cache_flags {
unsigned ignore_errors : 1;
unsigned renormalize : 1;
... etc ...
};
That gives us real type safety and eliminates the need to manually
assign numbers to each flag. But there are downsides:
- it's syntactically more awkward to modify the flags. I.e.:
foo(flags | BAR);
becomes:
struct flags new_flags = old_flags;
new_flags.bar = 1;
foo(&new_flags);
There might be some C99-isms that can help us out (though I suspect
not completely -- even if we can use anonymous structs, I don't
think there's such a thing as "initialize this anonymous struct and
then modify these fields").
- there's no grouping, so you can't mask off certain values
So I dunno. Maybe that road is too painful, and we're stuck with what C
gives us.
-Peff
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: git add —ignore-errors causes --renormalize
2019-01-17 16:27 ` Jeff King
2019-01-17 16:45 ` Jeff King
2019-01-19 17:08 ` Torsten Bögershausen
@ 2019-01-29 21:35 ` SZEDER Gábor
2019-01-29 22:51 ` Jeff King
2 siblings, 1 reply; 6+ messages in thread
From: SZEDER Gábor @ 2019-01-29 21:35 UTC (permalink / raw)
To: Jeff King
Cc: Dmitriy Smirnov, Torsten Bögershausen, git,
Kirill Likhodedov, Aleksey Pivovarov
On Thu, Jan 17, 2019 at 11:27:11AM -0500, Jeff King wrote:
> On Thu, Jan 17, 2019 at 06:22:05PM +0300, Dmitriy Smirnov wrote:
>
> > Calling `git add —ignore-errors` appears to be equal to calling `git add —renormalize`:
> >
> > Main.java is saved with CRLF in repo
> > git config core.autocrlf = input
> >
> > $ src git:(master) echo line >> Main.java
> > $ src git:(master) git add --ignore-errors Main.java
> > $ src git:(master) git commit -m "Ignore errors"
> > [master cf24b3b] Ignore errors
> > 1 file changed, 13 insertions(+), 12 deletions(-)
> >
> >
> > The reason appears to be wrong bit mask usage
> >
> > #define ADD_CACHE_IGNORE_ERRORS 4
> > and
> > #define HASH_RENORMALIZE 4
> >
> > Looks like a regression since 2.16.0 - 9472935d81eaf9faed771878c9df0216ae0d9045
>
> Thanks for a very clear report! The patch below should fix it.
>
> -- >8 --
> Subject: [PATCH] add: use separate ADD_CACHE_RENORMALIZE flag
>
> Commit 9472935d81 (add: introduce "--renormalize", 2017-11-16) taught
> git-add to pass HASH_RENORMALIZE to add_to_index(), which then passes
> the flag along to index_path(). However, the flags taken by
> add_to_index() and the ones taken by index_path() are distinct
> namespaces. We cannot take HASH_* flags in add_to_index(), because they
> overlap with the ADD_CACHE_* flags we already take (in this case,
> HASH_RENORMALIZE conflicts with ADD_CACHE_IGNORE_ERRORS).
>
> We can solve this by adding a new ADD_CACHE_RENORMALIZE flag, and using
> it to set HASH_RENORMALIZE within add_to_index(). In order to make it
> clear that these two flags come from distinct sets, let's also change
> the name "newflags" in the function to "hash_flags".
>
> Reported-by: Dmitriy Smirnov <dmitriy.smirnov@jetbrains.com>
> Signed-off-by: Jeff King <peff@peff.net>
t0025 with '--stress' usually fails within seconds when run on the
merge of 'jk/add-ignore-errors-bit-assignment-fix' and 'master' [1]:
+ echo *.txt text=auto
+ git add --renormalize *.txt
+ cat
+ sed -e s/ / /g -e s/ */ /g
+ sort
+ git ls-files --eol
+ test_cmp expect actual
+ diff -u expect actual
--- expect 2019-01-29 21:27:34.043344898 +0000
+++ actual 2019-01-29 21:27:34.055345252 +0000
@@ -1,3 +1,3 @@
-i/lf w/crlf attr/text=auto CRLF.txt
+i/crlf w/crlf attr/text=auto CRLF.txt
i/lf w/lf attr/text=auto LF.txt
-i/lf w/mixed attr/text=auto CRLF_mix_LF.txt
+i/mixed w/mixed attr/text=auto CRLF_mix_LF.txt
error: last command exited with $?=1
not ok 2 - renormalize CRLF in repo
#
# echo "*.txt text=auto" >.gitattributes &&
# git add --renormalize "*.txt" &&
# cat >expect <<-\EOF &&
# i/lf w/crlf attr/text=auto CRLF.txt
# i/lf w/lf attr/text=auto LF.txt
# i/lf w/mixed attr/text=auto CRLF_mix_LF.txt
# EOF
# git ls-files --eol |
# sed -e "s/ / /g" -e "s/ */ /g" |
# sort >actual &&
# test_cmp expect actual
#
[1] 'jk/add-ignore-errors-bit-assignment-fix' doesn't contains
'--stress' yet, hence the merge to 'master'.
> ---
> builtin/add.c | 2 +-
> cache.h | 1 +
> read-cache.c | 8 ++++----
> t/t0025-crlf-renormalize.sh | 9 +++++++++
> 4 files changed, 15 insertions(+), 5 deletions(-)
>
> diff --git a/builtin/add.c b/builtin/add.c
> index 12247b48fd..f481ae548e 100644
> --- a/builtin/add.c
> +++ b/builtin/add.c
> @@ -137,7 +137,7 @@ static int renormalize_tracked_files(const struct pathspec *pathspec, int flags)
> continue; /* do not touch non blobs */
> if (pathspec && !ce_path_match(&the_index, ce, pathspec, NULL))
> continue;
> - retval |= add_file_to_cache(ce->name, flags | HASH_RENORMALIZE);
> + retval |= add_file_to_cache(ce->name, flags | ADD_CACHE_RENORMALIZE);
> }
>
> return retval;
> diff --git a/cache.h b/cache.h
> index 49713cc5a5..11b19505fc 100644
> --- a/cache.h
> +++ b/cache.h
> @@ -745,6 +745,7 @@ extern int index_name_pos(const struct index_state *, const char *name, int name
> #define ADD_CACHE_JUST_APPEND 8 /* Append only; tree.c::read_tree() */
> #define ADD_CACHE_NEW_ONLY 16 /* Do not replace existing ones */
> #define ADD_CACHE_KEEP_CACHE_TREE 32 /* Do not invalidate cache-tree */
> +#define ADD_CACHE_RENORMALIZE 64 /* Pass along HASH_RENORMALIZE */
> extern int add_index_entry(struct index_state *, struct cache_entry *ce, int option);
> extern void rename_index_entry_at(struct index_state *, int pos, const char *new_name);
>
> diff --git a/read-cache.c b/read-cache.c
> index bfff271a3d..9783c493a3 100644
> --- a/read-cache.c
> +++ b/read-cache.c
> @@ -703,10 +703,10 @@ int add_to_index(struct index_state *istate, const char *path, struct stat *st,
> int intent_only = 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 newflags = HASH_WRITE_OBJECT;
> + int hash_flags = HASH_WRITE_OBJECT;
>
> - if (flags & HASH_RENORMALIZE)
> - newflags |= HASH_RENORMALIZE;
> + if (flags & ADD_CACHE_RENORMALIZE)
> + hash_flags |= HASH_RENORMALIZE;
>
> if (!S_ISREG(st_mode) && !S_ISLNK(st_mode) && !S_ISDIR(st_mode))
> return error(_("%s: can only add regular files, symbolic links or git-directories"), path);
> @@ -762,7 +762,7 @@ int add_to_index(struct index_state *istate, const char *path, struct stat *st,
> }
> }
> if (!intent_only) {
> - if (index_path(istate, &ce->oid, path, st, newflags)) {
> + if (index_path(istate, &ce->oid, path, st, hash_flags)) {
> discard_cache_entry(ce);
> return error(_("unable to index file '%s'"), path);
> }
> diff --git a/t/t0025-crlf-renormalize.sh b/t/t0025-crlf-renormalize.sh
> index 9d9e02a211..e13363ade5 100755
> --- a/t/t0025-crlf-renormalize.sh
> +++ b/t/t0025-crlf-renormalize.sh
> @@ -27,4 +27,13 @@ test_expect_success 'renormalize CRLF in repo' '
> test_cmp expect actual
> '
>
> +test_expect_success 'ignore-errors not mistaken for renormalize' '
> + git reset --hard &&
> + echo "*.txt text=auto" >.gitattributes &&
> + git ls-files --eol >expect &&
> + git add --ignore-errors "*.txt" &&
> + git ls-files --eol >actual &&
> + test_cmp expect actual
> +'
> +
> test_done
> --
> 2.20.1.689.g635a1dda8a
>
^ permalink raw reply [flat|nested] 6+ messages in thread