From: Nguyen Thai Ngoc Duy <pclouds@gmail.com>
Date: Tue, 11 Aug 2009 01:57:30 GMT
Subject: Re: [RFC PATCH v2 2/4] gitignore: read from index if .gitignore is assume-unchanged
Message-ID: <fcaeb9bf0908101857x7a44d3dfge20e45d24daee9bf@mail.gmail.com>
In-Reply-To: <alpine.DEB.1.00.0908101826250.8324@intel-tinevez-2-302>

2009/8/10 Johannes Schindelin <Johannes.Schindelin@gmx.de>:
> Hi,
>
> On Mon, 10 Aug 2009, Nguyễn Thái Ngọc Duy wrote:
>
>> diff --git a/dir.c b/dir.c
>> index e05b850..e55344f 100644
>> --- a/dir.c
>> +++ b/dir.c
>> @@ -200,11 +200,36 @@ void add_exclude(const char *string, const char *base,
>>       which->excludes[which->nr++] = x;
>>  }
>>
>> +static void *read_index_data(const char *path, size_t *size)
>
> How about calling it "read_assume_unchanged_from_index()" instead?  I
> suggest this because it does not read the index from the data if the path
> is not marked assume unchanged...

Agree.

>> @@ -212,27 +237,31 @@ static int add_excludes_from_file_1(const char *fname,
>>
>> [...]
>>
>>       if (buf_p)
>>               *buf_p = buf;
>> -     buf[size++] = '\n';
>>       entry = buf;
>> -     for (i = 0; i < size; i++) {
>> -             if (buf[i] == '\n') {
>> +     for (i = 0; i <= size; i++) {
>> +             if (i == size || buf[i] == '\n') {
>>                       if (entry != buf + i && entry[0] != '#') {
>>                               buf[i - (i && buf[i-1] == '\r')] = 0;
>>                               add_exclude(entry, base, baselen, which);
>
> Should this change not rather be a separate one?

You meant a separate patch? It is tied to this patch, because if bus
is read from read_index_data, it does not have extra space for '\n' at
the end.

>> @@ -241,17 +270,12 @@ static int add_excludes_from_file_1(const char *fname,
>>               }
>>       }
>>       return 0;
>> -
>> - err:
>> -     if (0 <= fd)
>> -             close(fd);
>> -     return -1;
>>  }
>>
>>  void add_excludes_from_file(struct dir_struct *dir, const char *fname)
>>  {
>>       if (add_excludes_from_file_1(fname, "", 0, NULL,
>> -                                  &dir->exclude_list[EXC_FILE]) < 0)
>> +                                  &dir->exclude_list[EXC_FILE], 0) < 0)
>
> Could you mention in the commit message why this function does not want to
> check the index (I _guess_ it is because this code path only tries to read
> .git/info/exclude, but it would be better to be sure).

To retain old behaviour. But I have to check its callers. Maybe we
want to check the index too.

>> @@ -85,6 +85,26 @@ test_expect_success \
>>         >output &&
>>       test_cmp expect output'
>>
>> +test_expect_success 'setup sparse gitignore' '
>> +     git add .gitignore one/.gitignore one/two/.gitignore &&
>> +     git update-index --assume-unchanged .gitignore one/.gitignore one/two/.gitignore &&
>> +     rm .gitignore one/.gitignore one/two/.gitignore
>> +'
>
> You're probably less sloppy than me; I'd have defined a variable like
> this:
>
>        ignores=".gitignore one/.gitignore one/two/.gitignore"
>
> and used it for the three calls, just to make sure that I do not fsck
> anything up there due to fat fingers.

I have slim ones :-) But "git add $ignores && git update-index
$ignores && rm $ignores" is easier to read.
-- 
Duy