Hi Matheus,
On Fri, 28 Jun 2019, Matheus Tavares Bernardino wrote:
> On Fri, Jun 28, 2019 at 9:50 AM Johannes Schindelin
> <Johannes.Schindelin@xxxxxx> wrote:
> >
> > Hi Matheus,
> >
> > On Thu, 27 Jun 2019, Matheus Tavares Bernardino wrote:
> >
> > > On Thu, Jun 27, 2019 at 3:47 PM Johannes Schindelin
> > > <Johannes.Schindelin@xxxxxx> wrote:
> > > >
> > > > On Thu, 27 Jun 2019, Matheus Tavares Bernardino wrote:
> > > >
> > > > > On Wed, Jun 26, 2019 at 3:04 PM Junio C Hamano <gitster@xxxxxxxxx> wrote:
> > > > > >
> > > > > > Johannes Schindelin <Johannes.Schindelin@xxxxxx> writes:
> > > > > >
> > > > > > > On Tue, 18 Jun 2019, Matheus Tavares wrote:
> > > > > > >
> > > > > > >>[...]
> > > > > > >> +/*
> > > > > > >> + * Look for a recursive symlink at iter->base.path pointing to any directory on
> > > > > > >> + * the previous stack levels. If it is found, return 1. If not, return 0.
> > > > > > >> + */
> > > > > > >> +static int find_recursive_symlinks(struct dir_iterator_int *iter)
> > > > > > >> +{
> > > > > > >> + int i;
> > > > > > >> +
> > > > > > >> + if (!(iter->flags & DIR_ITERATOR_FOLLOW_SYMLINKS) ||
> > > > > > >> + !S_ISDIR(iter->base.st.st_mode))
> > > > > > >> + return 0;
> > > > > > >>
> > > > > > >> + for (i = 0; i < iter->levels_nr; ++i)
> > > > > > >> + if (iter->base.st.st_ino == iter->levels[i].ino)
> > > > > > >
> > > > > > > This does not work on Windows. [[ Windows port does not have
> > > > > > > usable st_ino field ]]]
> > > > > >
> > > > > > And if you cross mountpoint, st_ino alone does not guarantee
> > > > > > uniqueness; you'd need to combine it with st_dev, I would think,
> > > > > > even on POSIX systems.
> > > > >
> > > > > Ok, thanks for letting me know. I'm trying to think of another
> > > > > approach to test for recursive symlinks that does not rely on inode:
> > > > > Given any symlink, we could get its real_path() and compare it with
> > > > > the path of the directory current being iterated. If the first is a
> > > > > prefix of the second, than we mark it as a recursive symlink.
> > > > >
> > > > > What do you think of this idea?
> > > >
> > > > I think this would be pretty expensive. Too expensive.
> > >
> > > Hmm, yes unfortunately :(
> > >
> > > > A better method might be to rely on st_ino/st_dev when we can, and just
> > > > not bother looking for recursive symlinks when we cannot,
> > >
> > > What if we fallback on the path prefix strategy when st_ino is not
> > > available? I mean, if we don't look for recursive symlinks, they would
> > > be iterated over and over until we get an ELOOP error. So I think
> > > using real_path() should be less expensive in this case. (But just as
> > > a fallback to st_ino, off course)
> > >
> > > > like I did in
> > > > https://github.com/git-for-windows/git/commit/979b00ccf44ec31cff4686e24adf27474923c33a
> > >
> > > Nice! At dir-iterator.h the documentation says that recursive symlinks
> > > will be ignored. If we don't implement any fallback, should we add
> > > that this is not available on Windows, perhaps?
> >
> > I do not really care, unless it breaks things on Windows that were not
> > broken before.
> >
> > You might also want to guard this behind `USE_STDEV` as Duy suggested (and
> > maybe use the opportunity to correct that constant to `USE_ST_DEV`; I
> > looked for it and did not find it because of that naming mistake).
>
> Ok, just to confirm, what I should do is send your fixup patch with
> the USE_STDEV guard addition, right? Also, USE_STDEV docs says it is
> used "from the update-index perspective", should I make it more
> generic as we're using it for other purposes or is it OK like this?
I thought Duy had verified that `USE_STDEV` would make sense in this
instance, but I agree with you that the idea of that compile time flag was
not to guard against a missing `st_dev` field, but about trusting it in
the presence of network filesystems.
So no, I revert my vote for using `USE_STDEV`.
Thanks for the sanity check.
Ciao,
Dscho
