[PATCH 1/1] apply.c: reject patch without --(ex,in)clude and path outside.

Bruce E. Robertson· Nov 9, 2011, 22:49 UTC · lore

From: "Bruce E. Robertson" <bruce.e.robertson@intel.com>

Patches are silently ignored when applied with neither --include nor --exclude options when the current working dir is not on patch's path. This contravenes the principle of least surprise.

"make test" results for this change: fixed 0 success 8032 failed 0 broken 58 total 8126

Signed-off-by: Bruce E. Robertson <bruce.e.robertson@intel.com>
---
 builtin/apply.c |   12 +++++++++---
 1 files changed, 9 insertions(+), 3 deletions(-)

Show changes to builtin/apply.c +9 −3

diff --git a/builtin/apply.c b/builtin/apply.c
index 84a8a0b..162e2aa 100644
--- a/builtin/apply.c
+++ b/builtin/apply.c
@@ -3619,6 +3619,7 @@ static struct lock_file lock_file;
 
 static struct string_list limit_by_name;
 static int has_include;
+static int has_exclude;
 static void add_name_limit(const char *name, int exclude)
 {
 	struct string_list_item *it;
@@ -3717,9 +3718,13 @@ static int apply_patch(int fd, const char *filename, int options)
 			listp = &patch->next;
 		}
 		else {
-			/* perhaps free it a bit better? */
-			free(patch);
-			skipped_patch++;
+			if ( !has_exclude && !has_include ) {
+				patch->rejected = 1;
+			} else {
+				/* perhaps free it a bit better? */
+				free(patch);
+				skipped_patch++;
+			}
 		}
 		offset += nr;
 	}
@@ -3773,6 +3778,7 @@ static int option_parse_exclude(const struct option *opt,
 				const char *arg, int unset)
 {
 	add_name_limit(arg, 1);
+	has_exclude = 1;
 	return 0;
 }

-- 
1.7.7.1.432.gca458.dirty

Junio C Hamano· Nov 9, 2011, 23:07 UTC · re: Bruce E. Robertson · lore

Re: [PATCH 1/1] apply.c: reject patch without --(ex,in)clude and path outside.

"Bruce E. Robertson" <bruce.e.robertson@intel.com> writes:

Show 5 quoted lines

> From: "Bruce E. Robertson" <bruce.e.robertson@intel.com>
>
> Patches are silently ignored when applied with neither --include nor
> --exclude options when the current working dir is not on patch's
> path. This contravenes the principle of least surprise.

I do not necessarily agree but if you think so perhaps the user should be told about which paths are rejected. In other words, I think it is wrong to change the exit code to 1 when you are applying a patch that touches outside your area, but I think it is not wrong if the program warned about it.

Does your patch behave like that?

Show 20 quoted lines

> diff --git a/builtin/apply.c b/builtin/apply.c
> index 84a8a0b..162e2aa 100644
> --- a/builtin/apply.c
> +++ b/builtin/apply.c
> @@ -3619,6 +3619,7 @@ static struct lock_file lock_file;
>  
>  static struct string_list limit_by_name;
>  static int has_include;
> +static int has_exclude;
>  static void add_name_limit(const char *name, int exclude)
>  {
>  	struct string_list_item *it;
> @@ -3717,9 +3718,13 @@ static int apply_patch(int fd, const char *filename, int options)
>  			listp = &patch->next;
>  		}
>  		else {
> -			/* perhaps free it a bit better? */
> -			free(patch);
> -			skipped_patch++;
> +			if ( !has_exclude && !has_include ) {

Style; extra SP inside ().

I am not convinced that the logic here is correct, either. If you have exclude but not include, and the patch records a path outside your area, the path will be rejected even if it does not match any of the exclude patterns. Shouldn't you be treating that case exactly the same as the case without any exclude patterns?

> +				patch->rejected = 1;

Doesn't it trigger "errs" to be set in write_out_results()? This patch is not free()'ed, but it is not on the "list" either. Where does it go, and how is that unfreed patch used later in the program?

Show 15 quoted lines

> +			} else {
> +				/* perhaps free it a bit better? */
> +				free(patch);
> +				skipped_patch++;
> +			}
>  		}
>  		offset += nr;
>  	}
> @@ -3773,6 +3778,7 @@ static int option_parse_exclude(const struct option *opt,
>  				const char *arg, int unset)
>  {
>  	add_name_limit(arg, 1);
> +	has_exclude = 1;
>  	return 0;
>  }

patchapply.c: reject patch without --(ex,in)clude and path outside.