> On Aug 16, 2022, at 00:02, Junio C Hamano <gitster@xxxxxxxxx> 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.
> On Aug 16, 2022, at 02:18, Junio C Hamano <gitster@xxxxxxxxx> 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.
> 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`.
> 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.
>
> 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.
>
>> +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.
>
>> + 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.
>> + 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.
>
>> + 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.
