Re: [RFC/PATCH] rm: delete .gitmodules entry of submodules removed from the work tree

[Jonathan Nieder <jrnieder@gmail.com>]

Hi,

Thanks for looking it over.

Ramkumar Ramachandra wrote:

> - Why are you hard-coding ".gitmodules" instead of using a simple #define?

Advantage of ".gitmodules": it's obvious what it means.
Advantage of DOT_GITMODULES: protection against spelling errors.

Git has a lot of use of both styles of string constant, for better or
worse.  Consistency means following what the surrounding code does,
and making changes if appropriate in a separate patch.

> - Why are you returning -1, instead of an error() with a message?

I think the idea is that remove_path_from_gitmodules() is idempotent:
if that path isn't listed in gitmodules, that's considered fine and
.gitmodules is left alone, instead of making a user that tries to
first remove a .gitmodules file and then all submodules suffer.

Perhaps a return value of '0 if gitmodules unmodified, 1 if modified'
would make it clearer that this isn't an error condition.

[...]
>> +       path_option = unsorted_string_list_lookup(&config_name_for_path, path);
>> +       if (!path_option) {
>> +               warning(_("Could not find section in .gitmodules where path=%s"), path);
>> +               return -1;
>> +       }
>
> Repetition from your mv series.  Why copy-paste, when you can factor
> it out into a function?

Do you mean that update_path_in_gitmodules should treat newpath ==
NULL as a cue to remove that entry, or something similar?

> Why are you calling warning() and then returning -1?

Sure, "return warning(...)" is a good shortcut.

> warning() not work?)  How is it a warning if you just stop all
> processing and return?

Probably it shouldn't warn in this case.

>> +       strbuf_addstr(&sect, "submodule.");
>> +       strbuf_addstr(&sect, path_option->util);
>
> What do you have against strbuf_addf()?

I think both addf and addstr are pretty clear.  The implementation of
addf is more complicated, which can be relevant in performance-critical
code (not here).

> Why is your variable named "sect"?  Did you mean "section"?

I think both "sect" and "section" are pretty clear.

[...]
>> +               /* Maybe the user already did that, don't error out here */
>> +               warning(_("Could not remove .gitmodules entry for %s"), path);
>> +               return -1;
>
> Maybe the user already did what?  What happens if she didn't do "it"
> and failure is due to some other cause?

git_config_rename_section_in_file() can fail for the following reasons:

 * invalid new section name (NULL is valid, so doesn't apply here)
 * could not lock config file
 * write error
 * could not commit config file

If the old section is missing, it doesn't even fail (it just
returns 0).  So I agree: this should be an error instead of a warning.

Hope that helps,
Jonathan