Re: [PATCH 2/2] git-svn: test for git-svn prefixed globs
- From
- Eric Wong <normalperson@yhbt.net>
- Date
- Dec 16, 2015, 21:28 UTC
- 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.
Show 8 quoted lines
> --- /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.
Show 5 quoted lines
> +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.