[PATCH (Apple Git) 00/13] Differences between git-2.20.1 and Apple Git-116

[PATCH (Apple Git) 00/13] Differences between git-2.20.1 and Apple Git-116

[Jeremy Huddleston Sequoia]

Xcode 10.2 Beta 1 contains Apple Git-116.  A couple months ago, Peff encouraged
me to re-send our changes in a patch series to the mailing list, so here they
are!

As reference, these (and past patch series for Apple Git) are availale at:
    https://github.com/jeremyhu/git

Some of these patches are upstreamable as is, but othes represent areas where
customization options could be provided upstream to allow similar behavior.

Note that I am very grateful for the recent changes in mainline to support
RUNTIME_PREFIX on darwin.  It almost completely replaced our RUNTIME_PREFIX
implementation and made the few remaining changes much more maintainale.

Please integrate the changes that are upstreamable and let's use the others
as a starting point for discussing how to accomplish the same effect through
configuration options or other means.

Thanks,
Jeremy

[PATCH (Apple Git) 01/13] .gitignore: Remove *.s as it matches *.S on case insensitive filesystem

[Jeremy Huddleston Sequoia]

This was causing problems with ppc/sha1ppc.S

Signed-off-by: Jeremy Huddleston Sequoia <jeremyhu@apple.com>
---
 .gitignore | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/.gitignore b/.gitignore
index 0d77ea5894..a5db584576 100644
--- a/.gitignore
+++ b/.gitignore
@@ -195,7 +195,7 @@
 *.deb
 /git.spec
 *.exe
-*.[aos]
+*.[ao]
 *.py[co]
 .depend/
 *.gcda
-- 
2.20.0 (Apple Git-115)

Re: [PATCH (Apple Git) 01/13] .gitignore: Remove *.s as it matches *.S on case insensitive filesystem

[Eric Sunshine]

On Tue, Jan 29, 2019 at 3:11 PM Jeremy Huddleston Sequoia
<jeremyhu@apple.com> wrote:
> This was causing problems with ppc/sha1ppc.S

What problems, exactly?

> Signed-off-by: Jeremy Huddleston Sequoia <jeremyhu@apple.com>
> ---
> diff --git a/.gitignore b/.gitignore
> @@ -195,7 +195,7 @@
> -*.[aos]
> +*.[ao]

Re: [PATCH (Apple Git) 01/13] .gitignore: Remove *.s as it matches *.S on case insensitive filesystem

[Jeremy Huddleston Sequoia]

> On Jan 30, 2019, at 03:33, Eric Sunshine <sunshine@sunshineco.com> wrote:
> 
> On Tue, Jan 29, 2019 at 3:11 PM Jeremy Huddleston Sequoia
> <jeremyhu@apple.com> wrote:
>> This was causing problems with ppc/sha1ppc.S
> 
> What problems, exactly?

The file is ignored, but it shouldn't be.

> 
>> Signed-off-by: Jeremy Huddleston Sequoia <jeremyhu@apple.com>
>> ---
>> diff --git a/.gitignore b/.gitignore
>> @@ -195,7 +195,7 @@
>> -*.[aos]
>> +*.[ao]

Re: [PATCH (Apple Git) 01/13] .gitignore: Remove *.s as it matches *.S on case insensitive filesystem

[Eric Sunshine]

On Wed, Jan 30, 2019 at 6:37 AM Jeremy Huddleston Sequoia
<jeremyhu@apple.com> wrote:
> > On Jan 30, 2019, at 03:33, Eric Sunshine <sunshine@sunshineco.com> wrote:
> > On Tue, Jan 29, 2019 at 3:11 PM Jeremy Huddleston Sequoia
> > <jeremyhu@apple.com> wrote:
> >> This was causing problems with ppc/sha1ppc.S
> >
> > What problems, exactly?
>
> The file is ignored, but it shouldn't be.

But what problem are you experiencing, exactly? .gitignore rules do
not impact tracked files such as ppc/sha1ppc.S, even if the name
matches an ignore-rule, so it's not clear what problem you're trying
to solve.

Re: [PATCH (Apple Git) 01/13] .gitignore: Remove *.s as it matches *.S on case insensitive filesystem

[Eric Sunshine]

On Wed, Jan 30, 2019 at 7:29 AM Eric Sunshine <sunshine@sunshineco.com> wrote:
> On Wed, Jan 30, 2019 at 6:37 AM Jeremy Huddleston Sequoia
> <jeremyhu@apple.com> wrote:
> > > On Jan 30, 2019, at 03:33, Eric Sunshine <sunshine@sunshineco.com> wrote:
> > > On Tue, Jan 29, 2019 at 3:11 PM Jeremy Huddleston Sequoia
> > > <jeremyhu@apple.com> wrote:
> > >> This was causing problems with ppc/sha1ppc.S
> > >
> > > What problems, exactly?
> >
> > The file is ignored, but it shouldn't be.
>
> But what problem are you experiencing, exactly? .gitignore rules do
> not impact tracked files such as ppc/sha1ppc.S, even if the name
> matches an ignore-rule, so it's not clear what problem you're trying
> to solve.

I'm guessing that this has something to do with HFS+ being
case-insensitive yet case-preserving, but an actual explanation of the
misbehavior experienced would be helpful.

Re: [PATCH (Apple Git) 01/13] .gitignore: Remove *.s as it matches *.S on case insensitive filesystem

[Johannes Schindelin]

Hi Jeremy,

On Wed, 30 Jan 2019, Jeremy Huddleston Sequoia wrote:

> > On Jan 30, 2019, at 03:33, Eric Sunshine <sunshine@sunshineco.com> wrote:
> > 
> > On Tue, Jan 29, 2019 at 3:11 PM Jeremy Huddleston Sequoia
> > <jeremyhu@apple.com> wrote:
> >> This was causing problems with ppc/sha1ppc.S
> > 
> > What problems, exactly?
> 
> The file is ignored, but it shouldn't be.

As somebody who sometimes (pretty rarely, but definitely more than once a
year) generates the assembler files to have a deeper look, I really
understand why *.s is ignored, and I think it should stay ignored.

What you probably want instead is

	# Accommodate for case-insensitive filesystems where *.s would catch
	!ppc/sha1ppc.S

after the `*.[aos]` line.

Ciao,
Johannes

> 
> > 
> >> Signed-off-by: Jeremy Huddleston Sequoia <jeremyhu@apple.com>
> >> ---
> >> diff --git a/.gitignore b/.gitignore
> >> @@ -195,7 +195,7 @@
> >> -*.[aos]
> >> +*.[ao]
> 
> 

Re: [PATCH (Apple Git) 01/13] .gitignore: Remove *.s as it matches *.S on case insensitive filesystem

[Jeremy Huddleston Sequoia]

> On Jan 30, 2019, at 04:42, Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
> 
> Hi Jeremy,
> 
> On Wed, 30 Jan 2019, Jeremy Huddleston Sequoia wrote:
> 
>>> On Jan 30, 2019, at 03:33, Eric Sunshine <sunshine@sunshineco.com> wrote:
>>> 
>>> On Tue, Jan 29, 2019 at 3:11 PM Jeremy Huddleston Sequoia
>>> <jeremyhu@apple.com> wrote:
>>>> This was causing problems with ppc/sha1ppc.S
>>> 
>>> What problems, exactly?
>> 
>> The file is ignored, but it shouldn't be.
> 
> As somebody who sometimes (pretty rarely, but definitely more than once a
> year) generates the assembler files to have a deeper look, I really
> understand why *.s is ignored, and I think it should stay ignored.
> 
> What you probably want instead is
> 
> 	# Accommodate for case-insensitive filesystems where *.s would catch
> 	!ppc/sha1ppc.S
> 
> after the `*.[aos]` line.

Thanks for the suggestion.  I didn't know that was possible with .gitignore.  That's a much better solution.  I was expecting this to just stay in our series as upstreamable, but I'm glad you pointed this out.  I got to learn something new and offload a patch.  Thanks!

I'll now be able include this in my followup series of patches that I think can be upstreamed! =)

> Ciao,
> Johannes
> 
>> 
>>> 
>>>> Signed-off-by: Jeremy Huddleston Sequoia <jeremyhu@apple.com>
>>>> ---
>>>> diff --git a/.gitignore b/.gitignore
>>>> @@ -195,7 +195,7 @@
>>>> -*.[aos]
>>>> +*.[ao]
>> 
>> 

Re: [PATCH (Apple Git) 01/13] .gitignore: Remove *.s as it matches *.S on case insensitive filesystem

[Junio C Hamano]

Jeremy Huddleston Sequoia <jeremyhu@apple.com> writes:

>> What you probably want instead is
>> 
>> 	# Accommodate for case-insensitive filesystems where *.s would catch
>> 	!ppc/sha1ppc.S
>> 
>> after the `*.[aos]` line.
>
> Thanks for the suggestion.  I didn't know that was possible with
> .gitignore.  That's a much better solution.

I still do not see what problem you need a "solution" for in the
first place---I saw a few comments asking it in the thread, but saw
no answer.  ppc/sha1ppc.S is already tracked, so any modification
you make in the working tree can be added to the index with "git
add" and "git status" would report when you have modification to
that file in the working tree, without any such extra entry in
.gitignore, no?

Re: [PATCH (Apple Git) 01/13] .gitignore: Remove *.s as it matches *.S on case insensitive filesystem

[Jeremy Sequoia]

Sent from my iPhone...

> On Jan 31, 2019, at 09:57, Junio C Hamano <gitster@pobox.com> wrote:
> 
> Jeremy Huddleston Sequoia <jeremyhu@apple.com> writes:
> 
>>> What you probably want instead is
>>> 
>>>    # Accommodate for case-insensitive filesystems where *.s would catch
>>>    !ppc/sha1ppc.S
>>> 
>>> after the `*.[aos]` line.
>> 
>> Thanks for the suggestion.  I didn't know that was possible with
>> .gitignore.  That's a much better solution.
> 
> I still do not see what problem you need a "solution" for in the
> first place---I saw a few comments asking it in the thread, but saw
> no answer.  ppc/sha1ppc.S is already tracked, so any modification
> you make in the working tree can be added to the index with "git
> add" and "git status" would report when you have modification to
> that file in the working tree, without any such extra entry in
> .gitignore, no?

I originally saw this because the .gitignore was present in the tarball, and we would only update to new versions by pulling in the newer tarball.  This resulted in *.S not being included in our repo.

When I switched to having upstream be a subtree instead of an extracted tarball, I noticed this difference when comparing the source code difference between the tarball based approach and the git subtree approach.

This would have implications for anyone doing something similar or to anyone intending to add new assembly files to the tree (since they wouldn’t show up in status or get added with add -A),

Re: [PATCH (Apple Git) 01/13] .gitignore: Remove *.s as it matches *.S on case insensitive filesystem

[Eric Sunshine]

On Thu, Jan 31, 2019 at 1:17 PM Jeremy Sequoia <jeremyhu@apple.com> wrote:
> > On Jan 31, 2019, at 09:57, Junio C Hamano <gitster@pobox.com> wrote:
> > I still do not see what problem you need a "solution" for in the
> > first place---I saw a few comments asking it in the thread, but saw
> > no answer.  ppc/sha1ppc.S is already tracked, so any modification
> > you make in the working tree can be added to the index with "git
> > add" and "git status" would report when you have modification to
> > that file in the working tree, without any such extra entry in
> > .gitignore, no?
>
> This would have implications for anyone doing something similar or
> to anyone intending to add new assembly files to the tree (since
> they wouldn’t show up in status or get added with add -A),

This nugget finally gives readers an idea of the sort of issue this
patch wants to "fix", which happens to be related to HFS+ being
case-insensitive. As noted upstream, though, files which are already
tracked, such as ppc/sha1ppc.S, are not subject to .gitignore, so
Dscho's proposed modification to .gitignore:

    ...
    *.[aos]
    !ppc/sha1ppc.S
    ...

doesn't actually help. Moreover, this .gitignore change doesn't at all
help the case you describe about new assembly files not being noticed
by "git status" or "git add -A" since any new files won't be named
"ppc/sha1ppc.S".

As Junio said upstream, .gitignore ignoring "*.s" files is the right
thing to do for this project and, as adding new .S files is so rare,
it seems unlikely that a patch changing .gitignore to accommodate the
above use-case for case-insensitive filesystems would make sense to
the project as a whole.

Re: [PATCH (Apple Git) 01/13] .gitignore: Remove *.s as it matches *.S on case insensitive filesystem

[Junio C Hamano]

Jeremy Huddleston Sequoia <jeremyhu@apple.com> writes:

> This was causing problems with ppc/sha1ppc.S

"causing problems"?  How?  The source file is already tracked so
"git add" to record updates to it would work just fine, and "git
status" would report "modified" for it, with or without "*.S" in the
.gitignore file, no?

    To emulate, I just added ".*S" to .gitignore and modified
    ppc/sha1ppc.S to double check whta happens.

We still have the build rule for %.s in our Makefile, so ignoring
".s" is still the right thing to do to make sure "git add dir/"
won't add "dir/frotz.s" by accident.  I sense the downside of doing
so outweighs whatever benefit it would have on case incapable
filesystems (and it is unclear what problem this change is trying to
work around).

> Signed-off-by: Jeremy Huddleston Sequoia <jeremyhu@apple.com>
> ---
>  .gitignore | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/.gitignore b/.gitignore
> index 0d77ea5894..a5db584576 100644
> --- a/.gitignore
> +++ b/.gitignore
> @@ -195,7 +195,7 @@
>  *.deb
>  /git.spec
>  *.exe
> -*.[aos]
> +*.[ao]
>  *.py[co]
>  .depend/
>  *.gcda

[PATCH (Apple Git) 02/13] test-lib: Export PERL5LIB for testing git-svn

[Jeremy Huddleston Sequoia]

Signed-off-by: Jeremy Huddleston Sequoia <jeremyhu@apple.com>
---
 t/test-lib.sh | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/t/test-lib.sh b/t/test-lib.sh
index 0f1faa24b2..4060a53f56 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -1017,6 +1017,9 @@ fi
 
 GITPERLLIB="$GIT_BUILD_DIR"/perl/build/lib
 export GITPERLLIB
+PERL_VERSION=$(grep DEFAULT /usr/local/versioner/perl/versions | sed 's:^.*= *\([^ ]*\)$:\1:')
+PERL5LIB="$GIT_BUILD_DIR"/perl:"$(xcode-select -p)"/Library/Perl/$PERL_VERSION
+export PERL5LIB
 test -d "$GIT_BUILD_DIR"/templates/blt || {
 	error "You haven't built things yet, have you?"
 }
-- 
2.20.0 (Apple Git-115)

Re: [PATCH (Apple Git) 02/13] test-lib: Export PERL5LIB for testing git-svn

[Junio C Hamano]

Jeremy Huddleston Sequoia <jeremyhu@apple.com> writes:

> Signed-off-by: Jeremy Huddleston Sequoia <jeremyhu@apple.com>
> ---
>  t/test-lib.sh | 3 +++
>  1 file changed, 3 insertions(+)

This obviously won't be acceptable as-is to my tree.  Shouldn't this
be something to be dealt with in config.mak.uname or something that
is meant to define platform-specific customization?

>
> diff --git a/t/test-lib.sh b/t/test-lib.sh
> index 0f1faa24b2..4060a53f56 100644
> --- a/t/test-lib.sh
> +++ b/t/test-lib.sh
> @@ -1017,6 +1017,9 @@ fi
>  
>  GITPERLLIB="$GIT_BUILD_DIR"/perl/build/lib
>  export GITPERLLIB
> +PERL_VERSION=$(grep DEFAULT /usr/local/versioner/perl/versions | sed 's:^.*= *\([^ ]*\)$:\1:')
> +PERL5LIB="$GIT_BUILD_DIR"/perl:"$(xcode-select -p)"/Library/Perl/$PERL_VERSION
> +export PERL5LIB
>  test -d "$GIT_BUILD_DIR"/templates/blt || {
>  	error "You haven't built things yet, have you?"
>  }

Re: [PATCH (Apple Git) 02/13] test-lib: Export PERL5LIB for testing git-svn

[Jeremy Huddleston Sequoia]

> On Jan 29, 2019, at 2:47 PM, Junio C Hamano <gitster@pobox.com> wrote:
> 
> Jeremy Huddleston Sequoia <jeremyhu@apple.com> writes:
> 
>> Signed-off-by: Jeremy Huddleston Sequoia <jeremyhu@apple.com>
>> ---
>> t/test-lib.sh | 3 +++
>> 1 file changed, 3 insertions(+)
> 
> This obviously won't be acceptable as-is to my tree.  Shouldn't this
> be something to be dealt with in config.mak.uname or something that
> is meant to define platform-specific customization?

The issue here is that we're not locating relocatable perl modules during testing.  This is a general problem with testing RUNTIME_PREFIX configurations, and a more general solution to this sledgehammer would be appropriate.  I don't think config.mak.uname really makes sense since it's a general RUNTIME_PREFIX issue and not specifically a darwin issue.

> 
>> 
>> diff --git a/t/test-lib.sh b/t/test-lib.sh
>> index 0f1faa24b2..4060a53f56 100644
>> --- a/t/test-lib.sh
>> +++ b/t/test-lib.sh
>> @@ -1017,6 +1017,9 @@ fi
>> 
>> GITPERLLIB="$GIT_BUILD_DIR"/perl/build/lib
>> export GITPERLLIB
>> +PERL_VERSION=$(grep DEFAULT /usr/local/versioner/perl/versions | sed 's:^.*= *\([^ ]*\)$:\1:')
>> +PERL5LIB="$GIT_BUILD_DIR"/perl:"$(xcode-select -p)"/Library/Perl/$PERL_VERSION
>> +export PERL5LIB
>> test -d "$GIT_BUILD_DIR"/templates/blt || {
>> 	error "You haven't built things yet, have you?"
>> }

Re: [PATCH (Apple Git) 02/13] test-lib: Export PERL5LIB for testing git-svn

[SZEDER Gábor]

On Tue, Jan 29, 2019 at 03:46:07PM -0800, Jeremy Huddleston Sequoia wrote:
> 
> 
> > On Jan 29, 2019, at 2:47 PM, Junio C Hamano <gitster@pobox.com> wrote:
> > 
> > Jeremy Huddleston Sequoia <jeremyhu@apple.com> writes:
> > 
> >> Signed-off-by: Jeremy Huddleston Sequoia <jeremyhu@apple.com>
> >> ---
> >> t/test-lib.sh | 3 +++
> >> 1 file changed, 3 insertions(+)
> > 
> > This obviously won't be acceptable as-is to my tree.  Shouldn't this
> > be something to be dealt with in config.mak.uname or something that
> > is meant to define platform-specific customization?
> 
> The issue here is that we're not locating relocatable perl modules
> during testing.  This is a general problem with testing
> RUNTIME_PREFIX configurations, and a more general solution to this
> sledgehammer would be appropriate.  I don't think config.mak.uname
> really makes sense since it's a general RUNTIME_PREFIX issue and not
> specifically a darwin issue.

But this patch is very darwin-specific ...

> >> diff --git a/t/test-lib.sh b/t/test-lib.sh
> >> index 0f1faa24b2..4060a53f56 100644
> >> --- a/t/test-lib.sh
> >> +++ b/t/test-lib.sh
> >> @@ -1017,6 +1017,9 @@ fi
> >> 
> >> GITPERLLIB="$GIT_BUILD_DIR"/perl/build/lib
> >> export GITPERLLIB
> >> +PERL_VERSION=$(grep DEFAULT /usr/local/versioner/perl/versions | sed 's:^.*= *\([^ ]*\)$:\1:')
> >> +PERL5LIB="$GIT_BUILD_DIR"/perl:"$(xcode-select -p)"/Library/Perl/$PERL_VERSION

... because what other platforms could possibly have 'xcode-select'
installed!?  Consequently:

  $ ./t0000-basic.sh 
  grep: /usr/local/versioner/perl/versions: No such file or directory
  ./t0000-basic.sh: 1154: ./test-lib.sh: xcode-select: not found
  ok 1 - verify that the running shell supports "local"
  ok 2 - .git/objects should be empty after git init in an empty repo
  ok 3 - .git/objects should have 3 subdirectories
  ok 4 - success is reported like this
  not ok 5 - pretend we have a fully passing test suite
  <...>
  # failed 29 among 82 test(s)



> >> +export PERL5LIB
> >> test -d "$GIT_BUILD_DIR"/templates/blt || {
> >> 	error "You haven't built things yet, have you?"
> >> }
> 

Re: [PATCH (Apple Git) 02/13] test-lib: Export PERL5LIB for testing git-svn

[Jeremy Sequoia]

Sent from my iPhone...

> On Jan 29, 2019, at 15:59, SZEDER Gábor <szeder.dev@gmail.com> wrote:
> 
>> On Tue, Jan 29, 2019 at 03:46:07PM -0800, Jeremy Huddleston Sequoia wrote:
>> 
>> 
>>> On Jan 29, 2019, at 2:47 PM, Junio C Hamano <gitster@pobox.com> wrote:
>>> 
>>> Jeremy Huddleston Sequoia <jeremyhu@apple.com> writes:
>>> 
>>>> Signed-off-by: Jeremy Huddleston Sequoia <jeremyhu@apple.com>
>>>> ---
>>>> t/test-lib.sh | 3 +++
>>>> 1 file changed, 3 insertions(+)
>>> 
>>> This obviously won't be acceptable as-is to my tree.  Shouldn't this
>>> be something to be dealt with in config.mak.uname or something that
>>> is meant to define platform-specific customization?
>> 
>> The issue here is that we're not locating relocatable perl modules
>> during testing.  This is a general problem with testing
>> RUNTIME_PREFIX configurations, and a more general solution to this
>> sledgehammer would be appropriate.  I don't think config.mak.uname
>> really makes sense since it's a general RUNTIME_PREFIX issue and not
>> specifically a darwin issue.
> 
> But this patch is very darwin-specific ...
> 
>>>> diff --git a/t/test-lib.sh b/t/test-lib.sh
>>>> index 0f1faa24b2..4060a53f56 100644
>>>> --- a/t/test-lib.sh
>>>> +++ b/t/test-lib.sh
>>>> @@ -1017,6 +1017,9 @@ fi
>>>> 
>>>> GITPERLLIB="$GIT_BUILD_DIR"/perl/build/lib
>>>> export GITPERLLIB
>>>> +PERL_VERSION=$(grep DEFAULT /usr/local/versioner/perl/versions | sed 's:^.*= *\([^ ]*\)$:\1:')
>>>> +PERL5LIB="$GIT_BUILD_DIR"/perl:"$(xcode-select -p)"/Library/Perl/$PERL_VERSION
> 
> ... because what other platforms could possibly have 'xcode-select'
> installed!?  Consequently:
> 
>  $ ./t0000-basic.sh 
>  grep: /usr/local/versioner/perl/versions: No such file or directory
>  ./t0000-basic.sh: 1154: ./test-lib.sh: xcode-select: not found
>  ok 1 - verify that the running shell supports "local"
>  ok 2 - .git/objects should be empty after git init in an empty repo
>  ok 3 - .git/objects should have 3 subdirectories
>  ok 4 - success is reported like this
>  not ok 5 - pretend we have a fully passing test suite
>  <...>
>  # failed 29 among 82 test(s)

Yes.  This is one of the patches that I said in the 00 message would certainly not be upstreamable but for which we should find a general solution to the problem if one is available.

> 
> 
> 
>>>> +export PERL5LIB
>>>> test -d "$GIT_BUILD_DIR"/templates/blt || {
>>>>    error "You haven't built things yet, have you?"
>>>> }
>> 

Re: [PATCH (Apple Git) 02/13] test-lib: Export PERL5LIB for testing git-svn

[Junio C Hamano]

Jeremy Sequoia <jeremyhu@apple.com> writes:

>> On Jan 29, 2019, at 15:59, SZEDER Gábor <szeder.dev@gmail.com> wrote:
>> 
>>> On Tue, Jan 29, 2019 at 03:46:07PM -0800, Jeremy Huddleston Sequoia wrote:
>>> 
>>> 
>>>> On Jan 29, 2019, at 2:47 PM, Junio C Hamano <gitster@pobox.com> wrote:
>>>> 
>>>> This obviously won't be acceptable as-is to my tree.  Shouldn't this
>>>> be something to be dealt with in config.mak.uname or something that
>>>> is meant to define platform-specific customization?
>>> 
>>> The issue here is that we're not locating relocatable perl modules
>>> during testing.  This is a general problem with testing
>>> RUNTIME_PREFIX configurations, and a more general solution to this
>>> sledgehammer would be appropriate.  I don't think config.mak.uname
>>> really makes sense since it's a general RUNTIME_PREFIX issue and not
>>> specifically a darwin issue.
>> 
>> But this patch is very darwin-specific ...
>>  ...
>
> Yes.  This is one of the patches that I said in the 00 message
> would certainly not be upstreamable but for which we should find a
> general solution to the problem if one is available.

Yes, I do appreciate seeing these non-upstreamable ones, as they
serve to illustrate issues that may want to be helped with a bit
more customizability in our tree.  I suspect some of them may
already have enough solution on our side without any need for
further patching (e.g. the "version" one Dscho mentioned for the
06/13), though.

BTW, I'll be mostly offline today, so I'll return to the discussion
tomorrow.

Re: [PATCH (Apple Git) 02/13] test-lib: Export PERL5LIB for testing git-svn

[Carlo Arenas]

/usr/local/versioner/perl/versions is also not provided with macOS or
Xcode AFAIK

Carlo

Re: [PATCH (Apple Git) 02/13] test-lib: Export PERL5LIB for testing git-svn

[Johannes Schindelin]

Hi Jeremy,

On Tue, 29 Jan 2019, Jeremy Huddleston Sequoia wrote:

> > On Jan 29, 2019, at 2:47 PM, Junio C Hamano <gitster@pobox.com> wrote:
> > 
> > Jeremy Huddleston Sequoia <jeremyhu@apple.com> writes:
> > 
> >> Signed-off-by: Jeremy Huddleston Sequoia <jeremyhu@apple.com>
> >> ---
> >> t/test-lib.sh | 3 +++
> >> 1 file changed, 3 insertions(+)
> > 
> > This obviously won't be acceptable as-is to my tree.  Shouldn't this
> > be something to be dealt with in config.mak.uname or something that
> > is meant to define platform-specific customization?
> 
> The issue here is that we're not locating relocatable perl modules
> during testing.  This is a general problem with testing RUNTIME_PREFIX
> configurations, and a more general solution to this sledgehammer would
> be appropriate.  I don't think config.mak.uname really makes sense since
> it's a general RUNTIME_PREFIX issue and not specifically a darwin issue.

First of all, as others have pointed out, this code is very, very specific
to Darwin (not only xcode-select but also Library/Perl/ are very, very
specific to that platform, I would even argue it is not even
Darwin-specific but instead macOS specific because bare-bones Darwin does
not have Library/Perl/, does it?).

So you *definitely* want to put that code into guards testing for that
platform (I do not think config.mak.uname is the correct place, though, as
it should be accessible to test scripts when run directly, i.e. not
through `make`).

But let's take a huge step back first: why? What is the exact problem this
commit tries to solve? The commit message unfortunately does not really
leave me any wiser.

So I am left with the unfortunate position of having to guess, which is
not really a good use of both of our time. If I allow myself to indulge in
the guessing game, I would guess that whatever `perl` executable is used
in your scenario picks up some unfortunate environment variable that
overrides its internal defaults where to look for Perl modules.

And that simply should not be the case. We are very careful to set
GITPERLLIB in bin-wrappers/, *not* PERL5LIB.

And when we build Git on macOS agents in Travis or Azure Pipelines and
then run the test suite, I fail to see any Perl-related error that looks
like it could be solved by this here patch.

In short: this commit is in dear want of a more substantive commit
message, and most likely in search for a different solution.

Ciao,
Johannes

> 
> > 
> >> 
> >> diff --git a/t/test-lib.sh b/t/test-lib.sh
> >> index 0f1faa24b2..4060a53f56 100644
> >> --- a/t/test-lib.sh
> >> +++ b/t/test-lib.sh
> >> @@ -1017,6 +1017,9 @@ fi
> >> 
> >> GITPERLLIB="$GIT_BUILD_DIR"/perl/build/lib
> >> export GITPERLLIB
> >> +PERL_VERSION=$(grep DEFAULT /usr/local/versioner/perl/versions | sed 's:^.*= *\([^ ]*\)$:\1:')
> >> +PERL5LIB="$GIT_BUILD_DIR"/perl:"$(xcode-select -p)"/Library/Perl/$PERL_VERSION
> >> +export PERL5LIB
> >> test -d "$GIT_BUILD_DIR"/templates/blt || {
> >> 	error "You haven't built things yet, have you?"
> >> }
> 
> 

Re: [PATCH (Apple Git) 02/13] test-lib: Export PERL5LIB for testing git-svn

[Jeremy Sequoia]

Sent from my iPhone...

> On Jan 30, 2019, at 04:51, Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
> 
> Hi Jeremy,
> 
> On Tue, 29 Jan 2019, Jeremy Huddleston Sequoia wrote:
> 
>>> On Jan 29, 2019, at 2:47 PM, Junio C Hamano <gitster@pobox.com> wrote:
>>> 
>>> Jeremy Huddleston Sequoia <jeremyhu@apple.com> writes:
>>> 
>>>> Signed-off-by: Jeremy Huddleston Sequoia <jeremyhu@apple.com>
>>>> ---
>>>> t/test-lib.sh | 3 +++
>>>> 1 file changed, 3 insertions(+)
>>> 
>>> This obviously won't be acceptable as-is to my tree.  Shouldn't this
>>> be something to be dealt with in config.mak.uname or something that
>>> is meant to define platform-specific customization?
>> 
>> The issue here is that we're not locating relocatable perl modules
>> during testing.  This is a general problem with testing RUNTIME_PREFIX
>> configurations, and a more general solution to this sledgehammer would
>> be appropriate.  I don't think config.mak.uname really makes sense since
>> it's a general RUNTIME_PREFIX issue and not specifically a darwin issue.
> 
> First of all, as others have pointed out, this code is very, very specific
> to Darwin (not only xcode-select but also Library/Perl/ are very, very
> specific to that platform, I would even argue it is not even
> Darwin-specific but instead macOS specific because bare-bones Darwin does
> not have Library/Perl/, does it?).

Yes.  I first pointed that out in my emails to Peff and in my 00 email ;).  Peff requested that I send  all of our changes (even ones I considered not upstreamable) in order to discuss possible generalized solutions that could apply to others as well.

> So you *definitely* want to put that code into guards testing for that
> platform (I do not think config.mak.uname is the correct place, though, as
> it should be accessible to test scripts when run directly, i.e. not
> through `make`).

It isn’t applicable to anyone outside of Apple internal build engineers (or maybe folks like OpenDarwin building from our OSS perl and python drops too) as it is specific to Apple’s build systems.

However a generalized solution would be useful to others.

> But let's take a huge step back first: why? What is the exact problem this
> commit tries to solve? The commit message unfortunately does not really
> leave me any wiser.
> 
> So I am left with the unfortunate position of having to guess, which is
> not really a good use of both of our time. If I allow myself to indulge in
> the guessing game, I would guess that whatever `perl` executable is used
> in your scenario picks up some unfortunate environment variable that
> overrides its internal defaults where to look for Perl modules.

The issue is with RUNTIME_PREFIX.  git’s RUNTINE_PREFIX support assumes that it is the only thing being relocated.  However, with Xcode, svn and its perl modules are relocated as well.  In order to test git-svn, we need to locate those perl modules.  Patch 10 takes care of this when running from the installed location, but we have no svn in the appropriate relative location from the build directory, so we add the explicit path here.

> And that simply should not be the case. We are very careful to set
> GITPERLLIB in bin-wrappers/, *not* PERL5LIB.
> 
> And when we build Git on macOS agents in Travis or Azure Pipelines and
> then run the test suite, I fail to see any Perl-related error that looks
> like it could be solved by this here patch.
> 
> In short: this commit is in dear want of a more substantive commit
> message, and most likely in search for a different solution.

Yes, a number of these patches (like this one) were requested to be sent to the list in order to spark a discussion for another generalized solution and not to be merged into mainline.

Is there a notation that would help to call that out on the commit?  I figured it was pretty obvious that this was one of those.

> 
> Ciao,
> Johannes
> 
>> 
>>> 
>>>> 
>>>> diff --git a/t/test-lib.sh b/t/test-lib.sh
>>>> index 0f1faa24b2..4060a53f56 100644
>>>> --- a/t/test-lib.sh
>>>> +++ b/t/test-lib.sh
>>>> @@ -1017,6 +1017,9 @@ fi
>>>> 
>>>> GITPERLLIB="$GIT_BUILD_DIR"/perl/build/lib
>>>> export GITPERLLIB
>>>> +PERL_VERSION=$(grep DEFAULT /usr/local/versioner/perl/versions | sed 's:^.*= *\([^ ]*\)$:\1:')
>>>> +PERL5LIB="$GIT_BUILD_DIR"/perl:"$(xcode-select -p)"/Library/Perl/$PERL_VERSION
>>>> +export PERL5LIB
>>>> test -d "$GIT_BUILD_DIR"/templates/blt || {
>>>>    error "You haven't built things yet, have you?"
>>>> }
>> 
>> 

[PATCH (Apple Git) 03/13] t0500: New regression test for git add of a path that contains a .git directory

[Jeremy Huddleston Sequoia]

Signed-off-by: Jeremy Huddleston Sequoia <jeremyhu@apple.com>
---
 t/t0500-apple.sh | 40 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 40 insertions(+)
 create mode 100755 t/t0500-apple.sh

diff --git a/t/t0500-apple.sh b/t/t0500-apple.sh
new file mode 100755
index 0000000000..d5f79237a8
--- /dev/null
+++ b/t/t0500-apple.sh
@@ -0,0 +1,40 @@
+#!/bin/sh
+#
+# Copyright (c) 2012-2016 Apple Inc.
+#
+# Tests for regressions found by Apple Inc. for issues that upstream does not
+# want to fix or accept tests for.
+
+
+test_description='Apple Inc. specific tests'
+
+. ./test-lib.sh
+
+TESTROOT=$(pwd)
+
+# <rdar://problem/10238070>
+#
+# This test case addresses a regression introduced between v1.7.3 and v1.7.5
+# git bisect good v1.7.3
+# git bisect bad v1.7.5
+# ...
+# found 18e051a3981f38db08521bb61ccf7e4571335353
+
+test_expect_success '<rdar://problem/10238070> -- git add of a path that contains a .git directory' '
+	rm -rf .git &&
+	mkdir -p orig/sub/dir/otherdir &&
+	cd orig/sub &&
+	echo "1" > dir/file &&
+	echo "2" > dir/otherdir/file &&
+	git init --quiet &&
+	git add -A &&
+	git commit -m "Initial Commit" --quiet &&
+	cd - > /dev/null &&
+	git init --bare --quiet "${TESTROOT}/git_dir.git" &&
+	git --git-dir="${TESTROOT}/git_dir.git" --work-tree=/ add -f -- "${TESTROOT}/orig/sub/" &&
+	git --git-dir="${TESTROOT}/git_dir.git" --work-tree=/ add -f -- "${TESTROOT}/orig/" &&
+	git --git-dir="${TESTROOT}/git_dir.git" --work-tree=/ commit -m "Commit." |
+		grep -q "2 files changed, 2 insertions"
+'
+
+test_done
-- 
2.20.0 (Apple Git-115)

Re: [PATCH (Apple Git) 03/13] t0500: New regression test for git add of a path that contains a .git directory

[Eric Sunshine]

On Tue, Jan 29, 2019 at 4:19 PM Jeremy Huddleston Sequoia
<jeremyhu@apple.com> wrote:
> Subject: t0500: New regression test for git add of a path that contains a .git directory

Please describe the actual problem here in the commit message so
readers of this change can understand what this is all about.

> Signed-off-by: Jeremy Huddleston Sequoia <jeremyhu@apple.com>
> ---
> diff --git a/t/t0500-apple.sh b/t/t0500-apple.sh
> @@ -0,0 +1,40 @@
> +#!/bin/sh
> +#
> +# Copyright (c) 2012-2016 Apple Inc.
> +#
> +# Tests for regressions found by Apple Inc. for issues that upstream does not
> +# want to fix or accept tests for.

This is an odd comment for a patch which is intended to be upstreamed.

> +test_description='Apple Inc. specific tests'

Is this script actually specific to Apple? If not, a better
description is likely warranted. Alternatively, place this new test in
an appropriate existing test script.

> +# <rdar://problem/10238070>

Inaccessible private bug report. Please describe the actual regression here.

> +# This test case addresses a regression introduced between v1.7.3 and v1.7.5
> +# git bisect good v1.7.3
> +# git bisect bad v1.7.5
> +# ...
> +# found 18e051a3981f38db08521bb61ccf7e4571335353

This commentary isn't very useful going forward, thus not worth having
in the script itself, although it may make useful information for the
commit message (though more likely not). Usually, such commentary
would be placed below the "---" line just under your sign-off.

> +test_expect_success '<rdar://problem/10238070> -- git add of a path that contains a .git directory' '

As above, a better title would be welcome, one which actually means
something to people without access to the private bug report.

> +       rm -rf .git &&
> +       mkdir -p orig/sub/dir/otherdir &&
> +       cd orig/sub &&

We don't 'cd' around inside tests without ensuring that the 'cd' is
undone automatically even if the test fails. (See below.)

> +       echo "1" > dir/file &&
> +       echo "2" > dir/otherdir/file &&
> +       git init --quiet &&

Why --quiet? Output generated by commands is already suppressed by
default when the test is run normally, but it is useful to have when
something goes wrong, so we don't usually want to suppress it
manually. Same comment applies to >/dev/null redirects.

> +       git add -A &&
> +       git commit -m "Initial Commit" --quiet &&
> +       cd - > /dev/null &&

If something fails above this point, then this "cd -" will never
execute, so any tests which get added below this one in the script
will operate in the wrong directory. The normal way to 'cd' within a
test is within a subshell so the 'cd' is undone automatically whether
the test fails or not:

    (
        cd orig/sub
        ...
    )

> +       git init --bare --quiet "${TESTROOT}/git_dir.git" &&
> +       git --git-dir="${TESTROOT}/git_dir.git" --work-tree=/ add -f -- "${TESTROOT}/orig/sub/" &&
> +       git --git-dir="${TESTROOT}/git_dir.git" --work-tree=/ add -f -- "${TESTROOT}/orig/" &&
> +       git --git-dir="${TESTROOT}/git_dir.git" --work-tree=/ commit -m "Commit." |
> +               grep -q "2 files changed, 2 insertions"
> +'

We don't normally place a Git command upstream of a pipe since its
exit status will get swallowed by the pipe, thus potentially losing
important information. Instead, redirect the command output to a file
and 'grep' on the file.

Also, the string you're grepping is likely to be localized, so use
test_i18ngrep() instead.

Re: [PATCH (Apple Git) 03/13] t0500: New regression test for git add of a path that contains a .git directory

[Johannes Schindelin]

Hi Jeremy,

On Tue, 29 Jan 2019, Jeremy Huddleston Sequoia wrote:

> Signed-off-by: Jeremy Huddleston Sequoia <jeremyhu@apple.com>

As Eric pointed out, commits with such a vanishing commit message are
very, very sad commits. And somewhere, a kitten dies every time you submit
such a commit.

> +test_expect_success '<rdar://problem/10238070> -- git add of a path that contains a .git directory' '
> +	rm -rf .git &&
> +	mkdir -p orig/sub/dir/otherdir &&
> +	cd orig/sub &&
> +	echo "1" > dir/file &&
> +	echo "2" > dir/otherdir/file &&
> +	git init --quiet &&
> +	git add -A &&
> +	git commit -m "Initial Commit" --quiet &&
> +	cd - > /dev/null &&
> +	git init --bare --quiet "${TESTROOT}/git_dir.git" &&
> +	git --git-dir="${TESTROOT}/git_dir.git" --work-tree=/ add -f -- "${TESTROOT}/orig/sub/" &&
> +	git --git-dir="${TESTROOT}/git_dir.git" --work-tree=/ add -f -- "${TESTROOT}/orig/" &&
> +	git --git-dir="${TESTROOT}/git_dir.git" --work-tree=/ commit -m "Commit." |
> +		grep -q "2 files changed, 2 insertions"
> +'

Let's try to waste some time and reverse engineer what this test is about,
shall we?

So first the .git directory is removed. I really have to wonder why
because we seem to do pretty much everything after that outside of that
directory, so I bet that the test would do the exact same thing without
mucking with that .git directory.

The some submodule with nested directories is set up (we could do this
much easier by using `mkdir orig && git init orig/sub && test_commit -C
orig/sub 1 && mkdir orig/sub/dir & test_commit -C orig/sub dir/2`, but
let's look further before suggesting a better way to implement this).

Then a bare directory is created *somewhere*, and then the submodule as
well as its parent directory is added.

Finally, a commit is created with that new index.

So is the problem that this test tries to catch that a directory
containing a submodule is added together with its .git directory?

I could understand that, I would understand that you would add a
regression test to catch this, but since it is added with
`test_expect_success`, I would expect this regression to be fixed for a
long time (and probably be committed together with a regression test that
verifies the very same as your new test).

Okay, so I give up on analyzing this further and simply go back to the
indicated commit introducing the regression, and applying your patch on
top, to see whether it fails. Because there is nothing Apple-specific
about it, I'll do this in an Ubuntu VM (because I have no Apple hardware
handy, so the only way for me to debug this on macOS would be via Azure
Pipelines, which is tedious and slow).

But no, this test fails with or without 18e051a3981f (setup: translate
symlinks in filename when using absolute paths, 2010-12-27) reverted.

So the ball is squarely back in your court: care to explain what the
haggling heck your patch is trying to achieve?

Thanks,
Johannes

> +
> +test_done
> -- 
> 2.20.0 (Apple Git-115)
> 
> 

Re: [PATCH (Apple Git) 03/13] t0500: New regression test for git add of a path that contains a .git directory

[Jeremy Huddleston Sequoia]

[-- Attachment #1: Type: text/plain, Size: 926 bytes --]



> On Jan 30, 2019, at 05:12, Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
> 
> Hi Jeremy,
> 
> On Tue, 29 Jan 2019, Jeremy Huddleston Sequoia wrote:
> 
>> Signed-off-by: Jeremy Huddleston Sequoia <jeremyhu@apple.com>
> 
> As Eric pointed out, commits with such a vanishing commit message are
> very, very sad commits. And somewhere, a kitten dies every time you submit
> such a commit.

Yes.  I removed the commit message once upstream git finally fixed the issue and the commit message no longer applied.  I didn't add a new verbose message because I was under the impression the git community did not want to receive any contribuitions (bug reports, feedback, upstreamed patches) from Apple, and the radar provided all the reference I needed.  If that situation is changing (as it seems to possibly be), I'm happy to update this with details about the issue.  Here's the thread from 2011-2014 about the issue.


[-- Attachment #2: Re: [PATCH git] setup: Do not strip trailing _ from paths.eml --]
[-- Type: message/rfc822, Size: 11990 bytes --]

[-- Attachment #2.1.1: Type: text/plain, Size: 5007 bytes --]

FWIW, it seems that this bug was addressed by ddc2a6281595fd24ea01497c496f88c40a59562f

Thanks Martin, now we're no longer carrying around an extra patch for our build of git ;)

--Jeremy

> On Oct 17, 2011, at 14:55, Jeremy Huddleston <jeremyhu@apple.com> wrote:
> 
> ping.  Did you get my response below with extra details?  I just got a duplicate bug report, so it apparently effects people...
> 
> Please let me know if I can be of further assistance.
> 
> On Oct 11, 2011, at 2:17 PM, Jeremy Huddleston wrote:
> 
>> Thanks for your response Junio.  The text of the original bug report is below.
>> 
>> I created a git bisect test script which bisected the problem and found out that the difference was that the trailing / was removed by your code change.  git treats paths with a trailing / differently.  I don't know *why* it treats them differently, but it does.
>> 
>> There's nothing "special" about JustDoItGit.tar.bz2 except that it contains a .git dir and has a file layout that works with the bisect script I wrote.  You can test this yourself by:
>> 
>> mkdir -p ~/tmp/PR-10238070
>> cd ~/tmp/PR-10238070
>> tar xjf JustDoItGit.tar.bz2
>> cd ~/git-checkout
>> /path/to/test_10238070.sh
>> 
>> Here's the original report:
>> 
>> I've tracked the cause of '<rdar://problem/10160992> ##snipped title##' down to a regression in git.
>> 
>> Unzip the attached JustDoItGit.zip project and replace the path in the following commands to the unzipped location on your system:
>> 
>> #delete git in /usr/bin/git
>> sudo rm -r /usr/bin/git
>> #link it to /usr/local/bin/git since that's where ditto will place the new bits
>> sudo ln -s /usr/local/bin/git /usr/bin/git
>> 
>> # first, install git 1.7.3.2 to verify that the bug does not reproduce
>> sudo ditto ~rc/Software/Slate/Roots/Git/Git-14~19.root/ /
>> sudo rm -r /Users/<you>/MyGitRepo.gitdir
>> git --git-dir=/Users/<you>/MyGitRepo.gitdir init --bare --quiet
>> git --git-dir=/Users/<you>/MyGitRepo.gitdir --work-tree=/ add -- /Users/<you>/Desktop/JustDoItGit/ /Users/<you>/Desktop/JustDoItGit/JustDoItGit/JustDoItGitAppDelegate.h /Users/<you>/Desktop/JustDoItGit/JustDoItGitTests
>> git --git-dir=/Users/<you>/MyGitRepo.gitdir --work-tree=/ commit -m "Hello."
>> 
>> The expected result of the commit is something like "18 files changed, 7364 insertions". If that's what you get, great, now keep going.
>> 
>> sudo rm -r /Users/<you>/MyGitRepo.gitdir
>> # install the slate version of git, 1.7.5.4
>> sudo ditto ~rc/Software/Slate/Roots/Git/Git-19.root~2/ /
>> sudo rm -r /Users/<you>/MyGitRepo.gitdir
>> git --git-dir=/Users/<you>/MyGitRepo.gitdir init --bare --quiet
>> git --git-dir=/Users/<you>/MyGitRepo.gitdir --work-tree=/ add -- /Users/<you>/Desktop/JustDoItGit/ /Users/<you>/Desktop/JustDoItGit/JustDoItGit/JustDoItGitAppDelegate.h /Users/<you>/Desktop/JustDoItGit/JustDoItGitTests
>> git --git-dir=/Users/<you>/MyGitRepo.gitdir --work-tree=/ commit -m "Hello."
>> 
>> The expected result is what's above, something like "18 files changed, 7364 insertions". But the actual result is that only the root folder "/Users/<you>/Desktop/JustDoItGit is added
>> 
>> This is a problem because it subsequently causes <rdar://problem/10160992> ##snipped title##
>> 
>> … and therefore breaks Xcode's snapshots feature.
>> 
>> <JustDoItGit.tar.bz2><test_10238070.sh>
>> 
>> On Oct 11, 2011, at 10:45, Junio C Hamano wrote:
>> 
>>> Jeremy Huddleston <jeremyhu@apple.com> writes:
>>> 
>>>> real_path will strip the trailing / from provided paths.  This fixes
>>>> a regression introduced in 18e051a3981f38db08521bb61ccf7e4571335353
>>> 
>>> What is the breakage? The above does not explain why stripping the '/' is
>>> a wrong thing, and which caller that used to work is broken by that
>>> behaviour.
>>> 
>>> A new test block in some of the t/t[0-9]*.sh script to demonstrate the
>>> breakage and fix to explain and justify your fix better, please?
>>> 
>>>> 
>>>> Signed-off-by: Jeremy Huddleston <jeremyhu@apple.com>
>>>> ---
>>>> 
>>>> Here's an updated version that should be a bit more portable and warning-free.
>>>> 
>>>> setup.c |   10 +++++++++-
>>>> 1 files changed, 9 insertions(+), 1 deletions(-)
>>>> 
>>>> diff --git a/setup.c b/setup.c
>>>> index 61c22e6..e3a8ae3 100644
>>>> --- a/setup.c
>>>> +++ b/setup.c
>>>> @@ -10,8 +10,16 @@ char *prefix_path(const char *prefix, int len, const char *path)
>>>> 	char *sanitized;
>>>> 	if (is_absolute_path(orig)) {
>>>> 		const char *temp = real_path(path);
>>>> -		sanitized = xmalloc(len + strlen(temp) + 1);
>>>> +		sanitized = xmalloc(len + strlen(temp) + 2);
>>>> 		strcpy(sanitized, temp);
>>>> +
>>>> +		temp = strrchr(path, '\0');
>>>> +		temp--;
>>>> +		if (*temp == '/') {
>>>> +			char *s = strrchr(sanitized, '\0');
>>>> +			s[0] = '/';
>>>> +			s[1] = '\0';
>>>> +		}
>>>> 	} else {
>>>> 		sanitized = xmalloc(len + strlen(path) + 1);
>>>> 		if (len)
>>> 
>> 
> 


[-- Attachment #2.1.2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 4109 bytes --]

[-- Attachment #3: Type: text/plain, Size: 3249 bytes --]



The problem was related to real_path not stripping the trailing '/' from paths which confused clients of prefix_path().  This caused the behavior used in the script to fail.  The issue was eventually fixed in ddc2a6281595fd24ea01497c496f88c40a59562f, so all that remains is our test for the original issue.  This test script mimics the behavior of the Xcode snapshotting feature that triggered the problem, which is what uncovered the original issue.

>> +test_expect_success '<rdar://problem/10238070> -- git add of a path that contains a .git directory' '
>> +	rm -rf .git &&
>> +	mkdir -p orig/sub/dir/otherdir &&
>> +	cd orig/sub &&
>> +	echo "1" > dir/file &&
>> +	echo "2" > dir/otherdir/file &&
>> +	git init --quiet &&
>> +	git add -A &&
>> +	git commit -m "Initial Commit" --quiet &&
>> +	cd - > /dev/null &&
>> +	git init --bare --quiet "${TESTROOT}/git_dir.git" &&
>> +	git --git-dir="${TESTROOT}/git_dir.git" --work-tree=/ add -f -- "${TESTROOT}/orig/sub/" &&
>> +	git --git-dir="${TESTROOT}/git_dir.git" --work-tree=/ add -f -- "${TESTROOT}/orig/" &&
>> +	git --git-dir="${TESTROOT}/git_dir.git" --work-tree=/ commit -m "Commit." |
>> +		grep -q "2 files changed, 2 insertions"
>> +'
> 
> Let's try to waste some time and reverse engineer what this test is about,
> shall we?
> 
> So first the .git directory is removed. I really have to wonder why
> because we seem to do pretty much everything after that outside of that
> directory, so I bet that the test would do the exact same thing without
> mucking with that .git directory.
> 
> The some submodule with nested directories is set up (we could do this
> much easier by using `mkdir orig && git init orig/sub && test_commit -C
> orig/sub 1 && mkdir orig/sub/dir & test_commit -C orig/sub dir/2`, but
> let's look further before suggesting a better way to implement this).
> 
> Then a bare directory is created *somewhere*, and then the submodule as
> well as its parent directory is added.
> 
> Finally, a commit is created with that new index.
> 
> So is the problem that this test tries to catch that a directory
> containing a submodule is added together with its .git directory?
> 
> I could understand that, I would understand that you would add a
> regression test to catch this, but since it is added with
> `test_expect_success`, I would expect this regression to be fixed for a
> long time (and probably be committed together with a regression test that
> verifies the very same as your new test).
> 
> Okay, so I give up on analyzing this further and simply go back to the
> indicated commit introducing the regression, and applying your patch on
> top, to see whether it fails. Because there is nothing Apple-specific
> about it, I'll do this in an Ubuntu VM (because I have no Apple hardware
> handy, so the only way for me to debug this on macOS would be via Azure
> Pipelines, which is tedious and slow).
> 
> But no, this test fails with or without 18e051a3981f (setup: translate
> symlinks in filename when using absolute paths, 2010-12-27) reverted.
> 
> So the ball is squarely back in your court: care to explain what the
> haggling heck your patch is trying to achieve?
> 
> Thanks,
> Johannes
> 
>> +
>> +test_done
>> -- 
>> 2.20.0 (Apple Git-115)
>> 
>> 

[PATCH (Apple Git) 04/13] t4014: git --version can have SP in it

[Jeremy Huddleston Sequoia]

480871e09e ("format-patch: show base info before email signature",
2016-09-07) added a helper function to recreate the signature at the end
of the e-mail, i.e. "-- " line followed by the version string of Git,
using output from "git --version" and stripping everything before the last
SP.

Because the default Git version string looks like "git version
2.10.0-1-g480871e09e", this was mostly OK, but people can change this
version string to arbitrary thing while compiling, which can break the
assumption if they had SP in it.  Notably, Apple ships modified Git with
" (Apple Git-xx)" appended to its version number.

Instead, come up with the version string by stripping the "git version "
from the beginning.

Regressed-in: 480871e09ed2e5275b4ba16b278681e5a8c122ae
Signed-off-by: Jeremy Huddleston Sequoia <jeremyhu@apple.com>
Reviewed-by: Josh Triplett <josh@joshtriplett.org>
---
 t/t4014-format-patch.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh
index 909c743c13..414c56fcff 100755
--- a/t/t4014-format-patch.sh
+++ b/t/t4014-format-patch.sh
@@ -757,7 +757,7 @@ test_expect_success 'format-patch --ignore-if-in-upstream HEAD' '
 	git format-patch --ignore-if-in-upstream HEAD
 '
 
-git_version="$(git --version | sed "s/.* //")"
+git_version="$(git --version | sed "s/git version //")"
 
 signature() {
 	printf "%s\n%s\n\n" "-- " "${1:-$git_version}"
-- 
2.20.0 (Apple Git-115)

Re: [PATCH (Apple Git) 04/13] t4014: git --version can have SP in it

[Junio C Hamano]

Jeremy Huddleston Sequoia <jeremyhu@apple.com> writes:

> Because the default Git version string looks like "git version
> 2.10.0-1-g480871e09e", this was mostly OK, but people can change this
> version string to arbitrary thing while compiling, which can break the
> assumption if they had SP in it.  Notably, Apple ships modified Git with
> " (Apple Git-xx)" appended to its version number.

I am not sure if that customization is a sensible thing to do in the
first place, but ...

>  
> -git_version="$(git --version | sed "s/.* //")"
> +git_version="$(git --version | sed "s/git version //")"
>  

... this is good, simply because in help.c::cmd_version() we see

        int cmd_version(int argc, const char **argv, const char *prefix)
        {
                ...
                printf("git version %s\n", git_version_string);

i.e. no matter how heavily modified git_version_string[] is, we will
always show "git version" at the beginning (unless a builder goes
one step further to customize the version string by modifying the
source, at which point all bets are off).

To save reviewers and readers from wasting time wondering what
happens when a company, which is even less reasonable than Apple,
modifies the version number to include "git version" in it, the
updated sed expression probably should anchor the pattern to the
left edge to clarify the intention, even though it would not make
any difference in practice, i.e.

	sed 's/^git version //'

Re: [PATCH (Apple Git) 04/13] t4014: git --version can have SP in it

[Johannes Schindelin]

Hi Jeremy,

On Tue, 29 Jan 2019, Jeremy Huddleston Sequoia wrote:

> 480871e09e ("format-patch: show base info before email signature",
> 2016-09-07) added a helper function to recreate the signature at the end
> of the e-mail, i.e. "-- " line followed by the version string of Git,
> using output from "git --version" and stripping everything before the last
> SP.
> 
> Because the default Git version string looks like "git version
> 2.10.0-1-g480871e09e", this was mostly OK, but people can change this
> version string to arbitrary thing while compiling, which can break the
> assumption if they had SP in it.  Notably, Apple ships modified Git with
> " (Apple Git-xx)" appended to its version number.

Here would be a fine place to add Junio's explanation that `git version`
always prefixes "git version " to the `git_version_string` and that the
default signature in `builtin/log.c` is defined as said
`git_version_string`.

> 
> Instead, come up with the version string by stripping the "git version "
> from the beginning.
> 
> Regressed-in: 480871e09ed2e5275b4ba16b278681e5a8c122ae

This is really not a good  way to reference a commit, what with our
intention to switch to SHA-256 at some stage.

Besides, this footer is completely redundant with the information that
starts the very first paragraph.

Ciao,
Johannes

> Signed-off-by: Jeremy Huddleston Sequoia <jeremyhu@apple.com>
> Reviewed-by: Josh Triplett <josh@joshtriplett.org>
> ---
>  t/t4014-format-patch.sh | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh
> index 909c743c13..414c56fcff 100755
> --- a/t/t4014-format-patch.sh
> +++ b/t/t4014-format-patch.sh
> @@ -757,7 +757,7 @@ test_expect_success 'format-patch --ignore-if-in-upstream HEAD' '
>  	git format-patch --ignore-if-in-upstream HEAD
>  '
>  
> -git_version="$(git --version | sed "s/.* //")"
> +git_version="$(git --version | sed "s/git version //")"

As Junio said, this should be anchored. And for extra safety, in case some
even more unreasonable company decides to change the output of `git
--version` itself, it should probably use

	git_version="$(expr "$(git --version)" : "^git version \(.*\)")"

Ciao,
Johannes

>  
>  signature() {
>  	printf "%s\n%s\n\n" "-- " "${1:-$git_version}"
> -- 
> 2.20.0 (Apple Git-115)
> 
> 

[PATCH (Apple Git) 05/13] t5701: git --version can have SP in it

[Jeremy Huddleston Sequoia]

Signed-off-by: Jeremy Huddleston Sequoia <jeremyhu@apple.com>
---
 t/t5701-git-serve.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/t5701-git-serve.sh b/t/t5701-git-serve.sh
index ae79c6bbc0..7bc25700fa 100755
--- a/t/t5701-git-serve.sh
+++ b/t/t5701-git-serve.sh
@@ -7,7 +7,7 @@ test_description='test git-serve and server commands'
 test_expect_success 'test capability advertisement' '
 	cat >expect <<-EOF &&
 	version 2
-	agent=git/$(git version | cut -d" " -f3)
+	agent=git/$(git --version | sed -e "s/git version //" -e "s/ /\./g")
 	ls-refs
 	fetch=shallow
 	server-option
-- 
2.20.0 (Apple Git-115)

Re: [PATCH (Apple Git) 05/13] t5701: git --version can have SP in it

[Johannes Schindelin]

Hi Jeremy,

On Tue, 29 Jan 2019, Jeremy Huddleston Sequoia wrote:

> Signed-off-by: Jeremy Huddleston Sequoia <jeremyhu@apple.com>


That commit message is again very short. Only because I remember the
previous patch's commit message do I have a clue what this is about.

You definitely need to write something here about customized forks of Git
adding suffixes including spaces to the Git version.

And you will need to state where those spaces are converted to dots in
Git's capability advertisement. The reason for this requirement: should
that logic change at any stage in the future, your patch will fail,
somebody will investigate and find this commit and *needs* a helpful
commit message.

> ---
>  t/t5701-git-serve.sh | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/t/t5701-git-serve.sh b/t/t5701-git-serve.sh
> index ae79c6bbc0..7bc25700fa 100755
> --- a/t/t5701-git-serve.sh
> +++ b/t/t5701-git-serve.sh
> @@ -7,7 +7,7 @@ test_description='test git-serve and server commands'
>  test_expect_success 'test capability advertisement' '
>  	cat >expect <<-EOF &&
>  	version 2
> -	agent=git/$(git version | cut -d" " -f3)
> +	agent=git/$(git --version | sed -e "s/git version //" -e "s/ /\./g")

This `git version` needs to be anchored, and it would be much conciser to
use `-e "y/ /./"`, which even BSD sed understands according to
https://pubs.opengroup.org/onlinepubs/9699919799/utilities/sed.html

Ciao,
Johannes

>  	ls-refs
>  	fetch=shallow
>  	server-option
> -- 
> 2.20.0 (Apple Git-115)
> 
> 

Re: [PATCH (Apple Git) 05/13] t5701: git --version can have SP in it

[Jeremy Huddleston Sequoia]

> On Jan 30, 2019, at 05:36, Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
> 
> Hi Jeremy,
> 
> On Tue, 29 Jan 2019, Jeremy Huddleston Sequoia wrote:
> 
>> Signed-off-by: Jeremy Huddleston Sequoia <jeremyhu@apple.com>
> 
> 
> That commit message is again very short. Only because I remember the
> previous patch's commit message do I have a clue what this is about.

Yeah, I became lax with commit messages when I was under the impression that the git community was not interested engaging with Apple.  I should have taken a pass through these commits before sending, but I half expected these patches to be ignored.  I am delighted to see that I was wrong and will go back and provide more details in the ones that I feel are upstreamable like this one.

> You definitely need to write something here about customized forks of Git
> adding suffixes including spaces to the Git version.
> 
> And you will need to state where those spaces are converted to dots in
> Git's capability advertisement. The reason for this requirement: should
> that logic change at any stage in the future, your patch will fail,
> somebody will investigate and find this commit and *needs* a helpful
> commit message.

Thanks, I'll add you to the CC of this patch, so you can critique my wording in the updated message.

> 
>> ---
>> t/t5701-git-serve.sh | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>> 
>> diff --git a/t/t5701-git-serve.sh b/t/t5701-git-serve.sh
>> index ae79c6bbc0..7bc25700fa 100755
>> --- a/t/t5701-git-serve.sh
>> +++ b/t/t5701-git-serve.sh
>> @@ -7,7 +7,7 @@ test_description='test git-serve and server commands'
>> test_expect_success 'test capability advertisement' '
>> 	cat >expect <<-EOF &&
>> 	version 2
>> -	agent=git/$(git version | cut -d" " -f3)
>> +	agent=git/$(git --version | sed -e "s/git version //" -e "s/ /\./g")
> 
> This `git version` needs to be anchored,

Good catch, thanks.

> and it would be much conciser to
> use `-e "y/ /./"`, which even BSD sed understands according to
> https://pubs.opengroup.org/onlinepubs/9699919799/utilities/sed.html

Also done, thanks.

> 
> Ciao,
> Johannes
> 
>> 	ls-refs
>> 	fetch=shallow
>> 	server-option
>> -- 
>> 2.20.0 (Apple Git-115)
>> 
>> 

[PATCH (Apple Git) 06/13] Set Apple Git version during build

[Jeremy Huddleston Sequoia]

Signed-off-by: Jeremy Huddleston Sequoia <jeremyhu@apple.com>
---
 GIT-VERSION-GEN | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/GIT-VERSION-GEN b/GIT-VERSION-GEN
index d1a2814ec7..6fb90854b9 100755
--- a/GIT-VERSION-GEN
+++ b/GIT-VERSION-GEN
@@ -3,6 +3,10 @@
 GVF=GIT-VERSION-FILE
 DEF_VER=v2.20.1
 
+if [ -n "$RC_ProjectSourceVersion" ] ; then
+	DEF_VER="$DEF_VER (Apple Git-$RC_ProjectSourceVersion)"
+fi
+
 LF='
 '
 
-- 
2.20.0 (Apple Git-115)

Re: [PATCH (Apple Git) 06/13] Set Apple Git version during build

[Johannes Schindelin]

Hi Jeremy,

On Tue, 29 Jan 2019, Jeremy Huddleston Sequoia wrote:

> Signed-off-by: Jeremy Huddleston Sequoia <jeremyhu@apple.com>
> ---
>  GIT-VERSION-GEN | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/GIT-VERSION-GEN b/GIT-VERSION-GEN
> index d1a2814ec7..6fb90854b9 100755
> --- a/GIT-VERSION-GEN
> +++ b/GIT-VERSION-GEN
> @@ -3,6 +3,10 @@
>  GVF=GIT-VERSION-FILE
>  DEF_VER=v2.20.1
>  
> +if [ -n "$RC_ProjectSourceVersion" ] ; then
> +	DEF_VER="$DEF_VER (Apple Git-$RC_ProjectSourceVersion)"
> +fi

This seems awfully specific to a very specific setup. It won't work when
building from a Git checkout, either, as `DEF_VER` is not even used then.

And the existing facility is the `version` file. Since you want to build
this in some sort of automated fashion anyway, you should probably execute

	sed -n "s/^DEF_VER=\\(.*\\)/\\1 (Apple Git-$RC_ProjectSourceVersion)/p" \
		<GIT-VERSION-GEN >version

in your automation script. As a bonus, this will work with any unpatched
Git source code, too!

Ciao,
Johannes

> +
>  LF='
>  '
>  
> -- 
> 2.20.0 (Apple Git-115)
> 
> 

Re: [PATCH (Apple Git) 06/13] Set Apple Git version during build

[Jeremy Huddleston Sequoia]

> On Jan 30, 2019, at 05:43, Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
> 
> Hi Jeremy,
> 
> On Tue, 29 Jan 2019, Jeremy Huddleston Sequoia wrote:
> 
>> Signed-off-by: Jeremy Huddleston Sequoia <jeremyhu@apple.com>
>> ---
>> GIT-VERSION-GEN | 4 ++++
>> 1 file changed, 4 insertions(+)
>> 
>> diff --git a/GIT-VERSION-GEN b/GIT-VERSION-GEN
>> index d1a2814ec7..6fb90854b9 100755
>> --- a/GIT-VERSION-GEN
>> +++ b/GIT-VERSION-GEN
>> @@ -3,6 +3,10 @@
>> GVF=GIT-VERSION-FILE
>> DEF_VER=v2.20.1
>> 
>> +if [ -n "$RC_ProjectSourceVersion" ] ; then
>> +	DEF_VER="$DEF_VER (Apple Git-$RC_ProjectSourceVersion)"
>> +fi
> 
> This seems awfully specific to a very specific setup. It won't work when
> building from a Git checkout, either, as `DEF_VER` is not even used then.

Yes.  This one was one of the ones I thought obviously fell into the "this is certainly not upstreamable, but let's discuss a general solution to this problem" category.  Sorry if that wasn't clear.

> And the existing facility is the `version` file. Since you want to build
> this in some sort of automated fashion anyway, you should probably execute
> 
> 	sed -n "s/^DEF_VER=\\(.*\\)/\\1 (Apple Git-$RC_ProjectSourceVersion)/p" \
> 		<GIT-VERSION-GEN >version
> 
> in your automation script. As a bonus, this will work with any unpatched
> Git source code, too!

Oh nice!  Thanks for getting rid of one of our patches.

> 
> Ciao,
> Johannes
> 
>> +
>> LF='
>> '
>> 
>> -- 
>> 2.20.0 (Apple Git-115)
>> 
>> 

[PATCH (Apple Git) 07/13] HTML documentation is not provided with Apple's git. Make the error message more on point.

[Jeremy Huddleston Sequoia]

Signed-off-by: Jeremy Huddleston Sequoia <jeremyhu@apple.com>
---
 builtin/help.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/builtin/help.c b/builtin/help.c
index 7739a5c155..e001b6157c 100644
--- a/builtin/help.c
+++ b/builtin/help.c
@@ -383,7 +383,7 @@ static void get_html_page_path(struct strbuf *page_path, const char *page)
 	if (!strstr(html_path, "://")) {
 		if (stat(mkpath("%s/git.html", html_path), &st)
 		    || !S_ISREG(st.st_mode))
-			die("'%s': not a documentation directory.", html_path);
+			die("HTML documentation is not provided by this distribution of git.");
 	}
 
 	strbuf_init(page_path, 0);
-- 
2.20.0 (Apple Git-115)

Re: [PATCH (Apple Git) 07/13] HTML documentation is not provided with Apple's git. Make the error message more on point.

[Junio C Hamano]

Jeremy Huddleston Sequoia <jeremyhu@apple.com> writes:

> Signed-off-by: Jeremy Huddleston Sequoia <jeremyhu@apple.com>
> ---
>  builtin/help.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/builtin/help.c b/builtin/help.c
> index 7739a5c155..e001b6157c 100644
> --- a/builtin/help.c
> +++ b/builtin/help.c
> @@ -383,7 +383,7 @@ static void get_html_page_path(struct strbuf *page_path, const char *page)
>  	if (!strstr(html_path, "://")) {
>  		if (stat(mkpath("%s/git.html", html_path), &st)
>  		    || !S_ISREG(st.st_mode))
> -			die("'%s': not a documentation directory.", html_path);
> +			die("HTML documentation is not provided by this distribution of git.");

Mentioning HTML in the message may be a good idea, but I feel that
"distribution of git" is not something we should say in the source
for those who are building from the source.  Distributors are free
to munge before they generate their binary distribution, of course
;-).

>  	}
>  
>  	strbuf_init(page_path, 0);

Re: [PATCH (Apple Git) 07/13] HTML documentation is not provided with Apple's git. Make the error message more on point.

[Johannes Schindelin]

Hi Junio,

On Tue, 29 Jan 2019, Junio C Hamano wrote:

> Jeremy Huddleston Sequoia <jeremyhu@apple.com> writes:
> 
> > Signed-off-by: Jeremy Huddleston Sequoia <jeremyhu@apple.com>
> > ---
> >  builtin/help.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/builtin/help.c b/builtin/help.c
> > index 7739a5c155..e001b6157c 100644
> > --- a/builtin/help.c
> > +++ b/builtin/help.c
> > @@ -383,7 +383,7 @@ static void get_html_page_path(struct strbuf *page_path, const char *page)
> >  	if (!strstr(html_path, "://")) {
> >  		if (stat(mkpath("%s/git.html", html_path), &st)
> >  		    || !S_ISREG(st.st_mode))
> > -			die("'%s': not a documentation directory.", html_path);
> > +			die("HTML documentation is not provided by this distribution of git.");
> 
> Mentioning HTML in the message may be a good idea, but I feel that
> "distribution of git" is not something we should say in the source
> for those who are building from the source.  Distributors are free
> to munge before they generate their binary distribution, of course
> ;-).

So maybe something like

#ifdef MISSING_HTML_MESSAGE
			die(_(MISSING_HTML_MESSAGE));
#else
			die("'%s': not a documentation directory.", html_path);
#endif

?

Ciao,
Johannes

> 
> >  	}
> >  
> >  	strbuf_init(page_path, 0);
> 

Re: [PATCH (Apple Git) 07/13] HTML documentation is not provided with Apple's git. Make the error message more on point.

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> Hi Junio,
>
> On Tue, 29 Jan 2019, Junio C Hamano wrote:
>
>> Jeremy Huddleston Sequoia <jeremyhu@apple.com> writes:
>> 
>> > Signed-off-by: Jeremy Huddleston Sequoia <jeremyhu@apple.com>
>> > ---
>> >  builtin/help.c | 2 +-
>> >  1 file changed, 1 insertion(+), 1 deletion(-)
>> >
>> > diff --git a/builtin/help.c b/builtin/help.c
>> > index 7739a5c155..e001b6157c 100644
>> > --- a/builtin/help.c
>> > +++ b/builtin/help.c
>> > @@ -383,7 +383,7 @@ static void get_html_page_path(struct strbuf *page_path, const char *page)
>> >  	if (!strstr(html_path, "://")) {
>> >  		if (stat(mkpath("%s/git.html", html_path), &st)
>> >  		    || !S_ISREG(st.st_mode))
>> > -			die("'%s': not a documentation directory.", html_path);
>> > +			die("HTML documentation is not provided by this distribution of git.");
>> 
>> Mentioning HTML in the message may be a good idea, but I feel that
>> "distribution of git" is not something we should say in the source
>> for those who are building from the source.  Distributors are free
>> to munge before they generate their binary distribution, of course
>> ;-).
>
> So maybe something like
>
> #ifdef MISSING_HTML_MESSAGE
> 			die(_(MISSING_HTML_MESSAGE));
> #else
> 			die("'%s': not a documentation directory.", html_path);
> #endif
>
> ?

No, distributors can fork and build from patched source.  What I
meant was along these lines:

    die(_("HTML documentation not installed in '%s'."), html_path));
    die(_("The installer chose to omit HTML docs from '%s''.", html_path));

Re: [PATCH (Apple Git) 07/13] HTML documentation is not provided with Apple's git. Make the error message more on point.

[Johannes Schindelin]

Hi Junio,

On Wed, 30 Jan 2019, Junio C Hamano wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> > On Tue, 29 Jan 2019, Junio C Hamano wrote:
> >
> >> Jeremy Huddleston Sequoia <jeremyhu@apple.com> writes:
> >> 
> >> > Signed-off-by: Jeremy Huddleston Sequoia <jeremyhu@apple.com>
> >> > ---
> >> >  builtin/help.c | 2 +-
> >> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >> >
> >> > diff --git a/builtin/help.c b/builtin/help.c
> >> > index 7739a5c155..e001b6157c 100644
> >> > --- a/builtin/help.c
> >> > +++ b/builtin/help.c
> >> > @@ -383,7 +383,7 @@ static void get_html_page_path(struct strbuf *page_path, const char *page)
> >> >  	if (!strstr(html_path, "://")) {
> >> >  		if (stat(mkpath("%s/git.html", html_path), &st)
> >> >  		    || !S_ISREG(st.st_mode))
> >> > -			die("'%s': not a documentation directory.", html_path);
> >> > +			die("HTML documentation is not provided by this distribution of git.");
> >> 
> >> Mentioning HTML in the message may be a good idea, but I feel that
> >> "distribution of git" is not something we should say in the source
> >> for those who are building from the source.  Distributors are free
> >> to munge before they generate their binary distribution, of course
> >> ;-).
> >
> > So maybe something like
> >
> > #ifdef MISSING_HTML_MESSAGE
> > 			die(_(MISSING_HTML_MESSAGE));
> > #else
> > 			die("'%s': not a documentation directory.", html_path);
> > #endif
> >
> > ?
> 
> No, distributors can fork and build from patched source.  What I
> meant was along these lines:
> 
>     die(_("HTML documentation not installed in '%s'."), html_path));
>     die(_("The installer chose to omit HTML docs from '%s''.", html_path));

Thanks for the clarification. I think your rationale makes a total lot of
sense.

Thank you,
Dscho

[PATCH (Apple Git) 08/13] git mergetool/difftool doesn't list 'opendiff' as an available tool on 10.8

[Jeremy Huddleston Sequoia]

See <rdar://problem/12652310>

Signed-off-by: Jeremy Huddleston Sequoia <jeremyhu@apple.com>
---
 git-mergetool--lib.sh | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh
index 83bf52494c..f85be7406f 100644
--- a/git-mergetool--lib.sh
+++ b/git-mergetool--lib.sh
@@ -273,9 +273,9 @@ list_merge_tool_candidates () {
 	then
 		if test -n "$GNOME_DESKTOP_SESSION_ID"
 		then
-			tools="meld opendiff kdiff3 tkdiff xxdiff $tools"
+			tools="meld kdiff3 tkdiff xxdiff $tools"
 		else
-			tools="opendiff kdiff3 tkdiff xxdiff meld $tools"
+			tools="kdiff3 tkdiff xxdiff meld $tools"
 		fi
 		tools="$tools gvimdiff diffuse diffmerge ecmerge"
 		tools="$tools p4merge araxis bc codecompare"
@@ -288,6 +288,8 @@ list_merge_tool_candidates () {
 		tools="$tools emerge vimdiff"
 		;;
 	esac
+
+	tools="opendiff $tools"
 }
 
 show_tool_help () {
-- 
2.20.0 (Apple Git-115)

Re: [PATCH (Apple Git) 08/13] git mergetool/difftool doesn't list 'opendiff' as an available tool on 10.8

[Johannes Schindelin]

Hi Jeremy,

On Tue, 29 Jan 2019, Jeremy Huddleston Sequoia wrote:

> See <rdar://problem/12652310>
> 
> Signed-off-by: Jeremy Huddleston Sequoia <jeremyhu@apple.com>

You know yourself how to improve this commit message rather dramatically.

> ---
>  git-mergetool--lib.sh | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh
> index 83bf52494c..f85be7406f 100644
> --- a/git-mergetool--lib.sh
> +++ b/git-mergetool--lib.sh
> @@ -273,9 +273,9 @@ list_merge_tool_candidates () {
>  	then
>  		if test -n "$GNOME_DESKTOP_SESSION_ID"
>  		then
> -			tools="meld opendiff kdiff3 tkdiff xxdiff $tools"
> +			tools="meld kdiff3 tkdiff xxdiff $tools"
>  		else
> -			tools="opendiff kdiff3 tkdiff xxdiff meld $tools"
> +			tools="kdiff3 tkdiff xxdiff meld $tools"
>  		fi
>  		tools="$tools gvimdiff diffuse diffmerge ecmerge"
>  		tools="$tools p4merge araxis bc codecompare"
> @@ -288,6 +288,8 @@ list_merge_tool_candidates () {
>  		tools="$tools emerge vimdiff"
>  		;;
>  	esac
> +
> +	tools="opendiff $tools"

That is a hack, not a solution.

A much better idea would be to leave the `DISPLAY` block alone and to add
an `elif test -x /path/to/known/location` block.

Ciao,
Johannes

>  }
>  
>  show_tool_help () {
> -- 
> 2.20.0 (Apple Git-115)
> 
> 

[PATCH (Apple Git) 09/13] Use symbolic links rather than hard links for files in libexec

[Jeremy Huddleston Sequoia]

See <rdar://problem/10573201>

Signed-off-by: Jeremy Huddleston Sequoia <jeremyhu@apple.com>
---
 Makefile | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/Makefile b/Makefile
index 1a44c811aa..60711d6abe 100644
--- a/Makefile
+++ b/Makefile
@@ -2065,10 +2065,7 @@ version.sp version.s version.o: EXTRA_CPPFLAGS = \
 		git rev-parse -q --verify HEAD 2>/dev/null)"'
 
 $(BUILT_INS): git$X
-	$(QUIET_BUILT_IN)$(RM) $@ && \
-	ln $< $@ 2>/dev/null || \
-	ln -s $< $@ 2>/dev/null || \
-	cp $< $@
+	$(QUIET_BUILT_IN)ln -fs $< $@
 
 command-list.h: generate-cmdlist.sh command-list.txt
 
@@ -2387,7 +2384,6 @@ git-remote-testsvn$X: remote-testsvn.o GIT-LDFLAGS $(GITLIBS) $(VCSSVN_LIB)
 
 $(REMOTE_CURL_ALIASES): $(REMOTE_CURL_PRIMARY)
 	$(QUIET_LNCP)$(RM) $@ && \
-	ln $< $@ 2>/dev/null || \
 	ln -s $< $@ 2>/dev/null || \
 	cp $< $@
 
-- 
2.20.0 (Apple Git-115)

Re: [PATCH (Apple Git) 09/13] Use symbolic links rather than hard links for files in libexec

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 373 bytes --]

On Tue, Jan 29, 2019 at 11:38:15AM -0800, Jeremy Huddleston Sequoia wrote:
> See <rdar://problem/10573201>

It's my understanding that Radars aren't public. Could you summarize the
reasons behind this change in the commit message for those of us who
don't have access to view this issue?
-- 
brian m. carlson: Houston, Texas, US
OpenPGP: https://keybase.io/bk2204

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 868 bytes --]

Re: [PATCH (Apple Git) 09/13] Use symbolic links rather than hard links for files in libexec

[Jeremy Huddleston Sequoia]

> On Jan 30, 2019, at 01:50, brian m. carlson <sandals@crustytoothpaste.net> wrote:
> 
> On Tue, Jan 29, 2019 at 11:38:15AM -0800, Jeremy Huddleston Sequoia wrote:
>> See <rdar://problem/10573201>
> 
> It's my understanding that Radars aren't public. Could you summarize the
> reasons behind this change in the commit message for those of us who
> don't have access to view this issue?

There was a bug in some tool in our packaging pipeline that resulted in hardlinks not being preserved.  That was fixed, but I decided to leave these as symlinks anyways in case users did a file operation on Xcode.app that didn't preserve hard links.

The point here is that it would probably be nice to have hard vs soft be a configuration option.

Re: [PATCH (Apple Git) 09/13] Use symbolic links rather than hard links for files in libexec

[Johannes Schindelin]

[-- Attachment #1: Type: text/plain, Size: 1239 bytes --]

Hi Jeremy,

On Wed, 30 Jan 2019, Jeremy Huddleston Sequoia wrote:

> > On Jan 30, 2019, at 01:50, brian m. carlson
> > <sandals@crustytoothpaste.net> wrote:
> > 
> > On Tue, Jan 29, 2019 at 11:38:15AM -0800, Jeremy Huddleston Sequoia wrote:
> >> See <rdar://problem/10573201>
> > 
> > It's my understanding that Radars aren't public. Could you summarize the
> > reasons behind this change in the commit message for those of us who
> > don't have access to view this issue?
> 
> There was a bug in some tool in our packaging pipeline that resulted in
> hardlinks not being preserved.  That was fixed, but I decided to leave
> these as symlinks anyways in case users did a file operation on
> Xcode.app that didn't preserve hard links.
> 
> The point here is that it would probably be nice to have hard vs soft be
> a configuration option.

Your patch does not make it a configuration option. (Or a build option,
which would probably be the more appropriate thing to do here.)

You need not spend the time on this, though, as Ævar already did, in
ad874608d8c9 (Makefile: optionally symlink libexec/git-core binaries to
bin/git, 2018-03-13), which made it in v2.18.0 already. All you need to do
is to define INSTALL_SYMLINKS.

Ciao,
Johannes

Re: [PATCH (Apple Git) 09/13] Use symbolic links rather than hard links for files in libexec

[Jeremy Huddleston Sequoia]

> On Jan 30, 2019, at 11:15, Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
> 
> Hi Jeremy,
> 
> On Wed, 30 Jan 2019, Jeremy Huddleston Sequoia wrote:
> 
>>> On Jan 30, 2019, at 01:50, brian m. carlson
>>> <sandals@crustytoothpaste.net> wrote:
>>> 
>>> On Tue, Jan 29, 2019 at 11:38:15AM -0800, Jeremy Huddleston Sequoia wrote:
>>>> See <rdar://problem/10573201>
>>> 
>>> It's my understanding that Radars aren't public. Could you summarize the
>>> reasons behind this change in the commit message for those of us who
>>> don't have access to view this issue?
>> 
>> There was a bug in some tool in our packaging pipeline that resulted in
>> hardlinks not being preserved.  That was fixed, but I decided to leave
>> these as symlinks anyways in case users did a file operation on
>> Xcode.app that didn't preserve hard links.
>> 
>> The point here is that it would probably be nice to have hard vs soft be
>> a configuration option.
> 
> Your patch does not make it a configuration option. (Or a build option,
> which would probably be the more appropriate thing to do here.)
> 
> You need not spend the time on this, though, as Ævar already did, in
> ad874608d8c9 (Makefile: optionally symlink libexec/git-core binaries to
> bin/git, 2018-03-13), which made it in v2.18.0 already. All you need to do
> is to define INSTALL_SYMLINKS.

Oh great, thanks!  I missed that when doing the last rebase.

[PATCH (Apple Git) 10/13] Support for Xcode.app co-exestince and relocation

[Jeremy Huddleston Sequoia]

This patch has been trimmed down significantly from its original version
when rebasing on top of git 2.18 because git 2.18 included support for
runtime prefix support for darwin, making this patch mostly duplicative.

The remaining changes are needed to ensure that git-perl can find the
subversion perl module (which relocates with it) and handle relocation
of python scripts.

Signed-off-by: Jeremy Huddleston Sequoia <jeremyhu@apple.com>
---
 Makefile                                      |  3 +++
 .../runtime_prefix.template.pl                | 25 +++++++++++++++++++
 2 files changed, 28 insertions(+)

diff --git a/Makefile b/Makefile
index 60711d6abe..97f46444f5 100644
--- a/Makefile
+++ b/Makefile
@@ -2171,6 +2171,7 @@ GIT-PERL-HEADER: $(PERL_HEADER_TEMPLATE) GIT-PERL-DEFINES Makefile
 	sed -e 's=@@PATHSEP@@=$(pathsep)=g' \
 	    -e "s=@@INSTLIBDIR@@=$$INSTLIBDIR=g" \
 	    -e 's=@@PERLLIBDIR_REL@@=$(perllibdir_relative_SQ)=g' \
+	    -e 's=@@PERLVERSION@@=$(shell grep DEFAULT /usr/local/versioner/perl/versions | sed 's:^.*= *\([^ ]*\)$$:\1:')=g' \
 	    -e 's=@@GITEXECDIR_REL@@=$(gitexecdir_relative_SQ)=g' \
 	    -e 's=@@LOCALEDIR_REL@@=$(localedir_relative_SQ)=g' \
 	    $< >$@+ && \
@@ -2206,6 +2207,8 @@ $(SCRIPT_PYTHON_GEN): GIT-CFLAGS GIT-PREFIX GIT-PYTHON-VARS
 $(SCRIPT_PYTHON_GEN): % : %.py
 	$(QUIET_GEN)$(RM) $@ $@+ && \
 	sed -e '1s|#!.*python|#!$(PYTHON_PATH_SQ)|' \
+	    -e 's|\(os\.getenv("GITPYTHONLIB"\)[^)]*)|\1,"@@INSTLIBDIR@@")|' \
+	    -e 's|"@@INSTLIBDIR@@"|os.path.realpath(os.path.dirname(sys.argv[0])) + "/../../share/git-core/python"|g' \
 	    $< >$@+ && \
 	chmod +x $@+ && \
 	mv $@+ $@
diff --git a/perl/header_templates/runtime_prefix.template.pl b/perl/header_templates/runtime_prefix.template.pl
index 9d28b3d863..b0b6b0bef1 100644
--- a/perl/header_templates/runtime_prefix.template.pl
+++ b/perl/header_templates/runtime_prefix.template.pl
@@ -1,3 +1,28 @@
+# BEGIN XCODE RUNTIME_PREFIX generated code
+BEGIN {
+    use File::Spec;
+    my $PERLVERSION = "@@PERLVERSION@@";
+    if ($^V =~ m/v([0-9]+).([0-9]+)/) {
+        $PERLVERSION = $1.".".$2;
+    }
+    my $__prefix = File::Spec->rel2abs( __FILE__ );
+
+    if ($__prefix =~ m/\/libexec\/git-core\// ) {
+        $__prefix =~ s/\/libexec\/git-core\/.*//;
+        unshift @INC, $__prefix . "/share/git-core/perl";
+        unshift @INC, $__prefix . "/../Library/Perl/".$PERLVERSION."/darwin-thread-multi-2level";
+    } elsif ($__prefix =~ m/\/bin\// ) {
+        $__prefix =~ s/\/bin\/.*//;
+        unshift @INC, $__prefix . "/share/git-core/perl";
+        unshift @INC, $__prefix . "/../Library/Perl/".$PERLVERSION."/darwin-thread-multi-2level";
+    } elsif ( $__prefix =~ m/\/usr\// ) {
+        $__prefix =~ s/\/usr\/.*/\/usr/;
+        unshift @INC, $__prefix . "/share/git-core/perl";
+        unshift @INC, $__prefix . "/../Library/Perl/".$PERLVERSION."/darwin-thread-multi-2level";
+    }
+}
+# END XCODE RUNTIME_PREFIX generated code.
+
 # BEGIN RUNTIME_PREFIX generated code.
 #
 # This finds our Git::* libraries relative to the script's runtime path.
-- 
2.20.0 (Apple Git-115)

Re: [PATCH (Apple Git) 10/13] Support for Xcode.app co-exestince and relocation

[Johannes Schindelin]

Hi Jeremy,

On Tue, 29 Jan 2019, Jeremy Huddleston Sequoia wrote:

> This patch has been trimmed down significantly from its original version
> when rebasing on top of git 2.18 because git 2.18 included support for
> runtime prefix support for darwin, making this patch mostly duplicative.
> 
> The remaining changes are needed to ensure that git-perl can find the
> subversion perl module (which relocates with it) and handle relocation
> of python scripts.
> 
> Signed-off-by: Jeremy Huddleston Sequoia <jeremyhu@apple.com>

I am really curious about this kind of problem now. Is it that you want to
bundle a Perl inside a portable Git, and that Perl is not at all
relocatable and so you force Git to pretend that it is?

Ciao,
Johannes

> ---
>  Makefile                                      |  3 +++
>  .../runtime_prefix.template.pl                | 25 +++++++++++++++++++
>  2 files changed, 28 insertions(+)
> 
> diff --git a/Makefile b/Makefile
> index 60711d6abe..97f46444f5 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -2171,6 +2171,7 @@ GIT-PERL-HEADER: $(PERL_HEADER_TEMPLATE) GIT-PERL-DEFINES Makefile
>  	sed -e 's=@@PATHSEP@@=$(pathsep)=g' \
>  	    -e "s=@@INSTLIBDIR@@=$$INSTLIBDIR=g" \
>  	    -e 's=@@PERLLIBDIR_REL@@=$(perllibdir_relative_SQ)=g' \
> +	    -e 's=@@PERLVERSION@@=$(shell grep DEFAULT /usr/local/versioner/perl/versions | sed 's:^.*= *\([^ ]*\)$$:\1:')=g' \
>  	    -e 's=@@GITEXECDIR_REL@@=$(gitexecdir_relative_SQ)=g' \
>  	    -e 's=@@LOCALEDIR_REL@@=$(localedir_relative_SQ)=g' \
>  	    $< >$@+ && \
> @@ -2206,6 +2207,8 @@ $(SCRIPT_PYTHON_GEN): GIT-CFLAGS GIT-PREFIX GIT-PYTHON-VARS
>  $(SCRIPT_PYTHON_GEN): % : %.py
>  	$(QUIET_GEN)$(RM) $@ $@+ && \
>  	sed -e '1s|#!.*python|#!$(PYTHON_PATH_SQ)|' \
> +	    -e 's|\(os\.getenv("GITPYTHONLIB"\)[^)]*)|\1,"@@INSTLIBDIR@@")|' \
> +	    -e 's|"@@INSTLIBDIR@@"|os.path.realpath(os.path.dirname(sys.argv[0])) + "/../../share/git-core/python"|g' \
>  	    $< >$@+ && \
>  	chmod +x $@+ && \
>  	mv $@+ $@
> diff --git a/perl/header_templates/runtime_prefix.template.pl b/perl/header_templates/runtime_prefix.template.pl
> index 9d28b3d863..b0b6b0bef1 100644
> --- a/perl/header_templates/runtime_prefix.template.pl
> +++ b/perl/header_templates/runtime_prefix.template.pl
> @@ -1,3 +1,28 @@
> +# BEGIN XCODE RUNTIME_PREFIX generated code
> +BEGIN {
> +    use File::Spec;
> +    my $PERLVERSION = "@@PERLVERSION@@";
> +    if ($^V =~ m/v([0-9]+).([0-9]+)/) {
> +        $PERLVERSION = $1.".".$2;
> +    }
> +    my $__prefix = File::Spec->rel2abs( __FILE__ );
> +
> +    if ($__prefix =~ m/\/libexec\/git-core\// ) {
> +        $__prefix =~ s/\/libexec\/git-core\/.*//;
> +        unshift @INC, $__prefix . "/share/git-core/perl";
> +        unshift @INC, $__prefix . "/../Library/Perl/".$PERLVERSION."/darwin-thread-multi-2level";
> +    } elsif ($__prefix =~ m/\/bin\// ) {
> +        $__prefix =~ s/\/bin\/.*//;
> +        unshift @INC, $__prefix . "/share/git-core/perl";
> +        unshift @INC, $__prefix . "/../Library/Perl/".$PERLVERSION."/darwin-thread-multi-2level";
> +    } elsif ( $__prefix =~ m/\/usr\// ) {
> +        $__prefix =~ s/\/usr\/.*/\/usr/;
> +        unshift @INC, $__prefix . "/share/git-core/perl";
> +        unshift @INC, $__prefix . "/../Library/Perl/".$PERLVERSION."/darwin-thread-multi-2level";
> +    }
> +}
> +# END XCODE RUNTIME_PREFIX generated code.
> +
>  # BEGIN RUNTIME_PREFIX generated code.
>  #
>  # This finds our Git::* libraries relative to the script's runtime path.
> -- 
> 2.20.0 (Apple Git-115)
> 
> 

Re: [PATCH (Apple Git) 10/13] Support for Xcode.app co-exestince and relocation

[Jeremy Huddleston Sequoia]

> On Jan 30, 2019, at 11:26, Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
> 
> Hi Jeremy,
> 
> On Tue, 29 Jan 2019, Jeremy Huddleston Sequoia wrote:
> 
>> This patch has been trimmed down significantly from its original version
>> when rebasing on top of git 2.18 because git 2.18 included support for
>> runtime prefix support for darwin, making this patch mostly duplicative.
>> 
>> The remaining changes are needed to ensure that git-perl can find the
>> subversion perl module (which relocates with it) and handle relocation
>> of python scripts.
>> 
>> Signed-off-by: Jeremy Huddleston Sequoia <jeremyhu@apple.com>
> 
> I am really curious about this kind of problem now. Is it that you want to
> bundle a Perl inside a portable Git, and that Perl is not at all
> relocatable and so you force Git to pretend that it is?

Not exactly, although that's an interesting problem as well.

In our specific case, Xcode.app is a relocatable bundle that contains the Xcode application, SDKs, toolchains, useful command line tools including git and svn, and more.  perl is distributed with the system in /usr/bin/perl and not with Xcode.

git-svn uses requires the perl modules that ship with subversion.  Since subversion ships with Xcode, those perl modules are packaged inside of Xcode, and the system perl has no logic to locate modules in various apps on the system as that would be an abstraction violation, IMO.

This patch changes the perl template to allow git-svn (or any other perl based tool installed by git) to locate these subversion perl modules within Xcode.app.  It requires the perl script itself to have been executed from within Xcode.app, which is why this complicates testing out of the build directory as discussed in patch 2 in the series.

The /usr/local/versioner/perl/versions logic there is specific to our build infrastructure, but if you're interested in taking something like this, I could come up with a better way to generalize it.

The issues for python are similar to for perl, but I forget the specifics around what exactly was failing in the python case.

> Ciao,
> Johannes
> 
>> ---
>> Makefile                                      |  3 +++
>> .../runtime_prefix.template.pl                | 25 +++++++++++++++++++
>> 2 files changed, 28 insertions(+)
>> 
>> diff --git a/Makefile b/Makefile
>> index 60711d6abe..97f46444f5 100644
>> --- a/Makefile
>> +++ b/Makefile
>> @@ -2171,6 +2171,7 @@ GIT-PERL-HEADER: $(PERL_HEADER_TEMPLATE) GIT-PERL-DEFINES Makefile
>> 	sed -e 's=@@PATHSEP@@=$(pathsep)=g' \
>> 	    -e "s=@@INSTLIBDIR@@=$$INSTLIBDIR=g" \
>> 	    -e 's=@@PERLLIBDIR_REL@@=$(perllibdir_relative_SQ)=g' \
>> +	    -e 's=@@PERLVERSION@@=$(shell grep DEFAULT /usr/local/versioner/perl/versions | sed 's:^.*= *\([^ ]*\)$$:\1:')=g' \
>> 	    -e 's=@@GITEXECDIR_REL@@=$(gitexecdir_relative_SQ)=g' \
>> 	    -e 's=@@LOCALEDIR_REL@@=$(localedir_relative_SQ)=g' \
>> 	    $< >$@+ && \
>> @@ -2206,6 +2207,8 @@ $(SCRIPT_PYTHON_GEN): GIT-CFLAGS GIT-PREFIX GIT-PYTHON-VARS
>> $(SCRIPT_PYTHON_GEN): % : %.py
>> 	$(QUIET_GEN)$(RM) $@ $@+ && \
>> 	sed -e '1s|#!.*python|#!$(PYTHON_PATH_SQ)|' \
>> +	    -e 's|\(os\.getenv("GITPYTHONLIB"\)[^)]*)|\1,"@@INSTLIBDIR@@")|' \
>> +	    -e 's|"@@INSTLIBDIR@@"|os.path.realpath(os.path.dirname(sys.argv[0])) + "/../../share/git-core/python"|g' \
>> 	    $< >$@+ && \
>> 	chmod +x $@+ && \
>> 	mv $@+ $@
>> diff --git a/perl/header_templates/runtime_prefix.template.pl b/perl/header_templates/runtime_prefix.template.pl
>> index 9d28b3d863..b0b6b0bef1 100644
>> --- a/perl/header_templates/runtime_prefix.template.pl
>> +++ b/perl/header_templates/runtime_prefix.template.pl
>> @@ -1,3 +1,28 @@
>> +# BEGIN XCODE RUNTIME_PREFIX generated code
>> +BEGIN {
>> +    use File::Spec;
>> +    my $PERLVERSION = "@@PERLVERSION@@";
>> +    if ($^V =~ m/v([0-9]+).([0-9]+)/) {
>> +        $PERLVERSION = $1.".".$2;
>> +    }
>> +    my $__prefix = File::Spec->rel2abs( __FILE__ );
>> +
>> +    if ($__prefix =~ m/\/libexec\/git-core\// ) {
>> +        $__prefix =~ s/\/libexec\/git-core\/.*//;
>> +        unshift @INC, $__prefix . "/share/git-core/perl";
>> +        unshift @INC, $__prefix . "/../Library/Perl/".$PERLVERSION."/darwin-thread-multi-2level";
>> +    } elsif ($__prefix =~ m/\/bin\// ) {
>> +        $__prefix =~ s/\/bin\/.*//;
>> +        unshift @INC, $__prefix . "/share/git-core/perl";
>> +        unshift @INC, $__prefix . "/../Library/Perl/".$PERLVERSION."/darwin-thread-multi-2level";
>> +    } elsif ( $__prefix =~ m/\/usr\// ) {
>> +        $__prefix =~ s/\/usr\/.*/\/usr/;
>> +        unshift @INC, $__prefix . "/share/git-core/perl";
>> +        unshift @INC, $__prefix . "/../Library/Perl/".$PERLVERSION."/darwin-thread-multi-2level";
>> +    }
>> +}
>> +# END XCODE RUNTIME_PREFIX generated code.
>> +
>> # BEGIN RUNTIME_PREFIX generated code.
>> #
>> # This finds our Git::* libraries relative to the script's runtime path.
>> -- 
>> 2.20.0 (Apple Git-115)
>> 
>> 

[PATCH (Apple Git) 11/13] Fix problem found from running the test suite.

[Jeremy Huddleston Sequoia]

From: Matt Wright <mww@apple.com>

Signed-off-by: Matt Wright <mww@apple.com>
---
 git-svn.perl | 30 ++++++++++++++++++++++++++++++
 1 file changed, 30 insertions(+)

diff --git a/git-svn.perl b/git-svn.perl
index 050f2a36f4..d29730be3b 100755
--- a/git-svn.perl
+++ b/git-svn.perl
@@ -1815,6 +1815,36 @@ sub complete_url_ls_init {
 
 sub verify_ref {
 	my ($ref) = @_;
+
+	if ($ref =~ /^(.*)\^0$/) {
+		my $baseref = $1;
+		my $p = "$ENV{GIT_DIR}/$baseref";
+		$p = "$ENV{GIT_DIR}/refs/remotes/$baseref" unless -e $p;
+		$p = "$ENV{GIT_DIR}/refs/$baseref" unless -e $p;
+		$p = "$ENV{GIT_DIR}/refs/heads/$baseref" unless -e $p;
+
+		my $resolved = undef;
+		if (-e $p) {
+			open FH, $p;
+			$resolved = <FH>;
+			chomp $resolved;
+			close FH;
+		} elsif (-e "$ENV{GIT_DIR}/packed-refs") {
+			open FH, "$ENV{GIT_DIR}/packed-refs";
+			while (<FH>) {
+				if ($_ =~ /^([0-9a-fA-F]+) ((refs\/)?(remotes\/|heads\/|\/)?$baseref)$/) {
+					$resolved = $1;
+					last;
+				}
+			}
+		}
+
+		if (defined($resolved)) {
+			return verify_ref("$1^0") if $resolved =~ /^ref: (.*)$/;
+			return $resolved
+		}
+	}
+
 	eval { command_oneline([ 'rev-parse', '--verify', $ref ],
 	                       { STDERR => 0 }); };
 }
-- 
2.20.0 (Apple Git-115)

Re: [PATCH (Apple Git) 11/13] Fix problem found from running the test suite.

[Junio C Hamano]

[jc: just forwarding to the area expert]

Jeremy Huddleston Sequoia <jeremyhu@apple.com> writes:

> From: Matt Wright <mww@apple.com>
>
> Signed-off-by: Matt Wright <mww@apple.com>
> ---
>  git-svn.perl | 30 ++++++++++++++++++++++++++++++
>  1 file changed, 30 insertions(+)
>
> diff --git a/git-svn.perl b/git-svn.perl
> index 050f2a36f4..d29730be3b 100755
> --- a/git-svn.perl
> +++ b/git-svn.perl
> @@ -1815,6 +1815,36 @@ sub complete_url_ls_init {
>  
>  sub verify_ref {
>  	my ($ref) = @_;
> +
> +	if ($ref =~ /^(.*)\^0$/) {
> +		my $baseref = $1;
> +		my $p = "$ENV{GIT_DIR}/$baseref";
> +		$p = "$ENV{GIT_DIR}/refs/remotes/$baseref" unless -e $p;
> +		$p = "$ENV{GIT_DIR}/refs/$baseref" unless -e $p;
> +		$p = "$ENV{GIT_DIR}/refs/heads/$baseref" unless -e $p;
> +
> +		my $resolved = undef;
> +		if (-e $p) {
> +			open FH, $p;
> +			$resolved = <FH>;
> +			chomp $resolved;
> +			close FH;
> +		} elsif (-e "$ENV{GIT_DIR}/packed-refs") {
> +			open FH, "$ENV{GIT_DIR}/packed-refs";
> +			while (<FH>) {
> +				if ($_ =~ /^([0-9a-fA-F]+) ((refs\/)?(remotes\/|heads\/|\/)?$baseref)$/) {
> +					$resolved = $1;
> +					last;
> +				}
> +			}
> +		}
> +
> +		if (defined($resolved)) {
> +			return verify_ref("$1^0") if $resolved =~ /^ref: (.*)$/;
> +			return $resolved
> +		}
> +	}
> +
>  	eval { command_oneline([ 'rev-parse', '--verify', $ref ],
>  	                       { STDERR => 0 }); };
>  }

Re: [PATCH (Apple Git) 11/13] Fix problem found from running the test suite.

[Eric Wong]

Jeremy Huddleston Sequoia <jeremyhu@apple.com> wrote:
> From: Matt Wright <mww@apple.com>
> 
> Signed-off-by: Matt Wright <mww@apple.com>

Hi Jeremy/Matt: I expect to see a description of said "problem"

More comments inline below...

> diff --git a/git-svn.perl b/git-svn.perl
> index 050f2a36f4..d29730be3b 100755
> --- a/git-svn.perl
> +++ b/git-svn.perl
> @@ -1815,6 +1815,36 @@ sub complete_url_ls_init {
>  
>  sub verify_ref {
>  	my ($ref) = @_;
> +
> +	if ($ref =~ /^(.*)\^0$/) {
> +		my $baseref = $1;
> +		my $p = "$ENV{GIT_DIR}/$baseref";
> +		$p = "$ENV{GIT_DIR}/refs/remotes/$baseref" unless -e $p;
> +		$p = "$ENV{GIT_DIR}/refs/$baseref" unless -e $p;
> +		$p = "$ENV{GIT_DIR}/refs/heads/$baseref" unless -e $p;

OK, this looks like we're reproducing rev-parse functionality...

> +		my $resolved = undef;
> +		if (-e $p) {
> +			open FH, $p;
> +			$resolved = <FH>;
> +			chomp $resolved;
> +			close FH;
> +		} elsif (-e "$ENV{GIT_DIR}/packed-refs") {
> +			open FH, "$ENV{GIT_DIR}/packed-refs";
> +			while (<FH>) {
> +				if ($_ =~ /^([0-9a-fA-F]+) ((refs\/)?(remotes\/|heads\/|\/)?$baseref)$/) {
> +					$resolved = $1;
> +					last;
> +				}
> +			}

And even more so...   This would be a pain to maintain with
proposed changes to ref storage (reftable/lmdb/...), so I really
don't want to reproduce rev-parse functionality in Perl.

But while we're in Perl; prefer something like:

	m!^([0-9a-f]+) ((refs/)?(remotes/|heads/|/)?$baseref)$!

So you don't have to escape '/' (leaning-toothpick syndrome) by
using m!$REGEX!.  You can also skip the unnecessary [A-F] match.

> +		}
> +
> +		if (defined($resolved)) {
> +			return verify_ref("$1^0") if $resolved =~ /^ref: (.*)$/;
> +			return $resolved
> +		}
> +	}

So without more details, we really need an explanation of why
this patch was made.  The test suite has been thousands of times
over the years on other platforms without changes to the
verify_ref() sub.

>  	eval { command_oneline([ 'rev-parse', '--verify', $ref ],
>  	                       { STDERR => 0 }); };
>  }

[PATCH (Apple Git) 12/13] Enable support for Xcode.app-bundled gitconfig

[Jeremy Huddleston Sequoia]

Useful for setting up osxkeychain in Xcode.app's gitconfig

Signed-off-by: Jeremy Huddleston Sequoia <jeremyhu@apple.com>
---
 config.c | 13 +++++++++++++
 config.h |  2 ++
 2 files changed, 15 insertions(+)

diff --git a/config.c b/config.c
index ff521eb27a..656bfef8ab 100644
--- a/config.c
+++ b/config.c
@@ -1631,6 +1631,14 @@ const char *git_etc_gitconfig(void)
 	return system_wide;
 }
 
+const char *git_xcode_gitconfig(void)
+{
+	static const char *xcode_config;
+	if (!xcode_config)
+		xcode_config = system_path("share/git-core/gitconfig");
+	return xcode_config;
+}
+
 /*
  * Parse environment variable 'k' as a boolean (in various
  * possible spellings); if missing, use the default value 'def'.
@@ -1673,6 +1681,11 @@ static int do_git_config_sequence(const struct config_options *opts,
 	else
 		repo_config = NULL;
 
+	current_parsing_scope = CONFIG_SCOPE_XCODE;
+	if (git_config_system() && git_xcode_gitconfig() && !access_or_die(git_xcode_gitconfig(), R_OK, 0))
+		ret += git_config_from_file(fn, git_xcode_gitconfig(),
+					    data);
+
 	current_parsing_scope = CONFIG_SCOPE_SYSTEM;
 	if (git_config_system() && !access_or_die(git_etc_gitconfig(), R_OK, 0))
 		ret += git_config_from_file(fn, git_etc_gitconfig(),
diff --git a/config.h b/config.h
index ee5d3fa7b4..f848423d28 100644
--- a/config.h
+++ b/config.h
@@ -115,6 +115,7 @@ extern int git_config_rename_section_in_file(const char *, const char *, const c
 extern int git_config_copy_section(const char *, const char *);
 extern int git_config_copy_section_in_file(const char *, const char *, const char *);
 extern const char *git_etc_gitconfig(void);
+extern const char *git_xcode_gitconfig(void);
 extern int git_env_bool(const char *, int);
 extern unsigned long git_env_ulong(const char *, unsigned long);
 extern int git_config_system(void);
@@ -131,6 +132,7 @@ enum config_scope {
 	CONFIG_SCOPE_GLOBAL,
 	CONFIG_SCOPE_REPO,
 	CONFIG_SCOPE_CMDLINE,
+	CONFIG_SCOPE_XCODE,
 };
 
 extern enum config_scope current_config_scope(void);
-- 
2.20.0 (Apple Git-115)

Re: [PATCH (Apple Git) 12/13] Enable support for Xcode.app-bundled gitconfig

[Junio C Hamano]

Jeremy Huddleston Sequoia <jeremyhu@apple.com> writes:

> Useful for setting up osxkeychain in Xcode.app's gitconfig
>
> Signed-off-by: Jeremy Huddleston Sequoia <jeremyhu@apple.com>
> ---

A concern shared with 13/13 is this.

While it may not hurt too much to look at one extra location even on
non-Apple platform, it probably is a mistake to have this xcode
specific change in generic part of the system like config.c or
attr.c.  For that matter, would it make sense to force Apple uses to
look at one extra location in the first place?  In other words, we
already have "system wide" location (i.e. system_path(ETC_GITCONFIG))
defined so system owners can give reasonable default to its users.
The value of not using that facility and instead adding yet another
place is dubious.


  




>  config.c | 13 +++++++++++++
>  config.h |  2 ++
>  2 files changed, 15 insertions(+)
>
> diff --git a/config.c b/config.c
> index ff521eb27a..656bfef8ab 100644
> --- a/config.c
> +++ b/config.c
> @@ -1631,6 +1631,14 @@ const char *git_etc_gitconfig(void)
>  	return system_wide;
>  }
>  
> +const char *git_xcode_gitconfig(void)
> +{
> +	static const char *xcode_config;
> +	if (!xcode_config)
> +		xcode_config = system_path("share/git-core/gitconfig");
> +	return xcode_config;
> +}
> +
>  /*
>   * Parse environment variable 'k' as a boolean (in various
>   * possible spellings); if missing, use the default value 'def'.
> @@ -1673,6 +1681,11 @@ static int do_git_config_sequence(const struct config_options *opts,
>  	else
>  		repo_config = NULL;
>  
> +	current_parsing_scope = CONFIG_SCOPE_XCODE;
> +	if (git_config_system() && git_xcode_gitconfig() && !access_or_die(git_xcode_gitconfig(), R_OK, 0))
> +		ret += git_config_from_file(fn, git_xcode_gitconfig(),
> +					    data);
> +
>  	current_parsing_scope = CONFIG_SCOPE_SYSTEM;
>  	if (git_config_system() && !access_or_die(git_etc_gitconfig(), R_OK, 0))
>  		ret += git_config_from_file(fn, git_etc_gitconfig(),
> diff --git a/config.h b/config.h
> index ee5d3fa7b4..f848423d28 100644
> --- a/config.h
> +++ b/config.h
> @@ -115,6 +115,7 @@ extern int git_config_rename_section_in_file(const char *, const char *, const c
>  extern int git_config_copy_section(const char *, const char *);
>  extern int git_config_copy_section_in_file(const char *, const char *, const char *);
>  extern const char *git_etc_gitconfig(void);
> +extern const char *git_xcode_gitconfig(void);
>  extern int git_env_bool(const char *, int);
>  extern unsigned long git_env_ulong(const char *, unsigned long);
>  extern int git_config_system(void);
> @@ -131,6 +132,7 @@ enum config_scope {
>  	CONFIG_SCOPE_GLOBAL,
>  	CONFIG_SCOPE_REPO,
>  	CONFIG_SCOPE_CMDLINE,
> +	CONFIG_SCOPE_XCODE,
>  };
>  
>  extern enum config_scope current_config_scope(void);

Re: [PATCH (Apple Git) 12/13] Enable support for Xcode.app-bundled gitconfig

[Jeremy Huddleston Sequoia]

> On Jan 29, 2019, at 3:10 PM, Junio C Hamano <gitster@pobox.com> wrote:
> 
> Jeremy Huddleston Sequoia <jeremyhu@apple.com> writes:
> 
>> Useful for setting up osxkeychain in Xcode.app's gitconfig
>> 
>> Signed-off-by: Jeremy Huddleston Sequoia <jeremyhu@apple.com>
>> ---
> 
> A concern shared with 13/13 is this.
> 
> While it may not hurt too much to look at one extra location even on
> non-Apple platform, it probably is a mistake to have this xcode
> specific change in generic part of the system like config.c or
> attr.c.  For that matter, would it make sense to force Apple uses to
> look at one extra location in the first place?  In other words, we
> already have "system wide" location (i.e. system_path(ETC_GITCONFIG))
> defined so system owners can give reasonable default to its users.
> The value of not using that facility and instead adding yet another
> place is dubious.

This allows for per-distribution configuration and could be useful for other applications as well that want customizations specific to their install of git.  For our specific use case, we do not want to munge the system policy when installing Xcode.  Prior to doing things this way, we were just changing the default in our distributed git binary, but this seems a bit more flexible.

> 
> 
> 
> 
> 
> 
> 
>> config.c | 13 +++++++++++++
>> config.h |  2 ++
>> 2 files changed, 15 insertions(+)
>> 
>> diff --git a/config.c b/config.c
>> index ff521eb27a..656bfef8ab 100644
>> --- a/config.c
>> +++ b/config.c
>> @@ -1631,6 +1631,14 @@ const char *git_etc_gitconfig(void)
>> 	return system_wide;
>> }
>> 
>> +const char *git_xcode_gitconfig(void)
>> +{
>> +	static const char *xcode_config;
>> +	if (!xcode_config)
>> +		xcode_config = system_path("share/git-core/gitconfig");
>> +	return xcode_config;
>> +}
>> +
>> /*
>>  * Parse environment variable 'k' as a boolean (in various
>>  * possible spellings); if missing, use the default value 'def'.
>> @@ -1673,6 +1681,11 @@ static int do_git_config_sequence(const struct config_options *opts,
>> 	else
>> 		repo_config = NULL;
>> 
>> +	current_parsing_scope = CONFIG_SCOPE_XCODE;
>> +	if (git_config_system() && git_xcode_gitconfig() && !access_or_die(git_xcode_gitconfig(), R_OK, 0))
>> +		ret += git_config_from_file(fn, git_xcode_gitconfig(),
>> +					    data);
>> +
>> 	current_parsing_scope = CONFIG_SCOPE_SYSTEM;
>> 	if (git_config_system() && !access_or_die(git_etc_gitconfig(), R_OK, 0))
>> 		ret += git_config_from_file(fn, git_etc_gitconfig(),
>> diff --git a/config.h b/config.h
>> index ee5d3fa7b4..f848423d28 100644
>> --- a/config.h
>> +++ b/config.h
>> @@ -115,6 +115,7 @@ extern int git_config_rename_section_in_file(const char *, const char *, const c
>> extern int git_config_copy_section(const char *, const char *);
>> extern int git_config_copy_section_in_file(const char *, const char *, const char *);
>> extern const char *git_etc_gitconfig(void);
>> +extern const char *git_xcode_gitconfig(void);
>> extern int git_env_bool(const char *, int);
>> extern unsigned long git_env_ulong(const char *, unsigned long);
>> extern int git_config_system(void);
>> @@ -131,6 +132,7 @@ enum config_scope {
>> 	CONFIG_SCOPE_GLOBAL,
>> 	CONFIG_SCOPE_REPO,
>> 	CONFIG_SCOPE_CMDLINE,
>> +	CONFIG_SCOPE_XCODE,
>> };
>> 
>> extern enum config_scope current_config_scope(void);

Re: [PATCH (Apple Git) 12/13] Enable support for Xcode.app-bundled gitconfig

[Johannes Schindelin]

Hi Jeremy,

On Tue, 29 Jan 2019, Jeremy Huddleston Sequoia wrote:

> > On Jan 29, 2019, at 3:10 PM, Junio C Hamano <gitster@pobox.com> wrote:
> > 
> > Jeremy Huddleston Sequoia <jeremyhu@apple.com> writes:
> > 
> >> Useful for setting up osxkeychain in Xcode.app's gitconfig
> >> 
> >> Signed-off-by: Jeremy Huddleston Sequoia <jeremyhu@apple.com>
> >> ---
> > 
> > A concern shared with 13/13 is this.
> > 
> > While it may not hurt too much to look at one extra location even on
> > non-Apple platform, it probably is a mistake to have this xcode
> > specific change in generic part of the system like config.c or
> > attr.c.  For that matter, would it make sense to force Apple uses to
> > look at one extra location in the first place?  In other words, we
> > already have "system wide" location (i.e. system_path(ETC_GITCONFIG))
> > defined so system owners can give reasonable default to its users.
> > The value of not using that facility and instead adding yet another
> > place is dubious.
> 
> This allows for per-distribution configuration and could be useful for
> other applications as well that want customizations specific to their
> install of git.  For our specific use case, we do not want to munge the
> system policy when installing Xcode.  Prior to doing things this way, we
> were just changing the default in our distributed git binary, but this
> seems a bit more flexible.

I think you misunderstood Junio, thinking that he referred to
/etc/gitconfig. He did not. system_path(ETC_GITCONFIG) refers to
<prefix>/etc/gitconfig, where <prefix> is that runtime prefix when
compiled with RUNTIME_PREFIX.

So you can definitely have your own per-distribution configuration: it
lives in that very <prefix>/etc/gitconfig where the portable Git is
installed.

And since we have that nice facility, I agree with Junio that we probably
do not even need an extra config, certainly not one just introduced for
XCode.

Ciao,
Johannes

> 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> >> config.c | 13 +++++++++++++
> >> config.h |  2 ++
> >> 2 files changed, 15 insertions(+)
> >> 
> >> diff --git a/config.c b/config.c
> >> index ff521eb27a..656bfef8ab 100644
> >> --- a/config.c
> >> +++ b/config.c
> >> @@ -1631,6 +1631,14 @@ const char *git_etc_gitconfig(void)
> >> 	return system_wide;
> >> }
> >> 
> >> +const char *git_xcode_gitconfig(void)
> >> +{
> >> +	static const char *xcode_config;
> >> +	if (!xcode_config)
> >> +		xcode_config = system_path("share/git-core/gitconfig");
> >> +	return xcode_config;
> >> +}
> >> +
> >> /*
> >>  * Parse environment variable 'k' as a boolean (in various
> >>  * possible spellings); if missing, use the default value 'def'.
> >> @@ -1673,6 +1681,11 @@ static int do_git_config_sequence(const struct config_options *opts,
> >> 	else
> >> 		repo_config = NULL;
> >> 
> >> +	current_parsing_scope = CONFIG_SCOPE_XCODE;
> >> +	if (git_config_system() && git_xcode_gitconfig() && !access_or_die(git_xcode_gitconfig(), R_OK, 0))
> >> +		ret += git_config_from_file(fn, git_xcode_gitconfig(),
> >> +					    data);
> >> +
> >> 	current_parsing_scope = CONFIG_SCOPE_SYSTEM;
> >> 	if (git_config_system() && !access_or_die(git_etc_gitconfig(), R_OK, 0))
> >> 		ret += git_config_from_file(fn, git_etc_gitconfig(),
> >> diff --git a/config.h b/config.h
> >> index ee5d3fa7b4..f848423d28 100644
> >> --- a/config.h
> >> +++ b/config.h
> >> @@ -115,6 +115,7 @@ extern int git_config_rename_section_in_file(const char *, const char *, const c
> >> extern int git_config_copy_section(const char *, const char *);
> >> extern int git_config_copy_section_in_file(const char *, const char *, const char *);
> >> extern const char *git_etc_gitconfig(void);
> >> +extern const char *git_xcode_gitconfig(void);
> >> extern int git_env_bool(const char *, int);
> >> extern unsigned long git_env_ulong(const char *, unsigned long);
> >> extern int git_config_system(void);
> >> @@ -131,6 +132,7 @@ enum config_scope {
> >> 	CONFIG_SCOPE_GLOBAL,
> >> 	CONFIG_SCOPE_REPO,
> >> 	CONFIG_SCOPE_CMDLINE,
> >> +	CONFIG_SCOPE_XCODE,
> >> };
> >> 
> >> extern enum config_scope current_config_scope(void);
> 
> 

Re: [PATCH (Apple Git) 12/13] Enable support for Xcode.app-bundled gitconfig

[Jeremy Huddleston Sequoia]

> On Jan 30, 2019, at 11:32, Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
> 
> Hi Jeremy,
> 
> On Tue, 29 Jan 2019, Jeremy Huddleston Sequoia wrote:
> 
>>> On Jan 29, 2019, at 3:10 PM, Junio C Hamano <gitster@pobox.com> wrote:
>>> 
>>> Jeremy Huddleston Sequoia <jeremyhu@apple.com> writes:
>>> 
>>>> Useful for setting up osxkeychain in Xcode.app's gitconfig
>>>> 
>>>> Signed-off-by: Jeremy Huddleston Sequoia <jeremyhu@apple.com>
>>>> ---
>>> 
>>> A concern shared with 13/13 is this.
>>> 
>>> While it may not hurt too much to look at one extra location even on
>>> non-Apple platform, it probably is a mistake to have this xcode
>>> specific change in generic part of the system like config.c or
>>> attr.c.  For that matter, would it make sense to force Apple uses to
>>> look at one extra location in the first place?  In other words, we
>>> already have "system wide" location (i.e. system_path(ETC_GITCONFIG))
>>> defined so system owners can give reasonable default to its users.
>>> The value of not using that facility and instead adding yet another
>>> place is dubious.
>> 
>> This allows for per-distribution configuration and could be useful for
>> other applications as well that want customizations specific to their
>> install of git.  For our specific use case, we do not want to munge the
>> system policy when installing Xcode.  Prior to doing things this way, we
>> were just changing the default in our distributed git binary, but this
>> seems a bit more flexible.
> 
> I think you misunderstood Junio, thinking that he referred to
> /etc/gitconfig. He did not. system_path(ETC_GITCONFIG) refers to
> <prefix>/etc/gitconfig, where <prefix> is that runtime prefix when
> compiled with RUNTIME_PREFIX.

Oh!  Awesome.  I didn't even notice this was a thing.  That would exactly solve our use case.  I'll give that a whirl.  That likely allows us to eliminate these two patches completely!

> So you can definitely have your own per-distribution configuration: it
> lives in that very <prefix>/etc/gitconfig where the portable Git is
> installed.
> 
> And since we have that nice facility, I agree with Junio that we probably
> do not even need an extra config, certainly not one just introduced for
> XCode.
> 
> Ciao,
> Johannes
> 
>> 
>>> 
>>> 
>>> 
>>> 
>>> 
>>> 
>>> 
>>>> config.c | 13 +++++++++++++
>>>> config.h |  2 ++
>>>> 2 files changed, 15 insertions(+)
>>>> 
>>>> diff --git a/config.c b/config.c
>>>> index ff521eb27a..656bfef8ab 100644
>>>> --- a/config.c
>>>> +++ b/config.c
>>>> @@ -1631,6 +1631,14 @@ const char *git_etc_gitconfig(void)
>>>> 	return system_wide;
>>>> }
>>>> 
>>>> +const char *git_xcode_gitconfig(void)
>>>> +{
>>>> +	static const char *xcode_config;
>>>> +	if (!xcode_config)
>>>> +		xcode_config = system_path("share/git-core/gitconfig");
>>>> +	return xcode_config;
>>>> +}
>>>> +
>>>> /*
>>>> * Parse environment variable 'k' as a boolean (in various
>>>> * possible spellings); if missing, use the default value 'def'.
>>>> @@ -1673,6 +1681,11 @@ static int do_git_config_sequence(const struct config_options *opts,
>>>> 	else
>>>> 		repo_config = NULL;
>>>> 
>>>> +	current_parsing_scope = CONFIG_SCOPE_XCODE;
>>>> +	if (git_config_system() && git_xcode_gitconfig() && !access_or_die(git_xcode_gitconfig(), R_OK, 0))
>>>> +		ret += git_config_from_file(fn, git_xcode_gitconfig(),
>>>> +					    data);
>>>> +
>>>> 	current_parsing_scope = CONFIG_SCOPE_SYSTEM;
>>>> 	if (git_config_system() && !access_or_die(git_etc_gitconfig(), R_OK, 0))
>>>> 		ret += git_config_from_file(fn, git_etc_gitconfig(),
>>>> diff --git a/config.h b/config.h
>>>> index ee5d3fa7b4..f848423d28 100644
>>>> --- a/config.h
>>>> +++ b/config.h
>>>> @@ -115,6 +115,7 @@ extern int git_config_rename_section_in_file(const char *, const char *, const c
>>>> extern int git_config_copy_section(const char *, const char *);
>>>> extern int git_config_copy_section_in_file(const char *, const char *, const char *);
>>>> extern const char *git_etc_gitconfig(void);
>>>> +extern const char *git_xcode_gitconfig(void);
>>>> extern int git_env_bool(const char *, int);
>>>> extern unsigned long git_env_ulong(const char *, unsigned long);
>>>> extern int git_config_system(void);
>>>> @@ -131,6 +132,7 @@ enum config_scope {
>>>> 	CONFIG_SCOPE_GLOBAL,
>>>> 	CONFIG_SCOPE_REPO,
>>>> 	CONFIG_SCOPE_CMDLINE,
>>>> +	CONFIG_SCOPE_XCODE,
>>>> };
>>>> 
>>>> extern enum config_scope current_config_scope(void);
>> 
>> 

Re: [PATCH (Apple Git) 12/13] Enable support for Xcode.app-bundled gitconfig

[Jeremy Huddleston Sequoia]

> On Jan 30, 2019, at 13:09, Jeremy Huddleston Sequoia <jeremyhu@apple.com> wrote:
> 
> 
> 
>> On Jan 30, 2019, at 11:32, Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
>> 
>> Hi Jeremy,
>> 
>> On Tue, 29 Jan 2019, Jeremy Huddleston Sequoia wrote:
>> 
>>>> On Jan 29, 2019, at 3:10 PM, Junio C Hamano <gitster@pobox.com> wrote:
>>>> 
>>>> Jeremy Huddleston Sequoia <jeremyhu@apple.com> writes:
>>>> 
>>>>> Useful for setting up osxkeychain in Xcode.app's gitconfig
>>>>> 
>>>>> Signed-off-by: Jeremy Huddleston Sequoia <jeremyhu@apple.com>
>>>>> ---
>>>> 
>>>> A concern shared with 13/13 is this.
>>>> 
>>>> While it may not hurt too much to look at one extra location even on
>>>> non-Apple platform, it probably is a mistake to have this xcode
>>>> specific change in generic part of the system like config.c or
>>>> attr.c.  For that matter, would it make sense to force Apple uses to
>>>> look at one extra location in the first place?  In other words, we
>>>> already have "system wide" location (i.e. system_path(ETC_GITCONFIG))
>>>> defined so system owners can give reasonable default to its users.
>>>> The value of not using that facility and instead adding yet another
>>>> place is dubious.
>>> 
>>> This allows for per-distribution configuration and could be useful for
>>> other applications as well that want customizations specific to their
>>> install of git.  For our specific use case, we do not want to munge the
>>> system policy when installing Xcode.  Prior to doing things this way, we
>>> were just changing the default in our distributed git binary, but this
>>> seems a bit more flexible.
>> 
>> I think you misunderstood Junio, thinking that he referred to
>> /etc/gitconfig. He did not. system_path(ETC_GITCONFIG) refers to
>> <prefix>/etc/gitconfig, where <prefix> is that runtime prefix when
>> compiled with RUNTIME_PREFIX.
> 
> Oh!  Awesome.  I didn't even notice this was a thing.  That would exactly solve our use case.  I'll give that a whirl.  That likely allows us to eliminate these two patches completely!

Unfortunately, I was quick to celebrate.  This picks up the bundled file instead of a system-wide file.  I'd love it if we could still honor system-wide config/attributes in addition to RUNTIME_PREFIX-relative ones (eg: user overrides system which overrides distribution).  I worry that as is, we'd stop referencing the system-wide configs which might confuse users.

Is that something you'd be interested in, or should we just continue to maintain our separate patches?

> 
>> So you can definitely have your own per-distribution configuration: it
>> lives in that very <prefix>/etc/gitconfig where the portable Git is
>> installed.
>> 
>> And since we have that nice facility, I agree with Junio that we probably
>> do not even need an extra config, certainly not one just introduced for
>> XCode.
>> 
>> Ciao,
>> Johannes
>> 
>>> 
>>>> 
>>>> 
>>>> 
>>>> 
>>>> 
>>>> 
>>>> 
>>>>> config.c | 13 +++++++++++++
>>>>> config.h |  2 ++
>>>>> 2 files changed, 15 insertions(+)
>>>>> 
>>>>> diff --git a/config.c b/config.c
>>>>> index ff521eb27a..656bfef8ab 100644
>>>>> --- a/config.c
>>>>> +++ b/config.c
>>>>> @@ -1631,6 +1631,14 @@ const char *git_etc_gitconfig(void)
>>>>> 	return system_wide;
>>>>> }
>>>>> 
>>>>> +const char *git_xcode_gitconfig(void)
>>>>> +{
>>>>> +	static const char *xcode_config;
>>>>> +	if (!xcode_config)
>>>>> +		xcode_config = system_path("share/git-core/gitconfig");
>>>>> +	return xcode_config;
>>>>> +}
>>>>> +
>>>>> /*
>>>>> * Parse environment variable 'k' as a boolean (in various
>>>>> * possible spellings); if missing, use the default value 'def'.
>>>>> @@ -1673,6 +1681,11 @@ static int do_git_config_sequence(const struct config_options *opts,
>>>>> 	else
>>>>> 		repo_config = NULL;
>>>>> 
>>>>> +	current_parsing_scope = CONFIG_SCOPE_XCODE;
>>>>> +	if (git_config_system() && git_xcode_gitconfig() && !access_or_die(git_xcode_gitconfig(), R_OK, 0))
>>>>> +		ret += git_config_from_file(fn, git_xcode_gitconfig(),
>>>>> +					    data);
>>>>> +
>>>>> 	current_parsing_scope = CONFIG_SCOPE_SYSTEM;
>>>>> 	if (git_config_system() && !access_or_die(git_etc_gitconfig(), R_OK, 0))
>>>>> 		ret += git_config_from_file(fn, git_etc_gitconfig(),
>>>>> diff --git a/config.h b/config.h
>>>>> index ee5d3fa7b4..f848423d28 100644
>>>>> --- a/config.h
>>>>> +++ b/config.h
>>>>> @@ -115,6 +115,7 @@ extern int git_config_rename_section_in_file(const char *, const char *, const c
>>>>> extern int git_config_copy_section(const char *, const char *);
>>>>> extern int git_config_copy_section_in_file(const char *, const char *, const char *);
>>>>> extern const char *git_etc_gitconfig(void);
>>>>> +extern const char *git_xcode_gitconfig(void);
>>>>> extern int git_env_bool(const char *, int);
>>>>> extern unsigned long git_env_ulong(const char *, unsigned long);
>>>>> extern int git_config_system(void);
>>>>> @@ -131,6 +132,7 @@ enum config_scope {
>>>>> 	CONFIG_SCOPE_GLOBAL,
>>>>> 	CONFIG_SCOPE_REPO,
>>>>> 	CONFIG_SCOPE_CMDLINE,
>>>>> +	CONFIG_SCOPE_XCODE,
>>>>> };
>>>>> 
>>>>> extern enum config_scope current_config_scope(void);
>>> 
>>> 
> 

Re: [PATCH (Apple Git) 12/13] Enable support for Xcode.app-bundled gitconfig

[Jonathan Nieder]

Hi,

Jeremy Huddleston Sequoia wrote:

> Unfortunately, I was quick to celebrate.  This picks up the bundled
> file instead of a system-wide file.  I'd love it if we could still
> honor system-wide config/attributes in addition to
> RUNTIME_PREFIX-relative ones (eg: user overrides system which
> overrides distribution).  I worry that as is, we'd stop referencing
> the system-wide configs which might confuse users.
> 
> Is that something you'd be interested in, or should we just continue
> to maintain our separate patches?

For the internal deployment at Google, what we've done is to put an
[include] path directive in the global gitconfig:

	[include]
		path = /usr/share/git-core/config

Users can edit the global git config in etc, but the distributed
config at /usr/share/git-core/config is read-only as part of the
distributed package.

We considered making an upstream change to bake in the distributed
config in the git binary but decided that this way is a little
nicer since it lets people comment out the include.path setting if
they want to e.g. for experimentation.  It's also more explicit
(hence easier to understand).

Would a similar approach work for your setup?  Can you say a little
more about how you'd like things to work from an end-user pov?

Thanks,
Jonathan

Re: [PATCH (Apple Git) 12/13] Enable support for Xcode.app-bundled gitconfig

[Jeremy Huddleston Sequoia]

> On Jan 30, 2019, at 16:01, Jonathan Nieder <jrnieder@gmail.com> wrote:
> 
> Hi,
> 
> Jeremy Huddleston Sequoia wrote:
> 
>> Unfortunately, I was quick to celebrate.  This picks up the bundled
>> file instead of a system-wide file.  I'd love it if we could still
>> honor system-wide config/attributes in addition to
>> RUNTIME_PREFIX-relative ones (eg: user overrides system which
>> overrides distribution).  I worry that as is, we'd stop referencing
>> the system-wide configs which might confuse users.
>> 
>> Is that something you'd be interested in, or should we just continue
>> to maintain our separate patches?
> 
> For the internal deployment at Google, what we've done is to put an
> [include] path directive in the global gitconfig:
> 
> 	[include]
> 		path = /usr/share/git-core/config
> 
> Users can edit the global git config in etc, but the distributed
> config at /usr/share/git-core/config is read-only as part of the
> distributed package.
> 
> We considered making an upstream change to bake in the distributed
> config in the git binary but decided that this way is a little
> nicer since it lets people comment out the include.path setting if
> they want to e.g. for experimentation.  It's also more explicit
> (hence easier to understand).
> 
> Would a similar approach work for your setup?  Can you say a little
> more about how you'd like things to work from an end-user pov?

That might work.  I could put this in the Xcode.app gitconfig:

	[include]
		path = /private/etc/gitconfig

Would that result in /private/etc/gitconfig's taking precedence or Xcode.app's?
Is there anything analogous I could do for gitattributes?

Re: [PATCH (Apple Git) 12/13] Enable support for Xcode.app-bundled gitconfig

[Junio C Hamano]

Jeremy Huddleston Sequoia <jeremyhu@apple.com> writes:

>>>> A concern shared with 13/13 is this.
>>>> 
>>>> While it may not hurt too much to look at one extra location even on
>>>> non-Apple platform, it probably is a mistake to have this xcode
>>>> specific change in generic part of the system like config.c or
>>>> attr.c.  For that matter, would it make sense to force Apple uses to
>>>> look at one extra location in the first place?  In other words, we
>>>> already have "system wide" location (i.e. system_path(ETC_GITCONFIG))
>>>> defined so system owners can give reasonable default to its users.
>>>> The value of not using that facility and instead adding yet another
>>>> place is dubious.
>>> 
>>> This allows for per-distribution configuration and could be useful for
>>> other applications as well that want customizations specific to their
>>> install of git.  For our specific use case, we do not want to munge the
>>> system policy when installing Xcode.  Prior to doing things this way, we
>>> were just changing the default in our distributed git binary, but this
>>> seems a bit more flexible.
>> 
>> I think you misunderstood Junio, thinking that he referred to
>> /etc/gitconfig. He did not. system_path(ETC_GITCONFIG) refers to
>> <prefix>/etc/gitconfig, where <prefix> is that runtime prefix when
>> compiled with RUNTIME_PREFIX.
>
> Oh!  Awesome.

I do not think you misunderstood.  system_path(ETC_GITCONFIG) may be
in <prefix>/etc/gitconfig when building with RUNTIME_PREFIX, but
then I do not think /etc/gitconfig (without <prefix>) comes into the
picture.  So as long as you want to add "a forced by distribution,
not editable by end user to set a global policy for the entire box"
configuration, that goes against the design of the configuration
system, which wants to have three levels (i.e. per repository, per
user and per box).

I think the arrangement jrnieder illustrates in a message in this
thread to use the inclusion of distro-provided file from
/etc/gitconfig, which documents what is happening clearly and still
allows the user to disable the distro-provided one if needed, is
probably the best solution under the current design.

Re: [PATCH (Apple Git) 12/13] Enable support for Xcode.app-bundled gitconfig

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 1912 bytes --]

On Tue, Jan 29, 2019 at 03:10:30PM -0800, Junio C Hamano wrote:
> Jeremy Huddleston Sequoia <jeremyhu@apple.com> writes:
> 
> > Useful for setting up osxkeychain in Xcode.app's gitconfig
> >
> > Signed-off-by: Jeremy Huddleston Sequoia <jeremyhu@apple.com>
> > ---
> 
> A concern shared with 13/13 is this.
> 
> While it may not hurt too much to look at one extra location even on
> non-Apple platform, it probably is a mistake to have this xcode
> specific change in generic part of the system like config.c or
> attr.c.  For that matter, would it make sense to force Apple uses to
> look at one extra location in the first place?  In other words, we
> already have "system wide" location (i.e. system_path(ETC_GITCONFIG))
> defined so system owners can give reasonable default to its users.
> The value of not using that facility and instead adding yet another
> place is dubious.

I think it's relevant in this case if I point out what the gitconfig and
gitattributes files contain on macOS. The gitconfig file sets up the
default credential helper, and the gitattributes file sets up default
diff helpers for Objective C and Swift.

To my knowledge, I believe what other distributors (including Homebrew)
do is they provide the system configuration file with default options if
one is missing, allowing the user to retain or delete these options as
the administrator sees fit. Apple may or may not want to do that, but I
believe that's probably the way that we'll choose to support.

While distributors will of course customize Git in whatever way seems
most appropriate, I think it's better if those customizations are
editable by the user, since that makes it easier to write per-user
configuration files that are consistent across platforms. For a tool
like Git, that can be quite helpful.
-- 
brian m. carlson: Houston, Texas, US
OpenPGP: https://keybase.io/bk2204

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 868 bytes --]

[PATCH (Apple Git) 13/13] Enable support for Xcode.app-bundled gitattributes

[Jeremy Huddleston Sequoia]

Signed-off-by: Jeremy Huddleston Sequoia <jeremyhu@apple.com>
---
 attr.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/attr.c b/attr.c
index eaece6658d..1b87905d4a 100644
--- a/attr.c
+++ b/attr.c
@@ -823,6 +823,14 @@ static const char *git_etc_gitattributes(void)
 	return system_wide;
 }
 
+static const char *git_xcode_gitattributes(void)
+{
+	static const char *xcode_gitattributes;
+	if (!xcode_gitattributes)
+		xcode_gitattributes = system_path("share/git-core/gitattributes");
+	return xcode_gitattributes;
+}
+
 static const char *get_home_gitattributes(void)
 {
 	if (!git_attributes_file)
@@ -864,6 +872,9 @@ static void bootstrap_attr_stack(const struct index_state *istate,
 
 	/* system-wide frame */
 	if (git_attr_system()) {
+		e = read_attr_from_file(git_xcode_gitattributes(), 1);
+		push_stack(stack, e, NULL, 0);
+
 		e = read_attr_from_file(git_etc_gitattributes(), 1);
 		push_stack(stack, e, NULL, 0);
 	}
-- 
2.20.0 (Apple Git-115)