Re: [PATCH v4 1/3] hide-refs: add hook to force hide refs
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Aug 15, 2022, 18:18 UTC
- Message-ID
- <xmqqa6851ic9.fsf@gitster.g>
- In-Reply-To
- <01c63ea5feefd57721bdcab9f0a30d9c0112e753.1660575688.git.gitgitgadget@gmail.com>
This step seems to do too many things at once and is hard to assess its correctness, I am afraid. Anybody who has deep understanding of the protocol involved share that impression?
Show 10 quoted lines
> To enable the `hide-refs` hook, we should config hiderefs with `force:` > option, eg: > > git config --add transfer.hiderefs force:refs/prefix1/ > git config --add uploadpack.hiderefs force:!refs/prefix2/ > > the `hide-refs` will be called during reference discovery phase and > check each matched reference, a 'hide' response means the reference will > be hidden for its private data even if `allowTipSHA1InWant` or > `allowReachableSHA1InWant` are set to true.
If the prefix is a sign to let the external process to tell if it is to be hidden or shown, it does not sound like "force" at all, at least to me ("force" sounds more like "no matter what other things may want to show it, these are hidden").
Show 11 quoted lines
> diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c > index 31b48e728be..16f2a21e97a 100644 > --- a/builtin/receive-pack.c > +++ b/builtin/receive-pack.c > @@ -296,7 +296,7 @@ static int show_ref_cb(const char *path_full, const struct object_id *oid, > struct oidset *seen = data; > const char *path = strip_namespace(path_full); > > - if (ref_is_hidden(path, path_full)) > + if (ref_is_hidden(path, path_full) || ref_is_force_hidden(path, path_full)) > return 0;
Are there places where only ref_is_hidden() is called, or do codepaths that used to care ref_is_hidden() now all have to write the above (A || B) conditional? I am wondering why the new "force-hidden" check is not part of ref_is_hidden() so that the callers do not have to care.
Show 7 quoted lines
> @@ -1794,7 +1794,8 @@ static void reject_updates_to_hidden(struct command *commands) > strbuf_setlen(&refname_full, prefix_len); > strbuf_addstr(&refname_full, cmd->ref_name); > > - if (!ref_is_hidden(cmd->ref_name, refname_full.buf)) > + if (!ref_is_hidden(cmd->ref_name, refname_full.buf) && > + !ref_is_force_hidden(cmd->ref_name, refname_full.buf))
Likewise.
Show 13 quoted lines
> diff --git a/ls-refs.c b/ls-refs.c > index 98e69373c84..b5cb1316d38 100644 > --- a/ls-refs.c > +++ b/ls-refs.c > @@ -84,7 +84,7 @@ static int send_ref(const char *refname, const struct object_id *oid, > > strbuf_reset(&data->buf); > > - if (ref_is_hidden(refname_nons, refname)) > + if (mark_our_ref(refname_nons, refname, oid)) > return 0; > > if (!ref_match(&data->prefixes, refname_nons))
It used to be that send_ref() did not touch the object flag bits. It just said "if it is hidden, or if it is outside the namespace, do not show and return" before telling the other side about the ref, and even the ref we send to the other side, we did not muck with flag bits with OUR_REF bit (and we didn't touch HIDDEN_REF bit, either).
Now we do. How can it be determined if this change is correct and safe?
If the ref is not hidden (either in the traditional sense, or with the new "force" sense), we do not return 0. What if it is outside the namespace so we returned without sending it to the other side? The original code didn't touch the flags bit, but now we mark the object with OUR_REF bit even though we ended up not sending the ref to the other side. Is that an intended change?
Show 18 quoted lines
> int parse_hide_refs_config(const char *var, const char *value, const char *section)
> {
> const char *key;
> + int force = 0;
> +
> if (!strcmp("transfer.hiderefs", var) ||
> (!parse_config_key(var, section, NULL, NULL, &key) &&
> !strcmp(key, "hiderefs"))) {
> char *ref;
> int len;
> + int forcelen;
>
> if (!value)
> return config_error_nonbool(var);
> +
> + forcelen = strlen("force:");
> + len = strlen(value);
> + if ((len >= forcelen) && !strncmp(value, "force:", forcelen)) {skip_prefix() would probably be a good API function to learn here, perhaps?
> +static struct child_process *hide_refs_proc;
> +static struct packet_reader *hide_refs_reader;
> +static void create_hide_refs_process(void) {Style. The braces around a function block occupy their own line by themselves.
Show 10 quoted lines
> + struct child_process *proc;
> + struct packet_reader *reader;
> + const char *hook_path;
> + int version = 0;
> + int code;
> +
> + hook_path = find_hook("hide-refs");
> + if (!hook_path) {
> + die("can not find hide-refs hook");
> + }No need for braces around a single statement block.
Is that a condition worth dying, indicating a misconfiguration by the user? Or would it make more sense to treat as if the process says no refs are hidden (or all refs are hidden)?
I do not think we spell "cannot" as "can not" in our messages.
> + proc = (struct child_process *) xcalloc (1, sizeof (struct child_process)); > + reader = (struct packet_reader *) xcalloc (1, sizeof(struct packet_reader));
Style. No SP after xcalloc, or sizeof.
Show 10 quoted lines
> + 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;
> +
> + code = start_command(proc);
> + if (code)
> + die("can not run hook hide-refs");Unusually named variable. I think "code" here is a variable normally called "status" (or "ret" if it eventually becomes the return value from this function). Shouldn't this function return an error and have it handled by its caller, by the way, instead of returning void and making liberal calls to die()?
Show 9 quoted lines
> + sigchain_push(SIGPIPE, SIG_IGN); > + > + /* Version negotiaton */ > + packet_reader_init(reader, proc->out, NULL, 0, > + PACKET_READ_CHOMP_NEWLINE | > + PACKET_READ_GENTLE_ON_EOF); > + code = packet_write_fmt_gently(proc->in, "version=1%c%s", '\0', hide_refs_section.buf); > + if (!code) > + code = packet_flush_gently(proc->in);
In general, it is a bad pattern to hide mainline "good case" processing inside "if (previous steps all went fine)" conditionals, as it makes the code unnecessarily hard to follow.
Instead, we typically write more like this:
-- >8 -- cut here -- >8 --
int ret = -1; /* assume failure */
if (packet_write_fmt_gently(...)) goto error_exit;
for (;;) {
... interact with the other side ...
if (error)
goto error_exit;
}... continue with mainline "good case" processing ...
... after all went well ... ret = 0;
error_exit:
if (ret < 0) {
... emit error message ...
... clean-up specific to error case if necessary
}
... clean-up as needed ...return ret;
-- 8< -- cut here -- 8< --
I am not reviewing the rest of the patch in this sitting---I may later come back to continue reading it, but I'll stop here and send out comments on what I have seen first.
Thanks.