[PATCH 0 of 5] remove

Matt Mackall mpm at selenic.com
Mon May 23 16:09:58 CDT 2011


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

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.

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.

-- 
Mathematics is the supreme nostalgia of our time.




More information about the Mercurial-devel mailing list