git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Jonathan Tan <jonathantanmy@google.com>
To: gitster@pobox.com
Cc: peff@peff.net, jonathantanmy@google.com, git@vger.kernel.org
Subject: Re: [RFC PATCH] Modify fetch-pack to no longer die on error?
Date: Tue, 28 Jul 2020 12:23:50 -0700	[thread overview]
Message-ID: <20200728192350.352978-1-jonathantanmy@google.com> (raw)
In-Reply-To: <xmqqft9fi4hm.fsf@gitster.c.googlers.com>

> Jeff King <peff@peff.net> writes:
> 
> > I think it was this one:
> >
> >   https://lore.kernel.org/git/20160927191955.mympqgylrxhkp24n@sigill.intra.peff.net/
> >
> > I still think it's a good idea, but the hard part is lib-ifying all of
> > the functions that call die() to instead return an error up the stack
> > (and handle cleanup, etc). Which is why I never really pushed it
> > further. But if we're going to be lib-ifying such calls anyway, I think
> > it's nice to do this flexible thing (from their perspective it's no more
> > work to trigger the callback than it is to call error() and return).
> 
> Yeah, I almost 80%+ agree.

Thanks Peff for the pointer. That looks like a good idea, and more
comprehensive than my suggested approach.

> The remainder of 20% is that I am not so convinced that (fmt_string
> + va_list) that isn't pretty much usable for anything but generating
> human readable error messages is enough.  It is certainly a good
> enough replacement for (and much better than) the approach to
> prepare an error string in a "struct strbuf err" that was passeed by
> the caller, but I am not sure if that is a good bar to aim to pass
> to begin with ;-).

I think that functions that inform their callers about different errors
already do so through return values, but in any case I think that Peff's
idea can be extended once we need it (e.g. by adding error reporting
functions that can fallback to the string function if the special
function is not present).

> > That said, I do think for this particular case, your "just run it in a
> > sub-process" suggestion may be simpler and more robust.
> 
> For this particular case, with the performance and all, I agree that
> the stupid and robust approach would be the best.

I'm concerned that we will be painting ourselves into a corner here - I
have been appreciating the richer interface that a C call provides us,
compared to sub-processes where we have to communicate through a single
input stream and a single output stream. For example, "wanted-refs" and
packfile URIs (a.k.a. CDN offloading) were made much simpler because it
was quite easy to communicate back the updated hashes for refs (for
"wanted-refs") and more than one lockfile (for packfile URIs). If we
were doing it with "fetch" in remote helpers (like in HTTP protocol v0),
we would have to augment the textual protocol to communicate back the
updated hashes and the list of lockfiles - doable, but more cumbersome.
(That is also why I only implemented those for protocol v2, because
protocol v2 had "stateless-connect" for HTTP.)

Being limited to sub-processes also stopped me from implementing an
improvement to how fetch-pack calls index-pack - when I added debugging
information (list of refs and hashes) when writing the .promisor file,
what I wanted to do was to pass a callback (function pointer + data
pointer) to index-pack to tell it what to write to .promisor, but I
couldn't do that. I also couldn't write the list to the stdin of
index-pack, because the stdin was already used to communicate the
packfile. I could write the list to a temporary file and pass that name
to index-pack, but by that stage I thought it would be simpler to just
have fetch-pack write the .promisor file itself, even though I think
that this should be an index-pack concern, not a fetch-pack one.

(Also, I think that debugging within a process is easier than debugging
across processes, but that might not be a concern that other people
share.)

In this particular case, when doing the lazy fetch, we pass a special
flag that skips some steps - among other things, negotiation. Admittedly
we could add this flag to "fetch" (or whatever command we're going to
invoke to implement this sub-process part), or add the equivalent
general-purpose flags/configs (e.g. fetch.negotiationAlgorithm=null)
(but I haven't looked thoroughly at what flags would be needed). These
seem surmountable and I can't think of anything unsurmountable, but I'm
still worried that we're painting ourselves into a corner.

  parent reply	other threads:[~2020-07-28 19:23 UTC|newest]

Thread overview: 57+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-07-24 22:38 [RFC PATCH] Modify fetch-pack to no longer die on error? Jonathan Tan
2020-07-24 23:07 ` Junio C Hamano
2020-07-24 23:11 ` Junio C Hamano
2020-07-25 21:41   ` Jeff King
2020-07-25 23:01     ` Junio C Hamano
2020-07-27 17:11       ` Jeff King
2020-07-28 19:23       ` Jonathan Tan [this message]
2020-07-28 20:08         ` Jeff King
2020-07-29 18:53           ` Jonathan Tan
2020-07-29 19:29             ` Jeff King
2020-07-29 19:02           ` Junio C Hamano
2020-07-29 22:55             ` Jonathan Tan
2020-08-05  1:20 ` [RFC PATCH 0/7] Lazy fetch with subprocess Jonathan Tan
2020-08-05  1:20   ` [RFC PATCH 1/7] fetch-pack: allow NULL negotiator->add_tip Jonathan Tan
2020-08-05 19:53     ` Junio C Hamano
2020-08-07 20:53       ` Jonathan Tan
2020-08-05  1:20   ` [RFC PATCH 2/7] fetch-pack: allow NULL negotiator->known_common Jonathan Tan
2020-08-05 20:08     ` Junio C Hamano
2020-08-05 22:11       ` Junio C Hamano
2020-08-07 20:59         ` Jonathan Tan
2020-08-05  1:20   ` [RFC PATCH 3/7] negotiator/null: add null fetch negotiator Jonathan Tan
2020-08-05  1:20   ` [RFC PATCH 4/7] fetch: --stdin Jonathan Tan
2020-08-05 20:33     ` Junio C Hamano
2020-08-07 21:10       ` Jonathan Tan
2020-08-07 21:58         ` Junio C Hamano
2020-08-07 21:10       ` Jonathan Tan
2020-08-05  1:20   ` [RFC PATCH 5/7] fetch: submodule config Jonathan Tan
2020-08-05  1:20   ` [RFC PATCH 6/7] fetch: only populate existing_refs if needed Jonathan Tan
2020-08-05  1:20   ` [RFC PATCH 7/7] promisor-remote: use subprocess to fetch Jonathan Tan
2020-08-11 22:52 ` [PATCH v2 0/7] Lazy fetch with subprocess Jonathan Tan
2020-08-11 22:52   ` [PATCH v2 1/7] negotiator/null: add null fetch negotiator Jonathan Tan
2020-08-12 12:55     ` Derrick Stolee
2020-08-12 16:44       ` Junio C Hamano
2020-08-12 17:29         ` Jonathan Tan
2020-08-11 22:52   ` [PATCH v2 2/7] fetch: allow refspecs specified through stdin Jonathan Tan
2020-08-11 22:52   ` [PATCH v2 3/7] fetch: avoid reading submodule config until needed Jonathan Tan
2020-08-12 17:34     ` Junio C Hamano
2020-08-11 22:52   ` [PATCH v2 4/7] fetch: only populate existing_refs if needed Jonathan Tan
2020-08-12 18:06     ` Junio C Hamano
2020-08-11 22:52   ` [PATCH v2 5/7] fetch-pack: do not lazy-fetch during ref iteration Jonathan Tan
2020-08-12 18:25     ` Junio C Hamano
2020-08-11 22:52   ` [PATCH v2 6/7] promisor-remote: lazy-fetch objects in subprocess Jonathan Tan
2020-08-12 18:28     ` Junio C Hamano
2020-08-11 22:52   ` [PATCH v2 7/7] fetch-pack: remove no_dependents code Jonathan Tan
2020-08-12 12:51   ` [PATCH v2 0/7] Lazy fetch with subprocess Derrick Stolee
2020-08-18  4:01 ` [PATCH v3 " Jonathan Tan
2020-08-18  4:01   ` [PATCH v3 1/7] negotiator/noop: add noop fetch negotiator Jonathan Tan
2020-08-18  4:01   ` [PATCH v3 2/7] fetch: allow refspecs specified through stdin Jonathan Tan
2020-08-18  4:01   ` [PATCH v3 3/7] fetch: avoid reading submodule config until needed Jonathan Tan
2020-08-18  4:01   ` [PATCH v3 4/7] fetch: only populate existing_refs if needed Jonathan Tan
2020-08-18  4:01   ` [PATCH v3 5/7] fetch-pack: do not lazy-fetch during ref iteration Jonathan Tan
2020-08-18  4:01   ` [PATCH v3 6/7] promisor-remote: lazy-fetch objects in subprocess Jonathan Tan
2020-08-18  4:01   ` [PATCH v3 7/7] fetch-pack: remove no_dependents code Jonathan Tan
2020-08-18 19:56   ` [PATCH v3 0/7] Lazy fetch with subprocess Junio C Hamano
2020-08-18 22:32     ` Junio C Hamano
2020-08-18 23:36       ` [PATCH] fixup! promisor-remote: lazy-fetch objects in subprocess Jonathan Tan
2020-08-18 23:57         ` Junio C Hamano

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20200728192350.352978-1-jonathantanmy@google.com \
    --to=jonathantanmy@google.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=peff@peff.net \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).