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

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 5, 2015, 01:11 UTC
Message-ID: <xmqqvbig5jud.fsf@gitster.dls.corp.google.com>
In-Reply-To: <CAFAMDXbY6szmQ-gLB0_j2cYRVHCsJfszw2XU16eb6i5NJPX_pQ@mail.gmail.com>

Amate Yolande <yolandeamate@gmail.com> writes:

Show 6 quoted lines

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

Show 15 quoted lines

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