console: admin.clearHistory() command by sorin · Pull Request #15614 · ethereum/go-ethereum

sorin · 2017-12-06T06:30:14Z

No description provided.

holiman · 2017-12-06T08:19:20Z

I have not tested this, but based manual look at the code, I wonder if it actually deletes the history-file from disk ?

fjl · 2017-12-06T09:44:30Z

Please call os.Remove(c.histPath) here.

done. my original reasoning was that history is being persisted at the end of the session but I agree this is safer.

… sorin-console-clear-history

sorin · 2017-12-07T06:28:40Z

@fjl added a one liner test fix, please approve. thanks!

holiman · 2017-12-07T08:19:18Z

 	AppendHistory(command string)

+	// ClearHistory clears the entire history
+	ClearHistory()


I'd suggest ClearHistory() error, now that the os.remove may return an error

good suggestion, but since https://github.com/sorin/go-ethereum/blob/sorin-console-clear-history/vendor/github.com/peterh/liner/common.go#L148-L152 does not return an error I'd say ClearHistory() (which is basically a wrapper on top of it) stays like this. Also os.remove is not called inside this ClearHistory() - I did add an error return to Console.clearHistory

holiman · 2017-12-07T08:20:33Z

 	return nil
 }

+func (c *Console) clearHistory() {


I'd suggest to let this return an error

holiman · 2017-12-07T08:21:07Z

+func (c *Console) clearHistory() {
+	c.history = nil
+	c.prompter.ClearHistory()
+	os.Remove(c.histPath)


... and do return os.Remove(c.histPath)

holiman · 2017-12-07T08:22:31Z

+	c.history = nil
+	c.prompter.ClearHistory()
+	os.Remove(c.histPath)
+	fmt.Fprintf(c.printer, "history cleared\n")


Not sure if this is needed.. @fjl ?

sorin · 2017-12-07T20:20:37Z

@holiman @fjl take another look please, made Console.clearHistory return an error and removed the text out confirmation. Another option would be to have clearHistory return bool, err to keep it consistent with https://github.com/sorin/go-ethereum/blob/sorin-console-clear-history/node/api.go#L63-L76

holiman · 2017-12-08T08:02:58Z

Hm.. I was just about to press 'approve' on this, but then I decided to test it, and found two snags...

When running geth as I do, geth --config prod.config, and have the datadir set to something non-standard, e.g./media/datadisk/eth_datadir/.. And later, when I do a geth attach /media/datadisk/eth_datadir/geth.ipc, the history is saved as ~/.ethereum/history, since geth attach instance is not using the same datadir, just the same ipc.

This is a problem with the current code, not a new problem in this PR. To fix this, we really should have the ability to run geth --config prod.config attach , and have the second instance use the same datadir.

Another problem is if you call the clearHistory twice, or the file simply doesn't exist :

undefined
> 
> 
> admin.clearHistory()
panic: Object.getOwnPropertyNames returned unexpected type <nil>

goroutine 13 [running]:
github.com/ethereum/go-ethereum/internal/jsre.iterOwnKeys(0xc4202a7c60, 0xc420e503e0, 0xc420e95578)
	/home/martin/go/src/github.com/ethereum/go-ethereum/build/_workspace/src/github.com/ethereum/go-ethereum/internal/jsre/pretty.go:239 +0x372
github.com/ethereum/go-ethereum/internal/jsre.iterOwnAndConstructorKeys(0xc4202a7c60, 0xc420e503e0, 0xc420e956f0)
	/home/martin/go/src/github.com/ethereum/go-ethereum/build/_workspace/src/github.com/ethereum/go-ethereum/internal/jsre/pretty.go:212 +0xfc
github.com/ethereum/go-ethereum/internal/jsre.ppctx.fields(0xc4202a7c60, 0x1851320, 0xc42000e018, 0xc420e503e0, 0x0, 0xa7b8bd, 0xc42026b760)
	/home/martin/go/src/github.com/ethereum/go-ethereum/build/_workspace/src/github.com/ethereum/go-ethereum/internal/jsre/pretty.go:204 +0x190
github.com/ethereum/go-ethereum/internal/jsre.ppctx.printObject(0xc4202a7c60, 0x1851320, 0xc42000e018, 0xc420e503e0, 0x0, 0xc420e95b00)
	/home/martin/go/src/github.com/ethereum/go-ethereum/build/_workspace/src/github.com/ethereum/go-ethereum/internal/jsre/pretty.go:141 +0x992
github.com/ethereum/go-ethereum/internal/jsre.ppctx.printValue(0xc4202a7c60, 0x1851320, 0xc42000e018, 0x5, 0xf266c0, 0xc420e263c0, 0x0, 0x0)
	/home/martin/go/src/github.com/ethereum/go-ethereum/build/_workspace/src/github.com/ethereum/go-ethereum/internal/jsre/pretty.go:88 +0x8d5
github.com/ethereum/go-ethereum/internal/jsre.prettyPrint(0xc4202a7c60, 0x5, 0xf266c0, 0xc420e263c0, 0x1851320, 0xc42000e018)
	/home/martin/go/src/github.com/ethereum/go-ethereum/build/_workspace/src/github.com/ethereum/go-ethereum/internal/jsre/pretty.go:56 +0x6b
github.com/ethereum/go-ethereum/internal/jsre.(*JSRE).Evaluate.func1(0xc4202a7c60)
	/home/martin/go/src/github.com/ethereum/go-ethereum/build/_workspace/src/github.com/ethereum/go-ethereum/internal/jsre/jsre.go:316 +0x154
github.com/ethereum/go-ethereum/internal/jsre.(*JSRE).runEventLoop(0xc42029f6c0)
	/home/martin/go/src/github.com/ethereum/go-ethereum/build/_workspace/src/github.com/ethereum/go-ethereum/internal/jsre/jsre.go:207 +0x69d
created by github.com/ethereum/go-ethereum/internal/jsre.New
	/home/martin/go/src/github.com/ethereum/go-ethereum/build/_workspace/src/github.com/ethereum/go-ethereum/internal/jsre/jsre.go:78 +0x145

fjl · 2017-12-08T11:00:25Z

clearHistory is called from JS and cannot return error. It could return a JS object, but that's not really what you want. The fix I've pushed now prints an error message if there is a problem deleting the file. I will merge this as soon as CI turns green.

sorin mentioned this pull request Dec 6, 2017

how to clear history from geth javascript console? #2288

Closed

fjl reviewed Dec 6, 2017

View reviewed changes

console: admin.clearHistory() command

271a978

sorin force-pushed the sorin-console-clear-history branch from 0650063 to 271a978 Compare December 6, 2017 15:26

fjl approved these changes Dec 6, 2017

View reviewed changes

sorin added 2 commits December 6, 2017 15:58

console test: add missing ClearHistory func

dfb97ec

Merge branch 'master' of https://github.com/ethereum/go-ethereum into…

b1534d1

… sorin-console-clear-history

holiman suggested changes Dec 7, 2017

View reviewed changes

console: clearHistory() now returns an error

c79f7a4

console: fixup clearHistory

1225817

fjl merged commit 586198c into ethereum:master Dec 8, 2017

karalabe added this to the 1.8.0 milestone Dec 14, 2017

5chdn mentioned this pull request Jan 30, 2018

Merge go-ethereum updates into 2.5.4 Musicoin/go-musicoin#77

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

console: admin.clearHistory() command#15614

console: admin.clearHistory() command#15614
fjl merged 5 commits into
ethereum:masterfrom
sorin:sorin-console-clear-history

sorin commented Dec 6, 2017

Uh oh!

holiman commented Dec 6, 2017

Uh oh!

fjl Dec 6, 2017

Uh oh!

sorin Dec 6, 2017

Uh oh!

sorin commented Dec 7, 2017

Uh oh!

holiman Dec 7, 2017

Uh oh!

sorin Dec 7, 2017

Uh oh!

holiman Dec 7, 2017

Uh oh!

holiman Dec 7, 2017

Uh oh!

holiman Dec 7, 2017

Uh oh!

sorin commented Dec 7, 2017

Uh oh!

holiman commented Dec 8, 2017 •

edited

Loading

Uh oh!

fjl commented Dec 8, 2017 •

edited

Loading

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

sorin commented Dec 6, 2017

Uh oh!

holiman commented Dec 6, 2017

Uh oh!

fjl Dec 6, 2017

Choose a reason for hiding this comment

Uh oh!

sorin Dec 6, 2017

Choose a reason for hiding this comment

Uh oh!

sorin commented Dec 7, 2017

Uh oh!

holiman Dec 7, 2017

Choose a reason for hiding this comment

Uh oh!

sorin Dec 7, 2017

Choose a reason for hiding this comment

Uh oh!

holiman Dec 7, 2017

Choose a reason for hiding this comment

Uh oh!

holiman Dec 7, 2017

Choose a reason for hiding this comment

Uh oh!

holiman Dec 7, 2017

Choose a reason for hiding this comment

Uh oh!

sorin commented Dec 7, 2017

Uh oh!

holiman commented Dec 8, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fjl commented Dec 8, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

holiman commented Dec 8, 2017 •

edited

Loading

fjl commented Dec 8, 2017 •

edited

Loading