git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Kaartic Sivaraam <kaartic.sivaraam@gmail.com>
To: Christian Couder <christian.couder@gmail.com>
Cc: git <git@vger.kernel.org>,
	"Shourya Shukla" <shouryashukla.oo@gmail.com>,
	"Johannes Schindelin" <Johannes.Schindelin@gmx.de>,
	"Christian Couder" <chriscool@tuxfamily.org>,
	"Đoàn Trần Công Danh" <congdanhqx@gmail.com>,
	"Junio C Hamano" <gitster@pobox.com>,
	"Denton Liu" <liu.denton@gmail.com>,
	"Eric Sunshine" <sunshine@sunshineco.com>
Subject: Re: [GSoC][PATCH v5] submodule: port subcommand 'set-branch' from shell to C
Date: Fri, 5 Jun 2020 00:56:20 +0530	[thread overview]
Message-ID: <92bad281-dd38-aef2-9910-659b41cdd830@gmail.com> (raw)
In-Reply-To: <CAP8UFD3Qe3iDe+ymKsqv9HarFLYDohXmUGbkNwZ4MdVQ=XP7yQ@mail.gmail.com>

On 03-06-2020 01:15, Christian Couder wrote:
> On Tue, Jun 2, 2020 at 9:01 PM Kaartic Sivaraam
> <kaartic.sivaraam@gmail.com> wrote:
>>
>> I'm having second thoughts about my suggestion[1] to include
>> the short option for '--quiet' in the usage. This is the only
>> usage in submodule--helper that mentions that '-q' is a short
>> hand for '--quiet'. That seems inconsistent. I see two ways but
>> I'm not sure which one of these would be better:
>>
>> A. Dropping the mention of '-q' in this usage thus making it consistent
>>     with the other usages printed by submodule--helper.
>>
>> B. Fixing other usages of submodule--helper to mention that '-q' is
>>     shorthand for quiet. This has the benefit of properly advertising
>>     the shorthand.
>>
>> C. Just ignore this?
> 
> The `git submodule` documentation has:
> 
> -q::
> --quiet::
>         Only print error messages.
> 
> even though the Synopsis is:
> 
> 'git submodule' [--quiet] [--cached]
> 'git submodule' [--quiet] add [<options>] [--] <repository> [<path>]
> 'git submodule' [--quiet] status [--cached] [--recursive] [--] [<path>...]
> ...
> 
> So I prefer B, and maybe updating the synopsis, as I think most Git
> commands have '-q' meaning '--quiet'.
> 

Makes sense.

> [...]
> 
>> So, according to this, I think the usage should be ...
>>
>>      git submodule--helper set-branch [-q | --quiet] [-d | --default] <path>
>>
>> ... and ...
>>
>>      git submodule--helper set-branch [-q|--quiet] [-b | --branch]<branch> <path>
>>
>> ... respectively.
> 
> I don't agree. I think `git submodule--helper set-branch ...` requires
> either "-d | --default" or "-b | --branch", while for example:
> 
> git submodule--helper set-branch [-q | --quiet] [-d | --default] <path>
> 
> would mean that "git submodule--helper set-branch my/path" is valid.
> 

You're right. Even I thought about the same thing when I came up with
that suggestion after quoting that portion of the CodingGuidelines. But
it was also curious for me to observe that the original used parenthesis
to mention the short and long options of an argument:

> +	const char *const usage[] = {
> +		N_("git submodule--helper set-branch [-q|--quiet] (-d|--default) <path>"),
> +		N_("git submodule--helper set-branch [-q|--quiet] (-b|--branch) <branch> <path>"),
> +		NULL
> +	};

I've not seen such a usage before. That's what actually made me take a
look at the CodingGuidelines for this. As the CodingGuidelined doesn't
seem to be mentioning anything about this explicitly, let's see if I
could find something in the usage printed by other commands.

---
> git am -h
usage: git am [<options>] [(<mbox> | <Maildir>)...]
   or: git am [<options>] (--continue | --skip | --abort)

   <options snipped>

> git branch -h
usage: git branch [<options>] [-r | -a] [--merged | --no-merged]
   or: git branch [<options>] [-l] [-f] <branch-name> [<start-point>]
   or: git branch [<options>] [-r] (-d | -D) <branch-name>...
   or: git branch [<options>] (-m | -M) [<old-branch>] <new-branch>
   or: git branch [<options>] (-c | -C) [<old-branch>] <new-branch>
   or: git branch [<options>] [-r | -a] [--points-at]
   or: git branch [<options>] [-r | -a] [--format]

   <options snipped>

> git checkout -h
usage: git checkout [<options>] <branch>
   or: git checkout [<options>] [<branch>] -- <file>...

   <options snipped>

> git switch -h
usage: git switch [<options>] [<branch>]

   <options snipped>

---
Hmm. Looks like it's not common for us to mention both the short
and long options in the usage itself. This might be to avoid the
redundancy as the usage is usually followed by the list of options.

With this info, I think we could've just gone with the following as the
usage strings for the `set-branch` subcommand:

    git submodule--helper set-branch [<options>] -d <path>
    git submodule--helper set-branch [<options>] -b <branch> <path>

This also solves the problem with `--quiet` I mentioned earlier while
making it concise and inline with the usages printed by other commands.

All this said, I don't think it's worth a re-roll now for several
reasons.

-- 
Sivaraam

      parent reply	other threads:[~2020-06-04 19:26 UTC|newest]

Thread overview: 29+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-05-21 16:38 [PATCH v3] submodule: port subcommand 'set-branch' from shell to C Shourya Shukla
2020-05-21 18:44 ` Junio C Hamano
2020-05-21 19:03   ` Denton Liu
2020-05-21 19:50     ` Junio C Hamano
2020-05-22 19:39       ` Shourya Shukla
2020-05-24 16:07         ` Junio C Hamano
2020-05-21 23:04 ` Đoàn Trần Công Danh
2020-05-22 22:21 ` Johannes Schindelin
2020-05-24 23:15   ` Junio C Hamano
2020-05-24 23:18     ` Junio C Hamano
2020-05-23 16:39 ` [PATCH v4] " Shourya Shukla
2020-05-23 18:49   ` Kaartic Sivaraam
2020-05-23 23:18     ` Đoàn Trần Công Danh
2020-05-27 17:13     ` Shourya Shukla
2020-05-28 12:21       ` Đoàn Trần Công Danh
2020-05-28 14:01         ` Đoàn Trần Công Danh
2020-05-28 15:55           ` Đoàn Trần Công Danh
2020-06-02 16:35   ` [GSoC][PATCH v5] " Shourya Shukla
2020-06-02 17:58     ` Junio C Hamano
2020-06-03  0:12       ` Đoàn Trần Công Danh
2020-06-03 20:02         ` Junio C Hamano
2020-06-04  7:17           ` Shourya Shukla
2020-06-04  7:49             ` Christian Couder
2020-06-04 15:03             ` Junio C Hamano
2020-06-02 19:01     ` Kaartic Sivaraam
2020-06-02 19:10       ` Kaartic Sivaraam
2020-06-02 19:45       ` Christian Couder
2020-06-04  7:09         ` Shourya Shukla
2020-06-04 19:26         ` Kaartic Sivaraam [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=92bad281-dd38-aef2-9910-659b41cdd830@gmail.com \
    --to=kaartic.sivaraam@gmail.com \
    --cc=Johannes.Schindelin@gmx.de \
    --cc=chriscool@tuxfamily.org \
    --cc=christian.couder@gmail.com \
    --cc=congdanhqx@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=liu.denton@gmail.com \
    --cc=shouryashukla.oo@gmail.com \
    --cc=sunshine@sunshineco.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).