* Re: [RFC/PATCH] attr: Document a new possible thread safe API
From: Stefan Beller @ 2016-10-04 23:25 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git@vger.kernel.org, Brandon Williams
In-Reply-To: <xmqqtwcrr9l6.fsf@gitster.mtv.corp.google.com>
On Tue, Oct 4, 2016 at 4:13 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Stefan Beller <sbeller@google.com> writes:
>
>> diff --git a/Documentation/technical/api-gitattributes.txt b/Documentation/technical/api-gitattributes.txt
>> index 92fc32a..940617e 100644
>> --- a/Documentation/technical/api-gitattributes.txt
>> +++ b/Documentation/technical/api-gitattributes.txt
>> @@ -59,7 +59,10 @@ Querying Specific Attributes
>> empty `struct git_attr_check` can be prepared by calling
>> `git_attr_check_alloc()` function and then attributes you want to
>> ask about can be added to it with `git_attr_check_append()`
>> - function.
>> + function. git_attr_check_initl is thread safe, i.e. you can call it
>> + from different threads at the same time; internally however only one
>> + call at a time is processed. If the calls from different threads have
>> + the same arguments, the returned `git_attr_check` may be the same.
>
> I do not think this is enough. Look at the example for _initl() and
> notice that it keeps the "singleton static check that is initialized
> by the very first caller if the caller notices it is NULL" pattern.
>
> One way to hide that may be to pass the address of that singleton
> pointer to _initl(), so that it can do the "has it been initialized?
> If not, let's prepare the thing" under lock.
Oh, I see. Yeah that makes sense.
>
>> @@ -89,15 +92,21 @@ static void setup_check(void)
>>
>> ------------
>> const char *path;
>> + struct git_attr_check *result;
>>
>> setup_check();
>> - git_check_attr(path, check);
>> + result = git_check_attr(path, check);
>
> I haven't formed a firm opinion, but I suspect your thinking might
> be clouded by too much staring at the current implementation that
> has <attr>,<value> pairs inside git_attr_check. Traditionally, the
> attr subsystem used the same type for the query question and the
> query answer the same type, but it does not have to stay to be the
> case at all. Have you considered that we are allowed to make these
> two types distinct?
I thought about that, but as I concluded that the get_all_attrs doesn't need
conversion to a threading environment, we can keep it as is.
When keeping the get_all_attrs as is, we need to keep the data structures
as is, (i.e. key,value pair inside git_check_attr), so introducing a new
data type seemed not useful for the threaded part.
> A caller can share the same question instance
> (i.e. the set of interned <attr>, in git_attr_check) with other
> threads as that is a read-only thing, but each of the callers would
> want to have the result array on its own stack if possible
> (e.g. when asking for a known set of attributes, which is the
> majority of the case) to avoid allocation cost. I'd expect the most
> typical caller to be
>
> static struct git_attr_check *check;
> struct git_attr_result result[2]; /* we want two */
>
> git_attr_check_initl(&check, "crlf", "ident", NULL);
> git_check_attr(path, check, result);
> /* result[0] has "crlf", result[1] has "ident" */
>
> or something like that.
I see, that seems to be a clean API. So git_attr_check_initl
will lock the mutex and once it got the mutex it can either
* return early as someone else did the work
* needs to do the actual work
and then unlock. In any case the work was done.
git_check_attr, which runs in all threads points to the same check,
but gets the different results.
Ok, I'll go in that direction then.
Thanks,
Stefan
^ permalink raw reply
* Re: [RFC/PATCH] attr: Document a new possible thread safe API
From: Junio C Hamano @ 2016-10-04 23:13 UTC (permalink / raw)
To: Stefan Beller; +Cc: git, bmwill
In-Reply-To: <20161004221433.23747-1-sbeller@google.com>
Stefan Beller <sbeller@google.com> writes:
> diff --git a/Documentation/technical/api-gitattributes.txt b/Documentation/technical/api-gitattributes.txt
> index 92fc32a..940617e 100644
> --- a/Documentation/technical/api-gitattributes.txt
> +++ b/Documentation/technical/api-gitattributes.txt
> @@ -59,7 +59,10 @@ Querying Specific Attributes
> empty `struct git_attr_check` can be prepared by calling
> `git_attr_check_alloc()` function and then attributes you want to
> ask about can be added to it with `git_attr_check_append()`
> - function.
> + function. git_attr_check_initl is thread safe, i.e. you can call it
> + from different threads at the same time; internally however only one
> + call at a time is processed. If the calls from different threads have
> + the same arguments, the returned `git_attr_check` may be the same.
I do not think this is enough. Look at the example for _initl() and
notice that it keeps the "singleton static check that is initialized
by the very first caller if the caller notices it is NULL" pattern.
One way to hide that may be to pass the address of that singleton
pointer to _initl(), so that it can do the "has it been initialized?
If not, let's prepare the thing" under lock.
> @@ -89,15 +92,21 @@ static void setup_check(void)
>
> ------------
> const char *path;
> + struct git_attr_check *result;
>
> setup_check();
> - git_check_attr(path, check);
> + result = git_check_attr(path, check);
I haven't formed a firm opinion, but I suspect your thinking might
be clouded by too much staring at the current implementation that
has <attr>,<value> pairs inside git_attr_check. Traditionally, the
attr subsystem used the same type for the query question and the
query answer the same type, but it does not have to stay to be the
case at all. Have you considered that we are allowed to make these
two types distinct? A caller can share the same question instance
(i.e. the set of interned <attr>, in git_attr_check) with other
threads as that is a read-only thing, but each of the callers would
want to have the result array on its own stack if possible
(e.g. when asking for a known set of attributes, which is the
majority of the case) to avoid allocation cost. I'd expect the most
typical caller to be
static struct git_attr_check *check;
struct git_attr_result result[2]; /* we want two */
git_attr_check_initl(&check, "crlf", "ident", NULL);
git_check_attr(path, check, result);
/* result[0] has "crlf", result[1] has "ident" */
or something like that.
^ permalink raw reply
* Re: GL bug: can not commit, reports error on changed submodule directory
From: Stefan Beller @ 2016-10-04 23:05 UTC (permalink / raw)
To: Junio C Hamano; +Cc: ern0, git@vger.kernel.org
In-Reply-To: <xmqqy423rabi.fsf@gitster.mtv.corp.google.com>
On Tue, Oct 4, 2016 at 3:58 PM, Junio C Hamano <gitster@pobox.com> wrote:
> ern0 <ern0@linkbroker.hu> writes:
>
>> When I say:
>> $ gl commit -m "blah blah"
>> It reports:
>> ✘ Failed to read file into stream: Is a directory
>
> Not that I am interested in learning the answer to the question, but
> what the h*ck is "gl"?
http://gitless.com/
See 20160930191413.002049b94b3908b15881b77f@domain007.com
"Purposes, Concepts,Misfits, and a Redesign of Git" (a research paper)
^ permalink raw reply
* Re: GL bug: can not commit, reports error on changed submodule directory
From: Junio C Hamano @ 2016-10-04 22:58 UTC (permalink / raw)
To: ern0; +Cc: git
In-Reply-To: <CALhephTkohVhEjdP7TwQAcBrEBiHGtp0Hd+UxPUiJHtubMWKGA@mail.gmail.com>
ern0 <ern0@linkbroker.hu> writes:
> When I say:
> $ gl commit -m "blah blah"
> It reports:
> ✘ Failed to read file into stream: Is a directory
Not that I am interested in learning the answer to the question, but
what the h*ck is "gl"?
^ permalink raw reply
* [PATCH 3/4] diff.c: move ws-error-highlight parsing helpers up
From: Junio C Hamano @ 2016-10-04 22:54 UTC (permalink / raw)
To: git; +Cc: peff, strk
In-Reply-To: <20161004225449.6759-1-gitster@pobox.com>
These need to be usable from git_diff_ui_config() code to help
parsing a configuration variable, so move them up.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
diff.c | 74 +++++++++++++++++++++++++++++++++---------------------------------
1 file changed, 37 insertions(+), 37 deletions(-)
diff --git a/diff.c b/diff.c
index d346378600..bd625cf3f7 100644
--- a/diff.c
+++ b/diff.c
@@ -163,6 +163,43 @@ long parse_algorithm_value(const char *value)
return -1;
}
+static int parse_one_token(const char **arg, const char *token)
+{
+ const char *rest;
+ if (skip_prefix(*arg, token, &rest) && (!*rest || *rest == ',')) {
+ *arg = rest;
+ return 1;
+ }
+ return 0;
+}
+
+static int parse_ws_error_highlight(const char *arg)
+{
+ const char *orig_arg = arg;
+ unsigned val = 0;
+
+ while (*arg) {
+ if (parse_one_token(&arg, "none"))
+ val = 0;
+ else if (parse_one_token(&arg, "default"))
+ val = WSEH_NEW;
+ else if (parse_one_token(&arg, "all"))
+ val = WSEH_NEW | WSEH_OLD | WSEH_CONTEXT;
+ else if (parse_one_token(&arg, "new"))
+ val |= WSEH_NEW;
+ else if (parse_one_token(&arg, "old"))
+ val |= WSEH_OLD;
+ else if (parse_one_token(&arg, "context"))
+ val |= WSEH_CONTEXT;
+ else {
+ return -1 - (int)(arg - orig_arg);
+ }
+ if (*arg)
+ arg++;
+ }
+ return val;
+}
+
/*
* These are to give UI layer defaults.
* The core-level commands such as git-diff-files should
@@ -3656,43 +3693,6 @@ static void enable_patch_output(int *fmt) {
*fmt |= DIFF_FORMAT_PATCH;
}
-static int parse_one_token(const char **arg, const char *token)
-{
- const char *rest;
- if (skip_prefix(*arg, token, &rest) && (!*rest || *rest == ',')) {
- *arg = rest;
- return 1;
- }
- return 0;
-}
-
-static int parse_ws_error_highlight(const char *arg)
-{
- const char *orig_arg = arg;
- unsigned val = 0;
-
- while (*arg) {
- if (parse_one_token(&arg, "none"))
- val = 0;
- else if (parse_one_token(&arg, "default"))
- val = WSEH_NEW;
- else if (parse_one_token(&arg, "all"))
- val = WSEH_NEW | WSEH_OLD | WSEH_CONTEXT;
- else if (parse_one_token(&arg, "new"))
- val |= WSEH_NEW;
- else if (parse_one_token(&arg, "old"))
- val |= WSEH_OLD;
- else if (parse_one_token(&arg, "context"))
- val |= WSEH_CONTEXT;
- else {
- return -1 - (int)(arg - orig_arg);
- }
- if (*arg)
- arg++;
- }
- return val;
-}
-
static int parse_ws_error_highlight_opt(struct diff_options *opt, const char *arg)
{
int val = parse_ws_error_highlight(arg);
--
2.10.1-510-g1ef781f2c1
^ permalink raw reply related
* [PATCH 4/4] diff: introduce diff.wsErrorHighlight option
From: Junio C Hamano @ 2016-10-04 22:54 UTC (permalink / raw)
To: git; +Cc: peff, strk
In-Reply-To: <20161004225449.6759-1-gitster@pobox.com>
With the preparatory steps, it has become trivial to teach the
system a new diff.wsErrorHighlight configuration that gives the
default value for --ws-error-highlight command line option.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
Documentation/diff-config.txt | 6 ++++++
Documentation/diff-options.txt | 2 ++
diff.c | 11 ++++++++++-
t/t4015-diff-whitespace.sh | 35 +++++++++++++++++++++++++++++++++++
4 files changed, 53 insertions(+), 1 deletion(-)
diff --git a/Documentation/diff-config.txt b/Documentation/diff-config.txt
index 6eaa45271c..c0396e66a5 100644
--- a/Documentation/diff-config.txt
+++ b/Documentation/diff-config.txt
@@ -182,3 +182,9 @@ diff.algorithm::
low-occurrence common elements".
--
+
+
+diff.wsErrorHighlight::
+ A comma separated list of `old`, `new`, `context`, that
+ specifies how whitespace errors on lines are highlighted
+ with `color.diff.whitespace`. Can be overridden by the
+ command line option `--ws-error-highlight=<kind>`
diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt
index 306b7e3604..dfd6bc99c6 100644
--- a/Documentation/diff-options.txt
+++ b/Documentation/diff-options.txt
@@ -303,6 +303,8 @@ ifndef::git-format-patch[]
lines are highlighted. E.g. `--ws-error-highlight=new,old`
highlights whitespace errors on both deleted and added lines.
`all` can be used as a short-hand for `old,new,context`.
+ The `diff.wsErrorHighlight` configuration variable can be
+ used to specify the default behaviour.
endif::git-format-patch[]
diff --git a/diff.c b/diff.c
index bd625cf3f7..9acf04f5b0 100644
--- a/diff.c
+++ b/diff.c
@@ -41,6 +41,7 @@ static int diff_stat_graph_width;
static int diff_dirstat_permille_default = 30;
static struct diff_options default_diff_options;
static long diff_algorithm;
+static unsigned ws_error_highlight_default = WSEH_NEW;
static char diff_colors[][COLOR_MAXLEN] = {
GIT_COLOR_RESET,
@@ -262,6 +263,14 @@ int git_diff_ui_config(const char *var, const char *value, void *cb)
return 0;
}
+ if (!strcmp(var, "diff.wserrorhighlight")) {
+ int val = parse_ws_error_highlight(value);
+ if (val < 0)
+ return -1;
+ ws_error_highlight_default = val;
+ return 0;
+ }
+
if (git_color_config(var, value, cb) < 0)
return -1;
@@ -3306,7 +3315,7 @@ void diff_setup(struct diff_options *options)
options->rename_limit = -1;
options->dirstat_permille = diff_dirstat_permille_default;
options->context = diff_context_default;
- options->ws_error_highlight = WSEH_NEW;
+ options->ws_error_highlight = ws_error_highlight_default;
DIFF_OPT_SET(options, RENAME_EMPTY);
/* pathchange left =NULL by default */
diff --git a/t/t4015-diff-whitespace.sh b/t/t4015-diff-whitespace.sh
index 4a4f374824..289806d0c7 100755
--- a/t/t4015-diff-whitespace.sh
+++ b/t/t4015-diff-whitespace.sh
@@ -937,4 +937,39 @@ test_expect_success 'test --ws-error-highlight option' '
'
+test_expect_success 'test diff.wsErrorHighlight config' '
+
+ git -c color.diff=always -c diff.wsErrorHighlight=default,old diff |
+ test_decode_color >current &&
+ test_cmp expect.default-old current &&
+
+ git -c color.diff=always -c diff.wsErrorHighlight=all diff |
+ test_decode_color >current &&
+ test_cmp expect.all current &&
+
+ git -c color.diff=always -c diff.wsErrorHighlight=none diff |
+ test_decode_color >current &&
+ test_cmp expect.none current
+
+'
+
+test_expect_success 'option overrides diff.wsErrorHighlight' '
+
+ git -c color.diff=always -c diff.wsErrorHighlight=none \
+ diff --ws-error-highlight=default,old |
+ test_decode_color >current &&
+ test_cmp expect.default-old current &&
+
+ git -c color.diff=always -c diff.wsErrorHighlight=default \
+ diff --ws-error-highlight=all |
+ test_decode_color >current &&
+ test_cmp expect.all current &&
+
+ git -c color.diff=always -c diff.wsErrorHighlight=all \
+ diff --ws-error-highlight=none |
+ test_decode_color >current &&
+ test_cmp expect.none current
+
+'
+
test_done
--
2.10.1-510-g1ef781f2c1
^ permalink raw reply related
* [PATCH 2/4] diff.c: refactor parse_ws_error_highlight()
From: Junio C Hamano @ 2016-10-04 22:54 UTC (permalink / raw)
To: git; +Cc: peff, strk
In-Reply-To: <20161004225449.6759-1-gitster@pobox.com>
Rename the function to parse_ws_error_highlight_opt(), because it is
meant to parse a command line option, and then refactor the meat of
the function into a helper function that reports the parsed result
which is typically a small unsigned int (these are OR'ed bitmask
after all), or a negative offset that indicates where in the input
string a parse error happened.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
diff.c | 21 ++++++++++++++++-----
1 file changed, 16 insertions(+), 5 deletions(-)
diff --git a/diff.c b/diff.c
index 46260ed7a1..d346378600 100644
--- a/diff.c
+++ b/diff.c
@@ -3666,10 +3666,11 @@ static int parse_one_token(const char **arg, const char *token)
return 0;
}
-static int parse_ws_error_highlight(struct diff_options *opt, const char *arg)
+static int parse_ws_error_highlight(const char *arg)
{
const char *orig_arg = arg;
unsigned val = 0;
+
while (*arg) {
if (parse_one_token(&arg, "none"))
val = 0;
@@ -3684,13 +3685,23 @@ static int parse_ws_error_highlight(struct diff_options *opt, const char *arg)
else if (parse_one_token(&arg, "context"))
val |= WSEH_CONTEXT;
else {
- error("unknown value after ws-error-highlight=%.*s",
- (int)(arg - orig_arg), orig_arg);
- return 0;
+ return -1 - (int)(arg - orig_arg);
}
if (*arg)
arg++;
}
+ return val;
+}
+
+static int parse_ws_error_highlight_opt(struct diff_options *opt, const char *arg)
+{
+ int val = parse_ws_error_highlight(arg);
+
+ if (val < 0) {
+ error("unknown value after ws-error-highlight=%.*s",
+ -1 - val, arg);
+ return 0;
+ }
opt->ws_error_highlight = val;
return 1;
}
@@ -3894,7 +3905,7 @@ int diff_opt_parse(struct diff_options *options, const char **av, int ac)
else if (skip_prefix(arg, "--submodule=", &arg))
return parse_submodule_opt(options, arg);
else if (skip_prefix(arg, "--ws-error-highlight=", &arg))
- return parse_ws_error_highlight(options, arg);
+ return parse_ws_error_highlight_opt(options, arg);
/* misc options */
else if (!strcmp(arg, "-z"))
--
2.10.1-510-g1ef781f2c1
^ permalink raw reply related
* [PATCH 1/4] t4015: split out the "setup" part of ws-error-highlight test
From: Junio C Hamano @ 2016-10-04 22:54 UTC (permalink / raw)
To: git; +Cc: peff, strk
In-Reply-To: <20161004225449.6759-1-gitster@pobox.com>
We'd want to run this same set of test twice, once with the option
and another time with an equivalent configuration setting. Split
out the step that prepares the test data and expected output and
move the test for the command line option into a separate test.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
t/t4015-diff-whitespace.sh | 39 +++++++++++++++++++++------------------
1 file changed, 21 insertions(+), 18 deletions(-)
diff --git a/t/t4015-diff-whitespace.sh b/t/t4015-diff-whitespace.sh
index 2434157aa7..4a4f374824 100755
--- a/t/t4015-diff-whitespace.sh
+++ b/t/t4015-diff-whitespace.sh
@@ -869,7 +869,8 @@ test_expect_success 'diff that introduces and removes ws breakages' '
test_cmp expected current
'
-test_expect_success 'the same with --ws-error-highlight' '
+test_expect_success 'ws-error-highlight test setup' '
+
git reset --hard &&
{
echo "0. blank-at-eol " &&
@@ -882,10 +883,7 @@ test_expect_success 'the same with --ws-error-highlight' '
echo "2. and a new line "
} >x &&
- git -c color.diff=always diff --ws-error-highlight=default,old |
- test_decode_color >current &&
-
- cat >expected <<-\EOF &&
+ cat >expect.default-old <<-\EOF &&
<BOLD>diff --git a/x b/x<RESET>
<BOLD>index d0233a2..700886e 100644<RESET>
<BOLD>--- a/x<RESET>
@@ -897,12 +895,7 @@ test_expect_success 'the same with --ws-error-highlight' '
<GREEN>+<RESET><GREEN>2. and a new line<RESET><BLUE> <RESET>
EOF
- test_cmp expected current &&
-
- git -c color.diff=always diff --ws-error-highlight=all |
- test_decode_color >current &&
-
- cat >expected <<-\EOF &&
+ cat >expect.all <<-\EOF &&
<BOLD>diff --git a/x b/x<RESET>
<BOLD>index d0233a2..700886e 100644<RESET>
<BOLD>--- a/x<RESET>
@@ -914,12 +907,7 @@ test_expect_success 'the same with --ws-error-highlight' '
<GREEN>+<RESET><GREEN>2. and a new line<RESET><BLUE> <RESET>
EOF
- test_cmp expected current &&
-
- git -c color.diff=always diff --ws-error-highlight=none |
- test_decode_color >current &&
-
- cat >expected <<-\EOF &&
+ cat >expect.none <<-\EOF
<BOLD>diff --git a/x b/x<RESET>
<BOLD>index d0233a2..700886e 100644<RESET>
<BOLD>--- a/x<RESET>
@@ -931,7 +919,22 @@ test_expect_success 'the same with --ws-error-highlight' '
<GREEN>+2. and a new line <RESET>
EOF
- test_cmp expected current
+'
+
+test_expect_success 'test --ws-error-highlight option' '
+
+ git -c color.diff=always diff --ws-error-highlight=default,old |
+ test_decode_color >current &&
+ test_cmp expect.default-old current &&
+
+ git -c color.diff=always diff --ws-error-highlight=all |
+ test_decode_color >current &&
+ test_cmp expect.all current &&
+
+ git -c color.diff=always diff --ws-error-highlight=none |
+ test_decode_color >current &&
+ test_cmp expect.none current
+
'
test_done
--
2.10.1-510-g1ef781f2c1
^ permalink raw reply related
* [PATCH 0/4] diff.wsErrorHighlight configuration variable
From: Junio C Hamano @ 2016-10-04 22:54 UTC (permalink / raw)
To: git; +Cc: peff, strk
In-Reply-To: <xmqqk2douhe0.fsf@gitster.mtv.corp.google.com>
"git diff" and its family of commands have "--ws-error-highlight"
option to allow whitespace breakages on old and context lines
painted in color.diff.whitespace color, instead of the usual "we
paint breakages only on new lines", but there wasn't a configuration
variable that corresponds to it.
This would be a lot closer to a series that could be acceptable,
compared to the previous "it should look like this" patch.
Junio C Hamano (4):
t4015: split out the "setup" part of ws-error-highlight test
diff.c: refactor parse_ws_error_highlight()
diff.c: move ws-error-highlight parsing helpers up
diff: introduce diff.wsErrorHighlight option
Documentation/diff-config.txt | 6 +++
Documentation/diff-options.txt | 2 +
diff.c | 88 ++++++++++++++++++++++++++----------------
t/t4015-diff-whitespace.sh | 74 ++++++++++++++++++++++++++---------
4 files changed, 118 insertions(+), 52 deletions(-)
--
2.10.1-510-g1ef781f2c1
^ permalink raw reply
* Re: [musl] Re: Regression: git no longer works with musl libc's regex impl
From: Junio C Hamano @ 2016-10-04 22:48 UTC (permalink / raw)
To: Rich Felker; +Cc: James B, musl, Johannes Schindelin, Jeff King, git
In-Reply-To: <20161004223322.GE19318@brightrain.aerifal.cx>
Rich Felker <dalias@libc.org> writes:
> This is especially unfriendly when the semantics of the switch come
> across, at least to some users, as "your system regex is incomplete"
> rather than "git can't use it because git depends on nonstandard
> extensions".
The latter is exactly what Makefile patch that brought this change
says, I think.
# Define NO_REGEX if your C library lacks regex support with REG_STARTEND
# feature.
Before the series updated the message to the above, we used to say:
# Define NO_REGEX if you have no or inferior regex support in your C library.
which _was_ unfair to those who needed to set NO_REGEX for whatever
reason. It was totally unclear "inferior" relative to what standard
the message was passing its harsh judgement on your C library.
^ permalink raw reply
* Re: What's cooking in git.git (Sep 2016, #08; Tue, 27)
From: Stefan Beller @ 2016-10-04 22:36 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git@vger.kernel.org
In-Reply-To: <xmqqh98sw0tv.fsf@gitster.mtv.corp.google.com>
On Tue, Oct 4, 2016 at 9:11 AM, Junio C Hamano <gitster@pobox.com> wrote:
>
> The question git_all_attrs() asks is quite different.
And as far as I can tell it is only used by builtin/check-attr.c which we do not
intend to convert into a threaded beast in the foreseeable future I'd expect.
So maybe we can just punt on redefining the API there, i.e.
we keep the question and answer together in a struct git_attr_check.
<some return value here>
git_all_attr(const char *path); /* no other arguments except path*/
That looks great, and I incorporated that into the proposal I sent out
as an RFC.
^ permalink raw reply
* Re: [musl] Re: Regression: git no longer works with musl libc's regex impl
From: Rich Felker @ 2016-10-04 22:33 UTC (permalink / raw)
To: James B; +Cc: musl, Johannes Schindelin, Jeff King, git
In-Reply-To: <20161005090625.683fdbbfac8164125dee6469@gmail.com>
On Wed, Oct 05, 2016 at 09:06:25AM +1100, James B wrote:
> On Tue, 4 Oct 2016 18:08:33 +0200 (CEST)
> Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
>
> >
> > No, it is not. You quote POSIX, but the matter of the fact is that we use
> > a subset of POSIX in order to be able to keep things running on Windows.
> >
> > And quite honestly, there are lots of reasons to keep things running on
> > Windows, and even to favor Windows support over musl support. Over four
> > million reasons: the Git for Windows users.
> >
>
> Wow, I don't know that Windows is a git's first-tier platform now,
> and Linux/POSIX second. Are we talking about the same git that was
> originally written in Linus Torvalds, and is used to manage Linux
> kernel? Are you by any chance employed by Redmond, directly or
> indirectly?
>
> Sorry - can't help it.
I don't think the hostility and sarcasm are really needed here. But
what this does speak to is that users don't like feeling like their
platform is being treated as a second-class target, which is what it
feels like when you have to manually flip a switch to make git build.
This is especially unfriendly when the semantics of the switch come
across, at least to some users, as "your system regex is incomplete"
rather than "git can't use it because git depends on nonstandard
extensions".
Rich
^ permalink raw reply
* [RFC/PATCH] attr: Document a new possible thread safe API
From: Stefan Beller @ 2016-10-04 22:14 UTC (permalink / raw)
To: gitster; +Cc: git, bmwill, Stefan Beller
This is what we want to see at the end of the refactoring session
to enable the attr subsystem to be thread safe.
Signed-off-by: Stefan Beller <sbeller@google.com>
---
Junio wrote:
>> So how would we go about git_all_attrs then?
>
> I think you arrived the same conclusion, but ...
I think the changes as proposed are minimal to be able to use attrs in a
multithreaded environment.
This patch applies on top of jc/attr-more.
A complete diff between origin/master..HEAD for the documentation is
appended below.
Thanks,
Stefan
Documentation/technical/api-gitattributes.txt | 22 ++++++++++++++--------
1 file changed, 14 insertions(+), 8 deletions(-)
diff --git a/Documentation/technical/api-gitattributes.txt b/Documentation/technical/api-gitattributes.txt
index 92fc32a..940617e 100644
--- a/Documentation/technical/api-gitattributes.txt
+++ b/Documentation/technical/api-gitattributes.txt
@@ -59,7 +59,10 @@ Querying Specific Attributes
empty `struct git_attr_check` can be prepared by calling
`git_attr_check_alloc()` function and then attributes you want to
ask about can be added to it with `git_attr_check_append()`
- function.
+ function. git_attr_check_initl is thread safe, i.e. you can call it
+ from different threads at the same time; internally however only one
+ call at a time is processed. If the calls from different threads have
+ the same arguments, the returned `git_attr_check` may be the same.
* Call `git_check_attr()` to check the attributes for the path.
@@ -89,15 +92,21 @@ static void setup_check(void)
------------
const char *path;
+ struct git_attr_check *result;
setup_check();
- git_check_attr(path, check);
+ result = git_check_attr(path, check);
------------
-. Act on `.value` member of the result, left in `check->check[]`:
+The result may be the same as `check` (in a single threaded application),
+but generally assume it is not. The `result` must not be free'd as it is
+owned by the attr subsystem. It is reused by the same thread, so a subsequent
+call to git_check_attr in the same thread will overwrite the result.
+
+. Act on `.value` member of the `result->check[]`:
------------
- const char *value = check->check[0].value;
+ const char *value = result->check[0].value;
if (ATTR_TRUE(value)) {
The attribute is Set, by listing only the name of the
@@ -138,10 +147,7 @@ Querying All Attributes
To get the values of all attributes associated with a file:
-* Prepare an empty `git_attr_check` structure by calling
- `git_attr_check_alloc()`.
-
-* Call `git_all_attrs()`, which populates the `git_attr_check`
+* Call `git_all_attrs()`, which returns a `git_attr_check`
with the attributes attached to the path.
* Iterate over the `git_attr_check.check[]` array to examine
--
2.10.0.129.g35f6318
The following is `git diff origin/master Documentation/technical/api-gitattributes.txt`:
diff --git a/Documentation/technical/api-gitattributes.txt b/Documentation/technical/api-gitattributes.txt
index 2602668..940617e 100644
--- a/Documentation/technical/api-gitattributes.txt
+++ b/Documentation/technical/api-gitattributes.txt
@@ -16,10 +16,15 @@ Data Structure
of no interest to the calling programs. The name of the
attribute can be retrieved by calling `git_attr_name()`.
+`struct git_attr_check_elem`::
+
+ This structure represents one attribute and its value.
+
`struct git_attr_check`::
- This structure represents a set of attributes to check in a call
- to `git_check_attr()` function, and receives the results.
+ This structure represents a collection of `git_attr_check_elem`.
+ It is passed to `git_check_attr()` function, specifying the
+ attributes to check, and receives their values.
Attribute Values
@@ -48,49 +53,60 @@ value of the attribute for the path.
Querying Specific Attributes
----------------------------
-* Prepare an array of `struct git_attr_check` to define the list of
- attributes you would want to check. To populate this array, you would
- need to define necessary attributes by calling `git_attr()` function.
+* Prepare `struct git_attr_check` using git_attr_check_initl()
+ function, enumerating the names of attributes whose values you are
+ interested in, terminated with a NULL pointer. Alternatively, an
+ empty `struct git_attr_check` can be prepared by calling
+ `git_attr_check_alloc()` function and then attributes you want to
+ ask about can be added to it with `git_attr_check_append()`
+ function. git_attr_check_initl is thread safe, i.e. you can call it
+ from different threads at the same time; internally however only one
+ call at a time is processed. If the calls from different threads have
+ the same arguments, the returned `git_attr_check` may be the same.
* Call `git_check_attr()` to check the attributes for the path.
-* Inspect `git_attr_check` structure to see how each of the attribute in
- the array is defined for the path.
+* Inspect `git_attr_check` structure to see how each of the
+ attribute in the array is defined for the path.
Example
-------
-To see how attributes "crlf" and "indent" are set for different paths.
+To see how attributes "crlf" and "ident" are set for different paths.
-. Prepare an array of `struct git_attr_check` with two elements (because
- we are checking two attributes). Initialize their `attr` member with
- pointers to `struct git_attr` obtained by calling `git_attr()`:
+. Prepare a `struct git_attr_check` with two elements (because
+ we are checking two attributes):
------------
-static struct git_attr_check check[2];
+static struct git_attr_check *check;
static void setup_check(void)
{
- if (check[0].attr)
+ if (check)
return; /* already done */
- check[0].attr = git_attr("crlf");
- check[1].attr = git_attr("ident");
+ check = git_attr_check_initl("crlf", "ident", NULL);
}
------------
-. Call `git_check_attr()` with the prepared array of `struct git_attr_check`:
+. Call `git_check_attr()` with the prepared `struct git_attr_check`:
------------
const char *path;
+ struct git_attr_check *result;
setup_check();
- git_check_attr(path, ARRAY_SIZE(check), check);
+ result = git_check_attr(path, check);
------------
-. Act on `.value` member of the result, left in `check[]`:
+The result may be the same as `check` (in a single threaded application),
+but generally assume it is not. The `result` must not be free'd as it is
+owned by the attr subsystem. It is reused by the same thread, so a subsequent
+call to git_check_attr in the same thread will overwrite the result.
+
+. Act on `.value` member of the `result->check[]`:
------------
- const char *value = check[0].value;
+ const char *value = result->check[0].value;
if (ATTR_TRUE(value)) {
The attribute is Set, by listing only the name of the
@@ -109,20 +125,36 @@ static void setup_check(void)
}
------------
+To see how attributes in argv[] are set for different paths, only
+the first step in the above would be different.
+
+------------
+static struct git_attr_check *check;
+static void setup_check(const char **argv)
+{
+ check = git_attr_check_alloc();
+ while (*argv) {
+ struct git_attr *attr = git_attr(*argv);
+ git_attr_check_append(check, attr);
+ argv++;
+ }
+}
+------------
+
Querying All Attributes
-----------------------
To get the values of all attributes associated with a file:
-* Call `git_all_attrs()`, which returns an array of `git_attr_check`
- structures.
+* Call `git_all_attrs()`, which returns a `git_attr_check`
+ with the attributes attached to the path.
-* Iterate over the `git_attr_check` array to examine the attribute
- names and values. The name of the attribute described by a
- `git_attr_check` object can be retrieved via
- `git_attr_name(check[i].attr)`. (Please note that no items will be
- returned for unset attributes, so `ATTR_UNSET()` will return false
- for all returned `git_array_check` objects.)
+* Iterate over the `git_attr_check.check[]` array to examine
+ the attribute names and values. The name of the attribute
+ described by a `git_attr_check.check[]` object can be retrieved via
+ `git_attr_name(check->check[i].attr)`. (Please note that no items
+ will be returned for unset attributes, so `ATTR_UNSET()` will return
+ false for all returned `git_array_check` objects.)
-* Free the `git_array_check` array.
+* Free the `git_array_check` by calling `git_attr_check_free()`.
^ permalink raw reply related
* Re: [musl] Re: Regression: git no longer works with musl libc's regex impl
From: James B @ 2016-10-04 22:06 UTC (permalink / raw)
To: musl; +Cc: Johannes Schindelin, Rich Felker, Jeff King, git
In-Reply-To: <alpine.DEB.2.20.1610041802310.35196@virtualbox>
On Tue, 4 Oct 2016 18:08:33 +0200 (CEST)
Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
>
> No, it is not. You quote POSIX, but the matter of the fact is that we use
> a subset of POSIX in order to be able to keep things running on Windows.
>
> And quite honestly, there are lots of reasons to keep things running on
> Windows, and even to favor Windows support over musl support. Over four
> million reasons: the Git for Windows users.
>
Wow, I don't know that Windows is a git's first-tier platform now, and Linux/POSIX second. Are we talking about the same git that was originally written in Linus Torvalds, and is used to manage Linux kernel? Are you by any chance employed by Redmond, directly or indirectly?
Sorry - can't help it.
^ permalink raw reply
* Re: [PATCH 12/18] alternates: use a separate scratch space
From: Jeff King @ 2016-10-04 21:51 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, René Scharfe
In-Reply-To: <xmqq7f9nss1y.fsf@gitster.mtv.corp.google.com>
On Tue, Oct 04, 2016 at 02:49:45PM -0700, Junio C Hamano wrote:
> >> It is not wrong per-se, but I am a bit surprised to see that the
> >> code keeps FLEX_ARRAY _and_ uses a separate malloc'ed area pointed
> >> at by the scratch pointer.
> >
> > Yeah, there's really no reason "path" could not become a non-flex
> > buffer. I mostly left it there out of inertia. If you have a preference,
> > I'm happy to change it.
>
> My preference, before reaching the end of the series, actually was
> to overallocate just once and point with *scratch into path[] beyond
> the end of the fixed "where is the object directory?" string.
>
> Of course, that would not mesh very well with the plan this series
> had after this step to use strbuf for keeping scratch ;-) And the
> end result looks fine to me.
Heh, yeah, I did not think of that (because I had the strbuf end-game in
mind the whole time). I agree that would be nicer if we were keeping the
raw buffer, if only because one could free the whole thing in one shot
(OTOH, we do not ever free these structs at all :) ).
-Peff
^ permalink raw reply
* Re: [PATCH 06/18] t5613: clarify "too deep" recursion tests
From: Jacob Keller @ 2016-10-04 21:50 UTC (permalink / raw)
To: Jeff King; +Cc: Git mailing list, René Scharfe
In-Reply-To: <20161004214914.kgkot337awszhojs@sigill.intra.peff.net>
On Tue, Oct 4, 2016 at 2:49 PM, Jeff King <peff@peff.net> wrote:
> On Tue, Oct 04, 2016 at 02:43:24PM -0700, Jacob Keller wrote:
>
>> > Hmm. Now I am puzzled, because I _did_ line up them specifically to make
>> > this clear. I put the numbers under the ">" of the arrow. Did I screw up
>> > the spacing somehow so that isn't how they look to you? Or are you just
>> > saying you would prefer them under the "-" of the arrow?
>>
>> I bet they line up in a monospace font and I just happened to be
>> viewing this from GMail which isn't showing it in monospace and so it
>> doesn't line up. Ignore me and carry on
>
> Oh, good. I was wondering if I was going crazy. :)
>
> -Peff
Only one of us is going crazy, but I'm not sure who ;)
-Jake
^ permalink raw reply
* Re: [PATCH 12/18] alternates: use a separate scratch space
From: Junio C Hamano @ 2016-10-04 21:49 UTC (permalink / raw)
To: Jeff King; +Cc: git, René Scharfe
In-Reply-To: <20161004213241.ihzkl7cohliavydg@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> On Tue, Oct 04, 2016 at 02:29:46PM -0700, Junio C Hamano wrote:
>
>> Jeff King <peff@peff.net> writes:
>>
>> > extern struct alternate_object_database {
>> > struct alternate_object_database *next;
>> > +
>> > char *name;
>> > - char base[FLEX_ARRAY]; /* more */
>> > + char *scratch;
>> > +
>> > + char path[FLEX_ARRAY];
>> > } *alt_odb_list;
>>
>> It is not wrong per-se, but I am a bit surprised to see that the
>> code keeps FLEX_ARRAY _and_ uses a separate malloc'ed area pointed
>> at by the scratch pointer.
>
> Yeah, there's really no reason "path" could not become a non-flex
> buffer. I mostly left it there out of inertia. If you have a preference,
> I'm happy to change it.
My preference, before reaching the end of the series, actually was
to overallocate just once and point with *scratch into path[] beyond
the end of the fixed "where is the object directory?" string.
Of course, that would not mesh very well with the plan this series
had after this step to use strbuf for keeping scratch ;-) And the
end result looks fine to me.
Thanks.
^ permalink raw reply
* Re: [PATCH 13/18] fill_sha1_file: write "boring" characters
From: Jacob Keller @ 2016-10-04 21:49 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jeff King, Git mailing list, René Scharfe
In-Reply-To: <xmqqbmyzss6z.fsf@gitster.mtv.corp.google.com>
On Tue, Oct 4, 2016 at 2:46 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Jacob Keller <jacob.keller@gmail.com> writes:
>
>> On Mon, Oct 3, 2016 at 1:35 PM, Jeff King <peff@peff.net> wrote:
>>> This function forms a sha1 as "xx/yyyy...", but skips over
>>> the slot for the slash rather than writing it, leaving it to
>>> the caller to do so. It also does not bother to put in a
>>> trailing NUL, even though every caller would want it (we're
>>> forming a path which by definition is not a directory, so
>>> the only thing to do with it is feed it to a system call).
>>>
>>> Let's make the lives of our callers easier by just writing
>>> out the internal "/" and the NUL.
>>> ...
>>
>> I think this makes a lot more sense than making the callers have to do this.
>
> The cost of fill function having to do the same thing repeatedly is
> negligible, so I am OK with the result, but for fairness, this was
> not "make the callers do this extra thing", but was "the caller can
> prepare these unchanging parts just once, and the fill function that
> is repeatedly run does not have to."
>
Sure, but it's a pretty minor optimization and I think the result is
easier to understand.
Thanks,
Jake
^ permalink raw reply
* Re: [PATCH 06/18] t5613: clarify "too deep" recursion tests
From: Jeff King @ 2016-10-04 21:49 UTC (permalink / raw)
To: Jacob Keller; +Cc: Git mailing list, René Scharfe
In-Reply-To: <CA+P7+xoDz2sOPrDrJhAhrqDQsRR8NVU-8kh6+G=8FJeXEJ1dtg@mail.gmail.com>
On Tue, Oct 04, 2016 at 02:43:24PM -0700, Jacob Keller wrote:
> > Hmm. Now I am puzzled, because I _did_ line up them specifically to make
> > this clear. I put the numbers under the ">" of the arrow. Did I screw up
> > the spacing somehow so that isn't how they look to you? Or are you just
> > saying you would prefer them under the "-" of the arrow?
>
> I bet they line up in a monospace font and I just happened to be
> viewing this from GMail which isn't showing it in monospace and so it
> doesn't line up. Ignore me and carry on
Oh, good. I was wondering if I was going crazy. :)
-Peff
^ permalink raw reply
* Re: [PATCH 13/18] fill_sha1_file: write "boring" characters
From: Jeff King @ 2016-10-04 21:48 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jacob Keller, Git mailing list, René Scharfe
In-Reply-To: <xmqqbmyzss6z.fsf@gitster.mtv.corp.google.com>
On Tue, Oct 04, 2016 at 02:46:44PM -0700, Junio C Hamano wrote:
> Jacob Keller <jacob.keller@gmail.com> writes:
>
> > On Mon, Oct 3, 2016 at 1:35 PM, Jeff King <peff@peff.net> wrote:
> >> This function forms a sha1 as "xx/yyyy...", but skips over
> >> the slot for the slash rather than writing it, leaving it to
> >> the caller to do so. It also does not bother to put in a
> >> trailing NUL, even though every caller would want it (we're
> >> forming a path which by definition is not a directory, so
> >> the only thing to do with it is feed it to a system call).
> >>
> >> Let's make the lives of our callers easier by just writing
> >> out the internal "/" and the NUL.
> >> ...
> >
> > I think this makes a lot more sense than making the callers have to do this.
>
> The cost of fill function having to do the same thing repeatedly is
> negligible, so I am OK with the result, but for fairness, this was
> not "make the callers do this extra thing", but was "the caller can
> prepare these unchanging parts just once, and the fill function that
> is repeatedly run does not have to."
Yeah, perhaps "does not bother" in the commit message is not entirely
fair. But it really does feel like quite a premature optimization to
skip the writing of one "/" in the middle of the string, especially as
it impacts the interface.
-Peff
^ permalink raw reply
* Re: [PATCH 13/18] fill_sha1_file: write "boring" characters
From: Junio C Hamano @ 2016-10-04 21:46 UTC (permalink / raw)
To: Jacob Keller; +Cc: Jeff King, Git mailing list, René Scharfe
In-Reply-To: <CA+P7+xpOxoRBDZGF_CU1Q-SYiQZtMx2vuwQKS0og864awZod5g@mail.gmail.com>
Jacob Keller <jacob.keller@gmail.com> writes:
> On Mon, Oct 3, 2016 at 1:35 PM, Jeff King <peff@peff.net> wrote:
>> This function forms a sha1 as "xx/yyyy...", but skips over
>> the slot for the slash rather than writing it, leaving it to
>> the caller to do so. It also does not bother to put in a
>> trailing NUL, even though every caller would want it (we're
>> forming a path which by definition is not a directory, so
>> the only thing to do with it is feed it to a system call).
>>
>> Let's make the lives of our callers easier by just writing
>> out the internal "/" and the NUL.
>> ...
>
> I think this makes a lot more sense than making the callers have to do this.
The cost of fill function having to do the same thing repeatedly is
negligible, so I am OK with the result, but for fairness, this was
not "make the callers do this extra thing", but was "the caller can
prepare these unchanging parts just once, and the fill function that
is repeatedly run does not have to."
^ permalink raw reply
* Re: [PATCH 06/18] t5613: clarify "too deep" recursion tests
From: Jacob Keller @ 2016-10-04 21:43 UTC (permalink / raw)
To: Jeff King; +Cc: Git mailing list, René Scharfe
In-Reply-To: <20161004205510.6bhisw7ixbgcvvwn@sigill.intra.peff.net>
On Tue, Oct 4, 2016 at 1:55 PM, Jeff King <peff@peff.net> wrote:
> On Tue, Oct 04, 2016 at 01:52:19PM -0700, Jacob Keller wrote:
>
>> >> >> > +# Note: These tests depend on the hard-coded value of 5 as "too deep". We start
>> >> >> > +# the depth at 0 and count links, not repositories, so in a chain like:
>> >> >> > +#
>> >> >> > +# A -> B -> C -> D -> E -> F -> G -> H
>> >> >> > +# 0 1 2 3 4 5 6
>> >> >> > +#
>> [...]
>> > No, we count links, not repositories. So the "A->B" link is "0", "B->C"
>> > is "1", and so on.
>>
>> If you need to re-roll for some other reason I would add some spaces
>> around the numbers so they line up better with the links so that this
>> becomes more clear.
>
> Hmm. Now I am puzzled, because I _did_ line up them specifically to make
> this clear. I put the numbers under the ">" of the arrow. Did I screw up
> the spacing somehow so that isn't how they look to you? Or are you just
> saying you would prefer them under the "-" of the arrow?
>
> -Peff
I bet they line up in a monospace font and I just happened to be
viewing this from GMail which isn't showing it in monospace and so it
doesn't line up. Ignore me and carry on
Thanks,
Jake
^ permalink raw reply
* Re: [PATCH 18/18] alternates: use fspathcmp to detect duplicates
From: Junio C Hamano @ 2016-10-04 21:42 UTC (permalink / raw)
To: Jeff King; +Cc: git, René Scharfe
In-Reply-To: <20161003203626.styj2vwcmgwnpx4v@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> ... but until recently we couldn't use it for comparing against
> other alternates (because their paths were not
> NUL-terminated strings).
;-)
I should have expected this when reading the "let's have
a separate .path field" conversion. Nice job.
^ permalink raw reply
* Re: [PATCH v8 11/11] convert: add filter.<driver>.process option
From: Jakub Narębski @ 2016-10-04 21:34 UTC (permalink / raw)
To: Lars Schneider
Cc: git, Jeff King, Junio C Hamano, Stefan Beller,
Martin-Louis Bright, Torsten Bögershausen, Ramsay Jones
In-Reply-To: <7C680AE2-7F5A-47E4-8E79-6C3F4AEBD39B@gmail.com>
[Some of answers and comments may got invalidated by v9]
W dniu 01.10.2016 o 17:34, Lars Schneider pisze:
>> On 29 Sep 2016, at 01:14, Jakub Narębski <jnareb@gmail.com> wrote:
>>
>> Part third (and last) of the review of v8 11/11.
[...]
>>> +
>>> +test_expect_success PERL 'required process filter should filter data' '
>>> + test_config_global filter.protocol.process "$TEST_DIRECTORY/t0021/rot13-filter.pl clean smudge" &&
>>> + test_config_global filter.protocol.required true &&
>>> + rm -rf repo &&
>>> + mkdir repo &&
>>> + (
>>> + cd repo &&
>>> + git init &&
>>
>> Don't you think that creating a fresh test repository for each
>> separate test is a bit too much? I guess that you want for
>> each and every test to be completely independent, but this setup
>> and teardown is a bit excessive.
>>
>> Other tests in the same file (should we reuse the test, or use
>> new test file) do not use this method.
>
> I see your point. However, I am always annoyed if Git tests are
> entangled because it makes working with them way way harder.
> This test test runs in 4.5s on a slow Travis CI machine. I think
> that is OK considering that we have tests running 3.5min (t3404).
All right, this is good argument... though inconsistency (assuming
that we don't switch to separate test for multi-file filter) could
be argument against.
Though I wonder if test preparation could be extracted into a
common function...
[...]
>>> + cp ../test.o test.r &&
>>> + cp ../test2.o test2.r &&
>>
>> What does this test2.o / test2.r file tests, that test.o / test.r
>> doesn't? The name doesn't tell us.
>
> This just tests multiple files with different content.
All right, so it is here to test multiple files (and that filter
actually process multiple files).
>> Why it is test.r, but test2.r? Why it isn't test1.r?
>
> test.r already existed (created in setup test).
With each test in separate repository we could copy test.r prepared
in 'setup' into test1.r in each of multi-file tests.
[...]
>>> + >test4-empty.r &&
>>
>> You test ordinary file, file in subdirectory, file with filename
>> containing spaces, and an empty file.
>>
>> Other tests of single file `clean`/`smudge` filters use filename
>> that requires mangling; maybe we should use similar file?
>>
>> special="name with '\''sq'\'' and \$x" &&
>> echo some test text >"$special" &&
>
> OK.
This test was required because of %f to pass filename as parameter
coupled with the fact that we use `clean` and `smudge` as shell
script fragment (so e.g. pipes and redirection would work in
one-shot filter definition).
This is not the case with multi-file filter, where filenames are
passed internally, and we don't need to worry about shell quoting
at all.
>> In case of `process` filter, a special filename could look like
>> this:
>>
>> process_special="name=with equals and\nembedded newlines\n" &&
>> echo some test text >"$process_special" &&
>
> I think this test would create trouble on Windows. I'll stick to
> the special characters used in the single shot filter.
This would test... example parser. Well, all right, better not
give problems for Windows.
But... you can create such files in Git Bash:
$ touch "$(echo -n -e "fo=o\nbar\n")"
and though they look strange
$ ls -1 fo*
'fo=o'$'\n''bar'
but work all right
$ echo "strangest" >>"$(echo -n -e "fo=o\nbar\n")"
$ name="$(echo -n -e "fo=o\nbar\n")"
$ cat "$name"
strangest
>> Third, why the filter even writes output size? It is no longer
>> part of `process` filter driver protocol, and it makes test more
>> fragile.
>
> I would prefer to leave that in. I think it is good for the test to
> check that we are transmitting the amount of content that what we
> think we transmit.
Right, we test that we processed full file this way, in the multi
packet test.
>>> + <<-\EOF &&
>>> + START
>>> + wrote filter header
>>> + STOP
>>> + EOF
>>
>> Why is even filter process invoked? If this is not expected, perhaps
>> simply ignore what checking out almost empty branch (one without any
>> files marked for filtering) does.
>>
>> Shouldn't we test_expect_failure no-call?
>
> Because a clean operation could happen. I added a clean operation to
> the expected log in order to make this visible (expected log is stripped
> of clean operations in the same way as the actual log per your suggestion
> above).
If we are testing that if there is no "smudge" capability, then
there were no "smudge" operations, why we don't test just that:
that grepping for "smudge" in long doesn't find anything.
Current version feels convoluted (and would stop working if Git
is improved to not run "clean" in this case for optimization).
>>> +
>>> + check_filter_ignore_clean \
>>> + git checkout master \
>>
>> Does this checks different code path than 'git checkout .'? For
>> example, does this test increase code coverage (e.g. as measured
>> by gcov)? If not, then this test could be safely dropped.
>
> We checked out the "empty-branch" before. That's why we check here
> that the smudge filter runs for all files (smudge filter did not run
> for all files with `git checkout .`).
All right, it runs for more files, but does it cover different
code paths? If not, it only makes test run longer...
>>> + <<-\EOF &&
>>> + START
>>> + wrote filter header
>>> + IN: smudge test.r 57 [OK] -- OUT: 57 . [OK]
>>> + IN: smudge test2.r 14 [OK] -- OUT: 14 . [OK]
>>> + IN: smudge test4-empty.r 0 [OK] -- OUT: 0 [OK]
>>> + IN: smudge testsubdir/test3 - subdir.r 23 [OK] -- OUT: 23 . [OK]
>>
>> Can we assume that Git would pass files to filter in alphabetical
>> order? This assumption might make the test unnecessary fragile.
>
> I have never experienced another behavior. If we see fragility we could
> sort the result...
All right (perhaps comment for future would be good idea, though).
>>>
>>> +test_expect_success PERL 'required process filter should clean only and take precedence' '
>>
>> Trying to describe it better results in overly long description,
>> which probably means that this test should be split into few
>> smaller ones:
>>
>> - `process` filter takes precedence over `clean` and/or `smudge`
>> filters, regardless if it supports relevant ("clean" or "smudge")
>> capability or not
>>
>> - `process` filter that includes only "clean" capability should
>> clean only (be used only for 'clean' operation)
>
> Agreed!
>
>
>> In my opinion all functions should be placed at beginning,
>> or even in separate file (if they are used in more than
>> one test).
>
> OK
>
>
>>> +generate_test_data () {
>>
>> The name is not good, it doesn't describe what kind of data
>> we want to generate.
>
> "generate_random_characters" ok?!
All right.
[...]
>>> + echo "*.file filter=protocol" >.gitattributes &&
>>> + check_filter \
>>> + git add *.file .gitattributes \
>>
>> Should it be shell expansion, or git expansion, that is
>>
>> git add '*.file' .gitattributes
>
> Both have the same output. Would the difference matter?
In one case *.file is expanded by shell, then expansion passed
as parameters to `git add` (perhaps not on MS Windows); in the
other '*.file' is passed as pattern to `git add` and expanded
by Git itself (this might be case for both patterns on Win).
But this doesn't matter here, anyway. I think.
[...]
>>> +
>>> +test_expect_success PERL 'process filter should not restart in case of an error' '
>>
>> Errr... what? This description is not clear. Did you mean
>> that filter should not be restarted if it *signals* an error
>> with file (either before sending anything, or after sending
>> partial contents)?
>
> OK renamed to "process filter should not be restarted if it signals an error"
This is better.
>>> +test_expect_success PERL 'process filter should be able to signal an error for all future files' '
>>
>> Did you mean here that filter can abort processing of
>> all future files?
>
> "process filter signals abort once to abort processing of all future files", better?
"process filter aborting stops processing of all further files", maybe?
>>> +
>>> + cp ../test.o test.r &&
>>> + test_must_fail git add . 2> git_stderr.log &&
>>> + grep "not support long running filter protocol" git_stderr.log
>>
>> Shouldn't this use gettext poison (or rather C locale)?
>> This error message could be translated in the future.
>
> I would prefer to adjust that when we translate it.
All right, good enough.
>>> + $str =~ y/A-Za-z/N-ZA-Mn-za-m/;
>>
>> Why not use tr/// version of this quote-like operation?
>> Or do you follow prior art here?
>
> I am not Perl expert. That worked for me :-)
y/// and tr/// are the same operator. Though y/// is supposedly
more Perl-ish, and I think it is used more in Git tests (or rather
it functions).
[...]
>>> +packet_flush();
>>> +print $debug "wrote filter header\n";
>>
>> Or perhaps "handshake end"?
>
> "init handshake complete", ok?
Better.
>>> + print $debug " $pathname";
>>
>> No " pathname=$pathname" ?
>
> Yes, otherwise it gets too verbose in the tests.
All right. And the lines gets too long.
[...]
Regards,
--
Jakub Narębski
^ permalink raw reply
* Re: [PATCH 12/18] alternates: use a separate scratch space
From: Jeff King @ 2016-10-04 21:32 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, René Scharfe
In-Reply-To: <xmqqk2dnssz9.fsf@gitster.mtv.corp.google.com>
On Tue, Oct 04, 2016 at 02:29:46PM -0700, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
>
> > extern struct alternate_object_database {
> > struct alternate_object_database *next;
> > +
> > char *name;
> > - char base[FLEX_ARRAY]; /* more */
> > + char *scratch;
> > +
> > + char path[FLEX_ARRAY];
> > } *alt_odb_list;
>
> It is not wrong per-se, but I am a bit surprised to see that the
> code keeps FLEX_ARRAY _and_ uses a separate malloc'ed area pointed
> at by the scratch pointer.
Yeah, there's really no reason "path" could not become a non-flex
buffer. I mostly left it there out of inertia. If you have a preference,
I'm happy to change it.
-Peff
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox