git-apply - Add --include=PATH

PATCH In review Joe Perches 2 participants 4 messages Jun 15, 2016 Documentation

lore

Share & tools

lore

Joe Perches

author

2016-06-15 22:45 UTC #0 lore raw

PATCH 1/1 git-apply - Add --include=PATH

Add similar capability to --exclude=

Allows selection of files to patch from a
large patchset.

Signed-off-by Joe Perches <[email protected]>

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

Documentation/git-apply.txt +7 -1

index feb51f1..2467e62 100644
@@ -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

builtin-apply.c +25 -2

index 2216a0b..121a6d0 100644
@@ -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;

Junio C Hamano

2016-06-15 22:45 UTC #1 lore raw

Quoted 6 lines · Joe Perches

Add similar capability to --exclude=
Allows selection of files to patch from a
large patchset.
Signed-off-by Joe Perches <[email protected]>

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

builtin-apply.c static struct excludes { · line 3002

Quoted 8 lines

builtin-apply.c +4 -0

@@ -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.

Joe Perches

author

2016-06-15 22:45 UTC #2 lore raw

Quoted 11 lines · Junio C Hamano

Quoted 3 lines · Joe Perches
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.
Quoted 4 lines
builtin-apply.c +0 -0
@@ -2996,10 +2996,16 @@ static struct excludes {
 	const char *path;
 } *excludes;
 

Quoted 8 lines

builtin-apply.c static struct excludes { · line 3002
Quoted 4 lines
builtin-apply.c +4 -0
+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.

Quoted 4 lines

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

Junio C Hamano

2016-06-15 22:45 UTC #3 lore raw

Quoted 13 lines · Joe Perches

Quoted 4 lines
Quoted 4 lines
(quoted diff) +0 -0
@@ -2996,10 +2996,16 @@ static struct excludes {
 	const char *path;
 } *excludes;
 
Quoted 8 lines
Review static struct excludes { · line 3002
Quoted 4 lines
(quoted diff) +4 -0
+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.

Quoted 2 lines

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(-)

builtin-apply.c +33 ~~-15~~

index 2216a0b..967ebec 100644
@@ -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")) {