Re: [PATCH] git-p4: Do not include diff in spec file when just preparing p4
frrr...@gmail.com wrote on Sat, 24 May 2014 02:39 +0100: > The diff information render the spec file unusable as is by p4, > do not include it when run with --prepare-p4-only so that the > given file can be directly passed to p4. > > With --prepare-p4-only, git-p4 already tells the user it can use > p4 submit with the generated spec file. This fails because of the > diff being present in the file. Not including the diff fixes that. > > Without --prepare-p4-only, keeping the diff makes sense for a > quick review of the patch before submitting it. And does not cause > problems with p4 as we remove it programmatically. > > Signed-off-by: Maxime Coste Hi Maxime. This looks really good. Even the Windows section is fine; thanks for paying attention there too. I'm not particularly worried about having a new test for this. Your tweak to the existing 9807 is fine. Unless of course you have one ready to go. Acked-by: Pete Wyckoff You might add my ack and send it directly to Junio + CC the list. It'll be a nice improvement for the next available release. -- Pete -- 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] git-p4: Do not include diff in spec file when just preparing p4
Hello Sorry for the delay, I hope that version is more acceptable. I updated the test case as well, but did not manage to get the actual p4 tests to work here (I have p4 and p4d installed, they start but all the other tests seems to fail). Still the change is straightforward. Cheers, Maxime Coste. -- 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] git-p4: Do not include diff in spec file when just preparing p4
frrr...@gmail.com wrote on Mon, 13 Jan 2014 12:10 +: > Hello, > > On Sun, Jan 12, 2014 at 05:29:46PM -0500, Pete Wyckoff wrote: > > Thanks for the patch, but I'm curious how you'd like this to > > work. I never use the option myself. > > > > As it is, --prepare-p4-only generates a file in /tmp/ that has > > exactly the contents you'd see in the editor during "git p4 > > submit". It includes the diff of the change, presumably to help > > with writing the description. > > Yes, I believe it makes sense to display the diff in this case, as we > can remove it later programmatically. > > > Now you can't actually feed this file directly to "p4 submit" > > without deleting the diff. That's the part you don't like? > > Yes, I do not use that for submitting, but for shelving. I can run > git p4 submit --prepare-p4-only followed by p4 shelve -i < /tmp/... > and perforce will shelve the corresponding change. > > Removing the diff could be done externally, however git-p4 itself > tells the user it can submit using the generated file, which is > not the case if we keep the diff in it. I'm convinced. That explanation makes sense, thanks. It would be nice to do a few more things with this patch. Here's some ideas, sorted in priority order. 1. Put slightly more text into the commit message, possibly from your email above. 2. Refactor out that big chunk of code instead of just moving it. Selectively call it only if not prepare_p4_only. 3. Modify the t9807 test 'submit --prepare-p4-only' to make sure the diff isn't there. 4. Documentation update? Probably not necessary. Let me know if you're interested in doing any of this. -- Pete -- 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] git-p4: Do not include diff in spec file when just preparing p4
Hello, On Sun, Jan 12, 2014 at 05:29:46PM -0500, Pete Wyckoff wrote: > Thanks for the patch, but I'm curious how you'd like this to > work. I never use the option myself. > > As it is, --prepare-p4-only generates a file in /tmp/ that has > exactly the contents you'd see in the editor during "git p4 > submit". It includes the diff of the change, presumably to help > with writing the description. Yes, I believe it makes sense to display the diff in this case, as we can remove it later programmatically. > Now you can't actually feed this file directly to "p4 submit" > without deleting the diff. That's the part you don't like? Yes, I do not use that for submitting, but for shelving. I can run git p4 submit --prepare-p4-only followed by p4 shelve -i < /tmp/... and perforce will shelve the corresponding change. Removing the diff could be done externally, however git-p4 itself tells the user it can submit using the generated file, which is not the case if we keep the diff in it. Cheers, Maxime Coste. -- 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] git-p4: Do not include diff in spec file when just preparing p4
frrr...@gmail.com wrote on Fri, 10 Jan 2014 18:18 +: > The diff information render the spec file unusable as is by p4, > do not include it when run with --prepare-p4-only so that the > given file can be directly passed to p4. Thanks for the patch, but I'm curious how you'd like this to work. I never use the option myself. As it is, --prepare-p4-only generates a file in /tmp/ that has exactly the contents you'd see in the editor during "git p4 submit". It includes the diff of the change, presumably to help with writing the description. Now you can't actually feed this file directly to "p4 submit" without deleting the diff. That's the part you don't like? -- Pete -- 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
