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

2012-12-26 Thread Adam Spiers
On Thu, Sep 20, 2012 at 10:43 PM, Junio C Hamano gits...@pobox.com wrote:
 Junio C Hamano gits...@pobox.com writes:
 Adam Spiers g...@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.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


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

2012-09-24 Thread Junio C Hamano
Adam Spiers g...@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 gits...@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

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


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

2012-09-21 Thread Junio C Hamano
Adam Spiers g...@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 

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

2012-09-20 Thread 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

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


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

2012-09-20 Thread Junio C Hamano
Adam Spiers g...@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.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


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

2012-09-20 Thread Junio C Hamano
Junio C Hamano gits...@pobox.com writes:

 Adam Spiers g...@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?
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


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

2012-09-20 Thread Adam Spiers
On Thu, Sep 20, 2012 at 10:43 PM, Junio C Hamano gits...@pobox.com wrote:
 Junio C Hamano gits...@pobox.com writes:
 Adam Spiers g...@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 21 | 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.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


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

2012-09-20 Thread Junio C Hamano
Adam Spiers g...@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).
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html