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

[Miguel Torroja]

very nice subcommand, I tested it with a shelved CL and a submitted CL.
Find my comments inline

On Mon, Feb 26, 2018 at 12:48 PM, Luke Diamand  wrote:
> 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 
> ---
>  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 

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

[Miguel Torroja]

On Fri, Feb 23, 2018 at 6:22 PM, Luke Diamand <l...@diamand.org> wrote:
> 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

The behavior of "p4 unshelve" and your "git p4 unshelve" approach I
think are equivalent
as p4 just copies the whole contents of the shelved file locally,
regardless of the previous revision synced.
In other words, you get the same result with "git p4 unshelve" than
with "p4 unshelve" now.

What about creating a new command in a future update to apply just the
change to your local tree?
One approach we took in the past was to create a diff patch and then
apply it to the working tree.
The command "p4 describe -S -du 12345" will output a patch but it has
two problems:
 * the header for each file is not standard, it has to be parsed and
converted, (it starts with " //depot/..." and it needs to be
converted to "--- a/...")
 * The new files are not output as a patch
let's say we call the command "git p4 cherry-pick"

Miguel

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

[Miguel Torroja]

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.

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

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


Thanks,


On Thu, Feb 22, 2018 at 10:50 AM, Luke Diamand  wrote:
> 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 
> ---
>  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
> 

Re: [PATCH] git-p4: parse marshal output "p4 -G" in p4 changes

[Miguel Torroja]

Thanks,

I've just sent in reply to your previous e-mail three different patches.

* The first patch is just to show some broken tests,
* Second patch is to fix the original issue I had (the one that
initiated this thread)
* Third patch is the one that filters out "info" messages in p4CmdList
(this time default is reversed and set to False, what is the original
behaviour). The two test cases that are cured with this change have to
set explicitely skip_info=True.


On Wed, Jul 12, 2017 at 7:13 PM, Junio C Hamano <gits...@pobox.com> wrote:
> Miguel Torroja <miguel.torr...@gmail.com> writes:
>
>> The motivation for setting skip_info default to True is because any
>> extra message  output by a p4 trigger to stdout, seems to be reported
>> as {'code':'info'} when the p4 command output is marshalled.
>>
>> I though it was the less intrusive way to filter out the verbose
>> server trigger scripts, as some commands are waiting for a specific
>> order and size of the list returned e.g:
>>
>>  def p4_last_change():
>>  results = p4CmdList(["changes", "-m", "1"])
>>  return int(results[0]['change'])
>>  .
>>  def p4_describe(change):
>> ds = p4CmdList(["describe", "-s", str(change)])
>> if len(ds) != 1:
>> die("p4 describe -s %d did not return 1 result: %s" % (change, 
>> str(ds)))
>>
>> Previous examples would be broken if we allow extra "info" marshalled
>> messages to be exposed.
>>
>> In the case of the command that was broken with the new default
>> behaviour , when calling modfyChangelistUser, it is waiting for any
>> message with 'data' that is not an error to consider command was
>> succesful
>
> I somehow feel that this logic is totally backwards.  The current
> callers of p4CmdList() before your patch did not special case an
> entry that was marked as 'info' in its 'code' field.  Your new
> caller, which switched from using p4_read_pipe_lines() to p4CmdList()
> is one caller that you *know* wants to special case such an entry
> and wanted to skip.
>
> Your original patch that was queued to 'pu' for a while and then
> ejected from it after Travis saw an issue *assumed* that all other
> callers to p4CmdList() also want to special case such an entry, and
> that is why it made skip_info parameter default to True.
>
> The difference between knowing and assuming is the cause of the bug
> your original patch introduced into modifyChangelistUser().
>
> The way I read Luke's suggestion was that you can avoid making the
> same mistake by not changing the behaviour for existing callers you
> didn't look at.
>
> Instead of assuming everybody else do not want an entry with 'code'
> set to 'info', assume all the callers before your patch is doing
> fine, and when you *know* some of them are better off ignoring such
> an entry, explicitly tell them to do so, by:
>
>  * The first patch adds skip_info parameter that defaults to False
>to p4CmdList() and do the special casing when it is set to True.
>
>  * The second patch updates p4ChangesForPaths() to use the updated
>p4CmdList() and pass skip_info=True.  It is OK to squash this
>step into the first patch.
>
>  * The third and later patches, if you need them, each examines an
>existing caller of p4CmdList(), and add a new test to demonstrate
>the existing breakage that comes from not ignoring an entry whose
>'code' is 'info'.  That test would serve as a good documentation
>to explain why it is better for the caller to pass skip_info=True,
>so the same patch would also update the code to do so.
>
> While I was thinking the above through, here are a few cosmetic
> things that I noticed.  There is another comma that is not followed
> by a space in existing code that might want to be corrected in a
> clean-up patch but that is totally outside of the scope of this
> series.
>
> Thanks.
>
>  git-p4.py | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/git-p4.py b/git-p4.py
> index 1facf32db7..0d75753bce 100755
> --- a/git-p4.py
> +++ b/git-p4.py
> @@ -1501,7 +1501,7 @@ class P4Submit(Command, P4UserMap):
>  c['User'] = newUser
>  input = marshal.dumps(c)
>
> -result = p4CmdList("change -f -i", stdin=input,skip_info=False)
> +result = p4CmdList("change -f -i", stdin=input, skip_info=False)
>  for r in result:
>  if r.has_key('code'):
>  if r['code'] == 'error':
> @@ -1575,7 +1575,7 @@ class P4Submit(Command, P4UserMap):
>  files_list.append(value)
>  continue
>  # Output in the order expected by prepareLogMessage
> -for key in ['Change','Client','User','Status','Description','Jobs']:
> +for key in ['Change', 'Client', 'User', 'Status', 'Description', 
> 'Jobs']:
>  if not change_entry.has_key(key):
>  continue
>  template += '\n'

[PATCH 2/3] git-p4: parse marshal output "p4 -G" in p4 changes

[Miguel Torroja]

The option -G of p4 (python marshal output) gives more context about the
data being output. That's useful when using the command "change -o" as
we can distinguish between warning/error line and real change description.

This fixes the case where a p4 trigger for  "p4 change" is set and the command 
git-p4 submit is run.

Signed-off-by: Miguel Torroja <miguel.torr...@gmail.com>
---
 git-p4.py  | 85 +++---
 t/t9831-git-p4-triggers.sh |  2 +-
 2 files changed, 58 insertions(+), 29 deletions(-)

diff --git a/git-p4.py b/git-p4.py
index 8d151da91..e3a2791e0 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -879,8 +879,12 @@ def p4ChangesForPaths(depotPaths, changeRange, 
requestedBlockSize):
 cmd += ["%s...@%s" % (p, revisionRange)]
 
 # Insert changes in chronological order
-for line in reversed(p4_read_pipe_lines(cmd)):
-changes.add(int(line.split(" ")[1]))
+for entry in reversed(p4CmdList(cmd)):
+if entry.has_key('p4ExitCode'):
+die('Error retrieving changes descriptions 
({})'.format(entry['p4ExitCode']))
+if not entry.has_key('change'):
+continue
+changes.add(int(entry['change']))
 
 if not block_size:
 break
@@ -1526,37 +1530,62 @@ class P4Submit(Command, P4UserMap):
 
 [upstream, settings] = findUpstreamBranchPoint()
 
-template = ""
+template = """\
+# A Perforce Change Specification.
+#
+#  Change:  The change number. 'new' on a new changelist.
+#  Date:The date this specification was last modified.
+#  Client:  The client on which the changelist was created.  Read-only.
+#  User:The user who created the changelist.
+#  Status:  Either 'pending' or 'submitted'. Read-only.
+#  Type:Either 'public' or 'restricted'. Default is 'public'.
+#  Description: Comments about the changelist.  Required.
+#  Jobs:What opened jobs are to be closed by this changelist.
+#   You may delete jobs from this list.  (New changelists only.)
+#  Files:   What opened files from the default changelist are to be added
+#   to this changelist.  You may delete files from this list.
+#   (New changelists only.)
+"""
+files_list = []
 inFilesSection = False
+change_entry = None
 args = ['change', '-o']
 if changelist:
 args.append(str(changelist))
-
-for line in p4_read_pipe_lines(args):
-if line.endswith("\r\n"):
-line = line[:-2] + "\n"
-if inFilesSection:
-if line.startswith("\t"):
-# path starts and ends with a tab
-path = line[1:]
-lastTab = path.rfind("\t")
-if lastTab != -1:
-path = path[:lastTab]
-if settings.has_key('depot-paths'):
-if not [p for p in settings['depot-paths']
-if p4PathStartsWith(path, p)]:
-continue
-else:
-if not p4PathStartsWith(path, self.depotPath):
-continue
+for entry in p4CmdList(args):
+if not entry.has_key('code'):
+continue
+if entry['code'] == 'stat':
+change_entry = entry
+break
+if not change_entry:
+die('Failed to decode output of p4 change -o')
+for key, value in change_entry.iteritems():
+if key.startswith('File'):
+if settings.has_key('depot-paths'):
+if not [p for p in settings['depot-paths']
+if p4PathStartsWith(value, p)]:
+continue
 else:
-inFilesSection = False
-else:
-if line.startswith("Files:"):
-inFilesSection = True
-
-template += line
-
+if not p4PathStartsWith(value, self.depotPath):
+continue
+files_list.append(value)
+continue
+# Output in the order expected by prepareLogMessage
+for key in ['Change', 'Client', 'User', 'Status', 'Description', 
'Jobs']:
+if not change_entry.has_key(key):
+continue
+template += '\n'
+template += key + ':'
+if key == 'Description':
+template += '\n'
+for field_line in change_entry[key].splitlines():
+template += '\t'+field_line+'\n'
+if len(files_list) > 0:
+template += '\n'
+

[PATCH 3/3] git-p4: filter for {'code':'info'} in p4CmdList

[Miguel Torroja]

The function p4CmdList accepts a new argument: skip_info. When set to
True it ignores any 'code':'info' entry (skip_info=False by default).

That allows us to fix some of the tests in t9831-git-p4-triggers.sh
known to be broken with verobse p4 triggers

Signed-off-by: Miguel Torroja <miguel.torr...@gmail.com>
---
 git-p4.py  | 9 ++---
 t/t9831-git-p4-triggers.sh | 4 ++--
 2 files changed, 8 insertions(+), 5 deletions(-)

diff --git a/git-p4.py b/git-p4.py
index e3a2791e0..2fa581789 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -313,7 +313,7 @@ def p4_move(src, dest):
 p4_system(["move", "-k", wildcard_encode(src), wildcard_encode(dest)])
 
 def p4_last_change():
-results = p4CmdList(["changes", "-m", "1"])
+results = p4CmdList(["changes", "-m", "1"], skip_info=True)
 return int(results[0]['change'])
 
 def p4_describe(change):
@@ -321,7 +321,7 @@ def p4_describe(change):
the presence of field "time".  Return a dict of the
results."""
 
-ds = p4CmdList(["describe", "-s", str(change)])
+ds = p4CmdList(["describe", "-s", str(change)], skip_info=True)
 if len(ds) != 1:
 die("p4 describe -s %d did not return 1 result: %s" % (change, 
str(ds)))
 
@@ -509,7 +509,7 @@ def isModeExec(mode):
 def isModeExecChanged(src_mode, dst_mode):
 return isModeExec(src_mode) != isModeExec(dst_mode)
 
-def p4CmdList(cmd, stdin=None, stdin_mode='w+b', cb=None):
+def p4CmdList(cmd, stdin=None, stdin_mode='w+b', cb=None, skip_info=False):
 
 if isinstance(cmd,basestring):
 cmd = "-G " + cmd
@@ -545,6 +545,9 @@ def p4CmdList(cmd, stdin=None, stdin_mode='w+b', cb=None):
 try:
 while True:
 entry = marshal.load(p4.stdout)
+if skip_info:
+if 'code' in entry and entry['code'] == 'info':
+continue
 if cb is not None:
 cb(entry)
 else:
diff --git a/t/t9831-git-p4-triggers.sh b/t/t9831-git-p4-triggers.sh
index 871544b1c..bbcf14c66 100755
--- a/t/t9831-git-p4-triggers.sh
+++ b/t/t9831-git-p4-triggers.sh
@@ -20,7 +20,7 @@ test_expect_success 'init depot' '
)
 '
 
-test_expect_failure 'clone with extra info lines from verbose p4 trigger' '
+test_expect_success 'clone with extra info lines from verbose p4 trigger' '
test_when_finished cleanup_git &&
(
p4 triggers -i <<-EOF
@@ -38,7 +38,7 @@ test_expect_failure 'clone with extra info lines from verbose 
p4 trigger' '
)
 '
 
-test_expect_failure 'import with extra info lines from verbose p4 trigger' '
+test_expect_success 'import with extra info lines from verbose p4 trigger' '
test_when_finished cleanup_git &&
(
cd "$cli" &&
-- 
2.11.0

[PATCH 1/3] git-p4: git-p4 tests with p4 triggers

[Miguel Torroja]

Some p4 triggers in the server side generate some warnings when
executed. Unfortunately those messages are mixed with the output of
p4 commands. A few git-p4 commands don't expect extra messages or output
lines and may fail with verbose triggers.
New tests added are known to be broken.

Signed-off-by: Miguel Torroja <miguel.torr...@gmail.com>
---
 t/t9831-git-p4-triggers.sh | 103 +
 1 file changed, 103 insertions(+)
 create mode 100755 t/t9831-git-p4-triggers.sh

diff --git a/t/t9831-git-p4-triggers.sh b/t/t9831-git-p4-triggers.sh
new file mode 100755
index 0..28cafe469
--- /dev/null
+++ b/t/t9831-git-p4-triggers.sh
@@ -0,0 +1,103 @@
+#!/bin/sh
+
+test_description='git p4 with server triggers'
+
+. ./lib-git-p4.sh
+
+test_expect_success 'start p4d' '
+   start_p4d
+'
+
+test_expect_success 'init depot' '
+   (
+   cd "$cli" &&
+   echo file1 >file1 &&
+   p4 add file1 &&
+   p4 submit -d "change 1"
+   echo file2 >file2 &&
+   p4 add file2 &&
+   p4 submit -d "change 2"
+   )
+'
+
+test_expect_failure 'clone with extra info lines from verbose p4 trigger' '
+   test_when_finished cleanup_git &&
+   (
+   p4 triggers -i <<-EOF
+   Triggers: p4triggertest-command command pre-user-change "echo 
verbose trigger"
+   EOF
+   ) &&
+   (
+   p4 change -o |  grep -s "verbose trigger"
+   ) &&
+   git p4 clone --dest="$git" //depot/@all &&
+   (
+   p4 triggers -i <<-EOF
+   Triggers:
+   EOF
+   )
+'
+
+test_expect_failure 'import with extra info lines from verbose p4 trigger' '
+   test_when_finished cleanup_git &&
+   (
+   cd "$cli" &&
+   echo file3 >file3 &&
+   p4 add file3 &&
+   p4 submit -d "change 3"
+   ) &&
+   (
+   p4 triggers -i <<-EOF
+   Triggers: p4triggertest-command command pre-user-describe "echo 
verbose trigger"
+   EOF
+   ) &&
+   (
+   p4 describe 1 |  grep -s "verbose trigger"
+   ) &&
+   git p4 clone --dest="$git" //depot/@all &&
+   (
+   cd "$git" &&
+   git p4 sync
+   )&&
+   (
+   p4 triggers -i <<-EOF
+   Triggers:
+   EOF
+   )
+'
+
+test_expect_failure 'submit description with extra info lines from verbose p4 
change trigger' '
+   test_when_finished cleanup_git &&
+   (
+   p4 triggers -i <<-EOF
+   Triggers: p4triggertest-command command pre-user-change "echo 
verbose trigger"
+   EOF
+   ) &&
+   (
+   p4 change -o |  grep -s "verbose trigger"
+   ) &&
+   git p4 clone --dest="$git" //depot &&
+   (
+   cd "$git" &&
+   git config git-p4.skipSubmitEdit true &&
+   echo file4 >file4 &&
+   git add file4 &&
+   git commit -m file4 &&
+   git p4 submit
+   ) &&
+   (
+   p4 triggers -i <<-EOF
+   Triggers:
+   EOF
+   ) &&
+   (
+   cd "$cli" &&
+   test_path_is_file file4
+   )
+'
+
+test_expect_success 'kill p4d' '
+   kill_p4d
+'
+
+test_done
-- 
2.11.0

Re: [PATCH] git-p4: parse marshal output "p4 -G" in p4 changes

[Miguel Torroja]

The motivation for setting skip_info default to True is because any
extra message  output by a p4 trigger to stdout, seems to be reported
as {'code':'info'} when the p4 command output is marshalled.

I though it was the less intrusive way to filter out the verbose
server trigger scripts, as some commands are waiting for a specific
order and size of the list returned e.g:

 def p4_last_change():
 results = p4CmdList(["changes", "-m", "1"])
 return int(results[0]['change'])
 .
 def p4_describe(change):
ds = p4CmdList(["describe", "-s", str(change)])
if len(ds) != 1:
die("p4 describe -s %d did not return 1 result: %s" % (change, str(ds)))

Previous examples would be broken if we allow extra "info" marshalled
messages to be exposed.

In the case of the command that was broken with the new default
behaviour , when calling modfyChangelistUser, it is waiting for any
message with 'data' that is not an error to consider command was
succesful


Thanks,


On Wed, Jul 12, 2017 at 10:25 AM, Luke Diamand <l...@diamand.org> wrote:
> On 11 July 2017 at 23:53, Miguel Torroja <miguel.torr...@gmail.com> wrote:
>> The option -G of p4 (python marshal output) gives more context about the
>> data being output. That's useful when using the command "change -o" as
>> we can distinguish between warning/error line and real change description.
>>
>> Some p4 triggers in the server side generate some warnings when
>> executed. Unfortunately those messages are mixed with the output of
>> "p4 change -o". Those extra warning lines are reported as {'code':'info'}
>> in python marshal output (-G). The real change output is reported as
>> {'code':'stat'}
>>
>> the function p4CmdList accepts a new argument: skip_info. When set to
>> True it ignores any 'code':'info' entry (skip_info=True by default).
>>
>> A new test has been created in t9807-git-p4-submit.sh adding a p4 trigger
>> that outputs extra lines with "p4 change -o" and "p4 changes"
>>
>> Signed-off-by: Miguel Torroja <miguel.torr...@gmail.com>
>> ---
>>  git-p4.py| 92 
>> 
>>  t/t9807-git-p4-submit.sh | 30 
>>  2 files changed, 92 insertions(+), 30 deletions(-)
>>
>> diff --git a/git-p4.py b/git-p4.py
>> index 8d151da91..1facf32db 100755
>> --- a/git-p4.py
>> +++ b/git-p4.py
>> @@ -509,7 +509,7 @@ def isModeExec(mode):
>>  def isModeExecChanged(src_mode, dst_mode):
>>  return isModeExec(src_mode) != isModeExec(dst_mode)
>>
>> -def p4CmdList(cmd, stdin=None, stdin_mode='w+b', cb=None):
>> +def p4CmdList(cmd, stdin=None, stdin_mode='w+b', cb=None, skip_info=True):
>>
>>  if isinstance(cmd,basestring):
>>  cmd = "-G " + cmd
>> @@ -545,6 +545,9 @@ def p4CmdList(cmd, stdin=None, stdin_mode='w+b', 
>> cb=None):
>>  try:
>>  while True:
>>  entry = marshal.load(p4.stdout)
>> +if skip_info:
>> +if 'code' in entry and entry['code'] == 'info':
>> +continue
>>  if cb is not None:
>>  cb(entry)
>>  else:
>> @@ -879,8 +882,12 @@ def p4ChangesForPaths(depotPaths, changeRange, 
>> requestedBlockSize):
>>  cmd += ["%s...@%s" % (p, revisionRange)]
>>
>>  # Insert changes in chronological order
>> -for line in reversed(p4_read_pipe_lines(cmd)):
>> -changes.add(int(line.split(" ")[1]))
>> +for entry in reversed(p4CmdList(cmd)):
>> +if entry.has_key('p4ExitCode'):
>> +die('Error retrieving changes descriptions 
>> ({})'.format(entry['p4ExitCode']))
>> +if not entry.has_key('change'):
>> +continue
>> +changes.add(int(entry['change']))
>>
>>  if not block_size:
>>  break
>> @@ -1494,7 +1501,7 @@ class P4Submit(Command, P4UserMap):
>>  c['User'] = newUser
>>  input = marshal.dumps(c)
>>
>> -result = p4CmdList("change -f -i", stdin=input)
>> +result = p4CmdList("change -f -i", stdin=input,skip_info=False)
>
> Is there any reason this change sets skip_info to False in this one
> place, rather than defaulting to False (the original behavior) and
> setting it to True where it's needed?
>
> I worry that there might be other unexpected side effects in places
> not covered by the tests.
>
> Thanks
> Luke

[PATCH] git-p4: parse marshal output "p4 -G" in p4 changes

[Miguel Torroja]

The option -G of p4 (python marshal output) gives more context about the
data being output. That's useful when using the command "change -o" as
we can distinguish between warning/error line and real change description.

Some p4 triggers in the server side generate some warnings when
executed. Unfortunately those messages are mixed with the output of
"p4 change -o". Those extra warning lines are reported as {'code':'info'}
in python marshal output (-G). The real change output is reported as
{'code':'stat'}

the function p4CmdList accepts a new argument: skip_info. When set to
True it ignores any 'code':'info' entry (skip_info=True by default).

A new test has been created in t9807-git-p4-submit.sh adding a p4 trigger
that outputs extra lines with "p4 change -o" and "p4 changes"

Signed-off-by: Miguel Torroja <miguel.torr...@gmail.com>
---
 git-p4.py| 92 
 t/t9807-git-p4-submit.sh | 30 
 2 files changed, 92 insertions(+), 30 deletions(-)

diff --git a/git-p4.py b/git-p4.py
index 8d151da91..1facf32db 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -509,7 +509,7 @@ def isModeExec(mode):
 def isModeExecChanged(src_mode, dst_mode):
 return isModeExec(src_mode) != isModeExec(dst_mode)
 
-def p4CmdList(cmd, stdin=None, stdin_mode='w+b', cb=None):
+def p4CmdList(cmd, stdin=None, stdin_mode='w+b', cb=None, skip_info=True):
 
 if isinstance(cmd,basestring):
 cmd = "-G " + cmd
@@ -545,6 +545,9 @@ def p4CmdList(cmd, stdin=None, stdin_mode='w+b', cb=None):
 try:
 while True:
 entry = marshal.load(p4.stdout)
+if skip_info:
+if 'code' in entry and entry['code'] == 'info':
+continue
 if cb is not None:
 cb(entry)
 else:
@@ -879,8 +882,12 @@ def p4ChangesForPaths(depotPaths, changeRange, 
requestedBlockSize):
 cmd += ["%s...@%s" % (p, revisionRange)]
 
 # Insert changes in chronological order
-for line in reversed(p4_read_pipe_lines(cmd)):
-changes.add(int(line.split(" ")[1]))
+for entry in reversed(p4CmdList(cmd)):
+if entry.has_key('p4ExitCode'):
+die('Error retrieving changes descriptions 
({})'.format(entry['p4ExitCode']))
+if not entry.has_key('change'):
+continue
+changes.add(int(entry['change']))
 
 if not block_size:
 break
@@ -1494,7 +1501,7 @@ class P4Submit(Command, P4UserMap):
 c['User'] = newUser
 input = marshal.dumps(c)
 
-result = p4CmdList("change -f -i", stdin=input)
+result = p4CmdList("change -f -i", stdin=input,skip_info=False)
 for r in result:
 if r.has_key('code'):
 if r['code'] == 'error':
@@ -1526,37 +1533,62 @@ class P4Submit(Command, P4UserMap):
 
 [upstream, settings] = findUpstreamBranchPoint()
 
-template = ""
+template = """\
+# A Perforce Change Specification.
+#
+#  Change:  The change number. 'new' on a new changelist.
+#  Date:The date this specification was last modified.
+#  Client:  The client on which the changelist was created.  Read-only.
+#  User:The user who created the changelist.
+#  Status:  Either 'pending' or 'submitted'. Read-only.
+#  Type:Either 'public' or 'restricted'. Default is 'public'.
+#  Description: Comments about the changelist.  Required.
+#  Jobs:What opened jobs are to be closed by this changelist.
+#   You may delete jobs from this list.  (New changelists only.)
+#  Files:   What opened files from the default changelist are to be added
+#   to this changelist.  You may delete files from this list.
+#   (New changelists only.)
+"""
+files_list = []
 inFilesSection = False
+change_entry = None
 args = ['change', '-o']
 if changelist:
 args.append(str(changelist))
-
-for line in p4_read_pipe_lines(args):
-if line.endswith("\r\n"):
-line = line[:-2] + "\n"
-if inFilesSection:
-if line.startswith("\t"):
-# path starts and ends with a tab
-path = line[1:]
-lastTab = path.rfind("\t")
-if lastTab != -1:
-path = path[:lastTab]
-if settings.has_key('depot-paths'):
-if not [p for p in settings['depot-paths']
-if p4PathStartsWith(path, p)]:
-continue
-else:
-if not p4PathStartsWith(path, self.depotPath):
-

Re: [PATCH] git-p4: parse marshal output "p4 -G" in p4 changes

[Miguel Torroja]

Hi Luke,


My bad as I didn't check that case.
It was p4CmdList as you said. the default value of the new field
skip_info (set to True) ignores any info messages. and the script is
waiting for a valid message.
If I set it to False, then it does return an info entry and it accepts
the submit change

I'm sending another patch update

On Tue, Jul 11, 2017 at 10:35 AM, Luke Diamand <l...@diamand.org> wrote:
> On 3 July 2017 at 23:57, Miguel Torroja <miguel.torr...@gmail.com> wrote:
>> The option -G of p4 (python marshal output) gives more context about the
>> data being output. That's useful when using the command "change -o" as
>> we can distinguish between warning/error line and real change description.
>>
>> Some p4 triggers in the server side generate some warnings when
>> executed. Unfortunately those messages are mixed with the output of
>> "p4 change -o". Those extra warning lines are reported as {'code':'info'}
>> in python marshal output (-G). The real change output is reported as
>> {'code':'stat'}
>>
>> the function p4CmdList accepts a new argument: skip_info. When set to
>> True it ignores any 'code':'info' entry (skip_info=True by default).
>>
>> A new test has been created to t9807-git-p4-submit.sh adding a p4 trigger
>> that outputs extra lines with "p4 change -o" and "p4 changes"
>
> The latest version of mt/p4-parse-G-output (09521c7a0) seems to break
> t9813-git-p4-preserve-users.sh.
>
> I don't quite know why, but I wonder if it's the change to p4CmdList() ?
>
> Luke
>
>>
>> Signed-off-by: Miguel Torroja <miguel.torr...@gmail.com>
>> ---
>>  git-p4.py| 90 
>> 
>>  t/t9807-git-p4-submit.sh | 30 
>>  2 files changed, 91 insertions(+), 29 deletions(-)
>>
>> diff --git a/git-p4.py b/git-p4.py
>> index 8d151da91..a262e3253 100755
>> --- a/git-p4.py
>> +++ b/git-p4.py
>> @@ -509,7 +509,7 @@ def isModeExec(mode):
>>  def isModeExecChanged(src_mode, dst_mode):
>>  return isModeExec(src_mode) != isModeExec(dst_mode)
>>
>> -def p4CmdList(cmd, stdin=None, stdin_mode='w+b', cb=None):
>> +def p4CmdList(cmd, stdin=None, stdin_mode='w+b', cb=None, skip_info=True):
>>
>>  if isinstance(cmd,basestring):
>>  cmd = "-G " + cmd
>> @@ -545,6 +545,9 @@ def p4CmdList(cmd, stdin=None, stdin_mode='w+b', 
>> cb=None):
>>  try:
>>  while True:
>>  entry = marshal.load(p4.stdout)
>> +if skip_info:
>> +if 'code' in entry and entry['code'] == 'info':
>> +continue
>>  if cb is not None:
>>  cb(entry)
>>  else:
>> @@ -879,8 +882,12 @@ def p4ChangesForPaths(depotPaths, changeRange, 
>> requestedBlockSize):
>>  cmd += ["%s...@%s" % (p, revisionRange)]
>>
>>  # Insert changes in chronological order
>> -for line in reversed(p4_read_pipe_lines(cmd)):
>> -changes.add(int(line.split(" ")[1]))
>> +for entry in reversed(p4CmdList(cmd)):
>> +if entry.has_key('p4ExitCode'):
>> +die('Error retrieving changes descriptions 
>> ({})'.format(entry['p4ExitCode']))
>> +if not entry.has_key('change'):
>> +continue
>> +changes.add(int(entry['change']))
>>
>>  if not block_size:
>>  break
>> @@ -1526,37 +1533,62 @@ class P4Submit(Command, P4UserMap):
>>
>>  [upstream, settings] = findUpstreamBranchPoint()
>>
>> -template = ""
>> +template = """\
>> +# A Perforce Change Specification.
>> +#
>> +#  Change:  The change number. 'new' on a new changelist.
>> +#  Date:The date this specification was last modified.
>> +#  Client:  The client on which the changelist was created.  Read-only.
>> +#  User:The user who created the changelist.
>> +#  Status:  Either 'pending' or 'submitted'. Read-only.
>> +#  Type:Either 'public' or 'restricted'. Default is 'public'.
>> +#  Description: Comments about the changelist.  Required.
>> +#  Jobs:What opened jobs are to be closed by this changelist.
>> +#   You may delete jobs from this list.  (New changelists only.)
>> +#  Files:   What opened files from the default changelist are to be 
>> added
>> +#   to this changelist.  You may delete f

[PATCH] git-p4: parse marshal output "p4 -G" in p4 changes

[Miguel Torroja]

The option -G of p4 (python marshal output) gives more context about the
data being output. That's useful when using the command "change -o" as
we can distinguish between warning/error line and real change description.

Some p4 triggers in the server side generate some warnings when
executed. Unfortunately those messages are mixed with the output of
"p4 change -o". Those extra warning lines are reported as {'code':'info'}
in python marshal output (-G). The real change output is reported as
{'code':'stat'}

the function p4CmdList accepts a new argument: skip_info. When set to
True it ignores any 'code':'info' entry (skip_info=True by default).

A new test has been created to t9807-git-p4-submit.sh adding a p4 trigger
that outputs extra lines with "p4 change -o" and "p4 changes"

Signed-off-by: Miguel Torroja <miguel.torr...@gmail.com>
---
 git-p4.py| 90 
 t/t9807-git-p4-submit.sh | 30 
 2 files changed, 91 insertions(+), 29 deletions(-)

diff --git a/git-p4.py b/git-p4.py
index 8d151da91..a262e3253 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -509,7 +509,7 @@ def isModeExec(mode):
 def isModeExecChanged(src_mode, dst_mode):
 return isModeExec(src_mode) != isModeExec(dst_mode)
 
-def p4CmdList(cmd, stdin=None, stdin_mode='w+b', cb=None):
+def p4CmdList(cmd, stdin=None, stdin_mode='w+b', cb=None, skip_info=True):
 
 if isinstance(cmd,basestring):
 cmd = "-G " + cmd
@@ -545,6 +545,9 @@ def p4CmdList(cmd, stdin=None, stdin_mode='w+b', cb=None):
 try:
 while True:
 entry = marshal.load(p4.stdout)
+if skip_info:
+if 'code' in entry and entry['code'] == 'info':
+continue
 if cb is not None:
 cb(entry)
 else:
@@ -879,8 +882,12 @@ def p4ChangesForPaths(depotPaths, changeRange, 
requestedBlockSize):
 cmd += ["%s...@%s" % (p, revisionRange)]
 
 # Insert changes in chronological order
-for line in reversed(p4_read_pipe_lines(cmd)):
-changes.add(int(line.split(" ")[1]))
+for entry in reversed(p4CmdList(cmd)):
+if entry.has_key('p4ExitCode'):
+die('Error retrieving changes descriptions 
({})'.format(entry['p4ExitCode']))
+if not entry.has_key('change'):
+continue
+changes.add(int(entry['change']))
 
 if not block_size:
 break
@@ -1526,37 +1533,62 @@ class P4Submit(Command, P4UserMap):
 
 [upstream, settings] = findUpstreamBranchPoint()
 
-template = ""
+template = """\
+# A Perforce Change Specification.
+#
+#  Change:  The change number. 'new' on a new changelist.
+#  Date:The date this specification was last modified.
+#  Client:  The client on which the changelist was created.  Read-only.
+#  User:The user who created the changelist.
+#  Status:  Either 'pending' or 'submitted'. Read-only.
+#  Type:Either 'public' or 'restricted'. Default is 'public'.
+#  Description: Comments about the changelist.  Required.
+#  Jobs:What opened jobs are to be closed by this changelist.
+#   You may delete jobs from this list.  (New changelists only.)
+#  Files:   What opened files from the default changelist are to be added
+#   to this changelist.  You may delete files from this list.
+#   (New changelists only.)
+"""
+files_list = []
 inFilesSection = False
+change_entry = None
 args = ['change', '-o']
 if changelist:
 args.append(str(changelist))
-
-for line in p4_read_pipe_lines(args):
-if line.endswith("\r\n"):
-line = line[:-2] + "\n"
-if inFilesSection:
-if line.startswith("\t"):
-# path starts and ends with a tab
-path = line[1:]
-lastTab = path.rfind("\t")
-if lastTab != -1:
-path = path[:lastTab]
-if settings.has_key('depot-paths'):
-if not [p for p in settings['depot-paths']
-if p4PathStartsWith(path, p)]:
-continue
-else:
-if not p4PathStartsWith(path, self.depotPath):
-continue
+for entry in p4CmdList(args):
+if not entry.has_key('code'):
+continue
+if entry['code'] == 'stat':
+change_entry = entry
+break
+if not change_entry:
+die('Failed to decode output of p4 change -o')
+for key, value in change_entr

Re: [PATCH] git-p4: parse marshal output "p4 -G" in p4 changes

[Miguel Torroja]

I changed the patch a little bit, first change is to ignore by default
any {'code':'info'} in p4CmdList (they are not exposed to the caller)
as those are the verbose messages from triggers (p4 Debug does show
them). the second change is to check the p4 trigger is really set in
the test (Lars suggestion),

On Fri, Jun 30, 2017 at 6:02 PM, Miguel Torroja
<miguel.torr...@gmail.com> wrote:
> On Fri, Jun 30, 2017 at 12:13 PM, Lars Schneider
> <larsxschnei...@gmail.com> wrote:
>>
>>> On 30 Jun 2017, at 11:41, Miguel Torroja <miguel.torr...@gmail.com> wrote:
>>>
>>> On Fri, Jun 30, 2017 at 10:26 AM, Lars Schneider
>>> <larsxschnei...@gmail.com> wrote:
>>>>
>>>>> On 30 Jun 2017, at 00:46, miguel torroja <miguel.torr...@gmail.com> wrote:
>>>>>
>>>>> The option -G of p4 (python marshal output) gives more context about the
>>>>> data being output. That's useful when using the command "change -o" as
>>>>> we can distinguish between warning/error line and real change description.
>>>>>
>>>>> Some p4 triggers in the server side generate some warnings when
>>>>> executed. Unfortunately those messages are mixed with the output of
>>>>> "p4 change -o". Those extra warning lines are reported as {'code':'info'}
>>>>> in python marshal output (-G). The real change output is reported as
>>>>> {'code':'stat'}
>>>>>
>>>>> A new test has been created to t9807-git-p4-submit.sh adding a p4 trigger
>>>>> that outputs extra lines with "p4 change -o" and "p4 changes"
>>>>>
>>>>> Signed-off-by: Miguel Torroja <miguel.torr...@gmail.com>
>>>>> Signed-off-by: Junio C Hamano <gits...@pobox.com>
>>>>> ---
>>>>> ...
>>>>
>>>> I have never worked with p4 triggers and that might be
>>>> the reason why I don't understand your test case.
>>>> Maybe you can help me?
>>>>
>>>>> +test_expect_success 'description with extra lines from verbose p4 
>>>>> trigger' '
>>>>> + test_when_finished cleanup_git &&
>>>>> + git p4 clone --dest="$git" //depot &&
>>>>> + (
>>>>> + p4 triggers -i <<-EOF
>>>>> + Triggers: p4triggertest-command command pre-user-change 
>>>>> "echo verbose trigger"
>>>>> + EOF
>>>>> + ) &&
>>>>
>>>> You clone the test repo and install a trigger.
>>>>
>>>>> + (
>>>>> + cd "$git" &&
>>>>> + git config git-p4.skipSubmitEdit true &&
>>>>> + echo file20 >file20 &&
>>>>> + git add file20 &&
>>>>> + git commit -m file20 &&
>>>>> + git p4 submit
>>>>> + ) &&
>>>>
>>>> You make a new commit. This should run the "echo verbose trigger", right?
>>>
>>> Yes, that's correct. In this case the trigger is run with p4 change
>>> and p4 changes
>>>
>>>>
>>>>> + (
>>>>> + p4 triggers -i <<-EOF
>>>>> + Triggers:
>>>>> + EOF
>>>>> + ) &&
>>>>
>>>> You delete the trigger.
>>>>
>>>>> + (
>>>>> + cd "$cli" &&
>>>>> + test_path_is_file file20
>>>>> + )
>>>>
>>>> You check that the file20 is available in P4.
>>>>
>>>>
>>>> What would happen if I run this test case without your patch?
>>>> Wouldn't it pass just fine?
>>>
>>> If you run it without the patch for git-p4.py, the test doesn't pass
>>
>> You are right. I did not run "make" properly before running the test :)
>>
>>
>>>> Wouldn't we need to check that no warning/error is in the
>>>> real change description?
>>>>
>>>
>>> that can also be added, something like this: 'p4 change -o | grep
>>> "verbose trigger"' after setting the trigger?
>>
>> Yeah, maybe. I hope this is no stupid question, but: If you clone the
>> repo with git-p4 *again* ... would you see the "verbose trigger" output
>> in the Git commit message?
>>
>
> The commands that are affected are the ones that don't use the -G
> option, as everything is sent to the standard output without being
> able to filter out what is the real contents or just info messages.
> That's not the case with the python output (-G). Having said that... I
> tried what you just said (just to be sure) and the function
> p4_last_change fails... as it expects the first dictionary returned by
> p4CmdList is the one that contains the change:
> "int(results[0]['change'])" and that's not the case as it's an info
> entry (no 'change' key, that's in the next entry...)  I'll update with
> new patches
>
> I didn't notice that before because the P4 server we have in our
> office only outputs extra info messages with the command "p4 change".
>
>
>> - Lars

Re: [PATCH] git-p4: parse marshal output "p4 -G" in p4 changes

[Miguel Torroja]

On Fri, Jun 30, 2017 at 12:13 PM, Lars Schneider
<larsxschnei...@gmail.com> wrote:
>
>> On 30 Jun 2017, at 11:41, Miguel Torroja <miguel.torr...@gmail.com> wrote:
>>
>> On Fri, Jun 30, 2017 at 10:26 AM, Lars Schneider
>> <larsxschnei...@gmail.com> wrote:
>>>
>>>> On 30 Jun 2017, at 00:46, miguel torroja <miguel.torr...@gmail.com> wrote:
>>>>
>>>> The option -G of p4 (python marshal output) gives more context about the
>>>> data being output. That's useful when using the command "change -o" as
>>>> we can distinguish between warning/error line and real change description.
>>>>
>>>> Some p4 triggers in the server side generate some warnings when
>>>> executed. Unfortunately those messages are mixed with the output of
>>>> "p4 change -o". Those extra warning lines are reported as {'code':'info'}
>>>> in python marshal output (-G). The real change output is reported as
>>>> {'code':'stat'}
>>>>
>>>> A new test has been created to t9807-git-p4-submit.sh adding a p4 trigger
>>>> that outputs extra lines with "p4 change -o" and "p4 changes"
>>>>
>>>> Signed-off-by: Miguel Torroja <miguel.torr...@gmail.com>
>>>> Signed-off-by: Junio C Hamano <gits...@pobox.com>
>>>> ---
>>>> ...
>>>
>>> I have never worked with p4 triggers and that might be
>>> the reason why I don't understand your test case.
>>> Maybe you can help me?
>>>
>>>> +test_expect_success 'description with extra lines from verbose p4 
>>>> trigger' '
>>>> + test_when_finished cleanup_git &&
>>>> + git p4 clone --dest="$git" //depot &&
>>>> + (
>>>> + p4 triggers -i <<-EOF
>>>> + Triggers: p4triggertest-command command pre-user-change 
>>>> "echo verbose trigger"
>>>> + EOF
>>>> + ) &&
>>>
>>> You clone the test repo and install a trigger.
>>>
>>>> + (
>>>> + cd "$git" &&
>>>> + git config git-p4.skipSubmitEdit true &&
>>>> + echo file20 >file20 &&
>>>> + git add file20 &&
>>>> + git commit -m file20 &&
>>>> + git p4 submit
>>>> + ) &&
>>>
>>> You make a new commit. This should run the "echo verbose trigger", right?
>>
>> Yes, that's correct. In this case the trigger is run with p4 change
>> and p4 changes
>>
>>>
>>>> + (
>>>> + p4 triggers -i <<-EOF
>>>> + Triggers:
>>>> + EOF
>>>> + ) &&
>>>
>>> You delete the trigger.
>>>
>>>> + (
>>>> + cd "$cli" &&
>>>> + test_path_is_file file20
>>>> + )
>>>
>>> You check that the file20 is available in P4.
>>>
>>>
>>> What would happen if I run this test case without your patch?
>>> Wouldn't it pass just fine?
>>
>> If you run it without the patch for git-p4.py, the test doesn't pass
>
> You are right. I did not run "make" properly before running the test :)
>
>
>>> Wouldn't we need to check that no warning/error is in the
>>> real change description?
>>>
>>
>> that can also be added, something like this: 'p4 change -o | grep
>> "verbose trigger"' after setting the trigger?
>
> Yeah, maybe. I hope this is no stupid question, but: If you clone the
> repo with git-p4 *again* ... would you see the "verbose trigger" output
> in the Git commit message?
>

The commands that are affected are the ones that don't use the -G
option, as everything is sent to the standard output without being
able to filter out what is the real contents or just info messages.
That's not the case with the python output (-G). Having said that... I
tried what you just said (just to be sure) and the function
p4_last_change fails... as it expects the first dictionary returned by
p4CmdList is the one that contains the change:
"int(results[0]['change'])" and that's not the case as it's an info
entry (no 'change' key, that's in the next entry...)  I'll update with
new patches

I didn't notice that before because the P4 server we have in our
office only outputs extra info messages with the command "p4 change".


> - Lars

Re: [PATCH] git-p4: parse marshal output "p4 -G" in p4 changes

[Miguel Torroja]

On Fri, Jun 30, 2017 at 10:26 AM, Lars Schneider
<larsxschnei...@gmail.com> wrote:
>
>> On 30 Jun 2017, at 00:46, miguel torroja <miguel.torr...@gmail.com> wrote:
>>
>> The option -G of p4 (python marshal output) gives more context about the
>> data being output. That's useful when using the command "change -o" as
>> we can distinguish between warning/error line and real change description.
>>
>> Some p4 triggers in the server side generate some warnings when
>> executed. Unfortunately those messages are mixed with the output of
>> "p4 change -o". Those extra warning lines are reported as {'code':'info'}
>> in python marshal output (-G). The real change output is reported as
>> {'code':'stat'}
>>
>> A new test has been created to t9807-git-p4-submit.sh adding a p4 trigger
>> that outputs extra lines with "p4 change -o" and "p4 changes"
>>
>> Signed-off-by: Miguel Torroja <miguel.torr...@gmail.com>
>> Signed-off-by: Junio C Hamano <gits...@pobox.com>
>> ---
>> git-p4.py| 85 
>> 
>> t/t9807-git-p4-submit.sh | 27 +++
>> 2 files changed, 84 insertions(+), 28 deletions(-)
>>
>> diff --git a/git-p4.py b/git-p4.py
>> index 8d151da91..61dc045f3 100755
>> --- a/git-p4.py
>> +++ b/git-p4.py
>> @@ -879,8 +879,12 @@ def p4ChangesForPaths(depotPaths, changeRange, 
>> requestedBlockSize):
>> cmd += ["%s...@%s" % (p, revisionRange)]
>>
>> # Insert changes in chronological order
>> -for line in reversed(p4_read_pipe_lines(cmd)):
>> -changes.add(int(line.split(" ")[1]))
>> +for entry in reversed(p4CmdList(cmd)):
>> +if entry.has_key('p4ExitCode'):
>> +die('Error retrieving changes descriptions 
>> ({})'.format(entry['p4ExitCode']))
>> +if not entry.has_key('change'):
>> +continue
>> +changes.add(int(entry['change']))
>>
>> if not block_size:
>> break
>> @@ -1526,37 +1530,62 @@ class P4Submit(Command, P4UserMap):
>>
>> [upstream, settings] = findUpstreamBranchPoint()
>>
>> -template = ""
>> +template = """\
>> +# A Perforce Change Specification.
>> +#
>> +#  Change:  The change number. 'new' on a new changelist.
>> +#  Date:The date this specification was last modified.
>> +#  Client:  The client on which the changelist was created.  Read-only.
>> +#  User:The user who created the changelist.
>> +#  Status:  Either 'pending' or 'submitted'. Read-only.
>> +#  Type:Either 'public' or 'restricted'. Default is 'public'.
>> +#  Description: Comments about the changelist.  Required.
>> +#  Jobs:What opened jobs are to be closed by this changelist.
>> +#   You may delete jobs from this list.  (New changelists only.)
>> +#  Files:   What opened files from the default changelist are to be 
>> added
>> +#   to this changelist.  You may delete files from this list.
>> +#   (New changelists only.)
>> +"""
>> +files_list = []
>> inFilesSection = False
>> +change_entry = None
>> args = ['change', '-o']
>> if changelist:
>> args.append(str(changelist))
>> -
>> -for line in p4_read_pipe_lines(args):
>> -if line.endswith("\r\n"):
>> -line = line[:-2] + "\n"
>> -if inFilesSection:
>> -if line.startswith("\t"):
>> -# path starts and ends with a tab
>> -path = line[1:]
>> -lastTab = path.rfind("\t")
>> -if lastTab != -1:
>> -path = path[:lastTab]
>> -if settings.has_key('depot-paths'):
>> -if not [p for p in settings['depot-paths']
>> -if p4PathStartsWith(path, p)]:
>> -continue
>> -else:
>> -if not p4PathStartsWith(path, self.depotPath):
>> -continue
>> +for entry in p4CmdList(args):
>> +if not entry.has_key('code'):
>> +continue
>> +if entry['

Re: [PATCH] git-p4: parse marshal output "p4 -G" in p4 changes

[miguel torroja]

The Latest patch I sent was already the squashed version with the fix
to pass the tests.

Thanks,

On Fri, Jun 30, 2017 at 9:56 AM, Luke Diamand <l...@diamand.org> wrote:
> On 29 June 2017 at 23:41, miguel torroja <miguel.torr...@gmail.com> wrote:
>> On Thu, Jun 29, 2017 at 8:59 AM, Luke Diamand <l...@diamand.org> wrote:
>>> On 28 June 2017 at 14:14, miguel torroja <miguel.torr...@gmail.com> wrote:
>>>> Thanks Luke,
>>>>
>>>> regarding the error in t9800 (not ok 18 - unresolvable host in P4PORT
>>>> should display error), for me it's very weird too as it doesn't seem
>>>> to be related to this particular change, as the patch changes are not
>>>> exercised with that test.
>>>
>>> I had a look at this. The problem is that the old code uses
>>> p4_read_pipe_lines() which calls sys.exit() if the subprocess fails.
>>>
>>> But the new code calls p4CmdList() which has does error handling by
>>> setting "p4ExitCode" to a non-zero value in the returned dictionary.
>>>
>>> I think if you just check for that case, the test will then pass
>>
>> Thank you for debugging this,  I did as you suggested and it passed that 
>> test!
>>
>>>>
>>>> The test 21 in t9807 was precisely the new test added to test the
>>>> change (it was passing with local setup), the test log is truncated
>>>> before the output of test 21 in t9807 but I'm afraid I'm not very
>>>> familiar with Travis, so maybe I'm missing something. Is there a way
>>>> to have the full logs or they are always truncated after some number
>>>> of lines?
>>>
>>> For me, t9807 is working fine.
>>>
>>>>
>>>> I think you get an error with git diff --check because I added spaces
>>>> after a tab, but those spaces are intentional, the tabs are for the
>>>> "<<-EOF" and spaces are for the "p4 triggers" specificiation.
>>>
>>> OK.
>>>
>>
>> In the end, ,the reason t9807 was not passing was precisely the tabs
>> and spaces of the patch. the original patch had:
>> ., as I explained, the tabs were supposed to be
>> ignored by "<<-EOF" and the spaces were supposed to be sent to stdin
>> of p4 triggers, but when the patch was applied to upstream the
>>  were substituted by tabs what led to a malformed  "p4
>> trigger" description. I just collapsed the description in one single
>> line and now it's passing
>>>
>>> Luke
>>
>>
>> I'm sending a new patch with the two changes I just mentioned.
>
> Looks good to me, Ack. Can we squash the two changes together?
>
> Luke

[PATCH] git-p4: parse marshal output "p4 -G" in p4 changes

[Miguel Torroja]

The option -G of p4 (python marshal output) gives more context about the
data being output. That's useful when using the command "change -o" as
we can distinguish between warning/error line and real change description.

Some p4 triggers in the server side generate some warnings when
executed. Unfortunately those messages are mixed with the output of
"p4 change -o". Those extra warning lines are reported as {'code':'info'}
in python marshal output (-G). The real change output is reported as
{'code':'stat'}

A new test has been created to t9807-git-p4-submit.sh adding a p4 trigger
that outputs extra lines with "p4 change -o" and "p4 changes"

Signed-off-by: Miguel Torroja <miguel.torr...@gmail.com>
Signed-off-by: Junio C Hamano <gits...@pobox.com>
---
 git-p4.py| 85 
 t/t9807-git-p4-submit.sh | 27 +++
 2 files changed, 84 insertions(+), 28 deletions(-)

diff --git a/git-p4.py b/git-p4.py
index 8d151da91..61dc045f3 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -879,8 +879,12 @@ def p4ChangesForPaths(depotPaths, changeRange, 
requestedBlockSize):
 cmd += ["%s...@%s" % (p, revisionRange)]
 
 # Insert changes in chronological order
-for line in reversed(p4_read_pipe_lines(cmd)):
-changes.add(int(line.split(" ")[1]))
+for entry in reversed(p4CmdList(cmd)):
+if entry.has_key('p4ExitCode'):
+die('Error retrieving changes descriptions 
({})'.format(entry['p4ExitCode']))
+if not entry.has_key('change'):
+continue
+changes.add(int(entry['change']))
 
 if not block_size:
 break
@@ -1526,37 +1530,62 @@ class P4Submit(Command, P4UserMap):
 
 [upstream, settings] = findUpstreamBranchPoint()
 
-template = ""
+template = """\
+# A Perforce Change Specification.
+#
+#  Change:  The change number. 'new' on a new changelist.
+#  Date:The date this specification was last modified.
+#  Client:  The client on which the changelist was created.  Read-only.
+#  User:The user who created the changelist.
+#  Status:  Either 'pending' or 'submitted'. Read-only.
+#  Type:Either 'public' or 'restricted'. Default is 'public'.
+#  Description: Comments about the changelist.  Required.
+#  Jobs:What opened jobs are to be closed by this changelist.
+#   You may delete jobs from this list.  (New changelists only.)
+#  Files:   What opened files from the default changelist are to be added
+#   to this changelist.  You may delete files from this list.
+#   (New changelists only.)
+"""
+files_list = []
 inFilesSection = False
+change_entry = None
 args = ['change', '-o']
 if changelist:
 args.append(str(changelist))
-
-for line in p4_read_pipe_lines(args):
-if line.endswith("\r\n"):
-line = line[:-2] + "\n"
-if inFilesSection:
-if line.startswith("\t"):
-# path starts and ends with a tab
-path = line[1:]
-lastTab = path.rfind("\t")
-if lastTab != -1:
-path = path[:lastTab]
-if settings.has_key('depot-paths'):
-if not [p for p in settings['depot-paths']
-if p4PathStartsWith(path, p)]:
-continue
-else:
-if not p4PathStartsWith(path, self.depotPath):
-continue
+for entry in p4CmdList(args):
+if not entry.has_key('code'):
+continue
+if entry['code'] == 'stat':
+change_entry = entry
+break
+if not change_entry:
+die('Failed to decode output of p4 change -o')
+for key, value in change_entry.iteritems():
+if key.startswith('File'):
+if settings.has_key('depot-paths'):
+if not [p for p in settings['depot-paths']
+if p4PathStartsWith(value, p)]:
+continue
 else:
-inFilesSection = False
-else:
-if line.startswith("Files:"):
-inFilesSection = True
-
-template += line
-
+if not p4PathStartsWith(value, self.depotPath):
+continue
+files_list.append(value)
+continue
+# Output in the order expected by prepareLogMessage
+for key in ['Change','Client','User','Status','Description','Job

Re: [PATCH] git-p4: parse marshal output "p4 -G" in p4 changes

[miguel torroja]

On Thu, Jun 29, 2017 at 8:59 AM, Luke Diamand <l...@diamand.org> wrote:
> On 28 June 2017 at 14:14, miguel torroja <miguel.torr...@gmail.com> wrote:
>> Thanks Luke,
>>
>> regarding the error in t9800 (not ok 18 - unresolvable host in P4PORT
>> should display error), for me it's very weird too as it doesn't seem
>> to be related to this particular change, as the patch changes are not
>> exercised with that test.
>
> I had a look at this. The problem is that the old code uses
> p4_read_pipe_lines() which calls sys.exit() if the subprocess fails.
>
> But the new code calls p4CmdList() which has does error handling by
> setting "p4ExitCode" to a non-zero value in the returned dictionary.
>
> I think if you just check for that case, the test will then pass

Thank you for debugging this,  I did as you suggested and it passed that test!

>>
>> The test 21 in t9807 was precisely the new test added to test the
>> change (it was passing with local setup), the test log is truncated
>> before the output of test 21 in t9807 but I'm afraid I'm not very
>> familiar with Travis, so maybe I'm missing something. Is there a way
>> to have the full logs or they are always truncated after some number
>> of lines?
>
> For me, t9807 is working fine.
>
>>
>> I think you get an error with git diff --check because I added spaces
>> after a tab, but those spaces are intentional, the tabs are for the
>> "<<-EOF" and spaces are for the "p4 triggers" specificiation.
>
> OK.
>

In the end, ,the reason t9807 was not passing was precisely the tabs
and spaces of the patch. the original patch had:
., as I explained, the tabs were supposed to be
ignored by "<<-EOF" and the spaces were supposed to be sent to stdin
of p4 triggers, but when the patch was applied to upstream the
 were substituted by tabs what led to a malformed  "p4
trigger" description. I just collapsed the description in one single
line and now it's passing
>
> Luke


I'm sending a new patch with the two changes I just mentioned.

Thanks,

Re: [PATCH] git-p4: parse marshal output "p4 -G" in p4 changes

[miguel torroja]

Thanks Luke,

regarding the error in t9800 (not ok 18 - unresolvable host in P4PORT
should display error), for me it's very weird too as it doesn't seem
to be related to this particular change, as the patch changes are not
exercised with that test.

The test 21 in t9807 was precisely the new test added to test the
change (it was passing with local setup), the test log is truncated
before the output of test 21 in t9807 but I'm afraid I'm not very
familiar with Travis, so maybe I'm missing something. Is there a way
to have the full logs or they are always truncated after some number
of lines?

I think you get an error with git diff --check because I added spaces
after a tab, but those spaces are intentional, the tabs are for the
"<<-EOF" and spaces are for the "p4 triggers" specificiation.

Thanks,


On Wed, Jun 28, 2017 at 11:54 AM, Luke Diamand <l...@diamand.org> wrote:
> On 28 June 2017 at 05:08, Junio C Hamano <gits...@pobox.com> wrote:
>> Miguel Torroja <miguel.torr...@gmail.com> writes:
>>
>>> The option -G of p4 (python marshal output) gives more context about the
>>> data being output. That's useful when using the command "change -o" as
>>> we can distinguish between warning/error line and real change description.
>>>
>>> Some p4 triggers in the server side generate some warnings when
>>> executed. Unfortunately those messages are mixed with the output of
>>> "p4 change -o". Those extra warning lines are reported as {'code':'info'}
>>> in python marshal output (-G). The real change output is reported as
>>> {'code':'stat'}
>>>
>>> A new test has been created to t9807-git-p4-submit.sh adding a p4 trigger
>>> that outputs extra lines with "p4 change -o" and "p4 changes"
>>>
>>> Signed-off-by: Miguel Torroja <miguel.torr...@gmail.com>
>>> ---
>>
>> It appears that https://travis-ci.org/git/git/builds/247724639
>> does not like this change.  For example:
>>
>> https://travis-ci.org/git/git/jobs/247724642#L1848
>>
>> indicates that not just 9807 (new tests added by this patch) but
>> also 9800 starts to fail.
>>
>> I'd wait for git-p4 experts to comment and help guiding this change
>> forward.
>
> I only see a (very weird) failure in t9800. I wonder if there are some
> P4 version differences.
>
> Client: Rev. P4/LINUX26X86_64/2015.1/1024208 (2015/03/16).
> Server: P4D/LINUX26X86_64/2015.1/1028542 (2015/03/20)
>
> There's also a whitespace error according to "git diff --check".
> :
> Sadly I don't think there's any way to do this and yet keep the "#
> edit" comments. It looks like "p4 change -o" outputs lines with "'#
> edit" on the end, but the (supposedly semantically equivalent) "p4 -G
> change -o" command does not. I think that's a P4 bug.
>
> So we have a choice of fixing a garbled message in the face of scripts
> in the backend, or keeping the comments, or writing some extra Python
> to infer them. I vote for fixing the garbled message.
>
> Luke

[PATCH] git-p4: parse marshal output "p4 -G" in p4 changes

[Miguel Torroja]

The option -G of p4 (python marshal output) gives more context about the
data being output. That's useful when using the command "change -o" as
we can distinguish between warning/error line and real change description.

Some p4 triggers in the server side generate some warnings when
executed. Unfortunately those messages are mixed with the output of
"p4 change -o". Those extra warning lines are reported as {'code':'info'}
in python marshal output (-G). The real change output is reported as
{'code':'stat'}

A new test has been created to t9807-git-p4-submit.sh adding a p4 trigger
that outputs extra lines with "p4 change -o" and "p4 changes"

Signed-off-by: Miguel Torroja <miguel.torr...@gmail.com>
---
 git-p4.py| 83 
 t/t9807-git-p4-submit.sh | 28 
 2 files changed, 83 insertions(+), 28 deletions(-)

diff --git a/git-p4.py b/git-p4.py
index 8d151da91..239a8f144 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -879,8 +879,10 @@ def p4ChangesForPaths(depotPaths, changeRange, 
requestedBlockSize):
 cmd += ["%s...@%s" % (p, revisionRange)]
 
 # Insert changes in chronological order
-for line in reversed(p4_read_pipe_lines(cmd)):
-changes.add(int(line.split(" ")[1]))
+for entry in reversed(p4CmdList(cmd)):
+if not entry.has_key('change'):
+continue
+changes.add(int(entry['change']))
 
 if not block_size:
 break
@@ -1526,37 +1528,62 @@ class P4Submit(Command, P4UserMap):
 
 [upstream, settings] = findUpstreamBranchPoint()
 
-template = ""
+template = """\
+# A Perforce Change Specification.
+#
+#  Change:  The change number. 'new' on a new changelist.
+#  Date:The date this specification was last modified.
+#  Client:  The client on which the changelist was created.  Read-only.
+#  User:The user who created the changelist.
+#  Status:  Either 'pending' or 'submitted'. Read-only.
+#  Type:Either 'public' or 'restricted'. Default is 'public'.
+#  Description: Comments about the changelist.  Required.
+#  Jobs:What opened jobs are to be closed by this changelist.
+#   You may delete jobs from this list.  (New changelists only.)
+#  Files:   What opened files from the default changelist are to be added
+#   to this changelist.  You may delete files from this list.
+#   (New changelists only.)
+"""
+files_list = []
 inFilesSection = False
+change_entry = None
 args = ['change', '-o']
 if changelist:
 args.append(str(changelist))
-
-for line in p4_read_pipe_lines(args):
-if line.endswith("\r\n"):
-line = line[:-2] + "\n"
-if inFilesSection:
-if line.startswith("\t"):
-# path starts and ends with a tab
-path = line[1:]
-lastTab = path.rfind("\t")
-if lastTab != -1:
-path = path[:lastTab]
-if settings.has_key('depot-paths'):
-if not [p for p in settings['depot-paths']
-if p4PathStartsWith(path, p)]:
-continue
-else:
-if not p4PathStartsWith(path, self.depotPath):
-continue
+for entry in p4CmdList(args):
+if not entry.has_key('code'):
+continue
+if entry['code'] == 'stat':
+change_entry = entry
+break
+if not change_entry:
+die('Failed to decode output of p4 change -o')
+for key, value in change_entry.iteritems():
+if key.startswith('File'):
+if settings.has_key('depot-paths'):
+if not [p for p in settings['depot-paths']
+if p4PathStartsWith(value, p)]:
+continue
 else:
-inFilesSection = False
-else:
-if line.startswith("Files:"):
-inFilesSection = True
-
-template += line
-
+if not p4PathStartsWith(value, self.depotPath):
+continue
+files_list.append(value)
+continue
+# Output in the order expected by prepareLogMessage
+for key in ['Change','Client','User','Status','Description','Jobs']:
+if not change_entry.has_key(key):
+continue
+template += '\n'
+template += key + ':'
+if key == 'Description':
+

Re: [PATCH] git-p4: changelist template with p4 -G change -o

[miguel torroja]

Hi Lars/Luke,

I tried a first test extending t9807-git-p4-submit.sh. I set this p4
trigger: 'p4test command pre-user-change "echo verbose trigger" '. I'm
able to reproduce the issue I wanted to fix. However I found yet
another issue, in this case when reading the result from
p4_read_pipe_lines (in function p4ChangesForPaths), the
pre-user-change is triggered with any "p4 change" and "p4 changes"
command (the p4 server we have in production, only shows "extra"
messages with p4 change).
   I'll collapse in one single commit the fix for p4 change/p4 changes
and the new test.

Thanks,

Miguel

On Sat, Jun 24, 2017 at 10:37 PM, miguel torroja
<miguel.torr...@gmail.com> wrote:
> Hi Lars,
>
> I think it's doable to set a custom p4 trigger, created by the test case,
> that outputs "extra info" when requesting a changelist description.
> I'll do a specific test and post it to this thread.
>
>
> Thanks,
>
>
> El 24 jun. 2017 7:36 p. m., "Lars Schneider" <larsxschnei...@gmail.com>
> escribió:
>
>
>> On 24 Jun 2017, at 13:49, Luke Diamand <l...@diamand.org> wrote:
>>
>> On 22 June 2017 at 18:32, Junio C Hamano <gits...@pobox.com> wrote:
>>> Miguel Torroja <miguel.torr...@gmail.com> writes:
>>>
>>>> The option -G of p4 (python marshal output) gives more context about the
>>>> data being output. That's useful when using the command "change -o" as
>>>> we can distinguish between warning/error line and real change
>>>> description.
>>>>
>>>> Some p4 plugin/hooks in the server side generates some warnings when
>>>> executed. Unfortunately those messages are mixed with the output of
>>>> "p4 change -o". Those extra warning lines are reported as
>>>> {'code':'info'}
>>>> in python marshal output (-G). The real change output is reported as
>>>> {'code':'stat'}
>>
>> I think this seems like a reasonable thing to do if "p4 change -o" is
>> jumbling up output.
>>
>> One thing I notice trying it out by hand is that we seem to have lost
>> the annotation of the Perforce per-file modification type (is there a
>> proper name for this?).
>>
>> For example, if I add a file called "baz", then the original version
>> creates a template which looks like this:
>>
>>   //depot/baz# add
>>
>> But the new one creates a template which looks like:
>>
>>   //depot/baz
>
> @Miguel: You wrote that p4 plugins/hooks generate these warnings.
> I wonder if you see a way to replicate that in a test case. Either
> in t9800 or a new t98XX test case file?
>
> - Lars
>
>

Re: [PATCH] git-p4: changelist template with p4 -G change -o

[miguel torroja]

You are right about the "# add"comment. I couldn't find any extra info
in the marshaled output that I can use to add the change action
comment after the path. That's one downside of that change.

On Sat, Jun 24, 2017 at 1:49 PM, Luke Diamand <l...@diamand.org> wrote:
> On 22 June 2017 at 18:32, Junio C Hamano <gits...@pobox.com> wrote:
>> Miguel Torroja <miguel.torr...@gmail.com> writes:
>>
>>> The option -G of p4 (python marshal output) gives more context about the
>>> data being output. That's useful when using the command "change -o" as
>>> we can distinguish between warning/error line and real change description.
>>>
>>> Some p4 plugin/hooks in the server side generates some warnings when
>>> executed. Unfortunately those messages are mixed with the output of
>>> "p4 change -o". Those extra warning lines are reported as {'code':'info'}
>>> in python marshal output (-G). The real change output is reported as
>>> {'code':'stat'}
>
> I think this seems like a reasonable thing to do if "p4 change -o" is
> jumbling up output.
>
> One thing I notice trying it out by hand is that we seem to have lost
> the annotation of the Perforce per-file modification type (is there a
> proper name for this?).
>
> For example, if I add a file called "baz", then the original version
> creates a template which looks like this:
>
>//depot/baz# add
>
> But the new one creates a template which looks like:
>
>//depot/baz
>
> Luke

[PATCH] git-p4: changelist template with p4 -G change -o

[Miguel Torroja]

The option -G of p4 (python marshal output) gives more context about the
data being output. That's useful when using the command "change -o" as
we can distinguish between warning/error line and real change description.

Some p4 plugin/hooks in the server side generates some warnings when
executed. Unfortunately those messages are mixed with the output of
"p4 change -o". Those extra warning lines are reported as {'code':'info'}
in python marshal output (-G). The real change output is reported as
{'code':'stat'}

Signed-off-by: Miguel Torroja <miguel.torr...@gmail.com>
---
 git-p4.py | 77 ++-
 1 file changed, 51 insertions(+), 26 deletions(-)

diff --git a/git-p4.py b/git-p4.py
index 8d151da..a300474 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -1526,37 +1526,62 @@ class P4Submit(Command, P4UserMap):
 
 [upstream, settings] = findUpstreamBranchPoint()
 
-template = ""
+template = """\
+# A Perforce Change Specification.
+#
+#  Change:  The change number. 'new' on a new changelist.
+#  Date:The date this specification was last modified.
+#  Client:  The client on which the changelist was created.  Read-only.
+#  User:The user who created the changelist.
+#  Status:  Either 'pending' or 'submitted'. Read-only.
+#  Type:Either 'public' or 'restricted'. Default is 'public'.
+#  Description: Comments about the changelist.  Required.
+#  Jobs:What opened jobs are to be closed by this changelist.
+#   You may delete jobs from this list.  (New changelists only.)
+#  Files:   What opened files from the default changelist are to be added
+#   to this changelist.  You may delete files from this list.
+#   (New changelists only.)
+"""
+files_list = []
 inFilesSection = False
+change_entry = None
 args = ['change', '-o']
 if changelist:
 args.append(str(changelist))
-
-for line in p4_read_pipe_lines(args):
-if line.endswith("\r\n"):
-line = line[:-2] + "\n"
-if inFilesSection:
-if line.startswith("\t"):
-# path starts and ends with a tab
-path = line[1:]
-lastTab = path.rfind("\t")
-if lastTab != -1:
-path = path[:lastTab]
-if settings.has_key('depot-paths'):
-if not [p for p in settings['depot-paths']
-if p4PathStartsWith(path, p)]:
-continue
-else:
-if not p4PathStartsWith(path, self.depotPath):
-continue
+for entry in p4CmdList(args):
+if not entry.has_key('code'):
+continue
+if entry['code'] == 'stat':
+change_entry = entry
+break
+if not change_entry:
+die('Failed to decode output of p4 change -o')
+for key, value in change_entry.iteritems():
+if key.startswith('File'):
+if settings.has_key('depot-paths'):
+if not [p for p in settings['depot-paths']
+if p4PathStartsWith(value, p)]:
+continue
 else:
-inFilesSection = False
-else:
-if line.startswith("Files:"):
-inFilesSection = True
-
-template += line
-
+if not p4PathStartsWith(value, self.depotPath):
+continue
+files_list.append(value)
+continue
+# Output in the order expected by prepareLogMessage
+for key in ['Change','Client','User','Status','Description','Jobs']:
+if not change_entry.has_key(key):
+continue
+template += '\n'
+template += key + ':'
+if key == 'Description':
+template += '\n'
+for field_line in change_entry[key].splitlines():
+template += '\t'+field_line+'\n'
+if len(files_list) > 0:
+template += '\n'
+template += 'Files:\n'
+for path in files_list:
+template += '\t'+path+'\n'
 return template
 
 def edit_template(self, template_file):
-- 
2.1.4

Re: [PATCH] fast-export: deletion action first

[miguel torroja]

The previous patch reorders the delete operations in fast-export
(preceding any other one), keeps renaming as last operations to
process (as original source code) and for any other operation it keeps
the same order as "diff"

The non deterministic reordering was one of the concerns when I first
sent the patch.
The behavior for the other corner cases pointed out by Jeff
(delete/rename dir/file ) are not tackled in this patch and the final
result is unknown.


On Thu, May 4, 2017 at 9:36 PM, Miguel Torroja <miguel.torr...@gmail.com> wrote:
>
> The delete operations of the fast-export output should precede any addition
> belonging to the same commit, Addition and deletion with the same name
> entry could happen in case of file to directory and viceversa.
>
> As an equal comparison doesn't have any deterministic final order,
> it's better to keep original diff order input when there is no prefer order
> ( that's done comparing pointers)
>
> The fast-export sorting was added in 060df62 (fast-export: Fix output
> order of D/F changes). That change was made in order to fix the case of
> directory to file in the same commit, but it broke the reverse case
> (File to directory).
>
> The test "file becomes directory" has been added in order to exercise
> the original motivation of the deletion reorder.
>
> Signed-off-by: Miguel Torroja <miguel.torr...@gmail.com>
> ---
>  builtin/fast-export.c  | 32 +++-
>  t/t9350-fast-export.sh | 25 +
>  2 files changed, 40 insertions(+), 17 deletions(-)
>
> diff --git a/builtin/fast-export.c b/builtin/fast-export.c
> index e022063..e82f654 100644
> --- a/builtin/fast-export.c
> +++ b/builtin/fast-export.c
> @@ -260,26 +260,19 @@ static void export_blob(const struct object_id *oid)
> free(buf);
>  }
>
> -static int depth_first(const void *a_, const void *b_)
> +/*
> + * Compares two diff types to order based on output priorities.
> + */
> +static int diff_type_cmp(const void *a_, const void *b_)
>  {
> const struct diff_filepair *a = *((const struct diff_filepair **)a_);
> const struct diff_filepair *b = *((const struct diff_filepair **)b_);
> -   const char *name_a, *name_b;
> -   int len_a, len_b, len;
> int cmp;
>
> -   name_a = a->one ? a->one->path : a->two->path;
> -   name_b = b->one ? b->one->path : b->two->path;
> -
> -   len_a = strlen(name_a);
> -   len_b = strlen(name_b);
> -   len = (len_a < len_b) ? len_a : len_b;
> -
> -   /* strcmp will sort 'd' before 'd/e', we want 'd/e' before 'd' */
> -   cmp = memcmp(name_a, name_b, len);
> -   if (cmp)
> -   return cmp;
> -   cmp = len_b - len_a;
> +   /*
> +* Move Delete entries first so that an addition is always reported 
> after
> +*/
> +   cmp = (b->status == DIFF_STATUS_DELETED) - (a->status == 
> DIFF_STATUS_DELETED);
> if (cmp)
> return cmp;
> /*
> @@ -287,7 +280,12 @@ static int depth_first(const void *a_, const void *b_)
>  * appear in the output before it is renamed (e.g., when a file
>  * was copied and renamed in the same commit).
>  */
> -   return (a->status == 'R') - (b->status == 'R');
> +   cmp = (a->status == DIFF_STATUS_RENAMED) - (b->status == 
> DIFF_STATUS_RENAMED);
> +   if (cmp)
> +   return cmp;
> +
> +   /* For the remaining cases we keep the original ordering comparing 
> the pointers */
> +   return (a-b);
>  }
>
>  static void print_path_1(const char *path)
> @@ -347,7 +345,7 @@ static void show_filemodify(struct diff_queue_struct *q,
>  * Handle files below a directory first, in case they are all deleted
>  * and the directory changes to a file or symlink.
>  */
> -   QSORT(q->queue, q->nr, depth_first);
> +   QSORT(q->queue, q->nr, diff_type_cmp);
>
> for (i = 0; i < q->nr; i++) {
> struct diff_filespec *ospec = q->queue[i]->one;
> diff --git a/t/t9350-fast-export.sh b/t/t9350-fast-export.sh
> index b5149fd..d4f369a 100755
> --- a/t/t9350-fast-export.sh
> +++ b/t/t9350-fast-export.sh
> @@ -419,6 +419,31 @@ test_expect_success 'directory becomes symlink''
> (cd result && git show master:foo)
>  '
>
> +test_expect_success 'file becomes directory'  '
> +   git init filetodir_orig &&
> +   git init --bare filetodir_replica.git &&
> +   (
> +   cd filetodir_orig &&
> +   echo foo > filethendir

[PATCH] fast-export: deletion action first

[Miguel Torroja]

The delete operations of the fast-export output should precede any addition
belonging to the same commit, Addition and deletion with the same name
entry could happen in case of file to directory and viceversa.

As an equal comparison doesn't have any deterministic final order,
it's better to keep original diff order input when there is no prefer order
( that's done comparing pointers)

The fast-export sorting was added in 060df62 (fast-export: Fix output
order of D/F changes). That change was made in order to fix the case of
directory to file in the same commit, but it broke the reverse case
(File to directory).

The test "file becomes directory" has been added in order to exercise
the original motivation of the deletion reorder.

Signed-off-by: Miguel Torroja <miguel.torr...@gmail.com>
---
 builtin/fast-export.c  | 32 +++-
 t/t9350-fast-export.sh | 25 +
 2 files changed, 40 insertions(+), 17 deletions(-)

diff --git a/builtin/fast-export.c b/builtin/fast-export.c
index e022063..e82f654 100644
--- a/builtin/fast-export.c
+++ b/builtin/fast-export.c
@@ -260,26 +260,19 @@ static void export_blob(const struct object_id *oid)
free(buf);
 }
 
-static int depth_first(const void *a_, const void *b_)
+/*
+ * Compares two diff types to order based on output priorities.
+ */
+static int diff_type_cmp(const void *a_, const void *b_)
 {
const struct diff_filepair *a = *((const struct diff_filepair **)a_);
const struct diff_filepair *b = *((const struct diff_filepair **)b_);
-   const char *name_a, *name_b;
-   int len_a, len_b, len;
int cmp;
 
-   name_a = a->one ? a->one->path : a->two->path;
-   name_b = b->one ? b->one->path : b->two->path;
-
-   len_a = strlen(name_a);
-   len_b = strlen(name_b);
-   len = (len_a < len_b) ? len_a : len_b;
-
-   /* strcmp will sort 'd' before 'd/e', we want 'd/e' before 'd' */
-   cmp = memcmp(name_a, name_b, len);
-   if (cmp)
-   return cmp;
-   cmp = len_b - len_a;
+   /*
+* Move Delete entries first so that an addition is always reported 
after
+*/
+   cmp = (b->status == DIFF_STATUS_DELETED) - (a->status == 
DIFF_STATUS_DELETED);
if (cmp)
return cmp;
/*
@@ -287,7 +280,12 @@ static int depth_first(const void *a_, const void *b_)
 * appear in the output before it is renamed (e.g., when a file
 * was copied and renamed in the same commit).
 */
-   return (a->status == 'R') - (b->status == 'R');
+   cmp = (a->status == DIFF_STATUS_RENAMED) - (b->status == 
DIFF_STATUS_RENAMED);
+   if (cmp)
+   return cmp;
+
+   /* For the remaining cases we keep the original ordering comparing the 
pointers */
+   return (a-b);
 }
 
 static void print_path_1(const char *path)
@@ -347,7 +345,7 @@ static void show_filemodify(struct diff_queue_struct *q,
 * Handle files below a directory first, in case they are all deleted
 * and the directory changes to a file or symlink.
 */
-   QSORT(q->queue, q->nr, depth_first);
+   QSORT(q->queue, q->nr, diff_type_cmp);
 
for (i = 0; i < q->nr; i++) {
struct diff_filespec *ospec = q->queue[i]->one;
diff --git a/t/t9350-fast-export.sh b/t/t9350-fast-export.sh
index b5149fd..d4f369a 100755
--- a/t/t9350-fast-export.sh
+++ b/t/t9350-fast-export.sh
@@ -419,6 +419,31 @@ test_expect_success 'directory becomes symlink''
(cd result && git show master:foo)
 '
 
+test_expect_success 'file becomes directory'  '
+   git init filetodir_orig &&
+   git init --bare filetodir_replica.git &&
+   (
+   cd filetodir_orig &&
+   echo foo > filethendir &&
+   git add filethendir &&
+   test_tick &&
+   git commit -mfile &&
+   git rm filethendir &&
+   mkdir filethendir &&
+   echo bar > filethendir/a &&
+   git add filethendir/a &&
+   test_tick &&
+   git commit -mdir
+   ) &&
+   git --git-dir=filetodir_orig/.git fast-export master  |
+   git --git-dir=filetodir_replica.git/ fast-import &&
+   (
+   ORIG=$(git --git-dir=filetodir_orig/.git rev-parse --verify 
master) &&
+   REPLICA=$(git --git-dir=filetodir_replica.git rev-parse 
--verify master) &&
+   test $ORIG = $REPLICA
+   )
+'
+
 test_expect_success 'fast-export quotes pathnames' '
git init crazy-paths &&
(cd crazy-paths &&
-- 
2.1.4

[PATCH 2/2] fast-export: DIFF_STATUS_RENAMED instead of 'R'

[Miguel Torroja]

Minor change to be consistent with the rest of the fast-export code.
DIFF_STATUS_RENAMED is defined as 'R'.

Signed-off-by: Miguel Torroja <miguel.torr...@gmail.com>
---
 builtin/fast-export.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/builtin/fast-export.c b/builtin/fast-export.c
index a3ab7da..4d39324 100644
--- a/builtin/fast-export.c
+++ b/builtin/fast-export.c
@@ -280,7 +280,7 @@ static int diff_type_cmp(const void *a_, const void *b_)
 * appear in the output before it is renamed (e.g., when a file
 * was copied and renamed in the same commit).
 */
-   return (a->status == 'R') - (b->status == 'R');
+   return (a->status == DIFF_STATUS_RENAMED) - (b->status == 
DIFF_STATUS_RENAMED);
 }
 
 static void print_path_1(const char *path)
-- 
2.1.4

[PATCH 1/2] fast-export: deletion action first

[Miguel Torroja]

The delete operations of the fast-export output should precede any addition
belonging to the same commit, Addition and deletion with the same name
entry could happen in case of file to directory and viceversa.

The fast-export sorting was added in 060df62 (fast-export: Fix output
order of D/F changes). That change was made in order to fix the case of
directory to file in the same commit, but it broke the reverse case
(File to directory).

Signed-off-by: Miguel Torroja <miguel.torr...@gmail.com>
---
 builtin/fast-export.c | 25 +
 1 file changed, 9 insertions(+), 16 deletions(-)

diff --git a/builtin/fast-export.c b/builtin/fast-export.c
index e022063..a3ab7da 100644
--- a/builtin/fast-export.c
+++ b/builtin/fast-export.c
@@ -260,26 +260,19 @@ static void export_blob(const struct object_id *oid)
free(buf);
 }
 
-static int depth_first(const void *a_, const void *b_)
+/*
+ * Compares two diff types to order based on output priorities.
+ */
+static int diff_type_cmp(const void *a_, const void *b_)
 {
const struct diff_filepair *a = *((const struct diff_filepair **)a_);
const struct diff_filepair *b = *((const struct diff_filepair **)b_);
-   const char *name_a, *name_b;
-   int len_a, len_b, len;
int cmp;
 
-   name_a = a->one ? a->one->path : a->two->path;
-   name_b = b->one ? b->one->path : b->two->path;
-
-   len_a = strlen(name_a);
-   len_b = strlen(name_b);
-   len = (len_a < len_b) ? len_a : len_b;
-
-   /* strcmp will sort 'd' before 'd/e', we want 'd/e' before 'd' */
-   cmp = memcmp(name_a, name_b, len);
-   if (cmp)
-   return cmp;
-   cmp = len_b - len_a;
+   /*
+* Move Delete entries first so that an addition is always reported 
after
+*/
+   cmp = (b->status == DIFF_STATUS_DELETED) - (a->status == 
DIFF_STATUS_DELETED);
if (cmp)
return cmp;
/*
@@ -347,7 +340,7 @@ static void show_filemodify(struct diff_queue_struct *q,
 * Handle files below a directory first, in case they are all deleted
 * and the directory changes to a file or symlink.
 */
-   QSORT(q->queue, q->nr, depth_first);
+   QSORT(q->queue, q->nr, diff_type_cmp);
 
for (i = 0; i < q->nr; i++) {
struct diff_filespec *ospec = q->queue[i]->one;
-- 
2.1.4

[PATCH 2/2] fast-export: DIFF_STATUS_RENAMED instead of 'R'

[Miguel Torroja]

Minor change to be consistent with the rest of the fast-export code.
DIFF_STATUS_RENAMED is defined as 'R'.

Signed-off-by: Miguel Torroja <miguel.torr...@gmail.com>
---
 builtin/fast-export.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/builtin/fast-export.c b/builtin/fast-export.c
index a3ab7da..4d39324 100644
--- a/builtin/fast-export.c
+++ b/builtin/fast-export.c
@@ -280,7 +280,7 @@ static int diff_type_cmp(const void *a_, const void *b_)
 * appear in the output before it is renamed (e.g., when a file
 * was copied and renamed in the same commit).
 */
-   return (a->status == 'R') - (b->status == 'R');
+   return (a->status == DIFF_STATUS_RENAMED) - (b->status == 
DIFF_STATUS_RENAMED);
 }
 
 static void print_path_1(const char *path)
-- 
2.1.4

[PATCH 1/2] fast-export: deletion action first

[Miguel Torroja]

The delete operations of the fast-export output should precede any addition
belonging to the same commit, Addition and deletion with the same name
entry could happen in case of file to directory and viceversa.

The fast-export sorting was added in 060df62 (fast-export: Fix output
order of D/F changes). That change was made in order to fix the case of
directory to file in the same commit, but it broke the reverse case
(File to directory).

Signed-off-by: Miguel Torroja <miguel.torr...@gmail.com>
---
 builtin/fast-export.c | 25 +
 1 file changed, 9 insertions(+), 16 deletions(-)

diff --git a/builtin/fast-export.c b/builtin/fast-export.c
index e022063..a3ab7da 100644
--- a/builtin/fast-export.c
+++ b/builtin/fast-export.c
@@ -260,26 +260,19 @@ static void export_blob(const struct object_id *oid)
free(buf);
 }
 
-static int depth_first(const void *a_, const void *b_)
+/*
+ * Compares two diff types to order based on output priorities.
+ */
+static int diff_type_cmp(const void *a_, const void *b_)
 {
const struct diff_filepair *a = *((const struct diff_filepair **)a_);
const struct diff_filepair *b = *((const struct diff_filepair **)b_);
-   const char *name_a, *name_b;
-   int len_a, len_b, len;
int cmp;
 
-   name_a = a->one ? a->one->path : a->two->path;
-   name_b = b->one ? b->one->path : b->two->path;
-
-   len_a = strlen(name_a);
-   len_b = strlen(name_b);
-   len = (len_a < len_b) ? len_a : len_b;
-
-   /* strcmp will sort 'd' before 'd/e', we want 'd/e' before 'd' */
-   cmp = memcmp(name_a, name_b, len);
-   if (cmp)
-   return cmp;
-   cmp = len_b - len_a;
+   /*
+* Move Delete entries first so that an addition is always reported 
after
+*/
+   cmp = (b->status == DIFF_STATUS_DELETED) - (a->status == 
DIFF_STATUS_DELETED);
if (cmp)
return cmp;
/*
@@ -347,7 +340,7 @@ static void show_filemodify(struct diff_queue_struct *q,
 * Handle files below a directory first, in case they are all deleted
 * and the directory changes to a file or symlink.
 */
-   QSORT(q->queue, q->nr, depth_first);
+   QSORT(q->queue, q->nr, diff_type_cmp);
 
for (i = 0; i < q->nr; i++) {
struct diff_filespec *ospec = q->queue[i]->one;
-- 
2.1.4

[PATCH 2/2] fast-export: DIFF_STATUS_RENAMED instead of 'R'

[Miguel Torroja]

Minor change to be consistent with the rest of the fast-export code.
DIFF_STATUS_RENAMED is defined as 'R'.

Signed-off-by: Miguel Torroja <miguel.torr...@gmail.com>
---
 builtin/fast-export.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/builtin/fast-export.c b/builtin/fast-export.c
index a3ab7da..4d39324 100644
--- a/builtin/fast-export.c
+++ b/builtin/fast-export.c
@@ -280,7 +280,7 @@ static int diff_type_cmp(const void *a_, const void *b_)
 * appear in the output before it is renamed (e.g., when a file
 * was copied and renamed in the same commit).
 */
-   return (a->status == 'R') - (b->status == 'R');
+   return (a->status == DIFF_STATUS_RENAMED) - (b->status == 
DIFF_STATUS_RENAMED);
 }
 
 static void print_path_1(const char *path)
-- 
2.1.4

[PATCH 1/2] fast-export: deletion action first

[Miguel Torroja]

The delete operations of the fast-export output should precede any addition
belonging to the same commit, Addition and deletion with the same name
entry could happen in case of file to directory and viceversa.

The fast-export sorting was added in 060df62 (fast-export: Fix output
order of D/F changes). That change was made in order to fix the case of
directory to file in the same commit, but it broke the reverse case
(File to directory).

Signed-off-by: Miguel Torroja <miguel.torr...@gmail.com>
---
 builtin/fast-export.c | 25 +
 1 file changed, 9 insertions(+), 16 deletions(-)

diff --git a/builtin/fast-export.c b/builtin/fast-export.c
index e022063..a3ab7da 100644
--- a/builtin/fast-export.c
+++ b/builtin/fast-export.c
@@ -260,26 +260,19 @@ static void export_blob(const struct object_id *oid)
free(buf);
 }
 
-static int depth_first(const void *a_, const void *b_)
+/*
+ * Compares two diff types to order based on output priorities.
+ */
+static int diff_type_cmp(const void *a_, const void *b_)
 {
const struct diff_filepair *a = *((const struct diff_filepair **)a_);
const struct diff_filepair *b = *((const struct diff_filepair **)b_);
-   const char *name_a, *name_b;
-   int len_a, len_b, len;
int cmp;
 
-   name_a = a->one ? a->one->path : a->two->path;
-   name_b = b->one ? b->one->path : b->two->path;
-
-   len_a = strlen(name_a);
-   len_b = strlen(name_b);
-   len = (len_a < len_b) ? len_a : len_b;
-
-   /* strcmp will sort 'd' before 'd/e', we want 'd/e' before 'd' */
-   cmp = memcmp(name_a, name_b, len);
-   if (cmp)
-   return cmp;
-   cmp = len_b - len_a;
+   /*
+* Move Delete entries first so that an addition is always reported 
after
+*/
+   cmp = (b->status == DIFF_STATUS_DELETED) - (a->status == 
DIFF_STATUS_DELETED);
if (cmp)
return cmp;
/*
@@ -347,7 +340,7 @@ static void show_filemodify(struct diff_queue_struct *q,
 * Handle files below a directory first, in case they are all deleted
 * and the directory changes to a file or symlink.
 */
-   QSORT(q->queue, q->nr, depth_first);
+   QSORT(q->queue, q->nr, diff_type_cmp);
 
for (i = 0; i < q->nr; i++) {
struct diff_filespec *ospec = q->queue[i]->one;
-- 
2.1.4

[PATCH] p4: Retrieve the right revision of the UTF-16 file

[Miguel Torroja]

Fixing bug with UTF-16 files when they are retreived by git-p4.
It was always getting the tip version of the file and the history of the
file was lost.
---
 git-p4.py |2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/git-p4.py b/git-p4.py
index cdfa2df..be2c7da 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -2098,7 +2098,7 @@ class P4Sync(Command, P4UserMap):
 # them back too.  This is not needed to the cygwin windows version,
 # just the native NT type.
 #
-text = p4_read_pipe(['print', '-q', '-o', '-', file['depotFile']])
+text = p4_read_pipe(['print', '-q', '-o', '-', %s@%s % 
(file['depotFile'], file['change']) ])
 if p4_version_string().find(/NT) = 0:
 text = text.replace(\r\n, \n)
 contents = [ text ]
-- 
1.7.10.4

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

[PATCH] p4: Retrieve the right revision of the UTF-16 file

[Miguel Torroja]

Fixing bug with UTF-16 files when they are retrieved by git-p4.
It was always getting the tip version of the file and the history of the
file was lost.

Signed-off-by: Miguel Torroja miguel.torr...@gmail.com
---
 git-p4.py |2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/git-p4.py b/git-p4.py
index cdfa2df..be2c7da 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -2098,7 +2098,7 @@ class P4Sync(Command, P4UserMap):
 # them back too.  This is not needed to the cygwin windows version,
 # just the native NT type.
 #
-text = p4_read_pipe(['print', '-q', '-o', '-', file['depotFile']])
+text = p4_read_pipe(['print', '-q', '-o', '-', %s@%s % 
(file['depotFile'], file['change']) ])
 if p4_version_string().find(/NT) = 0:
 text = text.replace(\r\n, \n)
 contents = [ text ]
-- 
1.7.10.4

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