Re: [PATCH 0/2] git-p4: fix for handling of multiple depot paths
I'm actually surprised that the patch changes the order at all, since all it does is affect the decision (on a yes/no basis) to include a given file into a changelist. I'm going to have a look at that specific unit test, but of course as a user I'd prefer if the default behaviour could remain the same, unless it was actually a bug. -- Sam. On Tue, Dec 15, 2015, James Farwell wrote: > I'm not sure if my opinion as an outsider is of use, but since the perforce > change number is monotonically increasing, my expectation as a user would be > for them to be applied in order by the perforce change number. :) > > - James > > > From: Luke Diamand > Sent: Monday, December 14, 2015 3:09 PM > To: Junio C Hamano > Cc: Git Users; James Farwell; Lars Schneider; Eric Sunshine; Sam Hocevar > Subject: Re: [PATCH 0/2] git-p4: fix for handling of multiple depot paths > > Sorry - I've just run the tests, and this change causes one of the > test cases in t9800-git-p4-basic.sh to fail. > > It looks like the test case makes an assumption about who wins if two > P4 depots have changes to files that end up in the same place, and > this change reverses the order. It may actually be fine, but it needs > to be thought about a bit. > > Sam - do you have any thoughts on this? > > Thanks > Luke > > > > > > > On 14 December 2015 at 22:06, Junio C Hamano wrote: > > Luke Diamand writes: > > > >> On 14 December 2015 at 19:16, Junio C Hamano wrote: > >>> Luke Diamand writes: > >>> > >>>> Having just fixed this, I've now just spotted that Sam Hocevar's fix > >>>> to reduce the number of P4 transactions also fixes it: > >>>> > >>>> https://urldefense.proofpoint.com/v2/url?u=https-3A__www.mail-2Darchive.com_git-2540vger.kernel.org_msg81880.html&d=BQIBaQ&c=Sqcl0Ez6M0X8aeM67LKIiDJAXVeAw-YihVMNtXt-uEs&r=wkCayFhpIBdAOEa7tZDTcd1weqwtiFMEIQTL-WQPwC4&m=q8dsOAHvUiDzzPNGRAfMMrcXstxNlI-v7I_03uEL1e8&s=C8wVLMC-iU7We0r36sxOuu920ZjZYdpy7ysNi_5PYv8&e= > >>>> > >>>> That seems like a cleaner fix. > >>> > >>> Hmm, do you mean I should ignore this series and take the other one, > >>> take only 1/2 from this for tests and then both patches in the other > >>> one, or something else? > >> > >> The second of those (take only 1/2 from this for tests, and then both > >> from the other) seems like the way to go. > > > > OK. Should I consider the two patches from Sam "Reviewed-by" you? echo "creationism" | tr -d "holy godly goal" -- 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 1/2] git-p4: support multiple depot paths in p4 submit
On Tue, Dec 08, 2015, Lars Schneider wrote: > > Would a refactor of lib-git-p4.sh (and probably all git-p4 tests) to > > support multiple depots be acceptable and/or welcome? I prefer to ask > > before I dig into the task. > > Can you outline your idea a bit? Are you aware of the following way to define > client specs: [1] ? Would that help? That's the idea, but the bug occurs when the client view looks like this: //depot/... //client/dir1/... //depot2/... //client/dir2/... And is then cloned with (it is not legal in Perforce to specify //... directly to grab both depots at once): git p4 clone --use-client-spec //depot/... //depot2/... Then when a file is modified in dir2/, git p4 submit does not elect it for the changelist. A file in dir1/ will work fine. Unfortunately the current test suite assumes everything is under //depot/ so in order to write a test for this situation there are a few things to change in lib-git-p4.sh. Regards, -- Sam. -- 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 1/2] git-p4: support multiple depot paths in p4 submit
On Sun, Dec 06, 2015, Lars Schneider wrote: > Thanks for the patch! Do you see a way to demonstrate the bug in a test case > similar to t9821 [1]? Not yet, I'm afraid. It's proving trickier than expected because for now I can only reproduce the bug when the view uses multiples depots (solution #2 on http://answers.perforce.com/articles/KB/2437), and unfortunately the test case system in Git was designed for a single depot. Would a refactor of lib-git-p4.sh (and probably all git-p4 tests) to support multiple depots be acceptable and/or welcome? I prefer to ask before I dig into the task. Regards, -- Sam. -- 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: support multiple depot paths in p4 submit
When submitting from a repository that was cloned using a client spec,
use the full list of paths when ruling out files that are outside the
view. This fixes a bug where only files pertaining to the first path
would be included in the p4 submit.
Signed-off-by: Sam Hocevar
---
git-p4.py | 11 +--
1 file changed, 9 insertions(+), 2 deletions(-)
diff --git a/git-p4.py b/git-p4.py
index a79b6d8..210f100 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -1253,6 +1253,8 @@ class P4Submit(Command, P4UserMap):
Remove lines in the Files section that show changes to files
outside the depot path we're committing into."""
+[upstream, settings] = findUpstreamBranchPoint()
+
template = ""
inFilesSection = False
for line in p4_read_pipe_lines(['change', '-o']):
@@ -1265,8 +1267,13 @@ class P4Submit(Command, P4UserMap):
lastTab = path.rfind("\t")
if lastTab != -1:
path = path[:lastTab]
-if not p4PathStartsWith(path, self.depotPath):
-continue
+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
else:
inFilesSection = False
else:
--
2.6.2
--
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 2/2] git-p4: reduce number of server queries for fetches
When fetching changes from a depot using a full client spec, there
is no need to perform as many queries as there are top-level paths
in the client spec. Instead we query all changes in chronological
order, also getting rid of the need to sort the results and remove
duplicates.
Signed-off-by: Sam Hocevar
---
git-p4.py | 43 ---
1 file changed, 20 insertions(+), 23 deletions(-)
diff --git a/git-p4.py b/git-p4.py
index 210f100..ea2bbb2 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -796,39 +796,36 @@ def p4ChangesForPaths(depotPaths, changeRange,
requestedBlockSize):
die("cannot use --changes-block-size with non-numeric
revisions")
block_size = None
-# Accumulate change numbers in a dictionary to avoid duplicates
-changes = {}
+changes = []
-for p in depotPaths:
-# Retrieve changes a block at a time, to prevent running
-# into a MaxResults/MaxScanRows error from the server.
+# Retrieve changes a block at a time, to prevent running
+# into a MaxResults/MaxScanRows error from the server.
-while True:
-cmd = ['changes']
+while True:
+cmd = ['changes']
-if block_size:
-end = min(changeEnd, changeStart + block_size)
-revisionRange = "%d,%d" % (changeStart, end)
-else:
-revisionRange = "%s,%s" % (changeStart, changeEnd)
+if block_size:
+end = min(changeEnd, changeStart + block_size)
+revisionRange = "%d,%d" % (changeStart, end)
+else:
+revisionRange = "%s,%s" % (changeStart, changeEnd)
+for p in depotPaths:
cmd += ["%s...@%s" % (p, revisionRange)]
-for line in p4_read_pipe_lines(cmd):
-changeNum = int(line.split(" ")[1])
-changes[changeNum] = True
+# Insert changes in chronological order
+for line in reversed(p4_read_pipe_lines(cmd)):
+changes.append(int(line.split(" ")[1]))
-if not block_size:
-break
+if not block_size:
+break
-if end >= changeEnd:
-break
+if end >= changeEnd:
+break
-changeStart = end + 1
+changeStart = end + 1
-changelist = changes.keys()
-changelist.sort()
-return changelist
+return changes
def p4PathStartsWith(path, prefix):
# This method tries to remedy a potential mixed-case issue:
--
2.6.2
--
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
