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

[Eric Sunshine]

On Wed, Mar 21, 2018 at 2:11 PM, Junio C Hamano  wrote:
> Pratik Karki  writes:
>>  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.

One other issue I forgot to mention is that the commit message in v3
started getting too wide again[1]; it was fine in v2. Pratik, try to
keep the commit message wrapped to about 70-72 characters or so.

[1]: 
https://public-inbox.org/git/CAPig+cRPzyw525ODC4=-E7w=zbpbhvn2eqxsydslij5wfw8...@mail.gmail.com/

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

[Eric Sunshine]

On Wed, Mar 21, 2018 at 2:11 PM, Junio C Hamano  wrote:
> Pratik Karki  writes:
>> 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.

Thanks, Junio. I had just started writing a review of v3 when your
review arrived, and you covered every point I was going to make, thus
saved me the effort. I agree with everything in your review.

One additional comment, Pratik, is that this patch seems to be based
upon a slightly old version of the Git source code, thus does not
apply cleanly to present-day 'master'. Before re-rolling, update to
the latest Git and rebase your patch atop it.

>> - 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 > + PACK6=$((
>
> I thought that Eric already pointed out and explained why this
> change to PACK6 is wrong in the previous round?

I probably should have been more explicit by naming PACK6 directly.
Comparing v3 against v2, I see that Pratik probably misunderstood my
comment, thinking that I was talking about losing the whitespace
inside PACK5=$(...); v2 dropped that whitespace and v3 restored it.

Pratik, dropping the unnecessary whitespace inside PACK5=$(...) is
fine (no complaint about that), but changing PACK6=$( (...) ) to
PACK6=$((...)) is outright incorrect as explained in [1].

[1]: 
https://public-inbox.org/git/capig+ctkkp6kpfcjfvv8w1ejcrcwqh33mhtgfun+mpmgw5i...@mail.gmail.com/

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

[Junio C Hamano]

Pratik Karki  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  + 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: 

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

[Pratik Karki]

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.


>> test_cmp expect.two output.two
>> @@ -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. If this was the only instance in this test script of
>whitespace needing correction, then it _might_ be okay to include it
>in this patch, however, that's not the case. There are many other such
>instance in this test script which could be corrected, so it doesn't
>make sense to single out these two and ignore all the others.
>Therefore, you should omit this change.

I used tabify in Emacs and it must have messed up the whitespace
change. 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.
If I change this change back to original the 'git diff --check'
reports whitespace identation with space. So, isn't this
supposedly a fix?

[1]: 
https://public-inbox.org/git/20180319173204.31952-1-predatoram...@gmail.com/
[2]: https://github.com/git/git/blob/master/Documentation/SubmittingPatches

- >8

 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.

Signed-off-by: Pratik Karki 
---
 t/t5300-pack-object.sh | 10 +++---
 t/t5510-fetch.sh   |  8 ++---
 t/t7001-mv.sh  | 22 ++--
 t/t7003-filter-branch.sh   |  9 +++--
 t/t9104-git-svn-follow-parent.sh   | 16 +
 t/t9108-git-svn-glob.sh| 14 
 t/t9109-git-svn-multi-glob.sh  | 38 +++--
 t/t9110-git-svn-use-svm-props.sh   | 42 +++
 t/t9111-git-svn-use-svnsync-props.sh   | 36 ++--
 t/t9114-git-svn-dcommit-merge.sh   | 10 +++---
 t/t9130-git-svn-authors-file.sh| 28 +---
 t/t9138-git-svn-authors-prog.sh| 31 -
 t/t9153-git-svn-rewrite-uuid.sh|  8 ++---
 t/t9168-git-svn-partially-globbed-names.sh | 34 +++
 t/t9350-fast-export.sh | 54 --
 15 files changed, 193 insertions(+), 167 deletions(-)

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 &&
+   PACK5=$( git pack-objects test-5 &1 | \
-   grep -e "->" | cut -c 22- >../actual
+   git -c fetch.output=full fetch origin >actual2 2>&1 &&
+   grep -e "->" actual2 | cut -c 22- >../actual
) &&
cat >expect <<-\EOF &&
master   -> origin/master
@@ -708,8 +708,8 @@ test_expect_success C_LOCALE_OUTPUT 'fetch compact output' '
test_commit extraaa &&
(
cd compact &&
-   git -c fetch.output=compact fetch origin 2>&1 | \
-   grep -e "->" | cut -c 22- >../actual
+   git -c fetch.output=compact fetch origin >actual2 2>&1 &&
+   grep -e "->" actual2 | cut -c 22- >../actual
) &&
cat >expect <<-\EOF &&
master -> origin/*
diff --git a/t/t7001-mv.sh b/t/t7001-mv.sh
index 6e5031f56..00aa9e45b 100755
--- a/t/t7001-mv.sh
+++ b/t/t7001-mv.sh
@@ -21,8 +21,8 @@ test_expect_success \
 
 test_expect_success \
 'checking the