Re: [PATCH 3/7] refs: convert AUTO_MERGE to become a normal pseudo-ref

>
> On Fri, Jan 19, 2024 at 11:28:10AM -0800, Junio C Hamano wrote:
> > Patrick Steinhardt <ps@pks.im> 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.