Re: [PATCH v3 07/20] attr: remove an implicit dependency on the_index
On 06/06, Duy Nguyen wrote: > On Wed, Jun 6, 2018 at 6:50 PM, Brandon Williams wrote: > > On 06/06, 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. > > > > Ugh I remember this when I was doing some refactoring to the attr > > subsystem a year or so ago. I really wanted to get rid of the whole > > "direction" thing because if I remember correctly its one of the only > > remaining globals that affects the outcome of an attr check (everything > > else was converted to use the attr check struct. I always got way to > > far into the weeds when trying to do that too. I'm not expecting that > > from this series (since its way out of scope) but maybe it'll be easier > > afterwards. > > It's not that much easier. This direction thing is still global by > design. It would be great if we have something like Scheme's parameter > (aka. dynamic scoping iirc) then we can still scope this nicely. Git > does not live in such worlds. Yeah i realized this after sending. Either way thanks for simplifying the global state by getting ride of the index global. > -- > Duy -- Brandon Williams
Re: [PATCH v3 07/20] attr: remove an implicit dependency on the_index
On Wed, Jun 6, 2018 at 6:50 PM, Brandon Williams wrote: > On 06/06, 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. > > Ugh I remember this when I was doing some refactoring to the attr > subsystem a year or so ago. I really wanted to get rid of the whole > "direction" thing because if I remember correctly its one of the only > remaining globals that affects the outcome of an attr check (everything > else was converted to use the attr check struct. I always got way to > far into the weeds when trying to do that too. I'm not expecting that > from this series (since its way out of scope) but maybe it'll be easier > afterwards. It's not that much easier. This direction thing is still global by design. It would be great if we have something like Scheme's parameter (aka. dynamic scoping iirc) then we can still scope this nicely. Git does not live in such worlds. -- Duy
Re: [PATCH v3 07/20] attr: remove an implicit dependency on the_index
On 06/06, 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.
Ugh I remember this when I was doing some refactoring to the attr
subsystem a year or so ago. I really wanted to get rid of the whole
"direction" thing because if I remember correctly its one of the only
remaining globals that affects the outcome of an attr check (everything
else was converted to use the attr check struct. I always got way to
far into the weeds when trying to do that too. I'm not expecting that
from this series (since its way out of scope) but maybe it'll be easier
afterwards.
>
> Signed-off-by: Nguyễn Thái Ngọc Duy
> ---
> 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 4fe7bec60c..3b4db8956a 100644
> --- a/archive.c
> +++ b/archive.c
> @@ -108,7 +108,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) {
>
Re: [PATCH v3 07/20] attr: remove an implicit dependency on the_index
On 06/06/18 08:39, 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 s/commited * with/commited with/ ATB, Ramsay Jones
[PATCH v3 07/20] attr: remove an implicit dependency on the_index
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.
Signed-off-by: Nguyễn Thái Ngọc Duy
---
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 4fe7bec60c..3b4db8956a 100644
--- a/archive.c
+++ b/archive.c
@@ -108,7 +108,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 o
