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

[Patrick Steinhardt <ps@pks.im>]

[-- Attachment #1: Type: text/plain, Size: 2240 bytes --]

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.

Patrick

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]