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

[Luke Diamand <luke@diamand.org>]

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>

> 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.

> + 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).

> + 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 ";"

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

As above.

> + 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