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

[Durham Goode]

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