On Tue, Jan 21, 2025 at 12:11 AM Eric Sunshine [off-list ref] wrote:

On Mon, Jan 20, 2025 at 1:17 PM Usman Akinyemi
[off-list ref] wrote:

quoted

On Sat, Jan 18, 2025 at 3:14 AM Eric Sunshine [off-list ref] wrote:

quoted

On Fri, Jan 17, 2025 at 5:47 AM Usman Akinyemi
[off-list ref] wrote:

quoted

+       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.