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

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

[me]

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.

 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

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

[Stefan Beller]

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
>

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

[Valery Tolstov]

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

Thank you for your detailed responses. Yes, we difenitely need this
patch first. All tests passed when I applied it.


Regards,
  Valery Tolstov

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

[Stefan Beller]

On Wed, Mar 8, 2017 at 11:59 AM, Valery Tolstov <me@vtolstov.org> wrote:
>> 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.
>
> Thank you for your detailed responses. Yes, we difenitely need this
> patch first. All tests passed when I applied it.
>

Thanks for testing!

Then the next step (as outlined by Documentation/SubmittingPatches)
is to figure out how to best present this to the mailing list; I think the best
way is to send out a patch series consisting of both of these 2 patches,
the "connect_work_tree_and_git_dir: safely create leading directories,"
first and then your deduplication patch.

When applied in that order, then git passes the test suite  at all commits
(which is very nice when using e.g. git-bisect on git).

Thanks,
Stefan

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

[Valery Tolstov]

> Then the next step (as outlined by Documentation/SubmittingPatches)
> is to figure out how to best present this to the mailing list; I think the best
> way is to send out a patch series consisting of both of these 2 patches,
> the "connect_work_tree_and_git_dir: safely create leading directories,"
> first and then your deduplication patch.

Is there a handy way to forward your patch in new patch series? Also, 
should I start new thread for new patch series?


Regards,
  Valery Tolstov

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

[Stefan Beller]

On Wed, Mar 8, 2017 at 3:05 PM, Valery Tolstov <me@vtolstov.org> wrote:
>> Then the next step (as outlined by Documentation/SubmittingPatches)
>> is to figure out how to best present this to the mailing list; I think the best
>> way is to send out a patch series consisting of both of these 2 patches,
>> the "connect_work_tree_and_git_dir: safely create leading directories,"
>> first and then your deduplication patch.
>
> Is there a handy way to forward your patch in new patch series?

No there is not. :(

Here is what I would do:
* Take the patch and "git am" it (you probably did that for testing already.)
* use "git rebase --interactive" to get the order right,
* use "git format-patch HEAD^^ --cover-letter" to create the series.
    (This will take care of e.g. rewriting the patch to be numbered correctly)
* use "git send-email 00* --to=list --cc=Junio ..." etc to send it out
as a new series.

Alternatively you can take the patch file you sent and that patch and
manually edit their numbering and send them out.
(PATCH 1/2 and PATCH 2/2)

> Also,
> should I start new thread for new patch series?

As you like.
As far as I understand, it is very easy for Junio to take a whole
(sub-)thread of patches and apply that and make a branch with
multiple commits out of it as he has tooling for that.

>
> Regards,
>   Valery Tolstov

Thanks,
Stefan

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

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

>> Also,
>> should I start new thread for new patch series?
>
> As you like.
> As far as I understand, it is very easy for Junio to take a whole
> (sub-)thread of patches and apply that and make a branch with
> multiple commits out of it as he has tooling for that.

Note that the world does not revolve around _me_.  I was once asked
for my preference and I responded and that is what you are recalling
here.  

Others on the list do review and keeping it easy for them to is also
important.  What's _your_ preference?

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

[Stefan Beller]

On Wed, Mar 8, 2017 at 3:24 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Stefan Beller <sbeller@google.com> writes:
>
>>> Also,
>>> should I start new thread for new patch series?
>>
>> As you like.
>> As far as I understand, it is very easy for Junio to take a whole
>> (sub-)thread of patches and apply that and make a branch with
>> multiple commits out of it as he has tooling for that.
>
> Note that the world does not revolve around _me_.  I was once asked
> for my preference and I responded and that is what you are recalling
> here.
>
> Others on the list do review and keeping it easy for them to is also
> important.  What's _your_ preference?
>

I use gmail, that has a broken threading model
(it groups emails by subject lines; apparently not using any "in-reply-to"
relationship to build up a thread), so I do not care. At all.

I do not know about the preference of the next most likely
people to review submodule code, so I refrained from giving advice
depending on reviewer preference, but instead gave advice that would
ease your work as I recall.

After thinking about it further, you may want to make sure that the
topic is coherent and it is easy to discover all related emails in an
archive of the mailing list. https://public-inbox.org/git/

So in the cover letter you could link to the previous thread,
https://public-inbox.org/git/20170308174449.24266-1-me@vtolstov.org/
or just continue that thread by replying to an email that is appropriate for
the series.

Thanks,
Stefan

[PATCH v2 0/2] Remove duplicate code from module_clone()

[Valery Tolstov]

> Then the next step (as outlined by Documentation/SubmittingPatches)
> is to figure out how to best present this to the mailing list; I think the best
> way is to send out a patch series consisting of both of these 2 patches,
> the "connect_work_tree_and_git_dir: safely create leading directories,"
> first and then your deduplication patch.

Combined two patches

Stefan Beller (1):
  connect_work_tree_and_git_dir: safely create leading directories

Valery Tolstov (1):
  submodule--helper.c: remove duplicate code

 builtin/submodule--helper.c | 20 ++------------------
 dir.c                       | 32 +++++++++++++++++++++-----------
 submodule.c                 | 11 ++---------
 3 files changed, 25 insertions(+), 38 deletions(-)

-- 
2.12.0.192.gbdb9d28a5

[PATCH v2 1/2] connect_work_tree_and_git_dir: safely create leading directories

[Valery Tolstov]

From: Stefan Beller <sbeller@google.com>

In a later patch we'll use connect_work_tree_and_git_dir when the
directory for the gitlink file doesn't exist yet. This patch makes
connect_work_tree_and_git_dir safe to use for both cases of
either the git dir or the working dir missing.

To do so, we need to call safe_create_leading_directories[_const]
on both directories. However this has to happen before we construct
the absolute paths as real_pathdup assumes the directories to
be there already.

So for both the config file in the git dir as well as the .git link
file we need to
a) construct the name
b) call SCLD
c) get the absolute path
d) once a-c is done for both we can consume the absolute path
   to compute the relative path to each other and store those
   relative paths.

The implementation provided here puts a) and b) for both cases first,
and then performs c and d after.

One of the two users of 'connect_work_tree_and_git_dir' already checked
for the directory being there, so we can loose that check as
connect_work_tree_and_git_dir handles this functionality now.

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 dir.c       | 32 +++++++++++++++++++++-----------
 submodule.c | 11 ++---------
 2 files changed, 23 insertions(+), 20 deletions(-)

diff --git a/dir.c b/dir.c
index 4541f9e14..6f52af7ab 100644
--- a/dir.c
+++ b/dir.c
@@ -2728,23 +2728,33 @@ void untracked_cache_add_to_index(struct index_state *istate,
 /* Update gitfile and core.worktree setting to connect work tree and git dir */
 void connect_work_tree_and_git_dir(const char *work_tree_, const char *git_dir_)
 {
-	struct strbuf file_name = STRBUF_INIT;
+	struct strbuf gitfile_sb = STRBUF_INIT;
+	struct strbuf cfg_sb = STRBUF_INIT;
 	struct strbuf rel_path = STRBUF_INIT;
-	char *git_dir = real_pathdup(git_dir_);
-	char *work_tree = real_pathdup(work_tree_);
+	char *git_dir, *work_tree;
 
-	/* Update gitfile */
-	strbuf_addf(&file_name, "%s/.git", work_tree);
-	write_file(file_name.buf, "gitdir: %s",
-		   relative_path(git_dir, work_tree, &rel_path));
+	/* Prepare .git file */
+	strbuf_addf(&gitfile_sb, "%s/.git", work_tree_);
+	if (safe_create_leading_directories_const(gitfile_sb.buf))
+		die(_("could not create directories for %s"), gitfile_sb.buf);
+
+	/* Prepare config file */
+	strbuf_addf(&cfg_sb, "%s/config", git_dir_);
+	if (safe_create_leading_directories_const(cfg_sb.buf))
+		die(_("could not create directories for %s"), cfg_sb.buf);
 
+	git_dir = real_pathdup(git_dir_);
+	work_tree = real_pathdup(work_tree_);
+
+	/* Write .git file */
+	write_file(gitfile_sb.buf, "gitdir: %s",
+		   relative_path(git_dir, work_tree, &rel_path));
 	/* Update core.worktree setting */
-	strbuf_reset(&file_name);
-	strbuf_addf(&file_name, "%s/config", git_dir);
-	git_config_set_in_file(file_name.buf, "core.worktree",
+	git_config_set_in_file(cfg_sb.buf, "core.worktree",
 			       relative_path(work_tree, git_dir, &rel_path));
 
-	strbuf_release(&file_name);
+	strbuf_release(&gitfile_sb);
+	strbuf_release(&cfg_sb);
 	strbuf_release(&rel_path);
 	free(work_tree);
 	free(git_dir);
diff --git a/submodule.c b/submodule.c
index 3b98766a6..45e93a1d5 100644
--- a/submodule.c
+++ b/submodule.c
@@ -1445,8 +1445,6 @@ void absorb_git_dir_into_superproject(const char *prefix,
 
 	/* Not populated? */
 	if (!sub_git_dir) {
-		char *real_new_git_dir;
-		const char *new_git_dir;
 		const struct submodule *sub;
 
 		if (err_code == READ_GITFILE_ERR_STAT_FAILED) {
@@ -1469,13 +1467,8 @@ void absorb_git_dir_into_superproject(const char *prefix,
 		sub = submodule_from_path(null_sha1, path);
 		if (!sub)
 			die(_("could not lookup name for submodule '%s'"), path);
-		new_git_dir = git_path("modules/%s", sub->name);
-		if (safe_create_leading_directories_const(new_git_dir) < 0)
-			die(_("could not create directory '%s'"), new_git_dir);
-		real_new_git_dir = real_pathdup(new_git_dir);
-		connect_work_tree_and_git_dir(path, real_new_git_dir);
-
-		free(real_new_git_dir);
+		connect_work_tree_and_git_dir(path,
+			git_path("modules/%s", sub->name));
 	} else {
 		/* Is it already absorbed into the superprojects git dir? */
 		char *real_sub_git_dir = real_pathdup(sub_git_dir);
-- 
2.12.0.192.gbdb9d28a5

[PATCH v2 2/2] submodule--helper.c: remove duplicate code

[Valery Tolstov]

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>
---
 builtin/submodule--helper.c | 20 ++------------------
 1 file changed, 2 insertions(+), 18 deletions(-)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 899dc334e..405cbea07 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);
+	/* Connect module worktree and git dir */
+	connect_work_tree_and_git_dir(path, sm_gitdir);
 
-	/* 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));
 
 	/* setup alternateLocation and alternateErrorStrategy in the cloned submodule if needed */
 	git_config_get_string("submodule.alternateLocation", &sm_alternate);
-- 
2.12.0.192.gbdb9d28a5

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

[Brandon Williams]

On 03/09, Valery Tolstov wrote:
> 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>

Patch looks good all the tests pass when running this on top of
Stefan's checkout series 'origin/sb/checkout-recurse-submodules'.
There is still one more bit of cleanup that you can do.

> ---
>  builtin/submodule--helper.c | 20 ++------------------
>  1 file changed, 2 insertions(+), 18 deletions(-)
> 
> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> index 899dc334e..405cbea07 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;

rel_path is no longer used so it and the call to strbuf_release() to
free its memory can be removed.

-- 
Brandon Williams

[PATCH v3 0/2] Remove duplicate code from module_clone()

[Valery Tolstov]

Stefan Beller (1):
  connect_work_tree_and_git_dir: safely create leading directories

Valery Tolstov (1):
  submodule--helper.c: remove duplicate code

 builtin/submodule--helper.c | 22 ++--------------------
 dir.c                       | 32 +++++++++++++++++++++-----------
 submodule.c                 | 11 ++---------
 3 files changed, 25 insertions(+), 40 deletions(-)

-- 
2.12.0.192.gbdb9d28a5

[PATCH v3 1/2] connect_work_tree_and_git_dir: safely create leading directories

[Valery Tolstov]

From: Stefan Beller <sbeller@google.com>

In a later patch we'll use connect_work_tree_and_git_dir when the
directory for the gitlink file doesn't exist yet. This patch makes
connect_work_tree_and_git_dir safe to use for both cases of
either the git dir or the working dir missing.

To do so, we need to call safe_create_leading_directories[_const]
on both directories. However this has to happen before we construct
the absolute paths as real_pathdup assumes the directories to
be there already.

So for both the config file in the git dir as well as the .git link
file we need to
a) construct the name
b) call SCLD
c) get the absolute path
d) once a-c is done for both we can consume the absolute path
   to compute the relative path to each other and store those
   relative paths.

The implementation provided here puts a) and b) for both cases first,
and then performs c and d after.

One of the two users of 'connect_work_tree_and_git_dir' already checked
for the directory being there, so we can loose that check as
connect_work_tree_and_git_dir handles this functionality now.

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 dir.c       | 32 +++++++++++++++++++++-----------
 submodule.c | 11 ++---------
 2 files changed, 23 insertions(+), 20 deletions(-)

diff --git a/dir.c b/dir.c
index 4541f9e14..6f52af7ab 100644
--- a/dir.c
+++ b/dir.c
@@ -2728,23 +2728,33 @@ void untracked_cache_add_to_index(struct index_state *istate,
 /* Update gitfile and core.worktree setting to connect work tree and git dir */
 void connect_work_tree_and_git_dir(const char *work_tree_, const char *git_dir_)
 {
-	struct strbuf file_name = STRBUF_INIT;
+	struct strbuf gitfile_sb = STRBUF_INIT;
+	struct strbuf cfg_sb = STRBUF_INIT;
 	struct strbuf rel_path = STRBUF_INIT;
-	char *git_dir = real_pathdup(git_dir_);
-	char *work_tree = real_pathdup(work_tree_);
+	char *git_dir, *work_tree;
 
-	/* Update gitfile */
-	strbuf_addf(&file_name, "%s/.git", work_tree);
-	write_file(file_name.buf, "gitdir: %s",
-		   relative_path(git_dir, work_tree, &rel_path));
+	/* Prepare .git file */
+	strbuf_addf(&gitfile_sb, "%s/.git", work_tree_);
+	if (safe_create_leading_directories_const(gitfile_sb.buf))
+		die(_("could not create directories for %s"), gitfile_sb.buf);
+
+	/* Prepare config file */
+	strbuf_addf(&cfg_sb, "%s/config", git_dir_);
+	if (safe_create_leading_directories_const(cfg_sb.buf))
+		die(_("could not create directories for %s"), cfg_sb.buf);
 
+	git_dir = real_pathdup(git_dir_);
+	work_tree = real_pathdup(work_tree_);
+
+	/* Write .git file */
+	write_file(gitfile_sb.buf, "gitdir: %s",
+		   relative_path(git_dir, work_tree, &rel_path));
 	/* Update core.worktree setting */
-	strbuf_reset(&file_name);
-	strbuf_addf(&file_name, "%s/config", git_dir);
-	git_config_set_in_file(file_name.buf, "core.worktree",
+	git_config_set_in_file(cfg_sb.buf, "core.worktree",
 			       relative_path(work_tree, git_dir, &rel_path));
 
-	strbuf_release(&file_name);
+	strbuf_release(&gitfile_sb);
+	strbuf_release(&cfg_sb);
 	strbuf_release(&rel_path);
 	free(work_tree);
 	free(git_dir);
diff --git a/submodule.c b/submodule.c
index 3b98766a6..45e93a1d5 100644
--- a/submodule.c
+++ b/submodule.c
@@ -1445,8 +1445,6 @@ void absorb_git_dir_into_superproject(const char *prefix,
 
 	/* Not populated? */
 	if (!sub_git_dir) {
-		char *real_new_git_dir;
-		const char *new_git_dir;
 		const struct submodule *sub;
 
 		if (err_code == READ_GITFILE_ERR_STAT_FAILED) {
@@ -1469,13 +1467,8 @@ void absorb_git_dir_into_superproject(const char *prefix,
 		sub = submodule_from_path(null_sha1, path);
 		if (!sub)
 			die(_("could not lookup name for submodule '%s'"), path);
-		new_git_dir = git_path("modules/%s", sub->name);
-		if (safe_create_leading_directories_const(new_git_dir) < 0)
-			die(_("could not create directory '%s'"), new_git_dir);
-		real_new_git_dir = real_pathdup(new_git_dir);
-		connect_work_tree_and_git_dir(path, real_new_git_dir);
-
-		free(real_new_git_dir);
+		connect_work_tree_and_git_dir(path,
+			git_path("modules/%s", sub->name));
 	} else {
 		/* Is it already absorbed into the superprojects git dir? */
 		char *real_sub_git_dir = real_pathdup(sub_git_dir);
-- 
2.12.0.192.gbdb9d28a5

[PATCH v3 2/2] submodule--helper.c: remove duplicate code

[Valery Tolstov]

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>
---
 builtin/submodule--helper.c | 22 ++--------------------
 1 file changed, 2 insertions(+), 20 deletions(-)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 899dc334e..86bafe166 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -579,9 +579,7 @@ 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;
 	struct string_list reference = STRING_LIST_INIT_NODUP;
 	char *sm_alternate = NULL, *error_strategy = NULL;
@@ -653,27 +651,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);
+	/* Connect module worktree and git dir */
+	connect_work_tree_and_git_dir(path, sm_gitdir);
 
-	/* 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));
 
 	/* setup alternateLocation and alternateErrorStrategy in the cloned submodule if needed */
 	git_config_get_string("submodule.alternateLocation", &sm_alternate);
@@ -689,7 +672,6 @@ static int module_clone(int argc, const char **argv, const char *prefix)
 	free(error_strategy);
 
 	strbuf_release(&sb);
-	strbuf_release(&rel_path);
 	free(sm_gitdir);
 	free(path);
 	free(p);
-- 
2.12.0.192.gbdb9d28a5

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

[Brandon Williams]

On 03/09, Valery Tolstov wrote:
> 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>

Looks good.

-- 
Brandon Williams

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

[Junio C Hamano]

Brandon Williams <bmwill@google.com> writes:

> On 03/09, Valery Tolstov wrote:
>> 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>
>
> Looks good.

I'll queue with your Reviewed-by: added.

If sb/checkout-recurse-submodules is going to be rerolled, I'd
appreciate if it includes this patch inserted at an appropriate
place in the series, instead of me having to remember re-applying
this patch every time it happens.

Thanks.

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

[Stefan Beller]

On Fri, Mar 10, 2017 at 11:42 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Brandon Williams <bmwill@google.com> writes:
>
>> On 03/09, Valery Tolstov wrote:
>>> 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>
>>
>> Looks good.
>
> I'll queue with your Reviewed-by: added.
>
> If sb/checkout-recurse-submodules is going to be rerolled, I'd
> appreciate if it includes this patch inserted at an appropriate
> place in the series, instead of me having to remember re-applying
> this patch every time it happens.

Instead of mixing these two series, can you just take Valerys series as is,
and sb/checkout-recurse-submodules builds on top of that when rerolled?

Thanks,
Stefan

>
> Thanks.

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

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> On Fri, Mar 10, 2017 at 11:42 AM, Junio C Hamano <gitster@pobox.com> wrote:
>> Brandon Williams <bmwill@google.com> writes:
>>
>>> On 03/09, Valery Tolstov wrote:
>>>> 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>
>>>
>>> Looks good.
>>
>> I'll queue with your Reviewed-by: added.
>>
>> If sb/checkout-recurse-submodules is going to be rerolled, I'd
>> appreciate if it includes this patch inserted at an appropriate
>> place in the series, instead of me having to remember re-applying
>> this patch every time it happens.
>
> Instead of mixing these two series, can you just take Valerys series as is,
> and sb/checkout-recurse-submodules builds on top of that when rerolled?

That's fine by me, too, but that still means I need to keep an eye
on two independent topics that interact each other.  Is a single
patch 2/2 that important to be on a separate topic?  Expressed in
another way, is it expected that sb/checkout-recurse-submodules
would take forever to mature relative to these two patches?

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

[Stefan Beller]

On Fri, Mar 10, 2017 at 12:40 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Stefan Beller <sbeller@google.com> writes:
>
>> On Fri, Mar 10, 2017 at 11:42 AM, Junio C Hamano <gitster@pobox.com> wrote:
>>> Brandon Williams <bmwill@google.com> writes:
>>>
>>>> On 03/09, Valery Tolstov wrote:
>>>>> 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>
>>>>
>>>> Looks good.
>>>
>>> I'll queue with your Reviewed-by: added.
>>>
>>> If sb/checkout-recurse-submodules is going to be rerolled, I'd
>>> appreciate if it includes this patch inserted at an appropriate
>>> place in the series, instead of me having to remember re-applying
>>> this patch every time it happens.
>>
>> Instead of mixing these two series, can you just take Valerys series as is,
>> and sb/checkout-recurse-submodules builds on top of that when rerolled?
>
> That's fine by me, too, but that still means I need to keep an eye
> on two independent topics that interact each other.  Is a single
> patch 2/2 that important to be on a separate topic?  Expressed in
> another way, is it expected that sb/checkout-recurse-submodules
> would take forever to mature relative to these two patches?

Using the times and number of rerolls this has been around, it
is a not unreasonable to estimate sb/checkout-... will take longer
than this code deduplication patch.

Thanks,
Stefan

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

[Valery Tolstov]

As remainder. It is better if only [PATCH v3 2/2] is taken,
on top the patch from here
https://public-inbox.org/git/20170309221543.15897-8-sbeller@google.com/

The [PATCH v3 1/2] is just a copy of latter.

Re: [PATCH v2 0/2] Remove duplicate code from module_clone()

[Brandon Williams]

On 03/09, Valery Tolstov wrote:
> > Then the next step (as outlined by Documentation/SubmittingPatches)
> > is to figure out how to best present this to the mailing list; I think the best
> > way is to send out a patch series consisting of both of these 2 patches,
> > the "connect_work_tree_and_git_dir: safely create leading directories,"
> > first and then your deduplication patch.
> 
> Combined two patches
> 
> Stefan Beller (1):
>   connect_work_tree_and_git_dir: safely create leading directories

It looks like this patch is apart of Stefan's checkout series.  So I was
slightly confused when first looking at this patch since I'd seen it before.
The usual protocol would be to rebase off of Stefan's series and build
on that (assuming you have a dependency against his series), indicating
that you are doing as such in your cover letter.  Though what you have
here should hopefully give the maintainer enough context to know where to
put it, so you should be good :)

> 
> Valery Tolstov (1):
>   submodule--helper.c: remove duplicate code
> 
>  builtin/submodule--helper.c | 20 ++------------------
>  dir.c                       | 32 +++++++++++++++++++++-----------
>  submodule.c                 | 11 ++---------
>  3 files changed, 25 insertions(+), 38 deletions(-)
> 
> -- 
> 2.12.0.192.gbdb9d28a5
> 

-- 
Brandon Williams

Re: Re: [PATCH v2 0/2] Remove duplicate code from module_clone()

[Valery Tolstov]

> The usual protocol would be to rebase off of Stefan's series and build
> on that (assuming you have a dependency against his series), indicating
> that you are doing as such in your cover letter.

So, should I send only my patch, or current format (patch and dependency) 
is acceptalbe?

Regards,
  Valery Tolstov

Re: [PATCH v2 0/2] Remove duplicate code from module_clone()

[Brandon Williams]

On 03/09, Valery Tolstov wrote:
> > The usual protocol would be to rebase off of Stefan's series and build
> > on that (assuming you have a dependency against his series), indicating
> > that you are doing as such in your cover letter.
> 
> So, should I send only my patch, or current format (patch and dependency) 
> is acceptalbe?

What you did here should be fine.  I was just pointing that out for future
series that you send out to the list.

-- 
Brandon Williams