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

From: Junio C Hamano <gitster@pobox.com>
Date: Jan 9, 2021, 00:05 UTC
Message-ID: <xmqq7dongeji.fsf@gitster.c.googlers.com>
In-Reply-To: <53705281b60285837905137f45fc8607012d2f19.1610107599.git.ps@pks.im>

Patrick Steinhardt <ps@pks.im> writes:

Show 21 quoted lines

> +	/*
> +	 * 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?

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.

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.