From: Nguyen Thai Ngoc Duy <pclouds@gmail.com>
Date: Fri, 30 Jul 2010 08:24:59 GMT
Subject: Re: [PATCH 1/3] Fix sparse checkout not removing files from index
Message-ID: <AANLkTi=3pX=k=Pf5SnWt8s=hbvwAXcZdmcqP_+kgCyE5@mail.gmail.com>
In-Reply-To: <20100730013534.GB2182@burratino>

2010/7/30 Jonathan Nieder <jrnieder@gmail.com>:
>> +++ b/cache.h
>> @@ -179,8 +179,7 @@ struct cache_entry {
>>  #define CE_UNHASHED  (0x200000)
>>  #define CE_CONFLICTED (0x800000)
>>
>> -/* Only remove in work directory, not index */
>> -#define CE_WT_REMOVE (0x400000)
>> +#define CE_WT_REMOVE (0x400000) /* remove in work directory */
>
> Before[1], CE_REMOVE was used by read-tree et al in core to represent
> something to be removed from the index file and work tree (e.g., when
> switching branches).
>
> In the new order, one uses CE_REMOVE|CE_WT_REMOVE for that, right?

For unpack_trees() only, yes. You made me grep for CE_REMOVE and found
that do_diff_cache() may turn CE_REMOVE on. The function is used by
blame and reset.

Hmm.. It's for removing staged entries. So it's probably fine.

>> +++ b/t/t1011-read-tree-sparse-checkout.sh
>> @@ -147,4 +147,11 @@ test_expect_success 'read-tree adds to worktree, dirty case' '
>>       grep -q dirty sub/added
>>  '
>>
>> +test_expect_success 'read-tree --reset removes outside worktree' '
>> +     echo init.t > .git/info/sparse-checkout &&
>> +     git checkout -f top &&
>> +     git reset --hard removed &&
>> +     test -z "$(git ls-files sub/added)"
>> +'
>> +
>
> Using reset --hard to remove a file outside the sparse checkout.
> A sane, simple test; thanks.  (Nitpick: I would have used
>
>        >empty &&
>        git ls-files sub/added >output &&
>        test_cmp empty output
>
> even though that does not produce any more helpful output in this
> case.)

Thanks. That looks better.

>
>> +++ b/unpack-trees.c
>> @@ -53,6 +53,9 @@ static void add_entry(struct unpack_trees_options *o, struct cache_entry *ce,
>>
>>       clear |= CE_HASHED | CE_UNHASHED;
>>
>> +     if (set & CE_REMOVE)
>> +             set |= CE_WT_REMOVE;
>> +
>
> A bridge between the old and new worlds.
>
> I would have added CE_WT_REMOVE to callers instead (they all pass
> constants more or less), but I suppose this way makes the patch
> shorter.

It'd better be done in one place. I don't expect anybody to set
CE_REMOVE without CE_WT_REMOVE any time soon. Adding it the the
callers, the new callers might miss it.

>> @@ -799,10 +796,15 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options
>>                       /*
>>                        * Merge strategies may set CE_UPDATE|CE_REMOVE outside checkout
>>                        * area as a result of ce_skip_worktree() shortcuts in
>> -                      * verify_absent() and verify_uptodate(). Clear them.
>> +                      * verify_absent() and verify_uptodate().
>> +                      * Make sure they don't modify worktree.
>>                        */
>> -                     if (ce_skip_worktree(ce))
>> -                             ce->ce_flags &= ~(CE_UPDATE | CE_REMOVE);
>> +                     if (ce_skip_worktree(ce)) {
>> +                             ce->ce_flags &= ~CE_UPDATE;
>> +
>> +                             if (ce->ce_flags & CE_REMOVE)
>> +                                     ce->ce_flags &= ~CE_WT_REMOVE;
>> +                     }
>>                       else
>>                               empty_worktree = 0;
>
> Ah, and at last we come to the fix. :)
>
> This is a little tricky: the CE_WT_REMOVE case (without CE_REMOVE)
> represents a narrowing of the checkout and should be preserved,
> while CE_WT_REMOVE|CE_REMOVE represents a removed index entry and
> should be changed to just CE_REMOVE.

Yeah. I did wonder if it's worth a comment to explain. I forget why I
did not add that comment now.
-- 
Duy