public inbox for git@vger.kernel.org 
 help / color / mirror / Atom feed

From: Junio C Hamano <gitster@pobox•com>
To: slavomir vlcek <svlc@inventati•org>
Cc: git@vger•kernel.org, sschuberth@gmail•com
Subject: Re: [PATCH/RFC] builtin: move builtin retrieval to get_builtin()
Date: Thu, 13 Nov 2014 10:19:55 -0800	[thread overview]
Message-ID: <xmqq389n2cac.fsf@gitster.dls.corp.google.com> (raw)
In-Reply-To: <5463DA20.3080703@inventati.org> (slavomir vlcek's message of "Wed, 12 Nov 2014 23:07:28 +0100")

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?

>  	}
>  }

next prev parent reply	other threads:[~2014-11-13 18:20 UTC|newest]

Thread overview: 9+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2014-11-12 22:07 [PATCH/RFC] builtin: move builtin retrieval to get_builtin() slavomir vlcek
2014-11-13  0:29 ` [PATCH] SubmittingPatches: fix an inconsistency slavomir vlcek
2014-11-13 18:20   ` Junio C Hamano
2014-11-13 18:28     ` Junio C Hamano
2014-11-13 18:30       ` Junio C Hamano
2014-11-13 21:41         ` slavomir vlcek
2014-11-13 18:19 ` Junio C Hamano [this message]
2014-11-16 23:33   ` [PATCH/RFC] builtin: move builtin retrieval to get_builtin() Slavomir Vlcek
2014-11-17 16:42     ` Junio C Hamano

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=xmqq389n2cac.fsf@gitster.dls.corp.google.com \
    --to=gitster@pobox$(echo .)com \
    --cc=git@vger$(echo .)kernel.org \
    --cc=sschuberth@gmail$(echo .)com \
    --cc=svlc@inventati$(echo .)org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox