Re: [GSoC][PATCH] lib-read-tree-m-3way: modernize a test script (style)

[Eric Sunshine <sunshine@sunshineco.com>]

On Fri, Jan 28, 2022 at 4:51 AM Shaoxuan Yuan <shaoxuan.yuan02@gmail.com> wrote:
> On Fri, Jan 28, 2022 at 4:34 PM Eric Sunshine <sunshine@sunshineco.com> wrote:
> > In this case, the indentation of the entire body of the for-loop needs
> > to be fixed to use tabs rather than spaces, however, fixing all the
> > indentation problems together with the other problems can make for a
> > too-noisy patch for reviewers to really see what is going on. As such,
> > it may be better to make this a multi-patch series in which one patch
> > fixes indentation problems only.
>
> > As mentioned above, changing the body of the test from double- to
> > single-quoted string should (presumably) be okay since the body gets
> > eval'd and `$p` will be visible at the time of `eval`, however, mixing
> > this sort of change in with all the other changes being made makes it
> > hard for reviewers to spot such little details, let alone reason about
> > correctness. As such, switching of quote types is also probably the
> > sort of change which should be split out into its own patch. The same
> > comment applies to other changes following this one.
>
> I think so. I was thinking fixing all the general styling issues in one
> patch (since they are all style related), but now I realize that the general
> style patch can be divided into sub-patches for clearer review experience.
>
> And my question is, should I do this "multi-patch series" thing in the form of
> "-v<n>" (all under this thread), e.g. "v2" or "v3"? Or I just submit
> multiple patches separately (a new thread for each one)?

A multi-patch series as v2, v3, etc. would indeed be appropriate, as
you already figured out[1] before I got around to answering belatedly.

> > Overall, with the exception of the test title which needs to
> > interpolate `$p`, the rest of the changes are probably reasonable and
> > benign. It's important to understand that lengthy patches like this
> > full of mechanical changes tend to be quite taxing on reviewers, so
> > it's a good idea to help in any way you can to ease the review burden.
> > This can be done, for instance, by making only a single type of change
> > per patch (i.e. indentation fixes), or by limiting the scope or
> > breadth of the changes, which is especially important for this sort of
>
> I'm not quite sure what this means, and I quote, "limiting the scope or
> breadth of the changes". Are you suggesting, for example,
> fixing fewer occurrences of tab indentation issue in a patch; or, for
> example, limiting the
> fix to a specific "test_expect_success" block, and do multiple patches
> sequentially?

Sorry for being unclear. I just meant that as a microproject, it would
have worked equally well to pick a much smaller test script with style
problems (if you could find one) rather than a long script. After all,
the purpose of a microproject is to give you experience with the
submission-review process and to give reviewers and mentors an idea of
how you interact. It's the process which is important, in this case,
not the size of the submission.

Anyhow, it looks like Junio is happy with your v3 and will be merging
it down to "next", so it all worked out.

[1]: https://lore.kernel.org/git/20220202064300.3601-1-shaoxuan.yuan02@gmail.com/
[2]: https://lore.kernel.org/git/xmqqr18jnr2t.fsf@gitster.g/