On Fri, Jan 19, 2024 at 11:28:10AM -0800, Junio C Hamano wrote:
> Patrick Steinhardt <ps@xxxxxx> writes:
>
> > In 70c70de616 (refs: complete list of special refs, 2023-12-14) we have
> > inrtoduced a new `is_special_ref()` function that classifies some refs
>
> "introduced"
>
> > @@ -4687,10 +4685,17 @@ void merge_switch_to_result(struct merge_options *opt,
> > trace2_region_leave("merge", "record_conflicted", opt->repo);
> >
> > trace2_region_enter("merge", "write_auto_merge", opt->repo);
> > - filename = git_path_auto_merge(opt->repo);
> > - fp = xfopen(filename, "w");
> > - fprintf(fp, "%s\n", oid_to_hex(&result->tree->object.oid));
> > - fclose(fp);
> > + if (refs_update_ref(get_main_ref_store(opt->repo), "", "AUTO_MERGE",
> > + &result->tree->object.oid, NULL, REF_NO_DEREF,
> > + UPDATE_REFS_MSG_ON_ERR)) {
> > + /* failure to function */
> > + opt->priv = NULL;
> > + result->clean = -1;
> > + merge_finalize(opt, result);
> > + trace2_region_leave("merge", "write_auto_merge",
> > + opt->repo);
> > + return;
> > + }
> > trace2_region_leave("merge", "write_auto_merge", opt->repo);
> > }
> > if (display_update_msgs)
>
> We used to ignore errors while updating AUTO_MERGE, implying that it
> is an optional nicety that does not have to block the merge. Now we
> explicitly mark the resulting index unclean. While my gut feeling
> says that it should not matter all that much (as such a failure
> would be rare enough that the user may want to inspect and double
> check the situation before going forward), I am not 100% sure if the
> change is behaviour is acceptable by everybody (cc'ed Elijah for
> second opinion).
We only ignored _some_ errors when updating AUTO_MERGE. Most notably we
die when we fail to create the file, but we succeed in case its contents
aren't written. This does not make much sense to me -- my expectation
would be that we should verify either the complete operation or nothing
of it and ignore all failures. Gracefully leaving an empty file behind
is a weird in-between state, so I'd claim it's more or less an oversight
that we did not perform proper error checking here.
Patrick
Attachment:
signature.asc
Description: PGP signature
