* Re: [PATCH 6/6] Automatically switch to crc32 checksum for index when it's too large
From: Dave Zarzycki @ 2012-02-06 9:07 UTC (permalink / raw)
To: Nguyen Thai Ngoc Duy; +Cc: git, Thomas Rast, Joshua Redstone
In-Reply-To: <CACsJy8Dv9fUzL3COZKVw_KR6aF20kHaw8M4CdBXJDA9H3fbxLw@mail.gmail.com>
On Feb 6, 2012, at 12:54 AM, Nguyen Thai Ngoc Duy <pclouds@gmail.com> wrote:
> 2012/2/6 Dave Zarzycki <zarzycki@apple.com>:
>> Which crc32 polynomial is being used? crc32c (a.k.a. Castagnoli)? It would be great if this were the same polynomial that Intel implements in hardware via SSE4.2.
>
> It's zlib's crc32.
That's too bad. Zlib uses crc32, not crc32c. The Intel instruction is crc32c and is 2-3 times faster than the best software based implementation.
http://www.strchr.com/crc32_popcnt
>
>> On Feb 5, 2012, at 9:48 PM, Nguyễn Thái Ngọc Duy <pclouds@gmail.com> wrote:
>>
>>> An experiment with -O3 is done on Intel D510@1.66GHz. At around 250k
>>> entries, index reading time exceeds 0.5s. Switching to crc32 brings it
>>> back lower than 0.2s.
>>>
>>> On 4M files index, reading time with SHA-1 takes ~8.4, crc32 2.8s.
> --
> Duy
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH 6/6] Automatically switch to crc32 checksum for index when it's too large
From: Dave Zarzycki @ 2012-02-06 8:50 UTC (permalink / raw)
To: Nguyễn Thái Ngọc Duy; +Cc: git, Thomas Rast, Joshua Redstone
In-Reply-To: <1328507319-24687-6-git-send-email-pclouds@gmail.com>
Which crc32 polynomial is being used? crc32c (a.k.a. Castagnoli)? It would be great if this were the same polynomial that Intel implements in hardware via SSE4.2.
On Feb 5, 2012, at 9:48 PM, Nguyễn Thái Ngọc Duy <pclouds@gmail.com> wrote:
> An experiment with -O3 is done on Intel D510@1.66GHz. At around 250k
> entries, index reading time exceeds 0.5s. Switching to crc32 brings it
> back lower than 0.2s.
>
> On 4M files index, reading time with SHA-1 takes ~8.4, crc32 2.8s.
^ permalink raw reply
* [PATCH 0/2] config includes 3: revenge of the killer includes
From: Jeff King @ 2012-02-06 9:53 UTC (permalink / raw)
To: git
So here's a third version of the config include patches that I think are
sane. For those of you who missed episode 2, our heroes decided to drop
the include-from-ref bits from the series, but otherwise incorporated
all suggestions. There was a surprise cliffhanger ending in which it was
discovered that suppressing duplicate include files was actually the
villain!
This version keeps a simple depth counter to stop cycles from causing
infinite loops, but otherwise allows multiple inclusion of files.
And now, the exciting conclusion...
[1/2]: docs: add a basic description of the config API
[2/2]: config: add include directive
-Peff
^ permalink raw reply
* [PATCH 1/2] docs: add a basic description of the config API
From: Jeff King @ 2012-02-06 9:53 UTC (permalink / raw)
To: git
In-Reply-To: <20120206095306.GA2404@sigill.intra.peff.net>
This wasn't documented at all; this is pretty bare-bones,
but it should at least give new git hackers a basic idea of
how the reading side works.
Signed-off-by: Jeff King <peff@peff.net>
---
Documentation/technical/api-config.txt | 101 ++++++++++++++++++++++++++++++++
1 files changed, 101 insertions(+), 0 deletions(-)
create mode 100644 Documentation/technical/api-config.txt
diff --git a/Documentation/technical/api-config.txt b/Documentation/technical/api-config.txt
new file mode 100644
index 0000000..f428c5c
--- /dev/null
+++ b/Documentation/technical/api-config.txt
@@ -0,0 +1,101 @@
+config API
+==========
+
+The config API gives callers a way to access git configuration files
+(and files which have the same syntax). See linkgit:git-config[1] for a
+discussion of the config file syntax.
+
+General Usage
+-------------
+
+Config files are parsed linearly, and each variable found is passed to a
+caller-provided callback function. The callback function is responsible
+for any actions to be taken on the config option, and is free to ignore
+some options (it is not uncommon for the configuration to be parsed
+several times during the run of a git program, with different callbacks
+picking out different variables useful to themselves).
+
+A config callback function takes three parameters:
+
+- the name of the parsed variable. This is in canonical "flat" form: the
+ section, subsection, and variable segments will be separated by dots,
+ and the section and variable segments will be all lowercase. E.g.,
+ `core.ignorecase`, `diff.SomeType.textconv`.
+
+- the value of the found variable, as a string. If the variable had no
+ value specified, the value will be NULL (typically this means it
+ should be interpreted as boolean true).
+
+- a void pointer passed in by the caller of the config API; this can
+ contain callback-specific data
+
+A config callback should return 0 for success, or -1 if the variable
+could not be parsed properly.
+
+Basic Config Querying
+---------------------
+
+Most programs will simply want to look up variables in all config files
+that git knows about, using the normal precedence rules. To do this,
+call `git_config` with a callback function and void data pointer.
+
+`git_config` will read all config sources in order of increasing
+priority. Thus a callback should typically overwrite previously-seen
+entries with new ones (e.g., if both the user-wide `~/.gitconfig` and
+repo-specific `.git/config` contain `color.ui`, the config machinery
+will first feed the user-wide one to the callback, and then the
+repo-specific one; by overwriting, the higher-priority repo-specific
+value is left at the end).
+
+There is a special version of `git_config` called `git_config_early`
+that takes an additional parameter to specify the repository config.
+This should be used early in a git program when the repository location
+has not yet been determined (and calling the usual lazy-evaluation
+lookup rules would yield an incorrect location).
+
+Reading Specific Files
+----------------------
+
+To read a specific file in git-config format, use
+`git_config_from_file`. This takes the same callback and data parameters
+as `git_config`.
+
+Value Parsing Helpers
+---------------------
+
+To aid in parsing string values, the config API provides callbacks with
+a number of helper functions, including:
+
+`git_config_int`::
+Parse the string to an integer, including unit factors. Dies on error;
+otherwise, returns the parsed result.
+
+`git_config_ulong`::
+Identical to `git_config_int`, but for unsigned longs.
+
+`git_config_bool`::
+Parse a string into a boolean value, respecting keywords like "true" and
+"false". Integer values are converted into true/false values (when they
+are non-zero or zero, respectively). Other values cause a die(). If
+parsing is successful, the return value is the result.
+
+`git_config_bool_or_int`::
+Same as `git_config_bool`, except that integers are returned as-is, and
+an `is_bool` flag is unset.
+
+`git_config_maybe_bool`::
+Same as `git_config_bool`, except that it returns -1 on error rather
+than dying.
+
+`git_config_string`::
+Allocates and copies the value string into the `dest` parameter; if no
+string is given, prints an error message and returns -1.
+
+`git_config_pathname`::
+Similar to `git_config_string`, but expands `~` or `~user` into the
+user's home directory when found at the beginning of the path.
+
+Writing Config Files
+--------------------
+
+TODO
--
1.7.9.rc1.29.g43677
^ permalink raw reply related
* Re: [PATCH 0/2] config includes, take 2
From: Michael Haggerty @ 2012-02-06 9:53 UTC (permalink / raw)
To: Jeff King; +Cc: git
In-Reply-To: <20120206062713.GA9699@sigill.intra.peff.net>
On 02/06/2012 07:27 AM, Jeff King wrote:
> 3. perform cycle and duplicate detection on included files
> [...]
>
> We first read the global config, which sets the value to "global", then
> includes foo, which overwrites it to "foo". Then we read the repo
> config, which sets the value to "repo", and then does _not_ actually
> read foo. Because git config is read linearly and later values tend to
> overwrite earlier ones, we would want to suppress the _first_ instance
> of a file, not the second (or really, the final if it is included many
> times). But that is impossible to do without generating a complete graph
> of includes and then pruning the early ones.
ISTM that the main goal was to prevent infinite recursion, not avoid a
little bit of redundant reading. If that is the goal, all you have to
record is the "include stack" context that caused the
currently-being-included file to be read and make sure that the
currently-being-included file didn't appear earlier on the stack. The
fact that the same file was included earlier (but not as part of the
same include chain) needn't be considered an error.
> [...]
> So I'm actually thinking I should drop the duplicate suppression and
> just do some sort of sanity check on include-depth to break cycles
> (i.e., just die because it's a crazy thing to do, and we are really just
> trying to tell the user their config is broken rather than go into an
> infinite loop). As a bonus, it makes the code much simpler, too.
I agree that it is more sensible to error out than to ignore a recursive
include.
It might nevertheless be useful to have an "include call stack"
available for generating output for errors that occur when parsing
config files; something like:
Error when parsing /home/me/bogusconfigfile line 75
which was included from /home/me/okconfigfile line 13
which was included from /home/me/.gitconfig line 22
or
Error: /home/me/bogusconfigfile included recursively by
/home/me/bogusfile2 line 75
which was included from /home/me/bogusconfigfile line 13
which was included from /home/me/.gitconfig line 22
OTOH such error stack dumps could be generated when unwinding the C call
stack without the need for a separate "include call stack".
However, one could even imagine a command like
$ git config --where-defined user.email
user.email defined by /home/me/myfile2 line 75
which was included from /home/me/myfile1 line 13
which was included from /home/me/.gitconfig line 22
user.email redefined by /home/me/project/.git/companyconfig line 8
which was included from /home/me/project/.git/config line 20
This usage could not be implemented using the C stack, because the C
stack cannot be unwound multiple times.
But these are all just wild ideas. I doubt that people's config files
will become so complicated that this much infrastructure is needed.
Michael
--
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/
^ permalink raw reply
* [PATCH 2/2] config: add include directive
From: Jeff King @ 2012-02-06 9:54 UTC (permalink / raw)
To: git
In-Reply-To: <20120206095306.GA2404@sigill.intra.peff.net>
It can be useful to split your ~/.gitconfig across multiple
files. For example, you might have a "main" file which is
used on many machines, but a small set of per-machine
tweaks. Or you may want to make some of your config public
(e.g., clever aliases) while keeping other data back (e.g.,
your name or other identifying information). Or you may want
to include a number of config options in some subset of your
repos without copying and pasting (e.g., you want to
reference them from the .git/config of participating repos).
This patch introduces an include directive for config files.
It looks like:
[include]
path = /path/to/file
This is syntactically backwards-compatible with existing git
config parsers (i.e., they will see it as another config
entry and ignore it unless you are looking up include.path).
The implementation provides a "git_config_include" callback
which wraps regular config callbacks. Callers can pass it to
git_config_from_file, and it will transparently follow any
include directives, passing all of the discovered options to
the real callback.
Include directives are turned on automatically for "regular"
git config parsing. This includes calls to git_config, as
well as calls to the "git config" program that do not
specify a single file (e.g., using "-f", "--global", etc).
They are not turned on in other cases, including:
1. Parsing of other config-like files, like .gitmodules.
There isn't a real need, and I'd rather be conservative
and avoid unnecessary incompatibility or confusion.
2. Reading single files via "git config". This is for two
reasons:
a. backwards compatibility with scripts looking at
config-like files.
b. inspection of a specific file probably means you
care about just what's in that file, not a general
lookup for "do we have this value anywhere at
all". If that is not the case, the caller can
always specify "--includes".
3. Writing files via "git config"; we want to treat
include.* variables as literal items to be copied (or
modified), and not expand them. So "git config
--unset-all foo.bar" would operate _only_ on
.git/config, not any of its included files (just as it
also does not operate on ~/.gitconfig).
Signed-off-by: Jeff King <peff@peff.net>
---
Documentation/config.txt | 15 ++++
Documentation/git-config.txt | 5 +
Documentation/technical/api-config.txt | 22 ++++++
builtin/config.c | 31 ++++++--
cache.h | 8 ++
config.c | 69 +++++++++++++++++-
t/t1305-config-include.sh | 126 ++++++++++++++++++++++++++++++++
7 files changed, 268 insertions(+), 8 deletions(-)
create mode 100755 t/t1305-config-include.sh
diff --git a/Documentation/config.txt b/Documentation/config.txt
index abeb82b..e55dae1 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -84,6 +84,17 @@ customary UNIX fashion.
Some variables may require a special value format.
+Includes
+~~~~~~~~
+
+You can include one config file from another by setting the special
+`include.path` variable to the name of the file to be included. The
+included file is expanded immediately, as if its contents had been
+found at the location of the include directive. If the value of the
+`include.path` variable is a relative path, the path is considered to be
+relative to the configuration file in which the include directive was
+found. See below for examples.
+
Example
~~~~~~~
@@ -106,6 +117,10 @@ Example
gitProxy="ssh" for "kernel.org"
gitProxy=default-proxy ; for the rest
+ [include]
+ path = /path/to/foo.inc ; include by absolute path
+ path = foo ; expand "foo" relative to the current file
+
Variables
~~~~~~~~~
diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt
index e7ecf5d..aa8303b 100644
--- a/Documentation/git-config.txt
+++ b/Documentation/git-config.txt
@@ -178,6 +178,11 @@ See also <<FILES>>.
Opens an editor to modify the specified config file; either
'--system', '--global', or repository (default).
+--includes::
+--no-includes::
+ Respect `include.*` directives in config files when looking up
+ values. Defaults to on.
+
[[FILES]]
FILES
-----
diff --git a/Documentation/technical/api-config.txt b/Documentation/technical/api-config.txt
index f428c5c..c60b6b3 100644
--- a/Documentation/technical/api-config.txt
+++ b/Documentation/technical/api-config.txt
@@ -95,6 +95,28 @@ string is given, prints an error message and returns -1.
Similar to `git_config_string`, but expands `~` or `~user` into the
user's home directory when found at the beginning of the path.
+Include Directives
+------------------
+
+By default, the config parser does not respect include directives.
+However, a caller can use the special `git_config_include` wrapper
+callback to support them. To do so, you simply wrap your "real" callback
+function and data pointer in a `struct config_include_data`, and pass
+the wrapper to the regular config-reading functions. For example:
+
+-------------------------------------------
+int read_file_with_include(const char *file, config_fn_t fn, void *data)
+{
+ struct config_include_data inc = CONFIG_INCLUDE_INIT;
+ inc.fn = fn;
+ inc.data = data;
+ return git_config_from_file(git_config_include, file, &inc);
+}
+-------------------------------------------
+
+`git_config` respects includes automatically. The lower-level
+`git_config_from_file` does not.
+
Writing Config Files
--------------------
diff --git a/builtin/config.c b/builtin/config.c
index d35c06a..09bf778 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -25,6 +25,7 @@ static const char *given_config_file;
static int actions, types;
static const char *get_color_slot, *get_colorbool_slot;
static int end_null;
+static int respect_includes = -1;
#define ACTION_GET (1<<0)
#define ACTION_GET_ALL (1<<1)
@@ -74,6 +75,7 @@ static struct option builtin_config_options[] = {
OPT_BIT(0, "path", &types, "value is a path (file or directory name)", TYPE_PATH),
OPT_GROUP("Other"),
OPT_BOOLEAN('z', "null", &end_null, "terminate values with NUL byte"),
+ OPT_BOOL(0, "includes", &respect_includes, "respect include directives on lookup"),
OPT_END(),
};
@@ -161,6 +163,9 @@ static int get_value(const char *key_, const char *regex_)
int ret = -1;
char *global = NULL, *repo_config = NULL;
const char *system_wide = NULL, *local;
+ struct config_include_data inc = CONFIG_INCLUDE_INIT;
+ config_fn_t fn;
+ void *data;
local = config_exclusive_filename;
if (!local) {
@@ -213,19 +218,28 @@ static int get_value(const char *key_, const char *regex_)
}
}
+ fn = show_config;
+ data = NULL;
+ if (respect_includes) {
+ inc.fn = fn;
+ inc.data = data;
+ fn = git_config_include;
+ data = &inc;
+ }
+
if (do_all && system_wide)
- git_config_from_file(show_config, system_wide, NULL);
+ git_config_from_file(fn, system_wide, data);
if (do_all && global)
- git_config_from_file(show_config, global, NULL);
+ git_config_from_file(fn, global, data);
if (do_all)
- git_config_from_file(show_config, local, NULL);
- git_config_from_parameters(show_config, NULL);
+ git_config_from_file(fn, local, data);
+ git_config_from_parameters(fn, data);
if (!do_all && !seen)
- git_config_from_file(show_config, local, NULL);
+ git_config_from_file(fn, local, data);
if (!do_all && !seen && global)
- git_config_from_file(show_config, global, NULL);
+ git_config_from_file(fn, global, data);
if (!do_all && !seen && system_wide)
- git_config_from_file(show_config, system_wide, NULL);
+ git_config_from_file(fn, system_wide, data);
free(key);
if (regexp) {
@@ -384,6 +398,9 @@ int cmd_config(int argc, const char **argv, const char *prefix)
config_exclusive_filename = given_config_file;
}
+ if (respect_includes == -1)
+ respect_includes = !config_exclusive_filename;
+
if (end_null) {
term = '\0';
delim = '\n';
diff --git a/cache.h b/cache.h
index 9bd8c2d..7cf6dc1 100644
--- a/cache.h
+++ b/cache.h
@@ -1139,6 +1139,14 @@ extern const char *get_commit_output_encoding(void);
extern int git_config_parse_parameter(const char *, config_fn_t fn, void *data);
+struct config_include_data {
+ int depth;
+ config_fn_t fn;
+ void *data;
+};
+#define CONFIG_INCLUDE_INIT { 0 }
+extern int git_config_include(const char *name, const char *value, void *data);
+
extern const char *config_exclusive_filename;
#define MAX_GITNAME (1000)
diff --git a/config.c b/config.c
index 40f9c6d..e3fcf75 100644
--- a/config.c
+++ b/config.c
@@ -28,6 +28,69 @@ static int zlib_compression_seen;
const char *config_exclusive_filename = NULL;
+#define MAX_INCLUDE_DEPTH 10
+static const char include_depth_advice[] =
+"exceeded maximum include depth (%d) while including\n"
+" %s\n"
+"from\n"
+" %s\n"
+"Do you have circular includes?";
+static int handle_path_include(const char *path, struct config_include_data *inc)
+{
+ int ret = 0;
+ struct strbuf buf = STRBUF_INIT;
+
+ /*
+ * Use an absolute path as-is, but interpret relative paths
+ * based on the including config file.
+ */
+ if (!is_absolute_path(path)) {
+ char *slash;
+
+ if (!cf || !cf->name)
+ return error("relative config includes must come from files");
+
+ slash = find_last_dir_sep(cf->name);
+ if (slash)
+ strbuf_add(&buf, cf->name, slash - cf->name + 1);
+ strbuf_addstr(&buf, path);
+ path = buf.buf;
+ }
+
+ if (!access(path, R_OK)) {
+ if (++inc->depth > MAX_INCLUDE_DEPTH)
+ die(include_depth_advice, MAX_INCLUDE_DEPTH, path,
+ cf && cf->name ? cf->name : "the command line");
+ ret = git_config_from_file(git_config_include, path, inc);
+ inc->depth--;
+ }
+ strbuf_release(&buf);
+ return ret;
+}
+
+int git_config_include(const char *var, const char *value, void *data)
+{
+ struct config_include_data *inc = data;
+ const char *type;
+ int ret;
+
+ /*
+ * Pass along all values, including "include" directives; this makes it
+ * possible to query information on the includes themselves.
+ */
+ ret = inc->fn(var, value, inc->data);
+ if (ret < 0)
+ return ret;
+
+ type = skip_prefix(var, "include.");
+ if (!type)
+ return ret;
+
+ if (!strcmp(type, "path"))
+ ret = handle_path_include(value, inc);
+ return ret;
+}
+
static void lowercase(char *p)
{
for (; *p; p++)
@@ -921,9 +984,13 @@ int git_config(config_fn_t fn, void *data)
{
char *repo_config = NULL;
int ret;
+ struct config_include_data inc = CONFIG_INCLUDE_INIT;
+
+ inc.fn = fn;
+ inc.data = data;
repo_config = git_pathdup("config");
- ret = git_config_early(fn, data, repo_config);
+ ret = git_config_early(git_config_include, &inc, repo_config);
if (repo_config)
free(repo_config);
return ret;
diff --git a/t/t1305-config-include.sh b/t/t1305-config-include.sh
new file mode 100755
index 0000000..0a27ec4
--- /dev/null
+++ b/t/t1305-config-include.sh
@@ -0,0 +1,126 @@
+#!/bin/sh
+
+test_description='test config file include directives'
+. ./test-lib.sh
+
+test_expect_success 'include file by absolute path' '
+ echo "[test]one = 1" >one &&
+ echo "[include]path = \"$PWD/one\"" >.gitconfig &&
+ echo 1 >expect &&
+ git config test.one >actual &&
+ test_cmp expect actual
+'
+
+test_expect_success 'include file by relative path' '
+ echo "[test]one = 1" >one &&
+ echo "[include]path = one" >.gitconfig &&
+ echo 1 >expect &&
+ git config test.one >actual &&
+ test_cmp expect actual
+'
+
+test_expect_success 'chained relative paths' '
+ mkdir subdir &&
+ echo "[test]three = 3" >subdir/three &&
+ echo "[include]path = three" >subdir/two &&
+ echo "[include]path = subdir/two" >.gitconfig &&
+ echo 3 >expect &&
+ git config test.three >actual &&
+ test_cmp expect actual
+'
+
+test_expect_success 'include options can still be examined' '
+ echo "[test]one = 1" >one &&
+ echo "[include]path = one" >.gitconfig &&
+ echo one >expect &&
+ git config include.path >actual &&
+ test_cmp expect actual
+'
+
+test_expect_success 'listing includes option and expansion' '
+ echo "[test]one = 1" >one &&
+ echo "[include]path = one" >.gitconfig &&
+ cat >expect <<-\EOF &&
+ include.path=one
+ test.one=1
+ EOF
+ git config --list >actual.full &&
+ grep -v ^core actual.full >actual &&
+ test_cmp expect actual
+'
+
+test_expect_success 'single file lookup does not expand includes by default' '
+ echo "[test]one = 1" >one &&
+ echo "[include]path = one" >.gitconfig &&
+ test_must_fail git config -f .gitconfig test.one &&
+ test_must_fail git config --global test.one &&
+ echo 1 >expect &&
+ git config --includes -f .gitconfig test.one >actual &&
+ test_cmp expect actual
+'
+
+test_expect_success 'writing config file does not expand includes' '
+ echo "[test]one = 1" >one &&
+ echo "[include]path = one" >.gitconfig &&
+ git config test.two 2 &&
+ echo 2 >expect &&
+ git config --no-includes test.two >actual &&
+ test_cmp expect actual &&
+ test_must_fail git config --no-includes test.one
+'
+
+test_expect_success 'config modification does not affect includes' '
+ echo "[test]one = 1" >one &&
+ echo "[include]path = one" >.gitconfig &&
+ git config test.one 2 &&
+ echo 1 >expect &&
+ git config -f one test.one >actual &&
+ test_cmp expect actual &&
+ cat >expect <<-\EOF &&
+ 1
+ 2
+ EOF
+ git config --get-all test.one >actual &&
+ test_cmp expect actual
+'
+
+test_expect_success 'missing include files are ignored' '
+ cat >.gitconfig <<-\EOF &&
+ [include]path = foo
+ [test]value = yes
+ EOF
+ echo yes >expect &&
+ git config test.value >actual &&
+ test_cmp expect actual
+'
+
+test_expect_success 'absolute includes from command line work' '
+ echo "[test]one = 1" >one &&
+ echo 1 >expect &&
+ git -c include.path="$PWD/one" config test.one >actual &&
+ test_cmp expect actual
+'
+
+test_expect_success 'relative includes from command line fail' '
+ echo "[test]one = 1" >one &&
+ test_must_fail git -c include.path=one config test.one
+'
+
+test_expect_success 'include cycles are detected' '
+ cat >.gitconfig <<-\EOF &&
+ [test]value = gitconfig
+ [include]path = cycle
+ EOF
+ cat >cycle <<-\EOF &&
+ [test]value = cycle
+ [include]path = .gitconfig
+ EOF
+ cat >expect <<-\EOF &&
+ gitconfig
+ cycle
+ EOF
+ test_must_fail git config --get-all test.value 2>stderr &&
+ grep "exceeded maximum include depth" stderr
+'
+
+test_done
--
1.7.9.rc1.29.g43677
^ permalink raw reply related
* Re: [PATCH 0/2] config includes, take 2
From: Jeff King @ 2012-02-06 10:06 UTC (permalink / raw)
To: Michael Haggerty; +Cc: git
In-Reply-To: <4F2FA330.7020803@alum.mit.edu>
On Mon, Feb 06, 2012 at 10:53:52AM +0100, Michael Haggerty wrote:
> ISTM that the main goal was to prevent infinite recursion, not avoid a
> little bit of redundant reading.
It was both, actually. There was a sense that we should not end up with
duplicate entries by reading the same file twice. However, entries which
actually create lists (and could therefore create duplicates) are by far
the minority compared to entries which overwrite. And it is the
overwrite-style entries which are harmed by suppressing duplicates.
> If that is the goal, all you have to record is the "include stack"
> context that caused the currently-being-included file to be read and
> make sure that the currently-being-included file didn't appear earlier
> on the stack. The fact that the same file was included earlier (but
> not as part of the same include chain) needn't be considered an error.
I considered this, but decided the complexity wasn't worth it,
especially because getting it accurate means cooperation from
git_config_from_file, which otherwise doesn't know or care about this
mechanism. Instead I keep a simple depth counter and barf at a
reasonable maximum, printing the "from" and "to" files. Yes, it's not
nearly as elegant as keeping the whole stack, but I really don't expect
people to have complex super-deep includes, nor for it to be difficult
to hunt down the cause of a cycle.
Maybe that is short-sighted or lazy of me, but I'd just really be
surprised if people ever went more than 1 or 2 layers deep, or if they
actually ended up with a cycle at all.
There is a stack kept already by git_config_from_file, but it doesn't
respect the call-chain (so if I start a new git_config() call from
inside a callback, it is still part of the same stack).
We could possibly put a marker in the stack for that situation, and then
it would be usable for include cycle-detection.
> However, one could even imagine a command like
>
> $ git config --where-defined user.email
> user.email defined by /home/me/myfile2 line 75
> which was included from /home/me/myfile1 line 13
> which was included from /home/me/.gitconfig line 22
> user.email redefined by /home/me/project/.git/companyconfig line 8
> which was included from /home/me/project/.git/config line 20
You can already implement this with the existing stack by providing a
suitable callback (since its your callback, you'd know that there was no
recursion of git_config, and therefore the stack refers only to a single
set of includes).
-Peff
^ permalink raw reply
* Re: [PATCH 0/2] config includes, take 2
From: Jeff King @ 2012-02-06 10:16 UTC (permalink / raw)
To: Michael Haggerty; +Cc: git
In-Reply-To: <20120206100622.GC4300@sigill.intra.peff.net>
On Mon, Feb 06, 2012 at 05:06:22AM -0500, Jeff King wrote:
> > If that is the goal, all you have to record is the "include stack"
> > context that caused the currently-being-included file to be read and
> > make sure that the currently-being-included file didn't appear earlier
> > on the stack. The fact that the same file was included earlier (but
> > not as part of the same include chain) needn't be considered an error.
>
> I considered this, but decided the complexity wasn't worth it,
> especially because getting it accurate means cooperation from
> git_config_from_file, which otherwise doesn't know or care about this
> mechanism. Instead I keep a simple depth counter and barf at a
> reasonable maximum, printing the "from" and "to" files. Yes, it's not
> nearly as elegant as keeping the whole stack, but I really don't expect
> people to have complex super-deep includes, nor for it to be difficult
> to hunt down the cause of a cycle.
Oh, one other thing: to detect cycles, you have to use a canonical
version of the filename for comparisons. That makes the existing stack
that git_config_from_file keeps useless. Using real_path is hard,
because it will die() if some path components don't exist. Using
absolute_path is a reasonable compromise, but it can actually miss some
cycles if they involve symbolic links.
Not insurmountable if you can accept teaching git_config_from_file to
real_path() each file it reads. But again, it ends up getting complex
for IMHO not much gain. A stupid depth counter can't show you a stack
trace (and it may have false positives), but it's dirt simple and
probably good enough.
-Peff
^ permalink raw reply
* [PATCH 0/4] Deprecate "not allow as-is commit with i-t-a entries"
From: Nguyễn Thái Ngọc Duy @ 2012-02-06 10:57 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jonathan Nieder,
Nguyễn Thái Ngọc Duy
By the end of this series "git commit -m foo" will proceed regardless
intent-to-add (aka "git add -N") entries. commit.ignoreIntentToAdd is
used as transitiono config key.
The plan is in 2/4. I set 1.8.0 as the first deprecation date, but of
course it's just a suggestion. The second deprecation date might be
1.8.5, long enough for users to adapt to the new behavior.
..or we switch back half way because we find current behavior does
make more sense.
Nguyễn Thái Ngọc Duy (4):
cache-tree: update API to take abitrary flags
commit: introduce a config key to allow as-is commit with i-t-a
entries
commit: turn commit.ignoreIntentToAdd to true by default
commit: remove commit.ignoreIntentToAdd, assume it's always true
Documentation/git-add.txt | 12 ++++++++++--
builtin/commit.c | 9 ++++++---
cache-tree.c | 35 +++++++++++++++++------------------
cache-tree.h | 5 ++++-
merge-recursive.c | 2 +-
t/t2203-add-intent.sh | 21 ++++++++++++++++++++-
test-dump-cache-tree.c | 2 +-
7 files changed, 59 insertions(+), 27 deletions(-)
--
1.7.8.36.g69ee2
^ permalink raw reply
* [PATCH 1/4] cache-tree: update API to take abitrary flags
From: Nguyễn Thái Ngọc Duy @ 2012-02-06 10:57 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jonathan Nieder,
Nguyễn Thái Ngọc Duy
In-Reply-To: <1328525855-2547-1-git-send-email-pclouds@gmail.com>
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
builtin/commit.c | 4 ++--
cache-tree.c | 27 ++++++++++++---------------
cache-tree.h | 4 +++-
merge-recursive.c | 2 +-
test-dump-cache-tree.c | 2 +-
5 files changed, 19 insertions(+), 20 deletions(-)
diff --git a/builtin/commit.c b/builtin/commit.c
index eba1377..bf42bb3 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -400,7 +400,7 @@ static char *prepare_index(int argc, const char **argv, const char *prefix,
fd = hold_locked_index(&index_lock, 1);
add_files_to_cache(also ? prefix : NULL, pathspec, 0);
refresh_cache_or_die(refresh_flags);
- update_main_cache_tree(1);
+ update_main_cache_tree(WRITE_TREE_SILENT);
if (write_cache(fd, active_cache, active_nr) ||
close_lock_file(&index_lock))
die(_("unable to write new_index file"));
@@ -421,7 +421,7 @@ static char *prepare_index(int argc, const char **argv, const char *prefix,
fd = hold_locked_index(&index_lock, 1);
refresh_cache_or_die(refresh_flags);
if (active_cache_changed) {
- update_main_cache_tree(1);
+ update_main_cache_tree(WRITE_TREE_SILENT);
if (write_cache(fd, active_cache, active_nr) ||
commit_locked_index(&index_lock))
die(_("unable to write new_index file"));
diff --git a/cache-tree.c b/cache-tree.c
index 8de3959..16355d6 100644
--- a/cache-tree.c
+++ b/cache-tree.c
@@ -150,9 +150,10 @@ void cache_tree_invalidate_path(struct cache_tree *it, const char *path)
}
static int verify_cache(struct cache_entry **cache,
- int entries, int silent)
+ int entries, int flags)
{
int i, funny;
+ int silent = flags & WRITE_TREE_SILENT;
/* Verify that the tree is merged */
funny = 0;
@@ -241,10 +242,11 @@ static int update_one(struct cache_tree *it,
int entries,
const char *base,
int baselen,
- int missing_ok,
- int dryrun)
+ int flags)
{
struct strbuf buffer;
+ int missing_ok = flags & WRITE_TREE_MISSING_OK;
+ int dryrun = flags & WRITE_TREE_DRY_RUN;
int i;
if (0 <= it->entry_count && has_sha1_file(it->sha1))
@@ -288,8 +290,7 @@ static int update_one(struct cache_tree *it,
cache + i, entries - i,
path,
baselen + sublen + 1,
- missing_ok,
- dryrun);
+ flags);
if (subcnt < 0)
return subcnt;
i += subcnt - 1;
@@ -371,15 +372,13 @@ static int update_one(struct cache_tree *it,
int cache_tree_update(struct cache_tree *it,
struct cache_entry **cache,
int entries,
- int missing_ok,
- int dryrun,
- int silent)
+ int flags)
{
int i;
- i = verify_cache(cache, entries, silent);
+ i = verify_cache(cache, entries, flags);
if (i)
return i;
- i = update_one(it, cache, entries, "", 0, missing_ok, dryrun);
+ i = update_one(it, cache, entries, "", 0, flags);
if (i < 0)
return i;
return 0;
@@ -572,11 +571,9 @@ int write_cache_as_tree(unsigned char *sha1, int flags, const char *prefix)
was_valid = cache_tree_fully_valid(active_cache_tree);
if (!was_valid) {
- int missing_ok = flags & WRITE_TREE_MISSING_OK;
-
if (cache_tree_update(active_cache_tree,
active_cache, active_nr,
- missing_ok, 0, 0) < 0)
+ flags) < 0)
return WRITE_TREE_UNMERGED_INDEX;
if (0 <= newfd) {
if (!write_cache(newfd, active_cache, active_nr) &&
@@ -672,10 +669,10 @@ int cache_tree_matches_traversal(struct cache_tree *root,
return 0;
}
-int update_main_cache_tree (int silent)
+int update_main_cache_tree (int flags)
{
if (!the_index.cache_tree)
the_index.cache_tree = cache_tree();
return cache_tree_update(the_index.cache_tree,
- the_index.cache, the_index.cache_nr, 0, 0, silent);
+ the_index.cache, the_index.cache_nr, flags);
}
diff --git a/cache-tree.h b/cache-tree.h
index 0ec0b2a..d8cb2e9 100644
--- a/cache-tree.h
+++ b/cache-tree.h
@@ -29,13 +29,15 @@ void cache_tree_write(struct strbuf *, struct cache_tree *root);
struct cache_tree *cache_tree_read(const char *buffer, unsigned long size);
int cache_tree_fully_valid(struct cache_tree *);
-int cache_tree_update(struct cache_tree *, struct cache_entry **, int, int, int, int);
+int cache_tree_update(struct cache_tree *, struct cache_entry **, int, int);
int update_main_cache_tree(int);
/* bitmasks to write_cache_as_tree flags */
#define WRITE_TREE_MISSING_OK 1
#define WRITE_TREE_IGNORE_CACHE_TREE 2
+#define WRITE_TREE_DRY_RUN 4
+#define WRITE_TREE_SILENT 8
/* error return codes */
#define WRITE_TREE_UNREADABLE_INDEX (-1)
diff --git a/merge-recursive.c b/merge-recursive.c
index d83cd6c..6479a60 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -264,7 +264,7 @@ struct tree *write_tree_from_memory(struct merge_options *o)
if (!cache_tree_fully_valid(active_cache_tree) &&
cache_tree_update(active_cache_tree,
- active_cache, active_nr, 0, 0, 0) < 0)
+ active_cache, active_nr, 0) < 0)
die("error building trees");
result = lookup_tree(active_cache_tree->sha1);
diff --git a/test-dump-cache-tree.c b/test-dump-cache-tree.c
index e6c2923..a6ffdf3 100644
--- a/test-dump-cache-tree.c
+++ b/test-dump-cache-tree.c
@@ -59,6 +59,6 @@ int main(int ac, char **av)
struct cache_tree *another = cache_tree();
if (read_cache() < 0)
die("unable to read index file");
- cache_tree_update(another, active_cache, active_nr, 0, 1, 0);
+ cache_tree_update(another, active_cache, active_nr, WRITE_TREE_DRY_RUN);
return dump_cache_tree(active_cache_tree, another, "");
}
--
1.7.8.36.g69ee2
^ permalink raw reply related
* [PATCH 2/4] commit: introduce a config key to allow as-is commit with i-t-a entries
From: Nguyễn Thái Ngọc Duy @ 2012-02-06 10:57 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jonathan Nieder,
Nguyễn Thái Ngọc Duy
In-Reply-To: <1328525855-2547-1-git-send-email-pclouds@gmail.com>
This is the start of deprecating current commit behavior (do not allow
commit as-is when there are i-t-a entries in index). Users will be
annoyed by a warning saying the behavior will change in 1.8.0 and asked
to set commit.ignoreIntentToAdd properly.
The plan after that is:
- Keep it this way until 1.8.0
- In 1.8.0, we consider the lack of commit.ignoreIntentToAdd means
"true". A deprecation date is set. The warning message is updated
stating that from that day, this config key will be ignored. The
behavior will be as if this config key is always "true".
- In the chosen release, remove support for this config key.
WRITE_TREE_IGNORE_INTENT_TO_ADD is set unconditionally.
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
Documentation/config.txt | 10 ++++++++++
Documentation/git-add.txt | 12 ++++++++++--
builtin/commit.c | 28 +++++++++++++++++++++++++---
cache-tree.c | 8 +++++---
cache-tree.h | 1 +
t/t2203-add-intent.sh | 21 ++++++++++++++++++++-
6 files changed, 71 insertions(+), 9 deletions(-)
diff --git a/Documentation/config.txt b/Documentation/config.txt
index abeb82b..6ec81a8 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -831,6 +831,16 @@ commit.template::
"{tilde}/" is expanded to the value of `$HOME` and "{tilde}user/" to the
specified user's home directory.
+commit.ignoreIntentToAdd::
+ Allow to commit the index as-is even if there are
+ intent-to-add entries (see option `-N` in linkgit:git-add[1])
+ in index. Set to `false` to disallow commit in this acase, or `true`
+ to allow it.
++
+By default, `git commit` refuses to commit as-is when you have intent-to-add
+entries. This will change in 1.8.0, where `git commit` allows it. If you
+prefer current behavior, please set it to `false`.
+
credential.helper::
Specify an external helper to be called when a username or
password credential is needed; the helper may consult external
diff --git a/Documentation/git-add.txt b/Documentation/git-add.txt
index 9c1d395..ec548ea 100644
--- a/Documentation/git-add.txt
+++ b/Documentation/git-add.txt
@@ -123,8 +123,16 @@ subdirectories.
Record only the fact that the path will be added later. An entry
for the path is placed in the index with no content. This is
useful for, among other things, showing the unstaged content of
- such files with `git diff` and committing them with `git commit
- -a`.
+ such files with `git diff`.
++
+Paths added with this option have intent-to-add flag in index. The
+flag is removed once real content is added or updated. By default you
+cannot commit the index as-is from until this flag is removed from all
+entries (i.e. all entries have real content). See commit.ignoreIntentToAdd
+regardless the flag.
++
+Committing with `git commit -a` or with selected paths works
+regardless the config key and the flag.
--refresh::
Don't add the file(s), but only refresh their stat()
diff --git a/builtin/commit.c b/builtin/commit.c
index bf42bb3..af3250c 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -86,6 +86,7 @@ static int all, also, interactive, patch_interactive, only, amend, signoff;
static int edit_flag = -1; /* unspecified */
static int quiet, verbose, no_verify, allow_empty, dry_run, renew_authorship;
static int no_post_rewrite, allow_empty_message;
+static int cache_tree_flags;
static char *untracked_files_arg, *force_date, *ignore_submodule_arg;
static char *sign_commit;
@@ -117,6 +118,8 @@ static enum {
} status_format = STATUS_FORMAT_LONG;
static int status_show_branch;
+static int set_commit_ignoreintenttoadd;
+
static int opt_parse_m(const struct option *opt, const char *arg, int unset)
{
struct strbuf *buf = opt->value;
@@ -400,7 +403,7 @@ static char *prepare_index(int argc, const char **argv, const char *prefix,
fd = hold_locked_index(&index_lock, 1);
add_files_to_cache(also ? prefix : NULL, pathspec, 0);
refresh_cache_or_die(refresh_flags);
- update_main_cache_tree(WRITE_TREE_SILENT);
+ update_main_cache_tree(cache_tree_flags | WRITE_TREE_SILENT);
if (write_cache(fd, active_cache, active_nr) ||
close_lock_file(&index_lock))
die(_("unable to write new_index file"));
@@ -420,8 +423,20 @@ static char *prepare_index(int argc, const char **argv, const char *prefix,
if (!pathspec || !*pathspec) {
fd = hold_locked_index(&index_lock, 1);
refresh_cache_or_die(refresh_flags);
+ if (!set_commit_ignoreintenttoadd) {
+ int i;
+ for (i = 0; i < active_nr; i++)
+ if (active_cache[i]->ce_flags & CE_INTENT_TO_ADD)
+ break;
+ if (i < active_nr)
+ warning(_("You are committing as-is with intent-to-add entries as the result of\n"
+ "\"git add -N\". Git currently forbids this case. This will change in\n"
+ "1.8.0 where intent-to-add entries are simply ignored when committing\n"
+ "as-is. Please look up document and set commit.ignoreIntentToAdd\n"
+ "properly to stop this warning."));
+ }
if (active_cache_changed) {
- update_main_cache_tree(WRITE_TREE_SILENT);
+ update_main_cache_tree(cache_tree_flags | WRITE_TREE_SILENT);
if (write_cache(fd, active_cache, active_nr) ||
commit_locked_index(&index_lock))
die(_("unable to write new_index file"));
@@ -870,7 +885,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
*/
discard_cache();
read_cache_from(index_file);
- if (update_main_cache_tree(0)) {
+ if (update_main_cache_tree(cache_tree_flags)) {
error(_("Error building trees"));
return 0;
}
@@ -1338,6 +1353,13 @@ static int git_commit_config(const char *k, const char *v, void *cb)
include_status = git_config_bool(k, v);
return 0;
}
+ if (!strcmp(k, "commit.ignoreintenttoadd")) {
+ set_commit_ignoreintenttoadd = 1;
+ if (git_config_bool(k, v))
+ cache_tree_flags |= WRITE_TREE_IGNORE_INTENT_TO_ADD;
+ else
+ cache_tree_flags &= ~WRITE_TREE_IGNORE_INTENT_TO_ADD;
+ }
status = git_gpg_config(k, v, NULL);
if (status)
diff --git a/cache-tree.c b/cache-tree.c
index 16355d6..d0be159 100644
--- a/cache-tree.c
+++ b/cache-tree.c
@@ -159,7 +159,9 @@ static int verify_cache(struct cache_entry **cache,
funny = 0;
for (i = 0; i < entries; i++) {
struct cache_entry *ce = cache[i];
- if (ce_stage(ce) || (ce->ce_flags & CE_INTENT_TO_ADD)) {
+ if (ce_stage(ce) ||
+ ((flags & WRITE_TREE_IGNORE_INTENT_TO_ADD) == 0 &&
+ (ce->ce_flags & CE_INTENT_TO_ADD))) {
if (silent)
return -1;
if (10 < ++funny) {
@@ -339,8 +341,8 @@ static int update_one(struct cache_tree *it,
mode, sha1_to_hex(sha1), entlen+baselen, path);
}
- if (ce->ce_flags & CE_REMOVE)
- continue; /* entry being removed */
+ if (ce->ce_flags & (CE_REMOVE | CE_INTENT_TO_ADD))
+ continue; /* entry being removed or placeholder */
strbuf_grow(&buffer, entlen + 100);
strbuf_addf(&buffer, "%o %.*s%c", mode, entlen, path + baselen, '\0');
diff --git a/cache-tree.h b/cache-tree.h
index d8cb2e9..af3b917 100644
--- a/cache-tree.h
+++ b/cache-tree.h
@@ -38,6 +38,7 @@ int update_main_cache_tree(int);
#define WRITE_TREE_IGNORE_CACHE_TREE 2
#define WRITE_TREE_DRY_RUN 4
#define WRITE_TREE_SILENT 8
+#define WRITE_TREE_IGNORE_INTENT_TO_ADD 16
/* error return codes */
#define WRITE_TREE_UNREADABLE_INDEX (-1)
diff --git a/t/t2203-add-intent.sh b/t/t2203-add-intent.sh
index 58a3299..88a508e 100755
--- a/t/t2203-add-intent.sh
+++ b/t/t2203-add-intent.sh
@@ -41,7 +41,26 @@ test_expect_success 'cannot commit with i-t-a entry' '
echo frotz >nitfol &&
git add rezrov &&
git add -N nitfol &&
- test_must_fail git commit
+ test_must_fail git commit -minitial
+'
+
+test_expect_success 'can commit tree with i-t-a entry' '
+ git reset --hard &&
+ echo xyzzy >rezrov &&
+ echo frotz >nitfol &&
+ git add rezrov &&
+ git add -N nitfol &&
+ git config commit.ignoreIntentToAdd true &&
+ git commit -m initial &&
+ git ls-tree -r HEAD >actual &&
+ cat >expected <<EOF &&
+100644 blob ce013625030ba8dba906f756967f9e9ca394464a elif
+100644 blob ce013625030ba8dba906f756967f9e9ca394464a file
+100644 blob cf7711b63209d0dbc2d030f7fe3513745a9880e4 rezrov
+EOF
+ test_cmp expected actual &&
+ git config commit.ignoreIntentToAdd false &&
+ git reset HEAD^
'
test_expect_success 'can commit with an unrelated i-t-a entry in index' '
--
1.7.8.36.g69ee2
^ permalink raw reply related
* [PATCH 3/4] commit: turn commit.ignoreIntentToAdd to true by default
From: Nguyễn Thái Ngọc Duy @ 2012-02-06 10:57 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jonathan Nieder,
Nguyễn Thái Ngọc Duy
In-Reply-To: <1328525855-2547-1-git-send-email-pclouds@gmail.com>
From now on, those who has not set commit.ignoreIntentToAdd can commit
as-is even if there are intent-to-add entries. Users are advised/annoyed
to switch to new behavior.
Support for "commit.ignoreIntentToAdd = true" will be dropped in future.
The placeholder "FIXME" needs to be replaced when it's decided what
release will drop config support.
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
Documentation/config.txt | 5 ++---
builtin/commit.c | 13 ++++++++-----
t/t2203-add-intent.sh | 4 ++--
3 files changed, 12 insertions(+), 10 deletions(-)
diff --git a/Documentation/config.txt b/Documentation/config.txt
index 6ec81a8..f9a05ac 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -837,9 +837,8 @@ commit.ignoreIntentToAdd::
in index. Set to `false` to disallow commit in this acase, or `true`
to allow it.
+
-By default, `git commit` refuses to commit as-is when you have intent-to-add
-entries. This will change in 1.8.0, where `git commit` allows it. If you
-prefer current behavior, please set it to `false`.
+By default, `git commit` allows to commit as-is when you have intent-to-add
+entries. Support for this configuration variable will be dropped in FIXME.
credential.helper::
Specify an external helper to be called when a username or
diff --git a/builtin/commit.c b/builtin/commit.c
index af3250c..eb0ca49 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -423,17 +423,17 @@ static char *prepare_index(int argc, const char **argv, const char *prefix,
if (!pathspec || !*pathspec) {
fd = hold_locked_index(&index_lock, 1);
refresh_cache_or_die(refresh_flags);
- if (!set_commit_ignoreintenttoadd) {
+ if (!(cache_tree_flags & WRITE_TREE_IGNORE_INTENT_TO_ADD)) {
int i;
for (i = 0; i < active_nr; i++)
if (active_cache[i]->ce_flags & CE_INTENT_TO_ADD)
break;
if (i < active_nr)
warning(_("You are committing as-is with intent-to-add entries as the result of\n"
- "\"git add -N\". Git currently forbids this case. This will change in\n"
- "1.8.0 where intent-to-add entries are simply ignored when committing\n"
- "as-is. Please look up document and set commit.ignoreIntentToAdd\n"
- "properly to stop this warning."));
+ "\"git add -N\". Git currently forbids this case. But this is deprecated\n"
+ "support for this behavior will be dropped in FIXME.\n"
+ "Please look up document and set commit.ignoreIntentToAdd to true\n"
+ "or remove it."));
}
if (active_cache_changed) {
update_main_cache_tree(cache_tree_flags | WRITE_TREE_SILENT);
@@ -1423,6 +1423,9 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
git_config(git_commit_config, &s);
determine_whence(&s);
+ if (!set_commit_ignoreintenttoadd)
+ cache_tree_flags |= WRITE_TREE_IGNORE_INTENT_TO_ADD;
+
if (get_sha1("HEAD", sha1))
current_head = NULL;
else {
diff --git a/t/t2203-add-intent.sh b/t/t2203-add-intent.sh
index 88a508e..09b8bbf 100755
--- a/t/t2203-add-intent.sh
+++ b/t/t2203-add-intent.sh
@@ -41,11 +41,11 @@ test_expect_success 'cannot commit with i-t-a entry' '
echo frotz >nitfol &&
git add rezrov &&
git add -N nitfol &&
- test_must_fail git commit -minitial
+ git commit -minitial
'
test_expect_success 'can commit tree with i-t-a entry' '
- git reset --hard &&
+ git reset --hard HEAD^ &&
echo xyzzy >rezrov &&
echo frotz >nitfol &&
git add rezrov &&
--
1.7.8.36.g69ee2
^ permalink raw reply related
* [PATCH 4/4] commit: remove commit.ignoreIntentToAdd, assume it's always true
From: Nguyễn Thái Ngọc Duy @ 2012-02-06 10:57 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jonathan Nieder,
Nguyễn Thái Ngọc Duy
In-Reply-To: <1328525855-2547-1-git-send-email-pclouds@gmail.com>
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
Documentation/config.txt | 9 ---------
builtin/commit.c | 24 +-----------------------
t/t2203-add-intent.sh | 2 +-
3 files changed, 2 insertions(+), 33 deletions(-)
diff --git a/Documentation/config.txt b/Documentation/config.txt
index f9a05ac..abeb82b 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -831,15 +831,6 @@ commit.template::
"{tilde}/" is expanded to the value of `$HOME` and "{tilde}user/" to the
specified user's home directory.
-commit.ignoreIntentToAdd::
- Allow to commit the index as-is even if there are
- intent-to-add entries (see option `-N` in linkgit:git-add[1])
- in index. Set to `false` to disallow commit in this acase, or `true`
- to allow it.
-+
-By default, `git commit` allows to commit as-is when you have intent-to-add
-entries. Support for this configuration variable will be dropped in FIXME.
-
credential.helper::
Specify an external helper to be called when a username or
password credential is needed; the helper may consult external
diff --git a/builtin/commit.c b/builtin/commit.c
index eb0ca49..491cae1 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -118,8 +118,6 @@ static enum {
} status_format = STATUS_FORMAT_LONG;
static int status_show_branch;
-static int set_commit_ignoreintenttoadd;
-
static int opt_parse_m(const struct option *opt, const char *arg, int unset)
{
struct strbuf *buf = opt->value;
@@ -423,18 +421,6 @@ static char *prepare_index(int argc, const char **argv, const char *prefix,
if (!pathspec || !*pathspec) {
fd = hold_locked_index(&index_lock, 1);
refresh_cache_or_die(refresh_flags);
- if (!(cache_tree_flags & WRITE_TREE_IGNORE_INTENT_TO_ADD)) {
- int i;
- for (i = 0; i < active_nr; i++)
- if (active_cache[i]->ce_flags & CE_INTENT_TO_ADD)
- break;
- if (i < active_nr)
- warning(_("You are committing as-is with intent-to-add entries as the result of\n"
- "\"git add -N\". Git currently forbids this case. But this is deprecated\n"
- "support for this behavior will be dropped in FIXME.\n"
- "Please look up document and set commit.ignoreIntentToAdd to true\n"
- "or remove it."));
- }
if (active_cache_changed) {
update_main_cache_tree(cache_tree_flags | WRITE_TREE_SILENT);
if (write_cache(fd, active_cache, active_nr) ||
@@ -1353,13 +1339,6 @@ static int git_commit_config(const char *k, const char *v, void *cb)
include_status = git_config_bool(k, v);
return 0;
}
- if (!strcmp(k, "commit.ignoreintenttoadd")) {
- set_commit_ignoreintenttoadd = 1;
- if (git_config_bool(k, v))
- cache_tree_flags |= WRITE_TREE_IGNORE_INTENT_TO_ADD;
- else
- cache_tree_flags &= ~WRITE_TREE_IGNORE_INTENT_TO_ADD;
- }
status = git_gpg_config(k, v, NULL);
if (status)
@@ -1423,8 +1402,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
git_config(git_commit_config, &s);
determine_whence(&s);
- if (!set_commit_ignoreintenttoadd)
- cache_tree_flags |= WRITE_TREE_IGNORE_INTENT_TO_ADD;
+ cache_tree_flags |= WRITE_TREE_IGNORE_INTENT_TO_ADD;
if (get_sha1("HEAD", sha1))
current_head = NULL;
diff --git a/t/t2203-add-intent.sh b/t/t2203-add-intent.sh
index 09b8bbf..7c7ab54 100755
--- a/t/t2203-add-intent.sh
+++ b/t/t2203-add-intent.sh
@@ -50,7 +50,7 @@ test_expect_success 'can commit tree with i-t-a entry' '
echo frotz >nitfol &&
git add rezrov &&
git add -N nitfol &&
- git config commit.ignoreIntentToAdd true &&
+ git config commit.ignoreIntentToAdd false &&
git commit -m initial &&
git ls-tree -r HEAD >actual &&
cat >expected <<EOF &&
--
1.7.8.36.g69ee2
^ permalink raw reply related
* Re: Specifying revisions in the future
From: Matthieu Moy @ 2012-02-06 11:43 UTC (permalink / raw)
To: Andreas Schwab; +Cc: Philip Oakley, Jakub Narebski, jpaugh, git
In-Reply-To: <m2obtcx4i2.fsf@igel.home>
Andreas Schwab <schwab@linux-m68k.org> writes:
> The rule should be to follow the leftmost parent as far as possible.
But then, if --first-parent doesn't reach the commit you want, there may
be several paths not following --first-parent that reach it. And you'll
have to invent some more rules to order them.
Sure, that's not impossible, but is the complexity really worth it?
--
Matthieu Moy
http://www-verimag.imag.fr/~moy/
^ permalink raw reply
* Re: [bug] blame duplicates trailing ">" in mailmapped emails
From: Felipe Contreras @ 2012-02-06 12:14 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jeff King, Jonathan Nieder, git, SZEDER Gábor
In-Reply-To: <7vy5sgaby1.fsf@alter.siamese.dyndns.org>
On Mon, Feb 6, 2012 at 5:16 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Jeff King <peff@peff.net> writes:
>
>> Ugh, yeah. I was thinking about how it would improve this call site, but
>> I don't want to get into auditing the others. Let's drop it and go with
>> your patch.
>
> In any case, here is what I queued for tonight.
>
> -- >8 --
> Subject: [PATCH] mailmap: do not leave '>' in the output when answering "we did something"
This subject doesn't explain the *purpose* of the patch: always return
a plain mail address from map_user()
That would be enough for most readers.
I think the immediate problem should be here:
Currently 'git blame -e' would add an extra '>' if map_user() returns
true, which would end up as '<foo@bar.com>>'. This is because
map_user() sometimes modifies, the mail string, but sometimes not. So
let's always modify it.
At this point a lot of readers can skip the rest of the explanation.
> The callers of map_user() give email and name to it, and expect to get an
> up-to-date versions of email and/or name to be used in their output. The
> function rewrites the given buffers in place. To optimize the majority of
> cases, the function returns 0 when it did not do anything, and it returns
> 1 when the caller should use the updated contents.
>
> The 'email' input to the function is terminated by '>' or a NUL (whichever
> comes first) for historical reasons, but when a rewrite happens, the value
> is replaced with the mailbox inside the <> pair. However, it failed to
> meet this expectation when it only rewrote the name part without rewriting
> the email part, and the email in the input was terminated by '>'.
With the above explanation I suggested, I think this can be summarized as:
As of right now most of the callers of map_user() give a plan address,
and expect a plain address, however, 'git blame -e' passes along a
mail address that ends with >, and uses the return value to determine
if a transformation was made, and adds the missing '>'. This is
because when map_user() does the transformation, it returns a plan
address.
This might have worked in previous versions of map_user(), but now not
only does it transforms mail addresses, but also the name (phrase). So
now callers can't use the return value to know if they need to add an
extra '>', or not. So lets always remove the '>', so they can.
> This causes an extra '>' to appear in the output of "blame -e", because the
> caller does send in '>'-terminated email, and when the function returned 1
> to tell it that rewriting happened, it appends '>' that is necessary when
> the email part was rewritten.
It took too long to reach the problem. As a reader, that's the first
thing I would expect.
> The patch looks bigger than it actually is, because this change makes a
> variable that points at the end of the email part in the input 'p' live
> much longer than it used to, deserving a more descriptive name.
This is a *separate* logically independent patch. And you yourself
mention, makes the review of the patch more difficult, as of right now
it's difficult to spot the functional change, because it's mixed with
non-functional changes.
I find it peculiar how patches in the Linux kernel are truly logically
independent, but Git has a tendency to squash many things: cleanups,
fixes, documentation, tests, extra tests, remove unused code, etc.
Cheers.
--
Felipe Contreras
^ permalink raw reply
* Re: Specifying revisions in the future
From: Andreas Schwab @ 2012-02-06 12:27 UTC (permalink / raw)
To: Matthieu Moy; +Cc: Philip Oakley, Jakub Narebski, jpaugh, git
In-Reply-To: <vpq62fk89x1.fsf@bauges.imag.fr>
Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:
> Andreas Schwab <schwab@linux-m68k.org> writes:
>
>> The rule should be to follow the leftmost parent as far as possible.
>
> But then, if --first-parent doesn't reach the commit you want, there may
> be several paths not following --first-parent that reach it. And you'll
> have to invent some more rules to order them.
The leftmost parent is not necessarily the first parent, but the
leftmost parent that still reaches the commit. It's a depth-first
search: if following the first parent doesn't reach the commit any more,
try again with the second parent.
Andreas.
--
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756 01D3 44D5 214B 8276 4ED5
"And now for something completely different."
^ permalink raw reply
* Re: [RFD] Rewriting safety - warn before/when rewriting published history
From: Jakub Narebski @ 2012-02-06 13:45 UTC (permalink / raw)
To: Steven Michalske; +Cc: git
In-Reply-To: <EAF9D593-4E0C-4C95-A048-3F6AC8ADD866@gmail.com>
On Mon, 6 Feb 2012, Steven Michalske wrote:
> See inlined responses below.
Is this comment necessary at all?
> On Feb 4, 2012, at 11:45 AM, Jakub Narebski wrote:
>
> > So people would like for git to warn them about rewriting history before
> > they attempt a push and it turns out to not fast-forward.
> >
>
> I like this idea and I encounter this issue with my co-workers new to git.
> It scares them thinking they broke the repository.
It is true that while this feature would be useful also for "power
users", it would be most helpful for newbies (users new to git).
So I am afraid that implementing it with example hooks that must be
turned on explicitly might be not enough...
> > In Mercurial 2.1 there are three available phases: 'public' for
> > published commits, 'draft' for local un-published commits and
> > 'secret' for local un-published commits which are not meant to
> > be published.
> >
> > The phase of a changeset is always equal to or higher than the phase
> > of it's descendants, according to the following order:
> >
> > public < draft < secret
>
> Let's not limit ourselves to just three levels. They are a great start
> but I propose the following.
As we don't have any implementation, I'd rather we don't multiply entities.
I was even thinking about limiting to just 'public' and 'draft' "phases".
> published - The commits that are on a public repository that if are
> rewritten will invoke uprisings. general rule here would be
> to revert or patch, no rewrites.
> based - The commits that the core developers have work based upon.
> (not just the commits in their repo.)
> general rule is notify your fellow developers before a rewrite.
> shared - The commits that are known to your fellow core developers.
> These commits are known, but have not had work based off of them.
> Minimal risk to rewrite.
All these are very fairly nuanced, with minuscule differences between
them. I'd rather not multiply entities, especially not introduce such
hard to guess what it about from their name.
In Mercurial phases share hierarchy of traits:
http://mercurial.selenic.com/wiki/Phases
| traits |
.......................
| immutable | shared |
----------+-----------+---------+
public | x | x | ^
draft | | x | ^
secret | | | ^
The names of those traits probably should be changed in Git.
Those traits are boolean in Mercurial, but I think we can implement
what you would like to have to change them to tristate: 'deny' (unless
forced, i.e. the same as true), 'warn', 'ignore' (i.e. the same as false).
I think that it would be nice to be able to tune "severity" of trait
on per-remote and/or per-branch basis. This way you would get warned
before rewriting commits that were pushed to your group repository,
and prevented from rewriting commits that are present in projects public
repository.
Nevertheless I think it is something better left for later, and added
only if it turns out to be really needed.
> local - The commits that are local only, no one else has a copy.
> Commits your willing to share, but have not been yet shared,
> either from actions of you, or a fetch from others.
That's Mercurial's 'draft' phase.
> restricted or private - The commits that you do not want shared.
> Manually added, think of a branch tip marked as restricted
> automatically promotes commits to the branch as restricted.
That's Mercurial 'secret' phase.
> Maybe make these like nice levels, but as two components,
> publicity 0-100 and rewritability 0-100
> Published is publicity 100 and rewritability 0
> Restricted is publicity 0 and rewritability 100
> Based publicity 75 and rewritability 25
> Shared publicity 50 and rewritability 50
> Local publicity 25 and rewritability 75
> Restricted publicity 0 and rewritability 100
Continuous traits are IMHO a bad idea. You would have to quantize them
and turn them on into specific behavior: ignore, warn, deny.
For example WTF does 25 "publicity" (bad name) or "rewritability" actually
means in term of git behavior, eh?
> Other option are flags stating if the commit is published, based,
> shared, or restricted. You could have a published and based commit
> that is more opposed to rewrite than a public commit.
>
> Call security on a published restricted commit ;-)
Please note that while "phases" look like they are trait of individual
commits, they are in fact artifact of revision walking. The idea is
that ancestors of 'private' commit can be 'private', 'draft' or 'public',
that ancestors of 'draft' commit are 'draft' or 'public', and that _all_
ancestors of 'public' commit are 'public'.
> Commits are by default local.
This 'by default' needs to be specified further, because for example
all commits in freshly cloned repository should be in 'public' phase
by default.
Also, don't say 'commits are local', 'commits are published'; use "phases"
nomenclature (at least until we invent something so much better that it
is worth breaking consistency with Mercurial terminology).
>
> Commits are published when they are pushed or fetched and merged to
> a publishing branch of a repository.
BTW. I am not sure if pushing to remote repository updates (or can update)
any remote-tracking branches...
> On fetch/merge a post merge hook should send back a note to
> the remote repository that the commits were published.
I think this is unnecessary in the "best practices" scenario, where each
user has separate private repository where he/she does his/her work, and
one's own public repository, where people fetch from. He/she can push
to some shared repository, and that has to be supported too.
Though there is mothership/ sattellite situation, where you can pull and
push only from one direction. There we might want for some way to notify
that some commits were fetched and should now be considered 'public'.
Though I am not sure if it is really necessary.
> Restricted commits/branches/tags should not be made public, error out and
> require clearing of the attribute or a --force-restricted option that
> automatically removes the restricted attribute. They are at least promoted
> to shared, if not published.
Or just skip them (silently or not) if we push using globbing refspec, and
glob matches some refs marked as 'private'.
> Based is only used in situations where you have developers sharing amongst
> their repositories, and you want a rule that is less restrictive than
> no rewrites.
Multiplying entities.
> Shared is what we have now when a commit is in a remote repository without
> the no rewrite options. e.g. receive.denyNonFastForwards.
Multiplying entities.
[...]
> > Using the nomenclature from Mercurial
> > public < draft < secret
>
> public -> publicity 100, rewritability 0
> draft -> publicity ?, rewritability 50
> secret -> publicity 0, rewritability 100
That doesn't really help, at all.
--
Jakub Narebski
Poland
^ permalink raw reply
* Re: [RFD] Rewriting safety - warn before/when rewriting published history
From: Jakub Narebski @ 2012-02-06 14:44 UTC (permalink / raw)
To: Johan Herland; +Cc: Philip Oakley, git
In-Reply-To: <CALKQrgcAsPXziQCTReZkCKnnXTX=rwPFrzp0wJ3ZYwn0b_M5Tw@mail.gmail.com>
On Sun, 5 Feb 2012, Johan Herland wrote:
> On Sun, Feb 5, 2012 at 21:46, Jakub Narebski <jnareb@gmail.com> wrote:
>> On Sun, 5 Feb 2012, Johan Herland wrote:
>>> 2012/2/5 Jakub Narebski <jnareb@gmail.com>:
[...]
>>> I agree that the 'public' state should (by default) be automatically
>>> inferred from remote-tracking branches. As it stands, we can do this
>>> with current git, by writing a pre-rebase hook that checks if any of
>>> the commits to-be-rebased are reachable from any remote-tracking
>>> branch.
>>
>> It is nice that we can achieve a large part of this feature with existing
>> infrastructure. It would be nice if we ship such pre-rebase hook with
>> git, so people can just enable it if they want to use this functionality,
>> like the default pre-commit hook that checks for whitespace errors.
>
> Yeah. As it is, the pre-rebase hook shipped with v1.7.9 (when
> activated) does something similar (i.e. prevent rewriting 'public'
> commits). However, it's highly workflow-specific, since it determines
> whether the branch being rebased has been merged into "next" or
> "master". IMHO, a hook that tested for reachability from
> remote-tracking refs would be more generally useful. Obviously, the
> two can be combined, and even further combinations may be desirable
> (e.g. also checking for reachability from commits annotated in
> refs/notes/public).
Relying on (default) hooks to implement this feature has the disadvantage
that it wouldn't be turned on by default... while this feature would be
most helpful for users new to git (scared by refuse to push).
I am not sure either if everything (wrt. safety net) can be implemented
via hooks. One thing that I forgot about is preventing rewinding of
branch past the published commit using e.g. "git reset --hard <commit>".
Unless `pre-rewrite` hook could be used for that safety too...
[...]
>> Note however that the safety net, i.e. refusing or warning against attempted
>> rewrite of published history is only part of issue. Another important part
>> is querying and showing "phase" of a commit. What I'd like to see is
>> ability to show among others in "git log" and "git show" output if commit
>> was already published or not (and if it is marked 'secret').
>
> Today, you can use --decorate to display remote-tracking refs in the
> log/show output. However, only the tip commits are decorated, so if
> the commits shown are not at the tip, you're out of luck. I believe
> teaching log/show to decorate _all_ commits that are reachable from
> some given ref(s) should be fairly straightforward.
That would be nice.
> If you use 'git notes' to annotate 'public' and 'secret' states, then
> you can also use the --show-notes=<ref> option to let show/log display
> the annotations on 'public'/'secret' commits.
First, in my opinion annotating _all_ commits with their phase is I think
out of question, especially annotating 'public' commits. I don't think
git-notes mechanism would scale well to annotating every commit; but
perhaps this was tested to work, and I am mistaken.
Second, I have doubts if "phase" is really state of an individual commit,
and not the feature of revision walking.
Take for example the situation where given commit is reference by
remote-tracking branch 'public/foo', and also by two local branches:
'foo' with upstream 'public/foo', and local branch 'bar' with no upstream.
Now it is quite obvious that this feature should prevent rewriting 'foo'
branch, for which commits are published upstream. But what about branch
'bar'? Should we prevent rewriting (e.g. rebase) here too? What about
rewinding 'bar' to point somewhere else. What if 'bar' is really detached
HEAD?
These questions need to be answered...
[...]
>>> Also, if you want to record where 'public' commits have been sent
>>> (other than what can be inferred from the remote-tracking branches),
>>> you could write this into the refs/notes/public annotation.
>>
>> I wonder if this too can be done by hook...
>
> You're looking for someting like a post-push hook that runs on the
> _client_ after a successful push. AFAIK, that doesn't exist yet. (Not
> to be confused with the receive/update hooks that run on the
> _server_.)
And such hook could react to what was successfully pushed. Without
such hook we would have to write wrapper around git-push and parse
its output...
Nb. such hook could create "fake" remote-tracking branches if given
push-only remote doesn't have them set up.
>>> As for 'secret' commits, you could annotate these on a
>>> refs/notes/secret notes ref, and then teach 'git push' (or whatever
>>> other method for publishing commits you use) to refuse to publish
>>> commits annotated on this notes ref. Possibly we would want to add a
>>> "pre-push" or "pre-publish" hook.
>>
>> Well, addition of pre-push / pre-publish was resisted on the grounds
>> that all it does is something that can be as easy done by hand before
>> push. Perhaps this new use case would help bring it forward, don't
>> you think?
>
> Maybe. I didn't follow the original discussion. From my POV, you could
> argue that instead of another hook, you could always write a script
> that does the 'secret' check before invoking 'git push', and then
> you'd use that script instead of 'git push'. But you could argue the
> same point for pretty much all of the other existing hooks (e.g.
> instead of a pre-commit hook you could have your own commit wrapper
> script). So I don't think that's a sufficient argument to refuse the
> existence of a pre-push/publish hook.
Right, This would be for this feature very much like pre-commit hook.
--
Jakub Narebski
Poland
^ permalink raw reply
* Re: [PATCH/RFC v4] grep: Add the option '--exclude'
From: Nguyen Thai Ngoc Duy @ 2012-02-06 15:16 UTC (permalink / raw)
To: Albert Yale; +Cc: git, gitster
In-Reply-To: <1328192753-29162-1-git-send-email-surfingalbert@gmail.com>
On Thu, Feb 2, 2012 at 9:25 PM, Albert Yale <surfingalbert@gmail.com> wrote:
> I added a "struct pathspec_set" as you suggested
> in your previous review. It had the side effect
> of forcing me to update a few more files than was
> previously necessary.
Please make it a separate patch, it's hard to follow an all-in-one
patch. Although those changes might be unnecessary (see below).
> --- a/builtin/grep.c
> +++ b/builtin/grep.c
> @@ -566,6 +566,10 @@ static int grep_tree(struct grep_opt *opt, const struct pathspec *pathspec,
> while (tree_entry(tree, &entry)) {
> int te_len = tree_entry_len(&entry);
>
> + if (!match_pathspec_depth(pathspec,
> + entry.path, strlen(entry.path),
> + 0, NULL))
> + continue;
> if (match != all_entries_interesting) {
> match = tree_entry_interesting(&entry, base, tn_len, pathspec);
> if (match == all_entries_not_interesting)
tree_entry_interesting() is equivalent to match_pathspec_depth(). The
only difference is that the former is designed to match on trees why
the latter a list. And tree_entry_interesting() is more efficient than
match_pathspec_depth(). As you can see there's
tree_entry_interesting() call above already. You should make the
tree_entry_interesting() understand exclude pathspec instead of adding
match_pathspec_depth() in.
> @@ -295,6 +298,25 @@ int match_pathspec_depth(const struct pathspec *ps,
> return retval;
> }
>
> +int match_pathspec_depth(const struct pathspec *ps,
> + const char *name, int namelen,
> + int prefix, char *seen)
> +{
> + int retval = match_pathspec_set_depth(&ps->include,
> + ps->recursive, ps->max_depth,
> + name, namelen, prefix, seen);
> +
> + if (retval && ps->exclude.nr)
> + {
> + if (match_pathspec_set_depth(&ps->exclude,
> + ps->recursive, ps->max_depth,
> + name, namelen, prefix, seen))
> + return 0;
> + }
> +
> + return retval;
> +}
> +
> static int no_wildcard(const char *string)
> {
> return string[strcspn(string, "*?[{\\")] == '\0';
It makes me wonder, why not add match_pathspec_with_exclusion(const
struct pathspec *include_ps, const struct pathspec *exclude_ps,...),
use the new function in grep.c and revert struct pathspec back to
original? The same can be applied to tree_entry_interesting() (i.e.
add a new one that takes two pathspec sets, which supports exclusion)
I think you may make less changes that way. I'm bad at naming. It's up
to you to rename match_pathspec_with_exclusion to something meaningful
and short enough.
--
Duy
^ permalink raw reply
* Re: Git performance results on a large repository
From: Joey Hess @ 2012-02-06 15:40 UTC (permalink / raw)
To: git@vger.kernel.org
In-Reply-To: <CACsJy8Bf95JMp1qOiruR7+Tdi7JN42KNeMqGLud+z3O26DREnw@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 1176 bytes --]
Nguyen Thai Ngoc Duy wrote:
> The "interface to report which files have changed" is exactly "git
> update-index --[no-]assume-unchanged" is for. Have a look at the man
> page. Basically you can mark every file "unchanged" in the beginning
> and git won't bother lstat() them. What files you change, you have to
> explicitly run "git update-index --no-assume-unchanged" to tell git.
>
> Someone on HN suggested making assume-unchanged files read-only to
> avoid 90% accidentally changing a file without telling git. When
> assume-unchanged bit is cleared, the file is made read-write again.
That made me think about using assume-unchanged with git-annex since it
already has read-only files.
But, here's what seems a misfeature... If an assume-unstaged file has
modifications and I git add it, nothing happens. To stage a change, I
have to explicitly git update-index --no-assume-unchanged and only then
git add, and then I need to remember to reset the assume-unstaged bit
when I'm done working on that file for now. Compare with running git mv
on the same file, which does stage the move despite assume-unstaged. (So
does git rm.)
--
see shy jo
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 828 bytes --]
^ permalink raw reply
* Re: [PATCH 0/3] On compresing large index
From: Joshua Redstone @ 2012-02-06 15:54 UTC (permalink / raw)
To: Nguyen Thai Ngoc Duy, Thomas Rast; +Cc: git@vger.kernel.org
In-Reply-To: <CACsJy8AnGg11PeCGFs_BxOM3wAjwzs2tOCWJV31_2_KMFTxhDA@mail.gmail.com>
Fwiw, specifically related to 'git ls-files', since it is a relatively
rare operation, it's probably ok if it's a bit slow. I know you chose it
as a good benchmark of index reading performance. I just mention it
because, in some hypothetical wild-and-crazy world in which we had a
git-aware file system layer, one could imagine doing away with most of the
index file and querying the file system for info on what's changed, SHA1
of subtrees, etc.
Do you have a sense of which operations on the index are high-value pain
points for large repositories? I can imagine things like 'git-add' and
'git-commit', but I'm not super familiar with other common operations it
has a role in.
Josh
On 2/5/12 8:35 PM, "Nguyen Thai Ngoc Duy" <pclouds@gmail.com> wrote:
>2012/2/6 Thomas Rast <trast@inf.ethz.ch>:
>>> We need to figure out what git uses 4s user time for.
>>
>> When I worked on the cache-tree stuff, my observation (based on
>> profiling, so I had actual data :-) was that computing SHA1s absolutely
>> dominates everything in such operations. It does that when writing the
>> index to write the trailing checksum, and also when loading it to verify
>> that the index is valid.
>
>You're right. This is on another machine but with same index (2M
>files), without SHA1 checksum:
>
>$ time ~/w/git/git ls-files --stage|head > /dev/null
>real 0m1.533s
>user 0m1.228s
>sys 0m0.306s
>
>and with SHA-1 checksum:
>
>$ time git ls-files --stage|head > /dev/null
>real 0m7.525s
>user 0m7.257s
>sys 0m0.268s
>
>I guess we could fall back to cheaper digests for such a large index.
>Still more than one second for doing nothing but reading index is too
>slow to me.
>
>> ls-files shouldn't be so slow though. A quick run with callgrind in a
>> linux-2.6.git tells me it spends about 45% of its time on SHA1s and a
>> whopping 25% in quote_c_style(). I wonder what's so hard about
>> quoting...
>
>That's why I put "| head" there, to cut output processing overhead
>(hopefully).
>--
>Duy
^ permalink raw reply
* Re: [RFD] Rewriting safety - warn before/when rewriting published history
From: Johan Herland @ 2012-02-06 15:59 UTC (permalink / raw)
To: Jakub Narebski; +Cc: Philip Oakley, git
In-Reply-To: <201202061544.14417.jnareb@gmail.com>
On Mon, Feb 6, 2012 at 15:44, Jakub Narebski <jnareb@gmail.com> wrote:
> On Sun, 5 Feb 2012, Johan Herland wrote:
> Relying on (default) hooks to implement this feature has the disadvantage
> that it wouldn't be turned on by default... while this feature would be
> most helpful for users new to git (scared by refuse to push).
True. I too believe that this will be most helpful if it is enabled by
default. That said, the easiest way to get there might be through
first demonstrating that it works in practice when implemented as
hooks.
> I am not sure either if everything (wrt. safety net) can be implemented
> via hooks. One thing that I forgot about is preventing rewinding of
> branch past the published commit using e.g. "git reset --hard <commit>".
> Unless `pre-rewrite` hook could be used for that safety too...
Hmm. I don't think we'll be able to "plug" all the holes that might
leave the user in a rewritten state (e.g. what if the user (possibly
with the help of some tool) does an "echo $SHA1 >
.git/refs/head/master"?). And trying to plug too many holes might end
up annoying more experienced users who "know what they're doing".
Instead we might want to add a client-side check at push time. I
realize that this check is already done by the remote end, but the
client-side might be able to give a more helpful response along the
lines of:
You are trying to push branch X to remote Y, but remote Y already
has a branch X that is N commits in front of you. You may want to
rebase your work on top of the remote branch (see 'git pull
--rebase'), If you instead force this push (with --force), you will
remove those N commits, and replace them with the M last commits from
your branch X.
(followed by a list of the remote N and local M commits, respectively)
[...]
>> If you use 'git notes' to annotate 'public' and 'secret' states, then
>> you can also use the --show-notes=<ref> option to let show/log display
>> the annotations on 'public'/'secret' commits.
>
> First, in my opinion annotating _all_ commits with their phase is I think
> out of question, especially annotating 'public' commits. I don't think
> git-notes mechanism would scale well to annotating every commit; but
> perhaps this was tested to work, and I am mistaken.
First, we don't need to annotate _all_ commits. For the 'public'
state, we only annotate the last/tip commit that was pushed/published.
From there, we can defer that all ancestor commits are also 'public'.
For the 'secret' state, we do indeed annotate _all_ secret commits,
but I believe this will be a somewhat limited number of commits. If
your workflow forces you to annotate millions of commits as 'secret',
I claim there is something wrong with your workflow.
Second, git-notes were indeed designed scale well to handle a large
number of notes, up to the same order of magnitude as the number of
commits in your repo. (When git-notes was originally written, I
successfully tested it on versions of a linux-kernel repo where every
single commit was annotated). In this case, the number of 'public'
annotations in your repo would be equal to the number of pushes you
do, and the number of 'secret' annotations would be equal to the
number of 'secret' commits in your repo. I'd expect both of these
numbers to be orders of magnitude smaller than the total number of
commits in your repo (given a fairly typical workflow in a fairly
typical repo).
> Second, I have doubts if "phase" is really state of an individual commit,
> and not the feature of revision walking.
I believe the 'public' state is a "feature of revision walking" (i.e.
one annotated 'public' commit implies that all its ancestors are also
'public'). However, the 'secret' state should be bound to the
individual commit, IMHO.
> Take for example the situation where given commit is reference by
> remote-tracking branch 'public/foo', and also by two local branches:
> 'foo' with upstream 'public/foo', and local branch 'bar' with no upstream.
>
> Now it is quite obvious that this feature should prevent rewriting 'foo'
> branch, for which commits are published upstream. But what about branch
> 'bar'? Should we prevent rewriting (e.g. rebase) here too? What about
> rewinding 'bar' to point somewhere else. What if 'bar' is really detached
> HEAD?
>
> These questions need to be answered...
Good point. There are two questions we may need to answer: "Has commit
X ever been published?", and "Has commit X ever been published in the
context of branch Y?". In the latter case, we do indeed need to take
the upstream branch into account.
Basically, there are three different "levels" for this rewrite/publish
protection to run at:
1. Do not meddle at all. This is the current behavior, and assumes
that if the user rewrites and pushes something, the user knows what
he/she is doing, and Git should not meddle (obviously unless the
server refuses the push).
2. Warn/refuse rewriting commits in your upstream. This would only
check branch X against its registered upstream. Only if there is a
registered upstream, and you're about to rewrite commits that are
reachable from the upstream remote-tracking branch, should Git
intervene and warn/refuse the rewrite. This level would IMHO provide
most of the benefit, and little or no trouble (i.e. false positives).
3. Warn/refuse rewriting _any_ 'public' commit. Refuse to rewrite any
commit that is reachable from any remote-tracking branch. Some would
say that this is a Good Thing(tm), since it prevents a commit from
being _copied_ (i.e. rebased or cherry-picked) between branches (you'd
be in this camp if you run a tightly-controlled workflow, where you
e.g. mandate upmerging patches from the oldest applicable branch
instead of cherry-picking patches from a newer branch). However, other
people would say that this is too limiting, and imposes unnecessary
rules on the workflow of the project (where e.g. copying (by way of
git-rebase) a topic branch from one place to another would cause an
annoying false positive).
[...]
Have fun! :)
...Johan
--
Johan Herland, <johan@herland.net>
www.herland.net
^ permalink raw reply
* Re: Git performance results on a large repository
From: Matt Graham @ 2012-02-06 16:23 UTC (permalink / raw)
To: Joshua Redstone; +Cc: Nguyen Thai Ngoc Duy, git@vger.kernel.org
In-Reply-To: <243C23AF01622E49BEA3F28617DBF0AD5912CA85@SC-MBX02-5.TheFacebook.com>
On Sat, Feb 4, 2012 at 18:05, Joshua Redstone <joshua.redstone@fb.com> wrote:
> [ wanted to reply to my initial msg, but wasn't subscribed to the list at time of mailing, so replying to most recent post instead ]
>
> Matt Graham: I don't have file stats at the moment. It's mostly code files, with a few larger data files here and there. We also don't do sparse checkouts, primarily because most people use git (whether on top of SVN or not), which doesn't support it.
This doesn't help your original goal, but while you're still working
with git-svn, you can do sparse checkouts. Use --ignore-paths when you
do the original clone and it will filter out directories that are not
of interest.
We used this at Etsy to keep git svn checkouts manageable when we
still had a gigantic svn repo. You've repeatedly said you don't want
to reorganize your repos but you may find this writeup informative
about how Etsy migrated to git (which included a health amount of repo
manipuation).
http://codeascraft.etsy.com/2011/12/02/moving-from-svn-to-git-in-1000-easy-steps/
^ permalink raw reply
* [PATCH] fsck: give accurate error message on empty loose object files
From: Matthieu Moy @ 2012-02-06 16:24 UTC (permalink / raw)
To: git, gitster; +Cc: Matthieu Moy
In-Reply-To: <vpqfwf6en6e.fsf@bauges.imag.fr>
Since 3ba7a065527a (A loose object is not corrupt if it
cannot be read due to EMFILE), "git fsck" on a repository with an empty
loose object file complains with the error message
fatal: failed to read object <sha1>: Invalid argument
This comes from a failure of mmap on this empty file, which sets errno to
EINVAL. Instead of calling xmmap on empty file, we display a clean error
message ourselves, and return a NULL pointer. The new message is
error: object file .git/objects/09/<rest-of-sha1> is empty
fatal: loose object <sha1> (stored in .git/objects/09/<rest-of-sha1>) is corrupt
The second line was already there before the regression in 3ba7a065527a,
and the first is an additional message, that should help diagnosing the
problem for the user.
Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>
---
sha1_file.c | 5 +++++
1 files changed, 5 insertions(+), 0 deletions(-)
diff --git a/sha1_file.c b/sha1_file.c
index 88f2151..fafc187 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -1202,6 +1202,11 @@ void *map_sha1_file(const unsigned char *sha1, unsigned long *size)
if (!fstat(fd, &st)) {
*size = xsize_t(st.st_size);
+ if (*size == 0) {
+ /* mmap() is forbidden on empty files */
+ error("object file %s is empty", sha1_file_name(sha1));
+ return NULL;
+ }
map = xmmap(NULL, *size, PROT_READ, MAP_PRIVATE, fd, 0);
}
close(fd);
--
1.7.9.111.gf3fb0.dirty
^ permalink raw reply related
* Re: git-gui Ctrl-U (unstage) broken
From: Stefan Haller @ 2012-02-06 16:48 UTC (permalink / raw)
To: Pat Thoyts, Victor Engmark; +Cc: git
In-Reply-To: <877h0at7ua.fsf@fox.patthoyts.tk>
Pat Thoyts <patthoyts@users.sourceforge.net> wrote:
> Victor Engmark <victor.engmark@gmail.com> writes:
>
> >Using the git-gui available with the default Ubuntu 10.10 repos, I'm
> >not able to unstage files with the default keyboard shortcut. To
> >reproduce:
> >1. Change a file in the repository
> >2. Run `git gui`
> >3. Stage the changed file
> >4. Select the changed file in the "Staged Changes (Will Commit)" list
> >5. Click Ctrl-U
> >
> >Expected outcome: The selected file should be unstaged.
> >
> >Actual outcome: Nothing at all changes in the GUI.
>
> I checked this with the current version (gitgui-0.16.0) and it works ok
> for me (on windows) - ie: ctrl-u unstaged a selected file.
Pat, it depends on where the focus is when you press ctrl-u. If you
click in the diff pane, and then select the file to unstage, and then
press ctrl-u, then nothing happens. If you click in the commit message
field, then ctrl-u works fine.
The same problem exists with ctrl-j for "Revert Changes". In that case
it is caused by the vi bindings that were introduced by 60aa065f69. It
seems that a binding for <Key-j> is also triggered by ctrl-j (if you
have a long diff you can see the diff pane scroll down by one line).
I'm not sure how to explain why ctrl-u doesn't work though, as I can't
see any binding for <Key-u>, but maybe this gives a clue to someone who
knows more about TCL than I do.
(It's not Windows-specific, btw: the same problem exists on Mac with
Command-U and Command-J.)
--
Stefan Haller
Berlin, Germany
http://www.haller-berlin.de/
^ 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