<div dir="ltr"><div>@<a href="mailto:raf@durin42.com">raf@durin42.com</a></div><div><br></div><div>>Behavior is undefined if it updates to some completely unrelated portion of history.</div><div>>Can it abort instead? Undefined behavior in my history editing tools feels...risky.</div><div><br></div><div>My fault, commit msg was untrue. It is not true that this behaviour is undefined,</div><div>user can't update while executing command, if he tries he will run into error</div><div>"abort: histedit in progress" and return from update will be non zero. Histedit will</div><div>stop and wait for user intervention.</div><div><br></div><div>> typo: amend has only one m, not two.</div><div><br></div><div>Fixed it</div><div><br></div><div>> Related: there are some bugs around doing 'hg amend' in the middle of</div><div>> a stream. Might need to fix those or otherwise defend against that</div><div>> bug: <a href="http://bz.selenic.com/show_bug.cgi?id=4800">http://bz.selenic.com/show_bug.cgi?id=4800</a></div><div><br></div><div>I will try to fix this, but current issue in my view is unrelated because as long as</div><div>you don't encounter that errors it seems to work. Fixing those error will fix </div><div>error in exec with ammend, so probably this is not the place to work on it.</div><div>Let me know what you think</div><div><br></div><div>> Might be worth breaking this next line into another section, eg:</div><div><br></div><div>Done,added "Between changes...."</div><div><br></div><div>> This construction *might* be bad for internationalization, I'm not</div><div>> sure. Either way, you need to wrap the string here in _().</div><div><br></div><div>Wrapped in function invocation in _()</div><div><br></div><div>> Unknown files shouldn't be considered dirty? Look later in</div><div>> histedit.py for the call to continuedirty().</div><div>(*)</div><div>To preserve current behaviour not. It can be checked that in current implementation</div><div>doing 'edit' on commit , adding file ,doing 'histedit --continue' doesnt find </div><div>current working directory dirty. In previous patch i checked for it with</div><div>repo.status(unknown=True) but to preserve current general behaviour of histedit </div><div>i removed it. Changing it probabably would more fit in new issue about histedit</div><div>not checking for unknown file in current working dir.</div><div><br></div><div>> Might be worth teasing that out into a _dirtywc(repo) method to prevent future inconsistencies.</div><div><br></div><div>Method extracted, change bootstrapcontinue method to use it</div><div><br></div><div>@<a href="mailto:durham@fb.com">durham@fb.com</a></div><div><br></div><div>> If you name it 'execute' you can drop the awkward trailing _</div><div><br></div><div>You are totally right, i renamed it to execute</div><div><br></div><div>> self.node is meant to represent the commit that this action is defined with in the editor list. </div><div>> In this case you're using it to store the parentctxnode, which is not the same thing. </div><div>> In fact, since the you have access to the state in the run function below, </div><div>> there's no need to store a self.node at all here. Just pass None to the subclass</div><div>> constructor or don't call it all.</div><div><br></div><div>Right, new implementation of execute doesnt use it</div><div><br></div><div>> Since we already store the state on the histedit class, there's no need to store the repo </div><div>> separately.</div><div><br></div><div>Deleted it, now always get repo by self.state.repo</div><div><br></div><div>> As mentioned above, I don't think we want to pass the state.parentctxnode here.</div><div><br></div><div>Removed it in new patch</div><div><br></div><div>> You're using continuedirty as a commit function, when it's meant to be step 2 of the action. I'd > do the commit manually (so you can override continuedirty below).</div><div><br></div><div>Introduced _commit method for this</div><div><br></div><div>> This will execute the command relative to the current working directory right? I think we </div><div>> want to make it execute always relative to the repo root (cwd=state.repo.root)</div><div>> (git does it this way, for > what it's worth).</div><div><br></div><div>Right, added cwd=repo.root parameter to ui.system invocation</div><div><br></div><div>> Also, I believe ui.system() can throw an OSError. So we probably want to catch it and </div><div>> replace it with a InterventionRequired error.</div><div><br></div><div>In new patch it is catching OSError , but honestly i cant find where it can raise it. Method ui.system could throw exception only inside util.system, util.system could throw by methods:</div><div>- os.system </div><div>- subprocess.call, subprocess.Popen</div><div>- onerr(errmsg) but in our case onerr is None</div><div><br></div><div>subprocess methods could raise subprocess.CalledProcessError(which is not extending OSError). Only exception i found that os.system could raise is TypeError when it is invoked with None as cmd.</div><div>I would be thankful for explanation on this.</div><div><br></div><div>> The relock should be in a finally statement so that we always leave this function in a lock </div><div>> state that matches the one we came in with.</div><div><br></div><div>Right, it is now in finally</div><div><br></div><div>> We also call repo.invalidate() and repo.invalidatedirstate() in our version of exec. Not sure if > it's strictly necessary though.</div><div><br></div><div>It is probably not necessary because in new patch unknown files is not checked, i </div><div>written about it in (*). From tests it seems to work without this.</div><div><br></div><div>> The normal pattern here is to "return self.continueclean()". That is what will be called if you > do 'hg histedit --continue', so it's the code path for finishing a histedit action. In exec's case > you'll need to change it to not depend on self.node since exec has no such notion. Instead </div><div>> mark the old state.parentctxnode as becoming the new self.repo['.'] context (like you are </div><div>> doing now, but without the self.node middleman).</div><div><br></div><div>Right, new patch is not using self.node, see self.continueclean()</div><div><br></div><div>> Also, if the current node matches the old node, I think you want to return []</div><div><br></div><div>It will never match because it performs update/commit at the beggining as describe in commit </div><div>msg. Even if user in exec does not change repository ,the last node was changed. "Copying" </div><div>last node is done to allow user change last node (ammending) in exec.</div><div><br></div><div>> I'd mention what the error code actually was.</div><div><br></div><div>Changed message to 'Return code of "%s" is %d (expected zero)'</div><div><br></div><div>> I think you need to implement continuedirty() to always throw an exception. If the exec </div><div>> fails, it will require intervention, but then when you run 'hg histedit --continue' the default </div><div>> continuedirty will attempt to make a commit, which is probably not the desired behavior.</div><div>> This will mean you can't use the continuedirty() function inside run(), but you probably</div><div>> shouldn't use it there anyway, since all you're really trying to do is make a commit </div><div>> (and thus just abusing the base continuedirty implementation).</div><div><br></div><div>True, changed according to explanation</div><div><br></div></div><div class="gmail_extra"><br><div class="gmail_quote">2015-09-11 0:25 GMT+02:00 timeless <span dir="ltr"><<a href="mailto:timeless@gmail.com" target="_blank">timeless@gmail.com</a>></span>:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class="">> + self.getinterventionmsg(<br>
> + "Working copy dirty after exec %s" % self.cmd))<br>
<br>
</span>This produces a string with 'self.cmd' expanded (I believe cmd comes<br>
from the user) and sends it here:<br>
<span class=""><br>
> + def getinterventionmsg(self, errormsg):<br>
> + return _(errormsg + "\n" +<br>
> + 'Make changes as needed, you may commit or record as needed '<br>
> + 'now.\nWhen you are finished, run hg histedit --continue to '<br>
> + 'resume.')<br>
<br>
</span>Here, errormsg is folded into a message which is then sent for translation.<br>
<br>
The a not horribly incorrect approach would be:<br>
<br>
self.getinterventionmsg(_("Working copy dirty after exec %s") % self.cmd)<br>
<br>
return _("%\n" +<br>
<span class=""> 'Make changes as needed, you may commit or record as needed '<br>
</span><span class=""> 'now.\nWhen you are finished, run hg histedit --continue to '<br>
</span> 'resume.' % errormsg)<br>
<span class=""><br>
Augie wrote:<br>
> This construction *might* be bad for internationalization,<br>
<br>
</span>it is<br>
<div class="HOEnZb"><div class="h5"><br>
> I'm not sure. Either way, you need to wrap the string here in _().<br>
</div></div></blockquote></div><br></div>