Re: [PATCH v2] git-sh-setup: Fix scripts whose PWD is a symlink into a git work-dir
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Dec 10, 2008, 20:18 UTC
- Message-ID
- <7viqprzsvs.fsf@gitster.siamese.dyndns.org>
- In-Reply-To
- <1228921454-22416-1-git-send-email-marcel@oak.homeunix.org>
"Marcel M. Cary" <marcel@oak.homeunix.org> writes:
Show 5 quoted lines
> * When interpretting a relative upward (../) path in cd_to_toplevel, > prepend the cwd without symlinks, given by /bin/pwd > * Add tests for cd_to_toplevel and "git pull" in a symlinked > directory that failed before this fix, plus constrasting > scenarios that already worked
These are descriptions of changes (and good ones at that, but "constrasting?").
It however is a good idea to describe the problem the patch tries to solve *before* going into details of what you did. "If A is B, operation C tries to incorrectly access directory D; it should use directory E. This breakage is because F is confused by G..."
Yes, the "Subject:" already hints about the "If A is B" part, and the second bullet point uses the word "failed" to hint that there was a breakage, but that will not be sufficient description to recall the analysis you did of the problem, when you have read the commit log message 6 months from now what the breakage was about.
In order to justify the change against "Doctor if A is B, it hurts --- don't do it then" rebuttals, it further may make sense to defend why it is sometimes useful to be able to satisify the precondition that triggers the existing problem. That would come before the problem description to prepare readers with the context of the patch.
Show 24 quoted lines
> diff --git a/t/t2300-cd-to-toplevel.sh b/t/t2300-cd-to-toplevel.sh
> new file mode 100755
> index 0000000..293dc35
> --- /dev/null
> +++ b/t/t2300-cd-to-toplevel.sh
> @@ -0,0 +1,37 @@
> +#!/bin/sh
> +
> +test_description='cd_to_toplevel'
> +
> +. ./test-lib.sh
> +
> +test_cd_to_toplevel () {
> + test_expect_success "$2" '
> + (
> + cd '"'$1'"' &&
> + . git-sh-setup &&
> + cd_to_toplevel &&
> + [ "$(pwd -P)" = "$TOPLEVEL" ]
> + )
> + '
> +}
> +
> +TOPLEVEL="$(pwd -P)/repo"Hmm. Does it make sense to assume everybody's pwd can take -P when the primary change this patch introduces carefully avoids assuming the availability of -P for "cd"?
Show 9 quoted lines
> +ln -s repo symrepo > +test_cd_to_toplevel symrepo 'at symbolic root' > + > +ln -s repo/sub/dir subdir-link > +test_cd_to_toplevel subdir-link 'at symbolic subdir' > + > +cd repo > +ln -s sub/dir internal-link > +test_cd_to_toplevel internal-link 'at internal symbolic subdir'
To be very honest, although it is good that you made them work, I am still not getting why the latter two scenarios are worth supporting. The first one I am Ok with, though.
Show 24 quoted lines
> diff --git a/t/t5521-pull-symlink.sh b/t/t5521-pull-symlink.sh > new file mode 100755 > index 0000000..f18fec7 > --- /dev/null > +++ b/t/t5521-pull-symlink.sh > @@ -0,0 +1,67 @@ > +#!/bin/sh > + > +test_description='pulling from symlinked subdir' > + > +. ./test-lib.sh > + > +D=`pwd` > + > +# The scenario we are building: > +# > +# trash\ directory/ > +# clone-repo/ > +# subdir/ > +# bar > +# subdir-link -> clone-repo/subdir/ > +# > +# The working directory is subdir-link. > +#
It is great to see the scenario explained like this. It makes it easier to follow what the tests are trying to do.
Show 31 quoted lines
> +test_expect_success setup ' > + > + mkdir subdir && > + touch subdir/bar && > + git add subdir/bar && > + git commit -m empty && > + git clone . clone-repo && > + # demonstrate that things work without the symlink > + test_debug "cd clone-repo/subdir/ && git pull; cd ../.." && > + ln -s clone-repo/subdir/ subdir-link && > + cd subdir-link/ && > + test_debug "set +x" > +' > + > +# From subdir-link, pulling should work as it does from > +# clone-repo/subdir/. > +# > +# Instead, the error pull gave was: > +# > +# fatal: 'origin': unable to chdir or not a git archive > +# fatal: The remote end hung up unexpectedly > +# > +# because git would find the .git/config for the "trash directory" > +# repo, not for the clone-repo repo. The "trash directory" repo > +# had no entry for origin. Git found the wrong .git because > +# git rev-parse --show-cdup printed a path relative to > +# clone-repo/subdir/, not subdir-link/. Git rev-parse --show-cdup > +# used the correct .git, but when the git pull shell script did > +# "cd `git rev-parse --show-cdup`", it ended up in the wrong > +# directory. Shell "cd" works a little different from chdir() in C. > +# Bash's "cd -P" works like chdir() in C.
This is a very good analysis. s/Bash's "cd -P"/"cd -P" in POSIX shells/, though.
Show 5 quoted lines
> +# > +test_expect_success 'pulling from symlinked subdir' ' > + > + git pull > +'
I'd prefer to see each test_expect_success be able to fail independently, which would mean (1) when you chdir around, do so in a subshell, and (2) each test_expect_success assumes it begins in the same directory.
In the case of these tests, I think it is just the matter of moving the last two lines from the previous test to the beginning of this test and enclosing this test in (), right?
Show 10 quoted lines
> + > +# Prove that the remote end really is a repo, and other commands > +# work fine in this context. > +# > +test_debug " > + test_expect_success 'pushing from symlinked subdir' ' > + > + git push > + ' > +"
Why should this be hidden inside test_debug?