Fix for help when $HOME is / #10625

duglin · 2015-02-06T22:46:27Z

@estesp noticed that when $HOME is / the ~ substitutions messes up
because it tries to replace all paths that start with "/" with "~".
This fixes it so that it will only replace it when $HOME isn't "/".

This is a follow-on/fix to: #10547

Signed-off-by: Doug Davis dug@us.ibm.com

tianon · 2015-02-06T22:49:05Z

tianon commented

Feb 6, 2015

Is it really such a good idea for us to be substituting ~ in these help paths? ~ is not expanded by Go, but by the shell, so it might not always work depending on how a user is plugging it in after copy/pasting it from the help text to change "one little thing" about it, for example.

jessfraz · 2015-02-06T22:49:55Z

this should use the new homedir package? no?

phemmer · 2015-02-06T23:01:15Z

@tianon I had that same thought when I made the comment here, but didn't mention at the time.
I think the ideal solution is if these fields weren't quoted. Then the copy/paste issue would be solved. But I didn't think the effort to handle removing quotes on these few fields was worth it.

tianon · 2015-02-06T23:08:26Z

Do users not usually recognize their own homedir? I know any time I see /home/tianon, I recognize it for what it is... 😄

phemmer · 2015-02-06T23:10:23Z

@tianon see the linked comment. The issue is the width required to show the home directory.

duglin · 2015-02-06T23:49:32Z

If people think this was being "too smart for our own good" I'm ok with removing it. But as @phemmer said, this was to help make sure we fit within 80 chars.

@jfrazelle I switched it to use the new pkg just in case we don't rip it out.

tiborvass · 2015-02-07T00:46:19Z

integration-cli/docker_cli_help_test.go

 	"os/exec"
 	"runtime"
 	"strings"
 	"testing"
 	"unicode"
+
+	"github.com/docker/docker/pkg/homedir"
 )

 func TestMainHelpWidth(t *testing.T) {


@duglin test passes on master is that normal? Even with HOME=/

hmmm, it should fail on master because the check looks for "/" and should Fatal if it sees it.
When you do docker help what do you see?
I get:
-p, --pidfile="~var/run/docker.pid" Path to use for daemon PID file

@duglin yeah i get the same thing on master (and this PR fixes it), however this test still passes on master.

@tiborvass I'm a little bit confused on this one. If, when you run the tests, $HOME is / then https://github.com/docker/docker/blob/master/integration-cli/docker_cli_help_test.go#L84 should cause your test to fail because at some point it should find a / in your help text. The fact that it doesn't must mean that $HOME isn't really set to /.

However, now I realize that the Contains() in there is looking for too much. Instead of just looking for $HOME it should be looking for "$HOME (ie. $HOME after a quote). That should remove some of the potential false-negative results. However, the check for $HOME being / is still needed because we don't want /some/path to become ~some/path.

jessfraz · 2015-02-07T01:17:29Z

I see -p, --pidfile="/var/run/docker.pid" Path to use for daemon PID file

with master

duglin · 2015-02-07T01:19:07Z

is your $HOME set to / ?

jessfraz · 2015-02-07T01:19:24Z

/home/jessie

jessfraz · 2015-02-07T01:20:00Z

jessie at debian in ~/docker on master
$ echo $HOME
/home/jessie

jessie at debian in ~/docker on master
$ sudo su
root@debian:/home/jessie/docker# echo $HOME
/root
root@debian:/home/jessie/docker#

duglin · 2015-02-07T01:24:15Z

Right, "normally" it should replace all /home/jessie's or /root's (depending on which env your in) with ~ in your docker help output. Hopefully, your master is doing that. But, when you set $HOME to / then it should get too aggressive and you'll see ~ in other weird spots like ~var/...

jessfraz · 2015-02-07T01:25:20Z

oh i see what you are saying mine works fine because my home is not / LGTM

tiborvass · 2015-02-08T14:42:47Z

integration-cli/docker_cli_help_test.go

+
+		// Don't check when HOME is /
+		if home == "/" {
+			home = ""


@duglin when $HOME is / you reset it to empty string here.

Correct, when home is "" I skip the check. So, $HOME="/" or being on windows will skip it.

@duglin What I'm saying is that, that seems why the test passes on master as well, since it never satisfies the home != "" condition, even on master.

Perhaps I misunderstood, but I thought you originally were saying that this test passed for you w/o this PR (on master), which didn't check for / so the if-statement should have generated a failure when $HOME is /.

@duglin okay, now I get it, my bad. I was testing your test on top of master, to see if the coverage increased. But the test passes on master because it was a false positive. So if I only checkout the pkg/mflag change from this PR and run the old test from master, I get an error, but only if I set HOME=/.

Maybe we should temporarily set HOME to / in the test?

Ah, good point. Instead of just testing the current env's $HOME I should test / too. I merged the two tests and looped it now so that on non-windows we'll test the current $HOME and /. See what you think.

tiborvass · 2015-02-08T19:46:25Z

integration-cli/docker_cli_help_test.go

-			break
+	for _, home := range homes {
+		os.Setenv("HOME", home)
+		scanForHome := false


scanForHome := runtime.GOOS != "windows" && home != "/"

estesp · 2015-02-09T14:12:09Z

Since I forgot to comment over the weekend, just wanted to note that the merging of the tests fixes the remaining problem when $HOME == "/" for me, given you had made the change in main help, but not in command help. Now that they are one test, it resolves the bug that command help wasn't conforming "/" to "".

duglin · 2015-02-10T20:39:34Z

any more comments on this one?

jessfraz · 2015-02-10T22:36:38Z

ping @tiborvass

tiborvass · 2015-02-12T23:39:39Z

@duglin when HOME=/, you don't test for replacing / by ~ (in --pidfile), which is basically what this PR fixes, no?

duglin · 2015-02-20T00:30:21Z

@tiborvass rebased - should be good for final review now

duglin · 2015-02-20T01:49:14Z

@ahmetalpbalkan I added a homedir.Set() - hope that's ok

tiborvass · 2015-02-20T01:54:20Z

LGTM

ahmetb · 2015-02-20T01:59:43Z

@duglin. It's probably not ok. not all windows apps read it from USERPROFILE. There is also %HOMEDRIVE%%HOMEPATH%. Why do you need this?

duglin · 2015-02-20T02:02:11Z

In the testcase we set HOME to some value, I need to reset it when done. If on windows we're not setting HOME to a new value but instead setting USERPROFILE then I need to reset that value. no?

tiborvass · 2015-02-20T02:08:16Z

@ahmetalpbalkan doesn't matter, whatever he "gets" from homedir.Get, is what he's setting with homedir.Set. So nothing changes.

ahmetb · 2015-02-20T02:13:38Z

@tiborvass @duglin that's really hacky. Can you instead write a homedir.GetEnvironmentVariableName() and make homedir.Get() use it? This way you can set the specific cmd.Env key before caling the command and you don't pollute os.Getenv for the next tests.

tiborvass · 2015-02-20T02:27:13Z

@ahmetalpbalkan I don't understand. What's wrong with doing Set(key, Get(key)) ?

ahmetb · 2015-02-20T03:03:15Z

pkg/homedir/homedir.go

@@ -15,6 +15,14 @@ func Get() string {
 	return os.Getenv("HOME")
 }

+func Set(val string) {


this is duplication. exact same if block exist for Get(), consider extracting into EnvironmentKey() method.

ahmetb · 2015-02-20T03:03:47Z

@tianon I don't see the upside. What's wrong with using cmd.Env?

Honestly a homedir package with Set method doesn't make sense to me semantically.

ahmetb · 2015-02-20T05:57:44Z

pkg/homedir/homedir.go

@@ -5,14 +5,20 @@ import (
 	"runtime"
 )

+// Return the Env Var Name for the user's home dir. On Linux this is
+// "HOME" while on windows its "USERPROFILE"


this should be starting with function's name per idiomatic Go.

HomeKey returns the environment name for user's home directory
depending on platform running on.

is probably just fine. By adding more stuff you're risking comments to get out of sync with code. :)

Also I'd call this something else. instead of homedir.HomeKey(), homedir.EnvironmentKey() seems like a better fit to me.

@ahmetalpbalkan done for both but used Key() instead per our irc chat.

ahmetb · 2015-02-20T05:58:44Z

LGTM except I left some minor comments. Thanks @duglin.

estesp noticed that when $HOME is / the ~ substitutions messes up becuase it tries to replace all paths that start with "/" with "~". This fixes it so that it will only replace it when $HOME isn't "/". Signed-off-by: Doug Davis <dug@us.ibm.com>

ahmetb · 2015-02-20T07:00:30Z

LGTM.

duglin · 2015-02-20T07:04:13Z

Made some mods per @ahmetalpbalkan's suggestion.

Fix for help when $HOME is /

duglin force-pushed the FixHelpHomeSlash branch from 84ba61a to ba6b21e Compare February 6, 2015 23:49

tiborvass added this to the 1.5.1 milestone Feb 7, 2015

tiborvass reviewed Feb 7, 2015
View reviewed changes

duglin force-pushed the FixHelpHomeSlash branch 3 times, most recently from 0a7e34a to 40830fc Compare February 8, 2015 13:54

tiborvass reviewed Feb 8, 2015
View reviewed changes

duglin force-pushed the FixHelpHomeSlash branch 2 times, most recently from 7ae6e00 to dfec474 Compare February 8, 2015 19:22

tiborvass reviewed Feb 8, 2015
View reviewed changes

duglin force-pushed the FixHelpHomeSlash branch from dfec474 to 2888493 Compare February 8, 2015 21:31

jessfraz added the status/2-code-review label Feb 10, 2015

duglin force-pushed the FixHelpHomeSlash branch from 2888493 to 41ffc49 Compare February 13, 2015 01:26

duglin force-pushed the FixHelpHomeSlash branch from 41ffc49 to dc28db8 Compare February 20, 2015 00:00

duglin force-pushed the FixHelpHomeSlash branch from dc28db8 to fc3546f Compare February 20, 2015 01:47

duglin force-pushed the FixHelpHomeSlash branch from fc3546f to 7b8fe6d Compare February 20, 2015 02:09

ahmetb reviewed Feb 20, 2015
View reviewed changes

duglin force-pushed the FixHelpHomeSlash branch from 7b8fe6d to cc9c874 Compare February 20, 2015 05:47

ahmetb reviewed Feb 20, 2015
View reviewed changes

duglin force-pushed the FixHelpHomeSlash branch from cc9c874 to 3272211 Compare February 20, 2015 06:05

Fix for help when $HOME is /

116367e

estesp noticed that when $HOME is / the ~ substitutions messes up becuase it tries to replace all paths that start with "/" with "~". This fixes it so that it will only replace it when $HOME isn't "/". Signed-off-by: Doug Davis <dug@us.ibm.com>

duglin force-pushed the FixHelpHomeSlash branch from 3272211 to 116367e Compare February 20, 2015 06:21

tiborvass added a commit that referenced this pull request Feb 20, 2015

Merge pull request #10625 from duglin/FixHelpHomeSlash

fdfa7ee

Fix for help when $HOME is /

tiborvass merged commit fdfa7ee into moby:master Feb 20, 2015

dmp42 removed the status/2-code-review label Feb 20, 2015

SvenDowideit mentioned this pull request Feb 26, 2015

possible PRs to merge for a docs-update #11031

Closed

duglin deleted the FixHelpHomeSlash branch March 6, 2015 04:54

jessfraz modified the milestones: 1.5.1, 1.6.0 Mar 16, 2015

Fix for help when $HOME is / #10625

Fix for help when $HOME is / #10625

Uh oh!

Conversation

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!