| Message ID | 278bd185aec26285f8c00aca838f89e5f3877748.1662735985.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | hiderefs: add hide-refs hook to hide refs dynamically | expand |
"Sun Chao via GitGitGadget" <gitgitgadget@gmail.com> writes: > From: Sun Chao <sunchao9@huawei.com> > > Gerrit is implemented by JGit and is known as a centralized workflow system > which supports reference-level access control for repository. If we choose > to work in centralized workflow like what Gerrit provided, reference-level > access control is needed and we might add a reference filter hook > `hide-refs` to hide the private data. Please rewrite the above so that it does not sound like "Gerrit supports it, there are tons of users of Gerrit, we must support it, too". If this feature is meaningful for us, even if Gerrit folks were deprecating and planning to remove the support of it, we would add it. If it is not, even if Gerrit folks support it, we wouldn't. > + > + /* > + * the prefix 'hook:' means that the matched refs will be > + * checked by the hide-refs hook dynamically, we need to put > + * the 'ref' string to the hook_hide_refs list > + */ I am not sure if this deserves a five-line comment. We didn't need to have a comment that says "value without hook: means the matched refs will be hidden and we need to remember them in the hide_refs string_list" for over 10 years after all. > + if (skip_prefix(value, "hook:", &value)) { > + if (!strlen(value)) > + return error(_("missing value for '%s' after hook option"), var); I am not sure it is a good idea to special case an empty string, especially here at this point in the code flow. There would be strings that cannot be a refname prefix (e.g. "foo..bar") and such a check is better done at the place where the accumuldated list of ref patterns are actually used. If you are using prefix match, a value of an empty string here would be a very natural way to say "we pass all the refs through our hook". By the way, how does the negated entry work with this new one? For static ones, [transfer] hiderefs = !refs/heads/ would hide everything other than refs/heads/ hierarchy, I suppose. Would we spell [transfer] hiderefs = hook:!refs/heads/ or [transfer] hiderefs = !hook:refs/heads/ to say "send everything outside the branches to hook"? If the former, you'd also need to special case "!" the same way as you special case an empty string (in short, I am saying that the special case only for an empty string does not make much sense). How does this mechanism work with gitnamespaces (see "git config --help" and read on transfer.hideRerfs)? > + hook = 1; > + } > + > ref = xstrdup(value); > len = strlen(ref); > while (len && ref[len - 1] == '/') > ref[--len] = '\0'; > - if (!hide_refs) { > - CALLOC_ARRAY(hide_refs, 1); > - hide_refs->strdup_strings = 1; > + > + if (hook) { > + if (!hook_hide_refs) { > + CALLOC_ARRAY(hook_hide_refs, 1); > + hook_hide_refs->strdup_strings = 1; > + } > + string_list_append(hook_hide_refs, ref); > + } else { > + if (!hide_refs) { > + CALLOC_ARRAY(hide_refs, 1); > + hide_refs->strdup_strings = 1; > + } > + string_list_append(hide_refs, ref); > } > - string_list_append(hide_refs, ref); > } That's a somewhat duplicated code. I wonder /* no need for "hook" variable anymore */ struct string_list **refs_list= &hide_refs; if (strip "hook:" prefix from value) refs_list = &hook_hide_refs; ... if (!*refs_list) { *refs_list = xcalloc(1, sizeof(*refs_list)); (*refs_list)->strdup_strings = 1; } string_list_append(*refs_list, ref); would be a better organization. I dunno. > + > + /* > + * Once hide-refs hook is invoked, Git need to do version negotiation, > + * with it, version number and process name ('uploadpack' or 'receive') > + * will send to it in pkt-line format, the proccess name is recorded > + * by hide_refs_section > + */ Grammar. > + if (hook && hide_refs_section.len == 0) > + strbuf_addstr(&hide_refs_section, section); > + I am not sure if this is correct at all, but because the 1/N patch has only code without documentation I cannot guess the intention. The first conditional to parse the configuration variable name var tries to handle both generic transfer.hideRefs and direction specific {receive,uploadpack}.hideRefs and "section" at this point has "transfer", "receive" or "uploadpack", doesn't it? As this is a git_config() callback, when we have [receive] hiderefs = hook:refs/foo [uploadpack] hiderefs = hook:refs/bar [transfer] hiderefs = hook:refs/baz we would want to send refs/bar and refs/baz to the hook if we are a "uploadpack" process. But because the above code records the first section we happen to see (which is "receive"), hide_refs_section has that value. I am not sure how a code that later user that piece of information can behave any sensibly. Does it say "We are a 'uploadpack', but hide_refs_section says 'receive', so we should ignore what is in hook_hide_refs string list"? I'll stop reading at this point for now, as it is not a good use of our time to review the implementation until we know the basic design is sound, which I do not quite see from what we saw up to this point. It might have made sense if each string list element had the ref pattern to match as its value and stored extra info, like "is this negated?", "is this hook pattern or static?", "is this transfer, receive, or uploadpack?" in its .util member, for example. Thanks.
Junio C Hamano <gitster@pobox.com> writes: > The first conditional to parse the configuration variable name var > tries to handle both generic transfer.hideRefs and direction > specific {receive,uploadpack}.hideRefs and "section" at this point > has "transfer", "receive" or "uploadpack", doesn't it? This part of my review comments was based on my misreading the patch. Sorry. "section" comes from the caller and says either "receive" or "uploadpack". But because the updating of hide_refs_section is done outside that "first conditional" that makes sure we are actually looking at a "*.hiderefs" configuration variable, it is misleading. The only saving grace is that it is guarded by "hook" > + if (hook && hide_refs_section.len == 0) > + strbuf_addstr(&hide_refs_section, section); > + that is only set inside the body of the if statement of the first conditional that ensures that we are reading *.hiderefs variable, but it would make more sense to move it inside it. Or even better would be to clean the function up with a preliminary patch to return early when we are not looking at *.hiderefs variable, perhaps like the attached, and then build on top. refs.c | 39 ++++++++++++++++++++++----------------- 1 file changed, 22 insertions(+), 17 deletions(-) diff --git i/refs.c w/refs.c index c89d558892..8cf8c58ebd 100644 --- i/refs.c +++ w/refs.c @@ -1393,24 +1393,29 @@ static struct string_list *hide_refs; int parse_hide_refs_config(const char *var, const char *value, const char *section) { const char *key; - if (!strcmp("transfer.hiderefs", var) || - (!parse_config_key(var, section, NULL, NULL, &key) && - !strcmp(key, "hiderefs"))) { - char *ref; - int len; - - if (!value) - return config_error_nonbool(var); - ref = xstrdup(value); - len = strlen(ref); - while (len && ref[len - 1] == '/') - ref[--len] = '\0'; - if (!hide_refs) { - CALLOC_ARRAY(hide_refs, 1); - hide_refs->strdup_strings = 1; - } - string_list_append(hide_refs, ref); + char *ref; + int len; + + /* + * "section" is either "receive" or "uploadpack"; are we looking + * at transfer.hiderefs or $section.hiderefs? + */ + if (strcmp("transfer.hiderefs", var) && + !(!parse_config_key(var, section, NULL, NULL, &key) && + !strcmp(key, "hiderefs"))) + return 0; /* neither */ + if (!value) + return config_error_nonbool(var); + ref = xstrdup(value); + len = strlen(ref); + while (len && ref[len - 1] == '/') + ref[--len] = '\0'; + if (!hide_refs) { + CALLOC_ARRAY(hide_refs, 1); + hide_refs->strdup_strings = 1; } + string_list_append(hide_refs, ref); + return 0; }
> On Sep 14, 2022, at 01:01, Junio C Hamano <gitster@pobox.com> wrote: > >> Gerrit is implemented by JGit and is known as a centralized workflow system >> which supports reference-level access control for repository. If we choose >> to work in centralized workflow like what Gerrit provided, reference-level >> access control is needed and we might add a reference filter hook >> `hide-refs` to hide the private data. > > Please rewrite the above so that it does not sound like "Gerrit > supports it, there are tons of users of Gerrit, we must support it, > too". If this feature is meaningful for us, even if Gerrit folks > were deprecating and planning to remove the support of it, we would > add it. If it is not, even if Gerrit folks support it, we wouldn't. Hi Junio, thanks for your advice here, I cannot agree with you more, I will do it. > >> + >> + /* >> + * the prefix 'hook:' means that the matched refs will be >> + * checked by the hide-refs hook dynamically, we need to put >> + * the 'ref' string to the hook_hide_refs list >> + */ > > I am not sure if this deserves a five-line comment. We didn't need > to have a comment that says "value without hook: means the matched > refs will be hidden and we need to remember them in the hide_refs > string_list" for over 10 years after all. Agree, and I will remove them. > >> + if (skip_prefix(value, "hook:", &value)) { >> + if (!strlen(value)) >> + return error(_("missing value for '%s' after hook option"), var); > > I am not sure it is a good idea to special case an empty string, > especially here at this point in the code flow. There would be > strings that cannot be a refname prefix (e.g. "foo..bar") and such a > check is better done at the place where the accumuldated list of ref > patterns are actually used. If you are using prefix match, a value > of an empty string here would be a very natural way to say "we pass > all the refs through our hook". Yes, this is a good advice. Previously I cannot pass all the refs through the new hook unless set two config items like: [transfer] hiderefs = hook:HEAD hiderefs = hook:refs I thinks it is a good idea to use only one config item to replace them: [transfer] hiderefs = hook: > > By the way, how does the negated entry work with this new one? For > static ones, > > [transfer] hiderefs = !refs/heads/ > > would hide everything other than refs/heads/ hierarchy, I suppose. > Would we spell > > [transfer] hiderefs = hook:!refs/heads/ > > or > > [transfer] hiderefs = !hook:refs/heads/ > > to say "send everything outside the branches to hook"? If the > former, you'd also need to special case "!" the same way as you > special case an empty string (in short, I am saying that the special > case only for an empty string does not make much sense). In my patch I put the "!" after the "hook:", and negate passing all the refs to the hook would like [transfer] hiderefs = hook:! however according to the match mechanism of hiderefs, it will be better to delete the config item above. If there are no config item, the hook will not be called. So if I want to pass all the refs but some scope of them, it will be like (use a empty string to match all the refs) [transfer] hiderefs = hook: hiderefs = hook:!refs/pull/ which means pass all the refs except for the ones begins with 'refs/pull/' > How does this mechanism work with gitnamespaces (see "git config --help" > and read on transfer.hideRerfs)? In my patch Git will send refname and refnamefull(with namepsace) to the hook, the hook will check it and response with 'hide' or not. In the following example, the letter 'G' stands for 'git-receive-pack' or 'git-upload-pack' and the letter 'H' stands for this hook # Send reference filter request to hook G: PKT-LINE(ref <refname>:<refnamefull>) G: flush-pkt # Receive result from the hook. # Case 1: this reference is hidden H: PKT-LINE(hide) H: flush-pkt # Case 2: this reference can be advertised H: flush-pkt I'm not sure if it is suitable or not, I think it will be better to send both the refname and the refnamefull to the hook. > That's a somewhat duplicated code. I wonder > > /* no need for "hook" variable anymore */ > struct string_list **refs_list= &hide_refs; > > if (strip "hook:" prefix from value) > refs_list = &hook_hide_refs; > ... > if (!*refs_list) { > *refs_list = xcalloc(1, sizeof(*refs_list)); > (*refs_list)->strdup_strings = 1; > } > string_list_append(*refs_list, ref); > > would be a better organization. I dunno. Agree, it looks better, I will do it. > >> + >> + /* >> + * Once hide-refs hook is invoked, Git need to do version negotiation, >> + * with it, version number and process name ('uploadpack' or 'receive') >> + * will send to it in pkt-line format, the proccess name is recorded >> + * by hide_refs_section >> + */ > > Grammar. Will fix. > On Sep 17, 2022, at 01:52, Junio C Hamano <gitster@pobox.com> wrote: > > Junio C Hamano <gitster@pobox.com> writes: > > ... ... > >> + if (hook && hide_refs_section.len == 0) >> + strbuf_addstr(&hide_refs_section, section); >> + > > that is only set inside the body of the if statement of the first > conditional that ensures that we are reading *.hiderefs variable, > but it would make more sense to move it inside it. Agree, it will be better to move it inside. > > Or even better would be to clean the function up with a preliminary > patch to return early when we are not looking at *.hiderefs variable, > perhaps like the attached, and then build on top. > > ... ... > > + > + /* > + * "section" is either "receive" or "uploadpack"; are we looking > + * at transfer.hiderefs or $section.hiderefs? > + */ > + if (strcmp("transfer.hiderefs", var) && > + !(!parse_config_key(var, section, NULL, NULL, &key) && > + !strcmp(key, "hiderefs"))) > + return 0; /* neither */ > + if (!value) > + return config_error_nonbool(var); > + ref = xstrdup(value); > + len = strlen(ref); > + while (len && ref[len - 1] == '/') > + ref[--len] = '\0'; > + if (!hide_refs) { > + CALLOC_ARRAY(hide_refs, 1); > + hide_refs->strdup_strings = 1; > } > + string_list_append(hide_refs, ref); > + > return 0; > } Thanks for the advice here, I Will do it.
diff --git a/refs.c b/refs.c index 92819732ab7..a99734fedcd 100644 --- a/refs.c +++ b/refs.c @@ -8,6 +8,7 @@ #include "lockfile.h" #include "iterator.h" #include "refs.h" +#include "pkt-line.h" #include "refs/refs-internal.h" #include "run-command.h" #include "hook.h" @@ -1384,10 +1385,14 @@ char *shorten_unambiguous_ref(const char *refname, int strict) } static struct string_list *hide_refs; +static struct string_list *hook_hide_refs; +static struct strbuf hide_refs_section = STRBUF_INIT; int parse_hide_refs_config(const char *var, const char *value, const char *section) { const char *key; + int hook = 0; + if (!strcmp("transfer.hiderefs", var) || (!parse_config_key(var, section, NULL, NULL, &key) && !strcmp(key, "hiderefs"))) { @@ -1396,27 +1401,218 @@ int parse_hide_refs_config(const char *var, const char *value, const char *secti if (!value) return config_error_nonbool(var); + + /* + * the prefix 'hook:' means that the matched refs will be + * checked by the hide-refs hook dynamically, we need to put + * the 'ref' string to the hook_hide_refs list + */ + if (skip_prefix(value, "hook:", &value)) { + if (!strlen(value)) + return error(_("missing value for '%s' after hook option"), var); + hook = 1; + } + ref = xstrdup(value); len = strlen(ref); while (len && ref[len - 1] == '/') ref[--len] = '\0'; - if (!hide_refs) { - CALLOC_ARRAY(hide_refs, 1); - hide_refs->strdup_strings = 1; + + if (hook) { + if (!hook_hide_refs) { + CALLOC_ARRAY(hook_hide_refs, 1); + hook_hide_refs->strdup_strings = 1; + } + string_list_append(hook_hide_refs, ref); + } else { + if (!hide_refs) { + CALLOC_ARRAY(hide_refs, 1); + hide_refs->strdup_strings = 1; + } + string_list_append(hide_refs, ref); } - string_list_append(hide_refs, ref); } + + /* + * Once hide-refs hook is invoked, Git need to do version negotiation, + * with it, version number and process name ('uploadpack' or 'receive') + * will send to it in pkt-line format, the proccess name is recorded + * by hide_refs_section + */ + if (hook && hide_refs_section.len == 0) + strbuf_addstr(&hide_refs_section, section); + return 0; } -int ref_is_hidden(const char *refname, const char *refname_full) +static struct child_process *hide_refs_proc; +static struct packet_reader *hide_refs_reader; + +/* + * Create the hide-refs hook child process and complete version negotiation, + * return non-zero upon success, otherwise 0 + */ +static int create_hide_refs_process(void) +{ + struct child_process *proc; + struct packet_reader *reader; + const char *hook_path; + int version = 0; + int err; + + hook_path = find_hook("hide-refs"); + if (!hook_path) + return 0; + + proc = (struct child_process *)xcalloc(1, sizeof (struct child_process)); + reader = (struct packet_reader *)xcalloc(1, sizeof(struct packet_reader)); + + child_process_init(proc); + strvec_push(&proc->args, hook_path); + proc->in = -1; + proc->out = -1; + proc->trace2_hook_name = "hide-refs"; + proc->err = 0; + + err = start_command(proc); + if (err) + goto cleanup; + + sigchain_push(SIGPIPE, SIG_IGN); + + /* Version negotiaton */ + packet_reader_init(reader, proc->out, NULL, 0, + PACKET_READ_CHOMP_NEWLINE | PACKET_READ_GENTLE_ON_EOF); + err = packet_write_fmt_gently(proc->in, "version=1%c%s", '\0', hide_refs_section.buf); + if (!err) + err = packet_flush_gently(proc->in); + + if (!err) + for (;;) { + enum packet_read_status status; + + status = packet_reader_read(reader); + if (status != PACKET_READ_NORMAL) { + /* Check whether hide-refs exited abnormally */ + if (status == PACKET_READ_EOF) + goto failure; + break; + } + + if (reader->pktlen > 8 && starts_with(reader->line, "version=")) { + version = atoi(reader->line + 8); + } + } + + if (err) + goto failure; + + switch (version) { + case 0: + /* fallthrough */ + case 1: + break; + default: + trace_printf(_("hook hide-refs version '%d' is not supported"), version); + goto failure; + } + + sigchain_pop(SIGPIPE); + + hide_refs_proc = proc; + hide_refs_reader = reader; + return 1; + +failure: + close(proc->in); + close(proc->out); + kill(proc->pid, SIGTERM); + finish_command_in_signal(proc); + +cleanup: + free(proc); + free(reader); + sigchain_pop(SIGPIPE); + return 0; +} + +/* If hide-refs child process start failed, set skip_hide_refs_proc to true */ +static int skip_hide_refs_proc; + +/* + * Return non-zero if hide-refs hook want to hide the ref and 0 otherwise, + * and return 0 if hide-refs child proccess start failed or exit abnormally + */ +static int ref_hidden_check_by_hook(const char *refname, const char *refname_full) +{ + struct strbuf buf = STRBUF_INIT; + int err; + int ret = 0; + + if (skip_hide_refs_proc) + return 0; + + if (!hide_refs_proc) + if (!create_hide_refs_process()) { + skip_hide_refs_proc = 1; + return 0; + } + + sigchain_push(SIGPIPE, SIG_IGN); + err = packet_write_fmt_gently(hide_refs_proc->in, "ref %s:%s", refname, refname_full); + if (err) + goto cleanup; + + err = packet_flush_gently(hide_refs_proc->in); + if (err) + goto cleanup; + + for (;;) { + enum packet_read_status status; + + status = packet_reader_read(hide_refs_reader); + if (status != PACKET_READ_NORMAL) { + /* Check whether hide-refs exited abnormally */ + if (status == PACKET_READ_EOF) + goto cleanup; + break; + } + + strbuf_addstr(&buf, hide_refs_reader->line); + } + + if (!strncmp("hide", buf.buf, 4)) + ret = 1; + + sigchain_pop(SIGPIPE); + return ret; + +cleanup: + close(hide_refs_proc->in); + close(hide_refs_proc->out); + kill(hide_refs_proc->pid, SIGTERM); + finish_command_in_signal(hide_refs_proc); + + free(hide_refs_proc); + free(hide_refs_reader); + sigchain_pop(SIGPIPE); + + skip_hide_refs_proc = 1; + return 0; +} + +static int ref_hidden_check(const char *refname, const char *refname_full, int hook) { + struct string_list *hide_refs_list = hide_refs; int i; - if (!hide_refs) + if (hook) + hide_refs_list = hook_hide_refs; + + if (!hide_refs_list) return 0; - for (i = hide_refs->nr - 1; i >= 0; i--) { - const char *match = hide_refs->items[i].string; + for (i = hide_refs_list->nr - 1; i >= 0; i--) { + const char *match = hide_refs_list->items[i].string; const char *subject; int neg = 0; const char *p; @@ -1436,12 +1632,25 @@ int ref_is_hidden(const char *refname, const char *refname_full) /* refname can be NULL when namespaces are used. */ if (subject && skip_prefix(subject, match, &p) && - (!*p || *p == '/')) - return !neg; + (!*p || *p == '/')) { + if (neg) + return 0; + if (!hook) + return 1; + return ref_hidden_check_by_hook(refname, refname_full); + } } return 0; } +int ref_is_hidden(const char *refname, const char *refname_full) +{ + if (ref_hidden_check(refname, refname_full, 0) || + ref_hidden_check(refname, refname_full, 1)) + return 1; + return 0; +} + const char *find_descendant_ref(const char *dirname, const struct string_list *extras, const struct string_list *skip)