Re: [GSoC PATCH v5 2/5] repo: add the field references.format

On Sun, Jul 27, 2025 at 1:52 PM Lucas Seiki Oshiro <lucasseikioshiro@gmail.com> wrote:

Based upon the implementation, I can see that the user must type the key in "dotted" form:

    git repo info references.format

but I wonder if the above documentation actually conveys this requirement. I don't think I would figure it out easily. Perhaps hand-holding the user by giving an example would help.

How can we ensure that the lexicographical-order requirement won't break? If someone adds a new entry which is not in its proper place, presumably that will be noticed because some existing test in a test script will stop working, but it feels unnecessarily fragile and a bit of a maintenance burden. Also, this requirement does feel like a premature optimization. Do you expect this list to become so huge and the corresponding lookup function to be called so frequently that a simple brute-force linear search would be too slow?

I can see from the implementation that you are sorting the incoming arguments in order to detect and fold out duplicates. However, that raises a couple questions. First, is it really a good idea to do something other than what the user asked for? Second, if this is a good idea, then should the behavior be documented?

I can see arguments in favor of sorting and de-duplicating, as well as in favor of producing exactly the (unordered) output the user asked for, including duplicates, so I don't have a strong opinion either way. But, if you do retain this behavior, then the sorting and deduplication behaviors should probably be documented.

A couple observations:

First, you don't actually use the strbuf until the call to get_value(), so the strbuf_init() call seems to be too early, with the result that you need a corresponding strbuf_release() in the error branch. If you move the strbuf_init() so it occurs immediately before get_value(), then you can simplify the early exit case.

Second, this seems to be getting unnecessarily intimate with strbuf. I can guess that you're doing this late strbuf_init() to avoid an allocation in the case when a duplicate key was encountered, however, STRBUF_INIT doesn't actually perform an allocation, so it would be clearer to just initialize the strbuf at the time you declare it rather than calling strbuf_init() manually. However...

Although it is a micro optimization (as well as a premature-optimization), it is far more common in this code base to hoist the strbuf outside of the loop and instead call strbuf_reset() it upon each iteration:

    struct strbuf value = STRBUF_INIT;
    for (...) {
        strbuf_reset(&value);
        ...
        if (error_condition) {
            strbuf_release(...);
            return error(...);
        }
       ...
    }
    strbuf_release(...);

Looking at this from a higher level, is it presenting a good user-experience by potentially printing some output but then erroring out upon the first unrecognized key? Would the user-experience be improved by instead continuing the loop even after reporting an error, and then adjusting the final `return 0` to conditionally return success or error depending upon whether any keys were unrecognized?

This is talking about null-terminated format, but the implementation doesn't seem to emit NUL-terminated output at all.

It seems that this could be simplified by crafting the expected output more precisely?

    eval ... &&
    echo "$key=$expected_value" >expect &&
    git -C repo repo info "$key" >actual &&
    test_cmp expect actual

By the way, we typically avoid cleaning up test detritus merely for the sake of cleaning up because doing so slows down the already too-slow test suite. Instead, cleanup is usually only performed when absolutely necessary to avoid some undesirable interaction between tests. Avoiding cleanup also makes it easier to debug failed tests since (hopefully) the cause of the failure is still present in the "trash" directory.

In this case, if you call this function with a distinct repository name each time, then you don't have to remove the repository at all. Moreover, giving each repository a distinct and _meaningful_ name, rather than reusing the same name, could also be helpful when diagnosing failures.

This is overly fragile. The `test_repo_info` function hardcodes the name "repo" but then the caller is also expected to pass in the name as part of the initialization argument (i.e. `git init ... repo`). To make this more robust, either don't hardcode it in the function, or stop expecting the caller to supply the name as part of the initialization argument.

With only two callers, it's not clear at this point whether the `test_repo_info` function is providing any added value, especially since the additional abstraction increases cognitive load, but perhaps later patches in this series add more callers?

This test title can be single-quoted rather than double-quoted.

As above, it seems that this could be simplified by crafting the expected state more precisely rather than slicing and dicing the actual output. Perhaps something like this?

    val=$(git rev-parse --show-ref-format) &&
    echo "references.format=$val" >expect &&
    git repo info references.format references.format >actual &&
    test_cmp expect actual

Aside from the above tests, based upon the implementation, I also expected to find a test checking that the command correctly outputs multiple values, but perhaps a later patch adds that since, presently, the implementation only knows "references.format" (thus with deduplication, you can't yet implement such a test).