[PATCH] histedit: add support for exec/x command to histedit (issue4036)
Piotr Listkiewicz
piotr.listkiewicz at gmail.com
Sat Oct 24 16:13:23 UTC 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.
>
>
Ok, reentrance is done in version V2
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.
We should talk about this, i still have no idea how can i check this or
what to do with it
2015-09-29 19:40 GMT+01:00 Pierre-Yves David <pierre-yves.david at ens-lyon.org
>:
>
>
> On 09/29/2015 11:36 AM, Piotr Listkiewicz wrote:
>
>> I am still interested in doing this task, please reply or let me know if
>> you need more time
>>
>
> Thanks for pinging me. I'll trying to get to it soonâ„¢, ping me again if I
> fails to do so :-/
>
>
>> 2015-09-22 11:42 GMT+02:00 Piotr Listkiewicz
>> <piotr.listkiewicz at gmail.com <mailto: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
>> <mailto: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).
>>
>>
>>
>>
> --
> Pierre-Yves David
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://selenic.com/pipermail/mercurial-devel/attachments/20151024/b9ab879e/attachment.html>
More information about the Mercurial-devel
mailing list