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

Pratik Karki [off-list ref] 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.

quoted

quoted

@@ -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/*" &&

quoted

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.

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

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?