From: Eric Wong <normalperson@yhbt.net>
Date: Wed, 16 Dec 2015 21:28:11 GMT
Subject: Re: [PATCH 2/2] git-svn: test for git-svn prefixed globs
Message-ID: <20151216212811.GA19884@dcvr.yhbt.net>
In-Reply-To: <1450270869-29822-3-git-send-email-vleschuk@accesssoftek.com>

Thanks for this work.  Most things look fine with 1/2, comments
on 2/2 below...

Victor Leschuk <vleschuk@gmail.com> wrote:
> Add test for git-svn prefixed globs.

Why a separate patch?  Unless there's some documentation purpose
for a regression, usually tests and a feature should be added
atomically in the same commit.

> --- /dev/null
> +++ b/t/t9168-git-svn-prefixed-glob.sh
> @@ -0,0 +1,136 @@
> +#!/bin/sh
> +test_description='git svn globbing refspecs with prefixed globs'
> +. ./lib-git-svn.sh
> +
> +cat > expect.end <<EOF

We prefer redirects in new code to be in the form of ">foo" (no space)
(or ">>foo" for append).

It wasn't in the old tests, either, but Documentation/CodingGuidelines
favors this for new code.

> +the end
> +hi
> +start a new branch
> +initial
> +EOF

All the setup code be checked for errors with '&&' as well.

> +	test "`git rev-parse refs/remotes/tags/t_end~1`" = \
> +		"`git rev-parse refs/remotes/branches/b_start`" &&
> +	test "`git rev-parse refs/remotes/branches/b_start~2`" = \
> +		"`git rev-parse refs/remotes/trunk`" &&

And we prefer $(command) instead of `command` for nestability
as Documentation/CodingGuidelines suggests.

(yeah, most of the old tests don't follow the guidelines, but the
 guidelines also warn against fixup patches for them).

Thanks again.