Re: [PATCH] remote-hg: add shared repo upgrade

[Junio C Hamano <gitster@pobox.com>]

Antoine Pelisse <apelisse@gmail.com> writes:

> On Mon, Aug 5, 2013 at 11:02 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> Antoine Pelisse <apelisse@gmail.com> writes:
>> Is the untold
>> and obvious-to-those-who-are-familiar-with-this-codepath assumption
>> that it is guaranteed that there is at most one "*/clone/.hg" under
>> shared_path?
>
> No, there is no such assumption.
> That is why we create a repository just below if it doesn't exist (no
> copy was found).
> That's also why I don't see how we could split the patch.
>
> We could improve that part of the commit message:
>
>     It's trivial to upgrade to the new organization by copying the Mercurial
>     repo from one of the remotes (e.g. 'origin'), so let's do so. If
>     we can't find
>     any existing repo, we create an empty one.

That is fine, and I do not (yet) have an opinion on this patch
needing to be further split.

Quoting that part I was asking about again:

> +        # check and upgrade old organization
> +        hg_path = os.path.join(shared_path, '.hg')
> +        if os.path.exists(shared_path) and not os.path.exists(hg_path):
> +            repos = os.listdir(shared_path)
> +            for x in repos:
> +                local_hg = os.path.join(shared_path, x, 'clone', '.hg')
> +                if not os.path.exists(local_hg):
> +                    continue
> +                shutil.copytree(local_hg, hg_path)

if you can have more than one 'x' such that

    local_hg = os.path.join(shared_path, x, 'clone', '.hg')

exists, that means in repos[], there are two (or more) x1,and x2,
and in this loop you will run

	shutil.copytree(local_hg, hg_path)

twice, once for local_hg derived from x1 and another time from x2,
both to the same hg_path directory that does not change inside the
loop.  shutil.copytree(src, dst) however creates leading paths down
to dst and it would barf when dst already exists, no?

That is what I was puzzled about the code.  The log message says "we
can copy from one of them if exists, so let's do so", which makes
sense, and a code structure that may match would have looked like
so:

	for x in repos:
        	'''pick one at random, copy it and leave'''
                copytree()
                break
	else:
        	'''nothing to be copied, do it the hard way by cloning'''

but that is not what I saw so that is where my confusion came from.