Re: [PATCH v2 4/3] init: combine set_git_dir_init() and init_db() into one

[Duy Nguyen]

On Sat, Sep 24, 2016 at 11:55:33AM -0700, Junio C Hamano wrote:
> Junio C Hamano  writes:
> 
> > I think this 4/3 is not quite enough to fix the damage to the code
> > caused by 2/3.
> > ...
> > after 4/3 is applied, we should be able to remove the global
> > variable 2/3 introduced, make init_db() receive that information as
> > the return value of set_git_dir_init(), and pass that as a parameter
> > to create_default_files().
> 
> That would look something like this squashed into 4/3, I think.  I
> am not sure if a commit that squashes 2/3, 3/3, 4/3 and this update
> together is harder to understand than keeping 2/3, 3/3 and a fixed
> 4/3 separate, though.  The end result looks much better structured
> than before 2/3 is applied to my quick scan-through of the code.
> ...

How about this?

  [1/5] init: correct re-initialization from a linked worktree
  [2/5] init: call set_git_dir_init() from within init_db()
  [3/5] init: kill set_git_dir_init()
  [4/5] init: do not set unnecessary core.worktree
  [5/5] init: kill git_link variable

I went a bit further, merging set_git_dir_init() back to init_db() so
I can kill "git_link" variable cleanly in 5/5. 3/5 does make init_db()
a bit longer, but not to the alarming level yet. By 4/5,
set_git_dir_init() is gone, so init_db() just needs to save and pass
original_git_dir down to needs_work_tree_config().
--
Duy

Re: [PATCH v2 4/3] init: combine set_git_dir_init() and init_db() into one

[Junio C Hamano]

Junio C Hamano  writes:

> I think this 4/3 is not quite enough to fix the damage to the code
> caused by 2/3.
> ...
> after 4/3 is applied, we should be able to remove the global
> variable 2/3 introduced, make init_db() receive that information as
> the return value of set_git_dir_init(), and pass that as a parameter
> to create_default_files().

That would look something like this squashed into 4/3, I think.  I
am not sure if a commit that squashes 2/3, 3/3, 4/3 and this update
together is harder to understand than keeping 2/3, 3/3 and a fixed
4/3 separate, though.  The end result looks much better structured
than before 2/3 is applied to my quick scan-through of the code.

In any case, the log message of 2/3 needs to be updated to retitle
it, I think.  "do not ... more often than necessary" makes it sound
as if we were doing things that did not make any difference in the
end result, wasting cycles.  But what you actually wanted to achieve
was not to "avoid unnecessary work"--doing so gave a broken
behaviour and that was what you were fixing, "do not record broken
core.worktree", perhaps?

The solution (if we squash 2-4 and the fixup below) is to stop
feeding get_git_dir() to needs_work_tree_config(), because the
parameter to the latter is the path to ".git" that presumably is at
the top of the working tree, and get_git_dir() is not that when
"gitdir" file is involved.  So a rewritten log message may say
something like...

init: do not set unnecessary core.worktree

The function needs_work_tree_config() that is called from
create_default_files() is supposed to be fed the path to ".git"
that looks as if it is at the top of the working tree, and
decide if that location matches the actual worktree being used.
This comparison allows "git init" to decide if core.worktree
needs to be recorded in the working tree.

In the current code, however, we feed the return value from
get_git_dir(), which can be totally different from what the
function expects when "gitdir" file is involved.  Instead of
giving the path to the ".git" at the top of the working tree, we
end up feeding the actual path that the file points at.

This original location of ".git" however is only known to a
helper function set_git_dir_init() that must be called before
init_db() is called (they both have only two callsites, one in
"git init" and the other in "git clone"), and in the current
code, this original location is not visible to its callers.

By doing the following two things:

* Move call to set_git_dir_init() to init_db(), as the two must
  always be called in this order, and adjust its current
  callers.

* Make set_git_dir_init() return the original location of ".git"
  to the caller, which is init_db(), and have it passed to
  create_default_files() as a new parameter.

   pass the correct location down to needs_work_tree_config() to fix
   this.

This suggests that 2/3, 3/3 and fixed 4/3 could be done in two
logical steps.  The first bullet point can be done as a separate
preparatory step, and on top of that, the second bullet point can be
done as a separate "fix".



 builtin/init-db.c | 21 +++--
 1 file changed, 11 insertions(+), 10 deletions(-)

diff --git a/builtin/init-db.c b/builtin/init-db.c
index ee7942f..527722c 100644
--- a/builtin/init-db.c
+++ b/builtin/init-db.c
@@ -23,7 +23,6 @@ static int init_is_bare_repository = 0;
 static int init_shared_repository = -1;
 static const char *init_db_template_dir;
 static const char *git_link;
-static const char *original_git_dir;
 
 static void copy_templates_1(struct strbuf *path, struct strbuf *template,
 DIR *dir)
@@ -172,7 +171,8 @@ static int needs_work_tree_config(const char *git_dir, 
const char *work_tree)
return 1;
 }
 
-static int create_default_files(const char *template_path)
+static int create_default_files(const char *template_path,
+   const char *original_git_dir)
 {
struct stat st1;
struct strbuf buf = STRBUF_INIT;
@@ -312,11 +312,11 @@ static void create_object_directory(void)
strbuf_release();
 }
 
-static int set_git_dir_init(const char *git_dir,
-   const char *real_git_dir,
-   int exist_ok)
+static char *set_git_dir_init(const char *git_dir,
+ const char *real_git_dir,
+ int exist_ok)
 {
-   original_git_dir = xstrdup(real_path(git_dir));
+   char *original_git_dir = xstrdup(real_path(git_dir));
 
if (real_git_dir) {
struct stat st;
@@ -339,7 +339,7 @@ static int set_git_dir_init(const char *git_dir,
git_link = NULL;
}
startup_info->have_repository = 1;
-   return 0;
+   return original_git_dir;
 }
 
 static void separate_git_dir(const char *git_dir)
@@ -367,9 +367,10 @@ 

Re: [PATCH v2 4/3] init: combine set_git_dir_init() and init_db() into one

[Junio C Hamano]

Nguyễn Thái Ngọc Duy   writes:

>  I think a separate commit for this is better than combining back to
>  2/3 so we can explain the problem properly (without making 2/3 commit
>  message even longer)
>
>  Not sure if you want to s/contains/contain/ in 2/3 by yourself or I
>  should resend the whole series. Let me know.

I think this 4/3 is not quite enough to fix the damage to the code
caused by 2/3.

Given that

 - set_git_dir_init() is is the only one that sets original_git_dir,

 - create_default_files() is the only one that uses
   original_git_dir, and

 - init_db() is the only one that calls set_git_dir_init() and
   create_default_files()

after 4/3 is applied, we should be able to remove the global
variable 2/3 introduced, make init_db() receive that information as
the return value of set_git_dir_init(), and pass that as a parameter
to create_default_files().

Re: [PATCH v2 4/3] init: combine set_git_dir_init() and init_db() into one

[Junio C Hamano]

Nguyễn Thái Ngọc Duy   writes:

>  I think a separate commit for this is better than combining back to
>  2/3 so we can explain the problem properly (without making 2/3 commit
>  message even longer)
>
>  Not sure if you want to s/contains/contain/ in 2/3 by yourself or I
>  should resend the whole series. Let me know.

OK, I just amended it before applying this on top.

> + flags |= INIT_DB_EXIST_OK;
> + return init_db(git_dir, real_git_dir, template_dir, flags);

I do not think of anything better, but EXIST_OK does not sound
grammatical.  "REINIT" is not quite it--we are merely allowing
the function to re-init if there already is a repository.  And
OK_TO_REINIT is a bit too long.  Let's take the patch as-is for
now.

Thanks.

[PATCH v2 4/3] init: combine set_git_dir_init() and init_db() into one

[Nguyễn Thái Ngọc Duy]

Commit "init: do not set core.worktree more often than necessary" adds a
subtle dependency between set_git_dir_init() and init_db(). The former
_must_ be called before init_db() so that original_git_dir can be set
properly. If something else, like enter_repo() or setup_git_directory(),
is used instead, the trick in that commit breaks down.

To eliminate the possibility that init_db() in future may be called
without set_git_dir_init(), init_db() now calls that function internally
(and does not allow anybody else to use it).

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 I think a separate commit for this is better than combining back to
 2/3 so we can explain the problem properly (without making 2/3 commit
 message even longer)

 Not sure if you want to s/contains/contain/ in 2/3 by yourself or I
 should resend the whole series. Let me know.

 builtin/clone.c   | 15 +++
 builtin/init-db.c | 18 +++---
 cache.h   |  5 +++--
 3 files changed, 21 insertions(+), 17 deletions(-)

diff --git a/builtin/clone.c b/builtin/clone.c
index 6616392..29b1832 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -928,23 +928,22 @@ int cmd_clone(int argc, const char **argv, const char 
*prefix)
set_git_work_tree(work_tree);
}
 
-   junk_git_dir = git_dir;
+   junk_git_dir = real_git_dir ? real_git_dir : git_dir;
if (safe_create_leading_directories_const(git_dir) < 0)
die(_("could not create leading directories of '%s'"), git_dir);
 
-   set_git_dir_init(git_dir, real_git_dir, 0);
-   if (real_git_dir) {
-   git_dir = real_git_dir;
-   junk_git_dir = real_git_dir;
-   }
-
if (0 <= option_verbosity) {
if (option_bare)
fprintf(stderr, _("Cloning into bare repository 
'%s'...\n"), dir);
else
fprintf(stderr, _("Cloning into '%s'...\n"), dir);
}
-   init_db(option_template, INIT_DB_QUIET);
+
+   init_db(git_dir, real_git_dir, option_template, INIT_DB_QUIET);
+
+   if (real_git_dir)
+   git_dir = real_git_dir;
+
write_config(_config);
 
git_config(git_default_config, NULL);
diff --git a/builtin/init-db.c b/builtin/init-db.c
index d70fc45..ee7942f 100644
--- a/builtin/init-db.c
+++ b/builtin/init-db.c
@@ -312,8 +312,9 @@ static void create_object_directory(void)
strbuf_release();
 }
 
-int set_git_dir_init(const char *git_dir, const char *real_git_dir,
-int exist_ok)
+static int set_git_dir_init(const char *git_dir,
+   const char *real_git_dir,
+   int exist_ok)
 {
original_git_dir = xstrdup(real_path(git_dir));
 
@@ -362,10 +363,14 @@ static void separate_git_dir(const char *git_dir)
write_file(git_link, "gitdir: %s", git_dir);
 }
 
-int init_db(const char *template_dir, unsigned int flags)
+int init_db(const char *git_dir, const char *real_git_dir,
+   const char *template_dir, unsigned int flags)
 {
int reinit;
-   const char *git_dir = get_git_dir();
+
+   set_git_dir_init(git_dir, real_git_dir, flags & INIT_DB_EXIST_OK);
+
+   git_dir = get_git_dir();
 
if (git_link)
separate_git_dir(git_dir);
@@ -585,7 +590,6 @@ int cmd_init_db(int argc, const char **argv, const char 
*prefix)
set_git_work_tree(work_tree);
}
 
-   set_git_dir_init(git_dir, real_git_dir, 1);
-
-   return init_db(template_dir, flags);
+   flags |= INIT_DB_EXIST_OK;
+   return init_db(git_dir, real_git_dir, template_dir, flags);
 }
diff --git a/cache.h b/cache.h
index b2d77f3..7fc875f 100644
--- a/cache.h
+++ b/cache.h
@@ -525,9 +525,10 @@ extern void verify_non_filename(const char *prefix, const 
char *name);
 extern int path_inside_repo(const char *prefix, const char *path);
 
 #define INIT_DB_QUIET 0x0001
+#define INIT_DB_EXIST_OK 0x0002
 
-extern int set_git_dir_init(const char *git_dir, const char *real_git_dir, 
int);
-extern int init_db(const char *template_dir, unsigned int flags);
+extern int init_db(const char *git_dir, const char *real_git_dir,
+  const char *template_dir, unsigned int flags);
 
 extern void sanitize_stdfds(void);
 extern int daemonize(void);
-- 
2.8.2.524.g6ff3d78