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

> On Aug 19, 2022, at 02:51, Calvin Wan <calvinwan@google.com> 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.  

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

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