Re: [PATCH] git-p4: Fix multi-path changelist empty commits

[George Vanburgh <george@vanburgh.me>]

On 17 December 2016 01:47:55 CET, Luke Diamand <luke@diamand.org> wrote:
>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.

Not a problem - thanks to you guys for maintaining such an awesome tool!

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

Oops - I'll correct that and resubmit :)

>Possibly need to rewrap the comment line (I think there's a 72
>character limit) ?

Sure - I'll fix that in the resubmission

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