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

Tue Sep 29 13:36:21 CDT 2015

I am still interested in doing this task, please reply or let me know if
you need more time

2015-09-22 11:42 GMT+02:00 Piotr Listkiewicz <piotr.listkiewicz at gmail.com>:

> 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/20150929/d50162a8/attachment.html>