Re: [PATCH 1/3] Fix sparse checkout not removing files from index
- From
Jonathan Nieder <jrnieder@gmail.com>
- Date
- Jul 30, 2010, 01:35 UTC
- Message-ID
- <20100730013534.GB2182@burratino>
- In-Reply-To
- <1280135310-2347-1-git-send-email-pclouds@gmail.com>
Nguyễn Thái Ngọc Duy wrote:
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
The log message left me confused. Let’s see if the code is easier.
Show 8 quoted lines
> +++ 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?
Show 12 quoted lines
> +++ 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.)
Show 8 quoted lines
> +++ 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.
[...]
Show 6 quoted lines
> @@ -84,7 +87,7 @@ static int check_updates(struct unpack_trees_options *o)
> if (o->update && o->verbose_update) {
> for (total = cnt = 0; cnt < index->cache_nr; cnt++) {
> struct cache_entry *ce = index->cache[cnt];
> - if (ce->ce_flags & (CE_UPDATE | CE_REMOVE | CE_WT_REMOVE))
> + if (ce->ce_flags & (CE_UPDATE | CE_WT_REMOVE))Only entries marked CE_WT_REMOVE will be touched by read-tree -u.
Show 6 quoted lines
> @@ -104,12 +107,6 @@ static int check_updates(struct unpack_trees_options *o)
> unlink_entry(ce);
> continue;
> }
> -
> - if (ce->ce_flags & CE_REMOVE) {[...]
and the CE_WT_REMOVE case takes care of that. Makes sense.
Show 18 quoted lines
> @@ -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.
But it is a clear improvement over the code from before and should behave as advertised now. So aside from the log message,
Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>
Thanks.
[1] v1.6.6.1~23^2~1 (Make on-disk index representation separate from in-core one, 2008-01-14) and v0.99~295 (git-read-tree: remove deleted files in the working directory, 2005-06-09).