From 3be18b47e4249a3e3f8ae9968abffc954c7e293a Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Tue, 26 Jul 2016 18:05:43 +0200 Subject: t5520: verify that `pull --rebase` shows the helpful advice when failing It was noticed by Brendan Forster last October that the builtin `git am` regressed on that. Our hot fix reverted to spawning the recursive merge instead of using it as a library function. As we are about to revert that hot fix, after making the recursive merge a true library function (i.e. a function that does not die() in case of "normal" errors), let's add a test that verifies that we do not regress on the same problem which made the hot fix necessary in the first place. Signed-off-by: Johannes Schindelin Signed-off-by: Junio C Hamano diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh index 37ebbcf..6ad37b5 100755 --- a/t/t5520-pull.sh +++ b/t/t5520-pull.sh @@ -255,6 +255,38 @@ test_expect_success '--rebase' ' test new = "$(git show HEAD:file2)" ' +test_expect_success '--rebase with conflicts shows advice' ' + test_when_finished "git rebase --abort; git checkout -f to-rebase" && + git checkout -b seq && + test_seq 5 >seq.txt && + git add seq.txt && + test_tick && + git commit -m "Add seq.txt" && + echo 6 >>seq.txt && + test_tick && + git commit -m "Append to seq.txt" seq.txt && + git checkout -b with-conflicts HEAD^ && + echo conflicting >>seq.txt && + test_tick && + git commit -m "Create conflict" seq.txt && + test_must_fail git pull --rebase . seq 2>err >out && + grep "When you have resolved this problem" out +' + +test_expect_success 'failed --rebase shows advice' ' + test_when_finished "git rebase --abort; git checkout -f to-rebase" && + git checkout -b diverging && + test_commit attributes .gitattributes "* text=auto" attrs && + sha1="$(printf "1\\r\\n" | git hash-object -w --stdin)" && + git update-index --cacheinfo 0644 $sha1 file && + git commit -m v1-with-cr && + # force checkout because `git reset --hard` will not leave clean `file` + git checkout -f -b fails-to-rebase HEAD^ && + test_commit v2-without-cr file "2" file2-lf && + test_must_fail git pull --rebase . diverging 2>err >out && + grep "When you have resolved this problem" out +' + test_expect_success '--rebase fails with multiple branches' ' git reset --hard before-rebase && test_must_fail git pull --rebase . copy master 2>err && -- cgit v0.10.2-6-g49f6 From ef1177d18e35c030c37aa533002a11d98361e6b9 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Tue, 26 Jul 2016 18:05:50 +0200 Subject: die("bug"): report bugs consistently The vast majority of error messages in Git's source code which report a bug use the convention to prefix the message with "BUG:". As part of cleaning up merge-recursive to stop die()ing except in case of detected bugs, let's just make the remainder of the bug reports consistent with the de facto rule. Signed-off-by: Johannes Schindelin Signed-off-by: Junio C Hamano diff --git a/builtin/ls-files.c b/builtin/ls-files.c index f02e3d2..00ea91a 100644 --- a/builtin/ls-files.c +++ b/builtin/ls-files.c @@ -118,7 +118,8 @@ static void show_killed_files(struct dir_struct *dir) */ pos = cache_name_pos(ent->name, ent->len); if (0 <= pos) - die("bug in show-killed-files"); + die("BUG: killed-file %.*s not found", + ent->len, ent->name); pos = -pos - 1; while (pos < active_nr && ce_stage(active_cache[pos])) diff --git a/builtin/update-index.c b/builtin/update-index.c index 6cdfd5f..ba04b19 100644 --- a/builtin/update-index.c +++ b/builtin/update-index.c @@ -1146,7 +1146,7 @@ int cmd_update_index(int argc, const char **argv, const char *prefix) report(_("Untracked cache enabled for '%s'"), get_git_work_tree()); break; default: - die("Bug: bad untracked_cache value: %d", untracked_cache); + die("BUG: bad untracked_cache value: %d", untracked_cache); } if (active_cache_changed) { diff --git a/grep.c b/grep.c index 394c856..22cbb73 100644 --- a/grep.c +++ b/grep.c @@ -693,10 +693,10 @@ static struct grep_expr *prep_header_patterns(struct grep_opt *opt) for (p = opt->header_list; p; p = p->next) { if (p->token != GREP_PATTERN_HEAD) - die("bug: a non-header pattern in grep header list."); + die("BUG: a non-header pattern in grep header list."); if (p->field < GREP_HEADER_FIELD_MIN || GREP_HEADER_FIELD_MAX <= p->field) - die("bug: unknown header field %d", p->field); + die("BUG: unknown header field %d", p->field); compile_regexp(p, opt); } @@ -709,7 +709,7 @@ static struct grep_expr *prep_header_patterns(struct grep_opt *opt) h = compile_pattern_atom(&pp); if (!h || pp != p->next) - die("bug: malformed header expr"); + die("BUG: malformed header expr"); if (!header_group[p->field]) { header_group[p->field] = h; continue; @@ -1514,7 +1514,7 @@ static int grep_source_1(struct grep_opt *opt, struct grep_source *gs, int colle case GREP_BINARY_TEXT: break; default: - die("bug: unknown binary handling mode"); + die("BUG: unknown binary handling mode"); } } diff --git a/imap-send.c b/imap-send.c index db0fafe..0f5f476 100644 --- a/imap-send.c +++ b/imap-send.c @@ -511,7 +511,7 @@ static int nfsnprintf(char *buf, int blen, const char *fmt, ...) va_start(va, fmt); if (blen <= 0 || (unsigned)(ret = vsnprintf(buf, blen, fmt, va)) >= (unsigned)blen) - die("Fatal: buffer too small. Please report a bug."); + die("BUG: buffer too small. Please report a bug."); va_end(va); return ret; } diff --git a/merge-recursive.c b/merge-recursive.c index a4a1195..4338b73 100644 --- a/merge-recursive.c +++ b/merge-recursive.c @@ -268,7 +268,7 @@ struct tree *write_tree_from_memory(struct merge_options *o) fprintf(stderr, "BUG: %d %.*s\n", ce_stage(ce), (int)ce_namelen(ce), ce->name); } - die("Bug in merge-recursive.c"); + die("BUG: unmerged index entries in merge-recursive.c"); } if (!active_cache_tree) @@ -966,9 +966,8 @@ static struct merge_file_info merge_file_1(struct merge_options *o, if (!oid_eq(&a->oid, &b->oid)) result.clean = 0; - } else { - die(_("unsupported object type in the tree")); - } + } else + die(_("BUG: unsupported object type in the tree")); } return result; @@ -1354,7 +1353,7 @@ static int process_renames(struct merge_options *o, const char *ren2_dst = ren2->pair->two->path; enum rename_type rename_type; if (strcmp(ren1_src, ren2_src) != 0) - die("ren1_src != ren2_src"); + die("BUG: ren1_src != ren2_src"); ren2->dst_entry->processed = 1; ren2->processed = 1; if (strcmp(ren1_dst, ren2_dst) != 0) { @@ -1388,7 +1387,7 @@ static int process_renames(struct merge_options *o, ren2 = lookup->util; ren2_dst = ren2->pair->two->path; if (strcmp(ren1_dst, ren2_dst) != 0) - die("ren1_dst != ren2_dst"); + die("BUG: ren1_dst != ren2_dst"); clean_merge = 0; ren2->processed = 1; @@ -1812,7 +1811,7 @@ static int process_entry(struct merge_options *o, */ remove_file(o, 1, path, !a_mode); } else - die(_("Fatal merge failure, shouldn't happen.")); + die(_("BUG: fatal merge failure, shouldn't happen.")); return clean_merge; } @@ -1870,7 +1869,7 @@ int merge_trees(struct merge_options *o, for (i = 0; i < entries->nr; i++) { struct stage_data *e = entries->items[i].util; if (!e->processed) - die(_("Unprocessed path??? %s"), + die(_("BUG: unprocessed path??? %s"), entries->items[i].string); } diff --git a/sha1_file.c b/sha1_file.c index d5e1121..5085fe0 100644 --- a/sha1_file.c +++ b/sha1_file.c @@ -795,7 +795,7 @@ void close_all_packs(void) for (p = packed_git; p; p = p->next) if (p->do_not_close) - die("BUG! Want to close pack marked 'do-not-close'"); + die("BUG: want to close pack marked 'do-not-close'"); else close_pack(p); } @@ -2330,7 +2330,7 @@ void *unpack_entry(struct packed_git *p, off_t obj_offset, case OBJ_OFS_DELTA: case OBJ_REF_DELTA: if (data) - die("BUG in unpack_entry: left loop at a valid delta"); + die("BUG: unpack_entry: left loop at a valid delta"); break; case OBJ_COMMIT: case OBJ_TREE: diff --git a/trailer.c b/trailer.c index 8e48a5c..c6ea9ac 100644 --- a/trailer.c +++ b/trailer.c @@ -562,7 +562,7 @@ static int git_trailer_config(const char *conf_key, const char *value, void *cb) warning(_("unknown value '%s' for key '%s'"), value, conf_key); break; default: - die("internal bug in trailer.c"); + die("BUG: trailer.c: unhandled type %d", type); } return 0; } diff --git a/transport.c b/transport.c index b233e3e..04d9454 100644 --- a/transport.c +++ b/transport.c @@ -566,7 +566,7 @@ void transport_take_over(struct transport *transport, struct git_transport_data *data; if (!transport->smart_options) - die("Bug detected: Taking over transport requires non-NULL " + die("BUG: taking over transport requires non-NULL " "smart_options field."); data = xcalloc(1, sizeof(*data)); diff --git a/wt-status.c b/wt-status.c index 19cbc39..f8ae0c2 100644 --- a/wt-status.c +++ b/wt-status.c @@ -263,7 +263,7 @@ static const char *wt_status_unmerged_status_string(int stagemask) case 7: return _("both modified:"); default: - die("bug: unhandled unmerged status %x", stagemask); + die("BUG: unhandled unmerged status %x", stagemask); } } @@ -388,7 +388,7 @@ static void wt_status_print_change_data(struct wt_status *s, status_printf(s, color(WT_STATUS_HEADER, s), "\t"); what = wt_status_diff_status_string(status); if (!what) - die("bug: unhandled diff status %c", status); + die("BUG: unhandled diff status %c", status); len = label_width - utf8_strwidth(what); assert(len >= 0); if (status == DIFF_STATUS_COPIED || status == DIFF_STATUS_RENAMED) -- cgit v0.10.2-6-g49f6 From 7e97e1003349107b9fdc388bb4aa006fae9ab560 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Tue, 26 Jul 2016 18:05:53 +0200 Subject: die(_("BUG")): avoid translating bug messages While working on the patch series that avoids die()ing in recursive merges, the issue came up that bug reports (i.e. die("BUG: ...") constructs) should never be translated, as the target audience is the Git developer community, not necessarily the current user, and hence a translated message would make it *harder* to address the problem. So let's stop translating the obvious ones. As it is really, really outside the purview of this patch series to see whether there are more die() statements that report bugs and are currently translated, that task is left for another day and patch. Signed-off-by: Johannes Schindelin Signed-off-by: Junio C Hamano diff --git a/merge-recursive.c b/merge-recursive.c index 4338b73..1b6db87 100644 --- a/merge-recursive.c +++ b/merge-recursive.c @@ -967,7 +967,7 @@ static struct merge_file_info merge_file_1(struct merge_options *o, if (!oid_eq(&a->oid, &b->oid)) result.clean = 0; } else - die(_("BUG: unsupported object type in the tree")); + die("BUG: unsupported object type in the tree"); } return result; @@ -1811,7 +1811,7 @@ static int process_entry(struct merge_options *o, */ remove_file(o, 1, path, !a_mode); } else - die(_("BUG: fatal merge failure, shouldn't happen.")); + die("BUG: fatal merge failure, shouldn't happen."); return clean_merge; } @@ -1869,7 +1869,7 @@ int merge_trees(struct merge_options *o, for (i = 0; i < entries->nr; i++) { struct stage_data *e = entries->items[i].util; if (!e->processed) - die(_("BUG: unprocessed path??? %s"), + die("BUG: unprocessed path??? %s", entries->items[i].string); } -- cgit v0.10.2-6-g49f6 From f8d83fb66c653fff0541067a0b5a0821d3f548f9 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Tue, 26 Jul 2016 18:05:57 +0200 Subject: merge-recursive: clarify code in was_tracked() It can be puzzling to see that was_tracked() asks to get an index entry by name, but does not take a negative return value for an answer. The reason we have to do this is that cache_name_pos() only looks for entries in stage 0, even if nobody asked for any stage in particular. Let's rewrite the logic a little bit, to handle the easy case early: if cache_name_pos() returned a non-negative position, we know it is a match, and we do not even have to compare the name again (cache_name_pos() did that for us already). We can say right away: yes, this file was tracked. Only if there was no exact match do we need to look harder for any matching entry in stage 2. Signed-off-by: Johannes Schindelin Signed-off-by: Junio C Hamano diff --git a/merge-recursive.c b/merge-recursive.c index 1b6db87..3a652b7 100644 --- a/merge-recursive.c +++ b/merge-recursive.c @@ -667,23 +667,21 @@ static int was_tracked(const char *path) { int pos = cache_name_pos(path, strlen(path)); - if (pos < 0) - pos = -1 - pos; - while (pos < active_nr && - !strcmp(path, active_cache[pos]->name)) { - /* - * If stage #0, it is definitely tracked. - * If it has stage #2 then it was tracked - * before this merge started. All other - * cases the path was not tracked. - */ - switch (ce_stage(active_cache[pos])) { - case 0: - case 2: + if (0 <= pos) + /* we have been tracking this path */ + return 1; + + /* + * Look for an unmerged entry for the path, + * specifically stage #2, which would indicate + * that "our" side before the merge started + * had the path tracked (and resulted in a conflict). + */ + for (pos = -1 - pos; + pos < active_nr && !strcmp(path, active_cache[pos]->name); + pos++) + if (ce_stage(active_cache[pos]) == 2) return 1; - } - pos++; - } return 0; } -- cgit v0.10.2-6-g49f6 From f241ff0d0a9ddb84b2f673a1b7a92fea0d6add3a Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Tue, 26 Jul 2016 18:06:02 +0200 Subject: prepare the builtins for a libified merge_recursive() Previously, callers of merge_trees() or merge_recursive() expected that code to die() with an error message. This used to be okay because we called those commands from scripts, and had a chance to print out a message in case the command failed fatally (read: with exit code 128). As scripting incurs its own set of problems (portability, speed, idiosyncrasies of different shells, limited data structures leading to inefficient code), we are converting more and more of these scripts into builtins, using library functions directly. We already tried to use merge_recursive() directly in the builtin git-am, for example. Unfortunately, we had to roll it back temporarily because some of the code in merge-recursive.c still deemed it okay to call die(), when the builtin am code really wanted to print out a useful advice after the merge failed fatally. In the next commits, we want to fix that. The code touched by this commit expected merge_trees() to die() with some useful message when there is an error condition, but merge_trees() is going to be improved by converting all die() calls to return error() instead (i.e. return value -1 after printing out the message as before), so that the caller can react more flexibly. This is a step to prepare for the version of merge_trees() that no longer dies, even if we just imitate the previous behavior by calling exit(128): this is what callers of e.g. `git merge` have come to expect. Note that the callers of the sequencer (revert and cherry-pick) already fail fast even for the return value -1; The only difference is that they now get a chance to say " failed". A caller of merge_trees() might want handle error messages themselves (or even suppress them). As this patch is already complex enough, we leave that change for a later patch. Signed-off-by: Johannes Schindelin Signed-off-by: Junio C Hamano diff --git a/builtin/checkout.c b/builtin/checkout.c index 27c1a05..07dea3b 100644 --- a/builtin/checkout.c +++ b/builtin/checkout.c @@ -567,8 +567,10 @@ static int merge_working_tree(const struct checkout_opts *opts, o.ancestor = old->name; o.branch1 = new->name; o.branch2 = "local"; - merge_trees(&o, new->commit->tree, work, + ret = merge_trees(&o, new->commit->tree, work, old->commit->tree, &result); + if (ret < 0) + exit(128); ret = reset_tree(new->commit->tree, opts, 0, writeout_error); if (ret) diff --git a/builtin/merge.c b/builtin/merge.c index 19b3bc2..148a9a5 100644 --- a/builtin/merge.c +++ b/builtin/merge.c @@ -673,6 +673,8 @@ static int try_merge_strategy(const char *strategy, struct commit_list *common, hold_locked_index(&lock, 1); clean = merge_recursive(&o, head, remoteheads->item, reversed, &result); + if (clean < 0) + exit(128); if (active_cache_changed && write_locked_index(&the_index, &lock, COMMIT_LOCK)) die (_("unable to write %s"), get_index_file()); diff --git a/sequencer.c b/sequencer.c index cdfac82..286a435 100644 --- a/sequencer.c +++ b/sequencer.c @@ -293,6 +293,8 @@ static int do_recursive_merge(struct commit *base, struct commit *next, clean = merge_trees(&o, head_tree, next_tree, base_tree, &result); + if (clean < 0) + return clean; if (active_cache_changed && write_locked_index(&the_index, &index_lock, COMMIT_LOCK)) @@ -559,6 +561,8 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts) if (!opts->strategy || !strcmp(opts->strategy, "recursive") || opts->action == REPLAY_REVERT) { res = do_recursive_merge(base, next, base_label, next_label, head, &msgbuf, opts); + if (res < 0) + return res; write_message(&msgbuf, git_path_merge_msg()); } else { struct commit_list *common = NULL; -- cgit v0.10.2-6-g49f6 From de8946de1694a8cf311daab7b2c416d76cb04d23 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Tue, 26 Jul 2016 18:06:07 +0200 Subject: merge_recursive: abort properly upon errors There are a couple of places where return values never indicated errors before, as we simply died instead of returning. But now negative return values mean that there was an error and we have to abort the operation. Let's do exactly that. Signed-off-by: Johannes Schindelin Signed-off-by: Junio C Hamano diff --git a/merge-recursive.c b/merge-recursive.c index 3a652b7..58ced25 100644 --- a/merge-recursive.c +++ b/merge-recursive.c @@ -1949,17 +1949,19 @@ int merge_recursive(struct merge_options *o, /* * When the merge fails, the result contains files * with conflict markers. The cleanness flag is - * ignored, it was never actually used, as result of - * merge_trees has always overwritten it: the committed - * "conflicts" were already resolved. + * ignored (unless indicating an error), it was never + * actually used, as result of merge_trees has always + * overwritten it: the committed "conflicts" were + * already resolved. */ discard_cache(); saved_b1 = o->branch1; saved_b2 = o->branch2; o->branch1 = "Temporary merge branch 1"; o->branch2 = "Temporary merge branch 2"; - merge_recursive(o, merged_common_ancestors, iter->item, - NULL, &merged_common_ancestors); + if (merge_recursive(o, merged_common_ancestors, iter->item, + NULL, &merged_common_ancestors) < 0) + return -1; o->branch1 = saved_b1; o->branch2 = saved_b2; o->call_depth--; @@ -1975,6 +1977,8 @@ int merge_recursive(struct merge_options *o, o->ancestor = "merged common ancestors"; clean = merge_trees(o, h1->tree, h2->tree, merged_common_ancestors->tree, &mrtree); + if (clean < 0) + return clean; if (o->call_depth) { *result = make_virtual_commit(mrtree, "merged tree"); @@ -2031,6 +2035,9 @@ int merge_recursive_generic(struct merge_options *o, hold_locked_index(lock, 1); clean = merge_recursive(o, head_commit, next_commit, ca, result); + if (clean < 0) + return clean; + if (active_cache_changed && write_locked_index(&the_index, lock, COMMIT_LOCK)) return error(_("Unable to write index.")); -- cgit v0.10.2-6-g49f6 From 3c8a51e89a90cb2a4016b1c30a10c7245bbdbeda Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Tue, 26 Jul 2016 18:06:10 +0200 Subject: merge-recursive: avoid returning a wholesale struct It is technically allowed, as per C89, for functions' return type to be complete structs (i.e. *not* just pointers to structs). However, it was just an oversight of this developer when converting Python code to C code in 6d297f8 (Status update on merge-recursive in C, 2006-07-08) which introduced such a return type. Besides, by converting this construct to pass in the struct, we can now start returning a value that can indicate errors in future patches. This will help the current effort to libify merge-recursive.c. Signed-off-by: Johannes Schindelin Signed-off-by: Junio C Hamano diff --git a/merge-recursive.c b/merge-recursive.c index 58ced25..2be1e17 100644 --- a/merge-recursive.c +++ b/merge-recursive.c @@ -894,47 +894,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; - oidcpy(&result.oid, &a->oid); + result->mode = a->mode; + oidcpy(&result->oid, &a->oid); } else { - result.mode = b->mode; - oidcpy(&result.oid, &b->oid); + result->mode = b->mode; + oidcpy(&result->oid, &b->oid); } } else { if (!oid_eq(&a->oid, &one->oid) && !oid_eq(&b->oid, &one->oid)) - 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 (oid_eq(&a->oid, &b->oid) || oid_eq(&a->oid, &one->oid)) - oidcpy(&result.oid, &b->oid); + oidcpy(&result->oid, &b->oid); else if (oid_eq(&b->oid, &one->oid)) - oidcpy(&result.oid, &a->oid); + oidcpy(&result->oid, &a->oid); else if (S_ISREG(a->mode)) { mmbuffer_t result_buf; int merge_status; @@ -946,64 +946,66 @@ 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.oid.hash)) + blob_type, result->oid.hash)) 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.oid.hash, + result->clean = merge_submodule(result->oid.hash, one->path, one->oid.hash, a->oid.hash, b->oid.hash, !o->call_depth); } else if (S_ISLNK(a->mode)) { - oidcpy(&result.oid, &a->oid); + oidcpy(&result->oid, &a->oid); if (!oid_eq(&a->oid, &b->oid)) - 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 struct object_id *o_oid, int o_mode, const struct object_id *a_oid, int a_mode, const struct object_id *b_oid, int b_mode, const char *branch1, - const char *branch2) + const char *branch2, + struct merge_file_info *mfi) { struct diff_filespec one, a, b; @@ -1014,7 +1016,7 @@ static struct merge_file_info merge_file_one(struct merge_options *o, a.mode = a_mode; oidcpy(&b.oid, b_oid); 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, @@ -1187,11 +1189,12 @@ 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->oid, one->mode, &a->oid, a->mode, &b->oid, b->mode, - ci->branch1, ci->branch2); + ci->branch1, ci->branch2, &mfi)) + return; /* * FIXME: For rename/add-source conflicts (if we could detect * such), this is wrong. We should instead find a unique @@ -1245,12 +1248,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, - o->branch1, c1->path, - o->branch2, ci->ren1_other.path); - mfi_c2 = merge_file_special_markers(o, b, &ci->ren2_other, c2, - o->branch1, ci->ren2_other.path, - o->branch2, c2->path); + if (merge_file_special_markers(o, a, c1, &ci->ren1_other, + o->branch1, c1->path, + o->branch2, ci->ren1_other.path, &mfi_c1) || + merge_file_special_markers(o, b, &ci->ren2_other, c2, + o->branch1, ci->ren2_other.path, + o->branch2, c2->path, &mfi_c2)) + return; if (o->call_depth) { /* @@ -1473,12 +1477,13 @@ 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_oid, 0, - &ren1->pair->two->oid, - ren1->pair->two->mode, - &dst_other.oid, - dst_other.mode, - branch1, branch2); + if (merge_file_one(o, ren1_dst, &null_oid, 0, + &ren1->pair->two->oid, + ren1->pair->two->mode, + &dst_other.oid, + dst_other.mode, + branch1, branch2, &mfi)) + return -1; output(o, 1, _("Adding merged %s"), ren1_dst); update_file(o, 0, &mfi.oid, mfi.mode, ren1_dst); @@ -1636,9 +1641,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, - o->branch1, path1, - o->branch2, path2); + if (merge_file_special_markers(o, &one, &a, &b, + o->branch1, path1, + o->branch2, path2, &mfi)) + return -1; if (mfi.clean && !df_conflict_remains && oid_eq(&mfi.oid, a_oid) && mfi.mode == a_mode) { -- cgit v0.10.2-6-g49f6 From fbc87eb54409b5c7b080f1c23b767daefc9d6ede Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Tue, 26 Jul 2016 18:06:17 +0200 Subject: merge-recursive: allow write_tree_from_memory() to error out It is possible that a tree cannot be written (think: disk full). We will want to give the caller a chance to clean up instead of letting the program die() in such a case. Signed-off-by: Johannes Schindelin Signed-off-by: Junio C Hamano diff --git a/merge-recursive.c b/merge-recursive.c index 2be1e17..1f86338 100644 --- a/merge-recursive.c +++ b/merge-recursive.c @@ -1888,8 +1888,8 @@ int merge_trees(struct merge_options *o, else clean = 1; - if (o->call_depth) - *result = write_tree_from_memory(o); + if (o->call_depth && !(*result = write_tree_from_memory(o))) + return -1; return clean; } -- cgit v0.10.2-6-g49f6 From 75456f96d4bd201f815484e9d2ff2ae429305ab5 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Tue, 26 Jul 2016 18:06:21 +0200 Subject: merge-recursive: handle return values indicating errors We are about to libify the recursive merge machinery, where we only die() in case of a bug or memory contention. To that end, we must heed negative return values as indicating errors. This requires our functions to be careful to pass through error conditions in call chains, and for quite a few functions this means that they have to return values to begin with. The next step will be to convert the places where we currently die() to return negative values (read: -1) instead. Note that we ignore errors reported by make_room_for_path(), consistent with the previous behavior (update_file_flags() used the return value of make_room_for_path() only to indicate an early return, but not a fatal error): if the error is really a fatal error, we will notice later; If not, it was not that serious a problem to begin with. (Witnesses in favor of this reasoning are t4151-am-abort and t7610-mergetool, which would start failing if we stopped on errors reported by make_room_for_path()). Also note: while this patch makes the code slightly less readable in update_file_flags() (we introduce a new "goto free_buf;" instead of an explicit "free(buf); return;"), it is a preparatory change for the next patch where we will convert all of the die() calls in the same function to go through the free_buf return path instead. Signed-off-by: Johannes Schindelin Signed-off-by: Junio C Hamano diff --git a/merge-recursive.c b/merge-recursive.c index 1f86338..6beb1e4 100644 --- a/merge-recursive.c +++ b/merge-recursive.c @@ -742,12 +742,12 @@ 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, - const struct object_id *oid, - unsigned mode, - const char *path, - int update_cache, - int update_wd) +static int update_file_flags(struct merge_options *o, + const struct object_id *oid, + unsigned mode, + const char *path, + int update_cache, + int update_wd) { if (o->call_depth) update_wd = 0; @@ -783,8 +783,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; @@ -807,20 +806,22 @@ static void update_file_flags(struct merge_options *o, } else die(_("do not know what to do with %06o %s '%s'"), mode, oid_to_hex(oid), path); + free_buf: free(buf); } update_index: if (update_cache) add_cacheinfo(mode, oid, path, 0, update_wd, ADD_CACHE_OK_TO_ADD); + return 0; } -static void update_file(struct merge_options *o, - int clean, - const struct object_id *oid, - unsigned mode, - const char *path) +static int update_file(struct merge_options *o, + int clean, + const struct object_id *oid, + unsigned mode, + const char *path) { - update_file_flags(o, oid, mode, path, o->call_depth || clean, !o->call_depth); + return update_file_flags(o, oid, mode, path, o->call_depth || clean, !o->call_depth); } /* Low level file merging, update and removal */ @@ -1019,7 +1020,7 @@ static int merge_file_one(struct merge_options *o, return merge_file_1(o, &one, &a, &b, branch1, branch2, mfi); } -static void handle_change_delete(struct merge_options *o, +static int handle_change_delete(struct merge_options *o, const char *path, const struct object_id *o_oid, int o_mode, const struct object_id *a_oid, int a_mode, @@ -1027,6 +1028,7 @@ static void handle_change_delete(struct merge_options *o, const char *change, const char *change_past) { char *renamed = NULL; + int ret = 0; if (dir_in_way(path, !o->call_depth)) { renamed = unique_path(o, path, a_oid ? o->branch1 : o->branch2); } @@ -1037,21 +1039,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_oid, o_mode, renamed ? renamed : path); + ret = remove_file_from_cache(path); + if (!ret) + ret = update_file(o, 0, o_oid, o_mode, + renamed ? renamed : path); } else if (!a_oid) { if (!renamed) { output(o, 1, _("CONFLICT (%s/delete): %s deleted in %s " "and %s in %s. Version %s of %s left in tree."), change, path, o->branch1, change_past, o->branch2, o->branch2, path); - update_file(o, 0, b_oid, b_mode, path); + ret = update_file(o, 0, b_oid, b_mode, path); } else { output(o, 1, _("CONFLICT (%s/delete): %s deleted in %s " "and %s in %s. Version %s of %s left in tree at %s."), change, path, o->branch1, change_past, o->branch2, o->branch2, path, renamed); - update_file(o, 0, b_oid, b_mode, renamed); + ret = update_file(o, 0, b_oid, b_mode, renamed); } } else { if (!renamed) { @@ -1064,7 +1068,7 @@ static void handle_change_delete(struct merge_options *o, "and %s in %s. Version %s of %s left in tree at %s."), change, path, o->branch2, change_past, o->branch1, o->branch1, path, renamed); - update_file(o, 0, a_oid, a_mode, renamed); + ret = update_file(o, 0, a_oid, a_mode, renamed); } /* * No need to call update_file() on path when !renamed, since @@ -1074,9 +1078,11 @@ static void handle_change_delete(struct merge_options *o, */ } free(renamed); + + return ret; } -static void conflict_rename_delete(struct merge_options *o, +static int conflict_rename_delete(struct merge_options *o, struct diff_filepair *pair, const char *rename_branch, const char *other_branch) @@ -1096,21 +1102,20 @@ static void conflict_rename_delete(struct merge_options *o, b_mode = dest->mode; } - handle_change_delete(o, - o->call_depth ? orig->path : dest->path, - &orig->oid, orig->mode, - a_oid, a_mode, - b_oid, b_mode, - _("rename"), _("renamed")); - - if (o->call_depth) { - remove_file_from_cache(dest->path); - } else { - update_stages(dest->path, NULL, - rename_branch == o->branch1 ? dest : NULL, - rename_branch == o->branch1 ? NULL : dest); - } + if (handle_change_delete(o, + o->call_depth ? orig->path : dest->path, + &orig->oid, orig->mode, + a_oid, a_mode, + b_oid, b_mode, + _("rename"), _("renamed"))) + return -1; + if (o->call_depth) + return remove_file_from_cache(dest->path); + else + return update_stages(dest->path, NULL, + rename_branch == o->branch1 ? dest : NULL, + rename_branch == o->branch1 ? NULL : dest); } static struct diff_filespec *filespec_from_entry(struct diff_filespec *target, @@ -1126,7 +1131,7 @@ static struct diff_filespec *filespec_from_entry(struct diff_filespec *target, return target; } -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) @@ -1136,6 +1141,7 @@ static void handle_file(struct merge_options *o, const char *cur_branch, *other_branch; struct diff_filespec other; struct diff_filespec *add; + int ret; if (stage == 2) { dst_entry = ci->dst_entry1; @@ -1150,7 +1156,8 @@ static void handle_file(struct merge_options *o, add = filespec_from_entry(&other, dst_entry, stage ^ 1); if (add) { char *add_name = unique_path(o, rename->path, other_branch); - update_file(o, 0, &add->oid, add->mode, add_name); + if (update_file(o, 0, &add->oid, add->mode, add_name)) + return -1; remove_file(o, 0, rename->path, 0); dst_name = unique_path(o, rename->path, cur_branch); @@ -1161,17 +1168,20 @@ static void handle_file(struct merge_options *o, rename->path, other_branch, dst_name); } } - update_file(o, 0, &rename->oid, rename->mode, dst_name); - if (stage == 2) - update_stages(rename->path, NULL, rename, add); + if ((ret = update_file(o, 0, &rename->oid, rename->mode, dst_name))) + ; /* fall through, do allow dst_name to be released */ + else if (stage == 2) + ret = update_stages(rename->path, NULL, rename, add); else - update_stages(rename->path, NULL, add, rename); + ret = update_stages(rename->path, NULL, add, rename); if (dst_name != rename->path) free(dst_name); + + return ret; } -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) { /* One file was renamed in both branches, but to different names. */ @@ -1194,14 +1204,16 @@ static void conflict_rename_rename_1to2(struct merge_options *o, &a->oid, a->mode, &b->oid, b->mode, ci->branch1, ci->branch2, &mfi)) - return; + return -1; + /* * FIXME: For rename/add-source conflicts (if we could detect * such), this is wrong. We should instead find a unique * 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.oid, mfi.mode, one->path); + if (update_file(o, 0, &mfi.oid, mfi.mode, one->path)) + return -1; /* * Above, we put the merged content at the merge-base's @@ -1212,22 +1224,26 @@ 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->oid, add->mode, a->path); + if (add) { + if (update_file(o, 0, &add->oid, add->mode, a->path)) + return -1; + } else remove_file_from_cache(a->path); add = filespec_from_entry(&other, ci->dst_entry2, 3 ^ 1); - if (add) - update_file(o, 0, &add->oid, add->mode, b->path); + if (add) { + if (update_file(o, 0, &add->oid, add->mode, b->path)) + return -1; + } else remove_file_from_cache(b->path); - } else { - handle_file(o, a, 2, ci); - handle_file(o, b, 3, ci); - } + } else if (handle_file(o, a, 2, ci) || handle_file(o, b, 3, ci)) + return -1; + + return 0; } -static void conflict_rename_rename_2to1(struct merge_options *o, +static int conflict_rename_rename_2to1(struct merge_options *o, struct rename_conflict_info *ci) { /* Two files, a & b, were renamed to the same thing, c. */ @@ -1238,6 +1254,7 @@ static void conflict_rename_rename_2to1(struct merge_options *o, char *path = c1->path; /* == c2->path */ struct merge_file_info mfi_c1; struct merge_file_info mfi_c2; + int ret; output(o, 1, _("CONFLICT (rename/rename): " "Rename %s->%s in %s. " @@ -1254,7 +1271,7 @@ static void conflict_rename_rename_2to1(struct merge_options *o, merge_file_special_markers(o, b, &ci->ren2_other, c2, o->branch1, ci->ren2_other.path, o->branch2, c2->path, &mfi_c2)) - return; + return -1; if (o->call_depth) { /* @@ -1265,19 +1282,25 @@ static void conflict_rename_rename_2to1(struct merge_options *o, * again later for the non-recursive merge. */ remove_file(o, 0, path, 0); - update_file(o, 0, &mfi_c1.oid, mfi_c1.mode, a->path); - update_file(o, 0, &mfi_c2.oid, mfi_c2.mode, b->path); + ret = update_file(o, 0, &mfi_c1.oid, mfi_c1.mode, a->path); + if (!ret) + ret = update_file(o, 0, &mfi_c2.oid, mfi_c2.mode, + b->path); } else { char *new_path1 = unique_path(o, path, ci->branch1); char *new_path2 = unique_path(o, path, ci->branch2); output(o, 1, _("Renaming %s to %s and %s to %s instead"), a->path, new_path1, b->path, new_path2); remove_file(o, 0, path, 0); - update_file(o, 0, &mfi_c1.oid, mfi_c1.mode, new_path1); - update_file(o, 0, &mfi_c2.oid, mfi_c2.mode, new_path2); + ret = update_file(o, 0, &mfi_c1.oid, mfi_c1.mode, new_path1); + if (!ret) + ret = update_file(o, 0, &mfi_c2.oid, mfi_c2.mode, + new_path2); free(new_path2); free(new_path1); } + + return ret; } static int process_renames(struct merge_options *o, @@ -1462,12 +1485,13 @@ static int process_renames(struct merge_options *o, * update_file_flags() instead of * update_file(). */ - update_file_flags(o, - &ren1->pair->two->oid, - ren1->pair->two->mode, - ren1_dst, - 1, /* update_cache */ - 0 /* update_wd */); + if (update_file_flags(o, + &ren1->pair->two->oid, + ren1->pair->two->mode, + ren1_dst, + 1, /* update_cache */ + 0 /* update_wd */)) + clean_merge = -1; } else if (!oid_eq(&dst_other.oid, &null_oid)) { clean_merge = 0; try_merge = 1; @@ -1482,22 +1506,28 @@ static int process_renames(struct merge_options *o, ren1->pair->two->mode, &dst_other.oid, dst_other.mode, - branch1, branch2, &mfi)) - return -1; + branch1, branch2, &mfi)) { + clean_merge = -1; + goto cleanup_and_return; + } output(o, 1, _("Adding merged %s"), ren1_dst); - update_file(o, 0, &mfi.oid, - mfi.mode, ren1_dst); + if (update_file(o, 0, &mfi.oid, + mfi.mode, ren1_dst)) + clean_merge = -1; try_merge = 0; } else { char *new_path = unique_path(o, ren1_dst, branch2); output(o, 1, _("Adding as %s instead"), new_path); - update_file(o, 0, &dst_other.oid, - dst_other.mode, new_path); + if (update_file(o, 0, &dst_other.oid, + dst_other.mode, new_path)) + clean_merge = -1; free(new_path); } } else try_merge = 1; + if (clean_merge < 0) + goto cleanup_and_return; if (try_merge) { struct diff_filespec *one, *a, *b; src_other.path = (char *)ren1_src; @@ -1524,6 +1554,7 @@ static int process_renames(struct merge_options *o, } } } +cleanup_and_return: string_list_clear(&a_by_dst, 0); string_list_clear(&b_by_dst, 0); @@ -1586,18 +1617,18 @@ error_return: return ret; } -static void handle_modify_delete(struct merge_options *o, +static int handle_modify_delete(struct merge_options *o, const char *path, struct object_id *o_oid, int o_mode, struct object_id *a_oid, int a_mode, struct object_id *b_oid, int b_mode) { - handle_change_delete(o, - path, - o_oid, o_mode, - a_oid, a_mode, - b_oid, b_mode, - _("modify"), _("modified")); + return handle_change_delete(o, + path, + o_oid, o_mode, + a_oid, a_mode, + b_oid, b_mode, + _("modify"), _("modified")); } static int merge_content(struct merge_options *o, @@ -1671,7 +1702,8 @@ static int merge_content(struct merge_options *o, output(o, 1, _("CONFLICT (%s): Merge conflict in %s"), reason, path); if (rename_conflict_info && !df_conflict_remains) - update_stages(path, &one, &a, &b); + if (update_stages(path, &one, &a, &b)) + return -1; } if (df_conflict_remains) { @@ -1679,30 +1711,33 @@ static int merge_content(struct merge_options *o, if (o->call_depth) { remove_file_from_cache(path); } else { - if (!mfi.clean) - update_stages(path, &one, &a, &b); - else { + if (!mfi.clean) { + if (update_stages(path, &one, &a, &b)) + return -1; + } else { int file_from_stage2 = was_tracked(path); struct diff_filespec merged; oidcpy(&merged.oid, &mfi.oid); merged.mode = mfi.mode; - update_stages(path, NULL, - file_from_stage2 ? &merged : NULL, - file_from_stage2 ? NULL : &merged); + if (update_stages(path, NULL, + file_from_stage2 ? &merged : NULL, + file_from_stage2 ? NULL : &merged)) + return -1; } } new_path = unique_path(o, path, rename_conflict_info->branch1); output(o, 1, _("Adding as %s instead"), new_path); - update_file(o, 0, &mfi.oid, mfi.mode, new_path); + if (update_file(o, 0, &mfi.oid, mfi.mode, new_path)) { + free(new_path); + return -1; + } free(new_path); mfi.clean = 0; - } else { - update_file(o, mfi.clean, &mfi.oid, mfi.mode, path); - } + } else if (update_file(o, mfi.clean, &mfi.oid, mfi.mode, path)) + return -1; return mfi.clean; - } /* Per entry merge function */ @@ -1730,17 +1765,21 @@ static int process_entry(struct merge_options *o, break; case RENAME_DELETE: clean_merge = 0; - conflict_rename_delete(o, conflict_info->pair1, - conflict_info->branch1, - conflict_info->branch2); + if (conflict_rename_delete(o, + conflict_info->pair1, + conflict_info->branch1, + conflict_info->branch2)) + clean_merge = -1; break; case RENAME_ONE_FILE_TO_TWO: clean_merge = 0; - conflict_rename_rename_1to2(o, conflict_info); + if (conflict_rename_rename_1to2(o, conflict_info)) + clean_merge = -1; break; case RENAME_TWO_FILES_TO_ONE: clean_merge = 0; - conflict_rename_rename_2to1(o, conflict_info); + if (conflict_rename_rename_2to1(o, conflict_info)) + clean_merge = -1; break; default: entry->processed = 0; @@ -1760,8 +1799,9 @@ static int process_entry(struct merge_options *o, } else { /* Modify/delete; deleted side may have put a directory in the way */ clean_merge = 0; - handle_modify_delete(o, path, o_oid, o_mode, - a_oid, a_mode, b_oid, b_mode); + if (handle_modify_delete(o, path, o_oid, o_mode, + a_oid, a_mode, b_oid, b_mode)) + clean_merge = -1; } } else if ((!o_oid && a_oid && !b_oid) || (!o_oid && !a_oid && b_oid)) { @@ -1793,14 +1833,16 @@ static int process_entry(struct merge_options *o, output(o, 1, _("CONFLICT (%s): There is a directory with name %s in %s. " "Adding %s as %s"), conf, path, other_branch, path, new_path); - update_file(o, 0, oid, mode, new_path); - if (o->call_depth) + if (update_file(o, 0, oid, mode, new_path)) + clean_merge = -1; + else if (o->call_depth) remove_file_from_cache(path); free(new_path); } else { output(o, 2, _("Adding %s"), path); /* do not overwrite file if already present */ - update_file_flags(o, oid, mode, path, 1, !a_oid); + if (update_file_flags(o, oid, mode, path, 1, !a_oid)) + clean_merge = -1; } } else if (a_oid && b_oid) { /* Case C: Added in both (check for same permissions) and */ @@ -1863,12 +1905,18 @@ int merge_trees(struct merge_options *o, re_head = get_renames(o, head, common, head, merge, entries); re_merge = get_renames(o, merge, common, head, merge, entries); clean = process_renames(o, re_head, re_merge); + if (clean < 0) + return clean; for (i = entries->nr-1; 0 <= i; i--) { const char *path = entries->items[i].string; struct stage_data *e = entries->items[i].util; - if (!e->processed - && !process_entry(o, path, e)) - clean = 0; + if (!e->processed) { + int ret = process_entry(o, path, e); + if (!ret) + clean = 0; + else if (ret < 0) + return ret; + } } for (i = 0; i < entries->nr; i++) { struct stage_data *e = entries->items[i].util; -- cgit v0.10.2-6-g49f6 From 6003303a1e5086a9f28f18469a8a72ad22f75c86 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Tue, 26 Jul 2016 18:06:26 +0200 Subject: merge-recursive: switch to returning errors instead of dying The recursive merge machinery is supposed to be a library function, i.e. it should return an error when it fails. Originally the functions were part of the builtin "merge-recursive", though, where it was simpler to call die() and be done with error handling. The existing callers were already prepared to detect negative return values to indicate errors and to behave as previously: exit with code 128 (which is the same thing that die() does, after printing the message). Signed-off-by: Johannes Schindelin Signed-off-by: Junio C Hamano diff --git a/merge-recursive.c b/merge-recursive.c index 6beb1e4..bc59815 100644 --- a/merge-recursive.c +++ b/merge-recursive.c @@ -275,8 +275,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; + } result = lookup_tree(active_cache_tree->sha1); @@ -716,12 +718,10 @@ static int make_room_for_path(struct merge_options *o, const char *path) /* Make sure leading directories are created */ status = safe_create_leading_directories_const(path); if (status) { - if (status == SCLD_EXISTS) { + if (status == SCLD_EXISTS) /* something else exists */ - error(msg, path, _(": perhaps a D/F conflict?")); - return -1; - } - die(msg, path, ""); + return error(msg, path, _(": perhaps a D/F conflict?")); + return error(msg, path, ""); } /* @@ -749,6 +749,8 @@ static int update_file_flags(struct merge_options *o, int update_cache, int update_wd) { + int ret = 0; + if (o->call_depth) update_wd = 0; @@ -769,9 +771,11 @@ static int update_file_flags(struct merge_options *o, buf = read_sha1_file(oid->hash, &type, &size); if (!buf) - die(_("cannot read object %s '%s'"), oid_to_hex(oid), path); - if (type != OBJ_BLOB) - die(_("blob expected for %s '%s'"), oid_to_hex(oid), path); + return error(_("cannot read object %s '%s'"), oid_to_hex(oid), path); + if (type != OBJ_BLOB) { + ret = error(_("blob expected for %s '%s'"), oid_to_hex(oid), path); + goto free_buf; + } if (S_ISREG(mode)) { struct strbuf strbuf = STRBUF_INIT; if (convert_to_working_tree(path, buf, size, &strbuf)) { @@ -792,8 +796,11 @@ static int update_file_flags(struct merge_options *o, else mode = 0666; fd = open(path, O_WRONLY | O_TRUNC | O_CREAT, mode); - if (fd < 0) - die_errno(_("failed to open '%s'"), path); + if (fd < 0) { + ret = error_errno(_("failed to open '%s'"), + path); + goto free_buf; + } write_in_full(fd, buf, size); close(fd); } else if (S_ISLNK(mode)) { @@ -801,18 +808,18 @@ static int update_file_flags(struct merge_options *o, safe_create_leading_directories_const(path); unlink(path); if (symlink(lnk, path)) - die_errno(_("failed to symlink '%s'"), path); + ret = error_errno(_("failed to symlink '%s'"), path); free(lnk); } else - die(_("do not know what to do with %06o %s '%s'"), - mode, oid_to_hex(oid), path); + ret = error(_("do not know what to do with %06o %s '%s'"), + mode, oid_to_hex(oid), path); free_buf: free(buf); } update_index: - if (update_cache) + if (!ret && update_cache) add_cacheinfo(mode, oid, path, 0, update_wd, ADD_CACHE_OK_TO_ADD); - return 0; + return ret; } static int update_file(struct merge_options *o, @@ -938,20 +945,22 @@ static int merge_file_1(struct merge_options *o, oidcpy(&result->oid, &a->oid); else if (S_ISREG(a->mode)) { mmbuffer_t result_buf; - int merge_status; + int ret = 0, merge_status; merge_status = merge_3way(o, &result_buf, one, a, b, branch1, branch2); if ((merge_status < 0) || !result_buf.ptr) - die(_("Failed to execute internal merge")); + ret = error(_("Failed to execute internal merge")); - if (write_sha1_file(result_buf.ptr, result_buf.size, - blob_type, result->oid.hash)) - die(_("Unable to add %s to database"), - a->path); + if (!ret && write_sha1_file(result_buf.ptr, result_buf.size, + blob_type, result->oid.hash)) + ret = error(_("Unable to add %s to database"), + a->path); free(result_buf.ptr); + if (ret) + return ret; result->clean = (merge_status == 0); } else if (S_ISGITLINK(a->mode)) { result->clean = merge_submodule(result->oid.hash, @@ -1885,11 +1894,10 @@ int merge_trees(struct merge_options *o, if (code != 0) { if (show(o, 4) || o->call_depth) - die(_("merging of trees %s and %s failed"), + error(_("merging of trees %s and %s failed"), oid_to_hex(&head->object.oid), oid_to_hex(&merge->object.oid)); - else - exit(128); + return -1; } if (unmerged_cache()) { @@ -2021,7 +2029,7 @@ int merge_recursive(struct merge_options *o, o->call_depth--; if (!merged_common_ancestors) - die(_("merge returned no commit")); + return error(_("merge returned no commit")); } discard_cache(); -- cgit v0.10.2-6-g49f6 From 3f338f43b0eb8081660bbf694074a368cf07355e Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Tue, 26 Jul 2016 18:06:30 +0200 Subject: am -3: use merge_recursive() directly again Last October, we had to change this code to run `git merge-recursive` in a child process: git-am wants to print some helpful advice when the merge failed, but the code in question was not prepared to return, it die()d instead. We are finally at a point when the code *is* prepared to return errors, and can avoid the child process again. This reverts commit c63d4b2 (am -3: do not let failed merge from completing the error codepath, 2015-10-09), with the necessary changes to adjust for the fact that Git's source code changed in the meantime (such as: using OIDs instead of hashes in the recursive merge, and a removed gender bias). Note: the code now calls merge_recursive_generic() again. Unlike merge_trees() and merge_recursive(), this function returns 0 upon success, as most of Git's functions. Therefore, the error value -1 naturally is handled correctly, and we do not have to take care of it specifically. Signed-off-by: Johannes Schindelin Signed-off-by: Junio C Hamano diff --git a/builtin/am.c b/builtin/am.c index b77bf11..cfb79ea 100644 --- a/builtin/am.c +++ b/builtin/am.c @@ -1579,47 +1579,18 @@ static int build_fake_ancestor(const struct am_state *state, const char *index_f } /** - * Do the three-way merge using fake ancestor, their tree constructed - * from the fake ancestor and the postimage of the patch, and our - * state. - */ -static int run_fallback_merge_recursive(const struct am_state *state, - unsigned char *orig_tree, - unsigned char *our_tree, - unsigned char *their_tree) -{ - struct child_process cp = CHILD_PROCESS_INIT; - int status; - - cp.git_cmd = 1; - - argv_array_pushf(&cp.env_array, "GITHEAD_%s=%.*s", - sha1_to_hex(their_tree), linelen(state->msg), state->msg); - if (state->quiet) - argv_array_push(&cp.env_array, "GIT_MERGE_VERBOSITY=0"); - - argv_array_push(&cp.args, "merge-recursive"); - argv_array_push(&cp.args, sha1_to_hex(orig_tree)); - argv_array_push(&cp.args, "--"); - argv_array_push(&cp.args, sha1_to_hex(our_tree)); - argv_array_push(&cp.args, sha1_to_hex(their_tree)); - - status = run_command(&cp) ? (-1) : 0; - discard_cache(); - read_cache(); - return status; -} - -/** * Attempt a threeway merge, using index_path as the temporary index. */ static int fall_back_threeway(const struct am_state *state, const char *index_path) { - unsigned char orig_tree[GIT_SHA1_RAWSZ], their_tree[GIT_SHA1_RAWSZ], - our_tree[GIT_SHA1_RAWSZ]; + struct object_id orig_tree, their_tree, our_tree; + const struct object_id *bases[1] = { &orig_tree }; + struct merge_options o; + struct commit *result; + char *their_tree_name; - if (get_sha1("HEAD", our_tree) < 0) - hashcpy(our_tree, EMPTY_TREE_SHA1_BIN); + if (get_oid("HEAD", &our_tree) < 0) + hashcpy(our_tree.hash, EMPTY_TREE_SHA1_BIN); if (build_fake_ancestor(state, index_path)) return error("could not build fake ancestor"); @@ -1627,7 +1598,7 @@ static int fall_back_threeway(const struct am_state *state, const char *index_pa discard_cache(); read_cache_from(index_path); - if (write_index_as_tree(orig_tree, &the_index, index_path, 0, NULL)) + if (write_index_as_tree(orig_tree.hash, &the_index, index_path, 0, NULL)) return error(_("Repository lacks necessary blobs to fall back on 3-way merge.")); say(state, stdout, _("Using index info to reconstruct a base tree...")); @@ -1643,7 +1614,7 @@ static int fall_back_threeway(const struct am_state *state, const char *index_pa init_revisions(&rev_info, NULL); rev_info.diffopt.output_format = DIFF_FORMAT_NAME_STATUS; diff_opt_parse(&rev_info.diffopt, &diff_filter_str, 1, rev_info.prefix); - add_pending_sha1(&rev_info, "HEAD", our_tree, 0); + add_pending_sha1(&rev_info, "HEAD", our_tree.hash, 0); diff_setup_done(&rev_info.diffopt); run_diff_index(&rev_info, 1); } @@ -1652,7 +1623,7 @@ static int fall_back_threeway(const struct am_state *state, const char *index_pa return error(_("Did you hand edit your patch?\n" "It does not apply to blobs recorded in its index.")); - if (write_index_as_tree(their_tree, &the_index, index_path, 0, NULL)) + if (write_index_as_tree(their_tree.hash, &the_index, index_path, 0, NULL)) return error("could not write tree"); say(state, stdout, _("Falling back to patching base and 3-way merge...")); @@ -1668,11 +1639,22 @@ static int fall_back_threeway(const struct am_state *state, const char *index_pa * changes. */ - if (run_fallback_merge_recursive(state, orig_tree, our_tree, their_tree)) { + init_merge_options(&o); + + o.branch1 = "HEAD"; + their_tree_name = xstrfmt("%.*s", linelen(state->msg), state->msg); + o.branch2 = their_tree_name; + + if (state->quiet) + o.verbosity = 0; + + if (merge_recursive_generic(&o, &our_tree, &their_tree, 1, bases, &result)) { rerere(state->allow_rerere_autoupdate); + free(their_tree_name); return error(_("Failed to merge in the changes.")); } + free(their_tree_name); return 0; } -- cgit v0.10.2-6-g49f6 From bc9204d4ef6e0672389fdfb0d398fa9a39dba3d5 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Mon, 1 Aug 2016 13:44:37 +0200 Subject: merge-recursive: flush output buffer before printing error messages The data structure passed to the recursive merge machinery has a feature where the caller can ask for the output to be buffered into a strbuf, by setting the field 'buffer_output'. Previously, we died without flushing, losing accumulated output. With this patch, we show the output first, and only then print the error message. Currently, the only user of that buffering is merge_recursive() itself, to avoid the progress output to interfere. In the next patches, we will introduce a new buffer_output mode that forces merge_recursive() to retain the output buffer for further processing by the caller. If the caller asked for that, we will then also write the error messages into the output buffer. This is necessary to give the caller more control not only how to react in case of errors but also control how/if to display the error messages. Signed-off-by: Johannes Schindelin Signed-off-by: Junio C Hamano diff --git a/merge-recursive.c b/merge-recursive.c index bc59815..b972a83 100644 --- a/merge-recursive.c +++ b/merge-recursive.c @@ -23,6 +23,28 @@ #include "dir.h" #include "submodule.h" +static void flush_output(struct merge_options *o) +{ + if (o->obuf.len) { + fputs(o->obuf.buf, stdout); + strbuf_reset(&o->obuf); + } +} + +static int err(struct merge_options *o, const char *err, ...) +{ + va_list params; + + flush_output(o); + va_start(params, err); + strbuf_vaddf(&o->obuf, err, params); + va_end(params); + error("%s", o->obuf.buf); + strbuf_reset(&o->obuf); + + return -1; +} + static struct tree *shift_tree_object(struct tree *one, struct tree *two, const char *subtree_shift) { @@ -148,14 +170,6 @@ static int show(struct merge_options *o, int v) return (!o->call_depth && o->verbosity >= v) || o->verbosity >= 5; } -static void flush_output(struct merge_options *o) -{ - if (o->obuf.len) { - fputs(o->obuf.buf, stdout); - strbuf_reset(&o->obuf); - } -} - __attribute__((format (printf, 3, 4))) static void output(struct merge_options *o, int v, const char *fmt, ...) { @@ -198,7 +212,8 @@ static void output_commit_title(struct merge_options *o, struct commit *commit) } } -static int add_cacheinfo(unsigned int mode, const struct object_id *oid, +static int add_cacheinfo(struct merge_options *o, + unsigned int mode, const struct object_id *oid, const char *path, int stage, int refresh, int options) { struct cache_entry *ce; @@ -206,7 +221,7 @@ static int add_cacheinfo(unsigned int mode, const struct object_id *oid, ce = make_cache_entry(mode, oid ? oid->hash : null_sha1, path, stage, 0); if (!ce) - return error(_("addinfo_cache failed for path '%s'"), path); + return err(o, _("addinfo_cache failed for path '%s'"), path); ret = add_cache_entry(ce, options); if (refresh) { @@ -276,7 +291,7 @@ struct tree *write_tree_from_memory(struct merge_options *o) if (!cache_tree_fully_valid(active_cache_tree) && cache_tree_update(&the_index, 0) < 0) { - error(_("error building trees")); + err(o, _("error building trees")); return NULL; } @@ -544,7 +559,8 @@ static struct string_list *get_renames(struct merge_options *o, return renames; } -static int update_stages(const char *path, const struct diff_filespec *o, +static int update_stages(struct merge_options *opt, const char *path, + const struct diff_filespec *o, const struct diff_filespec *a, const struct diff_filespec *b) { @@ -563,13 +579,13 @@ static int update_stages(const char *path, const struct diff_filespec *o, if (remove_file_from_cache(path)) return -1; if (o) - if (add_cacheinfo(o->mode, &o->oid, path, 1, 0, options)) + if (add_cacheinfo(opt, o->mode, &o->oid, path, 1, 0, options)) return -1; if (a) - if (add_cacheinfo(a->mode, &a->oid, path, 2, 0, options)) + if (add_cacheinfo(opt, a->mode, &a->oid, path, 2, 0, options)) return -1; if (b) - if (add_cacheinfo(b->mode, &b->oid, path, 3, 0, options)) + if (add_cacheinfo(opt, b->mode, &b->oid, path, 3, 0, options)) return -1; return 0; } @@ -720,8 +736,8 @@ static int make_room_for_path(struct merge_options *o, const char *path) if (status) { if (status == SCLD_EXISTS) /* something else exists */ - return error(msg, path, _(": perhaps a D/F conflict?")); - return error(msg, path, ""); + return err(o, msg, path, _(": perhaps a D/F conflict?")); + return err(o, msg, path, ""); } /* @@ -729,7 +745,7 @@ static int make_room_for_path(struct merge_options *o, const char *path) * tracking it. */ if (would_lose_untracked(path)) - return error(_("refusing to lose untracked file at '%s'"), + return err(o, _("refusing to lose untracked file at '%s'"), path); /* Successful unlink is good.. */ @@ -739,7 +755,7 @@ static int make_room_for_path(struct merge_options *o, const char *path) if (errno == ENOENT) return 0; /* .. but not some other error (who really cares what?) */ - return error(msg, path, _(": perhaps a D/F conflict?")); + return err(o, msg, path, _(": perhaps a D/F conflict?")); } static int update_file_flags(struct merge_options *o, @@ -771,9 +787,9 @@ static int update_file_flags(struct merge_options *o, buf = read_sha1_file(oid->hash, &type, &size); if (!buf) - return error(_("cannot read object %s '%s'"), oid_to_hex(oid), path); + return err(o, _("cannot read object %s '%s'"), oid_to_hex(oid), path); if (type != OBJ_BLOB) { - ret = error(_("blob expected for %s '%s'"), oid_to_hex(oid), path); + ret = err(o, _("blob expected for %s '%s'"), oid_to_hex(oid), path); goto free_buf; } if (S_ISREG(mode)) { @@ -797,8 +813,8 @@ static int update_file_flags(struct merge_options *o, mode = 0666; fd = open(path, O_WRONLY | O_TRUNC | O_CREAT, mode); if (fd < 0) { - ret = error_errno(_("failed to open '%s'"), - path); + ret = err(o, _("failed to open '%s': %s"), + path, strerror(errno)); goto free_buf; } write_in_full(fd, buf, size); @@ -808,17 +824,19 @@ static int update_file_flags(struct merge_options *o, safe_create_leading_directories_const(path); unlink(path); if (symlink(lnk, path)) - ret = error_errno(_("failed to symlink '%s'"), path); + ret = err(o, _("failed to symlink '%s': %s"), + path, strerror(errno)); free(lnk); } else - ret = error(_("do not know what to do with %06o %s '%s'"), - mode, oid_to_hex(oid), path); + ret = err(o, + _("do not know what to do with %06o %s '%s'"), + mode, oid_to_hex(oid), path); free_buf: free(buf); } update_index: if (!ret && update_cache) - add_cacheinfo(mode, oid, path, 0, update_wd, ADD_CACHE_OK_TO_ADD); + add_cacheinfo(o, mode, oid, path, 0, update_wd, ADD_CACHE_OK_TO_ADD); return ret; } @@ -951,12 +969,12 @@ static int merge_file_1(struct merge_options *o, branch1, branch2); if ((merge_status < 0) || !result_buf.ptr) - ret = error(_("Failed to execute internal merge")); + ret = err(o, _("Failed to execute internal merge")); if (!ret && write_sha1_file(result_buf.ptr, result_buf.size, blob_type, result->oid.hash)) - ret = error(_("Unable to add %s to database"), - a->path); + ret = err(o, _("Unable to add %s to database"), + a->path); free(result_buf.ptr); if (ret) @@ -1122,7 +1140,7 @@ static int conflict_rename_delete(struct merge_options *o, if (o->call_depth) return remove_file_from_cache(dest->path); else - return update_stages(dest->path, NULL, + return update_stages(o, dest->path, NULL, rename_branch == o->branch1 ? dest : NULL, rename_branch == o->branch1 ? NULL : dest); } @@ -1180,9 +1198,9 @@ static int handle_file(struct merge_options *o, if ((ret = update_file(o, 0, &rename->oid, rename->mode, dst_name))) ; /* fall through, do allow dst_name to be released */ else if (stage == 2) - ret = update_stages(rename->path, NULL, rename, add); + ret = update_stages(o, rename->path, NULL, rename, add); else - ret = update_stages(rename->path, NULL, add, rename); + ret = update_stages(o, rename->path, NULL, add, rename); if (dst_name != rename->path) free(dst_name); @@ -1575,23 +1593,25 @@ static struct object_id *stage_oid(const struct object_id *oid, unsigned mode) return (is_null_oid(oid) || mode == 0) ? NULL: (struct object_id *)oid; } -static int read_oid_strbuf(const struct object_id *oid, struct strbuf *dst) +static int read_oid_strbuf(struct merge_options *o, + const struct object_id *oid, struct strbuf *dst) { void *buf; enum object_type type; unsigned long size; buf = read_sha1_file(oid->hash, &type, &size); if (!buf) - return error(_("cannot read object %s"), oid_to_hex(oid)); + return err(o, _("cannot read object %s"), oid_to_hex(oid)); if (type != OBJ_BLOB) { free(buf); - return error(_("object %s is not a blob"), oid_to_hex(oid)); + return err(o, _("object %s is not a blob"), oid_to_hex(oid)); } strbuf_attach(dst, buf, size, size + 1); return 0; } -static int blob_unchanged(const struct object_id *o_oid, +static int blob_unchanged(struct merge_options *opt, + const struct object_id *o_oid, unsigned o_mode, const struct object_id *a_oid, unsigned a_mode, @@ -1609,7 +1629,7 @@ static int blob_unchanged(const struct object_id *o_oid, return 0; assert(o_oid && a_oid); - if (read_oid_strbuf(o_oid, &o) || read_oid_strbuf(a_oid, &a)) + if (read_oid_strbuf(opt, o_oid, &o) || read_oid_strbuf(opt, a_oid, &a)) goto error_return; /* * Note: binary | is used so that both renormalizations are @@ -1698,7 +1718,7 @@ static int merge_content(struct merge_options *o, */ path_renamed_outside_HEAD = !path2 || !strcmp(path, path2); if (!path_renamed_outside_HEAD) { - add_cacheinfo(mfi.mode, &mfi.oid, path, + add_cacheinfo(o, mfi.mode, &mfi.oid, path, 0, (!o->call_depth), 0); return mfi.clean; } @@ -1711,7 +1731,7 @@ static int merge_content(struct merge_options *o, output(o, 1, _("CONFLICT (%s): Merge conflict in %s"), reason, path); if (rename_conflict_info && !df_conflict_remains) - if (update_stages(path, &one, &a, &b)) + if (update_stages(o, path, &one, &a, &b)) return -1; } @@ -1721,7 +1741,7 @@ static int merge_content(struct merge_options *o, remove_file_from_cache(path); } else { if (!mfi.clean) { - if (update_stages(path, &one, &a, &b)) + if (update_stages(o, path, &one, &a, &b)) return -1; } else { int file_from_stage2 = was_tracked(path); @@ -1729,7 +1749,7 @@ static int merge_content(struct merge_options *o, oidcpy(&merged.oid, &mfi.oid); merged.mode = mfi.mode; - if (update_stages(path, NULL, + if (update_stages(o, path, NULL, file_from_stage2 ? &merged : NULL, file_from_stage2 ? NULL : &merged)) return -1; @@ -1797,8 +1817,8 @@ static int process_entry(struct merge_options *o, } else if (o_oid && (!a_oid || !b_oid)) { /* Case A: Deleted in one */ if ((!a_oid && !b_oid) || - (!b_oid && blob_unchanged(o_oid, o_mode, a_oid, a_mode, normalize, path)) || - (!a_oid && blob_unchanged(o_oid, o_mode, b_oid, b_mode, normalize, path))) { + (!b_oid && blob_unchanged(o, o_oid, o_mode, a_oid, a_mode, normalize, path)) || + (!a_oid && blob_unchanged(o, o_oid, o_mode, b_oid, b_mode, normalize, path))) { /* Deleted in both or deleted in one and * unchanged in the other */ if (a_oid) @@ -1894,7 +1914,7 @@ int merge_trees(struct merge_options *o, if (code != 0) { if (show(o, 4) || o->call_depth) - error(_("merging of trees %s and %s failed"), + err(o, _("merging of trees %s and %s failed"), oid_to_hex(&head->object.oid), oid_to_hex(&merge->object.oid)); return -1; @@ -2029,7 +2049,7 @@ int merge_recursive(struct merge_options *o, o->call_depth--; if (!merged_common_ancestors) - return error(_("merge returned no commit")); + return err(o, _("merge returned no commit")); } discard_cache(); @@ -2088,7 +2108,7 @@ int merge_recursive_generic(struct merge_options *o, for (i = 0; i < num_base_list; ++i) { struct commit *base; if (!(base = get_ref(base_list[i], oid_to_hex(base_list[i])))) - return error(_("Could not parse object '%s'"), + return err(o, _("Could not parse object '%s'"), oid_to_hex(base_list[i])); commit_list_insert(base, &ca); } @@ -2102,7 +2122,7 @@ int merge_recursive_generic(struct merge_options *o, if (active_cache_changed && write_locked_index(&the_index, lock, COMMIT_LOCK)) - return error(_("Unable to write index.")); + return err(o, _("Unable to write index.")); return clean ? 0 : 1; } -- cgit v0.10.2-6-g49f6 From dde75cb0561fa8a6bbb2de4c3662dbcc33728938 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Mon, 1 Aug 2016 13:44:45 +0200 Subject: merge-recursive: write the commit title in one go In 66a155b (Enable output buffering in merge-recursive., 2007-01-14), we changed the code such that it prints the output in one go, to avoid interfering with the progress output. Let's make sure that the same holds true when outputting the commit title: previously, we used several printf() statements to stdout and assumed that stdout's buffer is large enough to hold the entire commit title. Apart from making that speculation unnecessary, we change the code to add the message to the output buffer before flushing for another reason: the next commit will introduce a new level of output buffering, where the caller can request the output not to be flushed, but to be retained for further processing. This latter feature will be needed when teaching the sequencer to do rebase -i's brunt work: it wants to control the output of the cherry-picks (i.e. recursive merges). Signed-off-by: Johannes Schindelin Signed-off-by: Junio C Hamano diff --git a/merge-recursive.c b/merge-recursive.c index b972a83..99c9635 100644 --- a/merge-recursive.c +++ b/merge-recursive.c @@ -191,25 +191,26 @@ static void output(struct merge_options *o, int v, const char *fmt, ...) static void output_commit_title(struct merge_options *o, struct commit *commit) { - int i; - flush_output(o); - for (i = o->call_depth; i--;) - fputs(" ", stdout); + strbuf_addchars(&o->obuf, ' ', o->call_depth * 2); if (commit->util) - printf("virtual %s\n", merge_remote_util(commit)->name); + strbuf_addf(&o->obuf, "virtual %s\n", + merge_remote_util(commit)->name); else { - printf("%s ", find_unique_abbrev(commit->object.oid.hash, DEFAULT_ABBREV)); + strbuf_addf(&o->obuf, "%s ", + find_unique_abbrev(commit->object.oid.hash, + DEFAULT_ABBREV)); if (parse_commit(commit) != 0) - printf(_("(bad commit)\n")); + strbuf_addf(&o->obuf, _("(bad commit)\n")); else { const char *title; const char *msg = get_commit_buffer(commit, NULL); int len = find_commit_subject(msg, &title); if (len) - printf("%.*s\n", len, title); + strbuf_addf(&o->obuf, "%.*s\n", len, title); unuse_commit_buffer(commit, msg); } } + flush_output(o); } static int add_cacheinfo(struct merge_options *o, -- cgit v0.10.2-6-g49f6 From f1e2426b28399b563527d110c849acd65b680de6 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Mon, 1 Aug 2016 13:44:50 +0200 Subject: merge-recursive: offer an option to retain the output in 'obuf' Since 66a155b (Enable output buffering in merge-recursive., 2007-01-14), we already accumulate the output in a buffer. The idea was to avoid interfering with the progress output that goes to stderr, which is unbuffered, when we write to stdout, which is buffered. We extend that buffering to allow the caller to handle the output (possibly suppressing it). This will help us when extending the sequencer to do rebase -i's brunt work: it does not want the picks to print anything by default but instead determine itself whether to print the output or not. Note that we also redirect the error messages into the output buffer when the caller asked not to flush the output buffer, for two reasons: 1) to retain the correct output order, and 2) to allow the caller to suppress *all* output. Signed-off-by: Johannes Schindelin Signed-off-by: Junio C Hamano diff --git a/merge-recursive.c b/merge-recursive.c index 99c9635..ec50932 100644 --- a/merge-recursive.c +++ b/merge-recursive.c @@ -25,7 +25,7 @@ static void flush_output(struct merge_options *o) { - if (o->obuf.len) { + if (o->buffer_output < 2 && o->obuf.len) { fputs(o->obuf.buf, stdout); strbuf_reset(&o->obuf); } @@ -35,12 +35,21 @@ static int err(struct merge_options *o, const char *err, ...) { va_list params; - flush_output(o); + if (o->buffer_output < 2) + flush_output(o); + else { + strbuf_complete(&o->obuf, '\n'); + strbuf_addstr(&o->obuf, "error: "); + } va_start(params, err); strbuf_vaddf(&o->obuf, err, params); va_end(params); - error("%s", o->obuf.buf); - strbuf_reset(&o->obuf); + if (o->buffer_output > 1) + strbuf_addch(&o->obuf, '\n'); + else { + error("%s", o->obuf.buf); + strbuf_reset(&o->obuf); + } return -1; } diff --git a/merge-recursive.h b/merge-recursive.h index d415724..735343b 100644 --- a/merge-recursive.h +++ b/merge-recursive.h @@ -13,7 +13,7 @@ struct merge_options { MERGE_RECURSIVE_THEIRS } recursive_variant; const char *subtree_shift; - unsigned buffer_output : 1; + unsigned buffer_output; /* 1: output at end, 2: keep buffered */ unsigned renormalize : 1; long xdl_opts; int verbosity; -- cgit v0.10.2-6-g49f6 From 548009c0d58c74dde2c51f675b369b4c50878f1b Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Mon, 1 Aug 2016 13:44:53 +0200 Subject: merge_trees(): ensure that the callers release output buffer The recursive merge machinery accumulates its output in an output buffer, to be flushed at the end of merge_recursive(). At this point, we forgot to release the output buffer. When calling merge_trees() (i.e. the non-recursive part of the recursive merge) directly, the output buffer is never flushed because the caller may be merge_recursive() which wants to flush the output itself. For the same reason, merge_trees() cannot release the output buffer: it may still be needed. Forgetting to release the output buffer did not matter much when running git-checkout, or git-merge-recursive, because we exited after the operation anyway. Ever since cherry-pick learned to pick a commit range, however, this memory leak had the potential of becoming a problem. Signed-off-by: Johannes Schindelin Signed-off-by: Junio C Hamano diff --git a/builtin/checkout.c b/builtin/checkout.c index 07dea3b..8d852d4 100644 --- a/builtin/checkout.c +++ b/builtin/checkout.c @@ -573,6 +573,7 @@ static int merge_working_tree(const struct checkout_opts *opts, exit(128); ret = reset_tree(new->commit->tree, opts, 0, writeout_error); + strbuf_release(&o.obuf); if (ret) return ret; } diff --git a/merge-recursive.c b/merge-recursive.c index ec50932..9e527de 100644 --- a/merge-recursive.c +++ b/merge-recursive.c @@ -2078,6 +2078,8 @@ int merge_recursive(struct merge_options *o, commit_list_insert(h2, &(*result)->parents->next); } flush_output(o); + if (!o->call_depth && o->buffer_output < 2) + strbuf_release(&o->obuf); if (show(o, 2)) diff_warn_rename_limit("merge.renamelimit", o->needed_rename_limit, 0); diff --git a/sequencer.c b/sequencer.c index 286a435..ec50519 100644 --- a/sequencer.c +++ b/sequencer.c @@ -293,6 +293,7 @@ static int do_recursive_merge(struct commit *base, struct commit *next, clean = merge_trees(&o, head_tree, next_tree, base_tree, &result); + strbuf_release(&o.obuf); if (clean < 0) return clean; -- cgit v0.10.2-6-g49f6 From 6999bc7074c2a8d7605c3ad24bd4e5371a6df71c Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Mon, 1 Aug 2016 13:44:57 +0200 Subject: merge-recursive: flush output buffer even when erroring out Ever since 66a155b (Enable output buffering in merge-recursive., 2007-01-14), we had a problem: When the merge failed in a fatal way, all regular output was swallowed because we called die() and did not get a chance to drain the output buffers. To fix this, several modifications were necessary: - we needed to stop die()ing, to give callers a chance to do something when an error occurred (in this case, flush the output buffers), - we needed to delay printing the error message so that the caller can print the buffered output before that, and - we needed to make sure that the output buffers are flushed even when the return value indicates an error. The first two changes were introduced through earlier commits in this patch series, and this commit addresses the third one. Signed-off-by: Johannes Schindelin Signed-off-by: Junio C Hamano diff --git a/merge-recursive.c b/merge-recursive.c index 9e527de..c9e4dbc 100644 --- a/merge-recursive.c +++ b/merge-recursive.c @@ -2069,8 +2069,10 @@ int merge_recursive(struct merge_options *o, o->ancestor = "merged common ancestors"; clean = merge_trees(o, h1->tree, h2->tree, merged_common_ancestors->tree, &mrtree); - if (clean < 0) + if (clean < 0) { + flush_output(o); return clean; + } if (o->call_depth) { *result = make_virtual_commit(mrtree, "merged tree"); -- cgit v0.10.2-6-g49f6