From: Junio C Hamano <gitster@pobox.com>
To: Jameson Miller <jameson.miller81@gmail.com>
Cc: bmwill@google.com, git@vger.kernel.org, jamill@microsoft.com,
peff@peff.net, sbeller@google.com
Subject: Re: [PATCH v3 1/4] status: add option to show ignored files differently
Date: Mon, 23 Oct 2017 12:34:41 +0900 [thread overview]
Message-ID: <xmqqzi8ibkq6.fsf@gitster.mtv.corp.google.com> (raw)
In-Reply-To: <20171019160601.9382-2-jamill@microsoft.com> (Jameson Miller's message of "Thu, 19 Oct 2017 12:05:58 -0400")
Jameson Miller <jameson.miller81@gmail.com> writes:
> Teach the status command more flexibility in how ignored files are
> reported. Currently, the reporting of ignored files and untracked
> files are linked. You cannot control how ignored files are reported
> independently of how untracked files are reported (i.e. `all` vs
> `normal`). This makes it impossible to show untracked files with the
> `all` option, but show ignored files with the `normal` option.
>
> This work 1) adds the ability to control the reporting of ignored
> files independently of untracked files and 2) introduces the concept
> of status reporting ignored paths that explicitly match an ignored
> pattern. ...
>
> When status is set to report matching ignored files, it has the
> following behavior. Ignored files and directories that explicitly
> match an exclude pattern are reported. If an ignored directory matches
> an exclude pattern, then the path of the directory is returned. If a
> directory does not match an exclude pattern, but all of its contents
> are ignored, then the contained files are reported instead of the
> directory.
Thanks for an updated log message. Very nicely explained.
> +static void handle_ignored_arg(struct wt_status *s)
> +{
> + if (!ignored_arg)
> + ; /* default already initialized */
> + else if (!strcmp(ignored_arg, "traditional"))
> + s->show_ignored_mode = SHOW_TRADITIONAL_IGNORED;
> + else if (!strcmp(ignored_arg, "no"))
> + s->show_ignored_mode = SHOW_NO_IGNORED;
> + else if (!strcmp(ignored_arg, "matching"))
> + s->show_ignored_mode = SHOW_MATCHING_IGNORED;
> + else
> + die(_("Invalid ignored mode '%s'"), ignored_arg);
> +}
>
> static void handle_untracked_files_arg(struct wt_status *s)
> {
> @@ -1363,8 +1376,10 @@ int cmd_status(int argc, const char **argv, const char *prefix)
> N_("mode"),
> N_("show untracked files, optional modes: all, normal, no. (Default: all)"),
> PARSE_OPT_OPTARG, NULL, (intptr_t)"all" },
> - OPT_BOOL(0, "ignored", &show_ignored_in_status,
> - N_("show ignored files")),
> + { OPTION_STRING, 0, "ignored", &ignored_arg,
> + N_("mode"),
> + N_("show ignored files, optional modes: traditional, matching, no. (Default: traditional)"),
> + PARSE_OPT_OPTARG, NULL, (intptr_t)"traditional" },
We used to use "show_ignored_in_status" variable that can be either
0 (when no --ignored is given, or an explicit --no-ignored is given,
on the command line) or 1 (when --ignored is given on the command
line).
We still allow "--ignored" without value for backward compatibility,
but
$ git status -uall --ignored \*.c
may trigger die() from handle_ignored_arg(). I wonder if this is
something we care about.
... goes and digs ...
OK, because of PARSE_OPT_OPTARG, the above is safe; "*.c" is not
mistaken as the value given to the --ignored option. It also means
that
$ git status -uno --ignored matching
would not mean what a naïve reader may expect and does not error
out, even though
$ git status -uno --ignored=matching
would. Which is something we eventually might care about, but that
is how parse-options PARSE_OPT_OPTARG works, and I consider "fixing"
it is totally out of the scope of this series (e.g. the next option
"--ignore-submodules" below shares exactly the same issue).
> { OPTION_STRING, 0, "ignore-submodules", &ignore_submodule_arg, N_("when"),
> N_("ignore changes to submodules, optional when: all, dirty, untracked. (Default: all)"),
> PARSE_OPT_OPTARG, NULL, (intptr_t)"all" },
> diff --git a/wt-status.c b/wt-status.c
> index 6f730ee8f2..8301c84946 100644
> --- a/wt-status.c
> +++ b/wt-status.c
> @@ -660,10 +660,15 @@ static void wt_status_collect_untracked(struct wt_status *s)
> if (s->show_untracked_files != SHOW_ALL_UNTRACKED_FILES)
> dir.flags |=
> DIR_SHOW_OTHER_DIRECTORIES | DIR_HIDE_EMPTY_DIRECTORIES;
> - if (s->show_ignored_files)
> + if (s->show_ignored_mode) {
Now we no longer use show_ignored_files that was a boolean yes/no,
and instead use an enum show_ignored_mode, we'd better spell this
out like so:
if (s->show_ignored_mode == SHOW_NO_IGNORED) {
> dir.flags |= DIR_SHOW_IGNORED_TOO;
> - else
> +
> + if (s->show_ignored_mode == SHOW_MATCHING_IGNORED)
> + dir.flags |= DIR_SHOW_IGNORED_TOO_MODE_MATCHING;
Mental note: the old "show_ignored_files is true" case is now split
into two, i.e. traditional vs matching. When matching is used,
dir.flags gets a new bit, i.e. DIR_SHOW_IGNORED_TOO_MODE_MATCHING.
Lack of that bit means dir.c helpers should behave as before.
> + } else {
> dir.untracked = the_index.untracked;
> + }
> +
> setup_standard_excludes(&dir);
>
> fill_directory(&dir, &the_index, &s->pathspec);
> @@ -1621,7 +1626,7 @@ static void wt_longstatus_print(struct wt_status *s)
> }
> if (s->show_untracked_files) {
> wt_longstatus_print_other(s, &s->untracked, _("Untracked files"), "add");
> - if (s->show_ignored_files)
> + if (s->show_ignored_mode)
Likewise, i.e.
if (s->show_ignored_mode == SHOW_NO_IGNORED)
> wt_longstatus_print_other(s, &s->ignored, _("Ignored files"), "add -f");
> if (advice_status_u_option && 2000 < s->untracked_in_ms) {
> status_printf_ln(s, GIT_COLOR_NORMAL, "%s", "");
> diff --git a/dir.c b/dir.c
> index 1d17b800cf..b9af87eca9 100644
> --- a/dir.c
> +++ b/dir.c
> @@ -1389,6 +1389,30 @@ static enum path_treatment treat_directory(struct dir_struct *dir,
> case index_nonexistent:
> if (dir->flags & DIR_SHOW_OTHER_DIRECTORIES)
> break;
> + if (exclude &&
> + (dir->flags & DIR_SHOW_IGNORED_TOO) &&
> + (dir->flags & DIR_SHOW_IGNORED_TOO_MODE_MATCHING)) {
It somehow feels superfluous to check both TOO and TOO_MODE_MATCHING
here (IOW, wouldn't the latter be superset of the former? Is there
a valid case where the latter is set but not the former?), but it is
perhaps OK as belt-and-suspenders sanity.
> + /*
> + * This is an excluded directory and we are
> + * showing ignored paths that match an exclude
> + * pattern. (e.g. show directory as ignored
> + * only if it matches an exclude pattern).
> + * This path will either be 'path_excluded`
> + * (if we are showing empty directories or if
> + * the directory is not empty), or will be
> + * 'path_none' (empty directory, and we are
> + * not showing empty directories).
> + */
The only caller of treat_directory() calls us when the path we are
seeing (sans trailing '/' that was added immediately before we were
called) passed "is_excluded()" check in the caller, so at this
point, the directory in question is being excluded because it
matched an exclude pattern. IOW, if we are a/b/c, a pattern such as
"a/b/*" may have said we are excluded and made the call to this
function. Do we need to ensure that a pattern like "a/*" is not
what excluded us? Or IGNORED_TOO_MODE_MATCHING also should show
"a/b/c" when a pattern "a/*" matched it as "ignored because it
matched an exclude pattern"?
> + if (!(dir->flags & DIR_HIDE_EMPTY_DIRECTORIES))
> + return path_excluded;
> +
> + if (read_directory_recursive(dir, istate, dirname, len,
> + untracked, 1, 1, pathspec) == path_excluded)
> + return path_excluded;
> +
> + return path_none;
> + }
... I guess we won't get into such a situation because this codepath
will return only _excluded or _none when we are handing "a/b" which
is already excluded "a/*", and does not cause the caller to recurse
into "a/b/c" in the first place. So the updated code looks fine.
> if (!(dir->flags & DIR_NO_GITLINKS)) {
> unsigned char sha1[20];
> if (resolve_gitlink_ref(dirname, "HEAD", sha1) == 0)
> diff --git a/dir.h b/dir.h
> index e3717055d1..57b0943dae 100644
> --- a/dir.h
> +++ b/dir.h
> @@ -152,7 +152,8 @@ struct dir_struct {
> DIR_COLLECT_IGNORED = 1<<4,
> DIR_SHOW_IGNORED_TOO = 1<<5,
> DIR_COLLECT_KILLED_ONLY = 1<<6,
> - DIR_KEEP_UNTRACKED_CONTENTS = 1<<7
> + DIR_KEEP_UNTRACKED_CONTENTS = 1<<7,
> + DIR_SHOW_IGNORED_TOO_MODE_MATCHING = 1<<8
> } flags;
> struct dir_entry **entries;
> struct dir_entry **ignored;
next prev parent reply other threads:[~2017-10-23 3:34 UTC|newest]
Thread overview: 49+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-10-05 20:54 [PATCH 0/6] Teach Status options around showing ignored files jameson.miller81
2017-10-05 20:54 ` [PATCH 1/6] Teach status " jameson.miller81
2017-10-05 20:54 ` [PATCH 2/6] Update documentation for new directory and status logic jameson.miller81
2017-10-05 21:51 ` Stefan Beller
2017-10-05 20:54 ` [PATCH 3/6] Add tests for git status `--ignored=matching` jameson.miller81
2017-10-05 21:59 ` Stefan Beller
2017-10-05 20:54 ` [PATCH 4/6] Expand support for ignored arguments on status jameson.miller81
2017-10-05 20:54 ` [PATCH 5/6] Add tests around status handling of ignored arguments jameson.miller81
2017-10-05 20:54 ` [PATCH 6/6] Handle unsupported combination status arguments jameson.miller81
2017-10-05 22:08 ` Stefan Beller
2017-10-05 21:16 ` [PATCH 0/6] Teach Status options around showing ignored files Jonathan Nieder
2017-10-05 21:22 ` Jameson Miller
2017-10-11 13:34 ` [PATCH v2 0/5] " Jameson Miller
2017-10-11 13:35 ` [PATCH v2 1/5] Teach status " Jameson Miller
2017-10-12 2:49 ` Junio C Hamano
2017-10-12 20:15 ` Jameson Miller
2017-10-11 13:35 ` [PATCH v2 2/5] Update documentation for new directory and status logic Jameson Miller
2017-10-12 2:55 ` Junio C Hamano
2017-10-12 20:54 ` Jameson Miller
2017-10-13 0:42 ` Junio C Hamano
2017-10-11 13:35 ` [PATCH v2 3/5] Add tests for git status `--ignored=matching` Jameson Miller
2017-10-11 13:35 ` [PATCH v2 4/5] Expand support for ignored arguments on status Jameson Miller
2017-10-12 3:58 ` Junio C Hamano
2017-10-11 13:35 ` [PATCH v2 5/5] Add tests around status handling of ignored arguments Jameson Miller
2017-10-12 4:06 ` Junio C Hamano
2017-10-12 20:16 ` Jameson Miller
2017-10-13 0:49 ` Junio C Hamano
2017-10-12 2:33 ` [PATCH v2 0/5] Teach Status options around showing ignored files Junio C Hamano
2017-10-12 20:20 ` Jameson Miller
2017-10-19 16:05 ` [PATCH v3 0/4] status: add option to show ignored files differently Jameson Miller
2017-10-19 16:05 ` [PATCH v3 1/4] " Jameson Miller
2017-10-23 3:34 ` Junio C Hamano [this message]
2017-10-19 16:05 ` [PATCH v3 2/4] status: report matching ignored and normal untracked Jameson Miller
2017-10-19 16:06 ` [PATCH v3 3/4] status: document options to show matching ignored files Jameson Miller
2017-10-19 16:06 ` [PATCH v3 4/4] status: test --ignored=matching Jameson Miller
2017-10-19 19:33 ` Stefan Beller
2017-10-22 2:10 ` Junio C Hamano
2017-10-23 4:46 ` [PATCH v3 0/4] status: add option to show ignored files differently Junio C Hamano
2017-10-23 17:05 ` [PATCH v4 " Jameson Miller
2017-10-23 17:05 ` [PATCH v4 1/4] " Jameson Miller
2017-10-23 17:05 ` [PATCH v4 2/4] status: report matching ignored and normal untracked Jameson Miller
2017-10-23 17:05 ` [PATCH v4 3/4] status: document options to show matching ignored files Jameson Miller
2017-10-23 17:05 ` [PATCH v4 4/4] status: test ignored modes Jameson Miller
2017-10-30 17:21 ` [PATCH v5 0/4] status: add option to show ignored files differently jameson.miller81
2017-10-30 17:21 ` [PATCH v5 1/4] " jameson.miller81
2017-10-30 17:21 ` [PATCH v5 2/4] status: report matching ignored and normal untracked jameson.miller81
2017-10-30 17:21 ` [PATCH v5 3/4] status: document options to show matching ignored files jameson.miller81
2017-10-30 17:21 ` [PATCH v5 4/4] status: test ignored modes jameson.miller81
2017-10-31 3:16 ` [PATCH v5 0/4] status: add option to show ignored files differently 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
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=xmqqzi8ibkq6.fsf@gitster.mtv.corp.google.com \
--to=gitster@pobox.com \
--cc=bmwill@google.com \
--cc=git@vger.kernel.org \
--cc=jameson.miller81@gmail.com \
--cc=jamill@microsoft.com \
--cc=peff@peff.net \
--cc=sbeller@google.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.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
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 read-only IMAP folder(s) and NNTP newsgroup(s).