Re: [PATCH 5/8] checkout(-index): do not checkout i-t-a entries
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Aug 25, 2015, 17:36 UTC
- Message-ID
- <xmqq8u8z1d8r.fsf@gitster.dls.corp.google.com>
- In-Reply-To
- <1440205700-19749-6-git-send-email-pclouds@gmail.com>
Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:
Show 8 quoted lines
> The cached blob of i-t-a entries are empty blob. By checkout, we delete > the content we have. Don't do it. > > This is done higher up instead of inside checkout_entry() because we > would have limited options in there: silently ignore, loudly ignore, > die. At higher level we can do better reporting. For example, "git > checkout -- foo" will complain that "foo" does not match pathspec, just > like when "foo" is not registered with "git add -N"
Hmm, I am not sure about the example, though. The user _told_ Git that 'foo' is a path she cares about so "does not match pathspec" is a very unfriendly thing to say. "foo does not exist in the index hence we cannot check it out" is a very good thing to say, though (and of course, I agree that not touching the working tree is a good thing to do).
Show 21 quoted lines
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
> builtin/checkout-index.c | 5 ++++-
> builtin/checkout.c | 2 ++
> t/t2203-add-intent.sh | 34 ++++++++++++++++++++++++++++++++++
> 3 files changed, 40 insertions(+), 1 deletion(-)
>
> diff --git a/builtin/checkout-index.c b/builtin/checkout-index.c
> index 8028c37..eca975d 100644
> --- a/builtin/checkout-index.c
> +++ b/builtin/checkout-index.c
> @@ -56,7 +56,8 @@ static int checkout_file(const char *name, const char *prefix)
> while (pos < active_nr) {
> struct cache_entry *ce = active_cache[pos];
> if (ce_namelen(ce) != namelen ||
> - memcmp(ce->name, name, namelen))
> + memcmp(ce->name, name, namelen) ||
> + ce_intent_to_add(ce))
> break;
> has_same_name = 1;
> pos++;'pos' here comes from cache_name_pos(), and it is either the location an entry with the same name exists, or the location a new entry with the given name would be inserted at.
When we detect that the entry at pos is different from the given name, we break out of the loop, because we _know_ that an entry with the same name cannot exist. Does the same hold for an i-t-a entry that exactly records the given name?
The answer is yes, as you cannot have an unmerged i-t-a entry. If a path marked as i-t-a is in the index, no other entry with the same name would exist.
So this change is good. I would actually have introduced another bool has_i_t_a to make the reporting richer, as it is your primary reason why you are touching this part of the code instead of checkout_entry(). The reporting part then would become
fprintf(stderr, "git checkout-index: %s ", name); - if (!has_same_name) + if (has_i_t_a) + fprintf(stderr, "is not yet in the cache); else if (!has_same_name) fprintf(stderr, "is not in the cache");
;-)
Show 6 quoted lines
> @@ -99,6 +100,8 @@ static void checkout_all(const char *prefix, int prefix_length) > if (ce_stage(ce) != checkout_stage > && (CHECKOUT_ALL != checkout_stage || !ce_stage(ce))) > continue; > + if (ce_intent_to_add(ce)) > + continue;
This one is obviously good ;-)
Show 13 quoted lines
> diff --git a/builtin/checkout.c b/builtin/checkout.c > index e1403be..02889d4 100644 > --- a/builtin/checkout.c > +++ b/builtin/checkout.c > @@ -300,6 +300,8 @@ static int checkout_paths(const struct checkout_opts *opts, > * anything to this entry at all. > */ > continue; > + if (ce_intent_to_add(ce)) > + continue; > /* > * Either this entry came from the tree-ish we are > * checking the paths out of, or we are checking out
Hmm, while this does prevent the later code from checking it out, I am not sure how well this interacts with ps_matched[] logic here. If the user told Git that 'foo' is a path that she cares about with "add -N foo", and said "git checkout -- foo", should we be somehow saying that 'foo' did match but there is nothing to check out, or something?