Git development
 help / color / mirror / Atom feed
* Re: "git diff --stat" doesn't show added empty file
From: Johannes Schindelin @ 2009-01-13 11:27 UTC (permalink / raw)
  To: Michael J Gruber; +Cc: Ping Yin, Git Mailing List
In-Reply-To: <496B64BA.7000408@drmicha.warpmail.net>

Hi,

On Mon, 12 Jan 2009, Michael J Gruber wrote:

> Johannes Schindelin venit, vidit, dixit 01/12/09 13:19:
> 
> > On Mon, 12 Jan 2009, Ping Yin wrote:
> > 
> >> $ git --version
> >> git version 1.6.1.9.g97c34
> >> $ mkdir test && cd test && git init && git commit --allow-empty -m
> >> "Initial commit"
> >> $ touch .gitignore && git add .gitignore && git commit -m "Add empty .gitignore"
> >> $ git diff --stat HEAD^..
> >>  0 files changed, 0 insertions(+), 0 deletions(-)
> >>
> >> May be the following is better?
> >>
> >>  .gitignore |    0 +
> >>  1 files changed, 0 insertions(+), 0 deletions(-)
> > 
> > Have fun coding that.
> 
> Removing 5 lines from diff.c does the job,

I can only think that you mean these lines:

                else if (!data->files[i]->is_renamed &&
                         (added + deleted == 0)) {
                        total_files--;
                        continue;
                }

However, they are also present in the initial stat code in d75f7952:

                } else if (added + deleted == 0) {
                        total_files--;
                        continue;
                }

Now, the fun part is finding out why this code is there.  Are there 
unintended side effects?  If so, are they still there?  Or was it 
introduced due to an explicit request back then?  Was it just sloppy?

Removing the 5 lines is not all you have to do, by _far_.

Lots of fun,
Dscho

^ permalink raw reply

* Re: how to combine 2 commits?
From: Sitaram Chamarty @ 2009-01-13 11:32 UTC (permalink / raw)
  To: git
In-Reply-To: <vpqiqojpuqn.fsf@bauges.imag.fr>

On 2009-01-13, Matthieu Moy <Matthieu.Moy@imag.fr> wrote:
> Sitaram Chamarty <sitaramc@gmail.com> writes:
>
>> I like using "git rebase -i HEAD~5" (if I want to rebase the
>> last 5 commits).  In the editor that pops up, I reorder the
>> ones that I know should be together, and on each set to be
>> squashed, I change the "pick" to "s" (for squash) on all but
>> the first one.  Save the file and it's all done.
>
> And right after, you probably want to do stg like
>
>   git diff master@{1} master
>
> to see whether you actually changed the result by reordering your
> patches.

I used to do that, but now I've stopped.  If it doesn't
conflict, I know it's good.  I go straight to the equivalent
of make && make test.

I must mention though, that I don't reorder the relative
sequences of commits affecting the same file/set of files.
That might have something to do with my confidence :-)

^ permalink raw reply

* [PATCH/RFC v7 0/5] git checkout: optimise away lots of lstat() calls
From: Kjetil Barvik @ 2009-01-13 12:29 UTC (permalink / raw)
  To: git; +Cc: Junio C Hamano, Pete Harlan, Linus Torvalds, Kjetil Barvik

Changes since version 6:

a) The patch-split was not correct for patch 3/3.  In v6 it could look
   like the lines:

-	char path[PATH_MAX];
+	char path[PATH_MAX + 1];

   was a random bugfix.  In this version for patch 3/5 we also change
   "&& last_slash < PATH_MAX" into "&& last_slash <= PATH_MAX" (and
   similar for the else-if part).  This could also make the intro-
   duction of the 'FL_FULLPATH' flag more readable for this patch.

b) Cleanup and added 3 comments to 'greatest_match_lstat_cache()'

c) Introduction of the 'invalidate_lstat_cache()' function.  How does
   the interface look?  good?  bad?  Does the function do what people
   expect it to do?

d) Reintroduction of the 'clear_lstat_cache()' function.

Junio, I hope it is possible to use patches 1/5, 2/5 and 3/5 from this
version instead of 1/3, 2/3 and 3/3 from version 6, for the possible
future in origin/pu?  See also a) above.  Thanks in advance!

In general, are we allowed to redesign the patch-series while the
patches is inside origin/pu?


Kjetil Barvik (5):
  lstat_cache(): more cache effective symlink/directory detection
  lstat_cache(): introduce has_symlink_or_noent_leading_path() function
  lstat_cache(): introduce has_dirs_only_path() function
  lstat_cache(): introduce invalidate_lstat_cache() function
  lstat_cache(): introduce clear_lstat_cache() function

 cache.h        |    4 +
 entry.c        |   34 +++-----
 symlinks.c     |  249 ++++++++++++++++++++++++++++++++++++++++++++++----------
 unpack-trees.c |    4 +-
 4 files changed, 222 insertions(+), 69 deletions(-)

^ permalink raw reply

* [PATCH/RFC v7 1/5] lstat_cache(): more cache effective symlink/directory detection
From: Kjetil Barvik @ 2009-01-13 12:29 UTC (permalink / raw)
  To: git; +Cc: Junio C Hamano, Pete Harlan, Linus Torvalds, Kjetil Barvik
In-Reply-To: <1231849748-8244-1-git-send-email-barvik@broadpark.no>

Make the cache functionality more effective.  Previously when A/B/C/D
was in the cache and A/B/C/E/file.c was called for, there was no match
at all from the cache.  Now we use the fact that the paths "A", "A/B"
and "A/B/C" are already tested, and we only need to do an lstat() call
on "A/B/C/E".

We only cache/store the last path regardless of its type.  Since the
cache functionality is always used with alphabetically sorted names
(at least it seems so for me), there is no need to store both the last
symlink-leading path and the last real-directory path.  Note that if
the cache is not called with (mostly) alphabetically sorted names,
neither the old, nor this new one, would be very effective.

Previously, when symlink A/B/C/S was cached/stored in the symlink-
leading path, and A/B/C/file.c was called for, it was not easy to use
the fact that we already knew that the paths "A", "A/B" and "A/B/C"
are real directories.

Avoid copying the first path components of the name 2 zillion times
when we test new path components.  Since we always cache/store the
last path, we can copy each component as we test those directly into
the cache.  Previously we ended up doing a memcpy() for the full
path/name right before each lstat() call, and when updating the cache
for each time we have tested a new path component.

We also use less memory, that is, PATH_MAX bytes less memory on the
stack and PATH_MAX bytes less memory on the heap.

Thanks to Junio C Hamano, Linus Torvalds and Rene Scharfe for valuable
comments to this patch!

Signed-off-by: Kjetil Barvik <barvik@broadpark.no>
---
 symlinks.c |  157 ++++++++++++++++++++++++++++++++++++++++++++---------------
 1 files changed, 117 insertions(+), 40 deletions(-)

diff --git a/symlinks.c b/symlinks.c
index 5a5e781..b17e4b3 100644
--- a/symlinks.c
+++ b/symlinks.c
@@ -1,64 +1,141 @@
 #include "cache.h"
 
-struct pathname {
-	int len;
+static struct cache_def {
 	char path[PATH_MAX];
-};
+	int len;
+	int flags;
+} cache;
 
-/* Return matching pathname prefix length, or zero if not matching */
-static inline int match_pathname(int len, const char *name, struct pathname *match)
+/* Returns the length (on a path component basis) of the longest
+ * common prefix match of 'name' and the cached path string.
+ */
+static inline int greatest_match_lstat_cache(int len, const char *name)
 {
-	int match_len = match->len;
-	return (len > match_len &&
-		name[match_len] == '/' &&
-		!memcmp(name, match->path, match_len)) ? match_len : 0;
+	int max_len, match_len = 0, i = 0;
+
+	max_len = len < cache.len ? len : cache.len;
+	while (i < max_len && name[i] == cache.path[i]) {
+		if (name[i] == '/')
+			match_len = i;
+		i++;
+	}
+	/* Is the cached path string a substring of 'name'? */
+	if (i == cache.len && len > cache.len && name[cache.len] == '/')
+		match_len = cache.len;
+	/* Is 'name' a substring of the cached path string? */
+	else if ((i == len && len < cache.len && cache.path[len] == '/') ||
+		 (i == len && len == cache.len))
+		match_len = len;
+	return match_len;
 }
 
-static inline void set_pathname(int len, const char *name, struct pathname *match)
+static inline void reset_lstat_cache(void)
 {
-	if (len < PATH_MAX) {
-		match->len = len;
-		memcpy(match->path, name, len);
-		match->path[len] = 0;
-	}
+	cache.path[0] = '\0';
+	cache.len   = 0;
+	cache.flags = 0;
 }
 
-int has_symlink_leading_path(int len, const char *name)
+#define FL_DIR      (1 << 0)
+#define FL_SYMLINK  (1 << 1)
+#define FL_LSTATERR (1 << 2)
+#define FL_ERR      (1 << 3)
+
+/*
+ * Check if name 'name' of length 'len' has a symlink leading
+ * component, or if the directory exists and is real.
+ *
+ * To speed up the check, some information is allowed to be cached.
+ * This is can be indicated by the 'track_flags' argument.
+ */
+static int lstat_cache(int len, const char *name,
+		       int track_flags)
 {
-	static struct pathname link, nonlink;
-	char path[PATH_MAX];
+	int match_len, last_slash, last_slash_dir;
+	int match_flags, ret_flags, save_flags, max_len;
 	struct stat st;
-	char *sp;
-	int known_dir;
 
 	/*
-	 * See if the last known symlink cache matches.
+	 * Check to see if we have a match from the cache for the
+	 * symlink path type.
 	 */
-	if (match_pathname(len, name, &link))
-		return 1;
-
+	match_len = last_slash = greatest_match_lstat_cache(len, name);
+	match_flags = cache.flags & track_flags & FL_SYMLINK;
+	if (match_flags && match_len == cache.len)
+		return match_flags;
 	/*
-	 * Get rid of the last known directory part
+	 * If 'name' is a substring of the cache on a path component
+	 * basis, and a directory is cached, we can return
+	 * immediately.
 	 */
-	known_dir = match_pathname(len, name, &nonlink);
+	match_flags = cache.flags & track_flags & FL_DIR;
+	if (match_flags && match_len == len)
+		return match_flags;
 
-	while ((sp = strchr(name + known_dir + 1, '/')) != NULL) {
-		int thislen = sp - name ;
-		memcpy(path, name, thislen);
-		path[thislen] = 0;
+	/* Okay, no match from the cache so far, so now we have to
+	 * check the rest of the path components.
+	 */
+	ret_flags = FL_DIR;
+	last_slash_dir = last_slash;
+	max_len = len < PATH_MAX ? len : PATH_MAX;
+	while (match_len < max_len) {
+		do {
+			cache.path[match_len] = name[match_len];
+			match_len++;
+		} while (match_len < max_len && name[match_len] != '/');
+		if (match_len >= max_len)
+			break;
+		last_slash = match_len;
+		cache.path[last_slash] = '\0';
 
-		if (lstat(path, &st))
-			return 0;
-		if (S_ISDIR(st.st_mode)) {
-			set_pathname(thislen, path, &nonlink);
-			known_dir = thislen;
+		if (lstat(cache.path, &st)) {
+			ret_flags = FL_LSTATERR;
+		} else if (S_ISDIR(st.st_mode)) {
+			last_slash_dir = last_slash;
 			continue;
-		}
-		if (S_ISLNK(st.st_mode)) {
-			set_pathname(thislen, path, &link);
-			return 1;
+		} else if (S_ISLNK(st.st_mode)) {
+			ret_flags = FL_SYMLINK;
+		} else {
+			ret_flags = FL_ERR;
 		}
 		break;
 	}
-	return 0;
+
+	/* At the end update the cache.  Note that max 2 different
+	 * path types, FL_SYMLINK and FL_DIR, can be cached for the
+	 * moment!
+	 */
+	save_flags = ret_flags & track_flags & FL_SYMLINK;
+	if (save_flags && last_slash > 0 && last_slash < PATH_MAX) {
+		cache.path[last_slash] = '\0';
+		cache.len   = last_slash;
+		cache.flags = save_flags;
+	} else if (track_flags & FL_DIR &&
+		   last_slash_dir > 0 && last_slash_dir < PATH_MAX) {
+		/* We have a separate test for the directory case,
+		 * since it could be that we have found a symlink and
+		 * the track_flags says that we can not cache this
+		 * fact, so the cache would then have been left empty
+		 * in this case.
+		 *
+		 * But, if we is allowed to track real directories, we
+		 * can still cache the path components before the last
+		 * one (the found symlink component).
+		 */
+		cache.path[last_slash_dir] = '\0';
+		cache.len   = last_slash_dir;
+		cache.flags = FL_DIR;
+	} else {
+		reset_lstat_cache();
+	}
+	return ret_flags;
+}
+
+/* Return non-zero if path 'name' has a leading symlink component.
+ */
+int has_symlink_leading_path(int len, const char *name)
+{
+	return lstat_cache(len, name,
+			   FL_SYMLINK|FL_DIR) &
+		FL_SYMLINK;
 }
-- 
1.6.0.2.GIT

^ permalink raw reply related

* [PATCH/RFC v7 2/5] lstat_cache(): introduce has_symlink_or_noent_leading_path() function
From: Kjetil Barvik @ 2009-01-13 12:29 UTC (permalink / raw)
  To: git; +Cc: Junio C Hamano, Pete Harlan, Linus Torvalds, Kjetil Barvik
In-Reply-To: <1231849748-8244-1-git-send-email-barvik@broadpark.no>

In some cases, especially inside the unpack-trees.c file, and inside
the verify_absent() function, we can avoid some unnecessary calls to
lstat(), if the lstat_cache() function can also be told to keep track
of none existing directories.

So we update the lstat_cache() function to handle this new fact,
introduce a new wrapper function, and the result is that we save lots
of lstat() calls for a removed directory which previously contained
lots of files, when we call this new wrapper of lstat_cache() instead
of the old one.

We do similar changes inside the unlink_entry() function, since if we
can already say that the leading directory component of a pathname
does not exist, it is not necessary to try to remove a pathname below
it!

Thanks to Junio C Hamano, Linus Torvalds and Rene Scharfe for valuable
comments to this patch!

Signed-off-by: Kjetil Barvik <barvik@broadpark.no>
---
 cache.h        |    1 +
 symlinks.c     |   87 ++++++++++++++++++++++++++++++++++++--------------------
 unpack-trees.c |    4 +-
 3 files changed, 59 insertions(+), 33 deletions(-)

diff --git a/cache.h b/cache.h
index 231c06d..11181aa 100644
--- a/cache.h
+++ b/cache.h
@@ -720,6 +720,7 @@ struct checkout {
 
 extern int checkout_entry(struct cache_entry *ce, const struct checkout *state, char *topath);
 extern int has_symlink_leading_path(int len, const char *name);
+extern int has_symlink_or_noent_leading_path(int len, const char *name);
 
 extern struct alternate_object_database {
 	struct alternate_object_database *next;
diff --git a/symlinks.c b/symlinks.c
index b17e4b3..52b3b0e 100644
--- a/symlinks.c
+++ b/symlinks.c
@@ -4,6 +4,7 @@ static struct cache_def {
 	char path[PATH_MAX];
 	int len;
 	int flags;
+	int track_flags;
 } cache;
 
 /* Returns the length (on a path component basis) of the longest
@@ -29,21 +30,23 @@ static inline int greatest_match_lstat_cache(int len, const char *name)
 	return match_len;
 }
 
-static inline void reset_lstat_cache(void)
+static inline void reset_lstat_cache(int track_flags)
 {
 	cache.path[0] = '\0';
 	cache.len   = 0;
 	cache.flags = 0;
+	cache.track_flags = track_flags;
 }
 
 #define FL_DIR      (1 << 0)
-#define FL_SYMLINK  (1 << 1)
-#define FL_LSTATERR (1 << 2)
-#define FL_ERR      (1 << 3)
+#define FL_NOENT    (1 << 1)
+#define FL_SYMLINK  (1 << 2)
+#define FL_LSTATERR (1 << 3)
+#define FL_ERR      (1 << 4)
 
 /*
  * Check if name 'name' of length 'len' has a symlink leading
- * component, or if the directory exists and is real.
+ * component, or if the directory exists and is real, or not.
  *
  * To speed up the check, some information is allowed to be cached.
  * This is can be indicated by the 'track_flags' argument.
@@ -55,22 +58,32 @@ static int lstat_cache(int len, const char *name,
 	int match_flags, ret_flags, save_flags, max_len;
 	struct stat st;
 
-	/*
-	 * Check to see if we have a match from the cache for the
-	 * symlink path type.
-	 */
-	match_len = last_slash = greatest_match_lstat_cache(len, name);
-	match_flags = cache.flags & track_flags & FL_SYMLINK;
-	if (match_flags && match_len == cache.len)
-		return match_flags;
-	/*
-	 * If 'name' is a substring of the cache on a path component
-	 * basis, and a directory is cached, we can return
-	 * immediately.
-	 */
-	match_flags = cache.flags & track_flags & FL_DIR;
-	if (match_flags && match_len == len)
-		return match_flags;
+	if (cache.track_flags != track_flags) {
+		/*
+		 * As a safeguard we clear the cache if the value of
+		 * track_flags does not match with the last supplied
+		 * value.
+		 */
+		reset_lstat_cache(track_flags);
+		match_len = last_slash = 0;
+	} else {
+		/*
+		 * Check to see if we have a match from the cache for
+		 * the 2 "excluding" path types.
+		 */
+		match_len = last_slash = greatest_match_lstat_cache(len, name);
+		match_flags = cache.flags & track_flags & (FL_NOENT|FL_SYMLINK);
+		if (match_flags && match_len == cache.len)
+			return match_flags;
+		/*
+		 * If 'name' is a substring of the cache on a path
+		 * component basis, and a directory is cached, we
+		 * can return immediately.
+		 */
+		match_flags = cache.flags & track_flags & FL_DIR;
+		if (match_flags && match_len == len)
+			return match_flags;
+	}
 
 	/* Okay, no match from the cache so far, so now we have to
 	 * check the rest of the path components.
@@ -90,6 +103,8 @@ static int lstat_cache(int len, const char *name,
 
 		if (lstat(cache.path, &st)) {
 			ret_flags = FL_LSTATERR;
+			if (errno == ENOENT)
+				ret_flags |= FL_NOENT;
 		} else if (S_ISDIR(st.st_mode)) {
 			last_slash_dir = last_slash;
 			continue;
@@ -101,11 +116,11 @@ static int lstat_cache(int len, const char *name,
 		break;
 	}
 
-	/* At the end update the cache.  Note that max 2 different
-	 * path types, FL_SYMLINK and FL_DIR, can be cached for the
-	 * moment!
+	/* At the end update the cache.  Note that max 3 different
+	 * path types, FL_NOENT, FL_SYMLINK and FL_DIR, can be cached
+	 * for the moment!
 	 */
-	save_flags = ret_flags & track_flags & FL_SYMLINK;
+	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;
@@ -113,20 +128,20 @@ static int lstat_cache(int len, const char *name,
 	} else if (track_flags & FL_DIR &&
 		   last_slash_dir > 0 && last_slash_dir < PATH_MAX) {
 		/* We have a separate test for the directory case,
-		 * since it could be that we have found a symlink and
-		 * the track_flags says that we can not cache this
-		 * fact, so the cache would then have been left empty
-		 * in this case.
+		 * since it could be that we have found a symlink or a
+		 * none existing directory and the track_flags says
+		 * that we can not cache this fact, so the cache would
+		 * then have been left empty in this case.
 		 *
 		 * But, if we is allowed to track real directories, we
 		 * can still cache the path components before the last
-		 * one (the found symlink component).
+		 * one (the found symlink or none existing component).
 		 */
 		cache.path[last_slash_dir] = '\0';
 		cache.len   = last_slash_dir;
 		cache.flags = FL_DIR;
 	} else {
-		reset_lstat_cache();
+		reset_lstat_cache(track_flags);
 	}
 	return ret_flags;
 }
@@ -139,3 +154,13 @@ int has_symlink_leading_path(int len, const char *name)
 			   FL_SYMLINK|FL_DIR) &
 		FL_SYMLINK;
 }
+
+/* Return non-zero if path 'name' has a leading symlink component or
+ * if some leading path component does not exists.
+ */
+int has_symlink_or_noent_leading_path(int len, const char *name)
+{
+	return lstat_cache(len, name,
+			   FL_SYMLINK|FL_NOENT|FL_DIR) &
+		(FL_SYMLINK|FL_NOENT);
+}
diff --git a/unpack-trees.c b/unpack-trees.c
index 54f301d..a3fd383 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -61,7 +61,7 @@ static void unlink_entry(struct cache_entry *ce)
 	char *cp, *prev;
 	char *name = ce->name;
 
-	if (has_symlink_leading_path(ce_namelen(ce), ce->name))
+	if (has_symlink_or_noent_leading_path(ce_namelen(ce), ce->name))
 		return;
 	if (unlink(name))
 		return;
@@ -584,7 +584,7 @@ static int verify_absent(struct cache_entry *ce, const char *action,
 	if (o->index_only || o->reset || !o->update)
 		return 0;
 
-	if (has_symlink_leading_path(ce_namelen(ce), ce->name))
+	if (has_symlink_or_noent_leading_path(ce_namelen(ce), ce->name))
 		return 0;
 
 	if (!lstat(ce->name, &st)) {
-- 
1.6.0.2.GIT

^ permalink raw reply related

* [PATCH/RFC v7 3/5] lstat_cache(): introduce has_dirs_only_path() function
From: Kjetil Barvik @ 2009-01-13 12:29 UTC (permalink / raw)
  To: git; +Cc: Junio C Hamano, Pete Harlan, Linus Torvalds, Kjetil Barvik
In-Reply-To: <1231849748-8244-1-git-send-email-barvik@broadpark.no>

Inside the create_directories() function inside entry.c we currently
do an stat() or lstat() call for each path component of the pathname
'path' each and every time called.  For the 'git checkout' command,
this function is called on each file for which we must do an update
(ce->ce_flags & CE_UPDATE), so we get lots and lots of calls...

To fix this, we make a new wrapper to the lstat_cache() function, and
call the wrapper function instead of the calls to the stat() or the
lstat() functions.  Since the paths given to the create_directories()
function, is sorted alphabetically, the new wrapper would be very
cache effective in this situation.

To support it we must update the lstat_cache() function to be able to
say that "pleace test the complete length of 'name'", and also to give
it the length of a prefix, where the cache should use the stat()
function instead of the lstat() function to test each path component.

Thanks to Junio C Hamano, Linus Torvalds and Rene Scharfe for valuable
comments to this patch!

Signed-off-by: Kjetil Barvik <barvik@broadpark.no>
---
 cache.h    |    1 +
 entry.c    |   34 ++++++++++---------------------
 symlinks.c |   63 +++++++++++++++++++++++++++++++++++++++++++----------------
 3 files changed, 58 insertions(+), 40 deletions(-)

diff --git a/cache.h b/cache.h
index 11181aa..7c8c8e4 100644
--- a/cache.h
+++ b/cache.h
@@ -721,6 +721,7 @@ struct checkout {
 extern int checkout_entry(struct cache_entry *ce, const struct checkout *state, char *topath);
 extern int has_symlink_leading_path(int len, const char *name);
 extern int has_symlink_or_noent_leading_path(int len, const char *name);
+extern int has_dirs_only_path(int len, const char *name, int prefix_len);
 
 extern struct alternate_object_database {
 	struct alternate_object_database *next;
diff --git a/entry.c b/entry.c
index aa2ee46..b0295bd 100644
--- a/entry.c
+++ b/entry.c
@@ -8,35 +8,23 @@ static void create_directories(const char *path, const struct checkout *state)
 	const char *slash = path;
 
 	while ((slash = strchr(slash+1, '/')) != NULL) {
-		struct stat st;
-		int stat_status;
-
 		len = slash - path;
 		memcpy(buf, path, len);
 		buf[len] = 0;
 
-		if (len <= state->base_dir_len)
-			/*
-			 * checkout-index --prefix=<dir>; <dir> is
-			 * allowed to be a symlink to an existing
-			 * directory.
-			 */
-			stat_status = stat(buf, &st);
-		else
-			/*
-			 * if there currently is a symlink, we would
-			 * want to replace it with a real directory.
-			 */
-			stat_status = lstat(buf, &st);
-
-		if (!stat_status && S_ISDIR(st.st_mode))
+		/* For 'checkout-index --prefix=<dir>', <dir> is
+		 * allowed to be a symlink to an existing directory,
+		 * and we set 'state->base_dir_len' below, such that
+		 * we test the path components of the prefix with the
+		 * stat() function instead of the lstat() function.
+		 */
+		if (has_dirs_only_path(len, buf, state->base_dir_len))
 			continue; /* ok, it is already a directory. */
 
-		/*
-		 * We know stat_status == 0 means something exists
-		 * there and this mkdir would fail, but that is an
-		 * error codepath; we do not care, as we unlink and
-		 * mkdir again in such a case.
+		/* If this mkdir() would fail, it could be that there
+		 * is already a symlink or something else exists
+		 * there, therefore we then try to unlink it and try
+		 * one more time to create the directory.
 		 */
 		if (mkdir(buf, 0777)) {
 			if (errno == EEXIST && state->force &&
diff --git a/symlinks.c b/symlinks.c
index 52b3b0e..f9d1821 100644
--- a/symlinks.c
+++ b/symlinks.c
@@ -1,10 +1,11 @@
 #include "cache.h"
 
 static struct cache_def {
-	char path[PATH_MAX];
+	char path[PATH_MAX + 1];
 	int len;
 	int flags;
 	int track_flags;
+	int prefix_len_stat_func;
 } cache;
 
 /* Returns the length (on a path component basis) of the longest
@@ -30,12 +31,13 @@ static inline int greatest_match_lstat_cache(int len, const char *name)
 	return match_len;
 }
 
-static inline void reset_lstat_cache(int track_flags)
+static inline void reset_lstat_cache(int track_flags, int prefix_len_stat_func)
 {
 	cache.path[0] = '\0';
 	cache.len   = 0;
 	cache.flags = 0;
 	cache.track_flags = track_flags;
+	cache.prefix_len_stat_func = prefix_len_stat_func;
 }
 
 #define FL_DIR      (1 << 0)
@@ -43,28 +45,35 @@ static inline void reset_lstat_cache(int track_flags)
 #define FL_SYMLINK  (1 << 2)
 #define FL_LSTATERR (1 << 3)
 #define FL_ERR      (1 << 4)
+#define FL_FULLPATH (1 << 5)
 
 /*
  * Check if name 'name' of length 'len' has a symlink leading
  * component, or if the directory exists and is real, or not.
  *
  * To speed up the check, some information is allowed to be cached.
- * This is can be indicated by the 'track_flags' argument.
+ * This is can be indicated by the 'track_flags' argument, which also
+ * can be used to indicate that we should always check the full path.
+ *
+ * The 'prefix_len_stat_func' parameter can be used to set the length
+ * of the prefix, where the cache should use the stat() function
+ * instead of the lstat() function to test each path component.
  */
 static int lstat_cache(int len, const char *name,
-		       int track_flags)
+		       int track_flags, int prefix_len_stat_func)
 {
 	int match_len, last_slash, last_slash_dir;
-	int match_flags, ret_flags, save_flags, max_len;
+	int match_flags, ret_flags, save_flags, max_len, ret;
 	struct stat st;
 
-	if (cache.track_flags != track_flags) {
+	if (cache.track_flags != track_flags ||
+	    cache.prefix_len_stat_func != prefix_len_stat_func) {
 		/*
-		 * As a safeguard we clear the cache if the value of
-		 * track_flags does not match with the last supplied
-		 * value.
+		 * As a safeguard we clear the cache if the values of
+		 * track_flags and/or prefix_len_stat_func does not
+		 * match with the last supplied values.
 		 */
-		reset_lstat_cache(track_flags);
+		reset_lstat_cache(track_flags, prefix_len_stat_func);
 		match_len = last_slash = 0;
 	} else {
 		/*
@@ -96,12 +105,17 @@ static int lstat_cache(int len, const char *name,
 			cache.path[match_len] = name[match_len];
 			match_len++;
 		} while (match_len < max_len && name[match_len] != '/');
-		if (match_len >= max_len)
+		if (match_len >= max_len && !(track_flags & FL_FULLPATH))
 			break;
 		last_slash = match_len;
 		cache.path[last_slash] = '\0';
 
-		if (lstat(cache.path, &st)) {
+		if (last_slash <= prefix_len_stat_func)
+			ret = stat(cache.path, &st);
+		else
+			ret = lstat(cache.path, &st);
+
+		if (ret) {
 			ret_flags = FL_LSTATERR;
 			if (errno == ENOENT)
 				ret_flags |= FL_NOENT;
@@ -121,12 +135,12 @@ static int lstat_cache(int len, const char *name,
 	 * for the moment!
 	 */
 	save_flags = ret_flags & track_flags & (FL_NOENT|FL_SYMLINK);
-	if (save_flags && last_slash > 0 && last_slash < PATH_MAX) {
+	if (save_flags && last_slash > 0 && last_slash <= PATH_MAX) {
 		cache.path[last_slash] = '\0';
 		cache.len   = last_slash;
 		cache.flags = save_flags;
 	} else if (track_flags & FL_DIR &&
-		   last_slash_dir > 0 && last_slash_dir < PATH_MAX) {
+		   last_slash_dir > 0 && last_slash_dir <= PATH_MAX) {
 		/* We have a separate test for the directory case,
 		 * since it could be that we have found a symlink or a
 		 * none existing directory and the track_flags says
@@ -141,17 +155,19 @@ static int lstat_cache(int len, const char *name,
 		cache.len   = last_slash_dir;
 		cache.flags = FL_DIR;
 	} else {
-		reset_lstat_cache(track_flags);
+		reset_lstat_cache(track_flags, prefix_len_stat_func);
 	}
 	return ret_flags;
 }
 
+#define USE_ONLY_LSTAT  0
+
 /* Return non-zero if path 'name' has a leading symlink component.
  */
 int has_symlink_leading_path(int len, const char *name)
 {
 	return lstat_cache(len, name,
-			   FL_SYMLINK|FL_DIR) &
+			   FL_SYMLINK|FL_DIR, USE_ONLY_LSTAT) &
 		FL_SYMLINK;
 }
 
@@ -161,6 +177,19 @@ int has_symlink_leading_path(int len, const char *name)
 int has_symlink_or_noent_leading_path(int len, const char *name)
 {
 	return lstat_cache(len, name,
-			   FL_SYMLINK|FL_NOENT|FL_DIR) &
+			   FL_SYMLINK|FL_NOENT|FL_DIR, USE_ONLY_LSTAT) &
 		(FL_SYMLINK|FL_NOENT);
 }
+
+/* Return non-zero if all path components of 'name' exists as a
+ * directory.  If prefix_len > 0, we will test with the stat()
+ * function instead of the lstat() function for a prefix length of
+ * 'prefix_len', thus we then allow for symlink(s) in the prefix part
+ * as long as those points to real existing directories.
+ */
+int has_dirs_only_path(int len, const char *name, int prefix_len)
+{
+	return lstat_cache(len, name,
+			   FL_DIR|FL_FULLPATH, prefix_len) &
+		FL_DIR;
+}
-- 
1.6.0.2.GIT

^ permalink raw reply related

* [PATCH/RFC v7 4/5] lstat_cache(): introduce invalidate_lstat_cache() function
From: Kjetil Barvik @ 2009-01-13 12:29 UTC (permalink / raw)
  To: git; +Cc: Junio C Hamano, Pete Harlan, Linus Torvalds, Kjetil Barvik
In-Reply-To: <1231849748-8244-1-git-send-email-barvik@broadpark.no>

In some cases it could maybe be necessary to say to the cache that
"Hey, I deleted this directory and if you currently has it inside your
cache, you should deleted it".  This patch introduce a function which
support this.

Signed-off-by: Kjetil Barvik <barvik@broadpark.no>
---
 cache.h    |    1 +
 symlinks.c |   39 +++++++++++++++++++++++++++++++--------
 2 files changed, 32 insertions(+), 8 deletions(-)

diff --git a/cache.h b/cache.h
index 7c8c8e4..f4452a8 100644
--- a/cache.h
+++ b/cache.h
@@ -722,6 +722,7 @@ extern int checkout_entry(struct cache_entry *ce, const struct checkout *state,
 extern int has_symlink_leading_path(int len, const char *name);
 extern int has_symlink_or_noent_leading_path(int len, const char *name);
 extern int has_dirs_only_path(int len, const char *name, int prefix_len);
+extern void invalidate_lstat_cache(int len, const char *name);
 
 extern struct alternate_object_database {
 	struct alternate_object_database *next;
diff --git a/symlinks.c b/symlinks.c
index f9d1821..3b1c3da 100644
--- a/symlinks.c
+++ b/symlinks.c
@@ -11,23 +11,30 @@ static struct cache_def {
 /* Returns the length (on a path component basis) of the longest
  * common prefix match of 'name' and the cached path string.
  */
-static inline int greatest_match_lstat_cache(int len, const char *name)
+static inline int greatest_match_lstat_cache(int len, const char *name,
+					     int *previous_slash)
 {
-	int max_len, match_len = 0, i = 0;
+	int max_len, match_len = 0, match_len_prev = 0, i = 0;
 
 	max_len = len < cache.len ? len : cache.len;
 	while (i < max_len && name[i] == cache.path[i]) {
-		if (name[i] == '/')
+		if (name[i] == '/') {
+			match_len_prev = match_len;
 			match_len = i;
+		}
 		i++;
 	}
 	/* Is the cached path string a substring of 'name'? */
-	if (i == cache.len && len > cache.len && name[cache.len] == '/')
+	if (i == cache.len && len > cache.len && name[cache.len] == '/') {
+		match_len_prev = match_len;
 		match_len = cache.len;
 	/* Is 'name' a substring of the cached path string? */
-	else if ((i == len && len < cache.len && cache.path[len] == '/') ||
-		 (i == len && len == cache.len))
+	} else if ((i == len && len < cache.len && cache.path[len] == '/') ||
+		   (i == len && len == cache.len)) {
+		match_len_prev = match_len;
 		match_len = len;
+	}
+	*previous_slash = match_len_prev;
 	return match_len;
 }
 
@@ -62,7 +69,7 @@ static inline void reset_lstat_cache(int track_flags, int prefix_len_stat_func)
 static int lstat_cache(int len, const char *name,
 		       int track_flags, int prefix_len_stat_func)
 {
-	int match_len, last_slash, last_slash_dir;
+	int match_len, last_slash, last_slash_dir, previous_slash;
 	int match_flags, ret_flags, save_flags, max_len, ret;
 	struct stat st;
 
@@ -80,7 +87,8 @@ static int lstat_cache(int len, const char *name,
 		 * Check to see if we have a match from the cache for
 		 * the 2 "excluding" path types.
 		 */
-		match_len = last_slash = greatest_match_lstat_cache(len, name);
+		match_len = last_slash =
+			greatest_match_lstat_cache(len, name, &previous_slash);
 		match_flags = cache.flags & track_flags & (FL_NOENT|FL_SYMLINK);
 		if (match_flags && match_len == cache.len)
 			return match_flags;
@@ -160,6 +168,21 @@ static int lstat_cache(int len, const char *name,
 	return ret_flags;
 }
 
+/* Invalidate the given 'name' from the cache, if 'name' matches
+ * completely with the cache.
+ */
+void invalidate_lstat_cache(int len, const char *name)
+{
+	int match_len, previous_slash;
+
+	match_len = greatest_match_lstat_cache(len, name, &previous_slash);
+	if (match_len == len) {
+		cache.path[previous_slash] = '\0';
+		cache.len   = previous_slash;
+		cache.flags = previous_slash ? FL_DIR : 0;
+	}
+}
+
 #define USE_ONLY_LSTAT  0
 
 /* Return non-zero if path 'name' has a leading symlink component.
-- 
1.6.0.2.GIT

^ permalink raw reply related

* [PATCH/RFC v7 5/5] lstat_cache(): introduce clear_lstat_cache() function
From: Kjetil Barvik @ 2009-01-13 12:29 UTC (permalink / raw)
  To: git; +Cc: Junio C Hamano, Pete Harlan, Linus Torvalds, Kjetil Barvik
In-Reply-To: <1231849748-8244-1-git-send-email-barvik@broadpark.no>

If you want to completely clear the contents of the lstat_cache(), then
call this new function.

Signed-off-by: Kjetil Barvik <barvik@broadpark.no>
---
 cache.h    |    1 +
 symlinks.c |    7 +++++++
 2 files changed, 8 insertions(+), 0 deletions(-)

diff --git a/cache.h b/cache.h
index f4452a8..468daf6 100644
--- a/cache.h
+++ b/cache.h
@@ -723,6 +723,7 @@ extern int has_symlink_leading_path(int len, const char *name);
 extern int has_symlink_or_noent_leading_path(int len, const char *name);
 extern int has_dirs_only_path(int len, const char *name, int prefix_len);
 extern void invalidate_lstat_cache(int len, const char *name);
+extern void clear_lstat_cache(void);
 
 extern struct alternate_object_database {
 	struct alternate_object_database *next;
diff --git a/symlinks.c b/symlinks.c
index 3b1c3da..7a634cb 100644
--- a/symlinks.c
+++ b/symlinks.c
@@ -183,6 +183,13 @@ void invalidate_lstat_cache(int len, const char *name)
 	}
 }
 
+/* Completely clear the contents of the cache!
+ */
+void clear_lstat_cache(void)
+{
+	reset_lstat_cache(0, 0);
+}
+
 #define USE_ONLY_LSTAT  0
 
 /* Return non-zero if path 'name' has a leading symlink component.
-- 
1.6.0.2.GIT

^ permalink raw reply related

* Re: How to pull remote branch with specified commit id?
From: Emily Ren @ 2009-01-13 13:15 UTC (permalink / raw)
  To: Andreas Ericsson; +Cc: Git Mailinglist
In-Reply-To: <496C5BE3.2040206@op5.se>

Andreas,

I tried your method, it works. Thank you very much !

Emily

On Tue, Jan 13, 2009 at 5:16 PM, Andreas Ericsson <ae@op5.se> wrote:
> Emily Ren wrote:
>>
>> Git experts,
>>
>> I want to pull remote branch with specified commit id, how to do it?
>>
>> Below command can get remote branch
>> $git pull remote refs/heads/$branch_name
>>
>> Below command doesn't work
>> $git pull remote objects/$commit_id
>>
>
> You need to fetch it first, and then merge the commit you want. The
> tools operating the fetching protocol only use refs, so if you want
> to fetch (or pull) a specific version that has neither a tag nor a
> branch head pointing to it, you'll have to write a new tool for that.
>
> The end-result of the following command will be, barring side-effects
> in the remote-tracking branches, identical to what you're trying to
> do though:
> git fetch remote && git merge $commit_id
>
> --
> Andreas Ericsson                   andreas.ericsson@op5.se
> OP5 AB                             www.op5.se
> Tel: +46 8-230225                  Fax: +46 8-230231
>

^ permalink raw reply

* gitignore excludes too much
From: Jan Engelhardt @ 2009-01-13 13:30 UTC (permalink / raw)
  To: git

Hi,


I noticed that having "*.d" in .gitignore ignores files that would start 
with a dot, such as ".main.o.d". This is against Unix shell behavior;
but maybe it's a feature rather than a bug?
In case of latter, please fix :-)


Jan

^ permalink raw reply

* Re: [PATCH,v2] contrib/examples/README: give an explanation of the status of these files
From: Andreas Ericsson @ 2009-01-13 13:43 UTC (permalink / raw)
  To: Thomas Adam; +Cc: jidanni, gitster, git
In-Reply-To: <18071eea0901130201r41c576e6t7f8c9fda7259e9f2@mail.gmail.com>

Thomas Adam wrote:
> 2009/1/13  <jidanni@jidanni.org>:
>> @@ -0,0 +1,3 @@
>> +These are original scripted implementations, kept primarily for their
>> +reference value to any aspiring plumbing users who want to learn how
>> +pieces can be fit together.
> 
> "... who want to learn how the pieces fit together" is the correct
> phrasing.  The above is incorrect.
> 

Actually, "can be fitted together" would be correct, as we're not
talking about a jigsaw puzzle with just one solution, but rather
a few possible ways of building something out of the lego-style
programs that make up git.

-- 
Andreas Ericsson                   andreas.ericsson@op5.se
OP5 AB                             www.op5.se
Tel: +46 8-230225                  Fax: +46 8-230231

^ permalink raw reply

* git-svn fails to fetch repository
From: Vladimir Pouzanov @ 2009-01-13 14:53 UTC (permalink / raw)
  To: git

Hi all,

I'm trying to fetch svn repository:
git svn clone http://qsb-mac.googlecode.com/svn/trunk qsb-mac

This one fails at random time at:
Temp file with moniker ' at /opt/local/lib/perl5/site_perl/5.8.8/Git.pm
line 1011.

I know nothing about perl, so can't make anything out of that. Any hints?

Running Git 1.6.1, perl 5.8.8, OSX 10.5.6

PS: Please CC me on answer.

^ permalink raw reply

* Re: git-svn fails to fetch repository
From: Vladimir Pouzanov @ 2009-01-13 15:03 UTC (permalink / raw)
  To: git
In-Reply-To: <loom.20090113T145019-951@post.gmane.org>

Also just tried on linux box (git 1.6.0.4, perl 5.8.8). Got all the revisions
but git segfaulted at the end:
...
r21 = e839272549fd746cf45542a7aa6cb151ae3813da (trunk)
Checked out HEAD:
  http://qsb-mac.googlecode.com/svn/trunk r21
Segmentation fault

^ permalink raw reply

* Re: [PATCH] Simplest update to bash completions to prevent unbounded variable errors
From: Shawn O. Pearce @ 2009-01-13 15:20 UTC (permalink / raw)
  To: Ted Pavlic; +Cc: git, Junio C Hamano
In-Reply-To: <496C1F5F.9020604@tedpavlic.com>

Ted Pavlic <ted@tedpavlic.com> wrote:
> Another try at fixing bash completions in "set -u" environments.

I agree with Junio; setting -u in your interactive shell is as bad
as export CDPATH.  Its crazy.

> Additionally added some comments and omitted things like Vim modelines.

These are orthogonal to the -u corrections.  They should be in a
different patch.  The comments are wecome.  The '#!bash' looks like
a good idea.  But a vim specific modeline, I don't like, for the
reasons Junio has already stated.

> +# __gitdir accepts 0 or 1 arguments (i.e., location)
> +# returns location of .git repo
>  __gitdir ()
>  {
> -	if [ -z "$1" ]; then
> +	if [ $# -eq 0 ] || [ -z "$1" ]; then

This is one of those places where [ -z "${1-}" ] is likely easier
to read then the || usage you have introduced.  We don't care if
we got no args, or we got one that is the empty string, either way
the $1 cannot be a gitdir and we need to guess it.

> @@ -111,7 +116,7 @@ __git_ps1 ()
>  			fi
>  		fi
>
> -		if [ -n "$1" ]; then
> +		if [ $# -gt 0 ] && [ -n "$1" ]; then
>  			printf "$1" "${b##refs/heads/}$r"

Eh, I'd rather see [ -n "${1-}" ] over the && test.

> -complete -o default -o nospace -F _git git
> -complete -o default -o nospace -F _gitk gitk
> +complete -o bashdefault -o default -o nospace -F _git git 2>/dev/null \
> +	|| complete -o default -o nospace -F _git git
> +complete -o bashdefault -o default -o nospace -F _gitk gitk 2>/dev/null \
> +	|| complete -o default -o nospace -F _gitk gitk

Why are we switching to bashdefault?  Is this an unrelated change
from the -u stuff and should go into its own commit, with its own
justification?

-- 
Shawn.

^ permalink raw reply

* Re: [PATCH] Simplest update to bash completions to prevent unbounded variable errors
From: Ted Pavlic @ 2009-01-13 15:30 UTC (permalink / raw)
  To: Shawn O. Pearce; +Cc: git, Junio C Hamano
In-Reply-To: <20090113152047.GO10179@spearce.org>

>> Another try at fixing bash completions in "set -u" environments.
> I agree with Junio; setting -u in your interactive shell is as bad
> as export CDPATH.  Its crazy.

This whole series of patches was inspired by a group of workstations at 
a university that set -u by default for all users.

Additionally, doesn't "set -u" make tcsh users feel more at home in 
bash? Certainly other shells have this same behavior in their 
interactive modes.

>> Additionally added some comments and omitted things like Vim modelines.
>
> These are orthogonal to the -u corrections.  They should be in a
> different patch.  The comments are wecome.  The '#!bash' looks like
> a good idea.  But a vim specific modeline, I don't like, for the
> reasons Junio has already stated.

OK. Can do.

>> -	if [ -z "$1" ]; then
>> +	if [ $# -eq 0 ] || [ -z "$1" ]; then
>
> This is one of those places where [ -z "${1-}" ] is likely easier

That was a mistake. I missed that hunk. I meant to use the ${1-}.

>> +		if [ $# -gt 0 ]&&  [ -n "$1" ]; then
>
> Eh, I'd rather see [ -n "${1-}" ] over the&&  test.

Again, my mistake. It was late and I missed it.


>> +complete -o bashdefault -o default -o nospace -F _git git 2>/dev/null \
>> +	|| complete -o default -o nospace -F _git git
>> +complete -o bashdefault -o default -o nospace -F _gitk gitk 2>/dev/null \
>> +	|| complete -o default -o nospace -F _gitk gitk
>
> Why are we switching to bashdefault?  Is this an unrelated change
> from the -u stuff and should go into its own commit, with its own
> justification?

Ok.

 From what I understand, normal bash completion is like setting "-o 
bashdefault -o default". That is, it tries the bash completions first 
before going to the filename completion. This change makes it so that 
git jumps back to bash completion if nothing git-specific is found. If 
nothing bash-specific is found, it will go back to standard default 
filename completion.

--Ted



-- 
Ted Pavlic <ted@tedpavlic.com>

   Please visit my ALS association page:
         http://web.alsa.org/goto/tedpavlic
   My family appreciates your support in the fight to defeat ALS.

^ permalink raw reply

* Re: [PATCH] Simplest update to bash completions to prevent unbounded variable errors
From: Shawn O. Pearce @ 2009-01-13 15:33 UTC (permalink / raw)
  To: Ted Pavlic; +Cc: git, Junio C Hamano
In-Reply-To: <496CB3B0.7010605@tedpavlic.com>

Ted Pavlic <ted@tedpavlic.com> wrote:
>>> Another try at fixing bash completions in "set -u" environments.
>> I agree with Junio; setting -u in your interactive shell is as bad
>> as export CDPATH.  Its crazy.
>
> This whole series of patches was inspired by a group of workstations at  
> a university that set -u by default for all users.

The changes look less nasty than I originally thought.  If you can
split the history out and justify the changes in the corresponding
commit messages, I think I can ACK the series.

-- 
Shawn.

^ permalink raw reply

* Re: git-patches list?
From: Akira Kitada @ 2009-01-13 15:55 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git
In-Reply-To: <7vr638f5ch.fsf@gitster.siamese.dyndns.org>

This list seems to be used as a bug tracker, discussing git development,
user questions, announcement etc.
I thought this is so unusual list that something should be wrong here,
but it turned out that it looks working right in mysteryous way.
I take that back now.
Hmm, interesting...

Thanks,

On Tue, Jan 13, 2009 at 8:54 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Akira Kitada <akitada@gmail.com> writes:
>
>> Can I propose having another mailing list for posting patches to avoid
>> daily mail flood to this list?
>>
>> Yes, I can filter out the emails but still...
>
> This list has always been the only place where git development happens.
> It would make the development very awkward to set up another list only for
> patches, forbid patches to be sent to anywhere but that new list, but
> still discuss the patches on this list.
>
> It does not make much sense to me.
>
> Consider what you would do when you see a problem somebody is having on
> this list, and wanted to respond with a quick "this may fix it" patch?
> Should you be sending that to the patches list, and sending a separate
> message to this list saying that you have a potential fix in mind you
> would want to discuss here, but the patches list rule mandated that you
> had to send the patch to the other list, asking people who are reading
> this list to look at the other list as well?
>
> And no, having a separate "user list" won't solve the above problem either
> and that is not what I am suggesting.
>
> Not that I am seeing any problem right now; I am saying that the split
> list as you suggest _will_ create problems.
>

^ permalink raw reply

* [PATCH 1/3] Purest update to bash completions to prevent unbounded variable errors.
From: Ted Pavlic @ 2009-01-13 16:08 UTC (permalink / raw)
  To: Shawn O. Pearce; +Cc: git, Junio C Hamano


First in a series of patches that make bash completions more robust to
different interactive shell configurations and editors.


Signed-off-by: Ted Pavlic <ted@tedpavlic.com>
---
  contrib/completion/git-completion.bash |   18 +++++++++---------
  1 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/contrib/completion/git-completion.bash 
b/contrib/completion/git-completion.bash
index 7b074d7..5d1515c 100755
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -52,7 +52,7 @@ esac

  __gitdir ()
  {
-	if [ -z "$1" ]; then
+	if [ -z "${1-}" ]; then
  		if [ -n "$__git_dir" ]; then
  			echo "$__git_dir"
  		elif [ -d .git ]; then
@@ -111,7 +111,7 @@ __git_ps1 ()
  			fi
  		fi

-		if [ -n "$1" ]; then
+		if [ -n "${1-}" ]; then
  			printf "$1" "${b##refs/heads/}$r"
  		else
  			printf " (%s)" "${b##refs/heads/}$r"
@@ -143,8 +143,8 @@ __gitcomp ()
  		;;
  	*)
  		local IFS=$'\n'
-		COMPREPLY=($(compgen -P "$2" \
-			-W "$(__gitcomp_1 "$1" "$4")" \
+		COMPREPLY=($(compgen -P "${2-}" \
+			-W "$(__gitcomp_1 "${1-}" "${4-}")" \
  			-- "$cur"))
  		;;
  	esac
@@ -152,13 +152,13 @@ __gitcomp ()

  __git_heads ()
  {
-	local cmd i is_hash=y dir="$(__gitdir "$1")"
+	local cmd i is_hash=y dir="$(__gitdir "${1-}")"
  	if [ -d "$dir" ]; then
  		git --git-dir="$dir" for-each-ref --format='%(refname:short)' \
  			refs/heads
  		return
  	fi
-	for i in $(git ls-remote "$1" 2>/dev/null); do
+	for i in $(git ls-remote "${1-}" 2>/dev/null); do
  		case "$is_hash,$i" in
  		y,*) is_hash=n ;;
  		n,*^{}) is_hash=y ;;
@@ -170,13 +170,13 @@ __git_heads ()

  __git_tags ()
  {
-	local cmd i is_hash=y dir="$(__gitdir "$1")"
+	local cmd i is_hash=y dir="$(__gitdir "${1-}")"
  	if [ -d "$dir" ]; then
  		git --git-dir="$dir" for-each-ref --format='%(refname:short)' \
  			refs/tags
  		return
  	fi
-	for i in $(git ls-remote "$1" 2>/dev/null); do
+	for i in $(git ls-remote "${1-}" 2>/dev/null); do
  		case "$is_hash,$i" in
  		y,*) is_hash=n ;;
  		n,*^{}) is_hash=y ;;
@@ -188,7 +188,7 @@ __git_tags ()

  __git_refs ()
  {
-	local i is_hash=y dir="$(__gitdir "$1")"
+	local i is_hash=y dir="$(__gitdir "${1-}")"
  	local cur="${COMP_WORDS[COMP_CWORD]}" format refs
  	if [ -d "$dir" ]; then
  		case "$cur" in
-- 
1.6.1.87.g15624

^ permalink raw reply related

* [PATCH 2/3] Makes the bash completion script try *bash* completions before simple, filetype completions when a git completion is not found. If bash, completions aren't available, the default file completions are used. This, behavior was inspired by Mercurial's bash completion script.
From: Ted Pavlic @ 2009-01-13 16:10 UTC (permalink / raw)
  To: Shawn O. Pearce; +Cc: git, Junio C Hamano


Second in a series of patches that make bash completions more robust to
different interactive shell configurations and editors.


[PATCH 2/3] Makes the bash completion script try *bash* completions 
before simple
  filetype completions when a git completion is not found. If bash
  completions aren't available, the default file completions are used. This
  behavior was inspired by Mercurial's bash completion script.


Signed-off-by: Ted Pavlic <ted@tedpavlic.com>
---
  contrib/completion/git-completion.bash |    9 ++++++---
  1 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/contrib/completion/git-completion.bash 
b/contrib/completion/git-completion.bash
index 5d1515c..201f9a6 100755
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -1766,13 +1766,16 @@ _gitk ()
  	__git_complete_revlist
  }

-complete -o default -o nospace -F _git git
-complete -o default -o nospace -F _gitk gitk
+complete -o bashdefault -o default -o nospace -F _git git 2>/dev/null \
+	|| complete -o default -o nospace -F _git git
+complete -o bashdefault -o default -o nospace -F _gitk gitk 2>/dev/null \
+	|| complete -o default -o nospace -F _gitk gitk

  # The following are necessary only for Cygwin, and only are needed
  # when the user has tab-completed the executable name and consequently
  # included the '.exe' suffix.
  #
  if [ Cygwin = "$(uname -o 2>/dev/null)" ]; then
-complete -o default -o nospace -F _git git.exe
+complete -o bashdefault -o default -o nospace -F _git git.exe 2>/dev/null \
+	|| complete -o default -o nospace -F _git git.exe
  fi
-- 
1.6.1.87.g15624

^ permalink raw reply related

* [PATCH 3/3] Adds a #!bash to the top of bash completions so that editors can recognize, it as a bash script. Also adds a few simple comments above commands that, take arguments. The comments are meant to remind editors of potential, problems that can occur when the script is sourced on systems with "set, -u."
From: Ted Pavlic @ 2009-01-13 16:11 UTC (permalink / raw)
  To: Shawn O. Pearce; +Cc: git, Junio C Hamano


Third in a series of patches that make bash completions more robust to
different interactive shell configurations and editors.

[PATCH 3/3] Adds a #!bash to the top of bash completions so that editors 
can recognize
  it as a bash script. Also adds a few simple comments above commands that
  take arguments. The comments are meant to remind editors of potential
  problems that can occur when the script is sourced on systems with "set
  -u."

Signed-off-by: Ted Pavlic <ted@tedpavlic.com>
---
  contrib/completion/git-completion.bash |   15 +++++++++++++++
  1 files changed, 15 insertions(+), 0 deletions(-)

diff --git a/contrib/completion/git-completion.bash 
b/contrib/completion/git-completion.bash
index 201f9a6..f8b845a 100755
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -1,3 +1,4 @@
+#!bash
  #
  # bash completion support for core Git.
  #
@@ -50,6 +51,8 @@ case "$COMP_WORDBREAKS" in
  *)   COMP_WORDBREAKS="$COMP_WORDBREAKS:"
  esac

+# __gitdir accepts 0 or 1 arguments (i.e., location)
+# returns location of .git repo
  __gitdir ()
  {
  	if [ -z "${1-}" ]; then
@@ -67,6 +70,8 @@ __gitdir ()
  	fi
  }

+# __git_ps1 accepts 0 or 1 arguments (i.e., format string)
+# returns text to add to bash PS1 prompt (includes branch name)
  __git_ps1 ()
  {
  	local g="$(git rev-parse --git-dir 2>/dev/null)"
@@ -119,6 +124,7 @@ __git_ps1 ()
  	fi
  }

+# __gitcomp_1 requires 2 arguments
  __gitcomp_1 ()
  {
  	local c IFS=' '$'\t'$'\n'
@@ -131,6 +137,8 @@ __gitcomp_1 ()
  	done
  }

+# __gitcomp accepts 1, 2, 3, or 4 arguments
+# generates completion reply with compgen
  __gitcomp ()
  {
  	local cur="${COMP_WORDS[COMP_CWORD]}"
@@ -150,6 +158,7 @@ __gitcomp ()
  	esac
  }

+# __git_heads accepts 0 or 1 arguments (to pass to __gitdir)
  __git_heads ()
  {
  	local cmd i is_hash=y dir="$(__gitdir "${1-}")"
@@ -168,6 +177,7 @@ __git_heads ()
  	done
  }

+# __git_tags accepts 0 or 1 arguments (to pass to __gitdir)
  __git_tags ()
  {
  	local cmd i is_hash=y dir="$(__gitdir "${1-}")"
@@ -186,6 +196,7 @@ __git_tags ()
  	done
  }

+# __git_refs accepts 0 or 1 arguments (to pass to __gitdir)
  __git_refs ()
  {
  	local i is_hash=y dir="$(__gitdir "${1-}")"
@@ -218,6 +229,7 @@ __git_refs ()
  	done
  }

+# __git_refs2 requires 1 argument (to pass to __git_refs)
  __git_refs2 ()
  {
  	local i
@@ -226,6 +238,7 @@ __git_refs2 ()
  	done
  }

+# __git_refs_remotes requires 1 argument (to pass to ls-remote)
  __git_refs_remotes ()
  {
  	local cmd i is_hash=y
@@ -470,6 +483,7 @@ __git_aliases ()
  	done
  }

+# __git_aliased_command requires 1 argument
  __git_aliased_command ()
  {
  	local word cmdline=$(git --git-dir="$(__gitdir)" \
@@ -482,6 +496,7 @@ __git_aliased_command ()
  	done
  }

+# __git_find_subcommand requires 1 argument
  __git_find_subcommand ()
  {
  	local word subcommand c=1
-- 
1.6.1.87.g15624

^ permalink raw reply related

* Re: [PATCH 1/3] Purest update to bash completions to prevent unbounded variable errors.
From: Shawn O. Pearce @ 2009-01-13 16:34 UTC (permalink / raw)
  To: Ted Pavlic; +Cc: git, Junio C Hamano
In-Reply-To: <496CBC98.7090101@tedpavlic.com>

Ted Pavlic <ted@tedpavlic.com> wrote:
>
> First in a series of patches that make bash completions more robust to
> different interactive shell configurations and editors.
>
>
> Signed-off-by: Ted Pavlic <ted@tedpavlic.com>

Your commit message leaves a lot to be desired.  I would instead
have written something like this:

  bash-completion: Support running when set -u is enabled

  Under set -u semenatics it is an error to access undefined
  variables.  Some user environments may enable this in the
  interactive shell.

  In any context where the completion functions access an undefined
  variable, accessing a default empty string (aka "${1-}" instead of
  "$1") is a reasonable way to code the function, as it silences
  the undefined variable error while still supplying an empty string.

Acked-by: Shawn O. Pearce <spearce@spearce.org>

> ---
>  contrib/completion/git-completion.bash |   18 +++++++++---------
>  1 files changed, 9 insertions(+), 9 deletions(-)

-- 
Shawn.

^ permalink raw reply

* Re: [PATCH 2/3] Makes the bash completion script try *bash* completions before simple, filetype completions when a git completion is not found. If bash, completions aren't available, the default file completions are used. This, behavior was inspired by Mercurial's bash completion script.
From: Shawn O. Pearce @ 2009-01-13 16:38 UTC (permalink / raw)
  To: Ted Pavlic; +Cc: git, Junio C Hamano
In-Reply-To: <496CBCED.80402@tedpavlic.com>

Ted Pavlic <ted@tedpavlic.com> wrote:
>
> Second in a series of patches that make bash completions more robust to
> different interactive shell configurations and editors.
>
>
> [PATCH 2/3] Makes the bash completion script try *bash* completions  
> before simple
>  filetype completions when a git completion is not found. If bash
>  completions aren't available, the default file completions are used. This
>  behavior was inspired by Mercurial's bash completion script.

Again, I would have used this as my commit message:

	bash-completion: Try bash completions before file completions

	Try bash completions before any simple file completions
	whenever a git completion is not found.  This may help
	users to complete BLAH BLAH BLAH WHAT THE HECK IS THIS GOOD
	FOR ANYWAY.

	Behavior was inspired by Mercurial's bash completion script.

No ack, because I still don't understand why this is a good thing.
Yes, I could look it up online in the bash docs.  I shouldn't need
to go do research like that to understand the justification for
a change, it should be better explained in the message.

-- 
Shawn.

^ permalink raw reply

* Re: [PATCH 1/3] Purest update to bash completions to prevent unbounded variable errors.
From: Ted Pavlic @ 2009-01-13 16:40 UTC (permalink / raw)
  To: Shawn O. Pearce; +Cc: git, Junio C Hamano
In-Reply-To: <20090113163421.GQ10179@spearce.org>

> Your commit message leaves a lot to be desired.  I would instead
> have written something like this:

I agree completely. Sorry about that. stg fired off vim to edit the 
commit message, and I just wasn't thinking.

Would you like me to modify the commit message and resubmit?

--Ted

-- 
Ted Pavlic <ted@tedpavlic.com>

   Please visit my ALS association page:
         http://web.alsa.org/goto/tedpavlic
   My family appreciates your support in the fight to defeat ALS.

^ permalink raw reply

* stg coalesce -> squash?
From: Karl Hasselström @ 2009-01-13 16:41 UTC (permalink / raw)
  To: Catalin Marinas; +Cc: git

Would it be a good idea to rename "stg coalesce" to "stg squash"?
That's the term git uses, and it's shorter, easier to spell, and
easier to say. coalesce is still not in any released version of StGit,
so now would be a good time to do it if we wanted to.

-- 
Karl Hasselström, kha@treskal.com
      www.treskal.com/kalle

^ permalink raw reply

* Re: [PATCH 3/3] Adds a #!bash to the top of bash completions so that editors can recognize, it as a bash script. Also adds a few simple comments above commands that, take arguments. The comments are meant to remind editors of potential, problems that can occur when the script is sourced on systems with "set, -u."
From: Shawn O. Pearce @ 2009-01-13 16:45 UTC (permalink / raw)
  To: Ted Pavlic; +Cc: git, Junio C Hamano
In-Reply-To: <496CBD17.3000207@tedpavlic.com>

Ted Pavlic <ted@tedpavlic.com> wrote:
>
> Third in a series of patches that make bash completions more robust to
> different interactive shell configurations and editors.
>
> [PATCH 3/3] Adds a #!bash to the top of bash completions so that editors  
> can recognize
>  it as a bash script. Also adds a few simple comments above commands that
>  take arguments. The comments are meant to remind editors of potential
>  problems that can occur when the script is sourced on systems with "set
>  -u."

Aside from the message format... OK.  The message really should
have looked like this from an mbox point of view:

	From: Ted Pavlic <ted@tedpavlic.com>
	To: git <git@vger.kernel.org>, Junio C Hamano <gitster@pobox.com>
	Cc: "Shawn O. Pearce" <spearce@spearce.org>
	Bcc: 
	Subject: [PATCH 3/3] bash-completion: Add internal function documentation

	Slightly document the internal functions of the bash
	completion package, so callers are more easily able to
	determine the expected arguments.

	Signed-off-by: Ted Pavlic <ted@tedpavlic.com>
	---

	 Third in a series to improve the bash completion package,
	 so it sucks less.

	 contrib/completion/git-completion.bash |   15 +++++++++++++++
	 1 files changed, 15 insertions(+), 0 deletions(-)

	diff --git a/contrib/completion/git-completion.bash  

See how the stuff that doesn't matter to the commit message itself
goes after the "---" line?  And how the subject is a niceshort, one
line summary of the module impacted and the change?  These show up in
gitk and git shortlog, and thus in the "What's changed in git.git"
newsletters Junio publishes.  Its important that the subject be
really short and sweet.  You can put more detail above the "---"
line, and it will be included in the commit when Junio applies it.

This is all based on the formatting at the time of commit.
Anything up to the first "\n\n" in a commit message goes into the
email subject line.  The rest goes into the email body, but above the
"---" line.  You can then edit the buffer before sending to insert
non-commit message text after the "---" and before the diff stat.

You can include my Ack'd by line below your Signed-off-by when you
resend it.

Acked-by: Shawn O. Pearce <spearce@spearce.org>

> ---
>  contrib/completion/git-completion.bash |   15 +++++++++++++++
>  1 files changed, 15 insertions(+), 0 deletions(-)

-- 
Shawn.

^ permalink raw reply


This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox