threads / patch / 15283

patchStart conforming code to "git subcmd" style

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

## tl;dr

6 messages between Aug 30, 2008 and Aug 31, 2008. Diffs are folded; open one to read it.

replies: 5people: 5as markdown or json

Heikki Orsila· Aug 30, 2008, 11:12 UTC · lore

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(-)
Show changes to 7 files +17 −17

builtin-apply.c, builtin-archive.c, builtin-blame.c, builtin-branch.c, builtin-bundle.c, builtin-cat-file.c, builtin-check-ref-format.c

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
Junio C Hamano· Aug 30, 2008, 20:49 UTC · re: Heikki Orsila · lore

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

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.

Show 13 quoted lines
> 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).

Show 10 quoted lines
> 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?
Show 6 quoted lines
> 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.
Christian Couder· Aug 31, 2008, 01:53 UTC · re: Junio C Hamano · lore

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

Le samedi 30 août 2008, Junio C Hamano a écrit :
Show 16 quoted lines
>
> 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?
Jakub Narebski· Aug 31, 2008, 09:32 UTC · re: Junio C Hamano · lore

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

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'.
...
Show 19 quoted lines
> > 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...).

Show 20 quoted lines
> > 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.

Show 16 quoted lines
> 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
Junio C Hamano· Aug 31, 2008, 16:14 UTC · re: Jakub Narebski · lore

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

Jakub Narebski <jnareb@gmail.com> writes:
Show 24 quoted lines
> 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.

Heikki Orsila· Aug 31, 2008, 16:37 UTC · re: Jakub Narebski · lore

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

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

← back to recent threads