Jens Lehmann wrote:
> builtin/rm.c | 14 +++++++--
> submodule.c | 31 ++++++++++++++++++++
> submodule.h | 1 +
> t/t3600-rm.sh | 72 ++++++++++++++++++++++++++++++++++++++++++----
> t/t7400-submodule-basic.sh | 14 +++------
> t/t7610-mergetool.sh | 6 ++--
> 6 files changed, 117 insertions(+), 21 deletions(-)
Should be very similar to your mv series, as it's essentially the same
challenge.
> diff --git a/submodule.c b/submodule.c
> index 8ce6a7d..6b01a02 100644
> --- a/submodule.c
> +++ b/submodule.c
> @@ -63,6 +63,37 @@ int update_path_in_gitmodules(const char *oldpath, const char *newpath)
> return 0;
> }
>
> +/*
> + * Try to remove the "submodule.<name>" section from .gitmodules where the
> + * given path is configured.
> + */
> +int remove_path_from_gitmodules(const char *path)
> +{
> + struct strbuf sect = STRBUF_INIT;
> + struct string_list_item *path_option;
> +
> + if (!file_exists(".gitmodules")) /* Do nothing whithout .gitmodules */
> + return -1;
- Why are you doing this here? Is there no initialization function?
- Why are you hard-coding ".gitmodules" instead of using a simple #define?
- Why are you returning -1, instead of an error() with a message?
> + if (gitmodules_is_unmerged)
> + die(_("Cannot change unmerged .gitmodules, resolve merge conflicts first"));
Again, no sanity-checking initialization code?
> + 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?
Why are you calling warning() and then returning -1? (does return
warning() not work?) How is it a warning if you just stop all
processing and return?
> + strbuf_addstr(§, "submodule.");
> + strbuf_addstr(§, path_option->util);
What do you have against strbuf_addf()? I noticed a lot of ugly
addstr() calls in your mv series also.
Why is your variable named "sect"? Did you mean "section"?
> + if (git_config_rename_section_in_file(".gitmodules", sect.buf, NULL) < 0) {
You hardcoded ".gitmodules" again!
> + /* 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?
Thanks.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html