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

From: 孙超 <16657101987@163.com>
Date: Aug 16, 2022, 11:22 UTC
Message-ID: <AB93B5DC-7F78-4A55-BBA4-7CB10CAAFEF3@163.com>
In-Reply-To: <xmqqa6851ic9.fsf@gitster.g>

Show 10 quoted lines

> On Aug 16, 2022, at 00:02, Junio C Hamano <gitster@pobox.com> wrote:
> 
> Not "right now".
> 
> Instead of flooding the list with repeated "oops that was wrong"
> updates, it may be more effective use of others' time to wait for
> more feedback before acting on them, and to take time to proofread
> the result of your updates before sending them out.
> 
> Thanks.

Got it, I'll take more time in reviewing my patch updates in the future.

Show 6 quoted lines

> On Aug 16, 2022, at 02:18, Junio C Hamano <gitster@pobox.com> wrote:
> 
> If the prefix is a sign to let the external process to tell if it is
> to be hidden or shown, it does not sound like "force" at all, at
> least to me ("force" sounds more like "no matter what other things
> may want to show it, these are hidden").

I’ve read the codes about refs hidden and I had 3 ideas in mind, but I
didn't know which is better: 1) create a new config item (like transfer.forceHideRefsByHook),
2) call `hide-refs` hook directly to check all refs if it exists, 3) add a new prefix option
to `transfer.hiderefs` to call the new hook.

I choose the third one but it is indeed hard to understand and the `force` is not appropriate.

Show 15 quoted lines

> Are there places where only ref_is_hidden() is called, or do
> codepaths that used to care ref_is_hidden() now all have to write
> the above (A || B) conditional?  I am wondering why the new
> "force-hidden" check is not part of ref_is_hidden() so that the
> callers do not have to care.
> 
>> @@ -1794,7 +1794,8 @@ static void reject_updates_to_hidden(struct command *commands)
>> 		strbuf_setlen(&refname_full, prefix_len);
>> 		strbuf_addstr(&refname_full, cmd->ref_name);
>> 
>> -		if (!ref_is_hidden(cmd->ref_name, refname_full.buf))
>> +		if (!ref_is_hidden(cmd->ref_name, refname_full.buf) &&
>> +			!ref_is_force_hidden(cmd->ref_name, refname_full.buf))
> 
> Likewise.

In `mark_our_ref` I call ref_is_force_hidden() and ref_is_hidden() separately for a ref, and put a new `HIDDEN_REF_FORCE` bit to the object flags if the ref is force hidden, then in `has_unreachable` function the objects with this flag bit will be considered as 'not ancestors of our ref’. And I do not want to change the original mechanism of ref_is_hidden(), this is why I add a new object flag bit `HIDDEN_REF_FORCE`.

Show 9 quoted lines

> It used to be that send_ref() did not touch the object flag bits.
> It just said "if it is hidden, or if it is outside the namespace, do
> not show and return" before telling the other side about the ref,
> and even the ref we send to the other side, we did not muck with
> flag bits with OUR_REF bit (and we didn't touch HIDDEN_REF bit,
> either).
> 
> Now we do.  How can it be determined if this change is correct and
> safe?

I’m not very confident about the changes either so I have added some test cases for upload-pack V1 and V2, but I think I need add more test cases for other commands after considering your comment here.

Show 7 quoted lines

> 
> If the ref is not hidden (either in the traditional sense, or with
> the new "force" sense), we do not return 0.  What if it is outside
> the namespace so we returned without sending it to the other side?
> The original code didn't touch the flags bit, but now we mark the
> object with OUR_REF bit even though we ended up not sending the ref
> to the other side.  Is that an intended change?

Yes, this is a intended change here.

If I understand correctly, command `ls-refs` is called during protocol V2, I mark the object flags here and use them when responding to command `fetch`, and I call `check_non_tip()` before send the `want` data to the client which will check the force hidden objects.

>> +		if ((len >= forcelen) && !strncmp(value, "force:", forcelen)) {
> 
> skip_prefix() would probably be a good API function to learn here, perhaps?
>

Will fix it.

Show 8 quoted lines

> 
>> +static struct child_process *hide_refs_proc;
>> +static struct packet_reader *hide_refs_reader;
>> +static void create_hide_refs_process(void) {
> 
> Style.  The braces around a function block occupy their own line by
> themselves.
>

Will fix it. And I will read the style document again before update the patches.

> 
> No need for braces around a single statement block.
>

Will fix it.

> Is that a condition worth dying, indicating a misconfiguration by
> the user?  Or would it make more sense to treat as if the process
> says no refs are hidden (or all refs are hidden)?

Thanks for your question here, I will think about it later. Just like `pre-receive` hook, git server will skip it if it does not exists. I think we should not die here.

> 
> I do not think we spell "cannot" as "can not" in our messages.

Will fix it.

Show 5 quoted lines

> 
>> +	proc = (struct child_process *) xcalloc (1, sizeof (struct child_process));
>> +	reader = (struct packet_reader *) xcalloc (1, sizeof(struct packet_reader));
> 
> Style.  No SP after xcalloc, or sizeof.

Will fix it.

Show 9 quoted lines

>> +	code = start_command(proc);
>> +	if (code)
>> +		die("can not run hook hide-refs");
> 
> Unusually named variable.  I think "code" here is a variable
> normally called "status" (or "ret" if it eventually becomes the
> return value from this function).  Shouldn't this function return an
> error and have it handled by its caller, by the way, instead of
> returning void and making liberal calls to die()?

Will do.

Show 46 quoted lines

> 
>> +	code = packet_write_fmt_gently(proc->in, "version=1%c%s", '\0', hide_refs_section.buf);
>> +	if (!code)
>> +		code = packet_flush_gently(proc->in);
> 
> In general, it is a bad pattern to hide mainline "good case"
> processing inside "if (previous steps all went fine)" conditionals,
> as it makes the code unnecessarily hard to follow.
> 
> Instead, we typically write more like this:
> 
> -- >8 -- cut here -- >8 --
> 
> 	int ret = -1; /* assume failure */
> 
>        if (packet_write_fmt_gently(...))
> 		goto error_exit;
> 
> 	for (;;) {
> 		... interact with the other side ...
> 		if (error)
> 			goto error_exit;
> 	}
> 
> 	... continue with mainline "good case" processing ...
> 
> 	... after all went well ...
> 	ret = 0;
> 
> error_exit:
> 	if (ret < 0) {
> 		... emit error message ...
> 		... clean-up specific to error case if necessary
> 	}
> 	... clean-up as needed ...
> 
> 	return ret;
> 
> -- 8< -- cut here -- 8< --
> 
> I am not reviewing the rest of the patch in this sitting---I may
> later come back to continue reading it, but I'll stop here and send
> out comments on what I have seen first.
> 
> Thanks.
>

Thanks a lot, I Will do it.