Re: [PATCH/RFC] builtin: move builtin retrieval to get_builtin()

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

slavomir vlcek <svlc@inventati.org> writes:

> Hi,
> found a small code redundancy in a builtin command retrieval ('git.c').
>
> For the "master" branch.
>
> Thanks in advance for any suggestions.
>
> Signed-off-by: slavomir vlcek <svlc@inventati.org>
> ---

Thanks.  Please do realize that all of the above before the
three-dash line and nothing else will be made into the commit log
message (together with what you wrote on the Subject: line).

Which means these lines...

> From 78228e3f7c3029d07827f973fa7992777d6e0cb9 Mon Sep 17 00:00:00 2001
> From: slavomir vlcek <svlc@inventati.org>
> Date: Wed, 12 Nov 2014 14:10:22 +0100
> Subject: [PATCH] builtin: move builtin retrieval to get_builtin()
>
> There was a redundant code for a builtin command retrieval
> in 'handle_builtin()' and 'is_builtin()'.
>
> This was solved by adding a new function 'get_builtin()'
> and by making a boolean wrapper out of the 'is_builtin()'.
> ---

... will not be part of the log message, which is definitely wrong.

To correct this:

$ git checkout 78228e3f7c3029d0
$ git commit --amend -s --no-edit

to add your sign-off in the log message, then do

$ git format-patch -1 --stdout >patch.mail

Slurp patch.mail into your MUA, move the content on "Subject: " to
where your MUA expects to see the subject line, remove other header
material starting from "From 7822..." so that the message body
begins with "There was a redundant code for...".  And send it out.

>  git.c | 27 +++++++++++++++------------
>  1 file changed, 15 insertions(+), 12 deletions(-)
>
> diff --git a/git.c b/git.c
> index 18fbf79..e32c5b8 100644
> --- a/git.c
> +++ b/git.c
> @@ -487,15 +487,20 @@ static struct cmd_struct commands[] = {
>  	{ "write-tree", cmd_write_tree, RUN_SETUP },
>  };
>  
> -int is_builtin(const char *s)
> +struct cmd_struct *get_builtin(const char *s)

I do not think this has to be extern.

	static struct cmd_struct *get_builtin(const char *s)

perhaps.

> @@ -519,15 +525,12 @@ static void handle_builtin(int argc, const char **argv)
>  		argv[0] = cmd = "help";
>  	}
>  
> -	for (i = 0; i < ARRAY_SIZE(commands); i++) {
> -		struct cmd_struct *p = commands+i;
> -		if (strcmp(p->cmd, cmd))
> -			continue;
> -		if (saved_environment && (p->option & NO_SETUP)) {
> +	builtin = get_builtin(cmd);

Nice.

> +	if (builtin) {
> +		if (saved_environment && (builtin->option & NO_SETUP))
>  			restore_env();
> -			break;
> -		}
> -		exit(run_builtin(p, argc, argv));
> +		else
> +			exit(run_builtin(builtin, argc, argv));

This change does not seem to have anything to do with the topic of
the change.  Why is it necessary?

>  	}
>  }