[PATCH 0 of 5] remove

[Matt Mackall]

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

-- 
Mathematics is the supreme nostalgia of our time.