Re: [Outreachy] [PATCH v2] diff: do not show submodule with untracked files as "-dirty"

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Oct 21, 2020, 23:04 UTC
Message-ID: <CAPig+cRLGsSzHZ-280WJD1vMLvK-kL2EEg3mc8gFiEEa8Tj6Aw@mail.gmail.com>
In-Reply-To: <CAHjREB6Agm9Nia2+e4=1zAJ6Y3NXgPQLmN_he1353ScaznK5hQ@mail.gmail.com>

On Wed, Oct 21, 2020 at 3:40 PM Sangeeta NB <sangunb09@gmail.com> wrote:

Show 12 quoted lines

> > Another option is to have a preparatory patch which first modernizes
> > the script, and then your new tests would follow modern style. But,
> > that may be outside of scope of your submission.
> >
> > To summarize: The only really actionable review comments are the minor
> > style nits in the C code. The nits about style issues in the tests are
> > judgement calls, and could be handled (by someone) at a later date.
>
> For the style issues in the tests maybe what I can do is I can submit
> another patch with this patch to modernize the test scripts of these
> files? I am expecting some feedback from the community on how to
> handle this.

If this is a microproject, then it probably makes sense just to fix the remaining style nits in the C code, and leave it at that.

Even if this is not a microproject, the answer really depends upon how much time and effort you want to devote to this. As a reviewer, I don't demand or expect you to fix all the problems in the old test scripts just to get the change accepted which prompted your submission in the first place. It's important to focus on that first rather than getting sidetracked working on stuff which may not be important to your primary goal. Also, keep in mind that fixing all the style issues in those old test scripts may require multiple preparatory patches, rather than just one, and multiple re-rolls, which could end up eating up a lot of extra time to get it all polished properly.

For these reasons, I'm inclined to suggest polishing the current submission to get it accepted. Then decide afterward if you want to devote time to working on these ancillary tasks.