Skip to content

Conversation

fwolfst
Copy link
Contributor

@fwolfst fwolfst commented Jan 15, 2018

Replace many occurrences of the old (non-https and HTTP/301 moved permanently anyway) URL to MIT license by newer link.

(Archive) First_ attempt was:
In two steps, update the links as described in the issue.

First commit: sed everything.
Second commit: Fix whitespace syntactic sugar that got bitter due to different length of URLs.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be submitted upstream for the ctaes subtree, if wanted

Copy link
Contributor Author

@fwolfst fwolfst Jan 15, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point, I do not know the codebase very well yet. Issued PR to ctaes in bitcoin-core/ctaes#13 .

@morcos
Copy link
Contributor

morcos commented Jan 16, 2018

@fwolfst Please see the notes on Scripted Diffs in doc/developer-notes.md.

Please note that there has been a large increase in PR activity recently (a good problem to have!) and reviewers are struggling to keep up, so simple refactoring, language changes or other cleanup sometimes don't get a lot of immediate attention.

@maflcko
Copy link
Member

maflcko commented Jan 16, 2018

You could also help with review, if you feel like it. Due to the amount of open pull requests, there are some easier ones to get started.

Even with limited understanding of the code, just compiling them and passing in edge-cases until they break is useful.

https://github.com/bitcoin/bitcoin/pulls?q=is%3Apr+is%3Aopen+label%3ADocs
https://github.com/bitcoin/bitcoin/pulls?q=is%3Apr+is%3Aopen+label%3ATests
https://github.com/bitcoin/bitcoin/pulls?q=is%3Apr+is%3Aopen+label%3ARPC%2FREST%2FZMQ

@fanquake fanquake added the Docs label Jan 18, 2018
@fanquake fanquake changed the title Trivial: Issue 12190: Update http URL of MIT license to use https Trivial: Update http URL of MIT license to use https Jan 18, 2018
@fwolfst
Copy link
Contributor Author

fwolfst commented Jan 24, 2018

@fwolfst Please see the notes on Scripted Diffs in doc/developer-notes.md.
@morcos Thanks for pointing that out, I'll happily do that. Sorry for being such a pita-noob, whats the suggested work flow now? Closing this PR and reopen a new one?

Edit Will figure that one out - will re-read CONTRIBUTE.md and just try if a changed commit message influences the PR.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please send secp256k1 changes upstream.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for pointing that out. Issued in bitcoin-core/secp256k1#503 .

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Univalue changes also need to go upstream.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for pointing that out. Issued at jgarzik/univalue#47 .

@fanquake
Copy link
Member

fanquake commented Jan 29, 2018

@fwolfst Please don't close PRs and open new ones with the same changes. For this to be merged, you'll need to remove any changes to sub-trees in this repository (you can open PRs with the changes upstream if you'd like).

Then rebase and update the commits to be proper scripted diffs, see developer-notes.

A large diff like this will likely be merged after a branch off.

@maflcko
Copy link
Member

maflcko commented Feb 10, 2018

Needs rebase if still relevant

@fwolfst
Copy link
Contributor Author

fwolfst commented Feb 11, 2018 via email

@laanwj
Copy link
Member

laanwj commented Feb 13, 2018

From a 'use https everywhere' perspective I agree with this change, but changing every single source file seems a bit much.

@promag
Copy link
Contributor

promag commented Feb 13, 2018

New URL ACK:

curl -v http://www.opensource.org/licenses/mit-license.php
< HTTP/1.1 301 Moved Permanently
< Location: http://opensource.org/licenses/mit-license.php

curl -v http://opensource.org/licenses/mit-license.php
< HTTP/1.1 301 Moved Permanently
< Location: https://opensource.org/licenses/mit-license.php

curl -v https://opensource.org/licenses/mit-license.php
< HTTP/1.1 200 OK

Agree with @morcos regarding scripted diff.

@laanwj does this change conflicts with other branches?

@laanwj
Copy link
Member

laanwj commented Feb 13, 2018

@laanwj does this change conflicts with other branches?

Depending on how github computes conflicts (I'm not sure if, and if so how much, lines of context it uses - but how easy this builds up a list of conflicts below is one omen), possibly every single one.

@fwolfst
Copy link
Contributor Author

fwolfst commented Feb 13, 2018

I will rebase soon and include a scripted_diff commit (I will have to figure out how this works WITHIN this PR). The target is moving a bit, because changes have already been accepted in included projects (jgarzik/univalue#47 and I guess bitcoin-core/secp256k1#503 will follow).

@fwolfst fwolfst force-pushed the 12190-UPDATE_MIT_LINK_TO_HTTPS branch from 9c47171 to 8d16208 Compare February 14, 2018 07:23
@fwolfst
Copy link
Contributor Author

fwolfst commented Feb 14, 2018

I am not git native yet, but I think 8d16208 comes close to what should be (have been) done:

I reset my branch, merged upstream and created a new "scripted" diff.

There are still some 70 occurences of the old URL, which I will gladly scripted-diff-replace once I get green lights here.

@fanquake
Copy link
Member

@fwolfst 8d16208 is still modifying the univalue subtree.

@fwolfst fwolfst force-pushed the 12190-UPDATE_MIT_LINK_TO_HTTPS branch from 8d16208 to a164d7f Compare February 14, 2018 12:31
@fwolfst
Copy link
Contributor Author

fwolfst commented Feb 14, 2018

@fanquake Dealt with in fwolfst@a164d7f . Thanks.

@maflcko
Copy link
Member

maflcko commented Feb 14, 2018

utACK a164d7f6dd5df1dbb9a77c56a57350f469e4b2da

At this point you might as well bump the copyright headers altogether. Please add another commit with the dates bumped to 2018 (see ./contrib/devtools/README.md).

@fwolfst
Copy link
Contributor Author

fwolfst commented Feb 14, 2018

@MarcoFalke Yep, cool. Tough my preferred way to deal with the copyright extension would be the following: you create an issue for that and assign it to me - I'd personally prefer to keep that stuff in a separate PR, as it has a different class of legal implications, but if the majority thinks otherwise I'd give it a go.

@fanquake
Copy link
Member

fanquake commented Feb 14, 2018

@fwolfst Creating additional issues isn't required. Bumping the year of all copyright headers is trivial, and easily verifiable. See #12062 & #9450 for examples. I'm not sure what you mean by legal implications?

@fwolfst
Copy link
Contributor Author

fwolfst commented Feb 14, 2018

Well its a legal statement indicating when (in certain countries) these (versions of the) files will enter the public domain. Anyway. Done.

@maflcko
Copy link
Member

maflcko commented Feb 20, 2018

Needs rebase

@fwolfst fwolfst force-pushed the 12190-UPDATE_MIT_LINK_TO_HTTPS branch from 3297f28 to 25ac012 Compare February 21, 2018 06:58
@fwolfst
Copy link
Contributor Author

fwolfst commented Feb 21, 2018

@MarcoFalke Thanks for seeing that. Done. Now, needs merge ;)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My mistake. Glitch in my workflow after new rebase experience. Will fix soon. If I might ask: currently I replace old commits in this PR/my branch with hard resets and push -f, to keep changes small and honor the scripted_diff checks (except in last case, my apologies) - is that a correct thing to do?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Jup, correct. You can also check the scripted diffs with the ./contrib/devtools/commit-script-check.sh script.

@fwolfst fwolfst force-pushed the 12190-UPDATE_MIT_LINK_TO_HTTPS branch from 25ac012 to 506c92c Compare February 23, 2018 08:27
@fwolfst
Copy link
Contributor Author

fwolfst commented Feb 23, 2018

Corrected in cdc7d3c .
And also, I adjusted the URL. The canonical URL (and the one linked already in Bitcoins README as well as from within the opensource.org site) is https://opensource.org/licenses/MIT .

curl --silent --head  https://opensource.org/licenses/mit-license.php -o -  | grep canon
Link: </licenses/MIT>; rel="canonical",</node/66>; rel="shortlink"

@laanwj
Copy link
Member

laanwj commented Feb 23, 2018

And also, I adjusted the URL. The canonical URL (and the one linked already in Bitcoins README as well as from within the opensource.org site) is https://opensource.org/licenses/MIT .

Yup, good catch! If we're going to change this, let's be 100% sure we're using the correct address, and the one that is most likely to stay the same going forward.

Which makes me wonder (sorry for always being a heretic) - what is the motivation for an external link at all? We carry a copy of the license in the repository, after all. Is the answer to cover for individually copied files?

@fwolfst
Copy link
Contributor Author

fwolfst commented Feb 23, 2018 via email

@laanwj
Copy link
Member

laanwj commented Feb 23, 2018

Thanks. To be clear I'm not advocating for removing the headers completely! that would require an infinite amount of discussion which I'm not going to spend my time left on earth partaking in. I was just curious as to an external URL link, as it is both more[1] and less[2] self-contained at the same time.

*1 disambiguation of "MIT license" for individual files, as you say, without having to copy the whopping thing into every single file
*2 the link is external, so it could potentially change under us

But to be clear, keeping it like this is OK to me. utACK.
I've verified that commit 506c92ce208f2e620314dd9952516ebbd68b3f20 and cdc7d3ce48df4575ba1313a7225372008763c31b match the output of their scripts.

@maflcko
Copy link
Member

maflcko commented Feb 23, 2018

Hmm, find . will recurse into .git and potentially corrupt the directory. Could you change that to git ls-files | xargs sed ..., please?

Sorry for mentioning that so late.

This commit deals with one part of bitcoin#12190 by replacing many occurrences
of
  http://www.opensource.org/licenses/mit-license.php
with
  https://opensource.org/licenses/MIT
. A second commit could tackle the other occurrences where either
"comment-star-boxes" (/* 80 chars */) are used or the URL appears
in a different context.
The src/univalue/ path is ignored, as it references
https://github.com/jgarzik/univalue , where a similar issue is
ady
closed (jgarzik/univalue#47) and waiting to be
included into this (bitcoin) repository.

-BEGIN VERIFY SCRIPT-
git ls-files | grep --invert-match 'src/univalue' | xargs sed -i \
-e 's,http://www.opensource.org/licenses/mit-license.php.$,https://opensource.org/licenses/MIT.,g'
-END VERIFY SCRIPT-
@laanwj
Copy link
Member

laanwj commented Feb 23, 2018 via email

@fwolfst fwolfst force-pushed the 12190-UPDATE_MIT_LINK_TO_HTTPS branch from cdc7d3c to 23f4029 Compare February 23, 2018 14:13
Used the contrib/devtools/copyright_header.py script to extend
Copyright in files edited in 2018.

-BEGIN VERIFY SCRIPT-
contrib/devtools/copyright_header.py update .
-END VERIFY SCRIPT-
@fwolfst
Copy link
Contributor Author

fwolfst commented Feb 23, 2018

@laanwj Yes, you are right. Unfortunately the --exclude option of git ls-files seems to only be able to exclude untracked files.
But facepalm - messed up the commit message in 23f4029 (ady instead of already). Also there are still occurrences of the old URL in those shiny /* boxes */. If there is green lights, I'd like to change the commit message, get this merged and come back with the next step in a couple of days/weeks.

@maflcko
Copy link
Member

maflcko commented Feb 25, 2018

utACK b3047f4

@maflcko
Copy link
Member

maflcko commented Mar 22, 2018

@fwolfst Needs rebase

@fanquake
Copy link
Member

fanquake commented Apr 8, 2018

Closing now that #12914 is open.

@fanquake fanquake closed this Apr 8, 2018
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants