> On Aug 19, 2022, at 02:51, Calvin Wan <calvinwan@xxxxxxxxxx> wrote:
>
> 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.
First, I really appreciate that you spent your precious time reviewing my
patches and tell me where is not enough and how to do it. I will check
my patches again and I wish I can update it with better descriptions in
a week (I can do it only in the evening time so it need couple days).
>
> 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.
Yes, Junio had given me some important comments just like yours and I
still very seriously consider how to solve them. I will split up my changes
and write more explanations.
>
> For your tests, they should show a working example of thie feature, the
> motivation behind the feature, and a description of the interface. The
Thanks, will do it.
> 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.
Thanks, I will refactor the tests and add more descriptions to make them easily
understand.
>
>> `hide-refs` to hide the private data.
>
> Why is Gerrit being centralized relevant to ref-level access control?
Will explain it in my next new update why I think so.
>
>> 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?
I will try to learn about the differences between them and answer the
question here.
>
>> +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].
I will not be discouraged, and I’m very glad and appreciate that I can receive
important review notes from Junio and the mailing list. I want to do more
contributions to git and wish one day I can help to review other patches. But
first I will fix my patches and make it more clearly.
I need to reply first and wishing not making noise, because I think it will
take me couple days to resolve the review comments and update the patches again.
Thanks again.
>
> [1] <CAJoAoZmsuwYCA8XGziEA-qwghg9h22Af98JQE1AuHHBRfQgrDA@xxxxxxxxxxxxxx>
>
