[PATCH 2/2] git-p4 tests: work with python3 as well as python2

[Luke Diamand]

Update the git-p4 tests so that they work with both
Python2 and Python3.

We have to be explicit about the difference between
Unicode text strings (Python3 default) and raw binary
strings which will be exchanged with Perforce.

Additionally, print always takes braces in Python3.

Signed-off-by: Luke Diamand <l...@diamand.org>
---
 t/lib-git-p4.sh| 5 +++--
 t/t9802-git-p4-filetype.sh | 6 +++---
 2 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/t/lib-git-p4.sh b/t/lib-git-p4.sh
index 77802fe..b97d27c 100644
--- a/t/lib-git-p4.sh
+++ b/t/lib-git-p4.sh
@@ -198,9 +198,10 @@ marshal_dump() {
cat >"$TRASH_DIRECTORY/marshal-dump.py" <<-EOF &&
import marshal
import sys
+   instream = getattr(sys.stdin, 'buffer', sys.stdin)
for i in range($line):
-   d = marshal.load(sys.stdin)
-   print d['$what']
+   d = marshal.load(instream)
+   print(d[b'$what'].decode('utf-8'))
EOF
"$PYTHON_PATH" "$TRASH_DIRECTORY/marshal-dump.py"
 }
diff --git a/t/t9802-git-p4-filetype.sh b/t/t9802-git-p4-filetype.sh
index 66d3fc9..eb9a8ed 100755
--- a/t/t9802-git-p4-filetype.sh
+++ b/t/t9802-git-p4-filetype.sh
@@ -223,12 +223,12 @@ build_gendouble() {
import sys
import struct
 
-   s = struct.pack(">LL18s",
+   s = struct.pack(b">LL18s",
0x00051607,  # AppleDouble
0x0002,  # version 2
-   ""   # pad to 26 bytes
+   b""  # pad to 26 bytes
)
-   sys.stdout.write(s)
+   getattr(sys.stdout, 'buffer', sys.stdout).write(s)
EOF
 }
 
-- 
2.8.1.218.gd2cea43.dirty

--
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 1/2] git-p4 tests: cd to testdir before running python

[Luke Diamand]

The python one-liner for getting the current time prints out
error messages if the current directory is deleted while it is
running if using python3.

Avoid these messages by switching back to the test directory
before running python3, instead of remaining in the trash
directory.

Signed-off-by: Luke Diamand <l...@diamand.org>
---
 t/lib-git-p4.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/lib-git-p4.sh b/t/lib-git-p4.sh
index f9ae1d7..77802fe 100644
--- a/t/lib-git-p4.sh
+++ b/t/lib-git-p4.sh
@@ -50,7 +50,7 @@ native_path() {
 # at runtime (e.g. via NTP). The 'clock_gettime(CLOCK_MONOTONIC)'
 # function could fix that but it is not in Python until 3.3.
 time_in_seconds() {
-   python -c 'import time; print int(time.time())'
+   (cd "$TEST_DIRECTORY" && python -c 'import time; 
print(int(time.time()))')
 }
 
 # Try to pick a unique port: guess a large number, then hope
-- 
2.8.1.218.gd2cea43.dirty

--
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 0/2] git-p4: support python3 in the tests

[Luke Diamand]

This patchset updates the git-p4 tests so that they work with
either Python2 or Python3.

Note that this does *not* fix git-p4 to work with Python3 - that's
a much bigger challenge.

Luke Diamand (2):
  git-p4 tests: cd to testdir before running python
  git-p4 tests: work with python3 as well as python2

 t/lib-git-p4.sh| 7 ---
 t/t9802-git-p4-filetype.sh | 6 +++---
 2 files changed, 7 insertions(+), 6 deletions(-)

-- 
2.8.1.218.gd2cea43.dirty

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

Re: [PATCH] t9824: fix wrong reference value

[Luke Diamand]

On 29 April 2016 at 21:29, Lars Schneider  wrote:
>
> On 29 Apr 2016, at 19:34, Junio C Hamano  wrote:
>
>> Lars Schneider  writes:
>>
>>> 0492eb4 fixed a broken &&-chain in this test which broke the test as it
>>> checked for a wrong size. The expected size of the file under test is
>>> 39 bytes. The test checked that the size is 13 bytes. Fix the reference
>>> value to make the test pass, again.
>>>
>>> Signed-off-by: Lars Schneider 
>>> ---
>>
>> That breaking "fix" seems to have been acked by you.
>>
>> It was sort of clear that SZEDER didn't actually ran the test from
>> the patch, saying "As far as I can tell after eyeballing the test
>> script,", but you obviously didn't actually have a chance to test it
>> until now.
>>
>> Thanks for fixing it before it hits 'master'; this time I think it
>> is safe to assume that this was actually tested ;-)
>
> Yes! Lesson learned! Sorry Szeder!

How does the old saying go? If it hasn't been tested, it doesn't work!

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

Re: [PATCH 0/2] git-p4: support python3 in the tests

[Luke Diamand]

On 25 April 2016 at 23:07, Junio C Hamano <gits...@pobox.com> wrote:
> Luke Diamand <l...@diamand.org> writes:
>
>> This patchset updates the git-p4 tests so that they work with
>> either Python2 or Python3.
>>
>> Note that this does *not* fix git-p4 to work with Python3 - that's
>> a much bigger challenge.
>
> We use Python outside p4 tests (e.g. remote-svn test), and the way
> they invoke the interpreter is to say "$PYTHON_PATH" and avoid
> saying "python" which picks whatever random version of Python
> interpreter happens to be the first on $PATH.  Shouldn't the tests
> touched by this series be doing the same?

Yes, they should. I'll update them accordingly.

But the real reason for doing this is that at some point, git-p4 has
to start working with python3, since python2 is going away (albeit not
until 2020).

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

[PATCHv2 3/3] git-p4 tests: time_in_seconds should use $PYTHON_PATH

[Luke Diamand]

The time_in_seconds script should use $PYTHON_PATH, rather than
just hard-coded python, so that users can override which version
gets used, as is done for other python invocations.

Signed-off-by: Luke Diamand <l...@diamand.org>
---
 t/lib-git-p4.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/lib-git-p4.sh b/t/lib-git-p4.sh
index 7393ee2..012d40e 100644
--- a/t/lib-git-p4.sh
+++ b/t/lib-git-p4.sh
@@ -50,7 +50,7 @@ native_path() {
 # at runtime (e.g. via NTP). The 'clock_gettime(CLOCK_MONOTONIC)'
 # function could fix that but it is not in Python until 3.3.
 time_in_seconds() {
-   (cd / && python -c 'import time; print(int(time.time()))')
+   (cd / && "$PYTHON_PATH" -c 'import time; print(int(time.time()))')
 }
 
 # Try to pick a unique port: guess a large number, then hope
-- 
2.8.1.218.gd2cea43.dirty

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

[PATCHv2 2/3] git-p4 tests: work with python3 as well as python2

[Luke Diamand]

Update the git-p4 tests so that they work with both
Python2 and Python3.

We have to be explicit about the difference between
Unicode text strings (Python3 default) and raw binary
strings which will be exchanged with Perforce.

Additionally, print always takes braces in Python3.

Signed-off-by: Luke Diamand <l...@diamand.org>
---
 t/lib-git-p4.sh| 5 +++--
 t/t9802-git-p4-filetype.sh | 6 +++---
 2 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/t/lib-git-p4.sh b/t/lib-git-p4.sh
index 724bc43..7393ee2 100644
--- a/t/lib-git-p4.sh
+++ b/t/lib-git-p4.sh
@@ -198,9 +198,10 @@ marshal_dump() {
cat >"$TRASH_DIRECTORY/marshal-dump.py" <<-EOF &&
import marshal
import sys
+   instream = getattr(sys.stdin, 'buffer', sys.stdin)
for i in range($line):
-   d = marshal.load(sys.stdin)
-   print d['$what']
+   d = marshal.load(instream)
+   print(d[b'$what'].decode('utf-8'))
EOF
"$PYTHON_PATH" "$TRASH_DIRECTORY/marshal-dump.py"
 }
diff --git a/t/t9802-git-p4-filetype.sh b/t/t9802-git-p4-filetype.sh
index 66d3fc9..eb9a8ed 100755
--- a/t/t9802-git-p4-filetype.sh
+++ b/t/t9802-git-p4-filetype.sh
@@ -223,12 +223,12 @@ build_gendouble() {
import sys
import struct
 
-   s = struct.pack(">LL18s",
+   s = struct.pack(b">LL18s",
0x00051607,  # AppleDouble
0x0002,  # version 2
-   ""   # pad to 26 bytes
+   b""  # pad to 26 bytes
)
-   sys.stdout.write(s)
+   getattr(sys.stdout, 'buffer', sys.stdout).write(s)
EOF
 }
 
-- 
2.8.1.218.gd2cea43.dirty

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

[PATCHv2 0/3] git-p4: support python3 in the tests

[Luke Diamand]

Updates to my patches to allow the git-p4 tests to work with python3.

Incorporates suggestions from Junio to just switch to "/" and to
use $PYTHON_PATH.

Luke Diamand (3):
  git-p4 tests: cd to / before running python
  git-p4 tests: work with python3 as well as python2
  git-p4 tests: time_in_seconds should use $PYTHON_PATH

 t/lib-git-p4.sh| 7 ---
 t/t9802-git-p4-filetype.sh | 6 +++---
 2 files changed, 7 insertions(+), 6 deletions(-)

-- 
2.8.1.218.gd2cea43.dirty

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

[PATCHv2 1/3] git-p4 tests: cd to / before running python

[Luke Diamand]

The python one-liner for getting the current time prints out
error messages if the current directory is deleted while it is
running if using python3.

Avoid these messages by switching to "/" before running it.

This problem does not arise if using python2.

Signed-off-by: Luke Diamand <l...@diamand.org>
---
 t/lib-git-p4.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/lib-git-p4.sh b/t/lib-git-p4.sh
index f9ae1d7..724bc43 100644
--- a/t/lib-git-p4.sh
+++ b/t/lib-git-p4.sh
@@ -50,7 +50,7 @@ native_path() {
 # at runtime (e.g. via NTP). The 'clock_gettime(CLOCK_MONOTONIC)'
 # function could fix that but it is not in Python until 3.3.
 time_in_seconds() {
-   python -c 'import time; print int(time.time())'
+   (cd / && python -c 'import time; print(int(time.time()))')
 }
 
 # Try to pick a unique port: guess a large number, then hope
-- 
2.8.1.218.gd2cea43.dirty

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

Re: [BUG] t9801 and t9803 broken on next

[Luke Diamand]

On 14 May 2016 at 19:15, Junio C Hamano  wrote:
> Lars Schneider  writes:
>
>>> On 13 May 2016, at 18:37, Junio C Hamano  wrote:
>>>
>>> Are you saying that "git p4" itself breaks unless fast-import always
>>> writes a new (tiny) packfile?  That sounds quite broken, and setting
>>> unpacklimit to 0 does not sound like a sensible "fix".  Of course,
>>> if the test script is somehow looking at the number of packs or
>>> loose objects and declaring a failure, even when the resulting
>>> history in p4 and git are correct, then that is a different issue,
>>> and forcing to explode a tiny pack is a reasonable workaround.  I
>>> couldn't quite tell which the case is.
>>>
>>> Puzzled.  Please help.
>>
>> t9801 "import depot, branch detection" is the first test that fails
>> with a fast import error:
>> https://github.com/git/git/blob/78b384c29366e199741393e56030a8384110760d/t/t9801-git-p4-branch.sh#L110
>>
>> fast-import crash report:
>> fast-import process: 77079
>> parent process : 77077
>> at 2016-05-14 07:48:40 +
>>
>> fatal: offset beyond end of packfile (truncated pack?)
>
> Hmm, does that suggest Eric's "explode them into loose instead of
> keeping a small pack" insufficient?  It sounds like that somebody
> wanted to read some data back from its packfile without knowing that
> the updated code may make it available in a loose object form, which
> would mean that somebody needs to be told about what is going on,
> namely, these objects are not in a half-written pack but are found
> as loose objects.
>
>> The problem seems to be related to the git-p4 branch import.
>> I don't have time this weekend to look further into it, but
>> I will try next week.

The last thing that fast-import is sent before it crashes is this:

checkpoint
  progress checkpoint
  commit refs/remotes/p4/depot/branch2
  mark :6
  committer Dr. author  1463212118 +
  data 

Re: [PATCHv1] git-p4: workaround p4 removal of client directory

[Luke Diamand]

[+Git, Junio]

On 29 April 2016 at 15:10, Jacob Smith <jaros...@gmail.com> wrote:
> That's the identical patch I'm maintaining internally; it appears to work.

Thanks, I think that counts as a "Tested-by" then.

Luke


>
> Sent from my iPhone
>
>> On Apr 29, 2016, at 8:52 AM, Luke Diamand <l...@diamand.org> wrote:
>>
>>> On 29 April 2016 at 13:34, Jacob Smith <jaros...@gmail.com> wrote:
>>> Thank you, Luke! I'll raise a radar so (hopefully) it gets integrated into 
>>> Apple's distribution.
>>
>> Can you check if it works for you? I can't reproduce it here.
>>
>> Thanks!
>> Luke
>>
>>
>>
>>
>>>
>>>> On Apr 29, 2016, at 2:39 AM, Luke Diamand <l...@diamand.org> wrote:
>>>>
>>>> Adding correct email for Jacob.
>>>>
>>>>> On 29 April 2016 at 08:40, Luke Diamand <l...@diamand.org> wrote:
>>>>> On some platforms, "p4 sync -f" will remove the workspace
>>>>> directory after we have just created it; on some it won't.
>>>>> This causes problems later when git finds itself in an
>>>>> orphaned directory.
>>>>>
>>>>> Workaround this by cd'ing back to the directory after
>>>>> the "p4 sync -f".
>>>>>
>>>>> Signed-off-by: Luke Diamand <l...@diamand.org>
>>>>> ---
>>>>> git-p4.py | 5 -
>>>>> 1 file changed, 4 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/git-p4.py b/git-p4.py
>>>>> index 527d44b..2b75a61 100755
>>>>> --- a/git-p4.py
>>>>> +++ b/git-p4.py
>>>>> @@ -1,4 +1,4 @@
>>>>> -#!/usr/bin/env python
>>>>> +#!/usr/bin/env python2
>>>>> #
>>>>> # git-p4.py -- A tool for bidirectional operation between a Perforce 
>>>>> depot and git.
>>>>> #
>>>>> @@ -1956,6 +1956,9 @@ class P4Submit(Command, P4UserMap):
>>>>>if new_client_dir:
>>>>># old one was destroyed, and maybe nobody told p4
>>>>>p4_sync("...", "-f")
>>>>> +
>>>>> +# sometimes p4 will unlink the directory and recreate it
>>>>> +chdir(self.clientPath, is_client_path=True)
>>>>>else:
>>>>>p4_sync("...")
>>>>>self.check()
>>>>> --
>>>>> 2.8.1.218.gd2cea43.dirty
>>>>>
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] Adding list of p4 jobs to git commit message

[Luke Diamand]

On 15 April 2016 at 21:27, Junio C Hamano  wrote:
> Jan Durovec  writes:
>
>> ---
>
> A few issues.  Please:
>
>  (1) Sign-off your work.
>
>  (2) Try to find those who are familiar with the area and Cc them.
>
>  "git shortlog -s -n --since=18.months --no-merges git-p4.py"
>  may help.
>
>  (3) Follow the style of existing commits when giving a title to
>  your patch.
>
>  "git shortlog --since=18.months --no-merges git-p4.py" may
>  help you notice "git-p4: do this thing" is the common way to
>  title "git p4" patches.
>
>  (4) Justify why your change is a good thing in your log message.
>  What you did, i.e. "list p4 jobs when making a commit", can be
>  seen by the patch, but readers cannot guess why you thought it
>  is a good idea to extract "job%d" out of the P4 commit and to
>  record them in the resulting Git commit, unless you explain
>  things like:
>
>  - what goes wrong if you don't?
>  - when would "job%d" appear in P4 commit?
>  - is it sane to assume "job0", "job1",... appear consecutively?
>
>  (5) Describe what your change does clearly.  "Adding list" is not
>  quite clear.  Where in the "git commit message" are you adding
>  the list, and why is that location in the message the most
>  appropriate place to add it?

Additionally, it would be very useful to add a test case (see t/t98*).
There are quite a few test cases for git-p4, and they make maintenance
a lot easier.

Some documentation (Documentation/git-p4.txt) would also be useful.

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

Re: [PATCH v2] git-p4: add P4 jobs to git commit message

[Luke Diamand]

On 19 April 2016 at 22:44, Jan Durovec  wrote:
>> By the way, you may or may not have noticed that I've been
>> reordering the lines of your message quoted in my responses; around
>> here, top-posting is frowned upon.
>
> I haven't noticed. Thanks for pointing out.
>
> As for the submitGit cover letter I wanted to raise at least an issue
> (if not create a fix itself) but it seems to be raised already as
> https://github.com/rtyley/submitgit/issues/9

To be honest, it would be just as easy to learn how to use git
format-patch and git send-email. It makes your head hurt a bit the
first few times you use it, but it's pretty straightforward.

And it also means that if there's a zombie apocalypse, and github gets
overrun, you can still exchange patches to your anti-virus and save
humanity from extinction.

Of course, if you don't care about the future of the human race, then
that's up to you :-)

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

Re: [PATCH v2] git-p4: add P4 jobs to git commit message

[Luke Diamand]

On 19 April 2016 at 22:39, Junio C Hamano  wrote:
> Jan Durovec  writes:
>
>> On Tue, Apr 19, 2016 at 11:09 PM, Junio C Hamano  wrote:
>>
>>> For a series this small it does not matter, but anything longer it
>>> would be easier to review with a cover letter (i.e. [PATCH 0/N]).  I
>>> do not know if submitGit lets us do that, though.
>>
>> There's a comment on PR itself (in addition to individual commits) so
>> theoretically it could.
>>
>> It seems that for [PATCH ... n/m] e-mails the commit messages are used,
>> so there's no reason why the PR comment couldn't be used for a cover
>> letter.
>>
>> In this case the PR comment was the same as for one of the commits.
>> Maybe submitGit tried to be smart there? Or maybe it just really
>> doesn't support it (yet?).
>
> In any case, I've queued the updated ones as they looked good.
> Thanks.

One thing I wondered about is whether this should be enabled by
default or not. Long-time users of git-p4 might be a bit surprised to
find their git commits suddenly gaining an extra Job: field.

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

Re: [PATCH v2] git-p4: add P4 jobs to git commit message

[Luke Diamand]

On 19 April 2016 at 02:15, Junio C Hamano  wrote:
> Jan Durovec  writes:
>
>> When migrating from Perforce to git the information about P4 jobs
>> associated with P4 changelists is lost.
>>
>> Having these jobs listed on messages of related git commits enables smooth
>> migration for projects that take advantage of e.g. JIRA integration
>> (which uses jobs on Perforce side and parses commit messages on git side).
>>
>> The jobs are added to the message in the same format as is expected when
>> migrating in the reverse direction.
>>
>> Signed-off-by: Jan Durovec 
>> ---
>
> Thanks for describing the change more throughly than the previous
> round.
>
> Luke, how does this one look?
>
>>  git-p4.py  | 12 ++
>>  t/lib-git-p4.sh| 10 +
>>  t/t9829-git-p4-jobs.sh | 99 
>> ++
>>  3 files changed, 121 insertions(+)
>>  create mode 100755 t/t9829-git-p4-jobs.sh
>>
>> diff --git a/git-p4.py b/git-p4.py
>> index 527d44b..8f869d7 100755
>> --- a/git-p4.py
>> +++ b/git-p4.py
>> @@ -2320,6 +2320,15 @@ def extractFilesFromCommit(self, commit):
>>  fnum = fnum + 1
>>  return files
>>
>> +def extractJobsFromCommit(self, commit):
>> +jobs = []
>> +jnum = 0
>> +while commit.has_key("job%s" % jnum):
>> +job = commit["job%s" % jnum]
>> +jobs.append(job)
>> +jnum = jnum + 1
>
> I am not familiar with "Perforce jobs", but I assume that they are
> always named as "job" + small non-negative integer in a dense way
> and it is OK for this loop to always begin at 0 and immediately stop
> when job + num does not exist (i.e. if job7 is missing, it is
> guaranteed that we will not see job8).

This is OK - P4 jobs have arbitrary names, but this code is just
extracting an array of them from the commit by index.

>
> Shouldn't the formatting be "job%d" % jnum, though, as you are using
> jnum as a number that begins at 0 and increments by 1?

Python seems to handle this by turning jnum into a string.

>
>> +return jobs
>> +
>>  def stripRepoPath(self, path, prefixes):
>>  """When streaming files, this is called to map a p4 depot path
>> to where it should go in git.  The prefixes are either
>> @@ -2665,6 +2674,7 @@ def hasBranchPrefix(self, path):
>>  def commit(self, details, files, branch, parent = ""):
>>  epoch = details["time"]
>>  author = details["user"]
>> +jobs = self.extractJobsFromCommit(details)
>>
>>  if self.verbose:
>>  print('commit into {0}'.format(branch))
>> @@ -2692,6 +2702,8 @@ def commit(self, details, files, branch, parent = ""):
>>
>>  self.gitStream.write("data <>  self.gitStream.write(details["desc"])
>> +if len(jobs) > 0:
>> +self.gitStream.write("\nJobs: %s" % (' '.join(jobs)))
>>  self.gitStream.write("\n[git-p4: depot-paths = \"%s\": change = %s" 
>> %
>>   (','.join(self.branchPrefixes), 
>> details["change"]))
>>  if len(details['options']) > 0:
>> diff --git a/t/lib-git-p4.sh b/t/lib-git-p4.sh
>> index f9ae1d7..3907560 100644
>> --- a/t/lib-git-p4.sh
>> +++ b/t/lib-git-p4.sh
>> @@ -160,6 +160,16 @@ p4_add_user() {
>>   EOF
>>  }
>>
>> +p4_add_job() {
>
> Not a new problem in this script, but we'd prefer to spell this as
>
> p4_add_job () {
>
> i.e. a space on both sides of ().
>
>> + name=$1 &&
>> + p4 job -f -i <<-EOF
>> + Job: $name
>> + Status: open
>> + User: dummy
>> + Description:
>> + EOF
>> +}
>
> It may be better without $name?
>
>> +test_expect_success 'check log message of changelist with no jobs' '
>> + client_view "//depot/... //client/..." &&
>> + test_when_finished cleanup_git &&
>> + (
>> + cd "$git" &&
>> + git init . &&
>> + git p4 clone --use-client-spec --destination="$git" 
>> //depot@all &&
>> + cat >expect <<-\EOF &&
>> +Add file 1
>> +[git-p4: depot-paths = "//depot/": change = 1]
>> +
>> + EOF
>
> As you are using <<- to begin the here document, it is easier on the
> eyes if you indented the text with HT, i.e.
>
> cat >expect <<-\EOF &&
> Add file 1
> [git-p4: depot-paths = "//depot/": change = 1]
>
> EOF
>
> I won't repeat the same for other instances below.
>
> Thanks.

Modulo Junio's other comments, this seems fine to me. I tried it out
on a scratch repo and it works very nicely, thanks!

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

Re: [PATCH v2] git-p4: add P4 jobs to git commit message

[Luke Diamand]

On 20 April 2016 at 16:51, Junio C Hamano <gits...@pobox.com> wrote:
> Luke Diamand <l...@diamand.org> writes:
>
>> One thing I wondered about is whether this should be enabled by
>> default or not. Long-time users of git-p4 might be a bit surprised to
>> find their git commits suddenly gaining an extra Job: field.
>
> Ahh, I didn't even wonder about but that is not because I didn't
> think it matters.
>
> Does this change affect reproducibility of importing the history
> from P4, doesn't it?  Would that be a problem?

It would change the history created, but I don't see why that would be
a problem.

>
> How common is it to have the "extra" Job: thing in the history on P4
> side?

Where I work currently we don't use jobs (at present). Where I worked
before, jobs were created automatically to track issues in JIRA, and
were (supposed to be) entered into commits. It's potentially quite
useful so I guess might be quite widespread.

> If the answer to this question is "on rare occasions and only
> when there is a very good reason to have 'jobs' associated with the
> changelist", then the 'might be a bit surprised' brought by this
> change can probably be explained away as "a fix to a (design) bug
> that used to discard crucial information" that (unfortunately) have
> to change the resulting Git object names.
>

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

Re: [PATCHv2 2/3] git-p4 tests: work with python3 as well as python2

[Luke Diamand]

On 26 April 2016 at 18:48, Junio C Hamano <gits...@pobox.com> wrote:
> Luke Diamand <l...@diamand.org> writes:
>
>> Update the git-p4 tests so that they work with both
>> Python2 and Python3.
>>
>> We have to be explicit about the difference between
>> Unicode text strings (Python3 default) and raw binary
>> strings which will be exchanged with Perforce.
>>
>> Additionally, print always takes braces in Python3.
>
> s/braces/parentheses/, I think (can locally tweak if you say so--in
> which case no need to resend).

Please do so, thanks!

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

Re: BUG on OSX `git p4 submit` can fail when the workspace root doesn't exist locally.

[Luke Diamand]

On 27 April 2016 at 21:53, Stefan Beller  wrote:
> On Wed, Apr 27, 2016 at 11:06 AM, Jacob Smith  wrote:
>> I debugged the issue using the script here:
>> https://github.com/git/git/blob/master/git-p4.py
>> I'm not sure how close to the main repo that is.
>>
>> On Wed, Apr 27, 2016 at 11:28 AM, Stefan Beller  wrote:
>>> On Wed, Apr 27, 2016 at 9:15 AM, Jacob Smith  wrote:
 On OS X,
>>>
>>> Do you use the Git as provided from OS X? In that case you better report 
>>> the bug
>>> to Apple, as their version of Git is slightly different (not close on
>>> upstream, by both
>>> having additional patches as well as not following the upstream closely 
>>> IIUC).
>
> In that case, I have cc'd Luke and Lars, who work on p4

Which version of p4 are you using?

Your suggested fix looks plausible though. Possibly it needs both
chdirs() so that "p4 sync" will work if the caller is using a
.p4config file in the p4 client directory to set the P4CLIENT.

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

Re: [PATCH v2] git-p4: Fix git-p4.mapUser on Windows

[Luke Diamand]

On 27 January 2017 at 17:33, Junio C Hamano  wrote:
> George Vanburgh  writes:
>
>> From: George Vanburgh 
>>
>> When running git-p4 on Windows, with multiple git-p4.mapUser entries in
>> git config - no user mappings are applied to the generated repository.
>> ...
>> Using splitlines solves this issue, by splitting config on all
>> typical delimiters ('\n', '\r\n' etc.)
>
> Luke, Lars, this version seems to be in line with the conclusion of
> your earlier reviews, e.g.
>
> 
>
> Even though it looks OK to my eyes, I'll wait for Acks or further
> refinement suggestions from either of you two before acting on this
> patch.

It looks good to me. The tests all pass, and the change looks correct.

Ack.

Luke


>
> Thanks.
>
>> Signed-off-by: George Vanburgh 
>> ---
>>  git-p4.py | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/git-p4.py b/git-p4.py
>> index f427bf6..b66f68b 100755
>> --- a/git-p4.py
>> +++ b/git-p4.py
>> @@ -656,7 +656,7 @@ def gitConfigInt(key):
>>  def gitConfigList(key):
>>  if not _gitConfig.has_key(key):
>>  s = read_pipe(["git", "config", "--get-all", key], 
>> ignore_error=True)
>> -_gitConfig[key] = s.strip().split(os.linesep)
>> +_gitConfig[key] = s.strip().splitlines()
>>  if _gitConfig[key] == ['']:
>>  _gitConfig[key] = []
>>  return _gitConfig[key]
>>
>> --
>> https://github.com/git/git/pull/319

Re: [PATCH v2] git-p4: fix git-p4.pathEncoding for removed files

[Luke Diamand]

On 9 February 2017 at 23:39, Junio C Hamano  wrote:
> Lars Schneider  writes:
>
>> unfortunately, I missed to send this v2. I agree with Luke's review and
>> I moved the re-encode of the path name to the `streamOneP4File` and
>> `streamOneP4Deletion` explicitly.
>>
>> Discussion:
>> http://public-inbox.org/git/CAE5ih7-=bd_zol5pfyfd2qvy-xe24v_cgge0xoavuotk02e...@mail.gmail.com/
>>
>> Thanks,
>> Lars
>
> Thanks.  Will replace but will not immediately merge to 'next' yet,
> just in case Luke wants to tell me add his "Reviewed-by:".

Yes, this looks good to me now.

Luke

Re: [PATCH] git-p4: Fix git-p4.mapUser on Windows

[Luke Diamand]

I'm confusedsee below.

On 21 January 2017 at 15:21, George Vanburgh  wrote:
>> On 21 Jan 2017, at 13:33, Lars Schneider 
>> > On 21 Jan 2017, at 13:02, George Vanburgh 
>> wrote:
>> >
>> > From: George Vanburgh 
>> >
>> > When running git-p4 on Windows, with multiple git-p4.mapUser entries
>> > in git config - no user mappings are applied to the generated
> repository.
>> >
>> > Reproduction Steps:
>> >
>> > 1. Add multiple git-p4.mapUser entries to git config on a Windows
>> >   machine
>> > 2. Attempt to clone a p4 repository
>> >
>> > None of the user mappings will be applied.
>> >
>> > This issue is caused by the fact that gitConfigList, uses
>> > split(os.linesep) to convert the output of git config --get-all into a
>> > list.
>> >
>> > On Windows, os.linesep is equal to '\r\n' - however git.exe returns
>> > configuration with a line seperator of '\n'. This leads to the list
>> > returned by gitConfigList containing only one element - which contains
>> > the full output of git config --get-all in string form. This causes
>> > problems for the code introduced to getUserMapFromPerforceServer in
>> > 10d08a1.
>> >
>> > This issue should be caught by the test introduced in 10d08a1, and
>> > would require running on Windows to reproduce. When running inside
>> > MinGW/Cygwin, however, os.linesep correctly returns '\n', and
>> > everything works as expected.
>>
>> This surprises me. I would expect `\r\n` in a MinGW env...
>> Nevertheless, I wouldn't have caught that as I don't run the git-p4 tests
> on
>> Windows...
>
> It appears I was mistaken - the successful tests I ran were actually under
> the Ubuntu subsystem for Windows, which (obviously) passed.
>
> Just did a quick experiment:
>
> Git Bash (MinGW):
> georg@TEMPEST MINGW64 ~
> $ python -c "import os
> print(repr(os.linesep))"
> '\r\n'
>
> Powershell:
> PS C:\Users\georg> python -c "import os
>>> print(repr(os.linesep))"
> '\r\n'
>
> Ubuntu subsystem for Windows:
> george@TEMPEST:~$ python -c "import os
> print(repr(os.linesep))"
> '\n'
>
> So this issue applies to git-p4 running under both PowerShell and MinGW.
>
>>
>>
>> > The simplest fix for this issue would be to convert the line split
>> > logic inside gitConfigList to use splitlines(), which splits on any
>> > standard line delimiter. However, this function was only introduced in
>> > Python 2.7, and would mean a bump in the minimum required version of
>> > Python required to run git-p4. The alternative fix, implemented here,
>> > is to use '\n' as a delimiter, which git.exe appears to output
>> > consistently on Windows anyway.

Have you tried a 2.6 Python with splitlines? I just tried this code
and it seems fine.

> val = s.strip().splitlines()
> print("splitlines:", val)

Tried with 2.7 and 2.6, and bot print out an array of strings returned
from read_pipe().

And 'grep -r splitlines' on the Python2.6 source has lots of hits.

>>
>> Well, that also means if we ever use splitlines() then your fix below
> would
>> brake the code, right?
>>
>> Python 2.7 was released 7 years ago in 2010.
>
> Now I feel old...

:-)

>
>> Therefore, I would vote to
>> bump the minimum version. But that's just my opinion :-)
>
> I feel like splitlines is the better/safer fix - but figured bumping the
> minimum
> Python version was probably part of a wider discussion. If it's something
> people
> are comfortable with - I'd be happy to rework the fix to use splitlines.
> Luke - do you have any thoughts on this?

I agree that we have to stop supporting 2.6 at some point (and start
supporting 3.x, but that's another world of hurt). But does 2.6 really
not have splitlines?

If you send a patch with splitlines I can try it out (although I guess
it could be broken under Windows).

Luke

Re: [PATCH] [git-p4.py] Add --checkpoint-period option to sync/clone

[Luke Diamand]

On 12 September 2016 at 23:02, Ori Rawlings  wrote:
> Importing a long history from Perforce into git using the git-p4 tool
> can be especially challenging. The `git p4 clone` operation is based
> on an all-or-nothing transactionality guarantee. Under real-world
> conditions like network unreliability or a busy Perforce server,
> `git p4 clone` and  `git p4 sync` operations can easily fail, forcing a
> user to restart the import process from the beginning. The longer the
> history being imported, the more likely a fault occurs during the
> process. Long enough imports thus become statistically unlikely to ever
> succeed.

That would never happen :-)

The usual thing that I find is that my Perforce login session expires.

>
> The underlying git fast-import protocol supports an explicit checkpoint
> command. The idea here is to optionally allow the user to force an
> explicit checkpoint every  seconds. If the sync/clone operation fails
> branches are left updated at the appropriate commit available during the
> latest checkpoint. This allows a user to resume importing Perforce
> history while only having to repeat at most approximately  seconds
> worth of import activity.

I think this ought to work, and could be quite useful. It would be
good to have some kind of test case for it though, and updated
documentation.

Luke

>
> Signed-off-by: Ori Rawlings 
> ---
>  git-p4.py | 8 
>  1 file changed, 8 insertions(+)
>
> diff --git a/git-p4.py b/git-p4.py
> index fd5ca52..40cb64f 100755
> --- a/git-p4.py
> +++ b/git-p4.py
> @@ -2244,6 +2244,7 @@ class P4Sync(Command, P4UserMap):
>  optparse.make_option("-/", dest="cloneExclude",
>   action="append", type="string",
>   help="exclude depot path"),
> +optparse.make_option("--checkpoint-period", 
> dest="checkpointPeriod", type="int", help="Period in seconds between explict 
> git fast-import checkpoints (by default, no explicit checkpoints are 
> performed)"),
>  ]
>  self.description = """Imports from Perforce into a git repository.\n
>  example:
> @@ -2276,6 +2277,7 @@ class P4Sync(Command, P4UserMap):
>  self.tempBranches = []
>  self.tempBranchLocation = "refs/git-p4-tmp"
>  self.largeFileSystem = None
> +self.checkpointPeriod = -1

Or use None?

>
>  if gitConfig('git-p4.largeFileSystem'):
>  largeFileSystemConstructor = 
> globals()[gitConfig('git-p4.largeFileSystem')]
> @@ -3031,6 +3033,8 @@ class P4Sync(Command, P4UserMap):
>
>  def importChanges(self, changes):
>  cnt = 1
> +if self.checkpointPeriod > -1:
> +self.lastCheckpointTime = time.time()

Could you just always set the lastCheckpointTime?

>  for change in changes:
>  description = p4_describe(change)
>  self.updateOptionDict(description)
> @@ -3107,6 +3111,10 @@ class P4Sync(Command, P4UserMap):
>  self.initialParent)
>  # only needed once, to connect to the previous commit
>  self.initialParent = ""
> +
> +if self.checkpointPeriod > -1 and time.time() - 
> self.lastCheckpointTime > self.checkpointPeriod:
> +self.checkpoint()
> +self.lastCheckpointTime = time.time()

If you use time.time(), then this could fail to work as expected if
the system time goes backwards (e.g. NTP updates). However, Python 2
doesn't have access to clock_gettime() without jumping through hoops,
so perhaps we leave this as a bug until git-p4 gets ported to Python
3.



>  except IOError:
>  print self.gitError.read()
>  sys.exit(1)
> --
> 2.7.4 (Apple Git-66)
>

[PATCHv2] git-p4 worktree support

[Luke Diamand]

Second attempt at teaching git-p4 about worktrees.

Earlier discussion here:

http://marc.info/?l=git=148097985622294

Git-p4 exports GIT_DIR so that when it chdirs into the
P4 client area to apply the change, git commands called
from there will work correctly.

Luke Diamand (1):
  git-p4: support git worktrees

 git-p4.py   | 47 ++-
 t/t9800-git-p4-basic.sh | 20 
 2 files changed, 46 insertions(+), 21 deletions(-)

-- 
2.8.2.703.g78b384c.dirty

[PATCHv2] git-p4: support git worktrees

[Luke Diamand]

git-p4 would attempt to find the git directory using
its own specific code, which did not know about git
worktrees. This caused git operations to fail needlessly.

Rework it to use "git rev-parse --git-dir" instead, which
knows about worktrees.

Signed-off-by: Luke Diamand <l...@diamand.org>
---
 git-p4.py   | 47 ++-
 t/t9800-git-p4-basic.sh | 20 
 2 files changed, 46 insertions(+), 21 deletions(-)

diff --git a/git-p4.py b/git-p4.py
index fd5ca52..6aa8957 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -49,6 +49,13 @@ defaultLabelRegexp = r'[a-zA-Z0-9_\-.]+$'
 # Grab changes in blocks of this many revisions, unless otherwise requested
 defaultBlockSize = 512
 
+def gitdir():
+d = read_pipe("git rev-parse --git-dir").strip()
+if not d or len(d) == 0:
+return None
+else:
+return d
+
 def p4_build_cmd(cmd):
 """Build a suitable p4 command line.
 
@@ -562,12 +569,6 @@ def currentGitBranch():
 else:
 return read_pipe(["git", "name-rev", "HEAD"]).split(" ")[1].strip()
 
-def isValidGitDir(path):
-if (os.path.exists(path + "/HEAD")
-and os.path.exists(path + "/refs") and os.path.exists(path + 
"/objects")):
-return True;
-return False
-
 def parseRevision(ref):
 return read_pipe("git rev-parse %s" % ref).strip()
 
@@ -3462,7 +3463,7 @@ class P4Sync(Command, P4UserMap):
 if self.tempBranches != []:
 for branch in self.tempBranches:
 read_pipe("git update-ref -d %s" % branch)
-os.rmdir(os.path.join(os.environ.get("GIT_DIR", ".git"), 
self.tempBranchLocation))
+os.rmdir(os.path.join(gitdir(), self.tempBranchLocation))
 
 # Create a symbolic ref p4/HEAD pointing to p4/ to allow
 # a convenient shortcut refname "p4".
@@ -3678,23 +3679,27 @@ def main():
 (cmd, args) = parser.parse_args(sys.argv[2:], cmd);
 global verbose
 verbose = cmd.verbose
+
 if cmd.needsGit:
-if cmd.gitdir == None:
-cmd.gitdir = os.path.abspath(".git")
-if not isValidGitDir(cmd.gitdir):
-cmd.gitdir = read_pipe("git rev-parse --git-dir").strip()
-if os.path.exists(cmd.gitdir):
-cdup = read_pipe("git rev-parse --show-cdup").strip()
-if len(cdup) > 0:
-chdir(cdup);
-
-if not isValidGitDir(cmd.gitdir):
-if isValidGitDir(cmd.gitdir + "/.git"):
-cmd.gitdir += "/.git"
-else:
+if cmd.gitdir:
+os.environ["GIT_DIR"] = cmd.gitdir
+
+# did we get a valid git dir on the command line or via $GIT_DIR?
+if not gitdir():
 die("fatal: cannot locate git repository at %s" % cmd.gitdir)
 
-os.environ["GIT_DIR"] = cmd.gitdir
+else:
+# already in a git directory?
+if not gitdir():
+die("fatal: not in a valid git repository")
+
+cdup = read_pipe("git rev-parse --show-cdup").strip()
+if len(cdup) > 0:
+chdir(cdup);
+
+# ensure subshells spawned in the p4 repo directory
+# get the correct GIT_DIR
+os.environ["GIT_DIR"] = os.path.abspath(gitdir())
 
 if not cmd.run(args):
 parser.print_help()
diff --git a/t/t9800-git-p4-basic.sh b/t/t9800-git-p4-basic.sh
index 0730f18..093e9bd 100755
--- a/t/t9800-git-p4-basic.sh
+++ b/t/t9800-git-p4-basic.sh
@@ -257,6 +257,26 @@ test_expect_success 'submit from detached head' '
)
 '
 
+test_expect_success 'submit from worktree' '
+   test_when_finished cleanup_git &&
+   git p4 clone --dest="$git" //depot &&
+   (
+   cd "$git" &&
+   git worktree add ../worktree-test
+   ) &&
+   (
+   cd "$git/../worktree-test" &&
+   test_commit "worktree-commit" &&
+   git config git-p4.skipSubmitEdit true &&
+   git p4 submit
+   ) &&
+   (
+   cd "$cli" &&
+   p4 sync &&
+   test_path_is_file worktree-commit.t
+   )
+'
+
 test_expect_success 'kill p4d' '
kill_p4d
 '
-- 
2.8.2.703.g78b384c.dirty

Re: [PATCHv2] git-p4: support git worktrees

[Luke Diamand]

On 10 December 2016 at 21:57, Luke Diamand <l...@diamand.org> wrote:
> git-p4 would attempt to find the git directory using
> its own specific code, which did not know about git
> worktrees. This caused git operations to fail needlessly.
>
> Rework it to use "git rev-parse --git-dir" instead, which
> knows about worktrees.

Actually this doesn't work as well as the original version. "git
rev-parse --git-dir" won't go and find the ".git" subdirectory. The
previous version would go looking for it, so this would introduce a
regression.

Luke


>
> Signed-off-by: Luke Diamand <l...@diamand.org>
> ---
>  git-p4.py   | 47 ++-
>  t/t9800-git-p4-basic.sh | 20 
>  2 files changed, 46 insertions(+), 21 deletions(-)
>
> diff --git a/git-p4.py b/git-p4.py
> index fd5ca52..6aa8957 100755
> --- a/git-p4.py
> +++ b/git-p4.py
> @@ -49,6 +49,13 @@ defaultLabelRegexp = r'[a-zA-Z0-9_\-.]+$'
>  # Grab changes in blocks of this many revisions, unless otherwise requested
>  defaultBlockSize = 512
>
> +def gitdir():
> +d = read_pipe("git rev-parse --git-dir").strip()
> +if not d or len(d) == 0:
> +return None
> +else:
> +return d
> +
>  def p4_build_cmd(cmd):
>  """Build a suitable p4 command line.
>
> @@ -562,12 +569,6 @@ def currentGitBranch():
>  else:
>  return read_pipe(["git", "name-rev", "HEAD"]).split(" ")[1].strip()
>
> -def isValidGitDir(path):
> -if (os.path.exists(path + "/HEAD")
> -and os.path.exists(path + "/refs") and os.path.exists(path + 
> "/objects")):
> -return True;
> -return False
> -
>  def parseRevision(ref):
>  return read_pipe("git rev-parse %s" % ref).strip()
>
> @@ -3462,7 +3463,7 @@ class P4Sync(Command, P4UserMap):
>  if self.tempBranches != []:
>  for branch in self.tempBranches:
>  read_pipe("git update-ref -d %s" % branch)
> -os.rmdir(os.path.join(os.environ.get("GIT_DIR", ".git"), 
> self.tempBranchLocation))
> +os.rmdir(os.path.join(gitdir(), self.tempBranchLocation))
>
>  # Create a symbolic ref p4/HEAD pointing to p4/ to allow
>  # a convenient shortcut refname "p4".
> @@ -3678,23 +3679,27 @@ def main():
>  (cmd, args) = parser.parse_args(sys.argv[2:], cmd);
>  global verbose
>  verbose = cmd.verbose
> +
>  if cmd.needsGit:
> -if cmd.gitdir == None:
> -cmd.gitdir = os.path.abspath(".git")
> -if not isValidGitDir(cmd.gitdir):
> -cmd.gitdir = read_pipe("git rev-parse --git-dir").strip()
> -if os.path.exists(cmd.gitdir):
> -cdup = read_pipe("git rev-parse --show-cdup").strip()
> -if len(cdup) > 0:
> -chdir(cdup);
> -
> -if not isValidGitDir(cmd.gitdir):
> -if isValidGitDir(cmd.gitdir + "/.git"):
> -cmd.gitdir += "/.git"
> -else:
> +if cmd.gitdir:
> +os.environ["GIT_DIR"] = cmd.gitdir
> +
> +# did we get a valid git dir on the command line or via $GIT_DIR?
> +if not gitdir():
>  die("fatal: cannot locate git repository at %s" % cmd.gitdir)
>
> -os.environ["GIT_DIR"] = cmd.gitdir
> +else:
> +# already in a git directory?
> +if not gitdir():
> +die("fatal: not in a valid git repository")
> +
> +cdup = read_pipe("git rev-parse --show-cdup").strip()
> +if len(cdup) > 0:
> +chdir(cdup);
> +
> +# ensure subshells spawned in the p4 repo directory
> +# get the correct GIT_DIR
> +os.environ["GIT_DIR"] = os.path.abspath(gitdir())
>
>  if not cmd.run(args):
>  parser.print_help()
> diff --git a/t/t9800-git-p4-basic.sh b/t/t9800-git-p4-basic.sh
> index 0730f18..093e9bd 100755
> --- a/t/t9800-git-p4-basic.sh
> +++ b/t/t9800-git-p4-basic.sh
> @@ -257,6 +257,26 @@ test_expect_success 'submit from detached head' '
> )
>  '
>
> +test_expect_success 'submit from worktree' '
> +   test_when_finished cleanup_git &&
> +   git p4 clone --dest="$git" //depot &&
> +   (
> +   cd "$git" &&
> +   git worktree add ../worktree-test
> +   ) &&
> +   (
> +   cd "$git/../worktree-test" &&
> +   test_commit "worktree-commit" &&
> +   git config git-p4.skipSubmitEdit true &&
> +   git p4 submit
> +   ) &&
> +   (
> +   cd "$cli" &&
> +   p4 sync &&
> +   test_path_is_file worktree-commit.t
> +   )
> +'
> +
>  test_expect_success 'kill p4d' '
> kill_p4d
>  '
> --
> 2.8.2.703.g78b384c.dirty
>

[PATCHv3] git-p4: support git worktrees

[Luke Diamand]

git-p4 would attempt to find the git directory using
its own specific code, which did not know about git
worktrees.

Rework it to use "git rev-parse --git-dir" instead.

Add test cases for worktree usage and specifying
git directory via --git-dir and $GIT_DIR.

Signed-off-by: Luke Diamand <l...@diamand.org>
---
 git-p4.py | 17 +
 t/t9800-git-p4-basic.sh   | 20 
 t/t9806-git-p4-options.sh | 32 
 3 files changed, 65 insertions(+), 4 deletions(-)

diff --git a/git-p4.py b/git-p4.py
index fd5ca52..6a1f65f 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -85,6 +85,16 @@ def p4_build_cmd(cmd):
 real_cmd += cmd
 return real_cmd
 
+def git_dir(path):
+""" Return TRUE if the given path is a git directory (/path/to/dir/.git).
+This won't automatically add ".git" to a directory.
+"""
+d = read_pipe(["git", "--git-dir", path, "rev-parse", "--git-dir"], 
True).strip()
+if not d or len(d) == 0:
+return None
+else:
+return d
+
 def chdir(path, is_client_path=False):
 """Do chdir to the given path, and set the PWD environment
variable for use by P4.  It does not look at getcwd() output.
@@ -563,10 +573,7 @@ def currentGitBranch():
 return read_pipe(["git", "name-rev", "HEAD"]).split(" ")[1].strip()
 
 def isValidGitDir(path):
-if (os.path.exists(path + "/HEAD")
-and os.path.exists(path + "/refs") and os.path.exists(path + 
"/objects")):
-return True;
-return False
+return git_dir(path) != None
 
 def parseRevision(ref):
 return read_pipe("git rev-parse %s" % ref).strip()
@@ -3682,6 +3689,7 @@ def main():
 if cmd.gitdir == None:
 cmd.gitdir = os.path.abspath(".git")
 if not isValidGitDir(cmd.gitdir):
+# "rev-parse --git-dir" without arguments will try $PWD/.git
 cmd.gitdir = read_pipe("git rev-parse --git-dir").strip()
 if os.path.exists(cmd.gitdir):
 cdup = read_pipe("git rev-parse --show-cdup").strip()
@@ -3694,6 +3702,7 @@ def main():
 else:
 die("fatal: cannot locate git repository at %s" % cmd.gitdir)
 
+# so git commands invoked from the P4 workspace will succeed
 os.environ["GIT_DIR"] = cmd.gitdir
 
 if not cmd.run(args):
diff --git a/t/t9800-git-p4-basic.sh b/t/t9800-git-p4-basic.sh
index 0730f18..093e9bd 100755
--- a/t/t9800-git-p4-basic.sh
+++ b/t/t9800-git-p4-basic.sh
@@ -257,6 +257,26 @@ test_expect_success 'submit from detached head' '
)
 '
 
+test_expect_success 'submit from worktree' '
+   test_when_finished cleanup_git &&
+   git p4 clone --dest="$git" //depot &&
+   (
+   cd "$git" &&
+   git worktree add ../worktree-test
+   ) &&
+   (
+   cd "$git/../worktree-test" &&
+   test_commit "worktree-commit" &&
+   git config git-p4.skipSubmitEdit true &&
+   git p4 submit
+   ) &&
+   (
+   cd "$cli" &&
+   p4 sync &&
+   test_path_is_file worktree-commit.t
+   )
+'
+
 test_expect_success 'kill p4d' '
kill_p4d
 '
diff --git a/t/t9806-git-p4-options.sh b/t/t9806-git-p4-options.sh
index 254d428..1ab76c4 100755
--- a/t/t9806-git-p4-options.sh
+++ b/t/t9806-git-p4-options.sh
@@ -269,6 +269,38 @@ test_expect_success 'submit works with two branches' '
)
 '
 
+test_expect_success 'use --git-dir option and GIT_DIR' '
+   test_when_finished cleanup_git &&
+   git p4 clone //depot --destination="$git" &&
+   (
+   cd "$git" &&
+   git config git-p4.skipSubmitEdit true &&
+   test_commit first-change &&
+   git p4 submit --git-dir "$git"
+   ) &&
+   (
+   cd "$cli" &&
+   p4 sync &&
+   test_path_is_file first-change.t &&
+   echo "cli_file" >cli_file.t &&
+   p4 add cli_file.t &&
+   p4 submit -d "cli change"
+   ) &&
+   (git --git-dir "$git" p4 sync) &&
+   (cd "$git" && git checkout -q p4/master) &&
+   test_path_is_file "$git"/cli_file.t &&
+   (
+   cd "$cli" &&
+   echo "cli_file2" >cli_file2.t &&
+   p4 add cli_file2.t  &&
+   p4 submit -d "cli change2"
+   ) &&
+   (GIT_DIR="$git" git p4 sync) &&
+   (cd "$git" && git checkout -q p4/master) &&
+   test_path_is_file "$git"/cli_file2.t
+'
+
+
 test_expect_success 'kill p4d' '
kill_p4d
 '
-- 
2.8.2.703.g78b384c.dirty

[PATCHv3] git-p4 worktree support

[Luke Diamand]

Support for git-p4 worktrees.

Adds test cases for using git from a worktree, and
specifying the git directory either with the --git-dir
command-line option, or with $ENV{GIT_DIR}.

Luke Diamand (1):
  git-p4: support git worktrees

 git-p4.py | 17 +
 t/t9800-git-p4-basic.sh   | 20 
 t/t9806-git-p4-options.sh | 32 
 3 files changed, 65 insertions(+), 4 deletions(-)

-- 
2.8.2.703.g78b384c.dirty

[PATCHv1 0/1] git-p4: handle symlinked directories

[Luke Diamand]

There's a long-standing bug around handling of symlinked directories
in git-p4.

If in git you create a directory, and then symlink it, when git-p4
tries to create the diff-summary of what it's about to do, it
tries to open the symlink as a regular file, and fails.

Luke Diamand (1):
  git-p4: avoid crash adding symlinked directory

 git-p4.py | 26 --
 t/t9830-git-p4-symlink-dir.sh | 43 +++
 2 files changed, 63 insertions(+), 6 deletions(-)
 create mode 100755 t/t9830-git-p4-symlink-dir.sh

-- 
2.11.0.274.g0ea315c

[PATCHv1 1/1] git-p4: avoid crash adding symlinked directory

[Luke Diamand]

When submitting to P4, if git-p4 came across a symlinked
directory, then during the generation of the submit diff, it would
try to open it as a normal file and fail.

Spot this case, and instead output the target of the symlink in
the submit diff.

Add a test case.

Signed-off-by: Luke Diamand <l...@diamand.org>
---
 git-p4.py | 26 --
 t/t9830-git-p4-symlink-dir.sh | 43 +++
 2 files changed, 63 insertions(+), 6 deletions(-)
 create mode 100755 t/t9830-git-p4-symlink-dir.sh

diff --git a/git-p4.py b/git-p4.py
index fd5ca5246..d2f59bd91 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -25,6 +25,7 @@ import stat
 import zipfile
 import zlib
 import ctypes
+import errno
 
 try:
 from subprocess import CalledProcessError
@@ -1538,7 +1539,7 @@ class P4Submit(Command, P4UserMap):
 if response == 'n':
 return False
 
-def get_diff_description(self, editedFiles, filesToAdd):
+def get_diff_description(self, editedFiles, filesToAdd, symlinks):
 # diff
 if os.environ.has_key("P4DIFF"):
 del(os.environ["P4DIFF"])
@@ -1553,10 +1554,17 @@ class P4Submit(Command, P4UserMap):
 newdiff += " new file \n"
 newdiff += "--- /dev/null\n"
 newdiff += "+++ %s\n" % newFile
-f = open(newFile, "r")
-for line in f.readlines():
-newdiff += "+" + line
-f.close()
+
+try:
+f = open(newFile, "r")
+for line in f.readlines():
+newdiff += "+" + line
+f.close()
+except IOError as e:
+if e.errno == errno.EISDIR and newFile in symlinks:
+newdiff += "+%s\n" % os.readlink(newFile)
+else:
+raise
 
 return (diff + newdiff).replace('\r\n', '\n')
 
@@ -1574,6 +1582,7 @@ class P4Submit(Command, P4UserMap):
 filesToDelete = set()
 editedFiles = set()
 pureRenameCopy = set()
+symlinks = set()
 filesToChangeExecBit = {}
 
 for line in diff:
@@ -1590,6 +1599,11 @@ class P4Submit(Command, P4UserMap):
 filesToChangeExecBit[path] = diff['dst_mode']
 if path in filesToDelete:
 filesToDelete.remove(path)
+
+dst_mode = int(diff['dst_mode'], 8)
+if dst_mode == 012:
+symlinks.add(path)
+
 elif modifier == "D":
 filesToDelete.add(path)
 if path in filesToAdd:
@@ -1727,7 +1741,7 @@ class P4Submit(Command, P4UserMap):
 separatorLine = " everything below this line is just the diff 
###\n"
 if not self.prepare_p4_only:
 submitTemplate += separatorLine
-submitTemplate += self.get_diff_description(editedFiles, 
filesToAdd)
+submitTemplate += self.get_diff_description(editedFiles, 
filesToAdd, symlinks)
 
 (handle, fileName) = tempfile.mkstemp()
 tmpFile = os.fdopen(handle, "w+b")
diff --git a/t/t9830-git-p4-symlink-dir.sh b/t/t9830-git-p4-symlink-dir.sh
new file mode 100755
index 0..3dc528bb1
--- /dev/null
+++ b/t/t9830-git-p4-symlink-dir.sh
@@ -0,0 +1,43 @@
+#!/bin/sh
+
+test_description='git p4 symlinked directories'
+
+. ./lib-git-p4.sh
+
+test_expect_success 'start p4d' '
+   start_p4d
+'
+
+test_expect_success 'symlinked directory' '
+   (
+   cd "$cli" &&
+   : >first_file.t &&
+   p4 add first_file.t &&
+   p4 submit -d "first change"
+   ) &&
+   git p4 clone --dest "$git" //depot &&
+   (
+   cd "$git" &&
+   mkdir -p some/sub/directory &&
+   mkdir -p other/subdir2 &&
+   : > other/subdir2/file.t &&
+   (cd some/sub/directory && ln -s ../../../other/subdir2 .) &&
+   git add some other &&
+   git commit -m "symlinks" &&
+   git config git-p4.skipSubmitEdit true &&
+   git p4 submit -v
+   ) &&
+   (
+   cd "$cli" &&
+   p4 sync &&
+   test -L some/sub/directory/subdir2
+   test_path_is_file some/sub/directory/subdir2/file.t
+   )
+
+'
+
+test_expect_success 'kill p4d' '
+   kill_p4d
+'
+
+test_done
-- 
2.11.0.274.g0ea315c

Re: [PATCH] git-p4: add p4 shelf support

[Luke Diamand]

On 6 December 2016 at 16:12, Nuno Subtil  wrote:
> Yeah, it looks similar. This change can be ignored if those have already
> been accepted.

Thanks, that's appreciated!

Luke

[PATCHv1 0/2] git-p4 patches

[Luke Diamand]

This is a couple of small patches for git-p4.

The first just teaches git-p4 about git workspaces, so that you can
do "git p4 submit" from within a workspace (P.S. workspaces
are completely *awesome*).

The second follows on from the work by Vinicius for shelving
changelists, by letting you update an existing shelved changelist.

This is a bit like using "git commit --amend" only far more painful

Luke Diamand (2):
  git-p4: support git-workspaces
  git-p4: support updating an existing shelved changelist

 Documentation/git-p4.txt |  4 
 git-p4.py| 39 +++
 t/t9807-git-p4-submit.sh | 38 ++
 3 files changed, 77 insertions(+), 4 deletions(-)

-- 
2.11.0.274.g0ea315c

[PATCHv1 2/2] git-p4: support updating an existing shelved changelist

[Luke Diamand]

Adds new option "--update-shelve CHANGELIST" which updates
an existing shelved changelist.

The original changelist must have been created by the current user.

This allows workflow something like:

   hack hack hack
   git commit
   git p4 submit --shelve
   $mail interested parties about shelved changelist
   make corrections
   git commit --amend
   git p4 submit --update-shelve $CHANGELIST
   $mail interested parties about shelved changelist
   etc

Signed-off-by: Luke Diamand <l...@diamand.org>
---
 Documentation/git-p4.txt |  4 
 git-p4.py| 33 +
 t/t9807-git-p4-submit.sh | 38 ++
 3 files changed, 71 insertions(+), 4 deletions(-)

diff --git a/Documentation/git-p4.txt b/Documentation/git-p4.txt
index 1bbf43d15..ce40b9a54 100644
--- a/Documentation/git-p4.txt
+++ b/Documentation/git-p4.txt
@@ -308,6 +308,10 @@ These options can be used to modify 'git p4 submit' 
behavior.
After creating each shelve, the relevant files are reverted/deleted.
If you have multiple commits pending multiple shelves will be created.
 
+--update-shelve CHANGELIST::
+   Update an existing shelved changelist with this commit. Implies
+   --shelve.
+
 --conflict=(ask|skip|quit)::
Conflicts can occur when applying a commit to p4.  When this
happens, the default behavior ("ask") is to prompt whether to
diff --git a/git-p4.py b/git-p4.py
index 5e2db1919..36242d384 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -262,6 +262,10 @@ def p4_revert(f):
 def p4_reopen(type, f):
 p4_system(["reopen", "-t", type, wildcard_encode(f)])
 
+def p4_reopen_in_change(changelist, files):
+cmd = ["reopen", "-c", str(changelist)] + files
+p4_system(cmd)
+
 def p4_move(src, dest):
 p4_system(["move", "-k", wildcard_encode(src), wildcard_encode(dest)])
 
@@ -1298,6 +1302,9 @@ class P4Submit(Command, P4UserMap):
 optparse.make_option("--shelve", dest="shelve", 
action="store_true",
  help="Shelve instead of submit. Shelved 
files are reverted, "
  "restoring the workspace to the state 
before the shelve"),
+optparse.make_option("--update-shelve", dest="update_shelve", 
action="store", type="int",
+ metavar="CHANGELIST",
+ help="update an existing shelved 
changelist, implies --shelve")
 ]
 self.description = "Submit changes from git to the perforce depot."
 self.usage += " [name of git branch to submit into perforce depot]"
@@ -1306,6 +1313,7 @@ class P4Submit(Command, P4UserMap):
 self.preserveUser = gitConfigBool("git-p4.preserveUser")
 self.dry_run = False
 self.shelve = False
+self.update_shelve = None
 self.prepare_p4_only = False
 self.conflict_behavior = None
 self.isWindows = (platform.system() == "Windows")
@@ -1474,7 +1482,7 @@ class P4Submit(Command, P4UserMap):
 return 1
 return 0
 
-def prepareSubmitTemplate(self):
+def prepareSubmitTemplate(self, changelist=None):
 """Run "p4 change -o" to grab a change specification template.
This does not use "p4 -G", as it is nice to keep the submission
template in original order, since a human might edit it.
@@ -1486,7 +1494,11 @@ class P4Submit(Command, P4UserMap):
 
 template = ""
 inFilesSection = False
-for line in p4_read_pipe_lines(['change', '-o']):
+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:
@@ -1585,11 +1597,14 @@ class P4Submit(Command, P4UserMap):
 editedFiles = set()
 pureRenameCopy = set()
 filesToChangeExecBit = {}
+all_files = list()
 
 for line in diff:
 diff = parseDiffTreeEntry(line)
 modifier = diff['status']
 path = diff['src']
+all_files.append(path)
+
 if modifier == "M":
 p4_edit(path)
 if isModeExecChanged(diff['src_mode'], diff['dst_mode']):
@@ -1715,6 +1730,10 @@ class P4Submit(Command, P4UserMap):
 mode = filesToChangeExecBit[f]
 setP4ExecBit(f, mode)
 
+if self.update_shelve:
+print("all_files = %s" % str(all_files))
+p4_reopen_in_change(self.update_shelve, all_

[PATCHv1 1/2] git-p4: support git-workspaces

[Luke Diamand]

Teach git-p4 about git-workspaces.

Signed-off-by: Luke Diamand <l...@diamand.org>
---
 git-p4.py | 6 ++
 1 file changed, 6 insertions(+)

diff --git a/git-p4.py b/git-p4.py
index 0c4f2afd2..5e2db1919 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -566,6 +566,12 @@ def isValidGitDir(path):
 if (os.path.exists(path + "/HEAD")
 and os.path.exists(path + "/refs") and os.path.exists(path + 
"/objects")):
 return True;
+
+# git workspace directory?
+if (os.path.exists(path + "/HEAD")
+and os.path.exists(path + "/gitdir")):
+return True
+
 return False
 
 def parseRevision(ref):
-- 
2.11.0.274.g0ea315c

Re: [PATCH] allow git-p4 to create shelved changelists

[Luke Diamand]

On 28 November 2016 at 19:06, Junio C Hamano  wrote:
> Vinicius Kursancew  writes:
>
>> This patch adds a "--shelve" option to the submit subcommand, it will
>> save the changes to a perforce shelve instead of commiting them.

Looks good to me, thanks!

Works a treat.

Ack.

>>
>> Vinicius Kursancew (1):
>>   git-p4: allow submit to create shelved changelists.
>>
>>  Documentation/git-p4.txt |  5 +
>>  git-p4.py| 36 ++--
>>  t/t9807-git-p4-submit.sh | 31 +++
>>  3 files changed, 58 insertions(+), 14 deletions(-)
>
> Thanks, but I am a wrong person to review this change, so I'll
> summon two people who appear in "git shortlog --since=18.months"
> output to help review it.
>
>

Re: [PATCHv1 1/2] git-p4: support git-workspaces

[Luke Diamand]

On 5 December 2016 at 20:53, Junio C Hamano <gits...@pobox.com> wrote:
> Luke Diamand <l...@diamand.org> writes:
>
>> Teach git-p4 about git-workspaces.
>
> Is this what we call "git worktree", or something else?

Ah, I think you're right!

>
>>
>> Signed-off-by: Luke Diamand <l...@diamand.org>
>> ---
>>  git-p4.py | 6 ++
>>  1 file changed, 6 insertions(+)
>>
>> diff --git a/git-p4.py b/git-p4.py
>> index 0c4f2afd2..5e2db1919 100755
>> --- a/git-p4.py
>> +++ b/git-p4.py
>> @@ -566,6 +566,12 @@ def isValidGitDir(path):
>>  if (os.path.exists(path + "/HEAD")
>>  and os.path.exists(path + "/refs") and os.path.exists(path + 
>> "/objects")):
>>  return True;
>> +
>> +# git workspace directory?
>> +if (os.path.exists(path + "/HEAD")
>> +and os.path.exists(path + "/gitdir")):
>> +return True
>> +
>>  return False
>>
>>  def parseRevision(ref):

Re: [PATCHv1 1/2] git-p4: support git-workspaces

[Luke Diamand]

On 5 December 2016 at 21:24, Junio C Hamano <gits...@pobox.com> wrote:
> Luke Diamand <l...@diamand.org> writes:
>
>> On 5 December 2016 at 20:53, Junio C Hamano <gits...@pobox.com> wrote:
>>> Luke Diamand <l...@diamand.org> writes:
>>>
>>>> Teach git-p4 about git-workspaces.
>>>
>>> Is this what we call "git worktree", or something else?
>>
>> Ah, I think you're right!
>
> Then I'll queue it like the attached.
>
> HOWEVER.
>
> How fast does isValidGitDir() function need to be?

It doesn't need to be fast.

> The primary one
> seems to check HEAD (but it does not notice a non-repository that
> has a directory with that name), refs and objects (but it does not
> notice a non-repository that has a non-directory with these names),
> and this new one uses a test that is even more sloppy.
>
> What I am trying to get at is if we want to use a single command
> that can be given a path and answer "Yes, that is a repository"
> here, and that single command should know how the repository should
> look like.  I offhand do not know we already have such a command we
> can use, e.g. "git rev-parse --is-git-dir $path", but if there isn't
> perhaps we would want one, so that not just "git p4" but other
> scripted Porcelains can make use of it?

That would be nicer than random ad-hoc rules, certainly. I couldn't
find any obvious combination that would work.

Luke

Re: [PATCH] git-p4: add p4 shelf support

[Luke Diamand]

On 6 December 2016 at 02:02, Nuno Subtil  wrote:
> Extends the submit command to support shelving a commit instead of
> submitting it to p4 (similar to --prepare-p4-only).

Is this just the same as these two changes?

http://www.spinics.net/lists/git/msg290755.html
http://www.spinics.net/lists/git/msg291103.html

Thanks,
Luke

>
> Signed-off-by: Nuno Subtil 
> ---
>  git-p4.py | 36 ++--
>  1 file changed, 30 insertions(+), 6 deletions(-)
>
> diff --git a/git-p4.py b/git-p4.py
> index fd5ca52..3c4be22 100755
> --- a/git-p4.py
> +++ b/git-p4.py
> @@ -1286,6 +1286,8 @@ def __init__(self):
>  optparse.make_option("--export-labels", dest="exportLabels", 
> action="store_true"),
>  optparse.make_option("--dry-run", "-n", dest="dry_run", 
> action="store_true"),
>  optparse.make_option("--prepare-p4-only", 
> dest="prepare_p4_only", action="store_true"),
> +optparse.make_option("--shelve-only", dest="shelve_only", 
> action="store_true", help="Create P4 shelf for first change that would be 
> submitted (using a new CL)"),
> +optparse.make_option("--shelve-cl", dest="shelve_cl", 
> help="Replace shelf under existing CL number (previously shelved files will 
> be deleted)"),
>  optparse.make_option("--conflict", dest="conflict_behavior",
>   choices=self.conflict_behavior_choices),
>  optparse.make_option("--branch", dest="branch"),
> @@ -1297,6 +1299,8 @@ def __init__(self):
>  self.preserveUser = gitConfigBool("git-p4.preserveUser")
>  self.dry_run = False
>  self.prepare_p4_only = False
> +self.shelve_only = False
> +self.shelve_cl = None
>  self.conflict_behavior = None
>  self.isWindows = (platform.system() == "Windows")
>  self.exportLabels = False
> @@ -1496,6 +1500,12 @@ def prepareSubmitTemplate(self):
>  else:
>  inFilesSection = False
>  else:
> +if self.shelve_only and self.shelve_cl:
> +if line.startswith("Change:"):
> +line = "Change: %s\n" % self.shelve_cl
> +if line.startswith("Status:"):
> +line = "Status: pending\n"
> +
>  if line.startswith("Files:"):
>  inFilesSection = True
>
> @@ -1785,7 +1795,11 @@ def applyCommit(self, id):
>  if self.isWindows:
>  message = message.replace("\r\n", "\n")
>  submitTemplate = message[:message.index(separatorLine)]
> -p4_write_pipe(['submit', '-i'], submitTemplate)
> +
> +if self.shelve_only:
> +p4_write_pipe(['shelve', '-i', '-r'], submitTemplate)
> +else:
> +p4_write_pipe(['submit', '-i'], submitTemplate)
>
>  if self.preserveUser:
>  if p4User:
> @@ -1799,12 +1813,17 @@ def applyCommit(self, id):
>  # new file.  This leaves it writable, which confuses p4.
>  for f in pureRenameCopy:
>  p4_sync(f, "-f")
> -submitted = True
> +
> +if not self.shelve_only:
> +submitted = True
>
>  finally:
>  # skip this patch
>  if not submitted:
> -print "Submission cancelled, undoing p4 changes."
> +if not self.shelve_only:
> +print "Submission cancelled, undoing p4 changes."
> +else:
> +print "Change shelved, undoing p4 changes."
>  for f in editedFiles:
>  p4_revert(f)
>  for f in filesToAdd:
> @@ -2034,9 +2053,13 @@ def run(self, args):
>  if ok:
>  applied.append(commit)
>  else:
> -if self.prepare_p4_only and i < last:
> -print "Processing only the first commit due to option" \
> -  " --prepare-p4-only"
> +if (self.prepare_p4_only or self.shelve_only) and i < last:
> +if self.prepare_p4_only:
> +print "Processing only the first commit due to 
> option" \
> +  " --prepare-p4-only"
> +else:
> +print "Processing only the first commit due to 
> option" \
> +  " --shelve-only"
>  break
>  if i < last:
>  quit = False
> @@ -3638,6 +3661,7 @@ def printUsage(commands):
>  "debug" : P4Debug,
>  "submit" : P4Submit,
>  "commit" : P4Submit,
> +"shelve" : P4Submit,
>  "sync" : P4Sync,
>  "rebase" : P4Rebase,
>  "clone" : P4Clone,
>
> --
> 

Re: [PATCH v1] git-p4: add config to retry p4 commands; retry 3 times by default

[Luke Diamand]

On 4 December 2016 at 14:03,   wrote:
> From: Lars Schneider 
>
> P4 commands can fail due to random network issues. P4 users can counter
> these issues by using a retry flag supported by all p4 commands [1].
>
> Add an integer Git config value `git-p4.retries` to define the number of
> retries for all p4 invocations. If the config is not defined then set
> the default retry count to 3.

Looks good to me, ack.

>
> [1] 
> https://www.perforce.com/perforce/doc.current/manuals/cmdref/global.options.html
>
> Signed-off-by: Lars Schneider 
> ---
>
> Notes:
> Base Commit: 454cb6b (v2.11.0)
> Diff on Web: 
> https://github.com/git/git/compare/454cb6b...larsxschneider:654c727
> Checkout:git fetch https://github.com/larsxschneider/git 
> git-p4/retries-v1 && git checkout 654c727
>
>  Documentation/git-p4.txt | 4 
>  git-p4.py| 5 +
>  2 files changed, 9 insertions(+)
>
> diff --git a/Documentation/git-p4.txt b/Documentation/git-p4.txt
> index c83aaf39c3..656587248c 100644
> --- a/Documentation/git-p4.txt
> +++ b/Documentation/git-p4.txt
> @@ -467,6 +467,10 @@ git-p4.client::
> Client specified as an option to all p4 commands, with
> '-c ', including the client spec.
>
> +git-p4.retries::
> +   Specifies the number of times to retry a p4 command (notably,
> +   'p4 sync') if the network times out. The default value is 3.
> +
>  Clone and sync variables
>  
>  git-p4.syncFromOrigin::
> diff --git a/git-p4.py b/git-p4.py
> index fd5ca52462..2422178210 100755
> --- a/git-p4.py
> +++ b/git-p4.py
> @@ -78,6 +78,11 @@ def p4_build_cmd(cmd):
>  if len(client) > 0:
>  real_cmd += ["-c", client]
>
> +retries = gitConfigInt("git-p4.retries")
> +if retries is None:
> +# Perform 3 retries by default
> +retries = 3
> +real_cmd += ["-r", str(retries)]
>
>  if isinstance(cmd,basestring):
>  real_cmd = ' '.join(real_cmd) + ' ' + cmd
> --
> 2.11.0
>

Re: [PATCH v4 2/2] t9813: avoid using pipes

[Luke Diamand]

On 8 January 2017 at 16:55, Pranit Bauva  wrote:
> The exit code of the upstream in a pipe is ignored thus we should avoid
> using it. By writing out the output of the git command to a file, we can
> test the exit codes of both the commands.

Looks good to me, thanks!

Ack.

>
> Signed-off-by: Pranit Bauva 
> ---
>  t/t9813-git-p4-preserve-users.sh | 8 
>  1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/t/t9813-git-p4-preserve-users.sh 
> b/t/t9813-git-p4-preserve-users.sh
> index 76004a5..bda222a 100755
> --- a/t/t9813-git-p4-preserve-users.sh
> +++ b/t/t9813-git-p4-preserve-users.sh
> @@ -118,12 +118,12 @@ test_expect_success 'not preserving user with mixed 
> authorship' '
> make_change_by_user usernamefile3 Derek de...@example.com &&
> P4EDITOR=cat P4USER=alice P4PASSWD=secret &&
> export P4EDITOR P4USER P4PASSWD &&
> -   git p4 commit |\
> -   grep "git author de...@example.com does not match" &&
> +   git p4 commit >actual &&
> +   grep "git author de...@example.com does not match" actual &&
>
> make_change_by_user usernamefile3 Charlie char...@example.com 
> &&
> -   git p4 commit |\
> -   grep "git author char...@example.com does not match" &&
> +   git p4 commit >actual &&
> +   grep "git author char...@example.com does not match" actual &&
>
> make_change_by_user usernamefile3 alice al...@example.com &&
> git p4 commit >actual &&
>
> --
> https://github.com/git/git/pull/314

Re: [PATCH] git-p4: do not pass '-r 0' to p4 commands

[Luke Diamand]

On 29 December 2016 at 09:05, Igor Kushnir  wrote:
> git-p4 crashes when used with a very old p4 client version
> that does not support the '-r ' option in its commands.
>
> Allow making git-p4 work with old p4 clients by setting git-p4.retries to 0.
>
> Alternatively git-p4.retries could be made opt-in.
> But since only very old, barely maintained p4 versions don't support
> the '-r' option, the setting-retries-to-0 workaround would do.
>
> The "-r retries" option is present in Perforce 2012.2 Command Reference,
> but absent from Perforce 2012.1 Command Reference.

That looks like a good fix, thanks.

I feel sad for anyone still using 2012.1.

Luke


>
> Signed-off-by: Igor Kushnir 
> ---
>  Documentation/git-p4.txt | 1 +
>  git-p4.py| 4 +++-
>  2 files changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/git-p4.txt b/Documentation/git-p4.txt
> index bae862ddc..f4f1be5be 100644
> --- a/Documentation/git-p4.txt
> +++ b/Documentation/git-p4.txt
> @@ -479,6 +479,7 @@ git-p4.client::
>  git-p4.retries::
> Specifies the number of times to retry a p4 command (notably,
> 'p4 sync') if the network times out. The default value is 3.
> +   '-r 0' is not passed to p4 commands if this option is set to 0.
>
>  Clone and sync variables
>  
> diff --git a/git-p4.py b/git-p4.py
> index 22e3f57e7..e5a9e1cce 100755
> --- a/git-p4.py
> +++ b/git-p4.py
> @@ -83,7 +83,9 @@ def p4_build_cmd(cmd):
>  if retries is None:
>  # Perform 3 retries by default
>  retries = 3
> -real_cmd += ["-r", str(retries)]
> +if retries != 0:
> +# Provide a way to not pass this option by setting git-p4.retries to > 0
> +real_cmd += ["-r", str(retries)]
>
>  if isinstance(cmd,basestring):
>  real_cmd = ' '.join(real_cmd) + ' ' + cmd
> --
> 2.11.0
>

Re: [PATCH] don't use test_must_fail with grep

[Luke Diamand]

On 31 December 2016 at 11:44, Pranit Bauva  wrote:
> test_must_fail should only be used for testing git commands. To test the
> failure of other commands use `!`.
>
> Reported-by: Stefan Beller 
> Signed-off-by: Pranit Bauva 
> ---
>  t/t3510-cherry-pick-sequence.sh  |  6 +++---
>  t/t5504-fetch-receive-strict.sh  |  2 +-
>  t/t5516-fetch-push.sh|  2 +-
>  t/t5601-clone.sh |  2 +-
>  t/t6030-bisect-porcelain.sh  |  2 +-
>  t/t7610-mergetool.sh |  2 +-
>  t/t9001-send-email.sh|  2 +-
>  t/t9117-git-svn-init-clone.sh| 12 ++--
>  t/t9813-git-p4-preserve-users.sh |  8 
>  t/t9814-git-p4-rename.sh |  6 +++---
>  10 files changed, 22 insertions(+), 22 deletions(-)
>
> diff --git a/t/t3510-cherry-pick-sequence.sh b/t/t3510-cherry-pick-sequence.sh
> index 372307c21..0acf4b146 100755
> --- a/t/t3510-cherry-pick-sequence.sh
> +++ b/t/t3510-cherry-pick-sequence.sh
> @@ -385,7 +385,7 @@ test_expect_success '--continue respects opts' '
> git cat-file commit HEAD~1 >picked_msg &&
> git cat-file commit HEAD~2 >unrelatedpick_msg &&
> git cat-file commit HEAD~3 >initial_msg &&
> -   test_must_fail grep "cherry picked from" initial_msg &&
> +   ! grep "cherry picked from" initial_msg &&
> grep "cherry picked from" unrelatedpick_msg &&
> grep "cherry picked from" picked_msg &&
> grep "cherry picked from" anotherpick_msg
> @@ -426,9 +426,9 @@ test_expect_failure '--signoff is automatically 
> propagated to resolved conflict'
> git cat-file commit HEAD~1 >picked_msg &&
> git cat-file commit HEAD~2 >unrelatedpick_msg &&
> git cat-file commit HEAD~3 >initial_msg &&
> -   test_must_fail grep "Signed-off-by:" initial_msg &&
> +   ! grep "Signed-off-by:" initial_msg &&
> grep "Signed-off-by:" unrelatedpick_msg &&
> -   test_must_fail grep "Signed-off-by:" picked_msg &&
> +   ! grep "Signed-off-by:" picked_msg &&
> grep "Signed-off-by:" anotherpick_msg
>  '
>
> diff --git a/t/t5504-fetch-receive-strict.sh b/t/t5504-fetch-receive-strict.sh
> index 9b19cff72..49d3621a9 100755
> --- a/t/t5504-fetch-receive-strict.sh
> +++ b/t/t5504-fetch-receive-strict.sh
> @@ -152,7 +152,7 @@ test_expect_success 'push with 
> receive.fsck.missingEmail=warn' '
> git --git-dir=dst/.git config --add \
> receive.fsck.badDate warn &&
> git push --porcelain dst bogus >act 2>&1 &&
> -   test_must_fail grep "missingEmail" act
> +   ! grep "missingEmail" act
>  '
>
>  test_expect_success \
> diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
> index 26b2cafc4..0fc5a7c59 100755
> --- a/t/t5516-fetch-push.sh
> +++ b/t/t5516-fetch-push.sh
> @@ -1004,7 +1004,7 @@ test_expect_success 'push --porcelain' '
>  test_expect_success 'push --porcelain bad url' '
> mk_empty testrepo &&
> test_must_fail git push >.git/bar --porcelain asdfasdfasd 
> refs/heads/master:refs/remotes/origin/master &&
> -   test_must_fail grep -q Done .git/bar
> +   ! grep -q Done .git/bar
>  '
>
>  test_expect_success 'push --porcelain rejected' '
> diff --git a/t/t5601-clone.sh b/t/t5601-clone.sh
> index a43339420..4241ea5b3 100755
> --- a/t/t5601-clone.sh
> +++ b/t/t5601-clone.sh
> @@ -151,7 +151,7 @@ test_expect_success 'clone --mirror does not repeat tags' 
> '
> git clone --mirror src mirror2 &&
> (cd mirror2 &&
>  git show-ref 2> clone.err > clone.out) &&
> -   test_must_fail grep Duplicate mirror2/clone.err &&
> +   ! grep Duplicate mirror2/clone.err &&
> grep some-tag mirror2/clone.out
>
>  '
> diff --git a/t/t6030-bisect-porcelain.sh b/t/t6030-bisect-porcelain.sh
> index 5e5370feb..8c2c6eaef 100755
> --- a/t/t6030-bisect-porcelain.sh
> +++ b/t/t6030-bisect-porcelain.sh
> @@ -407,7 +407,7 @@ test_expect_success 'good merge base when good and bad 
> are siblings' '
> test_i18ngrep "merge base must be tested" my_bisect_log.txt &&
> grep $HASH4 my_bisect_log.txt &&
> git bisect good > my_bisect_log.txt &&
> -   test_must_fail grep "merge base must be tested" my_bisect_log.txt &&
> +   ! grep "merge base must be tested" my_bisect_log.txt &&
> grep $HASH6 my_bisect_log.txt &&
> git bisect reset
>  '
> diff --git a/t/t7610-mergetool.sh b/t/t7610-mergetool.sh
> index 63d36fb28..0fe7e58cf 100755
> --- a/t/t7610-mergetool.sh
> +++ b/t/t7610-mergetool.sh
> @@ -602,7 +602,7 @@ test_expect_success MKTEMP 'temporary filenames are used 
> with mergetool.writeToT
> test_config mergetool.myecho.trustExitCode true &&
> test_must_fail git merge master &&
> git mergetool --no-prompt --tool myecho -- both >actual &&
> -   test_must_fail grep ^\./both_LOCAL_ actual >/dev/null &&
> +   ! grep ^\./both_LOCAL_ actual >/dev/null &&
> grep /both_LOCAL_ 

Re: [PATCH] don't use test_must_fail with grep

[Luke Diamand]

On 1 January 2017 at 14:50, Johannes Sixt <j...@kdbg.org> wrote:
> Am 01.01.2017 um 15:23 schrieb Luke Diamand:
>>
>> On 31 December 2016 at 11:44, Pranit Bauva <pranit.ba...@gmail.com> wrote:
>>>
>>> diff --git a/t/t9813-git-p4-preserve-users.sh
>>> b/t/t9813-git-p4-preserve-users.sh
>>> index 0fe231280..2384535a7 100755
>>> --- a/t/t9813-git-p4-preserve-users.sh
>>> +++ b/t/t9813-git-p4-preserve-users.sh
>>> @@ -126,13 +126,13 @@ test_expect_success 'not preserving user with mixed
>>> authorship' '
>>> grep "git author char...@example.com does not match" &&
>>>
>>> make_change_by_user usernamefile3 alice al...@example.com
>>> &&
>>> -   git p4 commit |\
>>> -   test_must_fail grep "git author.*does not match" &&
>>> +   ! git p4 commit |\
>>> +   grep "git author.*does not match" &&
>>
>>
>> Would it be clearer to use this?
>>
>> git p4 commit |\
>> grep -q -v "git author.*does not match" &&
>>
>> With your original change, I think that if "git p4 commit" fails, then
>> that expression will be treated as a pass.
>
>
> No. The exit code of the upstream in a pipe is ignored. For this reason,
> having a git invocation as the upstream of a pipe *anywhere* in the test
> suite is frowned upon. Hence, a better rewrite would be
>
> git p4 commit >actual &&
> ! grep "git author.*does not match" actual &&
>
> which makes me wonder: Is the message that we do expect not to occur
> actually printed on stdout? It sounds much more like an error message, i.e.,
> text that is printed on stderr. Wouldn't we need this?
>
> git p4 commit >actual 2>&1 &&
> ! grep "git author.*does not match" actual &&

The message is actually part of a template presented to the user via
their chosen editor. For this test, we set the editor to be "cat", so
it comes out on stdout.

Your first suggestion would therefore be fine (and similarly for the
other cases).

Re: [PATCH v3 2/2] t9813: avoid using pipes

[Luke Diamand]

On 3 January 2017 at 19:57, Pranit Bauva  wrote:
> The exit code of the upstream in a pipe is ignored thus we should avoid
> using it. By writing out the output of the git command to a file, we can
> test the exit codes of both the commands.

Do we also need to fix t9814-git-p4-rename.sh ?

>
> Signed-off-by: Pranit Bauva 
> ---
>  t/t9813-git-p4-preserve-users.sh | 8 
>  1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/t/t9813-git-p4-preserve-users.sh 
> b/t/t9813-git-p4-preserve-users.sh
> index 798bf2b67..2133b21ae 100755
> --- a/t/t9813-git-p4-preserve-users.sh
> +++ b/t/t9813-git-p4-preserve-users.sh
> @@ -118,12 +118,12 @@ test_expect_success 'not preserving user with mixed 
> authorship' '
> make_change_by_user usernamefile3 Derek de...@example.com &&
> P4EDITOR=cat P4USER=alice P4PASSWD=secret &&
> export P4EDITOR P4USER P4PASSWD &&
> -   git p4 commit |\
> -   grep "git author de...@example.com does not match" &&
> +   git p4 commit >actual &&
> +   grep "git author de...@example.com does not match" actual &&
>
> make_change_by_user usernamefile3 Charlie char...@example.com 
> &&
> -   git p4 commit |\
> -   grep "git author char...@example.com does not match" &&
> +   git p4 commit >actual &&
> +   grep "git author char...@example.com does not match" actual &&
>
> make_change_by_user usernamefile3 alice al...@example.com &&
> git p4 commit >actual 2>&1 &&
> --
> 2.11.0
>

Re: [PATCH v2] git-p4: do not pass '-r 0' to p4 commands

[Luke Diamand]

On 3 January 2017 at 20:02, Ori Rawlings  wrote:
> Looks good to me.

And me.



>
>
> Ori Rawlings

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

[Luke Diamand]

On 19 December 2016 at 17:49, Junio C Hamano  wrote:
> George Vanburgh  writes:
>
>> From: George Vanburgh 
>>
>> When importing from multiple perforce paths - we may attempt to import
>> a changelist that contains files from two (or more) of these depot
>> paths. Currently, this results in multiple git commits - one
>> containing the changes, and the other(s) as empty commit(s). This
>> behavior was introduced in commit 1f90a64
>> ("git-p4: reduce number of server queries for fetches", 2015-12-19).
>>
>> Reproduction Steps:
>>
>> 1. Have a git repo cloned from a perforce repo using multiple depot
>> paths (e.g. //depot/foo and //depot/bar).
>> 2. Submit a single change to the perforce repo that makes changes in
>> both //depot/foo and //depot/bar.
>> 3. Run "git p4 sync" to sync the change from #2.
>>
>> Change is synced as multiple commits, one for each depot path that was
>> affected.
>>
>> Using a set, instead of a list inside p4ChangesForPaths() ensures that
>> each changelist is unique to the returned list, and therefore only a
>> single commit is generated for each changelist.
>>
>> Reported-by: James Farwell 
>> Signed-off-by: George Vanburgh 
>> ---
>
> Thanks, George.  Luke, can I add your "Reviewed-by:" here?

Yes, thanks.

Luke

Re: [PATCH v1] git-p4: fix git-p4.pathEncoding for removed files

[Luke Diamand]

On 19 December 2016 at 21:29, Junio C Hamano  wrote:
> larsxschnei...@gmail.com writes:
>
>> From: Lars Schneider 
>>
>> In a9e38359e3 we taught git-p4 a way to re-encode path names from what
>> was used in Perforce to UTF-8. This path re-encoding worked properly for
>> "added" paths. "Removed" paths were not re-encoded and therefore
>> different from the "added" paths. Consequently, these files were not
>> removed in a git-p4 cloned Git repository because the path names did not
>> match.
>>
>> Fix this by moving the re-encoding to a place that affects "added" and
>> "removed" paths. Add a test to demonstrate the issue.
>>
>> Signed-off-by: Lars Schneider 
>> ---
>
> Thanks.
>
> The above description makes me wonder what happens to "modified"
> paths, but presumably they are handled in a separate codepath?  Or
> does this also cover not just "removed" but also paths with any
> change?
>
> Luke, does this look good?

I'm not totally sure. In the previous version the conversion happened
in streamOneP4File(). There is a counterpart to this,
streamOneP4Deletion() which would seem like the callpoint that needs
to know about this.

The change puts the logic into stripRepoPath() instead, which is
indeed called from both of those functions (good), but also from
splitFilesIntoBranches(), but only if self.useClientSpec is set. That
function only gets used if we're doing the automatic branch detection
logic, so it's possible that this code might now be broken and we
wouldn't know.

Lars, what do you think? Other than the above, the change looks good,
so it may all be fine.

(As an aside, this is the heart of the code that's going to need some
careful rework if/when we ever move to Python3).

Luke

Re: [PATCH v1] git-p4: add diff/merge properties to .gitattributes for GitLFS files

[Luke Diamand]

On 19 December 2016 at 21:29, Junio C Hamano  wrote:
> larsxschnei...@gmail.com writes:
>
>> From: Lars Schneider 
>>
>> The `git lfs track` command generates a .gitattributes file with diff
>> and merge properties [1]. Set the same .gitattributes format for files
>> tracked with GitLFS in git-p4.
>>
>> [1] 
>> https://github.com/git-lfs/git-lfs/blob/v1.5.3/commands/command_track.go#L121
>>
>> Signed-off-by: Lars Schneider 
>> ---
>
> Thanks.  Luke, does this look ok?

Yes, looks good to me.

Luke


>
>>
>> Notes:
>> Base Commit: d1271bddd4 (v2.11.0)
>> Diff on Web: 
>> https://github.com/git/git/compare/d1271bddd4...larsxschneider:e045b3d5c8
>> Checkout:git fetch https://github.com/larsxschneider/git 
>> git-p4/fix-lfs-attributes-v1 && git checkout e045b3d5c8
>>
>>  git-p4.py |  4 ++--
>>  t/t9824-git-p4-git-lfs.sh | 24 
>>  2 files changed, 14 insertions(+), 14 deletions(-)
>>
>> diff --git a/git-p4.py b/git-p4.py
>> index fd5ca52462..87b6932c81 100755
>> --- a/git-p4.py
>> +++ b/git-p4.py
>> @@ -1098,10 +1098,10 @@ class GitLFS(LargeFileSystem):
>>  '# Git LFS (see https://git-lfs.github.com/)\n',
>>  '#\n',
>>  ] +
>> -['*.' + f.replace(' ', '[[:space:]]') + ' filter=lfs -text\n'
>> +['*.' + f.replace(' ', '[[:space:]]') + ' filter=lfs diff=lfs 
>> merge=lfs -text\n'
>>  for f in sorted(gitConfigList('git-p4.largeFileExtensions'))
>>  ] +
>> -['/' + f.replace(' ', '[[:space:]]') + ' filter=lfs -text\n'
>> +['/' + f.replace(' ', '[[:space:]]') + ' filter=lfs diff=lfs 
>> merge=lfs -text\n'
>>  for f in sorted(self.largeFiles) if not 
>> self.hasLargeFileExtension(f)
>>  ]
>>  )
>> diff --git a/t/t9824-git-p4-git-lfs.sh b/t/t9824-git-p4-git-lfs.sh
>> index 110a7e7924..1379db6357 100755
>> --- a/t/t9824-git-p4-git-lfs.sh
>> +++ b/t/t9824-git-p4-git-lfs.sh
>> @@ -81,9 +81,9 @@ test_expect_success 'Store files in LFS based on size (>24 
>> bytes)' '
>>   #
>>   # Git LFS (see https://git-lfs.github.com/)
>>   #
>> - /file2.dat filter=lfs -text
>> - /file4.bin filter=lfs -text
>> - /path[[:space:]]with[[:space:]]spaces/file3.bin filter=lfs 
>> -text
>> + /file2.dat filter=lfs diff=lfs merge=lfs -text
>> + /file4.bin filter=lfs diff=lfs merge=lfs -text
>> + /path[[:space:]]with[[:space:]]spaces/file3.bin filter=lfs 
>> diff=lfs merge=lfs -text
>>   EOF
>>   test_path_is_file .gitattributes &&
>>   test_cmp expect .gitattributes
>> @@ -109,7 +109,7 @@ test_expect_success 'Store files in LFS based on size 
>> (>25 bytes)' '
>>   #
>>   # Git LFS (see https://git-lfs.github.com/)
>>   #
>> - /file4.bin filter=lfs -text
>> + /file4.bin filter=lfs diff=lfs merge=lfs -text
>>   EOF
>>   test_path_is_file .gitattributes &&
>>   test_cmp expect .gitattributes
>> @@ -135,7 +135,7 @@ test_expect_success 'Store files in LFS based on 
>> extension (dat)' '
>>   #
>>   # Git LFS (see https://git-lfs.github.com/)
>>   #
>> - *.dat filter=lfs -text
>> + *.dat filter=lfs diff=lfs merge=lfs -text
>>   EOF
>>   test_path_is_file .gitattributes &&
>>   test_cmp expect .gitattributes
>> @@ -163,8 +163,8 @@ test_expect_success 'Store files in LFS based on size 
>> (>25 bytes) and extension
>>   #
>>   # Git LFS (see https://git-lfs.github.com/)
>>   #
>> - *.dat filter=lfs -text
>> - /file4.bin filter=lfs -text
>> + *.dat filter=lfs diff=lfs merge=lfs -text
>> + /file4.bin filter=lfs diff=lfs merge=lfs -text
>>   EOF
>>   test_path_is_file .gitattributes &&
>>   test_cmp expect .gitattributes
>> @@ -199,8 +199,8 @@ test_expect_success 'Remove file from repo and store 
>> files in LFS based on size
>>   #
>>   # Git LFS (see https://git-lfs.github.com/)
>>   #
>> - /file2.dat filter=lfs -text
>> - /path[[:space:]]with[[:space:]]spaces/file3.bin filter=lfs 
>> -text
>> + /file2.dat filter=lfs diff=lfs merge=lfs -text
>> + /path[[:space:]]with[[:space:]]spaces/file3.bin filter=lfs 
>> diff=lfs merge=lfs -text
>>   EOF
>>   test_path_is_file .gitattributes &&
>>   test_cmp expect .gitattributes
>> @@ -237,8 +237,8 @@ test_expect_success 'Add .gitattributes and store files 
>> in LFS based on size (>2
>>   #
>>   # Git LFS (see https://git-lfs.github.com/)
>>  

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

[Luke Diamand]

On 15 December 2016 at 17:14, George Vanburgh  wrote:
> From: George Vanburgh 
>
> When importing from multiple perforce paths - we may attempt to import a 
> changelist that contains files from two (or more) of these depot paths. 
> Currently, this results in multiple git commits - one containing the changes, 
> and the other(s) as empty commits. This behavior was introduced in commit 
> 1f90a64 ("git-p4: reduce number of server queries for fetches", 2015-12-19).

That's definitely a bug, thanks for spotting that! Even more so for
adding a test case.

>
> Reproduction Steps:
>
> 1. Have a git repo cloned from a perforce repo using multiple depot paths 
> (e.g. //depot/foo and //depot/bar).
> 2. Submit a single change to the perforce repo that makes changes in both 
> //depot/foo and //depot/bar.
> 3. Run "git p4 sync" to sync the change from #2.
>
> Change is synced as multiple commits, one for each depot path that was 
> affected.
>
> Using a set, instead of a list inside p4ChangesForPaths() ensures that each 
> changelist is unique to the returned list, and therefore only a single commit 
> is generated for each changelist.

The change looks good to me apart from one missing "&&" in the test
case (see below).

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

Luke


>
> Reported-by: James Farwell 
> Signed-off-by: George Vanburgh 
> ---
>  git-p4.py   |  4 ++--
>  t/t9800-git-p4-basic.sh | 22 +-
>  2 files changed, 23 insertions(+), 3 deletions(-)
>
> diff --git a/git-p4.py b/git-p4.py
> index fd5ca52..6307bc8 100755
> --- a/git-p4.py
> +++ b/git-p4.py
> @@ -822,7 +822,7 @@ def p4ChangesForPaths(depotPaths, changeRange, 
> requestedBlockSize):
>  die("cannot use --changes-block-size with non-numeric 
> revisions")
>  block_size = None
>
> -changes = []
> +changes = set()
>
>  # Retrieve changes a block at a time, to prevent running
>  # into a MaxResults/MaxScanRows error from the server.
> @@ -841,7 +841,7 @@ def p4ChangesForPaths(depotPaths, changeRange, 
> requestedBlockSize):
>
>  # Insert changes in chronological order
>  for line in reversed(p4_read_pipe_lines(cmd)):
> -changes.append(int(line.split(" ")[1]))
> +changes.add(int(line.split(" ")[1]))
>
>  if not block_size:
>  break
> diff --git a/t/t9800-git-p4-basic.sh b/t/t9800-git-p4-basic.sh
> index 0730f18..4d72e0b 100755
> --- a/t/t9800-git-p4-basic.sh
> +++ b/t/t9800-git-p4-basic.sh
> @@ -131,6 +131,26 @@ test_expect_success 'clone two dirs, @all, conflicting 
> files' '
> )
>  '
>
> +test_expect_success 'clone two dirs, each edited by submit, single git 
> commit' '
> +   (
> +   cd "$cli" &&
> +   echo sub1/f4 >sub1/f4 &&
> +   p4 add sub1/f4 &&
> +   echo sub2/f4 >sub2/f4 &&
> +   p4 add sub2/f4 &&
> +   p4 submit -d "sub1/f4 and sub2/f4"
> +   ) &&
> +   git p4 clone --dest="$git" //depot/sub1@all //depot/sub2@all &&
> +   test_when_finished cleanup_git &&
> +   (
> +   cd "$git"

Missing &&

> +   git ls-files >lines &&
> +   test_line_count = 4 lines &&
> +   git log --oneline p4/master >lines &&
> +   test_line_count = 5 lines
> +   )
> +'
> +
>  revision_ranges="2000/01/01,#head \
>  1,2080/01/01 \
>  2000/01/01,2080/01/01 \
> @@ -147,7 +167,7 @@ test_expect_success 'clone using non-numeric revision 
> ranges' '
> (
> cd "$git" &&
> git ls-files >lines &&
> -   test_line_count = 6 lines
> +   test_line_count = 8 lines
> )
> done
>  '
>
> --
> https://github.com/git/git/pull/311

[PATCHv2] git-p4: handle symlinked directories

[Luke Diamand]

This is an updated version of my earlier git-p4 symlink fix.

This one now treats addition of all symlinks in the same way,
rather than displaying the target file if linking to a file,
or just the link target name if it's a directory.

That makes the git-p4 summary behave more like normal git
commands (which also treat symlinks uniformaly).

This is a very slight change in behaviour, but I don't think
it can break anything since it is only when generating
the summary that goes after the P4 change template.

Luke Diamand (1):
  git-p4: avoid crash adding symlinked directory

 git-p4.py | 26 --
 t/t9830-git-p4-symlink-dir.sh | 43 +++
 2 files changed, 63 insertions(+), 6 deletions(-)
 create mode 100755 t/t9830-git-p4-symlink-dir.sh

-- 
2.8.2.703.g78b384c.dirty

[PATCHv2] git-p4: avoid crash adding symlinked directory

[Luke Diamand]

When submitting to P4, if git-p4 came across a symlinked
directory, then during the generation of the submit diff, it would
try to open it as a normal file and fail.

Spot symlinks (of any type) and output a description of the symlink
instead.

Add a test case.

Signed-off-by: Luke Diamand <l...@diamand.org>
---
 git-p4.py | 26 --
 t/t9830-git-p4-symlink-dir.sh | 43 +++
 2 files changed, 63 insertions(+), 6 deletions(-)
 create mode 100755 t/t9830-git-p4-symlink-dir.sh

diff --git a/git-p4.py b/git-p4.py
index fd5ca52..16d0b8a 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -25,6 +25,7 @@ import stat
 import zipfile
 import zlib
 import ctypes
+import errno
 
 try:
 from subprocess import CalledProcessError
@@ -1538,7 +1539,7 @@ class P4Submit(Command, P4UserMap):
 if response == 'n':
 return False
 
-def get_diff_description(self, editedFiles, filesToAdd):
+def get_diff_description(self, editedFiles, filesToAdd, symlinks):
 # diff
 if os.environ.has_key("P4DIFF"):
 del(os.environ["P4DIFF"])
@@ -1553,10 +1554,17 @@ class P4Submit(Command, P4UserMap):
 newdiff += " new file \n"
 newdiff += "--- /dev/null\n"
 newdiff += "+++ %s\n" % newFile
-f = open(newFile, "r")
-for line in f.readlines():
-newdiff += "+" + line
-f.close()
+
+is_link = os.path.islink(newFile)
+expect_link = newFile in symlinks
+
+if is_link and expect_link:
+newdiff += "+%s\n" % os.readlink(newFile)
+else:
+f = open(newFile, "r")
+for line in f.readlines():
+newdiff += "+" + line
+f.close()
 
 return (diff + newdiff).replace('\r\n', '\n')
 
@@ -1574,6 +1582,7 @@ class P4Submit(Command, P4UserMap):
 filesToDelete = set()
 editedFiles = set()
 pureRenameCopy = set()
+symlinks = set()
 filesToChangeExecBit = {}
 
 for line in diff:
@@ -1590,6 +1599,11 @@ class P4Submit(Command, P4UserMap):
 filesToChangeExecBit[path] = diff['dst_mode']
 if path in filesToDelete:
 filesToDelete.remove(path)
+
+dst_mode = int(diff['dst_mode'], 8)
+if dst_mode == 012:
+symlinks.add(path)
+
 elif modifier == "D":
 filesToDelete.add(path)
 if path in filesToAdd:
@@ -1727,7 +1741,7 @@ class P4Submit(Command, P4UserMap):
 separatorLine = " everything below this line is just the diff 
###\n"
 if not self.prepare_p4_only:
 submitTemplate += separatorLine
-submitTemplate += self.get_diff_description(editedFiles, 
filesToAdd)
+submitTemplate += self.get_diff_description(editedFiles, 
filesToAdd, symlinks)
 
 (handle, fileName) = tempfile.mkstemp()
 tmpFile = os.fdopen(handle, "w+b")
diff --git a/t/t9830-git-p4-symlink-dir.sh b/t/t9830-git-p4-symlink-dir.sh
new file mode 100755
index 000..3dc528b
--- /dev/null
+++ b/t/t9830-git-p4-symlink-dir.sh
@@ -0,0 +1,43 @@
+#!/bin/sh
+
+test_description='git p4 symlinked directories'
+
+. ./lib-git-p4.sh
+
+test_expect_success 'start p4d' '
+   start_p4d
+'
+
+test_expect_success 'symlinked directory' '
+   (
+   cd "$cli" &&
+   : >first_file.t &&
+   p4 add first_file.t &&
+   p4 submit -d "first change"
+   ) &&
+   git p4 clone --dest "$git" //depot &&
+   (
+   cd "$git" &&
+   mkdir -p some/sub/directory &&
+   mkdir -p other/subdir2 &&
+   : > other/subdir2/file.t &&
+   (cd some/sub/directory && ln -s ../../../other/subdir2 .) &&
+   git add some other &&
+   git commit -m "symlinks" &&
+   git config git-p4.skipSubmitEdit true &&
+   git p4 submit -v
+   ) &&
+   (
+   cd "$cli" &&
+   p4 sync &&
+   test -L some/sub/directory/subdir2
+   test_path_is_file some/sub/directory/subdir2/file.t
+   )
+
+'
+
+test_expect_success 'kill p4d' '
+   kill_p4d
+'
+
+test_done
-- 
2.8.2.703.g78b384c.dirty

[PATCH] git-p4: Add failing test case for name-rev vs symbolic-ref

[Luke Diamand]

As per the discussion about use of "git name-rev" vs "git symbolic-ref" in
git-p4 here:

http://marc.info/?l=git=148979063421355

git-p4 could get confused about the branch it is on and affects
the git-p4.allowSubmit  option. This adds a failing
test case for the problem.

Luke Diamand (1):
  git-p4: add failing test for name-rev rather than symbolic-ref

 t/t9807-git-p4-submit.sh | 16 
 1 file changed, 16 insertions(+)

-- 
2.8.2.703.g78b384c.dirty

[PATCH] git-p4: add failing test for name-rev rather than symbolic-ref

[Luke Diamand]

Using name-rev to find the current git branch means that git-p4
does not correctly get the current branch name if there are
multiple branches pointing at HEAD, or a tag.

This change adds a test case which demonstrates the problem.
Configuring which branches are allowed to be submitted from goes
wrong, as git-p4 gets confused about which branch is in use.

This appears to be the only place that git-p4 actually cares
about the current branch.

Signed-off-by: Luke Diamand <l...@diamand.org>
---
 t/t9807-git-p4-submit.sh | 16 
 1 file changed, 16 insertions(+)

diff --git a/t/t9807-git-p4-submit.sh b/t/t9807-git-p4-submit.sh
index e37239e..ae05816 100755
--- a/t/t9807-git-p4-submit.sh
+++ b/t/t9807-git-p4-submit.sh
@@ -139,6 +139,22 @@ test_expect_success 'submit with master branch name from 
argv' '
)
 '
 
+test_expect_failure 'allow submit from branch with same revision but different 
name' '
+   test_when_finished cleanup_git &&
+   git p4 clone --dest="$git" //depot &&
+   (
+   cd "$git" &&
+   test_commit "file8" &&
+   git checkout -b branch1 &&
+   git checkout -b branch2 &&
+   git config git-p4.skipSubmitEdit true &&
+   git config git-p4.allowSubmit "branch1" &&
+   test_must_fail git p4 submit &&
+   git checkout branch1 &&
+   git p4 submit
+   )
+'
+
 #
 # Basic submit tests, the five handled cases
 #
-- 
2.8.2.703.g78b384c.dirty

Re: [PATCH] git-p4: Add failing test case for name-rev vs symbolic-ref

[Luke Diamand]

On 27 March 2017 at 00:18, Junio C Hamano <gits...@pobox.com> wrote:
> Luke Diamand <l...@diamand.org> writes:
>
>> As per the discussion about use of "git name-rev" vs "git symbolic-ref" in
>> git-p4 here:
>>
>> http://marc.info/?l=git=148979063421355
>>
>> git-p4 could get confused about the branch it is on and affects
>> the git-p4.allowSubmit  option. This adds a failing
>> test case for the problem.
>>
>> Luke Diamand (1):
>>   git-p4: add failing test for name-rev rather than symbolic-ref
>>
>>  t/t9807-git-p4-submit.sh | 16 
>>  1 file changed, 16 insertions(+)
>
> Ahh, thanks for tying loose ends.  I completely forgot about that
> topic.
>
> If we queue this and then update the function in git-p4.py that
> (mis)uses name-rev to learn the current branch to use symbolic-ref
> instead, can we flip the "expect_failure" to "expect_success"?

Yes. The test passes with your change.

>
> IOW, can we have a follow up to this patch that fixes a known
> breakage the patch documents ;-)?

Yes.

Regards!
Luke

[PATCH 0/3] git-p4: use symbolic-ref instead of name-rev

[Luke Diamand]

Followup to earlier discussion about use of name-rev in git-p4.

http://marc.info/?l=git=148979063421355

Luke Diamand (3):
  git-p4: add failing test for name-rev rather than symbolic-ref
  git-p4: add read_pipe_text() internal function
  git-p4: don't use name-rev to get current branch

 git-p4.py| 38 +-
 t/t9807-git-p4-submit.sh | 16 
 2 files changed, 45 insertions(+), 9 deletions(-)

-- 
2.12.2.719.gcbd162c

[PATCH 3/3] git-p4: don't use name-rev to get current branch

[Luke Diamand]

git-p4 was using "git name-rev" to find out the current branch.

That is not safe, since if multiple branches or tags point at
the same revision, the result obtained might not be what is
expected.

Instead use "git symbolic-ref".

Signed-off-by: Luke Diamand <l...@diamand.org>
---
 git-p4.py| 7 +--
 t/t9807-git-p4-submit.sh | 2 +-
 2 files changed, 2 insertions(+), 7 deletions(-)

diff --git a/git-p4.py b/git-p4.py
index 584b81775..8d151da91 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -602,12 +602,7 @@ def p4Where(depotPath):
 return clientPath
 
 def currentGitBranch():
-retcode = system(["git", "symbolic-ref", "-q", "HEAD"], ignore_error=True)
-if retcode != 0:
-# on a detached head
-return None
-else:
-return read_pipe(["git", "name-rev", "HEAD"]).split(" ")[1].strip()
+return read_pipe_text(["git", "symbolic-ref", "--short", "-q", "HEAD"])
 
 def isValidGitDir(path):
 return git_dir(path) != None
diff --git a/t/t9807-git-p4-submit.sh b/t/t9807-git-p4-submit.sh
index ae05816e0..3457d5db6 100755
--- a/t/t9807-git-p4-submit.sh
+++ b/t/t9807-git-p4-submit.sh
@@ -139,7 +139,7 @@ test_expect_success 'submit with master branch name from 
argv' '
)
 '
 
-test_expect_failure 'allow submit from branch with same revision but different 
name' '
+test_expect_success 'allow submit from branch with same revision but different 
name' '
test_when_finished cleanup_git &&
git p4 clone --dest="$git" //depot &&
(
-- 
2.12.2.719.gcbd162c

[PATCH 1/3] git-p4: add failing test for name-rev rather than symbolic-ref

[Luke Diamand]

Using name-rev to find the current git branch means that git-p4
does not correctly get the current branch name if there are
multiple branches pointing at HEAD, or a tag.

This change adds a test case which demonstrates the problem.
Configuring which branches are allowed to be submitted from goes
wrong, as git-p4 gets confused about which branch is in use.

This appears to be the only place that git-p4 actually cares
about the current branch.

Signed-off-by: Luke Diamand <l...@diamand.org>
Signed-off-by: Junio C Hamano <gits...@pobox.com>
---
 t/t9807-git-p4-submit.sh | 16 
 1 file changed, 16 insertions(+)

diff --git a/t/t9807-git-p4-submit.sh b/t/t9807-git-p4-submit.sh
index e37239e65..ae05816e0 100755
--- a/t/t9807-git-p4-submit.sh
+++ b/t/t9807-git-p4-submit.sh
@@ -139,6 +139,22 @@ test_expect_success 'submit with master branch name from 
argv' '
)
 '
 
+test_expect_failure 'allow submit from branch with same revision but different 
name' '
+   test_when_finished cleanup_git &&
+   git p4 clone --dest="$git" //depot &&
+   (
+   cd "$git" &&
+   test_commit "file8" &&
+   git checkout -b branch1 &&
+   git checkout -b branch2 &&
+   git config git-p4.skipSubmitEdit true &&
+   git config git-p4.allowSubmit "branch1" &&
+   test_must_fail git p4 submit &&
+   git checkout branch1 &&
+   git p4 submit
+   )
+'
+
 #
 # Basic submit tests, the five handled cases
 #
-- 
2.12.2.719.gcbd162c

[PATCH 2/3] git-p4: add read_pipe_text() internal function

[Luke Diamand]

The existing read_pipe() function returns an empty string on
error, but also returns an empty string if the command returns
an empty string.

This leads to ugly constructions trying to detect error cases.

Add read_pipe_text() which just returns None on error.

Signed-off-by: Luke Diamand <l...@diamand.org>
---
 git-p4.py | 31 ---
 1 file changed, 28 insertions(+), 3 deletions(-)

diff --git a/git-p4.py b/git-p4.py
index eab319d76..584b81775 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -160,17 +160,42 @@ def p4_write_pipe(c, stdin):
 real_cmd = p4_build_cmd(c)
 return write_pipe(real_cmd, stdin)
 
-def read_pipe(c, ignore_error=False):
+def read_pipe_full(c):
+""" Read output from  command. Returns a tuple
+of the return status, stdout text and stderr
+text.
+"""
 if verbose:
 sys.stderr.write('Reading pipe: %s\n' % str(c))
 
 expand = isinstance(c,basestring)
 p = subprocess.Popen(c, stdout=subprocess.PIPE, stderr=subprocess.PIPE, 
shell=expand)
 (out, err) = p.communicate()
-if p.returncode != 0 and not ignore_error:
-die('Command failed: %s\nError: %s' % (str(c), err))
+return (p.returncode, out, err)
+
+def read_pipe(c, ignore_error=False):
+""" Read output from  command. Returns the output text on
+success. On failure, terminates execution, unless
+ignore_error is True, when it returns an empty string.
+"""
+(retcode, out, err) = read_pipe_full(c)
+if retcode != 0:
+if ignore_error:
+out = ""
+else:
+die('Command failed: %s\nError: %s' % (str(c), err))
 return out
 
+def read_pipe_text(c):
+""" Read output from a command with trailing whitespace stripped.
+On error, returns None.
+"""
+(retcode, out, err) = read_pipe_full(c)
+if retcode != 0:
+return None
+else:
+return out.rstrip()
+
 def p4_read_pipe(c, ignore_error=False):
 real_cmd = p4_build_cmd(c)
 return read_pipe(real_cmd, ignore_error)
-- 
2.12.2.719.gcbd162c

Re: [PATCH] git-p4: improve branch option handling

[Luke Diamand]

On 20 April 2017 at 14:52, Andrew Oakley  wrote:
> It is sometimes useful (much quicker) to request commands only operate
> on a single branch.
>
> The P4Sync command has been updated to handle self.branch being None for
> consitency with the P4Submit.

Should that be consistency?


>
> The P4Rebase command has been given a branch option which is forwarded
> to the P4Sync command it runs.
>
> The P4Submit command has been simplified to not call P4Sync itself, it
> lets P4Rebase do it instead (now that the branch can be handled).  This
> fixes an issue where P4Submit does a sync of the requested branch and
> then does a rebase which does a sync of all branches.
>
> Signed-off-by: Andrew Oakley 
> ---
>  Documentation/git-p4.txt |  4 
>  git-p4.py| 15 +++
>  2 files changed, 11 insertions(+), 8 deletions(-)
>
> diff --git a/Documentation/git-p4.txt b/Documentation/git-p4.txt
> index 7436c64..a03a291 100644
> --- a/Documentation/git-p4.txt
> +++ b/Documentation/git-p4.txt
> @@ -328,6 +328,10 @@ Rebase options
>  ~~
>  These options can be used to modify 'git p4 rebase' behavior.
>
> +--branch ::
> +   Sync this named branch instead of the default p4/master.  See the
> +   "Sync options" section above for more information.
> +
>  --import-labels::
> Import p4 labels.
>
> diff --git a/git-p4.py b/git-p4.py
> index 8d151da..e58b34a 100755
> --- a/git-p4.py
> +++ b/git-p4.py
> @@ -2161,13 +2161,9 @@ class P4Submit(Command, P4UserMap):
>  elif len(commits) == len(applied):
>  print ("All commits {0}!".format(shelved_applied))
>
> -sync = P4Sync()
> -if self.branch:
> -sync.branch = self.branch
> -sync.run([])
> -
>  rebase = P4Rebase()
> -rebase.rebase()
> +rebase.branch = self.branch
> +rebase.run([])
>
>  else:
>  if len(applied) == 0:
> @@ -2343,7 +2339,7 @@ class P4Sync(Command, P4UserMap):
>  self.silent = False
>  self.createdBranches = set()
>  self.committedChanges = set()
> -self.branch = ""
> +self.branch = None
>  self.detectBranches = False
>  self.detectLabels = False
>  self.importLabels = False
> @@ -3281,7 +3277,7 @@ class P4Sync(Command, P4UserMap):
>  system("git fetch origin")
>
>  branch_arg_given = bool(self.branch)
> -if len(self.branch) == 0:
> +if not branch_arg_given:
>  self.branch = self.refPrefix + "master"
>  if gitBranchExists("refs/heads/p4") and self.importIntoRemotes:
>  system("git update-ref %s refs/heads/p4" % self.branch)
> @@ -3567,14 +3563,17 @@ class P4Rebase(Command):
>  def __init__(self):
>  Command.__init__(self)
>  self.options = [
> +optparse.make_option("--branch", dest="branch"),
>  optparse.make_option("--import-labels", dest="importLabels", 
> action="store_true"),
>  ]
> +self.branch = None
>  self.importLabels = False
>  self.description = ("Fetches the latest revision from perforce and "
>  + "rebases the current work (branch) against it")
>
>  def run(self, args):
>  sync = P4Sync()
> +sync.branch = self.branch
>  sync.importLabels = self.importLabels
>  sync.run([])
>
> --
> 2.10.2
>

Apart from the typo above, this looks sensible to me. Would you be
able to add a test case?

Thanks!
Luke

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

[Luke Diamand]

On 11 July 2017 at 23:53, Miguel Torroja  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 
> ---
>  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

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

[Luke Diamand]

On 3 July 2017 at 23:57, Miguel Torroja  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 
> ---
>  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 

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

[Luke Diamand]

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

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

[Luke Diamand]

On 28 June 2017 at 05:08, Junio C Hamano  wrote:
> Miguel Torroja  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 
>> ---
>
> 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

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

[Luke Diamand]

On 28 June 2017 at 14:14, miguel torroja  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.

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


Luke

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

[Luke Diamand]

On 22 June 2017 at 18:32, Junio C Hamano  wrote:
> Miguel Torroja  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

Re: Git p4 sync changelist interval

[Luke Diamand]

On 6 June 2017 at 00:29, Luke Diamand <l...@diamand.org> wrote:
> On 5 June 2017 at 19:50, Андрей Ефанов <1134t...@gmail.com> wrote:
>> 2017-06-04 14:09 GMT+03:00 Luke Diamand <l...@diamand.org>:
>>>
>>>
>>> >
>>> > The problem, as I see it, is that before syncing changes in the given
>>> > range, p4 task does not sync to cl1 version of the repo, and applies
>>> > commits to the empty repository.
>>> > Is it a bug or my misunderstanding of how git p4 should work?
>>>
>>> Possibly I'm misunderstanding what you're doing! Can you give a
>>> sequence of steps to show the problem?
>>
>> What I meant is:
>>
>> 1. Create p4 depot
>> 2. Add first.file (CL 2)
>> 3. Add second.file (at CL 3)
>> 4. Add third.file (at CL 4)
>> 5. Modify first.file (at CL 5)
>> 4. git-p4 clone //depot@3,5
>>
>> In this case first.file, will not be represented in the repository.
>
> Hmmm, it's not working right for me. Although in my case I seem to be
> missing the second file.
>
> It's fine if I don't use the revision range "3,5".

It's also fine if I do:

git p4 sync //depot@3
cd depot
git p4 rebase

Re: Git p4 sync changelist interval

[Luke Diamand]

On 6 June 2017 at 08:00, Luke Diamand <l...@diamand.org> wrote:
> On 6 June 2017 at 00:29, Luke Diamand <l...@diamand.org> wrote:
>> On 5 June 2017 at 19:50, Андрей Ефанов <1134t...@gmail.com> wrote:
>>> 2017-06-04 14:09 GMT+03:00 Luke Diamand <l...@diamand.org>:
>>>>
>>>>
>>>> >
>>>> > The problem, as I see it, is that before syncing changes in the given
>>>> > range, p4 task does not sync to cl1 version of the repo, and applies
>>>> > commits to the empty repository.
>>>> > Is it a bug or my misunderstanding of how git p4 should work?
>>>>
>>>> Possibly I'm misunderstanding what you're doing! Can you give a
>>>> sequence of steps to show the problem?
>>>
>>> What I meant is:
>>>
>>> 1. Create p4 depot
>>> 2. Add first.file (CL 2)
>>> 3. Add second.file (at CL 3)
>>> 4. Add third.file (at CL 4)
>>> 5. Modify first.file (at CL 5)
>>> 4. git-p4 clone //depot@3,5
>>>
>>> In this case first.file, will not be represented in the repository.
>>
>> Hmmm, it's not working right for me. Although in my case I seem to be
>> missing the second file.
>>
>> It's fine if I don't use the revision range "3,5".
>
> It's also fine if I do:
>
> git p4 sync //depot@3
> cd depot
> git p4 rebase

It seems to be something to do with the code around line 3395 that says:

if self.changeRange == "@all":
self.changeRange = ""
elif ',' not in self.changeRange:

It's finding a lower revision number with which to later call
importHeadRevision(), but that only seems to be called if the revision
range does *not* have a "," in it. As a result, we never actually call
importHeadRevision() and so files end up missing.

The obvious fix of fishing out the "@3" from the "@3,5" revision range
works in this instance, but breaks some of the regression tests.

Luke

Re: Git p4 sync changelist interval

[Luke Diamand]

On 6 June 2017 at 08:56, Андрей Ефанов <1134t...@gmail.com> wrote:
>>
>> It seems to be something to do with the code around line 3395 that says:
>>
>> if self.changeRange == "@all":
>> self.changeRange = ""
>> elif ',' not in self.changeRange:
>>
>> It's finding a lower revision number with which to later call
>> importHeadRevision(), but that only seems to be called if the revision
>> range does *not* have a "," in it. As a result, we never actually call
>> importHeadRevision() and so files end up missing.
>>
>> The obvious fix of fishing out the "@3" from the "@3,5" revision range
>> works in this instance, but breaks some of the regression tests.
>>
>> Luke
>
> I did the same change before and it worked for me. I'd like to find
> out if it is a bug (I think it is), a normal behavior or am I doing
> something wrong.
>
> So according to what you say, it is a bug.

It's a bug!

I think you can workaround it by doing:

$ git p4 clone //depot@3
$ git p4 sync

I'll see if I can workout why my obvious fix caused the tests to fail.

Re: Git p4 sync changelist interval

[Luke Diamand]

On 4 June 2017 at 10:56, Андрей Ефанов <1134t...@gmail.com> wrote:
> Hello,
>
> My goal is to sync the repository from p4 using an interval of
> changelists so that the first changelist version of the repository
> would be considered as an initial commit.
> So I used the following command:
>
>  git p4 clone //depot@cl1,cl2
>
> And when it finished, the files, that were created before the cl1 were
> not in the HEAD.

Do you mean that if foo.c was created at cl1+1, that after doing the
clone, it wasn't there?

If so, that doesn't sound right to me.

I have just tried doing what I think you mean:

1. Create p4 depot
2. Add foo.c (at CL 2)
3. Add bar.c (at CL 3)
4. git-p4 clone //depot@2,3

I end up with both files.

>
> The problem, as I see it, is that before syncing changes in the given
> range, p4 task does not sync to cl1 version of the repo, and applies
> commits to the empty repository.
> Is it a bug or my misunderstanding of how git p4 should work?

Possibly I'm misunderstanding what you're doing! Can you give a
sequence of steps to show the problem?

Thanks!
Luke

Re: Git p4 sync changelist interval

[Luke Diamand]

On 5 June 2017 at 19:50, Андрей Ефанов <1134t...@gmail.com> wrote:
> 2017-06-04 14:09 GMT+03:00 Luke Diamand <l...@diamand.org>:
>>
>> On 4 June 2017 at 10:56, Андрей Ефанов <1134t...@gmail.com> wrote:
>> > Hello,
>> >
>> > My goal is to sync the repository from p4 using an interval of
>> > changelists so that the first changelist version of the repository
>> > would be considered as an initial commit.
>> > So I used the following command:
>> >
>> >  git p4 clone //depot@cl1,cl2
>> >
>> > And when it finished, the files, that were created before the cl1 were
>> > not in the HEAD.
>>
>> Do you mean that if foo.c was created at cl1+1, that after doing the
>> clone, it wasn't there?
>>
>> If so, that doesn't sound right to me.
>>
>> I have just tried doing what I think you mean:
>>
>> 1. Create p4 depot
>> 2. Add foo.c (at CL 2)
>> 3. Add bar.c (at CL 3)
>> 4. git-p4 clone //depot@2,3
>>
>> I end up with both files.
>>
>> >
>> > The problem, as I see it, is that before syncing changes in the given
>> > range, p4 task does not sync to cl1 version of the repo, and applies
>> > commits to the empty repository.
>> > Is it a bug or my misunderstanding of how git p4 should work?
>>
>> Possibly I'm misunderstanding what you're doing! Can you give a
>> sequence of steps to show the problem?
>
> What I meant is:
>
> 1. Create p4 depot
> 2. Add first.file (CL 2)
> 3. Add second.file (at CL 3)
> 4. Add third.file (at CL 4)
> 5. Modify first.file (at CL 5)
> 4. git-p4 clone //depot@3,5
>
> In this case first.file, will not be represented in the repository.

Hmmm, it's not working right for me. Although in my case I seem to be
missing the second file.

It's fine if I don't use the revision range "3,5".

Luke

Re: Bug in "revision.c: --all adds HEAD from all worktrees" ?

[Luke Diamand]

+Jeff King

On 13 November 2017 at 22:15, Stefan Beller <sbel...@google.com> wrote:
> On Mon, Nov 13, 2017 at 2:03 PM, Luke Diamand <l...@diamand.org> wrote:
>> On 13 November 2017 at 19:51, Luke Diamand <l...@diamand.org> wrote:
>>> Hi!
>>>
>>> I think there may be a regression caused by this change which means
>>> that "git fetch origin" doesn't work:
>>>
>>> commit d0c39a49ccb5dfe7feba4325c3374d99ab123c59 (refs/bisect/bad)
>>> Author: Nguyễn Thái Ngọc Duy <pclo...@gmail.com>
>>> Date:   Wed Aug 23 19:36:59 2017 +0700
>>>
>>> revision.c: --all adds HEAD from all worktrees
>>>
>>> $ git fetch origin
>>> fatal: bad object HEAD
>>> error: ssh://my_remote_host/reponame did not send all necessary objects
>>>
>>> I used git bisect to find the problem, and it seems pretty consistent.
>>> "git fetch" with the previous revision works fine.
>>>
>>> FWIW, I've got a lot of git worktrees associated with this repo, so
>>> that may be why it's failing. The remote repo is actually a git-p4
>>> clone, so HEAD there actually ends up pointing at
>>> refs/remote/p4/master.
>>>
>>> Thanks,
>>> Luke
>>
>> Quite a few of the worktrees have expired - their head revision has
>> been GC'd and no longer points to anything sensible
>> (gc.worktreePruneExpire). The function other_head_refs() in worktree.c
>> bails out if there's an error, which I think is the problem. I wonder
>> if it should instead just report something and then keep going.
>
> Also see
> https://public-inbox.org/git/cagz79kyp0z1g_h3nwfmshrawhmbocik5lepuxkj0nveebri...@mail.gmail.com/

So is this a bug or user error on my part?

Surely at the very least "git fetch" shouldn't give a cryptic error
message just because one of my git worktrees has expired!

Re: Bug in "revision.c: --all adds HEAD from all worktrees" ?

[Luke Diamand]

On 15 November 2017 at 22:08, Junio C Hamano <gits...@pobox.com> wrote:
> Luke Diamand <l...@diamand.org> writes:
>
>> Quite a few of the worktrees have expired - their head revision has
>> been GC'd and no longer points to anything sensible
>> (gc.worktreePruneExpire). The function other_head_refs() in worktree.c
>> bails out if there's an error, which I think is the problem. I wonder
>> if it should instead just report something and then keep going.
>
> Am I correct to understand that your "git fsck" would fail because
> these HEAD refs used by other stale worktrees are pointing at
> missing objects?

git fsck says:

Checking object directories: 100% (256/256), done.
Checking objects: 100% (1434634/1434634), done.
error: HEAD: invalid reflog entry
7fa2b7ee4bc0d11529f659db8b13ed1f537d2a98
error: HEAD: invalid reflog entry
7fa2b7ee4bc0d11529f659db8b13ed1f537d2a98
error: HEAD: invalid reflog entry
7e79e09e8a7382f91610f7255a1b99ea59f68c0b
error: refs/stash: invalid reflog entry
feeb35e7b045d28943c706e761d0a2ac8206af2f
error: refs/remotes/origin/master: invalid reflog entry
7fa2b7ee4bc0d11529f659db8b13ed1f537d2a98
Checking connectivity: 1419477, done.
missing tree 1480c0a7ed2ad59ae701667292399c38d294658e
missing tree ca2a01116bfbbd1fcbcf9812b95d8dc6c39e69d5
missing tree 5b7c41e547fc5c4c840e5b496da13d3daebc5fbe
...
...

>
> What do you mean by "expired"?  "Even though I want to keep using
> them, Git for some reason decided to destroy them." or "I no longer
> use them but kept them lying around."?

git worktree automatically prunes work trees:

"The working tree’s administrative files in the repository (see
"DETAILS" below) will eventually be removed automatically (see
gc.worktreePruneExpire in git-config(1)),"

In my case I didn't actually want them removed, but fortunately
there's nothing important in them (there certainly isn't anymore...).

>
> If the latter, I wonder "worktree prune" to remove the
> admininstrative information for them would unblock you?

It doesn't seem to help.

$ git worktree prune -n

$ git worktree prune
$ git fetch
remote: Counting objects: 35, done.
remote: Compressing objects: 100% (20/20), done.
remote: Total 21 (delta 17), reused 5 (delta 1)
Unpacking objects: 100% (21/21), done.
fatal: bad object HEAD
error: ssh://whatever/myrepol did not send all necessary objects
$ /usr/bin/git-2.7.3 fetch

Bug in "revision.c: --all adds HEAD from all worktrees" ?

[Luke Diamand]

Hi!

I think there may be a regression caused by this change which means
that "git fetch origin" doesn't work:

commit d0c39a49ccb5dfe7feba4325c3374d99ab123c59 (refs/bisect/bad)
Author: Nguyễn Thái Ngọc Duy 
Date:   Wed Aug 23 19:36:59 2017 +0700

revision.c: --all adds HEAD from all worktrees

$ git fetch origin
fatal: bad object HEAD
error: ssh://my_remote_host/reponame did not send all necessary objects

I used git bisect to find the problem, and it seems pretty consistent.
"git fetch" with the previous revision works fine.

FWIW, I've got a lot of git worktrees associated with this repo, so
that may be why it's failing. The remote repo is actually a git-p4
clone, so HEAD there actually ends up pointing at
refs/remote/p4/master.

Thanks,
Luke

Re: Bug in "revision.c: --all adds HEAD from all worktrees" ?

[Luke Diamand]

On 13 November 2017 at 19:51, Luke Diamand <l...@diamand.org> wrote:
> Hi!
>
> I think there may be a regression caused by this change which means
> that "git fetch origin" doesn't work:
>
> commit d0c39a49ccb5dfe7feba4325c3374d99ab123c59 (refs/bisect/bad)
> Author: Nguyễn Thái Ngọc Duy <pclo...@gmail.com>
> Date:   Wed Aug 23 19:36:59 2017 +0700
>
> revision.c: --all adds HEAD from all worktrees
>
> $ git fetch origin
> fatal: bad object HEAD
> error: ssh://my_remote_host/reponame did not send all necessary objects
>
> I used git bisect to find the problem, and it seems pretty consistent.
> "git fetch" with the previous revision works fine.
>
> FWIW, I've got a lot of git worktrees associated with this repo, so
> that may be why it's failing. The remote repo is actually a git-p4
> clone, so HEAD there actually ends up pointing at
> refs/remote/p4/master.
>
> Thanks,
> Luke

Quite a few of the worktrees have expired - their head revision has
been GC'd and no longer points to anything sensible
(gc.worktreePruneExpire). The function other_head_refs() in worktree.c
bails out if there's an error, which I think is the problem. I wonder
if it should instead just report something and then keep going.

Re: git-p4: cloning with a change number does not import all files

[Luke Diamand]

On 2 December 2017 at 15:35, Patrick Rouleau  wrote:
> On Fri, Dec 1, 2017 at 7:45 AM, Lars Schneider  
> wrote:
>> Oh, I am with you. However, I only used git-p4 for a very short time in the
>> way you want to use it. Therefore, I don't have much experience in that kind
>> of usage pattern. I was able to convice my management to move all source to
>> Git and I used git-p4 to migrate the repositories ;-)
>>
>> Here is a talk on the topic if you want to learn more:
>> https://www.youtube.com/watch?v=QNixDNtwYJ0
>>
>> Cheers,
>> Lars
>
> Sadly, there is no way I convince the company to switch to git. They
> have acquired
> many companies in the past years and they have standardized around Perforce.
> It is in part because they want access control and they need to store
> a lot of binary
> files (including big VM images).
>
> I keep this video close to explain why I like git:
> https://www.youtube.com/watch?v=o4PFDKIc2fs

I feel your pain.

I think I've sort of stumbled across something like the problem you've
described in the past. Perhaps the files you need have been deleted
and then re-integrated or some such.

Would you be able to take a look at some files with this problem and
see if you can spot what's happened to it ("p4 changes" and perhaps
"p4 changes -i").

One thing that can certainly happen is that Perforce gets *very*
confused if you start getting too clever with symlinked directories,
and git-p4 can only do so much in the face of this. But it may be
something else.

Thanks
Luke

[PATCH 1/1] git-p4: update multiple shelved change lists

[Luke Diamand]

--update-shelve can now be specified multiple times on the
command-line, to update multiple shelved changelists in a single
submit.

This then means that a git patch series can be mirrored to a
sequence of shelved changelists, and (relatively easily) kept in
sync as changes are made in git.

Note that Perforce does not really support overlapping shelved
changelists where one change touches the files modified by
another. Trying to do this will result in merge conflicts.

Signed-off-by: Luke Diamand <l...@diamand.org>
---
 Documentation/git-p4.txt |  8 +++-
 git-p4.py| 41 ++---
 t/t9807-git-p4-submit.sh | 24 ++--
 3 files changed, 47 insertions(+), 26 deletions(-)

diff --git a/Documentation/git-p4.txt b/Documentation/git-p4.txt
index 7436c64a9..d8c8f11c9 100644
--- a/Documentation/git-p4.txt
+++ b/Documentation/git-p4.txt
@@ -157,6 +157,12 @@ The p4 changes will be created as the user invoking 'git 
p4 submit'. The
 according to the author of the Git commit.  This option requires admin
 privileges in p4, which can be granted using 'p4 protect'.
 
+To shelve changes instead of submitting, use `--shelve` and `--update-shelve`:
+
+
+$ git p4 submit --shelve
+$ git p4 submit --update-shelve 1234 --update-shelve 2345
+
 
 OPTIONS
 ---
@@ -310,7 +316,7 @@ These options can be used to modify 'git p4 submit' 
behavior.
 
 --update-shelve CHANGELIST::
Update an existing shelved changelist with this commit. Implies
-   --shelve.
+   --shelve. Repeat for multiple shelved changelists.
 
 --conflict=(ask|skip|quit)::
Conflicts can occur when applying a commit to p4.  When this
diff --git a/git-p4.py b/git-p4.py
index 76859b453..7bb9cadc6 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -1178,6 +1178,12 @@ class Command:
 self.needsGit = True
 self.verbose = False
 
+# This is required for the "append" cloneExclude action
+def ensure_value(self, attr, value):
+if not hasattr(self, attr) or getattr(self, attr) is None:
+setattr(self, attr, value)
+return getattr(self, attr)
+
 class P4UserMap:
 def __init__(self):
 self.userMapFromPerforceServer = False
@@ -1343,9 +1349,10 @@ class P4Submit(Command, P4UserMap):
 optparse.make_option("--shelve", dest="shelve", 
action="store_true",
  help="Shelve instead of submit. Shelved 
files are reverted, "
  "restoring the workspace to the state 
before the shelve"),
-optparse.make_option("--update-shelve", dest="update_shelve", 
action="store", type="int",
+optparse.make_option("--update-shelve", dest="update_shelve", 
action="append", type="int",
  metavar="CHANGELIST",
- help="update an existing shelved 
changelist, implies --shelve")
+ help="update an existing shelved 
changelist, implies --shelve, "
+   "repeat in-order for multiple 
shelved changelists")
 ]
 self.description = "Submit changes from git to the perforce depot."
 self.usage += " [name of git branch to submit into perforce depot]"
@@ -1354,7 +1361,7 @@ class P4Submit(Command, P4UserMap):
 self.preserveUser = gitConfigBool("git-p4.preserveUser")
 self.dry_run = False
 self.shelve = False
-self.update_shelve = None
+self.update_shelve = list()
 self.prepare_p4_only = False
 self.conflict_behavior = None
 self.isWindows = (platform.system() == "Windows")
@@ -1809,9 +1816,10 @@ class P4Submit(Command, P4UserMap):
 mode = filesToChangeExecBit[f]
 setP4ExecBit(f, mode)
 
-if self.update_shelve:
-print("all_files = %s" % str(all_files))
-p4_reopen_in_change(self.update_shelve, all_files)
+update_shelve = 0
+if len(self.update_shelve) > 0:
+update_shelve = self.update_shelve.pop(0)
+p4_reopen_in_change(update_shelve, all_files)
 
 #
 # Build p4 change description, starting with the contents
@@ -1821,7 +1829,7 @@ class P4Submit(Command, P4UserMap):
 logMessage = logMessage.strip()
 (logMessage, jobs) = self.separate_jobs_from_description(logMessage)
 
-template = self.prepareSubmitTemplate(self.update_shelve)
+template = self.prepareSubmitTemplate(update_shelve)
 submitTemplate = self.prepareLogMessage(template, logMessage, jobs)
 
 if self.preserveUser:
@@ -1894,7 +1902,7 @@ class P4Submit(Command, P4UserMap):
  

[PATCH 0/1] git-p4: update multiple shelved change lists

[Luke Diamand]

This change lets you update several P4 changelists in sequence. Say
you have several git commits which are all somehow related. You would
start by shelving them (e.g. for a review), something like this:

 git p4 submit --origin HEAD^2 --shelve

You then make changes to these commits (in git) and now need to re-shelve
them. Before this change you would need to cherry-pick each change onto
a clean branch and do "git p4 --update-shelve", then remove the tip and
repeat.

With this change, you can just do:

 git p4 submit --origin HEAD^2 --update-shelve $CL1 --update-shelve $CL2

If the shelved changelists overlap (one changelist touches the same line
as another) then this won't work, but that problem already exists with
the --shelve option. Solving that is pretty hard to do as P4 really
only understands files, not changes. Despite this shortcoming, it's
very useful to be able to do update shelved changelists like this.

Luke Diamand (1):
  git-p4: update multiple shelved change lists

 Documentation/git-p4.txt |  8 +++-
 git-p4.py| 41 ++---
 t/t9807-git-p4-submit.sh | 24 ++--
 3 files changed, 47 insertions(+), 26 deletions(-)

-- 
2.15.0.276.g89ea799.dirty

Re: [PATCH] git-p4: add options --commit and --disable-rebase

[Luke Diamand]

On 8 May 2018 at 01:37, Merland Romain <merlo...@yahoo.fr> wrote:
> From f5229be8e2a3340af853227929818940323a8062 Mon Sep 17 00:00:00 2001
> From: Romain Merland <merlo...@yahoo.fr>
> Date: Tue, 8 May 2018 02:03:11 +0200
> Subject: [PATCH] git-p4: add options --commit and --disable-rebase
> To: git@vger.kernel.org
> Cc: Luke Diamand <l...@diamand.org>,
> Junio C Hamano <gits...@pobox.com>,
> Matthieu Moy <matthieu@imag.fr>,
> Vinicius Kursancew <viniciusalexan...@gmail.com>,
> Jeff King <p...@peff.net>,
> Cedric Borgese <cedric.borg...@pdgm.com>,
> Fabien Boutantin <fabien.boutan...@pdgm.com>
> Thanks-to: Cedric Borgese
> Signed-off-by: Romain Merland
>
> On a daily work with multiple local git branches, the usual way to submit
> only a
> specified commit was to cherry-pick the commit on master then run git-p4
> submit.
> It can be very annoying to switch between local branches and master, only to
> submit one commit.
> The proposed new way is to select directly the commit you want to submit.
>
> add option --commit to command 'git-p4 submit' in order to submit only
> specified commit(s) in p4.
>
> On a daily work developping software with big compilation time, one may not
> want
> to rebase on his local git tree, in order to avoid long recompilation.
>
> add option --disable-rebase to command 'git-p4 submit' in order to disable
> rebase after submission.

Looks good to me.

I just tried it (shelving a change about 3 commits down) and it works
like a charm!

The "--disable-rebase" will also be very useful (I wonder whether a
git-config option might be a useful addition at a later date).

(You might need to resubmit your change as plain text via "git
format-patch" and "git send-email" as I'm not sure if Junio will
accept a mime-encoded patch).

Thanks
Luke

> ---
>  Documentation/git-p4.txt | 14 ++
>  git-p4.py| 29 +++--
>  t/t9807-git-p4-submit.sh | 40 
>  3 files changed, 77 insertions(+), 6 deletions(-)
>
> diff --git a/Documentation/git-p4.txt b/Documentation/git-p4.txt
> index d8c8f11c9..88d109deb 100644
> --- a/Documentation/git-p4.txt
> +++ b/Documentation/git-p4.txt
> @@ -149,6 +149,12 @@ To specify a branch other than the current one, use:
>  $ git p4 submit topicbranch
>  
>
> +To specify a single commit or a range of commits, use:
> +
> +$ git p4 submit --commit 
> +$ git p4 submit --commit 
> +
> +
>  The upstream reference is generally 'refs/remotes/p4/master', but can
>  be overridden using the `--origin=` command-line option.
>
> @@ -330,6 +336,14 @@ These options can be used to modify 'git p4 submit'
> behavior.
>   p4/master.  See the "Sync options" section above for more
>   information.
>
> +--commit |::
> +Submit only the specified commit or range of commits, instead of the
> full
> +list of changes that are in the current Git branch.
> +
> +--disable-rebase::
> +Disable the automatic rebase after all commits have been successfully
> +submitted.
> +
>  Rebase options
>  ~~
>  These options can be used to modify 'git p4 rebase' behavior.
> diff --git a/git-p4.py b/git-p4.py
> index 7bb9cadc6..f4a6f3b4c 100755
> --- a/git-p4.py
> +++ b/git-p4.py
> @@ -1352,7 +1352,12 @@ class P4Submit(Command, P4UserMap):
>  optparse.make_option("--update-shelve",
> dest="update_shelve", action="append", type="int",
>   metavar="CHANGELIST",
>   help="update an existing shelved
> changelist, implies --shelve, "
> -   "repeat in-order for multiple
> shelved changelists")
> +   "repeat in-order for multiple
> shelved changelists"),
> +optparse.make_option("--commit", dest="commit",
> metavar="COMMIT",
> + help="submit only the specified
> commit(s), one commit or xxx..xxx"),
> +optparse.make_option("--disable-rebase",
> dest="disable_rebase", action="store_true",
> + help="Disable rebase after submit is
> completed. Can be useful if you "
> + "work from a local git branch that is
> not master")
>  ]
>  self.description = "Submit chan

Re: [PATCH v4 3/6] git-p4: change "commitish" typo to "committish"

[Luke Diamand]

On 10 May 2018 at 13:43, Ævar Arnfjörð Bjarmason  wrote:
> This was the only occurrence of "commitish" in the tree, but as the
> log will reveal we've had others in the past. Fixes up code added in
> 00ad6e3182 ("git-p4: work with a detached head", 2015-11-21).
>
> Signed-off-by: Ævar Arnfjörð Bjarmason 

Looks good to me!

Thanks,
Luke


> ---
>  git-p4.py | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/git-p4.py b/git-p4.py
> index 7bb9cadc69..1afa87cd9d 100755
> --- a/git-p4.py
> +++ b/git-p4.py
> @@ -2099,11 +2099,11 @@ class P4Submit(Command, P4UserMap):
>
>  commits = []
>  if self.master:
> -commitish = self.master
> +committish = self.master
>  else:
> -commitish = 'HEAD'
> +committish = 'HEAD'
>
> -for line in read_pipe_lines(["git", "rev-list", "--no-merges", 
> "%s..%s" % (self.origin, commitish)]):
> +for line in read_pipe_lines(["git", "rev-list", "--no-merges", 
> "%s..%s" % (self.origin, committish)]):
>  commits.append(line.strip())
>  commits.reverse()
>
> --
> 2.17.0.410.g4ac3413cc8
>

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

[Luke Diamand]

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

For example:

  $ git p4 unshelve 12345

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

If that branch already exists, it is renamed - for example
the above branch would be saved as p4/unshelved/12345.1.

Caveat:

The unshelving is done against the current "p4/master" branch;
git-p4 uses "p4 print" to get the file contents at the requested
revision, and then fast-import creates a commit relative to p4/master.

Ideally what you would want is for fast-import to create the
commit based on the Perforce "revision" prior to the shelved commit,
but Perforce doesn't have such a concept - to do this, git-p4
would need to figure out the revisions of the individual files
before the shelved changelist, and then construct a temporary
git branch which matched this.

It's possible to do this, but doing so makes this change a lot more
complicated.

This limitation means that if you unshelve a change where some
of the changed files were not based on p4/master, you will get
an amalgam of the change you wanted, and these other changes.

The reference branch can be changed manually with the "--origin"
option.

The change adds a new Unshelve command class. This just runs the
existing P4Sync code tweaked to handle a shelved changelist.

Signed-off-by: Luke Diamand <l...@diamand.org>
---
 Documentation/git-p4.txt |  26 ++
 git-p4.py| 171 ++-
 t/t9832-unshelve.sh  |  99 +++
 3 files changed, 260 insertions(+), 36 deletions(-)
 create mode 100755 t/t9832-unshelve.sh

diff --git a/Documentation/git-p4.txt b/Documentation/git-p4.txt
index d8c8f11c9f..2d768eec10 100644
--- a/Documentation/git-p4.txt
+++ b/Documentation/git-p4.txt
@@ -164,6 +164,25 @@ $ 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, so if this
+is behind Perforce itself, it may include more changes than you expected. You 
can
+change the reference branch with the "--origin" option.
+
+If the target branch in refs/remotes/p4/unshelved already exists, the old one 
will
+be renamed.
+
+
+$ git p4 sync
+$ git p4 unshelve 12345
+$ git show refs/remotes/p4/unshelved/12345
+
+
 OPTIONS
 ---
 
@@ -337,6 +356,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..dcf6dc9f4f 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)))
 
@@ -662,6 +667,12 @@ def gitBranchExists(branch):
 stderr=subprocess.PIPE, stdout=subprocess.PIPE);
 return proc.wait() == 0;
 
+def gitUpdateRef(ref, newvalue):
+subprocess.check_call(["git", "update-ref", ref, newvalue])
+
+def gitDeleteRef(ref):
+subprocess.check_call(["git", "update-ref", "-d", ref])
+
 _gitConfig = {}
 
 def gitConfig(key, typeSpecifier=None):
@@ -2411,6 +2422,7 @@ class P4Sync(Command, P4UserMap):
 self.tempBranches = []
 self.tempBranchLocation = "refs/git-p4-tmp"
 self.largeFileSystem = None
+self.suppress_meta_comment = False
 
 if gitConfig('git-p4.largeFileSystem'):
 largeFileSystemConstructor = 
globals()[gitConfig('git-p4.largeFileSystem')]
@@ -2421,6 +2433,18 @@ class P4Sync(Command, P4UserMap):
 if gitConfig("git-p4.syncFromOrigin") == "false":
 self.syncWithOrigin = False
 
+self.depotPaths = []
+self.changeRange = ""
+self.previous

[PATCHv2 0/1] add git-p4 unshelve command

[Luke Diamand]

This is another attempt to make a "git p4 unshelve" command.

Unshelving in p4 is a bit like a cross between cherry-pick
and "am", and is very commonly used for review.

This command helps git users who want to try out a shelved
p4 change from some other repo:

e.g.

   $ git p4 unshelve 12345
   unshelved CL12345 into refs/remotes/p4/unshelved/12345
   $ git show refs/remotes/p4/unshelved/12345

I abandoned an earlier attempt because it seemed like there
is no way to get around a rather nasty problem: git-p4
just constructs the commit and passes the file contents to
git-fastimport. But there's no easy way to construct the
*prior* commit, because Perforce doesn't record this
information, and so you can end up with other changes
mixed into the unshelved commit - these are the differences
between your tree and the other tree, for each file that
has been modified.

However, I think the command is sufficiently useful that
it's worth supporting anyway, even with that caveat.

I also tried to use "p4 describe" to get the deltas, but
that's very unsatisfactory: I found myself writing a
second-rate version of git's diff tool to try to make
up for the deficiencies in Perforce's diff tool.

It might be possible to reconstruct the missing base
commit information, but that's a reasonably tricky task.

I have incorporated some of the comments from the earlier
review rounds, in particular:

- no longer adds the [git-p4...] annotation in unshelve
- try to use .format() in place of %
- rename the target branch if it already exists

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

 Documentation/git-p4.txt |  26 ++
 git-p4.py| 171 ++-
 t/t9832-unshelve.sh  |  99 +++
 3 files changed, 260 insertions(+), 36 deletions(-)
 create mode 100755 t/t9832-unshelve.sh

-- 
2.17.0.392.gdeb1a6e9b7

Re: [PATCH] git-p4: add options --commit and --disable-rebase

[Luke Diamand]

On 9 May 2018 at 16:32, Romain Merland  wrote:
> On a daily work with multiple local git branches, the usual way to submit 
> only a
> specified commit was to cherry-pick the commit on master then run git-p4 
> submit.
> It can be very annoying to switch between local branches and master, only to
> submit one commit.
> The proposed new way is to select directly the commit you want to submit.
>
> add option --commit to command 'git-p4 submit' in order to submit only 
> specified commit(s) in p4.
>
> On a daily work developping software with big compilation time, one may not 
> want
> to rebase on his local git tree, in order to avoid long recompilation.
>
> add option --disable-rebase to command 'git-p4 submit' in order to disable 
> rebase after submission.

I've been using this for real and it works well for me. Ack.

Because of the way I'm using git-p4, the --disable-rebase option
doesn't really help me - I really need a --disable-sync option but
that's a different feature.

Thanks
Luke



> ---
>  Documentation/git-p4.txt | 14 ++
>  git-p4.py| 29 +++--
>  t/t9807-git-p4-submit.sh | 40 
>  3 files changed, 77 insertions(+), 6 deletions(-)
>
> diff --git a/Documentation/git-p4.txt b/Documentation/git-p4.txt
> index d8c8f11c9..88d109deb 100644
> --- a/Documentation/git-p4.txt
> +++ b/Documentation/git-p4.txt
> @@ -149,6 +149,12 @@ To specify a branch other than the current one, use:
>  $ git p4 submit topicbranch
>  
>
> +To specify a single commit or a range of commits, use:
> +
> +$ git p4 submit --commit 
> +$ git p4 submit --commit 
> +
> +
>  The upstream reference is generally 'refs/remotes/p4/master', but can
>  be overridden using the `--origin=` command-line option.
>
> @@ -330,6 +336,14 @@ These options can be used to modify 'git p4 submit' 
> behavior.
> p4/master.  See the "Sync options" section above for more
> information.
>
> +--commit |::
> +Submit only the specified commit or range of commits, instead of the full
> +list of changes that are in the current Git branch.
> +
> +--disable-rebase::
> +Disable the automatic rebase after all commits have been successfully
> +submitted.
> +
>  Rebase options
>  ~~
>  These options can be used to modify 'git p4 rebase' behavior.
> diff --git a/git-p4.py b/git-p4.py
> index 7bb9cadc6..f4a6f3b4c 100755
> --- a/git-p4.py
> +++ b/git-p4.py
> @@ -1352,7 +1352,12 @@ class P4Submit(Command, P4UserMap):
>  optparse.make_option("--update-shelve", 
> dest="update_shelve", action="append", type="int",
>   metavar="CHANGELIST",
>   help="update an existing shelved 
> changelist, implies --shelve, "
> -   "repeat in-order for multiple 
> shelved changelists")
> +   "repeat in-order for multiple 
> shelved changelists"),
> +optparse.make_option("--commit", dest="commit", 
> metavar="COMMIT",
> + help="submit only the specified 
> commit(s), one commit or xxx..xxx"),
> +optparse.make_option("--disable-rebase", 
> dest="disable_rebase", action="store_true",
> + help="Disable rebase after submit is 
> completed. Can be useful if you "
> + "work from a local git branch that is 
> not master")
>  ]
>  self.description = "Submit changes from git to the perforce depot."
>  self.usage += " [name of git branch to submit into perforce depot]"
> @@ -1362,6 +1367,8 @@ class P4Submit(Command, P4UserMap):
>  self.dry_run = False
>  self.shelve = False
>  self.update_shelve = list()
> +self.commit = ""
> +self.disable_rebase = False
>  self.prepare_p4_only = False
>  self.conflict_behavior = None
>  self.isWindows = (platform.system() == "Windows")
> @@ -2103,9 +2110,18 @@ class P4Submit(Command, P4UserMap):
>  else:
>  commitish = 'HEAD'
>
> -for line in read_pipe_lines(["git", "rev-list", "--no-merges", 
> "%s..%s" % (self.origin, commitish)]):
> -commits.append(line.strip())
> -commits.reverse()
> +if self.commit != "":
> +if self.commit.find("..") != -1:
> +limits_ish = self.commit.split("..")
> +for line in read_pipe_lines(["git", "rev-list", 
> "--no-merges", "%s..%s" % (limits_ish[0], limits_ish[1])]):
> +commits.append(line.strip())
> +commits.reverse()
> +else:
> +commits.append(self.commit)
> +else:
> +for line in read_pipe_lines(["git", "rev-list", "--no-merges", 
> "%s..%s" % (self.origin, commitish)]):
> +   

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

[Luke Diamand]

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

For example:

  $ git p4 unshelve 12345

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

If that branch already exists, it is renamed - for example
the above branch would be saved as p4/unshelved/12345.1.

git-p4 checks that the shelved changelist is based on files
which are at the same Perforce revision as the origin branch
being used for the unshelve (HEAD by default). If they are not,
it will refuse to unshelve. This is to ensure that the unshelved
change does not contain other changes mixed-in.

The reference branch can be changed manually with the "--origin"
option.

The change adds a new Unshelve command class. This just runs the
existing P4Sync code tweaked to handle a shelved changelist.

Signed-off-by: Luke Diamand <l...@diamand.org>
---
 Documentation/git-p4.txt |  32 ++
 git-p4.py| 207 ---
 t/t9832-unshelve.sh  | 153 +
 3 files changed, 356 insertions(+), 36 deletions(-)
 create mode 100755 t/t9832-unshelve.sh

diff --git a/Documentation/git-p4.txt b/Documentation/git-p4.txt
index d8c8f11c9f..d3cb249fc2 100644
--- a/Documentation/git-p4.txt
+++ b/Documentation/git-p4.txt
@@ -164,6 +164,31 @@ $ 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 origin revision (HEAD by 
default).
+If the shelved changelist's parent revisions differ, git-p4 will refuse to 
unshelve;
+you need to be unshelving onto an equivalent tree.
+
+The origin revision can be changed with the "--origin" option.
+
+If the target branch in refs/remotes/p4/unshelved already exists, the old one 
will
+be renamed.
+
+
+$ git p4 sync
+$ git p4 unshelve 12345
+$ git show refs/remotes/p4/unshelved/12345
+
+$ git p4 unshelve 12345
+
+
+
+
 OPTIONS
 ---
 
@@ -337,6 +362,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..9390d58a84 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)))
 
@@ -662,6 +667,12 @@ def gitBranchExists(branch):
 stderr=subprocess.PIPE, stdout=subprocess.PIPE);
 return proc.wait() == 0;
 
+def gitUpdateRef(ref, newvalue):
+subprocess.check_call(["git", "update-ref", ref, newvalue])
+
+def gitDeleteRef(ref):
+subprocess.check_call(["git", "update-ref", "-d", ref])
+
 _gitConfig = {}
 
 def gitConfig(key, typeSpecifier=None):
@@ -2411,6 +2422,7 @@ class P4Sync(Command, P4UserMap):
 self.tempBranches = []
 self.tempBranchLocation = "refs/git-p4-tmp"
 self.largeFileSystem = None
+self.suppress_meta_comment = False
 
 if gitConfig('git-p4.largeFileSystem'):
 largeFileSystemConstructor = 
globals()[gitConfig('git-p4.largeFileSystem')]
@@ -2421,6 +2433,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")
@@

[PATCHv4 0/1] git-p4: unshelving: fix for python2.6

[Luke Diamand]

This is the same as the previous unshelve change, other than fixing
the "{}".format(foo) constructs I introduced to be compatible with
Python2.6.

https://marc.info/?l=git=152637850403462

Python2.6 doesn't understand empty format fields ("{}"), so I have
added element indexes, e.g. "{0}".format(foo).

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

 Documentation/git-p4.txt |  32 ++
 git-p4.py| 207 ---
 t/t9832-unshelve.sh  | 153 +
 3 files changed, 356 insertions(+), 36 deletions(-)
 create mode 100755 t/t9832-unshelve.sh

-- 
2.17.0.392.gdeb1a6e9b7

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

[Luke Diamand]

On 19 May 2018 at 12:26, merlo...@yahoo.fr <merlo...@yahoo.fr> wrote:
> Hi Luke,
>
> In the P4Unshelve classe, could you add an help description directly in the
> optparse.add_option of --origin ?

Sure. I'll do a re-roll.


>
> From the workfow point of you, do you think it will be possible to make
> changes in the git branch of the unshelved CL (remotes/p4/unshelved/),
> then update the p4 shelved CL directly ? It would be great.

That's an interesting idea. You would need to check it out somehow,
but then it should just work.

i.e.

$ git p4 unshelve 
$ git checkout remotes/p4/unshelved/
$ vim foo.c
$ git commit --amend foo.c
$ git p4 submit --origin HEAD^ --update-shelve 

I think it should work as-is.


>
> Romain
>
> Envoyé depuis Yahoo Mail pour Android
>
> Le sam., mai 19, 2018 à 12:00, Luke Diamand
> <l...@diamand.org> a écrit :
> 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
>
> If that branch already exists, it is renamed - for example
> the above branch would be saved as p4/unshelved/12345.1.
>
> git-p4 checks that the shelved changelist is based on files
> which are at the same Perforce revision as the origin branch
> being used for the unshelve (HEAD by default). If they are not,
> it will refuse to unshelve. This is to ensure that the unshelved
> change does not contain other changes mixed-in.
>
> The reference branch can be changed manually with the "--origin"
> option.
>
> The change adds a new Unshelve command class. This just runs the
> existing P4Sync code tweaked to handle a shelved changelist.
>
> Signed-off-by: Luke Diamand <l...@diamand.org>
> ---
> Documentation/git-p4.txt |  32 ++
> git-p4.py| 207 ---
> t/t9832-unshelve.sh  | 153 +
> 3 files changed, 356 insertions(+), 36 deletions(-)
> create mode 100755 t/t9832-unshelve.sh
>
> diff --git a/Documentation/git-p4.txt b/Documentation/git-p4.txt
> index d8c8f11c9f..d3cb249fc2 100644
> --- a/Documentation/git-p4.txt
> +++ b/Documentation/git-p4.txt
> @@ -164,6 +164,31 @@ $ 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 origin revision (HEAD by
> default).
> +If the shelved changelist's parent revisions differ, git-p4 will refuse to
> unshelve;
> +you need to be unshelving onto an equivalent tree.
> +
> +The origin revision can be changed with the "--origin" option.
> +
> +If the target branch in refs/remotes/p4/unshelved already exists, the old
> one will
> +be renamed.
> +
> +
> +$ git p4 sync
> +$ git p4 unshelve 12345
> +$ git show refs/remotes/p4/unshelved/12345
> +
> +$ git p4 unshelve 12345
> +
> +
> +
> +
> OPTIONS
> ---
>
> @@ -337,6 +362,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..9390d58a84 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)))
>
> @@ -662,6 +667,12 @@ def gitBranchExists(branch):
> stderr=subprocess.PIPE, st

[PATCHv3 0/1] git-p4 unshelve

[Luke Diamand]

This is a git-p4 unshelve command which detects when extra
changes are going to be included, and refuses to unhshelve.

As in my earlier unshelve change, this uses git fast-import
to do the actual delta generation, but now checks to see
if the files unshelved are based on the same revisions that
fast-import will be using, using the revision numbers in
the "p4 describe -S" command output. If they're different,
it refuses to unshelve.

That makes it safe to use, rather than potentially generating
deltas that contain bits from other changes.

I have added a test for this case.

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

 Documentation/git-p4.txt |  32 ++
 git-p4.py| 207 ---
 t/t9832-unshelve.sh  | 153 +
 3 files changed, 356 insertions(+), 36 deletions(-)
 create mode 100755 t/t9832-unshelve.sh

-- 
2.17.0.392.gdeb1a6e9b7

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

[Luke Diamand]

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

For example:

  $ git p4 unshelve 12345

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

If that branch already exists, it is renamed - for example
the above branch would be saved as p4/unshelved/12345.1.

git-p4 checks that the shelved changelist is based on files
which are at the same Perforce revision as the origin branch
being used for the unshelve (HEAD by default). If they are not,
it will refuse to unshelve. This is to ensure that the unshelved
change does not contain other changes mixed-in.

The reference branch can be changed manually with the "--origin"
option.

The change adds a new Unshelve command class. This just runs the
existing P4Sync code tweaked to handle a shelved changelist.

Signed-off-by: Luke Diamand <l...@diamand.org>
---
 Documentation/git-p4.txt |  32 ++
 git-p4.py| 207 ---
 t/t9832-unshelve.sh  | 153 +
 3 files changed, 356 insertions(+), 36 deletions(-)
 create mode 100755 t/t9832-unshelve.sh

diff --git a/Documentation/git-p4.txt b/Documentation/git-p4.txt
index d8c8f11c9f..d3cb249fc2 100644
--- a/Documentation/git-p4.txt
+++ b/Documentation/git-p4.txt
@@ -164,6 +164,31 @@ $ 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 origin revision (HEAD by 
default).
+If the shelved changelist's parent revisions differ, git-p4 will refuse to 
unshelve;
+you need to be unshelving onto an equivalent tree.
+
+The origin revision can be changed with the "--origin" option.
+
+If the target branch in refs/remotes/p4/unshelved already exists, the old one 
will
+be renamed.
+
+
+$ git p4 sync
+$ git p4 unshelve 12345
+$ git show refs/remotes/p4/unshelved/12345
+
+$ git p4 unshelve 12345
+
+
+
+
 OPTIONS
 ---
 
@@ -337,6 +362,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..dcff31a1d4 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)))
 
@@ -662,6 +667,12 @@ def gitBranchExists(branch):
 stderr=subprocess.PIPE, stdout=subprocess.PIPE);
 return proc.wait() == 0;
 
+def gitUpdateRef(ref, newvalue):
+subprocess.check_call(["git", "update-ref", ref, newvalue])
+
+def gitDeleteRef(ref):
+subprocess.check_call(["git", "update-ref", "-d", ref])
+
 _gitConfig = {}
 
 def gitConfig(key, typeSpecifier=None):
@@ -2411,6 +2422,7 @@ class P4Sync(Command, P4UserMap):
 self.tempBranches = []
 self.tempBranchLocation = "refs/git-p4-tmp"
 self.largeFileSystem = None
+self.suppress_meta_comment = False
 
 if gitConfig('git-p4.largeFileSystem'):
 largeFileSystemConstructor = 
globals()[gitConfig('git-p4.largeFileSystem')]
@@ -2421,6 +2433,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")
@@

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

[Luke Diamand]

On 21 May 2018 at 08:07, Junio C Hamano  wrote:
> SZEDER Gábor  writes:
>
>>> diff --git a/t/t9832-unshelve.sh b/t/t9832-unshelve.sh
>>> new file mode 100755
>>> index 00..cca2dec536
>
> ... in short, I'd queue a fix-up on top like this to be later
> squashed after getting an ack?

That looks good to me, thanks! Ack.

Luke

>
>  t/t9832-unshelve.sh | 23 ---
>  1 file changed, 4 insertions(+), 19 deletions(-)
>
> diff --git a/t/t9832-unshelve.sh b/t/t9832-unshelve.sh
> index cca2dec536..3513abd21a 100755
> --- a/t/t9832-unshelve.sh
> +++ b/t/t9832-unshelve.sh
> @@ -1,6 +1,6 @@
>  #!/bin/sh
>
> -last_shelved_change() {
> +last_shelved_change () {
> p4 changes -s shelved -m1 | cut -d " " -f 2
>  }
>
> @@ -17,7 +17,7 @@ test_expect_success 'init depot' '
> cd "$cli" &&
> echo file1 >file1 &&
> p4 add file1 &&
> -   p4 submit -d "change 1"
> +   p4 submit -d "change 1" &&
> : >file_to_delete &&
> p4 add file_to_delete &&
> p4 submit -d "file to delete"
> @@ -120,29 +120,14 @@ EOF
> )
>  '
>
> -diff_adds_line() {
> -   text="$1" &&
> -   file="$2" &&
> -   grep -q "^+$text" $file || (echo "expected \"text\" $text not found 
> in $file" && exit 1)
> -}
> -
> -diff_excludes_line() {
> -   text="$1" &&
> -   file="$2" &&
> -   if grep -q "^+$text" $file; then
> -   echo "unexpected text \"$text\" found in $file" &&
> -   exit 1
> -   fi
> -}
> -
>  # Now try to unshelve it. git-p4 should refuse to do so.
>  test_expect_success 'try to unshelve the change' '
> test_when_finished cleanup_git &&
> (
> change=$(last_shelved_change) &&
> cd "$git" &&
> -   ! git p4 unshelve $change >out.txt 2>&1 &&
> -   grep -q "cannot unshelve" out.txt
> +   test_must_fail git p4 unshelve $change 2>err.txt &&
> +   grep -q "cannot unshelve" err.txt
> )
>  '
>

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

[Luke Diamand]

On 21 May 2018 at 23:09, Luke Diamand <l...@diamand.org> wrote:
> On 21 May 2018 at 22:39, SZEDER Gábor <szeder@gmail.com> wrote:
>>> diff --git a/t/t9832-unshelve.sh b/t/t9832-unshelve.sh
>
> ...
> ...
>
>>> +'
>>
>> This test fails on my box and on Travis CI (in all four standard Linux
>> and OSX build jobs) with:
>>
>>   + cd /home/szeder/src/git/t/trash directory.t9832-unshelve/cli
>>   + p4 edit file1
>>   //depot/file1#1 - opened for edit
>>   + echo a change
>>   + echo new file
>>   + p4 add file2
>>   //depot/file2#1 - opened for add
>>   + p4 delete file_to_delete
>>   //depot/file_to_delete#1 - opened for delete
>>   + p4 opened
>>   //depot/file1#1 - edit default change (text)
>>   //depot/file2#1 - add default change (text)
>>   //depot/file_to_delete#1 - delete default change (text)
>>   + p4 shelve -i
>>   Change 3 created with 3 open file(s).
>>   Shelving files for change 3.
>>   edit //depot/file1#1
>>   add //depot/file2#1
>>   delete //depot/file_to_delete#1
>>   Change 3 files shelved.
>>   + cd /home/szeder/src/git/t/trash directory.t9832-unshelve/git
>>   + last_shelved_change
>>   + p4 changes -s shelved -m1
>>   + cut -d   -f 2
>>   + change=3
>>   + git p4 unshelve 3
>>   Traceback (most recent call last):
>> File "/home/szeder/src/git/git-p4", line 3975, in 
>>   main()
>> File "/home/szeder/src/git/git-p4", line 3969, in main
>>   if not cmd.run(args):
>> File "/home/szeder/src/git/git-p4", line 3851, in run
>>   sync.importChanges(changes, shelved=True,
>>   origin_revision=origin_revision)
>> File "/home/szeder/src/git/git-p4", line 3296, in importChanges
>>   files = self.extractFilesFromCommit(description, shelved, change,
>>   origin_revision)
>> File "/home/szeder/src/git/git-p4", line 2496, in
>>   extractFilesFromCommit
>>   not self.cmp_shelved(path, file["rev"], origin_revision):
>> File "/home/szeder/src/git/git-p4", line 2467, in cmp_shelved
>>   return ret["status"] == "identical"
>>   KeyError: 'status'
>>   error: last command exited with $?=1
>>   not ok 4 - create shelved changelist
>
> It works fine for me - but given where it's failing, my first
> suspicion would be p4 client version (or server) differences.
>
> I'm using 2015.1 for server and client. Could you check which version
> you are using?

In fact, no need. It works fine with 2015.1 but 2017.1 fails with this
error. Sigh. I'll reroll.

Luke


>
> Thanks,
> Luke

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

[Luke Diamand]

On 21 May 2018 at 22:39, SZEDER Gábor  wrote:
>> diff --git a/t/t9832-unshelve.sh b/t/t9832-unshelve.sh

...
...

>> +'
>
> This test fails on my box and on Travis CI (in all four standard Linux
> and OSX build jobs) with:
>
>   + cd /home/szeder/src/git/t/trash directory.t9832-unshelve/cli
>   + p4 edit file1
>   //depot/file1#1 - opened for edit
>   + echo a change
>   + echo new file
>   + p4 add file2
>   //depot/file2#1 - opened for add
>   + p4 delete file_to_delete
>   //depot/file_to_delete#1 - opened for delete
>   + p4 opened
>   //depot/file1#1 - edit default change (text)
>   //depot/file2#1 - add default change (text)
>   //depot/file_to_delete#1 - delete default change (text)
>   + p4 shelve -i
>   Change 3 created with 3 open file(s).
>   Shelving files for change 3.
>   edit //depot/file1#1
>   add //depot/file2#1
>   delete //depot/file_to_delete#1
>   Change 3 files shelved.
>   + cd /home/szeder/src/git/t/trash directory.t9832-unshelve/git
>   + last_shelved_change
>   + p4 changes -s shelved -m1
>   + cut -d   -f 2
>   + change=3
>   + git p4 unshelve 3
>   Traceback (most recent call last):
> File "/home/szeder/src/git/git-p4", line 3975, in 
>   main()
> File "/home/szeder/src/git/git-p4", line 3969, in main
>   if not cmd.run(args):
> File "/home/szeder/src/git/git-p4", line 3851, in run
>   sync.importChanges(changes, shelved=True,
>   origin_revision=origin_revision)
> File "/home/szeder/src/git/git-p4", line 3296, in importChanges
>   files = self.extractFilesFromCommit(description, shelved, change,
>   origin_revision)
> File "/home/szeder/src/git/git-p4", line 2496, in
>   extractFilesFromCommit
>   not self.cmp_shelved(path, file["rev"], origin_revision):
> File "/home/szeder/src/git/git-p4", line 2467, in cmp_shelved
>   return ret["status"] == "identical"
>   KeyError: 'status'
>   error: last command exited with $?=1
>   not ok 4 - create shelved changelist

It works fine for me - but given where it's failing, my first
suspicion would be p4 client version (or server) differences.

I'm using 2015.1 for server and client. Could you check which version
you are using?

Thanks,
Luke

Re: [PATCH 1/1] git-p4: unshelve: use action==add instead of rev==none

[Luke Diamand]

On 22 May 2018 at 11:15, SZEDER Gábor <szeder@gmail.com> wrote:
> On Tue, May 22, 2018 at 10:41 AM, Luke Diamand <l...@diamand.org> wrote:
>> SZEDER Gábor found that the unshelve tests fail with newer
>> versions of Perforce (2016 vs 2015).
>>
>> The problem arises because when a file is added in a P4
>> shelved changelist, the depot revision is shown as "none"
>> if using the older p4d (which makes sense - the file doesn't
>> yet exist, so can't have a revision), but as "1" in the newer
>> versions of p4d.
>>
>> For example, adding a file called "new" with 2015.1 and then
>> shelving that change gives this from "p4 describe" :
>>
>> ... //depot/new#none add
>>
>> Using the 2018.1 server gives this:
>>
>> ... //depot/new#1 add
>>
>> We can detect that a file has been added simply by using the
>> file status ("add") instead, rather than the depot revision,
>> which is what this change does.
>>
>> This also fixes a few verbose prints used for debugging this
>> to be more friendly.
>>
>> Signed-off-by: Luke Diamand <l...@diamand.org>
>
> For what it's worth, I can confirm that 't9832-unshelve.sh' passes
> with these changes, here and in all Linux and OSX build jobs on Travis
> CI.

Thanks!

>
> However, instead of a separate patch, wouldn't it be better to squash
> it into the previous one?  So 'make test' would succeed on every
> commit even with a newer p4 version.

Junio?

I can squash together the original commit and the two fixes if that
would be better?

Thanks!
Luke

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

[Luke Diamand]

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

For example:

  $ git p4 unshelve 12345

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

If that branch already exists, it is renamed - for example
the above branch would be saved as p4/unshelved/12345.1.

git-p4 checks that the shelved changelist is based on files
which are at the same Perforce revision as the origin branch
being used for the unshelve (HEAD by default). If they are not,
it will refuse to unshelve. This is to ensure that the unshelved
change does not contain other changes mixed-in.

The reference branch can be changed manually with the "--origin"
option.

The change adds a new Unshelve command class. This just runs the
existing P4Sync code tweaked to handle a shelved changelist.

Signed-off-by: Luke Diamand <l...@diamand.org>
---
 Documentation/git-p4.txt |  32 ++
 git-p4.py| 215 ---
 t/t9832-unshelve.sh  | 138 +
 3 files changed, 348 insertions(+), 37 deletions(-)
 create mode 100755 t/t9832-unshelve.sh

diff --git a/Documentation/git-p4.txt b/Documentation/git-p4.txt
index d8c8f11c9f..d3cb249fc2 100644
--- a/Documentation/git-p4.txt
+++ b/Documentation/git-p4.txt
@@ -164,6 +164,31 @@ $ 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 origin revision (HEAD by 
default).
+If the shelved changelist's parent revisions differ, git-p4 will refuse to 
unshelve;
+you need to be unshelving onto an equivalent tree.
+
+The origin revision can be changed with the "--origin" option.
+
+If the target branch in refs/remotes/p4/unshelved already exists, the old one 
will
+be renamed.
+
+
+$ git p4 sync
+$ git p4 unshelve 12345
+$ git show refs/remotes/p4/unshelved/12345
+
+$ git p4 unshelve 12345
+
+
+
+
 OPTIONS
 ---
 
@@ -337,6 +362,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..c80d85af89 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)))
 
@@ -662,6 +667,12 @@ def gitBranchExists(branch):
 stderr=subprocess.PIPE, stdout=subprocess.PIPE);
 return proc.wait() == 0;
 
+def gitUpdateRef(ref, newvalue):
+subprocess.check_call(["git", "update-ref", ref, newvalue])
+
+def gitDeleteRef(ref):
+subprocess.check_call(["git", "update-ref", "-d", ref])
+
 _gitConfig = {}
 
 def gitConfig(key, typeSpecifier=None):
@@ -2411,6 +2422,7 @@ class P4Sync(Command, P4UserMap):
 self.tempBranches = []
 self.tempBranchLocation = "refs/git-p4-tmp"
 self.largeFileSystem = None
+self.suppress_meta_comment = False
 
 if gitConfig('git-p4.largeFileSystem'):
 largeFileSystemConstructor = 
globals()[gitConfig('git-p4.largeFileSystem')]
@@ -2421,6 +2433,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")
@@

[PATCHv5 0/1] git-p4: unshelve: fix problem with newer p4d

[Luke Diamand]

This is v5 of my git-p4 unshelve patch. It fixes a problem found by
SZEDER Gábor with newer versions of Perforce (2016 vs 2015).  

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

 Documentation/git-p4.txt |  32 ++
 git-p4.py| 215 ---
 t/t9832-unshelve.sh  | 138 +
 3 files changed, 348 insertions(+), 37 deletions(-)
 create mode 100755 t/t9832-unshelve.sh

-- 
2.17.0.392.gdeb1a6e9b7

[PATCHv6 0/1] git-p4: unshelve

[Luke Diamand]

This just removes the verbose print change, which will end up causing
conflicts later since it's also being fixed in another commit.

Discussed here:

https://public-inbox.org/git/byapr08mb38455afe85ae6b04eb31ef92da...@byapr08mb3845.namprd08.prod.outlook.com/

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

 Documentation/git-p4.txt |  32 ++
 git-p4.py| 213 ---
 t/t9832-unshelve.sh  | 138 +
 3 files changed, 347 insertions(+), 36 deletions(-)
 create mode 100755 t/t9832-unshelve.sh

-- 
2.17.0.392.gdeb1a6e9b7

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

[Luke Diamand]

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

For example:

  $ git p4 unshelve 12345

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

If that branch already exists, it is renamed - for example
the above branch would be saved as p4/unshelved/12345.1.

git-p4 checks that the shelved changelist is based on files
which are at the same Perforce revision as the origin branch
being used for the unshelve (HEAD by default). If they are not,
it will refuse to unshelve. This is to ensure that the unshelved
change does not contain other changes mixed-in.

The reference branch can be changed manually with the "--origin"
option.

The change adds a new Unshelve command class. This just runs the
existing P4Sync code tweaked to handle a shelved changelist.

Signed-off-by: Luke Diamand <l...@diamand.org>
---
 Documentation/git-p4.txt |  32 ++
 git-p4.py| 213 ---
 t/t9832-unshelve.sh  | 138 +
 3 files changed, 347 insertions(+), 36 deletions(-)
 create mode 100755 t/t9832-unshelve.sh

diff --git a/Documentation/git-p4.txt b/Documentation/git-p4.txt
index d8c8f11c9f..d3cb249fc2 100644
--- a/Documentation/git-p4.txt
+++ b/Documentation/git-p4.txt
@@ -164,6 +164,31 @@ $ 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 origin revision (HEAD by 
default).
+If the shelved changelist's parent revisions differ, git-p4 will refuse to 
unshelve;
+you need to be unshelving onto an equivalent tree.
+
+The origin revision can be changed with the "--origin" option.
+
+If the target branch in refs/remotes/p4/unshelved already exists, the old one 
will
+be renamed.
+
+
+$ git p4 sync
+$ git p4 unshelve 12345
+$ git show refs/remotes/p4/unshelved/12345
+
+$ git p4 unshelve 12345
+
+
+
+
 OPTIONS
 ---
 
@@ -337,6 +362,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..74d58afad3 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)))
 
@@ -662,6 +667,12 @@ def gitBranchExists(branch):
 stderr=subprocess.PIPE, stdout=subprocess.PIPE);
 return proc.wait() == 0;
 
+def gitUpdateRef(ref, newvalue):
+subprocess.check_call(["git", "update-ref", ref, newvalue])
+
+def gitDeleteRef(ref):
+subprocess.check_call(["git", "update-ref", "-d", ref])
+
 _gitConfig = {}
 
 def gitConfig(key, typeSpecifier=None):
@@ -2411,6 +2422,7 @@ class P4Sync(Command, P4UserMap):
 self.tempBranches = []
 self.tempBranchLocation = "refs/git-p4-tmp"
 self.largeFileSystem = None
+self.suppress_meta_comment = False
 
 if gitConfig('git-p4.largeFileSystem'):
 largeFileSystemConstructor = 
globals()[gitConfig('git-p4.largeFileSystem')]
@@ -2421,6 +2433,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")
@@

Re: What's cooking in git.git (May 2018, #03; Wed, 23)

[Luke Diamand]

On 24 May 2018 at 01:02, Junio C Hamano  wrote:
> Here are the topics that have been cooking.  Commits prefixed with
> '-' are only in 'pu' (proposed updates) while commits prefixed with
> '+' are in 'next'.  The ones marked with '.' do not appear in any of
> the integration branches, but I am still holding onto them.
>
> You can find the changes described here in the integration branches
> of the repositories listed at
>
> http://git-blame.blogspot.com/p/git-public-repositories.html
>




>
>
> * ld/p4-unshelve (2018-05-23) 3 commits
>  - git-p4: unshelve: use action==add instead of rev==none
>  - SQUASH???
>  - git-p4: add unshelve command
>
>  "git p4" learned to "unshelve" shelved commit from P4.
>
>  Expecting a reroll.
>  cf. 

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

[Luke Diamand]

Exiting"
> +return True
> +
> +# get the diff and copy to diff directory
> +for f in editedFiles:
> +p4diff = p4_read_pipe(['diff2', '-du', 
> f["depotFile"]+'#'+f["rev"], f["depotFile"]+'@='+self.unshelve])
> +p4diff = "\n".join(p4diff.split("\n")[1:])
> +p4diff = '--- '+f["gitFile"]+'\n' + '+++ '+f["gitFile"]+'\n' 
> + p4diff
> +write_pipe(['patch', '-d/', '-p0'], p4diff)
> +write_pipe(['git', 'add', '-f', f["gitFile"]], "")
> +for f in filesToAdd:
> +p4_write_pipe(['print', '-o', f["gitFile"], 
> f["depotFile"]+'@='+self.unshelve], "")
> +write_pipe(['git', 'add', '-f', f["gitFile"]], "")
> +for f in filesToDelete:
> +os.remove(f["gitFile"])
> +write_pipe(['git', 'rm', f["gitFile"]], "")
> +for f in binaryFiles:
> +p4_write_pipe(['print', '-o', f["gitFile"], 
> f["depotFile"]+'@='+self.unshelve], "")
> +write_pipe(['git', 'add', '-f', f["gitFile"]], "")
> +
> +# finalize: commit in git
> +write_pipe(['git', 'commit', '-m', description["desc"]], "")
> +return True
> +
>  print "Perforce checkout for depot path %s located at %s" % 
> (self.depotPath, self.clientPath)
>  self.oldWorkingDirectory = os.getcwd()
>
> Romain
>
>
>
> Le samedi 12 mai 2018 à 23:24:48 UTC+2, Luke Diamand <l...@diamand.org> a 
> écrit :
>
>
>
>
>
> 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
>
> If that branch already exists, it is renamed - for example
> the above branch would be saved as p4/unshelved/12345.1.
>
> Caveat:
>
> The unshelving is done against the current "p4/master" branch;
> git-p4 uses "p4 print" to get the file contents at the requested
> revision, and then fast-import creates a commit relative to p4/master.
>
> Ideally what you would want is for fast-import to create the
> commit based on the Perforce "revision" prior to the shelved commit,
> but Perforce doesn't have such a concept - to do this, git-p4
> would need to figure out the revisions of the individual files
> before the shelved changelist, and then construct a temporary
> git branch which matched this.
>
> It's possible to do this, but doing so makes this change a lot more
> complicated.
>
> This limitation means that if you unshelve a change where some
> of the changed files were not based on p4/master, you will get
> an amalgam of the change you wanted, and these other changes.
>
> The reference branch can be changed manually with the "--origin"
> option.
>
> The change adds a new Unshelve command class. This just runs the
> existing P4Sync code tweaked to handle a shelved changelist.
>
> Signed-off-by: Luke Diamand <l...@diamand.org>

Re: [PATCH 1/1] git-p4: unshelve: use action==add instead of rev==none

[Luke Diamand]

On 23 May 2018 at 17:41, Mazo, Andrey  wrote:
>> The last one (i.e. "even if it is verbose, if fileSize is not
>> reported, do not write the verbose output") does not look like it is
>> limited to the unshelve feature, so it might, even though it is a
>> one-liner, deserve to be a separate preparatory patch if you want.
>> But I do not feel strongly about either way.
>
> This was actually discussed in a separate thread [1] some time ago with 
> patches proposed by Thandesha and me.
> I haven't yet got time to cook a final patch, which addresses both 
> Thandesha's and mine use-cases though,
> so this wasn't submitted to Junio yet.
> In the meantime, I guess, one of the patches [2] from that thread can be 
> taken as is.
>
> [1] "[BUG] git p4 clone fails when p4 sizes does not return 'fileSize' key"
>   
> https://public-inbox.org/git/cajjpmi-plb4qcka5alkxa8b1vozfff+oaq0fguq9yviobrp...@mail.gmail.com/t/#mee2ec50a40242089741f808f06214a44278055b3
> [2] "[PATCH 1/1] git-p4: fix `sync --verbose` traceback due to 'fileSize'"
>   
> https://public-inbox.org/git/2e2b2add4e4fffa4228b8ab9f6cd47fa9bf25207.1523981210.git.am...@checkvideo.com/

Should I re-roll my patch without this change then?

Luke

[PATCH 1/1] git-p4: unshelve: use action==add instead of rev==none

[Luke Diamand]

SZEDER Gábor found that the unshelve tests fail with newer
versions of Perforce (2016 vs 2015).

The problem arises because when a file is added in a P4
shelved changelist, the depot revision is shown as "none"
if using the older p4d (which makes sense - the file doesn't
yet exist, so can't have a revision), but as "1" in the newer
versions of p4d.

For example, adding a file called "new" with 2015.1 and then
shelving that change gives this from "p4 describe" :

... //depot/new#none add

Using the 2018.1 server gives this:

... //depot/new#1 add

We can detect that a file has been added simply by using the
file status ("add") instead, rather than the depot revision,
which is what this change does.

This also fixes a few verbose prints used for debugging this
to be more friendly.

Signed-off-by: Luke Diamand <l...@diamand.org>
---
 git-p4.py | 11 ---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/git-p4.py b/git-p4.py
index 364d86dbcc..c80d85af89 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -2463,7 +2463,7 @@ class P4Sync(Command, P4UserMap):
 """
 ret = p4Cmd(["diff2", "{0}#{1}".format(path, filerev), 
"{0}@{1}".format(path, revision)])
 if verbose:
-print("p4 diff2 %s %s %s => %s" % (path, filerev, revision, ret))
+print("p4 diff2 path %s filerev %s revision %s => %s" % (path, 
filerev, revision, ret))
 return ret["status"] == "identical"
 
 def extractFilesFromCommit(self, commit, shelved=False, shelved_cl = 0, 
origin_revision = 0):
@@ -2492,7 +2492,12 @@ class P4Sync(Command, P4UserMap):
 if shelved:
 file["shelved_cl"] = int(shelved_cl)
 
-if file["rev"] != "none" and \
+# For shelved changelists, check that the revision of each 
file that the
+# shelve was based on matches the revision that we are using 
for the
+# starting point for git-fast-import (self.initialParent). 
Otherwise
+# the resulting diff will contain deltas from multiple commits.
+
+if file["action"] != "add" and \
 not self.cmp_shelved(path, file["rev"], origin_revision):
 sys.exit("change {0} not based on {1} for {2}, cannot 
unshelve".format(
 commit["change"], self.initialParent, path))
@@ -2610,7 +2615,7 @@ class P4Sync(Command, P4UserMap):
 def streamOneP4File(self, file, contents):
 relPath = self.stripRepoPath(file['depotFile'], self.branchPrefixes)
 relPath = self.encodeWithUTF8(relPath)
-if verbose:
+if verbose and 'fileSize' in self.stream_file:
 size = int(self.stream_file['fileSize'])
 sys.stdout.write('\r%s --> %s (%i MB)\n' % (file['depotFile'], 
relPath, size/1024/1024))
 sys.stdout.flush()
-- 
2.17.0.392.gdeb1a6e9b7

[PATCH 0/1] git-p4: unshelving: fix problem with newer P4

[Luke Diamand]

This fixes a problem found by SZEDER Gábor with newer versions of
the Perforce database engine (2016 onwards). It looks like the
behaviour has change subtly when reporting the revision of newly
added files. The fix is to just use the file status.

Luke Diamand (1):
  git-p4: unshelve: use action==add instead of rev==none

 git-p4.py | 11 ---
 1 file changed, 8 insertions(+), 3 deletions(-)

-- 
2.17.0.392.gdeb1a6e9b7

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

[Luke Diamand]

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

Works OK on Windows with 2017.2 client/server.