[PATCH 0 of 5] remove

[Adrian Buehlmann]

On 2011-05-23 23:09, Matt Mackall wrote:
> On Mon, 2011-05-23 at 22:50 +0200, Adrian Buehlmann wrote:
>> On 2011-05-23 22:22, Matt Mackall wrote:
>>> On Mon, 2011-05-23 at 22:01 +0200, Adrian Buehlmann wrote:
>>>> On 2011-05-23 21:48, Matt Mackall wrote:
>>>>> On Mon, 2011-05-23 at 21:33 +0200, Adrian Buehlmann wrote:
>>>>>> On 2011-05-23 21:26, Matt Mackall wrote:
>>>>>>> On Mon, 2011-05-23 at 21:07 +0200, Adrian Buehlmann wrote:
>>>>>>>> On 2011-05-23 20:25, Matt Mackall wrote:
>>>>>>>>> On Mon, 2011-05-23 at 16:56 +0200, Adrian Buehlmann wrote:
>>>>>>>>>> see patches
>>>>>>>>>
>>>>>>>>> I'm afraid I can't really see what's motivating this, can you elucidate
>>>>>>>>> please?
>>>>>>>>>
>>>>>>>>
>>>>>>>> If you don't want to look at the patches: No.
>>>>>>>
>>>>>>> I did look, was confused, asked for clarification, and got rude
>>>>>>> response. If you'd rather that I drop your patches than spend a couple
>>>>>>> sentences explaining why we should want them, then I suppose that's your
>>>>>>> choice. Moving on..
>>>>>>>
>>>>>>
>>>>>> All I tried to say is, that you have to look at the individual patches.
>>>>>> I don't think I was rude?
>>>>>
>>>>> Is this a collection of unrelated patches? It appears not, but I can't
>>>>> quite work out what the goal is. There's some stuff here that appears
>>>>> iffy to me, knowing what the larger goal is will help me evaluate it.
>>>>>
>>>>
>>>> Patch 2 depends on patch 1.
>>>>
>>>> Can you look at patch 2 and see if that makes any sense for you? I wrote
>>>> a comment to that.
>>>
>>> Ok, so my sense now is that this is recapitulating the oscillation we've
>>> had with remove/forget at the command layer, except at the mid-layer.
>>>
>>> Ages ago, we had remove and forget commands. Then some eager coder
>>> decided that forget was a special case of remove and folded it into
>>> remove as a flag. And then we had to grow that really confusing behavior
>>> table as the behavior now wasn't obvious to everyone (or anyone,
>>> actually). The behavior of remove is sensible on inspection, but no one
>>> can be expected to arrive at it from first principles.
>>>
>>> And then we decided that this was all way more pain than it was worth
>>> and added the forget command back as the obvious way to undo an add.
>>>
>>> This seems to be the start of a similar undertaking, except internal. We
>>> have remove and forget at the command layer (with remove having some
>>> forget magic), remove and forget at the context layer, and remove and
>>> forget at the dirstate layer. I don't see that dropping forget in the
>>> mid-layer in exchange for a second argument to ctx.remove is a win.
>>>
>>> (Though it is a little suspect that commands.forget is calling
>>> ctx.remove!)
>>
>> I'm aware of (almost) all that.
>>
>> But if you look at how command.forget is implemented, you see that it
>> only calls remove (!)  :-p
> 
> I believe you'll see I addressed that five lines up. Seems the right fix
> is to have commands.forget use ctx.forget which calls dirstate.forget.
> And then see if we can drop the forget logic in ctx.remove().

The point is. That it *only* calls remove.

And I don't think there's anything suspect with that.

The only suspect thing is ctx.forget, which is not needed at all, as
I've demonstrated with my patches.

>> The only place where workingctx.forget is called, is in the
>> implementation of the 'hg remove' command.
>>
>> And there, the two calls on workingctx are doing
>>
>>    acquire wlock
>>    release wlock
>>    acquire wlock
>>    release wlock
>>
>> for a _single_ 'hg remove' invocation.
>>
>> This seems quite messy and inefficient to me.
> 
> (..and this is precisely how you should describe your patches in the
> first place.)

Well. It was so obvious to me.

> I don't think this is the right fix, and here's why: multiple types of
> actions per working directory 'transaction' is a generic problem. We've
> got it in addremove, import, commit, update, etc. We usually deal with
> that by using the fact that locks nest and grabbing the lock at a higher
> level. The locking at the context level is mostly there as a
> convenience.

I think we don't need nesting for this particular case. A single
ctx.remove function will do just fine.

> For now, it's quite silly for both context.remove() and forget() to
> acquire a lock for an empty list. Or, alternately, for commands.remove
> to call them with empty lists.
> 

There are quite a couple of silly things there, indeed. But it's not a
lot of code.

IMHO I still believe we don't need ctx.forget() at all.
ctx.remove(unlink=False) is doing just fine for implementing the forget
command.

And we can use the same ctx.remove() for implementing 'hg remove'.

The only thing that needs to be adjusted is that we need a way to teach
ctx.remove to not unlink the added files. ctx.remove knows by itself
which files are in added state. I think it is pointless to collect a
separate 'forget' list in commands.remove. That's why I nuked it there
with my patch 2.

Anyway. I'll redo patch 4 as you said for now. Then we'll see what can
be done about the rest (but honestly, I still fail to see what's wrong
with my patches 1 & 2).