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

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, &mi->inbody_header_accum, mi->s_hdr_data, 0));
+   okay = check_header(mi, &mi->inbody_header_accum, mi->s_hdr_data, 0);
+   assert(okay);
strbuf_reset(&mi->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 
---

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, &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 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  writes:
>
>>>> This is obviously an improvement, but it makes me wonder if we  
>>>> should be
>>>> doing:
>>>>
>>>> if (!check_header(mi, &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 
Helped-by: Jeff King 
Acked-by: Johannes Schindelin 
Signed-off-by: Kyle J. McKay 
---

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, &mi->inbody_header_accum, mi->s_hdr_data, 0));
+   if (!check_header(mi, &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(&mi->inbody_header_accum);
 }
 
---

[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"  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 
Helped-by: Jeff King 
Acked-by: Johannes Schindelin 
Signed-off-by: Kyle J. McKay 
---

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, &mi->inbody_header_accum, mi->s_hdr_data, 0));
+   if (!check_header(mi, &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(&mi->inbody_header_accum);
 }
 
---

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

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

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

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

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

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

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

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

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

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

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] 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 
---
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;
}

Re: [BUG?] git http connection reuse

[Kyle J. McKay]

On Feb 15, 2014, at 20:05, Jeff King wrote:
I've noticed that git does not reuse http connections when fetching,  
and

I'm trying to figure out why. It seems related to our use of two curl
features:

 1. If we use the curl_multi interface (even though we are doing the
requests sequentially), we do not get reuse.


Take a look at the curl_multi_setopt page [1].  I think these explain  
the curl_multi issue:



CURLMOPT_PIPELINING

Pass a long set to 1 to enable or 0 to disable. Enabling pipelining  
on a multi handle will make it attempt to perform HTTP Pipelining as  
far as possible for transfers using this handle. This means that if  
you add a second request that can use an already existing  
connection, the second request will be "piped" on the same  
connection rather than being executed in parallel. (Added in 7.16.0)



CURLMOPT_MAX_TOTAL_CONNECTIONS

Pass a long. The set number will be used as the maximum amount of  
simultaneously open connections in total. For each new session,  
libcurl will open a new connection up to the limit set by  
CURLMOPT_MAX_TOTAL_CONNECTIONS. When the limit is reached, the  
sessions will be pending until there are available connections. If  
CURLMOPT_PIPELINING is 1, libcurl will try to pipeline if the host  
is capable of it.


The default value is 0, which means that there is no limit. However,  
for backwards compatibility, setting it to 0 when  
CURLMOPT_PIPELINING is 1 will not be treated as unlimited. Instead  
it will open only 1 connection and try to pipeline on it.


(Added in 7.30.0)


If pipelining is off (the default) and total connections is not 1 it  
sounds to me from the description above that the requests will be  
executed on separate connections until the maximum number of  
connections is in use and then there might be some reuse.  That might  
not be what's actually going on with multi, but that's my guess and I  
think setting CURLMOPT_PIPELINING to to 1 and then also setting  
CURLMOPT_MAX_TOTAL_CONNECTIONS to a non-zero value might be what you  
want.



 2. If we set CURLOPT_HTTPAUTH to CURLAUTH_ANY, we do not get reuse.

[snip]

My curl version is 7.35.0, if that makes a difference.


And that is explained by this from the CHANGES file:


Daniel Stenberg (7 Jan 2014)
- ConnectionExists: fix NTLM check for new connection

  When the requested authentication bitmask includes NTLM, we cannot
  re-use a connection for another username/password as we then risk
  re-using NTLM (connection-based auth).

  This has the unfortunate downside that if you include NTLM as a  
possible
  auth, you cannot re-use connections for other usernames/passwords  
even

  if NTLM doesn't end up the auth type used.

  Reported-by: Paras S
  Patched-by: Paras S
  Bug: http://curl.haxx.se/mail/lib-2014-01/0046.html


Looks like you're just lucky as that above change first appears in  
7.35.0.  But it seems there are some patches for older versions so  
they might be affected as well [2].


Adjusting your test program to leave the first connection set to  
CURLAUTH_ANY, but then setting the second one to CURLAUTH_ANY with the  
two NTLM bits turned off (CURLAUTH_NTLM and CURLAUTH_NTLM_WB) results  
in the connection being reused for me. :)


With the older cURL versions I already had installed, the second  
connection is always reused.  I didn't see the non-reuse with your  
sample code until I fetched 7.35.0.


--Kyle

[1] http://curl.haxx.se/libcurl/c/curl_multi_setopt.html
[2] http://curl.haxx.se/docs/adv_20140129.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: [BUG] Halt during fetch on MacOS

[Kyle J. McKay]

On Feb 28, 2014, at 22:15, Jeff King wrote:

On Fri, Feb 28, 2014 at 03:26:28PM -0800, Conley Owens wrote:


test.sh
"
#!/bin/bash
rungit() {
   mkdir $1
   GIT_DIR=$1 git init --bare
   echo '[remote "aosp"]' > $1/config
   echo 'url =
https://android.googlesource.com/platform/external/tinyxml2' >>
$1/config
   GIT_DIR=$1 git fetch aosp +refs/heads/master:refs/remotes/aosp/ 
master


I don't think this is affecting your test, but you probably want  
">>" to

append to the config for the first line, too. Otherwise you are
overwriting some of git's default settings.


I replaced it with a call to git config in my version.

When everything cools, you can see that there are some fetches  
hanging

(typically).
$ ps | grep 'git fetch'
...
63310 ttys0040:00.01 git fetch aosp
+refs/heads/master:refs/remotes/aosp/master
[...]


I can't reproduce here on Linux. Can you find out what the processes  
are

doing with something like strace?


I can't reproduce, mostly, on Mac OS X 10.5.8 or 10.6.8.

What I mean by mostly is that the very first time I ran the test  
script I got approximately 36 of these errors:


fatal: unable to access 'https://android.googlesource.com/platform/external/tinyxml2/' 
: Unknown SSL protocol error in connection to android.googlesource.com: 
443


The rest of the fetches completed.  That was with Git 1.8.5.1.

However, I was never able to reproduce those errors again.  All the  
subsequent runs completed all fetches successfully using that same Git  
version so I also tried Git 1.8.5.2, 1.8.5.5 and Git 1.7.6.1 on the  
original and another machine.


I am, however NAT'd, so it's possible the NAT was somehow responsible  
for the initial 36 failures.


Perhaps you are seeing a similar issue.

You might try setting these sysctl variables:

# Timeout new TCP connections after 30 seconds instead of 75
net.inet.tcp.keepinit=3
# Always keep alive TCP connections
net.inet.tcp.always_keepalive=1
# Start keep alive checks after 30 seconds instead of 2 hours
net.inet.tcp.keepidle=3
# Wait 5 seconds between probes instead of 75
# Note that 8 probes in a row must fail to drop the connection
net.inet.tcp.keepintvl=5000

then running your test again and see if the hanging git fetch  
processes die with some kind of failed connection error within about  
70 seconds or so.  With the default sysctl settings, even with Git  
enabling keep alives, it would likely take a bit over two hours for a  
dead connection to be noticed.


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

Potential GSOC microproject idea

[Kyle J. McKay]

On Mar 3, 2014, at 23:58, Michael Haggerty wrote:

list
regulars should FEEL ENCOURAGED to submit microprojects to add to the
list.  (Either submit them as a pull request to the GitHub repository
that contains the text [1] or to the mailing list with CC to me.)


Potential idea for a microproject:

Add a new config setting:

  user.allowImplicitIdentity
Defaults to true.
If user.name and GIT_COMMITTER_NAME are unset or user.email and
GIT_COMMITTER_EMAIL and EMAIL are unset, an implicit value is
substituted for one or both of user.name and user.email.  If
an automatically generated value is used for both name and email
a warning "Your name and email address were configured  
automatically..."

is displayed.  If set to false, no or never, instead of a warning,
an error is generated and the operation fails:

*** Please tell me who you are.

Run

  git config --global user.email "y...@example.com"
  git config --global user.name "Your Name"

to set your account's default identity.
Omit --global to set the identity only in this repository.

fatal: implicit user identity not allowed

the advice portion can be suppressed by setting  
advice.implicitIdentity to false,

but not the "fatal: implicit user identity not allowed" part.

Note that if "git config --system --bool user.allowImplicitIdentity  
false" is in effect, it should still be possible to clone (ref logs  
may be updated, but they should be allowed to use an implicit  
identity).  In other words user.allowImplicitIdentity=false should  
only inhibit writing any new commit/tag objects that need the current  
user's name and email when it has not been explicitly provided.


I'm not sure how micro this is.  :)  I do think the amount of code  
involved is rather small though.  Support for something like this has  
popped up on the list before.  Perhaps "user.allowAutomaticIdentity"  
and "advice.automaticIdentity" would be better config names.


--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: fix t7001 cp to use POSIX options

[Kyle J. McKay]

Since 11502468 and 04c1ee57 (both first appearing in v1.8.5), the
t7001-mv test has used "cp -a" to perform a copy in several of the
tests.

However, the "-a" option is not required for a POSIX cp utility and
some platforms' cp utilities do not support it.

The POSIX equivalent of -a is -R -P -p.

Change "cp -a" to "cp -R -P -p" so that the t7001-mv test works
on systems with a cp utility that only implements the POSIX
required set of options and not the "-a" option.

Signed-off-by: Kyle J. McKay 

---
 t/t7001-mv.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/t/t7001-mv.sh b/t/t7001-mv.sh
index 215d43d6..c8ff9115 100755
--- a/t/t7001-mv.sh
+++ b/t/t7001-mv.sh
@@ -308,7 +308,7 @@ test_expect_success 'git mv moves a submodule with a .git 
directory and no .gitm
(
cd sub &&
rm -f .git &&
-   cp -a ../.git/modules/sub .git &&
+   cp -R -P -p ../.git/modules/sub .git &&
GIT_WORK_TREE=. git config --unset core.worktree
) &&
mkdir mod &&
@@ -331,7 +331,7 @@ test_expect_success 'git mv moves a submodule with a .git 
directory and .gitmodu
(
cd sub &&
rm -f .git &&
-   cp -a ../.git/modules/sub .git &&
+   cp -R -P -p ../.git/modules/sub .git &&
GIT_WORK_TREE=. git config --unset core.worktree
) &&
mkdir mod &&
-- 
tg: (0bc85abb..) t/t7001-posix-cp (depends on: maint)
--
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 0/3] Fix support for FreeBSD's /bin/sh

[Kyle J. McKay]

This patch series words around the FreeBSD /bin/sh problems that cause rebase
to fail on FreeBSD as well as test t5560-http-backend-noserver.

The rebase issue was first introduced in Git v1.8.4 which started using the
"return" statement to return from a "dot" command script where the "dot"
command itself was located inside a function.  This behaves badly when using
FreeBSD's /bin/sh.  An attempt was made to fix this in Git v1.8.4.1, but it
only addressed part of the problem.

Patch 1/3 fixes the problem by eliminating such use of "return", patch 2/3
then reverts the earlier workaround since it is no longer needed and did
not fully solve the problem.

The t5560 issue was first introduced in Git v1.7.0 which started using a
backslash escapes in ${...} expansions.  The FreeBSD /bin/sh does not
properly support such escapes, but there is a simple change that works
everywhere (using [?] instead of \?).

There are more details in the individual patches.

This patch series is based on maint since these are bug fixes and that's
what SubmittingPatches says to do...

Kyle J. McKay (3):
  rebase: avoid non-function use of "return" on FreeBSD
  Revert "rebase: fix run_specific_rebase's use of "return" on FreeBSD"
  test: fix t5560 on FreeBSD

 git-rebase--am.sh| 131 +++
 git-rebase--interactive.sh   | 341 ---
 git-rebase--merge.sh |  87 +-
 git-rebase.sh|  11 +-
 t/t5560-http-backend-noserver.sh |   4 +-
 5 files changed, 287 insertions(+), 287 deletions(-)

The above stat may seem like a lot, but when using diff -w --stat you get this:

 git-rebase--am.sh|  3 +++
 git-rebase--interactive.sh   |  3 +++
 git-rebase--merge.sh |  3 +++
 git-rebase.sh| 11 +--
 t/t5560-http-backend-noserver.sh |  4 ++--
 5 files changed, 12 insertions(+), 12 deletions(-)

which more accurately reflects what was actually touched.
--
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/3] Revert "rebase: fix run_specific_rebase's use of "return" on FreeBSD"

[Kyle J. McKay]

This reverts commit 99855ddf4bd319cd06a0524e755ab1c1b7d39f3b.

The workaround 99855ddf introduced to deal with problematic
"return" statements in scripts run by "dot" commands located
inside functions only handles one part of the problem.  The
issue has now been addressed by not using "return" statements
in this way in the git-rebase--*.sh scripts.

This workaround is therefore no longer necessary, so clean
up the code by reverting it.

Conflicts:
git-rebase.sh

Signed-off-by: Kyle J. McKay 

---
 git-rebase.sh | 11 +--
 1 file changed, 1 insertion(+), 10 deletions(-)

diff --git a/git-rebase.sh b/git-rebase.sh
index 8a3efa29..07e2bd48 100755
--- a/git-rebase.sh
+++ b/git-rebase.sh
@@ -169,22 +169,13 @@ You can run "git stash pop" or "git stash drop" at any 
time.
rm -rf "$state_dir"
 }
 
-run_specific_rebase_internal () {
+run_specific_rebase () {
if [ "$interactive_rebase" = implied ]; then
GIT_EDITOR=:
export GIT_EDITOR
autosquash=
fi
-   # On FreeBSD, the shell's "return" returns from the current
-   # function, not from the current file inclusion.
-   # run_specific_rebase_internal has the file inclusion as a
-   # last statement, so POSIX and FreeBSD's return will do the
-   # same thing.
. git-rebase--$type
-}
-
-run_specific_rebase () {
-   run_specific_rebase_internal
ret=$?
if test $ret -eq 0
then
-- 
tg: (1bb252a9..) t/revert-99855ddf (depends on: t/freebsd-sh-return)
--
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/3] rebase: avoid non-function use of "return" on FreeBSD

[Kyle J. McKay]

Since a1549e10, 15d4bf2e and 01a1e646 (first appearing in v1.8.4) the
git-rebase--*.sh scripts have used a "return" to return from the "dot"
command that runs them.  While this is allowed by POSIX, the FreeBSD
/bin/sh utility behaves poorly under some circumstances when such a
"return" is executed.

In particular, if the "dot" command is contained within a function,
then when a "return" is executed by the script it runs (that is not
itself inside a function), control will return from the function
that contains the "dot" command skipping any statements that might
follow the dot command inside that function.  Commit 99855ddf (first
appearing in v1.8.4.1) addresses this by making the "dot" command
the last line in the function.  Unfortunately the FreeBSD /bin/sh
may also execute some statements in the script run by the "dot"
command that appear after the troublesome "return".  The fix in
99855ddf does not address this problem.

For example, if you have script1.sh with these contents:

#!/bin/sh
# script1.sh
run_script2() {
. "$(dirname -- "$0")/script2.sh"
_e=$?
echo only this line should show
[ $_e -eq 5 ] || echo expected status 5 got $_e
return 3
}
run_script2
e=$?
[ $e -eq 3 ] || { echo expected status 3 got $e; exit 1; }

And script2.sh with these contents:

# script2.sh
if [ 5 -gt 3 ]; then
return 5
fi
case bad in *)
echo always shows
esac
echo should not get here
! :

When running script1.sh (e.g. '/bin/sh script1.sh' or './script1.sh'
after making it executable), the expected output from a POSIX shell
is simply the single line:

only this line should show

However, when run using FreeBSD's /bin/sh, the following output
appears instead:

should not get here
expected status 3 got 1

Not only did the lines following the "dot" command in the run_script2
function in script1.sh get skipped, but additional lines in script2.sh
following the "return" got executed -- but not all of them (e.g. the
"echo always shows" line did not run).

These issues can be avoided by not using a top-level "return" in
script2.sh.  If script2.sh is changed to this:

# script2.sh fixed
main() {
if [ 5 -gt 3 ]; then
return 5
fi
case bad in *)
echo always shows
esac
echo should not get here
! :
}
main

Then it behaves the same when using FreeBSD's /bin/sh as when using
other more POSIX compliant /bin/sh implementations.

We fix the git-rebase--*.sh scripts in a similar fashion by moving
the top-level code that contains "return" statements into its own
function and then calling that as the last line in the script.

The changes introduced by this commit are best viewed with the
--ignore-all-space (-w) diff option.

Signed-off-by: Kyle J. McKay 

---
 git-rebase--am.sh  | 117 
 git-rebase--interactive.sh | 335 +++--
 git-rebase--merge.sh   |  87 ++--
 3 files changed, 274 insertions(+), 265 deletions(-)


For convenience, here are the diffs using -w:

|--- a/git-rebase--am.sh
|+++ b/git-rebase--am.sh
|@@ -4,6 +4,7 @@
 # Copyright (c) 2010 Junio C Hamano.
 #
 
+git_rebase__am() {
case "$action" in
continue)
git am --resolved --resolvemsg="$resolvemsg" &&
|@@ -73,3 +74,5 @@ then
fi
 
move_to_original_branch
+}
+git_rebase__am

|--- a/git-rebase--interactive.sh
|+++ b/git-rebase--interactive.sh
|@@ -810,6 +810,7 @@ add_exec_commands () {
mv "$1.new" "$1"
 }
 
+git_rebase__interactive() {
case "$action" in
continue)
# do we have anything to commit?
|@@ -1042,3 +1043,5 @@ GIT_REFLOG_ACTION="$GIT_REFLOG_ACTION: checkout 
$onto_name"
output git checkout $onto || die_abort "could not detach HEAD"
git update-ref ORIG_HEAD $orig_head
do_rest
+}
+git_rebase__interactive

|--- a/git-rebase--merge.sh
|+++ b/git-rebase--merge.sh
|@@ -101,6 +101,7 @@ finish_rb_merge () {
say All done.
 }
 
+git_rebase__merge() {
case "$action" in
continue)
read_state
|@@ -151,3 +152,5 @@ do
done
 
finish_rb_merge
+}
+git_rebase__merge


diff --git a/git-rebase--am.sh b/git-rebase--am.sh
index a4f683a5..2d3f6d55 100644
--- a/git-rebase--am.sh
+++ b/git-rebase--am.sh
@@ -4,72 +4,75 @@
 # Copyright (c) 2010 Junio C Hamano.
 #
 
-case "$action" in
-continue)
-   git am --resolved --resolvemsg="$resolvemsg" &&
-   move_to_original_branch
-   return
-   ;;
-skip)
-   git am --skip --resolvemsg="$resolvemsg" &&
-   move_to_original_branch
-   return

[PATCH 3/3] test: fix t5560 on FreeBSD

[Kyle J. McKay]

Since fd0a8c2e (first appearing in v1.7.0), the
t/t5560-http-backend-noserver.sh test has used a backslash escape
inside a ${} expansion in order to specify a literal '?' character.

Unfortunately the FreeBSD /bin/sh does not interpret this correctly.

In a POSIX compliant shell, the following:

x='one?two?three'
echo "${x#*\?}"

Would be expected to produce this:

two?three

When using the FreeBSD /bin/sh instead you get this:

one?two?three

In fact the FreeBSD /bin/sh treats the backslash as a literal
character to match so that this:

y='one\two\three'
echo "${y#*\?}"

Produces this unexpected value:

wo\three

In this case the backslash is not only treated literally, it also
fails to defeat the special meaning of the '?' character.

Instead, we can use the [...] construct to defeat the special meaning
of the '?' character and match it exactly in a way that works for the
FreeBSD /bin/sh as well as other POSIX /bin/sh implementations.

Changing the example like so:

x='one?two?three'
echo "${x#*[?]}"

Produces the expected output using the FreeBSD /bin/sh.

Therefore, change the use of \? to [?] in order to be compatible with
the FreeBSD /bin/sh which allows t/t5560-http-backend-noserver.sh to
pass on FreeBSD again.

Signed-off-by: Kyle J. McKay 

---
 t/t5560-http-backend-noserver.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/t/t5560-http-backend-noserver.sh b/t/t5560-http-backend-noserver.sh
index 9be9ae34..5abd11a5 100755
--- a/t/t5560-http-backend-noserver.sh
+++ b/t/t5560-http-backend-noserver.sh
@@ -9,8 +9,8 @@ test_have_prereq GREP_STRIPS_CR && export GREP_OPTIONS=-U
 
 run_backend() {
echo "$2" |
-   QUERY_STRING="${1#*\?}" \
-   PATH_TRANSLATED="$HTTPD_DOCUMENT_ROOT_PATH/${1%%\?*}" \
+   QUERY_STRING="${1#*[?]}" \
+   PATH_TRANSLATED="$HTTPD_DOCUMENT_ROOT_PATH/${1%%[?]*}" \
git http-backend >act.out 2>act.err
 }
 
-- 
tg: (532c2992..) t/freebsd-t5560 (depends on: t/revert-99855ddf)
--
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: fix t7001 cp to use POSIX options

[Kyle J. McKay]

On Apr 11, 2014, at 04:43, Jeff King wrote:
> On Fri, Apr 11, 2014 at 01:24:02AM -0700, Kyle J. McKay wrote:
>
>> Since 11502468 and 04c1ee57 (both first appearing in v1.8.5), the
>> t7001-mv test has used "cp -a" to perform a copy in several of the
>> tests.
>>
>> However, the "-a" option is not required for a POSIX cp utility and
>> some platforms' cp utilities do not support it.
>>
>> The POSIX equivalent of -a is -R -P -p.
>>
>> Change "cp -a" to "cp -R -P -p" so that the t7001-mv test works
>> on systems with a cp utility that only implements the POSIX
>> required set of options and not the "-a" option.
>
> I wonder if the "-R" is the part that we actually care about here.
> Including the others does not hurt in that case, but using only "-R"
> would perhaps make it more obvious to a later reader of the code  
> exactly
> what we are trying to do.

I was wondering the same thing myself, but Jens is on the Cc: list and  
added both of those, so I'm hoping he'll pipe in here about that.  I  
did notice that the other test scripts seem to only use -R, so that  
would definitely be a more consistent change to match the rest of the  
tests.

In any case v2 of the patch with just -R is attached below.  It seems
to pass the tests so it's probably fine.

--Kyle

 8< 

Subject: [PATCH v2] test: fix t7001 cp to use POSIX options

Since 11502468 and 04c1ee57 (both first appearing in v1.8.5), the
t7001-mv test has used "cp -a" to perform a copy in several of the
tests.

However, the "-a" option is not required for a POSIX cp utility and
some platforms' cp utilities do not support it.

The POSIX equivalent of -a is -R -P -p, but the only option we
actually care about for the test is -R.

Change "cp -a" to "cp -R" so that the t7001-mv test works
on systems with a cp utility that only implements the POSIX
required set of options and not the "-a" option.

Signed-off-by: Kyle J. McKay 

---
 t/t7001-mv.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/t/t7001-mv.sh b/t/t7001-mv.sh
index 215d43d6..675ca5bd 100755
--- a/t/t7001-mv.sh
+++ b/t/t7001-mv.sh
@@ -308,7 +308,7 @@ test_expect_success 'git mv moves a submodule with a .git 
directory and no .gitm
(
cd sub &&
rm -f .git &&
-   cp -a ../.git/modules/sub .git &&
+   cp -R ../.git/modules/sub .git &&
GIT_WORK_TREE=. git config --unset core.worktree
) &&
mkdir mod &&
@@ -331,7 +331,7 @@ test_expect_success 'git mv moves a submodule with a .git 
directory and .gitmodu
(
cd sub &&
rm -f .git &&
-   cp -a ../.git/modules/sub .git &&
+   cp -R ../.git/modules/sub .git &&
GIT_WORK_TREE=. git config --unset core.worktree
) &&
mkdir mod &&
-- 
tg: (0bc85abb..) t/t7001-posix-cp (depends on: maint)
--
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/3] rebase: avoid non-function use of "return" on FreeBSD

[Kyle J. McKay]

On Apr 11, 2014, at 01:48, Matthieu Moy wrote:

"Kyle J. McKay"  writes:


If script2.sh is changed to this:

# script2.sh fixed
main() {
   if [ 5 -gt 3 ]; then
   return 5
   fi
   case bad in *)
   echo always shows
   esac
   echo should not get here
   ! :
}
main


Wouldn't it be better to just stop using . within function?

The .-ed script could define the complete function, and then the
function would be used from the toplevel script.

If I understand correctly, your version uses nested functions with  
file
inclusion between both levels of nesting. That might work for the  
shells

you tested, but if the goal is to avoid using tricky features that may
trigger bugs on some shells, that seems backward.


There are already nested functions with file inclusion between both  
levels of nesting in git-rebase--interactive.sh and git-rebase-- 
merge.sh now, so it's not introducing anything new.



IOW, why not move the whole run_specific_rebase_internal function to
git-rebase--$type?


So what change are you proposing exactly?

The current code in maint does this:

git-rebase.sh: top-level
  git-rebase.sh: run_specific_rebase()
git-rebase.sh: run_specific_rebase_internal() -- contains "dot"
  git-rebase--$type.sh: top-level -- has "return"

To make the kind of change I think you're proposing would be somewhat  
more invasive than the proposed patch.  Each of the git-rebase--$type  
scripts would have to be modified not to do anything other than define  
functions when included which would require some code movement to  
avoid having two "main" functions -- either that or there need to be  
multiple "dot" includes in git-rebase.sh so the code not in a function  
only executes when the selected case that uses it is active -- or the  
entire contents of each git-rebase--$type script gets indented and  
becomes a function definition, but that would introduce functions  
defining functions which seems like it would add use of a new tricky  
feature not previously used.


I'm not saying it's a bad idea, it's just somewhat more invasive than  
simply inserting 3 lines.


--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/3] rebase: avoid non-function use of "return" on FreeBSD

[Kyle J. McKay]

On Apr 11, 2014, at 10:30, Matthieu Moy wrote:

"Kyle J. McKay"  writes:


There are already nested functions with file inclusion between both
levels of nesting in git-rebase--interactive.sh and git-rebase--
merge.sh now, so it's not introducing anything new.


OK, so it's less serious than I thought. But still, we're  
introducing a
function with 3 levels of nesting, split accross files, in an area  
where

we know that at least one shell is buggy ...


Currently in maint:

The current code in maint does this:

git-rebase.sh: top-level
  git-rebase.sh: run_specific_rebase()
git-rebase.sh: run_specific_rebase_internal() -- contains "dot"
  git-rebase--interactive.sh: top-level (using --continue or -- 
skip)

git-rebase--interactive.sh: do_rest
  git-rebase--interactive.sh: do_next
git-rebase--interactive.sh: record_in_rewritten
  git-rebase--interactive.sh: flush_rewritten_pending

So I really do not see the additional level of nesting as an issue  
since we've already got much more than 3 levels of nesting going on  
now.  If nesting was going to be a problem, something would have  
broken already.  In fact, since the follow on patch removes the  
run_specific_rebase_internal function what we would have after the  
originally proposed first two patches is:


git-rebase.sh: top-level
  git-rebase.sh: run_specific_rebase() -- contains "dot"
git-rebase--interactive.sh: top-level (using --continue or --skip)
  git-rebase--interactive.sh: git_rebase__interactive
git-rebase--interactive.sh: do_rest
  git-rebase--interactive.sh: do_next
git-rebase--interactive.sh: record_in_rewritten
  git-rebase--interactive.sh: flush_rewritten_pending

Which has exactly the same nesting depth albeit the "dot" has moved up  
one level.



IOW, why not move the whole run_specific_rebase_internal function to
git-rebase--$type?


So what change are you proposing exactly?


Something along the lines of this:



diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 6046778..5dfbf14 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -820,6 +820,7 @@ add_exec_commands () {
mv "$1.new" "$1"
}

+run_specific_rebase_infile() {
case "$action" in
continue)
# do we have anything to commit?
@@ -1055,3 +1056,4 @@ GIT_REFLOG_ACTION="$GIT_REFLOG_ACTION:  
checkout $onto_name"

output git checkout $onto || die_abort "could not detach HEAD"
git update-ref ORIG_HEAD $orig_head
do_rest
+}
diff --git a/git-rebase--merge.sh b/git-rebase--merge.sh
index d84f412..907aa46 100644
--- a/git-rebase--merge.sh
+++ b/git-rebase--merge.sh
@@ -99,6 +99,7 @@ finish_rb_merge () {
say All done.
}

+run_specific_rebase_infile () {
case "$action" in
continue)
read_state
@@ -149,3 +150,4 @@ do
done

finish_rb_merge
+}



The problem with these changes, particularly the git-rebase-- 
interactive.sh one is that a bunch of code is still run when the file  
is "dot" included.  With the changes to git-rebase.sh, that code will  
now run regardless of the action and it will run before it would have  
now.  So if any of the variables it sets affect the functions like  
read_basic_state or finish_rebase (they don't currently appear to),  
then there's a potential for new bugs.  That initial code would not  
previously have run in the --abort case at all.


But, you say the tests pass with those changes, so the changes are  
probably okay.  However, they create a potential situation where some  
code is added to the top of one of the git-rebase--$type.sh scripts  
and suddenly git rebase --abort stops working right because that code  
is being run when it shouldn't or the operation of read_basic_state  
and/or finish_rebase is adversely affected.  Hopefully the rebase  
tests would catch any such issue right away though.


So, in light of the fact that function nesting seems to be a non-issue  
here, and it seems to me the originally proposed changes have much  
less potential to introduce breakage either now or in the future, I  
still prefer them.


--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/3] rebase: avoid non-function use of "return" on FreeBSD

[Kyle J. McKay]

On Apr 12, 2014, at 10:07, Matthieu Moy wrote:
> "Kyle J. McKay"  writes:
>
>> On Apr 11, 2014, at 10:30, Matthieu Moy wrote:
>>> "Kyle J. McKay"  writes:
>>>
>>>> There are already nested functions with file inclusion between both
>>>> levels of nesting in git-rebase--interactive.sh and git-rebase--
>>>> merge.sh now, so it's not introducing anything new.
>>>
>>> OK, so it's less serious than I thought. But still, we're
>>> introducing a
>>> function with 3 levels of nesting, split accross files, in an area
>>> where
>>> we know that at least one shell is buggy ...
>>
>> Currently in maint:
>>
>> The current code in maint does this:
>>
>> git-rebase.sh: top-level
>>  git-rebase.sh: run_specific_rebase()
>>git-rebase.sh: run_specific_rebase_internal() -- contains "dot"
>>  git-rebase--interactive.sh: top-level (using --continue or --
>> skip)
>>git-rebase--interactive.sh: do_rest
>>  git-rebase--interactive.sh: do_next
>
> You're confusing function calls and function nesting. do_rest calls
> do_next, but the definition of do_next is not nested within do_rest.
>
> When I talk about nested function, I mean
>
> f() {
>   g() {
>   ...
>   }
> }
>
> Obviously, having functions call each other is not an issue. That's  
> what
> functions are meant to be.
>
> Now, having run_specific_rebase_internal include a file which defines
> functions which contain nested functions _is_ something I find  
> weird. It
> both stresses the shell in a buggy area and makes the code harder to
> understand.

I meant: "nested functions" = "nested function calls"
You meant: "nested functions" = "nested function definitions"

Okay.  But nested function definitions is not something new to the
rebase code.

>> The problem with these changes, particularly the git-rebase--
>> interactive.sh one is that a bunch of code is still run when the file
>> is "dot" included.
>
> Function definitions, and variables assignments. Is it so much of an
> issue?
>
> What's the difference between a function definition or variable
> assignment within git-rebase--*.sh and within git-rebase.sh?

As I said, this is the issue:

On Apr 11, 2014, at 16:08, Kyle J. McKay wrote:

> The problem with these changes, particularly the git-rebase-- 
> interactive.sh one is that a bunch of code is still run when the  
> file is "dot" included.  With the changes to git-rebase.sh, that  
> code will now run regardless of the action and it will run before it  
> would have now.  So if any of the variables it sets affect the  
> functions like read_basic_state or finish_rebase (they don't  
> currently appear to), then there's a potential for new bugs.  That  
> initial code would not previously have run in the --abort case at all.

Let me rephrase.

Patch 1/3 does not change the order in which individual statements are  
executed in the rebase code.  Nor does it change the logic.  It simply  
introduces one additional function callstack level, but the same  
individual statements are executed in the same order for all control  
flows.  No additional statements other than the introduced callstack  
level.  Nothing's executed in a different order.  No control paths  
execute additional statements they did not before.

The changes you propose introduce exactly the same additional function  
callstack level.  Then they proceed to alter the order in which  
statements are executed.  Statements that did not execute in some  
control paths before are now executed in those control paths.  Other  
statements are moved around to execute earlier/later than they did  
before.

My point is not that this is wrong.  It's nice, really, to move the  
"dot" include to the top level.  The point is that's not necessary to  
fix the problem and moving statements around and adding statements to  
some control paths increases the risk of introducing an uncaught bug  
when it's not necessary to fix the problem.

I would like to get a fix in so that rebase works out-of-the-box when  
Git version 1.8.4 or later is built on FreeBSD.

So I'm not going to belabor the point anymore.

There's a follow-up patch 4/3 attached to the end of this message that  
makes the changes you have proposed in your earlier email on top of  
patches 1/3 and 2/3.  The log message and all changes are taken from  
your emails and so this patch is assigned to you and has a
'Needs-Signed-off-by:' line.

On Apr 11, 2014, at 10:30, Matthieu Moy wrote:
> The real patch is a bit more tricky thou

Re: [PATCH 1/3] rebase: avoid non-function use of "return" on FreeBSD

[Kyle J. McKay]

On Apr 14, 2014, at 15:51, Junio C Hamano wrote:

I think we would want to see the actual change formatted this way
(without needing to pass "-w" to "git show"), as it will make it
clear that this artificial extra level of "define the whole thing
inside a function and then make a single call to it" is a workaround
of specific shell's glitch that we would not have to have in an
ideal world ;-)

Besides that would make it less likely to cause conflicts with the
real changes in flight.


Sounds good to me.


Please double check what I queued on 'pu' when I push out today's
integration result.



diff --git a/git-rebase--am.sh b/git-rebase--am.sh
index a4f683a5..2ab514ea 100644
--- a/git-rebase--am.sh
+++ b/git-rebase--am.sh
@@ -4,6 +4,13 @@
 # Copyright (c) 2010 Junio C Hamano.
 #

+# The whole contents of the file is run by dot-sourcing this file  
from

+# inside a shell function, and "return"s we see below are expected to
+# return from that function that dot-sources us.  However, FreeBSD
+# /bin/sh misbehaves on such a construct, so we will work it around  
by

+# enclosing the whole thing inside an extra layer of a function.
+git_rebase__am () {


I think the wording may be just a bit off:


and "return"s we see below are expected to return from that function
that dot-sources us.


I thought that was one of the buggy behaviors we are working around?

Maybe I'm just reading it wrong...

Does this wording seem any clearer?

and "return"s we see below are expected not to return
from the function that dot-sources us, but rather to
the next command after the one in the function that
dot-sources us.

Otherwise the patch in pu looks fine (all t34*.sh tests pass for me on  
FreeBSD with pu at 8d8dc6db).


Thank you for adding the comments.

If I'm the only one getting a wrong meaning from the comments, then no  
reason to change them.


--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/3] rebase: avoid non-function use of "return" on FreeBSD

[Kyle J. McKay]

On Apr 16, 2014, at 11:11, Junio C Hamano wrote:

Junio C Hamano  writes:


"Kyle J. McKay"  writes:

If I'm the only one getting a wrong meaning from the comments,  
then no

reason to change them.


I agree that the description does not read well with the work-around
already there.  I am not sure what would be a better way to phrase
it, though.


Here is a tentative rewrite at the beginning and the end of rebase-am:

   # The whole contents of the file is run by dot-sourcing this
   # file from inside a shell function.  It used to be that
   # "return"s we see below were not inside any function, and
   # expected to return from the function that dot-sourced us.


I think it's the "return from the function that dot-sourced us" that  
gives me the wrong meaning.  The "return from" part says to me that  
function will be returning which it will not unless the dot command  
was the last command in the function.


Either "return to the function that dot-sourced us" or "return from  
the dot command that dot-sourced us", but using the original wording  
implies to me that the function that dot-sourced us will return as  
soon as the dot-sourced script executes the return and that is exactly  
one of the bugs we're working around.


I think just the s/from/to/ would fix it so it does not give me the  
wrong impression, but that doesn't mean that would not confuse  
everyone else.  ;)



   #
   # However, FreeBSD /bin/sh misbehaves on such a construct and
   # continues to run the statements that follow such a "return".
   # As a work-around, we introduce an extra layer of a function
   # here, and immediately call it after defining it.
   git_rebase__am () {

   ...

   }
   # ... and then we call the whole thing.
   git_rebase__am


On Apr 16, 2014, at 11:23, Junio C Hamano wrote:

Junio C Hamano  writes:
By the way, you have this in your log message:


   ... the git-rebase--*.sh scripts have used a "return" to return
   from the "dot" command that runs them.  While this is allowed by
   POSIX,...


Is it "this is allowed", or is it "this should be the way and shells
that do not do so are buggy"?


Answering myself...

The only "unspecified" I see is this:

   If the shell is not currently executing a function or dot
   script, the results are unspecified.

which clearly does not apply to the version before this patch (we
are executing a dot script).  And

   The return utility shall cause the shell to stop executing the
   current function or dot script.

would mean that we are correct to expect that "should not get here"
is not reached, as the "return 5" would cause the shell to stop
executing the dot script there.

So "while this is allowed by POSIX" may be a bit misleading and
needs to be reworded, I guess?


"allowed by POSIX" is not incorrect. ;)

But perhaps the log message should say 'While POSIX specifies that a  
"return" may be used to exit a "dot" sourced script,' there instead to  
be clearer.  Which would make that whole first paragraph become:


Since a1549e10, 15d4bf2e and 01a1e646 (first appearing in v1.8.4)  
the
git-rebase--*.sh scripts have used a "return" to return from the  
"dot"
command that runs them.  While POSIX specifies that a "return"  
may be

used to exit a "dot" sourced script, the FreeBSD /bin/sh utility
behaves poorly under some circumstances when such a "return" is  
executed.


--
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/3] rebase: avoid non-function use of "return" on FreeBSD

[Kyle J. McKay]

On Apr 17, 2014, at 10:15, Junio C Hamano wrote:


I think just the s/from/to/ would fix it so it does not give me the
wrong impression, but that doesn't mean that would not confuse
everyone else.  ;)


Yeah, let's do that.  Thanks for carefully reading them.



I'd think it makes it clearer to say that we take "return stopping
the dot-sourced file" as a given and FreeBSD does not behave that
way.

-- > 8 --
From: "Kyle J. McKay" 
Date: Fri, 11 Apr 2014 01:28:17 -0700
Subject: [PATCH] rebase: avoid non-function use of "return" on FreeBSD

[...]

The new version of the patch looks great, let's use that.  Thanks for  
adjusting it.


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

Re: Bug/request: the empty string should be a valid git note

[Kyle J. McKay]

On Sep 20, 2014, at 18:44, Johan Herland wrote:


At least, we should fix

   git notes add -C e69de29bb2d1d6434b8b29ae775ad8c2e48c5391

Whether we should also change

   git notes add -m ''

to create an empty note, or leave it as-is, (i.e. similar in spirit to
"git commit -m ''"), I'll leave up to further discussion.


The help for git commit has this:

  --allow-empty-message
Like --allow-empty this command is primarily for use by
foreign SCM interface scripts. It allows you to create
a commit with an empty commit message without using
plumbing commands like git-commit-tree(1).

Why not add the same/similar option to git notes add?

So this:

  git notes add --allow-empty-message -m ''

creates an empty note.  (Perhaps --allow-empty-note should
be an alias?)

With your patch to allow -C e69de29bb2d1d6434b8b29ae775ad8c2e48c5391
there's already support for it, it just needs the option
parsing added.  :)

--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: [RFC] Git.pm: stat the FD to ensure the file is really open

[Kyle J. McKay]

On Oct 30, 2014, at 15:08, Eric Wong wrote:

For a (currently) unknown reason, Git::SVN::Fetcher::close_file
sometimes triggers "Bad file descriptor" errors on syswrite when
writing symlink contents on the "svn_hash" tempfile.

The current IO::Handle::opened call in Git.pm is not a
sufficient check, as the underlying file descriptor is closed
without the PerlIO layer knowing about it.  This is likely a bug
inside libsvn (1.6.17), as none of the Git.pm or Git::SVN
modules close IOs without the knowledge of the PerlIO layer.

Cc: Kyle J. McKay 
Signed-off-by: Eric Wong 
---
Kyle/Junio: thoughts?  I'm running out of time to track this down
so it might be necessary for 2.2-rc0.  What's even stranger is
I cannot always reproduce the problem even without this patch,
so it may be only triggered by network latency...
Thanks.


With this patch added, do you then see the warning:

  carp "Temp file '", $name,
"' was closed. Opening replacement.";

From _temp_cache?  It would seem odd if you didn't unless there was  
only one symlink involved.


I suspect that symlinks were rare in the repositories I was testing  
against.  I wonder if having a test svn repo that adds several new  
symlinks for several revisions in a row might trigger this problem  
more robustly.


The _repository->temp_acquire('svn_hash') call is assigned to a "my"  
variable and then has Git::temp_release called on it later in the same  
function and the only calls made on it in between are Git::temp_path,  
so I don't see how the libsvn library could be responsible for closing  
it since it looks to me like it never sees it.  But I'm looking at  
v2.1.2 of Fetcher.pm, so if some other calls have been inserted there  
since then.



perl/Git.pm | 18 ++
1 file changed, 14 insertions(+), 4 deletions(-)

diff --git a/perl/Git.pm b/perl/Git.pm
index b5905ee..f1f210b 100644
--- a/perl/Git.pm
+++ b/perl/Git.pm
@@ -99,7 +99,7 @@ increase notwithstanding).
=cut


-use Carp qw(carp croak); # but croak is bad - throw instead
+use Carp qw(carp croak cluck); # but croak is bad - throw instead
use Error qw(:try);
use Cwd qw(abs_path cwd);
use IPC::Open2 qw(open2);
@@ -1206,6 +1206,16 @@ sub temp_acquire {
$temp_fd;
}

+sub _temp_really_open {
+   my ($fh) = @_;
+
+   if (defined($fh) && $fh->opened) {
+   return 1 if stat($fh);
+   cluck("$fh closed independently of PerlIO\n");
+   }
+   return 0;
+}
+
=item temp_is_locked ( NAME )

Returns true if the internal lock created by a previous  
C

@@ -1232,7 +1242,7 @@ sub temp_is_locked {
my ($self, $name) = _maybe_self(@_);
my $temp_fd = \$TEMP_FILEMAP{$name};

-	defined $$temp_fd && $$temp_fd->opened && $TEMP_FILES{$$temp_fd} 
{locked};

+   _temp_really_open($$temp_fd) && $TEMP_FILES{$$temp_fd}{locked};
}

=item temp_release ( NAME )
@@ -1264,7 +1274,7 @@ sub temp_release {
carp "Attempt to release temp file '",
$temp_fd, "' that has not been locked";
}
-   temp_reset($temp_fd) if $trunc and $temp_fd->opened;
+   temp_reset($temp_fd) if $trunc && _temp_really_open($temp_fd);

$TEMP_FILES{$temp_fd}{locked} = 0;
undef;
@@ -1276,7 +1286,7 @@ sub _temp_cache {
_verify_require();

my $temp_fd = \$TEMP_FILEMAP{$name};
-   if (defined $$temp_fd and $$temp_fd->opened) {
+   if (_temp_really_open($$temp_fd)) {
if ($TEMP_FILES{$$temp_fd}{locked}) {
throw Error::Simple("Temp file with moniker '" .
$name . "' already in use");
--
EW


The changes themselves look okay, but eee.  I don't see how libsvn  
can see the svn_hash fd to close it unless it's randomly closing fds.


--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: [ANNOUNCE] Git v2.0.0-rc0

[Kyle J. McKay]

On Apr 18, 2014, at 12:37, Junio C Hamano wrote:
> An early preview release Git v2.0.0-rc0 is now available for
> testing at the usual places.

I have run into the following test failures with v2.0.0-rc0:

Test Summary Report
---
t9117-git-svn-init-clone.sh  (Wstat: 256 Tests: 11 Failed: 2)
   Failed tests:  10-11
   Non-zero exit status: 1
Files=658, Tests=11878, 3684 wallclock secs
Result: FAIL

The cause of the failure in both tests is this:

  $ git svn init -s "$svnrepo"/project project --prefix=""
  Option prefix requires an argument

The problem with --prefix="" is this (from the Getopt::Long CHANGES file):

Changes in version 2.37
---

 * Bugfix: With gnu_compat, --foo= will no longer trigger "Option
   requires an argument" but return the empty string.

The system I ran the tests on happens to have Getopt::Long version 2.35.

The --prefix="" option can be rewritten --prefix "" in both tests and then  
they pass.

Alternatively this change can be made in git-svn.perl:

|diff --git a/git-svn.perl b/git-svn.perl
|index 7349ffea..284f458a 100755
|--- a/git-svn.perl
|+++ b/git-svn.perl
|@@ -149,7 +149,7 @@ my ($_trunk, @_tags, @_branches, $_stdlayout);
  my %icv;
  my %init_opts = ( 'template=s' => \$_template, 'shared:s' => \$_shared,
'trunk|T=s' => \$_trunk, 'tags|t=s@' => \@_tags,
-  'branches|b=s@' => \@_branches, 'prefix=s' => \$_prefix,
+  'branches|b=s@' => \@_branches, 'prefix:s' => \$_prefix,
'stdlayout|s' => \$_stdlayout,
'minimize-url|m!' => \$Git::SVN::_minimize_url,
   'no-metadata' => sub { $icv{noMetadata} = 1 },

Which makes the argument to --prefix optional (in which case it is set  
to "").

I'm not really sure what the best thing to do here is.  I thought the  
documentation talked somewhere about using --prefix="" (or just --prefix=)
to get the old behavior, but now I can't find that.  In that  
case I think probably just changing the tests to use --prefix ""  
instead of --prefix="" should take care of it especially since the log  
message for fe191fca (which actually changes the default) talks about  
using --prefix "" and not --prefix="".  I have attached a patch below  
(against master) to make that change to the two affected subtests of t9117.

--Kyle

 >8 
Subject: [PATCH] t9117: use --prefix "" instead of --prefix=""

Versions of Perl's Getopt::Long module before 2.37 do not contain
this fix that first appeared in Getopt::Long version 2.37:

* Bugfix: With gnu_compat, --foo= will no longer trigger "Option
  requires an argument" but return the empty string.

Instead of using --prefix="" use --prefix "" when testing an
explictly empty prefix string in order to work with older versions
of Perl's Getopt::Long module.

Signed-off-by: Kyle J. McKay 
---
 t/t9117-git-svn-init-clone.sh | 8 
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/t/t9117-git-svn-init-clone.sh b/t/t9117-git-svn-init-clone.sh
index dfed76fe..a66f43c6 100755
--- a/t/t9117-git-svn-init-clone.sh
+++ b/t/t9117-git-svn-init-clone.sh
@@ -101,18 +101,18 @@ test_expect_success 'clone with -s/-T/-b/-t assumes 
--prefix=origin/' '
rm -f warning
'
 
-test_expect_success 'init with -s/-T/-b/-t and --prefix="" still works' '
+test_expect_success 'init with -s/-T/-b/-t and --prefix "" still works' '
test ! -d project &&
-   git svn init -s "$svnrepo"/project project --prefix="" 2>warning &&
+   git svn init -s "$svnrepo"/project project --prefix "" 2>warning &&
test_must_fail grep -q prefix warning &&
test_svn_configured_prefix "" &&
rm -rf project &&
rm -f warning
'
 
-test_expect_success 'clone with -s/-T/-b/-t and --prefix="" still works' '
+test_expect_success 'clone with -s/-T/-b/-t and --prefix "" still works' '
test ! -d project &&
-   git svn clone -s "$svnrepo"/project --prefix="" 2>warning &&
+   git svn clone -s "$svnrepo"/project --prefix "" 2>warning &&
test_must_fail grep -q prefix warning &&
test_svn_configured_prefix "" &&
rm -rf project &&
-- 
1.8.5

--
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] pack-bitmap: do not core dump

[Kyle J. McKay]

So I was trying to use pack.writebitmaps=true and all I got was core dumps.

The fix with a real subject line ;) is below.  I think perhaps this should be
picked up for the 2.0.0 release.  (Patch is against master.)

--Kyle

 >8 
Subject: [PATCH] ewah_bitmap.c: do not assume size_t and eword_t are the same 
size

When buffer_grow changes the size of the buffer using realloc,
it first computes and saves the rlw pointer's offset into the
buffer using (uint8_t *) math before the realloc but then
restores it using (eword_t *) math.

In order to do this it's necessary to convert the (uint8_t *)
offset into an (eword_t *) offset.  It was doing this by
dividing by the sizeof(size_t).  Unfortunately sizeof(size_t)
is not same as sizeof(eword_t) on all platforms.

This causes illegal memory accesses and other bad things to
happen when attempting to use bitmaps on those platforms.

Fix this by dividing by the sizeof(eword_t) instead which
will always be correct for all platforms.

Signed-off-by: Kyle J. McKay 
---
 ewah/ewah_bitmap.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/ewah/ewah_bitmap.c b/ewah/ewah_bitmap.c
index 9ced2dad..fccb42b5 100644
--- a/ewah/ewah_bitmap.c
+++ b/ewah/ewah_bitmap.c
@@ -41,7 +41,7 @@ static inline void buffer_grow(struct ewah_bitmap *self, 
size_t new_size)
self->alloc_size = new_size;
self->buffer = ewah_realloc(self->buffer,
self->alloc_size * sizeof(eword_t));
-   self->rlw = self->buffer + (rlw_offset / sizeof(size_t));
+   self->rlw = self->buffer + (rlw_offset / sizeof(eword_t));
 }
 
 static inline void buffer_push(struct ewah_bitmap *self, eword_t value)
-- 
1.8.5

--
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] pack-bitmap: do not core dump

[Kyle J. McKay]

On Apr 22, 2014, at 16:17, Jeff King wrote:

but I do not think that is necessarily any more readable, especially
because we probably need to cast it like:

 self->rlw = (eword_t *)((uint8_t *)self->buffer + rlw_offset);


I suspect that will produce a warning about a cast increasing pointer  
alignment in some cases.



So why do any uint8_t math in the first place?


Just guessing it started out as uint8_t math throughout then the  
warning about increasing alignment showed up so that part got changed  
but the save pointer offset part did not.



I think we could write it as:

eword_t *old = self->buffer;
... realloc ...
self->rlw = self->buffer + (self->rlw - old);


Yeah that seems good too.


I'm fine with your patch, though.



thanks.

I tend to gravitate towards the most minimal change that fixes the  
issue when touching someone else's code especially if I'm not familiar  
with it.  There's also the minor issue of optimizing out the pointer  
arithmetic implicit divide by sizeof(eword_t) for the subtract and the  
implicit multiply by sizeof(eword_t) for the add -- I didn't check to  
see if one of the alternatives is best -- the one you mention above  
with the cast (keeping the uint8_t math) is probably guaranteed not to  
have any implicit multiply, but there's that pesky warning to get rid  
of.  Of course those multiplies and divides should just end up being  
shifts so not a big deal if they didn't get optimized out (the realloc  
time should dwarf them into irrelevancy 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 2/9] strbuf: add strbuf_tolower function

[Kyle J. McKay]

On May 21, 2014, at 03:27, Jeff King wrote:


This makes config's lowercase() function public.

Note that we could continue to offer a pure-string
lowercase, but there would be no callers (in most
pure-string cases, we actually duplicate and lowercase the
duplicate).

Signed-off-by: Jeff King 
---
This ones gets used later on...

Documentation/technical/api-strbuf.txt | 4 
config.c   | 8 +---
strbuf.c   | 7 +++
strbuf.h   | 1 +
4 files changed, 13 insertions(+), 7 deletions(-)

diff --git a/Documentation/technical/api-strbuf.txt b/Documentation/ 
technical/api-strbuf.txt

index 3350d97..8480f89 100644
--- a/Documentation/technical/api-strbuf.txt
+++ b/Documentation/technical/api-strbuf.txt
@@ -125,6 +125,10 @@ Functions

Strip whitespace from the end of a string.

+`strbuf_tolower`::
+
+   Lowercase each character in the buffer using `tolower`.
+
`strbuf_cmp`::

	Compare two buffers. Returns an integer less than, equal to, or  
greater

diff --git a/config.c b/config.c
index a30cb5c..03ce5c6 100644
--- a/config.c
+++ b/config.c
@@ -147,12 +147,6 @@ int git_config_include(const char *var, const  
char *value, void *data)

return ret;
}

-static void lowercase(char *p)
-{
-   for (; *p; p++)
-   *p = tolower(*p);
-}
-
void git_config_push_parameter(const char *text)
{
struct strbuf env = STRBUF_INIT;
@@ -180,7 +174,7 @@ int git_config_parse_parameter(const char *text,
strbuf_list_free(pair);
return error("bogus config parameter: %s", text);
}
-   lowercase(pair[0]->buf);
+   strbuf_tolower(pair[0]);
if (fn(pair[0]->buf, pair[1] ? pair[1]->buf : NULL, data) < 0) {
strbuf_list_free(pair);
return -1;
diff --git a/strbuf.c b/strbuf.c
index ee96dcf..a1b8a47 100644
--- a/strbuf.c
+++ b/strbuf.c
@@ -106,6 +106,13 @@ void strbuf_ltrim(struct strbuf *sb)
sb->buf[sb->len] = '\0';
}

+void strbuf_tolower(struct strbuf *sb)
+{
+   size_t i;
+   for (i = 0; i < sb->len; i++)
+   sb->buf[i] = tolower(sb->buf[i]);
+}
+


Wouldn't a direct transfer of the lowercase function be something more  
like:



void strbuf_tolower(struct strbuf *sb)
{
char *p = sb->buf;
for (; *p; p++)
*p = tolower(*p);
}

That seems to me to be a bit more efficient.  According to the  
comments in strbuf.c, "people can always assume buf is non NULL and - 
>buf is NUL terminated even for a freshly initialized strbuf."



struct strbuf **strbuf_split_buf(const char *str, size_t slen,
 int terminator, int max)
{
diff --git a/strbuf.h b/strbuf.h
index 39c14cf..6b6f745 100644
--- a/strbuf.h
+++ b/strbuf.h
@@ -45,6 +45,7 @@ static inline void strbuf_setlen(struct strbuf  
*sb, size_t len)

extern void strbuf_trim(struct strbuf *);
extern void strbuf_rtrim(struct strbuf *);
extern void strbuf_ltrim(struct strbuf *);
+extern void strbuf_tolower(struct strbuf *sb);
extern int strbuf_cmp(const struct strbuf *, const struct strbuf *);

/*
--
2.0.0.rc1.436.g03cb729



--
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 7/9] remote-curl: recognize text/plain with a charset parameter

[Kyle J. McKay]

On May 21, 2014, at 03:33, Jeff King wrote:


Commit 426e70d (remote-curl: show server content on http
errors, 2013-04-05) tried to recognize text/plain content
types, but fails to do so if they have any parameters.

This patches makes our parsing more liberal, though we still
do not do anything useful with a charset parameter.

Signed-off-by: Jeff King 
---
remote-curl.c  | 26 ++
t/lib-httpd/error.sh   |  8 +++-
t/t5550-http-fetch-dumb.sh |  5 +
3 files changed, 30 insertions(+), 9 deletions(-)

diff --git a/remote-curl.c b/remote-curl.c
index a5ab977..6d1b206 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -194,20 +194,30 @@ static void free_discovery(struct discovery *d)
}
}

+/*
+ * We only show text/plain parts, as other types are likely
+ * to be ugly to look at on the user's terminal.
+ */
+static int content_type_is_terminal_friendly(struct strbuf *type)
+{
+   const char *p;
+
+   p = skip_prefix(type->buf, "text/plain");
+   if (!p || (*p && *p != ';'))
+   return 0;
+
+   return 1;
+}
+


I think that a strict reading of RFC 2616 allows "text/plain ;  
charset=utf-8" as well as "text/plain;charset=utf-8" and "text/plain;  
charset=utf-8".  So perhaps this if line instead:


+   if (!p || (*p && *p != ';' && *p != ' ' && *p != '\t'))

See RFC 2616 section 2.2 for the definition of linear white space  
(LWS) and the end of section 2.1 for the "implied *LWS" rule that  
allows it.



static int show_http_message(struct strbuf *type, struct strbuf *msg)
{
const char *p, *eol;

-   /*
-* We only show text/plain parts, as other types are likely
-* to be ugly to look at on the user's terminal.
-*
-* TODO should handle "; charset=XXX", and re-encode into
-* logoutputencoding
-*/
-   if (strcmp(type->buf, "text/plain"))
+   if (!content_type_is_terminal_friendly(type))
return -1;

+	/* TODO should record charset and reencode msg to  
logOutputEncoding */

+
strbuf_trim(msg);
if (!msg->len)
return -1;
diff --git a/t/lib-httpd/error.sh b/t/lib-httpd/error.sh
index 786f281..02e80b3 100755
--- a/t/lib-httpd/error.sh
+++ b/t/lib-httpd/error.sh
@@ -3,6 +3,7 @@
printf "Status: 500 Intentional Breakage\n"

printf "Content-Type: "
+charset=iso8859-1
case "$PATH_INFO" in
*html*)
printf "text/html"
@@ -10,8 +11,13 @@ case "$PATH_INFO" in
*text*)
printf "text/plain"
;;
+*charset*)
+   printf "text/plain; charset=utf-8"
+   charset=utf-8
+   ;;
esac
printf "\n"

printf "\n"
-printf "this is the error message\n"
+printf "this is the error message\n" |
+iconv -f us-ascii -t $charset
diff --git a/t/t5550-http-fetch-dumb.sh b/t/t5550-http-fetch-dumb.sh
index 13defd3..b35b261 100755
--- a/t/t5550-http-fetch-dumb.sh
+++ b/t/t5550-http-fetch-dumb.sh
@@ -181,5 +181,10 @@ test_expect_success 'git client does not show  
html errors' '

! grep "this is the error message" stderr
'

+test_expect_success 'git client shows text/plain with a charset' '
+   test_must_fail git clone "$HTTPD_URL/error/charset" 2>stderr &&
+   grep "this is the error message" stderr
+'
+
stop_httpd
test_done
--
2.0.0.rc1.436.g03cb729



--
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 9/9] remote-curl: reencode http error messages

[Kyle J. McKay]

On May 21, 2014, at 03:33, Jeff King wrote:


As of the last commit, we now recognize an error message
with a content-type "text/plain; charset=utf-16" as text,
but we ignore the charset parameter entirely. Let's encode
it to log_output_encoding, which is presumably something the
user's terminal can handle.

Signed-off-by: Jeff King 
---
remote-curl.c  | 37 +
t/lib-httpd/error.sh   |  4 
t/t5550-http-fetch-dumb.sh |  5 +
3 files changed, 42 insertions(+), 4 deletions(-)

diff --git a/remote-curl.c b/remote-curl.c
index 6d1b206..1dc90d7 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -194,11 +194,34 @@ static void free_discovery(struct discovery *d)
}
}

+static char *find_param(const char *str, const char *name)
+{
+   int len = strlen(name);
+
+   for (; *str; str++) {
+   const char *p;
+
+   if (*p++ != ' ')
+   continue;
+
+   if (strncmp(p, name, len))
+   continue;
+   p += len;
+
+   if (*p++ != '=')
+   continue;
+
+   return xstrndup(p, strchrnul(p, ' ') - p);
+   }
+
+   return NULL;
+}
+
/*
 * We only show text/plain parts, as other types are likely
 * to be ugly to look at on the user's terminal.
 */
-static int content_type_is_terminal_friendly(struct strbuf *type)
+static int content_type_is_terminal_friendly(struct strbuf *type,  
char **charset)

{
const char *p;

@@ -206,17 +229,23 @@ static int  
content_type_is_terminal_friendly(struct strbuf *type)

if (!p || (*p && *p != ';'))
return 0;

+   *charset = find_param(p, "charset");
+   /* default charset from rfc2616 */
+   if (!*charset)
+   *charset = xstrdup("iso8859-1");


Actually the name should be "ISO-8859-1".  See RFC 2616 section  
3.7.1.  Since it's case insensitive "iso-8859-1" would be fine too.



+
return 1;
}

static int show_http_message(struct strbuf *type, struct strbuf *msg)
{
const char *p, *eol;
+   char *charset;

-   if (!content_type_is_terminal_friendly(type))
+   if (!content_type_is_terminal_friendly(type, &charset))
return -1;
-
-	/* TODO should record charset and reencode msg to  
logOutputEncoding */

+   strbuf_reencode(msg, charset, get_log_output_encoding());
+   free(charset);

strbuf_trim(msg);
if (!msg->len)
diff --git a/t/lib-httpd/error.sh b/t/lib-httpd/error.sh
index 02e80b3..4efbce7 100755
--- a/t/lib-httpd/error.sh
+++ b/t/lib-httpd/error.sh
@@ -15,6 +15,10 @@ case "$PATH_INFO" in
printf "text/plain; charset=utf-8"
charset=utf-8
;;
+*utf16*)
+   printf "text/plain; charset=utf-16"
+   charset=utf-16
+   ;;
esac
printf "\n"

diff --git a/t/t5550-http-fetch-dumb.sh b/t/t5550-http-fetch-dumb.sh
index b35b261..01b8aae 100755
--- a/t/t5550-http-fetch-dumb.sh
+++ b/t/t5550-http-fetch-dumb.sh
@@ -186,5 +186,10 @@ test_expect_success 'git client shows text/ 
plain with a charset' '

grep "this is the error message" stderr
'

+test_expect_success 'http error messages are reencoded' '
+   test_must_fail git clone "$HTTPD_URL/error/utf16" 2>stderr &&
+   grep "this is the error message" stderr
+'
+
stop_httpd
test_done
--
2.0.0.rc1.436.g03cb729


--
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 7/9] remote-curl: recognize text/plain with a charset parameter

[Kyle J. McKay]

On May 21, 2014, at 23:05, Jeff King wrote:


On Wed, May 21, 2014 at 05:07:38PM -0700, Kyle J. McKay wrote:


+   p = skip_prefix(type->buf, "text/plain");
+   if (!p || (*p && *p != ';'))
+   return 0;
+
+   return 1;
+}
+


I think that a strict reading of RFC 2616 allows "text/plain ;
charset=utf-8" as well as "text/plain;charset=utf-8" and "text/plain;
charset=utf-8".  So perhaps this if line instead:

+   if (!p || (*p && *p != ';' && *p != ' ' && *p != '\t'))

See RFC 2616 section 2.2 for the definition of linear white space  
(LWS) and

the end of section 2.1 for the "implied *LWS" rule that allows it.


You're right. There are a few exceptions in 3.7:

 Linear white space (LWS) MUST NOT be used between the type and
 subtype, nor between an attribute and its value.

but it does not include between the subtype and parameter. So the
"find_parameter" also needs to accept the collapsed whitespace, too, I
guess.


Yeah I think so too.  It's probably enough though just to just strip  
all " " and "\t" characters at the same time the content type is  
lowercased.  While that would cause invalid content types such as  
"text / plain" to be recognized it would keep the rest of the code  
simpler.  Since a producer of an invalid content type shouldn't really  
be depending on any particular behavior by a receiver of an invalid  
content type it's probably not an issue.


--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/9] strbuf: add strbuf_tolower function

[Kyle J. McKay]

On May 22, 2014, at 11:41, Jeff King wrote:


On Thu, May 22, 2014 at 11:36:37AM -0700, Junio C Hamano wrote:

Yes, and that would be fine with me (I actually wrote  
strbuf_tolower for
my own use, and _then_ realized that we already had such a thing  
that

could be replaced).


Do we forbid that sb->buf[x] for some x < sb->len to be NUL, and if
there is such a byte we stop running tolower() on the remainder?


Christian brought this up elsewhere, and I agree it's probably  
better to
work over the whole buffer, NULs included. I'm happy to re-roll (or  
you

can just pick up the version of the patch in this thread),


The only reason I brought up the code difference in the first place  
was that the comment was "This makes config's lowercase() function  
public" which made me expect to see basically the equivalent of  
replacing a "static" with an "extern", but actually the function being  
made public was a different implementation than config's lowercase()  
function.  So it looks like the original PATCH 2/9 version of the code  
is best, but perhaps the comment can be tweaked a bit to not convey  
the same impression I got.  Maybe something like "Replace config's  
lowercase() function with a public one".


--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 4/8] http: extract type/subtype portion of content-type

[Kyle J. McKay]

On May 22, 2014, at 02:29, Jeff King wrote:


When we get a content-type from curl, we get the whole
header line, including any parameters, and without any
normalization (like downcasing or whitespace) applied.
If we later try to match it with strcmp() or even
strcasecmp(), we may get false negatives.

This could cause two visible behaviors:

 1. We might fail to recognize a smart-http server by its
content-type.

 2. We might fail to relay text/plain error messages to
users (especially if they contain a charset parameter).

This patch teaches the http code to extract and normalize
just the type/subtype portion of the string. This is
technically passing out less information to the callers, who
can no longer see the parameters. But none of the current
callers cares, and a future patch will add back an
easier-to-use method for accessing those parameters.

Signed-off-by: Jeff King 
---
http.c | 32 +---
remote-curl.c  |  2 +-
t/lib-httpd/error.sh   |  8 +++-
t/t5550-http-fetch-dumb.sh |  5 +
4 files changed, 42 insertions(+), 5 deletions(-)

diff --git a/http.c b/http.c
index 94e1afd..4edf5b9 100644
--- a/http.c
+++ b/http.c
@@ -906,6 +906,29 @@ static CURLcode curlinfo_strbuf(CURL *curl,  
CURLINFO info, struct strbuf *buf)

return ret;
}

+/*
+ * Extract a normalized version of the content type, with any
+ * spaces suppressed, all letters lowercased, and no trailing ";"
+ * or parameters.
+ *
+ * Example:
+ *   "TEXT/PLAIN; charset=utf-8" -> "text/plain"
+ */
+static void extract_content_type(struct strbuf *raw, struct strbuf  
*type)

+{
+   const char *p;
+
+   strbuf_reset(type);
+   strbuf_grow(type, raw->len);
+   for (p = raw->buf; *p; p++) {
+   if (isspace(*p))
+   continue;
+   if (*p == ';')
+   break;
+   strbuf_addch(type, tolower(*p));
+   }
+}
+


This will parse invalid content types as valid.  Probably not  
important since the producer of an invalid content type shouldn't be  
depending on any particular behavior by the consumer of such a type,  
but I think it warrants a note in the comment block, perhaps something  
like:


  * Note that an invalid content-type may be converted to a valid one

or some such.

--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] strbuf: add strbuf_tolower function

[Kyle J. McKay]

On May 22, 2014, at 06:42, Jeff King wrote:


[re-adding list cc]

On Thu, May 22, 2014 at 03:16:45PM +0200, Christian Couder wrote:


+void strbuf_tolower(struct strbuf *sb)
+{
+   char *p;
+   for (p = sb->buf; *p; p++)
+   *p = tolower(*p);
+}


Last time I tried a change like the above, I was told that strbufs  
are

buffers that can contain NUL bytes. So maybe it should be:

  for (p = sb->buf; p < sb->buf + sb->len; p++)
 *p = tolower(*p);


Hah. I wrote it like that originally, and in review was asked to  
switch
it. I agree that it might be worth keeping strbuf functions 8bit- 
clean.

The original intent of the strbuf code was to make C strings easier to
use, but I think we sometimes use them as general buffers, too.



I didn't see this patch before I sent the other email, but this is the  
relevant part:


On May 22, 2014, at 15:52, Kyle J. McKay wrote:
The only reason I brought up the code difference in the first place  
was that the comment was "This makes config's lowercase() function  
public" which made me expect to see basically the equivalent of  
replacing a "static" with an "extern", but actually the function  
being made public was a different implementation than config's  
lowercase() function.  So it looks like the original PATCH 2/9  
version of the code is best, but perhaps the comment can be tweaked  
a bit to not convey the same impression I got.  Maybe something like  
"Replace config's lowercase() function with a public one".



Or even just delete the "This makes config's lowercase() function  
public" line after switching the implementation back.


--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 4/8] http: extract type/subtype portion of content-type

[Kyle J. McKay]

On May 23, 2014, at 13:12, Jeff King wrote:

On Thu, May 22, 2014 at 03:52:21PM -0700, Kyle J. McKay wrote:

+static void extract_content_type(struct strbuf *raw, struct  
strbuf *type)

+{
+   const char *p;
+
+   strbuf_reset(type);
+   strbuf_grow(type, raw->len);
+   for (p = raw->buf; *p; p++) {
+   if (isspace(*p))
+   continue;
+   if (*p == ';')
+   break;
+   strbuf_addch(type, tolower(*p));
+   }
+}
+


This will parse invalid content types as valid.  Probably not  
important
since the producer of an invalid content type shouldn't be  
depending on any
particular behavior by the consumer of such a type, but I think it  
warrants

a note in the comment block, perhaps something like:

 * Note that an invalid content-type may be converted to a valid one

or some such.


Yeah, that is intentional based on our earlier discussion (this  
function

started as "normalize_content_type" :) ). I think it's not a big deal,
but agree it's worth a comment. Like:

diff --git a/http.c b/http.c
index 4edf5b9..6bfd093 100644
--- a/http.c
+++ b/http.c
@@ -911,8 +911,14 @@ static CURLcode curlinfo_strbuf(CURL *curl,  
CURLINFO info, struct strbuf *buf)

 * spaces suppressed, all letters lowercased, and no trailing ";"
 * or parameters.
 *
+ * Note that we will silently remove even invalid whitespace. For
+ * example, "text / plain" is specifically forbidden by RFC 2616,
+ * but "text/plain" is the only reasonable output, and this keeps
+ * our code simple.


Very nice.  :)


+ *
 * Example:
 *   "TEXT/PLAIN; charset=utf-8" -> "text/plain"
+ *   "text / plain" -> "text/plain"
 */
static void extract_content_type(struct strbuf *raw, struct strbuf  
*type)

{

-Peff


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

Re: [PATCH 2/9] strbuf: add strbuf_tolower function

[Kyle J. McKay]

On May 23, 2014, at 13:05, Jeff King wrote:

On Thu, May 22, 2014 at 03:52:08PM -0700, Kyle J. McKay wrote:

Christian brought this up elsewhere, and I agree it's probably  
better to
work over the whole buffer, NULs included. I'm happy to re-roll  
(or you

can just pick up the version of the patch in this thread),


The only reason I brought up the code difference in the first place  
was that
the comment was "This makes config's lowercase() function public"  
which made
me expect to see basically the equivalent of replacing a "static"  
with an

"extern", but actually the function being made public was a different
implementation than config's lowercase() function.


Yeah, sorry if it sounded like I was complaining about your review
elsewhere. I mostly found it amusing that I got two opposite-direction
reviews.


I didn't seem like complaining to me.  I also was amused.  :)

I agree that clarifying the situation in the commit message is best,  
and

I've done that in the version I just posted.


It looks great.  And I suspect you're correct that a modern compiler  
would optimize the index-based version to be as efficient as the  
pointer arithmetic version.


--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] hooks/post-receive-email: do not truncate messages at a '.' line

[Kyle J. McKay]

When sendmail is reading an incoming message, by default it will
terminate it if it reads a dot on a line by itself.

The post-receive-email script can generate such a line if the
message in a tag object contains a dot ('.') on a line by itself.

There are two options to turn off this behavior, -i and -oi which
are equivalent.

The choice of which to use is based on the observation that -i
appears to be the more commonly used option for this and that all
current sendmail-compatible programs appear to accept both.

Therefore add the -i option to the invocations of /usr/sbin/sendmail
to avoid prematurely terminating the input message at the occurence
of a dot ('.') on a line by itself.

Signed-off-by: Kyle J. McKay 
---

BTW, what is the status of post-receive-email?

I find it quite useful for a minimal server that only needs Git
binaries and a POSIX shell.

The only real issue with it I'm aware of is that if multiple
branch heads are updated in a single push and two or more
contain the same new revison(s), those revision(s) will be
incorrectly omitted from the emails sent out.

I have a patch to correct that, but it's not quite as simple
as this patch and would need more review, so I don't really
want to send it out and waste folks' time if there's no interest
in picking up updates to contrib/hooks/post-receive-email.

 contrib/hooks/post-receive-email | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/contrib/hooks/post-receive-email b/contrib/hooks/post-receive-email
index 8747b843..6207be60 100755
--- a/contrib/hooks/post-receive-email
+++ b/contrib/hooks/post-receive-email
@@ -704,9 +704,9 @@ limit_lines()
 send_mail()
 {
if [ -n "$envelopesender" ]; then
-   /usr/sbin/sendmail -t -f "$envelopesender"
+   /usr/sbin/sendmail -i -t -f "$envelopesender"
else
-   /usr/sbin/sendmail -t
+   /usr/sbin/sendmail -i -t
fi
 }
 
-- 
1.8.5

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

git_inetd_server: run git-http-backend using inetd

[Kyle J. McKay]

===
What is it?
===
I have created a script (POSIX sh compatible) that allows running git- 
http-backend via inetd or similar service (it makes it like git-http- 
backend had a --inet option just like git-daemon does).


This allows the Git smart HTTP protocol to be served without needing  
to run a standard web server (assuming inetd, xinetd, launchd or  
similar is already available).


It is available at [1] with a GPL license.


Why?

I have several machines/virtual machines that need to be able to fetch  
from a central Git repository without authentication (several of them  
are test platforms and are therefore not trusted and typing a password  
on every fetch is a non-starter as some of the fetches are  
automated).  I also don't want to have to run a web server just to  
serve Git fetches.


Of course you say, "why not use git daemon for that?"  Well, I was (in  
--inetd mode), but it has a serious problem and until recently I  
didn't know the cause.


Talking to a git-daemon server works great for my FreeBSD, Darwin,  
cygwin, msysgit platforms, but it's terrible for my Linux platforms.


On the Linux platforms every git fetch would hang for at least 15  
seconds before starting the fetch.  I just wrote that off as some odd  
configuration issue I was going to have to live with (although it only  
happened with Git and not web browsing or curl fetches).  It's killer  
though when fetching a project with lots of submodules as each one has  
the delay.


Recently though, I discovered the cause.  While I don't actually run  
Debian itself, all my Linux platforms are Debian offspring (Ubuntu,  
Raspbian, Linux Mint, etc.).  The problem is in the Debian git package  
[2] specifically the "0010-transport-optionally-honor-DNS-SRV- 
records.diff" patch located in the Debian diffs file [3].


The 0010 patch makes "git://host/..." fetches first try to fetch a git  
SRV record from DNS.  This patch has a compile time option to enable  
it, but once baked in it cannot be turned off at runtime and the  
binary packages for my distributions all have it turned on and baked  
in.  And none of these distributions have a visible "WARNING: Git has  
patches that may cause it to behave oddly, violate RFC standards and/ 
or experience unexpected delays."


It so happens that my internal network is setup using mDNS [4] (on  
Linux/FreeBSD by installing Avahi and nss-mdns packages).


When I then try to fetch using a "git://host/..." URL where "host" is  
an mDNS host name, the 0010 patch causes git to attempt to lookup a  
DNS SRV record on the non-mDNS regular DNS service (a violation of RFC  
6762 [4] section 22) and this is what has to time out before the real  
fetch can start.


Since the patch does not try to lookup an SRV record for "http:// 
host/..." URLs, switching from git: to http: for fetches eliminates  
the problem.  However, git-http-backend does not have any of the -- 
daemon or --inetd related options that git-daemon does.  Hence the  
git_inetd_server.sh script [1] that provides the equivalent of a -- 
inetd option for git-http-backend.


I have since discovered that the 0010 patch behavior can be bypassed  
by explicitly including a port number in the git: URLs.  So instead of  
"git://example.local/repo.git" using "git://example.local:9418/ 
repo.git" avoids the delaying SRV lookup.  This is highly unintuitive  
since omitting the default port for a scheme should not change its  
behavior (according to RFC 3986 [5] section 3.2.3 the port should be  
omitted by producers and normalizers if it's the same as the scheme's  
default port).  In any case, unless git_inetd_server.sh can be run on  
the default http port (80), the resulting http: URLs are no more  
convenient than the git: URLs with an explicit port.


Nonetheless, others may find this script helpful as it allows the Git  
smart (and non-smart) HTTP protocol to be served without needing to  
run a standard web server so I decided to share it.


--Kyle

=
Links
=
[1] https://github.com/mackyle/git_inetd_server
[2] https://packages.debian.org/wheezy/git
[3] 
http://ftp.de.debian.org/debian/pool/main/g/git/git_1.7.10.4-1+wheezy1.diff.gz
[4] RFC 6762 http://tools.ietf.org/html/rfc6762
[5] RFC 3986 http://tools.ietf.org/html/rfc3986

--
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_inetd_server: run git-http-backend using inetd

[Kyle J. McKay]

On Jul 17, 2014, at 15:10, Jonathan Nieder wrote:

Hi,

Kyle J. McKay wrote:


When I then try to fetch using a "git://host/..." URL where "host"
is an mDNS host name, the 0010 patch causes git to attempt to lookup
a DNS SRV record on the non-mDNS regular DNS service (a violation of
RFC 6762 [4] section 22) and this is what has to time out before the
real fetch can start.


That patch uses res_query from libresolv to do the lookup.  It doesn't
know what DNS server to use and relies on system libraries to get it
right.


If I understand the libresolv implementation correctly, it only  
supports DNS and not mDNS.  And as far as I know, the nss plugins only  
support A and  lookups, not SRV lookups.  However, it looks like  
there are some attempts to add mDNS support directly to res_query and  
friends [4].  Maybe that code could serve as a model.



It was added to respond to a feature request within Debian but it is
intended to eventually go upstream.  I'm glad you found this issue
before that could happen. :)


Is there some reason the patch is not opt-in at runtime?  In other  
words the code is there, but not enabled unless you do something like  
'git config --system --bool git.srvlookup true'?  If it's off by  
default it doesn't matter so much that it's standards violating as it  
won't bite you unless you turn it on and then presumably if you do you  
know what you're doing and understand the possible downside.



Should git automatically disable the SRV lookups when it sees one of
the six domains named in RFC6762, or is there some system library call
that can use mDNS when appropriate automatically (or get partway there
without having to hard-code the domains)?


Sadly I think mDNS support is relegated to an add-on package on  
Linux.  And Avahi [1] is the package that generally provides it  
there.  The recommended interface for C is the avahi-client API (see  
[2]).  However, that is Avahi-only.


If the patch is to go upstream it probably needs to use the dns-sd.h  
header API for maximum compatibility (Avahi provides an avahi-compat- 
libdns_sd interface via the libavahi-compat-libdnssd-dev package).  I  
believe the correct function would be DNSServiceQueryRecord in this  
case.


That said, for the code to work properly it would need to:

1) Get the canonical name of the host.  If "local" is one of the  
search domains or the default domain you could have a mDNS name  
without an explicit .local suffix.  I'm not sure best how to do this.  
The getaddrinfo function has an AI_CANONNAME flag but the call will  
fail if the host does not have an A or  record and in the case of  
redirection-by-SRV it may have neither.  You probably just have to  
loop through the search domains keeping in mind the Security  
Considerations of section 21 of RFC 6762 [3] (a host MUST NOT append  
the search suffix ".local.", if present, to any relative (partially  
qualified) host name containing two or more labels. Appending  
".local." to single-label relative host names is acceptable).  So if a  
host name does not end in .local (or .local.) it can only be an mDNS  
name if it contains no dots ('.') AND "local" is one of the search  
domains or is the default domain.


2) For mDNS either use the #include  DNSServiceQueryRecord  
functionality if available or just skip it if not available.


3) For non-mDNS use the same query as it does now.

Even if the choice is to just disable SRV lookups for mDNS hosts at a  
minimum the code will have to determine whether or not the given host  
name is a mDNS name and it can't reliably do that for a host name  
without any dots in it without at least looking at the default domain  
name and possibly the search domain(s) as well.


I think it would be much simpler just to make this opt-in via a config  
option rather than baked in as it's probably going to be rather messy  
without direct mDNS support in the res_query interface.


--Kyle

[1] http://avahi.org/
[2] http://avahi.org/download/doxygen/index.html
[3] http://tools.ietf.org/html/rfc6762#page-52
[4] https://www.mail-archive.com/tech@openbsd.org/msg06100.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: git_inetd_server: run git-http-backend using inetd

[Kyle J. McKay]

On Jul 17, 2014, at 19:22, Jonathan Nieder wrote:


Thanks for these details.  I'll file a bug and mull it over some more.

RFC 6762 makes it clear that what the package is currently doing is
wrong.  Given that Debian's libc knows nothing about mdns on its own,
I think I'll need to parse resolv.conf (that's what libc-ares does).


You might also want to take a look at [1] which suggests that when  
doing SRV lookups for URLs they should be done regardless of whether  
or not a port number is present (which then eliminates the RFC 3986  
issue the current SRV lookup code has).  It's only a draft (and  
expired now and looks like it received a rather chilly reception from  
some), but I haven't noticed anything else that addresses what the  
patch code is trying to do.  The thread at [2] is related to what the  
patch is trying to do.


[1] http://tools.ietf.org/html/draft-ohta-urlsrv
[2] http://www.ietf.org/mail-archive/web/pcp/current/msg01727.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: git_inetd_server: run git-http-backend using inetd

[Kyle J. McKay]

On Jul 18, 2014, at 10:16, Jonathan Nieder wrote:


Kyle J. McKay wrote:


You might also want to take a look at [1] which suggests that when
doing SRV lookups for URLs they should be done regardless of whether
or not a port number is present (which then eliminates the RFC 3986
issue the current SRV lookup code has).


"Git URLs" as described e.g. in git-clone(1) weren't intended to be
actual URIs.


According to RFC 3968 section 1.1.3:
"A URI can be further classified as a locator, a name, or both. The  
term "Uniform Resource Locator" (URL) refers to the subset of  
URIs" [...]


So actually they are URIs.


What would be the interoperability advantage of making
them URIs?


According to RFC 3968 they are already considered URIs.



This has come up before, with e.g. people asking to introduce a
git+ssh:// and git+http://


How is a discussion about changing the scheme name relevant to a  
discussion about treating a URL with an explicit default port the same  
as one without (which Git already does but stops doing with the 0010  
git SRV patch)?  That would seem to be an orthogonal discussion to  
whether or not to change the scheme name(s) used by Git more than 9  
years after it first came out.

--
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_inetd_server: run git-http-backend using inetd

[Kyle J. McKay]

On Jul 18, 2014, at 17:19, Jonathan Nieder wrote:


Kyle J. McKay wrote:

On Jul 18, 2014, at 10:16, Jonathan Nieder wrote:


"Git URLs" as described e.g. in git-clone(1) weren't intended to be
actual URIs.


According to RFC 3968 section 1.1.3:
"A URI can be further classified as a locator, a name, or both. The
term "Uniform Resource Locator" (URL) refers to the subset of URIs"
[...]

So actually they are URIs.


You mean abusing the word URL when we meant URL-shaped thing makes it
into a URL?


That is your contention.


Do you
mean that I should read that RFC and be convinced that what you are
saying about ports is the right thing to do?


It's the right thing to do because it's the standard for how URLs are  
expected to behave.



"It's in a standard that you never
intended to follow" is not particularly convincing or relevant.


That is your contention.  If it is truly the case that where the Git  
documentation uses the "URL" acronym that Git does not actually intend  
for "URL" to be interpreted as a "URL" as defined by the various  
standards covering such URLs then explicit text needs to be added to  
the documentation saying so to avoid confusion.  In the absence of  
such text, expecting a reader of Git documentation to interpret the  
term "URL" any other way is irrational.

--
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 on Mac OS X 10.4.10

[Kyle J. McKay]

On Aug 14, 2014, at 16:18, Junio C Hamano wrote:

> Markus Hitter  writes:
>
>>> The   is in Mac OS X 10.6 .. 10.9,
>>> but not in 10.4 (I don't know about 10.5).

That header is new with 10.5

> Is this about platform dependency, or what the end user happens to
> choose to install (in other words, is there an add-on users of 10.4
> can choose to add, which allows them to use that header)?

Nope, it's a platform dependency.  Not available prior to 10.5.

The below patch does the right thing.  Conveniently there's already
a test for 10.4 and earlier so only a single line need be added.

--Kyle

 8< 
Subject: [PATCH] config.mak.uname: set NO_APPLE_COMMON_CRYPTO on older systems

Older MacOS systems prior to 10.5 do not have the CommonCrypto
support Git uses so set NO_APPLE_COMMON_CRYPTO on those systems.

Signed-off-by: Kyle J. McKay 
---
 config.mak.uname | 1 +
 1 file changed, 1 insertion(+)

diff --git a/config.mak.uname b/config.mak.uname
index 7846bd76..f8e12c96 100644
--- a/config.mak.uname
+++ b/config.mak.uname
@@ -88,6 +88,7 @@ ifeq ($(uname_S),Darwin)
NEEDS_LIBICONV = YesPlease
ifeq ($(shell expr "$(uname_R)" : '[15678]\.'),2)
OLD_ICONV = UnfortunatelyYes
+   NO_APPLE_COMMON_CRYPTO = YesPlease
endif
ifeq ($(shell expr "$(uname_R)" : '[15]\.'),2)
NO_STRLCPY = YesPlease
-- 
1.8.5

--
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 on Mac OS X 10.4.10

[Kyle J. McKay]

On Aug 15, 2014, at 10:02, Junio C Hamano wrote:


By the way, can we document this "uname_R on MacOS X" business
nearby, perhaps like this?

-- >8 --
Subject: config.mak.uname: add hint on uname_R for MacOS X

I always have to scratch my head every time I see this cryptic
pattern "[15678]\."; leave a short note to remind the maintainer
and the reviewers.

Signed-off-by: Junio C Hamano 
---
config.mak.uname | 4 
1 file changed, 4 insertions(+)

diff --git a/config.mak.uname b/config.mak.uname
index f8e12c9..7e49aca 100644
--- a/config.mak.uname
+++ b/config.mak.uname
@@ -86,6 +86,10 @@ ifeq ($(uname_S),Darwin)
NEEDS_CRYPTO_WITH_SSL = YesPlease
NEEDS_SSL_WITH_CRYPTO = YesPlease
NEEDS_LIBICONV = YesPlease
+   # Note: $(uname_R) gives us the underlying Darwin version.
+   # - MacOS 10.0 = Darwin 1.*
+   # - MacOS 10.x.? = Darwin (x+4).* for (1 <= x)
+   # i.e. "begins with [15678] and the a dot" means "10.4.* or older".


s/the a dot/a dot/


ifeq ($(shell expr "$(uname_R)" : '[15678]\.'),2)
OLD_ICONV = UnfortunatelyYes
NO_APPLE_COMMON_CRYPTO = YesPlease


Otherwise looks good.  Mac OS X 10.1.0 doesn't actually fit the  
pattern (it's still Darwin 1.*), but it's so old and it doesn't affect  
the 10.4.* or older test (or the later 10.1.* or older test), so let's  
just ignore that anomaly.  :)

--
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 on Mac OS X 10.4.10

[Kyle J. McKay]

On Aug 15, 2014, at 11:04, Junio C Hamano wrote:


The 10.1.0 anomaly actually was bothering me, too.  How
about doing it this way?

-- >8 --
Subject: [PATCH v2] config.mak.uname: add hint on uname_R for MacOS X

I always have to scratch my head every time I see this cryptic
pattern "[15678]\."; leave a short note to remind the maintainer
and the reviewers.

Signed-off-by: Junio C Hamano 
---
config.mak.uname | 4 
1 file changed, 4 insertions(+)

diff --git a/config.mak.uname b/config.mak.uname
index f8e12c9..414760f 100644
--- a/config.mak.uname
+++ b/config.mak.uname
@@ -86,6 +86,10 @@ ifeq ($(uname_S),Darwin)
NEEDS_CRYPTO_WITH_SSL = YesPlease
NEEDS_SSL_WITH_CRYPTO = YesPlease
NEEDS_LIBICONV = YesPlease
+   # Note: $(uname_R) gives us the underlying Darwin version.
+   # - MacOS 10.0.* and MacOS 10.1.0 = Darwin 1.*
+   # - MacOS 10.x.* = Darwin (x+4).* for (1 <= x)
+   # i.e. "begins with [15678] and a dot" means "10.4.* or older".
ifeq ($(shell expr "$(uname_R)" : '[15678]\.'),2)
OLD_ICONV = UnfortunatelyYes
NO_APPLE_COMMON_CRYPTO = YesPlease
--
2.1.0-rc2-283-g1433d67



Very nice.
--
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 18/18] signed push: final protocol update

[Kyle J. McKay]

On Aug 21, 2014, at 16:40, Junio C Hamano wrote:


* The receiving end will issue "push-cert=" in its initial
  capability advertisement, and this  will be given on the
  PUSH_CERT_NONCE environment to the pre/post-receive hooks, to
  allow the "nonce " header in the signed certificate to be
  checked against it.  You cannot capture my an earlier push to the
  authoritative server and replay it later.

  That would all work well within a single receive-pack process,
  but with "stateless" RPC, it is unclear to me how we should
  arrange the  the initial instance of receive-pack placed
  on its capability advertisement to be securely passed to the
  instance of receive-pack that actually receives the push
  certificate.


Have you considered having the advertised nonce only be updated after  
receipt of a successful signed push?


It would eliminate the stateless issue.  And since the next nonce to  
be advertised would be updated at the successful completion of a  
receive of a signed push no replay would be possible.  (I'm assuming  
that receive hook activity is already pipelined in the case of  
simultaneous pushes via some lock file or something or this scheme  
falls apart.)


The obvious downside is that only one of two or more simultaneous  
signed pushers could succeed.  But the sender could be modified to  
automatically retry (a limited number of times) on a nonce mismatch  
error.


A receive hook could also be responsible for generating the next nonce  
value using this technique.

--
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 merge: conflict is expected, but not detected

[Kyle J. McKay]

On Nov 29, 2013, at 06:26, Evgeniy Ivanov wrote:

Let's say I have two identical branches: master and topic. In master I
remove some code, i.e. function bar(). In topic I do the same (commit)
and after some time I realize I need bar() and revert previous commit
with removal.
So I end with master with no bar() and topic with bar() in its
original state. When I merge I get code without bar() and no merge
conflict (recursive or resolve strategies). Is it possible to detect
such situations as conflicts? When bar() is C++ virtual there is no
possibility to catch this with compiler.


You can do something like:

git checkout master
git merge -s ours --no-commit topic
# conflicts, if any, will happen during cherry-pick
git cherry-pick --no-commit ..topic
git commit -m "Merge branch 'topic'"

Which will give you a merge commit as though using "git merge" but it  
will have restored the bar() function.  However, depending on what's  
happened on the topic branch, you might have to wade through some  
conflicts that would not happen with a real "git merge" since cherry- 
pick will replay all the commits from the topic branch that aren't in  
master.  Maybe some day "git merge" will grow a "--cherry-pick" option.

--
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] gc: notice gc processes run by other users

[Kyle J. McKay]

Since 64a99eb4 git gc refuses to run without the --force option if
another gc process on the same repository is already running.

However, if the repository is shared and user A runs git gc on the
repository and while that gc is still running user B runs git gc on
the same repository the gc process run by user A will not be noticed
and the gc run by user B will go ahead and run.

The problem is that the kill(pid, 0) test fails with an EPERM error
since user B is not allowed to signal processes owned by user A
(unless user B is root).

Update the test to recognize an EPERM error as meaning the process
exists and another gc should not be run (unless --force is given).
---

I suggest this be included in maint as others may also have expected the
shared repository, different user gc scenario to be caught by the new
code when in fact it's not without this patch.

 builtin/gc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/builtin/gc.c b/builtin/gc.c
index c14190f8..25f2237c 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -222,7 +222,7 @@ static const char *lock_repo_for_gc(int force, pid_t* 
ret_pid)
time(NULL) - st.st_mtime <= 12 * 3600 &&
fscanf(fp, "%"PRIuMAX" %127c", &pid, locking_host) == 2 
&&
/* be gentle to concurrent "gc" on remote hosts */
-   (strcmp(locking_host, my_host) || !kill(pid, 0));
+   (strcmp(locking_host, my_host) || !kill(pid, 0) || 
errno == EPERM);
if (fp != NULL)
fclose(fp);
if (should_exit) {
-- 
1.8.5.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] Fix typesetting in Bugs section of 'git-rebase' man page (web version)

[Kyle J. McKay]

On Jan 14, 2014, at 21:36, Jason St. John wrote:
On Mon, Jan 13, 2014 at 12:55 PM, Junio C Hamano   
wrote:

"Jason St. John"  writes:


What AsciiDoc formatter (and version) do you use?


   $ asciidoc --version
   asciidoc 8.6.8

Checking with www.methods.co.nz/asciidoc, I am behind by about 2
months, it seems, though.


I just went through git-scm.com's GitHub issue tracker, and I found
that git-scm.com is, in fact, using their own AsciiDoc parser[1].

This issue, along with the other formatting issues found in
git-svn.txt and gitweb.txt, will hopefully be resolved soon, as one of
the GitHub maintainers opened an issue to update their AsciiDoc
parser[2].

[1] https://github.com/git/git-scm.com/issues/ 
134#issuecomment-27539970

[2] https://github.com/git/git-scm.com/issues/350


Hmmm.  I've also had some problems as well getting a correct man page  
build although it's the non-web version.  In my case I use the git- 
config.1 man page to detect the problem.  Uninteresting lines have  
been replaced with "[...]" for brevity in the following samples.   
Using 'MANWIDTH=65 man -c ./git-config.1' the bad man page has a  
section that looks like this:


http..*
   Any of the http.* options above can be applied
   selectively to some urls. For a config key to
   match a URL, each element of the config key is
   compared to that of the URL, in the following
   order:

1. Scheme (e.g., https in
   https://example.com/). This field must
   match exactly between the config key and
   the URL.
   [...]
5. User name (e.g., user in
   https://u...@example.com/repo.git). If the
   [...]
   precedence than a config key with a user
   name.
   The list above is ordered by decreasing
   precedence; a URL that matches a config
   [...]
   be preferred over a config key match of
   https://u...@example.com.

   All URLs are normalized before attempting
   any matching (the password part, if
   [...]
   any URLs visited as a result of a
   redirection do not participate in matching.

   i18n.commitEncoding
   Character encoding the commit messages are
   stored in; Git itself does not care per se,

The rest of the man page continues to be incorrectly indented too far,  
but I've omitted it for brevity.


The good output for the same section looks like this:

http..*
   Any of the http.* options above can be applied
   selectively to some urls. For a config key to
   match a URL, each element of the config key is
   compared to that of the URL, in the following
   order:

1. Scheme (e.g., https in
   https://example.com/). This field must
   match exactly between the config key and
   the URL.
   [...]
5. User name (e.g., user in
   https://u...@example.com/repo.git). If the
   [...]
   precedence than a config key with a user
   name.

   The list above is ordered by decreasing
   precedence; a URL that matches a config key's
   [...]
   preferred over a config key match of
   https://u...@example.com.

   All URLs are normalized before attempting any
   matching (the password part, if embedded in the
   [...]
   URLs visited as a result of a redirection do
   not participate in matching.

   i18n.commitEncoding
   Character encoding the commit messages are
   stored in; Git itself does not care per se, but

I have two different systems to build these pages on:

System A:
  OS X + Mac Ports
  asciidoc --version
asciidoc 8.6.6
  xmlto --version
xmlto version 0.0.24
  docbook-xsl is 1.75.2_0
  produces bad git-config.1

System B:
  Ubuntu
  asciidoc --version
asciidoc 8.5.2
  xmlto --version
xmlto version 0.0.23
  docbook-xsl is 1.75.2+dfsg-3
  produces good git-config.1

The git-config.1 file is built like so:

1. asciidoc produces git-config.xml from git-config.txt

2. xmlto produces git-config.1 from git-config.xml

System A:
  git-config.txt: md5=c63cbafddb1bc3d6c38b6b2941f8cbce
  git-config.xml: md5=99c0caa45ec02e8712c346e8842a7ce6
  git-config.1:   md5=812f8d180ec82c2786b95e986219a5d0

System B:
  git-config.txt: md5=c63cbafddb1bc3d6c38b6b2941f8cbce
  git-config.xml: md5=99c0caa45ec02e8712c346e8842a7ce6
  git-config.1:   md5=7298467bedd0225d5edefa8700bc9b2a

So actually the problem here is not the asciidoc step, it's actually  
the xmlto step.


And, if I make the following changes to the git-config.1 file produced  
on system A:


diff --git git-config.1 git-config-fix.1
--- git-config.1
+++ git-config-fix.1
@@ -3299,7 +3299,7 @@ user
 in
 https://user@example\&.com/repo\&.git)\&. If the config key  
has[...]

 .RE
-.RS 4
+.sp
 The list above is ordered by decreasing precedence; a URL that  
[...]

 https://

Re: [PATCH] send-email: If the ca path is not specified, use the defaults

[Kyle J. McKay]

On Jan 16, 2014, at 15:19, Junio C Hamano wrote:

Jonathan Nieder  writes:


FWIW this should help on Mac OS X, too.  Folks using git on mac
at $DAYJOB have been using the workaround described at
http://mercurial.selenic.com/wiki/CACertificates#Mac_OS_X_10.6_and_higher
so I forgot to report it. :/


Hmph, is that the same issue, though?  That page seems to suggest
using an empty ca file that does not have any useful information as
a workaround.


I had written up this draft email that describes the situation on OS X  
and decided it might not be exactly on topic and wasn't going to send  
it, but, since you asked... ;)


OpenSSL's default location can be found with:

  openssl version -d

On my Ubuntu system I get this:

  $ openssl version -d
  OPENSSLDIR: "/usr/lib/ssl"

Then if I look in there like so:

  $ ls -l /usr/lib/ssl
  total 8
  lrwxrwxrwx 1 root root   14 certs -> /etc/ssl/certs
  drwxr-xr-x 2 root root 4096 engines
  drwxr-xr-x 2 root root 4096 misc
  lrwxrwxrwx 1 root root   20 openssl.cnf -> /etc/ssl/openssl.cnf
  lrwxrwxrwx 1 root root   16 private -> /etc/ssl/private

And that's how it ends up using /etc/ssl/certs.  From the  
SSL_CTX_load_verify_locations man page:


  "When looking up CA certificates, the OpenSSL library will first  
search the certificates in CAfile, then those in CApath."


The default CAfile is "cert.pem" and the default CApath is "certs"  
both located in the openssl version -d directory.  See the crypto/ 
cryptlib.h header [1].


On my OS X platform depending on which version of OpenSSL I'm using,  
the OPENSSLDIR path would be one of these:


  /System/Library/OpenSSL
  /opt/local/etc/openssl

And neither of those uses a "certs" directory, they both use a  
"cert.pem" bundle instead:


  $ ls -l /System/Library/OpenSSL
  total 32
  lrwxrwxrwx  1 root  wheel42 cert.pem -> ../../../usr/share/curl/ 
curl-ca-bundle.crt

  drwxr-xr-x  2 root  wheel68 certs
  drwxr-xr-x  8 root  wheel   272 misc
  -rw-r--r--  1 root  wheel  9381 openssl.cnf
  drwxr-xr-x  2 root  wheel68 private
  # the certs directory is empty

  $ ls -l /opt/local/etc/openssl
  total 32
  lrwxrwxrwx   1 root  admin35 cert.pem@ -> ../../share/curl/curl- 
ca-bundle.crt

  drwxr-xr-x   9 root  admin   306 misc/
  -rw-r--r--   1 root  admin 10835 openssl.cnf

Notice neither of those refers to /etc/ssl/certs at all.

So the short answer is, yes, hard-coding /etc/ssl/certs as the path on  
OS X is incorrect and if setting /etc/ssl/certs as the path has the  
effect of replacing the default locations the verification will  
fail.   Of course Apple patches the included version of OpenSSL  
starting with OS X 10.6 to look in Apple's keychain, but if you're  
using a MacPorts-built version that won't happen and still /etc/ssl/ 
certs will be wrong in both cases.


As for the hint in that wiki/CACertificates link above, that does seem  
odd to me as well.


A much better solution would be (if python simply MUST have a file) to  
export the system's list of trusted root certificates like so:


  security export -k \
/System/Library/Keychains/SystemRootCertificates.keychain \
-t certs -f pemseq > rootcerts.pem

  # the generated rootcerts.pem file is suitable for use with the
  # openssl verify -CAfile option (root certs)

  # Substituting SystemCACertificates for SystemRootCertificates
  # produces a file suitable for use with the
  # openssl verify -untrusted option (intermediate certs)

and then point python to that rootcerts.pem file.  This file [2] may  
be helpful in understanding what's in SystemRootCertificates.keychain  
and SystemCACertificates.keychain.  The intermediate certs may also  
need to be exported to a file and pointed to as well (a quick glance  
at the hgrc docs did not turn up an option for this).


--Kyle

[1] http://git.openssl.org/gitweb/?p=openssl.git;a=blob;f=crypto/cryptlib.h#l84
[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/3] setup_pager: set MORE=R

[Kyle J. McKay]

On Jan 16, 2014, at 20:21, Jeff King wrote:

When we run the pager, we always set "LESS=R" to tell the
pager to pass through ANSI colors. On modern versions of
FreeBSD, the system "more" can do the same trick.

[snip]

diff --git a/pager.c b/pager.c
index 90d237e..2303164 100644
--- a/pager.c
+++ b/pager.c
@@ -87,6 +87,10 @@ void setup_pager(void)
argv_array_push(&env, "LESS=FRSX");
if (!getenv("LV"))
argv_array_push(&env, "LV=-c");
+#ifdef PAGER_MORE_UNDERSTANDS_R
+   if (!getenv("MORE"))
+   argv_array_push(&env, "MORE=R");
+#endif


How about adding a leading "-" to both the LESS and MORE settings?   
Since you're in there patching... :)


The man page for more states:

"Options are also taken from the environment variable MORE (make sure  
to precede them with a dash (``-'')) but command line options will  
override them."


And while the less man page does not have that wording, it does show  
this:


  LESS="-options"; export LESS

and this:

  LESS="-Dn9.1$-Ds4.1"

So it looks like both LESS and MORE prefer to have their options start  
with a '-' more or less.

--
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] send-email: If the ca path is not specified, use the defaults

[Kyle J. McKay]

On Jan 17, 2014, at 10:14, Junio C Hamano wrote:

If I am reading the code correctly, if /etc/ssl/certs does not exist
on the filesystem at all, it wouldn't even attempt verification, so
I take your "the verification will fail" to mean that you forgot to
also mention "And on OS X, /etc/ssl/certs directory still exists,
even though OpenSSL does not use it."  If that is the case, then our
current code indeed is broken in exactly the same way for OS X as
for Fedora.


My bad.  That directory does not normally exist, but, if some errant  
installer were to create an empty one...



In short, I agree with you on both counts (the current code is wrong
for OS X, and the proposed change will fix it).  I just want to make
sure that my understanding of the current breakage is in line with
the reality ;-)


Yes, you're right.  :)
--
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/3] setup_pager: set MORE=R

[Kyle J. McKay]

On Jan 20, 2014, at 21:30, Jeff King wrote:

Ugh. Having just read the LESS discussion, it makes me wonder if the

 strchr(getenv("LESS"), 'R')

check I add elsewhere in the series is sufficient. I suspect in  
practice

it is good enough, but I would not be surprised if there is a way to
fool it with a sufficiently complex LESS variable. Maybe we should  
just

assume it is enough, and people with crazy LESS settings can set
color.pager as appropriate?



This attempts to show colors but doesn't work properly:

  LESS=-+R git -c color.ui=auto -c color.pager=true log --oneline -- 
graph


but this does

  LESS=-R git -c color.ui=auto -c color.pager=true log --oneline -- 
graph


so yeah, just checking for 'R' in $LESS is only approximate.  :)
Also -r is good enough to show colors too...
--
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] Added get sendmail from .mailrc

[Kyle J. McKay]

On Jan 27, 2014, at 17:15, Jeff King wrote:
On Sat, Jan 25, 2014 at 01:46:50PM +0400, Brilliantov Kirill  
Vladimirovich wrote:



+   if (!defined $smtp_server) {
+   my $mailrc = File::HomeDir->my_home . "/.mailrc";


Actually, based on the output of "man mail", this should probably be  
something more like


  my $mailrc = $ENV{'MAILRC'} || "$ENV{'HOME'}/.mailrc";

which takes into account any MAILRC setting and also avoids the use of  
File::HomeDir.



+   while () {
+   chomp;
+   if (/set sendmail=.*/) {
+   my @data = split '"';
+   $smtp_server = $data[1];
+   last;
+   }


Your split is a rather unusual way to do the parsing, and it took me a
minute to figure it out. It might be more obvious as:

 if (/set sendmail="(.*)"/) {
 $smtp_server = $1;
 last;
 }

I do not know anything about the mailrc format, nor does it seem to be
well documented. Are the double-quotes required? If not, then the  
above
regex can easily make them optional. I also wonder if any whitespace  
is

allowed.


From "man mail":

   set   (se) With no arguments, prints all variable values.   
Otherwise,

 sets option.  Arguments are of the form option=value (no space
 before or after `=') or option.  Quotation marks may be placed
 around any part of the assignment statement to quote blanks or
 tabs, i.e. ``set indentprefix="->"''

My version of "man mail" does not list all the variables that can be  
set but it refers to "The Mail Reference Manual" document which  
presumably does.  I did find this [1] that documents many of the  
available variables including the sendmail one.  I then tried this:


cat < /tmp/shim
#!/bin/sh
exec cat
EOF
chmod a+x /tmp/shim
cat < /tmp/testrc
  se   send"mail"=/tm"p/"shim
EOF
echo 'testing' | MAILRC=/tmp/testrc mail -s test nobody

And to my surprise the contents of the new message were cat'd out to  
the terminal rather than being sent.  So clearly there's some room for  
improvement with the "set", white space and quote checking.


[1] http://www.cs.fsu.edu/sysinfo/mail/mailrc.html

--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/3] setup_pager: set MORE=R

[Kyle J. McKay]

On Feb 4, 2014, at 14:12, Jeff King wrote:

On Tue, Jan 21, 2014 at 11:23:30AM -0800, Junio C Hamano wrote:

does complicate the point of my series, which was to add more  
intimate

logic about how we handle LESS.
...
   return !x || strchr(x, 'R');

[...]

I am not sure if it is even a good idea for us to have so intimate
logic for various pagers in the first place.  I'd seriously wonder
if it is better to take this position:

A platform packager who sets the default pager and/or the
default environment for the pager at the build time, or an
individual user who tells your Git what pager you want to
use and/or with what setting that pager should be run under
with environment variables. These people ought to know far
better than Git what their specific choices do. Do not try
to second-guess them.


Sorry for letting this topic lapse; I wanted to look at some possible
Makefile improvements, which I'll be posting in a moment. I did want  
to

address this point, though.

If we are just talking about packagers and defaults (e.g., setting
MORE=R on FreeBSD), then I agree that the right tool for the job is
empowering the packagers, and the Makefile knob we've discussed does
that.


[snip]


It seems a shame to me that we cannot do better for such users.
However, the level of intimacy with the pager is getting to be a bit  
too
much for my taste, and the saving grace is that not that many people  
set

LESS themselves (and git is widely enough known that "R" is a common
recommendation when people do). So it's probably the lesser of two  
evils
to ignore the situation, and let people who run into it find the  
answers

on the web.

So I think there is nothing to be done.  But I did want to mention  
it in

case somebody else can come up with some clever solution. :)


I think we can do better without adding intimate pager knowledge.  And  
at the same time make it a simple matter of tweaking the Makefile to  
deal with new pagers on specific systems.


There are two parts to the proposal.

Part 1

Add a new configuration item which I will call "pager.env" for now  
that can have multiple values of the form ENV=value (whether multiple  
lines or whitespace separated on a single line to be decided later).


On a system where more can support color by setting MORE=-R it might  
look like this:


[pager]
env = LESS=-FRSX LV=-c MORE=-R

On a system where more does not it might look like this:

[pager]
env = LESS=-FRSX LV=-c

The default value of pager.env is to be configured in a system- 
specific way (i.e. Makefile knob) at build time.


Then, if Git is going to output color AND start a pager (that logic  
remains unchanged by this proposal), then it processes the pager.env  
value by examining each ENV=value setting and if the environment  
variable ENV is NOT already set, then sets it to value before starting  
the pager.


This is mostly a clean up and shouldn't really be controversial since  
before commit e54c1f2d2 the system basically behaved like this where  
pager.env was always set to "LESS=FRSX" and after it behaves as though  
pager.env is always set to "LESS=FRSX LV=-c".


Part 2

Alongside the current false/never, always, true/auto settings for  
"color.ui" a new "carefully" setting is introduced and the color.ui  
default if not set is changed from auto (commit 4c7f1819) to the new  
"carefully" setting.


Why a new setting?  So that people who have explicitly set color.ui to  
auto (or one of the other values) will not be affected by the proposed  
new logic.


Another new configuration item which I will call "pager.carefully" for  
now is introduced that again can have multiple values of the form  
name=ENV.  It might look like this:


[pager]
carefully = less=LESS LV=lv more=MORE

Again the default value of pager.carefully can be configured in a  
system-specific way (i.e. Makefile knob) at build time.


If color.ui is set to "carefully", then the logic is as follows:

a) Decide whether to output color the same way as for color.ui=auto
b) Select the pager the same way as for color.ui=auto
c) If (a) and (b) have selected a pager AND enabled color output then  
check to see if the selected pager appears in pager.carefully as one  
of the name values (perhaps a suffix match on the first whitespace- 
separated word of the selected pager value).
d) If the selected pager does not appear in pager.carefully, disable  
color output.
e) If the selected pager appears in pager.carefully, but the ENV  
associated with it does NOT appear in pager.env, disable color output.
f) If the pager appears in pager.carefully, but the ENV associated  
with it is already set, disable color output.
g) Otherwise, go ahead with color output as the change in Part 1 above  
will properly set the pager's options to enable it.


This has the following advantages:

1. No intimate pager knowledge.
2. Color output will be selected on supported 

Re: git-note -C changes commit type?

[Kyle J. McKay]

On Feb 11, 2014, at 16:06, Junio C Hamano wrote:

Johan Herland  writes:


There is currently no way the "git notes" commands will allow you to
store the 3d7de37 commit object directly as a note. There is also
(AFAICS) no easy workaround (git fast-import could've been a
workaround if it did not already require the first N/notemodify
argument to be a blob object). The best alternative, off the top of  
my

head, would be to write your own program using the notes.h API to
manipulate the notes tree directly (or - suboptimally - use other
low-level Git operations to do the same).


Even worse. I do not think such a non-blob object in the notes tree
does not participate in the reachability at all, so you won't be
able to fetch "refs/notes/whatever" and expect to get a useful
result.  I do not think storing the raw bits of commit object as a
blob in the notes tree is useful behaviour, either.  The command
probably should refuse to get anything non-blob via that option.


It would be nice if it let you store a tree or a blob, but I agree  
that it should complain about anything non-blob by default and if tree  
were to be allowed, that should require a special option.


If you do manually construct a notes tree that has a 'tree' entry  
instead of a blob, as soon as you add a new note, that 'tree' gets  
turned back into a blob again.  I was trying to attach a 'tree' as my  
note a while back and decided not to pursue it further after I found  
it got transformed into a 'blob' on the next notes modification.

--
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] notes: accept any ref for merge

[Kyle J. McKay]

  
allowing the user to remain in control, I have gussied up Johan's  
suggested fix for the failing notes test into the following patch.

--Kyle

-- 8< --
Subject: [PATCH] t/t3308-notes-merge.sh: succeed with relaxed notes refs

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 
Signed-off-by: Kyle J. McKay 
---
 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
--
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: OpenSSL deprecation warnings under Xcode

[Kyle J. McKay]

On Nov 30, 2014, at 21:31, Torsten Bögershausen wrote:


On 12/01/2014 04:02 AM, Michael Blume wrote:
I have no idea whether this should concern anyone, but my mac build  
of git shows


CC imap-send.o
imap-send.c:183:36: warning: 'ERR_error_string' is deprecated: first
deprecated in OS X 10.7 [-Wdeprecated-declarations]
fprintf(stderr, "%s: %s\n", func,
ERR_error_string(ERR_get_error(), NULL));
  ^

[]
Isn't the warning a warning ;-)
I don't see this warnings because my openssl comes from /opt/local/ 
include (Mac ports)
Does anybody know which new functions exist in Mac OS X versions >=  
10.7  ?


From [1]:

In addition to these APIs, a number of open source tools use OpenSSL  
for secure networking. If you use OpenSSL in your publicly shipping  
apps, you must provide your own copy of the OpenSSL libraries,  
preferably as part of your app bundle; the OpenSSL libraries that OS  
X provides are deprecated.


So using the version from Mac Ports is the right idea to avoid the  
problem.  You can always define NO_OPENSSL.  imap-send.c has a --curl  
option now. (Which presumably automatically becomes the default if you  
define NO_OPENSSL and not NO_CURL?)


--Kyle

[1] --

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

Re: [RFC][PATCH] send-email: add --[no-]xmailer option

[Kyle J. McKay]

On Dec 2, 2014, at 18:34, Eric Wong wrote:


Luis Henriques  wrote:

On Mon, Mar 24, 2014 at 09:38:27PM +, Luis Henriques wrote:
Add --[no-]xmailer that allows a user to disable adding the 'X- 
Mailer:'

header to the email being sent.



Ping

It's been a while since I sent this patch.  Is there any interest in
having this switch in git-send-email?


I wasn't paying attention when the original was sent, but this
looks good to me.

Acked-by: Eric Wong 

I honestly don't like disclosing too much information about my  
system,

in this case which MUA I'm using and its version.


Right on.  I would even favor this being the default.


I fully agree with you.

Auto-generated Message-Id headers also shows the use of git-send- 
email;

perhaps there can be a way to configure that, too.  However,
git-send-email respects manually-added Message-Id headers in the
original patch, so it's less of a problem, I suppose.


It can be hashed like so to avoid leaking information:

diff --git a/git-send-email.orig b/git-send-email.new
index f3d75e8..d0b4bff 100755
--- a/git-send-email.orig
+++ b/git-send-email.new
@@ -27,6 +27,7 @@ use Data::Dumper;
 use Term::ANSIColor;
 use File::Temp qw/ tempdir tempfile /;
 use File::Spec::Functions qw(catfile);
+use Digest::MD5 qw(md5_hex);
 use Error qw(:try);
 use Git;

@@ -901,8 +903,10 @@ sub make_message_id {
require Sys::Hostname;
$du_part = 'user@' . Sys::Hostname::hostname();
}
-   my $message_id_template = "<%s-git-send-email-%s>";
+   my $message_id_template = "%s-git-send-email-%s";
$message_id = sprintf($message_id_template, $uniq, $du_part);
+   @_ = split /@/, $message_id;
+	$message_id = '<'.substr(md5_hex($_[0]), 
0,31).'@'.substr(md5_hex($_[1]),1,31).'>';

#print "new message id = $message_id\n"; # Was useful for debugging
 }

---

--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 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 
Signed-off-by: Kyle J. McKay 
---
 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 v2 1/2] notes: accept any ref

[Kyle J. McKay]

From: Scott Chacon 

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 
---
 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 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" and "   or:" that are
properly supplied by the parse_options API.

Signed-off-by: Kyle J. McKay 
---
 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 [] [] [[--] ...]\n")
-   N_("   or: git show [options] ..."),
+   N_("git log [] [] [[--] ...]"),
+   N_("git show [options] ..."),
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 
---
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] 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 
---

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

*** 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"  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 ''

  2) Cannot merge empty notes ref () 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/ 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/

Re: [PATCH 2/2] gettext.h: add parentheses around N_ expansion

[Kyle J. McKay]

On Jan 6, 2015, at 05:24, Ramsay Jones wrote:


On 06/01/15 10:34, Kyle J. McKay wrote:

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



Hmm, see commit 642f85faa ("i18n: avoid parenthesized string as
array initializer", 07-04-2011), for a counter-point. :-P


Oh interesting.  So the bug bug fixed by 1/2 must have crept in after  
that change then.


Even clang -ansi -pedantic doesn't seem to complain about this.  And  
("a") is just as much a constant expression as "a".  Are you sure it's  
not just a tcc bug?


In any case, 642f85faa is talking about this:

  static const char ignore_error[] = ("something");

which is assigning a const char * to a const char [].

But builtin/log.c uses this:

  static const char * const builtin_log_usage[] = {("something",  
"else"};


Which is assigning a const char * to a const char * const.

Anyhow, if it breaks some existing compiler that works now, it  
shouldn't be picked up.  But it would sure be nice to have something  
to prevent a recurrence of the bug 1/2 fixes.  Perhaps the (msgid)  
version should be enabled when __GNUC__ is defined -- that should be  
compatible and enough folks use that it would catch such a problem  
early.


-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

[ANNOUNCE] Git Universal OS X Installer

[Kyle J. McKay]

What is it?
---

A universal installer for Git on OS X supporting Mac OS X 10.4.8 or  
later including PowerPC/Intel and both 32 and 64 bit.



Isn't there one already?


Yes, there is another OS X installer for Git available.  That one,  
however, did not meet my needs so I created this one and decided to  
share it in case anyone else has similar requirements.



Features


* This installer includes PowerPC/Intel 32/64 bit binaries and runs on  
10.4.8 or later.  So if you're running PPC 10.5 on a G5 and need a 64- 
bit Git you're good to go.


* The Git translations ARE included and may optionally be chosen based  
on the System Preferences languages selection(s) instead of the LANG  
and related environment variables.


* Besides the man pages; the html docs, release notes, api, howto etc.  
docs are also included.


* This is a Secure Transport-based Git -- no OpenSSL is used.  This  
means all root certificates (by default) come from the standard OS X  
keychain location(s).  Both git imap-send and git send-email operate  
using Secure Transport in this build (via libcurl).


* git send-email, git imap-send and git instaweb work out-of-the-box  
with no additional software installation required.


* git subtree and git-credential-osxkeychain are included.

* A build of the curl command-line utility that uses Secure Transport  
is also included (may be optionally disabled in the installer) for  
convenience.


* A build of the GnuPG gpg utility (with docs) is included (may be  
optionally disabled in the installer) for signing/verifying  
convenience which also supports creating top secret strength RSA keys.


* A build of TopGit is included (may be optionally disabled in the  
installer) for patch management.


* Library headers are included  (may be optionally disabled in the  
installer) for ease of building a custom Git against the same  
libraries the installed Git uses.


* Provides a complete solution for using Git on Mac OS X 10.4.x and  
10.5.x including full support for https/imaps/ftps/smtps TLS  
certificates which use the SHA-2 family of hash functions (such as  
SHA-256).


* Fully supports TLS 1.2 when run on a version of OS X with TLS 1.2  
support (OS X 10.9 and later).


* Currently provides Git version 2.1.4 (with backports of the  
executable-config-file fix, reflog-reading fix and imap-send-via- 
libcurl patch).  Installers for later versions of Git are planned.



Downloads
-

Download links (and hashes) for the 11 MiB installer (a .dmg image)  
are available at:


  http://mackyle.github.io/git-osx-installer/


-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-gui.sh: support Tcl 8.4

[Kyle J. McKay]

On Jan 6, 2015, at 11:42, Junio C Hamano wrote:


"Kyle J. McKay"  writes:


Tcl 8.5 introduced an extended vsatisfies syntax that is not
supported by Tcl 8.4.


Interesting.  We discussed this exact thing just before 2.0 in

   http://thread.gmane.org/gmane.comp.version-control.git/247511/focus=248858

and nobody seems to have noticed that giving the new range notation
to vsatisfies is too new back then.


Since only Tcl 8.4 is required this presents a problem.


Indeed.


However, what we really want is just a basic version compare,
so use vcompare instead to restore compatibility with Tcl 8.4.


My Tcl is not just rusty but corroded, so help me out here.


My Tcl is barely operational, but I'll give it a shot.  :)


* Your version that compares the sign of the result looks more
  correct than $gmane/248858; was the patch proposed back then but
  did not get applied wrong?  This question is out of mere
  curiosity.


Thanks for the reference.  That patch proposed this type of change:

-   if {[package vsatisfies $::_git_version 1.6.3]} {
+   if {[package vcompare $::_git_version 1.6.3]} {

But that's wrong because vsatisfies returns a boolean but vcompare  
returns an integer (think strcmp result) so the proposed change is  
testing whether the version is not 1.6.3 rather than being 1.6.3 or  
greater.  But Jens mentions this in $gmane/249491 (that the original  
patch was missing the ">= 0" part).


I can't find anything in that thread about why vsatisfies was  
preferred over vcompare other than the obvious that the vsatisfies  
version is only a 1-character change.  And that would be more than  
enough except that Tcl 8.4 doesn't support the trailing '-' vsatisfies  
syntax.  There are complaints about this problem with git-gui [1] by  
folks who have Tcl 8.4 on their system and have upgraded to Git 2.0 or  
later.



* Would it be a good idea to update the places $gmane/248895 points
  out?  It is clearly outside the scope of this fix, but we may
  want to do so while our mind is on the "how do we check required
  version?" in a separate patch.


Makes sense to me, but my Tcl knowledge isn't up to making those  
changes as the code's a bit different.  I have to paraphrase Chris's  
message here by saying that I guess those checks are correct if not  
consistent with the others.


[1] 
http://stackoverflow.com/questions/24315854/git-gui-cannot-start-because-of-bad-version-number

-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 2/2] t/t3308-notes-merge.sh: succeed with relaxed notes refs

[Kyle J. McKay]

On Jan 6, 2015, at 10:25, Junio C Hamano wrote:


"Kyle J. McKay"  writes:


So despite the name of the test, the actual tree contents do not seem
to be examined.


Yes, but the thing is, thanks to refs/notes restriction, there is no
need to do such examination [*1*].

Note that it is an entirely different matter when you deliberately
violate the expectation using plumbing (e.g. update-ref).  Users of
plumbing commands are expected to know what they are doing, so the
level of safety we need to give them can be much lower than Porcelain
commands such as 'git notes'.

But when you stick to Porcelain commands, it is very hard to mix
refs/notes/* with refs/heads/* and refs/remotes/* by mistake.  You
have to really work on it by doing something unusual to have a non
commit in refs/heads/*, a commit in refs/notes/*, or a regular
non-note commit in refs/notes/*.


Perhaps that is the crux of the issue.  There is no git notes-plumbing  
command where the git notes command continues to apply restrictions  
but the vaporware notes-plumbing command allows anything just like  
update-ref does.


I think there are two issues here:

1) There's no simple way to store remote notes refs outside the refs/ 
notes namespace in such a way that they can be used with git notes  
commands.


2) People who want to experiment with using git notes storage as a  
basis for building some new feature are forced to store their refs  
under the refs/notes namespace even if that does not make sense for  
the feature when what's stored in the notes tree is not intended to  
provide any content that is directly meaningful to the user.



That is exactly what I meant by that the existing safety pays price
of not being able to store notes outside refs/notes, which may be
too high a price to pay.


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.


As I said earlier (assuming you read footnotes before you read a new
paragraph), the ship has already sailed.


Hmpf.  So the only possible safety check is refname-based.  But, as  
you say, it's no use crying over spilled milk [3].



Obvious two sensible ways forward are to do a blacklist (i.e. allow
anywhere except for known non-notes namespaces like refs/heads/) or
do a whitelist (i.e. allow refs/ in addition to
refs/notes) of the namespace, and the latter is vastly preferrable
than the former, because you can practically *never* crawl back a
namespace once you give it to the general public, even if you later
find it a grave error to open it too widely and need a more
controlled access [*2*].  And the name of the game for a software
that can be relied for a long haul is to avoid painting ourselves in
a corner that we cannot get out of.

If we add refs/remote-notes/* to the whitelist now, and if later it
turns out to be not so ideal and we would prefer another layout for
remotes, e.g. refs/remotesNew/*/{heads,notes,tags}/, we can add
refs/remotesNew/*/notes/ to the whitelist _without_ breaking those
who have already started using refs/remote-notes/*.  That is why
I said whitelist is preferrable than blacklist.


A whitelist solves issue (1) but is no help for issue (2) unless some  
additional additional part of the refs namespace were to be also  
whitelisted.  Perhaps something like refs/x-/... in the same  
vein as the various IETF standards for experimental names.



[Footnote]

*1* I actually do not think a tree examination would help very much
   here.  IIRC, somebody decided long time ago that it would be a
   good idea to be able to store a path that is not a fanned-out
   40-hex in a notes tree and 'git notes merge' would accept such a
   notes-tree.  Although I doubt that the resulting notes-tree
   produced by 'notes merge' is carefully designed one (as opposed
   to whatever the implementation happens to do) with sensible
   semantics, people may already be relying on it.

*2* The above 'notes-tree can store non fanned-out 40-hex' is a good
   example why you need to start strict and loosen only when it
   becomes necessary.  Despite that even Git itself does not use
   that "facility" to do anything useful AFAIK, only because we
   started with a loose variant that allows arbitrary garbage, we
   cannot retroactively tighten the definition of what a notes-tree
   should look like without risking to break practice of other
   people.



[3] http://cheezburger.com/6423972864

-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 2/2] t/t3308-notes-merge.sh: succeed with relaxed notes refs

[Kyle J. McKay]

On Jan 6, 2015, at 15:54, Junio C Hamano wrote:


"Kyle J. McKay"  writes:


A whitelist solves issue (1) but is no help for issue (2) unless some
additional additional part of the refs namespace were to be also
whitelisted.  Perhaps something like refs/x-/... in the  
same

vein as the various IETF standards for experimental names.


Your (2) is about people who are _experimenting_, no?

Why can't they use refs/notes/x-/* while doing so, and
when that matures and proves useful to wider public we can make it
more official by whitelisting refs/ in addition to
refs/notes/, and the problem is solved, no?


I supposed if that's the best that can be done.  The problem is then  
that if you set notes.displayRef to refs/notes/* to see all relevant  
notes, don't you end up seeing these experimental, not-meant-for-end- 
user-consumption notes if they are somewhere under refs/notes/ ?  My  
glob specs are a little fuzzy, maybe it's refs/notes/** that gets  
everything under it?

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

Re: [PATCH v2 2/2] t/t3308-notes-merge.sh: succeed with relaxed notes refs

[Kyle J. McKay]

On Jan 6, 2015, at 16:28, Johan Herland wrote:

On Wed, Jan 7, 2015 at 12:29 AM, Kyle J. McKay   
wrote:
Perhaps that is the crux of the issue.  There is no git notes- 
plumbing
command where the git notes command continues to apply restrictions  
but the
vaporware notes-plumbing command allows anything just like update- 
ref does.


Good observation. From my POV, as people start creating tools that use
notes in a more structured manner (than as free-form plain-text  
appendages
to commit messages), the sharp and pointy bits (and bugs) of the  
interface
come into view. This applies to the safety features being discussed  
in this
thread, but also IMHO to the other things being currently discussed/ 
fixed

in the notes code:
[...]
I haven't considered that we might at some point be
better off splitting out a separate plumbing command for manipulating
notes. However, I'm not sure we're there, yet. The problems raised so
far do not IMHO warrant the creation of a whole new plumbing command.
Instead, we can still keep fixing and improving 'git notes'.

1) There's no simple way to store remote notes refs outside the  
refs/notes
namespace in such a way that they can be used with git notes  
commands.


2) People who want to experiment with using git notes storage as a  
basis for

building some new feature are forced to store their refs under the
refs/notes namespace even if that does not make sense for the  
feature when
what's stored in the notes tree is not intended to provide any  
content that

is directly meaningful to the user.



A whitelist solves issue (1) but is no help for issue (2) unless some
additional additional part of the refs namespace were to be also
whitelisted.  Perhaps something like refs/x-/... in the  
same vein

as the various IETF standards for experimental names.


Alternatively (or additionally), for issue (2), we could add a
--disable-ref-safety option to 'git notes', to explicitly disable the
safety checks for "experimental" use.


I like that.  Sort of a poor-man's git notes-plumbing option.  And  
there is precedence for this sort of thing as recently (v2.2.0) git  
hash-object learned a "--literally" option to bypass checks it would  
otherwise normally perform.


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

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

#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 
---
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 v3] gettext.h: add parentheses around N_ expansion if supported

[Kyle J. McKay]

The gettext 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 silently combine the strings into one and at runtime
the string passed to gettext will not match the strings that
were translated so no translation will actually occur.

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, allow this to be controlled by the
Makefile with the default only enabled when the compiler is
known to support the syntax.

For now only __GNUC__ enables this by default which covers both
gcc and clang which should result in early detection of any
adjacent N_ macros.

Although the necessary tests make the affected files a bit 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 additional
configuration tests introduce.

Helped-by: Junio C Hamano 
Signed-off-by: Kyle J. McKay 
---

The "yes", "no", "auto" settings for the new Makefile configuration
variable "USE_PARENS_AROUND_GETTEXT_N" are based on the way the
Makefile variable "COMPUTE_HEADER_DEPENDENCIES" works.  So I used
"yes" and "no" for consistency instead of "Yes" and "No".

 Makefile  | 17 +
 gettext.h | 24 
 git-compat-util.h |  4 
 3 files changed, 45 insertions(+)

diff --git a/Makefile b/Makefile
index 06e5d243..0f3fbccf 100644
--- a/Makefile
+++ b/Makefile
@@ -343,6 +343,15 @@ all::
 # return NULL when it receives a bogus time_t.
 #
 # Define HAVE_CLOCK_GETTIME if your platform has clock_gettime in librt.
+#
+# Define USE_PARENS_AROUND_GETTEXT_N to "yes" if your compiler happily
+# compiles the following initialization:
+#
+#   static const char s[] = ("FOO");
+#
+# 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.
 
 GIT-VERSION-FILE: FORCE
@$(SHELL_PATH) ./GIT-VERSION-GEN
@@ -950,6 +959,14 @@ ifneq (,$(SOCKLEN_T))
BASIC_CFLAGS += -Dsocklen_t=$(SOCKLEN_T)
 endif
 
+ifeq (yes,$(USE_PARENS_AROUND_GETTEXT_N))
+   BASIC_CFLAGS += -DUSE_PARENS_AROUND_GETTEXT_N=1
+else
+ifeq (no,$(USE_PARENS_AROUND_GETTEXT_N))
+   BASIC_CFLAGS += -DUSE_PARENS_AROUND_GETTEXT_N=0
+endif
+endif
+
 ifeq ($(uname_S),Darwin)
ifndef NO_FINK
ifeq ($(shell test -d /sw/lib && echo y),y)
diff --git a/gettext.h b/gettext.h
index 7671d09d..dc1722dd 100644
--- a/gettext.h
+++ b/gettext.h
@@ -63,6 +63,30 @@ const char *Q_(const char *msgid, const char *plu, unsigned 
long n)
 }
 
 /* Mark msgid for translation but do not translate it. */
+#if !USE_PARENS_AROUND_GETTEXT_N
 #define N_(msgid) msgid
+#else
+/*
+ * Strictly speaking, this will lead to invalid C when
+ * used this way:
+ * static const char s[] = N_("FOO");
+ * which will expand to
+ * static const char s[] = ("FOO");
+ * and in valid C, the initializer on the right hand side must
+ * be without the parentheses.  But many compilers do accept it
+ * as a language extension and it will allow us to catch mistakes
+ * like:
+ * static const char *msgs[] = {
+ * N_("one")
+ * N_("two"),
+ * N_("three"),
+ * NULL
+ * };
+ * (notice the missing comma on one of the lines) by forcing
+ * a compilation error, because parenthesised ("one") ("two")
+ * will not get silently turned into ("onetwo").
+ */
+#define N_(msgid) (msgid)
+#endif
 
 #endif
diff --git a/git-compat-util.h b/git-compat-util.h
index dcecd857..107e68e1 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -867,4 +867,8 @@ struct tm *git_gmtime_r(const time_t *, struct tm *);
 #define gmtime_r git_gmtime_r
 #endif
 
+#if !defined(USE_PARENS_AROUND_GETTEXT_N) && defined(__GNUC__)
+#define USE_PARENS_AROUND_GETTEXT_N 1
+#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

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 

-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: 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: 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  
 wrote:
On Wed, Jan 14, 2015 at 10:48 AM, Michael Blume  
 wrote:
On Wed, Jan 14, 2015 at 10:44 AM, Michael Blume > wrote:
On Wed, Jan 14, 2015 at 10:40 AM, Michael Blume > wrote:
On Wed, Jan 14, 2015 at 10:20 AM, Michael Blume > 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

2>&1


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

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 "/ 
private/tmp/clojure/.git/rebase-apply/patch", options=0) at builtin/ 
apply.c:4189
#7  0x0001ea3a in cmd_apply (argc=1, argv=0x7fff5fbff178,  
prefix_=0x0) a

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  
 wrote:
On Wed, Jan 14, 2015 at 10:48 AM, Michael Blume > wrote:
On Wed, Jan 14, 2015 at 10:44 AM, Michael Blume > wrote:
On Wed, Jan 14, 2015 at 10:40 AM, Michael Blume > wrote:
On Wed, Jan 14, 2015 at 10:20 AM, Michael Blume > 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

2>&1


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

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

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

* 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

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

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"  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 22, 2015, at 11:23, Junio C Hamano wrote:

"Kyle J. McKay"  writes:


On Jan 21, 2015, at 14:33, Junio C Hamano wrote:


"Kyle J. McKay"  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