Re: [PATCH 0/2] git-p4: fix for handling of multiple depot paths
On 16/12/15 00:38, Sam Hocevar wrote: 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. We ask for changes in //depot/sub1/...@1,6 //depot/sub2/...@1,6' which gives us [4, 6, 3, 5]. The old code used to sort this list but this change removes the sort. Maybe putting the sort back would fix it? 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. In answer to James' question, the test checks that the most recent change wins (i.e. applied in order). 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: 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 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 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?-- 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: 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://www.mail-archive.com/git%40vger.kernel.org/msg81880.html 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
Re: [PATCH 0/2] git-p4: fix for handling of multiple depot paths
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://www.mail-archive.com/git%40vger.kernel.org/msg81880.html >>> >>> 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
Re: [PATCH 0/2] git-p4: fix for handling of multiple depot paths
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://www.mail-archive.com/git%40vger.kernel.org/msg81880.html >> >> 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. 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 0/2] git-p4: fix for handling of multiple depot paths
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://www.mail-archive.com/git%40vger.kernel.org/msg81880.html > > 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? Thanks. > > Luke > > > On 13 December 2015 at 20:07, Luke Diamand wrote: >> James Farwell reported a bug I introduced into git-p4 with >> handling of multiple depot paths: >> >> http://article.gmane.org/gmane.comp.version-control.git/282297 >> >> This patch series adds a failing test case, and a fix for this >> problem. >> >> Luke >> >> Luke Diamand (2): >> git-p4: failing test case for skipping changes with multiple depots >> git-p4: fix handling of multiple depot paths >> >> git-p4.py | 8 +--- >> t/t9818-git-p4-block.sh | 28 +++- >> 2 files changed, 32 insertions(+), 4 deletions(-) >> >> -- >> 2.6.2.474.g3eb3291 >> -- 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: fix for handling of multiple depot paths
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://www.mail-archive.com/git%40vger.kernel.org/msg81880.html That seems like a cleaner fix. Luke On 13 December 2015 at 20:07, Luke Diamand wrote: > James Farwell reported a bug I introduced into git-p4 with > handling of multiple depot paths: > > http://article.gmane.org/gmane.comp.version-control.git/282297 > > This patch series adds a failing test case, and a fix for this > problem. > > Luke > > Luke Diamand (2): > git-p4: failing test case for skipping changes with multiple depots > git-p4: fix handling of multiple depot paths > > git-p4.py | 8 +--- > t/t9818-git-p4-block.sh | 28 +++- > 2 files changed, 32 insertions(+), 4 deletions(-) > > -- > 2.6.2.474.g3eb3291 > -- 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
