promise: introduce promises to track success or error #1666
Conversation
Welcome to GitGitGadgetHi @philip-peterson, and welcome to GitGitGadget, the GitHub App to send patch series to the Git mailing list from GitHub Pull Requests. Please make sure that your Pull Request has a good description, as it will be used as cover letter. You can CC potential reviewers by adding a footer to the PR description with the following syntax: Also, it is a good idea to review the commit messages one last time, as the Git project expects them in a quite specific form:
It is in general a good idea to await the automated test ("Checks") in this Pull Request before contributing the patches, e.g. to avoid trivial issues such as unportable code. Contributing the patchesBefore you can contribute the patches, your GitHub username needs to be added to the list of permitted users. Any already-permitted user can do that, by adding a comment to your PR of the form Both the person who commented An alternative is the channel Once on the list of permitted usernames, you can contribute the patches to the Git mailing list by adding a PR comment If you want to see what email(s) would be sent for a After you submit, GitGitGadget will respond with another comment that contains the link to the cover letter mail in the Git mailing list archive. Please make sure to monitor the discussion in that thread and to address comments and suggestions (while the comments and suggestions will be mirrored into the PR by GitGitGadget, you will still want to reply via mail). If you do not want to subscribe to the Git mailing list just to be able to respond to a mail, you can download the mbox from the Git mailing list archive (click the curl -g --user "<EMailAddress>:<Password>" \
--url "imaps://imap.gmail.com/INBOX" -T /path/to/raw.txtTo iterate on your change, i.e. send a revised patch or patch series, you will first want to (force-)push to the same branch. You probably also want to modify your Pull Request description (or title). It is a good idea to summarize the revision by adding something like this to the cover letter (read: by editing the first comment on the PR, i.e. the PR description): To send a new iteration, just add another PR comment with the contents: Need help?New contributors who want advice are encouraged to join [email protected], where volunteers who regularly contribute to Git are willing to answer newbie questions, give advice, or otherwise provide mentoring to interested contributors. You must join in order to post or view messages, but anyone can join. You may also be able to find help in real time in the developer IRC channel, |
|
There are issues in commit eeb6bcc: |
|
There are issues in commit 0d1b212: |
philip-peterson
force-pushed
the
peterson/email
branch
from
0d1b212
to
481cc20
Compare
|
There are issues in commit dd0feb8: |
|
There are issues in commit 481cc20: |
philip-peterson
force-pushed
the
peterson/email
branch
from
481cc20
to
4bbeb03
Compare
|
There are issues in commit 8fbd738: |
|
There are issues in commit 4bbeb03: |
philip-peterson
force-pushed
the
peterson/email
branch
from
4bbeb03
to
f9f0558
Compare
|
There are issues in commit 280ef74: |
|
There are issues in commit f9f0558: |
philip-peterson
force-pushed
the
peterson/email
branch
from
f9f0558
to
3f43a0a
Compare
|
There are issues in commit dea1420: |
|
There are issues in commit 3f43a0a: |
philip-peterson
force-pushed
the
peterson/email
branch
from
3f43a0a
to
69a499e
Compare
|
There are issues in commit 11a330b: |
|
There are issues in commit 69a499e: |
philip-peterson
force-pushed
the
peterson/email
branch
from
69a499e
to
a44682b
Compare
|
There are issues in commit 11a330b: |
|
There are issues in commit a44682b: |
|
/allow |
|
User philip-peterson is now allowed to use GitGitGadget. WARNING: philip-peterson has no public email address set on GitHub; |
|
@philip-peterson interesting idea!
Please review https://github.blog/2022-06-30-write-better-commits-build-better-projects/ for guidance how to improve the commit message so that the reviewers on the Git mailing list won't focus on a too-short commit message but instead focus on the much harder (but also much more important) task of reviewing the concept behind these patches.
If you look at existing commits, you will see the |
apply.c
Outdated
| _("could not find file diff header")); | ||
| } | ||
|
|
||
| git_hdr_len = promise->result.success_result; |
There was a problem hiding this comment.
| git_hdr_len = promise->result.success_result; | |
| git_hdr_len = parse_git_diff_header_promise->result.success_result; |
Without this, you see tons of test failures.
This kind of finicky issue (where it is too easy to have perfectly compiling code even though the wrong promise object was used by mistake) points to a problem with this approach that you may want to mention in the cover letter: this approach requires a lot of repetitions.
There was a problem hiding this comment.
Great point. Do you have any ideas by chance for how to reduce the repetition?
There was a problem hiding this comment.
You could use the comma operator to combine the conditional return with the assignment of the return value but elegant it ain't...
philip-peterson
force-pushed
the
peterson/email
branch
from
a44682b
to
e406764
Compare
|
There are issues in commit d8fddd9: |
|
There are issues in commit e406764: |
philip-peterson
force-pushed
the
peterson/email
branch
from
e406764
to
1a96a04
Compare
|
There are issues in commit 4e2e1c8: |
philip-peterson
force-pushed
the
peterson/email
branch
from
4e2e1c8
to
1743e58
Compare
|
There are issues in commit 1743e58: |
apply.c
Outdated
| nr = parse_chunk(state, buf.buf + offset, buf.len - offset, patch); | ||
| if (nr < 0) { | ||
|
|
||
| parse_chunk_promise = promise_init(); | ||
| parse_chunk(state, buf.buf + offset, buf.len - offset, patch, parse_chunk_promise); | ||
|
|
||
| promise_assert_finished(parse_chunk_promise); | ||
|
|
||
| if (parse_chunk_promise->state == PROMISE_FAILURE) { | ||
| int nr; | ||
| error("\n\t%s", parse_chunk_promise->result.failure_result.message.buf); | ||
|
|
||
| nr = parse_chunk_promise->result.failure_result.status; | ||
| free_patch(patch); | ||
| if (nr == -128) { | ||
| res = -128; | ||
| goto end; | ||
| } | ||
| break; | ||
| } |
There was a problem hiding this comment.
Please note that the original code prints nothing if nr < 0 unless it is -128. That's because it is not an error for a patch to have non-diff lines at the end; They are simply ignored. Therefore, you need to make sure to print the error only if we are actually erroring out, i.e.:
diff --git a/apply.c b/apply.c
index a5ad0355b5d..546ae4728bd 100644
--- a/apply.c
+++ b/apply.c
@@ -4794,11 +4794,10 @@ static int apply_patch(struct apply_state *state,
if (parse_chunk_promise->state == PROMISE_FAILURE) {
int nr;
- error("\n\t%s", parse_chunk_promise->result.failure_result.message.buf);
-
nr = parse_chunk_promise->result.failure_result.status;
free_patch(patch);
if (nr == -128) {
+ error("\n\t%s", parse_chunk_promise->result.failure_result.message.buf);
res = -128;
goto end;
}With that applied, t3701 passes again for me.
t/t4254-am-corrupt.sh
Outdated
| @@ -60,7 +60,13 @@ test_expect_success setup ' | |||
| test_expect_success 'try to apply corrupted patch' ' | |||
| test_when_finished "git am --abort" && | |||
| test_must_fail git -c advice.amWorkDir=false am bad-patch.diff 2>actual && | |||
| echo "error: git diff header lacks filename information (line 4)" >expected && | |||
| echo \ | |||
| "error: | |||
There was a problem hiding this comment.
As you saw, the check-whitespace job is really unhappy with that trailing space. There's code at the beginning of the file that provides inspiration how to deal with it:
diff --git a/t/t4254-am-corrupt.sh b/t/t4254-am-corrupt.sh
index 3de909a5f92..265bcc41bbd 100755
--- a/t/t4254-am-corrupt.sh
+++ b/t/t4254-am-corrupt.sh
@@ -60,8 +60,9 @@ test_expect_success setup '
test_expect_success 'try to apply corrupted patch' '
test_when_finished "git am --abort" &&
test_must_fail git -c advice.amWorkDir=false am bad-patch.diff 2>actual &&
+ space=" " &&
echo \
-"error:
+"error:$space
could not find header
caused by:
could not find file diff header
|
@dscho Thank you so much for figuring that out!! It will make things so much easier. I really appreciate it. 😊 |
philip-peterson
force-pushed
the
peterson/email
branch
3 times, most recently
from
9086541
to
c4b838f
Compare
Introduce a promise paradigm. A promise starts off in the pending state, and represents an asynchronous (or synchronous) action that will eventually end in either a successful result or a failure result. If a failure result, an error message may be provided. This allows us to represent tasks which may fail, while deferring any control flow actions or error printing that may occur in relation to said task. Signed-off-by: Philip Peterson <[email protected]>
Uses the new promise paradigm in a simple example with git apply and git am. Several operations that may produce errors are encapsulated in a promise, so that any errors may be captured and handled according to how the caller wants to do so. As an added bonus, we can see more context in error messages now, for example: % git apply -3 garbage.patch error: could not find header caused by: no lines to read Signed-off-by: Philip Peterson <[email protected]>
The test suite matched against a regex that expected the entire errored output to end before the additional context added to `git apply`. Signed-off-by: Philip Peterson <[email protected]>
This test was failing because it expects the invocation of `git apply` to be silent. Because previous patches introduce verbosity where previously there was a silent error (in the form of a return code), this adds an opportunity for a bug to become visible. The bug is in the way `git am` invokes `git apply`, not passing through --quiet when it is specified. Signed-off-by: Philip Peterson <[email protected]>
The old error text was shorter than the new, so this just adds the new context message to the expected text. Signed-off-by: Philip Peterson <[email protected]>
philip-peterson
force-pushed
the
peterson/email
branch
from
c4b838f
to
c6cbb7b
Compare
|
/preview |
|
Preview email sent as [email protected] |
|
/submit |
|
Submitted as [email protected] To fetch this version into To fetch this version to local tag |
|
On the Git mailing list, Phillip Wood wrote (reply to this): Hi Philip
On 18/02/2024 07:33, Philip Peterson via GitGitGadget wrote:
> Hello all, this is my first patchset so thank you for being patient with me
> as I learn the process and conventions of your very fine project. These
> patches are intended as part of the Libification effort, to show how we
> could use a Promise structure to help return values from functions.
I agree that we could do with a better way of propagating errors ups the call-chain that (a) allows us to pass more detailed information about the error to the caller and (b) add useful context to the error message as the stack in unwound. I'm afraid I do not think that the promise implementation in this patch series is a good way forward for several reasons.
1) It is hard to see how we can wrap the return value of the function in a promise and preserve type safety. Even if we used some kind of union the compiler will not warn us if the caller reads a different member to the one that the callee set.
2) It obscures the return type of the function and forces callers to unpack the value from the promise adding complexity to the calling code.
3) It imposes a cost in terms of dynamic memory allocation on code that is called synchronously and therefore does not need to allocate the promise on the heap. This adds complexity and is sure to result in memory leaks.
4) If the function fails we need to propagate the error using PROMISE_BUBBLE_UP() which forces the caller to add more context to the error message even if it does covey anything useful to the user. For example in patch 5 we see
error:
could not find header
caused by:
could not find file diff header
caused by:
git diff header lacks filename information (line 4)" >expected
The error message starts by saying it couldn't find the header and ends by saying it did actually find the header but it could not parse it.
5) The cover letter talks about adding asynchronous examples in the future but it is not clear what part of the git code base it is referring to.
I think we'd be better served by some kind of structured error type like the failure_result in this patch series that is allocated on the stack by the caller at the entry point to the library and passed down the call chain. That avoids the need for lots of dynamic allocations and allows us to continue allocating "out" parameters on the stack. For example
int f(struct repository *r) {
struct object_id oid;
if (repo_get_oid(r, "HEAD", &oid))
return error(_("could not parse HEAD"))
/* use oid here */
}
would become
int f(struct repository *r, struct error *err) {
struct object_id oid;
if (repo_get_oid(r, "HEAD", &oid))
return error(&err, _("could not parse HEAD"))
/* use oid here */
}
I'm sure this has been discussed in the past but I didn't manage to turn anything up with a quick search of the archive on lore.kernel.org.
Best Wishes
Phillip
> Problems
> ========
> > We seek to make libification easier by establishing a pattern for tracking
> whether a function errored in a rich way. Currently, any given function
> could immediately die(), or use error() to print directly to the console,
> bypassing any relevant verbosity checks. The use of die() currently makes
> use of Git as a library inconvenient since it is not graceful.
> > Additionally, returning using return error(...) (as is commonly done) always
> just returns a generic error value, -1, which provides little information.
> > > Approach
> ========
> > I solve this problem by splitting the single return value into two return
> values: error, and message. However, managing two output variables can
> require some coordination, and this coordination can be abstracted away by
> use of an existing pattern named Promise.
> > > Promise Concept
> ===============
> > A promise is a contract representing "some task" that will eventually
> complete. Initially a promise is considered in a pending state. When it
> completes, one of two codepaths will eventually be entered: reject, or
> resolve. Once resolved or rejected, the promise enters a different state
> representing the result. Reject or resolve may only be called once on a
> given promise.
> > Until now, everything I described up to this point is consistent with other
> implementations, such as the ECMAScript standard for promises. However, this
> implementation departs from the complexity of those promises. In this
> implementation, promises are simple and canNOT be chained using .then(...)
> and do NOT have any notion of automatic bubbling (via re-entering the
> pending state).
> > > Sample output and reproduction
> ==============================
> > During an error, we can have richer feedback as to what caused the problem.
> > % git apply garbage.patch
> error:
> could not find header
> caused by:
> patch fragment without header at line 1: @@ -2 +2 @@
> > > To reproduce this output, you can use the following patch (garbage.patch):
> > @@ -2 +2 @@
> > > > Goals
> =====
> > I would love to get feedback on this approach. This patchset is kept small,
> so as to serve as a minimal proof of concept. It is intended to abstract to
> asynchronous use-cases even though this is only a synchronous one.
> Eventually, any top-level function, such as apply_all_patches(...) would
> return its output via a promise to make the library interface as clean as
> possible, but this patchset does not accomplish this goal. Hopefully it can
> provide a direction to go in to achieve that.
> > > Diversion
> =========
> > While building this patchset, I noted a bug that may not have a concrete
> repro case in the master branch. The bug is that when invoking git am, it
> can call out to git apply, passing many flags but interestingly not the
> --quiet flag. I included a fix for this issue in the patchset.
> > > Potential Issue
> ===============
> > There is one difficulty with this approach, which is the high level of
> repetition in the code required. Tracking which promise is which is its own
> source of complexity and may make mistakes more prone to happen. If anyone
> has suggestions for how to make the code cleaner, I would love to hear.
> > Thank you, Philip
> > Philip Peterson (5):
> promise: add promise pattern to track success/error from operations
> apply: use new promise structures in git-apply logic as a proving
> ground
> apply: update t4012 test suite
> apply: pass through quiet flag to fix t4150
> am: update test t4254 by adding the new error text
> > Makefile | 1 +
> apply.c | 133 +++++++++++++++++++++++++++--------------
> apply.h | 9 ++-
> builtin/am.c | 5 ++
> promise.c | 89 +++++++++++++++++++++++++++
> promise.h | 71 ++++++++++++++++++++++
> range-diff.c | 14 +++--
> t/t4012-diff-binary.sh | 4 +-
> t/t4254-am-corrupt.sh | 9 ++-
> 9 files changed, 279 insertions(+), 56 deletions(-)
> create mode 100644 promise.c
> create mode 100644 promise.h
> > > base-commit: 2996f11c1d11ab68823f0939b6469dedc2b9ab90
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1666%2Fphilip-peterson%2Fpeterson%2Femail-v1
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1666/philip-peterson/peterson/email-v1
> Pull-Request: https://github.com/git/git/pull/1666
|
|
User |
|
On the Git mailing list, Jeff King wrote (reply to this): On Mon, Feb 19, 2024 at 02:25:29PM +0000, Phillip Wood wrote:
> I think we'd be better served by some kind of structured error type like the
> failure_result in this patch series that is allocated on the stack by the
> caller at the entry point to the library and passed down the call chain.
> That avoids the need for lots of dynamic allocations and allows us to
> continue allocating "out" parameters on the stack. For example
>
> int f(struct repository *r) {
> struct object_id oid;
>
> if (repo_get_oid(r, "HEAD", &oid))
> return error(_("could not parse HEAD"))
>
> /* use oid here */
> }
>
> would become
> int f(struct repository *r, struct error *err) {
> struct object_id oid;
>
> if (repo_get_oid(r, "HEAD", &oid))
> return error(&err, _("could not parse HEAD"))
>
> /* use oid here */
> }
>
> I'm sure this has been discussed in the past but I didn't manage to turn
> anything up with a quick search of the archive on lore.kernel.org.
There's some discussion in this sub-thread:
https://lore.kernel.org/git/[email protected]/
that also references this earlier thread:
https://lore.kernel.org/git/[email protected]/
I still think this is a reasonable way to go. At one point I had a
proof-of-concept conversion of some of the ref code, but I don't think I
have it any more.
-Peff |
|
User |
|
On the Git mailing list, Phillip Wood wrote (reply to this): Hi Peff
On 20/02/2024 02:57, Jeff King wrote:
> On Mon, Feb 19, 2024 at 02:25:29PM +0000, Phillip Wood wrote:
> >> I'm sure this has been discussed in the past but I didn't manage to turn
>> anything up with a quick search of the archive on lore.kernel.org.
> > There's some discussion in this sub-thread:
> > https://lore.kernel.org/git/[email protected]/
> > that also references this earlier thread:
> > https://lore.kernel.org/git/[email protected]/
Thanks for digging up those links
> I still think this is a reasonable way to go. At one point I had a
> proof-of-concept conversion of some of the ref code, but I don't think I
> have it any more.
Ah, that's interesting - the ref transaction functions already take a struct strbuf to populate with an error message so maybe that would be a simple place to start with a conversion to an error struct.
Best Wishes
Phillip
|
|
On the Git mailing list, Patrick Steinhardt wrote (reply to this): On Tue, Feb 20, 2024 at 10:53:06AM +0000, Phillip Wood wrote:
> Hi Peff
>
> On 20/02/2024 02:57, Jeff King wrote:
> > On Mon, Feb 19, 2024 at 02:25:29PM +0000, Phillip Wood wrote:
> >
> > > I'm sure this has been discussed in the past but I didn't manage to turn
> > > anything up with a quick search of the archive on lore.kernel.org.
> >
> > There's some discussion in this sub-thread:
> >
> > https://lore.kernel.org/git/[email protected]/
> >
> > that also references this earlier thread:
> >
> > https://lore.kernel.org/git/[email protected]/
>
> Thanks for digging up those links
>
> > I still think this is a reasonable way to go. At one point I had a
> > proof-of-concept conversion of some of the ref code, but I don't think I
> > have it any more.
>
> Ah, that's interesting - the ref transaction functions already take a struct
> strbuf to populate with an error message so maybe that would be a simple
> place to start with a conversion to an error struct.
I would certainly welcome such a change. It might make sense to wait a
bit until the reftable dust has settled, but once that code has landed
I'd be quite happy to see improvements to our error handling.
While we're already at it throwing ideas around, I also have to wonder
whether this would be a long-term solution towards computer-friendly
errors. One of the problems we quite frequently hit in Gitaly is that we
are forced to parse error messages in order to figure out why exactly
something has failed. Needless to say, this is quite fragile and also
feels very wrong.
Now if we had a more structured way to pass errors around this might
also enable us to convey more meaning to the caller of Git commands. In
a hypothetical world where all errors were using an explicit error type,
then this error type could eventually become richer and contain more
information that is relevant to the calling script. And if such rich
error information was available, then it would not be that far fetched
to ask Git to emit errors in a computer-parsable format like for example
JSON.
Patrick |
|
User |
|
On the Git mailing list, Junio C Hamano wrote (reply to this): Patrick Steinhardt <[email protected]> writes:
> While we're already at it throwing ideas around, I also have to wonder
> whether this would be a long-term solution towards computer-friendly
> errors. One of the problems we quite frequently hit in Gitaly is that we
> are forced to parse error messages in order to figure out why exactly
> something has failed. Needless to say, this is quite fragile and also
> feels very wrong.
>
> Now if we had a more structured way to pass errors around this might
> also enable us to convey more meaning to the caller of Git commands. In
> a hypothetical world where all errors were using an explicit error type,
> then this error type could eventually become richer and contain more
> information that is relevant to the calling script. And if such rich
> error information was available, then it would not be that far fetched
> to ask Git to emit errors in a computer-parsable format like for example
> JSON.
I do not know about the "JSON-parseable" part, but a structured
error message, or even just a set of error codes that can be
recorded in an index, might already be a great improvement. |
|
On the Git mailing list, Jeff King wrote (reply to this): On Tue, Feb 20, 2024 at 01:19:16PM +0100, Patrick Steinhardt wrote:
> While we're already at it throwing ideas around, I also have to wonder
> whether this would be a long-term solution towards computer-friendly
> errors. One of the problems we quite frequently hit in Gitaly is that we
> are forced to parse error messages in order to figure out why exactly
> something has failed. Needless to say, this is quite fragile and also
> feels very wrong.
>
> Now if we had a more structured way to pass errors around this might
> also enable us to convey more meaning to the caller of Git commands. In
> a hypothetical world where all errors were using an explicit error type,
> then this error type could eventually become richer and contain more
> information that is relevant to the calling script. And if such rich
> error information was available, then it would not be that far fetched
> to ask Git to emit errors in a computer-parsable format like for example
> JSON.
I think what I'm proposing (and if I understand correctly what Phillip
was thinking) is somewhat orthogonal. I agree that structured errors are
nice for computers to read. But it does open up a pretty big can of
worms regarding classifying each error, especially as root causes are
often multi-level.
For example, imagine that the caller asks to resolve a ref. We might get
a syscall error opening the loose ref. Or we might get one opening the
packed-refs file (in a reftable world, you might imagine errors opening
various other files). What is the structured error? Obviously it is "we
can't resolve that ref" at some level. But the caller might want to know
about the failed open (whether it is just ENOENT, or if we ran into
EPERM or even EIO).
Or looking at higher levels; if I ask for the merge base between A and
B, but one of those can't be resolved, how do we communicate that error?
It is some sort of "cannot resolve" error, but it needs to be
parameterized to know which is which.
All of those questions can be answered, of course, but now we are
developing a micro-format that lets us describe all errors in a
standardized way. And I think that is going to put a burden on the code
which is generating the errors (and potentially on the consumers, too,
if they have to decipher the structure to figure out what they want).
Whereas what I was really advocating for is punting on the structured
thing entirely. We keep our unstructured string errors for the most
part, but we simply let the caller pass in a context that tells us what
to do with them. That lets us keep providing specific error messages
from low-level functions without printing to stderr or exiting, which
higher-level code (especially lib-ified code) would not want.
I think it could also be the first building block for making more
structured errors (since those low-level callers are free to provide
lots of details), but it doesn't have to be.
-Peff |
|
On the Git mailing list, Philip wrote (reply to this): Hi all,
Thanks for the feedback. Reading between the lines, it sounds like we are not
quite happy with the flavor of the approach. To deconstruct, so we can evaluate
each axis:
1. Canonical indication of pass/fail (as opposed to implicit rules about
return value being negative or positive or nonzero indicating failure)
2. Coupling of control flow with result reporting Bubble-up functionality -
Essentially a convention for handling die()-like functionality without
calling die() and ending the process
3. Further error context via breadcrumbs (Foo errored because Bar errored
because Baz errored)
4. Error codes - Rather than just -1, having a richer, named space of errors
5. Rich error data - void *data attached to each error (not actually proposed
as part of promise but may be relevant)
6. Async-capable - shelving this since it may require further justification.
First, bubble-up functionality (#3) seems like it will be critical to support if
we intend to stop calling die(). This can be done either exhaustively (by
manually checking each error to see if it is fatal, and if so, invoking a
control-flow action), or by a macro. I couldn’t find any established conventions
for doing this in C, and the original patch series doesn’t really address this
well either since it requires a manual `if (...)` check, so it is an area for
further exploration. (Actually another proposed alternative was `longjmp`, but
it sounds like that is a can of worms to be avoided.)
Back to the overall approach. To contrast to one alternative, error reporting
functions a la the intriguing thread by Peff:
https://lore.kernel.org/git/[email protected]/
With that pattern, there is no concept of an overall status, either pass or
fail. One must assume that if there was an error, then the operation failed, and
if no error, then it passed. But then there is a question of whether an error
was fatal. We know that some errors are fatal (meaning an immediate halt of
execution is in order) and others are not. If we implemented this approach we
would probably need to add this bool is_fatal as a field to struct
error_context.
Also, unless a macro is employed, the reporting of errors is not coupled with
control flow (#2) unlike promise, since PROMISE_THROW() invokes a return
statement. Of course, Peff’s code there was just a sketch instead of a
full-blown patch, but not coupling the two could easily lead to bugs in the form
of accidentally-omitted return statements, so perhaps a macro is in order if
that pattern is implemented.
The lifecycle of anything in void *data poses its own challenges. While promise
offers macros to handle the strbuf release at EOL, special handlers would need
to be written to free anything stored in void *data. Which brings me to this
feedback:
> 1) It is hard to see how we can wrap the return value of the function in a
> promise and preserve type safety. Even if we used some kind of union the
> compiler will not warn us if the caller reads a different member to the one
> that the callee set.
>
> 2) It obscures the return type of the function and forces callers to unpack
> the value from the promise adding complexity to the calling code.
I should clarify that the promise concept is not intended to expand in scope as
far as the return value. It should remain as `int` and only ever `int`, so no
return value would be anything other than a simple number, meaning AFAIK (1)
should not apply. For (2), if auxiliary data structures (additional ints, or
structs, etc.) are outputs, they would need to be via "output parameters" passed
into the function. Those output parameters should not need to be unpacked, since
their type is preserved in the normal way.
However to return to void *data in struct error_context, the unpacking *would*
be necessary since *data doesn’t have a concrete type. Therefore, if we go with
struct error_context, the *data field should be omitted since it adds complexity
without need.
Since we are dropping async support, I am going to rename the topic from
`Promise` to `Result` as well, since the idea is equivalent to `Result<T, E>` in
Rust, aka `Either a b` in Haskell.
Regarding error messages, totally valid criticism that they are confusing to the
user as posed in the original patchset. Eventually perhaps the messages could be
reworded to increase clarity, but to avoid scope creep, let’s just not show
them. On the topic of scope, I agree with what I believe Phillip Wood implied,
that excessive refactoring should not be required to adopt this pattern. As such
I will keep this in mind in the next iteration.
There is more to discuss about strings vs error codes and the future of error
codes as well as memory allocation, but this is probably enough for now. I may
be interested to try out the error_context as well as a concept next just to see
what it would be like, but do people agree with the differential analysis so far
and changes proposed? And curious if you have any other thoughts?
- Philip
On Wed, Feb 21, 2024 at 1:03 PM Jeff King <[email protected]> wrote:
>
> On Tue, Feb 20, 2024 at 01:19:16PM +0100, Patrick Steinhardt wrote:
>
> > While we're already at it throwing ideas around, I also have to wonder
> > whether this would be a long-term solution towards computer-friendly
> > errors. One of the problems we quite frequently hit in Gitaly is that we
> > are forced to parse error messages in order to figure out why exactly
> > something has failed. Needless to say, this is quite fragile and also
> > feels very wrong.
> >
> > Now if we had a more structured way to pass errors around this might
> > also enable us to convey more meaning to the caller of Git commands. In
> > a hypothetical world where all errors were using an explicit error type,
> > then this error type could eventually become richer and contain more
> > information that is relevant to the calling script. And if such rich
> > error information was available, then it would not be that far fetched
> > to ask Git to emit errors in a computer-parsable format like for example
> > JSON.
>
> I think what I'm proposing (and if I understand correctly what Phillip
> was thinking) is somewhat orthogonal. I agree that structured errors are
> nice for computers to read. But it does open up a pretty big can of
> worms regarding classifying each error, especially as root causes are
> often multi-level.
>
> For example, imagine that the caller asks to resolve a ref. We might get
> a syscall error opening the loose ref. Or we might get one opening the
> packed-refs file (in a reftable world, you might imagine errors opening
> various other files). What is the structured error? Obviously it is "we
> can't resolve that ref" at some level. But the caller might want to know
> about the failed open (whether it is just ENOENT, or if we ran into
> EPERM or even EIO).
>
> Or looking at higher levels; if I ask for the merge base between A and
> B, but one of those can't be resolved, how do we communicate that error?
> It is some sort of "cannot resolve" error, but it needs to be
> parameterized to know which is which.
>
> All of those questions can be answered, of course, but now we are
> developing a micro-format that lets us describe all errors in a
> standardized way. And I think that is going to put a burden on the code
> which is generating the errors (and potentially on the consumers, too,
> if they have to decipher the structure to figure out what they want).
>
> Whereas what I was really advocating for is punting on the structured
> thing entirely. We keep our unstructured string errors for the most
> part, but we simply let the caller pass in a context that tells us what
> to do with them. That lets us keep providing specific error messages
> from low-level functions without printing to stderr or exiting, which
> higher-level code (especially lib-ified code) would not want.
>
> I think it could also be the first building block for making more
> structured errors (since those low-level callers are free to provide
> lots of details), but it doesn't have to be.
>
> -Peff |
|
User |
Hello all, this is my first patchset so thank you for being patient with me as I learn the process and conventions of your very fine project. These patches are intended as part of the Libification effort, to show how we could use a Promise structure to help return values from functions.
Problems
We seek to make libification easier by establishing a pattern for tracking whether a function errored in a rich way. Currently, any given function could immediately die(), or use error() to print directly to the console, bypassing any relevant verbosity checks. The use of die() currently makes use of Git as a library inconvenient since it is not graceful.
Additionally, returning using return error(...) (as is commonly done) always just returns a generic error value, -1, which provides little information.
Approach
I solve this problem by splitting the single return value into two return values: error, and message. However, managing two output variables can require some coordination, and this coordination can be abstracted away by use of an existing pattern named Promise.
Promise Concept
A promise is a contract representing "some task" that will eventually complete. Initially a promise is considered in a pending state. When it completes, one of two codepaths will eventually be entered: reject, or resolve. Once resolved or rejected, the promise enters a different state representing the result. Reject or resolve may only be called once on a given promise.
Until now, everything I described up to this point is consistent with other implementations, such as the ECMAScript standard for promises. However, this implementation departs from the complexity of those promises. In this implementation, promises are simple and canNOT be chained using .then(...) and do NOT have any notion of automatic bubbling (via re-entering the pending state).
Sample output and reproduction
During an error, we can have richer feedback as to what caused the problem.
To reproduce this output, you can use the following patch (
garbage.patch):Goals
I would love to get feedback on this approach. This patchset is kept small, so as to serve as a minimal proof of concept. It is intended to abstract to asynchronous use-cases even though this is only a synchronous one. Eventually, any top-level function, such as apply_all_patches(...) would return its output via a promise to make the library interface as clean as possible, but this patchset does not accomplish this goal. Hopefully it can provide a direction to go in to achieve that.
Diversion
While building this patchset, I noted a bug that may not have a concrete repro case in the master branch. The bug is that when invoking
git am, it can call out togit apply, passing many flags but interestingly not the--quietflag. I included a fix for this issue in the patchset.Potential Issue
There is one difficulty with this approach, which is the high level of repetition in the code required. Tracking which promise is which is its own source of complexity and may make mistakes more prone to happen. If anyone has suggestions for how to make the code cleaner, I would love to hear.
Thank you,
Philip
CC: Johannes Schindelin [email protected], Emily Shaffer [email protected]
cc: Phillip Wood [email protected]
cc: Jeff King [email protected]
cc: Patrick Steinhardt [email protected]
cc: Philip [email protected]