* git add —ignore-errors causes --renormalize
@ 2019-01-17 15:22 Dmitriy Smirnov
2019-01-17 16:27 ` Jeff King
0 siblings, 1 reply; 6+ messages in thread
From: Dmitriy Smirnov @ 2019-01-17 15:22 UTC (permalink / raw)
To: git; +Cc: Kirill Likhodedov, Aleksey Pivovarov
Hello,
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: git add —ignore-errors causes --renormalize
2019-01-17 15:22 git add —ignore-errors causes --renormalize Dmitriy Smirnov
@ 2019-01-17 16:27 ` Jeff King
2019-01-17 16:45 ` Jeff King
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Jeff King @ 2019-01-17 16:27 UTC (permalink / raw)
To: Dmitriy Smirnov
Cc: Torsten Bögershausen, git, Kirill Likhodedov,
Aleksey Pivovarov
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>
---
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 related [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
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
2 siblings, 0 replies; 6+ messages in thread
From: Torsten Bögershausen @ 2019-01-19 17:08 UTC (permalink / raw)
To: Jeff King, Dmitriy Smirnov; +Cc: git, Kirill Likhodedov, Aleksey Pivovarov
On 17.01.19 17:27, 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.
...
Thanks for cleaning up my mess
^ 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
* Re: git add —ignore-errors causes --renormalize
2019-01-29 21:35 ` SZEDER Gábor
@ 2019-01-29 22:51 ` Jeff King
0 siblings, 0 replies; 6+ messages in thread
From: Jeff King @ 2019-01-29 22:51 UTC (permalink / raw)
To: SZEDER Gábor
Cc: Dmitriy Smirnov, Torsten Bögershausen, git,
Kirill Likhodedov, Aleksey Pivovarov
On Tue, Jan 29, 2019 at 10:35:33PM +0100, SZEDER Gábor wrote:
> t0025 with '--stress' usually fails within seconds when run on the
> merge of 'jk/add-ignore-errors-bit-assignment-fix' and 'master' [1]:
Thanks for the pointer. I mostly cargo-culted the test from the rest of
that script, so I didn't think too hard about race conditions. I think
the key difference is that the other tests are _actually_ using
--renormalize, which is going to re-hash the files and notice the line
ending change. And my test is not, so it will sometimes notice and
sometimes not.
It seems like we could probably solve it through some clever tweaking of
mtimes to make sure we have a consistent timestamp setup, but it's
resisting my quick efforts. I'm at an airport traveling right now, so I
may not get to it for a day or three.
-Peff
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2019-01-29 22:51 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-01-17 15:22 git add —ignore-errors causes --renormalize Dmitriy Smirnov
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
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).