[PATCH][RFC] read-cache: read_index_from() accepts repo as arg

[PATCH][RFC] read-cache: read_index_from() accepts repo as arg

[Kapil Jain]

Signed-off-by: Kapil Jain <jkapil.cs@gmail.com>
---

In read-cache, the read_index_from() function had a TODO task,
this patch completes that. There are some other functions in the same file
where this exact TODO needs to be done, will proceed to do them once this patch is accepted.

running test gave 256 not okays, each had a label as `# TODO known breakage`, which i think
are not concerned to this patch.

 apply.c            | 2 +-
 builtin/worktree.c | 2 +-
 cache-tree.c       | 2 +-
 cache.h            | 4 ++--
 read-cache.c       | 4 ++--
 repository.c       | 2 +-
 revision.c         | 2 +-
 7 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/apply.c b/apply.c
index f15afa9f6a..3b4d128149 100644
--- a/apply.c
+++ b/apply.c
@@ -4021,7 +4021,7 @@ static int read_apply_cache(struct apply_state *state)
 {
 	if (state->index_file)
 		return read_index_from(state->repo->index, state->index_file,
-				       get_git_dir());
+				       get_git_dir(), state->repo);
 	else
 		return repo_read_index(state->repo);
 }
diff --git a/builtin/worktree.c b/builtin/worktree.c
index 6cc094a453..874adebd2c 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -737,7 +737,7 @@ static void validate_no_submodules(const struct worktree *wt)
 		 */
 		found_submodules = 1;
 	} else if (read_index_from(&istate, worktree_git_path(wt, "index"),
-				   get_worktree_git_dir(wt)) > 0) {
+				   get_worktree_git_dir(wt), the_repository) > 0) {
 		for (i = 0; i < istate.cache_nr; i++) {
 			struct cache_entry *ce = istate.cache[i];
 			int err;
diff --git a/cache-tree.c b/cache-tree.c
index b13bfaf71e..84f19b224e 100644
--- a/cache-tree.c
+++ b/cache-tree.c
@@ -616,7 +616,7 @@ int write_index_as_tree(struct object_id *oid, struct index_state *index_state,
 
 	hold_lock_file_for_update(&lock_file, index_path, LOCK_DIE_ON_ERROR);
 
-	entries = read_index_from(index_state, index_path, get_git_dir());
+	entries = read_index_from(index_state, index_path, get_git_dir(), the_repository);
 	if (entries < 0) {
 		ret = WRITE_TREE_UNREADABLE_INDEX;
 		goto out;
diff --git a/cache.h b/cache.h
index ac92421f3a..3850c82fc9 100644
--- a/cache.h
+++ b/cache.h
@@ -420,7 +420,7 @@ extern struct index_state the_index;
 #define active_cache_tree (the_index.cache_tree)
 
 #define read_cache() repo_read_index(the_repository)
-#define read_cache_from(path) read_index_from(&the_index, (path), (get_git_dir()))
+#define read_cache_from(path) read_index_from(&the_index, (path), (get_git_dir()), the_repository)
 #define read_cache_preload(pathspec) repo_read_index_preload(the_repository, (pathspec), 0)
 #define is_cache_unborn() is_index_unborn(&the_index)
 #define read_cache_unmerged() repo_read_index_unmerged(the_repository)
@@ -678,7 +678,7 @@ extern void preload_index(struct index_state *index,
 extern int do_read_index(struct index_state *istate, const char *path,
 			 int must_exist); /* for testting only! */
 extern int read_index_from(struct index_state *, const char *path,
-			   const char *gitdir);
+			   const char *gitdir, const struct repository *repo);
 extern int is_index_unborn(struct index_state *);
 
 /* For use with `write_locked_index()`. */
diff --git a/read-cache.c b/read-cache.c
index 4dc6de1b55..0444703284 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -2256,7 +2256,7 @@ static void freshen_shared_index(const char *shared_index, int warn)
 }
 
 int read_index_from(struct index_state *istate, const char *path,
-		    const char *gitdir)
+		    const char *gitdir, const struct repository *repo)
 {
 	struct split_index *split_index;
 	int ret;
@@ -2292,7 +2292,7 @@ int read_index_from(struct index_state *istate, const char *path,
 		split_index->base = xcalloc(1, sizeof(*split_index->base));
 
 	base_oid_hex = oid_to_hex(&split_index->base_oid);
-	base_path = xstrfmt("%s/sharedindex.%s", gitdir, base_oid_hex);
+	base_path = xstrfmt("%s/sharedindex.%s", repo->gitdir, base_oid_hex);
 	trace2_region_enter_printf("index", "shared/do_read_index",
 				   the_repository, "%s", base_path);
 	ret = do_read_index(split_index->base, base_path, 1);
diff --git a/repository.c b/repository.c
index 682c239fe3..8ac2b65f61 100644
--- a/repository.c
+++ b/repository.c
@@ -264,7 +264,7 @@ int repo_read_index(struct repository *repo)
 	if (!repo->index)
 		repo->index = xcalloc(1, sizeof(*repo->index));
 
-	return read_index_from(repo->index, repo->index_file, repo->gitdir);
+	return read_index_from(repo->index, repo->index_file, repo->gitdir, repo);
 }
 
 int repo_hold_locked_index(struct repository *repo,
diff --git a/revision.c b/revision.c
index eb8e51bc63..247a4d5704 100644
--- a/revision.c
+++ b/revision.c
@@ -1556,7 +1556,7 @@ void add_index_objects_to_pending(struct rev_info *revs, unsigned int flags)
 
 		if (read_index_from(&istate,
 				    worktree_git_path(wt, "index"),
-				    get_worktree_git_dir(wt)) > 0)
+				    get_worktree_git_dir(wt), the_repository) > 0)
 			do_add_index_objects_to_pending(revs, &istate, flags);
 		discard_index(&istate);
 	}
-- 
2.20.1

Re: [PATCH][RFC] read-cache: read_index_from() accepts repo as arg

[Duy Nguyen]

On Sun, Apr 07, 2019 at 01:07:12PM +0530, Kapil Jain wrote:
> Signed-off-by: Kapil Jain <jkapil.cs@gmail.com>
> ---
> 
> In read-cache, the read_index_from() function had a TODO task, this
> patch completes that.

This line at least should be above the "---" line (i.e. part of the
commit message).

> diff --git a/read-cache.c b/read-cache.c
> index 4dc6de1b55..0444703284 100644
> --- a/read-cache.c
> +++ b/read-cache.c
> @@ -2256,7 +2256,7 @@ static void freshen_shared_index(const char *shared_index, int warn)
>  }
>  
>  int read_index_from(struct index_state *istate, const char *path,
> -		    const char *gitdir)
> +		    const char *gitdir, const struct repository *repo)

"struct repository *" by convention is always the first argument. See

https://public-inbox.org/git/xmqqsh2p6l43.fsf@gitster-ct.c.googlers.com/

You also do not need "gitdir" as a separate argument because gitdir is
an attribute of a repository. Passing it separately is just a trap to
give inconsistent information (e.g. one repo and one gitdir from
another one).

I see you already use repo->gitdir below. Which means this argument
"gitdir" is no longer used anyway. Please remove.

>  {
>  	struct split_index *split_index;
>  	int ret;
> @@ -2292,7 +2292,7 @@ int read_index_from(struct index_state *istate, const char *path,
>  		split_index->base = xcalloc(1, sizeof(*split_index->base));
>  
>  	base_oid_hex = oid_to_hex(&split_index->base_oid);
> -	base_path = xstrfmt("%s/sharedindex.%s", gitdir, base_oid_hex);
> +	base_path = xstrfmt("%s/sharedindex.%s", repo->gitdir, base_oid_hex);
>  	trace2_region_enter_printf("index", "shared/do_read_index",
>  				   the_repository, "%s", base_path);

"the_repository" here (and all others in this function) should be
replaced with "repo". The TODO comment in this function should be
removed as well.

>  	ret = do_read_index(split_index->base, base_path, 1);
> diff --git a/revision.c b/revision.c
> index eb8e51bc63..247a4d5704 100644
> --- a/revision.c
> +++ b/revision.c
> @@ -1556,7 +1556,7 @@ void add_index_objects_to_pending(struct rev_info *revs, unsigned int flags)
>  
>  		if (read_index_from(&istate,
>  				    worktree_git_path(wt, "index"),
> -				    get_worktree_git_dir(wt)) > 0)
> +				    get_worktree_git_dir(wt), the_repository) > 0)

We have revs->repo to refer to the main repo being examined here. But
if the "gitdir" argument is deleted, then revs->repo->gitdir gives a
_wrong_ gitdir, it's not the same as get_worktree_git_dir(wt).

So before you can continue here, you'll need to add a function,
e.g. repo_worktree_init() (similar to repo_init() and
repo_submodule_init()). This one should take "struct worktree *" as
the argument and return "struct repository *"

So, with something like a patch below (not tested), you should be able
to write

	worktrees = repo_get_worktrees(revs->repo, 0);
	...
		struct repository *r = repo_worktree_init(wt);
		if (read_index_from(&istate, r->index_file, r) > 0)
			do_add_index_objects_to_pending(revs, &istate, flags);
		repo_clear(r);

-- 8< --
diff --git a/repository.c b/repository.c
index 682c239fe3..5a0c7972db 100644
--- a/repository.c
+++ b/repository.c
@@ -10,6 +10,7 @@
 #include "object.h"
 #include "lockfile.h"
 #include "submodule-config.h"
+#include "worktree.h"
 
 /* The main repository */
 static struct repository the_repo;
@@ -227,6 +228,28 @@ int repo_submodule_init(struct repository *subrepo,
 	return ret;
 }
 
+int repo_worktree_init(struct repository *repo,
+		       struct repository *source_repo,
+		       const struct worktree *wt)
+{
+	struct strbuf gitdir = STRBUF_INIT;
+	int ret;
+
+	if (!wt)
+		return -1;
+
+	if (!wt->id)
+		strbuf_addstr(&gitdir, source_repo->commondir);
+	else
+		strbuf_addf(&gitdir, "%s/worktrees/%s",
+			    source_repo->commondir,
+			    wt->id);
+
+	ret = repo_init(source_repo, gitdir.buf, wt->path);
+	strbuf_release(&gitdir);
+	return ret ? -1 : 0;
+}
+
 void repo_clear(struct repository *repo)
 {
 	FREE_AND_NULL(repo->gitdir);
diff --git a/repository.h b/repository.h
index 4fb6a5885f..d3c21c7ab5 100644
--- a/repository.h
+++ b/repository.h
@@ -122,6 +122,10 @@ void repo_set_hash_algo(struct repository *repo, int algo);
 void initialize_the_repository(void);
 int repo_init(struct repository *r, const char *gitdir, const char *worktree);
 
+struct worktree;
+int repo_worktree_init(struct repository *r, struct repository *source,
+		       const struct worktree *wt);
+
 /*
  * Initialize the repository 'subrepo' as the submodule given by the
  * struct submodule 'sub' in parent repository 'superproject'.
diff --git a/worktree.c b/worktree.c
index b45bfeb9d3..188ea04c61 100644
--- a/worktree.c
+++ b/worktree.c
@@ -44,19 +44,19 @@ static void add_head_info(struct worktree *wt)
 /**
  * get the main worktree
  */
-static struct worktree *get_main_worktree(void)
+static struct worktree *get_main_worktree(struct repository *r)
 {
 	struct worktree *worktree = NULL;
 	struct strbuf path = STRBUF_INIT;
 	struct strbuf worktree_path = STRBUF_INIT;
 	int is_bare = 0;
 
-	strbuf_add_absolute_path(&worktree_path, get_git_common_dir());
+	strbuf_add_absolute_path(&worktree_path, r->commondir);
 	is_bare = !strbuf_strip_suffix(&worktree_path, "/.git");
 	if (is_bare)
 		strbuf_strip_suffix(&worktree_path, "/.");
 
-	strbuf_addf(&path, "%s/HEAD", get_git_common_dir());
+	strbuf_addf(&path, "%s/HEAD", r->commondir);
 
 	worktree = xcalloc(1, sizeof(*worktree));
 	worktree->path = strbuf_detach(&worktree_path, NULL);
@@ -68,7 +68,7 @@ static struct worktree *get_main_worktree(void)
 	return worktree;
 }
 
-static struct worktree *get_linked_worktree(const char *id)
+static struct worktree *get_linked_worktree(struct repository *r, const char *id)
 {
 	struct worktree *worktree = NULL;
 	struct strbuf path = STRBUF_INIT;
@@ -77,7 +77,7 @@ static struct worktree *get_linked_worktree(const char *id)
 	if (!id)
 		die("Missing linked worktree name");
 
-	strbuf_git_common_path(&path, the_repository, "worktrees/%s/gitdir", id);
+	strbuf_git_common_path(&path, r, "worktrees/%s/gitdir", id);
 	if (strbuf_read_file(&worktree_path, path.buf, 0) <= 0)
 		/* invalid gitdir file */
 		goto done;
@@ -90,7 +90,7 @@ static struct worktree *get_linked_worktree(const char *id)
 	}
 
 	strbuf_reset(&path);
-	strbuf_addf(&path, "%s/worktrees/%s/HEAD", get_git_common_dir(), id);
+	strbuf_addf(&path, "%s/worktrees/%s/HEAD", r->commondir, id);
 
 	worktree = xcalloc(1, sizeof(*worktree));
 	worktree->path = strbuf_detach(&worktree_path, NULL);
@@ -127,7 +127,7 @@ static int compare_worktree(const void *a_, const void *b_)
 	return fspathcmp((*a)->path, (*b)->path);
 }
 
-struct worktree **get_worktrees(unsigned flags)
+struct worktree **repo_get_worktrees(struct repository *r, unsigned flags)
 {
 	struct worktree **list = NULL;
 	struct strbuf path = STRBUF_INIT;
@@ -137,9 +137,9 @@ struct worktree **get_worktrees(unsigned flags)
 
 	ALLOC_ARRAY(list, alloc);
 
-	list[counter++] = get_main_worktree();
+	list[counter++] = get_main_worktree(r);
 
-	strbuf_addf(&path, "%s/worktrees", get_git_common_dir());
+	strbuf_addf(&path, "%s/worktrees", r->commondir);
 	dir = opendir(path.buf);
 	strbuf_release(&path);
 	if (dir) {
@@ -148,7 +148,7 @@ struct worktree **get_worktrees(unsigned flags)
 			if (is_dot_or_dotdot(d->d_name))
 				continue;
 
-			if ((linked = get_linked_worktree(d->d_name))) {
+			if ((linked = get_linked_worktree(r, d->d_name))) {
 				ALLOC_GROW(list, counter + 1, alloc);
 				list[counter++] = linked;
 			}
diff --git a/worktree.h b/worktree.h
index 9e3b0b7b6f..cb4307db16 100644
--- a/worktree.h
+++ b/worktree.h
@@ -30,7 +30,8 @@ struct worktree {
  * The caller is responsible for freeing the memory from the returned
  * worktree(s).
  */
-extern struct worktree **get_worktrees(unsigned flags);
+struct worktree **repo_get_worktrees(struct repository *r, unsigned flags);
+#define get_worktrees(flags) repo_get_worktrees(the_repository, flags)
 
 /*
  * Returns 1 if linked worktrees exist, 0 otherwise.
-- 8< --



>  			do_add_index_objects_to_pending(revs, &istate, flags);
>  		discard_index(&istate);
>  	}
> -- 
> 2.20.1
> 

Re: [PATCH][RFC] read-cache: read_index_from() accepts repo as arg

[Duy Nguyen]

On Sun, Apr 07, 2019 at 05:00:11PM +0700, Duy Nguyen wrote:
> So, with something like a patch below (not tested), you should be able
> to write
> 
> 	worktrees = repo_get_worktrees(revs->repo, 0);
> 	...
> 		struct repository *r = repo_worktree_init(wt);
> 		if (read_index_from(&istate, r->index_file, r) > 0)
> 			do_add_index_objects_to_pending(revs, &istate, flags);
> 		repo_clear(r);

Actually it gets better. You can just use repo_read_index()

diff --git a/revision.c b/revision.c
index eb8e51bc63..93fe8bbbfe 100644
--- a/revision.c
+++ b/revision.c
@@ -1546,19 +1546,18 @@ void add_index_objects_to_pending(struct rev_info *revs, unsigned int flags)
 	if (revs->single_worktree)
 		return;
 
-	worktrees = get_worktrees(0);
+	worktrees = repo_get_worktrees(revs->repo);
 	for (p = worktrees; *p; p++) {
+		struct repository repo;
 		struct worktree *wt = *p;
-		struct index_state istate = { NULL };
 
 		if (wt->is_current)
 			continue; /* current index already taken care of */
 
-		if (read_index_from(&istate,
-				    worktree_git_path(wt, "index"),
-				    get_worktree_git_dir(wt)) > 0)
-			do_add_index_objects_to_pending(revs, &istate, flags);
-		discard_index(&istate);
+		repo_worktree_init(repo, revs->repo, wt);
+		if (repo_read_index(repo) > 0)
+			do_add_index_objects_to_pending(revs, repo->index, flags);
+		repo_clear(&repo);
 	}
 	free_worktrees(worktrees);
 }
--
Duy

Re: [PATCH][RFC] read-cache: read_index_from() accepts repo as arg

[Taylor Blau]

Hi Kapil,

On Sun, Apr 07, 2019 at 01:07:12PM +0530, Kapil Jain wrote:
> Signed-off-by: Kapil Jain <jkapil.cs@gmail.com>
> ---
>
> In read-cache, the read_index_from() function had a TODO task,
> this patch completes that. There are some other functions in the same file
> where this exact TODO needs to be done, will proceed to do them once this patch is accepted.
>
> running test gave 256 not okays, each had a label as `# TODO known breakage`, which i think
> are not concerned to this patch.

Please make sure to wrap your commit messages at 72 characters per-line.
Incidentally, I just wrote another email about this same topic [1],
which has some good advice for how to do this in an automated way.

>  apply.c            | 2 +-
>  builtin/worktree.c | 2 +-
>  cache-tree.c       | 2 +-
>  cache.h            | 4 ++--
>  read-cache.c       | 4 ++--
>  repository.c       | 2 +-
>  revision.c         | 2 +-
>  7 files changed, 9 insertions(+), 9 deletions(-)
>
> diff --git a/apply.c b/apply.c
> index f15afa9f6a..3b4d128149 100644
> --- a/apply.c
> +++ b/apply.c
> @@ -4021,7 +4021,7 @@ static int read_apply_cache(struct apply_state *state)
>  {
>  	if (state->index_file)
>  		return read_index_from(state->repo->index, state->index_file,
> -				       get_git_dir());
> +				       get_git_dir(), state->repo);
>  	else
>  		return repo_read_index(state->repo);
>  }
> diff --git a/builtin/worktree.c b/builtin/worktree.c
> index 6cc094a453..874adebd2c 100644
> --- a/builtin/worktree.c
> +++ b/builtin/worktree.c
> @@ -737,7 +737,7 @@ static void validate_no_submodules(const struct worktree *wt)
>  		 */
>  		found_submodules = 1;
>  	} else if (read_index_from(&istate, worktree_git_path(wt, "index"),
> -				   get_worktree_git_dir(wt)) > 0) {
> +				   get_worktree_git_dir(wt), the_repository) > 0) {
>  		for (i = 0; i < istate.cache_nr; i++) {
>  			struct cache_entry *ce = istate.cache[i];
>  			int err;
> diff --git a/cache-tree.c b/cache-tree.c
> index b13bfaf71e..84f19b224e 100644
> --- a/cache-tree.c
> +++ b/cache-tree.c
> @@ -616,7 +616,7 @@ int write_index_as_tree(struct object_id *oid, struct index_state *index_state,
>
>  	hold_lock_file_for_update(&lock_file, index_path, LOCK_DIE_ON_ERROR);
>
> -	entries = read_index_from(index_state, index_path, get_git_dir());
> +	entries = read_index_from(index_state, index_path, get_git_dir(), the_repository);
>  	if (entries < 0) {
>  		ret = WRITE_TREE_UNREADABLE_INDEX;
>  		goto out;
> diff --git a/cache.h b/cache.h
> index ac92421f3a..3850c82fc9 100644
> --- a/cache.h
> +++ b/cache.h
> @@ -420,7 +420,7 @@ extern struct index_state the_index;
>  #define active_cache_tree (the_index.cache_tree)
>
>  #define read_cache() repo_read_index(the_repository)
> -#define read_cache_from(path) read_index_from(&the_index, (path), (get_git_dir()))
> +#define read_cache_from(path) read_index_from(&the_index, (path), (get_git_dir()), the_repository)
>  #define read_cache_preload(pathspec) repo_read_index_preload(the_repository, (pathspec), 0)
>  #define is_cache_unborn() is_index_unborn(&the_index)
>  #define read_cache_unmerged() repo_read_index_unmerged(the_repository)
> @@ -678,7 +678,7 @@ extern void preload_index(struct index_state *index,
>  extern int do_read_index(struct index_state *istate, const char *path,
>  			 int must_exist); /* for testting only! */
>  extern int read_index_from(struct index_state *, const char *path,
> -			   const char *gitdir);
> +			   const char *gitdir, const struct repository *repo);
>  extern int is_index_unborn(struct index_state *);
>
>  /* For use with `write_locked_index()`. */
> diff --git a/read-cache.c b/read-cache.c
> index 4dc6de1b55..0444703284 100644
> --- a/read-cache.c
> +++ b/read-cache.c
> @@ -2256,7 +2256,7 @@ static void freshen_shared_index(const char *shared_index, int warn)
>  }
>
>  int read_index_from(struct index_state *istate, const char *path,
> -		    const char *gitdir)
> +		    const char *gitdir, const struct repository *repo)
>  {
>  	struct split_index *split_index;
>  	int ret;
> @@ -2292,7 +2292,7 @@ int read_index_from(struct index_state *istate, const char *path,
>  		split_index->base = xcalloc(1, sizeof(*split_index->base));
>
>  	base_oid_hex = oid_to_hex(&split_index->base_oid);
> -	base_path = xstrfmt("%s/sharedindex.%s", gitdir, base_oid_hex);
> +	base_path = xstrfmt("%s/sharedindex.%s", repo->gitdir, base_oid_hex);
>  	trace2_region_enter_printf("index", "shared/do_read_index",
>  				   the_repository, "%s", base_path);
>  	ret = do_read_index(split_index->base, base_path, 1);
> diff --git a/repository.c b/repository.c
> index 682c239fe3..8ac2b65f61 100644
> --- a/repository.c
> +++ b/repository.c
> @@ -264,7 +264,7 @@ int repo_read_index(struct repository *repo)
>  	if (!repo->index)
>  		repo->index = xcalloc(1, sizeof(*repo->index));
>
> -	return read_index_from(repo->index, repo->index_file, repo->gitdir);
> +	return read_index_from(repo->index, repo->index_file, repo->gitdir, repo);
>  }
>
>  int repo_hold_locked_index(struct repository *repo,
> diff --git a/revision.c b/revision.c
> index eb8e51bc63..247a4d5704 100644
> --- a/revision.c
> +++ b/revision.c
> @@ -1556,7 +1556,7 @@ void add_index_objects_to_pending(struct rev_info *revs, unsigned int flags)
>
>  		if (read_index_from(&istate,
>  				    worktree_git_path(wt, "index"),
> -				    get_worktree_git_dir(wt)) > 0)
> +				    get_worktree_git_dir(wt), the_repository) > 0)
>  			do_add_index_objects_to_pending(revs, &istate, flags);
>  		discard_index(&istate);
>  	}
> --
> 2.20.1
>
Thanks,
Taylor

[1]: https://public-inbox.org/git/20190409020004.GA81620@Taylors-MBP.hsd1.wa.comcast.net/