git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
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 v2 1/5] Teach status options around showing ignored files
Date: Thu, 12 Oct 2017 11:49:04 +0900	[thread overview]
Message-ID: <xmqqy3oh13of.fsf@gitster.mtv.corp.google.com> (raw)
In-Reply-To: <20171011133504.15049-2-jamill@microsoft.com> (Jameson Miller's message of "Wed, 11 Oct 2017 09:35:00 -0400")

Jameson Miller <jameson.miller81@gmail.com> writes:

> This change teaches the status command more options to control which
> ignored files are reported independently of the which untracked files
> are reported by allowing the `--ignored` option to take additional
> arguments.  Currently, the shown ignored files are linked to how
> untracked files are reported. Both ignored and untracked files are
> controlled by arguments to `--untracked-files` option. This makes it
> impossible to show all untracked files individually, but show ignored
> "files and directories" (that is, ignored directories are shown as 1
> entry, instead of having all contents shown).

The description makes sense.  And it makes sense to show a directory
known to contain only ignored paths as just one entry, instead of
exploding that to individual files.

> Our application (Visual Studio) has a specific set of requirements
> about how it wants untracked / ignored files reported by git status.

This sentence does not read well.  VS has no obligation to read from
"git status", so there is no "specific set of requirements" that
make us care.  If the output from "status" is insufficient you could
be reading from "ls-files --ignored", for example, if you want more
details than "git status" gives you.

The sentence, and the paragraph that immediately follows it, need a
serious attitude adjustment ;-), even though it is good as read as
an explanation of what the proposed output wants to show.

> The reason for controlling these behaviors separately is that there
> can be a significant performance impact to scanning the contents of
> excluded directories. Additionally, knowing whether a directory
> explicitly matches an exclude pattern can help the application make
> decisions about how to handle the directory. If an ignored directory
> itself matches an exclude pattern, then the application will know that
> any files underneath the directory must be ignored as well.

While the above description taken standalone makes sense, doesn't
the "we want all paths listed, without abbreviated to the directory
and requiring the reader to infer from the fact that aidrectory is
shown that everything in it are ignored" the log message stated
earlier contradict another change you earlier sent, that avoids
scanning a directory that is known to be completely untracked
(i.e. no path under it in the index) and return early once an
untracked file is found in it?

> Signed-off-by: Jameson Miller <jamill@microsoft.com>
> ---
>  builtin/commit.c | 31 +++++++++++++++++++++++++------
>  dir.c            | 24 ++++++++++++++++++++++++
>  dir.h            |  3 ++-
>  wt-status.c      | 11 ++++++++---
>  wt-status.h      |  8 +++++++-
>  5 files changed, 66 insertions(+), 11 deletions(-)
>
> diff --git a/builtin/commit.c b/builtin/commit.c
> index d75b3805ea..98d84d0277 100644
> --- a/builtin/commit.c
> +++ b/builtin/commit.c
> @@ -118,7 +118,7 @@ static int edit_flag = -1; /* unspecified */
>  static int quiet, verbose, no_verify, allow_empty, dry_run, renew_authorship;
>  static int config_commit_verbose = -1; /* unspecified */
>  static int no_post_rewrite, allow_empty_message;
> -static char *untracked_files_arg, *force_date, *ignore_submodule_arg;
> +static char *untracked_files_arg, *force_date, *ignore_submodule_arg, *ignored_arg;
>  static char *sign_commit;
>  
>  /*
> @@ -139,7 +139,7 @@ static const char *cleanup_arg;
>  static enum commit_whence whence;
>  static int sequencer_in_use;
>  static int use_editor = 1, include_status = 1;
> -static int show_ignored_in_status, have_option_m;
> +static int have_option_m;
>  static struct strbuf message = STRBUF_INIT;
>  
>  static enum wt_status_format status_format = STATUS_FORMAT_UNSPECIFIED;
> @@ -1075,6 +1075,19 @@ static const char *find_author_by_nickname(const char *name)
>  	die(_("--author '%s' is not 'Name <email>' and matches no existing author"), name);
>  }
>  
> +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" },
>  		{ 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" },
> @@ -1383,8 +1398,12 @@ int cmd_status(int argc, const char **argv, const char *prefix)
>  	finalize_deferred_config(&s);
>  
>  	handle_untracked_files_arg(&s);
> -	if (show_ignored_in_status)
> -		s.show_ignored_files = 1;
> +	handle_ignored_arg(&s);
> +
> +	if (s.show_ignored_mode == SHOW_MATCHING_IGNORED &&
> +	    s.show_untracked_files == SHOW_NO_UNTRACKED_FILES)
> +		die(_("Unsupported combination of ignored and untracked-files arguments"));
> +
>  	parse_pathspec(&s.pathspec, 0,
>  		       PATHSPEC_PREFER_FULL,
>  		       prefix, argv);
> 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)) {
> +
> +			/*
> +			 * 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).
> +			 */
> +			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;
> +		}
>  		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;
> 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) {
>  		dir.flags |= DIR_SHOW_IGNORED_TOO;
> -	else
> +
> +		if (s->show_ignored_mode == SHOW_MATCHING_IGNORED)
> +			dir.flags |= DIR_SHOW_IGNORED_TOO_MODE_MATCHING;
> +	} 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)
>  			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/wt-status.h b/wt-status.h
> index 64f4d33ea1..fe27b465e2 100644
> --- a/wt-status.h
> +++ b/wt-status.h
> @@ -27,6 +27,12 @@ enum untracked_status_type {
>  	SHOW_ALL_UNTRACKED_FILES
>  };
>  
> +enum show_ignored_type {
> +	SHOW_NO_IGNORED,
> +	SHOW_TRADITIONAL_IGNORED,
> +	SHOW_MATCHING_IGNORED,
> +};
> +
>  /* from where does this commit originate */
>  enum commit_whence {
>  	FROM_COMMIT,     /* normal */
> @@ -70,7 +76,7 @@ struct wt_status {
>  	int display_comment_prefix;
>  	int relative_paths;
>  	int submodule_summary;
> -	int show_ignored_files;
> +	enum show_ignored_type show_ignored_mode;
>  	enum untracked_status_type show_untracked_files;
>  	const char *ignore_submodule_arg;
>  	char color_palette[WT_STATUS_MAXSLOT][COLOR_MAXLEN];

  reply	other threads:[~2017-10-12  2:49 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 [this message]
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
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=xmqqy3oh13of.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).