> On 13 Sep 2016, at 17:22, Junio C Hamano <gits...@pobox.com> wrote:
>
> larsxschnei...@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"?
>
>> +#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!
>> +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!
>> 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
