Re: [PATCH v3] submodule: fetch missing objects from default remote
- From
Nasser Grainawi <nasser.grainawi@oss.qualcomm.com>
- Date
- Feb 27, 2026, 18:29 UTC
- Message-ID
- <CAFcKa=9PLNDQcvM1bFq=8_nbP-Ha1qDVHSSwde=apiXTcAC+DQ@mail.gmail.com>
- In-Reply-To
- <xmqq5x8to53y.fsf@gitster.g>
On Thu, Jan 22, 2026 at 11:49 AM Junio C Hamano <gitster@pobox.com> wrote:
Show 35 quoted lines
>
> Nasser Grainawi <nasser.grainawi@oss.qualcomm.com> writes:
> >
> > diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> > index d537ab087a..b180a24091 100644
> > --- a/builtin/submodule--helper.c
> > +++ b/builtin/submodule--helper.c
> > @@ -112,6 +112,43 @@ static int get_default_remote_submodule(const char *module_path, char **default_
> > return 0;
> > }
> >
> > +static int module_get_default_remote(int argc, const char **argv, const char *prefix,
> > + struct repository *repo UNUSED)
> > +{
> > + const char *path;
> > + char *resolved_path = NULL;
> > + char *default_remote = NULL;
> > + int code;
> > + struct option options[] = {
> > + OPT_END()
> > + };
> > + const char *const usage[] = {
> > + N_("git submodule--helper get-default-remote <path>"),
> > + NULL
> > + };
> > +
> > + argc = parse_options(argc, argv, prefix, options, usage, 0);
> > + if (argc != 1)
> > + usage_with_options(usage, options);
>
> Hmph, I am not sure what is going on. What are we getting out of
> parse_options() here? Would it be the same to see if we got
> anything remaining on the command line by checking argc and call
> usage_with_options() without calling parse_options(), or am I
> missing something?I had found a few other places following this same pattern (for example: gc.c maintenance_stop() and notes.c list()) and I thought it was because parse_options() handles common options like '-h' and has standardized messages for errors like unknown options.
Show 16 quoted lines
> > + code = get_default_remote_submodule(path, &default_remote);
> > + if (code) {
> > + free(resolved_path);
> > + return code;
> > + }
> > +
> > + printf("%s\n", default_remote);
>
> Do we know that the value of default_remote has no funny bytes in
> it, like newline? In the end the name has to become part of
> refs/remotes/<name>/HEAD that has to be a valid refname, so not
> giving any facility to quote funny bytes and allowing the caller of
> this helper to assume a LF terminated single line should be fine, so
> I am guessing that the answer is yes, but I offhand do not know how
> we know that we do not have to worry about such a situation in the
> code path that begins with get_default_remote_submodule().I don't think we know that, or at least I can't find proof of it. All I can find is that remote.c issues a warning for remote names starting with '/'. That seems to miss cases tested in t0602-reffiles-fsck.sh. However, I'm not sure if this helper should be responsible for validating the remote name as that seems like something remote.c should be doing when parsing configs.
> Hmph, these overly long lines are eyesore. I wonder if we can do > something about them?
I'll send a fixed version.
Show 5 quoted lines
> I also wonder if we want to make sure we are getting from the remote > that is given the custom name in a more direct way (instead of "we > see that our fetch succeeds, and because there is no other remote, > it must have gotten what is needed from the renamed one"), or is it > too much paranoia?
I can capture the fetch command output and compare it to some expected output where we have the remote paths, but that still doesn't show the remote name. But it looks like I can inspect the GIT_TRACE output and compare the `git submodule--helper get-default-remote` and subsequent `git fetch` commands to expected output. I've added both methods to this new test and the existing test it was modeled on so that it's obvious there's a difference between them.
Show 29 quoted lines
>
> > +test_expect_success 'fetch new submodule commit on-demand in FETCH_HEAD from custom remote' '
> > + # depends on the previous test for setup
> > +
> > + C=$(git -C submodule commit-tree -m "another change outside refs/heads for custom remote" HEAD^{tree}) &&
> > + git -C submodule update-ref refs/changes/custom4 $C &&
> > + git update-index --cacheinfo 160000 $C submodule &&
> > + test_tick &&
> > +
> > + D=$(git -C sub1 commit-tree -m "another change outside refs/heads for custom remote" HEAD^{tree}) &&
> > + git -C sub1 update-ref refs/changes/custom5 $D &&
> > + git update-index --cacheinfo 160000 $D sub1 &&
> > +
> > + git commit -m "updated submodules outside of refs/heads" &&
> > + E=$(git rev-parse HEAD) &&
> > + git update-ref refs/changes/custom6 $E &&
> > + (
> > + cd downstream &&
> > + git fetch --recurse-submodules origin refs/changes/custom6 &&
> > + git -C submodule cat-file -t $C &&
> > + git -C sub1 cat-file -t $D &&
> > + git checkout --recurse-submodules FETCH_HEAD
> > + )
> > +'
>
> Are we testing anything new in this test, compared to the previous
> one? Both update the submodule sub1 by adding a new ref under
> refs/changes/ hierachy and have "git fetch --recurse-submodules"
> follow the changes.I think the only difference is not creating a new superproject ref under refs/heads/ when we fetch. I mirrored it after the test above 'fetch new submodule commit on-demand in FETCH_HEAD', but for the intent of testing this remote name feature, I don't think we need both. I don't mind dropping it if you think it's unnecessary.