From: Jonathan Tan <email@example.com> To: firstname.lastname@example.org Cc: email@example.com, firstname.lastname@example.org, email@example.com 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: <firstname.lastname@example.org> (raw) In-Reply-To: <email@example.com> > Jeff King <firstname.lastname@example.org> writes: > > > I think it was this one: > > > > https://email@example.com/ > > > > 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.
next prev 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 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 \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --subject='Re: [RFC PATCH] Modify fetch-pack to no longer die on error?' \ /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
Code repositories for project(s) associated with this 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).