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

Reply via email to