git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Junio C Hamano <gitster@pobox.com>
To: Pratik Karki <predatoramigo@gmail.com>
Cc: Git List <git@vger.kernel.org>, Eric Sunshine <sunshine@sunshineco.com>
Subject: Re: [GSoC][PATCH v3] test: avoid pipes in git related commands for test
Date: Wed, 21 Mar 2018 11:11:06 -0700	[thread overview]
Message-ID: <xmqqo9jhpadh.fsf@gitster-ct.c.googlers.com> (raw)
In-Reply-To: <20180321152356.10754-1-predatoramigo@gmail.com> (Pratik Karki's message of "Wed, 21 Mar 2018 21:08:56 +0545")

Pratik Karki <predatoramigo@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?


  reply	other threads:[~2018-03-21 18:11 UTC|newest]

Thread overview: 16+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-03-13 20:19 [GSoC] [PATCH] test: avoid pipes in git related commands for test suite Pratik Karki
2018-03-14  7:30 ` Eric Sunshine
2018-03-14  9:57   ` Ævar Arnfjörð Bjarmason
2018-03-14 18:22     ` Eric Sunshine
2018-03-15 17:04       ` Junio C Hamano
2018-03-19 17:32         ` [GSoC][PATCH] " Pratik Karki
2018-03-21 11:02           ` Eric Sunshine
2018-03-21 15:23             ` [GSoC][PATCH v3] test: avoid pipes in git related commands for test Pratik Karki
2018-03-21 18:11               ` Junio C Hamano [this message]
2018-03-21 18:45                 ` Eric Sunshine
2018-03-21 18:58                 ` Eric Sunshine
2018-03-23 15:01                   ` [GSoC][PATCH v4] " Pratik Karki
2018-03-25  8:37                     ` Eric Sunshine
2018-03-27 17:31                       ` [GSoC][PATCH v5] " Pratik Karki
2018-03-30 21:45                         ` Eric Sunshine
2018-03-30 22:08                           ` Junio C Hamano

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=xmqqo9jhpadh.fsf@gitster-ct.c.googlers.com \
    --to=gitster@pobox.com \
    --cc=git@vger.kernel.org \
    --cc=predatoramigo@gmail.com \
    --cc=sunshine@sunshineco.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).