Pratik Karki <predatoram...@gmail.com> 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?
