Re: [RFC PATCH v2 2/4] gitignore: read from index if .gitignore is assume-unchanged

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Aug 11, 2009, 08:12 UTC
Message-ID: <alpine.DEB.1.00.0908111009010.4638@intel-tinevez-2-302>
In-Reply-To: <fcaeb9bf0908101857x7a44d3dfge20e45d24daee9bf@mail.gmail.com>

Hi,

On Tue, 11 Aug 2009, Nguyen Thai Ngoc Duy wrote:

Show 23 quoted lines

> 2009/8/10 Johannes Schindelin <Johannes.Schindelin@gmx.de>:
>
> > On Mon, 10 Aug 2009, Nguyễn Thái Ngọc Duy wrote:
> >
> >> @@ -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?

Yes, sorry, I meant exactly that.

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

But you could separate it out as a patch that just avoids writing to buf. IMHO this would make following the patch series easier.

Show 23 quoted lines

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

Yes, it would be good to illustrate in the commit message which code paths want to check the index, and why.

Show 19 quoted lines

> >> @@ -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 :-)

Oh, don't get me wrong: my fingers are slim enough to play piano, and even to write pretty fast on my EeePC 701.

But darn, there are days when they feel as if they were thick like a brick.

Ciao, Dscho