Re: [RFC PATCH 6/6] ls-tree: introduce '--pattern' option

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Nov 17, 2022, 14:03 UTC
Message-ID: <221117.867cztisk0.gmgdl@evledraar.gmail.com>
In-Reply-To: <20221117113023.65865-7-tenglong.tl@alibaba-inc.com>

On Thu, Nov 17 2022, Teng Long wrote:

Show 9 quoted lines

> diff --git a/builtin/ls-tree.c b/builtin/ls-tree.c
> index 03dd3fbcb26..576fc9ad16f 100644
> --- a/builtin/ls-tree.c
> +++ b/builtin/ls-tree.c
> @@ -13,6 +13,7 @@
>  #include "builtin.h"
>  #include "parse-options.h"
>  #include "pathspec.h"
> +#include <stdio.h>

Aside from anything else I've mentionded (e.g. overall goals), don't include "<>" headers in anything except git-compat-util.h and similar.

In this case we don't need this at all, but if we did it should be added there...

Show 5 quoted lines

> [...]
>  	}
> -	ret = regexec(&r, line, 1, m, 0);
> +
> +	ret = regexec(regex, line, 1, m, 0);

Some whitespace-churn after the last commit...

Show 5 quoted lines

>  static void show_tree_common_default_long(struct show_tree_data *data)
>  {
>  	int base_len = data->base->len;
> +	struct strbuf sb = STRBUF_INIT;
> +	int sb_len = 0;

It's size_t, not int, so if you need to keep track of a strbuf's length use the type of its "len".

This would be better named "oldlen" or something...

> +		printf("%s", sb.buf);

puts(sb.buf) instead;

> +	if (pattern && !strlen(pattern))

pattern && !*pattern is more idiomatic IMO.

> +test_expect_success 'combine with "--object-only"' '
> +	cat > expect <<-EOF &&

Style: No " " after ">"

> +		6da7993

Style: Let's avoid \t\t indenting for here-docs, just use the indenting
of the "cat".