[PATCH 41/68] init: use strbufs to store paths
The init code predates strbufs, and uses PATH_MAX-sized buffers along with many manual checks on intermediate sizes (some of which make magic assumptions, such as that init will not create a path inside .git longer than 50 characters). We can simplify this greatly by using strbufs, which drops some hard-to-verify strcpy calls. Note that we need to update probe_utf8_pathname_composition, too, as it assumes we are passing a buffer large enough to append its probe filenames (it now just takes a strbuf, which also gets rid of the confusing "len" parameter, which was not the length of "path" but rather the offset to start writing). Some of the conversion makes new calls to git_path_buf. While we're in the area, let's also convert existing calls to git_path to the safer git_path_buf (our existing calls were passed to pretty tame functions, and so were not a problem, but it's easy to be consistent and safe here). Note that we had an explicit test that "git init" rejects long template directories. This comes from 32d1776 (init: Do not segfault on big GIT_TEMPLATE_DIR environment variable, 2009-04-18). We can drop the test_must_fail here, as we now accept this and need only confirm that we don't segfault, which was the original point of the test. Signed-off-by: Jeff King --- builtin/init-db.c| 174 --- compat/precompose_utf8.c | 12 ++-- compat/precompose_utf8.h | 2 +- git-compat-util.h| 2 +- t/t0001-init.sh | 4 +- 5 files changed, 87 insertions(+), 107 deletions(-) diff --git a/builtin/init-db.c b/builtin/init-db.c index e7d0e31..cf6a3c8 100644 --- a/builtin/init-db.c +++ b/builtin/init-db.c @@ -36,10 +36,11 @@ static void safe_create_dir(const char *dir, int share) die(_("Could not make %s writable by group"), dir); } -static void copy_templates_1(char *path, int baselen, -char *template, int template_baselen, +static void copy_templates_1(struct strbuf *path, struct strbuf *template, DIR *dir) { + size_t path_baselen = path->len; + size_t template_baselen = template->len; struct dirent *de; /* Note: if ".git/hooks" file exists in the repository being @@ -49,77 +50,64 @@ static void copy_templates_1(char *path, int baselen, * with the way the namespace under .git/ is organized, should * be really carefully chosen. */ - safe_create_dir(path, 1); + safe_create_dir(path->buf, 1); while ((de = readdir(dir)) != NULL) { struct stat st_git, st_template; - int namelen; int exists = 0; + strbuf_setlen(path, path_baselen); + strbuf_setlen(template, template_baselen); + if (de->d_name[0] == '.') continue; - namelen = strlen(de->d_name); - if ((PATH_MAX <= baselen + namelen) || - (PATH_MAX <= template_baselen + namelen)) - die(_("insanely long template name %s"), de->d_name); - memcpy(path + baselen, de->d_name, namelen+1); - memcpy(template + template_baselen, de->d_name, namelen+1); - if (lstat(path, &st_git)) { + strbuf_addstr(path, de->d_name); + strbuf_addstr(template, de->d_name); + if (lstat(path->buf, &st_git)) { if (errno != ENOENT) - die_errno(_("cannot stat '%s'"), path); + die_errno(_("cannot stat '%s'"), path->buf); } else exists = 1; - if (lstat(template, &st_template)) - die_errno(_("cannot stat template '%s'"), template); + if (lstat(template->buf, &st_template)) + die_errno(_("cannot stat template '%s'"), template->buf); if (S_ISDIR(st_template.st_mode)) { - DIR *subdir = opendir(template); - int baselen_sub = baselen + namelen; - int template_baselen_sub = template_baselen + namelen; + DIR *subdir = opendir(template->buf); if (!subdir) - die_errno(_("cannot opendir '%s'"), template); - path[baselen_sub++] = - template[template_baselen_sub++] = '/'; - path[baselen_sub] = - template[template_baselen_sub] = 0; - copy_templates_1(path, baselen_sub, -template, template_baselen_sub, -subdir); + die_errno(_("cannot opendir '%s'"), template->buf); + strbuf_addch(path, '/'); +
Re: [PATCH 41/68] init: use strbufs to store paths
On Thu, Sep 24, 2015 at 2:07 PM, Jeff King wrote: > The init code predates strbufs, and uses PATH_MAX-sized > buffers along with many manual checks on intermediate sizes > (some of which make magic assumptions, such as that init > will not create a path inside .git longer than 50 > characters). > > We can simplify this greatly by using strbufs, which drops > some hard-to-verify strcpy calls. Note that we need to > update probe_utf8_pathname_composition, too, as it assumes > we are passing a buffer large enough to append its probe > filenames (it now just takes a strbuf, which also gets rid > of the confusing "len" parameter, which was not the length of > "path" but rather the offset to start writing). > > Some of the conversion makes new calls to git_path_buf. > While we're in the area, let's also convert existing calls > to git_path to the safer git_path_buf (our existing calls > were passed to pretty tame functions, and so were not a > problem, but it's easy to be consistent and safe here). > > Note that we had an explicit test that "git init" rejects > long template directories. This comes from 32d1776 (init: Do > not segfault on big GIT_TEMPLATE_DIR environment variable, > 2009-04-18). We can drop the test_must_fail here, as we now > accept this and need only confirm that we don't segfault, > which was the original point of the test. > > Signed-off-by: Jeff King > --- > builtin/init-db.c| 174 > --- > compat/precompose_utf8.c | 12 ++-- > compat/precompose_utf8.h | 2 +- > git-compat-util.h| 2 +- > t/t0001-init.sh | 4 +- > 5 files changed, 87 insertions(+), 107 deletions(-) > > diff --git a/builtin/init-db.c b/builtin/init-db.c > index e7d0e31..cf6a3c8 100644 > --- a/builtin/init-db.c > +++ b/builtin/init-db.c > @@ -36,10 +36,11 @@ static void safe_create_dir(const char *dir, int share) > die(_("Could not make %s writable by group"), dir); > } > > -static void copy_templates_1(char *path, int baselen, > -char *template, int template_baselen, > +static void copy_templates_1(struct strbuf *path, struct strbuf *template, > DIR *dir) > { > + size_t path_baselen = path->len; > + size_t template_baselen = template->len; > struct dirent *de; > > /* Note: if ".git/hooks" file exists in the repository being > @@ -49,77 +50,64 @@ static void copy_templates_1(char *path, int baselen, > * with the way the namespace under .git/ is organized, should > * be really carefully chosen. > */ > - safe_create_dir(path, 1); > + safe_create_dir(path->buf, 1); > while ((de = readdir(dir)) != NULL) { > struct stat st_git, st_template; > - int namelen; > int exists = 0; > > + strbuf_setlen(path, path_baselen); > + strbuf_setlen(template, template_baselen); > + > if (de->d_name[0] == '.') > continue; > - namelen = strlen(de->d_name); > - if ((PATH_MAX <= baselen + namelen) || > - (PATH_MAX <= template_baselen + namelen)) > - die(_("insanely long template name %s"), de->d_name); > - memcpy(path + baselen, de->d_name, namelen+1); > - memcpy(template + template_baselen, de->d_name, namelen+1); > - if (lstat(path, &st_git)) { > + strbuf_addstr(path, de->d_name); > + strbuf_addstr(template, de->d_name); > + if (lstat(path->buf, &st_git)) { > if (errno != ENOENT) > - die_errno(_("cannot stat '%s'"), path); > + die_errno(_("cannot stat '%s'"), path->buf); > } > else > exists = 1; > > - if (lstat(template, &st_template)) > - die_errno(_("cannot stat template '%s'"), template); > + if (lstat(template->buf, &st_template)) > + die_errno(_("cannot stat template '%s'"), > template->buf); > > if (S_ISDIR(st_template.st_mode)) { > - DIR *subdir = opendir(template); > - int baselen_sub = baselen + namelen; > - int template_baselen_sub = template_baselen + namelen; > + DIR *subdir = opendir(template->buf); > if (!subdir) > - die_errno(_("cannot opendir '%s'"), template); > - path[baselen_sub++] = > - template[template_baselen_sub++] = '/'; > - path[baselen_sub] = > - template[template_baselen_sub] = 0; > - copy_templates_1(path, baselen_sub, > -
Re: [PATCH 41/68] init: use strbufs to store paths
On Tue, Sep 29, 2015 at 04:50:39PM -0700, Michael Blume wrote: > I see compile errors on my mac: > > First a whole bunch of > > ./compat/precompose_utf8.h:30:45: warning: declaration of 'struct > strbuf' will not be visible outside of this function [-Wvisibility] > void probe_utf8_pathname_composition(struct strbuf *path); Wow, my patch isn't even close to reasonable. I didn't realize because we do not compile this code at all for non-Mac platforms. Sorry. It probably needs something like this squashed in (completely untested): diff --git a/builtin/init-db.c b/builtin/init-db.c index cf6a3c8..c643054 100644 --- a/builtin/init-db.c +++ b/builtin/init-db.c @@ -283,7 +283,7 @@ static int create_default_files(const char *template_path) path = git_path_buf(&buf, "CoNfIg"); if (!access(path, F_OK)) git_config_set("core.ignorecase", "true"); - probe_utf8_pathname_composition(path); + probe_utf8_pathname_composition(&buf); } strbuf_release(&buf); diff --git a/compat/precompose_utf8.c b/compat/precompose_utf8.c index b4dd3c7..d2d2405 100644 --- a/compat/precompose_utf8.c +++ b/compat/precompose_utf8.c @@ -8,6 +8,7 @@ #include "cache.h" #include "utf8.h" #include "precompose_utf8.h" +#include "strbuf.h" typedef char *iconv_ibp; static const char *repo_encoding = "UTF-8"; @@ -45,17 +46,17 @@ void probe_utf8_pathname_composition(struct strbuf *path) if (precomposed_unicode != -1) return; /* We found it defined in the global config, respect it */ strbuf_addstr(path, auml_nfc); - output_fd = open(path, O_CREAT|O_EXCL|O_RDWR, 0600); + output_fd = open(path->buf, O_CREAT|O_EXCL|O_RDWR, 0600); if (output_fd >= 0) { close(output_fd); strbuf_setlen(path, baselen); strbuf_addstr(path, auml_nfd); - precomposed_unicode = access(path, R_OK) ? 0 : 1; + precomposed_unicode = access(path->buf, R_OK) ? 0 : 1; git_config_set("core.precomposeunicode", precomposed_unicode ? "true" : "false"); strbuf_setlen(path, baselen); strbuf_addstr(path, auml_nfc); - if (unlink(path)) - die_errno(_("failed to unlink '%s'"), path); + if (unlink(path->buf)) + die_errno(_("failed to unlink '%s'"), path->buf); } strbuf_setlen(path, baselen); } diff --git a/compat/precompose_utf8.h b/compat/precompose_utf8.h index 7fc7be5..229e772 100644 --- a/compat/precompose_utf8.h +++ b/compat/precompose_utf8.h @@ -4,6 +4,7 @@ #include #include +struct strbuf; typedef struct dirent_prec_psx { ino_t d_ino;/* Posix */ -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 41/68] init: use strbufs to store paths
Jeff King writes: > On Tue, Sep 29, 2015 at 04:50:39PM -0700, Michael Blume wrote: > >> I see compile errors on my mac: >> >> First a whole bunch of >> >> ./compat/precompose_utf8.h:30:45: warning: declaration of 'struct >> strbuf' will not be visible outside of this function [-Wvisibility] >> void probe_utf8_pathname_composition(struct strbuf *path); > > Wow, my patch isn't even close to reasonable. I didn't realize because > we do not compile this code at all for non-Mac platforms. Sorry. Perhaps the way we completely stub out the platform specific helpers contributes to this kind of gotchas? I am wondering how much additional safety we would gain if we start doing something like this. Two things to note: * "struct strbuf" needs to be visible when the compiler sees this part, which is an indication of the same issue shown in the above error message, is not addressed. * precompose_str() does not seem to be defined or used, hence removed. git-compat-util.h | 8 +--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/git-compat-util.h b/git-compat-util.h index 712de7f..6710ff7 100644 --- a/git-compat-util.h +++ b/git-compat-util.h @@ -227,9 +227,11 @@ typedef unsigned long uintptr_t; #ifdef PRECOMPOSE_UNICODE #include "compat/precompose_utf8.h" #else -#define precompose_str(in,i_nfd2nfc) -#define precompose_argv(c,v) -#define probe_utf8_pathname_composition(p) +static inline void precompose_argv(int, const char **); +static inline void probe_utf8_pathname_composition(struct strbuf *buf) +{ + ; /* no-op */ +} #endif #ifdef MKDIR_WO_TRAILING_SLASH -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 41/68] init: use strbufs to store paths
On Wed, Sep 30, 2015 at 01:00:56PM -0700, Junio C Hamano wrote: > > Wow, my patch isn't even close to reasonable. I didn't realize because > > we do not compile this code at all for non-Mac platforms. Sorry. > > Perhaps the way we completely stub out the platform specific helpers > contributes to this kind of gotchas? I am wondering how much additional > safety we would gain if we start doing something like this. I think it is an improvement, but it does not solve all of the problems. I also botched the implementation of probe_utf8_pathname_composition, and that does not get compiled on most platforms (though we _could_ compile it and just never call it). -Peff -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 41/68] init: use strbufs to store paths
On 10/01/2015 04:51 AM, Jeff King wrote: On Wed, Sep 30, 2015 at 01:00:56PM -0700, Junio C Hamano wrote: Wow, my patch isn't even close to reasonable. I didn't realize because we do not compile this code at all for non-Mac platforms. Sorry. Perhaps the way we completely stub out the platform specific helpers contributes to this kind of gotchas? I am wondering how much additional safety we would gain if we start doing something like this. I think it is an improvement, but it does not solve all of the problems. I also botched the implementation of probe_utf8_pathname_composition, and that does not get compiled on most platforms (though we _could_ compile it and just never call it). -Peff Peff, are you planing a re-roll ? Or. Junio, do you plan to fix it ? Or should I send a patch on top of pu ? The compilation can be tested under Linux like this: diff --git a/config.mak.uname b/config.mak.uname index 7486a7e..6d09bd0 100644 --- a/config.mak.uname +++ b/config.mak.uname @@ -13,6 +13,9 @@ ifdef MSVC uname_O := Windows endif +COMPAT_OBJS += compat/precompose_utf8.o +BASIC_CFLAGS += -DPRECOMPOSE_UNICODE + -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 41/68] init: use strbufs to store paths
On Fri, Oct 02, 2015 at 08:00:24AM +0200, Torsten Bögershausen wrote: > Peff, are you planing a re-roll ? > Or. Junio, do you plan to fix it ? > Or should I send a patch on top of pu ? I am on vacation, so I am hoping that somebody on OS X can confirm that the patch that I sent earlier does indeed fix it, and that Junio can squash it in. Makefile hack similar to what you posted, but I cannot actually on Linux that the code does what it should). > The compilation can be tested under Linux like this: Yeah, I did something similar to see that it at least compiled, but I don't have an easy way to actually run t3910. -Peff -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 41/68] init: use strbufs to store paths
On 30.09.15 02:23, Jeff King wrote: > On Tue, Sep 29, 2015 at 04:50:39PM -0700, Michael Blume wrote: > >> I see compile errors on my mac: >> This is my attempt, passing the test, but not fully polished. diff --git a/builtin/init-db.c b/builtin/init-db.c index 89f2c05..60b559c 100644 --- a/builtin/init-db.c +++ b/builtin/init-db.c @@ -276,7 +276,9 @@ static int create_default_files(const char *template_path) path = git_path_buf(&buf, "CoNfIg"); if (!access(path, F_OK)) git_config_set("core.ignorecase", "true"); - probe_utf8_pathname_composition(path); + /* Probe utf-8 normalization withou mangling CoNfIG */ + path = git_path_buf(&buf, "config"); + probe_utf8_pathname_composition(path, strlen(path)); } strbuf_release(&buf); diff --git a/compat/precompose_utf8.c b/compat/precompose_utf8.c index b4dd3c7..37172a4 100644 --- a/compat/precompose_utf8.c +++ b/compat/precompose_utf8.c @@ -8,6 +8,7 @@ #include "cache.h" #include "utf8.h" #include "precompose_utf8.h" +#include "strbuf.h" typedef char *iconv_ibp; static const char *repo_encoding = "UTF-8"; @@ -36,28 +37,33 @@ static size_t has_non_ascii(const char *s, size_t maxlen, size_t *strlen_c) } -void probe_utf8_pathname_composition(struct strbuf *path) +void probe_utf8_pathname_composition(char *path, int len) { static const char *auml_nfc = "\xc3\xa4"; static const char *auml_nfd = "\x61\xcc\x88"; - size_t baselen = path->len; + struct strbuf sbuf; int output_fd; if (precomposed_unicode != -1) return; /* We found it defined in the global config, respect it */ - strbuf_addstr(path, auml_nfc); - output_fd = open(path, O_CREAT|O_EXCL|O_RDWR, 0600); + strbuf_init(&sbuf, len+3); + strbuf_add(&sbuf, path, len); + strbuf_addstr(&sbuf, auml_nfc); + output_fd = open(sbuf.buf, O_CREAT|O_EXCL|O_RDWR, 0600); + fprintf(stderr, "%s/%s:%d sbuf.buf=%s\n", + __FILE__, __FUNCTION__, __LINE__, sbuf.buf); if (output_fd >= 0) { close(output_fd); - strbuf_setlen(path, baselen); - strbuf_addstr(path, auml_nfd); + strbuf_setlen(&sbuf, len); + strbuf_addstr(&sbuf, auml_nfd); + fprintf(stderr, "%s/%s:%d sbuf.buf=%s\n", + __FILE__, __FUNCTION__, __LINE__, sbuf.buf); precomposed_unicode = access(path, R_OK) ? 0 : 1; git_config_set("core.precomposeunicode", precomposed_unicode ? "true" : "false"); - strbuf_setlen(path, baselen); - strbuf_addstr(path, auml_nfc); + strcpy(path + len, auml_nfc); if (unlink(path)) die_errno(_("failed to unlink '%s'"), path); } - strbuf_setlen(path, baselen); + strbuf_release(&sbuf); } diff --git a/compat/precompose_utf8.h b/compat/precompose_utf8.h index 7fc7be5..3b73585 100644 --- a/compat/precompose_utf8.h +++ b/compat/precompose_utf8.h @@ -27,7 +27,7 @@ typedef struct { } PREC_DIR; void precompose_argv(int argc, const char **argv); -void probe_utf8_pathname_composition(struct strbuf *path); +void probe_utf8_pathname_composition(char *, int); PREC_DIR *precompose_utf8_opendir(const char *dirname); struct dirent_prec_psx *precompose_utf8_readdir(PREC_DIR *dirp); -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 41/68] init: use strbufs to store paths
Torsten Bögershausen writes: > On 30.09.15 02:23, Jeff King wrote: >> On Tue, Sep 29, 2015 at 04:50:39PM -0700, Michael Blume wrote: >> >>> I see compile errors on my mac: >>> > > This is my attempt, passing the test, but not fully polished. Thanks. > diff --git a/builtin/init-db.c b/builtin/init-db.c > index 89f2c05..60b559c 100644 > --- a/builtin/init-db.c > +++ b/builtin/init-db.c > @@ -276,7 +276,9 @@ static int create_default_files(const char *template_path) > path = git_path_buf(&buf, "CoNfIg"); > if (!access(path, F_OK)) > git_config_set("core.ignorecase", "true"); > - probe_utf8_pathname_composition(path); > + /* Probe utf-8 normalization withou mangling CoNfIG */ > + path = git_path_buf(&buf, "config"); > + probe_utf8_pathname_composition(path, strlen(path)); Hmph, Peff's quick-fix passed the original "CoNfIg" in &buf directly to probe_utf8_pathname_composition() without changing its signature. What is the reason behind these two changes? i.e. why is it inappropriate to use "CoNfIg" (and append the auml to it to use for the checking) and why does the function need to take pointer + len, only to store it in another strbuf itself? > diff --git a/compat/precompose_utf8.c b/compat/precompose_utf8.c > index b4dd3c7..37172a4 100644 > --- a/compat/precompose_utf8.c > +++ b/compat/precompose_utf8.c > @@ -8,6 +8,7 @@ > #include "cache.h" > #include "utf8.h" > #include "precompose_utf8.h" > +#include "strbuf.h" > > typedef char *iconv_ibp; > static const char *repo_encoding = "UTF-8"; > @@ -36,28 +37,33 @@ static size_t has_non_ascii(const char *s, size_t maxlen, > size_t *strlen_c) > } > > > -void probe_utf8_pathname_composition(struct strbuf *path) > +void probe_utf8_pathname_composition(char *path, int len) > { > static const char *auml_nfc = "\xc3\xa4"; > static const char *auml_nfd = "\x61\xcc\x88"; > - size_t baselen = path->len; > + struct strbuf sbuf; > int output_fd; > if (precomposed_unicode != -1) > return; /* We found it defined in the global config, respect it > */ > - strbuf_addstr(path, auml_nfc); > - output_fd = open(path, O_CREAT|O_EXCL|O_RDWR, 0600); > + strbuf_init(&sbuf, len+3); > + strbuf_add(&sbuf, path, len); > + strbuf_addstr(&sbuf, auml_nfc); > + output_fd = open(sbuf.buf, O_CREAT|O_EXCL|O_RDWR, 0600); > + fprintf(stderr, "%s/%s:%d sbuf.buf=%s\n", > + __FILE__, __FUNCTION__, __LINE__, > sbuf.buf); > if (output_fd >= 0) { > close(output_fd); > - strbuf_setlen(path, baselen); > - strbuf_addstr(path, auml_nfd); > + strbuf_setlen(&sbuf, len); > + strbuf_addstr(&sbuf, auml_nfd); > + fprintf(stderr, "%s/%s:%d sbuf.buf=%s\n", > + __FILE__, __FUNCTION__, __LINE__, > sbuf.buf); > precomposed_unicode = access(path, R_OK) ? 0 : 1; > git_config_set("core.precomposeunicode", precomposed_unicode ? > "true" : "false"); > - strbuf_setlen(path, baselen); > - strbuf_addstr(path, auml_nfc); > + strcpy(path + len, auml_nfc); > if (unlink(path)) > die_errno(_("failed to unlink '%s'"), path); > } > - strbuf_setlen(path, baselen); > + strbuf_release(&sbuf); > } > > > diff --git a/compat/precompose_utf8.h b/compat/precompose_utf8.h > index 7fc7be5..3b73585 100644 > --- a/compat/precompose_utf8.h > +++ b/compat/precompose_utf8.h > @@ -27,7 +27,7 @@ typedef struct { > } PREC_DIR; > > void precompose_argv(int argc, const char **argv); > -void probe_utf8_pathname_composition(struct strbuf *path); > +void probe_utf8_pathname_composition(char *, int); > > PREC_DIR *precompose_utf8_opendir(const char *dirname); > struct dirent_prec_psx *precompose_utf8_readdir(PREC_DIR *dirp); -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 41/68] init: use strbufs to store paths
On 03.10.15 18:54, Junio C Hamano wrote: > Torsten Bögershausen writes: > >> On 30.09.15 02:23, Jeff King wrote: >>> On Tue, Sep 29, 2015 at 04:50:39PM -0700, Michael Blume wrote: >>> I see compile errors on my mac: >> >> This is my attempt, passing the test, but not fully polished. > > Thanks. > >> diff --git a/builtin/init-db.c b/builtin/init-db.c >> index 89f2c05..60b559c 100644 >> --- a/builtin/init-db.c >> +++ b/builtin/init-db.c >> @@ -276,7 +276,9 @@ static int create_default_files(const char >> *template_path) >> path = git_path_buf(&buf, "CoNfIg"); >> if (!access(path, F_OK)) >> git_config_set("core.ignorecase", "true"); >> -probe_utf8_pathname_composition(path); >> +/* Probe utf-8 normalization withou mangling CoNfIG */ >> +path = git_path_buf(&buf, "config"); >> +probe_utf8_pathname_composition(path, strlen(path)); > > Hmph, Peff's quick-fix passed the original "CoNfIg" in &buf directly > to probe_utf8_pathname_composition() without changing its signature. True, ( I was thinking that the test did only work on case insensitive FS). We can skip that change. Beside that, I later realized, that a better signature could be: +void probe_utf8_pathname_composition(const char *path, size_t len) I can send a proper patch the next days. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 41/68] init: use strbufs to store paths
On Sat, Oct 03, 2015 at 11:12:13PM +0200, Torsten Bögershausen wrote: > > Hmph, Peff's quick-fix passed the original "CoNfIg" in &buf directly > > to probe_utf8_pathname_composition() without changing its signature. > True, ( I was thinking that the test did only work on case insensitive FS). > We can skip that change. > > Beside that, I later realized, that a better signature could be: > +void probe_utf8_pathname_composition(const char *path, size_t len) > > I can send a proper patch the next days. That is the original signature, before my sprintf series. I do not mind leaving that as-is, and simply cleaning up probe_utf8_pathname_composition by using a strbuf internally there. Though I have to wonder if it even needs us to pass _anything_ at that point. It could just call git_path_buf("config%s", auml_nfd) itself. The whole reason to pass anything was to let it reuse the buffer the caller had. -Peff -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 41/68] init: use strbufs to store paths
On 2015-10-04 05.37, Jeff King wrote: > On Sat, Oct 03, 2015 at 11:12:13PM +0200, Torsten Bögershausen wrote: > >>> Hmph, Peff's quick-fix passed the original "CoNfIg" in &buf directly >>> to probe_utf8_pathname_composition() without changing its signature. >> True, ( I was thinking that the test did only work on case insensitive FS). >> We can skip that change. >> >> Beside that, I later realized, that a better signature could be: >> +void probe_utf8_pathname_composition(const char *path, size_t len) >> >> I can send a proper patch the next days. > That is the original signature, before my sprintf series. I do not mind > leaving that as-is, and simply cleaning up probe_utf8_pathname_composition > by using a strbuf internally there. Though I have to wonder if it even > needs us to pass _anything_ at that point. It could just call > git_path_buf("config%s", auml_nfd) itself. The whole reason to pass > anything was to let it reuse the buffer the caller had. > > -Peff Makes sense, here is V2: git diff 07690109b6a252ac7cbede diff --git a/builtin/init-db.c b/builtin/init-db.c index 89f2c05..4892579 100644 --- a/builtin/init-db.c +++ b/builtin/init-db.c @@ -276,7 +276,7 @@ static int create_default_files(const char *template_path) path = git_path_buf(&buf, "CoNfIg"); if (!access(path, F_OK)) git_config_set("core.ignorecase", "true"); -probe_utf8_pathname_composition(path); +probe_utf8_pathname_composition(); } strbuf_release(&buf); diff --git a/compat/precompose_utf8.c b/compat/precompose_utf8.c index b4dd3c7..64b85f2 100644 --- a/compat/precompose_utf8.c +++ b/compat/precompose_utf8.c @@ -8,6 +8,7 @@ #include "cache.h" #include "utf8.h" #include "precompose_utf8.h" +#include "strbuf.h" typedef char *iconv_ibp; static const char *repo_encoding = "UTF-8"; @@ -36,28 +37,27 @@ static size_t has_non_ascii(const char *s, size_t maxlen, size_t *strlen_c) } -void probe_utf8_pathname_composition(struct strbuf *path) +void probe_utf8_pathname_composition(void) { +struct strbuf sbuf = STRBUF_INIT; static const char *auml_nfc = "\xc3\xa4"; static const char *auml_nfd = "\x61\xcc\x88"; -size_t baselen = path->len; +const char *path; int output_fd; if (precomposed_unicode != -1) return; /* We found it defined in the global config, respect it */ -strbuf_addstr(path, auml_nfc); +path = git_path_buf(&sbuf, "%s", auml_nfc); output_fd = open(path, O_CREAT|O_EXCL|O_RDWR, 0600); if (output_fd >= 0) { close(output_fd); -strbuf_setlen(path, baselen); -strbuf_addstr(path, auml_nfd); +path = git_path_buf(&sbuf, "%s", auml_nfd); precomposed_unicode = access(path, R_OK) ? 0 : 1; git_config_set("core.precomposeunicode", precomposed_unicode ? "true" : "false"); -strbuf_setlen(path, baselen); -strbuf_addstr(path, auml_nfc); +path = git_path_buf(&sbuf, "%s", auml_nfc); if (unlink(path)) die_errno(_("failed to unlink '%s'"), path); } -strbuf_setlen(path, baselen); +strbuf_release(&sbuf); } diff --git a/compat/precompose_utf8.h b/compat/precompose_utf8.h index 7fc7be5..a94e7c4 100644 --- a/compat/precompose_utf8.h +++ b/compat/precompose_utf8.h @@ -27,7 +27,7 @@ typedef struct { } PREC_DIR; void precompose_argv(int argc, const char **argv); -void probe_utf8_pathname_composition(struct strbuf *path); +void probe_utf8_pathname_composition(void); PREC_DIR *precompose_utf8_opendir(const char *dirname); struct dirent_prec_psx *precompose_utf8_readdir(PREC_DIR *dirp); And this is fix for David: diff --git a/refs.h b/refs.h index f499093..7dee497 100644 --- a/refs.h +++ b/refs.h @@ -670,7 +670,6 @@ typedef int (*ref_transaction_verify_fn)(struct ref_transaction *transaction, unsigned int flags, struct strbuf *err); typedef int (*ref_transaction_commit_fn)(struct ref_transaction *transaction, struct strbuf *err); -typedef void (*ref_transaction_free_fn)(struct ref_transaction *transaction); /* reflog functions */ typedef int (*for_each_reflog_ent_fn)(const char *refname, -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 41/68] init: use strbufs to store paths
On Sun, Oct 04, 2015 at 08:31:31AM +0200, Torsten Bögershausen wrote: > > That is the original signature, before my sprintf series. I do not mind > > leaving that as-is, and simply cleaning up probe_utf8_pathname_composition > > by using a strbuf internally there. Though I have to wonder if it even > > needs us to pass _anything_ at that point. It could just call > > git_path_buf("config%s", auml_nfd) itself. The whole reason to pass > > anything was to let it reuse the buffer the caller had. > > > > -Peff > Makes sense, here is V2: Yeah, I think this is much nicer. And because it decouples the interface between init-db.c and the precompose code, it is easy to do it as a separate patch before the init-db one. > diff --git a/compat/precompose_utf8.c b/compat/precompose_utf8.c > index b4dd3c7..64b85f2 100644 > --- a/compat/precompose_utf8.c > +++ b/compat/precompose_utf8.c > @@ -8,6 +8,7 @@ > #include "cache.h" > #include "utf8.h" > #include "precompose_utf8.h" > +#include "strbuf.h" I think this is actually redundant; it is part of cache.h included above (and the precompose_utf8.h header file does not need to care anymore, since the strbuf is not part of the interface). > -void probe_utf8_pathname_composition(struct strbuf *path) > +void probe_utf8_pathname_composition(void) > { > +struct strbuf sbuf = STRBUF_INIT; > static const char *auml_nfc = "\xc3\xa4"; > static const char *auml_nfd = "\x61\xcc\x88"; > -size_t baselen = path->len; > +const char *path; I don't think we need this separate "path"; we can just access the strbuf directly (that makes the diff a little noisier, but I think the end result is simpler). > diff --git a/compat/precompose_utf8.h b/compat/precompose_utf8.h > index 7fc7be5..a94e7c4 100644 > --- a/compat/precompose_utf8.h > +++ b/compat/precompose_utf8.h > @@ -27,7 +27,7 @@ typedef struct { > } PREC_DIR; > > void precompose_argv(int argc, const char **argv); > -void probe_utf8_pathname_composition(struct strbuf *path); > +void probe_utf8_pathname_composition(void); I think we need a similar fix for the compat macro to build on non-Mac platforms. Here's a mini-series I came up with, which I hope is polished enough for Junio to apply as a drop-in replacement for the "init: use strbufs" patch from my original series. I compiled-tested it on Linux, with and without precompose_utf8.o support hacked in. I don't have access to an OS X machine to test on, so I'd appreciate confirmation that t3910 still passes there. [1/3]: precompose_utf8: drop unused variable [2/3]: probe_utf8_pathname_composition: use internal strbuf [3/3]: init: use strbufs to store paths -Peff -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html