On Mon, Jan 22, 2024 at 2:45 AM Patrick Steinhardt <ps@xxxxxx> wrote:
>
> 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.
I can confirm it was indeed just an oversight. I like your change to
make this code more careful.
