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

From: Ramkumar Ramachandra <artagnon@gmail.com>
Date: Apr 10, 2013, 22:24 UTC
Message-ID: <CALkWK0mzbgFP7JnCP7=NCA1guGg8ayF-mn7WdJEZyYX5hgePFw@mail.gmail.com>
In-Reply-To: <5165E1CC.7090004@web.de>

Jens Lehmann wrote:

Show 7 quoted lines

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

Show 19 quoted lines

> 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?

Show 5 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?

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(&sect, "submodule.");
> +       strbuf_addstr(&sect, 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.