Re: [PATCH] Adds configuration options for some commonly used command-line options (GSOC micro-project)

[Junio C Hamano <gitster@pobox.com>]

Amate Yolande <yolandeamate@gmail.com> writes:

> 	This is a patch for my work on one of the micro projects, as I intend
> to apply for the Google Summer of Code 2015 under the Git community.
> This patch gives the user the oppotunity to specify configuration
> options for some commonly used command-line options for exampel:
> 	git config defopt.am 'am -3'
> ---

Check Documentaiton/SubmittingPatches again?  It would be beneficial
to use the output of "git log --no-merges" for recent history to see
the recommended style of log messages while studying it.

>  Makefile |    1 +
>  defopt.c |   24 ++++++++++++++++++++++++
>  git.c    |   56 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++

Docs and tests?

> +static int handle_defopt(int *argcp, const char ***argv)
> +{	
> +	int envchanged = 0, ret = 0, saved_errno = errno;

What is the point of having a local envchanged here, receiving the
info from handle_options() only to discard?

> +	subdir = setup_git_directory_gently(&unused_nongit);
> + ...
> +	if (subdir && chdir(subdir))
> +		die_errno("Cannot change to '%s'", subdir);

Why do you need to do this chdir() here?  Wouldn't the caller of the
codepath after the callsite you added the call to this function go
to the top-level as necessary already?

> +	errno = saved_errno;
> +
> +}
> +
> +
>  static int handle_alias(int *argcp, const char ***argv)
>  {
>  	int envchanged = 0, ret = 0, saved_errno = errno;
> @@ -517,6 +570,9 @@ static void handle_builtin(int argc, const char **argv)
>  		argv[1] = argv[0];
>  		argv[0] = cmd = "help";
>  	}
> +	
> +	if(is_builtin(cmd) && argc == 1)
> +		handle_defopt(&argc, &argv);

You used "am -3" as an example, but is "am" a built-in?

Even if it were, being able to say "git am" (no arguments) and
getting that rewritten to "git am -3", only when there is no other
arguments, is not all that useful, as a typical workflow with "am"
is to save a series of patches in a mailbox file (e.g. I would save
the message I am responding to in ./+ay-defopt.mbox) and then feed
that as an argument (e.g. "git am ./+ay-defopt.mbox").

A lazier version of me (but not me who is typing this message) might
appreciate it if "git am ./+ay-defopt.mbox" gets rewritten to "git
am -3 ./+ay-defopt.mbox" by setting "git config am.threeway yes"
once, while having an option to countermand that per invocation, by
saying "git am --no-3way ./+ay-defopt.mbox".

I think what I am saying is that an ultra-generic solution like the
patch I am commenting on implements is way too simple to be useful.

With today's code, our users can do this once:

    git config alias.am3 "am -3"

and then "git am3 ./+ay-defopt.mbox" would behave as if they typed
"git am -3 ./+ay-defopt.mbox", which would already be one step more
useful than this "only when there is no argument" design.

I think the problem with this patch ultimately came from a poor
phrasing of the Micro suggestion, which said something like "find
some common command options and add configuration".  It was meant to
allow many different people to do many different things (i.e. one
person does am.threeway and am.threeway only), not an invitation to
design something that is generic that covers all these command
options in one go.

So, perhaps limit the scope of Micro to a single command with a few
commonly desired configured options and try again?

Thanks.