On 6/17/2025 3:25 PM, Junio C Hamano wrote:
> Jacob Keller <jacob.e.keller@xxxxxxxxx> writes:
>
>> From: Jacob Keller <jacob.keller@xxxxxxxxx>
>
> 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);
> }
> }
>
