debdiff does not look like a patch

Maybe bugs doesn't like the .debdiff extension?

I just tried uploading the same patch without the extension on staging. I got the same question.

Yes, it's the extension that isn't liked. There are two ways to consider this bug: 1) it's an issue with zope.contenttype and Python's mimetypes module which relies on extensions rather than magic numbers or similar to determine file type, or 2) it's an issue with Launchpad relying on zope.contenttype rather than running a job that tests compatibility with patch or bzrz patch or something.

Emmet, you are absolutely right with your diagnosis. Checking the file content somehow would indeed be a better option. Several people tried already file(1) -- problem is that its implementation currently used in Ubuntu lets you suspect that it is on drugs when you run it on "bzr diff" output. For example,

   bzr diff -r 10511..10512 | file -

for lp:~launchpad-pqm/launchpad/devel/ return "/dev/stdin: ASCII English text"...

Trying "patch" or "bzr patch" is an interesting option, but will break for projects that don't use Launchpad for code hosting. Also, it might be difficult to distinguish cases where the file in question indeed contains diff data but where the patch can't be cleanly applied and the case where the file contains some completely different data.

For now, I' think we should simply add the file name suffix ".debdiff" to the extensions Launchpad considers to be patch data.

For those watching the bug closely, don't read my un-milestoning it as de-prioritizing the bug. The branch won't land in time to make 10.03, and I've removed the milestone completely because the script we have to add tags for QA will assign the bug to the current milestone at that time it lands.

Just a reference: for a good discussion of how Launchpad determines an attachment's content type, see http://code.launchpad.net/~adeuring/launchpad/bug-172501/+merge/18187 .

> problem is that its implementation currently used in Ubuntu lets you suspect that it is on drugs when you run it on "bzr diff" output. For example,
>
> bzr diff -r 10511..10512 | file -
> for lp:~launchpad-pqm/launchpad/devel/ return "/dev/stdin: ASCII English text"...

Um... has anyone reported this weird behaviour as a bug somewhere? Link?

Testing by adding a ".debdiff" patch attachment right now.

Okay, the bugfix is not yet live on edge. Will test again after edge/staging updates are back.

Basing this on a filename extension is broken. .debdidf isn't meaningful.

... .debdiff

No, it's still an improvement. Remember, this is all about a heuristic anyway: the user uploads an attachment, claiming it is a patch, and the question is, should Launchpad believe the user vs should Launchpad ask the user to confirm that it's really a patch?

The logic that determines whether we interpose this confirmation step has never been perfect, but this change makes it better. If a file ends in ".debdiff", ".diff", or ".patch", it's pretty likely to be a patch -- the chances that someone clicked on the wrong file in the upload file browser are much higher than the chances that they mis-named the file in the first place (or got a mis-named file from elsewhere). So we're adjusting our confirmation bias accordingly.

Launchpad can still guess wrong, of course. But the only consequence is that an attachment may be marked as a patch when it's not really a patch.

Obviously, the ideal solution would look at the contents of the file and see if it "looks like a patch". That's harder to code, though, and didn't seem like a good use of time, considering that it's not important to get perfect answers every time here.

I don't recall ever getting the confirmation dialogue before I filed my dupe of this bug. I find it pretty user hostile.

Maybe Ubuntu developers could be exempted from the test? Presumably they'd know if it's a patch or not.

I agree the confirmation dialogue is user-hostile in some circumstances, but this bugfix makes that dialog come up strictly *less* often. It can only reduce the frequency with which you get the dialog, never increase it.

I kind of like the idea of considering the person in determining whether or not to present the dialog, though. Want to file a separate bug report about that? (I think this bug reports' comments aren't really the place, for the reasons given above.)

Do you find the dialog user-hostile in practice, or did you just now deliberately stimulate it to see what it looks like? I mean, it does provide some protection from uploading the wrong file (though since the "patch" label is revokable, it's debatable whether that protection is really necessary).

I'll file another bug.

Since I just told the system it was a patch, I think it's user hostile for the system to tell me I'm wrong. It's doubly frustrating when the system is slow and wrong.

Fortunately (for me), as an Ubuntu core-dev it's not something I hit regularly (generally I just upload ), but I still need to use debdiffs for security updates.

I think your point about the action being reversable is a good one. If it were up to me I'd remove the check entirely. I think if the system is going to second guess a user in a situation like this it should have a high confidence of being correct.

I agree with your comment that it's not worth the effort to make it that good.

Fixed in stable r10620 <http://bazaar.launchpad.net/~launchpad-pqm/launchpad/stable/revision/10620>

On 7 April 2010 04:59, Karl Fogel <email address hidden> wrote:
> I agree the confirmation dialogue is user-hostile in some circumstances,
> but this bugfix makes that dialog come up strictly *less* often.  It can
> only reduce the frequency with which you get the dialog, never increase
> it.

There's another bug pointing out that the dialog is a bit
confused/confusing about whether you mean "this is a patch that can be
parsed by /usr/bin/patch" or "this is a patch that fixes this bug."
To me this seems related: sniffing the content can only possibly tell
you about the first, but Launchpad's patch workflow seems to be about
the second.

--
Martin <http://launchpad.net/~mbp/>