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.
