On Sat, Dec 29, 2018 at 12:34 AM Junio C Hamano <[email protected]> wrote:
>
> [email protected] writes:
>
> > Subject: Re: [PATCH 1/2] t5403: Refactor
>
> Hmph. "Refactor" alone leaves readers wondering "refactor what?",
> "refactor for what?" and "refactor how?". Given that the overfall
> goal this change seeks seems to be to simplify it by losing about 20
> lines, how about justifying it like so?
>
> Subject: t5403: simplify by using a single repository
>
> There is no strong reason to use separate clones to run
> these tests; just use a single test repository prepared
> with more modern test_commit shell helper function.
>
> While at it, replace three "awk '{print $N}'" on the same
> file with shell built-in "read" into three variables.
Done
[snip]
> > + mv .git/hooks-disabled .git/hooks &&
>
> I am not sure why you want to do this---it sends a wrong signal to
> readers saying that you want to use whatever hook that are in the
> moved-away .git/hooks-disabled/ directory. I am guessing that the
> only reason why you do this is because there must be .git/hooks
> directory in order for write_script below to work, so a more
> readable approach would be to "mkdir .git/hooks" instead, no?
Agreed. Done.
> > + write_script .git/hooks/post-checkout <<-\EOF &&
> > + echo $@ >.git/post-checkout.args
>
> A dollar-at inside a shell script that is not in a pair of dq always
> makes readers wonder if the author forgot dq around it or omitted eq
> around it deliberately; avoid it.
>
> In this case, use "$@" (i.e. within dq) would be more friendly to
> readers.
Done.
[snip]
> > + EOF
> > + test_commit one &&
> > + test_commit two &&
> > + test_commit three three
>
> Makes readers wonder why the last one duplicates. Is this because
> you somehow do not want to use three.t as the pathname in a later
> test? "test_commit X" that creates test file X.t is a quite well
> established convention (see "git grep '\.t\>' t/"), by the way.
Done. I wasn't aware of that.
[snip]
> > + test -e .git/post-checkout.args &&
>
> Use "test -f", as you do know you'd be creating a file ("-e"
> succeeds as long as it _exists_, and does not care if it is a file
> or directory or whatever).
Just removed these `test -e` lines. read fails anyway if the file doesn't exist.
> > + read old new flag <.git/post-checkout.args &&
>
> This indeed is much nicer.
Credit goes to Johannes :)
> > + test $old = $new && test $flag = 1 &&
> > + rm -f .git/post-checkout.args
>
> The last one is not a test but a clean-up. If any of the earlier
> step failed (e.g. $old and $new were different), the output file
> would be left behind, resulting in confusing the next test.
>
> Instead, do it like so:
>
> test_expect_success 'title of the test' '
> test_when_finished "rm -f .git/post-checkout.args" &&
> git checkout master &&
> test -f .git/post-checkout.args &&
> read old new flag <.git/post-checkout.args &&
> test $old = $new &&
> test $flag = 1
> '
>
> That is, use test_when_finished() before the step that creates the
> file that may be left behind to arrange that it will be cleaned at
> the end.
>
> This comment on clean-up applies to _all_ tests in this patch that
> has "rm -f .git/post-checkout.args" at the end.
Done
> > test_expect_success 'post-checkout runs as expected ' '
> > - GIT_DIR=clone1/.git git checkout master &&
> > - test -e clone1/.git/post-checkout.args
> > + git checkout master &&
> > + test -e .git/post-checkout.args &&
> > + rm -f .git/post-checkout.args
> > '
>
> Now that the script got so simplified, this one looks even more
> redundant, given that the previous one already checked the same
> "checkout 'master' when already at the tip of 'master'" situation.
>
> Do we still need this one, in other words?
I agree. Removed.
> > test_expect_success 'post-checkout args are correct with git checkout -b '
> > '
> > - GIT_DIR=clone1/.git git checkout -b new1 &&
> > - old=$(awk "{print \$1}" clone1/.git/post-checkout.args) &&
> > - new=$(awk "{print \$2}" clone1/.git/post-checkout.args) &&
> > - flag=$(awk "{print \$3}" clone1/.git/post-checkout.args) &&
> > - test $old = $new && test $flag = 1
> > + git checkout -b new1 &&
> > + read old new flag <.git/post-checkout.args &&
> > + test $old = $new && test $flag = 1 &&
> > + rm -f .git/post-checkout.args
> > '
>
> This one forgets "did the hook run and create the file" before
> "read", unlike the previous tests. It is not strictly necessary as
> "read" will fail if the file is not there, but it'd be better to be
> consistent.
Made consistent by removing file existence test, and left only read.
> > if test "$(git config --bool core.filemode)" = true; then
>
> This is a tangent but this conditional came from an ancient d42ec126
> ("disable post-checkout test on Cygwin", 2009-03-17) that says
>
> disable post-checkout test on Cygwin
>
> It is broken because of the tricks we have to play with
> lstat to get the bearable perfomance out of the call.
> Sadly, it disables access to Cygwin's executable attribute,
> which Windows filesystems do not have at all.
>
> I wonder if this is still relevant these days (Cc'ed Ramsay for
> input). Windows port should be running enabled hooks (and X_OK is
> made pretty much no-op in compat/mingw.c IIUC), so the above
> conditional is overly broad anyway, even if Cygwin still has issues
> with the executable bit.
Reverted.
Thanks,
- Orgad
