Re: [PATCH 5/9] merge-recursive: avoid returning a wholesale struct
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Jun 29, 2016, 20:21 UTC
- Message-ID
- <xmqqr3bf3fvr.fsf@gitster.mtv.corp.google.com>
- In-Reply-To
- <c33e9cdb1ec6cbebcc3124a62b7b9d52b92cf6c9.1467199553.git.johannes.schindelin@gmx.de>
Johannes Schindelin <johannes.schindelin@gmx.de> 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.
Show 6 quoted lines
> 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.
Show 224 quoted lines
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
> 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) {