@matheusbsbs / matheus-bagliano / commits / f62dcc7f30

remote: remove branch->merge_name and fix branch_release()

The branch structure has both branch->merge_name and branch->merge for tracking the merge information. The former is allocated by add_merge() and stores the names read from the configuration file. The latter is allocated by set_merge() which is called by branch_get() when an external caller requests a branch. This leads to the confusing situation where branch->merge_nr tracks both the size of branch->merge (once its allocated) and branch->merge_name. The branch_release() function incorrectly assumes that branch->merge is always set when branch->merge_nr is non-zero, and can potentially crash if read_config() is called without branch_get() being called on every branch. In addition, branch_release() fails to free some of the memory associated with the structure including: * Failure to free the refspec_item containers in branch->merge[i] * Failure to free the strings in branch->merge_name[i] * Failure to free the branch->merge_name parent array. The set_merge() function sets branch->merge_nr to 0 when there is no valid remote_name, to avoid external callers seeing a non-zero merge_nr but a NULL merge array. This results in failure to release most of the merge data as well. These issues could be fixed directly, and indeed I initially proposed such a change at [1] in the past. While this works, there was some confusion during review because of the inconsistencies. Instead, its time to clean up the situation properly. Remove branch->merge_name entirely. Instead, allocate branch->merge earlier within add_merge() instead of within set_merge(). Instead of having set_merge() copy from merge_name[i] to merge[i]->src, just have add_merge() directly initialize merge[i]->src. Modify the add_merge() to call xstrdup() itself, instead of having the caller of add_merge() do so. This makes it more obvious which code owns the memory. Update all callers which use branch->merge_name[i] to use branch->merge[i]->src instead. Add a merge_clear() function which properly releases all of the merge-related memory, and which sets branch->merge_nr to zero. Use this both in branch_release() and in set_merge(), fixing the leak when set_merge() finds no valid remote_name. Add a set_merge variable to the branch structure, which indicates whether set_merge() has been called. This replaces the previous use of a NULL check against the branch->merge array. With these changes, the merge array is always allocated when merge_nr is non-zero. This use of refspec_item to store the names should be safe. External callers should be using branch_get() to obtain a pointer to the branch, which will call set_merge(), and the callers internal to remote.c already handle the partially initialized refpsec_item structure safely. This end result is cleaner, and avoids duplicating the merge names twice. Signed-off-by: Jacob Keller <jacob.keller@gmail.com> Link: [1] https://lore.kernel.org/git/20250617-jk-submodule-helper-use-url-v2-1-04cbb003177d@gmail.com/ Signed-off-by: Junio C Hamano <gitster@pobox.com>

Jacob Keller committed Jun 23, 2025 at 16:11 UTC f62dcc7f30d16af29c0f707005aceb5eb6119279

4 files changed +33 -21

branch.c

+2 -2

@@ -230,7 +230,7 @@ static int inherit_tracking(struct tracking *tracking, const char *orig_ref)

230 return -1;

231 }

232

233 - if (branch->merge_nr < 1 || !branch->merge_name || !branch->merge_name[0]) {

233 + if (branch->merge_nr < 1 || !branch->merge || !branch->merge[0] || !branch->merge[0]->src) {

234 warning(_("asked to inherit tracking from '%s', but no merge configuration is set"),

235 bare_ref);

236 return -1;

@@ -238,7 +238,7 @@ static int inherit_tracking(struct tracking *tracking, const char *orig_ref)

238

239 tracking->remote = branch->remote_name;

240 for (i = 0; i < branch->merge_nr; i++)

241 - string_list_append(tracking->srcs, branch->merge_name[i]);

241 + string_list_append(tracking->srcs, branch->merge[i]->src);

242 return 0;

243 }

244

builtin/pull.c

+1 -1

@@ -487,7 +487,7 @@ static void NORETURN die_no_merge_candidates(const char *repo, const char **refs

487 } else

488 fprintf_ln(stderr, _("Your configuration specifies to merge with the ref '%s'\n"

489 "from the remote, but no such ref was fetched."),

490 - *curr_branch->merge_name);

490 + curr_branch->merge[0]->src);

491 exit(1);

492 }

493

remote.c

+28 -16

@@ -174,9 +174,15 @@ static void remote_clear(struct remote *remote)

174

175 static void add_merge(struct branch *branch, const char *name)

176 {

177 - ALLOC_GROW(branch->merge_name, branch->merge_nr + 1,

177 + struct refspec_item *merge;

178 +

179 + ALLOC_GROW(branch->merge, branch->merge_nr + 1,

180 branch->merge_alloc);

179 - branch->merge_name[branch->merge_nr++] = name;

181 +

182 + merge = xcalloc(1, sizeof(*merge));

183 + merge->src = xstrdup(name);

184 +

185 + branch->merge[branch->merge_nr++] = merge;

186 }

187

188 struct branches_hash_key {

@@ -247,15 +253,23 @@ static struct branch *make_branch(struct remote_state *remote_state,

253 return ret;

254 }

255

256 +static void merge_clear(struct branch *branch)

257 +{

258 + for (int i = 0; i < branch->merge_nr; i++) {

259 + refspec_item_clear(branch->merge[i]);

260 + free(branch->merge[i]);

261 + }

262 + FREE_AND_NULL(branch->merge);

263 + branch->merge_nr = 0;

264 +}

265 +

266 static void branch_release(struct branch *branch)

267 {

268 free((char *)branch->name);

269 free((char *)branch->refname);

270 free(branch->remote_name);

271 free(branch->pushremote_name);

256 - for (int i = 0; i < branch->merge_nr; i++)

257 - refspec_item_clear(branch->merge[i]);

258 - free(branch->merge);

272 + merge_clear(branch);

273 }

274

275 static struct rewrite *make_rewrite(struct rewrites *r,

@@ -429,7 +443,7 @@ static int handle_config(const char *key, const char *value,

443 } else if (!strcmp(subkey, "merge")) {

444 if (!value)

445 return config_error_nonbool(key);

432 - add_merge(branch, xstrdup(value));

446 + add_merge(branch, value);

447 }

448 return 0;

449 }

@@ -692,7 +706,7 @@ char *remote_ref_for_branch(struct branch *branch, int for_push)

706 if (branch) {

707 if (!for_push) {

708 if (branch->merge_nr) {

695 - return xstrdup(branch->merge_name[0]);

709 + return xstrdup(branch->merge[0]->src);

710 }

711 } else {

712 char *dst;

@@ -1731,32 +1745,30 @@ static void set_merge(struct remote_state *remote_state, struct branch *ret)

1745

1746 if (!ret)

1747 return; /* no branch */

1734 - if (ret->merge)

1748 + if (ret->set_merge)

1749 return; /* already run */

1750 if (!ret->remote_name || !ret->merge_nr) {

1751 /*

1752 * no merge config; let's make sure we don't confuse callers

1753 * with a non-zero merge_nr but a NULL merge

1754 */

1741 - ret->merge_nr = 0;

1755 + merge_clear(ret);

1756 return;

1757 }

1758 + ret->set_merge = 1;

1759

1760 remote = remotes_remote_get(remote_state, ret->remote_name);

1761

1747 - CALLOC_ARRAY(ret->merge, ret->merge_nr);

1762 for (i = 0; i < ret->merge_nr; i++) {

1749 - ret->merge[i] = xcalloc(1, sizeof(**ret->merge));

1750 - ret->merge[i]->src = xstrdup(ret->merge_name[i]);

1763 if (!remote_find_tracking(remote, ret->merge[i]) ||

1764 strcmp(ret->remote_name, "."))

1765 continue;

1754 - if (repo_dwim_ref(the_repository, ret->merge_name[i],

1755 - strlen(ret->merge_name[i]), &oid, &ref,

1766 + if (repo_dwim_ref(the_repository, ret->merge[i]->src,

1767 + strlen(ret->merge[i]->src), &oid, &ref,

1768 0) == 1)

1769 ret->merge[i]->dst = ref;

1770 else

1759 - ret->merge[i]->dst = xstrdup(ret->merge_name[i]);

1771 + ret->merge[i]->dst = xstrdup(ret->merge[i]->src);

1772 }

1773 }

1774

@@ -1776,7 +1788,7 @@ struct branch *branch_get(const char *name)

1788

1789 int branch_has_merge_config(struct branch *branch)

1790 {

1779 - return branch && !!branch->merge;

1791 + return branch && branch->set_merge;

1792 }

1793

1794 int branch_merge_matches(struct branch *branch,

remote.h

+2 -2

@@ -315,8 +315,8 @@ struct branch {

315

316 char *pushremote_name;

317

318 - /* An array of the "merge" lines in the configuration. */

319 - const char **merge_name;

318 + /* True if set_merge() has been called to finalize the merge array */

319 + int set_merge;

320

321 /**

322 * An array of the struct refspecs used for the merge lines. That is,