Patrick Steinhardt [off-list ref] writes:
quoted
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.
Hmph, do you mean
git fetch --append remote-a & git fetch --append remote-b
or something else [*1*]?
With the current write-out codepath, there is no guarantee that ...
quoted
+ 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);
... these stdio operations for a single record would result in a
single atomic write() that would not compete with writes by other
processes. So I wouldn't call "the current design allows ... at the
same time." It has been broken for years and nobody complained.
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.
I view it a good thing, actually, for another reason [*2*], but that
would take a follow-up fix that is much more involved, so let's not
go there (yet). At least buffering a single line's worth of output
in a buffer and writing it out in a single write() would get us much
closer to solving the "mixed up output" from multiple processes
problem the current code seems to already have.
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.
That one may be a valid concern.
Thanks.
[Footnote]
*1* "git fetch --all/--multiple" was strictly serial operation to
avoid such a mixed-up output problem, but perhaps we weren't
careful enough when we introduced parallel fetch and broke it?
*2* When fetching from a single remote, the code makes sure that a
record that is not marked as 'not-for-merge' is listed first by
reordering the records, but it does not work well when fetching
from multiple remotes. Queuing all in the buffer before showing
them would give us an opportunity to sort, but as I said, it
takes coordination among the processes---instead of each process
writing to FETCH_HEAD on their own, somebody has to collect the
records from them, reorder as needed and write them all out.
cf. https://lore.kernel.org/git/X8oL190Vl03B0cQ%2F@coredump.intra.peff.net/
On Mon, Jan 11, 2021 at 11:47:08AM -0800, Junio C Hamano wrote:
Patrick Steinhardt [off-list ref] writes:
quoted
quoted
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.
Hmph, do you mean
git fetch --append remote-a & git fetch --append remote-b
or something else [*1*]?
That's what I mean.
With the current write-out codepath, there is no guarantee that ...
quoted
quoted
+ 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);
... these stdio operations for a single record would result in a
single atomic write() that would not compete with writes by other
processes. So I wouldn't call "the current design allows ... at the
same time." It has been broken for years and nobody complained.
Fair enough. I somehow blindly assumed that `git fetch --all`, which
does invoke `git fetch --append`, did perform the fetch in parallel. If
that's not the case: all the better.
quoted
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.
I view it a good thing, actually, for another reason [*2*], but that
would take a follow-up fix that is much more involved, so let's not
go there (yet). At least buffering a single line's worth of output
in a buffer and writing it out in a single write() would get us much
closer to solving the "mixed up output" from multiple processes
problem the current code seems to already have.
The buffering of a single line is exactly what we're doing now in the
non-atomic case. Previously there had been multiple writes, now we only
call `strbuf_write()` once on the buffer for each reference.
quoted
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.
That one may be a valid concern.
Thanks.
Just to be explicit: are you fine with changes in this patch as-is? They
don't solve concurrent writes to FETCH_HEAD, but they should make it
easier to solve that if we ever need to.
Patrick
[Footnote]
*1* "git fetch --all/--multiple" was strictly serial operation to
avoid such a mixed-up output problem, but perhaps we weren't
careful enough when we introduced parallel fetch and broke it?
*2* When fetching from a single remote, the code makes sure that a
record that is not marked as 'not-for-merge' is listed first by
reordering the records, but it does not work well when fetching
from multiple remotes. Queuing all in the buffer before showing
them would give us an opportunity to sort, but as I said, it
takes coordination among the processes---instead of each process
writing to FETCH_HEAD on their own, somebody has to collect the
records from them, reorder as needed and write them all out.
cf. https://lore.kernel.org/git/X8oL190Vl03B0cQ%2F@coredump.intra.peff.net/
On Tue, Jan 12, 2021 at 01:22:40PM +0100, Patrick Steinhardt wrote:
On Mon, Jan 11, 2021 at 11:47:08AM -0800, Junio C Hamano wrote:
quoted
Patrick Steinhardt [off-list ref] writes:
[snip]
quoted
quoted
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.
I view it a good thing, actually, for another reason [*2*], but that
would take a follow-up fix that is much more involved, so let's not
go there (yet). At least buffering a single line's worth of output
in a buffer and writing it out in a single write() would get us much
closer to solving the "mixed up output" from multiple processes
problem the current code seems to already have.
The buffering of a single line is exactly what we're doing now in the
non-atomic case. Previously there had been multiple writes, now we only
call `strbuf_write()` once on the buffer for each reference.
quoted
quoted
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.
That one may be a valid concern.
Thanks.
Just to be explicit: are you fine with changes in this patch as-is? They
don't solve concurrent writes to FETCH_HEAD, but they should make it
easier to solve that if we ever need to.
Patrick
Never mind, I forgot that I'm still replying on v2 of this patch series.
Patrick