]> git.ipfire.org Git - thirdparty/git.git/commitdiff
Merge branch 'js/ort-clean-up-after-failed-merge' into maint
authorJunio C Hamano <gitster@pobox.com>
Fri, 26 Aug 2022 18:13:11 +0000 (11:13 -0700)
committerJunio C Hamano <gitster@pobox.com>
Fri, 26 Aug 2022 18:13:11 +0000 (11:13 -0700)
Plug memory leaks in the failure code path in the "merge-ort" merge
strategy backend.
source: <pull.1307.v2.git.1659114727.gitgitgadget@gmail.com>

* js/ort-clean-up-after-failed-merge:
  merge-ort: do leave trace2 region even if checkout fails
  merge-ort: clean up after failed merge

1  2 
merge-ort.c

diff --combined merge-ort.c
index 8c4927f0e1f745cbdb0f436676991fbe08ea4da7,07f54e409fac2e63f730ae6e8635e48f702a680e..ebb9a75425137eaf38bb5647802744e32cadbf16
@@@ -634,57 -634,17 +634,57 @@@ static void path_msg(struct merge_optio
                     const char *fmt, ...)
  {
        va_list ap;
 -      struct strbuf *sb = strmap_get(&opt->priv->output, path);
 +      struct strbuf *sb, *dest;
 +      struct strbuf tmp = STRBUF_INIT;
 +
 +      if (opt->record_conflict_msgs_as_headers && omittable_hint)
 +              return; /* Do not record mere hints in headers */
 +      if (opt->priv->call_depth && opt->verbosity < 5)
 +              return; /* Ignore messages from inner merges */
 +
 +      sb = strmap_get(&opt->priv->output, path);
        if (!sb) {
                sb = xmalloc(sizeof(*sb));
                strbuf_init(sb, 0);
                strmap_put(&opt->priv->output, path, sb);
        }
  
 +      dest = (opt->record_conflict_msgs_as_headers ? &tmp : sb);
 +
        va_start(ap, fmt);
 -      strbuf_vaddf(sb, fmt, ap);
 +      if (opt->priv->call_depth) {
 +              strbuf_addchars(dest, ' ', 2);
 +              strbuf_addstr(dest, "From inner merge:");
 +              strbuf_addchars(dest, ' ', opt->priv->call_depth * 2);
 +      }
 +      strbuf_vaddf(dest, fmt, ap);
        va_end(ap);
  
 +      if (opt->record_conflict_msgs_as_headers) {
 +              int i_sb = 0, i_tmp = 0;
 +
 +              /* Start with the specified prefix */
 +              if (opt->msg_header_prefix)
 +                      strbuf_addf(sb, "%s ", opt->msg_header_prefix);
 +
 +              /* Copy tmp to sb, adding spaces after newlines */
 +              strbuf_grow(sb, sb->len + 2*tmp.len); /* more than sufficient */
 +              for (; i_tmp < tmp.len; i_tmp++, i_sb++) {
 +                      /* Copy next character from tmp to sb */
 +                      sb->buf[sb->len + i_sb] = tmp.buf[i_tmp];
 +
 +                      /* If we copied a newline, add a space */
 +                      if (tmp.buf[i_tmp] == '\n')
 +                              sb->buf[++i_sb] = ' ';
 +              }
 +              /* Update length and ensure it's NUL-terminated */
 +              sb->len += i_sb;
 +              sb->buf[sb->len] = '\0';
 +
 +              strbuf_release(&tmp);
 +      }
 +
 +      /* Add final newline character to sb */
        strbuf_addch(sb, '\n');
  }
  
@@@ -728,15 -688,13 +728,15 @@@ static void add_flattened_path(struct s
                        out->buf[i] = '_';
  }
  
 -static char *unique_path(struct strmap *existing_paths,
 +static char *unique_path(struct merge_options *opt,
                         const char *path,
                         const char *branch)
  {
 +      char *ret = NULL;
        struct strbuf newpath = STRBUF_INIT;
        int suffix = 0;
        size_t base_len;
 +      struct strmap *existing_paths = &opt->priv->paths;
  
        strbuf_addf(&newpath, "%s~", path);
        add_flattened_path(&newpath, branch);
                strbuf_addf(&newpath, "_%d", suffix++);
        }
  
 -      return strbuf_detach(&newpath, NULL);
 +      /* Track the new path in our memory pool */
 +      ret = mem_pool_alloc(&opt->priv->pool, newpath.len + 1);
 +      memcpy(ret, newpath.buf, newpath.len + 1);
 +      strbuf_release(&newpath);
 +      return ret;
  }
  
  /*** Function Grouping: functions related to collect_merge_info() ***/
@@@ -1594,7 -1548,6 +1594,7 @@@ static int find_first_merges(struct rep
        }
  
        object_array_clear(&merges);
 +      release_revisions(&revs);
        return result->nr;
  }
  
@@@ -1790,7 -1743,7 +1790,7 @@@ static int merge_3way(struct merge_opti
        mmfile_t orig, src1, src2;
        struct ll_merge_options ll_opts = {0};
        char *base, *name1, *name2;
 -      int merge_status;
 +      enum ll_merge_result merge_status;
  
        if (!opt->priv->attr_index.initialized)
                initialize_attr_index(opt);
        merge_status = ll_merge(result_buf, path, &orig, base,
                                &src1, name1, &src2, name2,
                                &opt->priv->attr_index, &ll_opts);
 +      if (merge_status == LL_MERGE_BINARY_CONFLICT)
 +              path_msg(opt, path, 0,
 +                       "warning: Cannot merge binary files: %s (%s vs. %s)",
 +                       path, name1, name2);
  
        free(base);
        free(name1);
@@@ -1939,7 -1888,7 +1939,7 @@@ static int handle_content_merge(struct 
  
                if (!ret &&
                    write_object_file(result_buf.ptr, result_buf.size,
 -                                    blob_type, &result->oid))
 +                                    OBJ_BLOB, &result->oid))
                        ret = err(opt, _("Unable to add %s to database"),
                                  path);
  
@@@ -2069,7 -2018,7 +2069,7 @@@ static char *handle_path_level_conflict
         * to ensure that's the case.
         */
        c_info = strmap_get(collisions, new_path);
 -      if (c_info == NULL)
 +      if (!c_info)
                BUG("c_info is NULL");
  
        /*
@@@ -2260,27 -2209,6 +2260,27 @@@ static void compute_collisions(struct s
        }
  }
  
 +static void free_collisions(struct strmap *collisions)
 +{
 +      struct hashmap_iter iter;
 +      struct strmap_entry *entry;
 +
 +      /* Free each value in the collisions map */
 +      strmap_for_each_entry(collisions, &iter, entry) {
 +              struct collision_info *info = entry->value;
 +              string_list_clear(&info->source_files, 0);
 +      }
 +      /*
 +       * In compute_collisions(), we set collisions.strdup_strings to 0
 +       * so that we wouldn't have to make another copy of the new_path
 +       * allocated by apply_dir_rename().  But now that we've used them
 +       * and have no other references to these strings, it is time to
 +       * deallocate them.
 +       */
 +      free_strmap_strings(collisions);
 +      strmap_clear(collisions, 1);
 +}
 +
  static char *check_for_directory_rename(struct merge_options *opt,
                                        const char *path,
                                        unsigned side_index,
                                        struct strmap *collisions,
                                        int *clean_merge)
  {
 -      char *new_path = NULL;
 +      char *new_path;
        struct strmap_entry *rename_info;
 -      struct strmap_entry *otherinfo = NULL;
 +      struct strmap_entry *otherinfo;
        const char *new_dir;
 +      int other_side = 3 - side_index;
  
 +      /*
 +       * Cases where we don't have or don't want a directory rename for
 +       * this path.
 +       */
        if (strmap_empty(dir_renames))
 -              return new_path;
 +              return NULL;
 +      if (strmap_get(&collisions[other_side], path))
 +              return NULL;
        rename_info = check_dir_renamed(path, dir_renames);
        if (!rename_info)
 -              return new_path;
 -      /* old_dir = rename_info->key; */
 -      new_dir = rename_info->value;
 +              return NULL;
  
        /*
         * This next part is a little weird.  We do not want to do an
         * As it turns out, this also prevents N-way transient rename
         * confusion; See testcases 9c and 9d of t6043.
         */
 +      new_dir = rename_info->value; /* old_dir = rename_info->key; */
        otherinfo = strmap_get_entry(dir_rename_exclusions, new_dir);
        if (otherinfo) {
                path_msg(opt, rename_info->key, 1,
        }
  
        new_path = handle_path_level_conflicts(opt, path, side_index,
 -                                             rename_info, collisions);
 +                                             rename_info,
 +                                             &collisions[side_index]);
        *clean_merge &= (new_path != NULL);
  
        return new_path;
@@@ -2495,7 -2416,7 +2495,7 @@@ static void apply_directory_rename_modi
                 */
                ci->path_conflict = 1;
                if (pair->status == 'A')
 -                      path_msg(opt, new_path, 0,
 +                      path_msg(opt, new_path, 1,
                                 _("CONFLICT (file location): %s added in %s "
                                   "inside a directory that was renamed in %s, "
                                   "suggesting it should perhaps be moved to "
                                 old_path, branch_with_new_path,
                                 branch_with_dir_rename, new_path);
                else
 -                      path_msg(opt, new_path, 0,
 +                      path_msg(opt, new_path, 1,
                                 _("CONFLICT (file location): %s renamed to %s "
                                   "in %s, inside a directory that was renamed "
                                   "in %s, suggesting it should perhaps be "
@@@ -3052,15 -2973,18 +3052,15 @@@ static int detect_regular_renames(struc
  static int collect_renames(struct merge_options *opt,
                           struct diff_queue_struct *result,
                           unsigned side_index,
 +                         struct strmap *collisions,
                           struct strmap *dir_renames_for_side,
                           struct strmap *rename_exclusions)
  {
        int i, clean = 1;
 -      struct strmap collisions;
        struct diff_queue_struct *side_pairs;
 -      struct hashmap_iter iter;
 -      struct strmap_entry *entry;
        struct rename_info *renames = &opt->priv->renames;
  
        side_pairs = &renames->pairs[side_index];
 -      compute_collisions(&collisions, dir_renames_for_side, side_pairs);
  
        for (i = 0; i < side_pairs->nr; ++i) {
                struct diff_filepair *p = side_pairs->queue[i];
                                                      side_index,
                                                      dir_renames_for_side,
                                                      rename_exclusions,
 -                                                    &collisions,
 +                                                    collisions,
                                                      &clean);
  
                possibly_cache_new_pair(renames, p, side_index, new_path);
                result->queue[result->nr++] = p;
        }
  
 -      /* Free each value in the collisions map */
 -      strmap_for_each_entry(&collisions, &iter, entry) {
 -              struct collision_info *info = entry->value;
 -              string_list_clear(&info->source_files, 0);
 -      }
 -      /*
 -       * In compute_collisions(), we set collisions.strdup_strings to 0
 -       * so that we wouldn't have to make another copy of the new_path
 -       * allocated by apply_dir_rename().  But now that we've used them
 -       * and have no other references to these strings, it is time to
 -       * deallocate them.
 -       */
 -      free_strmap_strings(&collisions);
 -      strmap_clear(&collisions, 1);
        return clean;
  }
  
@@@ -3110,22 -3048,18 +3110,22 @@@ static int detect_and_process_renames(s
                                      struct tree *side1,
                                      struct tree *side2)
  {
 -      struct diff_queue_struct combined;
 +      struct diff_queue_struct combined = { 0 };
        struct rename_info *renames = &opt->priv->renames;
 -      int need_dir_renames, s, clean = 1;
 +      struct strmap collisions[3];
 +      int need_dir_renames, s, i, clean = 1;
        unsigned detection_run = 0;
  
 -      memset(&combined, 0, sizeof(combined));
        if (!possible_renames(renames))
                goto cleanup;
  
        trace2_region_enter("merge", "regular renames", opt->repo);
        detection_run |= detect_regular_renames(opt, MERGE_SIDE1);
        detection_run |= detect_regular_renames(opt, MERGE_SIDE2);
 +      if (renames->needed_limit) {
 +              renames->cached_pairs_valid_side = 0;
 +              renames->redo_after_renames = 0;
 +      }
        if (renames->redo_after_renames && detection_run) {
                int i, side;
                struct diff_filepair *p;
        ALLOC_GROW(combined.queue,
                   renames->pairs[1].nr + renames->pairs[2].nr,
                   combined.alloc);
 +      for (i = MERGE_SIDE1; i <= MERGE_SIDE2; i++) {
 +              int other_side = 3 - i;
 +              compute_collisions(&collisions[i],
 +                                 &renames->dir_renames[other_side],
 +                                 &renames->pairs[i]);
 +      }
        clean &= collect_renames(opt, &combined, MERGE_SIDE1,
 +                               collisions,
                                 &renames->dir_renames[2],
                                 &renames->dir_renames[1]);
        clean &= collect_renames(opt, &combined, MERGE_SIDE2,
 +                               collisions,
                                 &renames->dir_renames[1],
                                 &renames->dir_renames[2]);
 +      for (i = MERGE_SIDE1; i <= MERGE_SIDE2; i++)
 +              free_collisions(&collisions[i]);
        STABLE_QSORT(combined.queue, combined.nr, compare_pairs);
        trace2_region_leave("merge", "directory renames", opt->repo);
  
@@@ -3209,9 -3133,13 +3209,9 @@@ simple_cleanup
                free(renames->pairs[s].queue);
                DIFF_QUEUE_CLEAR(&renames->pairs[s]);
        }
 -      if (combined.nr) {
 -              int i;
 -              for (i = 0; i < combined.nr; i++)
 -                      pool_diff_free_filepair(&opt->priv->pool,
 -                                              combined.queue[i]);
 -              free(combined.queue);
 -      }
 +      for (i = 0; i < combined.nr; i++)
 +              pool_diff_free_filepair(&opt->priv->pool, combined.queue[i]);
 +      free(combined.queue);
  
        return clean;
  }
@@@ -3415,7 -3343,7 +3415,7 @@@ static void write_tree(struct object_i
        }
  
        /* Write this object file out, and record in result_oid */
 -      write_object_file(buf.buf, buf.len, tree_type, result_oid);
 +      write_object_file(buf.buf, buf.len, OBJ_TREE, result_oid);
        strbuf_release(&buf);
  }
  
@@@ -3709,7 -3637,7 +3709,7 @@@ static void process_entry(struct merge_
                 */
                df_file_index = (ci->dirmask & (1 << 1)) ? 2 : 1;
                branch = (df_file_index == 1) ? opt->branch1 : opt->branch2;
 -              path = unique_path(&opt->priv->paths, path, branch);
 +              path = unique_path(opt, path, branch);
                strmap_put(&opt->priv->paths, path, new_ci);
  
                path_msg(opt, path, 0,
                        /* Insert entries into opt->priv_paths */
                        assert(rename_a || rename_b);
                        if (rename_a) {
 -                              a_path = unique_path(&opt->priv->paths,
 -                                                   path, opt->branch1);
 +                              a_path = unique_path(opt, path, opt->branch1);
                                strmap_put(&opt->priv->paths, a_path, ci);
                        }
  
                        if (rename_b)
 -                              b_path = unique_path(&opt->priv->paths,
 -                                                   path, opt->branch2);
 +                              b_path = unique_path(opt, path, opt->branch2);
                        else
                                b_path = path;
                        strmap_put(&opt->priv->paths, b_path, new_ci);
@@@ -4087,8 -4017,8 +4087,8 @@@ static void process_entries(struct merg
        trace2_region_enter("merge", "process_entries cleanup", opt->repo);
        if (dir_metadata.offsets.nr != 1 ||
            (uintptr_t)dir_metadata.offsets.items[0].util != 0) {
 -              printf("dir_metadata.offsets.nr = %d (should be 1)\n",
 -                     dir_metadata.offsets.nr);
 +              printf("dir_metadata.offsets.nr = %"PRIuMAX" (should be 1)\n",
 +                     (uintmax_t)dir_metadata.offsets.nr);
                printf("dir_metadata.offsets.items[0].util = %u (should be 0)\n",
                       (unsigned)(uintptr_t)dir_metadata.offsets.items[0].util);
                fflush(stdout);
@@@ -4227,7 -4157,7 +4227,7 @@@ static int record_conflicted_index_entr
                                struct stat st;
  
                                if (!lstat(path, &st)) {
 -                                      char *new_name = unique_path(&opt->priv->paths,
 +                                      char *new_name = unique_path(opt,
                                                                     path,
                                                                     "cruft");
  
                                                 _("Note: %s not up to date and in way of checking out conflicted version; old copy renamed to %s"),
                                                 path, new_name);
                                        errs |= rename(path, new_name);
 -                                      free(new_name);
                                }
                                errs |= checkout_entry(ce, &state, NULL, NULL);
                        }
@@@ -4294,6 -4225,8 +4294,8 @@@ void merge_switch_to_result(struct merg
                if (checkout(opt, head, result->tree)) {
                        /* failure to function */
                        result->clean = -1;
+                       merge_finalize(opt, result);
+                       trace2_region_leave("merge", "checkout", opt->repo);
                        return;
                }
                trace2_region_leave("merge", "checkout", opt->repo);
                        /* failure to function */
                        opt->priv = NULL;
                        result->clean = -1;
+                       merge_finalize(opt, result);
+                       trace2_region_leave("merge", "record_conflicted",
+                                           opt->repo);
                        return;
                }
                opt->priv = NULL;
                struct string_list olist = STRING_LIST_INIT_NODUP;
                int i;
  
 +              if (opt->record_conflict_msgs_as_headers)
 +                      BUG("Either display conflict messages or record them as headers, not both");
 +
                trace2_region_enter("merge", "display messages", opt->repo);
  
                /* Hack to pre-allocate olist to the desired size */
@@@ -4428,9 -4361,6 +4433,9 @@@ static void merge_start(struct merge_op
        assert(opt->recursive_variant >= MERGE_VARIANT_NORMAL &&
               opt->recursive_variant <= MERGE_VARIANT_THEIRS);
  
 +      if (opt->msg_header_prefix)
 +              assert(opt->record_conflict_msgs_as_headers);
 +
        /*
         * detect_renames, verbosity, buffer_output, and obuf are ignored
         * fields that were used by "recursive" rather than "ort" -- but
@@@ -4631,7 -4561,6 +4636,7 @@@ redo
        trace2_region_leave("merge", "process_entries", opt->repo);
  
        /* Set return values */
 +      result->path_messages = &opt->priv->output;
        result->tree = parse_tree_indirect(&working_tree_oid);
        /* existence of conflicted entries implies unclean */
        result->clean &= strmap_empty(&opt->priv->conflicted);
@@@ -4651,7 -4580,7 +4656,7 @@@ static void merge_ort_internal(struct m
                               struct commit *h2,
                               struct merge_result *result)
  {
 -      struct commit_list *iter;
 +      struct commit *next;
        struct commit *merged_merge_bases;
        const char *ancestor_name;
        struct strbuf merge_base_abbrev = STRBUF_INIT;
        }
  
        merged_merge_bases = pop_commit(&merge_bases);
 -      if (merged_merge_bases == NULL) {
 +      if (!merged_merge_bases) {
                /* if there is no common ancestor, use an empty tree */
                struct tree *tree;
  
                ancestor_name = merge_base_abbrev.buf;
        }
  
 -      for (iter = merge_bases; iter; iter = iter->next) {
 +      for (next = pop_commit(&merge_bases); next;
 +           next = pop_commit(&merge_bases)) {
                const char *saved_b1, *saved_b2;
                struct commit *prev = merged_merge_bases;
  
                saved_b2 = opt->branch2;
                opt->branch1 = "Temporary merge branch 1";
                opt->branch2 = "Temporary merge branch 2";
 -              merge_ort_internal(opt, NULL, prev, iter->item, result);
 +              merge_ort_internal(opt, NULL, prev, next, result);
                if (result->clean < 0)
                        return;
                opt->branch1 = saved_b1;
                                                         result->tree,
                                                         "merged tree");
                commit_list_insert(prev, &merged_merge_bases->parents);
 -              commit_list_insert(iter->item,
 -                                 &merged_merge_bases->parents->next);
 +              commit_list_insert(next, &merged_merge_bases->parents->next);
  
                clear_or_reinit_internal_opts(opt->priv, 1);
        }