subject:"\[PATCH 41\/68\] init\: use strbufs to store paths"

Re: [PATCH 41/68] init: use strbufs to store paths

2015-10-04 Thread Jeff King

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

Re: [PATCH 41/68] init: use strbufs to store paths

2015-10-04 Thread Torsten Bögershausen



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  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(, "CoNfIg");
 if (!access(path, F_OK))
 git_config_set("core.ignorecase", "true");
-probe_utf8_pathname_composition(path);
+probe_utf8_pathname_composition();
 }
 
 strbuf_release();
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(, "%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(, "%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(, "%s", auml_nfc);
 if (unlink(path))
 die_errno(_("failed to unlink '%s'"), path);
 }
-strbuf_setlen(path, baselen);
+strbuf_release();
 }
 
 
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

2015-10-03 Thread Torsten Bögershausen

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(, "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(, "config");
>> +probe_utf8_pathname_composition(path, strlen(path));
> 
> Hmph, Peff's quick-fix passed the original "CoNfIg" in  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

2015-10-03 Thread Junio C Hamano

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(, "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(, "config");
> + probe_utf8_pathname_composition(path, strlen(path));

Hmph, Peff's quick-fix passed the original "CoNfIg" in  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(, len+3);
> + strbuf_add(, path, len);
> + strbuf_addstr(, 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(, len);
> + strbuf_addstr(, 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();
>  }
>  
>  
> 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

2015-10-03 Thread Jeff King

On Sat, Oct 03, 2015 at 11:12:13PM +0200, Torsten Bögershausen wrote:

> > Hmph, Peff's quick-fix passed the original "CoNfIg" in  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

2015-10-02 Thread Torsten Bögershausen


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

2015-10-02 Thread Jeff King

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

2015-10-02 Thread Torsten Bögershausen

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(, "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(, "config");
+   probe_utf8_pathname_composition(path, strlen(path));
}
 
strbuf_release();
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(, len+3);
+   strbuf_add(, path, len);
+   strbuf_addstr(, 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(, len);
+   strbuf_addstr(, 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();
 }
 
 
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

2015-09-30 Thread Junio C Hamano

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

2015-09-30 Thread Jeff King

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

2015-09-29 Thread Michael Blume

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, _git)) {
> +   strbuf_addstr(path, de->d_name);
> +   strbuf_addstr(template, de->d_name);
> +   if (lstat(path->buf, _git)) {
> if (errno != ENOENT)
> -   die_errno(_("cannot stat '%s'"), path);
> +   die_errno(_("cannot stat '%s'"), path->buf);
> }
> else
> exists = 1;
>
> -   if (lstat(template, _template))
> -   die_errno(_("cannot stat template '%s'"), template);
> +   if (lstat(template->buf, _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

2015-09-29 Thread Jeff King

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(, "CoNfIg");
if (!access(path, F_OK))
git_config_set("core.ignorecase", "true");
-   probe_utf8_pathname_composition(path);
+   probe_utf8_pathname_composition();
}
 
strbuf_release();
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

[PATCH 41/68] init: use strbufs to store paths

2015-09-24 Thread Jeff King

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, _git)) {
+   strbuf_addstr(path, de->d_name);
+   strbuf_addstr(template, de->d_name);
+   if (lstat(path->buf, _git)) {
if (errno != ENOENT)
-   die_errno(_("cannot stat '%s'"), path);
+   die_errno(_("cannot stat '%s'"), path->buf);
}
else
exists = 1;
 
-   if (lstat(template, _template))
-   die_errno(_("cannot stat template '%s'"), template);
+   if (lstat(template->buf, _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

Re: [PATCH 41/68] init: use strbufs to store paths

Re: [PATCH 41/68] init: use strbufs to store paths

Re: [PATCH 41/68] init: use strbufs to store paths

Re: [PATCH 41/68] init: use strbufs to store paths

Re: [PATCH 41/68] init: use strbufs to store paths

Re: [PATCH 41/68] init: use strbufs to store paths

Re: [PATCH 41/68] init: use strbufs to store paths

Re: [PATCH 41/68] init: use strbufs to store paths

Re: [PATCH 41/68] init: use strbufs to store paths

Re: [PATCH 41/68] init: use strbufs to store paths

Re: [PATCH 41/68] init: use strbufs to store paths

[PATCH 41/68] init: use strbufs to store paths

13 matches

Site Navigation

Mail list logo

Footer information