# [PATCH] submodule--helper.c: remove duplicate code 24 messages from 2017-03-08 to 2017-03-10. Participants: me@vtolstov.org, Stefan Beller, Valery Tolstov, Junio C Hamano, Brandon Williams. Thread: https://gitlist.dev/t/45318 ## me@vtolstov.org, 2017-03-08 17:44 Subject: [PATCH] submodule--helper.c: remove duplicate code Message-ID: <20170308174449.24266-1-me@vtolstov.org> URL: https://gitlist.dev/e/20170308174449.24266-1-me%40vtolstov.org ``` From: 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 --- >> 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 ``` ## Stefan Beller, 2017-03-08 18:53 Subject: Re: [PATCH] submodule--helper.c: remove duplicate code Message-ID: URL: https://gitlist.dev/e/CAGZ79kYJ9he-jhnA35m6az-T5Z58efmKsUaBw--_KzdGJPZb-Q%40mail.gmail.com In-Reply-To: <20170308174449.24266-1-me@vtolstov.org> ``` On Wed, Mar 8, 2017 at 9:44 AM, wrote: > From: 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 > --- >>> 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 > ``` ## Valery Tolstov, 2017-03-08 19:59 Subject: Re: [PATCH] submodule--helper.c: remove duplicate code Message-ID: <20170308195916.7349-1-me@vtolstov.org> URL: https://gitlist.dev/e/20170308195916.7349-1-me%40vtolstov.org In-Reply-To: ``` > 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 ``` ## Stefan Beller, 2017-03-08 20:25 Subject: Re: [PATCH] submodule--helper.c: remove duplicate code Message-ID: URL: https://gitlist.dev/e/CAGZ79kbnpUtrKdjQdQ-r6rRuVvnawooLFk1bO8jOSgxNkx2Dbg%40mail.gmail.com In-Reply-To: <20170308195916.7349-1-me@vtolstov.org> ``` On Wed, Mar 8, 2017 at 11:59 AM, Valery Tolstov 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 ``` ## Stefan Beller, 2017-03-08 23:15 Subject: Re: [PATCH] submodule--helper.c: remove duplicate code Message-ID: URL: https://gitlist.dev/e/CAGZ79ka7PNKq5JWLPujvVHJWf6eEUadaJXd5AmKEvKT_y1ghOA%40mail.gmail.com In-Reply-To: <20170308230512.30572-1-me@vtolstov.org> ``` On Wed, Mar 8, 2017 at 3:05 PM, 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. > > 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 ``` ## Junio C Hamano, 2017-03-08 23:24 Subject: Re: [PATCH] submodule--helper.c: remove duplicate code Message-ID: URL: https://gitlist.dev/e/xmqq60jj1heh.fsf%40gitster.mtv.corp.google.com In-Reply-To: ``` Stefan Beller 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? ``` ## Valery Tolstov, 2017-03-09 00:03 Subject: [PATCH v2 1/2] connect_work_tree_and_git_dir: safely create leading directories Message-ID: <20170309000352.18330-2-me@vtolstov.org> URL: https://gitlist.dev/e/20170309000352.18330-2-me%40vtolstov.org In-Reply-To: <20170309000352.18330-1-me@vtolstov.org> ``` From: Stefan Beller 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 --- 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 ``` ## Valery Tolstov, 2017-03-09 00:03 Subject: [PATCH v2 2/2] submodule--helper.c: remove duplicate code Message-ID: <20170309000352.18330-3-me@vtolstov.org> URL: https://gitlist.dev/e/20170309000352.18330-3-me%40vtolstov.org In-Reply-To: <20170309000352.18330-1-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 --- 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 ``` ## Stefan Beller, 2017-03-08 23:34 Subject: Re: [PATCH] submodule--helper.c: remove duplicate code Message-ID: URL: https://gitlist.dev/e/CAGZ79kZY%2BcyPBGDmhYfJNtaS4UgUp-2nsOKLBgNzwfRpuCn7PA%40mail.gmail.com In-Reply-To: ``` On Wed, Mar 8, 2017 at 3:24 PM, Junio C Hamano wrote: > Stefan Beller 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 ``` ## Valery Tolstov, 2017-03-09 00:03 Subject: [PATCH v2 0/2] Remove duplicate code from module_clone() Message-ID: <20170309000352.18330-1-me@vtolstov.org> URL: https://gitlist.dev/e/20170309000352.18330-1-me%40vtolstov.org In-Reply-To: ``` > 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 ``` ## Brandon Williams, 2017-03-09 00:28 Subject: Re: [PATCH v2 0/2] Remove duplicate code from module_clone() Message-ID: <20170309002818.GA153031@google.com> URL: https://gitlist.dev/e/20170309002818.GA153031%40google.com In-Reply-To: <20170309000352.18330-1-me@vtolstov.org> ``` 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 ``` ## Valery Tolstov, 2017-03-09 00:56 Subject: Re: Re: [PATCH v2 0/2] Remove duplicate code from module_clone() Message-ID: <20170309005636.20254-1-me@vtolstov.org> URL: https://gitlist.dev/e/20170309005636.20254-1-me%40vtolstov.org In-Reply-To: <20170309002818.GA153031@google.com> ``` > 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 ``` ## Brandon Williams, 2017-03-09 00:38 Subject: Re: [PATCH v2 2/2] submodule--helper.c: remove duplicate code Message-ID: <20170309003858.GB153031@google.com> URL: https://gitlist.dev/e/20170309003858.GB153031%40google.com In-Reply-To: <20170309000352.18330-3-me@vtolstov.org> ``` 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 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 ``` ## Valery Tolstov, 2017-03-09 01:27 Subject: [PATCH v3 2/2] submodule--helper.c: remove duplicate code Message-ID: <20170309012734.21541-3-me@vtolstov.org> URL: https://gitlist.dev/e/20170309012734.21541-3-me%40vtolstov.org In-Reply-To: <20170309012734.21541-1-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 --- 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 ``` ## Valery Tolstov, 2017-03-09 01:27 Subject: [PATCH v3 1/2] connect_work_tree_and_git_dir: safely create leading directories Message-ID: <20170309012734.21541-2-me@vtolstov.org> URL: https://gitlist.dev/e/20170309012734.21541-2-me%40vtolstov.org In-Reply-To: <20170309012734.21541-1-me@vtolstov.org> ``` From: Stefan Beller 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 --- 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 ``` ## Valery Tolstov, 2017-03-09 01:27 Subject: [PATCH v3 0/2] Remove duplicate code from module_clone() Message-ID: <20170309012734.21541-1-me@vtolstov.org> URL: https://gitlist.dev/e/20170309012734.21541-1-me%40vtolstov.org In-Reply-To: <20170309003858.GB153031@google.com> ``` 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 ``` ## Valery Tolstov, 2017-03-08 23:05 Subject: Re: [PATCH] submodule--helper.c: remove duplicate code Message-ID: <20170308230512.30572-1-me@vtolstov.org> URL: https://gitlist.dev/e/20170308230512.30572-1-me%40vtolstov.org In-Reply-To: ``` > 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 ``` ## Brandon Williams, 2017-03-09 18:18 Subject: Re: [PATCH v3 2/2] submodule--helper.c: remove duplicate code Message-ID: <20170309181837.GF153031@google.com> URL: https://gitlist.dev/e/20170309181837.GF153031%40google.com In-Reply-To: <20170309012734.21541-3-me@vtolstov.org> ``` 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 Looks good. -- Brandon Williams ``` ## Brandon Williams, 2017-03-09 18:19 Subject: Re: [PATCH v2 0/2] Remove duplicate code from module_clone() Message-ID: <20170309181927.GG153031@google.com> URL: https://gitlist.dev/e/20170309181927.GG153031%40google.com In-Reply-To: <20170309005636.20254-1-me@vtolstov.org> ``` 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 ``` ## Valery Tolstov, 2017-03-09 22:54 Subject: Re: [PATCH v3 2/2] submodule--helper.c: remove duplicate code Message-ID: <20170309225411.2283-1-me@vtolstov.org> URL: https://gitlist.dev/e/20170309225411.2283-1-me%40vtolstov.org In-Reply-To: <20170309012734.21541-3-me@vtolstov.org> ``` 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. ``` ## Junio C Hamano, 2017-03-10 19:42 Subject: Re: [PATCH v3 2/2] submodule--helper.c: remove duplicate code Message-ID: URL: https://gitlist.dev/e/xmqqvargyl39.fsf%40gitster.mtv.corp.google.com In-Reply-To: <20170309181837.GF153031@google.com> ``` Brandon Williams 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 > > 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. ``` ## Stefan Beller, 2017-03-10 19:49 Subject: Re: [PATCH v3 2/2] submodule--helper.c: remove duplicate code Message-ID: URL: https://gitlist.dev/e/CAGZ79kbhG9XhRccqA555Zp-FENJ5Q6Nd%2BLnNTZno1J_%3DcGQctQ%40mail.gmail.com In-Reply-To: ``` On Fri, Mar 10, 2017 at 11:42 AM, Junio C Hamano wrote: > Brandon Williams 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 >> >> 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. ``` ## Junio C Hamano, 2017-03-10 20:40 Subject: Re: [PATCH v3 2/2] submodule--helper.c: remove duplicate code Message-ID: URL: https://gitlist.dev/e/xmqq7f3wyifv.fsf%40gitster.mtv.corp.google.com In-Reply-To: ``` Stefan Beller writes: > On Fri, Mar 10, 2017 at 11:42 AM, Junio C Hamano wrote: >> Brandon Williams 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 >>> >>> 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? ``` ## Stefan Beller, 2017-03-10 20:48 Subject: Re: [PATCH v3 2/2] submodule--helper.c: remove duplicate code Message-ID: URL: https://gitlist.dev/e/CAGZ79kbX6Wq_58MPRM08b0eL8vqtnRytr1PHSCTRDnA%3D3FEXwA%40mail.gmail.com In-Reply-To: ``` On Fri, Mar 10, 2017 at 12:40 PM, Junio C Hamano wrote: > Stefan Beller writes: > >> On Fri, Mar 10, 2017 at 11:42 AM, Junio C Hamano wrote: >>> Brandon Williams 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 >>>> >>>> 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 ```