Re: [PATCH v2 1/6] remote: fix tear down of struct branch and struct remote

[Jacob Keller <jacob.e.keller@intel.com>]

On 6/17/2025 3:25 PM, Junio C Hamano wrote:
> Jacob Keller <jacob.e.keller@intel.com> writes:
> 
>> From: Jacob Keller <jacob.keller@gmail.com>
> 
> where we iterate over branch->merge[] and branch->merge_name[]
> for branch->nr times and each time we check the NULL-ness of these
> two pointers.
> 
> merge_nr is only incremented inside add_merge() when merge_name[]
> array is grown by the same amount.  Do we need to check for the
> NULL-ness of branch->merge_name?
> 
> Near the beginning of set_merge() we allocate branch->merge[] 
> only when branch->merge_nr is non-zero (the assumption seems to be
> that we accumulate in the merge_names[] while reading the config,
> so as long as branch->merge is initialized properly to NULL, it will
> never be NULL if merge_nr is not 0, no?
> 
> Thanks.

Perhaps something like this might be a better solution so that we don't
need to worry about merge being NULL and tracking the difference between
merge_name and merge. Its a bit ugly still with a partially initialized
refspec_item so I am not 100% sure but I think this is overall a bit
better than the current situation:

> diff --git i/remote.h w/remote.h
> index 59033d5d82dd..0ca399e1835b 100644
> --- i/remote.h
> +++ w/remote.h
> @@ -316,8 +316,8 @@ struct branch {
> 
>         char *pushremote_name;
> 
> -       /* An array of the "merge" lines in the configuration. */
> -       char **merge_name;
> +       /* True if set_merge() has been called to finalize the merge array */
> +       int set_merge;
> 
>         /**
>          * An array of the struct refspecs used for the merge lines. That is,
> diff --git i/branch.c w/branch.c
> index 6d01d7d6bdb2..93f5b4e8dd9f 100644
> --- i/branch.c
> +++ w/branch.c
> @@ -230,7 +230,7 @@ static int inherit_tracking(struct tracking *tracking, const char *orig_ref)
>                 return -1;
>         }
> 
> -       if (branch->merge_nr < 1 || !branch->merge_name || !branch->merge_name[0]) {
> +       if (branch->merge_nr < 1 || !branch->merge || !branch->merge[0] || !branch->merge[0]->src) {
>                 warning(_("asked to inherit tracking from '%s', but no merge configuration is set"),
>                         bare_ref);
>                 return -1;
> @@ -238,7 +238,7 @@ static int inherit_tracking(struct tracking *tracking, const char *orig_ref)
> 
>         tracking->remote = branch->remote_name;
>         for (i = 0; i < branch->merge_nr; i++)
> -               string_list_append(tracking->srcs, branch->merge_name[i]);
> +               string_list_append(tracking->srcs, branch->merge[i]->src);
>         return 0;
>  }
> 
> diff --git i/builtin/pull.c w/builtin/pull.c
> index a1ebc6ad3328..f4556ae155ce 100644
> --- i/builtin/pull.c
> +++ w/builtin/pull.c
> @@ -487,7 +487,7 @@ static void NORETURN die_no_merge_candidates(const char *repo, const char **refs
>         } else
>                 fprintf_ln(stderr, _("Your configuration specifies to merge with the ref '%s'\n"
>                         "from the remote, but no such ref was fetched."),
> -                       *curr_branch->merge_name);
> +                       curr_branch->merge[0]->src);
>         exit(1);
>  }
> 
> diff --git i/remote.c w/remote.c
> index 706c25af0c27..1850e8fa4e42 100644
> --- i/remote.c
> +++ w/remote.c
> @@ -177,9 +177,15 @@ static void remote_clear(struct remote *remote)
> 
>  static void add_merge(struct branch *branch, const char *name)
>  {
> -       ALLOC_GROW(branch->merge_name, branch->merge_nr + 1,
> +       struct refspec_item *merge;
> +
> +       ALLOC_GROW(branch->merge, branch->merge_nr + 1,
>                    branch->merge_alloc);
> -       branch->merge_name[branch->merge_nr++] = xstrdup(name);
> +
> +       merge = xcalloc(1, sizeof(*merge));
> +       merge->src = xstrdup(name);
> +
> +       branch->merge[branch->merge_nr++] = merge;
>  }
> 
>  struct branches_hash_key {
> @@ -250,22 +256,23 @@ static struct branch *make_branch(struct remote_state *remote_state,
>         return ret;
>  }
> 
> +static void merge_clear(struct branch *branch)
> +{
> +       for (int i = 0; i < branch->merge_nr; i++) {
> +               refspec_item_clear(branch->merge[i]);
> +               free(branch->merge[i]);
> +       }
> +       free(branch->merge);
> +       branch->merge_nr = 0;
> +}
> +
>  static void branch_release(struct branch *branch)
>  {
>         free((char *)branch->name);
>         free((char *)branch->refname);
>         free(branch->remote_name);
>         free(branch->pushremote_name);
> -       for (int i = 0; i < branch->merge_nr; i++) {
> -               if (branch->merge) {
> -                       refspec_item_clear(branch->merge[i]);
> -                       free(branch->merge[i]);
> -               }
> -               if (branch->merge_name)
> -                       free(branch->merge_name[i]);
> -       }
> -       free(branch->merge);
> -       free(branch->merge_name);
> +       merge_clear(branch);
>  }
> 
>  static struct rewrite *make_rewrite(struct rewrites *r,
> @@ -700,7 +707,7 @@ char *remote_ref_for_branch(struct branch *branch, int for_push)
>         if (branch) {
>                 if (!for_push) {
>                         if (branch->merge_nr) {
> -                               return xstrdup(branch->merge_name[0]);
> +                               return xstrdup(branch->merge[0]->src);
>                         }
>                 } else {
>                         char *dst;
> @@ -1738,32 +1745,30 @@ static void set_merge(struct repository *repo, struct branch *ret)
> 
>         if (!ret)
>                 return; /* no branch */
> -       if (ret->merge)
> +       if (ret->set_merge)
>                 return; /* already run */
>         if (!ret->remote_name || !ret->merge_nr) {
>                 /*
>                  * no merge config; let's make sure we don't confuse callers
>                  * with a non-zero merge_nr but a NULL merge
>                  */
> -               ret->merge_nr = 0;
> +               merge_clear(ret);
>                 return;
>         }
> +       ret->set_merge = 1;
> 
>         remote = remotes_remote_get(repo, ret->remote_name);
> 
> -       CALLOC_ARRAY(ret->merge, ret->merge_nr);
>         for (i = 0; i < ret->merge_nr; i++) {
> -               ret->merge[i] = xcalloc(1, sizeof(**ret->merge));
> -               ret->merge[i]->src = xstrdup(ret->merge_name[i]);
>                 if (!remote_find_tracking(remote, ret->merge[i]) ||
>                     strcmp(ret->remote_name, "."))
>                         continue;
> -               if (repo_dwim_ref(repo, ret->merge_name[i],
> -                                 strlen(ret->merge_name[i]), &oid, &ref,
> +               if (repo_dwim_ref(repo, ret->merge[i]->src,
> +                                 strlen(ret->merge[i]->src), &oid, &ref,
>                                   0) == 1)
>                         ret->merge[i]->dst = ref;
>                 else
> -                       ret->merge[i]->dst = xstrdup(ret->merge_name[i]);
> +                       ret->merge[i]->dst = xstrdup(ret->merge[i]->src);
>         }
>  }
>