[PATCH/RFC v1 0/6] git checkout: more cleanups, optimisation, less lstat() calls

[PATCH/RFC v1 0/6] git checkout: more cleanups, optimisation, less lstat() calls

[Kjetil Barvik]

Here is 6 small patches which should further improve the time it take
to do 'git checkout'.  After the 6 patches, the numbers from the 'git
checkout -q my-v2.6.27' test, should now look like this:

TOTAL        136752 100.000% OK:124567 NOT: 12185   8.641698 sec   63 usec/call
lstat64       55502  40.586% OK: 49122 NOT:  6380   2.463762 sec   44 usec/call
    strings   55502 tot  30163 uniq   1.840 /uniq   2.463762 sec   44 usec/call
    files     47122 tot  23813 uniq   1.979 /uniq   2.070486 sec   44 usec/call
    dirs       2000 tot   1436 uniq   1.393 /uniq   0.087281 sec   44 usec/call
    errors     6380 tot   5187 uniq   1.230 /uniq   0.305995 sec   48 usec/call
                  4   0.007% OK:     3 NOT:     1  "arch/sh/boards"
                  3   0.005% OK:     3 NOT:     0  ".git/HEAD"
                  3   0.005% OK:     3 NOT:     0  ".git/refs/heads/my-v2.6.27"
                  3   0.005% OK:     3 NOT:     0  ".gitignore"
                  3   0.005% OK:     3 NOT:     0  ".mailmap"
                  3   0.005% OK:     3 NOT:     0  "CREDITS"
                  3   0.005% OK:     3 NOT:     0  "Documentation"
                  3   0.005% OK:     3 NOT:     0  "Documentation/00-INDEX"
                  3   0.005% OK:     3 NOT:     0  "Documentation/ABI/testing/sysfs-block"
                  3   0.005% OK:     3 NOT:     0  "Documentation/ABI/testing/sysfs-firmware-acpi"
                  3   0.005% OK:     3 NOT:     0  "Documentation/CodingStyle"
                  3   0.005% OK:     3 NOT:     0  "Documentation/DMA-API.txt"
                  3   0.005% OK:     3 NOT:     0  "Documentation/DMA-mapping.txt"
                  3   0.005% OK:     3 NOT:     0  "Documentation/DocBook/Makefile"
                  3   0.005% OK:     3 NOT:     0  "Documentation/DocBook/gadget.tmpl"
                  3   0.005% OK:     3 NOT:     0  "Documentation/DocBook/kernel-api.tmpl"
                  3   0.005% OK:     3 NOT:     0  "Documentation/DocBook/kernel-locking.tmpl"
                  3   0.005% OK:     3 NOT:     0  "Documentation/DocBook/procfs-guide.tmpl"
                  3   0.005% OK:     3 NOT:     0  "Documentation/DocBook/procfs_example.c"
                  3   0.005% OK:     3 NOT:     0  "Documentation/DocBook/rapidio.tmpl"
<snipp>
fstat64       14403  10.532% OK: 14403 NOT:     0   0.179938 sec   12 usec/call

So, since last time, and because of patch 4/6, almost 14 400 of the
lstat() calls has now become fstat() calls, and it seems we have saved
(* 14403 (- 44 12)) = 460 896 microseconds system time because of
this.  I am planing to do a more complete and long-running
/usr/bin/time test, to get real numbers.

With both patch-series, the count of lstat() calls for this particular
test have dropped from 120 954 to 55 502, which is a total reduction
of 65 452 calls or 54%.

Please note that patch 6/6 is only to be a debug patch, to catch a
possible ping-pong situation inside the lstat_cache(), so I think that
it should at _most_ be merged to the pu branch, such that people who
wish to test, can 'git cherry-pick' the patch from there.


Kjetil Barvik (6):
  symlinks.c: small cleanup and optimisation
  remove some memcpy() and strchr() calls inside create_directories()
  cleanup of write_entry() in entry.c
  use fstat() instead of lstat() when we have an opened file
  combine-diff.c: remove a call to fstat() inside show_patch_diff()
  lstat_cache(): print a warning if doing ping-pong between cache types

 combine-diff.c |    5 +-
 entry.c        |  144 ++++++++++++++++++++++++++++++--------------------------
 symlinks.c     |   48 ++++++++++++++-----
 3 files changed, 115 insertions(+), 82 deletions(-)

[PATCH/RFC v1 1/6] symlinks.c: small cleanup and optimisation

[Kjetil Barvik]

Simplify the if-else test in longest_match_lstat_cache() such that we
only have one simple if test.  Instead of testing for 'i == cache.len'
or 'i == len', we transform this to a common test for 'i == max_len'.

And to further optimise we use 'i >= max_len' instead of 'i ==
max_len', the reason is that it is now the exact opposite of one part
inside the while-loop termination expression 'i < max_len && name[i]
== cache.path[i]', and then the compiler can hopefully/probably reuse
a test-result from it.

We do similar transformations inside the lstat_cache() and the
invalidate_lstat_cache() functions.

The result is a little smaller text-size as shown below:

 ~/git/git $ size symlinks_??_*
   text	   data	    bss	    dec	    hex	filename
   1282       0	   4116	   5398	   1516	symlinks_O2_after.o
   1378       0	   4116	   5494	   1576	symlinks_O2_before.o
    896       0	   4116	   5012	   1394	symlinks_Os_after.o
    902       0	   4116	   5018	   139a	symlinks_Os_before.o

'O2' means that the file is compiled with 'gcc -O2', and similar 'Os'
means that the file is compiled with 'gcc -Os' (gcc 4.3.2).

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

diff --git a/symlinks.c b/symlinks.c
index f262b7c..81f490c 100644
--- a/symlinks.c
+++ b/symlinks.c
@@ -25,15 +25,16 @@ static inline int longest_match_lstat_cache(int len, const char *name,
 		}
 		i++;
 	}
-	/* Is the cached path string a substring of 'name'? */
-	if (i == cache.len && cache.len < 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)) {
+	/*
+	 * Is the cached path string a substring of 'name', is 'name'
+	 * a substring of the cached path string, or is 'name' and the
+	 * cached path string the exact same string?
+	 */
+	if (i >= max_len && ((i < len && name[i] == '/') ||
+			     (i < cache.len && cache.path[i] == '/') ||
+			     (len == cache.len))) {
 		match_len_prev = match_len;
-		match_len = len;
+		match_len = i;
 	}
 	*previous_slash = match_len_prev;
 	return match_len;
@@ -91,7 +92,7 @@ static int lstat_cache(int len, const char *name,
 		match_len = last_slash =
 			longest_match_lstat_cache(len, name, &previous_slash);
 		match_flags = cache.flags & track_flags & (FL_NOENT|FL_SYMLINK);
-		if (match_flags && match_len == cache.len)
+		if (match_flags && match_len >= cache.len)
 			return match_flags;
 		/*
 		 * If we now have match_len > 0, we would know that
@@ -102,7 +103,7 @@ static int lstat_cache(int len, const char *name,
 		 * we can return immediately.
 		 */
 		match_flags = track_flags & FL_DIR;
-		if (match_flags && len == match_len)
+		if (match_flags && match_len >= len)
 			return match_flags;
 	}
 
@@ -153,7 +154,7 @@ static int lstat_cache(int len, const char *name,
 		cache.path[last_slash] = '\0';
 		cache.len = last_slash;
 		cache.flags = save_flags;
-	} else if (track_flags & FL_DIR &&
+	} else if ((track_flags & FL_DIR) &&
 		   last_slash_dir > 0 && last_slash_dir <= PATH_MAX) {
 		/*
 		 * We have a separate test for the directory case,
@@ -184,7 +185,7 @@ void invalidate_lstat_cache(int len, const char *name)
 	int match_len, previous_slash;
 
 	match_len = longest_match_lstat_cache(len, name, &previous_slash);
-	if (len == match_len) {
+	if (match_len >= len) {
 		if ((cache.track_flags & FL_DIR) && previous_slash > 0) {
 			cache.path[previous_slash] = '\0';
 			cache.len = previous_slash;
-- 
1.6.1.349.g99fa5

[PATCH/RFC v1 2/6] remove some memcpy() and strchr() calls inside create_directories()

[Kjetil Barvik]

Remove the call to memcpy() and strchr() for each path component
tested, and instead add each path component as we go forward inside
the while-loop.

Impact: small optimisation

Signed-off-by: Kjetil Barvik <barvik@broadpark.no>
---

OK, maybe I instead should have tried to merge the function
create_directories() with the safe_create_leading_directories() and
*_const() functions?  What do pepople think?


 entry.c |   23 ++++++++++++++---------
 1 files changed, 14 insertions(+), 9 deletions(-)

diff --git a/entry.c b/entry.c
index 05aa58d..c2404ea 100644
--- a/entry.c
+++ b/entry.c
@@ -2,15 +2,19 @@
 #include "blob.h"
 #include "dir.h"
 
-static void create_directories(const char *path, const struct checkout *state)
+static void
+create_directories(int path_len, const char *path, const struct checkout *state)
 {
-	int len = strlen(path);
-	char *buf = xmalloc(len + 1);
-	const char *slash = path;
-
-	while ((slash = strchr(slash+1, '/')) != NULL) {
-		len = slash - path;
-		memcpy(buf, path, len);
+	char *buf = xmalloc(path_len + 1);
+	int len = 0;
+
+	while (len < path_len) {
+		do {
+			buf[len] = path[len];
+			len++;
+		} while (len < path_len && path[len] != '/');
+		if (len >= path_len)
+			break;
 		buf[len] = 0;
 
 		/*
@@ -190,6 +194,7 @@ int checkout_entry(struct cache_entry *ce, const struct checkout *state, char *t
 
 	memcpy(path, state->base_dir, len);
 	strcpy(path + len, ce->name);
+	len += ce_namelen(ce);
 
 	if (!lstat(path, &st)) {
 		unsigned changed = ce_match_stat(ce, &st, CE_MATCH_IGNORE_VALID);
@@ -218,6 +223,6 @@ int checkout_entry(struct cache_entry *ce, const struct checkout *state, char *t
 			return error("unable to unlink old '%s' (%s)", path, strerror(errno));
 	} else if (state->not_new)
 		return 0;
-	create_directories(path, state);
+	create_directories(len, path, state);
 	return write_entry(ce, path, state, 0);
 }
-- 
1.6.1.349.g99fa5

[PATCH/RFC v1 3/6] cleanup of write_entry() in entry.c

[Kjetil Barvik]

The switch-cases for S_IFREG and S_IFLNK was so similar that it will
be better to do some cleanup and use the common parts of it.

Also fold the longest lines such that no line is longer then 80 chars
or so.

And the entry.c file should now be clean for 'gcc -Wextra' warnings.

Signed-off-by: Kjetil Barvik <barvik@broadpark.no>
---

If people do not like this approach I can be willing to drop it from
this patch-series, but then I get some source code duplication from
the next patch (4/6).


 entry.c |  114 +++++++++++++++++++++++++++++++-------------------------------
 1 files changed, 57 insertions(+), 57 deletions(-)

diff --git a/entry.c b/entry.c
index c2404ea..8543755 100644
--- a/entry.c
+++ b/entry.c
@@ -78,7 +78,7 @@ static int create_file(const char *path, unsigned int mode)
 	return open(path, O_WRONLY | O_CREAT | O_EXCL, mode);
 }
 
-static void *read_blob_entry(struct cache_entry *ce, const char *path, unsigned long *size)
+static void *read_blob_entry(struct cache_entry *ce, unsigned long *size)
 {
 	enum object_type type;
 	void *new = read_sha1_file(ce->sha1, &type, size);
@@ -91,88 +91,86 @@ static void *read_blob_entry(struct cache_entry *ce, const char *path, unsigned
 	return NULL;
 }
 
-static int write_entry(struct cache_entry *ce, char *path, const struct checkout *state, int to_tempfile)
+static int
+write_entry(struct cache_entry *ce, char *path, const struct checkout *state,
+	    int to_tempfile)
 {
-	int fd;
-	long wrote;
-
-	switch (ce->ce_mode & S_IFMT) {
-		char *new;
-		struct strbuf buf;
-		unsigned long size;
-
+	unsigned int ce_mode_s_ifmt = ce->ce_mode & S_IFMT;
+	int fd, ret;
+	char *new;
+	struct strbuf buf = STRBUF_INIT;
+	unsigned long size;
+	size_t wrote, newsize = 0;
+
+	switch (ce_mode_s_ifmt) {
 	case S_IFREG:
-		new = read_blob_entry(ce, path, &size);
+	case S_IFLNK:
+		new = read_blob_entry(ce, &size);
 		if (!new)
-			return error("git checkout-index: unable to read sha1 file of %s (%s)",
-				path, sha1_to_hex(ce->sha1));
+			return error("git checkout-index: "\
+				     "unable to read sha1 file of %s (%s)",
+				     path, sha1_to_hex(ce->sha1));
+
+		if (ce_mode_s_ifmt == S_IFLNK && has_symlinks && !to_tempfile) {
+			ret = symlink(new, path);
+			free(new);
+			if (ret)
+				return error("git checkout-index: "\
+					     "unable to create symlink %s (%s)",
+					     path, strerror(errno));
+			break;
+		}
 
 		/*
 		 * Convert from git internal format to working tree format
 		 */
-		strbuf_init(&buf, 0);
-		if (convert_to_working_tree(ce->name, new, size, &buf)) {
-			size_t newsize = 0;
+		if (ce_mode_s_ifmt == S_IFREG &&
+		    convert_to_working_tree(ce->name, new, size, &buf)) {
 			free(new);
 			new = strbuf_detach(&buf, &newsize);
 			size = newsize;
 		}
 
 		if (to_tempfile) {
-			strcpy(path, ".merge_file_XXXXXX");
+			if (ce_mode_s_ifmt == S_IFREG)
+				strcpy(path, ".merge_file_XXXXXX");
+			else
+				strcpy(path, ".merge_link_XXXXXX");
 			fd = mkstemp(path);
-		} else
-			fd = create_file(path, ce->ce_mode);
+		} else {
+			if (ce_mode_s_ifmt == S_IFREG)
+				fd = create_file(path, ce->ce_mode);
+			else
+				fd = create_file(path, 0666);
+		}
 		if (fd < 0) {
 			free(new);
-			return error("git checkout-index: unable to create file %s (%s)",
-				path, strerror(errno));
+			return error("git checkout-index: "\
+				     "unable to create file %s (%s)",
+				     path, strerror(errno));
 		}
 
 		wrote = write_in_full(fd, new, size);
 		close(fd);
 		free(new);
 		if (wrote != size)
-			return error("git checkout-index: unable to write file %s", path);
-		break;
-	case S_IFLNK:
-		new = read_blob_entry(ce, path, &size);
-		if (!new)
-			return error("git checkout-index: unable to read sha1 file of %s (%s)",
-				path, sha1_to_hex(ce->sha1));
-		if (to_tempfile || !has_symlinks) {
-			if (to_tempfile) {
-				strcpy(path, ".merge_link_XXXXXX");
-				fd = mkstemp(path);
-			} else
-				fd = create_file(path, 0666);
-			if (fd < 0) {
-				free(new);
-				return error("git checkout-index: unable to create "
-						 "file %s (%s)", path, strerror(errno));
-			}
-			wrote = write_in_full(fd, new, size);
-			close(fd);
-			free(new);
-			if (wrote != size)
-				return error("git checkout-index: unable to write file %s",
-					path);
-		} else {
-			wrote = symlink(new, path);
-			free(new);
-			if (wrote)
-				return error("git checkout-index: unable to create "
-						 "symlink %s (%s)", path, strerror(errno));
-		}
+			return error("git checkout-index: "\
+				     "unable to write file %s", path);
+
 		break;
 	case S_IFGITLINK:
 		if (to_tempfile)
-			return error("git checkout-index: cannot create temporary subproject %s", path);
+			return error("git checkout-index: "\
+				     "cannot create temporary subproject %s",
+				     path);
 		if (mkdir(path, 0777) < 0)
-			return error("git checkout-index: cannot create subproject directory %s", path);
+			return error("git checkout-index: "\
+				     "cannot create subproject directory %s (%s)",
+				     path, strerror(errno));
 		break;
 	default:
-		return error("git checkout-index: unknown file mode for %s", path);
+		return error("git checkout-index: "\
+			     "unknown file mode for %s", path);
 	}
 
 	if (state->refresh_cache) {
@@ -202,7 +200,8 @@ int checkout_entry(struct cache_entry *ce, const struct checkout *state, char *t
 			return 0;
 		if (!state->force) {
 			if (!state->quiet)
-				fprintf(stderr, "git-checkout-index: %s already exists\n", path);
+				fprintf(stderr, "git-checkout-index: "\
+					"%s already exists\n", path);
 			return -1;
 		}
 
@@ -220,7 +219,8 @@ int checkout_entry(struct cache_entry *ce, const struct checkout *state, char *t
 				return error("%s is a directory", path);
 			remove_subtree(path);
 		} else if (unlink(path))
-			return error("unable to unlink old '%s' (%s)", path, strerror(errno));
+			return error("unable to unlink old '%s' (%s)",
+				     path, strerror(errno));
 	} else if (state->not_new)
 		return 0;
 	create_directories(len, path, state);
-- 
1.6.1.349.g99fa5

[PATCH/RFC v1 4/6] use fstat() instead of lstat() when we have an opened file

[Kjetil Barvik]

Currently inside write_entry() we do an lstat(path, &st) call on a
file which have just been opened inside the exact same function.  It
should be better to call fstat(fd, &st) on the file while it is
opened, and it should be at least as fast as the lstat() method.

Signed-off-by: Kjetil Barvik <barvik@broadpark.no>
---
 entry.c |   11 ++++++++---
 1 files changed, 8 insertions(+), 3 deletions(-)

diff --git a/entry.c b/entry.c
index 8543755..c932ae8 100644
--- a/entry.c
+++ b/entry.c
@@ -96,11 +96,12 @@ write_entry(struct cache_entry *ce, char *path, const struct checkout *state,
 	    int to_tempfile)
 {
 	unsigned int ce_mode_s_ifmt = ce->ce_mode & S_IFMT;
-	int fd, ret;
+	int fd, ret, fstat_done = 0;
 	char *new;
 	struct strbuf buf = STRBUF_INIT;
 	unsigned long size;
 	size_t wrote, newsize = 0;
+	struct stat st;
 
 	switch (ce_mode_s_ifmt) {
 	case S_IFREG:
@@ -151,6 +152,10 @@ write_entry(struct cache_entry *ce, char *path, const struct checkout *state,
 		}
 
 		wrote = write_in_full(fd, new, size);
+		if (state->refresh_cache) {
+			fstat(fd, &st);
+			fstat_done = 1;
+		}
 		close(fd);
 		free(new);
 		if (wrote != size)
@@ -174,8 +179,8 @@ write_entry(struct cache_entry *ce, char *path, const struct checkout *state,
 	}
 
 	if (state->refresh_cache) {
-		struct stat st;
-		lstat(ce->name, &st);
+		if (!fstat_done)
+			lstat(ce->name, &st);
 		fill_stat_cache_info(ce, &st);
 	}
 	return 0;
-- 
1.6.1.349.g99fa5

[PATCH/RFC v1 5/6] combine-diff.c: remove a call to fstat() inside show_patch_diff()

[Kjetil Barvik]

Currently inside show_patch_diff() we have and fstat() call after an
ok lstat() call.  Since we before the call to fstat() have already
test for the link case with S_ISLNK() the fstat() can be removed.

Signed-off-by: Kjetil Barvik <barvik@broadpark.no>
---
 combine-diff.c |    5 ++---
 1 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/combine-diff.c b/combine-diff.c
index bccc018..ab4df31 100644
--- a/combine-diff.c
+++ b/combine-diff.c
@@ -713,9 +713,8 @@ static void show_patch_diff(struct combine_diff_path *elem, int num_parent,
 			result_size = buf.len;
 			result = strbuf_detach(&buf, NULL);
 			elem->mode = canon_mode(st.st_mode);
-		}
-		else if (0 <= (fd = open(elem->path, O_RDONLY)) &&
-			 !fstat(fd, &st)) {
+
+		} else if (0 <= (fd = open(elem->path, O_RDONLY))) {
 			size_t len = xsize_t(st.st_size);
 			ssize_t done;
 			int is_file, i;
-- 
1.6.1.349.g99fa5

Re: [PATCH/RFC v1 5/6] combine-diff.c: remove a call to fstat() inside show_patch_diff()

[Mike Ralphson]

2009/1/26 Kjetil Barvik <barvik@broadpark.no>:
> Currently inside show_patch_diff() we have and fstat() call after an
> ok lstat() call.  Since we before the call to fstat() have already
> test for the link case with S_ISLNK() the fstat() can be removed.

s/have and/have an/ ?

Re: [PATCH/RFC v1 5/6] combine-diff.c: remove a call to fstat() inside show_patch_diff()

[Kjetil Barvik]

Mike Ralphson <mike.ralphson@gmail.com> writes:

> 2009/1/26 Kjetil Barvik <barvik@broadpark.no>:
>> Currently inside show_patch_diff() we have and fstat() call after an
>> ok lstat() call.  Since we before the call to fstat() have already
>> test for the link case with S_ISLNK() the fstat() can be removed.
>
> s/have and/have an/ ?

  Correct! Will fix.  Thanks!

  -- kjetil

Re: [PATCH/RFC v1 5/6] combine-diff.c: remove a call to fstat() inside show_patch_diff()

[Mike Ralphson]

2009/1/27 Kjetil Barvik <barvik@broadpark.no>
> Mike Ralphson <mike.ralphson@gmail.com> writes:
>
> > 2009/1/26 Kjetil Barvik <barvik@broadpark.no>:
> >> Currently inside show_patch_diff() we have and fstat() call after an
> >> ok lstat() call.  Since we before the call to fstat() have already
> >> test for the link case with S_ISLNK() the fstat() can be removed.
> >
> > s/have and/have an/ ?
>
>  Correct! Will fix.  Thanks!

Typo stood out and blinded me to the rest of the grammar... How about:

Currently inside show_patch_diff() we have an fstat() call after an
ok lstat() call.  Since before the call to fstat() we have already
tested for the link case with S_ISLNK(), the fstat() can be removed.

'Half-a-job' Mike

??? Re: [PATCH/RFC v1 1/6] symlinks.c: small cleanup and optimisation

[Junio C Hamano]

Kjetil Barvik <barvik@broadpark.no> writes:

> Simplify the if-else test in longest_match_lstat_cache() such that we
> only have one simple if test.  Instead of testing for 'i == cache.len'
> or 'i == len', we transform this to a common test for 'i == max_len'.
>
> And to further optimise we use 'i >= max_len' instead of 'i ==
> max_len', the reason is that it is now the exact opposite of one part
> inside the while-loop termination expression 'i < max_len && name[i]
> == cache.path[i]', and then the compiler can hopefully/probably reuse
> a test-result from it.

This is *dense*, and made me wonder if is this really worth doing.

> +	/*
> +	 * Is the cached path string a substring of 'name', is 'name'
> +	 * a substring of the cached path string, or is 'name' and the
> +	 * cached path string the exact same string?
> +	 */
> +	if (i >= max_len && ((i < len && name[i] == '/') ||
> +			     (i < cache.len && cache.path[i] == '/') ||
> +			     (len == cache.len))) {

As you described in your commit log message, what this really wants to
check is (i == max_len).  By saying (i >= max_len) you are losing
readability, optimizing for compilers (that do not notice this) and
pessimizing for human readers (like me, who had to spend a few dozens of
seconds to realize what you are doing, to speculate why you might have
thought this would be a good idea, and writing this paragraph).  I do not
know if it is a good trade-off.

> @@ -91,7 +92,7 @@ static int lstat_cache(int len, const char *name,
>  		match_len = last_slash =
>  			longest_match_lstat_cache(len, name, &previous_slash);
>  		match_flags = cache.flags & track_flags & (FL_NOENT|FL_SYMLINK);
> -		if (match_flags && match_len == cache.len)
> +		if (match_flags && match_len >= cache.len)
>  			return match_flags;
>  		/*
>  		 * If we now have match_len > 0, we would know that

Likewise.

> @@ -102,7 +103,7 @@ static int lstat_cache(int len, const char *name,
>  		 * we can return immediately.
>  		 */
>  		match_flags = track_flags & FL_DIR;
> -		if (match_flags && len == match_len)
> +		if (match_flags && match_len >= len)
>  			return match_flags;
>  	}
>  

Likewise.

> @@ -184,7 +185,7 @@ void invalidate_lstat_cache(int len, const char *name)
>  	int match_len, previous_slash;
>  
>  	match_len = longest_match_lstat_cache(len, name, &previous_slash);
> -	if (len == match_len) {
> +	if (match_len >= len) {
>  		if ((cache.track_flags & FL_DIR) && previous_slash > 0) {
>  			cache.path[previous_slash] = '\0';
>  			cache.len = previous_slash;

Likewise.

People who would want to fix potential bugs in this code 3 months down the
road may not have a ready access to your commit log message (they may be
looking at an extract from a release tarball, scratching their heads
wondering why you are not checking for equality).  Perhaps the inline
function can have comments that says something like "when comparing the
return value from this function to see if it is equal to cache.len or len,
checking if the returned value is >= might help your compiler optimize it
better" to help the readers, but still, I do not know if it is a good
trade-off.

> @@ -153,7 +154,7 @@ static int lstat_cache(int len, const char *name,
>  		cache.path[last_slash] = '\0';
>  		cache.len = last_slash;
>  		cache.flags = save_flags;
> -	} else if (track_flags & FL_DIR &&
> +	} else if ((track_flags & FL_DIR) &&
>  		   last_slash_dir > 0 && last_slash_dir <= PATH_MAX) {
>  		/*
>  		 * We have a separate test for the directory case,

Good.

Re: [PATCH/RFC v1 2/6] remove some memcpy() and strchr() calls inside create_directories()

[Junio C Hamano]

Kjetil Barvik <barvik@broadpark.no> writes:

> OK, maybe I instead should have tried to merge the function
> create_directories() with the safe_create_leading_directories() and
> *_const() functions?  What do pepople think?

Strictly speaking, the safe_create_leading_* functions are meant to work
on paths inside $GIT_DIR and are not meant for paths inside the work tree,
which is this function is about.  Their semantics are different with
respect to adjust_shared_perm().

Which means that you would either have two variants (one for work tree
paths, and another for paths inside $GIT_DIR), or a unified function that
has an argument to specify whether to run adjust_shared_perm().

HOWEVER.

That is only "strictly speaking".

A non-bare repository that is shared feels like an oximoron, but perhaps
there is a valid "shared work area" workflow that may benefit from such a
setup.

I see existing (mis)uses of the safe_create_leading_* in builtin-apply.c,
builtin-clone.c (the one that creates the work_tree, the other one is Ok),
merge-recursive.c (both call sites) that are used to touch the work tree,
but all places that create regular files in the work tree do not run
adjust_shared_perm() but simply honor the user's umask.

If we _were_ to support a "shared work area" workflow, having a unified
"create leading directory" function that always calls adjust_shared_perm()
may make sense (note that adjust_shared_perm() is a no-op in a non-shared
repository).  We then need to also call adjust_shared_perm() for codepaths
that create regular files as well, though (e.g. write_entry() in entry.c,
but there are many others).

> diff --git a/entry.c b/entry.c
> index 05aa58d..c2404ea 100644
> --- a/entry.c
> +++ b/entry.c
> @@ -2,15 +2,19 @@
>  #include "blob.h"
>  #include "dir.h"
>  
> -static void create_directories(const char *path, const struct checkout *state)
> +static void
> +create_directories(int path_len, const char *path, const struct checkout *state)

Please do not split the line like this.

The existing sources are not laid out to allow "grep ^funcname(", nor are
we going to reformat all the files to support such a use case.

When we pass <string, length> pair to functions, I think we pass them in
the order I just wrote in all the other functions.

The micro-optimization itself makes sense to me, though.

Re: [PATCH/RFC v1 5/6] combine-diff.c: remove a call to fstat() inside show_patch_diff()

[Junio C Hamano]

Kjetil Barvik <barvik@broadpark.no> writes:

> Currently inside show_patch_diff() we have and fstat() call after an
> ok lstat() call.  Since we before the call to fstat() have already
> test for the link case with S_ISLNK() the fstat() can be removed.

Good eyes.  Thanks.

Re: [PATCH/RFC v1 3/6] cleanup of write_entry() in entry.c

[Junio C Hamano]

Kjetil Barvik <barvik@broadpark.no> writes:

> The switch-cases for S_IFREG and S_IFLNK was so similar that it will
> be better to do some cleanup and use the common parts of it.
>
> Also fold the longest lines such that no line is longer then 80 chars
> or so.
>
> And the entry.c file should now be clean for 'gcc -Wextra' warnings.
>
> Signed-off-by: Kjetil Barvik <barvik@broadpark.no>
> ---
>
> If people do not like this approach I can be willing to drop it from
> this patch-series, but then I get some source code duplication from
> the next patch (4/6).

Merging of the two similar codepaths looked good from a cursory reading,
but if you have doubts about one patch, it usually is easier for other
people to work with you if the series is reordered to have it near the
end, iow, undisputably good ones first.

> -			return error("git checkout-index: unable to read sha1 file of %s (%s)",
> -				path, sha1_to_hex(ce->sha1));
> +			return error("git checkout-index: "\
> +				     "unable to read sha1 file of %s (%s)",
> +				     path, sha1_to_hex(ce->sha1));

You lost greppability when somebody calls for help saying "I am getting an
error message that says 'git checkout-index: unable to read sha1 file'".

Re: [PATCH/RFC v1 5/6] combine-diff.c: remove a call to fstat() inside show_patch_diff()

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Kjetil Barvik <barvik@broadpark.no> writes:
>
>> Currently inside show_patch_diff() we have and fstat() call after an
>> ok lstat() call.  Since we before the call to fstat() have already
>> test for the link case with S_ISLNK() the fstat() can be removed.
>
> Good eyes.  Thanks.

Heh, I noticed you will update the commit log message, so I'll dequeue
this and wait for an update.

Re: [PATCH/RFC v1 5/6] combine-diff.c: remove a call to fstat() inside show_patch_diff()

[Kjetil Barvik]

Junio C Hamano <gitster@pobox.com> writes:

> Junio C Hamano <gitster@pobox.com> writes:
>
>> Kjetil Barvik <barvik@broadpark.no> writes:
>>
>>> Currently inside show_patch_diff() we have and fstat() call after an
>>> ok lstat() call.  Since we before the call to fstat() have already
>>> test for the link case with S_ISLNK() the fstat() can be removed.
>>
>> Good eyes.  Thanks.
>
> Heh, I noticed you will update the commit log message, so I'll dequeue
> this and wait for an update.

  Yes, I am planing a v2 in 1 or 3 days (it takes some time to run long-
  running tests).  And then hopefully I have addressed all comments so
  far.  Thanks for comments!

  -- kjetil

Re: ??? Re: [PATCH/RFC v1 1/6] symlinks.c: small cleanup and optimisation

[Kjetil Barvik]

* Kjetil Barvik <barvik@broadpark.no> writes:
> +	/*
> +	 * Is the cached path string a substring of 'name', is 'name'
> +	 * a substring of the cached path string, or is 'name' and the
> +	 * cached path string the exact same string?
> +	 */
> +	if (i >= max_len && ((i < len && name[i] == '/') ||
> +			     (i < cache.len && cache.path[i] == '/') ||
> +			     (len == cache.len))) {

* Junio C Hamano <gitster@pobox.com> writes:
> As you described in your commit log message, what this really wants to
> check is (i == max_len).  By saying (i >= max_len) you are losing
> readability, optimizing for compilers (that do not notice this) 

  When the compiler see the source line:

    'while (i < max_len && name[i] == cache.path[i])'

  it will, from what I know about compilers and machine instructions for
  intel cpu's, generate the following pseudo instructions for this line
  (before more compiler optimisation is done):

   1    test !(i < max_len)   /* which is (i >= max_len) */
   2    jump-if-not-zero  first_instruction_address_outside_the_loop
   3    "some instructions to retrieve some memory locations and test
         name[i] == cache.path[i]"
   4    jump-if-not-zero instruction-address-to-continue-to-loop

  So, I thought that if I could use the exact same test as the compiler
  have to generate for line 1, then I would maybe saved a test, and
  maybe also a jump instruction.

  Compiling with 'gcc -Os' (optimise for text segment size) I was able to
  save 4 bytes for this file, which I think shows that gcc is able to
  take advantage of this trick.

> and pessimizing for human readers (like me, who had to spend a few
> dozens of seconds to realize what you are doing, to speculate why you
> might have thought this would be a good idea, and writing this
> paragraph).  I do not know if it is a good trade-off.

  But, it was not easy to come up with a real test which shows a
  noticeable time difference from this trick, so, I guess this is only a
  trick for the kernel people (at most), so I revert it and will use
  '==' for version 2, such that we keep a little better readability.

  -- kjetil

  ps! Compiling with '-march=core2 -O2 -g0 -s -fomit-frame-pointer', the
      difference in text segment was 64 bytes when using this trick.