[PATCH] Start conforming code to "git subcmd" style

[PATCH] Start conforming code to "git subcmd" style

[Heikki Orsila]

User notifications are presented as 'git cmd', and code comments
are presented as '"cmd"' or 'git's cmd', rather than 'git-cmd'.

Signed-off-by: Heikki Orsila <heikki.orsila@iki.fi>
---
There will be more patches to come. I'm splitting these patches to 
approximately 200 lines for better review.

The patch tries to conform to Junio's style suggestions.

 builtin-apply.c            |    8 ++++----
 builtin-archive.c          |    8 ++++----
 builtin-blame.c            |    6 +++---
 builtin-branch.c           |    2 +-
 builtin-bundle.c           |    4 ++--
 builtin-cat-file.c         |    4 ++--
 builtin-check-ref-format.c |    2 +-
 7 files changed, 17 insertions(+), 17 deletions(-)

diff --git a/builtin-apply.c b/builtin-apply.c
index 2216a0b..4eb263e 100644
--- a/builtin-apply.c
+++ b/builtin-apply.c
@@ -274,7 +274,7 @@ static void say_patch_name(FILE *output, const char *pre,
 static void read_patch_file(struct strbuf *sb, int fd)
 {
 	if (strbuf_read(sb, fd, 0) < 0)
-		die("git-apply: read returned %s", strerror(errno));
+		die("git apply: read returned %s", strerror(errno));
 
 	/*
 	 * Make sure that we have some slop in the buffer
@@ -506,17 +506,17 @@ static char *gitdiff_verify_name(const char *line, int isnull, char *orig_name,
 		name = orig_name;
 		len = strlen(name);
 		if (isnull)
-			die("git-apply: bad git-diff - expected /dev/null, got %s on line %d", name, linenr);
+			die("git apply: bad git-diff - expected /dev/null, got %s on line %d", name, linenr);
 		another = find_name(line, NULL, p_value, TERM_TAB);
 		if (!another || memcmp(another, name, len))
-			die("git-apply: bad git-diff - inconsistent %s filename on line %d", oldnew, linenr);
+			die("git apply: bad git-diff - inconsistent %s filename on line %d", oldnew, linenr);
 		free(another);
 		return orig_name;
 	}
 	else {
 		/* expect "/dev/null" */
 		if (memcmp("/dev/null", line, 9) || line[9] != '\n')
-			die("git-apply: bad git-diff - expected /dev/null on line %d", linenr);
+			die("git apply: bad git-diff - expected /dev/null on line %d", linenr);
 		return NULL;
 	}
 }
diff --git a/builtin-archive.c b/builtin-archive.c
index 22445ac..5ceec43 100644
--- a/builtin-archive.c
+++ b/builtin-archive.c
@@ -47,18 +47,18 @@ static int run_remote_archiver(const char *remote, int argc,
 
 	len = packet_read_line(fd[0], buf, sizeof(buf));
 	if (!len)
-		die("git-archive: expected ACK/NAK, got EOF");
+		die("git archive: expected ACK/NAK, got EOF");
 	if (buf[len-1] == '\n')
 		buf[--len] = 0;
 	if (strcmp(buf, "ACK")) {
 		if (len > 5 && !prefixcmp(buf, "NACK "))
-			die("git-archive: NACK %s", buf + 5);
-		die("git-archive: protocol error");
+			die("git archive: NACK %s", buf + 5);
+		die("git archive: protocol error");
 	}
 
 	len = packet_read_line(fd[0], buf, sizeof(buf));
 	if (len)
-		die("git-archive: expected a flush");
+		die("git archive: expected a flush");
 
 	/* Now, start reading from fd[0] and spit it out to stdout */
 	rv = recv_sideband("archive", fd[0], 1, 2);
diff --git a/builtin-blame.c b/builtin-blame.c
index e4d12de..b8fa914 100644
--- a/builtin-blame.c
+++ b/builtin-blame.c
@@ -1787,7 +1787,7 @@ static int prepare_lines(struct scoreboard *sb)
 
 /*
  * Add phony grafts for use with -S; this is primarily to
- * support git-cvsserver that wants to give a linear history
+ * support git's cvsserver that wants to give a linear history
  * to its clients.
  */
 static int read_ancestry(const char *graft_file)
@@ -2299,12 +2299,12 @@ int cmd_blame(int argc, const char **argv, const char *prefix)
 		OPT_BIT('f', "show-name", &output_option, "Show original filename (Default: auto)", OUTPUT_SHOW_NAME),
 		OPT_BIT('n', "show-number", &output_option, "Show original linenumber (Default: off)", OUTPUT_SHOW_NUMBER),
 		OPT_BIT('p', "porcelain", &output_option, "Show in a format designed for machine consumption", OUTPUT_PORCELAIN),
-		OPT_BIT('c', NULL, &output_option, "Use the same output mode as git-annotate (Default: off)", OUTPUT_ANNOTATE_COMPAT),
+		OPT_BIT('c', NULL, &output_option, "Use the same output mode as git annotate (Default: off)", OUTPUT_ANNOTATE_COMPAT),
 		OPT_BIT('t', NULL, &output_option, "Show raw timestamp (Default: off)", OUTPUT_RAW_TIMESTAMP),
 		OPT_BIT('l', NULL, &output_option, "Show long commit SHA1 (Default: off)", OUTPUT_LONG_OBJECT_NAME),
 		OPT_BIT('s', NULL, &output_option, "Suppress author name and timestamp (Default: off)", OUTPUT_NO_AUTHOR),
 		OPT_BIT('w', NULL, &xdl_opts, "Ignore whitespace differences", XDF_IGNORE_WHITESPACE),
-		OPT_STRING('S', NULL, &revs_file, "file", "Use revisions from <file> instead of calling git-rev-list"),
+		OPT_STRING('S', NULL, &revs_file, "file", "Use revisions from <file> instead of calling git rev-list"),
 		OPT_STRING(0, "contents", &contents_from, "file", "Use <file>'s contents as the final image"),
 		{ OPTION_CALLBACK, 'C', NULL, &opt, "score", "Find line copies within and across files", PARSE_OPT_OPTARG, blame_copy_callback },
 		{ OPTION_CALLBACK, 'M', NULL, &opt, "score", "Find line movements within and across files", PARSE_OPT_OPTARG, blame_move_callback },
diff --git a/builtin-branch.c b/builtin-branch.c
index b1a2ad7..8efc828 100644
--- a/builtin-branch.c
+++ b/builtin-branch.c
@@ -526,7 +526,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
 		},
 		OPT__ABBREV(&abbrev),
 
-		OPT_GROUP("Specific git-branch actions:"),
+		OPT_GROUP("Specific git branch actions:"),
 		OPT_SET_INT('a', NULL, &kinds, "list both remote-tracking and local branches",
 			REF_REMOTE_BRANCH | REF_LOCAL_BRANCH),
 		OPT_BIT('d', NULL, &delete, "delete fully merged branch", 1),
diff --git a/builtin-bundle.c b/builtin-bundle.c
index ac476e7..9b58152 100644
--- a/builtin-bundle.c
+++ b/builtin-bundle.c
@@ -6,10 +6,10 @@
  * Basic handler for bundle files to connect repositories via sneakernet.
  * Invocation must include action.
  * This function can create a bundle or provide information on an existing
- * bundle supporting git-fetch, git-pull, and git-ls-remote
+ * bundle supporting "fetch", "pull", and "ls-remote".
  */
 
-static const char *bundle_usage="git-bundle (create <bundle> <git-rev-list args> | verify <bundle> | list-heads <bundle> [refname]... | unbundle <bundle> [refname]... )";
+static const char *bundle_usage="git bundle (create <bundle> <git rev-list args> | verify <bundle> | list-heads <bundle> [refname]... | unbundle <bundle> [refname]... )";
 
 int cmd_bundle(int argc, const char **argv, const char *prefix)
 {
diff --git a/builtin-cat-file.c b/builtin-cat-file.c
index 7441a56..3fba6b9 100644
--- a/builtin-cat-file.c
+++ b/builtin-cat-file.c
@@ -137,11 +137,11 @@ static int cat_one_file(int opt, const char *exp_type, const char *obj_name)
 		break;
 
 	default:
-		die("git-cat-file: unknown option: %s\n", exp_type);
+		die("git cat-file: unknown option: %s\n", exp_type);
 	}
 
 	if (!buf)
-		die("git-cat-file %s: bad file", obj_name);
+		die("git cat-file %s: bad file", obj_name);
 
 	write_or_die(1, buf, size);
 	return 0;
diff --git a/builtin-check-ref-format.c b/builtin-check-ref-format.c
index fe04be7..701de43 100644
--- a/builtin-check-ref-format.c
+++ b/builtin-check-ref-format.c
@@ -9,6 +9,6 @@
 int cmd_check_ref_format(int argc, const char **argv, const char *prefix)
 {
 	if (argc != 2)
-		usage("git-check-ref-format refname");
+		usage("git check-ref-format refname");
 	return !!check_ref_format(argv[1]);
 }
-- 
1.6.0.1

Re: [PATCH] Start conforming code to "git subcmd" style

[Junio C Hamano]

Heikki Orsila <heikki.orsila@iki.fi> writes:

> User notifications are presented as 'git cmd', and code comments
> are presented as '"cmd"' or 'git's cmd', rather than 'git-cmd'.

Thanks.

The part I will _not_ comment on in your patch looked all good, so to
reduce further back-and-forth, I'd apply them to 'maint', excluding the
parts I did comment on in this message.

> diff --git a/builtin-apply.c b/builtin-apply.c
> ...
> @@ -506,17 +506,17 @@ static char *gitdiff_verify_name(const char *li
> ...
> -			die("git-apply: bad git-diff - expected /dev/nu...
> +			die("git apply: bad git-diff - expected /dev/nu...
> ...
> -			die("git-apply: bad git-diff - inconsistent %s ...
> +			die("git apply: bad git-diff - inconsistent %s ...
> ...
> -			die("git-apply: bad git-diff - expected /dev/nu...
> +			die("git apply: bad git-diff - expected /dev/nu...
> ...

I'd vote for doing "s/git-diff/patch/" here.  After looking at
builtin-apply.c, there is no other error/die messages that would become
ambiguous, so such a rewording won't make it harder to help people who saw
any of these error messages (or other error messages from the "git-apply"
program).

> diff --git a/builtin-blame.c b/builtin-blame.c
> ...
> @@ -2299,12 +2299,12 @@ int cmd_blame(int argc, const char **argv, co...
> ...
> -		OPT_BIT(..."Use the ... as git-annotate (Default: off)"...
> +		OPT_BIT(..."Use the ... as git annotate (Default: off)"...
> ...
> -		OPT_STRING(..."Use ...instead of calling git-rev-list"),
> +		OPT_STRING(..."Use ...instead of calling git rev-list"),
> ...

A two-word command name in a prose is hard to read; "rev-list" is not a
word and that makes the problem less serious, but it would be easier to
read if these two word command names are quoted or grouped together in
some way to make it clear they form a single noun and the sentence is
talking about a single "thing".

The old "git-foo" spelling was good for that purpose, but it will invite
user confusion so we cannot use it anymore.  Perhaps we can say "instead
of calling 'git rev-list'"?

The command name at the beginning of die message does not have this issue.
E.g. the colon in:

	die("git foo: I hate you");

is sufficient to make it clear that these two words form a single noun;
i.e. "I'm 'git foo' program, and I am telling you that I hate you".

But it might be just me, so before asking you to reroll another round, I'd
like to hear opinions from the list.

 (1) No, JC is worrying too much about readability; Heikki's patch is good;

 (2) JC's right -- "instead of calling 'git rev-list'" is much better;

 (3) Something else?

> diff --git a/builtin-branch.c b/builtin-branch.c
> @@ -526,7 +526,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
> ...
> -		OPT_GROUP("Specific git-branch actions:"),
> +		OPT_GROUP("Specific git branch actions:"),
> ...

Likewise.

Re: [PATCH] Start conforming code to "git subcmd" style

[Christian Couder]

Le samedi 30 août 2008, Junio C Hamano a écrit :
>
> The command name at the beginning of die message does not have this
> issue. E.g. the colon in:
>
> 	die("git foo: I hate you");
>
> is sufficient to make it clear that these two words form a single noun;
> i.e. "I'm 'git foo' program, and I am telling you that I hate you".
>
> But it might be just me, so before asking you to reroll another round,
> I'd like to hear opinions from the list.
>
>  (1) No, JC is worrying too much about readability; Heikki's patch is
> good;
>
>  (2) JC's right -- "instead of calling 'git rev-list'" is much better;

I agree with (2).

Regards,
Christian.

>  (3) Something else?

Re: [PATCH] Start conforming code to "git subcmd" style

[Jakub Narebski]

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

> Heikki Orsila <heikki.orsila@iki.fi> writes:
> 
> > User notifications are presented as 'git cmd', and code comments
> > are presented as '"cmd"' or 'git's cmd', rather than 'git-cmd'.
...
> > diff --git a/builtin-apply.c b/builtin-apply.c
> > ...
> > @@ -506,17 +506,17 @@ static char *gitdiff_verify_name(const char *li
> > ...
> > -			die("git-apply: bad git-diff - expected /dev/nu...
> > +			die("git apply: bad git-diff - expected /dev/nu...
> > ...
> > -			die("git-apply: bad git-diff - inconsistent %s ...
> > +			die("git apply: bad git-diff - inconsistent %s ...
> > ...
> > -			die("git-apply: bad git-diff - expected /dev/nu...
> > +			die("git apply: bad git-diff - expected /dev/nu...
> > ...
> 
> I'd vote for doing "s/git-diff/patch/" here.  After looking at
> builtin-apply.c, there is no other error/die messages that would become
> ambiguous, so such a rewording won't make it harder to help people who saw
> any of these error messages (or other error messages from the "git-apply"
> program).

I agree.  git-apply in general is presented a patch output, not
necessary git-diff output (it could be output generated by GNU diff,
or by 'scm diff' from some SCM...).

> > diff --git a/builtin-blame.c b/builtin-blame.c
> > ...
> > @@ -2299,12 +2299,12 @@ int cmd_blame(int argc, const char **argv, co...
> > ...
> > -		OPT_BIT(..."Use the ... as git-annotate (Default: off)"...
> > +		OPT_BIT(..."Use the ... as git annotate (Default: off)"...
> > ...
> > -		OPT_STRING(..."Use ...instead of calling git-rev-list"),
> > +		OPT_STRING(..."Use ...instead of calling git rev-list"),
> > ...
> 
> A two-word command name in a prose is hard to read; "rev-list" is not a
> word and that makes the problem less serious, but it would be easier to
> read if these two word command names are quoted or grouped together in
> some way to make it clear they form a single noun and the sentence is
> talking about a single "thing".
> 
> The old "git-foo" spelling was good for that purpose, but it will invite
> user confusion so we cannot use it anymore.  Perhaps we can say "instead
> of calling 'git rev-list'"?

Either "git-rev-list" or "'git rev-list'" is fine; "git rev-list"
is not, as it requires careful reading to notice which part is
proposed git command, and which the rest of message.

> The command name at the beginning of die message does not have this issue.
> E.g. the colon in:
> 
> 	die("git foo: I hate you");
> 
> is sufficient to make it clear that these two words form a single noun;
> i.e. "I'm 'git foo' program, and I am telling you that I hate you".
> 
> But it might be just me, so before asking you to reroll another round, I'd
> like to hear opinions from the list.
> 
>  (1) No, JC is worrying too much about readability; Heikki's patch is good;
> 
>  (2) JC's right -- "instead of calling 'git rev-list'" is much better;
> 
>  (3) Something else?

I think that "git foo: message" is unambiguous, and I guess _that_
could be even in one single large patch.  Other cases I guess need
careful review and thinking about in a case by case basis,
unfortunately.

Better to be careful about that change than to make change and then
notice that it is not good...

-- 
Jakub Narebski
Poland
ShadeHawk on #git

Re: [PATCH] Start conforming code to "git subcmd" style

[Junio C Hamano]

Jakub Narebski <jnareb@gmail.com> writes:

> Junio C Hamano <gitster@pobox.com> writes:
>
>> Heikki Orsila <heikki.orsila@iki.fi> writes:
>> 
>> > User notifications are presented as 'git cmd', and code comments
>> > are presented as '"cmd"' or 'git's cmd', rather than 'git-cmd'.
> ...
>> > diff --git a/builtin-apply.c b/builtin-apply.c
>> > ...
>> > @@ -506,17 +506,17 @@ static char *gitdiff_verify_name(const char *li
>> > ...
>> > -			die("git-apply: bad git-diff - expected /dev/nu...
>> > +			die("git apply: bad git-diff - expected /dev/nu...
>> > ...
>> 
>> I'd vote for doing "s/git-diff/patch/" here.  After looking at
>> builtin-apply.c, there is no other error/die messages that would become
>> ambiguous, so such a rewording won't make it harder to help people who saw
>> any of these error messages (or other error messages from the "git-apply"
>> program).
>
> I agree.  git-apply in general is presented a patch output, not
> necessary git-diff output (it could be output generated by GNU diff,
> or by 'scm diff' from some SCM...).

This codepath only is reached after we determine "diff --git" header, so
we are specifically expecting a patch intput with git flavor here.  The
original wording of the message helps somebody who diagnoses at what point
in git-apply program the input is considered corrupt.  My point was that
that line of thinking caters to a git programmer/debugger but changing it
to end user language (I suggested "patch", but I think "bad input" might
be even better) would not harm the debuggability this message is giving
us, because there is no similar message from the command from any other
codepath.

> I think that "git foo: message" is unambiguous, and I guess _that_
> could be even in one single large patch.  Other cases I guess need
> careful review and thinking about in a case by case basis,
> unfortunately.

Yes, I think that is a very good suggestion.  Thanks.

    $ git grep -c -e '\(die\|error\|warning\)("git-[^ ]*:' 34baebc -- '*.c'
    ho/dashless:builtin-apply.c:3
    ho/dashless:builtin-checkout-index.c:3
    ho/dashless:builtin-commit-tree.c:1
    ho/dashless:builtin-fetch-pack.c:1
    ho/dashless:builtin-grep.c:1
    ho/dashless:builtin-ls-files.c:3
    ho/dashless:builtin-rm.c:2
    ho/dashless:builtin-show-ref.c:3
    ho/dashless:builtin-tar-tree.c:2
    ho/dashless:builtin-update-index.c:8
    ho/dashless:connect.c:2
    ho/dashless:entry.c:10
    ho/dashless:merge-index.c:3
    ho/dashless:tree-diff.c:1
    ho/dashless:upload-pack.c:9

I've looked at the hits from the above command (without -c) and they all
looked good candidate, so I'll do that myself to reduce the amount of
patches that do need thinking and inspection.

Re: [PATCH] Start conforming code to "git subcmd" style

[Heikki Orsila]

On Sun, Aug 31, 2008 at 02:32:05AM -0700, Jakub Narebski wrote:
> Either "git-rev-list" or "'git rev-list'" is fine; "git rev-list"
> is not, as it requires careful reading to notice which part is
> proposed git command, and which the rest of message.

I would go for "'git rev-list'". I'll use that form in coming patches, 
unless there is resistance to this idea.

-- 
Heikki Orsila
heikki.orsila@iki.fi
http://www.iki.fi/shd