[bug#57598] [PATCH] doc: Update contribution guidelines on patches, etc.

["Ludovic Courtès" <ludo@gnu.org>]

Hi!

Thanks for this welcome addition!  Modulo the cosmetic suggestions
below, I think it’s fine.

Maintainers, if you have something to say on the guidelines, now’s the
time!

  https://issues.guix.gnu.org/57598

Maxime Devos <maximedevos@telenet.be> skribis:

> * doc/contributing.texi (Modifying Sources): Replaced with ...
> ("Modifying Sources"): ... this.  List more use cases and some principles.
>
> This patch incorporates some text written by Liliana Marie Prikler.  The
> structure is based on a proposal by Julien Lepiller.  The text has been
> revised based on reviews by David Larsson and blake.
>
> (! remove following text before committing !)

You can write comments below the --- and diffstats.  That way, ‘git am’
will ignore them when applying the patch.

> +Guix has tree main ways of modifying the source code of a package, that
> +you as a packager may use.  These are patches, snippets and phases.
                            ^
“may use: patches, snippets, and build phases.”

> +Each one has its strengths and drawbacks.  To decide between the three,

s/decide between the three/choose among these/

> +there are a few guiding principles to satisfy simultanuously where
> +possible:
> +
> +@itemize
> +@item
> +In principle, Guix only has free software; when the upstream source

s/In principle, Guix only has free software/Every package in Guix is
free software/g

> +community (i.e., patterns that appear throughout @code{gnu/packages/...})

Normally such parenthetical expressions go between em dashes:

  community---i.e., patterns that appear throughout
  @file{gnu/packages}---and …

> +To make things more concrete and to resolve conflicts between the
> +principles, a few cases have been worked out:
> +
> +@subsubsection Removing non-free software
> +Non-free software has to be removed in snippets; the reason is that
> +patches or phases will not work.

You can’t have a colon between the section heading.  Instead, you can
write “a few cases have been worked out and will be illustrated in the
following sections.”

Section titles should be capitalized.

Leave a blank line after the section title.

(Same comment for the sections that come after.)

Instead of “will not work”, which is vague, I’d suggest more explicit
wording: “would not be appropriate because they would expose the
offending code.”

> +For patches, the problem is that a patch removing a non-free file
> +automatically contains the non-free file@footnote{It has been noted that
> +git patches support removing files without including the file in the
> +patch in
> +@url{https://yhetil.org/guix/8b13e899-eb60-490b-a268-639249698c81@@www.fastmail.com/}. If
> +it is verified that the @command{patch} utility supports such patches,
> +this method can be used and this policy adjusted appropriately.}, and we
> +do not want anything non-free in Guix even if only in its patches.

I’d drop the footnote, it’s already dense enough.

> +@code{delete-file-recursively}. There are a few benefits for snippets here:
> +
> +When using snippets, the bundled library does not occur in the source

s/snippets here:/snippets here./
s/When using snippets/First, when using snippets/

> +As such, snippets are recommended here.

s/are recommended here/are the recommended way to delete non-free material/

> +@subsubsection Fixing technical issues (compilation errors, test failures, other bugs ...)

In addition to capitalizing, please remove the parenthetical bit.

> +@subsubsection Adding new functionality
> +To add new functionality, a patch is almost always the most convenient
> +choice of the three -- patches are usually multi-line changes, which are

s/three -- patches/three---patches/

That’s all I have to say!

I think what the text says is fine.  It’s dense and rather long though,
which means that people may overlook things.  OTOH it’s structured so
it’s easier to skim through it, and I wouldn’t know what to remove.

Could you send an updated version?

Thanks,
Ludo’.