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

RESOLVED FIXED in Firefox 61

Status

P3
normal
RESOLVED FIXED
a year ago
7 months ago

People

(Reporter: bagder, Assigned: glowka.tom)

Tracking

Trunk
Firefox 61

Firefox Tracking Flags

(firefox61 fixed)

Details

Attachments

(2 attachments, 3 obsolete attachments)

(Reporter)

Description

a year ago
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
(Reporter)

Comment 1

a year ago
Created attachment 8911770 [details] [diff] [review]
0001-Bug-1402828-don-t-use-X-HEAD-don-t-send-H-Host-r.patch
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)
(Reporter)

Comment 3

a year ago
Created attachment 8912110 [details] [diff] [review]
v2-0001-Bug-1402828-don-t-use-X-HEAD-don-t-send-H-Host-r-.patch

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)
status-firefox57: --- → fix-optional
Priority: -- → P3
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-
(Reporter)

Comment 5

a year ago
Created attachment 8913596 [details] [diff] [review]
v3-0001-Bug-1402828-improve-copy-as-cURL-r-Honza.patch

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
(Reporter)

Comment 7

a year ago
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.
(Reporter)

Comment 8

a year ago
Created attachment 8913606 [details] [diff] [review]
0001-Bug-1402828-improve-copy-as-cURL-r-Honza.patch
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+
(Reporter)

Updated

a year ago
Keywords: checkin-needed

Comment 10

a year ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4d059e20a773
Improve "copy as cURL". r=Honza
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)
(Reporter)

Comment 13

a year ago
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)
(Reporter)

Comment 14

a year ago
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
(Assignee)

Comment 15

10 months ago
Created attachment 8963375 [details] [diff] [review]
Bug-1402828-copy-as-cURL-test adjustments.patch
(Assignee)

Updated

10 months ago
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+
(Assignee)

Updated

10 months ago
Keywords: checkin-needed
(Reporter)

Comment 18

10 months ago
Thanks Tom!

Comment 19

10 months ago
Pushed by csabou@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/07448700dd65
improve "copy as cURL", r=Honza
https://hg.mozilla.org/integration/mozilla-inbound/rev/eaa2ef08792b
"copy as cURL" test adjustments; r=Honza
Keywords: checkin-needed

Comment 20

10 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/07448700dd65
https://hg.mozilla.org/mozilla-central/rev/eaa2ef08792b
Status: ASSIGNED → RESOLVED
Last Resolved: 10 months ago
status-firefox61: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 61
status-firefox57: fix-optional → ---
status-firefox58: affected → ---

Updated

7 months ago
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.