Hi Peff,
On Wed, 24 Oct 2018, Jeff King wrote:
> The upload_pack_config() callback uses an if/else chain
> like:
>
> if (!strcmp(var, "a"))
> ...
> else if (!strcmp(var, "b"))
> ...
> etc
>
> This works as long as the conditions are mutually exclusive,
> but one of them is not. 20b20a22f8 (upload-pack: provide a
> hook for running pack-objects, 2016-05-18) added:
>
> else if (current_config_scope() != CONFIG_SCOPE_REPO) {
> ... check some more options ...
> }
>
> That was fine in that commit, because it came at the end of
> the chain. But later, 10ac85c785 (upload-pack: add object
> filtering for partial clone, 2017-12-08) did this:
>
> else if (current_config_scope() != CONFIG_SCOPE_REPO) {
> ... check some more options ...
> } else if (!strcmp("uploadpack.allowfilter", var))
> ...
>
> We'd always check the scope condition first, meaning we'd
> _only_ respect allowfilter when it's in the repo config. You
> can see this with:
>
> git -c uploadpack.allowfilter=true upload-pack . | head -1
>
> which will not advertise the filter capability (but will
> after this patch). We never noticed because:
>
> - our tests always set it in the repo config
>
> - in protocol v2, we use a different code path that
> actually calls repo_config_get_bool() separately, so
> that _does_ work. Real-world people experimenting with
> this may be using v2.
>
> The more recent uploadpack.allowrefinwant option is in the
> same boat.
>
> There are a few possible fixes:
>
> 1. Bump the scope conditional back to the bottom of the
> chain. But that just means somebody else is likely to
> make the same mistake later.
>
> 2. Make the conditional more like the others. I.e.:
>
> else if (!current_config_scope() != CONFIG_SCOPE_REPO &&
> !strcmp(var, "uploadpack.notallowedinrepo"))
>
> This works, but the idea of the original structure was
> that we may grow multiple sensitive options like this.
>
> 3. Pull it out of the chain entirely. The chain mostly
> serves to avoid extra strcmp() calls after we've found
> a match. But it's not worth caring about those. In the
> worst case, when there isn't a match, we're already
> hitting every strcmp (and this happens regularly for
> stuff like "core.bare", etc).
>
> This patch does (3).
>
> Signed-off-by: Jeff King <peff@xxxxxxxx>
> ---
> Phew. That was a lot of explanation for a small patch, but
> this was sufficiently subtle I thought it worth it. And
> also, I was really surprised the bug managed to exist for
> this long without anybody noticing.
Maybe a lot of explanation, but definitely a good one. The explanation and
the patch look good to me.
Thanks,
Dscho
>
> upload-pack.c | 9 ++++++---
> 1 file changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/upload-pack.c b/upload-pack.c
> index 540778d1dd..489c18e222 100644
> --- a/upload-pack.c
> +++ b/upload-pack.c
> @@ -1028,14 +1028,17 @@ static int upload_pack_config(const char *var, const char *value, void *unused)
> keepalive = git_config_int(var, value);
> if (!keepalive)
> keepalive = -1;
> - } else if (current_config_scope() != CONFIG_SCOPE_REPO) {
> - if (!strcmp("uploadpack.packobjectshook", var))
> - return git_config_string(&pack_objects_hook, var, value);
> } else if (!strcmp("uploadpack.allowfilter", var)) {
> allow_filter = git_config_bool(var, value);
> } else if (!strcmp("uploadpack.allowrefinwant", var)) {
> allow_ref_in_want = git_config_bool(var, value);
> }
> +
> + if (current_config_scope() != CONFIG_SCOPE_REPO) {
> + if (!strcmp("uploadpack.packobjectshook", var))
> + return git_config_string(&pack_objects_hook, var, value);
> + }
> +
> return parse_hide_refs_config(var, value, "uploadpack");
> }
>
> --
> 2.19.1.1094.gd480080bf6
>
