Message-ID: <37d426d6246dbf7702e86eac43fd393407ec6e28.camel@cogitri.dev>
Subject: Re: Enforcing patch headers for patches
From: Rasmus Thomsen <oss@cogitri.dev>
To: Natanael Copa <ncopa@alpinelinux.org>
Cc: ~alpine/devel@lists.alpinelinux.org
Date: Thu, 30 Apr 2020 12:45:40 +0200
In-Reply-To: <20200430102823.11d1ed66@ncopa-desktop.copa.dup.pw>
References: <4ac4d3aad3bf0f050070166b27a2344efe32e9b7.camel@cogitri.dev>
	 <20200430102823.11d1ed66@ncopa-desktop.copa.dup.pw>
Content-Type: text/plain; charset="UTF-8"
Mime-Version: 1.0
Content-Transfer-Encoding: 7bit

On Thu, 2020-04-30 at 10:28 +0200, Natanael Copa wrote:
> What should we set it to if the patch is suitable for upstreaming but
> not yet submitted/reported upstream?

Then you should upstream it and add the Url to the patch. The "I'll
upstream it sometime later" thing often times doesn't work out and we
end up having yet another downstream patch.

> What if we are unsure if its suitable for upstreaming?

Then you should ask upstream (or someone else who knows it, e.g. the
reviewers of your MR) if it's suitable for upstreaming.

> What happens if it was proposed upstream but later rejected or
> reverted (eg. upstream finds ou they don't want support musl at all)

Then we'll have to keep it downstream, but because we have a "Url"
field we can track what happened to the thing, e.g. because upstream
commented on the issue we linked that they don't want to support musl.
The patch doesn't have to be updated then, that'd be a lot of effort,
but I don't think that this will happen too often.

> What if we lift patches from debian or fedora and they have different
> syntax for similar do we need to convert it to our format?

Hm, although I think it'd be best if everything followed that format,
so you don't have to spend too much time figuring out where to find
information about a patch as reviewer/future maintainer/etc. I
understand that that might be too much effort. I think if the
Debian/Fedora patches are similiar enough (as in they provide the
information we need, namely upstream status, an URL to track progress
and an explanation) we can use them too.

> I am afraid of needing to change the upstreaming state of the patch
> when it changes or that the specified upstreaming state information
> getting outdated. This info becomes useless if it cannot be trusted.

That's what the URL is for - if you want more info you can easily look
it up by the URL value. Here is an example:

Upstream: Yes, pending
Url: https://github.com/ximion/appstream-generator/pull/75
Reason: Add support for scraping .apk packages for .desktop and
.appdata.xml files

Once this PR gets merged we don't have to touch the patch - interested
parties can just check the Url. After thinking about it for a bit,
maybe we could live with just an Upstream and a Reason field, like so:

Upstream: Yes, pending: 
https://github.com/ximion/appstream-generator/pull/75
Reason: Add support for scraping .apk packages for .desktop and
.appdata.xml files

The "Upstream" field isn't necessarily meant to indicate that a patch
has been accepted upstream, it's just an indicator that upstream knows
about the patch. This is the case when we opened an MR about it or we
cherry picked a commit from upstream etc.

> May be good to have some indication for patches that are not
> upstreamable and alpine specific. Maybe this is what you mean with
> "Upstream: no"

Yes, although it should be "Upstream: No, because $reason" :)


> There may be various different URLs. Where the patch comes from, URL
> to
> bug tracker, URL to other distros bugtracker or mailing list
> discussions.

I think the URL most relevant to the patch should be used. E.g. if the
patch has been submitted upstream but not yet merged, link to the MR.
If the patch is cherry picked from upstream you can link to the commit.
If it's from a mailing list, link to that, etc.
The Url: field isn't meant to inform the reader about why a patch is
applied (that's what Reason: is for), it's meant so people looking at
the patch at a later point can check what the progress of getting the
patch upstreamed is or if there are new versions fo the patch.

> I agree that having some reference where the patch comes from, and
> where to find upstream status is useful. I try (tried?) add that to
> patches whenever I can.

Yup, I've seen a few patches which already use a similiar scheme, but
since we don't enforce it currently we still have loads of patches
which don't have this and it's a lot of work to search for the
information that is easily accessible from the patch header later on.