@matheusbsbs / matheus-bagliano / commits / 35f6967161

ll-merge: make callers responsible for showing warnings

Since some callers may want to send warning messages to somewhere other than stdout/stderr, stop printing "warning: Cannot merge binary files" from ll-merge and instead modify the return status of ll_merge() to indicate when a merge of binary files has occurred. Message printing probably does not belong in a "low-level merge" anyway. This commit continues printing the message as-is, just from the callers instead of within ll_merge(). Future changes will start handling the message differently in the merge-ort codepath. There was one special case here: the callers in rerere.c do NOT check for and print such a message; since those code paths explicitly skip over binary files, there is no reason to check for a return status of LL_MERGE_BINARY_CONFLICT or print the related message. Note that my methodology included first modifying ll_merge() to return a struct, so that the compiler would catch all the callers for me and ensure I had modified all of them. After modifying all of them, I then changed the struct to an enum. Signed-off-by: Elijah Newren <newren@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>

Elijah Newren committed Feb 2, 2022 at 02:37 UTC 35f6967161860cb5c067e961b7283e8389ff0726

9 files changed +63 -32

apply.c

+4 -1

@@ -3492,7 +3492,7 @@ static int three_way_merge(struct apply_state *state,

3492 {

3493 mmfile_t base_file, our_file, their_file;

3494 mmbuffer_t result = { NULL };

3495 - int status;

3495 + enum ll_merge_result status;

3496

3497 /* resolve trivial cases first */

3498 if (oideq(base, ours))

@@ -3509,6 +3509,9 @@ static int three_way_merge(struct apply_state *state,

3509 &their_file, "theirs",

3510 state->repo->index,

3511 NULL);

3512 + if (status == LL_MERGE_BINARY_CONFLICT)

3513 + warning("Cannot merge binary files: %s (%s vs. %s)",

3514 + path, "ours", "theirs");

3515 free(base_file.ptr);

3516 free(our_file.ptr);

3517 free(their_file.ptr);

builtin/checkout.c

+8 -4

@@ -245,6 +245,7 @@ static int checkout_merged(int pos, const struct checkout *state,

245 struct cache_entry *ce = active_cache[pos];

246 const char *path = ce->name;

247 mmfile_t ancestor, ours, theirs;

248 + enum ll_merge_result merge_status;

249 int status;

250 struct object_id oid;

251 mmbuffer_t result_buf;

@@ -275,13 +276,16 @@ static int checkout_merged(int pos, const struct checkout *state,

276 memset(&ll_opts, 0, sizeof(ll_opts));

277 git_config_get_bool("merge.renormalize", &renormalize);

278 ll_opts.renormalize = renormalize;

278 - status = ll_merge(&result_buf, path, &ancestor, "base",

279 - &ours, "ours", &theirs, "theirs",

280 - state->istate, &ll_opts);

279 + merge_status = ll_merge(&result_buf, path, &ancestor, "base",

280 + &ours, "ours", &theirs, "theirs",

281 + state->istate, &ll_opts);

282 free(ancestor.ptr);

283 free(ours.ptr);

284 free(theirs.ptr);

284 - if (status < 0 || !result_buf.ptr) {

285 + if (merge_status == LL_MERGE_BINARY_CONFLICT)

286 + warning("Cannot merge binary files: %s (%s vs. %s)",

287 + path, "ours", "theirs");

288 + if (merge_status < 0 || !result_buf.ptr) {

289 free(result_buf.ptr);

290 return error(_("path '%s': cannot merge"), path);

291 }

ll-merge.c

+22 -18

@@ -14,7 +14,7 @@

15 struct ll_merge_driver;

17 -typedef int (*ll_merge_fn)(const struct ll_merge_driver *,

17 +typedef enum ll_merge_result (*ll_merge_fn)(const struct ll_merge_driver *,

18 mmbuffer_t *result,

19 const char *path,

20 mmfile_t *orig, const char *orig_name,

@@ -49,7 +49,7 @@ void reset_merge_attributes(void)

49 /*

50 * Built-in low-levels

51 */

52 -static int ll_binary_merge(const struct ll_merge_driver *drv_unused,

52 +static enum ll_merge_result ll_binary_merge(const struct ll_merge_driver *drv_unused,

53 mmbuffer_t *result,

54 const char *path,

55 mmfile_t *orig, const char *orig_name,

@@ -58,6 +58,7 @@ static int ll_binary_merge(const struct ll_merge_driver *drv_unused,

58 const struct ll_merge_options *opts,

59 int marker_size)

60 {

61 + enum ll_merge_result ret;

62 mmfile_t *stolen;

63 assert(opts);

@@ -68,16 +69,19 @@ static int ll_binary_merge(const struct ll_merge_driver *drv_unused,

69 */

70 if (opts->virtual_ancestor) {

71 stolen = orig;

72 + ret = LL_MERGE_OK;

73 } else {

74 switch (opts->variant) {

75 default:

74 - warning("Cannot merge binary files: %s (%s vs. %s)",

75 - path, name1, name2);

76 - /* fallthru */

76 + ret = LL_MERGE_BINARY_CONFLICT;

77 + stolen = src1;

78 + break;

79 case XDL_MERGE_FAVOR_OURS:

80 + ret = LL_MERGE_OK;

81 stolen = src1;

82 break;

83 case XDL_MERGE_FAVOR_THEIRS:

84 + ret = LL_MERGE_OK;

85 stolen = src2;

86 break;

87 }

@@ -87,16 +91,10 @@ static int ll_binary_merge(const struct ll_merge_driver *drv_unused,

91 result->size = stolen->size;

92 stolen->ptr = NULL;

90 - /*

91 - * With -Xtheirs or -Xours, we have cleanly merged;

92 - * otherwise we got a conflict.

93 - */

94 - return opts->variant == XDL_MERGE_FAVOR_OURS ||

95 - opts->variant == XDL_MERGE_FAVOR_THEIRS ?

96 - 0 : 1;

94 + return ret;

95 }

99 -static int ll_xdl_merge(const struct ll_merge_driver *drv_unused,

97 +static enum ll_merge_result ll_xdl_merge(const struct ll_merge_driver *drv_unused,

98 mmbuffer_t *result,

99 const char *path,

100 mmfile_t *orig, const char *orig_name,

@@ -105,7 +103,9 @@ static int ll_xdl_merge(const struct ll_merge_driver *drv_unused,

103 const struct ll_merge_options *opts,

104 int marker_size)

105 {

106 + enum ll_merge_result ret;

107 xmparam_t xmp;

108 + int status;

109 assert(opts);

110

111 if (orig->size > MAX_XDIFF_SIZE ||

@@ -133,10 +133,12 @@ static int ll_xdl_merge(const struct ll_merge_driver *drv_unused,

133 xmp.ancestor = orig_name;

134 xmp.file1 = name1;

135 xmp.file2 = name2;

136 - return xdl_merge(orig, src1, src2, &xmp, result);

136 + status = xdl_merge(orig, src1, src2, &xmp, result);

137 + ret = (status > 0) ? LL_MERGE_CONFLICT : status;

138 + return ret;

139 }

140

139 -static int ll_union_merge(const struct ll_merge_driver *drv_unused,

141 +static enum ll_merge_result ll_union_merge(const struct ll_merge_driver *drv_unused,

142 mmbuffer_t *result,

143 const char *path,

144 mmfile_t *orig, const char *orig_name,

@@ -178,7 +180,7 @@ static void create_temp(mmfile_t *src, char *path, size_t len)

180 /*

181 * User defined low-level merge driver support.

182 */

181 -static int ll_ext_merge(const struct ll_merge_driver *fn,

183 +static enum ll_merge_result ll_ext_merge(const struct ll_merge_driver *fn,

184 mmbuffer_t *result,

185 const char *path,

186 mmfile_t *orig, const char *orig_name,

@@ -194,6 +196,7 @@ static int ll_ext_merge(const struct ll_merge_driver *fn,

196 const char *args[] = { NULL, NULL };

197 int status, fd, i;

198 struct stat st;

199 + enum ll_merge_result ret;

200 assert(opts);

201

202 sq_quote_buf(&path_sq, path);

@@ -236,7 +239,8 @@ static int ll_ext_merge(const struct ll_merge_driver *fn,

239 unlink_or_warn(temp[i]);

240 strbuf_release(&cmd);

241 strbuf_release(&path_sq);

239 - return status;

242 + ret = (status > 0) ? LL_MERGE_CONFLICT : status;

243 + return ret;

244 }

245

246 /*

@@ -362,7 +366,7 @@ static void normalize_file(mmfile_t *mm, const char *path, struct index_state *i

366 }

367 }

368

365 -int ll_merge(mmbuffer_t *result_buf,

369 +enum ll_merge_result ll_merge(mmbuffer_t *result_buf,

370 const char *path,

371 mmfile_t *ancestor, const char *ancestor_label,

372 mmfile_t *ours, const char *our_label,

ll-merge.h

+8 -1

@@ -82,13 +82,20 @@ struct ll_merge_options {

82 long xdl_opts;

83 };

85 +enum ll_merge_result {

86 + LL_MERGE_ERROR = -1,

87 + LL_MERGE_OK = 0,

88 + LL_MERGE_CONFLICT,

89 + LL_MERGE_BINARY_CONFLICT,

90 +};

91 +

92 /**

93 * Perform a three-way single-file merge in core. This is a thin wrapper

94 * around `xdl_merge` that takes the path and any merge backend specified in

95 * `.gitattributes` or `.git/info/attributes` into account.

96 * Returns 0 for a clean merge.

97 */

91 -int ll_merge(mmbuffer_t *result_buf,

98 +enum ll_merge_result ll_merge(mmbuffer_t *result_buf,

99 const char *path,

100 mmfile_t *ancestor, const char *ancestor_label,

101 mmfile_t *ours, const char *our_label,

merge-blobs.c

+4 -1

@@ -36,7 +36,7 @@ static void *three_way_filemerge(struct index_state *istate,

36 mmfile_t *their,

37 unsigned long *size)

38 {

39 - int merge_status;

39 + enum ll_merge_result merge_status;

40 mmbuffer_t res;

42 /*

@@ -50,6 +50,9 @@ static void *three_way_filemerge(struct index_state *istate,

50 istate, NULL);

51 if (merge_status < 0)

52 return NULL;

53 + if (merge_status == LL_MERGE_BINARY_CONFLICT)

54 + warning("Cannot merge binary files: %s (%s vs. %s)",

55 + path, ".our", ".their");

57 *size = res.size;

58 return res.ptr;

merge-ort.c

+4 -1

@@ -1743,7 +1743,7 @@ static int merge_3way(struct merge_options *opt,

1743 mmfile_t orig, src1, src2;

1744 struct ll_merge_options ll_opts = {0};

1745 char *base, *name1, *name2;

1746 - int merge_status;

1746 + enum ll_merge_result merge_status;

1747

1748 if (!opt->priv->attr_index.initialized)

1749 initialize_attr_index(opt);

@@ -1787,6 +1787,9 @@ static int merge_3way(struct merge_options *opt,

1787 merge_status = ll_merge(result_buf, path, &orig, base,

1788 &src1, name1, &src2, name2,

1789 &opt->priv->attr_index, &ll_opts);

1790 + if (merge_status == LL_MERGE_BINARY_CONFLICT)

1791 + warning("Cannot merge binary files: %s (%s vs. %s)",

1792 + path, name1, name2);

1793

1794 free(base);

1795 free(name1);

merge-recursive.c

+4 -1

@@ -1044,7 +1044,7 @@ static int merge_3way(struct merge_options *opt,

1044 mmfile_t orig, src1, src2;

1045 struct ll_merge_options ll_opts = {0};

1046 char *base, *name1, *name2;

1047 - int merge_status;

1047 + enum ll_merge_result merge_status;

1048

1049 ll_opts.renormalize = opt->renormalize;

1050 ll_opts.extra_marker_size = extra_marker_size;

@@ -1090,6 +1090,9 @@ static int merge_3way(struct merge_options *opt,

1090 merge_status = ll_merge(result_buf, a->path, &orig, base,

1091 &src1, name1, &src2, name2,

1092 opt->repo->index, &ll_opts);

1093 + if (merge_status == LL_MERGE_BINARY_CONFLICT)

1094 + warning("Cannot merge binary files: %s (%s vs. %s)",

1095 + a->path, name1, name2);

1096

1097 free(base);

1098 free(name1);

notes-merge.c

+4 -1

@@ -344,7 +344,7 @@ static int ll_merge_in_worktree(struct notes_merge_options *o,

344 {

345 mmbuffer_t result_buf;

346 mmfile_t base, local, remote;

347 - int status;

347 + enum ll_merge_result status;

348

349 read_mmblob(&base, &p->base);

350 read_mmblob(&local, &p->local);

@@ -358,6 +358,9 @@ static int ll_merge_in_worktree(struct notes_merge_options *o,

358 free(local.ptr);

359 free(remote.ptr);

360

361 + if (status == LL_MERGE_BINARY_CONFLICT)

362 + warning("Cannot merge binary files: %s (%s vs. %s)",

363 + oid_to_hex(&p->obj), o->local_ref, o->remote_ref);

364 if ((status < 0) || !result_buf.ptr)

365 die("Failed to execute internal merge");

366

rerere.c

+5 -4

@@ -609,19 +609,20 @@ static int try_merge(struct index_state *istate,

609 const struct rerere_id *id, const char *path,

610 mmfile_t *cur, mmbuffer_t *result)

611 {

612 - int ret;

612 + enum ll_merge_result ret;

613 mmfile_t base = {NULL, 0}, other = {NULL, 0};

614

615 if (read_mmfile(&base, rerere_path(id, "preimage")) ||

616 - read_mmfile(&other, rerere_path(id, "postimage")))

617 - ret = 1;

618 - else

616 + read_mmfile(&other, rerere_path(id, "postimage"))) {

617 + ret = LL_MERGE_CONFLICT;

618 + } else {

619 /*

620 * A three-way merge. Note that this honors user-customizable

621 * low-level merge driver settings.

622 */

623 ret = ll_merge(result, path, &base, NULL, cur, "", &other, "",

624 istate, NULL);

625 + }

626

627 free(base.ptr);

628 free(other.ptr);