[PATCH] histedit: add support for exec/x command to histedit (issue4036)

[Piotr Listkiewicz]

@raf at durin42.com

>Behavior is undefined if it updates to some completely unrelated portion
of history.
>Can it abort instead? Undefined behavior in my history editing tools
feels...risky.

My fault, commit msg was untrue. It is not true that this behaviour is
undefined,
user can't update while executing command, if he tries he will run into
error
"abort: histedit in progress" and return from update will be non zero.
Histedit will
stop and wait for user intervention.

> typo: amend has only one m, not two.

Fixed it

> Related: there are some bugs around doing 'hg amend' in the middle of
> a stream. Might need to fix those or otherwise defend against that
> bug: http://bz.selenic.com/show_bug.cgi?id=4800

I will try to fix this, but current issue in my view is unrelated because
as long as
you don't encounter that errors it seems to work. Fixing those error will
fix
error in exec with ammend, so probably this is not the place to work on it.
Let me know what you think

> Might be worth breaking this next line into another section, eg:

Done,added "Between changes...."

> This construction *might* be bad for internationalization, I'm not
> sure. Either way, you need to wrap the string here in _().

Wrapped in function invocation in _()

> Unknown files shouldn't be considered dirty? Look later in
> histedit.py for the call to continuedirty().
(*)
To preserve current behaviour not. It can be checked that in current
implementation
doing 'edit' on commit , adding file ,doing 'histedit --continue' doesnt
find
current working directory dirty. In previous patch i checked for it with
repo.status(unknown=True) but to preserve current general behaviour of
histedit
i removed it. Changing it probabably would more fit in new issue about
histedit
not checking for unknown file in current working dir.

> Might be worth teasing that out into a _dirtywc(repo) method to prevent
future inconsistencies.

Method extracted, change bootstrapcontinue method to use it

@durham at fb.com

> If you name it 'execute' you can drop the awkward trailing _

You are totally right, i renamed it to execute

> self.node is meant to represent the commit that this action is defined
with in the editor list.
> In this case you're using it to store the parentctxnode, which is not the
same thing.
> In fact, since the you have access to the state in the run function
below,
> there's no need to store a self.node at all here.  Just pass None to the
subclass
> constructor or don't call it all.

Right, new implementation of execute doesnt use it

> Since we already store the state on the histedit class, there's no need
to store the repo
> separately.

Deleted it, now always get repo by self.state.repo

> As mentioned above, I don't think we want to pass the state.parentctxnode
here.

Removed it in new patch

> 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).

Introduced _commit method for this

> This will execute the command relative to the current working directory
right?  I think we
> want to make it execute always relative to the repo root
(cwd=state.repo.root)
> (git does it this way, for > what it's worth).

Right, added cwd=repo.root parameter to ui.system invocation

> Also, I believe ui.system() can throw an OSError.  So we probably want to
catch it and
> replace it with a InterventionRequired error.

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:
- os.system
- subprocess.call, subprocess.Popen
- onerr(errmsg) but in our case onerr is None

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.
I would be thankful for explanation on this.

> The relock should be in a finally statement so that we always leave this
function in a lock
> state that matches the one we came in with.

Right, it is now in finally

> We also call repo.invalidate() and repo.invalidatedirstate() in our
version of exec.  Not sure if > it's strictly necessary though.

It is probably not necessary because in new patch unknown files is not
checked, i
written about it in (*). From tests it seems to work without this.

> 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
> mark the old state.parentctxnode as becoming the new self.repo['.']
context (like you are
> doing now, but without the self.node middleman).

Right, new patch is not using self.node, see self.continueclean()

> Also, if the current node matches the old node, I think you want to
return []

It will never match because it performs update/commit at the beggining as
describe in commit
msg. Even if user in exec does not change repository ,the last node was
changed. "Copying"
last node is done to allow user change last node (ammending) in exec.

> I'd mention what the error code actually was.

Changed message to 'Return code of "%s" is %d (expected zero)'

> I think you need to implement continuedirty() to always throw an
exception.  If the exec
> fails, it will require intervention, but then when you run 'hg histedit
--continue' the default
> continuedirty will attempt to make a commit, which is probably not the
desired behavior.
> This will mean you can't use the continuedirty() function inside run(),
but you probably
> shouldn't use it there anyway, since all you're really trying to do is
make a commit
> (and thus just abusing the base continuedirty implementation).

True, changed according to explanation


2015-09-11 0:25 GMT+02:00 timeless <timeless at gmail.com>:

> > +                    self.getinterventionmsg(
> > +                        "Working copy dirty after exec %s" % self.cmd))
>
> This produces a string with 'self.cmd' expanded (I believe cmd comes
> from the user) and sends it here:
>
> > +    def getinterventionmsg(self, errormsg):
> > +        return _(errormsg + "\n" +
> > +              'Make changes as needed, you may commit or record as
> needed '
> > +              'now.\nWhen you are finished, run hg histedit --continue
> to '
> > +              'resume.')
>
> Here, errormsg is folded into a message which is then sent for translation.
>
> The a not horribly incorrect approach would be:
>
> self.getinterventionmsg(_("Working copy dirty after exec %s") % self.cmd)
>
>  return _("%\n" +
>               'Make changes as needed, you may commit or record as needed '
>               'now.\nWhen you are finished, run hg histedit --continue to '
>               'resume.' % errormsg)
>
> Augie wrote:
> > This construction *might* be bad for internationalization,
>
> it is
>
> > I'm not sure. Either way, you need to wrap the string here in _().
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://selenic.com/pipermail/mercurial-devel/attachments/20150913/97e17cf2/attachment.html>