threads / patch / 24901

patchbuiltin/merge_recursive.c: Add a usage string and make use of it.

Subject: [PATCH] builtin/merge_recursive.c: Add a usage string and make use of it.

## tl;dr

10 messages between Aug 29, 2010 and Sep 1, 2010. Diffs are folded; open one to read it.

replies: 9people: 3as markdown or json

Thiago Farina· Aug 29, 2010, 17:39 UTC · lore

This improves the usage output by adding builtin_merge_recursive_usage string that follows the same pattern used by the other builtin commands.

Also it uses usage() function instead of the usagef() function.

The previous output for git merger-recursive was: usage: merge-recursive <base>... -- <head> <remote> ...

Now the output is: usage: git merge-recursive <base>... -- <head> <remote> ...

Signed-off-by: Thiago Farina <tfransosi@gmail.com>
---
 builtin/merge-recursive.c |   17 ++++++++++-------
 1 files changed, 10 insertions(+), 7 deletions(-)
Show changes to builtin/merge-recursive.c +10 −7
diff --git a/builtin/merge-recursive.c b/builtin/merge-recursive.c
index d8875d5..46c2a85 100644
--- a/builtin/merge-recursive.c
+++ b/builtin/merge-recursive.c
@@ -3,6 +3,9 @@
 #include "tag.h"
 #include "merge-recursive.h"
 
+static const char builtin_merge_recursive_usage[] =
+	"git merge-recursive <base>... -- <head> <remote> ...";
+
 static const char *better_branch_name(const char *branch)
 {
 	static char githead_env[8 + 40 + 1];
@@ -29,7 +32,7 @@ int cmd_merge_recursive(int argc, const char **argv, const char *prefix)
 		o.subtree_shift = "";
 
 	if (argc < 4)
-		usagef("%s <base>... -- <head> <remote> ...", argv[0]);
+		usage(builtin_merge_recursive_usage);
 
 	for (i = 1; i < argc; ++i) {
 		const char *arg = argv[i];
@@ -37,19 +40,19 @@ int cmd_merge_recursive(int argc, const char **argv, const char *prefix)
 		if (!prefixcmp(arg, "--")) {
 			if (!arg[2])
 				break;
-			if (!strcmp(arg+2, "ours"))
+			if (!strcmp(arg + 2, "ours"))
 				o.recursive_variant = MERGE_RECURSIVE_OURS;
-			else if (!strcmp(arg+2, "theirs"))
+			else if (!strcmp(arg + 2, "theirs"))
 				o.recursive_variant = MERGE_RECURSIVE_THEIRS;
-			else if (!strcmp(arg+2, "subtree"))
+			else if (!strcmp(arg + 2, "subtree"))
 				o.subtree_shift = "";
-			else if (!prefixcmp(arg+2, "subtree="))
+			else if (!prefixcmp(arg + 2, "subtree="))
 				o.subtree_shift = arg + 10;
 			else
 				die("Unknown option %s", arg);
 			continue;
 		}
-		if (bases_count < ARRAY_SIZE(bases)-1) {
+		if (bases_count < ARRAY_SIZE(bases) - 1) {
 			unsigned char *sha = xmalloc(20);
 			if (get_sha1(argv[i], sha))
 				die("Could not parse object '%s'", argv[i]);
@@ -58,7 +61,7 @@ int cmd_merge_recursive(int argc, const char **argv, const char *prefix)
 		else
 			warning("Cannot handle more than %d bases. "
 				"Ignoring %s.",
-				(int)ARRAY_SIZE(bases)-1, argv[i]);
+				(int)ARRAY_SIZE(bases) - 1, argv[i]);
 	}
 	if (argc - i != 3) /* "--" "<head>" "<remote>" */
 		die("Not handling anything other than two heads merge.");

-- 
1.7.2.1.95.g3d045
Jonathan Nieder· Aug 29, 2010, 21:46 UTC · re: Thiago Farina · lore

Re: [PATCH] builtin/merge_recursive.c: Add a usage string and make use of it.

Hi Thiago,
Thiago Farina wrote:
> This improves the usage output by adding builtin_merge_recursive_usage string
> that follows the same pattern used by the other builtin commands.
> 
> Also it uses usage() function instead of the usagef() function.
FWIW cmd_merge_recursive is used to handle four different commands.

$ git grep -F -e cmd_merge_recursive builtin.h:extern int cmd_merge_recursive(int argc, const char **argv, const char *prefix); builtin/merge-recursive.c:int cmd_merge_recursive(int argc, const char **argv, const char *prefix) git.c: { "merge-recursive", cmd_merge_recursive, RUN_SETUP | NEED_WORK_TREE }, git.c: { "merge-recursive-ours", cmd_merge_recursive, RUN_SETUP | NEED_WORK_TREE }, git.c: { "merge-recursive-theirs", cmd_merge_recursive, RUN_SETUP | NEED_WORK_TREE }, git.c: { "merge-subtree", cmd_merge_recursive, RUN_SETUP | NEED_WORK_TREE },

What will be the output from
 $ git merge-subtree -h
after this change?
Show 8 quoted lines
> --- a/builtin/merge-recursive.c
> +++ b/builtin/merge-recursive.c
> @@ -37,19 +40,19 @@ int cmd_merge_recursive(int argc, const char **argv, const char *prefix)
>  		if (!prefixcmp(arg, "--")) {
>  			if (!arg[2])
>  				break;
> -			if (!strcmp(arg+2, "ours"))
> +			if (!strcmp(arg + 2, "ours"))

Changes like these, while nice, would make more sense as a separate patch.

Hope that helps, Jonathan

Thiago Farina· Aug 29, 2010, 22:20 UTC · re: Jonathan Nieder · lore

[PATCH v2] builtin/merge_recursive.c: Add an usage string and make use of it.

This improves the usage output by adding builtin_merge_recursive_usage string that follows the same pattern used by the other builtin commands.

The previous output for git merger-recursive was: usage: merge-recursive <base>... -- <head> <remote> ...

Now the output is: usage: git merge-recursive <base>... -- <head> <remote> ...

Signed-off-by: Thiago Farina <tfransosi@gmail.com>
---
 builtin/merge-recursive.c |   17 ++++++++++-------
 1 files changed, 10 insertions(+), 7 deletions(-)
Show changes to builtin/merge-recursive.c +10 −7
diff --git a/builtin/merge-recursive.c b/builtin/merge-recursive.c
index d8875d5..189ba42 100644
--- a/builtin/merge-recursive.c
+++ b/builtin/merge-recursive.c
@@ -3,6 +3,9 @@
 #include "tag.h"
 #include "merge-recursive.h"
 
+static const char builtin_merge_recursive_usage[] =
+	"git %s <base>... -- <head> <remote> ...";
+
 static const char *better_branch_name(const char *branch)
 {
 	static char githead_env[8 + 40 + 1];
@@ -29,7 +32,7 @@ int cmd_merge_recursive(int argc, const char **argv, const char *prefix)
 		o.subtree_shift = "";
 
 	if (argc < 4)
-		usagef("%s <base>... -- <head> <remote> ...", argv[0]);
+		usagef(builtin_merge_recursive_usage, argv[0]);
 
 	for (i = 1; i < argc; ++i) {
 		const char *arg = argv[i];
@@ -37,19 +40,19 @@ int cmd_merge_recursive(int argc, const char **argv, const char *prefix)
 		if (!prefixcmp(arg, "--")) {
 			if (!arg[2])
 				break;
-			if (!strcmp(arg+2, "ours"))
+			if (!strcmp(arg + 2, "ours"))
 				o.recursive_variant = MERGE_RECURSIVE_OURS;
-			else if (!strcmp(arg+2, "theirs"))
+			else if (!strcmp(arg + 2, "theirs"))
 				o.recursive_variant = MERGE_RECURSIVE_THEIRS;
-			else if (!strcmp(arg+2, "subtree"))
+			else if (!strcmp(arg + 2, "subtree"))
 				o.subtree_shift = "";
-			else if (!prefixcmp(arg+2, "subtree="))
+			else if (!prefixcmp(arg + 2, "subtree="))
 				o.subtree_shift = arg + 10;
 			else
 				die("Unknown option %s", arg);
 			continue;
 		}
-		if (bases_count < ARRAY_SIZE(bases)-1) {
+		if (bases_count < ARRAY_SIZE(bases) - 1) {
 			unsigned char *sha = xmalloc(20);
 			if (get_sha1(argv[i], sha))
 				die("Could not parse object '%s'", argv[i]);
@@ -58,7 +61,7 @@ int cmd_merge_recursive(int argc, const char **argv, const char *prefix)
 		else
 			warning("Cannot handle more than %d bases. "
 				"Ignoring %s.",
-				(int)ARRAY_SIZE(bases)-1, argv[i]);
+				(int)ARRAY_SIZE(bases) - 1, argv[i]);
 	}
 	if (argc - i != 3) /* "--" "<head>" "<remote>" */
 		die("Not handling anything other than two heads merge.");

-- 
1.7.2.1.95.g3d045
Jonathan Nieder· Aug 30, 2010, 02:42 UTC · re: Thiago Farina · lore

Re: [PATCH v2] builtin/merge_recursive.c: Add an usage string and make use of it.

Thiago Farina wrote:
Show 5 quoted lines
> The previous output for git merger-recursive was:
> usage: merge-recursive <base>... -- <head> <remote> ...
> 
> Now the output is:
> usage: git merge-recursive <base>... -- <head> <remote> ...
[...]
Show 8 quoted lines
> --- a/builtin/merge-recursive.c
> +++ b/builtin/merge-recursive.c
> @@ -3,6 +3,9 @@
>  #include "tag.h"
>  #include "merge-recursive.h"
>  
> +static const char builtin_merge_recursive_usage[] =
> +	"git %s <base>... -- <head> <remote> ...";

Now that you've researched it, wouldn't it make sense to include an explanation for this %s in the log message?

Still, ack on this part.
Show 6 quoted lines
> @@ -37,19 +40,19 @@ int cmd_merge_recursive(int argc, const char **argv, const char *prefix)
>  		if (!prefixcmp(arg, "--")) {
>  			if (!arg[2])
>  				break;
> -			if (!strcmp(arg+2, "ours"))
> +			if (!strcmp(arg + 2, "ours"))

My comment on the rest still applies: it is best if patches do not contain unrelated changes to unrelated parts of the files they touch.

Thiago Farina· Aug 30, 2010, 03:30 UTC · re: Jonathan Nieder · lore

[PATCH v3] builtin/merge_recursive.c: Add an usage string and make use of it.

This improves the usage output by adding builtin_merge_recursive_usage string that follows the same pattern used by the other builtin commands.

The previous output for git merger-recursive was: usage: merge-recursive <base>... -- <head> <remote> ...

Now the output is: usage: git merge-recursive <base>... -- <head> <remote> ...

Since cmd_merge_recursive is used to handle four different commands we need the %s in the usage string, so the following example:

$ git merge-subtree -h

Will output: usage: git merge-subtree <base>... -- <head> <remote> ...

Signed-off-by: Thiago Farina <tfransosi@gmail.com>
---
 builtin/merge-recursive.c |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)
Show changes to builtin/merge-recursive.c +4 −1
diff --git a/builtin/merge-recursive.c b/builtin/merge-recursive.c
index d8875d5..3d00adb 100644
--- a/builtin/merge-recursive.c
+++ b/builtin/merge-recursive.c
@@ -3,6 +3,9 @@
 #include "tag.h"
 #include "merge-recursive.h"
 
+static const char builtin_merge_recursive_usage[] =
+	"git %s <base>... -- <head> <remote> ...";
+
 static const char *better_branch_name(const char *branch)
 {
 	static char githead_env[8 + 40 + 1];
@@ -29,7 +32,7 @@ int cmd_merge_recursive(int argc, const char **argv, const char *prefix)
 		o.subtree_shift = "";
 
 	if (argc < 4)
-		usagef("%s <base>... -- <head> <remote> ...", argv[0]);
+		usagef(builtin_merge_recursive_usage, argv[0]);
 
 	for (i = 1; i < argc; ++i) {
 		const char *arg = argv[i];

-- 
1.7.2.1.95.g3d045
Junio C Hamano· Aug 30, 2010, 06:04 UTC · re: Thiago Farina · lore

Re: [PATCH v3] builtin/merge_recursive.c: Add an usage string and make use of it.

Thiago Farina <tfransosi@gmail.com> writes:
Show 9 quoted lines
> Since cmd_merge_recursive is used to handle four different commands we need
> the %s in the usage string, so the following example:
>
> $ git merge-subtree -h
>
> Will output:
> usage: git merge-subtree <base>... -- <head> <remote> ...
>
> Signed-off-by: Thiago Farina <tfransosi@gmail.com>
Hmm, why isn't this a single liner
> -		usagef("%s <base>... -- <head> <remote> ...", argv[0]);
> +		usagef("git %s <base>... -- <head> <remote> ...", argv[0]);
Jonathan Nieder· Aug 31, 2010, 23:42 UTC · re: Junio C Hamano · lore

Re: [PATCH v3] builtin/merge_recursive.c: Add an usage string and make use of it.

Junio C Hamano wrote:
> Hmm, why isn't this a single liner
>
>> -		usagef("%s <base>... -- <head> <remote> ...", argv[0]);
>> +		usagef("git %s <base>... -- <head> <remote> ...", argv[0]);

I can't speak for Thiago, but I assume it's for unity of style with more complicated commands like "git fetch" that include a usage message at the top of the file as:

 - a quick introduction for people who don't like to scroll
 - a variable that can be used from other functions when command-line
   mistakes are noticed late.
I would have gone for the one-liner, myself.
Thiago Farina· Sep 1, 2010, 02:20 UTC · re: Jonathan Nieder · lore

Re: [PATCH v3] builtin/merge_recursive.c: Add an usage string and make use of it.

On Tue, Aug 31, 2010 at 8:42 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:
Show 16 quoted lines
> Junio C Hamano wrote:
>
>> Hmm, why isn't this a single liner
>>
>>> -            usagef("%s <base>... -- <head> <remote> ...", argv[0]);
>>> +            usagef("git %s <base>... -- <head> <remote> ...", argv[0]);
>
> I can't speak for Thiago, but I assume it's for unity of style with
> more complicated commands like "git fetch" that include a usage
> message at the top of the file as:
>
>  - a quick introduction for people who don't like to scroll
>
>  - a variable that can be used from other functions when command-line
>   mistakes are noticed late.
>
Precise explanation :-)
Thanks Jonathan.
Thiago Farina· Aug 30, 2010, 03:32 UTC · re: Jonathan Nieder · lore

Re: [PATCH v2] builtin/merge_recursive.c: Add an usage string and make use of it.

On Sun, Aug 29, 2010 at 11:42 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:
> Now that you've researched it, wouldn't it make sense to include an
> explanation for this %s in the log message?
>
Sure, added an the explanation of %s in the usage string and the example too.
> Still, ack on this part.
>
Thanks.
Show 10 quoted lines
>> @@ -37,19 +40,19 @@ int cmd_merge_recursive(int argc, const char **argv, const char *prefix)
>>               if (!prefixcmp(arg, "--")) {
>>                       if (!arg[2])
>>                               break;
>> -                     if (!strcmp(arg+2, "ours"))
>> +                     if (!strcmp(arg + 2, "ours"))
>
> My comment on the rest still applies: it is best if patches do not
> contain unrelated changes to unrelated parts of the files they touch.
>
Reverted these style fixes in patchset v3.
Thiago Farina· Aug 29, 2010, 22:24 UTC · re: Jonathan Nieder · lore

Re: [PATCH] builtin/merge_recursive.c: Add a usage string and make use of it.

On Sun, Aug 29, 2010 at 6:46 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:
Show 23 quoted lines
> Hi Thiago,
>
> Thiago Farina wrote:
>
>> This improves the usage output by adding builtin_merge_recursive_usage string
>> that follows the same pattern used by the other builtin commands.
>>
>> Also it uses usage() function instead of the usagef() function.
>
> FWIW cmd_merge_recursive is used to handle four different commands.
>
> $ git grep -F -e cmd_merge_recursive
> builtin.h:extern int cmd_merge_recursive(int argc, const char **argv, const char *prefix);
> builtin/merge-recursive.c:int cmd_merge_recursive(int argc, const char **argv, const char *prefix)
> git.c:          { "merge-recursive", cmd_merge_recursive, RUN_SETUP | NEED_WORK_TREE },
> git.c:          { "merge-recursive-ours", cmd_merge_recursive, RUN_SETUP | NEED_WORK_TREE },
> git.c:          { "merge-recursive-theirs", cmd_merge_recursive, RUN_SETUP | NEED_WORK_TREE },
> git.c:          { "merge-subtree", cmd_merge_recursive, RUN_SETUP | NEED_WORK_TREE },
>
> What will be the output from
>
>  $ git merge-subtree -h
>
Thanks for catching this, fixed in the v2 patch.

← back to recent threads