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

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

[slavomir vlcek]

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

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


 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)
 {
 	int i;
 	for (i = 0; i < ARRAY_SIZE(commands); i++) {
-		struct cmd_struct *p = commands+i;
+		struct cmd_struct *p = commands + i;
 		if (!strcmp(s, p->cmd))
-			return 1;
+			return p;
 	}
-	return 0;
+	return NULL;
+}
+
+int is_builtin(const char *s)
+{
+	return !!get_builtin(s);
 }
 
 static void handle_builtin(int argc, const char **argv)
@@ -503,6 +508,7 @@ static void handle_builtin(int argc, const char **argv)
 	const char *cmd = argv[0];
 	int i;
 	static const char ext[] = STRIP_EXTENSION;
+	struct cmd_struct *builtin;
 
 	if (sizeof(ext) > 1) {
 		i = strlen(argv[0]) - strlen(ext);
@@ -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);
+	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));
 	}
 }
 
-- 
2.0.1

[PATCH] SubmittingPatches: fix an inconsistency

[slavomir vlcek]

The 'SubmittingPatches' document contains a small inconsistency
 in a patch-email sending procedure.

Not a big thing,
but a newcomer could get confused.

Please,
also consider adding the definition/explanation for all the branches available
at the beginning of this document (and maybe even what their names stand for).
Thanks.

Signed-off-by: slavomir vlcek <svlc@inventati.org>
---

>From 74859712cf805663e3863686bdc09511c74b207b Mon Sep 17 00:00:00 2001
From: slavomir vlcek <svlc@inventati.org>
Date: Thu, 13 Nov 2014 00:18:39 +0100
Subject: [PATCH] SubmittingPatches: fix an inconsistency

At line 213 there was an instruction:
  "re-send it with "To:" set to the maintainer [*1*] and "cc:" the list [*2*]"

and this instruction got repeated once more in the document (line 340):
  "Send it to the list and cc the maintainer."

This inconsistency was solved by editing the second occurance.
---
 Documentation/SubmittingPatches | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Documentation/SubmittingPatches b/Documentation/SubmittingPatches
index e6d46ed..fa71b5f 100644
--- a/Documentation/SubmittingPatches
+++ b/Documentation/SubmittingPatches
@@ -337,7 +337,7 @@ suggests to the contributors:
      spend their time to improve your patch.  Go back to step (2).
 
  (4) The list forms consensus that the last round of your patch is
-     good.  Send it to the list and cc the maintainer.
+     good.  Send it to the maintainer and cc the list.
 
  (5) A topic branch is created with the patch and is merged to 'next',
      and cooked further and eventually graduates to 'master'.
-- 
2.0.1

Re: [PATCH] SubmittingPatches: fix an inconsistency

[Junio C Hamano]

slavomir vlcek <svlc@inventati.org> writes:

> The 'SubmittingPatches' document contains a small inconsistency
>  in a patch-email sending procedure.
>
> Not a big thing,
> but a newcomer could get confused.
>
> Please,
> also consider adding the definition/explanation for all the branches available
> at the beginning of this document (and maybe even what their names stand for).
> Thanks.
>
> Signed-off-by: slavomir vlcek <svlc@inventati.org>
> ---
>
> From 74859712cf805663e3863686bdc09511c74b207b Mon Sep 17 00:00:00 2001
> From: slavomir vlcek <svlc@inventati.org>
> Date: Thu, 13 Nov 2014 00:18:39 +0100
> Subject: [PATCH] SubmittingPatches: fix an inconsistency
>
> At line 213 there was an instruction:
>   "re-send it with "To:" set to the maintainer [*1*] and "cc:" the list [*2*]"
>
> and this instruction got repeated once more in the document (line 340):
>   "Send it to the list and cc the maintainer."
>
> This inconsistency was solved by editing the second occurance.
> ---

The same comment applies to the log message part.

Will queue; no need to resend.

Thanks.

>  Documentation/SubmittingPatches | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/Documentation/SubmittingPatches b/Documentation/SubmittingPatches
> index e6d46ed..fa71b5f 100644
> --- a/Documentation/SubmittingPatches
> +++ b/Documentation/SubmittingPatches
> @@ -337,7 +337,7 @@ suggests to the contributors:
>       spend their time to improve your patch.  Go back to step (2).
>  
>   (4) The list forms consensus that the last round of your patch is
> -     good.  Send it to the list and cc the maintainer.
> +     good.  Send it to the maintainer and cc the list.
>  
>   (5) A topic branch is created with the patch and is merged to 'next',
>       and cooked further and eventually graduates to 'master'.

Re: [PATCH] SubmittingPatches: fix an inconsistency

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

>> Signed-off-by: slavomir vlcek <svlc@inventati.org>

> The same comment applies to the log message part.

I said:

> Will queue; no need to resend.
>
> Thanks.

But one thing to make sure.  Do you really mean to have your
sign-off with all lowercase?  I can amend the patch to read

    Signed-off-by: Slavomir Vlcek <svlc@inventati.org>

while applying, so that your name does not stand out like a sore
thumb in "git shortlog -20 -s" output, if you want.

Re: [PATCH] SubmittingPatches: fix an inconsistency

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Junio C Hamano <gitster@pobox.com> writes:
>
>>> Signed-off-by: slavomir vlcek <svlc@inventati.org>
>
>> The same comment applies to the log message part.
>
> I said:
>
>> Will queue; no need to resend.
>>
>> Thanks.
>
> But one thing to make sure.  Do you really mean to have your
> sign-off with all lowercase?  I can amend the patch to read
>
>     Signed-off-by: Slavomir Vlcek <svlc@inventati.org>
>
> while applying, so that your name does not stand out like a sore
> thumb in "git shortlog -20 -s" output, if you want.

... by the above, I mean something like what appears after the
scissors "-- >8 --" line below.

-- >8 --
From: Slavomir Vlcek <svlc@inventati.org>
Date: Thu, 13 Nov 2014 00:18:39 +0100
Subject: [PATCH] SubmittingPatches: final submission is To: maintainer and CC: list

In an earlier part there is:

  "re-send it with "To:" set to the maintainer [*1*] and "cc:" the list [*2*]"

for the final submission, but later we see

  "Send it to the list and cc the maintainer."

Fix the later one to match the previous.

Signed-off-by: Slavomir Vlcek <svlc@inventati.org>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 Documentation/SubmittingPatches | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Documentation/SubmittingPatches b/Documentation/SubmittingPatches
index e6d46ed..fa71b5f 100644
--- a/Documentation/SubmittingPatches
+++ b/Documentation/SubmittingPatches
@@ -337,7 +337,7 @@ suggests to the contributors:
      spend their time to improve your patch.  Go back to step (2).
 
  (4) The list forms consensus that the last round of your patch is
-     good.  Send it to the list and cc the maintainer.
+     good.  Send it to the maintainer and cc the list.
 
  (5) A topic branch is created with the patch and is merged to 'next',
      and cooked further and eventually graduates to 'master'.
-- 
2.2.0-rc1-84-gcd6439f

Re: [PATCH] SubmittingPatches: fix an inconsistency

[slavomir vlcek]

On 11/13/2014 07:30 PM, Junio C Hamano wrote:
> Junio C Hamano <gitster@pobox.com> writes:
> 
>> Junio C Hamano <gitster@pobox.com> writes:
>>
>>>> Signed-off-by: slavomir vlcek <svlc@inventati.org>
>>
>>> The same comment applies to the log message part.
>>
>> I said:
>>
>>> Will queue; no need to resend.
>>>
>>> Thanks.
>>
>> But one thing to make sure.  Do you really mean to have your
>> sign-off with all lowercase?  I can amend the patch to read
>>
>>     Signed-off-by: Slavomir Vlcek <svlc@inventati.org>
>>
>> while applying, so that your name does not stand out like a sore
>> thumb in "git shortlog -20 -s" output, if you want.
> 
> ... by the above, I mean something like what appears after the
> scissors "-- >8 --" line below.
> 

Yes, agreed. Thanks for the corrections.

> -- >8 --
> From: Slavomir Vlcek <svlc@inventati.org>
> Date: Thu, 13 Nov 2014 00:18:39 +0100
> Subject: [PATCH] SubmittingPatches: final submission is To: maintainer and CC: list
> 
> In an earlier part there is:
> 
>   "re-send it with "To:" set to the maintainer [*1*] and "cc:" the list [*2*]"
> 
> for the final submission, but later we see
> 
>   "Send it to the list and cc the maintainer."
> 
> Fix the later one to match the previous.
> 
> Signed-off-by: Slavomir Vlcek <svlc@inventati.org>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
>  Documentation/SubmittingPatches | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/Documentation/SubmittingPatches b/Documentation/SubmittingPatches
> index e6d46ed..fa71b5f 100644
> --- a/Documentation/SubmittingPatches
> +++ b/Documentation/SubmittingPatches
> @@ -337,7 +337,7 @@ suggests to the contributors:
>       spend their time to improve your patch.  Go back to step (2).
>  
>   (4) The list forms consensus that the last round of your patch is
> -     good.  Send it to the list and cc the maintainer.
> +     good.  Send it to the maintainer and cc the list.
>  
>   (5) A topic branch is created with the patch and is merged to 'next',
>       and cooked further and eventually graduates to 'master'.
> 

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

[Junio C Hamano]

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?

>  	}
>  }

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

[Slavomir Vlcek]

On 11/13/2014 07:19 PM, Junio C Hamano wrote:
>>  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?

Does the commit message lack some explanation
or the patch would better be divided into several parts?

I noticed that the patch has been modified (suggested 'static'
scope modification, commit message) and added
to the 'next' branch. So does this mean my task is done
or is there still something I should explain?

Thank you for your corrections.

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

[Junio C Hamano]

Slavomir Vlcek <svlc@inventati.org> writes:

> I noticed that the patch has been modified (suggested 'static'
> scope modification, commit message) and added to the 'next'
> branch. So does this mean my task is done [...]?

Even after the change hits 'next', other people may still find
problems and rooms for improvements that you and those who reviewed
the patch missed, and you may have to respond with follow-up patches
on top.  We'll usually give 3 days to a few weeks, depending on the
complexity of the change, for that to happen and call it "cooking
the patch in 'next'".  After that the change will be merged to
'master' so that it can appear in the next release.

For now, the patch you sent is "done", even though you can still
send improvements on top if you want to.

Thanks.