patch-id: tighten code to detect the patch header

The get_one_patchid() function unconditionally takes a line that matches the patch header (namely, a line that begins with a full object name, possibly prefixed by "commit" or "From" plus a space) as the beginning of a patch. Even when it is *not* looking for one (namely, when the previous call found the patch header and returned, and then we are called again to skip the log message and process the patch whose header was found by the previous invocation). As a consequence, a line in the commit log message that begins with one of these patterns can be mistaken to start another patch, with current message entirely skipped (because we haven't even reached the patch at all). Allow the caller to tell us if it called us already and saw the patch header (in which case we shouldn't be looking for another one, until we see the "diff" part of the patch; instead we simply should be skipping these lines as part of the commit log message), and skip the header processing logic when that is the case. In the helper function, it also needs to flip this "are we looking for a header?" bit, once it finished skipping the commit log message and started processing the patches, as the patch header of the _next_ message is the only clue in the input that the current patch is done. Signed-off-by: Junio C Hamano <gitster@pobox.com>

Junio C Hamano committed Jul 29, 2024 at 18:17 UTC a6e9429f728d088999b815c25adbd2f2c115e051
2 files changed +53 -13
builtin/patch-id.c
+36 -13
@@ -60,10 +60,17 @@ static int scan_hunk_header(const char *p, int *p_before, int *p_after)
60
61 /*
62 * flag bits to control get_one_patchid()'s behaviour.
63 + *
64 + * STABLE/VERBATIM are given from the command line option as
65 + * --stable/--verbatim. FIND_HEADER conveys the internal state
66 + * maintained by the caller to allow the function to avoid mistaking
67 + * lines of log message before seeing the "diff" part as the beginning
68 + * of the next patch.
69 */
70 enum {
71 GOPID_STABLE = (1<<0), /* --stable */
72 GOPID_VERBATIM = (1<<1), /* --verbatim */
73 + GOPID_FIND_HEADER = (1<<2), /* stop at the beginning of patch message */
74 };
75
76 static int get_one_patchid(struct object_id *next_oid, struct object_id *result,
@@ -71,6 +78,7 @@ static int get_one_patchid(struct object_id *next_oid, struct object_id *result,
78 {
79 int stable = flags & GOPID_STABLE;
80 int verbatim = flags & GOPID_VERBATIM;
81 + int find_header = flags & GOPID_FIND_HEADER;
82 int patchlen = 0, found_next = 0;
83 int before = -1, after = -1;
84 int diff_is_binary = 0;
@@ -86,26 +94,39 @@ static int get_one_patchid(struct object_id *next_oid, struct object_id *result,
94 int len;
95
96 /*
89 - * If we see a line that begins with "<object name>",
90 - * "commit <object name>" or "From <object name>", it is
91 - * the beginning of a patch. Return to the caller, as
92 - * we are done with the one we have been processing.
97 + * The caller hasn't seen us find a patch header and
98 + * return to it, or we have started processing patch
99 + * and may encounter the beginning of the next patch.
100 */
94 - if (skip_prefix(line, "commit ", &p))
95 - ;
96 - else if (skip_prefix(line, "From ", &p))
97 - ;
98 - if (!get_oid_hex(p, next_oid)) {
99 - if (verbatim)
100 - the_hash_algo->update_fn(&ctx, line, strlen(line));
101 - found_next = 1;
102 - break;
101 + if (find_header) {
102 + /*
103 + * If we see a line that begins with "<object name>",
104 + * "commit <object name>" or "From <object name>", it is
105 + * the beginning of a patch. Return to the caller, as
106 + * we are done with the one we have been processing.
107 + */
108 + if (skip_prefix(line, "commit ", &p))
109 + ;
110 + else if (skip_prefix(line, "From ", &p))
111 + ;
112 + if (!get_oid_hex(p, next_oid)) {
113 + if (verbatim)
114 + the_hash_algo->update_fn(&ctx, line, strlen(line));
115 + found_next = 1;
116 + break;
117 + }
118 }
119
120 /* Ignore commit comments */
121 if (!patchlen && !starts_with(line, "diff "))
122 continue;
123
124 + /*
125 + * We are past the commit log message. Prepare to
126 + * stop at the beginning of the next patch header.
127 + */
128 + find_header = 1;
129 +
130 /* Parsing diff header? */
131 if (before == -1) {
132 if (starts_with(line, "GIT binary patch") ||
@@ -201,11 +222,13 @@ static void generate_id_list(unsigned flags)
222 struct strbuf line_buf = STRBUF_INIT;
223
224 oidclr(&oid);
225 + flags |= GOPID_FIND_HEADER;
226 while (!feof(stdin)) {
227 patchlen = get_one_patchid(&n, &result, &line_buf, flags);
228 if (patchlen)
229 flush_current_id(&oid, &result);
230 oidcpy(&oid, &n);
231 + flags &= ~GOPID_FIND_HEADER;
232 }
233 strbuf_release(&line_buf);
234 }
t/t4204-patch-id.sh
+17
@@ -137,6 +137,23 @@ test_expect_success 'patch-id computes the same for various formats' '
137 test_cmp actual expect
138 '
139
140 +hash=$(git rev-parse same:)
141 +for cruft in "$hash" "commit $hash is bad" "From $hash status"
142 +do
143 + test_expect_success "patch-id with <$cruft> in log message" '
144 + git format-patch -1 --stdout same >patch-0 &&
145 + git patch-id <patch-0 >expect &&
146 +
147 + {
148 + sed -e "/^$/q" patch-0 &&
149 + printf "random message\n%s\n\n" "$cruft" &&
150 + sed -e "1,/^$/d" patch-0
151 + } >patch-cruft &&
152 + git patch-id <patch-cruft >actual &&
153 + test_cmp actual expect
154 + '
155 +done
156 +
157 test_expect_success 'whitespace is irrelevant in footer' '
158 get_patch_id main &&
159 git checkout same &&