"copy as curl": don't use -X for HEAD, don't send custom Host:

RESOLVED FIXED in Firefox 61

Status

enhancement
P3
normal
RESOLVED FIXED
2 years ago
Last year

People

(Reporter: bagder, Assigned: glowka.tom)

Tracking

Trunk
Firefox 61

Firefox Tracking Flags

(firefox61 fixed)

Details

Attachments

(2 attachments, 3 obsolete attachments)

Simplify the generated command lines:

1. don't use "-X HEAD" when -I is used, as -I implies HEAD

2. don't send custom Host: headers as curl will create them itself based on the URL
Assignee: nobody → daniel
Status: NEW → ASSIGNED
Attachment #8911770 - Flags: review?(jsnajdr)
Comment on attachment 8911770 [details] [diff] [review]
0001-Bug-1402828-don-t-use-X-HEAD-don-t-send-H-Host-r.patch

I'll redirect this to Honza, since I don't think Jarda is active lately.

I did notice the commit message seems to have some kind of encoding issue.
Attachment #8911770 - Flags: review?(jsnajdr) → review?(odvarko)
Another try to fix the encoding glitch and correct r=
Attachment #8911770 - Attachment is obsolete: true
Attachment #8911770 - Flags: review?(odvarko)
Attachment #8912110 - Flags: review?(odvarko)
Comment on attachment 8912110 [details] [diff] [review]
v2-0001-Bug-1402828-don-t-use-X-HEAD-don-t-send-H-Host-r-.patch

Review of attachment 8912110 [details] [diff] [review]:
-----------------------------------------------------------------

The patch seems reasonable, thanks for working on this!

Just two things:

1) The commit message is weird, using 'Subject' and wrong reviewer nickname. It should be something as follows:
Bug 1402828 - Improve copy as curl action; r=Honza

2) Please rebase on the latest head. I can't apply it cleanly.

Honza
Attachment #8912110 - Flags: review?(odvarko) → review-
Updated and rebased to apply fine after Bug 1253487.
Attachment #8912110 - Attachment is obsolete: true
Attachment #8913596 - Flags: review?(odvarko)
Comment on attachment 8913596 [details] [diff] [review]
v3-0001-Bug-1402828-improve-copy-as-cURL-r-Honza.patch

Review of attachment 8913596 [details] [diff] [review]:
-----------------------------------------------------------------

The commit message is still wrong, please remove the "Subject: [PATCH v3] " part
nit: ..and there should be '-' (dash) after the bug#

See also:
https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Committing_Rules_and_Responsibilities#Commit_message_restrictions

Honza
I've landed a large amount of patches using such a subject part as it is the standard git format-patch output. The [PATCH] parts gets stripped out on merge.
Attachment #8913596 - Attachment is obsolete: true
Attachment #8913596 - Flags: review?(odvarko)
Attachment #8913606 - Flags: review?(odvarko)
Comment on attachment 8913606 [details] [diff] [review]
0001-Bug-1402828-improve-copy-as-cURL-r-Honza.patch

Review of attachment 8913606 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for the patch!

Honza
Attachment #8913606 - Flags: review?(odvarko) → review+
Keywords: checkin-needed
@Daniel, the log says:

TEST-UNEXPECTED-FAIL | devtools/client/netmonitor/test/browser_net_copy_as_curl.js | Timed out while polling clipboard for pasted data

Honza
Flags: needinfo?(odvarko) → needinfo?(daniel)
Yeah, there's apparently a test verifying the wrong behavior. I clearly never ran it on try, which is totally my fault for missing that step.
Flags: needinfo?(daniel)
This is still a bug that should be fixed, but this is not my area and I'm out of time for it. The patch is fine, the existing tests are not...
Assignee: daniel → nobody
Status: ASSIGNED → NEW
Assignee: nobody → glowka.tom
Status: NEW → ASSIGNED
Attachment #8963375 - Flags: review?(odvarko)
Comment on attachment 8963375 [details] [diff] [review]
Bug-1402828-copy-as-cURL-test adjustments.patch

Review of attachment 8963375 [details] [diff] [review]:
-----------------------------------------------------------------

Looks great to me Tom!

Don't forget to set checkin-needed in order to land both patches.

Thanks,
Honza
Attachment #8963375 - Flags: review?(odvarko) → review+
Keywords: checkin-needed
Thanks Tom!
https://hg.mozilla.org/mozilla-central/rev/07448700dd65
https://hg.mozilla.org/mozilla-central/rev/eaa2ef08792b
Status: ASSIGNED → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → Firefox 61
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.