{"thread":{"id":"28898","subject":"[PATCH 1/1] apply.c: reject patch without --(ex,in)clude and path outside.","startedAt":"2011-11-09T22:49:02Z","lastAt":"2011-11-09T23:07:15Z","messageCount":2,"participants":["Bruce E. Robertson","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":1},"messages":[{"id":"179225","messageId":"1320878942-9811-1-git-send-email-bruce.e.robertson@intel.com","threadId":"28898","inReplyTo":null,"subject":"[PATCH 1/1] apply.c: reject patch without --(ex,in)clude and path outside.","fromName":"Bruce E. Robertson","fromEmail":"bruce.e.robertson@intel.com","sentAt":"2011-11-09T22:49:02Z","receivedAt":"2011-11-09T22:49:02Z","isPatch":true,"sender":{"key":"bruce.e.robertson@intel.com","avatar":null},"body":"From: \"Bruce E. Robertson\" <bruce.e.robertson@intel.com>\n\nPatches are silently ignored when applied with neither --include nor\n--exclude options when the current working dir is not on patch's\npath. This contravenes the principle of least surprise.\n\n\"make test\" results for this change:\nfixed   0\nsuccess 8032\nfailed  0\nbroken  58\ntotal   8126\n\nSigned-off-by: Bruce E. Robertson <bruce.e.robertson@intel.com>\n---\n builtin/apply.c |   12 +++++++++---\n 1 files changed, 9 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/apply.c b/builtin/apply.c\nindex 84a8a0b..162e2aa 100644\n--- a/builtin/apply.c\n+++ b/builtin/apply.c\n@@ -3619,6 +3619,7 @@ static struct lock_file lock_file;\n \n static struct string_list limit_by_name;\n static int has_include;\n+static int has_exclude;\n static void add_name_limit(const char *name, int exclude)\n {\n \tstruct string_list_item *it;\n@@ -3717,9 +3718,13 @@ static int apply_patch(int fd, const char *filename, int options)\n \t\t\tlistp = &patch->next;\n \t\t}\n \t\telse {\n-\t\t\t/* perhaps free it a bit better? */\n-\t\t\tfree(patch);\n-\t\t\tskipped_patch++;\n+\t\t\tif ( !has_exclude && !has_include ) {\n+\t\t\t\tpatch->rejected = 1;\n+\t\t\t} else {\n+\t\t\t\t/* perhaps free it a bit better? */\n+\t\t\t\tfree(patch);\n+\t\t\t\tskipped_patch++;\n+\t\t\t}\n \t\t}\n \t\toffset += nr;\n \t}\n@@ -3773,6 +3778,7 @@ static int option_parse_exclude(const struct option *opt,\n \t\t\t\tconst char *arg, int unset)\n {\n \tadd_name_limit(arg, 1);\n+\thas_exclude = 1;\n \treturn 0;\n }\n \n-- \n1.7.7.1.432.gca458.dirty\n"},{"id":"179226","messageId":"7vipmsuc7w.fsf@alter.siamese.dyndns.org","threadId":"28898","inReplyTo":"1320878942-9811-1-git-send-email-bruce.e.robertson@intel.com","subject":"Re: [PATCH 1/1] apply.c: reject patch without --(ex,in)clude and path outside.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-11-09T23:07:15Z","receivedAt":"2011-11-09T23:07:15Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Bruce E. Robertson\" <bruce.e.robertson@intel.com> writes:\n\n> From: \"Bruce E. Robertson\" <bruce.e.robertson@intel.com>\n>\n> Patches are silently ignored when applied with neither --include nor\n> --exclude options when the current working dir is not on patch's\n> path. This contravenes the principle of least surprise.\n\nI do not necessarily agree but if you think so perhaps the user should be\ntold about which paths are rejected. In other words, I think it is wrong\nto change the exit code to 1 when you are applying a patch that touches\noutside your area, but I think it is not wrong if the program warned about\nit.\n\nDoes your patch behave like that?\n\n> diff --git a/builtin/apply.c b/builtin/apply.c\n> index 84a8a0b..162e2aa 100644\n> --- a/builtin/apply.c\n> +++ b/builtin/apply.c\n> @@ -3619,6 +3619,7 @@ static struct lock_file lock_file;\n>  \n>  static struct string_list limit_by_name;\n>  static int has_include;\n> +static int has_exclude;\n>  static void add_name_limit(const char *name, int exclude)\n>  {\n>  \tstruct string_list_item *it;\n> @@ -3717,9 +3718,13 @@ static int apply_patch(int fd, const char *filename, int options)\n>  \t\t\tlistp = &patch->next;\n>  \t\t}\n>  \t\telse {\n> -\t\t\t/* perhaps free it a bit better? */\n> -\t\t\tfree(patch);\n> -\t\t\tskipped_patch++;\n> +\t\t\tif ( !has_exclude && !has_include ) {\n\nStyle; extra SP inside ().\n\nI am not convinced that the logic here is correct, either.  If you have\nexclude but not include, and the patch records a path outside your area,\nthe path will be rejected even if it does not match any of the exclude\npatterns. Shouldn't you be treating that case exactly the same as the case\nwithout any exclude patterns?\n\n> +\t\t\t\tpatch->rejected = 1;\n\nDoesn't it trigger \"errs\" to be set in write_out_results()?  This patch is\nnot free()'ed, but it is not on the \"list\" either. Where does it go, and\nhow is that unfreed patch used later in the program?\n\n> +\t\t\t} else {\n> +\t\t\t\t/* perhaps free it a bit better? */\n> +\t\t\t\tfree(patch);\n> +\t\t\t\tskipped_patch++;\n> +\t\t\t}\n>  \t\t}\n>  \t\toffset += nr;\n>  \t}\n> @@ -3773,6 +3778,7 @@ static int option_parse_exclude(const struct option *opt,\n>  \t\t\t\tconst char *arg, int unset)\n>  {\n>  \tadd_name_limit(arg, 1);\n> +\thas_exclude = 1;\n>  \treturn 0;\n>  }\n"}]}