git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Junio C Hamano <gitster@pobox.com>
To: "Sun Chao via GitGitGadget" <gitgitgadget@gmail.com>
Cc: git@vger.kernel.org, Sun Chao <16657101987@163.com>,
	Sun Chao <sunchao9@huawei.com>
Subject: Re: [PATCH v4 1/3] hide-refs: add hook to force hide refs
Date: Mon, 15 Aug 2022 11:18:14 -0700	[thread overview]
Message-ID: <xmqqa6851ic9.fsf@gitster.g> (raw)
In-Reply-To: <01c63ea5feefd57721bdcab9f0a30d9c0112e753.1660575688.git.gitgitgadget@gmail.com> (Sun Chao via GitGitGadget's message of "Mon, 15 Aug 2022 15:01:26 +0000")

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?

> 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").

> 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.

> @@ -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.

> 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?

>  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.

> +	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.

> +	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()?

> +	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.

  reply	other threads:[~2022-08-15 18:52 UTC|newest]

Thread overview: 42+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-08-03 16:17 [PATCH 0/3] refs-advertise: add hook to filter advertised refs Sun Chao via GitGitGadget
2022-08-03 16:17 ` [PATCH 1/3] " Sun Chao via GitGitGadget
2022-08-03 16:17 ` [PATCH 2/3] t1419: add test cases for refs-advertise hook Sun Chao via GitGitGadget
2022-08-03 16:17 ` [PATCH 3/3] doc: add documentation for the " Sun Chao via GitGitGadget
2022-08-03 20:27 ` [PATCH 0/3] refs-advertise: add hook to filter advertised refs Junio C Hamano
2022-08-04  8:27   ` 孙超
2022-08-10  1:06 ` Jiang Xin
2022-08-10 13:09   ` 孙超
2022-08-15  0:54 ` [PATCH v2 0/3] hide-refs: add hook to force hide refs Sun Chao via GitGitGadget
2022-08-15  0:54   ` [PATCH v2 1/3] " Sun Chao via GitGitGadget
2022-08-15  0:54   ` [PATCH v2 2/3] t1419: add test cases for hide-refs hook Sun Chao via GitGitGadget
2022-08-15  0:54   ` [PATCH v2 3/3] doc: add documentation for the " Sun Chao via GitGitGadget
2022-08-15  4:12     ` Eric Sunshine
2022-08-15 14:49       ` 孙超
2022-08-15 16:02         ` Junio C Hamano
2022-08-15 14:56   ` [PATCH v3 0/3] hide-refs: add hook to force hide refs Sun Chao via GitGitGadget
2022-08-15 14:56     ` [PATCH v3 1/3] " Sun Chao via GitGitGadget
2022-08-15 14:56     ` [PATCH v3 2/3] t1419: add test cases for hide-refs hook Sun Chao via GitGitGadget
2022-08-15 14:56     ` [PATCH v3 3/3] doc: add documentation for the " Sun Chao via GitGitGadget
2022-08-15 15:01     ` [PATCH v4 0/3] hide-refs: add hook to force hide refs Sun Chao via GitGitGadget
2022-08-15 15:01       ` [PATCH v4 1/3] " Sun Chao via GitGitGadget
2022-08-15 18:18         ` Junio C Hamano [this message]
2022-08-16 11:22           ` 孙超
2022-08-18 18:51         ` Calvin Wan
2022-08-19 15:30           ` 孙超
2022-08-15 15:01       ` [PATCH v4 2/3] t1419: add test cases for hide-refs hook Sun Chao via GitGitGadget
2022-08-15 15:01       ` [PATCH v4 3/3] doc: add documentation for the " Sun Chao via GitGitGadget
2022-09-09 15:06       ` [PATCH v5 0/5] hiderefs: add hide-refs hook to hide refs dynamically Sun Chao via GitGitGadget
2022-09-09 15:06         ` [PATCH v5 1/5] " Sun Chao via GitGitGadget
2022-09-13 17:01           ` Junio C Hamano
2022-09-16 17:52             ` Junio C Hamano
2022-09-17  8:14               ` 孙超
2022-09-09 15:06         ` [PATCH v5 2/5] hiderefs: use new flag to mark force hidden refs Sun Chao via GitGitGadget
2022-09-09 15:06         ` [PATCH v5 3/5] hiderefs: hornor hide flags in wire protocol V2 Sun Chao via GitGitGadget
2022-09-09 15:06         ` [PATCH v5 4/5] test: add test cases for hide-refs hook Sun Chao via GitGitGadget
2022-09-09 15:06         ` [PATCH v5 5/5] doc: add documentation for the " Sun Chao via GitGitGadget
2022-09-20  8:22         ` [PATCH v6 0/5] hiderefs: add hide-refs hook to hide refs dynamically Sun Chao via GitGitGadget
2022-09-20  8:22           ` [PATCH v6 1/5] " Sun Chao via GitGitGadget
2022-09-20  8:22           ` [PATCH v6 2/5] hiderefs: use a new flag to mark force hidden refs Sun Chao via GitGitGadget
2022-09-20  8:22           ` [PATCH v6 3/5] hiderefs: hornor hide flags in wire protocol V2 Sun Chao via GitGitGadget
2022-09-20  8:22           ` [PATCH v6 4/5] test: add test cases for hide-refs hook Sun Chao via GitGitGadget
2022-09-20  8:22           ` [PATCH v6 5/5] doc: add documentation for the " Sun Chao via GitGitGadget

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=xmqqa6851ic9.fsf@gitster.g \
    --to=gitster@pobox.com \
    --cc=16657101987@163.com \
    --cc=git@vger.kernel.org \
    --cc=gitgitgadget@gmail.com \
    --cc=sunchao9@huawei.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).