[PATCH 0 of 5] remove

[Adrian Buehlmann]

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'll paste the code below (removing the doc text, excerpt from commands.py):


def forget(ui, repo, *pats, **opts):
    if not pats:
        raise util.Abort(_('no files specified'))

    m = scmutil.match(repo, pats, opts)
    s = repo.status(match=m, clean=True)
    forget = sorted(s[0] + s[1] + s[3] + s[6])
    errs = 0

    for f in m.files():
        if f not in repo.dirstate and not os.path.isdir(m.rel(f)):
            ui.warn(_('not removing %s: file is already untracked\n')
                    % m.rel(f))
            errs = 1

    for f in forget:
        if ui.verbose or not m.exact(f):
            ui.status(_('removing %s\n') % m.rel(f))

    repo[None].remove(forget, unlink=False)
    return errs


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.

The thing is, I was looking at something entirely different (still on an
auditor fishing expedition, I'll probably demonstrate in few days what I
found there) and stumbled across this double wlocking, which looked
messy to me, so I tried taking a stab at morphing this into a single
wlock acquisition.

(apologies again for my lousy -- or rather missing -- explanations.)

>> Patch 3 removes function forget of workingctx, which is obsoleted by
>> patches 1 and 2.
>>
>> Patch 4 is not strictly related, but I happened to have that done after
>> 3. As I wrote in the comment there: it moves the unlinking of the files
>> to under the lock.
> 
> That seems sensible. I'll definitely take that one if you send it
> separately.

I'll do that. Thanks.

>> Patch 5 is not particularly related, it just improves the help text if
>> of the 'hg remove' command.
> 
> And this one is of course fine.