* [PATCH v10 2/5] lstat_cache(): introduce has_symlink_or_noent_leading_path() function
From: Kjetil Barvik @ 2009-01-18 15:14 UTC (permalink / raw)
To: git; +Cc: Kjetil Barvik
In-Reply-To: <1232291694-18083-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 non-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 | 94 +++++++++++++++++++++++++++++++++++--------------------
unpack-trees.c | 4 +-
3 files changed, 63 insertions(+), 36 deletions(-)
diff --git a/cache.h b/cache.h
index 8e1af26..518e4c7 100644
--- a/cache.h
+++ b/cache.h
@@ -717,6 +717,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 49fb4d8..c69556a 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;
/*
@@ -30,21 +31,23 @@ static inline int longest_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 can be indicated by the 'track_flags' argument.
@@ -56,25 +59,35 @@ 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 = longest_match_lstat_cache(len, name);
- match_flags = cache.flags & track_flags & FL_SYMLINK;
- if (match_flags && match_len == cache.len)
- return match_flags;
- /*
- * If we now have match_len > 0, we would know that the
- * matched part will always be a directory.
- *
- * Also, if we are tracking directories and 'name' is a
- * substring of the cache on a path component basis, we can
- * return immediately.
- */
- match_flags = track_flags & FL_DIR;
- if (match_flags && len == match_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 = longest_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 we now have match_len > 0, we would know that
+ * the matched part will always be a directory.
+ *
+ * Also, if we are tracking directories and 'name' is
+ * a substring of the cache on a path component basis,
+ * we can return immediately.
+ */
+ match_flags = track_flags & FL_DIR;
+ if (match_flags && len == match_len)
+ return match_flags;
+ }
/*
* Okay, no match from the cache so far, so now we have to
@@ -95,6 +108,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;
@@ -107,11 +122,11 @@ static int lstat_cache(int len, const char *name,
}
/*
- * 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;
@@ -120,20 +135,20 @@ static int lstat_cache(int len, const char *name,
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 cannot 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
+ * non-existing directory and the track_flags says
+ * that we cannot cache this fact, so the cache would
+ * then have been left empty in this case.
*
* But if we are 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 non-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;
}
@@ -147,3 +162,14 @@ 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 15c9ef5..16bc2ca 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;
@@ -580,7 +580,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.1.83.gd727f
^ permalink raw reply related
* [PATCH v10 3/5] lstat_cache(): introduce has_dirs_only_path() function
From: Kjetil Barvik @ 2009-01-18 15:14 UTC (permalink / raw)
To: git; +Cc: Kjetil Barvik
In-Reply-To: <1232291694-18083-1-git-send-email-barvik@broadpark.no>
The create_directories() function in entry.c currently calls stat()
or lstat() for each path component of the pathname 'path' each and every
time. 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 "please 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 | 64 ++++++++++++++++++++++++++++++++++++++++++++---------------
3 files changed, 60 insertions(+), 39 deletions(-)
diff --git a/cache.h b/cache.h
index 518e4c7..110b9f9 100644
--- a/cache.h
+++ b/cache.h
@@ -718,6 +718,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..01a683e 100644
--- a/entry.c
+++ b/entry.c
@@ -8,35 +8,25 @@ 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 c69556a..918e24a 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;
/*
@@ -31,12 +32,13 @@ static inline int longest_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)
@@ -44,28 +46,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 can be indicated by the 'track_flags' argument.
+ * This can be indicated by the 'track_flags' argument, which also can
+ * be used to indicate that we should 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 {
/*
@@ -101,12 +110,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;
@@ -127,12 +141,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
@@ -148,18 +162,20 @@ 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;
}
@@ -170,6 +186,20 @@ 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 symlinks 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.1.83.gd727f
^ permalink raw reply related
* [PATCH v10 5/5] lstat_cache(): introduce clear_lstat_cache() function
From: Kjetil Barvik @ 2009-01-18 15:14 UTC (permalink / raw)
To: git; +Cc: Kjetil Barvik
In-Reply-To: <1232291694-18083-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 | 8 ++++++++
2 files changed, 9 insertions(+), 0 deletions(-)
diff --git a/cache.h b/cache.h
index efcceec..8e22c27 100644
--- a/cache.h
+++ b/cache.h
@@ -720,6 +720,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 dbdfec4..83cecd7 100644
--- a/symlinks.c
+++ b/symlinks.c
@@ -195,6 +195,14 @@ 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
/*
--
1.6.1.83.gd727f
^ permalink raw reply related
* [PATCH v10 4/5] lstat_cache(): introduce invalidate_lstat_cache() function
From: Kjetil Barvik @ 2009-01-18 15:14 UTC (permalink / raw)
To: git; +Cc: Kjetil Barvik
In-Reply-To: <1232291694-18083-1-git-send-email-barvik@broadpark.no>
In some cases it could maybe be necessary to say to the cache that
"Hey, I deleted/changed the type of this pathname and if you currently
have 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 | 44 ++++++++++++++++++++++++++++++++++++--------
2 files changed, 37 insertions(+), 8 deletions(-)
diff --git a/cache.h b/cache.h
index 110b9f9..efcceec 100644
--- a/cache.h
+++ b/cache.h
@@ -719,6 +719,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 918e24a..dbdfec4 100644
--- a/symlinks.c
+++ b/symlinks.c
@@ -12,23 +12,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 longest_match_lstat_cache(int len, const char *name)
+static inline int longest_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 && cache.len < len && name[cache.len] == '/')
+ 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))
+ } 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;
}
@@ -63,7 +70,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;
@@ -81,7 +88,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 = longest_match_lstat_cache(len, 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)
return match_flags;
@@ -167,6 +175,26 @@ 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 = longest_match_lstat_cache(len, name, &previous_slash);
+ if (len == match_len) {
+ if (cache.track_flags & FL_DIR && previous_slash > 0) {
+ cache.path[previous_slash] = '\0';
+ cache.len = previous_slash;
+ cache.flags = FL_DIR;
+ } else
+ reset_lstat_cache(cache.track_flags,
+ cache.prefix_len_stat_func);
+ }
+}
+
#define USE_ONLY_LSTAT 0
/*
--
1.6.1.83.gd727f
^ permalink raw reply related
* Re: [WIP Patch 08/12] Use the new http API in update_remote_info_refs()
From: Johannes Schindelin @ 2009-01-18 15:18 UTC (permalink / raw)
To: Mike Hommey; +Cc: git, gitster
In-Reply-To: <1232265877-3649-9-git-send-email-mh@glandium.org>
Hi,
On Sun, 18 Jan 2009, Mike Hommey wrote:
>
> Signed-off-by: Mike Hommey <mh@glandium.org>
> ---
> http-push.c | 29 ++++++++++-------------------
> 1 files changed, 10 insertions(+), 19 deletions(-)
>
> diff --git a/http-push.c b/http-push.c
> index e0b4f5a..7627860 100644
> --- a/http-push.c
> +++ b/http-push.c
> @@ -1960,29 +1960,20 @@ static void update_remote_info_refs(struct remote_lock *lock)
> static int remote_exists(const char *path)
> {
Heh, I see where your commit subject comes from, but it should rather
mention the function "remote_exists()"...
> char *url = xmalloc(strlen(remote->url) + strlen(path) + 1);
> - struct active_request_slot *slot;
> - struct slot_results results;
> - int ret = -1;
> + int ret;
>
> sprintf(url, "%s%s", remote->url, path);
>
> - slot = get_active_slot();
> - slot->results = &results;
> - curl_easy_setopt(slot->curl, CURLOPT_URL, url);
> - curl_easy_setopt(slot->curl, CURLOPT_NOBODY, 1);
> -
> - if (start_active_slot(slot)) {
> - run_active_slot(slot);
> - if (results.http_code == 404)
> - ret = 0;
> - else if (results.curl_result == CURLE_OK)
> - ret = 1;
> - else
> - fprintf(stderr, "HEAD HTTP error %ld\n", results.http_code);
> - } else {
> - fprintf(stderr, "Unable to start HEAD request\n");
> + switch (http_get_strbuf(url, NULL, 0)) {
> + case HTTP_OK:
> + ret = 1;
> + break;
> + case HTTP_MISSING_TARGET:
> + ret = 0;
> + break;
> + default:
> + ret = -1;
> }
Does http_get_strbuf() already show the error? Not as far as I can see,
even if it would make sense, no? At least you'll have to "return
error(...)".
Ciao,
Dscho
^ permalink raw reply
* Re: [PATCH v4 0/7] customizable --color-words
From: Santi Béjar @ 2009-01-18 15:29 UTC (permalink / raw)
To: Thomas Rast
Cc: git, Junio C Hamano, Johannes Schindelin, Boyd Stephen Smith Jr.,
Teemu Likonen
In-Reply-To: <adf1fd3d0901180705s260f0051wb4e3a978601618ec@mail.gmail.com>
2009/1/18 Santi Béjar <santi@agolina.net>:
> 2009/1/17 Thomas Rast <trast@student.ethz.ch>:
>> Johannes Schindelin wrote:
>>> Thomas, could you pick up the patches from my 'my-next' branch and
>>> maintain an "official" topic branch?
>>
>> I cherry-picked the three commits you had there, and rebuilt on top.
>> I pushed them to
>>
>> git://repo.or.cz/git/trast.git tr/word-diff-p2
>>
>> again (js/word-diff-p1 again points directly at your half).
>
> I've tested tr/word-diff-p2 and I have not found any issues. I've even
> tested that nothing changed from the tradicional word diff to:
>
> git log -p --color-words="[^[:space:]]+"
>
> for the whole git history.
>
What I tested is that the new code produces the same result for this
two commands:
git log -p --color-words="[^[:space:]]+"
git log -p --color-words
The old code produced color codes before and after each word, while
the new only at the begining of the color and the end of the color. So
they cannot produce the same output but equivalent.
Santi
^ permalink raw reply
* Re: [PATCH 1/3] sha1_file: add function to insert alternate object db
From: Johannes Schindelin @ 2009-01-18 15:32 UTC (permalink / raw)
To: Lars Hjemli; +Cc: Junio C Hamano, git
In-Reply-To: <1232275999-14852-2-git-send-email-hjemli@gmail.com>
Hi,
On Sun, 18 Jan 2009, Lars Hjemli wrote:
> diff --git a/cache.h b/cache.h
> index 8e1af26..daa2d4e 100644
> --- a/cache.h
> +++ b/cache.h
> @@ -724,6 +724,7 @@ extern struct alternate_object_database {
> char base[FLEX_ARRAY]; /* more */
> } *alt_odb_list;
> extern void prepare_alt_odb(void);
> +extern int add_alt_odb(const char *path);
> extern void add_to_alternates_file(const char *reference);
> typedef int alt_odb_fn(struct alternate_object_database *, void *);
> extern void foreach_alt_odb(alt_odb_fn, void*);
> diff --git a/sha1_file.c b/sha1_file.c
> index f08493f..19f9725 100644
> --- a/sha1_file.c
> +++ b/sha1_file.c
> @@ -356,6 +356,11 @@ static void link_alt_odb_entries(const char *alt, const char *ep, int sep,
> }
> }
>
> +int add_alt_odb(const char *path)
> +{
> + return link_alt_odb_entry(path, strlen(path), NULL, 0);
This function can return the error message "object directory %s does not
exist; check .git/objects/info/alternates." Maybe you want to change
that, even if the user you are introducing might not hit that code path.
Ciao,
Dscho
^ permalink raw reply
* Re: easy way to make tracking branches?
From: Sitaram Chamarty @ 2009-01-18 15:37 UTC (permalink / raw)
To: git
In-Reply-To: <20090118105530.GG11992@leksak.fem-net>
On 2009-01-18, Stephan Beyer <s-beyer@gmx.net> wrote:
> Now I want to make "foo" a tracking branch for "bar".
> I do:
>
> git config branch.foo.remote srv
> git config branch.foo.merge refs/heads/bar
I just do a "git pull srv" when the tracking is *not* setup,
and git reminds me what commands to use.
> And to get a comfortable git-push, I do:
>
> git config --add remote.srv.push foo:bar
This one you'll just have to remember, I guess :-)
> git checkout -b foo2 srv/bar
> git branch -d foo
> git branch -m foo
>
> which is suboptimal because deleting foo can remove some
> other settings for the branch, e.g. mergeoptions.
it also doesn't seem to set remote.srv.push, as far as I can
tell.
^ permalink raw reply
* Re: [PATCH/RFC] git-am: Make it easier to see which patch failed
From: Jeff King @ 2009-01-18 15:39 UTC (permalink / raw)
To: Stephan Beyer; +Cc: Junio C Hamano, Jonas Flodén, git, Johannes Schindelin
In-Reply-To: <20090118094113.GE11992@leksak.fem-net>
On Sun, Jan 18, 2009 at 10:41:13AM +0100, Stephan Beyer wrote:
> > Looks sane except that I do not think you need printf nor the leading
> > blank line, i.e.
> >
> > echo "Patch failed at $msgnum ($FIRSTLINE)"
>
> Hmm, IIRC if $FIRSTLINE contains \n or something like that, it will
> interpret this as newline in some shell/echo implementations.
>
> So printf "...%s..." "$FOO" is always sane for user input.
Yes, I'm surprised Junio doesn't remember the mass conversions we already had
to do (4b7cc26a and 293623ed). But looking at the date, I guess it _has_
been a year and a half. :)
-Peff
^ permalink raw reply
* Re: [PATCH 2/3] Teach read_tree_recursive() how to traverse into submodules
From: Johannes Schindelin @ 2009-01-18 15:48 UTC (permalink / raw)
To: Lars Hjemli; +Cc: Junio C Hamano, git
In-Reply-To: <1232275999-14852-3-git-send-email-hjemli@gmail.com>
Hi,
On Sun, 18 Jan 2009, Lars Hjemli wrote:
> diff --git a/environment.c b/environment.c
> index e278bce..35cc557 100644
> --- a/environment.c
> +++ b/environment.c
> @@ -53,6 +53,8 @@ static char *work_tree;
> static const char *git_dir;
> static char *git_object_dir, *git_index_file, *git_refs_dir, *git_graft_file;
>
> +static int traverse_gitlinks = 0;
> +
> static void setup_git_env(void)
> {
> git_dir = getenv(GIT_DIR_ENVIRONMENT);
> @@ -159,3 +161,13 @@ int set_git_dir(const char *path)
> setup_git_env();
> return 0;
> }
> +
> +int get_traverse_gitlinks()
> +{
> + return traverse_gitlinks;
> +}
> +
> +void set_traverse_gitlinks(int traverse)
> +{
> + traverse_gitlinks = traverse;
> +}
If you have full accessors anyway, it is much easier and cleaner to make
this a global variable to begin with.
However, environment.c is reserved for things that come from the config
and can be overridden by the user. That is certainly not the case for
traverse_gitlinks.
But let's think about it again: should traverse_gitlinks be a global
varible at all? I think not. It should be a per-call decision.
> diff --git a/tree.c b/tree.c
> index 03e782a..87cf309 100644
> --- a/tree.c
> +++ b/tree.c
> @@ -5,6 +5,7 @@
> #include "commit.h"
> #include "tag.h"
> #include "tree-walk.h"
> +#include "refs.h"
>
> const char *tree_type = "tree";
>
> @@ -89,6 +90,61 @@ static int match_tree_entry(const char *base, int baselen, const char *path, uns
> return 0;
> }
>
> +/* Try to add the objectdb of a submodule */
> +int add_gitlink_odb(char *relpath)
This wants to be static.
> +{
> + const char *odbpath;
> + struct stat st;
> +
> + odbpath = read_gitfile_gently(mkpath("%s/.git", relpath));
> + if (!odbpath)
> + odbpath = mkpath("%s/.git/objects", relpath);
> +
> + if (stat(odbpath, &st))
> + return 1;
> +
> + return add_alt_odb(odbpath);
> +}
> +
> +/* Check if we should recurse into the specified submodule */
> +int traverse_gitlink(char *path, const unsigned char *commit_sha1,
This, too.
> + struct tree **subtree)
> +{
> + unsigned char sha1[20];
> + int linked_odb = 0;
> + struct commit *commit;
> + void *buffer;
> + enum object_type type;
> + unsigned long size;
> +
> + hashcpy(sha1, commit_sha1);
> + if (!add_gitlink_odb(path)) {
> + linked_odb = 1;
> + if (resolve_gitlink_ref(path, "HEAD", sha1))
> + die("Unable to lookup HEAD in %s", path);
> + }
Why would you want to continue if add_gitlink_odb() did not find a checked
out submodule?
Seems you want to fall back to look in the superproject's object database.
But I think that is wrong, as I have a superproject with many platform
dependent submodules, only one of which is checked out, and for
convenience, the submodules all live in the superproject's repository.
But I might misunderstand your code.
> + commit = lookup_commit(sha1);
> + if (!commit)
> + die("traverse_gitlink(): internal error");
s/internal error/could not access commit '%s' of submodule '%s'",
sha1_to_hex(sha1), path);/
> @@ -132,6 +188,30 @@ int read_tree_recursive(struct tree *tree,
> return -1;
> continue;
> }
> + if (S_ISGITLINK(entry.mode) && get_traverse_gitlinks()) {
Like I said, traverse_gitlinks should be a flag to read_tree_recursive.
So preferably, you should add a parameter 'flags' and make that option an
enum.
> + int retval;
> + char *newbase;
> + struct tree *subtree;
> + unsigned int pathlen = tree_entry_len(entry.path, entry.sha1);
Nit: Long line.
> +
> + newbase = xmalloc(baselen + 1 + pathlen);
> + memcpy(newbase, base, baselen);
> + memcpy(newbase + baselen, entry.path, pathlen);
> + newbase[baselen + pathlen] = 0;
We have strbufs for that.
> + if (!traverse_gitlink(newbase, entry.sha1, &subtree)) {
> + free(newbase);
> + continue;
> + }
> + newbase[baselen + pathlen] = '/';
... to avoid this off-by-one.
> + retval = read_tree_recursive(subtree,
> + newbase,
> + baselen + pathlen + 1,
> + stage, match, fn, context);
> + free(newbase);
> + if (retval)
> + return -1;
> + continue;
> + }
> }
> return 0;
> }
Ciao,
Dscho
^ permalink raw reply
* Re: [PATCH 3/3] git-archive: add support for --submodules
From: Johannes Schindelin @ 2009-01-18 15:51 UTC (permalink / raw)
To: Lars Hjemli; +Cc: Junio C Hamano, git
In-Reply-To: <1232275999-14852-4-git-send-email-hjemli@gmail.com>
Hi,
On Sun, 18 Jan 2009, Lars Hjemli wrote:
> @@ -320,6 +323,7 @@ static int parse_archive_args(int argc, const char **argv,
> args->base = base;
> args->baselen = strlen(base);
>
> + set_traverse_gitlinks(submodules);
As I said, this is a per-call thing. So you need to add that option to
the archiver_args struct and use it in write_archive_entries().
Ciao,
Dscho
^ permalink raw reply
* Re: easy way to make tracking branches?
From: Stephan Beyer @ 2009-01-18 15:53 UTC (permalink / raw)
To: Sitaram Chamarty; +Cc: git
In-Reply-To: <slrngn6j4t.9he.sitaramc@sitaramc.homelinux.net>
Hi,
Sitaram Chamarty wrote:
> > Now I want to make "foo" a tracking branch for "bar".
> > I do:
> >
> > git config branch.foo.remote srv
> > git config branch.foo.merge refs/heads/bar
>
> I just do a "git pull srv" when the tracking is *not* setup,
> and git reminds me what commands to use.
Oh, this is great! (Just tested.)
Usually (in my use case) there is no need to pull before I set up
a tracking branch, so I never tried this.
And git only reminds you if you have no tracking branch set for
srv yet. But that's no problem, because if one is set I can
copy&paste the lines in .git/config.
Altogether, this is a solution I can live with.
> > And to get a comfortable git-push, I do:
> >
> > git config --add remote.srv.push foo:bar
>
> This one you'll just have to remember, I guess :-)
This is not too hard. :-)
> > git checkout -b foo2 srv/bar
> > git branch -d foo
> > git branch -m foo
> >
> > which is suboptimal because deleting foo can remove some
> > other settings for the branch, e.g. mergeoptions.
>
> it also doesn't seem to set remote.srv.push, as far as I can
> tell.
Right.
Thanks,
Stephan
--
Stephan Beyer <s-beyer@gmx.net>, PGP 0x6EDDD207FCC5040F
^ permalink raw reply
* [PATCH] sha1_file: add function to insert alternate object db
From: Lars Hjemli @ 2009-01-18 15:55 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: Junio C Hamano, git
In-Reply-To: <alpine.DEB.1.00.0901181629590.3586@pacific.mpi-cbg.de>
This function will be used when implementing traversal into submodules.
Signed-off-by: Lars Hjemli <hjemli@gmail.com>
---
On Sun, Jan 18, 2009 at 16:32, Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
>
> On Sun, 18 Jan 2009, Lars Hjemli wrote:
>> +int add_alt_odb(const char *path)
>> +{
>> + return link_alt_odb_entry(path, strlen(path), NULL, 0);
>
> This function can return the error message "object directory %s does not
> exist; check .git/objects/info/alternates." Maybe you want to change
> that, even if the user you are introducing might not hit that code path.
Something like this, maybe?
cache.h | 1 +
sha1_file.c | 17 ++++++++++++-----
2 files changed, 13 insertions(+), 5 deletions(-)
diff --git a/cache.h b/cache.h
index 8e1af26..0dbe2a6 100644
--- a/cache.h
+++ b/cache.h
@@ -724,6 +724,7 @@ extern struct alternate_object_database {
char base[FLEX_ARRAY]; /* more */
} *alt_odb_list;
extern void prepare_alt_odb(void);
+extern int add_alt_odb(const char *path, int quiet);
extern void add_to_alternates_file(const char *reference);
typedef int alt_odb_fn(struct alternate_object_database *, void *);
extern void foreach_alt_odb(alt_odb_fn, void*);
diff --git a/sha1_file.c b/sha1_file.c
index f08493f..4b7e691 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -252,7 +252,8 @@ static void read_info_alternates(const char * alternates, int depth);
* SHA1, an extra slash for the first level indirection, and the
* terminating NUL.
*/
-static int link_alt_odb_entry(const char * entry, int len, const char * relative_base, int depth)
+static int link_alt_odb_entry(const char * entry, int len,
+ const char * relative_base, int depth, int quiet)
{
const char *objdir = get_object_directory();
struct alternate_object_database *ent;
@@ -285,9 +286,10 @@ static int link_alt_odb_entry(const char * entry, int len, const char * relative
/* Detect cases where alternate disappeared */
if (!is_directory(ent->base)) {
- error("object directory %s does not exist; "
- "check .git/objects/info/alternates.",
- ent->base);
+ if (!quiet)
+ error("object directory %s does not exist; "
+ "check .git/objects/info/alternates.",
+ ent->base);
free(ent);
return -1;
}
@@ -347,7 +349,7 @@ static void link_alt_odb_entries(const char *alt, const char *ep, int sep,
relative_base, last);
} else {
link_alt_odb_entry(last, cp - last,
- relative_base, depth);
+ relative_base, depth, 0);
}
}
while (cp < ep && *cp == sep)
@@ -356,6 +358,11 @@ static void link_alt_odb_entries(const char *alt, const char *ep, int sep,
}
}
+int add_alt_odb(const char *path, int quiet)
+{
+ return link_alt_odb_entry(path, strlen(path), NULL, 0, quiet);
+}
+
static void read_info_alternates(const char * relative_base, int depth)
{
char *map;
--
1.6.1.150.g5e733b
^ permalink raw reply related
* meaning of --8<--
From: Markus Heidelberg @ 2009-01-18 15:56 UTC (permalink / raw)
To: git
Hi,
I've seen lines like "--8<--" several times on this list, but have no
clue what it is about. OK, seems like it's used to insert diffs in the
middle of a mail message.
But is this a common convention or git specific and handled by git-am?
Is it documented anywhere?
Markus
^ permalink raw reply
* Re: meaning of --8<--
From: Peter Harris @ 2009-01-18 16:04 UTC (permalink / raw)
To: markus.heidelberg; +Cc: git
In-Reply-To: <200901181656.37813.markus.heidelberg@web.de>
On Sun, Jan 18, 2009 at 10:56 AM, Markus Heidelberg wrote:
> I've seen lines like "--8<--" several times on this list, but have no
> clue what it is about. OK, seems like it's used to insert diffs in the
> middle of a mail message.
> But is this a common convention or git specific and handled by git-am?
> Is it documented anywhere?
Common convention, meaning 'cut here'.
Perforated line ------
Scissors 8<
Also used in print (although usually with picture of scissors instead
of the ASCII-art).
Peter Harris
^ permalink raw reply
* Re: [PATCH 2/3] Teach read_tree_recursive() how to traverse into submodules
From: René Scharfe @ 2009-01-18 16:13 UTC (permalink / raw)
To: Lars Hjemli; +Cc: Junio C Hamano, git
In-Reply-To: <1232275999-14852-3-git-send-email-hjemli@gmail.com>
Lars Hjemli schrieb:
> The traversal of submodules is only triggered if the current submodule
> HEAD commit object is accessible. To this end, read_tree_recursive()
> will try to insert the submodule odb as an alternate odb but the lack
> of such an odb is not treated as an error since it is then assumed that
> the user is not interested in the submodule content. However, if the
> submodule odb is found it is treated as an error if the HEAD commit
> object is missing.
Callers of read_tree_recursive() specify a tree to traverse.
Unconditionally using the HEAD of submodules feels a bit restrictive,
but I don't use submodules, so I have no idea what I'm actually talking
about here. :)
> int read_tree_recursive(struct tree *tree,
> const char *base, int baselen,
> int stage, const char **match,
> @@ -132,6 +188,30 @@ int read_tree_recursive(struct tree *tree,
> return -1;
> continue;
> }
> + if (S_ISGITLINK(entry.mode) && get_traverse_gitlinks()) {
> + int retval;
> + char *newbase;
> + struct tree *subtree;
> + unsigned int pathlen = tree_entry_len(entry.path, entry.sha1);
> +
> + newbase = xmalloc(baselen + 1 + pathlen);
> + memcpy(newbase, base, baselen);
> + memcpy(newbase + baselen, entry.path, pathlen);
> + newbase[baselen + pathlen] = 0;
> + if (!traverse_gitlink(newbase, entry.sha1, &subtree)) {
> + free(newbase);
> + continue;
> + }
> + newbase[baselen + pathlen] = '/';
> + retval = read_tree_recursive(subtree,
> + newbase,
> + baselen + pathlen + 1,
> + stage, match, fn, context);
> + free(newbase);
> + if (retval)
> + return -1;
> + continue;
> + }
> }
> return 0;
> }
You don't need to call get_traverse_gitlinks() in the if statement above
if you make all read_tree_recursive() callback functions return 0 for
gitlinks that they don't want to follow and READ_TREE_RECURSIVE for
those they do. It's cleaner without the static variable and its
accessors and more flexible, too: the callbacks might decide to traverse
only certain submodules.
René
^ permalink raw reply
* Re: meaning of --8<--
From: Jay Soffian @ 2009-01-18 16:13 UTC (permalink / raw)
To: markus.heidelberg; +Cc: git
In-Reply-To: <200901181656.37813.markus.heidelberg@web.de>
On Sun, Jan 18, 2009 at 10:56 AM, Markus Heidelberg
<markus.heidelberg@web.de> wrote:
> Hi,
>
> I've seen lines like "--8<--" several times on this list, but have no
> clue what it is about.
It is supposed to represent a pair of scissors. Sometimes it is the
other direction: >8. And I've seen folks botch it as: <8 or 8>. This
list is the first (and maybe only?) place I've seen it. Typically you
might see:
--snip--
or
--cut--
> But is this a common convention or git specific and handled by git-am?
Seems to be a convention of this list, and git-am does nothing with it afaik.
j.
^ permalink raw reply
* Re: [PATCH/RFC] git-am: Make it easier to see which patch failed
From: Johannes Schindelin @ 2009-01-18 16:17 UTC (permalink / raw)
To: Jeff King; +Cc: Stephan Beyer, Junio C Hamano, Jonas Flodén, git
In-Reply-To: <20090118153928.GA16664@coredump.intra.peff.net>
Hi,
On Sun, 18 Jan 2009, Jeff King wrote:
> On Sun, Jan 18, 2009 at 10:41:13AM +0100, Stephan Beyer wrote:
>
> > > Looks sane except that I do not think you need printf nor the leading
> > > blank line, i.e.
> > >
> > > echo "Patch failed at $msgnum ($FIRSTLINE)"
> >
> > Hmm, IIRC if $FIRSTLINE contains \n or something like that, it will
> > interpret this as newline in some shell/echo implementations.
> >
> > So printf "...%s..." "$FOO" is always sane for user input.
>
> Yes, I'm surprised Junio doesn't remember the mass conversions we
> already had to do (4b7cc26a and 293623ed). But looking at the date, I
> guess it _has_ been a year and a half. :)
Hey, be nice to Junio. Have you seen the amount of mails on this list
recently? I think Junio's the only one really reading all of them; even
if you were right, he would be entitled to a nicer reminder.
But you are wrong. And Stephan is wrong, too.
The name "FIRSTLINE" suggests that it is indeed a first line, and
consequently cannot contain a newline.
And indeed, it is defined as
FIRSTLINE=$(sed 1q "$dotest/final-commit")
Just do the following in any of your favorite shells:
$ FIRSTLINE=$(sed 1q README)
$ echo "$FIRSTLINE."
You'll find that the "." is not in a new line.
And I know that we relied on that behavior for an eternity.
So there is certainly no need for a printf here.
'nuff said,
Dscho
^ permalink raw reply
* Re: [TOY PATCH] git-resurrect: find traces of a branch name and resurrect it
From: Johannes Schindelin @ 2009-01-18 16:19 UTC (permalink / raw)
To: Thomas Rast; +Cc: git, Junio C Hamano
In-Reply-To: <1232242703-19086-1-git-send-email-trast@student.ethz.ch>
Hi,
On Sun, 18 Jan 2009, Thomas Rast wrote:
> Makefile | 1 +
> git-resurrect.sh | 109 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
Maybe have it in contrib/ instead?
Ciao,
Dscho
^ permalink raw reply
* Re: [PATCH 2/3] Teach read_tree_recursive() how to traverse into submodules
From: Lars Hjemli @ 2009-01-18 16:37 UTC (permalink / raw)
To: René Scharfe; +Cc: Junio C Hamano, git
In-Reply-To: <49735530.4090901@lsrfire.ath.cx>
On Sun, Jan 18, 2009 at 17:13, René Scharfe <rene.scharfe@lsrfire.ath.cx> wrote:
> Lars Hjemli schrieb:
>> The traversal of submodules is only triggered if the current submodule
>> HEAD commit object is accessible. To this end, read_tree_recursive()
>> will try to insert the submodule odb as an alternate odb but the lack
>> of such an odb is not treated as an error since it is then assumed that
>> the user is not interested in the submodule content. However, if the
>> submodule odb is found it is treated as an error if the HEAD commit
>> object is missing.
>
> Callers of read_tree_recursive() specify a tree to traverse.
> Unconditionally using the HEAD of submodules feels a bit restrictive,
> but I don't use submodules, so I have no idea what I'm actually talking
> about here. :)
For bare repositories (where the submodule repo is added to
objects/info/alternates), following the tree of the linked commit is
the only option. And for non-bare repositories with the submodule
checked out, I think we should honor the users choice of checked out
HEAD in the submodule (especially since we don't have any other way to
specify which submodule commit to follow).
>
>> int read_tree_recursive(struct tree *tree,
>> const char *base, int baselen,
>> int stage, const char **match,
>> @@ -132,6 +188,30 @@ int read_tree_recursive(struct tree *tree,
>> return -1;
>> continue;
>> }
>> + if (S_ISGITLINK(entry.mode) && get_traverse_gitlinks()) {
>> + int retval;
>> + char *newbase;
>> + struct tree *subtree;
>> + unsigned int pathlen = tree_entry_len(entry.path, entry.sha1);
>> +
>> + newbase = xmalloc(baselen + 1 + pathlen);
>> + memcpy(newbase, base, baselen);
>> + memcpy(newbase + baselen, entry.path, pathlen);
>> + newbase[baselen + pathlen] = 0;
>> + if (!traverse_gitlink(newbase, entry.sha1, &subtree)) {
>> + free(newbase);
>> + continue;
>> + }
>> + newbase[baselen + pathlen] = '/';
>> + retval = read_tree_recursive(subtree,
>> + newbase,
>> + baselen + pathlen + 1,
>> + stage, match, fn, context);
>> + free(newbase);
>> + if (retval)
>> + return -1;
>> + continue;
>> + }
>> }
>> return 0;
>> }
>
> You don't need to call get_traverse_gitlinks() in the if statement above
> if you make all read_tree_recursive() callback functions return 0 for
> gitlinks that they don't want to follow and READ_TREE_RECURSIVE for
> those they do. It's cleaner without the static variable and its
> accessors and more flexible, too: the callbacks might decide to traverse
> only certain submodules.
I like the idea, but it will require thorough review of all
read_tree_recursive() consumers. So now we've got three different
approaches:
* me: global setting
* dscho: parameter to read_tree_recursive()
* you: accept the return value from the callback function
Junio, what would you prefer?
--
larsh
^ permalink raw reply
* Re: [PATCH/RFC] git-am: Make it easier to see which patch failed
From: Thomas Rast @ 2009-01-18 16:49 UTC (permalink / raw)
To: Johannes Schindelin
Cc: Jeff King, Stephan Beyer, Junio C Hamano, Jonas Flodén, git
In-Reply-To: <alpine.DEB.1.00.0901181711090.3586@pacific.mpi-cbg.de>
[-- Attachment #1: Type: text/plain, Size: 995 bytes --]
Johannes Schindelin wrote:
> On Sun, 18 Jan 2009, Jeff King wrote:
> > On Sun, Jan 18, 2009 at 10:41:13AM +0100, Stephan Beyer wrote:
> > > Hmm, IIRC if $FIRSTLINE contains \n or something like that, it will
> > > interpret this as newline in some shell/echo implementations.
> > >
> > > So printf "...%s..." "$FOO" is always sane for user input.
>
> But you are wrong. And Stephan is wrong, too.
>
> The name "FIRSTLINE" suggests that it is indeed a first line, and
> consequently cannot contain a newline.
I think the point was that $FIRSTLINE can contain a backslash sequence
such as (literally) \n or \r. Indeed 'man 1p echo' on my system says
_string_ A string to be written to standard output. If the first
operand is -n, or if any of the operands contain a
backslash ( '\' ) character, the results are
implementation- defined.
(Those POSIX manpages are really useful!)
--
Thomas Rast
trast@{inf,student}.ethz.ch
[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 197 bytes --]
^ permalink raw reply
* Re: [PATCH/RFC] git-am: Make it easier to see which patch failed
From: Stephan Beyer @ 2009-01-18 16:53 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: Jeff King, Junio C Hamano, Jonas Flodén, git
In-Reply-To: <alpine.DEB.1.00.0901181711090.3586@pacific.mpi-cbg.de>
Hi,
Johannes Schindelin wrote:
> On Sun, 18 Jan 2009, Jeff King wrote:
>
> > On Sun, Jan 18, 2009 at 10:41:13AM +0100, Stephan Beyer wrote:
> >
> > > > Looks sane except that I do not think you need printf nor the leading
> > > > blank line, i.e.
> > > >
> > > > echo "Patch failed at $msgnum ($FIRSTLINE)"
> > >
> > > Hmm, IIRC if $FIRSTLINE contains \n or something like that, it will
> > > interpret this as newline in some shell/echo implementations.
> > >
> > > So printf "...%s..." "$FOO" is always sane for user input.
> >
> > Yes, I'm surprised Junio doesn't remember the mass conversions we
> > already had to do (4b7cc26a and 293623ed). But looking at the date, I
> > guess it _has_ been a year and a half. :)
>
> Hey, be nice to Junio. Have you seen the amount of mails on this list
> recently? I think Junio's the only one really reading all of them; even
> if you were right, he would be entitled to a nicer reminder.
I had almost written the same text but then I thought Jeff did not mean
it bad, he was just surprised.
> But you are wrong. And Stephan is wrong, too.
>
> The name "FIRSTLINE" suggests that it is indeed a first line, and
> consequently cannot contain a newline.
>
> And indeed, it is defined as
>
> FIRSTLINE=$(sed 1q "$dotest/final-commit")
>
> Just do the following in any of your favorite shells:
>
> $ FIRSTLINE=$(sed 1q README)
> $ echo "$FIRSTLINE."
>
> You'll find that the "." is not in a new line.
I have to disagree:
$ cat newline
foo\nbar
$ FIRSTLINE=$(sed 1q newline)
$ echo "$FIRSTLINE."
foo
bar.
$ exit
> And I know that we relied on that behavior for an eternity.
Where? We should perhaps fix it then.
Regards,
Stephan
--
Stephan Beyer <s-beyer@gmx.net>, PGP 0x6EDDD207FCC5040F
^ permalink raw reply
* Re: fatal: git grep: cannot generate relative filenames containing '..'
From: George Spelvin @ 2009-01-18 16:58 UTC (permalink / raw)
To: gitster, azure; +Cc: linux, git
In-Reply-To: <83vdsefz9j.fsf@kalahari.s2.org>
Hannu Koivisto <azure@iki.fi> wrote:
> It turns out that this "entire git tree" is practically my only use
> case when using normal (find and) grep and when I first tried git
> grep, I actually expected it to do just that with no path specified.
>
> Why? Because of the way at least git diff and git log work. I
> thought that just like with them, "git grep foo" would search the
> entire git tree and I would have to say "git grep foo ." to limit to
> the current directory and its subdirectories. git-grep(1)'s "Look
> for specified patterns in the working tree files..." at least
> didn't seem to disagree with my expectation so I was a bit puzzled.
>
> So I'd rather see git grep behave in a way consistent with git log
> and git diff (I realize that would change current behaviour instead
> of extending it).
D'oh. You're completely right. Space-dot is trivial to type if you
want the current directory only, and that is more consistent.
Could that be considered for 1.7?
^ permalink raw reply
* Re: [PATCH/RFC] git-am: Make it easier to see which patch failed
From: Jeff King @ 2009-01-18 17:07 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: Stephan Beyer, Junio C Hamano, Jonas Flodén, git
In-Reply-To: <alpine.DEB.1.00.0901181711090.3586@pacific.mpi-cbg.de>
On Sun, Jan 18, 2009 at 05:17:43PM +0100, Johannes Schindelin wrote:
> > Yes, I'm surprised Junio doesn't remember the mass conversions we
> > already had to do (4b7cc26a and 293623ed). But looking at the date, I
> > guess it _has_ been a year and a half. :)
>
> Hey, be nice to Junio. Have you seen the amount of mails on this list
> recently? I think Junio's the only one really reading all of them; even
> if you were right, he would be entitled to a nicer reminder.
I didn't mean to be mean. On the contrary, I was surprised because _he_
usually is the one reminding _me_ about such fixes. I guess Junio is
human, after all. :)
> But you are wrong. And Stephan is wrong, too.
>
> The name "FIRSTLINE" suggests that it is indeed a first line, and
> consequently cannot contain a newline.
It is not "this is a problem because it might contain a newline" but "this
is a problem because it might contain an escape sequence, _an example_
of which is a \n newline." So the question is whether you can guarantee
that $FIRSTLINE does not contain a backslash. Which I don't think is the
case here.
-Peff
^ permalink raw reply
* Re: [PATCH 1/2] color: make it easier for non-config to parse color specs
From: René Scharfe @ 2009-01-18 17:10 UTC (permalink / raw)
To: Jeff King; +Cc: Junio C Hamano, Markus Heidelberg, git
In-Reply-To: <20090117153229.GA27071@coredump.intra.peff.net>
Jeff King schrieb:
> We have very featureful color-parsing routines which are
> used for color.diff.* and other options. Let's make it
> easier to use those routines from other parts of the code.
>
> This patch adds a color_parse_mem() helper function which
> takes a length-bounded string instead of a NUL-terminated
> one. While the helper is only a few lines long, it is nice
> to abstract this out so that:
>
> - callers don't forget to free() the temporary buffer
>
> - right now, it is implemented in terms of color_parse().
> But it would be more efficient to reverse this and
> implement color_parse in terms of color_parse_mem.
Thusly?
diff --git a/color.c b/color.c
index fc0b72a..14eac93 100644
--- a/color.c
+++ b/color.c
@@ -41,6 +41,11 @@ static int parse_attr(const char *name, int len)
void color_parse(const char *value, const char *var, char *dst)
{
+ color_parse_mem(value, strlen(value), var, dst);
+}
+
+void color_parse_mem(const char *value, int len, const char *var, char *dst)
+{
const char *ptr = value;
int attr = -1;
int fg = -2;
@@ -52,18 +57,22 @@ void color_parse(const char *value, const char *var, char *dst)
}
/* [fg [bg]] [attr] */
- while (*ptr) {
+ while (len > 0) {
const char *word = ptr;
- int val, len = 0;
+ int val, wordlen = 0;
- while (word[len] && !isspace(word[len]))
- len++;
+ while (len > 0 && !isspace(word[wordlen])) {
+ wordlen++;
+ len--;
+ }
- ptr = word + len;
- while (*ptr && isspace(*ptr))
+ ptr = word + wordlen;
+ while (len > 0 && isspace(*ptr)) {
ptr++;
+ len--;
+ }
- val = parse_color(word, len);
+ val = parse_color(word, wordlen);
if (val >= -1) {
if (fg == -2) {
fg = val;
@@ -75,7 +84,7 @@ void color_parse(const char *value, const char *var, char *dst)
}
goto bad;
}
- val = parse_attr(word, len);
+ val = parse_attr(word, wordlen);
if (val < 0 || attr != -1)
goto bad;
attr = val;
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox