Bug: git-p4 can generate duplicate commits when syncing changes that span multiple depot paths
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. Expected Behavior: Change should be synced as a single commit to the git repo. Actual Behavior: Change is synced as multiple commits, one for each depot path that was affected. Best Guess: I believe this is happening because the command syntax "p4 changes //depot/foo/...@123,456 //depot/bar/...@123,456", which git-p4 uses to get the list of changes to sync, will return the same change number multiple times if the change was present in multiple depot paths. This is expected behavior as per the p4 changes documentation: "If p4 changes is called with multiple file arguments, the sets of changelists that affect each argument are evaluated individually. The final output is neither combined nor sorted; the effect is the same as calling p4 changes multiple times, once for each file argument." git-p4 is handling the sorting itself, but it is not handling the combining. I would imagine this is fixable in the p4ChangesForPaths() method by dropping non-unique elements of the list before or after sorting. Rudimentary testing in the python interpreter would suggest that something like "changes = sorted(set(changes))" should do the trick, but I am no python expert so there may be a better way. Thanks! - James
Re: [PATCH 0/2] git-p4: fix for handling of multiple depot paths
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 <l...@diamand.org> 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 <gits...@pobox.com> wrote: > Luke Diamand <l...@diamand.org> writes: > >> On 14 December 2015 at 19:16, Junio C Hamano <gits...@pobox.com> wrote: >>> Luke Diamand <l...@diamand.org> 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=BQIBaQ=Sqcl0Ez6M0X8aeM67LKIiDJAXVeAw-YihVMNtXt-uEs=wkCayFhpIBdAOEa7tZDTcd1weqwtiFMEIQTL-WQPwC4=q8dsOAHvUiDzzPNGRAfMMrcXstxNlI-v7I_03uEL1e8=C8wVLMC-iU7We0r36sxOuu920ZjZYdpy7ysNi_5PYv8= >>>> >>>> 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?-- 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
Bug: git-p4 can skip changes when syncing large from multiple depot paths
Reproduction Steps: 1. Have a git repo cloned from a perforce repo using multiple depot paths (e.g. //depot/foo/ and //depot/bar/). 2. Add changes to the perforce repo in both depot paths. (e.g. 5 changes in each) 2. Do a "git p4 sync --changes_block_size n" where n is smaller than the number of changes applied to each depot path. (e.g. 2) Expected Behavior: All changes should sync and become commits in the git repo. Actual Behavior: All changes from the first depot path (if any) sync. After that only a small subset of changes from the remaining depot paths sync, causing some changes to be skipped entirely. Best Guess: I believe this was introduced in commit 1051ef00636357061d72bcf673da86054fb14a12. The important code in question is the p4ChangesForPaths function, which contains a for loop that iterates over the depot paths, which then contains a while loop which iterates over the blocks. This change modified the inner while loop so that with every iteration it modifies changeStart, which causes the original value of changeStart to be lost. The first iteration of the for loop will correctly iterate across all the blocks until changeStart is within one block of the last change number, but then all subsequent iterations of the for loop will use that final changeStart value, causing any changes in those depot paths in earlier blocks to be skipped. This can probably be easily remedied by using a temporary "start" variable for the block iteration, much like there is already a temporary "end" variable, and resetting it to the value of changeStart at the top of the for loop. (Note: this appears to be how the code prior to 1051ef00636357061d72bcf673da86054fb14a12 functioned). Thanks! - James-- 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
