Re: [PATCH v7 1/5] command-list.txt: prepare with [commands] header

From: Eric Sunshine <sunshine@sunshineco.com>
Date: May 15, 2015, 21:15 UTC
Message-ID: <CAPig+cQ3mAAAVUZ=8mMb_siZ20Nw1J-LgkTkF_1FzttKk334NQ@mail.gmail.com>
In-Reply-To: <1431714904-16599-2-git-send-email-sebastien.guimmara@gmail.com>

On Fri, May 15, 2015 at 2:35 PM, Sébastien Guimmara <sebastien.guimmara@gmail.com> wrote:

Show 7 quoted lines

> command-list.txt: prepare with [commands] header
>
> Add a [commands] header before the actual command list, then make the
> following files ignore this header in their parsing:
>
>         * cmd-list.perl
>         * Makefile (check-docks target)

When these patches are ultimately applied, they won't have the benefit of the cover letter to explain the purpose of this change and, unfortunately, this commit message is lacking in that respect. The commit message should explain the reason you're making this change ("prepare machinery to deal with upcoming new common group specification" or such). With a proper explanation, it's not necessary to state (as you do above) precisely which bits of machinery you are changing. The patch itself already does that well enough.

More below.

Show 15 quoted lines

> Helped-by: Eric Sunshine <sunshine@sunshineco.com>
> Signed-off-by: Sébastien Guimmara <sebastien.guimmara@gmail.com>
> ---
> diff --git a/Documentation/howto/new-command.txt b/Documentation/howto/new-command.txt
> index d7de5a3..6d772bd 100644
> --- a/Documentation/howto/new-command.txt
> +++ b/Documentation/howto/new-command.txt
> @@ -95,7 +95,9 @@ your language, document it in the INSTALL file.
>  that categorizes commands by type, so they can be listed in appropriate
>  subsections in the documentation's summary command list.  Add an entry
>  for yours.  To understand the categories, look at git-commands.txt
> -in the main directory.
> +in the main directory.  If the new command is part of the typical Git
> +workflow and you believe it common enough to be mentioned in 'git help',
> +map this command to a common group in the column [common].

I think you meant to fold this change into patch 2/5 where the [common] block is actually added. It doesn't make much sense to talk about it here before exists.

Show 12 quoted lines

>  7. Give the maintainer one paragraph to include in the RelNotes file
>  to describe the new feature; a good place to do so is in the cover
> diff --git a/Makefile b/Makefile
> index 25a453b..0cb2045 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -2454,7 +2454,7 @@ check-docs::
>                 esac ; \
>                 test -f "Documentation/$$v.txt" || \
>                 echo "no doc: $$v"; \
> -               sed -e '/^#/d' command-list.txt | \
> +               sed -e '1,/^\[commands\]/d' -e '/^#/d' <command-list.txt | \

This is partly my fault since my original suggestion for how to skip [common] and [commands] sections[1] was typed on-the-spot when composing the mail message, without the context of looking at this Makefile, but changing this invocation to use redirection (<command-list.txt) rather than a simple argument (command-list.txt) seems unwarranted. Worse, it may confuse readers into thinking that there is some important but obscure reason for the change which they cannot fathom. Also, the invocation in the hunk just below this one does not bother with redirection, so use of redirection here seems doubly mysterious.

[1]: http://article.gmane.org/gmane.comp.version-control.git/268598

Show 8 quoted lines

>                 grep -q "^$$v[  ]" || \
>                 case "$$v" in \
>                 git) ;; \
> @@ -2463,6 +2463,7 @@ check-docs::
>         done; \
>         ( \
>                 sed -e '/^#/d' \
> +                   -e '1,/^\[commands\]/d' \

This won't work as expected. If you invoke 'check-docs', you'll see this (undesired) output:

    % make check-docs
    removed but listed: [commands]
    removed but listed: [common]
    removed but listed: history
    removed but listed: info
    removed but listed: init
    removed but listed: remote
    removed but listed: worktree
    %

which is the two [headers] you added plus all the new common group tags. To fix, re-order the sed expressions so that 1,/^\[commands\]/d comes first:

    sed -e '1,/^\[commands\]/d' \
        -e '/^#/d' \
        ...

Show 16 quoted lines

>                     -e 's/[     ].*//' \
>                     -e 's/^/listed /' command-list.txt; \
>                 $(MAKE) -C Documentation print-man1 | \
> diff --git a/command-list.txt b/command-list.txt
> index 54d8d21..caed872 100644
> --- a/command-list.txt
> +++ b/command-list.txt
> @@ -1,5 +1,6 @@
>  # List of known git commands.
>  # command name                          category [deprecated] [common]
> +[commands]
>  git-add                                 mainporcelain common
>  git-am                                  mainporcelain
>  git-annotate                            ancillaryinterrogators
> --
> 2.4.0