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

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

[Bruce E. Robertson]

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

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

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

[Junio C Hamano]

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

> 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?

> 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?

> +			} 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;
>  }