RE: [PATCH 1/1] reset: fix reset when using the sparse-checkout feature.

> Kevin Willford <kewillf@microsoft.com> writes:
> 
> > diff --git a/builtin/reset.c b/builtin/reset.c
> > index d72c7d1c96..1b8bb45989 100644
> > --- a/builtin/reset.c
> > +++ b/builtin/reset.c
> > @@ -24,6 +24,7 @@
> >  #include "cache-tree.h"
> >  #include "submodule.h"
> >  #include "submodule-config.h"
> > +#include "dir.h"
> >
> >  static const char * const git_reset_usage[] = {
> >  	N_("git reset [--mixed | --soft | --hard | --merge | --keep] [-q]
> [<commit>]"),
> > @@ -124,8 +125,32 @@ static void update_index_from_diff(struct
> diff_queue_struct *q,
> >
> >  	for (i = 0; i < q->nr; i++) {
> >  		struct diff_filespec *one = q->queue[i]->one;
> > +		struct diff_filespec *two = q->queue[i]->two;
> > +		int pos;
> >  		int is_missing = !(one->mode && !is_null_oid(&one->oid));
> > +		int was_missing = !two->mode && is_null_oid(&two->oid);
> >  		struct cache_entry *ce;
> > +		struct cache_entry *ceBefore;
> > +		struct checkout state = CHECKOUT_INIT;
> 
> The five new variables are only used in the new block, so it
> probably is better to limit their scope to the "we do something
> unusual when sparse checkout is in effect" block as well.  The scope
> for the latter two can further be narrowed down to "we do need to
> force a checkout of this entry" block.
> 
> We prefer to name our variables with underscore (e.g. ce_before)
> over camelCase (e.g. ceBefore) unless there is a compelling reason
> (e.g. a platform specific code in compat/ layer to match platform
> convention).
> 

> > +
> > +		if (core_apply_sparse_checkout && !file_exists(two->path)) {
> > +			pos = cache_name_pos(two->path, strlen(two->path));
> > +			if ((pos >= 0 && ce_skip_worktree(active_cache[pos]))
> &&
> > +			    (is_missing || !was_missing))
> > +			{
> > +				state.force = 1;
> > +				state.refresh_cache = 1;
> > +				state.istate = &the_index;
> > +				ceBefore = make_cache_entry(two->mode,
> > +							    two->oid.hash,
> > +							    two->path, 0, 0);
> > +				if (!ceBefore)
> > +					die(_("make_cache_entry failed for path
> '%s'"),
> > +						two->path);
> > +
> > +				checkout_entry(ceBefore, &state, NULL);
> > +			}
> > +		}
> 
> Can we tell between the case where the reason why the path was not
> there in the working tree was due to the path being excluded by the
> sparse checkout and the path being removed manually by the end user?
> 
> I guess ce_skip_worktree() check is sufficient; we force checkout only
> when the path is marked to be skipped due to "sparse" thing.
> 
> Do we have to worry about the reverse case, in which file_exists(two->path)
> is true (i.e. the user created a file there manually) even though
> the path is marked to be skipped due to "sparse" setting?
>