reject non-code patch attachments

It may be difficult to define a "code patch". There's lots of text/plain that's not patches, and it may be that fixing an issue requires an update to a graphic, sound file, or similar supplementary file. Further some patches are compressed to save space With a sufficiently selectable mechanism, and sufficiently intelligent processing, this would be quite interesting.

From an email thread with mdz and dholbach:

> > It should be possible to identify attachments that are patches
> > programmatically, at least to some degree of reliability. Maybe that
> > would be more useful than asking the user to indicate whether they're
> > attaching a patch?
>
> That's a good point. Given that it's possible to attach diffs which aren't
> actually patches to fix the bug (which, as I understand it, is what this
> flag is meant for), this couldn't replace the checkbox, but it could
> definitely prevent this type of mistake.
>
> Could we ignore the value of the checkbox if the content is not a diff?

Or maybe add a confirmation step (in cases where the value of the
checkbox and the content of the attachment are in conflict). Ignoring
form elements is (imho) not a good idea.

Emmet's comment seems to indicate a potential need for separating the concept of "this bug has a patch" from "this attachment is a patch". If so, I think that's a distinct bug which can be handled separately. Independent of that, I think it would be useful to prevent attachments from being flagged as patches if they are not.

Perhaps a 'What is a patch?' link with a nice description of what a
patch is and some guidelines about when to use the checkbox would help
improve the situation.

Documentation to elaborate "What is a patch" would be good, but it ought include not only unified diffs to code, but also other useful formats. Examples might include modified graphic files (e.g. background transparency for icons), recreated sounds (when sounds are non-free, or otherwise not correct), etc. While it is the common case, the preferred form for modification is not always text, and the preferred form for reception of alterations is not always diff (xdiff of .png (or even diff of .sng) is painful).

On Wed, Feb 06, 2008 at 10:44:18AM -0000, Emmet Hikory wrote:
> Documentation to elaborate "What is a patch" would be good, but it ought
> include not only unified diffs to code, but also other useful formats.
> Examples might include modified graphic files (e.g. background
> transparency for icons), recreated sounds (when sounds are non-free, or
> otherwise not correct), etc. While it is the common case, the preferred
> form for modification is not always text, and the preferred form for
> reception of alterations is not always diff (xdiff of .png (or even diff
> of .sng) is painful).

I think we need to reconsider the use cases we are trying to address.

The only one I know of is enabling developers to search for bugs which have
a patch (which I would define generally as a ready-to-merge fix) available,
so that they can review and merge them. For this use case, recording which
attachment is relevant is not necessary, and a simple "patch" tag (as used
in debbugs) would suffice, and avoid all of this confusion about identifying
attachments.

--
 - mdz

I'd completely agree with that. The existence of this bug grew out of an ubuntu-devel thread discussing whether the use of the "patch" tag was appropriate for that purpose.

(thread start: https://lists.ubuntu.com/archives/ubuntu-devel/2007-November/024790.html)

Personally, I'd be happy to see this bug marked "wontfix", the patch flag disappear as often abused at some point in the future, and the bugsquad encouraged to add the "patch" tag to bugs for which there is a clear and obvious patch (in whatever form). As a one-time transition, perhaps all bugs with attachments carrying the patch flag could have the "patch" tag added to avoid anything being lost.

A statement that this is the preferred method of identifying patches may be best made on the ubuntu-devel mailing list or the launchpad documentation indicating how to use Malone: in the absence of such a statement, interest in an automated solution remains an impediment to the effective deployment of a manual system.

Abel Deuring and I like Tom's idea in comment #2: if someone attaches something that claims to be a patch, but Launchpad is not able to automatically determine that it really *is* a patch, then insert a confirmation step that asks them if they really mean it's a patch. (The text would link to a canonical definition of "What is a patch?" as mentioned by Jamu Kakar in comment #4.)

@Matt and @Emmet: listing bugs-with-patches is part of https://dev.launchpad.net/Bugs/PatchTracking. IMHO the benefits of an automated solution seem worth trying for; the bugsquad shouldn't have to manually add this tag if we can reliably autodetect the presence of a patch -- either as an attachment, or even in a branch (e.g., bug has a branch attached and that branch gets commits and the bug goes to "fix committed" or "fix released"; while Ubuntu might then have the fix, we also want to make sure upstreams can find out about it and have easy access to it as well).

Note that bug #298099 contains a suggestion for making the "This attachment is a patch" checkbox less error-prone, too.

I agree that automation of the bugsquad workflow is vastly helpful. In implementing this automation, please also preserve the ability for bugsquad to override the automated detection, and indicate the bug indeed has a patch, even if it ends up being in a format different than that expected (e.g. replacement graphic or audio artifact, inline patch pasted in bug comment, etc.). In the case of replacement artifacts, this might be as simple as editing the attachment to confirm it really *is* a patch. In the case of inline patches, it would be nice if it didn't requrie someone to extract the patch and resubmit it as an attachment or branch, but rather just add a tag or press a button.

Emmet,

> In the case of inline patches, it would be nice if it didn't requrie
> someone to extract the patch and resubmit it as an attachment or branch,
> but rather just add a tag or press a button.

I have some doubts about this. Sure, if somebody is too lazy to add a really useful patch as an attchment and simply pastes it into a comment, the information "we have a patch" is lost. But including the diff in a comment may also indicate that the commenter simply wants to say "I know, this is a crappy workaround, but it fixes my actual problem", thus indicating that s/he does not expect the diff to be used as a real fix. Or am I putting too much interpretation into the location of a diff?

Abel,
    I think you're putting too much interpretation into the location of a diff. My experience with that class of patch has been that it's either a one-line change (and so may be easier to copy/paste from another window than to construct a file and load as an attachment), or a result of someone not being sufficiently motivated to learn that Malone expects patches to be uploaded inline. Some of the patches that have been presented to me in this way were both useful and appeared to be the correct solution.

    That said, I'm unsure that there is any value in someone extracting such a diff and uploading as a file just to set a flag, which flag I expect to become a significant attractor of developer attention once widely present. bugsquad could surely do it, and it may only be occasional patches that are submitted that way, but as long as an interface is being designed, I'd think it oughtn't be hard to allow for manual tweaking of the result in some way.

On Tue, Jan 5, 2010 at 1:44 PM, Emmet Hikory <email address hidden> wrote:
> In the case of inline patches, it would be nice if it
> didn't requrie someone to extract the patch and resubmit it as an
> attachment or branch, but rather just add a tag or press a button.
>

I agree this would be nice. It's worthy a separate bug, and wouldn't
help address this one.

jml

Note that it should include bugs that affect the user via dupes. The following new properties were committed in the fix for bug #505845:

   Bug.users_affected_with_dupes
   Bug.users_affected_count_with_dupes
   Bug.users_unaffected

Aaaaaaaargh. Commented in wrong bug, please ignore above.

(And have now filed bug #512489 about why one should be able to remove one's own comments when there are no followups.)

Fixed in devel r10253 <http://bazaar.launchpad.net/~launchpad-pqm/launchpad/devel/revision/10253>