Re: [PATCH] urlmatch: use hex2chr() in append_normalized_escapes()

[Kyle J. McKay]

On Jul 8, 2017, at 01:59, René Scharfe wrote:


Simplify the code by using hex2chr() to convert and check for invalid
characters at the same time instead of doing that sequentially with
one table lookup for each.


I think that comment may be a bit misleading as the changes are just
switching from one set of inlines to another.  Essentially the same
sequential check takes place in the hex2chr inlined function which is
being used to replace the "one table lookup for each".  An optimizing
compiler will likely eliminate any difference between the before and
after patch versions.  Nothing immediately comes to mind as an alternate
comment though, so I'm not proposing any changes to the comment.

The before version only requires knowledge of the standards-defined  
isxdigit
and the hexval function which is Git-specific, but its semantics are  
fairly

obvious from the surrounding code.

I suspect the casual reader of the function will have to go check and  
see

what hex2chr does exactly.  For example, does it accept "0x41" or not?
(It doesn't.)  What does it do with a single hex digit? (An error.)
It does do pretty much the same thing as the code it's
replacing (although that's not immediately obvious unless you go look
at it), so this seems like a reasonable change.

From the perspective of how many characters the original is versus how
many characters the replacement is, it's certainly a simplification.

But from the perspective of a reviewer of the urlmatch functionality
attempting to determine how well the code does or does not match the
respective standards it requires more work.  Now one must examine the
hex2chr function to be certain it doesn't include any extra unwanted
behavior with regards to how well urlmatch complies with the applicable
standards.  And in that sense it is not a simplification at all.

But that's all really just nit picking since hex2chr is a simple
inlined function that's relatively easy to find (and understand).

Therefore I don't have any objections to this change.

Acked-by: Kyle J. McKay


Signed-off-by: Rene Scharfe <l@web.de>
---
urlmatch.c | 10 +-
1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/urlmatch.c b/urlmatch.c
index 4bbde924e8..3e42bd7504 100644
--- a/urlmatch.c
+++ b/urlmatch.c
@@ -42,12 +42,12 @@ static int append_normalized_escapes(struct  
strbuf *buf,


from_len--;
if (ch == '%') {
-   if (from_len < 2 ||
-   !isxdigit(from[0]) ||
-   !isxdigit(from[1]))
+   if (from_len < 2)
return 0;
-   ch = hexval(*from++) << 4;
-   ch |= hexval(*from++);
+   ch = hex2chr(from);
+   if (ch < 0)
+   return 0;
+   from += 2;
from_len -= 2;
was_esc = 1;
}

[PATCH] [BUG] test_expect_failure mailinfo -b fatals

[Kyle J. McKay]

This subject line:

> Subject: [other] [PATCH] message

does not get along with the git mailinfo "-b" option.  In fact,
it triggers a fatal bug:

> fatal: `pos + len' is too far after the end of the buffer

While I did not do any exhaustive checking, I do happen to have
a few builds of various versions of Git lying around and it
fails at least as far back as v1.7.6.  (The -b option was
introduced in v1.6.6 I believe.)

At the very least this is now a "known breakage", so might
as well have the tests for it.

If someone comes along and fixes it it's a simple matter
to flip them to test_expect_success instead.

--Kyle

P.S. Oh yes, the real patch subject is below (love those >8).

-- 8< --
Subject: [PATCH] t5100: add some more mailinfo tests

Add some more simple mailinfo tests including a few that
produce:

  fatal: `pos + len' is too far after the end of the buffer

Mark those as 'test_expect_failure'.

Signed-off-by: Kyle J. McKay <mack...@gmail.com>
---

Notes:
checking known breakage:
subj="$(echo "Subject: [other] [PATCH] message" |
git mailinfo -b /dev/null /dev/null)" &&
test z"$subj" = z"Subject: [other] message"

fatal: `pos + len' is too far after the end of the buffer
not ok 46 - mailinfo -b trailing [PATCH] # TODO known breakage

checking known breakage:
subj="$(echo "Subject: [PATCH] [other] [PATCH] message" |
git mailinfo -b /dev/null /dev/null)" &&
test z"$subj" = z"Subject: [other] message"

fatal: `pos + len' is too far after the end of the buffer
not ok 47 - mailinfo -b separated double [PATCH] # TODO known breakage

 t/t5100-mailinfo.sh | 41 +
 1 file changed, 41 insertions(+)

diff --git a/t/t5100-mailinfo.sh b/t/t5100-mailinfo.sh
index 7171f675..95c8 100755
--- a/t/t5100-mailinfo.sh
+++ b/t/t5100-mailinfo.sh
@@ -170,5 +170,46 @@ test_expect_success 'mailinfo with mailinfo.scissors 
config' '
test_cmp "$DATA/info0014--scissors" info0014.sc
 '
 
+test_expect_success 'mailinfo no options' '
+   subj="$(echo "Subject: [PATCH] [other] [PATCH] message" |
+   git mailinfo /dev/null /dev/null)" &&
+   test z"$subj" = z"Subject: message"
+'
+
+test_expect_success 'mailinfo -k' '
+   subj="$(echo "Subject: [PATCH] [other] [PATCH] message" |
+   git mailinfo -k /dev/null /dev/null)" &&
+   test z"$subj" = z"Subject: [PATCH] [other] [PATCH] message"
+'
+
+test_expect_success 'mailinfo -b no [PATCH]' '
+   subj="$(echo "Subject: [other] message" |
+   git mailinfo -b /dev/null /dev/null)" &&
+   test z"$subj" = z"Subject: [other] message"
+'
+
+test_expect_success 'mailinfo -b leading [PATCH]' '
+   subj="$(echo "Subject: [PATCH] [other] message" |
+   git mailinfo -b /dev/null /dev/null)" &&
+   test z"$subj" = z"Subject: [other] message"
+'
+
+test_expect_success 'mailinfo -b double [PATCH]' '
+   subj="$(echo "Subject: [PATCH] [PATCH] message" |
+   git mailinfo -b /dev/null /dev/null)" &&
+   test z"$subj" = z"Subject: message"
+'
+
+test_expect_failure 'mailinfo -b trailing [PATCH]' '
+   subj="$(echo "Subject: [other] [PATCH] message" |
+   git mailinfo -b /dev/null /dev/null)" &&
+   test z"$subj" = z"Subject: [other] message"
+'
+
+test_expect_failure 'mailinfo -b separated double [PATCH]' '
+   subj="$(echo "Subject: [PATCH] [other] [PATCH] message" |
+   git mailinfo -b /dev/null /dev/null)" &&
+   test z"$subj" = z"Subject: [other] message"
+'
 
 test_done
---

Re: [PATCH] mailinfo.c: move side-effects outside of assert

[Kyle J. McKay]

On Dec 21, 2016, at 18:21, Kyle J. McKay wrote:


On Dec 21, 2016, at 07:55, Jeff King wrote:


On Tue, Dec 20, 2016 at 09:54:15PM -0800, Kyle J. McKay wrote:

I wasn't aware anybody actually built with NDEBUG at all. You'd  
have to

explicitly ask for it via CFLAGS, so I assume most people don't.


Not a good assumption.  You know what happens when you assume[1],  
right? ;)


Kind of. If it's a configuration that nobody[1] in the Git  
development
community intended to support or test, then isn't the person  
triggering

it the one making assumptions?


No, I don't think so.  NDEBUG is very clearly specified in POSIX [1].

If NDEBUG is defined then "assert(...)" disappears (and in a nice  
way so as not to precipitate "unused variable" warnings).  "N" being  
"No" or "Not" or "Negated" or "bar over the top" + "DEBUG" meaning  
Not DEBUG.  So the code that goes away when NDEBUG is defined is  
clearly debug code.


I think there is a useful distinction here that I make that's worth  
sharing.  Perhaps it's splitting hairs, but I categorize this "extra"  
code that we've been discussing ("assert(...)" or "if (!...) die(...)"  
or "verify(...)" into two groups:



1) DEBUG code

This is code that developers use when creating new features.  Or  
helpful code that's needed when stepping through a program with the  
debugger to debug a problem.  Or even code that's only used by some  
kind of external "test".  It may be expensive, it may do things that  
should never be done in a build for wider consumption (such as write  
information to special log files, write special syslog messages  
etc.).  Often this code is used in combination with a "-g" debug  
symbols build and possibly even a "-O0" or "-O1" option.


Code like this has no place in a release executable meant for general  
use by an end user.


2) DIAGNOSTIC code

This is near zero overhead code that is intended to be left in a  
release build meant for general use and normally sits there not doing  
anything and NOT leaching any performance out of the build either.   
Its sole purpose in life is to provide a trail of "bread crumbs" if  
the executable goes ***BOOM***.  These "bread crumbs" should be just  
enough when combined with feedback from the unfortunate user who  
experienced the meltdown to re-create the issue in a real DEBUG build  
and find and fix the problem.



It seems to me what you are saying is that Git's "assert" calls are  
DIAGNOSTIC and therefore belong in a release build -- well, except for  
the nedmalloc "assert" calls which do not.


What I'm saying is if they are diagnostic and not debug (and I'm not  
arguing one way or the other, but you've already indicated they are  
near zero overhead which suggests they are indeed diagnostic in  
nature), then they do not belong inside an "assert" which can be  
disabled with "NDEBUG".  I'm arguing that "assert" is not intended for  
diagnostic code, but only debug code as used by nedmalloc.  Having Git  
treat "NDEBUG" one way -- "no, no, do NOT define NDEBUG because that  
disables Git diagnostics and I promise you there's no performance  
penalty" -- versus nedmalloc -- "yes, yes please DO define NDEBUG  
unless you really need our slow debugging code to be present for  
debugging purposes" -- just creates needless unnecessary confusion.


--Kyle

Re: [PATCH] mailinfo.c: move side-effects outside of assert

[Kyle J. McKay]

On Dec 21, 2016, at 07:55, Jeff King wrote:


On Tue, Dec 20, 2016 at 09:54:15PM -0800, Kyle J. McKay wrote:

I wasn't aware anybody actually built with NDEBUG at all. You'd  
have to

explicitly ask for it via CFLAGS, so I assume most people don't.


Not a good assumption.  You know what happens when you assume[1],  
right? ;)


Kind of. If it's a configuration that nobody[1] in the Git development
community intended to support or test, then isn't the person  
triggering

it the one making assumptions?


No, I don't think so.  NDEBUG is very clearly specified in POSIX [1].

If NDEBUG is defined then "assert(...)" disappears (and in a nice way  
so as not to precipitate "unused variable" warnings).  "N" being "No"  
or "Not" or "Negated" or "bar over the top" + "DEBUG" meaning Not  
DEBUG.  So the code that goes away when NDEBUG is defined is clearly  
debug code.


Considering the wide deployment and use of Git at this point I think  
rather the opposite to be true that "Git does Not require DEBUGging  
code to be enabled for everyday use."  The alternative that it does  
suggests it's not ready for prime time and quite clearly that's not  
the case.


I've been defining NDEBUG whenever I make a release build for quite  
some
time (not just for Git) in order to squeeze every last possible  
drop of

performance out of it.


I think here you are getting into superstition. Is there any single
assert() in Git that will actually have an impact on performance?


You have suggested there is and that Git is enabling NDEBUG for  
exactly that reason -- to increase performance:



On Dec 20, 2016, at 08:45, Jeff King wrote:

I do notice that we set NDEBUG for nedmalloc, though if I am  
reading the

Makefile right, it is just for compiling those files. It looks like
there are a ton of asserts there that _are_ potentially expensive



Perhaps Git should provide a "verify" macro.  Works like "assert"  
except
that it doesn't go away when NDEBUG is defined.  Being Git-provided  
it could
also use Git's die function.  Then Git could do a global replace of  
assert

with verify and institute a no-assert policy.


What would be the advantage of that over `if(...) die("BUG: ...")`? It
does not require you to write a reason in the die(), but I am not sure
that is a good thing.


You have stated that you believe the current "assert" calls in Git  
(excluding nedmalloc) should not magically disappear when NDEBUG is  
defined.  So precluding a more labor intensive approach where all  
currently existing "assert(...)" calls are replaced with an "if (!...)  
die(...)" combination, providing a "verify" macro is a quick way to  
make that happen.  Consider this, was the value that Jonathan provided  
for the "die" string immediately obvious to you?  It sure wasn't to  
me.  That means that whoever does the "assert(...)" -> "if(!...)die"  
swap out may need to be intimately familiar with that particular piece  
of code or the result will be no better than using a "verify" macro.


I'm just trying to find a quick and easy way to accommodate your  
wishes without redefining the semantics of NDEBUG. ;)


--Kyle

[1] http://pubs.opengroup.org/onlinepubs/9699919799/basedefs/assert.h.html

Re: [PATCH] mailinfo.c: move side-effects outside of assert

[Kyle J. McKay]

On Dec 20, 2016, at 08:45, Jeff King wrote:


On Tue, Dec 20, 2016 at 03:12:35PM +0100, Johannes Schindelin wrote:


On Dec 19, 2016, at 09:45, Johannes Schindelin wrote:

ACK. I noticed this problem (and fixed it independently as a part  
of a
huge patch series I did not get around to submit yet) while  
trying to

get Git to build correctly with Visual C.


Does this mean that Dscho and I are the only ones who add -DNDEBUG  
for
release builds?  Or are we just the only ones who actually run the  
test

suite on such builds?


It seems you and I are for the moment the only ones bothering with  
running

the test suite on release builds.


I wasn't aware anybody actually built with NDEBUG at all. You'd have  
to

explicitly ask for it via CFLAGS, so I assume most people don't.


Not a good assumption.  You know what happens when you assume[1],  
right? ;)


I've been defining NDEBUG whenever I make a release build for quite  
some time (not just for Git) in order to squeeze every last possible  
drop of performance out of it.


Certainly I never have when deploying to GitHub's cluster (let alone  
my

personal use), and I note that the Debian package also does not.


Yeah, I don't do it for my personal use because those are often not  
based on a release tag so I want to see any assertion failures that  
might happen and they're also not performance critical either.



So from my perspective it is not so much "do not bother with release
builds" as "are release builds even a thing for git?"


They should be if you're deploying Git in a performance critical  
environment.



One of the
reasons I suggested switching the assert() to a die("BUG") is that the
latter cannot be disabled. We generally seem to prefer those to  
assert()

in our code-base (though there is certainly a mix). If the assertions
are not expensive to compute, I think it is better to keep them in for
all builds. I'd much rather get a report from a user that says "I hit
this BUG" than "git segfaulted and I have no idea where" (of course I
prefer a backtrace even more, but that's not always an option).


Perhaps Git should provide a "verify" macro.  Works like "assert"  
except that it doesn't go away when NDEBUG is defined.  Being Git- 
provided it could also use Git's die function.  Then Git could do a  
global replace of assert with verify and institute a no-assert policy.


I do notice that we set NDEBUG for nedmalloc, though if I am reading  
the

Makefile right, it is just for compiling those files. It looks like
there are a ton of asserts there that _are_ potentially expensive, so
that makes sense.


So there's no way to get a non-release build of nedmalloc inside Git  
then without hacking the Makefile?  What if you need those assertions  
enabled?  Maybe NDEBUG shouldn't be defined by default for any files.


--Kyle

[1] https://www.youtube.com/watch?v=KEP1acj29-Y

[PATCH v3] mailinfo.c: move side-effects outside of assert

[Kyle J. McKay]

On Dec 19, 2016, at 15:26, Junio C Hamano wrote:

> "Kyle J. McKay" <mack...@gmail.com> writes:
>
>>> OK.  So we do not expect it to fail, but we still do want the side
>>> effect of that function (i.e. accmulation into the field).
>>>
>>> Somebody care to send a final "agreed-upon" version?
>>
>> Yup, here it is:
>
> Thanks.

Whoops. there's an extra paragraph in the commit description that I
meant to remove and, of course, I didn't notice it until I sent the
copy to the list.  :(

I don't think a "fixup" or "squash" can replace a description, right?

So here's a replacement patch with the correct description with the
deleted paragrah:

-- >8 --

Since 6b4b013f18 (mailinfo: handle in-body header continuations,
2016-09-20, v2.11.0) mailinfo.c has contained new code with an
assert of the form:

assert(call_a_function(...))

The function in question, check_header, has side effects.  This
means that when NDEBUG is defined during a release build the
function call is omitted entirely, the side effects do not
take place and tests (fortunately) start failing.

Since the only time that mi->inbody_header_accum is appended to is
in check_inbody_header, and appending onto a blank
mi->inbody_header_accum always happens when is_inbody_header is
true, this guarantees a prefix that causes check_header to always
return true.

Therefore replace the assert with an if !check_header + DIE
combination to reflect this.

Helped-by: Jonathan Tan <jonathanta...@google.com>
Helped-by: Jeff King <p...@peff.net>
Acked-by: Johannes Schindelin <johannes.schinde...@gmx.de>
Signed-off-by: Kyle J. McKay <mack...@gmail.com>
---

Notes:
Please include this PATCH in 2.11.x maint

 mailinfo.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/mailinfo.c b/mailinfo.c
index 2fb3877e..a489d9d0 100644
--- a/mailinfo.c
+++ b/mailinfo.c
@@ -710,7 +710,8 @@ static void flush_inbody_header_accum(struct mailinfo *mi)
 {
if (!mi->inbody_header_accum.len)
return;
-   assert(check_header(mi, >inbody_header_accum, mi->s_hdr_data, 0));
+   if (!check_header(mi, >inbody_header_accum, mi->s_hdr_data, 0))
+   die("BUG: inbody_header_accum, if not empty, must always 
contain a valid in-body header");
strbuf_reset(>inbody_header_accum);
 }
 
---

[PATCH v2] mailinfo.c: move side-effects outside of assert

[Kyle J. McKay]

On Dec 19, 2016, at 13:01, Junio C Hamano wrote:

> Jonathan Tan <jonathanta...@google.com> writes:
>
>>>> This is obviously an improvement, but it makes me wonder if we  
>>>> should be
>>>> doing:
>>>>
>>>> if (!check_header(mi, >inbody_header_accum, mi->s_hdr_data))
>>>>die("BUG: some explanation of why this can never happen");
>>>>
>>>> which perhaps documents the intended assumptions more clearly. A  
>>>> comment
>>>> regarding the side effects might also be helpful.
>>>
>>> I wondered exactly the same thing myself.  I was hoping Jonathan  
>>> would
>>> pipe in here with some analysis about whether this is:
>>>
>>>  a) a super paranoid, just-in-case, can't really ever fail because  
>>> by
>>> the time we get to this code we've already effectively validated
>>> everything that could cause check_header to return false in this  
>>> case
>>> ...
>> The answer is "a". The only time that mi->inbody_header_accum is
>> appended to is in check_inbody_header, and appending onto a blank
>> mi->inbody_header_accum always happens when is_inbody_header is true
>> (which guarantees a prefix that causes check_header to always return
>> true).
>>
>> Peff's suggestion sounds reasonable to me, maybe with an error  
>> message
>> like "BUG: inbody_header_accum, if not empty, must always contain a
>> valid in-body header".
>
> OK.  So we do not expect it to fail, but we still do want the side
> effect of that function (i.e. accmulation into the field).
>
> Somebody care to send a final "agreed-upon" version?

Yup, here it is:

-- 8< --

Since 6b4b013f18 (mailinfo: handle in-body header continuations,
2016-09-20, v2.11.0) mailinfo.c has contained new code with an
assert of the form:

assert(call_a_function(...))

The function in question, check_header, has side effects.  This
means that when NDEBUG is defined during a release build the
function call is omitted entirely, the side effects do not
take place and tests (fortunately) start failing.

Move the function call outside of the assert and assert on
the result of the function call instead so that the code
still works properly in a release build and passes the tests.

Since the only time that mi->inbody_header_accum is appended to is
in check_inbody_header, and appending onto a blank
mi->inbody_header_accum always happens when is_inbody_header is
true, this guarantees a prefix that causes check_header to always
return true.

Therefore replace the assert with an if !check_header + DIE
combination to reflect this.

Helped-by: Jonathan Tan <jonathanta...@google.com>
Helped-by: Jeff King <p...@peff.net>
Acked-by: Johannes Schindelin <johannes.schinde...@gmx.de>
Signed-off-by: Kyle J. McKay <mack...@gmail.com>
---

Notes:
Please include this PATCH in 2.11.x maint

 mailinfo.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/mailinfo.c b/mailinfo.c
index 2fb3877e..a489d9d0 100644
--- a/mailinfo.c
+++ b/mailinfo.c
@@ -710,7 +710,8 @@ static void flush_inbody_header_accum(struct mailinfo *mi)
 {
if (!mi->inbody_header_accum.len)
return;
-   assert(check_header(mi, >inbody_header_accum, mi->s_hdr_data, 0));
+   if (!check_header(mi, >inbody_header_accum, mi->s_hdr_data, 0))
+   die("BUG: inbody_header_accum, if not empty, must always 
contain a valid in-body header");
strbuf_reset(>inbody_header_accum);
 }
 
---

Re: [PATCH] mailinfo.c: move side-effects outside of assert

[Kyle J. McKay]

On Dec 19, 2016, at 12:03, Jeff King wrote:


On Sat, Dec 17, 2016 at 11:54:18AM -0800, Kyle J. McKay wrote:


Since 6b4b013f18 (mailinfo: handle in-body header continuations,
2016-09-20, v2.11.0) mailinfo.c has contained new code with an
assert of the form:

assert(call_a_function(...))

The function in question, check_header, has side effects.  This
means that when NDEBUG is defined during a release build the
function call is omitted entirely, the side effects do not
take place and tests (fortunately) start failing.

Move the function call outside of the assert and assert on
the result of the function call instead so that the code
still works properly in a release build and passes the tests.

Signed-off-by: Kyle J. McKay <mack...@gmail.com>
---

Notes:
   Please include this PATCH in 2.11.x maint


This is obviously an improvement, but it makes me wonder if we  
should be

doing:

 if (!check_header(mi, >inbody_header_accum, mi->s_hdr_data))
die("BUG: some explanation of why this can never happen");

which perhaps documents the intended assumptions more clearly. A  
comment

regarding the side effects might also be helpful.


I wondered exactly the same thing myself.  I was hoping Jonathan would  
pipe in here with some analysis about whether this is:


  a) a super paranoid, just-in-case, can't really ever fail because  
by the time we get to this code we've already effectively validated  
everything that could cause check_header to return false in this case


-or-

  b) Yeah, it could fail in the real world and it should "die" (and  
probably have a test added that triggers such death)


-or-

  c) Actually, if check_header does return false we can keep going  
without problem


-or-

  d) Actually, if check_header does return false we can keep going by  
making a minor change that should be in the patch


I assume that since Jonathan added the code he will just know the  
answer as to which one it is and I won't have to rely on the results  
of my imaginary analysis.  ;)


On Dec 19, 2016, at 09:45, Johannes Schindelin wrote:


ACK. I noticed this problem (and fixed it independently as a part of a
huge patch series I did not get around to submit yet) while trying  
to get

Git to build correctly with Visual C.


Does this mean that Dscho and I are the only ones who add -DNDEBUG for  
release builds?  Or are we just the only ones who actually run the  
test suite on such builds?


--Kyle

[PATCH] mailinfo.c: move side-effects outside of assert

[Kyle J. McKay]

Since 6b4b013f18 (mailinfo: handle in-body header continuations,
2016-09-20, v2.11.0) mailinfo.c has contained new code with an
assert of the form:

assert(call_a_function(...))

The function in question, check_header, has side effects.  This
means that when NDEBUG is defined during a release build the
function call is omitted entirely, the side effects do not
take place and tests (fortunately) start failing.

Move the function call outside of the assert and assert on
the result of the function call instead so that the code
still works properly in a release build and passes the tests.

Signed-off-by: Kyle J. McKay <mack...@gmail.com>
---

Notes:
Please include this PATCH in 2.11.x maint

 mailinfo.c | 5 -
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/mailinfo.c b/mailinfo.c
index 2fb3877e..47442fb5 100644
--- a/mailinfo.c
+++ b/mailinfo.c
@@ -708,9 +708,12 @@ static int is_scissors_line(const char *line)
 
 static void flush_inbody_header_accum(struct mailinfo *mi)
 {
+   int okay;
+
if (!mi->inbody_header_accum.len)
return;
-   assert(check_header(mi, >inbody_header_accum, mi->s_hdr_data, 0));
+   okay = check_header(mi, >inbody_header_accum, mi->s_hdr_data, 0);
+   assert(okay);
strbuf_reset(>inbody_header_accum);
 }
 
---

Re: Git v2.11.0 breaks max depth nested alternates

[Kyle J. McKay]

On Dec 3, 2016, at 20:55, Jeff King wrote:


So I do think this is worth dealing with, but I'm also curious why
you're hitting the depth-5 limit. I'm guessing it has to do with  
hosting

a hierarchy of related repos. But is your system then always in danger
of busting the 5-limit if people create too deep a repository  
hierarchy?


No we check for the limit.  Anything at the limit gets broken by the  
quarantine change though.


Specifically, I'm wondering if it would be sufficient to just bump  
it to

6. Or 100.


Well, if we left the current limit in place, but as you say:


Of course any static bump runs into the funny case where a repo
_usually_ works, but fails when pushed to. Which is kind of nasty and
unintuitive. And your patch fixes that,


Yes.  That's not nice, hence the patch.  Without the fix, pushing  
might work sometimes until you actually need to access cut-off objects  
at pre-receive time.  So you might be able to push sometimes and  
sometimes it breaks.



and we can leave the idea of
bumping the static depth number as an orthogonal issue (that  
personally,

I do not care about much about either way).


The patch is a step on that road.  It doesn't go that far but all it  
would take is connecting the introduced variable to a config item.   
But you still need to bump it by 1 during quarantine operations.  Such  
support would even allow alternates to be disallowed (except during  
quarantine).  I wonder if there's an opportunity for further pack  
operation optimizations in such a case (you know there are no  
alternates because they're not allowed)?



diff --git a/common-main.c b/common-main.c
index c654f955..9f747491 100644
--- a/common-main.c
+++ b/common-main.c
@@ -37,5 +37,8 @@ int main(int argc, const char **argv)

restore_sigpipe_to_default();

+   if (getenv(GIT_QUARANTINE_ENVIRONMENT))
+   alt_odb_max_depth++;
+
return cmd_main(argc, argv);


After reading your problem description, my initial thought was to
increment the counter when we allocate the tmp-objdir, and decrement
when it is destroyed. Because the parent receive-pack process adds  
it to

its alternates, too. But:

 1. Receive-pack doesn't care; it adds the tmp-objdir as an alternate,
rather than adding it as its main object dir and bumping down the
main one.

 2. There would have to be some way of communicating to sub-processes
that they should bump their max-depth by one.


All true.  And I had similar thoughts.  Perhaps we should add your  
comments to the patch description?  There seems to be a trend towards  
having longer patch descriptions these days... ;)



You've basically used the quarantine-path variable as the
inter-process flag for (2). Which feels a little funny, because its
value is unrelated to the alt-odb setup. But it is a reliable  
signal, so

there's a certain elegance. It's probably the best option, given that
the alternative is a specific variable to say "hey, bump your
max-alt-odb-depth by one". That's pretty ugly, too. :)


You took the words right out of my mouth...   I guess I need to work  
on doing a better job of dumping my stream-of-thoughts that go into a  
patch into the emails to the list.


Most all of your comments could be dumped into the patch description  
as-is to pimp it out some.  I have no objection to that, even adding  
an "Additional-analysis-by:" (or similar) credit line too.  :)


--Kyle

Git v2.11.0 breaks max depth nested alternates

[Kyle J. McKay]

The recent addition of pre-receive quarantining breaks nested  
alternates that are already at the maximum alternates nesting depth.

In the file sha1_file.c in the function link_alt_odb_entries we have  
this:

 > if (depth > 5) {
 > error("%s: ignoring alternate object stores, nesting too deep.",
 > relative_base);
 > return;
 > }

When the incoming quarantine takes place the current objects directory  
is demoted to an alternate thereby increasing its depth (and any  
alternates it references) by one and causing any object store that was  
previously at the maximum nesting depth to be ignored courtesy of the  
above hard-coded maximum depth.

If the incoming push happens to need access to some of those objects  
to perhaps "--fix-thin" its pack it will crash and burn.

Originally I was not going to include a patch to fix this, but simply  
suggest that the expeditious fix is to just allow one additional  
alternates nesting depth level during quarantine operations.

However, it was so simple, I have included the patch below :)

I have verified that where a push with Git v2.10.2 succeeds and a push  
with Git v2.11.0 to the same repository fails because of this problem  
that the below patch does indeed correct the issue and allow the push  
to succeed.

Cheers,

Kyle

-- 8< --
Subject: [PATCH] receive-pack: increase max alternates depth during quarantine

Ever since 722ff7f876 (receive-pack: quarantine objects until
pre-receive accepts, 2016-10-03, v2.11.0), Git has been quarantining
objects and packs received during an incoming push into a separate
objects directory and using the alternates mechanism to make them
available until they are either accepted and moved into the main
objects directory or rejected and discarded.

Unfortunately this has the side effect of increasing the alternates
nesting depth level by one for all pre-existing alternates.

If a repository is already at the maximum alternates nesting depth,
then this quarantining operation can temporarily push it over making
the incoming push fail.

To prevent the failure we simply increase the allowed alternates
nesting depth by one whenever a quarantine operation is in effect.

Signed-off-by: Kyle J. McKay <mack...@gmail.com>
---

Notes:
Some alternates nesting depth background:

If base/fork0/fork1/fork2/fork3/fork4/fork5 represents
seven git repositories where base.git has no alternates,
fork0.git has base.git as an alternate, fork1.git has
fork0.git as an alternate and so on where fork5.git has
only fork4.git as an alternate, then fork5.git is at
the maximum allowed depth of 5.  git fsck --strict --full
works without complaint on fork5.git.

However, in base/fork0/fork1/fork2/fork3/fork4/fork5/fork6,
an fsck --strict --full of fork6.git will generate complaints
and any objects/packs present in base.git will be ignored.

 cache.h   | 1 +
 common-main.c | 3 +++
 environment.c | 1 +
 sha1_file.c   | 2 +-
 4 files changed, 6 insertions(+), 1 deletion(-)

diff --git a/cache.h b/cache.h
index a50a61a1..25c17c29 100644
--- a/cache.h
+++ b/cache.h
@@ -676,6 +676,7 @@ extern size_t packed_git_limit;
 extern size_t delta_base_cache_limit;
 extern unsigned long big_file_threshold;
 extern unsigned long pack_size_limit_cfg;
+extern int alt_odb_max_depth;
 
 /*
  * Accessors for the core.sharedrepository config which lazy-load the value
diff --git a/common-main.c b/common-main.c
index c654f955..9f747491 100644
--- a/common-main.c
+++ b/common-main.c
@@ -37,5 +37,8 @@ int main(int argc, const char **argv)
 
restore_sigpipe_to_default();
 
+   if (getenv(GIT_QUARANTINE_ENVIRONMENT))
+   alt_odb_max_depth++;
+
return cmd_main(argc, argv);
 }
diff --git a/environment.c b/environment.c
index 0935ec69..32e11f70 100644
--- a/environment.c
+++ b/environment.c
@@ -64,6 +64,7 @@ int merge_log_config = -1;
 int precomposed_unicode = -1; /* see probe_utf8_pathname_composition() */
 unsigned long pack_size_limit_cfg;
 enum hide_dotfiles_type hide_dotfiles = HIDE_DOTFILES_DOTGITONLY;
+int alt_odb_max_depth = 5;
 
 #ifndef PROTECT_HFS_DEFAULT
 #define PROTECT_HFS_DEFAULT 0
diff --git a/sha1_file.c b/sha1_file.c
index 9c86d192..15b8432e 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -337,7 +337,7 @@ static void link_alt_odb_entries(const char *alt, int len, 
int sep,
int i;
struct strbuf objdirbuf = STRBUF_INIT;
 
-   if (depth > 5) {
+   if (depth > alt_odb_max_depth) {
error("%s: ignoring alternate object stores, nesting too deep.",
relative_base);
return;
---

[ANNOUNCE] git-log-compact v1.0

[Kyle J. McKay]

> NOTE: If you read the excellent Git Rev News [1], then you
> already know all about git-log-compact :)

The git-log-compact script provides a compact alternative to the
`git log --oneline` output that includes dates, times and author
and/or committer initials in a space efficient output format.

`git-log-compact` is intended to fill the gap between the single line
`--oneline` log output format and the multiline `--pretty=short` /
`--pretty=medium` output formats.

It is not strictly a one line per commit output format (but almost) and
while it only shows the title of each commit (like the short format) it
does include author and date information (similarly to the medium format)
except that timestamps are shown very compactly and author/committer
names are shown as initials.

This allows one to get a complete view of repository activity in a very
compact output format that can show many commits per screen full and is
fully compatible with the `--graph` option.

Simple example output from the Git repository:

git log-compact --graph --date-order --decorate --no-merges -n 5 v2.5.3

=== 2015-09-17 ===
  * ee6ad5f4 12:16 jch (tag: v2.5.3) Git 2.5.3
=== 2015-09-09 ===
  * b9d66899 14:22 js  am --skip/--abort: merge HEAD/ORIG_HEAD tree into index
  |   === 2015-09-04 ===
  | * 27ea6f85 10:46 jch (tag: v2.5.2) Git 2.5.2
  * 74b67638 10:36 jch (tag: v2.4.9) Git 2.4.9
   ..
  * ecad27cf 10:32 jch (tag: v2.3.9) Git 2.3.9

I have been wanting a compact one line output format that included dates,
times and initials for some time that is compatible with --graph, clearly
shows root commits and eliminates confusion over whether or not two adjacent
lines in the output are related as parent/child (the --show-linear-break
option does not work with --graph).

The git-log-compact utility is the result.  Except for --notes, --pretty and
--format options (which would make the output a non-oneline format) any
other `git log` option may be used (including things like --cherry-mark,
--patch, --raw, --stat, --summary, --show-linear-break etc.),

There are a few new options specific to git-log-compact which are described
in the README and the `git-log-compact -h` output that can be used to alter
the dates, times and/or initials displayed.

The project page with detailed help and many screen shots is located at:

  https://mackyle.github.io/git-log-compact/

Alternatively the repository can be cloned from:

  https://github.com/mackyle/git-log-compact.git

Or the script file itself (which is really all you need) can be
viewed/fetched from:

  https://github.com/mackyle/git-log-compact/blob/HEAD/git-log-compact

The git-log-compact script should work with any version of Git released
in the last several years.

--Kyle

[1] https://git.github.io/rev_news/2016/10/19/edition-20/

Re: [PATCH 10/10] get_short_sha1: list ambiguous objects on error

[Kyle J. McKay]

On Sep 29, 2016, at 06:24, Jeff King wrote:

If you are doing "git show 235234" it should pick the tag (if it  
peels to a
committish) because Git has already set a precedent of preferring  
tags over
commits when it disambiguates ref names and otherwise pick the  
commit.


I'm not convinced that picking the tag is actually helpful in this  
case;

I agree with Linus that feeding something to "git show" almost always
wants to choose the commit.


Since "git show" peels tags you end up seeing the commit it refers to  
(assuming it's a committish tag).


I also don't think tag ambiguity in short sha1s is all that  
interesting.


The Linux repository has this:

   901069c:
  901069c71415a76d commit iwlagn: change Copyright to 2011
  901069c5c5b15532 tag(v2.6.38-rc4) Linux 2.6.38-rc4

Since that tag peels to a commit, it seems like it would be incorrect  
to pick the commit over the tag when you're looking for a committish.


Either 901069c should resolve to the tag (which gets peeled to the  
commit) or it should error out with the hint messages.


The Git repository has this:

   c512b03:
  c512b035556eff4d commit Merge branch 'rc/maint-reflog-msg-for- 
forced

  c512b0344196931a tag(v0.99.9a) GIT 0.99.9a

So perhaps it's a little bit more interesting than it first appears.  :)

--Kyle

Re: [PATCH 10/10] get_short_sha1: list ambiguous objects on error

[Kyle J. McKay]

On Sep 26, 2016, at 09:36, Linus Torvalds wrote:


On Mon, Sep 26, 2016 at 5:00 AM, Jeff King  wrote:


This patch teaches get_short_sha1() to list the sha1s of the
objects it found, along with a few bits of information that
may help the user decide which one they meant.


This looks very good to me, but I wonder if it couldn't be even more  
aggressive.


In particular, the only hashes that most people ever use in short form
are commit hashes. Those are the ones you'd use in normal human
interactions to point to something happening.

So when the disambiguation notices that there is ambiguity, but there
is only _one_ commit, maybe it should just have an aggressive mode
that says "use that as if it wasn't ambiguous".


If you have this:

faa23ec9b437812ce2fc9a5b3d59418d672debc1 refs/heads/ambig
7f40afe646fa3f8a0f361b6f567d8f7d7a184c10 refs/tags/ambig

and you do this:

$ git rev-parse ambig
warning: refname 'ambig' is ambiguous.
7f40afe646fa3f8a0f361b6f567d8f7d7a184c10

Git automatically prefers the tag over the branch, but it does spit  
out a warning.



And then have an explicit command (or flag) to do disambiguation for
when you explicitly want it.


I think you don't even need that.  Git already does disambiguation for  
ref names, picks one and spits out a warning.


Why not do the same for short hash names when it makes sense?


Rationale: you'd never care about short forms for tags. You'd just use
the tag name. And while blob ID's certainly show up in short form in
diff output (in the "index" line), very few people will use them. And
tree hashes are basically never seen outside of any plumbing commands
and then seldom in shortened form.

So I think it would make sense to default to a mode that just picks
the commit hash if there is only one such hash. Sure, some command
might want a "treeish", but a commit is still more likely than a tree
or a tag.

But regardless, this series looks like a good thing.


I like it too.

But perhaps it makes sense to actually pick one if there's only one  
disambiguation of the type you're looking for.


For example given:

235234a blob
2352347 tag
235234f tree
2352340 commit

If you are doing "git cat-file blob 235234" it should pick the blob  
and spit out a warning (and similarly for other cat-file types).  But  
"git cat-file -p 235234" would give the fatal error with the  
disambiguation hints because it wants type "any".


If you are doing "git show 235234" it should pick the tag (if it peels  
to a committish) because Git has already set a precedent of preferring  
tags over commits when it disambiguates ref names and otherwise pick  
the commit.


Lets consider this approach using the stats for the Linux kernel:


Ambiguous prefix length 7 counts:
  prefixes:   44733
   objects:   89766

Ambiguous length 11 (but not at length 12) info:
  prefixes:   2
  0 (with 1 or more commit disambiguations)

Ambiguous length 10 (but not at length 11) info:
  prefixes:  12
  3 (with 1 or more commit disambiguations)
  0 (with 2 or more commit disambiguations)

Ambiguous length 9 (but not at length 10) info:
  prefixes: 186
 43 (with 1 or more commit disambiguations)
  1 (with 2 or more commit disambiguations)

Ambiguous length 8 (but not at length 9) info:
  prefixes:2723
651 (with 1 or more commit disambiguations)
 40 (with 2 or more commit disambiguations)

Ambiguous length 7 (but not at length 8) info:
  prefixes:   41864
   9842 (with 1 or more commit disambiguations)
680 (with 2 or more commit disambiguations)


Of the 44733 ambiguous length 7 prefixes, only about 10539 of them  
disambiguate into one or more commit objects.


But if we apply the "spit a warning and prefer a commit object if  
there's only one and you're looking for a committish" rule, that drops  
the number from 10539 to about 721.  In other words, only about 7% of  
the previously ambiguous short commit SHA1 prefixes would continue to  
be ambiguous at length 7.  In fact it almost makes a prefix length of  
9 good enough, there's just the one at length 9 that disambiguates  
into more than one commit (45f014c52).


--Kyle

Re: Changing the default for "core.abbrev"?

[Kyle J. McKay]

On Sep 25, 2016, at 18:39, Linus Torvalds wrote:


The kernel, these days, is at roughly 5 million objects, and while the
seven hex digits are still often enough for uniqueness (and git will
always add digits *until* it is unique), it's long been at the point
where I tell people to do

   git config --global core.abbrev 12

because even though git will extend the seven hex digits until the
object name is unique, that only reflects the *current* situation in
the repository. With 5 million objects and a very healthy growth rate,
a 7-8 hex digit number that is unique today is not necessarily unique
a month or two from now, and then it gets annoying when a commit
message has a short git ID that is no longer unique when you go back
and try to figure out what went wrong in that commit.


On Sep 25, 2016, at 20:46, Junio C Hamano wrote:


Linus Torvalds  writes:


I can just keep reminding kernel maintainers and developers to update
their git config, but maybe it would be a good idea to just admit  
that

the defaults picked in 2005 weren't necessarily the best ones
possible, and those could be bumped up a bit?


I am not quite sure how good any new default would be, though.  Just
like any timeout is not long enough for somebody, growing projects
will eventually hit whatever abbreviation length they start with.


This made me curious what the situation is really like.  So I crunched  
some data.


Using a recent clone of $korg/torvalds/linux:

$ git rev-parse --verify d597639e203
error: short SHA1 d597639e203 is ambiguous.
fatal: Needed a single revision

So the kernel already has 11-character "short" SHA1s that are  
ambiguous.  Is a core.abbrev setting of 12 really good enough?


Here are the stats on the kernel's repository:

Ambiguous length 11 (but not at length 12) info:
  prefixes:   2
  0 (with 1 or more commit disambiguations)

Ambiguous length 10 (but not at length 11) info:
  prefixes:  12
  3 (with 1 or more commit disambiguations)
  0 (with 2 or more commit disambiguations)

Ambiguous length 9 (but not at length 10) info:
  prefixes: 186
 43 (with 1 or more commit disambiguations)
  1 (with 2 or more commit disambiguations)
  0 (with 3 or more disambiguations)

Ambiguous length 8 (but not at length 9) info:
  prefixes:2723
651 (with 1 or more commit disambiguations)
 40 (with 2 or more commit disambiguations)
  1 (with 3 or more disambiguations)
  maxambig:   3 (there is 1 of them)

Ambiguous length 7 (but not at length 8) info:
  prefixes:   41864
   9842 (with 1 or more commit disambiguations)
680 (with 2 or more commit disambiguations)
299 (with 3 or more disambiguations)
  maxambig:   3 (there are 299 of them)

The "maxambig" value is the maximum number of disambiguations for any  
single prefix at that prefix length.  So for prefixes of length 7  
there are 299 that disambiguate into 3 objects.


Just out of curiosity, generating stats on the Git repository gives:

Ambiguous length 8 (but not at length 9) info:
  prefixes:   7
  3 (with 1 or more commit disambiguations)
  2 (with 2 or more commit disambiguations)
  0 (with 3 or more disambiguations)

Ambiguous length 7 (but not at length 8) info:
  prefixes:  87
 36 (with 1 or more commit disambiguations)
  3 (with 2 or more commit disambiguations)
  0 (with 3 or more disambiguations)

Running the stats on $github/gitster/git produces some ambiguous  
length 9 prefixes (one of which contains a commit disambiguation).


--Kyle

Re: [PATCH 10/10] get_short_sha1: list ambiguous objects on error

[Kyle J. McKay]

On Sep 26, 2016, at 05:00, Jeff King wrote:


 $ git rev-parse b2e1
 error: short SHA1 b2e1 is ambiguous
 hint: The candidates are:
 hint:   b2e1196 tag v2.8.0-rc1
 hint:   b2e11d1 tree
 hint:   b2e1632 commit 2007-11-14 - Merge branch 'bs/maint-commit- 
options'

 hint:   b2e1759 blob
 hint:   b2e18954 blob
 hint:   b2e1895c blob
 fatal: ambiguous argument 'b2e1': unknown revision or path not in  
the working tree.

 Use '--' to separate paths from revisions, like this:
 'git  [...] -- [...]'


This hint: information is excellent.  There needs to be a way to show  
it on demand.


$ git rev-parse --disambiguate=b2e1
b2e11962c5e6a9c81aa712c751c83a743fd4f384
b2e11d1bb40c5f81a2f4e37b9f9a60ec7474eeab
b2e163272c01aca4aee4684f5c683ba341c1953d
b2e18954c03ff502053cb74d142faab7d2a8dacb
b2e1895ca92ec2037349d88b945ba64ebf16d62d

Not nearly so helpful, but the operation of --disambiguate cannot be  
changed without breaking current scripts.


Can your excellent "hint:" output above be attached to the -- 
disambiguate option somehow, please.  Something like this perhaps:


$ git rev-parse --disambiguate-list=b2e1
b2e1196 tag v2.8.0-rc1
b2e11d1 tree
b2e1632 commit 2007-11-14 - Merge branch 'bs/maint-commit-options'
b2e1759 blob
b2e18954 blob
b2e1895c blob

Any option name will do, --disambiguate-verbose, --disambiguate- 
extended, --disambiguate-long, --disambiguate-log, --disambiguate- 
help, --disambiguate-show-me-something-useful-to-humans-not-scripts ...


--Kyle

Re: [PATCH/RFC] git log --oneline alternative with dates, times and initials

[Kyle J. McKay]

On Sep 29, 2016, at 01:33, Jeff King wrote:


On Wed, Sep 28, 2016 at 10:34:51PM -0700, Kyle J. McKay wrote:


git log-times --graph --date-order --decorate --no-merges -n 5 v2.5.3

   === 2015-09-17 ===
 * ee6ad5f4 12:16 jch (tag: v2.5.3) Git 2.5.3
   === 2015-09-09 ===
 * b9d66899 14:22 js  am --skip/--abort: merge HEAD/ORIG_HEAD tree  
into index

 |   === 2015-09-04 ===
 | * 27ea6f85 10:46 jch (tag: v2.5.2) Git 2.5.2
 * 74b67638 10:36 jch (tag: v2.4.9) Git 2.4.9
  ..
 * ecad27cf 10:32 jch (tag: v2.3.9) Git 2.3.9


I was surprised to see this as a separate script, but it is true  
that we

cannot quite pull it off with --format. I think we are very close,
though.  With the patches below I think you can do:

 git log \
   --commit-header='%C(auto,bold blue)== %as ==%C(auto,reset)'
   --format='%C(auto)%h %C(auto,green)%ad %C(auto,red)%aS/%cS%C(auto) 
%d%C(auto,reset) %s' \

   --graph --no-merges --author-date-order --date=format:%H:%M

and get the same (or very similar) output.

 [1/5]: pretty: allow formatting DATE_SHORT
 [2/5]: pretty: allow formatting names as initials
 [3/5]: graph: fix extra spaces in graph_padding_line
 [4/5]: graph: helper functions for printing commit header
 [5/5]: log: add --commit-header option

Each of those commits[1] needs some minor polish, and as I'm not  
really

that interested in fancy log output myself, I don't plan on working on
them further. I was mostly curious just how close we were. But if  
you'd

like to pursue it, feel free to use them as a starting point.


Those patches are missing some of the features like showing root  
commits, handling two letter initials, showing the weekday, inserting  
a break where needed to avoid parent-child confusion in graph output  
and properly handling Duy's initials. :)


I suppose if all the objects that output a date took a '('   
')' option that would get you part of the way -- it could replace  
DATE_SHORT with DATE_STRFTIME.


Also the above example doesn't handle marks properly in graph mode.   
Yes, you can add the "%m" format option but it does something odd and  
the script fixes it up.


On the other hand, git-log-times started out as a script for something  
else (a shell script actually) and just got embellished further and  
turned into a perl script for speed.


Your patches are a good first start though but reading the --graph  
code gives me headaches and I figured it would be like going down a  
rabbit hole to make the code support everything the script does.


The script also has one big advantage.  It works with the version of  
Git everybody already has installed.  :)


And nobody is ever going to want to type several lines of arcane  
formatting instructions to get the output.  ;_)


It would need a new option, perhaps --oneline-extended or something.

The patches are a good start but that doesn't help anyone using Git  
today which is why git-log-times is submitted as a contrib script --  
much like the way diff-highlight is still a contrib script and not  
supported directly by Git either.


--Kyle

[PATCH/RFC] git log --oneline alternative with dates, times and initials

[Kyle J. McKay]

Simple example output from the Git repository:

git log-times --graph --date-order --decorate --no-merges -n 5 v2.5.3

=== 2015-09-17 ===
  * ee6ad5f4 12:16 jch (tag: v2.5.3) Git 2.5.3
=== 2015-09-09 ===
  * b9d66899 14:22 js  am --skip/--abort: merge HEAD/ORIG_HEAD tree into index
  |   === 2015-09-04 ===
  | * 27ea6f85 10:46 jch (tag: v2.5.2) Git 2.5.2
  * 74b67638 10:36 jch (tag: v2.4.9) Git 2.4.9
   ..
  * ecad27cf 10:32 jch (tag: v2.3.9) Git 2.3.9

I have been wanting a compact one line output format that included dates,
times and initials for some time that is compatible with --graph, clearly
shows root commits and eliminates confusion over whether or not two adjacent
lines in the output are related as parent/child (the --show-linear-break
option does not work with --graph).

The git-log-times utility is the result.  Except for --notes, --pretty and
--format options (which would make the output a non-oneline format) any
other `git log` option may be used (including things like --cherry-mark,
--patch, --raw, --stat, --summary, --show-linear-break etc.),

There are a few new options specific to git-log-times which are described
in the README and the `git-log-times -h` output that can be used to alter
the dates, times and/or initials displayed.

The patch below adds a contrib/git-log-times directory containing the
executable (git-log-times) and the README.

--Kyle

P.S. git am complains about 26 lines with whitespace errors.  They are
 not whitespace errors.  The README is in markdown format and they
 are explicit line break instructions to markdown (2 trailing blanks).
 Removing them would corrupt the markdown output.

P.P.S A picture is worth a thousand words, so the formatted help text,
  and several images of actual git-log-times output are available at
  https://gist.github.com/mackyle/4c33e4802a8269b3f200f2c00352ce6a

-- 8< --
Subject: [PATCH] contrib/git-log-times: alternative git log --oneline utility

The git-log-times utility provides an alternative interface to using
git log --oneline that includes dates, times and author initials.

Additionally root commits are marked for easy identification and
when using --graph mode breaks are inserted when necessary to prevent
two adjacent output lines from being misconstrued as having a parent
child relationship when they actually do not.

Other than --notes, --pretty and --format options which are not
allowed (because that would no longer be a one line format) all git
log options are available for use.

Output will be colorized using the same rules used for git log
output.

The three extra items in the output (dates, times and initials) use
'color.log-times.date', 'color.log-times.time' and
'color.log-times.initials' to change their default color.

Other options specific to git-log-times may be shown by using the
-h option (i.e. `git-log-times -h`).

One or more default options which behave as though they are the
first option argument(s) on the command line may be set by assigning
them to the 'log-times.defaults' config value as space-separated
options each including its leading '-' or '--'.

Signed-off-by: Kyle J. McKay <mack...@gmail.com>
---
 contrib/git-log-times/README| 256 
 contrib/git-log-times/git-log-times | 464 
 2 files changed, 720 insertions(+)
 create mode 100644 contrib/git-log-times/README
 create mode 100755 contrib/git-log-times/git-log-times

diff --git a/contrib/git-log-times/README b/contrib/git-log-times/README
new file mode 100644
index ..65f1d2c5
--- /dev/null
+++ b/contrib/git-log-times/README
@@ -0,0 +1,256 @@
+git-log-times
+=
+
+An alterative to `git log --oneline` that includes dates, times and
+author initials in a compact one line output format.
+
+The `--notes`, `--pretty` and `--format` options are not allowed but any
+other `git log` options should work fine including `--graph`.
+
+In both `--graph` and non `--graph` modes:
+
+   * Root commits are identified by `_` on either side of the hash
+
+When `--graph` mode is enabled, the graph output is enhanced as follows:
+
+   * Breaks are inserted when necessary to avoid parent/child ambiguity
+
+
+Installation
+
+
+Put the `git-log-times` executable file in one of the directories
+included in the `PATH` environment variable.
+
+Optionally set a global alias to save typing such as `lo` like so:
+
+git config --global alias.lo log-times
+
+Optionally set global default options such as `--two-initials` and
+`--abbrev=8` like so:
+
+git config --global log-times.defaults "--two-initials --abbrev=8"
+
+
+Dates & Times
+-
+
+Dates and times are shown in the local timezone.  Set the TZ variable
+before running `git log-times` (e.g. `TZ=UTC git log-times` to show
+dates and times in UTC) or use the `--time-zone=` option (e.g.
+`git log-times --time-zone=UTC`) to change that.
+
+Dates a

Re: [PATCH 0/3] Make httpd tests run

[Kyle J. McKay]

On Apr 8, 2015, at 08:05, Michael J Gruber wrote:

This series grew from an attempt at enlarging my personal test run  
coverage

on a standard Fedora 21 64bit box. Aka chain-lint fall-out.

With 1/3, I get all httpd tests to run (when port is set, of course).

2/3 and 3/3 are an attempt at getting git-svn over http tests to run.
2/3 is certainly correct but not sufficient.
3/3 gets httpd to run but svn does not connect. This is WIP and RFH,
and maybe requires rewriting lib-git-svn to use a config which depends
on the apache version (like lib-hhtpd does), or to leverage lib-httpd.

Michael J Gruber (3):
 t/lib-httpd: load mod_unixd
 t/lib-git-svn: check same httpd module dirs as lib-httpd
 t/lib-git-svn: adjust config to apache 2.4


The changes in this series appear to break compatibility with Apache  
2.2.


Does that mean that Apache 2.2 is no longer supported for running  
these tests?


Or am I missing something here...

-Kyle
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/3] Make httpd tests run

[Kyle J. McKay]

On Apr 9, 2015, at 06:04, Kyle J. McKay wrote:


On Apr 8, 2015, at 08:05, Michael J Gruber wrote:

This series grew from an attempt at enlarging my personal test run  
coverage

on a standard Fedora 21 64bit box. Aka chain-lint fall-out.

With 1/3, I get all httpd tests to run (when port is set, of course).

2/3 and 3/3 are an attempt at getting git-svn over http tests to run.
2/3 is certainly correct but not sufficient.
3/3 gets httpd to run but svn does not connect. This is WIP and RFH,
and maybe requires rewriting lib-git-svn to use a config which  
depends
on the apache version (like lib-hhtpd does), or to leverage lib- 
httpd.


Michael J Gruber (3):
t/lib-httpd: load mod_unixd
t/lib-git-svn: check same httpd module dirs as lib-httpd
t/lib-git-svn: adjust config to apache 2.4


The changes in this series appear to break compatibility with Apache  
2.2.


Does that mean that Apache 2.2 is no longer supported for running  
these tests?


Or am I missing something here...


Never mind.  I see the IfVersion... they're wrapped in now.  Would  
have been nice if they were indented so there was a hint about that in  
the diff, or the diff included enough context to see that.  But that  
file formatting has nothing to do with your change.


Sorry for the noise.

-Kyle
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: git 2.3.4, ssh: Could not resolve hostname

[Kyle J. McKay]

On Apr 2, 2015, at 17:02, Torsten Bögershausen wrote:


On 2015-04-02 21.35, Jeff King wrote:

On Thu, Apr 02, 2015 at 12:31:14PM -0700, Reid Woodbury Jr. wrote:


Ah, understand. Here's my project URL for 'remote origin' with a
more meaningful representation of their internal FQDN:

url = ssh://rwoodbury@systemname.groupname.online:/opt/git/inventory.git

The online is their literal internal TLD.


Thanks. The problem is the extra : after online; your URL is
malformed. You can just drop that colon entirely.

I do not think we need to support this syntax going forward (the  
colon

is meaningless here, and our documentation is clear that it should go
with a port number), but on the other hand, it might be nice to be  
more
liberal, as we were in v2.3.3 and prior. I'll leave it to Torsten  
to see
whether supporting that would hurt some of the other cases, or  
whether

it would make the code too awkward.

-Peff


Thanks for digging.

This makes my think that it is
a) non-standard to have the extra colon


It's not.  See RFC 3986 appendix A:

  authority = [ userinfo @ ] host [ : port ]

  port = *DIGIT

*DIGIT means (see RFC 2234 section 3.6) zero or more digits.


b) The error message could be better
c) We don't have a test case
d) This reminds my of an improvement from Linus:
608d48b2207a61528
..
   So when somebody passes me a please pull request pointing to  
something

   like the following

git://git.kernel.org:/pub/scm/linux/kernel/git/mchehab/v4l-dvb.git

   (note the extraneous colon at the end of the host name), git  
would happily
   try to connect to port 0, which would generally just cause the  
remote to
   not even answer, and the connect() will take a long time to  
time out.

.

Sorry guys for the regression, the old parser handled the extra  
colon as port 0,
the new one looks for the / as the end of the hostname (and the  
beginning of the path)


Either we accept the extra colon as before, or the parser puts out a  
better error message,


[...]

Spontaneously I would say that a trailing ':' at the end of a  
hostname in the ssh:// scheme

can be safely ignored, what do you think ?


You know, there is a url_normalize routine in urlmatch.h/urlmatch.c  
that checks for a lot of these things and provides a translated error  
message if there's a problem as well as normalizing and separating out  
the various parts of the URL.  It does not currently handle default  
ports for anything other than http[s] but it would be simple enough to  
add support for ssh, git, ftp[s] and rsync default ports too.


-Kyle--
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] diff-highlight: Fix broken multibyte string

[Kyle J. McKay]

On Apr 3, 2015, at 15:08, Jeff King wrote:

Doing:

diff --git a/contrib/diff-highlight/diff-highlight b/contrib/diff- 
highlight/diff-highlight

index 08c88bb..1c4b599 100755
--- a/contrib/diff-highlight/diff-highlight
+++ b/contrib/diff-highlight/diff-highlight
@@ -165,7 +165,7 @@ sub highlight_pair {
sub split_line {
local $_ = shift;
return map { /$COLOR/ ? $_ : (split //) }
-  split /($COLOR*)/;
+  split /($COLOR+)/;
}

sub highlight_line {

gives me a 25% speed improvement, and the same output processing
git.git's entire git log -p output.

I thought that meant we could also optimize out the map call  
entirely,
and just use the first split (with *) to end up with a list of  
$COLOR

chunks and single characters, but it does not seem to work. So maybe I
am misreading something about what is going on.


I think our emails crossed in flight...

Using just the first split (with *) produces useless empty elements  
which I think ends up causing problems.  I suppose you could surround  
it with a grep /./ to remove them but that would defeat the point of  
the optimization.


-Kyle
--
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 v3] diff-highlight: do not split multibyte characters

[Kyle J. McKay]

When the input is UTF-8 and Perl is operating on bytes instead of
characters, a diff that changes one multibyte character to another
that shares an initial byte sequence will result in a broken diff
display as the common byte sequence prefix will be separated from
the rest of the bytes in the multibyte character.

For example, if a single line contains only the unicode character
U+C9C4 (encoded as UTF-8 0xEC, 0xA7, 0x84) and that line is then
changed to the unicode character U+C9C0 (encoded as UTF-8 0xEC,
0xA7, 0x80), when operating on bytes diff-highlight will show only
the single byte change from 0x84 to 0x80 thus creating invalid UTF-8
and a broken diff display.

Fix this by putting Perl into character mode when splitting the line
and then back into byte mode after the split is finished.

The utf8::xxx functions require Perl 5.8 so we require that as well.

Also, since we are mucking with code in the split_line function, we
change a '*' quantifier to a '+' quantifier when matching the $COLOR
expression which has the side effect of speeding everything up while
eliminating useless '' elements in the returned array.

Reported-by: Yi EungJun semtlen...@gmail.com
Signed-off-by: Kyle J. McKay mack...@gmail.com
---

On Apr 2, 2015, at 19:19, Yi, EungJun wrote:
 I timed this one versus the existing diff-highlight. It's about 7%
 slower. That's not great, but is acceptable to me. The  
 String::Multibyte
 version was a lot faster, which was nice (but I'm still unclear on
 _why_).

 I think the reason is here:

 sub split_line {
   local $_ = shift;
   return map { /$COLOR/ ? $_ : ($mbcs ? $mbcs-strsplit('', $_) :  
 split //) }
  split /($COLOR)/;
 }

 I removed * from split /($COLOR*)/. Actually I don't know why *
 was required but I need to remove it to make my patch works correctly.

 On Fri, Apr 3, 2015 at 10:24 AM, Jeff King p...@peff.net wrote:
 EungJun, does this version meet your needs?

This version differs from the former as follows:

1) Slightly faster code that eliminates the need for Encode::is_utf8.

2) The '*' quantifier is changed to '+' in the split_line regexs which  
was probably the original intent anyway as using '*' generates useless  
empty elements.  This has the side effect of greatly increasing the  
speed so the tiny speed penalty for the UTF-8 checking is vastly  
overwhelmed by the overall speed up. :)

3) The 'use 5.008;' line has been added since the utf8::xxx functions  
require Perl 5.8

-Kyle

 contrib/diff-highlight/diff-highlight | 9 +++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/contrib/diff-highlight/diff-highlight 
b/contrib/diff-highlight/diff-highlight
index 08c88bbc..ffefc31a 100755
--- a/contrib/diff-highlight/diff-highlight
+++ b/contrib/diff-highlight/diff-highlight
@@ -1,5 +1,6 @@
 #!/usr/bin/perl
 
+use 5.008;
 use warnings FATAL = 'all';
 use strict;
 
@@ -164,8 +165,12 @@ sub highlight_pair {
 
 sub split_line {
local $_ = shift;
-   return map { /$COLOR/ ? $_ : (split //) }
-  split /($COLOR*)/;
+   return utf8::decode($_) ?
+   map { utf8::encode($_); $_ }
+   map { /$COLOR/ ? $_ : (split //) }
+   split /($COLOR+)/ :
+   map { /$COLOR/ ? $_ : (split //) }
+   split /($COLOR+)/;
 }
 
 sub highlight_line {
---
--
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] diff-highlight: Fix broken multibyte string

[Kyle J. McKay]

On Apr 2, 2015, at 18:24, Jeff King wrote:


On Thu, Apr 02, 2015 at 05:49:24PM -0700, Kyle J. McKay wrote:


Subject: [PATCH v2] diff-highlight: do not split multibyte characters

When the input is UTF-8 and Perl is operating on bytes instead
of characters, a diff that changes one multibyte character to
another that shares an initial byte sequence will result in a
broken diff display as the common byte sequence prefix will be
separated from the rest of the bytes in the multibyte character.


Thanks, I had a feeling we should be able to do something with perl's
builtin utf8 support.  This doesn't help people with other encodings,


It should work as well as the original did for any 1-byte encoding.   
That is, if it's not valid UTF-8 it should pass through unchanged and  
any single byte encoding should just work.  But, as you point out,  
multibyte encodings other than UTF-8 won't work, but they should  
behave the same as they did before.



but I'm not sure the original was all that helpful either (in that we
don't actually _know_ the file encodings in the first place).


I think it should work fine on any single byte encoding (i.e. ISO-8859- 
x, WINDOWS-1252, etc.).



I timed this one versus the existing diff-highlight. It's about 7%
slower.


I'd expect that, we're doing extra work we weren't doing before.


That's not great, but is acceptable to me. The String::Multibyte
version was a lot faster, which was nice (but I'm still unclear on
_why_).


Must be the mbcs-strsplit routine has special case code for splitting  
on '' to just split on character boundaries.



Fix this by putting Perl into character mode when splitting the
line and then back into byte mode after the split is finished.


I also wondered if we could simply put stdin into utf8 mode. But it
looks like it will barf whenever it gets invalid utf8. Checking for
valid utf8 and only doing the multi-byte split in that case (as you do
here) is a lot more robust.


While the utf8::xxx functions are built-in and do not require
any 'use' statement, the utf8::is_utf8 function did not appear
until Perl 5.8.1, but is identical to the Encode::is_utf8
function which is available in 5.8 so we use that instead of
utf8::is_utf8.


Makes sense. I'm happy enough listing perl 5.8 as a dependency.


Maybe that should be added.  The rest of Git's perl code seems to have  
a 'use 5.008;' already, so I figured that was a reasonable  
dependency.  :)


-Kyle
--
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] diff-highlight: Fix broken multibyte string

[Kyle J. McKay]

On Mar 30, 2015, at 15:16, Jeff King wrote:

 Yeah, I agree the current output is not ideal, and this should address
 the problem. I was worried that multi-byte splitting would make things
 slower, but in my tests, it actually speeds things up!

[...]

 Unfortunately, String::Multibyte is not a standard module, and is not
 even packed for Debian systems (I got mine from CPAN). Can we make
 this
 a conditional include (e.g., 'eval require String::Multibyte' in
 get_mbcs, and return undef if that fails?). Then people without it can
 still use the script.

[...]

 Yuck. This is a lot more intimate with String::Multibyte's
 implementation than I'd like to be.

So I was curious about this and played with it and was able to
reproduce the problem as described.

Here's an alternate fix that should work for everyone with Perl 5.8
or later.

-Kyle

-- 8 --
Subject: [PATCH v2] diff-highlight: do not split multibyte characters

When the input is UTF-8 and Perl is operating on bytes instead
of characters, a diff that changes one multibyte character to
another that shares an initial byte sequence will result in a
broken diff display as the common byte sequence prefix will be
separated from the rest of the bytes in the multibyte character.

For example, if a single line contains only the unicode
character U+C9C4 (encoded as UTF-8 0xEC, 0xA7, 0x84) and that
line is then changed to the unicode character U+C9C0 (encoded as
UTF-8 0xEC, 0xA7, 0x80), when operating on bytes diff-highlight
will show only the single byte change from 0x84 to 0x80 thus
creating invalid UTF-8 and a broken diff display.

Fix this by putting Perl into character mode when splitting the
line and then back into byte mode after the split is finished.

While the utf8::xxx functions are built-in and do not require
any 'use' statement, the utf8::is_utf8 function did not appear
until Perl 5.8.1, but is identical to the Encode::is_utf8
function which is available in 5.8 so we use that instead of
utf8::is_utf8.

Reported-by: Yi EungJun semtlen...@gmail.com
Signed-off-by: Kyle J. McKay mack...@gmail.com
---
 contrib/diff-highlight/diff-highlight | 7 +--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/contrib/diff-highlight/diff-highlight 
b/contrib/diff-highlight/diff-highlight
index 08c88bbc..8e9b5ada 100755
--- a/contrib/diff-highlight/diff-highlight
+++ b/contrib/diff-highlight/diff-highlight
@@ -2,6 +2,7 @@
 
 use warnings FATAL = 'all';
 use strict;
+use Encode ();
 
 # Highlight by reversing foreground and background. You could do
 # other things like bold or underline if you prefer.
@@ -164,8 +165,10 @@ sub highlight_pair {
 
 sub split_line {
local $_ = shift;
-   return map { /$COLOR/ ? $_ : (split //) }
-  split /($COLOR*)/;
+   utf8::decode($_);
+   return map { utf8::encode($_) if Encode::is_utf8($_); $_ }
+   map { /$COLOR/ ? $_ : (split //) }
+   split /($COLOR*)/;
 }
 
 sub highlight_line {
---
--
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: What's cooking in git.git (Apr 2015, #01; Thu, 2)

[Kyle J. McKay]

On Apr 2, 2015, at 15:09, Junio C Hamano wrote:


* jc/show-branch (2014-03-24) 5 commits
- show-branch: use commit slab to represent bitflags of arbitrary  
width

- show-branch.c: remove all_mask
- show-branch.c: abstract out flags operation
- show-branch.c: lift all_mask/all_revs to a global static
- show-branch.c: update comment style

Waiting for the final step to lift the hard-limit before sending it  
out.



May I ask, now that this topic is over 1-year old, please, what is the  
final step?


Perhaps someone might submit a patch... ;)

-Kyle
--
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: What's cooking in git.git (Mar 2015, #09; Thu, 26)

[Kyle J. McKay]

On Mar 26, 2015, at 14:09, Junio C Hamano wrote:


* jc/show-branch (2014-03-24) 5 commits
- show-branch: use commit slab to represent bitflags of arbitrary  
width

- show-branch.c: remove all_mask
- show-branch.c: abstract out flags operation
- show-branch.c: lift all_mask/all_revs to a global static
- show-branch.c: update comment style

Waiting for the final step to lift the hard-limit before sending it  
out.


May I ask, on the 1-year anniversary of this topic, please, what is  
the final step?


Perhaps someone might submit a patch...  ;)

-Kyle
--
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: What's cooking in git.git (Mar 2015, #07; Fri, 20)

[Kyle J. McKay]

On Mar 20, 2015, at 15:02, Junio C Hamano wrote:


* bc/object-id (2015-03-13) 10 commits
- apply: convert threeway_stage to object_id
- patch-id: convert to use struct object_id
- commit: convert parts to struct object_id
- diff: convert struct combine_diff_path to object_id
- bulk-checkin.c: convert to use struct object_id
- zip: use GIT_SHA1_HEXSZ for trailers
- archive.c: convert to use struct object_id
- bisect.c: convert leaf functions to use struct object_id
- define utility functions for object IDs
- define a structure for object IDs

Identify parts of the code that knows that we use SHA-1 hash to
name our objects too much, and use (1) symbolic constants instead
of hardcoded 20 as byte count and/or (2) use struct object_id
instead of unsigned char [20] for object names.

Will cook in 'next'.



Has this been merged to 'next'?  Even fetching github.com/gitster/ 
git.git I'm only seeing it in pu:


$ git rev-parse bc/object-id
d07d4ab401173a10173f2747cf5e0f074b6d2b39

$ git branch --contains d07d4ab401173a10173f2747cf5e0f074b6d2b39 --all
  bc/object-id
  jch
  pu
  remotes/github2/pu
  remotes/gob-private/pu
  remotes/gph/pu
  remotes/ko/pu
  remotes/repo/pu

-Kyle
--
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: What's cooking in git.git (Mar 2015, #07; Fri, 20)

[Kyle J. McKay]

On Mar 20, 2015, at 16:29, Stefan Beller wrote:

On Fri, Mar 20, 2015 at 4:24 PM, Kyle J. McKay mack...@gmail.com  
wrote:



On Mar 20, 2015, at 15:02, Junio C Hamano wrote:


* bc/object-id (2015-03-13) 10 commits

[snip]

Will cook in 'next'.


Has this been merged to 'next'?


Usually Junio writes the mail first and then does a git push all  
the branches

just before being done for the day. At least that's my suspicion as an
observer of
the timing when git fetch returns new shiny stuff and when these
emails are sent.



I would expect that if it said, Will merge to 'next'.

However the What's cooking in git.git (Mar 2015, #06; Tue, 17) also  
says Will cook in 'next' for this topic so I think that perhaps it's  
fallen through the cracks somehow.


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

Bug in fetch-pack.c, please confirm

[Kyle J. McKay]

Hi guys,

So I was looking at fetch-pack.c (from master @ 52cae643, but I think  
it's the same everywhere):

# Lines 626-648

 for (retval = 1, ref = *refs; ref ; ref = ref-next) {
 const unsigned char *remote = ref-old_sha1;
 unsigned char local[20];
 struct object *o;

 o = lookup_object(remote);
 if (!o || !(o-flags  COMPLETE)) {
 retval = 0;
 if (!args-verbose)
 continue;
 fprintf(stderr,
 want %s (%s)\n, sha1_to_hex(remote),
 ref-name);
 continue;
 }

 hashcpy(ref-new_sha1, local);
 if (!args-verbose)
 continue;
 fprintf(stderr,
 already have %s (%s)\n, sha1_to_hex(remote),
 ref-name);
 }

Peff, weren't you having some issue with want and have and hide refs?

Tell me please how the local variable above gets initialized?

It's declared on the stack inside the for loop scope so only  
guaranteed to have garbage.

It's passed to hashcpy which has this prototype:

  inline void hashcpy(unsigned char *sha_dst, const unsigned char *sha_src);

So it looks to me like garbage is copied into rev-new_sha1, yes?

Something's very wrong here.

It looks to me like the bug was introduced in 49bb805e (Do not ask for  
objects known to be complete. 2005-10-19).

I've taken a stab a a fix below.

-Kyle

-- 8 --
Subject: [PATCH] fetch-pack.c: do not use uninitialized sha1 value

Since 49bb805e (Do not ask for objects known to be complete. 2005-10-19)
when the read_ref call was replaced with a parse_object call, the
automatic variable 'local' containing an sha1 has been left uninitialized.

Subsequently in 1baaae5e (Make maximal use of the remote refs, 2005-10-28)
the parse_object call was replaced with a lookup_object call but still
the 'local' variable was left uninitialized.

However, it's used as the source to update another sha1 value in the case
that we already have it and in that case the other ref will end up with
whatever garbage was in the 'local' variable.

Fix this by removing the 'local' variable and using the value from the
result of the lookup_object call instead.

Signed-off-by: Kyle J. McKay mack...@gmail.com
---
 fetch-pack.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/fetch-pack.c b/fetch-pack.c
index 655ee642..c0b4d84f 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -621,29 +621,28 @@ static int everything_local(struct fetch_pack_args *args,
}
}
 
filter_refs(args, refs, sought, nr_sought);
 
for (retval = 1, ref = *refs; ref ; ref = ref-next) {
const unsigned char *remote = ref-old_sha1;
-   unsigned char local[20];
struct object *o;
 
o = lookup_object(remote);
if (!o || !(o-flags  COMPLETE)) {
retval = 0;
if (!args-verbose)
continue;
fprintf(stderr,
want %s (%s)\n, sha1_to_hex(remote),
ref-name);
continue;
}
 
-   hashcpy(ref-new_sha1, local);
+   hashcpy(ref-new_sha1, o-sha1);
if (!args-verbose)
continue;
fprintf(stderr,
already have %s (%s)\n, sha1_to_hex(remote),
ref-name);
}
return retval;
---
--
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: Bug in fetch-pack.c, please confirm

[Kyle J. McKay]

On Mar 15, 2015, at 00:30, Junio C Hamano wrote:


Junio C Hamano gits...@pobox.com writes:


Kyle J. McKay mack...@gmail.com writes:


Hi guys,

So I was looking at fetch-pack.c (from master @ 52cae643, but I  
think

it's the same everywhere):


...

-   hashcpy(ref-new_sha1, local);
+   hashcpy(ref-new_sha1, o-sha1);
if (!args-verbose)
continue;
fprintf(stderr,
already have %s (%s)\n, sha1_to_hex(remote),
ref-name);
}
return retval;
---


One thing I wonder is if this hashcpy() is doing anything useful,
though.  Is ref-new_sha1 used after we are done in this codepath,
or is the reason nobody noticed it is because it does not matter
whatever garbage is in that field nobody looks at it?


My thoughts exactly. hence the please confirm request.  I'm not  
familiar enough with the fetch pack code to know the answer off the  
top of my head.  I was hoping someone else who's been in the fetch  
pack code recently (*Hi Peff*) might just already know.  :)

--
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: Any chance for a Git v2.1.5 release?

[Kyle J. McKay]

The first two messages in this thread were dropped by the vger mailing  
list for some unknown reason.


In an attempt to make the mailing list archives of this thread  
complete, the original two messages in this thread are included below.


-Kyle

 BEGIN FIRST MESSAGE 
From: Kyle J. McKay mack...@gmail.com
Date: February 24, 2015 09:16:05 PST
To: Junio C Hamano gits...@pobox.com
Cc: Git Mailing List git@vger.kernel.org
Subject: Any chance for a Git v2.1.5 release?
Message-Id: c5211e53-8905-41c9-9d28-26d7bb51e...@gmail.com
Content-Type: text/plain; charset=US-ASCII; format=flowed; delsp=yes

As you know, I help out with repo.or.cz.  Recently the admins have  
been discussing upgrading the version of Git we use in order to  
support newer features such as pack bitmaps.  While we do use a  
slightly customized gitweb, we have always stuck to an official Git  
release for everything else.


Repo.or.cz provides fetch (git, http, https, ssh), browsing (gitweb)  
and push (ssh, https).  Additionally, created repositories can be  
mirrors (no push allowed) of other repositories (including svn via git- 
svn).  Email notification of ref changes (along with diffs) can also  
be requested.


We are finding that in order to upgrade Git at this point we would  
need to build a custom Git with cherry picked fixes for various issues  
that have come up or they would likely be triggered by one of the  
services repo.or.cz provides.  In addition, as there are numerous  
reports of an unresolved issue with git-svn starting with v2.2.0  
upgrading to v2.2.0 or later presents a problem since we have several  
mirrors that rely on git-svn.


Which brings us back to the subject of this email, is there any chance  
for a v2.1.5 release?


After reviewing a few release dates:

2011-06-26T12:41:26-07:00 v1.7.6
2012-02-05T23:51:07-08:00 v1.7.6.6

2013-11-27T12:14:52-08:00 v1.8.5
2014-02-13T13:42:01-08:00 v1.8.5.5

2014-02-14T11:36:11-08:00 v1.9.0
2014-05-30T10:15:10-07:00 v1.9.4

2014-05-28T11:04:29-07:00 v2.0.0
2014-07-30T14:20:01-07:00 v2.0.4

2014-08-15T15:09:28-07:00 v2.1.0
2014-10-29T10:48:57-07:00 v2.1.3

2014-11-26T13:18:43-08:00 v2.2.0
2015-01-12T14:06:20-08:00 v2.2.2

2015-02-05T13:24:05-08:00 v2.3.0

(I have omitted the dates of the 5 security releases on 2014-12-17 as  
that seems like an extraordinary event unlikely to be repeated.)


It appears that the average support lifespan of a Git release from  
initial release date through last released maintenance update is  
approximately 2-3 months with the 1.7.6 release being an exception at  
a little over 7 months.


If a v2.1.5 release is out of the question, would it be possible to  
periodically designate certain Git releases as long term support  
releases?  Meaning that they would receive maintenance updates (e.g.  
fixes for invalid memory accesses/crashes, regressions or security  
issues) for an extended period of time, say at least 6 months?



Here's the analysis that led to this request:


Goal 1: add support for symref=HEAD:refs/... capability

Goal 2: add support for pack bitmaps

Nice to have: The CVE-2014-9390 fix, but repo.or.cz does not create  
any working trees so it's not mandatory.



Goal 1:

symref=HEAD:refs/... requires at least Git 1.8.4.3.  However,  
repo.or.cz runs git-daemon with read-only access to the repositories  
and since Git 1.8.4.2 shallow clones require write access.


This was corrected in v2.0.0.  So at least v2.0.0 would be needed for  
symref=HEAD:refs/



Goal 2:

Pack bitmap support was added in v2.0.0, but it's probably not a good  
idea to run without 21134714 (pack-objects: turn off bitmaps when we  
split packs) just in case which requires at least v2.1.3.



Nice to have:

If at least v2.1.3 is required, then we might as well use v2.1.4 since  
the primary change from v2.1.3 to v2.1.4 is the addition of the  
CVE-2014-9390 fix.



What about a later release, v2.2.0 or later?


git-svn is reported to suffer from occasional .git/ 
Git_svn_hash_XX: Bad file descriptor errors since v2.2.0 making  
that a non-starter.  No fix is currently available in the Git  
repository.


Since 660c889e (sha1_file: add for_each iterators for loose and packed  
objects) loose objects in alternates directories may not be found when  
pruning.  This affects v2.2.0 and later.  A fix is currently in  
master.  This is an absolute must have for repo.or.cz as alternates  
are used to implement repository forks.


Since d38379e (make update-server-info more robust), affecting v2.2.x,  
the files used by non-smart HTTP clients could have the wrong  
permissions.  This might preclude them from being updated correctly on  
repo.or.cz.  It would require research to see if repo.or.cz is  
affected.  The fix for this d91175b2 (update-server-info: create info/ 
* with mode 0666) was released in v2.3.0.



So why not v2.1.4 then?


There's an XDL_FAST_HASH performance regression that affects v1.7.11  
and later [1].  But that can be turned

Re: [PATCH v2 00/10] Use a structure for object IDs.

[Kyle J. McKay]

On Mar 7, 2015, at 15:23, brian m. carlson wrote:
This is a patch series to convert some of the relevant uses of  
unsigned

char [20] to struct object_id.

brian m. carlson (10):


All patches applied for me (to master) and all tests pass.

Tested-by: Kyle J. McKay



 Define a structure for object IDs.


Comments in reply to the patch.



 Define utility functions for object IDs.
 bisect.c: convert leaf functions to use struct object_id
 archive.c: convert to use struct object_id
 zip: use GIT_SHA1_HEXSZ for trailers
 bulk-checkin.c: convert to use struct object_id
 diff: convert struct combine_diff_path to object_id
 commit: convert parts to struct object_id
 patch-id: convert to use struct object_id
 apply: convert threeway_stage to object_id


These all look good, the conversions are simple and easy to follow.


On Mar 7, 2015, at 23:43, Junio C Hamano wrote:

brian m. carlson sand...@crustytoothpaste.net writes:

Certain parts of the code have to be converted before others to  
keep the

patch sizes small, maintainable, and bisectable, so functions and
structures that are used across the codebase (e.g. struct object)  
will

be converted later.  Conversion has been done in a somewhat haphazard
manner by converting modules with leaf functions and less commonly  
used

structs first.


That leaf-first approach sounds very sensible.

In the medium term, I wonder if the changes can progress faster and
in a less error prone way if you first used GIT_SHA1_RAWSZ in places
that cannot be immediately converted to the struct yet.  That way,
we will be easily tell by git grep GIT_SHA1_RAWSZ how many more
places need treatment.  I do not know if that is all that useful
offhand, though.  Those places need to be touched in the second pass
to use the struct again, after the s/\[20\]/[GIT_SHA1_RAWSZ]/
first pass.


I definitely noticed the leaf-first approach as I was looking through  
the patches where, for example (03/10), this prototype was left  
unchanged:


static int register_ref(const char *refname, const unsigned char *sha1,
int flags, void *cb_data)

but its contents got the update leaving it half converted.  As  
mentioned above this makes the patches more manageable, maintainable  
and bisectable.  However, these functions could be converted to take a  
typedef (a quick grep of 'CodingGuidelines' does not mention typedef)  
and perhaps, as Junio mentions above, help the changes progress faster  
by making it easier to find the affected code (e.g. changing or  
removing the typedef would make the compiler find them for you).


For example, if we added this to object.h:

typedef unsigned char sha1raw_t[GIT_SHA1_RAWSZ];

then the above prototype could be immediately converted to (and this  
does compile and pass all the tests):


static int register_ref(const char *refname, const sha1raw_t sha,
int flags, void *cb_data)

So that together with Junio's suggestion above (and perhaps also a  
sha1hex_t type) would help mark everything in the first pass that  
needs to be touched again in the second pass.  (I'm just throwing out  
some typedef names as an example, there may be more preferable names  
to sha1raw_t and sha1hex_t, but those names would end up being  
replaced eventually anyway.)


-Kyle 
--

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] protocol upload-pack-v2

[Kyle J. McKay]

On Mar 9, 2015, at 18:38, Duy Nguyen wrote:

A minor point on capability negotiation. I remember why I passed
capabilities via command line instead of this proposal. With this
proposal, daemon.c does not recognize i18n capability and cannot
switch to the correct language before it reports an error.

But perhaps I approached it the wrong way. Instead of letting the
daemon produce non-English language directly, if could sort of
standardize the messages and send an error code back in ERR line,
together with an English message (current pack-protocol.txt says ERR
followed by explanation-text). The client can use this error code to
print non-English messages if it wants to. Perhaps we can reuse http
(or ftp) return codes or some other protocol then customize to fit
Git's needs.


May I suggest that you take a look at RFC 2034 [1].  It describes  
almost exactly this same situation and how SMTP was enhanced to  
support error code numbers that could then be translated.


No reason this enhancement needs to be restricted to protocol v2 if  
it's just an enhancedstatuscodes capability the server sends back to  
indicate that its ERR text is in that format.


-Kyle

[1] http://tools.ietf.org/html/rfc2034
--
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] t7510: do not fail when gpg warns about insecure memory

[Kyle J. McKay]

Depending on how gpg was built, it may issue the following
message to stderr when run:

  Warning: using insecure memory!

When the test is collecting gpg output it is therefore not
enough to just match on a gpg:  prefix it must also match
on a Warning:  prefix wherever it needs to match lines
that have been produced by gpg.

Signed-off-by: Kyle J. McKay mack...@gmail.com
---

How about this patch instead.  It just treats Warning: lines as gpg  
output and the test still passes when Warning: using insecure memory  
shows up.

-Kyle

 t/t7510-signed-commit.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/t/t7510-signed-commit.sh b/t/t7510-signed-commit.sh
index 474dab38..3cef18cf 100755
--- a/t/t7510-signed-commit.sh
+++ b/t/t7510-signed-commit.sh
@@ -86,8 +86,8 @@ test_expect_success GPG 'show signed commit with signature' '
git show -s --show-signature initial show 
git verify-commit -v initial verify.1 2verify.2 
git cat-file commit initial cat 
-   grep -v gpg:  show show.commit 
-   grep gpg:  show show.gpg 
+   grep -v -e gpg:  -e Warning:  show show.commit 
+   grep -e gpg:  -e Warning:  show show.gpg 
grep -v ^  cat | grep -v ^gpgsig  cat.commit 
test_cmp show.commit commit 
test_cmp show.gpg verify.2 
---
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] help.c: use SHELL_PATH instead of hard-coded /bin/sh

[Kyle J. McKay]

On Mar 7, 2015, at 23:52, Junio C Hamano wrote:

Kyle J. McKay mack...@gmail.com writes:


If the user has set SHELL_PATH in the Makefile then we
should respect that value and use it.

Signed-off-by: Kyle J. McKay mack...@gmail.com
---
builtin/help.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/builtin/help.c b/builtin/help.c
index 6133fe49..2ae8a1e9 100644
--- a/builtin/help.c
+++ b/builtin/help.c
@@ -171,7 +171,7 @@ static void exec_man_cmd(const char *cmd, const  
char *page)

{
struct strbuf shell_cmd = STRBUF_INIT;
strbuf_addf(shell_cmd, %s %s, cmd, page);
-   execl(/bin/sh, sh, -c, shell_cmd.buf, (char *)NULL);
+   execl(SHELL_PATH, SHELL_PATH, -c, shell_cmd.buf, (char *)NULL);


It is a common convention to make the first argument the command
name without its path, and this change breaks that convention.


Hmpf.  I present these for your consideration:

$ sh -c 'echo $0'
sh
$ /bin/sh -c 'echo $0'
/bin/sh
$ cd /etc
$ ../bin/sh -c 'echo $0'
../bin/sh

I always thought it was the actual argument used to invoke the item.   
If the item is in the PATH and was invoked with a bare word then arg0  
would be just the bare word or possibly the actual full pathname as  
found in PATH.  Whereas if it's invoked with a path (relative or  
absolute) that would passed instead.



Does it matter, or would it break something?  I recall that some
implementations of shell (e.g. bash) change their behaviour
depending on how they are invoked (e.g. ln -s bash /bin/sh makes
it run in posix mode) but I do not know if they do so by paying
attention to their argv[0].


Several shells are sensitive to argv[0] in that if it starts with a  
'-' then they become a login shell.  Setting SHELL_PATH to anything  
that is not an absolute path is likely to break things in other ways  
though so that doesn't seem like a possibility here.



There might be other fallouts I do not
think of offhand here.

I do not have an objection to what these patches want to do, though.


I also have no objection to changing it to:


-   execl(/bin/sh, sh, -c, shell_cmd.buf, (char *)NULL);
+	execl(SHELL_PATH, basename(SHELL_PATH), -c, shell_cmd.buf, (char  
*)NULL);


just to maintain the current behavior.

Would you be able to squash that change in or shall I re-roll?

-Kyle

--
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] t7510: do not fail when gpg warns about insecure memory

[Kyle J. McKay]

On Mar 8, 2015, at 18:22, brian m. carlson wrote:


On Sun, Mar 08, 2015 at 06:15:55PM -0400, Eric Sunshine wrote:

On Sun, Mar 8, 2015 at 6:04 PM, brian m. carlson
sand...@crustytoothpaste.net wrote:

Perhaps this is better?

Unfortunately when running the test, that message is found in the   
standard
output of git show -s --show-signature, but in the standard  error  
of git
verify-commit -v causing the comparisons of both standard  output  
and

standard error to fail.


That doesn't help me parse it any better.  It's the but without a
corresponding not which seems to be throwing me off. Typically, one
would write this but not that or not this but that. I can't tell
if there is a not missing or if the but is supposed to be an  
and

or if something else was intended.


The intended meaning is and with the additional sense of contrast.  
The sentence, if read with verbal emphasis, is, …is found in the  
standard *output* of git show -s --signature, but in the standard  
*error* of git verify-commit -v, thus demonstrating why the test  
fails: the pairs of output files don't match up.


Maybe you can suggest a less confusing wording.


I like Brian's wording, but how about this slight variation, does this  
parse any better for you?


Unfortunately when running the test, while that message is found in
the standard output of git show -s --show-signature, it is found in
the standard error of git verify-commit -v causing the comparisons
of both standard output and standard error to fail.

-Kyle

--
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] t5528: do not fail with FreeBSD shell

[Kyle J. McKay]

On Mar 8, 2015, at 10:56, Jeff King wrote:

On Sun, Mar 08, 2015 at 08:37:50AM -0700, Kyle J. McKay wrote:


The FreeBSD shell converts this expression:

 git ${1:+-c push.default=$1} push

to this when $1 is not empty:

 git -c push.default=$1 push

which causes git to fail.


Hmph, just when I thought I knew about all of the weird shell  
quirks. :)


I am not convinced this isn't a violation of POSIX (which specifies  
that

field splitting is done on the results of parameter expansions outside
of double-quotes). But whether it is or not, we have to live with it.


That's not the only problem the shell has, t5560 had an issue, rebase  
had issues.  They've have been worked around.  It probably also  
affects related BSDs' shells as well (at least older versions that  
didn't change the shell).



For my own curiosity, what does:

 foo='with space'
 printf %s\n ${foo:+first $foo}

print? That is, are the double-quotes even doing anything on such a
shell? On bash and dash, it prints:

 first
 with space

which is what I would expect.



$ foo='with space'
$ printf %s\n ${foo:+first $foo}
first with space

I also happen to have a handy-dandy test program called showargs.

$ foo='with space'
$ showargs ${foo:+first $foo}
uid=1001 euid=1001
gid=1001 egid=1001
umask(octal)=022
stdin=/dev/pts/12 stdout=/dev/pts/12 stderr=/dev/pts/12
pid=5261
$0=showargs
$1=first with space

So no quotes are being passed on.  Of course bash works just fine.


So does ash (0.5.7, packaged for
Debian), which is what I _thought_ FreeBSD's shell was based on. But
clearly there is some divergence.


I like to test on FreeBSD 8, which is slightly older, once in a while  
to make sure I catch stuff like this.  :)


Running ident /bin/sh shows a bunch of source file names which  
matches up pretty well with the dash distribution so I'm pretty sure  
it's just a much older ancestor of dash/ash.


If I run dash 0.5.6 (installed via FreeBSD ports), it works properly  
too.



I guess they are getting eaten by your shell, otherwise we would pass
them along to git in the test script, which would complain.


When I run t5528 with -v -x -d -i this is where it dies (without the  
fix):


+ git '-c push.default=upstream' push
Unknown option: -c push.default=upstream

So yeah, the quotes are gone, but no word-splitting occurred.

--
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] t7510: do not fail when gpg warns about insecure memory

[Kyle J. McKay]

Depending on how gpg was built, it may issue the following
message to stderr when run:

  Warning: using insecure memory!

Unfortunately when running the test, that message gets
collected in the stdout result of git show -s --show-signature
but is collected in the stderr result of git verify-commit -v
causing both the stdout and stderr result comparisions to fail.

Since checking for secure memory use by gpg is not the point of
this test, filter out such messages to allow the test to pass
even when gpg is using insecure memory.

Signed-off-by: Kyle J. McKay mack...@gmail.com
---
 t/t7510-signed-commit.sh | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/t/t7510-signed-commit.sh b/t/t7510-signed-commit.sh
index 474dab38..e86923bc 100755
--- a/t/t7510-signed-commit.sh
+++ b/t/t7510-signed-commit.sh
@@ -84,9 +84,10 @@ test_expect_success GPG 'verify and show signatures' '
 test_expect_success GPG 'show signed commit with signature' '
git show -s initial commit 
git show -s --show-signature initial show 
-   git verify-commit -v initial verify.1 2verify.2 
+   git verify-commit -v initial verify.1 2verify.2.out 
git cat-file commit initial cat 
-   grep -v gpg:  show show.commit 
+   grep -v -e gpg:  -e insecure memory show show.commit 
+   grep -v insecure memory verify.2.out verify.2 
grep gpg:  show show.gpg 
grep -v ^  cat | grep -v ^gpgsig  cat.commit 
test_cmp show.commit commit 
---
--
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] t5528: do not fail with FreeBSD shell

[Kyle J. McKay]

The FreeBSD shell converts this expression:

  git ${1:+-c push.default=$1} push

to this when $1 is not empty:

  git -c push.default=$1 push

which causes git to fail.  To avoid this we simply break up the
expansion into two parts so that the whitespace which creates
two arguments instead of one is outside the ${...} like so:

  git ${1:+-c} ${1:+push.default=$1} push

This has the desired effect on all platforms allowing the test
to pass on FreeBSD.

Signed-off-by: Kyle J. McKay mack...@gmail.com
---
 t/t5528-push-default.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/t/t5528-push-default.sh b/t/t5528-push-default.sh
index cc745190..73f4bb63 100755
--- a/t/t5528-push-default.sh
+++ b/t/t5528-push-default.sh
@@ -26,7 +26,7 @@ check_pushed_commit () {
 # $2 = expected target branch for the push
 # $3 = [optional] repo to check for actual output (repo1 by default)
 test_push_success () {
-   git ${1:+-c push.default=$1} push 
+   git ${1:+-c} ${1:+push.default=$1} push 
check_pushed_commit HEAD $2 $3
 }
 
@@ -34,7 +34,7 @@ test_push_success () {
 # check that push fails and does not modify any remote branch
 test_push_failure () {
git --git-dir=repo1 log --no-walk --format='%h %s' --all expect 
-   test_must_fail git ${1:+-c push.default=$1} push 
+   test_must_fail git ${1:+-c} ${1:+push.default=$1} push 
git --git-dir=repo1 log --no-walk --format='%h %s' --all actual 
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

[PATCH 1/2] configure: support HAVE_BSD_SYSCTL option

[Kyle J. McKay]

On BSD-compatible systems some information such as the number
of available CPUs may only be available via the sysctl function.

Add support for a HAVE_BSD_SYSCTL option complete with autoconf
support and include the sys/syctl.h header when the option is
enabled to make the sysctl function available.

Signed-off-by: Kyle J. McKay mack...@gmail.com
---
 Makefile  |  6 ++
 config.mak.uname  |  5 +
 configure.ac  | 23 +++
 git-compat-util.h |  3 +++
 4 files changed, 37 insertions(+)

diff --git a/Makefile b/Makefile
index 44f1dd10..5f3987fe 100644
--- a/Makefile
+++ b/Makefile
@@ -357,6 +357,8 @@ all::
 # and define it to no if you need to remove the parentheses () around the
 # constant.  The default is auto, which means to use parentheses if your
 # compiler is detected to support it.
+#
+# Define HAVE_BSD_SYSCTL if your platform has a BSD-compatible sysctl function.
 
 GIT-VERSION-FILE: FORCE
@$(SHELL_PATH) ./GIT-VERSION-GEN
@@ -1431,6 +1433,10 @@ ifdef HAVE_CLOCK_MONOTONIC
BASIC_CFLAGS += -DHAVE_CLOCK_MONOTONIC
 endif
 
+ifdef HAVE_BSD_SYSCTL
+   BASIC_CFLAGS += -DHAVE_BSD_SYSCTL
+endif
+
 ifeq ($(TCLTK_PATH),)
 NO_TCLTK = NoThanks
 endif
diff --git a/config.mak.uname b/config.mak.uname
index b64b63c3..f4e77cb9 100644
--- a/config.mak.uname
+++ b/config.mak.uname
@@ -107,6 +107,7 @@ ifeq ($(uname_S),Darwin)
COMPAT_OBJS += compat/precompose_utf8.o
BASIC_CFLAGS += -DPRECOMPOSE_UNICODE
BASIC_CFLAGS += -DPROTECT_HFS_DEFAULT=1
+   HAVE_BSD_SYSCTL = YesPlease
 endif
 ifeq ($(uname_S),SunOS)
NEEDS_SOCKET = YesPlease
@@ -199,6 +200,7 @@ ifeq ($(uname_S),FreeBSD)
PYTHON_PATH = /usr/local/bin/python
HAVE_PATHS_H = YesPlease
GMTIME_UNRELIABLE_ERRORS = UnfortunatelyYes
+   HAVE_BSD_SYSCTL = YesPlease
 endif
 ifeq ($(uname_S),OpenBSD)
NO_STRCASESTR = YesPlease
@@ -208,6 +210,7 @@ ifeq ($(uname_S),OpenBSD)
BASIC_CFLAGS += -I/usr/local/include
BASIC_LDFLAGS += -L/usr/local/lib
HAVE_PATHS_H = YesPlease
+   HAVE_BSD_SYSCTL = YesPlease
 endif
 ifeq ($(uname_S),MirBSD)
NO_STRCASESTR = YesPlease
@@ -215,6 +218,7 @@ ifeq ($(uname_S),MirBSD)
USE_ST_TIMESPEC = YesPlease
NEEDS_LIBICONV = YesPlease
HAVE_PATHS_H = YesPlease
+   HAVE_BSD_SYSCTL = YesPlease
 endif
 ifeq ($(uname_S),NetBSD)
ifeq ($(shell expr $(uname_R) : '[01]\.'),2)
@@ -225,6 +229,7 @@ ifeq ($(uname_S),NetBSD)
USE_ST_TIMESPEC = YesPlease
NO_MKSTEMPS = YesPlease
HAVE_PATHS_H = YesPlease
+   HAVE_BSD_SYSCTL = YesPlease
 endif
 ifeq ($(uname_S),AIX)
DEFAULT_PAGER = more
diff --git a/configure.ac b/configure.ac
index 55e5a9b3..bbdde85c 100644
--- a/configure.ac
+++ b/configure.ac
@@ -1046,6 +1046,29 @@ GIT_CONF_SUBST([NO_INITGROUPS])
 #
 # Define NO_ICONV if your libc does not properly support iconv.
 
+AC_DEFUN([BSD_SYSCTL_SRC], [
+AC_LANG_PROGRAM([[
+#include stddef.h
+#include sys/types.h
+#include sys/sysctl.h
+]],[[
+int val, mib[2];
+size_t len;
+mib[0] = CTL_HW;
+mib[1] = 1;
+len = sizeof(val);
+return sysctl(mib, 2, val, len, NULL, 0) ? 1 : 0;
+]])])
+
+#
+# Define HAVE_BSD_SYSCTL=YesPlease if a BSD-compatible sysctl function is 
available.
+AC_MSG_CHECKING([for BSD sysctl])
+AC_COMPILE_IFELSE([BSD_SYSCTL_SRC],
+   [AC_MSG_RESULT([yes])
+   HAVE_BSD_SYSCTL=YesPlease],
+   [AC_MSG_RESULT([no])
+   HAVE_BSD_SYSCTL=])
+GIT_CONF_SUBST([HAVE_BSD_SYSCTL])
 
 ## Other checks.
 # Define USE_PIC if you need the main git objects to be built with -fPIC
diff --git a/git-compat-util.h b/git-compat-util.h
index a3095be9..50691d3c 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -127,6 +127,9 @@
 #else
 #include poll.h
 #endif
+#ifdef HAVE_BSD_SYSCTL
+#include sys/sysctl.h
+#endif
 
 #if defined(__MINGW32__)
 /* pull in Windows compatibility stuff */
---
--
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] thread-utils.c: detect CPU count on older BSD-like systems

[Kyle J. McKay]

Not all systems support using sysconf to detect the number
of available CPU cores.  Older BSD and BSD-derived systems
only provide the information via the sysctl function.

If HAVE_BSD_SYSCTL is defined attempt to retrieve the number
of available CPU cores using the sysctl function.

If HAVE_BSD_SYSCTL is not defined or the sysctl function
fails, we still attempt to get the information via sysconf.

Signed-off-by: Kyle J. McKay mack...@gmail.com
---
 thread-utils.c | 18 +-
 1 file changed, 17 insertions(+), 1 deletion(-)

diff --git a/thread-utils.c b/thread-utils.c
index 97396a75..a2135e07 100644
--- a/thread-utils.c
+++ b/thread-utils.c
@@ -35,7 +35,23 @@ int online_cpus(void)
 
if (!pstat_getdynamic(psd, sizeof(psd), (size_t)1, 0))
return (int)psd.psd_proc_cnt;
-#endif
+#elif defined(HAVE_BSD_SYSCTL)  defined(HW_NCPU)
+   int mib[2];
+   size_t len;
+   int cpucount;
+
+   mib[0] = CTL_HW;
+#  ifdef HW_AVAILCPU
+   mib[1] = HW_AVAILCPU;
+   len = sizeof(cpucount);
+   if (!sysctl(mib, 2, cpucount, len, NULL, 0))
+   return cpucount;
+#  endif /* HW_AVAILCPU */
+   mib[1] = HW_NCPU;
+   len = sizeof(cpucount);
+   if (!sysctl(mib, 2, cpucount, len, NULL, 0))
+   return cpucount;
+#endif /* defined(HAVE_BSD_SYSCTL)  defined(HW_NCPU) */
 
 #ifdef _SC_NPROCESSORS_ONLN
if ((ncpus = (long)sysconf(_SC_NPROCESSORS_ONLN))  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

[PATCH] git-instaweb: allow running in a working tree subdirectory

[Kyle J. McKay]

Signed-off-by: Kyle J. McKay mack...@gmail.com
---
 git-instaweb.sh | 1 +
 1 file changed, 1 insertion(+)

diff --git a/git-instaweb.sh b/git-instaweb.sh
index 513efa66..4c0af04f 100755
--- a/git-instaweb.sh
+++ b/git-instaweb.sh
@@ -20,6 +20,7 @@ start  start the web server
 restartrestart the web server
 
 
+SUBDIRECTORY_OK=Yes
 . git-sh-setup
 
 fqgitdir=$GIT_DIR
---
--
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: Please consider adding a -f switch to git-clone (or something similar)

[Kyle J. McKay]

On Mar 7, 2015, at 17:53, Diego Viola wrote:

Something like this is the scenario I'm talking about:

$ mkdir non-empty-dir
$ cd non-empty-dir
$ touch foo bar baz
$ git clone -f url:user/dotfiles.git .
$ git status
On branch master
Your branch is up-to-date with 'origin/master'.
Untracked files:
 (use git add file... to include in what will be committed)

   bar
   baz
   foo

nothing added to commit but untracked files present (use git add  
to track)


Have you considered using an alias?

git config --global alias.irfc \
  '!sh -c '\''git init  git remote add origin $1  git fetch   
git checkout ${2:-master}'\'' sh'


(You'll likely have to carefully unwrap that line above.)

Then you get

  git irfc URL [branch]

where branch defaults to master.
So your scenario would become just:



$ mkdir non-empty-dir
$ cd non-empty-dir
$ touch foo bar baz
$ git irfc url:user/dotfiles.git
$ git status
On branch master
Your branch is up-to-date with 'origin/master'.
Untracked files:
 (use git add file... to include in what will be committed)

   bar
   baz
   foo

nothing added to commit but untracked files present (use git add to  
track)



-Kyle

P.S. irfc = init, remote, fetch, checkout.  But do make up a better  
name. :)

--
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] help.c: use SHELL_PATH instead of hard-coded /bin/sh

[Kyle J. McKay]

If the user has set SHELL_PATH in the Makefile then we
should respect that value and use it.

Signed-off-by: Kyle J. McKay mack...@gmail.com
---
 builtin/help.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/builtin/help.c b/builtin/help.c
index 6133fe49..2ae8a1e9 100644
--- a/builtin/help.c
+++ b/builtin/help.c
@@ -171,7 +171,7 @@ static void exec_man_cmd(const char *cmd, const char *page)
 {
struct strbuf shell_cmd = STRBUF_INIT;
strbuf_addf(shell_cmd, %s %s, cmd, page);
-   execl(/bin/sh, sh, -c, shell_cmd.buf, (char *)NULL);
+   execl(SHELL_PATH, SHELL_PATH, -c, shell_cmd.buf, (char *)NULL);
warning(_(failed to exec '%s': %s), cmd, strerror(errno));
 }
 
---
--
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-compat-util.h: move SHELL_PATH default into header

[Kyle J. McKay]

If SHELL_PATH is not defined we use /bin/sh.  However,
run-command.c is not the only file that needs to use
the default value so move it into a common header.

Signed-off-by: Kyle J. McKay mack...@gmail.com
---
 git-compat-util.h | 4 
 run-command.c | 4 
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/git-compat-util.h b/git-compat-util.h
index a3095be9..fbfd10da 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -876,4 +876,8 @@ struct tm *git_gmtime_r(const time_t *, struct tm *);
 #define USE_PARENS_AROUND_GETTEXT_N 1
 #endif
 
+#ifndef SHELL_PATH
+# define SHELL_PATH /bin/sh
+#endif
+
 #endif
diff --git a/run-command.c b/run-command.c
index 0b432cc9..3afb124c 100644
--- a/run-command.c
+++ b/run-command.c
@@ -4,10 +4,6 @@
 #include sigchain.h
 #include argv-array.h
 
-#ifndef SHELL_PATH
-# define SHELL_PATH /bin/sh
-#endif
-
 void child_process_init(struct child_process *child)
 {
memset(child, 0, sizeof(*child));
---
--
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] git-instaweb: use @SHELL_PATH@ instead of /bin/sh

[Kyle J. McKay]

If the user has configured a value for SHELL_PATH then
be sure to use it for any generated scripts instead of
hard-coding /bin/sh.

The first line of the script is handled specially, but
the embedded #!/bin/sh line in the here document will
not be automatically updated unless it uses @SHELL_PATH@.

Signed-off-by: Kyle J. McKay mack...@gmail.com
---
 git-instaweb.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/git-instaweb.sh b/git-instaweb.sh
index 513efa66..7b6ba2bc 100755
--- a/git-instaweb.sh
+++ b/git-instaweb.sh
@@ -204,7 +204,7 @@ webrick_conf () {
# actual gitweb.cgi using a shell script to force it
   wrapper=$fqgitdir/gitweb/$httpd/wrapper.sh
cat  $wrapper EOF
-#!/bin/sh
+#!@SHELL_PATH@
 # we use this shell script wrapper around the real gitweb.cgi since
 # there appears to be no other way to pass arbitrary environment variables
 # into the CGI process
---
--
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] imap-send: use cURL automatically when NO_OPENSSL defined

[Kyle J. McKay]

If both USE_CURL_FOR_IMAP_SEND and NO_OPENSSL are defined do
not force the user to add --curl to get a working git imap-send
command.

Instead automatically select --curl and warn and ignore the
--no-curl option.  And while we're in there, correct the
warning message when --curl is requested but not supported.

Signed-off-by: Kyle J. McKay mack...@gmail.com
---
 Documentation/git-imap-send.txt |  3 ++-
 imap-send.c | 17 +++--
 2 files changed, 17 insertions(+), 3 deletions(-)

diff --git a/Documentation/git-imap-send.txt b/Documentation/git-imap-send.txt
index 77aacf13..5d1e4c80 100644
--- a/Documentation/git-imap-send.txt
+++ b/Documentation/git-imap-send.txt
@@ -44,7 +44,8 @@ OPTIONS
 
 --no-curl::
Talk to the IMAP server using git's own IMAP routines instead of
-   using libcurl.
+   using libcurl.  Ignored if Git was built with the NO_OPENSSL option
+   set.
 
 
 CONFIGURATION
diff --git a/imap-send.c b/imap-send.c
index d69887da..37ac4aa8 100644
--- a/imap-send.c
+++ b/imap-send.c
@@ -34,8 +34,16 @@ typedef void *SSL;
 #include http.h
 #endif
 
+#if defined(USE_CURL_FOR_IMAP_SEND)  defined(NO_OPENSSL)
+/* only available option */
+#define USE_CURL_DEFAULT 1
+#else
+/* strictly opt in */
+#define USE_CURL_DEFAULT 0
+#endif
+
 static int verbosity;
-static int use_curl; /* strictly opt in */
+static int use_curl = USE_CURL_DEFAULT;
 
 static const char * const imap_send_usage[] = { git imap-send [-v] [-q] 
[--[no-]curl]  mbox, NULL };
 
@@ -1504,9 +1512,14 @@ int main(int argc, char **argv)
 
 #ifndef USE_CURL_FOR_IMAP_SEND
if (use_curl) {
-   warning(--use-curl not supported in this build);
+   warning(--curl not supported in this build);
use_curl = 0;
}
+#elif defined(NO_OPENSSL)
+   if (!use_curl) {
+   warning(--no-curl not supported in this build);
+   use_curl = 1;
+   }
 #endif
 
if (!server.port)
---
--
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

Is the Git Mailing List dropping messages?

[Kyle J. McKay]

About 10 days ago I sent out this message (just reproducing the  
relevant headers here):



From: Kyle J. McKay mack...@gmail.com
Date: February 24, 2015 09:16:05 PST
To: Junio C Hamano gits...@pobox.com
Cc: Git Mailing List git@vger.kernel.org
Subject: Any chance for a Git v2.1.5 release?
Message-Id: c5211e53-8905-41c9-9d28-26d7bb51e...@gmail.com
Content-Type: text/plain; charset=US-ASCII; format=flowed; delsp=yes


And I got back a reply (again just the relevant headers):


From: Junio C Hamano gits...@pobox.com
Date: February 24, 2015 11:52:03 PST
To: Kyle J. McKay mack...@gmail.com
Cc: Git Mailing List git@vger.kernel.org
Subject: Re: Any chance for a Git v2.1.5 release?
Message-Id: xmqqk2z7qe8s@gitster.dls.corp.google.com
Content-Type: text/plain


And I responded and that response and the rest of the thread are  
available on gmane [1], but the first two messages are not.  I waited  
10 days just to make sure there were no bounce emails or undeliverable  
notifications coming back and none did.  I have checked the other list  
archives [2] and cannot find the first two messages there either.


I have therefore concluded that the git@vger mailing list ate them for  
a late breakfast snack on 2015-02-24.


Has anyone else noticed any problems with their messages to the  
git@vger list not showing up on the archives?


-Kyle

[1] http://thread.gmane.org/gmane.comp.version-control.git/264365
[2] https://git.wiki.kernel.org/index.php/ 
GitCommunity#Mailing_List_Archives


P.S. The full text of the two first messages is included below:

 BEGIN FIRST MESSAGE 
From: Kyle J. McKay mack...@gmail.com
Date: February 24, 2015 09:16:05 PST
To: Junio C Hamano gits...@pobox.com
Cc: Git Mailing List git@vger.kernel.org
Subject: Any chance for a Git v2.1.5 release?
Message-Id: c5211e53-8905-41c9-9d28-26d7bb51e...@gmail.com
Content-Type: text/plain; charset=US-ASCII; format=flowed; delsp=yes

As you know, I help out with repo.or.cz.  Recently the admins have  
been discussing upgrading the version of Git we use in order to  
support newer features such as pack bitmaps.  While we do use a  
slightly customized gitweb, we have always stuck to an official Git  
release for everything else.


Repo.or.cz provides fetch (git, http, https, ssh), browsing (gitweb)  
and push (ssh, https).  Additionally, created repositories can be  
mirrors (no push allowed) of other repositories (including svn via git- 
svn).  Email notification of ref changes (along with diffs) can also  
be requested.


We are finding that in order to upgrade Git at this point we would  
need to build a custom Git with cherry picked fixes for various issues  
that have come up or they would likely be triggered by one of the  
services repo.or.cz provides.  In addition, as there are numerous  
reports of an unresolved issue with git-svn starting with v2.2.0  
upgrading to v2.2.0 or later presents a problem since we have several  
mirrors that rely on git-svn.


Which brings us back to the subject of this email, is there any chance  
for a v2.1.5 release?


After reviewing a few release dates:

2011-06-26T12:41:26-07:00 v1.7.6
2012-02-05T23:51:07-08:00 v1.7.6.6

2013-11-27T12:14:52-08:00 v1.8.5
2014-02-13T13:42:01-08:00 v1.8.5.5

2014-02-14T11:36:11-08:00 v1.9.0
2014-05-30T10:15:10-07:00 v1.9.4

2014-05-28T11:04:29-07:00 v2.0.0
2014-07-30T14:20:01-07:00 v2.0.4

2014-08-15T15:09:28-07:00 v2.1.0
2014-10-29T10:48:57-07:00 v2.1.3

2014-11-26T13:18:43-08:00 v2.2.0
2015-01-12T14:06:20-08:00 v2.2.2

2015-02-05T13:24:05-08:00 v2.3.0

(I have omitted the dates of the 5 security releases on 2014-12-17 as  
that seems like an extraordinary event unlikely to be repeated.)


It appears that the average support lifespan of a Git release from  
initial release date through last released maintenance update is  
approximately 2-3 months with the 1.7.6 release being an exception at  
a little over 7 months.


If a v2.1.5 release is out of the question, would it be possible to  
periodically designate certain Git releases as long term support  
releases?  Meaning that they would receive maintenance updates (e.g.  
fixes for invalid memory accesses/crashes, regressions or security  
issues) for an extended period of time, say at least 6 months?



Here's the analysis that led to this request:


Goal 1: add support for symref=HEAD:refs/... capability

Goal 2: add support for pack bitmaps

Nice to have: The CVE-2014-9390 fix, but repo.or.cz does not create  
any working trees so it's not mandatory.



Goal 1:

symref=HEAD:refs/... requires at least Git 1.8.4.3.  However,  
repo.or.cz runs git-daemon with read-only access to the repositories  
and since Git 1.8.4.2 shallow clones require write access.


This was corrected in v2.0.0.  So at least v2.0.0 would be needed for  
symref=HEAD:refs/



Goal 2:

Pack bitmap support was added in v2.0.0, but it's probably not a good  
idea to run without 21134714 (pack-objects: turn off bitmaps when we  
split packs) just

Re: [RFC/PATCH 2/5] upload-pack: support out of band client capability requests

[Kyle J. McKay]

On Feb 28, 2015, at 03:22, Duy Nguyen wrote:

The client should only trigger this behavior when it knows the server
can deal with it. And that is possible because in the last fetch, the
server has told the client that it's capable of receiving this
capabilities argument. Backward compatibility is a concern at client
side, not server side.


I've looked at those links and it's unclear to me how they support an
out-of-band option for ssh, they seem to be targeted at git- 
daemon.  Maybe

there's another reference?


For ssh, I think connect.c is the one that constructs and executes  
ssh command.


This I assume you're referring to this change in connect.c from [1]:

@@ -729,6 +734,8 @@ struct child_process *git_connect(int fd[2], const  
char *url,

conn-use_shell = 1;
}
argv_array_push(argv, cmd.buf);
+   if (service_flags)
+   argv_array_push(argv, service_flags);
conn-argv = argv.argv;
if (start_command(conn))
die(unable to fork);

That's not going to work for ssh servers running a stock git-shell and  
I haven't seen any updates to shell.c to match.  git-shell does not  
allow anything other than one argument to be passed to git-upload-pack/ 
git-receive-pack.


When shell.c calls do_generic_cmd and it calls sq_dequote on its  
argument that contains 'dir' 'service-flags' it's going to return  
NULL and shell.c will die(bad argument).  So I don't see how this  
supports ssh as-is even if you know in advance the server supports the  
new protocol.  I don't see any changes to shell.c in that uploadpack2  
branch nor in this patch series.


-Kyle

[1] 
https://github.com/pclouds/git/commit/20d048e5fc650b20fdc7dd8bbe35cb8510ac9c50
--
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 2/5] upload-pack: support out of band client capability requests

[Kyle J. McKay]

On Feb 27, 2015, at 17:01, Stefan Beller wrote:

From: Nguyễn Thái Ngọc Duy pclo...@gmail.com

The only difference from the original protocol client capabilities are
negotiated before initial refs advertisment.

Client capabilities are sent out of band (upload-pack receives it as
the second command line argument). The server sends one pkt-line back
advertising its capabilities.

Signed-off-by: Stefan Beller sbel...@google.com
---

Notes:
   v1:
   I am still undecided if the client should then accept/resend
   the capabilities to confirm them, which would make the client the
   ultimate decider which capabilities are used.

   My gut feeling is to rather let the server make the final decision
   for the capabilities, as it will use some requested capabilities
   already to not send out all the refs.

Documentation/git-upload-pack.txt | 10 +-
upload-pack.c | 42 ++ 
+

2 files changed, 38 insertions(+), 14 deletions(-)

diff --git a/Documentation/git-upload-pack.txt b/Documentation/git- 
upload-pack.txt

index 0abc806..ad3a89d 100644
--- a/Documentation/git-upload-pack.txt
+++ b/Documentation/git-upload-pack.txt
@@ -9,7 +9,7 @@ git-upload-pack - Send objects packed back to git- 
fetch-pack

SYNOPSIS

[verse]
-'git-upload-pack' [--strict] [--timeout=n] directory
+'git-upload-pack' [--strict] [--timeout=n] directory  
[capabilities]


Isn't the problem with this that passing the extra argument to ssh  
servers will cause them to fail?


Having just looked at the upload-pack.c source it looks to me like  
trying to send git-upload-pack 'dir' 'capabilities' to an ssh git  
server running a current version of the code will just end up  
failing.  I realize the extra argument is optional, so does that mean  
there's no out-of-band support for ssh connections since the extra  
argument would have to be omitted to remain compatible?


On Feb 26, 2015, at 12:13, Junio C Hamano wrote:

The capability-based sidegrade does not solve the problem
when the problem to be solved is that the server side needs to spend
a lot of cycles and the network needs to carry megabytes of data
before capability exchange happens.



On Feb 27, 2015, at 16:46, Stefan Beller wrote:
I'll try to present a 'client asks for options first out of band'  
instead of the

way you describe.


On Feb 27, 2015, at 15:44, Stefan Beller wrote:

For both native git as well as ssh, Duy presented a solution at [1, 2]
a year ago, which essentially presents the desired client capabilites
'out of band' to the server via an argument to the server.  So we'd
only need to examine the http(s) path how to pass in arguments there.



I've looked at those links and it's unclear to me how they support an  
out-of-band option for ssh, they seem to be targeted at git-daemon.   
Maybe there's another reference?


But there does seem to be a way to pass the protocol information out- 
of-band for each of the HTTP, git and ssh connections to stop the  
initial ref advertisement.


As already suggested [1, 2], for git: another Extended attribute can  
be added after the host=...\0 to become host=...\0protocol=extended\0  
or similar:


For HTTP, just add a second parameter in the query string .../info/ 
refs?service=git-upload-packprotocol=extended.  Alternatively an X- 
Git-Protocol: extended or similar header can be added by the client.   
It looks to me like the current http-backend.c already ignores any  
extra parameters/headers.


That leaves ssh.  A bit more problematic, but if the server side adds  
AcceptEnv GIT_PROTOCOL to its sshd_config and then the client does  
setenv(GIT_PROTOCOL,extended) and adds a -o SendEnv=GIT_PROTOCOL  
option to the ssh command line the GIT_PROTOCOL variable will be  
passed along.  This one might need a config option to always disable  
adding the -o ... option to the command line in case connect.c  
guesses wrongly about it being OpenSSH and such is not likely to be  
supported other than with OpenSSH on both ends.  I'm not seeing any  
other way to pass out-of-band information to an ssh server configured  
to run git-shell that is safely ignored by current versions of the code.


Worst case with SSH is the initial ref advertisement is not suppressed  
even though the server does support protocol v2 and a capability-based  
sidegrade is required which is unfortunate.


-Kyle

[1] 
https://github.com/pclouds/git/commit/e26fa77c4d9ace06b9f2c80091af9eb7b63a1c95
[2] 
https://github.com/pclouds/git/commit/20d048e5fc650b20fdc7dd8bbe35cb8510ac9c50
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: git svn import failure : write .git/Git_svn_hash_BmjclS: Bad file descriptor

[Kyle J. McKay]

On Feb 26, 2015, at 01:09, Nico Schl=F6mer wrote:
 All,

 I applied Kyle's latest patch

 diff --git a/perl/Git/SVN/Ra.pm b/perl/Git/SVN/Ra.pm
 index 622535e2..96888228 100644
 --- a/perl/Git/SVN/Ra.pm
 +++ b/perl/Git/SVN/Ra.pm
 @@ -391,6 +391,7 @@ sub longest_common_path {
 sub gs_fetch_loop_common {
my ($self, $base, $head, $gsv, $globs) =3D @_;
return if ($base  $head);
 +   $::_repository-_open_cat_blob_if_needed;
my $gpool =3D SVN::Pool-new_default;
my $ra_url =3D $self-url;
my $reload_ra =3D sub {

 alone and this fixes the bug for me. Thanks a lot, Kyle!

The updated patch with the additional fix is below.

There are two symptoms it addresses:

1) the 'Git_svn_hash_... bad file descriptor' failure

2) the 'out pipe went bad' failure

While (1) could probably also be addressed by Eric's alternate
destroy all tempfiles when reloading RA patch, that would require re-
opening all the temp files every 100 revisions (or whatever the log
window size is changed to).

I noticed that with the default log window size of 100 revisions, each
time the connection was reset at the 100-revision boundary (to reduce
the overall memory usage) it seemed to take approx. 3 seconds to start
up again processing revisions on that gmsh repository.  If you're
cloning 3 revisions, that adds 15 minutes to the total clone time
already.  Admittedly opening new temp files will be a lot faster and
hardly noticeable, but doing that 300 times over the course of 3
revisions will probably add at least a little extra delay and since
blowing away all the temp files does not seem to be necessary, why
incur the extra delay?

Also the destroy all tempfiles when reloading RA patch isn't going
to fix the cat_blob 'out pipe went bad' problem since that has nothing
to do with the temp files so another change will still be required for
that.

-Kyle

-- 8 --
Subject: [PATCH v2] Git::SVN::*: avoid premature FileHandle closure

Since b19138b (git-svn: Make it incrementally faster by minimizing temp
files, v1.6.0), git-svn has been using the Git.pm temp_acquire and
temp_release mechanism to avoid unnecessary temp file churn and provide
a speed boost.

However, that change introduced a call to temp_acquire inside the
Git::SVN::Fetcher::close_file function for an 'svn_hash' temp file.
Because an SVN::Pool is active at the time this function is called, if
the Git::temp_acquire function ends up actually creating a new
FileHandle for the temp file (which it will the first time it's called
with the name 'svn_hash') that FileHandle will end up in the SVN::Pool
and should that pool have SVN::Pool::clear called on it that FileHandle
will be closed out from under Git::temp_acquire.

Since the only call site to Git::temp_acquire with the name 'svn_hash'
is inside the close_file function, if an 'svn_hash' temp file is ever
created its FileHandle is guaranteed to be created in the active
SVN::Pool.

This has not been a problem in the past because the SVN::Pool was not
being cleared.  However, since dfa72fdb (git-svn: reload RA every
log-window-size, v2.2.0) the pool has been getting cleared periodically
at which point the FileHandle for the 'svn_hash' temp file gets closed.
Any subsequent calls to Git::temp_acquire for 'svn_hash', however,
succeed without creating/opening a new temporary file since it still has
the now invalid FileHandle in its cache.  Callers that then attempt to
use that FileHandle fail with an error.

We avoid this problem by making sure the 'svn_hash' temp file is created
in the same place the 'svn_delta_...' and 'git_blob_...' temp files are
(and then temp_release'd) so that it can be safely used inside the
close_file function without having its FileHandle end up in an SVN::Pool
that gets cleared.

Additionally the Git.pm cat_blob function creates a bidirectional pipe
FileHandle using the IPC::Open2::open2 function.  If that handle is
created too late, it also gets caught up in the SVN::Pool and incorrectly
closed by the SVN::Pool::clear call.  But this only seems to happen with
more recent versions of Perl and svn.

To avoid this problem we add an explicit call to _open_cat_blob_if_needed
before the first call to SVN::Pool-new_default to make sure the open2
handle does not end up in the SVN::Pool.

Signed-off-by: Kyle J. McKay mack...@gmail.com
---
 perl/Git/SVN/Fetcher.pm | 8 
 perl/Git/SVN/Ra.pm  | 3 +++
 2 files changed, 11 insertions(+)

diff --git a/perl/Git/SVN/Fetcher.pm b/perl/Git/SVN/Fetcher.pm
index 10edb277..613055a3 100644
--- a/perl/Git/SVN/Fetcher.pm
+++ b/perl/Git/SVN/Fetcher.pm
@@ -322,6 +322,14 @@ sub apply_textdelta {
# (but $base does not,) so dup() it for reading in close_file
open my $dup, '', $fh or croak $!;
my $base = $::_repository-temp_acquire(git_blob_${$}_$suffix);
+   # close_file may call temp_acquire on 'svn_hash', but because of the
+   # call chain, if the temp_acquire call from close_file ends up being

Re: Any chance for a Git v2.1.5 release?

[Kyle J. McKay]

On Feb 26, 2015, at 12:54, Junio C Hamano wrote:


Kyle J. McKay mack...@gmail.com writes:


I would like to better understand how the various heads are
maintained.  I've read MaintNotes and I've got the concepts, but I'm
still a little fuzzy on some details.  It looks to me like all topics
still only in pu after master has been updated are then rebased onto
the new master and then pu is rebuilt.


Topics in 'pu' not yet in 'next' _can_ be rebased, but unless there
is a good reason to do so, I wouldn't spend extra cycles necessary
to do such thing.  I even try to keep the same base when replacing
the contents of a branch with a rerolled series.  For example, today
I have this:

   $ git config alias.lgf
   log --oneline --boundary --first-parent
   $ git lgf master..nd/slim-index-pack-memory-usage
   3538291 index-pack: kill union delta_base to save memory
   7b4ff41 FIXUP
   45b6b71 index-pack: reduce memory footprint a bit
   - 9874fca Git 2.3

and Duy has a newer iteration of it starting at $gmane/264429.  What
I would do, after saving these patches in a mbox +nd-index-pack,
would be to

   $ git checkout 9874fca
   $ git am -s3c ./+nd-index-pack
   $ git show-branch HEAD nd/slim-index-pack-memory-usage
   ... compare corresponding changes between the old and the new
   ... until I am satisified; otherwise I may tweak the new one
   $ git rebase -i 9874fca
   ... and then finally
   $ git branch -f nd/slim-index-pack-memory-usage HEAD

to update the topic.  Of course, it would be necessary to rebuild
'pu' by merging all the topics that are in it on top of 'master'.


Thanks.  That's helpful.


After finishing 2.3.0 release, at some point while 'master' is still
at 2.3.0, something like this would happen:

   $ git branch -m maint maint-2.2
   $ git branch maint master


So the reason I don't notice force-updates to maint when this happens  
is because of the Sync with maint commits that make sure the new  
maint contains the old one.



Also, how do you handle a down merge to maint when you have this:

* (master)
* merge topic foo
|\
| * topic foo
|/
* c
* b
* a
* (tag: vX.X.X, maint)
*


I don't, and this is done by making sure I do not get into such a
situation in the first place.

A general rule of thumb when applying a set of patches that are
fixes eventually worth having in older maintenance tracks is to find
the oldest maintenance branch and apply them there.


If I were to keep a maint-lts branch somewhere I would need to cherry- 
pick topics with desired fixes that fall into that situation.  That's  
what I expected but when you mentioned down merging the fixes I wanted  
to make sure I wasn't overlooking something.


I'll see about setting up a maint-lts in a local git repository clone  
and tracking LTS fixes.  If I'm able to keep that going without it  
becoming a black-hole of temporal need that sucks the life right out  
of me  ;)  then perhaps we can have a discussion at a future date  
about what would be needed for you to consider pulling from it and  
issuing LTS releases off it.  :)


-Kyle
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: git svn import failure : write .git/Git_svn_hash_BmjclS: Bad file descriptor

[Kyle J. McKay]

On Feb 25, 2015, at 07:07, Nico Schlömer wrote:


Thanks Kyle for the patch! I applied it and ran
```
git svn clone https://geuz.org/svn/gmsh/trunk
```
Unfortunately, I'm still getting
```
[...]
r100 = e2a9b5baa2cebb18591ecb04ff350410d52f36de (refs/remotes/git-svn)
error closing pipe: Bad file descriptor at /usr/share/perl5/Git/SVN/ 
Fetcher.pm line 335.
error closing pipe: Bad file descriptor at /usr/share/perl5/Git/SVN/ 
Fetcher.pm line 335.

out pipe went bad at /usr/share/perl5/Git.pm line 955.
```


Are you certain you're running with the patch?  I ask because earlier  
you sent this:


On Jan 31, 2015, at 04:51, Nico Schlömer wrote:


I tried the patch and I still get
```
[...]
r100 = e2a9b5baa2cebb18591ecb04ff350410d52f36de (refs/remotes/git-svn)
Unexpected result returned from git cat-file at
/home/nschloe/share/perl/5.18.2/Git/SVN/Fetcher.pm line 335.
Failed to read object 619f9d1d857fb287d06a70c9dac6b8b534d0de6a at
/home/nschloe/share/perl/5.18.2/Git/SVN/Fetcher.pm line 336, GEN16
line 757.

error closing pipe: Bad file descriptor at
/home/nschloe/libexec/git-core/git-svn line 0.
error closing pipe: Bad file descriptor at
/home/nschloe/libexec/git-core/git-svn line 0.
```
when
```
git svn clone https://geuz.org/svn/gmsh/trunk
```


And as the patch below applies to Fetcher.pm at line 322 and inserts 8  
lines, I do not see how you could be getting the same error message at  
line 335 if the patch was present.  Even if you manually applied it by  
only inserting the two lines of code, the line numbers would be at  
least 337, not 335 as reported above.  I also notice the path to  
Fetcher.pm is different from your earlier report.


That said, the fact that it happens right after r100 (which is when  
SVN::Pool::clear is getting called) suggests there's another file  
handle getting caught up in the SVN::Pool somehow.  (When I try to  
clone gmsh, I got to r4885 before a server disconnection.)


It could be as simple as the open2 call FileHandle result in later  
perl versions ends up in the SVN::Pool whereas in earlier Perl and/or  
svn versions it does not.


What happens if you add this change on top of the Fetcher.pm change:

diff --git a/perl/Git/SVN/Ra.pm b/perl/Git/SVN/Ra.pm
index 622535e2..96888228 100644
--- a/perl/Git/SVN/Ra.pm
+++ b/perl/Git/SVN/Ra.pm
@@ -391,6 +391,7 @@ sub longest_common_path {
 sub gs_fetch_loop_common {
my ($self, $base, $head, $gsv, $globs) = @_;
return if ($base  $head);
+   $::_repository-_open_cat_blob_if_needed;
my $gpool = SVN::Pool-new_default;
my $ra_url = $self-url;
my $reload_ra = sub {

-Kyle


Cheers,
Nico

On Wed, Feb 25, 2015 at 11:20 AM Kyle J. McKay mack...@gmail.com  
wrote:

I believe I have been able to track down this problem and implement a
fix.  Please report back if this patch fixes the problem for you.

-Kyle

-- 8 --
Subject: [PATCH] Git::SVN::Fetcher: avoid premature 'svn_hash' temp  
file closure


Since b19138b (git-svn: Make it incrementally faster by minimizing  
temp

files, v1.6.0), git-svn has been using the Git.pm temp_acquire and
temp_release mechanism to avoid unnecessary temp file churn and  
provide

a speed boost.

However, that change introduced a call to temp_acquire inside the
Git::SVN::Fetcher::close_file function for an 'svn_hash' temp file.
Because an SVN::Pool is active at the time this function is called, if
the Git::temp_acquire function ends up actually creating a new
FileHandle for the temp file (which it will the first time it's called
with the name 'svn_hash') that FileHandle will end up in the SVN::Pool
and should that pool have SVN::Pool::clear called on it that  
FileHandle

will be closed out from under Git::temp_acquire.

Since the only call site to Git::temp_acquire with the name 'svn_hash'
is inside the close_file function, if an 'svn_hash' temp file is ever
created its FileHandle is guaranteed to be created in the active
SVN::Pool.

This has not been a problem in the past because the SVN::Pool was not
being cleared.  However, since dfa72fdb (git-svn: reload RA every
log-window-size, v2.2.0) the pool has been getting cleared  
periodically
at which point the FileHandle for the 'svn_hash' temp file gets  
closed.

Any subsequent calls to Git::temp_acquire for 'svn_hash', however,
succeed without creating/opening a new temporary file since it still  
has

the now invalid FileHandle in its cache.  Callers that then attempt to
use that FileHandle fail with an error.

We avoid this problem by making sure the 'svn_hash' temp file is  
created
in the same place the 'svn_delta_...' and 'git_blob_...' temp files  
are

(and then temp_release'd) so that it can be safely used inside the
close_file function without having its FileHandle end up in an  
SVN::Pool

that gets cleared.

Signed-off-by: Kyle J. McKay mack...@gmail.com
---
 perl/Git/SVN/Fetcher.pm | 8 
 1 file changed, 8 insertions(+)

diff --git a/perl/Git/SVN/Fetcher.pm b/perl/Git/SVN/Fetcher.pm
index

Re: Any chance for a Git v2.1.5 release?

[Kyle J. McKay]

On Feb 24, 2015, at 21:13, Junio C Hamano wrote:


Kyle J. McKay mack...@gmail.com writes:


I can designate ;-), but I do not think I'd be the right person to
maintain or long-term-support it.  Are you volunteering to oversee
the LTS team?


I could not promise a team of more than one member.  And that would
not be full-time 24/7 either.


Heh. Making noises to find like-minded people would be the first
step to build a viable team, and hopefully you are already doing a
good job here ;-)


Doesn't seem to be catching much interest though.  Maybe they're all  
just watching silently peering through the blinds waiting to see how  
it turns out.  ;)



It would involve:

  - Monitor git log --first-parent maint-lts..master and find
the tip of topic branches that need to be down-merged;

  - Down-merge such topics to maint-lts; this might involve
cherry-picking instead of merge, as the bugfix topics may
originally be done on the codebase newer than maint-lts;


I've been cherry-picking fixes for a while now onto older  
releases.  I

don't suppose down-merging would be that much more difficult with a
fallback to cherry-picking.


  - Use the tip of the maint-lts branch in everyday work.

The last item is the most important of the above, because I do not
have time for that.  I can help with the first two to some degree,
though.


That's pretty much all I use at this point -- a slightly older  
release

with cherry-picked fixes.  While it did cause me to find the problem
with the first version of the loose alternates fix, having only one
person use such a release doesn't provide that much coverage.


That is why I used the word team.


It occurs to me that if the maint-lts updates were limited to crash
fixes, regressions and security issues then often the pre-built man
pages and docs from the release it's based on could be used as-is  
with

the exception of the new release notes which might save some time.


Cutting release tarballs including the pre-formatting docs might
consume a lot of machine time, but it does not cost me time at all.
I have Makefile for that ;-)

Judging which fixes that have proven themselves to be safe and sound
(by being in 'next', 'master' and hopefully 'maint' for some time)
are worthy enough of down-merging to the LTS track is something I'd
want to farm out to the LTS team.  I am already doing the safe and
sound part by deciding which topics to merge to 'maint' among the
topics that have gone through 'pu' to 'next' to 'master' branches,
but not all that are worthy enough to be merged to 'maint' may be
important enough to bother downmerging and updating LTS track with,
and picking which ones matter to the LTS users is what the folks who
are interested in the LTS can help.


If I were to keep a maint-lts up-to-date somewhere (with suitable down- 
merges matching the manner in which you maintain your tree) that you  
could pull from and potentially make releases from.  I would not want  
to pick up anything that hadn't already made it into master or maint  
and I don't think any actual release based off maint-lts should have  
any fix that has not already appeared in another non-maint-lts  
release.  So any given maint-lts release date would be expected to lag  
behind the corresponding master/maint release date that contained the  
same fixes (except possibly for a coordinated security fix release).


That said, there's no reason I couldn't also keep a pu-lts up-to-date  
somewhere external to your tree that interested parties could grab  
that would include fixes making their way into maint/master that I  
believe would be candidates for inclusion in maint-lts once they  
graduated.


I would like to better understand how the various heads are  
maintained.  I've read MaintNotes and I've got the concepts, but I'm  
still a little fuzzy on some details.  It looks to me like all topics  
still only in pu after master has been updated are then rebased onto  
the new master and then pu is rebuilt.  MaintNotes doesn't get into  
the rebasing details.  None of the graphic log viewers I've tried are  
much help -- too many lines running around although the graphiclog on  
repo.or.cz helps a bit as it shows --first-parent links as a bolder  
line, but still after the first page that's not much help either.


I vaguely recall you may have explained some of this in more detail in  
the context of explaining how you used the scripts in todo to put  
everything together when someone asked a question about it on the  
list.  But I can't seem to find that message anymore.  :(


I think I mostly understand how master, next and pu are maintained.   
But MaintNotes says Whenever a feature release is made, 'maint'  
branch is forked off from 'master' at that point.  What happens to  
the previous maint at that time?  Is it just renamed to maint-X.X?


Also, how do you handle a down merge to maint when you have this:

* (master)
* merge topic foo
|\
| * topic foo
|/
* c
* b
* a
* (tag

Re: git svn import failure : write .git/Git_svn_hash_BmjclS: Bad file descriptor

[Kyle J. McKay]

I believe I have been able to track down this problem and implement a
fix.  Please report back if this patch fixes the problem for you.

-Kyle

-- 8 --
Subject: [PATCH] Git::SVN::Fetcher: avoid premature 'svn_hash' temp file closure

Since b19138b (git-svn: Make it incrementally faster by minimizing temp
files, v1.6.0), git-svn has been using the Git.pm temp_acquire and
temp_release mechanism to avoid unnecessary temp file churn and provide
a speed boost.

However, that change introduced a call to temp_acquire inside the
Git::SVN::Fetcher::close_file function for an 'svn_hash' temp file.
Because an SVN::Pool is active at the time this function is called, if
the Git::temp_acquire function ends up actually creating a new
FileHandle for the temp file (which it will the first time it's called
with the name 'svn_hash') that FileHandle will end up in the SVN::Pool
and should that pool have SVN::Pool::clear called on it that FileHandle
will be closed out from under Git::temp_acquire.

Since the only call site to Git::temp_acquire with the name 'svn_hash'
is inside the close_file function, if an 'svn_hash' temp file is ever
created its FileHandle is guaranteed to be created in the active
SVN::Pool.

This has not been a problem in the past because the SVN::Pool was not
being cleared.  However, since dfa72fdb (git-svn: reload RA every
log-window-size, v2.2.0) the pool has been getting cleared periodically
at which point the FileHandle for the 'svn_hash' temp file gets closed.
Any subsequent calls to Git::temp_acquire for 'svn_hash', however,
succeed without creating/opening a new temporary file since it still has
the now invalid FileHandle in its cache.  Callers that then attempt to
use that FileHandle fail with an error.

We avoid this problem by making sure the 'svn_hash' temp file is created
in the same place the 'svn_delta_...' and 'git_blob_...' temp files are
(and then temp_release'd) so that it can be safely used inside the
close_file function without having its FileHandle end up in an SVN::Pool
that gets cleared.

Signed-off-by: Kyle J. McKay mack...@gmail.com
---
 perl/Git/SVN/Fetcher.pm | 8 
 1 file changed, 8 insertions(+)

diff --git a/perl/Git/SVN/Fetcher.pm b/perl/Git/SVN/Fetcher.pm
index 10edb277..613055a3 100644
--- a/perl/Git/SVN/Fetcher.pm
+++ b/perl/Git/SVN/Fetcher.pm
@@ -322,6 +322,14 @@ sub apply_textdelta {
# (but $base does not,) so dup() it for reading in close_file
open my $dup, '', $fh or croak $!;
my $base = $::_repository-temp_acquire(git_blob_${$}_$suffix);
+   # close_file may call temp_acquire on 'svn_hash', but because of the
+   # call chain, if the temp_acquire call from close_file ends up being the
+   # call that first creates the 'svn_hash' temp file, then the FileHandle
+   # that's created as a result will end up in an SVN::Pool that we clear
+   # in SVN::Ra::gs_fetch_loop_common.  Avoid that by making sure the
+   # 'svn_hash' FileHandle is already created before close_file is called.
+   my $tmp_fh = $::_repository-temp_acquire('svn_hash');
+   $::_repository-temp_release($tmp_fh, 1);
 
if ($fb-{blob}) {
my ($base_is_link, $size);
--
--
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: Any chance for a Git v2.1.5 release?

[Kyle J. McKay]

On Feb 24, 2015, at 11:52, Junio C Hamano wrote:


Kyle J. McKay mack...@gmail.com writes:

Which brings us back to the subject of this email, is there any  
chance

for a v2.1.5 release?
...
It appears that the average support lifespan of a Git release from
initial release date through last released maintenance update is
approximately 2-3 months with the 1.7.6 release being an exception at
a little over 7 months.


That matches my expectation.

A typical cycle lasts for 8-12 weeks, and during that time, topics
that are bugfixes that have graduated to the 'master' branch are
merged to the 'maint' branch with some lag and then the tip of
'maint' gets tagged as a maintenance release from time to time.
Some important but trivial fixes are further merged to older
maitenance tracks like 'maint-2.2', 'maint-2.1', etc.

But these topics downmerged to older maint-* branches have to be
very trivial for an obvious reason: there are only 24 hours a day
and 7 days in a week, and bugs that affect real world use cases are
found by using the software in real world use cases.  Usually I use
something a bit ahead of 'next' exactly for this reason---we would
want to catch bugs before topics are merged to 'master'.  Although I
sometimes have let's use 'maint' for my work day once or twice
every month, I cannot afford to do that for anything older than the
tip of 'maint' myself.


Obviously there would have to actually be some interest in having an  
older long-term-support release and some folks willing to exercise  
such a thing.  Unless we can figure out a way to clone you. ;) ;)



The consequence of the above is this.  v2.1.1 may be more stable
than v2.1.0 and v2.1.2 may be more stable than v2.1.1, but later
tagged versions on older maintenance tracks are made by merging
topics only after ah, these are obvious enough eyeballing without
real use (at least by me), once newer feature release is made and
there is a newer maintenance track.  I would not be surprised if
v2.1.5, if it is made, has hidden interactions between the changes
since v2.1.4 and the older codebase to cause unforeseen bugs.

When I say the tip of 'master' is meant to be more stable than any
tagged versions, I do mean it.


Some fixes would likely not be back portable (e.g. to fix X you first  
need change Y which needs change Z which needs ...), not without  
ending up pulling in things that exceed the scope of a maintenance  
update.



Having said all that, if I were to tag maint-2.1 branch as 2.1.5
today, we would have

   6aaf956 is_hfs_dotgit: loosen over-eager match of \u{..47}

that does not exist in 2.1.4.  Is that what you want?


I suppose in that it fixes false positives it is a regression fix,  
but if that's all that showed up in v2.1.5, no, that wouldn't make it  
worthwhile.



If a v2.1.5 release is out of the question, would it be possible to
periodically designate certain Git releases as long term support
releases?


I can designate ;-), but I do not think I'd be the right person to
maintain or long-term-support it.  Are you volunteering to oversee
the LTS team?


I could not promise a team of more than one member.  And that would  
not be full-time 24/7 either.



It would involve:

   - Monitor git log --first-parent maint-lts..master and find
 the tip of topic branches that need to be down-merged;

   - Down-merge such topics to maint-lts; this might involve
 cherry-picking instead of merge, as the bugfix topics may
 originally be done on the codebase newer than maint-lts;


I've been cherry-picking fixes for a while now onto older releases.  I  
don't suppose down-merging would be that much more difficult with a  
fallback to cherry-picking.



   - Use the tip of the maint-lts branch in everyday work.

The last item is the most important of the above, because I do not
have time for that.  I can help with the first two to some degree,
though.


That's pretty much all I use at this point -- a slightly older release  
with cherry-picked fixes.  While it did cause me to find the problem  
with the first version of the loose alternates fix, having only one  
person use such a release doesn't provide that much coverage.



If the lts releases need to be tagged and published by me, then lts
team can have me pull from the tip of maint-lts they are confident
with and have me sign it and push it out.


It occurs to me that if the maint-lts updates were limited to crash  
fixes, regressions and security issues then often the pre-built man  
pages and docs from the release it's based on could be used as-is with  
the exception of the new release notes which might save some time.



If the primary concern you have with the currently maintained
releases is git-svn, perhaps a better way forward for you,
especially if you are willing to maintain your own release plus
patches,


That was the discussion the admins had that we prefer to avoid rolling  
our own, but if there's no other option then we will do that.  Some  
other Git

Re: git mac 10.7.x

[Kyle J. McKay]

On Feb 20, 2015, at 12:01, sojourner wrote:
What's the difference between this installer and the other one? Why  
is this installer going to work?


See the website for a description.  It was built to work with 10.4.8  
or later.  It was built using Apple's older GCC specifically to be  
compatible so does not contain any illegal instructions that only  
work on 10.8.x or later.  Besides, I've actually run it on the older  
systems without problem so I'm pretty confident you won't have an  
issue with it.


-Kyle


On 20 Feb 2015, at 13:32, Kyle J. McKay wrote:


On Feb 20, 2015, at 02:38, sojourner wrote:
Installed Git via installer. Updated path in .bash_profile. Get  
error Illegal instruction: 4 when trying to run Git.


Built Git from source. Searches for the compiled source  
unsuccessful. Which is nice: there's nothing to uninstall.


Searching online has a lot of suggestions and ideas. Anybody have  
anything that actually works?--


You can get a working installer from http://mackyle.github.io/git-osx-installer/ 
 that should work for you on 10.7.x just fine.


-Kyle




--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: git mac 10.7.x

[Kyle J. McKay]

On Feb 20, 2015, at 02:38, sojourner wrote:
Installed Git via installer. Updated path in .bash_profile. Get  
error Illegal instruction: 4 when trying to run Git.


Built Git from source. Searches for the compiled source  
unsuccessful. Which is nice: there's nothing to uninstall.


Searching online has a lot of suggestions and ideas. Anybody have  
anything that actually works?--


You can get a working installer from http://mackyle.github.io/git-osx-installer/ 
 that should work for you on 10.7.x just fine.


-Kyle
--
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: [RFD/PATCH] stash: introduce checkpoint mode

[Kyle J. McKay]

On Feb 19, 2015, at 04:34, Michael J Gruber wrote:


git stash save performs the steps create-store-reset. Often,
users try to use stash save as a way to to save their current state
(index, worktree) before an operation like checkout/reset --patch  
they
don't feel confident about, and are forced to do git stash save   
git

stash apply.

Provide an extra mode that does create-store only without the reset,
so that one can ceckpoint the sate and keep working on it.


s/sate/state/


Suggested-by: Kyle J. McKay mack...@gmail.com
Signed-off-by: Michael J Gruber g...@drmicha.warpmail.net
---

Notes:
   I'm not sure about how to best expose this mode:

   git stash checkpoint
   git stash save --checkpoint

   Maybe it is best to document the former and rename --checkpoint
   to --no-reset?


Once the user figures out that save is really save-and-reset I  
think --no-reset makes more sense.


It certainly seems more discoverable via an explicit checkpoint  
command though, but that's really just an alias so maybe it's better  
left up to the user to make one.


There would need to be some updated docs (git-stash.txt) to go with  
the change...



   Also, a safe return to a checkpoint probably requires

   git reset --hard  git stash pop

   although git stash pop will do in many cases. Should we provide  
a shortcut

   restore which does the reset-and-pop?


What about a shortcut to reset-and-apply as well?

I have often been frustrated when git stash apply refuses to work  
because I have changes that would be stepped on and there's no --force  
option like git checkout has.  I end up doing a reset just so I can  
run stash apply.


What about if git stash apply/pop grokked a --force option?  That  
would seem to eliminate the need for a reset-and-pop/reset-and- 
apply shortcut while also being useful to non-checkpoint stashes as  
well.



git-stash.sh | 13 +
1 file changed, 13 insertions(+)

diff --git a/git-stash.sh b/git-stash.sh
index d4cf818..42f140c 100755
--- a/git-stash.sh
+++ b/git-stash.sh
@@ -193,12 +193,16 @@ store_stash () {
}

save_stash () {
+   checkpoint=
keep_index=
patch_mode=
untracked=
while test $# != 0
do
case $1 in
+   -c|--checkpoint)
+   checkpoint=t
+   ;;
-k|--keep-index)
keep_index=t
;;
@@ -267,6 +271,11 @@ save_stash () {
die $(gettext Cannot save the current status)
say Saved working directory and index state $stash_msg

+   if test -n $checkpoint
+   then
+   exit 0
+   fi
+
if test -z $patch_mode
then
git reset --hard ${GIT_QUIET:+-q}
@@ -576,6 +585,10 @@ save)
shift
save_stash $@
;;
+checkpoint)
+   shift
+   save_stash --checkpoint $@
+   ;;
apply)
shift
apply_stash $@
--


Otherwise this looks good.  A very small change to add the  
functionality.


-Kyle
--
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: [RFD/PATCH] stash: introduce checkpoint mode

[Kyle J. McKay]

On Feb 19, 2015, at 09:49, Junio C Hamano wrote:

Kyle J. McKay mack...@gmail.com writes:


What about a shortcut to reset-and-apply as well?

I have often been frustrated when git stash apply refuses to work
because I have changes that would be stepped on and there's no -- 
force

option like git checkout has.  I end up doing a reset just so I can
run stash apply.


Doesn't that cut both ways, though?

A single step short-cut, done in any way other than a more explicit
way such as git reset --hard  git stash apply (e.g. git stash
reset-and-apply or git stash apply --force) that makes it crystal
clear that the user _is_ discarding, has a risk of encouraging users
to form a dangerous habit of invoking the short-cut without thinking
and leading to oops, I didn't mean that!.


Does that reasoning not also apply to the plethora of commands that  
take --force already?


I didn't check them all, but tag, checkout, push and branch  
immediately come to mind.  Why is it okay for all those other commands  
to have a --force mode, but not git stash?

--
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: Lift --stdout restriction for using reachability bitmap in pack-objects?

[Kyle J. McKay]

On Feb 17, 2015, at 02:42, Jeff King wrote:


On Tue, Feb 17, 2015 at 05:36:30PM +0700, Duy Nguyen wrote:


On Tue, Feb 17, 2015 at 5:13 PM, Jeff King p...@peff.net wrote:

If the only reason is for gdb, then perhaps:

 set args pack-objects --stdout /dev/null /dev/null

in gdb would help?


Right. I used gdb --args command /dev/null instead. Stupid
question. Sorry for the noise.


I've made the same mistake myself many times. I really wish gdb would
interact over /dev/tty by default. The perl debugger does this, and I
find it quite handy. But I've never managed to make gdb do it. Maybe
there is an option I've missed[1].


You may want to try cgdb.  It is a curses front-end to gdb.  I almost  
never run bare gdb anymore.  It has a source file pane (no more line- 
by-line), a gdb pane and, although a bit clumsy, a TTY pane (not shown  
in the screen shot) that you can interact with the debuggee with.   
It's been around for a while, so debian-derived distros can usually  
just `apt-get install cgdb`.  There's a screen shot available [1].


-Kyle

[1] http://cgdb.github.io/
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/2] Getopt::Long workaround in send-email

[Kyle J. McKay]

On Feb 13, 2015, at 12:19, Junio C Hamano wrote:

The first one is a replay of Kyle's workaround for older versions of
Getopt::Long that did not take --no-option to negate a boolean
option --option.  The second one reverts the workarounds made to
the test script over time, and should break if the first one does
not work well for older Getopt::Long (I have no reason to suspect it
would break, though).

I am inclined to squash these into one commit before starting to
merge them down to 'next' and then to 'master', after getting
Tested-by: from those with older Getopt::Long (prior to 2.32).


I have no objection to them being squashed together.

-Kyle
--
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] sha1_file: fix iterating loose alternate objects

[Kyle J. McKay]

On Feb 8, 2015, at 17:15, Jeff King wrote:
[...]

Signed-off-by: Jonathon Mah m...@jonathonmah.com
Helped-by: Kyle J. McKay mack...@gmail.com
Signed-off-by: Jeff King p...@peff.net
---
I think the S-O-B should still stand, as the code is now a mix of our
work, and the tests are still Jonathon's. But let me know if you do  
not

want your name attached to this. ;)


That's fine.

This fix looks much better. :)

Unfortunately I can no longer reproduce the original bug as the  
repository that caused it is no longer in a state that triggers the  
problem (and my backups of it are either slightly too old or slightly  
too new).


-Kyle

--
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] sha1_file.c: make sure open_sha1_file does not open a directory

[Kyle J. McKay]

Since sha1_file: fix iterating loose alternate objects, it's possible
for the base member of the alt_odb_list structure to be NUL terminated
rather than ending with a '/' when open_sha1_file is called.

Unfortunately this causes a directory to be passed to git_open_noatime
instead of a file which it happily opens and returns whereupon this
open directory file handle is then passed to mmap.

mmap does not like this and fails with an invalid argument error
at which point the pack-objects process dies prematurely.

To avoid this we preserve the last character of the base member,
set it to '/', make the git_open_noatime call and then restore
it to its previous value which allows pack-objects to function properly.

Signed-off-by: Kyle J. McKay mack...@gmail.com
---

*

While this patch can be applied without sha1_file: fix iterating
loose alternate objects you cannot even get to the failure this fixes
without first having that patch applied.

All this stuffing of a single char back and forth into a [-1] index
seems awfully kludgy to me.

However, without this fix and the previous sha1_file fix I would need
to jettison the latest stuff and revert back to 2.1.4.

I suggest that this (or another fix for the problem) go into maint
together with the sha1_file: fix iterating loose alternate objects
fix.

*

 sha1_file.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/sha1_file.c b/sha1_file.c
index f575b3cf..235f6ad8 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -1491,8 +1491,11 @@ static int open_sha1_file(const unsigned char *sha1)
 
prepare_alt_odb();
for (alt = alt_odb_list; alt; alt = alt-next) {
+   char c = alt-name[-1];
+   alt-name[-1] = '/';
fill_sha1_path(alt-name, sha1);
fd = git_open_noatime(alt-base);
+   alt-name[-1] = c;
if (fd = 0)
return fd;
if (most_interesting_errno == ENOENT)
--
--
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-compat-util: do not step on MAC_OS_X_VERSION_MIN_REQUIRED

[Kyle J. McKay]

On Feb 6, 2015, at 02:00, Eric Sunshine wrote:
On Fri, Feb 6, 2015 at 4:35 AM, Kyle J. McKay mack...@gmail.com  
wrote:

#ifndef NO_OPENSSL
+#ifdef __APPLE__
#define __AVAILABILITY_MACROS_USES_AVAILABILITY 0
-#define MAC_OS_X_VERSION_MIN_REQUIRED MAC_OS_X_VERSION_10_6
+#include AvailabilityMacros.h
+#undef DEPRECATED_ATTRIBUTE
+#define DEPRECATED_ATTRIBUTE
+#undef __AVAILABILITY_MACROS_USES_AVAILABILITY
+#endif
#include openssl/ssl.h
#include openssl/err.h
-#undef MAC_OS_X_VERSION_MIN_REQUIRED
-#undef __AVAILABILITY_MACROS_USES_AVAILABILITY


DEPRECATED_ATTRIBUTE is a fairly generic name. Do we want to be extra
careful and #undef it here to avoid potential unintended interactions
with other #includes (Apple or not)?


The new patch only mucks with it when we have #ifdef __APPLE__ and  
pretty much any apple code is going to end up including the  
availability stuff sooner or later which manipulates  
DEPRECATED_ATTRIBUTE.  Essentially that makes DEPRECATED_ATTRIBUTE a  
reserved macro name on __APPLE__.




   #ifdef __APPLE__
   #undef DEPRECATED_ATTRIBUTE
   #endif


If we do that, won't the compiler then helpfully supply a 0 when the  
macro is used?  Or is that only when an undefined macro is used inside  
an #if or #elif ?


That would break all later declarations that use it.

On the other hand, we've already mucked with it, so #undef may be  
superfluous.


Actually I just tested it.  If we #undef it we could end up producing  
these:


  error: syntax error before ‘DEPRECATED_ATTRIBUTE’

So I think it needs to stay #define'd to nothing to be safe in case  
anything later on ends up including stuff that uses it.  The worst  
that happens is you don't see some deprecation warnings that you  
otherwise would.  It won't make any functions available that shouldn't  
be or make unavailable functions that should be.


-Kyle--
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-compat-util: do not step on MAC_OS_X_VERSION_MIN_REQUIRED

[Kyle J. McKay]

On Feb 6, 2015, at 12:05, Junio C Hamano wrote:


Kyle J. McKay mack...@gmail.com writes:


Actually I just tested it.  If we #undef it we could end up producing
these:

  error: syntax error before DEPRECATED_ATTRIBUTE

So I think it needs to stay #define'd to nothing to be safe in case
anything later on ends up including stuff that uses it.


Doesn't the fact that your test failed indicates that it is not jsut
to be safe in case but is required for correctness?

The first hit for MAC_OS_X_VERSION_MIN_REQUIRED was this:

 
https://opensource.apple.com/source/CarbonHeaders/CarbonHeaders-18.1/AvailabilityMacros.h

which marks quite a many macros to that value.  I do not know what
changes they make to openssl/*.h (which is included just after the
above header is included, but I would imagine that is where the
AVAILABLE_MAC_OS_X_VERSION_XXX_AND_LATER_BUT_DEPRECATED_IN_MAC_OS_VERSION_YYY
macros are checked and annoying warnings that are being squelched by
the previous change are given?


Yes.

Although Eric didn't specify exactly where when he suggested adding  
this:


On Feb 6, 2015, at 02:00, Eric Sunshine wrote:

   #ifdef __APPLE__
   #undef DEPRECATED_ATTRIBUTE
   #endif



I took the suggestion to be after the openssl/*.h headers are included  
which would avoid the error of having DEPRECATED_ATTRIBUTE be #undef'd  
for them.  But, even math.h can end up including AvailabilityMacros.h,  
so I think #undef'ing DEPRECATED_ATTRIBUTE after the openssl/*.h  
headers are included would be unsafe in general.  While we might  
happen to get away with that today, if say compat/apple-common- 
crypto.h changes in the future (or for that matter any sequence of  
includes in other files or any headers in the Apple SDK) we could  
start seeing the error.


TLDR; yeah, DEPRECATED_ATTRIBUTE needs to remain defined to nothing.

-Kyle
--
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] git-compat-util: do not step on MAC_OS_X_VERSION_MIN_REQUIRED

[Kyle J. McKay]

MAC_OS_X_VERSION_MIN_REQUIRED may be defined by the builder to a
specific version in order to produce compatible binaries for a
particular system.  Blindly defining it to MAC_OS_X_VERSION_10_6
is bad.

Additionally MAC_OS_X_VERSION_10_6 will not be defined on older
systems and should AvailabilityMacros.h be included on such as
system an error will result.  However, using the explicit value
of 1060 (which is what MAC_OS_X_VERSION_10_6 is defined to) does
not solve the problem.

The changes that introduced stepping on MAC_OS_X_VERSION_MIN were
made in b195aa00 (git-compat-util: suppress unavoidable
Apple-specific deprecation warnings) to avoid deprecation
warnings.

Instead of blindly setting MAC_OS_X_VERSION_MIN to 1060 change
the definition of DEPRECATED_ATTRIBUTE to empty to avoid the
warnings.  This preserves any MAC_OS_X_VERSION_MIN_REQUIRED
setting while avoiding the warnings as intended by b195aa00.

Signed-off-by: Kyle J. McKay mack...@gmail.com
---
 git-compat-util.h | 9 ++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/git-compat-util.h b/git-compat-util.h
index eb9b0ff3..0efd32c4 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -212,12 +212,15 @@ extern char *gitbasename(char *);
 #endif
 
 #ifndef NO_OPENSSL
+#ifdef __APPLE__
 #define __AVAILABILITY_MACROS_USES_AVAILABILITY 0
-#define MAC_OS_X_VERSION_MIN_REQUIRED MAC_OS_X_VERSION_10_6
+#include AvailabilityMacros.h
+#undef DEPRECATED_ATTRIBUTE
+#define DEPRECATED_ATTRIBUTE
+#undef __AVAILABILITY_MACROS_USES_AVAILABILITY
+#endif
 #include openssl/ssl.h
 #include openssl/err.h
-#undef MAC_OS_X_VERSION_MIN_REQUIRED
-#undef __AVAILABILITY_MACROS_USES_AVAILABILITY
 #ifdef NO_HMAC_CTX_CLEANUP
 #define HMAC_CTX_cleanup HMAC_cleanup
 #endif
--
--
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/2] Makefile: Use the same source directory for ln -s as for ln / cp

[Kyle J. McKay]

On Feb 5, 2015, at 12:59, Junio C Hamano wrote:

I do not know (and I am not quite sure if I want to know) how
serious your potential problems would be, and I do not doubt you
know OS X quirks much better than I do and do not intend to argue
there aren't such problems.  I am just curious how user-facing
comes into the picture.


Suppose you have built and installed some software to /usr/local and  
it's ended up in the usual subdirectories, /usr/local/bin, /usr/local/ 
lib, etc.


Now you get an installer for package X, and to avoid stepping on  
things it installs to /opt/XYZ with the usual suspects /opt/XYZ/bin, / 
opt/XYZ/lib.  There are several of these that I'm aware of where XYZ  
is whatever package you're installing.


Now since /opt/XYZ/bin is not usually in one's PATH it installs a few  
select symlinks from /usr/local/bin/whatever to /opt/XYZ/bin/ 
whatever.  Not all of the /opt/XYZ installers do this, for some of  
them it's a selectable option.


So now we have:

# This one can be a relative or absolute symlink, doesn't matter
/usr/local/bin/foo - /opt/XYZ/bin/foo

And we also now have this:

# A real binary, not a symlink
# Uses and locates libfoo with ../lib/libfoo.dylib
/opt/XYZ/bin/foo

# A real library binary, not a symlink
/opt/XYZ/lib/libfoo.dylib

So /opt/XYZ/bin/foo locates libfoo.dylib by using a relative path  
embedded within it (in this case it would be @loader_path/../lib/ 
libfoo.dylib).


So far, so good, right?

Nothing complicated going on.

But the user has built and installed an older version of libfoo  
already to /usr/local/lib/libfoo.dylib.


Ah-oh.

User attempts to run foo.

Shell finds foo at /usr/local/bin/foo and runs it.

The dynamic linker correctly loads /opt/XYZ/bin/foo and starts to  
resolve the dynamic library links embedded in it.


For the @loader_path/../lib/libfoo.dylib one it first tries /usr/ 
local/bin/../lib/libfoo.dylib rather than /opt/XYZ/bin/foo/../lib/ 
libfoo.dylib because /usr/local/bin/foo was the executable that was  
run and since the user has installed an older version of libfoo.dylib  
in /usr/local/lib, it finds and loads that instead of the intended  
one.  If it didn't find that it would then try /opt/XYZ/bin/foo/../lib/ 
libfoo.dylib and get the right one.


Sometimes you get an immediate error if the unintended library version  
is just too old to be compatible.  On the other hand, if it's just old  
enough to be buggy but not version incompatible now you're running  
with a buggy library or some other incompatibility that doesn't show  
up right away.


I think this is a nasty bug specific to OS X, the fact that it tries  
both paths though suggests it was deliberate.


I have not tested all versions of OS X to see if it might have been  
fixed in the latest, but there it is.


If you put /opt/XYZ/bin directly in the PATH this can't happen  
(provided /opt/XYZ/bin/foo is not itself a symlink).


So that's how a user-facing binary that is a symlink can cause  
problems.  I suppose it's not just limited to user-facing binaries,  
but that's where it tends to show up as it seems the non-user-facing,  
non-executable stuff usually has a sufficiently controlled surrounds  
that they're not vulnerable.


I say user-facing because the binary is found in the user's $PATH.   
I do not consider the libexec/git-core binaries to be user-facing  
since they are not normally in the user's $PATH but rather  normally  
accessed via the git binary which is likely in the user's $PATH.


In the case of Git, a /usr/local/bin/git - ../libexec/git-core/git  
symlink should not be vulnerable to this problem, because even if the  
executable ends up with some relative dylib paths in it (usually not  
the case) when applied to /usr/local/bin they're unlikely to lead  
anywhere with user-installed libraries since git-core is one directory  
deeper than bin.


Anyhow, that's why I tend to avoid putting symlink-to-binary stuff in  
PATH.  In any case a packager can make adjustments to avoid the  
problem before creating an installer.


-Kyle

P.S.

On Feb 5, 2015, at 12:59, Junio C Hamano wrote:

I'm not sure exactly why, but I think:

On Jan 30, 2015, at 13:10, Junio C Hamano wrote:

That would make me feel dirty.


That is a confusing style of quoting.  I suspect that I said the
above in a totally different context.


But I've been dying to work that in somehow ever since I saw that.  ;)
--
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/2] Makefile: Use the same source directory for ln -s as for ln / cp

[Kyle J. McKay]

On Feb 5, 2015, at 07:51, Sebastian Schuberth wrote:
For consistency, we should use the same source for symbolic links as  
for

hard links and copies.

Signed-off-by: Sebastian Schuberth sschube...@gmail.com
---
Makefile | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/Makefile b/Makefile
index c44eb3a..21f23cb 100644
--- a/Makefile
+++ b/Makefile
@@ -2265,14 +2265,14 @@ endif
$(RM) $$bindir/$$p  \
test -z $(NO_INSTALL_HARDLINKS)  \
ln $$bindir/git$X $$bindir/$$p 2/dev/null || \
-ln -s git$X $$bindir/$$p 2/dev/null || \
+ln -s $$bindir/git$X $$bindir/$$p 2/dev/null || \


This is wrong.

Currently with symlinks you will get installed into bindir something  
like this:


  git
  git-tag - git
  git-show - git

etc.

With your change you would have

  git
  git-tag - /usr/local/libexec/git-core/git
  git-show - /usr/local/libexec/git-core/git

And I don't think we want that.  While those absolute path symlinks  
are technically correct, what we have now is much simpler.  While  
there are a number of build-time paths hard-coded into the  
executables, the binaries work for the most part if you relocate them  
as a whole.  Your change totally breaks that.


-Kyle
--
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] Makefile: Also add a symlink fallback to installing ALL_PROGRAMS

[Kyle J. McKay]

On Feb 5, 2015, at 07:52, Sebastian Schuberth wrote:
We do this for BUILT_INS and REMOTE_CURL_ALIASES, so we should do so  
here,

too.

Signed-off-by: Sebastian Schuberth sschube...@gmail.com
---
Makefile | 1 +
1 file changed, 1 insertion(+)

diff --git a/Makefile b/Makefile
index 21f23cb..d2849c3 100644
--- a/Makefile
+++ b/Makefile
@@ -2258,6 +2258,7 @@ endif
$(RM) $$execdir/$$p  \
test -z $(NO_INSTALL_HARDLINKS)$ 
(NO_CROSS_DIRECTORY_HARDLINKS)  \

ln $$bindir/$$p $$execdir/$$p 2/dev/null || \
+ln -s $$bindir/$$p $$execdir/$$p 2/dev/null || \


Can we not start installing absolute path symlinks please.

-Kyle
--
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/2] Makefile: Use the same source directory for ln -s as for ln / cp

[Kyle J. McKay]

On Feb 5, 2015, at 11:51, Jeff King wrote:

On Thu, Feb 05, 2015 at 08:26:08PM +0100, Sebastian Schuberth wrote:


It is not even correct, is it?

When DESTDIR is set to allow you to install into a temporary place
only so that you can tar up the resulting filesystem tree, bindir
points at the location we need to cp the built programs into, i.e.
inside DESTDIR.


Agreed folks, please disregard this as well as 2/2 of this series.


We would still want an equivalent to 2/2 to set up a relative symlink
for $(ALL_PROGRAMS), though, right?


It's this line here:


ln $$bindir/$$p $$execdir/$$p 2/dev/null || \


But since bindir and execdir can be configured to be arbitrary paths  
you must compute the relative path between them in order to create a  
relative symlink.  In principle you just remove the common directory  
prefix, remove the non-directory part from the destination, change any  
remaining directories in the destination to '..' and then append  
what's left of the source to get the new source.


So if you have

  bindir=/usr/local/bin
  execdir=/usr/local/libexec/git-core

And the ln line would be:

  ln /usr/local/bin/git /usr/local/libexec/git-core/git

1) Strip the common prefix which is /usr/local/

  source = bin/git
  dest = libexec/git-core/git

2) remove non-directory part of dest (the basename part) and replace  
remaining dirs with '..'


  source = bin/git
  dest = ../../

3) append source to dest and you get

  ../../bin/git

So the symlink line becomes:

  ln -s ../../bin/git /usr/local/libexec/git-core/git

Now, can you do that easily in a Makefile? ;)

And lastly there's the issue of which should be the symlink and which  
should be the binary?


Should /usr/local/bin/git be the symlink or the binary?

If it's the binary, then /usr/local/libexec/git-core/git will be a  
symlink to it.  But we're already installing several other symlinks to  
'git' in /usr/local/libexec/git-core so they will need to traverse two  
symlinks to get to the binary rather than just the one.


That seems suboptimal.

On the other hand if /usr/local/bin/git becomes the symlink then we  
have a user-facing binary that's a symlink.  As generally it's the  
bindir that ends up in the PATH.


I'm not sure exactly why, but I think:

On Jan 30, 2015, at 13:10, Junio C Hamano wrote:

That would make me feel dirty.



Having a user-facing binary that is actually a symlink can potentially  
cause problems on OS X if the binary it refers to locates its  
libraries using a relative path.  Not sure if that's the case on other  
systems or not.


Neither of these is probably a show-stopper (two-symlinks-to-binary or  
user-facing-symlink-binary) assuming the magic relative symlink  
computation can be worked out.


But it does not surprise me that those items were allowed to skip  
making a symlink and just go directly to cp.


-Kyle
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: git tag --no-merged?

[Kyle J. McKay]

On Feb 4, 2015, at 07:19, Peter Krefting wrote:
Using git branch --no-merged I can get a list of branches that I  
have that I haven't merged into my current branch. git tag doesn't  
have such an option, is there a way to achieve something similar  
listing tags that point to commits that aren't in my history?


I think something like this might do what you want:

git for-each-ref --format='%(refname)' refs/tags | \
while read t; do
  if ! git merge-base --is-ancestor $t HEAD; then
echo ${t#refs/tags/}
  fi
done

If you run that on the Git repository (assuming you've checked out  
master), after a while (there are a lot of tags to check) it spits out  
v1.4.4.5 (along with some complaints about *-gpg-pub tags that refer  
to blobs).


And sure enough, `git log v1.4.4.5 ^master` shows 5 commits not  
contained in master so I think it does what you want.  You'll want to  
restrict the for-each-ref output further, if possible, to make it  
quicker.


-Kyle

--
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: [BUG] Move tracking in git diff is not as good as in git status

[Kyle J. McKay]

On Feb 4, 2015, at 22:11, Scott Schmit wrote:


In my use of git, I've noticed that git status is a lot better at
tracking moves and renames than git diff, and this has recently  
caused

me a lot of headaches because a large number of moves were made in a
single commit, and it was very difficult to figure out which moves  
were

right and which were wrong.

I was using a fairly old version of git (1.7.11), but was able to
reproduce it on git 2.2.1.

Here's a reproduction recipe:

[...]

# Now shift the files
git mv 2 3
git mv 1 2

[...]

git commit -m 2=1;3=2;

# Neither of these commands get it (but -C gets a glimmer of the  
truth)

git diff -M --stat --summary HEAD~..
git diff -C --stat --summary HEAD~..


Ah, but did you try this:

  git diff -B -M --stat --summary HEAD~..


# Swap the files in place
git mv 3 tmp
git mv 2 3
git mv tmp 2

[...]

git commit -m Swap 2  3

# Diff has no idea
git diff -M --stat --summary HEAD~..
git diff -C --stat --summary HEAD~..


Again, try this:

  git diff -B -M --stat --summary HEAD~..

You can even use this:

  git log -B -M --summary

to see them all.

While you can configure -M (or -C) to be on by default (see git config  
diff.renames), there does not appear to be a config option to turn on - 
B (--break-rewrites) by default.


And according to a recent thread [1], using -B and -M together can  
produce incorrect results so you might not want them both on by  
default anyway.


-Kyle

[1] http://thread.gmane.org/gmane.linux.kernel/1879635
--
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] t9001: use older Getopt::Long boolean prefix '--no' rather than '--no-'

[Kyle J. McKay]

On Feb 1, 2015, at 17:33, Junio C Hamano wrote:


Kyle J. McKay mack...@gmail.com writes:


use 5.008;


So either that needs to change or the code should properly deal with
the version of Getopt::Long that comes with 5.8.0.

Since it's really not very difficult or invasive to add support for
the no- variants, here's a patch to do so:


Doesn't that approach add what does --no-no-chain-rely-to even
mean? confusion to the resulting system?  If that is not the case,
then I am all for it, but otherwise, let's not.


No.  You have to append the '!' to get the automagic no prefix  
alternative(s), so while 'chain-reply-to!' means support chain-reply- 
to, nochain-reply-to and (if you have a new enough Getopt::Long) no- 
chain-reply-to, just using 'no-chain-reply-to' without the trailing  
'!' means that nono-chain-reply-to and no-no-chain-reply-to remain  
invalid options that will generate an error.


--
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: Handle broken curl version number in version check

[Kyle J. McKay]

On Jan 30, 2015, at 06:50, Andreas Schwab wrote:


Tom G. Christensen t...@statsbiblioteket.dk writes:


diff --git a/Makefile b/Makefile
index c44eb3a..69a2ce3 100644
--- a/Makefile
+++ b/Makefile
@@ -1035,13 +1035,13 @@ else
REMOTE_CURL_NAMES = $(REMOTE_CURL_PRIMARY) $(REMOTE_CURL_ALIASES)
PROGRAM_OBJS += http-fetch.o
PROGRAMS += $(REMOTE_CURL_NAMES)
-	curl_check := $(shell (echo 070908; curl-config --vernum) 2/dev/ 
null | sort -r | sed -ne 2p)
+	curl_check := $(shell (echo 070908; curl-config --vernum | sed -e  
'/^70[B-C]/ s/^7/07/') 2/dev/null | sort -r | sed -ne 2p)


How about 's/^.$/0/' ?


Much nicer.  But that '$' will have to be escaped from make so it will  
need to be 's/^.$$/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] t9001: use older Getopt::Long boolean prefix '--no' rather than '--no-'

[Kyle J. McKay]

On Jan 30, 2015, at 15:05, brian m. carlson wrote:
 On Fri, Jan 30, 2015 at 07:24:45AM +0100, Tom G. Christensen wrote:
 The '--no-xmailer' option is a Getopt::Long boolean option. The
 '--no-' prefix (as in --no-xmailer) for boolean options is not
 supported in Getopt::Long version 2.32 which was released with Perl  
 5.8.0.
 This version only supports '--no' as in '--noxmailer'.  More recent
 versions of Getopt::Long, such as version 2.34, support either  
 prefix. So
 use the older form in the tests.

 See also:

 d2559f734bba7fe5257720356a92f3b7a5b0d37c
 907a0b1e04ea31cb368e9422df93d8ebb0187914
 84eeb687de7a6c7c42af3fb51b176e0f412a979e
 3fee1fe87144360a1913eab86af9ad136c810076

 Signed-off-by: Tom G. Christensen t...@statsbiblioteket.dk
 ---
 t/t9001-send-email.sh | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

 diff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh
 index af6a3e8..30df6ae 100755
 --- a/t/t9001-send-email.sh
 +++ b/t/t9001-send-email.sh
 @@ -1580,20 +1580,20 @@ do_xmailer_test () {

 test_expect_success $PREREQ '--[no-]xmailer without any  
 configuration' '
  do_xmailer_test 1 --xmailer 
 -do_xmailer_test 0 --no-xmailer
 +do_xmailer_test 0 --noxmailer

 I don't think this is an adequate fix.  The documented option is -- 
 no-xmailer.  If your version of Getopt::Long is not capable of that,  
 then the program doesn't work as documented, and the test is  
 correctly failing.  --noxmailer is not documented at all, so it's  
 not something we should be testing.

It is not alone.  From the git-send-email help these are all boolean  
options:

 git send-email

   Composing:
 --[no-]xmailer
 --[no-]annotate

   Automating:
 --[no-]cc-cover
 --[no-]to-cover
 --[no-]signed-off-by-cc
 --[no-]suppress-from
 --[no-]chain-reply-to
 --[no-]thread

   Administering:
 --[no-]validate
 --[no-]format-patch


Anything done to fix --no-xmailer should be applied for all the other  
--no-... options as well.

 We should probably require a certain version of Getopt::Long or  
 explicitly handle this in the parsing code itself.  I think the  
 former is a better choice, since no security-supported OS still  
 ships with such a positively ancient version.

I don't really like that second option because all the .perl files have:

 use 5.008;

So either that needs to change or the code should properly deal with  
the version of Getopt::Long that comes with 5.8.0.

Since it's really not very difficult or invasive to add support for  
the no- variants, here's a patch to do so:

-- 8 --
Subject: [PATCH] git-send-email.perl: support no- prefix with older GetOptions

Only Perl version 5.8.0 or later is required, but that comes with
an older Getopt::Long (2.32) that does not support the 'no-'
prefix.  Support for that was added in Getopt::Long version 2.33.

Since the help only mentions the 'no-' prefix and not the 'no'
prefix, add explicit support for the 'no-' prefix when running
with older GetOptions versions.

Reported-by: Tom G. Christensen t...@statsbiblioteket.dk
Signed-off-by: Kyle J. McKay mack...@gmail.com
---
 git-send-email.perl | 10 ++
 1 file changed, 10 insertions(+)

diff --git a/git-send-email.perl b/git-send-email.perl
index 3092ab35..a18a7959 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -299,6 +299,7 @@ my $rc = GetOptions(h = \$help,
bcc=s = \@bcclist,
no-bcc = \$no_bcc,
chain-reply-to! = \$chain_reply_to,
+   no-chain-reply-to = sub {$chain_reply_to = 0},
smtp-server=s = \$smtp_server,
smtp-server-option=s = \@smtp_server_options,
smtp-server-port=s = \$smtp_server_port,
@@ -311,25 +312,34 @@ my $rc = GetOptions(h = \$help,
smtp-domain:s = \$smtp_domain,
identity=s = \$identity,
annotate! = \$annotate,
+   no-annotate = sub {$annotate = 0},
compose = \$compose,
quiet = \$quiet,
cc-cmd=s = \$cc_cmd,
suppress-from! = \$suppress_from,
+   no-suppress-from = sub {$suppress_from = 0},
suppress-cc=s = \@suppress_cc,
signed-off-cc|signed-off-by-cc! = \$signed_off_by_cc,
+   no-signed-off-cc|no-signed-off-by-cc = sub 
{$signed_off_by_cc = 0},
cc-cover|cc-cover! = \$cover_cc,
+   no-cc-cover = sub {$cover_cc = 0},
to-cover|to-cover! = \$cover_to,
+   no-to-cover = sub {$cover_to = 0},
confirm=s = \$confirm,
dry-run = \$dry_run,
envelope-sender=s = \$envelope_sender,
thread! = \$thread,
+   no-thread = sub {$thread = 0},
validate! = \$validate

Re: [PATCH] test: add git apply whitespace expansion tests

[Kyle J. McKay]

On Jan 22, 2015, at 11:23, Junio C Hamano wrote:

Kyle J. McKay mack...@gmail.com writes:


On Jan 21, 2015, at 14:33, Junio C Hamano wrote:


Kyle J. McKay mack...@gmail.com writes:


So since I've not been able to get test 2 or 3 to core dump (even
before 250b3c6c) I tend to believe you are correct in that the code
thinks (incorrectly) that the result should fit within the buffer.


Thanks; let me steal your tests when I reroll.


Awesome. :)

But please squash in this tiny change if using the tests verbatim:


Thanks.  I actually have a question wrt the need for $MAKE_PATCHES.

It would have been more natural to do something like:

test_expect_success 'setup' '
printf \t%s\n 1 2 3 4 5 6 before 
printf \t%s\n 1 2 3 after 
printf %64s\n a b c after 
printf \t%s\n 4 5 6 after 
git diff --no-index before after |
sed -e s/before/test-1/ -e s/after/test-1/ patch1.patch 
printf %64s\n 1 2 3 4 5 6 test-1 
printf %64s\n 1 2 3 a b c 4 5 6 expect-1 

printf \t%s\n a b c d e f before 
printf \t%s\n a b c after 
   ...
cat test-4 expect-4 
printf %64s\n a b c expect-4 
while test $x -lt 100
do
printf %63s%02d\n  $x test-4
printf %63s%02d\n  $x expect-4
x=$(( $x + 1 ))
done 

git config core.whitespace tab-in-indent,tabwidth=63 
   git config apply.whitespace fix
'

test_expect_success 'apply with ws expansion (1)' '
git apply patch1.patch 
   test_cmp test-1 expect-1
'

and if you want test files you can just skip tests #2 and later,
without introducing an ad-hoc mechanism like you did.

Was there something more than that that you wanted from
$MAKE_PATCHES?


Well, see I found t/t4135/make-patches that makes patches for use by  
t4135-apply-weird-filenames.sh and thought perhaps that was the  
approved way to do things.


But then it seemed overkill since making the patches takes so little  
time it didn't seem to warrant a separate directory.  But the ability  
to just make the patch files without requiring any external scripts or  
test framework seemed nice so I added those two extra lines to make it  
possible.


I don't have any strong feelings about it.  The setup test plus 4  
explicit tests looks fine.



In any case, here is an update to that sanity check patch to catch
the two cases the BUG did not trigger.

Sometimes the caller under-counted the size of the result but
thought that it would still fit within the original (hence allowing
us to update in-place by passing postlen==0) but the actual result
was larger than the space we have allocated in the postimage,
clobbering the piece of memory after the postimage-buf.


diff --git a/builtin/apply.c b/builtin/apply.c
index 31f8733..3b7ba63 100644
--- a/builtin/apply.c
+++ b/builtin/apply.c
@@ -2171,6 +2171,12 @@ static void update_pre_post_images(struct  
image *preimage,

ctx++;
}

+   if (postlen
+   ? postlen  new - postimage-buf
+   : postimage-len  new - postimage-buf)
+		die(BUG: caller miscounted postlen: asked %d, orig = %d, used =  
%d,
+		(int)postlen, (int) postimage-len, (int)(new - postimage- 
buf));

+
/* Fix the length of the whole thing */
postimage-len = new - postimage-buf;
postimage-nr -= reduced;


Nice.  No more of those bogus results can slip through that somehow  
evade evoking a core dump.


-Kyle
--
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: add git apply whitespace expansion tests

[Kyle J. McKay]

On Jan 21, 2015, at 14:33, Junio C Hamano wrote:


Kyle J. McKay mack...@gmail.com writes:


So since I've not been able to get test 2 or 3 to core dump (even
before 250b3c6c) I tend to believe you are correct in that the code
thinks (incorrectly) that the result should fit within the buffer.


Thanks; let me steal your tests when I reroll.


Awesome. :)

But please squash in this tiny change if using the tests verbatim:

On Jan 18, 2015, at 02:49, Kyle J. McKay wrote:


+#
+## create test-N, patchN.patch, expect-N files
+#
+
+# test 1
+printf '\t%s\n' 1 2 3 4 5 6  before
+printf '\t%s\n' 1 2 3  after
+printf '%64s\n' a b c $x  after


This line ^ in test 1 should not have a '$x' in it.  It should just be:


+printf '%64s\n' a b c  after


The test runs fine currently, but if somehow x should get defined  
before running the tests, test 1 would fail.  All the other '$x' in  
the other tests are correct.



+printf '\t%s\n' 4 5 6  after
+git diff --no-index before after | \
+sed -e 's/before/test-1/' -e 's/after/test-1/'  patch1.patch
+printf '%64s\n' 1 2 3 4 5 6  test-1
+printf '%64s\n' 1 2 3 a b c 4 5 6  expect-1
+
+# test 2


-Kyle
--
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: add git apply whitespace expansion tests

[Kyle J. McKay]

On Jan 18, 2015, at 14:11, Junio C Hamano wrote:

On Sun, Jan 18, 2015 at 2:49 AM, Kyle J. McKay mack...@gmail.com  
wrote:
* Here's some tests.  With apply: make update_pre_post_images()  
sanity

 check the given postlen but not apply: count the size of postimage
 correctly test 1/4 and 4/4 trigger the 'die(BUG: postlen...' but
 test 2/4 and 3/4 do not although they fail because git apply  
generates

 garbage.


Do the failing cases that do not trigger the new postlen check fail
because the original (mis)counting thinks (incorrectly) that the
rewritten result _should_ fit within the original postlen (hence we
allow an in-place rewrite by passing postlen=0 to the helper), but
in fact after rewriting postimage-len ends up being longer due to
the miscounting?


I'm not 100%, but I think so because:

Before 250b3c6c (apply --whitespace=fix: avoid running over the  
postimage buffer, 2013-03-22), tests 1 and 4 tend to easily cause seg  
faults whereas 2 and 3 just give garbage.


After 250b3c6c (apply --whitespace=fix: avoid running over the  
postimage buffer, 2013-03-22), tests 1 and 4 may pass without seg  
faulting although clearly there's some memory corruption going on  
because after apply: make update_pre_post_images() sanity check the  
given postlen they always die with the BUG message.


I created the tests after reading your description in apply: count  
the size of postimage correctly.  I made a guess about what would  
trigger the problem -- I do not have a deep understanding of the  
builtin/apply.c code though.  Tests 2 and 3 were attempts to make the  
discrepancy between counted and needed (assuming the apply: count the  
size of postimage correctly fix has not been applied) progressively  
worse and instead I ended up with a different kind of failure.  Test 4  
was then an alternate attempt to create a very large discrepancy and  
it ended up with BUG: values not that dissimilar from test 1.


FYI, without the counting fix, test 1 causes BUG: postlen = 390, used  
= 585 and test 4 causes BUG: postlen = 396, used = 591.  So while I  
did manage to increase the discrepancy a bit from the values you  
reported for clojure (postlen = 262, used = 273), I was actually  
aiming for a difference big enough to pretty much always guarantee a  
core dump.


The garbage tests 2 and 3 produce without the counting fix is  
reminiscent of what you get when you use memcpy instead of memmove for  
an overlapping memory copy operation.


A slightly modified version of these 4 tests can be run as far back a  
v1.7.4 (when apply --whitespace=fix started fixing tab-in-indent  
errors) and you get core dumps or garbage there too for all 4.


So since I've not been able to get test 2 or 3 to core dump (even  
before 250b3c6c) I tend to believe you are correct in that the code  
thinks (incorrectly) that the result should fit within the buffer.  I  
say buffer because the test 3 patch inserts 100 lines into a 6 line  
file and yet it never seems to cause a core dump (even in v1.7.4), so  
the buffer size must be based on the patch, not the original -- I'm  
sure that would make sense if I understood what's going on in the  
apply code.


I did manage to create a test 5 that causes BUG: postlen = 3036, used  
= 3542, but its verbose output has unfriendly long lines and it's  
fixed by the same counting fix as the others so it doesn't seem  
worthwhile to include it.


-Kyle
--
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: add git apply whitespace expansion tests

[Kyle J. McKay]

When git apply fixes whitespace, the result can end up being
longer than the initial text if whitespace ends up being expanded
(such as with the tab-in-indent option).

Since 250b3c6c (apply --whitespace=fix: avoid running over the
postimage buffer, 2013-03-22) an attempt has been made to compute
the correct final length in such a case.

These tests all stress the whitespace-expansion-during-apply
condition and can result in core dump failures when the final
length is not computed correctly.

Signed-off-by: Kyle J. McKay mack...@gmail.com
---

* Here's some tests.  With apply: make update_pre_post_images() sanity
  check the given postlen but not apply: count the size of postimage
  correctly test 1/4 and 4/4 trigger the 'die(BUG: postlen...' but
  test 2/4 and 3/4 do not although they fail because git apply generates
  garbage.

* After applying apply: count the size of postimage correctly all 4
  tests pass whereas they all fail without that.  It's interesting that
  the BUG: postlen check does not trigger for 2/4 or 3/4 but the output
  is garbage in those cases without the fix.

* Theses tests can easily trigger core dumps.  It seems to depend on how
  the git binary was built and what exactly ends up getting stepped on as
  I have several different Git builds and some of them core dump on tests
  that others pass or just produce garbage for, but none of them passes
  2/4 or 3/4 without the count postimage size correctly fix.

 t/t4138-apply-ws-expansion.sh | 121 ++
 1 file changed, 121 insertions(+)
 create mode 100755 t/t4138-apply-ws-expansion.sh

diff --git a/t/t4138-apply-ws-expansion.sh b/t/t4138-apply-ws-expansion.sh
new file mode 100755
index ..140ed9ac
--- /dev/null
+++ b/t/t4138-apply-ws-expansion.sh
@@ -0,0 +1,121 @@
+#!/bin/sh
+#
+# Copyright (C) 2015 Kyle J. McKay
+#
+
+# NOTE: To facilitate separate testing, this script can be run
+# standalone to just create the test files and do nothing else
+# by first setting the environment variable MAKE_PATCHES=1.
+
+test_description='git apply test patches with whitespace expansion.'
+
+[ -n $MAKE_PATCHES ] || \
+. ./test-lib.sh
+
+#
+## create test-N, patchN.patch, expect-N files
+#
+
+# test 1
+printf '\t%s\n' 1 2 3 4 5 6  before
+printf '\t%s\n' 1 2 3  after
+printf '%64s\n' a b c $x  after
+printf '\t%s\n' 4 5 6  after
+git diff --no-index before after | \
+sed -e 's/before/test-1/' -e 's/after/test-1/'  patch1.patch
+printf '%64s\n' 1 2 3 4 5 6  test-1
+printf '%64s\n' 1 2 3 a b c 4 5 6  expect-1
+
+# test 2
+printf '\t%s\n' a b c d e f  before
+printf '\t%s\n' a b c  after
+n=10
+x=1
+while [ $x -lt $n ]; do
+   printf '%63s%d\n' '' $x  after
+   x=$(( $x + 1 ))
+done
+printf '\t%s\n' d e f  after
+git diff --no-index before after | \
+sed -e 's/before/test-2/' -e 's/after/test-2/'  patch2.patch
+printf '%64s\n' a b c d e f  test-2
+printf '%64s\n' a b c  expect-2
+x=1
+while [ $x -lt $n ]; do
+   printf '%63s%d\n' '' $x  expect-2
+   x=$(( $x + 1 ))
+done
+printf '%64s\n' d e f  expect-2
+
+# test 3
+printf '\t%s\n' a b c d e f  before
+printf '\t%s\n' a b c  after
+n=100
+x=0
+while [ $x -lt $n ]; do
+   printf '%63s%02d\n' '' $x  after
+   x=$(( $x + 1 ))
+done
+printf '\t%s\n' d e f  after
+git diff --no-index before after | \
+sed -e 's/before/test-3/' -e 's/after/test-3/'  patch3.patch
+printf '%64s\n' a b c d e f  test-3
+printf '%64s\n' a b c  expect-3
+x=0
+while [ $x -lt $n ]; do
+   printf '%63s%02d\n' '' $x  expect-3
+   x=$(( $x + 1 ))
+done
+printf '%64s\n' d e f  expect-3
+
+# test 4
+ before
+x=0
+while [ $x -lt 50 ]; do
+   printf '\t%02d\n' $x  before
+   x=$(( $x + 1 ))
+done
+cat before  after
+printf '%64s\n' a b c  after
+while [ $x -lt 100 ]; do
+   printf '\t%02d\n' $x  before
+   printf '\t%02d\n' $x  after
+   x=$(( $x + 1 ))
+done
+git diff --no-index before after | \
+sed -e 's/before/test-4/' -e 's/after/test-4/'  patch4.patch
+ test-4
+x=0
+while [ $x -lt 50 ]; do
+   printf '%63s%02d\n' '' $x  test-4
+   x=$(( $x + 1 ))
+done
+cat test-4  expect-4
+printf '%64s\n' a b c  expect-4
+while [ $x -lt 100 ]; do
+   printf '%63s%02d\n' '' $x  test-4
+   printf '%63s%02d\n' '' $x  expect-4
+   x=$(( $x + 1 ))
+done
+
+# cleanup
+rm before after
+
+[ -z $MAKE_PATCHES ] || exit 0
+
+#
+## Run the tests
+#
+
+# Note that `patch` can successfully apply all patches when run
+# with the --ignore-whitespace option.
+
+for t in 1 2 3 4; do
+   test_expect_success apply with ws expansion (t=$t) '
+   git -c core.whitespace=tab-in-indent,tabwidth=63 \
+   apply --whitespace=fix patch$t.patch 
+   test_cmp test-$t expect-$t
+   '
+done
+
+test_done
--
--
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] t/lib-httpd: switch SANITY check for NOT_ROOT

[Kyle J. McKay]

On Jan 15, 2015, at 19:34, Jeff King wrote:


On Thu, Jan 15, 2015 at 07:27:34PM -0800, Kyle J. McKay wrote:

id -u works for me in MSYS and cygwin (each appears to have it's  
own

id.exe).


That's comforting. MSYS was the one I was most worried about. What UID
do they report? I.e., do they correctly tell us if we are root (or
more accurately, if we are not root)?


It's funny, really.  The MSYS version gives a different answer than  
the cygwin version although both are non-zero.  The MSYS perl gives  
the same answer as the MSYS id and the cygwin perl gives the same  
answer as the cygwin id.


I'm not even sure what it would mean to be root on one of those  
systems.


The closest I can think of would be to run as the SYSTEM user.  And  
that's not nearly as simple as just sudo -s. [1].


I haven't tested that.  I will try to remember to give that a try next  
time I'm feeling the need for some frustration. ;)


-Kyle

[1] http://cygwin.com/ml/cygwin/2010-04/msg00651.html
--
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] t/lib-httpd: switch SANITY check for NOT_ROOT

[Kyle J. McKay]

On Jan 16, 2015, at 01:16, Jeff King wrote:


Subject: [PATCH] t/lib-httpd: switch SANITY check for NOT_ROOT

[...]

We implement NOT_ROOT by checking `id -u`, which is in POSIX
and seems to be available even on MSYS.  Note that we cannot
just call this ROOT and ask for !ROOT. The possible
outcomes are:

 1. we know we are root

 2. we know we are not root

 3. we could not tell, because `id` was not available

We should conservatively treat (3) as does not have the
prerequisite, which means that a naive negation would not
work.

[...]

+
+test_lazy_prereq NOT_ROOT '
+   uid=$(id -u) 
+   test $uid != 0
+'


That looks good to me and worked as expected when I tried it.

-Kyle

--
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] t/lib-httpd: switch SANITY check for NOT_ROOT

[Kyle J. McKay]

On Jan 15, 2015, at 17:32, Jeff King wrote:


On Thu, Jan 15, 2015 at 04:04:24PM -0800, Junio C Hamano wrote:


I wondered what 'perl -e 'print $' would say in mingw, and if that
is portable enough, though.


Good thinking. I guess the best way to find out is to convince  
somebody

from msysgit to try this patch. :)

We may simply find that nobody there even has apache installed on  
their

box, and they do not run the http tests at all.


[...]

We implement NOT_ROOT by checking perl's $ variable,
since we cannot rely on the id program being available
everywhere (and we would rather avoid writing a custom C
program to run geteuid if we can).


Does it make a difference that id is POSIX [1]?

So the test if [ $(id -u) = 0 ] or similar ought to work.

id -u works for me in MSYS and cygwin (each appears to have it's own  
id.exe).



+
+test_lazy_prereq NOT_ROOT '
+   uid=$(perl -e print \$) 
+   test $uid != 0
+'


Does NO_PERL affect this?  Or is Perl always required to run the tests.

Also $ is real user id.  Don't you want effective user id ($),  
that's what the comment says...


Both $ and $ work for me in MSYS and cygwin although if I run it  
from cmd.exe using strawberry perl, both $ and $ give 0.   
(There's no id.exe for cmd.exe unless it finds the cygwin/msys one.)


As long as NO_PERL is not also intended to affect make test either  
the perl or id version seems fine  to me (as long as it's Perl's $)  
since I doubt the tests would run with just cmd.exe. :)


-Kyle

[1] http://pubs.opengroup.org/onlinepubs/009604499/utilities/id.html
--
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: Segmentation fault in git apply

[Kyle J. McKay]

On Jan 14, 2015, at 11:09, Michael Blume wrote:
On Wed, Jan 14, 2015 at 10:58 AM, Michael Blume  
blume.m...@gmail.com wrote:
On Wed, Jan 14, 2015 at 10:48 AM, Michael Blume  
blume.m...@gmail.com wrote:
On Wed, Jan 14, 2015 at 10:44 AM, Michael Blume blume.m...@gmail.com 
 wrote:
On Wed, Jan 14, 2015 at 10:40 AM, Michael Blume blume.m...@gmail.com 
 wrote:
On Wed, Jan 14, 2015 at 10:20 AM, Michael Blume blume.m...@gmail.com 
 wrote:
This is a mac with a fresh build of git from pu branch, commit  
53b80d0.


With my gitconfig looking like

[user]
   email = blume.m...@gmail.com
   name = Michael Blume
[apply]
   whitespace = fix
[core]
   whitespace = fix,trailing-space,space-before-tab, tab-in- 
indent, tabwidth=4


If I run
git clone g...@github.com:MichaelBlume/clojure.git
cd clojure
git checkout origin/rebase-start
git rebase origin/rebase-base

I get

[...]

Applying: CLJ-1295: Speed up dissoc on array-maps
Applying: some throwing
Applying: don't pass offset to ArrayChunk
Applying: make EMPTY accessible
Applying: add handy create methods
Applying: regenerate
Applying: regenerate
/Users/michael.blume/libexec/git-core/git-am: line 854: 92059
Segmentation fault: 11  git apply --index $dotest/patch  / 
dev/null

21


I can reproduce in a 64-bit v2.1.4 as well, but not in a 32-bit v2.1.4  
build.


My recipe is slightly different to facilitate automation:

cd /tmp
git clone git://github.com/MichaelBlume/clojure.git
cd clojure
git config user.email blume.m...@gmail.com
git config user.name Michael Blume
git config apply.whitespace fix
git config core.whitespace \
  fix,trailing-space,space-before-tab, tab-in-indent, tabwidth=4
git checkout origin/rebase-start
git rebase origin/rebase-base

Looks like v1.7.6.6 64-bit works okay.

Running git bisect run...

5782..2890..1445..722..361..179..91..44..23..13..7..3..1..0

And the winner is (first appearing in v1.8.2.2):

commit 250b3c6c992b3cb04e756eb33bed99442fc55193
Author: Junio C Hamano gits...@pobox.com
Date:   Fri Mar 22 11:10:03 2013 -0700

apply --whitespace=fix: avoid running over the postimage buffer

Originally update-pre-post-images could assume that any whitespace
fixing will make the result only shorter by unexpanding runs of
leading SPs into HTs and removing trailing whitespaces at the end  
of

lines.  Updating the post-image we read from the patch to match the
actual result can be performed in-place under this assumption.
These days, however, we have tab-in-indent (aka Python) rule whose
result can be longer than the original, and we do need to allocate
a larger buffer than the input and replace the result.

Fortunately the support for lengthening rewrite was already added
when we began supporting match while ignoring whitespace
differences mode in 86c91f91794c (git apply: option to ignore
whitespace differences, 2009-08-04).  We only need to correctly
count the number of bytes necessary to hold the updated result and
tell the function to allocate a new buffer.

Signed-off-by: Junio C Hamano gits...@pobox.com

And just to confirm, building with 250b3c6c^ (which also happens to be  
v1.8.0.3) does not fail.


And the stack trace from the crash dump of a debug build of 250b3c6c is:

Thread 0 Crashed:
0   libSystem.B.dylib  0x7fff8290242a szone_free + 1222
1   git0x00019fe9 apply_one_fragment + 2164  
(apply.c:2816)
2   git0x0001a760 apply_fragments + 195  
(apply.c:2959)

3   git0x0001b62d apply_data + 96 (apply.c:3340)
4   git0x0001c0b1 check_patch + 869 (apply.c: 
3559)
5   git0x0001c157 check_patch_list + 83  
(apply.c:3574)
6   git0x0001dc70 apply_patch + 646 (apply.c: 
4189)
7   git0x0001ea3a cmd_apply + 2700 (apply.c: 
4418)

8   git0x00011ae8 run_builtin + 402 (git.c:306)
9   git0x00011c9a handle_internal_command +  
181 (git.c:467)

10  git0x00011dab run_argv + 41 (git.c:516)
11  git0x00011ede main + 258 (git.c:588)
12  git0x00010ee8 start + 52

And the gdb backtrace from the core file:

#0  0x7fff8290242a at szone_free + 1222
#1  0x00019fe9 in apply_one_fragment (img=0x7fff5fbfe640,  
frag=0x100400a60, inaccurate_eof=0, ws_rule=3268, nth_fragment=1) at  
builtin/apply.c:2815
#2  0x0001a760 in apply_fragments (img=0x7fff5fbfe640,  
patch=0x1004005e0) at builtin/apply.c:2959
#3  0x0001b62d in apply_data (patch=0x1004005e0,  
st=0x7fff5fbfe6b0, ce=0x1004072e0) at builtin/apply.c:3340
#4  0x0001c0b1 in check_patch (patch=0x1004005e0) at builtin/ 
apply.c:3559
#5  0x0001c157 in check_patch_list (patch=0x1004005e0) at  
builtin/apply.c:3574
#6  0x0001dc70 in apply_patch (fd=3, filename=0x7fff5fbff33a / 

Re: Segmentation fault in git apply

[Kyle J. McKay]

On Jan 15, 2015, at 00:26, Kyle J. McKay wrote:


On Jan 14, 2015, at 11:09, Michael Blume wrote:
On Wed, Jan 14, 2015 at 10:58 AM, Michael Blume  
blume.m...@gmail.com wrote:
On Wed, Jan 14, 2015 at 10:48 AM, Michael Blume blume.m...@gmail.com 
 wrote:
On Wed, Jan 14, 2015 at 10:44 AM, Michael Blume blume.m...@gmail.com 
 wrote:
On Wed, Jan 14, 2015 at 10:40 AM, Michael Blume blume.m...@gmail.com 
 wrote:
On Wed, Jan 14, 2015 at 10:20 AM, Michael Blume blume.m...@gmail.com 
 wrote:
This is a mac with a fresh build of git from pu branch, commit  
53b80d0.


With my gitconfig looking like

[user]
  email = blume.m...@gmail.com
  name = Michael Blume
[apply]
  whitespace = fix
[core]
  whitespace = fix,trailing-space,space-before-tab, tab-in- 
indent, tabwidth=4


If I run
git clone g...@github.com:MichaelBlume/clojure.git
cd clojure
git checkout origin/rebase-start
git rebase origin/rebase-base

I get

[...]

Applying: CLJ-1295: Speed up dissoc on array-maps
Applying: some throwing
Applying: don't pass offset to ArrayChunk
Applying: make EMPTY accessible
Applying: add handy create methods
Applying: regenerate
Applying: regenerate
/Users/michael.blume/libexec/git-core/git-am: line 854: 92059
Segmentation fault: 11  git apply --index $dotest/patch  / 
dev/null

21


I can reproduce in a 64-bit v2.1.4 as well, but not in a 32-bit  
v2.1.4 build.


My recipe is slightly different to facilitate automation:

cd /tmp
git clone git://github.com/MichaelBlume/clojure.git
cd clojure
git config user.email blume.m...@gmail.com
git config user.name Michael Blume
git config apply.whitespace fix
git config core.whitespace \
 fix,trailing-space,space-before-tab, tab-in-indent, tabwidth=4
git checkout origin/rebase-start
git rebase origin/rebase-base

Looks like v1.7.6.6 64-bit works okay.

Running git bisect run...

5782..2890..1445..722..361..179..91..44..23..13..7..3..1..0

And the winner is (first appearing in v1.8.2.2):

commit 250b3c6c992b3cb04e756eb33bed99442fc55193
Author: Junio C Hamano gits...@pobox.com
Date:   Fri Mar 22 11:10:03 2013 -0700

   apply --whitespace=fix: avoid running over the postimage buffer

[...]
And just to confirm, building with 250b3c6c^ (which also happens to  
be v1.8.0.3) does not fail.

[...]
Running with various MallocCheckHeap and MallocErrorAbort settings  
leads to:


git(12926) malloc: *** error for object 0x10040be80: incorrect  
checksum for freed object - object was probably modified after being  
freed.


And a new backtrace from the core file:

#0  0x7fff82962da6 at __kill + 10
#1  0x7fff829c5af8 at szone_error + 476
#2  0x7fff829c7218 at szone_check + 637
#3  0x7fff829caaf8 at malloc_zone_check + 42
#4  0x7fff829cb11d at internal_check + 14
#5  0x7fff828fc939 at malloc_zone_malloc + 60
#6  0x7fff828fc8e0 at malloc + 44
#7  0x000100131ae4 in xmalloc (size=47378) at wrapper.c:50
#8  0x0001950b in update_image (img=0x7fff5fbfe4a0,  
applied_pos=1569, preimage=0x7fff5fbfe340, postimage=0x7fff5fbfe310)  
at builtin/apply.c:2533
#9  0x00019fa7 in apply_one_fragment (img=0x7fff5fbfe4a0,  
frag=0x100400a60, inaccurate_eof=0, ws_rule=3268, nth_fragment=1) at  
builtin/apply.c:2808
#10 0x0001a760 in apply_fragments (img=0x7fff5fbfe4a0,  
patch=0x1004005e0) at builtin/apply.c:2959
#11 0x0001b62d in apply_data (patch=0x1004005e0,  
st=0x7fff5fbfe510, ce=0x1004072e0) at builtin/apply.c:3340
#12 0x0001c0b1 in check_patch (patch=0x1004005e0) at builtin/ 
apply.c:3559
#13 0x0001c157 in check_patch_list (patch=0x1004005e0) at  
builtin/apply.c:3574
#14 0x0001dc70 in apply_patch (fd=9, filename=0x7fff5fbff1e2  
/private/tmp/clojure/.git/rebase-apply/patch, options=0) at  
builtin/apply.c:4189
#15 0x0001ea3a in cmd_apply (argc=1, argv=0x7fff5fbfefe0,  
prefix_=0x0) at builtin/apply.c:4418
#16 0x00011ae8 in run_builtin (p=0x1001a7070, argc=3,  
argv=0x7fff5fbfefe0) at git.c:306
#17 0x00011c9a in handle_internal_command (argc=3,  
argv=0x7fff5fbfefe0) at git.c:467
#18 0x00011dab in run_argv (argcp=0x7fff5fbfef9c,  
argv=0x7fff5fbfef90) at git.c:513
#19 0x00011ede in main (argc=3, argv=0x7fff5fbfefe0) at  
git.c:588


I looked at the code a bit, but a fix does not just jump out at me.   
From the debug info it seems pretty clear that some memory's being  
stepped on.


If I make this change on top of 250b3c6c:

diff --git a/builtin/apply.c b/builtin/apply.c
index df773c75..8795e830 100644
--- a/builtin/apply.c
+++ b/builtin/apply.c
@@ -2390,6 +2390,8 @@ static int match_fragment(struct image *img,
fixed_buf = strbuf_detach(fixed, fixed_len);
if (postlen  postimage-len)
postlen = 0;
+   if (postlen)
+   postlen = 2 * postimage-len;
update_pre_post_images(preimage, postimage,
   fixed_buf, fixed_len, postlen);
return 1;

Then the problem goes away.  That seems to suggest that postlen

Re: t5539 broken under Mac OS X

[Kyle J. McKay]

On Jan 14, 2015, at 13:17, Jeff King wrote:

On Wed, Jan 14, 2015 at 08:50:47PM +0100, Torsten Bögershausen wrote:

But, why does e.g. t0004 behave more gracefully (and skips) and  
t5539 just dies ?


./t0004-unwritable.sh
ok 1 - setup
ok 2 # skip write-tree should notice unwritable repository (missing  
SANITY of POSIXPERM,SANITY)


The http code uses test_skip_or_die when it runs into setup errors.  
The

intent there is that the user has either:

 1. Told us explicitly that they want http tests by setting
GIT_TEST_HTTPD=true.

 2. Wants to run http tests if they can by setting GIT_TEST_HTTPD=auto
(or leaving it unset, as that is the default).

In case (1), we treat this as a test failure. They asked for httpd
tests, and we could not run them. In case (2), we would just skip  
all of

the tests.

You may want to loosen your GIT_TEST_HTTPD setting (pre-83d842dc, you
had to set it to true to run the tests at all, but nowadays we have
auto).


I ran into this problem.  It seems like (at least on older Mac OS X)  
that the root directory is created like so:


  drwxrwxr-t  39 root  admin  /

And since the first (and likely only user) on Mac OS X is a member of  
the admin group, the SANITY test fails and complains even though  
you're not running as root (the failure message is misleading).


I ended up removing group write permission from / (which happened to  
find a bug in another script of mine) and then it was happy.


-Kyle--
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] http-push: trim trailing newline from remote symref

[Kyle J. McKay]

On Jan 12, 2015, at 18:28, Jeff King wrote:


When we fetch a symbolic ref file from the remote, we get
the whole string ref: refs/heads/master\n, recognize it by
skipping past the ref: , and store the rest. We should
chomp the trailing newline.
[..]
This is a regression in v2.1.0.

It was causing t5540 to fail, but I realized I have been building with
NO_EXPAT for a while, so I didn't notice. Frankly, I'm kind of  
surprised

and disturbed that nobody noticed it before now. More evidence that we
can kill off dumb http-push? I would have thought somebody else would
have noticed the test failure, though.


I have this line in my 2.1.4 test output log:

t5540-http-push-webdav.sh .. ok

And again in my 2.2.2 test output log:

t5540-http-push-webdav.sh .. ok

Running the 2.2.2 version with -v I get:

t5540-http-push-webdav.sh ..
ok 1 - setup remote repository
ok 2 - create password-protected repository
ok 3 - setup askpass helper
ok 4 - clone remote repository
ok 5 - push to remote repository with packed refs
ok 6 - push already up-to-date
ok 7 - push to remote repository with unpacked refs
ok 8 - http-push fetches unpacked objects
ok 9 - http-push fetches packed objects
ok 10 - create and delete remote branch
ok 11 - MKCOL sends directory names with trailing slashes
ok 12 - PUT and MOVE sends object to URLs with SHA-1 hash suffix
ok 13 - non-fast-forward push fails
ok 14 - non-fast-forward push show ref status
ok 15 - non-fast-forward push shows help message
not ok 16 - force with lease aka cas # TODO known breakage
ok 17 - push to password-protected repository (user in URL)
not ok 18 - user was prompted only once for password # TODO known  
breakage
not ok 19 - push to password-protected repository (no user in URL) #  
TODO known breakage

# still have 3 known breakage(s)
# passed all remaining 16 test(s)
1..19
ok

I also get the same results using v2.3.0-rc0.

I do not build with NO_EXPAT.  This is running the tests on OS X  
without this patch applied.  Is something else required to get a  
failure?


-Kyle
--
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] http-push: trim trailing newline from remote symref

[Kyle J. McKay]

On Jan 13, 2015, at 11:58, Jeff King wrote:

On Tue, Jan 13, 2015 at 08:26:31AM -0800, Kyle J. McKay wrote:


I have this line in my 2.1.4 test output log:

t5540-http-push-webdav.sh .. ok
[...]
I do not build with NO_EXPAT.  This is running the tests on OS X  
without

this patch applied.  Is something else required to get a failure?


Hmm. I think it is probably this:

 http://curl.haxx.se/docs/adv_20150108B.html

where curl has started complaining about URLs with newlines in them.  
So

ae021d8 did introduce a bug, but older versions of curl did not really
care. The combination of ae021d8 with a new version of curl triggers  
the

problem.


I'm running curl 7.38, and in this context older is anything before  
7.40, so that would explain it.  curl 7.38 was released 2014-09-10, so  
it's only 4 months old at this point.  7.40 was only released 5 days  
ago on 2015-01-08 which is probably why there have not been a whole  
lot of reports about this so far.


And that also explains why it worked prior to eecc8367f4; curl was  
more

forgiving. Also, interestingly, if you git log -S'- 6' http-push.c,
you can see the exact same bug reappear and go away in 2006/2007. The
implicit chop one character behavior is there in the original
3dfaf7bc, adding http-push support. Then it disappears as a side  
effect

of bfbd0bb6, and then comes back again in eecc8367.


After updating to curl 7.40 I get:

t5540-http-push-webdav.sh (Wstat: 256 Tests: 19 Failed: 1)
  Failed test:  10
  Non-zero exit status: 1

Anyway. I think my patch is still the right thing. But that does  
explain

why we didn't notice the test failure.


And then after applying your patch I'm back to:

t5540-http-push-webdav.sh .. ok

So definitely a needed patch.

Tested-by: Kyle J. McKay mack...@gmail.com

-Kyle
--
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] gettext.h: add parentheses around N_ expansion if supported

[Kyle J. McKay]

On Jan 8, 2015, at 11:10, Junio C Hamano wrote:


Kyle J. McKay mack...@gmail.com writes:


For now only __GNUC__ is tested which covers both gcc and clang
which should result in early detection of any adjacent N_ macros.


I didn't check the list of -W options, but if there were a way to
tell gcc to stick to the C standard in a more strict way than its
default, wouldn't this patch start causing trouble?


With this test program:

-BEGIN TEST.C-
#include stdio.h

#define msg1(x) x
#define msg2(x) (x)

static const char *const a1[] = {
msg1(hi),
msg2(bye) /* line 8 */
};

static const char s1[] = msg1(hi);

static const char s2[] = msg2(bye); /* line 13 */

int main()
{
puts(a1[0]);
puts(a1[1]);
puts(s1);
puts(s2);
return 0;
}
-END TEST.C-

gcc, (but not clang) emits a warning (it still compiles just fine)  
when -pedantic is used:


  test.c:13: warning: array initialized from parenthesized string  
constant


However, none of -ansi, -Wall or -Wextra trigger that warning.

Neither does using -ansi -Wall -Wextra together cause a warning to be  
emitted (by either gcc or clang).


Note that line 8 never causes a problem (nor should it), only line 13  
is in question.


After a quick read-through of the -Wxxx options there does not appear  
to be a separate -Wxxx option to get that particular warning.


And compiling Git with -pedantic spits out a LOT of warnings (over  
7200) even before making the (msgid) change so I don't think there's  
an issue as apparently -pedantic is not normally used to compile Git.


Note that Git will not compile with gcc using -ansi (unless you add - 
Dinline=__inline__) and the change does not cause any new warnings to  
be emitted with -ansi (after adding the needed -Dinline=__inline__)  
since -pedantic is required for the parenthesized string constant  
warning.


I'm not super attached to this change, it's just that it seems to me  
that translation support for Git is a scarce resource.  I'm guessing  
that when considering the 7 complete translations (bg, ca, de, fr, sv,  
vi and zh_CN) the average number of translators per language is in the  
low single digits.  So I hate to see unnecessary translation churn,  
not when it can be so easily prevented.


-Kyle


Although the necessary #ifdef makes the header less elegant,
the benefit of avoiding propagation of a translation-marking
error to all the translation teams thus creating extra work
for them when the error is eventually detected and fixed would
seem to outweigh the minor inelegance the #ifdef introduces.

Signed-off-by: Kyle J. McKay mack...@gmail.com
---
gettext.h | 14 +-
1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/gettext.h b/gettext.h
index 7671d09d..80ec29b5 100644
--- a/gettext.h
+++ b/gettext.h
@@ -62,7 +62,19 @@ const char *Q_(const char *msgid, const char  
*plu, unsigned long n)

return ngettext(msgid, plu, n);
}

-/* Mark msgid for translation but do not translate it. */
+/* Mark msgid for translation but do not translate it.
+ *
+ * In order to prevent accidents where two adjacent N_ macros
+ * are mistakenly used, this macro is defined with parentheses
+ * when the compiler is known to support parenthesized string
+ * literal assignments.  This guarantees a compiler error in
+ * such a case rather than a silent conjoining of the strings
+ * by the preprocessor which results in translation failures.
+ */
+#ifdef __GNUC__
+#define N_(msgid) (msgid)
+#else
#define N_(msgid) msgid
+#endif

#endif


--
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] gettext.h: add parentheses around N_ expansion if supported

[Kyle J. McKay]

The N_ macro is used to mark strings for translation without
actually translating them.  At runtime the string is expected
to be passed to the gettext API for translation.

If two N_ macro invocations appear next to each other with only
whitespace (or nothing at all) between them, the two separate
strings will be marked for translation, but the preprocessor
will then combine the strings into one and at runtime the
string passed to gettext will not match the strings that were
translated.

Avoid this by adding parentheses around the expansion of the
N_ macro so that instead of ending up with two adjacent strings
that are then combined by the preprocessor, two adjacent strings
surrounded by parentheses result instead which causes a compile
error so the mistake can be quickly found and corrected.

However, since these string literals are typically assigned to
static variables and not all compilers support parenthesized
string literal assignments, only add the parentheses when the
compiler is known to support the syntax.

For now only __GNUC__ is tested which covers both gcc and clang
which should result in early detection of any adjacent N_ macros.

Although the necessary #ifdef makes the header less elegant,
the benefit of avoiding propagation of a translation-marking
error to all the translation teams thus creating extra work
for them when the error is eventually detected and fixed would
seem to outweigh the minor inelegance the #ifdef introduces.

Signed-off-by: Kyle J. McKay mack...@gmail.com
---
 gettext.h | 14 +-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/gettext.h b/gettext.h
index 7671d09d..80ec29b5 100644
--- a/gettext.h
+++ b/gettext.h
@@ -62,7 +62,19 @@ const char *Q_(const char *msgid, const char *plu, unsigned 
long n)
return ngettext(msgid, plu, n);
 }
 
-/* Mark msgid for translation but do not translate it. */
+/* Mark msgid for translation but do not translate it.
+ *
+ * In order to prevent accidents where two adjacent N_ macros
+ * are mistakenly used, this macro is defined with parentheses
+ * when the compiler is known to support parenthesized string
+ * literal assignments.  This guarantees a compiler error in
+ * such a case rather than a silent conjoining of the strings
+ * by the preprocessor which results in translation failures.
+ */
+#ifdef __GNUC__
+#define N_(msgid) (msgid)
+#else
 #define N_(msgid) msgid
+#endif
 
 #endif
-- 
2.1.4

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 1/2] log.c: fix translation markings

[Kyle J. McKay]

The parse_options API expects an array of alternative usage lines
to which it automatically ads the language-appropriate or when
displaying.  Each of these options is marked for translation with N_
and then later translated when gettext is called on each element
of the array.

Since the N_ macro just expands to its argument, if two N_-marked
strings appear next to each other without being separated by anything
else such as a comma, the preprocessor will join them into one string.

In that case two separate strings get marked for translation, but at
runtime they have been joined into a single string passed to gettext
which then fails to get translated because the combined string was
never marked for translation.

Fix this by properly separating the two N_ marked strings with
a comma and removing the embedded \n andor: that are
properly supplied by the parse_options API.

Signed-off-by: Kyle J. McKay mack...@gmail.com
---
 builtin/log.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/builtin/log.c b/builtin/log.c
index f2a9f015..923ffe72 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -38,8 +38,8 @@ static const char *fmt_patch_subject_prefix = PATCH;
 static const char *fmt_pretty;
 
 static const char * const builtin_log_usage[] = {
-   N_(git log [options] [revision range] [[--] path...]\n)
-   N_(   or: git show [options] object...),
+   N_(git log [options] [revision range] [[--] path...]),
+   N_(git show [options] object...),
NULL
 };
 
-- 
2.1.4

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 2/2] gettext.h: add parentheses around N_ expansion

[Kyle J. McKay]

The N_ macro is used to mark strings for translation without
actually translating them.  At runtime the string is expected
to be passed to the gettext API for translation.

If two N_ macro invocations appear next to each other with only
whitespace (or nothing at all) between them, the two separate
strings will be marked for translation, but the preprocessor
will then combine the strings into one and at runtime the
string passed to gettext will not match the strings that were
translated.

Avoid this by adding parentheses around the expansion of the
N_ macro so that instead of ending up with two adjacent strings
that are then combined by the preprocessor, two adjacent strings
surrounded by parentheses result instead which causes a compile
error so the mistake can be quickly found and corrected.

Signed-off-by: Kyle J. McKay mack...@gmail.com
---
This patch is optional, but prevents the problem fixed by 1/2
from recurring.

 gettext.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/gettext.h b/gettext.h
index 7671d09d..d11a4139 100644
--- a/gettext.h
+++ b/gettext.h
@@ -63,6 +63,6 @@ const char *Q_(const char *msgid, const char *plu, unsigned 
long n)
 }
 
 /* Mark msgid for translation but do not translate it. */
-#define N_(msgid) msgid
+#define N_(msgid) (msgid)
 
 #endif
-- 
2.1.4

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 1/2] notes: accept any ref

[Kyle J. McKay]

From: Scott Chacon scha...@gmail.com

Currently if you try to merge notes, the notes code ensures that the
reference is under the 'refs/notes' namespace. In order to do any sort
of collaborative workflow, this doesn't work well as you can't easily
have local notes refs seperate from remote notes refs.

This patch changes the expand_notes_ref function to check for simply a
leading refs/ instead of refs/notes to check if we're being passed an
expanded notes reference. This would allow us to set up
refs/remotes-notes or otherwise keep mergeable notes references outside
of what would be contained in the notes push refspec.

Signed-off-by: Scott Chacon scha...@gmail.com
---
 notes.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/notes.c b/notes.c
index c763a21e..7165a33f 100644
--- a/notes.c
+++ b/notes.c
@@ -1292,7 +1292,7 @@ int copy_note(struct notes_tree *t,
 
 void expand_notes_ref(struct strbuf *sb)
 {
-   if (starts_with(sb-buf, refs/notes/))
+   if (starts_with(sb-buf, refs/))
return; /* we're happy */
else if (starts_with(sb-buf, notes/))
strbuf_insert(sb, 0, refs/, 5);
-- 
2.1.4

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 0/2] Accept any notes ref

[Kyle J. McKay]

This is a resend of the original patch by Scott Chacon combined with the
suggested patch by Johan Herland with a summary of the discussion so far
in the hope to restart discussion about including this change.

The original thread can be found at [1].

The history so far:

The Patch 1/2 was originally submitted to allow using refs outside of
the refs/notes/ namespace as notes refs for the purpose of merging notes.
However, that patch actually just relaxes the restriction on notes refs so
that a fully-qualified ref will be used as-is in the notes code and affects
more than just notes merging.

The concern was then raised that this is a stealth-enabler patch and that
even if notes refs are allowed outside of refs/notes/ then they should still
be restricted to a some subset (to be determined) of the the refs/ hierarchy
and not just allowed anywhere.

But, I feel that the rest of Git does not restrict the user in this way -- if
the user really wants to do something that is not recommended there is
almost always a mechanism and/or option to leave the user in control and allow
it despite any recommendation(s) to the contrary.

So I am resending this summary with attached patches (both of which are
very trivial) to allow using any ref for notes as long as it's fully qualified
(i.e. starts with refs/).  Patch 1/2 does that and patch 2/2 fixes the test
that breaks because of it.

In the hope of restarting discussion towards enabling use of refs outside of
refs/notes/ with the notes machinery I conclude by including Peff's final
reply to the original thread which I think contains the most compelling
aregument for inclusion:

On Dec 4, 2014, at 02:26, Jeff King wrote:

 On Sat, Nov 22, 2014 at 10:04:57AM -0800, Kyle J. McKay wrote:

 On Sep 19, 2014, at 11:22, Junio C Hamano wrote:

 By stealth enabler I mean the removal of refs/notes/ restriction
 that was originally done as a safety measure to avoid mistakes of
 storing notes outside.  The refs/remote-notes/ future direction
 declares that it is no longer a mistake to store notes outside
 refs/notes/, but that does not necessarily have to mean that
 anywhere under refs/ is fine.  It may make more sense to be explicit
 with the code touched here to allow traditional refs/notes/ and the
 new hierarchy only.  That way, we will still keep the avoid
 mistakes safety and enable the new layout at the same time.

 This is the part where I want to lobby for inclusion of this change.
 I do not believe it is consistent with existing Git practice to
 enforce restrictions like this (you can only display/edit/etc. notes
 under refs/notes/*).

 Yeah, this is the compelling part to me. There is literally no way to
 utilize the notes codes for any ref outside of refs/notes currently.
 We don't know if refs/remote-notes is the future, or refs/remotes/
 origin/notes, or something else. But we can't even experiment with it in a
 meaningful way because the plumbing layer is so restrictive.

 The notes feature has stagnated. Not many people use it because it  
 requires so much magic to set up and share notes. I think it makes sense to  
 remove a safety feature that is making it harder to experiment with. If the  
 worst case is that people start actually _using_ notes and we get  
 proliferation of places that people are sticking them in the refs hierarchy,
 that is vastly preferable IMHO to the current state, in which few people use
 them and there is little support for sharing them at all.

-Kyle

[1] http://thread.gmane.org/gmane.comp.version-control.git/257281

Kyle J. McKay (1):
  t/t3308-notes-merge.sh: succeed with relaxed notes refs

Scott Chacon (1):
  notes: accept any ref

 notes.c| 2 +-
 t/t3308-notes-merge.sh | 1 -
 2 files changed, 1 insertion(+), 2 deletions(-)

-- 
2.1.4

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 2/2] t/t3308-notes-merge.sh: succeed with relaxed notes refs

[Kyle J. McKay]

With the recent change to allow notes refs to be located in
the refs hierarchy in locations other than refs/notes/ the
'git notes merge refs/heads/master' test started succeeding.

Previously refs/heads/master would have been expanded to
a non-existing, ref refs/notes/refs/heads/master, and the
merge would have failed (as expected).

Now, however, since refs/heads/master exists and the new,
more relaxed notes refs rules leave it unchanged, the merge
succeeds.  This has a follow-on effect which makes the
next two tests fail as well.

The refs/heads/master ref could just be replaced with
another ref name that does not exist such as refs/heads/xmaster,
but there are already several tests using non-existant refs
so instead just remove the refs/heads/master line.

Suggested-by: Johan Herland jo...@herland.net
Signed-off-by: Kyle J. McKay mack...@gmail.com
---
 t/t3308-notes-merge.sh | 1 -
 1 file changed, 1 deletion(-)

diff --git a/t/t3308-notes-merge.sh b/t/t3308-notes-merge.sh
index 24d82b49..f0feb64b 100755
--- a/t/t3308-notes-merge.sh
+++ b/t/t3308-notes-merge.sh
@@ -90,7 +90,6 @@ test_expect_success 'fail to merge various non-note-trees' '
test_must_fail git notes merge refs/notes/ 
test_must_fail git notes merge refs/notes/dir 
test_must_fail git notes merge refs/notes/dir/ 
-   test_must_fail git notes merge refs/heads/master 
test_must_fail git notes merge x: 
test_must_fail git notes merge x:foo 
test_must_fail git notes merge foo^{bar
-- 
2.1.4

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] git-gui.sh: support Tcl 8.4

[Kyle J. McKay]

Tcl 8.5 introduced an extended vsatisfies syntax that is not
supported by Tcl 8.4.

Since only Tcl 8.4 is required this presents a problem.

The extended syntax was used starting with Git 2.0.0 in
commit b3f0c5c0 so that a major version change would still
satisfy the condition.

However, what we really want is just a basic version compare,
so use vcompare instead to restore compatibility with Tcl 8.4.

Signed-off-by: Kyle J. McKay
---
 git-gui/git-gui.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/git-gui/git-gui.sh b/git-gui/git-gui.sh
index b186329d..a1a23b56 100755
--- a/git-gui/git-gui.sh
+++ b/git-gui/git-gui.sh
@@ -1283,7 +1283,7 @@ load_config 0
 apply_config
 
 # v1.7.0 introduced --show-toplevel to return the canonical work-tree
-if {[package vsatisfies $_git_version 1.7.0-]} {
+if {[package vcompare $_git_version 1.7.0] = 0} {
if { [is_Cygwin] } {
catch {set _gitworktree [exec cygpath --windows [git rev-parse 
--show-toplevel]]}
} else {
@@ -1539,7 +1539,7 @@ proc rescan_stage2 {fd after} {
close $fd
}
 
-   if {[package vsatisfies $::_git_version 1.6.3-]} {
+   if {[package vcompare $::_git_version 1.6.3] = 0} {
set ls_others [list --exclude-standard]
} else {
set ls_others [list --exclude-per-directory=.gitignore]
-- 
2.1.4

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] imap-send.c: support GIT_CURL_VERBOSE

[Kyle J. McKay]

When using git-imap-send to send via cURL, support setting
the GIT_CURL_VERBOSE environment variable to enable cURL's
verbose mode.

The existing http.c code already supports this and does
it by simply checking to see whether or not the environment
variable exists -- it does not examine the value at all.

For consistency, enable CURLOPT_VERBOSE when GIT_CURL_VERBOSE
is set by using the exact same test that http.c does.

Signed-off-by: Kyle J. McKay mack...@gmail.com
---

*** PATCH IS AGAINST NEXT ***

In particular, this patch requires br/imap-send-via-libcurl


 imap-send.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/imap-send.c b/imap-send.c
index 4dfe4c25..060df834 100644
--- a/imap-send.c
+++ b/imap-send.c
@@ -1431,7 +1431,7 @@ static CURL *setup_curl(struct imap_server_conf *srvc)
 
curl_easy_setopt(curl, CURLOPT_UPLOAD, 1L);
 
-   if (0  verbosity)
+   if (0  verbosity || getenv(GIT_CURL_VERBOSE))
curl_easy_setopt(curl, CURLOPT_VERBOSE, 1L);
 
return curl;
-- 
2.1.4

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] imap-send.c: set CURLOPT_USE_SSL to CURLUSESSL_TRY

[Kyle J. McKay]

According to the cURL documentation for the CURLOPT_USE_SSL option,
it is only used with plain text protocols that get upgraded to SSL
using the STARTTLS command.

The server.use_ssl variable is only set when we are using a protocol
that is already SSL/TLS (i.e. imaps), so setting CURLOPT_USE_SSL
when the server.use_ssl variable is set has no effect whatsoever.

Instead, set CURLOPT_USE_SSL to CURLUSESSL_TRY when the server.use_ssl
variable is NOT set so that cURL will attempt to upgrade the plain
text connection to SSL/TLS using STARTTLS in that case.

This much more closely matches the behavior of the non-cURL code path.

Signed-off-by: Kyle J. McKay mack...@gmail.com
---

*** PATCH IS AGAINST NEXT ***

In particular, this patch requires br/imap-send-via-libcurl


 imap-send.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/imap-send.c b/imap-send.c
index 4dfe4c25..5251b750 100644
--- a/imap-send.c
+++ b/imap-send.c
@@ -1421,8 +1421,8 @@ static CURL *setup_curl(struct imap_server_conf *srvc)
strbuf_release(auth);
}
 
-   if (server.use_ssl)
-   curl_easy_setopt(curl, CURLOPT_USE_SSL, (long)CURLUSESSL_ALL);
+   if (!server.use_ssl)
+   curl_easy_setopt(curl, CURLOPT_USE_SSL, (long)CURLUSESSL_TRY);
 
curl_easy_setopt(curl, CURLOPT_SSL_VERIFYPEER, server.ssl_verify);
curl_easy_setopt(curl, CURLOPT_SSL_VERIFYHOST, server.ssl_verify);
-- 
2.1.4

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 2/2] t/t3308-notes-merge.sh: succeed with relaxed notes refs

[Kyle J. McKay]

On Jan 6, 2015, at 02:20, Junio C Hamano wrote:


Kyle J. McKay mack...@gmail.com writes:


Now, however, since refs/heads/master exists and the new,
more relaxed notes refs rules leave it unchanged, the merge
succeeds. ...
...
diff --git a/t/t3308-notes-merge.sh b/t/t3308-notes-merge.sh
index 24d82b49..f0feb64b 100755
--- a/t/t3308-notes-merge.sh
+++ b/t/t3308-notes-merge.sh
@@ -90,7 +90,6 @@ test_expect_success 'fail to merge various non- 
note-trees' '

test_must_fail git notes merge refs/notes/ 
test_must_fail git notes merge refs/notes/dir 
test_must_fail git notes merge refs/notes/dir/ 
-   test_must_fail git notes merge refs/heads/master 
test_must_fail git notes merge x: 
test_must_fail git notes merge x:foo 
test_must_fail git notes merge foo^{bar


The test title reads fail to merge non-note trees, and I am
assuming that the tree-ish refs/heads/master (aka 'master' branch)
represents does not look anything like a typical note tree where
pathnames are 40-hex with fan-out.


In fact it looks like this:

100644 blob 2a5d0158a25a97e8ebf4158d9187acb124da50ea1st.t
100644 blob 3f514b8c0d8e9345bda64de1664eb43d7d38d12a2nd.t
100644 blob e5404b81e697da4f0c99aac167b5e63bcce4b78b3rd.t
100644 blob c950fbad52232390031696035ad79c670ee3bd7b4th.t
100644 blob ba96e617c4d2741ac7693ca7eb20f9dddf4754f65th.t

so you are correct.


The fact that git notes merge refs/heads/master fails is a very
good prevention of end-user mistakes, and this removal of test
demonstrates that we are dropping a valuable safety.


At the point the dropped line runs, core.notesRef has been set to refs/ 
notes/y which does not exist.


All of the tests in the 'fail to merge various non-note-trees' section  
fail with one of these errors:


  1) Failed to resolve remote notes ref 'ref-being-tested'

  2) Cannot merge empty notes ref (ref-being-tested) into empty  
notes ref (refs/notes/y)


  3) error: object 6c99d48c9905deea5d59d723468862362918626a is a  
tree, not a commit


The 3rd error comes from the git notes merge x: attempt.

So despite the name of the test, the actual tree contents do not seem  
to be examined.


When the notes ref checking is relaxed to leave refs/heads/master  
alone rather than turning it into refs/notes/refs/heads/master, the  
previous error (#2 in this case) goes away and since refs/notes/y does  
not exist, it is simply updated to the value of refs/heads/master  
without any checks.  Of course that refs/heads/master tree doesn't  
look like a notes tree.


And if we do this:

  git update-ref refs/notes/refs/heads/master master

Then git notes merge refs/heads/master also succeeds without  
complaining that the refs/notes/refs/heads/master tree does not look  
like a notes tree and we didn't need to relax the refs/notes  
restriction and, as you point out, the name of the test seems to imply  
that would be rejected.


Interestingly, if we then attempt to merge refs/notes/x into this non- 
notes refs/notes/y tree, it also succeeds and even keeps the things  
that do not look like notes.  The reverse merge (y into x) succeeds as  
well, but the non-notes stuff in y is not merged in in that case.



Arguably, not being able to save notes tree anywhere outside of
refs/notes/ hierarchy may be too high a price to pay in order to
prevent refs/heads/master from being considered (hence to avoid such
end-user mistakes), but at the same time, losing this safetly may
also be too high a price to pay in order to allow people to store
their notes in somewhere outside e.g. refs/remote-notes/origin/foo.
Somewhere outside does not mean Including other hierarchies like
refs/heads and refs/tags that have long established meaning.


If we relax the refs/notes restriction, putting a notes ref in refs/ 
heads/whatever doesn't necessarily seem like that's a terrible thing  
as long as it's really a notes tree if used with the notes machinery.   
AIUI, the refs/heads convention only requires the ref to point to the  
tip of a commit chain which all of the refs under refs/notes satisfy.   
The refs/heads convention AIUI does not impose any requirement about  
the contents of the tree(s) those commits in the chain refer to.  But  
at the same time I can't think of any particular reason I'd want to  
store notes refs in there either.



Although I am not fundamentally against allowing to store notes
outside refs/notes/, it is different from anywhere is fine.
Can't we do this widening in a less damaging way?


Without arbitrarily restricting where notes can be stored it seems to  
me the only option would be to have the notes machinery actually  
inspect the tree of any existing notes ref it's passed.  That would  
also catch the case where git update-ref refs/notes/refs/heads/master  
master was run as well.  It also seems like a good check to have in  
place to help catch user errors.


I'm not all that familiar with the notes code, perhaps there's already