Re: [PATCH 1/2] pack-refs: write peeled entry for non-tags
On Sat, Mar 16, 2013 at 02:50:56PM +0100, Michael Haggerty wrote: @@ -39,14 +40,13 @@ static int handle_one_ref(const char *path, const unsigned char *sha1, return 0; fprintf(cb-refs_file, %s %s\n, sha1_to_hex(sha1), path); - if (is_tag_ref) { - struct object *o = parse_object(sha1); - if (o-type == OBJ_TAG) { - o = deref_tag(o, path, 0); - if (o) - fprintf(cb-refs_file, ^%s\n, - sha1_to_hex(o-sha1)); - } + + o = parse_object(sha1); + if (o-type == OBJ_TAG) { You suggested that I add a test (o != NULL) at the equivalent place in my code (which was derived from this code). Granted, my code was explicitly intending to pass invalid SHA1 values to parse_object(). But wouldn't it be a good defensive step to add the same check here? Hmm, yeah. That is not new code, but rather just reindented from above (diff -w makes it much more obvious what is going on). It is probably worth dying rather than segfaulting, though it should be a separate patch (and I do not think it is sane to do anything except die here). I almost wonder if parse_object should die by default on bogus or missing objects, and the few callers who really want to handle the error can call parse_object_gently. I do not relish analyzing each caller, though. It would be simpler to add parse_object_or_die. +# This matches show-ref's output +print_ref() { + echo `git rev-parse $1` $1 +} + CodingGuidelines prefers $() over ``. Old habits die hard. :) I'll re-roll with your suggestions in a moment. -Peff -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] pack-refs: add fully-peeled trait
On Sat, Mar 16, 2013 at 03:06:22PM +0100, Michael Haggerty wrote: refname = parse_ref_line(refline, sha1); if (refname) { - last = create_ref_entry(refname, sha1, flag, 1); + /* +* Older git did not write peel lines for anything +* outside of refs/tags/; if the fully-peeled trait +* is not set, we are dealing with such an older +* git and cannot assume an omitted peel value +* means the ref is not a tag object. +*/ + int this_flag = flag; + if (!fully_peeled prefixcmp(refname, refs/tags/)) + this_flag = ~REF_KNOWS_PEELED; + + last = create_ref_entry(refname, sha1, this_flag, 1); add_ref(dir, last); continue; } I have to admit that I am partial to my variant of this code [1] because the logic makes it clearer when the affirmative decision can be made to set the REF_KNOWS_PEELED flag. But this version also looks correct to me and equivalent (aside from the idea that a few lines later if a peeled value is found then the REF_KNOWS_PEELED bit could also be set). Yeah, I think they are equivalent, but I agree yours is a little more readable. I'll switch it in my re-roll, and I will go ahead and set the REF_KNOWS_PEELED bit when we see a peel line. That code should not be triggered in general, but it is the sane thing for the reader to do, so it makes the code more obvious and readable. -Peff -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC/PATCH] Introduce remote.pushdefault
Ramkumar Ramachandra artag...@gmail.com writes: How will adding remote.pushdefault have any impact, unless I explicitly remove this branch-specific remote configuration? Besides, without branch.name.remote configured, I can't even pull and expect changes to be merged. If the triangle topology is the norm for your project, I would expect that it would be pretty common to have all of your branches pull from one place and have all of them push to another place. In the central repository workflow, it is very common that all of your branches pull from one place and all of them push to the same place. In the bigger picture, forcing to set the branch specific remote is a mistake in the first place; in the longer term, you should be able to say My project uses the central repository workflow once and you should be able to pull without branch.master.remote; just record where the default origin for all branches is. I think the per-branch branch.*.pushremote is actively making things worse by repeating the same mistake for the triangle topology. You should be able to say My project uses the triangle workflow once and specify two remotes, where you pull from and where you push to, no? A per-branch override, be it branch.*.remote or brnach.*.pushremote, is useful to give an escape hatch in order to handle special cases with additional flexibility. But making it the sole mechanism and forcing the user to repeat the same which remote does it use all over the place does not sound like a solid engineering. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/3] fix unparsed object access in upload-pack
Jeff King p...@peff.net writes: [3/3]: upload-pack: load non-tip want objects from disk While investigating the bug, I found some weirdness around the stateless-rpc check_non_tip code. As far as I can tell, that code never actually gets triggered. It's not too surprising that we wouldn't have noticed, because it is about falling back due to a race condition. But please sanity check my explanation and patch. Thanks. That fall-back is Shawn's doing and I suspect that nobody is exercising the codepath (he isn't). I almost wonder if we should cut it out entirely. It is definitely a possible race condition, but I wonder if anybody actually hits it in practice (and if they do, the consequence is that the fetch fails and needs to be retried). As far as I can tell, the code path has never actually been followed, and I do not recall ever seeing a bug report or complaint about it (though perhaps it happened once, which spurred the initial development?). If you run multiple servers serving the same repository at the same URL with a small mirroring lag, one may observe a set of refs from one server, that are a tad older than the other server you actually fetch from. k.org may have such an arrangement, but does GitHub serve the same repository on multiple machines without tying the same client to the same backend? -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Preallocate hash tables when the number of inserts are known in advance
Duy Nguyen pclo...@gmail.com writes: On Sun, Mar 17, 2013 at 12:34 PM, Junio C Hamano gits...@pobox.com wrote: Nguyễn Thái Ngọc Duy pclo...@gmail.com writes: This avoids unnecessary re-allocations and reinsertions. On webkit.git (i.e. about 182k inserts to the name hash table), this reduces about 100ms out of 3s user time. Signed-off-by: Nguyễn Thái Ngọc Duy pclo...@gmail.com I think this is a very good idea, but I would prefer the second parameter to the preallocate to be expected number of entries and have the preallocate, which is a part of the hash API, decide how to inflate that number to adjust to the desired load factor of the hash table. We shouldn't have to adjust the caller when the internal implementation of the hash table changes. OK will do. I've squashed it in myself, so no need to resend only for this. diff --git a/diffcore-rename.c b/diffcore-rename.c index 8d3d9bb..6c7a72f 100644 --- a/diffcore-rename.c +++ b/diffcore-rename.c @@ -389,7 +389,7 @@ static int find_exact_renames(struct diff_options *options) struct hash_table file_table; init_hash(file_table); - preallocate_hash(file_table, (rename_src_nr + rename_dst_nr) * 2); + preallocate_hash(file_table, rename_src_nr + rename_dst_nr); for (i = 0; i rename_src_nr; i++) insert_file_table(file_table, -1, i, rename_src[i].p-one); diff --git a/hash.h b/hash.h index 244d1fe..1d43ac0 100644 --- a/hash.h +++ b/hash.h @@ -40,11 +40,11 @@ static inline void init_hash(struct hash_table *table) table-array = NULL; } -static inline void preallocate_hash(struct hash_table *table, unsigned int size) +static inline void preallocate_hash(struct hash_table *table, unsigned int elts) { assert(table-size == 0 table-nr == 0 table-array == NULL); - table-size = size; - table-array = xcalloc(sizeof(struct hash_table_entry), size); + table-size = elts * 2; + table-array = xcalloc(sizeof(struct hash_table_entry), table-size); } #endif diff --git a/name-hash.c b/name-hash.c index 90c7b99..2a1f108 100644 --- a/name-hash.c +++ b/name-hash.c @@ -93,7 +93,7 @@ static void lazy_init_name_hash(struct index_state *istate) if (istate-name_hash_initialized) return; if (istate-cache_nr) - preallocate_hash(istate-name_hash, istate-cache_nr * 2); + preallocate_hash(istate-name_hash, istate-cache_nr); for (nr = 0; nr istate-cache_nr; nr++) hash_index_entry(istate, istate-cache[nr]); istate-name_hash_initialized = 1; -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v1 22/45] archive: convert to use parse_pathspec
Duy Nguyen pclo...@gmail.com writes: No, the literal strings are reparsed in path_exists() before being fed to read_tree_recursive. Yuck. OK. Thanks. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] sha1_name: pass object name length to diagnose_invalid_sha1_path()
René Scharfe rene.scha...@lsrfire.ath.cx writes: The only caller of diagnose_invalid_sha1_path() extracts a substring from an object name by creating a NUL-terminated copy of the interesting part. Add a length parameter to the function and thus avoid the need for an allocation, thereby simplifying the code. Signed-off-by: Rene Scharfe rene.scha...@lsrfire.ath.cx --- sha1_name.c | 32 ++-- 1 file changed, 14 insertions(+), 18 deletions(-) diff --git a/sha1_name.c b/sha1_name.c index 95003c7..4cea6d3 100644 --- a/sha1_name.c +++ b/sha1_name.c @@ -1137,7 +1137,8 @@ int get_sha1_blob(const char *name, unsigned char *sha1) static void diagnose_invalid_sha1_path(const char *prefix, const char *filename, const unsigned char *tree_sha1, -const char *object_name) +const char *object_name, +int object_name_len) { struct stat st; unsigned char sha1[20]; @@ -1147,8 +1148,8 @@ static void diagnose_invalid_sha1_path(const char *prefix, prefix = ; if (!lstat(filename, st)) - die(Path '%s' exists on disk, but not in '%s'., - filename, object_name); + die(Path '%s' exists on disk, but not in '%.*s'., + filename, object_name_len, object_name); if (errno == ENOENT || errno == ENOTDIR) { char *fullname = xmalloc(strlen(filename) + strlen(prefix) + 1); @@ -1158,16 +1159,16 @@ static void diagnose_invalid_sha1_path(const char *prefix, if (!get_tree_entry(tree_sha1, fullname, sha1, mode)) { die(Path '%s' exists, but not '%s'.\n - Did you mean '%s:%s' aka '%s:./%s'?, + Did you mean '%.*s:%s' aka '.*%.*s:./%s'?, This is so unlike what I call Scharfe patch, which I can apply with my eyes closed and expect everything to be perfect. Other than that, I see this as a usual Scharfe patch ;-) Will squash an obvious fix in and apply. Thanks. fullname, filename, - object_name, + object_name_len, object_name, fullname, - object_name, + object_name_len, object_name, filename); } - die(Path '%s' does not exist in '%s', - filename, object_name); + die(Path '%s' does not exist in '%.*s', + filename, object_name_len, object_name); } } @@ -1332,13 +1333,8 @@ static int get_sha1_with_context_1(const char *name, } if (*cp == ':') { unsigned char tree_sha1[20]; - char *object_name = NULL; - if (only_to_die) { - object_name = xmalloc(cp-name+1); - strncpy(object_name, name, cp-name); - object_name[cp-name] = '\0'; - } - if (!get_sha1_1(name, cp-name, tree_sha1, GET_SHA1_TREEISH)) { + int len = cp - name; + if (!get_sha1_1(name, len, tree_sha1, GET_SHA1_TREEISH)) { const char *filename = cp+1; char *new_filename = NULL; @@ -1348,8 +1344,8 @@ static int get_sha1_with_context_1(const char *name, ret = get_tree_entry(tree_sha1, filename, sha1, oc-mode); if (ret only_to_die) { diagnose_invalid_sha1_path(prefix, filename, -tree_sha1, object_name); - free(object_name); +tree_sha1, +name, len); } hashcpy(oc-tree, tree_sha1); strncpy(oc-path, filename, @@ -1360,7 +1356,7 @@ static int get_sha1_with_context_1(const char *name, return ret; } else { if (only_to_die) - die(Invalid object name '%s'., object_name); + die(Invalid object name '%.*s'., len, name); } } return ret; -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 0/4] peel-ref optimization fixes
Here's a re-roll that takes into account the feedback from round 1: [1/4]: avoid segfaults on parse_object failure [2/4]: use parse_object_or_die instead of die(bad object) These two patches are new; they are conceptually independent of the rest of the series, but there's a textual dependency in later patches. [3/4]: pack-refs: write peeled entry for non-tags Same as before, but rebased on patch 1, and s/``/$()/. [4/4]: pack-refs: add fully-peeled trait Rewritten using Michael's approach, which is more readable. -Peff -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 1/4] avoid segfaults on parse_object failure
Many call-sites of parse_object assume that they will get a non-NULL return value; this is not the case if we encounter an error while parsing the object. This patch adds a wrapper function around parse_object that handles dying automatically, and uses it anywhere we immediately try to access the return value as a non-NULL pointer (i.e., anywhere that we would currently segfault). This wrapper may also be useful in other places. The most obvious one is code like: o = parse_object(sha1); if (!o) die(...); However, these should not be mechanically converted to parse_object_or_die, as the die message is sometimes customized. Later patches can address these sites on a case-by-case basis. Signed-off-by: Jeff King p...@peff.net --- bundle.c| 6 +++--- object.c| 10 ++ object.h| 13 - pack-refs.c | 2 +- 4 files changed, 26 insertions(+), 5 deletions(-) diff --git a/bundle.c b/bundle.c index 8d12816..26ceebd 100644 --- a/bundle.c +++ b/bundle.c @@ -279,12 +279,12 @@ int create_bundle(struct bundle_header *header, const char *path, if (buf.len 0 buf.buf[0] == '-') { write_or_die(bundle_fd, buf.buf, buf.len); if (!get_sha1_hex(buf.buf + 1, sha1)) { - struct object *object = parse_object(sha1); + struct object *object = parse_object_or_die(sha1, buf.buf); object-flags |= UNINTERESTING; add_pending_object(revs, object, xstrdup(buf.buf)); } } else if (!get_sha1_hex(buf.buf, sha1)) { - struct object *object = parse_object(sha1); + struct object *object = parse_object_or_die(sha1, buf.buf); object-flags |= SHOWN; } } @@ -361,7 +361,7 @@ int create_bundle(struct bundle_header *header, const char *path, * end up triggering empty bundle * error. */ - obj = parse_object(sha1); + obj = parse_object_or_die(sha1, e-name); obj-flags |= SHOWN; add_pending_object(revs, obj, e-name); } diff --git a/object.c b/object.c index 4af3451..20703f5 100644 --- a/object.c +++ b/object.c @@ -185,6 +185,16 @@ struct object *parse_object_buffer(const unsigned char *sha1, enum object_type t return obj; } +struct object *parse_object_or_die(const unsigned char *sha1, + const char *name) +{ + struct object *o = parse_object(sha1); + if (o) + return o; + + die(_(unable to parse object: %s), name ? name : sha1_to_hex(sha1)); +} + struct object *parse_object(const unsigned char *sha1) { unsigned long size; diff --git a/object.h b/object.h index 6a97b6b..97d384b 100644 --- a/object.h +++ b/object.h @@ -54,9 +54,20 @@ struct object *parse_object(const unsigned char *sha1); extern void *create_object(const unsigned char *sha1, int type, void *obj); -/** Returns the object, having parsed it to find out what it is. **/ +/* + * Returns the object, having parsed it to find out what it is. + * + * Returns NULL if the object is missing or corrupt. + */ struct object *parse_object(const unsigned char *sha1); +/* + * Like parse_object, but will die() instead of returning NULL. If the + * name parameter is not NULL, it is included in the error message + * (otherwise, the sha1 hex is given). + */ +struct object *parse_object_or_die(const unsigned char *sha1, const char *name); + /* Given the result of read_sha1_file(), returns the object after * parsing it. eaten_p indicates if the object has a borrowed copy * of buffer and the caller should not free() it. diff --git a/pack-refs.c b/pack-refs.c index f09a054..6a689f3 100644 --- a/pack-refs.c +++ b/pack-refs.c @@ -40,7 +40,7 @@ static int handle_one_ref(const char *path, const unsigned char *sha1, fprintf(cb-refs_file, %s %s\n, sha1_to_hex(sha1), path); if (is_tag_ref) { - struct object *o = parse_object(sha1); + struct object *o = parse_object_or_die(sha1, path); if (o-type == OBJ_TAG) { o = deref_tag(o, path, 0); if (o) -- 1.8.2.rc2.7.gef06216 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 2/4] use parse_object_or_die instead of die(bad object)
Some call-sites do: o = parse_object(sha1); if (!o) die(bad object %s, some_name); We can now handle that as a one-liner, and get more consistent output. In the third case of this patch, it looks like we are losing information, as the existing message also outputs the sha1 hex; however, parse_object will already have written a more specific complaint about the sha1, so there is no point in repeating it here. Signed-off-by: Jeff King p...@peff.net --- builtin/grep.c | 4 +--- builtin/prune.c | 4 +--- reachable.c | 4 +--- 3 files changed, 3 insertions(+), 9 deletions(-) diff --git a/builtin/grep.c b/builtin/grep.c index 8025964..159e65d 100644 --- a/builtin/grep.c +++ b/builtin/grep.c @@ -820,9 +820,7 @@ int cmd_grep(int argc, const char **argv, const char *prefix) unsigned char sha1[20]; /* Is it a rev? */ if (!get_sha1(arg, sha1)) { - struct object *object = parse_object(sha1); - if (!object) - die(_(bad object %s), arg); + struct object *object = parse_object_or_die(sha1, arg); if (!seen_dashdash) verify_non_filename(prefix, arg); add_object_array(object, arg, list); diff --git a/builtin/prune.c b/builtin/prune.c index 8cb8b91..85843d4 100644 --- a/builtin/prune.c +++ b/builtin/prune.c @@ -149,9 +149,7 @@ int cmd_prune(int argc, const char **argv, const char *prefix) const char *name = *argv++; if (!get_sha1(name, sha1)) { - struct object *object = parse_object(sha1); - if (!object) - die(bad object: %s, name); + struct object *object = parse_object_or_die(sha1, name); add_pending_object(revs, object, ); } else diff --git a/reachable.c b/reachable.c index bf79706..e7e6a1e 100644 --- a/reachable.c +++ b/reachable.c @@ -152,11 +152,9 @@ static int add_one_ref(const char *path, const unsigned char *sha1, int flag, vo static int add_one_ref(const char *path, const unsigned char *sha1, int flag, void *cb_data) { - struct object *object = parse_object(sha1); + struct object *object = parse_object_or_die(sha1, path); struct rev_info *revs = (struct rev_info *)cb_data; - if (!object) - die(bad object ref: %s:%s, path, sha1_to_hex(sha1)); add_pending_object(revs, object, ); return 0; -- 1.8.2.rc2.7.gef06216 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 3/4] pack-refs: write peeled entry for non-tags
When we pack an annotated tag ref, we write not only the sha1 of the tag object along with the ref, but also the sha1 obtained by peeling the tag. This lets readers of the pack-refs file know the peeled value without having to actually load the object, speeding up upload-pack's ref advertisement. The writer marks a packed-refs file with peeled refs using the peeled trait at the top of the file. When the reader sees this trait, it knows that each ref is either followed by its peeled value, or it is not an annotated tag. However, there is a mismatch between the assumptions of the reader and writer. The writer will only peel refs under refs/tags, but the reader does not know this; it will assume a ref without a peeled value must not be a tag object. Thus an annotated tag object placed outside of the refs/tags hierarchy will not have its peeled value printed by upload-pack. The simplest way to fix this is to start writing peel values for all refs. This matches what the reader expects for both new and old versions of git. Signed-off-by: Jeff King p...@peff.net --- pack-refs.c | 16 t/t3211-peel-ref.sh | 42 ++ 2 files changed, 50 insertions(+), 8 deletions(-) create mode 100755 t/t3211-peel-ref.sh diff --git a/pack-refs.c b/pack-refs.c index 6a689f3..ebde785 100644 --- a/pack-refs.c +++ b/pack-refs.c @@ -27,6 +27,7 @@ static int handle_one_ref(const char *path, const unsigned char *sha1, int flags, void *cb_data) { struct pack_refs_cb_data *cb = cb_data; + struct object *o; int is_tag_ref; /* Do not pack the symbolic refs */ @@ -39,14 +40,13 @@ static int handle_one_ref(const char *path, const unsigned char *sha1, return 0; fprintf(cb-refs_file, %s %s\n, sha1_to_hex(sha1), path); - if (is_tag_ref) { - struct object *o = parse_object_or_die(sha1, path); - if (o-type == OBJ_TAG) { - o = deref_tag(o, path, 0); - if (o) - fprintf(cb-refs_file, ^%s\n, - sha1_to_hex(o-sha1)); - } + + o = parse_object_or_die(sha1, path); + if (o-type == OBJ_TAG) { + o = deref_tag(o, path, 0); + if (o) + fprintf(cb-refs_file, ^%s\n, + sha1_to_hex(o-sha1)); } if ((cb-flags PACK_REFS_PRUNE) !do_not_prune(flags)) { diff --git a/t/t3211-peel-ref.sh b/t/t3211-peel-ref.sh new file mode 100755 index 000..85f09be --- /dev/null +++ b/t/t3211-peel-ref.sh @@ -0,0 +1,42 @@ +#!/bin/sh + +test_description='tests for the peel_ref optimization of packed-refs' +. ./test-lib.sh + +test_expect_success 'create annotated tag in refs/tags' ' + test_commit base + git tag -m annotated foo +' + +test_expect_success 'create annotated tag outside of refs/tags' ' + git update-ref refs/outside/foo refs/tags/foo +' + +# This matches show-ref's output +print_ref() { + echo $(git rev-parse $1) $1 +} + +test_expect_success 'set up expected show-ref output' ' + { + print_ref refs/heads/master + print_ref refs/outside/foo + print_ref refs/outside/foo^{} + print_ref refs/tags/base + print_ref refs/tags/foo + print_ref refs/tags/foo^{} + } expect +' + +test_expect_success 'refs are peeled outside of refs/tags (loose)' ' + git show-ref -d actual + test_cmp expect actual +' + +test_expect_success 'refs are peeled outside of refs/tags (packed)' ' + git pack-refs --all + git show-ref -d actual + test_cmp expect actual +' + +test_done -- 1.8.2.rc2.7.gef06216 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 4/4] pack-refs: add fully-peeled trait
From: Michael Haggerty mhag...@alum.mit.edu Older versions of pack-refs did not write peel lines for refs outside of refs/tags. This meant that on reading the pack-refs file, we might set the REF_KNOWS_PEELED flag for such a ref, even though we do not know anything about its peeled value. The previous commit updated the writer to always peel, no matter what the ref is. That means that packed-refs files written by newer versions of git are fine to be read by both old and new versions of git. However, we still have the problem of reading packed-refs files written by older versions of git, or by other implementations which have not yet learned the same trick. The simplest fix would be to always unset the REF_KNOWS_PEELED flag for refs outside of refs/tags that do not have a peel line (if it has a peel line, we know it is valid, but we cannot assume a missing peel line means anything). But that loses an important optimization, as upload-pack should not need to load the object pointed to by refs/heads/foo to determine that it is not a tag. Instead, we add a fully-peeled trait to the packed-refs file. If it is set, we know that we can trust a missing peel line to mean that a ref cannot be peeled. Otherwise, we fall back to assuming nothing. [commit message and tests by Jeff King p...@peff.net] Signed-off-by: Jeff King p...@peff.net --- This uses Michael's approach for managing the flags within read_packed_refs, which is more readable. As I picked up his code and comments, I realized that there was basically nothing of mine left, so I switched the authorship. But do note: 1. It should have Michael's signoff, which was not present in the commit I lifted the code from. 2. I tweaked the big comment above read_packed_refs to reduce some ambiguities. Please double-check that I am not putting inaccurate words in your mouth. :) pack-refs.c | 2 +- refs.c | 43 +-- t/t3211-peel-ref.sh | 22 ++ 3 files changed, 64 insertions(+), 3 deletions(-) diff --git a/pack-refs.c b/pack-refs.c index ebde785..4461f71 100644 --- a/pack-refs.c +++ b/pack-refs.c @@ -128,7 +128,7 @@ int pack_refs(unsigned int flags) die_errno(unable to create ref-pack file structure); /* perhaps other traits later as well */ - fprintf(cbdata.refs_file, # pack-refs with: peeled \n); + fprintf(cbdata.refs_file, # pack-refs with: peeled fully-peeled \n); for_each_ref(handle_one_ref, cbdata); if (ferror(cbdata.refs_file)) diff --git a/refs.c b/refs.c index 175b9fc..bdeac28 100644 --- a/refs.c +++ b/refs.c @@ -803,11 +803,39 @@ static void read_packed_refs(FILE *f, struct ref_dir *dir) return line; } +/* + * Read f, which is a packed-refs file, into dir. + * + * A comment line of the form # pack-refs with: may contain zero or + * more traits. We interpret the traits as follows: + * + * No traits: + * + * Probably no references are peeled. But if the file contains a + * peeled value for a reference, we will use it. + * + * peeled: + * + * References under refs/tags/, if they *can* be peeled, *are* + * peeled in this file. References outside of refs/tags/ are + * probably not peeled even if they could have been, but if we find + * a peeled value for such a reference we will use it. + * + * fully-peeled: + * + * All references in the file that can be peeled are peeled. + * Inversely (and this is more important, any references in the + * file for which no peeled value is recorded is not peelable. This + * trait should typically be written alongside fully-peeled for + * compatibility with older clients, but we do not require it + * (i.e., peeled is a no-op if fully-peeled is set). + */ static void read_packed_refs(FILE *f, struct ref_dir *dir) { struct ref_entry *last = NULL; char refline[PATH_MAX]; int flag = REF_ISPACKED; + int refs_tags_peeled = 0; while (fgets(refline, sizeof(refline), f)) { unsigned char sha1[20]; @@ -816,8 +844,10 @@ static void read_packed_refs(FILE *f, struct ref_dir *dir) if (!strncmp(refline, header, sizeof(header)-1)) { const char *traits = refline + sizeof(header) - 1; - if (strstr(traits, peeled )) + if (strstr(traits, fully-peeled )) flag |= REF_KNOWS_PEELED; + else if (strstr(traits, peeled )) + refs_tags_peeled = 1; /* perhaps other traits later as well */ continue; } @@ -825,6 +855,8 @@ static void read_packed_refs(FILE *f, struct ref_dir *dir) refname = parse_ref_line(refline, sha1); if (refname) { last = create_ref_entry(refname, sha1, flag, 1);
[PATCH 0/2] minor fast-export speedup
While grepping through all of the calls to parse_object (to see how they handled error conditions, for the other series I just posted), I noticed this opportunity for a small speedup in fast-export (5-15%). The first patch is a cleanup, the second is the interesting bit. [1/2]: fast-export: rename handle_object function [2/2]: fast-export: do not load blob objects twice A useful third patch on top might be to stream blobs out rather than load them into memory, but I didn't want to go there tonight. -Peff -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 1/2] fast-export: rename handle_object function
The handle_object function is rather vaguely named; it only operates on blobs, and its purpose is to export the blob to the output stream. Let's call it export_blob to make it more clear what it does. Signed-off-by: Jeff King p...@peff.net --- builtin/fast-export.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/builtin/fast-export.c b/builtin/fast-export.c index 77dffd1..3eba852 100644 --- a/builtin/fast-export.c +++ b/builtin/fast-export.c @@ -113,7 +113,7 @@ static void show_progress(void) printf(progress %d objects\n, counter); } -static void handle_object(const unsigned char *sha1) +static void export_blob(const unsigned char *sha1) { unsigned long size; enum object_type type; @@ -312,7 +312,7 @@ static void handle_commit(struct commit *commit, struct rev_info *rev) /* Export the referenced blobs, and remember the marks. */ for (i = 0; i diff_queued_diff.nr; i++) if (!S_ISGITLINK(diff_queued_diff.queue[i]-two-mode)) - handle_object(diff_queued_diff.queue[i]-two-sha1); + export_blob(diff_queued_diff.queue[i]-two-sha1); mark_next_object(commit-object); if (!is_encoding_utf8(encoding)) @@ -512,7 +512,7 @@ static void get_tags_and_duplicates(struct rev_cmdline_info *info, commit = (struct commit *)tag; break; case OBJ_BLOB: - handle_object(tag-object.sha1); + export_blob(tag-object.sha1); continue; default: /* OBJ_TAG (nested tags) is already handled */ warning(Tag points to object of unexpected type %s, skipping., -- 1.8.2.rc2.7.gef06216 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 2/2] fast-export: do not load blob objects twice
When fast-export wants to export a blob object, it first calls parse_object to get a struct object and check whether we have already shown the object. If we haven't shown it, we then use read_sha1_file to pull it from disk and write it out. That means we load each blob from disk twice: once for parse_object to find its type and check its sha1, and a second time when we actually output it. We can drop this to a single load by using lookup_object to check the SHOWN flag, and then checking the signature on and outputting a single buffer. This provides modest speedups on git.git (best-of-five, git fast-export HEAD /dev/null): [before][after] real0m14.347s real0m13.780s user0m14.084s user0m13.620s sys 0m0.208ssys 0m0.100s and somewhat more on more blob-heavy repos (this is a repository full of media files): [before][after] real0m52.236s real0m44.451s user0m50.568s user0m43.000s sys 0m1.536ssys 0m1.284s Signed-off-by: Jeff King p...@peff.net --- We actually spend a non-trivial amount of time re-checking the sha1 of objects we are loading. This change also makes it easy to drop that checking, though perhaps the additional safety is a good thing to have during an export. The timings without it are: git.git (was 14.347s) real0m11.452s user0m11.336s sys 0m0.072s photos (was 44.451s) real0m18.383s user0m17.108s sys 0m1.224s builtin/fast-export.c | 16 ++-- 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/builtin/fast-export.c b/builtin/fast-export.c index 3eba852..d380155 100644 --- a/builtin/fast-export.c +++ b/builtin/fast-export.c @@ -119,6 +119,7 @@ static void export_blob(const unsigned char *sha1) enum object_type type; char *buf; struct object *object; + int eaten; if (no_data) return; @@ -126,16 +127,18 @@ static void export_blob(const unsigned char *sha1) if (is_null_sha1(sha1)) return; - object = parse_object(sha1); - if (!object) - die (Could not read blob %s, sha1_to_hex(sha1)); - - if (object-flags SHOWN) + object = lookup_object(sha1); + if (object object-flags SHOWN) return; buf = read_sha1_file(sha1, type, size); if (!buf) die (Could not read blob %s, sha1_to_hex(sha1)); + if (check_sha1_signature(sha1, buf, size, typename(type)) 0) + die(sha1 mismatch in blob %s, sha1_to_hex(sha1)); + object = parse_object_buffer(sha1, type, size, buf, eaten); + if (!object) + die(Could not read blob %s, sha1_to_hex(sha1)); mark_next_object(object); @@ -147,7 +150,8 @@ static void export_blob(const unsigned char *sha1) show_progress(); object-flags |= SHOWN; - free(buf); + if (!eaten) + free(buf); } static int depth_first(const void *a_, const void *b_) -- 1.8.2.rc2.7.gef06216 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/3] fix unparsed object access in upload-pack
On Sat, Mar 16, 2013 at 11:17:18PM -0700, Junio C Hamano wrote: I almost wonder if we should cut it out entirely. It is definitely a possible race condition, but I wonder if anybody actually hits it in practice (and if they do, the consequence is that the fetch fails and needs to be retried). As far as I can tell, the code path has never actually been followed, and I do not recall ever seeing a bug report or complaint about it (though perhaps it happened once, which spurred the initial development?). If you run multiple servers serving the same repository at the same URL with a small mirroring lag, one may observe a set of refs from one server, that are a tad older than the other server you actually fetch from. k.org may have such an arrangement, but does GitHub serve the same repository on multiple machines without tying the same client to the same backend? Each repository is a on a single backend host. They're redundant internally (each host is actually multiple hosts), but pure-git requests go to a single master for each host (though for some read-only operations I think we spread the load across the redundant spares). You might get a separate machine during a failover event, but they share block devices via DRBD, so in theory an fsync() should hit both machines, and there is no lag (and you are likely to get an intermittent failure in such a case, anyway, since the machine serving your git request probably died mid-packet). I thought this change was to prevent against the common race: 1. Client request stateless ref advertisement. 2. Somebody updates ref. 3. Client requests want objects based on old advertisement. and I think it does solve that (assuming step 2 is not a rewind). The important thing is that time always moves forward. But if you are talking about mirror lag, time can move in either direction. Imagine you have two machines, A and B, and A is missing an update to B. If you hit A first, then B, it is the same as the update sequence above. The patch helps. But if you hit B first, then A, you will ask it for objects it has not yet received, and we must fail. So I think any such mirroring setup would want to try very hard to make sure you hit the same machine both times anyway, regardless of this patch. I'm fine to leave it. I was just questioning its utility since AFAICT, it has never worked and nobody has cared. It's not too much code, though, and it is only run when we hit the race, so I don't think it is hurting anything. -Peff -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Preallocate hash tables when the number of inserts are known in advance
On Sun, Mar 17, 2013 at 10:28:06AM +0700, Nguyen Thai Ngoc Duy wrote: This avoids unnecessary re-allocations and reinsertions. On webkit.git (i.e. about 182k inserts to the name hash table), this reduces about 100ms out of 3s user time. Good idea. I had a similar thought when analyzing the hashing behavior of pack-objects' Counting objects... phase, but it had even less impact there. The insertions are just drowned out by the number of lookups in that case. -Peff -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 05/12] pretty: save commit encoding from logmsg_reencode if the caller needs it
On Fri, Mar 15, 2013 at 10:24 PM, Nguyễn Thái Ngọc Duy pclo...@gmail.com wrote: The commit encoding is parsed by logmsg_reencode, there's no need for the caller to re-parse it again. The reencoded message now have the s/have/has/ new encoding, not the original one. The caller would need to read commit object again before parsing. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 09/12] pretty: add %C(auto) for auto-coloring on the next placeholder
On Fri, Mar 15, 2013 at 10:24 PM, Nguyễn Thái Ngọc Duy pclo...@gmail.com wrote: This is not simply convenient over $C(auto,xxx). Some placeholders s/\$/%/ (actually only one, %d) do multi coloring and we can't emit a multiple colors with %C(auto,xxx). diff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt index 66345d1..8734224 100644 --- a/Documentation/pretty-formats.txt +++ b/Documentation/pretty-formats.txt @@ -154,7 +154,8 @@ The placeholders are: adding `auto,` at the beginning will emit color only when colors are enabled for log output (by `color.diff`, `color.ui`, or `--color`, and respecting the `auto` settings of the former if we are going to a - terminal) + terminal). `auto` alone (i.e. `%C(auto)`) will turn on auto coloring s/auto coloring/auto-coloring/ + on the following placeholder. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 10/12] pretty: support padding placeholders, % % and %
On Fri, Mar 15, 2013 at 10:24 PM, Nguyễn Thái Ngọc Duy pclo...@gmail.com wrote: Either %, % or % standing before a placeholder specifies how many s/%/%/ diff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt index 8734224..87ca2c4 100644 --- a/Documentation/pretty-formats.txt +++ b/Documentation/pretty-formats.txt @@ -162,6 +162,14 @@ The placeholders are: - '%x00': print a byte from a hex code - '%w([w[,i1[,i2]]])': switch line wrapping, like the -w option of linkgit:git-shortlog[1]. +- '%(N)': make the next placeholder take at least N columns, + padding spaces on the right if necessary +- '%|(N)': make the next placeholder take at least until Nth + columns, padding spaces on the right if necessary +- '%(N)', '%|(N)': similar to '%(N)', '%|(N)' s/N/N/g + respectively, but padding spaces on the left +- '%(N)', '%|(N)': similar to '%(N)', '%|(N)' Ditto: s/N/N/g + respectively, but padding both sides (i.e. the text is centered) -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 12/12] pretty: support % that steal trailing spaces
On Fri, Mar 15, 2013 at 10:24 PM, Nguyễn Thái Ngọc Duy pclo...@gmail.com wrote: This is pretty useful in `%(100)%s%Cred%(20)% an' where %s does not s/% an/%an/ use up all 100 columns and %an needs more than 20 columns. By replacing %(20) with %(20), %an can steal spaces from %s. diff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt index 17f82f4..036c14a 100644 --- a/Documentation/pretty-formats.txt +++ b/Documentation/pretty-formats.txt @@ -170,7 +170,10 @@ The placeholders are: columns, padding spaces on the right if necessary - '%(N)', '%|(N)': similar to '%(N)', '%|(N)' respectively, but padding spaces on the left -- '%(N)', '%|(N)': similar to '%(N)', '%|(N)' +- '%(N)', '%|(N)': similar to '%(N)', '%|(N)' s/N/N/g + respectively, except that if the next placeholder takes more spaces + than given and there are spaces on its left, use those spaces +- '%(N)', '%|(N)': similar to '% (N)', '%|(N)' Ditto: s/N/N/g respectively, but padding both sides (i.e. the text is centered) -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] sha1_name: pass object name length to diagnose_invalid_sha1_path()
Am 17.03.2013 08:10, schrieb Junio C Hamano: @@ -1158,16 +1159,16 @@ static void diagnose_invalid_sha1_path(const char *prefix, if (!get_tree_entry(tree_sha1, fullname, sha1, mode)) { die(Path '%s' exists, but not '%s'.\n - Did you mean '%s:%s' aka '%s:./%s'?, + Did you mean '%.*s:%s' aka '.*%.*s:./%s'?, Will squash an obvious fix in and apply. Did I try to make a point there? Certainly not. It seems I need to go back to http://vim-adventures.com/. Thank you for spotting this! René -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] gitk: Add user-configurable branch bg color
On 03/17/2013 03:57 AM, David Aguilar wrote: In some cases, the default branch background color (green) isn't an optimal choice, thus it can be difficult to read. I'm just curious -- is it difficult to read because gitk does not specify a foreground color, thus causing it to pickup a system default (which can vary), or is it for a different reason? If this is the reason then I wonder whether gitk should explicitly set a foreground color. Apologies if it already works that way -- I just wanted to better understand the motivation behind this patch. Yes, having gitk to specify a foreground color instead would probably solve it too: i can remember the branch rectangles were a lot more readable in the past, but this could probably be due to me using a dark system theme at the time. For example, in my case it would look a lot more readable if the text was white on green instead of black on green, that could be even hardcoded as it was with the green color, but the reason behind the choice to let the user customize it is that i think it is better to honour the user system colors giving the means to adjust it to match his actual system configuration, rather than overriding it. Regards, Manuel -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] combine-diff: coalesce lost lines optimally
This replaces the greedy implementation to coalesce lost lines by using dynamic programming to find the Longest Common Subsequence. The O(n²) time complexity is obviously bigger than previous implementation but it can produce shorter diff results (and most likely easier to read). List of lost lines is now doubly-linked because we reverse-read it when reading the direction matrix. Signed-off-by: Antoine Pelisse apeli...@gmail.com --- Hi, This is a very first draft for improving the way we coalesce lost lines. It has only been tested with the two scenarios below. What is left to do: - Test it more extensively - Had some tests scenarios I'm also having a hard time trying it with more than two parents. How I am supposed to have more than two parents while octopus merge refuses if there are conflicts ? Tested scenarios: git init test git add test git commit -m initial git checkout -b side1 seq 10 test git commit -m all -a git checkout master seq 1 2 10 test git commit -m three -a git merge side1 test git commit -m merge -a git show AND git init test git add test git commit -m initial echo 3\n1\n2\n4 test git commit -m shuffled -a git checkout -b side HEAD^ echo 1\n2\n3\n4 test git commit -m sorted -a git merge master test git commit -m merge -a git show combine-diff.c | 192 ++-- 1 file changed, 160 insertions(+), 32 deletions(-) diff --git a/combine-diff.c b/combine-diff.c index 35d41cd..252dd72 100644 --- a/combine-diff.c +++ b/combine-diff.c @@ -73,16 +73,24 @@ static struct combine_diff_path *intersect_paths(struct combine_diff_path *curr, /* Lines lost from parent */ struct lline { - struct lline *next; + struct lline *next, *prev; int len; unsigned long parent_map; char line[FLEX_ARRAY]; }; +/* Lines lost from current parent (before coalescing) */ +struct plost { + struct lline *lost_head, *lost_tail; + int len; +}; + /* Lines surviving in the merge result */ struct sline { - struct lline *lost_head, **lost_tail; - struct lline *next_lost; + /* Accumulated and coalesced lost lines */ + struct lline *lost; + int lenlost; + struct plost plost; char *bol; int len; /* bit 0 up to (N-1) are on if the parent has this line (i.e. @@ -94,6 +102,132 @@ struct sline { unsigned long *p_lno; }; +enum coalesce_direction { MATCH, BASE, NEW }; + +/* Coalesce new lines into base by finding LCS */ +static struct lline *coalesce_lines(struct lline *base, int *lenbase, + struct lline *new, int lennew, + unsigned long parent) +{ + int **lcs; + enum coalesce_direction **directions; + struct lline *baseend, *newend; + int i, j, origbaselen = *lenbase; + + if (new == NULL) + return base; + + if (base == NULL) { + *lenbase = lennew; + return new; + } + + /* +* Coalesce new lines into base by finding the LCS +* - Create the table to run dynamic programing +* - Compute the LCS +* - Then reverse read the direction structure: +* - If we have MATCH, assign parent to base flag, and consume +* both baseend and newend +* - Else if we have BASE, consume baseend +* - Else if we have NEW, insert newend lline into base and +* consume newend +*/ + lcs = xcalloc(origbaselen + 1, sizeof(int*)); + directions = xcalloc(origbaselen + 1, sizeof(enum coalesce_direction*)); + for (i = 0; i origbaselen + 1; i++) { + lcs[i] = xcalloc(lennew + 1, sizeof(int)); + directions[i] = xcalloc(lennew + 1, sizeof(enum coalesce_direction)); + directions[i][0] = BASE; + } + for (j = 1; j lennew + 1; j++) + directions[0][j] = NEW; + + for (i = 1, baseend = base; i origbaselen + 1; i++) { + for (j = 1, newend = new; j lennew + 1; j++) { + if (baseend-len == newend-len + !memcmp(baseend-line, newend-line, baseend-len)) { + lcs[i][j] = lcs[i - 1][j - 1] + 1; + directions[i][j] = MATCH; + } else if (lcs[i][j - 1] = lcs[i - 1][j]) { + lcs[i][j] = lcs[i][j - 1]; + directions[i][j] = NEW; + } else { + lcs[i][j] = lcs[i - 1][j]; + directions[i][j] = BASE; + } + if (newend-next) + newend = newend-next; + } + if (baseend-next) + baseend = baseend-next; + } + + for (i = 0; i origbaselen + 1; i++) + free(lcs[i]); +
Re: [PATCH 4/6] introduce a commit metapack
On Thu, Jan 31, 2013 at 6:06 PM, Duy Nguyen pclo...@gmail.com wrote: On Wed, Jan 30, 2013 at 09:16:29PM +0700, Duy Nguyen wrote: Perhaps we could store abbrev sha-1 instead of full sha-1. Nice space/time trade-off. Following the on-disk format experiment yesterday, I changed the format to: - a list a _short_ SHA-1 of cached commits .. The length of SHA-1 is chosen to be able to unambiguously identify any cached commits. Full SHA-1 check is done after to catch false positives. For linux-2.6, SHA-1 length is 6 bytes, git and many moderate-sized projects are 4 bytes. And if we are going to create index v3, the same trick could be used for the sha-1 table in the index. We use the short sha-1 table for binary search and put the rest of sha-1 in a following table (just like file offset table). The advantage is a denser search space, about 1/4-1/3 the size of full sha-1 table. -- Duy -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] combine-diff: coalesce lost lines optimally
I'm also having a hard time trying it with more than two parents. How I am supposed to have more than two parents while octopus merge refuses if there are conflicts ? OK, creating the merge commit myself solves the issue: git init test git add test git commit -m initial seq 100 test git commit -m all -a git checkout -b side1 HEAD^1 seq 1 2 100 test git commit -m side1 -a git checkout -b side2 HEAD^1 seq 1 4 100 test git commit -m side2 -a git checkout -b side3 HEAD^1 seq 1 8 100 test git commit -m side3 -a git checkout -b side4 HEAD^1 seq 1 16 100 test git commit -m side4 -a git checkout master test git add test TREE=$(git write-tree) COMMIT=$(git commit-tree $TREE -p master -p side1 -p side2 -p side3 -p side4 -m merge) git show $COMMIT This will work with the basic greedy implementation if all parents are in this order. But the optimal result will be lost if we change the order of -p parameters in git-commit-tree. The patch seems to be correct, always finding the best result (we always have 100 lines diff) whatever the order of parents. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] safe_create_leading_directories: fix race that could give a false negative
If two processes are racing to create the same directory tree, they will both see that the directory doesn't exist, both try to mkdir(), and one of them will fail. This is okay, as we only care that the directory gets created. So, we add a check for EEXIST from mkdir, and continue if the directory now exists. Signed-off-by: Steven Walter stevenrwal...@gmail.com --- sha1_file.c |9 + 1 file changed, 9 insertions(+) diff --git a/sha1_file.c b/sha1_file.c index 40b2329..5668ecc 100644 --- a/sha1_file.c +++ b/sha1_file.c @@ -123,6 +123,15 @@ int safe_create_leading_directories(char *path) } } else if (mkdir(path, 0777)) { + if (errno == EEXIST) { + /* +* We could be racing with another process to +* create the directory. As long as the +* directory gets created, we don't care. +*/ + if (stat(path, st) S_ISDIR(st.st_mode)) + continue; + } *pos = '/'; return -1; } -- 1.7.10.4 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/3] fix unparsed object access in upload-pack
Am 17.03.2013 06:40, schrieb Jeff King: We do have the capability to roll out to one or a few of our servers (the granularity is not 0.2%, but it is still small). I'm going to try to keep us more in sync with upstream git, but I don't know if I will get to the point of ever deploying master or next, even for a small portion of the population. We are accumulating more hacks[1] on top of git, so it is not just run master for an hour on this server; I have to actually merge our fork. Did you perhaps intend to list these hacks in a footnote or link to a repository containing them? (I can't find the counterpart of that [1].) René -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] safe_create_leading_directories: fix race that could give a false negative
Steven Walter stevenrwal...@gmail.com writes: If two processes are racing to create the same directory tree, they will both see that the directory doesn't exist, both try to mkdir(), and one of them will fail. This is okay, as we only care that the directory gets created. So, we add a check for EEXIST from mkdir, and continue if the directory now exists. Signed-off-by: Steven Walter stevenrwal...@gmail.com --- sha1_file.c |9 + 1 file changed, 9 insertions(+) diff --git a/sha1_file.c b/sha1_file.c index 40b2329..5668ecc 100644 --- a/sha1_file.c +++ b/sha1_file.c @@ -123,6 +123,15 @@ int safe_create_leading_directories(char *path) } } else if (mkdir(path, 0777)) { + if (errno == EEXIST) { + /* + * We could be racing with another process to + * create the directory. As long as the + * directory gets created, we don't care. + */ + if (stat(path, st) S_ISDIR(st.st_mode)) + continue; You probably meant !stat() here, we can successfully stat() and it turns out that we already have a directory there, so let's not do the error thing. Don't you need to restore (*pos = '/') before doing anything else, like continue, by the way? We were given a/b/c, and in order to make sure a exists, we made it to a\0b/c, did a stat() and found it was missing, did a mkdir() and now we got EEXIST. pos points at that NUL, so I would imagine that in order to continue you need to * restore the string to be a/b/c; and * make pos to point at b in the string. Perhaps something like this instead? sha1_file.c | 9 +++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/sha1_file.c b/sha1_file.c index 9152974..964c4d4 100644 --- a/sha1_file.c +++ b/sha1_file.c @@ -122,8 +122,13 @@ int safe_create_leading_directories(char *path) } } else if (mkdir(path, 0777)) { - *pos = '/'; - return -1; + if (errno == EEXIST + !stat(path, st) S_ISDIR(st.st_mode)) { + ; /* somebody created it since we checked */ + } else { + *pos = '/'; + return -1; + } } else if (adjust_shared_perm(path)) { *pos = '/'; -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 4/4] pack-refs: add fully-peeled trait
Jeff King p...@peff.net writes: From: Michael Haggerty mhag...@alum.mit.edu Older versions of pack-refs did not write peel lines for refs outside of refs/tags. This meant that on reading the pack-refs file, we might set the REF_KNOWS_PEELED flag for such a ref, even though we do not know anything about its peeled value. The previous commit updated the writer to always peel, no matter what the ref is. That means that packed-refs files written by newer versions of git are fine to be read by both old and new versions of git. However, we still have the problem of reading packed-refs files written by older versions of git, or by other implementations which have not yet learned the same trick. The simplest fix would be to always unset the REF_KNOWS_PEELED flag for refs outside of refs/tags that do not have a peel line (if it has a peel line, we know it is valid, but we cannot assume a missing peel line means anything). But that loses an important optimization, as upload-pack should not need to load the object pointed to by refs/heads/foo to determine that it is not a tag. Instead, we add a fully-peeled trait to the packed-refs file. If it is set, we know that we can trust a missing peel line to mean that a ref cannot be peeled. Otherwise, we fall back to assuming nothing. [commit message and tests by Jeff King p...@peff.net] Signed-off-by: Jeff King p...@peff.net --- This uses Michael's approach for managing the flags within read_packed_refs, which is more readable. As I picked up his code and comments, I realized that there was basically nothing of mine left, so I switched the authorship. But do note: 1. It should have Michael's signoff, which was not present in the commit I lifted the code from. 2. I tweaked the big comment above read_packed_refs to reduce some ambiguities. Please double-check that I am not putting inaccurate words in your mouth. :) pack-refs.c | 2 +- refs.c | 43 +-- t/t3211-peel-ref.sh | 22 ++ 3 files changed, 64 insertions(+), 3 deletions(-) diff --git a/pack-refs.c b/pack-refs.c index ebde785..4461f71 100644 --- a/pack-refs.c +++ b/pack-refs.c @@ -128,7 +128,7 @@ int pack_refs(unsigned int flags) die_errno(unable to create ref-pack file structure); /* perhaps other traits later as well */ - fprintf(cbdata.refs_file, # pack-refs with: peeled \n); + fprintf(cbdata.refs_file, # pack-refs with: peeled fully-peeled \n); for_each_ref(handle_one_ref, cbdata); if (ferror(cbdata.refs_file)) diff --git a/refs.c b/refs.c index 175b9fc..bdeac28 100644 --- a/refs.c +++ b/refs.c @@ -803,11 +803,39 @@ static void read_packed_refs(FILE *f, struct ref_dir *dir) return line; } +/* + * Read f, which is a packed-refs file, into dir. + * + * A comment line of the form # pack-refs with: may contain zero or + * more traits. We interpret the traits as follows: + * + * No traits: + * + * Probably no references are peeled. But if the file contains a + * peeled value for a reference, we will use it. + * + * peeled: + * + * References under refs/tags/, if they *can* be peeled, *are* + * peeled in this file. References outside of refs/tags/ are + * probably not peeled even if they could have been, but if we find + * a peeled value for such a reference we will use it. + * + * fully-peeled: + * + * All references in the file that can be peeled are peeled. + * Inversely (and this is more important, any references in the A missing closing paren after more important. Also the e-mail quote reveals there is some inconsistent indentation (HTs vs runs of SPs) here. + * file for which no peeled value is recorded is not peelable. This + * trait should typically be written alongside fully-peeled for Alongside peeled, no? @@ -816,8 +844,10 @@ static void read_packed_refs(FILE *f, struct ref_dir *dir) if (!strncmp(refline, header, sizeof(header)-1)) { const char *traits = refline + sizeof(header) - 1; - if (strstr(traits, peeled )) + if (strstr(traits, fully-peeled )) flag |= REF_KNOWS_PEELED; + else if (strstr(traits, peeled )) + refs_tags_peeled = 1; /* perhaps other traits later as well */ continue; } @@ -825,6 +855,8 @@ static void read_packed_refs(FILE *f, struct ref_dir *dir) refname = parse_ref_line(refline, sha1); if (refname) { last = create_ref_entry(refname, sha1, flag, 1); + if (refs_tags_peeled !prefixcmp(refname, refs/tags/)) + last-flag |= REF_KNOWS_PEELED; I am not sure why
Re: [PATCH] git-p4: support exclusively locked files
danny.tho...@blackboard.com wrote on Wed, 13 Mar 2013 13:51 -0400: By default, newly added binary files are exclusively locked by Perforce: 'add default change (binary+l) *exclusive*' This results in a 'Could not determine file type' error as the regex expects the line to end after the file type matching group. Some repositories are also configured to always require exclusive locks, so may be a problem for all revisions in some cases. Can you explain how to configure p4d to default everything to binary+l? Also, what version are you using (p4 info)? I'm trying to write a test case for this. I did find a way to play with typemap to get +l, as: ( p4 typemap -o ; printf \tbinary+l\t//.../bash* ) | p4 typemap -i With this, the 2011.1 here just says: tic-git-test$ p4 opened bash //depot/bash#1 - add default change (binary+l) I've not been able to make it say *exclusive* too. result = p4_read_pipe([opened, wildcard_encode(file)]) -match = re.match(.*\((.+)\)\r?$, result) +match = re.match(.*\((.+)\)(?:.+)?\r?$, result) I think this whole function would be less brittle using p4's -G output, like: d = p4Cmd([fstat, -T, type, wildcard_encode(file)]) # some error checking return d['type'] But I'm curious if your p4d includes *exclusive* in the type there too, in which case we'll have to strip it. Thanks for starting the patch on this. It's good if we can keep others from running into the same problem. -- Pete -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] combine-diff: coalesce lost lines optimally
Antoine Pelisse apeli...@gmail.com writes: This replaces the greedy implementation to coalesce lost lines by using dynamic programming to find the Longest Common Subsequence. The O(n²) time complexity is obviously bigger than previous implementation but it can produce shorter diff results (and most likely easier to read). List of lost lines is now doubly-linked because we reverse-read it when reading the direction matrix. Signed-off-by: Antoine Pelisse apeli...@gmail.com --- Hi, This is a very first draft for improving the way we coalesce lost lines. It has only been tested with the two scenarios below. What is left to do: - Test it more extensively - Had some tests scenarios I'm also having a hard time trying it with more than two parents. How I am supposed to have more than two parents while octopus merge refuses if there are conflicts ? 9fdb62af92c7 ([ACPI] merge 3549 4320 4485 4588 4980 5483 5651 acpica asus fops pnpacpi branches into release, 2006-01-24) is one of the amusing examples ;-) Cf. http://thread.gmane.org/gmane.comp.version-control.git/15486 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Make GIT_USE_LOOKUP default?
Duy Nguyen pclo...@gmail.com writes: This env comes from jc/sha1-lookup in 2008 (merge commit e9f9d4f), 5 years ago. I wonder if it's good enough to turn on by default and keep improving from there, or is it still experimental? The algorithm has been used in production in other codepaths like patch-ids and replace-object, so correctness-wise it should be fine to turn it on. I think nobody has bothered to benchmark with and without the environment to see if it is really worth the complexity. It may be a good idea to try doing so, now you have noticed it ;-). -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] combine-diff: coalesce lost lines optimally
Hopefully, my patch takes about the same time as git 1.7.9.5 and produces the same output on that commit ;) Unfortunately on a commit that would remove A LOT of lines (1) from 7 parents, the times goes from 0.01s to 1.5s... I'm pretty sure that scenario is quite uncommon though. On Sun, Mar 17, 2013 at 9:10 PM, Junio C Hamano gits...@pobox.com wrote: Antoine Pelisse apeli...@gmail.com writes: This replaces the greedy implementation to coalesce lost lines by using dynamic programming to find the Longest Common Subsequence. The O(n²) time complexity is obviously bigger than previous implementation but it can produce shorter diff results (and most likely easier to read). List of lost lines is now doubly-linked because we reverse-read it when reading the direction matrix. Signed-off-by: Antoine Pelisse apeli...@gmail.com --- Hi, This is a very first draft for improving the way we coalesce lost lines. It has only been tested with the two scenarios below. What is left to do: - Test it more extensively - Had some tests scenarios I'm also having a hard time trying it with more than two parents. How I am supposed to have more than two parents while octopus merge refuses if there are conflicts ? 9fdb62af92c7 ([ACPI] merge 3549 4320 4485 4588 4980 5483 5651 acpica asus fops pnpacpi branches into release, 2006-01-24) is one of the amusing examples ;-) Cf. http://thread.gmane.org/gmane.comp.version-control.git/15486 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] remote.name.pushurl does not consider aliases when pushing
Hi everyone! I found a bug in Git today and wrote up a fix; I did my best to conform to the rules layed out in Documentation/SubmittingPatches, but please let me know if I need to change anything to get my work merged. =) I have CC'ed Josh Triplet, as he was the last one to touch the line I modified. I hope my commit messages explain the problem I encountered well enough; if not, I can always go back and amend them. Patches follow. -Rob From 5007b11e86c0835807632cb41e6cfa75ce9a1aa1 Mon Sep 17 00:00:00 2001 From: Rob Hoelz r...@hoelz.ro Date: Sun, 17 Mar 2013 21:49:20 +0100 Subject: [PATCH 1/2] Add test for pushInsteadOf + pushurl git push currently doesn't consider pushInsteadOf when using pushurl; this test tests that. Signed-off-by: Rob Hoelz r...@hoelz.ro --- t/t5500-push-pushurl.sh | 37 + 1 file changed, 37 insertions(+) create mode 100644 t/t5500-push-pushurl.sh diff --git a/t/t5500-push-pushurl.sh b/t/t5500-push-pushurl.sh new file mode 100644 index 000..74d4ff6 --- /dev/null +++ b/t/t5500-push-pushurl.sh @@ -0,0 +1,37 @@ +#!/bin/sh +# +# Copyright (c) 2013 Rob Hoelz +# + +test_description='Test that remote.name.pushurl uses pushInsteadOf' +. ./test-lib.sh + +wd=$(pwd) +test_expect_success 'setup test repositories' ' + mkdir ro + mkdir rw + + git init --bare ro/repo + git init --bare rw/repo + git init test-repo +' + +test_expect_success 'setup remote config' + cd test-repo + git config 'url.file://$wd/ro/.insteadOf' herero: + git config 'url.file://$wd/rw/.pushInsteadOf' hererw: + git remote add origin herero:repo + git config remote.origin.pushurl hererw:repo + + +test_expect_success 'test commit and push' ' + test_commit one + git push origin master:master +' + +test_expect_success 'check for commits in rw repo' ' + cd ../rw/repo + git log --pretty=oneline | grep -q . +' + +test_done -- 1.8.2 From 0cbd1aab6bdc0c2f8893ed8b9a8e3eb0126917d1 Mon Sep 17 00:00:00 2001 From: Rob Hoelz r...@hoelz.ro Date: Sun, 17 Mar 2013 16:34:35 +0100 Subject: [PATCH 2/2] push: Alias pushurl from push rewrites If you use pushurl with an alias that has a pushInsteadOf configuration value, Git does not take advantage of it. For example: [url git://github.com/] insteadOf = github: [url git://github.com/myuser/] insteadOf = mygithub: [url g...@github.com:myuser/] pushInsteadOf = mygithub: [remote origin] url = github:organization/project pushurl = mygithub:project This commit fixes that. Signed-off-by: Rob Hoelz r...@hoelz.ro --- remote.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/remote.c b/remote.c index ca1f8f2..de7a915 100644 --- a/remote.c +++ b/remote.c @@ -465,7 +465,7 @@ static void alias_all_urls(void) if (!remotes[i]) continue; for (j = 0; j remotes[i]-pushurl_nr; j++) { - remotes[i]-pushurl[j] = alias_url(remotes[i]-pushurl[j], rewrites); + remotes[i]-pushurl[j] = alias_url(remotes[i]-pushurl[j], rewrites_push); } add_pushurl_aliases = remotes[i]-pushurl_nr == 0; for (j = 0; j remotes[i]-url_nr; j++) { -- 1.8.2 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v1 11/45] parse_pathspec: support stripping submodule trailing slashes
Nguyễn Thái Ngọc Duy pclo...@gmail.com writes: This flag is equivalent to builtin/ls-files.c:strip_trailing_slashes() and is intended to replace that function when ls-files is converted to use parse_pathspec. Signed-off-by: Nguyễn Thái Ngọc Duy pclo...@gmail.com --- pathspec.c | 9 + pathspec.h | 1 + 2 files changed, 10 insertions(+) diff --git a/pathspec.c b/pathspec.c index b2446c3..2da8bc9 100644 --- a/pathspec.c +++ b/pathspec.c @@ -204,6 +204,15 @@ static unsigned prefix_pathspec(struct pathspec_item *item, *raw = item-match = match; item-original = elt; item-len = strlen(item-match); + + if ((flags PATHSPEC_STRIP_SUBMODULE_SLASH_CHEAP) I see that having cheap and expensive variant at these steps is a way to achieve a bug-for-bug compatible rewrite of the existing code, but in the longer term should we lose one of them? After all, a path that points at the working tree of another repository that is beyond a symlink cannot be added to us as a submodule, so CHEAP can be used only when we know that any intermediate component on the path is not a symlink pointing elsewhere, and the user in the codepath of ls-files may know it. To put it differently, shouldn't CHEAP and EXPENSIVE be accompanied by in-code comment and updates to Documentation/technical/api-*.txt to help users of the API to decide which one to use? -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v1 14/45] Guard against new pathspec magic in pathspec matching code
Nguyễn Thái Ngọc Duy pclo...@gmail.com writes: GUARD_PATHSPEC() marks pathspec-sensitive code (basically anything in 'struct pathspec' except fields nr and original). GUARD_PATHSPEC() is not supposed to fail. The steps for a new pathspec magic or optimization would be: - update parse_pathspec, add extra information to struct pathspec - grep GUARD_PATHSPEC() and update all relevant code (or note those that won't work with your new stuff). Update GUARD_PATHSPEC mask accordingly. - update parse_pathspec calls to disable new magic early at command parsing level. Make sure parse_pathspec() catches unsupported syntax, not until GUARD_PATHSPEC catches it. - add tests to verify supported/unsupported commands both work as expected. I think Documentation/technical/api-*.txt wants to have all of the above. It is not clear from the description what the second bitmask is supposed to mean, without reading the implementation; I am guessing that you are supposed to list the kinds of magic that are supported in the codepath? Signed-off-by: Nguyễn Thái Ngọc Duy pclo...@gmail.com --- builtin/diff.c | 2 ++ dir.c | 2 ++ pathspec.h | 7 +++ tree-diff.c| 19 +++ tree-walk.c| 2 ++ 5 files changed, 32 insertions(+) diff --git a/builtin/diff.c b/builtin/diff.c index 8c2af6c..d237e0a 100644 --- a/builtin/diff.c +++ b/builtin/diff.c @@ -371,6 +371,8 @@ int cmd_diff(int argc, const char **argv, const char *prefix) die(_(unhandled object '%s' given.), name); } if (rev.prune_data.nr) { + /* builtin_diff_b_f() */ + GUARD_PATHSPEC(rev.prune_data, PATHSPEC_FROMTOP); if (!path) path = rev.prune_data.items[0].match; paths += rev.prune_data.nr; diff --git a/dir.c b/dir.c index 1e9db41..6094ba8 100644 --- a/dir.c +++ b/dir.c @@ -297,6 +297,8 @@ int match_pathspec_depth(const struct pathspec *ps, { int i, retval = 0; + GUARD_PATHSPEC(ps, PATHSPEC_FROMTOP | PATHSPEC_MAXDEPTH); + if (!ps-nr) { if (!ps-recursive || !(ps-magic PATHSPEC_MAXDEPTH) || diff --git a/pathspec.h b/pathspec.h index 1cef9c6..ed5d3a6 100644 --- a/pathspec.h +++ b/pathspec.h @@ -27,6 +27,13 @@ struct pathspec { } *items; }; +#define GUARD_PATHSPEC(ps, mask) \ + do { \ + if ((ps)-magic ~(mask)) \ + die(BUG:%s:%d: unsupported magic %x, \ + __FILE__, __LINE__, (ps)-magic ~(mask)); \ + } while (0) + /* parse_pathspec flags */ #define PATHSPEC_PREFER_CWD (10) /* No args means match cwd */ #define PATHSPEC_PREFER_FULL (11) /* No args means match everything */ diff --git a/tree-diff.c b/tree-diff.c index 826512e..5a87614 100644 --- a/tree-diff.c +++ b/tree-diff.c @@ -198,6 +198,25 @@ static void try_to_follow_renames(struct tree_desc *t1, struct tree_desc *t2, co const char *paths[1]; int i; + /* + * follow-rename code is very specific, we need exactly one + * path. Magic that matches more than one path is not + * supported. + */ + GUARD_PATHSPEC(opt-pathspec, PATHSPEC_FROMTOP); +#if 0 + /* + * We should reject wildcards as well. Unfortunately we + * haven't got a reliable way to detect that 'foo\*bar' in + * fact has no wildcards. nowildcard_len is merely a hint for + * optimization. Let it slip for now until wildmatch is taught + * about dry-run mode and returns wildcard info. + */ + if (opt-pathspec.has_wildcard) + die(BUG:%s:%d: wildcards are not supported, + __FILE__, __LINE__); +#endif + /* Remove the file creation entry from the diff queue, and remember it */ choice = q-queue[0]; q-nr = 0; diff --git a/tree-walk.c b/tree-walk.c index d399ca9..37b157e 100644 --- a/tree-walk.c +++ b/tree-walk.c @@ -636,6 +636,8 @@ enum interesting tree_entry_interesting(const struct name_entry *entry, enum interesting never_interesting = ps-has_wildcard ? entry_not_interesting : all_entries_not_interesting; + GUARD_PATHSPEC(ps, PATHSPEC_FROMTOP | PATHSPEC_MAXDEPTH); + if (!ps-nr) { if (!ps-recursive || !(ps-magic PATHSPEC_MAXDEPTH) || -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH/RFC] http_init: only initialize SSL for https
On Sun, 17 Mar 2013, Antoine Pelisse wrote: With redirects taken into account, I can't think of any really good way around avoiding this init... Is there any way for curl to initialize SSL on-demand ? Yes, but not without drawbacks. If you don't call curl_global_init() at all, libcurl will notice that on first use and then libcurl will call global_init by itself with a default bitmask. That automatic call of course will prevent the application from being able to set its own bitmask choice, and also the global_init function is not (necessarily) thread safe while all other libcurl functions are so the internal call to global_init from an otherwise thread-safe function is unfortunate. -- / daniel.haxx.se -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] remote.name.pushurl does not consider aliases when pushing
Rob Hoelz r...@hoelz.ro writes: Hi everyone! I found a bug in Git today and wrote up a fix; I did my best to conform to the rules layed out in Documentation/SubmittingPatches, but please let me know if I need to change anything to get my work merged. =) I have CC'ed Josh Triplet, as he was the last one to touch the line I modified. I hope my commit messages explain the problem I encountered well enough; if not, I can always go back and amend them. Patches follow. -Rob Please read Documentation/SubmittingPatches and follow it. The above is most likely to be the cover letter of a two-patch series (meaning you will be sending three pieces of e-mail messages), or perhaps out of band comment below the three-dash line of a single patch (you will send only one piece of e-mail message). See recent patches on the list from list regulars for good examples, e.g. http://thread.gmane.org/gmane.comp.version-control.git/218350 http://thread.gmane.org/gmane.comp.version-control.git/218177/focus=218361 From 5007b11e86c0835807632cb41e6cfa75ce9a1aa1 Mon Sep 17 00:00:00 2001 From: Rob Hoelz r...@hoelz.ro Date: Sun, 17 Mar 2013 21:49:20 +0100 Subject: [PATCH 1/2] Add test for pushInsteadOf + pushurl git push currently doesn't consider pushInsteadOf when using pushurl; this test tests that. Signed-off-by: Rob Hoelz r...@hoelz.ro --- t/t5500-push-pushurl.sh | 37 + 1 file changed, 37 insertions(+) create mode 100644 t/t5500-push-pushurl.sh The number 5500 is already taken. Please do not add a duplicate. I also wonder if we need to waste a new test number for this; perhaps adding new tests to 5516 that already tests insteadOf might be a better fit, but I didn't carefully read it. diff --git a/t/t5500-push-pushurl.sh b/t/t5500-push-pushurl.sh new file mode 100644 Test scripts are supposed to be executable. +test_expect_success 'test commit and push' ' + test_commit one + git push origin master:master +' + +test_expect_success 'check for commits in rw repo' ' + cd ../rw/repo + git log --pretty=oneline | grep -q . +' Are both expected to succeed in patch 1/2 without any code change? If you were doing a large code change, it is a good series structure to have tests first that are marked as expect_failure in an early patch, and then in a later patch that changes the code to fix it, update the tests that start to pass to expect_success. I personally do not think you need such a two-step approach for something small like this; instead you can just have a single patch that adds a set of tests that expect success and code change. Thanks. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] t1507: Test that branchname@{upstream} is interpreted as branch
Syntax branchname@{upstream} should interpret its argument as a name of a branch. Add the test to check that it doesn't try to interpret it as a refname if the branch in question does not exist. Signed-off-by: Kacper Kornet drae...@pld-linux.org --- Maybe I'm too cautious adding this test. But just in case here it is. t/t1507-rev-parse-upstream.sh | 4 1 file changed, 4 insertions(+) diff --git a/t/t1507-rev-parse-upstream.sh b/t/t1507-rev-parse-upstream.sh index d6e5761..b27a720 100755 --- a/t/t1507-rev-parse-upstream.sh +++ b/t/t1507-rev-parse-upstream.sh @@ -54,6 +54,10 @@ test_expect_success 'my-side@{upstream} resolves to correct full name' ' test refs/remotes/origin/side = $(full_name my-side@{u}) ' +test_expect_success 'refs/heads/my-side@{upstream} does not resolve to my-side{upstream}' ' + test_must_fail full_name refs/heads/my-side@{upstream} +' + test_expect_success 'my-side@{u} resolves to correct commit' ' git checkout side test_commit 5 -- 1.8.2 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH/RFC] http_init: only initialize SSL for https
Daniel Stenberg dan...@haxx.se writes: On Sun, 17 Mar 2013, Antoine Pelisse wrote: With redirects taken into account, I can't think of any really good way around avoiding this init... Is there any way for curl to initialize SSL on-demand ? Yes, but not without drawbacks. If you don't call curl_global_init() at all, libcurl will notice that on first use and then libcurl will call global_init by itself with a default bitmask. That automatic call of course will prevent the application from being able to set its own bitmask choice, and also the global_init function is not (necessarily) thread safe while all other libcurl functions are so the internal call to global_init from an otherwise thread-safe function is unfortunate. So in short, unless you are writing a custom application to talk to servers that you know will never redirect you to HTTPS, passing custom masks such as ALL~SSL to global-init is not going to be a valid optimization. I think that is a reasonable API; your custom application may want to go around your intranet servers all of which serve their status over plain HTTP, and it is a valid optimization to initialize the library with ALL~SSL. It is just that such an optimization does not apply to us---we let our users go to random hosts we have no control over, and they may redirect us in ways we cannot anticipate. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] combine-diff: coalesce lost lines optimally
Antoine Pelisse apeli...@gmail.com writes: +/* Coalesce new lines into base by finding LCS */ +static struct lline *coalesce_lines(struct lline *base, int *lenbase, + struct lline *new, int lennew, + unsigned long parent) +{ Don't you want to pass flags so that you can use match_string_spaces() at places like this: + for (i = 1, baseend = base; i origbaselen + 1; i++) { + for (j = 1, newend = new; j lennew + 1; j++) { + if (baseend-len == newend-len + !memcmp(baseend-line, newend-line, baseend-len)) { IOW, it looks to me that this wants to be rebuilt on top of fa04ae0be8cc (Allow combined diff to ignore white-spaces, 2013-03-14). -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] remote.name.pushurl does not consider aliases when pushing
On Sun, 17 Mar 2013 15:14:32 -0700 Junio C Hamano gits...@pobox.com wrote: Rob Hoelz r...@hoelz.ro writes: Hi everyone! I found a bug in Git today and wrote up a fix; I did my best to conform to the rules layed out in Documentation/SubmittingPatches, but please let me know if I need to change anything to get my work merged. =) I have CC'ed Josh Triplet, as he was the last one to touch the line I modified. I hope my commit messages explain the problem I encountered well enough; if not, I can always go back and amend them. Patches follow. -Rob Please read Documentation/SubmittingPatches and follow it. The above is most likely to be the cover letter of a two-patch series (meaning you will be sending three pieces of e-mail messages), or perhaps out of band comment below the three-dash line of a single patch (you will send only one piece of e-mail message). See recent patches on the list from list regulars for good examples, e.g. http://thread.gmane.org/gmane.comp.version-control.git/218350 http://thread.gmane.org/gmane.comp.version-control.git/218177/focus=218361 From 5007b11e86c0835807632cb41e6cfa75ce9a1aa1 Mon Sep 17 00:00:00 2001 From: Rob Hoelz r...@hoelz.ro Date: Sun, 17 Mar 2013 21:49:20 +0100 Subject: [PATCH 1/2] Add test for pushInsteadOf + pushurl git push currently doesn't consider pushInsteadOf when using pushurl; this test tests that. Signed-off-by: Rob Hoelz r...@hoelz.ro --- t/t5500-push-pushurl.sh | 37 + 1 file changed, 37 insertions(+) create mode 100644 t/t5500-push-pushurl.sh The number 5500 is already taken. Please do not add a duplicate. I also wonder if we need to waste a new test number for this; perhaps adding new tests to 5516 that already tests insteadOf might be a better fit, but I didn't carefully read it. diff --git a/t/t5500-push-pushurl.sh b/t/t5500-push-pushurl.sh new file mode 100644 Test scripts are supposed to be executable. +test_expect_success 'test commit and push' ' + test_commit one + git push origin master:master +' + +test_expect_success 'check for commits in rw repo' ' + cd ../rw/repo + git log --pretty=oneline | grep -q . +' Are both expected to succeed in patch 1/2 without any code change? If you were doing a large code change, it is a good series structure to have tests first that are marked as expect_failure in an early patch, and then in a later patch that changes the code to fix it, update the tests that start to pass to expect_success. I personally do not think you need such a two-step approach for something small like this; instead you can just have a single patch that adds a set of tests that expect success and code change. Thanks. Thanks for the feeback; another reply with the new patch follows. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] push: Alias pushurl from push rewrites
git push currently doesn't consider pushInsteadOf when using pushurl; this tests and fixes that. If you use pushurl with an alias that has a pushInsteadOf configuration value, Git does not take advantage of it. For example: [url git://github.com/] insteadOf = github: [url git://github.com/myuser/] insteadOf = mygithub: [url g...@github.com:myuser/] pushInsteadOf = mygithub: [remote origin] url = github:organization/project pushurl = mygithub:project Signed-off-by: Rob Hoelz r...@hoelz.ro --- remote.c | 2 +- t/t5516-fetch-push.sh | 20 2 files changed, 21 insertions(+), 1 deletion(-) diff --git a/remote.c b/remote.c index ca1f8f2..de7a915 100644 --- a/remote.c +++ b/remote.c @@ -465,7 +465,7 @@ static void alias_all_urls(void) if (!remotes[i]) continue; for (j = 0; j remotes[i]-pushurl_nr; j++) { - remotes[i]-pushurl[j] = alias_url(remotes[i]-pushurl[j], rewrites); + remotes[i]-pushurl[j] = alias_url(remotes[i]-pushurl[j], rewrites_push); } add_pushurl_aliases = remotes[i]-pushurl_nr == 0; for (j = 0; j remotes[i]-url_nr; j++) { diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh index b5417cc..272e225 100755 --- a/t/t5516-fetch-push.sh +++ b/t/t5516-fetch-push.sh @@ -244,6 +244,26 @@ test_expect_success 'push with pushInsteadOf and explicit pushurl (pushInsteadOf ) ' +test_expect_success 'push with pushInsteadOf and explicit pushurl (pushInsteadOf does rewrite in this case)' ' + mk_empty + TRASH=$(pwd)/ + mkdir ro + mkdir rw + git init --bare rw/testrepo + git config url.file://$TRASH/ro/.insteadOf ro: + git config url.file://$TRASH/rw/.pushInsteadOf rw: + git config remote.r.url ro:wrong + git config remote.r.pushurl rw:testrepo + git push r refs/heads/master:refs/remotes/origin/master + ( + cd rw/testrepo + r=$(git show-ref -s --verify refs/remotes/origin/master) + test z$r = z$the_commit + + test 1 = $(git for-each-ref refs/remotes/origin | wc -l) + ) +' + test_expect_success 'push with matching heads' ' mk_test heads/master -- 1.8.2 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] push: Alias pushurl from push rewrites
Rob Hoelz r...@hoelz.ro writes: git push currently doesn't consider pushInsteadOf when using pushurl; this tests and fixes that. If you use pushurl with an alias that has a pushInsteadOf configuration value, Git does not take advantage of it. For example: [url git://github.com/] insteadOf = github: [url git://github.com/myuser/] insteadOf = mygithub: [url g...@github.com:myuser/] pushInsteadOf = mygithub: [remote origin] url = github:organization/project pushurl = mygithub:project Incomplete sentence? For example [this is an example configuration] and then what happens? Something like with the sample configuration, 'git push origin' should follow pushurl and then turn it into X, but instead it ends up accessing Y. If there is no pushInsteadOf, does it still follow insteadOf? Is it tested already? Wouldn't you want to cover all the combinations to negative cases (i.e. making sure the codepath to support a new case does not affect behaviour of the code outside the new case)? A remote with and without pushurl (two combinations) and a pseudo URL scheme with and without pushInsteadOf (again, two combinations) will give you four cases. Thanks. Signed-off-by: Rob Hoelz r...@hoelz.ro --- remote.c | 2 +- t/t5516-fetch-push.sh | 20 2 files changed, 21 insertions(+), 1 deletion(-) diff --git a/remote.c b/remote.c index ca1f8f2..de7a915 100644 --- a/remote.c +++ b/remote.c @@ -465,7 +465,7 @@ static void alias_all_urls(void) if (!remotes[i]) continue; for (j = 0; j remotes[i]-pushurl_nr; j++) { - remotes[i]-pushurl[j] = alias_url(remotes[i]-pushurl[j], rewrites); + remotes[i]-pushurl[j] = alias_url(remotes[i]-pushurl[j], rewrites_push); } add_pushurl_aliases = remotes[i]-pushurl_nr == 0; for (j = 0; j remotes[i]-url_nr; j++) { diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh index b5417cc..272e225 100755 --- a/t/t5516-fetch-push.sh +++ b/t/t5516-fetch-push.sh @@ -244,6 +244,26 @@ test_expect_success 'push with pushInsteadOf and explicit pushurl (pushInsteadOf ) ' +test_expect_success 'push with pushInsteadOf and explicit pushurl (pushInsteadOf does rewrite in this case)' ' + mk_empty + TRASH=$(pwd)/ + mkdir ro + mkdir rw + git init --bare rw/testrepo + git config url.file://$TRASH/ro/.insteadOf ro: + git config url.file://$TRASH/rw/.pushInsteadOf rw: + git config remote.r.url ro:wrong + git config remote.r.pushurl rw:testrepo + git push r refs/heads/master:refs/remotes/origin/master + ( + cd rw/testrepo + r=$(git show-ref -s --verify refs/remotes/origin/master) + test z$r = z$the_commit + + test 1 = $(git for-each-ref refs/remotes/origin | wc -l) + ) +' + test_expect_success 'push with matching heads' ' mk_test heads/master -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v1 11/45] parse_pathspec: support stripping submodule trailing slashes
On Mon, Mar 18, 2013 at 4:55 AM, Junio C Hamano gits...@pobox.com wrote: Nguyễn Thái Ngọc Duy pclo...@gmail.com writes: This flag is equivalent to builtin/ls-files.c:strip_trailing_slashes() and is intended to replace that function when ls-files is converted to use parse_pathspec. Signed-off-by: Nguyễn Thái Ngọc Duy pclo...@gmail.com --- pathspec.c | 9 + pathspec.h | 1 + 2 files changed, 10 insertions(+) diff --git a/pathspec.c b/pathspec.c index b2446c3..2da8bc9 100644 --- a/pathspec.c +++ b/pathspec.c @@ -204,6 +204,15 @@ static unsigned prefix_pathspec(struct pathspec_item *item, *raw = item-match = match; item-original = elt; item-len = strlen(item-match); + + if ((flags PATHSPEC_STRIP_SUBMODULE_SLASH_CHEAP) I see that having cheap and expensive variant at these steps is a way to achieve a bug-for-bug compatible rewrite of the existing code, but in the longer term should we lose one of them? After all, a path that points at the working tree of another repository that is beyond a symlink cannot be added to us as a submodule, so CHEAP can be used only when we know that any intermediate component on the path is not a symlink pointing elsewhere, and the user in the codepath of ls-files may know it. I did not concentrate on the future part. But yes only one should remain in long term. Cheap vs expensive does not say much. To put it differently, shouldn't CHEAP and EXPENSIVE be accompanied by in-code comment and updates to Documentation/technical/api-*.txt to help users of the API to decide which one to use? Will do (also for the comment on 14/45) -- Duy -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: git: how the pack-objects.c find the object's delta
Hi, I can't say from a first glance. Maybe git@vger can help? On Sun, Mar 17, 2013 at 10:08 PM, 方栋 fangd...@pipul.org wrote: hello i don't understand this: src in builtin/pack-objects.c find_deltas() function, line 1800: static void find_deltas(struct object_entry **list, unsigned *list_size, int window, int depth, unsigned *processed) { .. /* * Move the best delta base up in the window, after the * currently deltified object, to keep it longer. It will * be the first base object to be attempted next. */ if (entry-delta) { struct unpacked swap = array[best_base]; int dist = (window + idx - best_base) % window; int dst = best_base; while (dist--) { int src = (dst + 1) % window; array[dst] = array[src]; dst = src; } array[dst] = swap; } .. } what this code block use for? thx -- 祝好 --方(tyut) -- Duy -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 4/4] pack-refs: add fully-peeled trait
Signed-off-by: Michael Haggerty mhag...@alum.mit.edu and ACK for the whole series, once Junio's points are addressed. Regarding Junio's readability suggestion: I agree that his versions are a bit more readable, albeit at the expense of having to evaluate a bit more logic for each reference rather than just once when the header line is handled. So I don't have a preference either way. Michael On 03/17/2013 09:28 AM, Jeff King wrote: From: Michael Haggerty mhag...@alum.mit.edu Older versions of pack-refs did not write peel lines for refs outside of refs/tags. This meant that on reading the pack-refs file, we might set the REF_KNOWS_PEELED flag for such a ref, even though we do not know anything about its peeled value. The previous commit updated the writer to always peel, no matter what the ref is. That means that packed-refs files written by newer versions of git are fine to be read by both old and new versions of git. However, we still have the problem of reading packed-refs files written by older versions of git, or by other implementations which have not yet learned the same trick. The simplest fix would be to always unset the REF_KNOWS_PEELED flag for refs outside of refs/tags that do not have a peel line (if it has a peel line, we know it is valid, but we cannot assume a missing peel line means anything). But that loses an important optimization, as upload-pack should not need to load the object pointed to by refs/heads/foo to determine that it is not a tag. Instead, we add a fully-peeled trait to the packed-refs file. If it is set, we know that we can trust a missing peel line to mean that a ref cannot be peeled. Otherwise, we fall back to assuming nothing. [commit message and tests by Jeff King p...@peff.net] Signed-off-by: Jeff King p...@peff.net --- This uses Michael's approach for managing the flags within read_packed_refs, which is more readable. As I picked up his code and comments, I realized that there was basically nothing of mine left, so I switched the authorship. But do note: 1. It should have Michael's signoff, which was not present in the commit I lifted the code from. 2. I tweaked the big comment above read_packed_refs to reduce some ambiguities. Please double-check that I am not putting inaccurate words in your mouth. :) pack-refs.c | 2 +- refs.c | 43 +-- t/t3211-peel-ref.sh | 22 ++ 3 files changed, 64 insertions(+), 3 deletions(-) diff --git a/pack-refs.c b/pack-refs.c index ebde785..4461f71 100644 --- a/pack-refs.c +++ b/pack-refs.c @@ -128,7 +128,7 @@ int pack_refs(unsigned int flags) die_errno(unable to create ref-pack file structure); /* perhaps other traits later as well */ - fprintf(cbdata.refs_file, # pack-refs with: peeled \n); + fprintf(cbdata.refs_file, # pack-refs with: peeled fully-peeled \n); for_each_ref(handle_one_ref, cbdata); if (ferror(cbdata.refs_file)) diff --git a/refs.c b/refs.c index 175b9fc..bdeac28 100644 --- a/refs.c +++ b/refs.c @@ -803,11 +803,39 @@ static void read_packed_refs(FILE *f, struct ref_dir *dir) return line; } +/* + * Read f, which is a packed-refs file, into dir. + * + * A comment line of the form # pack-refs with: may contain zero or + * more traits. We interpret the traits as follows: + * + * No traits: + * + * Probably no references are peeled. But if the file contains a + * peeled value for a reference, we will use it. + * + * peeled: + * + * References under refs/tags/, if they *can* be peeled, *are* + * peeled in this file. References outside of refs/tags/ are + * probably not peeled even if they could have been, but if we find + * a peeled value for such a reference we will use it. + * + * fully-peeled: + * + * All references in the file that can be peeled are peeled. + * Inversely (and this is more important, any references in the + * file for which no peeled value is recorded is not peelable. This + * trait should typically be written alongside fully-peeled for + * compatibility with older clients, but we do not require it + * (i.e., peeled is a no-op if fully-peeled is set). + */ static void read_packed_refs(FILE *f, struct ref_dir *dir) { struct ref_entry *last = NULL; char refline[PATH_MAX]; int flag = REF_ISPACKED; + int refs_tags_peeled = 0; while (fgets(refline, sizeof(refline), f)) { unsigned char sha1[20]; @@ -816,8 +844,10 @@ static void read_packed_refs(FILE *f, struct ref_dir *dir) if (!strncmp(refline, header, sizeof(header)-1)) { const char *traits = refline + sizeof(header) - 1; - if (strstr(traits, peeled )) + if (strstr(traits, fully-peeled ))
Re: Tag peeling peculiarities
On 03/16/2013 02:38 PM, Michael Haggerty wrote: On 03/16/2013 10:34 AM, Jeff King wrote: On Sat, Mar 16, 2013 at 09:48:42AM +0100, Michael Haggerty wrote: My patch series is nearly done. I will need another day or two to review and make it submission-ready, but I wanted to give you an idea of what I'm up to and I could also use your feedback on some points. I will wait for the dust to settle on Peff's peel-ref optimization fixes patches, then rebase my series on top of his before submitting. The rest of my series is not so urgent so I don't think the delay will be a problem. Michael -- Michael Haggerty mhag...@alum.mit.edu http://softwareswirl.blogspot.com/ -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
git branch based hook desigh
Hello list, I have implemented git pre-received hook successfully. And it works on the repo level. Could anyone suggest how to call branch level hook please ?-- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html