> On Jan 30, 2019, at 05:12, Johannes Schindelin <Johannes.Schindelin@xxxxxx> wrote: > > Hi Jeremy, > > On Tue, 29 Jan 2019, Jeremy Huddleston Sequoia wrote: > >> Signed-off-by: Jeremy Huddleston Sequoia <jeremyhu@xxxxxxxxx> > > 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.
--- Begin Message ---
- Subject: Re: [PATCH git] setup: Do not strip trailing / from paths
- From: Jeremy Huddleston Sequoia <jeremyhu@xxxxxxxxx>
- Date: Mon, 22 Sep 2014 14:46:40 -0700
- In-reply-to: <5616AB22-EF27-4989-8394-65F5CE691AD6@apple.com>
- References: <80D13461-9FBE-4228-8927-C1D36B3ACDE4@apple.com> <B697F375-DCDA-401E-A45B-0E80550F0054@apple.com> <7vzkh7fmmp.fsf@alter.siamese.dyndns.org> <6DE7C010-AF6B-4C22-9771-A4F94D59609D@apple.com> <5616AB22-EF27-4989-8394-65F5CE691AD6@apple.com>
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@xxxxxxxxx> 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@xxxxxxxxx> 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@xxxxxxxxx> >>>> --- >>>> >>>> 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: smime.p7s>>
--- End Message ---
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)
>>
>>
