Re: Issues in the Patch Tagging Guidelines

[Lucas Nussbaum <lucas@debian.org>]

Hi,

On 08/08/23 at 01:25 +0200, Guillem Jover wrote:
> Hi!
> 
> Lately I've been updating metadata in patches in packages I maintain and
> noticed several issues with the Patch Tagging Guidelines, and after Lucas
> created the new great patches UDD service [P] and we discussed some
> other issues there, it looked like the guidelines could do with some
> fixes and updates.
> 
>   [P] <https://udd.debian.org/patches.cgi> also part of DMD.
> 
> The following list are some of the issues or things that might deserve
> to be clarified, fixed or updated (for reference the current guidelines
> can be found at <https://dep.debian.net/deps/dep3>):

For context, there are several bugs about UDD's implementation of patch parsing:
#1028503 UDD: Unknown "yes" value for Forwarded field in patch metadata
#1031381 UDD/dmd: Incorrectly considers patches needing work
#1043043 UDD patches: marks Forwarded as invalid if not 'no', 'not-needed', 'yes' or URL

The UDD parser of DEP3 metadata is
https://salsa.debian.org/qa/udd/-/blob/master/rimporters/patches.rb#L163

The issues listed below fall in two categories:
1/ general issues about DEP3
2/ issues about computation of the "Forwarded" field value

1/ General issues about DEP3
============================

> * dpatch complicates parsing, it is deprecated, probably worth dropping
>   support for it.

I believe this is solved: dpatch is no longer available in
stable/testing/unstable, and we don't have any packages build-depending
on it. There are still some patches that are dpatch-formatted but do not
depend on dpatch-specific features, so this is not a big issue.

See:
select source, patch from sources_patches inner join patches using (hash, file)
where release='trixie' and headers ~* 'dpatch' order by 1,2;
=> 96 source packages, 295 patches

> * Although the guidelines seem to intend for git formatted patches to be
>   supported, the actual specification of the format is not very clear
>   on its usage and a strict reading does not really allow it.
> 
>   - There is a requirement for the first field to appear on the first
>     line, but git formatted patches start with «From ».

That's OK, since From is an alternative to Author, so it's a valid DEP3
field.

>   - You cannot easily add custom Patch Guideline patches into the first
>     git stanza, those need to go into the git trailers in the commit
>     message, separated by whatever text is found in the description,
>     which does not follow the deb822 format.

That's OK. You can have random text before the next DEP3 headers. See
the first example in the DEP3 specification.

>   - Having to store the patch guideline fields in git commit trailer
>     fields might mean these pollute the patch which might need to be
>     removed before submitting upstream.

In practice, supporting both git-formatted patches and custom-formatted
patches was a non-issue when writing a parser.

> * For Applied-Upstream it is not easy to predict what will be the
>   future upstream version that the patch will be included in. I've opted
>   for stuff like «3.2.1+, commit:<id>» when 3.2.1 is the last release,
>   but that does not seem optimal.
> 
> * The git Date field could somehow be used in place of Last-Update (even
>   though the Committer Date instead of the Author Date is what matches
>   here more closely, but which is not available from «git format-patch»).

Yes, the UDD implementation does that.

> * Add new metadata to track single-debian-patch autogenerated patches,
>   perhaps a new «Autogenerated: yes» or perhaps better something like
>   «Origin: autogenerated, by:dpkg-source» (or similar descriptive thing)?
>   These should in general not be warned as needing to be forwarded
>   upstream, at least not as-is (dpkg-source in 1.22.0 will add a
>   «Forwarded: not-needed» for these though).

A default of 'Forwarded: no' would be more appropriate, since some of
individual changes could be relevant for upstream...

I was surprised to see that there are only 144 packages in testing using
single-debian-patch, according to
 select source, patch, headers
 from sources_patches inner join patches using (hash, file)
 where release='trixie' and patch ='debian-changes' order by 1 ,2;

> * The language could use some clarification and standardize on the field
>   and stanza naming used in other parts of the deb822 ecosystem instead
>   of headers and sets of headers and similar.

> * It is not clear, but I think «Origin: vendor» should be clarified to
>   state that the actual vendor name should be included if there is no
>   other reference (such as an URL) as in say «Origin: vendor, Debian»?
>   Otherwise an undefined vendor is not really distinguishable from the
>   «other» value as it could be any vendor. Also because if a «vendor»
>   maintainer has created the patch then there might be no URL to point
>   to except for the VCS it is kept in (if any at all).

2/ issues about computation of the "Forwarded" field value
==========================================================

A clear status for each patch is useful to include that information in
various dashboards (tracker.d.o etc.)

Unfortunately, the fact that the value for Forwarded is not necessarily
explicit makes it hard to write a parser. It would be better if there
was a simple way to determine:
- if the patch was forwarded uptream (and how)
- if the patch does not need to be forwarded upstream
- if the patch still needs to be investigated

We could keep the current scheme of computing the real value of
Forwarded based on other fields, but then I think that DEP3 should
include pseudo-code to explain how to compute it.
I'm extremely biased of course, but I think that the UDD implementation could
serve as a basis. :-)
(And I would be happy to update the UDD implementation to match an updated DEP3).

> * Forwarded does not support recording it being sent to an email address.
>   Not all upstreams have a public mailing list or web site to file reports
>   or send comments to, and it might be worth allowing a mailto: reference,
>   or simply an email address.
> 
> * Forwarded lists "yes" as a valid implicit value, but does not make it
>   clear whether an explicit one should be supported. If a mailto: or
>   email is accepted then this is probably less of an issue. I've also
>   used this value when I have sent the patch upstream and it has been
>   applied before I have gotten around to updating the patch metadata,
>   but I guess at that point not providing the field would also be
>   fine.
> 
> * Forwarded fuzzy specification means parsing its values is rather hard,
>   see UDD <https://lists.debian.org/debian-qa/2023/01/msg00022.html>.
>   It would be better to be strict here. In general choosing to be
>   fuzzy about the whole specification with the intent to help humans,
>   I think means that programs have a hard time (see UDD above, and
>   lintian, where both disagree on semantics) and even humans can get
>   more confused when crafting or parsing them.

So I think that the next step would be for someone to start a draft to
update DEP3. I might do it myself at some point, but would be very happy
if someone else did it.

Lucas