On 08/13, Nguyễn Thái Ngọc Duy wrote:
> Make the attr API take an index_state instead of assuming the_index in
> attr code. All call sites are converted blindly to keep the patch
> simple and retain current behavior. Individual call sites may receive
> further updates to use the right index instead of the_index.
>
> There is one ugly temporary workaround added in attr.c that needs some
> more explanation.
>
> Commit c24f3abace (apply: file commited with CRLF should roundtrip
> diff and apply - 2017-08-19) forces one convert_to_git() call to NOT
> read the index at all. But what do you know, we read it anyway by
> falling back to the_index. When "istate" from convert_to_git is now
> propagated down to read_attr_from_array() we will hit segfault
> somewhere inside read_blob_data_from_index.
>
> The right way of dealing with this is to kill "use_index" variable and
> only follow "istate" but at this stage we are not ready for that:
> while most git_attr_set_direction() calls just passes the_index to be
> assigned to use_index, unpack-trees passes a different one which is
> used by entry.c code, which has no way to know what index to use if we
> delete use_index. So this has to be done later.
Yep, I remember back when I was doing some refactorings on the attr
system trying to get rid of the whole "use_index" thing. At that point
in time it wasn't feasible to do so, so i'm excited that it should be
done soon! :D
>
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@xxxxxxxxx>
> ---
> archive.c | 2 +-
> attr.c | 57 ++++++++++++++++++++++++++++--------------
> attr.h | 10 +++++---
> builtin/check-attr.c | 4 +--
> builtin/pack-objects.c | 2 +-
> convert.c | 2 +-
> dir.c | 2 +-
> ll-merge.c | 4 +--
> userdiff.c | 2 +-
> ws.c | 2 +-
> 10 files changed, 55 insertions(+), 32 deletions(-)
>
> diff --git a/archive.c b/archive.c
> index 78b0a398a0..a8397e6173 100644
> --- a/archive.c
> +++ b/archive.c
> @@ -109,7 +109,7 @@ static const struct attr_check *get_archive_attrs(const char *path)
> static struct attr_check *check;
> if (!check)
> check = attr_check_initl("export-ignore", "export-subst", NULL);
> - return git_check_attr(path, check) ? NULL : check;
> + return git_check_attr(&the_index, path, check) ? NULL : check;
> }
>
> static int check_attr_export_ignore(const struct attr_check *check)
> diff --git a/attr.c b/attr.c
> index 067fb9e0c0..863fad3bd1 100644
> --- a/attr.c
> +++ b/attr.c
> @@ -708,10 +708,10 @@ static struct attr_stack *read_attr_from_array(const char **list)
> * another thread could potentially be calling into the attribute system.
> */
> static enum git_attr_direction direction;
> -static struct index_state *use_index;
> +static const struct index_state *use_index;
>
> void git_attr_set_direction(enum git_attr_direction new_direction,
> - struct index_state *istate)
> + const struct index_state *istate)
> {
> if (is_bare_repository() && new_direction != GIT_ATTR_INDEX)
> BUG("non-INDEX attr direction in a bare repo");
> @@ -743,13 +743,24 @@ static struct attr_stack *read_attr_from_file(const char *path, int macro_ok)
> return res;
> }
>
> -static struct attr_stack *read_attr_from_index(const char *path, int macro_ok)
> +static struct attr_stack *read_attr_from_index(const struct index_state *istate,
> + const char *path,
> + int macro_ok)
> {
> struct attr_stack *res;
> char *buf, *sp;
> int lineno = 0;
> + const struct index_state *to_read_from;
>
> - buf = read_blob_data_from_index(use_index ? use_index : &the_index, path, NULL);
> + /*
> + * Temporary workaround for c24f3abace (apply: file commited
> + * with CRLF should roundtrip diff and apply - 2017-08-19)
> + */
> + to_read_from = use_index ? use_index : istate;
> + if (!to_read_from)
> + return NULL;
> +
> + buf = read_blob_data_from_index(to_read_from, path, NULL);
> if (!buf)
> return NULL;
>
> @@ -768,15 +779,16 @@ static struct attr_stack *read_attr_from_index(const char *path, int macro_ok)
> return res;
> }
>
> -static struct attr_stack *read_attr(const char *path, int macro_ok)
> +static struct attr_stack *read_attr(const struct index_state *istate,
> + const char *path, int macro_ok)
> {
> struct attr_stack *res = NULL;
>
> if (direction == GIT_ATTR_INDEX) {
> - res = read_attr_from_index(path, macro_ok);
> + res = read_attr_from_index(istate, path, macro_ok);
> } else if (!is_bare_repository()) {
> if (direction == GIT_ATTR_CHECKOUT) {
> - res = read_attr_from_index(path, macro_ok);
> + res = read_attr_from_index(istate, path, macro_ok);
> if (!res)
> res = read_attr_from_file(path, macro_ok);
> } else if (direction == GIT_ATTR_CHECKIN) {
> @@ -788,7 +800,7 @@ static struct attr_stack *read_attr(const char *path, int macro_ok)
> * We allow operation in a sparsely checked out
> * work tree, so read from it.
> */
> - res = read_attr_from_index(path, macro_ok);
> + res = read_attr_from_index(istate, path, macro_ok);
> }
> }
>
> @@ -859,7 +871,8 @@ static void push_stack(struct attr_stack **attr_stack_p,
> }
> }
>
> -static void bootstrap_attr_stack(struct attr_stack **stack)
> +static void bootstrap_attr_stack(const struct index_state *istate,
> + struct attr_stack **stack)
> {
> struct attr_stack *e;
>
> @@ -883,7 +896,7 @@ static void bootstrap_attr_stack(struct attr_stack **stack)
> }
>
> /* root directory */
> - e = read_attr(GITATTRIBUTES_FILE, 1);
> + e = read_attr(istate, GITATTRIBUTES_FILE, 1);
> push_stack(stack, e, xstrdup(""), 0);
>
> /* info frame */
> @@ -896,7 +909,8 @@ static void bootstrap_attr_stack(struct attr_stack **stack)
> push_stack(stack, e, NULL, 0);
> }
>
> -static void prepare_attr_stack(const char *path, int dirlen,
> +static void prepare_attr_stack(const struct index_state *istate,
> + const char *path, int dirlen,
> struct attr_stack **stack)
> {
> struct attr_stack *info;
> @@ -917,7 +931,7 @@ static void prepare_attr_stack(const char *path, int dirlen,
> * .gitattributes in deeper directories to shallower ones,
> * and finally use the built-in set as the default.
> */
> - bootstrap_attr_stack(stack);
> + bootstrap_attr_stack(istate, stack);
>
> /*
> * Pop the "info" one that is always at the top of the stack.
> @@ -973,7 +987,7 @@ static void prepare_attr_stack(const char *path, int dirlen,
> strbuf_add(&pathbuf, path + pathbuf.len, (len - pathbuf.len));
> strbuf_addf(&pathbuf, "/%s", GITATTRIBUTES_FILE);
>
> - next = read_attr(pathbuf.buf, 0);
> + next = read_attr(istate, pathbuf.buf, 0);
>
> /* reset the pathbuf to not include "/.gitattributes" */
> strbuf_setlen(&pathbuf, len);
> @@ -1095,7 +1109,9 @@ static void determine_macros(struct all_attrs_item *all_attrs,
> * If check->check_nr is non-zero, only attributes in check[] are collected.
> * Otherwise all attributes are collected.
> */
> -static void collect_some_attrs(const char *path, struct attr_check *check)
> +static void collect_some_attrs(const struct index_state *istate,
> + const char *path,
> + struct attr_check *check)
> {
> int i, pathlen, rem, dirlen;
> const char *cp, *last_slash = NULL;
> @@ -1114,7 +1130,7 @@ static void collect_some_attrs(const char *path, struct attr_check *check)
> dirlen = 0;
> }
>
> - prepare_attr_stack(path, dirlen, &check->stack);
> + prepare_attr_stack(istate, path, dirlen, &check->stack);
> all_attrs_init(&g_attr_hashmap, check);
> determine_macros(check->all_attrs, check->stack);
>
> @@ -1136,11 +1152,13 @@ static void collect_some_attrs(const char *path, struct attr_check *check)
> fill(path, pathlen, basename_offset, check->stack, check->all_attrs, rem);
> }
>
> -int git_check_attr(const char *path, struct attr_check *check)
> +int git_check_attr(const struct index_state *istate,
> + const char *path,
> + struct attr_check *check)
> {
> int i;
>
> - collect_some_attrs(path, check);
> + collect_some_attrs(istate, path, check);
>
> for (i = 0; i < check->nr; i++) {
> size_t n = check->items[i].attr->attr_nr;
> @@ -1153,12 +1171,13 @@ int git_check_attr(const char *path, struct attr_check *check)
> return 0;
> }
>
> -void git_all_attrs(const char *path, struct attr_check *check)
> +void git_all_attrs(const struct index_state *istate,
> + const char *path, struct attr_check *check)
> {
> int i;
>
> attr_check_reset(check);
> - collect_some_attrs(path, check);
> + collect_some_attrs(istate, path, check);
>
> for (i = 0; i < check->all_attrs_nr; i++) {
> const char *name = check->all_attrs[i].attr->name;
> diff --git a/attr.h b/attr.h
> index 46340010bb..3daca3c0cb 100644
> --- a/attr.h
> +++ b/attr.h
> @@ -1,6 +1,8 @@
> #ifndef ATTR_H
> #define ATTR_H
>
> +struct index_state;
> +
> /* An attribute is a pointer to this opaque structure */
> struct git_attr;
>
> @@ -60,13 +62,15 @@ void attr_check_free(struct attr_check *check);
> */
> const char *git_attr_name(const struct git_attr *);
>
> -int git_check_attr(const char *path, struct attr_check *check);
> +int git_check_attr(const struct index_state *istate,
> + const char *path, struct attr_check *check);
>
> /*
> * Retrieve all attributes that apply to the specified path.
> * check holds the attributes and their values.
> */
> -void git_all_attrs(const char *path, struct attr_check *check);
> +void git_all_attrs(const struct index_state *istate,
> + const char *path, struct attr_check *check);
>
> enum git_attr_direction {
> GIT_ATTR_CHECKIN,
> @@ -74,7 +78,7 @@ enum git_attr_direction {
> GIT_ATTR_INDEX
> };
> void git_attr_set_direction(enum git_attr_direction new_direction,
> - struct index_state *istate);
> + const struct index_state *istate);
>
> void attr_start(void);
>
> diff --git a/builtin/check-attr.c b/builtin/check-attr.c
> index 91444dc044..f7b59993d3 100644
> --- a/builtin/check-attr.c
> +++ b/builtin/check-attr.c
> @@ -63,9 +63,9 @@ static void check_attr(const char *prefix,
> prefix_path(prefix, prefix ? strlen(prefix) : 0, file);
>
> if (collect_all) {
> - git_all_attrs(full_path, check);
> + git_all_attrs(&the_index, full_path, check);
> } else {
> - if (git_check_attr(full_path, check))
> + if (git_check_attr(&the_index, full_path, check))
> die("git_check_attr died");
> }
> output_attr(check, file);
> diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
> index 4391504a91..3ff6da441f 100644
> --- a/builtin/pack-objects.c
> +++ b/builtin/pack-objects.c
> @@ -945,7 +945,7 @@ static int no_try_delta(const char *path)
>
> if (!check)
> check = attr_check_initl("delta", NULL);
> - if (git_check_attr(path, check))
> + if (git_check_attr(&the_index, path, check))
> return 0;
> if (ATTR_FALSE(check->items[0].value))
> return 1;
> diff --git a/convert.c b/convert.c
> index 7907efd16f..1935bde929 100644
> --- a/convert.c
> +++ b/convert.c
> @@ -1303,7 +1303,7 @@ static void convert_attrs(struct conv_attrs *ca, const char *path)
> git_config(read_convert_config, NULL);
> }
>
> - if (!git_check_attr(path, check)) {
> + if (!git_check_attr(&the_index, path, check)) {
> struct attr_check_item *ccheck = check->items;
> ca->crlf_action = git_path_check_crlf(ccheck + 4);
> if (ca->crlf_action == CRLF_UNDEFINED)
> diff --git a/dir.c b/dir.c
> index 21e6f2520a..29fbbd48c8 100644
> --- a/dir.c
> +++ b/dir.c
> @@ -281,7 +281,7 @@ static int match_attrs(const char *name, int namelen,
> {
> int i;
>
> - git_check_attr(name, item->attr_check);
> + git_check_attr(&the_index, name, item->attr_check);
> for (i = 0; i < item->attr_match_nr; i++) {
> const char *value;
> int matched;
> diff --git a/ll-merge.c b/ll-merge.c
> index a6ad2ec12d..0e2800f7bb 100644
> --- a/ll-merge.c
> +++ b/ll-merge.c
> @@ -371,7 +371,7 @@ int ll_merge(mmbuffer_t *result_buf,
> if (!check)
> check = attr_check_initl("merge", "conflict-marker-size", NULL);
>
> - if (!git_check_attr(path, check)) {
> + if (!git_check_attr(&the_index, path, check)) {
> ll_driver_name = check->items[0].value;
> if (check->items[1].value) {
> marker_size = atoi(check->items[1].value);
> @@ -398,7 +398,7 @@ int ll_merge_marker_size(const char *path)
>
> if (!check)
> check = attr_check_initl("conflict-marker-size", NULL);
> - if (!git_check_attr(path, check) && check->items[0].value) {
> + if (!git_check_attr(&the_index, path, check) && check->items[0].value) {
> marker_size = atoi(check->items[0].value);
> if (marker_size <= 0)
> marker_size = DEFAULT_CONFLICT_MARKER_SIZE;
> diff --git a/userdiff.c b/userdiff.c
> index 36af25e7f9..f3f4be579c 100644
> --- a/userdiff.c
> +++ b/userdiff.c
> @@ -278,7 +278,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(path, check))
> + if (git_check_attr(&the_index, path, check))
> return NULL;
>
> if (ATTR_TRUE(check->items[0].value))
> diff --git a/ws.c b/ws.c
> index a07caedd5a..5b67b426e7 100644
> --- a/ws.c
> +++ b/ws.c
> @@ -78,7 +78,7 @@ unsigned whitespace_rule(const char *pathname)
> if (!attr_whitespace_rule)
> attr_whitespace_rule = attr_check_initl("whitespace", NULL);
>
> - if (!git_check_attr(pathname, attr_whitespace_rule)) {
> + if (!git_check_attr(&the_index, pathname, attr_whitespace_rule)) {
> const char *value;
>
> value = attr_whitespace_rule->items[0].value;
> --
> 2.18.0.1004.g6639190530
>
--
Brandon Williams
