| Message ID | 20210924024606.20542-4-tbperrotta@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | send-email: shell completion improvements | expand |
On 24/09/21 09.46, Thiago Perrotta wrote: > SYNOPSIS > -------- > [verse] > -'git send-email' [<options>] <file|directory|rev-list options>... > +'git send-email' [<options>] <file|directory>... > +'git send-email' [<options>] <format-patch options> > 'git send-email' --dump-aliases Is <format-patch options> optional? If so, we can say [<format-patch options>].
On Thu, Sep 23, 2021 at 9:36 PM Bagas Sanjaya <bagasdotme@gmail.com> wrote: > > On 24/09/21 09.46, Thiago Perrotta wrote: > > SYNOPSIS > > -------- > > [verse] > > -'git send-email' [<options>] <file|directory|rev-list options>... > > +'git send-email' [<options>] <file|directory>... > > +'git send-email' [<options>] <format-patch options> > > 'git send-email' --dump-aliases > > Is <format-patch options> optional? If so, we can say [<format-patch > options>]. no; as Junio explained [1] this omission is intentional while the rev-list options that got cut to make space are not and are more relevant. IMHO leaving [<options>] to imply ALL options (that also include diff options, for example) is better Carlo [1] https://lore.kernel.org/git/xmqqh7fjuaar.fsf@gitster.g/
On 24/09/21 11.53, Carlo Arenas wrote: > no; as Junio explained [1] this omission is intentional while the > rev-list options that > got cut to make space are not and are more relevant. > > IMHO leaving [<options>] to imply ALL options (that also include diff > options, for example) is better Quoting what you linked: > The program works in two majorly different modes. It either takes > > * message files that are already proof-read and copy-edited from > the filesystem and sends them out, or > > * format-patch options to generate message files out of the commits > and send them out without any proofreading. I think we need to again modify SYNOPSIS and DESCRIPTIONS to highlight above fact: ---- 8> ---- diff --git a/Documentation/git-send-email.txt b/Documentation/git-send-email.txt index 3db4eab4ba..6002ca1a02 100644 --- a/Documentation/git-send-email.txt +++ b/Documentation/git-send-email.txt @@ -9,17 +9,19 @@ git-send-email - Send a collection of patches as emails SYNOPSIS -------- [verse] -'git send-email' [<options>] <file|directory|rev-list options>... +'git send-email' [<options>] <file|directory>... +'git send-email' [<options>] <revision list>... 'git send-email' --dump-aliases DESCRIPTION ----------- -Takes the patches given on the command line and emails them out. -Patches can be specified as files, directories (which will send all -files in the directory), or directly as a revision list. In the -last case, any format accepted by linkgit:git-format-patch[1] can -be passed to git send-email. +In the first form, take the patches in <file> or <directory> and +emails them out. In the second form, generate patches from specified +<revision list> (it can be revision range or explicit list of +revisions from linkgit:git-rev-list[1]), then emails them out. +<options> can also include options understood by +linkgit:git-format-patch[1] if the second form is specified. The header of the email is configurable via command-line options. If not specified on the command line, the user will be prompted with a ReadLine
On Thu, Sep 23, 2021 at 11:19 PM Bagas Sanjaya <bagasdotme@gmail.com> wrote: > > On 24/09/21 11.53, Carlo Arenas wrote: > > no; as Junio explained [1] this omission is intentional while the > > rev-list options that > > got cut to make space are not and are more relevant. > > > > IMHO leaving [<options>] to imply ALL options (that also include diff > > options, for example) is better > > Quoting what you linked: > > > The program works in two majorly different modes. It either takes > > > > * message files that are already proof-read and copy-edited from > > the filesystem and sends them out, or > > > > * format-patch options to generate message files out of the commits > > and send them out without any proofreading. > > I think we need to again modify SYNOPSIS and DESCRIPTIONS to highlight > above fact: Bagas, This is much better; can you SOB and help Thiago import it into his tree so it can be included in the next reroll? Thiago, Let's wait a little while to collect more feedback on patch2 before sending it, though Carlo
Carlo Arenas <carenas@gmail.com> writes: > On Thu, Sep 23, 2021 at 9:36 PM Bagas Sanjaya <bagasdotme@gmail.com> wrote: >> >> On 24/09/21 09.46, Thiago Perrotta wrote: >> > SYNOPSIS >> > -------- >> > [verse] >> > -'git send-email' [<options>] <file|directory|rev-list options>... >> > +'git send-email' [<options>] <file|directory>... >> > +'git send-email' [<options>] <format-patch options> >> > 'git send-email' --dump-aliases >> >> Is <format-patch options> optional? If so, we can say [<format-patch >> options>]. > > no; as Junio explained [1] this omission is intentional while the > rev-list options that > got cut to make space are not and are more relevant. > > IMHO leaving [<options>] to imply ALL options (that also include diff > options, for example) is better Could you claify this idea a bit more? Do you mean that the second form can just be: git send-email <format-patch options> That will exclude the send-email specific ones (like "--in-reply-to=<msg>"), so it may not be a great idea. Or do you mean git send-email <options> and have the <options> placeholder to stand for both send-email options and options meant for format-patch? Thanks.
On Fri, Sep 24, 2021 at 8:33 AM Junio C Hamano <gitster@pobox.com> wrote: > > Carlo Arenas <carenas@gmail.com> writes: > > > On Thu, Sep 23, 2021 at 9:36 PM Bagas Sanjaya <bagasdotme@gmail.com> wrote: > >> > >> On 24/09/21 09.46, Thiago Perrotta wrote: > >> > SYNOPSIS > >> > -------- > >> > [verse] > >> > -'git send-email' [<options>] <file|directory|rev-list options>... > >> > +'git send-email' [<options>] <file|directory>... > >> > +'git send-email' [<options>] <format-patch options> > >> > 'git send-email' --dump-aliases > >> > >> Is <format-patch options> optional? If so, we can say [<format-patch > >> options>]. > > > > no; as Junio explained [1] this omission is intentional while the > > rev-list options that > > got cut to make space are not and are more relevant. > > > > IMHO leaving [<options>] to imply ALL options (that also include diff > > options, for example) is better > > Could you claify this idea a bit more? Do you mean that the second form > can just be: > > git send-email <format-patch options> > > That will exclude the send-email specific ones (like > "--in-reply-to=<msg>"), so it may not be a great idea. > > Or do you mean > > git send-email <options> > > and have the <options> placeholder to stand for both send-email > options and options meant for format-patch? the later AND including a non optional part that explains that you need to indicate some sort of revision (which hopefully will also imply to users that all the related options are also expected), but also won't be specific, not to promote the use of this type of mode or the need to update it further to include the whole universe of options through format-patch (ex: log and diff) Granted, I could have worded it better, but Bagas' proposal[1] (based on this discussion) does and clarifies both; why this is not optional in a way that is less confusing than what Thiago posted and is IMHO[2] a good candidate for replacing this patch in the series Carlo [1] https://lore.kernel.org/git/20210924121352.42138-1-bagasdotme@gmail.com [2] https://lore.kernel.org/git/CAPUEsphn15H9HbHKHRk+GFMvjq5O=8NL0Vo4L8NoUwiRrQUJJA@mail.gmail.com/T/#m6f0600b597fbe0b59aa767603a7f1d24d60e8fe9
Carlo Arenas <carenas@gmail.com> writes: >> Or do you mean >> >> git send-email <options> >> >> and have the <options> placeholder to stand for both send-email >> options and options meant for format-patch? > > the later AND including a non optional part that explains that you > need to indicate some sort of revision ... Ah, thanks for explanation. git format-patch -2 would be options-only way to "indicate some sort of revision", so perhaps . git send-email <send-email options> files|directory . git send-email <send-email options> <format-patch options> (where "options" is used to refer to both --options and arguments) would illustrate the differences better?
On 25/09/21 03.03, Junio C Hamano wrote: > Ah, thanks for explanation. > > git format-patch -2 > > would be options-only way to "indicate some sort of revision", so > perhaps > > . git send-email <send-email options> files|directory > . git send-email <send-email options> <format-patch options> > > (where "options" is used to refer to both --options and arguments) > would illustrate the differences better? > But we can also directly specify revision range (commonly <common ancestor>..HEAD or HEAD ^<common ancestor>). So for the second form, the syntax will be: git send-email <send-email options> <format-patch options> <revision range>
Bagas Sanjaya <bagasdotme@gmail.com> writes: > On 25/09/21 03.03, Junio C Hamano wrote: >> Ah, thanks for explanation. >> git format-patch -2 >> would be options-only way to "indicate some sort of revision", so >> perhaps >> . git send-email <send-email options> files|directory >> . git send-email <send-email options> <format-patch options> >> (where "options" is used to refer to both --options and arguments) >> would illustrate the differences better? >> > > But we can also directly specify revision range (commonly <common > ancestor>..HEAD or HEAD ^<common ancestor>). That is exactly why I have the parenthetical definition of what "options" mean in my explanation. git format-patch -2 git format-patch master git format-patch master..HEAD Everything after "git format-patch", i.e. -2, master, master..HEAD, are usable, and there isn't much point in singling out revision ranges. FWIW, I think you can even give "-- <pathspec>" at the end, which are not options or revision ranges.
On Fri, Sep 24, 2021 at 09:07:35PM -0700, Junio C Hamano wrote: > Bagas Sanjaya <bagasdotme@gmail.com> writes: > > On 25/09/21 03.03, Junio C Hamano wrote: > >> Ah, thanks for explanation. > >> git format-patch -2 > >> would be options-only way to "indicate some sort of revision", so > >> perhaps > >> . git send-email <send-email options> files|directory > >> . git send-email <send-email options> <format-patch options> > >> (where "options" is used to refer to both --options and arguments) > >> would illustrate the differences better? > >> > > But we can also directly specify revision range (commonly <common > > ancestor>..HEAD or HEAD ^<common ancestor>). > > That is exactly why I have the parenthetical definition of what > "options" mean in my explanation. > > git format-patch -2 > git format-patch master > git format-patch master..HEAD > > Everything after "git format-patch", i.e. -2, master, master..HEAD, > are usable, and there isn't much point in singling out revision > ranges. FWIW, I think you can even give "-- <pathspec>" at the end, > which are not options or revision ranges. <format-patch options> then it is; would the following be worth adding in top so the recursive reference can be followed? Carlo ------ >8 ------ diff --git a/Documentation/git-format-patch.txt b/Documentation/git-format-patch.txt index fe2f69d36e..806ff93259 100644 --- a/Documentation/git-format-patch.txt +++ b/Documentation/git-format-patch.txt @@ -30,7 +30,7 @@ SYNOPSIS [--range-diff=<previous> [--creation-factor=<percent>]] [--filename-max-length=<n>] [--progress] - [<common diff options>] + [<common diff options>] [<rev-list options>] [ <since> | <revision range> ] DESCRIPTION
Carlo Marcelo Arenas Belón <carenas@gmail.com> writes: >> Everything after "git format-patch", i.e. -2, master, master..HEAD, >> are usable, and there isn't much point in singling out revision >> ranges. FWIW, I think you can even give "-- <pathspec>" at the end, >> which are not options or revision ranges. > > <format-patch options> then it is; would the following be worth adding > in top so the recursive reference can be followed? I am not sure what "the recursive reference" is an issue here, but I agree that we may want to improve upon <revision range> in the part you are touching, which currently we say: There are two ways to specify which commits to operate on. 1. A single commit, <since>, specifies that the commits leading to the tip of the current branch that are not in the history that leads to the <since> to be output. 2. Generic <revision range> expression (see "SPECIFYING REVISIONS" section in linkgit:gitrevisions[7]) means the commits in the specified range. The first rule takes precedence in the case of a single <commit>. To apply the second rule, i.e., format everything since the beginning of history up until <commit>, use the `--root` option: `git format-patch --root <commit>`. If you want to format only <commit> itself, you can do this with `git format-patch -1 <commit>`. What we refer to in the prose, e.g. "--root" and " -1", do not appear in the SYNOPSIS section. > diff --git a/Documentation/git-format-patch.txt b/Documentation/git-format-patch.txt > index fe2f69d36e..806ff93259 100644 > --- a/Documentation/git-format-patch.txt > +++ b/Documentation/git-format-patch.txt > @@ -30,7 +30,7 @@ SYNOPSIS > [--range-diff=<previous> [--creation-factor=<percent>]] > [--filename-max-length=<n>] > [--progress] > - [<common diff options>] > + [<common diff options>] [<rev-list options>] > [ <since> | <revision range> ] I think the "<rev-list options>" you are adding here is to enhance what <revision range> in the original wants to convey. In addition to things like @{u}..HEAD~2 (i.e. "the branch is mostly good, but the tip 2 commits are not yet ready so do not send them out"), you can do "-2" (i.e. "the topmost 2 commits"), which is not exactly what "SPECIFYING REVISIONS" part of gitrevisions(7) describes. So, yes, I like the spirit of the change, but no, I do not think it goes there; rather, it would replace or extend <revision range>, I would think. In addition, "Generic <revision range> expression (see "SPECIFYING REVISIONS" section...) may need to be updated. First, what we'd want to refer to is not ways to specify revisions, but ways to specify a range. IOW, it should be referring to "SPECIFYING RANGES" section instead. If we replace <revision range> with your <rev-list options> in the SYNOPSIS, that will fall out as a natural consequence. Perhaps, the second description and an earlier part of the explanation can be rewritten like so: 2. <rev-list options> that specifies a range of commits (see linkgit:git-rev-list[1]) to be shown. If you give a single <commit> and nothing else, it is taken as the <since> of the first form. If you want to format everything since the beginning of history up until <commit>, use ... Thanks.
diff --git a/Documentation/git-send-email.txt b/Documentation/git-send-email.txt index 3db4eab4ba..41cd8cb424 100644 --- a/Documentation/git-send-email.txt +++ b/Documentation/git-send-email.txt @@ -9,7 +9,8 @@ git-send-email - Send a collection of patches as emails SYNOPSIS -------- [verse] -'git send-email' [<options>] <file|directory|rev-list options>... +'git send-email' [<options>] <file|directory>... +'git send-email' [<options>] <format-patch options> 'git send-email' --dump-aliases @@ -19,7 +20,8 @@ Takes the patches given on the command line and emails them out. Patches can be specified as files, directories (which will send all files in the directory), or directly as a revision list. In the last case, any format accepted by linkgit:git-format-patch[1] can -be passed to git send-email. +be passed to git send-email, as well as options understood by +linkgit:git-format-patch[1]. The header of the email is configurable via command-line options. If not specified on the command line, the user will be prompted with a ReadLine
git-send-email(1) does not mention that "git format-patch" options are accepted. Augment SYNOPSIS and DESCRIPTION to mention it. Signed-off-by: Thiago Perrotta <tbperrotta@gmail.com> --- Documentation/git-send-email.txt | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)