From: Eric Sunshine <sunshine@sunshineco.com>
Date: Mon, 11 May 2015 06:44:26 GMT
Subject: Re: [PATCH v5 4/6] Makefile: update to new command-list.txt format
Message-ID: <CAPig+cQBUiniQ_A2BZGx-R4kxabE5hPcrbcoPXbq0kJS6oP+uw@mail.gmail.com>
In-Reply-To: <1431191856-10949-5-git-send-email-sebastien.guimmara@gmail.com>

On Sat, May 9, 2015 at 1:17 PM, Sébastien Guimmara
<sebastien.guimmara@gmail.com> wrote:
> * In target common-cmds.h:
>   The AWK script 'generate-cmdlist.awk' replaces 'generate-cmdlist.sh'
>
> * In target check-docs:
>   command-list.txt now contains common commands group in
>   the header [common]. sed ignore all lines in command-list.txt
>   until the [commands] list to correctly checks for missing
>   documentation on Git commands.
>
> For the target common-cmds.h part:
> Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>

The Makefile modification from my generate-cmdlist patch[1] is part of
the overall logical change of that patch. Its relation to the Makefile
changes in this patch is weak at best, or entirely non-existent.
Consequently, it should not be mixed with them. See my reply[2] to the
v5 cover letter for more information.

More below.

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

> For the target check-docs part:
> Helped-by: Eric Sunshine <sunshine@sunshineco.com>
> Signed-off-by: Sébastien Guimmara <sebastien.guimmara@gmail.com>
> ---
> diff --git a/Makefile b/Makefile
> index 5f3987f..9f333e9 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -1687,10 +1687,10 @@ $(BUILT_INS): git$X
>         ln -s $< $@ 2>/dev/null || \
>         cp $< $@
>
> -common-cmds.h: ./generate-cmdlist.sh command-list.txt
> +common-cmds.h: generate-cmdlist.awk command-list.txt
>
>  common-cmds.h: $(wildcard Documentation/git-*.txt)
> -       $(QUIET_GEN)./generate-cmdlist.sh > $@+ && mv $@+ $@
> +       $(QUIET_GEN)awk -f generate-cmdlist.awk command-list.txt > $@+ && mv $@+ $@
>
>  SCRIPT_DEFINES = $(SHELL_PATH_SQ):$(DIFF_SQ):$(GIT_VERSION):\
>         $(localedir_SQ):$(NO_CURL):$(USE_GETTEXT_SCHEME):$(SANE_TOOL_PATH_SQ):\
> @@ -2447,7 +2447,7 @@ check-docs::
>                 esac ; \
>                 test -f "Documentation/$$v.txt" || \
>                 echo "no doc: $$v"; \
> -               sed -e '/^#/d' command-list.txt | \
> +               sed -e '1,/^\[commands\]/d' <command-list.txt | \

I'm not convinced that it's a good idea to drop comment-line
processing from this sed invocation. Even though the current
command-list.txt may not have any comments following the [commands]
header, there is no guarantee that someone won't some day add some
comments following the header.

sed accepts multiple -e arguments, so retaining comment-line
processing does not make the extraction any more expensive. For
instance:

    sed -e '1,/^\[commands\]/d' -e '/^#/d' <command-list.txt | \

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

Ditto. It would be more robust to retain comment-line processing.

>                     -e 's/[     ].*//' \
>                     -e 's/^/listed /' command-list.txt; \
>                 $(MAKE) -C Documentation print-man1 | \
> --
> 2.4.0