Re: [PATCH 02/15] submodule: don't use submodule_from_name

On Sun, Jul 30, 2017 at 6:43 AM, Jens Lehmann [off-list ref] wrote:

Am 26.07.2017 um 23:06 schrieb Junio C Hamano:

quoted

Stefan Beller [off-list ref] writes:

quoted

Rereading the archives, there was quite some discussion on the design
of these patches, but these lines of code did not get any attention

     https://public-inbox.org/git/4CDB3063.5010801@web.de/

I cc'd Jens in the hope of him having a good memory why he
wrote the code that way. :)

Thanks for digging.  I wouldn't be surprised if this were a fallback
to help a broken entry in .gitmodules that lack .path variable, but
we shouldn't be sweeping the problem under the rug like that.

Sorry to disappoint you ;-) I added this in 7dce19d374 because
submodule by path lookup back then only parsed the checked out
.gitmodules file.

This is still the case AFAICT, as we never ask for a specific .gitmodules
file identified by sha1 of the commit.

So looking for it by name was a good guess to
fetch a new submodule that wasn't present in the current HEAD's
.gitmodules, as the path is used as the default name in "git
submodule add".

3 things:
a) I think it is not as much a feature ('fallback to still make it work'),
   but rather a bug as when there is no (or wrong) entry in the .gitmodules
   file, reporting it is better than trying something.
b) in the case of moved submodules (2 submodules swapped their path)
   this may be harmful as we'd get a wrong submodule potentially.

c) I wonder if we want to use a different default for submodule names
   as I have seen people get confused by path and name being the same,
   e.g. to move a submodule they would have not just adapted the path,
   but any occurrence of the string that reads like the path.
   (i.e. also change the name, defeating the purpose of name/path
   separation).

   For a new name default, I would wager for some non-legible gibberish
   such as "hash( path/time )", as that sends a clear message to not mess
   with the value of the name.

The refactoring in 851e18c385 could and should have removed that
because since then we use the .gitmodules path to name mapping
of the fetched commit.

quoted

I wonder if we should barf loudly if there shouldn't be a submodule
at that path, i.e.

        if (!submodule)
                die("there is no submodule defined for path '%s'"...);

though.

Not sure if you want to die() or just issue a warning(), but yes.

Either die() or "warning && return 0" is fine with me.