{"thread":{"id":"61669","subject":"[PATCH 0/5] Tighten patch header parsing in patch-id","startedAt":"2024-06-21T23:18:29Z","lastAt":"2024-07-30T05:12:52Z","messageCount":20,"participants":["Junio C Hamano","Patrick Steinhardt"],"isPatch":true,"patchVersion":1,"patchTotal":5},"messages":[{"id":"497526","messageId":"20240621231826.3280338-1-gitster@pobox.com","threadId":"61669","inReplyTo":null,"subject":"[PATCH 0/5] Tighten patch header parsing in patch-id","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-06-21T23:18:21Z","receivedAt":"2024-06-21T23:18:29Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"The patch-id command loops over a series of patches, picking up the\norigin commit object name (which is on the \"patch header\" line) and\nthen computing the patch identifier out of the \"patch\" (series of\n\"diff\") that follows the \"patch header\".\n\nThe parser is structured in a bit \"strange\" way.  It repeatedly\ncalls a single helper function get_one_patchid() that returns when a\npatch header is recognised, or skips until the \"patch\" part begins\nand then computes the \"patch id\" over the \"patch\" part, until it\nsees a patch header.  The caller knows that it gets just the \"patch\nheader\" for the first patch with its first call, and the second call\nis about computing the patch id for the first patch, whose\noriginating commit was obtained from the first call, etc.\n\nDuring the second and subsequent call (i.e. after finding a patch\nheader which caused the get_one_patchid() to return, calling the\nhelper again, expecting it to skip the commit log and find the patch\nfor which we are asked to compute the patch id), we shouldn't look\nfor the patch header at all.  Otherwise, a line that looks like a\npatch header in the log message can easily be mistaken to be the\nbeginning of a new patch header, as if the current message did not\nhave any patch text.\n\nThis 5-patch series is organized as follows:\n\n - patch 1 is about setting the baseline.  We need to recognise the\n   patch header produced by format-patch, log, and diff-tree --stdin.\n\n - patch 2 to patch 4 are bit of code restructuring without changing\n   the behaviour.\n\n - patch 5 stops looking for a patch header when we shouldn't, and\n   adds tests.\n\nJunio C Hamano (5):\n  t4204: patch-id supports various input format\n  patch-id: call flush_current_id() only when needed\n  patch-id: make get_one_patchid() more extensible\n  patch-id: rewrite code that detects the beginning of a patch\n  patch-id: tighten code to detect the patch header\n\n builtin/patch-id.c  | 82 +++++++++++++++++++++++++++++++++------------\n t/t4204-patch-id.sh | 40 ++++++++++++++++++++++\n 2 files changed, 101 insertions(+), 21 deletions(-)\n\n-- \n2.45.2-786-g49444cbe9a\n\n"},{"id":"497527","messageId":"20240621231826.3280338-2-gitster@pobox.com","threadId":"61669","inReplyTo":"20240621231826.3280338-1-gitster@pobox.com","subject":"[PATCH 1/5] t4204: patch-id supports various input format","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-06-21T23:18:22Z","receivedAt":"2024-06-21T23:18:30Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"git patch-id\" was first developed to read from \"git diff-tree\n--stdin -p\" output.  Later it was enhanced to read from \"git\ndiff-tree --stdin -p -v\", which was the downstream of an early\nimitation of \"git log\" (\"git rev-list\" run in the upstream of a pipe\nto feed the \"diff-tree\").  These days, we also read from \"git\nformat-patch\".\n\nTheir output begins slightly differently, but the patch-id computed\nover them for the same commit should be the same.  Ensure that we\nwon't accidentally break this expectation.\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n t/t4204-patch-id.sh | 23 +++++++++++++++++++++++\n 1 file changed, 23 insertions(+)\n\ndiff --git a/t/t4204-patch-id.sh b/t/t4204-patch-id.sh\nindex a7fa94ce0a..1627fdda1b 100755\n--- a/t/t4204-patch-id.sh\n+++ b/t/t4204-patch-id.sh\n@@ -114,6 +114,29 @@ test_expect_success 'patch-id supports git-format-patch output' '\n \ttest \"$2\" = $(git rev-parse HEAD)\n '\n \n+test_expect_success 'patch-id computes the same for various formats' '\n+\t# This test happens to consider \"git log -p -1\" output\n+\t# the canonical input format, so use it as the norm.\n+\tgit log -1 -p same >log-p.output &&\n+\tgit patch-id <log-p.output >expect &&\n+\n+\t# format-patch begins with \"From <commit object name>\"\n+\tgit format-patch -1 --stdout same >format-patch.output &&\n+\tgit patch-id <format-patch.output >actual &&\n+\ttest_cmp actual expect &&\n+\n+\t# \"diff-tree --stdin -p\" begins with \"<commit object name>\"\n+\tsame=$(git rev-parse same) &&\n+\techo $same | git diff-tree --stdin -p >diff-tree.output &&\n+\tgit patch-id <diff-tree.output >actual &&\n+\ttest_cmp actual expect &&\n+\n+\t# \"diff-tree --stdin -v -p\" begins with \"commit <commit object name>\"\n+\techo $same | git diff-tree --stdin -p -v >diff-tree-v.output &&\n+\tgit patch-id <diff-tree-v.output >actual &&\n+\ttest_cmp actual expect\n+'\n+\n test_expect_success 'whitespace is irrelevant in footer' '\n \tget_patch_id main &&\n \tgit checkout same &&\n-- \n2.45.2-786-g49444cbe9a\n\n"},{"id":"497528","messageId":"20240621231826.3280338-3-gitster@pobox.com","threadId":"61669","inReplyTo":"20240621231826.3280338-1-gitster@pobox.com","subject":"[PATCH 2/5] patch-id: call flush_current_id() only when needed","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-06-21T23:18:23Z","receivedAt":"2024-06-21T23:18:36Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"The caller passes a flag that is used to become no-op when calling\nflush_current_id().  Instead of calling something that becomes a\nno-op, teach the caller not to call it in the first place.\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n builtin/patch-id.c | 8 ++++----\n 1 file changed, 4 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/patch-id.c b/builtin/patch-id.c\nindex 3894d2b970..0f262e7a03 100644\n--- a/builtin/patch-id.c\n+++ b/builtin/patch-id.c\n@@ -6,10 +6,9 @@\n #include \"hex.h\"\n #include \"parse-options.h\"\n \n-static void flush_current_id(int patchlen, struct object_id *id, struct object_id *result)\n+static void flush_current_id(struct object_id *id, struct object_id *result)\n {\n-\tif (patchlen)\n-\t\tprintf(\"%s %s\\n\", oid_to_hex(result), oid_to_hex(id));\n+\tprintf(\"%s %s\\n\", oid_to_hex(result), oid_to_hex(id));\n }\n \n static int remove_space(char *line)\n@@ -181,7 +180,8 @@ static void generate_id_list(int stable, int verbatim)\n \toidclr(&oid);\n \twhile (!feof(stdin)) {\n \t\tpatchlen = get_one_patchid(&n, &result, &line_buf, stable, verbatim);\n-\t\tflush_current_id(patchlen, &oid, &result);\n+\t\tif (patchlen)\n+\t\t\tflush_current_id(&oid, &result);\n \t\toidcpy(&oid, &n);\n \t}\n \tstrbuf_release(&line_buf);\n-- \n2.45.2-786-g49444cbe9a\n\n"},{"id":"497529","messageId":"20240621231826.3280338-4-gitster@pobox.com","threadId":"61669","inReplyTo":"20240621231826.3280338-1-gitster@pobox.com","subject":"[PATCH 3/5] patch-id: make get_one_patchid() more extensible","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-06-21T23:18:24Z","receivedAt":"2024-06-21T23:18:38Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"We pass two independent Boolean flags (i.e. do we want the stable\nvariant of patch-id?  do we want to hash the stuff verbatim?) into\nthe function as two separate parameters.  Before adding the third\none and make the interface even wider, let's consolidate them into\na single flag word.\n\nNo changes in behaviour.  Just a trivial interface change.\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n builtin/patch-id.c | 20 +++++++++++++++-----\n 1 file changed, 15 insertions(+), 5 deletions(-)\n\ndiff --git a/builtin/patch-id.c b/builtin/patch-id.c\nindex 0f262e7a03..128e0997d8 100644\n--- a/builtin/patch-id.c\n+++ b/builtin/patch-id.c\n@@ -58,9 +58,14 @@ static int scan_hunk_header(const char *p, int *p_before, int *p_after)\n \treturn 1;\n }\n \n+#define GOPID_STABLE   01\n+#define GOPID_VERBATIM 02\n+\n static int get_one_patchid(struct object_id *next_oid, struct object_id *result,\n-\t\t\t   struct strbuf *line_buf, int stable, int verbatim)\n+\t\t\t   struct strbuf *line_buf, unsigned flags)\n {\n+\tint stable = flags & GOPID_STABLE;\n+\tint verbatim = flags & GOPID_VERBATIM;\n \tint patchlen = 0, found_next = 0;\n \tint before = -1, after = -1;\n \tint diff_is_binary = 0;\n@@ -171,7 +176,7 @@ static int get_one_patchid(struct object_id *next_oid, struct object_id *result,\n \treturn patchlen;\n }\n \n-static void generate_id_list(int stable, int verbatim)\n+static void generate_id_list(unsigned flags)\n {\n \tstruct object_id oid, n, result;\n \tint patchlen;\n@@ -179,7 +184,7 @@ static void generate_id_list(int stable, int verbatim)\n \n \toidclr(&oid);\n \twhile (!feof(stdin)) {\n-\t\tpatchlen = get_one_patchid(&n, &result, &line_buf, stable, verbatim);\n+\t\tpatchlen = get_one_patchid(&n, &result, &line_buf, flags);\n \t\tif (patchlen)\n \t\t\tflush_current_id(&oid, &result);\n \t\toidcpy(&oid, &n);\n@@ -218,6 +223,7 @@ int cmd_patch_id(int argc, const char **argv, const char *prefix)\n \t/* if nothing is set, default to unstable */\n \tstruct patch_id_opts config = {0, 0};\n \tint opts = 0;\n+\tunsigned flags = 0;\n \tstruct option builtin_patch_id_options[] = {\n \t\tOPT_CMDMODE(0, \"unstable\", &opts,\n \t\t    N_(\"use the unstable patch-id algorithm\"), 1),\n@@ -237,7 +243,11 @@ int cmd_patch_id(int argc, const char **argv, const char *prefix)\n \targc = parse_options(argc, argv, prefix, builtin_patch_id_options,\n \t\t\t     patch_id_usage, 0);\n \n-\tgenerate_id_list(opts ? opts > 1 : config.stable,\n-\t\t\t opts ? opts == 3 : config.verbatim);\n+\tif (opts ? opts > 1 : config.stable)\n+\t\tflags |= GOPID_STABLE;\n+\tif (opts ? opts == 3 : config.verbatim)\n+\t\tflags |= GOPID_VERBATIM;\n+\tgenerate_id_list(flags);\n+\n \treturn 0;\n }\n-- \n2.45.2-786-g49444cbe9a\n\n"},{"id":"497530","messageId":"20240621231826.3280338-6-gitster@pobox.com","threadId":"61669","inReplyTo":"20240621231826.3280338-1-gitster@pobox.com","subject":"[PATCH 5/5] patch-id: tighten code to detect the patch header","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-06-21T23:18:26Z","receivedAt":"2024-06-21T23:18:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"The get_one_patchid() function unconditionally takes a line that\nmatches the patch header (namely, a line that begins with a full\nobject name, possibly prefixed by \"commit\" or \"From\" plus a space)\nas the beginning of a patch.  Even when it is *not* looking for one\n(namely, when the previous call found the patch header and returned,\nand then we are called again to skip the log message and process the\npatch whose header was found by the previous invocation).\n\nAs a consequence, a line in the commit log message that begins with\none of these patterns can be mistaken to start another patch, with\ncurrent message entirely skipped (because we haven't even reached\nthe patch at all).\n\nAllow the caller to tell us if it called us already and saw the\npatch header (in which case we shouldn't be looking for another one,\nuntil we see the \"diff\" part of the patch; instead we simply should\nbe skipping these lines as part of the commit log message), and skip\nthe header processing logic when that is the case.  In the helper\nfunction, it also needs to flip this \"are we looking for a header?\"\nbit, once it finished skipping the commit log message and started\nprocessing the patches, as the patch header of the _next_ message is\nthe only clue in the input that the current patch is done.\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n builtin/patch-id.c  | 43 ++++++++++++++++++++++++++++++-------------\n t/t4204-patch-id.sh | 17 +++++++++++++++++\n 2 files changed, 47 insertions(+), 13 deletions(-)\n\ndiff --git a/builtin/patch-id.c b/builtin/patch-id.c\nindex a649966f31..0e6aab1ca2 100644\n--- a/builtin/patch-id.c\n+++ b/builtin/patch-id.c\n@@ -60,12 +60,14 @@ static int scan_hunk_header(const char *p, int *p_before, int *p_after)\n \n #define GOPID_STABLE   01\n #define GOPID_VERBATIM 02\n+#define GOPID_FIND_HEADER 04\n \n static int get_one_patchid(struct object_id *next_oid, struct object_id *result,\n \t\t\t   struct strbuf *line_buf, unsigned flags)\n {\n \tint stable = flags & GOPID_STABLE;\n \tint verbatim = flags & GOPID_VERBATIM;\n+\tint find_header = flags & GOPID_FIND_HEADER;\n \tint patchlen = 0, found_next = 0;\n \tint before = -1, after = -1;\n \tint diff_is_binary = 0;\n@@ -81,26 +83,39 @@ static int get_one_patchid(struct object_id *next_oid, struct object_id *result,\n \t\tint len;\n \n \t\t/*\n-\t\t * If we see a line that begins with \"<object name>\",\n-\t\t * \"commit <object name>\" or \"From <object name>\", it is\n-\t\t * the beginning of a patch.  Return to the caller, as\n-\t\t * we are done with the one we have been processing.\n+\t\t * The caller hasn't seen us find a patch header and\n+\t\t * return to it, or we have started processing patch\n+\t\t * and may encounter the beginning of the next patch.\n \t\t */\n-\t\tif (skip_prefix(line, \"commit \", &p))\n-\t\t\t;\n-\t\telse if (skip_prefix(line, \"From \", &p))\n-\t\t\t;\n-\t\tif (!get_oid_hex(p, next_oid)) {\n-\t\t\tif (verbatim)\n-\t\t\t\tthe_hash_algo->update_fn(&ctx, line, strlen(line));\n-\t\t\tfound_next = 1;\n-\t\t\tbreak;\n+\t\tif (find_header) {\n+\t\t\t/*\n+\t\t\t * If we see a line that begins with \"<object name>\",\n+\t\t\t * \"commit <object name>\" or \"From <object name>\", it is\n+\t\t\t * the beginning of a patch.  Return to the caller, as\n+\t\t\t * we are done with the one we have been processing.\n+\t\t\t */\n+\t\t\tif (skip_prefix(line, \"commit \", &p))\n+\t\t\t\t;\n+\t\t\telse if (skip_prefix(line, \"From \", &p))\n+\t\t\t\t;\n+\t\t\tif (!get_oid_hex(p, next_oid)) {\n+\t\t\t\tif (verbatim)\n+\t\t\t\t\tthe_hash_algo->update_fn(&ctx, line, strlen(line));\n+\t\t\t\tfound_next = 1;\n+\t\t\t\tbreak;\n+\t\t\t}\n \t\t}\n \n \t\t/* Ignore commit comments */\n \t\tif (!patchlen && !starts_with(line, \"diff \"))\n \t\t\tcontinue;\n \n+\t\t/*\n+\t\t * We are past the commit log message.  Prepare to\n+\t\t * stop at the beginning of the next patch header.\n+\t\t */\n+\t\tfind_header = 1;\n+\n \t\t/* Parsing diff header?  */\n \t\tif (before == -1) {\n \t\t\tif (starts_with(line, \"GIT binary patch\") ||\n@@ -196,11 +211,13 @@ static void generate_id_list(unsigned flags)\n \tstruct strbuf line_buf = STRBUF_INIT;\n \n \toidclr(&oid);\n+\tflags |= GOPID_FIND_HEADER;\n \twhile (!feof(stdin)) {\n \t\tpatchlen = get_one_patchid(&n, &result, &line_buf, flags);\n \t\tif (patchlen)\n \t\t\tflush_current_id(&oid, &result);\n \t\toidcpy(&oid, &n);\n+\t\tflags &= ~GOPID_FIND_HEADER;\n \t}\n \tstrbuf_release(&line_buf);\n }\ndiff --git a/t/t4204-patch-id.sh b/t/t4204-patch-id.sh\nindex 1627fdda1b..b1d98d4110 100755\n--- a/t/t4204-patch-id.sh\n+++ b/t/t4204-patch-id.sh\n@@ -137,6 +137,23 @@ test_expect_success 'patch-id computes the same for various formats' '\n \ttest_cmp actual expect\n '\n \n+hash=$(git rev-parse same:)\n+for cruft in \"$hash\" \"commit $hash is bad\" \"From $hash status\"\n+do\n+\ttest_expect_success \"patch-id with <$cruft> in log message\" '\n+\t\tgit format-patch -1 --stdout same >patch-0 &&\n+\t\tgit patch-id <patch-0 >expect &&\n+\n+\t\t{\n+\t\t\tsed -e \"/^$/q\" patch-0 &&\n+\t\t\tprintf \"random message\\n%s\\n\\n\" \"$cruft\" &&\n+\t\t\tsed -e \"1,/^$/d\" patch-0\n+\t\t} >patch-cruft &&\n+\t\tgit patch-id <patch-cruft >actual &&\n+\t\ttest_cmp actual expect\n+\t'\n+done\n+\n test_expect_success 'whitespace is irrelevant in footer' '\n \tget_patch_id main &&\n \tgit checkout same &&\n-- \n2.45.2-786-g49444cbe9a\n\n"},{"id":"497531","messageId":"20240621231826.3280338-5-gitster@pobox.com","threadId":"61669","inReplyTo":"20240621231826.3280338-1-gitster@pobox.com","subject":"[PATCH 4/5] patch-id: rewrite code that detects the beginning of a patch","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-06-21T23:18:25Z","receivedAt":"2024-06-21T23:18:45Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"The get_one_patchid() function reads input lines until it finds a\npatch header (the line that begins a patch), whose beginning is one\nof:\n\n (1) an \"<object name>\", which is \"git diff-tree --stdin\" shows;\n (2) \"commit <object name>\", which is \"git log\" shows; or\n (3) \"From <object name>\",  which is \"git log --format=email\" gives.\n\nWhen it finds such a line, it returns to the caller, reporting the\n<object name> it found, and the size of the \"patch\" it processed.\n\nThe caller then calls the function again, which then ignores the\ncommit log message, and then processes the lines in the patch part\nuntil it hits another \"beginning of a patch\".\n\nThe above logic was fairly easy to see until 2bb73ae8 (patch-id: use\nstarts_with() and skip_prefix(), 2016-05-28) reorganized the code,\nwhich made another logic that has nothing to do with the \"where does\nthe next patch begin?\" logic, which came from 2485eab5\n(git-patch-id: do not trip over \"no newline\" markers, 2011-02-17)\nthat ignores the \"\\ No newline at the end\", rolled into the same\nsingle if() statement.\n\nLet's split it out.  The \"\\ No newline at the end\" marker is part of\nthe patch, should not appear before we start reading the patch part,\nand does not belong to the detection of patch header.\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n builtin/patch-id.c | 29 +++++++++++++++++++++--------\n 1 file changed, 21 insertions(+), 8 deletions(-)\n\ndiff --git a/builtin/patch-id.c b/builtin/patch-id.c\nindex 128e0997d8..a649966f31 100644\n--- a/builtin/patch-id.c\n+++ b/builtin/patch-id.c\n@@ -80,16 +80,19 @@ static int get_one_patchid(struct object_id *next_oid, struct object_id *result,\n \t\tconst char *p = line;\n \t\tint len;\n \n-\t\t/* Possibly skip over the prefix added by \"log\" or \"format-patch\" */\n-\t\tif (!skip_prefix(line, \"commit \", &p) &&\n-\t\t    !skip_prefix(line, \"From \", &p) &&\n-\t\t    starts_with(line, \"\\\\ \") && 12 < strlen(line)) {\n+\t\t/*\n+\t\t * If we see a line that begins with \"<object name>\",\n+\t\t * \"commit <object name>\" or \"From <object name>\", it is\n+\t\t * the beginning of a patch.  Return to the caller, as\n+\t\t * we are done with the one we have been processing.\n+\t\t */\n+\t\tif (skip_prefix(line, \"commit \", &p))\n+\t\t\t;\n+\t\telse if (skip_prefix(line, \"From \", &p))\n+\t\t\t;\n+\t\tif (!get_oid_hex(p, next_oid)) {\n \t\t\tif (verbatim)\n \t\t\t\tthe_hash_algo->update_fn(&ctx, line, strlen(line));\n-\t\t\tcontinue;\n-\t\t}\n-\n-\t\tif (!get_oid_hex(p, next_oid)) {\n \t\t\tfound_next = 1;\n \t\t\tbreak;\n \t\t}\n@@ -130,6 +133,16 @@ static int get_one_patchid(struct object_id *next_oid, struct object_id *result,\n \t\t\t\tbreak;\n \t\t}\n \n+\t\t/*\n+\t\t * A hunk about an incomplete line may have this\n+\t\t * marker at the end, which should just be ignored.\n+\t\t */\n+\t\tif (starts_with(line, \"\\\\ \") && 12 < strlen(line)) {\n+\t\t\tif (verbatim)\n+\t\t\t\tthe_hash_algo->update_fn(&ctx, line, strlen(line));\n+\t\t\tcontinue;\n+\t\t}\n+\n \t\tif (diff_is_binary) {\n \t\t\tif (starts_with(line, \"diff \")) {\n \t\t\t\tdiff_is_binary = 0;\n-- \n2.45.2-786-g49444cbe9a\n\n"},{"id":"499507","messageId":"ZqeE2IaIhiWmpVM4@tanuki","threadId":"61669","inReplyTo":"20240621231826.3280338-4-gitster@pobox.com","subject":"Re: [PATCH 3/5] patch-id: make get_one_patchid() more extensible","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-07-29T12:02:32Z","receivedAt":"2024-07-29T12:02:50Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Fri, Jun 21, 2024 at 04:18:24PM -0700, Junio C Hamano wrote:\n> We pass two independent Boolean flags (i.e. do we want the stable\n> variant of patch-id?  do we want to hash the stuff verbatim?) into\n> the function as two separate parameters.  Before adding the third\n> one and make the interface even wider, let's consolidate them into\n> a single flag word.\n> \n> No changes in behaviour.  Just a trivial interface change.\n> \n> Signed-off-by: Junio C Hamano <gitster@pobox.com>\n> ---\n>  builtin/patch-id.c | 20 +++++++++++++++-----\n>  1 file changed, 15 insertions(+), 5 deletions(-)\n> \n> diff --git a/builtin/patch-id.c b/builtin/patch-id.c\n> index 0f262e7a03..128e0997d8 100644\n> --- a/builtin/patch-id.c\n> +++ b/builtin/patch-id.c\n> @@ -58,9 +58,14 @@ static int scan_hunk_header(const char *p, int *p_before, int *p_after)\n>  \treturn 1;\n>  }\n>  \n> +#define GOPID_STABLE   01\n> +#define GOPID_VERBATIM 02\n> +\n\nThis certainly is a worthwhile change. I have to wonder about code style\nthough:\n\n  - Using 01 and 02 as constants feels somewhat weird to me. Don't we\n    typically use `(1 << 0)` and `(1 << 1)` for such binary flags?\n\n  - What is our preferred style nowadays? Do we prefer defines over\n    enums? I rather had the feeling that enums are the go-to style for\n    things like this nowadays.\n\nIt would also be nice to have documentation for the flags.\n\nIn any case, all of these are really just smallish nits and I think that\nthis is a strict improvement regardless of whether we massage the style\nor not.\n\n> @@ -237,7 +243,11 @@ int cmd_patch_id(int argc, const char **argv, const char *prefix)\n>  \targc = parse_options(argc, argv, prefix, builtin_patch_id_options,\n>  \t\t\t     patch_id_usage, 0);\n>  \n> -\tgenerate_id_list(opts ? opts > 1 : config.stable,\n> -\t\t\t opts ? opts == 3 : config.verbatim);\n> +\tif (opts ? opts > 1 : config.stable)\n> +\t\tflags |= GOPID_STABLE;\n> +\tif (opts ? opts == 3 : config.verbatim)\n> +\t\tflags |= GOPID_VERBATIM;\n\nI was wondering whether we could use `OPT_BIT()` here to set those as\nflags directly. I guess that would require a bit more refactoring, but\nif we also converted `struct patch_id_opts` to have a `flags` field then\nthis might overall be easier to read than the weird massaging of opts\nthat we did before and after your change.\n\nPatrick\n"},{"id":"499508","messageId":"ZqeE_GHh1adyK5PM@tanuki","threadId":"61669","inReplyTo":"20240621231826.3280338-5-gitster@pobox.com","subject":"Re: [PATCH 4/5] patch-id: rewrite code that detects the beginning of a patch","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-07-29T12:03:08Z","receivedAt":"2024-07-29T12:03:59Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Fri, Jun 21, 2024 at 04:18:25PM -0700, Junio C Hamano wrote:\n> The get_one_patchid() function reads input lines until it finds a\n> patch header (the line that begins a patch), whose beginning is one\n> of:\n> \n>  (1) an \"<object name>\", which is \"git diff-tree --stdin\" shows;\n>  (2) \"commit <object name>\", which is \"git log\" shows; or\n>  (3) \"From <object name>\",  which is \"git log --format=email\" gives.\n\nAll of these items should probably say \"which is what ...\", where \"what\"\nis what is missing.\n\nPatrick\n"},{"id":"499509","messageId":"ZqeGBJrqIrU2A4jl@tanuki","threadId":"61669","inReplyTo":"20240621231826.3280338-6-gitster@pobox.com","subject":"Re: [PATCH 5/5] patch-id: tighten code to detect the patch header","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-07-29T12:07:32Z","receivedAt":"2024-07-29T12:08:24Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Fri, Jun 21, 2024 at 04:18:26PM -0700, Junio C Hamano wrote:\n> @@ -196,11 +211,13 @@ static void generate_id_list(unsigned flags)\n>  \tstruct strbuf line_buf = STRBUF_INIT;\n>  \n>  \toidclr(&oid);\n> +\tflags |= GOPID_FIND_HEADER;\n>  \twhile (!feof(stdin)) {\n>  \t\tpatchlen = get_one_patchid(&n, &result, &line_buf, flags);\n>  \t\tif (patchlen)\n>  \t\t\tflush_current_id(&oid, &result);\n>  \t\toidcpy(&oid, &n);\n> +\t\tflags &= ~GOPID_FIND_HEADER;\n>  \t}\n\nI think I'm missing the obvious. But why don't we have to set\n`GOPID_FIND_HEADER` when we have flushed the current patch ID? Is this\nbecause we know that `get_one_patchid()` stops once it finds the next\nline starting with a commit? Makes me wonder what happens when there is\nnon-diff garbage between patches for which we are about to generate\npatch IDs.\n\nPatrick\n"},{"id":"499531","messageId":"xmqq34nshso8.fsf@gitster.g","threadId":"61669","inReplyTo":"ZqeE2IaIhiWmpVM4@tanuki","subject":"Re: [PATCH 3/5] patch-id: make get_one_patchid() more extensible","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-07-29T20:03:35Z","receivedAt":"2024-07-29T20:03:41Z","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>> No changes in behaviour.  Just a trivial interface change.\n>> \n>> Signed-off-by: Junio C Hamano <gitster@pobox.com>\n>> ...\n>> +#define GOPID_STABLE   01\n>> +#define GOPID_VERBATIM 02\n>> +\n>\n> This certainly is a worthwhile change. I have to wonder about code style\n> though:\n>\n>   - Using 01 and 02 as constants feels somewhat weird to me. Don't we\n>     typically use `(1 << 0)` and `(1 << 1)` for such binary flags?\n>\n>   - What is our preferred style nowadays? Do we prefer defines over\n>     enums? I rather had the feeling that enums are the go-to style for\n>     things like this nowadays.\n>\n> It would also be nice to have documentation for the flags.\n\nFor an internal implementation detail that does not even cross file\nboundaries with descriptive _STABLE and _VERBATIM that corresponds\nto the member names of config structure?  I doubt it.\n\n> In any case, all of these are really just smallish nits and I think that\n> this is a strict improvement regardless of whether we massage the style\n> or not.\n> ...\n> I was wondering whether we could use `OPT_BIT()` here to set those as\n> flags directly. I guess that would require a bit more refactoring, but\n> if we also converted `struct patch_id_opts` to have a `flags` field then\n> this might overall be easier to read than the weird massaging of opts\n> that we did before and after your change.\n\nAs a longer direction, I envision that most of the implementation we\nsee in this file and what diff.c:diff_get_patch_id() does should be\nrefactored and one of them should just go.  So until that happens, I\nam inclined to keep the changes to this file to an absolute minimum.\n"},{"id":"499533","messageId":"xmqqwml4gdol.fsf@gitster.g","threadId":"61669","inReplyTo":"ZqeGBJrqIrU2A4jl@tanuki","subject":"Re: [PATCH 5/5] patch-id: tighten code to detect the patch header","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-07-29T20:12:42Z","receivedAt":"2024-07-29T20:12:47Z","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> On Fri, Jun 21, 2024 at 04:18:26PM -0700, Junio C Hamano wrote:\n>> @@ -196,11 +211,13 @@ static void generate_id_list(unsigned flags)\n>>  \tstruct strbuf line_buf = STRBUF_INIT;\n>>  \n>>  \toidclr(&oid);\n>> +\tflags |= GOPID_FIND_HEADER;\n>>  \twhile (!feof(stdin)) {\n>>  \t\tpatchlen = get_one_patchid(&n, &result, &line_buf, flags);\n>>  \t\tif (patchlen)\n>>  \t\t\tflush_current_id(&oid, &result);\n>>  \t\toidcpy(&oid, &n);\n>> +\t\tflags &= ~GOPID_FIND_HEADER;\n>>  \t}\n>\n> I think I'm missing the obvious. But why don't we have to set\n> `GOPID_FIND_HEADER` when we have flushed the current patch ID? Is this\n> because we know that `get_one_patchid()` stops once it finds the next\n> line starting with a commit?\n\nYup the original control flow is rather convoluted.  The first call\nstops when it finds the header that begins the log message part and\nreturns, but the subsequent calls are to (1) skip the log message\nand then (2) parse and hash the diff part, until it finds another\nheader that begins the log message part of the _next_ patch and\nreturn.  GOPID_FIND_HEADER bit is used to tell the callee when we\nhaven't found the header (hence we can stop at a line whose\nbeginning looks like a hash) or the previous round already found the\nheader and we positively know we are now in the \"skip the log\nmessage\" phase (hence a line whose beginning looks like a hash is\nnot a new header).\n\n> Makes me wonder what happens when there is\n> non-diff garbage between patches for which we are about to generate\n> patch IDs.\n\n\"Skip non-diff garbage until we see a patch\" is the mechanism used\nto skip the log message, so it would be a reasonable thing to skip\nsuch no-diff garbage between patches, no?\n"},{"id":"499543","messageId":"20240730011738.4032377-2-gitster@pobox.com","threadId":"61669","inReplyTo":"20240730011738.4032377-1-gitster@pobox.com","subject":"[PATCH v2 1/5] t4204: patch-id supports various input format","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-07-30T01:17:34Z","receivedAt":"2024-07-30T01:17:45Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"git patch-id\" was first developed to read from \"git diff-tree\n--stdin -p\" output.  Later it was enhanced to read from \"git\ndiff-tree --stdin -p -v\", which was the downstream of an early\nimitation of \"git log\" (\"git rev-list\" run in the upstream of a pipe\nto feed the \"diff-tree\").  These days, we also read from \"git\nformat-patch\".\n\nTheir output begins slightly differently, but the patch-id computed\nover them for the same commit should be the same.  Ensure that we\nwon't accidentally break this expectation.\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n t/t4204-patch-id.sh | 23 +++++++++++++++++++++++\n 1 file changed, 23 insertions(+)\n\ndiff --git a/t/t4204-patch-id.sh b/t/t4204-patch-id.sh\nindex a7fa94ce0a..1627fdda1b 100755\n--- a/t/t4204-patch-id.sh\n+++ b/t/t4204-patch-id.sh\n@@ -114,6 +114,29 @@ test_expect_success 'patch-id supports git-format-patch output' '\n \ttest \"$2\" = $(git rev-parse HEAD)\n '\n \n+test_expect_success 'patch-id computes the same for various formats' '\n+\t# This test happens to consider \"git log -p -1\" output\n+\t# the canonical input format, so use it as the norm.\n+\tgit log -1 -p same >log-p.output &&\n+\tgit patch-id <log-p.output >expect &&\n+\n+\t# format-patch begins with \"From <commit object name>\"\n+\tgit format-patch -1 --stdout same >format-patch.output &&\n+\tgit patch-id <format-patch.output >actual &&\n+\ttest_cmp actual expect &&\n+\n+\t# \"diff-tree --stdin -p\" begins with \"<commit object name>\"\n+\tsame=$(git rev-parse same) &&\n+\techo $same | git diff-tree --stdin -p >diff-tree.output &&\n+\tgit patch-id <diff-tree.output >actual &&\n+\ttest_cmp actual expect &&\n+\n+\t# \"diff-tree --stdin -v -p\" begins with \"commit <commit object name>\"\n+\techo $same | git diff-tree --stdin -p -v >diff-tree-v.output &&\n+\tgit patch-id <diff-tree-v.output >actual &&\n+\ttest_cmp actual expect\n+'\n+\n test_expect_success 'whitespace is irrelevant in footer' '\n \tget_patch_id main &&\n \tgit checkout same &&\n-- \n2.46.0-69-gd0749fd195\n\n"},{"id":"499544","messageId":"20240730011738.4032377-3-gitster@pobox.com","threadId":"61669","inReplyTo":"20240730011738.4032377-1-gitster@pobox.com","subject":"[PATCH v2 2/5] patch-id: call flush_current_id() only when needed","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-07-30T01:17:35Z","receivedAt":"2024-07-30T01:17:46Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"The caller passes a flag that is used to become no-op when calling\nflush_current_id().  Instead of calling something that becomes a\nno-op, teach the caller not to call it in the first place.\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n builtin/patch-id.c | 8 ++++----\n 1 file changed, 4 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/patch-id.c b/builtin/patch-id.c\nindex 3894d2b970..0f262e7a03 100644\n--- a/builtin/patch-id.c\n+++ b/builtin/patch-id.c\n@@ -6,10 +6,9 @@\n #include \"hex.h\"\n #include \"parse-options.h\"\n \n-static void flush_current_id(int patchlen, struct object_id *id, struct object_id *result)\n+static void flush_current_id(struct object_id *id, struct object_id *result)\n {\n-\tif (patchlen)\n-\t\tprintf(\"%s %s\\n\", oid_to_hex(result), oid_to_hex(id));\n+\tprintf(\"%s %s\\n\", oid_to_hex(result), oid_to_hex(id));\n }\n \n static int remove_space(char *line)\n@@ -181,7 +180,8 @@ static void generate_id_list(int stable, int verbatim)\n \toidclr(&oid);\n \twhile (!feof(stdin)) {\n \t\tpatchlen = get_one_patchid(&n, &result, &line_buf, stable, verbatim);\n-\t\tflush_current_id(patchlen, &oid, &result);\n+\t\tif (patchlen)\n+\t\t\tflush_current_id(&oid, &result);\n \t\toidcpy(&oid, &n);\n \t}\n \tstrbuf_release(&line_buf);\n-- \n2.46.0-69-gd0749fd195\n\n"},{"id":"499545","messageId":"20240730011738.4032377-1-gitster@pobox.com","threadId":"61669","inReplyTo":"20240621231826.3280338-1-gitster@pobox.com","subject":"[PATCH v2 0/5] Tighten patch header parsing in patch-id","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-07-30T01:17:33Z","receivedAt":"2024-07-30T01:17:47Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"    Updates from v1\n\n     - changed flag bits used internally from CPP macro to enum and\n       added a bit of comments.\n\nThe patch-id command loops over a series of patches, picking up the\norigin commit object name (which is on the \"patch header\" line) and\nthen computing the patch identifier out of the \"patch\" (series of\n\"diff\") that follows the \"patch header\".\n\nThe parser is structured in a bit \"strange\" way.  It repeatedly\ncalls a single helper function get_one_patchid() that returns when a\npatch header is recognised, or skips until the \"patch\" part begins\nand then computes the \"patch id\" over the \"patch\" part, until it\nsees a patch header.  The caller knows that it gets just the \"patch\nheader\" for the first patch with its first call, and the second call\nis about computing the patch id for the first patch, whose\noriginating commit was obtained from the first call, etc.\n\nDuring the second and subsequent call (i.e. after finding a patch\nheader which caused the get_one_patchid() to return, calling the\nhelper again, expecting it to skip the commit log and find the patch\nfor which we are asked to compute the patch id), we shouldn't look\nfor the patch header at all.  Otherwise, a line that looks like a\npatch header in the log message can easily be mistaken to be the\nbeginning of a new patch header, as if the current message did not\nhave any patch text.\n\nThis 5-patch series is organized as follows:\n\n - patch 1 is about setting the baseline.  We need to recognise the\n   patch header produced by format-patch, log, and diff-tree --stdin.\n\n - patch 2 to patch 4 are bit of code restructuring without changing\n   the behaviour.\n\n - patch 5 stops looking for a patch header when we shouldn't, and\n   adds tests.\n\n\nJunio C Hamano (5):\n  t4204: patch-id supports various input format\n  patch-id: call flush_current_id() only when needed\n  patch-id: make get_one_patchid() more extensible\n  patch-id: rewrite code that detects the beginning of a patch\n  patch-id: tighten code to detect the patch header\n\n builtin/patch-id.c  | 93 +++++++++++++++++++++++++++++++++++----------\n t/t4204-patch-id.sh | 40 +++++++++++++++++++\n 2 files changed, 112 insertions(+), 21 deletions(-)\n\nRange-diff against v1:\n1:  c1ff38c0b8 = 1:  e68a30f6c9 t4204: patch-id supports various input format\n2:  a201f344a6 = 2:  3afc63b210 patch-id: call flush_current_id() only when needed\n3:  237f8910ca ! 3:  22dd5e7a5b patch-id: make get_one_patchid() more extensible\n    @@ builtin/patch-id.c: static int scan_hunk_header(const char *p, int *p_before, in\n      \treturn 1;\n      }\n      \n    -+#define GOPID_STABLE   01\n    -+#define GOPID_VERBATIM 02\n    ++/*\n    ++ * flag bits to control get_one_patchid()'s behaviour.\n    ++ */\n    ++enum {\n    ++\tGOPID_STABLE = (1<<0),\t\t/* --stable */\n    ++\tGOPID_VERBATIM = (1<<1),\t/* --verbatim */\n    ++};\n     +\n      static int get_one_patchid(struct object_id *next_oid, struct object_id *result,\n     -\t\t\t   struct strbuf *line_buf, int stable, int verbatim)\n4:  d6d068c9dc = 4:  0cca1ed513 patch-id: rewrite code that detects the beginning of a patch\n5:  51af73722c ! 5:  ef422df7c1 patch-id: tighten code to detect the patch header\n    @@ Commit message\n      ## builtin/patch-id.c ##\n     @@ builtin/patch-id.c: static int scan_hunk_header(const char *p, int *p_before, int *p_after)\n      \n    - #define GOPID_STABLE   01\n    - #define GOPID_VERBATIM 02\n    -+#define GOPID_FIND_HEADER 04\n    + /*\n    +  * flag bits to control get_one_patchid()'s behaviour.\n    ++ *\n    ++ * STABLE/VERBATIM are given from the command line option as\n    ++ * --stable/--verbatim.  FIND_HEADER conveys the internal state\n    ++ * maintained by the caller to allow the function to avoid mistaking\n    ++ * lines of log message before seeing the \"diff\" part as the beginning\n    ++ * of the next patch.\n    +  */\n    + enum {\n    + \tGOPID_STABLE = (1<<0),\t\t/* --stable */\n    + \tGOPID_VERBATIM = (1<<1),\t/* --verbatim */\n    ++\tGOPID_FIND_HEADER = (1<<2),\t/* stop at the beginning of patch message */\n    + };\n      \n      static int get_one_patchid(struct object_id *next_oid, struct object_id *result,\n    - \t\t\t   struct strbuf *line_buf, unsigned flags)\n    +@@ builtin/patch-id.c: static int get_one_patchid(struct object_id *next_oid, struct object_id *result,\n      {\n      \tint stable = flags & GOPID_STABLE;\n      \tint verbatim = flags & GOPID_VERBATIM;\n-- \n2.46.0-69-gd0749fd195\n\n"},{"id":"499546","messageId":"20240730011738.4032377-4-gitster@pobox.com","threadId":"61669","inReplyTo":"20240730011738.4032377-1-gitster@pobox.com","subject":"[PATCH v2 3/5] patch-id: make get_one_patchid() more extensible","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-07-30T01:17:36Z","receivedAt":"2024-07-30T01:17:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"We pass two independent Boolean flags (i.e. do we want the stable\nvariant of patch-id?  do we want to hash the stuff verbatim?) into\nthe function as two separate parameters.  Before adding the third\none and make the interface even wider, let's consolidate them into\na single flag word.\n\nNo changes in behaviour.  Just a trivial interface change.\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n builtin/patch-id.c | 25 ++++++++++++++++++++-----\n 1 file changed, 20 insertions(+), 5 deletions(-)\n\ndiff --git a/builtin/patch-id.c b/builtin/patch-id.c\nindex 0f262e7a03..1d3b7ff12b 100644\n--- a/builtin/patch-id.c\n+++ b/builtin/patch-id.c\n@@ -58,9 +58,19 @@ static int scan_hunk_header(const char *p, int *p_before, int *p_after)\n \treturn 1;\n }\n \n+/*\n+ * flag bits to control get_one_patchid()'s behaviour.\n+ */\n+enum {\n+\tGOPID_STABLE = (1<<0),\t\t/* --stable */\n+\tGOPID_VERBATIM = (1<<1),\t/* --verbatim */\n+};\n+\n static int get_one_patchid(struct object_id *next_oid, struct object_id *result,\n-\t\t\t   struct strbuf *line_buf, int stable, int verbatim)\n+\t\t\t   struct strbuf *line_buf, unsigned flags)\n {\n+\tint stable = flags & GOPID_STABLE;\n+\tint verbatim = flags & GOPID_VERBATIM;\n \tint patchlen = 0, found_next = 0;\n \tint before = -1, after = -1;\n \tint diff_is_binary = 0;\n@@ -171,7 +181,7 @@ static int get_one_patchid(struct object_id *next_oid, struct object_id *result,\n \treturn patchlen;\n }\n \n-static void generate_id_list(int stable, int verbatim)\n+static void generate_id_list(unsigned flags)\n {\n \tstruct object_id oid, n, result;\n \tint patchlen;\n@@ -179,7 +189,7 @@ static void generate_id_list(int stable, int verbatim)\n \n \toidclr(&oid);\n \twhile (!feof(stdin)) {\n-\t\tpatchlen = get_one_patchid(&n, &result, &line_buf, stable, verbatim);\n+\t\tpatchlen = get_one_patchid(&n, &result, &line_buf, flags);\n \t\tif (patchlen)\n \t\t\tflush_current_id(&oid, &result);\n \t\toidcpy(&oid, &n);\n@@ -218,6 +228,7 @@ int cmd_patch_id(int argc, const char **argv, const char *prefix)\n \t/* if nothing is set, default to unstable */\n \tstruct patch_id_opts config = {0, 0};\n \tint opts = 0;\n+\tunsigned flags = 0;\n \tstruct option builtin_patch_id_options[] = {\n \t\tOPT_CMDMODE(0, \"unstable\", &opts,\n \t\t    N_(\"use the unstable patch-id algorithm\"), 1),\n@@ -237,7 +248,11 @@ int cmd_patch_id(int argc, const char **argv, const char *prefix)\n \targc = parse_options(argc, argv, prefix, builtin_patch_id_options,\n \t\t\t     patch_id_usage, 0);\n \n-\tgenerate_id_list(opts ? opts > 1 : config.stable,\n-\t\t\t opts ? opts == 3 : config.verbatim);\n+\tif (opts ? opts > 1 : config.stable)\n+\t\tflags |= GOPID_STABLE;\n+\tif (opts ? opts == 3 : config.verbatim)\n+\t\tflags |= GOPID_VERBATIM;\n+\tgenerate_id_list(flags);\n+\n \treturn 0;\n }\n-- \n2.46.0-69-gd0749fd195\n\n"},{"id":"499547","messageId":"20240730011738.4032377-5-gitster@pobox.com","threadId":"61669","inReplyTo":"20240730011738.4032377-1-gitster@pobox.com","subject":"[PATCH v2 4/5] patch-id: rewrite code that detects the beginning of a patch","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-07-30T01:17:37Z","receivedAt":"2024-07-30T01:17:52Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"The get_one_patchid() function reads input lines until it finds a\npatch header (the line that begins a patch), whose beginning is one\nof:\n\n (1) an \"<object name>\", which is what \"git diff-tree --stdin\" shows;\n (2) \"commit <object name>\", which is what \"git log\" shows; or\n (3) \"From <object name>\",  which is what \"git log --format=email\" shows.\n\nWhen it finds such a line, it returns to the caller, reporting the\n<object name> it found, and the size of the \"patch\" it processed.\n\nThe caller then calls the function again, which then ignores the\ncommit log message, and then processes the lines in the patch part\nuntil it hits another \"beginning of a patch\".\n\nThe above logic was fairly easy to see until 2bb73ae8 (patch-id: use\nstarts_with() and skip_prefix(), 2016-05-28) reorganized the code,\nwhich made another logic that has nothing to do with the \"where does\nthe next patch begin?\" logic, which came from 2485eab5\n(git-patch-id: do not trip over \"no newline\" markers, 2011-02-17)\nthat ignores the \"\\ No newline at the end\", rolled into the same\nsingle if() statement.\n\nLet's split it out.  The \"\\ No newline at the end\" marker is part of\nthe patch, should not appear before we start reading the patch part,\nand does not belong to the detection of patch header.\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n builtin/patch-id.c | 29 +++++++++++++++++++++--------\n 1 file changed, 21 insertions(+), 8 deletions(-)\n\ndiff --git a/builtin/patch-id.c b/builtin/patch-id.c\nindex 1d3b7ff12b..a2fdc0505d 100644\n--- a/builtin/patch-id.c\n+++ b/builtin/patch-id.c\n@@ -85,16 +85,19 @@ static int get_one_patchid(struct object_id *next_oid, struct object_id *result,\n \t\tconst char *p = line;\n \t\tint len;\n \n-\t\t/* Possibly skip over the prefix added by \"log\" or \"format-patch\" */\n-\t\tif (!skip_prefix(line, \"commit \", &p) &&\n-\t\t    !skip_prefix(line, \"From \", &p) &&\n-\t\t    starts_with(line, \"\\\\ \") && 12 < strlen(line)) {\n+\t\t/*\n+\t\t * If we see a line that begins with \"<object name>\",\n+\t\t * \"commit <object name>\" or \"From <object name>\", it is\n+\t\t * the beginning of a patch.  Return to the caller, as\n+\t\t * we are done with the one we have been processing.\n+\t\t */\n+\t\tif (skip_prefix(line, \"commit \", &p))\n+\t\t\t;\n+\t\telse if (skip_prefix(line, \"From \", &p))\n+\t\t\t;\n+\t\tif (!get_oid_hex(p, next_oid)) {\n \t\t\tif (verbatim)\n \t\t\t\tthe_hash_algo->update_fn(&ctx, line, strlen(line));\n-\t\t\tcontinue;\n-\t\t}\n-\n-\t\tif (!get_oid_hex(p, next_oid)) {\n \t\t\tfound_next = 1;\n \t\t\tbreak;\n \t\t}\n@@ -135,6 +138,16 @@ static int get_one_patchid(struct object_id *next_oid, struct object_id *result,\n \t\t\t\tbreak;\n \t\t}\n \n+\t\t/*\n+\t\t * A hunk about an incomplete line may have this\n+\t\t * marker at the end, which should just be ignored.\n+\t\t */\n+\t\tif (starts_with(line, \"\\\\ \") && 12 < strlen(line)) {\n+\t\t\tif (verbatim)\n+\t\t\t\tthe_hash_algo->update_fn(&ctx, line, strlen(line));\n+\t\t\tcontinue;\n+\t\t}\n+\n \t\tif (diff_is_binary) {\n \t\t\tif (starts_with(line, \"diff \")) {\n \t\t\t\tdiff_is_binary = 0;\n-- \n2.46.0-69-gd0749fd195\n\n"},{"id":"499548","messageId":"20240730011738.4032377-6-gitster@pobox.com","threadId":"61669","inReplyTo":"20240730011738.4032377-1-gitster@pobox.com","subject":"[PATCH v2 5/5] patch-id: tighten code to detect the patch header","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-07-30T01:17:38Z","receivedAt":"2024-07-30T01:17:56Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"The get_one_patchid() function unconditionally takes a line that\nmatches the patch header (namely, a line that begins with a full\nobject name, possibly prefixed by \"commit\" or \"From\" plus a space)\nas the beginning of a patch.  Even when it is *not* looking for one\n(namely, when the previous call found the patch header and returned,\nand then we are called again to skip the log message and process the\npatch whose header was found by the previous invocation).\n\nAs a consequence, a line in the commit log message that begins with\none of these patterns can be mistaken to start another patch, with\ncurrent message entirely skipped (because we haven't even reached\nthe patch at all).\n\nAllow the caller to tell us if it called us already and saw the\npatch header (in which case we shouldn't be looking for another one,\nuntil we see the \"diff\" part of the patch; instead we simply should\nbe skipping these lines as part of the commit log message), and skip\nthe header processing logic when that is the case.  In the helper\nfunction, it also needs to flip this \"are we looking for a header?\"\nbit, once it finished skipping the commit log message and started\nprocessing the patches, as the patch header of the _next_ message is\nthe only clue in the input that the current patch is done.\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n builtin/patch-id.c  | 49 +++++++++++++++++++++++++++++++++------------\n t/t4204-patch-id.sh | 17 ++++++++++++++++\n 2 files changed, 53 insertions(+), 13 deletions(-)\n\ndiff --git a/builtin/patch-id.c b/builtin/patch-id.c\nindex a2fdc0505d..f2e8feb6d3 100644\n--- a/builtin/patch-id.c\n+++ b/builtin/patch-id.c\n@@ -60,10 +60,17 @@ static int scan_hunk_header(const char *p, int *p_before, int *p_after)\n \n /*\n  * flag bits to control get_one_patchid()'s behaviour.\n+ *\n+ * STABLE/VERBATIM are given from the command line option as\n+ * --stable/--verbatim.  FIND_HEADER conveys the internal state\n+ * maintained by the caller to allow the function to avoid mistaking\n+ * lines of log message before seeing the \"diff\" part as the beginning\n+ * of the next patch.\n  */\n enum {\n \tGOPID_STABLE = (1<<0),\t\t/* --stable */\n \tGOPID_VERBATIM = (1<<1),\t/* --verbatim */\n+\tGOPID_FIND_HEADER = (1<<2),\t/* stop at the beginning of patch message */\n };\n \n static int get_one_patchid(struct object_id *next_oid, struct object_id *result,\n@@ -71,6 +78,7 @@ static int get_one_patchid(struct object_id *next_oid, struct object_id *result,\n {\n \tint stable = flags & GOPID_STABLE;\n \tint verbatim = flags & GOPID_VERBATIM;\n+\tint find_header = flags & GOPID_FIND_HEADER;\n \tint patchlen = 0, found_next = 0;\n \tint before = -1, after = -1;\n \tint diff_is_binary = 0;\n@@ -86,26 +94,39 @@ static int get_one_patchid(struct object_id *next_oid, struct object_id *result,\n \t\tint len;\n \n \t\t/*\n-\t\t * If we see a line that begins with \"<object name>\",\n-\t\t * \"commit <object name>\" or \"From <object name>\", it is\n-\t\t * the beginning of a patch.  Return to the caller, as\n-\t\t * we are done with the one we have been processing.\n+\t\t * The caller hasn't seen us find a patch header and\n+\t\t * return to it, or we have started processing patch\n+\t\t * and may encounter the beginning of the next patch.\n \t\t */\n-\t\tif (skip_prefix(line, \"commit \", &p))\n-\t\t\t;\n-\t\telse if (skip_prefix(line, \"From \", &p))\n-\t\t\t;\n-\t\tif (!get_oid_hex(p, next_oid)) {\n-\t\t\tif (verbatim)\n-\t\t\t\tthe_hash_algo->update_fn(&ctx, line, strlen(line));\n-\t\t\tfound_next = 1;\n-\t\t\tbreak;\n+\t\tif (find_header) {\n+\t\t\t/*\n+\t\t\t * If we see a line that begins with \"<object name>\",\n+\t\t\t * \"commit <object name>\" or \"From <object name>\", it is\n+\t\t\t * the beginning of a patch.  Return to the caller, as\n+\t\t\t * we are done with the one we have been processing.\n+\t\t\t */\n+\t\t\tif (skip_prefix(line, \"commit \", &p))\n+\t\t\t\t;\n+\t\t\telse if (skip_prefix(line, \"From \", &p))\n+\t\t\t\t;\n+\t\t\tif (!get_oid_hex(p, next_oid)) {\n+\t\t\t\tif (verbatim)\n+\t\t\t\t\tthe_hash_algo->update_fn(&ctx, line, strlen(line));\n+\t\t\t\tfound_next = 1;\n+\t\t\t\tbreak;\n+\t\t\t}\n \t\t}\n \n \t\t/* Ignore commit comments */\n \t\tif (!patchlen && !starts_with(line, \"diff \"))\n \t\t\tcontinue;\n \n+\t\t/*\n+\t\t * We are past the commit log message.  Prepare to\n+\t\t * stop at the beginning of the next patch header.\n+\t\t */\n+\t\tfind_header = 1;\n+\n \t\t/* Parsing diff header?  */\n \t\tif (before == -1) {\n \t\t\tif (starts_with(line, \"GIT binary patch\") ||\n@@ -201,11 +222,13 @@ static void generate_id_list(unsigned flags)\n \tstruct strbuf line_buf = STRBUF_INIT;\n \n \toidclr(&oid);\n+\tflags |= GOPID_FIND_HEADER;\n \twhile (!feof(stdin)) {\n \t\tpatchlen = get_one_patchid(&n, &result, &line_buf, flags);\n \t\tif (patchlen)\n \t\t\tflush_current_id(&oid, &result);\n \t\toidcpy(&oid, &n);\n+\t\tflags &= ~GOPID_FIND_HEADER;\n \t}\n \tstrbuf_release(&line_buf);\n }\ndiff --git a/t/t4204-patch-id.sh b/t/t4204-patch-id.sh\nindex 1627fdda1b..b1d98d4110 100755\n--- a/t/t4204-patch-id.sh\n+++ b/t/t4204-patch-id.sh\n@@ -137,6 +137,23 @@ test_expect_success 'patch-id computes the same for various formats' '\n \ttest_cmp actual expect\n '\n \n+hash=$(git rev-parse same:)\n+for cruft in \"$hash\" \"commit $hash is bad\" \"From $hash status\"\n+do\n+\ttest_expect_success \"patch-id with <$cruft> in log message\" '\n+\t\tgit format-patch -1 --stdout same >patch-0 &&\n+\t\tgit patch-id <patch-0 >expect &&\n+\n+\t\t{\n+\t\t\tsed -e \"/^$/q\" patch-0 &&\n+\t\t\tprintf \"random message\\n%s\\n\\n\" \"$cruft\" &&\n+\t\t\tsed -e \"1,/^$/d\" patch-0\n+\t\t} >patch-cruft &&\n+\t\tgit patch-id <patch-cruft >actual &&\n+\t\ttest_cmp actual expect\n+\t'\n+done\n+\n test_expect_success 'whitespace is irrelevant in footer' '\n \tget_patch_id main &&\n \tgit checkout same &&\n-- \n2.46.0-69-gd0749fd195\n\n"},{"id":"499552","messageId":"ZqhyI7Ta9g_4AwfI@tanuki","threadId":"61669","inReplyTo":"xmqqwml4gdol.fsf@gitster.g","subject":"Re: [PATCH 5/5] patch-id: tighten code to detect the patch header","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-07-30T04:55:12Z","receivedAt":"2024-07-30T04:55:17Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Mon, Jul 29, 2024 at 01:12:42PM -0700, Junio C Hamano wrote:\n> Patrick Steinhardt <ps@pks.im> writes:\n> \n> > On Fri, Jun 21, 2024 at 04:18:26PM -0700, Junio C Hamano wrote:\n> >> @@ -196,11 +211,13 @@ static void generate_id_list(unsigned flags)\n> >>  \tstruct strbuf line_buf = STRBUF_INIT;\n> >>  \n> >>  \toidclr(&oid);\n> >> +\tflags |= GOPID_FIND_HEADER;\n> >>  \twhile (!feof(stdin)) {\n> >>  \t\tpatchlen = get_one_patchid(&n, &result, &line_buf, flags);\n> >>  \t\tif (patchlen)\n> >>  \t\t\tflush_current_id(&oid, &result);\n> >>  \t\toidcpy(&oid, &n);\n> >> +\t\tflags &= ~GOPID_FIND_HEADER;\n> >>  \t}\n> >\n> > I think I'm missing the obvious. But why don't we have to set\n> > `GOPID_FIND_HEADER` when we have flushed the current patch ID? Is this\n> > because we know that `get_one_patchid()` stops once it finds the next\n> > line starting with a commit?\n> \n> Yup the original control flow is rather convoluted.  The first call\n> stops when it finds the header that begins the log message part and\n> returns, but the subsequent calls are to (1) skip the log message\n> and then (2) parse and hash the diff part, until it finds another\n> header that begins the log message part of the _next_ patch and\n> return.  GOPID_FIND_HEADER bit is used to tell the callee when we\n> haven't found the header (hence we can stop at a line whose\n> beginning looks like a hash) or the previous round already found the\n> header and we positively know we are now in the \"skip the log\n> message\" phase (hence a line whose beginning looks like a hash is\n> not a new header).\n\nOkay.\n\n> > Makes me wonder what happens when there is\n> > non-diff garbage between patches for which we are about to generate\n> > patch IDs.\n> \n> \"Skip non-diff garbage until we see a patch\" is the mechanism used\n> to skip the log message, so it would be a reasonable thing to skip\n> such no-diff garbage between patches, no?\n\nOh, yes, it is reasonable. I just didn't quite figure out the flow of\nthe above loop when reading through the code. As you say, it is somewhat\nconvoluted and not all that straight forward.\n\nIn any case though, your changes improve readability, so the fact that\nthings are not quite straight forward is not the fault of this patch\nseries here.\n\nPatrick\n"},{"id":"499553","messageId":"Zqh2Kije5omYzDPY@tanuki","threadId":"61669","inReplyTo":"ZqhyI7Ta9g_4AwfI@tanuki","subject":"Re: [PATCH 5/5] patch-id: tighten code to detect the patch header","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-07-30T05:12:10Z","receivedAt":"2024-07-30T05:12:16Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Tue, Jul 30, 2024 at 06:55:14AM +0200, Patrick Steinhardt wrote:\n> On Mon, Jul 29, 2024 at 01:12:42PM -0700, Junio C Hamano wrote:\n> > Patrick Steinhardt <ps@pks.im> writes:\n> > > On Fri, Jun 21, 2024 at 04:18:26PM -0700, Junio C Hamano wrote:\n> > \"Skip non-diff garbage until we see a patch\" is the mechanism used\n> > to skip the log message, so it would be a reasonable thing to skip\n> > such no-diff garbage between patches, no?\n> \n> Oh, yes, it is reasonable. I just didn't quite figure out the flow of\n> the above loop when reading through the code. As you say, it is somewhat\n> convoluted and not all that straight forward.\n\nAs far as I can see we didn't have a test for this yet, so I did have a\nquick go at it to reassure myself that things work as expected before\nand after your change. Feel free to pick it up if you feel like it, or\nto just ignore it :)\n\nPatrick\n\ntest_expect_success 'patch-id handles diffs with garbage in between' '\n\tcat >diff-with-garbage <<-\\EOF &&\n\t$(git rev-parse HEAD)\n\tdiff --git a/bar b/bar\n\tindex bdaf90f..31051f6 100644\n\t--- a/bar\n\t+++ b/bar\n\t@@ -2 +2,2 @@\n\t b\n\t+c\n\tsome\n\tgarbage\n\tlines\n\t$(git rev-parse HEAD)\n\tdiff --git a/car b/car\n\tindex 00750ed..2ae5e34 100644\n\t--- a/car\n\t+++ b/car\n\t@@ -1 +1,2 @@\n\t 3\n\t+d\n\tEOF\n\n\tcat >diff-without-garbage <<-\\EOF &&\n\t$(git rev-parse HEAD)\n\tdiff --git a/bar b/bar\n\tindex bdaf90f..31051f6 100644\n\t--- a/bar\n\t+++ b/bar\n\t@@ -2 +2,2 @@\n\t b\n\t+c\n\t$(git rev-parse HEAD)\n\tdiff --git a/car b/car\n\tindex 00750ed..2ae5e34 100644\n\t--- a/car\n\t+++ b/car\n\t@@ -1 +1,2 @@\n\t 3\n\t+d\n\tEOF\n\n\tfor stable in true false\n\tdo\n\t\ttest_config patchid.stable $stable &&\n\t\tgit patch-id <diff-with-garbage >id-with-garbage &&\n\t\tgit patch-id <diff-without-garbage >id-without-garbage &&\n\t\ttest_line_count -eq 2 id-with-garbage &&\n\t\ttest_cmp id-with-garbage id-without-garbage ||\n\t\treturn 1\n\tdone\n'\n"},{"id":"499554","messageId":"Zqh2T_2RLt0SeKF7@tanuki","threadId":"61669","inReplyTo":"20240730011738.4032377-1-gitster@pobox.com","subject":"Re: [PATCH v2 0/5] Tighten patch header parsing in patch-id","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-07-30T05:12:47Z","receivedAt":"2024-07-30T05:12:52Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Mon, Jul 29, 2024 at 06:17:33PM -0700, Junio C Hamano wrote:\n>     Updates from v1\n> \n>      - changed flag bits used internally from CPP macro to enum and\n>        added a bit of comments.\n\nThe changes look like what I expected, so this series looks good to me\noverall. Thanks!\n\nPatrick\n"}]}