| Message ID | 20210807071613.99610-2-raykar.ath@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | submodule: convert the rest of 'add' to C | expand |
On 07/08/21 12:46 pm, Atharva Raykar wrote: > Let's modify the interface to `compute_submodule_clone_url()` function > by adding two more arguments, so that we can reuse this in various parts > of `submodule--helper.c` that follow a common pattern, which is--read > the remote url configuration of the superproject and then call > `relative_url()`. > > This function is nearly identical to `resolve_relative_url()`, the only > difference being the extra warning message. We can add a quiet flag to > it, ... It took me a while to figure what "it" meant in the above sentence. Does it refer to `compute_submodule_clone_url` or `resolve_relative_url`. After one sees the patch and takes a look at `resolve_relative_url`, it's clear the "it" indeed does refer to `resolve_relative_url`. But it might worth clarifying this in the commit message itself. Certainly not worth a re-roll on its own. May be Junio could amend this while queing ? > ... to suppress that warning when not needed, and then refactor > `resolve_relative_url()` by using this function, something we will do in > the next patch. > > Having this functionality factored out will be useful for converting the > rest of `submodule add` in subsequent patches. > > Signed-off-by: Atharva Raykar <raykar.ath@gmail.com> > Mentored-by: Christian Couder <christian.couder@gmail.com> > Mentored-by: Shourya Shukla <periperidip@gmail.com> Otherwise, this looks good.
On 08/08/21 11:11 pm, Kaartic Sivaraam wrote: > On 07/08/21 12:46 pm, Atharva Raykar wrote: >> Let's modify the interface to `compute_submodule_clone_url()` function >> by adding two more arguments, so that we can reuse this in various parts >> of `submodule--helper.c` that follow a common pattern, which is--read >> the remote url configuration of the superproject and then call >> `relative_url()`. >> >> This function is nearly identical to `resolve_relative_url()`, the only >> difference being the extra warning message. We can add a quiet flag to >> it, ... > > It took me a while to figure what "it" meant in the above sentence. Does it > refer to `compute_submodule_clone_url` or `resolve_relative_url`. After one > sees the patch and takes a look at `resolve_relative_url`, it's clear the "it" > indeed does refer to `resolve_relative_url`. But it might worth clarifying this > in the commit message itself. > > Certainly not worth a re-roll on its own. May be Junio could amend this while queing ? > Actually, I just noticed two other things which might be re-roll worthy. Read on ... > -static char *compute_submodule_clone_url(const char *rel_url) > +static char *compute_submodule_clone_url(const char *rel_url, const char *up_path, int quiet) > { > char *remoteurl, *relurl; I know this isn't new code. But there's already an argument names 'rel_url'. So, a variable named 'relurl' in the same scope is making it hard to distinguish between these two. Could you also try distinguishing these better by renaming 'relurl' to 'res' or something else? > char *remote = get_default_remote(); > @@ -598,10 +598,14 @@ static char *compute_submodule_clone_url(const char *rel_url) > > strbuf_addf(&remotesb, "remote.%s.url", remote); > if (git_config_get_string(remotesb.buf, &remoteurl)) { > - warning(_("could not look up configuration '%s'. Assuming this repository is its own authoritative upstream."), remotesb.buf); > + if (!quiet) > + warning(_("could not look up configuration '%s'. " > + "Assuming this repository is its own " > + "authoritative upstream."), > + remotesb.buf); > remoteurl = xgetcwd(); > } > - relurl = relative_url(remoteurl, rel_url, NULL); > + relurl = relative_url(remoteurl, rel_url, up_path); After reading 2/8 of the series, I just noticed that 'remoteurl' is always initialized in 'resolve_realtive_url'. It is either initialized to the return value of 'xgetcwd' or retains its assigned value of 'NULL'. But it looks like that's not the case here. 'remoteurl' could be used uninitialized when the above if block does not get executed which in turn could result in weird behaviour in case 'remoteurl' gets a value of anything other than 'NULL' at runtime. This again has nothing to do with the change done in this patch. Regardless, it looks like something worth correcting. Thus, I thought of pointing it out. > > free(remote); > free(remoteurl); > @@ -660,7 +664,7 @@ static void init_submodule(const char *path, const char *prefix, > if (starts_with_dot_dot_slash(url) || > starts_with_dot_slash(url)) { > char *oldurl = url; > - url = compute_submodule_clone_url(oldurl); > + url = compute_submodule_clone_url(oldurl, NULL, 0); > free(oldurl); > } >
Kaartic Sivaraam <kaartic.sivaraam@gmail.com> writes: > On 08/08/21 11:11 pm, Kaartic Sivaraam wrote: >> On 07/08/21 12:46 pm, Atharva Raykar wrote: >>> [...] >> It took me a while to figure what "it" meant in the above sentence. Does it >> refer to `compute_submodule_clone_url` or `resolve_relative_url`. After one >> sees the patch and takes a look at `resolve_relative_url`, it's clear the "it" >> indeed does refer to `resolve_relative_url`. But it might worth clarifying this >> in the commit message itself. >> Certainly not worth a re-roll on its own. May be Junio could amend this while >> queing ? >> > Actually, I just noticed two other things which might be re-roll worthy. Read on ... I'll keep re-rolling till the code is good, it's never a problem ;-) >> -static char *compute_submodule_clone_url(const char *rel_url) >> +static char *compute_submodule_clone_url(const char *rel_url, const char *up_path, int quiet) >> { >> char *remoteurl, *relurl; > > I know this isn't new code. But there's already an argument names > 'rel_url'. So, a variable named 'relurl' in the same scope is making it > hard to distinguish between these two. Could you also try distinguishing > these better by renaming 'relurl' to 'res' or something else? Okay. >> char *remote = get_default_remote(); >> @@ -598,10 +598,14 @@ static char *compute_submodule_clone_url(const char *rel_url) >> strbuf_addf(&remotesb, "remote.%s.url", remote); >> if (git_config_get_string(remotesb.buf, &remoteurl)) { >> - warning(_("could not look up configuration '%s'. Assuming this repository is its own authoritative upstream."), remotesb.buf); >> + if (!quiet) >> + warning(_("could not look up configuration '%s'. " >> + "Assuming this repository is its own " >> + "authoritative upstream."), >> + remotesb.buf); >> remoteurl = xgetcwd(); >> } >> - relurl = relative_url(remoteurl, rel_url, NULL); >> + relurl = relative_url(remoteurl, rel_url, up_path); > > After reading 2/8 of the series, I just noticed that 'remoteurl' is always > initialized in 'resolve_realtive_url'. It is either initialized to the return > value of 'xgetcwd' or retains its assigned value of 'NULL'. But it looks > like that's not the case here. 'remoteurl' could be used uninitialized > when the above if block does not get executed which in turn could result in > weird behaviour in case 'remoteurl' gets a value of anything other than 'NULL' > at runtime. > > This again has nothing to do with the change done in this patch. Regardless, it > looks like something worth correcting. Thus, I thought of pointing it out. > Right. I agree it should be corrected. >> free(remote); >> free(remoteurl); >> @@ -660,7 +664,7 @@ static void init_submodule(const char *path, const char *prefix, >> if (starts_with_dot_dot_slash(url) || >> starts_with_dot_slash(url)) { >> char *oldurl = url; >> - url = compute_submodule_clone_url(oldurl); >> + url = compute_submodule_clone_url(oldurl, NULL, 0); >> free(oldurl); >> } >>
Atharva Raykar <raykar.ath@gmail.com> writes: > Kaartic Sivaraam <kaartic.sivaraam@gmail.com> writes: > >> On 08/08/21 11:11 pm, Kaartic Sivaraam wrote: >>> On 07/08/21 12:46 pm, Atharva Raykar wrote: >>> [...] >>> char *remote = get_default_remote(); >>> @@ -598,10 +598,14 @@ static char *compute_submodule_clone_url(const char *rel_url) >>> strbuf_addf(&remotesb, "remote.%s.url", remote); >>> if (git_config_get_string(remotesb.buf, &remoteurl)) { >>> - warning(_("could not look up configuration '%s'. Assuming this repository is its own authoritative upstream."), remotesb.buf); >>> + if (!quiet) >>> + warning(_("could not look up configuration '%s'. " >>> + "Assuming this repository is its own " >>> + "authoritative upstream."), >>> + remotesb.buf); >>> remoteurl = xgetcwd(); >>> } >>> - relurl = relative_url(remoteurl, rel_url, NULL); >>> + relurl = relative_url(remoteurl, rel_url, up_path); >> >> After reading 2/8 of the series, I just noticed that 'remoteurl' is always >> initialized in 'resolve_realtive_url'. It is either initialized to the return >> value of 'xgetcwd' or retains its assigned value of 'NULL'. But it looks >> like that's not the case here. 'remoteurl' could be used uninitialized >> when the above if block does not get executed which in turn could result in >> weird behaviour in case 'remoteurl' gets a value of anything other than 'NULL' >> at runtime. >> >> This again has nothing to do with the change done in this patch. Regardless, it >> looks like something worth correcting. Thus, I thought of pointing it out. >> > > Right. I agree it should be corrected. Actually on having another look, I'm not sure if we need to assign NULL to 'remoteurl' at all. The 'if (git_config_get_string(...))' on success will allocate 'remoteurl'. If it fails, it will be given the return value of 'xgetcwd()'. There is nothing in the config API docs that suggest a success mode for the git_config_get_*() functions that will assign nothing to the buffer we give it. Therefore, by the time we get to the variable's first use in the 'relative_url()' function, we are guaranteed to have a well-defined value. It seems to me that the original 'resolve_relative_url()' had an unnecessary NULL initialization.
On 09/08/21 2:17 pm, Atharva Raykar wrote: > > Atharva Raykar <raykar.ath@gmail.com> writes: > >> Kaartic Sivaraam <kaartic.sivaraam@gmail.com> writes: >> >>> On 08/08/21 11:11 pm, Kaartic Sivaraam wrote: >>>> On 07/08/21 12:46 pm, Atharva Raykar wrote: >>>> [...] >>>> char *remote = get_default_remote(); >>>> @@ -598,10 +598,14 @@ static char *compute_submodule_clone_url(const char *rel_url) >>>> strbuf_addf(&remotesb, "remote.%s.url", remote); >>>> if (git_config_get_string(remotesb.buf, &remoteurl)) { >>>> - warning(_("could not look up configuration '%s'. Assuming this repository is its own authoritative upstream."), remotesb.buf); >>>> + if (!quiet) >>>> + warning(_("could not look up configuration '%s'. " >>>> + "Assuming this repository is its own " >>>> + "authoritative upstream."), >>>> + remotesb.buf); >>>> remoteurl = xgetcwd(); >>>> } >>>> - relurl = relative_url(remoteurl, rel_url, NULL); >>>> + relurl = relative_url(remoteurl, rel_url, up_path); >>> >>> After reading 2/8 of the series, I just noticed that 'remoteurl' is always >>> initialized in 'resolve_realtive_url'. It is either initialized to the return >>> value of 'xgetcwd' or retains its assigned value of 'NULL'. But it looks >>> like that's not the case here. 'remoteurl' could be used uninitialized >>> when the above if block does not get executed which in turn could result in >>> weird behaviour in case 'remoteurl' gets a value of anything other than 'NULL' >>> at runtime. >>> >>> This again has nothing to do with the change done in this patch. Regardless, it >>> looks like something worth correcting. Thus, I thought of pointing it out. >>> >> >> Right. I agree it should be corrected. > > Actually on having another look, I'm not sure if we need to assign NULL > to 'remoteurl' at all. > > The 'if (git_config_get_string(...))' on success will allocate > 'remoteurl'. If it fails, it will be given the return value of > 'xgetcwd()'. There is nothing in the config API docs that suggest a > success mode for the git_config_get_*() functions that will assign > nothing to the buffer we give it. Therefore, by the time we get to the > variable's first use in the 'relative_url()' function, we are guaranteed > to have a well-defined value. > Ah ha! That explains why we haven't got any reports about weird behaviours when using the likes of `git submodule init` so far ;-) Thanks for digging this and sorry about the false flag! > It seems to me that the original 'resolve_relative_url()' had an > unnecessary NULL initialization. > Makes sense. I guess I feel into the trap of blindly trusting that the original code was written correctly x-<
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c index 6891480013..2d2d8ac637 100644 --- a/builtin/submodule--helper.c +++ b/builtin/submodule--helper.c @@ -590,7 +590,7 @@ static int module_foreach(int argc, const char **argv, const char *prefix) return 0; } -static char *compute_submodule_clone_url(const char *rel_url) +static char *compute_submodule_clone_url(const char *rel_url, const char *up_path, int quiet) { char *remoteurl, *relurl; char *remote = get_default_remote(); @@ -598,10 +598,14 @@ static char *compute_submodule_clone_url(const char *rel_url) strbuf_addf(&remotesb, "remote.%s.url", remote); if (git_config_get_string(remotesb.buf, &remoteurl)) { - warning(_("could not look up configuration '%s'. Assuming this repository is its own authoritative upstream."), remotesb.buf); + if (!quiet) + warning(_("could not look up configuration '%s'. " + "Assuming this repository is its own " + "authoritative upstream."), + remotesb.buf); remoteurl = xgetcwd(); } - relurl = relative_url(remoteurl, rel_url, NULL); + relurl = relative_url(remoteurl, rel_url, up_path); free(remote); free(remoteurl); @@ -660,7 +664,7 @@ static void init_submodule(const char *path, const char *prefix, if (starts_with_dot_dot_slash(url) || starts_with_dot_slash(url)) { char *oldurl = url; - url = compute_submodule_clone_url(oldurl); + url = compute_submodule_clone_url(oldurl, NULL, 0); free(oldurl); } @@ -2134,7 +2138,7 @@ static int prepare_to_clone_next_submodule(const struct cache_entry *ce, if (repo_config_get_string_tmp(the_repository, sb.buf, &url)) { if (starts_with_dot_slash(sub->url) || starts_with_dot_dot_slash(sub->url)) { - url = compute_submodule_clone_url(sub->url); + url = compute_submodule_clone_url(sub->url, NULL, 0); need_free_url = 1; } else url = sub->url;
Let's modify the interface to `compute_submodule_clone_url()` function by adding two more arguments, so that we can reuse this in various parts of `submodule--helper.c` that follow a common pattern, which is--read the remote url configuration of the superproject and then call `relative_url()`. This function is nearly identical to `resolve_relative_url()`, the only difference being the extra warning message. We can add a quiet flag to it, to suppress that warning when not needed, and then refactor `resolve_relative_url()` by using this function, something we will do in the next patch. Having this functionality factored out will be useful for converting the rest of `submodule add` in subsequent patches. Signed-off-by: Atharva Raykar <raykar.ath@gmail.com> Mentored-by: Christian Couder <christian.couder@gmail.com> Mentored-by: Shourya Shukla <periperidip@gmail.com> --- builtin/submodule--helper.c | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-)