On Tue, Jan 21, 2025 at 12:11 AM Eric Sunshine <sunshine@xxxxxxxxxxxxxx> wrote: > > On Mon, Jan 20, 2025 at 1:17 PM Usman Akinyemi > <usmanakinyemi202@xxxxxxxxx> wrote: > > On Sat, Jan 18, 2025 at 3:14 AM Eric Sunshine <sunshine@xxxxxxxxxxxxxx> wrote: > > > On Fri, Jan 17, 2025 at 5:47 AM Usman Akinyemi > > > <usmanakinyemi202@xxxxxxxxx> 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.
