Re: [PATCH 02/15] submodule: don't use submodule_from_name
Hi, sorry for the late reply, just stumpled upon this. On Mon, Jul 31, 2017 at 01:43:04PM -0700, Stefan Beller wrote: > On Sun, Jul 30, 2017 at 6:43 AM, Jens Lehmann wrote: > > Am 26.07.2017 um 23:06 schrieb Junio C Hamano: > >> > >> Stefan Beller writes: > >> > >>> 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.5010...@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. This was actually part of my original approach[1] but it seems I never got around to implement that last part for which I originally started the submodule config API: Proper recursive fetch. I still have a patch for moved submodules lying around which pass a commit id for a gitmodules file. That particular patch is quite simple and finished but I was planning to include that in the finished fetch series. So I can have a look if I can quickly update that to the current state, so we can at least have one proper user of the submodule config API. > > 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". I will have a look whether we can easily replace this hack with the proper lookup now. Lets see how many low hanging fruits we have lying around for recursive fetch. The full blown implementation including cloning of new submodules might still take some time... Cheers Heiko [1] https://public-inbox.org/git/f5baa2acc09531a16f4f693eebbe60706bb8ed1e.1361751905.git.hvo...@hvoigt.net/
Re: [PATCH 02/15] submodule: don't use submodule_from_name
On Sun, Jul 30, 2017 at 6:43 AM, Jens Lehmann wrote: > Am 26.07.2017 um 23:06 schrieb Junio C Hamano: >> >> Stefan Beller writes: >> >>> 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.5010...@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. > >> 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.
Re: [PATCH 02/15] submodule: don't use submodule_from_name
Jens Lehmann writes: >> 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. As long as the code after that point is prepared to see a NULL submodule and still behaves sensibly, then I would of course prefer not dying. Continuing with just a warning() may not be a safe thing to do if we are not prepared to see a NULL submodule after that point, though.
Re: [PATCH 02/15] submodule: don't use submodule_from_name
Am 26.07.2017 um 23:06 schrieb Junio C Hamano: Stefan Beller writes: 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.5010...@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. 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". 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. 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.
Re: [PATCH 02/15] submodule: don't use submodule_from_name
Stefan Beller writes: > 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.5010...@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. 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. > Note that this is the last caller of submodule_from_name being > removed, so I would expect removal of submodule_from_name from > the t/helper/test-submodule-config.c as well as > Documentation/technical/api-submodule-config.txt > in a later part of this series. (Well technically it could go outside > of the series, but in the mean time we'd document and test > dead code) Good thinking. As this is "cleanup" series, I think it is within its scope to remove an API function that becomes unused. > >> Signed-off-by: Brandon Williams >> --- >> submodule.c | 2 -- >> 1 file changed, 2 deletions(-) >> >> diff --git a/submodule.c b/submodule.c >> index 7e87e4698..fd391aea6 100644 >> --- a/submodule.c >> +++ b/submodule.c >> @@ -1177,8 +1177,6 @@ static int get_next_submodule(struct child_process *cp, >> continue; >> >> submodule = submodule_from_path(&null_oid, ce->name); >> - if (!submodule) >> - submodule = submodule_from_name(&null_oid, ce->name); >> >> default_argv = "yes"; >> if (spf->command_line_option == RECURSE_SUBMODULES_DEFAULT) { >> -- >> 2.14.0.rc0.400.g1c36432dff-goog >>
Re: [PATCH 02/15] submodule: don't use submodule_from_name
On Tue, Jul 25, 2017 at 2:39 PM, Brandon Williams wrote: > The function 'submodule_from_name()' is being used incorrectly here as a > submodule path is being used instead of a submodule name. Since the > correct function to use with a path to a submodule is already being used > ('submodule_from_path()') let's remove the call to > 'submodule_from_name()'. This blames to 851e18c385 (submodule: use new config API for worktree configurations, 2015-08-17), but that is a refactoring. The issue of using the path instead of a name was there before that. The actual issue was introduced in 7dce19d374 (fetch/pull: Add the --recurse-submodules option, 2010-11-12). + name = ce->name; + name_for_path = unsorted_string_list_lookup(&config_name_for_path, ce->name); + if (name_for_path) + name = name_for_path->util; 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.5010...@web.de/ I cc'd Jens in the hope of him having a good memory why he wrote the code that way. :) Note that this is the last caller of submodule_from_name being removed, so I would expect removal of submodule_from_name from the t/helper/test-submodule-config.c as well as Documentation/technical/api-submodule-config.txt in a later part of this series. (Well technically it could go outside of the series, but in the mean time we'd document and test dead code) > Signed-off-by: Brandon Williams > --- > submodule.c | 2 -- > 1 file changed, 2 deletions(-) > > diff --git a/submodule.c b/submodule.c > index 7e87e4698..fd391aea6 100644 > --- a/submodule.c > +++ b/submodule.c > @@ -1177,8 +1177,6 @@ static int get_next_submodule(struct child_process *cp, > continue; > > submodule = submodule_from_path(&null_oid, ce->name); > - if (!submodule) > - submodule = submodule_from_name(&null_oid, ce->name); > > default_argv = "yes"; > if (spf->command_line_option == RECURSE_SUBMODULES_DEFAULT) { > -- > 2.14.0.rc0.400.g1c36432dff-goog >
[PATCH 02/15] submodule: don't use submodule_from_name
The function 'submodule_from_name()' is being used incorrectly here as a submodule path is being used instead of a submodule name. Since the correct function to use with a path to a submodule is already being used ('submodule_from_path()') let's remove the call to 'submodule_from_name()'. Signed-off-by: Brandon Williams --- submodule.c | 2 -- 1 file changed, 2 deletions(-) diff --git a/submodule.c b/submodule.c index 7e87e4698..fd391aea6 100644 --- a/submodule.c +++ b/submodule.c @@ -1177,8 +1177,6 @@ static int get_next_submodule(struct child_process *cp, continue; submodule = submodule_from_path(&null_oid, ce->name); - if (!submodule) - submodule = submodule_from_name(&null_oid, ce->name); default_argv = "yes"; if (spf->command_line_option == RECURSE_SUBMODULES_DEFAULT) { -- 2.14.0.rc0.400.g1c36432dff-goog