Hi Junio
On 12/01/2022 18:51, Junio C Hamano wrote:
Phillip Wood <phillip.wood123@xxxxxxxxx> writes:
I'm not sure I want to go with your extra changes. I've left some
comments on them below
@@ -488,12 +499,12 @@ static int parse_diff(struct add_p_state *s, const struct pathspec *ps)
else if (starts_with(p, "@@ ") ||
(hunk == &file_diff->head &&
(skip_prefix(p, "deleted file", &deleted)))) {
- if (marker == '-' || marker == '+')
- /*
- * Should not happen; previous hunk did not end
- * in a context line? Handle it anyway.
- */
+ hunk->splittable_into++;
+ /*
+ * Should not increment "splittable_into";
+ * previous hunk did not end in a context
+ * line? Handle it anyway.
+ */
+ complete_file(marker, &hunk->splittable_into);
ALLOC_GROW_BY(file_diff->hunk, file_diff->hunk_nr, 1,
file_diff->hunk_alloc);
I deliberately left this alone as I think we should probably make this
BUG() out instead of silently accepting an invalid diff.
As we are reading our own output, I agree that such a data error is
a BUG().
In any case, a helper to see if the file ended without post-context
is one thing, and a helper that specify what happens after we are
done with a single file, before we move on top the next file or
after processing the last file, is another thing. The latter may be
able to make use of the former, but the latter may want to do more
than that in the future.
As complete_file() is about finalizing the processing we have done
to the current file, it should be used for that purpose, and nothing
else, I think the hunk I see at
https://github.com/git/git/commit/c082176f8c5a1fc1c8b2a93991ca28fd63aae73a
(reproduced below) is simply a nonsense.
Stepping back a bit, though, is this helper really finalizing the
current file, or is it finalizing the current hunk? If it were the
latter, then its use in the hunk I called "nonsense" above actually
makes perfect sense.
Even if the helper is finalizing the current hunk then I think that
"nonsense" hunk would still wrong as it would be calling finalize_hunk()
on _every_ context line in the hunk rather than just being called once
to finalize the hunk. We could call the function something like
update_splittable() but then we'd need to explain why we were calling
that function at the start of a diff and at the end of the loop.
There may not be anything other than finalizing
the last hunk when we see the end of a file right now, so we may not
need to add a finalize_file() helper right now, and when we need to
do something more than finalizing the last hunk, we may need to capture
the distinction by adding one.
Yes, if you're happy lets leave this series as it is
Best Wishes
Phillip
diff --git i/add-patch.c w/add-patch.c
index 89ffda32b2..6094290c86 100644
--- i/add-patch.c
+++ w/add-patch.c
@@ -578,8 +578,8 @@ static int parse_diff(struct add_p_state *s, const struct pathspec *ps)
(int)(eol - (plain->buf + file_diff->head.start)),
plain->buf + file_diff->head.start);
- if ((marker == '-' || marker == '+') && *p == ' ')
- hunk->splittable_into++;
+ if (*p == ' ')
+ complete_file(marker, &hunk->splittable_into);
if (marker && *p != '\\')
marker = *p;
