* [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 1/5] lstat_cache(): more cache effective symlink/directory detection
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>
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 | 165 +++++++++++++++++++++++++++++++++++++++++++++---------------
1 files changed, 125 insertions(+), 40 deletions(-)
diff --git a/symlinks.c b/symlinks.c
index 5a5e781..49fb4d8 100644
--- a/symlinks.c
+++ b/symlinks.c
@@ -1,64 +1,149 @@
#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 longest_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 && cache.len < 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 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 = longest_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 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.
*/
- known_dir = match_pathname(len, name, &nonlink);
+ match_flags = track_flags & FL_DIR;
+ if (match_flags && len == match_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 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).
+ */
+ 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.1.83.gd727f
^ permalink raw reply related
* [PATCH v10 0/5] git checkout: optimise away lots of lstat() calls
From: Kjetil Barvik @ 2009-01-18 15:14 UTC (permalink / raw)
To: git; +Cc: Kjetil Barvik
Changes since version 9
--- patch 1/5 ---
bug fix (missed optimisation):
If we have track_flags = FL_SYMLINK|FL_DIR, and:
cache => A/B/C/D/S (a symlink)
name => A/B/C (a directory)
The cache would return with a (cached) FL_DIR result as expected,
_but_ it would set the cache to "A/B/C", so we lose some information.
--- patch 4/5 ---
bug fix: if we get a match from the cache for the name to invalidate,
we reset the cache if we are not allowed to track directories (inside
the invalidate_lstat_cache() function).
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 | 263 ++++++++++++++++++++++++++++++++++++++++++++++---------
unpack-trees.c | 4 +-
4 files changed, 238 insertions(+), 67 deletions(-)
^ permalink raw reply
* Re: [WIP Patch 07/12] Use the new http API in http-push.c:fetch_indices()
From: Johannes Schindelin @ 2009-01-18 15:14 UTC (permalink / raw)
To: Mike Hommey; +Cc: git, gitster
In-Reply-To: <1232265877-3649-8-git-send-email-mh@glandium.org>
Hi,
On Sun, 18 Jan 2009, Mike Hommey wrote:
> @@ -1018,28 +1016,10 @@ static int fetch_indices(void)
> url = xmalloc(strlen(remote->url) + 20);
> sprintf(url, "%sobjects/info/packs", remote->url);
>
> - slot = get_active_slot();
> - slot->results = &results;
> - curl_easy_setopt(slot->curl, CURLOPT_FILE, &buffer);
> - curl_easy_setopt(slot->curl, CURLOPT_WRITEFUNCTION, fwrite_buffer);
> - curl_easy_setopt(slot->curl, CURLOPT_URL, url);
> - curl_easy_setopt(slot->curl, CURLOPT_HTTPHEADER, NULL);
> - if (start_active_slot(slot)) {
> - run_active_slot(slot);
> - if (results.curl_result != CURLE_OK) {
> - strbuf_release(&buffer);
> - free(url);
> - if (results.http_code == 404)
> - return 0;
> - else
> - return error("%s", curl_errorstr);
> - }
> - } else {
> - strbuf_release(&buffer);
> - free(url);
> - return error("Unable to start request");
> + if (http_get_strbuf(url, &buffer, 0) != HTTP_OK) {
> + ret = -1;
> + goto cleanup;
> }
Let's make that
ret = error("%s", curl_errorstr);
okay?
Ciao,
Dscho
^ permalink raw reply
* Re: [WIP Patch 05/12] Use the new http API in get_refs_via_curl()
From: Johannes Schindelin @ 2009-01-18 15:12 UTC (permalink / raw)
To: Mike Hommey; +Cc: git, gitster
In-Reply-To: <1232265877-3649-6-git-send-email-mh@glandium.org>
Hi,
On Sun, 18 Jan 2009, Mike Hommey wrote:
> @@ -450,26 +447,8 @@ static struct ref *get_refs_via_curl(struct transport *transport)
> refs_url = xmalloc(strlen(transport->url) + 11);
> sprintf(refs_url, "%s/info/refs", transport->url);
>
> - slot = get_active_slot();
> - slot->results = &results;
> - curl_easy_setopt(slot->curl, CURLOPT_FILE, &buffer);
> - curl_easy_setopt(slot->curl, CURLOPT_WRITEFUNCTION, fwrite_buffer);
> - curl_easy_setopt(slot->curl, CURLOPT_URL, refs_url);
> - curl_easy_setopt(slot->curl, CURLOPT_HTTPHEADER, NULL);
> -
> - if (start_active_slot(slot)) {
> - run_active_slot(slot);
> - if (results.curl_result != CURLE_OK) {
> - strbuf_release(&buffer);
> - if (missing_target(&results))
> - die("%s not found: did you run git update-server-info on the server?", refs_url);
> - else
> - die("%s download error - %s", refs_url, curl_errorstr);
> - }
> - } else {
> - strbuf_release(&buffer);
> - die("Unable to start HTTP request");
> - }
> + if (http_get_strbuf(refs_url, &buffer, HTTP_NO_CACHE) != HTTP_OK)
> + goto cleanup;
Why not die() as the original code?
Ciao,
Dscho
^ permalink raw reply
* Re: [WIP Patch 04/12] Use the new http API in http_fetch_ref()
From: Johannes Schindelin @ 2009-01-18 15:10 UTC (permalink / raw)
To: Mike Hommey; +Cc: git, gitster
In-Reply-To: <1232265877-3649-5-git-send-email-mh@glandium.org>
Hi,
On Sun, 18 Jan 2009, Mike Hommey wrote:
> diff --git a/http.c b/http.c
> index 82534cf..0c9504b 100644
> --- a/http.c
> +++ b/http.c
> @@ -604,34 +604,17 @@ int http_fetch_ref(const char *base, struct ref *ref)
> {
> char *url;
> struct strbuf buffer = STRBUF_INIT;
> - struct active_request_slot *slot;
> - struct slot_results results;
> - int ret;
> + int ret = -1;
>
> url = quote_ref_url(base, ref->name);
> - slot = get_active_slot();
> - slot->results = &results;
> - curl_easy_setopt(slot->curl, CURLOPT_FILE, &buffer);
> - curl_easy_setopt(slot->curl, CURLOPT_WRITEFUNCTION, fwrite_buffer);
> - curl_easy_setopt(slot->curl, CURLOPT_HTTPHEADER, NULL);
> - curl_easy_setopt(slot->curl, CURLOPT_URL, url);
> - if (start_active_slot(slot)) {
> - run_active_slot(slot);
> - if (results.curl_result == CURLE_OK) {
> - strbuf_rtrim(&buffer);
> - if (buffer.len == 40)
> - ret = get_sha1_hex(buffer.buf, ref->old_sha1);
> - else if (!prefixcmp(buffer.buf, "ref: ")) {
> - ref->symref = xstrdup(buffer.buf + 5);
> - ret = 0;
> - } else
> - ret = 1;
> - } else {
> - ret = error("Couldn't get %s for %s\n%s",
> - url, ref->name, curl_errorstr);
> + if (http_get_strbuf(url, &buffer, HTTP_NO_CACHE) == HTTP_OK) {
> + strbuf_rtrim(&buffer);
> + if (buffer.len == 40)
> + ret = get_sha1_hex(buffer.buf, ref->old_sha1);
> + else if (!prefixcmp(buffer.buf, "ref: ")) {
> + ref->symref = xstrdup(buffer.buf + 5);
> + ret = 0;
> }
> - } else {
> - ret = error("Unable to start request");
> }
Why not keep that error?
BTW I had to scratch my head for a few seconds why you do not need to
set "else ret=1;" for the HTTP_OK case; you set ret = 1 in the beginning.
I'd rather put that back for clarity.
Ciao,
Dscho
^ permalink raw reply
* Re: [PATCH v4 0/7] customizable --color-words
From: Santi Béjar @ 2009-01-18 15:05 UTC (permalink / raw)
To: Thomas Rast
Cc: git, Junio C Hamano, Johannes Schindelin, Boyd Stephen Smith Jr.,
Teemu Likonen
In-Reply-To: <1232209788-10408-1-git-send-email-trast@student.ethz.ch>
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.
At the end I've found that a general regex that works best for me is:
"[[:alpha:]]+|[[:digit:]]+|[^[:alnum:][:space:]]"
and that is what I tested.
Santi
^ permalink raw reply
* Re: [PATCH (topgit)] tg-patch: add support for generating patches against worktree and index
From: Kirill Smelkov @ 2009-01-18 15:06 UTC (permalink / raw)
To: martin f krafft; +Cc: Petr Baudis, Git Mailing List
In-Reply-To: <20090108211149.GA19983@roro3>
On Fri, Jan 09, 2009 at 12:11:49AM +0300, Kirill Smelkov wrote:
> I'm sorry, but I've found a mistake in my code for case:
>
> diff --git a/tg-patch.sh b/tg-patch.sh
> index db1ad09..d701c54 100644
> --- a/tg-patch.sh
> +++ b/tg-patch.sh
> @@ -17,8 +17,8 @@ while [ -n "$1" ]; do
> case "$arg" in
> -i)
> topic='(i)'
> - diff_opts="$diff_opts --cached";;
> - diff_committed_only=;
> + diff_opts="$diff_opts --cached";
> + diff_committed_only=;;
> -w)
> topic='(w)'
> diff_committed_only=;;
>
>
> So here is corrected patch:
>
>
> From: Kirill Smelkov <kirr@landau.phys.spbu.ru>
> To: Petr Baudis <pasky@suse.cz>
> Cc: martin f krafft <madduck@madduck.net>
> Cc: Git Mailing List <git@vger.kernel.org>
> Subject: [PATCH (topgit)] tg-patch: add support for generating patches against worktree and index
>
> This implements `tg patch -i` and `tg patch -w` to see current patch as
> generated against not-yet-committed index and worktree.
>
>
> NOTE: unfortunately `git cat-file blob <file>` does not provide an option
> to cat file from worktree (only from an object or from index), so I had to
> unroll my own `cat file topic:file` with special support for '(i)' and
> '(w)' topics.
>
> Signed-off-by: Kirill Smelkov <kirr@landau.phys.spbu.ru>
[...]
Petr, Martin,
What's the state of this patch?
I don't understand why this gets ignored. Maybe I do something wrong?
...
Thanks,
Kirill
^ permalink raw reply
* Re: [WIP Patch 03/12] Two new functions for the http API
From: Johannes Schindelin @ 2009-01-18 15:03 UTC (permalink / raw)
To: Mike Hommey; +Cc: git, gitster
In-Reply-To: <1232265877-3649-4-git-send-email-mh@glandium.org>
Hi,
On Sun, 18 Jan 2009, Mike Hommey wrote:
> http_get_strbuf and http_get_file allow respectively to retrieve contents of
> an URL to a strbuf or an opened file handle.
Yesss! Why have you hidden them that long?
Ciao,
Dscho
^ permalink raw reply
* Re: [PATCH (topgit) 0/2] tg-patch: fix pagination
From: Kirill Smelkov @ 2009-01-18 15:04 UTC (permalink / raw)
To: git; +Cc: pasky, martin f krafft
In-Reply-To: <cover.1231254832.git.kirr@landau.phys.spbu.ru>
On Tue, Jan 06, 2009 at 06:16:37PM +0300, Kirill Smelkov wrote:
> Kirill Smelkov (2):
> Implement setup_pager just like in git
> tg-patch: fix pagination
>
> tg-patch.sh | 3 +++
> tg.sh | 53 +++++++++++++++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 56 insertions(+), 0 deletions(-)
Petr, Martin,
What's the state of this patch series?
It fixes `tg patch`, and it seems we addressed all uglyness in
setup_pager, so is there any other reason not to merge this?
Thanks,
Kirill
^ permalink raw reply
* Re: [PATCH] http-push: update memory allocation for http headers
From: Johannes Schindelin @ 2009-01-18 14:55 UTC (permalink / raw)
To: Ray Chuan; +Cc: git
In-Reply-To: <be6fef0d0901180516q19327c18oa1d7ccc36ce3a87@mail.gmail.com>
Hi,
On Sun, 18 Jan 2009, Ray Chuan wrote:
> In 753bc91 (Remove the requirement opaquelocktoken uri scheme), the
> header strings were with the removal "opaquelocktoken:" (16
> characters).
>
> Unfortunately, this was without the corresponding change in memory
> allocation. In this patch we update these allocations.
Rather than doing this (which will result in as error-prone code), why
don't you replace those things by strbufs?
Ciao,
Dscho
^ permalink raw reply
* Re: What's cooking in git.git (Jan 2009, #03; Wed, 14)
From: Kirill Smelkov @ 2009-01-18 14:54 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: Junio C Hamano, git
In-Reply-To: <alpine.DEB.1.00.0901161253411.3586@pacific.mpi-cbg.de>
On Fri, Jan 16, 2009 at 12:54:28PM +0100, Johannes Schindelin wrote:
> Hi,
>
> On Fri, 16 Jan 2009, Junio C Hamano wrote:
>
> > I thought there is somebody on this list who insists his name is of form:
> >
> > From: A U Thor (MonikeR) <a.u@thor.xz>
>
> It is Philippe Bruhat (BooK), who sometimes forgets the closing
> parenthesis, and who is listed in .mailmap without the moniker.
So now I don't understand what to do.
>From one hand RFC822 says '(...)' is a comment, and from the other hand,
we have a use case where one guy wants this to stay.
For the record, here is the questionable patch. Any suggestion?
Thanks,
Kirill
commit 49bebfbe18dac296e5e246884bd98c1f90be9676
Author: Kirill Smelkov <kirr@landau.phys.spbu.ru>
Date: Tue Jan 13 01:21:04 2009 +0300
mailinfo: more smarter removal of rfc822 comments from 'From'
As described in RFC822 (3.4.3 COMMENTS, and A.1.4.), comments, as e.g.
John (zzz) Doe <john.doe@xz> (Comment)
should "NOT [be] included in the destination mailbox"
We need this functionality to pass all RFC2047 based tests in the next commit.
Signed-off-by: Kirill Smelkov <kirr@landau.phys.spbu.ru>
diff --git a/builtin-mailinfo.c b/builtin-mailinfo.c
index dacc8ac..958c905 100644
--- a/builtin-mailinfo.c
+++ b/builtin-mailinfo.c
@@ -29,6 +29,9 @@ static struct strbuf **p_hdr_data, **s_hdr_data;
#define MAX_HDR_PARSED 10
#define MAX_BOUNDARIES 5
+static void cleanup_space(struct strbuf *sb);
+
+
static void get_sane_name(struct strbuf *out, struct strbuf *name, struct strbuf *email)
{
struct strbuf *src = name;
@@ -120,6 +123,33 @@ static void handle_from(const struct strbuf *from)
strbuf_setlen(&f, f.len - 1);
}
+ /* This still could not be finished for emails like
+ *
+ * "John (zzz) Doe <john.doe@xz> (Comment)"
+ *
+ * The email part had already been removed, so let's kill comments as
+ * well -- RFC822 says comments should not be present in destination
+ * mailbox (3.4.3. Comments and A.1.4.)
+ */
+ while (1) {
+ char *ta;
+
+ at = strchr(f.buf, '(');
+ if (!at)
+ break;
+ ta = strchr(at, ')');
+ if (!ta)
+ break;
+
+ strbuf_remove(&f, at - f.buf, ta-at + (*ta ? 1 : 0));
+ }
+
+ /* and let's finally cleanup spaces that were around (possibly
+ * internal) comments
+ */
+ cleanup_space(&f);
+ strbuf_trim(&f);
+
get_sane_name(&name, &f, &email);
strbuf_release(&f);
}
diff --git a/t/t5100/sample.mbox b/t/t5100/sample.mbox
index 38725f3..4f80b82 100644
--- a/t/t5100/sample.mbox
+++ b/t/t5100/sample.mbox
@@ -2,10 +2,10 @@
From nobody Mon Sep 17 00:00:00 2001
-From: A
+From: A (zzz)
U
Thor
- <a.u.thor@example.com>
+ <a.u.thor@example.com> (Comment)
Date: Fri, 9 Jun 2006 00:44:16 -0700
Subject: [PATCH] a commit.
^ permalink raw reply related
* Re: [PATCH] http-push: support full URI in handle_remote_ls_ctx()
From: Johannes Schindelin @ 2009-01-18 14:51 UTC (permalink / raw)
To: Kirill A. Korinskiy; +Cc: Junio C Hamano, git
In-Reply-To: <1232278116-6631-1-git-send-email-catap@catap.ru>
Hi,
On Sun, 18 Jan 2009, Kirill A. Korinskiy wrote:
> The program calls remote_ls() to get list of files from the server
> over HTTP; handle_remote_ls_ctx() is used to parse its response to
> populate "struct remote_ls_ctx" that is returned from remote_ls().
>
> The handle_remote_ls_ctx() function assumed that the server returns a
> local path in href field, but RFC 4918 (14.7) demand of support full
> URI (e.g. "http://localhost:8080/repo.git").
>
> This resulted in push failure (e.g. git-http-push issues a PROPFIND
> request to "/repo.git/alhost:8080/repo.git/refs/" to the server).
>
> Signed-off-by: Kirill A. Korinskiy <catap@catap.ru>
> ---
> http-push.c | 24 ++++++++++++++++++------
> 1 files changed, 18 insertions(+), 6 deletions(-)
>
> diff --git a/http-push.c b/http-push.c
> index 7c6460919bf3eba10c46cede11ffdd9c53fd2dd2..2cb925a9ad857b6d79858d5187f14072167282e7 100644
You mean this patch is not vs
junio/next(bbe7a8ed3dac72b7a1372cd92f68f47965c10100) or junio/master or
junio/maint(both a4b7d08663504a57008f66a39fffe293f62c1d08) but
tags/v1.6.1-rc4~13?
Ciao,
Dscho
^ permalink raw reply
* [PATCH 3/4] Documentation: mention branches rather than heads
From: Anders Melchiorsen @ 2009-01-18 14:36 UTC (permalink / raw)
To: gitster; +Cc: git, Anders Melchiorsen
In-Reply-To: <1232289418-25627-3-git-send-email-mail@cup.kalibalik.dk>
Most of the git push page talks about branches, so make it consistent
also in this paragraph.
Signed-off-by: Anders Melchiorsen <mail@cup.kalibalik.dk>
---
Documentation/git-push.txt | 4 ++--
1 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/Documentation/git-push.txt b/Documentation/git-push.txt
index 6d3c711..a7a6f4c 100644
--- a/Documentation/git-push.txt
+++ b/Documentation/git-push.txt
@@ -57,8 +57,8 @@ Pushing an empty <src> allows you to delete the <dst> ref from
the remote repository.
+
The special refspec `:` (or `+:` to allow non-fast forward updates)
-directs git to push "matching" heads: for every head that exists on
-the local side, the remote side is updated if a head of the same name
+directs git to push "matching" branches: for every branch that exists on
+the local side, the remote side is updated if a branch of the same name
already exists on the remote side. This is the default operation mode
if no explicit refspec is found (that is neither on the command line
nor in any Push line of the corresponding remotes file---see below).
--
1.6.0.2.514.g23abd3
^ permalink raw reply related
* [PATCH 2/4] Documentation: remove a redundant elaboration
From: Anders Melchiorsen @ 2009-01-18 14:36 UTC (permalink / raw)
To: gitster; +Cc: git, Anders Melchiorsen
In-Reply-To: <1232289418-25627-2-git-send-email-mail@cup.kalibalik.dk>
The comment in parentheses is wrong, as one has to leave out both the
colon and <dst>. This situation is covered by the section a few lines
down:
A parameter <ref> without a colon pushes the <ref> from the source
repository to the destination repository under the same name.
So, just remove the parentheses.
Signed-off-by: Anders Melchiorsen <mail@cup.kalibalik.dk>
---
Documentation/git-push.txt | 3 +--
1 files changed, 1 insertions(+), 2 deletions(-)
diff --git a/Documentation/git-push.txt b/Documentation/git-push.txt
index 8bfa7cb..6d3c711 100644
--- a/Documentation/git-push.txt
+++ b/Documentation/git-push.txt
@@ -44,8 +44,7 @@ tip of `master` branch); see linkgit:git-rev-parse[1]) that you
want to push. The <dst> side represents the destination location.
+
The local ref that matches <src> is used
-to fast forward the remote ref that matches <dst> (or, if no <dst> was
-specified, the same ref that <src> referred to locally). If
+to fast forward the remote ref that matches <dst>. If
the optional leading plus `+` is used, the remote ref is updated
even if it does not result in a fast forward update.
+
--
1.6.0.2.514.g23abd3
^ permalink raw reply related
* [PATCH 1/4] Documentation: git push repository can also be a remote
From: Anders Melchiorsen @ 2009-01-18 14:36 UTC (permalink / raw)
To: gitster; +Cc: git, Anders Melchiorsen
In-Reply-To: <1232289418-25627-1-git-send-email-mail@cup.kalibalik.dk>
This is copied from pull-fetch-param.txt and helps the reader
to not get stuck in the URL section.
Signed-off-by: Anders Melchiorsen <mail@cup.kalibalik.dk>
---
Documentation/git-push.txt | 4 +++-
1 files changed, 3 insertions(+), 1 deletions(-)
diff --git a/Documentation/git-push.txt b/Documentation/git-push.txt
index 3321966..8bfa7cb 100644
--- a/Documentation/git-push.txt
+++ b/Documentation/git-push.txt
@@ -28,7 +28,9 @@ OPTIONS
-------
<repository>::
The "remote" repository that is destination of a push
- operation. See the section <<URLS,GIT URLS>> below.
+ operation. This parameter can be either a URL
+ (see the section <<URLS,GIT URLS>> below) or the name
+ of a remote (see the section <<REMOTES,REMOTES>> below).
<refspec>...::
The canonical format of a <refspec> parameter is
--
1.6.0.2.514.g23abd3
^ permalink raw reply related
* [PATCH 4/4] Documentation: avoid using undefined parameters
From: Anders Melchiorsen @ 2009-01-18 14:36 UTC (permalink / raw)
To: gitster; +Cc: git, Anders Melchiorsen
In-Reply-To: <1232289418-25627-4-git-send-email-mail@cup.kalibalik.dk>
The <ref> parameter has not been introduced, so rewrite to
avoid it.
Signed-off-by: Anders Melchiorsen <mail@cup.kalibalik.dk>
---
Documentation/git-push.txt | 4 ++--
1 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/Documentation/git-push.txt b/Documentation/git-push.txt
index a7a6f4c..7b27dc6 100644
--- a/Documentation/git-push.txt
+++ b/Documentation/git-push.txt
@@ -50,8 +50,8 @@ even if it does not result in a fast forward update.
+
`tag <tag>` means the same as `refs/tags/<tag>:refs/tags/<tag>`.
+
-A parameter <ref> without a colon pushes the <ref> from the source
-repository to the destination repository under the same name.
+A lonely <src> parameter (without a colon and a destination) pushes
+the <src> to the same name in the destination repository.
+
Pushing an empty <src> allows you to delete the <dst> ref from
the remote repository.
--
1.6.0.2.514.g23abd3
^ permalink raw reply related
* [PATCH 0/4] Clarifying the refspec documentation
From: Anders Melchiorsen @ 2009-01-18 14:36 UTC (permalink / raw)
To: gitster; +Cc: git, Anders Melchiorsen
This is a partial resend of some small updates to the git-push
refspec documentation. I have removed the parts of the patch
series that had comments last time, and will rework those later.
Meanwhile, I think that these simple modification do help in
making things more readable. Once you understand the refspec, it
is hard to realize how hard those few paragraphs are to digest.
Anders Melchiorsen (4):
Documentation: git push repository can also be a remote
Documentation: remove a redundant elaboration
Documentation: mention branches rather than heads
Documentation: avoid using undefined parameters
Documentation/git-push.txt | 15 ++++++++-------
1 files changed, 8 insertions(+), 7 deletions(-)
^ permalink raw reply
* Re: [PATCH] http-push: update memory allocation for http headers
From: Mike Hommey @ 2009-01-18 14:20 UTC (permalink / raw)
To: Ray Chuan; +Cc: git
In-Reply-To: <be6fef0d0901180516q19327c18oa1d7ccc36ce3a87@mail.gmail.com>
On Sun, Jan 18, 2009 at 09:16:04PM +0800, Ray Chuan wrote:
> In 753bc91 (Remove the requirement opaquelocktoken uri scheme), the
> header strings were with the removal "opaquelocktoken:" (16
> characters).
>
> Unfortunately, this was without the corresponding change in memory
> allocation. In this patch we update these allocations.
>
> Signed-off-by: Tay Ray Chuan <rctay89@gmail.com>
> ---
It would be better to just turn the whole thing to use strbuf, and/or
extend the patch set I sent earlier today to handle webdav.
Mike
^ permalink raw reply
* Re: is gitosis secure?
From: Boyd Stephen Smith Jr. @ 2009-01-18 14:19 UTC (permalink / raw)
To: Florian Weimer; +Cc: git
In-Reply-To: <87skngoifj.fsf@mid.deneb.enyo.de>
[-- Attachment #1: Type: text/plain, Size: 2114 bytes --]
On Sunday 18 January 2009, Florian Weimer <fw@deneb.enyo.de> wrote
about 'Re: is gitosis secure?':
>* Boyd Stephen Smith, Jr.:
>> On Sunday 18 January 2009, Florian Weimer <fw@deneb.enyo.de> wrote
>>
>> about 'Re: is gitosis secure?':
>>>* Sam Vilain:
>>>> Restricted unix shells are a technology which has been proven secure
>>>> for decades now.
>>>Huh? Things like scponly and rssh had their share of bugs, so I can
>>>see that there is some concern. (And restricted shells used to be
>>
>> From my understanding, a restricted shell is a difficult thing to
>> escape from unless a user is able to run binaries that they have
>> written. FWIW, I don't remember sftp or scponly having this particular
>> vulnerability.
>
>scponly issues due to interpretation conflicts:
Not sure all these apply, but I beleive some of them do, and I want to
leave the CVE numbers in case someone wants to look them up.
>CVE-2002-1469
>CVE-2004-1162
>CVE-2005-4533
>CVE-2007-6350
>CVE-2007-6415
>CVE-2004-1161
--- End of CVEs to investigate ---
>That's why I think it's not totally outlandish to assume that
>restricted shells are usually not very helpful for
>compartmentalization purposes.
I mostly agree with that statement. I make the assumption that, if the
user can login via ssh (even under "only" a restricted shell) they can do
anything a user in the same groups can do. I might be overestimating most
people, but I don't think I'm underestimating anyone. I do *hope* that I
get local privilege escalations patched before they are exploited, but I
can't guarantee that. (I'm not sure there's really anyway to guarantee
that, and I'd hate to upgrade a backup offline then replace the running
instance. Especially if I had to go back to when the local privilege
escalation was introduced [not just when it was "discovered"].)
--
Boyd Stephen Smith Jr. ,= ,-_-. =.
bss@iguanasuicide.net ((_/)o o(\_))
ICQ: 514984 YM/AIM: DaTwinkDaddy `-'(. .)`-'
http://iguanasuicide.net/ \_/
[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 197 bytes --]
^ permalink raw reply
* Re: [PATCH] mergetool: put the cursor on the editable file for Vim
From: SZEDER Gábor @ 2009-01-18 14:18 UTC (permalink / raw)
To: Junio C Hamano; +Cc: markus.heidelberg, git
In-Reply-To: <7v7i4t4crp.fsf@gitster.siamese.dyndns.org>
On Sat, Jan 17, 2009 at 05:35:22PM -0800, Junio C Hamano wrote:
> Markus Heidelberg <markus.heidelberg@web.de> writes:
>
> > When resolving conflicts, you only need to edit the $MERGED file. Put
> > the cursor automatically into its window for vimdiff and gvimdiff to
> > avoid doing <C-w>l every time.
>
> I think this is sensible.
>
> I do not use vim, and I do not know if the patch does what it claims to
> do, though.
It does.
Tested-by: SZEDER Gábor <szeder@ira.uka.de>
^ permalink raw reply
* Re: [PATCH] revision walker: include a detached HEAD in --all
From: Johannes Schindelin @ 2009-01-18 14:06 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Thomas Rast, Johannes Sixt, git
In-Reply-To: <7v3afh15pi.fsf@gitster.siamese.dyndns.org>
Hi,
On Sat, 17 Jan 2009, Junio C Hamano wrote:
> Subject: [PATCH] bundle: allow the same ref to be given more than once
>
> "git bundle create x master master" used to create a bundle that lists
> the same branch (master) twice. Cloning from such a bundle resulted in
> a needless warning "warning: Duplicated ref: refs/remotes/origin/master".
>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
> bundle.c | 2 ++
> object.c | 19 +++++++++++++++++++
> object.h | 1 +
> 3 files changed, 22 insertions(+), 0 deletions(-)
Yes, that would be good. You have my ACK on that if you want.
Another thing I am thinking about on and off:
You cannot really use bundles as a replacement for regular transports
(e.g. when you administrator does not let you ssh or git:// out, and you
do not have an HTTP server available [*1*]).
Suppose you have two branches, 'master' and 'side'. Now you make changes
to 'master' and send the complete repository as a bundle to your friend.
Now you delete the branch 'side', and send the next bundle (created with
--all implying HEAD, and --since=$(stat -c %Y <first-bundle>)).
Then your friend has no idea if 'side' was deleted or untouched.
So I think we'd need some option for "create bundle" to list all specified
refs, and if they have not really changed, their SHA-1s as prerequisites,
too. Maybe "--full-bundle", or just "--full"?
Another problem: suppose you have a branch, called 'private', that you
excluded from your bundles. Now you happened to make changes to it, and
by mistake, the branch gets included in the incremental bundle. No
problem for you, as your friend lacks the prerequisites to reconstruct it.
But unfortunately, your friend cannot even pull 'master' from it, because
of our overzealous verification process which refuses all fetches when
some prerequisites are missing locally, even if they are not even needed.
This problem is much harder to solve, I think, and maybe we just want to
leave it: as bundles are just fed into index-pack --fix-thin, which has no
idea what objects can be skipped. Maybe there is no clean solution to
that to begin with.
Ciao,
Dscho
[*1*] When people are stuck behind such a stupidly restrictive firewall,
often people come with "helpful" suggestions to use a VPN, or to publish
your private repository, or get an external machine to HTTP proxy their
connections. I find it outright mean to waste the time of people who
already have a big problem.
However, I believe that a mail based bundle exchange should be a
relatively easy way out for those situations, once it works.
^ permalink raw reply
* Re: [PATCH] revision walker: include a detached HEAD in --all
From: Johannes Schindelin @ 2009-01-18 13:42 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Thomas Rast, Johannes Sixt, git
In-Reply-To: <7v8wp917c3.fsf@gitster.siamese.dyndns.org>
Hi,
On Sat, 17 Jan 2009, Junio C Hamano wrote:
> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>
> > When HEAD is detached, --all should list it, too, logically, as a
> > detached HEAD is by definition a temporary, unnamed branch.
> >
> > It is especially necessary to list it when garbage collecting, as
> > the detached HEAD would be trashed.
> >
> > Noticed by Thomas Rast.
> >
> > Note that this affects creating bundles with --all; I contend that it
> > is a good change to add the HEAD, so that cloning from such a bundle
> > will give you a current branch. However, I had to fix t5701 as it
> > assumed that --all does not imply HEAD.
>
> Sorry, but I do not understand.
>
> > diff --git a/t/t5701-clone-local.sh b/t/t5701-clone-local.sh
> > index 8dfaaa4..14413f8 100755
> > --- a/t/t5701-clone-local.sh
> > +++ b/t/t5701-clone-local.sh
> > @@ -11,8 +11,8 @@ test_expect_success 'preparing origin repository' '
> > git clone --bare . x &&
> > test "$(GIT_CONFIG=a.git/config git config --bool core.bare)" = true &&
> > test "$(GIT_CONFIG=x/config git config --bool core.bare)" = true
> > - git bundle create b1.bundle --all HEAD &&
> > - git bundle create b2.bundle --all &&
> > + git bundle create b1.bundle master HEAD &&
> > + git bundle create b2.bundle master &&
>
> Because --all did not imply HEAD, "--all HEAD" used to be the way to say
> "everything and HEAD". Now --all does imply HEAD, but it should still be
> a valid way to say "everything, by the way, do not forget HEAD".
>
> Does the first one need to be changed to "master HEAD"? If "--all HEAD"
> makes the rest of the test unhappy because HEAD is listed twice, perhaps
> that is an independent bug that needs to be fixed?
I changed it away from --all because I am a fan of being explicit. We
want a bundle here that has master and HEAD in it. This being a test
case, being lazy is so wrong here. You should describe what you actually
want, not use a set of parameters that just happens to work (by chance as
we saw).
Ciao,
Dscho
^ permalink raw reply
* Re: [PATCH 2/2] http-push: remove MOVE step after PUT when sending objects to server
From: Johannes Schindelin @ 2009-01-18 13:32 UTC (permalink / raw)
To: Ray Chuan; +Cc: Junio C Hamano, git
In-Reply-To: <be6fef0d0901171919ub28dda7ref6443abec3627aa@mail.gmail.com>
Hi,
On Sun, 18 Jan 2009, Ray Chuan wrote:
> On Sun, Jan 18, 2009 at 12:48 AM, Junio C Hamano <gitster@pobox.com> wrote:
>
> > "Ray Chuan" <rctay89@gmail.com> writes:
> >
> >> A concern raised was repository corruption in the event of failure
> >> during PUT. "put && move" won't afford any more protection than using
> >> simply "put", since info/refs is not updated if a PUT fails, so there
> >> is no cause for concern.
> >
> > That's a completely bogus reasoning. Normal operation inside the
> > repository that was pushed into won't look at info/refs at all.
>
> i mentioned this "repository corruption" issue as it was raised
> previously by Johannes (towards the bottom):
>
> http://article.gmane.org/gmane.comp.version-control.git/106031
,.. and Junio just raised a new concern. The repository as it is on the
server could very well be served by other means, i.e. git:// and rsync://,
and there could be cron jobs and interactive Git sessions trying to work
with it.
The point is: the repository inside the document root of the web server is
still a valid repository.
And the assumption is that whenever you have a file that looks like a
valid object/pack inside a valid repository, that it does not need
replacing.
So even when optimizing the uncommon (HTTP push is 2nd class citizen), we
have to keep the common workflow intact (1st class citizens _are_ push by
file system, ssh or git://).
Which unfortunately means that put && move must stay.
The same reasoning explains why http-push has to ignore the info/refs file
when looking for common refs, BTW.
Ciao,
Dscho
^ permalink raw reply
* Re: is gitosis secure?
From: Florian Weimer @ 2009-01-18 13:25 UTC (permalink / raw)
To: Boyd Stephen Smith Jr.; +Cc: git
In-Reply-To: <200901180650.06605.bss@iguanasuicide.net>
* Boyd Stephen Smith, Jr.:
> On Sunday 18 January 2009, Florian Weimer <fw@deneb.enyo.de> wrote
> about 'Re: is gitosis secure?':
>>* Sam Vilain:
>>> Restricted unix shells are a technology which has been proven secure
>>> for decades now.
>>Huh? Things like scponly and rssh had their share of bugs, so I can
>>see that there is some concern. (And restricted shells used to be
>>circumvented by things like Netscape's print dialog.)
>
> From my understanding, a restricted shell is a difficult thing to escape
> from unless a user is able to run binaries that they have written. FWIW,
> I don't remember sftp or scponly having this particular vulnerability.
scponly issues due to interpretation conflicts:
CVE-2002-1469 scponly does not properly verify the path when finding the (1) scp or ...
CVE-2004-1162 The unison command in scponly before 4.0 does not properly restrict ...
CVE-2005-4533 Argument injection vulnerability in scponlyc in scponly 4.1 and ...
CVE-2007-6350 scponly 4.6 and earlier allows remote authenticated users to bypass ...
CVE-2007-6415 scponly 4.6 and earlier allows remote authenticated users to bypass ...
rssh has fewer such issues, only CVE-2004-1161 seems to be intrinsic
to the program's purpose (but some of the other issues might be used
as circumvention devices, too).
That's why I think it's not totally outlandish to assume that
restricted shells are usually not very helpful for
compartmentalization purposes.
^ permalink raw reply
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