Re: [PATCH v3 16/35] transport: convert transport_get_remote_refs to take a list of ref patterns

[Brandon Williams]

On 02/22, Jonathan Tan wrote:
> On Thu, 22 Feb 2018 10:26:47 -0800
> Brandon Williams  wrote:
> 
> > On 02/21, Jonathan Tan wrote:
> > > On Tue,  6 Feb 2018 17:12:53 -0800
> > > Brandon Williams  wrote:
> > > 
> > > > -const struct ref *transport_get_remote_refs(struct transport 
> > > > *transport)
> > > > +const struct ref *transport_get_remote_refs(struct transport 
> > > > *transport,
> > > > +   const struct argv_array 
> > > > *ref_patterns)
> > > >  {
> > > > if (!transport->got_remote_refs) {
> > > > -   transport->remote_refs = 
> > > > transport->vtable->get_refs_list(transport, 0, NULL);
> > > > +   transport->remote_refs =
> > > > +   transport->vtable->get_refs_list(transport, 0,
> > > > +ref_patterns);
> > > > transport->got_remote_refs = 1;
> > > > }
> > > 
> > > Should we do our own client-side filtering if the server side cannot do
> > > it for us (because it doesn't support protocol v2)? Either way, this
> > > decision should be mentioned in the commit message.
> > 
> > If someone wants to add this in the future they can, but that is outside
> > the scope of this series.
> 
> In that case, also document that this function is allowed to return refs
> that do not match the ref patterns.
> 
> Unlike in patch 15 (which deals with the interface between the transport
> code and transport vtables, which can be changed as long as the
> transport code is aware of it, as I wrote in [1]), this may result in
> user-visible differences depending on which protocol is used. But after
> more thinking, I don't think we're in a situation yet where having extra
> refs shown/written are harmful, and if it comes to that, we can tighten
> this code later without backwards incompatibility. So, OK, this is fine.
> 
> [1] 
> https://public-inbox.org/git/20180221145639.c6cf2409ce2120109bdd1...@google.com/

I'll add the documentation.

-- 
Brandon Williams

Re: [PATCH v3 16/35] transport: convert transport_get_remote_refs to take a list of ref patterns

[Jonathan Tan]

On Thu, 22 Feb 2018 10:26:47 -0800
Brandon Williams  wrote:

> On 02/21, Jonathan Tan wrote:
> > On Tue,  6 Feb 2018 17:12:53 -0800
> > Brandon Williams  wrote:
> > 
> > > -const struct ref *transport_get_remote_refs(struct transport *transport)
> > > +const struct ref *transport_get_remote_refs(struct transport *transport,
> > > + const struct argv_array 
> > > *ref_patterns)
> > >  {
> > >   if (!transport->got_remote_refs) {
> > > - transport->remote_refs = 
> > > transport->vtable->get_refs_list(transport, 0, NULL);
> > > + transport->remote_refs =
> > > + transport->vtable->get_refs_list(transport, 0,
> > > +  ref_patterns);
> > >   transport->got_remote_refs = 1;
> > >   }
> > 
> > Should we do our own client-side filtering if the server side cannot do
> > it for us (because it doesn't support protocol v2)? Either way, this
> > decision should be mentioned in the commit message.
> 
> If someone wants to add this in the future they can, but that is outside
> the scope of this series.

In that case, also document that this function is allowed to return refs
that do not match the ref patterns.

Unlike in patch 15 (which deals with the interface between the transport
code and transport vtables, which can be changed as long as the
transport code is aware of it, as I wrote in [1]), this may result in
user-visible differences depending on which protocol is used. But after
more thinking, I don't think we're in a situation yet where having extra
refs shown/written are harmful, and if it comes to that, we can tighten
this code later without backwards incompatibility. So, OK, this is fine.

[1] 
https://public-inbox.org/git/20180221145639.c6cf2409ce2120109bdd1...@google.com/

Re: [PATCH v3 16/35] transport: convert transport_get_remote_refs to take a list of ref patterns

[Brandon Williams]

On 02/21, Jonathan Tan wrote:
> On Tue,  6 Feb 2018 17:12:53 -0800
> Brandon Williams  wrote:
> 
> > -const struct ref *transport_get_remote_refs(struct transport *transport)
> > +const struct ref *transport_get_remote_refs(struct transport *transport,
> > +   const struct argv_array 
> > *ref_patterns)
> >  {
> > if (!transport->got_remote_refs) {
> > -   transport->remote_refs = 
> > transport->vtable->get_refs_list(transport, 0, NULL);
> > +   transport->remote_refs =
> > +   transport->vtable->get_refs_list(transport, 0,
> > +ref_patterns);
> > transport->got_remote_refs = 1;
> > }
> 
> Should we do our own client-side filtering if the server side cannot do
> it for us (because it doesn't support protocol v2)? Either way, this
> decision should be mentioned in the commit message.

If someone wants to add this in the future they can, but that is outside
the scope of this series.

-- 
Brandon Williams

Re: [PATCH v3 16/35] transport: convert transport_get_remote_refs to take a list of ref patterns

[Jonathan Tan]

On Tue,  6 Feb 2018 17:12:53 -0800
Brandon Williams  wrote:

> -const struct ref *transport_get_remote_refs(struct transport *transport)
> +const struct ref *transport_get_remote_refs(struct transport *transport,
> + const struct argv_array 
> *ref_patterns)
>  {
>   if (!transport->got_remote_refs) {
> - transport->remote_refs = 
> transport->vtable->get_refs_list(transport, 0, NULL);
> + transport->remote_refs =
> + transport->vtable->get_refs_list(transport, 0,
> +  ref_patterns);
>   transport->got_remote_refs = 1;
>   }

Should we do our own client-side filtering if the server side cannot do
it for us (because it doesn't support protocol v2)? Either way, this
decision should be mentioned in the commit message.

[PATCH v3 16/35] transport: convert transport_get_remote_refs to take a list of ref patterns

[Brandon Williams]

Convert 'transport_get_remote_refs()' to optionally take a list of ref
patterns.

Signed-off-by: Brandon Williams 
---
 builtin/clone.c | 2 +-
 builtin/fetch.c | 4 ++--
 builtin/ls-remote.c | 2 +-
 builtin/remote.c| 2 +-
 transport.c | 7 +--
 transport.h | 3 ++-
 6 files changed, 12 insertions(+), 8 deletions(-)

diff --git a/builtin/clone.c b/builtin/clone.c
index 284651797..6e77d993f 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -1121,7 +1121,7 @@ int cmd_clone(int argc, const char **argv, const char 
*prefix)
if (transport->smart_options && !deepen)
transport->smart_options->check_self_contained_and_connected = 
1;
 
-   refs = transport_get_remote_refs(transport);
+   refs = transport_get_remote_refs(transport, NULL);
 
if (refs) {
mapped_refs = wanted_peer_refs(refs, refspec);
diff --git a/builtin/fetch.c b/builtin/fetch.c
index 7bbcd26fa..850382f55 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -250,7 +250,7 @@ static void find_non_local_tags(struct transport *transport,
struct string_list_item *item = NULL;
 
for_each_ref(add_existing, _refs);
-   for (ref = transport_get_remote_refs(transport); ref; ref = ref->next) {
+   for (ref = transport_get_remote_refs(transport, NULL); ref; ref = 
ref->next) {
if (!starts_with(ref->name, "refs/tags/"))
continue;
 
@@ -336,7 +336,7 @@ static struct ref *get_ref_map(struct transport *transport,
/* opportunistically-updated references: */
struct ref *orefs = NULL, **oref_tail = 
 
-   const struct ref *remote_refs = transport_get_remote_refs(transport);
+   const struct ref *remote_refs = transport_get_remote_refs(transport, 
NULL);
 
if (refspec_count) {
struct refspec *fetch_refspec;
diff --git a/builtin/ls-remote.c b/builtin/ls-remote.c
index c4be98ab9..c6e9847c5 100644
--- a/builtin/ls-remote.c
+++ b/builtin/ls-remote.c
@@ -96,7 +96,7 @@ int cmd_ls_remote(int argc, const char **argv, const char 
*prefix)
if (uploadpack != NULL)
transport_set_option(transport, TRANS_OPT_UPLOADPACK, 
uploadpack);
 
-   ref = transport_get_remote_refs(transport);
+   ref = transport_get_remote_refs(transport, NULL);
if (transport_disconnect(transport))
return 1;
 
diff --git a/builtin/remote.c b/builtin/remote.c
index d95bf904c..d0b6ff6e2 100644
--- a/builtin/remote.c
+++ b/builtin/remote.c
@@ -862,7 +862,7 @@ static int get_remote_ref_states(const char *name,
if (query) {
transport = transport_get(states->remote, 
states->remote->url_nr > 0 ?
states->remote->url[0] : NULL);
-   remote_refs = transport_get_remote_refs(transport);
+   remote_refs = transport_get_remote_refs(transport, NULL);
transport_disconnect(transport);
 
states->queried = 1;
diff --git a/transport.c b/transport.c
index c54a44630..dfc603b36 100644
--- a/transport.c
+++ b/transport.c
@@ -1136,10 +1136,13 @@ int transport_push(struct transport *transport,
return 1;
 }
 
-const struct ref *transport_get_remote_refs(struct transport *transport)
+const struct ref *transport_get_remote_refs(struct transport *transport,
+   const struct argv_array 
*ref_patterns)
 {
if (!transport->got_remote_refs) {
-   transport->remote_refs = 
transport->vtable->get_refs_list(transport, 0, NULL);
+   transport->remote_refs =
+   transport->vtable->get_refs_list(transport, 0,
+ref_patterns);
transport->got_remote_refs = 1;
}
 
diff --git a/transport.h b/transport.h
index 731c78b67..4b656f315 100644
--- a/transport.h
+++ b/transport.h
@@ -178,7 +178,8 @@ int transport_push(struct transport *connection,
   int refspec_nr, const char **refspec, int flags,
   unsigned int * reject_reasons);
 
-const struct ref *transport_get_remote_refs(struct transport *transport);
+const struct ref *transport_get_remote_refs(struct transport *transport,
+   const struct argv_array 
*ref_patterns);
 
 int transport_fetch_refs(struct transport *transport, struct ref *refs);
 void transport_unlock_pack(struct transport *transport);
-- 
2.16.0.rc1.238.g530d649a79-goog