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

Tue Sep 22 04:42:39 CDT 2015

>
> 1) Check how the re-entrace is handled
> 2) Unlock the right amount of time
> 3) relock that amount of time
> We probably want a context manager to handle that at the lock level.
> Feel free to ask for more details if you need more guidance.

I really need guidance in this regard, i will be helpful for any
information about it,
because i even don't know how can i find case like this in source code.

My point here is that if some command changed stuff (new commit, bookmark
> location etc), some of it might not be detected by the current process that
> already parsed them from disk and naively assume they did not changed since
> it hold the lock.
> I would like to see some investigation to know if we need to do something
> special (or if it is already handled, for example by the lock). to make
> sure we are safe here.

I need help with this too, my current knowledge of code base is too little
to even know how can i investigate it. I can add additional test cases
which works, but this will not assure that it wouldn't crash on some
specific others.

I'm not familliar with this unknown file issue, do we something to fix
> there? Is there a bug tracking/explaining it?

I didn't find any bug related to it so i have no idea if this is a bug or a
feature. It can be tracked like this:

$ hg init edit_bug
$ cd edit_bug
$ touch a
$ hg add a
$ hg commit -m "a"
$ touch b
$ hg add b
$ hg commit -m "b"
$ touch c
$ hg add c
$ hg commit -m "c"
$ hg histedit 1 --command - 2>&1 << EOF
> edit 1
> pick 2
> EOF
// after command it stopps at commit 1(commit "b")
$ touch d
$ hg histedit --continue
// shows editor to change commit message, just write it to preserve old
// histedit commands ends here because we picked commit 2
$ hg status
? d

Histedit continued with untracked file d doesn't throwing any exception,
doesnt telling user about it. This behaviour is all about how
histedit.bootstrapcontinue works, in this patch i extracted current way of
checking if current working context is clean in _isdirtywc method. This
method to check this invokes repo.status() which by default doesn't check
untracked files. Changing invocation to repo.status(unknown=True) should
fix this behaviour, but i have no idea if this behaviour is bug.

edit was initially intended to just edit the changeset and possibly split
> it in multiple parts. It has known issue with anything else. CF:
> http://bz.selenic.com/show_bug.cgi?id=4800
> As exec will likely be heavily used for history edition we will hit this
> issues. not to talk about people dropping to a shell using exec
> Moreover edit have special logic looking for replacement on --continue
> that track replacement. I do not see such logic in the exec code.

My intention to prevent this described Durham Goode:

Regarding your initial points 1 and 2, he addresses this in the run()
> function by recreating the commit before running the shell command.  So the
> hash will always be different (though I think the _commit function may need
> to put some extra data in the extras field to ensure that the hash changes).

2015-09-16 7:27 GMT+02:00 Durham Goode <durham at fb.com>:

>
>
> On 9/15/15 1:54 PM, Pierre-Yves David wrote:
>
>>
>>
>> On 09/14/2015 01:04 PM, Piotr Listkiewicz wrote:
>>
>>>     This seems to be a resend, we usually add V2 (then V3, etc) to the
>>>     subject (using --flag V2 in patchbomb).
>>>
>>>
>>> My fault, Im new here and i probably omitted this information in
>>> contribution wiki
>>>
>>
>> Not a big deal, but please do for the next ones, it's a bit confusing on
>> the reviewer end otherwise.
>>
>>     Once we ensure we really release the lock (even when nested) we'll
>>>     have to ensure that we reacquires them with the same level of
>>> nesting.
>>>     (we probably want a context manager for this unlocking business)
>>>
>>>
>>> Honestly i don't know how can i release it properly and how to ensure
>>> that it reacquires them with the same level of nesting. I would be
>>> greateful for instruction/more information about it.
>>>
>>
>> 1) Check how the re-entrace is handled
>> 2) Unlock the right amount of time
>> 3) relock that amount of time
>>
>> We probably want a context manager to handle that at the lock level.
>>
>> Feel free to ask for more details if you need more guidance.
>>
>>     Also, if the commands changed the repository, do we need to forcibly
>>>     invalidate some cache to ensure a consistent view of the repository?
>>>     Or is this already taken care of by something else.
>>>
>>>  From tests it seems like it properly detects changes in repository
>>> other than unknown files.
>>>
>>
>> My point here is that if some command changed stuff (new commit, bookmark
>> location etc), some of it might not be detected by the current process that
>> already parsed them from disk and naively assume they did not changed since
>> it hold the lock.
>>
>> I would like to see some investigation to know if we need to do something
>> special (or if it is already handled, for example by the lock). to make
>> sure we are safe here.
>>
>>
>> But this behaviour is consistent with edit
>>> commands which doesn't detect unknown files also.
>>>
>>
>> The edit command terminates the current process and starts a new one, so
>> it is far more safe in this regards.
>>
>> In my view if we agree
>>> that unknown files should be detected by the exec command we should
>>> detect it in edit command as well.
>>>
>>
>> I'm not familliar with this unknown file issue, do we something to fix
>> there? Is there a bug tracking/explaining it?
>>
>>     This is going to break/misbehaver in in multiple cases.
>>>     1) if nothing changed, you code claim that changeset X replace
>>>     changesets X. If the current code does not explode with this yet,
>>>     this will probably bite use in the future
>>>     2) if anything else but an amend happened (fold, splitting, adding
>>>     more changesets) this is going to report wrong result and lead to
>>>     crash when moving bookmarks or creating obsolescence markers.
>>>
>>>
>>> It is most likely that i don't get it, but i have no idea why this would
>>> lead to such behaviour more than edit command. Edit command behaves same
>>> way if user amend to given commit, replacing it with new ammend version.
>>>
>>
>> edit was initially intended to just edit the changeset and possibly split
>> it in multiple parts. It has known issue with anything else. CF:
>> http://bz.selenic.com/show_bug.cgi?id=4800
>>
>> As exec will likely be heavily used for history edition we will hit this
>> issues. not to talk about people dropping to a shell using exec
>>
>> Moreover edit have special logic looking for replacement on --continue
>> that track replacement. I do not see such logic in the exec code.
>>
>>
>> To conclude, thanks again for trackling this exec commands, I'm trying to
>> point out at the know trap early on, because the topic is quite complex.
>>
> Regarding your initial points 1 and 2, he addresses this in the run()
> function by recreating the commit before running the shell command.  So the
> hash will always be different (though I think the _commit function may need
> to put some extra data in the extras field to ensure that the hash changes).
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://selenic.com/pipermail/mercurial-devel/attachments/20150922/56e15492/attachment.html>