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

Message ID 20190129193818.8645-4-jeremyhu@apple.com
State New
Headers show
Series
  • Differences between git-2.20.1 and Apple Git-116
Related show

Commit Message

Jeremy Sequoia Jan. 29, 2019, 7:38 p.m. UTC
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

Comments

Eric Sunshine Jan. 30, 2019, 11:47 a.m. UTC | #1
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.
Johannes Schindelin Jan. 30, 2019, 1:12 p.m. UTC | #2
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)
> 
>
Jeremy Sequoia Jan. 30, 2019, 7:04 p.m. UTC | #3
> 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.
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)
>>> 
>> 
>
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
diff mbox series

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