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 v5 1/5] hiderefs: add hide-refs hook to hide refs dynamically
Date: Tue, 13 Sep 2022 10:01:28 -0700	[thread overview]
Message-ID: <xmqqedwf9pjr.fsf@gitster.g> (raw)
In-Reply-To: <278bd185aec26285f8c00aca838f89e5f3877748.1662735985.git.gitgitgadget@gmail.com> (Sun Chao via GitGitGadget's message of "Fri, 09 Sep 2022 15:06:21 +0000")

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

  reply	other threads:[~2022-09-13 18:00 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
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 [this message]
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=xmqqedwf9pjr.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).