Re: [PATCH 1/2] pack-refs: write peeled entry for non-tags

2013-03-17 Thread Jeff King
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

2013-03-17 Thread Jeff King
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

2013-03-17 Thread Junio C Hamano
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

2013-03-17 Thread Junio C Hamano
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

2013-03-17 Thread Junio C Hamano
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

2013-03-17 Thread Junio C Hamano
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()

2013-03-17 Thread Junio C Hamano
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

2013-03-17 Thread Jeff King
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

2013-03-17 Thread Jeff King
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)

2013-03-17 Thread Jeff King
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

2013-03-17 Thread Jeff King
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

2013-03-17 Thread Jeff King
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

2013-03-17 Thread Jeff King
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

2013-03-17 Thread Jeff King
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

2013-03-17 Thread Jeff King
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

2013-03-17 Thread Jeff King
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

2013-03-17 Thread Jeff King
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

2013-03-17 Thread Eric Sunshine
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

2013-03-17 Thread Eric Sunshine
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 %

2013-03-17 Thread Eric Sunshine
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

2013-03-17 Thread Eric Sunshine
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()

2013-03-17 Thread René Scharfe

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

2013-03-17 Thread Manuel Bua

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

2013-03-17 Thread Antoine Pelisse
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

2013-03-17 Thread Duy Nguyen
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

2013-03-17 Thread Antoine Pelisse
 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

2013-03-17 Thread Steven Walter
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

2013-03-17 Thread René Scharfe

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

2013-03-17 Thread Junio C Hamano
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

2013-03-17 Thread Junio C Hamano
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

2013-03-17 Thread Pete Wyckoff
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

2013-03-17 Thread Junio C Hamano
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?

2013-03-17 Thread Junio C Hamano
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

2013-03-17 Thread Antoine Pelisse
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

2013-03-17 Thread Rob Hoelz
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

2013-03-17 Thread Junio C Hamano
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

2013-03-17 Thread Junio C Hamano
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

2013-03-17 Thread Daniel Stenberg

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

2013-03-17 Thread Junio C Hamano
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

2013-03-17 Thread Kacper Kornet
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

2013-03-17 Thread Junio C Hamano
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

2013-03-17 Thread Junio C Hamano
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

2013-03-17 Thread Rob Hoelz
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

2013-03-17 Thread Rob Hoelz
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

2013-03-17 Thread Junio C Hamano
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

2013-03-17 Thread Duy Nguyen
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

2013-03-17 Thread Duy Nguyen
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

2013-03-17 Thread Michael Haggerty
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

2013-03-17 Thread Michael Haggerty
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

2013-03-17 Thread Joydeep Bakshi
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