Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

A weird commit from 1970 #2713

Closed
mgol opened this issue Sep 5, 2015 · 17 comments
Closed

A weird commit from 1970 #2713

mgol opened this issue Sep 5, 2015 · 17 comments

Comments

@mgol
Copy link
Contributor

mgol commented Sep 5, 2015

GitHub detects one Git commit by @skomski from... 1970. This is breaking the contributors page. It was OK few months ago. Screenshot:
screen shot 2015-09-05 at 19 15 08

@rvagg
Copy link
Member

rvagg commented Sep 5, 2015

Something new for node-accept-pull-request to test for. If the commit is close to HEAD I'd be inclined to support a rebase and fix.

@jbergstroem
Copy link
Member

We briefly discussed this here #2402 (comment) where you also can find the relevant commit.

@thefourtheye
Copy link
Contributor

@rvagg it was first spotted in #2402 (comment). I am not sure if we can fix it now :(

@thefourtheye
Copy link
Contributor

@jbergstroem speak of the timing :D

@mscdex
Copy link
Contributor

mscdex commented Sep 6, 2015

Too bad when you highlight a span of time in that github timeline, it doesn't zoom that portion to fit the container.

@rvagg
Copy link
Member

rvagg commented Sep 6, 2015

Here's what's on top of that commit so far:

2015-09-04 14:48:13 -0700 - 2015-09-05 10:09:16 -0400 - deps: create .npmrc during npm tests
2015-09-04 14:47:14 -0700 - 2015-09-05 10:08:11 -0400 - deps: upgrade to npm 2.14.2
2015-08-11 00:48:49 -0700 - 2015-09-04 20:28:06 -0700 - deps: backport 75e43a6 from v8 upstream (again)
2015-09-04 15:41:21 +1000 - 2015-09-05 13:26:58 +1000 - build: fix .pkg creation tooling
2015-09-03 16:22:57 +1000 - 2015-09-05 11:51:02 +1000 - doc: add TSC meeting minutes 2015-09-02
2015-09-04 12:48:20 +0200 - 2015-09-04 20:42:59 +0200 - doc: update environment vars in manpage and --help
2015-09-03 10:10:30 +0200 - 2015-09-04 11:28:40 -0600 - src: use standard conform snprintf on windows
2015-09-03 10:10:29 +0200 - 2015-09-04 11:27:07 -0600 - src: fix buffer overflow for long exception lines
2015-09-02 14:54:24 +0530 - 2015-09-04 17:12:53 +0200 - doc,test: enable recursive file watching in Windows
2015-09-01 10:51:17 +0200 - 2015-09-04 08:06:52 +0200 - buffer: SlowBuffer only accept valid numeric values
2015-09-04 12:25:31 +1000 - 2015-09-04 12:34:44 +1000 - test: fix use of `common` before required
2015-08-31 17:10:53 -0700 - 2015-09-03 16:38:16 -0700 - deps: upgrade V8 to 4.5.103.30
2015-08-28 09:02:32 +0200 - 2015-09-03 16:38:16 -0700 - src: re-enable fast math on arm
2015-08-23 12:53:05 -0700 - 2015-09-03 16:38:16 -0700 - src: enable vector ics on arm again
2015-07-18 11:34:16 +0200 - 2015-09-03 16:38:15 -0700 - src: replace usage of v8::Handle with v8::Local v8::Handle is deprecated: https://codereview.chromium.org/1224623004
2015-07-01 23:47:37 +0200 - 2015-09-03 16:38:15 -0700 - src: enable v8 deprecation warnings and fix them
2015-08-23 07:24:54 -0700 - 2015-09-03 16:38:15 -0700 - test: fix test-repl-tab-complete.js for V8 4.5
2015-07-07 15:27:14 -0700 - 2015-09-03 16:38:15 -0700 - contextify: ignore getters during initialization
2015-08-23 06:28:57 -0700 - 2015-09-03 16:38:15 -0700 - src: apply debug force load fixups from 41e63fb
2015-08-23 06:09:40 -0700 - 2015-09-03 16:38:15 -0700 - deps: upgrade V8 to 4.5.103.24
2015-09-02 12:23:49 -0400 - 2015-09-03 17:32:51 -0400 - events,lib: don't require EE#listenerCount()
2015-08-14 10:07:18 +0200 - 2015-09-03 13:52:14 -0700 - build: add --enable-asan with builtin leakcheck
2015-09-02 14:16:24 -0700 - 2015-09-03 10:51:30 -0700 - child_process: check execFile and fork args
2015-08-28 19:56:22 -0700 - 2015-09-03 10:39:12 -0700 - test: refactor to eliminate flaky test
2015-08-28 16:58:18 -0400 - 2015-09-03 12:11:45 -0400 - doc: update url doc to account for escaping
2015-07-25 16:09:59 +1000 - 2015-09-03 06:29:27 -0400 - doc: reorder collaborators by their usernames
2015-09-02 11:30:00 +0200 - 2015-09-03 00:27:31 -0400 - test: mark eval_messages as flaky
2015-08-31 00:49:34 +0200 - 2015-09-02 19:39:35 -0400 - child_process: add callback parameter to .send()
1970-08-16 16:09:02 +0200 - 2015-09-02 18:39:39 -0400 - src: fix memory leak in ExternString

I personally have no qualms about rebasing a fix on that commit which would obviously mean changing hashes from there onwards, but I'm generally pretty relaxed wrt my git logs. @nodejs/tsc anyone feel strongly about not wanting to mess up the HEAD of the active branches (this would involve v3.x and v4.x too)?

@silverwind
Copy link
Contributor

We must save our history!

just-do-it

rvagg referenced this issue Sep 6, 2015
v8 will silently return an empty handle
which doesn't delete our data if string length is
above String::kMaxLength

Fixes: #1374
PR-URL: #2402
Reviewed-By: trevnorris - Trevor Norris <trev.norris@gmail.com>
Reviewed-By: indutny - Fedor Indutny <fedor.indutny@gmail.com>
Reviewed-By: bnoordhuis - Ben Noordhuis <info@bnoordhuis.nl>
@Fishrock123
Copy link
Contributor

Uh, +1 fixing the commit.

rvagg pushed a commit that referenced this issue Sep 6, 2015
v8 will silently return an empty handle
which doesn't delete our data if string length is
above String::kMaxLength

Fixes: #1374
PR-URL: #2402
Reviewed-By: trevnorris - Trevor Norris <trev.norris@gmail.com>
Reviewed-By: indutny - Fedor Indutny <fedor.indutny@gmail.com>
Reviewed-By: bnoordhuis - Ben Noordhuis <info@bnoordhuis.nl>

Amended by @rvagg to change author date from
  "1970-08-16 16:09:02 +0200"
to
  "2015-08-16 16:09:02 +0200"
as per discussion @ #2713
rvagg pushed a commit that referenced this issue Sep 6, 2015
v8 will silently return an empty handle
which doesn't delete our data if string length is
above String::kMaxLength

Fixes: #1374
PR-URL: #2402
Reviewed-By: trevnorris - Trevor Norris <trev.norris@gmail.com>
Reviewed-By: indutny - Fedor Indutny <fedor.indutny@gmail.com>
Reviewed-By: bnoordhuis - Ben Noordhuis <info@bnoordhuis.nl>

Amended by @rvagg to change author date from
  "1970-08-16 16:09:02 +0200"
to
  "2015-08-16 16:09:02 +0200"
as per discussion @ #2713
rvagg pushed a commit that referenced this issue Sep 6, 2015
v8 will silently return an empty handle
which doesn't delete our data if string length is
above String::kMaxLength

Fixes: #1374
PR-URL: #2402
Reviewed-By: trevnorris - Trevor Norris <trev.norris@gmail.com>
Reviewed-By: indutny - Fedor Indutny <fedor.indutny@gmail.com>
Reviewed-By: bnoordhuis - Ben Noordhuis <info@bnoordhuis.nl>

Amended by @rvagg to change author date from
  "1970-08-16 16:09:02 +0200"
to
  "2015-08-16 16:09:02 +0200"
as per discussion @ #2713
rvagg pushed a commit that referenced this issue Sep 6, 2015
v8 will silently return an empty handle
which doesn't delete our data if string length is
above String::kMaxLength

Fixes: #1374
PR-URL: #2402
Reviewed-By: trevnorris - Trevor Norris <trev.norris@gmail.com>
Reviewed-By: indutny - Fedor Indutny <fedor.indutny@gmail.com>
Reviewed-By: bnoordhuis - Ben Noordhuis <info@bnoordhuis.nl>

Amended by @rvagg to change author date from
  "1970-08-16 16:09:02 +0200"
to
  "2015-08-16 16:09:02 +0200"
as per discussion @ #2713
@rvagg
Copy link
Member

rvagg commented Sep 6, 2015

OK everyone, @nodejs/collaborators, I've applied the heavy hand after not getting any objection here and pinging various TSC members via other channels. So heads-up, you're going to have to reset your branches to the new remote branches.

for i ingit branch -r | grep origin; do echo $i; git log --oneline $i | grep 'src: fix memory leak in ExternString'; done showed that the commit had made it's way to the following branches:

origin/master
origin/v3.3.1-release
origin/v3.x
origin/v4.0.0-rc
origin/v4.x

The v3.3.1-release branch is just a release proposal and the only tag we're tampering with here is one I made for v4.0.0-rc.1 that sits on the v4.0.0-rc branch which has patches I've prematurely landed just to get an RC out.

I've amended the commit on each of these branches, using git commit --amend --date "Sun, 16 Aug 2015 16:09:02 +0200" which I believe is the correct original author date (the GitHub API tells me the PR was made not long after this time so I just adjusted the year portion of the original date). This means that downstream commits, listed above, have been rebased and had their hashes updated on each of the impacted branches. The changes have all been force pushed back up. This will obviously cause a tiny headache for those who have recent copies on your local machines. git reset origin/branch --hard I think is probably the easiest way to do this, or simply delete your local branch and checkout the nodejs/node version again. I've also recreated the v4.0.0-rc.1 tag and force pushed that.

On the brighter side, https://github.com/nodejs/node/graphs/contributors is fixed and the history is sane once again.

@silverwind
Copy link
Contributor

This should fix your local master (repeat for each branch, local changes will be lost):

git reset --hard 617ee324~1 && git fetch origin && git merge --ff-only origin/master

@mgol
Copy link
Contributor Author

mgol commented Sep 6, 2015

@silverwind Why not just (when you're on local master):

git fetch origin
git reset --hard origin/master

? You don't need to remember any commit hash then.

@mgol
Copy link
Contributor Author

mgol commented Sep 6, 2015

@rvagg Anything left or should this issue be closed now?

@thefourtheye
Copy link
Contributor

@mzgol I think this can be closed now. Thanks for bringing this to our notice :-)

@silverwind
Copy link
Contributor

These PRs got broken and need to be rebased now:

#2694, #2699 , #2700, #2701, #2702, #2706, #2707, #2710, #2712, #2714

@mzgol you're probably right. I was just thinking it's a bit safer to do fast-forward merges only.

@ChALkeR
Copy link
Member

ChALkeR commented Sep 6, 2015

@silverwind, not just those. #2534, for example, too.
All the PRs that were published or rebased in the last few days need to be rebased now.

@skomski
Copy link
Contributor

skomski commented Sep 7, 2015

I am sorry to have caused so much trouble. 😞 Hope I can make it up with some pull requests 😃

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

10 participants