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

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

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

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