Re: [PATCH] revision: remove stray whitespace when name empty

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Jun 8, 2019, 00:42 UTC
Message-ID: <CAPig+cS7ocDU3fGRPnQZqN+rT1Ujd2b=v5b1p5V6G++XmNPmsQ@mail.gmail.com>
In-Reply-To: <20190607225900.89299-1-emilyshaffer@google.com>

On Fri, Jun 7, 2019 at 6:59 PM Emily Shaffer <emilyshaffer@google.com> wrote:

Show 12 quoted lines

> Teach show_object_with_name() to avoid writing a space before a name
> which is empty. Also teach tests for rev-list --objects --filter to not
> require a space between the object ID and name.
> [...]
> Signed-off-by: Emily Shaffer <emilyshaffer@google.com>
> ---
> Not sure if making the whitespace optional is the right solution for the
> test, although I couldn't come up with a cleaner approach. Maybe
> something like this would be better, even though handling it in the
> regex is shorter?
>
>   if [[ -z "$name" ]] then

In Git, we avoid Bash-isms. Just use 'test'. And, style is to place 'then' on its own line.

    if test -z "$name"
    then

Show 9 quoted lines

> diff --git a/revision.c b/revision.c
> @@ -40,7 +40,8 @@ void show_object_with_name(FILE *out, struct object *obj, const char *name)
>  {
> -       fprintf(out, "%s ", oid_to_hex(&obj->oid));
> +       fprintf(out, "%s%s", oid_to_hex(&obj->oid),
> +               strcmp(name, "") == 0 ? "" : " ");
>         for (p = name; *p && *p != '\n'; p++)
>                 fputc(*p, out);
>         fputc('\n', out);

It's subjective, but this starts to be a bit too noisy and unreadable. An alternative:

    fputs(oid_to_hex(...), out);
    if (*name)
        putc(' ', out);

Show 5 quoted lines

> diff --git a/t/t6112-rev-list-filters-objects.sh b/t/t6112-rev-list-filters-objects.sh
> @@ -288,7 +288,7 @@ expect_has () {
>         hash=$(git -C r3 rev-parse $commit:$name) &&
> -       grep "^$hash $name$" actual
> +       grep "^$hash \?$name$" actual

This would be the first use of \? with 'grep' in the test suite. I would worry about portability. Your suggestion earlier to tailor 'grep' invocation based upon whether $name is empty would be safer.