[PATCH 1 of 1] commands.remove: don't use workingctx.remove(list, unlink=True)

[Adrian Buehlmann]

On 2011-05-25 19:24, Matt Mackall wrote:
> On Wed, 2011-05-25 at 14:45 +0200, Adrian Buehlmann wrote:
>> # HG changeset patch
>> # User Adrian Buehlmann <adrian at cadifra.com>
>> # Date 1306313693 -7200
>> # Node ID db3623327c9c02f44b6bd9374dfa8594b326820d
>> # Parent  a6b543e053058c39b52e2b7c1e9a4b7c14c66a56
>> commands.remove: don't use workingctx.remove(list, unlink=True)
>>
>> workingctx.remove(list, unlink=True) is unsuited here, because it does too
>> much: it also unlinks added files.
> 
> I think we're converging here. I've done a survey of the behavior of the
> remove/forget methods at all three levels. They all have significantly
> different semantics.
> 
> W = warning
> S = schedule removal from manifest (move to 'r' state)
> D = delete from working directory
> d = delete attempted
> U = unlist from dirstate
> - = silently ignore
> 
> A = added
> C = clean
> M = modified
> ! = missing
> ! = added but missing
> ? = unknown
> 
> command level:
> 
>  remove: all sorts of things
>          A  C  M  !  A! ?
>   none   W  SD W  S  U  W  <- safe
>   -f     U  SD SD S  U  W  <- dangerous
>   -A     W  W  W  S  U  W  <- confusing
>   -Af    U  S  S  S  U  W  <- same as forget, safe again
> 
>  forget: stop tracking file pattern
>          A  C  M  !  A! ?
>          U  S  S  S  U  W
> 
> context level:
> 
>  remove: stop tracking list of files, optional deletion
>          A  C  M  !  A! ?
>    none  U  S  S  S  U  W
>  unlink  UD SD SD Sd Ud WD  <- we delete added/unknown files, then warn
> 
>  forget: stop tracking newly-added list of files
>          A  C  M  !  A! ?
>          U  W  W  W  W  W
> 
> dirstate level:
>  remove: move a single file to the 'r' state
>          A  C  M  !  A! ?
>          S  S  S  S  S  -   <- moving 'a' to 'r' is nonsense
>  forget: drop a single file 
>          A  C  M  !  A! ?
>          U  U  U  U  U  W   <- dropping 'n' state may be corrupt
> 
> Observations:
> 
> a) commands.forget is identical to context.remove(False)
> b) context.remove(True) has some dangerous behavior
> c) context.forget is very narrow
> d) dirstate.remove and forget are overbroad
> 
> So it seems like we want to:
> 
> 1) move actual deletion out of context.remove() and drop the arg
> 2) unify remove and forget into a single method at both context and
> dirstate levels:
> 
>  newfunc:
>          A  C  M  !  A! ?
>          U  S  S  S  U  W
> 
> Note that if dirstate.newfunc knows how to deal with added files
> specially, this has the advantage that context.newfunc can delegate that
> detail and become very simple.
> 
> What this func should be called is not clear. On the one hand, it
> precisely matches commands.forget behavior, so forget seems good. But it
> also strongly disagrees with the existing context and dirstate.forget,
> so there might be some short-term pain associated with that change.
> 'remove' is not a good name because it doesn't agree with the command
> layer. 'drop' is another possibility.

It's good that you have a plan.

Is there anything you are expecting from me that I say here?

I think my patch conforms to your step 1), so it could be taken, because
it conforms to your plan.