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?
next prev parent 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).