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

From: Junio C Hamano <gitster@pobox.com>
Date: Aug 6, 2013, 06:36 UTC
Message-ID: <7vfvuniavq.fsf@alter.siamese.dyndns.org>
In-Reply-To: <CALWbr2wynb-K-r0sehuBUtmkbgp9Ev5iYK_v2ZFxsjcewTCmfQ@mail.gmail.com>

Antoine Pelisse <apelisse@gmail.com> writes:

Show 18 quoted lines

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

Show 9 quoted lines

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