Re: [PATCH 04/45] pathspec: add copy_pathspec

From: Junio C Hamano <gitster@pobox.com>
Date: Jun 12, 2013, 18:07 UTC
Message-ID: <7vmwqv42zt.fsf@alter.siamese.dyndns.org>
In-Reply-To: <CAPig+cTP8HuPzp=THuSTFtRW8uNgqxROWuQUG7_bYC_FSs5rpQ@mail.gmail.com>

Eric Sunshine <sunshine@sunshineco.com> writes:

Show 12 quoted lines

> On Sun, Jun 9, 2013 at 2:25 AM, Nguyễn Thái Ngọc Duy <pclouds@gmail.com> wrote:
>> The function is made to use with free_pathspec() because a simple
>> struct assignment is not enough (free_pathspec wants to free "items"
>> pointer).
>
> I had to read this about five or six times before I could parse and
> understand it. Perhaps rewrite something like this:
>
>     free_pathspec() frees pathspec->items, hence simple struct
>     assignment does not properly duplicate a pathspec. Introduce
>     copy_pathspec() to clone a pathspec correctly by deeply copying
>     the items member.

Thanks. It might be worth that this is not a full "deep" copy, perhaps like this:

    pathspec: add copy_pathspec
    
    Because free_pathspec wants to free "items" pointer in the pathspec
    structure, a simple structure assignment is not enough if you want
    to copy an existing pathspec into another.  Freeing the original will
    damage the copy unless a deep copy is made.
    
    Note that the strings in pathspec->items->match and the array
    pathspec->raw[] are still shared between the original and the
    copy.
    
    Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
    Signed-off-by: Junio C Hamano <gitster@pobox.com>