From: Elijah Newren <newren@gmail.com>
To: Matheus Tavares <matheus.bernardino@usp.br>
Cc: Git Mailing List <git@vger.kernel.org>,
Junio C Hamano <gitster@pobox.com>,
Derrick Stolee <stolee@gmail.com>
Subject: Re: [PATCH v2 6/7] add: warn when pathspec only matches SKIP_WORKTREE entries
Date: Thu, 4 Mar 2021 09:21:48 -0800 [thread overview]
Message-ID: <CABPp-BE9QA5RAwdebmAyK3b3wh1mGE+NF7sPS5RCf4FogYBV7A@mail.gmail.com> (raw)
In-Reply-To: <20210304152315.18498-1-matheus.bernardino@usp.br>
On Thu, Mar 4, 2021 at 7:23 AM Matheus Tavares
<matheus.bernardino@usp.br> wrote:
>
> Hi, Elijah
>
> I was going to send v3 a couple days ago, but there is still one issue
> that I'm having a bit of trouble with:
>
> On Wed, Feb 24, 2021 at 3:50 AM Elijah Newren <newren@gmail.com> wrote:
> >
> > Hmm...here's an interesting command sequence:
> >
> > git init lame
> > cd lame
> > mkdir baz
> > touch baz/tracked
> > git add baz/tracked
> > git update-index --skip-worktree baz/tracked
> > rm baz/tracked. # But leave the empty directory!
> > echo baz >.gitignore
> > git add --ignore-missing --dry-run baz
> >
> >
> > Reports the following:
> > """
> > The following pathspecs only matched index entries outside the current
> > sparse checkout:
> > baz
> > hint: Disable or modify the sparsity rules if you intend to update such entries.
> > hint: Disable this message with "git config advice.updateSparsePath false"
> > The following paths are ignored by one of your .gitignore files:
> > baz
> > hint: Use -f if you really want to add them.
> > hint: Turn this message off by running
> > hint: "git config advice.addIgnoredFile false"
> > """
> >
> > That's probably okay because it does match both, but the "only
> > matched" in the first message followed by saying it matched something
> > else seems a little surprising at first.
>
> After giving this a little more thought, I got to this idea [1]. With
> this patch, git-add doesn't warn about sparse paths that are inside
> ignored dirs (it only warns about the ignored dirs themselves). If
> --force is used, then git-add recurses into the ignored directories, and
> warns about any sparse path inside them. I think this is more consistent
> with the way in which ignored dirs (and their contents) are handled with
> and without --force. And we avoid the double warning.
>
> But there is a performance penalty. The relevant code is:
>
> @@ -2029,19 +2033,23 @@ static int exclude_matches_pathspec(const char *path, int pathlen,
> PATHSPEC_ICASE |
> PATHSPEC_EXCLUDE);
>
> + assert(matched_ignored);
> +
> for (i = 0; i < pathspec->nr; i++) {
> const struct pathspec_item *item = &pathspec->items[i];
> int len = item->nowildcard_len;
>
> - if (len == pathlen &&
> - !ps_strncmp(item, item->match, path, pathlen))
> - return 1;
> - if (len > pathlen &&
> - item->match[pathlen] == '/' &&
> - !ps_strncmp(item, item->match, path, pathlen))
> - return 1;
> + if (matched && matched_ignored[i])
> + continue;
> +
> + if ((len == pathlen ||
> + (len > pathlen && item->match[pathlen] == '/')) &&
> + !ps_strncmp(item, item->match, path, pathlen)) {
> + matched = 1;
> + matched_ignored[i] = 1;
> + }
> }
> - return 0;
> + return matched;
> }
>
> This function is called for each ignored path (not recursing into
> completely ignored dirs, though). Before the patch, the internal loop
> stops at the first pathspec match. But with the patch, the loop must
> continue as we need to know about all pathspecs that had matches with
> the ignored paths...
>
> And there is also one odd case:
> $ mkdir d
> $ touch d/s d/i
> $ git add d/s
> $ git update-index --skip-worktree d/s
> $ echo d/i >.gitinore
> $ git add 'd/[is]'
>
> This outputs:
> The following pathspecs only matched index entries outside the current
> sparse checkout:
> d/[is]
> (and no warning on ignored paths).
>
> I don't think the warning here is 100% correct, as 'd/[is]' *does* match
> other files, only they are ignored. And git-add only warns about an
> ignored path when the pathspec is an exact match with it (no glob
> patterns) or inside it.
>
> So, I don't know... Perhaps should we just rephrase the sparse warning
> to remove the "only" part (and don't try to avoid the double warnings)?
> I'm open to any suggestions on alternative wordings or ideas :)
>
> [1]: https://github.com/matheustavares/git/commit/5d4be321e1f69cb08b0666ffd3f8fa658e9f6953
The only thing that I think was problematic about the double warning
was the contradiction between them due to the use of "only" in the
first; if we remove that, I think printing two warnings is perfectly
fine. So, I'm in favor of just rephrasing as you suggest.
I think it's kind of lame that git-add won't provide a warning for
ignored files when the match is via glob/pattern; if git-add didn't
end up adding anything, but an ignored file was matched, I think a
warning is appropriate. However, I don't think your patch series
needs to address that; your series addressing sparse behavior is long
enough, and fixing the handling with ignored files could be done
separately.
next prev parent reply other threads:[~2021-03-04 17:23 UTC|newest]
Thread overview: 93+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-11-12 21:01 [PATCH] rm: honor sparse checkout patterns Matheus Tavares
2020-11-12 23:54 ` Elijah Newren
2020-11-13 13:47 ` Derrick Stolee
2020-11-15 20:12 ` Matheus Tavares Bernardino
2020-11-15 21:42 ` Johannes Sixt
2020-11-16 12:37 ` Matheus Tavares Bernardino
2020-11-23 13:23 ` Johannes Schindelin
2020-11-24 2:48 ` Matheus Tavares Bernardino
2020-11-16 14:30 ` Jeff Hostetler
2020-11-17 4:53 ` Elijah Newren
2020-11-16 13:58 ` [PATCH v2] " Matheus Tavares
2021-02-17 21:02 ` [RFC PATCH 0/7] add/rm: honor sparse checkout and warn on sparse paths Matheus Tavares
2021-02-17 21:02 ` [RFC PATCH 1/7] add --chmod: don't update index when --dry-run is used Matheus Tavares
2021-02-17 21:45 ` Junio C Hamano
2021-02-18 1:33 ` Matheus Tavares
2021-02-17 21:02 ` [RFC PATCH 2/7] add: include magic part of pathspec on --refresh error Matheus Tavares
2021-02-17 22:20 ` Junio C Hamano
2021-02-17 21:02 ` [RFC PATCH 3/7] t3705: add tests for `git add` in sparse checkouts Matheus Tavares
2021-02-17 23:01 ` Junio C Hamano
2021-02-17 23:22 ` Eric Sunshine
2021-02-17 23:34 ` Junio C Hamano
2021-02-18 3:11 ` Matheus Tavares Bernardino
2021-02-18 3:07 ` Matheus Tavares Bernardino
2021-02-18 14:38 ` Matheus Tavares
2021-02-18 19:05 ` Junio C Hamano
2021-02-18 19:02 ` Junio C Hamano
2021-02-22 18:53 ` Elijah Newren
2021-02-17 21:02 ` [RFC PATCH 4/7] add: make --chmod and --renormalize honor " Matheus Tavares
2021-02-17 21:02 ` [RFC PATCH 5/7] pathspec: allow to ignore SKIP_WORKTREE entries on index matching Matheus Tavares
2021-02-17 21:02 ` [RFC PATCH 6/7] add: warn when pathspec only matches SKIP_WORKTREE entries Matheus Tavares
2021-02-19 0:34 ` Junio C Hamano
2021-02-19 17:11 ` Matheus Tavares Bernardino
2021-02-17 21:02 ` [RFC PATCH 7/7] rm: honor sparse checkout patterns Matheus Tavares
2021-02-22 18:57 ` [RFC PATCH 0/7] add/rm: honor sparse checkout and warn on sparse paths Elijah Newren
2021-02-24 4:05 ` [PATCH v2 " Matheus Tavares
2021-02-24 4:05 ` [PATCH v2 1/7] add: include magic part of pathspec on --refresh error Matheus Tavares
2021-02-24 4:05 ` [PATCH v2 2/7] t3705: add tests for `git add` in sparse checkouts Matheus Tavares
2021-02-24 5:15 ` Elijah Newren
2021-02-24 4:05 ` [PATCH v2 3/7] add: make --chmod and --renormalize honor " Matheus Tavares
2021-02-24 4:05 ` [PATCH v2 4/7] pathspec: allow to ignore SKIP_WORKTREE entries on index matching Matheus Tavares
2021-02-24 5:23 ` Elijah Newren
2021-02-24 4:05 ` [PATCH v2 5/7] refresh_index(): add REFRESH_DONT_MARK_SPARSE_MATCHES flag Matheus Tavares
2021-02-24 4:05 ` [PATCH v2 6/7] add: warn when pathspec only matches SKIP_WORKTREE entries Matheus Tavares
2021-02-24 6:50 ` Elijah Newren
2021-02-24 15:33 ` Matheus Tavares
2021-03-04 15:23 ` Matheus Tavares
2021-03-04 17:21 ` Elijah Newren [this message]
2021-03-04 21:03 ` Junio C Hamano
2021-03-04 22:48 ` Elijah Newren
2021-03-04 21:26 ` Matheus Tavares Bernardino
2021-02-24 4:05 ` [PATCH v2 7/7] rm: honor sparse checkout patterns Matheus Tavares
2021-02-24 6:59 ` Elijah Newren
2021-02-24 7:05 ` [PATCH v2 0/7] add/rm: honor sparse checkout and warn on sparse paths Elijah Newren
2021-03-12 22:47 ` [PATCH v3 " Matheus Tavares
2021-03-12 22:47 ` [PATCH v3 1/7] add: include magic part of pathspec on --refresh error Matheus Tavares
2021-03-12 22:47 ` [PATCH v3 2/7] t3705: add tests for `git add` in sparse checkouts Matheus Tavares
2021-03-23 20:00 ` Derrick Stolee
2021-03-12 22:47 ` [PATCH v3 3/7] add: make --chmod and --renormalize honor " Matheus Tavares
2021-03-12 22:47 ` [PATCH v3 4/7] pathspec: allow to ignore SKIP_WORKTREE entries on index matching Matheus Tavares
2021-03-12 22:48 ` [PATCH v3 5/7] refresh_index(): add REFRESH_DONT_MARK_SPARSE_MATCHES flag Matheus Tavares
2021-03-18 23:45 ` Junio C Hamano
2021-03-19 0:00 ` Junio C Hamano
2021-03-19 12:23 ` Matheus Tavares Bernardino
2021-03-19 16:05 ` Junio C Hamano
2021-03-30 18:51 ` Matheus Tavares Bernardino
2021-03-31 9:14 ` Elijah Newren
2021-03-12 22:48 ` [PATCH v3 6/7] add: warn when asked to update SKIP_WORKTREE entries Matheus Tavares
2021-03-12 22:48 ` [PATCH v3 7/7] rm: honor sparse checkout patterns Matheus Tavares
2021-03-21 23:03 ` Ævar Arnfjörð Bjarmason
2021-03-22 1:08 ` Matheus Tavares Bernardino
2021-03-23 20:47 ` Derrick Stolee
2021-03-13 7:07 ` [PATCH v3 0/7] add/rm: honor sparse checkout and warn on sparse paths Elijah Newren
2021-04-08 20:41 ` [PATCH v4 " Matheus Tavares
2021-04-08 20:41 ` [PATCH v4 1/7] add: include magic part of pathspec on --refresh error Matheus Tavares
2021-04-08 20:41 ` [PATCH v4 2/7] t3705: add tests for `git add` in sparse checkouts Matheus Tavares
2021-04-14 16:39 ` Derrick Stolee
2021-04-08 20:41 ` [PATCH v4 3/7] add: make --chmod and --renormalize honor " Matheus Tavares
2021-04-08 20:41 ` [PATCH v4 4/7] pathspec: allow to ignore SKIP_WORKTREE entries on index matching Matheus Tavares
2021-04-08 20:41 ` [PATCH v4 5/7] refresh_index(): add flag to ignore SKIP_WORKTREE entries Matheus Tavares
2021-04-08 20:41 ` [PATCH v4 6/7] add: warn when asked to update " Matheus Tavares
2021-04-08 20:41 ` [PATCH v4 7/7] rm: honor sparse checkout patterns Matheus Tavares
2021-04-14 16:36 ` [PATCH v4 0/7] add/rm: honor sparse checkout and warn on sparse paths Elijah Newren
2021-04-14 18:04 ` Matheus Tavares Bernardino
2021-04-16 21:33 ` Junio C Hamano
2021-04-16 23:17 ` Elijah Newren
2020-11-16 20:14 ` [PATCH] rm: honor sparse checkout patterns Junio C Hamano
2020-11-17 5:20 ` Elijah Newren
2020-11-20 17:06 ` Elijah Newren
2020-12-31 20:03 ` sparse-checkout questions and proposals [Was: Re: [PATCH] rm: honor sparse checkout patterns] Elijah Newren
2021-01-04 3:02 ` Derrick Stolee
2021-01-06 19:15 ` Elijah Newren
2021-01-07 12:53 ` Derrick Stolee
2021-01-07 17:36 ` Elijah Newren
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=CABPp-BE9QA5RAwdebmAyK3b3wh1mGE+NF7sPS5RCf4FogYBV7A@mail.gmail.com \
--to=newren@gmail.com \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
--cc=matheus.bernardino@usp.br \
--cc=stolee@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
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;
as well as URLs for NNTP newsgroup(s).