From: Junio C Hamano [mailto:[email protected]]
Sent: Friday, September 8, 2017 1:02 PM
> Kevin Willford <[email protected]> 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).
>
Will update.
> > +
> > + 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?
>
I don't believe so because if the user has a file there whether they modified
it or not, it is what the user did and we just leave it there and a diff with
what the index gets reset to will show how the file is different from what the
index got reset to.
> Other than that, the patch looks quite cleanly done and well explained.
>
> Thanks.
>
