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

From: Jonathan Nieder <jrnieder@gmail.com>
Date: Apr 10, 2013, 23:30 UTC
Message-ID: <20130410233058.GI27070@google.com>
In-Reply-To: <CALkWK0mzbgFP7JnCP7=NCA1guGg8ayF-mn7WdJEZyYX5hgePFw@mail.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.

[...]

Show 8 quoted lines

>> +       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.

[...]

Show 6 quoted lines

>> +               /* 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