Re: [PATCH v2 2/2 / RFC] branch: quote branch/ref names to improve readability

[Stefan Beller]

On Tue, Aug 8, 2017 at 12:43 PM, Junio C Hamano  wrote:
> Stefan Beller  writes:
>
>> (Though wondering for non-submodule users, if they perceive it as
>> inconsistency as other parts of the code may not follow the rigorous quoting)
>
> Do you mean that we may instead want to remove the excessive quoting
> of branch names and stuff from submodule.c code, because they are
> newer ones that broke the consistency existed before them (i.e. not
> quoting)?

No, I do not. I was just wondering if a non-submodule user
may see differences between different commands now.

For example "checkout -b" already quotes 'external data', which
would be inline with this proposal, but there may be others.
My question was rather an encouragement to check the code base
if there are other occurrences left that do not quote.

In an ideal code base we could just grep for any %s that has no
surrounding quotes, but of course it is not as easy in the real world:
* some outputs use %s construction for non-human consumption
  in e.g. the diff machinery
* sometimes we play sentence lego, stringing words together
  which also is using %s unquoted correctly.

> That certainly is tempting, but I personally find it easier to read
> a message that marks parts that holds "external data" differently
> from the message's text, so I think this patch 2/2 goes in the right
> direction.

Yes. I like the direction this patch is going.

A note on 'external data':
For branchs, paths, submodule names a single quote seems
to be best (my opinion), whereas in e.g. git-status:

Submodule 'sm1' 000...1beffeb (new submodule)

parens seem to do a better job as they describe the state,
not reproducing external data. (This is also the place
where I was reminded of potential sentence lego)

Re: [PATCH v2 2/2 / RFC] branch: quote branch/ref names to improve readability

[Junio C Hamano]

Stefan Beller  writes:

> (Though wondering for non-submodule users, if they perceive it as
> inconsistency as other parts of the code may not follow the rigorous quoting)

Do you mean that we may instead want to remove the excessive quoting
of branch names and stuff from submodule.c code, because they are
newer ones that broke the consistency existed before them (i.e. not
quoting)?

That certainly is tempting, but I personally find it easier to read
a message that marks parts that holds "external data" differently
from the message's text, so I think this patch 2/2 goes in the right
direction.

Re: [PATCH v2 2/2 / RFC] branch: quote branch/ref names to improve readability

[Stefan Beller]

On Tue, Aug 8, 2017 at 10:11 AM, Kaartic Sivaraam
 wrote:
> Signed-off-by: Kaartic Sivaraam 
> ---
>  branch.c | 16 
>  1 file changed, 8 insertions(+), 8 deletions(-)

I like this patch.

In submodule.c we quote a lot of things (branches, submodules, paths), so
this is another step to make the output as a whole more consistent.
(Though wondering for non-submodule users, if they perceive it as
inconsistency as other parts of the code may not follow the rigorous quoting)

>
> diff --git a/branch.c b/branch.c
> index ad5a2299b..a40721f3c 100644
> --- a/branch.c
> +++ b/branch.c
> @@ -90,24 +90,24 @@ int install_branch_config(int flag, const char *local, 
> const char *origin, const
> if (shortname) {
> if (origin)
> printf_ln(rebasing ?
> - _("Branch %s set up to track remote 
> branch %s from %s by rebasing.") :
> - _("Branch %s set up to track remote 
> branch %s from %s."),
> + _("Branch '%s' set up to track 
> remote branch '%s' from '%s' by rebasing.") :
> + _("Branch '%s' set up to track 
> remote branch '%s' from '%s'."),
>   local, shortname, origin);
> else
> printf_ln(rebasing ?
> - _("Branch %s set up to track local 
> branch %s by rebasing.") :
> - _("Branch %s set up to track local 
> branch %s."),
> + _("Branch '%s' set up to track 
> local branch '%s' by rebasing.") :
> + _("Branch '%s' set up to track 
> local branch '%s'."),
>   local, shortname);
> } else {
> if (origin)
> printf_ln(rebasing ?
> - _("Branch %s set up to track remote 
> ref %s by rebasing.") :
> - _("Branch %s set up to track remote 
> ref %s."),
> + _("Branch '%s' set up to track 
> remote ref '%s' by rebasing.") :
> + _("Branch '%s' set up to track 
> remote ref '%s'."),
>   local, remote);
> else
> printf_ln(rebasing ?
> - _("Branch %s set up to track local 
> ref %s by rebasing.") :
> - _("Branch %s set up to track local 
> ref %s."),
> + _("Branch '%s' set up to track 
> local ref '%s' by rebasing.") :
> + _("Branch '%s' set up to track 
> local ref '%s'."),
>   local, remote);
> }
> }
> --
> 2.14.0.rc1.434.g6eded367a
>