[PATCH 1/2] symlinks: remove PATH_MAX limitation

[PATCH 1/2] symlinks: remove PATH_MAX limitation

[Karsten Blees]

'git checkout' fails if a directory is longer than PATH_MAX, because the
lstat_cache in symlinks.c checks if the leading directory exists using
PATH_MAX-bounded string operations.

Remove the limitation by using strbuf instead.

Signed-off-by: Karsten Blees <blees@dcon.de>
---

This fixes a bug on Windows with long paths [1].

[1] https://github.com/msysgit/msysgit/issues/227

 cache.h         |  8 ++++++--
 preload-index.c |  4 ++--
 symlinks.c      | 63 +++++++++++++++++++++++++--------------------------------
 3 files changed, 36 insertions(+), 39 deletions(-)

diff --git a/cache.h b/cache.h
index df65231..44aa439 100644
--- a/cache.h
+++ b/cache.h
@@ -1090,12 +1090,16 @@ struct checkout {
 extern int checkout_entry(struct cache_entry *ce, const struct checkout *state, char *topath);
 
 struct cache_def {
-	char path[PATH_MAX + 1];
-	int len;
+	struct strbuf path;
 	int flags;
 	int track_flags;
 	int prefix_len_stat_func;
 };
+#define CACHE_DEF_INIT { STRBUF_INIT, 0, 0, 0 }
+static inline void cache_def_free(struct cache_def *cache)
+{
+	strbuf_release(&cache->path);
+}
 
 extern int has_symlink_leading_path(const char *name, int len);
 extern int threaded_has_symlink_leading_path(struct cache_def *, const char *, int);
diff --git a/preload-index.c b/preload-index.c
index 968ee25..79ce8a9 100644
--- a/preload-index.c
+++ b/preload-index.c
@@ -37,9 +37,8 @@ static void *preload_thread(void *_data)
 	struct thread_data *p = _data;
 	struct index_state *index = p->index;
 	struct cache_entry **cep = index->cache + p->offset;
-	struct cache_def cache;
+	struct cache_def cache = CACHE_DEF_INIT;
 
-	memset(&cache, 0, sizeof(cache));
 	nr = p->nr;
 	if (nr + p->offset > index->cache_nr)
 		nr = index->cache_nr - p->offset;
@@ -64,6 +63,7 @@ static void *preload_thread(void *_data)
 			continue;
 		ce_mark_uptodate(ce);
 	} while (--nr > 0);
+	cache_def_free(&cache);
 	return NULL;
 }
 
diff --git a/symlinks.c b/symlinks.c
index c2b41a8..5261e8c 100644
--- a/symlinks.c
+++ b/symlinks.c
@@ -35,12 +35,11 @@ static int longest_path_match(const char *name_a, int len_a,
 	return match_len;
 }
 
-static struct cache_def default_cache;
+static struct cache_def default_cache = CACHE_DEF_INIT;
 
 static inline void reset_lstat_cache(struct cache_def *cache)
 {
-	cache->path[0] = '\0';
-	cache->len = 0;
+	strbuf_reset(&cache->path);
 	cache->flags = 0;
 	/*
 	 * The track_flags and prefix_len_stat_func members is only
@@ -73,7 +72,7 @@ static int lstat_cache_matchlen(struct cache_def *cache,
 				int prefix_len_stat_func)
 {
 	int match_len, last_slash, last_slash_dir, previous_slash;
-	int save_flags, max_len, ret;
+	int save_flags, ret;
 	struct stat st;
 
 	if (cache->track_flags != track_flags ||
@@ -93,14 +92,14 @@ static int lstat_cache_matchlen(struct cache_def *cache,
 		 * the 2 "excluding" path types.
 		 */
 		match_len = last_slash =
-			longest_path_match(name, len, cache->path, cache->len,
-					   &previous_slash);
+			longest_path_match(name, len, cache->path.buf,
+					   cache->path.len, &previous_slash);
 		*ret_flags = cache->flags & track_flags & (FL_NOENT|FL_SYMLINK);
 
 		if (!(track_flags & FL_FULLPATH) && match_len == len)
 			match_len = last_slash = previous_slash;
 
-		if (*ret_flags && match_len == cache->len)
+		if (*ret_flags && match_len == cache->path.len)
 			return match_len;
 		/*
 		 * If we now have match_len > 0, we would know that
@@ -121,21 +120,22 @@ static int lstat_cache_matchlen(struct cache_def *cache,
 	 */
 	*ret_flags = FL_DIR;
 	last_slash_dir = last_slash;
-	max_len = len < PATH_MAX ? len : PATH_MAX;
-	while (match_len < max_len) {
+	if (len > cache->path.len)
+		strbuf_grow(&cache->path, len - cache->path.len);
+	while (match_len < len) {
 		do {
-			cache->path[match_len] = name[match_len];
+			cache->path.buf[match_len] = name[match_len];
 			match_len++;
-		} while (match_len < max_len && name[match_len] != '/');
-		if (match_len >= max_len && !(track_flags & FL_FULLPATH))
+		} while (match_len < len && name[match_len] != '/');
+		if (match_len >= len && !(track_flags & FL_FULLPATH))
 			break;
 		last_slash = match_len;
-		cache->path[last_slash] = '\0';
+		cache->path.buf[last_slash] = '\0';
 
 		if (last_slash <= prefix_len_stat_func)
-			ret = stat(cache->path, &st);
+			ret = stat(cache->path.buf, &st);
 		else
-			ret = lstat(cache->path, &st);
+			ret = lstat(cache->path.buf, &st);
 
 		if (ret) {
 			*ret_flags = FL_LSTATERR;
@@ -158,12 +158,11 @@ static int lstat_cache_matchlen(struct cache_def *cache,
 	 * for the moment!
 	 */
 	save_flags = *ret_flags & track_flags & (FL_NOENT|FL_SYMLINK);
-	if (save_flags && last_slash > 0 && last_slash <= PATH_MAX) {
-		cache->path[last_slash] = '\0';
-		cache->len = last_slash;
+	if (save_flags && last_slash > 0) {
+		cache->path.buf[last_slash] = '\0';
+		cache->path.len = last_slash;
 		cache->flags = save_flags;
-	} else if ((track_flags & FL_DIR) &&
-		   last_slash_dir > 0 && last_slash_dir <= PATH_MAX) {
+	} else if ((track_flags & FL_DIR) && last_slash_dir > 0) {
 		/*
 		 * We have a separate test for the directory case,
 		 * since it could be that we have found a symlink or a
@@ -175,8 +174,8 @@ static int lstat_cache_matchlen(struct cache_def *cache,
 		 * can still cache the path components before the last
 		 * one (the found symlink or non-existing component).
 		 */
-		cache->path[last_slash_dir] = '\0';
-		cache->len = last_slash_dir;
+		cache->path.buf[last_slash_dir] = '\0';
+		cache->path.len = last_slash_dir;
 		cache->flags = FL_DIR;
 	} else {
 		reset_lstat_cache(cache);
@@ -273,21 +272,18 @@ static int threaded_has_dirs_only_path(struct cache_def *cache, const char *name
 		FL_DIR;
 }
 
-static struct removal_def {
-	char path[PATH_MAX];
-	int len;
-} removal;
+static struct strbuf removal = STRBUF_INIT;
 
 static void do_remove_scheduled_dirs(int new_len)
 {
 	while (removal.len > new_len) {
-		removal.path[removal.len] = '\0';
-		if (rmdir(removal.path))
+		removal.buf[removal.len] = '\0';
+		if (rmdir(removal.buf))
 			break;
 		do {
 			removal.len--;
 		} while (removal.len > new_len &&
-			 removal.path[removal.len] != '/');
+			 removal.buf[removal.len] != '/');
 	}
 	removal.len = new_len;
 }
@@ -297,7 +293,7 @@ void schedule_dir_for_removal(const char *name, int len)
 	int match_len, last_slash, i, previous_slash;
 
 	match_len = last_slash = i =
-		longest_path_match(name, len, removal.path, removal.len,
+		longest_path_match(name, len, removal.buf, removal.len,
 				   &previous_slash);
 	/* Find last slash inside 'name' */
 	while (i < len) {
@@ -317,11 +313,8 @@ void schedule_dir_for_removal(const char *name, int len)
 	 * If we go deeper down the directory tree, we only need to
 	 * save the new path components as we go down.
 	 */
-	if (match_len < last_slash) {
-		memcpy(&removal.path[match_len], &name[match_len],
-		       last_slash - match_len);
-		removal.len = last_slash;
-	}
+	if (match_len < last_slash)
+		strbuf_add(&removal, &name[match_len], last_slash - match_len);
 }
 
 void remove_scheduled_dirs(void)
-- 
1.9.4.msysgit.0.5.g1471ac1

-- 
-- 
*** Please reply-to-all at all times ***
*** (do not pretend to know who is subscribed and who is not) ***
*** Please avoid top-posting. ***
The msysGit Wiki is here: https://github.com/msysgit/msysgit/wiki - Github accounts are free.

You received this message because you are subscribed to the Google
Groups "msysGit" group.
To post to this group, send email to msysgit@googlegroups.com
To unsubscribe from this group, send email to
msysgit+unsubscribe@googlegroups.com
For more options, and view previous threads, visit this group at
http://groups.google.com/group/msysgit?hl=en_US?hl=en

--- 
You received this message because you are subscribed to the Google Groups "msysGit" group.
To unsubscribe from this group and stop receiving emails from it, send an email to msysgit+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[PATCH 2/2] dir: remove PATH_MAX limitation

[Karsten Blees]

'git status' segfaults if a directory is longer than PATH_MAX, because
processing .gitignore files in prep_exclude() writes past the end of a
PATH_MAX-bounded buffer.

Remove the limitation by using strbuf instead.

Note: this fix just 'abuses' strbuf as string allocator, len is always 0.
prep_exclude() can probably be simplified using more strbuf APIs.

Signed-off-by: Karsten Blees <blees@dcon.de>
---
 dir.c | 35 +++++++++++++++++++----------------
 dir.h |  4 ++--
 2 files changed, 21 insertions(+), 18 deletions(-)

diff --git a/dir.c b/dir.c
index e65888d..8d4d83c 100644
--- a/dir.c
+++ b/dir.c
@@ -798,7 +798,7 @@ static void prep_exclude(struct dir_struct *dir, const char *base, int baselen)
 	 * path being checked. */
 	while ((stk = dir->exclude_stack) != NULL) {
 		if (stk->baselen <= baselen &&
-		    !strncmp(dir->basebuf, base, stk->baselen))
+		    !strncmp(dir->base.buf, base, stk->baselen))
 			break;
 		el = &group->el[dir->exclude_stack->exclude_ix];
 		dir->exclude_stack = stk->prev;
@@ -833,48 +833,50 @@ static void prep_exclude(struct dir_struct *dir, const char *base, int baselen)
 		stk->baselen = cp - base;
 		stk->exclude_ix = group->nr;
 		el = add_exclude_list(dir, EXC_DIRS, NULL);
-		memcpy(dir->basebuf + current, base + current,
+		strbuf_grow(&dir->base, stk->baselen);
+		memcpy(dir->base.buf + current, base + current,
 		       stk->baselen - current);
 
 		/* Abort if the directory is excluded */
 		if (stk->baselen) {
 			int dt = DT_DIR;
-			dir->basebuf[stk->baselen - 1] = 0;
+			dir->base.buf[stk->baselen - 1] = 0;
 			dir->exclude = last_exclude_matching_from_lists(dir,
-				dir->basebuf, stk->baselen - 1,
-				dir->basebuf + current, &dt);
-			dir->basebuf[stk->baselen - 1] = '/';
+				dir->base.buf, stk->baselen - 1,
+				dir->base.buf + current, &dt);
+			dir->base.buf[stk->baselen - 1] = '/';
 			if (dir->exclude &&
 			    dir->exclude->flags & EXC_FLAG_NEGATIVE)
 				dir->exclude = NULL;
 			if (dir->exclude) {
-				dir->basebuf[stk->baselen] = 0;
+				dir->base.buf[stk->baselen] = 0;
 				dir->exclude_stack = stk;
 				return;
 			}
 		}
 
-		/* Try to read per-directory file unless path is too long */
-		if (dir->exclude_per_dir &&
-		    stk->baselen + strlen(dir->exclude_per_dir) < PATH_MAX) {
-			strcpy(dir->basebuf + stk->baselen,
+		/* Try to read per-directory file */
+		if (dir->exclude_per_dir) {
+			strbuf_grow(&dir->base, stk->baselen +
+				    strlen(dir->exclude_per_dir));
+			strcpy(dir->base.buf + stk->baselen,
 					dir->exclude_per_dir);
 			/*
-			 * dir->basebuf gets reused by the traversal, but we
+			 * dir->base gets reused by the traversal, but we
 			 * need fname to remain unchanged to ensure the src
 			 * member of each struct exclude correctly
 			 * back-references its source file.  Other invocations
 			 * of add_exclude_list provide stable strings, so we
 			 * strdup() and free() here in the caller.
 			 */
-			el->src = strdup(dir->basebuf);
-			add_excludes_from_file_to_list(dir->basebuf,
-					dir->basebuf, stk->baselen, el, 1);
+			el->src = strdup(dir->base.buf);
+			add_excludes_from_file_to_list(dir->base.buf,
+					dir->base.buf, stk->baselen, el, 1);
 		}
 		dir->exclude_stack = stk;
 		current = stk->baselen;
 	}
-	dir->basebuf[baselen] = '\0';
+	dir->base.buf[baselen] = '\0';
 }
 
 /*
@@ -1671,4 +1673,5 @@ void clear_directory(struct dir_struct *dir)
 		free(stk);
 		stk = prev;
 	}
+	strbuf_release(&dir->base);
 }
diff --git a/dir.h b/dir.h
index 55e5345..e870fb6 100644
--- a/dir.h
+++ b/dir.h
@@ -111,13 +111,13 @@ struct dir_struct {
 	 * per-directory exclude lists.
 	 *
 	 * exclude_stack points to the top of the exclude_stack, and
-	 * basebuf contains the full path to the current
+	 * base contains the full path to the current
 	 * (sub)directory in the traversal. Exclude points to the
 	 * matching exclude struct if the directory is excluded.
 	 */
 	struct exclude_stack *exclude_stack;
 	struct exclude *exclude;
-	char basebuf[PATH_MAX];
+	struct strbuf base;
 };
 
 /*
-- 
1.9.4.msysgit.0.5.g1471ac1

-- 
-- 
*** Please reply-to-all at all times ***
*** (do not pretend to know who is subscribed and who is not) ***
*** Please avoid top-posting. ***
The msysGit Wiki is here: https://github.com/msysgit/msysgit/wiki - Github accounts are free.

You received this message because you are subscribed to the Google
Groups "msysGit" group.
To post to this group, send email to msysgit@googlegroups.com
To unsubscribe from this group, send email to
msysgit+unsubscribe@googlegroups.com
For more options, and view previous threads, visit this group at
http://groups.google.com/group/msysgit?hl=en_US?hl=en

--- 
You received this message because you are subscribed to the Google Groups "msysGit" group.
To unsubscribe from this group and stop receiving emails from it, send an email to msysgit+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

Re: [PATCH 2/2] dir: remove PATH_MAX limitation

[Duy Nguyen]

On Sat, Jul 5, 2014 at 5:42 AM, Karsten Blees <karsten.blees@gmail.com> wrote:
> 'git status' segfaults if a directory is longer than PATH_MAX, because
> processing .gitignore files in prep_exclude() writes past the end of a
> PATH_MAX-bounded buffer.
>
> Remove the limitation by using strbuf instead.
>
> Note: this fix just 'abuses' strbuf as string allocator, len is always 0.
> prep_exclude() can probably be simplified using more strbuf APIs.

FYI I had a similar patch [1] that attempted to lazily strbuf_init()
instead so that strbuf_ API could be used.

[1] http://article.gmane.org/gmane.comp.version-control.git/248310
-- 
Duy

-- 
-- 
*** Please reply-to-all at all times ***
*** (do not pretend to know who is subscribed and who is not) ***
*** Please avoid top-posting. ***
The msysGit Wiki is here: https://github.com/msysgit/msysgit/wiki - Github accounts are free.

You received this message because you are subscribed to the Google
Groups "msysGit" group.
To post to this group, send email to msysgit@googlegroups.com
To unsubscribe from this group, send email to
msysgit+unsubscribe@googlegroups.com
For more options, and view previous threads, visit this group at
http://groups.google.com/group/msysgit?hl=en_US?hl=en

--- 
You received this message because you are subscribed to the Google Groups "msysGit" group.
To unsubscribe from this group and stop receiving emails from it, send an email to msysgit+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

Re: [PATCH 2/2] dir: remove PATH_MAX limitation

[Karsten Blees]

Am 05.07.2014 12:48, schrieb Duy Nguyen:
> On Sat, Jul 5, 2014 at 5:42 AM, Karsten Blees <karsten.blees@gmail.com> wrote:
>> 'git status' segfaults if a directory is longer than PATH_MAX, because
>> processing .gitignore files in prep_exclude() writes past the end of a
>> PATH_MAX-bounded buffer.
>>
>> Remove the limitation by using strbuf instead.
>>
>> Note: this fix just 'abuses' strbuf as string allocator, len is always 0.
>> prep_exclude() can probably be simplified using more strbuf APIs.
> 
> FYI I had a similar patch [1] that attempted to lazily strbuf_init()
> instead so that strbuf_ API could be used.
> 
> [1] http://article.gmane.org/gmane.comp.version-control.git/248310
> 

Sorry, I missed that one.

In my version, strbuf_grow() does the lazy initialization (in fact, many
strbuf_* functions can handle memset(0) strbufs just fine).

I was simply too lazy to understand (again) how prep_exclude works exactly, and
as string calculations like that have the tendency to be just 1 char off, I went
for the obviously correct solution (i.e. s/dir->basebuf/dir->base.buf/ plus
strbuf_grow() before we write the buffer).

But IMO your version is much cleaner already.

However, api-strbuf.txt says that buf != NULL is invariant after init, and
alloc is "somehow private" :-) , so perhaps you should

-	if (!dir->basebuf.alloc)
+	if (!dir->basebuf.buf)
		strbuf_init(&dir->basebuf, PATH_MAX);

-- 
-- 
*** Please reply-to-all at all times ***
*** (do not pretend to know who is subscribed and who is not) ***
*** Please avoid top-posting. ***
The msysGit Wiki is here: https://github.com/msysgit/msysgit/wiki - Github accounts are free.

You received this message because you are subscribed to the Google
Groups "msysGit" group.
To post to this group, send email to msysgit@googlegroups.com
To unsubscribe from this group, send email to
msysgit+unsubscribe@googlegroups.com
For more options, and view previous threads, visit this group at
http://groups.google.com/group/msysgit?hl=en_US?hl=en

--- 
You received this message because you are subscribed to the Google Groups "msysGit" group.
To unsubscribe from this group and stop receiving emails from it, send an email to msysgit+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

Re: [PATCH 2/2] dir: remove PATH_MAX limitation

[Jeff King]

On Sat, Jul 05, 2014 at 12:42:29AM +0200, Karsten Blees wrote:

> Note: this fix just 'abuses' strbuf as string allocator, len is always 0.
> prep_exclude() can probably be simplified using more strbuf APIs.

Hrm. It looks like you grow it and add some data, but really don't want
the length to expand (because the caller depends on it).

In other directory-traversal code we follow a pattern like:

  size_t prefix_len = dir->base.len;

  strbuf_add(&dir->base, cp, stk->baselen - current);
  /* use full path in dir->base, then "pop" */
  strbuf_setlen(&dir->base, stk->baselen);

That makes it a little more obvious that the memcpy matches the
strbuf_grow (because it all happens inside strbuf_add).

Is it possible to do something like that here?

-Peff

Re: [PATCH 2/2] dir: remove PATH_MAX limitation

[Junio C Hamano]

Karsten Blees <karsten.blees@gmail.com> writes:

> 'git status' segfaults if a directory is longer than PATH_MAX, because
> processing .gitignore files in prep_exclude() writes past the end of a
> PATH_MAX-bounded buffer.
>
> Remove the limitation by using strbuf instead.
>
> Note: this fix just 'abuses' strbuf as string allocator, len is always 0.
> prep_exclude() can probably be simplified using more strbuf APIs.

In addition to what Jeff already said, I am afraid that this may
make things a lot worse for normal case.  By sizing the strbuf to
absolute minimum as you dig deeper at each level, wouldn't you copy
and reallocate the buffer a lot more, compared to the case where
everything fits in the original buffer?


> Signed-off-by: Karsten Blees <blees@dcon.de>
> ---
>  dir.c | 35 +++++++++++++++++++----------------
>  dir.h |  4 ++--
>  2 files changed, 21 insertions(+), 18 deletions(-)
>
> diff --git a/dir.c b/dir.c
> index e65888d..8d4d83c 100644
> --- a/dir.c
> +++ b/dir.c
> @@ -798,7 +798,7 @@ static void prep_exclude(struct dir_struct *dir, const char *base, int baselen)
>  	 * path being checked. */
>  	while ((stk = dir->exclude_stack) != NULL) {
>  		if (stk->baselen <= baselen &&
> -		    !strncmp(dir->basebuf, base, stk->baselen))
> +		    !strncmp(dir->base.buf, base, stk->baselen))
>  			break;
>  		el = &group->el[dir->exclude_stack->exclude_ix];
>  		dir->exclude_stack = stk->prev;
> @@ -833,48 +833,50 @@ static void prep_exclude(struct dir_struct *dir, const char *base, int baselen)
>  		stk->baselen = cp - base;
>  		stk->exclude_ix = group->nr;
>  		el = add_exclude_list(dir, EXC_DIRS, NULL);
> -		memcpy(dir->basebuf + current, base + current,
> +		strbuf_grow(&dir->base, stk->baselen);
> +		memcpy(dir->base.buf + current, base + current,
>  		       stk->baselen - current);
>  
>  		/* Abort if the directory is excluded */
>  		if (stk->baselen) {
>  			int dt = DT_DIR;
> -			dir->basebuf[stk->baselen - 1] = 0;
> +			dir->base.buf[stk->baselen - 1] = 0;
>  			dir->exclude = last_exclude_matching_from_lists(dir,
> -				dir->basebuf, stk->baselen - 1,
> -				dir->basebuf + current, &dt);
> -			dir->basebuf[stk->baselen - 1] = '/';
> +				dir->base.buf, stk->baselen - 1,
> +				dir->base.buf + current, &dt);
> +			dir->base.buf[stk->baselen - 1] = '/';
>  			if (dir->exclude &&
>  			    dir->exclude->flags & EXC_FLAG_NEGATIVE)
>  				dir->exclude = NULL;
>  			if (dir->exclude) {
> -				dir->basebuf[stk->baselen] = 0;
> +				dir->base.buf[stk->baselen] = 0;
>  				dir->exclude_stack = stk;
>  				return;
>  			}
>  		}
>  
> -		/* Try to read per-directory file unless path is too long */
> -		if (dir->exclude_per_dir &&
> -		    stk->baselen + strlen(dir->exclude_per_dir) < PATH_MAX) {
> -			strcpy(dir->basebuf + stk->baselen,
> +		/* Try to read per-directory file */
> +		if (dir->exclude_per_dir) {
> +			strbuf_grow(&dir->base, stk->baselen +
> +				    strlen(dir->exclude_per_dir));
> +			strcpy(dir->base.buf + stk->baselen,
>  					dir->exclude_per_dir);
>  			/*
> -			 * dir->basebuf gets reused by the traversal, but we
> +			 * dir->base gets reused by the traversal, but we
>  			 * need fname to remain unchanged to ensure the src
>  			 * member of each struct exclude correctly
>  			 * back-references its source file.  Other invocations
>  			 * of add_exclude_list provide stable strings, so we
>  			 * strdup() and free() here in the caller.
>  			 */
> -			el->src = strdup(dir->basebuf);
> -			add_excludes_from_file_to_list(dir->basebuf,
> -					dir->basebuf, stk->baselen, el, 1);
> +			el->src = strdup(dir->base.buf);
> +			add_excludes_from_file_to_list(dir->base.buf,
> +					dir->base.buf, stk->baselen, el, 1);
>  		}
>  		dir->exclude_stack = stk;
>  		current = stk->baselen;
>  	}
> -	dir->basebuf[baselen] = '\0';
> +	dir->base.buf[baselen] = '\0';
>  }
>  
>  /*
> @@ -1671,4 +1673,5 @@ void clear_directory(struct dir_struct *dir)
>  		free(stk);
>  		stk = prev;
>  	}
> +	strbuf_release(&dir->base);
>  }
> diff --git a/dir.h b/dir.h
> index 55e5345..e870fb6 100644
> --- a/dir.h
> +++ b/dir.h
> @@ -111,13 +111,13 @@ struct dir_struct {
>  	 * per-directory exclude lists.
>  	 *
>  	 * exclude_stack points to the top of the exclude_stack, and
> -	 * basebuf contains the full path to the current
> +	 * base contains the full path to the current
>  	 * (sub)directory in the traversal. Exclude points to the
>  	 * matching exclude struct if the directory is excluded.
>  	 */
>  	struct exclude_stack *exclude_stack;
>  	struct exclude *exclude;
> -	char basebuf[PATH_MAX];
> +	struct strbuf base;
>  };
>  
>  /*
> -- 
> 1.9.4.msysgit.0.5.g1471ac1
>
> -- 

Re: [PATCH 2/2] dir: remove PATH_MAX limitation

[Karsten Blees]

Am 09.07.2014 18:33, schrieb Junio C Hamano:
> Karsten Blees <karsten.blees@gmail.com> writes:
> 
>> 'git status' segfaults if a directory is longer than PATH_MAX, because
>> processing .gitignore files in prep_exclude() writes past the end of a
>> PATH_MAX-bounded buffer.
>>
>> Remove the limitation by using strbuf instead.
>>
>> Note: this fix just 'abuses' strbuf as string allocator, len is always 0.
>> prep_exclude() can probably be simplified using more strbuf APIs.
> 
> In addition to what Jeff already said, I am afraid that this may
> make things a lot worse for normal case.  By sizing the strbuf to
> absolute minimum as you dig deeper at each level, wouldn't you copy
> and reallocate the buffer a lot more, compared to the case where
> everything fits in the original buffer?
> 

strbuf uses ALLOC_GROW, which resizes in (x+16)*1.5 steps (i.e. 24, 60, 114,
195, 316...). Max path len in git is 85, and linux and WebKit are 170ish. I
don't think three or four extra reallocs per directory traversal will be
noticeable.

Anyways, I'd like to kindly withdraw this patch in favor of Duy's version.

http://article.gmane.org/gmane.comp.version-control.git/248310

-- 
-- 
*** Please reply-to-all at all times ***
*** (do not pretend to know who is subscribed and who is not) ***
*** Please avoid top-posting. ***
The msysGit Wiki is here: https://github.com/msysgit/msysgit/wiki - Github accounts are free.

You received this message because you are subscribed to the Google
Groups "msysGit" group.
To post to this group, send email to msysgit@googlegroups.com
To unsubscribe from this group, send email to
msysgit+unsubscribe@googlegroups.com
For more options, and view previous threads, visit this group at
http://groups.google.com/group/msysgit?hl=en_US?hl=en

--- 
You received this message because you are subscribed to the Google Groups "msysGit" group.
To unsubscribe from this group and stop receiving emails from it, send an email to msysgit+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

Re: [PATCH 2/2] dir: remove PATH_MAX limitation

[Junio C Hamano]

Karsten Blees <karsten.blees@gmail.com> writes:

> Anyways, I'd like to kindly withdraw this patch in favor of Duy's version.
>
> http://article.gmane.org/gmane.comp.version-control.git/248310

Thanks; I've already reverted it from 'next'.

Is Duy's patch still viable?

-- 
-- 
*** Please reply-to-all at all times ***
*** (do not pretend to know who is subscribed and who is not) ***
*** Please avoid top-posting. ***
The msysGit Wiki is here: https://github.com/msysgit/msysgit/wiki - Github accounts are free.

You received this message because you are subscribed to the Google
Groups "msysGit" group.
To post to this group, send email to msysgit@googlegroups.com
To unsubscribe from this group, send email to
msysgit+unsubscribe@googlegroups.com
For more options, and view previous threads, visit this group at
http://groups.google.com/group/msysgit?hl=en_US?hl=en

--- 
You received this message because you are subscribed to the Google Groups "msysGit" group.
To unsubscribe from this group and stop receiving emails from it, send an email to msysgit+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

Re: [PATCH 2/2] dir: remove PATH_MAX limitation

[Karsten Blees]

Am 12.07.2014 00:29, schrieb Junio C Hamano:
> Karsten Blees <karsten.blees@gmail.com> writes:
> 
>> Anyways, I'd like to kindly withdraw this patch in favor of Duy's version.
>>
>> http://article.gmane.org/gmane.comp.version-control.git/248310
> 
> Thanks; I've already reverted it from 'next'.
> 
> Is Duy's patch still viable?
> 

I think so. It fixes the segfault with long paths on Windows as well
(Tested-by: <me>), uses strbuf APIs as Peff suggested, and initializes the
strbuf with PATH_MAX (i.e. no reallocs in the common case either ;-) ).

AFAICT the first two patches of that series are also completely unrelated
to the untracked-cache, so we may want to fast-track these?

[01/20] dir.c: coding style fix
[02/20] dir.h: move struct exclude declaration to top level
[03/20] prep_exclude: remove the artificial PATH_MAX limit

...perhaps with s/if (!dir->basebuf.alloc)/if (!dir->basebuf.buf)/

@Duy any reason for not signing off that series?

-- 
-- 
*** Please reply-to-all at all times ***
*** (do not pretend to know who is subscribed and who is not) ***
*** Please avoid top-posting. ***
The msysGit Wiki is here: https://github.com/msysgit/msysgit/wiki - Github accounts are free.

You received this message because you are subscribed to the Google
Groups "msysGit" group.
To post to this group, send email to msysgit@googlegroups.com
To unsubscribe from this group, send email to
msysgit+unsubscribe@googlegroups.com
For more options, and view previous threads, visit this group at
http://groups.google.com/group/msysgit?hl=en_US?hl=en

--- 
You received this message because you are subscribed to the Google Groups "msysGit" group.
To unsubscribe from this group and stop receiving emails from it, send an email to msysgit+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

Re: [PATCH 2/2] dir: remove PATH_MAX limitation

[Duy Nguyen]

On Sat, Jul 12, 2014 at 6:43 AM, Karsten Blees <karsten.blees@gmail.com> wrote:
> Am 12.07.2014 00:29, schrieb Junio C Hamano:
>> Karsten Blees <karsten.blees@gmail.com> writes:
>>
>>> Anyways, I'd like to kindly withdraw this patch in favor of Duy's version.
>>>
>>> http://article.gmane.org/gmane.comp.version-control.git/248310
>>
>> Thanks; I've already reverted it from 'next'.
>>
>> Is Duy's patch still viable?
>>
>
> I think so. It fixes the segfault with long paths on Windows as well
> (Tested-by: <me>), uses strbuf APIs as Peff suggested, and initializes the
> strbuf with PATH_MAX (i.e. no reallocs in the common case either ;-) ).
>
> AFAICT the first two patches of that series are also completely unrelated
> to the untracked-cache, so we may want to fast-track these?
>
> [01/20] dir.c: coding style fix
> [02/20] dir.h: move struct exclude declaration to top level
> [03/20] prep_exclude: remove the artificial PATH_MAX limit
>
> ...perhaps with s/if (!dir->basebuf.alloc)/if (!dir->basebuf.buf)/
>
> @Duy any reason for not signing off that series?

That series still need a lot more work, but for those first three, if
you want to fast track, you have my sign-offs.
-- 
Duy

-- 
-- 
*** Please reply-to-all at all times ***
*** (do not pretend to know who is subscribed and who is not) ***
*** Please avoid top-posting. ***
The msysGit Wiki is here: https://github.com/msysgit/msysgit/wiki - Github accounts are free.

You received this message because you are subscribed to the Google
Groups "msysGit" group.
To post to this group, send email to msysgit@googlegroups.com
To unsubscribe from this group, send email to
msysgit+unsubscribe@googlegroups.com
For more options, and view previous threads, visit this group at
http://groups.google.com/group/msysgit?hl=en_US?hl=en

--- 
You received this message because you are subscribed to the Google Groups "msysGit" group.
To unsubscribe from this group and stop receiving emails from it, send an email to msysgit+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

Re: [PATCH 2/2] dir: remove PATH_MAX limitation

[Junio C Hamano]

Duy Nguyen <pclouds@gmail.com> writes:

>> [01/20] dir.c: coding style fix
>> [02/20] dir.h: move struct exclude declaration to top level
>> [03/20] prep_exclude: remove the artificial PATH_MAX limit
>>
>> ...perhaps with s/if (!dir->basebuf.alloc)/if (!dir->basebuf.buf)/
>>
>> @Duy any reason for not signing off that series?
>
> That series still need a lot more work, but for those first three, if
> you want to fast track, you have my sign-offs.

If it is not too much trouble, can you send only the relevant
patches (these three) with sign-off again?  I'd prefer to give
patches a new chance to be eyeballed by people, and they will tend
to have a better chance by being a smaller and an isolated topic.

Thanks.

-- 
-- 
*** Please reply-to-all at all times ***
*** (do not pretend to know who is subscribed and who is not) ***
*** Please avoid top-posting. ***
The msysGit Wiki is here: https://github.com/msysgit/msysgit/wiki - Github accounts are free.

You received this message because you are subscribed to the Google
Groups "msysGit" group.
To post to this group, send email to msysgit@googlegroups.com
To unsubscribe from this group, send email to
msysgit+unsubscribe@googlegroups.com
For more options, and view previous threads, visit this group at
http://groups.google.com/group/msysgit?hl=en_US?hl=en

--- 
You received this message because you are subscribed to the Google Groups "msysGit" group.
To unsubscribe from this group and stop receiving emails from it, send an email to msysgit+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

Re: [PATCH 1/2] symlinks: remove PATH_MAX limitation

[Johannes Schindelin]

Hi,

On Sat, 5 Jul 2014, Karsten Blees wrote:

> 'git checkout' fails if a directory is longer than PATH_MAX, because the
> lstat_cache in symlinks.c checks if the leading directory exists using
> PATH_MAX-bounded string operations.
> 
> Remove the limitation by using strbuf instead.
> 
> Signed-off-by: Karsten Blees <blees@dcon.de>
> ---
> 
> This fixes a bug on Windows with long paths [1].
> 
> [1] https://github.com/msysgit/msysgit/issues/227

For context: on Windows, PATH_MAX is ridiculously small. And it really is
that small, file functions of the Win32 API cannot handle longer paths.

However, there is a (pretty ugly, if you ask me) workaround: prefixing the
paths with "\\?\" and lo and behold, *many* (but not all) Win32 API
functions can then handle paths up to something around 32,000 characters!

The problem Karsten addressed here is caused exactly by this oddity: we
*can* handle paths much longer than PATH_MAX. Therefore, we need to
decouple the path buffers from that hardcoded limitation.

Karsten, you rock.

Ciao,
Dscho

-- 
-- 
*** Please reply-to-all at all times ***
*** (do not pretend to know who is subscribed and who is not) ***
*** Please avoid top-posting. ***
The msysGit Wiki is here: https://github.com/msysgit/msysgit/wiki - Github accounts are free.

You received this message because you are subscribed to the Google
Groups "msysGit" group.
To post to this group, send email to msysgit@googlegroups.com
To unsubscribe from this group, send email to
msysgit+unsubscribe@googlegroups.com
For more options, and view previous threads, visit this group at
http://groups.google.com/group/msysgit?hl=en_US?hl=en

--- 
You received this message because you are subscribed to the Google Groups "msysGit" group.
To unsubscribe from this group and stop receiving emails from it, send an email to msysgit+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

Re: [PATCH 1/2] symlinks: remove PATH_MAX limitation

[Junio C Hamano]

Karsten Blees <karsten.blees@gmail.com> writes:

> 'git checkout' fails if a directory is longer than PATH_MAX, because the
> lstat_cache in symlinks.c checks if the leading directory exists using
> PATH_MAX-bounded string operations.
>
> Remove the limitation by using strbuf instead.

Good.

> diff --git a/cache.h b/cache.h
> index df65231..44aa439 100644
> --- a/cache.h
> +++ b/cache.h
> @@ -1090,12 +1090,16 @@ struct checkout {
>  extern int checkout_entry(struct cache_entry *ce, const struct checkout *state, char *topath);
>  
>  struct cache_def {
> -	char path[PATH_MAX + 1];
> -	int len;
> +	struct strbuf path;
>  	int flags;
>  	int track_flags;
>  	int prefix_len_stat_func;
>  };
> +#define CACHE_DEF_INIT { STRBUF_INIT, 0, 0, 0 }
> +static inline void cache_def_free(struct cache_def *cache)
> +{
> +	strbuf_release(&cache->path);
> +}

The above cache_def_free(cache) does not free the cache itself, but
only its associated data, so the name cache_def_free() is somewhat
misleading.

It seems that we use the name that consists solely of "something"
and "free" if the pointer to "something" itself is also freed.  For
example "diff_free_filespec_data(struct diff_filespec *)" frees data
associated with the filespec but not the filespec itself, which is
done with "diff_free_filespec()".

Another name that may not be misleading would be cache_def_clear();
I think I'd prefer it over cache_def_free_data().

-- 
-- 
*** Please reply-to-all at all times ***
*** (do not pretend to know who is subscribed and who is not) ***
*** Please avoid top-posting. ***
The msysGit Wiki is here: https://github.com/msysgit/msysgit/wiki - Github accounts are free.

You received this message because you are subscribed to the Google
Groups "msysGit" group.
To post to this group, send email to msysgit@googlegroups.com
To unsubscribe from this group, send email to
msysgit+unsubscribe@googlegroups.com
For more options, and view previous threads, visit this group at
http://groups.google.com/group/msysgit?hl=en_US?hl=en

--- 
You received this message because you are subscribed to the Google Groups "msysGit" group.
To unsubscribe from this group and stop receiving emails from it, send an email to msysgit+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

Re: [PATCH 1/2] symlinks: remove PATH_MAX limitation

[Karsten Blees]

Am 07.07.2014 20:30, schrieb Junio C Hamano:
> Karsten Blees <karsten.blees@gmail.com> writes:
> 
> The above cache_def_free(cache) does not free the cache itself, but
> only its associated data, so the name cache_def_free() is somewhat
> misleading.
> 

You already merged this to master ("kb/path-max-must-go" lol), should
I send a fixup! s/cache_def_free/cache_def_clear/ or is it OK as is?

-- 
-- 
*** Please reply-to-all at all times ***
*** (do not pretend to know who is subscribed and who is not) ***
*** Please avoid top-posting. ***
The msysGit Wiki is here: https://github.com/msysgit/msysgit/wiki - Github accounts are free.

You received this message because you are subscribed to the Google
Groups "msysGit" group.
To post to this group, send email to msysgit@googlegroups.com
To unsubscribe from this group, send email to
msysgit+unsubscribe@googlegroups.com
For more options, and view previous threads, visit this group at
http://groups.google.com/group/msysgit?hl=en_US?hl=en

--- 
You received this message because you are subscribed to the Google Groups "msysGit" group.
To unsubscribe from this group and stop receiving emails from it, send an email to msysgit+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

Re: [PATCH 1/2] symlinks: remove PATH_MAX limitation

[Junio C Hamano]

Karsten Blees <karsten.blees@gmail.com> writes:

> Am 07.07.2014 20:30, schrieb Junio C Hamano:
>> Karsten Blees <karsten.blees@gmail.com> writes:
>> 
>> The above cache_def_free(cache) does not free the cache itself, but
>> only its associated data, so the name cache_def_free() is somewhat
>> misleading.
>> 
>
> You already merged this to master ("kb/path-max-must-go" lol), should
> I send a fixup! s/cache_def_free/cache_def_clear/ or is it OK as is?

I do not think a fix-up would hurt other topics in flight, so
please do so.

-- 
-- 
*** Please reply-to-all at all times ***
*** (do not pretend to know who is subscribed and who is not) ***
*** Please avoid top-posting. ***
The msysGit Wiki is here: https://github.com/msysgit/msysgit/wiki - Github accounts are free.

You received this message because you are subscribed to the Google
Groups "msysGit" group.
To post to this group, send email to msysgit@googlegroups.com
To unsubscribe from this group, send email to
msysgit+unsubscribe@googlegroups.com
For more options, and view previous threads, visit this group at
http://groups.google.com/group/msysgit?hl=en_US?hl=en

--- 
You received this message because you are subscribed to the Google Groups "msysGit" group.
To unsubscribe from this group and stop receiving emails from it, send an email to msysgit+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[PATCH] fixup! symlinks: remove PATH_MAX limitation

[Karsten Blees]

Rename cache_def_free to cache_def_clear as it doesn't free the struct
cache_def, but just clears its content.

Signed-off-by: Karsten Blees <blees@dcon.de>
---
 cache.h         | 2 +-
 preload-index.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/cache.h b/cache.h
index 44aa439..378ee7f 100644
--- a/cache.h
+++ b/cache.h
@@ -1096,7 +1096,7 @@ struct cache_def {
 	int prefix_len_stat_func;
 };
 #define CACHE_DEF_INIT { STRBUF_INIT, 0, 0, 0 }
-static inline void cache_def_free(struct cache_def *cache)
+static inline void cache_def_clear(struct cache_def *cache)
 {
 	strbuf_release(&cache->path);
 }
diff --git a/preload-index.c b/preload-index.c
index 79ce8a9..c1fe3a3 100644
--- a/preload-index.c
+++ b/preload-index.c
@@ -63,7 +63,7 @@ static void *preload_thread(void *_data)
 			continue;
 		ce_mark_uptodate(ce);
 	} while (--nr > 0);
-	cache_def_free(&cache);
+	cache_def_clear(&cache);
 	return NULL;
 }
 
-- 
2.0.1.563.g66f467c.dirty

-- 
-- 
*** Please reply-to-all at all times ***
*** (do not pretend to know who is subscribed and who is not) ***
*** Please avoid top-posting. ***
The msysGit Wiki is here: https://github.com/msysgit/msysgit/wiki - Github accounts are free.

You received this message because you are subscribed to the Google
Groups "msysGit" group.
To post to this group, send email to msysgit@googlegroups.com
To unsubscribe from this group, send email to
msysgit+unsubscribe@googlegroups.com
For more options, and view previous threads, visit this group at
http://groups.google.com/group/msysgit?hl=en_US?hl=en

--- 
You received this message because you are subscribed to the Google Groups "msysGit" group.
To unsubscribe from this group and stop receiving emails from it, send an email to msysgit+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.