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

Status: RESOLVED FIXED

(DevTools :: Netmonitor, enhancement, P3)

Description

[Daniel Stenberg [:bagder]]

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

Comment 1

[Daniel Stenberg [:bagder]]

Attachment: 0001-Bug-1402828-don-t-use-X-HEAD-don-t-send-H-Host-r.patch

Comment 2

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

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.

Comment 3

[Daniel Stenberg [:bagder]]

Attachment: 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=

Comment 4

[Jan Honza Odvarko [:Honza] (always need-info? me)]

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

Comment 5

[Daniel Stenberg [:bagder]]

Attachment: v3-0001-Bug-1402828-improve-copy-as-cURL-r-Honza.patch

Updated and rebased to apply fine after Bug 1253487.

Comment 6

[Jan Honza Odvarko [:Honza] (always need-info? me)]

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

Comment 7

[Daniel Stenberg [:bagder]]

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.

Comment 8

[Daniel Stenberg [:bagder]]

Attachment: 0001-Bug-1402828-improve-copy-as-cURL-r-Honza.patch

Comment 9

[Jan Honza Odvarko [:Honza] (always need-info? me)]

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

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4d059e20a773
Improve "copy as cURL". r=Honza

Comment 11

[Andreea Pavel [:apavel]]

Backed out for failing devtools/client/netmonitor/test/browser_net_copy_as_curl.js 

Push that started the failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=4d059e20a773dc661f0d2fd45d2118230398f720

Error log: https://treeherder.mozilla.org/logviewer.html#?job_id=144833584&repo=mozilla-inbound&lineNumber=4308

Backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/d563998a63ffbc44cf022286322705d190305321

Comment 12

[Jan Honza Odvarko [:Honza] (always need-info? me)]

@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

Comment 13

[Daniel Stenberg [:bagder]]

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.

Comment 14

[Daniel Stenberg [:bagder]]

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...

Comment 15

[Tom Glowka]

Attachment: Bug-1402828-copy-as-cURL-test adjustments.patch

Comment 16

[Tom Glowka]

Passing on try now: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e46f85e23129a2625efa65ab5119465ce16e2218

Comment 17

[Jan Honza Odvarko [:Honza] (always need-info? me)]

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

Comment 18

[Daniel Stenberg [:bagder]]

Thanks Tom!

Comment 19

[Pulsebot]

Pushed by [e-mail]:
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

Comment 20

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/07448700dd65
https://hg.mozilla.org/mozilla-central/rev/eaa2ef08792b