Re: [PATCH 5/9] merge-recursive: avoid returning a wholesale struct
From: Junio C Hamano <hidden>
Date: 2016-06-29 20:23:17
Johannes Schindelin [off-list ref] writes:
It is technically allowed, as per C89, for functions' return type to be complete structs (i.e. *not* just pointers to structs), but it is bad practice.
Not necessarily.
This is a very late attempt to contain the damage done by this developer in 6d297f8 (Status update on merge-recursive in C, 2006-07-08) which introduced such a return type. It will also help the current effort to libify merge-recursive.c, as it will allow us to return proper error codes later.
But this part of the motivation does make sense. Having to pass an extra int &error_code field, only because the return value is already used for something else, is a lot more weird.
quoted hunk
Signed-off-by: Johannes Schindelin <redacted> --- merge-recursive.c | 93 ++++++++++++++++++++++++++++++------------------------- 1 file changed, 50 insertions(+), 43 deletions(-)diff --git a/merge-recursive.c b/merge-recursive.c index c4ece96..d56651c9 100644 --- a/merge-recursive.c +++ b/merge-recursive.c@@ -888,47 +888,47 @@ static int merge_3way(struct merge_options *o, return merge_status; } -static struct merge_file_info merge_file_1(struct merge_options *o, +static int merge_file_1(struct merge_options *o, const struct diff_filespec *one, const struct diff_filespec *a, const struct diff_filespec *b, const char *branch1, - const char *branch2) + const char *branch2, + struct merge_file_info *result) { - struct merge_file_info result; - result.merge = 0; - result.clean = 1; + result->merge = 0; + result->clean = 1; if ((S_IFMT & a->mode) != (S_IFMT & b->mode)) { - result.clean = 0; + result->clean = 0; if (S_ISREG(a->mode)) { - result.mode = a->mode; - hashcpy(result.sha, a->sha1); + result->mode = a->mode; + hashcpy(result->sha, a->sha1); } else { - result.mode = b->mode; - hashcpy(result.sha, b->sha1); + result->mode = b->mode; + hashcpy(result->sha, b->sha1); } } else { if (!sha_eq(a->sha1, one->sha1) && !sha_eq(b->sha1, one->sha1)) - result.merge = 1; + result->merge = 1; /* * Merge modes */ if (a->mode == b->mode || a->mode == one->mode) - result.mode = b->mode; + result->mode = b->mode; else { - result.mode = a->mode; + result->mode = a->mode; if (b->mode != one->mode) { - result.clean = 0; - result.merge = 1; + result->clean = 0; + result->merge = 1; } } if (sha_eq(a->sha1, b->sha1) || sha_eq(a->sha1, one->sha1)) - hashcpy(result.sha, b->sha1); + hashcpy(result->sha, b->sha1); else if (sha_eq(b->sha1, one->sha1)) - hashcpy(result.sha, a->sha1); + hashcpy(result->sha, a->sha1); else if (S_ISREG(a->mode)) { mmbuffer_t result_buf; int merge_status;@@ -940,62 +940,63 @@ static struct merge_file_info merge_file_1(struct merge_options *o, die(_("Failed to execute internal merge")); if (write_sha1_file(result_buf.ptr, result_buf.size, - blob_type, result.sha)) + blob_type, result->sha)) die(_("Unable to add %s to database"), a->path); free(result_buf.ptr); - result.clean = (merge_status == 0); + result->clean = (merge_status == 0); } else if (S_ISGITLINK(a->mode)) { - result.clean = merge_submodule(result.sha, + result->clean = merge_submodule(result->sha, one->path, one->sha1, a->sha1, b->sha1, !o->call_depth); } else if (S_ISLNK(a->mode)) { - hashcpy(result.sha, a->sha1); + hashcpy(result->sha, a->sha1); if (!sha_eq(a->sha1, b->sha1)) - result.clean = 0; + result->clean = 0; } else die(_("BUG: unsupported object type in the tree")); } - return result; + return 0; } -static struct merge_file_info -merge_file_special_markers(struct merge_options *o, +static int merge_file_special_markers(struct merge_options *o, const struct diff_filespec *one, const struct diff_filespec *a, const struct diff_filespec *b, const char *branch1, const char *filename1, const char *branch2, - const char *filename2) + const char *filename2, + struct merge_file_info *mfi) { char *side1 = NULL; char *side2 = NULL; - struct merge_file_info mfi; + int ret; if (filename1) side1 = xstrfmt("%s:%s", branch1, filename1); if (filename2) side2 = xstrfmt("%s:%s", branch2, filename2); - mfi = merge_file_1(o, one, a, b, - side1 ? side1 : branch1, side2 ? side2 : branch2); + ret = merge_file_1(o, one, a, b, + side1 ? side1 : branch1, side2 ? side2 : branch2, mfi); free(side1); free(side2); - return mfi; + return ret; } -static struct merge_file_info merge_file_one(struct merge_options *o, +static int merge_file_one(struct merge_options *o, const char *path, const unsigned char *o_sha, int o_mode, const unsigned char *a_sha, int a_mode, const unsigned char *b_sha, int b_mode, const char *branch1, - const char *branch2) + const char *branch2, + struct merge_file_info *mfi) { struct diff_filespec one, a, b;@@ -1006,7 +1007,7 @@ static struct merge_file_info merge_file_one(struct merge_options *o, a.mode = a_mode; hashcpy(b.sha1, b_sha); b.mode = b_mode; - return merge_file_1(o, &one, &a, &b, branch1, branch2); + return merge_file_1(o, &one, &a, &b, branch1, branch2, mfi); } static void handle_change_delete(struct merge_options *o,@@ -1179,11 +1180,14 @@ static void conflict_rename_rename_1to2(struct merge_options *o, struct merge_file_info mfi; struct diff_filespec other; struct diff_filespec *add; - mfi = merge_file_one(o, one->path, + + if (merge_file_one(o, one->path, one->sha1, one->mode, a->sha1, a->mode, b->sha1, b->mode, - ci->branch1, ci->branch2); + ci->branch1, ci->branch2, &mfi) < 0) + return; + /* * FIXME: For rename/add-source conflicts (if we could detect * such), this is wrong. We should instead find a unique@@ -1237,12 +1241,13 @@ static void conflict_rename_rename_2to1(struct merge_options *o, remove_file(o, 1, a->path, o->call_depth || would_lose_untracked(a->path)); remove_file(o, 1, b->path, o->call_depth || would_lose_untracked(b->path)); - mfi_c1 = merge_file_special_markers(o, a, c1, &ci->ren1_other, + if (merge_file_special_markers(o, a, c1, &ci->ren1_other, o->branch1, c1->path, - o->branch2, ci->ren1_other.path); - mfi_c2 = merge_file_special_markers(o, b, &ci->ren2_other, c2, + o->branch2, ci->ren1_other.path, &mfi_c1) < 0 || + merge_file_special_markers(o, b, &ci->ren2_other, c2, o->branch1, ci->ren2_other.path, - o->branch2, c2->path); + o->branch2, c2->path, &mfi_c2) < 0) + return; if (o->call_depth) { /*@@ -1463,10 +1468,11 @@ static int process_renames(struct merge_options *o, ren1_dst, branch2); if (o->call_depth) { struct merge_file_info mfi; - mfi = merge_file_one(o, ren1_dst, null_sha1, 0, + if (merge_file_one(o, ren1_dst, null_sha1, 0, ren1->pair->two->sha1, ren1->pair->two->mode, dst_other.sha1, dst_other.mode, - branch1, branch2); + branch1, branch2, &mfi) < 0) + return -1; output(o, 1, _("Adding merged %s"), ren1_dst); update_file(o, 0, mfi.sha, mfi.mode, ren1_dst); try_merge = 0;@@ -1622,9 +1628,10 @@ static int merge_content(struct merge_options *o, if (dir_in_way(path, !o->call_depth)) df_conflict_remains = 1; } - mfi = merge_file_special_markers(o, &one, &a, &b, + if (merge_file_special_markers(o, &one, &a, &b, o->branch1, path1, - o->branch2, path2); + o->branch2, path2, &mfi) < 0) + return -1; if (mfi.clean && !df_conflict_remains && sha_eq(mfi.sha, a_sha) && mfi.mode == a_mode) {