{"thread":{"id":"60946","subject":"[PATCH 0/5] promise: introduce promises to track success or error","startedAt":"2024-02-18T07:33:36Z","lastAt":"2024-03-12T04:19:17Z","messageCount":13,"participants":["Philip Peterson via GitGitGadget","Phillip Wood","Jeff King","Patrick Steinhardt","Junio C Hamano","Philip"],"isPatch":true,"patchVersion":1,"patchTotal":5},"messages":[{"id":"488864","messageId":"pull.1666.git.git.1708241612.gitgitgadget@gmail.com","threadId":"60946","inReplyTo":null,"subject":"[PATCH 0/5] promise: introduce promises to track success or error","fromName":"Philip Peterson via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-02-18T07:33:27Z","receivedAt":"2024-02-18T07:33:36Z","isPatch":true,"sender":{"key":"name:Philip Peterson","avatar":null},"body":"Hello all, this is my first patchset so thank you for being patient with me\nas I learn the process and conventions of your very fine project. These\npatches are intended as part of the Libification effort, to show how we\ncould use a Promise structure to help return values from functions.\n\n\nProblems\n========\n\nWe seek to make libification easier by establishing a pattern for tracking\nwhether a function errored in a rich way. Currently, any given function\ncould immediately die(), or use error() to print directly to the console,\nbypassing any relevant verbosity checks. The use of die() currently makes\nuse of Git as a library inconvenient since it is not graceful.\n\nAdditionally, returning using return error(...) (as is commonly done) always\njust returns a generic error value, -1, which provides little information.\n\n\nApproach\n========\n\nI solve this problem by splitting the single return value into two return\nvalues: error, and message. However, managing two output variables can\nrequire some coordination, and this coordination can be abstracted away by\nuse of an existing pattern named Promise.\n\n\nPromise Concept\n===============\n\nA promise is a contract representing \"some task\" that will eventually\ncomplete. Initially a promise is considered in a pending state. When it\ncompletes, one of two codepaths will eventually be entered: reject, or\nresolve. Once resolved or rejected, the promise enters a different state\nrepresenting the result. Reject or resolve may only be called once on a\ngiven promise.\n\nUntil now, everything I described up to this point is consistent with other\nimplementations, such as the ECMAScript standard for promises. However, this\nimplementation departs from the complexity of those promises. In this\nimplementation, promises are simple and canNOT be chained using .then(...)\nand do NOT have any notion of automatic bubbling (via re-entering the\npending state).\n\n\nSample output and reproduction\n==============================\n\nDuring an error, we can have richer feedback as to what caused the problem.\n\n% git apply garbage.patch\nerror: \n    could not find header\ncaused by:\n    patch fragment without header at line 1: @@ -2 +2 @@\n\n\nTo reproduce this output, you can use the following patch (garbage.patch):\n\n@@ -2 +2 @@\n\n\n\nGoals\n=====\n\nI would love to get feedback on this approach. This patchset is kept small,\nso as to serve as a minimal proof of concept. It is intended to abstract to\nasynchronous use-cases even though this is only a synchronous one.\nEventually, any top-level function, such as apply_all_patches(...) would\nreturn its output via a promise to make the library interface as clean as\npossible, but this patchset does not accomplish this goal. Hopefully it can\nprovide a direction to go in to achieve that.\n\n\nDiversion\n=========\n\nWhile building this patchset, I noted a bug that may not have a concrete\nrepro case in the master branch. The bug is that when invoking git am, it\ncan call out to git apply, passing many flags but interestingly not the\n--quiet flag. I included a fix for this issue in the patchset.\n\n\nPotential Issue\n===============\n\nThere is one difficulty with this approach, which is the high level of\nrepetition in the code required. Tracking which promise is which is its own\nsource of complexity and may make mistakes more prone to happen. If anyone\nhas suggestions for how to make the code cleaner, I would love to hear.\n\nThank you, Philip\n\nPhilip Peterson (5):\n  promise: add promise pattern to track success/error from operations\n  apply: use new promise structures in git-apply logic as a proving\n    ground\n  apply: update t4012 test suite\n  apply: pass through quiet flag to fix t4150\n  am: update test t4254 by adding the new error text\n\n Makefile               |   1 +\n apply.c                | 133 +++++++++++++++++++++++++++--------------\n apply.h                |   9 ++-\n builtin/am.c           |   5 ++\n promise.c              |  89 +++++++++++++++++++++++++++\n promise.h              |  71 ++++++++++++++++++++++\n range-diff.c           |  14 +++--\n t/t4012-diff-binary.sh |   4 +-\n t/t4254-am-corrupt.sh  |   9 ++-\n 9 files changed, 279 insertions(+), 56 deletions(-)\n create mode 100644 promise.c\n create mode 100644 promise.h\n\n\nbase-commit: 2996f11c1d11ab68823f0939b6469dedc2b9ab90\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1666%2Fphilip-peterson%2Fpeterson%2Femail-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1666/philip-peterson/peterson/email-v1\nPull-Request: https://github.com/git/git/pull/1666\n-- \ngitgitgadget\n"},{"id":"488865","messageId":"be270db2ff5c63612356c4ef2fafdbe1724b5b71.1708241613.git.gitgitgadget@gmail.com","threadId":"60946","inReplyTo":"pull.1666.git.git.1708241612.gitgitgadget@gmail.com","subject":"[PATCH 1/5] promise: add promise pattern to track success/error from operations","fromName":"Philip Peterson via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-02-18T07:33:28Z","receivedAt":"2024-02-18T07:33:36Z","isPatch":true,"sender":{"key":"name:Philip Peterson","avatar":null},"body":"From: Philip Peterson <philip.c.peterson@gmail.com>\n\nIntroduce a promise paradigm. A promise starts off in the pending state,\nand represents an asynchronous (or synchronous) action that will\neventually end in either a successful result or a failure result. If a\nfailure result, an error message may be provided.\n\nThis allows us to represent tasks which may fail, while deferring any\ncontrol flow actions or error printing that may occur in relation to\nsaid task.\n\nSigned-off-by: Philip Peterson <philip.c.peterson@gmail.com>\n---\n Makefile  |  1 +\n promise.c | 89 +++++++++++++++++++++++++++++++++++++++++++++++++++++++\n promise.h | 71 ++++++++++++++++++++++++++++++++++++++++++++\n 3 files changed, 161 insertions(+)\n create mode 100644 promise.c\n create mode 100644 promise.h\n\ndiff --git a/Makefile b/Makefile\nindex 78e874099d9..4851eb2d822 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -1109,6 +1109,7 @@ LIB_OBJS += preload-index.o\n LIB_OBJS += pretty.o\n LIB_OBJS += prio-queue.o\n LIB_OBJS += progress.o\n+LIB_OBJS += promise.o\n LIB_OBJS += promisor-remote.o\n LIB_OBJS += prompt.o\n LIB_OBJS += protocol.o\ndiff --git a/promise.c b/promise.c\nnew file mode 100644\nindex 00000000000..58ed8b67880\n--- /dev/null\n+++ b/promise.c\n@@ -0,0 +1,89 @@\n+/*\n+ * Generic implementation of callbacks with await checking.\n+ */\n+#include \"promise.h\"\n+\n+void promise_assert_finished(struct promise_t *p) {\n+\tif (p->state == PROMISE_UNRESOLVED) {\n+\t\tBUG(\"expected promise to have been resolved/rejected\");\n+\t}\n+}\n+\n+void promise_assert_failure(struct promise_t *p) {\n+\tif (p->state != PROMISE_FAILURE) {\n+\t\tBUG(\"expected promise to have been rejected\");\n+\t}\n+}\n+\n+void promise_resolve(struct promise_t *p, int status) {\n+\tif (p->state != PROMISE_UNRESOLVED) {\n+\t\tBUG(\"promise was already resolved/rejected\");\n+\t\treturn;\n+\t}\n+\tp->result.success_result = status;\n+\tp->state = PROMISE_SUCCESS;\n+}\n+\n+void promise_reject(struct promise_t *p, int status, const char* fmt, ...) {\n+\tva_list args;\n+\tif (p->state != PROMISE_UNRESOLVED) {\n+\t\tBUG(\"promise was already resolved/rejected\");\n+\t\treturn;\n+\t}\n+\tp->result.failure_result.status = status;\n+\n+\tstrbuf_init(&p->result.failure_result.message, 0);\n+\n+\tva_start(args, fmt);\n+\tstrbuf_vaddf(&p->result.failure_result.message, fmt, args);\n+\tva_end(args);\n+\n+\tp->state = PROMISE_FAILURE;\n+}\n+\n+struct promise_t *promise_init(void) {\n+\t// Promises are allocated on the heap, because they represent potentially long-running tasks,\n+\t// and a stack-allocated value might not live long enough.\n+\tstruct promise_t *new_promise = xmalloc(sizeof(struct promise_t));\n+\tstruct failure_result_t failure_result;\n+\n+\tnew_promise->state = PROMISE_UNRESOLVED;\n+\tfailure_result.status = 0;\n+\tnew_promise->result.failure_result = failure_result;\n+\n+\treturn new_promise;\n+}\n+\n+/**\n+ * Outputs an error message and size from a failed promise. The error message must be\n+ * free()'ed by the caller. Calling this function is not allowed if the promise is not\n+ * failed.\n+ *\n+ * Argument `size` may be omitted by passing in NULL.\n+ *\n+ * Note that although *error_message is null-terminated, its size may be larger\n+ * than the terminated string, and its actual size is indicated by *size.\n+ */\n+void promise_copy_error(struct promise_t *p, char **error_message, size_t *size) {\n+\tsize_t local_size;\n+\tpromise_assert_failure(p);\n+\n+\t*error_message = strbuf_detach(&p->result.failure_result.message, &local_size);\n+\tif (size) {\n+\t\t*size = local_size;\n+\t}\n+\n+\t// We are only doing a copy, not a consume, so we need to put the error message back\n+\t// the way we found it.\n+\tstrbuf_add(&p->result.failure_result.message, *error_message, strlen(*error_message));\n+}\n+\n+/**\n+ * Fully deallocates the promise as well as the error message, if any.\n+ */\n+void promise_release(struct promise_t *p) {\n+\tif (p->state == PROMISE_FAILURE) {\n+\t\tstrbuf_release(&p->result.failure_result.message);\n+\t}\n+\tfree(p);\n+}\ndiff --git a/promise.h b/promise.h\nnew file mode 100644\nindex 00000000000..c5500eba986\n--- /dev/null\n+++ b/promise.h\n@@ -0,0 +1,71 @@\n+#ifndef PROMISE_H\n+#define PROMISE_H\n+\n+#include \"git-compat-util.h\"\n+#include \"strbuf.h\"\n+\n+enum promise_state {\n+\tPROMISE_UNRESOLVED = 0,\n+\tPROMISE_SUCCESS = 1,\n+\tPROMISE_FAILURE = 2,\n+};\n+\n+typedef int success_result_t;\n+\n+struct failure_result_t {\n+\tint status;\n+\tstruct strbuf message;\n+};\n+\n+struct promise_t {\n+\tenum promise_state state;\n+\tunion {\n+\t\tsuccess_result_t success_result;\n+\t\tstruct failure_result_t failure_result;\n+\t} result;\n+};\n+\n+// Function to assert that a promise has been resolved\n+void promise_assert_finished(struct promise_t *p);\n+\n+// Function to assert that a promise has been rejected\n+void promise_assert_failure(struct promise_t *p);\n+\n+// Function to resolve a promise with a success result\n+void promise_resolve(struct promise_t *p, int status);\n+\n+// Function to reject a promise with a failure result and an optional formatted error message\n+void promise_reject(struct promise_t *p, int status, const char* fmt, ...);\n+\n+// Function to create a new promise\n+struct promise_t *promise_init(void);\n+\n+// Copies the error out of a failed promise\n+void promise_copy_error(struct promise_t *promise, char **error_message, size_t *size);\n+\n+// Fully deallocates the promise\n+void promise_release(struct promise_t *promise);\n+\n+#define PROMISE_SUCCEED(p, errcode) do { \\\n+\tpromise_resolve(p, errcode); \\\n+\treturn; \\\n+} while (0)\n+\n+#define PROMISE_THROW(p, errcode, ...) do { \\\n+\tpromise_reject(p, errcode, __VA_ARGS__); \\\n+\treturn; \\\n+} while (0)\n+\n+#define PROMISE_BUBBLE_UP(dst, src, ...) do { \\\n+\tif (strlen(src->result.failure_result.message.buf) != 0) { \\\n+\t\tstrbuf_insertf(&src->result.failure_result.message, 0, \"\\n\\t\"); \\\n+\t\tstrbuf_insertf(&src->result.failure_result.message, 0, _(\"caused by:\")); \\\n+\t\tstrbuf_insertf(&src->result.failure_result.message, 0, \"\\n\"); \\\n+\t\tstrbuf_insertf(&src->result.failure_result.message, 0, __VA_ARGS__); \\\n+\t} \\\n+\tpromise_reject(dst, src->result.failure_result.status, \"%s\", src->result.failure_result.message.buf); \\\n+\tpromise_release(src); \\\n+\treturn; \\\n+} while (0)\n+\n+#endif\n-- \ngitgitgadget\n\n"},{"id":"488866","messageId":"2da1b6509d810c76e5ea753a91b6a3a46a73fa6c.1708241613.git.gitgitgadget@gmail.com","threadId":"60946","inReplyTo":"pull.1666.git.git.1708241612.gitgitgadget@gmail.com","subject":"[PATCH 3/5] apply: update t4012 test suite","fromName":"Philip Peterson via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-02-18T07:33:30Z","receivedAt":"2024-02-18T07:33:39Z","isPatch":true,"sender":{"key":"name:Philip Peterson","avatar":null},"body":"From: Philip Peterson <philip.c.peterson@gmail.com>\n\nThe test suite matched against a regex that expected the entire errored\noutput to end before the additional context added to `git apply`.\n\nSigned-off-by: Philip Peterson <philip.c.peterson@gmail.com>\n---\n t/t4012-diff-binary.sh | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/t/t4012-diff-binary.sh b/t/t4012-diff-binary.sh\nindex c64d9d2f405..59defaa37b8 100755\n--- a/t/t4012-diff-binary.sh\n+++ b/t/t4012-diff-binary.sh\n@@ -68,7 +68,7 @@ test_expect_success 'apply detecting corrupt patch correctly' '\n \tgit diff >output &&\n \tsed -e \"s/-CIT/xCIT/\" <output >broken &&\n \ttest_must_fail git apply --stat --summary broken 2>detected &&\n-\tdetected=$(cat detected) &&\n+\tdetected=$(head -n 1 detected) &&\n \tdetected=$(expr \"$detected\" : \"error.*at line \\\\([0-9]*\\\\)\\$\") &&\n \tdetected=$(sed -ne \"${detected}p\" broken) &&\n \ttest \"$detected\" = xCIT\n@@ -77,7 +77,7 @@ test_expect_success 'apply detecting corrupt patch correctly' '\n test_expect_success 'apply detecting corrupt patch correctly' '\n \tgit diff --binary | sed -e \"s/-CIT/xCIT/\" >broken &&\n \ttest_must_fail git apply --stat --summary broken 2>detected &&\n-\tdetected=$(cat detected) &&\n+\tdetected=$(head -n 1 detected) &&\n \tdetected=$(expr \"$detected\" : \"error.*at line \\\\([0-9]*\\\\)\\$\") &&\n \tdetected=$(sed -ne \"${detected}p\" broken) &&\n \ttest \"$detected\" = xCIT\n-- \ngitgitgadget\n\n"},{"id":"488867","messageId":"677da78652c58f6d0e147638b7b2313dffb0d858.1708241613.git.gitgitgadget@gmail.com","threadId":"60946","inReplyTo":"pull.1666.git.git.1708241612.gitgitgadget@gmail.com","subject":"[PATCH 2/5] apply: use new promise structures in git-apply logic as a proving ground","fromName":"Philip Peterson via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-02-18T07:33:29Z","receivedAt":"2024-02-18T07:33:39Z","isPatch":true,"sender":{"key":"name:Philip Peterson","avatar":null},"body":"From: Philip Peterson <philip.c.peterson@gmail.com>\n\nUses the new promise paradigm in a simple example with git apply and git\nam. Several operations that may produce errors are encapsulated in a\npromise, so that any errors may be captured and handled according to how\nthe caller wants to do so.\n\nAs an added bonus, we can see more context in error messages now, for\nexample:\n\n% git apply -3 garbage.patch\nerror:\n\tcould not find header\ncaused by:\n\tno lines to read\n\nSigned-off-by: Philip Peterson <philip.c.peterson@gmail.com>\n---\n apply.c      | 133 +++++++++++++++++++++++++++++++++------------------\n apply.h      |   9 +++-\n range-diff.c |  14 ++++--\n 3 files changed, 103 insertions(+), 53 deletions(-)\n\ndiff --git a/apply.c b/apply.c\nindex 7608e3301ca..fb6b7074c19 100644\n--- a/apply.c\n+++ b/apply.c\n@@ -26,6 +26,7 @@\n #include \"object-file.h\"\n #include \"parse-options.h\"\n #include \"path.h\"\n+#include \"promise.h\"\n #include \"quote.h\"\n #include \"read-cache.h\"\n #include \"rerere.h\"\n@@ -1316,13 +1317,14 @@ static int check_header_line(int linenr, struct patch *patch)\n \treturn 0;\n }\n \n-int parse_git_diff_header(struct strbuf *root,\n+void parse_git_diff_header(struct strbuf *root,\n \t\t\t  int *linenr,\n \t\t\t  int p_value,\n \t\t\t  const char *line,\n \t\t\t  int len,\n \t\t\t  unsigned int size,\n-\t\t\t  struct patch *patch)\n+\t\t\t  struct patch *patch,\n+\t\t\t  struct promise_t* return_promise)\n {\n \tunsigned long offset;\n \tstruct gitdiff_data parse_hdr_state;\n@@ -1386,10 +1388,12 @@ int parse_git_diff_header(struct strbuf *root,\n \t\t\tif (len < oplen || memcmp(p->str, line, oplen))\n \t\t\t\tcontinue;\n \t\t\tres = p->fn(&parse_hdr_state, line + oplen, patch);\n-\t\t\tif (res < 0)\n-\t\t\t\treturn -1;\n-\t\t\tif (check_header_line(*linenr, patch))\n-\t\t\t\treturn -1;\n+\t\t\tif (res < 0) {\n+\t\t\t\tPROMISE_THROW(return_promise, APPLY_ERR_GENERIC, \"operation for \\\"%s\\\" failed with code: %d\", p->str, res);\n+\t\t\t}\n+\t\t\tif (check_header_line(*linenr, patch)) {\n+\t\t\t\tPROMISE_THROW(return_promise, APPLY_ERR_GENERIC, \"invalid header lines\");\n+\t\t\t}\n \t\t\tif (res > 0)\n \t\t\t\tgoto done;\n \t\t\tbreak;\n@@ -1399,25 +1403,25 @@ int parse_git_diff_header(struct strbuf *root,\n done:\n \tif (!patch->old_name && !patch->new_name) {\n \t\tif (!patch->def_name) {\n-\t\t\terror(Q_(\"git diff header lacks filename information when removing \"\n-\t\t\t\t \"%d leading pathname component (line %d)\",\n-\t\t\t\t \"git diff header lacks filename information when removing \"\n-\t\t\t\t \"%d leading pathname components (line %d)\",\n-\t\t\t\t parse_hdr_state.p_value),\n-\t\t\t      parse_hdr_state.p_value, *linenr);\n-\t\t\treturn -128;\n+\t\t\tPROMISE_THROW(return_promise, -128,\n+\t\t\t\tQ_(\"git diff header lacks filename information when removing \"\n+\t\t\t\t\t\"%d leading pathname component (line %d)\",\n+\t\t\t\t\t\"git diff header lacks filename information when removing \"\n+\t\t\t\t\t\"%d leading pathname components (line %d)\",\n+\t\t\t\t\tparse_hdr_state.p_value),\n+\t\t\t\t\tparse_hdr_state.p_value, *linenr\n+\t\t\t);\n \t\t}\n \t\tpatch->old_name = xstrdup(patch->def_name);\n \t\tpatch->new_name = xstrdup(patch->def_name);\n \t}\n \tif ((!patch->new_name && !patch->is_delete) ||\n \t    (!patch->old_name && !patch->is_new)) {\n-\t\terror(_(\"git diff header lacks filename information \"\n+\t\tPROMISE_THROW(return_promise, -128, _(\"git diff header lacks filename information \"\n \t\t\t\"(line %d)\"), *linenr);\n-\t\treturn -128;\n \t}\n \tpatch->is_toplevel_relative = 1;\n-\treturn offset;\n+\tPROMISE_SUCCEED(return_promise, offset);\n }\n \n static int parse_num(const char *line, unsigned long *p)\n@@ -1539,16 +1543,17 @@ static int parse_fragment_header(const char *line, int len, struct fragment *fra\n /*\n  * Find file diff header\n  *\n- * Returns:\n+ * Resolves promise with:\n  *  -1 if no header was found\n  *  -128 in case of error\n  *   the size of the header in bytes (called \"offset\") otherwise\n  */\n-static int find_header(struct apply_state *state,\n+static void find_header(struct apply_state *state,\n \t\t       const char *line,\n \t\t       unsigned long size,\n \t\t       int *hdrsize,\n-\t\t       struct patch *patch)\n+\t\t       struct patch *patch,\n+\t\t\t   struct promise_t* return_promise)\n {\n \tunsigned long offset, len;\n \n@@ -1577,9 +1582,8 @@ static int find_header(struct apply_state *state,\n \t\t\tstruct fragment dummy;\n \t\t\tif (parse_fragment_header(line, len, &dummy) < 0)\n \t\t\t\tcontinue;\n-\t\t\terror(_(\"patch fragment without header at line %d: %.*s\"),\n+\t\t\tPROMISE_THROW(return_promise, -128, _(\"patch fragment without header at line %d: %.*s\"),\n \t\t\t\t     state->linenr, (int)len-1, line);\n-\t\t\treturn -128;\n \t\t}\n \n \t\tif (size < len + 6)\n@@ -1590,15 +1594,25 @@ static int find_header(struct apply_state *state,\n \t\t * or mode change, so we handle that specially\n \t\t */\n \t\tif (!memcmp(\"diff --git \", line, 11)) {\n-\t\t\tint git_hdr_len = parse_git_diff_header(&state->root, &state->linenr,\n+\t\t\tint git_hdr_len;\n+\t\t\tstruct promise_t *parse_git_diff_header_promise = promise_init();\n+\t\t\tparse_git_diff_header(&state->root, &state->linenr,\n \t\t\t\t\t\t\t\tstate->p_value, line, len,\n-\t\t\t\t\t\t\t\tsize, patch);\n-\t\t\tif (git_hdr_len < 0)\n-\t\t\t\treturn -128;\n+\t\t\t\t\t\t\t\tsize, patch, parse_git_diff_header_promise);\n+\t\t\tpromise_assert_finished(parse_git_diff_header_promise);\n+\n+\t\t\tif (parse_git_diff_header_promise->state == PROMISE_FAILURE) {\n+\t\t\t\tPROMISE_BUBBLE_UP(return_promise, parse_git_diff_header_promise,\n+\t\t\t\t\t\t _(\"could not find file diff header\"));\n+\t\t\t}\n+\n+\t\t\tgit_hdr_len = parse_git_diff_header_promise->result.success_result;\n+\t\t\tpromise_release(parse_git_diff_header_promise);\n \t\t\tif (git_hdr_len <= len)\n \t\t\t\tcontinue;\n \t\t\t*hdrsize = git_hdr_len;\n-\t\t\treturn offset;\n+\t\t\tPROMISE_SUCCEED(return_promise, offset);\n+\t\t\treturn;\n \t\t}\n \n \t\t/* --- followed by +++ ? */\n@@ -1615,13 +1629,14 @@ static int find_header(struct apply_state *state,\n \t\t\tcontinue;\n \n \t\t/* Ok, we'll consider it a patch */\n-\t\tif (parse_traditional_patch(state, line, line+len, patch))\n-\t\t\treturn -128;\n+\t\tif (parse_traditional_patch(state, line, line+len, patch)) {\n+\t\t\tPROMISE_THROW(return_promise, -128, \"could not parse traditional patch\");\n+\t\t}\n \t\t*hdrsize = len + nextlen;\n \t\tstate->linenr += 2;\n-\t\treturn offset;\n+\t\tPROMISE_SUCCEED(return_promise, offset);\n \t}\n-\treturn -1;\n+\tPROMISE_THROW(return_promise, APPLY_ERR_GENERIC, \"no lines to read\");\n }\n \n static void record_ws_error(struct apply_state *state,\n@@ -2129,19 +2144,29 @@ static int use_patch(struct apply_state *state, struct patch *p)\n  * reading after seeing a single patch (i.e. changes to a single file).\n  * Create fragments (i.e. patch hunks) and hang them to the given patch.\n  *\n- * Returns:\n+ * Resolves promise with:\n  *   -1 if no header was found or parse_binary() failed,\n  *   -128 on another error,\n  *   the number of bytes consumed otherwise,\n  *     so that the caller can call us again for the next patch.\n  */\n-static int parse_chunk(struct apply_state *state, char *buffer, unsigned long size, struct patch *patch)\n+static void parse_chunk(struct apply_state *state, char *buffer, unsigned long size, struct patch *patch, struct promise_t* return_promise)\n {\n-\tint hdrsize, patchsize;\n-\tint offset = find_header(state, buffer, size, &hdrsize, patch);\n+\tint hdrsize = -1, patchsize, offset;\n+\tstruct promise_t *find_header_promise = promise_init();\n+\tfind_header(state, buffer, size, &hdrsize, patch, find_header_promise);\n+\tpromise_assert_finished(find_header_promise);\n \n-\tif (offset < 0)\n-\t\treturn offset;\n+\tif (find_header_promise->state == PROMISE_FAILURE) {\n+\t\tPROMISE_BUBBLE_UP(return_promise, find_header_promise, _(\"could not find header\"));\n+\t}\n+\n+\toffset = find_header_promise->result.success_result;\n+\tpromise_release(find_header_promise);\n+\n+\tif (offset < 0) {\n+\t\tPROMISE_SUCCEED(return_promise, offset);\n+\t}\n \n \tprefix_patch(state, patch);\n \n@@ -2159,8 +2184,9 @@ static int parse_chunk(struct apply_state *state, char *buffer, unsigned long si\n \t\t\t\t       size - offset - hdrsize,\n \t\t\t\t       patch);\n \n-\tif (patchsize < 0)\n-\t\treturn -128;\n+\tif (patchsize < 0) {\n+\t\tPROMISE_THROW(return_promise, -128, _(\"could not parse patch\"));\n+\t}\n \n \tif (!patchsize) {\n \t\tstatic const char git_binary[] = \"GIT binary patch\\n\";\n@@ -2173,8 +2199,9 @@ static int parse_chunk(struct apply_state *state, char *buffer, unsigned long si\n \t\t\tstate->linenr++;\n \t\t\tused = parse_binary(state, buffer + hd + llen,\n \t\t\t\t\t    size - hd - llen, patch);\n-\t\t\tif (used < 0)\n-\t\t\t\treturn -1;\n+\t\t\tif (used < 0) {\n+\t\t\t\tPROMISE_THROW(return_promise, -1, _(\"could not parse binary patch\"));\n+\t\t\t}\n \t\t\tif (used)\n \t\t\t\tpatchsize = used + llen;\n \t\t\telse\n@@ -2205,12 +2232,11 @@ static int parse_chunk(struct apply_state *state, char *buffer, unsigned long si\n \t\t */\n \t\tif ((state->apply || state->check) &&\n \t\t    (!patch->is_binary && !metadata_changes(patch))) {\n-\t\t\terror(_(\"patch with only garbage at line %d\"), state->linenr);\n-\t\t\treturn -128;\n+\t\t\tPROMISE_THROW(return_promise, -128, _(\"patch with only garbage at line %d\"), state->linenr);\n \t\t}\n \t}\n \n-\treturn offset + hdrsize + patchsize;\n+\tPROMISE_SUCCEED(return_promise, offset + hdrsize + patchsize);\n }\n \n static void reverse_patches(struct patch *p)\n@@ -4755,21 +4781,36 @@ static int apply_patch(struct apply_state *state,\n \t\treturn -128;\n \toffset = 0;\n \twhile (offset < buf.len) {\n-\t\tstruct patch *patch;\n \t\tint nr;\n+\t\tstruct patch *patch;\n+\t\tstruct promise_t *parse_chunk_promise;\n \n \t\tCALLOC_ARRAY(patch, 1);\n \t\tpatch->inaccurate_eof = !!(options & APPLY_OPT_INACCURATE_EOF);\n \t\tpatch->recount =  !!(options & APPLY_OPT_RECOUNT);\n-\t\tnr = parse_chunk(state, buf.buf + offset, buf.len - offset, patch);\n-\t\tif (nr < 0) {\n+\n+\t\tparse_chunk_promise = promise_init();\n+\t\tparse_chunk(state, buf.buf + offset, buf.len - offset, patch, parse_chunk_promise);\n+\n+\t\tpromise_assert_finished(parse_chunk_promise);\n+\n+\t\tif (parse_chunk_promise->state == PROMISE_FAILURE) {\n+\t\t\tint nr;\n+\t\t\tnr = parse_chunk_promise->result.failure_result.status;\n \t\t\tfree_patch(patch);\n \t\t\tif (nr == -128) {\n+\t\t\t\terror(\"\\n\\t%s\", parse_chunk_promise->result.failure_result.message.buf);\n+\t\t\t\tpromise_release(parse_chunk_promise);\n \t\t\t\tres = -128;\n \t\t\t\tgoto end;\n \t\t\t}\n+\t\t\tpromise_release(parse_chunk_promise);\n \t\t\tbreak;\n \t\t}\n+\n+\t\tnr = parse_chunk_promise->result.success_result;\n+\t\tpromise_release(parse_chunk_promise);\n+\n \t\tif (state->apply_in_reverse)\n \t\t\treverse_patches(patch);\n \t\tif (use_patch(state, patch)) {\ndiff --git a/apply.h b/apply.h\nindex 7cd38b1443c..44af75883c5 100644\n--- a/apply.h\n+++ b/apply.h\n@@ -5,6 +5,10 @@\n #include \"lockfile.h\"\n #include \"string-list.h\"\n #include \"strmap.h\"\n+#include \"promise.h\"\n+\n+/* Error codes (must be less than 0) */\n+#define APPLY_ERR_GENERIC -1\n \n struct repository;\n \n@@ -165,13 +169,14 @@ int check_apply_state(struct apply_state *state, int force_apply);\n  *\n  * Returns -1 on failure, the length of the parsed header otherwise.\n  */\n-int parse_git_diff_header(struct strbuf *root,\n+void parse_git_diff_header(struct strbuf *root,\n \t\t\t  int *linenr,\n \t\t\t  int p_value,\n \t\t\t  const char *line,\n \t\t\t  int len,\n \t\t\t  unsigned int size,\n-\t\t\t  struct patch *patch);\n+\t\t\t  struct patch *patch,\n+\t\t\t  struct promise_t *promise);\n \n void release_patch(struct patch *patch);\n \ndiff --git a/range-diff.c b/range-diff.c\nindex c45b6d849cb..3ef8b976a0c 100644\n--- a/range-diff.c\n+++ b/range-diff.c\n@@ -121,8 +121,8 @@ static int read_patches(const char *range, struct string_list *list,\n \t\tif (starts_with(line, \"diff --git\")) {\n \t\t\tstruct patch patch = { 0 };\n \t\t\tstruct strbuf root = STRBUF_INIT;\n+\t\t\tstruct promise_t *parse_git_diff_header_promise = promise_init();\n \t\t\tint linenr = 0;\n-\t\t\tint orig_len;\n \n \t\t\tin_header = 0;\n \t\t\tstrbuf_addch(&buf, '\\n');\n@@ -130,16 +130,20 @@ static int read_patches(const char *range, struct string_list *list,\n \t\t\t\tutil->diff_offset = buf.len;\n \t\t\tif (eol)\n \t\t\t\t*eol = '\\n';\n-\t\t\torig_len = len;\n-\t\t\tlen = parse_git_diff_header(&root, &linenr, 0, line,\n-\t\t\t\t\t\t    len, size, &patch);\n-\t\t\tif (len < 0) {\n+\t\t\tparse_git_diff_header(&root, &linenr, 0, line,\n+\t\t\t\t\t\t    len, size, &patch, parse_git_diff_header_promise);\n+\t\t\tpromise_assert_finished(parse_git_diff_header_promise);\n+\t\t\tif (parse_git_diff_header_promise->state == PROMISE_FAILURE) {\n+\t\t\t\tint orig_len = len;\n \t\t\t\terror(_(\"could not parse git header '%.*s'\"),\n \t\t\t\t      orig_len, line);\n \t\t\t\tFREE_AND_NULL(util);\n \t\t\t\tstring_list_clear(list, 1);\n+\t\t\t\tpromise_release(parse_git_diff_header_promise);\n \t\t\t\tgoto cleanup;\n \t\t\t}\n+\t\t\tlen = parse_git_diff_header_promise->result.success_result;\n+\t\t\tpromise_release(parse_git_diff_header_promise);\n \t\t\tstrbuf_addstr(&buf, \" ## \");\n \t\t\tif (patch.is_new > 0)\n \t\t\t\tstrbuf_addf(&buf, \"%s (new)\", patch.new_name);\n-- \ngitgitgadget\n\n"},{"id":"488868","messageId":"c0d670df198eabc20bf89854184db7a100cb3030.1708241613.git.gitgitgadget@gmail.com","threadId":"60946","inReplyTo":"pull.1666.git.git.1708241612.gitgitgadget@gmail.com","subject":"[PATCH 4/5] apply: pass through quiet flag to fix t4150","fromName":"Philip Peterson via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-02-18T07:33:31Z","receivedAt":"2024-02-18T07:33:40Z","isPatch":true,"sender":{"key":"name:Philip Peterson","avatar":null},"body":"From: Philip Peterson <philip.c.peterson@gmail.com>\n\nThis test was failing because it expects the invocation of `git apply`\nto be silent. Because previous patches introduce verbosity where\npreviously there was a silent error (in the form of a return code), this\nadds an opportunity for a bug to become visible. The bug is in the way\n`git am` invokes `git apply`, not passing through --quiet when it is\nspecified.\n\nSigned-off-by: Philip Peterson <philip.c.peterson@gmail.com>\n---\n builtin/am.c | 5 +++++\n 1 file changed, 5 insertions(+)\n\ndiff --git a/builtin/am.c b/builtin/am.c\nindex d1990d7edcb..799cb8128a3 100644\n--- a/builtin/am.c\n+++ b/builtin/am.c\n@@ -36,6 +36,7 @@\n #include \"mailinfo.h\"\n #include \"apply.h\"\n #include \"string-list.h\"\n+#include \"packfile.h\"\n #include \"pager.h\"\n #include \"path.h\"\n #include \"repository.h\"\n@@ -2412,6 +2413,10 @@ int cmd_am(int argc, const char **argv, const char *prefix)\n \n \targc = parse_options(argc, argv, prefix, options, usage, 0);\n \n+\tif (state.quiet) {\n+\t\tstrvec_push(&state.git_apply_opts, \"--quiet\");\n+\t}\n+\n \tif (binary >= 0)\n \t\tfprintf_ln(stderr, _(\"The -b/--binary option has been a no-op for long time, and\\n\"\n \t\t\t\t\"it will be removed. Please do not use it anymore.\"));\n-- \ngitgitgadget\n\n"},{"id":"488869","messageId":"c6cbb7b5875cd67485429200369b13a32c2ca886.1708241613.git.gitgitgadget@gmail.com","threadId":"60946","inReplyTo":"pull.1666.git.git.1708241612.gitgitgadget@gmail.com","subject":"[PATCH 5/5] am: update test t4254 by adding the new error text","fromName":"Philip Peterson via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-02-18T07:33:32Z","receivedAt":"2024-02-18T07:33:41Z","isPatch":true,"sender":{"key":"name:Philip Peterson","avatar":null},"body":"From: Philip Peterson <philip.c.peterson@gmail.com>\n\nThe old error text was shorter than the new, so this just adds the new\ncontext message to the expected text.\n\nSigned-off-by: Philip Peterson <philip.c.peterson@gmail.com>\n---\n t/t4254-am-corrupt.sh | 9 ++++++++-\n 1 file changed, 8 insertions(+), 1 deletion(-)\n\ndiff --git a/t/t4254-am-corrupt.sh b/t/t4254-am-corrupt.sh\nindex 45f1d4f95e5..265bcc41bbd 100755\n--- a/t/t4254-am-corrupt.sh\n+++ b/t/t4254-am-corrupt.sh\n@@ -60,7 +60,14 @@ test_expect_success setup '\n test_expect_success 'try to apply corrupted patch' '\n \ttest_when_finished \"git am --abort\" &&\n \ttest_must_fail git -c advice.amWorkDir=false am bad-patch.diff 2>actual &&\n-\techo \"error: git diff header lacks filename information (line 4)\" >expected &&\n+\tspace=\" \" &&\n+\techo \\\n+\"error:$space\n+\tcould not find header\n+caused by:\n+\tcould not find file diff header\n+caused by:\n+\tgit diff header lacks filename information (line 4)\" >expected &&\n \ttest_path_is_file f &&\n \ttest_cmp expected actual\n '\n-- \ngitgitgadget\n"},{"id":"488907","messageId":"bd340a27-bfb4-41b2-a1fa-356ab7dbbd36@gmail.com","threadId":"60946","inReplyTo":"pull.1666.git.git.1708241612.gitgitgadget@gmail.com","subject":"Re: [PATCH 0/5] promise: introduce promises to track success or error","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2024-02-19T14:25:29Z","receivedAt":"2024-02-19T14:25:33Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Philip\n\nOn 18/02/2024 07:33, Philip Peterson via GitGitGadget wrote:\n> Hello all, this is my first patchset so thank you for being patient with me\n> as I learn the process and conventions of your very fine project. These\n> patches are intended as part of the Libification effort, to show how we\n> could use a Promise structure to help return values from functions.\n\nI agree that we could do with a better way of propagating errors ups the \ncall-chain that (a) allows us to pass more detailed information about \nthe error to the caller and (b) add useful context to the error message \nas the stack in unwound. I'm afraid I do not think that the promise \nimplementation in this patch series is a good way forward for several \nreasons.\n\n1) It is hard to see how we can wrap the return value of the function in \na promise and preserve type safety. Even if we used some  kind of union \nthe compiler will not warn us if the caller reads a different member to \nthe one that the callee set.\n\n2) It obscures the return type of the function and forces callers to \nunpack the value from the promise adding complexity to the calling code.\n\n3) It imposes a cost in terms of dynamic memory allocation on code that \nis called synchronously and therefore does not need to allocate the \npromise on the heap. This adds complexity and is sure to result in \nmemory leaks.\n\n4) If the function fails we need to propagate the error using \nPROMISE_BUBBLE_UP() which forces the caller to add more context to the \nerror message even if it does covey anything useful to the user. For \nexample in patch 5 we see\n\n     error:\n\tcould not find header\n     caused by:\n\tcould not find file diff header\n     caused by:\n\tgit diff header lacks filename information (line 4)\" >expected\n\nThe error message starts by saying it couldn't find the header and ends \nby saying it did actually find the header but it could not parse it.\n\n5) The cover letter talks about adding asynchronous examples in the \nfuture but it is not clear what part of the git code base it is \nreferring to.\n\nI think we'd be better served by some kind of structured error type like \nthe failure_result in this patch series that is allocated on the stack \nby the caller at the entry point to the library and passed down the call \nchain. That avoids the need for lots of dynamic allocations and allows \nus to continue allocating \"out\" parameters on the stack. For example\n\n     int f(struct repository *r) {\n\tstruct object_id oid;\n\n\tif (repo_get_oid(r, \"HEAD\", &oid))\n\t\treturn error(_(\"could not parse HEAD\"))\n\n\t/* use oid here */\n     }\n\nwould become\n     int f(struct repository *r, struct error *err) {\n\tstruct object_id oid;\n\n\tif (repo_get_oid(r, \"HEAD\", &oid))\n\t\treturn error(&err, _(\"could not parse HEAD\"))\n\n\t/* use oid here */\n     }\n\nI'm sure this has been discussed in the past but I didn't manage to turn \nanything up with a quick search of the archive on lore.kernel.org.\n\nBest Wishes\n\nPhillip\n\n\n\n> Problems\n> ========\n> \n> We seek to make libification easier by establishing a pattern for tracking\n> whether a function errored in a rich way. Currently, any given function\n> could immediately die(), or use error() to print directly to the console,\n> bypassing any relevant verbosity checks. The use of die() currently makes\n> use of Git as a library inconvenient since it is not graceful.\n> \n> Additionally, returning using return error(...) (as is commonly done) always\n> just returns a generic error value, -1, which provides little information.\n> \n> \n> Approach\n> ========\n> \n> I solve this problem by splitting the single return value into two return\n> values: error, and message. However, managing two output variables can\n> require some coordination, and this coordination can be abstracted away by\n> use of an existing pattern named Promise.\n> \n> \n> Promise Concept\n> ===============\n> \n> A promise is a contract representing \"some task\" that will eventually\n> complete. Initially a promise is considered in a pending state. When it\n> completes, one of two codepaths will eventually be entered: reject, or\n> resolve. Once resolved or rejected, the promise enters a different state\n> representing the result. Reject or resolve may only be called once on a\n> given promise.\n> \n> Until now, everything I described up to this point is consistent with other\n> implementations, such as the ECMAScript standard for promises. However, this\n> implementation departs from the complexity of those promises. In this\n> implementation, promises are simple and canNOT be chained using .then(...)\n> and do NOT have any notion of automatic bubbling (via re-entering the\n> pending state).\n> \n> \n> Sample output and reproduction\n> ==============================\n> \n> During an error, we can have richer feedback as to what caused the problem.\n> \n> % git apply garbage.patch\n> error:\n>      could not find header\n> caused by:\n>      patch fragment without header at line 1: @@ -2 +2 @@\n> \n> \n> To reproduce this output, you can use the following patch (garbage.patch):\n> \n> @@ -2 +2 @@\n> \n> \n> \n> Goals\n> =====\n> \n> I would love to get feedback on this approach. This patchset is kept small,\n> so as to serve as a minimal proof of concept. It is intended to abstract to\n> asynchronous use-cases even though this is only a synchronous one.\n> Eventually, any top-level function, such as apply_all_patches(...) would\n> return its output via a promise to make the library interface as clean as\n> possible, but this patchset does not accomplish this goal. Hopefully it can\n> provide a direction to go in to achieve that.\n> \n> \n> Diversion\n> =========\n> \n> While building this patchset, I noted a bug that may not have a concrete\n> repro case in the master branch. The bug is that when invoking git am, it\n> can call out to git apply, passing many flags but interestingly not the\n> --quiet flag. I included a fix for this issue in the patchset.\n> \n> \n> Potential Issue\n> ===============\n> \n> There is one difficulty with this approach, which is the high level of\n> repetition in the code required. Tracking which promise is which is its own\n> source of complexity and may make mistakes more prone to happen. If anyone\n> has suggestions for how to make the code cleaner, I would love to hear.\n> \n> Thank you, Philip\n> \n> Philip Peterson (5):\n>    promise: add promise pattern to track success/error from operations\n>    apply: use new promise structures in git-apply logic as a proving\n>      ground\n>    apply: update t4012 test suite\n>    apply: pass through quiet flag to fix t4150\n>    am: update test t4254 by adding the new error text\n> \n>   Makefile               |   1 +\n>   apply.c                | 133 +++++++++++++++++++++++++++--------------\n>   apply.h                |   9 ++-\n>   builtin/am.c           |   5 ++\n>   promise.c              |  89 +++++++++++++++++++++++++++\n>   promise.h              |  71 ++++++++++++++++++++++\n>   range-diff.c           |  14 +++--\n>   t/t4012-diff-binary.sh |   4 +-\n>   t/t4254-am-corrupt.sh  |   9 ++-\n>   9 files changed, 279 insertions(+), 56 deletions(-)\n>   create mode 100644 promise.c\n>   create mode 100644 promise.h\n> \n> \n> base-commit: 2996f11c1d11ab68823f0939b6469dedc2b9ab90\n> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1666%2Fphilip-peterson%2Fpeterson%2Femail-v1\n> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1666/philip-peterson/peterson/email-v1\n> Pull-Request: https://github.com/git/git/pull/1666\n\n"},{"id":"488955","messageId":"20240220025722.GE2713741@coredump.intra.peff.net","threadId":"60946","inReplyTo":"bd340a27-bfb4-41b2-a1fa-356ab7dbbd36@gmail.com","subject":"Re: [PATCH 0/5] promise: introduce promises to track success or error","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-02-20T02:57:22Z","receivedAt":"2024-02-20T02:57:23Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Feb 19, 2024 at 02:25:29PM +0000, Phillip Wood wrote:\n\n> I think we'd be better served by some kind of structured error type like the\n> failure_result in this patch series that is allocated on the stack by the\n> caller at the entry point to the library and passed down the call chain.\n> That avoids the need for lots of dynamic allocations and allows us to\n> continue allocating \"out\" parameters on the stack. For example\n> \n>     int f(struct repository *r) {\n> \tstruct object_id oid;\n> \n> \tif (repo_get_oid(r, \"HEAD\", &oid))\n> \t\treturn error(_(\"could not parse HEAD\"))\n> \n> \t/* use oid here */\n>     }\n> \n> would become\n>     int f(struct repository *r, struct error *err) {\n> \tstruct object_id oid;\n> \n> \tif (repo_get_oid(r, \"HEAD\", &oid))\n> \t\treturn error(&err, _(\"could not parse HEAD\"))\n> \n> \t/* use oid here */\n>     }\n> \n> I'm sure this has been discussed in the past but I didn't manage to turn\n> anything up with a quick search of the archive on lore.kernel.org.\n\nThere's some discussion in this sub-thread:\n\n  https://lore.kernel.org/git/20171103191309.sth4zjokgcupvk2e@sigill.intra.peff.net/\n\nthat also references this earlier thread:\n\n  https://lore.kernel.org/git/20160927191955.mympqgylrxhkp24n@sigill.intra.peff.net/\n\nI still think this is a reasonable way to go. At one point I had a\nproof-of-concept conversion of some of the ref code, but I don't think I\nhave it any more.\n\n-Peff\n"},{"id":"488983","messageId":"70a81f5a-32d8-4517-83ea-8019be5f97d5@gmail.com","threadId":"60946","inReplyTo":"20240220025722.GE2713741@coredump.intra.peff.net","subject":"Re: [PATCH 0/5] promise: introduce promises to track success or error","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2024-02-20T10:53:06Z","receivedAt":"2024-02-20T10:53:09Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Peff\n\nOn 20/02/2024 02:57, Jeff King wrote:\n> On Mon, Feb 19, 2024 at 02:25:29PM +0000, Phillip Wood wrote:\n> \n>> I'm sure this has been discussed in the past but I didn't manage to turn\n>> anything up with a quick search of the archive on lore.kernel.org.\n> \n> There's some discussion in this sub-thread:\n> \n>    https://lore.kernel.org/git/20171103191309.sth4zjokgcupvk2e@sigill.intra.peff.net/\n> \n> that also references this earlier thread:\n> \n>    https://lore.kernel.org/git/20160927191955.mympqgylrxhkp24n@sigill.intra.peff.net/\n\nThanks for digging up those links\n\n> I still think this is a reasonable way to go. At one point I had a\n> proof-of-concept conversion of some of the ref code, but I don't think I\n> have it any more.\n\nAh, that's interesting - the ref transaction functions already take a \nstruct strbuf to populate with an error message so maybe that would be a \nsimple place to start with a conversion to an error struct.\n\nBest Wishes\n\nPhillip\n\n"},{"id":"488987","messageId":"ZdSYxF3Hd6Zqt3Wd@tanuki","threadId":"60946","inReplyTo":"70a81f5a-32d8-4517-83ea-8019be5f97d5@gmail.com","subject":"Re: [PATCH 0/5] promise: introduce promises to track success or error","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-02-20T12:19:16Z","receivedAt":"2024-02-20T12:19:25Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Tue, Feb 20, 2024 at 10:53:06AM +0000, Phillip Wood wrote:\n> Hi Peff\n> \n> On 20/02/2024 02:57, Jeff King wrote:\n> > On Mon, Feb 19, 2024 at 02:25:29PM +0000, Phillip Wood wrote:\n> > \n> > > I'm sure this has been discussed in the past but I didn't manage to turn\n> > > anything up with a quick search of the archive on lore.kernel.org.\n> > \n> > There's some discussion in this sub-thread:\n> > \n> >    https://lore.kernel.org/git/20171103191309.sth4zjokgcupvk2e@sigill.intra.peff.net/\n> > \n> > that also references this earlier thread:\n> > \n> >    https://lore.kernel.org/git/20160927191955.mympqgylrxhkp24n@sigill.intra.peff.net/\n> \n> Thanks for digging up those links\n> \n> > I still think this is a reasonable way to go. At one point I had a\n> > proof-of-concept conversion of some of the ref code, but I don't think I\n> > have it any more.\n> \n> Ah, that's interesting - the ref transaction functions already take a struct\n> strbuf to populate with an error message so maybe that would be a simple\n> place to start with a conversion to an error struct.\n\nI would certainly welcome such a change. It might make sense to wait a\nbit until the reftable dust has settled, but once that code has landed\nI'd be quite happy to see improvements to our error handling.\n\nWhile we're already at it throwing ideas around, I also have to wonder\nwhether this would be a long-term solution towards computer-friendly\nerrors. One of the problems we quite frequently hit in Gitaly is that we\nare forced to parse error messages in order to figure out why exactly\nsomething has failed. Needless to say, this is quite fragile and also\nfeels very wrong.\n\nNow if we had a more structured way to pass errors around this might\nalso enable us to convey more meaning to the caller of Git commands. In\na hypothetical world where all errors were using an explicit error type,\nthen this error type could eventually become richer and contain more\ninformation that is relevant to the calling script. And if such rich\nerror information was available, then it would not be that far fetched\nto ask Git to emit errors in a computer-parsable format like for example\nJSON.\n\nPatrick\n"},{"id":"488991","messageId":"xmqqa5nvumxx.fsf@gitster.g","threadId":"60946","inReplyTo":"ZdSYxF3Hd6Zqt3Wd@tanuki","subject":"Re: [PATCH 0/5] promise: introduce promises to track success or error","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-02-20T16:20:42Z","receivedAt":"2024-02-20T16:20:48Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt <ps@pks.im> writes:\n\n> While we're already at it throwing ideas around, I also have to wonder\n> whether this would be a long-term solution towards computer-friendly\n> errors. One of the problems we quite frequently hit in Gitaly is that we\n> are forced to parse error messages in order to figure out why exactly\n> something has failed. Needless to say, this is quite fragile and also\n> feels very wrong.\n>\n> Now if we had a more structured way to pass errors around this might\n> also enable us to convey more meaning to the caller of Git commands. In\n> a hypothetical world where all errors were using an explicit error type,\n> then this error type could eventually become richer and contain more\n> information that is relevant to the calling script. And if such rich\n> error information was available, then it would not be that far fetched\n> to ask Git to emit errors in a computer-parsable format like for example\n> JSON.\n\nI do not know about the \"JSON-parseable\" part, but a structured\nerror message, or even just a set of error codes that can be\nrecorded in an index, might already be a great improvement.\n"},{"id":"489101","messageId":"20240221180354.GE634809@coredump.intra.peff.net","threadId":"60946","inReplyTo":"ZdSYxF3Hd6Zqt3Wd@tanuki","subject":"Re: [PATCH 0/5] promise: introduce promises to track success or error","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-02-21T18:03:54Z","receivedAt":"2024-02-21T18:03:55Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Feb 20, 2024 at 01:19:16PM +0100, Patrick Steinhardt wrote:\n\n> While we're already at it throwing ideas around, I also have to wonder\n> whether this would be a long-term solution towards computer-friendly\n> errors. One of the problems we quite frequently hit in Gitaly is that we\n> are forced to parse error messages in order to figure out why exactly\n> something has failed. Needless to say, this is quite fragile and also\n> feels very wrong.\n> \n> Now if we had a more structured way to pass errors around this might\n> also enable us to convey more meaning to the caller of Git commands. In\n> a hypothetical world where all errors were using an explicit error type,\n> then this error type could eventually become richer and contain more\n> information that is relevant to the calling script. And if such rich\n> error information was available, then it would not be that far fetched\n> to ask Git to emit errors in a computer-parsable format like for example\n> JSON.\n\nI think what I'm proposing (and if I understand correctly what Phillip\nwas thinking) is somewhat orthogonal. I agree that structured errors are\nnice for computers to read. But it does open up a pretty big can of\nworms regarding classifying each error, especially as root causes are\noften multi-level.\n\nFor example, imagine that the caller asks to resolve a ref. We might get\na syscall error opening the loose ref. Or we might get one opening the\npacked-refs file (in a reftable world, you might imagine errors opening\nvarious other files). What is the structured error? Obviously it is \"we\ncan't resolve that ref\" at some level. But the caller might want to know\nabout the failed open (whether it is just ENOENT, or if we ran into\nEPERM or even EIO).\n\nOr looking at higher levels; if I ask for the merge base between A and\nB, but one of those can't be resolved, how do we communicate that error?\nIt is some sort of \"cannot resolve\" error, but it needs to be\nparameterized to know which is which.\n\nAll of those questions can be answered, of course, but now we are\ndeveloping a micro-format that lets us describe all errors in a\nstandardized way. And I think that is going to put a burden on the code\nwhich is generating the errors (and potentially on the consumers, too,\nif they have to decipher the structure to figure out what they want).\n\nWhereas what I was really advocating for is punting on the structured\nthing entirely. We keep our unstructured string errors for the most\npart, but we simply let the caller pass in a context that tells us what\nto do with them. That lets us keep providing specific error messages\nfrom low-level functions without printing to stderr or exiting, which\nhigher-level code (especially lib-ified code) would not want.\n\nI think it could also be the first building block for making more\nstructured errors (since those low-level callers are free to provide\nlots of details), but it doesn't have to be.\n\n-Peff\n"},{"id":"490443","messageId":"CAJ6X7_VsZ2EytWCcpYRqFtG_f+Tx-UXGR4XfL6J1sNhemfG84A@mail.gmail.com","threadId":"60946","inReplyTo":"20240221180354.GE634809@coredump.intra.peff.net","subject":"Re: [PATCH 0/5] promise: introduce promises to track success or error","fromName":"Philip","fromEmail":"philip.c.peterson@gmail.com","sentAt":"2024-03-12T04:18:40Z","receivedAt":"2024-03-12T04:19:17Z","isPatch":true,"sender":{"key":"philip.c.peterson@gmail.com","avatar":"https://gravatar.com/avatar/2dfa9764cb8e5ce07b180670c62dc3fc643e4f8e2f2afafcb7220ac10ea4b4ca?d=mp&s=160"},"body":"Hi all,\n\nThanks for the feedback. Reading between the lines, it sounds like we are not\nquite happy with the flavor of the approach. To deconstruct, so we can evaluate\neach axis:\n\n   1. Canonical indication of pass/fail (as opposed to implicit rules about\n      return value being negative or positive or nonzero indicating failure)\n\n   2. Coupling of control flow with result reporting Bubble-up functionality -\n      Essentially a convention for handling die()-like functionality without\n      calling die() and ending the process\n\n   3. Further error context via breadcrumbs (Foo errored because Bar errored\n      because Baz errored)\n\n   4. Error codes - Rather than just -1, having a richer, named space of errors\n\n   5. Rich error data - void *data attached to each error (not actually proposed\n      as part of promise but may be relevant)\n\n   6. Async-capable - shelving this since it may require further justification.\n\nFirst, bubble-up functionality (#3) seems like it will be critical to support if\nwe intend to stop calling die(). This can be done either exhaustively (by\nmanually checking each error to see if it is fatal, and if so, invoking a\ncontrol-flow action), or by a macro. I couldn’t find any established conventions\nfor doing this in C, and the original patch series doesn’t really address this\nwell either since it requires a manual `if (...)` check, so it is an area for\nfurther exploration. (Actually another proposed alternative was `longjmp`, but\nit sounds like that is a can of worms to be avoided.)\n\nBack to the overall approach. To contrast to one alternative, error reporting\nfunctions a la the intriguing thread by Peff:\n\nhttps://lore.kernel.org/git/20160927191955.mympqgylrxhkp24n@sigill.intra.peff.net/\n\nWith that pattern, there is no concept of an overall status, either pass or\nfail. One must assume that if there was an error, then the operation failed, and\nif no error, then it passed. But then there is a question of whether an error\nwas fatal. We know that some errors are fatal (meaning an immediate halt of\nexecution is in order) and others are not. If we implemented this approach we\nwould probably need to add this bool is_fatal as a field to struct\nerror_context.\n\nAlso, unless a macro is employed, the reporting of errors is not coupled with\ncontrol flow (#2) unlike promise, since PROMISE_THROW() invokes a return\nstatement. Of course, Peff’s code there was just a sketch instead of a\nfull-blown patch, but not coupling the two could easily lead to bugs in the form\nof accidentally-omitted return statements, so perhaps a macro is in order if\nthat pattern is implemented.\n\nThe lifecycle of anything in void *data poses its own challenges. While promise\noffers macros to handle the strbuf release at EOL, special handlers would need\nto be written to free anything stored in void *data. Which brings me to this\nfeedback:\n\n> 1) It is hard to see how we can wrap the return value of the function in a\n> promise and preserve type safety. Even if we used some  kind of union the\n> compiler will not warn us if the caller reads a different member to the one\n> that the callee set.\n>\n> 2) It obscures the return type of the function and forces callers to unpack\n> the value from the promise adding complexity to the calling code.\n\nI should clarify that the promise concept is not intended to expand in scope as\nfar as the return value. It should remain as `int` and only ever `int`, so no\nreturn value would be anything other than a simple number, meaning AFAIK (1)\nshould not apply. For (2), if auxiliary data structures (additional ints, or\nstructs, etc.) are outputs, they would need to be via \"output parameters\" passed\ninto the function. Those output parameters should not need to be unpacked, since\ntheir type is preserved in the normal way.\n\nHowever to return to void *data in struct error_context, the unpacking *would*\nbe necessary since *data doesn’t have a concrete type. Therefore, if we go with\nstruct error_context, the *data field should be omitted since it adds complexity\nwithout need.\n\nSince we are dropping async support, I am going to rename the topic from\n`Promise` to `Result` as well, since the idea is equivalent to `Result<T, E>` in\nRust, aka `Either a b` in Haskell.\n\nRegarding error messages, totally valid criticism that they are confusing to the\nuser as posed in the original patchset. Eventually perhaps the messages could be\nreworded to increase clarity, but to avoid scope creep, let’s just not show\nthem. On the topic of scope, I agree with what I believe Phillip Wood implied,\nthat excessive refactoring should not be required to adopt this pattern. As such\nI will keep this in mind in the next iteration.\n\nThere is more to discuss about strings vs error codes and the future of error\ncodes as well as memory allocation, but this is probably enough for now. I may\nbe interested to try out the error_context as well as a concept next just to see\nwhat it would be like, but do people agree with the differential analysis so far\nand changes proposed? And curious if you have any other thoughts?\n\n- Philip\n\nOn Wed, Feb 21, 2024 at 1:03 PM Jeff King <peff@peff.net> wrote:\n>\n> On Tue, Feb 20, 2024 at 01:19:16PM +0100, Patrick Steinhardt wrote:\n>\n> > While we're already at it throwing ideas around, I also have to wonder\n> > whether this would be a long-term solution towards computer-friendly\n> > errors. One of the problems we quite frequently hit in Gitaly is that we\n> > are forced to parse error messages in order to figure out why exactly\n> > something has failed. Needless to say, this is quite fragile and also\n> > feels very wrong.\n> >\n> > Now if we had a more structured way to pass errors around this might\n> > also enable us to convey more meaning to the caller of Git commands. In\n> > a hypothetical world where all errors were using an explicit error type,\n> > then this error type could eventually become richer and contain more\n> > information that is relevant to the calling script. And if such rich\n> > error information was available, then it would not be that far fetched\n> > to ask Git to emit errors in a computer-parsable format like for example\n> > JSON.\n>\n> I think what I'm proposing (and if I understand correctly what Phillip\n> was thinking) is somewhat orthogonal. I agree that structured errors are\n> nice for computers to read. But it does open up a pretty big can of\n> worms regarding classifying each error, especially as root causes are\n> often multi-level.\n>\n> For example, imagine that the caller asks to resolve a ref. We might get\n> a syscall error opening the loose ref. Or we might get one opening the\n> packed-refs file (in a reftable world, you might imagine errors opening\n> various other files). What is the structured error? Obviously it is \"we\n> can't resolve that ref\" at some level. But the caller might want to know\n> about the failed open (whether it is just ENOENT, or if we ran into\n> EPERM or even EIO).\n>\n> Or looking at higher levels; if I ask for the merge base between A and\n> B, but one of those can't be resolved, how do we communicate that error?\n> It is some sort of \"cannot resolve\" error, but it needs to be\n> parameterized to know which is which.\n>\n> All of those questions can be answered, of course, but now we are\n> developing a micro-format that lets us describe all errors in a\n> standardized way. And I think that is going to put a burden on the code\n> which is generating the errors (and potentially on the consumers, too,\n> if they have to decipher the structure to figure out what they want).\n>\n> Whereas what I was really advocating for is punting on the structured\n> thing entirely. We keep our unstructured string errors for the most\n> part, but we simply let the caller pass in a context that tells us what\n> to do with them. That lets us keep providing specific error messages\n> from low-level functions without printing to stderr or exiting, which\n> higher-level code (especially lib-ified code) would not want.\n>\n> I think it could also be the first building block for making more\n> structured errors (since those low-level callers are free to provide\n> lots of details), but it doesn't have to be.\n>\n> -Peff\n"}]}