[PATCH 2 of 2 stable] scmutil: record all symlinks to a repository in walkrepos

[Enrique A. Tobis]

On Fri, Dec 12, 2014 at 3:52 PM, Matt Mackall <mpm at selenic.com> wrote:
> On Thu, 2014-12-11 at 10:47 -0500, Enrique A. Tobis wrote:
>> # HG changeset patch
>> # User Enrique A. Tobis <enrique at tobis.com.ar>
>> # Date 1418275100 18000
>> #      Thu Dec 11 00:18:20 2014 -0500
>> # Branch stable
>> # Node ID a2351ea044f117402b17809050bdc5dc2de29ff4
>> # Parent  c67b563426866e1c717c6a25b0056bb51cb155e5
>> scmutil: record all symlinks to a repository in walkrepos
>>
>> In walkrepos, list all paths that lead to a repository when symlinks are
>> followed, while still avoiding infinite loops.
>
> This patch is too complex and includes unrelated changes that interfere
> with its readability. For instance:
>
>> -    def errhandler(err):
>> -        if err.filename == path:
>> -            raise err
>
> ..we used to have this error handling code that was presumably there for
> a reason. You removed it with no explanation.
>
> Even if there is a good reason to remove it, it doesn't belong in this
> patch.
>
> Relatedly, you've replaced os.walk without implementing its error
> handling.
>
> If you really need to rewrite os.walk (I'm not convinced) to get the
> behavior you want, you should do your patches like this:
>
> - cleanups
> - replace os.walk, no functional changes
> - behavior change
>
Thank you for your feedback! I'll be more careful with my patches in the future.

The current implementation of scmutil.walkrepo will follow symlinks if
asked, but if it finds a repository more than once, only the first
time it's found is yielded. My intention was to change that behavior
and make it return every occurence, not just the first. That is only
relevant in the context of symlinks, so I also want to avoid infinite
loops. if there is an infinite loop, I think it's reasonable to
traverse it once, but not more.

My idea for implementing this new behavior is to change the meaning of
seen_dirs. Right now it's, approximately, "every directory I've seen
so far since the top-most invocation of walkrepos." I want it to be
"every directory I've traversed to get here starting from the top-most
path." In other words, a stack of the directories traversed. I
couldn't come up with a way to do that using os.walk.

Do you think my change in behavior makes sense? If so, do you think it
can be implemented using os.walk?

Thanks again,
Enrique