From: Junio C Hamano <gitster@pobox•com>
To: "Nguyễn Thái Ngọc Duy" <pclouds@gmail•com>
Cc: git@vger•kernel.org
Subject: Re: [PATCH 5/6] checkout(-index): do not checkout i-t-a entries
Date: Mon, 28 Dec 2015 11:56:45 -0800 [thread overview]
Message-ID: <xmqq7fjyl536.fsf@gitster.mtv.corp.google.com> (raw)
In-Reply-To: <1451181092-26054-6-git-send-email-pclouds@gmail.com> ("Nguyễn Thái Ngọc Duy"'s message of "Sun, 27 Dec 2015 08:51:31 +0700")
Nguyễn Thái Ngọc Duy <pclouds@gmail•com> writes:
> diff --git a/builtin/checkout-index.c b/builtin/checkout-index.c
> index 9ca2da1..d9fe8f4 100644
> --- a/builtin/checkout-index.c
> +++ b/builtin/checkout-index.c
> @@ -48,6 +48,7 @@ static int checkout_file(const char *name, const char *prefix)
> int pos = cache_name_pos(name, namelen);
> int has_same_name = 0;
> int did_checkout = 0;
> + int has_intent_to_add = 0;
> int errs = 0;
>
> if (pos < 0)
> @@ -56,8 +57,11 @@ 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)) {
> + if (ce_intent_to_add(ce))
> + has_intent_to_add = 1;
> break;
> + }
> has_same_name = 1;
> pos++;
> if (ce_stage(ce) != checkout_stage
Sorry, but I cannot see what this hunk is trying to do.
Earlier, 'pos' is set to where the 'name' would be inserted (or
would replace, if there is an existing entry with the same name).
We iterate over the index starting from that 'pos' until we see a
different name (i.e. the length is different, or the name does not
match) and then break out, because that entry is not something we
want to muck with, because the entry here you are checking with
ce_intent_to_add() is an entry that is entirely unrelated to the
path you are checking out.
So you do not check it out, just as before, which is correct, but
then this 'has_intent_to_add' flag is used to say something
nonsensical below ...
> @@ -77,7 +81,9 @@ static int checkout_file(const char *name, const char *prefix)
>
> if (!state.quiet) {
> fprintf(stderr, "git checkout-index: %s ", name);
> - if (!has_same_name)
> + if (has_intent_to_add)
> + fprintf(stderr, "is not yet in the cache");
... the reason why you are here does not have anything to do with the
entry that happened to come after the path you wanted to check out
was marked with the I-T-A bit. The reason why you are here is
because 'did_checkout' was unset, which in turn was because there
wasn't a path that match 'name' you are checking out in the index,
with or without the I-T-A bit.
$ rm .git/index
$ >file
$ git add -N file
$ git checkout-index fild
would hit this codepath, I think, and the code would give a message
that says "fild is not yet in the cache", which may be technically
correct but it is the same as !has_same_name's "fild is not in the
cache".
Probably the I-T-A check must go after the code decides that 'ce' is
an entry for the 'name' the caller asked us to check out (and of
course we shouldn't check it out in that case).
> @@ -99,6 +105,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 probably correct, I didn't check the context to fully
see if it is valid, though.
> diff --git a/builtin/checkout.c b/builtin/checkout.c
> index 3e141fc..ac37d92 100644
> --- a/builtin/checkout.c
> +++ b/builtin/checkout.c
> @@ -328,12 +328,15 @@ static int checkout_paths(const struct checkout_opts *opts,
> if (opts->merge)
> unmerge_marked_index(&the_index);
>
> - /* Any unmerged paths? */
> for (pos = 0; pos < active_nr; pos++) {
> - const struct cache_entry *ce = active_cache[pos];
> + struct cache_entry *ce = active_cache[pos];
> if (ce->ce_flags & CE_MATCHED) {
> - if (!ce_stage(ce))
> + if (!ce_stage(ce)) {
> + if (ce_intent_to_add(ce))
> + ce->ce_flags &= ~CE_MATCHED;
> continue;
> + }
> + /* Unmerged paths */
> if (opts->force) {
> warning(_("path '%s' is unmerged"), ce->name);
> } else if (opts->writeout_stage) {
This loop has become dual-purpose; it is drops CE_MATCHED bit from
cache entries with the I-T-A bit, and it continues to handle errors
for an attempt to checkout unmerged paths.
$ rm .git/index
$ >file
$ git add -N file
$ git checkout file
would hit this codepath, and it will force-skip "file".
There is a small source of confusion for new people here, though.
$ git checkout filo
would complain that there is no 'filo'. But
$ git checkout file
would silently turn itself into a no-op. Don't we want to at least
give some indication that we are not checking out "file" because its
contents do not yet exist in the index, or something?
> diff --git a/t/t2203-add-intent.sh b/t/t2203-add-intent.sh
> index 96c8755..52e9f7f 100755
> --- a/t/t2203-add-intent.sh
> +++ b/t/t2203-add-intent.sh
> @@ -111,5 +111,39 @@ test_expect_success 'apply:check_preimage() not creating empty file' '
> )
> '
>
> +test_expect_success 'checkout ignores i-t-a' '
> + git init checkout &&
> + (
> + cd checkout &&
> + echo data >file &&
> + git add -N file &&
> + git checkout -- file &&
> + echo data >expected &&
> + test_cmp expected file
> + )
> +'
> +
> +test_expect_success 'checkout-index ignores i-t-a' '
> + (
> + cd checkout &&
> + git checkout-index file &&
> + echo data >expected &&
> + test_cmp expected file
> + )
> +'
> +
> +test_expect_success 'checkout-index --all ignores i-t-a' '
> + (
> + cd checkout &&
> + echo data >anotherfile &&
> + git add anotherfile &&
> + rm anotherfile &&
> + git checkout-index --all &&
> + echo data >expected &&
> + test_cmp expected file &&
> + test_cmp expected anotherfile
> + )
> +'
> +
> test_done
next prev parent reply other threads:[~2015-12-28 19:56 UTC|newest]
Thread overview: 15+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-12-27 1:51 [PATCH 0/6] nd/ita-cleanup updates Nguyễn Thái Ngọc Duy
2015-12-27 1:51 ` [PATCH 1/6] blame: remove obsolete comment Nguyễn Thái Ngọc Duy
2015-12-28 17:35 ` Junio C Hamano
2015-12-27 1:51 ` [PATCH 2/6] Add and use convenient macro ce_intent_to_add() Nguyễn Thái Ngọc Duy
2015-12-28 17:53 ` Junio C Hamano
2015-12-27 1:51 ` [PATCH 3/6] apply: fix adding new files on i-t-a entries Nguyễn Thái Ngọc Duy
2015-12-28 3:01 ` Junio C Hamano
2015-12-29 14:02 ` Duy Nguyen
2015-12-29 17:40 ` Junio C Hamano
2015-12-27 1:51 ` [PATCH 4/6] apply: make sure check_preimage() does not leave empty file on error Nguyễn Thái Ngọc Duy
2015-12-28 18:35 ` Junio C Hamano
2015-12-27 1:51 ` [PATCH 5/6] checkout(-index): do not checkout i-t-a entries Nguyễn Thái Ngọc Duy
2015-12-28 19:56 ` Junio C Hamano [this message]
2015-12-27 1:51 ` [PATCH 6/6] grep: make it clear i-t-a entries are ignored Nguyễn Thái Ngọc Duy
2015-12-28 19:59 ` Junio C Hamano
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=xmqq7fjyl536.fsf@gitster.mtv.corp.google.com \
--to=gitster@pobox$(echo .)com \
--cc=git@vger$(echo .)kernel.org \
--cc=pclouds@gmail$(echo .)com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox