Re: [PATCH 5/5] config: add default aliases

[Felipe Contreras <felipe.contreras@gmail.com>]

Ævar Arnfjörð Bjarmason wrote:
> 
> On Fri, Jul 02 2021, Felipe Contreras wrote:
> 
> > These are all the aliases everyone agrees are essential.
> >
> > Virtually all VCS in the world have aliases, except git, so let's change
> > that.
> >
> > Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
> > ---
> >  Documentation/git-branch.txt      |  4 ++++
> >  Documentation/git-cherry-pick.txt |  4 ++++
> >  Documentation/git-commit.txt      |  4 ++++
> >  Documentation/git-mergetool.txt   |  4 ++++
> >  Documentation/git-rebase.txt      |  4 ++++
> >  Documentation/git-status.txt      |  4 ++++
> >  config.c                          | 29 +++++++++++++++++++++++++++++
> >  config.h                          |  3 ++-
> >  t/test-lib.sh                     |  2 ++
> >  9 files changed, 57 insertions(+), 1 deletion(-)
> >
> > diff --git a/Documentation/git-branch.txt b/Documentation/git-branch.txt
> > index 94dc9a54f2..fbf5ebd27a 100644
> > --- a/Documentation/git-branch.txt
> > +++ b/Documentation/git-branch.txt
> > @@ -24,6 +24,10 @@ SYNOPSIS
> >  'git branch' (-d | -D) [-r] <branchname>...
> >  'git branch' --edit-description [<branchname>]
> >  
> > +ALIAS
> > +~~~~~
> > +'git br'
> 
> I think for these it would be good to explicitly mention the mnemonic, e.g.:
> 
> 'git br', git 'br'anch. It's pretty obvious in this case, but not all of
> them.

If we are on `man git-branch(1)`, `git help branch`, or
`git branch --help` I think it's pretty obvious what the alias is for.

Especially since it's right after the synopsis.

FTR all other SCM's specify the alias directly. Perhaps we could even do
'br' instead of 'git br'.

> This also addresses the '"ci" or "co"' discussion downthread
> somewhat, i.e. at least we'll see if we always pick the first two
> letters, or if it's somewhat arbitrary.

How? What would be the mnemonic for 'ci'?

> > +~~~~~
> > +'git pi'
> 
> I've got this this as 'git chrp' locally FWIW, I'd think this would make
> more sense if it was called 'git pick'.

Yeah, but we are aiming for two letters the only other good option is
'cp' which can be easily confused.

For a past discussion on this alias see [1].

> > +~~~~~
> > +'git co'
> 
> Not going to wade into the downhtread co/ci discussion, except to say
> that this is 'co'mmit, i.e. first two letters, like 'br'anch.

Yeap, so it's straightforward.

> >  'git mergetool' [--tool=<tool>] [-y | --[no-]prompt] [<file>...]
> >  
> > +ALIAS
> > +~~~~~
> > +'git mt'
> 
> Maybe it's just me, but I don't think I've ever used git-mergetool
> directly. I don't think it's worthy of squatting on such a short name.

Huh? How is a user supposed to jump from a merge failing to mergetool?
(or rebase, or cherr-pick)

> > +ALIAS
> > +~~~~~
> > +'git rb'
> 
> So 'r'e'b'ase, not 're'base.

I don't know if 're' makes more sense here.

> >  'git status' [<options>...] [--] [<pathspec>...]
> >  
> > +ALIAS
> > +~~~~~
> > +'git st'
> 
> FWIW I've got this aliased to 'git status --short', anyway, 'st'atus, so
> first two letters...

Me too. Actually --short --branch.

> > +static int git_config_default(config_fn_t fn, void *data)
> > +{
> > +	int ret = 0;
> > +	struct config_source source;
> > +
> > +	if (getenv("GIT_NO_DEFAULT_ALIASES"))
> > +		return 0;
> 
> Can't we just include this under GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS?
> Maybe rename it to GIT_TEST_DISALLOW_ABBREVIATED now that the "OPTIONS"
> part is considered inaccurate.

Fine by me.

> > +	memset(&source, 0, sizeof(source));
> > +	source.prev = cf;
> > +	source.origin_type = CONFIG_ORIGIN_DEFAULT;
> > +	cf = &source;
> > +
> > +	ret += fn("alias.co", "commit", data);
> > +	ret += fn("alias.rb", "rebase", data);
> > +	ret += fn("alias.st", "status", data);
> > +	ret += fn("alias.br", "branch", data);
> > +	ret += fn("alias.pi", "cherry-pick", data);
> > +	ret += fn("alias.mt", "mergetool", data);
> 
> I haven't looked but does this also inject things into the configset
> API, or is it just going to be used by things that do
> git_config_mycommand and fall back on git_config_default?

I'm not sure what you mean. But it's basically as if you have them in
a config file.

Initially I used a diffent approach but the bash completion did not pick
them up. This is as close to a config file as possible.

> >  static int config_parse_pair(const char *key, const char *value,
> >  			  config_fn_t fn, void *data)
> >  {
> > @@ -1897,6 +1921,9 @@ static int do_git_config_sequence(const struct config_options *opts,
> >  		repo_config = NULL;
> >  
> >  	current_parsing_scope = CONFIG_SCOPE_SYSTEM;
> > +
> > +	git_config_default(fn, data);
> > +
> >  	if (git_config_system() && system_config &&
> >  	    !access_or_die(system_config, R_OK,
> >  			   opts->system_gently ? ACCESS_EACCES_OK : 0))
> > @@ -3497,6 +3524,8 @@ const char *current_config_origin_type(void)
> >  		return "submodule-blob";
> >  	case CONFIG_ORIGIN_CMDLINE:
> >  		return "command line";
> > +	case CONFIG_ORIGIN_DEFAULT:
> > +		return "default";
> >  	default:
> >  		BUG("unknown config origin type");
> >  	}
> 
> Ah, this is likely it, do we incclude this in 'git config -l' etc? 

Yes. Just like all other configurations.

> > diff --git a/config.h b/config.h
> > index 9038538ffd..bc3ecca313 100644
> > --- a/config.h
> > +++ b/config.h
> > @@ -58,7 +58,8 @@ enum config_origin_type {
> >  	CONFIG_ORIGIN_FILE,
> >  	CONFIG_ORIGIN_STDIN,
> >  	CONFIG_ORIGIN_SUBMODULE_BLOB,
> > -	CONFIG_ORIGIN_CMDLINE
> > +	CONFIG_ORIGIN_CMDLINE,
> > +	CONFIG_ORIGIN_DEFAULT
> >  };
> >  
> >  enum config_event_t {
> > diff --git a/t/test-lib.sh b/t/test-lib.sh
> > index 49b80a4eb5..a15965e2f4 100644
> > --- a/t/test-lib.sh
> > +++ b/t/test-lib.sh
> > @@ -456,6 +456,8 @@ GIT_DEFAULT_HASH="${GIT_TEST_DEFAULT_HASH:-sha1}"
> >  export GIT_DEFAULT_HASH
> >  GIT_TEST_MERGE_ALGORITHM="${GIT_TEST_MERGE_ALGORITHM:-ort}"
> >  export GIT_TEST_MERGE_ALGORITHM
> > +GIT_NO_DEFAULT_ALIASES=1
> > +export GIT_NO_DEFAULT_ALIASES
> >  
> >  # Tests using GIT_TRACE typically don't want <timestamp> <file>:<line> output
> >  GIT_TRACE_BARE=1
> 
> Really needs more tests.
> 
> We had some other thread where this was discussed where I suggested that
> we implement some way to include default config. Ah, here it is:
> https://lore.kernel.org/git/87eedj74dr.fsf@evledraar.gmail.com/
> 
> It's more work for this, but I think it would really go a long way to
> addressing the concerns people are going to have about this.
> 
> I think we should not opt-in to this from day one, but have some knob to
> enable including one of those shipped-by-default alias includes. Then
> people could trivially mock svn/cvs or whatever their favorite VCS is,
> and eventually as people vote with their feed we could pick a canonical
> one.

As I mentioned there the problem is where do we put that file, and how
do we distribute it.

I think it's a cleaner approach, and we should definitely try it, but
ultimately it's not going to change the fact that these aliases should
be part of the distribution, especially if they are mentioned in the man
pages. So it would just be an alternative way of hardcoding them.

[1] https://lore.kernel.org/git/20140421204506.GD5105@thunk.org/

-- 
Felipe Contreras