kernel
/
git
mirror of https://git.kernel.org/pub/scm/git/git.git
1
0
Fork 0
Code Releases Activity

Merge branch 'en/ort-inner-merge-error-fix' 

The "ort" merge backend saw one bugfix for a crash that happens
when inner merge gets killed, and assorted code clean-ups.

* en/ort-inner-merge-error-fix:
  merge-ort: fix missing early return
  merge-ort: convert more error() cases to path_msg()
  merge-ort: upon merge abort, only show messages causing the abort
  merge-ort: loosen commented requirements
  merge-ort: clearer propagation of failure-to-function from merge_submodule
  merge-ort: fix type of local 'clean' var in handle_content_merge ()
  merge-ort: maintain expected invariant for priv member
  merge-ort: extract handling of priv member into reusable function
maint
Junio C Hamano 2024-07-16 11:18:55 -07:00
parent ad850ef1cf fcf59ac136
commit ffc8f1142c
2 changed files with 165 additions and 46 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

167
merge-ort.c
View File

@ -545,17 +545,35 @@ enum conflict_and_info_types {
CONFLICT_SUBMODULE_HISTORY_NOT_AVAILABLE, CONFLICT_SUBMODULE_HISTORY_NOT_AVAILABLE,
CONFLICT_SUBMODULE_MAY_HAVE_REWINDS, CONFLICT_SUBMODULE_MAY_HAVE_REWINDS,
CONFLICT_SUBMODULE_NULL_MERGE_BASE, CONFLICT_SUBMODULE_NULL_MERGE_BASE,
CONFLICT_SUBMODULE_CORRUPT,
/* INSERT NEW ENTRIES HERE */

/*
* Keep this entry after all regular conflict and info types; only
* errors (failures causing immediate abort of the merge) should
* come after this.
*/
NB_REGULAR_CONFLICT_TYPES,

/*
* Something is seriously wrong; cannot even perform merge;
* Keep this group _last_ other than NB_TOTAL_TYPES
*/
ERROR_SUBMODULE_CORRUPT,
ERROR_THREEWAY_CONTENT_MERGE_FAILED,
ERROR_OBJECT_WRITE_FAILED,
ERROR_OBJECT_READ_FAILED,
ERROR_OBJECT_NOT_A_BLOB,


/* Keep this entry _last_ in the list */ /* Keep this entry _last_ in the list */
NB_CONFLICT_TYPES, NB_TOTAL_TYPES,
}; };


/* /*
* Short description of conflict type, relied upon by external tools. * Short description of conflict type, relied upon by external tools.
* *
* We can add more entries, but DO NOT change any of these strings. Also, * We can add more entries, but DO NOT change any of these strings. Also,
* Order MUST match conflict_info_and_types. * please ensure the order matches what is used in conflict_info_and_types.
*/ */
static const char *type_short_descriptions[] = { static const char *type_short_descriptions[] = {
/*** "Simple" conflicts and informational messages ***/ /*** "Simple" conflicts and informational messages ***/
@ -599,8 +617,18 @@ static const char *type_short_descriptions[] = {
"CONFLICT (submodule may have rewinds)", "CONFLICT (submodule may have rewinds)",
[CONFLICT_SUBMODULE_NULL_MERGE_BASE] = [CONFLICT_SUBMODULE_NULL_MERGE_BASE] =
"CONFLICT (submodule lacks merge base)", "CONFLICT (submodule lacks merge base)",
[CONFLICT_SUBMODULE_CORRUPT] =
"CONFLICT (submodule corrupt)" /* Something is seriously wrong; cannot even perform merge */
[ERROR_SUBMODULE_CORRUPT] =
"ERROR (submodule corrupt)",
[ERROR_THREEWAY_CONTENT_MERGE_FAILED] =
"ERROR (three-way content merge failed)",
[ERROR_OBJECT_WRITE_FAILED] =
"ERROR (object write failed)",
[ERROR_OBJECT_READ_FAILED] =
"ERROR (object read failed)",
[ERROR_OBJECT_NOT_A_BLOB] =
"ERROR (object is not a blob)",
}; };


struct logical_conflict_info { struct logical_conflict_info {
@ -764,7 +792,8 @@ static void path_msg(struct merge_options *opt,


/* Sanity checks */ /* Sanity checks */
assert(omittable_hint == assert(omittable_hint ==
!starts_with(type_short_descriptions[type], "CONFLICT") || (!starts_with(type_short_descriptions[type], "CONFLICT") &&
!starts_with(type_short_descriptions[type], "ERROR")) ||
type == CONFLICT_DIR_RENAME_SUGGESTED); type == CONFLICT_DIR_RENAME_SUGGESTED);
if (opt->record_conflict_msgs_as_headers && omittable_hint) if (opt->record_conflict_msgs_as_headers && omittable_hint)
return; /* Do not record mere hints in headers */ return; /* Do not record mere hints in headers */
@ -1819,9 +1848,9 @@ static int merge_submodule(struct merge_options *opt,
/* check whether both changes are forward */ /* check whether both changes are forward */
ret2 = repo_in_merge_bases(&subrepo, commit_o, commit_a); ret2 = repo_in_merge_bases(&subrepo, commit_o, commit_a);
if (ret2 < 0) { if (ret2 < 0) {
path_msg(opt, CONFLICT_SUBMODULE_CORRUPT, 0, path_msg(opt, ERROR_SUBMODULE_CORRUPT, 0,
path, NULL, NULL, NULL, path, NULL, NULL, NULL,
_("Failed to merge submodule %s " _("error: failed to merge submodule %s "
"(repository corrupt)"), "(repository corrupt)"),
path); path);
ret = -1; ret = -1;
@ -1830,9 +1859,9 @@ static int merge_submodule(struct merge_options *opt,
if (ret2 > 0) if (ret2 > 0)
ret2 = repo_in_merge_bases(&subrepo, commit_o, commit_b); ret2 = repo_in_merge_bases(&subrepo, commit_o, commit_b);
if (ret2 < 0) { if (ret2 < 0) {
path_msg(opt, CONFLICT_SUBMODULE_CORRUPT, 0, path_msg(opt, ERROR_SUBMODULE_CORRUPT, 0,
path, NULL, NULL, NULL, path, NULL, NULL, NULL,
_("Failed to merge submodule %s " _("error: failed to merge submodule %s "
"(repository corrupt)"), "(repository corrupt)"),
path); path);
ret = -1; ret = -1;
@ -1850,9 +1879,9 @@ static int merge_submodule(struct merge_options *opt,
/* Case #1: a is contained in b or vice versa */ /* Case #1: a is contained in b or vice versa */
ret2 = repo_in_merge_bases(&subrepo, commit_a, commit_b); ret2 = repo_in_merge_bases(&subrepo, commit_a, commit_b);
if (ret2 < 0) { if (ret2 < 0) {
path_msg(opt, CONFLICT_SUBMODULE_CORRUPT, 0, path_msg(opt, ERROR_SUBMODULE_CORRUPT, 0,
path, NULL, NULL, NULL, path, NULL, NULL, NULL,
_("Failed to merge submodule %s " _("error: failed to merge submodule %s "
"(repository corrupt)"), "(repository corrupt)"),
path); path);
ret = -1; ret = -1;
@ -1869,9 +1898,9 @@ static int merge_submodule(struct merge_options *opt,
} }
ret2 = repo_in_merge_bases(&subrepo, commit_b, commit_a); ret2 = repo_in_merge_bases(&subrepo, commit_b, commit_a);
if (ret2 < 0) { if (ret2 < 0) {
path_msg(opt, CONFLICT_SUBMODULE_CORRUPT, 0, path_msg(opt, ERROR_SUBMODULE_CORRUPT, 0,
path, NULL, NULL, NULL, path, NULL, NULL, NULL,
_("Failed to merge submodule %s " _("error: failed to merge submodule %s "
"(repository corrupt)"), "(repository corrupt)"),
path); path);
ret = -1; ret = -1;
@ -1903,9 +1932,9 @@ static int merge_submodule(struct merge_options *opt,
&merges); &merges);
switch (parent_count) { switch (parent_count) {
case -1: case -1:
path_msg(opt, CONFLICT_SUBMODULE_CORRUPT, 0, path_msg(opt, ERROR_SUBMODULE_CORRUPT, 0,
path, NULL, NULL, NULL, path, NULL, NULL, NULL,
_("Failed to merge submodule %s " _("error: failed to merge submodule %s "
"(repository corrupt)"), "(repository corrupt)"),
path); path);
ret = -1; ret = -1;
@ -2111,7 +2140,7 @@ static int handle_content_merge(struct merge_options *opt,
* merges, which happens for example with rename/rename(2to1) and * merges, which happens for example with rename/rename(2to1) and
* rename/add conflicts. * rename/add conflicts.
*/ */
unsigned clean = 1; int clean = 1;


/* /*
* handle_content_merge() needs both files to be of the same type, i.e. * handle_content_merge() needs both files to be of the same type, i.e.
@ -2175,18 +2204,28 @@ static int handle_content_merge(struct merge_options *opt,
pathnames, extra_marker_size, pathnames, extra_marker_size,
&result_buf); &result_buf);


if ((merge_status < 0) || !result_buf.ptr) if ((merge_status < 0) || !result_buf.ptr) {
ret = error(_("failed to execute internal merge")); path_msg(opt, ERROR_THREEWAY_CONTENT_MERGE_FAILED, 0,
pathnames[0], pathnames[1], pathnames[2], NULL,
_("error: failed to execute internal merge for %s"),
path);
ret = -1;
}


if (!ret && if (!ret &&
write_object_file(result_buf.ptr, result_buf.size, write_object_file(result_buf.ptr, result_buf.size,
OBJ_BLOB, &result->oid)) OBJ_BLOB, &result->oid)) {
ret = error(_("unable to add %s to database"), path); path_msg(opt, ERROR_OBJECT_WRITE_FAILED, 0,

pathnames[0], pathnames[1], pathnames[2], NULL,
_("error: unable to add %s to database"), path);
ret = -1;
}
free(result_buf.ptr); free(result_buf.ptr);

if (ret) if (ret)
return -1; return -1;
clean &= (merge_status == 0); if (merge_status > 0)
clean = 0;
path_msg(opt, INFO_AUTO_MERGING, 1, path, NULL, NULL, NULL, path_msg(opt, INFO_AUTO_MERGING, 1, path, NULL, NULL, NULL,
_("Auto-merging %s"), path); _("Auto-merging %s"), path);
} else if (S_ISGITLINK(a->mode)) { } else if (S_ISGITLINK(a->mode)) {
@ -2194,6 +2233,8 @@ static int handle_content_merge(struct merge_options *opt,
clean = merge_submodule(opt, pathnames[0], clean = merge_submodule(opt, pathnames[0],
two_way ? null_oid() : &o->oid, two_way ? null_oid() : &o->oid,
&a->oid, &b->oid, &result->oid); &a->oid, &b->oid, &result->oid);
if (clean < 0)
return -1;
if (opt->priv->call_depth && two_way && !clean) { if (opt->priv->call_depth && two_way && !clean) {
result->mode = o->mode; result->mode = o->mode;
oidcpy(&result->oid, &o->oid); oidcpy(&result->oid, &o->oid);
@ -3559,18 +3600,27 @@ static int sort_dirs_next_to_their_children(const char *one, const char *two)
return c1 - c2; return c1 - c2;
} }


static int read_oid_strbuf(const struct object_id *oid, static int read_oid_strbuf(struct merge_options *opt,
struct strbuf *dst) const struct object_id *oid,
struct strbuf *dst,
const char *path)
{ {
void *buf; void *buf;
enum object_type type; enum object_type type;
unsigned long size; unsigned long size;
buf = repo_read_object_file(the_repository, oid, &type, &size); buf = repo_read_object_file(the_repository, oid, &type, &size);
if (!buf) if (!buf) {
return error(_("cannot read object %s"), oid_to_hex(oid)); path_msg(opt, ERROR_OBJECT_READ_FAILED, 0,
path, NULL, NULL, NULL,
_("error: cannot read object %s"), oid_to_hex(oid));
return -1;
}
if (type != OBJ_BLOB) { if (type != OBJ_BLOB) {
free(buf); free(buf);
return error(_("object %s is not a blob"), oid_to_hex(oid)); path_msg(opt, ERROR_OBJECT_NOT_A_BLOB, 0,
path, NULL, NULL, NULL,
_("error: object %s is not a blob"), oid_to_hex(oid));
return -1;
} }
strbuf_attach(dst, buf, size, size + 1); strbuf_attach(dst, buf, size, size + 1);
return 0; return 0;
@ -3594,8 +3644,8 @@ static int blob_unchanged(struct merge_options *opt,
if (oideq(&base->oid, &side->oid)) if (oideq(&base->oid, &side->oid))
return 1; return 1;


if (read_oid_strbuf(&base->oid, &basebuf) || if (read_oid_strbuf(opt, &base->oid, &basebuf, path) ||
read_oid_strbuf(&side->oid, &sidebuf)) read_oid_strbuf(opt, &side->oid, &sidebuf, path))
goto error_return; goto error_return;
/* /*
* Note: binary | is used so that both renormalizations are * Note: binary | is used so that both renormalizations are
@ -4645,6 +4695,7 @@ void merge_display_update_messages(struct merge_options *opt,
struct hashmap_iter iter; struct hashmap_iter iter;
struct strmap_entry *e; struct strmap_entry *e;
struct string_list olist = STRING_LIST_INIT_NODUP; struct string_list olist = STRING_LIST_INIT_NODUP;
FILE *o = stdout;


if (opt->record_conflict_msgs_as_headers) if (opt->record_conflict_msgs_as_headers)
BUG("Either display conflict messages or record them as headers, not both"); BUG("Either display conflict messages or record them as headers, not both");
@ -4661,6 +4712,10 @@ void merge_display_update_messages(struct merge_options *opt,
} }
string_list_sort(&olist); string_list_sort(&olist);


/* Print to stderr if we hit errors rather than just conflicts */
if (result->clean < 0)
o = stderr;

/* Iterate over the items, printing them */ /* Iterate over the items, printing them */
for (int path_nr = 0; path_nr < olist.nr; ++path_nr) { for (int path_nr = 0; path_nr < olist.nr; ++path_nr) {
struct string_list *conflicts = olist.items[path_nr].util; struct string_list *conflicts = olist.items[path_nr].util;
@ -4668,24 +4723,30 @@ void merge_display_update_messages(struct merge_options *opt,
struct logical_conflict_info *info = struct logical_conflict_info *info =
conflicts->items[i].util; conflicts->items[i].util;


/* On failure, ignore regular conflict types */
if (result->clean < 0 &&
info->type < NB_REGULAR_CONFLICT_TYPES)
continue;

if (detailed) { if (detailed) {
printf("%lu", (unsigned long)info->paths.nr); fprintf(o, "%lu", (unsigned long)info->paths.nr);
putchar('\0'); fputc('\0', o);
for (int n = 0; n < info->paths.nr; n++) { for (int n = 0; n < info->paths.nr; n++) {
fputs(info->paths.v[n], stdout); fputs(info->paths.v[n], o);
putchar('\0'); fputc('\0', o);
} }
fputs(type_short_descriptions[info->type], fputs(type_short_descriptions[info->type], o);
stdout); fputc('\0', o);
putchar('\0');
} }
puts(conflicts->items[i].string); fputs(conflicts->items[i].string, o);
fputc('\n', o);
if (detailed) if (detailed)
putchar('\0'); fputc('\0', o);
} }
} }
string_list_clear(&olist, 0); string_list_clear(&olist, 0);


if (result->clean >= 0)
print_submodule_conflict_suggestion(&opti->conflicted_submodules); print_submodule_conflict_suggestion(&opti->conflicted_submodules);


/* Also include needed rename limit adjustment now */ /* Also include needed rename limit adjustment now */
@ -5002,6 +5063,26 @@ static void merge_check_renames_reusable(struct merge_result *result,


/*** Function Grouping: merge_incore_*() and their internal variants ***/ /*** Function Grouping: merge_incore_*() and their internal variants ***/


static void move_opt_priv_to_result_priv(struct merge_options *opt,
struct merge_result *result)
{
/*
* opt->priv and result->priv are a bit weird. opt->priv contains
* information that we can re-use in subsequent merge operations to
* enable our cached renames optimization. The best way to provide
* that to subsequent merges is putting it in result->priv.
* However, putting it directly there would mean retrofitting lots
* of functions in this file to also take a merge_result pointer,
* which is ugly and annoying. So, we just make sure at the end of
* the merge (the outer merge if there are internal recursive ones)
* to move it.
*/
assert(opt->priv && !result->priv);
result->priv = opt->priv;
result->_properly_initialized = RESULT_INITIALIZED;
opt->priv = NULL;
}

/* /*
* Originally from merge_trees_internal(); heavily adapted, though. * Originally from merge_trees_internal(); heavily adapted, though.
*/ */
@ -5032,6 +5113,7 @@ redo:
oid_to_hex(&side1->object.oid), oid_to_hex(&side1->object.oid),
oid_to_hex(&side2->object.oid)); oid_to_hex(&side2->object.oid));
result->clean = -1; result->clean = -1;
move_opt_priv_to_result_priv(opt, result);
return; return;
} }
trace2_region_leave("merge", "collect_merge_info", opt->repo); trace2_region_leave("merge", "collect_merge_info", opt->repo);
@ -5062,11 +5144,8 @@ redo:
/* existence of conflicted entries implies unclean */ /* existence of conflicted entries implies unclean */
result->clean &= strmap_empty(&opt->priv->conflicted); result->clean &= strmap_empty(&opt->priv->conflicted);
} }
if (!opt->priv->call_depth) { if (!opt->priv->call_depth || result->clean < 0)
result->priv = opt->priv; move_opt_priv_to_result_priv(opt, result);
result->_properly_initialized = RESULT_INITIALIZED;
opt->priv = NULL;
}
} }


/* /*

42
t/t6406-merge-attr.sh
View File

@ -185,7 +185,7 @@ test_expect_success !WINDOWS 'custom merge driver that is killed with a signal'


>./please-abort && >./please-abort &&
echo "* merge=custom" >.gitattributes && echo "* merge=custom" >.gitattributes &&
test_must_fail git merge main 2>err && test_expect_code 2 git merge main 2>err &&
grep "^error: failed to execute internal merge" err && grep "^error: failed to execute internal merge" err &&
git ls-files -u >output && git ls-files -u >output &&
git diff --name-only HEAD >>output && git diff --name-only HEAD >>output &&
@ -261,4 +261,44 @@ test_expect_success 'binary files with union attribute' '
grep -i "warning.*cannot merge.*HEAD vs. bin-main" output grep -i "warning.*cannot merge.*HEAD vs. bin-main" output
' '


test_expect_success !WINDOWS 'custom merge driver that is killed with a signal on recursive merge' '
test_when_finished "rm -f output please-abort" &&
test_when_finished "git checkout side" &&

git reset --hard anchor &&

git checkout -b base-a main^ &&
echo base-a >text &&
git commit -m base-a text &&

git checkout -b base-b main^ &&
echo base-b >text &&
git commit -m base-b text &&

git checkout -b recursive-a base-a &&
test_must_fail git merge base-b &&
echo recursive-a >text &&
git add text &&
git commit -m recursive-a &&

git checkout -b recursive-b base-b &&
test_must_fail git merge base-a &&
echo recursive-b >text &&
git add text &&
git commit -m recursive-b &&

git config --replace-all \
merge.custom.driver "./custom-merge %O %A %B 0 %P %S %X %Y" &&
git config --replace-all \
merge.custom.name "custom merge driver for testing" &&

>./please-abort &&
echo "* merge=custom" >.gitattributes &&
test_expect_code 2 git merge recursive-a 2>err &&
grep "error: failed to execute internal merge" err &&
git ls-files -u >output &&
git diff --name-only HEAD >>output &&
test_must_be_empty output
'

test_done test_done