git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [PATCHv3 0/2] Fix relative path issues in recursive submodules.

From
Ramsay Jones <ramsay@ramsayjones.plus.com>
Date
Apr 1, 2016, 14:41 UTC
Message-ID
<56FE8898.20906@ramsayjones.plus.com>
In-Reply-To
<1459469849-9643-1-git-send-email-sbeller@google.com>
On 01/04/16 01:17, Stefan Beller wrote:
Show 7 quoted lines
> Thanks Junio for review!
> 
> v3:
>  * This is a resend of the last two patches of the series, i.e. it replaces
>    44859d6626d4 and efdef1e2e in sb/submodule-helper-clone-regression-fix
>   
>  * use absolute_path for sm_gitdir
Hi Stefan,

In response to v1 of this series, I sent you a fixup patch to suppress a sparse warning (submodule: don't use an integer as a NULL pointer, 21-02-2016).

In v2, you introduced a second identical warning (rather, for the same reason: using 0 as a NULL pointer as the second argument to strbuf_detach()).

I was just about to send another patch, when you sent this out. As a result, you have suppressed the new warning, but the original remains.

So, ...
Show 45 quoted lines
>  * removed todo
>  * we need to free the intermediate sm_gitdir, so rename that to sm_gitdir_rel
>    and free it afterwards.
>  * while we currently do not support an absolute `path`, we eventually might.
>    If `path` is absolute it would be a pointer to `argv`, which would lead to a
>    crash. Duplicate the path and the crash is prevented.
>  (* I thought we could use it as well for `path`, but we cannot; as 
>    get_git_work_tree() != cwd)
>  * diff to sb/submodule-helper-clone-regression-fix:
> 
> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> index 89cbbda..be7bf5f 100644
> --- a/builtin/submodule--helper.c
> +++ b/builtin/submodule--helper.c
> @@ -153,12 +153,13 @@ static int clone_submodule(const char *path, const char *gitdir, const char *url
>  
>  static int module_clone(int argc, const char **argv, const char *prefix)
>  {
> -	const char *path = NULL, *name = NULL, *url = NULL;
> +	const char *name = NULL, *url = NULL;
>  	const char *reference = NULL, *depth = NULL;
>  	int quiet = 0;
>  	FILE *submodule_dot_git;
> -	char *sm_gitdir, *p;
> -	struct strbuf rel_path = STRBUF_INIT; /* for relative_path */
> +	char *sm_gitdir_rel, *p, *path = NULL;
> +	const char *sm_gitdir;
> +	struct strbuf rel_path = STRBUF_INIT;
>  	struct strbuf sb = STRBUF_INIT;
>  
>  	struct option module_clone_options[] = {
> @@ -198,26 +199,14 @@ static int module_clone(int argc, const char **argv, const char *prefix)
>  		die(_("submodule--helper: unspecified or empty --path"));
>  
>  	strbuf_addf(&sb, "%s/modules/%s", get_git_dir(), name);
> -	sm_gitdir = strbuf_detach(&sb, NULL);
> -
> -
> -	if (!is_absolute_path(sm_gitdir)) {
> -		char *cwd = xgetcwd();
> -		strbuf_addf(&sb, "%s/%s", cwd, sm_gitdir);
> -		sm_gitdir = strbuf_detach(&sb, 0);
> -		free(cwd);
> -	}
> +	sm_gitdir_rel = strbuf_detach(&sb, NULL);
... this is good, but ...
Show 12 quoted lines
> +	sm_gitdir = absolute_path(sm_gitdir_rel);
>  
>  	if (!is_absolute_path(path)) {
> -		/*
> -		 * TODO: add prefix here once we allow calling from non root
> -		 * directory?
> -		 */
> -		strbuf_addf(&sb, "%s/%s",
> -			    get_git_work_tree(),
> -			    path);
> +		strbuf_addf(&sb, "%s/%s", get_git_work_tree(), path);
>  		path = strbuf_detach(&sb, 0);
... can you please fix this up.
Thanks!

ATB, Ramsay Jones

Show 72 quoted lines
> -	}
> +	} else
> +		path = xstrdup(path);
>  
>  	if (!file_exists(sm_gitdir)) {
>  		if (safe_create_leading_directories_const(sm_gitdir) < 0)
> @@ -240,6 +229,7 @@ static int module_clone(int argc, const char **argv, const char *prefix)
>  	submodule_dot_git = fopen(sb.buf, "w");
>  	if (!submodule_dot_git)
>  		die_errno(_("cannot open file '%s'"), sb.buf);
> +
>  	fprintf_or_die(submodule_dot_git, "gitdir: %s\n",
>  		       relative_path(sm_gitdir, path, &rel_path));
>  	if (fclose(submodule_dot_git))
> @@ -255,8 +245,8 @@ static int module_clone(int argc, const char **argv, const char *prefix)
>  			       relative_path(path, sm_gitdir, &rel_path));
>  	strbuf_release(&sb);
>  	strbuf_release(&rel_path);
> -	free(sm_gitdir);
> -
> +	free(sm_gitdir_rel);
> +	free(path);
>  	free(p);
>  	return 0;
>  }
> 
> v2:
>  * reworded commit message for test (Thanks Junio!)
>  * instead of "simplifying" the path handling in patch 2, we are prepared
>    for anything the user throws at us (i.e. instead of segfault we
>        die(_("submodule--helper: unspecified or empty --path"));
>    (Thanks Eric, Jacob for pushing back here!)
>  * reword commit message for patch 3 (safe_create_leading_directories_const is
>    not a check, Thanks Junio!)
>  * patch 4 (the fix): That got reworked completely. No flow controlling
>    conditions in the write out phase!
>  * patch 5 is a bonus! (replace fprintf by fprintf_or die) When implementing
>    that I wondered if we also have close_or_die and open_or_die, but that doesn't
>    seem to be the case.
> 
> Thanks,
> Stefan
> 
> v1:
> 
> It took me longer than expected to fix this bug.
> 
> It comes with a test and minor refactoring and applies on top of
> origin/sb/submodule-helper, such that it can be merged into 2.7, 2.8 as well
> as master.
> 
> Patch 1 is a test which fails; it has a similar layout as the
> real failing repository Norio Nomura pointed out, but simplified to the
> bare essentials for reproduction of the bug.
> 
> Patch 2&3 are not strictly necessary for fixing the isseu, but it removes
> stupid code I wrote, so the resulting code looks a little better.
> 
> Patch 4 fixes the issue by giving more information to relative_path,
> such that a relative path can be found in all cases.
> 
> Thanks,
> Stefan
> 
> Stefan Beller (2):
>   submodule--helper, module_clone: always operate on absolute paths
>   submodule--helper, module_clone: catch fprintf failure
> 
>  builtin/submodule--helper.c | 32 ++++++++++++++------------------
>  t/t7400-submodule-basic.sh  |  2 +-
>  2 files changed, 15 insertions(+), 19 deletions(-)
>
Previous: Stefan BellerNext: Stefan Beller
Message 8 of 15 in “[PATCHv3 0/2] Fix relative path issues in recursive submodules.”
  1. Stefan BellerApr 1, 2016
  2. 1/2 submodule--helper, module_clone: always operate on absolute pathsStefan Beller, Apr 1, 2016
  3. Junio C HamanoApr 1, 2016
  4. Junio C HamanoApr 1, 2016
  5. Eric SunshineApr 1, 2016
  6. Junio C HamanoApr 1, 2016
  7. 2/2 submodule--helper, module_clone: catch fprintf failureStefan Beller, Apr 1, 2016
  8. Ramsay JonesApr 1, 2016
  9. Stefan BellerApr 12, 2016
  10. Ramsay JonesApr 13, 2016
  11. Stefan BellerApr 13, 2016
  12. Junio C HamanoApr 13, 2016
  13. Stefan BellerApr 13, 2016
  14. Junio C HamanoApr 13, 2016
  15. Junio C HamanoApr 13, 2016

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.