Web lists-archives.com

Re: [PATCH 1/2] t5403: Refactor




On Sat, Dec 29, 2018 at 12:34 AM Junio C Hamano <gitster@xxxxxxxxx> wrote:
>
> orgads@xxxxxxxxx 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