Re: [PATCH v7 10/10] convert: add filter.<driver>.process option

From: Junio C Hamano <gitster@pobox.com>
Date: Sep 13, 2016, 15:22 UTC
Message-ID: <xmqq8tuvx1sz.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <20160908182132.50788-11-larsxschneider@gmail.com>

larsxschneider@gmail.com writes:

Show 15 quoted lines

> diff --git a/contrib/long-running-filter/example.pl b/contrib/long-running-filter/example.pl
> ...
> +sub packet_read {
> +    my $buffer;
> +    my $bytes_read = read STDIN, $buffer, 4;
> +    if ( $bytes_read == 0 ) {
> +
> +        # EOF - Git stopped talking to us!
> +        exit();
> +...
> +packet_write( "clean=true\n" );
> +packet_write( "smudge=true\n" );
> +packet_flush();
> +
> +while (1) {

These extra SP around the contents inside () pair look unfamiliar and somewhat strange to me, but as long as they are consistently done (and I think you are mostly being consistent), it is OK.

> +#define CAP_CLEAN    (1u<<0)
> +#define CAP_SMUDGE   (1u<<1)

As these are meant to be usable together, i.e. bits in a single flag word, they are of type "unsigned int", which makes perfect sense.

Make sure your variables and fields that store them are of the same type. I think I saw "int' used to pass them in at least one place.

Show 5 quoted lines

> +static int apply_filter(const char *path, const char *src, size_t len,
> +                        int fd, struct strbuf *dst, struct convert_driver *drv,
> +                        const int wanted_capability)
> +{
> +	const char* cmd = NULL;

"const char *cmd = NULL;" of course.

Show 12 quoted lines

> diff --git a/unpack-trees.c b/unpack-trees.c
> index 11c37fb..f6798f8 100644
> --- a/unpack-trees.c
> +++ b/unpack-trees.c
> @@ -10,6 +10,7 @@
>  #include "attr.h"
>  #include "split-index.h"
>  #include "dir.h"
> +#include "convert.h"
>  
>  /*
>   * Error messages expected by scripts out of plumbing commands such as

Why?  The resulting file seems to compile without this addition.