On Fri, Jun 07, 2019 at 08:42:45PM -0400, Eric Sunshine wrote:
> On Fri, Jun 7, 2019 at 6:59 PM Emily Shaffer <emilyshaffer@xxxxxxxxxx> 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 <emilyshaffer@xxxxxxxxxx>
> > ---
> > 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.
Thanks for the review, Eric. I'm going to try again from scratch with
Peff and Junio's suggestion about adding an option to remove object
names, and I'll try to keep your general style concerns in mind when I
do so.
- Emily
