Re: [PATCH] pull: add angle brackets to usage string

[Junio C Hamano]

Alex Henrie  writes:

> I pushed to change [options] to [] because even if the angle
> brackets don't help new users or translators in this particular case,
> the angle brackets encourage Git authors to use angle brackets when
> writing commands that are not so easy to understand. If you think that
> [...] is better because it is even more consistent, I would be
> happy to send a patch to make that change.

I do agree that [...] would make things consistent between
options and non-option arguments.  But I also would expect that
reasonably intelligent readers know that options are special, and
they would understand that [options] and [] mean the same
thing as [...] anyway, so I do not feel too strongly either
way myself (meaning: I wouldn't be motivated to prepare patches for
it myself, I wouldn't jump up and down saying they are wrong and
revert them if such patches were applied by an interim maintainer
while I am on vacation, and I would apply such patches myself only
if they do not overly interfere with topics in flight while
merging).

Thanks.







--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] pull: add angle brackets to usage string

[Alex Henrie]

2015-10-19 23:17 GMT-06:00 Junio C Hamano :
> Alex Henrie  writes:
>
>> 2015-10-16 11:42 GMT-06:00 Junio C Hamano :
>>>
>>> Yes, but that fixes historical "mistake", no?
>>>
>>> With this, you are breaking historical practice by changing only one
>>> instance to deviate from the then-current practice of saying
>>> 'options' without brackets.  It is based on the point of view that
>>> considers anything inside  and a fixed string 'options' are
>>> meant to be replaced by intelligent readers, which is as valid as
>>> the more recent practice to consider only things inside 
>>> are placeholders.
>>
>> OK, I see. You're saying that it's OK to fix typos and grammatical
>> errors in contrib/examples, but it's not okay to modernize the
>> scripts' designs.
>
> Please read it again, look at contrib/examples and realize that that
> is not what I said at all.
>
> This is not about modern vs old-school.  The reason why the part of
> the patch to contrib/ under discussion is wrong is because of
> (in)consistency.
>
> Look at the output from "git grep option contrib/examples/" and
> notice that in the old days, these scripted Porcelains consistently
> said "[options]" without "".
>
> It would have been a different matter if the patch _were_ to update
> all "[options]" to "[]" in contrib/examples/ consistently,
> and such a patch might have even been an improvement, especially if
> the modern style were clearly superiour than the old-school style
> (which is not, by they way [*1*]).
>
> But that is not what the patch did.  It turned only one of them into
> "[]", making the single instance inconsistent from all the
> others around it.  That is why it was wrong.

I understand now, thanks. I really appreciate your commitment to being
consistent.

> [Footnote]
>
> *1* The "modern" style is not necessarily an improvement, by the
> way.  The way we specify that a "thing" in the help text is a
> placeholder and that there may be more instances of the same
> "thing" is to say "[...]", but in your "modernized" form,
> unlike all the other usual "things", possibly multiple options
> are spelled "[]" without having ellipses at the end,
> which is an oddball.  If we are to treat options specially like
> that anyway, intelligent readers can read an "old-school"
> description "[options]" and understand that that token stands
> for possibly multiple options just fine, and all we have gained
> by going to the "modernized" form is to waste two characters for
> .
>
> I am not saying that we should not apply the other half of the
> patch that makes builtin/pull.c say "[]".  These days,
> many other commands nearby (i.e. the "modern" ones) do use that
> form consistently, so it is an improvement.

I pushed to change [options] to [] because even if the angle
brackets don't help new users or translators in this particular case,
the angle brackets encourage Git authors to use angle brackets when
writing commands that are not so easy to understand. If you think that
[...] is better because it is even more consistent, I would be
happy to send a patch to make that change.

Anyway, thanks again for your attention to detail.

-Alex
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] pull: add angle brackets to usage string

[Alex Henrie]

2015-10-16 11:42 GMT-06:00 Junio C Hamano :
> Alex Henrie  writes:
>
>> 2015-10-16 10:36 GMT-06:00 Junio C Hamano :
>>> Makes sense, as all the other  in the usage string are
>>> bracketted.
>>>
>>> Does it make sense to do this for contrib/examples, which is the
>>> historical record, though?
>>
>> I didn't know that contrib/examples was a historical record. The last
>> patch I submitted, b7447679e84ed973430ab19fce87f56857b83068, also
>> modified contrib/examples.
>
> Yes, but that fixes historical "mistake", no?
>
> With this, you are breaking historical practice by changing only one
> instance to deviate from the then-current practice of saying
> 'options' without brackets.  It is based on the point of view that
> considers anything inside  and a fixed string 'options' are
> meant to be replaced by intelligent readers, which is as valid as
> the more recent practice to consider only things inside 
> are placeholders.

OK, I see. You're saying that it's OK to fix typos and grammatical
errors in contrib/examples, but it's not okay to modernize the
scripts' designs. That's fine; standardizing placeholder syntax is
primarily for the benefit of translators, and contrib/ is not
translated, so it's not causing a problem.

-Alex
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] pull: add angle brackets to usage string

[Junio C Hamano]

Alex Henrie  writes:

> 2015-10-16 11:42 GMT-06:00 Junio C Hamano :
>>
>> Yes, but that fixes historical "mistake", no?
>>
>> With this, you are breaking historical practice by changing only one
>> instance to deviate from the then-current practice of saying
>> 'options' without brackets.  It is based on the point of view that
>> considers anything inside  and a fixed string 'options' are
>> meant to be replaced by intelligent readers, which is as valid as
>> the more recent practice to consider only things inside 
>> are placeholders.
>
> OK, I see. You're saying that it's OK to fix typos and grammatical
> errors in contrib/examples, but it's not okay to modernize the
> scripts' designs.

Please read it again, look at contrib/examples and realize that that
is not what I said at all.

This is not about modern vs old-school.  The reason why the part of
the patch to contrib/ under discussion is wrong is because of
(in)consistency.

Look at the output from "git grep option contrib/examples/" and
notice that in the old days, these scripted Porcelains consistently
said "[options]" without "".

It would have been a different matter if the patch _were_ to update
all "[options]" to "[]" in contrib/examples/ consistently,
and such a patch might have even been an improvement, especially if
the modern style were clearly superiour than the old-school style
(which is not, by they way [*1*]).

But that is not what the patch did.  It turned only one of them into
"[]", making the single instance inconsistent from all the
others around it.  That is why it was wrong.


[Footnote]

*1* The "modern" style is not necessarily an improvement, by the
way.  The way we specify that a "thing" in the help text is a
placeholder and that there may be more instances of the same
"thing" is to say "[...]", but in your "modernized" form,
unlike all the other usual "things", possibly multiple options
are spelled "[]" without having ellipses at the end,
which is an oddball.  If we are to treat options specially like
that anyway, intelligent readers can read an "old-school"
description "[options]" and understand that that token stands
for possibly multiple options just fine, and all we have gained
by going to the "modernized" form is to waste two characters for
.

I am not saying that we should not apply the other half of the
patch that makes builtin/pull.c say "[]".  These days,
many other commands nearby (i.e. the "modern" ones) do use that
form consistently, so it is an improvement.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] pull: add angle brackets to usage string

[Ralf Thielow]

Alex Henrie  wrote:
> ---
>  builtin/pull.c   | 2 +-
>  contrib/examples/git-pull.sh | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/builtin/pull.c b/builtin/pull.c
> index a39bb0a..bf3fd3f 100644
> --- a/builtin/pull.c
> +++ b/builtin/pull.c
> @@ -66,7 +66,7 @@ static int parse_opt_rebase(const struct option *opt, const 
> char *arg, int unset
>  }
>  
>  static const char * const pull_usage[] = {
> - N_("git pull [options] [ [...]]"),
> + N_("git pull [] [ [...]]"),
>   NULL
>  };


There seem to be three more places left missing these angle brances
at the usage string.
Junio, feel free to squash this or treat it as a separate patch
on top, if suitable.

-- >8 --
From: Ralf Thielow 
Date: Fri, 16 Oct 2015 19:09:57 +0200
Subject: [PATCH] am, credential-cache: add angle brackets to usage string

Signed-off-by: Ralf Thielow 
---
 builtin/am.c   | 4 ++--
 credential-cache.c | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/builtin/am.c b/builtin/am.c
index 4f77e07..98992cd 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -2218,8 +2218,8 @@ int cmd_am(int argc, const char **argv, const char 
*prefix)
int in_progress;
 
const char * const usage[] = {
-   N_("git am [options] [(|)...]"),
-   N_("git am [options] (--continue | --skip | --abort)"),
+   N_("git am [] [(|)...]"),
+   N_("git am [] (--continue | --skip | --abort)"),
NULL
};
 
diff --git a/credential-cache.c b/credential-cache.c
index 8689a15..f4afdc6 100644
--- a/credential-cache.c
+++ b/credential-cache.c
@@ -88,7 +88,7 @@ int main(int argc, const char **argv)
int timeout = 900;
const char *op;
const char * const usage[] = {
-   "git credential-cache [options] ",
+   "git credential-cache [] ",
NULL
};
struct option options[] = {
-- 
2.6.1.339.g81d1034


--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] pull: add angle brackets to usage string

[Junio C Hamano]

Alex Henrie  writes:

> 2015-10-16 10:36 GMT-06:00 Junio C Hamano :
>> Makes sense, as all the other  in the usage string are
>> bracketted.
>>
>> Does it make sense to do this for contrib/examples, which is the
>> historical record, though?
>
> I didn't know that contrib/examples was a historical record. The last
> patch I submitted, b7447679e84ed973430ab19fce87f56857b83068, also
> modified contrib/examples.

Yes, but that fixes historical "mistake", no?

With this, you are breaking historical practice by changing only one
instance to deviate from the then-current practice of saying
'options' without brackets.  It is based on the point of view that
considers anything inside  and a fixed string 'options' are
meant to be replaced by intelligent readers, which is as valid as
the more recent practice to consider only things inside 
are placeholders.

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] pull: add angle brackets to usage string

[Junio C Hamano]

Ralf Thielow  writes:

> There seem to be three more places left missing these angle brances
> at the usage string.
> Junio, feel free to squash this or treat it as a separate patch
> on top, if suitable.

Thanks. Will queue.

>
> -- >8 --
> From: Ralf Thielow 
> Date: Fri, 16 Oct 2015 19:09:57 +0200
> Subject: [PATCH] am, credential-cache: add angle brackets to usage string
>
> Signed-off-by: Ralf Thielow 
> ---
>  builtin/am.c   | 4 ++--
>  credential-cache.c | 2 +-
>  2 files changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/builtin/am.c b/builtin/am.c
> index 4f77e07..98992cd 100644
> --- a/builtin/am.c
> +++ b/builtin/am.c
> @@ -2218,8 +2218,8 @@ int cmd_am(int argc, const char **argv, const char 
> *prefix)
>   int in_progress;
>  
>   const char * const usage[] = {
> - N_("git am [options] [(|)...]"),
> - N_("git am [options] (--continue | --skip | --abort)"),
> + N_("git am [] [(|)...]"),
> + N_("git am [] (--continue | --skip | --abort)"),
>   NULL
>   };
>  
> diff --git a/credential-cache.c b/credential-cache.c
> index 8689a15..f4afdc6 100644
> --- a/credential-cache.c
> +++ b/credential-cache.c
> @@ -88,7 +88,7 @@ int main(int argc, const char **argv)
>   int timeout = 900;
>   const char *op;
>   const char * const usage[] = {
> - "git credential-cache [options] ",
> + "git credential-cache [] ",
>   NULL
>   };
>   struct option options[] = {
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] pull: add angle brackets to usage string

[Junio C Hamano]

Alex Henrie  writes:

> Signed-off-by: Alex Henrie 
> ---

Makes sense, as all the other  in the usage string are
bracketted.

Does it make sense to do this for contrib/examples, which is the
historical record, though?  The first one I found with

$ less contrib/examples/*

was this:

#!/bin/sh

OPTIONS_KEEPDASHDASH=t
OPTIONS_SPEC="\
git-checkout [options] [] [...]

and the next one (clean) follows the same pattern.

I'd discard the part of the patch for contrib/ and queue.

Thanks.

>  builtin/pull.c   | 2 +-
>  contrib/examples/git-pull.sh | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/builtin/pull.c b/builtin/pull.c
> index a39bb0a..bf3fd3f 100644
> --- a/builtin/pull.c
> +++ b/builtin/pull.c
> @@ -66,7 +66,7 @@ static int parse_opt_rebase(const struct option *opt, const 
> char *arg, int unset
>  }
>  
>  static const char * const pull_usage[] = {
> - N_("git pull [options] [ [...]]"),
> + N_("git pull [] [ [...]]"),
>   NULL
>  };
>  
> diff --git a/contrib/examples/git-pull.sh b/contrib/examples/git-pull.sh
> index 6b3a03f..bcf362e 100755
> --- a/contrib/examples/git-pull.sh
> +++ b/contrib/examples/git-pull.sh
> @@ -8,7 +8,7 @@ SUBDIRECTORY_OK=Yes
>  OPTIONS_KEEPDASHDASH=
>  OPTIONS_STUCKLONG=Yes
>  OPTIONS_SPEC="\
> -git pull [options] [ [...]]
> +git pull [] [ [...]]
>  
>  Fetch one or more remote refs and integrate it/them with the current HEAD.
>  --
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] pull: add angle brackets to usage string

[Alex Henrie]

2015-10-16 10:36 GMT-06:00 Junio C Hamano :
> Makes sense, as all the other  in the usage string are
> bracketted.
>
> Does it make sense to do this for contrib/examples, which is the
> historical record, though?

I didn't know that contrib/examples was a historical record. The last
patch I submitted, b7447679e84ed973430ab19fce87f56857b83068, also
modified contrib/examples.

-Alex
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] pull: add angle brackets to usage string

[Alex Henrie]

Signed-off-by: Alex Henrie 
---
 builtin/pull.c   | 2 +-
 contrib/examples/git-pull.sh | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/builtin/pull.c b/builtin/pull.c
index a39bb0a..bf3fd3f 100644
--- a/builtin/pull.c
+++ b/builtin/pull.c
@@ -66,7 +66,7 @@ static int parse_opt_rebase(const struct option *opt, const 
char *arg, int unset
 }
 
 static const char * const pull_usage[] = {
-   N_("git pull [options] [ [...]]"),
+   N_("git pull [] [ [...]]"),
NULL
 };
 
diff --git a/contrib/examples/git-pull.sh b/contrib/examples/git-pull.sh
index 6b3a03f..bcf362e 100755
--- a/contrib/examples/git-pull.sh
+++ b/contrib/examples/git-pull.sh
@@ -8,7 +8,7 @@ SUBDIRECTORY_OK=Yes
 OPTIONS_KEEPDASHDASH=
 OPTIONS_STUCKLONG=Yes
 OPTIONS_SPEC="\
-git pull [options] [ [...]]
+git pull [] [ [...]]
 
 Fetch one or more remote refs and integrate it/them with the current HEAD.
 --
-- 
2.6.1

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html