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

2015-04-17 Thread Lex Spoon 
Thanks, all. I will update the patch as requested and resend a [PATCH
v3]. This time without the redundant headers. I will also make an
extra effort to make sure that the raw tabs do not get converted to
spaces this time. Oof, I am really out of practice at programming with
raw tabs, much less getting them to make it through email software.
Thank you for your patience.

test_seq is a neat utility. Also, I don't know why I didn't think to
update the document page. Certainly it needs to be updated.


Lex Spoon
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2015-04-16 Thread Junio C Hamano 
Lex Spoon l...@lexspoon.org writes:

 From 9cc607667a20317c837afd90d50c078da659b72f Mon Sep 17 00:00:00 2001
 From: Lex Spoon l...@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.

 Signed-off-by: Lex Spoon l...@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.

 diff --git a/t/t9818-git-p4-block.sh b/t/t9818-git-p4-block.sh
 new file mode 100755
 index 000..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.

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

 + 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?
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2015-04-16 Thread Luke Diamand 

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

 From 9cc607667a20317c837afd90d50c078da659b72f Mon Sep 17 00:00:00 2001
From: Lex Spoon l...@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 000..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


--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2015-04-14 Thread Luke Diamand 
On 11 April 2015 at 16:17, Lex Spoon l...@lexspoon.org wrote:


 Signed-off-by: Lex Spoon l...@lexspoon.org
 ---
 This patch addresses a problem I am running into with a client. I am
 attempting to mirror their Perforce repository into Git, and on certain
 branches their Perforce server is responding with an error about too many
 rows scanned. This change has git-p4 use the -m option to return just 500
 changes at a time, thus avoiding the problem.

Thanks - that's a problem I also occasionally hit, and it definitely
needs fixing.

Your fix is quite nice - I started out thinking this should be easy,
but it's not!

A test case addition would be good if you can though - otherwise it's
certain to break at some point in the future. Would you have time to
add that?

Thanks!
Luke



 I have tested this on a small test repository (2000 revisions) and it
 appears to work fine. I have also run all the t98* tests; those print a
 number of yellow not ok results but no red ones. I presume this is the
 expected test behavior?

Yes.


 I considered making the block size configurable, but it seems unlikely
 anyone will strongly benefit from changing it. 500 is large enough that it
 should only take a modest number of iterations to scan the full changes
 list, but it's small enough that any reasonable Perforce server should allow
 the request.

Might be useful when making test harnesses though :-)



 This patch is also available on GitHub:
 https://github.com/lexspoon/git/tree/p4-sync-batches

  git-p4.py | 40 +---
  1 file changed, 33 insertions(+), 7 deletions(-)

 diff --git a/git-p4.py b/git-p4.py
 index 549022e..ce1447b 100755
 --- a/git-p4.py
 +++ b/git-p4.py
 @@ -742,15 +742,41 @@ def originP4BranchesExist():

  def p4ChangesForPaths(depotPaths, changeRange):
  assert depotPaths
 -cmd = ['changes']
 -for p in depotPaths:
 -cmd += [%s...%s % (p, changeRange)]
 -output = p4_read_pipe_lines(cmd)

 +# Parse the change range into start and end
 +if changeRange is None or changeRange == '':
 +changeStart = '@1'
 +changeEnd = '#head'
 +else:
 +parts = changeRange.split(',')
 +assert len(parts) == 2
 +changeStart = parts[0]
 +changeEnd = parts[1]
 +
 +# Accumulate change numbers in a dictionary to avoid duplicates
  changes = {}
 -for line in output:
 -changeNum = int(line.split( )[1])
 -changes[changeNum] = True
 +
 +for p in depotPaths:
 +# Retrieve changes a block at a time, to prevent running
 +# into a MaxScanRows error from the server.
 +block_size = 500
 +start = changeStart
 +end = changeEnd
 +get_another_block = True
 +while get_another_block:
 +new_changes = []
 +cmd = ['changes']
 +cmd += ['-m', str(block_size)]
 +cmd += [%s...%s,%s % (p, start, end)]
 +for line in p4_read_pipe_lines(cmd):
 +changeNum = int(line.split( )[1])
 +new_changes.append(changeNum)
 +changes[changeNum] = True
 +if len(new_changes) == block_size:
 +get_another_block = True
 +end = '@' + str(min(new_changes))
 +else:
 +get_another_block = False

  changelist = changes.keys()
  changelist.sort()
 --
 1.9.1

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

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

Signed-off-by: Lex Spoon l...@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)

-def p4ChangesForPaths(depotPaths, changeRange):
+def p4ChangesForPaths(depotPaths, changeRange, block_size):
 assert depotPaths
-cmd = ['changes']
-for p in depotPaths:
-cmd += [%s...%s % (p, changeRange)]
-output = p4_read_pipe_lines(cmd)
+assert block_size
+
+# Parse the change range into start and end
+if changeRange is None or changeRange == '':
+changeStart = '@1'
+changeEnd = '#head'
+else:
+parts = changeRange.split(',')
+assert len(parts) == 2
+changeStart = parts[0]
+changeEnd = parts[1]

+# Accumulate change numbers in a dictionary to avoid duplicates
 changes = {}
-for line in output:
-changeNum = int(line.split( )[1])
-changes[changeNum] = True
+
+for p in depotPaths:
+# Retrieve changes a block at a time, to prevent running
+# into a MaxScanRows error from the server.
+start = changeStart
+end = changeEnd
+get_another_block = True
+while get_another_block:
+new_changes = []
+cmd = ['changes']
+cmd += ['-m', str(block_size)]
+cmd += [%s...%s,%s % (p, start, end)]
+for line in p4_read_pipe_lines(cmd):
+changeNum = int(line.split( )[1])
+new_changes.append(changeNum)
+changes[changeNum] = True
+if len(new_changes) == block_size:
+get_another_block = True
+end = '@' + str(min(new_changes))
+else:
+get_another_block = False

 changelist = changes.keys()
 changelist.sort()
@@ -1912,6 +1938,8 @@ class P4Sync(Command, P4UserMap):
 optparse.make_option(--import-local,
dest=importIntoRemotes, action=store_false,
  help=Import into refs/heads/ ,
not refs/remotes),
 optparse.make_option(--max-changes, dest=maxChanges),
+optparse.make_option(--changes-block-size,
dest=changes_block_size, type=int,
+ help=Block size for calling p4 changes),
 optparse.make_option(--keep-path,
dest=keepRepoPath, action='store_true',
  help=Keep entire
BRANCH/DIR/SUBDIR prefix during import),
 optparse.make_option(--use-client-spec,
dest=useClientSpec, action='store_true',
@@ -1940,6 +1968,7 @@ class P4Sync(Command, P4UserMap):
 self.syncWithOrigin = True
 self.importIntoRemotes = True
 self.maxChanges = 
+self.changes_block_size = 500
 self.keepRepoPath = False
 self.depotPaths = None
 self.p4BranchesInGit = []
@@ -2578,7 +2607,7 @@ class P4Sync(Command, P4UserMap):

 return 

-def importNewBranch(self, branch, maxChange):
+def importNewBranch(self, branch, maxChange, changes_block_size):
 # make fast-import flush all changes to disk and update the
refs using the checkpoint
 # command so that we can try to find the branch parent in the
git history
 self.gitStream.write(checkpoint\n\n);
@@ -2586,7 +2615,7 @@ class P4Sync(Command, P4UserMap):
 branchPrefix = self.depotPaths[0] + branch + /
 range = @1,%s % maxChange
 #print prefix + branchPrefix
-changes = p4ChangesForPaths([branchPrefix], range)
+changes = p4ChangesForPaths([branchPrefix], range, changes_block_size)
 if len(changes) = 0:
 return False
 firstChange = changes[0]
@@ -3002,7 +3031,7 @@ class P4Sync(Command, P4UserMap):
 if self.verbose:
 print Getting p4 changes for %s...%s % (',
'.join(self.depotPaths),
   self.changeRange)
-changes = p4ChangesForPaths(self.depotPaths, self.changeRange)
+changes = p4ChangesForPaths(self.depotPaths,
self.changeRange, self.changes_block_size)

 if len(self.maxChanges)  0:
 changes = changes[:min(int(self.maxChanges),

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

2015-04-14 Thread Lex Spoon 
(resending with accidental HTML removed)


Great, I'm glad it looks like a good approach!

I'll add a test case for it and to support the test case, an
option for the block size. I guess the block-size option will go on
sync, clone, and fetch. Alternatively, maybe someone has a
better suggestion of how to configure the block size.

Lex Spoon
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html