Re: [PATCH 3/6] NetworkPkg/IScsiDxe: distinguish "maximum" and "selected" CHAP digest sizes
On 06/09/21 12:43, Philippe Mathieu-Daudé wrote:
Hi Laszlo,Interesting; I thought that showing all uses of ISCSI_CHAP_RSP_LEN in
one patch, and classifying each use one way or another at once, was the
best for reviewer understanding. Basically it's a single "mental loop"
over all uses, and in the "loop body", we have an "if" (classification)
-- allocation vs. processing.
What you propose is basically "two loops". In that approach, in the
first patch (= the first "mental loop"), only "processing" uses would be
updated; the "allocation sites" wouldn't be shown at all. I feel that
this approach is counter-intuitive:
From the body of the first patch,
- the reviewer can check the *correctness* of the patch (that is,
whether everything that I converted is indeed "processing"),
- but they can't check the *completeness* of the patch (that is, whether
there is a "processing" site that I should have converted, but missed).
For the reviewer to verify the completeness of the first patch, they
have to apply it (or check out the branch at that stage), and go over
all the *remaining* ISCSI_CHAP_RSP_LEN instances, to see if I missed
something. And, if the reviewer has to check every single instance of
ISCSI_CHAP_RSP_LEN right after the first patch, they end up doing the
same work as if they had just reviewed this particular patch.
I think your approach boils down to the following idea:
The completeness of the first patch would be proved by the correctness
of the second patch.
That is, *after* you reviewed the second patch (and see that every site
converted is indeed an allocation site, and that the ISCSI_CHAP_RSP_LEN
macro definition is being removed, so no other ISCSI_CHAP_RSP_LEN
instance remains), you could be sure that no processing site was missed
in the first patch.
Technically / mathematically, this is entirely true; I just prefer
avoiding situations where you have to review patch (N+X) to see the
validity (completeness) of patch (N). I quite dislike jumping between
patches during review.
Does my explanation make sense?
If you still prefer the split, I'm OK to do it.
Distinguishing these purposes is justified because purpose (b) --