Re: Proposed branch tag performance patch for feature

[Mark D. Baushke]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Kelly F. Hickel <kfh@mqsoftware.com> writes:

> The not as good news, quite possibly to the above mentioned non-groking
> of CVS_LOCAL_BRANCH_NUM, it looks to me that the feature patch is
> probably wrong.  Looks like your using that to set the defaultrv param
> to findnextmagicrev, but that parameter is only used if
> findnextmagicrev_proc returns an empty list (no candidate revisions, or
> some error). I had imagined that the patch for the feature release would
> have to go through the sorted list of revisions and find the highest
> value that was less than CVS_LOCAL_BRANCH_NUM and start there.  The
> other possibility that I came up with (in case I flipped the meaning of
> CVS_LOCAL_BRANCH_NUM) was that, if the highest found revision was less
> than CVS_LOCAL_BRANCH_NUM, then I'd have to start from
> CVS_LOCAL_BRANCH_NUM.  It hinges on whether CVS_LOCAL_BRANCH_NUM means
> that when branching on this repo one needs to create a magic branch tag
> with a revision above CVS_LOCAL_BRANCH_NUM, or if it needs to be less
> than CVS_LOCAL_BRANCH_NUM.

Hmmm... the branch needs to be above CVS_LOCAL_BRANCH_NUM, so you are
correct that the patch as written is wrong.

One way to deal with this is to only use the findnextmagicrev function
if there is not an initial CVS_LOCAL_BRANCH_NUM from which to start.

    local_branch_num = getenv ("CVS_LOCAL_BRANCH_NUM");
    if (local_branch_num)
    {
        rev_num = atoi (local_branch_num);
        if (rev_num < 2)
            rev_num = 2;
        else
            rev_num &= ~1;
    }
    else
    {
        /* Find the next unused magic rev,
         * if none are found, it should return 2.
         */
        rev_num = findnextmagicrev (rcs, rev, 2);
    }

Given that most of the time there should only be a 'few' local branches
in a CVSup mirror of the main repository, this may not be a horrible
problem.

> The way the feature patch currently is set up, most of the time
> CVS_LOCAL_BRANCH_NUM isn't going to figure in the calculation at all,
> which I suspect is the wrong thing. And, there probably needs to be a
> sanity.sh test for whichever is the correct behavior, and I really don't
> have any idea how to write that test (although I might, once the issues
> in the previous paragraph are resolved).

Yes, this will require a few more client tests.

        -- Mark
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.6 (FreeBSD)

iD8DBQFFnSnZCg7APGsDnFERAj16AJ4+gG8DzLQZpqdqiUsIiMDEe2ZPwACg8s1N
i23oAyVQJo7YW1RgYJzexSU=
=z50m
-----END PGP SIGNATURE-----