Re: [PATCH] submodule--helper.c: remove duplicate code

[Stefan Beller <sbeller@google.com>]

On Wed, Mar 8, 2017 at 9:44 AM,  <me@vtolstov.org> wrote:
> From: Valery Tolstov <me@vtolstov.org>
>
> Remove code fragment from module_clone that duplicates functionality
> of connect_work_tree_and_git_dir in dir.c
>
> Signed-off-by: Valery Tolstov <me@vtolstov.org>
> ---
>>> I think we can reuse code from module_clone that writes .git link.
>>> Possibly this code fragment needs to be factored out from module_clone
>>
>> That fragment already exists, see dir.h:
>> connect_work_tree_and_git_dir(work_tree, git_dir);
>> Maybe another good microproject is to use that in module_clone.
>
> By suggestion of Stefan Beller I would like to make this micro
> improvement as my microproject for GSoc.

Thanks for looking into this code deduplication!

Well these two parts of the code are subtly different, though.
When applying this patch to origin/master (3bc53220cb2 is the
last time I fetched from Junio), then

    $ make
    $ cd t
    $ prove --timer --jobs 25 ./t[0-8][0-9]*.sh
... lots of output...
 Test Summary Report
-------------------
./t3600-rm.sh                               (Wstat: 256 Tests: 79 Failed: 24)
  Failed tests:  39-40, 43-59, 61-65
  Non-zero exit status: 1
./t4059-diff-submodule-not-initialized.sh   (Wstat: 256 Tests: 8 Failed: 5)
  Failed tests:  4-8
  Non-zero exit status: 1
./t7001-mv.sh                               (Wstat: 256 Tests: 47 Failed: 7)
  Failed tests:  39-45
  Non-zero exit status: 1
./t7412-submodule-absorbgitdirs.sh          (Wstat: 256 Tests: 11 Failed: 6)
  Failed tests:  3-7, 9
  Non-zero exit status: 1
./t7400-submodule-basic.sh                  (Wstat: 256 Tests: 90 Failed: 13)
  Failed tests:  46-47, 49, 75, 79-87
  Non-zero exit status: 1
./t7406-submodule-update.sh                 (Wstat: 256 Tests: 52 Failed: 14)
  Failed tests:  5, 28-31, 33-34, 36-39, 43, 45-46
  Non-zero exit status: 1

When then running one of them with debug output
(See t/README for the debug flags, I remember divx,
the video format as a sufficient set of flags to get enough debug output)

    $ ./t7406-submodule-update.sh -d -i -v -x
...
++ cd foo
++ git submodule deinit -f sub
Cleared directory 'sub'
Submodule 'foo/sub' (/usr/local/google/home/sbeller/OSS/git/t/trash
directory.t7406-submodule-update/withsubs/../rebasing) unregistered
for path 'sub'
++ git submodule update --init sub
+ test_eval_ret_=1
+ want_trace
+ test t = t
+ test t = t
+ set +x
error: last command exited with $?=1
not ok 5 - submodule update --init from and of subdirectory
#
# git init withsubs &&
# (cd withsubs &&
# mkdir foo &&
# git submodule add "$(pwd)/../rebasing" foo/sub &&
# (cd foo &&
#  git submodule deinit -f sub &&
#  git submodule update --init sub 2>../../actual2
# )
# ) &&
# test_i18ncmp expect2 actual2
#

$ cd trash\ directory.t7406-submodule-update/withsubs/foo/
$  git submodule deinit -f sub
Cleared directory 'sub'
Submodule 'foo/sub' (/usr/local/google/home/sbeller/OSS/git/t/trash
directory.t7406-submodule-update/withsubs/../rebasing) unregistered
for path 'sub'

$ git submodule update --init sub
Submodule 'foo/sub' (/usr/local/google/home/sbeller/OSS/git/t/trash
directory.t7406-submodule-update/withsubs/../rebasing) registered for
path 'sub'
fatal: could not get submodule directory for
'/usr/local/google/home/sbeller/OSS/git/t/trash
directory.t7406-submodule-update/withsubs/foo/sub'
Failed to clone 'foo/sub'. Retry scheduled
fatal: could not get submodule directory for
'/usr/local/google/home/sbeller/OSS/git/t/trash
directory.t7406-submodule-update/withsubs/foo/sub'
Failed to clone 'foo/sub' a second time, aborting


So I think we're missing to create the directory? (Just guessing)

Maybe we need to have 2293f77a081
(connect_work_tree_and_git_dir: safely create leading directories,
part of origin/sb/checkout-recurse-submodules, also found at
https://public-inbox.org/git/20170306205919.9713-8-sbeller@google.com/ )
first before we can apply this patch.

Thanks,
Stefan

>
>  builtin/submodule--helper.c | 22 +++-------------------
>  1 file changed, 3 insertions(+), 19 deletions(-)
>
> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> index 899dc334e..cda8a3bc1 100644
> --- a/builtin/submodule--helper.c
> +++ b/builtin/submodule--helper.c
> @@ -579,7 +579,6 @@ static int module_clone(int argc, const char **argv, const char *prefix)
>         const char *name = NULL, *url = NULL, *depth = NULL;
>         int quiet = 0;
>         int progress = 0;
> -       FILE *submodule_dot_git;
>         char *p, *path = NULL, *sm_gitdir;
>         struct strbuf rel_path = STRBUF_INIT;
>         struct strbuf sb = STRBUF_INIT;
> @@ -653,27 +652,12 @@ static int module_clone(int argc, const char **argv, const char *prefix)
>                 strbuf_reset(&sb);
>         }
>
> -       /* Write a .git file in the submodule to redirect to the superproject. */
> -       strbuf_addf(&sb, "%s/.git", path);
> -       if (safe_create_leading_directories_const(sb.buf) < 0)
> -               die(_("could not create leading directories of '%s'"), sb.buf);
> -       submodule_dot_git = fopen(sb.buf, "w");
> -       if (!submodule_dot_git)
> -               die_errno(_("cannot open file '%s'"), sb.buf);
> -
> -       fprintf_or_die(submodule_dot_git, "gitdir: %s\n",
> -                      relative_path(sm_gitdir, path, &rel_path));
> -       if (fclose(submodule_dot_git))
> -               die(_("could not close file %s"), sb.buf);
> -       strbuf_reset(&sb);
> -       strbuf_reset(&rel_path);
> -
> -       /* Redirect the worktree of the submodule in the superproject's config */
>         p = git_pathdup_submodule(path, "config");
>         if (!p)
>                 die(_("could not get submodule directory for '%s'"), path);
> -       git_config_set_in_file(p, "core.worktree",
> -                              relative_path(path, sm_gitdir, &rel_path));
> +
> +       /* Connect module worktree and git dir */
> +       connect_work_tree_and_git_dir(path, sm_gitdir);
>
>         /* setup alternateLocation and alternateErrorStrategy in the cloned submodule if needed */
>         git_config_get_string("submodule.alternateLocation", &sm_alternate);
> --
> 2.12.0.190.g250ed7eaf
>