[PATCHv2 2/6] git-p4: add option to disable syncing of p4/master with p4

[Luke Diamand]

Add an option to the git-p4 submit command to disable syncing
with Perforce.

This is useful for the case where a git-p4 mirror has been setup
on a server somewhere, running from (e.g.) cron, and developers
then clone from this. Having the local cloned copy also sync
from Perforce just isn't useful.

Signed-off-by: Luke Diamand 
---
 Documentation/git-p4.txt |  8 
 git-p4.py| 31 ---
 2 files changed, 28 insertions(+), 11 deletions(-)

diff --git a/Documentation/git-p4.txt b/Documentation/git-p4.txt
index 3d83842e47..f0de3b891b 100644
--- a/Documentation/git-p4.txt
+++ b/Documentation/git-p4.txt
@@ -369,6 +369,11 @@ These options can be used to modify 'git p4 submit' 
behavior.
 Disable the automatic rebase after all commits have been successfully
 submitted. Can also be set with git-p4.disableRebase.
 
+--disable-p4sync::
+Disable the automatic sync of p4/master from Perforce after commits have
+been submitted. Implies --disable-rebase. Can also be set with
+git-p4.disableP4Sync. Sync with origin/master still goes ahead if possible.
+
 Rebase options
 ~~
 These options can be used to modify 'git p4 rebase' behavior.
@@ -693,6 +698,9 @@ git-p4.conflict::
 git-p4.disableRebase::
 Do not rebase the tree against p4/master following a submit.
 
+git-p4.disableP4Sync::
+Do not sync p4/master with Perforce following a submit. Implies 
git-p4.disableRebase.
+
 IMPLEMENTATION DETAILS
 --
 * Changesets from p4 are imported using Git fast-import.
diff --git a/git-p4.py b/git-p4.py
index 5ab9421af8..b61f47cc61 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -1368,7 +1368,9 @@ def __init__(self):
  help="submit only the specified 
commit(s), one commit or xxx..xxx"),
 optparse.make_option("--disable-rebase", 
dest="disable_rebase", action="store_true",
  help="Disable rebase after submit is 
completed. Can be useful if you "
- "work from a local git branch that is not 
master")
+ "work from a local git branch that is not 
master"),
+optparse.make_option("--disable-p4sync", 
dest="disable_p4sync", action="store_true",
+ help="Skip Perforce sync of p4/master 
after submit or shelve"),
 ]
 self.description = "Submit changes from git to the perforce depot."
 self.usage += " [name of git branch to submit into perforce depot]"
@@ -1380,6 +1382,7 @@ def __init__(self):
 self.update_shelve = list()
 self.commit = ""
 self.disable_rebase = gitConfigBool("git-p4.disableRebase")
+self.disable_p4sync = gitConfigBool("git-p4.disableP4Sync")
 self.prepare_p4_only = False
 self.conflict_behavior = None
 self.isWindows = (platform.system() == "Windows")
@@ -2240,11 +2243,14 @@ def run(self, args):
 sync = P4Sync()
 if self.branch:
 sync.branch = self.branch
-sync.run([])
+if self.disable_p4sync:
+sync.sync_origin_only()
+else:
+sync.run([])
 
-if self.disable_rebase is False:
-rebase = P4Rebase()
-rebase.rebase()
+if not self.disable_rebase:
+rebase = P4Rebase()
+rebase.rebase()
 
 else:
 if len(applied) == 0:
@@ -3324,6 +3330,14 @@ def importChanges(self, changes, shelved=False, 
origin_revision=0):
 print self.gitError.read()
 sys.exit(1)
 
+def sync_origin_only(self):
+if self.syncWithOrigin:
+self.hasOrigin = originP4BranchesExist()
+if self.hasOrigin:
+if not self.silent:
+print 'Syncing with origin first, using "git fetch origin"'
+system("git fetch origin")
+
 def importHeadRevision(self, revision):
 print "Doing initial import of %s from revision %s into %s" % (' 
'.join(self.depotPaths), revision, self.branch)
 
@@ -3402,12 +3416,7 @@ def run(self, args):
 else:
 self.refPrefix = "refs/heads/p4/"
 
-if self.syncWithOrigin:
-self.hasOrigin = originP4BranchesExist()
-if self.hasOrigin:
-if not self.silent:
-print 'Syncing with origin first, using "git fetch origin"'
-system("git fetch origin")
+self.sync_origin_only()
 
 branch_arg_given = bool(self.branch)
 if len(self.branch) == 0:
-- 
2.17.0.392.gdeb1a6e9b7

[PATCHv2 1/6] git-p4: disable-rebase: allow setting this via configuration

[Luke Diamand]

This just lets you set the --disable-rebase option with the
git configuration options git-p4.disableRebase. If you're
using this option, you probably want to set it all the time
for a given repo.

Signed-off-by: Luke Diamand 
---
 Documentation/git-p4.txt | 5 -
 git-p4.py| 2 +-
 2 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/Documentation/git-p4.txt b/Documentation/git-p4.txt
index e8452528fc..3d83842e47 100644
--- a/Documentation/git-p4.txt
+++ b/Documentation/git-p4.txt
@@ -367,7 +367,7 @@ These options can be used to modify 'git p4 submit' 
behavior.
 
 --disable-rebase::
 Disable the automatic rebase after all commits have been successfully
-submitted.
+submitted. Can also be set with git-p4.disableRebase.
 
 Rebase options
 ~~
@@ -690,6 +690,9 @@ git-p4.conflict::
Specify submit behavior when a conflict with p4 is found, as per
--conflict.  The default behavior is 'ask'.
 
+git-p4.disableRebase::
+Do not rebase the tree against p4/master following a submit.
+
 IMPLEMENTATION DETAILS
 --
 * Changesets from p4 are imported using Git fast-import.
diff --git a/git-p4.py b/git-p4.py
index c4581d20a6..5ab9421af8 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -1379,7 +1379,7 @@ def __init__(self):
 self.shelve = False
 self.update_shelve = list()
 self.commit = ""
-self.disable_rebase = False
+self.disable_rebase = gitConfigBool("git-p4.disableRebase")
 self.prepare_p4_only = False
 self.conflict_behavior = None
 self.isWindows = (platform.system() == "Windows")
-- 
2.17.0.392.gdeb1a6e9b7

[PATCHv2 5/6] git-p4: narrow the scope of exceptions caught when parsing an int

[Luke Diamand]

The current code traps all exceptions around some code which parses an
integer, and then talks to Perforce.

That can result in errors from Perforce being ignored. Change the code
to only catch the integer conversion exceptions.

Signed-off-by: Luke Diamand 
---
 git-p4.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/git-p4.py b/git-p4.py
index f4b5deeb83..5f59feb5bb 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -959,7 +959,7 @@ def p4ChangesForPaths(depotPaths, changeRange, 
requestedBlockSize):
 try:
 (changeStart, changeEnd) = p4ParseNumericChangeRange(parts)
 block_size = chooseBlockSize(requestedBlockSize)
-except:
+except ValueError:
 changeStart = parts[0][1:]
 changeEnd = parts[1]
 if requestedBlockSize:
-- 
2.17.0.392.gdeb1a6e9b7

[PATCHv2 6/6] git-p4: auto-size the block

[Luke Diamand]

git-p4 originally would fetch changes in one query. On large repos this
could fail because of the limits that Perforce imposes on the number of
items returned and the number of queries in the database.

To fix this, git-p4 learned to query changes in blocks of 512 changes,
However, this can be very slow - if you have a few million changes,
with each chunk taking about a second, it can be an hour or so.

Although it's possible to tune this value manually with the
"--changes-block-size" option, it's far from obvious to ordinary users
that this is what needs doing.

This change alters the block size dynamically by looking for the
specific error messages returned from the Perforce server, and reducing
the block size if the error is seen, either to the limit reported by the
server, or to half the current block size.

That means we can start out with a very large block size, and then let
it automatically drop down to a value that works without error, while
still failing correctly if some other error occurs.

Signed-off-by: Luke Diamand 
---
 git-p4.py   | 27 +--
 t/t9818-git-p4-block.sh |  8 
 2 files changed, 29 insertions(+), 6 deletions(-)

diff --git a/git-p4.py b/git-p4.py
index 5f59feb5bb..0354d4df5c 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -47,8 +47,8 @@ def __str__(self):
 # Only labels/tags matching this will be imported/exported
 defaultLabelRegexp = r'[a-zA-Z0-9_\-.]+$'
 
-# Grab changes in blocks of this many revisions, unless otherwise requested
-defaultBlockSize = 512
+# The block size is reduced automatically if required
+defaultBlockSize = 1<<20
 
 p4_access_checked = False
 
@@ -969,7 +969,8 @@ def p4ChangesForPaths(depotPaths, changeRange, 
requestedBlockSize):
 changes = set()
 
 # Retrieve changes a block at a time, to prevent running
-# into a MaxResults/MaxScanRows error from the server.
+# into a MaxResults/MaxScanRows error from the server. If
+# we _do_ hit one of those errors, turn down the block size
 
 while True:
 cmd = ['changes']
@@ -983,10 +984,24 @@ def p4ChangesForPaths(depotPaths, changeRange, 
requestedBlockSize):
 for p in depotPaths:
 cmd += ["%s...@%s" % (p, revisionRange)]
 
+# fetch the changes
+try:
+result = p4CmdList(cmd, errors_as_exceptions=True)
+except P4RequestSizeException as e:
+if not block_size:
+block_size = e.limit
+elif block_size > e.limit:
+block_size = e.limit
+else:
+block_size = max(2, block_size // 2)
+
+if verbose: print("block size error, retrying with block size 
{0}".format(block_size))
+continue
+except P4Exception as e:
+die('Error retrieving changes description 
({0})'.format(e.p4ExitCode))
+
 # Insert changes in chronological order
-for entry in reversed(p4CmdList(cmd)):
-if entry.has_key('p4ExitCode'):
-die('Error retrieving changes descriptions 
({})'.format(entry['p4ExitCode']))
+for entry in reversed(result):
 if not entry.has_key('change'):
 continue
 changes.add(int(entry['change']))
diff --git a/t/t9818-git-p4-block.sh b/t/t9818-git-p4-block.sh
index 8840a183ac..ce7cb22ad3 100755
--- a/t/t9818-git-p4-block.sh
+++ b/t/t9818-git-p4-block.sh
@@ -129,6 +129,7 @@ test_expect_success 'Create a repo with multiple depot 
paths' '
 '
 
 test_expect_success 'Clone repo with multiple depot paths' '
+   test_when_finished cleanup_git &&
(
cd "$git" &&
git p4 clone --changes-block-size=4 //depot/pathA@all 
//depot/pathB@all \
@@ -138,6 +139,13 @@ test_expect_success 'Clone repo with multiple depot paths' 
'
)
 '
 
+test_expect_success 'Clone repo with self-sizing block size' '
+   test_when_finished cleanup_git &&
+   git p4 clone --changes-block-size=100 //depot@all 
--destination="$git" &&
+   git -C "$git" log --oneline >log &&
+   test_line_count \> 10 log
+'
+
 test_expect_success 'kill p4d' '
kill_p4d
 '
-- 
2.17.0.392.gdeb1a6e9b7

[PATCHv2 3/6] git-p4: better error reporting when p4 fails

[Luke Diamand]

Currently when p4 fails to run, git-p4 just crashes with an obscure
error message.

For example, if the P4 ticket has expired, you get:

  Error: Cannot locate perforce checkout of  in client view

This change checks whether git-p4 can talk to the Perforce server when
the first P4 operation is attempted, and tries to print a meaningful
error message if it fails.

Signed-off-by: Luke Diamand 
---
 git-p4.py | 55 +
 t/t9833-errors.sh | 78 +++
 2 files changed, 133 insertions(+)
 create mode 100755 t/t9833-errors.sh

diff --git a/git-p4.py b/git-p4.py
index b61f47cc61..3de12a4a0a 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -50,6 +50,8 @@ def __str__(self):
 # Grab changes in blocks of this many revisions, unless otherwise requested
 defaultBlockSize = 512
 
+p4_access_checked = False
+
 def p4_build_cmd(cmd):
 """Build a suitable p4 command line.
 
@@ -91,6 +93,13 @@ def p4_build_cmd(cmd):
 real_cmd = ' '.join(real_cmd) + ' ' + cmd
 else:
 real_cmd += cmd
+
+# now check that we can actually talk to the server
+global p4_access_checked
+if not p4_access_checked:
+p4_access_checked = True# suppress access checks in 
p4_check_access itself
+p4_check_access()
+
 return real_cmd
 
 def git_dir(path):
@@ -264,6 +273,52 @@ def p4_system(cmd):
 if retcode:
 raise CalledProcessError(retcode, real_cmd)
 
+def die_bad_access(s):
+die("failure accessing depot: {0}".format(s.rstrip()))
+
+def p4_check_access(min_expiration=1):
+""" Check if we can access Perforce - account still logged in
+"""
+results = p4CmdList(["login", "-s"])
+
+if len(results) == 0:
+# should never get here: always get either some results, or a 
p4ExitCode
+assert("could not parse response from perforce")
+
+result = results[0]
+
+if 'p4ExitCode' in result:
+# p4 returned non-zero status, e.g. P4PORT invalid, or p4 not in path
+die_bad_access("could not run p4")
+
+code = result.get("code")
+if not code:
+# we get here if we couldn't connect and there was nothing to unmarshal
+die_bad_access("could not connect")
+
+elif code == "stat":
+expiry = result.get("TicketExpiration")
+if expiry:
+expiry = int(expiry)
+if expiry > min_expiration:
+# ok to carry on
+return
+else:
+die_bad_access("perforce ticket expires in {0} 
seconds".format(expiry))
+
+else:
+# account without a timeout - all ok
+return
+
+elif code == "error":
+data = result.get("data")
+if data:
+die_bad_access("p4 error: {0}".format(data))
+else:
+die_bad_access("unknown error")
+else:
+die_bad_access("unknown error code {0}".format(code))
+
 _p4_version_string = None
 def p4_version_string():
 """Read the version string, showing just the last line, which
diff --git a/t/t9833-errors.sh b/t/t9833-errors.sh
new file mode 100755
index 00..9ba892de7a
--- /dev/null
+++ b/t/t9833-errors.sh
@@ -0,0 +1,78 @@
+#!/bin/sh
+
+test_description='git p4 errors'
+
+. ./lib-git-p4.sh
+
+test_expect_success 'start p4d' '
+   start_p4d
+'
+
+test_expect_success 'add p4 files' '
+   (
+   cd "$cli" &&
+   echo file1 >file1 &&
+   p4 add file1 &&
+   p4 submit -d "file1"
+   )
+'
+
+# after this test, the default user requires a password
+test_expect_success 'error handling' '
+   git p4 clone --dest="$git" //depot@all &&
+   (
+   cd "$git" &&
+   P4PORT=: test_must_fail git p4 submit 2>errmsg
+   ) &&
+   p4 passwd -P newpassword &&
+   (
+   P4PASSWD=badpassword test_must_fail git p4 clone //depot/foo 
2>errmsg &&
+   grep -q "failure accessing depot.*P4PASSWD" errmsg
+   )
+'
+
+test_expect_success 'ticket logged out' '
+   P4TICKETS="$cli/tickets" &&
+   echo "newpassword" | p4 login &&
+   (
+   cd "$git" &&
+   test_commit "ticket-auth-check" &&
+   p4 logout &&
+   test_must_fail git p4 submit 2>errmsg &&
+   grep -q "failure accessing depot" errmsg
+   )
+'
+
+test_expect_success 'create group with short ticket expiry' '
+   P4TICKETS="$cli/tickets" &&
+   echo "newpassword" | p4 logi

[PATCHv2 0/6] git-p4: some small fixes updated

[Luke Diamand]

This is an updated version of the set of changes I posted recently,
following comments on the list:

disable automatic sync after git-p4 submit:
https://marc.info/?l=git=152818734814838=2

better handling of being logged out by Perforce:
   https://marc.info/?l=git=152818893815326=2

adapt the block size automatically on git-p4 submit:
   https://marc.info/?l=git=152819004315688=2

- Spelling corrections (Eric)
- Improved comments (Eric)
- Exception class hierarchy fix (Merland)
- test simplification (Eric)


Luke Diamand (6):
  git-p4: disable-rebase: allow setting this via configuration
  git-p4: add option to disable syncing of p4/master with p4
  git-p4: better error reporting when p4 fails
  git-p4: raise exceptions from p4CmdList based on error from p4 server
  git-p4: narrow the scope of exceptions caught when parsing an int
  git-p4: auto-size the block

 Documentation/git-p4.txt |  13 +++-
 git-p4.py| 161 +--
 t/t9818-git-p4-block.sh  |   8 ++
 t/t9833-errors.sh|  78 +++
 4 files changed, 236 insertions(+), 24 deletions(-)
 create mode 100755 t/t9833-errors.sh

-- 
2.17.0.392.gdeb1a6e9b7

[PATCHv2 4/6] git-p4: raise exceptions from p4CmdList based on error from p4 server

[Luke Diamand]

This change lays some groundwork for better handling of rowcount errors
from the server, where it fails to send us results because we requested
too many.

It adds an option to p4CmdList() to return errors as a Python exception.

The exceptions are derived from P4Exception (something went wrong),
P4ServerException (the server sent us an error code) and
P4RequestSizeException (we requested too many rows/results from the
server database).

This makes the code that handles the errors a bit easier.

The default behavior is unchanged; the new code is enabled with a flag.

Signed-off-by: Luke Diamand 
---
 git-p4.py | 44 
 1 file changed, 40 insertions(+), 4 deletions(-)

diff --git a/git-p4.py b/git-p4.py
index 3de12a4a0a..f4b5deeb83 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -566,10 +566,30 @@ def isModeExec(mode):
 # otherwise False.
 return mode[-3:] == "755"
 
+class P4Exception(Exception):
+""" Base class for exceptions from the p4 client """
+def __init__(self, exit_code):
+self.p4ExitCode = exit_code
+
+class P4ServerException(P4Exception):
+""" Base class for exceptions where we get some kind of marshalled up 
result from the server """
+def __init__(self, exit_code, p4_result):
+super(P4ServerException, self).__init__(exit_code)
+self.p4_result = p4_result
+self.code = p4_result[0]['code']
+self.data = p4_result[0]['data']
+
+class P4RequestSizeException(P4ServerException):
+""" One of the maxresults or maxscanrows errors """
+def __init__(self, exit_code, p4_result, limit):
+super(P4RequestSizeException, self).__init__(exit_code, p4_result)
+self.limit = limit
+
 def isModeExecChanged(src_mode, dst_mode):
 return isModeExec(src_mode) != isModeExec(dst_mode)
 
-def p4CmdList(cmd, stdin=None, stdin_mode='w+b', cb=None, skip_info=False):
+def p4CmdList(cmd, stdin=None, stdin_mode='w+b', cb=None, skip_info=False,
+errors_as_exceptions=False):
 
 if isinstance(cmd,basestring):
 cmd = "-G " + cmd
@@ -616,9 +636,25 @@ def p4CmdList(cmd, stdin=None, stdin_mode='w+b', cb=None, 
skip_info=False):
 pass
 exitCode = p4.wait()
 if exitCode != 0:
-entry = {}
-entry["p4ExitCode"] = exitCode
-result.append(entry)
+if errors_as_exceptions:
+if len(result) > 0:
+data = result[0].get('data')
+if data:
+m = re.search('Too many rows scanned \(over (\d+)\)', data)
+if not m:
+m = re.search('Request too large \(over (\d+)\)', data)
+
+if m:
+limit = int(m.group(1))
+raise P4RequestSizeException(exitCode, result, limit)
+
+raise P4ServerException(exitCode, result)
+else:
+raise P4Exception(exitCode)
+else:
+entry = {}
+entry["p4ExitCode"] = exitCode
+result.append(entry)
 
 return result
 
-- 
2.17.0.392.gdeb1a6e9b7

Re: Re :Re: [PATCHv3 0/1] git-p4 unshelve

[Luke Diamand]

On 16 June 2018 at 10:58, merlo...@yahoo.fr  wrote:
> Yes Luke, my colleagues and I, we care ! Our repository is p4 (choice of the
> high management, sigh...). Some of us are working natively on p4, but others
> like me are working on git through git-p4. We often want to share pieces of
> codes to check compilation on misc platforms/configs, and shelve/unshelve
> mechanism is commonly used between nativ p4 users.
> For git-p4 users we have a temporary hack to unshelve, but it often fails to
> apply the whole p4 diff, and we end up finishing stuff by hand, checking
> line by line, sigh... Without speaking about diffs that are accross several
> local workspaces.
> Hopefully it is on small shelved p4 change lists for the moment, but we
> cannot deploy on a larger scale.
> Please continue your hard work on unshelve stuff.
>
> For your last remark, the members of our team often need to work on non
> synchro revisions, but still need to share via shelve/unshelve, and I am
> sure we will have conflits as you describe, leading unshelve to fail.
> Automatic fast import would save us the need to stop our current work, sync
> with p4 and launch a 1h compilation, before we could unshelve... So yes we
> need it !

OK, I'll give it a go, in my copious free time :-)

Luke

Re: [PATCHv2 0/6] git-p4: some small fixes updated

[Luke Diamand]

On 12 June 2018 at 22:35, Junio C Hamano  wrote:
> Luke Diamand  writes:
>
>> On 12 June 2018 at 18:10, Junio C Hamano  wrote:
>>> Luke Diamand  writes:
>>>
>>>> This is an updated version of the set of changes I posted recently,
>>>> following comments on the list:
>>>>
>>>> disable automatic sync after git-p4 submit:
>>>> https://marc.info/?l=git=152818734814838=2
>>>>
>>>> better handling of being logged out by Perforce:
>>>>https://marc.info/?l=git=152818893815326=2
>>>>
>>>> adapt the block size automatically on git-p4 submit:
>>>>https://marc.info/?l=git=152819004315688=2
>>>>
>>>> - Spelling corrections (Eric)
>>>> - Improved comments (Eric)
>>>> - Exception class hierarchy fix (Merland)
>>>> - test simplification (Eric)
>>>>
>>>
>>> That reminds me of one thing.
>>>
>>> This 6-patch series depends on the rm/p4-submit-with-commit-option
>>> that came without and still waiting for a sign-off by the original
>>> author.  Also I do not think the original patch reached the public
>>> list, so I'm attaching the patch to make sure people know which
>>> patch I am talking about.
>>>
>>> Romain, can we get your sign-off on the patch you sent earlier?
>>
>> Wasn't it already sent in this message:
>>
>> https://marc.info/?l=git=152783923418317=2
>>
>> Luke
>
>
> Thanks for a pointer.  Will replace and requeue.

Thanks. While on the subject of git-p4, I thought I should mention
that I've been looking at getting git-p4 to work with Python3.

I've got some low risk easy (mostly automated) changes which get it to
the point where it compiles. After that I have to figure out how to
fix the byte-vs-string unicode problem that Python3 brings. Having
been playing around with it, I think it should be possible to make the
same git-p4.py script work with 2.7, 3.6 and probably 2.6, although
I'm still some way from making this last bit work.

Luke

Re: [PATCHv2 0/6] git-p4: some small fixes updated

[Luke Diamand]

On 12 June 2018 at 18:10, Junio C Hamano  wrote:
> Luke Diamand  writes:
>
>> This is an updated version of the set of changes I posted recently,
>> following comments on the list:
>>
>> disable automatic sync after git-p4 submit:
>> https://marc.info/?l=git=152818734814838=2
>>
>> better handling of being logged out by Perforce:
>>https://marc.info/?l=git=152818893815326=2
>>
>> adapt the block size automatically on git-p4 submit:
>>https://marc.info/?l=git=152819004315688=2
>>
>> - Spelling corrections (Eric)
>> - Improved comments (Eric)
>> - Exception class hierarchy fix (Merland)
>> - test simplification (Eric)
>>
>
> That reminds me of one thing.
>
> This 6-patch series depends on the rm/p4-submit-with-commit-option
> that came without and still waiting for a sign-off by the original
> author.  Also I do not think the original patch reached the public
> list, so I'm attaching the patch to make sure people know which
> patch I am talking about.
>
> Romain, can we get your sign-off on the patch you sent earlier?

Wasn't it already sent in this message:

https://marc.info/?l=git=152783923418317=2

Luke


>
> Thanks.
>
> -- >8 --
> From: Romain Merland 
> Date: Wed, 9 May 2018 17:32:12 +0200
> Subject: [PATCH] git-p4: add options --commit and --disable-rebase
>
> On a daily work with multiple local git branches, the usual way to
> submit only a specified commit was to cherry-pick the commit on
> master then run git-p4 submit.  It can be very annoying to switch
> between local branches and master, only to submit one commit.  The
> proposed new way is to select directly the commit you want to
> submit.
>
> Add option --commit to command 'git-p4 submit' in order to submit
> only specified commit(s) in p4.
>
> On a daily work developping software with big compilation time, one
> may not want to rebase on his local git tree, in order to avoid long
> recompilation.
>
> Add option --disable-rebase to command 'git-p4 submit' in order to
> disable rebase after submission.
>
> Reviewed-by: Luke Diamand 
> Signed-off-by: Junio C Hamano 
> ---
>  Documentation/git-p4.txt | 14 ++
>  git-p4.py| 29 +++--
>  t/t9807-git-p4-submit.sh | 40 
>  3 files changed, 77 insertions(+), 6 deletions(-)
>
> diff --git a/Documentation/git-p4.txt b/Documentation/git-p4.txt
> index d8c8f11c9f..88d109debb 100644
> --- a/Documentation/git-p4.txt
> +++ b/Documentation/git-p4.txt
> @@ -149,6 +149,12 @@ To specify a branch other than the current one, use:
>  $ git p4 submit topicbranch
>  
>
> +To specify a single commit or a range of commits, use:
> +
> +$ git p4 submit --commit 
> +$ git p4 submit --commit 
> +
> +
>  The upstream reference is generally 'refs/remotes/p4/master', but can
>  be overridden using the `--origin=` command-line option.
>
> @@ -330,6 +336,14 @@ These options can be used to modify 'git p4 submit' 
> behavior.
> p4/master.  See the "Sync options" section above for more
> information.
>
> +--commit |::
> +Submit only the specified commit or range of commits, instead of the full
> +list of changes that are in the current Git branch.
> +
> +--disable-rebase::
> +Disable the automatic rebase after all commits have been successfully
> +submitted.
> +
>  Rebase options
>  ~~
>  These options can be used to modify 'git p4 rebase' behavior.
> diff --git a/git-p4.py b/git-p4.py
> index 7bb9cadc69..f4a6f3b4c3 100755
> --- a/git-p4.py
> +++ b/git-p4.py
> @@ -1352,7 +1352,12 @@ def __init__(self):
>  optparse.make_option("--update-shelve", 
> dest="update_shelve", action="append", type="int",
>   metavar="CHANGELIST",
>   help="update an existing shelved 
> changelist, implies --shelve, "
> -   "repeat in-order for multiple 
> shelved changelists")
> +   "repeat in-order for multiple 
> shelved changelists"),
> +optparse.make_option("--commit", dest="commit", 
> metavar="COMMIT",
> + help="submit only the specified 
> commit(s), one commit or xxx..xxx"),
> +optparse.make_option("--disable-rebase", 
> dest="disable_rebase", action="store_

Re: [PATCHv2 0/6] git-p4: some small fixes updated

[Luke Diamand]

On 12 June 2018 at 22:53, Eric Sunshine  wrote:
> On Tue, Jun 12, 2018 at 5:49 PM, Luke Diamand  wrote:
>> Thanks. While on the subject of git-p4, I thought I should mention
>> that I've been looking at getting git-p4 to work with Python3.
>>
>> I've got some low risk easy (mostly automated) changes which get it to
>> the point where it compiles. After that I have to figure out how to
>> fix the byte-vs-string unicode problem that Python3 brings. [...]
>
> See how reposurgeon handles the problem[1]. It defines polystr() and
> polybytes() functions which coerce strings and byte sequences as
> needed. It's not pretty, but it works.
>
> [1]: https://gitlab.com/esr/reposurgeon/blob/master/reposurgeon#L100

Thanks, that's got some useful tips!

Re: [PATCHv3 0/1] git-p4 unshelve

[Luke Diamand]

On 15 May 2018 at 11:01, Luke Diamand  wrote:
> This is a git-p4 unshelve command which detects when extra
> changes are going to be included, and refuses to unhshelve.
>
> As in my earlier unshelve change, this uses git fast-import
> to do the actual delta generation, but now checks to see
> if the files unshelved are based on the same revisions that
> fast-import will be using, using the revision numbers in
> the "p4 describe -S" command output. If they're different,
> it refuses to unshelve.
>
> That makes it safe to use, rather than potentially generating
> deltas that contain bits from other changes.

Having been using this, when it works it's great, and it's nice that
it errors out rather than creating a garbled patch. But it would be
really useful if it could somehow automatically build some kind of
diff-base for fast-import to use.

Doing so is probably not that hard given that it already knows the
versions that it wants for the error reports, but it's not completely
trivial either. I think you'd need to get the list of base revisions
you want, then hijack the P4Sync class to generate a commit via
fast-import, then feed that to the next stage as the base commit. I
think it would make sense to see if anyone other than me actually
cares!

Luke

[PATCH 2/6] git-p4: python3: replace dict.has_key(k) with "k in dict"

[Luke Diamand]

Python3 does not have the dict.has_key() function, so replace all
such calls with "k in dict". This will still work with python2.6
and python2.7.

Converted using 2to3 (plus some hand-editing)

Signed-off-by: Luke Diamand 
---
 git-p4.py | 78 +++
 1 file changed, 39 insertions(+), 39 deletions(-)

diff --git a/git-p4.py b/git-p4.py
index 51e9e64a73..6fcad35104 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -767,7 +767,7 @@ def gitDeleteRef(ref):
 _gitConfig = {}
 
 def gitConfig(key, typeSpecifier=None):
-if not _gitConfig.has_key(key):
+if key not in _gitConfig:
 cmd = [ "git", "config" ]
 if typeSpecifier:
 cmd += [ typeSpecifier ]
@@ -781,12 +781,12 @@ def gitConfigBool(key):
variable is set to true, and False if set to false or not present
in the config."""
 
-if not _gitConfig.has_key(key):
+if key not in _gitConfig:
 _gitConfig[key] = gitConfig(key, '--bool') == "true"
 return _gitConfig[key]
 
 def gitConfigInt(key):
-if not _gitConfig.has_key(key):
+if key not in _gitConfig:
 cmd = [ "git", "config", "--int", key ]
 s = read_pipe(cmd, ignore_error=True)
 v = s.strip()
@@ -797,7 +797,7 @@ def gitConfigInt(key):
 return _gitConfig[key]
 
 def gitConfigList(key):
-if not _gitConfig.has_key(key):
+if key not in _gitConfig:
 s = read_pipe(["git", "config", "--get-all", key], ignore_error=True)
 _gitConfig[key] = s.strip().splitlines()
 if _gitConfig[key] == ['']:
@@ -855,7 +855,7 @@ def findUpstreamBranchPoint(head = "HEAD"):
 tip = branches[branch]
 log = extractLogMessageFromGitCommit(tip)
 settings = extractSettingsGitLog(log)
-if settings.has_key("depot-paths"):
+if "depot-paths" in settings:
 paths = ",".join(settings["depot-paths"])
 branchByDepotPath[paths] = "remotes/p4/" + branch
 
@@ -865,9 +865,9 @@ def findUpstreamBranchPoint(head = "HEAD"):
 commit = head + "~%s" % parent
 log = extractLogMessageFromGitCommit(commit)
 settings = extractSettingsGitLog(log)
-if settings.has_key("depot-paths"):
+if "depot-paths" in settings:
 paths = ",".join(settings["depot-paths"])
-if branchByDepotPath.has_key(paths):
+if paths in branchByDepotPath:
 return [branchByDepotPath[paths], settings]
 
 parent = parent + 1
@@ -891,8 +891,8 @@ def createOrUpdateBranchesFromOrigin(localRefPrefix = 
"refs/remotes/p4/", silent
 originHead = line
 
 original = 
extractSettingsGitLog(extractLogMessageFromGitCommit(originHead))
-if (not original.has_key('depot-paths')
-or not original.has_key('change')):
+if ('depot-paths' not in original
+or 'change' not in original):
 continue
 
 update = False
@@ -902,7 +902,7 @@ def createOrUpdateBranchesFromOrigin(localRefPrefix = 
"refs/remotes/p4/", silent
 update = True
 else:
 settings = 
extractSettingsGitLog(extractLogMessageFromGitCommit(remoteHead))
-if settings.has_key('change') > 0:
+if 'change' in settings:
 if settings['depot-paths'] == original['depot-paths']:
 originP4Change = int(original['change'])
 p4Change = int(settings['change'])
@@ -1002,7 +1002,7 @@ def p4ChangesForPaths(depotPaths, changeRange, 
requestedBlockSize):
 
 # Insert changes in chronological order
 for entry in reversed(result):
-if not entry.has_key('change'):
+if 'change' not in entry:
 continue
 changes.add(int(entry['change']))
 
@@ -1312,7 +1312,7 @@ def p4UserId(self):
 
 results = p4CmdList("user -o")
 for r in results:
-if r.has_key('User'):
+if 'User' in r:
 self.myP4UserId = r['User']
 return r['User']
 die("Could not find your p4 user id")
@@ -1336,7 +1336,7 @@ def getUserMapFromPerforceServer(self):
 self.emails = {}
 
 for output in p4CmdList("users"):
-if not output.has_key("User"):
+if "User" not in output:
 continue
 self.users[output["User"]] = output["FullName"] + " <" + 
output["Email"] + ">"
 self.emails[output["Email"]] = output["User"]
@@ -1588,7 +1588,7 @@ def p4UserForCommit(self,id):
 gitEmail = read_pipe(["git", &q

[PATCH 4/6] git-p4: python3: basestring workaround

[Luke Diamand]

In Python3, basestring no longer exists, so use this workaround.

Signed-off-by: Luke Diamand 
---
 git-p4.py | 16 
 1 file changed, 16 insertions(+)

diff --git a/git-p4.py b/git-p4.py
index 67865d14aa..f127ebce27 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -27,6 +27,22 @@
 import ctypes
 import errno
 
+# support basestring in python3
+try:
+unicode = unicode
+except NameError:
+# 'unicode' is undefined, must be Python 3
+str = str
+unicode = str
+bytes = bytes
+basestring = (str,bytes)
+else:
+# 'unicode' exists, must be Python 2
+str = str
+unicode = unicode
+bytes = str
+basestring = basestring
+
 try:
 from subprocess import CalledProcessError
 except ImportError:
-- 
2.18.0.rc1.242.g61856ae69a

[PATCH 1/6] git-p4: python3: replace <> with !=

[Luke Diamand]

The <> string inequality operator (which doesn't seem to be even
documented) no longer exists in python3. Replace with !=.

This still works with python2.

Signed-off-by: Luke Diamand 
---
 git-p4.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/git-p4.py b/git-p4.py
index 0354d4df5c..51e9e64a73 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -3590,7 +3590,7 @@ def run(self, args):
 prev_list = prev.split("/")
 cur_list = cur.split("/")
 for i in range(0, min(len(cur_list), 
len(prev_list))):
-if cur_list[i] <> prev_list[i]:
+if cur_list[i] != prev_list[i]:
 i = i - 1
 break
 
-- 
2.18.0.rc1.242.g61856ae69a

[PATCH 0/6] git-p4: small step towards Python3 support

[Luke Diamand]

This patchset is a first small step towards Python3 support for
git-p4.py.

These are all the nice easy changes which can almost be done
automatically using 2to3.

After these changes, it compiles using Python3, but fails to run.
That's because of the bytes vs string change in Python3. Fixing that is
quite a bit harder (but not impossible).

I have some further changes to address this, but they are quite a bit
more invasive, and not actually working yet. It's based very loosely on the
"polystr()" suggestion from Eric on this list.

It still works fine with Python2.7 and Python2.6.

Luke Diamand (6):
  git-p4: python3: replace <> with !=
  git-p4: python3: replace dict.has_key(k) with "k in dict"
  git-p4: python3: remove backticks
  git-p4: python3: basestring workaround
  git-p4: python3: use print() function
  git-p4: python3: fix octal constants

 git-p4.py | 348 --
 1 file changed, 182 insertions(+), 166 deletions(-)

-- 
2.18.0.rc1.242.g61856ae69a

[PATCH 6/6] git-p4: python3: fix octal constants

[Luke Diamand]

See PEP3127. Works fine with python2 as well.

Signed-off-by: Luke Diamand 
---
 git-p4.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/git-p4.py b/git-p4.py
index 714e442d7c..b449db1cc9 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -1841,7 +1841,7 @@ def applyCommit(self, id):
 filesToDelete.remove(path)
 
 dst_mode = int(diff['dst_mode'], 8)
-if dst_mode == 012:
+if dst_mode == 0o12:
 symlinks.add(path)
 
 elif modifier == "D":
-- 
2.18.0.rc1.242.g61856ae69a

[PATCHv1 0/2] git-p4: disable sync after submit

[Luke Diamand]

This is a small patch to git-p4 to disable the automatic sync after
submit.

In my day-to-day work, I have a central git-p4 repo which is
automatically kept up-to-date, so the repos where I actually work and
submit from don't even need the sync. I usually end up hitting Ctrl-C
partway through the sync.

I imagine other people with large git-p4 projects do something similar
and have the same problem.

This also updates Merland's recent submit-selection change so that both
options can be set via configuration rather than being set on the
command line.

Luke Diamand (2):
  git-p4: disable-rebase: allow setting this via configuration
  git-p4: add option to disable syncing of p4/master with p4

 Documentation/git-p4.txt | 13 -
 git-p4.py| 33 +
 2 files changed, 33 insertions(+), 13 deletions(-)

-- 
2.17.0.392.gdeb1a6e9b7

[PATCHv1 2/2] git-p4: add option to disable syncing of p4/master with p4

[Luke Diamand]

Add an option to the git-p4 submit command to disable syncing
with Perforce.

This is useful for the case where a git-p4 mirror has been setup
on a server somewhere, running from (e.g.) cron, and developers
then clone from this. Having the local cloned copy also sync
from Perforce just isn't useful.

Signed-off-by: Luke Diamand 
---
 Documentation/git-p4.txt |  8 
 git-p4.py| 31 ---
 2 files changed, 28 insertions(+), 11 deletions(-)

diff --git a/Documentation/git-p4.txt b/Documentation/git-p4.txt
index 3d83842e47..8f6a7543fd 100644
--- a/Documentation/git-p4.txt
+++ b/Documentation/git-p4.txt
@@ -369,6 +369,11 @@ These options can be used to modify 'git p4 submit' 
behavior.
 Disable the automatic rebase after all commits have been successfully
 submitted. Can also be set with git-p4.disableRebase.
 
+--disable-p4sync::
+Disable the automatic sync of p4/master from Perforce after commit have
+been submitted. Implies --disable-rebase. Can also be set with
+git-p4.disableP4Sync. Sync with origin/master still goes ahead if possible.
+
 Rebase options
 ~~
 These options can be used to modify 'git p4 rebase' behavior.
@@ -693,6 +698,9 @@ git-p4.conflict::
 git-p4.disableRebase::
 Do not rebase the tree against p4/master following a submit.
 
+git-p4.disableP4Sync::
+Do not sync p4/master with Perforce following a submit. Implies 
git-p4.disableRebase.
+
 IMPLEMENTATION DETAILS
 --
 * Changesets from p4 are imported using Git fast-import.
diff --git a/git-p4.py b/git-p4.py
index 5ab9421af8..b9e79f1d8b 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -1368,7 +1368,9 @@ def __init__(self):
  help="submit only the specified 
commit(s), one commit or xxx..xxx"),
 optparse.make_option("--disable-rebase", 
dest="disable_rebase", action="store_true",
  help="Disable rebase after submit is 
completed. Can be useful if you "
- "work from a local git branch that is not 
master")
+ "work from a local git branch that is not 
master"),
+optparse.make_option("--disable-p4sync", 
dest="disable_p4sync", action="store_true",
+ help="Skip perforce sync of p4/master 
after submit or shelve"),
 ]
 self.description = "Submit changes from git to the perforce depot."
 self.usage += " [name of git branch to submit into perforce depot]"
@@ -1380,6 +1382,7 @@ def __init__(self):
 self.update_shelve = list()
 self.commit = ""
 self.disable_rebase = gitConfigBool("git-p4.disableRebase")
+self.disable_p4sync = gitConfigBool("git-p4.disableP4Sync")
 self.prepare_p4_only = False
 self.conflict_behavior = None
 self.isWindows = (platform.system() == "Windows")
@@ -2240,11 +2243,14 @@ def run(self, args):
 sync = P4Sync()
 if self.branch:
 sync.branch = self.branch
-sync.run([])
+if self.disable_p4sync:
+sync.sync_origin_only()
+else:
+sync.run([])
 
-if self.disable_rebase is False:
-rebase = P4Rebase()
-rebase.rebase()
+if not self.disable_rebase:
+rebase = P4Rebase()
+rebase.rebase()
 
 else:
 if len(applied) == 0:
@@ -3324,6 +3330,14 @@ def importChanges(self, changes, shelved=False, 
origin_revision=0):
 print self.gitError.read()
 sys.exit(1)
 
+def sync_origin_only(self):
+if self.syncWithOrigin:
+self.hasOrigin = originP4BranchesExist()
+if self.hasOrigin:
+if not self.silent:
+print 'Syncing with origin first, using "git fetch origin"'
+system("git fetch origin")
+
 def importHeadRevision(self, revision):
 print "Doing initial import of %s from revision %s into %s" % (' 
'.join(self.depotPaths), revision, self.branch)
 
@@ -3402,12 +3416,7 @@ def run(self, args):
 else:
 self.refPrefix = "refs/heads/p4/"
 
-if self.syncWithOrigin:
-self.hasOrigin = originP4BranchesExist()
-if self.hasOrigin:
-if not self.silent:
-print 'Syncing with origin first, using "git fetch origin"'
-system("git fetch origin")
+self.sync_origin_only()
 
 branch_arg_given = bool(self.branch)
 if len(self.branch) == 0:
-- 
2.17.0.392.gdeb1a6e9b7

[PATCHv1 1/2] git-p4: disable-rebase: allow setting this via configuration

[Luke Diamand]

This just lets you set the --disable-rebase option with the
git configuration options git-p4.disableRebase. If you're
using this option, you probably want to set it all the time
for a given repo.

Signed-off-by: Luke Diamand 
---
 Documentation/git-p4.txt | 5 -
 git-p4.py| 2 +-
 2 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/Documentation/git-p4.txt b/Documentation/git-p4.txt
index e8452528fc..3d83842e47 100644
--- a/Documentation/git-p4.txt
+++ b/Documentation/git-p4.txt
@@ -367,7 +367,7 @@ These options can be used to modify 'git p4 submit' 
behavior.
 
 --disable-rebase::
 Disable the automatic rebase after all commits have been successfully
-submitted.
+submitted. Can also be set with git-p4.disableRebase.
 
 Rebase options
 ~~
@@ -690,6 +690,9 @@ git-p4.conflict::
Specify submit behavior when a conflict with p4 is found, as per
--conflict.  The default behavior is 'ask'.
 
+git-p4.disableRebase::
+Do not rebase the tree against p4/master following a submit.
+
 IMPLEMENTATION DETAILS
 --
 * Changesets from p4 are imported using Git fast-import.
diff --git a/git-p4.py b/git-p4.py
index c4581d20a6..5ab9421af8 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -1379,7 +1379,7 @@ def __init__(self):
 self.shelve = False
 self.update_shelve = list()
 self.commit = ""
-self.disable_rebase = False
+self.disable_rebase = gitConfigBool("git-p4.disableRebase")
 self.prepare_p4_only = False
 self.conflict_behavior = None
 self.isWindows = (platform.system() == "Windows")
-- 
2.17.0.392.gdeb1a6e9b7

[PATCHv1 1/1] git-p4: better error reporting when p4 fails

[Luke Diamand]

Currently when p4 fails to run, git-p4 just crashes with an obscure
error message.

For example, if the P4 ticket has expired, you get:

  Error: Cannot locate perforce checkout of  in client view

This change checks whether git-p4 can talk to the Perforce server when
the first P4 operation is attempted, and tries to print a meaningful
error message if it fails.

Signed-off-by: Luke Diamand 
---
 git-p4.py | 55 +
 t/t9833-errors.sh | 78 +++
 2 files changed, 133 insertions(+)
 create mode 100755 t/t9833-errors.sh

diff --git a/git-p4.py b/git-p4.py
index b9e79f1d8b..66652f8280 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -50,6 +50,8 @@ def __str__(self):
 # Grab changes in blocks of this many revisions, unless otherwise requested
 defaultBlockSize = 512
 
+p4_access_checked = False
+
 def p4_build_cmd(cmd):
 """Build a suitable p4 command line.
 
@@ -91,6 +93,13 @@ def p4_build_cmd(cmd):
 real_cmd = ' '.join(real_cmd) + ' ' + cmd
 else:
 real_cmd += cmd
+
+# now check that we can actually talk to the server
+global p4_access_checked
+if not p4_access_checked:
+p4_access_checked = True
+p4_check_access()
+
 return real_cmd
 
 def git_dir(path):
@@ -264,6 +273,52 @@ def p4_system(cmd):
 if retcode:
 raise CalledProcessError(retcode, real_cmd)
 
+def die_bad_access(s):
+die("failure accessing depot: {0}".format(s.rstrip()))
+
+def p4_check_access(min_expiration=1):
+""" Check if we can access Perforce - account still logged in
+"""
+results = p4CmdList(["login", "-s"])
+
+if len(results) == 0:
+# should never get here: always get either some results, or a 
p4ExitCode
+assert("could not parse response from perforce")
+
+result = results[0]
+
+if 'p4ExitCode' in result:
+# p4 returned non-zero status, e.g. P4PORT invalid, or p4 not in path
+die_bad_access("could not run p4")
+
+code = result.get("code")
+if not code:
+# we get here if we couldn't connect and there was nothing to unmarshal
+die_bad_access("could not connect")
+
+elif code == "stat":
+expiry = result.get("TicketExpiration")
+if expiry:
+expiry = int(expiry)
+if expiry > min_expiration:
+# ok to carry on
+return
+else:
+die_bad_access("perforce ticket expires in {0} 
seconds".format(expiry))
+
+else:
+# account without a timeout - all ok
+return
+
+elif code == "error":
+data = result.get("data")
+if data:
+die_bad_access("p4 error: {0}".format(data))
+else:
+die_bad_access("unknown error")
+else:
+die_bad_access("unknown error code {0}".format(code))
+
 _p4_version_string = None
 def p4_version_string():
 """Read the version string, showing just the last line, which
diff --git a/t/t9833-errors.sh b/t/t9833-errors.sh
new file mode 100755
index 00..9ba892de7a
--- /dev/null
+++ b/t/t9833-errors.sh
@@ -0,0 +1,78 @@
+#!/bin/sh
+
+test_description='git p4 errors'
+
+. ./lib-git-p4.sh
+
+test_expect_success 'start p4d' '
+   start_p4d
+'
+
+test_expect_success 'add p4 files' '
+   (
+   cd "$cli" &&
+   echo file1 >file1 &&
+   p4 add file1 &&
+   p4 submit -d "file1"
+   )
+'
+
+# after this test, the default user requires a password
+test_expect_success 'error handling' '
+   git p4 clone --dest="$git" //depot@all &&
+   (
+   cd "$git" &&
+   P4PORT=: test_must_fail git p4 submit 2>errmsg
+   ) &&
+   p4 passwd -P newpassword &&
+   (
+   P4PASSWD=badpassword test_must_fail git p4 clone //depot/foo 
2>errmsg &&
+   grep -q "failure accessing depot.*P4PASSWD" errmsg
+   )
+'
+
+test_expect_success 'ticket logged out' '
+   P4TICKETS="$cli/tickets" &&
+   echo "newpassword" | p4 login &&
+   (
+   cd "$git" &&
+   test_commit "ticket-auth-check" &&
+   p4 logout &&
+   test_must_fail git p4 submit 2>errmsg &&
+   grep -q "failure accessing depot" errmsg
+   )
+'
+
+test_expect_success 'create group with short ticket expiry' '
+   P4TICKETS="$cli/tickets" &&
+   echo "newpassword" | p4 login &&
+   p4_add_user short

[PATCHv1 0/1] git-p4: better error reporting

[Luke Diamand]

If git-p4 cannot talk to the Perforce server, it will either give a
confusing error message, or just crash. Even I get tripped up by this.

This change just checks that the client can talk to the server, and in
particular that the user is logged in (the default login timeout is 12
hours).

It would be nice to also check for ticket expiration during long
Perforce operations, but this change does not attempt to do that.

Luke Diamand (1):
  git-p4: better error reporting when p4 fails

 git-p4.py | 55 +
 t/t9833-errors.sh | 78 +++
 2 files changed, 133 insertions(+)
 create mode 100755 t/t9833-errors.sh

-- 
2.17.0.392.gdeb1a6e9b7

Re: [PATCHv1 1/3] git-p4: raise exceptions from p4CmdList based on error from p4 server

[Luke Diamand]

On 5 June 2018 at 10:54, Eric Sunshine  wrote:
> On Tue, Jun 5, 2018 at 5:14 AM Luke Diamand  wrote:
>> This change lays some groundwork for better handling of rowcount errors
>> from the server, where it fails to send us results because we requested
>> too many.
>>
>> It adds an option to p4CmdList() to return errors as a Python exception.
>>
>> The exceptions are derived from P4Exception (something went wrong),
>> P4ServerException (the server sent us an error code) and
>> P4RequestSizeException (we requested too many rows/results from the
>> server database).
>>
>> This makes makes the code that handles the errors a bit easier.
>>
>> The default behavior is unchanged; the new code is enabled with a flag.
>>
>> Signed-off-by: Luke Diamand 
>> ---
>> diff --git a/git-p4.py b/git-p4.py
>> @@ -566,10 +566,30 @@ def isModeExec(mode):
>> +class P4ServerException(Exception):
>> +""" Base class for exceptions where we get some kind of marshalled up 
>> result from the server """
>> +def __init__(self, exit_code, p4_result):
>> +super(P4ServerException, self).__init__(exit_code)
>> +self.p4_result = p4_result
>> +self.code = p4_result[0]['code']
>> +self.data = p4_result[0]['data']
>
> The subsequent patches never seem to access any of these fields, so
> it's difficult to judge whether it's worthwhile having 'code' and
> 'data' bits split out like this.

These changes don't use it, but I thought that future changes might
need them, and perhaps when I put that code in I was thinking I might
need it myself.

>
>> @@ -616,9 +636,25 @@ def p4CmdList(cmd, stdin=None, stdin_mode='w+b', 
>> cb=None, skip_info=False):
>>  if exitCode != 0:
>> -entry = {}
>> -entry["p4ExitCode"] = exitCode
>> -result.append(entry)
>> +if errors_as_exceptions:
>> +if len(result) > 0:
>> +data = result[0].get('data')
>> +if data:
>> +m = re.search('Too many rows scanned \(over (\d+)\)', 
>> data)
>> +if not m:
>> +m = re.search('Request too large \(over (\d+)\)', 
>> data)
>
> Does 'p4' localize these error messages?

That's a good question.

The marshalled-up error from Perforce looks like this:

 ([{'generic': 35, 'code': 'error', 'data': "Too many rows scanned
(over 40); see 'p4 help maxscanrows'.\n", 'severity': 3}])

It turns out that Perforce open-sourced the P4 client in 2014 (I only
recently found this out) so we can actually look at the code now!

https://swarm.workshop.perforce.com/projects/perforce_software-p4

Clone it like this:

mkdir p4 &&
(cd p4 && git init && git config --add git-p4.branchList p4/2018-1:2018-1) &&
P4USER=guest P4PORT=workshop.perforce.com:1666 git p4 clone
--detect-branches --destination p4  //guest/perforce_software/p4@all

Here's the code:

// ErrorId graveyard: retired/deprecated ErrorIds.

ErrorId MsgDb::MaxResults  = { ErrorOf( ES_DB, 32,
E_FAILED, EV_ADMIN, 1 ), "Request too large (over %maxResults%); see
'p4 help maxresults'." } ;//NOTRANS
ErrorId MsgDb::MaxScanRows = { ErrorOf( ES_DB, 61,
E_FAILED, EV_ADMIN, 1 ), "Too many rows scanned (over %maxScanRows%);
see 'p4 help maxscanrows'." } ;//NOTRANS


I don't think there's actually a way to make it return any language
other than English though. There's a P4LANGUAGE environment variable,
but it just says "this is for system integrators":

https://www.perforce.com/perforce/r15.2/manuals/cmdref/P4LANGUAGE.html

So I think probably the language is always English.

Luke

[PATCHv1 3/3] git-p4: auto-size the block

[Luke Diamand]

git-p4 originally would fetch changes in one query. On large repos this
could fail because of the limits that Perforce imposes on the number of
items returned and the number of queries in the database.

To fix this, git-p4 learned to query changes in blocks of 512 changes,
However, this can be very slow - if you have a few million changes,
with each chunk taking about a second, it can be an hour or so.

Although it's possible to tune this value manually with the
"--changes-block-size" option, it's far from obvious to ordinary users
that this is what needs doing.

This change alters the block size dynamically by looking for the
specific error messages returned from the Perforce server, and reducing
the block size if the error is seen, either to the limit reported by the
server, or to half the current block size.

That means we can start out with a very large block size, and then let
it automatically drop down to a value that works without error, while
still failing correctly if some other error occurs.

Signed-off-by: Luke Diamand 
---
 git-p4.py   | 26 +-
 t/t9818-git-p4-block.sh | 11 +++
 2 files changed, 32 insertions(+), 5 deletions(-)

diff --git a/git-p4.py b/git-p4.py
index b337562b39..6736f81b60 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -48,7 +48,8 @@ def __str__(self):
 defaultLabelRegexp = r'[a-zA-Z0-9_\-.]+$'
 
 # Grab changes in blocks of this many revisions, unless otherwise requested
-defaultBlockSize = 512
+# The code should now automatically reduce the block size if it is required
+defaultBlockSize = 1<<20
 
 p4_access_checked = False
 
@@ -969,7 +970,8 @@ def p4ChangesForPaths(depotPaths, changeRange, 
requestedBlockSize):
 changes = set()
 
 # Retrieve changes a block at a time, to prevent running
-# into a MaxResults/MaxScanRows error from the server.
+# into a MaxResults/MaxScanRows error from the server. If
+# we _do_ hit one of those errors, turn down the block size
 
 while True:
 cmd = ['changes']
@@ -983,10 +985,24 @@ def p4ChangesForPaths(depotPaths, changeRange, 
requestedBlockSize):
 for p in depotPaths:
 cmd += ["%s...@%s" % (p, revisionRange)]
 
+# fetch the changes
+try:
+result = p4CmdList(cmd, errors_as_exceptions=True)
+except P4RequestSizeException as e:
+if not block_size:
+block_size = e.limit
+elif block_size > e.limit:
+block_size = e.limit
+else:
+block_size = max(2, block_size // 2)
+
+if verbose: print("block size error, retry with block size 
{0}".format(block_size))
+continue
+except P4Exception as e:
+die('Error retrieving changes description 
({0})'.format(e.p4ExitCode))
+
 # Insert changes in chronological order
-for entry in reversed(p4CmdList(cmd)):
-if entry.has_key('p4ExitCode'):
-die('Error retrieving changes descriptions 
({})'.format(entry['p4ExitCode']))
+for entry in reversed(result):
 if not entry.has_key('change'):
 continue
 changes.add(int(entry['change']))
diff --git a/t/t9818-git-p4-block.sh b/t/t9818-git-p4-block.sh
index 8840a183ac..e5ec9cdec8 100755
--- a/t/t9818-git-p4-block.sh
+++ b/t/t9818-git-p4-block.sh
@@ -129,6 +129,7 @@ test_expect_success 'Create a repo with multiple depot 
paths' '
 '
 
 test_expect_success 'Clone repo with multiple depot paths' '
+   test_when_finished cleanup_git &&
(
cd "$git" &&
git p4 clone --changes-block-size=4 //depot/pathA@all 
//depot/pathB@all \
@@ -138,6 +139,16 @@ test_expect_success 'Clone repo with multiple depot paths' 
'
)
 '
 
+test_expect_success 'Clone repo with self-sizing block size' '
+   test_when_finished cleanup_git &&
+   git p4 clone --changes-block-size=100 //depot@all 
--destination="$git" &&
+   (
+   cd "$git" &&
+   git log --oneline > log &&
+   test_line_count \> 10 log
+   )
+'
+
 test_expect_success 'kill p4d' '
kill_p4d
 '
-- 
2.17.0.392.gdeb1a6e9b7

[PATCHv1 1/3] git-p4: raise exceptions from p4CmdList based on error from p4 server

[Luke Diamand]

This change lays some groundwork for better handling of rowcount errors
from the server, where it fails to send us results because we requested
too many.

It adds an option to p4CmdList() to return errors as a Python exception.

The exceptions are derived from P4Exception (something went wrong),
P4ServerException (the server sent us an error code) and
P4RequestSizeException (we requested too many rows/results from the
server database).

This makes makes the code that handles the errors a bit easier.

The default behavior is unchanged; the new code is enabled with a flag.

Signed-off-by: Luke Diamand 
---
 git-p4.py | 44 
 1 file changed, 40 insertions(+), 4 deletions(-)

diff --git a/git-p4.py b/git-p4.py
index 66652f8280..d856078ec8 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -566,10 +566,30 @@ def isModeExec(mode):
 # otherwise False.
 return mode[-3:] == "755"
 
+class P4Exception(Exception):
+""" Base class for exceptions from the p4 client """
+def __init__(self, exit_code):
+self.p4ExitCode = exit_code
+
+class P4ServerException(Exception):
+""" Base class for exceptions where we get some kind of marshalled up 
result from the server """
+def __init__(self, exit_code, p4_result):
+super(P4ServerException, self).__init__(exit_code)
+self.p4_result = p4_result
+self.code = p4_result[0]['code']
+self.data = p4_result[0]['data']
+
+class P4RequestSizeException(P4ServerException):
+""" One of the maxresults or maxscanrows errors """
+def __init__(self, exit_code, p4_result, limit):
+super(P4RequestSizeException, self).__init__(exit_code, p4_result)
+self.limit = limit
+
 def isModeExecChanged(src_mode, dst_mode):
 return isModeExec(src_mode) != isModeExec(dst_mode)
 
-def p4CmdList(cmd, stdin=None, stdin_mode='w+b', cb=None, skip_info=False):
+def p4CmdList(cmd, stdin=None, stdin_mode='w+b', cb=None, skip_info=False,
+errors_as_exceptions=False):
 
 if isinstance(cmd,basestring):
 cmd = "-G " + cmd
@@ -616,9 +636,25 @@ def p4CmdList(cmd, stdin=None, stdin_mode='w+b', cb=None, 
skip_info=False):
 pass
 exitCode = p4.wait()
 if exitCode != 0:
-entry = {}
-entry["p4ExitCode"] = exitCode
-result.append(entry)
+if errors_as_exceptions:
+if len(result) > 0:
+data = result[0].get('data')
+if data:
+m = re.search('Too many rows scanned \(over (\d+)\)', data)
+if not m:
+m = re.search('Request too large \(over (\d+)\)', data)
+
+if m:
+limit = int(m.group(1))
+raise P4RequestSizeException(exitCode, result, limit)
+
+raise P4ServerException(exitCode, result)
+else:
+raise P4Exception(exitCode)
+else:
+entry = {}
+entry["p4ExitCode"] = exitCode
+result.append(entry)
 
 return result
 
-- 
2.17.0.392.gdeb1a6e9b7

[PATCHv1 2/3] git-p4: narrow the scope of exceptions caught when parsing an int

[Luke Diamand]

The current code traps all exceptions around some code which parses an
integer, and then talks to Perforce.

That can result in errors from Perforce being ignored. Change the code
to only catch the integer conversion exceptions.

Signed-off-by: Luke Diamand 
---
 git-p4.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/git-p4.py b/git-p4.py
index d856078ec8..b337562b39 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -959,7 +959,7 @@ def p4ChangesForPaths(depotPaths, changeRange, 
requestedBlockSize):
 try:
 (changeStart, changeEnd) = p4ParseNumericChangeRange(parts)
 block_size = chooseBlockSize(requestedBlockSize)
-except:
+except ValueError:
 changeStart = parts[0][1:]
 changeEnd = parts[1]
 if requestedBlockSize:
-- 
2.17.0.392.gdeb1a6e9b7

Re: [PATCHv1 1/1] git-p4: better error reporting when p4 fails

[Luke Diamand]

On 5 June 2018 at 11:49, Merland Romain  wrote:
>
>> @@ -91,6 +93,13 @@ def p4_build_cmd(cmd):
>> real_cmd = ' '.join(real_cmd) + ' ' + cmd
>> else:
>> real_cmd += cmd
>> +
>> +# now check that we can actually talk to the server
>> +global p4_access_checked
>> +if not p4_access_checked:
>> +p4_access_checked = True
>> +p4_check_access()
>> +
>
> Just switch the 2 lines 'p4_access_checked = True' and 'p4_check_access()'
> It seems to me more logical

Like this:

+p4_check_access()
+p4_access_checked = True

You need to set p4_access_checked first so that it doesn't go and try
to check the p4 access before running "p4 login -s", which would then
get stuck forever.

>
> Romain

[PATCHv1 0/3] git-p4: improvements to p4 "blocking"

[Luke Diamand]

This patchset improves the way that git-p4 sends requests in "blocks".

The problem comes about because the Perforce server limits the maximum
number of results it will send back (there are actually two different
limits).

This causes git-p4 failures if you ask for too much data.

In commit 96b2d54aee ("git-p4: use -m when running p4 changes",
2015-04-20), git-p4 learned to make requests in blocks, by default 512
in size.

The problem with this, however, is that it can be very slow on large
repositories - you might have millions of commits, although only a
handful actually relate to the code you are trying to clone. Each 512
block request takes a sizeable fraction of a second to complete.

There's a command-line option to increase the block size, but unless you
know about it, it won't be at all obvious that you could use this to
speed things up.

This change
~~~

The function p4CmdList() has been taught to raise an exception when it
gets an error, and in particular, to notice and report the two "too many
results" errors.

The code that does the blocking can now start out with a nice large
block size, and then reduce it if it sees an error.

Luke Diamand (3):
  git-p4: raise exceptions from p4CmdList based on error from p4 server
  git-p4: narrow the scope of exceptions caught when parsing an int
  git-p4: auto-size the block

 git-p4.py   | 72 +++--
 t/t9818-git-p4-block.sh | 11 +++
 2 files changed, 73 insertions(+), 10 deletions(-)

-- 
2.17.0.392.gdeb1a6e9b7

Re: [PATCHv1 1/3] git-p4: raise exceptions from p4CmdList based on error from p4 server

[Luke Diamand]

On 5 June 2018 at 20:41, Eric Sunshine  wrote:
> On Tue, Jun 5, 2018 at 6:56 AM Luke Diamand  wrote:
>> On 5 June 2018 at 10:54, Eric Sunshine  wrote:
>> > On Tue, Jun 5, 2018 at 5:14 AM Luke Diamand  wrote:
>> >> +m = re.search('Too many rows scanned \(over 
>> >> (\d+)\)', data)
>> >> +if not m:
>> >> +m = re.search('Request too large \(over 
>> >> (\d+)\)', data)
>> >
>> > Does 'p4' localize these error messages?
>>
>> That's a good question.
>>
>> It turns out that Perforce open-sourced the P4 client in 2014 (I only
>> recently found this out) so we can actually look at the code now!
>>
>> Here's the code:
>>
>> // ErrorId graveyard: retired/deprecated ErrorIds.
>
> Hmm, the "too many rows" error you're seeing is retired/deprecated(?).

There's some code elsewhere that suggests it's being kept alive:

// Retired ErrorIds. We need to keep these so that clients
// built with newer apis can commnunicate with older servers
// still sending these.

static ErrorId MaxResults; // DEPRECATED
static ErrorId MaxScanRows; // DEPRECATED


>
>> ErrorId MsgDb::MaxResults  = { ErrorOf( ES_DB, 32,
>> E_FAILED, EV_ADMIN, 1 ), "Request too large (over %maxResults%); see
>> 'p4 help maxresults'." } ;//NOTRANS
>> ErrorId MsgDb::MaxScanRows = { ErrorOf( ES_DB, 61,
>> E_FAILED, EV_ADMIN, 1 ), "Too many rows scanned (over %maxScanRows%);
>> see 'p4 help maxscanrows'." } ;//NOTRANS
>>
>> I don't think there's actually a way to make it return any language
>> other than English though. [...]
>> So I think probably the language is always English.
>
> The "NOTRANS" annotation on the error messages is reassuring.

I'll check it works OK on Windows; charset translation might cause a problem.

Re: What's cooking in git.git (Jun 2018, #02; Mon, 4)

[Luke Diamand]

On 4 June 2018 at 14:57, Junio C Hamano  wrote:
> Here are the topics that have been cooking.  Commits prefixed with
> '-' are only in 'pu' (proposed updates) while commits prefixed with
> '+' are in 'next'.  The ones marked with '.' do not appear in any of
> the integration branches, but I am still holding onto them.
>
> Generally I try to avoid sending issues of "What's cooking" report
> in close succession, but as I plan to go offline for most of the
> remainder of the week, here is how the tree looks like as of
> tonight, just after tagging 2.18-rc1.  Hopefully we'll have another
> rc next week and then the real thing a week after that.
>
> You can find the changes described here in the integration branches
> of the repositories listed at
>
> http://git-blame.blogspot.com/p/git-public-repositories.html
>



>
> * rm/p4-submit-with-commit-option (2018-05-21) 1 commit
>  - git-p4: add options --commit and --disable-rebase
>
>  Needs sign-off.

I think the signed-off version was sent in just last week:

http://lists-archives.com/git/925692-git-p4-add-options-commit-and-disable-rebase.html

Thanks,
Luke

Re: [PATCH 17/29] t: use test_must_fail() instead of checking exit code manually

[Luke Diamand]

On 26 June 2018 at 08:29, Eric Sunshine  wrote:
> These tests intentionally break the &&-chain to manually check the exit
> code of invoked commands which they expect to fail, and invert that
> local expected failure into a successful exit code for the test overall.
> Such manual exit code manipulation predates the invention of
> test_must_fail().
>
> An upcoming change will teach --chain-lint to check the &&-chain inside
> subshells. This sort of manual exit code checking will trip up
> --chain-lint due to the intentional break in the &&-chain. Therefore,
> replace the manual exit code management with test_must_fail() and a
> normal &&-chain.
>
> Signed-off-by: Eric Sunshine 
> ---
>  t/t5405-send-pack-rewind.sh |  3 +--
>  t/t9814-git-p4-rename.sh| 16 ++--
>  2 files changed, 3 insertions(+), 16 deletions(-)
>
> diff --git a/t/t5405-send-pack-rewind.sh b/t/t5405-send-pack-rewind.sh
> index 4bda18a662..235fb7686a 100755
> --- a/t/t5405-send-pack-rewind.sh
> +++ b/t/t5405-send-pack-rewind.sh
> @@ -25,8 +25,7 @@ test_expect_success 'non forced push should die not 
> segfault' '
>
> (
> cd another &&
> -   git push .. master:master
> -   test $? = 1
> +   test_must_fail git push .. master:master
> )
>
>  '
> diff --git a/t/t9814-git-p4-rename.sh b/t/t9814-git-p4-rename.sh
> index e7e0268e98..80aac5ab16 100755
> --- a/t/t9814-git-p4-rename.sh
> +++ b/t/t9814-git-p4-rename.sh
> @@ -12,20 +12,8 @@ test_expect_success 'start p4d' '
>  test_expect_success 'p4 help unknown returns 1' '
> (
> cd "$cli" &&
> -   (
> -   p4 help client >errs 2>&1
> -   echo $? >retval
> -   )
> -   echo 0 >expected &&
> -   test_cmp expected retval &&
> -   rm retval &&
> -   (
> -   p4 help nosuchcommand >errs 2>&1
> -   echo $? >retval
> -   )
> -   echo 1 >expected &&
> -   test_cmp expected retval &&
> -   rm retval
> +   p4 help client &&
> +   test_must_fail p4 help nosuchcommand

This seems a lot more sensible. It works fine in my tests.

Thanks,
Luke

Re: [PATCH] git-p4 - Add option --sha1 to submit in p4

[Luke Diamand]

On 2 May 2018 at 15:32, Merland Romain <merlo...@yahoo.fr> wrote:
> From 4867808cad2b759ebf31c275356e602b72c5659f Mon Sep 17 00:00:00 2001
> From: Romain Merland <merlo...@yahoo.fr>
> To: git@vger.kernel.org
> Cc: Junio C Hamano <gits...@pobox.com>, Jeff King <p...@peff.net>, Luke
> Diamand <l...@diamand.org>, Vinicius Kursancew <viniciusalexan...@gmail.com>
> Date: Wed, 2 May 2018 15:02:11 +0200
> Subject: [PATCH] git-p4 - Add option --sha1 to submit in p4
>
> Add option --sha1 to command 'git-p4 submit' in order to submit in p4 a
> commit
> that is not necessarily on master.
> In that case, don't rebase the submitted changes.

That could be very useful, I often find the commit I want to submit is
half-way down a long list of other commits.

Currently I end up cherry-picking the one I want into a clean repo,
but that's much more awkward than your --sha1 change.

A few comments inline:

> Signed-off-by: Romain Merland <merlo...@yahoo.fr>
> ---
>  git-p4.py | 21 +++--
>  1 file changed, 15 insertions(+), 6 deletions(-)
>
> diff --git a/git-p4.py b/git-p4.py
> index 7bb9cadc6..d64ff79dd 100755
> --- a/git-p4.py
> +++ b/git-p4.py
> @@ -1352,7 +1352,9 @@ class P4Submit(Command, P4UserMap):
>  optparse.make_option("--update-shelve",
> dest="update_shelve", action="append", type="int",
>   metavar="CHANGELIST",
>   help="update an existing shelved
> changelist, implies --shelve, "
> -   "repeat in-order for multiple
> shelved changelists")
> +   "repeat in-order for multiple
> shelved changelists"),
> +optparse.make_option("--sha1", dest="sha1", metavar="SHA1",
> + help="submit only the specified
> commit, don't rebase afterwards")

Is there a better name than "sha1" ? If git ever changes its hash to
something else will this still make sense?

I wonder why you wouldn't rebase afterwards?

Perhaps an additional option to skip the rebase?

>  ]
>  self.description = "Submit changes from git to the perforce depot."
>  self.usage += " [name of git branch to submit into perforce depot]"
> @@ -1362,6 +1364,7 @@ class P4Submit(Command, P4UserMap):
>  self.dry_run = False
>  self.shelve = False
>  self.update_shelve = list()
> +self.sha1 = ""
>  self.prepare_p4_only = False
>  self.conflict_behavior = None
>  self.isWindows = (platform.system() == "Windows")
> @@ -2103,9 +2106,12 @@ class P4Submit(Command, P4UserMap):
>  else:
>  commitish = 'HEAD'
>
> -for line in read_pipe_lines(["git", "rev-list", "--no-merges",
> "%s..%s" % (self.origin, commitish)]):
> -commits.append(line.strip())
> -commits.reverse()
> +if self.sha1 != "":
> +commits.append(self.sha1)
> +else:
> +for line in read_pipe_lines(["git", "rev-list", "--no-merges",
> "%s..%s" % (self.origin, commitish)]):
> +commits.append(line.strip())
> +commits.reverse()
>
>  if self.preserveUser or gitConfigBool("git-p4.skipUserNameCheck"):
>  self.checkAuthorship = False
> @@ -2215,8 +2221,11 @@ class P4Submit(Command, P4UserMap):
>  sync.branch = self.branch
>  sync.run([])
>
> -rebase = P4Rebase()
> -rebase.rebase()
> +if self.sha1 == "":

if not self.skip_rebase:

> +rebase = P4Rebase()
> +rebase.rebase()
> +else:
> +print "You will have to do 'git p4 sync' and rebase."
>
>  else:
>  if len(applied) == 0:
> --
> 2.17.0
>
>

This would be better with some documentation in git-p4.txt and a test case!

Regards!
Luke

Re: [PATCH] git-p4 - Add option --sha1 to submit in p4

[Luke Diamand]

On 2 May 2018 at 16:51, Merland Romain  wrote:
> Thanks Luke,
>
> Following your comments, I will:
> - change the option name to --commit if it suits you

Seems like a good name.

> - add an option --force-rebase which defaults to false. Setting it to true
> will rebase even with --commit option used.

Or "--disable-rebase" ?  I think there's no reason you couldn't rebase
after doing a commit like this is there?

And "--disable-rebase" would be useful generally, not just with the
--commit option, but also with the regular options.

"--force-rebase" is a bit confusing since for the normal flow, it
always rebases anyway, and there's no need to force!

But you're the one who is using this in anger, so your opinion likely
counts for more than mine!

> it can be useful too if some commits have been skipped and user wants to
> rebase anyway
> - add an entry in the documentation
> - (learn how to create a test and) add tests for these specific cases

To create a test just look in t/ for the t98*.sh files. Cut-n-paste
one or add to an existing one. Send an email here if you get stuck
(but it's pretty straightforward).

You can run only the git-p4 tests with:

$ (cd t && make T=t98* -j)

>
> What is the best practice ? to send a new email with the new diff ? to
> continue this discussion ?

I think either, probably a new email is easiest. See
Documentation/SubmittingPatches for the general process.


Regards!
Luke

git-p4 + watchman - watching the p4 repo?

[Luke Diamand]

Hi!

I could be wrong about this, but I when I tried mixing watchman with
git-p4, I found that on "git p4 submit" it ended up watching the p4
repo, which seems a bit pointless (and was also very slow).

$ [create git-p4 clone of some p4 repo]
$ : >bar
$ git add bar && git commit -m 'adding bar'
$ git p4 submit --origin HEAD^ --shelve
Perforce checkout for depot path //depot/ located at /tmp/p4/cli/
Synchronizing p4 checkout...
... - file(s) up-to-date.
Applying 4ce4057 change
//depot/bar#1 - opened for edit
Adding '/tmp/p4/cli' to watchman's watch list.

Is there any way to stop it doing this?

Thanks!
Luke

[PATCH 1/1] git-p4: add unshelve command

[Luke Diamand]

This can be used to "unshelve" a shelved P4 commit into
a git commit.

For example:

  $ git p4 unshelve 12345

The resulting commit ends up in the branch:
   refs/remotes/p4/unshelved/12345

Signed-off-by: Luke Diamand <l...@diamand.org>
---
 Documentation/git-p4.txt |  22 
 git-p4.py| 128 +++
 t/t9832-unshelve.sh  |  67 +
 3 files changed, 186 insertions(+), 31 deletions(-)
 create mode 100755 t/t9832-unshelve.sh

diff --git a/Documentation/git-p4.txt b/Documentation/git-p4.txt
index d8c8f11c9f..910ae63a1c 100644
--- a/Documentation/git-p4.txt
+++ b/Documentation/git-p4.txt
@@ -164,6 +164,21 @@ $ git p4 submit --shelve
 $ git p4 submit --update-shelve 1234 --update-shelve 2345
 
 
+
+Unshelve
+
+Unshelving will take a shelved P4 changelist, and produce the equivalent git 
commit
+in the branch refs/remotes/p4/unshelved/.
+
+The git commit is created relative to the current p4/master branch, so if this
+branch is behind Perforce itself, it may include more changes than you 
expected.
+
+
+$ git p4 sync
+$ git p4 unshelve 12345
+$ git show refs/remotes/p4/unshelved/12345
+
+
 OPTIONS
 ---
 
@@ -337,6 +352,13 @@ These options can be used to modify 'git p4 rebase' 
behavior.
 --import-labels::
Import p4 labels.
 
+Unshelve options
+
+
+--origin::
+Sets the git refspec against which the shelved P4 changelist is compared.
+Defaults to p4/master.
+
 DEPOT PATH SYNTAX
 -
 The p4 depot path argument to 'git p4 sync' and 'git p4 clone' can
diff --git a/git-p4.py b/git-p4.py
index 7bb9cadc69..59bd4ff64f 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -316,12 +316,17 @@ def p4_last_change():
 results = p4CmdList(["changes", "-m", "1"], skip_info=True)
 return int(results[0]['change'])
 
-def p4_describe(change):
+def p4_describe(change, shelved=False):
 """Make sure it returns a valid result by checking for
the presence of field "time".  Return a dict of the
results."""
 
-ds = p4CmdList(["describe", "-s", str(change)], skip_info=True)
+cmd = ["describe", "-s"]
+if shelved:
+cmd += ["-S"]
+cmd += [str(change)]
+
+ds = p4CmdList(cmd, skip_info=True)
 if len(ds) != 1:
 die("p4 describe -s %d did not return 1 result: %s" % (change, 
str(ds)))
 
@@ -2421,6 +2426,18 @@ class P4Sync(Command, P4UserMap):
 if gitConfig("git-p4.syncFromOrigin") == "false":
 self.syncWithOrigin = False
 
+self.depotPaths = []
+self.changeRange = ""
+self.previousDepotPaths = []
+self.hasOrigin = False
+
+# map from branch depot path to parent branch
+self.knownBranches = {}
+self.initialParents = {}
+
+self.tz = "%+03d%02d" % (- time.timezone / 3600, ((- time.timezone % 
3600) / 60))
+self.labels = {}
+
 # Force a checkpoint in fast-import and wait for it to finish
 def checkpoint(self):
 self.gitStream.write("checkpoint\n\n")
@@ -2429,7 +2446,7 @@ class P4Sync(Command, P4UserMap):
 if self.verbose:
 print "checkpoint finished: " + out
 
-def extractFilesFromCommit(self, commit):
+def extractFilesFromCommit(self, commit, shelved=False, shelved_cl = 0):
 self.cloneExclude = [re.sub(r"\.\.\.$", "", path)
  for path in self.cloneExclude]
 files = []
@@ -2452,6 +2469,9 @@ class P4Sync(Command, P4UserMap):
 file["rev"] = commit["rev%s" % fnum]
 file["action"] = commit["action%s" % fnum]
 file["type"] = commit["type%s" % fnum]
+if shelved:
+file["shelved_cl"] = int(shelved_cl)
+
 files.append(file)
 fnum = fnum + 1
 return files
@@ -2743,7 +2763,16 @@ class P4Sync(Command, P4UserMap):
 def streamP4FilesCbSelf(entry):
 self.streamP4FilesCb(entry)
 
-fileArgs = ['%s#%s' % (f['path'], f['rev']) for f in filesToRead]
+fileArgs = []
+for f in filesToRead:
+if 'shelved_cl' in f:
+# Handle shelved CLs using the "p4 print file@=N" syntax 
to print
+# the contents
+fileArg = '%s@=%d' % (f['path'], f['shelved_cl'])
+else:
+fileArg = '%s#%s' % (f['path'], f['rev'])
+
+fileArgs.append(fileArg)
 
 p4CmdList(["-x", "-", "print"],
   stdin=fileArgs,
@@ -3162,10 +3191,10 @@ class P4Sync(Command, P4UserMap

[PATCH 0/1] git-p4: add unshelve command

[Luke Diamand]

This is an initial attempt at adding an unshelve command to
git-p4.

For those not familiar with it, p4 shelve creates a "pending"
changelist, which isn't committed into the central repo but is
nonetheless visible to other develoeprs. The "unshelve" command
takes one of these pending changelists and applies it to your repo.
It is used quite a lot for code review.

git-p4 learned about shelving changelists recently; this completes
the picture by letting you unshelve them as well.

This was inspired by the stackoverflow answer here:


https://stackoverflow.com/questions/41841917/git-p4-how-to-fetch-a-changelist

The secret is to use the "p4 print file@=N" syntax to get the
contents of a shelved changelist, which has long perplexed me.

I haven't used this a great deal, so it may still have a few rough
edges.

In particular, it currently puts the unshelved commit into
refs/remotes/p4/unshelved/

where  is the changelist being unshelved. That might not be
the best way to do this.


Luke Diamand (1):
  git-p4: add unshelve command

 Documentation/git-p4.txt |  22 
 git-p4.py| 128 +++
 t/t9832-unshelve.sh  |  67 +
 3 files changed, 186 insertions(+), 31 deletions(-)
 create mode 100755 t/t9832-unshelve.sh

-- 
2.15.1.272.gc310869385

Re: [PATCH 1/1] git-p4: add unshelve command

[Luke Diamand]

On 22 February 2018 at 21:39, Miguel Torroja  wrote:
> Hi Luke,
>
> I really like the idea of creating a branch based on a shelved CL (We
> particularly use shelves all the time), I tested your change and I
> have some comments.
>
>  - I have some concerns about having the same "[git-p4...change =
> .]" as if it were a real submitted CL.
> One use case I foresee of the new implementation could be to
> cherry-pick that change on another branch (or current branch) prior to
> a git p4 submit.

OK, I think we could just not add that in the case of an unshelved commit.

>
>  - I see that the new p4/unshelve... branch is based on the tip of
> p4/master by default. what if we set the default to the current HEAD?

There's a "--origin" option you can use to set it to whatever you want.

I started out with HEAD as the default, but then found that to get a
sensible diff you have to both sync and rebase, which can be quite
annoying.

In my case, in my early testing, I ended up with a git commit which
included both the creation of a file, and a subsequent change, even
though I had only unshelved the subsequent change. That was because
HEAD didn't include the file creation change (but p4/master _did_).

>
>  - Shelved CLs can be updated and you might have to run "git p4
> unshelve" a second time to get the latest updates. if we call it a
> second time it fails as it's trying to update p4/unshelve... rather
> than discarding previous one and creating a new one.

OK, that should also be fixable.

>
>
> Thanks,

Thanks for the feedback, very useful! I'll reroll.
Luke

Re: [PATCH 1/1] git-p4: add unshelve command

[Luke Diamand]

On 22 February 2018 at 22:28, Luke Diamand <l...@diamand.org> wrote:
> On 22 February 2018 at 21:39, Miguel Torroja <miguel.torr...@gmail.com> wrote:
>> Hi Luke,
>>
>> I really like the idea of creating a branch based on a shelved CL (We
>> particularly use shelves all the time), I tested your change and I
>> have some comments.
>>
>>  - I have some concerns about having the same "[git-p4...change =
>> .]" as if it were a real submitted CL.
>> One use case I foresee of the new implementation could be to
>> cherry-pick that change on another branch (or current branch) prior to
>> a git p4 submit.
>
> OK, I think we could just not add that in the case of an unshelved commit.
>
>>
>>  - I see that the new p4/unshelve... branch is based on the tip of
>> p4/master by default. what if we set the default to the current HEAD?
>
> There's a "--origin" option you can use to set it to whatever you want.
>
> I started out with HEAD as the default, but then found that to get a
> sensible diff you have to both sync and rebase, which can be quite
> annoying.
>
> In my case, in my early testing, I ended up with a git commit which
> included both the creation of a file, and a subsequent change, even
> though I had only unshelved the subsequent change. That was because
> HEAD didn't include the file creation change (but p4/master _did_).

Discussing this with some of my colleagues, and playing around with
it, it seems that what it really needs to do is to figure out the
parent commit of the shelved changelist, and use that as the basis for
the diff.

Unfortunately, Perforce doesn't have any concept of a "parent commit".
One option that would be possible to implement though is to look at
the shelved changelist, and foreach file, find the original revision
number ("//depot/foo.c#97"). Then "p4 changes //depot/foo.c" would
give you the changelist number for that file. Find the most recent P4
changelist, find the git commit corresponding to that, and do the diff
against that.

It's pretty clunky, and I'm quite glad I didn't try to do that in the
initial revision, as I would surely have given up!

To do it properly of course you need to handle the case where the
shelved changelist author had some files at one changelist, and others
at another. But I think that's just far too complicated to deal with.

Luke

Re: [PATCH] git-p4: Fix depot path stripping

[Luke Diamand]

On 27 February 2018 at 04:16, Nuno Subtil  wrote:
> When useClientSpec=true, stripping of P4 depot paths doesn't happen
> correctly on sync. Modifies stripRepoPath to handle this case better.

Can you give an example of how this shows up? I could quickly write a
test case for this if I knew what was going on.

Thanks
Luke


>
> Signed-off-by: Nuno Subtil 
> ---
>  git-p4.py | 12 +---
>  1 file changed, 9 insertions(+), 3 deletions(-)
>
> diff --git a/git-p4.py b/git-p4.py
> index 7bb9cadc69738..3df95d0fd1d83 100755
> --- a/git-p4.py
> +++ b/git-p4.py
> @@ -2480,7 +2480,7 @@ def stripRepoPath(self, path, prefixes):
>  if path.startswith(b + "/"):
>  path = path[len(b)+1:]
>
> -elif self.keepRepoPath:
> +if self.keepRepoPath:
>  # Preserve everything in relative path name except leading
>  # //depot/; just look at first prefix as they all should
>  # be in the same depot.
> @@ -2490,6 +2490,12 @@ def stripRepoPath(self, path, prefixes):
>
>  else:
>  for p in prefixes:
> +   if self.useClientSpec and not self.keepRepoPath:
> +# when useClientSpec is false, the prefix will contain 
> the depot name but the path will not
> +# extract the depot name and add it to the path so the 
> match below will do the right thing
> +depot = re.sub("^(//[^/]+/).*", r'\1', p)
> +path = depot + path
> +
>  if p4PathStartsWith(path, p):
>  path = path[len(p):]
>  break
> @@ -2526,8 +2532,8 @@ def splitFilesIntoBranches(self, commit):
>  # go in a p4 client
>  if self.useClientSpec:
>  relPath = self.clientSpecDirs.map_in_client(path)
> -else:
> -relPath = self.stripRepoPath(path, self.depotPaths)
> +
> +relPath = self.stripRepoPath(path, self.depotPaths)
>
>  for branch in self.knownBranches.keys():
>  # add a trailing slash so that a commit into qt/4.2foo
>
> --
> https://github.com/git/git/pull/463

[PATCH 5/6] git-p4: python3: use print() function

[Luke Diamand]

Replace calls to print ... with the function form, print(...), to
allow use with python3 as well as python2.x.

Converted using 2to3 (and some hand-editing).

Signed-off-by: Luke Diamand 
---
 git-p4.py | 248 +++---
 1 file changed, 124 insertions(+), 124 deletions(-)

diff --git a/git-p4.py b/git-p4.py
index f127ebce27..714e442d7c 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -892,7 +892,7 @@ def findUpstreamBranchPoint(head = "HEAD"):
 
 def createOrUpdateBranchesFromOrigin(localRefPrefix = "refs/remotes/p4/", 
silent=True):
 if not silent:
-print ("Creating/updating branch(es) in %s based on origin branch(es)"
+print("Creating/updating branch(es) in %s based on origin branch(es)"
% localRefPrefix)
 
 originPrefix = "origin/p4/"
@@ -914,7 +914,7 @@ def createOrUpdateBranchesFromOrigin(localRefPrefix = 
"refs/remotes/p4/", silent
 update = False
 if not gitBranchExists(remoteHead):
 if verbose:
-print "creating %s" % remoteHead
+print("creating %s" % remoteHead)
 update = True
 else:
 settings = 
extractSettingsGitLog(extractLogMessageFromGitCommit(remoteHead))
@@ -923,13 +923,13 @@ def createOrUpdateBranchesFromOrigin(localRefPrefix = 
"refs/remotes/p4/", silent
 originP4Change = int(original['change'])
 p4Change = int(settings['change'])
 if originP4Change > p4Change:
-print ("%s (%s) is newer than %s (%s). "
+print("%s (%s) is newer than %s (%s). "
"Updating p4 branch from origin."
% (originHead, originP4Change,
   remoteHead, p4Change))
 update = True
 else:
-print ("Ignoring: %s was imported from %s while "
+print("Ignoring: %s was imported from %s while "
"%s was imported from %s"
% (originHead, ','.join(original['depot-paths']),
   remoteHead, ','.join(settings['depot-paths'])))
@@ -1397,9 +1397,9 @@ def __init__(self):
 def run(self, args):
 j = 0
 for output in p4CmdList(args):
-print 'Element: %d' % j
+print('Element: %d' % j)
 j += 1
-print output
+print(output)
 return True
 
 class P4RollBack(Command):
@@ -1440,14 +1440,14 @@ def run(self, args):
 
 if len(p4Cmd("changes -m 1 "  + ' '.join (['%s...@%s' % (p, 
maxChange)
for p in 
depotPaths]))) == 0:
-print "Branch %s did not exist at change %s, deleting." % 
(ref, maxChange)
+print("Branch %s did not exist at change %s, deleting." % 
(ref, maxChange))
 system("git update-ref -d %s `git rev-parse %s`" % (ref, 
ref))
 continue
 
 while change and int(change) > maxChange:
 changed = True
 if self.verbose:
-print "%s is at %s ; rewinding towards %s" % (ref, 
change, maxChange)
+print("%s is at %s ; rewinding towards %s" % (ref, 
change, maxChange))
 system("git update-ref %s \"%s^\"" % (ref, ref))
 log = extractLogMessageFromGitCommit(ref)
 settings =  extractSettingsGitLog(log)
@@ -1457,7 +1457,7 @@ def run(self, args):
 change = settings['change']
 
 if changed:
-print "%s rewound to %s" % (ref, change)
+print("%s rewound to %s" % (ref, change))
 
 return True
 
@@ -1593,10 +1593,10 @@ def patchRCSKeywords(self, file, pattern):
 except:
 # cleanup our temporary file
 os.unlink(outFileName)
-print "Failed to strip RCS keywords in %s" % file
+print("Failed to strip RCS keywords in %s" % file)
 raise
 
-print "Patched up RCS keywords in %s" % file
+print("Patched up RCS keywords in %s" % file)
 
 def p4UserForCommit(self,id):
 # Return the tuple (perforce user,git email) for a given git commit id
@@ -1616,7 +1616,7 @@ def checkValidP4Users(self,commits):
 if not user:
 msg = "Cannot find p4 user for email %s in commit %s." % 
(email, id)
 if gitConfigBool("git-p4.allowMissingP4Users"

[PATCH 3/6] git-p4: python3: remove backticks

[Luke Diamand]

Backticks around a variable are a deprecated alias for repr().
This has been removed in python3, so just use the string
representation instead, which is equivalent.

Signed-off-by: Luke Diamand 
---
 git-p4.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/git-p4.py b/git-p4.py
index 6fcad35104..67865d14aa 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -3089,7 +3089,7 @@ def getLabels(self):
 
 l = p4CmdList(["labels"] + ["%s..." % p for p in self.depotPaths])
 if len(l) > 0 and not self.silent:
-print "Finding files belonging to labels in %s" % `self.depotPaths`
+print("Finding files belonging to labels in %s" % self.depotPaths)
 
 for output in l:
 label = output["label"]
-- 
2.18.0.rc1.242.g61856ae69a

Re: [PATCH 1/1] Add the `p4-pre-submit` hook

[Luke Diamand]

On 30 July 2018 at 20:07, Junio C Hamano  wrote:
> Chen Bin  writes:
>
>> The `p4-pre-submit` hook is executed before git-p4 submits code.
>> If the hook exits with non-zero value, submit process not start.
>>
>> Signed-off-by: Chen Bin 
>> ---
>
> Luke, does this version look good to you?
>

Yes, I think so, We might find in the future that we do need an
additional hook *after* the change has been applied, but as per Chen's
comment, it sounds like that's not what is needed here; it can easily
be added in the future if necessary.

And there's no directly equivalent existing git-hook which could be
used instead, so this seems like a useful addition.

Possibly it should say "it takes no parameters" rather than "it takes
no parameter"; it is confusing that in English, zero takes the plural
rather than the singular. There's a PhD in linguistics there for
someone!

Luke


> I still think the addition to self.description is a bit too much but
> other than that the incremental since the last one looked sensible
> to my untrained eyes ;-)
>
> Thanks, both.
>
>>  Documentation/git-p4.txt   |  8 
>>  Documentation/githooks.txt |  7 +++
>>  git-p4.py  | 16 +++-
>>  t/t9800-git-p4-basic.sh| 29 +
>>  4 files changed, 59 insertions(+), 1 deletion(-)
>>
>> diff --git a/Documentation/git-p4.txt b/Documentation/git-p4.txt
>> index f0de3b891..a7aac1b92 100644
>> --- a/Documentation/git-p4.txt
>> +++ b/Documentation/git-p4.txt
>> @@ -374,6 +374,14 @@ These options can be used to modify 'git p4 submit' 
>> behavior.
>>  been submitted. Implies --disable-rebase. Can also be set with
>>  git-p4.disableP4Sync. Sync with origin/master still goes ahead if 
>> possible.
>>
>> +Hook for submit
>> +~~~
>> +The `p4-pre-submit` hook is executed if it exists and is executable.
>> +The hook takes no parameter and nothing from standard input. Exiting with
>> +non-zero status from this script prevents `git-p4 submit` from launching.
>> +
>> +One usage scenario is to run unit tests in the hook.
>> +
>>  Rebase options
>>  ~~
>>  These options can be used to modify 'git p4 rebase' behavior.
>> diff --git a/Documentation/githooks.txt b/Documentation/githooks.txt
>> index e3c283a17..22fcabbe2 100644
>> --- a/Documentation/githooks.txt
>> +++ b/Documentation/githooks.txt
>> @@ -485,6 +485,13 @@ The exit status determines whether git will use the 
>> data from the
>>  hook to limit its search.  On error, it will fall back to verifying
>>  all files and folders.
>>
>> +p4-pre-submit
>> +~
>> +
>> +This hook is invoked by `git-p4 submit`. It takes no parameter and nothing
>> +from standard input. Exiting with non-zero status from this script prevent
>> +`git-p4 submit` from launching. Run `git-p4 submit --help` for details.
>> +
>>  GIT
>>  ---
>>  Part of the linkgit:git[1] suite
>> diff --git a/git-p4.py b/git-p4.py
>> index b449db1cc..879abfd2b 100755
>> --- a/git-p4.py
>> +++ b/git-p4.py
>> @@ -1494,7 +1494,13 @@ def __init__(self):
>>  optparse.make_option("--disable-p4sync", 
>> dest="disable_p4sync", action="store_true",
>>   help="Skip Perforce sync of p4/master 
>> after submit or shelve"),
>>  ]
>> -self.description = "Submit changes from git to the perforce depot."
>> +self.description = """Submit changes from git to the perforce 
>> depot.\n
>> +The `p4-pre-submit` hook is executed if it exists and is executable.
>> +The hook takes no parameter and nothing from standard input. Exiting 
>> with
>> +non-zero status from this script prevents `git-p4 submit` from 
>> launching.
>> +
>> +One usage scenario is to run unit tests in the hook."""
>> +
>>  self.usage += " [name of git branch to submit into perforce depot]"
>>  self.origin = ""
>>  self.detectRenames = False
>> @@ -2303,6 +2309,14 @@ def run(self, args):
>>  sys.exit("number of commits (%d) must match number of shelved 
>> changelist (%d)" %
>>   (len(commits), num_shelves))
>>
>> +hooks_path = gitConfig("core.hooksPath")
>> +if len(hooks_path) <= 0:
>> +hooks_path = os.path.join(os.environ.get("GIT_DIR", ".git"), 
>> "hooks")
>> +
>> +hook_file = os.path.join(hooks_path, "p4-pre-submit")
>> +if os.path.isfile(hook_file) and os.access(hook_file, os.X_OK) and 
>> subprocess.call([hook_file]) != 0:
>> +sys.exit(1)
>> +
>>  #
>>  # Apply the commits, one at a time.  On failure, ask if should
>>  # continue to try the rest of the patches, or quit.
>> diff --git a/t/t9800-git-p4-basic.sh b/t/t9800-git-p4-basic.sh
>> index 4849edc4e..2b7baa95d 100755
>> --- a/t/t9800-git-p4-basic.sh
>> +++ b/t/t9800-git-p4-basic.sh
>> @@ -261,6 +261,35 @@ test_expect_success 'unresolvable host in P4PORT should 
>> display error' '
>>   )
>>  '
>>
>> +# Test 

Re: [PATCH 1/1] add hook pre-p4-submit

[Luke Diamand]

On 26 July 2018 at 10:21, Eric Sunshine  wrote:
> On Wed, Jul 25, 2018 at 10:08 PM chen bin  wrote:
>> The hook does not receive any information or input from git. The
>> original requirement
>> comes from my colleague. He want to run unit test automatically before
>> submitting code
>> to remote repository. Or else CI server will send the blame mail to the 
>> manager.
>
> Okay, that seems in line with a hook such as pre-commit. Please do
> update the documentation to mention that the hook takes no arguments
> and nothing on standard input, and perhaps describe in the
> documentation an example use-case (as you did here).
>
> I'm not a p4 or git-p4 user, but, out of curiosity, would there be any
> information which could be supplied to the hook as arguments or
> standard input (or both) which would help the hook author implement
> the hook more easily? Perhaps such information would be fodder for a
> future enhancement (not necessarily needed for this patch).


I tried to think of a use-case for a hook requiring any more
information, but I can't think of any. You're already chdir()'d to the
P4 shadow repo which is what you really need.

Anything where you just need the commit hash (e.g. checking the commit
message) can already be done with one of the existing git hooks; I
don't think git-p4 needs to duplicate that.

And we can't write a commit hook that can know about the Perforce
changelist, because we don't know what it is yet.

However, looking at the code, it runs the hook at the point just
*before* the changes are applied to the P4 shadow repo. Would it make
more sense to run the hook *after* they have been applied (but before
being P4 submitted) ?

That way you can run your tests on the checked-out P4 shadow directory
with your changes - as it stands, you can only run them on your git
repo at this point, which might not be in sync with Perforce (and
could be quite a long way out in fact).

Luke


>
>> The hook actually stops the submit process from start instead of abort
>> submit in midway.
>> So nothing is touched when hook exits with status 1.
>
> This might be a good thing to add to the documentation, as well.
>
>> I'm not sure whether `git-p4` should print some "hook rejection" message.
>> Current implementation is same as other hooks (`pre-commit`, for example).
>> Only hook itself is responsible to print error messages.
>>
>> Personally I don't have opinion whether we should print out hook
>> related message inside
>> `git-p4.py`. I just try to following existing convention of git.
>>
>> What you guys think?
>
> Following existing practice makes sense. It can always be revisited
> later if needed.
>
> Thanks.

Re: [PATCH 1/1] Add the `p4-pre-submit` hook

[Luke Diamand]

I think there is an error in the test harness.

On 31 July 2018 at 10:46, SZEDER Gábor  wrote:
>> + test_must_fail git-p4 submit --dry-run >errs 2>&1 &&>
>> + ! grep "Would apply" err

It writes to the file "errs" but then looks for the message in "err".

Luke

Re: [PATCH 1/1] Add the `p4-pre-submit` hook

[Luke Diamand]

On 31 July 2018 at 16:40, Junio C Hamano  wrote:
> Luke Diamand  writes:
>
>> I think there is an error in the test harness.
>>
>> On 31 July 2018 at 10:46, SZEDER Gábor  wrote:
>>>> + test_must_fail git-p4 submit --dry-run >errs 2>&1 &&>
>>>> + ! grep "Would apply" err
>>
>> It writes to the file "errs" but then looks for the message in "err".
>>
>> Luke
>
> Sigh. Thanks for spotting, both of you.
>
> Here is what I'd locally squash in.  If there is anything else, I'd
> rather see a refreshed final one sent by the author.
>
> Thanks.
>
>  t/t9800-git-p4-basic.sh | 8 
>  1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/t/t9800-git-p4-basic.sh b/t/t9800-git-p4-basic.sh
> index 2b7baa95d2..729cd25770 100755
> --- a/t/t9800-git-p4-basic.sh
> +++ b/t/t9800-git-p4-basic.sh
> @@ -274,19 +274,19 @@ test_expect_success 'run hook p4-pre-submit before 
> submit' '
> git add hello.txt &&
> git commit -m "add hello.txt" &&
> git config git-p4.skipSubmitEdit true &&
> -   git-p4 submit --dry-run >out &&
> +   git p4 submit --dry-run >out &&
> grep "Would apply" out &&
> mkdir -p .git/hooks &&
> write_script .git/hooks/p4-pre-submit <<-\EOF &&
> exit 0
> EOF
> -   git-p4 submit --dry-run >out &&
> +   git p4 submit --dry-run >out &&
> grep "Would apply" out &&
> write_script .git/hooks/p4-pre-submit <<-\EOF &&
> exit 1
> EOF
> -   test_must_fail git-p4 submit --dry-run >errs 2>&1 &&
> -   ! grep "Would apply" err
> +   test_must_fail git p4 submit --dry-run >errs 2>&1 &&
> +   ! grep "Would apply" errs
> )
>  '

That set of deltas works for me.

Sorry, I should have run the tests myself earlier and I would have
picked up on this.

Re: [PATCH 1/1] add hook pre-p4-submit

[Luke Diamand]

On 23 July 2018 at 12:27, Chen Bin  wrote:
> Hook pre-p4-submit is executed before git-p4 actually submits code.
> If the hook exits with non-zero value, submit process will abort.


Looks good to me - could you add some documentation please
(Documentation/git-p4.txt).

Thanks!
Luke


>
> Signed-off-by: Chen Bin 
> ---
>  git-p4.py   |  6 ++
>  t/t9800-git-p4-basic.sh | 22 ++
>  2 files changed, 28 insertions(+)
>
> diff --git a/git-p4.py b/git-p4.py
> index b449db1cc..69ee9ce41 100755
> --- a/git-p4.py
> +++ b/git-p4.py
> @@ -2303,6 +2303,12 @@ def run(self, args):
>  sys.exit("number of commits (%d) must match number of shelved 
> changelist (%d)" %
>   (len(commits), num_shelves))
>
> +# locate hook at `.git/hooks/pre-p4-submit`
> +hook_file = os.path.join(read_pipe("git rev-parse 
> --git-dir").strip(), "hooks", "pre-p4-submit")
> +# Execute hook. If it exit with non-zero value, do NOT continue.
> +if os.path.isfile(hook_file) and os.access(hook_file, os.X_OK) and 
> subprocess.call([hook_file]) != 0:
> +sys.exit(1)
> +
>  #
>  # Apply the commits, one at a time.  On failure, ask if should
>  # continue to try the rest of the patches, or quit.
> diff --git a/t/t9800-git-p4-basic.sh b/t/t9800-git-p4-basic.sh
> index 4849edc4e..48b768fa7 100755
> --- a/t/t9800-git-p4-basic.sh
> +++ b/t/t9800-git-p4-basic.sh
> @@ -261,6 +261,28 @@ test_expect_success 'unresolvable host in P4PORT should 
> display error' '
> )
>  '
>
> +# Test following scenarios:
> +#   - Without hook ".git/hooks/pre-p4-submit", submit should continue
> +#   - With hook returning 0, submit should continue
> +#   - With hook returning 1, submit should abort
> +test_expect_success 'run hook pre-p4-submit before submit' '
> +   test_when_finished cleanup_git &&
> +   git p4 clone --dest="$git" //depot &&
> +   (
> +   cd "$git" &&
> +   echo "hello world" >hello.txt &&
> +   git add hello.txt &&
> +   git commit -m "add hello.txt" &&
> +   git config git-p4.skipSubmitEdit true &&
> +   git p4 submit --dry-run | grep "Would apply" &&
> +   mkdir -p .git/hooks &&
> +   : >.git/hooks/pre-p4-submit && chmod +x 
> .git/hooks/pre-p4-submit &&
> +   git p4 submit --dry-run | grep "Would apply" &&
> +   echo #!/bin/sh && echo exit 1 >.git/hooks/pre-p4-submit &&
> +   git p4 submit --dry-run | grep "Would apply" || echo "Abort 
> submit"
> +   )
> +'
> +
>  test_expect_success 'submit from detached head' '
> test_when_finished cleanup_git &&
> git p4 clone --dest="$git" //depot &&
> --
> 2.18.0
>

Re: [PATCH 7/9] tests: include detailed trace logs with --write-junit-xml upon failure

[Luke Diamand]

On 4 September 2018 at 12:09, Johannes Schindelin
 wrote:
> Hi Eric,
>
> On Tue, 4 Sep 2018, Eric Sunshine wrote:
>
>> On Mon, Sep 3, 2018 at 5:10 PM Johannes Schindelin via GitGitGadget
>>  wrote:
>> > So let's do something different in VSTS: let's run all the tests with
>> > `--quiet` first, and only if a failure is encountered, try to trace the
>> > commands as they are executed. [...]

Is this re-running just an individual test on its own or all the tests
within a single file?

The latter shouldn't need this at all. And the former, I'm not sure
will actually work - most of the tests assume some particular p4
state. But perhaps I'm missing something?

I also think it does look kind of ugly. And if there's one thing I've
learned, it's that the ugly hack you write today with the words "we'll
tidy this up later" goes on to live with you forever!

>> >
>> > Signed-off-by: Johannes Schindelin 
>> > ---
>> > diff --git a/t/test-lib.sh b/t/test-lib.sh
>> > @@ -445,10 +452,37 @@ test_ok_ () {
>> >  test_failure_ () {
>> > if test -n "$write_junit_xml"
>> > then
>> > +   if test -z "$GIT_TEST_TEE_OUTPUT_FILE"
>> > +   then
>> > +   case "$(type kill_p4d 2>/dev/null | head -n 1)" in
>> > +   *function*) kill_p4d;;
>> > +   esac
>> > +
>> > +   case "$(type stop_git_daemon 2>/dev/null |
>> > +   head -n 1)" in
>> > +   *function*) stop_git_daemon;;
>> > +   esac
>>
>> In the long run, it might make more sense, and be more scalable, to
>> have those scripts define a "prepare_for_rerun" variable or function
>> which this code then runs generically rather than having special
>> knowledge of those facilities.
>>
>> I could imagine, for instance, test-lib.sh defining a no-op:
>>
>> test_failure_prepare_rerun () {}
>>
>> and then each of those scripts overriding the function:
>>
>> # in lib-git-p4.sh
>> test_failure_prepare_rerun () {
>> kill_p4d
>> }
>>
>> # in lib-git-daemon.sh
>> test_failure_prepare_rerun () {
>> stop_git_daemon
>> }
>
> Or we could implement `test_atexit` (similar to `test_when_finished`, but
> to be executed at `test_done` time). I guess that's what the p4 and daemon
> tests really needed to begin with (and probably also the apache2-using
> tests).
>
>>
>> > +   # re-run with --verbose-log
>> > +   echo "# Re-running: $junit_rerun_options_sq" >&2
>> > +
>> > +   cd "$TEST_DIRECTORY" &&
>> > +   eval "${TEST_SHELL_PATH}" 
>> > "$junit_rerun_options_sq" \
>> > +   >/dev/null 2>&1
>> > +   status=$?
>> > +
>> > +   say_color "" "$(test 0 = $status ||
>> > +   echo "not ")ok $test_count - (re-ran with 
>> > trace)"
>> > +   say "1..$test_count"
>> > +   GIT_EXIT_OK=t
>> > +   exit $status
>> > +   fi
>> > +
>> > junit_insert="> > junit_insert="$junit_insert $(xml_attr_encode "$1")\">"
>> > junit_insert="$junit_insert $(xml_attr_encode \
>> > -   "$(printf '%s\n' "$@" | sed 1d)")"
>> > +   "$(cat "$GIT_TEST_TEE_OUTPUT_FILE")")"
>> > +   >"$GIT_TEST_TEE_OUTPUT_FILE"
>> > junit_insert="$junit_insert"
>> > write_junit_xml_testcase "$1" "  $junit_insert"
>> > fi
>>
>> This junit-related stuff is getting pretty lengthy. I wonder if it
>> would make sense to pull it out to its own function at some point
>> (again, in the long run).
>
> Now that you mention it... I agree. This is getting long.
>
> In the short run, I have two things to consider, though: I want to make
> this work first, then think about introducing a layer of abstraction, and
> I want to go on vacation tomorrow.
>
> So I agree that this is something to be considered in the long run, i.e.
> not right now ;-)
>
> Thanks,
> Dscho

Re: [PATCH 7/9] tests: include detailed trace logs with --write-junit-xml upon failure

[Luke Diamand]

On 5 September 2018 at 13:39, Johannes Schindelin
 wrote:
> Hi Luke,
>
> On Wed, 5 Sep 2018, Luke Diamand wrote:
>
>> On 4 September 2018 at 12:09, Johannes Schindelin
>>  wrote:
>> >
>> > On Tue, 4 Sep 2018, Eric Sunshine wrote:
>> >
>> >> On Mon, Sep 3, 2018 at 5:10 PM Johannes Schindelin via GitGitGadget
>> >>  wrote:
>> >> > So let's do something different in VSTS: let's run all the tests with
>> >> > `--quiet` first, and only if a failure is encountered, try to trace the
>> >> > commands as they are executed. [...]
>>
>> Is this re-running just an individual test on its own or all the tests
>> within a single file?
>
> Upon encountering a failed test, it is re-running the entire test script
> afresh.
>
>> The latter shouldn't need this at all.
>
> Please do not let me die dumb. In other words, I would love for you to
> explain what exactly you mean by that sentence.

Just re-run the script. You shouldn't need to kill p4d, as each script
starts up its own instance of p4d, and shuts it down when it exits.

$ cd t
$ ./t9800-git-p4-basic.sh
Ctrl^C
$ ./t9800-git-p4-basic.sh -v

There's a cleanup() function in lib-git-p4.sh which kills the p4d
server, and that's invoked via:

  trap cleanup EXIT

That's the only cleanup that each of the scripts require AFAIK.

>
>> And the former, I'm not sure will actually work - most of the tests
>> assume some particular p4 state. But perhaps I'm missing something?
>
> No, the former would not work at all. Not only for the p4 tests: Git's
> tests frequently commit the deadly sin of relying on output of one another
> (wreaking havoc e.g. when test cases are skipped due to missing
> prerequisites, and latter test cases relying on their output). It is not
> the only thing that is wrong with the test suite, of course.
>
>> I also think it does look kind of ugly. And if there's one thing I've
>> learned, it's that the ugly hack you write today with the words "we'll
>> tidy this up later" goes on to live with you forever!
>
> Okay.
>
> (And having read lib-git-p4.sh, I kind of see where you learned that.)
>
> But maybe you also have some splendid idea what to do instead? Because
> doing something about it, that we need. We can't just say "oh, the only
> solution we found is ugly, so let's not do it at all".
>
> I am even going so far as to say: unless you have a better idea, it is
> pretty detrimental to criticize the current approach. It is the opposite
> of constructive.
>
> So let's hear some ideas how to improve the situation, m'kay?
>
> Just as a reminder, this is the problem I want to solve: I want to run the
> tests in a light-weight manner, with minimal output, and only in case of
> an error do I want to crank up the verbosity. Instead of wasting most of the
> effort to log everything and then throwing it away in most of the common
> cases, I suggest to re-run the entire test.
>
> What do you suggest to solve this?
>

I don't know about any other tests, but the git-p4 tests don't take
any longer in verbose mode. So one simple solution is to just run it
in verbose mode - unless there are other tests which have more
overhead.

The trap/exit/cleanup method that the git-p4 tests already use would
seem to be ideally suited to cleaning up everything on exit.

There might be some specific tests where this doesn't quite work out,
if you let me know what they are I can have a look at fixing them for
you.

Thanks,
Luke

Re: [PATCH 1/1] git-p4: add format-patch subcommand

[Luke Diamand]

On 26 February 2018 at 23:48, Eric Sunshine <sunsh...@sunshineco.com> wrote:
> On Mon, Feb 26, 2018 at 6:48 AM, Luke Diamand <l...@diamand.org> wrote:
>> It takes a list of P4 changelists and generates a patch for
>> each one, using "p4 describe".
>> [...]

Just to say that I almost got this working reasonably well, but it
gets pretty nasty making binary files work - in order to generate the
git binary format I need a base85 encoder which exists in Python3, but
not in Python2. And while I could obviously write it in Python easily
enough, it starts to feel like an awful lot of code implementing (and
maintaining) a slightly second-rate "git diff". And while it's
tempting to just not support binary files, in reality Perforce repos
seem to end up with lots of them.

So I think I might have another go at the previous scheme. What I will
have to do is to first create a faked-up commit representing the point
prior to the shelved commit for each file ("for file@rev-1 in files"),
and then create the real commit ("for file@rev in files"). It's
probably not as grim as it sounds and then means that we end up with
git doing all the hard work of diffing files, rather than relying on
Perforce's diff engine in conjunction with some Python.


>> Signed-off-by: Luke Diamand <l...@diamand.org>
>> ---
>> diff --git a/git-p4.py b/git-p4.py
>> @@ -3749,6 +3761,277 @@ class P4Branches(Command):
>> +class P4MakePatch(Command,P4UserMap):
>> +def run(self, args):
>> +if self.output and not os.path.isdir(self.output):
>> +sys.exit("output directory %s does not exist" % self.output)
>
> For consistency with "git format-patch", this could create the output
> directory automatically rather than erroring out.
>
>> +if self.strip_depot_prefix:
>> +self.clientSpec = getClientSpec()
>> +else:
>> +self.clientSpec = None
>> +
>> +self.loadUserMapFromCache()
>> +if len(args) < 1:
>> +return False
>
> Would it make sense to handle this no-arguments case earlier before
> doing work, such as loading the user map from cache?
>
>> +for change in args:
>> +self.make_patch(int(change))
>> +
>> +return True
>> +
>> +def p4_fetch_delta(self, change, files, shelved=False):
>> +cmd = ["describe"]
>> +if shelved:
>> +cmd += ["-S"]
>> +cmd += ["-du"]
>> +cmd += ["%s" % change]
>> +cmd = p4_build_cmd(cmd)
>> +
>> +p4 = subprocess.Popen(cmd, shell=False, stdout=subprocess.PIPE)
>> +try:
>> +result = p4.stdout.readlines()
>> +except EOFError:
>> +pass
>> +in_diff = False
>> +matcher = re.compile('\s+(.*)#(\d+)\s+\(text\)\s+')
>> +diffmatcher = re.compile("Differences ...")
>
> I'm not familiar with the output of "p4 describe", but does it include
> user-supplied text? If so, would it be safer to anchor 'diffmatcher',
> for instance, "^Diferences...$"?
>
>> +delta = ""
>> +skip_next_blank_line = False
>> +
>> +for line in result:
>> +if diffmatcher.match(line):
>
> Stepping back, does "Differences..." appear on a line of its own? If
> so, why does this need to be a regex at all? Can't it just do a direct
> string comparison?
>
>> +in_diff = True
>> +continue
>> +
>> +if in_diff:
>> +
>> +if skip_next_blank_line and \
>> +line.rstrip() == "":
>> +skip_next_blank_line = False
>> +continue
>> +
>> +m = matcher.match(line)
>> +if m:
>> +file = self.map_path(m.group(1))
>> +ver = m.group(2)
>> +delta += "diff --git a%s b%s\n" % (file, file)
>> +delta += "--- a%s\n" % file
>> +delta += "+++ b%s\n" % file
>> +skip_next_blank_line = True
>> +else:
>> +delta += line
>> +
>> +delta += "\n"
>> +
>> +exitCode = p4.wait()
>> +if exitCode != 0:
>> +raise IOError("p4 '%s' command failed" % str(cm

Re: [GSoC][PATCH] git-ci: use pylint to analyze the git-p4 code

[Luke Diamand]

On 13 March 2018 at 03:36, Viet Hung Tran  wrote:
> Hello Mr. Schneider,
>
> Here is my link for the passed CI build I tested on my fork:
> https://travis-ci.org/VietHTran/git/builds/35210
>
> In order to test .travis.yml configuration alone, I used a sample python
> script call "test.py" that is already successfully tested on my computer
> instead of using the git-p4.py like in the patch I submitted since the
> git-p4.py file still reports a lot of errors when running pylint.
>
> It is possible to fix all the git-p4.py errors. However, I think it would
> be best to fix each error separately with multiple commits (each should
> fix a specific problem such as indentication, variable naming, etc.)
> since there are a lot of changes needed to be made in the codes. Since the
> microproject requires that I submit one patch, I decided to submit a patch
> that includes changes in .travis.yml only. If you like, I can also submit the
> patches addressing changes on the git-p4.py that fix the pylint errors.

You can turn down the amount of messages it spews out, at which point
it actually becomes quite useful.

I had a quick go with the list of enabled checkers found here:

   https://github.com/ClusterHQ/flocker/blob/master/.pylintrc

and it then gives about 40 warnings, all of which look like they could
do with fixing.

I think fixing them in a few patches would be OK, rather than
submitting 40 separate patches.

Quite a lot of the warnings are about the use of "file" - I've got a
change I'm working on which fixes some of those already.

Luke


>
> Thank you for feedback,
>
> Viet
>
> Signed-off-by: Viet Hung Tran 
> ---
>  .travis.yml | 10 ++
>  1 file changed, 10 insertions(+)
>
> diff --git a/.travis.yml b/.travis.yml
> index 5f5ee4f3b..581d75319 100644
> --- a/.travis.yml
> +++ b/.travis.yml
> @@ -46,6 +46,16 @@ matrix:
>  - docker
>before_install:
>script: ci/run-linux32-docker.sh
> +- env: jobname=Pylint
> +  compiler:
> +  addons:
> +apt:
> +  packages:
> +  - python
> +  before_install:
> +  install: pip install --user pylint
> +  script: pylint git-p4.py
> +  after_failure:
>  - env: jobname=StaticAnalysis
>os: linux
>compiler:
> --
> 2.16.2.440.gc6284da
>

Re: [BUG] git p4 clone fails when p4 sizes does not return 'fileSize' key

[Luke Diamand]

On 17 April 2018 at 20:12, Thandesha VK  wrote:
> I have few cases where even p4 -G sizes (or p4 sizes) is not returning
> the size value even with latest version of p4 (17.2). In that case, we
> have to regenerate the digest for file save it - It mean something is
> wrong with the file in perforce.

Just to be clear - git-p4 does not use the p4 "sizes" command anywhere AFAIK.

We are just talking about the output from "p4 print" and the
"fileSize" key, right?

Does that happen with the 17.2 version of p4?

> Regarding, sys.stdout.write v/s print, I see script using both of them
> without a common pattern. I can change it to whatever is more
> appropriate.

print() probably makes more sense; can we try to use the function form
so that we don't deliberately make the path to python3 harder (albeit
in a very tiny way).

>
> On Tue, Apr 17, 2018 at 11:47 AM, Mazo, Andrey  wrote:
>> Does a missing "fileSize" actually mean that there is something wrong with 
>> the file?
>> Because, for me, `p4 -G print` doesn't print "fileSize" for _any_ file.
>> (which I attribute to our rather ancient (2007.2) Perforce server)
>> I'm not an expert in Perforce, so don't know for sure.

My 2015 version of p4d reports a fileSize.

>>
>> However, `p4 -G sizes` works fine even with our p4 server.
>> Should we then go one step further and use `p4 -G sizes` to obtain the 
>> "fileSize" when it's not returned by `p4 -G print`?
>> Or is it an overkill for a simple verbose print out?

If your server isn't reporting "fileSize" then there are a few other
places where I would expect git-p4 to also fail.

If we're going to support this very ancient version of p4d, then
gracefully handling a missing fileSize will be useful.

>>
>> Also, please, find one comment inline below.
>>
>> Thank you,
>> Andrey
>>
>> From: Thandesha VK 
>>> Sounds good. How about an enhanced version of fix from both of us.
>>> This will let us know that something is not right with the file but
>>> will not bark
>>>
>>> $ git diff
>>> diff --git a/git-p4.py b/git-p4.py
>>> index 7bb9cadc6..df901976f 100755
>>> --- a/git-p4.py
>>> +++ b/git-p4.py
>>> @@ -2566,7 +2566,12 @@ class P4Sync(Command, P4UserMap):
>>>  relPath = self.stripRepoPath(file['depotFile'], 
>>> self.branchPrefixes)
>>>  relPath = self.encodeWithUTF8(relPath)
>>>  if verbose:
>>> -size = int(self.stream_file['fileSize'])
>>> +if 'fileSize' not in self.stream_file:
>>> +   print "WARN: File size from perforce unknown. Please verify 
>>> by p4 sizes %s" %(file['depotFile'])
>> For whatever reason, the code below uses sys.stdout.write() instead of 
>> print().
>> Should it be used here for consistency as well?
>>
>>> +   size = "-1"
>>> +else:
>>> +   size = self.stream_file['fileSize']
>>> +size = int(size)
>>>  sys.stdout.write('\r%s --> %s (%i MB)\n' %
>>> (file['depotFile'], relPath, size/1024/1024))
>>>  sys.stdout.flush()
>>>
>>>
>>> On Tue, Apr 17, 2018 at 10:33 AM, Mazo, Andrey  wrote:
 Sure, I totally agree.
 Sorry, I just wasn't clear enough in my previous email.
 I meant that your patch suppresses "%s --> %s (%i MB)" line in case 
 "fileSize" is not available,
 while my patch suppresses just "(%i MB)" portion if the "fileSize" is not 
 known.
 In other words,
  * if "fileSize" is known:
  ** both yours and mine patches don't change existing behavior;
  * if "fileSize" is not known:
  ** your patch makes streamOneP4File() not print anything;
  ** my patch makes streamOneP4File() print "%s --> %s".

 Hope, I'm clearer this time.

 Thank you,
 Andrey

 From: Thandesha VK 
> *I* think keeping the filesize info is better with --verbose option as
> that gives some clue about the file we are working on. What do you
> think?
> Script has similar checks of key existence at other places where it is
> looking for fileSize.
>
> On Tue, Apr 17, 2018 at 9:22 AM, Andrey Mazo  wrote:
>> Huh, I actually have a slightly different fix for the same issue.
>> It doesn't suppress the corresponding verbose output completely, but 
>> just removes the size information from it.
>>
>> Also, I'd mention that the workaround is trivial -- simply omit the 
>> "--verbose" option.
>>
>> Andrey Mazo (1):
>>   git-p4: fix `sync --verbose` traceback due to 'fileSize'
>>
>>  git-p4.py | 8 ++--
>>  1 file changed, 6 insertions(+), 2 deletions(-)
>>

Thanks
Luke

[PATCH 0/1] git-p4: add format-patch subcommand

[Luke Diamand]

This is an initial attempt to add a "format-patch" command
to git-p4, following on from the earlier discussion about
shelving.

It uses the "p4 describe" command to generate the diff content and
post-processes it enough to generate git-style patches. These
can be fed to tools such as patch, or "git am".

This is useful for "unshelving" a P4 changelist into your git tree,
since the usual git subcommands (sync, clone) cannot easily read
a shelved changelist: there is no good way to get from Perforce
a consistent single revision against which to generate a diff
using git fast-import, since Perforce doesn't have the concept of
a repo revision.

By default, it leaves the depot prefix in the patch, but using
the option "--strip-depot-prefix" makes it suitable for "git am".

Use it like this:

 $ git p4 format-patch 12345 >out.patch

or
 $ mkdir patches
 $ git p4 format-patch --output patches 12345 12346

or
 $ git p4 format-patch --strip-depot-prefix 12347 >out.patch
 $ git am out.patch

Limitations of "p4 describe" mean that this will not work reliably
with binary files. There's no easy way around this. The change makes
a small attempt to at least stop on binary files, but in the case
of a file marked in P4 as "text", which contains binary deltas, the
file will unavoidably come out corrupted.

Luke Diamand (1):
  git-p4: add format-patch subcommand

 Documentation/git-p4.txt |  33 +
 git-p4.py| 304 +--
 t/t9832-make-patch.sh| 135 +
 3 files changed, 462 insertions(+), 10 deletions(-)
 create mode 100755 t/t9832-make-patch.sh

-- 
2.15.1.272.gc310869385

[PATCH 1/1] git-p4: add format-patch subcommand

[Luke Diamand]

It takes a list of P4 changelists and generates a patch for
each one, using "p4 describe".

This is especially useful for applying shelved changelists
to your git-p4 tree; the existing "git p4" subcommands do
not handle these.

That's because they "p4 print" the contents of each file at
a given revision, and then use git-fastimport to generate the
deltas. But in the case of a shelved changelist, there is no
easy way to find out what the previous file state was - Perforce
does not have the concept of a single repo-wide revision.

Unfortunately, using "p4 describe" comes with a price: it cannot
be used to reliably generate diffs for binary files (it tries to
linebreak on LF characters) and it is also _much_ slower.

Unicode character correctness is untested - in theory if
"p4 describe" knows about the character encoding it shouldn't
break unicode characters if they happen to contain LF, but I
haven't tested this.

Signed-off-by: Luke Diamand <l...@diamand.org>
---
 Documentation/git-p4.txt |  33 +
 git-p4.py| 304 +--
 t/t9832-make-patch.sh| 135 +
 3 files changed, 462 insertions(+), 10 deletions(-)
 create mode 100755 t/t9832-make-patch.sh

diff --git a/Documentation/git-p4.txt b/Documentation/git-p4.txt
index d8c8f11c9f..1908b00de2 100644
--- a/Documentation/git-p4.txt
+++ b/Documentation/git-p4.txt
@@ -164,6 +164,28 @@ $ git p4 submit --shelve
 $ git p4 submit --update-shelve 1234 --update-shelve 2345
 
 
+
+format-patch
+~~
+This will attempt to create a unified diff (using the git patch variant) which
+can be passed to patch. This is generated using the output from "p4 describe".
+
+It includes the contents of added files (which "p4 describe" does not).
+
+Binary files cannot be handled correctly due to limitations in "p4 describe".
+
+It will handle both normal and shelved (pending) changelists.
+
+Because of the way this works, it will be much slower than the normal git-p4 
clone
+path.
+
+
+$ git p4 format-patch 12345
+$ git p4 format-patch --output patchdir 12345 12346 12347
+$ git p4 format-patch --strip-depot-prefix 12348 > out.patch
+$ git am out.patch
+
+
 OPTIONS
 ---
 
@@ -337,6 +359,17 @@ These options can be used to modify 'git p4 rebase' 
behavior.
 --import-labels::
Import p4 labels.
 
+Format-patch options
+
+
+--output::
+Write patches to this directory (which must exist) instead of to
+standard output.
+
+--strip-depot-prefix::
+Strip the depot prefix from filenames in the patch.  This makes
+it suitable for passing to tools such as "git am".
+
 DEPOT PATH SYNTAX
 -
 The p4 depot path argument to 'git p4 sync' and 'git p4 clone' can
diff --git a/git-p4.py b/git-p4.py
index 7bb9cadc69..a1e998e6f5 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -26,6 +26,7 @@ import zipfile
 import zlib
 import ctypes
 import errno
+import time
 
 try:
 from subprocess import CalledProcessError
@@ -316,12 +317,17 @@ def p4_last_change():
 results = p4CmdList(["changes", "-m", "1"], skip_info=True)
 return int(results[0]['change'])
 
-def p4_describe(change):
+def p4_describe(change, shelved=False):
 """Make sure it returns a valid result by checking for
the presence of field "time".  Return a dict of the
results."""
 
-ds = p4CmdList(["describe", "-s", str(change)], skip_info=True)
+cmd = ["describe", "-s"]
+if shelved:
+cmd += ["-S"]
+cmd += [str(change)]
+
+ds = p4CmdList(cmd, skip_info=True)
 if len(ds) != 1:
 die("p4 describe -s %d did not return 1 result: %s" % (change, 
str(ds)))
 
@@ -372,7 +378,14 @@ def split_p4_type(p4type):
 mods = ""
 if len(s) > 1:
 mods = s[1]
-return (base, mods)
+
+git_mode = "100644"
+if "x" in mods:
+git_mode = "100755"
+if base == "symlink":
+git_mode = "12"
+
+return (base, mods, git_mode)
 
 #
 # return the raw p4 type of a file (text, text+ko, etc)
@@ -413,7 +426,7 @@ def p4_keywords_regexp_for_file(file):
 if not os.path.exists(file):
 return None
 else:
-(type_base, type_mods) = split_p4_type(p4_type(file))
+(type_base, type_mods, _) = split_p4_type(p4_type(file))
 return p4_keywords_regexp_for_type(type_base, type_mods)
 
 def setP4ExecBit(file, mode):
@@ -1208,6 +1221,9 @@ class P4UserMap:
 else:
 return True
 
+def getP4UsernameEmail(self, userid):
+return self.users[userid]
+
 def getUserCacheFilename(self):
 home = os.environ.get("HOME", os.environ.get("USERPROFILE"))
 r

Re: [PATCH] git-p4: Fix depot path stripping

[Luke Diamand]

On 27 February 2018 at 08:43, Nuno Subtil <sub...@gmail.com> wrote:
> The issue is that passing in --use-client-spec will always cause git-p4 to
> preserve the full depot path in the working tree that it creates, even when
> --keep-path is not used.
>
> The repro case is fairly simple: cloning a given p4 path that is nested 2 or
> more levels below the depot root will have different results with and
> without --use-client-spec (even when the client spec is just a
> straightforward map of the entire depot).
>
> E.g., 'git p4 clone //p4depot/path/to/some/project/...' will create a
> working tree rooted at project. However, 'git p4 clone --use-client-spec
> //p4depot/path/to/some/project/...' will create a working tree rooted at the
> root of the depot.

I think it _might_ be by design.

At least, the test ./t9809-git-p4-client-view.sh seems to fail for me
with your change, although I haven't investigated what's going on:

$ ./t9809-git-p4-client-view.sh
...
...
Doing initial import of //depot/dir1/ from revision #head into
refs/remotes/p4/master
./t9809-git-p4-client-view.sh: 10: eval: cannot create dir1/file13:
Directory nonexistent
not ok 23 - subdir clone, submit add

I think originally the logic came from this change:

   21ef5df43 git p4: make branch detection work with --use-client-spec

which was fixing what seems like the same problem but with branch
detection enabled.


>
> Thanks,
> Nuno
>
>
> On Tue, Feb 27, 2018 at 12:10 AM, Luke Diamand <l...@diamand.org> wrote:
>>
>> On 27 February 2018 at 04:16, Nuno Subtil <sub...@gmail.com> wrote:
>> > When useClientSpec=true, stripping of P4 depot paths doesn't happen
>> > correctly on sync. Modifies stripRepoPath to handle this case better.
>>
>> Can you give an example of how this shows up? I could quickly write a
>> test case for this if I knew what was going on.
>>
>> Thanks
>> Luke
>>
>>
>> >
>> > Signed-off-by: Nuno Subtil <sub...@gmail.com>
>> > ---
>> >  git-p4.py | 12 +---
>> >  1 file changed, 9 insertions(+), 3 deletions(-)
>> >
>> > diff --git a/git-p4.py b/git-p4.py
>> > index 7bb9cadc69738..3df95d0fd1d83 100755
>> > --- a/git-p4.py
>> > +++ b/git-p4.py
>> > @@ -2480,7 +2480,7 @@ def stripRepoPath(self, path, prefixes):
>> >  if path.startswith(b + "/"):
>> >  path = path[len(b)+1:]
>> >
>> > -elif self.keepRepoPath:
>> > +if self.keepRepoPath:
>> >  # Preserve everything in relative path name except leading
>> >  # //depot/; just look at first prefix as they all should
>> >  # be in the same depot.
>> > @@ -2490,6 +2490,12 @@ def stripRepoPath(self, path, prefixes):
>> >
>> >  else:
>> >  for p in prefixes:
>> > +   if self.useClientSpec and not self.keepRepoPath:
>> > +# when useClientSpec is false, the prefix will
>> > contain the depot name but the path will not
>> > +# extract the depot name and add it to the path so
>> > the match below will do the right thing
>> > +depot = re.sub("^(//[^/]+/).*", r'\1', p)
>> > +path = depot + path
>> > +
>> >  if p4PathStartsWith(path, p):
>> >  path = path[len(p):]
>> >  break
>> > @@ -2526,8 +2532,8 @@ def splitFilesIntoBranches(self, commit):
>> >  # go in a p4 client
>> >  if self.useClientSpec:
>> >  relPath = self.clientSpecDirs.map_in_client(path)
>> > -else:
>> > -relPath = self.stripRepoPath(path, self.depotPaths)
>> > +
>> > +relPath = self.stripRepoPath(path, self.depotPaths)
>> >
>> >  for branch in self.knownBranches.keys():
>> >  # add a trailing slash so that a commit into qt/4.2foo
>> >
>> > --
>> > https://github.com/git/git/pull/463
>
>

Re: What's cooking in git.git (Mar 2018, #01; Thu, 1)

[Luke Diamand]

On 1 March 2018 at 22:20, Junio C Hamano  wrote:
> Here are the topics that have been cooking.  Commits prefixed with
> '-' are only in 'pu' (proposed updates) while commits prefixed with
> '+' are in 'next'.  The ones marked with '.' do not appear in any of
> the integration branches, but I am still holding onto them.
>
> You can find the changes described here in the integration branches
> of the repositories listed at
>
> http://git-blame.blogspot.com/p/git-public-repositories.html
>
> --
> --
> [New Topics]
>
>
> * ld/p4-unshelve (2018-02-22) 1 commit
>  - git-p4: add unshelve command
>
>  "git p4" learned to "unshelve" shelved commit from P4.
>
>  Will merge to 'next'.

The unshelve change should be left off next for now.

The problem with it is that it can't easily find a sensible consistent
point prior to the shelved changelist to generate the diff from (P4
has no concept of a tree revision). So you can end up "unshelving" and
pickup not only the shelved changelist, but also a bunch of
intervening changes (or the effect of some missing changelists). That
can be quite surprising.

This is actually pretty close to the behaviour of P4 unshelve itself,
which does somewhat the same thing. From the p4 manual page:

https://www.perforce.com/perforce/doc.current/manuals/cmdref/Content/CmdRef/p4_unshelve.html

   " Unshelving copies the shelved files into the user’s workspace as
they existed when they were shelved. (For example, a file open for
edit when shelved will also be open for edit in the unshelving user’s
workspace.)"

There's a better change which I posted which adds a "git p4
format-change" command which uses the diffs from Perforce. I think
that has a better chance of working properly. I had some review
comments which I need to take, after which it could be a candidate for
next.

It _might_ though be possible to resurrect the unshelve code by doing
something like extracting the previous versions  of the files (which
is kind of doable) and then constructing a temporary branch in git to
do the comparison against. Sounds pretty nasty though.

Thanks
Luke

Re: [PATCH] git-p4: Fix depot path stripping

[Luke Diamand]

On 27 February 2018 at 19:00, Nuno Subtil <sub...@gmail.com> wrote:
> I originally thought this had been designed such that the p4 client spec
> determines the paths, which would make sense. However, git-p4 still ignores
> the local path mappings in the client spec when syncing p4 depot paths
> w/--use-client-spec. Effectively, it looks as though --use-client-spec
> always implies --keep-paths, which didn't seem to me like what was intended.
>
> For the use case I'm dealing with (syncing a p4 path but ignoring some
> directories inside it), there seems to be no way today to generate a git
> tree rooted at the p4 path location instead of the root of the depot, which
> looks like a bug to me. I don't really understand the overall design well
> enough to tell whether the bug lies in stripRepoPath or somewhere else, to
> be honest, but that's where I saw the inconsistency manifest itself.

I replied but managed to drop git-users off the thread. So trying again!

The behavior is a touch surprising, but I _think_ it's correct.

With --use-client-spec enabled, paths in the git repot get mapped as
if you had used the file mapping in the client spec, using "p4 where".

So, for example, if you have a client spec which looks like:
   //depot/... //my_client_spec/...

then you're going to get the full repo structure, even if you only
clone a subdirectory.

e.g. if you clone //depot/a/b/c then with "--use-client-spec" enabled,
you will get a/b/c in your git repo, and with it not enabled, you will
just get c/.

If instead your client spec looks like:
  //depot/a/b/... //my_client_spec/...

then you should only get c/d. (And a quick test confirms that).

I think Nuno's original question comes from wanting to map some files
into place which the clientspec mapping emulation in git-p4 was
possibly not handling - if we can get a test case for that (or an
example) then we can see if it's just that the client mapping code
that Pete put in is insufficient, or if there's some other way around.
Or if indeed I'm wrong, and there's a bug! There may be some weird
corner-cases where it might do the wrong thing.

Thanks,
Luke


>
> Nuno
>
>
> On Tue, Feb 27, 2018 at 3:12 AM, Luke Diamand <l...@diamand.org> wrote:
>>
>> On 27 February 2018 at 08:43, Nuno Subtil <sub...@gmail.com> wrote:
>> > The issue is that passing in --use-client-spec will always cause git-p4
>> > to
>> > preserve the full depot path in the working tree that it creates, even
>> > when
>> > --keep-path is not used.
>> >
>> > The repro case is fairly simple: cloning a given p4 path that is nested
>> > 2 or
>> > more levels below the depot root will have different results with and
>> > without --use-client-spec (even when the client spec is just a
>> > straightforward map of the entire depot).
>> >
>> > E.g., 'git p4 clone //p4depot/path/to/some/project/...' will create a
>> > working tree rooted at project. However, 'git p4 clone --use-client-spec
>> > //p4depot/path/to/some/project/...' will create a working tree rooted at
>> > the
>> > root of the depot.
>>
>> I think it _might_ be by design.
>>
>> At least, the test ./t9809-git-p4-client-view.sh seems to fail for me
>> with your change, although I haven't investigated what's going on:
>>
>> $ ./t9809-git-p4-client-view.sh
>> ...
>> ...
>> Doing initial import of //depot/dir1/ from revision #head into
>> refs/remotes/p4/master
>> ./t9809-git-p4-client-view.sh: 10: eval: cannot create dir1/file13:
>> Directory nonexistent
>> not ok 23 - subdir clone, submit add
>>
>> I think originally the logic came from this change:
>>
>>21ef5df43 git p4: make branch detection work with --use-client-spec
>>
>> which was fixing what seems like the same problem but with branch
>> detection enabled.
>>
>>
>> >
>> > Thanks,
>> > Nuno
>> >
>> >
>> > On Tue, Feb 27, 2018 at 12:10 AM, Luke Diamand <l...@diamand.org> wrote:
>> >>
>> >> On 27 February 2018 at 04:16, Nuno Subtil <sub...@gmail.com> wrote:
>> >> > When useClientSpec=true, stripping of P4 depot paths doesn't happen
>> >> > correctly on sync. Modifies stripRepoPath to handle this case better.
>> >>
>> >> Can you give an example of how this shows up? I could quickly write a
>> >> test case for this if I knew what was going on.
>> >>
>> >> Thanks
>> >> Luke
>> >>
>> >>
>> >> >
>> >> > Signed-off-by: Nuno Subtil <sub...@gmail.com>
>> >&

Re: [PATCHv1 2/3] git-p4: unshelve into refs/remotes/p4-unshelved, not refs/remotes/p4/unshelved

[Luke Diamand]

On Fri, 12 Oct 2018 at 14:45, Junio C Hamano  wrote:
>
> Luke Diamand  writes:
>
> > The branch detection code looks for branches under refs/remotes/p4/...
> > and can end up getting confused if there are unshelved changes in
> > there as well. This happens in the function p4BranchesInGit().
> >
> > Instead, put the unshelved changes into refs/remotes/p4-unshelved/.
>
> I am not a p4 user (and not a git-p4 user), so it is a bit hard for
> me to assess if this is a backward incompatibile change and if so
> how serious potential breakage to existing users would be.

I don't think it's a particularly serious breakage - it reports the
branch it unshelves to, so it should be fairly obvious.

However, maybe it would make sense to pull this into a separate commit
to make it more obvious? I should have thought of that before
submitting.

>
> >
> > -If the target branch in refs/remotes/p4/unshelved already exists, the old 
> > one will
> > +If the target branch in refs/remotes/p4-unshelved already exists, the old 
> > one will
> >  be renamed.
> >
> >  
> >  $ git p4 sync
> >  $ git p4 unshelve 12345
> > -$ git show refs/remotes/p4/unshelved/12345
> > +$ git show p4/unshelved/12345
>
> Isn't this "p4-unshelved/12345" now?

Yes, I think another reason to pull into a separate commit.

Luke

>

Re: [PATCH v2 09/13] git-p4: use `test_atexit` to kill the daemon

[Luke Diamand]

On Mon, 15 Oct 2018 at 16:02, Johannes Schindelin
 wrote:
>
> Hi Luke,
>
> On Mon, 15 Oct 2018, Luke Diamand wrote:
>
> > On Mon, 15 Oct 2018 at 11:12, Johannes Schindelin via GitGitGadget
> >  wrote:
> > >
> > > From: Johannes Schindelin 
> > >
> > > This should be more reliable than the current method, and prepares the
> > > test suite for a consistent way to clean up before re-running the tests
> > > with different options.
> > >
> >
> > I'm finding that it's leaving p4d processes lying around.
>
> That's a bummer!
>
> > e.g.
> >
> > $ ./t9820-git-p4-editor-handling.sh
> > 
> > $ ./t9820-git-p4-editor-handling.sh
> > 
>
> Since I do not currently have a setup with p4d installed, can you run that
> with `sh -x ...` and see whether this code path is hit?

All you need to do is to put p4 and p4d in your PATH.

https://www.perforce.com/downloads/helix-core-p4d
https://www.perforce.com/downloads/helix-command-line-client-p4

The server is free to use for a small number of users, you don't need
to do anything to make it go.


>
>  test_done () {
> GIT_EXIT_OK=t
>
> +   test -n "$immediate" || test_atexit_handler
> +

+ test -n
+ test_atexit_handler
./t9820-git-p4-editor-handling.sh: 764:
./t9820-git-p4-editor-handling.sh: test_atexit_handler: not found

Is that expected?




> if test -n "$write_junit_xml" && test -n "$junit_xml_path"
> then
>
> > And also
> >
> > $ ./t9800-git-p4-basic.sh
> > 
> > Ctrl-C
>
> Oh, you're right. I think I need to do something in this line:
>
> trap 'exit $?' INT
>
> in t/test-lib.sh, something like
>
> trap 'exit_code=$?; test_atexit_handler; exit $exit_code' INT
>
> would you agree? (And: could you test?)

Not sure.
Send me a patch and I can try it out.

Thanks,
Luke

Re: [PATCHv1 2/3] git-p4: unshelve into refs/remotes/p4-unshelved, not refs/remotes/p4/unshelved

[Luke Diamand]

On Fri, 12 Oct 2018 at 19:19, Luke Diamand  wrote:
>
> On Fri, 12 Oct 2018 at 14:45, Junio C Hamano  wrote:
> >
> > Luke Diamand  writes:
> >
> > > The branch detection code looks for branches under refs/remotes/p4/...
> > > and can end up getting confused if there are unshelved changes in
> > > there as well. This happens in the function p4BranchesInGit().
> > >
> > > Instead, put the unshelved changes into refs/remotes/p4-unshelved/.
> >
> > I am not a p4 user (and not a git-p4 user), so it is a bit hard for
> > me to assess if this is a backward incompatibile change and if so
> > how serious potential breakage to existing users would be.
>
> I don't think it's a particularly serious breakage - it reports the
> branch it unshelves to, so it should be fairly obvious.
>
> However, maybe it would make sense to pull this into a separate commit
> to make it more obvious? I should have thought of that before
> submitting.
>
> >
> > >
> > > -If the target branch in refs/remotes/p4/unshelved already exists, the 
> > > old one will
> > > +If the target branch in refs/remotes/p4-unshelved already exists, the 
> > > old one will
> > >  be renamed.
> > >
> > >  
> > >  $ git p4 sync
> > >  $ git p4 unshelve 12345
> > > -$ git show refs/remotes/p4/unshelved/12345
> > > +$ git show p4/unshelved/12345
> >
> > Isn't this "p4-unshelved/12345" now?
>
> Yes, I think another reason to pull into a separate commit.

D'oh. It's already in a separate commit.
I'll re-roll fixing that documentation.

I think it will be fine to change the branch that the unshelving
happens into - I think it's very unlikely anyone is basing some
automated scripts on this, because of the way that unshelving is used
anyway.

Luke

Re: [PATCH v2 09/13] git-p4: use `test_atexit` to kill the daemon

[Luke Diamand]

On Mon, 15 Oct 2018 at 11:12, Johannes Schindelin via GitGitGadget
 wrote:
>
> From: Johannes Schindelin 
>
> This should be more reliable than the current method, and prepares the
> test suite for a consistent way to clean up before re-running the tests
> with different options.
>

I'm finding that it's leaving p4d processes lying around.

e.g.

$ ./t9820-git-p4-editor-handling.sh

$ ./t9820-git-p4-editor-handling.sh


And also

$ ./t9800-git-p4-basic.sh

Ctrl-C

$ ./t9800-git-p4-basic.sh


$ ps | grep p4d
21392 pts/100:00:00 p4d  <


Luke

[PATCHv2 0/3] git-p4: improved unshelving - small fixes

[Luke Diamand]

This is the same as my earlier patch other than fixing the
documentation to reflect the change to the destination branch,
as noticed by Junio.

Luke Diamand (3):
  git-p4: do not fail in verbose mode for missing 'fileSize' key
  git-p4: unshelve into refs/remotes/p4-unshelved, not
refs/remotes/p4/unshelved
  git-p4: fully support unshelving changelists

 Documentation/git-p4.txt | 10 ++---
 git-p4.py| 90 +++-
 t/t9832-unshelve.sh  | 75 ++---
 3 files changed, 117 insertions(+), 58 deletions(-)

-- 
2.19.1.331.gae0ed827e6

[PATCHv2 3/3] git-p4: fully support unshelving changelists

[Luke Diamand]

The previous git-p4 unshelve support would check for changes
in Perforce to the files being unshelved since the original
shelve, and would complain if any were found.

This was to ensure that the user wouldn't end up with both the
shelved change delta, and some deltas from other changes in their
git commit.

e.g. given fileA:
  the
  quick
  brown
  fox

  change1: s/the/The/   <- p4 shelve this change
  change2: s/fox/Fox/   <- p4 submit this change
  git p4 unshelve 1 <- FAIL

This change teaches the P4Unshelve class to always create a parent
commit which matches the P4 tree (for the files being unshelved) at
the point prior to the P4 shelve being created (which is reported
in the p4 description for a shelved changelist).

That then means git-p4 can always create a git commit matching the
P4 shelve that was originally created, without any extra deltas.

The user might still need to use the --origin option though - there
is no way for git-p4 to work out the versions of all of the other
*unchanged* files in the shelve, since this information is not recorded
by Perforce.

Additionally this fixes handling of shelved 'move' operations.

Signed-off-by: Luke Diamand 
---
 Documentation/git-p4.txt |  4 +-
 git-p4.py| 84 +++-
 t/t9832-unshelve.sh  | 69 ++---
 3 files changed, 106 insertions(+), 51 deletions(-)

diff --git a/Documentation/git-p4.txt b/Documentation/git-p4.txt
index 6c0017e36e..f0a0280954 100644
--- a/Documentation/git-p4.txt
+++ b/Documentation/git-p4.txt
@@ -177,8 +177,8 @@ Unshelving will take a shelved P4 changelist, and produce 
the equivalent git com
 in the branch refs/remotes/p4-unshelved/.
 
 The git commit is created relative to the current origin revision (HEAD by 
default).
-If the shelved changelist's parent revisions differ, git-p4 will refuse to 
unshelve;
-you need to be unshelving onto an equivalent tree.
+A parent commit is created based on the origin, and then the unshelve commit is
+created based on that.
 
 The origin revision can be changed with the "--origin" option.
 
diff --git a/git-p4.py b/git-p4.py
index 76c18a22e9..1998c3e141 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -1306,6 +1306,9 @@ def processContent(self, git_mode, relPath, contents):
 return LargeFileSystem.processContent(self, git_mode, relPath, 
contents)
 
 class Command:
+delete_actions = ( "delete", "move/delete", "purge" )
+add_actions = ( "add", "move/add" )
+
 def __init__(self):
 self.usage = "usage: %prog [options]"
 self.needsGit = True
@@ -2524,7 +2527,6 @@ def map_in_client(self, depot_path):
 return ""
 
 class P4Sync(Command, P4UserMap):
-delete_actions = ( "delete", "move/delete", "purge" )
 
 def __init__(self):
 Command.__init__(self)
@@ -2612,20 +2614,7 @@ def checkpoint(self):
 if self.verbose:
 print("checkpoint finished: " + out)
 
-def cmp_shelved(self, path, filerev, revision):
-""" Determine if a path at revision #filerev is the same as the file
-at revision @revision for a shelved changelist. If they don't 
match,
-unshelving won't be safe (we will get other changes mixed in).
-
-This is comparing the revision that the shelved changelist is 
*based* on, not
-the shelved changelist itself.
-"""
-ret = p4Cmd(["diff2", "{0}#{1}".format(path, filerev), 
"{0}@{1}".format(path, revision)])
-if verbose:
-print("p4 diff2 path %s filerev %s revision %s => %s" % (path, 
filerev, revision, ret))
-return ret["status"] == "identical"
-
-def extractFilesFromCommit(self, commit, shelved=False, shelved_cl = 0, 
origin_revision = 0):
+def extractFilesFromCommit(self, commit, shelved=False, shelved_cl = 0):
 self.cloneExclude = [re.sub(r"\.\.\.$", "", path)
  for path in self.cloneExclude]
 files = []
@@ -2650,17 +2639,6 @@ def extractFilesFromCommit(self, commit, shelved=False, 
shelved_cl = 0, origin_r
 file["type"] = commit["type%s" % fnum]
 if shelved:
 file["shelved_cl"] = int(shelved_cl)
-
-# For shelved changelists, check that the revision of each 
file that the
-# shelve was based on matches the revision that we are using 
for the
-# starting point for git-fast-import (self.initialParent). 
Otherwise
-# the resulting diff will contain deltas from multiple commits.
-
-if file["action"] != "add" and \
-not self.cmp_shelved(path, file[&

[PATCHv2 1/3] git-p4: do not fail in verbose mode for missing 'fileSize' key

[Luke Diamand]

If deleting or moving a file, sometimes P4 doesn't report the file size.

The code handles this just fine but some logging crashes. Stop this
happening.

There was some earlier discussion on the list about this:

https://public-inbox.org/git/xmqq1sqpp1vv@gitster.mtv.corp.google.com/

Signed-off-by: Luke Diamand 
---
 git-p4.py | 5 -
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/git-p4.py b/git-p4.py
index 7fab255584..5701bad06a 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -2775,7 +2775,10 @@ def streamOneP4File(self, file, contents):
 relPath = self.stripRepoPath(file['depotFile'], self.branchPrefixes)
 relPath = self.encodeWithUTF8(relPath)
 if verbose:
-size = int(self.stream_file['fileSize'])
+if 'fileSize' in self.stream_file:
+size = int(self.stream_file['fileSize'])
+else:
+size = 0 # deleted files don't get a fileSize apparently
 sys.stdout.write('\r%s --> %s (%i MB)\n' % (file['depotFile'], 
relPath, size/1024/1024))
 sys.stdout.flush()
 
-- 
2.19.1.331.gae0ed827e6

[PATCHv2 2/3] git-p4: unshelve into refs/remotes/p4-unshelved, not refs/remotes/p4/unshelved

[Luke Diamand]

The branch detection code looks for branches under refs/remotes/p4/...
and can end up getting confused if there are unshelved changes in
there as well. This happens in the function p4BranchesInGit().

Instead, put the unshelved changes into refs/remotes/p4-unshelved/.

Signed-off-by: Luke Diamand 
---
 Documentation/git-p4.txt | 6 +++---
 git-p4.py| 3 ++-
 t/t9832-unshelve.sh  | 6 +++---
 3 files changed, 8 insertions(+), 7 deletions(-)

diff --git a/Documentation/git-p4.txt b/Documentation/git-p4.txt
index 41780a5aa9..6c0017e36e 100644
--- a/Documentation/git-p4.txt
+++ b/Documentation/git-p4.txt
@@ -174,7 +174,7 @@ $ git p4 submit --update-shelve 1234 --update-shelve 2345
 Unshelve
 
 Unshelving will take a shelved P4 changelist, and produce the equivalent git 
commit
-in the branch refs/remotes/p4/unshelved/.
+in the branch refs/remotes/p4-unshelved/.
 
 The git commit is created relative to the current origin revision (HEAD by 
default).
 If the shelved changelist's parent revisions differ, git-p4 will refuse to 
unshelve;
@@ -182,13 +182,13 @@ you need to be unshelving onto an equivalent tree.
 
 The origin revision can be changed with the "--origin" option.
 
-If the target branch in refs/remotes/p4/unshelved already exists, the old one 
will
+If the target branch in refs/remotes/p4-unshelved already exists, the old one 
will
 be renamed.
 
 
 $ git p4 sync
 $ git p4 unshelve 12345
-$ git show refs/remotes/p4/unshelved/12345
+$ git show p4-unshelved/12345
 
 $ git p4 unshelve 12345
 
diff --git a/git-p4.py b/git-p4.py
index 5701bad06a..76c18a22e9 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -3956,7 +3956,8 @@ def __init__(self):
 ]
 self.verbose = False
 self.noCommit = False
-self.destbranch = "refs/remotes/p4/unshelved"
+self.destbranch = "refs/remotes/p4-unshelved"
+self.origin = "p4/master"
 
 def renameBranch(self, branch_name):
 """ Rename the existing branch to branch_name.N
diff --git a/t/t9832-unshelve.sh b/t/t9832-unshelve.sh
index 48ec7679b8..c3d15ceea8 100755
--- a/t/t9832-unshelve.sh
+++ b/t/t9832-unshelve.sh
@@ -54,8 +54,8 @@ EOF
cd "$git" &&
change=$(last_shelved_change) &&
git p4 unshelve $change &&
-   git show refs/remotes/p4/unshelved/$change | grep -q "Further 
description" &&
-   git cherry-pick refs/remotes/p4/unshelved/$change &&
+   git show refs/remotes/p4-unshelved/$change | grep -q "Further 
description" &&
+   git cherry-pick refs/remotes/p4-unshelved/$change &&
test_path_is_file file2 &&
test_cmp file1 "$cli"/file1 &&
test_cmp file2 "$cli"/file2 &&
@@ -88,7 +88,7 @@ EOF
cd "$git" &&
change=$(last_shelved_change) &&
git p4 unshelve $change &&
-   git diff refs/remotes/p4/unshelved/$change.0 
refs/remotes/p4/unshelved/$change | grep -q file3
+   git diff refs/remotes/p4-unshelved/$change.0 
refs/remotes/p4-unshelved/$change | grep -q file3
)
 '
 
-- 
2.19.1.331.gae0ed827e6

[PATCHv1 3/3] git-p4: fully support unshelving changelists

[Luke Diamand]

The previous git-p4 unshelve support would check for changes
in Perforce to the files being unshelved since the original
shelve, and would complain if any were found.

This was to ensure that the user wouldn't end up with both the
shelved change delta, and some deltas from other changes in their
git commit.

e.g. given fileA:
  the
  quick
  brown
  fox

  change1: s/the/The/   <- p4 shelve this change
  change2: s/fox/Fox/   <- p4 submit this change
  git p4 unshelve 1 <- FAIL

This change teaches the P4Unshelve class to always create a parent
commit which matches the P4 tree (for the files being unshelved) at
the point prior to the P4 shelve being created (which is reported
in the p4 description for a shelved changelist).

That then means git-p4 can always create a git commit matching the
P4 shelve that was originally created, without any extra deltas.

The user might still need to use the --origin option though - there
is no way for git-p4 to work out the versions of all of the other
*unchanged* files in the shelve, since this information is not recorded
by Perforce.

Additionally this fixes handling of shelved 'move' operations.

Signed-off-by: Luke Diamand 
---
 Documentation/git-p4.txt |  4 +-
 git-p4.py| 84 +++-
 t/t9832-unshelve.sh  | 69 ++---
 3 files changed, 106 insertions(+), 51 deletions(-)

diff --git a/Documentation/git-p4.txt b/Documentation/git-p4.txt
index c7705ae6e7..d8332e99c1 100644
--- a/Documentation/git-p4.txt
+++ b/Documentation/git-p4.txt
@@ -177,8 +177,8 @@ Unshelving will take a shelved P4 changelist, and produce 
the equivalent git com
 in the branch refs/remotes/p4-unshelved/.
 
 The git commit is created relative to the current origin revision (HEAD by 
default).
-If the shelved changelist's parent revisions differ, git-p4 will refuse to 
unshelve;
-you need to be unshelving onto an equivalent tree.
+A parent commit is created based on the origin, and then the unshelve commit is
+created based on that.
 
 The origin revision can be changed with the "--origin" option.
 
diff --git a/git-p4.py b/git-p4.py
index 76c18a22e9..1998c3e141 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -1306,6 +1306,9 @@ def processContent(self, git_mode, relPath, contents):
 return LargeFileSystem.processContent(self, git_mode, relPath, 
contents)
 
 class Command:
+delete_actions = ( "delete", "move/delete", "purge" )
+add_actions = ( "add", "move/add" )
+
 def __init__(self):
 self.usage = "usage: %prog [options]"
 self.needsGit = True
@@ -2524,7 +2527,6 @@ def map_in_client(self, depot_path):
 return ""
 
 class P4Sync(Command, P4UserMap):
-delete_actions = ( "delete", "move/delete", "purge" )
 
 def __init__(self):
 Command.__init__(self)
@@ -2612,20 +2614,7 @@ def checkpoint(self):
 if self.verbose:
 print("checkpoint finished: " + out)
 
-def cmp_shelved(self, path, filerev, revision):
-""" Determine if a path at revision #filerev is the same as the file
-at revision @revision for a shelved changelist. If they don't 
match,
-unshelving won't be safe (we will get other changes mixed in).
-
-This is comparing the revision that the shelved changelist is 
*based* on, not
-the shelved changelist itself.
-"""
-ret = p4Cmd(["diff2", "{0}#{1}".format(path, filerev), 
"{0}@{1}".format(path, revision)])
-if verbose:
-print("p4 diff2 path %s filerev %s revision %s => %s" % (path, 
filerev, revision, ret))
-return ret["status"] == "identical"
-
-def extractFilesFromCommit(self, commit, shelved=False, shelved_cl = 0, 
origin_revision = 0):
+def extractFilesFromCommit(self, commit, shelved=False, shelved_cl = 0):
 self.cloneExclude = [re.sub(r"\.\.\.$", "", path)
  for path in self.cloneExclude]
 files = []
@@ -2650,17 +2639,6 @@ def extractFilesFromCommit(self, commit, shelved=False, 
shelved_cl = 0, origin_r
 file["type"] = commit["type%s" % fnum]
 if shelved:
 file["shelved_cl"] = int(shelved_cl)
-
-# For shelved changelists, check that the revision of each 
file that the
-# shelve was based on matches the revision that we are using 
for the
-# starting point for git-fast-import (self.initialParent). 
Otherwise
-# the resulting diff will contain deltas from multiple commits.
-
-if file["action"] != "add" and \
-not self.cmp_shelved(path, file[&

[PATCHv1 0/3] git-p4: improved unshelving

[Luke Diamand]

This patch series teaches the git-p4 unshelve command to handle
intervening changes to the Perforce files.

At the moment if you try to unshelve a file, and that file has been
modified since the shelving, git-p4 refuses. That is so that it
doesn't end up generating a commit containing deltas from several P4
changes.

This gets to be more annoying as time goes on and the files you are
interested in get updated by other people.

However, what we can do is to create a parent commit matching the
state of the tree when the shelve happened, which then lets git-p4
create a git commit containing just the changes that are wanted.

It's still impossible to determine the true state of the complete
tree when the P4 shelve was created, since this information is not
recorded by Perforce. Manual intervention is required to fix that.

There are also a few other smaller fixes, the main one being
that it no longer unshelves into refs/remotes/p4/master/unshelved, but
instead into refs/remotes/p4-unshelved.

That's because the git-p4 branch detection gets confused by branches
appearing in refs/remotes/p4.


Luke Diamand (3):
  git-p4: do not fail in verbose mode for missing 'fileSize' key
  git-p4: unshelve into refs/remotes/p4-unshelved, not
refs/remotes/p4/unshelved
  git-p4: fully support unshelving changelists

 Documentation/git-p4.txt | 10 ++---
 git-p4.py| 90 +++-
 t/t9832-unshelve.sh  | 75 ++---
 3 files changed, 117 insertions(+), 58 deletions(-)

-- 
2.19.1.272.gf84b9b09d4

[PATCHv1 1/3] git-p4: do not fail in verbose mode for missing 'fileSize' key

[Luke Diamand]

If deleting or moving a file, sometimes P4 doesn't report the file size.

The code handles this just fine but some logging crashes. Stop this
happening.

There was some earlier discussion on the list about this:

https://public-inbox.org/git/xmqq1sqpp1vv@gitster.mtv.corp.google.com/

Signed-off-by: Luke Diamand 
---
 git-p4.py | 5 -
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/git-p4.py b/git-p4.py
index 7fab255584..5701bad06a 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -2775,7 +2775,10 @@ def streamOneP4File(self, file, contents):
 relPath = self.stripRepoPath(file['depotFile'], self.branchPrefixes)
 relPath = self.encodeWithUTF8(relPath)
 if verbose:
-size = int(self.stream_file['fileSize'])
+if 'fileSize' in self.stream_file:
+size = int(self.stream_file['fileSize'])
+else:
+size = 0 # deleted files don't get a fileSize apparently
 sys.stdout.write('\r%s --> %s (%i MB)\n' % (file['depotFile'], 
relPath, size/1024/1024))
 sys.stdout.flush()
 
-- 
2.19.1.272.gf84b9b09d4

[PATCHv1 2/3] git-p4: unshelve into refs/remotes/p4-unshelved, not refs/remotes/p4/unshelved

[Luke Diamand]

The branch detection code looks for branches under refs/remotes/p4/...
and can end up getting confused if there are unshelved changes in
there as well. This happens in the function p4BranchesInGit().

Instead, put the unshelved changes into refs/remotes/p4-unshelved/.

Signed-off-by: Luke Diamand 
---
 Documentation/git-p4.txt | 6 +++---
 git-p4.py| 3 ++-
 t/t9832-unshelve.sh  | 6 +++---
 3 files changed, 8 insertions(+), 7 deletions(-)

diff --git a/Documentation/git-p4.txt b/Documentation/git-p4.txt
index 41780a5aa9..c7705ae6e7 100644
--- a/Documentation/git-p4.txt
+++ b/Documentation/git-p4.txt
@@ -174,7 +174,7 @@ $ git p4 submit --update-shelve 1234 --update-shelve 2345
 Unshelve
 
 Unshelving will take a shelved P4 changelist, and produce the equivalent git 
commit
-in the branch refs/remotes/p4/unshelved/.
+in the branch refs/remotes/p4-unshelved/.
 
 The git commit is created relative to the current origin revision (HEAD by 
default).
 If the shelved changelist's parent revisions differ, git-p4 will refuse to 
unshelve;
@@ -182,13 +182,13 @@ you need to be unshelving onto an equivalent tree.
 
 The origin revision can be changed with the "--origin" option.
 
-If the target branch in refs/remotes/p4/unshelved already exists, the old one 
will
+If the target branch in refs/remotes/p4-unshelved already exists, the old one 
will
 be renamed.
 
 
 $ git p4 sync
 $ git p4 unshelve 12345
-$ git show refs/remotes/p4/unshelved/12345
+$ git show p4/unshelved/12345
 
 $ git p4 unshelve 12345
 
diff --git a/git-p4.py b/git-p4.py
index 5701bad06a..76c18a22e9 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -3956,7 +3956,8 @@ def __init__(self):
 ]
 self.verbose = False
 self.noCommit = False
-self.destbranch = "refs/remotes/p4/unshelved"
+self.destbranch = "refs/remotes/p4-unshelved"
+self.origin = "p4/master"
 
 def renameBranch(self, branch_name):
 """ Rename the existing branch to branch_name.N
diff --git a/t/t9832-unshelve.sh b/t/t9832-unshelve.sh
index 48ec7679b8..c3d15ceea8 100755
--- a/t/t9832-unshelve.sh
+++ b/t/t9832-unshelve.sh
@@ -54,8 +54,8 @@ EOF
cd "$git" &&
change=$(last_shelved_change) &&
git p4 unshelve $change &&
-   git show refs/remotes/p4/unshelved/$change | grep -q "Further 
description" &&
-   git cherry-pick refs/remotes/p4/unshelved/$change &&
+   git show refs/remotes/p4-unshelved/$change | grep -q "Further 
description" &&
+   git cherry-pick refs/remotes/p4-unshelved/$change &&
test_path_is_file file2 &&
test_cmp file1 "$cli"/file1 &&
test_cmp file2 "$cli"/file2 &&
@@ -88,7 +88,7 @@ EOF
cd "$git" &&
change=$(last_shelved_change) &&
git p4 unshelve $change &&
-   git diff refs/remotes/p4/unshelved/$change.0 
refs/remotes/p4/unshelved/$change | grep -q file3
+   git diff refs/remotes/p4-unshelved/$change.0 
refs/remotes/p4-unshelved/$change | grep -q file3
)
 '
 
-- 
2.19.1.272.gf84b9b09d4

Re: [PATCHv2 0/3] git-p4: improved unshelving - small fixes

[Luke Diamand]

On Tue, 16 Oct 2018 at 05:27, Junio C Hamano  wrote:
>
> Luke Diamand  writes:
>
> > This is the same as my earlier patch other than fixing the
> > documentation to reflect the change to the destination branch,
> > as noticed by Junio.
>
> Also you set up when-finished driver for clean-up before running
> clone, which I think is a good change, too.
>
> Will replace.  Thanks.

At the moment the parent commit file revisions are constructed by
p4-printing the relevant files at the required version. That's because
it's easy to do - I can just make use of the existing infrastructure
for grabbing P4 changes.

However, in most cases we will already have these files present in the
git repo - it's just that we don't know where. I think it ought to be
possible to look at the revisions of each file, and then search
through the git revision history for that file to find the magic
comment marker that git-p4 inserts (which has the P4 changelist
embedded in it) and then use that - rather than going back to the
Perforce depot.

In most cases that ought to be quite a bit faster, especially for large files.

For now I'm not proposing to do this as it's quite a bit more work
(this isn't really my day job!) and for the typical use case (at least
the ones I encounter) the performance of unshelving isn't really that
important - the fact that it does it at all is quite amazing.

But anyway, if someone somewhere finds that git-p4 unshelve
performance is too slow, then that's the change to make.

Luke

Re: [ANNOUNCE] Git Merge Contributor's Summit Jan 31, 2019, Brussels

[Luke Diamand]

On Fri, 9 Nov 2018 at 10:48, Jeff King  wrote:
>
> On Fri, Nov 09, 2018 at 10:44:10AM +, Luca Milanesio wrote:
>
> > > On 9 Nov 2018, at 10:42, Jeff King  wrote:
> > >
> > > Git Merge 2019 is happening on February 1st. There will be a
> > > Contributor's Summit the day before. Here are the details:
> > >
> > >  When: Thursday, January 31, 2019. 10am-5pm.
> > >  Where: The Egg[1], Brussels, Belgium
> > >  What: Round-table discussion about Git
> > >  Who: All contributors to Git or related projects in the Git ecosystem
> > >   are invited; if you're not sure if you qualify, please ask!
> >
> > Hi Jeff,
> > is Gerrit included in the "Git ecosystem"?
>
> Yeah, I think so. At least the Git parts of it. :)

git-p4?

(The git parts obviously!)

>
> -Peff