From: Matheus Tavares Bernardino <matheus.bernardino@usp.br>
To: Elijah Newren <newren@gmail.com>
Cc: Git Mailing List <git@vger.kernel.org>,
Junio C Hamano <gitster@pobox.com>,
Derrick Stolee <stolee@gmail.com>,
Jonathan Tan <jonathantanmy@google.com>
Subject: Re: [PATCH v3 4/5] grep: honor sparse checkout patterns
Date: Wed, 10 Jun 2020 14:08:35 -0300 [thread overview]
Message-ID: <CAHd-oW4bKG+MDCaDm+5Ha-+vvFy=LT9nnxCmQUre-BCtqWcd3Q@mail.gmail.com> (raw)
In-Reply-To: <CABPp-BF6=s-cAy98d-FTTExeR18YhxrHmDNzbH-1P8AiMfskfg@mail.gmail.com>
On Tue, Jun 2, 2020 at 11:38 PM Elijah Newren <newren@gmail.com> wrote:
>
> On Sun, May 31, 2020 at 9:44 PM Matheus Tavares Bernardino
> <matheus.bernardino@usp.br> wrote:
> >
> > On Sat, May 30, 2020 at 12:48 PM Elijah Newren <newren@gmail.com> wrote:
> > >
> > > On Wed, May 27, 2020 at 6:13 PM Matheus Tavares
> > > <matheus.bernardino@usp.br> wrote:
> > > >
> > > > +static int in_sparse_checkout(struct strbuf *path, int prefix_len,
> > >
> > > This function name in_sparse_checkout() makes me think "Does the
> > > working tree represent a sparse checkout?" Perhaps we could rename it
> > > to path_matches_sparsity_patterns() ?
> > >
> > > Also, is there a reason we can't use dir.c's
> > > path_matches_pattern_list() here?
> >
> > Oh, we do use path_matches_pattern_list() inside:
> >
> > > > + *match = path_matches_pattern_list(path->buf, path->len,
> > > > + path->buf + prefix_len, &dtype,
> > > > + sparsity, istate);
> > > > + if (*match == UNDECIDED)
> > > > + *match = parent_match;
> >
> > > How does this new function differ
> > > in behavior from that function?
> >
> > The idea of in_sparse_checkout() is to implement a logic closer to
> > what we have in clear_ce_flags_1(). Here, it is effectively a wrapper
> > to path_matches_pattern_list() but with some extra logic to decide
> > whether grep should search in a given entry, based on its mode, the
> > match result against the sparsity patterns, and the result from the
> > parent dir.
>
> I've had this response and one to 5/5 sitting in my draft folder for
> over a day because I was hoping to go read clear_ce_flags_1() and find
> out what it is. I have no idea, so your answer doesn't answer my
> question... ;-) I'll try to find some time and maybe respond further
> after I do.
Oops, sorry for the incomplete answer. clear_ce_flags() recursively
traverses the index entries, unsetting the bits specified in a given
mask when the entry matches a given pattern list. (It is used in
unpack-trees.c:mark_new_skip_worktree() to clear the
CE_NEW_SKIP_WORKTREE bit for the matched entries.) clear_ce_flags()
does use path_matches_pattern_list() but it also has to check some
additional rules for cone mode (as there might be recursive
matches/non-matches). These rules are implemented in
clear_ce_flags_dir().
in_sparse_checkout() is a small wrapper around
path_matches_pattern_list() with (1) the additional checks for cone
mode, similar to what clear_ce_flags_dir() implements, and (2) the
usage of the parent dir's match_result when undecided about the
current path. We could just implement this directly in grep_tree(),
but I thought that isolating this logic into its own static function
would make grep_tree() more readable.
> > > > diff --git a/t/t7817-grep-sparse-checkout.sh b/t/t7817-grep-sparse-checkout.sh
> > > > new file mode 100755
> > > > index 0000000000..ce080cf572
> > > > --- /dev/null
> > > > +++ b/t/t7817-grep-sparse-checkout.sh
> > >
> > > Looks good. Do we want to add a testcase where a file is unmerged and
> > > present in the working copy despite not matching the sparsity patterns
> > > (i.e. to emulate being in the middle of a merge/rebase/cherry-pick)?
> >
> > Sure, I can add that. But after a quick test here, it seems that the
> > unmerged path doesn't have the SKIP_WORKTREE bit set. Is this how it
> > should be?
>
> Right, the merge machinery will clear the SKIP_WORKTREE bit when it
> writes out conflicted files. Also, any future 'git sparse-checkout'
> commands will see the unmerged entry and avoid marking it as
> SKIP_WORKTREE even though it doesn't match the sparsity patterns.
> Thus, grep doesn't have to do any special checking for whether the
> files are merged or not, and from your current implementation probably
> doesn't look like a special case at all -- you just check the
> SKIP_WORKTREE bit.
>
> However, I think the test still has value because the test enforces
> that other areas of the code (merge, sparse-checkout) don't break the
> invariants that grep is relying on. (I could see someone making a
> merge change that keeps the SKIP_WORKTREE bit accidentally set even
> though it writes the file out to the working tree, for example.)
> Sure, merge has some tests around that, so it might be viewed as
> slightly duplicative, but I see it as an interesting edge case that
> exercises whether the SKIP_WORKTREE bit should really be set and since
> grep expects a certain invariant about how that is handled, the
> testcase will help make sure our expectations aren't violated.
OK. I will add this test for the next version.
next prev parent reply other threads:[~2020-06-10 17:08 UTC|newest]
Thread overview: 123+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-03-24 6:04 [RFC PATCH 0/3] grep: honor sparse checkout and add option to ignore it Matheus Tavares
2020-03-24 6:11 ` [RFC PATCH 1/3] doc: grep: unify info on configuration variables Matheus Tavares
2020-03-24 7:57 ` Elijah Newren
2020-03-24 21:26 ` Junio C Hamano
2020-03-24 23:38 ` Matheus Tavares
2020-03-24 6:12 ` [RFC PATCH 2/3] grep: honor sparse checkout patterns Matheus Tavares
2020-03-24 7:15 ` Elijah Newren
2020-03-24 15:12 ` Derrick Stolee
2020-03-24 16:16 ` Elijah Newren
2020-03-24 17:02 ` Derrick Stolee
2020-03-24 23:01 ` Matheus Tavares Bernardino
2020-03-24 22:55 ` Matheus Tavares Bernardino
2020-04-21 2:10 ` Matheus Tavares Bernardino
2020-04-21 3:08 ` Elijah Newren
2020-04-22 12:08 ` Derrick Stolee
2020-04-23 6:09 ` Matheus Tavares Bernardino
2020-03-24 6:13 ` [RFC PATCH 3/3] grep: add option to ignore sparsity patterns Matheus Tavares
2020-03-24 7:54 ` Elijah Newren
2020-03-24 18:30 ` Junio C Hamano
2020-03-24 19:07 ` Elijah Newren
2020-03-25 20:18 ` Junio C Hamano
2020-03-30 3:23 ` Matheus Tavares Bernardino
2020-03-31 19:12 ` Elijah Newren
2020-03-31 20:02 ` Derrick Stolee
2020-04-27 17:15 ` Matheus Tavares Bernardino
2020-04-29 16:46 ` Elijah Newren
2020-04-29 17:21 ` Elijah Newren
2020-03-25 23:15 ` Matheus Tavares Bernardino
2020-03-26 6:02 ` Elijah Newren
2020-03-27 15:51 ` Junio C Hamano
2020-03-27 19:01 ` Elijah Newren
2020-03-30 1:12 ` Matheus Tavares Bernardino
2020-03-31 16:48 ` Elijah Newren
2020-05-10 0:41 ` [RFC PATCH v2 0/4] grep: honor sparse checkout and add option to ignore it Matheus Tavares
2020-05-10 0:41 ` [RFC PATCH v2 1/4] doc: grep: unify info on configuration variables Matheus Tavares
2020-05-10 0:41 ` [RFC PATCH v2 2/4] config: load the correct config.worktree file Matheus Tavares
2020-05-11 19:10 ` Junio C Hamano
2020-05-12 22:55 ` Matheus Tavares Bernardino
2020-05-12 23:22 ` Junio C Hamano
2020-05-10 0:41 ` [RFC PATCH v2 3/4] grep: honor sparse checkout patterns Matheus Tavares
2020-05-11 19:35 ` Junio C Hamano
2020-05-13 0:05 ` Matheus Tavares Bernardino
2020-05-13 0:17 ` Junio C Hamano
2020-05-21 7:26 ` Elijah Newren
2020-05-21 17:35 ` Matheus Tavares Bernardino
2020-05-21 17:52 ` Elijah Newren
2020-05-22 5:49 ` Matheus Tavares Bernardino
2020-05-22 14:26 ` Elijah Newren
2020-05-22 15:36 ` Elijah Newren
2020-05-22 20:54 ` Matheus Tavares Bernardino
2020-05-22 21:06 ` Elijah Newren
2020-06-10 11:40 ` Derrick Stolee
2020-06-10 16:22 ` Matheus Tavares Bernardino
2020-06-10 17:42 ` Derrick Stolee
2020-06-10 18:14 ` Matheus Tavares Bernardino
2020-06-10 20:12 ` Elijah Newren
2020-06-10 19:58 ` Elijah Newren
2020-05-21 7:36 ` Elijah Newren
2020-05-10 0:41 ` [RFC PATCH v2 4/4] config: add setting to ignore sparsity patterns in some cmds Matheus Tavares
2020-05-10 4:23 ` Matheus Tavares Bernardino
2020-05-21 17:18 ` Elijah Newren
2020-05-21 7:09 ` Elijah Newren
2020-05-28 1:12 ` [PATCH v3 0/5] grep: honor sparse checkout and add option to ignore it Matheus Tavares
2020-05-28 1:12 ` [PATCH v3 1/5] doc: grep: unify info on configuration variables Matheus Tavares
2020-05-28 1:13 ` [PATCH v3 2/5] t/helper/test-config: return exit codes consistently Matheus Tavares
2020-05-30 14:29 ` Elijah Newren
2020-06-01 4:36 ` Matheus Tavares Bernardino
2020-05-28 1:13 ` [PATCH v3 3/5] config: correctly read worktree configs in submodules Matheus Tavares
2020-05-30 14:49 ` Elijah Newren
2020-06-01 4:38 ` Matheus Tavares Bernardino
2020-05-28 1:13 ` [PATCH v3 4/5] grep: honor sparse checkout patterns Matheus Tavares
2020-05-30 15:48 ` Elijah Newren
2020-06-01 4:44 ` Matheus Tavares Bernardino
2020-06-03 2:38 ` Elijah Newren
2020-06-10 17:08 ` Matheus Tavares Bernardino [this message]
2020-05-28 1:13 ` [PATCH v3 5/5] config: add setting to ignore sparsity patterns in some cmds Matheus Tavares
2020-05-30 16:18 ` Elijah Newren
2020-06-01 4:45 ` Matheus Tavares Bernardino
2020-06-03 2:39 ` Elijah Newren
2020-06-10 21:15 ` Matheus Tavares Bernardino
2020-06-11 0:35 ` Elijah Newren
2020-06-12 15:44 ` [PATCH v4 0/6] grep: honor sparse checkout and add option to ignore it Matheus Tavares
2020-06-12 15:44 ` [PATCH v4 1/6] doc: grep: unify info on configuration variables Matheus Tavares
2020-06-12 15:45 ` [PATCH v4 2/6] t/helper/test-config: return exit codes consistently Matheus Tavares
2020-06-12 15:45 ` [PATCH v4 3/6] t/helper/test-config: facilitate addition of new cli options Matheus Tavares
2020-06-12 15:45 ` [PATCH v4 4/6] config: correctly read worktree configs in submodules Matheus Tavares
2020-06-16 19:13 ` Elijah Newren
2020-06-21 16:05 ` Matheus Tavares Bernardino
2020-09-01 2:41 ` Jonathan Nieder
2020-09-01 21:44 ` Matheus Tavares Bernardino
2020-06-12 15:45 ` [PATCH v4 5/6] grep: honor sparse checkout patterns Matheus Tavares
2020-06-12 15:45 ` [PATCH v4 6/6] config: add setting to ignore sparsity patterns in some cmds Matheus Tavares
2020-06-16 22:31 ` [PATCH v4 0/6] grep: honor sparse checkout and add option to ignore it Elijah Newren
2020-09-02 6:17 ` [PATCH v5 0/8] " Matheus Tavares
2020-09-02 6:17 ` [PATCH v5 1/8] doc: grep: unify info on configuration variables Matheus Tavares
2020-09-02 6:17 ` [PATCH v5 2/8] t1308-config-set: avoid false positives when using test-config Matheus Tavares
2020-09-02 6:57 ` Eric Sunshine
2020-09-02 16:16 ` Matheus Tavares Bernardino
2020-09-02 16:38 ` Eric Sunshine
2020-09-02 6:17 ` [PATCH v5 3/8] t/helper/test-config: be consistent with exit codes Matheus Tavares
2020-09-02 6:17 ` [PATCH v5 4/8] t/helper/test-config: check argc before accessing argv Matheus Tavares
2020-09-02 7:18 ` Eric Sunshine
2020-09-02 6:17 ` [PATCH v5 5/8] t/helper/test-config: unify exit labels Matheus Tavares
2020-09-02 7:30 ` Eric Sunshine
2020-09-02 6:17 ` [PATCH v5 6/8] config: correctly read worktree configs in submodules Matheus Tavares
2020-09-02 20:15 ` Jonathan Nieder
2020-09-09 13:04 ` Matheus Tavares Bernardino
2020-09-09 23:32 ` Jonathan Nieder
2020-09-02 6:17 ` [PATCH v5 7/8] grep: honor sparse checkout patterns Matheus Tavares
2020-09-02 6:17 ` [PATCH v5 8/8] config: add setting to ignore sparsity patterns in some cmds Matheus Tavares
2020-09-10 17:21 ` [PATCH v6 0/9] grep: honor sparse checkout and add option to ignore it Matheus Tavares
2020-09-10 17:21 ` [PATCH v6 1/9] doc: grep: unify info on configuration variables Matheus Tavares
2020-09-10 17:21 ` [PATCH v6 2/9] t1308-config-set: avoid false positives when using test-config Matheus Tavares
2020-09-10 17:21 ` [PATCH v6 3/9] t/helper/test-config: be consistent with exit codes Matheus Tavares
2020-09-10 17:21 ` [PATCH v6 4/9] t/helper/test-config: diagnose missing arguments Matheus Tavares
2020-09-10 17:21 ` [PATCH v6 5/9] t/helper/test-config: unify exit labels Matheus Tavares
2020-09-10 17:21 ` [PATCH v6 6/9] config: make do_git_config_sequence receive a 'struct repository' Matheus Tavares
2020-09-10 17:21 ` [PATCH v6 7/9] config: correctly read worktree configs in submodules Matheus Tavares
2020-09-10 17:21 ` [PATCH v6 8/9] grep: honor sparse checkout patterns Matheus Tavares
2020-09-10 17:21 ` [PATCH v6 9/9] config: add setting to ignore sparsity patterns in some cmds Matheus Tavares
2021-02-09 21:33 ` [PATCH v7] grep: honor sparse-checkout on working tree searches Matheus Tavares
2021-02-09 23:23 ` Junio C Hamano
2021-02-10 6:12 ` 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='CAHd-oW4bKG+MDCaDm+5Ha-+vvFy=LT9nnxCmQUre-BCtqWcd3Q@mail.gmail.com' \
--to=matheus.bernardino@usp.br \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
--cc=jonathantanmy@google.com \
--cc=newren@gmail.com \
--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).