[PATCH 0 of 5] remove

[Adrian Buehlmann]

On 2011-05-24 23:42, Matt Mackall wrote:
> On Tue, 2011-05-24 at 00:13 +0200, Adrian Buehlmann wrote:
>>> 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.
> 
> Let me try this again:
> 
> (a) We have extensive experience that making forget a special case of
> remove via an extra arg is bad, confusing design at the command line
> level.
> 
> (b) We have remove and forget at three levels (implementation quirks
> aside): commands, context, and dirstate.
> 
> Given (a) and (b), why should I think that merging remove and forget at
> the context level by adding an extra arg is forward progress
> design-wise?
> 
> It seems to me that forward progress would be untangling the remove and
> forget paths in the lower levels further and cleaning up the legacy at
> the top level.

I just think it is very strange that the _only_ place where ctx.forget
is called _at all_ is in commands.remove. ctx.forget is not called
anywhere else. Not even commands.forget needs it (commands.forget uses
ctx.remove, as we have noted already a couple of times -- and which I
think is fine).

And I think the only reason for why ctx.forget was used in
commands.remove _at all_, is because commands.remove can't include the
added files in the 'list' param for the

   ctx.remove(list, unlink=not after)

call, because otherwise those added files would then be deleted if
'after' is True.

Added files are never ever deleted by commands.remove.

What's also interesting, ctx.remove already *does* handle added files
special. It calls dirstate.forget for them, whereas it calls
dirstate.remove for all others (which is stange too, since it seems to
me the dirstate should already know the added files, but well...).

It's all pretty weird. But at least the current (strange) implementation
has the nice property that the commands do what we expect from them.
Just reading that code isn't very funny.

> I'm not convinced by the locking argument. We need to use nested locks
> all over the place - basically everywhere we're doing more than one of
> add, remove, forget, copy, etc. in the same action. The methods on
> context are only taking locks internally as a convenience. We could
> instead change them to assert that locks are held and force the callers
> to actually think about locking/transactions/etc which would probably
> result in more correct code overall. But merging functions because they
> want to share a lock will just end up with us merging all the functions.

Ok. I think the shortest path out (of these discussions) which also
improves something would probably be to wrap those two calls in
commands.remove into a wlock acquire block.

Then we would at least have a program that doesn't acquire and release
the wlock twice for a single 'hg remove' call.

I'll probably send a patch for that tomorrow and then leave it at that
(not because I "give up quickly", but because I simply fail to see a way
out here).