Best practices for quilt patches

[Frank Heimes]

Hi Robie, I think it's great to have best practices for quilt patches.

Since you also mentioned renaming of patches, this raises the question if
there is also a best practice for a quilt patch naming scheme? (I'm at
least not aware of one.)
I noticed several different ones, just to name a few:
<full-hash>.patch
<name of the upstream patch, spaces replaced by dashes, shortened>.patch
<arbitrary name, different from the upstream patch description
description>.patch
lp-<bug number>-<name of the upstream patch, spaces replaced by dashes,
shortened>.patch
etc. (I think I also saw '.diff' as suffix instead of '.patch')

What I personally liked most so far, for upstream patches (and adopted
since then) is:
<short-hash>-<name of the upstream patch, spaces replaced by dashes,
shortened>.patch

But maybe different teams (or individuals) just have different preferences.

And btw. I '+1' with Marc - also ran once into issues in the past, where
offsets piled up too much.
So it might be needed - in rare cases. (But understood - only in such rare
cases...)

Bye, Frank


On Wed, May 25, 2022 at 5:31 PM Marc Deslauriers <
marc.deslauriers at canonical.com> wrote:

>
> On 2022-05-25 10:50, Robie Basak wrote:
> > Today on my SRU shift I spent some effort trying to see past quilt diff
> > noise. Most of it seemed unnecessary and was only present because of
> > gratuitous refreshes.
> >
> > I'm not sure we have best practices are documented anywhere. I'd like to
> > define some guidelines that make reviews easier. I think many people
> > stick to these already. Can we agree that they're a good thing and try
> > to get more people to do them?
> >
> > Summary
> >
> > 1. Use dep3 headers (https://dep-team.pages.debian.net/deps/dep3/)
> >
> > 2. Use "-p ab --no-timestamps --no-index" or equivalent (as documented at
> > https://wiki.debian.org/UsingQuilt#Environment_variables).
> >
> > 3. Try not to update or refresh any quilt patch unless specifically
> > required (ie. a patch doesn't apply, or applies with fuzz). Exception:
> > do update dep3 headers.
> >
> > Details and rationale:
> >
> > 1. Use dep3 headers (https://dep-team.pages.debian.net/deps/dep3/)
> >
> > This makes it easy for reviewers to correlate the patch against
> > upstream, find any related upstream commentary or subsequent related
> > commits, etc.
> >
> > 2. Use "-p ab --no-timestamps --no-index" or equivalent (as documented at
> > https://wiki.debian.org/UsingQuilt#Environment_variables).
> >
> > This reduces diff noise in the future if an update becomes required.
> >
> > Exception: if taking the patch from upstream and it applies as-is, then
> > using the upstream form of the patch reduces the initial diff further,
> > so that's preferable. So this combines with 3: use these settings when a
> > refresh is required or generating the patch from scratch, but don't
> > refresh to apply the settings to reduce noise unless actually required.
> >
> > 3. Try not to update or refresh any quilt patch unless specifically
> > required (ie. a patch doesn't apply, or applies with fuzz). Exception:
> > do update dep3 headers.
> >
> > This reduces the size of any diff taken against any other version of the
> > quilt patch. Hopefully to zero.
> >
> > If one patch needs refreshing, refresh just that one rather than all of
> > them.
> >
> > If a patch applies with offset, that's not a reason to refresh it.
>
> I kind of disagree with this, I've hit issues before when the offsets were
> big
> enough that adding another patch before one with offsets would result in
> the
> patch being applied to the wrong function.
>
> While nobody should be refreshing patches for no reason to keep the debdiff
> changes to a minimum, I think it's reasonable to refresh newly added
> patches,
> and I would disagree with any best practice that states the opposite.
>
> >
> > Example 1: you can append ".patch" to an upstream Github URL, download
> > that to debian/patches/, rename and add dep3 headers but without
> > modifying the patch itself. Then a reviewer can  look at the dep3
> > header, identify that the origin was GitHub, diff against that same URL,
> > and easily confirm that the patch hasn't been modified.
> >
> > Example 2: if I'm reviewing an SRU to multiple releases and the quilt
> > patches are exactly identical, then I only have to review the patch
> > itself once[1]. But if you've unnecessarily refreshed the patch on each
> > upload, now I have a bunch of noise I have to review to verify that there
> > is no functional change introduced.
> >
> > Does this sound reasonable to everyone to follow in general, such that
> > we can document these as guidelines somewhere? I wouldn't expect them to
> > be enforced as a hard rule, but once documented I also think it'd be
> > reasonable for any reviewer to choose to push back where non-adherence
> > is causing an actual problem.
> >
> > Anything to change, or anything to add?
> >
> > Thanks,
> >
> > Robie
> >
> >
> > [1] The context outside the patch itself might be different and I do
> > have to consider that too, but if I know the patches are identical, that
> > is also made easier.
> >
> >
>
> Marc.
>
> --
> ubuntu-devel mailing list
> ubuntu-devel at lists.ubuntu.com
> Modify settings or unsubscribe at:
> https://lists.ubuntu.com/mailman/listinfo/ubuntu-devel
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.ubuntu.com/archives/ubuntu-devel/attachments/20220525/e5e2a6f4/attachment-0001.html>