Re: [GSoC][PATCH v3] test: avoid pipes in git related commands for test

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Pratik Karki <predatoramigo@xxxxxxxxx> writes:

> Thank you Eric, for the review. This is follow on patch[1].
>
> The changes in patch increased from v1 to v2 because I
> got excited to work in Git codebase and I tried to
> fix the exisiting problems as much as possible.
> Hence, the large number of changes.

Eric understands why the scope was increased between the two; he
explained why it is not a good idea to increase the scope in the
middle, and I tend to agree with his reasoning.  The reason why the
scope was increased does not matter.

>>> @@ -120,18 +122,20 @@ test_expect_success 'test another branch' '
>>>         git config --add svn-remote.four.url "$svnrepo" &&
>>>         git config --add svn-remote.four.fetch trunk:refs/remotes/four/trunk &&
>>>         git config --add svn-remote.four.branches \
>>> -                        "branches/*/*:refs/remotes/four/branches/*/*" &&
>>> +                        "branches/*/*:refs/remotes/four/branches/*/*" &&
>>>         git config --add svn-remote.four.tags \
>>> -                        "tags/*:refs/remotes/four/tags/*" &&
>>> +                        "tags/*:refs/remotes/four/tags/*" &&
>
>>I guess you sneaked in a whitespace change here which is unrelated to
>>the stated purpose of this patch, thus acted as a speed bump during
>>review....
>>Therefore, you should omit this change.
>
> I used tabify in Emacs and it must have messed up the whitespace
> change.

Then don't.  Make sure the lines _you_ change are indented and
formatted correctly.  Do not touch near-by (or far-away for that
matter) lines that you do not have to touch only to change the
formatting.

> I read SubmittingPatches guideline[2] for git where it
> is said that whitespace check must be done and git community is
> picky about it and 'git diff --check' must be done before commit.

Yes.

> If I change this change back to original the 'git diff --check'
> reports whitespace identation with space.

I do not think 'diff --check' would.  Save the patch you sent to a
file, edit it so that these two lines Eric pointed out are not
changed, and then apply it with "git apply --whitespace=nowarn".
Then ask "git diff --check"---it should not complain about an
existing whitespace glitch that you did not introduce.

> So, isn't this
> supposedly a fix?

Unless it is a "here is a patch to reindent and fix whitespaces"
patch that does nothing else, it is an unwelcome noise that
distracts reviewers from the real changes.

> ------------------------------------- >8----------------------------------------

This is not wrong per se, but just a

-- >8 --

is sufficient ;-)

>
>  Avoid using pipes downstream of Git commands since the exit
>  codes of commands upstream of pipes get swallowed, thus potentially hiding
>  failure of those commands. Instead, capture Git command output to a file and
>  apply the downstream command(s) to that file.

Please do not indent the body of the log message by one space.

> diff --git a/t/t5300-pack-object.sh b/t/t5300-pack-object.sh
> index 9c68b9925..707208284 100755
> --- a/t/t5300-pack-object.sh
> +++ b/t/t5300-pack-object.sh
> @@ -311,9 +311,9 @@ test_expect_success 'unpacking with --strict' '
>  	rm -f .git/index &&
>  	tail -n 10 LIST | git update-index --index-info &&
>  	ST=$(git write-tree) &&
> -	PACK5=$( git rev-list --objects "$LIST" "$LI" "$ST" | \
> -		git pack-objects test-5 ) &&
> -	PACK6=$( (
> +	git rev-list --objects "$LIST" "$LI" "$ST" >actual &&
> +	PACK5=$( git pack-objects test-5 <actual ) &&
> +	PACK6=$((

I thought that Eric already pointed out and explained why this
change to PACK6 is wrong in the previous round?

> diff --git a/t/t9111-git-svn-use-svnsync-props.sh b/t/t9111-git-svn-use-svnsync-props.sh
> index 22b6e5ee7..a4225c9f6 100755
> --- a/t/t9111-git-svn-use-svnsync-props.sh
> +++ b/t/t9111-git-svn-use-svnsync-props.sh
> @@ -20,32 +20,32 @@ uuid=161ce429-a9dd-4828-af4a-52023f968c89
>  
>  bar_url=http://mayonaise/svnrepo/bar
>  test_expect_success 'verify metadata for /bar' "
> -	git cat-file commit refs/remotes/bar | \
> -	   grep '^git-svn-id: $bar_url@12 $uuid$' &&
> -	git cat-file commit refs/remotes/bar~1 | \
> -	   grep '^git-svn-id: $bar_url@11 $uuid$' &&
> -	git cat-file commit refs/remotes/bar~2 | \
> -	   grep '^git-svn-id: $bar_url@10 $uuid$' &&
> -	git cat-file commit refs/remotes/bar~3 | \
> -	   grep '^git-svn-id: $bar_url@9 $uuid$' &&
> -	git cat-file commit refs/remotes/bar~4 | \
> -	   grep '^git-svn-id: $bar_url@6 $uuid$' &&
> -	git cat-file commit refs/remotes/bar~5 | \
> -	   grep '^git-svn-id: $bar_url@1 $uuid$'
> +	git cat-file commit refs/remotes/bar >actual &&
> +	grep '^git-svn-id: $bar_url@12 $uuid$' actual &&
> +	git cat-file commit refs/remotes/bar~1 >actual1 &&
> +	grep '^git-svn-id: $bar_url@11 $uuid$' actual1 &&
> +	git cat-file commit refs/remotes/bar~2 >actual2 &&
> +	grep '^git-svn-id: $bar_url@10 $uuid$' actual2 &&
> +	git cat-file commit refs/remotes/bar~3 >actual3 &&
> +	grep '^git-svn-id: $bar_url@9 $uuid$' actual3 &&
> +	git cat-file commit refs/remotes/bar~4 >actual4 &&
> +	grep '^git-svn-id: $bar_url@6 $uuid$' actual4 &&
> +	git cat-file commit refs/remotes/bar~5 >actual5 &&
> +	grep '^git-svn-id: $bar_url@1 $uuid$' actual5
>  	"

I also thought that Eric already pointed out that the above is not a
good idea because it forces readers to wonder if "actual[1-5]" need
to exist together with "actual" at the same time in the previous
round?




[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]

  Powered by Linux