Merge branch 'jc/apply-reject-noop-hunk'
authorJunio C Hamano <gitster@pobox.com>
Wed, 24 Jun 2015 19:21:39 +0000 (12:21 -0700)
committerJunio C Hamano <gitster@pobox.com>
Wed, 24 Jun 2015 19:21:39 +0000 (12:21 -0700)
"git apply" cannot diagnose a patch corruption when the breakage is
to mark the length of the hunk shorter than it really is on the
hunk header line "@@ -l,k +m,n @@"; one special case it could is
when the hunk becomes no-op (e.g. k == n == 2 for two-line context
patch output), and it learned how to do so.

* jc/apply-reject-noop-hunk:
apply: reject a hunk that does not do anything

1  2 
builtin/apply.c
diff --combined builtin/apply.c
index 146be97a1a879242aa12eb066c5c69cd6af92409,606eddd5f6dcd6b3982f9acc6eb6f93eee235649..54aba4e351257f8bc4b46d3f507c1ed9525ec7fc
@@@ -51,12 -51,11 +51,12 @@@ static int apply_verbosely
  static int allow_overlap;
  static int no_add;
  static int threeway;
 +static int unsafe_paths;
  static const char *fake_ancestor;
  static int line_termination = '\n';
  static unsigned int p_context = UINT_MAX;
  static const char * const apply_usage[] = {
 -      N_("git apply [options] [<patch>...]"),
 +      N_("git apply [<options>] [<patch>...]"),
        NULL
  };
  
@@@ -208,7 -207,7 +208,7 @@@ struct patch 
        struct patch *next;
  
        /* three-way fallback result */
 -      unsigned char threeway_stage[3][20];
 +      struct object_id threeway_stage[3];
  };
  
  static void free_fragment_list(struct fragment *list)
@@@ -658,6 -657,11 +658,6 @@@ static size_t diff_timestamp_len(const 
        return line + len - end;
  }
  
 -static char *null_strdup(const char *s)
 -{
 -      return s ? xstrdup(s) : NULL;
 -}
 -
  static char *find_name_common(const char *line, const char *def,
                              int p_value, const char *end, int terminate)
  {
                        start = line;
        }
        if (!start)
 -              return squash_slash(null_strdup(def));
 +              return squash_slash(xstrdup_or_null(def));
        len = line - start;
        if (!len)
 -              return squash_slash(null_strdup(def));
 +              return squash_slash(xstrdup_or_null(def));
  
        /*
         * Generally we prefer the shorter name, especially
@@@ -905,7 -909,7 +905,7 @@@ static void parse_traditional_patch(con
                        patch->old_name = name;
                } else {
                        patch->old_name = name;
 -                      patch->new_name = null_strdup(name);
 +                      patch->new_name = xstrdup_or_null(name);
                }
        }
        if (!name)
@@@ -994,7 -998,7 +994,7 @@@ static int gitdiff_delete(const char *l
  {
        patch->is_delete = 1;
        free(patch->old_name);
 -      patch->old_name = null_strdup(patch->def_name);
 +      patch->old_name = xstrdup_or_null(patch->def_name);
        return gitdiff_oldmode(line, patch);
  }
  
@@@ -1002,7 -1006,7 +1002,7 @@@ static int gitdiff_newfile(const char *
  {
        patch->is_new = 1;
        free(patch->new_name);
 -      patch->new_name = null_strdup(patch->def_name);
 +      patch->new_name = xstrdup_or_null(patch->def_name);
        return gitdiff_newmode(line, patch);
  }
  
@@@ -1601,9 -1605,6 +1601,9 @@@ static int parse_fragment(const char *l
                        if (!deleted && !added)
                                leading++;
                        trailing++;
 +                      if (!apply_in_reverse &&
 +                          ws_error_action == correct_ws_error)
 +                              check_whitespace(line, len, patch->ws_rule);
                        break;
                case '-':
                        if (apply_in_reverse &&
        }
        if (oldlines || newlines)
                return -1;
+       if (!deleted && !added)
+               return -1;
        fragment->leading = leading;
        fragment->trailing = trailing;
  
@@@ -2234,12 -2238,6 +2237,12 @@@ static void update_pre_post_images(stru
                ctx++;
        }
  
 +      if (postlen
 +          ? postlen < new - postimage->buf
 +          : postimage->len < new - postimage->buf)
 +              die("BUG: caller miscounted postlen: asked %d, orig = %d, used = %d",
 +                  (int)postlen, (int) postimage->len, (int)(new - postimage->buf));
 +
        /* Fix the length of the whole thing */
        postimage->len = new - postimage->buf;
        postimage->nr -= reduced;
@@@ -2395,27 -2393,10 +2398,27 @@@ static int match_fragment(struct image 
  
        /*
         * The hunk does not apply byte-by-byte, but the hash says
 -       * it might with whitespace fuzz. We haven't been asked to
 +       * it might with whitespace fuzz. We weren't asked to
         * ignore whitespace, we were asked to correct whitespace
         * errors, so let's try matching after whitespace correction.
         *
 +       * While checking the preimage against the target, whitespace
 +       * errors in both fixed, we count how large the corresponding
 +       * postimage needs to be.  The postimage prepared by
 +       * apply_one_fragment() has whitespace errors fixed on added
 +       * lines already, but the common lines were propagated as-is,
 +       * which may become longer when their whitespace errors are
 +       * fixed.
 +       */
 +
 +      /* First count added lines in postimage */
 +      postlen = 0;
 +      for (i = 0; i < postimage->nr; i++) {
 +              if (!(postimage->line[i].flag & LINE_COMMON))
 +                      postlen += postimage->line[i].len;
 +      }
 +
 +      /*
         * The preimage may extend beyond the end of the file,
         * but in this loop we will only handle the part of the
         * preimage that falls within the file.
        strbuf_init(&fixed, preimage->len + 1);
        orig = preimage->buf;
        target = img->buf + try;
 -      postlen = 0;
        for (i = 0; i < preimage_limit; i++) {
                size_t oldlen = preimage->line[i].len;
                size_t tgtlen = img->line[try_lno + i].len;
                match = (tgtfix.len == fixed.len - fixstart &&
                         !memcmp(tgtfix.buf, fixed.buf + fixstart,
                                             fixed.len - fixstart));
 -              postlen += tgtfix.len;
 +
 +              /* Add the length if this is common with the postimage */
 +              if (preimage->line[i].flag & LINE_COMMON)
 +                      postlen += tgtfix.len;
  
                strbuf_release(&tgtfix);
                if (!match)
@@@ -2776,8 -2755,7 +2779,8 @@@ static int apply_one_fragment(struct im
                default:
                        if (apply_verbosely)
                                error(_("invalid start of line: '%c'"), first);
 -                      return -1;
 +                      applied_pos = -1;
 +                      goto out;
                }
                if (added_blank_line) {
                        if (!new_blank_lines_at_end)
                              (int)(old - oldlines), oldlines);
        }
  
 +out:
        free(oldlines);
        strbuf_release(&newlines);
        free(preimage.line_allocated);
@@@ -3227,7 -3204,7 +3230,7 @@@ static int load_patch_target(struct str
                             const char *name,
                             unsigned expected_mode)
  {
 -      if (cached) {
 +      if (cached || check_index) {
                if (read_file_or_gitlink(ce, buf))
                        return error(_("read of %s failed"), name);
        } else if (name) {
                                return read_file_or_gitlink(ce, buf);
                        else
                                return SUBMODULE_PATCH_WITHOUT_INDEX;
 +              } else if (has_symlink_leading_path(name, strlen(name))) {
 +                      return error(_("reading from '%s' beyond a symbolic link"), name);
                } else {
                        if (read_old_data(st, name, buf))
                                return error(_("read of %s failed"), name);
@@@ -3426,11 -3401,11 +3429,11 @@@ static int try_threeway(struct image *i
        if (status) {
                patch->conflicted_threeway = 1;
                if (patch->is_new)
 -                      hashclr(patch->threeway_stage[0]);
 +                      oidclr(&patch->threeway_stage[0]);
                else
 -                      hashcpy(patch->threeway_stage[0], pre_sha1);
 -              hashcpy(patch->threeway_stage[1], our_sha1);
 -              hashcpy(patch->threeway_stage[2], post_sha1);
 +                      hashcpy(patch->threeway_stage[0].hash, pre_sha1);
 +              hashcpy(patch->threeway_stage[1].hash, our_sha1);
 +              hashcpy(patch->threeway_stage[2].hash, post_sha1);
                fprintf(stderr, "Applied patch to '%s' with conflicts.\n", patch->new_name);
        } else {
                fprintf(stderr, "Applied patch to '%s' cleanly.\n", patch->new_name);
@@@ -3577,121 -3552,6 +3580,121 @@@ static int check_to_create(const char *
        return 0;
  }
  
 +/*
 + * We need to keep track of how symlinks in the preimage are
 + * manipulated by the patches.  A patch to add a/b/c where a/b
 + * is a symlink should not be allowed to affect the directory
 + * the symlink points at, but if the same patch removes a/b,
 + * it is perfectly fine, as the patch removes a/b to make room
 + * to create a directory a/b so that a/b/c can be created.
 + */
 +static struct string_list symlink_changes;
 +#define SYMLINK_GOES_AWAY 01
 +#define SYMLINK_IN_RESULT 02
 +
 +static uintptr_t register_symlink_changes(const char *path, uintptr_t what)
 +{
 +      struct string_list_item *ent;
 +
 +      ent = string_list_lookup(&symlink_changes, path);
 +      if (!ent) {
 +              ent = string_list_insert(&symlink_changes, path);
 +              ent->util = (void *)0;
 +      }
 +      ent->util = (void *)(what | ((uintptr_t)ent->util));
 +      return (uintptr_t)ent->util;
 +}
 +
 +static uintptr_t check_symlink_changes(const char *path)
 +{
 +      struct string_list_item *ent;
 +
 +      ent = string_list_lookup(&symlink_changes, path);
 +      if (!ent)
 +              return 0;
 +      return (uintptr_t)ent->util;
 +}
 +
 +static void prepare_symlink_changes(struct patch *patch)
 +{
 +      for ( ; patch; patch = patch->next) {
 +              if ((patch->old_name && S_ISLNK(patch->old_mode)) &&
 +                  (patch->is_rename || patch->is_delete))
 +                      /* the symlink at patch->old_name is removed */
 +                      register_symlink_changes(patch->old_name, SYMLINK_GOES_AWAY);
 +
 +              if (patch->new_name && S_ISLNK(patch->new_mode))
 +                      /* the symlink at patch->new_name is created or remains */
 +                      register_symlink_changes(patch->new_name, SYMLINK_IN_RESULT);
 +      }
 +}
 +
 +static int path_is_beyond_symlink_1(struct strbuf *name)
 +{
 +      do {
 +              unsigned int change;
 +
 +              while (--name->len && name->buf[name->len] != '/')
 +                      ; /* scan backwards */
 +              if (!name->len)
 +                      break;
 +              name->buf[name->len] = '\0';
 +              change = check_symlink_changes(name->buf);
 +              if (change & SYMLINK_IN_RESULT)
 +                      return 1;
 +              if (change & SYMLINK_GOES_AWAY)
 +                      /*
 +                       * This cannot be "return 0", because we may
 +                       * see a new one created at a higher level.
 +                       */
 +                      continue;
 +
 +              /* otherwise, check the preimage */
 +              if (check_index) {
 +                      struct cache_entry *ce;
 +
 +                      ce = cache_file_exists(name->buf, name->len, ignore_case);
 +                      if (ce && S_ISLNK(ce->ce_mode))
 +                              return 1;
 +              } else {
 +                      struct stat st;
 +                      if (!lstat(name->buf, &st) && S_ISLNK(st.st_mode))
 +                              return 1;
 +              }
 +      } while (1);
 +      return 0;
 +}
 +
 +static int path_is_beyond_symlink(const char *name_)
 +{
 +      int ret;
 +      struct strbuf name = STRBUF_INIT;
 +
 +      assert(*name_ != '\0');
 +      strbuf_addstr(&name, name_);
 +      ret = path_is_beyond_symlink_1(&name);
 +      strbuf_release(&name);
 +
 +      return ret;
 +}
 +
 +static void die_on_unsafe_path(struct patch *patch)
 +{
 +      const char *old_name = NULL;
 +      const char *new_name = NULL;
 +      if (patch->is_delete)
 +              old_name = patch->old_name;
 +      else if (!patch->is_new && !patch->is_copy)
 +              old_name = patch->old_name;
 +      if (!patch->is_delete)
 +              new_name = patch->new_name;
 +
 +      if (old_name && !verify_path(old_name))
 +              die(_("invalid path '%s'"), old_name);
 +      if (new_name && !verify_path(new_name))
 +              die(_("invalid path '%s'"), new_name);
 +}
 +
  /*
   * Check and apply the patch in-core; leave the result in patch->result
   * for the caller to write it out to the final destination.
@@@ -3779,22 -3639,6 +3782,22 @@@ static int check_patch(struct patch *pa
                }
        }
  
 +      if (!unsafe_paths)
 +              die_on_unsafe_path(patch);
 +
 +      /*
 +       * An attempt to read from or delete a path that is beyond a
 +       * symbolic link will be prevented by load_patch_target() that
 +       * is called at the beginning of apply_data() so we do not
 +       * have to worry about a patch marked with "is_delete" bit
 +       * here.  We however need to make sure that the patch result
 +       * is not deposited to a path that is beyond a symbolic link
 +       * here.
 +       */
 +      if (!patch->is_delete && path_is_beyond_symlink(patch->new_name))
 +              return error(_("affected file '%s' is beyond a symbolic link"),
 +                           patch->new_name);
 +
        if (apply_data(patch, &st, ce) < 0)
                return error(_("%s: patch does not apply"), name);
        patch->rejected = 0;
@@@ -3805,7 -3649,6 +3808,7 @@@ static int check_patch_list(struct patc
  {
        int err = 0;
  
 +      prepare_symlink_changes(patch);
        prepare_fn_table(patch);
        while (patch) {
                if (apply_verbosely)
@@@ -3888,7 -3731,7 +3891,7 @@@ static void build_fake_ancestor(struct 
                        if (!preimage_sha1_in_gitlink_patch(patch, sha1))
                                ; /* ok, the textual part looks sane */
                        else
 -                              die("sha1 information is lacking or useless for submoule %s",
 +                              die("sha1 information is lacking or useless for submodule %s",
                                    name);
                } else if (!get_sha1_blob(patch->old_sha1_prefix, sha1)) {
                        ; /* ok */
@@@ -4186,14 -4029,14 +4189,14 @@@ static void add_conflicted_stages_file(
  
        remove_file_from_cache(patch->new_name);
        for (stage = 1; stage < 4; stage++) {
 -              if (is_null_sha1(patch->threeway_stage[stage - 1]))
 +              if (is_null_oid(&patch->threeway_stage[stage - 1]))
                        continue;
                ce = xcalloc(1, ce_size);
                memcpy(ce->name, patch->new_name, namelen);
                ce->ce_mode = create_ce_mode(mode);
                ce->ce_flags = create_ce_flags(stage);
                ce->ce_namelen = namelen;
 -              hashcpy(ce->sha1, patch->threeway_stage[stage - 1]);
 +              hashcpy(ce->sha1, patch->threeway_stage[stage - 1].hash);
                if (add_cache_entry(ce, ADD_CACHE_OK_TO_ADD) < 0)
                        die(_("unable to add cache entry for %s"), patch->new_name);
        }
@@@ -4340,7 -4183,7 +4343,7 @@@ static int write_out_results(struct pat
        if (cpath.nr) {
                struct string_list_item *item;
  
 -              sort_string_list(&cpath);
 +              string_list_sort(&cpath);
                for_each_string_list_item(item, &cpath)
                        fprintf(stderr, "U %s\n", item->string);
                string_list_clear(&cpath, 0);
@@@ -4544,8 -4387,6 +4547,8 @@@ int cmd_apply(int argc, const char **ar
                        N_("make sure the patch is applicable to the current index")),
                OPT_BOOL(0, "cached", &cached,
                        N_("apply a patch without touching the working tree")),
 +              OPT_BOOL(0, "unsafe-paths", &unsafe_paths,
 +                      N_("accept a patch that touches outside the working area")),
                OPT_BOOL(0, "apply", &force_apply,
                        N_("also apply the patch (use with --stat/--summary/--check)")),
                OPT_BOOL('3', "3way", &threeway,
                        die(_("--cached outside a repository"));
                check_index = 1;
        }
 +      if (check_index)
 +              unsafe_paths = 0;
 +
        for (i = 0; i < argc; i++) {
                const char *arg = argv[i];
                int fd;