gh alias set by vilmibm · Pull Request #970 · cli/cli

vilmibm · 2020-05-19T21:46:29Z

This PR adds gh alias set.

what this PR does

allows creating aliases, including ones with placeholder arguments (e.g. $1)
validates that you can't set an alias for a non-existent command
adds a new top level aliases section to our config which is created on first use of alias set

what this PR doesn't do

allow for composition of commands (see shell version of gh alias set #941 )
allow for deleting, listing, importing, nor exporting of aliases (see Basic Alias Support #936)

considerations

I did two things I'm nervous about in this PR.

modified main.go to initialize a Context, check the config, and rewrite argv if an alias is found.
disabled flag parsing for alias set. I encountered what I think is a bug where cobra would treat flags inside of single quoted strings as something it should parse (eg alias set il 'issue list --label="bug"'). Disabling flag processing for this command fixed the problem but if we ever want alias set to accept flags we'll need to revisit this.

I'm open to feedback on those things.

demo

(right click view image to make more legible)

todo todid

closes #938

mislav

I like the general approach taken here, but there are many edge cases that I feel should be addressed before shipping.

An idea: should we print information about alias expansion when DEBUG is active? This seems to me like a good use of debug mode; e.g.

$ DEBUG=1 gh pl foo
[pl foo] -> [pr list foo]
(...rest of output)

Additionally, I think that we should prevent recording and expanding an alias of the same name as a built-in CLI command. This is primarily for two reasons:

Ease of debugging user problem reports: someone could report to us an error they are getting with gh pr create and we could spend time going back and forth with that user until we find out that the user has pr configured among their aliases that expands to something unexpected.
Allowing our future commands to override people's aliases: it's possible that it would get popular that CLI users would share an alias named open within our community, but if ship a top-level command named open in a future release, we probably want our command to take precedence over the alias. Otherwise, our release notes are advertising gh open behavior that some users would not be getting because they have an active alias (and they might have forgotten about it).

cmd/gh/main.go

command/root.go

command/alias.go

mislav · 2020-05-25T11:20:09Z

command/alias.go

+	// TODO HACK: even when inside of a single-quoted string, cobra was noticing and parsing any flags
+	// used in an alias expansion string argument. Since this command needs no flags, I disabled their
+	// parsing. If we ever want to add flags to alias set we'll have to figure this out. I haven't
+	// checked if this is fixed in a new cobra.


Cobra will only ever try to parse arguments as flags if an argument begins with a -- or -. With default Cobra config, this will work:

gh alias set pc 'pr create --assignee mona'

i.e. Cobra will not try to parse flags within the pr create --assignee mona. However, the quote-free version will fail:

gh alias set pc pr create --assignee mona

This is because all "words" are now passed as distinct arguments to alias set and one of them looks like a flag. To enable this use, I think that DisableFlagParsing is a good approach, but you are right that we will have to revisit this should we ever want to enable flags specifically for alias set.

gh alias set pc 'pr create --assignee mona' is exactly what didn't work, hence my saying "even when inside of a single-quoted string" and saying it seemed like a bug.

I understand how argument passing works :)

gh alias set pc 'pr create --assignee mona' is exactly what didn't work

I only brought up my points because I have first tried to reproduce this with DisableFlagParsing: false, but was not able to.

this is my experience with DisableFlagParsing unset:

I had trouble reproducing this too. It might be that you’re trying to use the --author flag on pr list but it surprisingly doesn’t exist. I tried out these alias for issues and they worked:

ghd alias set foo ‘issue list —author=“probablycorey”’
ghd alias set foo2 ‘issue list —author=probablycorey’

the error is from alias set; not pr list. i get the same result with issue list:

I thought it was possibly something weird about my shell but I'm getting the same behavior in bash and zsh.

I noticed that your paste, corey, isn't using actual single quotes but rather the ‘ character. I tried that but it still fails for me; I wonder, however, if that character is treated different on macos?

@vilmibm I'm stumped. I cannot reproduce this when I remove the DisableFlagParsing workaround:

$ gh alias set ix 'issue list --author=vilmibm' - Adding alias for ix: issue list --author=vilmibm ✓ Added alias. $ gh alias set ix 'issue list --author="vilmibm"' - Adding alias for ix: issue list --author="vilmibm" ✓ Changed alias ix from issue list --author=vilmibm to issue list --author="vilmibm" $ gh ix Showing 11 of 11 issues in cli/cli that match your search #990 gh alias delete (aliases, enhancement) about 6 days ago ...

🤷 i have no further ideas. it is concretely broken for me. i think we should leave flag parsing disabled until we have a reason to worry about it; i'll amend my comment to mention the weird by-platform behavior.

@vilmibm I can see that you are running ghd instead of gh. Could you check in your ghd script that you are forwading the arguments with "$@" and not via $@ or $*?

command/root.go

mislav · 2020-05-25T12:40:55Z

internal/config/config_type.go

+
+func (c *fileConfig) parseAliasConfig(aliasesEntry *yaml.Node) (*AliasConfig, error) {
+	if aliasesEntry.Kind != yaml.MappingNode {
+		return nil, errors.New("expected aliases to be a map")


Could we also interpret a YAML null essentially as an empty mapping?

When experimenting with aliases, I've manually deleted alias entries in my config file, resulting in only this:

aliases:

the current alias-parsing code now aborts because aliases is not a map, but I think we should be permissive about this and interpret null as an empty map.

oops yeah, good catch. that shouldn't explode a config parsing.

probablycorey

This is great! I can't wait to use it for real!

An idea: should we print information about alias expansion when DEBUG is active? This seems to me like a good use of debug mode; e.g.

I really like this idea. I ran into a situation where I couldn’t figure out why an alias wouldn’t work and this would have helped!

One thing that surprised me: If I try to change an alias I got “alias foo already exists” I was expecting it to say something like “alias foo was ghd xyz but is now god abc

probablycorey · 2020-05-27T18:08:26Z

command/alias.go

+	// TODO HACK: even when inside of a single-quoted string, cobra was noticing and parsing any flags
+	// used in an alias expansion string argument. Since this command needs no flags, I disabled their
+	// parsing. If we ever want to add flags to alias set we'll have to figure this out. I haven't
+	// checked if this is fixed in a new cobra.


I had trouble reproducing this too. It might be that you’re trying to use the --author flag on pr list but it surprisingly doesn’t exist. I tried out these alias for issues and they worked:

ghd alias set foo ‘issue list —author=“probablycorey”’
ghd alias set foo2 ‘issue list —author=probablycorey’

probablycorey · 2020-05-27T18:25:52Z

command/alias.go

+	}
+
+	if !validCommand(expansion) {
+		return fmt.Errorf("could not create alias: %s does not correspond to a gh command", utils.Bold(expansion))


I like the "error on valid command" idea. I think @vilmibm's idea of warning on import would be an acceptable fix for the potential import problem.

probablycorey · 2020-05-27T18:26:34Z

command/alias.go

+func validCommand(expansion string) bool {
+	split, _ := shlex.Split(expansion)
+	cmd, _, err := RootCmd.Traverse(split)
+	return err == nil && cmd != RootCmd
+}


This is easier than I thought it would be!

vilmibm · 2020-05-28T04:35:06Z

I've caught up on all the feedback. The only thing that is giving me pause is I'm not pleased with my handling of the empty aliases key. I'll revisit that on Monday but it's possible that just leaving my kind of gross hack in place is enough for now.

Update: I moved that code into config_type and feel better about it.

mislav

Getting really close! I already love the functionality in this branch; works great for me ✨

My last remaining requests are related to tightening safety around how we pass arguments around so that single arguments that contain spaces do not accidentally get intepreted as multiple arguments due to shellsplitting.

mislav · 2020-06-02T09:19:24Z

command/alias.go

+	// TODO HACK: even when inside of a single-quoted string, cobra was noticing and parsing any flags
+	// used in an alias expansion string argument. Since this command needs no flags, I disabled their
+	// parsing. If we ever want to add flags to alias set we'll have to figure this out. I haven't
+	// checked if this is fixed in a new cobra.


@vilmibm I'm stumped. I cannot reproduce this when I remove the DisableFlagParsing workaround:

$ gh alias set ix 'issue list --author=vilmibm' - Adding alias for ix: issue list --author=vilmibm ✓ Added alias. $ gh alias set ix 'issue list --author="vilmibm"' - Adding alias for ix: issue list --author="vilmibm" ✓ Changed alias ix from issue list --author=vilmibm to issue list --author="vilmibm" $ gh ix Showing 11 of 11 issues in cli/cli that match your search #990 gh alias delete (aliases, enhancement) about 6 days ago ...

internal/config/config_type.go

command/root.go

command/alias.go

vilmibm · 2020-06-02T21:07:49Z

@mislav caught up once more.

mislav

One tiny edge case; otherwise everything looks great! Thank you for the hard work

mislav · 2020-06-03T12:04:33Z

command/alias.go

+		return args[0]
+	}
+
+	return strings.Join(args, " ")


I'm finding that this is still an issue. Repro:

$ gh config set editor 'path with spaces' # works $ gh alias set c config set editor 'path with spaces' $ gh c #=> error: "accepts 2 arg(s), received 4"

this is because 'path with spaces' stopped being treated as a single argument when serialized into config:

aliases: c: config set editor path with spaces

vilmibm · 2020-06-03T19:28:39Z

nevermind, i'm tired and this is fully PEBCAC. My ghd wrapper was interfering 🤦

@mislav ~~Something very strange is happening.~~

~~When I run gh, I lose quoted arguments. The quotes are gone from the moment I print each thing in os.Args.~~

~~when I print each thing in os.Args from another Go program, quoted arguments are preserved as expected.~~

~~It seems that something is pre-processing os.Args and what you're seeing is breaking long before the alias code gets to it.~~

~~I will continue investigating.~~

DanyC97 · 2020-06-03T20:09:04Z

@vilmibm are the aliases stored in a file somewhere? would be awesome if somehow gh can read from gitconfig so folks can build their own dotfiles and at the same time keep them in same place

vilmibm · 2020-06-03T20:11:34Z

@DanyC97 aliases are stored in the config.yml file; we have no plans to read them from gitconfig but we're actively removing the auth stuff from the gh config file so that people can start committing their gh config alongside other dotfiles.

vilmibm force-pushed the alias-set branch from d6e3e3c to d5b7f05 Compare May 22, 2020 21:15

vilmibm marked this pull request as ready for review May 22, 2020 21:15

vilmibm requested review from ampinsk, billygriffin, mislav and probablycorey May 22, 2020 21:15

vilmibm changed the title ~~[WIP] gh alias set~~ gh alias set May 22, 2020

vilmibm mentioned this pull request May 22, 2020

gh alias list #991

Merged

mislav suggested changes May 25, 2020

View reviewed changes

mislav changed the base branch from master to trunk May 27, 2020 11:41

probablycorey reviewed May 27, 2020

View reviewed changes

vilmibm requested review from mislav and probablycorey May 28, 2020 04:35

mislav suggested changes Jun 2, 2020

View reviewed changes

mislav mentioned this pull request Jun 2, 2020

Write per-host config info to hosts.yml instead of config.yml #1077

Merged

vilmibm added 14 commits June 2, 2020 11:26

implement gh alias set

09fa75e

docs

7ed2e47

doc

125f338

args length check; needs test

f84707b

0 args tst

5fde671

pass rest of args through

bace327

better placeholder handling

e37b517

allow overwriting aliases

3552e72

prevent overriding gh commands

24e97a4

TODO

ad7c484

oops

db741a0

only process aliases when non-existent command passed

2627c5b

todo

9c4bf00

WIP: handle empty aliases key

7b4c0c5

vilmibm added 10 commits June 2, 2020 11:26

partial cleanup

37761c9

test for empty aliases key

3b4b244

todo

52a02a7

support DEBUG when expanding aliases

8773a99

odot

4804c8b

remove a gross type hack

54183f4

update note

5119626

not needed in public interface

1b844d2

w h i t e s p a c e

c960e56

do not use string concatenation for extra args

a3a6f25

vilmibm requested a review from mislav June 2, 2020 21:07

vilmibm force-pushed the alias-set branch from 669748c to a3a6f25 Compare June 2, 2020 21:14

mislav approved these changes Jun 3, 2020

View reviewed changes

vilmibm added 2 commits June 3, 2020 15:00

finally properly handle quoted arguments to alias set

6defbbf

linter appeasement

7014145

vilmibm merged commit 7fc8677 into trunk Jun 3, 2020

mislav deleted the alias-set branch June 25, 2020 14:37

Conversation

vilmibm commented May 19, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

what this PR does

what this PR doesn't do

considerations

demo

todo todid

Uh oh!

mislav left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

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!

mislav May 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

vilmibm May 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

mislav Jun 3, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

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!

probablycorey left a comment

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!

vilmibm commented May 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mislav left a comment

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!

vilmibm commented Jun 2, 2020

Uh oh!

mislav left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vilmibm commented Jun 3, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vilmibm commented May 19, 2020 •

edited

Loading

mislav left a comment •

edited

Loading

mislav May 27, 2020 •

edited

Loading

vilmibm May 27, 2020 •

edited

Loading

mislav Jun 3, 2020 •

edited

Loading

vilmibm commented May 28, 2020 •

edited

Loading

vilmibm commented Jun 3, 2020 •

edited

Loading