Johannes Schindelin <johannes.schindelin@xxxxxx> writes:
> diff --git a/merge-recursive.c b/merge-recursive.c
> index 6ab7dfc..bb075e3 100644
> --- a/merge-recursive.c
> +++ b/merge-recursive.c
> @@ -266,8 +266,10 @@ struct tree *write_tree_from_memory(struct merge_options *o)
> active_cache_tree = cache_tree();
>
> if (!cache_tree_fully_valid(active_cache_tree) &&
> - cache_tree_update(&the_index, 0) < 0)
> - die(_("error building trees"));
> + cache_tree_update(&the_index, 0) < 0) {
> + error(_("error building trees"));
> + return NULL;
> + }
This actually is a BUG(), isn't it? We have already verified that
the cache is merged, so cache_tree_update() ought to be able to come
up with the whole-tree hash.
> @@ -548,19 +550,17 @@ static int update_stages(const char *path, const struct diff_filespec *o,
> */
> int clear = 1;
> int options = ADD_CACHE_OK_TO_ADD | ADD_CACHE_SKIP_DFCHECK;
> + int ret = 0;
> +
> if (clear)
> - if (remove_file_from_cache(path))
> - return -1;
> - if (o)
> - if (add_cacheinfo(o->mode, o->sha1, path, 1, 0, options))
> - return -1;
> - if (a)
> - if (add_cacheinfo(a->mode, a->sha1, path, 2, 0, options))
> - return -1;
> - if (b)
> - if (add_cacheinfo(b->mode, b->sha1, path, 3, 0, options))
> - return -1;
> - return 0;
> + ret = remove_file_from_cache(path);
> + if (!ret && o)
> + ret = add_cacheinfo(o->mode, o->sha1, path, 1, 0, options);
> + if (!ret && a)
> + ret = add_cacheinfo(a->mode, a->sha1, path, 2, 0, options);
> + if (!ret && b)
> + ret = add_cacheinfo(b->mode, b->sha1, path, 3, 0, options);
> + return ret;
> }
Aren't the preimage and the postimage doing the same thing? The
only two differences I spot are (1) it is clear in the original that
the returned value is -1 in the error case, even if the error
convention of remove_file_from_cache() and add_cacheinfo() were "0
is good, others are bad"; and (2) the control flow is easier to
follow in the original.
> @@ -736,7 +736,7 @@ static int make_room_for_path(struct merge_options *o, const char *path)
> return error(msg, path, _(": perhaps a D/F conflict?"));
> }
>
> -static void update_file_flags(struct merge_options *o,
> +static int update_file_flags(struct merge_options *o,
> const unsigned char *sha,
> unsigned mode,
> const char *path,
> @@ -777,8 +777,7 @@ static void update_file_flags(struct merge_options *o,
>
> if (make_room_for_path(o, path) < 0) {
> update_wd = 0;
> - free(buf);
> - goto update_index;
> + goto free_buf;
> }
> if (S_ISREG(mode) || (!has_symlinks && S_ISLNK(mode))) {
> int fd;
> @@ -801,20 +800,22 @@ static void update_file_flags(struct merge_options *o,
> } else
> die(_("do not know what to do with %06o %s '%s'"),
> mode, sha1_to_hex(sha), path);
> +free_buf:
> free(buf);
I somehow find the above change harder to follow than the original.
> }
> update_index:
> if (update_cache)
> add_cacheinfo(mode, sha, path, 0, update_wd, ADD_CACHE_OK_TO_ADD);
> + return 0;
> }
>
This one is in line with the stated goal of the patch.
> @@ -1028,21 +1030,23 @@ static void handle_change_delete(struct merge_options *o,
> * correct; since there is no true "middle point" between
> * them, simply reuse the base version for virtual merge base.
> */
> - remove_file_from_cache(path);
> - update_file(o, 0, o_sha, o_mode, renamed ? renamed : path);
> + ret = remove_file_from_cache(path);
> + if (!ret)
> + ret = update_file(o, 0, o_sha, o_mode,
> + renamed ? renamed : path);
As you noted in the log message, this does change the behaviour. If
remove returns non-zero for whatever reason, we still did update()
in the original, but we no longer do. Does this have negative
effect to the overall codeflow?
Or, assuming that everybody returns -1 for errors, perhaps
ret = remove();
ret |= update();
may be a more faithful and safe conversion?
> @@ -1087,21 +1094,22 @@ static void conflict_rename_delete(struct merge_options *o,
> b_mode = dest->mode;
> }
>
> - handle_change_delete(o,
> + ret = handle_change_delete(o,
> o->call_depth ? orig->path : dest->path,
> orig->sha1, orig->mode,
> a_sha, a_mode,
> b_sha, b_mode,
> _("rename"), _("renamed"));
> -
> - if (o->call_depth) {
> - remove_file_from_cache(dest->path);
> - } else {
> - update_stages(dest->path, NULL,
> + if (ret < 0)
> + return ret;
> + if (o->call_depth)
> + ret = remove_file_from_cache(dest->path);
> + else
> + ret = update_stages(dest->path, NULL,
> rename_branch == o->branch1 ? dest : NULL,
> rename_branch == o->branch1 ? NULL : dest);
> - }
Similarly, if handle_change_delete() returns non-zero, we no longer
call remove() or update(). Is that a good behaviour change?
> -static void handle_file(struct merge_options *o,
> +static int handle_file(struct merge_options *o,
> struct diff_filespec *rename,
> int stage,
> struct rename_conflict_info *ci)
Likewise.
> -static void conflict_rename_rename_1to2(struct merge_options *o,
> +static int conflict_rename_rename_1to2(struct merge_options *o,
> struct rename_conflict_info *ci)
> {
> ...
> - if (merge_file_one(o, one->path,
> + if ((ret = merge_file_one(o, one->path,
> one->sha1, one->mode,
> a->sha1, a->mode,
> b->sha1, b->mode,
> - ci->branch1, ci->branch2, &mfi) < 0)
> - return;
> + ci->branch1, ci->branch2, &mfi)))
> + return ret;
This does not change behaviour.
> @@ -1194,7 +1208,8 @@ static void conflict_rename_rename_1to2(struct merge_options *o,
> * pathname and then either rename the add-source file to that
> * unique path, or use that unique path instead of src here.
> */
> - update_file(o, 0, mfi.sha, mfi.mode, one->path);
> + if ((ret = update_file(o, 0, mfi.sha, mfi.mode, one->path)))
> + return ret;
But this does.
> @@ -1205,22 +1220,31 @@ static void conflict_rename_rename_1to2(struct merge_options *o,
> * resolving the conflict at that path in its favor.
> */
> add = filespec_from_entry(&other, ci->dst_entry1, 2 ^ 1);
> - if (add)
> - update_file(o, 0, add->sha1, add->mode, a->path);
> + if (add) {
> + if ((ret = update_file(o, 0, add->sha1, add->mode,
> + a->path)))
> + return ret;
> + }
So does this.
> else
> remove_file_from_cache(a->path);
> add = filespec_from_entry(&other, ci->dst_entry2, 3 ^ 1);
> - if (add)
> - update_file(o, 0, add->sha1, add->mode, b->path);
> + if (add) {
> + if ((ret = update_file(o, 0, add->sha1, add->mode,
> + b->path)))
> + return ret;
> + }
And this.
> } else {
> - handle_file(o, a, 2, ci);
> - handle_file(o, b, 3, ci);
> + if ((ret = handle_file(o, a, 2, ci)) ||
> + (ret = handle_file(o, b, 3, ci)))
> + return ret;
And this.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html