| Message ID | 20220328191112.3092139-3-calvinwan@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | object-info: add option for retrieving object info | expand |
Calvin Wan <calvinwan@google.com> writes: > Currently, object-info is always advertised by the server. Add a > config option to optionally advertise it. A subsequent patch uses this > option for testing. > > --- > Documentation/config/transfer.txt | 4 ++++ > protocol-caps.c | 11 +++++++++++ > protocol-caps.h | 6 ++++++ > serve.c | 2 +- > 4 files changed, 22 insertions(+), 1 deletion(-) > > diff --git a/Documentation/config/transfer.txt b/Documentation/config/transfer.txt > index b49429eb4d..14892a3155 100644 > --- a/Documentation/config/transfer.txt > +++ b/Documentation/config/transfer.txt > @@ -77,3 +77,7 @@ transfer.unpackLimit:: > transfer.advertiseSID:: > Boolean. When true, client and server processes will advertise their > unique session IDs to their remote counterpart. Defaults to false. > + > +transfer.advertiseObjectInfo:: > + Boolean. When true or not set, server processes will advertise its > + ability to accept `object-info` command requests > \ No newline at end of file Carefully proofread before sending your changes out ;-) By mimicking the style of the previous item more closely, i.e. Boolean. When true, ... requests. Defaults to true. it would make it easier to read, I suspect. > diff --git a/protocol-caps.c b/protocol-caps.c > index bbde91810a..f290e3cca2 100644 > --- a/protocol-caps.c > +++ b/protocol-caps.c > @@ -8,6 +8,9 @@ > #include "object-store.h" > #include "string-list.h" > #include "strbuf.h" > +#include "config.h" > + > +static int advertise_object_info = -1; It strikes me somewhat odd that this is the _first_ such capability that needs a variable here to control if it is advertised or not depending on the configuration file settings. In other words, "Why doesn't transfer.advertiseSID also have a similar thing in this file?" should be the first question that comes to any reviewers' mind. Perhaps we just invented a better way to handle such conditional capability, in which case we may want to port the existing ones that do not use the same arrangement over to the new way. Even though it would be outside the scope of the series, it would be a good way to make sure we won't accumulate too much technical debt to leave some hint somewhere near here or in protoco-caps.h where this patch touches. I cannot quite tell what is going on. > int object_info_advertise(struct repository *r, struct strbuf *value) > { > if (advertise_object_info == -1 && > git_config_get_bool("transfer.advertiseObjectInfo", &advertise_object_info)) > advertise_object_info = 0; > return advertise_object_info; > } "If unset(yet), try to get_bool() and if it fails, set it to 0" So advertise_object_info becomes true only if transfer.advertiseObjectInfo is set to true? It is inconsistent with what we read in the documentation, isn't it? > diff --git a/protocol-caps.h b/protocol-caps.h > index 15c4550360..36f7a46b37 100644 > --- a/protocol-caps.h > +++ b/protocol-caps.h > @@ -5,4 +5,10 @@ struct repository; > struct packet_reader; > int cap_object_info(struct repository *r, struct packet_reader *request); > > +/* > + * Advertises object-info capability if "objectinfo.advertise" is either > + * set to true or not set > + */ > +int object_info_advertise(struct repository *r, struct strbuf *value); > + > #endif /* PROTOCOL_CAPS_H */ > diff --git a/serve.c b/serve.c > index b3fe9b5126..fb4ad367a7 100644 > --- a/serve.c > +++ b/serve.c > @@ -133,7 +133,7 @@ static struct protocol_capability capabilities[] = { > }, > { > .name = "object-info", > - .advertise = always_advertise, > + .advertise = object_info_advertise, > .command = cap_object_info, > }, > }; This is a good step to add a test to see if the server does honor the (1) lack of (2) true set to and (3) false set to the configuration variable and sends (or does not send) the capability.
> It strikes me somewhat odd that this is the _first_ such capability > that needs a variable here to control if it is advertised or not > depending on the configuration file settings. > In other words, "Why doesn't transfer.advertiseSID also have a > similar thing in this file?" should be the first question that comes > to any reviewers' mind. It doesn't exist in protocol-caps.c, but instead in serve.c. Would be a good discussion for whether the advertise function should be serve.c or protocol-caps.c > "If unset(yet), try to get_bool() and if it fails, set it to 0" > So advertise_object_info becomes true only if transfer.advertiseObjectInfo > is set to true? This was the bug I introduced and will reroll On Tue, Mar 29, 2022 at 3:34 PM Junio C Hamano <gitster@pobox.com> wrote: > > Calvin Wan <calvinwan@google.com> writes: > > > Currently, object-info is always advertised by the server. Add a > > config option to optionally advertise it. A subsequent patch uses this > > option for testing. > > > > --- > > Documentation/config/transfer.txt | 4 ++++ > > protocol-caps.c | 11 +++++++++++ > > protocol-caps.h | 6 ++++++ > > serve.c | 2 +- > > 4 files changed, 22 insertions(+), 1 deletion(-) > > > > diff --git a/Documentation/config/transfer.txt b/Documentation/config/transfer.txt > > index b49429eb4d..14892a3155 100644 > > --- a/Documentation/config/transfer.txt > > +++ b/Documentation/config/transfer.txt > > @@ -77,3 +77,7 @@ transfer.unpackLimit:: > > transfer.advertiseSID:: > > Boolean. When true, client and server processes will advertise their > > unique session IDs to their remote counterpart. Defaults to false. > > + > > +transfer.advertiseObjectInfo:: > > + Boolean. When true or not set, server processes will advertise its > > + ability to accept `object-info` command requests > > \ No newline at end of file > > Carefully proofread before sending your changes out ;-) > > By mimicking the style of the previous item more closely, i.e. > > Boolean. When true, ... requests. Defaults to true. > > it would make it easier to read, I suspect. > > > diff --git a/protocol-caps.c b/protocol-caps.c > > index bbde91810a..f290e3cca2 100644 > > --- a/protocol-caps.c > > +++ b/protocol-caps.c > > @@ -8,6 +8,9 @@ > > #include "object-store.h" > > #include "string-list.h" > > #include "strbuf.h" > > +#include "config.h" > > + > > +static int advertise_object_info = -1; > > It strikes me somewhat odd that this is the _first_ such capability > that needs a variable here to control if it is advertised or not > depending on the configuration file settings. > > In other words, "Why doesn't transfer.advertiseSID also have a > similar thing in this file?" should be the first question that comes > to any reviewers' mind. > > Perhaps we just invented a better way to handle such conditional > capability, in which case we may want to port the existing ones that > do not use the same arrangement over to the new way. Even though it > would be outside the scope of the series, it would be a good way to > make sure we won't accumulate too much technical debt to leave some > hint somewhere near here or in protoco-caps.h where this patch > touches. I cannot quite tell what is going on. > > > int object_info_advertise(struct repository *r, struct strbuf *value) > > { > > if (advertise_object_info == -1 && > > git_config_get_bool("transfer.advertiseObjectInfo", &advertise_object_info)) > > advertise_object_info = 0; > > return advertise_object_info; > > } > > "If unset(yet), try to get_bool() and if it fails, set it to 0" > > So advertise_object_info becomes true only if transfer.advertiseObjectInfo > is set to true? > > It is inconsistent with what we read in the documentation, isn't it? > > > diff --git a/protocol-caps.h b/protocol-caps.h > > index 15c4550360..36f7a46b37 100644 > > --- a/protocol-caps.h > > +++ b/protocol-caps.h > > @@ -5,4 +5,10 @@ struct repository; > > struct packet_reader; > > int cap_object_info(struct repository *r, struct packet_reader *request); > > > > +/* > > + * Advertises object-info capability if "objectinfo.advertise" is either > > + * set to true or not set > > + */ > > +int object_info_advertise(struct repository *r, struct strbuf *value); > > + > > #endif /* PROTOCOL_CAPS_H */ > > diff --git a/serve.c b/serve.c > > index b3fe9b5126..fb4ad367a7 100644 > > --- a/serve.c > > +++ b/serve.c > > @@ -133,7 +133,7 @@ static struct protocol_capability capabilities[] = { > > }, > > { > > .name = "object-info", > > - .advertise = always_advertise, > > + .advertise = object_info_advertise, > > .command = cap_object_info, > > }, > > }; > > This is a good step to add a test to see if the server does honor > the (1) lack of (2) true set to and (3) false set to the > configuration variable and sends (or does not send) the capability.
On Tue, Mar 29, 2022 at 03:48:28PM -0700, Calvin Wan wrote: > > It strikes me somewhat odd that this is the _first_ such capability > > that needs a variable here to control if it is advertised or not > > depending on the configuration file settings. > > > In other words, "Why doesn't transfer.advertiseSID also have a > > similar thing in this file?" should be the first question that comes > > to any reviewers' mind. > > It doesn't exist in protocol-caps.c, but instead in serve.c. Would be > a good discussion for whether the advertise function should be serve.c > or protocol-caps.c Most of the others are defined statically within serve.c, but there are a couple of exceptions: - `ls_refs_advertise()` is defined in ls-refs.c, and - `upload_pack_advertise()` is defined in upload-pack.c. Even though cap_object_info() is defined in protocol-caps.c, I think we should probably keep the definition of `object_info_advertise()` local to serve.c. If there were a object-info.c, it would make sense to put it there, but since there isn't, it makes more sense to keep it local to serve.c, which avoids us having to declare the function in a header file. Thanks, Taylor
diff --git a/Documentation/config/transfer.txt b/Documentation/config/transfer.txt index b49429eb4d..14892a3155 100644 --- a/Documentation/config/transfer.txt +++ b/Documentation/config/transfer.txt @@ -77,3 +77,7 @@ transfer.unpackLimit:: transfer.advertiseSID:: Boolean. When true, client and server processes will advertise their unique session IDs to their remote counterpart. Defaults to false. + +transfer.advertiseObjectInfo:: + Boolean. When true or not set, server processes will advertise its + ability to accept `object-info` command requests \ No newline at end of file diff --git a/protocol-caps.c b/protocol-caps.c index bbde91810a..f290e3cca2 100644 --- a/protocol-caps.c +++ b/protocol-caps.c @@ -8,6 +8,9 @@ #include "object-store.h" #include "string-list.h" #include "strbuf.h" +#include "config.h" + +static int advertise_object_info = -1; struct requested_info { unsigned size : 1; @@ -111,3 +114,11 @@ int cap_object_info(struct repository *r, struct packet_reader *request) return 0; } + +int object_info_advertise(struct repository *r, struct strbuf *value) +{ + if (advertise_object_info == -1 && + git_config_get_bool("transfer.advertiseObjectInfo", &advertise_object_info)) + advertise_object_info = 0; + return advertise_object_info; +} diff --git a/protocol-caps.h b/protocol-caps.h index 15c4550360..36f7a46b37 100644 --- a/protocol-caps.h +++ b/protocol-caps.h @@ -5,4 +5,10 @@ struct repository; struct packet_reader; int cap_object_info(struct repository *r, struct packet_reader *request); +/* + * Advertises object-info capability if "objectinfo.advertise" is either + * set to true or not set + */ +int object_info_advertise(struct repository *r, struct strbuf *value); + #endif /* PROTOCOL_CAPS_H */ diff --git a/serve.c b/serve.c index b3fe9b5126..fb4ad367a7 100644 --- a/serve.c +++ b/serve.c @@ -133,7 +133,7 @@ static struct protocol_capability capabilities[] = { }, { .name = "object-info", - .advertise = always_advertise, + .advertise = object_info_advertise, .command = cap_object_info, }, };