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(§, "submodule.");
>> + strbuf_addstr(§, 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
