* [PATCH 3/5] grep: add submodules as a grep source type
From: Brandon Williams @ 2016-10-27 22:38 UTC (permalink / raw)
To: git; +Cc: Brandon Williams
In-Reply-To: <20161027223834.35312-1-bmwill@google.com>
Add `GREP_SOURCE_SUBMODULE` as a grep_source type and cases for this new
type in the various switch statements in grep.c.
When initializing a grep_source with type `GREP_SOURCE_SUBMODULE` the
identifier can either be NULL (to indicate that the working tree will be
used) or a SHA1 (the REV of the submodule to be grep'd). If the
identifier is a SHA1 then we want to fall through to the
`GREP_SOURCE_SHA1` case to handle the copying of the SHA1.
Signed-off-by: Brandon Williams <bmwill@google.com>
---
grep.c | 16 +++++++++++++++-
grep.h | 1 +
2 files changed, 16 insertions(+), 1 deletion(-)
diff --git a/grep.c b/grep.c
index 1194d35b5..0dbdc1d00 100644
--- a/grep.c
+++ b/grep.c
@@ -1735,12 +1735,23 @@ void grep_source_init(struct grep_source *gs, enum grep_source_type type,
case GREP_SOURCE_FILE:
gs->identifier = xstrdup(identifier);
break;
+ case GREP_SOURCE_SUBMODULE:
+ if (!identifier) {
+ gs->identifier = NULL;
+ break;
+ }
+ /*
+ * FALL THROUGH
+ * If the identifier is non-NULL (in the submodule case) it
+ * will be a SHA1 that needs to be copied.
+ */
case GREP_SOURCE_SHA1:
gs->identifier = xmalloc(20);
hashcpy(gs->identifier, identifier);
break;
case GREP_SOURCE_BUF:
gs->identifier = NULL;
+ break;
}
}
@@ -1760,6 +1771,7 @@ void grep_source_clear_data(struct grep_source *gs)
switch (gs->type) {
case GREP_SOURCE_FILE:
case GREP_SOURCE_SHA1:
+ case GREP_SOURCE_SUBMODULE:
free(gs->buf);
gs->buf = NULL;
gs->size = 0;
@@ -1831,8 +1843,10 @@ static int grep_source_load(struct grep_source *gs)
return grep_source_load_sha1(gs);
case GREP_SOURCE_BUF:
return gs->buf ? 0 : -1;
+ case GREP_SOURCE_SUBMODULE:
+ break;
}
- die("BUG: invalid grep_source type");
+ die("BUG: invalid grep_source type to load");
}
void grep_source_load_driver(struct grep_source *gs)
diff --git a/grep.h b/grep.h
index 5856a23e4..267534ca2 100644
--- a/grep.h
+++ b/grep.h
@@ -161,6 +161,7 @@ struct grep_source {
GREP_SOURCE_SHA1,
GREP_SOURCE_FILE,
GREP_SOURCE_BUF,
+ GREP_SOURCE_SUBMODULE,
} type;
void *identifier;
--
2.10.1.613.g6021889
^ permalink raw reply related
* [PATCH 1/5] submodules: add helper functions to determine presence of submodules
From: Brandon Williams @ 2016-10-27 22:38 UTC (permalink / raw)
To: git; +Cc: Brandon Williams
In-Reply-To: <20161027223834.35312-1-bmwill@google.com>
Add two helper functions to submodules.c.
`is_submodule_initialized()` checks if a submodule has been initialized
at a given path and `is_submodule_checked_out()` check if a submodule
has been checked out at a given path.
Signed-off-by: Brandon Williams <bmwill@google.com>
---
submodule.c | 39 +++++++++++++++++++++++++++++++++++++++
submodule.h | 2 ++
2 files changed, 41 insertions(+)
diff --git a/submodule.c b/submodule.c
index 6f7d883de..029b24440 100644
--- a/submodule.c
+++ b/submodule.c
@@ -198,6 +198,45 @@ void gitmodules_config(void)
}
}
+/*
+ * Determine if a submodule has been initialized at a given 'path'
+ */
+int is_submodule_initialized(const char *path)
+{
+ int ret = 0;
+ const struct submodule *module = NULL;
+
+ module = submodule_from_path(null_sha1, path);
+
+ if (module) {
+ struct strbuf buf = STRBUF_INIT;
+ char *submodule_url = NULL;
+
+ strbuf_addf(&buf, "submodule.%s.url",module->name);
+ ret = !git_config_get_string(buf.buf, &submodule_url);
+
+ free(submodule_url);
+ strbuf_release(&buf);
+ }
+
+ return ret;
+}
+
+/*
+ * Determine if a submodule has been checked out at a given 'path'
+ */
+int is_submodule_checked_out(const char *path)
+{
+ int ret = 0;
+ struct strbuf buf = STRBUF_INIT;
+
+ strbuf_addf(&buf, "%s/.git", path);
+ ret = file_exists(buf.buf);
+
+ strbuf_release(&buf);
+ return ret;
+}
+
int parse_submodule_update_strategy(const char *value,
struct submodule_update_strategy *dst)
{
diff --git a/submodule.h b/submodule.h
index d9e197a94..bd039ca98 100644
--- a/submodule.h
+++ b/submodule.h
@@ -37,6 +37,8 @@ void set_diffopt_flags_from_submodule_config(struct diff_options *diffopt,
const char *path);
int submodule_config(const char *var, const char *value, void *cb);
void gitmodules_config(void);
+extern int is_submodule_initialized(const char *path);
+extern int is_submodule_checked_out(const char *path);
int parse_submodule_update_strategy(const char *value,
struct submodule_update_strategy *dst);
const char *submodule_strategy_to_string(const struct submodule_update_strategy *s);
--
2.10.1.613.g6021889
^ permalink raw reply related
* [RFC PATCH 0/5] recursively grep across submodules
From: Brandon Williams @ 2016-10-27 22:38 UTC (permalink / raw)
To: git; +Cc: Brandon Williams
This patch series adds some basic api functions to the submodule interface as
well as teaching grep to recursively search in submodules.
The additions to the submodule interface allow grep to verify that a submodule
has been initialized and checked out prior to launching a child process. One
issue that still needs to be worked out is when greppig history, you could be
in a state where the submodule doesn't have a working tree (or the path you had
in the past doesn't match what currently exists) so instead of changing
directory into the submdoule you need to look for the .git directory for the
submodule in the parents .git/modules directory. If it exists we would need to
change directory to .git/modules/<submodule> and then run the child process
from there. This currently doesn't work due to commit <10f5c52656> since the
GIT_DIR env variable is explicitly set to be '.git'. I'm going to spend some
more time thinking about this problem and will address it as an additional patch in
the series at a later time.
As for the rest of the series, it should be ready for review or comments.
Brandon Williams (5):
submodules: add helper functions to determine presence of submodules
submodules: load gitmodules file from commit sha1
grep: add submodules as a grep source type
grep: optionally recurse into submodules
grep: enable recurse-submodules to work on <tree> objects
Documentation/git-grep.txt | 14 ++
builtin/grep.c | 364 ++++++++++++++++++++++++++++++++++---
cache.h | 2 +
config.c | 8 +-
git.c | 2 +-
grep.c | 16 +-
grep.h | 1 +
submodule-config.c | 6 +-
submodule-config.h | 3 +
submodule.c | 51 ++++++
submodule.h | 3 +
t/t7814-grep-recurse-submodules.sh | 141 ++++++++++++++
tree-walk.c | 17 +-
13 files changed, 588 insertions(+), 40 deletions(-)
create mode 100755 t/t7814-grep-recurse-submodules.sh
--
2.10.1.613.g6021889
^ permalink raw reply
* Re: [PATCH v3 2/3] sha1_file: open window into packfiles with O_CLOEXEC
From: Linus Torvalds @ 2016-10-27 22:38 UTC (permalink / raw)
To: Jeff King
Cc: Junio C Hamano, Git Mailing List, Lars Schneider, Eric Wong,
Johannes Schindelin
In-Reply-To: <20161027102419.dbzigj7wtr355ofh@sigill.intra.peff.net>
On Thu, Oct 27, 2016 at 3:24 AM, Jeff King <peff@peff.net> wrote:
>
> +cc Linus as the original author of 144bde78e9 in case there is
> something subtle I'm missing, but this really just seems like it's
> an outdated optimization.
I'd *really* like to keep O_NOATIME if at all possible. It made a huge
difference on older kernels, and I'm not convinced that relatime
really fixes it as well as O_NOATIME.
There are people who don't like relatime. And even if you do have
relatime enabled, it will update atime once every day, so then this
makes your filesystem have a storm of nasty inode writebacks if you
haven't touched that git repo in a while.
If this is purely about mixing things up with O_CLOEXEC, then that is
*trivially* fixable by just using
fcntl(fd, F_SETFD, FD_CLOEXEC);
after the open.
Which is what you have to do anyway if you want to be portable, so
its' not like you could avoid that complexity in the first place.
Note that you can *not* do the same thing with O_NOATIME, since the
whole point of O_NOATIME is that it changes the behavior of the open
itself (unlike O_CLOEXEC which changes _later_ behavior, and can
always be replaced by FD_CLOEXEC fnclt modulo races that are
immaterial for git)
Linus
^ permalink raw reply
* Re: feature request
From: Stefan Beller @ 2016-10-27 22:30 UTC (permalink / raw)
To: John Rood; +Cc: git@vger.kernel.org
In-Reply-To: <CALj-rGeoT_mpmuw8Put=6eRhzf-r2WUohu_Kd-wnpc=BvO5joA@mail.gmail.com>
On Thu, Oct 27, 2016 at 2:55 PM, John Rood <mr.john.rood@gmail.com> wrote:
> Users should be able to configure Git to not send them into a Vim editor.
>
> When users pull commits, and a new commit needs to be created for a
> merge, Git's current way of determining a commit message is to send
> the user into a Vim window so that they can write a message. There are
> 2 reasons why this might not be the ideal way to prompt for a commit
> message.
>
> 1. Many users are used to writing concise one-line commit messages and
> would not expect to save a commit message in a multi-line file. Some
> users will wonder why they are in a text editor or which file they are
> editing. Others may not, in fact, realize at all that a text editor is
> what they are in.
Look at the -m option of git commit,
git commit -a -m "look a commit with no editor, and a precise one line message"
I do not advocate this use though, as I think commit messages should be
more wordy.
>
> 2. Many users are not familiar with Vim, and do not understand how to
> modify, save, and exit. It is not very considerate to require a user
> to learn Vim in order to finish a commit that they are in the middle
> of.
That is true, but vi is like the most available editor as a relict
from ancient times;
as you are on Windows, maybe notepad is the best on that platform.
Maybe file a bug/issue at https://github.com/git-for-windows to change
the default?
>
> The existing behavior should be optional, and there should be two new options:
>
> 1. Use a simple inline prompt for a commit message (in the same way
> Git might prompt for a username).
>
> 2. Automatically assign names for commits in the form of "Merged x into y".
^ permalink raw reply
* Re: Expanding Includes in .gitignore
From: Aaron Pelly @ 2016-10-27 22:30 UTC (permalink / raw)
To: Jeff King; +Cc: git
In-Reply-To: <20161027210753.btc7zbndhdocsbwa@sigill.intra.peff.net>
On 28/10/16 10:07, Jeff King wrote:
> I think it does
> introduce compatibility complications. People with older git will not
> respect your .gitignore.d files. Whereas $GIT_DIR/info is purely a local
> matter.
I know. I don't think it's a serious compatibility issue, but more
important people may disagree, in which case I'll have to review my
stance. I think the pros of it working everywhere outweigh the cons.
I will say that I am a proponent of consistency and obviousness. I would
rather this worked the same everywhere or it was a completely novel
component in $GIT_DIR.
I don't think I like the idea much, but it would be possible to
magically generate an ignore file from a directory.
> But perhaps there is a use case I'm missing.
A new repo where the dev tools haven't been standardised yet. New tool,
new file. Simple, and somewhat self documenting as a table of contents
in .gitignore.d
^ permalink raw reply
* Re: feature request
From: Junio C Hamano @ 2016-10-27 22:27 UTC (permalink / raw)
To: John Rood; +Cc: Stefan Beller, git@vger.kernel.org
In-Reply-To: <CALj-rGfsy+KA=dsd+wobMiVOWHK1GU=dZRcQWtMoXwtBrMYPLg@mail.gmail.com>
John Rood <mr.john.rood@gmail.com> writes:
> I suppose I can do git config --global core.editor notepad
> However, this really only addresses my second concern.
>
> My first concern is that using a text editor at all seems like
> overkill in many scenarios.
Nobody stops you from writing a "type whatever you want; I won't let
you edit any mistakes as I am not even a text editor; just hit
RETURN when you are done, as you can only write a single line"
program and set it as your GIT_EDITOR.
I do not know what would happen when you need "git commit --amend",
though.
^ permalink raw reply
* Re: feature request
From: John Rood @ 2016-10-27 22:24 UTC (permalink / raw)
To: Stefan Beller; +Cc: git@vger.kernel.org
In-Reply-To: <CALj-rGeMHyN6Xhc0_KCChmWL4BDyF-ZJYdipbe1_yNvznrYPVA@mail.gmail.com>
I suppose I can do git config --global core.editor notepad
However, this really only addresses my second concern.
My first concern is that using a text editor at all seems like
overkill in many scenarios.
For that reason, I still think the other two options I mentioned would
be beneficial.
On Thu, Oct 27, 2016 at 5:05 PM, John Rood <mr.john.rood@gmail.com> wrote:
> Unfortunately, in my case I'm on windows (my company's choice, not mine).
>
> On Thu, Oct 27, 2016 at 5:01 PM, Stefan Beller <sbeller@google.com> wrote:
>> On Thu, Oct 27, 2016 at 2:55 PM, John Rood <mr.john.rood@gmail.com> wrote:
>>> Users should be able to configure Git to not send them into a Vim editor.
>>
>> See https://git-scm.com/docs/git-var
>>
>> GIT_EDITOR
>>
>> Text editor for use by Git commands. The value is meant to be interpreted
>> by the shell when it is used. Examples: ~/bin/vi, $SOME_ENVIRONMENT_VARIABLE,
>> "C:\Program Files\Vim\gvim.exe" --nofork. The order of preference is the
>> $GIT_EDITOR environment variable, then core.editor configuration, then
>> $VISUAL, then $EDITOR, and then the default chosen at compile time,
>> which is usually vi.
>>
>>
>> So maybe
>>
>> git config --global core.editor "nano"
>>
>> helps in your case?
^ permalink raw reply
* [PATCH] attr: convert to new threadsafe API
From: Stefan Beller @ 2016-10-27 22:15 UTC (permalink / raw)
To: gitster
Cc: git, bmwill, pclouds, Johannes.Schindelin, j6t, peff, simon,
Stefan Beller
In-Reply-To: <xmqqy41a8hxj.fsf@gitster.mtv.corp.google.com>
This revamps the API of the attr subsystem to be thread safe.
Before we had the question and its results in one struct type.
The typical usage of the API was
static struct git_attr_check *check;
if (!check)
check = git_attr_check_initl("text", NULL);
git_check_attr(path, check);
act_on(check->value[0]);
This has a couple of issues when it comes to thread safety:
* the initialization is racy in this implementation. To make it
thread safe, we need to acquire a mutex, such that only one
thread is executing the code in git_attr_check_initl.
As we do not want to introduce a mutex at each call site,
this is best done in the attr code. However to do so, we need
to have access to the `check` variable, i.e. the API has to
look like
git_attr_check_initl(struct git_attr_check**, ...);
Then one of the threads calling git_attr_check_initl will
acquire the mutex and init the `check`, while all other threads
will wait on the mutex just to realize they're late to the
party and they'll return with no work done.
* While the check for attributes to be questioned only need to
be initalized once as that part will be read only after its
initialisation, the answer may be different for each path.
Because of that we need to decouple the check and the answer,
such that each thread can obtain an answer for the path it
is currently processing.
This commit changes the API and adds locking in
git_attr_check_initl that provides the thread safety for constructing
`struct git_attr_check`.
The usage of the new API will be:
/*
* The initl call will thread-safely check whether the
* struct git_attr_check has been initialized. We only
* want to do the initialization work once, hence we do
* that work inside a thread safe environment.
*/
static struct git_attr_check *check;
git_attr_check_initl(&check, "text", NULL);
/* We're just asking for one attribute "text". */
git_attr_result myresult[1];
/* Perform the check and act on it: */
git_check_attr(path, check, myresult);
act_on(myresult[0].value);
/*
* No need to free the check as it is static, hence doesn't leak
* memory. The result is also static, so no need to free there either.
*/
Signed-off-by: Stefan Beller <sbeller@google.com>
---
* use attr_start on Windows to dynamically initialize the Single Big Attr Mutex
* rewrote the documentation by Junios guiding comments
* interned struct git_attr, i.e. you hand "const char *" to the attr API, and
internally it will make sense of it. When asking git_all_attrs, however you
still need to know about git_attrs as you need to find out the name of the
attribute via `git_attr_name(struct git_attr *)`.
If there are no other huge design discussions, I'll reroll the whole series.
Thanks,
Stefan
Documentation/technical/api-gitattributes.txt | 104 +++++++++++-------
archive.c | 11 +-
attr.c | 150 ++++++++++++++++++--------
attr.h | 76 +++++++------
builtin/check-attr.c | 47 ++++----
builtin/pack-objects.c | 16 +--
compat/mingw.c | 3 +
convert.c | 40 +++----
ll-merge.c | 24 +++--
userdiff.c | 16 +--
ws.c | 8 +-
11 files changed, 305 insertions(+), 190 deletions(-)
diff --git a/Documentation/technical/api-gitattributes.txt b/Documentation/technical/api-gitattributes.txt
index 92fc32a..ba04c30 100644
--- a/Documentation/technical/api-gitattributes.txt
+++ b/Documentation/technical/api-gitattributes.txt
@@ -16,15 +16,19 @@ 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 collection of `git_attr_check_elem`.
+ This structure represents a collection of `struct git_attrs`.
It is passed to `git_check_attr()` function, specifying the
- attributes to check, and receives their values.
+ attributes to check, and receives their values into a corresponding
+ `struct git_attr_result`.
+
+`struct git_attr_result`::
+
+ This structure represents one results for a check, such that an
+ array of `struct git_attr_results` corresponds to a
+ `struct git_attr_check`. The answers given in that array are in
+ the the same order as the check struct.
Attribute Values
@@ -32,7 +36,7 @@ Attribute Values
An attribute for a path can be in one of four states: Set, Unset,
Unspecified or set to a string, and `.value` member of `struct
-git_attr_check` records it. There are three macros to check these:
+git_attr_result` records it. There are three macros to check these:
`ATTR_TRUE()`::
@@ -53,19 +57,32 @@ value of the attribute for the path.
Querying Specific Attributes
----------------------------
-* Prepare `struct git_attr_check` using git_attr_check_initl()
+* Prepare a `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.
-
-* 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.
-
+ empty `struct git_attr_check` as allocated by git_attr_check_alloc()
+ 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.
+ Both ways with `git_attr_check_initl()` as well as the
+ alloc and append route are thread safe, i.e. you can call it
+ from different threads at the same time; when check determines
+ the initialization is still needed, the threads will use a
+ single global mutex to perform the initialization just once, the
+ others will wait on the the thread to actually perform the
+ initialization.
+
+* Allocate an array of `struct git_attr_result` either on the stack
+ or via `git_attr_result_alloc` on the heap when the result size
+ is not known at compile time.
+
+* Call `git_check_attr()` to check the attributes for the path,
+ the given `git_attr_result` will be filled with the result.
+ You must not change the `struct git_attr_check` after calling
+ `git_check_attr()`.
+
+* Inspect each `git_attr_result` structure to see how
+ each of the attribute in the array is defined for the path.
Example
-------
@@ -76,28 +93,23 @@ To see how attributes "crlf" and "ident" are set for different paths.
we are checking two attributes):
------------
-static struct git_attr_check *check;
-static void setup_check(void)
-{
- if (check)
- return; /* already done */
- check = git_attr_check_initl("crlf", "ident", NULL);
-}
+ static struct git_attr_check *check;
+ git_attr_check_initl(&check, "crlf", "ident", NULL);
------------
. Call `git_check_attr()` with the prepared `struct git_attr_check`:
------------
const char *path;
+ struct git_attr_result result[2];
- setup_check();
- git_check_attr(path, check);
+ git_check_attr(path, check, result);
------------
-. Act on `.value` member of the result, left in `check->check[]`:
+. Act on `result.value[]`:
------------
- const char *value = check->check[0].value;
+ const char *value = result.value[0];
if (ATTR_TRUE(value)) {
The attribute is Set, by listing only the name of the
@@ -123,12 +135,15 @@ the first step in the above would be different.
static struct git_attr_check *check;
static void setup_check(const char **argv)
{
+ if (check)
+ return; /* already done */
check = git_attr_check_alloc();
while (*argv) {
struct git_attr *attr = git_attr(*argv);
git_attr_check_append(check, attr);
argv++;
}
+ struct git_attr_result *result = git_attr_result_alloc(check);
}
------------
@@ -138,17 +153,30 @@ 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()`.
+* Setup a local variables for the question
+ `struct git_attr_check` as well as a pointer where the result
+ `struct git_attr_result` will be stored. Both should be initialized
+ to NULL.
+
+------------
+ struct git_attr_check *check = NULL;
+ struct git_attr_result *result = NULL;
+------------
+
+* Call `git_all_attrs()`.
-* Call `git_all_attrs()`, which populates the `git_attr_check`
- with the attributes attached to the path.
+------------
+ git_all_attrs(full_path, &check, &result);
+------------
-* 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
+* Iterate over the `git_attr_check.attr[]` array to examine the
+ attribute names. The name of the attribute described by a
+ `git_attr_check.attr[]` object can be retrieved via
+ `git_attr_name(check->attr[i])`. (Please note that no items
will be returned for unset attributes, so `ATTR_UNSET()` will return
false for all returned `git_array_check` objects.)
+ The respective value for an attribute can be found in the same
+ index position in of `git_attr_result`.
-* Free the `git_array_check` by calling `git_attr_check_free()`.
+* Clear the variables by calling `git_attr_check_clear()` and
+ `git_attr_result_free()`.
diff --git a/archive.c b/archive.c
index 11e3951..65bc6b7 100644
--- a/archive.c
+++ b/archive.c
@@ -111,6 +111,7 @@ static int write_archive_entry(const unsigned char *sha1, const char *base,
struct archiver_args *args = c->args;
write_archive_entry_fn_t write_entry = c->write_entry;
static struct git_attr_check *check;
+ struct git_attr_result result[2];
const char *path_without_prefix;
int err;
@@ -124,12 +125,12 @@ static int write_archive_entry(const unsigned char *sha1, const char *base,
strbuf_addch(&path, '/');
path_without_prefix = path.buf + args->baselen;
- if (!check)
- check = git_attr_check_initl("export-ignore", "export-subst", NULL);
- if (!git_check_attr(path_without_prefix, check)) {
- if (ATTR_TRUE(check->check[0].value))
+ git_attr_check_initl(&check, "export-ignore", "export-subst", NULL);
+
+ if (!git_check_attr(path_without_prefix, check, result)) {
+ if (ATTR_TRUE(result[0].value))
return 0;
- args->convert = ATTR_TRUE(check->check[1].value);
+ args->convert = ATTR_TRUE(result[1].value);
}
if (S_ISDIR(mode) || S_ISGITLINK(mode)) {
diff --git a/attr.c b/attr.c
index 881bdfa..ea3d742 100644
--- a/attr.c
+++ b/attr.c
@@ -14,6 +14,7 @@
#include "dir.h"
#include "utf8.h"
#include "quote.h"
+#include "thread-utils.h"
const char git_attr__true[] = "(builtin)true";
const char git_attr__false[] = "\0(builtin)false";
@@ -46,6 +47,21 @@ struct git_attr {
static int attr_nr;
static struct git_attr *(git_attr_hash[HASHSIZE]);
+#ifndef NO_PTHREADS
+
+static pthread_mutex_t attr_mutex = PTHREAD_MUTEX_INITIALIZER;
+#define attr_lock() pthread_mutex_lock(&attr_mutex)
+#define attr_unlock() pthread_mutex_unlock(&attr_mutex)
+void attr_start(void) { pthread_mutex_init(&attr_mutex, NULL); }
+
+#else
+
+#define attr_lock() (void)0
+#define attr_unlock() (void)0
+void attr_start(void) {;}
+
+#endif /* NO_PTHREADS */
+
/*
* NEEDSWORK: maybe-real, maybe-macro are not property of
* an attribute, as it depends on what .gitattributes are
@@ -55,6 +71,16 @@ static struct git_attr *(git_attr_hash[HASHSIZE]);
*/
static int cannot_trust_maybe_real;
+/*
+ * Send one or more git_attr_check to git_check_attrs(), and
+ * each 'value' member tells what its value is.
+ * Unset one is returned as NULL.
+ */
+struct git_attr_check_elem {
+ const struct git_attr *attr;
+ const char *value;
+};
+
/* NEEDSWORK: This will become per git_attr_check */
static struct git_attr_check_elem *check_all_attr;
@@ -781,7 +807,7 @@ static int macroexpand_one(int nr, int rem)
static int attr_check_is_dynamic(const struct git_attr_check *check)
{
- return (void *)(check->check) != (void *)(check + 1);
+ return (void *)(check->attr) != (void *)(check + 1);
}
static void empty_attr_check_elems(struct git_attr_check *check)
@@ -799,7 +825,9 @@ static void empty_attr_check_elems(struct git_attr_check *check)
* any and all attributes that are visible are collected in it.
*/
static void collect_some_attrs(const char *path, int pathlen,
- struct git_attr_check *check, int collect_all)
+ struct git_attr_check *check,
+ struct git_attr_result **result,
+ int collect_all)
{
struct attr_stack *stk;
@@ -825,13 +853,11 @@ static void collect_some_attrs(const char *path, int pathlen,
check_all_attr[i].value = ATTR__UNKNOWN;
if (!collect_all && !cannot_trust_maybe_real) {
- struct git_attr_check_elem *celem = check->check;
-
rem = 0;
for (i = 0; i < check->check_nr; i++) {
- if (!celem[i].attr->maybe_real) {
+ if (!check->attr[i]->maybe_real) {
struct git_attr_check_elem *c;
- c = check_all_attr + celem[i].attr->attr_nr;
+ c = check_all_attr + check->attr[i]->attr_nr;
c->value = ATTR__UNSET;
rem++;
}
@@ -845,38 +871,53 @@ static void collect_some_attrs(const char *path, int pathlen,
rem = fill(path, pathlen, basename_offset, stk, rem);
if (collect_all) {
- empty_attr_check_elems(check);
+ int check_nr = 0, check_alloc = 0;
+ const char **res = NULL;
+
for (i = 0; i < attr_nr; i++) {
const struct git_attr *attr = check_all_attr[i].attr;
const char *value = check_all_attr[i].value;
if (value == ATTR__UNSET || value == ATTR__UNKNOWN)
continue;
- git_attr_check_append(check, attr)->value = value;
+
+ ALLOC_GROW(check->attr, check->check_nr + 1, check->check_alloc);
+ check->attr[check->check_nr++] = attr;
+
+ ALLOC_GROW(res, check_nr + 1, check_alloc);
+ res[check_nr++] = value;
}
+
+ *result = git_attr_result_alloc(check);
+ for (i = 0; i < check->check_nr; i++)
+ (*result)[i].value = res[i];
+
+ free(res);
}
}
static int git_check_attrs(const char *path, int pathlen,
- struct git_attr_check *check)
+ struct git_attr_check *check,
+ struct git_attr_result *result)
{
int i;
- struct git_attr_check_elem *celem = check->check;
- collect_some_attrs(path, pathlen, check, 0);
+ collect_some_attrs(path, pathlen, check, &result, 0);
for (i = 0; i < check->check_nr; i++) {
- const char *value = check_all_attr[celem[i].attr->attr_nr].value;
+ const char *value = check_all_attr[check->attr[i]->attr_nr].value;
if (value == ATTR__UNKNOWN)
value = ATTR__UNSET;
- celem[i].value = value;
+ result[i].value = value;
}
return 0;
}
-void git_all_attrs(const char *path, struct git_attr_check *check)
+void git_all_attrs(const char *path,
+ struct git_attr_check *check,
+ struct git_attr_result **result)
{
- collect_some_attrs(path, strlen(path), check, 1);
+ collect_some_attrs(path, strlen(path), check, result, 1);
}
void git_attr_set_direction(enum git_attr_direction new, struct index_state *istate)
@@ -892,36 +933,40 @@ void git_attr_set_direction(enum git_attr_direction new, struct index_state *ist
use_index = istate;
}
-static int git_check_attr_counted(const char *path, int pathlen,
- struct git_attr_check *check)
+int git_check_attr(const char *path,
+ struct git_attr_check *check,
+ struct git_attr_result *result)
{
check->finalized = 1;
- return git_check_attrs(path, pathlen, check);
+ return git_check_attrs(path, strlen(path), check, result);
}
-int git_check_attr(const char *path, struct git_attr_check *check)
+void git_attr_check_initl(struct git_attr_check **check_,
+ const char *one, ...)
{
- return git_check_attr_counted(path, strlen(path), check);
-}
-
-struct git_attr_check *git_attr_check_initl(const char *one, ...)
-{
- struct git_attr_check *check;
int cnt;
va_list params;
const char *param;
+ struct git_attr_check *check;
+
+ attr_lock();
+ if (*check_) {
+ attr_unlock();
+ return;
+ }
va_start(params, one);
for (cnt = 1; (param = va_arg(params, const char *)) != NULL; cnt++)
;
va_end(params);
+
check = xcalloc(1,
- sizeof(*check) + cnt * sizeof(*(check->check)));
+ sizeof(*check) + cnt * sizeof(*(check->attr)));
check->check_nr = cnt;
check->finalized = 1;
- check->check = (struct git_attr_check_elem *)(check + 1);
+ check->attr = (const struct git_attr **)(check + 1);
- check->check[0].attr = git_attr(one);
+ check->attr[0] = git_attr(one);
va_start(params, one);
for (cnt = 1; cnt < check->check_nr; cnt++) {
struct git_attr *attr;
@@ -932,29 +977,48 @@ struct git_attr_check *git_attr_check_initl(const char *one, ...)
attr = git_attr(param);
if (!attr)
die("BUG: %s: not a valid attribute name", param);
- check->check[cnt].attr = attr;
+ check->attr[cnt] = attr;
}
va_end(params);
- return check;
+ *check_ = check;
+ attr_unlock();
}
-struct git_attr_check *git_attr_check_alloc(void)
+void git_attr_check_alloc(struct git_attr_check **check)
{
- return xcalloc(1, sizeof(struct git_attr_check));
+ attr_lock();
+ if (!*check)
+ *check = xcalloc(1, sizeof(struct git_attr_check));
+
+ attr_unlock();
}
-struct git_attr_check_elem *git_attr_check_append(struct git_attr_check *check,
- const struct git_attr *attr)
+struct git_attr_result *git_attr_result_alloc(struct git_attr_check *check)
{
- struct git_attr_check_elem *elem;
+ return xcalloc(1, sizeof(struct git_attr_result) * check->check_nr);
+}
+
+void git_attr_check_append(struct git_attr_check *check,
+ const char *name, int len)
+{
+ int i;
+ struct git_attr *attr;
if (check->finalized)
die("BUG: append after git_attr_check structure is finalized");
if (!attr_check_is_dynamic(check))
die("BUG: appending to a statically initialized git_attr_check");
- ALLOC_GROW(check->check, check->check_nr + 1, check->check_alloc);
- elem = &check->check[check->check_nr++];
- elem->attr = attr;
- return elem;
+ attr_lock();
+
+ attr = git_attr_counted(name, len);
+
+ for (i = 0; i < check->check_nr; i++)
+ if (check->attr[i] == attr)
+ break;
+ if (i == check->check_nr) {
+ ALLOC_GROW(check->attr, check->check_nr + 1, check->check_alloc);
+ check->attr[check->check_nr++] = attr;
+ }
+ attr_unlock();
}
void git_attr_check_clear(struct git_attr_check *check)
@@ -962,12 +1026,12 @@ void git_attr_check_clear(struct git_attr_check *check)
empty_attr_check_elems(check);
if (!attr_check_is_dynamic(check))
die("BUG: clearing a statically initialized git_attr_check");
- free(check->check);
+ free(check->attr);
check->check_alloc = 0;
}
-void git_attr_check_free(struct git_attr_check *check)
+void git_attr_result_free(struct git_attr_result *result)
{
- git_attr_check_clear(check);
- free(check);
+ /* No need to free values as they are interned. */
+ free(result);
}
diff --git a/attr.h b/attr.h
index 292d56f..381eba9 100644
--- a/attr.h
+++ b/attr.h
@@ -1,17 +1,14 @@
#ifndef ATTR_H
#define ATTR_H
-/* An attribute is a pointer to this opaque structure */
-struct git_attr;
-
/*
- * Given a string, return the gitattribute object that
- * corresponds to it.
+ * Must be called on platforms that do not support static initialisation
+ * of mutexes.
*/
-extern struct git_attr *git_attr(const char *);
+extern void attr_start(void);
-/* The same, but with counted string */
-extern struct git_attr *git_attr_counted(const char *, size_t);
+/* An attribute is a pointer to this opaque structure */
+struct git_attr;
/*
* Return the name of the attribute represented by the argument. The
@@ -32,44 +29,53 @@ extern const char git_attr__false[];
#define ATTR_FALSE(v) ((v) == git_attr__false)
#define ATTR_UNSET(v) ((v) == NULL)
-/*
- * Send one or more git_attr_check to git_check_attrs(), and
- * each 'value' member tells what its value is.
- * Unset one is returned as NULL.
- */
-struct git_attr_check_elem {
- const struct git_attr *attr;
- const char *value;
-};
-
struct git_attr_check {
int finalized;
int check_nr;
int check_alloc;
- struct git_attr_check_elem *check;
+ const struct git_attr **attr;
};
+#define GIT_ATTR_CHECK_INIT {0, 0, 0, NULL}
-extern struct git_attr_check *git_attr_check_initl(const char *, ...);
-extern int git_check_attr(const char *path, struct git_attr_check *);
+struct git_attr_result {
+ const char *value;
+};
-extern struct git_attr_check *git_attr_check_alloc(void);
-extern struct git_attr_check_elem *git_attr_check_append(struct git_attr_check *, const struct git_attr *);
+/*
+ * Initialize the `git_attr_check` via one of the following three functions:
+ *
+ * git_attr_check_alloc allocates an empty check,
+ * git_attr_check_append add an attribute to the given git_attr_check
+ *
+ * git_all_attrs allocates a check and fills in all attributes that
+ * are set for the given path.
+ * git_attr_check_initl takes a pointer to where the check will be initialized,
+ * followed by all attributes that are to be checked.
+ * This makes it potentially thread safe as it could
+ * internally have a mutex for that memory location.
+ * Currently it is not thread safe!
+ */
+extern void git_attr_check_alloc(struct git_attr_check **);
+extern struct git_attr_result *git_attr_result_alloc(struct git_attr_check *check);
+
+extern void git_attr_check_append(struct git_attr_check *,
+ const char *, int);
+extern void git_attr_check_initl(struct git_attr_check **,
+ const char *, ...);
+
+extern void git_all_attrs(const char *path,
+ struct git_attr_check *,
+ struct git_attr_result **);
+
+/* Query a path for its attributes */
+extern int git_check_attr(const char *path,
+ struct git_attr_check *,
+ struct git_attr_result *result);
extern void git_attr_check_clear(struct git_attr_check *);
-extern void git_attr_check_free(struct git_attr_check *);
-/*
- * Return the name of the attribute represented by the argument. The
- * return value is a pointer to a null-delimited string that is part
- * of the internal data structure; it should not be modified or freed.
- */
-extern const char *git_attr_name(const struct git_attr *);
+extern void git_attr_result_free(struct git_attr_result *);
-/*
- * Retrieve all attributes that apply to the specified path.
- * check holds the attributes and their values.
- */
-void git_all_attrs(const char *path, struct git_attr_check *check);
enum git_attr_direction {
GIT_ATTR_CHECKIN,
diff --git a/builtin/check-attr.c b/builtin/check-attr.c
index ec61476..6b9e746 100644
--- a/builtin/check-attr.c
+++ b/builtin/check-attr.c
@@ -24,13 +24,14 @@ static const struct option check_attr_options[] = {
OPT_END()
};
-static void output_attr(struct git_attr_check *check, const char *file)
+static void output_attr(struct git_attr_check *check,
+ struct git_attr_result *result, const char *file)
{
int j;
int cnt = check->check_nr;
for (j = 0; j < cnt; j++) {
- const char *value = check->check[j].value;
+ const char *value = result[j].value;
if (ATTR_TRUE(value))
value = "set";
@@ -44,11 +45,11 @@ static void output_attr(struct git_attr_check *check, const char *file)
"%s%c" /* attrname */
"%s%c" /* attrvalue */,
file, 0,
- git_attr_name(check->check[j].attr), 0, value, 0);
+ git_attr_name(check->attr[j]), 0, value, 0);
} else {
quote_c_style(file, NULL, stdout, 0);
printf(": %s: %s\n",
- git_attr_name(check->check[j].attr), value);
+ git_attr_name(check->attr[j]), value);
}
}
}
@@ -59,16 +60,20 @@ static void check_attr(const char *prefix,
{
char *full_path =
prefix_path(prefix, prefix ? strlen(prefix) : 0, file);
+ struct git_attr_check local_check = GIT_ATTR_CHECK_INIT;
+ struct git_attr_result *result = NULL;
+
if (check != NULL) {
- if (git_check_attr(full_path, check))
- die("git_check_attr died");
- output_attr(check, file);
+ result = git_attr_result_alloc(check);
+ git_check_attr(full_path, check, result);
} else {
- check = git_attr_check_alloc();
- git_all_attrs(full_path, check);
- output_attr(check, file);
- git_attr_check_free(check);
+ git_all_attrs(full_path, &local_check, &result);
+ check = &local_check;
}
+ output_attr(check, result, file);
+ git_attr_check_clear(&local_check);
+
+ git_attr_result_free(result);
free(full_path);
}
@@ -102,7 +107,7 @@ static NORETURN void error_with_usage(const char *msg)
int cmd_check_attr(int argc, const char **argv, const char *prefix)
{
- struct git_attr_check *check;
+ struct git_attr_check *check = NULL;
int cnt, i, doubledash, filei;
if (!is_bare_repository())
@@ -162,16 +167,16 @@ int cmd_check_attr(int argc, const char **argv, const char *prefix)
error_with_usage("No file specified");
}
- if (all_attrs) {
- check = NULL;
- } else {
- check = git_attr_check_alloc();
+ if (!all_attrs) {
+ git_attr_check_alloc(&check);
for (i = 0; i < cnt; i++) {
- struct git_attr *a = git_attr(argv[i]);
- if (!a)
- return error("%s: not a valid attribute name",
- argv[i]);
- git_attr_check_append(check, a);
+ if (!attr_name_valid(argv[i], strlen(argv[i]))) {
+ struct strbuf sb = STRBUF_INIT;
+ invalid_attr_name_message(&sb, argv[i],
+ strlen(argv[i]));
+ return error("%s", strbuf_detach(&sb, NULL));
+ }
+ git_attr_check_append(check, argv[i], strlen(argv[i]));
}
}
diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index 3918c07..3751836 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -899,14 +899,16 @@ static void write_pack_file(void)
static int no_try_delta(const char *path)
{
static struct git_attr_check *check;
+ int ret = 0;
+ struct git_attr_result result[1];
- if (!check)
- check = git_attr_check_initl("delta", NULL);
- if (git_check_attr(path, check))
- return 0;
- if (ATTR_FALSE(check->check[0].value))
- return 1;
- return 0;
+ git_attr_check_initl(&check, "delta", NULL);
+
+ if (!git_check_attr(path, check, result)) {
+ if (ATTR_FALSE(result[0].value))
+ ret = 1;
+ }
+ return ret;
}
/*
diff --git a/compat/mingw.c b/compat/mingw.c
index 3fbfda5..9173709 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -2232,6 +2232,9 @@ void mingw_startup(void)
/* initialize critical section for waitpid pinfo_t list */
InitializeCriticalSection(&pinfo_cs);
+ /* initialize critical sections in the attr code */
+ attr_start();
+
/* set up default file mode and file modes for stdin/out/err */
_fmode = _O_BINARY;
_setmode(_fileno(stdin), _O_BINARY);
diff --git a/convert.c b/convert.c
index 4eca0b5..6feffa3 100644
--- a/convert.c
+++ b/convert.c
@@ -722,9 +722,9 @@ static int ident_to_worktree(const char *path, const char *src, size_t len,
return 1;
}
-static enum crlf_action git_path_check_crlf(struct git_attr_check_elem *check)
+static enum crlf_action git_path_check_crlf(struct git_attr_result *result)
{
- const char *value = check->value;
+ const char *value = result->value;
if (ATTR_TRUE(value))
return CRLF_TEXT;
@@ -739,9 +739,9 @@ static enum crlf_action git_path_check_crlf(struct git_attr_check_elem *check)
return CRLF_UNDEFINED;
}
-static enum eol git_path_check_eol(struct git_attr_check_elem *check)
+static enum eol git_path_check_eol(struct git_attr_result *result)
{
- const char *value = check->value;
+ const char *value = result->value;
if (ATTR_UNSET(value))
;
@@ -752,9 +752,9 @@ static enum eol git_path_check_eol(struct git_attr_check_elem *check)
return EOL_UNSET;
}
-static struct convert_driver *git_path_check_convert(struct git_attr_check_elem *check)
+static struct convert_driver *git_path_check_convert(struct git_attr_result *result)
{
- const char *value = check->value;
+ const char *value = result->value;
struct convert_driver *drv;
if (ATTR_TRUE(value) || ATTR_FALSE(value) || ATTR_UNSET(value))
@@ -765,9 +765,9 @@ static struct convert_driver *git_path_check_convert(struct git_attr_check_elem
return NULL;
}
-static int git_path_check_ident(struct git_attr_check_elem *check)
+static int git_path_check_ident(struct git_attr_result *result)
{
- const char *value = check->value;
+ const char *value = result->value;
return !!ATTR_TRUE(value);
}
@@ -782,25 +782,27 @@ struct conv_attrs {
static void convert_attrs(struct conv_attrs *ca, const char *path)
{
static struct git_attr_check *check;
+ static int init_user_convert_tail;
+ struct git_attr_result result[5];
- if (!check) {
- check = git_attr_check_initl("crlf", "ident",
- "filter", "eol", "text",
- NULL);
+ git_attr_check_initl(&check, "crlf", "ident", "filter",
+ "eol", "text", NULL);
+
+ if (!init_user_convert_tail) {
user_convert_tail = &user_convert;
git_config(read_convert_config, NULL);
+ init_user_convert_tail = 1;
}
- if (!git_check_attr(path, check)) {
- struct git_attr_check_elem *ccheck = check->check;
- ca->crlf_action = git_path_check_crlf(ccheck + 4);
+ if (!git_check_attr(path, check, result)) {
+ ca->crlf_action = git_path_check_crlf(&result[4]);
if (ca->crlf_action == CRLF_UNDEFINED)
- ca->crlf_action = git_path_check_crlf(ccheck + 0);
+ ca->crlf_action = git_path_check_crlf(&result[0]);
ca->attr_action = ca->crlf_action;
- ca->ident = git_path_check_ident(ccheck + 1);
- ca->drv = git_path_check_convert(ccheck + 2);
+ ca->ident = git_path_check_ident(&result[1]);
+ ca->drv = git_path_check_convert(&result[2]);
if (ca->crlf_action != CRLF_BINARY) {
- enum eol eol_attr = git_path_check_eol(ccheck + 3);
+ enum eol eol_attr = git_path_check_eol(&result[3]);
if (ca->crlf_action == CRLF_AUTO && eol_attr == EOL_LF)
ca->crlf_action = CRLF_AUTO_INPUT;
else if (ca->crlf_action == CRLF_AUTO && eol_attr == EOL_CRLF)
diff --git a/ll-merge.c b/ll-merge.c
index bc6479c..d71354a 100644
--- a/ll-merge.c
+++ b/ll-merge.c
@@ -353,12 +353,14 @@ int ll_merge(mmbuffer_t *result_buf,
mmfile_t *theirs, const char *their_label,
const struct ll_merge_options *opts)
{
- static struct git_attr_check *check;
static const struct ll_merge_options default_opts;
const char *ll_driver_name = NULL;
int marker_size = DEFAULT_CONFLICT_MARKER_SIZE;
const struct ll_merge_driver *driver;
+ static struct git_attr_check *check;
+ struct git_attr_result result[2];
+
if (!opts)
opts = &default_opts;
@@ -368,13 +370,12 @@ int ll_merge(mmbuffer_t *result_buf,
normalize_file(theirs, path);
}
- if (!check)
- check = git_attr_check_initl("merge", "conflict-marker-size", NULL);
+ git_attr_check_initl(&check, "merge", "conflict-marker-size", NULL);
- if (!git_check_attr(path, check)) {
- ll_driver_name = check->check[0].value;
- if (check->check[1].value) {
- marker_size = atoi(check->check[1].value);
+ if (!git_check_attr(path, check, result)) {
+ ll_driver_name = result[0].value;
+ if (result[1].value) {
+ marker_size = atoi(result[1].value);
if (marker_size <= 0)
marker_size = DEFAULT_CONFLICT_MARKER_SIZE;
}
@@ -395,11 +396,12 @@ int ll_merge_marker_size(const char *path)
{
static struct git_attr_check *check;
int marker_size = DEFAULT_CONFLICT_MARKER_SIZE;
+ struct git_attr_result result[1];
+
+ git_attr_check_initl(&check, "conflict-marker-size", NULL);
- if (!check)
- check = git_attr_check_initl("conflict-marker-size", NULL);
- if (!git_check_attr(path, check) && check->check[0].value) {
- marker_size = atoi(check->check[0].value);
+ if (!git_check_attr(path, check, result) && !ATTR_UNSET(result[0].value)) {
+ marker_size = atoi(result[0].value);
if (marker_size <= 0)
marker_size = DEFAULT_CONFLICT_MARKER_SIZE;
}
diff --git a/userdiff.c b/userdiff.c
index 46dfd32..1d6d363 100644
--- a/userdiff.c
+++ b/userdiff.c
@@ -263,21 +263,23 @@ struct userdiff_driver *userdiff_find_by_name(const char *name) {
struct userdiff_driver *userdiff_find_by_path(const char *path)
{
static struct git_attr_check *check;
+ struct git_attr_result result[1];
- if (!check)
- check = git_attr_check_initl("diff", NULL);
if (!path)
return NULL;
- if (git_check_attr(path, check))
+
+ git_attr_check_initl(&check, "diff", NULL);
+
+ if (git_check_attr(path, check, result))
return NULL;
- if (ATTR_TRUE(check->check[0].value))
+ if (ATTR_TRUE(result[0].value))
return &driver_true;
- if (ATTR_FALSE(check->check[0].value))
+ if (ATTR_FALSE(result[0].value))
return &driver_false;
- if (ATTR_UNSET(check->check[0].value))
+ if (ATTR_UNSET(result[0].value))
return NULL;
- return userdiff_find_by_name(check->check[0].value);
+ return userdiff_find_by_name(result[0].value);
}
struct userdiff_driver *userdiff_get_textconv(struct userdiff_driver *driver)
diff --git a/ws.c b/ws.c
index bb3270c..7e355c4 100644
--- a/ws.c
+++ b/ws.c
@@ -74,14 +74,14 @@ unsigned parse_whitespace_rule(const char *string)
unsigned whitespace_rule(const char *pathname)
{
static struct git_attr_check *attr_whitespace_rule;
+ struct git_attr_result result[1];
- if (!attr_whitespace_rule)
- attr_whitespace_rule = git_attr_check_initl("whitespace", NULL);
+ git_attr_check_initl(&attr_whitespace_rule, "whitespace", NULL);
- if (!git_check_attr(pathname, attr_whitespace_rule)) {
+ if (!git_check_attr(pathname, attr_whitespace_rule, result)) {
const char *value;
- value = attr_whitespace_rule->check[0].value;
+ value = result[0].value;
if (ATTR_TRUE(value)) {
/* true (whitespace) */
unsigned all_rule = ws_tab_width(whitespace_rule_cfg);
--
2.10.1.530.g38c3896
^ permalink raw reply related
* Re: Expanding Includes in .gitignore
From: Aaron Pelly @ 2016-10-27 22:17 UTC (permalink / raw)
To: Jeff King; +Cc: git
In-Reply-To: <60a652f6-864e-bbda-7394-4751c92866b7@pelly.co>
On 28/10/16 10:55, Aaron Pelly wrote:
> 2) I fetch a repo with a hostile ignore file. It includes files from
> $GIT_DIR/test-data/ssl/private or some such. Change. Don't pay
> attention. Commit. Push. Problems if my test data comes from production.
>
> Is this mitigated currently?
>
> Not that git should be an enabler, but surely it falls on the user of
> untrusted software to ensure their own security?
Balls, I meant $GIT_WORK_TREE not $GIT_DIR
^ permalink raw reply
* [PATCH] valgrind: support test helpers
From: René Scharfe @ 2016-10-27 22:14 UTC (permalink / raw)
To: Git List; +Cc: Duy Nguyen, Johannes Schindelin, Junio C Hamano
Tests run with --valgrind call git commands through a wrapper script
that invokes valgrind on them. This script (valgrind.sh) is in turn
invoked through symlinks created for each command in t/valgrind/bin/.
Since e6e7530d (test helpers: move test-* to t/helper/ subdirectory)
these symlinks have been broken for test helpers -- they point to the
old locations in the root of the build directory. Fix that by teaching
the code for creating the links about the new location of the binaries,
and do the same in the wrapper script to allow it to find its payload.
Signed-off-by: Rene Scharfe <l.s.r@web.de>
---
t/test-lib.sh | 9 ++++++++-
t/valgrind/valgrind.sh | 12 ++++++++++--
2 files changed, 18 insertions(+), 3 deletions(-)
diff --git a/t/test-lib.sh b/t/test-lib.sh
index b859db6..a724181 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -809,7 +809,14 @@ then
return;
base=$(basename "$1")
- symlink_target=$GIT_BUILD_DIR/$base
+ case "$base" in
+ test-*)
+ symlink_target="$GIT_BUILD_DIR/t/helper/$base"
+ ;;
+ *)
+ symlink_target="$GIT_BUILD_DIR/$base"
+ ;;
+ esac
# do not override scripts
if test -x "$symlink_target" &&
test ! -d "$symlink_target" &&
diff --git a/t/valgrind/valgrind.sh b/t/valgrind/valgrind.sh
index 4215303..669ebaf 100755
--- a/t/valgrind/valgrind.sh
+++ b/t/valgrind/valgrind.sh
@@ -1,11 +1,19 @@
#!/bin/sh
base=$(basename "$0")
+case "$base" in
+test-*)
+ program="$GIT_VALGRIND/../../t/helper/$base"
+ ;;
+*)
+ program="$GIT_VALGRIND/../../$base"
+ ;;
+esac
TOOL_OPTIONS='--leak-check=no'
test -z "$GIT_VALGRIND_ENABLED" &&
-exec "$GIT_VALGRIND"/../../"$base" "$@"
+exec "$program" "$@"
case "$GIT_VALGRIND_MODE" in
memcheck-fast)
@@ -29,4 +37,4 @@ exec valgrind -q --error-exitcode=126 \
--log-fd=4 \
--input-fd=4 \
$GIT_VALGRIND_OPTIONS \
- "$GIT_VALGRIND"/../../"$base" "$@"
+ "$program" "$@"
--
2.10.1
^ permalink raw reply related
* Re: feature request
From: John Rood @ 2016-10-27 22:05 UTC (permalink / raw)
To: Stefan Beller; +Cc: git@vger.kernel.org
In-Reply-To: <CAGZ79kZAfqxB699MOs6A6RL==Ku-qF7ABiW=eA+TSrqK+8e_sA@mail.gmail.com>
Unfortunately, in my case I'm on windows (my company's choice, not mine).
On Thu, Oct 27, 2016 at 5:01 PM, Stefan Beller <sbeller@google.com> wrote:
> On Thu, Oct 27, 2016 at 2:55 PM, John Rood <mr.john.rood@gmail.com> wrote:
>> Users should be able to configure Git to not send them into a Vim editor.
>
> See https://git-scm.com/docs/git-var
>
> GIT_EDITOR
>
> Text editor for use by Git commands. The value is meant to be interpreted
> by the shell when it is used. Examples: ~/bin/vi, $SOME_ENVIRONMENT_VARIABLE,
> "C:\Program Files\Vim\gvim.exe" --nofork. The order of preference is the
> $GIT_EDITOR environment variable, then core.editor configuration, then
> $VISUAL, then $EDITOR, and then the default chosen at compile time,
> which is usually vi.
>
>
> So maybe
>
> git config --global core.editor "nano"
>
> helps in your case?
^ permalink raw reply
* Re: feature request
From: Stefan Beller @ 2016-10-27 22:01 UTC (permalink / raw)
To: John Rood; +Cc: git@vger.kernel.org
In-Reply-To: <CALj-rGeoT_mpmuw8Put=6eRhzf-r2WUohu_Kd-wnpc=BvO5joA@mail.gmail.com>
On Thu, Oct 27, 2016 at 2:55 PM, John Rood <mr.john.rood@gmail.com> wrote:
> Users should be able to configure Git to not send them into a Vim editor.
See https://git-scm.com/docs/git-var
GIT_EDITOR
Text editor for use by Git commands. The value is meant to be interpreted
by the shell when it is used. Examples: ~/bin/vi, $SOME_ENVIRONMENT_VARIABLE,
"C:\Program Files\Vim\gvim.exe" --nofork. The order of preference is the
$GIT_EDITOR environment variable, then core.editor configuration, then
$VISUAL, then $EDITOR, and then the default chosen at compile time,
which is usually vi.
So maybe
git config --global core.editor "nano"
helps in your case?
^ permalink raw reply
* Re: [PATCH] compat: Allow static initializer for pthreads on Windows
From: Stefan Beller @ 2016-10-27 21:59 UTC (permalink / raw)
To: Jacob Keller
Cc: Johannes Sixt, Junio C Hamano, Johannes Schindelin,
git@vger.kernel.org, Simon Ruderich, Jeff King
In-Reply-To: <CA+P7+xpckfaeHmoEGQBdLD-=Kf7gQ-jOxGFKrKmiFH1SBN7GjA@mail.gmail.com>
> there isn't really a great place to put a dynamic initialization.
Answering this question specifically (Where to put it?),
I am about to send out a patch that puts it in compat/mingw.c:2232:
diff --git a/compat/mingw.c b/compat/mingw.c
index 3fbfda5..9881c3d 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -2232,6 +2232,9 @@ void mingw_startup(void)
/* initialize critical section for waitpid pinfo_t list */
InitializeCriticalSection(&pinfo_cs);
+ /* initialize critical sections in the attr code */
+ start_attr();
+
/* set up default file mode and file modes for stdin/out/err */
_fmode = _O_BINARY;
_setmode(_fileno(stdin), _O_BINARY);
If I understood Johannes correctly this is the place to put it.
Junio seems to be ok with static mutex init for non windows platforms,
so I put it into the mingw specifc startup code.
Thanks,
Stefan
^ permalink raw reply related
* feature request
From: John Rood @ 2016-10-27 21:55 UTC (permalink / raw)
To: git
Users should be able to configure Git to not send them into a Vim editor.
When users pull commits, and a new commit needs to be created for a
merge, Git's current way of determining a commit message is to send
the user into a Vim window so that they can write a message. There are
2 reasons why this might not be the ideal way to prompt for a commit
message.
1. Many users are used to writing concise one-line commit messages and
would not expect to save a commit message in a multi-line file. Some
users will wonder why they are in a text editor or which file they are
editing. Others may not, in fact, realize at all that a text editor is
what they are in.
2. Many users are not familiar with Vim, and do not understand how to
modify, save, and exit. It is not very considerate to require a user
to learn Vim in order to finish a commit that they are in the middle
of.
The existing behavior should be optional, and there should be two new options:
1. Use a simple inline prompt for a commit message (in the same way
Git might prompt for a username).
2. Automatically assign names for commits in the form of "Merged x into y".
^ permalink raw reply
* Re: Expanding Includes in .gitignore
From: Aaron Pelly @ 2016-10-27 21:55 UTC (permalink / raw)
To: Jeff King; +Cc: git
In-Reply-To: <20161027205508.vqw44zlbnqpj2cvd@sigill.intra.peff.net>
On 28/10/16 09:55, Jeff King wrote:
> On Fri, Oct 28, 2016 at 09:28:23AM +1300, Aaron Pelly wrote:
>
>>> - we parse possibly-hostile .gitignore files from cloned repositories.
>>> What happens when I include ask to include /etc/passwd? Probably
>>> nothing, but there are setups where it might matter (e.g., something
>>> like Travis that auto-builds untrusted repositories, and you could
>>> potentially leak the contents of files via error messages). It's
>>> nice to avoid the issue entirely.
>>
>> I understand the issue.
>>
>> It's not obvious to me how using a .d solves this problem though.
>
> It doesn't by itself. But we are worried only about tracked .gitignore
> files (recall that even repo-level files in $GIT_DIR/info are generated
> fresh by the clone process, and don't come from the remote). If we apply
> the feature only to core.excludeFile and $GIT_DIR/info/exclude, those
> are already under the user's control.
The things you say make sense from this perspective.
I was hoping to employ this mechanism throughout the git ecosystem.
Thinking out loud for a minute:
1) I clone a repo with a hostile ignore file. It includes files from
/etc/ssl/private or some such. Change. Don't pay attention. Commit.
Push. Problems.
What is the use case for reaching out of the repo in the first place?
2) I fetch a repo with a hostile ignore file. It includes files from
$GIT_DIR/test-data/ssl/private or some such. Change. Don't pay
attention. Commit. Push. Problems if my test data comes from production.
Is this mitigated currently?
Not that git should be an enabler, but surely it falls on the user of
untrusted software to ensure their own security?
> It's true that we could make a similar exception for an "include"
> feature, and respect include directives only in those "safe" files.
> Somehow that seems more confusing to me, though, than doing adding the
> feature at the file level, as it introduces slightly varying syntax
> between the locations.
I'm quickly getting over the include file idea. But yes, that would be
non obvious.
>>> Whereas letting any of the user- or repo-level exclude files be a
>>> directory, and simply reading all of the files inside, seems simple and
>>> obvious.
>>
>> Apart from backwards compatibility, unless there's something I'm missing.
>
> I'm not sure what you mean. If we make:
>
> mkdir .git/info/exclude
> echo whatever >.git/info/exclude/local
>
> work, I don't think we have to care about backwards compatibility. That
> was nonsensical before, and never did anything useful (so somebody might
> have done it, but we can assume anybody relying on it not to read the
> contents is crazy).
Seeing your perspective, now, I can see why you didn't understand me. In
your context this makes perfect sense.
^ permalink raw reply
* Re: [PATCH] compat: Allow static initializer for pthreads on Windows
From: Jacob Keller @ 2016-10-27 21:49 UTC (permalink / raw)
To: Johannes Sixt
Cc: Stefan Beller, Junio C Hamano, Johannes Schindelin,
git@vger.kernel.org, Simon Ruderich, Jeff King
In-Reply-To: <a2e5acd6-485d-0387-7a85-6042dee702f7@kdbg.org>
On Thu, Oct 27, 2016 at 1:05 PM, Johannes Sixt <j6t@kdbg.org> wrote:
>> The implementation under discussion (well we did not discuss the
>> implementation a
>> whole lot yet) ...
>
>
> There's not a whole lot to discuss: it must be rewritten from scratch (it's
> not just the memory barriers, it is everything else, too). But time is much
> better spent on an attr_start() solution.
>
> -- Hannes
>
Ok, so I've been reading this thread. I don't understand your
objections to emulating in this way.. Could you clearly spell out why
you believe this solution isn't acceptable? So far all I've understood
was "it's not critical sections" and "it penalizes Windows too much"
but... If Windows cannot statically initialize a pthread mutex, then
we *have* to dynamically initialize it somewhere. This solution adds a
single check before each lock and is safe due to use of memory
barriers. Yes, this will cost a tiny bit extra overhead for each use
of "pthread_mutex_lock" but I fail to see how that is a huge
penalty...
So I'm trying to understand if that really is your only concern,
because I don't actually buy that it is a huge overhead to pay.
I agree with Stefan that there isn't really a great place to put a
dynamic initialization.
To be clear I am trying to understand the objections since so far all
I have read is "this is bad, and I object" without clearly explaining
reasoning.
Regards,
Jake
^ permalink raw reply
* Re: [PATCH v3 2/3] sha1_file: open window into packfiles with O_CLOEXEC
From: Junio C Hamano @ 2016-10-27 21:49 UTC (permalink / raw)
To: Jeff King
Cc: Linus Torvalds, git, Lars Schneider, Eric Wong,
Johannes Schindelin
In-Reply-To: <20161027102419.dbzigj7wtr355ofh@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> +cc Linus as the original author of 144bde78e9 in case there is
> something subtle I'm missing, but this really just seems like it's
> an outdated optimization.
>
> -- >8 --
> Subject: [PATCH] sha1_file: stop opening files with O_NOATIME
>
> When we open object files, we try to do so with O_NOATIME.
> This dates back to 144bde78e9 (Use O_NOATIME when opening
> the sha1 files., 2005-04-23), which is an optimization to
> avoid creating a bunch of dirty inodes when we're accessing
> many objects. But a few things have changed since then:
>
> 1. In June 2005, git learned about packfiles, which means
> we would do a lot fewer atime updates (rather than one
> per object access, we'd generally get one per packfile).
>
> 2. In late 2006, Linux learned about "relatime", which is
> generally the default on modern installs. So
> performance around atimes updates is a non-issue there
> these days.
>
> All the world isn't Linux, but as it turns out, Linux
> is the only platform to implement O_NOATIME in the
> first place.
>
> So it's very unlikely that this code is helping anybody
> these days.
>
> It's not a particularly large amount of code, but the
> fallback-retry creates complexity. E.g., we do a similar
> fallback for CLOEXEC; which one should take precedence, or
> should we try all possible combinations? Dropping O_NOATIME
> makes those questions go away.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
We may want to lose the surrounding for (;;) loop as there is only
one flag to retry without, which was the original code structure
back when 144bde78e9 ("Use O_NOATIME when opening the sha1 files.",
2005-04-23) was written and refactored by 44d1c19ee8 ("Make loose
object file reading more careful", 2008-06-14).
IOW, this on top. The update to ce_compare_data() Lars has in
a0a6cb9662 ("read-cache: make sure file handles are not inherited by
child processes", 2016-10-24) could then made into a call to
git_open().
sha1_file.c | 23 ++++++++---------------
1 file changed, 8 insertions(+), 15 deletions(-)
diff --git a/sha1_file.c b/sha1_file.c
index 6f02a57d8b..e18ea053e6 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -1553,24 +1553,17 @@ int check_sha1_signature(const unsigned char *sha1, void *map,
int git_open(const char *name)
{
- static int sha1_file_open_flag = O_CLOEXEC;
-
- for (;;) {
- int fd;
-
- errno = 0;
- fd = open(name, O_RDONLY | sha1_file_open_flag);
- if (fd >= 0)
- return fd;
+ static int cloexec = O_CLOEXEC;
+ int fd;
+ errno = 0;
+ fd = open(name, O_RDONLY | cloexec);
+ if ((cloexec & O_CLOEXEC) && fd < 0 && errno == EINVAL) {
/* Try again w/o O_CLOEXEC: the kernel might not support it */
- if ((sha1_file_open_flag & O_CLOEXEC) && errno == EINVAL) {
- sha1_file_open_flag &= ~O_CLOEXEC;
- continue;
- }
-
- return -1;
+ cloexec &= ~O_CLOEXEC;
+ fd = open(name, O_RDONLY | cloexec);
}
+ return fd;
}
static int stat_sha1_file(const unsigned char *sha1, struct stat *st)
^ permalink raw reply related
* Re: Expanding Includes in .gitignore
From: Jacob Keller @ 2016-10-27 21:39 UTC (permalink / raw)
To: Jeff King; +Cc: Aaron Pelly, Git mailing list
In-Reply-To: <20161027210458.ptzh4y75dkfaixeo@sigill.intra.peff.net>
On Thu, Oct 27, 2016 at 2:04 PM, Jeff King <peff@peff.net> wrote:
> I'm not convinced this is needed for in-repo .gitignore files. The point
> is that you are pulling together separate files that may be administered
> independently. But git repositories inherently have a whole-project
> view. I'm not sure that separate files buy you a lot there. And the
> compatibility issues are more complicated.
>
I had just assumed it would be used everywhere, so I hadn't really
considered whether that was any reason not to. I personally like the
idea of splitting my git ignores into separate files just so each file
can be about one thing, but I guess it's not really a problem either
way.
> I do agree that:
>
> cd .git/info
> git clone /my/exclude/repo exclude ;# or exclude.d
>
> should work; ignoring dotfiles when reading the directory solves that,
> and is a pretty standard solution.
>
> -Peff
^ permalink raw reply
* Re: [PATCH 32/36] pathspec: allow querying for attributes
From: Stefan Beller @ 2016-10-27 21:32 UTC (permalink / raw)
To: Duy Nguyen; +Cc: Junio C Hamano, Git Mailing List, Brandon Williams
In-Reply-To: <CACsJy8DhsaTY1w_e-0O5d4KLxr4Gmo3g9rDagQq1ooSHywcRoQ@mail.gmail.com>
On Wed, Oct 26, 2016 at 6:33 AM, Duy Nguyen <pclouds@gmail.com> wrote:
> (sorry if this should have been answered if I went through the series
> patch by patch, I wanted to do a proper review but finally have to
> admit to myself I won't, so I just skim through a single giant diff
> instead)
>
> On Sun, Oct 23, 2016 at 6:32 AM, Stefan Beller <sbeller@google.com> wrote:
>> +attr;;
>> +After `attr:` comes a space separated list of "attribute
>> +requirements", all of which must be met in order for the
>> +path to be considered a match;
>
> What about (attr=abc def,attr=ghi lkj)? Does it mean (abc && def) ||
> (ghi && lkj), or abc && def && ghi && lkj? Or is it forbidden to have
> multiple 'attr' attribute in the same pathspec?
Good point. I'll add a test for that.
Remembering the original discussion, multiple attrs
are forbidden for now as it is unclear what you want to see
as a user.
To model (abc && def) || (ghi && lkj), you would need to give
multiple pathspec items as these are naturally ORed:
git ls-files :(attr:abc def) :(attr:ghi lkj) .
(compare "git ls-files Makefile README" which gives
2 files to you, that are named respectively.)
To get "abc && def && ghi && lkj" you go with
git ls-files :(attr:abc def ghi lkj) .
as then all things included into this one attr are
ANDed. I hope the documentation is clear for one
attr.
Thanks,
Stefan
^ permalink raw reply
* Re: Expanding Includes in .gitignore
From: Aaron Pelly @ 2016-10-27 21:26 UTC (permalink / raw)
To: Jeff King, Jacob Keller; +Cc: Git mailing list
In-Reply-To: <20161027210458.ptzh4y75dkfaixeo@sigill.intra.peff.net>
On 28/10/16 10:04, Jeff King wrote:
> On Thu, Oct 27, 2016 at 12:48:34PM -0700, Jacob Keller wrote:
>
>>> I think the normal behavior in such "foo.d" directory is to just sort
>>> the contents lexically and read them in order, as if they were all
>>> concatenated together, and with no recursion. I.e., behave "as if" the
>>> user had run "cat $dir/*".
>>
>> Yea, this is the normal behavior, and the user is expected to order
>> their files lexically such as "00-name", "50-name" and so on. Pretty
>> traditional for a lot of newer configurations.
>
> One thing I will say about this approach is that you can implement it
> without any changes in git by doing:
>
> path=.git/info/exclude
> cat $path.d/* >$path
>
> and I have seen several config mechanisms basically do that (e.g.,
> Debian packaging for a program that doesn't have its own ".d" mechanism,
> but needs to grab config provided by several separate packages).
>
> The reason to teach that trick to git is convenience; you don't have to
> remember to build the master file from its parts because it's done
> dynamically whenever git needs to look at it.
Precisely.
>> One thing to keep in mind would be that we should make sure we can
>> handle the .gitignore being a submodule or a git repository, so that
>> users could just do something like
>
> I'm not convinced this is needed for in-repo .gitignore files. The point
> is that you are pulling together separate files that may be administered
> independently. But git repositories inherently have a whole-project
> view. I'm not sure that separate files buy you a lot there. And the
> compatibility issues are more complicated.
>
> I do agree that:
>
> cd .git/info
> git clone /my/exclude/repo exclude ;# or exclude.d
>
> should work; ignoring dotfiles when reading the directory solves that,
> and is a pretty standard solution.
You raise a good point about the requirement. I was about to make an
argument in favour of it, but I argued myself out of an argument. I will
say; why have it work in one place and not others?
^ permalink raw reply
* Re: [PATCH v2 2/2] convert.c: stream and fast search for binary
From: Junio C Hamano @ 2016-10-27 21:18 UTC (permalink / raw)
To: tboegi; +Cc: git
In-Reply-To: <20161012134727.28365-1-tboegi@web.de>
tboegi@web.de writes:
> From: Torsten Bögershausen <tboegi@web.de>
>
> When statistics are done for the autocrlf handling, the search in
> the content can be stopped, if e.g
> - a search for binary is done, and a NUL character is found
> - a search for CRLF is done, and the first CRLF is found.
>
> Similar when statistics for binary vs non-binary are gathered:
> Whenever a lone CR or NUL is found, the search can be aborted.
>
> When checking out files in "auto" mode, any file that has a "lone CR"
> or a CRLF will not be converted, so the search can be aborted early.
>
> Add the new bit, CONVERT_STAT_BITS_ANY_CR,
> which is set for either lone CR or CRLF.
>
> Many binary files have a NUL very early and it is often not necessary
> to load the whole content of a file or blob into memory.
>
> Split gather_stats() into gather_all_stats() and gather_stats_partly()
> to do a streaming handling for blobs and files in the worktree.
>
> Signed-off-by: Torsten Bögershausen <tboegi@web.de>
> ---
I'll try to review in reverse order, as this seems to be doing too
many things at once and cannot get my head around it without going
top down.
> @@ -338,11 +401,15 @@ static int crlf_to_worktree(const char *path, const char *src, size_t len,
> {
> char *to_free = NULL;
> struct text_stat stats;
> + unsigned search_only = 0;
>
> if (!len || output_eol(crlf_action) != EOL_CRLF)
> return 0;
>
> - gather_stats(src, len, &stats);
> + if (crlf_action == CRLF_AUTO || crlf_action == CRLF_AUTO_CRLF)
> + search_only = CONVERT_STAT_BITS_ANY_CR | CONVERT_STAT_BITS_BIN;
> +
> + gather_all_stats(src, len, &stats, search_only);
> if (!will_convert_lf_to_crlf(len, &stats, crlf_action))
> return 0;
This special case to decide whether we would limit the search_only
flag has too much intimate knowledge of what happens inside
will_convert_lf_to_crlf(). It knows that output_eol(crlf_action)
not being EOL_CRLF is the very first thing the function checks, too.
Makes one wonder if the check for output_eol(crlf_action) can be
removed from will_convert_lf_to_crlf(), no? It is not apparent if
that is a good idea for the other caller in crlf_to_git().
gather_all_stats() will give up immediately when it sees either
ANY_CR or BIN. If CR appears before we see any BIN, stat_bits would
not have BITS_BIN even if the buffer may have BIN byte later. It is
OK because either lonecr or crlf would be non-zero, and
will_convert_lf_to_crlf() would return 0. If BIN apepars before we
see any CR, neither lonecr nor crlf will become non-zero even if the
buffer may have CR byte later, but again it is OK because
will_convert_lf_to_crlf() will return 0 in that case.
This looks too brittle, even though it is correct.
> @@ -253,7 +300,8 @@ static int crlf_to_git(const char *path, const char *src, size_t len,
> {
> struct text_stat stats;
> char *dst;
> - int convert_crlf_into_lf;
> + int has_crlf_to_convert;
> + unsigned search_only = 0;
>
> if (crlf_action == CRLF_BINARY ||
> (src && !len))
> @@ -266,12 +314,16 @@ static int crlf_to_git(const char *path, const char *src, size_t len,
> if (!buf && !src)
> return 1;
>
> - gather_stats(src, len, &stats);
> + if (crlf_action == CRLF_AUTO || crlf_action == CRLF_AUTO_INPUT || crlf_action == CRLF_AUTO_CRLF)
> + search_only = CONVERT_STAT_BITS_BIN;
> +
> + gather_all_stats(src, len, &stats, search_only);
> +
> /* Optimization: No CRLF? Nothing to convert, regardless. */
> - convert_crlf_into_lf = !!stats.crlf;
> + has_crlf_to_convert = !!stats.crlf;
The comment here may need to say a lot more, now we do not even
count .crlf in some cases because of "search_only" setting.
> if (crlf_action == CRLF_AUTO || crlf_action == CRLF_AUTO_INPUT || crlf_action == CRLF_AUTO_CRLF) {
The new "search_only" criteria above was added to match this if
block; it is not as bad as the previous one in crlf_to_worktree()
that knows, and must be kept in sync with, what a separate function
will_convert_lf_to_crlf() does, but still it is horrible for both
maintainability and readability. Can you devise some mechanism to
ensure that these two if statements will stay in sync?
> - if (convert_is_binary(len, &stats))
> + if (stats.stat_bits & CONVERT_STAT_BITS_BIN)
> return 0;
We no longer need the helper function convert_is_binary() and
instead need only STATS_BITS_BIN bit, so the control flow that
reaches this point is obviously correct (assuming that
gather_all_stats() that is limited with search_only option counts
things correctly, that is).
But what happens when we don't return here? We didn't get the full
stats out of gather_all_stats() and continue. Let's see what
happens in that case...
> /*
> * If the file in the index has any CR in it, do not convert.
> @@ -280,24 +332,35 @@ static int crlf_to_git(const char *path, const char *src, size_t len,
> if (checksafe == SAFE_CRLF_RENORMALIZE)
> checksafe = SAFE_CRLF_FALSE;
> else if (has_cr_in_index(path))
> - convert_crlf_into_lf = 0;
> + has_crlf_to_convert = 0;
> }
So at this point, we cannot trust what is in "stats" (we may have
come out of the above if() statement because it wasn't binary after
all). It is unclear to me if we may or may not be able to trust
has_crlf_to_convert at this point. If crlf_action was one of the
three magic values that caused search_only for BITS_BIN set, then
stats.crlf may or may not have seen CRLF---if a NUL came before any
CRLF, gather_all_stats() would have returned without seeing a CRLF
that exists, and otherwise it may have seen one and counted, so
has_crlf_to_convert that is set immediately after gather_all_stats()
returned cannot be trusted at all, when BITS_BIN was set in the
result.
What saves this codepath is that we would have returned at this
point if BITS_BIN was set in the result, so stats.crlf immediately
after gather_all_stats() returned can be trusted in that case, which
in turn means has_crlf_to_convert can also be trusted here. Whew.
I hate to say this, and it certainly is not the fault of this patch,
but the result of applying this patch is undecipherable without a
great effort and is too brittle. A reviewer or any future developer
who has to touch this codepath should not be forced to do this kind
of analysis. Either the code should make everything I wrote above
clear by itself, or more in-code comment must talk about these
things.
Anyway, let's say we established that has_crlf_to_convert and
checksafe can be trustable at this point in the control flow, and
let's keep reading.
> if (checksafe && len) {
> struct text_stat new_stats;
> memcpy(&new_stats, &stats, sizeof(new_stats));
> /* simulate "git add" */
> - if (convert_crlf_into_lf) {
> + if (has_crlf_to_convert) {
> new_stats.lonelf += new_stats.crlf;
> new_stats.crlf = 0;
> + /* all crlf, if any, are gone. Update the bits */
> + new_stats.stat_bits = stats.stat_bits & CONVERT_STAT_BITS_BIN;
> + if (new_stats.lonelf)
> + new_stats.stat_bits |= CONVERT_STAT_BITS_TXT_LF;
> + if (new_stats.lonecr)
> + new_stats.stat_bits |= CONVERT_STAT_BITS_ANY_CR;
What's happening here? stats.crlf and stats.lonelf are both
trustable, because even when BITS_BIN optimization were asked when
calling gather_all_stats(), we wouldn't come here if the
optimization actually kicked in. If we convert crlf to lf, the
result would have more lonelf than the original by the number of
crlf to be converted, and the result would have no crlf, and
new_stats are adjusted to pretend as if we have ran the stat over
the buffer after conversion. You further adjust .stat_bits, which
were not necessary in the old code. The new comment says "Update
the bits" but what it should make it clear is why the new code cares
about the bits when the old code didn't. ANY_CR is the new thing
and is understandable, but old code didn't flip TXT_LF bit. Is it a
bugfix in the old code and it should have done so without your "find
partially and return early" optimization? If so, that should have
been a separate patch to be understandable.
I am guessing that this is an attempt to future-proof the contents
of new_stats, so that will_convert_lf_to_crlf() can take a short-cut
by looking at these bits, even though currently it does not look at
any bit other than BITS_BIN. If that is the case, that needs to be
told to the reader. "Update the bits" is something any reader can
see. You need to tell them why you are updating the bits.
> }
> /* simulate "git checkout" */
> if (will_convert_lf_to_crlf(len, &new_stats, crlf_action)) {
> new_stats.crlf += new_stats.lonelf;
> new_stats.lonelf = 0;
> + new_stats.stat_bits = stats.stat_bits & CONVERT_STAT_BITS_BIN;
> + if (new_stats.crlf)
> + new_stats.stat_bits |= CONVERT_STAT_BITS_TXT_CRLF | CONVERT_STAT_BITS_ANY_CR;
> + if (new_stats.lonecr)
> + new_stats.stat_bits |= CONVERT_STAT_BITS_ANY_CR;
> }
> check_safe_crlf(path, crlf_action, &stats, &new_stats, checksafe);
Likewise. Is this future-proofing new_stats to allow check_safe_crlf()
to use them in the future?
> @@ -86,41 +97,62 @@ static void gather_stats(const char *buf, unsigned long size, struct text_stat *
> * The same heuristics as diff.c::mmfile_is_binary()
> * We treat files with bare CR as binary
> */
> -static int convert_is_binary(unsigned long size, const struct text_stat *stats)
> +static void convert_nonprintable(struct text_stat *stats)
> {
> - if (stats->lonecr)
> - return 1;
> - if (stats->nul)
> - return 1;
> if ((stats->printable >> 7) < stats->nonprintable)
> - return 1;
> - return 0;
> + stats->stat_bits |= CONVERT_STAT_BITS_BIN;
> }
When search_only is set to BIN, stat_bits would have BIN if we saw
any non-printable control byte, so calling convert_nonprintable() at
the end of gather_all_stats() to flip the BIN bit on with
printable/nonprintable ratio has no effect in that case. If we
didn't see a non-printable control byte, gather_stats_partly()
wouldn't have set BIN in the result but then we know the count of
printable and nonprintable are trustworthy, so comparison to flip
the bit makes sense.
The presense of lonecr does not matter, as that would have flipped
BIN, too. So does the check for NUL, too, has become unnecessary.
I think the end result may be likely to be correct, but with too
many things going on at once, I cannot be confident in saying so.
This probably should be done as four more patches to become
reviewable.
- One to use the CONVERT_STAT_BITS a lot more for the conversion
decision than before,
- another to allow the caller to tell gather_stats() to give up
early with the "search_only" bits,
- another to update the get_*_convert_stats() functions to use
get_convert_stats_sha1(), and then finally
- use the streaming interface when reading from blob and file.
or something line that.
Thanks.
^ permalink raw reply
* Re: Expanding Includes in .gitignore
From: Jeff King @ 2016-10-27 21:07 UTC (permalink / raw)
To: Aaron Pelly; +Cc: git
In-Reply-To: <20161027205508.vqw44zlbnqpj2cvd@sigill.intra.peff.net>
On Thu, Oct 27, 2016 at 04:55:08PM -0400, Jeff King wrote:
> On Fri, Oct 28, 2016 at 09:28:23AM +1300, Aaron Pelly wrote:
>
> > > - we parse possibly-hostile .gitignore files from cloned repositories.
> > > What happens when I include ask to include /etc/passwd? Probably
> > > nothing, but there are setups where it might matter (e.g., something
> > > like Travis that auto-builds untrusted repositories, and you could
> > > potentially leak the contents of files via error messages). It's
> > > nice to avoid the issue entirely.
> >
> > I understand the issue.
> >
> > It's not obvious to me how using a .d solves this problem though.
>
> It doesn't by itself. But we are worried only about tracked .gitignore
> files (recall that even repo-level files in $GIT_DIR/info are generated
> fresh by the clone process, and don't come from the remote). If we apply
> the feature only to core.excludeFile and $GIT_DIR/info/exclude, those
> are already under the user's control.
>
> It's true that we could make a similar exception for an "include"
> feature, and respect include directives only in those "safe" files.
> Somehow that seems more confusing to me, though, than doing adding the
> feature at the file level, as it introduces slightly varying syntax
> between the locations.
Actually, I suppose even a ".gitignore.d" inside the repo solves that
problem, too, because the repository is not specifying paths, only
content (it can specify symlinks outside the repository, but like other
parts of git, we should be careful not to follow them in that case).
However, as I said elsewhere, I'm not convinced this feature is all that
helpful for in-repository .gitignore files, and I think it does
introduce compatibility complications. People with older git will not
respect your .gitignore.d files. Whereas $GIT_DIR/info is purely a local
matter.
But perhaps there is a use case I'm missing.
-Peff
^ permalink raw reply
* Re: Expanding Includes in .gitignore
From: Jeff King @ 2016-10-27 21:04 UTC (permalink / raw)
To: Jacob Keller; +Cc: Aaron Pelly, Git mailing list
In-Reply-To: <CA+P7+xqmVM-bEc7sZcn+p3qhFUUJvC+rko7CYu+KgyTAeiOifw@mail.gmail.com>
On Thu, Oct 27, 2016 at 12:48:34PM -0700, Jacob Keller wrote:
> > I think the normal behavior in such "foo.d" directory is to just sort
> > the contents lexically and read them in order, as if they were all
> > concatenated together, and with no recursion. I.e., behave "as if" the
> > user had run "cat $dir/*".
>
> Yea, this is the normal behavior, and the user is expected to order
> their files lexically such as "00-name", "50-name" and so on. Pretty
> traditional for a lot of newer configurations.
One thing I will say about this approach is that you can implement it
without any changes in git by doing:
path=.git/info/exclude
cat $path.d/* >$path
and I have seen several config mechanisms basically do that (e.g.,
Debian packaging for a program that doesn't have its own ".d" mechanism,
but needs to grab config provided by several separate packages).
The reason to teach that trick to git is convenience; you don't have to
remember to build the master file from its parts because it's done
dynamically whenever git needs to look at it.
> One thing to keep in mind would be that we should make sure we can
> handle the .gitignore being a submodule or a git repository, so that
> users could just do something like
I'm not convinced this is needed for in-repo .gitignore files. The point
is that you are pulling together separate files that may be administered
independently. But git repositories inherently have a whole-project
view. I'm not sure that separate files buy you a lot there. And the
compatibility issues are more complicated.
I do agree that:
cd .git/info
git clone /my/exclude/repo exclude ;# or exclude.d
should work; ignoring dotfiles when reading the directory solves that,
and is a pretty standard solution.
-Peff
^ permalink raw reply
* Re: Expanding Includes in .gitignore
From: Aaron Pelly @ 2016-10-27 20:59 UTC (permalink / raw)
To: Jacob Keller, Jeff King; +Cc: Git mailing list
In-Reply-To: <CA+P7+xqmVM-bEc7sZcn+p3qhFUUJvC+rko7CYu+KgyTAeiOifw@mail.gmail.com>
On 28/10/16 08:48, Jacob Keller wrote:
> I would strongly prefer rc.d style directories either with a "if the
> .gitignore is a directory treat it like rc.d" or even "add support for
> .gitignore.d as well as .gitignore"
I think adding .gitignore.d shouldn't break existing systems, is
intuitive, and solves my issue.
Does git know when it is in a repo that is too new to comprehend?
My current thinking is that anywhere a .gitignore can go, so can a
.gitignore.d (named appropriately of course.) Any existing .gitignore
should take precedence to the result of parsing the directory.
I haven't looked at the implementation of precedence yet, but I'd be
surprised if the existing mechanism can't be employed.
> One thing to keep in mind would be that we should make sure we can
> handle the .gitignore being a submodule or a git repository, so that
> users could just do something like
>
> "git submodule add <repo> .gitignore and then track git ignore
> contents from a repository in a nice way.
>
> By this I mean that the reading of files in .gitignore directory
> should exclude reading .git or other hidden files in some documented
> manor so as to avoid problems when linking to a git directory for its
> contents.
Nice! I like this a lot.
^ 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