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

From: Luke Diamand <luke@diamand.org>
Date: Apr 16, 2015, 23:15 UTC
Message-ID: <55304290.9070907@diamand.org>
In-Reply-To: <CALM2SnafBHz8YeWtUtQDUgLBP_s9AiJy=9UC6XveqP0zrYMEqA@mail.gmail.com>

On 15/04/15 04:47, Lex Spoon wrote:

>  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

This patch didn't want to apply for me, I'm not quite sure why but possibly it's become scrambled? Either that or I'm doing it wrong! If you use git send-email it should Just Work.

As an aside could you post reworked versions of patches with a subject line of [PATCH v2], [PATCH v3], etc, so reviewers can keep track of what's going on?

Note to other reviewers: the existing git-p4 has a --max-changes option for 'sync', but this doesn't do the same thing at all. It doesn't limit the number of changes requested from the server, it just limits the number of changes pulled down, after the p4 server has supplied those changes. This confused me at first!

Lex - I should have mentioned this before, but would you be able to add some documentation to Documentation/git-p4.txt to explain what your new option does? It would help to distinguish between your option and the existing --max-changes option.

I've put a few remarks below in your shell script; there are a few minor issues that could do with being tidied up.

Thanks! Luke

<snip>

Show 19 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
> +'
> +
> +test_expect_success 'Create a repo with 100 changes' '
> + (
> + cd "$cli" &&

This doesn't look like enough indentation. The tests normally get a hard tab indent at each level.

Show 12 quoted lines

> + touch file.txt &&
> + 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 &&

Please put the file argument immediately after the redirection, i.e.

    echo $i$j >file.txt &&

(Which you've done below in fact).

Show 27 quoted lines

> + p4 submit -d "Commit $i$j"
> + done
> + done
> + )
> +'
> +
> +test_expect_success 'Clone the repo' '
> + git p4 clone --dest="$git" --changes-block-size=10 --verbose //depot@all
> +'
> +
> +test_expect_success 'All files are present' '
> + echo file.txt >expected &&
> + test_write_lines outer0.txt outer1.txt outer2.txt outer3.txt
> outer4.txt >>expected &&
> + test_write_lines outer5.txt outer6.txt outer7.txt outer8.txt
> outer9.txt >>expected &&
> + ls "$git" >current &&
> + test_cmp expected current
> +'
> +
> +test_expect_success 'file.txt is correct' '
> + echo 99 >expected &&
> + test_cmp expected "$git/file.txt"
> +'
> +
> +test_expect_success 'Correct number of commits' '
> + (cd "$git"; git log --oneline) >log &&

Use "&&" rather than ";"

Show 5 quoted lines

> + test_line_count = 111 log
> +'
> +
> +test_expect_success 'Previous version of file.txt is correct' '
> + (cd "$git"; git checkout HEAD^^) &&

As above.

Show 10 quoted lines

> + echo 97 >expected &&
> + test_cmp expected "$git/file.txt"
> +'
> +
> +test_expect_success 'kill p4d' '
> + kill_p4d
> +'
> +
> +test_done
>

Looks good other than that (+Junio's comments).

Thanks! Luke