[PATCH] git-apply - Add --include=PATH

[PATCH] git-apply - Add --include=PATH

[Joe Perches]

Add similar capability to --exclude=

Allows selection of files to patch from a
large patchset.

Signed-off-by: Joe Perches <joe@perches.com>

 Documentation/git-apply.txt |    8 +++++++-
 builtin-apply.c             |   27 +++++++++++++++++++++++++--
 2 files changed, 32 insertions(+), 3 deletions(-)

diff --git a/Documentation/git-apply.txt b/Documentation/git-apply.txt
index feb51f1..2467e62 100644
--- a/Documentation/git-apply.txt
+++ b/Documentation/git-apply.txt
@@ -14,7 +14,8 @@ SYNOPSIS
 	  [--allow-binary-replacement | --binary] [--reject] [-z]
 	  [-pNUM] [-CNUM] [--inaccurate-eof] [--recount] [--cached]
 	  [--whitespace=<nowarn|warn|fix|error|error-all>]
-	  [--exclude=PATH] [--directory=<root>] [--verbose] [<patch>...]
+	  [--exclude=PATH] [--include=PATH] [--directory=<root>]
+	  [--verbose] [<patch>...]
 
 DESCRIPTION
 -----------
@@ -137,6 +138,11 @@ discouraged.
 	be useful when importing patchsets, where you want to exclude certain
 	files or directories.
 
+--include=<path-pattern>::
+	Apply changes to files matching the given path pattern. This can
+	be useful when importing patchsets, where you want to include certain
+	files or directories.
+
 --whitespace=<action>::
 	When applying a patch, detect a new or modified line that has
 	whitespace errors.  What are considered whitespace errors is
diff --git a/builtin-apply.c b/builtin-apply.c
index 2216a0b..121a6d0 100644
--- a/builtin-apply.c
+++ b/builtin-apply.c
@@ -46,7 +46,7 @@ static const char *fake_ancestor;
 static int line_termination = '\n';
 static unsigned long p_context = ULONG_MAX;
 static const char apply_usage[] =
-"git apply [--stat] [--numstat] [--summary] [--check] [--index] [--cached] [--apply] [--no-add] [--index-info] [--allow-binary-replacement] [--reverse] [--reject] [--verbose] [-z] [-pNUM] [-CNUM] [--whitespace=<nowarn|warn|fix|error|error-all>] <patch>...";
+"git apply [--stat] [--numstat] [--summary] [--check] [--index] [--cached] [--apply] [--no-add] [--index-info] [--allow-binary-replacement] [--reverse] [--reject] [--verbose] [-z] [-pNUM] [-CNUM] [--whitespace=<nowarn|warn|fix|error|error-all>] [--exclude=PATH] [--include=PATH] <patch>...";
 
 static enum ws_error_action {
 	nowarn_ws_error,
@@ -2996,10 +2996,16 @@ static struct excludes {
 	const char *path;
 } *excludes;
 
+static struct includes {
+	struct includes *next;
+	const char *path;
+} *includes;
+
 static int use_patch(struct patch *p)
 {
 	const char *pathname = p->new_name ? p->new_name : p->old_name;
 	struct excludes *x = excludes;
+	struct includes *y = includes;
 	while (x) {
 		if (fnmatch(x->path, pathname, 0) == 0)
 			return 0;
@@ -3011,7 +3017,17 @@ static int use_patch(struct patch *p)
 		    memcmp(prefix, pathname, prefix_length))
 			return 0;
 	}
-	return 1;
+
+	if (!y || !y->path)
+		return 1;
+
+	while (y && y->path) {
+		if (fnmatch(y->path, pathname, 0) == 0)
+			return 1;
+		y = y->next;
+	}
+
+	return 0;
 }
 
 static void prefix_one(char **name)
@@ -3160,6 +3176,13 @@ int cmd_apply(int argc, const char **argv, const char *unused_prefix)
 			excludes = x;
 			continue;
 		}
+		if (!prefixcmp(arg, "--include=")) {
+			struct includes *y = xmalloc(sizeof(*y));
+			y->path = arg + 10;
+			y->next = includes;
+			includes = y;
+			continue;
+		}
 		if (!prefixcmp(arg, "-p")) {
 			p_value = atoi(arg + 2);
 			p_value_known = 1;

Re: [PATCH] git-apply - Add --include=PATH

[Junio C Hamano]

Joe Perches <joe@perches.com> writes:

> Add similar capability to --exclude=
>
> Allows selection of files to patch from a
> large patchset.
>
> Signed-off-by: Joe Perches <joe@perches.com>

Thanks; I don't see anything fundamentally wrong with what this patch
tries to achieve.

> @@ -2996,10 +2996,16 @@ static struct excludes {
>  	const char *path;
>  } *excludes;
>  
> +static struct includes {
> +	struct includes *next;
> +	const char *path;
> +} *includes;

Now this is ugly.  You can just add a new variable "*includes" that is of
exactly the same type as existing "*excludes" without introducing a new
type.

You should then find it disturbing that the shared type is still called
"struct excludes" even though it is now used for things you would want to
include.  You are right.  You can then either rename it to a more neutral
name, or (even better) use an existing type, such as "string_list".

Which would mean that this patch should be done as two patches:

 [1/2] builtin-apply.c: Use "string_list" for "--excludes".
 [2/2] builtin-apply.c: Add "--includes" option.

The first will be a preparatory step that does not change any externally
visible behaviour.  The only thing it will do is to change the type of
"excludes" to "struct string_list", to update the option parser to use
string_list_append() to add to it, and to update the way "use_patch()" to
iterate over the items in the exclude list.

The second will add a new "includes" variable, and do the moral equivalent
of what your patch did.

The first patch most likely should introduce a new helper function:

  int string_list_has_match(struct string_list *s, const char *path);

so that the update to "use_patch()" that needs to be done in the second
patch can just pass a different string_list to implement the additional
check.

Re: [PATCH] git-apply - Add --include=PATH

[Joe Perches]

On Sat, 2008-08-23 at 17:54 -0700, Junio C Hamano wrote:
> Joe Perches <joe@perches.com> writes:
> > Add similar capability to --exclude=
> > Allows selection of files to patch from a
> > large patchset.
> Thanks; I don't see anything fundamentally wrong with what this patch
> tries to achieve.
> 
> > @@ -2996,10 +2996,16 @@ static struct excludes {
> >  	const char *path;
> >  } *excludes;
> >  
> > +static struct includes {
> > +	struct includes *next;
> > +	const char *path;
> > +} *includes;
> 
> Now this is ugly.  You can just add a new variable "*includes" that is of
> exactly the same type as existing "*excludes" without introducing a new
> type.

Yes, it's slightly ugly, but it was less work and much easier for
a human to parse.  I also didn't want to use "struct excludes"
for includes which I thought even uglier.

> You should then find it disturbing that the shared type is still called
> "struct excludes" even though it is now used for things you would want to
> include.  You are right.  You can then either rename it to a more neutral
> name, or (even better) use an existing type, such as "string_list".

I'm on holiday for a few days, but I'll submit 2 patches later:

1. Rename struct excludes to struct path_list
2. Add --includes

cheers, Joe

Re: [PATCH] git-apply - Add --include=PATH

[Junio C Hamano]

Joe Perches <joe@perches.com> writes:

>> > @@ -2996,10 +2996,16 @@ static struct excludes {
>> >  	const char *path;
>> >  } *excludes;
>> >  
>> > +static struct includes {
>> > +	struct includes *next;
>> > +	const char *path;
>> > +} *includes;
>> 
>> Now this is ugly.  You can just add a new variable "*includes" that is of
>> exactly the same type as existing "*excludes" without introducing a new
>> type.
>
> Yes, it's slightly ugly, but it was less work and much easier for
> a human to parse.

Another consideration is what should happen when you give contradicting
excludes and includes list.  For example, it is very plausible you might
want to say "apply to all but header files, except that you want the part
to one specific header file to also get applied).  Something like:

    $ git apply --include='specific-one.h' --exclude='*.h' --include='*' <patch

It is easy to declare that all the exclude patterns are processed and used
to reject paths, and then only after that include patterns, if any, are
used to limit the remainder.  But that is describing how the code does it,
and may not match what the users expect.  For example, the users would
expect:

    $ git apply --include='specific-one.h' --exclude='s*' <patch

to apply the part for "specific-one.h" but no other paths that begin with "s".
However, that is not what happens.

It would be much easier to explain to the end users if the rule were that
include and exclude patterns are examined in the order they are specified
on the command line, and the first match determines the each path's fate.

In order to support that, you do not want two separate lists.  Instead,
you would want to keep a single "static struct string_list limit_by_name",
append both excluded and included items to the list as you encounter with
string_list_append(), but in such a way that you can distinguish which one
is which later.  Also remember if you have seen any included item.

Then your use_patch() would:

 * first check and ignore patches about paths outside of the prefix if any
   is specified via --directory;

 * loop over the limit_by_name.items[] array, checking the path with each
   element in it with fnmatch().  If you find a match, then you know if it
   is excluded (return 0) or included (return 1);

 * if no patterns match, return 1 (i.e. modify this path) if you did not
   see any "include" pattern.  If you had any "include" pattern on the
   command line, return 0 (i.e. do not modify this path).

Perhaps something like this, but I did not test it.

 builtin-apply.c |   48 +++++++++++++++++++++++++++++++++---------------
 1 files changed, 33 insertions(+), 15 deletions(-)

diff --git c/builtin-apply.c w/builtin-apply.c
index 2216a0b..967ebec 100644
--- c/builtin-apply.c
+++ w/builtin-apply.c
@@ -2991,29 +2991,45 @@ static int write_out_results(struct patch *list, int skipped_patch)
 
 static struct lock_file lock_file;
 
-static struct excludes {
-	struct excludes *next;
-	const char *path;
-} *excludes;
+static struct string_list limit_by_name;
+static int has_include;
+static void add_name_limit(const char *name, int exclude)
+{
+	struct string_list_item *it;
+
+	it = string_list_append(name, &limit_by_name);
+	it->util = exclude ? NULL : (void *) 1;
+}
 
 static int use_patch(struct patch *p)
 {
 	const char *pathname = p->new_name ? p->new_name : p->old_name;
-	struct excludes *x = excludes;
-	while (x) {
-		if (fnmatch(x->path, pathname, 0) == 0)
-			return 0;
-		x = x->next;
-	}
+	int i;
+
+	/* Paths outside are not touched regardless of "--include" */
 	if (0 < prefix_length) {
 		int pathlen = strlen(pathname);
 		if (pathlen <= prefix_length ||
 		    memcmp(prefix, pathname, prefix_length))
 			return 0;
 	}
-	return 1;
+
+	/* See if it matches any of exclude/include rule */
+	for (i = 0; i < limit_by_name.nr; i++) {
+		struct string_list_item *it = &limit_by_name.items[i];
+		if (!fnmatch(it->string, pathname, 0))
+			return (it->util != NULL);
+	}
+
+	/*
+	 * If we had any include, a path that does not match any rule is
+	 * not used.  Otherwise, we saw bunch of exclude rules (or none)
+	 * and such a path is used.
+	 */
+	return !has_include;
 }
 
+
 static void prefix_one(char **name)
 {
 	char *old_name = *name;
@@ -3154,10 +3170,12 @@ int cmd_apply(int argc, const char **argv, const char *unused_prefix)
 			continue;
 		}
 		if (!prefixcmp(arg, "--exclude=")) {
-			struct excludes *x = xmalloc(sizeof(*x));
-			x->path = arg + 10;
-			x->next = excludes;
-			excludes = x;
+			add_name_limit(arg + 10, 1);
+			continue;
+		}
+		if (!prefixcmp(arg, "--include=")) {
+			add_name_limit(arg + 10, 0);
+			has_include = 1;
 			continue;
 		}
 		if (!prefixcmp(arg, "-p")) {