Re: [PATCH] git-p4: Use -m when running p4 changes

From: Junio C Hamano <gitster@pobox.com>
Date: Apr 16, 2015, 18:56 UTC
Message-ID: <xmqqfv7zj40w.fsf@gitster.dls.corp.google.com>
In-Reply-To: <CALM2SnafBHz8YeWtUtQDUgLBP_s9AiJy=9UC6XveqP0zrYMEqA@mail.gmail.com>

Lex Spoon <lex@lexspoon.org> writes:

> From 9cc607667a20317c837afd90d50c078da659b72f Mon Sep 17 00:00:00 2001
> From: Lex Spoon <lex@lexspoon.org>
> Date: Sat, 11 Apr 2015 10:01:15 -0400
> Subject: [PATCH] git-p4: Use -m when running p4 changes

All of the above is duplicate and shouldn't be added to the message; the recipient can pick them up from the e-mail headers.

Please explain what this change intends to do (e.g. Is it a fix? If so, what is broken without this change? Is it an enhancement? If so, what cannot be done without this change, and how and why is the new thing the change enables a good thing?), and why it is a good idea to use "-m" to realize that objective.

Show 19 quoted lines

> Signed-off-by: Lex Spoon <lex@lexspoon.org>
> ---
> Updated to include a test case
>
>  git-p4.py               | 51 ++++++++++++++++++++++++++++++---------
>  t/t9818-git-p4-block.sh | 64 +++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 104 insertions(+), 11 deletions(-)
>  create mode 100755 t/t9818-git-p4-block.sh
>
> diff --git a/git-p4.py b/git-p4.py
> index 549022e..2fc8d9c 100755
> --- a/git-p4.py
> +++ b/git-p4.py
> @@ -740,17 +740,43 @@ def
> createOrUpdateBranchesFromOrigin(localRefPrefix = "refs/remotes/p4/",
> silent
>  def originP4BranchesExist():
>          return gitBranchExists("origin") or
> gitBranchExists("origin/p4") or gitBranchExists("origin/p4/master")

It appears that the patch is severely linewrapped.

Show 15 quoted lines

> diff --git a/t/t9818-git-p4-block.sh b/t/t9818-git-p4-block.sh
> new file mode 100755
> index 0000000..73e545d
> --- /dev/null
> +++ b/t/t9818-git-p4-block.sh
> @@ -0,0 +1,64 @@
> +#!/bin/sh
> +
> +test_description='git p4 fetching changes in multiple blocks'
> +
> +. ./lib-git-p4.sh
> +
> +test_expect_success 'start p4d' '
> + start_p4d
> +'

We do not do one-SP indent.  Indent with tab instead.

Show 5 quoted lines

> +
> +test_expect_success 'Create a repo with 100 changes' '
> + (
> + cd "$cli" &&
> + touch file.txt &&

Do not use "touch" when the only thing you are interested in is that the file exists and you do not care about its timestamp. I.e. say

    >file.txt &&

instead.

Show 15 quoted lines

> + p4 add file.txt &&
> + p4 submit -d "Add file.txt" &&
> + for i in 0 1 2 3 4 5 6 7 8 9
> + do
> + touch outer$i.txt &&
> + p4 add outer$i.txt &&
> + p4 submit -d "Adding outer$i.txt" &&
> + for j in 0 1 2 3 4 5 6 7 8 9
> + do
> + p4 edit file.txt &&
> + echo $i$j > file.txt &&
> + p4 submit -d "Commit $i$j"
> + done
> + done
> + )

What happens when any of these commands in the &&-chain fails?

	(
        	cd "$cli" &&
                >file.txt &&
		p4 ... &&
                for i in $(test_seq ...)
                do
                	>"outer$i.txt" &&
                        p4 ... &&
			for j in $(test_seq ...)
			do
				p4 ... &&
				p4 ... || exit
			done
		done
	)

or something like that, perhaps?