* Re: [PATCH] Honor core.precomposeUnicode in more places
2019-04-23 17:30 [PATCH] Honor core.precomposeUnicode in more places Elijah Newren
@ 2019-04-23 18:29 ` Torsten Bögershausen
2019-04-23 19:06 ` Elijah Newren
2019-04-25 14:58 ` [PATCH v2] " Elijah Newren
2019-07-26 19:47 ` [PATCH] " Jeff King
2 siblings, 1 reply; 6+ messages in thread
From: Torsten Bögershausen @ 2019-04-23 18:29 UTC (permalink / raw)
To: Elijah Newren; +Cc: git
On Tue, Apr 23, 2019 at 10:30:56AM -0700, Elijah Newren wrote:
> On Mac's HFS ("Hilarious FileSystem"? "Halfwitted FileSystem"?) --
How about "Hierarchical File System" ?
> where git sets core.precomposeUnicode to true automatically by git
> init/clone -- when a user creates a simple unicode refname (in NFC
> format) such as españa:
>
> $ git branch españa
>
> different commands would display the branch name differently. For
> example, git branch, git log --decorate, and git fast-export all used
>
> 65 73 70 61 c3 b1 61 (or "espa\xc3\xb1a")
>
> (NFC form) while show-ref would use
>
> 65 73 70 61 6e cc 83 61 (or "espan\xcc\x83a")
>
> (NFD form). A stress test for git filter-repo was tripped up by this
> inconsistency, though digging in I found that the problems could
> compound; for example, if the user ran
>
> $ git pack-refs --all
>
> and then tried to check out the branch, they would be met with:
>
> $ git checkout españa
> error: pathspec 'españa' did not match any file(s) known to git
>
> $ git checkout españa --
> fatal: invalid reference: españa
>
> $ git branch
> españa
> * master
>
> Note that the user could run the `git branch` command first and copy and
> paste the `españa` portion of the output and still see the same two
> errors. Also, if the user added --no-prune to the pack-refs command,
> then they would see three branches: master, españa, and españa (those
> last two are NFC vs. NFD forms, even if they render the same).
>
> Further, if the user had the `españa` branch checked out before
> running `git pack-refs --all`, the user would be greeted with (note
> that I'm trimming trailing output with an ellipsis):
>
> $ git rev-parse HEAD
> fatal: ambiguous argument 'HEAD': unknown revision or path...
>
> $ git status
> On branch españa
>
> No commits yet...
>
> Or worse, if the user didn't check this stuff first, running `git
> commit` will create a new commit with all changes of all of history
> being squashed into it.
>
> In addition to pack-refs, one could also get into this state with
> upload-pack or anything that calls either pack-refs or upload-pack (e.g.
> gc or clone).
>
> Add code in a few places (pack-refs, show-ref, upload-pack) to check and
> honor the setting of core.precomposeUnicode to avoid these bugs.
That's all correct, one minor question below.
>
> Signed-off-by: Elijah Newren <newren@gmail.com>
> ---
> builtin/pack-refs.c | 2 ++
> builtin/show-ref.c | 3 +++
> upload-pack.c | 2 ++
> 3 files changed, 7 insertions(+)
>
> diff --git a/builtin/pack-refs.c b/builtin/pack-refs.c
> index f3353564f9..cfbd5c36c7 100644
> --- a/builtin/pack-refs.c
> +++ b/builtin/pack-refs.c
> @@ -1,4 +1,5 @@
> #include "builtin.h"
> +#include "config.h"
> #include "parse-options.h"
> #include "refs.h"
> #include "repository.h"
> @@ -16,6 +17,7 @@ int cmd_pack_refs(int argc, const char **argv, const char *prefix)
> OPT_BIT(0, "prune", &flags, N_("prune loose refs (default)"), PACK_REFS_PRUNE),
> OPT_END(),
> };
> + git_config(git_default_config, NULL);
> if (parse_options(argc, argv, prefix, opts, pack_refs_usage, 0))
> usage_with_options(pack_refs_usage, opts);
I wonder if we could move the call to git_config() into parse_options(),
(or another common place) but I haven't checked the details yet.
Same below for show_ref().
And thankks for picking this up.
> return refs_pack_refs(get_main_ref_store(the_repository), flags);
> diff --git a/builtin/show-ref.c b/builtin/show-ref.c
> index 6a706c02a6..6456da70cc 100644
> --- a/builtin/show-ref.c
> +++ b/builtin/show-ref.c
> @@ -1,5 +1,6 @@
> #include "builtin.h"
> #include "cache.h"
> +#include "config.h"
> #include "refs.h"
> #include "object-store.h"
> #include "object.h"
> @@ -182,6 +183,8 @@ static const struct option show_ref_options[] = {
>
> int cmd_show_ref(int argc, const char **argv, const char *prefix)
> {
> + git_config(git_default_config, NULL);
> +
> argc = parse_options(argc, argv, prefix, show_ref_options,
> show_ref_usage, 0);
>
> diff --git a/upload-pack.c b/upload-pack.c
> index d098ef5982..159f751ea4 100644
> --- a/upload-pack.c
> +++ b/upload-pack.c
> @@ -1064,6 +1064,8 @@ static int upload_pack_config(const char *var, const char *value, void *unused)
> allow_ref_in_want = git_config_bool(var, value);
> } else if (!strcmp("uploadpack.allowsidebandall", var)) {
> allow_sideband_all = git_config_bool(var, value);
> + } else if (!strcmp("core.precomposeunicode", var)) {
> + precomposed_unicode = git_config_bool(var, value);
> }
>
> if (current_config_scope() != CONFIG_SCOPE_REPO) {
> --
> 2.21.0.420.g4906d192b3
>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] Honor core.precomposeUnicode in more places
2019-04-23 18:29 ` Torsten Bögershausen
@ 2019-04-23 19:06 ` Elijah Newren
2019-04-24 1:56 ` Junio C Hamano
0 siblings, 1 reply; 6+ messages in thread
From: Elijah Newren @ 2019-04-23 19:06 UTC (permalink / raw)
To: Torsten Bögershausen; +Cc: Git Mailing List
On Tue, Apr 23, 2019 at 11:29 AM Torsten Bögershausen <tboegi@web.de> wrote:
>
> On Tue, Apr 23, 2019 at 10:30:56AM -0700, Elijah Newren wrote:
> > On Mac's HFS ("Hilarious FileSystem"? "Halfwitted FileSystem"?) --
>
> How about "Hierarchical File System" ?
Sorry, I should have removed my draft commentary before submitting.
Yes, you are of course right.
...
> > Add code in a few places (pack-refs, show-ref, upload-pack) to check and
> > honor the setting of core.precomposeUnicode to avoid these bugs.
>
> That's all correct, one minor question below.
...
> > @@ -16,6 +17,7 @@ int cmd_pack_refs(int argc, const char **argv, const char *prefix)
> > OPT_BIT(0, "prune", &flags, N_("prune loose refs (default)"), PACK_REFS_PRUNE),
> > OPT_END(),
> > };
> > + git_config(git_default_config, NULL);
> > if (parse_options(argc, argv, prefix, opts, pack_refs_usage, 0))
> > usage_with_options(pack_refs_usage, opts);
>
> I wonder if we could move the call to git_config() into parse_options(),
> (or another common place) but I haven't checked the details yet.
> Same below for show_ref().
Interesting idea. However, moving it into parse_options() presumes
that either `git_default_config` will always be passed to
git_config(), or else the arguments needed by git_config() will be
explicitly passed to parse_options. A quick grep through the source
code suggests that only about 1/3 of callsites pass git_default_config
to git_config(), and passing a config-related callback function to
parse_options seems a little weird to me. Plus, the fact that 2/3 of
the callsites use a special config callback function suggests that
we'd still not catch all cases with such a change, much like I had to
update the upload_pack_config special callback below.
> And thankks for picking this up.
>
> > return refs_pack_refs(get_main_ref_store(the_repository), flags);
> > diff --git a/builtin/show-ref.c b/builtin/show-ref.c
> > index 6a706c02a6..6456da70cc 100644
> > --- a/builtin/show-ref.c
> > +++ b/builtin/show-ref.c
> > @@ -1,5 +1,6 @@
> > #include "builtin.h"
> > #include "cache.h"
> > +#include "config.h"
> > #include "refs.h"
> > #include "object-store.h"
> > #include "object.h"
> > @@ -182,6 +183,8 @@ static const struct option show_ref_options[] = {
> >
> > int cmd_show_ref(int argc, const char **argv, const char *prefix)
> > {
> > + git_config(git_default_config, NULL);
> > +
> > argc = parse_options(argc, argv, prefix, show_ref_options,
> > show_ref_usage, 0);
> >
> > diff --git a/upload-pack.c b/upload-pack.c
> > index d098ef5982..159f751ea4 100644
> > --- a/upload-pack.c
> > +++ b/upload-pack.c
> > @@ -1064,6 +1064,8 @@ static int upload_pack_config(const char *var, const char *value, void *unused)
> > allow_ref_in_want = git_config_bool(var, value);
> > } else if (!strcmp("uploadpack.allowsidebandall", var)) {
> > allow_sideband_all = git_config_bool(var, value);
> > + } else if (!strcmp("core.precomposeunicode", var)) {
> > + precomposed_unicode = git_config_bool(var, value);
> > }
> >
> > if (current_config_scope() != CONFIG_SCOPE_REPO) {
> > --
> > 2.21.0.420.g4906d192b3
> >
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] Honor core.precomposeUnicode in more places
2019-04-23 19:06 ` Elijah Newren
@ 2019-04-24 1:56 ` Junio C Hamano
0 siblings, 0 replies; 6+ messages in thread
From: Junio C Hamano @ 2019-04-24 1:56 UTC (permalink / raw)
To: Elijah Newren; +Cc: Torsten Bögershausen, Git Mailing List
Elijah Newren <newren@gmail.com> writes:
> ..., and passing a config-related callback function to
> parse_options seems a little weird to me.
A little? That's a moderate understatement.
If parse_options API were the ONLY thing that gets affected by
precompose_unicode, having an "if the config has not been read yet,
read only that configuration variable and nothing else" there might
make sense, but that is not the case (i.e. the variable also affects
how readdir() works). So the alternatives that make sense are
(1) we stick to the current "make sure we read that variable
sufficiently early in the main flow of the program" pattern,
which this patch does, or
(2) we switch to "make sure the variable is read before we need it"
pattern, i.e. add that "read the single config variable from
the file, if it is not yet read" call to both parse_options()
and opendir()---if the set of operations affected by the
precompose_unicode variable grows, we'd need to add the same
call to the new places, too.
I think (1) is good at least for now.
>> > diff --git a/upload-pack.c b/upload-pack.c
>> > index d098ef5982..159f751ea4 100644
>> > --- a/upload-pack.c
>> > +++ b/upload-pack.c
>> > @@ -1064,6 +1064,8 @@ static int upload_pack_config(const char *var, const char *value, void *unused)
>> > allow_ref_in_want = git_config_bool(var, value);
>> > } else if (!strcmp("uploadpack.allowsidebandall", var)) {
>> > allow_sideband_all = git_config_bool(var, value);
>> > + } else if (!strcmp("core.precomposeunicode", var)) {
>> > + precomposed_unicode = git_config_bool(var, value);
>> > }
What the other hunks wanted to do was quite obvious (i.e. before
calling parse_options(), make sure we know precomposed_unicode is
set appropriately, so that argv[] can be tweaked correctly). But
this one was a bit less clear.
It turns out that upload-pack deliberately avoids using the default
config callback, but tries to limit itself to the minimally needed
set, so this hunk adds the precomposed_unicode to it. By doing so,
we trigger another effect of precomposed_unicode, i.e. tweaking the
paths we read out of readdir(), so that the refs we have to offer to
the other side are all normalzied to the precomposed form.
Makes sense. Thanks.
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH v2] Honor core.precomposeUnicode in more places
2019-04-23 17:30 [PATCH] Honor core.precomposeUnicode in more places Elijah Newren
2019-04-23 18:29 ` Torsten Bögershausen
@ 2019-04-25 14:58 ` Elijah Newren
2019-07-26 19:47 ` [PATCH] " Jeff King
2 siblings, 0 replies; 6+ messages in thread
From: Elijah Newren @ 2019-04-25 14:58 UTC (permalink / raw)
To: gitster; +Cc: git, Torsten Bögershausen, Elijah Newren
On Mac's HFS where git sets core.precomposeUnicode to true automatically
by git init/clone, when a user creates a simple unicode refname (in NFC
format) such as españa:
$ git branch españa
different commands would display the branch name differently. For
example, git branch, git log --decorate, and git fast-export all used
65 73 70 61 c3 b1 61 (or "espa\xc3\xb1a")
(NFC form) while show-ref would use
65 73 70 61 6e cc 83 61 (or "espan\xcc\x83a")
(NFD form). A stress test for git filter-repo was tripped up by this
inconsistency, though digging in I found that the problems could
compound; for example, if the user ran
$ git pack-refs --all
and then tried to check out the branch, they would be met with:
$ git checkout españa
error: pathspec 'españa' did not match any file(s) known to git
$ git checkout españa --
fatal: invalid reference: españa
$ git branch
españa
* master
Note that the user could run the `git branch` command first and copy and
paste the `españa` portion of the output and still see the same two
errors. Also, if the user added --no-prune to the pack-refs command,
then they would see three branches: master, españa, and españa (those
last two are NFC vs. NFD forms, even if they render the same).
Further, if the user had the `españa` branch checked out before
running `git pack-refs --all`, the user would be greeted with (note
that I'm trimming trailing output with an ellipsis):
$ git rev-parse HEAD
fatal: ambiguous argument 'HEAD': unknown revision or path...
$ git status
On branch españa
No commits yet...
Or worse, if the user didn't check this stuff first, running `git
commit` will create a new commit with all changes of all of history
being squashed into it.
In addition to pack-refs, one could also get into this state with
upload-pack or anything that calls either pack-refs or upload-pack (e.g.
gc or clone).
Add code in a few places (pack-refs, show-ref, upload-pack) to check and
honor the setting of core.precomposeUnicode to avoid these bugs.
Signed-off-by: Elijah Newren <newren@gmail.com>
---
Changes since v1:
* Excised unnecessary portion of the first sentence of the commit message
builtin/pack-refs.c | 2 ++
builtin/show-ref.c | 3 +++
upload-pack.c | 2 ++
3 files changed, 7 insertions(+)
diff --git a/builtin/pack-refs.c b/builtin/pack-refs.c
index f3353564f9..cfbd5c36c7 100644
--- a/builtin/pack-refs.c
+++ b/builtin/pack-refs.c
@@ -1,4 +1,5 @@
#include "builtin.h"
+#include "config.h"
#include "parse-options.h"
#include "refs.h"
#include "repository.h"
@@ -16,6 +17,7 @@ int cmd_pack_refs(int argc, const char **argv, const char *prefix)
OPT_BIT(0, "prune", &flags, N_("prune loose refs (default)"), PACK_REFS_PRUNE),
OPT_END(),
};
+ git_config(git_default_config, NULL);
if (parse_options(argc, argv, prefix, opts, pack_refs_usage, 0))
usage_with_options(pack_refs_usage, opts);
return refs_pack_refs(get_main_ref_store(the_repository), flags);
diff --git a/builtin/show-ref.c b/builtin/show-ref.c
index 6a706c02a6..6456da70cc 100644
--- a/builtin/show-ref.c
+++ b/builtin/show-ref.c
@@ -1,5 +1,6 @@
#include "builtin.h"
#include "cache.h"
+#include "config.h"
#include "refs.h"
#include "object-store.h"
#include "object.h"
@@ -182,6 +183,8 @@ static const struct option show_ref_options[] = {
int cmd_show_ref(int argc, const char **argv, const char *prefix)
{
+ git_config(git_default_config, NULL);
+
argc = parse_options(argc, argv, prefix, show_ref_options,
show_ref_usage, 0);
diff --git a/upload-pack.c b/upload-pack.c
index d098ef5982..159f751ea4 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -1064,6 +1064,8 @@ static int upload_pack_config(const char *var, const char *value, void *unused)
allow_ref_in_want = git_config_bool(var, value);
} else if (!strcmp("uploadpack.allowsidebandall", var)) {
allow_sideband_all = git_config_bool(var, value);
+ } else if (!strcmp("core.precomposeunicode", var)) {
+ precomposed_unicode = git_config_bool(var, value);
}
if (current_config_scope() != CONFIG_SCOPE_REPO) {
--
2.21.0.503.g24b69a0db9
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH] Honor core.precomposeUnicode in more places
2019-04-23 17:30 [PATCH] Honor core.precomposeUnicode in more places Elijah Newren
2019-04-23 18:29 ` Torsten Bögershausen
2019-04-25 14:58 ` [PATCH v2] " Elijah Newren
@ 2019-07-26 19:47 ` Jeff King
2 siblings, 0 replies; 6+ messages in thread
From: Jeff King @ 2019-07-26 19:47 UTC (permalink / raw)
To: Elijah Newren; +Cc: git, Torsten Bögershausen
On Tue, Apr 23, 2019 at 10:30:56AM -0700, Elijah Newren wrote:
> diff --git a/builtin/pack-refs.c b/builtin/pack-refs.c
> index f3353564f9..cfbd5c36c7 100644
> --- a/builtin/pack-refs.c
> +++ b/builtin/pack-refs.c
> @@ -1,4 +1,5 @@
> #include "builtin.h"
> +#include "config.h"
> #include "parse-options.h"
> #include "refs.h"
> #include "repository.h"
> @@ -16,6 +17,7 @@ int cmd_pack_refs(int argc, const char **argv, const char *prefix)
> OPT_BIT(0, "prune", &flags, N_("prune loose refs (default)"), PACK_REFS_PRUNE),
> OPT_END(),
> };
> + git_config(git_default_config, NULL);
> if (parse_options(argc, argv, prefix, opts, pack_refs_usage, 0))
> usage_with_options(pack_refs_usage, opts);
> return refs_pack_refs(get_main_ref_store(the_repository), flags);
I coincidentally just wrote the same patch (working off a branch of
v2.21, so I didn't yet have your fix here). Just for the record, this
fixes other bugs, too!
Mine was:
diff --git a/t/t3210-pack-refs.sh b/t/t3210-pack-refs.sh
index 9ea5fa4fd2..07d09ac67a 100755
--- a/t/t3210-pack-refs.sh
+++ b/t/t3210-pack-refs.sh
@@ -253,4 +253,13 @@ test_expect_success SYMLINKS 'pack symlinked packed-refs' '
test "$(readlink .git/packed-refs)" = "my-deviant-packed-refs"
'
+test_expect_success 'pack-refs respects core.useReplaceRefs' '
+ # this is broken because the object refers to itself
+ commit=$(git rev-parse HEAD) &&
+ test_when_finished "git update-ref -d refs/replace/$commit" &&
+ git update-ref refs/replace/$commit $commit &&
+ test_must_fail git pack-refs --all &&
+ git -c core.useReplaceRefs=false pack-refs --all
+'
+
test_done
It's probably not worth adding the test independently, but I thought I'd
throw it out here for the benefit of the list archive.
-Peff
^ permalink raw reply related [flat|nested] 6+ messages in thread