Re: [PATCH] mv: allow moving nested submodules

From: Stefan Beller <sbeller@google.com>
Date: Apr 18, 2016, 21:26 UTC
Message-ID: <CAGZ79kbVTUzKmSa9KjEJyDuRCtW5rygQmSYMpssLUKYoO1ooSw@mail.gmail.com>
In-Reply-To: <xmqqfuui4o4e.fsf@gitster.mtv.corp.google.com>

On Mon, Apr 18, 2016 at 2:13 PM, Junio C Hamano <gitster@pobox.com> wrote:

Show 33 quoted lines

> Junio C Hamano <gitster@pobox.com> writes:
>
>> If ignore-errors is set and rename fails, this would fall through
>> and try to touch this codepath...
>>
>>>                      if (submodule_gitfile[i]) {
>>>                              if (submodule_gitfile[i] != SUBMODULE_WITH_GITDIR)
>>>                                      connect_work_tree_and_git_dir(dst, submodule_gitfile[i]);
>>
>> ... but I am not sure if this thing is prepared to cope with such a
>> case?  src should have been moved to dst but if rename() failed we
>> wouldn't see what we expect at dst, or would we?
>
> In other words, I was wondering if this part should read more like
> this:
>
>  builtin/mv.c | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/builtin/mv.c b/builtin/mv.c
> index aeae855..37ed0fc 100644
> --- a/builtin/mv.c
> +++ b/builtin/mv.c
> @@ -252,9 +252,14 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
>                 int pos;
>                 if (show_only || verbose)
>                         printf(_("Renaming %s to %s\n"), src, dst);
> -               if (!show_only && mode != INDEX) {
> -                       if (rename(src, dst) < 0 && !ignore_errors)
> +               if (show_only)
> +                       ;
> +               else {
> +                       if (mode != INDEX && rename(src, dst) < 0) {

I agree until here.

> +                               if (ignore_errors)
> +                                       continue;
>                                 die_errno(_("renaming '%s' failed"), src);

This I thought would be better as:

    if (!ignore_errors)
        die_errno(...);

and not continue, but continuing is the right thing I would expect.

Speaking of which, connect_work_tree_and_git_dir as well as update_path_in_gitmodules need to learn about the ignore_errors flag, too. You would expect them to not die at the faintest problem, but rather honor the promise of "Skip move or rename actions which would lead to an error condition."

Thanks for a starting pointer for a new patch! Stefan

> +                       }
>                         if (submodule_gitfile[i]) {
>                                 if (submodule_gitfile[i] != SUBMODULE_WITH_GITDIR)
>                                         connect_work_tree_and_git_dir(dst, submodule_gitfile[i]);