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

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

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. 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.

> +~~~~~
> +'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'.

> +~~~~~
> +'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.

>  '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.

> +ALIAS
> +~~~~~
> +'git rb'

So 'r'e'b'ase, not 're'base.

>  'git status' [<options>...] [--] [<pathspec>...]
>  
> +ALIAS
> +~~~~~
> +'git st'

FWIW I've got this aliased to 'git status --short', anyway, 'st'atus, so
first two letters...

> +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.

> +	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?

As long as the aliases mechanism picks it up I suppose it's fine.

>  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? 

> 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.