Re: [PATCH v4 1/3] hide-refs: add hook to force hide refs

From: Calvin Wan <calvinwan@google.com>
Date: Aug 18, 2022, 18:51 UTC
Message-ID: <20220818185111.4062955-1-calvinwan@google.com>
In-Reply-To: <01c63ea5feefd57721bdcab9f0a30d9c0112e753.1660575688.git.gitgitgadget@gmail.com>

Hi Sun,

A couple of us from the mailing list reviewed your patch yesterday during review club and I'm going to summarize our thoughts here.

Starting with you commit message, it is not entirely clear what your series is trying to achieve. While you do attempt to set the scene in the first paragraph, it would be better to go into more detail of how a user would use this hook. Do you already have something like this working downstream for you at your company? If so, that would be a good reference to provide context for readers. If not, try to sell your use case better to us by providing examples and anything else this could be useful for. Your commit message should also have a broad description of the changes, explain difficult/tricky changes, and dicuss tradeoffs/complexity.

As Junio has noted, there is a lot going on here. For example, changes you make to pre-existing functionality should come with an explanation. One way to manage this complexity for reviewers is by splitting up your changes into more logically different commits.

For your tests, they should show a working example of thie feature, the motivation behind the feature, and a description of the interface. The structure of the tests is also confusing and there seem to be many unnecessary tests. It is OK to be verbose and obvious in tests -- it is very important for reviewers and others looking at your tests to easily understand what each test is doing.

"Sun Chao via GitGitGadget" <gitgitgadget@gmail.com> writes:

Show 7 quoted lines

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

Why is Gerrit being centralized relevant to ref-level access control?

Show 15 quoted lines

> 
> This hook would be invoked by 'git-receive-pack' and 'git-upload-pack'
> during the reference discovery phase, each reference will be filtered
> with this hook. The hook executes once with no arguments for each
> 'git-upload-pack' and 'git-receive-pack' process. Once the hook is invoked,
> a version number and server process name ('uploadpack' or 'receive') will
> send to it in pkt-line format, followed by a flush-pkt. The hook should
> respond with its version number.
> 
> During reference discovery phase, each reference will be filtered by this
> hook. In the following example, the letter 'G' stands for 'git-receive-pack'
> or 'git-upload-pack' and the letter 'H' stands for this hook. The hook
> decides if the reference will be hidden or not, it sends result back in
> pkt-line format protocol, a response "hide" means the references will hide
> to the client and can not fetch its private data even in protocol V2.

What is the reasoning behind special casing v2 here? Is it possible you're confusing remote helper protocol and wire protocol?

Show 6 quoted lines

> +static int lazy_load_hidden = 0;
> +// lazy load hidden refs for protocol V2
> +void lazy_load_hidden_refs(void) {
> +	lazy_load_hidden = 1;
> +}
> +

What does lazy_load_hidden do?

I know this is a lot to go thru for your first patch series, but please don't get discouraged! Feel free to ask any questions if you're confused about any of the feedback. We didn't dive too deeply into the specifics of your code since we believe there are higher level fundamental issues you should address first. There has also been similar discussion regarding differing ACLs within a single repository so it is probably worth a read here[1].

[1] <CAJoAoZmsuwYCA8XGziEA-qwghg9h22Af98JQE1AuHHBRfQgrDA@mail.gmail.com>