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

From: Lars Schneider <larsxschneider@gmail.com>
Date: Sep 15, 2016, 20:16 UTC
Message-ID: <928655D2-312B-4805-99DB-E73448232B9A@gmail.com>
In-Reply-To: <xmqq8tuvx1sz.fsf@gitster.mtv.corp.google.com>

Show 23 quoted lines

> On 13 Sep 2016, at 17:22, Junio C Hamano <gitster@pobox.com> wrote:
> 
> larsxschneider@gmail.com writes:
> 
>> 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.

Ups. I forgot to run PerlTidy here. I run PerlTidy with the flag "-pbp" (= Perl Best Practices). This seems to add no extra SP for functions with one parameter (e.g. `foo("bar")`) and extra SP for functions with multiple parameter (e.g. `foo( "bar", 1 )`). Is this still OK?

Does anyone have a "Git PerlTidy configuration"?

Show 9 quoted lines

> 
>> +#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.

Fixed!

Show 7 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.

Fixed!

Show 14 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.

Of course. That shouldn't have been part of this commit.

Thank you, Lars