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@xxxxxxxxx> writes:
> From: Sun Chao <sunchao9@xxxxxxxxxx>
>
> 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?
>
> 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?
> +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@xxxxxxxxxxxxxx>
