Re "Bug Report: git submodule overwrites submodules of same name but different path"

[K Jayatheerth <jayatheerthkulkarni2005@gmail.com>]

> Dear list,  
>  

Hey Moritz,
Thanks for the report

> I have encountered a problem with `git submodule add` that leads to info  
> of a submodule being lost if another submodule of the same name is  
> added. This can happen if a submodule has existed on the path in an  
> earlier commit but has been moved (`git mv` edits `path=` in `.gitmodules` but  
> not the submodule name, probably to avoid moving the corresponding  
> folder in `.git/modules`)  
>  
> Find below the summary from "git bugreport" (Answers in English).  
>  
> Best wishes,  
> Moritz  
>  
>  
> # What did you do to reproduce the error  
>  
>
> git init parent  
> cd parent  
> git commit --allow-empty -m "initial commit"  
> git init child  
> git -C child commit --allow-empty -m "initial commit"  
> git submodule add https://example.com/child.git child  
> git commit -m "Add submodule child"  
>  
> git mv child child_old  
> git commit -m "Move child to child_old"  
>  
> git init child  
> git -C child commit --allow-empty -m "initial commit"  
> git submodule add https://example.com/child2.git child  
> git commit -m "Add a new submodule at child/"  
>  
> cat .gitmodules  
> 
>  

I could reproduce the behaviour, neat observation!!

> # What did you expect to happen  
>  
> I expect that both submodules have entries in the `.gitmodules` file.  
> Git submodule names are given by the path, so I expected some way  
> of resolving ambiguity, for example, Git appends ".path" in other  
> cases to disambiguate. So the expected `.gitmodules` content would  
> be:  
>  
> 
> [submodule "child"]  
>     path = child_old  
>     url = https://example.com/child.git  
> [submodule "child.path"]  
>     path = child  
>     url = https://example.com/child2.git  
> 
>  
> # What happened instead  
>  
> I see that there is only one entry in `.gitmodules` listing the  
> new submodule, under the name "child":  
>  
>  
> [submodule "child"]  
>     path = child  
>     url = https://example.com/child2.git  
> 
>  
> # How is this different from the expected result  
>  
> The old submodule (path = `child_old`) is not listed. The information  
> about the origin of the old subrepo is lost.  
>  
>  

I think the main issue is as follows 
in the module_add() function, 
add_data.sm_name defaults to the basename of the path you’re
adding, it re‑uses the already‑existing name “child” even though the
previous entry now points at child_old/.

Nothing in module_add() checks whether that name is already taken for a
different path, so the new write silently overwrites the old one.


The original code is something like this 

if (!add_data.sm_name)
	add_data.sm_name = add_data.sm_path;

if (check_submodule_name(add_data.sm_name))
	die(_("'%s' is not a valid submodule name"), add_data.sm_name);


Something like this in between works for me

const struct submodule *existing =
			submodule_from_name(the_repository,
					null_oid(the_hash_algo),
					add_data.sm_name);
	
			if (existing && strcmp(existing->path, add_data.sm_path)) {
				if (!force) {
					die(_("submodule name '%s' already used for path '%s' "
					      "(use --name to choose another or --force to overwrite)"),
					    add_data.sm_name, existing->path);
				}
				/* auto-disambiguate: append ".<basename>" */
				add_data.sm_name = xstrfmt("%s.%s",
							   add_data.sm_name,
							   basename(add_data.sm_path));
                        }


This is just a vague format. Just to compiler and check.

With this the behaviour now throws a fatal error and not change the 
existing submodules.

Something like this:
fatal: submodule name 'child' already used for path 'child_old' (use --name to choose another or --force to overwrite)
On branch master
Untracked files:
  (use "git add <file>..." to include in what will be committed)
        child/

nothing added to commit but untracked files present (use "git add" to track)

If this is good 
I can send a patch covering the patch with this specific fix and the test file
I don't think this needs a new test file, I will add in any existing one.

If anyone has a better idea I'm open to it too.

Thank You,

-Jayatheerth