On Fri, Jun 7, 2019 at 6:59 PM Emily Shaffer <[email protected]> wrote:
> 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 <[email protected]>
> ---
> 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
> 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);
> 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.
