[term] x/term: exposing history based on proposal discussion
Gerrit Bot (Gerrit)
Gerrit Bot has uploaded the change for review![]( "Open in Gerrit")
Commit message
x/term: exposing history based on proposal discussion
https://github.com/golang/go/issues/68780#issuecomment-2737508310
Change diff
diff --git a/terminal.go b/terminal.go
index 14f8947..f7b5195 100644
--- a/terminal.go
+++ b/terminal.go
@@ -36,6 +36,16 @@
Reset: []byte{keyEscape, '[', '0', 'm'},
}
+type History interface {
+ // adds a new line into the history
+ Add(string)
+
+ // retrieves a line from history
+ // 0 should be the most recent entry
+ // ok = false when out of range
+ At(idx int) (string, bool)
+}
+
// Terminal contains the state for running a VT100 terminal that is capable of
// reading lines of input.
type Terminal struct {
@@ -86,9 +96,10 @@
remainder []byte
inBuf [256]byte
- // history contains previously entered commands so that they can be
+ // History allows to replace the default implementation of the history
+ // which contains previously entered commands so that they can be
// accessed with the up and down keys.
- history stRingBuffer
+ History History
// historyIndex stores the currently accessed history entry, where zero
// means the immediately previous entry.
historyIndex int
@@ -111,6 +122,7 @@
termHeight: 24,
echo: true,
historyIndex: -1,
+ History: &stRingBuffer{},
}
}
@@ -497,7 +509,7 @@
t.pos = len(t.line)
t.moveCursorToPos(t.pos)
case keyUp:
- entry, ok := t.history.NthPreviousEntry(t.historyIndex + 1)
+ entry, ok := t.History.At(t.historyIndex + 1)
if !ok {
return "", false
}
@@ -516,7 +528,7 @@
t.setLine(runes, len(runes))
t.historyIndex--
default:
- entry, ok := t.history.NthPreviousEntry(t.historyIndex - 1)
+ entry, ok := t.History.At(t.historyIndex - 1)
if ok {
t.historyIndex--
runes := []rune(entry)
@@ -781,7 +793,7 @@
if lineOk {
if t.echo {
t.historyIndex = -1
- t.history.Add(line)
+ t.History.Add(line)
}
if lineIsPasted {
err = ErrPasteIndicator
@@ -942,7 +954,7 @@
// If n is zero then the immediately prior value is returned, if one, then the
// next most recent, and so on. If such an element doesn't exist then ok is
// false.
-func (s *stRingBuffer) NthPreviousEntry(n int) (value string, ok bool) {
+func (s *stRingBuffer) At(n int) (value string, ok bool) {
if n < 0 || n >= s.size {
return "", false
}
Change information
- M terminal.go
Related details
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Gopher Robot (Gerrit)
Gopher Robot added 1 comment![]( "Open in Gerrit")
I spotted some possible problems.
These findings are based on simple heuristics. If a finding appears wrong, briefly reply here saying so. Otherwise, please address any problems and update the GitHub PR. When complete, mark this comment as 'Done' and click the [blue 'Reply' button](https://go.dev/wiki/GerritBot#i-left-a-reply-to-a-comment-in-gerrit-but-no-one-but-me-can-see-it) above.
Possible problems detected:
1. Are you describing the change in complete sentences with correct punctuation in the commit message body, including ending sentences with periods?
2. You usually need to reference a bug number for all but trivial or cosmetic fixes. For the term repo, the format is usually 'Fixes golang/go#12345' or 'Updates golang/go#12345' at the end of the commit message. Should you have a bug reference?
The commit title and commit message body come from the GitHub PR title and description, and must be edited in the GitHub web interface (not via git). For instructions, see [here](https://go.dev/wiki/GerritBot/#how-does-gerritbot-determine-the-final-commit-message). For guidelines on commit messages for the Go project, see [here](https://go.dev/doc/contribute#commit_messages).
(In general for Gerrit code reviews, the change author is expected to [log in to Gerrit](https://go-review.googlesource.com/login/) with a Gmail or other Google account and then close out each piece of feedback by marking it as 'Done' if implemented as suggested or otherwise reply to each review comment. See the [Review](https://go.dev/doc/contribute#review) section of the Contributing Guide for details.)
Related details
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Gerrit Bot (Gerrit)
Gerrit Bot uploaded new patchset![]( "Open in Gerrit")
Related details
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Gerrit Bot (Gerrit)
Gerrit Bot uploaded new patchset![]( "Open in Gerrit")
Related details
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Gerrit Bot (Gerrit)
Gerrit Bot uploaded new patchset![]( "Open in Gerrit")
Related details
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Laurent Demailly (Gerrit)
Laurent Demailly added 1 comment![]( "Open in Gerrit")
I spotted some possible problems.
These findings are based on simple heuristics. If a finding appears wrong, briefly reply here saying so. Otherwise, please address any problems and update the GitHub PR. When complete, mark this comment as 'Done' and click the [blue 'Reply' button](https://go.dev/wiki/GerritBot#i-left-a-reply-to-a-comment-in-gerrit-but-no-one-but-me-can-see-it) above.
Possible problems detected:
1. Are you describing the change in complete sentences with correct punctuation in the commit message body, including ending sentences with periods?
2. You usually need to reference a bug number for all but trivial or cosmetic fixes. For the term repo, the format is usually 'Fixes golang/go#12345' or 'Updates golang/go#12345' at the end of the commit message. Should you have a bug reference?The commit title and commit message body come from the GitHub PR title and description, and must be edited in the GitHub web interface (not via git). For instructions, see [here](https://go.dev/wiki/GerritBot/#how-does-gerritbot-determine-the-final-commit-message). For guidelines on commit messages for the Go project, see [here](https://go.dev/doc/contribute#commit_messages).
(In general for Gerrit code reviews, the change author is expected to [log in to Gerrit](https://go-review.googlesource.com/login/) with a Gmail or other Google account and then close out each piece of feedback by marking it as 'Done' if implemented as suggested or otherwise reply to each review comment. See the [Review](https://go.dev/doc/contribute#review) section of the Contributing Guide for details.)
Done
Related details
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Gerrit Bot (Gerrit)
Gerrit Bot uploaded new patchset![]( "Open in Gerrit")
Related details
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Gerrit Bot (Gerrit)
Gerrit Bot uploaded new patchset![]( "Open in Gerrit")
Related details
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Gerrit Bot (Gerrit)
Gerrit Bot uploaded new patchset![]( "Open in Gerrit")
Related details
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ian Lance Taylor (Gerrit)
Ian Lance Taylor voted and added 1 comment![]( "Open in Gerrit")
Related details
Code-Review
No-Holds
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Gerrit Bot (Gerrit)
Gerrit Bot uploaded new patchset![]( "Open in Gerrit")
Related details
- Ian Lance Taylor
Code-Review
No-Holds
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Gerrit Bot (Gerrit)
Gerrit Bot uploaded new patchset![]( "Open in Gerrit")
Related details
- Ian Lance Taylor
Code-Review
No-Holds
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Gerrit Bot (Gerrit)
Gerrit Bot uploaded new patchset![]( "Open in Gerrit")
Related details
- Ian Lance Taylor
Code-Review
No-Holds
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Laurent Demailly (Gerrit)
Laurent Demailly added 1 comment![]( "Open in Gerrit")
Putting on hold for proposal.
Can you un-hold it now that the proposal has been accepted? thank you!
Related details
- Ian Lance Taylor
Code-Review
No-Holds
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Sean Liao (Gerrit)
Sean Liao added 8 comments![]( "Open in Gerrit")
Implements accepted proposal https://github.com/golang/go/issues/68780#issuecomment-2810426043
(version 3)
---
Version 2 of https://github.com/golang/term/pull/15 - Much less footprint/changes in x/term in exchange for duplication and much more in callers (https://github.com/fortio/terminal/pull/85), including fairly tricky/obscure handling of adding to history automatically or not.
Fixes https://github.com/golang/go/issues/68780#issuecomment-2737508310Fixes golang/go#68780
// The History interface provides a way to replace the default automaticUse the comments as approved.
// History allows to replace the default implementation of the historyReplacing is a secondary concern, it should document what this field does,
and the default value if left unset.
t.historyAdd(line) // so this can output without deadlock.is this comment necessary?
panic(fmt.Sprintf("stRingBuffer: index [%d] out of range [0,%d)", n, s.size))let's not panic with the name of an internal type
term: history index ...
Related details
- Ian Lance Taylor
Code-Review
No-Holds
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Gerrit Bot (Gerrit)
Gerrit Bot uploaded new patchset![]( "Open in Gerrit")
Related details
- Ian Lance Taylor
Code-Review
No-Holds
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Laurent Demailly (Gerrit)
Laurent Demailly added 7 comments![]( "Open in Gerrit")
Thanks! updated (not some godoc (yet) as I think a merge between the 2 is needed to achieve both goals)
Implements accepted proposal https://github.com/golang/go/issues/68780#issuecomment-2810426043
(version 3)
---
Version 2 of https://github.com/golang/term/pull/15 - Much less footprint/changes in x/term in exchange for duplication and much more in callers (https://github.com/fortio/terminal/pull/85), including fairly tricky/obscure handling of adding to history automatically or not.
Done
Fixes https://github.com/golang/go/issues/68780#issuecomment-2737508310Laurent DemaillyFixes golang/go#68780
Done
defer t.lock.Lock()locks seem unnecessary?
it's very much necessary to _unlock_ so out can be used in History without deadlock
t.historyAdd(line) // so this can output without deadlock.Laurent Demaillyis this comment necessary?
will move it above, it's from when the unlock relock was inlined there
panic(fmt.Sprintf("stRingBuffer: index [%d] out of range [0,%d)", n, s.size))let's not panic with the name of an internal type
term: history index ...
- Ian Lance Taylor
- Sean Liao
Code-Review
No-Holds
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Gerrit Bot (Gerrit)
Gerrit Bot uploaded new patchset![]( "Open in Gerrit")
Related details
- Ian Lance Taylor
- Sean Liao
Code-Review
No-Holds
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Laurent Demailly (Gerrit)
Laurent Demailly added 1 comment![]( "Open in Gerrit")
// The History interface provides a way to replace the default automaticUse the comments as approved.
I think the approved comments are missing the "how/why do I implement this new History interface" to achieve "pluggable" side of the story and are abstract about how is History used. I welcome some suggestion to merge the 2 concerns.
(though I can also just remove the additional context if necessary)
Related details
- Ian Lance Taylor
- Sean Liao
Code-Review
No-Holds
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Gerrit Bot (Gerrit)
Gerrit Bot uploaded new patchset![]( "Open in Gerrit")
Related details
- Ian Lance Taylor
- Sean Liao
Code-Review
No-Holds
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Austin Clements (Gerrit)
Austin Clements added 6 comments![]( "Open in Gerrit")
Code LGTM, just some comments on comments.
// The History interface provides a way to replace the default automaticLaurent DemaillyUse the comments as approved.
I think the approved comments are missing the "how/why do I implement this new History interface" to achieve "pluggable" side of the story and are abstract about how is History used. I welcome some suggestion to merge the 2 concerns.
(though I can also just remove the additional context if necessary)
I think it's a good idea to add context, but let's put the summary first. How about:
// A History provides a (possibly bounded) queue of input lines read by [ReadLine].
//
// The default implementation of History provides a simple ring buffer limited
// to 100 slots. Clients can provide alternate implementations of History to
// change this behavior.
// the entry being added (e.g.,, if it's a whitespace-only entry),Extra ","
// Add will be called by Terminal to add a new line into the history.Maybe "[Terminal.ReadLine]" just to be specific?
// An implementation may decide to not add every lines by ignoring callsIsn't this last sentence redundant with the "It is allowed to drop any entry ..." sentence above?
// History allows to replace the default implementation of the historyReplacing is a secondary concern, it should document what this field does,
and the default value if left unset.
In the proposal, the doc comment is:
// History records and retrieves lines of input read by [ReadLine].
//
// It is not safe to call ReadLine concurrently with any methods on History.
//
// [NewTerminal] sets this to an implementation that records the last 100 lines of input.
It certainly doesn't have to be this word for word, but I think it captures a few important things: it says what the field does, as Sean pointed out; and it says what the default value is.
Related details
- Ian Lance Taylor
- Sean Liao
Code-Review
No-Holds
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Austin Clements (Gerrit)
Austin Clements removed a vote from this change![]( "Open in Gerrit")
Removed Hold+1 by Ian Lance Taylor <ia...@golang.org>
Related details
- Ian Lance Taylor
- Sean Liao
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Austin Clements (Gerrit)
Austin Clements added 1 comment![]( "Open in Gerrit")
Laurent DemaillyPutting on hold for proposal.
Can you un-hold it now that the proposal has been accepted? thank you!
- Ian Lance Taylor
- Laurent Demailly
- Sean Liao
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Laurent Demailly (Gerrit)
Laurent Demailly added 6 comments![]( "Open in Gerrit")
Thanks for the thoughtful comments - ptal
// The History interface provides a way to replace the default automaticLaurent DemaillyUse the comments as approved.
Austin ClementsI think the approved comments are missing the "how/why do I implement this new History interface" to achieve "pluggable" side of the story and are abstract about how is History used. I welcome some suggestion to merge the 2 concerns.
(though I can also just remove the additional context if necessary)
I think it's a good idea to add context, but let's put the summary first. How about:
// A History provides a (possibly bounded) queue of input lines read by [ReadLine].
//
// The default implementation of History provides a simple ring buffer limited
// to 100 slots. Clients can provide alternate implementations of History to
// change this behavior.
Done
// the entry being added (e.g.,, if it's a whitespace-only entry),Laurent DemaillyExtra ","
Done
// Add will be called by Terminal to add a new line into the history.Maybe "[Terminal.ReadLine]" just to be specific?
Done
// An implementation may decide to not add every lines by ignoring callsIsn't this last sentence redundant with the "It is allowed to drop any entry ..." sentence above?
Thanks for the comments. I removed it and merged the 2. lmk (after you see the next rev)
// History allows to replace the default implementation of the historyAustin ClementsReplacing is a secondary concern, it should document what this field does,
and the default value if left unset.
In the proposal, the doc comment is:
// History records and retrieves lines of input read by [ReadLine].
//
// It is not safe to call ReadLine concurrently with any methods on History.
//
// [NewTerminal] sets this to an implementation that records the last 100 lines of input.It certainly doesn't have to be this word for word, but I think it captures a few important things: it says what the field does, as Sean pointed out; and it says what the default value is.
yes I missed that part across multiple rev/copy paste/changes. fixing (there was the original text with mention of up/down keys, do we drop that? [it was on non exposed field so more a dev comment])
ps: seems like [links] don't work in struct comments
Related details
- Austin Clements
- Ian Lance Taylor
- Sean Liao
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Laurent Demailly (Gerrit)
Laurent Demailly added 1 comment![]( "Open in Gerrit")
// History allows to replace the default implementation of the historyAustin ClementsReplacing is a secondary concern, it should document what this field does,
and the default value if left unset.
Laurent DemaillyIn the proposal, the doc comment is:
// History records and retrieves lines of input read by [ReadLine].
//
// It is not safe to call ReadLine concurrently with any methods on History.
//
// [NewTerminal] sets this to an implementation that records the last 100 lines of input.It certainly doesn't have to be this word for word, but I think it captures a few important things: it says what the field does, as Sean pointed out; and it says what the default value is.
yes I missed that part across multiple rev/copy paste/changes. fixing (there was the original text with mention of up/down keys, do we drop that? [it was on non exposed field so more a dev comment])
ps: seems like [links] don't work in struct comments
Done
Gerrit Bot (Gerrit)
Gerrit Bot uploaded new patchset![]( "Open in Gerrit")
Related details
- Austin Clements
- Ian Lance Taylor
- Sean Liao
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Laurent Demailly (Gerrit)
Laurent Demailly added 3 comments![]( "Open in Gerrit")
ps: I don't know the protocol, is the original person supposed to be the one marking resolved after reviewing or... I mark it resolved when I think I addressed it and one reopens if there is an issue?
// An implementation may decide to not add every lines by ignoring callsLaurent DemaillyIsn't this last sentence redundant with the "It is allowed to drop any entry ..." sentence above?
Thanks for the comments. I removed it and merged the 2. lmk (after you see the next rev)
Done
defer t.lock.Lock()Laurent Demaillylocks seem unnecessary?
it's very much necessary to _unlock_ so out can be used in History without deadlock
Done
Related details
- Austin Clements
- Ian Lance Taylor
- Sean Liao
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Gerrit Bot (Gerrit)
Gerrit Bot uploaded new patchset![]( "Open in Gerrit")
Related details
- Austin Clements
- Ian Lance Taylor
- Sean Liao
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Laurent Demailly (Gerrit)
Laurent Demailly added 1 comment![]( "Open in Gerrit")
// Add adds will be called by [Terminal.ReadLine] to addtypo fixed in patchset 15 but gerrit lags a bit behind github
Related details
- Austin Clements
- Ian Lance Taylor
- Sean Liao
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Laurent Demailly (Gerrit)
Laurent Demailly added 1 comment![]( "Open in Gerrit")
// Add adds will be called by [Terminal.ReadLine] to addtypo fixed in patchset 15 but gerrit lags a bit behind github
Done
Related details
- Austin Clements
- Ian Lance Taylor
- Sean Liao
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Austin Clements (Gerrit)
Austin Clements voted and added 2 comments![]( "Open in Gerrit")
Votes added by Austin Clements
| Code-Review | +2 |
| Commit-Queue | +1 |
ps: I don't know the protocol, is the original person supposed to be the one marking resolved after reviewing or... I mark it resolved when I think I addressed it and one reopens if there is an issue?
Yeah, you can go ahead and mark things resolved as you address them (unless you have a follow-up, of course).
// History allows to replace the default implementation of the historyAustin ClementsReplacing is a secondary concern, it should document what this field does,
and the default value if left unset.
Laurent DemaillyIn the proposal, the doc comment is:
// History records and retrieves lines of input read by [ReadLine].
//
// It is not safe to call ReadLine concurrently with any methods on History.
//
// [NewTerminal] sets this to an implementation that records the last 100 lines of input.It certainly doesn't have to be this word for word, but I think it captures a few important things: it says what the field does, as Sean pointed out; and it says what the default value is.
Laurent Demaillyyes I missed that part across multiple rev/copy paste/changes. fixing (there was the original text with mention of up/down keys, do we drop that? [it was on non exposed field so more a dev comment])
ps: seems like [links] don't work in struct comments
Done
ps: seems like [links] don't work in struct comments
This is go.dev/issue/56004. Given how often we do this in the Go tree itself, I think this really is a bug and not working-as-intended.
Related details
- Ian Lance Taylor
- Laurent Demailly
- Sean Liao
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Gerrit Bot (Gerrit)
Gerrit Bot uploaded new patchset![]( "Open in Gerrit")
- Austin Clements
- Ian Lance Taylor
- Laurent Demailly
- Sean Liao
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Gerrit Bot (Gerrit)
Gerrit Bot uploaded new patchset![]( "Open in Gerrit")
- Austin Clements
- Ian Lance Taylor
- Sean Liao
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Michael Pratt (Gerrit)
Michael Pratt voted Code-Review+1![]( "Open in Gerrit")
| Code-Review | +1 |
Related details
- Austin Clements
- Ian Lance Taylor
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Jorropo (Gerrit)
Jorropo added 2 comments![]( "Open in Gerrit")
// The default implementation of History provides a simple ring buffer limitedWhat default implementation ? Who uses this ?
This paragraph would better fit next to the struct field:
```
History History
```
t.lock.Unlock() // Unlock to avoid deadlock if History methods use the output writer.It looks like calling `(*Terminal).ReadLines` concurrently used to be safe, while now this will cause very interesting bugs.
Is that ok ? I guess few pieces of code do this to begin with.
If breaking this is not ok we can keep this behavior while allowing reentrency on the output write by using two mutexes.
If breaking this is ok then please add something similar to `t.someUselessField = someUselessValue` before the critical sections of methods transitively calling `historyAt` and `historyAdd`, this will make them trip the race detector if called concurrently.
Related details
- Austin Clements
- Ian Lance Taylor
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Jorropo (Gerrit)
Jorropo added 1 comment![]( "Open in Gerrit")
t.lock.Unlock() // Unlock to avoid deadlock if History methods use the output writer.It looks like calling `(*Terminal).ReadLines` concurrently used to be safe, while now this will cause very interesting bugs.
Is that ok ? I guess few pieces of code do this to begin with.
If breaking this is not ok we can keep this behavior while allowing reentrency on the output write by using two mutexes.
If breaking this is ok then please add something similar to `t.someUselessField = someUselessValue` before the critical sections of methods transitively calling `historyAt` and `historyAdd`, this will make them trip the race detector if called concurrently.
Actually for the breaking this is ok case, similar to what I've said above you could add an unprotected read in front of all `t.lock.Lock()` and an unprotected write in after all `t.lock.Unlock()` in `historyAt` and `historyAdd` but this will avoid some false trips if `historyAt` and `historyAdd` are not ran.
Jorropo (Gerrit)
Jorropo added 1 comment![]( "Open in Gerrit")
t.lock.Unlock() // Unlock to avoid deadlock if History methods use the output writer.JorropoIt looks like calling `(*Terminal).ReadLines` concurrently used to be safe, while now this will cause very interesting bugs.
Is that ok ? I guess few pieces of code do this to begin with.
If breaking this is not ok we can keep this behavior while allowing reentrency on the output write by using two mutexes.
If breaking this is ok then please add something similar to `t.someUselessField = someUselessValue` before the critical sections of methods transitively calling `historyAt` and `historyAdd`, this will make them trip the race detector if called concurrently.
Actually for the breaking this is ok case, similar to what I've said above you could add an unprotected read in front of all `t.lock.Lock()` and an unprotected write in after all `t.lock.Unlock()` in `historyAt` and `historyAdd` but this will avoid some false trips if `historyAt` and `historyAdd` are not ran.
*unprotected read in front of `t.lock.Lock()` in methods transitively calling `historyAt` & `historyAdd`, not all of them
Laurent Demailly (Gerrit)
Laurent Demailly added 3 comments![]( "Open in Gerrit")
Thanks a lot for your time and comments YC
// The default implementation of History provides a simple ring buffer limitedWhat default implementation ? Who uses this ?
This paragraph would better fit next to the struct field:
```
History History
```
given in the godoc History shows up before Terminal it doesn't hurt to (repeat) what it is for?
t.lock.Unlock() // Unlock to avoid deadlock if History methods use the output writer.JorropoIt looks like calling `(*Terminal).ReadLines` concurrently used to be safe, while now this will cause very interesting bugs.
Is that ok ? I guess few pieces of code do this to begin with.
If breaking this is not ok we can keep this behavior while allowing reentrency on the output write by using two mutexes.
If breaking this is ok then please add something similar to `t.someUselessField = someUselessValue` before the critical sections of methods transitively calling `historyAt` and `historyAdd`, this will make them trip the race detector if called concurrently.
JorropoActually for the breaking this is ok case, similar to what I've said above you could add an unprotected read in front of all `t.lock.Lock()` and an unprotected write in after all `t.lock.Unlock()` in `historyAt` and `historyAdd` but this will avoid some false trips if `historyAt` and `historyAdd` are not ran.
*unprotected read in front of `t.lock.Lock()` in methods transitively calling `historyAt` & `historyAdd`, not all of them
You're right and as mentioned during the proposal lengthy discussion an interface callback in the middle for what was properly locked in all cases before opens up potential issues. It was deemed that documenting "don't do this" was enough but let me see if we can do better that that (in addition to preserving the ability to call concurrent Write which is my understanding of the main feature of Terminal)
Laurent Demailly (Gerrit)
Laurent Demailly added 2 comments![]( "Open in Gerrit")
t.lock.Unlock() // Unlock to avoid deadlock if History methods use the output writer.JorropoIt looks like calling `(*Terminal).ReadLines` concurrently used to be safe, while now this will cause very interesting bugs.
Is that ok ? I guess few pieces of code do this to begin with.
If breaking this is not ok we can keep this behavior while allowing reentrency on the output write by using two mutexes.
If breaking this is ok then please add something similar to `t.someUselessField = someUselessValue` before the critical sections of methods transitively calling `historyAt` and `historyAdd`, this will make them trip the race detector if called concurrently.
JorropoActually for the breaking this is ok case, similar to what I've said above you could add an unprotected read in front of all `t.lock.Lock()` and an unprotected write in after all `t.lock.Unlock()` in `historyAt` and `historyAdd` but this will avoid some false trips if `historyAt` and `historyAdd` are not ran.
Laurent Demailly*unprotected read in front of `t.lock.Lock()` in methods transitively calling `historyAt` & `historyAdd`, not all of them
You're right and as mentioned during the proposal lengthy discussion an interface callback in the middle for what was properly locked in all cases before opens up potential issues. It was deemed that documenting "don't do this" was enough but let me see if we can do better that that (in addition to preserving the ability to call concurrent Write which is my understanding of the main feature of Terminal)
Looking again at the code I found what I had noticed but forgot in the months since; unlocking mid way inside ReadLine is not new:
```go
t.lock.Unlock()
n, err = t.c.Read(readBuf)
t.lock.Lock()
```
(putting a comment on that line too)
Jorropo (Gerrit)
Jorropo added 3 comments![]( "Open in Gerrit")
// The default implementation of History provides a simple ring buffer limitedLaurent DemaillyWhat default implementation ? Who uses this ?
This paragraph would better fit next to the struct field:
```
History History
```
given in the godoc History shows up before Terminal it doesn't hurt to (repeat) what it is for?
See https://pkg.go.dev/net/http#RoundTripper for example, it has no default implementation paragraph.
Instead https://pkg.go.dev/net/http#Client.Transport mentions the default impl.
t.lock.Unlock() // Unlock to avoid deadlock if History methods use the output writer.JorropoIt looks like calling `(*Terminal).ReadLines` concurrently used to be safe, while now this will cause very interesting bugs.
Is that ok ? I guess few pieces of code do this to begin with.
If breaking this is not ok we can keep this behavior while allowing reentrency on the output write by using two mutexes.
If breaking this is ok then please add something similar to `t.someUselessField = someUselessValue` before the critical sections of methods transitively calling `historyAt` and `historyAdd`, this will make them trip the race detector if called concurrently.
JorropoActually for the breaking this is ok case, similar to what I've said above you could add an unprotected read in front of all `t.lock.Lock()` and an unprotected write in after all `t.lock.Unlock()` in `historyAt` and `historyAdd` but this will avoid some false trips if `historyAt` and `historyAdd` are not ran.
Laurent Demailly*unprotected read in front of `t.lock.Lock()` in methods transitively calling `historyAt` & `historyAdd`, not all of them
Laurent DemaillyYou're right and as mentioned during the proposal lengthy discussion an interface callback in the middle for what was properly locked in all cases before opens up potential issues. It was deemed that documenting "don't do this" was enough but let me see if we can do better that that (in addition to preserving the ability to call concurrent Write which is my understanding of the main feature of Terminal)
Looking again at the code I found what I had noticed but forgot in the months since; unlocking mid way inside ReadLine is not new:
```go
t.lock.Unlock()
n, err = t.c.Read(readBuf)
t.lock.Lock()
```
(putting a comment on that line too)
Acknowledged
t.lock.Unlock()- Austin Clements
- Ian Lance Taylor
- Laurent Demailly
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Laurent Demailly (Gerrit)
Laurent Demailly added 2 comments![]( "Open in Gerrit")
Removed the default impl. details from History interface.
// The default implementation of History provides a simple ring buffer limitedLaurent DemaillyWhat default implementation ? Who uses this ?
This paragraph would better fit next to the struct field:
```
History History
```
Jorropogiven in the godoc History shows up before Terminal it doesn't hurt to (repeat) what it is for?
See https://pkg.go.dev/net/http#RoundTripper for example, it has no default implementation paragraph.
Instead https://pkg.go.dev/net/http#Client.Transport mentions the default impl.
- Austin Clements
- Ian Lance Taylor
- Jorropo
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Gerrit Bot (Gerrit)
Gerrit Bot uploaded new patchset![]( "Open in Gerrit")
- Code-Review: +1 by Michael Pratt, +2 by Austin Clements
- TryBots-Pass: LUCI-TryBot-Result+1 by Go LUCI
Related details
- Austin Clements
- Ian Lance Taylor
- Jorropo
- Michael Pratt
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Jorropo (Gerrit)
Jorropo voted Commit-Queue+1![]( "Open in Gerrit")
- Austin Clements
- Ian Lance Taylor
- Michael Pratt
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Jorropo (Gerrit)
Jorropo voted![]( "Open in Gerrit")
| Auto-Submit | +1 |
| Code-Review | +2 |
| Commit-Queue | +1 |
Related details
- Austin Clements
- Ian Lance Taylor
- Michael Pratt
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Jorropo (Gerrit)
Jorropo voted Code-Review+1![]( "Open in Gerrit")
| Code-Review | +1 |
Related details
- Austin Clements
- Ian Lance Taylor
- Michael Pratt
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Austin Clements (Gerrit)
Austin Clements voted Code-Review+2![]( "Open in Gerrit")
- Ian Lance Taylor
- Michael Pratt
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Michael Pratt (Gerrit)
Michael Pratt voted Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
Related details
- Ian Lance Taylor
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Michael Pratt (Gerrit)
Michael Pratt voted Code-Review+1![]( "Open in Gerrit")
| Code-Review | +1 |
Related details
- Ian Lance Taylor
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Gopher Robot (Gerrit)
Gopher Robot submitted the change![]( "Open in Gerrit")
Change information
term: support pluggable history
Expose a new History interface that allows replacement of the default
ring buffer to customize what gets added or not; as well as to allow
saving/restoring history on either the default ringbuffer or a custom
replacement.
Fixes golang/go#68780
- M terminal.go
Code-Review: +1 by Michael Pratt, +2 by Austin Clements, +1 by Jorropo
TryBots-Pass: LUCI-TryBot-Result+1 by Go LUCI
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |