Re: Subscribing Apple people to git-security@googlegroups.com

[Jeremy Huddleston Sequoia <jeremyhu@apple.com>]

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



> On Jul 10, 2018, at 5:27 AM, Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:
> 
> On Tue, Jul 3, 2018 at 3:36 PM, Jeff King <peff@peff.net> wrote:
>> On Mon, Jul 02, 2018 at 01:15:19PM -0700, Jeremy Huddleston Sequoia wrote:
>> 
>>>> I hope that maybe they're also interested in reducing the overall
>>>> diff between upstream Git and what ships with XCode. Last time I
>>>> looked (which was admittedly a while ago), a lot of the changes
>>>> seemed like things that could probably be considered upstream.
>>> 
>>> I'm very very interested in having reduced differences between what we
>>> ship in Xcode and what is upstream.  I've been maintaining a repo with
>>> our patches that I rebase as we move forward, in the hope that these
>>> changes might be useful to others and a derivative of them might
>>> eventually be accepted upstream.  See
>>> https://github.com/jeremyhu/git/commits/master for the current set of
>>> changes that are in our shipping git (currently on top of 2.17.1).
>> 
>> Thanks for sharing. Skimming over it, I see:
>> 
>> - several of the changes look related to run-time relocation. There was
>>   a series that shipped in v2.18.0 related to this, so that may reduce
>>   your diff once you rebase.
>> 
>> - The xcode_gitattributes() bits aren't likely to go upstream as-is.
>>   But possibly these could ship as a default $sysconfdir/gitattributes?
>> 
>> - the rest look like assorted little fixes that probably could go
>>   upstream
> 
> Jeremy, could you elaborate on what
> https://github.com/jeremyhu/git/commit/61b42bc5d2 was about? I.e.
> where was this discussed & tests for this refused?
> 
> Seems sensible to me to have this in some form, but the test as-is
> seems to be a general regression test, not Apple-specific, so it would
> need to be changed somewhat, or does it only happen with some other
> custom patch of yours?

It was a bug in upstream git and not a bug specific to an Apple change.  We haven't traditionally had many custom changes on our end.  The few we have, we didn't feel they were appropriate or were often rejected when we tried (eg: using CommonCrypto and Security.framework, this one, etc.).

For this particular case, I discussed the bug with the committer (Carlo) and reviewer (Junio) of the commit (18e051a3981f38db08521bb61ccf7e4571335353) via email back in October 2011.  My proposed fix and test were never accepted.  As such, we continued to ship my patch in Xcode's git and MacPorts' git until the underlying bug was actually fixed by someone else in 2014 (ddc2a6281595fd24ea01497c496f88c40a59562f + 655ee9ea3e6c0af57d320e84723ec3bf656cdbf7).  I kept the test in our test suite to ensure we didn't regress.  Here's the final post from that thread after the fix in 2014:

[-- 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: 358 bytes --]




Once I rebase on top of 2.18, I'll send out the full set of changes to git@vger as a starting point for discussion again.  I imagine many are not acceptable in current form but might be a starting point for additional discussion (eg: adding options for vendor-specific version rather than the hard coded "Apple Git-##" string).

Thanks,
Jeremy