Re: [PATCH] git-p4: Fix multi-path changelist empty commits
- From
Luke Diamand <luke@diamand.org>
- Date
- Dec 17, 2016, 00:47 UTC
- Message-ID
- <CAE5ih7-+tahL4=OrW6F6UPKKRg1KFkw32e=pnTx6j2WTZ-BhOw@mail.gmail.com>
- In-Reply-To
- <01020159037a8995-2d1da9d4-4a27-4b98-818b-432fc0ad8a52-000000@eu-west-1.amazonses.com>
On 15 December 2016 at 17:14, George Vanburgh <george@vanburgh.me> wrote:
> From: George Vanburgh <gvanburgh@bloomberg.net>
>
> When importing from multiple perforce paths - we may attempt to import a changelist that contains files from two (or more) of these depot paths. Currently, this results in multiple git commits - one containing the changes, and the other(s) as empty commits. This behavior was introduced in commit 1f90a64 ("git-p4: reduce number of server queries for fetches", 2015-12-19).That's definitely a bug, thanks for spotting that! Even more so for adding a test case.
Show 10 quoted lines
> > Reproduction Steps: > > 1. Have a git repo cloned from a perforce repo using multiple depot paths (e.g. //depot/foo and //depot/bar). > 2. Submit a single change to the perforce repo that makes changes in both //depot/foo and //depot/bar. > 3. Run "git p4 sync" to sync the change from #2. > > Change is synced as multiple commits, one for each depot path that was affected. > > Using a set, instead of a list inside p4ChangesForPaths() ensures that each changelist is unique to the returned list, and therefore only a single commit is generated for each changelist.
The change looks good to me apart from one missing "&&" in the test case (see below).
Possibly need to rewrap the comment line (I think there's a 72 character limit) ?
Luke
Show 51 quoted lines
>
> Reported-by: James Farwell <jfarwell@vmware.com>
> Signed-off-by: George Vanburgh <gvanburgh@bloomberg.net>
> ---
> git-p4.py | 4 ++--
> t/t9800-git-p4-basic.sh | 22 +++++++++++++++++++++-
> 2 files changed, 23 insertions(+), 3 deletions(-)
>
> diff --git a/git-p4.py b/git-p4.py
> index fd5ca52..6307bc8 100755
> --- a/git-p4.py
> +++ b/git-p4.py
> @@ -822,7 +822,7 @@ def p4ChangesForPaths(depotPaths, changeRange, requestedBlockSize):
> die("cannot use --changes-block-size with non-numeric revisions")
> block_size = None
>
> - changes = []
> + changes = set()
>
> # Retrieve changes a block at a time, to prevent running
> # into a MaxResults/MaxScanRows error from the server.
> @@ -841,7 +841,7 @@ def p4ChangesForPaths(depotPaths, changeRange, requestedBlockSize):
>
> # Insert changes in chronological order
> for line in reversed(p4_read_pipe_lines(cmd)):
> - changes.append(int(line.split(" ")[1]))
> + changes.add(int(line.split(" ")[1]))
>
> if not block_size:
> break
> diff --git a/t/t9800-git-p4-basic.sh b/t/t9800-git-p4-basic.sh
> index 0730f18..4d72e0b 100755
> --- a/t/t9800-git-p4-basic.sh
> +++ b/t/t9800-git-p4-basic.sh
> @@ -131,6 +131,26 @@ test_expect_success 'clone two dirs, @all, conflicting files' '
> )
> '
>
> +test_expect_success 'clone two dirs, each edited by submit, single git commit' '
> + (
> + cd "$cli" &&
> + echo sub1/f4 >sub1/f4 &&
> + p4 add sub1/f4 &&
> + echo sub2/f4 >sub2/f4 &&
> + p4 add sub2/f4 &&
> + p4 submit -d "sub1/f4 and sub2/f4"
> + ) &&
> + git p4 clone --dest="$git" //depot/sub1@all //depot/sub2@all &&
> + test_when_finished cleanup_git &&
> + (
> + cd "$git"Missing &&
Show 22 quoted lines
> + git ls-files >lines && > + test_line_count = 4 lines && > + git log --oneline p4/master >lines && > + test_line_count = 5 lines > + ) > +' > + > revision_ranges="2000/01/01,#head \ > 1,2080/01/01 \ > 2000/01/01,2080/01/01 \ > @@ -147,7 +167,7 @@ test_expect_success 'clone using non-numeric revision ranges' ' > ( > cd "$git" && > git ls-files >lines && > - test_line_count = 6 lines > + test_line_count = 8 lines > ) > done > ' > > -- > https://github.com/git/git/pull/311