Re: [PATCH v2 4/4] fetch: implement support for atomic reference updates

From: Patrick Steinhardt <ps@pks.im>
Date: Jan 11, 2021, 10:42 UTC
Message-ID: <X/wrgYYcZfYZj+4/@ncase>
In-Reply-To: <xmqq7dongeji.fsf@gitster.c.googlers.com>

On Fri, Jan 08, 2021 at 04:05:53PM -0800, Junio C Hamano wrote:

Show 30 quoted lines

> Patrick Steinhardt <ps@pks.im> writes:
> 
> > +	/*
> > +	 * When using an atomic fetch, we do not want to update FETCH_HEAD if
> > +	 * any of the reference updates fails. We thus have to write all
> > +	 * updates to a buffer first and only commit it as soon as all
> > +	 * references have been successfully updated.
> > +	 */
> > +	if (atomic_fetch) {
> > +		strbuf_addf(&fetch_head->buf, "%s\t%s\t%s",
> > +			    old_oid, merge_status_marker, note);
> > +		strbuf_add(&fetch_head->buf, url, url_len);
> > +		strbuf_addch(&fetch_head->buf, '\n');
> > +	} else {
> > +		fprintf(fetch_head->fp, "%s\t%s\t%s",
> > +			old_oid, merge_status_marker, note);
> > +		for (i = 0; i < url_len; ++i)
> > +			if ('\n' == url[i])
> > +				fputs("\\n", fetch_head->fp);
> > +			else
> > +				fputc(url[i], fetch_head->fp);
> > +		fputc('\n', fetch_head->fp);
> > +	}
> 
> I do not want to see it done like this; formating what ought to come
> out identical with two separate mechanisms will lead to bugs under
> the road.
> 
> Also what is the deal about "\n" vs "\\n"?  Do we already have
> behaviour differences between two codepaths from the get-go?

Good point. I'll unify those code paths.

Show 8 quoted lines

> It would be much more preferrable to see this series go like so:
> 
>     [1/4] create append_fetch_head() that writes out to
>           fetch_head->fp
> 
>     [1.5/4] convert append_fetch_head() to ALWAYS accumulate in
>             fetch_head->buf, and ALWAYS flushes what is accumulated
>             at the end.

This is a change I'm hesitant to make. The thing is that FETCH_HEAD is quite weird as the current design allows different `git fetch --append` processes to write to FETCH_HEAD at the same time. If we change to always accumulate first and flush once we're done, then we essentially have a change in behaviour as FETCH_HEADs aren't written in an interleaving fashion anymore, but only at the end. Also, if there is any unexpected error in between updating references which causes us to die, then we wouldn't have written the already updated references to FETCH_HEAD now.

So I'm all in when it comes to merging formatting directives, even more so as I have missed the "\\n" weirdness. But for now, I'll keep the current flushing semantics in the non-atomic case. Please let me know if you're not convinced and I'll have another look for v4.

Show 17 quoted lines

> After these two patches are applied, there shouldn't be any
> behaviour change or change in the format in generated FETCH_HEAD.
> 
>     [2/4] and [3/4] stay the same
> 
>     [4/4] this step does not touch append_fetch_head() at all.  It
>     just changes the way how fetch_head->buf is flushed at the end
>     (namely, under atomic mode and after seeing any failure, the
>     accumulated output gets discarded without being written).
> 
> I also thought briefly about an alternative approach to rewind and
> truncate the output to its original length upon seeing a failure
> under the atomic mode, but rejected it because the code gets hairy.
> I think "accumulate until we know we want to actually write them out"
> is a good approach to this issue.
> 
> Thanks.

Patrick