[PATCH 0/9] new git check-ignore sub-command

[PATCH 0/9] new git check-ignore sub-command

[Adam Spiers]

I was browsing stackoverflow the other day and came across this question:

    http://stackoverflow.com/questions/12144633/which-gitignore-rule-is-ignoring-my-file/

A quick google revealed this thread from 2009:

    http://thread.gmane.org/gmane.comp.version-control.git/108671/focus=108815

where Junio and Jeff discussed the possibility of adding a new `git
check-ignore' subcommand somewhat analogous to the existing `git
check-attr', and suggested the beginnings of an implementation.  It
struck me that it might not be too hard to follow these ideas to their
natural conclusion, so I decided it would make a fun project :-)

The following series of patches is the outcome.  I am completely new
to git hacking, so whilst I have tried very hard to follow all the
conventions and documented guidelines, please go easy on me if there
are any glaring errors ;-)  However, the added test suite should cover
the new code paths thoroughly, and I also ran check-ignore through
valgrind and made some improvements accordingly, so hopefully it's
pretty near the mark.

I have a question and some comments about my current patch series.

Firstly, I re-used pathspec-handling code from builtin/add.c, so I
moved it to a new pathspec.c file.  It looks like setup.c might have
been a better candidate, but that library is already a fairly large
collection of apparently loosely associated things, so I wasn't sure.
According to the comments, get_pathspec() is due to be superceded by
the "struct pathspec" interface, so perhaps it would make sense to
split setup.c up into pathspec.c and one or two other files so as to
move towards a clean demarcation of this new API?

Secondly, in the course of trying to understand the code base, my
little brain got confused and I noticed a few areas where I thought
there was potential to make things a bit clearer.  So some of my
commits are janitorial in nature.

Thirdly, currently the new sub-command hardly looks at the cache.
This is partially because it doesn't need to in the most common use
case (i.e. user is confused about why files are/aren't being ignored).
It's also because this whole project took a lot longer than I
expected, so I'm running out of time :-) Perhaps someone can add this
in the future if it's needed.  Right now the cache is only used to
prevent recursing into submodules.

Thanks,
Adam

Adam Spiers (9):
  Update directory listing API doc to match code
  Improve documentation and comments regarding directory traversal API
  Rename cryptic 'which' variable to more consistent name
  Refactor excluded_from_list
  Refactor excluded and path_excluded
  For each exclude pattern, store information about where it came from
  Extract some useful pathspec handling code from builtin/add.c into a
    library
  Provide free_directory() for reclaiming dir_struct memory
  Add git-check-ignores

 .gitignore                                        |   1 +
 Documentation/git-check-ignore.txt                |  58 +++++
 Documentation/gitignore.txt                       |   6 +-
 Documentation/technical/api-directory-listing.txt |  23 +-
 Makefile                                          |   3 +
 builtin.h                                         |   1 +
 builtin/add.c                                     |  84 +-----
 builtin/check-ignore.c                            | 150 +++++++++++
 builtin/clean.c                                   |   2 +-
 builtin/ls-files.c                                |   3 +-
 command-list.txt                                  |   1 +
 contrib/completion/git-completion.bash            |   1 +
 dir.c                                             | 183 ++++++++++---
 dir.h                                             |  37 ++-
 git.c                                             |   1 +
 pathspec.c                                        |  87 +++++++
 pathspec.h                                        |   6 +
 t/t0007-ignores.sh                                | 301 ++++++++++++++++++++++
 18 files changed, 811 insertions(+), 137 deletions(-)
 create mode 100644 Documentation/git-check-ignore.txt
 create mode 100644 builtin/check-ignore.c
 create mode 100644 pathspec.c
 create mode 100644 pathspec.h
 create mode 100755 t/t0007-ignores.sh

-- 
1.7.12.155.ge5750d5.dirty

[PATCH 1/9] Update directory listing API doc to match code

[Adam Spiers]

7c4c97c0ac turned the flags in struct dir_struct into a single bitfield
variable, but forgot to update this document.

Signed-off-by: Adam Spiers <git@adamspiers.org>
---
 Documentation/technical/api-directory-listing.txt | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/Documentation/technical/api-directory-listing.txt b/Documentation/technical/api-directory-listing.txt
index add6f43..0356d25 100644
--- a/Documentation/technical/api-directory-listing.txt
+++ b/Documentation/technical/api-directory-listing.txt
@@ -17,24 +17,24 @@ options are:
 	The name of the file to be read in each directory for excluded
 	files (typically `.gitignore`).
 
-`collect_ignored`::
+`flags`::
 
-	Include paths that are to be excluded in the result.
+	A bit-field of options:
 
-`show_ignored`::
+`DIR_SHOW_IGNORED`:::
 
 	The traversal is for finding just ignored files, not unignored
 	files.
 
-`show_other_directories`::
+`DIR_SHOW_OTHER_DIRECTORIES`:::
 
 	Include a directory that is not tracked.
 
-`hide_empty_directories`::
+`DIR_HIDE_EMPTY_DIRECTORIES`:::
 
 	Do not include a directory that is not tracked and is empty.
 
-`no_gitlinks`::
+`DIR_NO_GITLINKS`:::
 
 	If set, recurse into a directory that looks like a git
 	directory.  Otherwise it is shown as a directory.
-- 
1.7.12.155.ge5750d5.dirty

[PATCH 2/9] Improve documentation and comments regarding directory traversal API

[Adam Spiers]

Signed-off-by: Adam Spiers <git@adamspiers.org>
---
 Documentation/technical/api-directory-listing.txt |  9 +++++---
 dir.c                                             |  8 ++++++-
 dir.h                                             | 26 +++++++++++++++++++++--
 3 files changed, 37 insertions(+), 6 deletions(-)

diff --git a/Documentation/technical/api-directory-listing.txt b/Documentation/technical/api-directory-listing.txt
index 0356d25..944fc39 100644
--- a/Documentation/technical/api-directory-listing.txt
+++ b/Documentation/technical/api-directory-listing.txt
@@ -9,8 +9,11 @@ Data structure
 --------------
 
 `struct dir_struct` structure is used to pass directory traversal
-options to the library and to record the paths discovered.  The notable
-options are:
+options to the library and to record the paths discovered.  A single
+`struct dir_struct` is used regardless of whether or not the traversal
+recursively descends into subdirectories.
+
+The notable options are:
 
 `exclude_per_dir`::
 
@@ -39,7 +42,7 @@ options are:
 	If set, recurse into a directory that looks like a git
 	directory.  Otherwise it is shown as a directory.
 
-The result of the enumeration is left in these fields::
+The result of the enumeration is left in these fields:
 
 `entries[]`::
 
diff --git a/dir.c b/dir.c
index 240bf0c..c9f341a 100644
--- a/dir.c
+++ b/dir.c
@@ -2,6 +2,8 @@
  * This handles recursive filename detection with exclude
  * files, index knowledge etc..
  *
+ * See Documentation/technical/api-directory-listing.txt
+ *
  * Copyright (C) Linus Torvalds, 2005-2006
  *		 Junio Hamano, 2005-2006
  */
@@ -449,6 +451,10 @@ void add_excludes_from_file(struct dir_struct *dir, const char *fname)
 		die("cannot use %s as an exclude file", fname);
 }
 
+/*
+ * Loads the per-directory exclude list for the substring of base
+ * which has a char length of baselen.
+ */
 static void prep_exclude(struct dir_struct *dir, const char *base, int baselen)
 {
 	struct exclude_list *el;
@@ -459,7 +465,7 @@ static void prep_exclude(struct dir_struct *dir, const char *base, int baselen)
 	    (baselen + strlen(dir->exclude_per_dir) >= PATH_MAX))
 		return; /* too long a path -- ignore */
 
-	/* Pop the ones that are not the prefix of the path being checked. */
+	/* Pop the directories that are not the prefix of the path being checked. */
 	el = &dir->exclude_list[EXC_DIRS];
 	while ((stk = dir->exclude_stack) != NULL) {
 		if (stk->baselen <= baselen &&
diff --git a/dir.h b/dir.h
index 893465a..a226fbc 100644
--- a/dir.h
+++ b/dir.h
@@ -1,6 +1,8 @@
 #ifndef DIR_H
 #define DIR_H
 
+/* See Documentation/technical/api-directory-listing.txt */
+
 #include "strbuf.h"
 
 struct dir_entry {
@@ -12,6 +14,12 @@ struct dir_entry {
 #define EXC_FLAG_ENDSWITH 4
 #define EXC_FLAG_MUSTBEDIR 8
 
+/*
+ * Each .gitignore file will be parsed into patterns which are then
+ * appended to the relevant exclude_list (either EXC_DIRS or
+ * EXC_FILE).  exclude_lists are also used to represent the list of
+ * --exclude values passed via CLI args (EXC_CMDL).
+ */
 struct exclude_list {
 	int nr;
 	int alloc;
@@ -26,9 +34,15 @@ struct exclude_list {
 	} **excludes;
 };
 
+/*
+ * The contents of the per-directory exclude files are lazily read on
+ * demand and then cached in memory, one per exclude_stack struct, in
+ * order to avoid opening and parsing each one every time that
+ * directory is traversed.
+ */
 struct exclude_stack {
-	struct exclude_stack *prev;
-	char *filebuf;
+	struct exclude_stack *prev; /* the struct exclude_stack for the parent directory */
+	char *filebuf; /* remember pointer to per-directory exclude file contents so we can free() */
 	int baselen;
 	int exclude_ix;
 };
@@ -59,6 +73,14 @@ struct dir_struct {
 #define EXC_DIRS 1
 #define EXC_FILE 2
 
+	/*
+	 * Temporary variables which are used during loading of the
+	 * per-directory exclude lists.
+	 *
+	 * exclude_stack points to the top of the exclude_stack, and
+	 * basebuf contains the full path to the current
+	 * (sub)directory in the traversal.
+	 */
 	struct exclude_stack *exclude_stack;
 	char basebuf[PATH_MAX];
 };
-- 
1.7.12.155.ge5750d5.dirty

[PATCH 3/9] Rename cryptic 'which' variable to more consistent name

[Adam Spiers]

'el' is only *slightly* less cryptic, but is already used as the
variable name for a struct exclude_list pointer in numerous other
places, so this reduces the number of cryptic variable names in use by
one :-)

Signed-off-by: Adam Spiers <git@adamspiers.org>
---
 dir.c | 10 +++++-----
 dir.h |  4 ++--
 2 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/dir.c b/dir.c
index c9f341a..57a5d11 100644
--- a/dir.c
+++ b/dir.c
@@ -311,7 +311,7 @@ static int no_wildcard(const char *string)
 }
 
 void add_exclude(const char *string, const char *base,
-		 int baselen, struct exclude_list *which)
+		 int baselen, struct exclude_list *el)
 {
 	struct exclude *x;
 	size_t len;
@@ -346,8 +346,8 @@ void add_exclude(const char *string, const char *base,
 	x->nowildcardlen = simple_length(string);
 	if (*string == '*' && no_wildcard(string+1))
 		x->flags |= EXC_FLAG_ENDSWITH;
-	ALLOC_GROW(which->excludes, which->nr + 1, which->alloc);
-	which->excludes[which->nr++] = x;
+	ALLOC_GROW(el->excludes, el->nr + 1, el->alloc);
+	el->excludes[el->nr++] = x;
 }
 
 static void *read_skip_worktree_file_from_index(const char *path, size_t *size)
@@ -389,7 +389,7 @@ int add_excludes_from_file_to_list(const char *fname,
 				   const char *base,
 				   int baselen,
 				   char **buf_p,
-				   struct exclude_list *which,
+				   struct exclude_list *el,
 				   int check_index)
 {
 	struct stat st;
@@ -436,7 +436,7 @@ int add_excludes_from_file_to_list(const char *fname,
 		if (buf[i] == '\n') {
 			if (entry != buf + i && entry[0] != '#') {
 				buf[i - (i && buf[i-1] == '\r')] = 0;
-				add_exclude(entry, base, baselen, which);
+				add_exclude(entry, base, baselen, el);
 			}
 			entry = buf + i + 1;
 		}
diff --git a/dir.h b/dir.h
index a226fbc..549a187 100644
--- a/dir.h
+++ b/dir.h
@@ -117,10 +117,10 @@ extern int path_excluded(struct path_exclude_check *, const char *, int namelen,
 
 
 extern int add_excludes_from_file_to_list(const char *fname, const char *base, int baselen,
-					  char **buf_p, struct exclude_list *which, int check_index);
+					  char **buf_p, struct exclude_list *el, int check_index);
 extern void add_excludes_from_file(struct dir_struct *, const char *fname);
 extern void add_exclude(const char *string, const char *base,
-			int baselen, struct exclude_list *which);
+			int baselen, struct exclude_list *el);
 extern void free_excludes(struct exclude_list *el);
 extern int file_exists(const char *);
 
-- 
1.7.12.155.ge5750d5.dirty

Re: [PATCH 3/9] Rename cryptic 'which' variable to more consistent name

[Junio C Hamano]

Adam Spiers <git@adamspiers.org> writes:

> 'el' is only *slightly* less cryptic, but is already used as the
> variable name for a struct exclude_list pointer in numerous other
> places, so this reduces the number of cryptic variable names in use by
> one :-)

The name originally meant to mean "to which element of the array
dir_struct.exclude_list[] are we adding this entry?" but I agree
"el" that stands for ExcludeList would be a better name.

Often we use "el" (or "elem") for elements of an iterable we are
iterating on in a loop, and the name of the iterable does not have
to be EsomethingLsomething, by the way.  Because no existing use of
"el" in this file is of that kind, I do not think this change
introduces new confusion to the code.

> Signed-off-by: Adam Spiers <git@adamspiers.org>
> ---
>  dir.c | 10 +++++-----
>  dir.h |  4 ++--
>  2 files changed, 7 insertions(+), 7 deletions(-)
>
> diff --git a/dir.c b/dir.c
> index c9f341a..57a5d11 100644
> --- a/dir.c
> +++ b/dir.c
> @@ -311,7 +311,7 @@ static int no_wildcard(const char *string)
>  }
>  
>  void add_exclude(const char *string, const char *base,
> -		 int baselen, struct exclude_list *which)
> +		 int baselen, struct exclude_list *el)
>  {
>  	struct exclude *x;
>  	size_t len;
> @@ -346,8 +346,8 @@ void add_exclude(const char *string, const char *base,
>  	x->nowildcardlen = simple_length(string);
>  	if (*string == '*' && no_wildcard(string+1))
>  		x->flags |= EXC_FLAG_ENDSWITH;
> -	ALLOC_GROW(which->excludes, which->nr + 1, which->alloc);
> -	which->excludes[which->nr++] = x;
> +	ALLOC_GROW(el->excludes, el->nr + 1, el->alloc);
> +	el->excludes[el->nr++] = x;
>  }
>  
>  static void *read_skip_worktree_file_from_index(const char *path, size_t *size)
> @@ -389,7 +389,7 @@ int add_excludes_from_file_to_list(const char *fname,
>  				   const char *base,
>  				   int baselen,
>  				   char **buf_p,
> -				   struct exclude_list *which,
> +				   struct exclude_list *el,
>  				   int check_index)
>  {
>  	struct stat st;
> @@ -436,7 +436,7 @@ int add_excludes_from_file_to_list(const char *fname,
>  		if (buf[i] == '\n') {
>  			if (entry != buf + i && entry[0] != '#') {
>  				buf[i - (i && buf[i-1] == '\r')] = 0;
> -				add_exclude(entry, base, baselen, which);
> +				add_exclude(entry, base, baselen, el);
>  			}
>  			entry = buf + i + 1;
>  		}
> diff --git a/dir.h b/dir.h
> index a226fbc..549a187 100644
> --- a/dir.h
> +++ b/dir.h
> @@ -117,10 +117,10 @@ extern int path_excluded(struct path_exclude_check *, const char *, int namelen,
>  
>  
>  extern int add_excludes_from_file_to_list(const char *fname, const char *base, int baselen,
> -					  char **buf_p, struct exclude_list *which, int check_index);
> +					  char **buf_p, struct exclude_list *el, int check_index);
>  extern void add_excludes_from_file(struct dir_struct *, const char *fname);
>  extern void add_exclude(const char *string, const char *base,
> -			int baselen, struct exclude_list *which);
> +			int baselen, struct exclude_list *el);
>  extern void free_excludes(struct exclude_list *el);
>  extern int file_exists(const char *);

[PATCH 4/9] Refactor excluded_from_list

[Adam Spiers]

The excluded function uses a new helper function called
exclude_from_list_1() to perform the inner loop over all of the
exclude patterns.  The helper just tells us whether the path is
included, excluded, or undecided.

However, it may be useful to know _which_ pattern was
triggered.  So let's pass out the entire exclude match,
which contains the status information we were already
passing out.

Further patches can make use of this.

This is a modified forward port of a patch from 2009 by Jeff King:
http://article.gmane.org/gmane.comp.version-control.git/108815

Signed-off-by: Adam Spiers <git@adamspiers.org>
---
 dir.c | 40 +++++++++++++++++++++++++++++-----------
 1 file changed, 29 insertions(+), 11 deletions(-)

diff --git a/dir.c b/dir.c
index 57a5d11..3a532d5 100644
--- a/dir.c
+++ b/dir.c
@@ -509,22 +509,24 @@ static void prep_exclude(struct dir_struct *dir, const char *base, int baselen)
 	dir->basebuf[baselen] = '\0';
 }
 
-/* Scan the list and let the last match determine the fate.
- * Return 1 for exclude, 0 for include and -1 for undecided.
+/*
+ * Scan the given exclude list in reverse to see whether pathname
+ * should be ignored.  The first match (i.e. the last on the list), if
+ * any, determines the fate.  Returns the exclude_list element which
+ * matched, or NULL for undecided.
  */
-int excluded_from_list(const char *pathname,
-		       int pathlen, const char *basename, int *dtype,
-		       struct exclude_list *el)
+struct exclude *excluded_from_list_1(const char *pathname, int pathlen,
+				     const char *basename, int *dtype,
+				     struct exclude_list *el)
 {
 	int i;
 
 	if (!el->nr)
-		return -1;	/* undefined */
+		return NULL;	/* undefined */
 
 	for (i = el->nr - 1; 0 <= i; i--) {
 		struct exclude *x = el->excludes[i];
 		const char *name, *exclude = x->pattern;
-		int to_exclude = x->to_exclude;
 		int namelen, prefix = x->nowildcardlen;
 
 		if (x->flags & EXC_FLAG_MUSTBEDIR) {
@@ -538,14 +540,14 @@ int excluded_from_list(const char *pathname,
 			/* match basename */
 			if (prefix == x->patternlen) {
 				if (!strcmp_icase(exclude, basename))
-					return to_exclude;
+					return x;
 			} else if (x->flags & EXC_FLAG_ENDSWITH) {
 				if (x->patternlen - 1 <= pathlen &&
 				    !strcmp_icase(exclude + 1, pathname + pathlen - x->patternlen + 1))
-					return to_exclude;
+					return x;
 			} else {
 				if (fnmatch_icase(exclude, basename, 0) == 0)
-					return to_exclude;
+					return x;
 			}
 			continue;
 		}
@@ -580,7 +582,23 @@ int excluded_from_list(const char *pathname,
 		}
 
 		if (!namelen || !fnmatch_icase(exclude, name, FNM_PATHNAME))
-			return to_exclude;
+			return x;
+	}
+	return NULL; /* undecided */
+}
+
+/*
+ * Scan the list and let the last match determine the fate.
+ * Return 1 for exclude, 0 for include and -1 for undecided.
+ */
+int excluded_from_list(const char *pathname,
+		       int pathlen, const char *basename, int *dtype,
+		       struct exclude_list *el)
+{
+	struct exclude *exclude;
+	exclude = excluded_from_list_1(pathname, pathlen, basename, dtype, el);
+	if (exclude) {
+		return exclude->to_exclude;
 	}
 	return -1; /* undecided */
 }
-- 
1.7.12.155.ge5750d5.dirty

Re: [PATCH 4/9] Refactor excluded_from_list

[Nguyen Thai Ngoc Duy]

On Sun, Sep 2, 2012 at 7:12 AM, Adam Spiers <git@adamspiers.org> wrote:
> diff --git a/dir.c b/dir.c
> index 57a5d11..3a532d5 100644
> --- a/dir.c
> +++ b/dir.c
> @@ -509,22 +509,24 @@ static void prep_exclude(struct dir_struct *dir, const char *base, int baselen)
>         dir->basebuf[baselen] = '\0';
>  }
>
> -/* Scan the list and let the last match determine the fate.
> - * Return 1 for exclude, 0 for include and -1 for undecided.
> +/*
> + * Scan the given exclude list in reverse to see whether pathname
> + * should be ignored.  The first match (i.e. the last on the list), if
> + * any, determines the fate.  Returns the exclude_list element which
> + * matched, or NULL for undecided.
>   */
> -int excluded_from_list(const char *pathname,
> -                      int pathlen, const char *basename, int *dtype,
> -                      struct exclude_list *el)
> +struct exclude *excluded_from_list_1(const char *pathname, int pathlen,
> +                                    const char *basename, int *dtype,
> +                                    struct exclude_list *el)
>  {
>         int i;
>

You should keep the new function static.
-- 
Duy

[PATCH 5/9] Refactor excluded and path_excluded

[Adam Spiers]

In a similar way to the previous commit, this extracts new helper
functions excluded_1() and path_excluded_1() which return the last
exclude_list element which matched, or NULL if no match was found.
excluded() and path_excluded() become wrappers around these, and just
return 0 or 1 depending on whether any matching exclude_list element
was found.

This allows callers to find out _why_ a given path was excluded,
rather than just whether it was or not, paving the way for a new git
sub-command which allows users to test their exclude lists from the
command line.

Signed-off-by: Adam Spiers <git@adamspiers.org>
---
 dir.c | 81 +++++++++++++++++++++++++++++++++++++++++++++++++++++--------------
 dir.h |  3 +++
 2 files changed, 67 insertions(+), 17 deletions(-)

diff --git a/dir.c b/dir.c
index 3a532d5..3d438c3 100644
--- a/dir.c
+++ b/dir.c
@@ -603,23 +603,42 @@ int excluded_from_list(const char *pathname,
 	return -1; /* undecided */
 }
 
-static int excluded(struct dir_struct *dir, const char *pathname, int *dtype_p)
+/*
+ * Loads the exclude lists for the directory containing pathname, then
+ * scans all exclude lists to determine whether pathname is excluded.
+ * Returns the exclude_list element which matched, or NULL for
+ * undecided.
+ */
+static struct exclude *excluded_1(struct dir_struct *dir, const char *pathname, int *dtype_p)
 {
 	int pathlen = strlen(pathname);
 	int st;
+	struct exclude *exclude;
 	const char *basename = strrchr(pathname, '/');
 	basename = (basename) ? basename+1 : pathname;
 
 	prep_exclude(dir, pathname, basename-pathname);
 	for (st = EXC_CMDL; st <= EXC_FILE; st++) {
-		switch (excluded_from_list(pathname, pathlen, basename,
-					   dtype_p, &dir->exclude_list[st])) {
-		case 0:
-			return 0;
-		case 1:
-			return 1;
+		exclude = excluded_from_list_1(pathname, pathlen, basename,
+					       dtype_p, &dir->exclude_list[st]);
+		if (exclude) {
+			return exclude;
 		}
 	}
+	return NULL;
+}
+
+/*
+ * Loads the exclude lists for the directory containing pathname, then
+ * scans all exclude lists to determine whether pathname is excluded.
+ * Returns 1 if true, otherwise 0.
+ */
+static int excluded(struct dir_struct *dir, const char *pathname, int *dtype_p)
+{
+	struct exclude *exclude = excluded_1(dir, pathname, dtype_p);
+	if (exclude) {
+		return exclude->to_exclude;
+	}
 	return 0;
 }
 
@@ -627,6 +646,7 @@ void path_exclude_check_init(struct path_exclude_check *check,
 			     struct dir_struct *dir)
 {
 	check->dir = dir;
+	check->exclude = NULL;
 	strbuf_init(&check->path, 256);
 }
 
@@ -636,18 +656,20 @@ void path_exclude_check_clear(struct path_exclude_check *check)
 }
 
 /*
- * Is this name excluded?  This is for a caller like show_files() that
- * do not honor directory hierarchy and iterate through paths that are
- * possibly in an ignored directory.
+ * For each subdirectory in name, starting with the top-most, checks
+ * to see if that subdirectory is excluded, and if so, returns the
+ * corresponding exclude structure.  Otherwise, checks whether name
+ * itself (which is presumably a file) is excluded.
  *
  * A path to a directory known to be excluded is left in check->path to
  * optimize for repeated checks for files in the same excluded directory.
  */
-int path_excluded(struct path_exclude_check *check,
-		  const char *name, int namelen, int *dtype)
+struct exclude *path_excluded_1(struct path_exclude_check *check,
+				const char *name, int namelen, int *dtype)
 {
 	int i;
 	struct strbuf *path = &check->path;
+	struct exclude *exclude;
 
 	/*
 	 * we allow the caller to pass namelen as an optimization; it
@@ -657,11 +679,18 @@ int path_excluded(struct path_exclude_check *check,
 	if (namelen < 0)
 		namelen = strlen(name);
 
+	/*
+	 * If path is non-empty, and name is equal to path or a
+	 * subdirectory of path, name should be excluded, because
+	 * it's inside a directory which is already known to be
+	 * excluded and was previously left in check->path.
+	 */
 	if (path->len &&
 	    path->len <= namelen &&
 	    !memcmp(name, path->buf, path->len) &&
-	    (!name[path->len] || name[path->len] == '/'))
-		return 1;
+	    (!name[path->len] || name[path->len] == '/')) {
+		return check->exclude;
+	}
 
 	strbuf_setlen(path, 0);
 	for (i = 0; name[i]; i++) {
@@ -669,8 +698,11 @@ int path_excluded(struct path_exclude_check *check,
 
 		if (ch == '/') {
 			int dt = DT_DIR;
-			if (excluded(check->dir, path->buf, &dt))
-				return 1;
+			exclude = excluded_1(check->dir, path->buf, &dt);
+			if (exclude) {
+				check->exclude = exclude;
+				return exclude;
+			}
 		}
 		strbuf_addch(path, ch);
 	}
@@ -678,7 +710,22 @@ int path_excluded(struct path_exclude_check *check,
 	/* An entry in the index; cannot be a directory with subentries */
 	strbuf_setlen(path, 0);
 
-	return excluded(check->dir, name, dtype);
+	return excluded_1(check->dir, name, dtype);
+}
+
+/*
+ * Is this name excluded?  This is for a caller like show_files() that
+ * do not honor directory hierarchy and iterate through paths that are
+ * possibly in an ignored directory.
+ */
+int path_excluded(struct path_exclude_check *check,
+		  const char *name, int namelen, int *dtype)
+{
+	struct exclude *exclude = path_excluded_1(check, name, namelen, dtype);
+	if (exclude) {
+		return exclude->to_exclude;
+	}
+	return 0;
 }
 
 static struct dir_entry *dir_entry_new(const char *pathname, int len)
diff --git a/dir.h b/dir.h
index 549a187..1b4f9dc 100644
--- a/dir.h
+++ b/dir.h
@@ -109,10 +109,13 @@ struct dir_entry *dir_add_ignored(struct dir_struct *dir, const char *pathname,
  */
 struct path_exclude_check {
 	struct dir_struct *dir;
+	struct exclude *exclude;
 	struct strbuf path;
 };
 extern void path_exclude_check_init(struct path_exclude_check *, struct dir_struct *);
 extern void path_exclude_check_clear(struct path_exclude_check *);
+extern struct exclude *path_excluded_1(struct path_exclude_check *, const char *,
+				       int namelen, int *dtype);
 extern int path_excluded(struct path_exclude_check *, const char *, int namelen, int *dtype);
 
 
-- 
1.7.12.155.ge5750d5.dirty

Re: [PATCH 5/9] Refactor excluded and path_excluded

[Nguyen Thai Ngoc Duy]

On Sun, Sep 2, 2012 at 7:12 AM, Adam Spiers <git@adamspiers.org> wrote:
>  extern void path_exclude_check_init(struct path_exclude_check *, struct dir_struct *);
>  extern void path_exclude_check_clear(struct path_exclude_check *);
> +extern struct exclude *path_excluded_1(struct path_exclude_check *, const char *,
> +                                      int namelen, int *dtype);
>  extern int path_excluded(struct path_exclude_check *, const char *, int namelen, int *dtype);

Exported functions should have nicer names than *_1. No idea what are
better names though, maybe exclude_path?
-- 
Duy

Re: [PATCH 5/9] Refactor excluded and path_excluded

[Junio C Hamano]

Nguyen Thai Ngoc Duy <pclouds@gmail.com> writes:

> On Sun, Sep 2, 2012 at 7:12 AM, Adam Spiers <git@adamspiers.org> wrote:
>>  extern void path_exclude_check_init(struct path_exclude_check *, struct dir_struct *);
>>  extern void path_exclude_check_clear(struct path_exclude_check *);
>> +extern struct exclude *path_excluded_1(struct path_exclude_check *, const char *,
>> +                                      int namelen, int *dtype);
>>  extern int path_excluded(struct path_exclude_check *, const char *, int namelen, int *dtype);
>
> Exported functions should have nicer names than *_1. No idea what are
> better names though, maybe exclude_path?

Which part is better?

Just like between path_excluded_1() and path_excluded() nobody can
tell how they differ (except perhaps the former smells more special
purpose thanks to its funny name) and wouldn't be able to tell which
one to call without looking at their sources, it is hard to tell
path_excluded() and exclude_path() apart.  In a sense, that pair is
even worse as there is no hint to suggest which one is more exotic
between them in their names.

Re: [PATCH 5/9] Refactor excluded and path_excluded

[Nguyen Thai Ngoc Duy]

On Wed, Sep 5, 2012 at 12:23 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Nguyen Thai Ngoc Duy <pclouds@gmail.com> writes:
>
>> On Sun, Sep 2, 2012 at 7:12 AM, Adam Spiers <git@adamspiers.org> wrote:
>>>  extern void path_exclude_check_init(struct path_exclude_check *, struct dir_struct *);
>>>  extern void path_exclude_check_clear(struct path_exclude_check *);
>>> +extern struct exclude *path_excluded_1(struct path_exclude_check *, const char *,
>>> +                                      int namelen, int *dtype);
>>>  extern int path_excluded(struct path_exclude_check *, const char *, int namelen, int *dtype);
>>
>> Exported functions should have nicer names than *_1. No idea what are
>> better names though, maybe exclude_path?
>
> Which part is better?
>
> Just like between path_excluded_1() and path_excluded() nobody can
> tell how they differ (except perhaps the former smells more special
> purpose thanks to its funny name) and wouldn't be able to tell which
> one to call without looking at their sources, it is hard to tell
> path_excluded() and exclude_path() apart.  In a sense, that pair is
> even worse as there is no hint to suggest which one is more exotic
> between them in their names.

We could introduce exclude_path() and kill path_excluded() then. There
are just about 5-6 call sites to replace.
-- 
Duy

Re: [PATCH 5/9] Refactor excluded and path_excluded

[Junio C Hamano]

Nguyen Thai Ngoc Duy <pclouds@gmail.com> writes:

> We could introduce exclude_path() and kill path_excluded() then. There
> are just about 5-6 call sites to replace.

The name path_excluded(... path ...) sounds like it is asking a
yes/no question "is this path excluded?", which actually is what is
going on.

The name exclude_path(... path ...) sounds as if you are requesting
somebody to exclude the path.  Does that meaning match the semantics
of the function?

Re: [PATCH 5/9] Refactor excluded and path_excluded

[Nguyen Thai Ngoc Duy]

On Thu, Sep 6, 2012 at 10:21 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Nguyen Thai Ngoc Duy <pclouds@gmail.com> writes:
>
>> We could introduce exclude_path() and kill path_excluded() then. There
>> are just about 5-6 call sites to replace.
>
> The name path_excluded(... path ...) sounds like it is asking a
> yes/no question "is this path excluded?", which actually is what is
> going on.
>
> The name exclude_path(... path ...) sounds as if you are requesting
> somebody to exclude the path.  Does that meaning match the semantics
> of the function?

I'm not great at naming. And path_excluded() cannot be reused to avoid
problems with other ongoing series if any. So path_is_excluded()?
-- 
Duy

Re: [PATCH 5/9] Refactor excluded and path_excluded

[Thiago Farina]

On Thu, Sep 6, 2012 at 9:13 AM, Nguyen Thai Ngoc Duy <pclouds@gmail.com> wrote:
> On Thu, Sep 6, 2012 at 10:21 AM, Junio C Hamano <gitster@pobox.com> wrote:
>> Nguyen Thai Ngoc Duy <pclouds@gmail.com> writes:
>>
>>> We could introduce exclude_path() and kill path_excluded() then. There
>>> are just about 5-6 call sites to replace.
>>
>> The name path_excluded(... path ...) sounds like it is asking a
>> yes/no question "is this path excluded?", which actually is what is
>> going on.
>>
>> The name exclude_path(... path ...) sounds as if you are requesting
>> somebody to exclude the path.  Does that meaning match the semantics
>> of the function?
>
> I'm not great at naming. And path_excluded() cannot be reused to avoid
> problems with other ongoing series if any. So path_is_excluded()?

is_path_excluded()?

Re: [PATCH 5/9] Refactor excluded and path_excluded

[Nguyen Thai Ngoc Duy]

On Thu, Sep 6, 2012 at 9:59 PM, Thiago Farina <tfransosi@gmail.com> wrote:
> On Thu, Sep 6, 2012 at 9:13 AM, Nguyen Thai Ngoc Duy <pclouds@gmail.com> wrote:
>> On Thu, Sep 6, 2012 at 10:21 AM, Junio C Hamano <gitster@pobox.com> wrote:
>>> Nguyen Thai Ngoc Duy <pclouds@gmail.com> writes:
>>>
>>>> We could introduce exclude_path() and kill path_excluded() then. There
>>>> are just about 5-6 call sites to replace.
>>>
>>> The name path_excluded(... path ...) sounds like it is asking a
>>> yes/no question "is this path excluded?", which actually is what is
>>> going on.
>>>
>>> The name exclude_path(... path ...) sounds as if you are requesting
>>> somebody to exclude the path.  Does that meaning match the semantics
>>> of the function?
>>
>> I'm not great at naming. And path_excluded() cannot be reused to avoid
>> problems with other ongoing series if any. So path_is_excluded()?
>
> is_path_excluded()?

Good too.
-- 
Duy

Re: [PATCH 5/9] Refactor excluded and path_excluded

[Adam Spiers]

On Thu, Sep 6, 2012 at 4:05 PM, Nguyen Thai Ngoc Duy <pclouds@gmail.com> wrote:
> On Thu, Sep 6, 2012 at 9:59 PM, Thiago Farina <tfransosi@gmail.com> wrote:
>> On Thu, Sep 6, 2012 at 9:13 AM, Nguyen Thai Ngoc Duy <pclouds@gmail.com> wrote:
>>> On Thu, Sep 6, 2012 at 10:21 AM, Junio C Hamano <gitster@pobox.com> wrote:
>>>> Nguyen Thai Ngoc Duy <pclouds@gmail.com> writes:
>>>>
>>>>> We could introduce exclude_path() and kill path_excluded() then. There
>>>>> are just about 5-6 call sites to replace.
>>>>
>>>> The name path_excluded(... path ...) sounds like it is asking a
>>>> yes/no question "is this path excluded?", which actually is what is
>>>> going on.
>>>>
>>>> The name exclude_path(... path ...) sounds as if you are requesting
>>>> somebody to exclude the path.  Does that meaning match the semantics
>>>> of the function?
>>>
>>> I'm not great at naming. And path_excluded() cannot be reused to avoid
>>> problems with other ongoing series if any. So path_is_excluded()?
>>
>> is_path_excluded()?

It makes me happy to see this suggestion :-) Because this is the exact
name I changed it to in an earlier version of my patch series.  But
then I got worried that a) it did not fit the `foo_1' convention which
already seemed to be in use, and b) people would complain about API
changes.  So I changed it back.  However if you would prefer to rename
the functions for clarity, then I have more suggestions:

  old                       new
  -----------------------------------------------------------
  path_excluded             is_path_excluded
  excluded                  is_excluded
  excluded_from_list        is_excluded_from_list
  path_excluded_1           last_exclude_matching_path
  excluded_1                last_exclude_matching
  excluded_from_list_1      last_exclude_matching_from_list

Re: [PATCH 5/9] Refactor excluded and path_excluded

[Junio C Hamano]

Nguyen Thai Ngoc Duy <pclouds@gmail.com> writes:

>>> I'm not great at naming. And path_excluded() cannot be reused to avoid
>>> problems with other ongoing series if any. So path_is_excluded()?
>>
>> is_path_excluded()?
>
> Good too.

Sure, either would work.

[PATCH 6/9] For each exclude pattern, store information about where it came from

[Adam Spiers]

For exclude patterns read in from files, the filename is stored together
with the corresponding line number (counting starting at 1).

For exclude patterns provided on the command line, the sequence number
is negative, with counting starting at -1, so for example the 2nd
pattern provided via --exclude would be numbered -2.  This allows any
future consumers of that data to easily distinguish between exclude
patterns from files vs. from the CLI.

Signed-off-by: Adam Spiers <git@adamspiers.org>
---
 builtin/clean.c    |  2 +-
 builtin/ls-files.c |  3 ++-
 dir.c              | 25 +++++++++++++++++++------
 dir.h              |  5 ++++-
 4 files changed, 26 insertions(+), 9 deletions(-)

diff --git a/builtin/clean.c b/builtin/clean.c
index 0c7b3d0..f618231 100644
--- a/builtin/clean.c
+++ b/builtin/clean.c
@@ -99,7 +99,7 @@ int cmd_clean(int argc, const char **argv, const char *prefix)
 
 	for (i = 0; i < exclude_list.nr; i++)
 		add_exclude(exclude_list.items[i].string, "", 0,
-			    &dir.exclude_list[EXC_CMDL]);
+			    &dir.exclude_list[EXC_CMDL], "--exclude option", -(i+1));
 
 	pathspec = get_pathspec(prefix, argv);
 
diff --git a/builtin/ls-files.c b/builtin/ls-files.c
index 31b3f2d..420ff40 100644
--- a/builtin/ls-files.c
+++ b/builtin/ls-files.c
@@ -35,6 +35,7 @@ static int error_unmatch;
 static char *ps_matched;
 static const char *with_tree;
 static int exc_given;
+static int exclude_args = 0;
 
 static const char *tag_cached = "";
 static const char *tag_unmerged = "";
@@ -423,7 +424,7 @@ static int option_parse_exclude(const struct option *opt,
 	struct exclude_list *list = opt->value;
 
 	exc_given = 1;
-	add_exclude(arg, "", 0, list);
+	add_exclude(arg, "", 0, list, "--exclude option", --exclude_args);
 
 	return 0;
 }
diff --git a/dir.c b/dir.c
index 3d438c3..ac8c838 100644
--- a/dir.c
+++ b/dir.c
@@ -311,7 +311,7 @@ static int no_wildcard(const char *string)
 }
 
 void add_exclude(const char *string, const char *base,
-		 int baselen, struct exclude_list *el)
+		 int baselen, struct exclude_list *el, const char *src, int srcpos)
 {
 	struct exclude *x;
 	size_t len;
@@ -341,6 +341,8 @@ void add_exclude(const char *string, const char *base,
 	x->base = base;
 	x->baselen = baselen;
 	x->flags = flags;
+	x->src = src;
+	x->srcpos = srcpos;
 	if (!strchr(string, '/'))
 		x->flags |= EXC_FLAG_NODIR;
 	x->nowildcardlen = simple_length(string);
@@ -393,7 +395,7 @@ int add_excludes_from_file_to_list(const char *fname,
 				   int check_index)
 {
 	struct stat st;
-	int fd, i;
+	int fd, i, lineno = 1;
 	size_t size = 0;
 	char *buf, *entry;
 
@@ -436,8 +438,9 @@ int add_excludes_from_file_to_list(const char *fname,
 		if (buf[i] == '\n') {
 			if (entry != buf + i && entry[0] != '#') {
 				buf[i - (i && buf[i-1] == '\r')] = 0;
-				add_exclude(entry, base, baselen, el);
+				add_exclude(entry, base, baselen, el, fname, lineno);
 			}
+			lineno++;
 			entry = buf + i + 1;
 		}
 	}
@@ -472,8 +475,10 @@ static void prep_exclude(struct dir_struct *dir, const char *base, int baselen)
 		    !strncmp(dir->basebuf, base, stk->baselen))
 			break;
 		dir->exclude_stack = stk->prev;
-		while (stk->exclude_ix < el->nr)
-			free(el->excludes[--el->nr]);
+		while (stk->exclude_ix < el->nr) {
+			struct exclude *exclude = el->excludes[--el->nr];
+			free(exclude);
+		}
 		free(stk->filebuf);
 		free(stk);
 	}
@@ -500,7 +505,15 @@ static void prep_exclude(struct dir_struct *dir, const char *base, int baselen)
 		memcpy(dir->basebuf + current, base + current,
 		       stk->baselen - current);
 		strcpy(dir->basebuf + stk->baselen, dir->exclude_per_dir);
-		add_excludes_from_file_to_list(dir->basebuf,
+
+		/* dir->basebuf gets reused by the traversal, but we
+		 * need fname to remain unchanged to ensure the src
+		 * member of each struct exclude correctly back-references
+		 * its source file.
+		 */
+		char *fname = strdup(dir->basebuf);
+
+		add_excludes_from_file_to_list(fname,
 					       dir->basebuf, stk->baselen,
 					       &stk->filebuf, el, 1);
 		dir->exclude_stack = stk;
diff --git a/dir.h b/dir.h
index 1b4f9dc..81efee4 100644
--- a/dir.h
+++ b/dir.h
@@ -31,6 +31,9 @@ struct exclude_list {
 		int baselen;
 		int to_exclude;
 		int flags;
+		const char *src;
+		int srcpos; /* counting starts from 1 for line numbers in ignore files,
+			       and from -1 decrementing for patterns from CLI (--exclude) */
 	} **excludes;
 };
 
@@ -123,7 +126,7 @@ extern int add_excludes_from_file_to_list(const char *fname, const char *base, i
 					  char **buf_p, struct exclude_list *el, int check_index);
 extern void add_excludes_from_file(struct dir_struct *, const char *fname);
 extern void add_exclude(const char *string, const char *base,
-			int baselen, struct exclude_list *el);
+			int baselen, struct exclude_list *el, const char *src, int srcpos);
 extern void free_excludes(struct exclude_list *el);
 extern int file_exists(const char *);
 
-- 
1.7.12.155.ge5750d5.dirty

Re: [PATCH 6/9] For each exclude pattern, store information about where it came from

[Philip Oakley]

From: "Adam Spiers" <git@adamspiers.org>
Sent: Sunday, September 02, 2012 1:12 AM
Subject: [PATCH 6/9] For each exclude pattern, store information about 
where it came from


> For exclude patterns read in from files, the filename is stored 
> together
> with the corresponding line number (counting starting at 1).

Is there a way to identify the config core.excludesfile if present? i.e. 
that it is from that config variable, rather than directory traversal.

This came up in a recent 
http://stackoverflow.com/questions/12199150/effective-gitignore-and-clean-strategy/12205852#12205852 
discussion.

>
> For exclude patterns provided on the command line, the sequence number
> is negative, with counting starting at -1, so for example the 2nd
> pattern provided via --exclude would be numbered -2.  This allows any
> future consumers of that data to easily distinguish between exclude
> patterns from files vs. from the CLI.
>
> Signed-off-by: Adam Spiers <git@adamspiers.org>
> ---
> builtin/clean.c    |  2 +-
> builtin/ls-files.c |  3 ++-
> dir.c              | 25 +++++++++++++++++++------
> dir.h              |  5 ++++-
> 4 files changed, 26 insertions(+), 9 deletions(-)
>
> diff --git a/builtin/clean.c b/builtin/clean.c
> index 0c7b3d0..f618231 100644
> --- a/builtin/clean.c
> +++ b/builtin/clean.c
> @@ -99,7 +99,7 @@ int cmd_clean(int argc, const char **argv, const 
> char *prefix)
>
>  for (i = 0; i < exclude_list.nr; i++)
>  add_exclude(exclude_list.items[i].string, "", 0,
> -     &dir.exclude_list[EXC_CMDL]);
> +     &dir.exclude_list[EXC_CMDL], "--exclude option", -(i+1));
>
>  pathspec = get_pathspec(prefix, argv);
>
> diff --git a/builtin/ls-files.c b/builtin/ls-files.c
> index 31b3f2d..420ff40 100644
> --- a/builtin/ls-files.c
> +++ b/builtin/ls-files.c
> @@ -35,6 +35,7 @@ static int error_unmatch;
> static char *ps_matched;
> static const char *with_tree;
> static int exc_given;
> +static int exclude_args = 0;
>
> static const char *tag_cached = "";
> static const char *tag_unmerged = "";
> @@ -423,7 +424,7 @@ static int option_parse_exclude(const struct 
> option *opt,
>  struct exclude_list *list = opt->value;
>
>  exc_given = 1;
> - add_exclude(arg, "", 0, list);
> + add_exclude(arg, "", 0, list, "--exclude option", --exclude_args);
>
>  return 0;
> }
> diff --git a/dir.c b/dir.c
> index 3d438c3..ac8c838 100644
> --- a/dir.c
> +++ b/dir.c
> @@ -311,7 +311,7 @@ static int no_wildcard(const char *string)
> }
>
> void add_exclude(const char *string, const char *base,
> - int baselen, struct exclude_list *el)
> + int baselen, struct exclude_list *el, const char *src, int srcpos)
> {
>  struct exclude *x;
>  size_t len;
> @@ -341,6 +341,8 @@ void add_exclude(const char *string, const char 
> *base,
>  x->base = base;
>  x->baselen = baselen;
>  x->flags = flags;
> + x->src = src;
> + x->srcpos = srcpos;
>  if (!strchr(string, '/'))
>  x->flags |= EXC_FLAG_NODIR;
>  x->nowildcardlen = simple_length(string);
> @@ -393,7 +395,7 @@ int add_excludes_from_file_to_list(const char 
> *fname,
>     int check_index)
> {
>  struct stat st;
> - int fd, i;
> + int fd, i, lineno = 1;
>  size_t size = 0;
>  char *buf, *entry;
>
> @@ -436,8 +438,9 @@ int add_excludes_from_file_to_list(const char 
> *fname,
>  if (buf[i] == '\n') {
>  if (entry != buf + i && entry[0] != '#') {
>  buf[i - (i && buf[i-1] == '\r')] = 0;
> - add_exclude(entry, base, baselen, el);
> + add_exclude(entry, base, baselen, el, fname, lineno);
>  }
> + lineno++;
>  entry = buf + i + 1;
>  }
>  }
> @@ -472,8 +475,10 @@ static void prep_exclude(struct dir_struct *dir, 
> const char *base, int baselen)
>      !strncmp(dir->basebuf, base, stk->baselen))
>  break;
>  dir->exclude_stack = stk->prev;
> - while (stk->exclude_ix < el->nr)
> - free(el->excludes[--el->nr]);
> + while (stk->exclude_ix < el->nr) {
> + struct exclude *exclude = el->excludes[--el->nr];
> + free(exclude);
> + }
>  free(stk->filebuf);
>  free(stk);
>  }
> @@ -500,7 +505,15 @@ static void prep_exclude(struct dir_struct *dir, 
> const char *base, int baselen)
>  memcpy(dir->basebuf + current, base + current,
>         stk->baselen - current);
>  strcpy(dir->basebuf + stk->baselen, dir->exclude_per_dir);
> - add_excludes_from_file_to_list(dir->basebuf,
> +
> + /* dir->basebuf gets reused by the traversal, but we
> + * need fname to remain unchanged to ensure the src
> + * member of each struct exclude correctly back-references
> + * its source file.
> + */
> + char *fname = strdup(dir->basebuf);
> +
> + add_excludes_from_file_to_list(fname,
>         dir->basebuf, stk->baselen,
>         &stk->filebuf, el, 1);
>  dir->exclude_stack = stk;
> diff --git a/dir.h b/dir.h
> index 1b4f9dc..81efee4 100644
> --- a/dir.h
> +++ b/dir.h
> @@ -31,6 +31,9 @@ struct exclude_list {
>  int baselen;
>  int to_exclude;
>  int flags;
> + const char *src;
> + int srcpos; /* counting starts from 1 for line numbers in ignore 
> files,
> +        and from -1 decrementing for patterns from CLI (--exclude) */
>  } **excludes;
> };
>
> @@ -123,7 +126,7 @@ extern int add_excludes_from_file_to_list(const 
> char *fname, const char *base, i
>    char **buf_p, struct exclude_list *el, int check_index);
> extern void add_excludes_from_file(struct dir_struct *, const char 
> *fname);
> extern void add_exclude(const char *string, const char *base,
> - int baselen, struct exclude_list *el);
> + int baselen, struct exclude_list *el, const char *src, int srcpos);
> extern void free_excludes(struct exclude_list *el);
> extern int file_exists(const char *);
>
> -- 
> 1.7.12.155.ge5750d5.dirty
>
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
>
> -----
> No virus found in this message.
> Checked by AVG - www.avg.com
> Version: 2012.0.2197 / Virus Database: 2437/5240 - Release Date: 
> 09/01/12
> 

Re: [PATCH 6/9] For each exclude pattern, store information about where it came from

[Junio C Hamano]

"Philip Oakley" <philipoakley@iee.org> writes:

> Is there a way to identify the config core.excludesfile if present?
> i.e. that it is from that config variable, rather than directory
> traversal.

If the code handles $GIT_DIR/info/exclude then that configuration
would also be handled the same way, no?

Re: [PATCH 6/9] For each exclude pattern, store information about where it came from

[Philip Oakley]

From: "Junio C Hamano" <gitster@pobox.com>
Sent: Sunday, September 02, 2012 8:02 PM
> "Philip Oakley" <philipoakley@iee.org> writes:
>
>> Is there a way to identify the config core.excludesfile if present?
>> i.e. that it is from that config variable, rather than directory
>> traversal.
>
> If the code handles $GIT_DIR/info/exclude then that configuration
> would also be handled the same way, no?

Probably not. The $GIT_DIR/info/exclude is directly a path, while the 
core.excludesfile could point anywhere. This assumes the path to the 
relevant ignore file is shown.

Given the suggested report format in the Documentation, this path could 
be reported as 'coreexclude', not just an 'exclude'.

If I've understood the regular code correctly, the core.excludesfile is 
always at one end of the exclude struct so should be easy to check at 
that position. 

Re: [PATCH 6/9] For each exclude pattern, store information about where it came from

[Adam Spiers]

On Sun, Sep 2, 2012 at 11:36 PM, Philip Oakley <philipoakley@iee.org> wrote:
> From: "Junio C Hamano" <gitster@pobox.com>
> Sent: Sunday, September 02, 2012 8:02 PM
>> "Philip Oakley" <philipoakley@iee.org> writes:
>>> Is there a way to identify the config core.excludesfile if present?
>>> i.e. that it is from that config variable, rather than directory
>>> traversal.

Yes, the output of git check-ignore includes the source file, so you
can easily see whether the ignore originated from a per-directory
exclude or from core.excludesfile.  One giveaway is that the former
is an absolute path, and the latter are all relative.

>> If the code handles $GIT_DIR/info/exclude then that configuration
>> would also be handled the same way, no?

Yes, they are both handled via setup_standard_excludes().

> If I've understood the regular code correctly, the core.excludesfile is
> always at one end of the exclude struct so should be easy to check at that
> position.

It's already checked in my patch series, and the tests cover this case:

  https://github.com/aspiers/git/blob/check-ignore/t/t0007-ignores.sh#L214

Having said that, I forgot to make them check $GIT_DIR/info/exclude.
I'll fix that.

[PATCH 7/9] Extract some useful pathspec handling code from builtin/add.c into a library

[Adam Spiers]

This is in preparation for reuse by a new git check-ignore command.

Signed-off-by: Adam Spiers <git@adamspiers.org>
---
 Makefile      |  2 ++
 builtin/add.c | 82 +++----------------------------------------------------
 pathspec.c    | 87 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 pathspec.h    |  6 +++++
 4 files changed, 98 insertions(+), 79 deletions(-)
 create mode 100644 pathspec.c
 create mode 100644 pathspec.h

diff --git a/Makefile b/Makefile
index 66e8216..272ab69 100644
--- a/Makefile
+++ b/Makefile
@@ -640,6 +640,7 @@ LIB_H += pack-revindex.h
 LIB_H += pack.h
 LIB_H += parse-options.h
 LIB_H += patch-ids.h
+LIB_H += pathspec.h
 LIB_H += pkt-line.h
 LIB_H += progress.h
 LIB_H += prompt.h
@@ -760,6 +761,7 @@ LIB_OBJS += parse-options-cb.o
 LIB_OBJS += patch-delta.o
 LIB_OBJS += patch-ids.o
 LIB_OBJS += path.o
+LIB_OBJS += pathspec.o
 LIB_OBJS += pkt-line.o
 LIB_OBJS += preload-index.o
 LIB_OBJS += pretty.o
diff --git a/builtin/add.c b/builtin/add.c
index 89dce56..a7ed2ad 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -6,6 +6,7 @@
 #include "cache.h"
 #include "builtin.h"
 #include "dir.h"
+#include "pathspec.h"
 #include "exec_cmd.h"
 #include "cache-tree.h"
 #include "run-command.h"
@@ -97,39 +98,6 @@ int add_files_to_cache(const char *prefix, const char **pathspec, int flags)
 	return !!data.add_errors;
 }
 
-static void fill_pathspec_matches(const char **pathspec, char *seen, int specs)
-{
-	int num_unmatched = 0, i;
-
-	/*
-	 * Since we are walking the index as if we were walking the directory,
-	 * we have to mark the matched pathspec as seen; otherwise we will
-	 * mistakenly think that the user gave a pathspec that did not match
-	 * anything.
-	 */
-	for (i = 0; i < specs; i++)
-		if (!seen[i])
-			num_unmatched++;
-	if (!num_unmatched)
-		return;
-	for (i = 0; i < active_nr; i++) {
-		struct cache_entry *ce = active_cache[i];
-		match_pathspec(pathspec, ce->name, ce_namelen(ce), 0, seen);
-	}
-}
-
-static char *find_used_pathspec(const char **pathspec)
-{
-	char *seen;
-	int i;
-
-	for (i = 0; pathspec[i];  i++)
-		; /* just counting */
-	seen = xcalloc(i, 1);
-	fill_pathspec_matches(pathspec, seen, i);
-	return seen;
-}
-
 static char *prune_directory(struct dir_struct *dir, const char **pathspec, int prefix)
 {
 	char *seen;
@@ -153,33 +121,6 @@ static char *prune_directory(struct dir_struct *dir, const char **pathspec, int
 	return seen;
 }
 
-static void treat_gitlinks(const char **pathspec)
-{
-	int i;
-
-	if (!pathspec || !*pathspec)
-		return;
-
-	for (i = 0; i < active_nr; i++) {
-		struct cache_entry *ce = active_cache[i];
-		if (S_ISGITLINK(ce->ce_mode)) {
-			int len = ce_namelen(ce), j;
-			for (j = 0; pathspec[j]; j++) {
-				int len2 = strlen(pathspec[j]);
-				if (len2 <= len || pathspec[j][len] != '/' ||
-				    memcmp(ce->name, pathspec[j], len))
-					continue;
-				if (len2 == len + 1)
-					/* strip trailing slash */
-					pathspec[j] = xstrndup(ce->name, len);
-				else
-					die (_("Path '%s' is in submodule '%.*s'"),
-						pathspec[j], len, ce->name);
-			}
-		}
-	}
-}
-
 static void refresh(int verbose, const char **pathspec)
 {
 	char *seen;
@@ -197,23 +138,6 @@ static void refresh(int verbose, const char **pathspec)
         free(seen);
 }
 
-static const char **validate_pathspec(int argc, const char **argv, const char *prefix)
-{
-	const char **pathspec = get_pathspec(prefix, argv);
-
-	if (pathspec) {
-		const char **p;
-		for (p = pathspec; *p; p++) {
-			if (has_symlink_leading_path(*p, strlen(*p))) {
-				int len = prefix ? strlen(prefix) : 0;
-				die(_("'%s' is beyond a symbolic link"), *p + len);
-			}
-		}
-	}
-
-	return pathspec;
-}
-
 int run_add_interactive(const char *revision, const char *patch_mode,
 			const char **pathspec)
 {
@@ -248,7 +172,7 @@ int interactive_add(int argc, const char **argv, const char *prefix, int patch)
 	const char **pathspec = NULL;
 
 	if (argc) {
-		pathspec = validate_pathspec(argc, argv, prefix);
+		pathspec = validate_pathspec(prefix, argv);
 		if (!pathspec)
 			return -1;
 	}
@@ -414,7 +338,7 @@ int cmd_add(int argc, const char **argv, const char *prefix)
 		fprintf(stderr, _("Maybe you wanted to say 'git add .'?\n"));
 		return 0;
 	}
-	pathspec = validate_pathspec(argc, argv, prefix);
+	pathspec = validate_pathspec(prefix, argv);
 
 	if (read_cache() < 0)
 		die(_("index file corrupt"));
diff --git a/pathspec.c b/pathspec.c
new file mode 100644
index 0000000..51cb8ce
--- /dev/null
+++ b/pathspec.c
@@ -0,0 +1,87 @@
+#include "cache.h"
+#include "dir.h"
+
+void validate_path(const char *prefix, const char *path)
+{
+	if (has_symlink_leading_path(path, strlen(path))) {
+		int len = prefix ? strlen(prefix) : 0;
+		die(_("'%s' is beyond a symbolic link"), path + len);
+	}
+}
+
+const char **validate_pathspec(const char *prefix, const char **files)
+{
+	const char **pathspec = get_pathspec(prefix, files);
+
+	if (pathspec) {
+		const char **p;
+		for (p = pathspec; *p; p++) {
+			validate_path(prefix, *p);
+		}
+	}
+
+	return pathspec;
+}
+
+void fill_pathspec_matches(const char **pathspec, char *seen, int specs)
+{
+	int num_unmatched = 0, i;
+
+	/*
+	 * Since we are walking the index as if we were walking the directory,
+	 * we have to mark the matched pathspec as seen; otherwise we will
+	 * mistakenly think that the user gave a pathspec that did not match
+	 * anything.
+	 */
+	for (i = 0; i < specs; i++)
+		if (!seen[i])
+			num_unmatched++;
+	if (!num_unmatched)
+		return;
+	for (i = 0; i < active_nr; i++) {
+		struct cache_entry *ce = active_cache[i];
+		match_pathspec(pathspec, ce->name, ce_namelen(ce), 0, seen);
+	}
+}
+
+char *find_used_pathspec(const char **pathspec)
+{
+	char *seen;
+	int i;
+
+	for (i = 0; pathspec[i];  i++)
+		; /* just counting */
+	seen = xcalloc(i, 1);
+	fill_pathspec_matches(pathspec, seen, i);
+	return seen;
+}
+
+void treat_gitlink(const char *path)
+{
+	int i, len = strlen(path);
+	for (i = 0; i < active_nr; i++) {
+		struct cache_entry *ce = active_cache[i];
+		if (S_ISGITLINK(ce->ce_mode)) {
+			int len2 = ce_namelen(ce);
+			if (len <= len2 || path[len2] != '/' ||
+			    memcmp(ce->name, path, len2))
+				continue;
+			if (len == len2 + 1)
+				/* strip trailing slash */
+				path = xstrndup(ce->name, len2);
+			else
+				die (_("Path '%s' is in submodule '%.*s'"),
+				     path, len2, ce->name);
+		}
+	}
+}
+
+void treat_gitlinks(const char **pathspec)
+{
+	if (!pathspec || !*pathspec)
+		return;
+
+	int i;
+	for (i = 0; pathspec[i]; i++)
+		treat_gitlink(pathspec[i]);
+}
diff --git a/pathspec.h b/pathspec.h
new file mode 100644
index 0000000..87e4296
--- /dev/null
+++ b/pathspec.h
@@ -0,0 +1,6 @@
+extern void validate_path(const char *prefix, const char *path);
+extern const char **validate_pathspec(const char *prefix, const char **files);
+extern char *find_used_pathspec(const char **pathspec);
+extern void fill_pathspec_matches(const char **pathspec, char *seen, int specs);
+extern void treat_gitlink(const char *path);
+extern void treat_gitlinks(const char **pathspec);
-- 
1.7.12.155.ge5750d5.dirty

[PATCH 8/9] Provide free_directory() for reclaiming dir_struct memory

[Adam Spiers]

Signed-off-by: Adam Spiers <git@adamspiers.org>
---
 Documentation/technical/api-directory-listing.txt |  2 ++
 dir.c                                             | 23 +++++++++++++++++++++--
 dir.h                                             |  1 +
 3 files changed, 24 insertions(+), 2 deletions(-)

diff --git a/Documentation/technical/api-directory-listing.txt b/Documentation/technical/api-directory-listing.txt
index 944fc39..e339c18 100644
--- a/Documentation/technical/api-directory-listing.txt
+++ b/Documentation/technical/api-directory-listing.txt
@@ -79,4 +79,6 @@ marked. If you to exclude files, make sure you have loaded index first.
 
 * Use `dir.entries[]`.
 
+* Call `free_directory()` when none of the contained elements are no longer in use.
+
 (JC)
diff --git a/dir.c b/dir.c
index ac8c838..80f9b22 100644
--- a/dir.c
+++ b/dir.c
@@ -454,6 +454,12 @@ void add_excludes_from_file(struct dir_struct *dir, const char *fname)
 		die("cannot use %s as an exclude file", fname);
 }
 
+static void free_exclude_stack(struct exclude_stack *stk)
+{
+	free(stk->filebuf);
+	free(stk);
+}
+
 /*
  * Loads the per-directory exclude list for the substring of base
  * which has a char length of baselen.
@@ -479,8 +485,7 @@ static void prep_exclude(struct dir_struct *dir, const char *base, int baselen)
 			struct exclude *exclude = el->excludes[--el->nr];
 			free(exclude);
 		}
-		free(stk->filebuf);
-		free(stk);
+		free_exclude_stack(stk);
 	}
 
 	/* Read from the parent directories and push them down. */
@@ -1467,3 +1472,17 @@ void free_pathspec(struct pathspec *pathspec)
 	free(pathspec->items);
 	pathspec->items = NULL;
 }
+
+void free_directory(struct dir_struct *dir)
+{
+	int st;
+	for (st = EXC_CMDL; st <= EXC_FILE; st++)
+		free_excludes(&dir->exclude_list[st]);
+
+	struct exclude_stack *prev, *stk = dir->exclude_stack;
+	while (stk) {
+		prev = stk->prev;
+		free_exclude_stack(stk);
+		stk = prev;
+	}
+}
diff --git a/dir.h b/dir.h
index 81efee4..f7cea9c 100644
--- a/dir.h
+++ b/dir.h
@@ -128,6 +128,7 @@ extern void add_excludes_from_file(struct dir_struct *, const char *fname);
 extern void add_exclude(const char *string, const char *base,
 			int baselen, struct exclude_list *el, const char *src, int srcpos);
 extern void free_excludes(struct exclude_list *el);
+extern void free_directory(struct dir_struct *dir);
 extern int file_exists(const char *);
 
 extern int is_inside_dir(const char *dir);
-- 
1.7.12.155.ge5750d5.dirty

[PATCH 9/9] Add git-check-ignores

[Adam Spiers]

This works in a similar manner to git-check-attr.  Some code
was reused from add.c by refactoring out into pathspec.c.

Thanks to Jeff King and Junio C Hamano for the idea:
http://thread.gmane.org/gmane.comp.version-control.git/108671/focus=108815

Signed-off-by: Adam Spiers <git@adamspiers.org>
---
 .gitignore                             |   1 +
 Documentation/git-check-ignore.txt     |  58 +++++++
 Documentation/gitignore.txt            |   6 +-
 Makefile                               |   1 +
 builtin.h                              |   1 +
 builtin/add.c                          |   2 +-
 builtin/check-ignore.c                 | 150 ++++++++++++++++
 command-list.txt                       |   1 +
 contrib/completion/git-completion.bash |   1 +
 git.c                                  |   1 +
 t/t0007-ignores.sh                     | 301 +++++++++++++++++++++++++++++++++
 11 files changed, 520 insertions(+), 3 deletions(-)
 create mode 100644 Documentation/git-check-ignore.txt
 create mode 100644 builtin/check-ignore.c
 create mode 100755 t/t0007-ignores.sh

diff --git a/.gitignore b/.gitignore
index bb5c91e..0cbe94c 100644
--- a/.gitignore
+++ b/.gitignore
@@ -22,6 +22,7 @@
 /git-bundle
 /git-cat-file
 /git-check-attr
+/git-check-ignore
 /git-check-ref-format
 /git-checkout
 /git-checkout-index
diff --git a/Documentation/git-check-ignore.txt b/Documentation/git-check-ignore.txt
new file mode 100644
index 0000000..3a85dbb
--- /dev/null
+++ b/Documentation/git-check-ignore.txt
@@ -0,0 +1,58 @@
+git-check-ignore(1)
+=================
+
+NAME
+----
+git-check-ignore - Debug gitignore / exclude files
+
+
+SYNOPSIS
+--------
+[verse]
+'git check-ignore' pathname...
+'git check-ignore' --stdin [-z] < <list-of-paths>
+
+DESCRIPTION
+-----------
+
+For each pathname given via the command-line or from a file via
+`--stdin`, this command will list the first exclude pattern found (if
+any) which explicitly excludes or includes that pathname.  Note that
+within any given exclude file, later patterns take precedence over
+earlier ones, so any matching pattern which this command outputs may
+not be the one you would immediately expect.
+
+OPTIONS
+-------
+--stdin::
+	Read file names from stdin instead of from the command-line.
+
+-z::
+	Only meaningful with `--stdin`; paths are separated with a
+	NUL character instead of a linefeed character.
+
+OUTPUT
+------
+
+The output is a series of lines of the form:
+
+<path> COLON SP <type> SP <pattern> SP <source> SP <position> LF
+
+<path> is the path of a file being queried, <type> is either
+'excluded' or 'included' (for patterns prefixed with '!'), <pattern>
+is the matching pattern, <source> is the pattern's source file (either
+as an absolute path or relative to the repository root), and
+<position> is the position of the pattern within that source.
+
+If no pattern matches a given path, nothing will be output for that
+path.
+
+SEE ALSO
+--------
+linkgit:gitignore[5]
+linkgit:gitconfig[5]
+linkgit:git-ls-files[5]
+
+GIT
+---
+Part of the linkgit:git[1] suite
diff --git a/Documentation/gitignore.txt b/Documentation/gitignore.txt
index c1f692a..7ba16fe 100644
--- a/Documentation/gitignore.txt
+++ b/Documentation/gitignore.txt
@@ -155,8 +155,10 @@ The second .gitignore prevents git from ignoring
 
 SEE ALSO
 --------
-linkgit:git-rm[1], linkgit:git-update-index[1],
-linkgit:gitrepository-layout[5]
+linkgit:git-rm[1],
+linkgit:git-update-index[1],
+linkgit:gitrepository-layout[5],
+linkgit:git-check-ignore[1]
 
 GIT
 ---
diff --git a/Makefile b/Makefile
index 272ab69..ce5e42f 100644
--- a/Makefile
+++ b/Makefile
@@ -825,6 +825,7 @@ BUILTIN_OBJS += builtin/branch.o
 BUILTIN_OBJS += builtin/bundle.o
 BUILTIN_OBJS += builtin/cat-file.o
 BUILTIN_OBJS += builtin/check-attr.o
+BUILTIN_OBJS += builtin/check-ignore.o
 BUILTIN_OBJS += builtin/check-ref-format.o
 BUILTIN_OBJS += builtin/checkout-index.o
 BUILTIN_OBJS += builtin/checkout.o
diff --git a/builtin.h b/builtin.h
index 8e37752..f812c97 100644
--- a/builtin.h
+++ b/builtin.h
@@ -57,6 +57,7 @@ extern int cmd_cat_file(int argc, const char **argv, const char *prefix);
 extern int cmd_checkout(int argc, const char **argv, const char *prefix);
 extern int cmd_checkout_index(int argc, const char **argv, const char *prefix);
 extern int cmd_check_attr(int argc, const char **argv, const char *prefix);
+extern int cmd_check_ignore(int argc, const char **argv, const char *prefix);
 extern int cmd_check_ref_format(int argc, const char **argv, const char *prefix);
 extern int cmd_cherry(int argc, const char **argv, const char *prefix);
 extern int cmd_cherry_pick(int argc, const char **argv, const char *prefix);
diff --git a/builtin/add.c b/builtin/add.c
index a7ed2ad..af68c32 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -273,7 +273,7 @@ static int add_files(struct dir_struct *dir, int flags)
 		fprintf(stderr, _(ignore_error));
 		for (i = 0; i < dir->ignored_nr; i++)
 			fprintf(stderr, "%s\n", dir->ignored[i]->name);
-		fprintf(stderr, _("Use -f if you really want to add them.\n"));
+		fprintf(stderr, _("Use -f if you really want to add them, or git check-ignore to see\nwhy they're ignored.\n"));
 		die(_("no files added"));
 	}
 
diff --git a/builtin/check-ignore.c b/builtin/check-ignore.c
new file mode 100644
index 0000000..146c00b
--- /dev/null
+++ b/builtin/check-ignore.c
@@ -0,0 +1,150 @@
+#include "builtin.h"
+#include "cache.h"
+#include "dir.h"
+#include "quote.h"
+#include "pathspec.h"
+#include "parse-options.h"
+
+static int stdin_paths;
+static const char * const check_ignore_usage[] = {
+"git check-ignore pathname...",
+"git check-ignore --stdin [-z] < <list-of-paths>",
+NULL
+};
+
+static int null_term_line;
+
+static const struct option check_ignore_options[] = {
+	OPT_BOOLEAN(0 , "stdin", &stdin_paths, "read file names from stdin"),
+	OPT_BOOLEAN('z', NULL, &null_term_line,
+		"input paths are terminated by a null character"),
+	OPT_END()
+};
+
+static void output_exclude(const char *path, struct exclude *exclude)
+{
+	char *type = exclude->to_exclude ? "excluded" : "included";
+	char *bang = exclude->to_exclude ? "" : "!";
+	char *dir  = (exclude->flags & EXC_FLAG_MUSTBEDIR) ? "/" : "";
+	printf(_("%s: %s %s%s%s "), path, type, bang, exclude->pattern, dir);
+	if (exclude->srcpos > 0) {
+		printf("%s %d", exclude->src, exclude->srcpos);
+	}
+	else {
+		/* Exclude was from CLI parameter.  This code path is
+		 * currently impossible to hit, but later on we might
+		 * want to add ignore tracing to other commands such
+		 * as git clean, which does accept --exclude.
+		 */
+		/* printf("%s %d", exclude->src, -exclude->srcpos); */
+	}
+	printf("\n");
+}
+
+static void check_ignore(const char *prefix, const char **pathspec)
+{
+	struct dir_struct dir;
+	const char *path;
+	char *seen = NULL;
+
+	/* read_cache() is only necessary so we can watch out for submodules. */
+	if (read_cache() < 0)
+		die(_("index file corrupt"));
+
+	memset(&dir, 0, sizeof(dir));
+	dir.flags |= DIR_COLLECT_IGNORED;
+	setup_standard_excludes(&dir);
+
+	if (pathspec) {
+		int i;
+		struct path_exclude_check check;
+		struct exclude *exclude;
+
+		path_exclude_check_init(&check, &dir);
+		if (!seen)
+			seen = find_used_pathspec(pathspec);
+		for (i = 0; pathspec[i]; i++) {
+			path = pathspec[i];
+			char *full_path =
+				prefix_path(prefix, prefix ? strlen(prefix) : 0, path);
+			treat_gitlink(full_path);
+			validate_path(prefix, full_path);
+			if (!seen[i] && path[0]) {
+				int dtype = DT_UNKNOWN;
+				exclude = path_excluded_1(&check, full_path, -1, &dtype);
+				if (exclude) {
+					output_exclude(path, exclude);
+				}
+			}
+		}
+		free(seen);
+		free_directory(&dir);
+		path_exclude_check_clear(&check);
+	}
+	else {
+		printf("no pathspec\n");
+	}
+}
+
+static void check_ignore_stdin_paths(const char *prefix)
+{
+	struct strbuf buf, nbuf;
+	char **pathspec = NULL;
+	size_t nr = 0, alloc = 0;
+	int line_termination = null_term_line ? 0 : '\n';
+
+	strbuf_init(&buf, 0);
+	strbuf_init(&nbuf, 0);
+	while (strbuf_getline(&buf, stdin, line_termination) != EOF) {
+		if (line_termination && buf.buf[0] == '"') {
+			strbuf_reset(&nbuf);
+			if (unquote_c_style(&nbuf, buf.buf, NULL))
+				die("line is badly quoted");
+			strbuf_swap(&buf, &nbuf);
+		}
+		ALLOC_GROW(pathspec, nr + 1, alloc);
+		pathspec[nr] = xcalloc(strlen(buf.buf) + 1, sizeof(*buf.buf));
+		strcpy(pathspec[nr++], buf.buf);
+	}
+	ALLOC_GROW(pathspec, nr + 1, alloc);
+	pathspec[nr] = NULL;
+	check_ignore(prefix, (const char **)pathspec);
+	maybe_flush_or_die(stdout, "attribute to stdout");
+	strbuf_release(&buf);
+	strbuf_release(&nbuf);
+	free(pathspec);
+}
+
+static NORETURN void error_with_usage(const char *msg)
+{
+	error("%s", msg);
+	usage_with_options(check_ignore_usage, check_ignore_options);
+}
+
+int cmd_check_ignore(int argc, const char **argv, const char *prefix)
+{
+	git_config(git_default_config, NULL);
+
+	argc = parse_options(argc, argv, prefix, check_ignore_options,
+			     check_ignore_usage, 0);
+
+	if (stdin_paths) {
+		if (0 < argc)
+			error_with_usage("Can't specify files with --stdin");
+	} else {
+		if (null_term_line)
+			error_with_usage("-z only makes sense with --stdin");
+
+		if (argc == 0)
+			error_with_usage("No path specified");
+	}
+
+	if (stdin_paths)
+		check_ignore_stdin_paths(prefix);
+	else {
+		check_ignore(prefix, argv);
+		maybe_flush_or_die(stdout, "ignore to stdout");
+	}
+
+	return 0;
+}
diff --git a/command-list.txt b/command-list.txt
index 7e8cfec..bf83303 100644
--- a/command-list.txt
+++ b/command-list.txt
@@ -12,6 +12,7 @@ git-branch                              mainporcelain common
 git-bundle                              mainporcelain
 git-cat-file                            plumbinginterrogators
 git-check-attr                          purehelpers
+git-check-ignore                        purehelpers
 git-checkout                            mainporcelain common
 git-checkout-index                      plumbingmanipulators
 git-check-ref-format                    purehelpers
diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 222b804..26d798f 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -594,6 +594,7 @@ __git_list_porcelain_commands ()
 		archimport)       : import;;
 		cat-file)         : plumbing;;
 		check-attr)       : plumbing;;
+		check-ignore)     : plumbing;;
 		check-ref-format) : plumbing;;
 		checkout-index)   : plumbing;;
 		commit-tree)      : plumbing;;
diff --git a/git.c b/git.c
index 8788b32..6caf78a 100644
--- a/git.c
+++ b/git.c
@@ -338,6 +338,7 @@ static void handle_internal_command(int argc, const char **argv)
 		{ "bundle", cmd_bundle, RUN_SETUP_GENTLY },
 		{ "cat-file", cmd_cat_file, RUN_SETUP },
 		{ "check-attr", cmd_check_attr, RUN_SETUP },
+		{ "check-ignore", cmd_check_ignore, RUN_SETUP | NEED_WORK_TREE },
 		{ "check-ref-format", cmd_check_ref_format },
 		{ "checkout", cmd_checkout, RUN_SETUP | NEED_WORK_TREE },
 		{ "checkout-index", cmd_checkout_index,
diff --git a/t/t0007-ignores.sh b/t/t0007-ignores.sh
new file mode 100755
index 0000000..4407634
--- /dev/null
+++ b/t/t0007-ignores.sh
@@ -0,0 +1,301 @@
+#!/bin/sh
+
+test_description=gitignores
+
+. ./test-lib.sh
+
+init_vars () {
+	global_excludes="$HOME/global-excludes"
+}
+
+enable_global_excludes () {
+	init_vars
+	git config core.excludesfile "$global_excludes"
+}
+
+ignore_check () {
+	paths="$1" expected="$2" global_args="$3"
+
+	if test -z "$expected"; then
+	    >"$HOME/expected" # avoid newline
+	else
+	    echo "$expected" >"$HOME/expected"
+	fi &&
+	run_check_ignore "$paths" "$global_args"
+}
+
+expect () {
+	echo "$*" >"$HOME/expected"
+}
+
+run_check_ignore () {
+	args="$1" global_args="$2"
+
+	init_vars &&
+	rm -f "$HOME/stdout" "$HOME/stderr" "$HOME/cmd" &&
+	echo `which git` $global_args check-ignore $args >"$HOME/cmd" &&
+	pwd >"$HOME/pwd" &&
+	git $global_args check-ignore $args >"$HOME/stdout" 2>"$HOME/stderr" &&
+	test_cmp "$HOME/expected" "$HOME/stdout" &&
+	test_line_count = 0 "$HOME/stderr"
+}
+
+test_expect_success 'setup' '
+	init_vars
+	mkdir -p a/b/ignored-dir a/submodule b &&
+	ln -s b a/symlink &&
+	(
+		cd a/submodule &&
+		git init &&
+		echo a > a &&
+		git add a &&
+		git commit -m"commit in submodule"
+	) &&
+	git add a/submodule &&
+	cat <<-EOF >.gitignore &&
+		one
+	EOF
+	cat <<-EOF >a/.gitignore &&
+		two*
+		*three
+	EOF
+	cat <<-EOF >a/b/.gitignore &&
+		four
+		five
+		# this comment should affect the line numbers
+		six
+		ignored-dir/
+		# and so should this blank line:
+
+		!on*
+		!two
+	EOF
+	echo "seven" >a/b/ignored-dir/.gitignore &&
+	test -n "$HOME" &&
+	cat <<-EOF >"$global_excludes"
+		globalone
+		!globaltwo
+		globalthree
+	EOF
+'
+
+test_expect_success 'empty command line' '
+	test_must_fail git check-ignore 2>"$HOME/stderr" &&
+	grep -q "error: No path specified" "$HOME/stderr"
+'
+
+test_expect_success 'erroneous use of --' '
+	test_must_fail git check-ignore -- 2>"$HOME/stderr" &&
+	grep -q "error: No path specified" "$HOME/stderr"
+'
+
+test_expect_success '--stdin with superfluous arg' '
+	test_must_fail git check-ignore --stdin foo 2>"$HOME/stderr" &&
+	grep -q "Can'\''t specify files with --stdin" "$HOME/stderr"
+'
+
+test_expect_success '--stdin -z with superfluous arg' '
+	test_must_fail git check-ignore --stdin -z foo 2>"$HOME/stderr" &&
+	grep -q "Can'\''t specify files with --stdin" "$HOME/stderr"
+'
+
+test_expect_success '-z without --stdin' '
+	test_must_fail git check-ignore -z 2>"$HOME/stderr" &&
+	grep -q "error: -z only makes sense with --stdin" "$HOME/stderr"
+'
+
+test_expect_success '-z without --stdin and superfluous arg' '
+	test_must_fail git check-ignore -z foo 2>"$HOME/stderr" &&
+	grep -q "error: -z only makes sense with --stdin" "$HOME/stderr"
+'
+
+test_expect_success 'needs work tree' '
+	(
+		cd .git &&
+		test_must_fail git check-ignore foo 2>"$HOME/stderr"
+	) &&
+	grep -q "fatal: This operation must be run in a work tree" "$HOME/stderr"
+
+'
+test_expect_success 'top-level not ignored' '
+	ignore_check foo ""
+'
+
+test_expect_success 'top-level ignored' '
+	ignore_check one "one: excluded one .gitignore 1"
+'
+
+test_expect_success 'sub-directory ignore from top' '
+	expect "a/one: excluded one .gitignore 1" &&
+	run_check_ignore a/one
+'
+
+test_expect_success 'sub-directory local ignore' '
+	expect "a/3-three: excluded *three a/.gitignore 2" &&
+	run_check_ignore "a/3-three a/three-not-this-one"
+'
+
+test_expect_success 'sub-directory local ignore inside a' '
+	expect "3-three: excluded *three a/.gitignore 2" &&
+	(
+		cd a &&
+		run_check_ignore "3-three three-not-this-one"
+	)
+'
+
+test_expect_success 'nested include' '
+	expect "a/b/one: included !on* a/b/.gitignore 8" &&
+	run_check_ignore "a/b/one"
+'
+
+test_expect_success 'ignored sub-directory' '
+	expect "a/b/ignored-dir: excluded ignored-dir/ a/b/.gitignore 5" &&
+	run_check_ignore "a/b/ignored-dir"
+'
+
+test_expect_success 'multiple files inside ignored sub-directory' '
+	cat <<-EOF >"$HOME/expected" &&
+		a/b/ignored-dir/foo: excluded ignored-dir/ a/b/.gitignore 5
+		a/b/ignored-dir/twoooo: excluded ignored-dir/ a/b/.gitignore 5
+		a/b/ignored-dir/seven: excluded ignored-dir/ a/b/.gitignore 5
+	EOF
+	run_check_ignore "a/b/ignored-dir/foo a/b/ignored-dir/twoooo a/b/ignored-dir/seven"
+'
+
+test_expect_success 'cd to ignored sub-directory' '
+	cat <<-EOF >"$HOME/expected" &&
+		foo: excluded ignored-dir/ a/b/.gitignore 5
+		twoooo: excluded ignored-dir/ a/b/.gitignore 5
+		../one: included !on* a/b/.gitignore 8
+		seven: excluded ignored-dir/ a/b/.gitignore 5
+		../../one: excluded one .gitignore 1
+	EOF
+	(
+		cd a/b/ignored-dir &&
+		run_check_ignore "foo twoooo ../one seven ../../one"
+	)
+'
+
+test_expect_success 'symlink' '
+	ignore_check "a/symlink" ""
+'
+
+test_expect_success 'beyond a symlink' '
+	test_must_fail git check-ignore "a/symlink/foo"
+'
+
+test_expect_success 'beyond a symlink from subdirectory' '
+	(
+		cd a &&
+		test_must_fail git check-ignore "symlink/foo"
+	)
+'
+
+test_expect_success 'submodule' '
+	test_must_fail git check-ignore "a/submodule/one" 2>"$HOME/stderr" &&
+	expect "fatal: Path '\''a/submodule/one'\'' is in submodule '\''a/submodule'\''" &&
+	test_cmp "$HOME/expected" "$HOME/stderr"
+'
+
+test_expect_success 'submodule from subdirectory' '
+	(
+		cd a &&
+		test_must_fail git check-ignore "submodule/one" 2>"$HOME/stderr"
+	) &&
+	expect "fatal: Path '\''a/submodule/one'\'' is in submodule '\''a/submodule'\''" &&
+	test_cmp "$HOME/expected" "$HOME/stderr"
+'
+
+test_expect_success 'global ignore not yet enabled' '
+	expect "a/globalthree: excluded *three a/.gitignore 2" &&
+	run_check_ignore "globalone a/globalthree a/globaltwo"
+'
+
+test_expect_success 'global ignore' '
+	enable_global_excludes &&
+	cat <<-EOF >"$HOME/expected" &&
+		globalone: excluded globalone $global_excludes 1
+		globalthree: excluded globalthree $global_excludes 3
+		a/globalthree: excluded *three a/.gitignore 2
+		globaltwo: included !globaltwo $global_excludes 2
+	EOF
+	run_check_ignore "globalone globalthree a/globalthree globaltwo"
+'
+
+test_expect_success '--stdin' '
+	cat <<-EOF >in.txt &&
+		one
+		a/one
+		a/b/on
+		a/b/one
+		a/b/two
+		a/b/twooo
+		globaltwo
+		a/globaltwo
+		a/b/globaltwo
+		b/globaltwo
+	EOF
+	cat <<-EOF >"$HOME/expected" &&
+		one: excluded one .gitignore 1
+		a/one: excluded one .gitignore 1
+		a/b/on: included !on* a/b/.gitignore 8
+		a/b/one: included !on* a/b/.gitignore 8
+		a/b/two: included !two a/b/.gitignore 9
+		a/b/twooo: excluded two* a/.gitignore 1
+		globaltwo: included !globaltwo $global_excludes 2
+		a/globaltwo: included !globaltwo $global_excludes 2
+		a/b/globaltwo: included !globaltwo $global_excludes 2
+		b/globaltwo: included !globaltwo $global_excludes 2
+	EOF
+	run_check_ignore --stdin < in.txt
+'
+
+test_expect_success '--stdin -z' '
+	tr "\n" "\0" < in.txt | run_check_ignore "--stdin -z"
+'
+
+test_expect_success '-z --stdin' '
+	tr "\n" "\0" < in.txt | run_check_ignore "-z --stdin"
+'
+
+test_expect_success '--stdin from subdirectory' '
+	cat <<-EOF >in.txt &&
+		../one
+		one
+		b/on
+		b/one
+		b/two
+		b/twooo
+		../globaltwo
+		globaltwo
+		b/globaltwo
+		../b/globaltwo
+	EOF
+	cat <<-EOF >"$HOME/expected" &&
+		../one: excluded one .gitignore 1
+		one: excluded one .gitignore 1
+		b/on: included !on* a/b/.gitignore 8
+		b/one: included !on* a/b/.gitignore 8
+		b/two: included !two a/b/.gitignore 9
+		b/twooo: excluded two* a/.gitignore 1
+		../globaltwo: included !globaltwo $global_excludes 2
+		globaltwo: included !globaltwo $global_excludes 2
+		b/globaltwo: included !globaltwo $global_excludes 2
+		../b/globaltwo: included !globaltwo $global_excludes 2
+	EOF
+	(
+		cd a &&
+		run_check_ignore --stdin < ../in.txt
+	)
+'
+
+test_expect_success '--stdin -z from subdirectory' '
+	tr "\n" "\0" < in.txt | ( cd a && run_check_ignore "--stdin -z" )
+'
+
+test_expect_success '-z --stdin from subdirectory' '
+	tr "\n" "\0" < in.txt | ( cd a && run_check_ignore "-z --stdin" )
+'
+
+
+test_done
-- 
1.7.12.155.ge5750d5.dirty

Re: [PATCH 9/9] Add git-check-ignores

[Nguyen Thai Ngoc Duy]

On Sun, Sep 2, 2012 at 7:12 AM, Adam Spiers <git@adamspiers.org> wrote:
> This works in a similar manner to git-check-attr.  Some code
> was reused from add.c by refactoring out into pathspec.c.

Thanks, comments from a quick glance. First of all, can we make it
work (or share code) with .gitattributes? We may need to debug
.gitattributes as well as .gitignore. A common command would be nice.

> +SYNOPSIS
> +--------
> +[verse]
> +'git check-ignore' pathname...
> +'git check-ignore' --stdin [-z] < <list-of-paths>

Also --quiet option, where check-ignore returns 0 if the given path is
ignored, 1 otherwise?

> +OUTPUT
> +------
> +
> +The output is a series of lines of the form:
> +
> +<path> COLON SP <type> SP <pattern> SP <source> SP <position> LF
> +
> +<path> is the path of a file being queried, <type> is either
> +'excluded' or 'included' (for patterns prefixed with '!'), <pattern>
> +is the matching pattern, <source> is the pattern's source file (either
> +as an absolute path or relative to the repository root), and
> +<position> is the position of the pattern within that source.

I think we should have a few levels of verbosity.

 - The --quiet I already mention above.
 - If many paths are given, then perhaps we could print ignored paths
(no extra info).
 - Going to the next level, we could print path and the the location
of the final exclude/include rule (file and line number).
 - For debugging, given one path, we print all the rules that are
applied to it, which may help understand how/why it goes wrong.

> @@ -338,6 +338,7 @@ static void handle_internal_command(int argc, const char **argv)
>                 { "bundle", cmd_bundle, RUN_SETUP_GENTLY },
>                 { "cat-file", cmd_cat_file, RUN_SETUP },
>                 { "check-attr", cmd_check_attr, RUN_SETUP },
> +               { "check-ignore", cmd_check_ignore, RUN_SETUP | NEED_WORK_TREE },
>                 { "check-ref-format", cmd_check_ref_format },
>                 { "checkout", cmd_checkout, RUN_SETUP | NEED_WORK_TREE },
>                 { "checkout-index", cmd_checkout_index,

I don't think we really need NEED_WORK_TREE here. .gitignore can be
read from index only.
-- 
Duy

Re: [PATCH 9/9] Add git-check-ignores

[Adam Spiers]

Hi there,

Firstly, thanks for the quick feedback!

On Sun, Sep 2, 2012 at 11:41 AM, Nguyen Thai Ngoc Duy <pclouds@gmail.com> wrote:
> On Sun, Sep 2, 2012 at 7:12 AM, Adam Spiers <git@adamspiers.org> wrote:
>> This works in a similar manner to git-check-attr.  Some code
>> was reused from add.c by refactoring out into pathspec.c.
>
> Thanks, comments from a quick glance. First of all, can we make it
> work (or share code) with .gitattributes? We may need to debug
> .gitattributes as well as .gitignore. A common command would be nice.

I'm no expert on .gitattributes and check-attr, but AFAICS, all the
opportunities to share code in the plumbing and front-end seem to be
taken already, e.g. the directory traversal and path handling.  The
CLI argument parsing is necessarily different because check-attr
requires a list of attributes as well as a list of files, and of
course the output routines have to be different too.

The only opportunity for code reuse which I saw but /didn't/ take was
around the --stdin line parsing code which is duplicated between:

    check_attr_stdin_paths
    check_ignore_stdin_paths
    cmd_checkout_index
    cmd_update_index
    hash_stdin_paths

I attempted to refactor these, but quickly realised that due to the
lack of proper closures in C, the overheads and complexity incurred by
performing such a refactoring probably outweighed the benefits, so I
gave up on the idea.

Having said that, I'm totally open to suggestions if you can spot
other places where code could be reused :)

>> +SYNOPSIS
>> +--------
>> +[verse]
>> +'git check-ignore' pathname...
>> +'git check-ignore' --stdin [-z] < <list-of-paths>
>
> Also --quiet option, where check-ignore returns 0 if the given path is
> ignored, 1 otherwise?

I considered that, but couldn't think of appropriate behaviour when
multiple paths are given, so in the end I decided to remain consistent
with check-attr, which always returns 0.  But I'm happy to change it
if you can think of a more useful behaviour.  For example we could
have a --count option which produces no output but has an exit status
corresponding to the number of ignored files.

>  - If many paths are given, then perhaps we could print ignored paths
> (no extra info).

How is this different to git ls-files -i -o ?

>  - Going to the next level, we could print path and the the location
> of the final exclude/include rule (file and line number).

That's the current behaviour, and I believe it covers the most common
use case.

>  - For debugging, given one path, we print all the rules that are
> applied to it, which may help understand how/why it goes wrong.

That would be nice, but I'm not sure it's a tremendously common use
case.  Could you think of a scenario in which it would be useful?  I
guess it could be done by adding a new DIR_DEBUG_IGNORED flag to
dir_struct which would make the exclude matcher functions collect all
matching patterns, rather than just returning the first one.  This in
turn would require another field for collecting all matched patterns.

>> @@ -338,6 +338,7 @@ static void handle_internal_command(int argc, const char **argv)
>>                 { "bundle", cmd_bundle, RUN_SETUP_GENTLY },
>>                 { "cat-file", cmd_cat_file, RUN_SETUP },
>>                 { "check-attr", cmd_check_attr, RUN_SETUP },
>> +               { "check-ignore", cmd_check_ignore, RUN_SETUP | NEED_WORK_TREE },
>>                 { "check-ref-format", cmd_check_ref_format },
>>                 { "checkout", cmd_checkout, RUN_SETUP | NEED_WORK_TREE },
>>                 { "checkout-index", cmd_checkout_index,
>
> I don't think we really need NEED_WORK_TREE here. .gitignore can be
> read from index only.

I thought about that, but in the end I decided it probably didn't make
sense, because none of the exclude matching routines match against the
index - they all match against the working tree and core.excludesfile.
This would also require changing the matching logic to honor the index,
but I didn't see the benefit in doing that, since all operations which
involve excludes (add, status, etc.) relate to a work tree.

But as with all of the above, please don't hesitate to point out if
I've missed something.  You guys are the experts, not me ;-)

Thanks again,
Adam

Re: [PATCH 9/9] Add git-check-ignores

[Junio C Hamano]

Adam Spiers <git@adamspiers.org> writes:

>>> +SYNOPSIS
>>> +--------
>>> +[verse]
>>> +'git check-ignore' pathname...
>>> +'git check-ignore' --stdin [-z] < <list-of-paths>
>>
>> Also --quiet option, where check-ignore returns 0 if the given path is
>> ignored, 1 otherwise?

I agree that multiple paths are problematic.

We could error out if multiple paths are given with --quiet until we
figure out what the useful result would be in such a case, and still
give a useful answer to callers that feed a single path, though.

That may encourage suboptimal coding to casual Porcelain writers,
i.e. it would allow

	for path in $paths
        do
		if git check-ignore -q "$path"
                then
			do something to "$path"
		fi
	done

even though we would rather want to encourage

	git check-ignore --name-only $paths |
        while read path
        do
		do something to "$path"
	done

But from lay-scriptors' point of view, being able to easily write a
script (even though it may be inefficient) to do the job at hand is
far better than having to give up writing one because the tool does
not allow easy-and-stupid scripting, so it is not exactly a huge
downside.

>>  - If many paths are given, then perhaps we could print ignored paths
>> (no extra info).
>
> How is this different to git ls-files -i -o ?

I personally think the parts of ls-files that deal with paths not in
the index outlived its usefulness ;-) and users deserve to be given
a better UI.

>>  - Going to the next level, we could print path and the the location
>> of the final exclude/include rule (file and line number).
>
> That's the current behaviour, and I believe it covers the most common
> use case.

Yes; I have a reservation on your output format, though.

>>  - For debugging, given one path, we print all the rules that are
>> applied to it, which may help understand how/why it goes wrong.

I do not think that would be terribly useful.  Maybe for people who
are learning how dir.c internally works, but not for people who are
trying to improve the set of .gitignore files in their project.

> I thought about that, but in the end I decided it probably didn't make
> sense, because none of the exclude matching routines match against the
> index - they all match against the working tree and core.excludesfile.
> This would also require changing the matching logic to honor the index,
> but I didn't see the benefit in doing that, since all operations which
> involve excludes (add, status, etc.) relate to a work tree.

The mechanism primarily is to see if a path in the working tree is a
cruft or a valuable still to be added; I am OK with NEED_WORK_TREE;
when we have a useful case to run this in a bare repository, we can
lift it.

As with the "what to do with multiple paths and -q", it is better to
start with feature set to cover only the known or easily anticipated
use cases, rejecting the cases for which good semantics are not
thought out.

An alternative would be a code that operates sanely only for known
or anticipated cases and do random things with irrational semantics
in other cases, and people start relying on the irrational behaviour
without realizing their input and the behaviour they are seeing are
not something that the feature is designed to for, but whatever the
code with loose precondition checking happens to do.  We do not want
to repeat that kind of mistake, which is hard to fix in future
versions.

Re: [PATCH 9/9] Add git-check-ignores

[Nguyen Thai Ngoc Duy]

On Sun, Sep 2, 2012 at 9:50 PM, Adam Spiers <git@adamspiers.org> wrote:
> I'm no expert on .gitattributes and check-attr, but AFAICS, all the
> opportunities to share code in the plumbing and front-end seem to be
> taken already, e.g. the directory traversal and path handling.  The
> CLI argument parsing is necessarily different because check-attr
> requires a list of attributes as well as a list of files, and of
> course the output routines have to be different too.
>
> The only opportunity for code reuse which I saw but /didn't/ take was
> around the --stdin line parsing code which is duplicated between:
>
>     check_attr_stdin_paths
>     check_ignore_stdin_paths
>     cmd_checkout_index
>     cmd_update_index
>     hash_stdin_paths
>
> I attempted to refactor these, but quickly realised that due to the
> lack of proper closures in C, the overheads and complexity incurred by
> performing such a refactoring probably outweighed the benefits, so I
> gave up on the idea.
>
> Having said that, I'm totally open to suggestions if you can spot
> other places where code could be reused :)

Yeah. That was my impression too. I just hoped a new set of eyes might
discover something ;) At lease we could prepare the output format that
can be reused (maybe with little changes) for check-attr debugging if
it comes later. Or make this command part of check-attr..

>> Also --quiet option, where check-ignore returns 0 if the given path is
>> ignored, 1 otherwise?
>
> I considered that, but couldn't think of appropriate behaviour when
> multiple paths are given, so in the end I decided to remain consistent
> with check-attr, which always returns 0.  But I'm happy to change it
> if you can think of a more useful behaviour.  For example we could
> have a --count option which produces no output but has an exit status
> corresponding to the number of ignored files.

We could take this opportunity to kill "add --ignore-missing", which
is basically .gitignore checking and it accepts multiple paths, I
think.

>>  - If many paths are given, then perhaps we could print ignored paths
>> (no extra info).
>
> How is this different to git ls-files -i -o ?

I think ls-files requires real files on working directory, but
check-ignore can deal with just non-existing paths.

>>  - For debugging, given one path, we print all the rules that are
>> applied to it, which may help understand how/why it goes wrong.
>
> That would be nice, but I'm not sure it's a tremendously common use
> case.  Could you think of a scenario in which it would be useful?  I
> guess it could be done by adding a new DIR_DEBUG_IGNORED flag to
> dir_struct which would make the exclude matcher functions collect all
> matching patterns, rather than just returning the first one.  This in
> turn would require another field for collecting all matched patterns.

Mixing include/exclude ignore rules multiple times could be hard to
figure out what goes wrong. But as we haven't seen an actual use case
yet, just leave it out.

>> I don't think we really need NEED_WORK_TREE here. .gitignore can be
>> read from index only.
>
> I thought about that, but in the end I decided it probably didn't make
> sense, because none of the exclude matching routines match against the
> index - they all match against the working tree and core.excludesfile.
> This would also require changing the matching logic to honor the index,
> but I didn't see the benefit in doing that, since all operations which
> involve excludes (add, status, etc.) relate to a work tree.
>
> But as with all of the above, please don't hesitate to point out if
> I've missed something.  You guys are the experts, not me ;-)

Again I was thinking that check-ignore could work with imaginary paths
and could be used by scripts. If a script just wants to check certain
paths are excluded, it should not need to move to working directory
(though it probably is in working directory most of the time).
-- 
Duy

Re: [PATCH 9/9] Add git-check-ignores

[Junio C Hamano]

Adam Spiers <git@adamspiers.org> writes:

> +OPTIONS
> +-------
> +--stdin::
> +	Read file names from stdin instead of from the command-line.
> +
> +-z::
> +	Only meaningful with `--stdin`; paths are separated with a
> +	NUL character instead of a linefeed character.

On input, or on output, or both?

The answer should be "both", otherwise you cannot safely handle
paths with funny character in your script, even if you wanted to.
Which means that this cannot only be meaningful with "--stdin", I
think.

> +OUTPUT
> +------
> +
> +The output is a series of lines of the form:
> +
> +<path> COLON SP <type> SP <pattern> SP <source> SP <position> LF
> +
> +<path> is the path of a file being queried, <type> is either
> +'excluded' or 'included' (for patterns prefixed with '!'), <pattern>
> +is the matching pattern, <source> is the pattern's source file (either
> +as an absolute path or relative to the repository root), and
> +<position> is the position of the pattern within that source.

Let's step back a bit and think what this command is about.  What is
the reason why the user wants to run "check-ignore $path" in the
first place?  I think there are two (or three, depending on how you
count).

 (1) You have one (or more) paths at hand.  You want to know if it
     is (or some of them are) ignored, but you do not particularly
     care how they are ignored.  Think of implementing your own "git
     add" as a script.

 (2) You have one or more paths that are ignored but do not want
     them to be, and want to find out why they are.  

For the former, your script may want to see the paths sifted into
"ignored" bin and "not-ignored" bin, so

	git check-ignore [-z] --name-only $paths

that gives you only the paths without any reason is more useful.
You also may want the opposite (show only paths not ignored), but
that can be computed easily by the script, so it is of lessor
importance.

For the latter, you are debugging the set of exclude sources and
want to learn where the decision to exclude it comes from.  For that
kind of use, it would be more useful if the output mimicked error
messages from the compilers and output from "grep -n" to show the
source, e.g.

	.gitignore:13:/git-am	git-am

Emacs users can use "M-x grep<RET>git check-ignore -v git-am<RET>",
see the output, and find the hit in its output (I would imagine vim
would have a similar feature).  The output format would be something
like:

	<source> <COLON> <linenum> <COLON> <pattern> <HT> <pathname>

I do not think you need excluded/included <type> as a separate item
in the non "-z" output, as it should be clear from the <pattern>.
Substitute "<cmdline>" (literally) as source for patterns obtained
from the command line.

I would also suggest to

 (1) make --name-only the default (i.e. no need to have the
     "--name-only" option);

 (2) give the version that mimicks "grep -n" when "-v|--verbose" is
     given; and

 (3) support "--quiet"; the command would exit with status 0 if
     _any_ of the paths given is ignored, or status 1 if none of the
     paths is ignored (or error out with die() when --quiet and
     multiple paths are given).

Regarding "-z" (for script consumption), I do not object to the
broken down format, e.g., "check-ignore -z -v" may give a sequence
of

    <pathname> NUL <type> NUL <pattern> NUL <source> NUL <position> NUL

while "check-ignore -z" would give a sequence of

    <pathname> NUL

Re: [PATCH 9/9] Add git-check-ignores

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Let's step back a bit and think what this command is about.  What is
> the reason why the user wants to run "check-ignore $path" in the
> first place?  I think there are two (or three, depending on how you
> count).
>
>  (1) You have one (or more) paths at hand.  You want to know if it
>      is (or some of them are) ignored, but you do not particularly
>      care how they are ignored.  Think of implementing your own "git
>      add" as a script.
>
>  (2) You have one or more paths that are ignored but do not want
>      them to be, and want to find out why they are.  

A reason related to (2) is to find out why the paths you want to be
excluded are included, so that you can fix it by disabling an entry
in .gitignore that covers too widely, or by adding a new entry to
override it.

For that to work, the "-v" mode needs to show all paths that were
given from the command line (or --stdin), to explain why each of
them is ignored or not ignored.  Hence, in addition:

> For the latter, you are debugging the set of exclude sources and
> want to learn where the decision to exclude it comes from.  For that
> kind of use, it would be more useful if the output mimicked error
> messages from the compilers and output from "grep -n" to show the
> source, e.g.
>
> 	.gitignore:13:/git-am	git-am

we would need an entry that shows !<include pattern> in the output.

A path that does not match any pattern anywhere in the exclude files
is taken as included---I am not sure what is the best way to explain
the reason why they are included.  If we are going to show the entry
that finally matched (either with negative or positive pattern) and
decided the "fate" as the explanation for both excluded and included
paths, perhaps not showing an entry for such a "never matched" path
might be a good enough explanation.

[PATCH v2 00/14] new git check-ignore sub-command

[Adam Spiers]

This is a re-vamp of my original check-ignore series, which aims to
address all the feedback which was raised in the first round of
reviews.  The most notable changes are the CLI options and output
formats as suggested by Junio and Nguyễn; now there are three levels
of verbosity: --quiet, default, and --verbose.  -z also now affects
the output and so is now compatible with the --stdin optin.

Some commits have been broken into smaller pieces to facilitate easier
reviews, and based on an earlier discussion, three exclude functions
have been given an 'is_' prefix to clarify their boolean nature.  The
helper functions extracted from these three now have more meaningful
names rather than just a '_1' suffix.

Other minor issues, such as inconsistent coding style, have been
fixed, and the modification to the output text in add.c has been
scrapped.

It has been rebased on the latest master, and passed a full test run.

Adam Spiers (14):
  Update directory listing API doc to match code
  Improve documentation and comments regarding directory traversal API
  Rename cryptic 'which' variable to more consistent name
  Rename path_excluded() to is_path_excluded()
  Rename excluded_from_list() to is_excluded_from_list()
  Rename excluded() to is_excluded()
  Refactor is_excluded_from_list()
  Refactor is_excluded()
  Refactor is_path_excluded()
  For each exclude pattern, store information about where it came from
  Refactor treat_gitlinks()
  Extract some useful pathspec handling code from builtin/add.c into a
    library
  Provide free_directory() for reclaiming dir_struct memory
  Add git-check-ignore sub-command

 .gitignore                                        |   1 +
 Documentation/git-check-ignore.txt                |  85 ++++
 Documentation/gitignore.txt                       |   6 +-
 Documentation/technical/api-directory-listing.txt |  23 +-
 Makefile                                          |   3 +
 attr.c                                            |   2 +-
 builtin.h                                         |   1 +
 builtin/add.c                                     |  84 +---
 builtin/check-ignore.c                            | 167 ++++++
 builtin/clean.c                                   |   2 +-
 builtin/ls-files.c                                |   5 +-
 command-list.txt                                  |   1 +
 contrib/completion/git-completion.bash            |   1 +
 dir.c                                             | 191 +++++--
 dir.h                                             |  47 +-
 git.c                                             |   1 +
 pathspec.c                                        |  97 ++++
 pathspec.h                                        |   6 +
 t/t0007-ignores.sh                                | 587 ++++++++++++++++++++++
 t/t9902-completion.sh                             |  24 +-
 unpack-trees.c                                    |  10 +-
 21 files changed, 1182 insertions(+), 162 deletions(-)
 create mode 100644 Documentation/git-check-ignore.txt
 create mode 100644 builtin/check-ignore.c
 create mode 100644 pathspec.c
 create mode 100644 pathspec.h
 create mode 100755 t/t0007-ignores.sh

-- 
1.7.12.147.g6d168f4

[PATCH v2 01/14] Update directory listing API doc to match code

[Adam Spiers]

7c4c97c0ac turned the flags in struct dir_struct into a single bitfield
variable, but forgot to update this document.

Signed-off-by: Adam Spiers <git@adamspiers.org>
---
 Documentation/technical/api-directory-listing.txt | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/Documentation/technical/api-directory-listing.txt b/Documentation/technical/api-directory-listing.txt
index add6f43..0356d25 100644
--- a/Documentation/technical/api-directory-listing.txt
+++ b/Documentation/technical/api-directory-listing.txt
@@ -17,24 +17,24 @@ options are:
 	The name of the file to be read in each directory for excluded
 	files (typically `.gitignore`).
 
-`collect_ignored`::
+`flags`::
 
-	Include paths that are to be excluded in the result.
+	A bit-field of options:
 
-`show_ignored`::
+`DIR_SHOW_IGNORED`:::
 
 	The traversal is for finding just ignored files, not unignored
 	files.
 
-`show_other_directories`::
+`DIR_SHOW_OTHER_DIRECTORIES`:::
 
 	Include a directory that is not tracked.
 
-`hide_empty_directories`::
+`DIR_HIDE_EMPTY_DIRECTORIES`:::
 
 	Do not include a directory that is not tracked and is empty.
 
-`no_gitlinks`::
+`DIR_NO_GITLINKS`:::
 
 	If set, recurse into a directory that looks like a git
 	directory.  Otherwise it is shown as a directory.
-- 
1.7.12.147.g6d168f4

[PATCH v2 02/14] Improve documentation and comments regarding directory traversal API

[Adam Spiers]

>From the perspective of a newcomer to the codebase, the directory
traversal API has a few potentially confusing properties.  These
comments clarify a few key aspects and will hopefully make it easier
to understand for other newcomers in the future.

Signed-off-by: Adam Spiers <git@adamspiers.org>
---
 Documentation/technical/api-directory-listing.txt |  9 +++++---
 dir.c                                             |  8 ++++++-
 dir.h                                             | 26 +++++++++++++++++++++--
 3 files changed, 37 insertions(+), 6 deletions(-)

diff --git a/Documentation/technical/api-directory-listing.txt b/Documentation/technical/api-directory-listing.txt
index 0356d25..944fc39 100644
--- a/Documentation/technical/api-directory-listing.txt
+++ b/Documentation/technical/api-directory-listing.txt
@@ -9,8 +9,11 @@ Data structure
 --------------
 
 `struct dir_struct` structure is used to pass directory traversal
-options to the library and to record the paths discovered.  The notable
-options are:
+options to the library and to record the paths discovered.  A single
+`struct dir_struct` is used regardless of whether or not the traversal
+recursively descends into subdirectories.
+
+The notable options are:
 
 `exclude_per_dir`::
 
@@ -39,7 +42,7 @@ options are:
 	If set, recurse into a directory that looks like a git
 	directory.  Otherwise it is shown as a directory.
 
-The result of the enumeration is left in these fields::
+The result of the enumeration is left in these fields:
 
 `entries[]`::
 
diff --git a/dir.c b/dir.c
index 4868339..98d1995 100644
--- a/dir.c
+++ b/dir.c
@@ -2,6 +2,8 @@
  * This handles recursive filename detection with exclude
  * files, index knowledge etc..
  *
+ * See Documentation/technical/api-directory-listing.txt
+ *
  * Copyright (C) Linus Torvalds, 2005-2006
  *		 Junio Hamano, 2005-2006
  */
@@ -451,6 +453,10 @@ void add_excludes_from_file(struct dir_struct *dir, const char *fname)
 		die("cannot use %s as an exclude file", fname);
 }
 
+/*
+ * Loads the per-directory exclude list for the substring of base
+ * which has a char length of baselen.
+ */
 static void prep_exclude(struct dir_struct *dir, const char *base, int baselen)
 {
 	struct exclude_list *el;
@@ -461,7 +467,7 @@ static void prep_exclude(struct dir_struct *dir, const char *base, int baselen)
 	    (baselen + strlen(dir->exclude_per_dir) >= PATH_MAX))
 		return; /* too long a path -- ignore */
 
-	/* Pop the ones that are not the prefix of the path being checked. */
+	/* Pop the directories that are not the prefix of the path being checked. */
 	el = &dir->exclude_list[EXC_DIRS];
 	while ((stk = dir->exclude_stack) != NULL) {
 		if (stk->baselen <= baselen &&
diff --git a/dir.h b/dir.h
index 893465a..a226fbc 100644
--- a/dir.h
+++ b/dir.h
@@ -1,6 +1,8 @@
 #ifndef DIR_H
 #define DIR_H
 
+/* See Documentation/technical/api-directory-listing.txt */
+
 #include "strbuf.h"
 
 struct dir_entry {
@@ -12,6 +14,12 @@ struct dir_entry {
 #define EXC_FLAG_ENDSWITH 4
 #define EXC_FLAG_MUSTBEDIR 8
 
+/*
+ * Each .gitignore file will be parsed into patterns which are then
+ * appended to the relevant exclude_list (either EXC_DIRS or
+ * EXC_FILE).  exclude_lists are also used to represent the list of
+ * --exclude values passed via CLI args (EXC_CMDL).
+ */
 struct exclude_list {
 	int nr;
 	int alloc;
@@ -26,9 +34,15 @@ struct exclude_list {
 	} **excludes;
 };
 
+/*
+ * The contents of the per-directory exclude files are lazily read on
+ * demand and then cached in memory, one per exclude_stack struct, in
+ * order to avoid opening and parsing each one every time that
+ * directory is traversed.
+ */
 struct exclude_stack {
-	struct exclude_stack *prev;
-	char *filebuf;
+	struct exclude_stack *prev; /* the struct exclude_stack for the parent directory */
+	char *filebuf; /* remember pointer to per-directory exclude file contents so we can free() */
 	int baselen;
 	int exclude_ix;
 };
@@ -59,6 +73,14 @@ struct dir_struct {
 #define EXC_DIRS 1
 #define EXC_FILE 2
 
+	/*
+	 * Temporary variables which are used during loading of the
+	 * per-directory exclude lists.
+	 *
+	 * exclude_stack points to the top of the exclude_stack, and
+	 * basebuf contains the full path to the current
+	 * (sub)directory in the traversal.
+	 */
 	struct exclude_stack *exclude_stack;
 	char basebuf[PATH_MAX];
 };
-- 
1.7.12.147.g6d168f4

[PATCH v2 03/14] Rename cryptic 'which' variable to more consistent name

[Adam Spiers]

'el' is only *slightly* less cryptic, but is already used as the
variable name for a struct exclude_list pointer in numerous other
places, so this reduces the number of cryptic variable names in use by
one :-)

Signed-off-by: Adam Spiers <git@adamspiers.org>
---
 dir.c | 10 +++++-----
 dir.h |  4 ++--
 2 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/dir.c b/dir.c
index 98d1995..91e57d9 100644
--- a/dir.c
+++ b/dir.c
@@ -311,7 +311,7 @@ static int no_wildcard(const char *string)
 }
 
 void add_exclude(const char *string, const char *base,
-		 int baselen, struct exclude_list *which)
+		 int baselen, struct exclude_list *el)
 {
 	struct exclude *x;
 	size_t len;
@@ -346,8 +346,8 @@ void add_exclude(const char *string, const char *base,
 	x->nowildcardlen = simple_length(string);
 	if (*string == '*' && no_wildcard(string+1))
 		x->flags |= EXC_FLAG_ENDSWITH;
-	ALLOC_GROW(which->excludes, which->nr + 1, which->alloc);
-	which->excludes[which->nr++] = x;
+	ALLOC_GROW(el->excludes, el->nr + 1, el->alloc);
+	el->excludes[el->nr++] = x;
 }
 
 static void *read_skip_worktree_file_from_index(const char *path, size_t *size)
@@ -389,7 +389,7 @@ int add_excludes_from_file_to_list(const char *fname,
 				   const char *base,
 				   int baselen,
 				   char **buf_p,
-				   struct exclude_list *which,
+				   struct exclude_list *el,
 				   int check_index)
 {
 	struct stat st;
@@ -438,7 +438,7 @@ int add_excludes_from_file_to_list(const char *fname,
 		if (buf[i] == '\n') {
 			if (entry != buf + i && entry[0] != '#') {
 				buf[i - (i && buf[i-1] == '\r')] = 0;
-				add_exclude(entry, base, baselen, which);
+				add_exclude(entry, base, baselen, el);
 			}
 			entry = buf + i + 1;
 		}
diff --git a/dir.h b/dir.h
index a226fbc..549a187 100644
--- a/dir.h
+++ b/dir.h
@@ -117,10 +117,10 @@ extern int path_excluded(struct path_exclude_check *, const char *, int namelen,
 
 
 extern int add_excludes_from_file_to_list(const char *fname, const char *base, int baselen,
-					  char **buf_p, struct exclude_list *which, int check_index);
+					  char **buf_p, struct exclude_list *el, int check_index);
 extern void add_excludes_from_file(struct dir_struct *, const char *fname);
 extern void add_exclude(const char *string, const char *base,
-			int baselen, struct exclude_list *which);
+			int baselen, struct exclude_list *el);
 extern void free_excludes(struct exclude_list *el);
 extern int file_exists(const char *);
 
-- 
1.7.12.147.g6d168f4

[PATCH v2 04/14] Rename path_excluded() to is_path_excluded()

[Adam Spiers]

Start adopting clearer names for exclude functions.  This 'is_*'
naming pattern for functions returning booleans was agreed here:

http://thread.gmane.org/gmane.comp.version-control.git/204661/focus=204924

Signed-off-by: Adam Spiers <git@adamspiers.org>
---
 builtin/add.c      | 2 +-
 builtin/ls-files.c | 2 +-
 dir.c              | 4 ++--
 dir.h              | 2 +-
 unpack-trees.c     | 2 +-
 5 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/builtin/add.c b/builtin/add.c
index e664100..075312a 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -454,7 +454,7 @@ int cmd_add(int argc, const char **argv, const char *prefix)
 			    && !file_exists(pathspec[i])) {
 				if (ignore_missing) {
 					int dtype = DT_UNKNOWN;
-					if (path_excluded(&check, pathspec[i], -1, &dtype))
+					if (is_path_excluded(&check, pathspec[i], -1, &dtype))
 						dir_add_ignored(&dir, pathspec[i], strlen(pathspec[i]));
 				} else
 					die(_("pathspec '%s' did not match any files"),
diff --git a/builtin/ls-files.c b/builtin/ls-files.c
index b5434af..d226456 100644
--- a/builtin/ls-files.c
+++ b/builtin/ls-files.c
@@ -203,7 +203,7 @@ static void show_ru_info(void)
 static int ce_excluded(struct path_exclude_check *check, struct cache_entry *ce)
 {
 	int dtype = ce_to_dtype(ce);
-	return path_excluded(check, ce->name, ce_namelen(ce), &dtype);
+	return is_path_excluded(check, ce->name, ce_namelen(ce), &dtype);
 }
 
 static void show_files(struct dir_struct *dir)
diff --git a/dir.c b/dir.c
index 91e57d9..dad1582 100644
--- a/dir.c
+++ b/dir.c
@@ -627,8 +627,8 @@ void path_exclude_check_clear(struct path_exclude_check *check)
  * A path to a directory known to be excluded is left in check->path to
  * optimize for repeated checks for files in the same excluded directory.
  */
-int path_excluded(struct path_exclude_check *check,
-		  const char *name, int namelen, int *dtype)
+int is_path_excluded(struct path_exclude_check *check,
+		     const char *name, int namelen, int *dtype)
 {
 	int i;
 	struct strbuf *path = &check->path;
diff --git a/dir.h b/dir.h
index 549a187..41a5e32 100644
--- a/dir.h
+++ b/dir.h
@@ -113,7 +113,7 @@ struct path_exclude_check {
 };
 extern void path_exclude_check_init(struct path_exclude_check *, struct dir_struct *);
 extern void path_exclude_check_clear(struct path_exclude_check *);
-extern int path_excluded(struct path_exclude_check *, const char *, int namelen, int *dtype);
+extern int is_path_excluded(struct path_exclude_check *, const char *, int namelen, int *dtype);
 
 
 extern int add_excludes_from_file_to_list(const char *fname, const char *base, int baselen,
diff --git a/unpack-trees.c b/unpack-trees.c
index 6d96366..724f69b 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -1373,7 +1373,7 @@ static int check_ok_to_remove(const char *name, int len, int dtype,
 		return 0;
 
 	if (o->dir &&
-	    path_excluded(o->path_exclude_check, name, -1, &dtype))
+	    is_path_excluded(o->path_exclude_check, name, -1, &dtype))
 		/*
 		 * ce->name is explicitly excluded, so it is Ok to
 		 * overwrite it.
-- 
1.7.12.147.g6d168f4

[PATCH v2 05/14] Rename excluded_from_list() to is_excluded_from_list()

[Adam Spiers]

Continue adopting clearer names for exclude functions.  This 'is_*'
naming pattern for functions returning booleans was discussed here:

http://thread.gmane.org/gmane.comp.version-control.git/204661/focus=204924

Signed-off-by: Adam Spiers <git@adamspiers.org>
---
 dir.c          | 11 ++++++-----
 dir.h          |  4 ++--
 unpack-trees.c |  8 +++++---
 3 files changed, 13 insertions(+), 10 deletions(-)

diff --git a/dir.c b/dir.c
index dad1582..8f262f6 100644
--- a/dir.c
+++ b/dir.c
@@ -514,9 +514,9 @@ static void prep_exclude(struct dir_struct *dir, const char *base, int baselen)
 /* Scan the list and let the last match determine the fate.
  * Return 1 for exclude, 0 for include and -1 for undecided.
  */
-int excluded_from_list(const char *pathname,
-		       int pathlen, const char *basename, int *dtype,
-		       struct exclude_list *el)
+int is_excluded_from_list(const char *pathname,
+			  int pathlen, const char *basename, int *dtype,
+			  struct exclude_list *el)
 {
 	int i;
 
@@ -596,8 +596,9 @@ static int excluded(struct dir_struct *dir, const char *pathname, int *dtype_p)
 
 	prep_exclude(dir, pathname, basename-pathname);
 	for (st = EXC_CMDL; st <= EXC_FILE; st++) {
-		switch (excluded_from_list(pathname, pathlen, basename,
-					   dtype_p, &dir->exclude_list[st])) {
+		switch (is_excluded_from_list(pathname, pathlen,
+					      basename, dtype_p,
+					      &dir->exclude_list[st])) {
 		case 0:
 			return 0;
 		case 1:
diff --git a/dir.h b/dir.h
index 41a5e32..6904a35 100644
--- a/dir.h
+++ b/dir.h
@@ -98,8 +98,8 @@ extern int within_depth(const char *name, int namelen, int depth, int max_depth)
 extern int fill_directory(struct dir_struct *dir, const char **pathspec);
 extern int read_directory(struct dir_struct *, const char *path, int len, const char **pathspec);
 
-extern int excluded_from_list(const char *pathname, int pathlen, const char *basename,
-			      int *dtype, struct exclude_list *el);
+extern int is_excluded_from_list(const char *pathname, int pathlen, const char *basename,
+				 int *dtype, struct exclude_list *el);
 struct dir_entry *dir_add_ignored(struct dir_struct *dir, const char *pathname, int len);
 
 /*
diff --git a/unpack-trees.c b/unpack-trees.c
index 724f69b..56127c9 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -837,7 +837,8 @@ static int clear_ce_flags_dir(struct cache_entry **cache, int nr,
 {
 	struct cache_entry **cache_end;
 	int dtype = DT_DIR;
-	int ret = excluded_from_list(prefix, prefix_len, basename, &dtype, el);
+	int ret = is_excluded_from_list(prefix, prefix_len,
+					basename, &dtype, el);
 
 	prefix[prefix_len++] = '/';
 
@@ -856,7 +857,7 @@ static int clear_ce_flags_dir(struct cache_entry **cache, int nr,
 	 * with ret (iow, we know in advance the incl/excl
 	 * decision for the entire directory), clear flag here without
 	 * calling clear_ce_flags_1(). That function will call
-	 * the expensive excluded_from_list() on every entry.
+	 * the expensive is_excluded_from_list() on every entry.
 	 */
 	return clear_ce_flags_1(cache, cache_end - cache,
 				prefix, prefix_len,
@@ -939,7 +940,8 @@ static int clear_ce_flags_1(struct cache_entry **cache, int nr,
 
 		/* Non-directory */
 		dtype = ce_to_dtype(ce);
-		ret = excluded_from_list(ce->name, ce_namelen(ce), name, &dtype, el);
+		ret = is_excluded_from_list(ce->name, ce_namelen(ce),
+					    name, &dtype, el);
 		if (ret < 0)
 			ret = defval;
 		if (ret > 0)
-- 
1.7.12.147.g6d168f4

[PATCH v2 06/14] Rename excluded() to is_excluded()

[Adam Spiers]

Continue adopting clearer names for exclude functions.  This is_*
naming pattern for functions returning booleans was discussed here:

http://thread.gmane.org/gmane.comp.version-control.git/204661/focus=204924

Signed-off-by: Adam Spiers <git@adamspiers.org>
---
 attr.c |  2 +-
 dir.c  | 10 +++++-----
 dir.h  |  4 ++--
 3 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/attr.c b/attr.c
index 887a9ae..fd33ff9 100644
--- a/attr.c
+++ b/attr.c
@@ -270,7 +270,7 @@ static struct match_attr *parse_attr_line(const char *line, const char *src,
  * (reading the file from top to bottom), .gitattribute of the root
  * directory (again, reading the file from top to bottom) down to the
  * current directory, and then scan the list backwards to find the first match.
- * This is exactly the same as what excluded() does in dir.c to deal with
+ * This is exactly the same as what is_excluded() does in dir.c to deal with
  * .gitignore
  */
 
diff --git a/dir.c b/dir.c
index 8f262f6..b381b1b 100644
--- a/dir.c
+++ b/dir.c
@@ -587,7 +587,7 @@ int is_excluded_from_list(const char *pathname,
 	return -1; /* undecided */
 }
 
-static int excluded(struct dir_struct *dir, const char *pathname, int *dtype_p)
+static int is_excluded(struct dir_struct *dir, const char *pathname, int *dtype_p)
 {
 	int pathlen = strlen(pathname);
 	int st;
@@ -637,7 +637,7 @@ int is_path_excluded(struct path_exclude_check *check,
 	/*
 	 * we allow the caller to pass namelen as an optimization; it
 	 * must match the length of the name, as we eventually call
-	 * excluded() on the whole name string.
+	 * is_excluded() on the whole name string.
 	 */
 	if (namelen < 0)
 		namelen = strlen(name);
@@ -654,7 +654,7 @@ int is_path_excluded(struct path_exclude_check *check,
 
 		if (ch == '/') {
 			int dt = DT_DIR;
-			if (excluded(check->dir, path->buf, &dt))
+			if (is_excluded(check->dir, path->buf, &dt))
 				return 1;
 		}
 		strbuf_addch(path, ch);
@@ -663,7 +663,7 @@ int is_path_excluded(struct path_exclude_check *check,
 	/* An entry in the index; cannot be a directory with subentries */
 	strbuf_setlen(path, 0);
 
-	return excluded(check->dir, name, dtype);
+	return is_excluded(check->dir, name, dtype);
 }
 
 static struct dir_entry *dir_entry_new(const char *pathname, int len)
@@ -963,7 +963,7 @@ static enum path_treatment treat_one_path(struct dir_struct *dir,
 					  const struct path_simplify *simplify,
 					  int dtype, struct dirent *de)
 {
-	int exclude = excluded(dir, path->buf, &dtype);
+	int exclude = is_excluded(dir, path->buf, &dtype);
 	if (exclude && (dir->flags & DIR_COLLECT_IGNORED)
 	    && exclude_matches_pathspec(path->buf, path->len, simplify))
 		dir_add_ignored(dir, path->buf, path->len);
diff --git a/dir.h b/dir.h
index 6904a35..009c9df 100644
--- a/dir.h
+++ b/dir.h
@@ -103,8 +103,8 @@ extern int is_excluded_from_list(const char *pathname, int pathlen, const char *
 struct dir_entry *dir_add_ignored(struct dir_struct *dir, const char *pathname, int len);
 
 /*
- * The excluded() API is meant for callers that check each level of leading
- * directory hierarchies with excluded() to avoid recursing into excluded
+ * The is_excluded() API is meant for callers that check each level of leading
+ * directory hierarchies with is_excluded() to avoid recursing into excluded
  * directories.  Callers that do not do so should use this API instead.
  */
 struct path_exclude_check {
-- 
1.7.12.147.g6d168f4

[PATCH v2 07/14] Refactor is_excluded_from_list()

[Adam Spiers]

The excluded function uses a new helper function called
last_exclude_matching_from_list() to perform the inner loop over all of
the exclude patterns.  The helper just tells us whether the path is
included, excluded, or undecided.

However, it may be useful to know _which_ pattern was triggered.  So
let's pass out the entire exclude match, which contains the status
information we were already passing out.

Further patches can make use of this.

This is a modified forward port of a patch from 2009 by Jeff King:
http://article.gmane.org/gmane.comp.version-control.git/108815

Signed-off-by: Adam Spiers <git@adamspiers.org>
---
 dir.c | 41 ++++++++++++++++++++++++++++++-----------
 1 file changed, 30 insertions(+), 11 deletions(-)

diff --git a/dir.c b/dir.c
index b381b1b..70b7d7e 100644
--- a/dir.c
+++ b/dir.c
@@ -511,22 +511,26 @@ static void prep_exclude(struct dir_struct *dir, const char *base, int baselen)
 	dir->basebuf[baselen] = '\0';
 }
 
-/* Scan the list and let the last match determine the fate.
- * Return 1 for exclude, 0 for include and -1 for undecided.
+/*
+ * Scan the given exclude list in reverse to see whether pathname
+ * should be ignored.  The first match (i.e. the last on the list), if
+ * any, determines the fate.  Returns the exclude_list element which
+ * matched, or NULL for undecided.
  */
-int is_excluded_from_list(const char *pathname,
-			  int pathlen, const char *basename, int *dtype,
-			  struct exclude_list *el)
+static struct exclude *last_exclude_matching_from_list(const char *pathname,
+						       int pathlen,
+						       const char *basename,
+						       int *dtype,
+						       struct exclude_list *el)
 {
 	int i;
 
 	if (!el->nr)
-		return -1;	/* undefined */
+		return NULL;	/* undefined */
 
 	for (i = el->nr - 1; 0 <= i; i--) {
 		struct exclude *x = el->excludes[i];
 		const char *name, *exclude = x->pattern;
-		int to_exclude = x->to_exclude;
 		int namelen, prefix = x->nowildcardlen;
 
 		if (x->flags & EXC_FLAG_MUSTBEDIR) {
@@ -540,14 +544,14 @@ int is_excluded_from_list(const char *pathname,
 			/* match basename */
 			if (prefix == x->patternlen) {
 				if (!strcmp_icase(exclude, basename))
-					return to_exclude;
+					return x;
 			} else if (x->flags & EXC_FLAG_ENDSWITH) {
 				if (x->patternlen - 1 <= pathlen &&
 				    !strcmp_icase(exclude + 1, pathname + pathlen - x->patternlen + 1))
-					return to_exclude;
+					return x;
 			} else {
 				if (fnmatch_icase(exclude, basename, 0) == 0)
-					return to_exclude;
+					return x;
 			}
 			continue;
 		}
@@ -582,8 +586,23 @@ int is_excluded_from_list(const char *pathname,
 		}
 
 		if (!namelen || !fnmatch_icase(exclude, name, FNM_PATHNAME))
-			return to_exclude;
+			return x;
 	}
+	return NULL; /* undecided */
+}
+
+/*
+ * Scan the list and let the last match determine the fate.
+ * Return 1 for exclude, 0 for include and -1 for undecided.
+ */
+int is_excluded_from_list(const char *pathname,
+			  int pathlen, const char *basename, int *dtype,
+			  struct exclude_list *el)
+{
+	struct exclude *exclude;
+	exclude = last_exclude_matching_from_list(pathname, pathlen, basename, dtype, el);
+	if (exclude)
+		return exclude->to_exclude;
 	return -1; /* undecided */
 }
 
-- 
1.7.12.147.g6d168f4

[PATCH v2 08/14] Refactor is_excluded()

[Adam Spiers]

In a similar way to the previous commit, this extracts a new helper
function last_exclude_matching() which returns the last exclude_list
element which matched, or NULL if no match was found.  is_excluded()
becomes a wrapper around this, and just returns 0 or 1 depending on
whether any matching exclude_list element was found.

This allows callers to find out _why_ a given path was excluded,
rather than just whether it was or not, paving the way for a new git
sub-command which allows users to test their exclude lists from the
command line.

Signed-off-by: Adam Spiers <git@adamspiers.org>
---
 dir.c | 38 +++++++++++++++++++++++++++++---------
 1 file changed, 29 insertions(+), 9 deletions(-)

diff --git a/dir.c b/dir.c
index 70b7d7e..3e2161e 100644
--- a/dir.c
+++ b/dir.c
@@ -606,24 +606,44 @@ int is_excluded_from_list(const char *pathname,
 	return -1; /* undecided */
 }
 
-static int is_excluded(struct dir_struct *dir, const char *pathname, int *dtype_p)
+/*
+ * Loads the exclude lists for the directory containing pathname, then
+ * scans all exclude lists to determine whether pathname is excluded.
+ * Returns the exclude_list element which matched, or NULL for
+ * undecided.
+ */
+static struct exclude *last_exclude_matching(struct dir_struct *dir,
+					     const char *pathname,
+					     int *dtype_p)
 {
 	int pathlen = strlen(pathname);
 	int st;
+	struct exclude *exclude;
 	const char *basename = strrchr(pathname, '/');
 	basename = (basename) ? basename+1 : pathname;
 
 	prep_exclude(dir, pathname, basename-pathname);
 	for (st = EXC_CMDL; st <= EXC_FILE; st++) {
-		switch (is_excluded_from_list(pathname, pathlen,
-					      basename, dtype_p,
-					      &dir->exclude_list[st])) {
-		case 0:
-			return 0;
-		case 1:
-			return 1;
-		}
+		exclude = last_exclude_matching_from_list(
+			pathname, pathlen, basename, dtype_p,
+			&dir->exclude_list[st]);
+		if (exclude)
+			return exclude;
 	}
+	return NULL;
+}
+
+/*
+ * Loads the exclude lists for the directory containing pathname, then
+ * scans all exclude lists to determine whether pathname is excluded.
+ * Returns 1 if true, otherwise 0.
+ */
+static int is_excluded(struct dir_struct *dir, const char *pathname, int *dtype_p)
+{
+	struct exclude *exclude =
+		last_exclude_matching(dir, pathname, dtype_p);
+	if (exclude)
+		return exclude->to_exclude;
 	return 0;
 }
 
-- 
1.7.12.147.g6d168f4

[PATCH v2 09/14] Refactor is_path_excluded()

[Adam Spiers]

In a similar way to the previous commit, this extracts a new helper
function last_exclude_matching_path() which return the last
exclude_list element which matched, or NULL if no match was found.
is_path_excluded() becomes a wrapper around this, and just returns 0
or 1 depending on whether any matching exclude_list element was found.

This allows callers to find out _why_ a given path was excluded,
rather than just whether it was or not, paving the way for a new git
sub-command which allows users to test their exclude lists from the
command line.

Signed-off-by: Adam Spiers <git@adamspiers.org>
---
 dir.c | 47 ++++++++++++++++++++++++++++++++++++++---------
 dir.h |  3 +++
 2 files changed, 41 insertions(+), 9 deletions(-)

diff --git a/dir.c b/dir.c
index 3e2161e..904a7f3 100644
--- a/dir.c
+++ b/dir.c
@@ -651,6 +651,7 @@ void path_exclude_check_init(struct path_exclude_check *check,
 			     struct dir_struct *dir)
 {
 	check->dir = dir;
+	check->exclude = NULL;
 	strbuf_init(&check->path, 256);
 }
 
@@ -660,18 +661,21 @@ void path_exclude_check_clear(struct path_exclude_check *check)
 }
 
 /*
- * Is this name excluded?  This is for a caller like show_files() that
- * do not honor directory hierarchy and iterate through paths that are
- * possibly in an ignored directory.
+ * For each subdirectory in name, starting with the top-most, checks
+ * to see if that subdirectory is excluded, and if so, returns the
+ * corresponding exclude structure.  Otherwise, checks whether name
+ * itself (which is presumably a file) is excluded.
  *
  * A path to a directory known to be excluded is left in check->path to
  * optimize for repeated checks for files in the same excluded directory.
  */
-int is_path_excluded(struct path_exclude_check *check,
-		     const char *name, int namelen, int *dtype)
+struct exclude *last_exclude_matching_path(struct path_exclude_check *check,
+					   const char *name, int namelen,
+					   int *dtype)
 {
 	int i;
 	struct strbuf *path = &check->path;
+	struct exclude *exclude;
 
 	/*
 	 * we allow the caller to pass namelen as an optimization; it
@@ -681,11 +685,17 @@ int is_path_excluded(struct path_exclude_check *check,
 	if (namelen < 0)
 		namelen = strlen(name);
 
+	/*
+	 * If path is non-empty, and name is equal to path or a
+	 * subdirectory of path, name should be excluded, because
+	 * it's inside a directory which is already known to be
+	 * excluded and was previously left in check->path.
+	 */
 	if (path->len &&
 	    path->len <= namelen &&
 	    !memcmp(name, path->buf, path->len) &&
 	    (!name[path->len] || name[path->len] == '/'))
-		return 1;
+		return check->exclude;
 
 	strbuf_setlen(path, 0);
 	for (i = 0; name[i]; i++) {
@@ -693,8 +703,12 @@ int is_path_excluded(struct path_exclude_check *check,
 
 		if (ch == '/') {
 			int dt = DT_DIR;
-			if (is_excluded(check->dir, path->buf, &dt))
-				return 1;
+			exclude = last_exclude_matching(check->dir,
+							path->buf, &dt);
+			if (exclude) {
+				check->exclude = exclude;
+				return exclude;
+			}
 		}
 		strbuf_addch(path, ch);
 	}
@@ -702,7 +716,22 @@ int is_path_excluded(struct path_exclude_check *check,
 	/* An entry in the index; cannot be a directory with subentries */
 	strbuf_setlen(path, 0);
 
-	return is_excluded(check->dir, name, dtype);
+	return last_exclude_matching(check->dir, name, dtype);
+}
+
+/*
+ * Is this name excluded?  This is for a caller like show_files() that
+ * do not honor directory hierarchy and iterate through paths that are
+ * possibly in an ignored directory.
+ */
+int is_path_excluded(struct path_exclude_check *check,
+		  const char *name, int namelen, int *dtype)
+{
+	struct exclude *exclude =
+		last_exclude_matching_path(check, name, namelen, dtype);
+	if (exclude)
+		return exclude->to_exclude;
+	return 0;
 }
 
 static struct dir_entry *dir_entry_new(const char *pathname, int len)
diff --git a/dir.h b/dir.h
index 009c9df..19beddb 100644
--- a/dir.h
+++ b/dir.h
@@ -109,10 +109,13 @@ struct dir_entry *dir_add_ignored(struct dir_struct *dir, const char *pathname,
  */
 struct path_exclude_check {
 	struct dir_struct *dir;
+	struct exclude *exclude;
 	struct strbuf path;
 };
 extern void path_exclude_check_init(struct path_exclude_check *, struct dir_struct *);
 extern void path_exclude_check_clear(struct path_exclude_check *);
+extern struct exclude *last_exclude_matching_path(struct path_exclude_check *, const char *,
+						  int namelen, int *dtype);
 extern int is_path_excluded(struct path_exclude_check *, const char *, int namelen, int *dtype);
 
 
-- 
1.7.12.147.g6d168f4

[PATCH v2 10/14] For each exclude pattern, store information about where it came from

[Adam Spiers]

For exclude patterns read in from files, the filename is stored together
with the corresponding line number (counting starting at 1).

For exclude patterns provided on the command line, the sequence number
is negative, with counting starting at -1, so for example the 2nd
pattern provided via --exclude would be numbered -2.  This allows any
future consumers of that data to easily distinguish between exclude
patterns from files vs. from the CLI.

Signed-off-by: Adam Spiers <git@adamspiers.org>
---
 builtin/clean.c    |  2 +-
 builtin/ls-files.c |  3 ++-
 dir.c              | 25 +++++++++++++++++++------
 dir.h              |  5 ++++-
 4 files changed, 26 insertions(+), 9 deletions(-)

diff --git a/builtin/clean.c b/builtin/clean.c
index 69c1cda..893dd7a 100644
--- a/builtin/clean.c
+++ b/builtin/clean.c
@@ -99,7 +99,7 @@ int cmd_clean(int argc, const char **argv, const char *prefix)
 
 	for (i = 0; i < exclude_list.nr; i++)
 		add_exclude(exclude_list.items[i].string, "", 0,
-			    &dir.exclude_list[EXC_CMDL]);
+			    &dir.exclude_list[EXC_CMDL], "--exclude option", -(i+1));
 
 	pathspec = get_pathspec(prefix, argv);
 
diff --git a/builtin/ls-files.c b/builtin/ls-files.c
index d226456..64b1969 100644
--- a/builtin/ls-files.c
+++ b/builtin/ls-files.c
@@ -35,6 +35,7 @@ static int error_unmatch;
 static char *ps_matched;
 static const char *with_tree;
 static int exc_given;
+static int exclude_args;
 
 static const char *tag_cached = "";
 static const char *tag_unmerged = "";
@@ -423,7 +424,7 @@ static int option_parse_exclude(const struct option *opt,
 	struct exclude_list *list = opt->value;
 
 	exc_given = 1;
-	add_exclude(arg, "", 0, list);
+	add_exclude(arg, "", 0, list, "--exclude option", --exclude_args);
 
 	return 0;
 }
diff --git a/dir.c b/dir.c
index 904a7f3..705546f 100644
--- a/dir.c
+++ b/dir.c
@@ -311,7 +311,7 @@ static int no_wildcard(const char *string)
 }
 
 void add_exclude(const char *string, const char *base,
-		 int baselen, struct exclude_list *el)
+		 int baselen, struct exclude_list *el, const char *src, int srcpos)
 {
 	struct exclude *x;
 	size_t len;
@@ -341,6 +341,8 @@ void add_exclude(const char *string, const char *base,
 	x->base = base;
 	x->baselen = baselen;
 	x->flags = flags;
+	x->src = src;
+	x->srcpos = srcpos;
 	if (!strchr(string, '/'))
 		x->flags |= EXC_FLAG_NODIR;
 	x->nowildcardlen = simple_length(string);
@@ -393,7 +395,7 @@ int add_excludes_from_file_to_list(const char *fname,
 				   int check_index)
 {
 	struct stat st;
-	int fd, i;
+	int fd, i, lineno = 1;
 	size_t size = 0;
 	char *buf, *entry;
 
@@ -438,8 +440,9 @@ int add_excludes_from_file_to_list(const char *fname,
 		if (buf[i] == '\n') {
 			if (entry != buf + i && entry[0] != '#') {
 				buf[i - (i && buf[i-1] == '\r')] = 0;
-				add_exclude(entry, base, baselen, el);
+				add_exclude(entry, base, baselen, el, fname, lineno);
 			}
+			lineno++;
 			entry = buf + i + 1;
 		}
 	}
@@ -474,8 +477,10 @@ static void prep_exclude(struct dir_struct *dir, const char *base, int baselen)
 		    !strncmp(dir->basebuf, base, stk->baselen))
 			break;
 		dir->exclude_stack = stk->prev;
-		while (stk->exclude_ix < el->nr)
-			free(el->excludes[--el->nr]);
+		while (stk->exclude_ix < el->nr) {
+			struct exclude *exclude = el->excludes[--el->nr];
+			free(exclude);
+		}
 		free(stk->filebuf);
 		free(stk);
 	}
@@ -502,7 +507,15 @@ static void prep_exclude(struct dir_struct *dir, const char *base, int baselen)
 		memcpy(dir->basebuf + current, base + current,
 		       stk->baselen - current);
 		strcpy(dir->basebuf + stk->baselen, dir->exclude_per_dir);
-		add_excludes_from_file_to_list(dir->basebuf,
+
+		/* dir->basebuf gets reused by the traversal, but we
+		 * need fname to remain unchanged to ensure the src
+		 * member of each struct exclude correctly back-references
+		 * its source file.
+		 */
+		char *fname = strdup(dir->basebuf);
+
+		add_excludes_from_file_to_list(fname,
 					       dir->basebuf, stk->baselen,
 					       &stk->filebuf, el, 1);
 		dir->exclude_stack = stk;
diff --git a/dir.h b/dir.h
index 19beddb..ebb0367 100644
--- a/dir.h
+++ b/dir.h
@@ -31,6 +31,9 @@ struct exclude_list {
 		int baselen;
 		int to_exclude;
 		int flags;
+		const char *src;
+		int srcpos; /* counting starts from 1 for line numbers in ignore files,
+			       and from -1 decrementing for patterns from CLI (--exclude) */
 	} **excludes;
 };
 
@@ -123,7 +126,7 @@ extern int add_excludes_from_file_to_list(const char *fname, const char *base, i
 					  char **buf_p, struct exclude_list *el, int check_index);
 extern void add_excludes_from_file(struct dir_struct *, const char *fname);
 extern void add_exclude(const char *string, const char *base,
-			int baselen, struct exclude_list *el);
+			int baselen, struct exclude_list *el, const char *src, int srcpos);
 extern void free_excludes(struct exclude_list *el);
 extern int file_exists(const char *);
 
-- 
1.7.12.147.g6d168f4

Re: [PATCH v2 10/14] For each exclude pattern, store information about where it came from

[Junio C Hamano]

Adam Spiers <git@adamspiers.org> writes:

>  void add_exclude(const char *string, const char *base,
> -		 int baselen, struct exclude_list *el)
> +		 int baselen, struct exclude_list *el, const char *src, int srcpos)
>  {
>  	struct exclude *x;
>  	size_t len;
> @@ -341,6 +341,8 @@ void add_exclude(const char *string, const char *base,
>  	x->base = base;
>  	x->baselen = baselen;
>  	x->flags = flags;
> +	x->src = src;
> +	x->srcpos = srcpos;

Hrm, don't all elements "x" in "el" share the same "src", even if
their srcpos may be different?

>  	if (!strchr(string, '/'))
>  		x->flags |= EXC_FLAG_NODIR;
>  	x->nowildcardlen = simple_length(string);
> @@ -393,7 +395,7 @@ int add_excludes_from_file_to_list(const char *fname,
>  				   int check_index)
>  {
>  	struct stat st;
> -	int fd, i;
> +	int fd, i, lineno = 1;
>  	size_t size = 0;
>  	char *buf, *entry;
>  
> @@ -438,8 +440,9 @@ int add_excludes_from_file_to_list(const char *fname,
>  		if (buf[i] == '\n') {
>  			if (entry != buf + i && entry[0] != '#') {
>  				buf[i - (i && buf[i-1] == '\r')] = 0;
> -				add_exclude(entry, base, baselen, el);
> +				add_exclude(entry, base, baselen, el, fname, lineno);
>  			}
> +			lineno++;
>  			entry = buf + i + 1;
>  		}
>  	}
> @@ -474,8 +477,10 @@ static void prep_exclude(struct dir_struct *dir, const char *base, int baselen)
>  		    !strncmp(dir->basebuf, base, stk->baselen))
>  			break;
>  		dir->exclude_stack = stk->prev;
> -		while (stk->exclude_ix < el->nr)
> -			free(el->excludes[--el->nr]);
> +		while (stk->exclude_ix < el->nr) {
> +			struct exclude *exclude = el->excludes[--el->nr];
> +			free(exclude);
> +		}
>  		free(stk->filebuf);
>  		free(stk);
>  	}
> @@ -502,7 +507,15 @@ static void prep_exclude(struct dir_struct *dir, const char *base, int baselen)
>  		memcpy(dir->basebuf + current, base + current,
>  		       stk->baselen - current);
>  		strcpy(dir->basebuf + stk->baselen, dir->exclude_per_dir);
> -		add_excludes_from_file_to_list(dir->basebuf,
> +
> +		/* dir->basebuf gets reused by the traversal, but we
> +		 * need fname to remain unchanged to ensure the src
> +		 * member of each struct exclude correctly back-references
> +		 * its source file.
> +		 */
> +		char *fname = strdup(dir->basebuf);


	/*
         * We try to format our multi-line comments
         * like this.
         *
         * By the way, who owns x->src and who is responsible for
         * freeing it when the exclude-stack is popped to make them
         * no longer necessary?
         *
         * Oh, by the way, that is a decl-after-statement.
         */

> +
> +		add_excludes_from_file_to_list(fname,
>  					       dir->basebuf, stk->baselen,
>  					       &stk->filebuf, el, 1);
>  		dir->exclude_stack = stk;
> diff --git a/dir.h b/dir.h
> index 19beddb..ebb0367 100644
> --- a/dir.h
> +++ b/dir.h
> @@ -31,6 +31,9 @@ struct exclude_list {
>  		int baselen;
>  		int to_exclude;
>  		int flags;
> +		const char *src;
> +		int srcpos; /* counting starts from 1 for line numbers in ignore files,
> +			       and from -1 decrementing for patterns from CLI (--exclude) */
>  	} **excludes;
>  };
>  
> @@ -123,7 +126,7 @@ extern int add_excludes_from_file_to_list(const char *fname, const char *base, i
>  					  char **buf_p, struct exclude_list *el, int check_index);
>  extern void add_excludes_from_file(struct dir_struct *, const char *fname);
>  extern void add_exclude(const char *string, const char *base,
> -			int baselen, struct exclude_list *el);
> +			int baselen, struct exclude_list *el, const char *src, int srcpos);
>  extern void free_excludes(struct exclude_list *el);
>  extern int file_exists(const char *);

Re: [PATCH v2 10/14] For each exclude pattern, store information about where it came from

[Adam Spiers]

On Thu, Sep 20, 2012 at 02:31:57PM -0700, Junio C Hamano wrote:
> Adam Spiers <git@adamspiers.org> writes:
>
> >  void add_exclude(const char *string, const char *base,
> > -		 int baselen, struct exclude_list *el)
> > +		 int baselen, struct exclude_list *el, const char *src, int srcpos)
> >  {
> >  	struct exclude *x;
> >  	size_t len;
> > @@ -341,6 +341,8 @@ void add_exclude(const char *string, const char *base,
> >  	x->base = base;
> >  	x->baselen = baselen;
> >  	x->flags = flags;
> > +	x->src = src;
> > +	x->srcpos = srcpos;
>
> Hrm, don't all elements "x" in "el" share the same "src", even if
> their srcpos may be different?

No not currently - please see the other mail I just sent to the
[PATCH v2 00/14] thread.

[PATCH v2 11/14] Refactor treat_gitlinks()

[Adam Spiers]

Extract the body of the for loop in treat_gitlinks() into a separate
treat_gitlink() function so that it can be reused elsewhere.  This
paves the way for a new check-ignore sub-command.

Signed-off-by: Adam Spiers <git@adamspiers.org>
---
 builtin/add.c | 49 +++++++++++++++++++++++++++++++------------------
 1 file changed, 31 insertions(+), 18 deletions(-)

diff --git a/builtin/add.c b/builtin/add.c
index 075312a..b4ec5cd 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -153,31 +153,44 @@ static char *prune_directory(struct dir_struct *dir, const char **pathspec, int
 	return seen;
 }
 
-static void treat_gitlinks(const char **pathspec)
+/*
+ * Check whether path refers to a submodule, or something inside a
+ * submodule.  If the former, returns the path with any trailing slash
+ * stripped.  If the latter, dies with an error message.
+ */
+const char *treat_gitlink(const char *path)
 {
-	int i;
-
-	if (!pathspec || !*pathspec)
-		return;
-
+	int i, path_len = strlen(path);
 	for (i = 0; i < active_nr; i++) {
 		struct cache_entry *ce = active_cache[i];
 		if (S_ISGITLINK(ce->ce_mode)) {
-			int len = ce_namelen(ce), j;
-			for (j = 0; pathspec[j]; j++) {
-				int len2 = strlen(pathspec[j]);
-				if (len2 <= len || pathspec[j][len] != '/' ||
-				    memcmp(ce->name, pathspec[j], len))
-					continue;
-				if (len2 == len + 1)
-					/* strip trailing slash */
-					pathspec[j] = xstrndup(ce->name, len);
-				else
-					die (_("Path '%s' is in submodule '%.*s'"),
-						pathspec[j], len, ce->name);
+			int ce_len = ce_namelen(ce);
+			if (path_len <= ce_len || path[ce_len] != '/' ||
+			    memcmp(ce->name, path, ce_len))
+				/* path does not refer to this
+				 * submodule or anything inside it */
+				continue;
+			if (path_len == ce_len + 1) {
+				/* path refers to submodule;
+				 * strip trailing slash */
+				return xstrndup(ce->name, ce_len);
+			} else {
+				die (_("Path '%s' is in submodule '%.*s'"),
+				     path, ce_len, ce->name);
 			}
 		}
 	}
+	return path;
+}
+
+void treat_gitlinks(const char **pathspec)
+{
+	if (!pathspec || !*pathspec)
+		return;
+
+	int i;
+	for (i = 0; pathspec[i]; i++)
+		pathspec[i] = treat_gitlink(pathspec[i]);
 }
 
 static void refresh(int verbose, const char **pathspec)
-- 
1.7.12.147.g6d168f4

[PATCH v2 12/14] Extract some useful pathspec handling code from builtin/add.c into a library

[Adam Spiers]

This is in preparation for reuse by a new git check-ignore command.

Signed-off-by: Adam Spiers <git@adamspiers.org>
---
 Makefile      |  2 ++
 builtin/add.c | 95 ++-------------------------------------------------------
 pathspec.c    | 97 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 pathspec.h    |  6 ++++
 4 files changed, 108 insertions(+), 92 deletions(-)
 create mode 100644 pathspec.c
 create mode 100644 pathspec.h

diff --git a/Makefile b/Makefile
index a49d1db..9a4bf9e 100644
--- a/Makefile
+++ b/Makefile
@@ -649,6 +649,7 @@ LIB_H += pack-revindex.h
 LIB_H += pack.h
 LIB_H += parse-options.h
 LIB_H += patch-ids.h
+LIB_H += pathspec.h
 LIB_H += pkt-line.h
 LIB_H += progress.h
 LIB_H += prompt.h
@@ -769,6 +770,7 @@ LIB_OBJS += parse-options-cb.o
 LIB_OBJS += patch-delta.o
 LIB_OBJS += patch-ids.o
 LIB_OBJS += path.o
+LIB_OBJS += pathspec.o
 LIB_OBJS += pkt-line.o
 LIB_OBJS += preload-index.o
 LIB_OBJS += pretty.o
diff --git a/builtin/add.c b/builtin/add.c
index b4ec5cd..c3def9c 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -6,6 +6,7 @@
 #include "cache.h"
 #include "builtin.h"
 #include "dir.h"
+#include "pathspec.h"
 #include "exec_cmd.h"
 #include "cache-tree.h"
 #include "run-command.h"
@@ -97,39 +98,6 @@ int add_files_to_cache(const char *prefix, const char **pathspec, int flags)
 	return !!data.add_errors;
 }
 
-static void fill_pathspec_matches(const char **pathspec, char *seen, int specs)
-{
-	int num_unmatched = 0, i;
-
-	/*
-	 * Since we are walking the index as if we were walking the directory,
-	 * we have to mark the matched pathspec as seen; otherwise we will
-	 * mistakenly think that the user gave a pathspec that did not match
-	 * anything.
-	 */
-	for (i = 0; i < specs; i++)
-		if (!seen[i])
-			num_unmatched++;
-	if (!num_unmatched)
-		return;
-	for (i = 0; i < active_nr; i++) {
-		struct cache_entry *ce = active_cache[i];
-		match_pathspec(pathspec, ce->name, ce_namelen(ce), 0, seen);
-	}
-}
-
-static char *find_used_pathspec(const char **pathspec)
-{
-	char *seen;
-	int i;
-
-	for (i = 0; pathspec[i];  i++)
-		; /* just counting */
-	seen = xcalloc(i, 1);
-	fill_pathspec_matches(pathspec, seen, i);
-	return seen;
-}
-
 static char *prune_directory(struct dir_struct *dir, const char **pathspec, int prefix)
 {
 	char *seen;
@@ -153,46 +121,6 @@ static char *prune_directory(struct dir_struct *dir, const char **pathspec, int
 	return seen;
 }
 
-/*
- * Check whether path refers to a submodule, or something inside a
- * submodule.  If the former, returns the path with any trailing slash
- * stripped.  If the latter, dies with an error message.
- */
-const char *treat_gitlink(const char *path)
-{
-	int i, path_len = strlen(path);
-	for (i = 0; i < active_nr; i++) {
-		struct cache_entry *ce = active_cache[i];
-		if (S_ISGITLINK(ce->ce_mode)) {
-			int ce_len = ce_namelen(ce);
-			if (path_len <= ce_len || path[ce_len] != '/' ||
-			    memcmp(ce->name, path, ce_len))
-				/* path does not refer to this
-				 * submodule or anything inside it */
-				continue;
-			if (path_len == ce_len + 1) {
-				/* path refers to submodule;
-				 * strip trailing slash */
-				return xstrndup(ce->name, ce_len);
-			} else {
-				die (_("Path '%s' is in submodule '%.*s'"),
-				     path, ce_len, ce->name);
-			}
-		}
-	}
-	return path;
-}
-
-void treat_gitlinks(const char **pathspec)
-{
-	if (!pathspec || !*pathspec)
-		return;
-
-	int i;
-	for (i = 0; pathspec[i]; i++)
-		pathspec[i] = treat_gitlink(pathspec[i]);
-}
-
 static void refresh(int verbose, const char **pathspec)
 {
 	char *seen;
@@ -210,23 +138,6 @@ static void refresh(int verbose, const char **pathspec)
         free(seen);
 }
 
-static const char **validate_pathspec(int argc, const char **argv, const char *prefix)
-{
-	const char **pathspec = get_pathspec(prefix, argv);
-
-	if (pathspec) {
-		const char **p;
-		for (p = pathspec; *p; p++) {
-			if (has_symlink_leading_path(*p, strlen(*p))) {
-				int len = prefix ? strlen(prefix) : 0;
-				die(_("'%s' is beyond a symbolic link"), *p + len);
-			}
-		}
-	}
-
-	return pathspec;
-}
-
 int run_add_interactive(const char *revision, const char *patch_mode,
 			const char **pathspec)
 {
@@ -261,7 +172,7 @@ int interactive_add(int argc, const char **argv, const char *prefix, int patch)
 	const char **pathspec = NULL;
 
 	if (argc) {
-		pathspec = validate_pathspec(argc, argv, prefix);
+		pathspec = validate_pathspec(prefix, argv);
 		if (!pathspec)
 			return -1;
 	}
@@ -428,7 +339,7 @@ int cmd_add(int argc, const char **argv, const char *prefix)
 		fprintf(stderr, _("Maybe you wanted to say 'git add .'?\n"));
 		return 0;
 	}
-	pathspec = validate_pathspec(argc, argv, prefix);
+	pathspec = validate_pathspec(prefix, argv);
 
 	if (read_cache() < 0)
 		die(_("index file corrupt"));
diff --git a/pathspec.c b/pathspec.c
new file mode 100644
index 0000000..10f6643
--- /dev/null
+++ b/pathspec.c
@@ -0,0 +1,97 @@
+#include "cache.h"
+#include "dir.h"
+
+void validate_path(const char *prefix, const char *path)
+{
+	if (has_symlink_leading_path(path, strlen(path))) {
+		int len = prefix ? strlen(prefix) : 0;
+		die(_("'%s' is beyond a symbolic link"), path + len);
+	}
+}
+
+const char **validate_pathspec(const char *prefix, const char **files)
+{
+	const char **pathspec = get_pathspec(prefix, files);
+
+	if (pathspec) {
+		const char **p;
+		for (p = pathspec; *p; p++) {
+			validate_path(prefix, *p);
+		}
+	}
+
+	return pathspec;
+}
+
+void fill_pathspec_matches(const char **pathspec, char *seen, int specs)
+{
+	int num_unmatched = 0, i;
+
+	/*
+	 * Since we are walking the index as if we were walking the directory,
+	 * we have to mark the matched pathspec as seen; otherwise we will
+	 * mistakenly think that the user gave a pathspec that did not match
+	 * anything.
+	 */
+	for (i = 0; i < specs; i++)
+		if (!seen[i])
+			num_unmatched++;
+	if (!num_unmatched)
+		return;
+	for (i = 0; i < active_nr; i++) {
+		struct cache_entry *ce = active_cache[i];
+		match_pathspec(pathspec, ce->name, ce_namelen(ce), 0, seen);
+	}
+}
+
+char *find_used_pathspec(const char **pathspec)
+{
+	char *seen;
+	int i;
+
+	for (i = 0; pathspec[i];  i++)
+		; /* just counting */
+	seen = xcalloc(i, 1);
+	fill_pathspec_matches(pathspec, seen, i);
+	return seen;
+}
+
+/*
+ * Check whether path refers to a submodule, or something inside a
+ * submodule.  If the former, returns the path with any trailing slash
+ * stripped.  If the latter, dies with an error message.
+ */
+const char *treat_gitlink(const char *path)
+{
+	int i, path_len = strlen(path);
+	for (i = 0; i < active_nr; i++) {
+		struct cache_entry *ce = active_cache[i];
+		if (S_ISGITLINK(ce->ce_mode)) {
+			int ce_len = ce_namelen(ce);
+			if (path_len <= ce_len || path[ce_len] != '/' ||
+			    memcmp(ce->name, path, ce_len))
+				/* path does not refer to this
+				 * submodule or anything inside it */
+				continue;
+			if (path_len == ce_len + 1) {
+				/* path refers to submodule;
+				 * strip trailing slash */
+				return xstrndup(ce->name, ce_len);
+			} else {
+				die (_("Path '%s' is in submodule '%.*s'"),
+				     path, ce_len, ce->name);
+			}
+		}
+	}
+	return path;
+}
+
+void treat_gitlinks(const char **pathspec)
+{
+	if (!pathspec || !*pathspec)
+		return;
+
+	int i;
+	for (i = 0; pathspec[i]; i++)
+		pathspec[i] = treat_gitlink(pathspec[i]);
+}
diff --git a/pathspec.h b/pathspec.h
new file mode 100644
index 0000000..4ed40a5
--- /dev/null
+++ b/pathspec.h
@@ -0,0 +1,6 @@
+extern void validate_path(const char *prefix, const char *path);
+extern const char **validate_pathspec(const char *prefix, const char **files);
+extern char *find_used_pathspec(const char **pathspec);
+extern void fill_pathspec_matches(const char **pathspec, char *seen, int specs);
+extern const char *treat_gitlink(const char **path);
+extern void treat_gitlinks(const char **pathspec);
-- 
1.7.12.147.g6d168f4

Re: [PATCH v2 12/14] Extract some useful pathspec handling code from builtin/add.c into a library

[Michael Haggerty]

On 09/20/2012 09:46 PM, Adam Spiers wrote:
> This is in preparation for reuse by a new git check-ignore command.
> 
> Signed-off-by: Adam Spiers <git@adamspiers.org>
> ---
>  Makefile      |  2 ++
>  builtin/add.c | 95 ++-------------------------------------------------------
>  pathspec.c    | 97 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  pathspec.h    |  6 ++++
>  4 files changed, 108 insertions(+), 92 deletions(-)
>  create mode 100644 pathspec.c
>  create mode 100644 pathspec.h
> 
> [...]
> diff --git a/pathspec.h b/pathspec.h
> new file mode 100644
> index 0000000..4ed40a5
> --- /dev/null
> +++ b/pathspec.h
> @@ -0,0 +1,6 @@
> +extern void validate_path(const char *prefix, const char *path);
> +extern const char **validate_pathspec(const char *prefix, const char **files);
> +extern char *find_used_pathspec(const char **pathspec);
> +extern void fill_pathspec_matches(const char **pathspec, char *seen, int specs);
> +extern const char *treat_gitlink(const char **path);
> +extern void treat_gitlinks(const char **pathspec);

It was unfortunate that these functions were not already documented.
But now that you are elevating them from static to global functions, it
would be great if you would add some comments for them.

(By the way, thanks for the docstrings that you added in an earlier patch.)

This patch is not only moving code around, but also:

* extracting a new function, validate_path()

* changing the signature of validate_pathspec()

* maybe other things?  How is a reviewer to know without examining every
line of the patch?

Each self-contained change should be done in a separate patch.  For
example, one patch should move the code while making only the minimal
changes logically connected to the move (e.g., removing "static"
qualifiers).  The other changes should be made separate commits.

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/

[PATCH v2 13/14] Provide free_directory() for reclaiming dir_struct memory

[Adam Spiers]

Signed-off-by: Adam Spiers <git@adamspiers.org>
---
 Documentation/technical/api-directory-listing.txt |  2 ++
 dir.c                                             | 23 +++++++++++++++++++++--
 dir.h                                             |  1 +
 3 files changed, 24 insertions(+), 2 deletions(-)

diff --git a/Documentation/technical/api-directory-listing.txt b/Documentation/technical/api-directory-listing.txt
index 944fc39..e339c18 100644
--- a/Documentation/technical/api-directory-listing.txt
+++ b/Documentation/technical/api-directory-listing.txt
@@ -79,4 +79,6 @@ marked. If you to exclude files, make sure you have loaded index first.
 
 * Use `dir.entries[]`.
 
+* Call `free_directory()` when none of the contained elements are no longer in use.
+
 (JC)
diff --git a/dir.c b/dir.c
index 705546f..a04739c 100644
--- a/dir.c
+++ b/dir.c
@@ -456,6 +456,12 @@ void add_excludes_from_file(struct dir_struct *dir, const char *fname)
 		die("cannot use %s as an exclude file", fname);
 }
 
+static void free_exclude_stack(struct exclude_stack *stk)
+{
+	free(stk->filebuf);
+	free(stk);
+}
+
 /*
  * Loads the per-directory exclude list for the substring of base
  * which has a char length of baselen.
@@ -481,8 +487,7 @@ static void prep_exclude(struct dir_struct *dir, const char *base, int baselen)
 			struct exclude *exclude = el->excludes[--el->nr];
 			free(exclude);
 		}
-		free(stk->filebuf);
-		free(stk);
+		free_exclude_stack(stk);
 	}
 
 	/* Read from the parent directories and push them down. */
@@ -1473,3 +1478,17 @@ void free_pathspec(struct pathspec *pathspec)
 	free(pathspec->items);
 	pathspec->items = NULL;
 }
+
+void free_directory(struct dir_struct *dir)
+{
+	int st;
+	for (st = EXC_CMDL; st <= EXC_FILE; st++)
+		free_excludes(&dir->exclude_list[st]);
+
+	struct exclude_stack *prev, *stk = dir->exclude_stack;
+	while (stk) {
+		prev = stk->prev;
+		free_exclude_stack(stk);
+		stk = prev;
+	}
+}
diff --git a/dir.h b/dir.h
index ebb0367..7da29da 100644
--- a/dir.h
+++ b/dir.h
@@ -128,6 +128,7 @@ extern void add_excludes_from_file(struct dir_struct *, const char *fname);
 extern void add_exclude(const char *string, const char *base,
 			int baselen, struct exclude_list *el, const char *src, int srcpos);
 extern void free_excludes(struct exclude_list *el);
+extern void free_directory(struct dir_struct *dir);
 extern int file_exists(const char *);
 
 extern int is_inside_dir(const char *dir);
-- 
1.7.12.147.g6d168f4

Re: [PATCH v2 13/14] Provide free_directory() for reclaiming dir_struct memory

[Michael Haggerty]

On 09/20/2012 09:46 PM, Adam Spiers wrote:
> Signed-off-by: Adam Spiers <git@adamspiers.org>
> ---
>  Documentation/technical/api-directory-listing.txt |  2 ++
>  dir.c                                             | 23 +++++++++++++++++++++--
>  dir.h                                             |  1 +
>  3 files changed, 24 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/technical/api-directory-listing.txt b/Documentation/technical/api-directory-listing.txt
> index 944fc39..e339c18 100644
> --- a/Documentation/technical/api-directory-listing.txt
> +++ b/Documentation/technical/api-directory-listing.txt
> @@ -79,4 +79,6 @@ marked. If you to exclude files, make sure you have loaded index first.
>  
>  * Use `dir.entries[]`.
>  
> +* Call `free_directory()` when none of the contained elements are no longer in use.
> +
>  (JC)
> [...]
> diff --git a/dir.h b/dir.h
> index ebb0367..7da29da 100644
> --- a/dir.h
> +++ b/dir.h
> @@ -128,6 +128,7 @@ extern void add_excludes_from_file(struct dir_struct *, const char *fname);
>  extern void add_exclude(const char *string, const char *base,
>  			int baselen, struct exclude_list *el, const char *src, int srcpos);
>  extern void free_excludes(struct exclude_list *el);
> +extern void free_directory(struct dir_struct *dir);
>  extern int file_exists(const char *);
>  
>  extern int is_inside_dir(const char *dir);

With I see a function like this, the first question in my head is always
"does it also free(dir), or does it only free the substructures, leaving
dir empty but allocated?"  There should be a comment documenting the
behavior.  I also find it helpful if a function that frees the top-level
structure has "free" in the name, while a function that only empties the
top-level structure without freeing it *not* have free in the name
(e.g., "clear_directory()").  But maybe that's just me.

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/

Re: [PATCH v2 13/14] Provide free_directory() for reclaiming dir_struct memory

[Junio C Hamano]

Michael Haggerty <mhagger@alum.mit.edu> writes:

> On 09/20/2012 09:46 PM, Adam Spiers wrote:
>> Signed-off-by: Adam Spiers <git@adamspiers.org>
>> ---
>>  Documentation/technical/api-directory-listing.txt |  2 ++
>>  dir.c                                             | 23 +++++++++++++++++++++--
>>  dir.h                                             |  1 +
>>  3 files changed, 24 insertions(+), 2 deletions(-)
>> 
>> diff --git a/Documentation/technical/api-directory-listing.txt b/Documentation/technical/api-directory-listing.txt
>> index 944fc39..e339c18 100644
>> --- a/Documentation/technical/api-directory-listing.txt
>> +++ b/Documentation/technical/api-directory-listing.txt
>> @@ -79,4 +79,6 @@ marked. If you to exclude files, make sure you have loaded index first.
>>  
>>  * Use `dir.entries[]`.
>>  
>> +* Call `free_directory()` when none of the contained elements are no longer in use.
>> +
>>  (JC)
>> [...]
>> diff --git a/dir.h b/dir.h
>> index ebb0367..7da29da 100644
>> --- a/dir.h
>> +++ b/dir.h
>> @@ -128,6 +128,7 @@ extern void add_excludes_from_file(struct dir_struct *, const char *fname);
>>  extern void add_exclude(const char *string, const char *base,
>>  			int baselen, struct exclude_list *el, const char *src, int srcpos);
>>  extern void free_excludes(struct exclude_list *el);
>> +extern void free_directory(struct dir_struct *dir);
>>  extern int file_exists(const char *);
>>  
>>  extern int is_inside_dir(const char *dir);
>
> With I see a function like this, the first question in my head is always
> "does it also free(dir), or does it only free the substructures, leaving
> dir empty but allocated?"  There should be a comment documenting the
> behavior.  I also find it helpful if a function that frees the top-level
> structure has "free" in the name, while a function that only empties the
> top-level structure without freeing it *not* have free in the name
> (e.g., "clear_directory()").  But maybe that's just me.

All good points, including the last one.  Thanks.

[PATCH v2 14/14] Add git-check-ignore sub-command

[Adam Spiers]

This works in a similar manner to git-check-attr.  Some code
was reused from add.c by refactoring out into pathspec.c.

Thanks to Jeff King and Junio C Hamano for the idea:
http://thread.gmane.org/gmane.comp.version-control.git/108671/focus=108815

Signed-off-by: Adam Spiers <git@adamspiers.org>
---
 .gitignore                             |   1 +
 Documentation/git-check-ignore.txt     |  85 +++++
 Documentation/gitignore.txt            |   6 +-
 Makefile                               |   1 +
 builtin.h                              |   1 +
 builtin/check-ignore.c                 | 167 ++++++++++
 command-list.txt                       |   1 +
 contrib/completion/git-completion.bash |   1 +
 git.c                                  |   1 +
 t/t0007-ignores.sh                     | 587 +++++++++++++++++++++++++++++++++
 t/t9902-completion.sh                  |  24 +-
 11 files changed, 861 insertions(+), 14 deletions(-)
 create mode 100644 Documentation/git-check-ignore.txt
 create mode 100644 builtin/check-ignore.c
 create mode 100755 t/t0007-ignores.sh

diff --git a/.gitignore b/.gitignore
index a188a82..11cd975 100644
--- a/.gitignore
+++ b/.gitignore
@@ -22,6 +22,7 @@
 /git-bundle
 /git-cat-file
 /git-check-attr
+/git-check-ignore
 /git-check-ref-format
 /git-checkout
 /git-checkout-index
diff --git a/Documentation/git-check-ignore.txt b/Documentation/git-check-ignore.txt
new file mode 100644
index 0000000..2de4e17
--- /dev/null
+++ b/Documentation/git-check-ignore.txt
@@ -0,0 +1,85 @@
+git-check-ignore(1)
+=================
+
+NAME
+----
+git-check-ignore - Debug gitignore / exclude files
+
+
+SYNOPSIS
+--------
+[verse]
+'git check-ignore' [options] pathname...
+'git check-ignore' [options] --stdin < <list-of-paths>
+
+DESCRIPTION
+-----------
+
+For each pathname given via the command-line or from a file via
+`--stdin`, this command will list the first exclude pattern found (if
+any) which explicitly excludes or includes that pathname.  Note that
+within any given exclude file, later patterns take precedence over
+earlier ones, so any matching pattern which this command outputs may
+not be the one you would immediately expect.
+
+OPTIONS
+-------
+-q, --quiet::
+	Don't output anything, just set exit status.  This is only
+	valid with a single pathname.
+
+-v, --verbose::
+	Also output details about the matching pattern (if any)
+	for each given pathname.
+
+--stdin::
+	Read file names from stdin instead of from the command-line.
+
+-z::
+	The output format is modified to be machine-parseable (see
+	below).  If `--stdin` is also given, input paths are separated
+	with a NUL character instead of a linefeed character.
+
+OUTPUT
+------
+
+By default, any of the given pathnames which match an ignore pattern
+will be output, one per line.  If no pattern matches a given path,
+nothing will be output for that path; this means that path will not be
+ignored.
+
+If `--verbose` is specified, the output is a series of lines of the form:
+
+<source> <COLON> <linenum> <COLON> <pattern> <HT> <pathname>
+
+<pathname> is the path of a file being queried, <pattern> is the
+matching pattern, <source> is the pattern's source file, and <linenum>
+is the line number of the pattern within that source.  If the pattern
+contained a `!` prefix or `/` suffix, it will be preserved in the
+output.  <source> will be an absolute path when referring to the file
+configured by `core.excludesfile`, or relative to the repository root
+when referring to `.git/info/exclude` or a per-directory exclude file.
+
+If `-z` is specified, the output is a series of lines of the form:
+
+EXIT STATUS
+-----------
+
+0::
+	One or more of the provided paths is ignored.
+
+1::
+	None of the provided paths are ignored.
+
+128::
+	A fatal error was encountered.
+
+SEE ALSO
+--------
+linkgit:gitignore[5]
+linkgit:gitconfig[5]
+linkgit:git-ls-files[5]
+
+GIT
+---
+Part of the linkgit:git[1] suite
diff --git a/Documentation/gitignore.txt b/Documentation/gitignore.txt
index c1f692a..7ba16fe 100644
--- a/Documentation/gitignore.txt
+++ b/Documentation/gitignore.txt
@@ -155,8 +155,10 @@ The second .gitignore prevents git from ignoring
 
 SEE ALSO
 --------
-linkgit:git-rm[1], linkgit:git-update-index[1],
-linkgit:gitrepository-layout[5]
+linkgit:git-rm[1],
+linkgit:git-update-index[1],
+linkgit:gitrepository-layout[5],
+linkgit:git-check-ignore[1]
 
 GIT
 ---
diff --git a/Makefile b/Makefile
index 9a4bf9e..2d78b83 100644
--- a/Makefile
+++ b/Makefile
@@ -834,6 +834,7 @@ BUILTIN_OBJS += builtin/branch.o
 BUILTIN_OBJS += builtin/bundle.o
 BUILTIN_OBJS += builtin/cat-file.o
 BUILTIN_OBJS += builtin/check-attr.o
+BUILTIN_OBJS += builtin/check-ignore.o
 BUILTIN_OBJS += builtin/check-ref-format.o
 BUILTIN_OBJS += builtin/checkout-index.o
 BUILTIN_OBJS += builtin/checkout.o
diff --git a/builtin.h b/builtin.h
index 95116b8..7519f81 100644
--- a/builtin.h
+++ b/builtin.h
@@ -55,6 +55,7 @@ extern int cmd_cat_file(int argc, const char **argv, const char *prefix);
 extern int cmd_checkout(int argc, const char **argv, const char *prefix);
 extern int cmd_checkout_index(int argc, const char **argv, const char *prefix);
 extern int cmd_check_attr(int argc, const char **argv, const char *prefix);
+extern int cmd_check_ignore(int argc, const char **argv, const char *prefix);
 extern int cmd_check_ref_format(int argc, const char **argv, const char *prefix);
 extern int cmd_cherry(int argc, const char **argv, const char *prefix);
 extern int cmd_cherry_pick(int argc, const char **argv, const char *prefix);
diff --git a/builtin/check-ignore.c b/builtin/check-ignore.c
new file mode 100644
index 0000000..74ea2fc
--- /dev/null
+++ b/builtin/check-ignore.c
@@ -0,0 +1,167 @@
+#include "builtin.h"
+#include "cache.h"
+#include "dir.h"
+#include "quote.h"
+#include "pathspec.h"
+#include "parse-options.h"
+
+static int quiet, verbose, stdin_paths;
+static const char * const check_ignore_usage[] = {
+"git check-ignore [options] pathname...",
+"git check-ignore [options] --stdin < <list-of-paths>",
+NULL
+};
+
+static int null_term_line;
+
+static const struct option check_ignore_options[] = {
+	OPT__QUIET(&quiet, N_("suppress progress reporting")),
+	OPT__VERBOSE(&verbose, N_("be verbose")),
+	OPT_GROUP(""),
+	OPT_BOOLEAN(0, "stdin", &stdin_paths,
+		    N_("read file names from stdin")),
+	OPT_BOOLEAN('z', NULL, &null_term_line,
+		    N_("input paths are terminated by a null character")),
+	OPT_END()
+};
+
+static void output_exclude(const char *path, struct exclude *exclude)
+{
+	char *bang = exclude->to_exclude ? "" : "!";
+	char *dir  = (exclude->flags & EXC_FLAG_MUSTBEDIR) ? "/" : "";
+	if (!null_term_line) {
+		if (!verbose) {
+			write_name_quoted(path, stdout, '\n');
+		} else {
+			quote_c_style(exclude->src, NULL, stdout, 0);
+			printf(":%d:%s%s%s\t",
+			       exclude->srcpos,
+			       bang, exclude->pattern, dir);
+			quote_c_style(path, NULL, stdout, 0);
+			fputc('\n', stdout);
+		}
+	} else {
+		if (!verbose) {
+			printf("%s%c", path, '\0');
+		} else {
+			printf("%s%c%d%c%s%s%s%c%s%c",
+			       exclude->src, '\0',
+			       exclude->srcpos, '\0',
+			       bang, exclude->pattern, dir, '\0',
+			       path, '\0');
+		}
+	}
+}
+
+static int check_ignore(const char *prefix, const char **pathspec)
+{
+	struct dir_struct dir;
+	const char *path;
+	char *seen = NULL;
+	int num_ignored = 0;
+
+	/* read_cache() is only necessary so we can watch out for submodules. */
+	if (read_cache() < 0)
+		die(_("index file corrupt"));
+
+	memset(&dir, 0, sizeof(dir));
+	dir.flags |= DIR_COLLECT_IGNORED;
+	setup_standard_excludes(&dir);
+
+	if (pathspec) {
+		int i;
+		struct path_exclude_check check;
+		struct exclude *exclude;
+
+		path_exclude_check_init(&check, &dir);
+		if (!seen)
+			seen = find_used_pathspec(pathspec);
+		for (i = 0; pathspec[i]; i++) {
+			path = pathspec[i];
+			char *full_path =
+				prefix_path(prefix, prefix ? strlen(prefix) : 0, path);
+			full_path = treat_gitlink(full_path);
+			validate_path(prefix, full_path);
+			if (!seen[i] && path[0]) {
+				int dtype = DT_UNKNOWN;
+				exclude = last_exclude_matching_path(&check, full_path,
+								     -1, &dtype);
+				if (exclude) {
+					if (!quiet)
+						output_exclude(path, exclude);
+					num_ignored++;
+				}
+			}
+		}
+		free(seen);
+		free_directory(&dir);
+		path_exclude_check_clear(&check);
+	} else {
+		printf("no pathspec\n");
+	}
+	return num_ignored;
+}
+
+static int check_ignore_stdin_paths(const char *prefix)
+{
+	struct strbuf buf, nbuf;
+	char **pathspec = NULL;
+	size_t nr = 0, alloc = 0;
+	int line_termination = null_term_line ? 0 : '\n';
+
+	strbuf_init(&buf, 0);
+	strbuf_init(&nbuf, 0);
+	while (strbuf_getline(&buf, stdin, line_termination) != EOF) {
+		if (line_termination && buf.buf[0] == '"') {
+			strbuf_reset(&nbuf);
+			if (unquote_c_style(&nbuf, buf.buf, NULL))
+				die("line is badly quoted");
+			strbuf_swap(&buf, &nbuf);
+		}
+		ALLOC_GROW(pathspec, nr + 1, alloc);
+		pathspec[nr] = xcalloc(strlen(buf.buf) + 1, sizeof(*buf.buf));
+		strcpy(pathspec[nr++], buf.buf);
+	}
+	ALLOC_GROW(pathspec, nr + 1, alloc);
+	pathspec[nr] = NULL;
+	int num_ignored = check_ignore(prefix, (const char **)pathspec);
+	maybe_flush_or_die(stdout, "attribute to stdout");
+	strbuf_release(&buf);
+	strbuf_release(&nbuf);
+	free(pathspec);
+	return num_ignored;
+}
+
+int cmd_check_ignore(int argc, const char **argv, const char *prefix)
+{
+	git_config(git_default_config, NULL);
+
+	argc = parse_options(argc, argv, prefix, check_ignore_options,
+			     check_ignore_usage, 0);
+
+	if (stdin_paths) {
+		if (0 < argc)
+			die(_("cannot specify pathnames with --stdin"));
+	} else {
+		if (null_term_line)
+			die(_("-z only makes sense with --stdin"));
+		if (argc == 0)
+			die(_("no path specified"));
+	}
+	if (quiet) {
+		if (argc > 1)
+			die(_("--quiet is only valid with a single pathname"));
+		if (verbose)
+			die(_("cannot have both --quiet and --verbose"));
+	}
+
+	int num_ignored = 0;
+	if (stdin_paths) {
+		num_ignored = check_ignore_stdin_paths(prefix);
+	} else {
+		num_ignored = check_ignore(prefix, argv);
+		maybe_flush_or_die(stdout, "ignore to stdout");
+	}
+
+	return num_ignored > 0 ? 0 : 1;
+}
diff --git a/command-list.txt b/command-list.txt
index 7e8cfec..bf83303 100644
--- a/command-list.txt
+++ b/command-list.txt
@@ -12,6 +12,7 @@ git-branch                              mainporcelain common
 git-bundle                              mainporcelain
 git-cat-file                            plumbinginterrogators
 git-check-attr                          purehelpers
+git-check-ignore                        purehelpers
 git-checkout                            mainporcelain common
 git-checkout-index                      plumbingmanipulators
 git-check-ref-format                    purehelpers
diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index be800e0..1efd366 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -594,6 +594,7 @@ __git_list_porcelain_commands ()
 		archimport)       : import;;
 		cat-file)         : plumbing;;
 		check-attr)       : plumbing;;
+		check-ignore)     : plumbing;;
 		check-ref-format) : plumbing;;
 		checkout-index)   : plumbing;;
 		commit-tree)      : plumbing;;
diff --git a/git.c b/git.c
index 8788b32..6caf78a 100644
--- a/git.c
+++ b/git.c
@@ -338,6 +338,7 @@ static void handle_internal_command(int argc, const char **argv)
 		{ "bundle", cmd_bundle, RUN_SETUP_GENTLY },
 		{ "cat-file", cmd_cat_file, RUN_SETUP },
 		{ "check-attr", cmd_check_attr, RUN_SETUP },
+		{ "check-ignore", cmd_check_ignore, RUN_SETUP | NEED_WORK_TREE },
 		{ "check-ref-format", cmd_check_ref_format },
 		{ "checkout", cmd_checkout, RUN_SETUP | NEED_WORK_TREE },
 		{ "checkout-index", cmd_checkout_index,
diff --git a/t/t0007-ignores.sh b/t/t0007-ignores.sh
new file mode 100755
index 0000000..a0f571e
--- /dev/null
+++ b/t/t0007-ignores.sh
@@ -0,0 +1,587 @@
+#!/bin/sh
+
+test_description=check-ignore
+
+. ./test-lib.sh
+
+init_vars () {
+	global_excludes="$HOME/global-excludes"
+}
+
+enable_global_excludes () {
+	init_vars
+	git config core.excludesfile "$global_excludes"
+}
+
+expect_in () {
+	dest="$HOME/expected-$1" text="$2"
+	if test -z "$text"
+	then
+		>"$dest" # avoid newline
+	else
+		echo -e "$text" >"$dest"
+	fi
+}
+
+expect () {
+	expect_in stdout "$1"
+}
+
+expect_from_stdin () {
+	cat >"$HOME/expected-stdout"
+}
+
+test_stderr () {
+	expected="$1"
+	expect_in stderr "$1" &&
+	test_cmp "$HOME/expected-stderr" "$HOME/stderr"
+}
+
+stderr_contains () {
+	regexp="$1"
+	if grep -q "$regexp" "$HOME/stderr"
+	then
+		return 0
+	else
+		echo "didn't find /$regexp/ in $HOME/stderr"
+		cat "$HOME/stderr"
+		return 1
+	fi
+}
+
+stderr_empty_on_success () {
+	expect_code="$1"
+	if test $expect_code = 0
+	then
+		test_stderr ""
+	else
+		# If we expect failure then stderr might or might not be empty
+		# due to --quiet - the caller can check its contents
+		return 0
+	fi
+}
+
+test_check_ignore () {
+	args="$1" expect_code="${2:-0}" global_args="$3"
+
+	init_vars &&
+	rm -f "$HOME/stdout" "$HOME/stderr" "$HOME/cmd" &&
+	echo $(which git) $global_args check-ignore $quiet_opt $verbose_opt $args \
+		>"$HOME/cmd" &&
+	pwd >"$HOME/pwd" &&
+	test_expect_code "$expect_code" \
+		git $global_args check-ignore $quiet_opt $verbose_opt $args \
+		>"$HOME/stdout" 2>"$HOME/stderr" &&
+	test_cmp "$HOME/expected-stdout" "$HOME/stdout" &&
+	stderr_empty_on_success "$expect_code"
+}
+
+test_expect_success_multi () {
+	testname="$1" expect_verbose="$2" code="$3"
+
+	expect=$( echo "$expect_verbose" | sed -e 's/.*	//' )
+
+	test_expect_success "$testname" "
+		expect '$expect' &&
+		$code
+	"
+
+	for quiet_opt in '-q' '--quiet'
+	do
+		test_expect_success "$testname${quiet_opt:+ with $quiet_opt}" "
+			expect '' &&
+			$code
+		"
+	done
+	quiet_opt=
+
+	for verbose_opt in '-v' '--verbose'
+	do
+		test_expect_success "$testname${verbose_opt:+ with $verbose_opt}" "
+			expect '$expect_verbose' &&
+			$code
+		"
+	done
+	verbose_opt=
+}
+
+test_expect_success 'setup' '
+	init_vars
+	mkdir -p a/b/ignored-dir a/submodule b &&
+	ln -s b a/symlink &&
+	(
+		cd a/submodule &&
+		git init &&
+		echo a > a &&
+		git add a &&
+		git commit -m"commit in submodule"
+	) &&
+	git add a/submodule &&
+	cat <<-\EOF >.gitignore &&
+		one
+	EOF
+	cat <<-\EOF >a/.gitignore &&
+		two*
+		*three
+	EOF
+	cat <<-\EOF >a/b/.gitignore &&
+		four
+		five
+		# this comment should affect the line numbers
+		six
+		ignored-dir/
+		# and so should this blank line:
+
+		!on*
+		!two
+	EOF
+	echo "seven" >a/b/ignored-dir/.gitignore &&
+	test -n "$HOME" &&
+	cat <<-\EOF >"$global_excludes" &&
+		globalone
+		!globaltwo
+		globalthree
+	EOF
+	cat <<-\EOF >>.git/info/exclude
+		per-repo
+	EOF
+'
+
+############################################################################
+#
+# test invalid inputs
+
+test_expect_success_multi 'empty command line' '' '
+	test_check_ignore "" 128 &&
+	stderr_contains "fatal: no path specified"
+'
+
+test_expect_success '-q with multiple args' '
+	expect "" &&
+	test_check_ignore "-q one two" 128 &&
+	stderr_contains "fatal: --quiet is only valid with a single pathname"
+'
+
+test_expect_success '--quiet with multiple args' '
+	expect "" &&
+	test_check_ignore "--quiet one two" 128 &&
+	stderr_contains "fatal: --quiet is only valid with a single pathname"
+'
+
+for verbose_opt in '-v' '--verbose'
+do
+	for quiet_opt in '-q' '--quiet'
+	do
+		test_expect_success "$quiet_opt $verbose_opt" "
+			expect '' &&
+			test_check_ignore '$quiet_opt $verbose_opt foo' 128 &&
+			stderr_contains 'fatal: cannot have both --quiet and --verbose'
+		"
+	done
+done
+
+test_expect_success '--quiet with multiple args' '
+	expect "" &&
+	test_check_ignore "--quiet one two" 128 &&
+	stderr_contains "fatal: --quiet is only valid with a single pathname"
+'
+
+test_expect_success_multi 'erroneous use of --' '' '
+	test_check_ignore "--" 128 &&
+	stderr_contains "fatal: no path specified"
+'
+
+test_expect_success_multi '--stdin with superfluous arg' '' '
+	test_check_ignore "--stdin foo" 128 &&
+	stderr_contains "fatal: cannot specify pathnames with --stdin"
+'
+
+test_expect_success_multi '--stdin -z with superfluous arg' '' '
+	test_check_ignore "--stdin -z foo" 128 &&
+	stderr_contains "fatal: cannot specify pathnames with --stdin"
+'
+
+test_expect_success_multi '-z without --stdin' '' '
+	test_check_ignore "-z" 128 &&
+	stderr_contains "fatal: -z only makes sense with --stdin"
+'
+
+test_expect_success_multi '-z without --stdin and superfluous arg' '' '
+	test_check_ignore "-z foo" 128 &&
+	stderr_contains "fatal: -z only makes sense with --stdin"
+'
+
+test_expect_success_multi 'needs work tree' '' '
+	(
+		cd .git &&
+		test_check_ignore "foo" 128
+	) &&
+	stderr_contains "fatal: This operation must be run in a work tree"
+'
+
+############################################################################
+#
+# test standard ignores
+
+test_expect_success_multi "top-level not ignored" '' '
+	test_check_ignore "foo" 1
+'
+
+test_expect_success_multi "top-level ignored" \
+	'.gitignore:1:one	one' '
+	test_check_ignore "one"
+'
+
+test_expect_success_multi 'sub-directory ignore from top' \
+	'.gitignore:1:one	a/one' '
+	test_check_ignore "a/one"
+'
+
+test_expect_success 'sub-directory local ignore' '
+	expect "a/3-three" &&
+	test_check_ignore "a/3-three a/three-not-this-one"
+'
+
+test_expect_success 'sub-directory local ignore with --verbose'  '
+	expect "a/.gitignore:2:*three	a/3-three" &&
+	test_check_ignore "--verbose a/3-three a/three-not-this-one"
+'
+
+test_expect_success 'local ignore inside a sub-directory' '
+	expect "3-three" &&
+	(
+		cd a &&
+		test_check_ignore "3-three three-not-this-one"
+	)
+'
+test_expect_success 'local ignore inside a sub-directory with --verbose' '
+	expect "a/.gitignore:2:*three	3-three" &&
+	(
+		cd a &&
+		test_check_ignore "--verbose 3-three three-not-this-one"
+	)
+'
+
+test_expect_success_multi 'nested include' \
+	'a/b/.gitignore:8:!on*	a/b/one' '
+	test_check_ignore "a/b/one"
+'
+
+############################################################################
+#
+# test ignored sub-directories
+
+test_expect_success_multi 'ignored sub-directory' \
+	'a/b/.gitignore:5:ignored-dir/	a/b/ignored-dir' '
+	test_check_ignore "a/b/ignored-dir"
+'
+
+test_expect_success 'multiple files inside ignored sub-directory' '
+	expect_from_stdin <<-\EOF &&
+		a/b/ignored-dir/foo
+		a/b/ignored-dir/twoooo
+		a/b/ignored-dir/seven
+	EOF
+	test_check_ignore "a/b/ignored-dir/foo a/b/ignored-dir/twoooo a/b/ignored-dir/seven"
+'
+
+test_expect_success 'multiple files inside ignored sub-directory with -v' '
+	expect_from_stdin <<-\EOF &&
+		a/b/.gitignore:5:ignored-dir/	a/b/ignored-dir/foo
+		a/b/.gitignore:5:ignored-dir/	a/b/ignored-dir/twoooo
+		a/b/.gitignore:5:ignored-dir/	a/b/ignored-dir/seven
+	EOF
+	test_check_ignore "-v a/b/ignored-dir/foo a/b/ignored-dir/twoooo a/b/ignored-dir/seven"
+'
+
+test_expect_success 'cd to ignored sub-directory' '
+	expect_from_stdin <<-\EOF &&
+		foo
+		twoooo
+		../one
+		seven
+		../../one
+	EOF
+	(
+		cd a/b/ignored-dir &&
+		test_check_ignore "foo twoooo ../one seven ../../one"
+	)
+'
+
+test_expect_success 'cd to ignored sub-directory with -v' '
+	expect_from_stdin <<-\EOF &&
+		a/b/.gitignore:5:ignored-dir/	foo
+		a/b/.gitignore:5:ignored-dir/	twoooo
+		a/b/.gitignore:8:!on*	../one
+		a/b/.gitignore:5:ignored-dir/	seven
+		.gitignore:1:one	../../one
+	EOF
+	(
+		cd a/b/ignored-dir &&
+		test_check_ignore "-v foo twoooo ../one seven ../../one"
+	)
+'
+
+############################################################################
+#
+# test handling of symlinks
+
+test_expect_success_multi 'symlink' '' '
+	test_check_ignore "a/symlink" 1
+'
+
+test_expect_success_multi 'beyond a symlink' '' '
+	test_check_ignore "a/symlink/foo" 128 &&
+	test_stderr "fatal: '\''a/symlink/foo'\'' is beyond a symbolic link"
+'
+
+test_expect_success_multi 'beyond a symlink from subdirectory' '' '
+	(
+		cd a &&
+		test_check_ignore "symlink/foo" 128
+	) &&
+	test_stderr "fatal: '\''symlink/foo'\'' is beyond a symbolic link"
+'
+
+############################################################################
+#
+# test handling of submodules
+
+test_expect_success_multi 'submodule' '' '
+	test_check_ignore "a/submodule/one" 128 &&
+	test_stderr "fatal: Path '\''a/submodule/one'\'' is in submodule '\''a/submodule'\''"
+'
+
+test_expect_success_multi 'submodule from subdirectory' '' '
+	(
+		cd a &&
+		test_check_ignore "submodule/one" 128
+	) &&
+	test_stderr "fatal: Path '\''a/submodule/one'\'' is in submodule '\''a/submodule'\''"
+'
+
+############################################################################
+#
+# test handling of global ignore files
+
+test_expect_success 'global ignore not yet enabled' '
+	expect_from_stdin <<-\EOF &&
+		.git/info/exclude:7:per-repo	per-repo
+		a/.gitignore:2:*three	a/globalthree
+		.git/info/exclude:7:per-repo	a/per-repo
+	EOF
+	test_check_ignore "-v globalone per-repo a/globalthree a/per-repo not-ignored a/globaltwo"
+'
+
+test_expect_success 'global ignore' '
+	enable_global_excludes &&
+	expect_from_stdin <<-\EOF &&
+		globalone
+		per-repo
+		globalthree
+		a/globalthree
+		a/per-repo
+		globaltwo
+	EOF
+	test_check_ignore "globalone per-repo globalthree a/globalthree a/per-repo not-ignored globaltwo"
+'
+
+test_expect_success 'global ignore with -v' '
+	enable_global_excludes &&
+	expect_from_stdin <<-EOF &&
+		$global_excludes:1:globalone	globalone
+		.git/info/exclude:7:per-repo	per-repo
+		$global_excludes:3:globalthree	globalthree
+		a/.gitignore:2:*three	a/globalthree
+		.git/info/exclude:7:per-repo	a/per-repo
+		$global_excludes:2:!globaltwo	globaltwo
+	EOF
+	test_check_ignore "-v globalone per-repo globalthree a/globalthree a/per-repo not-ignored globaltwo"
+'
+
+############################################################################
+#
+# test --stdin
+
+cat <<-\EOF >stdin
+	one
+	not-ignored
+	a/one
+	a/not-ignored
+	a/b/on
+	a/b/one
+	a/b/one one
+	"a/b/one two"
+	"a/b/one\"three"
+	a/b/not-ignored
+	a/b/two
+	a/b/twooo
+	globaltwo
+	a/globaltwo
+	a/b/globaltwo
+	b/globaltwo
+EOF
+cat <<-\EOF >expected-default
+	one
+	a/one
+	a/b/on
+	a/b/one
+	a/b/one one
+	a/b/one two
+	"a/b/one\"three"
+	a/b/two
+	a/b/twooo
+	globaltwo
+	a/globaltwo
+	a/b/globaltwo
+	b/globaltwo
+EOF
+cat <<-EOF >expected-verbose
+	.gitignore:1:one	one
+	.gitignore:1:one	a/one
+	a/b/.gitignore:8:!on*	a/b/on
+	a/b/.gitignore:8:!on*	a/b/one
+	a/b/.gitignore:8:!on*	a/b/one one
+	a/b/.gitignore:8:!on*	a/b/one two
+	a/b/.gitignore:8:!on*	"a/b/one\"three"
+	a/b/.gitignore:9:!two	a/b/two
+	a/.gitignore:1:two*	a/b/twooo
+	$global_excludes:2:!globaltwo	globaltwo
+	$global_excludes:2:!globaltwo	a/globaltwo
+	$global_excludes:2:!globaltwo	a/b/globaltwo
+	$global_excludes:2:!globaltwo	b/globaltwo
+EOF
+
+sed -e 's/^"//' -e 's/\\//' -e 's/"$//' stdin | \
+	tr "\n" "\0" >stdin0
+sed -e 's/^"//' -e 's/\\//' -e 's/"$//' expected-default | \
+	tr "\n" "\0" >expected-default0
+sed -e 's/	"/	/' -e 's/\\//' -e 's/"$//' expected-verbose | \
+	tr ":\t\n" "\0" >expected-verbose0
+
+test_expect_success '--stdin' '
+	expect_from_stdin <expected-default &&
+	test_check_ignore "--stdin" <stdin
+'
+
+test_expect_success '--stdin -q' '
+	expect "" &&
+	test_check_ignore "-q --stdin" <stdin
+'
+
+test_expect_success '--stdin -v' '
+	expect_from_stdin <expected-verbose &&
+	test_check_ignore "-v --stdin" <stdin
+'
+
+for opts in '--stdin -z' '-z --stdin'
+do
+	test_expect_success "$opts" "
+		expect_from_stdin <expected-default0 &&
+		test_check_ignore '$opts' <stdin0
+	"
+
+	test_expect_success "$opts -q" "
+		expect "" &&
+		test_check_ignore '-q $opts' <stdin0
+	"
+
+	test_expect_success "$opts -v" "
+		expect_from_stdin <expected-verbose0 &&
+		test_check_ignore '-v $opts' <stdin0
+	"
+done
+
+cat <<-\EOF >stdin
+	../one
+	../not-ignored
+	one
+	not-ignored
+	b/on
+	b/one
+	b/one one
+	"b/one two"
+	"b/one\"three"
+	b/two
+	b/not-ignored
+	b/twooo
+	../globaltwo
+	globaltwo
+	b/globaltwo
+	../b/globaltwo
+EOF
+cat <<-\EOF >expected-default
+	../one
+	one
+	b/on
+	b/one
+	b/one one
+	b/one two
+	"b/one\"three"
+	b/two
+	b/twooo
+	../globaltwo
+	globaltwo
+	b/globaltwo
+	../b/globaltwo
+EOF
+cat <<-EOF >expected-verbose
+	.gitignore:1:one	../one
+	.gitignore:1:one	one
+	a/b/.gitignore:8:!on*	b/on
+	a/b/.gitignore:8:!on*	b/one
+	a/b/.gitignore:8:!on*	b/one one
+	a/b/.gitignore:8:!on*	b/one two
+	a/b/.gitignore:8:!on*	"b/one\"three"
+	a/b/.gitignore:9:!two	b/two
+	a/.gitignore:1:two*	b/twooo
+	$global_excludes:2:!globaltwo	../globaltwo
+	$global_excludes:2:!globaltwo	globaltwo
+	$global_excludes:2:!globaltwo	b/globaltwo
+	$global_excludes:2:!globaltwo	../b/globaltwo
+EOF
+
+sed -e 's/^"//' -e 's/\\//' -e 's/"$//' stdin | \
+	tr "\n" "\0" >stdin0
+sed -e 's/^"//' -e 's/\\//' -e 's/"$//' expected-default | \
+	tr "\n" "\0" >expected-default0
+sed -e 's/	"/	/' -e 's/\\//' -e 's/"$//' expected-verbose | \
+	tr ":\t\n" "\0" >expected-verbose0
+
+test_expect_success '--stdin from subdirectory' '
+	expect_from_stdin <expected-default &&
+	(
+		cd a &&
+		test_check_ignore "--stdin" <../stdin
+	)
+'
+
+test_expect_success '--stdin from subdirectory with -v' '
+	expect_from_stdin <expected-verbose &&
+	(
+		cd a &&
+		test_check_ignore "--stdin -v" <../stdin
+	)
+'
+
+for opts in '--stdin -z' '-z --stdin'
+do
+	test_expect_success "$opts from subdirectory" '
+		expect_from_stdin <expected-default0 &&
+		(
+			cd a &&
+			test_check_ignore "'"$opts"'" <../stdin0
+		)
+	'
+
+	test_expect_success "$opts from subdirectory with -v" '
+		expect_from_stdin <expected-verbose0 &&
+		(
+			cd a &&
+			test_check_ignore "'"$opts"' -v" <../stdin0
+		)
+	'
+done
+
+
+test_done
diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
index 92d7eb4..cce51ac 100755
--- a/t/t9902-completion.sh
+++ b/t/t9902-completion.sh
@@ -213,19 +213,19 @@ test_expect_success 'general options' '
 '
 
 test_expect_success 'general options plus command' '
-	test_completion "git --version check" "checkout " &&
-	test_completion "git --paginate check" "checkout " &&
-	test_completion "git --git-dir=foo check" "checkout " &&
-	test_completion "git --bare check" "checkout " &&
+	test_completion "git --version checko" "checkout " &&
+	test_completion "git --paginate checko" "checkout " &&
+	test_completion "git --git-dir=foo checko" "checkout " &&
+	test_completion "git --bare checko" "checkout " &&
 	test_completion "git --help des" "describe " &&
-	test_completion "git --exec-path=foo check" "checkout " &&
-	test_completion "git --html-path check" "checkout " &&
-	test_completion "git --no-pager check" "checkout " &&
-	test_completion "git --work-tree=foo check" "checkout " &&
-	test_completion "git --namespace=foo check" "checkout " &&
-	test_completion "git --paginate check" "checkout " &&
-	test_completion "git --info-path check" "checkout " &&
-	test_completion "git --no-replace-objects check" "checkout "
+	test_completion "git --exec-path=foo checko" "checkout " &&
+	test_completion "git --html-path checko" "checkout " &&
+	test_completion "git --no-pager checko" "checkout " &&
+	test_completion "git --work-tree=foo checko" "checkout " &&
+	test_completion "git --namespace=foo checko" "checkout " &&
+	test_completion "git --paginate checko" "checkout " &&
+	test_completion "git --info-path checko" "checkout " &&
+	test_completion "git --no-replace-objects checko" "checkout "
 '
 
 test_done
-- 
1.7.12.147.g6d168f4

Re: [PATCH v2 14/14] Add git-check-ignore sub-command

[Johannes Sixt]

Am 9/20/2012 21:46, schrieb Adam Spiers:
>  test_expect_success 'general options plus command' '
> -	test_completion "git --version check" "checkout " &&
> -	test_completion "git --paginate check" "checkout " &&
> -	test_completion "git --git-dir=foo check" "checkout " &&
> -	test_completion "git --bare check" "checkout " &&
> +	test_completion "git --version checko" "checkout " &&
> +	test_completion "git --paginate checko" "checkout " &&
> +	test_completion "git --git-dir=foo checko" "checkout " &&
> +	test_completion "git --bare checko" "checkout " &&
> ...

I find this worrysome. Is check-ignore, being a debugging aid, so
important that it must be autocompleted?

-- Hannes

Re: [PATCH v2 14/14] Add git-check-ignore sub-command

[Junio C Hamano]

Johannes Sixt <j.sixt@viscovery.net> writes:

> Am 9/20/2012 21:46, schrieb Adam Spiers:
>>  test_expect_success 'general options plus command' '
>> -	test_completion "git --version check" "checkout " &&
>> -	test_completion "git --paginate check" "checkout " &&
>> -	test_completion "git --git-dir=foo check" "checkout " &&
>> -	test_completion "git --bare check" "checkout " &&
>> +	test_completion "git --version checko" "checkout " &&
>> +	test_completion "git --paginate checko" "checkout " &&
>> +	test_completion "git --git-dir=foo checko" "checkout " &&
>> +	test_completion "git --bare checko" "checkout " &&
>> ...
>
> I find this worrysome. Is check-ignore, being a debugging aid, so
> important that it must be autocompleted?

The shell function __git_list_porcelain_commands in contrib/completion/
starts from "git help -a" and filters plumbing commands and helpers
via a blacklist.  At least, check-ignore needs to be added there.

These days, we do not add random subcommands willy-nilly (I still
doubt if check-ignore needs to be a separate debugging command, or a
new mode of operation of ls-files or something), so the approach to
use a blacklist makes more sense.  "help -a" is designed to show
whatever the users throw in their ~/bin (assuming that is on $PATH)
under git-whatever name, and we _do_ want to complete "git wh<TAB>"
to that custom command, so a whitelist-based solution is unwieldy to
construct.

Re: [PATCH v2 14/14] Add git-check-ignore sub-command

[Johannes Sixt]

Am 9/26/2012 1:25, schrieb Junio C Hamano:
> Johannes Sixt <j.sixt@viscovery.net> writes:
> 
>> Am 9/20/2012 21:46, schrieb Adam Spiers:
>>>  test_expect_success 'general options plus command' '
>>> -	test_completion "git --version check" "checkout " &&
>>> -	test_completion "git --paginate check" "checkout " &&
>>> -	test_completion "git --git-dir=foo check" "checkout " &&
>>> -	test_completion "git --bare check" "checkout " &&
>>> +	test_completion "git --version checko" "checkout " &&
>>> +	test_completion "git --paginate checko" "checkout " &&
>>> +	test_completion "git --git-dir=foo checko" "checkout " &&
>>> +	test_completion "git --bare checko" "checkout " &&
>>> ...
>>
>> I find this worrysome. Is check-ignore, being a debugging aid, so
>> important that it must be autocompleted?
> 
> The shell function __git_list_porcelain_commands in contrib/completion/
> starts from "git help -a" and filters plumbing commands and helpers
> via a blacklist.  At least, check-ignore needs to be added there.
> 
> These days, we do not add random subcommands willy-nilly (I still
> doubt if check-ignore needs to be a separate debugging command, or a
> new mode of operation of ls-files or something), so the approach to
> use a blacklist makes more sense.  "help -a" is designed to show
> whatever the users throw in their ~/bin (assuming that is on $PATH)
> under git-whatever name, and we _do_ want to complete "git wh<TAB>"
> to that custom command, so a whitelist-based solution is unwieldy to
> construct.

We already have 'git check-attr', but it is obviously not among the
autocompleted commands, otherwise the above test would not have passed.
IMO, 'git check-ignore' falls into the same category as 'git check-attr'
with regard to completion.

-- Hannes

Re: [PATCH v2 14/14] Add git-check-ignore sub-command

[Junio C Hamano]

Johannes Sixt <j.sixt@viscovery.net> writes:

>> These days, we do not add random subcommands willy-nilly (I still
>> doubt if check-ignore needs to be a separate debugging command, or a
>> new mode of operation of ls-files or something), so the approach to
>> use a blacklist makes more sense.  "help -a" is designed to show
>> whatever the users throw in their ~/bin (assuming that is on $PATH)
>> under git-whatever name, and we _do_ want to complete "git wh<TAB>"
>> to that custom command, so a whitelist-based solution is unwieldy to
>> construct.
>
> We already have 'git check-attr', but it is obviously not among the
> autocompleted commands, otherwise the above test would not have passed.
> IMO, 'git check-ignore' falls into the same category as 'git check-attr'
> with regard to completion.

Exactly.

Actually I think what happened was that the submitter didn't have
change to contrib/completion/ in earlier private versions, saw the
test fail and updated t9902 without thinking.  The patch posted has
addition to contrib/completion/ that blacklists check-ignore the
same way as check-attr, which made the change to t9902 unnecessary
but because the update was done without thinking, it wasn't even
realized that the test would have passed without the patch to it.

Reverting the part that touches t9902 should still pass, I would
think.

 t/t9902-completion.sh | 24 ++++++++++++------------
 1 file changed, 12 insertions(+), 12 deletions(-)

diff --git c/t/t9902-completion.sh w/t/t9902-completion.sh
index cce51ac..92d7eb4 100755
--- c/t/t9902-completion.sh
+++ w/t/t9902-completion.sh
@@ -213,19 +213,19 @@ test_expect_success 'general options' '
 '
 
 test_expect_success 'general options plus command' '
-	test_completion "git --version checko" "checkout " &&
-	test_completion "git --paginate checko" "checkout " &&
-	test_completion "git --git-dir=foo checko" "checkout " &&
-	test_completion "git --bare checko" "checkout " &&
+	test_completion "git --version check" "checkout " &&
+	test_completion "git --paginate check" "checkout " &&
+	test_completion "git --git-dir=foo check" "checkout " &&
+	test_completion "git --bare check" "checkout " &&
 	test_completion "git --help des" "describe " &&
-	test_completion "git --exec-path=foo checko" "checkout " &&
-	test_completion "git --html-path checko" "checkout " &&
-	test_completion "git --no-pager checko" "checkout " &&
-	test_completion "git --work-tree=foo checko" "checkout " &&
-	test_completion "git --namespace=foo checko" "checkout " &&
-	test_completion "git --paginate checko" "checkout " &&
-	test_completion "git --info-path checko" "checkout " &&
-	test_completion "git --no-replace-objects checko" "checkout "
+	test_completion "git --exec-path=foo check" "checkout " &&
+	test_completion "git --html-path check" "checkout " &&
+	test_completion "git --no-pager check" "checkout " &&
+	test_completion "git --work-tree=foo check" "checkout " &&
+	test_completion "git --namespace=foo check" "checkout " &&
+	test_completion "git --paginate check" "checkout " &&
+	test_completion "git --info-path check" "checkout " &&
+	test_completion "git --no-replace-objects check" "checkout "
 '
 
 test_done

Re: [PATCH v2 14/14] Add git-check-ignore sub-command

[Michael Haggerty]

On 09/20/2012 09:46 PM, Adam Spiers wrote:
> This works in a similar manner to git-check-attr.  Some code
> was reused from add.c by refactoring out into pathspec.c.
> 
> Thanks to Jeff King and Junio C Hamano for the idea:
> http://thread.gmane.org/gmane.comp.version-control.git/108671/focus=108815
> 
> Signed-off-by: Adam Spiers <git@adamspiers.org>
> ---
>  .gitignore                             |   1 +
>  Documentation/git-check-ignore.txt     |  85 +++++
>  Documentation/gitignore.txt            |   6 +-
>  Makefile                               |   1 +
>  builtin.h                              |   1 +
>  builtin/check-ignore.c                 | 167 ++++++++++
>  command-list.txt                       |   1 +
>  contrib/completion/git-completion.bash |   1 +
>  git.c                                  |   1 +
>  t/t0007-ignores.sh                     | 587 +++++++++++++++++++++++++++++++++
>  t/t9902-completion.sh                  |  24 +-
>  11 files changed, 861 insertions(+), 14 deletions(-)
>  create mode 100644 Documentation/git-check-ignore.txt
>  create mode 100644 builtin/check-ignore.c
>  create mode 100755 t/t0007-ignores.sh
> 
> diff --git a/.gitignore b/.gitignore
> index a188a82..11cd975 100644
> --- a/.gitignore
> +++ b/.gitignore
> @@ -22,6 +22,7 @@
>  /git-bundle
>  /git-cat-file
>  /git-check-attr
> +/git-check-ignore
>  /git-check-ref-format
>  /git-checkout
>  /git-checkout-index
> diff --git a/Documentation/git-check-ignore.txt b/Documentation/git-check-ignore.txt
> new file mode 100644
> index 0000000..2de4e17
> --- /dev/null
> +++ b/Documentation/git-check-ignore.txt
> @@ -0,0 +1,85 @@
> +git-check-ignore(1)
> +=================
> +
> +NAME
> +----
> +git-check-ignore - Debug gitignore / exclude files
> +
> +
> +SYNOPSIS
> +--------
> +[verse]
> +'git check-ignore' [options] pathname...
> +'git check-ignore' [options] --stdin < <list-of-paths>
> +
> +DESCRIPTION
> +-----------
> +
> +For each pathname given via the command-line or from a file via
> +`--stdin`, this command will list the first exclude pattern found (if
> +any) which explicitly excludes or includes that pathname.  Note that
> +within any given exclude file, later patterns take precedence over
> +earlier ones, so any matching pattern which this command outputs may
> +not be the one you would immediately expect.

Can I tell from the output of "git check-ignore" whether a file is
really ignored?  The way I read the paragraph above, the output doesn't
necessarily show the pattern that determines whether a file is *really*
ignored.  That makes it sound like the ignore status of the file might
be different than what I would infer from the output.  If I am
misunderstanding the situation, then perhaps the explanation in the
above paragraph can be improved.

On the other hand, if my understanding is correct, then why did you
choose this (seemingly strange) policy?  It would seem more useful
either to output the pattern that has the definitive effect on the
file's status, or to output all patterns that match the file.

> +OPTIONS
> +-------
> +-q, --quiet::
> +	Don't output anything, just set exit status.  This is only
> +	valid with a single pathname.
> +
> +-v, --verbose::
> +	Also output details about the matching pattern (if any)
> +	for each given pathname.
> +
> +--stdin::
> +	Read file names from stdin instead of from the command-line.
> +
> +-z::
> +	The output format is modified to be machine-parseable (see
> +	below).  If `--stdin` is also given, input paths are separated
> +	with a NUL character instead of a linefeed character.
> +
> +OUTPUT
> +------
> +
> +By default, any of the given pathnames which match an ignore pattern
> +will be output, one per line.  If no pattern matches a given path,
> +nothing will be output for that path; this means that path will not be
> +ignored.
> +
> +If `--verbose` is specified, the output is a series of lines of the form:
> +
> +<source> <COLON> <linenum> <COLON> <pattern> <HT> <pathname>
> +
> +<pathname> is the path of a file being queried, <pattern> is the
> +matching pattern, <source> is the pattern's source file, and <linenum>
> +is the line number of the pattern within that source.  If the pattern
> +contained a `!` prefix or `/` suffix, it will be preserved in the
> +output.  <source> will be an absolute path when referring to the file
> +configured by `core.excludesfile`, or relative to the repository root
> +when referring to `.git/info/exclude` or a per-directory exclude file.
> +
> +If `-z` is specified, the output is a series of lines of the form:
> +
> +EXIT STATUS
> +-----------
> [...]

I think you forgot to finish the thought about "If -z is specified".

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/

Re: [PATCH v2 14/14] Add git-check-ignore sub-command

[Junio C Hamano]

Michael Haggerty <mhagger@alum.mit.edu> writes:

>> +For each pathname given via the command-line or from a file via
>> +`--stdin`, this command will list the first exclude pattern found (if
>> +any) which explicitly excludes or includes that pathname.  Note that
>> +within any given exclude file, later patterns take precedence over
>> +earlier ones, so any matching pattern which this command outputs may
>> +not be the one you would immediately expect.
>
> Can I tell from the output of "git check-ignore" whether a file is
> really ignored?  The way I read the paragraph above, the output doesn't
> necessarily show the pattern that determines whether a file is *really*
> ignored.  That makes it sound like the ignore status of the file might
> be different than what I would infer from the output.  If I am
> misunderstanding the situation, then perhaps the explanation in the
> above paragraph can be improved.
>
> On the other hand, if my understanding is correct, then why did you
> choose this (seemingly strange) policy?  It would seem more useful
> either to output the pattern that has the definitive effect on the
> file's status, or to output all patterns that match the file.

Very good point. I didn't look at this patch at all, but I would
guess the patch hooked this into the order in which the existing
ignore mechanism computes the match, and "the first I find" Adam
says is written from implementation point of view.

And that is a wrong way to go about it.

The existing document for the ignore mechanism explains things from
the end user perspective, i.e. within a file, the last matching
pattern determines the fate of the path.  And this debugging aid
should report which pattern from what source determined the fate of
the path, so from the end user perspective, saying "first" is
nonsense.

It just happens that the implementation optimizes by scanning the
list of patterns "backwards" and stops at the "first" hit, which is
the last matching pattern that determines the fate of the path.

The documentation of this debugging aid should use the same phrasing
"git help ignore" uses and say "show the pattern that decides the
outcome", and lose that confusing "Note that" that is not helping
anybody.

Re: [PATCH v2 14/14] Add git-check-ignore sub-command

[Junio C Hamano]

Adam Spiers <git@adamspiers.org> writes:

> +expect_in () {
> +	dest="$HOME/expected-$1" text="$2"
> +	if test -z "$text"
> +	then
> +		>"$dest" # avoid newline
> +	else
> +		echo -e "$text" >"$dest"

This breaks when your shell is not "bash".

> +test_check_ignore () {
> +	args="$1" expect_code="${2:-0}" global_args="$3"
> +
> +	init_vars &&
> +	rm -f "$HOME/stdout" "$HOME/stderr" "$HOME/cmd" &&
> +	echo $(which git) $global_args check-ignore $quiet_opt $verbose_opt $args \
> +		>"$HOME/cmd" &&

Don't use which in scripts.  Is this just leftover cruft from debugging?

Re: [PATCH v2 00/14] new git check-ignore sub-command

[Junio C Hamano]

Adam Spiers <git@adamspiers.org> writes:

> Adam Spiers (14):
>   Update directory listing API doc to match code
>   Improve documentation and comments regarding directory traversal API
>   Rename cryptic 'which' variable to more consistent name
>   Rename path_excluded() to is_path_excluded()
>   Rename excluded_from_list() to is_excluded_from_list()
>   Rename excluded() to is_excluded()
>   Refactor is_excluded_from_list()
>   Refactor is_excluded()
>   Refactor is_path_excluded()
>   For each exclude pattern, store information about where it came from
>   Refactor treat_gitlinks()
>   Extract some useful pathspec handling code from builtin/add.c into a
>     library
>   Provide free_directory() for reclaiming dir_struct memory
>   Add git-check-ignore sub-command

Please retitle these to have a short "prefix: " that names a
specific area the series intends to touch.  I retitled your other
series to share "test :" as their common prefix.

Thanks.

Re: [PATCH v2 00/14] new git check-ignore sub-command

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Adam Spiers <git@adamspiers.org> writes:
>
>> Adam Spiers (14):
>>   Update directory listing API doc to match code
>>   Improve documentation and comments regarding directory traversal API
>>   Rename cryptic 'which' variable to more consistent name
>>   Rename path_excluded() to is_path_excluded()
>>   Rename excluded_from_list() to is_excluded_from_list()
>>   Rename excluded() to is_excluded()
>>   Refactor is_excluded_from_list()
>>   Refactor is_excluded()
>>   Refactor is_path_excluded()
>>   For each exclude pattern, store information about where it came from
>>   Refactor treat_gitlinks()
>>   Extract some useful pathspec handling code from builtin/add.c into a
>>     library
>>   Provide free_directory() for reclaiming dir_struct memory
>>   Add git-check-ignore sub-command
>
> Please retitle these to have a short "prefix: " that names a
> specific area the series intends to touch.  I retitled your other
> series to share "test :" as their common prefix.

Just to clarify, I think most of them can say "dir.c: ".

I saw quite a few decl-after-statement in new code.  Please fix
them.

As to the "who owns x->src and when is it freed" question, it may
make sense to give el a "filename" field (command line and other
special cases would get whatever value you deem appropriate, like
NULL or "<command line>"), have x->src point at that field when you
queue many x's to the el in add_exc_from_file_to_list().  Then when
you pop an element in the exclude_stack, you can free el->filename
to plug a potential leak.

Also I do not see why you need to have the strdup() in the caller of
add_excludes_from_file_to_list().  If you need to keep it stable
because you are copying it away in exclude or excludde_list,
wouldn't it make more sense to do that at the beginning of the
callee, i.e. add_excludes_from_file_to_list() function?

Re: [PATCH v2 00/14] new git check-ignore sub-command

[Adam Spiers]

On Thu, Sep 20, 2012 at 10:43 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Junio C Hamano <gitster@pobox.com> writes:
>> Adam Spiers <git@adamspiers.org> writes:
>>> Adam Spiers (14):
>>>   Update directory listing API doc to match code
>>>   Improve documentation and comments regarding directory traversal API
>>>   Rename cryptic 'which' variable to more consistent name
>>>   Rename path_excluded() to is_path_excluded()
>>>   Rename excluded_from_list() to is_excluded_from_list()
>>>   Rename excluded() to is_excluded()
>>>   Refactor is_excluded_from_list()
>>>   Refactor is_excluded()
>>>   Refactor is_path_excluded()
>>>   For each exclude pattern, store information about where it came from
>>>   Refactor treat_gitlinks()
>>>   Extract some useful pathspec handling code from builtin/add.c into a
>>>     library
>>>   Provide free_directory() for reclaiming dir_struct memory
>>>   Add git-check-ignore sub-command
>>
>> Please retitle these to have a short "prefix: " that names a
>> specific area the series intends to touch.  I retitled your other
>> series to share "test :" as their common prefix.
>
> Just to clarify, I think most of them can say "dir.c: ".

Sure, I can do that, but shouldn't this convention be documented in
SubmittingPatches?

> I saw quite a few decl-after-statement in new code.  Please fix
> them.

Again, I can do that no problem, but again this convention is
undocumented and I am not psychic ;-)  I see that a patch was provided
5 years ago to document this, but was apparently never pulled in:

    http://thread.gmane.org/gmane.comp.version-control.git/47843/focus=48015

It would save everybody's time if these things are documented, since
then they wouldn't create noise during the review process.

I also see in the same thread that a patch to add
-Wdeclaration-after-statement to CFLAGS was also offered but never
pulled in, presumably on the stated grounds that that option was
relatively recent 5 years ago.  But wouldn't it be trivial to do

    gcc -v --help 2>&1 | grep declaration-after-statement

at build-time?

I'm also curious to know why this convention exists.  Are people
really still compiling git with compilers which don't support C99?

> As to the "who owns x->src and when is it freed" question, it may
> make sense to give el a "filename" field (command line and other
> special cases would get whatever value you deem appropriate, like
> NULL or "<command line>"), have x->src point at that field when you
> queue many x's to the el in add_exc_from_file_to_list().  Then when
> you pop an element in the exclude_stack, you can free el->filename
> to plug a potential leak.
>
> Also I do not see why you need to have the strdup() in the caller of
> add_excludes_from_file_to_list().  If you need to keep it stable
> because you are copying it away in exclude or excludde_list,
> wouldn't it make more sense to do that at the beginning of the
> callee, i.e. add_excludes_from_file_to_list() function?

Thanks, I'll take a look at these suggestions tomorrow.

Re: [PATCH v2 00/14] new git check-ignore sub-command

[Junio C Hamano]

Adam Spiers <git@adamspiers.org> writes:

> Sure, I can do that, but shouldn't this convention be documented in
> SubmittingPatches?

People must have learned this by imitating what senior contributors
send to the list, but the "[Subject] area: title" thing does not
appear in that document.  I agree it should (patches welcome).

>> I saw quite a few decl-after-statement in new code.  Please fix
>> them.
>
> Again, I can do that no problem, but again this convention is
> undocumented and I am not psychic.

Yeah, when there is no code that does decl-after-statement, with the
"imitate surrounding code" rule alone, I agree that it is a bit hard
to tell we do not allow it (as opposed to seeing a construct is
often used and assuming that the construct is permitted, which is
much easier).

> I see that a patch was provided
> 5 years ago to document this, but was apparently never pulled in:
>
>     http://thread.gmane.org/gmane.comp.version-control.git/47843/focus=48015

I just read SubmittingPatches again and looked for 1a as found in
that patch, and it is there.

> I also see in the same thread that a patch to add
> -Wdeclaration-after-statement to CFLAGS was also offered but never
> pulled in,

There is no guarantee your CC would understand it; you don't even
know if it is a gcc in the first place.

> I'm also curious to know why this convention exists.  Are people
> really still compiling git with compilers which don't support C99?

See 6d62c98 (Makefile: Change the default compiler from "gcc" to
"cc", 2011-12-20).

[PATCH 0/3] Help newbie git developers avoid obvious pitfalls

[Adam Spiers]

I fell into various newbie pitfalls when submitting my first patches
to git, despite my best attempts to adhere to documented guidelines.
This small patch series attempts to reduce the chances of other
developers making the same mistakes I did.

Adam Spiers (3):
  SubmittingPatches: add convention of prefixing commit messages
  Documentation: move support for old compilers to CodingGuidelines
  Makefile: use -Wdeclaration-after-statement if supported

 Documentation/CodingGuidelines  |  8 ++++++++
 Documentation/SubmittingPatches | 21 ++++++++-------------
 Makefile                        |  7 ++++++-
 3 files changed, 22 insertions(+), 14 deletions(-)

-- 
1.7.12.1.396.g53b3ea9

[PATCH 1/3] SubmittingPatches: add convention of prefixing commit messages

[Adam Spiers]

Conscientious newcomers to git development will read SubmittingPatches
and CodingGuidelines, but could easily miss the convention of
prefixing commit messages with a single word identifying the file
or area the commit touches.

Signed-off-by: Adam Spiers <git@adamspiers.org>
---
 Documentation/SubmittingPatches | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/Documentation/SubmittingPatches b/Documentation/SubmittingPatches
index 0dbf2c9..c107cb1 100644
--- a/Documentation/SubmittingPatches
+++ b/Documentation/SubmittingPatches
@@ -9,6 +9,14 @@ Checklist (and a short version for the impatient):
 	- the first line of the commit message should be a short
 	  description (50 characters is the soft limit, see DISCUSSION
 	  in git-commit(1)), and should skip the full stop
+	- it is also conventional in most cases to prefix the
+	  first line with "area: " where the area is a filename
+	  or identifier for the general area of the code being
+	  modified, e.g.
+	  . archive: ustar header checksum is computed unsigned
+	  . git-cherry-pick.txt: clarify the use of revision range notation
+	  (if in doubt which identifier to use, run "git log --no-merges"
+	  on the files you are modifying to see the current conventions)
 	- the body should provide a meaningful commit message, which:
 	  . explains the problem the change tries to solve, iow, what
 	    is wrong with the current code without the change.
-- 
1.7.12.1.396.g53b3ea9

Re: [PATCH 1/3] SubmittingPatches: add convention of prefixing commit messages

[Junio C Hamano]

Adam Spiers <git@adamspiers.org> writes:

> Conscientious newcomers to git development will read SubmittingPatches
> and CodingGuidelines, but could easily miss the convention of
> prefixing commit messages with a single word identifying the file
> or area the commit touches.
>
> Signed-off-by: Adam Spiers <git@adamspiers.org>
> ---
>  Documentation/SubmittingPatches | 8 ++++++++
>  1 file changed, 8 insertions(+)
>
> diff --git a/Documentation/SubmittingPatches b/Documentation/SubmittingPatches
> index 0dbf2c9..c107cb1 100644
> --- a/Documentation/SubmittingPatches
> +++ b/Documentation/SubmittingPatches
> @@ -9,6 +9,14 @@ Checklist (and a short version for the impatient):
>  	- the first line of the commit message should be a short
>  	  description (50 characters is the soft limit, see DISCUSSION
>  	  in git-commit(1)), and should skip the full stop
> +	- it is also conventional in most cases to prefix the
> +	  first line with "area: " where the area is a filename
> +	  or identifier for the general area of the code being
> +	  modified, e.g.
> +	  . archive: ustar header checksum is computed unsigned
> +	  . git-cherry-pick.txt: clarify the use of revision range notation
> +	  (if in doubt which identifier to use, run "git log --no-merges"
> +	  on the files you are modifying to see the current conventions)

Thanks; I have to wonder if these details should be left in the
longer version to keep the "short" one short, though.

We should probably add "learn from good examples." (aka "read 'git
log' output and the pattern should be obvious to you") as the first
item to this list, too.

>  	- the body should provide a meaningful commit message, which:
>  	  . explains the problem the change tries to solve, iow, what
>  	    is wrong with the current code without the change.

[PATCH 2/3] Documentation: move support for old compilers to CodingGuidelines

[Adam Spiers]

The "Try to be nice to older C compilers" text is clearly a guideline
to be borne in mind whilst coding rather than when submitting patches.

Signed-off-by: Adam Spiers <git@adamspiers.org>
---
 Documentation/CodingGuidelines  |  8 ++++++++
 Documentation/SubmittingPatches | 13 -------------
 2 files changed, 8 insertions(+), 13 deletions(-)

diff --git a/Documentation/CodingGuidelines b/Documentation/CodingGuidelines
index 57da6aa..69f7e9b 100644
--- a/Documentation/CodingGuidelines
+++ b/Documentation/CodingGuidelines
@@ -112,6 +112,14 @@ For C programs:
 
  - We try to keep to at most 80 characters per line.
 
+ - We try to support a wide range of C compilers to compile git with,
+   including old ones. That means that you should not use C99
+   initializers, even if a lot of compilers grok it.
+
+ - Variables have to be declared at the beginning of the block.
+
+ - NULL pointers shall be written as NULL, not as 0.
+
  - When declaring pointers, the star sides with the variable
    name, i.e. "char *string", not "char* string" or
    "char * string".  This makes it easier to understand code
diff --git a/Documentation/SubmittingPatches b/Documentation/SubmittingPatches
index c107cb1..c34c9d1 100644
--- a/Documentation/SubmittingPatches
+++ b/Documentation/SubmittingPatches
@@ -127,19 +127,6 @@ in templates/hooks--pre-commit.  To help ensure this does not happen,
 run git diff --check on your changes before you commit.
 
 
-(1a) Try to be nice to older C compilers
-
-We try to support a wide range of C compilers to compile
-git with. That means that you should not use C99 initializers, even
-if a lot of compilers grok it.
-
-Also, variables have to be declared at the beginning of the block
-(you can check this with gcc, using the -Wdeclaration-after-statement
-option).
-
-Another thing: NULL pointers shall be written as NULL, not as 0.
-
-
 (2) Generate your patch using git tools out of your commits.
 
 git based diff tools generate unidiff which is the preferred format.
-- 
1.7.12.1.396.g53b3ea9

[PATCH 3/3] Makefile: use -Wdeclaration-after-statement if supported

[Adam Spiers]

CodingGuidelines requests that code should be nice to older C compilers.
Since modern gcc can warn on code written using newer dialects such as C99,
it makes sense to take advantage of this by auto-detecting this capability
and enabling it when found.

Signed-off-by: Adam Spiers <git@adamspiers.org>
---
If we adopt this approach, it may make sense to enable other flags
where available (e.g. -Wzero-as-null-pointer-constant, maybe even
-ansi).  In that case, something like this might be a more efficient
way of writing it:

    GCC_FLAGS=-Wdeclaration-after-statement,-Wanother-flag,-Wand-another
    GCC_FLAGS_REGEXP=$(shell echo $(GCC_FLAGS) | sed 's/,/\\|/g')
    GCC_SUPPORTED_FLAGS=$(shell cc --help -v 2>&1 | \
            sed -n '/.* \($(GCC_FLAGS_REGEXP)\) .*/{s//\1/;p}')

 Makefile | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/Makefile b/Makefile
index a49d1db..aae70d4 100644
--- a/Makefile
+++ b/Makefile
@@ -331,8 +331,13 @@ endif
 # CFLAGS and LDFLAGS are for the users to override from the command line.
 
 CFLAGS = -g -O2 -Wall
+GCC_DECL_AFTER_STATEMENT = \
+	$(shell $(CC) --help -v 2>&1 | \
+		grep -q -- -Wdeclaration-after-statement && \
+	  echo -Wdeclaration-after-statement)
+GCC_FLAGS = $(GCC_DECL_AFTER_STATEMENT)
+ALL_CFLAGS = $(CPPFLAGS) $(CFLAGS) $(GCC_FLAGS)
 LDFLAGS =
-ALL_CFLAGS = $(CPPFLAGS) $(CFLAGS)
 ALL_LDFLAGS = $(LDFLAGS)
 STRIP ?= strip
 
-- 
1.7.12.1.396.g53b3ea9

Re: [PATCH 3/3] Makefile: use -Wdeclaration-after-statement if supported

[Junio C Hamano]

Adam Spiers <git@adamspiers.org> writes:

> If we adopt this approach,...
> diff --git a/Makefile b/Makefile
> index a49d1db..aae70d4 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -331,8 +331,13 @@ endif
>  # CFLAGS and LDFLAGS are for the users to override from the command line.
>  
>  CFLAGS = -g -O2 -Wall
> +GCC_DECL_AFTER_STATEMENT = \
> +	$(shell $(CC) --help -v 2>&1 | \
> +		grep -q -- -Wdeclaration-after-statement && \
> +	  echo -Wdeclaration-after-statement)
> +GCC_FLAGS = $(GCC_DECL_AFTER_STATEMENT)
> +ALL_CFLAGS = $(CPPFLAGS) $(CFLAGS) $(GCC_FLAGS)
>  LDFLAGS =
> -ALL_CFLAGS = $(CPPFLAGS) $(CFLAGS)
>  ALL_LDFLAGS = $(LDFLAGS)


Please do not do this.

People cannot disable it from the command line, like:

    $ make V=1 CFLAGS='-g -O0 -Wall'

If anything, this should be part of the default CFLAGS.

More importantly, this will run the $(shell ...) struct once for
every *.o file we produce, I think, in addition to running it twice
for the whole build.  If you add this:

@@ -345,7 +345,8 @@ CFLAGS = -g -O2 -Wall
 GCC_DECL_AFTER_STATEMENT = \
 	$(shell $(CC) --help -v 2>&1 | \
 		grep -q -- -Wdeclaration-after-statement && \
-	  echo -Wdeclaration-after-statement)
+	  echo -Wdeclaration-after-statement; \
+	  echo >&2 GCC_DECL_AFTER_STATEMENT CRUFT)
 GCC_FLAGS = $(GCC_DECL_AFTER_STATEMENT)
 ALL_CFLAGS = $(CPPFLAGS) $(CFLAGS) $(GCC_FLAGS)
 LDFLAGS =

remove git.o and dir.o from a fully built tree, and then try to
rebuild these two files, you will get this:

    $ make V=1 git.o dir.o
    GCC_DECL_AFTER_STATEMENT CRUFT
    GCC_DECL_AFTER_STATEMENT CRUFT
    GCC_DECL_AFTER_STATEMENT CRUFT
    cc -o git.o -c -MF ./.depend/git.o.d -MMD -MP  -g -O2 -Wall \
    -Wdeclaration-after-statement -I.  -DHAVE_PATHS_H -DHAVE_DEV_TTY \
    -DXDL_FAST_HASH -DSHA1_HEADER='<openssl/sha.h>'  -DNO_STRLCPY \
    -DNO_MKSTEMPS -DSHELL_PATH='"/bin/sh"' \
    '-DGIT_HTML_PATH="share/doc/git-doc"' '-DGIT_MAN_PATH="share/man"' \
    '-DGIT_INFO_PATH="share/info"' git.c
    GCC_DECL_AFTER_STATEMENT CRUFT
    cc -o dir.o -c -MF ./.depend/dir.o.d -MMD -MP  -g -O2 -Wall \
    -Wdeclaration-after-statement -I.  -DHAVE_PATHS_H -DHAVE_DEV_TTY \
    -DXDL_FAST_HASH -DSHA1_HEADER='<openssl/sha.h>'  -DNO_STRLCPY \
    -DNO_MKSTEMPS -DSHELL_PATH='"/bin/sh"'  dir.c
    $ make V=1 git.o dir.o
    GCC_DECL_AFTER_STATEMENT CRUFT
    GCC_DECL_AFTER_STATEMENT CRUFT
    make: `dir.o' is up to date.

Re: [PATCH 3/3] Makefile: use -Wdeclaration-after-statement if supported

[Adam Spiers]

On Sun, Dec 16, 2012 at 05:52:05PM -0800, Junio C Hamano wrote:
> Adam Spiers <git@adamspiers.org> writes:
> 
> > If we adopt this approach,...
> > diff --git a/Makefile b/Makefile
> > index a49d1db..aae70d4 100644
> > --- a/Makefile
> > +++ b/Makefile
> > @@ -331,8 +331,13 @@ endif
> >  # CFLAGS and LDFLAGS are for the users to override from the command line.
> >  
> >  CFLAGS = -g -O2 -Wall
> > +GCC_DECL_AFTER_STATEMENT = \
> > +	$(shell $(CC) --help -v 2>&1 | \
> > +		grep -q -- -Wdeclaration-after-statement && \
> > +	  echo -Wdeclaration-after-statement)
> > +GCC_FLAGS = $(GCC_DECL_AFTER_STATEMENT)
> > +ALL_CFLAGS = $(CPPFLAGS) $(CFLAGS) $(GCC_FLAGS)
> >  LDFLAGS =
> > -ALL_CFLAGS = $(CPPFLAGS) $(CFLAGS)
> >  ALL_LDFLAGS = $(LDFLAGS)
> 
> Please do not do this.
> 
> People cannot disable it from the command line, like:
> 
>     $ make V=1 CFLAGS='-g -O0 -Wall'
> 
> If anything, this should be part of the default CFLAGS.
> 
> More importantly, this will run the $(shell ...) struct once for
> every *.o file we produce, I think, in addition to running it twice
> for the whole build.

[snipped]

OK; I expect these issues with the implementation are all
surmountable.  I did not necessarily expect this to be the final
implementation anyhow, as indicated by my comments below the divider
line.  However it's not clear to me what you think about the idea in
principle, and whether other compiler flags would merit inclusion.

(And also, please don't let this discussion hold up acceptance of the
two prior patches in the series.  Even though they are independent,
they are somewhat logically related so I grouped them into the same
series, although I'm not sure if that was the right thing to do.)

Re: [PATCH 3/3] Makefile: use -Wdeclaration-after-statement if supported

[Junio C Hamano]

Adam Spiers <git@adamspiers.org> writes:

> OK; I expect these issues with the implementation are all
> surmountable.  I did not necessarily expect this to be the final
> implementation anyhow, as indicated by my comments below the divider
> line.  However it's not clear to me what you think about the idea in
> principle, and whether other compiler flags would merit inclusion.

As different versions of GCC behave differently, and the same GCC
(mis)detect issues differently depending on the optimization level,
I do not know if it will be a fruitful exercise to try to come up
with one expression to come up with the set of flags to suit
everybody.  One flag I prefer to use is -Werror, but that means the
other flags must have zero false positive rate.

If you are interested, the flags I personally use with the version
of GCC I happen to have is in the Make script on the 'todo' branch.

Re: [PATCH 3/3] Makefile: use -Wdeclaration-after-statement if supported

[Adam Spiers]

On Mon, Dec 17, 2012 at 4:18 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Adam Spiers <git@adamspiers.org> writes:
>
>> OK; I expect these issues with the implementation are all
>> surmountable.  I did not necessarily expect this to be the final
>> implementation anyhow, as indicated by my comments below the divider
>> line.  However it's not clear to me what you think about the idea in
>> principle, and whether other compiler flags would merit inclusion.
>
> As different versions of GCC behave differently, and the same GCC
> (mis)detect issues differently depending on the optimization level,
> I do not know if it will be a fruitful exercise to try to come up
> with one expression to come up with the set of flags to suit
> everybody.

Fair enough, but let's not allow perfect to become the enemy of good.
Other flags aside, surely enabling -Wdeclaration-after-statement when
it is available is an improvement on the status quo, if it is done in
a way which doesn't damage the current build process?  History shows
quite a few instances of other developers falling into the same trap I
did, e.g.

  http://search.gmane.org/search.php?group=gmane.comp.version-control.git&query=decl-after-statement

So if the check was automated in the majority of cases (I guess the
majority of developers use gcc), it would mean less review work for
you and fewer re-rolls.  If you agree, I will try to rework the patch
so that it doesn't damage the build.

> One flag I prefer to use is -Werror, but that means the
> other flags must have zero false positive rate.

Personally I'm a fan of -Werror too.  How frequent are false
positives, and are any of them ever insurmountable?

> If you are interested, the flags I personally use with the version
> of GCC I happen to have is in the Make script on the 'todo' branch.

Thanks for the info.

Re: [PATCH 3/3] Makefile: use -Wdeclaration-after-statement if supported

[Junio C Hamano]

Adam Spiers <git@adamspiers.org> writes:

> Fair enough, but let's not allow perfect to become the enemy of good.

That is why I would prefer a solution without any false positive
while allowing false negatives, i.e. not force everybody to use
these flags without giving a way to turn them off.

You could perhaps sell us a solution like this:

 * Put these more strict options to CC_FLAGS_PEDANTIC (you may later
   want to come up with LD_FLAGS_PEDANTIC and friends to make other
   comands also more strict).

 * Introduce PEDANTIC variable that turns XX_FLAGS_PEDANTIC
   variables to less strict when set to 0 and more strict when set
   to 1, similar to the way the variable V makes "make V=0" and
   "make V=1" behave slightly differently.

Then we could introduce PEDANTIC=0 as default first to have people
try it out, with an expectation that later we can flip the default
to 'on' when the feature matures.

Re: [PATCH v2 00/14] new git check-ignore sub-command

[Adam Spiers]

On Thu, Sep 20, 2012 at 10:43 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Junio C Hamano <gitster@pobox.com> writes:
>> Adam Spiers <git@adamspiers.org> writes:
>>> Adam Spiers (14):
>>>   Update directory listing API doc to match code
>>>   Improve documentation and comments regarding directory traversal API
>>>   Rename cryptic 'which' variable to more consistent name
>>>   Rename path_excluded() to is_path_excluded()
>>>   Rename excluded_from_list() to is_excluded_from_list()
>>>   Rename excluded() to is_excluded()
>>>   Refactor is_excluded_from_list()
>>>   Refactor is_excluded()
>>>   Refactor is_path_excluded()
>>>   For each exclude pattern, store information about where it came from
>>>   Refactor treat_gitlinks()
>>>   Extract some useful pathspec handling code from builtin/add.c into a
>>>     library
>>>   Provide free_directory() for reclaiming dir_struct memory
>>>   Add git-check-ignore sub-command

[snipped]

> As to the "who owns x->src and when is it freed" question, it may
> make sense to give el a "filename" field (command line and other
> special cases would get whatever value you deem appropriate, like
> NULL or "<command line>"), have x->src point at that field when you
> queue many x's to the el in add_exc_from_file_to_list().  Then when
> you pop an element in the exclude_stack, you can free el->filename
> to plug a potential leak.

I have done this, but it required a change to struct dir:

Currently, when dir->exclude_stack is more than one entry deep, the
exclude_list pointed to by dir->exclude_list[EXC_DIRS] is a
concatenation of exclude elements from multiple files, so there will
be different values for src.  The same is true of EXC_FILE, which
typically mixes patterns from .git/info/exclude and core.excludesfile.
Therefore I have split each exclude_list into potentially multiple
exclude_lists, one per pattern source, whilst preserving the EXC_*
grouping and ordering.

My latest re-roll of as/check-ignore is nearly ready, and when I send
it, you will see a new patch in there covering the above change.

> Also I do not see why you need to have the strdup() in the caller of
> add_excludes_from_file_to_list().  If you need to keep it stable
> because you are copying it away in exclude or excludde_list,
> wouldn't it make more sense to do that at the beginning of the
> callee, i.e. add_excludes_from_file_to_list() function?

No, because in all other callers of add_excludes_from_file_to_list(),
the exclude source is already stable.  The re-roll will make this
clearer.

Re: [PATCH v2 00/14] new git check-ignore sub-command

[Junio C Hamano]

Adam Spiers <git@adamspiers.org> writes:

> It has been rebased on the latest master, and passed a full test run.

FYI, I applied the attached on top before queuing it in 'pu'.

Points to note:

 * We match the underline and the title of documentation header;

 * a few type mismatches (constness of full_path and treat_gitlink()
   signature) that broke compilation;

 * decl-after-stmt;

 * multi-line comment style;

Thanks.

-- >8 --
Subject: check-ignore: minimum compilation fix

---
 Documentation/git-check-ignore.txt |  2 +-
 builtin/check-ignore.c             | 11 +++++++----
 dir.c                              | 12 ++++++------
 pathspec.c                         |  4 ++--
 pathspec.h                         |  2 +-
 5 files changed, 17 insertions(+), 14 deletions(-)

diff --git a/Documentation/git-check-ignore.txt b/Documentation/git-check-ignore.txt
index 2de4e17..96fa7bc 100644
--- a/Documentation/git-check-ignore.txt
+++ b/Documentation/git-check-ignore.txt
@@ -1,5 +1,5 @@
 git-check-ignore(1)
-=================
+===================
 
 NAME
 ----
diff --git a/builtin/check-ignore.c b/builtin/check-ignore.c
index 74ea2fc..3cbd3b9 100644
--- a/builtin/check-ignore.c
+++ b/builtin/check-ignore.c
@@ -77,9 +77,10 @@ static int check_ignore(const char *prefix, const char **pathspec)
 		if (!seen)
 			seen = find_used_pathspec(pathspec);
 		for (i = 0; pathspec[i]; i++) {
+			const char *full_path;
 			path = pathspec[i];
-			char *full_path =
-				prefix_path(prefix, prefix ? strlen(prefix) : 0, path);
+			full_path = prefix_path(prefix, prefix
+						? strlen(prefix) : 0, path);
 			full_path = treat_gitlink(full_path);
 			validate_path(prefix, full_path);
 			if (!seen[i] && path[0]) {
@@ -108,6 +109,7 @@ static int check_ignore_stdin_paths(const char *prefix)
 	char **pathspec = NULL;
 	size_t nr = 0, alloc = 0;
 	int line_termination = null_term_line ? 0 : '\n';
+	int num_ignored;
 
 	strbuf_init(&buf, 0);
 	strbuf_init(&nbuf, 0);
@@ -124,7 +126,7 @@ static int check_ignore_stdin_paths(const char *prefix)
 	}
 	ALLOC_GROW(pathspec, nr + 1, alloc);
 	pathspec[nr] = NULL;
-	int num_ignored = check_ignore(prefix, (const char **)pathspec);
+	num_ignored = check_ignore(prefix, (const char **)pathspec);
 	maybe_flush_or_die(stdout, "attribute to stdout");
 	strbuf_release(&buf);
 	strbuf_release(&nbuf);
@@ -134,6 +136,8 @@ static int check_ignore_stdin_paths(const char *prefix)
 
 int cmd_check_ignore(int argc, const char **argv, const char *prefix)
 {
+	int num_ignored = 0;
+
 	git_config(git_default_config, NULL);
 
 	argc = parse_options(argc, argv, prefix, check_ignore_options,
@@ -155,7 +159,6 @@ int cmd_check_ignore(int argc, const char **argv, const char *prefix)
 			die(_("cannot have both --quiet and --verbose"));
 	}
 
-	int num_ignored = 0;
 	if (stdin_paths) {
 		num_ignored = check_ignore_stdin_paths(prefix);
 	} else {
diff --git a/dir.c b/dir.c
index d516ddf..8fc162c 100644
--- a/dir.c
+++ b/dir.c
@@ -511,14 +511,13 @@ static void prep_exclude(struct dir_struct *dir, const char *base, int baselen)
 		       stk->baselen - current);
 		strcpy(dir->basebuf + stk->baselen, dir->exclude_per_dir);
 
-		/* dir->basebuf gets reused by the traversal, but we
+		/*
+		 * dir->basebuf gets reused by the traversal, but we
 		 * need fname to remain unchanged to ensure the src
 		 * member of each struct exclude correctly back-references
 		 * its source file.
 		 */
-		char *fname = strdup(dir->basebuf);
-
-		add_excludes_from_file_to_list(fname,
+		add_excludes_from_file_to_list(strdup(dir->basebuf),
 					       dir->basebuf, stk->baselen,
 					       &stk->filebuf, el, 1);
 		dir->exclude_stack = stk;
@@ -1479,13 +1478,14 @@ void free_pathspec(struct pathspec *pathspec)
 
 void free_directory(struct dir_struct *dir)
 {
+	struct exclude_stack *stk;
 	int st;
 	for (st = EXC_CMDL; st <= EXC_FILE; st++)
 		free_excludes(&dir->exclude_list[st]);
 
-	struct exclude_stack *prev, *stk = dir->exclude_stack;
+	stk = dir->exclude_stack;
 	while (stk) {
-		prev = stk->prev;
+		struct exclude_stack *prev = stk->prev;
 		free_exclude_stack(stk);
 		stk = prev;
 	}
diff --git a/pathspec.c b/pathspec.c
index 10f6643..9525c7c 100644
--- a/pathspec.c
+++ b/pathspec.c
@@ -88,10 +88,10 @@ const char *treat_gitlink(const char *path)
 
 void treat_gitlinks(const char **pathspec)
 {
+	int i;
+
 	if (!pathspec || !*pathspec)
 		return;
-
-	int i;
 	for (i = 0; pathspec[i]; i++)
 		pathspec[i] = treat_gitlink(pathspec[i]);
 }
diff --git a/pathspec.h b/pathspec.h
index 4ed40a5..b7c053a 100644
--- a/pathspec.h
+++ b/pathspec.h
@@ -2,5 +2,5 @@ extern void validate_path(const char *prefix, const char *path);
 extern const char **validate_pathspec(const char *prefix, const char **files);
 extern char *find_used_pathspec(const char **pathspec);
 extern void fill_pathspec_matches(const char **pathspec, char *seen, int specs);
-extern const char *treat_gitlink(const char **path);
+extern const char *treat_gitlink(const char *path);
 extern void treat_gitlinks(const char **pathspec);
-- 
1.7.12.1.441.gae568b5

compiler checks

[Adam Spiers]

On Fri, Sep 21, 2012 at 12:00:55PM -0700, Junio C Hamano wrote:
> Adam Spiers <git@adamspiers.org> writes:
> 
> > It has been rebased on the latest master, and passed a full test run.
> 
> FYI, I applied the attached on top before queuing it in 'pu'.
> 
> Points to note:
> 
>  * We match the underline and the title of documentation header;
> 
>  * a few type mismatches (constness of full_path and treat_gitlink()
>    signature) that broke compilation;

Of course I will incorporate these tweaks in my re-roll, but it
worries me that my environment yielded no compilation issues even
without these tweaks.  Obviously I wouldn't have dreamed of submitting
a patch series which didn't even compile!  I'm using a modern gcc, and
I guess you probably are too?  Which would suggest to me that either
your environment is somehow set up to perform stricter type checking
than mine[1], or that there's a weird compiler-oriented bug somewhere
(e.g. in Makefile).  Or maybe I'm missing something obvious ...

[1] I'm in favour of stricter compiler checks where possible:
    http://article.gmane.org/gmane.comp.version-control.git/211607

Re: [PATCH v2 00/14] new git check-ignore sub-command

[Junio C Hamano]

Adam Spiers <git@adamspiers.org> writes:

>  t/t0007-ignores.sh                                | 587 ++++++++++++++++++++++

I needed this to make it pass the test. I didn't see anything that
is passed to expect_in via $text that would benefit from "echo -e"
but I did not look too carefully; you may have had a reason to write
"-e" there, and this patch may break for somebody else (namely, you),
in which case please explain and justify the use of "-e" better. We
need a solution that does not rely on the bash-ism.

Thanks.

-- >8 --

"echo -e" is a bash-ism that does not seem to be needed in the
context of this test (both bash and dash passes the test without
the extraneous "-e", but having "-e" breaks test run under dash).

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 t/t0007-ignores.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/t0007-ignores.sh b/t/t0007-ignores.sh
index a0f571e..7fd7e53 100755
--- a/t/t0007-ignores.sh
+++ b/t/t0007-ignores.sh
@@ -19,7 +19,7 @@ expect_in () {
 	then
 		>"$dest" # avoid newline
 	else
-		echo -e "$text" >"$dest"
+		echo "$text" >"$dest"
 	fi
 }
 
-- 
1.7.12.1.451.gb433296

Re: [PATCH 9/9] Add git-check-ignores

[Nguyen Thai Ngoc Duy]

On Sun, Sep 2, 2012 at 7:12 AM, Adam Spiers <git@adamspiers.org> wrote:
> --- a/builtin/add.c
> +++ b/builtin/add.c
> @@ -273,7 +273,7 @@ static int add_files(struct dir_struct *dir, int flags)
>                 fprintf(stderr, _(ignore_error));
>                 for (i = 0; i < dir->ignored_nr; i++)
>                         fprintf(stderr, "%s\n", dir->ignored[i]->name);
> -               fprintf(stderr, _("Use -f if you really want to add them.\n"));
> +               fprintf(stderr, _("Use -f if you really want to add them, or git check-ignore to see\nwhy they're ignored.\n"));
>                 die(_("no files added"));
>         }

String too long (> 80 chars).

> +static const char * const check_ignore_usage[] = {
> +"git check-ignore pathname...",
> +"git check-ignore --stdin [-z] < <list-of-paths>",
> +NULL
> +};
> +
> +static const struct option check_ignore_options[] = {
> +       OPT_BOOLEAN(0 , "stdin", &stdin_paths, "read file names from stdin"),
> +       OPT_BOOLEAN('z', NULL, &null_term_line,
> +               "input paths are terminated by a null character"),
> +       OPT_END()
> +};

You may want to mark help strings ("read file names from stdin" and
"input paths... null character") and check_ignore_usage[] for
translation. Just wrap those strings with N_() and you'll be fine. For
similar changes, check out nd/i18n-parseopt-help on branch 'pu'.

> +static void output_exclude(const char *path, struct exclude *exclude)
> +{
> +       char *type = exclude->to_exclude ? "excluded" : "included";
> +       char *bang = exclude->to_exclude ? "" : "!";
> +       char *dir  = (exclude->flags & EXC_FLAG_MUSTBEDIR) ? "/" : "";
> +       printf(_("%s: %s %s%s%s "), path, type, bang, exclude->pattern, dir);

These English words "excluded" and "included" make the translator me
want to translate them. But they could be the markers for scripts, so
they may not be translated. How about using non alphanumeric letters
instead?

> +static void check_ignore(const char *prefix, const char **pathspec)
> +{
> +       struct dir_struct dir;
> +       const char *path;
> +       char *seen = NULL;
> +
> +       /* read_cache() is only necessary so we can watch out for submodules. */
> +       if (read_cache() < 0)
> +               die(_("index file corrupt"));
> +
> +       memset(&dir, 0, sizeof(dir));
> +       dir.flags |= DIR_COLLECT_IGNORED;
> +       setup_standard_excludes(&dir);

You should support ignore rules from files and command line arguments
too, like ls-files. For quick testing.

> +static NORETURN void error_with_usage(const char *msg)
> +{
> +       error("%s", msg);
> +       usage_with_options(check_ignore_usage, check_ignore_options);
> +}

Interesting. We have usage_msg_opt() in parse-options.c, but it's more
verbose. Perhaps this function should be moved to parse-options.c
because it may be useful to other commands as well?
-- 
Duy

Re: [PATCH 9/9] Add git-check-ignores

[Junio C Hamano]

Nguyen Thai Ngoc Duy <pclouds@gmail.com> writes:

>> +static void output_exclude(const char *path, struct exclude *exclude)
>> +{
>> +       char *type = exclude->to_exclude ? "excluded" : "included";
>> +       char *bang = exclude->to_exclude ? "" : "!";
>> +       char *dir  = (exclude->flags & EXC_FLAG_MUSTBEDIR) ? "/" : "";
>> +       printf(_("%s: %s %s%s%s "), path, type, bang, exclude->pattern, dir);
>
> These English words "excluded" and "included" make the translator me
> want to translate them. But they could be the markers for scripts, so
> they may not be translated. How about using non alphanumeric letters
> instead?

I agree they should not be translated, but it is a disease to think
unintelligible mnemonic is a better input format for scripts than
the spelled out words.  "excluded/included" pair is just fine.

Re: [PATCH 9/9] Add git-check-ignores

[Nguyen Thai Ngoc Duy]

On Wed, Sep 5, 2012 at 12:26 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Nguyen Thai Ngoc Duy <pclouds@gmail.com> writes:
>
>>> +static void output_exclude(const char *path, struct exclude *exclude)
>>> +{
>>> +       char *type = exclude->to_exclude ? "excluded" : "included";
>>> +       char *bang = exclude->to_exclude ? "" : "!";
>>> +       char *dir  = (exclude->flags & EXC_FLAG_MUSTBEDIR) ? "/" : "";
>>> +       printf(_("%s: %s %s%s%s "), path, type, bang, exclude->pattern, dir);
>>
>> These English words "excluded" and "included" make the translator me
>> want to translate them. But they could be the markers for scripts, so
>> they may not be translated. How about using non alphanumeric letters
>> instead?
>
> I agree they should not be translated, but it is a disease to think
> unintelligible mnemonic is a better input format for scripts than
> the spelled out words.  "excluded/included" pair is just fine.

Not all mnemonic is unintelligible though. "+" and "-" may fit well in
this case. I'm just trying to make sure we have checked the mnemonic
pool before ending up with excluded/included.
-- 
Duy

Re: [PATCH 9/9] Add git-check-ignores

[Adam Spiers]

On Wed, Sep 05, 2012 at 05:25:25PM +0700, Nguyen Thai Ngoc Duy wrote:
> On Wed, Sep 5, 2012 at 12:26 AM, Junio C Hamano <gitster@pobox.com> wrote:
> > Nguyen Thai Ngoc Duy <pclouds@gmail.com> writes:
> >
> >>> +static void output_exclude(const char *path, struct exclude *exclude)
> >>> +{
> >>> +       char *type = exclude->to_exclude ? "excluded" : "included";
> >>> +       char *bang = exclude->to_exclude ? "" : "!";
> >>> +       char *dir  = (exclude->flags & EXC_FLAG_MUSTBEDIR) ? "/" : "";
> >>> +       printf(_("%s: %s %s%s%s "), path, type, bang, exclude->pattern, dir);
> >>
> >> These English words "excluded" and "included" make the translator me
> >> want to translate them. But they could be the markers for scripts, so
> >> they may not be translated. How about using non alphanumeric letters
> >> instead?
> >
> > I agree they should not be translated, but it is a disease to think
> > unintelligible mnemonic is a better input format for scripts than
> > the spelled out words.  "excluded/included" pair is just fine.
> 
> Not all mnemonic is unintelligible though. "+" and "-" may fit well in
> this case. I'm just trying to make sure we have checked the mnemonic
> pool before ending up with excluded/included.

Personally I'd be against introducing "+" and "-" when we already have
"!" and "".  Even though "+" and "-" are more intuitive, it would
create inconsistency and IMHO confusion.

I'm still unconvinced that it's worth having a separate type field in
the output when the pattern field already has a "!" prefix for
inclusions.  Does a separate field really help porcelain writers or
make the output more readable?

Re: [PATCH 9/9] Add git-check-ignores

[Adam Spiers]

On Tue, Sep 04, 2012 at 08:06:12PM +0700, Nguyen Thai Ngoc Duy wrote:
> On Sun, Sep 2, 2012 at 7:12 AM, Adam Spiers <git@adamspiers.org> wrote:
> > --- a/builtin/add.c
> > +++ b/builtin/add.c
> > @@ -273,7 +273,7 @@ static int add_files(struct dir_struct *dir, int flags)
> >                 fprintf(stderr, _(ignore_error));
> >                 for (i = 0; i < dir->ignored_nr; i++)
> >                         fprintf(stderr, "%s\n", dir->ignored[i]->name);
> > -               fprintf(stderr, _("Use -f if you really want to add them.\n"));
> > +               fprintf(stderr, _("Use -f if you really want to add them, or git check-ignore to see\nwhy they're ignored.\n"));
> >                 die(_("no files added"));
> >         }
> 
> String too long (> 80 chars).

You mean the line of code is too long, or the argument to _(), or
both?  I didn't like this either, but I saw that builtin/checkout.c
already did something similar twice, and I wasn't sure how else to do
it.  Suggestions gratefully received.

> > +static const char * const check_ignore_usage[] = {
> > +"git check-ignore pathname...",
> > +"git check-ignore --stdin [-z] < <list-of-paths>",
> > +NULL
> > +};
> > +
> > +static const struct option check_ignore_options[] = {
> > +       OPT_BOOLEAN(0 , "stdin", &stdin_paths, "read file names from stdin"),
> > +       OPT_BOOLEAN('z', NULL, &null_term_line,
> > +               "input paths are terminated by a null character"),
> > +       OPT_END()
> > +};
> 
> You may want to mark help strings ("read file names from stdin" and
> "input paths... null character") and check_ignore_usage[] for
> translation. Just wrap those strings with N_() and you'll be fine. For
> similar changes, check out nd/i18n-parseopt-help on branch 'pu'.

Thanks, I'll do that.

[snipped discussion of "include" / "exclude" which already continued elsewhere]

> > +static void check_ignore(const char *prefix, const char **pathspec)
> > +{
> > +       struct dir_struct dir;
> > +       const char *path;
> > +       char *seen = NULL;
> > +
> > +       /* read_cache() is only necessary so we can watch out for submodules. */
> > +       if (read_cache() < 0)
> > +               die(_("index file corrupt"));
> > +
> > +       memset(&dir, 0, sizeof(dir));
> > +       dir.flags |= DIR_COLLECT_IGNORED;
> > +       setup_standard_excludes(&dir);
> 
> You should support ignore rules from files and command line arguments
> too, like ls-files. For quick testing.

You mean --exclude, --exclude-from, and --exclude-per-directory?
Sure, although I have limited time right now, so maybe these could be
added in a later iteration?

> > +static NORETURN void error_with_usage(const char *msg)
> > +{
> > +       error("%s", msg);
> > +       usage_with_options(check_ignore_usage, check_ignore_options);
> > +}
> 
> Interesting. We have usage_msg_opt() in parse-options.c, but it's more
> verbose. Perhaps this function should be moved to parse-options.c
> because it may be useful to other commands as well?

I'll look into it.

Re: [PATCH 9/9] Add git-check-ignores

[Nguyen Thai Ngoc Duy]

On Mon, Sep 10, 2012 at 6:09 PM, Adam Spiers <git@adamspiers.org> wrote:
>> >                         fprintf(stderr, "%s\n", dir->ignored[i]->name);
>> > -               fprintf(stderr, _("Use -f if you really want to add them.\n"));
>> > +               fprintf(stderr, _("Use -f if you really want to add them, or git check-ignore to see\nwhy they're ignored.\n"));
>> >                 die(_("no files added"));
>> >         }
>>
>> String too long (> 80 chars).
>
> You mean the line of code is too long, or the argument to _(), or
> both?  I didn't like this either, but I saw that builtin/checkout.c
> already did something similar twice, and I wasn't sure how else to do
> it.  Suggestions gratefully received.

I don't rememeber :( I might mean the output because I missed "\n" in
the middle. At least you can split the string in to at "\n" to make it
resemble output.

>> You should support ignore rules from files and command line arguments
>> too, like ls-files. For quick testing.
>
> You mean --exclude, --exclude-from, and --exclude-per-directory?
> Sure, although I have limited time right now, so maybe these could be
> added in a later iteration?

Sure, no problem. It's not hard to add them anyway (I think).
-- 
Duy

Re: [PATCH 9/9] Add git-check-ignores

[Junio C Hamano]

Adam Spiers <git@adamspiers.org> writes:

Administrivia.  Please never deflect direct responses to you with
"Mail-Followup-To" header.  I told my mailer to "follow-up" so that I
could give you advice in response, while adding others in the
discussion to Cc so that they do not have to repeat what I said, but
your "Mail-follow-up-to" forced my advice to go to Nguyen, who does
not need one.

> On Tue, Sep 04, 2012 at 08:06:12PM +0700, Nguyen Thai Ngoc Duy wrote:
>> On Sun, Sep 2, 2012 at 7:12 AM, Adam Spiers <git@adamspiers.org> wrote:
>> > --- a/builtin/add.c
>> > +++ b/builtin/add.c
>> > @@ -273,7 +273,7 @@ static int add_files(struct dir_struct *dir, int flags)
>> >                 fprintf(stderr, _(ignore_error));
>> >                 for (i = 0; i < dir->ignored_nr; i++)
>> >                         fprintf(stderr, "%s\n", dir->ignored[i]->name);
>> > -               fprintf(stderr, _("Use -f if you really want to add them.\n"));
>> > +               fprintf(stderr, _("Use -f if you really want to add them, or git check-ignore to see\nwhy they're ignored.\n"));
>> >                 die(_("no files added"));
>> >         }
>> 
>> String too long (> 80 chars).
>
> You mean the line of code is too long, or the argument to _(), or
> both?

Both.

               fprintf(stderr, _("Use -f if you really want to add them, or\n"
				 "run git check-ignore to see\nwhy they're ignored.\n"));

But in this particular case, I tend to think the additional noise
does not add much value and something the user wouldn't want to see
over and over again (in other words, it belongs to "an advice").

Re: [PATCH 0/9] new git check-ignore sub-command

[Junio C Hamano]

Adam Spiers <git@adamspiers.org> writes:

> I was browsing stackoverflow the other day and came across this question:
>
>     http://stackoverflow.com/questions/12144633/which-gitignore-rule-is-ignoring-my-file/
>
> A quick google revealed this thread from 2009:
>
>     http://thread.gmane.org/gmane.comp.version-control.git/108671/focus=108815
>
> where Junio and Jeff discussed the possibility of adding a new `git
> check-ignore' subcommand somewhat analogous to the existing `git
> check-attr', and suggested the beginnings of an implementation.  It
> struck me that it might not be too hard to follow these ideas to their
> natural conclusion, so I decided it would make a fun project :-)

Thanks.  I wish there are more people like you ;-)

As to styles, I spotted only three kinds of "Huh?":

 * do not initialise statics to 0 or NULL, e.g.

    -static int exclude_args = 0;
    +static int exclude_args;

 * avoid unnnecessary braces {} around single statement blocks, e.g.

    -if (exclude) {
    +if (exclude)
        return exclude;
    -}

 * else should follow close brace '}' of if clause, e.g.

     if (...) {
         ...
    -}
    -else {
    +} else {
         ...

For reviews on substance, please see other messages from me.

Re: [PATCH 0/9] new git check-ignore sub-command

[Adam Spiers]

On Sun, Sep 2, 2012 at 9:35 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Adam Spiers <git@adamspiers.org> writes:
>
>> I was browsing stackoverflow the other day and came across this question:
>>
>>     http://stackoverflow.com/questions/12144633/which-gitignore-rule-is-ignoring-my-file/
>>
>> A quick google revealed this thread from 2009:
>>
>>     http://thread.gmane.org/gmane.comp.version-control.git/108671/focus=108815
>>
>> where Junio and Jeff discussed the possibility of adding a new `git
>> check-ignore' subcommand somewhat analogous to the existing `git
>> check-attr', and suggested the beginnings of an implementation.  It
>> struck me that it might not be too hard to follow these ideas to their
>> natural conclusion, so I decided it would make a fun project :-)
>
> Thanks.  I wish there are more people like you ;-)

Thanks ;-)

> As to styles, I spotted only three kinds of "Huh?":
>
>  * do not initialise statics to 0 or NULL, e.g.
>
>     -static int exclude_args = 0;
>     +static int exclude_args;
>
>  * avoid unnnecessary braces {} around single statement blocks, e.g.
>
>     -if (exclude) {
>     +if (exclude)
>         return exclude;
>     -}
>
>  * else should follow close brace '}' of if clause, e.g.
>
>      if (...) {
>          ...
>     -}
>     -else {
>     +} else {
>          ...

OK thanks, I will fix these and also submit a patch for CodingGuidelines.

Re: [PATCH 0/9] new git check-ignore sub-command

[Adam Spiers]

On Sun, Sep 2, 2012 at 9:35 PM, Junio C Hamano <gitster@pobox.com> wrote:
>  * avoid unnnecessary braces {} around single statement blocks, e.g.
>
>     -if (exclude) {
>     +if (exclude)
>         return exclude;
>     -}
>
>  * else should follow close brace '}' of if clause, e.g.
>
>      if (...) {
>          ...
>     -}
>     -else {
>     +} else {
>          ...

What about when the if clause requires braces but the else clause
doesn't?  Should it be

	if (...) {
		...;
		...;
	} else
		...;

or

	if (...) {
		...;
		...;
	}
	else
		...;

?

Re: [PATCH 0/9] new git check-ignore sub-command

[Junio C Hamano]

Adam Spiers <git@adamspiers.org> writes:

> On Sun, Sep 2, 2012 at 9:35 PM, Junio C Hamano <gitster@pobox.com> wrote:
>>  * avoid unnnecessary braces {} around single statement blocks, e.g.
>>
>>     -if (exclude) {
>>     +if (exclude)
>>         return exclude;
>>     -}
>>
>>  * else should follow close brace '}' of if clause, e.g.
>>
>>      if (...) {
>>          ...
>>     -}
>>     -else {
>>     +} else {
>>          ...
>
> What about when the if clause requires braces but the else clause
> doesn't?  Should it be
>
> 	if (...) {
> 		...;
> 		...;
> 	} else
> 		...;
>
> or
>
> 	if (...) {
> 		...;
> 		...;
> 	}
> 	else
> 		...;
>
> ?

Neither.  We try to do (but often fail ;-)

	if (...) {
        	...;
        	...;
	} else {
        	...;
	}

following the kernel style, unless there is good reason not to.

[PATCH] Document conventions on static initialization and else cuddling

[Adam Spiers]

Signed-off-by: Adam Spiers <git@adamspiers.org>
---

I have begun work on fixing existing code to adhere to these
guidelines on braces, but there are currently a lot of violations,
which means any patches to fix them would be large.  So before I spend
any more time on it, I would like to check whether such patches would
be welcomed?  And if so, should I be doing that on the master branch?

I have also added some simple rules such as `check-brace-before-else'
to the top-level Makefile which perform appropriate `grep -n' commands
to detect violations and for example easily fix them via emacs' M-x
grep.  These rules can be invoked together via a `check-style' target.
Would this also be welcomed?  If so, should the checks all be
introduced in a single commit, or each check along with the code which
was fixed with its help?

BTW I briefly tried to find an existing tool out there which could
already do the checking for us, but only found ones like uncrustify
which rewrite code rather than warning on inconsistencies.  I also saw
that the kernel's scripts/checkpatch.pl only worked with patches and
was also extremely kernel-specific in the nature of its checks.

 Documentation/CodingGuidelines | 42 +++++++++++++++++++++++++++++++++++++-----
 1 file changed, 37 insertions(+), 5 deletions(-)

diff --git a/Documentation/CodingGuidelines b/Documentation/CodingGuidelines
index 57da6aa..1a2851d 100644
--- a/Documentation/CodingGuidelines
+++ b/Documentation/CodingGuidelines
@@ -117,17 +117,49 @@ For C programs:
    "char * string".  This makes it easier to understand code
    like "char *string, c;".
 
+ - We avoid unnecessary explicit initialization of BSS-allocated vars
+   (static and globals) to zero or NULL:
+
+	static int n;
+	static char **ch;
+
+   rather than:
+
+	static int n = 0;
+	static char **ch = NULL;
+
+   These are superfluous according to ISO/IEC 9899:1999 (a.k.a. C99);
+   see item 10 in section 6.7.8 ("Initialization") of WG14 N1256 for
+   the exact text:
+
+     http://open-std.org/JTC1/SC22/WG14/www/docs/n1256.pdf
+
  - We avoid using braces unnecessarily.  I.e.
 
 	if (bla) {
 		x = 1;
 	}
 
-   is frowned upon.  A gray area is when the statement extends
-   over a few lines, and/or you have a lengthy comment atop of
-   it.  Also, like in the Linux kernel, if there is a long list
-   of "else if" statements, it can make sense to add braces to
-   single line blocks.
+   is frowned upon.  A gray area is when the statement extends over a
+   few lines, and/or you have a lengthy comment atop of it.  Also,
+   like in the Linux kernel, it can make sense to add braces to single
+   line blocks if there are already braces in another branch of the
+   same conditional, and/or there is long list of "else if"
+   statements.
+
+ - When braces are required, we cuddle "else" and "else if", so the
+   preceding closing brace appears on the same line, e.g.
+
+	if (foo) {
+		...;
+	} else if (bar) {
+		...;
+		...;
+	} else {
+		...;
+	}
+
+   following Linux kernel style, unless there is a good reason not to.
 
  - We try to avoid assignments inside if().
 
-- 
1.7.12.147.g6d168f4

Re: [PATCH] Document conventions on static initialization and else cuddling

[Junio C Hamano]

Adam Spiers <git@adamspiers.org> writes:

> Signed-off-by: Adam Spiers <git@adamspiers.org>
> ---
>
> I have begun work on fixing existing code to adhere to these
> guidelines on braces, but there are currently a lot of violations,
> which means any patches to fix them would be large.  So before I spend
> any more time on it, I would like to check whether such patches would
> be welcomed? And if so, should I be doing that on the master branch?

In general, no.

The cost/benefit ratio of carrying such a change is very high.

The guidelines are not a set of hard rules that any and all code in
violation have to be fixed right away.  I am reluctant to pile more
detailed "rules" in the existing list in the document to give it an
incorrect impression that they are hard rules that all lines should
abide by, which inevitably leads to people who end up wasting time
on creating "style fixes" patches whose only practical effect is to
collide with other topics in flight.

Especially if such a "style fix" is done as a single patch, it will
block all the other topics that happen to touch the overlapping
regions for reasons more important than "style fix".

To mitigate the risk of such unnecessary collisions, while improving
the health of the codebase, we tend to do two things:

 - review new code and make sure it does not introduce guideline
   violations.  Two sets of new code that touch overlapping area by
   definition have to collide and their conflicts resolved anyway,
   so this does not add any additional cost to the project.

 - before updating existing code, clean up the area and small
   surrounding area it touches as a preparation step of the patch
   series.  As long as "surrounding area" is kept sufficiently small
   (the preparation step may have to refrain from touching the whole
   file, depending on what other topics are still in flight), this
   again does not add any additional cost to the project.

There is a third-option that we tend try to avoid but occasionally
do take:

 - update a dormant code that nobody has touched for a long time and
   no discussion on the list (with or without patch) is likely to
   result in a new topic that might want to touch it.

By the way, I have a handful of my favorite styles that are not
listed in the document, and when I write new code myself I try to
stick to them, but I do not raise a red (or any color) flag when
seeing "violating" code in a new patch [*1*].

> I have also added some simple rules such as `check-brace-before-else'
> to the top-level Makefile which perform appropriate `grep -n' commands
> to detect violations and for example easily fix them via emacs' M-x
> grep.  These rules can be invoked together via a `check-style' target.
> Would this also be welcomed?  If so, should the checks all be
> introduced in a single commit, or each check along with the code which
> was fixed with its help?

I am not enthused, personally.

> BTW I briefly tried to find an existing tool out there which could
> already do the checking for us, but only found ones like uncrustify
> which rewrite code rather than warning on inconsistencies.  I also saw
> that the kernel's scripts/checkpatch.pl only worked with patches and
> was also extremely kernel-specific in the nature of its checks.

The checkpatch.pl script works with patches for a very good
reason. It is a tool to look for "new" problems patches
introduce.

I often run "checkpatch.pl --no-tree" on incoming patches, by the
way.

>  Documentation/CodingGuidelines | 42 +++++++++++++++++++++++++++++++++++++-----
>  1 file changed, 37 insertions(+), 5 deletions(-)
>
> diff --git a/Documentation/CodingGuidelines b/Documentation/CodingGuidelines
> index 57da6aa..1a2851d 100644
> --- a/Documentation/CodingGuidelines
> +++ b/Documentation/CodingGuidelines
> @@ -117,17 +117,49 @@ For C programs:
>     "char * string".  This makes it easier to understand code
>     like "char *string, c;".
>  
> + - We avoid unnecessary explicit initialization of BSS-allocated vars
> +   (static and globals) to zero or NULL:
> +
> +	static int n;
> +	static char **ch;
> +
> +   rather than:
> +
> +	static int n = 0;
> +	static char **ch = NULL;

As I said, I am in general not in favor of piling more rules here,
but we've seen this one in new contribution often enough that it is
a good addition.

> +
> +   These are superfluous according to ISO/IEC 9899:1999 (a.k.a. C99);
> +   see item 10 in section 6.7.8 ("Initialization") of WG14 N1256 for
> +   the exact text:
> +
> +     http://open-std.org/JTC1/SC22/WG14/www/docs/n1256.pdf

But I do not think this explanation is necessary.  "This is one of
our house rules" is a sufficient reason people, who want to blindly
obey guidelines, need to see.

>   - We avoid using braces unnecessarily.  I.e.
>  
>  	if (bla) {
>  		x = 1;
>  	}
>  
> -   is frowned upon.  A gray area is when the statement extends
> -   over a few lines, and/or you have a lengthy comment atop of
> -   it.  Also, like in the Linux kernel, if there is a long list
> -   of "else if" statements, it can make sense to add braces to
> -   single line blocks.
> +   is frowned upon.  A gray area is when the statement extends
> +   over a few lines, and/or you have a lengthy comment atop of
> +   it.  Also, like in the Linux kernel, it can make sense to add
> +   braces to single line blocks if there are already braces in
> +   another branch of the same conditional, and/or there is long
> +   list of "else if" statements.

Reflowing text without reason is frowned upon as it makes the
review unnecessary harder by hiding where things have changed.

You are suggesting to

        /* Turn this into ... */        /* ... this instead */
        if (condition)                  if (condition) {
                true;                           true;
        else {                          } else {
                otherwise;                      otherwise;
                ...                             ...
        }                               }

I do not think such an update is wrong per-se, but among different
shades of gray, the left is probably one of the most minor kind of
violations.



[Footnote]

*1* An example of them is "when 'if (...) ... else ...' chooses
between two equally plausible conditions, write it in such a way
that the body of the else clause runs longer".  That is because
it is far easier to get confused about the condition when looking at
what the body in the "else" clause is doing after a long body in the
"if" clause:

        /* BAD */                       /* GOOD */
        if (condition) {                if (!condition) {
            ....                                ....
            ....                                ....
            ....                        } else {
            ....                                ....
            ....                                ....
            ....                                ....
            ....                                ....
        } else {                                ....
            ....                                ....
            ....                                ....
        }                               }

But that can safely be done only when "condition" and "!condition"
are both equally natural expressions.

Re: [PATCH] Document conventions on static initialization and else cuddling

[Adam Spiers]

On Wed, Sep 19, 2012 at 01:43:46PM -0700, Junio C Hamano wrote:
> Adam Spiers <git@adamspiers.org> writes:
> 
> > Signed-off-by: Adam Spiers <git@adamspiers.org>
> > ---
> >
> > I have begun work on fixing existing code to adhere to these
> > guidelines on braces, but there are currently a lot of violations,
> > which means any patches to fix them would be large.  So before I spend
> > any more time on it, I would like to check whether such patches would
> > be welcomed? And if so, should I be doing that on the master branch?
> 
> In general, no.

[snipped]

> > I have also added some simple rules such as `check-brace-before-else'
> > to the top-level Makefile which perform appropriate `grep -n' commands
> > to detect violations and for example easily fix them via emacs' M-x
> > grep.  These rules can be invoked together via a `check-style' target.
> > Would this also be welcomed?  If so, should the checks all be
> > introduced in a single commit, or each check along with the code which
> > was fixed with its help?
> 
> I am not enthused, personally.

OK, I'll drop work on these then.

[snipped]

> >  Documentation/CodingGuidelines | 42 +++++++++++++++++++++++++++++++++++++-----
> >  1 file changed, 37 insertions(+), 5 deletions(-)
> >
> > diff --git a/Documentation/CodingGuidelines b/Documentation/CodingGuidelines
> > index 57da6aa..1a2851d 100644
> > --- a/Documentation/CodingGuidelines
> > +++ b/Documentation/CodingGuidelines
> > @@ -117,17 +117,49 @@ For C programs:
> >     "char * string".  This makes it easier to understand code
> >     like "char *string, c;".
> >  
> > + - We avoid unnecessary explicit initialization of BSS-allocated vars
> > +   (static and globals) to zero or NULL:
> > +
> > +	static int n;
> > +	static char **ch;
> > +
> > +   rather than:
> > +
> > +	static int n = 0;
> > +	static char **ch = NULL;
> 
> As I said, I am in general not in favor of piling more rules here,
> but we've seen this one in new contribution often enough that it is
> a good addition.

Yes, I am proposing its addition precisely because its current
omission resulted in a small but non-negligible amount of both my time
and yours being wasted in a recent patch review cycle.

> > +
> > +   These are superfluous according to ISO/IEC 9899:1999 (a.k.a. C99);
> > +   see item 10 in section 6.7.8 ("Initialization") of WG14 N1256 for
> > +   the exact text:
> > +
> > +     http://open-std.org/JTC1/SC22/WG14/www/docs/n1256.pdf
> 
> But I do not think this explanation is necessary.  "This is one of
> our house rules" is a sufficient reason people, who want to blindly
> obey guidelines, need to see.

I presume I'm not the only person who wishes to obey guidelines but
also understand why they exist.  In general, a deeper understanding of
the purpose behind guidelines helps improve judgement about when and
how to apply them.

> >   - We avoid using braces unnecessarily.  I.e.
> >  
> >  	if (bla) {
> >  		x = 1;
> >  	}
> >  
> > -   is frowned upon.  A gray area is when the statement extends
> > -   over a few lines, and/or you have a lengthy comment atop of
> > -   it.  Also, like in the Linux kernel, if there is a long list
> > -   of "else if" statements, it can make sense to add braces to
> > -   single line blocks.
> > +   is frowned upon.  A gray area is when the statement extends
> > +   over a few lines, and/or you have a lengthy comment atop of
> > +   it.  Also, like in the Linux kernel, it can make sense to add
> > +   braces to single line blocks if there are already braces in
> > +   another branch of the same conditional, and/or there is long
> > +   list of "else if" statements.
> 
> Reflowing text without reason is frowned upon as it makes the
> review unnecessary harder by hiding where things have changed.

There was a reason, it just wasn't a particularly good one :-) (emacs
reflows by paragraph, so you have to go out of your way to stop it
reflowing existing text.)

BTW, for anyone else reading who is confused by the fact that the
quoted hunk above shows no signs of reflowing, I think the hunk itself
has been re-reflowed back to the original width (presumably
deliberately in order to determine what really changed).

> You are suggesting to
> 
>         /* Turn this into ... */        /* ... this instead */
>         if (condition)                  if (condition) {
>                 true;                           true;
>         else {                          } else {
>                 otherwise;                      otherwise;
>                 ...                             ...
>         }                               }
> 
> I do not think such an update is wrong per-se, but among different
> shades of gray, the left is probably one of the most minor kind of
> violations.

I was under the impression it was _your_ suggestion ;-)

  http://thread.gmane.org/gmane.comp.version-control.git/204661/focus=204975

Perhaps I read too much into that, since it was concerning the mirror
image case (i.e. where the 'else' branch is a single statement and the
'if' branch requires braces), but they seem like stylistically
equivalent debates to me.

Thanks for the review, Junio.  Based on my responses above, what would
you like me to do with the patch (if anything)?