Re: [PATCH v2 6/6] version: introduce osversion.command config for os-version output

[Usman Akinyemi <usmanakinyemi202@gmail.com>]

On Tue, Jan 21, 2025 at 12:11 AM Eric Sunshine <sunshine@sunshineco.com> wrote:
>
> On Mon, Jan 20, 2025 at 1:17 PM Usman Akinyemi
> <usmanakinyemi202@gmail.com> wrote:
> > On Sat, Jan 18, 2025 at 3:14 AM Eric Sunshine <sunshine@sunshineco.com> wrote:
> > > On Fri, Jan 17, 2025 at 5:47 AM Usman Akinyemi
> > > <usmanakinyemi202@gmail.com> wrote:
> > > > +       if test_have_prereq !WINDOWS
> > > > +       then
> > > > +               printf "\nos-version=%s\n" $(uname -srvm | test_redact_non_printables) >>agent_and_long_osversion
> > > > +       fi &&
> > >
> > > As an aid to future readers, please add an explanation either in the
> > > commit message or as a comment here in the code explaining why Windows
> > > is being singled out as special.
> >
> > The previous commit which introduced this has this information,
> > can we do some form of referencing ?
>
> My main concern is that someone looking at this change in the future
> -- who did not have the benefit of reading the cover letter or the
> review discussion -- may have a hard time understanding why Windows is
> singled out by this patch. As long as you give some sort of
> explanation, whether in the code or in the commit message, then you
> save that future user from having to figure it out on his or her own.
>
> So, your suggestion of referencing some other commit may work.
> Augmenting the commit message of this patch with something along the
> lines of:
>
>    As with the previous commit, we skip the tests on Windows.
>
> may be enough to tell the reader where to look for the explanation.
Yeah, thanks, this looks better and clearer. I will add this in the
next iteration
if we agree to include the osversion.command config.

Thank you.
Usman Akinyemi.