{"thread":{"id":"40665","subject":"[PATCH] git-p4: Handle p4 submit failure","startedAt":"2015-10-28T13:12:21Z","lastAt":"2015-10-30T20:23:42Z","messageCount":6,"participants":["Etienne Girard","Junio C Hamano","Luke Diamand"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"272404","messageId":"CAJA=mv4Tr_DoBMwR8hK_fEJ1PFCYTu17HHvEnFWMANGFcf0Wpg@mail.gmail.com","threadId":"40665","inReplyTo":null,"subject":"[PATCH] git-p4: Handle p4 submit failure","fromName":"Etienne Girard","fromEmail":"etienne.g.girard@gmail.com","sentAt":"2015-10-28T13:12:21Z","receivedAt":"2015-10-28T13:12:21Z","isPatch":true,"sender":{"key":"etienne.g.girard@gmail.com","avatar":null},"body":"Clean the workspace if p4_write_pipe raised SystemExit,\nso that the user don't have to do it themselves.\n\nSigned-off-by: GIRARD Etienne \n---\n git-p4.py | 71 +++++++++++++++++++++++++++++++++------------------------------\n 1 file changed, 37 insertions(+), 34 deletions(-)\n\nThe p4 submit command may fail, for example if the changelist contains\na job that does not exist in the Jobs section. If this is the case,\np4_write_pipe will exit the script. This change makes it so that the\nworkspace is reverted before letting the script die.\n\ndiff --git a/git-p4.py b/git-p4.py\nindex 0093fa3..d535904 100755\n--- a/git-p4.py\n+++ b/git-p4.py\n@@ -1538,44 +1538,47 @@ class P4Submit(Command, P4UserMap):\n #\n # Let the user edit the change description, then submit it.\n #\n- if self.edit_template(fileName):\n- # read the edited message and submit\n- ret = True\n- tmpFile = open(fileName, \"rb\")\n- message = tmpFile.read()\n- tmpFile.close()\n- if self.isWindows:\n- message = message.replace(\"\\r\\n\", \"\\n\")\n- submitTemplate = message[:message.index(separatorLine)]\n- p4_write_pipe(['submit', '-i'], submitTemplate)\n-\n- if self.preserveUser:\n- if p4User:\n- # Get last changelist number. Cannot easily get it from\n- # the submit command output as the output is\n- # unmarshalled.\n- changelist = self.lastP4Changelist()\n- self.modifyChangelistUser(changelist, p4User)\n-\n- # The rename/copy happened by applying a patch that created a\n- # new file. This leaves it writable, which confuses p4.\n- for f in pureRenameCopy:\n- p4_sync(f, \"-f\")\n+ submitted = False\n\n- else:\n+ try:\n+ if self.edit_template(fileName):\n+ # read the edited message and submit\n+ tmpFile = open(fileName, \"rb\")\n+ message = tmpFile.read()\n+ tmpFile.close()\n+ if self.isWindows:\n+ message = message.replace(\"\\r\\n\", \"\\n\")\n+ submitTemplate = message[:message.index(separatorLine)]\n+ p4_write_pipe(['submit', '-i'], submitTemplate)\n+\n+ if self.preserveUser:\n+ if p4User:\n+ # Get last changelist number. Cannot easily get it from\n+ # the submit command output as the output is\n+ # unmarshalled.\n+ changelist = self.lastP4Changelist()\n+ self.modifyChangelistUser(changelist, p4User)\n+\n+ # The rename/copy happened by applying a patch that created a\n+ # new file. This leaves it writable, which confuses p4.\n+ for f in pureRenameCopy:\n+ p4_sync(f, \"-f\")\n+ submitted = True\n+\n+ finally:\n # skip this patch\n- ret = False\n- print \"Submission cancelled, undoing p4 changes.\"\n- for f in editedFiles:\n- p4_revert(f)\n- for f in filesToAdd:\n- p4_revert(f)\n- os.remove(f)\n- for f in filesToDelete:\n- p4_revert(f)\n+ if not submitted:\n+ print \"Submission cancelled, undoing p4 changes.\"\n+ for f in editedFiles:\n+ p4_revert(f)\n+ for f in filesToAdd:\n+ p4_revert(f)\n+ os.remove(f)\n+ for f in filesToDelete:\n+ p4_revert(f)\n\n os.remove(fileName)\n- return ret\n+ return submitted\n\n # Export git tags as p4 labels. Create a p4 label and then tag\n # with that.\n-- \n1.9.1\n"},{"id":"272531","messageId":"xmqqeggcmhle.fsf@gitster.mtv.corp.google.com","threadId":"40665","inReplyTo":"CAJA=mv4Tr_DoBMwR8hK_fEJ1PFCYTu17HHvEnFWMANGFcf0Wpg@mail.gmail.com","subject":"Re: [PATCH] git-p4: Handle p4 submit failure","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-10-30T16:44:29Z","receivedAt":"2015-10-30T16:44:29Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Etienne Girard writes:\n\n> Clean the workspace if p4_write_pipe raised SystemExit,\n> so that the user don't have to do it themselves.\n>\n> Signed-off-by: GIRARD Etienne \n> ---\n\nThanks. As I do not really do \"p4\", I am CC'ing this response to\nthe area experts.\n\nFirst, one \"procedural\" comment.\n\nYou sent the patch from your gmail.com address, but signed it off\nwith another address (with name spelled differently). If applied\nas-is, the result will have your gmail.com address listed as the\nauthor and that will be the name you would see in \"git shortlog\".\n\nIf you rather prefer to be known as the name and address you placed\non your sign-off, while sending the patch from another address, you\ncan do so by staring the _body_ of the message with what is called\nan \"in-body header\", like this:\n\n From: GIRARD Etienne \n\n Clean the workspace if p4_write_pipe raised SystemExit,\n so that the user don't have to do it themselves.\n\n Signed-off-by: GIRARD Etienne \n ---\n\t ... here you have the usual patch ...\n\nThat \"From:\" line would sit on the first line of the body of your\nmessage, separated by a blank line from the body of the log message.\n\nThis is not even a problem from the project's point of view, but I\nthought you would want to be aware of it, and may want a chance to\nchange it.\n\n\tNote: this time, you do not need to resend the patch; just\n\tplease let me know if you want me to do the equivalent of\n\tthe above while applying to make your murex address and name\n\tappear as the author in \"git log\" and \"git shortlog\" output.\n\nFYI, you could override the \"Subject:\" of your e-mail header with an\nin-body header (this is often useful when replying to an existing\ndiscussion thread with a patch that shows a solution), e.g.\n\n From: GIRARD Etienne \n\tSubject: git-p4: clean up after p4 submit failure\n\n Clean the workspace if p4_write_pipe raised SystemExit,\n so that the user don't have to do it themselves.\n\n Signed-off-by: GIRARD Etienne \n ---\n\t ... here you have the usual patch ...\n\n> git-p4.py | 71 +++++++++++++++++++++++++++++++++------------------------------\n> 1 file changed, 37 insertions(+), 34 deletions(-)\n>\n> The p4 submit command may fail, for example if the changelist contains\n> a job that does not exist in the Jobs section. If this is the case,\n> p4_write_pipe will exit the script. This change makes it so that the\n> workspace is reverted before letting the script die.\n\nSome of the information contained in this paragraph deserves to be\nin the log message proper. How about\n\n From: GIRARD Etienne \n\tSubject: git-p4: clean up after p4 submit failure\n\n\tWhen \"p4 submit\" command fails in P4Submit.applyCommit, the\n\tworkspace is left with the changes. We already have a code\n to revert the changes to the workspace when the user decides\n\tto cancel submission by aborting the editor that edits the\n\tchange description, and we should treat the \"p4 submit\"\n\tfailure the same way.\n\n Clean the workspace if p4_write_pipe raised SystemExit,\n so that the user don't have to do it themselves.\n\n Signed-off-by: GIRARD Etienne \n\nor something like that?\n\nWhile trying to come up with the above description, I started\nwondering if the error message wants to differentiate the two cases.\n\nWhen self.edit_template() returns false, we know that the user\nactively said \"no I do not want to submit\", and \"Submission\ncancelled\" is perfectly fine, but when \"p4 submit\" fails because it\ndid not like the change description or if it found some other\nissues, it is not necessarily that the user said \"I want to cancel\";\nit is more like \"Submission failed\".\n\nHmm?\n"},{"id":"272537","messageId":"CAJA=mv7ydNCm-yy9Ukk2XB-xvAJ1VkyqHEGcCTA2PEg=5y9cFQ@mail.gmail.com","threadId":"40665","inReplyTo":"xmqqeggcmhle.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] git-p4: Handle p4 submit failure","fromName":"Etienne Girard","fromEmail":"etienne.g.girard@gmail.com","sentAt":"2015-10-30T17:33:07Z","receivedAt":"2015-10-30T17:33:07Z","isPatch":true,"sender":{"key":"etienne.g.girard@gmail.com","avatar":null},"body":"Hello,\n\n> Note: this time, you do not need to resend the patch; just\n> please let me know if you want me to do the equivalent of\n> the above while applying to make your murex address and name\n> appear as the author in \"git log\" and \"git shortlog\" output.\n\nI'd like my murex address to appear on the log please, if it is not\ntoo much trouble. Thank you for all these tips on the submitting\nprocess.\n\n>> The p4 submit command may fail, for example if the changelist contains\n>> a job that does not exist in the Jobs section. If this is the case,\n>> p4_write_pipe will exit the script. This change makes it so that the\n>> workspace is reverted before letting the script die.\n>\n> Some of the information contained in this paragraph deserves to be\n> in the log message proper. How about\n>\n> From: GIRARD Etienne \n> Subject: git-p4: clean up after p4 submit failure\n>\n> When \"p4 submit\" command fails in P4Submit.applyCommit, the\n> workspace is left with the changes. We already have a code\n> to revert the changes to the workspace when the user decides\n> to cancel submission by aborting the editor that edits the\n> change description, and we should treat the \"p4 submit\"\n> failure the same way.\n>\n> Clean the workspace if p4_write_pipe raised SystemExit,\n> so that the user don't have to do it themselves.\n>\n> Signed-off-by: GIRARD Etienne \n>\n> or something like that?\n\nIt seems like a good description. Please let me know if I should\nsubmit another patch with the proper log message\n\n>\n> While trying to come up with the above description, I started\n> wondering if the error message wants to differentiate the two cases.\n>\n> When self.edit_template() returns false, we know that the user\n> actively said \"no I do not want to submit\", and \"Submission\n> cancelled\" is perfectly fine, but when \"p4 submit\" fails because it\n> did not like the change description or if it found some other\n> issues, it is not necessarily that the user said \"I want to cancel\";\n> it is more like \"Submission failed\".\n\nYes, however if `p4 submit` fails the corresponding \"Command failed\"\nerror message is displayed, and the p4 error message itself is\ndisplayed if any.\nTthe script will also terminate successfully if self.edit_template\nreturns false but it will exit with error code 1 if p4 submit fails.\n\nSo the user will get \"Command failed: [...]\" followed by \"Submission\ncancelled, undoing p4 changes\", to let him know that the script failed\nbecause of p4 and that nothing was submitted.\n"},{"id":"272539","messageId":"xmqqy4ekkzmg.fsf@gitster.mtv.corp.google.com","threadId":"40665","inReplyTo":"CAJA=mv7ydNCm-yy9Ukk2XB-xvAJ1VkyqHEGcCTA2PEg=5y9cFQ@mail.gmail.com","subject":"Re: [PATCH] git-p4: Handle p4 submit failure","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-10-30T17:57:59Z","receivedAt":"2015-10-30T17:57:59Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Etienne Girard writes:\n\n> Yes, however if `p4 submit` fails the corresponding \"Command failed\"\n> error message is displayed, and the p4 error message itself is\n> displayed if any.\n> Tthe script will also terminate successfully if self.edit_template\n> returns false but it will exit with error code 1 if p4 submit fails.\n>\n> So the user will get \"Command failed: [...]\" followed by \"Submission\n> cancelled, undoing p4 changes\", to let him know that the script failed\n> because of p4 and that nothing was submitted.\n\nOK, then it sounds like all I have to do is to update the log\nmessage with the \"How about this\" version and correct the authorship\nto use your murex address, and then wait for reviews from real \"git\np4\" reviewers.\n\nThanks.\n"},{"id":"272573","messageId":"5633CABA.1000003@diamand.org","threadId":"40665","inReplyTo":"xmqqy4ekkzmg.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] git-p4: Handle p4 submit failure","fromName":"Luke Diamand","fromEmail":"luke@diamand.org","sentAt":"2015-10-30T19:53:30Z","receivedAt":"2015-10-30T19:53:30Z","isPatch":true,"sender":{"key":"luke@diamand.org","avatar":"https://avatars.githubusercontent.com/u/5330967?v=4"},"body":"On 30/10/15 17:57, Junio C Hamano wrote:\n> Etienne Girard writes:\n>\n>> Yes, however if `p4 submit` fails the corresponding \"Command failed\"\n>> error message is displayed, and the p4 error message itself is\n>> displayed if any.\n>> Tthe script will also terminate successfully if self.edit_template\n>> returns false but it will exit with error code 1 if p4 submit fails.\n>>\n>> So the user will get \"Command failed: [...]\" followed by \"Submission\n>> cancelled, undoing p4 changes\", to let him know that the script failed\n>> because of p4 and that nothing was submitted.\n>\n> OK, then it sounds like all I have to do is to update the log\n> message with the \"How about this\" version and correct the authorship\n> to use your murex address, and then wait for reviews from real \"git\n> p4\" reviewers.\n>\n\nLooks good to me. Nice use of try...finally.\n\nOne very small thing - test t9807-git-p4-submit.sh is now failing with \nthis change.\n\nThat's because it tries to delete one of the files it created, but you \nare now deleting it already! Could you just update that please?\n\nThanks!\nLuke\n"},{"id":"272577","messageId":"xmqqziz0jeb5.fsf@gitster.mtv.corp.google.com","threadId":"40665","inReplyTo":"5633CABA.1000003@diamand.org","subject":"Re: [PATCH] git-p4: Handle p4 submit failure","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-10-30T20:23:42Z","receivedAt":"2015-10-30T20:23:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Luke Diamand writes:\n\n> On 30/10/15 17:57, Junio C Hamano wrote:\n>> Etienne Girard writes:\n>>\n>>> Yes, however if `p4 submit` fails the corresponding \"Command failed\"\n>>> error message is displayed, and the p4 error message itself is\n>>> displayed if any.\n>>> Tthe script will also terminate successfully if self.edit_template\n>>> returns false but it will exit with error code 1 if p4 submit fails.\n>>>\n>>> So the user will get \"Command failed: [...]\" followed by \"Submission\n>>> cancelled, undoing p4 changes\", to let him know that the script failed\n>>> because of p4 and that nothing was submitted.\n>>\n>> OK, then it sounds like all I have to do is to update the log\n>> message with the \"How about this\" version and correct the authorship\n>> to use your murex address, and then wait for reviews from real \"git\n>> p4\" reviewers.\n>>\n>\n> Looks good to me. Nice use of try...finally.\n>\n> One very small thing - test t9807-git-p4-submit.sh is now failing with\n> this change.\n>\n> That's because it tries to delete one of the files it created, but you\n> are now deleting it already! Could you just update that please?\n\nI'll ask Etienne include that change in a reroll, as my environment\nis not equipped to run any p4 tests.\n\nThanks.\n"}]}