Re: [LTP] [PATCH] Add goals of patch review and tips

[Petr Vorel <pvorel@suse.cz>]

> Hi!
> > >> +1. Prevent false positive test results
> > >> +2. Prevent false negative test results
> > >> +3. Make future changes as easy as possible

> > > I would say that number 3 maybe be a bit controversial, I've seen cases
> > > where attempts to futureproof the code caused steep increase in the
> > > test
> > > complexity. So maybe:

> > > 3. Keep the code as simple as possible as well as futureproof

> > Perhaps just

> > 3. Keep the code as simple as possibe, but no simpler

> > This is possibly paraphrasing Einstein:
> > https://quoteinvestigator.com/2011/05/13/einstein-simple/


> > NOTE: I think future proofing is actually very dangerous. What I
> >       probably meant was

> >       3. Keep the code as simple as possible, while maintaining optionality,
> >          but if there appears to be a disproportionate increase in complexity
> >          for an increase in optionality then simplicity takes priority because
> >          identifying relevant optionality is hard.

> >       but "optionality" does not have a nice dictionary definition. I guess
> >       you could substitute it with "freedom". In any case it's not something I
> >       would want to write in documentation. There is no easy way to
> >       express it.

> That sounds way to complicated, unfortunately reality is often
> complicated and cannot be overly simplified.

> So I would go with the simple paraphrase to Einstein, that is short and
> to the point.

+1

> > >> +## How to get patches merged
> > >> +
> > >> +Once you think a patch is good enough you should add your Reviewed-by
> > >> +tags. This means you will get some credit for getting the patch
> > >> +merged. Also some blame if there are problems.

> > > Maybe we should mention the Tested-by: tag explicitly here as well.

> > I'm not sure how we interpret Tested-by when deciding to merge; does it
> > mean someone is happy for the test to be merged or not?

> > Should someone add both tags if they have reviewed and tested it?

> Tested-by: means that someone actually tried the test and that it did
> what it was supposed to do. This has obvious meaning for reproducers,
> and yes for a reproducer you can add both tags and both are meaningful.

> For regular tests Tested-by does not have that much value I guess.

I rarely add Tested-by, usually for non-intel architecture or something
which was non-trivial for me to test.

Kind regards,
Petr

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp