diff mbox series

[v5,17/23] userdiff.c: remove implicit dependency on the_index

Message ID 20180921155739.14407-18-pclouds@gmail.com (mailing list archive)
State New, archived
Headers show
Series Kill the_index part 4 | expand

Commit Message

Duy Nguyen Sept. 21, 2018, 3:57 p.m. UTC 
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 archive-zip.c      | 14 +++++++++-----
 builtin/grep.c     |  3 ++-
 combine-diff.c     |  2 +-
 diff.c             | 40 +++++++++++++++++++++++-----------------
 diff.h             |  3 ++-
 diffcore-pickaxe.c |  4 ++--
 grep.c             | 21 ++++++++++++---------
 grep.h             |  3 ++-
 line-range.c       |  2 +-
 userdiff.c         |  5 +++--
 userdiff.h         |  3 ++-
 11 files changed, 59 insertions(+), 41 deletions(-)

Comments

Junio C Hamano Sept. 21, 2018, 4:46 p.m. UTC | #1 
Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:

> diff --git a/userdiff.h b/userdiff.h
> index 2ef0ce5452..dad3fc03c1 100644
> --- a/userdiff.h
> +++ b/userdiff.h
> @@ -21,7 +21,8 @@ struct userdiff_driver {
>  
>  int userdiff_config(const char *k, const char *v);
>  struct userdiff_driver *userdiff_find_by_name(const char *name);
> -struct userdiff_driver *userdiff_find_by_path(const char *path);
> +struct userdiff_driver *userdiff_find_by_path(struct index_state *istate,
> +					      const char *path);
>  
>  /*
>   * Initialize any textconv-related fields in the driver and return it, or NULL

This needs fix described in

https://public-inbox.org/git/c46ca4a9-6822-436c-8e84-95b977527912@ramsayjones.plus.com/

I can squash it in.
Jeff King Oct. 10, 2018, 2:51 p.m. UTC | #2 
On Fri, Sep 21, 2018 at 05:57:33PM +0200, Nguyễn Thái Ngọc Duy wrote:

> diff --git a/diff.c b/diff.c
> index 140d0e86df..5256b9eabc 100644
> --- a/diff.c
> +++ b/diff.c
> @@ -2093,23 +2093,25 @@ static void diff_words_flush(struct emit_callback *ecbdata)
>  	}
>  }
>  
> -static void diff_filespec_load_driver(struct diff_filespec *one)
> +static void diff_filespec_load_driver(struct diff_filespec *one,
> +				      struct index_state *istate)

I hadn't looked at this topic until today, when I tried merging next
with some of my other local bits and ran into conflicts. I found the
idea of passing index_state here, rather than the whole "struct
repository", a bit curious.

I get why you're doing it: your topic here only cares about removing
index dependencies, so you did the minimal thing to move that forward.

But if you think about what this function is doing, it is quite clearly
dependent on the whole repository, since the userdiff config we're
looking up may come from repo config.

So I think in the long run this probably should take a repository
argument, and use r->index instead of a separate istate argument. That
said, I'm not totally opposed to taking this incremental step and
letting somebody later sort out the config portions. I wonder if it
would be worth calling that out in the commit message to help that later
person. But I guess it is a bit late if this is already in next.

Maybe this email will serve the same purpose. :)

-Peff
Junio C Hamano Oct. 10, 2018, 10:14 p.m. UTC | #3 
Jeff King <peff@peff.net> writes:

> I get why you're doing it: your topic here only cares about removing
> index dependencies, so you did the minimal thing to move that forward.
>
> But if you think about what this function is doing, it is quite clearly
> dependent on the whole repository, since the userdiff config we're
> looking up may come from repo config.

In the case of userdiff that is pretty much limited to read-only
operation, I fully agree, but in more general cases, we would need
to pass both the repository and an in-core index separately, I would
say.  Imagine doing a partial commit, where we construct a separate
istate that is not the "repo's index" and use that to write out a
tree object to be wrapped in a new commit, and update the current
branch ref.
Jeff King Oct. 11, 2018, 12:10 a.m. UTC | #4 
On Thu, Oct 11, 2018 at 07:14:31AM +0900, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > I get why you're doing it: your topic here only cares about removing
> > index dependencies, so you did the minimal thing to move that forward.
> >
> > But if you think about what this function is doing, it is quite clearly
> > dependent on the whole repository, since the userdiff config we're
> > looking up may come from repo config.
> 
> In the case of userdiff that is pretty much limited to read-only
> operation, I fully agree, but in more general cases, we would need
> to pass both the repository and an in-core index separately, I would
> say.  Imagine doing a partial commit, where we construct a separate
> istate that is not the "repo's index" and use that to write out a
> tree object to be wrapped in a new commit, and update the current
> branch ref.

Yeah, agreed. I was actually puzzled at first why userdiff needs to know
about the index at all. But the answer is that the attr code may read
.gitattributes out of the index. It's _possible_ somebody would want to
do that with an index besides the normal repo one, but I find it
somewhat unlikely. I think my instinct there is based on it being
"read-only", as you said.

One thing that confused me even more is that diff_options now has a
"struct repository" field in it. I get how that saves passing it around,
but I also wonder if it may run into similar issues at some point. I'm
perfectly willing to punt on it until it actually comes up in practice,
though.

-Peff
Duy Nguyen Oct. 14, 2018, 12:33 p.m. UTC | #5 
On Wed, Oct 10, 2018 at 4:51 PM Jeff King <peff@peff.net> wrote:
>
> On Fri, Sep 21, 2018 at 05:57:33PM +0200, Nguyễn Thái Ngọc Duy wrote:
>
> > diff --git a/diff.c b/diff.c
> > index 140d0e86df..5256b9eabc 100644
> > --- a/diff.c
> > +++ b/diff.c
> > @@ -2093,23 +2093,25 @@ static void diff_words_flush(struct emit_callback *ecbdata)
> >       }
> >  }
> >
> > -static void diff_filespec_load_driver(struct diff_filespec *one)
> > +static void diff_filespec_load_driver(struct diff_filespec *one,
> > +                                   struct index_state *istate)
>
> I hadn't looked at this topic until today, when I tried merging next
> with some of my other local bits and ran into conflicts. I found the
> idea of passing index_state here, rather than the whole "struct
> repository", a bit curious.
>
> I get why you're doing it: your topic here only cares about removing
> index dependencies, so you did the minimal thing to move that forward.
>
> But if you think about what this function is doing, it is quite clearly
> dependent on the whole repository, since the userdiff config we're
> looking up may come from repo config.
>
> So I think in the long run this probably should take a repository
> argument, and use r->index instead of a separate istate argument. That
> said, I'm not totally opposed to taking this incremental step and
> letting somebody later sort out the config portions. I wonder if it
> would be worth calling that out in the commit message to help that later
> person. But I guess it is a bit late if this is already in next.

Maybe. When I made this series, I tried to reduce the access scope as
much as possible. If it turns out it needs more, we can always turn
"struct index_state *" into "struct repository *" later,
diff mbox series

Patch

diff --git a/archive-zip.c b/archive-zip.c
index 5a62351ab1..155ee4a779 100644
--- a/archive-zip.c
+++ b/archive-zip.c
@@ -264,9 +264,10 @@  static int has_only_ascii(const char *s)
 	}
 }
 
-static int entry_is_binary(const char *path, const void *buffer, size_t size)
+static int entry_is_binary(struct index_state *istate, const char *path,
+			   const void *buffer, size_t size)
 {
-	struct userdiff_driver *driver = userdiff_find_by_path(path);
+	struct userdiff_driver *driver = userdiff_find_by_path(istate, path);
 	if (!driver)
 		driver = userdiff_find_by_name("default");
 	if (driver->binary != -1)
@@ -352,7 +353,8 @@  static int write_zip_entry(struct archiver_args *args,
 				return error(_("cannot read %s"),
 					     oid_to_hex(oid));
 			crc = crc32(crc, buffer, size);
-			is_binary = entry_is_binary(path_without_prefix,
+			is_binary = entry_is_binary(args->repo->index,
+						    path_without_prefix,
 						    buffer, size);
 			out = buffer;
 		}
@@ -428,7 +430,8 @@  static int write_zip_entry(struct archiver_args *args,
 				break;
 			crc = crc32(crc, buf, readlen);
 			if (is_binary == -1)
-				is_binary = entry_is_binary(path_without_prefix,
+				is_binary = entry_is_binary(args->repo->index,
+							    path_without_prefix,
 							    buf, readlen);
 			write_or_die(1, buf, readlen);
 		}
@@ -460,7 +463,8 @@  static int write_zip_entry(struct archiver_args *args,
 				break;
 			crc = crc32(crc, buf, readlen);
 			if (is_binary == -1)
-				is_binary = entry_is_binary(path_without_prefix,
+				is_binary = entry_is_binary(args->repo->index,
+							    path_without_prefix,
 							    buf, readlen);
 
 			zstream.next_in = buf;
diff --git a/builtin/grep.c b/builtin/grep.c
index 0667ffde84..0c3527242e 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -103,7 +103,8 @@  static void add_work(struct grep_opt *opt, const struct grep_source *gs)
 
 	todo[todo_end].source = *gs;
 	if (opt->binary != GREP_BINARY_TEXT)
-		grep_source_load_driver(&todo[todo_end].source);
+		grep_source_load_driver(&todo[todo_end].source,
+					opt->repo->index);
 	todo[todo_end].done = 0;
 	strbuf_reset(&todo[todo_end].out);
 	todo_end = (todo_end + 1) % ARRAY_SIZE(todo);
diff --git a/combine-diff.c b/combine-diff.c
index 9b43e557a1..41ab5b01de 100644
--- a/combine-diff.c
+++ b/combine-diff.c
@@ -987,7 +987,7 @@  static void show_patch_diff(struct combine_diff_path *elem, int num_parent,
 	const char *line_prefix = diff_line_prefix(opt);
 
 	context = opt->context;
-	userdiff = userdiff_find_by_path(elem->path);
+	userdiff = userdiff_find_by_path(opt->repo->index, elem->path);
 	if (!userdiff)
 		userdiff = userdiff_find_by_name("default");
 	if (opt->flags.allow_textconv)
diff --git a/diff.c b/diff.c
index 140d0e86df..5256b9eabc 100644
--- a/diff.c
+++ b/diff.c
@@ -2093,23 +2093,25 @@  static void diff_words_flush(struct emit_callback *ecbdata)
 	}
 }
 
-static void diff_filespec_load_driver(struct diff_filespec *one)
+static void diff_filespec_load_driver(struct diff_filespec *one,
+				      struct index_state *istate)
 {
 	/* Use already-loaded driver */
 	if (one->driver)
 		return;
 
 	if (S_ISREG(one->mode))
-		one->driver = userdiff_find_by_path(one->path);
+		one->driver = userdiff_find_by_path(istate, one->path);
 
 	/* Fallback to default settings */
 	if (!one->driver)
 		one->driver = userdiff_find_by_name("default");
 }
 
-static const char *userdiff_word_regex(struct diff_filespec *one)
+static const char *userdiff_word_regex(struct diff_filespec *one,
+				       struct index_state *istate)
 {
-	diff_filespec_load_driver(one);
+	diff_filespec_load_driver(one, istate);
 	return one->driver->word_regex;
 }
 
@@ -2132,9 +2134,9 @@  static void init_diff_words_data(struct emit_callback *ecbdata,
 			xcalloc(1, sizeof(struct emitted_diff_symbols));
 
 	if (!o->word_regex)
-		o->word_regex = userdiff_word_regex(one);
+		o->word_regex = userdiff_word_regex(one, o->repo->index);
 	if (!o->word_regex)
-		o->word_regex = userdiff_word_regex(two);
+		o->word_regex = userdiff_word_regex(two, o->repo->index);
 	if (!o->word_regex)
 		o->word_regex = diff_word_regex_cfg;
 	if (o->word_regex) {
@@ -3257,7 +3259,7 @@  int diff_filespec_is_binary(struct repository *r,
 			    struct diff_filespec *one)
 {
 	if (one->is_binary == -1) {
-		diff_filespec_load_driver(one);
+		diff_filespec_load_driver(one, r->index);
 		if (one->driver->binary != -1)
 			one->is_binary = one->driver->binary;
 		else {
@@ -3273,9 +3275,10 @@  int diff_filespec_is_binary(struct repository *r,
 	return one->is_binary;
 }
 
-static const struct userdiff_funcname *diff_funcname_pattern(struct diff_filespec *one)
+static const struct userdiff_funcname *
+diff_funcname_pattern(struct diff_options *o, struct diff_filespec *one)
 {
-	diff_filespec_load_driver(one);
+	diff_filespec_load_driver(one, o->repo->index);
 	return one->driver->funcname.pattern ? &one->driver->funcname : NULL;
 }
 
@@ -3287,12 +3290,13 @@  void diff_set_mnemonic_prefix(struct diff_options *options, const char *a, const
 		options->b_prefix = b;
 }
 
-struct userdiff_driver *get_textconv(struct diff_filespec *one)
+struct userdiff_driver *get_textconv(struct index_state *istate,
+				     struct diff_filespec *one)
 {
 	if (!DIFF_FILE_VALID(one))
 		return NULL;
 
-	diff_filespec_load_driver(one);
+	diff_filespec_load_driver(one, istate);
 	return userdiff_get_textconv(one->driver);
 }
 
@@ -3342,8 +3346,8 @@  static void builtin_diff(const char *name_a,
 	}
 
 	if (o->flags.allow_textconv) {
-		textconv_one = get_textconv(one);
-		textconv_two = get_textconv(two);
+		textconv_one = get_textconv(o->repo->index, one);
+		textconv_two = get_textconv(o->repo->index, two);
 	}
 
 	/* Never use a non-valid filename anywhere if at all possible */
@@ -3465,9 +3469,9 @@  static void builtin_diff(const char *name_a,
 		mf1.size = fill_textconv(o->repo, textconv_one, one, &mf1.ptr);
 		mf2.size = fill_textconv(o->repo, textconv_two, two, &mf2.ptr);
 
-		pe = diff_funcname_pattern(one);
+		pe = diff_funcname_pattern(o, one);
 		if (!pe)
-			pe = diff_funcname_pattern(two);
+			pe = diff_funcname_pattern(o, two);
 
 		memset(&xpp, 0, sizeof(xpp));
 		memset(&xecfg, 0, sizeof(xecfg));
@@ -4223,7 +4227,9 @@  static void run_diff_cmd(const char *pgm,
 
 
 	if (o->flags.allow_external) {
-		struct userdiff_driver *drv = userdiff_find_by_path(attr_path);
+		struct userdiff_driver *drv;
+
+		drv = userdiff_find_by_path(o->repo->index, attr_path);
 		if (drv && drv->external)
 			pgm = drv->external;
 	}
@@ -6399,7 +6405,7 @@  int textconv_object(struct repository *r,
 
 	df = alloc_filespec(path);
 	fill_filespec(df, oid, oid_valid, mode);
-	textconv = get_textconv(df);
+	textconv = get_textconv(r->index, df);
 	if (!textconv) {
 		free_filespec(df);
 		return 0;
diff --git a/diff.h b/diff.h
index b88fccd2fb..af26196224 100644
--- a/diff.h
+++ b/diff.h
@@ -455,7 +455,8 @@  size_t fill_textconv(struct repository *r,
  * and only if it has textconv enabled (otherwise return NULL). The result
  * can be passed to fill_textconv().
  */
-struct userdiff_driver *get_textconv(struct diff_filespec *one);
+struct userdiff_driver *get_textconv(struct index_state *istate,
+				     struct diff_filespec *one);
 
 /*
  * Prepare diff_filespec and convert it using diff textconv API
diff --git a/diffcore-pickaxe.c b/diffcore-pickaxe.c
index 7a5cf446ff..d2361e06a1 100644
--- a/diffcore-pickaxe.c
+++ b/diffcore-pickaxe.c
@@ -139,8 +139,8 @@  static int pickaxe_match(struct diff_filepair *p, struct diff_options *o,
 		return 0;
 
 	if (o->flags.allow_textconv) {
-		textconv_one = get_textconv(p->one);
-		textconv_two = get_textconv(p->two);
+		textconv_one = get_textconv(o->repo->index, p->one);
+		textconv_two = get_textconv(o->repo->index, p->two);
 	}
 
 	/*
diff --git a/grep.c b/grep.c
index 6c0eede3a1..f6bd89e40b 100644
--- a/grep.c
+++ b/grep.c
@@ -11,7 +11,8 @@ 
 #include "help.h"
 
 static int grep_source_load(struct grep_source *gs);
-static int grep_source_is_binary(struct grep_source *gs);
+static int grep_source_is_binary(struct grep_source *gs,
+				 struct index_state *istate);
 
 static struct grep_opt grep_defaults;
 
@@ -1547,7 +1548,7 @@  static int match_funcname(struct grep_opt *opt, struct grep_source *gs, char *bo
 {
 	xdemitconf_t *xecfg = opt->priv;
 	if (xecfg && !xecfg->find_func) {
-		grep_source_load_driver(gs);
+		grep_source_load_driver(gs, opt->repo->index);
 		if (gs->driver->funcname.pattern) {
 			const struct userdiff_funcname *pe = &gs->driver->funcname;
 			xdiff_set_find_func(xecfg, pe->pattern, pe->cflags);
@@ -1804,7 +1805,7 @@  static int grep_source_1(struct grep_opt *opt, struct grep_source *gs, int colle
 	opt->last_shown = 0;
 
 	if (opt->allow_textconv) {
-		grep_source_load_driver(gs);
+		grep_source_load_driver(gs, opt->repo->index);
 		/*
 		 * We might set up the shared textconv cache data here, which
 		 * is not thread-safe.
@@ -1821,11 +1822,11 @@  static int grep_source_1(struct grep_opt *opt, struct grep_source *gs, int colle
 	if (!textconv) {
 		switch (opt->binary) {
 		case GREP_BINARY_DEFAULT:
-			if (grep_source_is_binary(gs))
+			if (grep_source_is_binary(gs, opt->repo->index))
 				binary_match_only = 1;
 			break;
 		case GREP_BINARY_NOMATCH:
-			if (grep_source_is_binary(gs))
+			if (grep_source_is_binary(gs, opt->repo->index))
 				return 0; /* Assume unmatch */
 			break;
 		case GREP_BINARY_TEXT:
@@ -2171,22 +2172,24 @@  static int grep_source_load(struct grep_source *gs)
 	BUG("invalid grep_source type to load");
 }
 
-void grep_source_load_driver(struct grep_source *gs)
+void grep_source_load_driver(struct grep_source *gs,
+			     struct index_state *istate)
 {
 	if (gs->driver)
 		return;
 
 	grep_attr_lock();
 	if (gs->path)
-		gs->driver = userdiff_find_by_path(gs->path);
+		gs->driver = userdiff_find_by_path(istate, gs->path);
 	if (!gs->driver)
 		gs->driver = userdiff_find_by_name("default");
 	grep_attr_unlock();
 }
 
-static int grep_source_is_binary(struct grep_source *gs)
+static int grep_source_is_binary(struct grep_source *gs,
+				 struct index_state *istate)
 {
-	grep_source_load_driver(gs);
+	grep_source_load_driver(gs, istate);
 	if (gs->driver->binary != -1)
 		return gs->driver->binary;
 
diff --git a/grep.h b/grep.h
index 3651183971..1a57d12b90 100644
--- a/grep.h
+++ b/grep.h
@@ -220,7 +220,8 @@  void grep_source_init(struct grep_source *gs, enum grep_source_type type,
 		      const void *identifier);
 void grep_source_clear_data(struct grep_source *gs);
 void grep_source_clear(struct grep_source *gs);
-void grep_source_load_driver(struct grep_source *gs);
+void grep_source_load_driver(struct grep_source *gs,
+			     struct index_state *istate);
 
 
 int grep_source(struct grep_opt *opt, struct grep_source *gs);
diff --git a/line-range.c b/line-range.c
index 232c3909ec..7fa0d8bba5 100644
--- a/line-range.c
+++ b/line-range.c
@@ -198,7 +198,7 @@  static const char *parse_range_funcname(const char *arg, nth_line_fn_t nth_line_
 	anchor--; /* input is in human terms */
 	start = nth_line_cb(cb_data, anchor);
 
-	drv = userdiff_find_by_path(path);
+	drv = userdiff_find_by_path(&the_index, path);
 	if (drv && drv->funcname.pattern) {
 		const struct userdiff_funcname *pe = &drv->funcname;
 		xecfg = xcalloc(1, sizeof(*xecfg));
diff --git a/userdiff.c b/userdiff.c
index f3f4be579c..c913232396 100644
--- a/userdiff.c
+++ b/userdiff.c
@@ -270,7 +270,8 @@  struct userdiff_driver *userdiff_find_by_name(const char *name) {
 	return userdiff_find_by_namelen(name, len);
 }
 
-struct userdiff_driver *userdiff_find_by_path(const char *path)
+struct userdiff_driver *userdiff_find_by_path(struct index_state *istate,
+					      const char *path)
 {
 	static struct attr_check *check;
 
@@ -278,7 +279,7 @@  struct userdiff_driver *userdiff_find_by_path(const char *path)
 		check = attr_check_initl("diff", NULL);
 	if (!path)
 		return NULL;
-	if (git_check_attr(&the_index, path, check))
+	if (git_check_attr(istate, path, check))
 		return NULL;
 
 	if (ATTR_TRUE(check->items[0].value))
diff --git a/userdiff.h b/userdiff.h
index 2ef0ce5452..dad3fc03c1 100644
--- a/userdiff.h
+++ b/userdiff.h
@@ -21,7 +21,8 @@  struct userdiff_driver {
 
 int userdiff_config(const char *k, const char *v);
 struct userdiff_driver *userdiff_find_by_name(const char *name);
-struct userdiff_driver *userdiff_find_by_path(const char *path);
+struct userdiff_driver *userdiff_find_by_path(struct index_state *istate,
+					      const char *path);
 
 /*
  * Initialize any textconv-related fields in the driver and return it, or NULL