Re: [PATCH v3 06/35] transport: use get_refs_via_connect to get refs

[Brandon Williams]

On 02/27, Jonathan Nieder wrote:
> Brandon Williams wrote:
> > On 02/26, Jonathan Nieder wrote:
> >> Brandon Williams wrote:
> 
> >>> +++ b/transport.c
> >>> @@ -230,12 +230,8 @@ static int fetch_refs_via_pack(struct transport 
> >>> *transport,
> >>>   args.cloning = transport->cloning;
> >>>   args.update_shallow = data->options.update_shallow;
> >>>  
> >>> - if (!data->got_remote_heads) {
> >>> - connect_setup(transport, 0);
> >>> - get_remote_heads(data->fd[0], NULL, 0, _tmp, 0,
> >>> -  NULL, >shallow);
> >>> - data->got_remote_heads = 1;
> >>> - }
> >>> + if (!data->got_remote_heads)
> >>> + refs_tmp = get_refs_via_connect(transport, 0);
> >>
> >> The only difference between the old and new code is that the old code
> >> passes NULL as 'extra_have' and the new code passes >extra_have.
> >>
> >> That means this populates the data->extra_have oid_array.  Does it
> >> matter?
> [...]
> > I don't think its a problem to have extra_have populated, least I
> > haven't seen anything to lead me to believe it would be a problem.
> 
> Assuming it gets properly freed later, the only effect I can imagine
> is some increased memory usage.
> 
> I'm inclined to agree with you that the simplicity is worth it.  It
> seems worth mentioning in the commit message, though.
> 
> [...]
> >>> @@ -541,14 +537,8 @@ static int git_transport_push(struct transport 
> >>> *transport, struct ref *remote_re
> >>>   struct send_pack_args args;
> >>>   int ret;
> >>>  
> >>> - if (!data->got_remote_heads) {
> >>> - struct ref *tmp_refs;
> >>> - connect_setup(transport, 1);
> >>> -
> >>> - get_remote_heads(data->fd[0], NULL, 0, _refs, REF_NORMAL,
> >>> -  NULL, >shallow);
> >>> - data->got_remote_heads = 1;
> >>> - }
> >>> + if (!data->got_remote_heads)
> >>> + get_refs_via_connect(transport, 1);
> >>
> >> not a new problem, just curious: Does this leak tmp_refs?
> >
> > Maybe, though its removed by this patch.
> 
> Sorry for the lack of clarity.  If it was leaked before, then it is
> still leaked now, via the discarded return value from
> get_refs_via_connect.
> 
> Any idea how we can track that down?  E.g. are there ways to tell leak
> checkers "just tell me about this particular allocation"?

Hmm I wonder if that code path is even used, because it just throws away
the result.

-- 
Brandon Williams

Re: [PATCH v3 06/35] transport: use get_refs_via_connect to get refs

[Jonathan Nieder]

Brandon Williams wrote:
> On 02/26, Jonathan Nieder wrote:
>> Brandon Williams wrote:

>>> +++ b/transport.c
>>> @@ -230,12 +230,8 @@ static int fetch_refs_via_pack(struct transport 
>>> *transport,
>>> args.cloning = transport->cloning;
>>> args.update_shallow = data->options.update_shallow;
>>>  
>>> -   if (!data->got_remote_heads) {
>>> -   connect_setup(transport, 0);
>>> -   get_remote_heads(data->fd[0], NULL, 0, _tmp, 0,
>>> -NULL, >shallow);
>>> -   data->got_remote_heads = 1;
>>> -   }
>>> +   if (!data->got_remote_heads)
>>> +   refs_tmp = get_refs_via_connect(transport, 0);
>>
>> The only difference between the old and new code is that the old code
>> passes NULL as 'extra_have' and the new code passes >extra_have.
>>
>> That means this populates the data->extra_have oid_array.  Does it
>> matter?
[...]
> I don't think its a problem to have extra_have populated, least I
> haven't seen anything to lead me to believe it would be a problem.

Assuming it gets properly freed later, the only effect I can imagine
is some increased memory usage.

I'm inclined to agree with you that the simplicity is worth it.  It
seems worth mentioning in the commit message, though.

[...]
>>> @@ -541,14 +537,8 @@ static int git_transport_push(struct transport 
>>> *transport, struct ref *remote_re
>>> struct send_pack_args args;
>>> int ret;
>>>  
>>> -   if (!data->got_remote_heads) {
>>> -   struct ref *tmp_refs;
>>> -   connect_setup(transport, 1);
>>> -
>>> -   get_remote_heads(data->fd[0], NULL, 0, _refs, REF_NORMAL,
>>> -NULL, >shallow);
>>> -   data->got_remote_heads = 1;
>>> -   }
>>> +   if (!data->got_remote_heads)
>>> +   get_refs_via_connect(transport, 1);
>>
>> not a new problem, just curious: Does this leak tmp_refs?
>
> Maybe, though its removed by this patch.

Sorry for the lack of clarity.  If it was leaked before, then it is
still leaked now, via the discarded return value from
get_refs_via_connect.

Any idea how we can track that down?  E.g. are there ways to tell leak
checkers "just tell me about this particular allocation"?

Thanks,
Jonathan

Re: [PATCH v3 06/35] transport: use get_refs_via_connect to get refs

[Brandon Williams]

On 02/26, Jonathan Nieder wrote:
> Brandon Williams wrote:
> 
> > Remove code duplication and use the existing 'get_refs_via_connect()'
> > function to retrieve a remote's heads in 'fetch_refs_via_pack()' and
> > 'git_transport_push()'.
> > 
> > Signed-off-by: Brandon Williams 
> > ---
> >  transport.c | 18 --
> >  1 file changed, 4 insertions(+), 14 deletions(-)
> 
> I like the diffstat.
> 
> [...]
> > +++ b/transport.c
> > @@ -230,12 +230,8 @@ static int fetch_refs_via_pack(struct transport 
> > *transport,
> > args.cloning = transport->cloning;
> > args.update_shallow = data->options.update_shallow;
> >  
> > -   if (!data->got_remote_heads) {
> > -   connect_setup(transport, 0);
> > -   get_remote_heads(data->fd[0], NULL, 0, _tmp, 0,
> > -NULL, >shallow);
> > -   data->got_remote_heads = 1;
> > -   }
> > +   if (!data->got_remote_heads)
> > +   refs_tmp = get_refs_via_connect(transport, 0);
> 
> The only difference between the old and new code is that the old code
> passes NULL as 'extra_have' and the new code passes >extra_have.
> 
> That means this populates the data->extra_have oid_array.  Does it
> matter?
> 
> > @@ -541,14 +537,8 @@ static int git_transport_push(struct transport 
> > *transport, struct ref *remote_re
> > struct send_pack_args args;
> > int ret;
> >  
> > -   if (!data->got_remote_heads) {
> > -   struct ref *tmp_refs;
> > -   connect_setup(transport, 1);
> > -
> > -   get_remote_heads(data->fd[0], NULL, 0, _refs, REF_NORMAL,
> > -NULL, >shallow);
> > -   data->got_remote_heads = 1;
> > -   }
> > +   if (!data->got_remote_heads)
> > +   get_refs_via_connect(transport, 1);
> 
> not a new problem, just curious: Does this leak tmp_refs?

Maybe, though its removed by this patch.

> 
> Same question as the other caller about whether we mind getting
> extra_have populated.

I don't think its a problem to have extra_have populated, least I
haven't seen anything to lead me to believe it would be a problem.

-- 
Brandon Williams

Re: [PATCH v3 06/35] transport: use get_refs_via_connect to get refs

[Jonathan Nieder]

Brandon Williams wrote:

> Remove code duplication and use the existing 'get_refs_via_connect()'
> function to retrieve a remote's heads in 'fetch_refs_via_pack()' and
> 'git_transport_push()'.
> 
> Signed-off-by: Brandon Williams 
> ---
>  transport.c | 18 --
>  1 file changed, 4 insertions(+), 14 deletions(-)

I like the diffstat.

[...]
> +++ b/transport.c
> @@ -230,12 +230,8 @@ static int fetch_refs_via_pack(struct transport 
> *transport,
>   args.cloning = transport->cloning;
>   args.update_shallow = data->options.update_shallow;
>  
> - if (!data->got_remote_heads) {
> - connect_setup(transport, 0);
> - get_remote_heads(data->fd[0], NULL, 0, _tmp, 0,
> -  NULL, >shallow);
> - data->got_remote_heads = 1;
> - }
> + if (!data->got_remote_heads)
> + refs_tmp = get_refs_via_connect(transport, 0);

The only difference between the old and new code is that the old code
passes NULL as 'extra_have' and the new code passes >extra_have.

That means this populates the data->extra_have oid_array.  Does it
matter?

> @@ -541,14 +537,8 @@ static int git_transport_push(struct transport 
> *transport, struct ref *remote_re
>   struct send_pack_args args;
>   int ret;
>  
> - if (!data->got_remote_heads) {
> - struct ref *tmp_refs;
> - connect_setup(transport, 1);
> -
> - get_remote_heads(data->fd[0], NULL, 0, _refs, REF_NORMAL,
> -  NULL, >shallow);
> - data->got_remote_heads = 1;
> - }
> + if (!data->got_remote_heads)
> + get_refs_via_connect(transport, 1);

not a new problem, just curious: Does this leak tmp_refs?

Same question as the other caller about whether we mind getting
extra_have populated.

Thanks,
Jonathan

[PATCH v3 06/35] transport: use get_refs_via_connect to get refs

[Brandon Williams]

Remove code duplication and use the existing 'get_refs_via_connect()'
function to retrieve a remote's heads in 'fetch_refs_via_pack()' and
'git_transport_push()'.

Signed-off-by: Brandon Williams 
---
 transport.c | 18 --
 1 file changed, 4 insertions(+), 14 deletions(-)

diff --git a/transport.c b/transport.c
index fc802260f..8e8779096 100644
--- a/transport.c
+++ b/transport.c
@@ -230,12 +230,8 @@ static int fetch_refs_via_pack(struct transport *transport,
args.cloning = transport->cloning;
args.update_shallow = data->options.update_shallow;
 
-   if (!data->got_remote_heads) {
-   connect_setup(transport, 0);
-   get_remote_heads(data->fd[0], NULL, 0, _tmp, 0,
-NULL, >shallow);
-   data->got_remote_heads = 1;
-   }
+   if (!data->got_remote_heads)
+   refs_tmp = get_refs_via_connect(transport, 0);
 
refs = fetch_pack(, data->fd, data->conn,
  refs_tmp ? refs_tmp : transport->remote_refs,
@@ -541,14 +537,8 @@ static int git_transport_push(struct transport *transport, 
struct ref *remote_re
struct send_pack_args args;
int ret;
 
-   if (!data->got_remote_heads) {
-   struct ref *tmp_refs;
-   connect_setup(transport, 1);
-
-   get_remote_heads(data->fd[0], NULL, 0, _refs, REF_NORMAL,
-NULL, >shallow);
-   data->got_remote_heads = 1;
-   }
+   if (!data->got_remote_heads)
+   get_refs_via_connect(transport, 1);
 
memset(, 0, sizeof(args));
args.send_mirror = !!(flags & TRANSPORT_PUSH_MIRROR);
-- 
2.16.0.rc1.238.g530d649a79-goog