Re: [PATCH 3/2] t5004: resurrect original empty tar archive test

2013-05-11 Thread Jonathan Nieder
Hi,

René Scharfe wrote:

 [Subject: t5004: resurrect original empty tar archive test]
[...]
 The different approaches test different things: The existing one is
 for empty trees, for which we know the exact expected output and thus
 we can simply check it without extracting; the new one is for commits
 with empty trees, whose archives include stamps and so the more
 natural check by extraction is a better fit because it focuses on
 the interesting aspect, namely the absence of any archive entries.

 Signed-off-by: Rene Scharfe rene.scha...@lsrfire.ath.cx

When first reading I was a little confused: does this patch resurrect
the original, existing test for empty tree handling in the form it had
before patch 2/3, or is it adding a new, distinct test that
complements the existing one that patch 2/3 modified?

A quick glance back at v1.8.2.2~7^2 (t5004: fix issue with empty
archive test and bsdtar, 2013-04-10) cleared matters up.  The original
test that is being resurrected is the one from before that commit.

Maybe a reminder in the commit message would help.  E.g.,

The earlier version of the same check (before 24676f02, t5004: fix
issue with empty archive test and bsdtar) revived by this patch tests
a different thing: The modified check is for empty trees, for which we
know the exact expected output and thus we can simply check it without
extracting; the original one is for commits with empty trees, whose
archives include stamps and so the more natural check by extraction
is a better fit because it focuses on the interesting aspect, namely
the absence of any archive entries.

With or without such a change,
Reviewed-by: Jonathan Nieder jrnie...@gmail.com

Would it make sense to define HEADER_ONLY_TAR_OK as a lazy prereq in
the same file (even though it is only used once), so the code that
checks tar is not run if this test is being skipped (e.g.,
using GIT_TEST_SKIP) for some other reason?

Thanks,
Jonathan
--
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 support for -G'regex' pickaxe variant

2013-05-11 Thread Jonathan Nieder
Paul Mackerras wrote:

 I thought I had replied to this patch; maybe I only thought about it.

 Given that we already have a selector to choose between exact and
 regexp matching, it seems more natural to use that rather than add a
 new selector entry.  Arguably the IgnCase option should be disabled
 when adding/removing string is selected.

Thanks.  I think I disagree: log -G and log -S are different
operations, not variations on the same one.  

The description Find next commit adding/removing string: very
clearly conveys what -S means.  Maybe -G would be more clearly
described as Find next commit changing line that matches regex: or
Find next commit changing line containing:?

Jonathan
--
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] cache.h: eliminate SHA-1 deprecation warnings on OS X 10.8

2013-05-11 Thread Jonathan Nieder
Hi,

David Aguilar wrote:

 Mac OS X Mountain Lion prints warnings when building git:

   warning: 'SHA1_Init' is deprecated
   (declared at /usr/include/openssl/sha.h:121)

 Silence the warnings by using the Common Digest SHA-1
 functions for SHA1_Init(), SHA1_Update(), and SHA1_Final().

Thanks.

Does this perform better or worse than just setting
BLK_SHA1=YesPlease?  I'd naively think it could go either way: on one
hand adding another library dependency can slow down startup, and on
the other hand the implementation may or may not be optimized better
than the generic block-sha1/ implementation.

Curious,
Jonathan
--
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] imap-send: eliminate HMAC warnings on OS X 10.8

2013-05-11 Thread Jonathan Nieder
David Aguilar wrote:

 Mac OS X Mountain Lion warns that HMAC_Init() and friends are
 deprecated.  Use CommonCrypto's HMAC to eliminate the warnings.

Makes sense, and if the #define trick stops working some day
due to some conflicting macro in an openssl header some day, it
would just break the build, so this should be safe.  For what it's
worth,

Reviewed-by: Jonathan Nieder jrnie...@gmail.com

[...]
 This builds upon the patch I sent earlier, so technically it's 2/2

I'm less sure about whether patch 1/2 is a good idea.  Luckily the
conflicts when trying to untangle the two are only textual.

Thanks,
Jonathan
--
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] Makefile: fix default regex settings on Darwin

2013-05-11 Thread Jonathan Nieder
David Aguilar wrote:

 t0070-fundamental.sh fails on Mac OS X 10.8 by default.
 Fix it by using Git's regex library.

Can you say more about the failure?  What does

./t0070-fundamental.sh -v

say?

Curious,
Jonathan
--
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] Makefile: fix default regex settings on Darwin

2013-05-11 Thread David Aguilar
On Fri, May 10, 2013 at 11:31 PM, Jonathan Nieder jrnie...@gmail.com wrote:
 David Aguilar wrote:

 t0070-fundamental.sh fails on Mac OS X 10.8 by default.
 Fix it by using Git's regex library.

 Can you say more about the failure?  What does

 ./t0070-fundamental.sh -v

 say?

[..snip...]

expecting success:
# if this test fails, re-build git with NO_REGEX=1
test-regex

fatal: regex bug confirmed: re-build git with NO_REGEX=1
not ok 4 - check for a bug in the regex routines
#
# # if this test fails, re-build git with NO_REGEX=1
# test-regex
#

# failed 1 among 4 test(s)
1..4
--
David
--
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] Makefile: fix default regex settings on Darwin

2013-05-11 Thread Jonathan Nieder
David Aguilar wrote:

 expecting success:
 # if this test fails, re-build git with NO_REGEX=1
 test-regex

 fatal: regex bug confirmed: re-build git with NO_REGEX=1

Thanks.  Gah.  That means that regcomp() with REG_NEWLINE is letting

[^={} \t]+

match the newline in

={}\nfred

despite the POSIX requirement

A newline in string shall not be matched by a period outside
a bracket expression or by any form of a non-matching list

where

A non-matching list expression begins with a circumflex ('^')
and specifies a list that shall match any single-character
collating element except for the expressions represented in
the list after the leading circumflex.

and if I understand you correctly, this is a regression in Apple
libc. :(

With the commit message modified to mention the fatal: regex bug
confirmed: re-build git with NO_REGEX=1 and uname -a output,

Reviewed-by: Jonathan Nieder jrnie...@gmail.com

Do you know if this has been reported to Apple and
openradar.appspot.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


Re: [PATCH v2] cache.h: eliminate SHA-1 deprecation warnings on OS X 10.8

2013-05-11 Thread David Aguilar
On Fri, May 10, 2013 at 11:23 PM, Jonathan Nieder jrnie...@gmail.com wrote:
 Hi,

 David Aguilar wrote:

 Mac OS X Mountain Lion prints warnings when building git:

   warning: 'SHA1_Init' is deprecated
   (declared at /usr/include/openssl/sha.h:121)

 Silence the warnings by using the Common Digest SHA-1
 functions for SHA1_Init(), SHA1_Update(), and SHA1_Final().

 Thanks.

 Does this perform better or worse than just setting
 BLK_SHA1=YesPlease?  I'd naively think it could go either way: on one
 hand adding another library dependency can slow down startup, and on
 the other hand the implementation may or may not be optimized better
 than the generic block-sha1/ implementation.

Pretty much identical.

Here are the timings (I should probably read t/perf/README and get
better numbers):

Best of ten
$ time git rev-list --all --objects /dev/null

# master
git rev-list --all --objects  /dev/null  5.16s user 0.11s system 99%
cpu 5.277 total

# BLK_SHA1
git rev-list --all --objects  /dev/null  5.17s user 0.11s system 99%
cpu 5.299 total

# CommonDigest
git rev-list --all --objects  /dev/null  5.18s user 0.11s system 99%
cpu 5.301 total

The library startup cost is identical to master since at the end of
the day it's still libcrypto.

FWIW the way it's done in this patch still allows defining BLK_SHA1,
which is parity with how the Makefile behaves on Linux.

Re-roll?
--
David
--
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: Outdated and broken online versions of user-manual.html

2013-05-11 Thread Thomas Ackermann
W. Trevor King wking at tremily.us writes:

 
 I'm also surprised that I couldn't find a more obvious link to the
 manual from git-scm.com (I ended up taking a “See Also” link from
 gittutorial(7) [3]).  I'm not sure if this is intentional or not,
 since git-scm.com does prominently link Pro Git, and that overlaps
 fairly significantly with the manual.
 
 Folks with Git installed will generally have man pages, so it's not a
 big deal, but having current docs somewhere online to link against
 would be nice.  I'm also curious if I should be linking against a
 particular location.
 

IMHO user-manual is a natural step for a Git beginner after reading one 
of the books like Pro Git and before he is ready to digest the man pages. 
But up to now there are several problems with user-manual besides the
problems described by Trevor:
(1) Very poor html formatting (document type book causes
ugly TOCs per section and there's a Part I without a Part II)
(2) Partly outdated content
(3) Sub-optimal structuring (to-do list as part of the document,
glossary not at the end of the document)
(4) User-manual.PDF uses an independent tool chain which makes it
harder to do improvements for user-manual.html and also is the only
pdf doc we are creating. IMHO we should remove this altogether.
(5) Large overlapping with the tutorials. IMHO all of the 
tutorials should be blended into user-manual

I am currently working on (1)-(4) and then aiming for (5).
Comments are welcome ...

---
Thomas



--
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] Makefile: fix default regex settings on Darwin

2013-05-11 Thread David Aguilar
On Sat, May 11, 2013 at 12:04 AM, Jonathan Nieder jrnie...@gmail.com wrote:
 David Aguilar wrote:

 expecting success:
 # if this test fails, re-build git with NO_REGEX=1
 test-regex

 fatal: regex bug confirmed: re-build git with NO_REGEX=1

 Thanks.  Gah.  That means that regcomp() with REG_NEWLINE is letting

 [^={} \t]+

 match the newline in

 ={}\nfred

 despite the POSIX requirement

 A newline in string shall not be matched by a period outside
 a bracket expression or by any form of a non-matching list

 where

 A non-matching list expression begins with a circumflex ('^')
 and specifies a list that shall match any single-character
 collating element except for the expressions represented in
 the list after the leading circumflex.

 and if I understand you correctly, this is a regression in Apple
 libc. :(

 With the commit message modified to mention the fatal: regex bug
 confirmed: re-build git with NO_REGEX=1 and uname -a output,

 Reviewed-by: Jonathan Nieder jrnie...@gmail.com

Thanks. I can adjust the commit message and re-roll to make this patch 1/3.

 Do you know if this has been reported to Apple and
 openradar.appspot.com?

I checked and nope, there are no bug reports.  I tried to submit one
by creating a new radar but that page said to go to
bugreport.apple.com.

I tried to login there but after entering my credentials I got, An
error has occurred. Please report the error to Apple Inc. by emailing
the error detail to devb...@apple.com.  Oh bother ;-)

I refreshed the page and finally got in.  I submitted a bug.. Problem # 13868200

I hop on this laptop to test for cross-platform issues (I'm a Linux
user myself ;-) and it's been pretty good at finding 'em.
--
David
--
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] cache.h: eliminate SHA-1 deprecation warnings on OS X 10.8

2013-05-11 Thread Jeff King
On Sat, May 11, 2013 at 12:11:05AM -0700, David Aguilar wrote:

  Does this perform better or worse than just setting
  BLK_SHA1=YesPlease?  I'd naively think it could go either way: on one
  hand adding another library dependency can slow down startup, and on
  the other hand the implementation may or may not be optimized better
  than the generic block-sha1/ implementation.
 
 Pretty much identical.
 
 Here are the timings (I should probably read t/perf/README and get
 better numbers):
 
 Best of ten
 $ time git rev-list --all --objects /dev/null
 [...]

I'm not sure that's a great test of sha1 performance. It will hash the
commit and tree objects it loads during the traversal, but that time is
almost certainly dwarfed by zlib inflation and by lookup_object.

Adding --verify-objects would sha1 the blobs, too, which might be more
reasonable (or running git fsck). Something like git add on a large
blob would also be a good test.

-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/3] Makefile: fix default regex settings on Darwin

2013-05-11 Thread David Aguilar
t0070-fundamental.sh fails on Mac OS X 10.8:

$ uname -a
Darwin lustrous 12.2.0 Darwin Kernel Version 12.2.0:
Sat Aug 25 00:48:52 PDT 2012;
root:xnu-2050.18.24~1/RELEASE_X86_64 x86_64

$ ./t0070-fundamental.sh -v
fatal: regex bug confirmed: re-build git with NO_REGEX=1

Fix it by using Git's regex library.

Reviewed-by: Jonathan Nieder jrnie...@gmail.com
Signed-off-by: David Aguilar dav...@gmail.com
---
Added uname output and Jonathan's review tag.

 Makefile | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Makefile b/Makefile
index 0f931a2..f698c1a 100644
--- a/Makefile
+++ b/Makefile
@@ -1054,6 +1054,7 @@ ifeq ($(uname_S),Darwin)
BASIC_LDFLAGS += -L/opt/local/lib
endif
endif
+   NO_REGEX = YesPlease
PTHREAD_LIBS =
 endif
 
-- 
1.8.3.rc1.47.g41936fa

--
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/3] cache.h: eliminate SHA-1 deprecation warnings on OS X 10.8

2013-05-11 Thread David Aguilar
Mac OS X Mountain Lion prints warnings when building git:

warning: 'SHA1_Init' is deprecated
(declared at /usr/include/openssl/sha.h:121)

Silence the warnings by using the Common Digest SHA-1
functions for SHA1_Init(), SHA1_Update(), and SHA1_Final().

Add a COMMON_DIGEST_SHA1 knob to the Makefile to allow
choosing this implementation and define it by default on Darwin.

Signed-off-by: David Aguilar dav...@gmail.com
---
Unchanged since last time; rebased to 3/3.

 Makefile | 7 +++
 cache.h  | 7 +++
 2 files changed, 14 insertions(+)

diff --git a/Makefile b/Makefile
index 25282b4..8b2f9cc 100644
--- a/Makefile
+++ b/Makefile
@@ -1055,6 +1055,7 @@ ifeq ($(uname_S),Darwin)
endif
endif
COMMON_DIGEST_HMAC = YesPlease
+   COMMON_DIGEST_SHA1 = YesPlease
NO_REGEX = YesPlease
PTHREAD_LIBS =
 endif
@@ -1390,10 +1391,16 @@ ifdef PPC_SHA1
LIB_OBJS += ppc/sha1.o ppc/sha1ppc.o
LIB_H += ppc/sha1.h
 else
+ifdef COMMON_DIGEST_SHA1
+   BASIC_CFLAGS += -DCOMMON_DIGEST_FOR_SHA1=1
+   SHA1_HEADER = CommonCrypto/CommonDigest.h
+   EXTLIBS += $(LIB_4_CRYPTO)
+else
SHA1_HEADER = openssl/sha.h
EXTLIBS += $(LIB_4_CRYPTO)
 endif
 endif
+endif
 
 ifdef COMMON_DIGEST_HMAC
BASIC_CFLAGS += -DCOMMON_DIGEST_FOR_HMAC=1
diff --git a/cache.h b/cache.h
index 94ca1ac..e2b24c6 100644
--- a/cache.h
+++ b/cache.h
@@ -10,11 +10,18 @@
 
 #include SHA1_HEADER
 #ifndef git_SHA_CTX
+#ifdef COMMON_DIGEST_FOR_SHA1
+#define git_SHA_CTXCC_SHA1_CTX
+#define git_SHA1_Init  CC_SHA1_Init
+#define git_SHA1_UpdateCC_SHA1_Update
+#define git_SHA1_Final CC_SHA1_Final
+#else
 #define git_SHA_CTXSHA_CTX
 #define git_SHA1_Init  SHA1_Init
 #define git_SHA1_UpdateSHA1_Update
 #define git_SHA1_Final SHA1_Final
 #endif
+#endif
 
 #include zlib.h
 typedef struct git_zstream {
-- 
1.8.3.rc1.47.g41936fa

--
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] cache.h: eliminate SHA-1 deprecation warnings on OS X 10.8

2013-05-11 Thread David Aguilar
On Sat, May 11, 2013 at 1:22 AM, Jeff King p...@peff.net wrote:
 On Sat, May 11, 2013 at 12:11:05AM -0700, David Aguilar wrote:

  Does this perform better or worse than just setting
  BLK_SHA1=YesPlease?  I'd naively think it could go either way: on one
  hand adding another library dependency can slow down startup, and on
  the other hand the implementation may or may not be optimized better
  than the generic block-sha1/ implementation.

 Pretty much identical.

 Here are the timings (I should probably read t/perf/README and get
 better numbers):

 Best of ten
 $ time git rev-list --all --objects /dev/null
 [...]

 I'm not sure that's a great test of sha1 performance. It will hash the
 commit and tree objects it loads during the traversal, but that time is
 almost certainly dwarfed by zlib inflation and by lookup_object.

 Adding --verify-objects would sha1 the blobs, too, which might be more
 reasonable (or running git fsck). Something like git add on a large
 blob would also be a good test.

Thanks.  Here are the numbers with --verify-objects:

$ time git rev-list --all --objects --verify-objects /dev/null

# CommonCrypto 32.24s user 4.65s system 99% cpu 37.098 total
# master   33.00s user 4.68s system 99% cpu 37.852 total
# BLK_SHA1 54.17s user 4.67s system 99% cpu 58.928 total

Doing BLK_SHA1 seems like less of a good idea now, so I think my
latest re-roll might be the way to go...
--
David
--
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] cache.h: eliminate SHA-1 deprecation warnings on OS X 10.8

2013-05-11 Thread Jeff King
On Sat, May 11, 2013 at 01:38:32AM -0700, David Aguilar wrote:

  Adding --verify-objects would sha1 the blobs, too, which might be more
  reasonable (or running git fsck). Something like git add on a large
  blob would also be a good test.
 
 Thanks.  Here are the numbers with --verify-objects:
 
 $ time git rev-list --all --objects --verify-objects /dev/null
 
 # CommonCrypto 32.24s user 4.65s system 99% cpu 37.098 total
 # master   33.00s user 4.68s system 99% cpu 37.852 total
 # BLK_SHA1 54.17s user 4.67s system 99% cpu 58.928 total
 
 Doing BLK_SHA1 seems like less of a good idea now, so I think my
 latest re-roll might be the way to go...

Wow, that's really terrible. What level of optimization do you compile
with? With the other implementations, you are calling into
(presumably) optimized library code, but with BLK_SHA1 you are getting
whatever you just compiled.

Here are three timings that show how big a difference that can make:

  openssl,  -O00m21.152s
  BLK_SHA1, -O00m31.920s
  BLK_SHA1, -O30m19.652s

So it is a win over openssl with optimizations on, but a pretty big loss
otherwise.

-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: Re: [PATCH v3 5/5] do not die when error in config parsing of buf occurs

2013-05-11 Thread Heiko Voigt
On Fri, May 10, 2013 at 12:39:37AM +0200, Jeff King wrote:
 On Thu, May 09, 2013 at 06:21:02PM +0200, Heiko Voigt wrote:
 
  diff --git a/builtin/config.c b/builtin/config.c
  index 8d01b7a..de32977 100644
  --- a/builtin/config.c
  +++ b/builtin/config.c
  @@ -219,9 +219,11 @@ static int get_value(const char *key_, const char 
  *regex_)
  }
  }
   
  -   git_config_with_options(collect_config, values,
  +   ret = git_config_with_options(collect_config, values,
  given_config_file, given_config_blob,
  respect_includes);
  +   if (ret  0)
  +   goto free_values;
   
  ret = !values.nr;
   
  @@ -231,6 +233,7 @@ static int get_value(const char *key_, const char 
  *regex_)
  fwrite(buf-buf, 1, buf-len, stdout);
  strbuf_release(buf);
  }
  +free_values:
  free(values.items);
   
   free_strings:
 
 Hmm. values is a strbuf_list. Don't we need to strbuf_release() its
 individual members before freeing it? The writing loop directly above
 your label frees each one after writing. It would probably make sense to
 just split it into two loops, one for writing, and then one for
 free-ing. This is not a critical performance path that we can't iterate
 over the (probably handful of) entries twice.

Thanks for catching that. I missed the strbuf_release in the loop
somehow. Will fix in the next iteration.

  diff --git a/t/t1307-config-blob.sh b/t/t1307-config-blob.sh
  index fdc257e..a4929eb 100755
  --- a/t/t1307-config-blob.sh
  +++ b/t/t1307-config-blob.sh
  @@ -55,12 +55,17 @@ test_expect_success 'editing a blob is an error' '
   test_expect_success 'parse errors in blobs are properly attributed' '
  cat config -\EOF 
  [some]
  -   value = 
  +   value = 1
  +   error = 
  EOF
  git add config 
  git commit -m broken 
   
  -   test_must_fail git config --blob=HEAD:config some.value 2err 
  +   test_must_fail git config --blob=HEAD:config some.value 1out 2err 
  +
  +   # Make sure there is no output of values on stdout
  +   touch out.expect 
  +   test_cmp out.expect out 
 
 I'm not clear on what is being tested here. Were we outputting to stdout
 before this patch (I would have thought our die() meant we did not.

We where not outputting to stdout before so the test is correct there as
well. I extended the test when implementing the non-dying version of
git_config_with_options() so I could see the test fail. If just
returning the error it would still output the values read until then. So
if you think that it belongs into the initial version of this test
(maybe including some comment why we need an extra parseable value) I
am happy to move it there.

Cheers Heiko
--
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] cache.h: eliminate SHA-1 deprecation warnings on OS X 10.8

2013-05-11 Thread David Aguilar
On Sat, May 11, 2013 at 1:45 AM, Jeff King p...@peff.net wrote:
 On Sat, May 11, 2013 at 01:38:32AM -0700, David Aguilar wrote:

  Adding --verify-objects would sha1 the blobs, too, which might be more
  reasonable (or running git fsck). Something like git add on a large
  blob would also be a good test.

 Thanks.  Here are the numbers with --verify-objects:

 $ time git rev-list --all --objects --verify-objects /dev/null

 # CommonCrypto 32.24s user 4.65s system 99% cpu 37.098 total
 # master   33.00s user 4.68s system 99% cpu 37.852 total
 # BLK_SHA1 54.17s user 4.67s system 99% cpu 58.928 total

 Doing BLK_SHA1 seems like less of a good idea now, so I think my
 latest re-roll might be the way to go...

 Wow, that's really terrible. What level of optimization do you compile
 with? With the other implementations, you are calling into
 (presumably) optimized library code, but with BLK_SHA1 you are getting
 whatever you just compiled.

 Here are three timings that show how big a difference that can make:

   openssl,  -O00m21.152s
   BLK_SHA1, -O00m31.920s
   BLK_SHA1, -O30m19.652s

 So it is a win over openssl with optimizations on, but a pretty big loss
 otherwise.

Good catch.  I had a config.mak without any -O flags in CFLAGS.
Here are the timings with -O3.  We're back to parity.

$ time git rev-list --all --objects --verify-objects /dev/null

# CommonCrypto 28.95s user 4.62s system 99% cpu 33.630 total
# master   29.81s user 4.70s system 99% cpu 34.760 total
# BLK_SHA1 29.80s user 4.62s system 99% cpu 34.505 total

If BLK_SHA1 were the default on all platforms then I wouldn't have
bothered with the SHA-1 patch.  With this patch it makes it like Linux
in that Git can choose between the built-in functions and the external
library.

That's why I moved this patch to 3/3.. it could go either way.
--
David
--
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: [PATCHv3 3/7] show: honor --textconv for blobs

2013-05-11 Thread Michael J Gruber
Junio C Hamano venit, vidit, dixit 10.05.2013 19:02:
 Michael J Gruber g...@drmicha.warpmail.net writes:
 
 Currently, diff and cat-file for blobs honor --textconv options
 (with the former defaulting to --textconv and the latter to
 --no-textconv) whereas show does not honor this option, even though
 it takes diff options.

 Make show on blobs behave like diff, i.e. honor --textconv by
 default and --no-textconv when given.
 
 Hmm...

Sorry, I overlooked that ;)

 +static int show_blob_object(const unsigned char *sha1, struct rev_info 
 *rev, const char *obj_name)
  {
 +unsigned char sha1c[20];
 +struct object_context obj_context;
 +char *buf;
 +unsigned long size;
 +
  fflush(stdout);
 -return stream_blob_to_fd(1, sha1, NULL, 0);
 +if (!DIFF_OPT_TOUCHED(rev-diffopt, ALLOW_TEXTCONV) ||
 +!DIFF_OPT_TST(rev-diffopt, ALLOW_TEXTCONV))
 +return stream_blob_to_fd(1, sha1, NULL, 0);
 
 It is surprising that the necessary change is only this, but I think
 it is correct ;-).  We ignore textconv when the command line did not
 mention --[no-]textconv, or the command line said --no-textconv
 explicitly.
 
 This (especially the first condition) may deserve an in-code comment
 for anybody who wonders where this default behaviour is implemented.

It's not as if we would document behavior by in-code comments in
general, do we? The usual answer is git log -S or git blame.

 So show on blobs does show the raw contents by default, but the
 user can explicitly ask to enable textconv with --[no-]textconv.  Is
 the second paragraph in the log message still valid?
 
 +if (get_sha1_with_context(obj_name, 0, sha1c, obj_context))
 +die(Not a valid object name %s, obj_name);
 
 This looks somewhat unfortunate.
 
 We already have sha1[]; actually we not just know sha1[] but have
 the struct object for it.  How did we obtain it before we got here?
 
 Will we always have a valid name in rev.pending.objects-name?  Will
 that name convert back to the same sha1 we got in sha1[]?
 
 I think the answers are Yes (it is a command line argument), Yes
 (that is what setup_revisions() got by feeding the name to give us
 sha1[]).
 
 I wonder if enriching rev_info-pending with the context information
 might be a clean solution to avoid this redundant but unavoidable
 conversion, but that is a separate and future topic, I think.

Yes, I think both Jeff and I have thought it and came to the same
conclusion - later ;)

 +if (!obj_context.path[0] ||
 +!textconv_object(obj_context.path, obj_context.mode, sha1c, 1, 
 buf, size))
 +return stream_blob_to_fd(1, sha1, NULL, 0);
 +
 +if (!buf)
 +die(git show %s: bad file, obj_name);
 +
 +write_or_die(1, buf, size);
 +return 0;
  }
  
  static int show_tag_object(const unsigned char *sha1, struct rev_info *rev)
 @@ -526,7 +545,7 @@ int cmd_show(int argc, const char **argv, const char 
 *prefix)
  const char *name = objects[i].name;
  switch (o-type) {
  case OBJ_BLOB:
 -ret = show_blob_object(o-sha1, NULL);
 +ret = show_blob_object(o-sha1, rev, name);
  break;
  case OBJ_TAG: {
  struct tag *t = (struct tag *)o;
 diff --git a/t/t4030-diff-textconv.sh b/t/t4030-diff-textconv.sh
 index 3950fc9..0ebb028 100755
 --- a/t/t4030-diff-textconv.sh
 +++ b/t/t4030-diff-textconv.sh
 @@ -96,14 +96,14 @@ test_expect_success 'show blob produces binary' '
  test_cmp expect actual
  '
  
 -test_expect_failure 'show --textconv blob produces text' '
 +test_expect_success 'show --textconv blob produces text' '
  git show --textconv HEAD:file actual 
  printf 0\\n1\\n expect 
  test_cmp expect actual
  '
  
 -test_success 'show --no-textconv blob produces binary' '
 -git show --textconv HEAD:file actual 
 +test_expect_success 'show --no-textconv blob produces binary' '
 +git show --no-textconv HEAD:file actual 
  printf \\0\\n\\01\\n expect 
  test_cmp expect actual
  '
--
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 support for -G'regex' pickaxe variant

2013-05-11 Thread Paul Mackerras
On Fri, May 10, 2013 at 11:13:22PM -0700, Jonathan Nieder wrote:
 Paul Mackerras wrote:
 
  I thought I had replied to this patch; maybe I only thought about it.
 
  Given that we already have a selector to choose between exact and
  regexp matching, it seems more natural to use that rather than add a
  new selector entry.  Arguably the IgnCase option should be disabled
  when adding/removing string is selected.
 
 Thanks.  I think I disagree: log -G and log -S are different
 operations, not variations on the same one.  

OK, fair enough, and I see there is in fact a --pickaxe-regex we
could use.

 The description Find next commit adding/removing string: very
 clearly conveys what -S means.  Maybe -G would be more clearly
 described as Find next commit changing line that matches regex: or
 Find next commit changing line containing:?

How about changing lines matching:?  If it gets too long it will
take up too much horizontal room.

Paul.
--
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


Thoughts about eliminating reachability races

2013-05-11 Thread Michael Haggerty
How to get rid of reachability races?

This email is meant more as a basis for discussion (including at the
GitMerge conference that is currently running) than as a fully-baked
proposal.

A lot of reachability-related races have been discussed recently:

* If a reference is renamed while a prune is reading the loose
  references, it could be that prune sees neither the old nor the new
  version of the reference and could prune the objects referred to by
  the reference.

* If a process is building a tree, it could find that some objects are
  already in the DB, in which case it will refer to the existing
  objects.  But these objects might be unreachable, and could be
  pruned before the process gets around to creating a root reference
  that makes them reachable.

* Other races involving packed refs vs. loose refs.

These races are most critical for busy hosting sites like GitHub but
could theoretically impact anybody.

It has been discussed that we should offer, as an alternative to
packed-refs/loose refs in the filesystem, a way to store refs in a
single database store of some kind that offers ACID guarantees.  This
would solve the first and last kind of race in one step.

Other mechanisms are being discussed to address the second race, maybe
involving moving pruned objects into a kind of purgatory for a period
of time before they are finally deleted.

But I have the feeling that these races could also be solved via the
use of the same database.

I am thinking of a scheme in which the database stores two more things
during prunes:

1. The names of objects that are being pruned.  Such objects are
   considered deleted (even though they might not yet have been
   deleted from the object database) and no new references may be made
   to them.

2. The names of objects to which new links have been added.  Prune is
   not allowed to delete this objects or objects to which they refer.

like those used for incremental garbage
collection of memory, in which *during* a garbage collection the VM
keeps tracks of new references that are made to objects that are in
the generation that is currently being garbage collected.  The garbage
collector has to consider these links in its reachability analysis.

Our situation is a little bit more complicated, because it is not easy
to inform a git process that a prune has been started.  At best, the
process can check occasionally whether a prune is running.  On the
other hand, it *is* permissible for Git processes to fail in
exceptional circumstances, as long as it happens rarely and leaves the
object store in a valid state.

So I'm thinking of a scheme in which

1. prune has to list doomed objects to the database before deleting
   them.  Such objects are considered deleted by other processes (*if*
   they know that a prune is running).

2. If a process knows that a prune is running, then it records to the
   database any new links that it makes to existing objects.  Prune is
   not allowed to prune such objects.

3. To catch the situation that a process didn't know that a prune is
   running, we keep a prune generation number.  If that number changes
   between the beginning/end of a process, then the process fails
   without recording any new references.


Design goals:

* Correctness

* Normal processes don't block on each other or on prune.

* Normal processes don't get significantly slower in the usual case
  (but are allowed to get slower during pruning).


Assumptions:

* There is a store with atomic transactions.

* Most normal processes run more quickly than a prune.  It will be
  rare for an entire prune to run while a normal process is running
  (though even if it does we must behave correctly).

* Normal processes are allowed to fail occasionally as long as they
  leave the DB in a valid state.


In the store, record the following values:

* Git references -- (refname, object_name) pairs

* prune_in_progress (boolean) -- a destructive operation is running

* prune_generation (integer) -- incremented each time objects are
  invalidated

* doomed_objects -- a list of the names of objects that a prune is in
  the process of deleting.  These objects *are considered deleted* and
  may not be referred to.  Only used when prune_in_progress.

* new_links -- a list of the names of objects newly created or newly
  referenced during a prune.  These objects are considered referenced
  and *may not be pruned*.  Only used when prune_in_progress.



A nondestructive operation will usually have two interactions with the
store.  At start::

def record_new_link(sha1):
Record that there is a new link to object sha1.

if prune_in_progress:
if sha1 in doomed_objects:
die()
append sha1 to new_links (RAM copy)


with transaction:
read prune_generation
read prune_in_progress
if prune_in_progress:
read doomed_objects
read any needed Git refs

for each new object or new link to 

Re: [PATCH v2] cache.h: eliminate SHA-1 deprecation warnings on OS X 10.8

2013-05-11 Thread Jeff King
On Sat, May 11, 2013 at 02:17:10AM -0700, David Aguilar wrote:

 Good catch.  I had a config.mak without any -O flags in CFLAGS.
 Here are the timings with -O3.  We're back to parity.
 
 $ time git rev-list --all --objects --verify-objects /dev/null
 
 # CommonCrypto 28.95s user 4.62s system 99% cpu 33.630 total
 # master   29.81s user 4.70s system 99% cpu 34.760 total
 # BLK_SHA1 29.80s user 4.62s system 99% cpu 34.505 total
 
 If BLK_SHA1 were the default on all platforms then I wouldn't have
 bothered with the SHA-1 patch.  With this patch it makes it like Linux
 in that Git can choose between the built-in functions and the external
 library.

With those numbers above, it seems like CommonCrypto is still a
reasonable choice. I don't know anything about availability or other OS
X specific issues, but assuming there are no issues there, your patch
makes sense to me. Thanks for double-checking the timings.

-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: Re: [PATCH v3 5/5] do not die when error in config parsing of buf occurs

2013-05-11 Thread Jeff King
On Sat, May 11, 2013 at 10:55:31AM +0200, Heiko Voigt wrote:

   diff --git a/t/t1307-config-blob.sh b/t/t1307-config-blob.sh
   index fdc257e..a4929eb 100755
   --- a/t/t1307-config-blob.sh
   +++ b/t/t1307-config-blob.sh
   @@ -55,12 +55,17 @@ test_expect_success 'editing a blob is an error' '
test_expect_success 'parse errors in blobs are properly attributed' '
 cat config -\EOF 
 [some]
   - value = 
   + value = 1
   + error = 
 EOF
 git add config 
 git commit -m broken 

   - test_must_fail git config --blob=HEAD:config some.value 2err 
   + test_must_fail git config --blob=HEAD:config some.value 1out 2err 
   +
   + # Make sure there is no output of values on stdout
   + touch out.expect 
   + test_cmp out.expect out 
  
  I'm not clear on what is being tested here. Were we outputting to stdout
  before this patch (I would have thought our die() meant we did not.
 
 We where not outputting to stdout before so the test is correct there as
 well. I extended the test when implementing the non-dying version of
 git_config_with_options() so I could see the test fail. If just
 returning the error it would still output the values read until then. So
 if you think that it belongs into the initial version of this test
 (maybe including some comment why we need an extra parseable value) I
 am happy to move it there.

From what you wrote above, it sounds like we would expect git-config to
write out some.value before failing on some.error. But that isn't what
the test is checking, is it? It is checking that nothing is output.

I do not think the output matters much either way when git-config has
failed; if it returns a non-zero exit code, then the results of stdout
cannot be trusted (after all, it may have been killed by signal after it
had written out half of the output).

Still slightly puzzled...

-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] gitk: Add menu item for reverting commits

2013-05-11 Thread Paul Mackerras
On Sat, Apr 27, 2013 at 04:36:13PM +0200, Knut Franke wrote:
 Sometimes it's helpful (at least psychologically) to have this feature
 easily accessible. Code borrows heavily from cherrypick.
 
 Signed-off-by: Knut Franke knut.fra...@gmx.de

Thanks, applied, after undoing the linewrapping (done by your mailer?).

Paul.
--
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: simplify file filtering

2013-05-11 Thread Paul Mackerras
On Sat, Apr 27, 2013 at 05:01:39PM -0500, Felipe Contreras wrote:
 git diff is perfectly able to do this with '-- files', no need for
 manual filtering.
 
 Signed-off-by: Felipe Contreras felipe.contre...@gmail.com

Thanks, applied, with the commit message expanded to say that this
makes gettreediffs do the same as getblobdiffs.

Paul.
--
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: Re: [PATCH v3 5/5] do not die when error in config parsing of buf occurs

2013-05-11 Thread Heiko Voigt
On Sat, May 11, 2013 at 11:59:36AM +0200, Jeff King wrote:
 On Sat, May 11, 2013 at 10:55:31AM +0200, Heiko Voigt wrote:
  We where not outputting to stdout before so the test is correct there as
  well. I extended the test when implementing the non-dying version of
  git_config_with_options() so I could see the test fail. If just
  returning the error it would still output the values read until then. So
  if you think that it belongs into the initial version of this test
  (maybe including some comment why we need an extra parseable value) I
  am happy to move it there.
 
 From what you wrote above, it sounds like we would expect git-config to
 write out some.value before failing on some.error. But that isn't what
 the test is checking, is it? It is checking that nothing is output.
 
 I do not think the output matters much either way when git-config has
 failed; if it returns a non-zero exit code, then the results of stdout
 cannot be trusted (after all, it may have been killed by signal after it
 had written out half of the output).
 
 Still slightly puzzled...

Well before my do not die patch there was no output to stdout since git
already died during reading.
Afterwards there would be output of values to stdout until the error
occurred and then the error code would be returned. So my intend was to
keep the output the same. Since the blob reading is new I thinks its
fine either way. So currently I am thinking about just removing the
whole stop to write anything on error part of this patch and just
return the error. What do you think?

Cheers Heiko
--
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 log --cherry with disjoint roots

2013-05-11 Thread John Keeping
I use my own integration branch manager[1] to manage my WIP changes to
various projects, including git.git and one of the features of this is a
--status option that shows whether anything that I'm tracking has been
merged to the base branch I'm building on top of.  Since the commit IDs
will be different after being sent to the list, this uses the --cherry
option to git-log.

Today I realised that I wasn't tracking a git-gui change I sent to the
list a couple of weeks ago [2] and so I pulled that in and added it.
Getting the status for this is significantly slower than anything else
because it does this:

git log --cherry --oneline origin/master...git-gui-fix-subdir

and although there are only 5 commits in git-gui-fix-subdir not in
master there are 31964 commits in master not in git-gui-fix-subdir and
--cherry seems to inspect all of those.  So I get:

$ time git log --oneline --cherry master...git-gui-fix-subdir \
/dev/null
real0m41.378s
user0m40.248s
sys 0m0.986s

Now I know that I don't need to check anything older than commit 8ead1bf
(Merge tag 'gitgui-0.17.0' of git://repo.or.cz/git-gui, 2012-10-17) and
if I tell Git that it gets significantly better:

$ time git log --oneline --cherry master...git-gui-fix-subdir \
--not 8ead1bfe111085ef1ad7759e67340f074996b244 /dev/null
real0m2.163s
user0m2.070s
sys 0m0.084s

I'd like a way to automatically find the last merge that pulled in a
subtree so that my script can construct the above command line without
me needing to tell it the correct SHA-1 every time I have a branch that
is affected by this.

I guess this boils down to having a way to ask Git to list merges that
have a parent in a specified range, but perhaps I'm missing an easier
solution to this.

I also wonder if it would be interesting to cache patch IDs rather than
generating them every time...

[1] http://johnkeeping.github.io/git-integration/
[2] http://article.gmane.org/gmane.comp.version-control.git/222646
--
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: Outdated and broken online versions of user-manual.html

2013-05-11 Thread Philip Oakley

From: Thomas Ackermann th.ac...@arcor.de
Sent: Saturday, May 11, 2013 8:48 AM

W. Trevor King wking at tremily.us writes:



I'm also surprised that I couldn't find a more obvious link to the
manual from git-scm.com (I ended up taking a “See Also” link from
gittutorial(7) [3]).  I'm not sure if this is intentional or not,
since git-scm.com does prominently link Pro Git, and that overlaps
fairly significantly with the manual.

Folks with Git installed will generally have man pages, so it's not a
big deal, but having current docs somewhere online to link against
would be nice.  I'm also curious if I should be linking against a
particular location.



IMHO user-manual is a natural step for a Git beginner after reading 
one
of the books like Pro Git and before he is ready to digest the man 
pages.

But up to now there are several problems with user-manual besides the
problems described by Trevor:
(1) Very poor html formatting (document type book causes
ugly TOCs per section and there's a Part I without a Part II)
(2) Partly outdated content
(3) Sub-optimal structuring (to-do list as part of the document,
glossary not at the end of the document)
(4) User-manual.PDF uses an independent tool chain which makes it
harder to do improvements for user-manual.html and also is the only
pdf doc we are creating. IMHO we should remove this altogether.
(5) Large overlapping with the tutorials. IMHO all of the
tutorials should be blended into user-manual

I am currently working on (1)-(4) and then aiming for (5).
Comments are welcome ...

---
Thomas


I too had noticed that both the User-Manual and git Everyday are not 
formatted in a manner that allows them to be accessed via 'git help'.


I recently added a '--guides' option to 'git help' (v1.8.2-377-g73903d0) 
which will show the common guides but was frustrated that I couldn't 
include either the user-manual or everday in the list.


I'd welcome the provision of a 'man'/'html' formatted version of the two 
guides. Are you expecting to make then compatible with the man format?


I would be a little cautious of your point 5 if it squoze everything 
into one overlong document at the expense of losing the shorter 
documents - one can't eat a whole melon in one bite ;-) That said there 
is a significant opportunity for rationalisation and clarity 
improvement, even if it is only a proper realisation of where we 
deliberately duplicate information for ease of user learning.


I also liked the idea of all the documenation being collated into one 
Humongous pdf as a reference (there was a patch a while back - don't 
have a reference right now).


Philip 


--
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 4/4] t4300 (rebase): don't unnecessarily set GIT_TRACE

2013-05-11 Thread Ramkumar Ramachandra
Junio C Hamano wrote:
 But the output from passing -v before the test that breaks is not
 very useful for two reasons.

I sometimes checkout the Good branch in a different worktree, compare
the output/ state of the passing test with the failing one.  I've
never really found the outputs from earlier tests enlightening.  From
my experience, the failure is often due to an earlier test not
imposing tighter passing conditions: but because it's shell, the
debugging time is very small.  I always just patch-locally and run.

I'm not sure how to make the testing framework more useful.
--
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


[RFC/PATCH 0/2] merge-base: add --merge-child option

2013-05-11 Thread John Keeping
This is helpful when examining branches with disjoint roots, for example
because one is periodically merged into a subtree of the other.

With the --merge-child option, git merge-base will print a
first-parent ancestor of the first revision given, where the commit
printed is either a merge-base of the supplied revisions or a merge for
which one of its parents (not the first) is a merge-base.

For example, given the history:

A---C---G
 \
B-D---F
 \
  E

we have:

$ git merge-base F E
B

$ git merge-base --merge-child F E
D

$ git merge-base F G
C

$ git merge-base --merge-child F G
C

$ git log --left-right F...E
 F
 D
 C
 A
 E

$ git log --left-right F...E --not $(git merge-base --merge-child F E)
 F
 E

The git-log case is useful because it allows us to limit the range of
commits that we are examining for patch-identical changes when using
--cherry.  For example with git-gui in git.git I know that anything
before the last merge of git-gui is not interesting:

$ time git log --cherry master...git-gui/master /dev/null
real0m32.731s
user0m31.956s
sys 0m0.664s

$ time git log --cherry master...git-gui/master --not \
$(git merge-base --merge-child master git-gui/master) \
/dev/null
real0m2.296s
user0m2.193s
sys 0m0.092s


The first commit is a small prerequisite to extract a useful function
from builtin/tag.c to commit.c.  The second is the main change (the
commit message is identical to the text before this paragraph).

I'm not convinced that '--merge-child' is the right name for this but I
think the functionality itself is useful.

John Keeping (2):
  commit: add commit_list_contains function
  merge-base: add --merge-child option

 Documentation/git-merge-base.txt |  6 
 builtin/merge-base.c | 61 ++--
 builtin/tag.c| 10 +--
 commit.c |  8 ++
 commit.h |  1 +
 t/t6010-merge-base.sh| 25 ++--
 6 files changed, 98 insertions(+), 13 deletions(-)

-- 
1.8.3.rc1.289.gcb3647f

--
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


[RFC/PATCH 1/2] commit: add commit_list_contains function

2013-05-11 Thread John Keeping
This is the same as the in_commit_list function already in builtin/tag.c
so we also replace that by the new commit_list_contains function.

Signed-off-by: John Keeping j...@keeping.me.uk
---
 builtin/tag.c | 10 +-
 commit.c  |  8 
 commit.h  |  1 +
 3 files changed, 10 insertions(+), 9 deletions(-)

diff --git a/builtin/tag.c b/builtin/tag.c
index af3af3f..1c24772 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -65,14 +65,6 @@ static const unsigned char *match_points_at(const char 
*refname,
return NULL;
 }
 
-static int in_commit_list(const struct commit_list *want, struct commit *c)
-{
-   for (; want; want = want-next)
-   if (!hashcmp(want-item-object.sha1, c-object.sha1))
-   return 1;
-   return 0;
-}
-
 static int contains_recurse(struct commit *candidate,
const struct commit_list *want)
 {
@@ -85,7 +77,7 @@ static int contains_recurse(struct commit *candidate,
if (candidate-object.flags  UNINTERESTING)
return 0;
/* or are we it? */
-   if (in_commit_list(want, candidate))
+   if (commit_list_contains(want, candidate))
return 1;
 
if (parse_commit(candidate)  0)
diff --git a/commit.c b/commit.c
index 888e02a..a8263c3 100644
--- a/commit.c
+++ b/commit.c
@@ -420,6 +420,14 @@ void commit_list_sort_by_date(struct commit_list **list)
commit_list_compare_by_date);
 }
 
+int commit_list_contains(const struct commit_list *list, const struct commit 
*item)
+{
+   for (; list; list = list-next)
+   if (!hashcmp(list-item-object.sha1, item-object.sha1))
+   return 1;
+   return 0;
+}
+
 struct commit *pop_most_recent_commit(struct commit_list **list,
  unsigned int mark)
 {
diff --git a/commit.h b/commit.h
index 67bd509..d686ea8 100644
--- a/commit.h
+++ b/commit.h
@@ -60,6 +60,7 @@ unsigned commit_list_count(const struct commit_list *l);
 struct commit_list *commit_list_insert_by_date(struct commit *item,
struct commit_list **list);
 void commit_list_sort_by_date(struct commit_list **list);
+int commit_list_contains(const struct commit_list *list, const struct commit 
*item);
 
 void free_commit_list(struct commit_list *list);
 
-- 
1.8.3.rc1.289.gcb3647f

--
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


[RFC/PATCH 2/2] merge-base: add --merge-child option

2013-05-11 Thread John Keeping
This is helpful when examining branches with disjoint roots, for example
because one is periodically merged into a subtree of the other.

With the --merge-child option, git merge-base will print a
first-parent ancestor of the first revision given, where the commit
printed is either a merge-base of the supplied revisions or a merge for
which one of its parents (not the first) is a merge-base.

For example, given the history:

A---C---G
 \
B-D---F
 \
  E

we have:

$ git merge-base F E
B

$ git merge-base --merge-child F E
D

$ git merge-base F G
C

$ git merge-base --merge-child F G
C

$ git log --left-right F...E
 F
 D
 C
 A
 E

$ git log --left-right F...E --not $(git merge-base --merge-child F E)
 F
 E

The git-log case is useful because it allows us to limit the range of
commits that we are examining for patch-identical changes when using
--cherry.  For example with git-gui in git.git I know that anything
before the last merge of git-gui is not interesting:

$ time git log --cherry master...git-gui/master /dev/null
real0m32.731s
user0m31.956s
sys 0m0.664s

$ time git log --cherry master...git-gui/master --not \
$(git merge-base --merge-child master git-gui/master) \
/dev/null
real0m2.296s
user0m2.193s
sys 0m0.092s

Signed-off-by: John Keeping j...@keeping.me.uk
---
 Documentation/git-merge-base.txt |  6 
 builtin/merge-base.c | 61 ++--
 t/t6010-merge-base.sh| 25 ++--
 3 files changed, 88 insertions(+), 4 deletions(-)

diff --git a/Documentation/git-merge-base.txt b/Documentation/git-merge-base.txt
index 87842e3..a1404e1 100644
--- a/Documentation/git-merge-base.txt
+++ b/Documentation/git-merge-base.txt
@@ -10,6 +10,7 @@ SYNOPSIS
 
 [verse]
 'git merge-base' [-a|--all] commit commit...
+'git merge-base' [-a|--all] --merge-child commit...
 'git merge-base' [-a|--all] --octopus commit...
 'git merge-base' --is-ancestor commit commit
 'git merge-base' --independent commit...
@@ -39,6 +40,11 @@ As a consequence, the 'merge base' is not necessarily 
contained in each of the
 commit arguments if more than two commits are specified. This is different
 from linkgit:git-show-branch[1] when used with the `--merge-base` option.
 
+--merge-child::
+   Find the first-parent ancestor of the first commit that is either
+   reachable from all of the supplied commits or which has a parent that
+   is.
+
 --octopus::
Compute the best common ancestors of all supplied commits,
in preparation for an n-way merge.  This mimics the behavior
diff --git a/builtin/merge-base.c b/builtin/merge-base.c
index 1bc7991..0bf9f6d 100644
--- a/builtin/merge-base.c
+++ b/builtin/merge-base.c
@@ -1,7 +1,9 @@
 #include builtin.h
 #include cache.h
 #include commit.h
+#include diff.h
 #include parse-options.h
+#include revision.h
 
 static int show_merge_base(struct commit **rev, int rev_nr, int show_all)
 {
@@ -24,12 +26,61 @@ static int show_merge_base(struct commit **rev, int rev_nr, 
int show_all)
 
 static const char * const merge_base_usage[] = {
N_(git merge-base [-a|--all] commit commit...),
+   N_(git merge-base [-a|--all] --merge-child commit...),
N_(git merge-base [-a|--all] --octopus commit...),
N_(git merge-base --independent commit...),
N_(git merge-base --is-ancestor commit commit),
NULL
 };
 
+static int handle_merge_child(struct commit **rev, int rev_nr, const char 
*prefix, int show_all)
+{
+   struct commit_list *merge_bases;
+   struct rev_info revs;
+   struct commit *commit;
+
+   merge_bases = get_merge_bases_many(rev[0], rev_nr - 1, rev + 1, 0);
+
+   if (!merge_bases)
+   return 1;
+
+   init_revisions(revs, prefix);
+   revs.first_parent_only = 1;
+
+   clear_commit_marks(rev[0], SEEN | UNINTERESTING | SHOWN | ADDED);
+   add_pending_object(revs, rev[0]-object, rev0);
+   if (prepare_revision_walk(revs))
+   die(_(revision walk setup failed));
+
+   while ((commit = get_revision(revs)) != NULL) {
+   /*
+* If a merge base is a first-parent ancestor of rev[0] then
+* we print the commit itself instead of a merge which is its
+* child.
+*/
+   if (commit_list_contains(merge_bases, commit)) {
+   printf(%s\n, sha1_to_hex(commit-object.sha1));
+   if (!show_all)
+   return 0;
+   }
+
+   struct commit_list *parent;
+   for (parent = commit-parents; parent; parent = parent-next) {
+   /* Skip the 

Cannot push anything via export transport helper after push fails.

2013-05-11 Thread Andrey Borzenkov
I noticed that using git-remote-bzr, but as far as I can tell this is
generic for all transport helpers using fast-export.



What happened was git push failed due to merge conflict. So far so
good - but from now on git assumes everything is up to date.

bor@opensuse:/tmp/test/git git push origin master
To bzr::bzr+ssh://bor@localhost/tmp/test/bzr
 ! [rejected]master - master (non-fast-forward)
error: failed to push some refs to 'bzr::bzr+ssh://bor@localhost/tmp/test/bzr'
hint: Updates were rejected because the tip of your current branch is behind
hint: its remote counterpart. Merge the remote changes (e.g. 'git pull')
hint: before pushing again.
hint: See the 'Note about fast-forwards' in 'git push --help' for details.
bor@opensuse:/tmp/test/git git push origin master
Everything up-to-date
bor@opensuse:/tmp/test/git 

The problem seems to be that git fast-export updates marks
unconditionally, whether export actually applied or not. So next time
it assumes everything is already exported and does nothing.

Is it expected behavior?

TIA

-andrey
--
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: Cannot push anything via export transport helper after push fails.

2013-05-11 Thread John Keeping
On Sat, May 11, 2013 at 04:29:36PM +0400, Andrey Borzenkov wrote:
 I noticed that using git-remote-bzr, but as far as I can tell this is
 generic for all transport helpers using fast-export.
 
 
 
 What happened was git push failed due to merge conflict. So far so
 good - but from now on git assumes everything is up to date.
 
 bor@opensuse:/tmp/test/git git push origin master
 To bzr::bzr+ssh://bor@localhost/tmp/test/bzr
  ! [rejected]master - master (non-fast-forward)
 error: failed to push some refs to 'bzr::bzr+ssh://bor@localhost/tmp/test/bzr'
 hint: Updates were rejected because the tip of your current branch is behind
 hint: its remote counterpart. Merge the remote changes (e.g. 'git pull')
 hint: before pushing again.
 hint: See the 'Note about fast-forwards' in 'git push --help' for details.
 bor@opensuse:/tmp/test/git git push origin master
 Everything up-to-date
 bor@opensuse:/tmp/test/git 
 
 The problem seems to be that git fast-export updates marks
 unconditionally, whether export actually applied or not. So next time
 it assumes everything is already exported and does nothing.
 
 Is it expected behavior?

What version of Git are you using?

This sounds similar to the regression fixed by commit 126aac5
(transport-helper: fix remote helper namespace regression, 2013-05-10)
but that was only introduced in commit 664059f (transport-helper: update
remote helper namespace, 2013-04-17) which isn't in any released
versions of Git.
--
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 v4 0/5] allow more sources for config values

2013-05-11 Thread Heiko Voigt
Hi,

all the comments to the last iteration[1] incorporated.

You can also find this on my github[2]. This is only for review. I will
resubmit this once 1.8.3 is out.

Cheers Heiko

[1] http://article.gmane.org/gmane.comp.version-control.git/223743
[2] https://github.com/hvoigt/git/commits/hv/strbuf_config_parsing-series4

Heiko Voigt (5):
  config: factor out config file stack management
  config: drop cf validity check in get_next_char()
  config: make parsing stack struct independent from actual data source
  teach config --blob option to parse config from database
  do not die when error in config parsing of buf occurs

 builtin/config.c   |  31 ++-
 cache.h|   6 +-
 config.c   | 217 ++---
 t/t1307-config-blob.sh |  70 
 4 files changed, 271 insertions(+), 53 deletions(-)
 create mode 100755 t/t1307-config-blob.sh

-- 
1.8.3.rc1.53.g0126843

--
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 v4 1/5] config: factor out config file stack management

2013-05-11 Thread Heiko Voigt
Because a config callback may start parsing a new file, the
global context regarding the current config file is stored
as a stack. Currently we only need to manage that stack from
git_config_from_file. Let's factor it out to allow new
sources of config data.

Signed-off-by: Heiko Voigt hvo...@hvoigt.net
---
 config.c | 42 --
 1 file changed, 28 insertions(+), 14 deletions(-)

diff --git a/config.c b/config.c
index aefd80b..f0494f3 100644
--- a/config.c
+++ b/config.c
@@ -896,6 +896,32 @@ int git_default_config(const char *var, const char *value, 
void *dummy)
return 0;
 }
 
+/*
+ * The fields f and name of top need to be initialized before calling
+ * this function.
+ */
+static int do_config_from(struct config_file *top, config_fn_t fn, void *data)
+{
+   int ret;
+
+   /* push config-file parsing state stack */
+   top-prev = cf;
+   top-linenr = 1;
+   top-eof = 0;
+   strbuf_init(top-value, 1024);
+   strbuf_init(top-var, 1024);
+   cf = top;
+
+   ret = git_parse_file(fn, data);
+
+   /* pop config-file parsing state stack */
+   strbuf_release(top-value);
+   strbuf_release(top-var);
+   cf = top-prev;
+
+   return ret;
+}
+
 int git_config_from_file(config_fn_t fn, const char *filename, void *data)
 {
int ret;
@@ -905,22 +931,10 @@ int git_config_from_file(config_fn_t fn, const char 
*filename, void *data)
if (f) {
config_file top;
 
-   /* push config-file parsing state stack */
-   top.prev = cf;
top.f = f;
top.name = filename;
-   top.linenr = 1;
-   top.eof = 0;
-   strbuf_init(top.value, 1024);
-   strbuf_init(top.var, 1024);
-   cf = top;
-
-   ret = git_parse_file(fn, data);
-
-   /* pop config-file parsing state stack */
-   strbuf_release(top.value);
-   strbuf_release(top.var);
-   cf = top.prev;
+
+   ret = do_config_from(top, fn, data);
 
fclose(f);
}
-- 
1.8.3.rc1.53.g0126843

--
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 v4 2/5] config: drop cf validity check in get_next_char()

2013-05-11 Thread Heiko Voigt
The global variable cf is set with an initialized value in all codepaths before
calling this function.

The complete call graph looks like this:

  git_config_from_file
- do_config_from
  - git_parse_file
- get_next_char
- get_value
- get_next_char
- parse_value
- get_next_char
- get_base_var
- get_next_char
- get_extended_base_var
- get_next_char

The variable is initialized in do_config_from.

Signed-off-by: Heiko Voigt hvo...@hvoigt.net
---
 config.c | 29 +
 1 file changed, 13 insertions(+), 16 deletions(-)

diff --git a/config.c b/config.c
index f0494f3..046642b 100644
--- a/config.c
+++ b/config.c
@@ -169,26 +169,23 @@ int git_config_from_parameters(config_fn_t fn, void *data)
 static int get_next_char(void)
 {
int c;
-   FILE *f;
+   FILE *f = cf-f;
 
-   c = '\n';
-   if (cf  ((f = cf-f) != NULL)) {
+   c = fgetc(f);
+   if (c == '\r') {
+   /* DOS like systems */
c = fgetc(f);
-   if (c == '\r') {
-   /* DOS like systems */
-   c = fgetc(f);
-   if (c != '\n') {
-   ungetc(c, f);
-   c = '\r';
-   }
-   }
-   if (c == '\n')
-   cf-linenr++;
-   if (c == EOF) {
-   cf-eof = 1;
-   c = '\n';
+   if (c != '\n') {
+   ungetc(c, f);
+   c = '\r';
}
}
+   if (c == '\n')
+   cf-linenr++;
+   if (c == EOF) {
+   cf-eof = 1;
+   c = '\n';
+   }
return c;
 }
 
-- 
1.8.3.rc1.53.g0126843

--
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 v4 3/5] config: make parsing stack struct independent from actual data source

2013-05-11 Thread Heiko Voigt
To simplify adding other sources we extract all functions needed for
parsing into a list of callbacks. We implement those callbacks for the
current file parsing. A new source can implement its own set of callbacks.

Instead of storing the concrete FILE pointer for parsing we store a void
pointer. A new source can use this to store its custom data.

Signed-off-by: Heiko Voigt hvo...@hvoigt.net
---
 config.c | 66 ++--
 1 file changed, 44 insertions(+), 22 deletions(-)

diff --git a/config.c b/config.c
index 046642b..e21c24c 100644
--- a/config.c
+++ b/config.c
@@ -10,20 +10,41 @@
 #include strbuf.h
 #include quote.h
 
-typedef struct config_file {
-   struct config_file *prev;
-   FILE *f;
+struct config_source {
+   struct config_source *prev;
+   union {
+   FILE *file;
+   } u;
const char *name;
int linenr;
int eof;
struct strbuf value;
struct strbuf var;
-} config_file;
 
-static config_file *cf;
+   int (*fgetc)(struct config_source *c);
+   int (*ungetc)(int c, struct config_source *conf);
+   long (*ftell)(struct config_source *c);
+};
+
+static struct config_source *cf;
 
 static int zlib_compression_seen;
 
+static int config_file_fgetc(struct config_source *conf)
+{
+   return fgetc(conf-u.file);
+}
+
+static int config_file_ungetc(int c, struct config_source *conf)
+{
+   return ungetc(c, conf-u.file);
+}
+
+static long config_file_ftell(struct config_source *conf)
+{
+   return ftell(conf-u.file);
+}
+
 #define MAX_INCLUDE_DEPTH 10
 static const char include_depth_advice[] =
 exceeded maximum include depth (%d) while including\n
@@ -168,15 +189,13 @@ int git_config_from_parameters(config_fn_t fn, void *data)
 
 static int get_next_char(void)
 {
-   int c;
-   FILE *f = cf-f;
+   int c = cf-fgetc(cf);
 
-   c = fgetc(f);
if (c == '\r') {
/* DOS like systems */
-   c = fgetc(f);
+   c = cf-fgetc(cf);
if (c != '\n') {
-   ungetc(c, f);
+   cf-ungetc(c, cf);
c = '\r';
}
}
@@ -336,7 +355,7 @@ static int get_base_var(struct strbuf *name)
}
 }
 
-static int git_parse_file(config_fn_t fn, void *data)
+static int git_parse_source(config_fn_t fn, void *data)
 {
int comment = 0;
int baselen = 0;
@@ -894,10 +913,11 @@ int git_default_config(const char *var, const char 
*value, void *dummy)
 }
 
 /*
- * The fields f and name of top need to be initialized before calling
+ * All source specific fields in the union, name and the callbacks
+ * fgetc, ungetc, ftell of top need to be initialized before calling
  * this function.
  */
-static int do_config_from(struct config_file *top, config_fn_t fn, void *data)
+static int do_config_from_source(struct config_source *top, config_fn_t fn, 
void *data)
 {
int ret;
 
@@ -909,7 +929,7 @@ static int do_config_from(struct config_file *top, 
config_fn_t fn, void *data)
strbuf_init(top-var, 1024);
cf = top;
 
-   ret = git_parse_file(fn, data);
+   ret = git_parse_source(fn, data);
 
/* pop config-file parsing state stack */
strbuf_release(top-value);
@@ -926,12 +946,15 @@ int git_config_from_file(config_fn_t fn, const char 
*filename, void *data)
 
ret = -1;
if (f) {
-   config_file top;
+   struct config_source top;
 
-   top.f = f;
+   top.u.file = f;
top.name = filename;
+   top.fgetc = config_file_fgetc;
+   top.ungetc = config_file_ungetc;
+   top.ftell = config_file_ftell;
 
-   ret = do_config_from(top, fn, data);
+   ret = do_config_from_source(top, fn, data);
 
fclose(f);
}
@@ -1064,7 +1087,6 @@ static int store_aux(const char *key, const char *value, 
void *cb)
 {
const char *ep;
size_t section_len;
-   FILE *f = cf-f;
 
switch (store.state) {
case KEY_SEEN:
@@ -1076,7 +1098,7 @@ static int store_aux(const char *key, const char *value, 
void *cb)
return 1;
}
 
-   store.offset[store.seen] = ftell(f);
+   store.offset[store.seen] = cf-ftell(cf);
store.seen++;
}
break;
@@ -1103,19 +1125,19 @@ static int store_aux(const char *key, const char 
*value, void *cb)
 * Do not increment matches: this is no match, but we
 * just made sure we are in the desired section.
 */
-   store.offset[store.seen] = ftell(f);
+   store.offset[store.seen] = cf-ftell(cf);
/* fallthru */
case SECTION_END_SEEN:
case START:
if 

[PATCH v4 4/5] teach config --blob option to parse config from database

2013-05-11 Thread Heiko Voigt
This can be used to read configuration values directly from git's
database. For example it is useful for reading to be checked out
.gitmodules files directly from the database.

Signed-off-by: Heiko Voigt hvo...@hvoigt.net
---
 builtin/config.c   | 31 +++---
 cache.h|  6 +++-
 config.c   | 86 --
 t/t1307-config-blob.sh | 70 
 4 files changed, 186 insertions(+), 7 deletions(-)
 create mode 100755 t/t1307-config-blob.sh

diff --git a/builtin/config.c b/builtin/config.c
index 33c9bf9..8d01b7a 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -21,6 +21,7 @@ static char term = '\n';
 
 static int use_global_config, use_system_config, use_local_config;
 static const char *given_config_file;
+static const char *given_config_blob;
 static int actions, types;
 static const char *get_color_slot, *get_colorbool_slot;
 static int end_null;
@@ -53,6 +54,7 @@ static struct option builtin_config_options[] = {
OPT_BOOLEAN(0, system, use_system_config, N_(use system config 
file)),
OPT_BOOLEAN(0, local, use_local_config, N_(use repository config 
file)),
OPT_STRING('f', file, given_config_file, N_(file), N_(use given 
config file)),
+   OPT_STRING(0, blob, given_config_blob, N_(blob-id), N_(read 
config from given blob object)),
OPT_GROUP(N_(Action)),
OPT_BIT(0, get, actions, N_(get value: name [value-regex]), 
ACTION_GET),
OPT_BIT(0, get-all, actions, N_(get all values: key 
[value-regex]), ACTION_GET_ALL),
@@ -218,7 +220,8 @@ static int get_value(const char *key_, const char *regex_)
}
 
git_config_with_options(collect_config, values,
-   given_config_file, respect_includes);
+   given_config_file, given_config_blob,
+   respect_includes);
 
ret = !values.nr;
 
@@ -302,7 +305,8 @@ static void get_color(const char *def_color)
get_color_found = 0;
parsed_color[0] = '\0';
git_config_with_options(git_get_color_config, NULL,
-   given_config_file, respect_includes);
+   given_config_file, given_config_blob,
+   respect_includes);
 
if (!get_color_found  def_color)
color_parse(def_color, command line, parsed_color);
@@ -330,7 +334,8 @@ static int get_colorbool(int print)
get_colorbool_found = -1;
get_diff_color_found = -1;
git_config_with_options(git_get_colorbool_config, NULL,
-   given_config_file, respect_includes);
+   given_config_file, given_config_blob,
+   respect_includes);
 
if (get_colorbool_found  0) {
if (!strcmp(get_colorbool_slot, color.diff))
@@ -348,6 +353,12 @@ static int get_colorbool(int print)
return get_colorbool_found ? 0 : 1;
 }
 
+static void check_blob_write(void)
+{
+   if (given_config_blob)
+   die(writing config blobs is not supported);
+}
+
 int cmd_config(int argc, const char **argv, const char *prefix)
 {
int nongit = !startup_info-have_repository;
@@ -359,7 +370,8 @@ int cmd_config(int argc, const char **argv, const char 
*prefix)
 builtin_config_usage,
 PARSE_OPT_STOP_AT_NON_OPTION);
 
-   if (use_global_config + use_system_config + use_local_config + 
!!given_config_file  1) {
+   if (use_global_config + use_system_config + use_local_config +
+   !!given_config_file + !!given_config_blob  1) {
error(only one config file at a time.);
usage_with_options(builtin_config_usage, 
builtin_config_options);
}
@@ -438,6 +450,7 @@ int cmd_config(int argc, const char **argv, const char 
*prefix)
check_argc(argc, 0, 0);
if (git_config_with_options(show_all_config, NULL,
given_config_file,
+   given_config_blob,
respect_includes)  0) {
if (given_config_file)
die_errno(unable to read config file '%s',
@@ -450,6 +463,8 @@ int cmd_config(int argc, const char **argv, const char 
*prefix)
check_argc(argc, 0, 0);
if (!given_config_file  nongit)
die(not in a git directory);
+   if (given_config_blob)
+   die(editing blobs is not supported);
git_config(git_default_config, NULL);
launch_editor(given_config_file ?
  given_config_file : git_path(config),
@@ -457,6 +472,7 @@ int cmd_config(int argc, const char **argv, const char 
*prefix)
}

[PATCH v4 5/5] do not die when error in config parsing of buf occurs

2013-05-11 Thread Heiko Voigt
If a config parsing error in a file occurs we can die and let the user
fix the issue. This is different for the buf parsing function since it
can be used to parse blobs of .gitmodules files. If a parsing error
occurs here we should proceed since otherwise a database containing such
an error in a single revision could be rendered unusable.

Signed-off-by: Heiko Voigt hvo...@hvoigt.net
---
 config.c | 10 --
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/config.c b/config.c
index 23b14f4..a8d3dcf 100644
--- a/config.c
+++ b/config.c
@@ -21,6 +21,7 @@ struct config_source {
} buf;
} u;
const char *name;
+   int die_on_error;
int linenr;
int eof;
struct strbuf value;
@@ -442,7 +443,10 @@ static int git_parse_source(config_fn_t fn, void *data)
if (get_value(fn, data, var)  0)
break;
}
-   die(bad config file line %d in %s, cf-linenr, cf-name);
+   if (cf-die_on_error)
+   die(bad config file line %d in %s, cf-linenr, cf-name);
+   else
+   return error(bad config file line %d in %s, cf-linenr, 
cf-name);
 }
 
 static int parse_unit_factor(const char *end, uintmax_t *val)
@@ -940,7 +944,7 @@ int git_default_config(const char *var, const char *value, 
void *dummy)
 }
 
 /*
- * All source specific fields in the union, name and the callbacks
+ * All source specific fields in the union, die_on_error, name and the 
callbacks
  * fgetc, ungetc, ftell of top need to be initialized before calling
  * this function.
  */
@@ -977,6 +981,7 @@ int git_config_from_file(config_fn_t fn, const char 
*filename, void *data)
 
top.u.file = f;
top.name = filename;
+   top.die_on_error = 1;
top.fgetc = config_file_fgetc;
top.ungetc = config_file_ungetc;
top.ftell = config_file_ftell;
@@ -997,6 +1002,7 @@ int git_config_from_buf(config_fn_t fn, const char *name, 
const char *buf,
top.u.buf.len = len;
top.u.buf.pos = 0;
top.name = name;
+   top.die_on_error = 0;
top.fgetc = config_buf_fgetc;
top.ungetc = config_buf_ungetc;
top.ftell = config_buf_ftell;
-- 
1.8.3.rc1.53.g0126843

--
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] test-bzr: Do not use unportable sed \+

2013-05-11 Thread Torsten Bögershausen
Using sed -e '/[0-9]\+//' to find one ore more digits
is not portable.
Use the Basic Regular Expression '/[0-9][0-9]*//' instead

Signed-off-by: Torsten Bögershausen tbo...@web.de
---
 contrib/remote-helpers/test-bzr.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/contrib/remote-helpers/test-bzr.sh 
b/contrib/remote-helpers/test-bzr.sh
index d9c32f4..5dfa070 100755
--- a/contrib/remote-helpers/test-bzr.sh
+++ b/contrib/remote-helpers/test-bzr.sh
@@ -328,7 +328,7 @@ test_expect_success 'strip' '
 
   echo four  content 
   bzr commit -m four 
-  bzr log --line | sed -e s/^[0-9]\+: //  ../expected
+  bzr log --line | sed -e s/^[0-9][0-9]*: //  ../expected
   ) 
 
   (cd gitrepo 
-- 
1.8.2.1.614.g66d7af5

--
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: Cannot push anything via export transport helper after push fails.

2013-05-11 Thread Felipe Contreras
On Sat, May 11, 2013 at 7:29 AM, Andrey Borzenkov arvidj...@gmail.com wrote:
 I noticed that using git-remote-bzr, but as far as I can tell this is
 generic for all transport helpers using fast-export.



 What happened was git push failed due to merge conflict. So far so
 good - but from now on git assumes everything is up to date.

 bor@opensuse:/tmp/test/git git push origin master
 To bzr::bzr+ssh://bor@localhost/tmp/test/bzr
  ! [rejected]master - master (non-fast-forward)
 error: failed to push some refs to 'bzr::bzr+ssh://bor@localhost/tmp/test/bzr'
 hint: Updates were rejected because the tip of your current branch is behind
 hint: its remote counterpart. Merge the remote changes (e.g. 'git pull')
 hint: before pushing again.
 hint: See the 'Note about fast-forwards' in 'git push --help' for details.
 bor@opensuse:/tmp/test/git git push origin master
 Everything up-to-date
 bor@opensuse:/tmp/test/git

 The problem seems to be that git fast-export updates marks
 unconditionally, whether export actually applied or not. So next time
 it assumes everything is already exported and does nothing.

 Is it expected behavior?

Indeed, this is the way it currently works, and it's not easy to fix.
We would need some way to make fast-export wait until we know the exit
status of the remote helper, and then tell it when it failed, so the
marks are not updated.

However, the way remote-bzr/hg work is that the commits are still
there anyway. So if you merge the next time you push those commits are
already converted, so it's not a problem if fast-export is not
exporting them again.

So even though it's not ideal, it should work.

The problem is when the remote-helper crashes and the marks of
fast-export and the remote-helper are out of sync, and then the user
is really screwed.

Cheers.

-- 
Felipe Contreras
--
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


Aw: Re: Outdated and broken online versions of user-manual.html

2013-05-11 Thread Thomas Ackermann
 
  (1) Very poor html formatting (document type book causes
  ugly TOCs per section and there's a Part I without a Part II)
  (2) Partly outdated content
  (3) Sub-optimal structuring (to-do list as part of the document,
  glossary not at the end of the document)
  (4) User-manual.PDF uses an independent tool chain which makes it
  harder to do improvements for user-manual.html and also is the only
  pdf doc we are creating. IMHO we should remove this altogether.
  (5) Large overlapping with the tutorials. IMHO all of the
  tutorials should be blended into user-manual

 I'd welcome the provision of a 'man'/'html' formatted version of the two 
 guides. Are you expecting to make then compatible with the man format?

This is another topic on my to-do list ...
 
 I would be a little cautious of your point 5 if it squoze everything 
 into one overlong document at the expense of losing the shorter 
 documents - one can't eat a whole melon in one bite ;-) That said there 
 is a significant opportunity for rationalisation and clarity 
 improvement, even if it is only a proper realisation of where we 
 deliberately duplicate information for ease of user learning.
 
I did not have a closer look on this but just from reading the documents
I got the impression that the tutorials are already more or less contained
in the user-manual and by a little restructuring the melon will fall into handy 
pieces ;-)

 I also liked the idea of all the documenation being collated into one 
 Humongous pdf as a reference (there was a patch a while back - don't 
 have a reference right now).

Thanks :-) My patches were rejected but (4) shows that now I know why ;-)
Nevertheless I maintain this in https://github.com/tacker66/git.git and provide
the documents for every major version of Git in 
https://github.com/tacker66/git-docpdf.git.
(4) is also the reason why I stopped using the pdf target and instead create 
user-manual.pdf
with wkhtmltopdf from user-manual.html.
Additionally I use Amazons kindlegen to create mobi version from the html 
documents
which look quite good on a Kindle.


---
Thomas
--
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


[ANN] git-tbdiff: topic branch diff

2013-05-11 Thread Thomas Rast
Hi,

Spawned by discussion here at git-merge, I created a script that diffs
the state of branches.

You can grab it from

  https://github.com/trast/tbdiff.git

The usage is

  git tbdiff A..B C..D

Make sure to pass two ranges; it doesn't check that at all, and just
hands it off to format-patch, so that a lone commit 'foo' means
'foo..HEAD' in the usual format-patch semantics.

It does not use a longest-common subsequence approach like all other
diffs, since the commits on the branch could have been reordered.
Unlike in code, where only special cases of reorderings do not change
the meaning[1], these reorderings frequently do not change the semantics
of the series.

I tried an approach that goes looking for a minimum cost bipartite
matching, where the cost is given by the interdiff between the commits.
This seems to be working nicely; for example, between v2 and v3 of the
fc/remote-hg topic[2] it shows (in glorious color that cannot be
represented here):

  $ git tbdiff origin/master~2..fc/remote-hg-2  origin/master..fc/remote-hg-3
   1:  95bb1c2 =  1:  832f6a6 remote-hg: trivial test cleanups
   2:  ebff16c =  2:  1512332 remote-hg: make sure fake bookmarks are updated
  --:    3:  1584d8b remote-hg: fix bad state issue
   3:  8db4546 =  4:  0d6054b remote-hg: redirect buggy mercurial output
  --:    5:  ff9def9 remote-hg: fix bad file paths
   5:  2406703 =  6:  f467a2d remote-hg: refactor export
   6:  e3018d1 =  7:  70e4855 remote-hg: fix for files with spaces
   7:  a35adfb =  8:  7af557e remote-hg: update remote bookmarks
  --:    9:  99fe20c remote-hg: document location of stored hg 
repository
   8:  4ec2382 = 10:  d360d7c remote-hg: add missing config variable in doc
   9:  1c94a96 = 11:  0c51226 remote-hg: trivial cleanups
   4:  fd646af = 12:  e9f3e36 remote-hg: update tags globally
  10:  dc097ea = 13:  1ed339d remote-hg: properly report errors on bookmark 
pushes
  12:  d8a548c = 14:  f887082 remote-hg: split bookmark handling
  --:   15:  a9ac462 remote-hg: add basic author tests
  --:   16:  c68bed9 remote-hg: add simple mail test
  --:   17:  a350abe remote-hg: add 'insecure' option
  11:  0870eb6 ! 18:  66e283d remote-hg: push to the appropriate branch
  @@ -11,7 +11,7 @@

   +# Check if the ref is supposed to be a named branch
   +if ref.startswith('refs/heads/branches/'):
  -+extra['branch'] = ref.rpartition('/')[2]
  ++extra['branch'] = ref.replace('refs/heads/branches/', '')
   +
if mode == 'hg':
i = data.find('\n--HG--\n')

  13:  a583fa6  --:  remote-hg: force remote push
  --:   19:  ff4571d remote-hg: show more proper errors
  --:   20:  34b0b77 remote-hg: force remote push

So you can clearly see that commit old-4 moved to position 12, commit 3
is new, etc.  The failure to match up old-13 with new-20 is not a bug;
the two are so different that the heuristics split it into a
creation/deletion.

Another use-case is when Junio picked up a series that you submitted.
Comparing Peff's jk/packed-refs-race on github with what was actually
merged into git.git shows

  $ git tbdiff origin/next..peff/jk/packed-refs-race origin/next..7c95033
  1:  eb4f175 ! 1:  ed485e2 resolve_ref: close race condition for packed refs
  @@ -25,6 +25,9 @@
   resolve_ref_unsafe. In the situation described above, we may
   still be depending on a cached view of the packed-refs file;
   that race will be dealt with in a future patch.
  +
  +Signed-off-by: Jeff King p...@peff.net
  +Signed-off-by: Junio C Hamano gits...@pobox.com
   diff --git a/refs.c b/refs.c
   --- a/refs.c
   +++ b/refs.c

  2:  d9d9e0f ! 2:  40aba32 add a stat_validity struct
  @@ -13,6 +13,9 @@
   reuse the logic about which stat entries to trust for a
   particular platform, but hides the complexity behind two
   simple functions: check and update.
  +
  +Signed-off-by: Jeff King p...@peff.net
  +Signed-off-by: Junio C Hamano gits...@pobox.com
   diff --git a/cache.h b/cache.h
   --- a/cache.h
   +++ b/cache.h

  3:  4e566d4 ! 3:  7c95033 for_each_ref: load all loose refs before packed refs
  @@ -40,6 +40,17 @@
   from that tip (the same thing can happen if repack -ad is
   used, as it simply drops unreachable objects that are
   packed).
  +
  +This patch solves it by loading all of the loose refs for
  +our traversal into our in-memory cache, and then refreshing
  +the packed-refs cache. Because a pack-refs writer will
  +always put the new packed-refs file into place before
  +starting the prune, we know that any loose refs we fail to
  +see will either truly be missing, or will have already been
  +put in the packed-refs file by the time we refresh.
  +
  +Signed-off-by: Jeff King p...@peff.net
  +Signed-off-by: Junio C Hamano gits...@pobox.com

Re: [ANN] git-tbdiff: topic branch diff

2013-05-11 Thread Thomas Rast
Thomas Rast tr...@inf.ethz.ch writes:

 Hi,

 Spawned by discussion here at git-merge, I created a script that diffs
 the state of branches.

 You can grab it from

   https://github.com/trast/tbdiff.git

 The usage is

   git tbdiff A..B C..D

BTW, I should've mentioned this here too (it's in the README shown on
github): it currently depends on the 'hungarian' module for the matching
algorithm, found here:

  https://pypi.python.org/pypi/hungarian

It's easy to install, but in the interests of wider usability I'll
probably write the same in pure python eventually.

-- 
Thomas Rast
trast@{inf,student}.ethz.ch
--
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


[RFC/PATCH 0/4] update tracking refs on explicit fetch

2013-05-11 Thread Jeff King
This is a cleaned-up version of the oft-discussed[1] concept that git
pull origin master should update refs/remotes/origin/master.

It is a little bit of a risky change, in that anybody who deeply cares
about when their tracking ref branches are updated would be impacted.
But I think the general consensus is that while you can come up with a
hypothetical use case, the logic is quite tortured, and you could
accomplish the same thing by keeping a local branch. Most people seem to
have the mental model that remote tracking branches are basically a
cache for what's on the remote, updated opportunistically as
appropriate.

  [1/4]: t5510: start tracking-ref tests from a known state
  [2/4]: fetch/pull doc: untangle meaning of bare ref
  [3/4]: refactor ref-merge flag
  [4/4]: fetch: opportunistically update tracking refs

The final patch is the moral equivalent of the patch I've posted before
when this topic comes up (and referenced in the threads below). But that
patch did not take care not to write duplicate (mergable!) entries into
FETCH_HEAD, leading to great confusion from git-pull.  This series deals
with that, and takes care to write the exact same FETCH_HEAD we would
have written before the series.

However, Jonathan raised an interesting question: what is the point of
not-for-merge entries at all? We are not going to merge them, so none of
the pull/merge machinery cares about them. We do not consider extra
entries in FETCH_HEAD for reachability or anything like that. Is there
anybody who reads them?

If we do not care about them, we can simplify this much further, and
either write the duplicates as not-for-merge, or even omit not-for-merge
entries entirely. However, it is possible that some third-party scripts
process FETCH_HEAD, so I took the conservative route.

Note that this patch is as greedy as possible[2]; whenever we see the
LHS is fetched, we update the RHS side, even if the user did:

  git fetch origin master:foobar

Some of the discussions below argue that the behavior should not kick in
for such a case. I am not sure I agree (and argue against it in those
discussions). I think at this point that is the only potentially
contentious item.

-Peff

[1] Past discussions:

http://thread.gmane.org/gmane.comp.version-control.git/127163/focus=127215

http://thread.gmane.org/gmane.comp.version-control.git/192252

http://thread.gmane.org/gmane.comp.version-control.git/203357/focus=203442

http://article.gmane.org/gmane.comp.version-control.git/221234
--
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/4] t5510: start tracking-ref tests from a known state

2013-05-11 Thread Jeff King
We have three sequential tests for for whether tracking refs
are updated by various fetches and pulls; the first two
should not update the ref, and the third should. Each test
depends on the state left by the test before.

This is fragile (a failing early test will confuse later
tests), and means we cannot add more should update tests
after the third one.

Let's instead save the initial state before these tests, and
then reset to a known state before running each test.

Signed-off-by: Jeff King p...@peff.net
---
 t/t5510-fetch.sh | 10 ++
 1 file changed, 10 insertions(+)

diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh
index d7a19a1..789c228 100755
--- a/t/t5510-fetch.sh
+++ b/t/t5510-fetch.sh
@@ -370,12 +370,20 @@ test_expect_success 'explicit fetch should not update 
tracking' '
 
 '
 
+test_expect_success 'mark initial state of origin/master' '
+   (
+   cd three 
+   git tag base-origin-master refs/remotes/origin/master
+   )
+'
+
 test_expect_success 'explicit fetch should not update tracking' '
 
cd $D 
git branch -f side 
(
cd three 
+   git update-ref refs/remotes/origin/master base-origin-master 
o=$(git rev-parse --verify refs/remotes/origin/master) 
git fetch origin master 
n=$(git rev-parse --verify refs/remotes/origin/master) 
@@ -390,6 +398,7 @@ test_expect_success 'explicit pull should not update 
tracking' '
git branch -f side 
(
cd three 
+   git update-ref refs/remotes/origin/master base-origin-master 
o=$(git rev-parse --verify refs/remotes/origin/master) 
git pull origin master 
n=$(git rev-parse --verify refs/remotes/origin/master) 
@@ -404,6 +413,7 @@ test_expect_success 'configured fetch updates tracking' '
git branch -f side 
(
cd three 
+   git update-ref refs/remotes/origin/master base-origin-master 
o=$(git rev-parse --verify refs/remotes/origin/master) 
git fetch origin 
n=$(git rev-parse --verify refs/remotes/origin/master) 
-- 
1.8.3.rc1.2.g12db477

--
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/4] fetch/pull doc: untangle meaning of bare ref

2013-05-11 Thread Jeff King
From: Thomas Rast tr...@inf.ethz.ch

The documentation erroneously used the same wording for both fetch and
pull, stating that something will be merged even in git-fetch(1).

In addition, saying that ref is equivalent to ref: doesn't
really help anyone who still needs to read manpages.  Clarify what is
actually going on.

Signed-off-by: Thomas Rast tr...@inf.ethz.ch
Signed-off-by: Jeff King p...@peff.net
---
 Documentation/pull-fetch-param.txt | 11 ---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/Documentation/pull-fetch-param.txt 
b/Documentation/pull-fetch-param.txt
index 94a9d32..6f5ca21 100644
--- a/Documentation/pull-fetch-param.txt
+++ b/Documentation/pull-fetch-param.txt
@@ -68,6 +68,11 @@ Some short-cut notations are also supported.
 +
 * `tag tag` means the same as `refs/tags/tag:refs/tags/tag`;
   it requests fetching everything up to the given tag.
-* A parameter ref without a colon is equivalent to
-  ref: when pulling/fetching, so it merges ref into the current
-  branch without storing the remote branch anywhere locally
+ifndef::git-pull[]
+* A parameter ref without a colon fetches that ref into FETCH_HEAD,
+endif::git-pull[]
+ifdef::git-pull[]
+* A parameter ref without a colon merges ref into the current
+  branch,
+endif::git-pull[]
+  while not storing the branch anywhere locally.
-- 
1.8.3.rc1.2.g12db477

--
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 3/4] refactor ref-merge flag

2013-05-11 Thread Jeff King
Each struct ref has a boolean flag that is set by the
fetch code to determine whether the ref should be marked as
not-for-merge or not when we write it out to FETCH_HEAD.

It would be useful to turn this boolean into a tri-state,
with the third state meaning do not bother writing it out
to FETCH_HEAD at all. That would let us add extra refs to
the set of refs to be stored (e.g., to store copies of
things we fetched) without impacting FETCH_HEAD.

This patch turns it into an enum that covers the tri-state
case, and hopefully makes the code more explicit and easier
to read.

Signed-off-by: Jeff King p...@peff.net
---
 builtin/fetch.c | 57 +++--
 cache.h | 14 +-
 2 files changed, 48 insertions(+), 23 deletions(-)

diff --git a/builtin/fetch.c b/builtin/fetch.c
index 4b6b1df..287cf4c 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -119,7 +119,7 @@ static void add_merge_config(struct ref **head,
 
for (rm = *head; rm; rm = rm-next) {
if (branch_merge_matches(branch, i, rm-name)) {
-   rm-merge = 1;
+   rm-fetch_head_status = FETCH_HEAD_MERGE;
break;
}
}
@@ -140,7 +140,7 @@ static void add_merge_config(struct ref **head,
refspec.src = branch-merge[i]-src;
get_fetch_map(remote_refs, refspec, tail, 1);
for (rm = *old_tail; rm; rm = rm-next)
-   rm-merge = 1;
+   rm-fetch_head_status = FETCH_HEAD_MERGE;
}
 }
 
@@ -167,7 +167,7 @@ static struct ref *get_ref_map(struct transport *transport,
}
/* Merge everything on the command line, but not --tags */
for (rm = ref_map; rm; rm = rm-next)
-   rm-merge = 1;
+   rm-fetch_head_status = FETCH_HEAD_MERGE;
if (tags == TAGS_SET)
get_fetch_map(remote_refs, tag_refspec, tail, 0);
} else {
@@ -186,7 +186,7 @@ static struct ref *get_ref_map(struct transport *transport,
*autotags = 1;
if (!i  !has_merge  ref_map 
!remote-fetch[0].pattern)
-   ref_map-merge = 1;
+   ref_map-fetch_head_status = 
FETCH_HEAD_MERGE;
}
/*
 * if the remote we're fetching from is the same
@@ -202,7 +202,7 @@ static struct ref *get_ref_map(struct transport *transport,
ref_map = get_remote_ref(remote_refs, HEAD);
if (!ref_map)
die(_(Couldn't find remote ref HEAD));
-   ref_map-merge = 1;
+   ref_map-fetch_head_status = FETCH_HEAD_MERGE;
tail = ref_map-next;
}
}
@@ -389,7 +389,7 @@ static int store_updated_refs(const char *raw_url, const 
char *remote_name,
const char *what, *kind;
struct ref *rm;
char *url, *filename = dry_run ? /dev/null : git_path(FETCH_HEAD);
-   int want_merge;
+   int want_status;
 
fp = fopen(filename, a);
if (!fp)
@@ -407,19 +407,22 @@ static int store_updated_refs(const char *raw_url, const 
char *remote_name,
}
 
/*
-* The first pass writes objects to be merged and then the
-* second pass writes the rest, in order to allow using
-* FETCH_HEAD as a refname to refer to the ref to be merged.
+* We do a pass for each fetch_head_status type in their enum order, so
+* merged entries are written before not-for-merge. That lets readers
+* use FETCH_HEAD as a refname to refer to the ref to be merged.
 */
-   for (want_merge = 1; 0 = want_merge; want_merge--) {
+   for (want_status = FETCH_HEAD_MERGE;
+want_status = FETCH_HEAD_IGNORE;
+want_status++) {
for (rm = ref_map; rm; rm = rm-next) {
struct ref *ref = NULL;
+   const char *merge_status_marker = ;
 
commit = lookup_commit_reference_gently(rm-old_sha1, 
1);
if (!commit)
-   rm-merge = 0;
+   rm-fetch_head_status = 
FETCH_HEAD_NOT_FOR_MERGE;
 
-   if (rm-merge != want_merge)
+   if (rm-fetch_head_status != want_status)
continue;
 
if (rm-peer_ref) {
@@ -465,16 +468,26 @@ static int store_updated_refs(const char *raw_url, const 
char *remote_name,
strbuf_addf(note, %s , kind);

[PATCH 4/4] fetch: opportunistically update tracking refs

2013-05-11 Thread Jeff King
When we run a regular git fetch without arguments, we
update the tracking refs according to the configured
refspec. However, when we run git fetch origin master (or
git pull origin master), we do not look at the configured
refspecs at all, and just update FETCH_HEAD.

We miss an opportunity to update refs/remotes/origin/master
(or whatever the user has configured). Some users find this
confusing, because they would want to do further comparisons
against the old state of the remote master, like:

  $ git pull origin master
  $ git log HEAD...origin/master

In the currnet code, they are comparing against whatever
commit happened to be in origin/master from the last time
they did a complete git fetch.  This patch will update a
ref from the RHS of a configured refspec whenever we happen
to be fetching its LHS. That makes the case above work.

The downside is that any users who really care about whether
and when their tracking branches are updated may be
surprised.

Signed-off-by: Jeff King p...@peff.net
---
 Documentation/pull-fetch-param.txt |  2 +-
 builtin/fetch.c| 16 
 t/t5510-fetch.sh   |  8 
 3 files changed, 21 insertions(+), 5 deletions(-)

diff --git a/Documentation/pull-fetch-param.txt 
b/Documentation/pull-fetch-param.txt
index 6f5ca21..18cffc2 100644
--- a/Documentation/pull-fetch-param.txt
+++ b/Documentation/pull-fetch-param.txt
@@ -75,4 +75,4 @@ endif::git-pull[]
 * A parameter ref without a colon merges ref into the current
   branch,
 endif::git-pull[]
-  while not storing the branch anywhere locally.
+  and updates the remote-tracking branches (if any).
diff --git a/builtin/fetch.c b/builtin/fetch.c
index 287cf4c..e41cc0d 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -160,6 +160,8 @@ static struct ref *get_ref_map(struct transport *transport,
const struct ref *remote_refs = transport_get_remote_refs(transport);
 
if (ref_count || tags == TAGS_SET) {
+   struct ref **old_tail;
+
for (i = 0; i  ref_count; i++) {
get_fetch_map(remote_refs, refs[i], tail, 0);
if (refs[i].dst  refs[i].dst[0])
@@ -170,6 +172,20 @@ static struct ref *get_ref_map(struct transport *transport,
rm-fetch_head_status = FETCH_HEAD_MERGE;
if (tags == TAGS_SET)
get_fetch_map(remote_refs, tag_refspec, tail, 0);
+
+   /*
+* For any refs that we happen to be fetching via command-line
+* arguments, take the opportunity to update their configured
+* counterparts. However, we do not want to mention these
+* entries in FETCH_HEAD at all, as they would simply be
+* duplicates of existing entries.
+*/
+   old_tail = tail;
+   for (i = 0; i  transport-remote-fetch_refspec_nr; i++)
+   get_fetch_map(ref_map, transport-remote-fetch[i],
+ tail, 0);
+   for (rm = *old_tail; rm; rm = rm-next)
+   rm-fetch_head_status = FETCH_HEAD_IGNORE;
} else {
/* Use the defaults */
struct remote *remote = transport-remote;
diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh
index 789c228..ff43e08 100755
--- a/t/t5510-fetch.sh
+++ b/t/t5510-fetch.sh
@@ -377,7 +377,7 @@ test_expect_success 'mark initial state of origin/master' '
)
 '
 
-test_expect_success 'explicit fetch should not update tracking' '
+test_expect_success 'explicit fetch should update tracking' '
 
cd $D 
git branch -f side 
@@ -387,12 +387,12 @@ test_expect_success 'explicit fetch should not update 
tracking' '
o=$(git rev-parse --verify refs/remotes/origin/master) 
git fetch origin master 
n=$(git rev-parse --verify refs/remotes/origin/master) 
-   test $o = $n 
+   test $o != $n 
test_must_fail git rev-parse --verify refs/remotes/origin/side
)
 '
 
-test_expect_success 'explicit pull should not update tracking' '
+test_expect_success 'explicit pull should update tracking' '
 
cd $D 
git branch -f side 
@@ -402,7 +402,7 @@ test_expect_success 'explicit pull should not update 
tracking' '
o=$(git rev-parse --verify refs/remotes/origin/master) 
git pull origin master 
n=$(git rev-parse --verify refs/remotes/origin/master) 
-   test $o = $n 
+   test $o != $n 
test_must_fail git rev-parse --verify refs/remotes/origin/side
)
 '
-- 
1.8.3.rc1.2.g12db477
--
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


[PATCHv2 00/10] Prepare for alternative remote-tracking branch location

2013-05-11 Thread Johan Herland
Hi,

Here is the second iteration of the current series for teaching git to
work with remote ref namespaces. This iteration is pretty much a full
rework of the first iteration, based on Junio's comments to the first
iteration, and discussion with other Git developers at the Git Merge
conference in Berlin, particularily Jonathan Nieder has helped to work
out some of these issues.

Patches #1 - #2 are pretty much unchanged from v1, except that we now
put the remote refs in refs/peers/* instead of /refs/remotes/*.

Patches #3 - #5 gets us to the point where git rev-parse origin/master
will expand to refs/peers/origin/heads/master, and vice versa for
shortening

Patches #6-#10 (except #7 which is a small unrelated test addition) is
about making git branch -a and git branch -r output remote-tracking
branches from the refs/peers/* hierarchy.

Note that this patch series is based on top of the jh/shorten-refname
series in 'pu'.


Have fun!

...Johan


Johan Herland (10):
  t7900: Start testing usability of namespaced remote refs
  t7900: Demonstrate failure to expand $peer/$branch according to refspecs
  refs.c: Refactor code for mapping between shorthand names and full refnames
  remote: Reject remote names containing '/'
  refs.c: Add support for expanding/shortening refs in refs/peers/*
  t7900: Test git branch -r/-a output w/remote-tracking branches in refs/peers/*
  t3203: Add testcase for fix in 1603ade81352a526ccb206f41ff81ecbc855df2d
  builtin/branch.c: Refactor ref_item.name and .dest into strbufs
  builtin/branch.c: Refactor remotes/ prepending to remote-tracking branches
  branch: Fix display of remote branches in refs/peers/*

 builtin/branch.c   | 114 +--
 builtin/remote.c   |   4 +-
 cache.h|   4 -
 refs.c | 256 ++---
 refs.h |   6 +
 remote.c   |   6 +-
 t/t3203-branch-output.sh   |  15 ++
 t/t5505-remote.sh  |  12 ++
 t/t7900-working-with-namespaced-remote-refs.sh | 131 +
 t/t7901-multi-level-remote-name-failure.sh |  20 ++
 10 files changed, 430 insertions(+), 138 deletions(-)
 create mode 100755 t/t7900-working-with-namespaced-remote-refs.sh
 create mode 100755 t/t7901-multi-level-remote-name-failure.sh

-- 
1.8.1.3.704.g33f7d4f

--
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


[PATCHv2 01/10] t7900: Start testing usability of namespaced remote refs

2013-05-11 Thread Johan Herland
Some users are interested in fetching remote refs into a separate namespace
in the local repo. E.g. instead of the usual remote config:

  [remote origin]
fetch = +refs/heads/*:refs/remotes/origin/*
url = ...

they want to keep remote tags separate from local tags, and they may also
want to fetch other ref types:

  [remote origin]
fetch = +refs/heads/*:refs/peers/origin/heads/*
fetch = +refs/tags/*:refs/peers/origin/tags/*
fetch = +refs/notes/*:refs/peers/origin/notes/*
fetch = +refs/replace/*:refs/peers/origin/replace/*
tagopt = --no-tags
url = ...

This configuration creates a separate namespace under refs/peers/origin/*
mirroring the structure of local refs (under refs/*) where all the relevant
refs from the 'origin' remote can be found. The reason for renaming the
refs/remotes/ prefix to refs/peers/ is to allow Git to treat the two
hierarchies somewhat differently with respect to how shorthand names are
expanded by the ref machinery.

This patch introduces a test whose main purpose is to verify that git will
work comfortably with this kind of setup. For now, we only verify that it
is possible (though not exactly easy) to establish a clone with the above
configuration, and that fetching into it yields the expected result.

Signed-off-by: Johan Herland jo...@herland.net
---
 t/t7900-working-with-namespaced-remote-refs.sh | 82 ++
 1 file changed, 82 insertions(+)
 create mode 100755 t/t7900-working-with-namespaced-remote-refs.sh

diff --git a/t/t7900-working-with-namespaced-remote-refs.sh 
b/t/t7900-working-with-namespaced-remote-refs.sh
new file mode 100755
index 000..dfd916b
--- /dev/null
+++ b/t/t7900-working-with-namespaced-remote-refs.sh
@@ -0,0 +1,82 @@
+#!/bin/sh
+
+test_description='testing end-user usability of namespaced remote refs
+
+Set up a local repo with namespaced remote refs, like this:
+
+[remote origin]
+   fetch = +refs/heads/*:refs/peers/origin/heads/*
+   fetch = +refs/tags/*:refs/peers/origin/tags/*
+   fetch = +refs/notes/*:refs/peers/origin/notes/*
+   fetch = +refs/replace/*:refs/peers/origin/replace/*
+   tagopt = --no-tags
+   url = ...
+
+Test that the usual end-user operations work as expected with this setup.
+'
+
+. ./test-lib.sh
+
+test_expect_success 'setup server repo' '
+   git init server 
+   (
+   cd server 
+   test_commit server_master_a 
+   git checkout -b other 
+   test_commit server_other_b 
+   git checkout master 
+   test_commit server_master_b
+   )
+'
+
+server_master_a=$(git --git-dir=server/.git rev-parse --verify server_master_a)
+server_master_b=$(git --git-dir=server/.git rev-parse --verify server_master_b)
+server_other_b=$(git --git-dir=server/.git rev-parse --verify server_other_b)
+
+cat expect.refspecs  EOF
++refs/heads/*:refs/peers/origin/heads/*
++refs/tags/*:refs/peers/origin/tags/*
++refs/notes/*:refs/peers/origin/notes/*
++refs/replace/*:refs/peers/origin/replace/*
+EOF
+
+cat expect.show-ref  EOF
+$server_master_b refs/heads/master
+$server_master_b refs/peers/origin/heads/master
+$server_other_b refs/peers/origin/heads/other
+$server_master_a refs/peers/origin/tags/server_master_a
+$server_master_b refs/peers/origin/tags/server_master_b
+$server_other_b refs/peers/origin/tags/server_other_b
+EOF
+
+test_clone() {
+   ( cd $1  git config --get-all remote.origin.fetch ) actual.refspecs 

+   test_cmp expect.refspecs actual.refspecs 
+   ( cd $1  git show-ref ) actual.show-ref 
+   test_cmp expect.show-ref actual.show-ref
+}
+
+test_expect_failure 'clone with namespaced remote refs' '
+   git clone --layout=peers server client 
+   test_clone client
+'
+
+# Work-around for the not-yet-existing clone option used above
+test_expect_success 'work-around clone with namespaced remote refs' '
+   rm -rf client 
+   git init client 
+   (
+   cd client 
+   git config remote.origin.url ../server 
+   git config --add remote.origin.fetch 
+refs/heads/*:refs/peers/origin/heads/* 
+   git config --add remote.origin.fetch 
+refs/tags/*:refs/peers/origin/tags/* 
+   git config --add remote.origin.fetch 
+refs/notes/*:refs/peers/origin/notes/* 
+   git config --add remote.origin.fetch 
+refs/replace/*:refs/peers/origin/replace/* 
+   git config remote.origin.tagopt --no-tags 
+   git fetch 
+   git checkout master
+   ) 
+   test_clone client
+'
+
+test_done
-- 
1.8.1.3.704.g33f7d4f

--
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


[PATCHv2 02/10] t7900: Demonstrate failure to expand $peer/$branch according to refspecs

2013-05-11 Thread Johan Herland
This test verifies that the following expressions all evaluate to the
full refname refs/peers/origin/heads/master:

 - refs/peers/origin/heads/master
 - peers/origin/heads/master
 - origin/heads/master
 - origin/master

(We assume that there are no other conflicting refs for which the above
expressions would cause ambiguity.)

Currently none of these work, because the refs machinery don't know how
to expand shorthand names within the refs/peers/* hierarchy.

Mirroring the expansion of the above 4 expressions into the full refname,
the same 4 expressions should also be shortened into origin/master when
abbreviating them into their shortest unambiguous representation, e.g.
when running git rev-parse --abbrev-ref on them. A (currently failing)
test verifying this behavior is also added by this patch.

Signed-off-by: Johan Herland jo...@herland.net
---
 t/t7900-working-with-namespaced-remote-refs.sh | 28 ++
 1 file changed, 28 insertions(+)

diff --git a/t/t7900-working-with-namespaced-remote-refs.sh 
b/t/t7900-working-with-namespaced-remote-refs.sh
index dfd916b..109e9b8 100755
--- a/t/t7900-working-with-namespaced-remote-refs.sh
+++ b/t/t7900-working-with-namespaced-remote-refs.sh
@@ -79,4 +79,32 @@ test_expect_success 'work-around clone with namespaced 
remote refs' '
test_clone client
 '
 
+test_expect_success 'enter client repo' '
+   cd client
+'
+
+test_expect_failure 'short-hand notation expands correctly for remote-tracking 
branches' '
+   echo refs/peers/origin/heads/master expect 
+   git rev-parse --symbolic-full-name refs/peers/origin/heads/master 
actual 
+   test_cmp expect actual 
+   git rev-parse --symbolic-full-name peers/origin/heads/master actual 
+   test_cmp expect actual 
+   git rev-parse --symbolic-full-name origin/heads/master actual 
+   test_cmp expect actual 
+   git rev-parse --symbolic-full-name origin/master actual 
+   test_cmp expect actual
+'
+
+test_expect_failure 'remote-tracking branches are shortened correctly' '
+   echo origin/master expect 
+   git rev-parse --abbrev-ref refs/peers/origin/heads/master actual 
+   test_cmp expect actual 
+   git rev-parse --abbrev-ref peers/origin/heads/master actual 
+   test_cmp expect actual 
+   git rev-parse --abbrev-ref origin/heads/master actual 
+   test_cmp expect actual 
+   git rev-parse --abbrev-ref origin/master actual 
+   test_cmp expect actual
+'
+
 test_done
-- 
1.8.1.3.704.g33f7d4f

--
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


[PATCHv2 03/10] refs.c: Refactor code for mapping between shorthand names and full refnames

2013-05-11 Thread Johan Herland
This patch is in preparation for extending the ways in which we expand
shorthand names into full refnames, and shorten full refnames into
unambiguous shorthand names.

We collect the logic for performing the expansion and shortening into two
functions: refname_expand() and refname_shorten(). refname_expand() takes
a shorthand name, and a pattern containing a wildcard (more on those
below), and substitutes the shorthand name for the wildcard. The result
is written into a strbuf provided by the caller. Correspondingly,
refname_shorten() takes a full refname and matches it against the given
pattern, extracting the portion of the full refname that matches the
pattern wildcard. The resulting shorthand name is written into a strbuf
provided by the caller.

The refname_expand() function no longer uses mkpath()/mksnpath() to
perform the pattern expansion. Instead, it uses strbuf_expand(), which
removes the need for using fixed-length buffers from the code. This also
means we no longer call cleanup_path() on the resulting refs, but AFAICS,
this is not really needed anyway (also it breaks no existing tests).

Since the pattern expansion is now done by strbuf_expand(), the syntax of
the pattern wildcard must change as well. Whereas a pattern used to look
like this:

refs/remotes/%.*s/HEAD

it now looks like this:

refs/remotes/%*/HEAD

where the %* wildcard is meant to be read as the full shorthand name.
More wildcards will be added by future patches that require more complex
wildcard matching.

The list of patterns has been renamed from ref_rev_parse_rules to
refname_patterns (which should be more descriptive), and it has lost
the NULL terminator, which is no longer needed, since all iterations of
the list now uses ARRAY_SIZE(refname_patterns) to end iteration, instead
of testing each member against NULL.

Furthermore, refname_match() used to take the list of patterns as an
argument, but since all callers pass the one and only pattern list, we
can drop the argument altogether, and use refname_patterns directly in
refname_match(). This also enables us to make the pattern list static.

The code for shortening full refnames into shorthands, has been adjusted
to deal with the new %* patterns. It now uses strbuf_split_str() with
the '%' as terminator, to split the prefix part from the suffix part.
Then - as before - it matches the prefix from the start of the given
refname and the suffix from the end of the refname. If both matches are
successful, the middle portion is extracted as the resulting shorthand
name.

The end result of this refactoring is equivalent in behavior to the
existing code, but makes it easier for future patches to adjust how
expansion and shortening happens.

All of the existing users of ref_rev_parse_rules are converted to
refname_patterns by this patch, so ref_rev_parse_rules is removed
as well.

Signed-off-by: Johan Herland jo...@herland.net
---
 cache.h  |   4 --
 refs.c   | 187 ++-
 refs.h   |   3 +
 remote.c |   6 +-
 4 files changed, 119 insertions(+), 81 deletions(-)

diff --git a/cache.h b/cache.h
index ac620b2..0cc43b2 100644
--- a/cache.h
+++ b/cache.h
@@ -874,10 +874,6 @@ extern int dwim_log(const char *str, int len, unsigned 
char *sha1, char **ref);
 extern int interpret_branch_name(const char *str, struct strbuf *);
 extern int get_sha1_mb(const char *str, unsigned char *sha1);
 
-extern int refname_match(const char *abbrev_name, const char *full_name, const 
char **rules);
-extern const char *ref_rev_parse_rules[];
-#define ref_fetch_rules ref_rev_parse_rules
-
 extern int create_symref(const char *ref, const char *refs_heads_master, const 
char *logmsg);
 extern int validate_headref(const char *ref);
 
diff --git a/refs.c b/refs.c
index 006f369..ab5e120 100644
--- a/refs.c
+++ b/refs.c
@@ -1783,27 +1783,91 @@ const char *prettify_refname(const char *name)
0);
 }
 
-const char *ref_rev_parse_rules[] = {
-   %.*s,
-   refs/%.*s,
-   refs/tags/%.*s,
-   refs/heads/%.*s,
-   refs/remotes/%.*s,
-   refs/remotes/%.*s/HEAD,
-   NULL
+static const char *refname_patterns[] = {
+   %*,
+   refs/%*,
+   refs/tags/%*,
+   refs/heads/%*,
+   refs/remotes/%*,
+   refs/remotes/%*/HEAD
 };
 
-int refname_match(const char *abbrev_name, const char *full_name, const char 
**rules)
+struct wildcard_data {
+   const char *s;
+   size_t len;
+   int done;
+};
+
+static size_t refname_expand_helper(struct strbuf *sb, const char *placeholder,
+   void *context)
 {
-   const char **p;
-   const int abbrev_name_len = strlen(abbrev_name);
+   struct wildcard_data *cb = context;
+   if (*placeholder == '*'  !cb-done) {
+   strbuf_add(sb, cb-s, cb-len);
+   cb-done = 1;
+   return 1;
+   }
+   return 0;
+}
 
-   for (p = rules; *p; p++) {
-   if 

[PATCHv2 04/10] remote: Reject remote names containing '/'

2013-05-11 Thread Johan Herland
Although we definitely support and encourage use of multi-level branch
names, we have never conciously tried to give support for multi-level
remote names. Currently, they are allowed, but there is no evidence that
they are commonly used.

Now, they do provide a source of problems when trying to expand the
$nick/$name shorthand notation (where $nick matches a remote name)
into a full refname. Consider the shorthand foo/bar/baz: Does this
parse as $nick = foo, $name = bar/baz, or $nick = foo/bar, $name = baz?

Since we need to be unambiguous about these things, we hereby declare
that a remote name shall never contain a '/' character, and that the
only correct way to parse foo/bar/baz is $nick = foo, $name = bar/baz.

This patch teaches 'git remote' to reject remote names with slashes,
and adds tests verifying this.

Signed-off-by: Johan Herland jo...@herland.net
---
 builtin/remote.c  |  4 ++--
 t/t5505-remote.sh | 12 
 2 files changed, 14 insertions(+), 2 deletions(-)

diff --git a/builtin/remote.c b/builtin/remote.c
index 5e54d36..6e7369d 100644
--- a/builtin/remote.c
+++ b/builtin/remote.c
@@ -195,7 +195,7 @@ static int add(int argc, const char **argv)
die(_(remote %s already exists.), name);
 
strbuf_addf(buf2, refs/heads/test:refs/remotes/%s/test, name);
-   if (!valid_fetch_refspec(buf2.buf))
+   if (!valid_fetch_refspec(buf2.buf) || strchr(name, '/'))
die(_('%s' is not a valid remote name), name);
 
strbuf_addf(buf, remote.%s.url, name);
@@ -646,7 +646,7 @@ static int mv(int argc, const char **argv)
die(_(remote %s already exists.), rename.new);
 
strbuf_addf(buf, refs/heads/test:refs/remotes/%s/test, rename.new);
-   if (!valid_fetch_refspec(buf.buf))
+   if (!valid_fetch_refspec(buf.buf) || strchr(rename.new, '/'))
die(_('%s' is not a valid remote name), rename.new);
 
strbuf_reset(buf);
diff --git a/t/t5505-remote.sh b/t/t5505-remote.sh
index dd10ff0..f7098fe 100755
--- a/t/t5505-remote.sh
+++ b/t/t5505-remote.sh
@@ -621,6 +621,12 @@ test_expect_success 'reject adding remote with an invalid 
name' '
 
 '
 
+test_expect_success 'reject adding remote with slash in name' '
+
+   test_must_fail git remote add multi/level .
+
+'
+
 # The first three test if the tracking branches are properly renamed,
 # the last two ones check if the config is updated.
 
@@ -668,6 +674,12 @@ test_expect_success 'rename a remote with name prefix of 
other remote' '
 
 '
 
+test_expect_success 'rename a remote with slash in name' '
+
+   test_must_fail git remote rename upstream up/stream
+
+'
+
 cat  remotes_origin  EOF
 URL: $(pwd)/one
 Push: refs/heads/master:refs/heads/upstream
-- 
1.8.1.3.704.g33f7d4f

--
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


[PATCHv2 06/10] t7900: Test git branch -r/-a output w/remote-tracking branches in refs/peers/*

2013-05-11 Thread Johan Herland
These tests will be made to pass by subsequent patches.

Signed-off-by: Johan Herland jo...@herland.net
---
 t/t7900-working-with-namespaced-remote-refs.sh | 21 +
 1 file changed, 21 insertions(+)

diff --git a/t/t7900-working-with-namespaced-remote-refs.sh 
b/t/t7900-working-with-namespaced-remote-refs.sh
index 33266e0..279664c 100755
--- a/t/t7900-working-with-namespaced-remote-refs.sh
+++ b/t/t7900-working-with-namespaced-remote-refs.sh
@@ -107,4 +107,25 @@ test_expect_success 'remote-tracking branches are 
shortened correctly' '
test_cmp expect actual
 '
 
+cat expect.branch-r  EOF
+  origin/master
+  origin/other
+EOF
+
+test_expect_failure 'git branch -r should show remote-tracking branches' '
+   git branch -r actual.branch-r 
+   test_cmp expect.branch-r actual.branch-r
+'
+
+cat expect.branch-a  EOF
+* master
+  peers/origin/heads/master
+  peers/origin/heads/other
+EOF
+
+test_expect_failure 'git branch -a should also show remote-tracking branches' '
+   git branch -a actual.branch-a 
+   test_cmp expect.branch-a actual.branch-a
+'
+
 test_done
-- 
1.8.1.3.704.g33f7d4f

--
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


[PATCHv2 07/10] t3203: Add testcase for fix in 1603ade81352a526ccb206f41ff81ecbc855df2d

2013-05-11 Thread Johan Herland
This adds a testcase for some behavior that I very nearly broke while
refactoring some stuff in builtin/branch.c.

Signed-off-by: Johan Herland jo...@herland.net
---
 t/t3203-branch-output.sh | 15 +++
 1 file changed, 15 insertions(+)

diff --git a/t/t3203-branch-output.sh b/t/t3203-branch-output.sh
index ba4f98e..c312d3a 100755
--- a/t/t3203-branch-output.sh
+++ b/t/t3203-branch-output.sh
@@ -106,4 +106,19 @@ EOF
test_i18ncmp expect actual
 '
 
+test_expect_success 'git branch -a --merged handles broken refs gracefully' '
+   cat expect EOF 
+  branch-one
+  branch-two
+* master
+  remotes/origin/HEAD - origin/branch-one
+  remotes/origin/branch-one
+  remotes/origin/branch-two
+EOF
+   git checkout master 
+   echo broken ref  .git/refs/heads/broken 
+   test_must_fail git branch -a --merged actual 
+   test_cmp expect actual
+'
+
 test_done
-- 
1.8.1.3.704.g33f7d4f

--
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


[PATCHv2 08/10] builtin/branch.c: Refactor ref_item.name and .dest into strbufs

2013-05-11 Thread Johan Herland
In preparation of a future patch which reorganizes how the display of the
ref_list is done.

Signed-off-by: Johan Herland jo...@herland.net
---
 builtin/branch.c | 37 +
 1 file changed, 21 insertions(+), 16 deletions(-)

diff --git a/builtin/branch.c b/builtin/branch.c
index 0836890..c8b49e3 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -276,8 +276,8 @@ static int delete_branches(int argc, const char **argv, int 
force, int kinds,
 }
 
 struct ref_item {
-   char *name;
-   char *dest;
+   struct strbuf name;
+   struct strbuf dest;
unsigned int kind, width;
struct commit *commit;
 };
@@ -380,11 +380,15 @@ static int append_ref(const char *refname, const unsigned 
char *sha1, int flags,
 
/* Record the new item */
newitem = (ref_list-list[ref_list-index++]);
-   newitem-name = xstrdup(refname);
+   strbuf_init(newitem-name, 0);
+   strbuf_addstr(newitem-name, refname);
newitem-kind = kind;
newitem-commit = commit;
newitem-width = utf8_strwidth(refname);
-   newitem-dest = resolve_symref(orig_refname, prefix);
+   strbuf_init(newitem-dest, 0);
+   orig_refname = resolve_symref(orig_refname, prefix);
+   if (orig_refname)
+   strbuf_addstr(newitem-dest, orig_refname);
/* adjust for remotes/ */
if (newitem-kind == REF_REMOTE_BRANCH 
ref_list-kinds != REF_REMOTE_BRANCH)
@@ -400,8 +404,8 @@ static void free_ref_list(struct ref_list *ref_list)
int i;
 
for (i = 0; i  ref_list-index; i++) {
-   free(ref_list-list[i].name);
-   free(ref_list-list[i].dest);
+   strbuf_release(ref_list-list[i].name);
+   strbuf_release(ref_list-list[i].dest);
}
free(ref_list-list);
 }
@@ -413,7 +417,7 @@ static int ref_cmp(const void *r1, const void *r2)
 
if (c1-kind != c2-kind)
return c1-kind - c2-kind;
-   return strcmp(c1-name, c2-name);
+   return strbuf_cmp(c1-name, c2-name);
 }
 
 static void fill_tracking_info(struct strbuf *stat, const char *branch_name,
@@ -496,7 +500,7 @@ static void add_verbose_info(struct strbuf *out, struct 
ref_item *item,
}
 
if (item-kind == REF_LOCAL_BRANCH)
-   fill_tracking_info(stat, item-name, verbose  1);
+   fill_tracking_info(stat, item-name.buf, verbose  1);
 
strbuf_addf(out,  %s %s%s,
find_unique_abbrev(item-commit-object.sha1, abbrev),
@@ -534,7 +538,7 @@ static void print_ref_item(struct ref_item *item, int 
maxwidth, int verbose,
color = BRANCH_COLOR_CURRENT;
}
 
-   strbuf_addf(name, %s%s, prefix, item-name);
+   strbuf_addf(name, %s%s, prefix, item-name.buf);
if (verbose) {
int utf8_compensation = strlen(name.buf) - 
utf8_strwidth(name.buf);
strbuf_addf(out, %c %s%-*s%s, c, branch_get_color(color),
@@ -544,8 +548,8 @@ static void print_ref_item(struct ref_item *item, int 
maxwidth, int verbose,
strbuf_addf(out, %c %s%s%s, c, branch_get_color(color),
name.buf, branch_get_color(BRANCH_COLOR_RESET));
 
-   if (item-dest)
-   strbuf_addf(out,  - %s, item-dest);
+   if (item-dest.len)
+   strbuf_addf(out,  - %s, item-dest.buf);
else if (verbose)
/*  f7c0c00 [ahead 58, behind 197] vcs-svn: drop obj_pool.h */
add_verbose_info(out, item, verbose, abbrev);
@@ -601,15 +605,16 @@ static void show_detached(struct ref_list *ref_list)
 
if (head_commit  is_descendant_of(head_commit, 
ref_list-with_commit)) {
struct ref_item item;
-   item.name = get_head_description();
-   item.width = utf8_strwidth(item.name);
+   strbuf_init(item.name, 0);
+   strbuf_addstr(item.name, get_head_description());
+   item.width = utf8_strwidth(item.name.buf);
item.kind = REF_LOCAL_BRANCH;
-   item.dest = NULL;
+   strbuf_init(item.dest, 0);
item.commit = head_commit;
if (item.width  ref_list-maxwidth)
ref_list-maxwidth = item.width;
print_ref_item(item, ref_list-maxwidth, ref_list-verbose, 
ref_list-abbrev, 1, );
-   free(item.name);
+   strbuf_release(item.name);
}
 }
 
@@ -655,7 +660,7 @@ static int print_ref_list(int kinds, int detached, int 
verbose, int abbrev, stru
for (i = 0; i  ref_list.index; i++) {
int current = !detached 
(ref_list.list[i].kind == REF_LOCAL_BRANCH) 
-   !strcmp(ref_list.list[i].name, head);
+   !strcmp(ref_list.list[i].name.buf, head);
char *prefix = (kinds != REF_REMOTE_BRANCH 

[PATCHv2 09/10] builtin/branch.c: Refactor remotes/ prepending to remote-tracking branches

2013-05-11 Thread Johan Herland
When running git branch -a (instead of git branch -r), we prefix the
remote-tracking branches with remotes/ to allow the user to more easily
tell them apart from the local branches.

The code that prepended remotes/ to remote-tracking branches was
located in print_ref_item(), while the code that adjusted the
ref_item.width (to account for the prepended remotes/) was located in
append_ref().

This code moves the prepending of remotes/ up into append_ref(), which
is a nice cleanup of the code, as well as a preparation for changing the
logic when remote-tracking branches start showing up in refs/peers/*.

Signed-off-by: Johan Herland jo...@herland.net
---
 builtin/branch.c | 23 +--
 1 file changed, 9 insertions(+), 14 deletions(-)

diff --git a/builtin/branch.c b/builtin/branch.c
index c8b49e3..4480be2 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -384,7 +384,6 @@ static int append_ref(const char *refname, const unsigned 
char *sha1, int flags,
strbuf_addstr(newitem-name, refname);
newitem-kind = kind;
newitem-commit = commit;
-   newitem-width = utf8_strwidth(refname);
strbuf_init(newitem-dest, 0);
orig_refname = resolve_symref(orig_refname, prefix);
if (orig_refname)
@@ -392,7 +391,8 @@ static int append_ref(const char *refname, const unsigned 
char *sha1, int flags,
/* adjust for remotes/ */
if (newitem-kind == REF_REMOTE_BRANCH 
ref_list-kinds != REF_REMOTE_BRANCH)
-   newitem-width += 8;
+   strbuf_insert(newitem-name, 0, remotes/, 8);
+   newitem-width = utf8_strwidth(newitem-name.buf);
if (newitem-width  ref_list-maxwidth)
ref_list-maxwidth = newitem-width;
 
@@ -510,12 +510,12 @@ static void add_verbose_info(struct strbuf *out, struct 
ref_item *item,
 }
 
 static void print_ref_item(struct ref_item *item, int maxwidth, int verbose,
-  int abbrev, int current, char *prefix)
+  int abbrev, int current)
 {
char c;
int color;
struct commit *commit = item-commit;
-   struct strbuf out = STRBUF_INIT, name = STRBUF_INIT;
+   struct strbuf out = STRBUF_INIT;
 
if (!matches_merge_filter(commit))
return;
@@ -538,15 +538,14 @@ static void print_ref_item(struct ref_item *item, int 
maxwidth, int verbose,
color = BRANCH_COLOR_CURRENT;
}
 
-   strbuf_addf(name, %s%s, prefix, item-name.buf);
if (verbose) {
-   int utf8_compensation = strlen(name.buf) - 
utf8_strwidth(name.buf);
+   int utf8_compensation = strlen(item-name.buf) - 
utf8_strwidth(item-name.buf);
strbuf_addf(out, %c %s%-*s%s, c, branch_get_color(color),
-   maxwidth + utf8_compensation, name.buf,
+   maxwidth + utf8_compensation, item-name.buf,
branch_get_color(BRANCH_COLOR_RESET));
} else
strbuf_addf(out, %c %s%s%s, c, branch_get_color(color),
-   name.buf, branch_get_color(BRANCH_COLOR_RESET));
+   item-name.buf, 
branch_get_color(BRANCH_COLOR_RESET));
 
if (item-dest.len)
strbuf_addf(out,  - %s, item-dest.buf);
@@ -559,7 +558,6 @@ static void print_ref_item(struct ref_item *item, int 
maxwidth, int verbose,
} else {
printf(%s\n, out.buf);
}
-   strbuf_release(name);
strbuf_release(out);
 }
 
@@ -613,7 +611,7 @@ static void show_detached(struct ref_list *ref_list)
item.commit = head_commit;
if (item.width  ref_list-maxwidth)
ref_list-maxwidth = item.width;
-   print_ref_item(item, ref_list-maxwidth, ref_list-verbose, 
ref_list-abbrev, 1, );
+   print_ref_item(item, ref_list-maxwidth, ref_list-verbose, 
ref_list-abbrev, 1);
strbuf_release(item.name);
}
 }
@@ -661,11 +659,8 @@ static int print_ref_list(int kinds, int detached, int 
verbose, int abbrev, stru
int current = !detached 
(ref_list.list[i].kind == REF_LOCAL_BRANCH) 
!strcmp(ref_list.list[i].name.buf, head);
-   char *prefix = (kinds != REF_REMOTE_BRANCH 
-   ref_list.list[i].kind == REF_REMOTE_BRANCH)
-   ? remotes/ : ;
print_ref_item(ref_list.list[i], ref_list.maxwidth, verbose,
-  abbrev, current, prefix);
+  abbrev, current);
}
 
free_ref_list(ref_list);
-- 
1.8.1.3.704.g33f7d4f

--
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


[PATCHv2 10/10] branch: Fix display of remote branches in refs/peers/*

2013-05-11 Thread Johan Herland
The current branch display code assumes all remote-tracking branches live
inside refs/remotes/*, and that their appropriate shorthand names can be
retrieved by simply stripping off the refs/remotes/ prefix.

When we add remote-tracking branches in refs/peers/$remote/heads/*, the
code must not only be taught how to list refs from the new location, but
also how to appropriately derive shorthand names for the remote-tracking
branches there (instead of stripping off a hardcoded prefix, we must use
the refname_shorten() function introduced in an earlier patch.)

There is already a good set of tests for the expected output from git
branch, and this patch does not break any of them. However, we do fix
the two tests with remote-tracking branches in refs/peers/* introduced
in a previous patch.

Signed-off-by: Johan Herland jo...@herland.net
---
 builtin/branch.c   | 66 ++
 refs.c |  4 +-
 refs.h |  3 ++
 t/t7900-working-with-namespaced-remote-refs.sh |  4 +-
 4 files changed, 42 insertions(+), 35 deletions(-)

diff --git a/builtin/branch.c b/builtin/branch.c
index 4480be2..9a6bce8 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -290,18 +290,16 @@ struct ref_list {
int kinds;
 };
 
-static char *resolve_symref(const char *src, const char *prefix)
+static void resolve_symref(struct strbuf *shortname, const char *pattern, 
const char *src)
 {
unsigned char sha1[20];
int flag;
-   const char *dst, *cp;
+   const char *dst;
 
dst = resolve_ref_unsafe(src, sha1, 0, flag);
-   if (!(dst  (flag  REF_ISSYMREF)))
-   return NULL;
-   if (prefix  (cp = skip_prefix(dst, prefix)))
-   dst = cp;
-   return xstrdup(dst);
+   if (dst  (flag  REF_ISSYMREF) 
+   refname_shorten(shortname, pattern, dst, strlen(dst)))
+   strbuf_addstr(shortname, dst);
 }
 
 struct append_ref_cb {
@@ -328,74 +326,80 @@ static int append_ref(const char *refname, const unsigned 
char *sha1, int flags,
struct ref_list *ref_list = cb-ref_list;
struct ref_item *newitem;
struct commit *commit;
+   struct strbuf ref = STRBUF_INIT;
int kind, i;
-   const char *prefix, *orig_refname = refname;
+   const char *pattern;
+   size_t refname_len = strlen(refname);
 
static struct {
int kind;
-   const char *prefix;
-   int pfxlen;
+   const char *pattern;
} ref_kind[] = {
-   { REF_LOCAL_BRANCH, refs/heads/, 11 },
-   { REF_REMOTE_BRANCH, refs/remotes/, 13 },
+   { REF_LOCAL_BRANCH, refs/heads/%* },
+   { REF_REMOTE_BRANCH, refs/remotes/%* },
+   { REF_REMOTE_BRANCH, refs/peers/%1/heads/%* },
};
 
/* Detect kind */
for (i = 0; i  ARRAY_SIZE(ref_kind); i++) {
-   prefix = ref_kind[i].prefix;
-   if (strncmp(refname, prefix, ref_kind[i].pfxlen))
+   if (refname_shorten(ref, ref_kind[i].pattern, refname, 
refname_len))
continue;
kind = ref_kind[i].kind;
-   refname += ref_kind[i].pfxlen;
+   pattern = ref_kind[i].pattern;
break;
}
if (ARRAY_SIZE(ref_kind) = i)
-   return 0;
+   goto out;
 
/* Don't add types the caller doesn't want */
if ((kind  ref_list-kinds) == 0)
-   return 0;
+   goto out;
 
-   if (!match_patterns(cb-pattern, refname))
-   return 0;
+   if (!match_patterns(cb-pattern, ref.buf))
+   goto out;
 
commit = NULL;
if (ref_list-verbose || ref_list-with_commit || merge_filter != 
NO_FILTER) {
commit = lookup_commit_reference_gently(sha1, 1);
if (!commit) {
-   cb-ret = error(_(branch '%s' does not point at a 
commit), refname);
-   return 0;
+   cb-ret = error(_(branch '%s' does not point at a 
commit), ref.buf);
+   goto out;
}
 
/* Filter with with_commit if specified */
if (!is_descendant_of(commit, ref_list-with_commit))
-   return 0;
+   goto out;
 
if (merge_filter != NO_FILTER)
add_pending_object(ref_list-revs,
-  (struct object *)commit, refname);
+  (struct object *)commit, ref.buf);
}
 
+   /*
+* When displaying more then just remote-tracking branches, make the
+* remote-tracking branches more explicit, e.g. instead of printing
+* origin/master, we should print remote/origin/master (or
+* 

[PATCHv2 05/10] refs.c: Add support for expanding/shortening refs in refs/peers/*

2013-05-11 Thread Johan Herland
This patch adds the following patterns for expanding/shortening refs:

  refs/peers/%*
  refs/peers/%1/tags/%*
  refs/peers/%1/heads/%*

These allow shorthand names like origin/master to expand to refs in
the refs/peers/* hierarchy (in this case, the likely expansion would be
by the middle rule above, resulting in refs/peers/origin/heads/master).

To accomplish this, we have added the new %1 wildcard which shall
expand into the first component (i.e. up to the first '/') of the given
shorthand. The other wildcard (%*) shall then expand into the remainder
of the shorthand (i.e. following the first '/').

Correspondingly, when shortening according to a pattern with %1, a
single component (not including any '/' character) shall be extracted
from that point in the given refname, and shall be added to the resulting
shorthand, with a trailing '/'. Then, when hitting the %*, the
remainder of the given refname (modulo a trailing match in the pattern)
shall be extracted and appended to the portion previously extracted by
the %1 wildcard.

The need to split the $remote/$ref into its $remote and $ref parts is
the reason why multi-level remote names will no longer work (and hence
were disallowed in the previous patch). A testcase demonstrating how
multi-level remote names fail is therefore included in this patch.

Signed-off-by: Johan Herland jo...@herland.net
---
 refs.c | 101 +
 t/t7900-working-with-namespaced-remote-refs.sh |   4 +-
 t/t7901-multi-level-remote-name-failure.sh |  20 +
 3 files changed, 107 insertions(+), 18 deletions(-)
 create mode 100755 t/t7901-multi-level-remote-name-failure.sh

diff --git a/refs.c b/refs.c
index ab5e120..188a9eb 100644
--- a/refs.c
+++ b/refs.c
@@ -1789,7 +1789,10 @@ static const char *refname_patterns[] = {
refs/tags/%*,
refs/heads/%*,
refs/remotes/%*,
-   refs/remotes/%*/HEAD
+   refs/remotes/%*/HEAD,
+   refs/peers/%*,
+   refs/peers/%1/tags/%*,
+   refs/peers/%1/heads/%*
 };
 
 struct wildcard_data {
@@ -1806,6 +1809,15 @@ static size_t refname_expand_helper(struct strbuf *sb, 
const char *placeholder,
strbuf_add(sb, cb-s, cb-len);
cb-done = 1;
return 1;
+   } else if (*placeholder == '1'  !cb-done) {
+   const char *p = memchr(cb-s, '/', cb-len);
+   size_t copy_len = p ? p - cb-s : cb-len;
+   strbuf_add(sb, cb-s, copy_len);
+   if (copy_len  cb-len  cb-s[copy_len] == '/')
+   copy_len++;
+   cb-s += copy_len;
+   cb-len -= copy_len;
+   return 1;
}
return 0;
 }
@@ -1821,6 +1833,46 @@ static int refname_expand(struct strbuf *dst, const char 
*pattern,
return !cbdata.done;
 }
 
+static int handle_fragment(struct strbuf *dst, struct strbuf *fragment,
+  int first, int last,
+  const char *refname, size_t refname_len)
+{
+   const char *ref = refname, *p;
+   size_t ref_len = refname_len, trail_len;
+   if (!first) {
+   /* extract wildcard according to wildcard char */
+   switch (fragment-buf[0]) {
+   case '1':
+   /* extract up to next '/' from refname */
+   p = memchr(ref, '/', ref_len);
+   if (!p)
+   return -1;
+   strbuf_add(dst, ref, p - ref);
+   strbuf_addch(dst, '/');
+   ref_len -= p - ref;
+   ref += p - ref;
+   break;
+   case '*':
+   /* extract all up to matching trailer */
+   assert(last);
+   trail_len = fragment-len - 1;
+   if (trail_len  ref_len)
+   return -1;
+   strbuf_add(dst, ref, ref_len - trail_len);
+   ref += ref_len - trail_len;
+   ref_len -= ref_len - trail_len;
+   break;
+   }
+   }
+
+   /* match rest of fragment verbatim */
+   p = fragment-buf + (first ? 0 : 1); /* skip wildcard character */
+   trail_len = fragment-len - ((first ? 0 : 1) + (last ? 0 : 1));
+   if (trail_len  ref_len || memcmp(ref, p, trail_len))
+   return -1;
+   return (ref - refname) + trail_len;
+}
+
 static int refname_shorten(struct strbuf *dst, const char *pattern,
   const char *refname, size_t refname_len)
 {
@@ -1830,26 +1882,43 @@ static int refname_shorten(struct strbuf *dst, const 
char *pattern,
 * Return 0 on success (positive match, wildcard-matching portion
 * copied into dst), and non-zero on failure (no match, dst empty).
 */
-   const char *match_end;
-   struct strbuf 

[PATCH 0/2] Colored prompt for zsh

2013-05-11 Thread Ramkumar Ramachandra
Hi,

I was using ZSH's vcs_info until now, and just discovered
contrib/completion/git-prompt.sh.  Other differences aside, it's
simple enough that I can hack on it.

The first patch strips a small whitespace (we can't waste valuable
real estate on unnecessary whitespace), and the second patch should
get you a beautiful colored prompt in ZSH.

Enjoy!

Ramkumar Ramachandra (2):
  git-prompt.sh: strip unnecessary space in prompt string
  git-prompt.zsh: introduce thin ZSH wrapper

 contrib/completion/git-prompt.sh  | 80 ---
 contrib/completion/git-prompt.zsh | 59 +
 2 files changed, 101 insertions(+), 38 deletions(-)
 create mode 100644 contrib/completion/git-prompt.zsh

-- 
1.8.3.rc1.52.g4537cf1

--
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] git-prompt.sh: strip unnecessary space in prompt string

2013-05-11 Thread Ramkumar Ramachandra
Nobody has branch names that end with + or *.  Then why put a space
after the branch name and before [*|+][=||] in the prompt string?

Before this, your prompt might have looked like:

artagnon|master *=:~/src/git$

Now, it will look like:

artagnon|master*=:~/src/git$

Signed-off-by: Ramkumar Ramachandra artag...@gmail.com
---
 contrib/completion/git-prompt.sh | 7 ++-
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/contrib/completion/git-prompt.sh b/contrib/completion/git-prompt.sh
index eaf5c36..08c9b22 100644
--- a/contrib/completion/git-prompt.sh
+++ b/contrib/completion/git-prompt.sh
@@ -383,9 +383,6 @@ __git_ps1 ()
# is necessary to prevent wrapping issues!

gitstring=\[$branch_color\]$branchstring\[$c_clear\]
 
-   if [ -n $w$i$s$u$r$p ]; then
-   gitstring=$gitstring 
-   fi
if [ $w = * ]; then
gitstring=$gitstring\[$bad_color\]$w
fi
@@ -400,13 +397,13 @@ __git_ps1 ()
fi
gitstring=$gitstring\[$c_clear\]$r$p
else
-   gitstring=$c${b##refs/heads/}${f:+ $f}$r$p
+   gitstring=$c${b##refs/heads/}${f:+$f}$r$p
fi
gitstring=$(printf -- $printf_format $gitstring)
PS1=$ps1pc_start$gitstring$ps1pc_end
else
# NO color option unless in PROMPT_COMMAND mode
-   printf -- $printf_format $c${b##refs/heads/}${f:+ 
$f}$r$p
+   printf -- $printf_format 
$c${b##refs/heads/}${f:+$f}$r$p
fi
fi
 }
-- 
1.8.3.rc1.52.g4537cf1

--
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] git-prompt.zsh: introduce thin ZSH wrapper

2013-05-11 Thread Ramkumar Ramachandra
To facilitate a colored prompt in ZSH, write a thin wrapper around
git-prompt.sh, factoring out and overriding the coloring logic.  Since
ZSH lacks a PROMPT_COMMAND, instruct the user to execute __git_ps1
inside precmd().

Signed-off-by: Ramkumar Ramachandra artag...@gmail.com
---
 contrib/completion/git-prompt.sh  | 73 +--
 contrib/completion/git-prompt.zsh | 59 +++
 2 files changed, 99 insertions(+), 33 deletions(-)
 create mode 100644 contrib/completion/git-prompt.zsh

diff --git a/contrib/completion/git-prompt.sh b/contrib/completion/git-prompt.sh
index 08c9b22..0bc51ad 100644
--- a/contrib/completion/git-prompt.sh
+++ b/contrib/completion/git-prompt.sh
@@ -222,6 +222,45 @@ __git_ps1_show_upstream ()
 
 }
 
+# Helper function that is meant to be called from __git_ps1.  It
+# builds up a gitstring injecting color codes into the appropriate
+# places.
+__git_ps1_colorize_gitstring ()
+{
+   local c_red='\e[31m'
+   local c_green='\e[32m'
+   local c_lblue='\e[1;34m'
+   local c_clear='\e[0m'
+   local bad_color=$c_red
+   local ok_color=$c_green
+   local branch_color=$c_clear
+   local flags_color=$c_lblue
+   local branchstring=$c${b##refs/heads/}
+
+   if [ $detached = no ]; then
+   branch_color=$ok_color
+   else
+   branch_color=$bad_color
+   fi
+
+   # Setting gitstring directly with \[ and \] around colors
+   # is necessary to prevent wrapping issues!
+   gitstring=\[$branch_color\]$branchstring\[$c_clear\]
+
+   if [ $w = * ]; then
+   gitstring=$gitstring\[$bad_color\]$w
+   fi
+   if [ -n $i ]; then
+   gitstring=$gitstring\[$ok_color\]$i
+   fi
+   if [ -n $s ]; then
+   gitstring=$gitstring\[$flags_color\]$s
+   fi
+   if [ -n $u ]; then
+   gitstring=$gitstring\[$bad_color\]$u
+   fi
+   gitstring=$gitstring\[$c_clear\]$r$p
+}
 
 # __git_ps1 accepts 0 or 1 arguments (i.e., format string)
 # when called from PS1 using command substitution
@@ -363,39 +402,7 @@ __git_ps1 ()
if [ $pcmode = yes ]; then
local gitstring=
if [ -n ${GIT_PS1_SHOWCOLORHINTS-} ]; then
-   local c_red='\e[31m'
-   local c_green='\e[32m'
-   local c_lblue='\e[1;34m'
-   local c_clear='\e[0m'
-   local bad_color=$c_red
-   local ok_color=$c_green
-   local branch_color=$c_clear
-   local flags_color=$c_lblue
-   local branchstring=$c${b##refs/heads/}
-
-   if [ $detached = no ]; then
-   branch_color=$ok_color
-   else
-   branch_color=$bad_color
-   fi
-
-   # Setting gitstring directly with \[ and \] 
around colors
-   # is necessary to prevent wrapping issues!
-   
gitstring=\[$branch_color\]$branchstring\[$c_clear\]
-
-   if [ $w = * ]; then
-   gitstring=$gitstring\[$bad_color\]$w
-   fi
-   if [ -n $i ]; then
-   gitstring=$gitstring\[$ok_color\]$i
-   fi
-   if [ -n $s ]; then
-   gitstring=$gitstring\[$flags_color\]$s
-   fi
-   if [ -n $u ]; then
-   gitstring=$gitstring\[$bad_color\]$u
-   fi
-   gitstring=$gitstring\[$c_clear\]$r$p
+   __git_ps1_colorize_gitstring
else
gitstring=$c${b##refs/heads/}${f:+$f}$r$p
fi
diff --git a/contrib/completion/git-prompt.zsh 
b/contrib/completion/git-prompt.zsh
new file mode 100644
index 000..dc164dd
--- /dev/null
+++ b/contrib/completion/git-prompt.zsh
@@ -0,0 +1,59 @@
+# git prompt support for zsh: wrapper around git-prompt.sh
+#
+# To enable:
+#
+#1) Copy this file and git-prompt.sh to ~/.zsh/prompt
+#2) Add the following lines to your .zshrc:
+#
+#  source ~/.zsh/prompt/git-prompt.zsh
+#  GIT_PS1_SHOWCOLORHINTS=true
+#  precmd () { __git_ps1 %n| :%~$  %s }
+#
+#3) You can now add the following to ~/.zshrc and expect the
+#   characters to be displayed in color:
+#
+#  GIT_PS1_DESCRIBE_STYLE=branch
+#  GIT_PS1_SHOWUPSTREAM=auto
+#  

Re: Misusing git: trimming old commits

2013-05-11 Thread Martin Langhoff
On Thu, May 9, 2013 at 11:40 AM, Martin Langhoff
martin.langh...@gmail.com wrote:
 With the exaction of the final destination, I want to expire reports
 that are old and successfully transferred.

OK, that took some effort to make work. Make sure you are not using
reflogs (or that reflogs are promptly expired).

# right after a successful push of all heads to the receiving server...
for head in $(git branch|sed 's/^..//'); do
# FIXME period
graft_sha1=$(git log --until=one.month.ago -n1 --pretty=format:%H ${head})
if [[ -z $graft_sha1 ]]; then
# nothing to prune
continue
fi
# is it already grafted?
if grep -q ^${graft_sha1}  ${GIT_DIR}/info/grafts /dev/null ; then
# don't duplicate the graft
continue
fi
some_grafted='true'
# prep empty commit
# skip git read-tree --empty, we get the same with
export GIT_INDEX_FILE=/tmp/ppg-emptytree.$$
empty_tree=$(git write-tree)
rm ${GIT_INDEX_FILE}
unset GIT_INDEX_FILE
empty_commit=$(git commit-tree -m empty ${empty_tree})
echo ${graft_sha1} ${empty_commit}  ${GIT_DIR}/info/grafts
done

if [[ -z $some_grafted ]]; then
# nothing to do
exit 0
fi

# ppg-repack makes the unreachable objects loose
# (it is git-repack hacked to remove --keep-true-parents),
# git prune --expire actually deletes them
$PPG_EXEC_PATH/ppg-repack -AFfd
git prune --expire=now

### Cleanup stale grafts
# current grafts points are reachable,
# pruned graft points (made obsolete by a newer graft)
# cannot be retrieved and git cat-file exit code is 128
touch ${GIT_DIR}/info/grafts.tmp.$$
while read line; do
graftpoint=$(echo ${line} | cut -d' ' -f1)
if git cat-file commit ${graftpoint} /dev/null ; then
echo ${line}  ${GIT_DIR}/info/grafts.tmp.$$
fi
done  ${GIT_DIR}/info/grafts

if [ -s ${GIT_DIR}/info/grafts.tmp.$$ ]; then
mv ${GIT_DIR}/info/grafts.tmp.$$ ${GIT_DIR}/info/grafts
fi

Perhaps it helps someone else trying to run git as a spooler :-)

cheers,



m
--
 martin.langh...@gmail.com
 -  ask interesting questions
 - don't get distracted with shiny stuff  - working code first
 ~ http://docs.moodle.org/en/User:Martin_Langhoff
--
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: [PATCHv3 3/7] show: honor --textconv for blobs

2013-05-11 Thread Junio C Hamano
Michael J Gruber g...@drmicha.warpmail.net writes:

 +   if (!DIFF_OPT_TOUCHED(rev-diffopt, ALLOW_TEXTCONV) ||
 +   !DIFF_OPT_TST(rev-diffopt, ALLOW_TEXTCONV))
 +   return stream_blob_to_fd(1, sha1, NULL, 0);
 
 It is surprising that the necessary change is only this, but I think
 it is correct ;-).  We ignore textconv when the command line did not
 mention --[no-]textconv, or the command line said --no-textconv
 explicitly.
 
 This (especially the first condition) may deserve an in-code comment
 for anybody who wonders where this default behaviour is implemented.

 It's not as if we would document behavior by in-code comments in
 general, do we? The usual answer is git log -S or git blame.

The comment and the future reader I had in mind was more like

Default to --no-textconv, even though cmd_log_init_defaults()
sets the bit, when the user did not explicitly ask for it.

sought by somebody who wonders _where_ in the code we ignore
ALLOW_TEXTCONV that is set in cmd_log_init_defaults().

That is not something you can find with log -S or blame, is 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: [RFC/PATCH 0/2] merge-base: add --merge-child option

2013-05-11 Thread Junio C Hamano
John Keeping j...@keeping.me.uk writes:

 This is helpful when examining branches with disjoint roots, for example
 because one is periodically merged into a subtree of the other.

 With the --merge-child option, git merge-base will print a
 first-parent ancestor of the first revision given, where the commit
 printed is either a merge-base of the supplied revisions or a merge for
 which one of its parents (not the first) is a merge-base.

The above two doe snot connect at least to me.  The second paragraph
seems to describe how this mysterious mode decides its output to a
sufficient detail, but what the output _means_, and it is unclear
how it relates to gitk/git-gui style merges.

 For example, given the history:

 A---C---G
  \
 B-D---F
  \
   E

 we have:
 ...
 $ git log --left-right F...E --not $(git merge-base --merge-child F E)
  F
  E

 The git-log case is useful because it allows us to limit the range of
 commits that we are examining for patch-identical changes when using
 --cherry.

Hmph, is this reinventing ancestry-path in a different way?  At the
low level machinery, you are finding D to show only F and E, and
your goal seems to be to ignore the side ancestry A--C--G, but it is
not clear if you prefer E D F(which would be what F...E would give
in a history limited to ancestry-path, ignoring C) over E F.

 For example with git-gui in git.git I know that anything
 before the last merge of git-gui is not interesting:

Can this be extended to find the second last such merge?  Or is the
last one always special?

Still skeptical, but I'll let people discuss it during the feature
freeze ;-).
--
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: [PULL] git-svn updates for 1.8.3-rc2

2013-05-11 Thread Junio C Hamano
Thanks, pulled.
--
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: Cannot push anything via export transport helper after push fails.

2013-05-11 Thread Andrey Borzenkov
В Sat, 11 May 2013 08:57:14 -0500
Felipe Contreras felipe.contre...@gmail.com пишет:

 
  The problem seems to be that git fast-export updates marks
  unconditionally, whether export actually applied or not. So next time
  it assumes everything is already exported and does nothing.
 
  Is it expected behavior?
 
 Indeed, this is the way it currently works, and it's not easy to fix.
 We would need some way to make fast-export wait until we know the exit
 status of the remote helper, and then tell it when it failed, so the
 marks are not updated.
 

One possibility would be to omit *export-marks and manage GIT marks in
remote helper as well. Helper would then update synchronously both GIT
and BZR marks if no errors were detected. Or even better, it could
update just those commits that had been successful.

 However, the way remote-bzr/hg work is that the commits are still
 there anyway. So if you merge the next time you push those commits are
 already converted, so it's not a problem if fast-export is not
 exporting them again.
 

As I understand bzr commit ID is stable. What happens if we try to
commit the same ID second time?

 So even though it's not ideal, it should work.
 

I'm more concerned about transport errors. Any network glitch during
push renders you repository unusable (at least, without much efforts).

 The problem is when the remote-helper crashes and the marks of
 fast-export and the remote-helper are out of sync, and then the user
 is really screwed.
 

This case would benefit from moving processing of GIT marks into remote
helper as well.
--
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 0/2] merge-base: add --merge-child option

2013-05-11 Thread John Keeping
On Sat, May 11, 2013 at 10:54:12AM -0700, Junio C Hamano wrote:
 John Keeping j...@keeping.me.uk writes:
 
  This is helpful when examining branches with disjoint roots, for example
  because one is periodically merged into a subtree of the other.
 
  With the --merge-child option, git merge-base will print a
  first-parent ancestor of the first revision given, where the commit
  printed is either a merge-base of the supplied revisions or a merge for
  which one of its parents (not the first) is a merge-base.
 
 The above two doe snot connect at least to me.  The second paragraph
 seems to describe how this mysterious mode decides its output to a
 sufficient detail, but what the output _means_, and it is unclear
 how it relates to gitk/git-gui style merges.
 
  For example, given the history:
 
  A---C---G
   \
  B-D---F
   \
E
 
  we have:
  ...
  $ git log --left-right F...E --not $(git merge-base --merge-child F 
  E)
   F
   E
 
  The git-log case is useful because it allows us to limit the range of
  commits that we are examining for patch-identical changes when using
  --cherry.
 
 Hmph, is this reinventing ancestry-path in a different way?  At the
 low level machinery, you are finding D to show only F and E, and
 your goal seems to be to ignore the side ancestry A--C--G, but it is
 not clear if you prefer E D F(which would be what F...E would give
 in a history limited to ancestry-path, ignoring C) over E F.

I hadn't considered ancestry-path, but I don't think it does what I
want.  What I want if for LEFT to be B--D--F and RIGHT to be B--E,
ignoring A--C--G because I know that none of those are patch identical
to anything in B--E.

So what I want is more descendant-path than ancestry path in that I
don't want anything that isn't a descendant of the merge base of the
supplied arguments.

  For example with git-gui in git.git I know that anything
  before the last merge of git-gui is not interesting:
 
 Can this be extended to find the second last such merge?  Or is the
 last one always special?

In this implementation it only finds the last one because that's where
the merge base is.

 Still skeptical, but I'll let people discuss it during the feature
 freeze ;-).

I'm not convinced this is easy to explain myself, which may make it a
bad idea.  Perhaps a --descendant-path argument to git-log is a better
way to help with this case.
--
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] test-bzr: Do not use unportable sed \+

2013-05-11 Thread Junio C Hamano
Torsten Bögershausen tbo...@web.de writes:

 Using sed -e '/[0-9]\+//' to find one ore more digits
 is not portable.
 Use the Basic Regular Expression '/[0-9][0-9]*//' instead

 Signed-off-by: Torsten Bögershausen tbo...@web.de

Thanks.  Is there another one in t/t5551-http-fetch.sh that checks
the tags?

 ---
  contrib/remote-helpers/test-bzr.sh | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

 diff --git a/contrib/remote-helpers/test-bzr.sh 
 b/contrib/remote-helpers/test-bzr.sh
 index d9c32f4..5dfa070 100755
 --- a/contrib/remote-helpers/test-bzr.sh
 +++ b/contrib/remote-helpers/test-bzr.sh
 @@ -328,7 +328,7 @@ test_expect_success 'strip' '
  
echo four  content 
bzr commit -m four 
 -  bzr log --line | sed -e s/^[0-9]\+: //  ../expected
 +  bzr log --line | sed -e s/^[0-9][0-9]*: //  ../expected
) 
  
(cd gitrepo 
--
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: [ANN] git-tbdiff: topic branch diff

2013-05-11 Thread Junio C Hamano
Thomas Rast tr...@inf.ethz.ch writes:

 In this case the matching up is trivial, but you can see that it clearly
 shows the added Signoffs and edited parts of message and patch.

 Have fun, and let me know if it breaks!

Nice.

I need to do this kind of comparison quite often, when a rerolled
series is received.  Since there is no reason to re_base_ the series
with with an updated one on a different fork-point, I tend to do:

$ git checkout master...tr/topic
$ git am -s3c ./+tr-topic-2.mbox
... inspect the differences between tr/topic and HEAD ...
$ git branch -f tr/topic

where the first step finds the bottom boundary of the previous
iteration and detaches the HEAD at the commit, the second step
applies the rerolled series and the topic branch is updated in
the final step.

The inspect the differences step can be helped if I can use this
tool to say

$ git tbdiff master..tr/topic master..HEAD

It would probably be trivial to extend the command line parser to
also accept

$ git tbdiff tr/topic...

but that is an icing on the cake.
--
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


[ANN] Mestral – Hook management for Git

2013-05-11 Thread Sebastian Staudt
Hi,

today I'd like to introduce a new tool to manage Git hooks. It's called Mestral
and is available at GitHub: https://github.com/mestral/mestral

It is written in Ruby and inspired by Homebrew (http://brew.sh). It uses Git
itself to load and update hooks and aims to be easy to use for both users and
hook authors.

Currently, it's an early proof of concept and I'm looking for some early
adopters who want to try it, give valuable feedback and maybe contribute to the
development. That means any feedback is appreciated, may it be a short tweet to
@k0raktor, a reply to this mail, an issue on GitHub or a pull request.

Best regards,
Sebastian
--
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] test-bzr: Do not use unportable sed \+

2013-05-11 Thread Junio C Hamano
Junio C Hamano gits...@pobox.com writes:

 Thanks.  Is there another one in t/t5551-http-fetch.sh that checks
 the tags?

I think your sed will see the same breakage for the one in 5551 (my
sed is unfortunately GNU and .\+ does not break it).  Could you
test this patch with:

 GIT_TEST_LONG=YesPlease GIT_TEST_HTTPD=YesPlease \
 ./t5551-http-fetch.sh

Thanks.

 t/t5551-http-fetch.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/t5551-http-fetch.sh b/t/t5551-http-fetch.sh
index b23efbb..4a3184e 100755
--- a/t/t5551-http-fetch.sh
+++ b/t/t5551-http-fetch.sh
@@ -209,7 +209,7 @@ test_expect_success EXPENSIVE 'create 50,000 tags in the 
repo' '
 
# now assign tags to all the dangling commits we created above
tag=$($PERL_PATH -e print \bla\ x 30) 
-   sed -e s/^:\(.\+\) \(.\+\)$/\2 refs\/tags\/$tag-\1/ marks 
packed-refs
+   sed -e s|^:\([^ ]*\) \(.*\)$|\2 refs/tags/$tag-\1| marks 
packed-refs
)
 '
 
--
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] patch-ids.c: cache patch IDs in a notes tree

2013-05-11 Thread John Keeping
This adds a new configuration variable patchid.cacheRef which controls
whether (and where) patch IDs will be cached in a notes tree.

Caching patch IDs in this way results in a performance improvement in
every case I have tried, for example when comparing the git-gui tree to
git.git (where git-gui has been merged into git.git but most git.git
commits do not appear in git-gui):

Without patch ID caching:
$ time git log --cherry master...git-gui/master /dev/null
real0m32.981s
user0m32.364s
sys 0m0.621s

Prime the cache:
$ time git -c patchid.cacheref=patchids log --cherry \
master...git-gui/master /dev/null
real0m33.860s
user0m32.832s
sys 0m0.986s

With all patch IDs cached:
$ time git -c patchid.cacheref=patchids log --cherry \
master...git-gui/master /dev/null
real0m1.041s
user0m0.679s
sys 0m0.363s

Signed-off-by: John Keeping j...@keeping.me.uk
---
This is another approach to fixing the log --cherry takes a long time
issue I encountered comparing commits built on git-gui to those in
git.git [1][2].

I think this is a useful feature even outside that use case.  I measured
a small improvement (when the cache is primed) even comparing two
branches with 1 and 2 different commits respectively.

[1] http://article.gmane.org/gmane.comp.version-control.git/223959
[2] http://article.gmane.org/gmane.comp.version-control.git/223956

 Documentation/config.txt |  7 +++
 patch-ids.c  | 91 +++-
 patch-ids.h  |  1 +
 t/t6007-rev-list-cherry-pick-file.sh | 16 +++
 4 files changed, 114 insertions(+), 1 deletion(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 6e53fc5..e36585c 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1798,6 +1798,13 @@ pager.cmd::
precedence over this option.  To disable pagination for all
commands, set `core.pager` or `GIT_PAGER` to `cat`.
 
+patchid.cacheRef::
+   The name of a notes ref in which to store patch IDs for commits.
+   The ref is taken to be in `refs/notes/` if it is not qualified.
++
+Note that the patch ID notes are never pruned automatically, so you may
+wish to periodically run `git notes --ref ref prune` against this ref.
+
 pretty.name::
Alias for a --pretty= format string, as specified in
linkgit:git-log[1]. Any aliases defined here can be used just
diff --git a/patch-ids.c b/patch-ids.c
index bc8a28f..cb05eec 100644
--- a/patch-ids.c
+++ b/patch-ids.c
@@ -1,6 +1,9 @@
 #include cache.h
+#include blob.h
 #include diff.h
 #include commit.h
+#include notes.h
+#include refs.h
 #include sha1-lookup.h
 #include patch-ids.h
 
@@ -34,12 +37,38 @@ struct patch_id_bucket {
struct patch_id bucket[BUCKET_SIZE];
 };
 
+static int patch_id_config(const char *var, const char *value, void *cb)
+{
+   const char **cacheref = cb;
+
+   if (!strcmp(var, patchid.cacheref))
+   return git_config_string(cacheref, var, value);
+
+   return 0;
+}
+
 int init_patch_ids(struct patch_ids *ids)
 {
+   char *cacheref = NULL;
+
memset(ids, 0, sizeof(*ids));
diff_setup(ids-diffopts);
DIFF_OPT_SET(ids-diffopts, RECURSIVE);
diff_setup_done(ids-diffopts);
+
+   git_config(patch_id_config, cacheref);
+   if (cacheref) {
+   struct strbuf sb = STRBUF_INIT;
+   strbuf_addstr(sb, cacheref);
+   expand_notes_ref(sb);
+
+   ids-cache = xcalloc(1, sizeof(*ids-cache));
+   init_notes(ids-cache, sb.buf, combine_notes_overwrite, 0);
+
+   strbuf_release(sb);
+   free(cacheref);
+   }
+
return 0;
 }
 
@@ -52,9 +81,67 @@ int free_patch_ids(struct patch_ids *ids)
next = patches-next;
free(patches);
}
+
+   if (ids-cache) {
+   unsigned char notes_sha1[20];
+   if (write_notes_tree(ids-cache, notes_sha1) ||
+   update_ref(patch-id: update cache, ids-cache-ref,
+   notes_sha1, NULL, 0, QUIET_ON_ERR))
+   error(_(failed to write patch ID cache));
+
+   free_notes(ids-cache);
+   ids-cache = NULL;
+   }
+
return 0;
 }
 
+static int load_cached_patch_id(struct commit *commit,
+   struct notes_tree *cache, unsigned char *sha1)
+{
+   const unsigned char *note_sha1;
+   char *note;
+   enum object_type type;
+   unsigned long notelen;
+   int result = 1;
+
+   if (!cache)
+   return 1;
+
+   note_sha1 = get_note(cache, commit-object.sha1);
+   if (!note_sha1)
+   return 1;
+
+   if (!(note = read_sha1_file(note_sha1, type, notelen)) || !notelen ||
+   type != OBJ_BLOB)
+   goto out;
+
+   if (get_sha1_hex(note, sha1))
+   goto out;
+
+   

Re: [PATCH] test-bzr: Do not use unportable sed \+

2013-05-11 Thread Torsten Bögershausen
On 11.05.13 21:45, Junio C Hamano wrote:
 Junio C Hamano gits...@pobox.com writes:
 
 Thanks.  Is there another one in t/t5551-http-fetch.sh that checks
 the tags?
 
 I think your sed will see the same breakage for the one in 5551 (my
 sed is unfortunately GNU and .\+ does not break it).  Could you
 test this patch with:
 
  GIT_TEST_LONG=YesPlease GIT_TEST_HTTPD=YesPlease \
  ./t5551-http-fetch.sh
 
 Thanks.
 
  t/t5551-http-fetch.sh | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)
 
 diff --git a/t/t5551-http-fetch.sh b/t/t5551-http-fetch.sh
 index b23efbb..4a3184e 100755
 --- a/t/t5551-http-fetch.sh
 +++ b/t/t5551-http-fetch.sh
 @@ -209,7 +209,7 @@ test_expect_success EXPENSIVE 'create 50,000 tags in the 
 repo' '
  
   # now assign tags to all the dangling commits we created above
   tag=$($PERL_PATH -e print \bla\ x 30) 
 - sed -e s/^:\(.\+\) \(.\+\)$/\2 refs\/tags\/$tag-\1/ marks 
 packed-refs
 + sed -e s|^:\([^ ]*\) \(.*\)$|\2 refs/tags/$tag-\1| marks 
 packed-refs
   )

I did,
the interesting thing is that the test passes with and without your patch.
(After enabling  GIT_TEST_LONG and GIT_TEST_HTTPD in both cases)


Side note:
I added this line in in t/check-non-portable-shell.pl
  /sed[^]+[^]+\\([+])/ and err sed \\$1 is not portable);






--
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] test-bzr: Do not use unportable sed \+

2013-05-11 Thread Junio C Hamano
Torsten Bögershausen tbo...@web.de writes:

 I did,
 the interesting thing is that the test passes with and without your patch.
 (After enabling  GIT_TEST_LONG and GIT_TEST_HTTPD in both cases)

Strange.  Do you see differences between the produced packed-refs
file?
--
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] test-bzr: Do not use unportable sed \+

2013-05-11 Thread Torsten Bögershausen

On 05/11/2013 03:25 PM, Torsten Bögershausen wrote:
Sorry, I forgot to mention that there is another test case that fails
in test-bzr.sh (Both Linux and MacOS):

Cloning into 'gitrepo'...
--- ../expected 2013-05-11 20:07:17.678360248 +
+++ actual  2013-05-11 20:07:21.510312073 +
@@ -1,3 +1,3 @@
-origin/HEAD
+origin
 origin/branch
 origin/trunk
not ok 11 - proper bzr repo











--
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] test-bzr: Do not use unportable sed \+

2013-05-11 Thread Torsten Bögershausen
On 11.05.13 22:09, Junio C Hamano wrote:
 Torsten Bögershausen tbo...@web.de writes:
 
 I did,
 the interesting thing is that the test passes with and without your patch.
 (After enabling  GIT_TEST_LONG and GIT_TEST_HTTPD in both cases)
 
 Strange.  Do you see differences between the produced packed-refs
 file?

The original version seems to look like this:
:1 666527db455708922859283c673094002092910b
:2 1e2acf73c6db881cfb1d56d67662e3d9260be2cf
[snip]

The fixed POSIX version follows that style:
666527db455708922859283c673094002092910b 
refs/tags/blablablablablablablablablablablablablablablablablablablablablablablablablablablablablabla-1
1e2acf73c6db881cfb1d56d67662e3d9260be2cf 
refs/tags/blablablablablablablablablablablablablablablablablablablablablablablablablablablablablabla-2
[snip]

(Just to verify the changes:)
diff --git a/t/t5551-http-fetch.sh b/t/t5551-http-fetch.sh
index b23efbb..68965f0 100755
--- a/t/t5551-http-fetch.sh
+++ b/t/t5551-http-fetch.sh
@@ -210,8 +210,11 @@ test_expect_success EXPENSIVE 'create 50,000 tags in the 
repo' '
# now assign tags to all the dangling commits we created above
tag=$($PERL_PATH -e print \bla\ x 30) 
sed -e s/^:\(.\+\) \(.\+\)$/\2 refs\/tags\/$tag-\1/ marks 
packed-refs
+   sed -e s|^:\([^ ]*\) \(.*\)$|\2 refs/tags/$tag-\1| marks 
packed-refs.junio
+   sed -e s/^:\(.\+\) \(.\+\)$/\2 refs\/tags\/$tag-\1/ marks 
packed-refs.orig
)
 '
+   exit 0

--
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] patch-ids.c: cache patch IDs in a notes tree

2013-05-11 Thread Linus Torvalds
On Sat, May 11, 2013 at 12:54 PM, John Keeping j...@keeping.me.uk wrote:
 This adds a new configuration variable patchid.cacheRef which controls
 whether (and where) patch IDs will be cached in a notes tree.

Patch ID's aren't stable wrt different diff options, so I think this
is potentially a very bad idea.

   Linus
--
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: Cannot push anything via export transport helper after push fails.

2013-05-11 Thread Felipe Contreras
On Sat, May 11, 2013 at 1:48 PM, Andrey Borzenkov arvidj...@gmail.com wrote:
 В Sat, 11 May 2013 08:57:14 -0500
 Felipe Contreras felipe.contre...@gmail.com пишет:

 
  The problem seems to be that git fast-export updates marks
  unconditionally, whether export actually applied or not. So next time
  it assumes everything is already exported and does nothing.
 
  Is it expected behavior?

 Indeed, this is the way it currently works, and it's not easy to fix.
 We would need some way to make fast-export wait until we know the exit
 status of the remote helper, and then tell it when it failed, so the
 marks are not updated.


 One possibility would be to omit *export-marks and manage GIT marks in
 remote helper as well. Helper would then update synchronously both GIT
 and BZR marks if no errors were detected. Or even better, it could
 update just those commits that had been successful.

That would need to change the whole architecture, because right now
the remote helpers are agnostic of Git SHA-1s.

 However, the way remote-bzr/hg work is that the commits are still
 there anyway. So if you merge the next time you push those commits are
 already converted, so it's not a problem if fast-export is not
 exporting them again.


 As I understand bzr commit ID is stable. What happens if we try to
 commit the same ID second time?

It's skipped, because it's already converted.

 So even though it's not ideal, it should work.


 I'm more concerned about transport errors. Any network glitch during
 push renders you repository unusable (at least, without much efforts).

No, it doesn't. If the remote-helper fails gracefully, the bzr
revisions are converted and stored in the bzr repo, even if they were
not pushed to the remote. So it's OK if fast-export never exports them
again; we already have them.

Cheers.

-- 
Felipe Contreras
--
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] git-prompt.zsh: introduce thin ZSH wrapper

2013-05-11 Thread Felipe Contreras
On Sat, May 11, 2013 at 11:25 AM, Ramkumar Ramachandra
artag...@gmail.com wrote:
 To facilitate a colored prompt in ZSH, write a thin wrapper around
 git-prompt.sh, factoring out and overriding the coloring logic.  Since
 ZSH lacks a PROMPT_COMMAND, instruct the user to execute __git_ps1
 inside precmd().

I think this is two logical changes into one.

 --- /dev/null
 +++ b/contrib/completion/git-prompt.zsh
 @@ -0,0 +1,59 @@
 +# git prompt support for zsh: wrapper around git-prompt.sh
 +#
 +# To enable:
 +#
 +#1) Copy this file and git-prompt.sh to ~/.zsh/prompt
 +#2) Add the following lines to your .zshrc:
 +#
 +#  source ~/.zsh/prompt/git-prompt.zsh
 +#  GIT_PS1_SHOWCOLORHINTS=true
 +#  precmd () { __git_ps1 %n| :%~$  %s }
 +#
 +#3) You can now add the following to ~/.zshrc and expect the
 +#   characters to be displayed in color:
 +#
 +#  GIT_PS1_DESCRIBE_STYLE=branch
 +#  GIT_PS1_SHOWUPSTREAM=auto
 +#  GIT_PS1_SHOWDIRTYSTATE=true
 +#  GIT_PS1_SHOWUNTRACKEDFILES=true
 +
 +test -z $script  script=$(dirname 
 ${funcsourcetrace[1]%:*})/git-prompt.sh
 +ZSH_VERSION='' . $script

I've been thinking that this method of loading the script is not the
best; we should probably have a list of locations where distributions
usually put this file. So if we could avoid the creation of of a new
file, that would be great.

 +autoload colors
 +colors
 +
 +__git_ps1_colorize_gitstring ()
 +{
 +   local c_red='%F{red}'
 +   local c_green='%F{green}'
 +   local c_lblue='%F{blue}'
 +   local c_clear='%f'
 +   local bad_color=$c_red
 +   local ok_color=$c_green
 +   local branch_color=$c_clear
 +   local flags_color=$c_lblue
 +   local branchstring=$c${b##refs/heads/}

This is the only real difference with bash colors, no? I think it's
overkill to create a new file, and load the old one, only to override
part of a function. We should probably start with everything in the
same file, and only later if we find it's necessary, split.

 +   if [ $detached = no ]; then
 +   branch_color=$ok_color
 +   else
 +   branch_color=$bad_color
 +   fi
 +
 +   gitstring=$branch_color$branchstring$c_clear
 +
 +   if [ $w = * ]; then
 +   gitstring=$gitstring$bad_color$w
 +   fi
 +   if [ -n $i ]; then
 +   gitstring=$gitstring$ok_color$i
 +   fi
 +   if [ -n $s ]; then
 +   gitstring=$gitstring$flags_color$s
 +   fi
 +   if [ -n $u ]; then
 +   gitstring=$gitstring$bad_color$u
 +   fi
 +   gitstring=$gitstring$c_clear$r$p
 +}
 --

-- 
Felipe Contreras
--
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] patch-ids.c: cache patch IDs in a notes tree

2013-05-11 Thread John Keeping
On Sat, May 11, 2013 at 02:10:01PM -0700, Linus Torvalds wrote:
 On Sat, May 11, 2013 at 12:54 PM, John Keeping j...@keeping.me.uk wrote:
  This adds a new configuration variable patchid.cacheRef which controls
  whether (and where) patch IDs will be cached in a notes tree.
 
 Patch ID's aren't stable wrt different diff options, so I think this
 is potentially a very bad idea.

Hmm... I hadn't realised that.  Looking a bit closer, it looks like
init_patch_ids sets up its own diffopts so its not affected by the
command line (except for pathspecs which would be easy to check for).
Of course that still means it can be affected by settings in the user's
configuration.

It's a pity that this can't be done since it gives a significant
performance improvement for some tasks that I perform relatively
frequently.  Is there a reason patch IDs couldn't be generated using
fixed diff options?  Since there's no way to control it from the command
line it seems surprising that the results of log --cherry might be
different based on what's in your config.

That could go either way I suppose - is it useful to be able to change
patch IDs based on command line arguments or is it wrong that as we add
persistent diff settings to the configuration we've been silently
changing the behaviour of git patch-id and git cherry.
--
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] git-prompt.sh: colorize ZSH prompt

2013-05-11 Thread Ramkumar Ramachandra
Add colors suitable for use in the ZSH prompt.  Having learnt that the
ZSH equivalent of PROMPT_COMMAND is precmd (), you can now use
GIT_PS1_SHOWCOLORHINTS with ZSH.

Signed-off-by: Ramkumar Ramachandra artag...@gmail.com
---
 You like this more?  I don't mind going either way.

 contrib/completion/git-prompt.sh | 67 
 1 file changed, 47 insertions(+), 20 deletions(-)

diff --git a/contrib/completion/git-prompt.sh b/contrib/completion/git-prompt.sh
index 08c9b22..26e5bc2 100644
--- a/contrib/completion/git-prompt.sh
+++ b/contrib/completion/git-prompt.sh
@@ -20,7 +20,8 @@
 #post, which are strings you would put in $PS1 before
 #and after the status string generated by the git-prompt
 #machinery.  e.g.
-#   PROMPT_COMMAND='__git_ps1 \u@\h:\w \\\$ '
+#Bash: PROMPT_COMMAND='__git_ps1 \u@\h:\w \\\$ '
+#ZSH: precmd () { __git_ps1 %n :%~$  |%s }
 #will show username, at-sign, host, colon, cwd, then
 #various status string, followed by dollar and SP, as
 #your prompt.
@@ -363,10 +364,18 @@ __git_ps1 ()
if [ $pcmode = yes ]; then
local gitstring=
if [ -n ${GIT_PS1_SHOWCOLORHINTS-} ]; then
-   local c_red='\e[31m'
-   local c_green='\e[32m'
-   local c_lblue='\e[1;34m'
-   local c_clear='\e[0m'
+   if [[ -n ${ZSH_VERSION-} ]]; then
+   local c_red='%F{red}'
+   local c_green='%F{green}'
+   local c_lblue='%F{blue}'
+   local c_clear='%f'
+   else
+   local c_red='\e[31m'
+   local c_green='\e[32m'
+   local c_lblue='\e[1;34m'
+   local c_clear='\e[0m'
+   fi
+
local bad_color=$c_red
local ok_color=$c_green
local branch_color=$c_clear
@@ -379,23 +388,41 @@ __git_ps1 ()
branch_color=$bad_color
fi
 
-   # Setting gitstring directly with \[ and \] 
around colors
-   # is necessary to prevent wrapping issues!
-   
gitstring=\[$branch_color\]$branchstring\[$c_clear\]
+   if [[ -n ${ZSH_VERSION-} ]]; then
+   
gitstring=$branch_color$branchstring$c_clear
 
-   if [ $w = * ]; then
-   gitstring=$gitstring\[$bad_color\]$w
-   fi
-   if [ -n $i ]; then
-   gitstring=$gitstring\[$ok_color\]$i
-   fi
-   if [ -n $s ]; then
-   gitstring=$gitstring\[$flags_color\]$s
-   fi
-   if [ -n $u ]; then
-   gitstring=$gitstring\[$bad_color\]$u
+   if [ $w = * ]; then
+   
gitstring=$gitstring$bad_color$w
+   fi
+   if [ -n $i ]; then
+   
gitstring=$gitstring$ok_color$i
+   fi
+   if [ -n $s ]; then
+   
gitstring=$gitstring$flags_color$s
+   fi
+   if [ -n $u ]; then
+   
gitstring=$gitstring$bad_color$u
+   fi
+   gitstring=$gitstring$c_clear$r$p
+   else
+   # Setting gitstring directly with \[ 
and \] around colors
+   # is necessary to prevent wrapping 
issues!
+   
gitstring=\[$branch_color\]$branchstring\[$c_clear\]
+
+   if [ $w = * ]; then
+   
gitstring=$gitstring\[$bad_color\]$w
+   fi
+   if [ -n $i ]; then
+   
gitstring=$gitstring\[$ok_color\]$i
+   fi
+  

Re: [PATCH v2] git-prompt.sh: colorize ZSH prompt

2013-05-11 Thread Felipe Contreras
On Sat, May 11, 2013 at 5:18 PM, Ramkumar Ramachandra
artag...@gmail.com wrote:
 Add colors suitable for use in the ZSH prompt.  Having learnt that the
 ZSH equivalent of PROMPT_COMMAND is precmd (), you can now use
 GIT_PS1_SHOWCOLORHINTS with ZSH.

 Signed-off-by: Ramkumar Ramachandra artag...@gmail.com
 ---
  You like this more?  I don't mind going either way.

Not really. If we need to avoid the \[\], it makes sense to have a
separate function, but what I meant is that this function should be
initially on the same file, and created in a separate patch.

-- 
Felipe Contreras
--
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] git-prompt.sh: colorize ZSH prompt

2013-05-11 Thread Ramkumar Ramachandra
Felipe Contreras wrote:
 Not really. If we need to avoid the \[\], it makes sense to have a
 separate function, but what I meant is that this function should be
 initially on the same file, and created in a separate patch.

What are you saying?

1. It makes sense to have a separate function specifically for ZSH,
but it should be located in the same physical file (why?)
2. You like the original patch, but would prefer two different parts (a nit)
--
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] patch-ids.c: cache patch IDs in a notes tree

2013-05-11 Thread Linus Torvalds
On Sat, May 11, 2013 at 2:49 PM, John Keeping j...@keeping.me.uk wrote:

 Hmm... I hadn't realised that.  Looking a bit closer, it looks like
 init_patch_ids sets up its own diffopts so its not affected by the
 command line (except for pathspecs which would be easy to check for).
 Of course that still means it can be affected by settings in the user's
 configuration.

.. and in the actual diff algorithm.

The thing is - patches ARE NOT STABLE. There are many valid ways to
get a patch from one version to another, and even without command line
changes, we've had different versions of git generating different
patches. There are heuristics in xdiff to avoid some nasty use up
tons of CPU-time things that have been tweaked over time. And even
for simple cases there are ambiguous ways to describe the patch.

Now, maybe we don't care, because in practice, most of the time this
just doesn't much matter. And anybody who uses patch-ID's had better
not rely on them being guaranteed to be unique anyway, so it's more of
a if the patch ID is the same, it's almost guaranteed to be the same
patch, but that's a big almost. And no, it's not some theoretical
SHA1 collisions are very unlikely kind of thing, it's a the patch
ID actually ignores a lot of data in order to give the same ID even if
lins have been added above it, and the patch is at different line
numbers etc.

So maybe it doesn't matter. But at the same time, I really think
caching patch ID's should be something people should be aware of is
fundamentally wrong, even if it might work.

And quite frankly, if you do rebases etc so much that you think patch
ID's are so important that they need to be cached, you may be doing
odd/wrong things.

 Linus
--
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] git-prompt.sh: colorize ZSH prompt

2013-05-11 Thread Felipe Contreras
On Sat, May 11, 2013 at 5:40 PM, Ramkumar Ramachandra
artag...@gmail.com wrote:
 Felipe Contreras wrote:
 Not really. If we need to avoid the \[\], it makes sense to have a
 separate function, but what I meant is that this function should be
 initially on the same file, and created in a separate patch.

 What are you saying?

 1. It makes sense to have a separate function specifically for ZSH,
 but it should be located in the same physical file (why?)

This is what I mean:

commit 4726a5f76ec7d96c34863d6640488e2183cc8f00
Author: Ramkumar Ramachandra artag...@gmail.com
Date:   Sat May 11 21:55:13 2013 +0530

prompt: split code into __git_ps1_colorize_gitstring()

Will be useful for reorganizations.

Signed-off-by: Felipe Contreras felipe.contre...@gmail.com

diff --git a/contrib/completion/git-prompt.sh b/contrib/completion/git-prompt.sh
index eaf5c36..1187292 100644
--- a/contrib/completion/git-prompt.sh
+++ b/contrib/completion/git-prompt.sh
@@ -222,6 +222,45 @@ __git_ps1_show_upstream ()

 }

+__git_ps1_colorize_gitstring ()
+{
+   local c_red='\e[31m'
+   local c_green='\e[32m'
+   local c_lblue='\e[1;34m'
+   local c_clear='\e[0m'
+   local bad_color=$c_red
+   local ok_color=$c_green
+   local branch_color=$c_clear
+   local flags_color=$c_lblue
+   local branchstring=$c${b##refs/heads/}
+
+   if [ $detached = no ]; then
+   branch_color=$ok_color
+   else
+   branch_color=$bad_color
+   fi
+
+   # Setting gitstring directly with \[ and \] around colors
+   # is necessary to prevent wrapping issues!
+   gitstring=\[$branch_color\]$branchstring\[$c_clear\]
+
+   if [ -n $w$i$s$u$r$p ]; then
+   gitstring=$gitstring 
+   fi
+   if [ $w = * ]; then
+   gitstring=$gitstring\[$bad_color\]$w
+   fi
+   if [ -n $i ]; then
+   gitstring=$gitstring\[$ok_color\]$i
+   fi
+   if [ -n $s ]; then
+   gitstring=$gitstring\[$flags_color\]$s
+   fi
+   if [ -n $u ]; then
+   gitstring=$gitstring\[$bad_color\]$u
+   fi
+   gitstring=$gitstring\[$c_clear\]$r$p
+}

 # __git_ps1 accepts 0 or 1 arguments (i.e., format string)
 # when called from PS1 using command substitution
@@ -363,42 +402,7 @@ __git_ps1 ()
if [ $pcmode = yes ]; then
local gitstring=
if [ -n ${GIT_PS1_SHOWCOLORHINTS-} ]; then
-   local c_red='\e[31m'
-   local c_green='\e[32m'
-   local c_lblue='\e[1;34m'
-   local c_clear='\e[0m'
-   local bad_color=$c_red
-   local ok_color=$c_green
-   local branch_color=$c_clear
-   local flags_color=$c_lblue
-   local branchstring=$c${b##refs/heads/}
-
-   if [ $detached = no ]; then
-   branch_color=$ok_color
-   else
-   branch_color=$bad_color
-   fi
-
-   # Setting gitstring directly with \[ and \] 
around colors
-   # is necessary to prevent wrapping issues!
-   
gitstring=\[$branch_color\]$branchstring\[$c_clear\]
-
-   if [ -n $w$i$s$u$r$p ]; then
-   gitstring=$gitstring 
-   fi
-   if [ $w = * ]; then
-   gitstring=$gitstring\[$bad_color\]$w
-   fi
-   if [ -n $i ]; then
-   gitstring=$gitstring\[$ok_color\]$i
-   fi
-   if [ -n $s ]; then
-   gitstring=$gitstring\[$flags_color\]$s
-   fi
-   if [ -n $u ]; then
-   gitstring=$gitstring\[$bad_color\]$u
-   fi
-   gitstring=$gitstring\[$c_clear\]$r$p
+   __git_ps1_colorize_gitstring
else
gitstring=$c${b##refs/heads/}${f:+ $f}$r$p
fi

commit 23d89b370ac36401da1d9a98754e222a7a4c93e5
Author: Felipe Contreras felipe.contre...@gmail.com
Date:   Sat May 11 18:03:39 2013 -0500

prompt: add colorization for zsh

Signed-off-by: Felipe Contreras felipe.contre...@gmail.com

diff --git a/contrib/completion/git-prompt.sh b/contrib/completion/git-prompt.sh
index 1187292..8946a29 100644
--- a/contrib/completion/git-prompt.sh
+++ 

Re: [PATCH 1/8] am: suppress error output from a conditional

2013-05-11 Thread Ramkumar Ramachandra
Junio C Hamano wrote:
 If you are introducing dotest exists but next/last may not be
 present as a valid new state, it probably should check the presence
 and/or absence of them explicitly,

Um, a test -f preceding the cat?  Why, when cat implicitly checks
existence anyway?

 but more importantly, it is a
 good idea to move test $# != 0 higher.

This?

diff --git a/git-am.sh b/git-am.sh
index 47c1021..1e10e31 100755
--- a/git-am.sh
+++ b/git-am.sh
@@ -446,9 +446,9 @@ done
 # If the dotest directory exists, but we have finished applying all the
 # patches in them, clear it out.
 if test -d $dotest 
+   test $# != 0 
last=$(cat $dotest/last 2/dev/null) 
next=$(cat $dotest/next 2/dev/null) 
-   test $# != 0 
test $next -gt $last
 then
rm -fr $dotest

 Earlier it did not matter
 because when $dotest existed, next/last were supposed to be there
 and absence of them was an error codepath. Now, missing these files
 is not an error but is a perfectly normal state,

It's not a normal state for the purposes of git-am.sh.  There is no
codepath in the program that depends only on $dotest, but not
next/last.  I would frame this as: checking for $dotest is not a
sufficient prerequisite anymore; we have to additionally look for
next/last.  To git-am.sh, an empty $dotest or just a
$dotest/autostash is equivalent to no $dotest at all.

 so checking what
 can be checked more cheaply makes sense as a general code hygiene.

Yeah, I agree.  See am: tighten a conditional that checks for
$dotest, where I get away with just checking $dotest/last (and assume
that $dotest/next is present by extension).  How do I apply your
suggestion to this particular patch?
--
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] patch-ids.c: cache patch IDs in a notes tree

2013-05-11 Thread Johannes Schindelin
Hi John  Linus,

On Sat, 11 May 2013, Linus Torvalds wrote:

 [...] I really think caching patch ID's should be something people
 should be aware of is fundamentally wrong, even if it might work.

Given the incredible performance win, I would say it is still worth
looking into.

If you store also a hash of Git version and diff options (may even be the
hash of the raw bytes of the diff options if you do not plan to share the
ref between machines) with the patch ID, you can make it safe.

That hash would be generated at patch_id init time and
load_cached_patch_id() would check this hash in addition to the return
value of get_sha1() (and ignore the note if the version/diff options
differ).

If you are following git.git slavishly, maybe hashing just the major/minor
Git version would be in order to avoid frequent regeneration of identical
patch IDs.

 And quite frankly, if you do rebases etc so much that you think patch
 ID's are so important that they need to be cached, you may be doing
 odd/wrong things.

AFAICT John actually gave a very valid scenario that validates his use
case: git-gui patches are best tested in the git.git scenario but have to
be contributed via git-gui.git. It's not John's fault that this typically
requires a lot of rebasing between vastly divergent histories.

John, do you think you can incorporate that finger-print of the patch ID
generation and store it in the same note?

Ciao,
Johannes
--
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] git-gui: fix file name handling with non-empty prefix

2013-05-11 Thread Andrew Wong
Sorry for the late reply. I was able to reproduce the problem that you
were describing a while ago. And your patch indeed fixes it. It's a much
more elegant way of dealing with the absolute vs relative path problem
that I was trying to fix.

Thanks!

As for Pat, I'm not sure wha'ts going on with his email address. It was
working back in October, and his username still seems to be active over
at SourceForge... let's see if this email reaches him.

Here's a link for his reference just in case he missed your original email:
http://thread.gmane.org/gmane.comp.version-control.git/222646


On 04/27/13 10:18, John Keeping wrote:
 I got a bounce with 550 no such user for Pat's email address when
 sending this.  Does anyone have more up-to-date contact details?  Or is
 it just SourceForge being broken?

 On Sat, Apr 27, 2013 at 02:24:16PM +0100, John Keeping wrote:
 Commit e3d06ca (git-gui: Detect full path when parsing arguments -
 2012-10-02) fixed the handling of absolute paths passed to the browser
 and blame subcommands by checking whether the file exists without the
 prefix before prepending the prefix and checking again.  Since we have
 chdir'd to the top level of the working tree before doing this, this
 does not work if a file with the same name exists in a subdirectory and
 at the top level (for example Makefile in git.git's t/ directory).

 Instead of doing this, revert that patch and fix absolute path issue by
 using file join to prepend the prefix to the supplied path.  This will
 correctly handle absolute paths by skipping the prefix in that case.

 Signed-off-by: John Keeping j...@keeping.me.uk
 ---
  git-gui.sh | 14 +++---
  1 file changed, 3 insertions(+), 11 deletions(-)

 diff --git a/git-gui.sh b/git-gui.sh
 index e11..a94ad7f 100755
 --- a/git-gui.sh
 +++ b/git-gui.sh
 @@ -3003,19 +3003,11 @@ blame {
  set jump_spec {}
  set is_path 0
  foreach a $argv {
 -if {[file exists $a]} {
 -if {$path ne {}} usage
 -set path [normalize_relpath $a]
 -break
 -} elseif {[file exists $_prefix$a]} {
 -if {$path ne {}} usage
 -set path [normalize_relpath $_prefix$a]
 -break
 -}
 +set p [file join $_prefix $a]
  
 -if {$is_path} {
 +if {$is_path || [file exists $p]} {
  if {$path ne {}} usage
 -set path [normalize_relpath $_prefix$a]
 +set path [normalize_relpath $p]
  break
  } elseif {$a eq {--}} {
  if {$path ne {}} {
 -- 
 1.8.3.rc0.149.g98a72f2.dirty

--
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] patch-ids.c: cache patch IDs in a notes tree

2013-05-11 Thread Junio C Hamano
Linus Torvalds torva...@linux-foundation.org writes:

 On Sat, May 11, 2013 at 2:49 PM, John Keeping j...@keeping.me.uk wrote:

 Hmm... I hadn't realised that.  Looking a bit closer, it looks like
 init_patch_ids sets up its own diffopts so its not affected by the
 command line (except for pathspecs which would be easy to check for).
 Of course that still means it can be affected by settings in the user's
 configuration.

 .. and in the actual diff algorithm.

As to the objection side of the argument, I already said
essentially the same thing several months ago:

  http://thread.gmane.org/gmane.comp.version-control.git/202654/focus=202898

and do not have much to add [*1*].

However.

The use of patch-id in cherry and rebase is to facilitate avoiding
to replay commits that are obviously identical to the ones you have
in your history.  The cached patch id for an existing old commit may
differ from a patch id you freshly compute for a new commit you are
trying to see if it truly new, even though they may represent the
same change.  So we may incorrectly think such a new commit is not
yet in your history and attempt to replay it.

But it is not a big problem.  Either 3-way merge notices that there
is nothing new, or you get a conflict and have chance to inspect
what is going on.

A conceptually much larger and more problematic issue is that we may
discard a truly new change that you still need as an old one you
already have due to a hash collision and discard it.  Because the
hash space of SHA-1 is so large, however, it is not a problem in
practice, and more importantly, that hash space is just as large as
the hash space used by Git to reduce a patch to a patch id, the
filtering done with patch-id in cherry and rebase _already_ have
that exact problem with or without this additional cache layer. A
stale cache may make the possibility of lost change due to such a
hash collision merely twice as likely.

 ... it's a the patch ID actually ignores a lot of data in order
 to give the same ID even if lins have been added above it, and the
 patch is at different line numbers etc.

Yes.

 So maybe it doesn't matter. But at the same time, I really think
 caching patch ID's should be something people should be aware of is
 fundamentally wrong, even if it might work.

I do not think it is caching patch ID that people should be aware
of is fundamentally wrong.  What is fundamentally wrong, even if it
might work, is using patch ID itself.

 And quite frankly, if you do rebases etc so much that you think patch
 ID's are so important that they need to be cached, you may be doing
 odd/wrong things.

And that, too ;-)


[Footnote]

*1* For people listening from the sidelines, the fact that Git
algorithm can improve over time is a real issue, and and has caused
one issue that still hasn't been solved in the k.org upload process.
Somebody who has a repository there could *theoretically*:

 - push her v1.1 release via Git (git push origin v1.1);
 - create a tarball (git archive -o v1.1.tar v1.1) and diff since the
   last release (git diff v1.0 v1.1 v1.0-v1.1.diff) locally;
 - GPG sign them (gpg -b v1.1.tar, gpg -b v1.0-v1.1.diff); and
 - upload only the signature files

and have k.org create the tarballs and diff to save bandwidth of
uploading logically derivable stuff over and over again.  But that
can be done only when output from git archive and git diff are
stable, which is not the case.  We changed how extended header fields
are used in the tar archive for a long pathname recently, and also
we changed use of XDF_NEED_MINIMAL a couple of years ago in git diff;
both of these affect the output.
--
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 1/8] am: suppress error output from a conditional

2013-05-11 Thread Junio C Hamano
Ramkumar Ramachandra artag...@gmail.com writes:

 Junio C Hamano wrote:
 If you are introducing dotest exists but next/last may not be
 present as a valid new state, it probably should check the presence
 and/or absence of them explicitly,

 Um, a test -f preceding the cat?  Why, when cat implicitly checks
 existence anyway?

With that logic, 'test -d $dotest' on the first line would also be
redundant, as we read from $dotest/last and that will catch an
error.

It is more about what is conceptually right.  You need to think what
the condition _means_.

We protect the remove $dotest directory with the precondition you
are changing, but what is that precondition really trying to say?
If $dotest does not exist, obviously we do not need to remove it,
but what is the essence of that sereis of tests?

It is what the comment says:

We have finished applying all the patches in them

Earlier, presense of $dotest _must_ have meant that last and next
should exist (otherwise you have a corrupt state and we did want to
see error messages), and the check original did was perfectly fine.

Now, a mere presense of $dotest does _not_ mean we have finished
applying all the patches, because sometimes you create it without
having anything to put in last or next yet.  That is why it starts
to make sense to do

if test -d $dotest 
   test -f $dotest/last 
   test -f $dotest/next 
   last=$(cat $dotest/last) 
   ...

--
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] test-bzr: Do not use unportable sed \+

2013-05-11 Thread Junio C Hamano
Torsten Bögershausen tbo...@web.de writes:

 On 11.05.13 22:09, Junio C Hamano wrote:
 Torsten Bögershausen tbo...@web.de writes:
 
 I did,
 the interesting thing is that the test passes with and without your patch.
 (After enabling  GIT_TEST_LONG and GIT_TEST_HTTPD in both cases)
 
 Strange.  Do you see differences between the produced packed-refs
 file?

 The original version seems to look like this:
 :1 666527db455708922859283c673094002092910b
 :2 1e2acf73c6db881cfb1d56d67662e3d9260be2cf
 [snip]

 The fixed POSIX version follows that style:
 666527db455708922859283c673094002092910b 
 refs/tags/blablablablablablablablablablablablablablablablablablablablablablablablablablablablablabla-1
 1e2acf73c6db881cfb1d56d67662e3d9260be2cf 
 refs/tags/blablablablablablablablablablablablablablablablablablablablablablablablablablablablablabla-2
 [snip]

That means whatever the test that comes after that set-up step to
populate the packed-refs file does not need the packed-refs file.

In fact, the test only sees if clone succeeds, without checking
what refs are present in the resulting repository.  The original
repository lacks these 5 tags due to the non-portable sed
construct (it has a corrupt packed-refs file) and it may or may not
have other refs, but because cloning an empty repository does not
error out these days, the test just passes.

Sloppy.


--
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