From: Junio C Hamano [mailto:gitster@xxxxxxxxx] Sent: Friday, September 8, 2017 1:02 PM > Kevin Willford <kewillf@xxxxxxxxxxxxx> 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. >
