* Re: [PATCH 4/6] add -u: only show pathless 'add -u' warning when changes exist outside cwd
From: Jeff King @ 2013-03-20 5:06 UTC (permalink / raw)
To: Jonathan Nieder
Cc: Junio C Hamano, Matthieu Moy, git,
Nguyễn Thái Ngọc Duy
In-Reply-To: <20130319225050.GE19014@google.com>
On Tue, Mar 19, 2013 at 03:50:50PM -0700, Jonathan Nieder wrote:
> This is the interesting one.
> [...]
> * summarized Peff's review with an Ack. I hope that's ok.
Yeah, OK with me. I certainly agree with the intent, and I think your
reasoning on the performance change is valid.
I don't see anything wrong in the patch itself, except for one minor
nit:
> @@ -121,17 +140,30 @@ static void update_callback(struct diff_queue_struct *q,
> }
> }
>
> +#define ADD_CACHE_IMPLICIT_DOT 32
> int add_files_to_cache(const char *prefix, const char **pathspec, int flags)
> {
> struct update_callback_data data;
> struct rev_info rev;
Should this be defined in cache.h with the other flags? I realize it's
mostly private to builtin/add.c, but without even a comment in cache.h
saying "/* 32 is reserved */", we run the risk of another commit
adding a new flag with the same number.
-Peff
^ permalink raw reply
* Re: [PATCH v1 39/45] parse_pathspec: make sure the prefix part is wildcard-free
From: Duy Nguyen @ 2013-03-20 1:32 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <7vli9ji6gu.fsf@alter.siamese.dyndns.org>
On Wed, Mar 20, 2013 at 1:34 AM, Junio C Hamano <gitster@pobox.com> wrote:
> This seems to break t7300-clean.sh #8
Repeatedly? I saw some t7300-clean.sh failures when running tests in
parallel, but never been able to reproduce it alone.
--
Duy
^ permalink raw reply
* Re: regression in multi-threaded git-pack-index
From: Duy Nguyen @ 2013-03-20 1:17 UTC (permalink / raw)
To: Thomas Rast; +Cc: Stefan Zager, git, Jeff King, Junio C Hamano
In-Reply-To: <87obef2wut.fsf@pctrast.inf.ethz.ch>
On Tue, Mar 19, 2013 at 11:11 PM, Thomas Rast <trast@student.ethz.ch> wrote:
> Actually scratch that again. It *is* a stack overflow, except that this
> is a thread stack, for which the OS X defaults are 512kB apparently, as
> opposed to 2MB on linux.
Thanks. I was scratching my head last night wondering if there was
unprotected access to the object database and probably would have
continued this morning.
--
Duy
^ permalink raw reply
* [Notice] The tip of 'next' rewound, 1st wave of topics graduated
From: Junio C Hamano @ 2013-03-19 23:31 UTC (permalink / raw)
To: git
The subject says it all. I didn't get around to Jonathan's
add-u/-A, Thomas's packed_object_info() work, .gitattributes
regression that made plain pattern not to match directories, or
longstanding apply --whitespace=fix breakage today.
^ permalink raw reply
* [PATCH 6/6] git add: -u/-A now affects the entire working tree
From: Jonathan Nieder @ 2013-03-19 22:53 UTC (permalink / raw)
To: Junio C Hamano
Cc: Matthieu Moy, Jeff King, git,
Nguyễn Thái Ngọc Duy
In-Reply-To: <20130319224400.GA19014@google.com>
From: Junio C Hamano <gitster@pobox.com>
As promised in 0fa2eb530fb7 (add: warn when -u or -A is used without
pathspec, 2013-01-28), in Git 2.0, "git add -u/-A" that is run
without pathspec in a subdirectory updates all updated paths in the
entire working tree, not just the current directory and its
subdirectories.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
Thanks for reading.
Documentation/git-add.txt | 16 +++----
builtin/add.c | 110 ++++------------------------------------------
t/t2200-add-update.sh | 9 +---
3 files changed, 19 insertions(+), 116 deletions(-)
diff --git a/Documentation/git-add.txt b/Documentation/git-add.txt
index b0944e5..02b99cb 100644
--- a/Documentation/git-add.txt
+++ b/Documentation/git-add.txt
@@ -104,10 +104,10 @@ apply to the index. See EDITING PATCHES below.
<pathspec>. This removes as well as modifies index entries to
match the working tree, but adds no new files.
+
-If no <pathspec> is given, the current version of Git defaults to
-"."; in other words, update all tracked files in the current directory
-and its subdirectories. This default will change in a future version
-of Git, hence the form without <pathspec> should not be used.
+If no <pathspec> is given when `-u` option is used, all
+tracked files in the entire working tree are updated (old versions
+of Git used to limit the update to the current directory and its
+subdirectories).
-A::
--all::
@@ -116,10 +116,10 @@ of Git, hence the form without <pathspec> should not be used.
entry. This adds, modifies, and removes index entries to
match the working tree.
+
-If no <pathspec> is given, the current version of Git defaults to
-"."; in other words, update all files in the current directory
-and its subdirectories. This default will change in a future version
-of Git, hence the form without <pathspec> should not be used.
+If no <pathspec> is given when `-A` option is used, all
+files in the entire working tree are updated (old versions
+of Git used to limit the update to the current directory and its
+subdirectories).
-N::
--intent-to-add::
diff --git a/builtin/add.c b/builtin/add.c
index 2493493..5c0a869 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -26,50 +26,8 @@ static int take_worktree_changes;
struct update_callback_data {
int flags;
int add_errors;
- const char *implicit_dot;
- size_t implicit_dot_len;
};
-static const char *option_with_implicit_dot;
-static const char *short_option_with_implicit_dot;
-
-static void warn_pathless_add(void)
-{
- static int shown;
- assert(option_with_implicit_dot && short_option_with_implicit_dot);
-
- if (shown)
- return;
- shown = 1;
-
- /*
- * To be consistent with "git add -p" and most Git
- * commands, we should default to being tree-wide, but
- * this is not the original behavior and can't be
- * changed until users trained themselves not to type
- * "git add -u" or "git add -A". For now, we warn and
- * keep the old behavior. Later, the behavior can be changed
- * to tree-wide, keeping the warning for a while, and
- * eventually we can drop the warning.
- */
- warning(_("The behavior of 'git add %s (or %s)' with no path argument from a\n"
- "subdirectory of the tree will change in Git 2.0 and should not be used anymore.\n"
- "To add content for the whole tree, run:\n"
- "\n"
- " git add %s :/\n"
- " (or git add %s :/)\n"
- "\n"
- "To restrict the command to the current directory, run:\n"
- "\n"
- " git add %s .\n"
- " (or git add %s .)\n"
- "\n"
- "With the current Git version, the command is restricted to the current directory."),
- option_with_implicit_dot, short_option_with_implicit_dot,
- option_with_implicit_dot, short_option_with_implicit_dot,
- option_with_implicit_dot, short_option_with_implicit_dot);
-}
-
static int fix_unmerged_status(struct diff_filepair *p,
struct update_callback_data *data)
{
@@ -96,27 +54,11 @@ static void update_callback(struct diff_queue_struct *q,
{
int i;
struct update_callback_data *data = cbdata;
- const char *implicit_dot = data->implicit_dot;
- size_t implicit_dot_len = data->implicit_dot_len;
for (i = 0; i < q->nr; i++) {
struct diff_filepair *p = q->queue[i];
const char *path = p->one->path;
- /*
- * Check if "git add -A" or "git add -u" was run from a
- * subdirectory with a modified file outside that directory,
- * and warn if so.
- *
- * "git add -u" will behave like "git add -u :/" instead of
- * "git add -u ." in the future. This warning prepares for
- * that change.
- */
- if (implicit_dot &&
- strncmp_icase(path, implicit_dot, implicit_dot_len)) {
- warn_pathless_add();
- continue;
- }
switch (fix_unmerged_status(p, data)) {
default:
die(_("unexpected diff status %c"), p->status);
@@ -140,24 +82,13 @@ static void update_callback(struct diff_queue_struct *q,
}
}
-#define ADD_CACHE_IMPLICIT_DOT 32
int add_files_to_cache(const char *prefix, const char **pathspec, int flags)
{
struct update_callback_data data;
struct rev_info rev;
memset(&data, 0, sizeof(data));
- data.flags = flags & ~ADD_CACHE_IMPLICIT_DOT;
- if ((flags & ADD_CACHE_IMPLICIT_DOT) && prefix) {
- /*
- * Check for modified files throughout the worktree so
- * update_callback has a chance to warn about changes
- * outside the cwd.
- */
- data.implicit_dot = prefix;
- data.implicit_dot_len = strlen(prefix);
- pathspec = NULL;
- }
+ data.flags = flags;
init_revisions(&rev, prefix);
setup_revisions(0, NULL, &rev, NULL);
@@ -170,9 +101,7 @@ int add_files_to_cache(const char *prefix, const char **pathspec, int flags)
return !!data.add_errors;
}
-#define WARN_IMPLICIT_DOT (1u << 0)
-static char *prune_directory(struct dir_struct *dir, const char **pathspec,
- int prefix, unsigned flag)
+static char *prune_directory(struct dir_struct *dir, const char **pathspec, int prefix)
{
char *seen;
int i, specs;
@@ -189,16 +118,6 @@ static char *prune_directory(struct dir_struct *dir, const char **pathspec,
if (match_pathspec(pathspec, entry->name, entry->len,
prefix, seen))
*dst++ = entry;
- else if (flag & WARN_IMPLICIT_DOT)
- /*
- * "git add -A" was run from a subdirectory with a
- * new file outside that directory.
- *
- * "git add -A" will behave like "git add -A :/"
- * instead of "git add -A ." in the future.
- * Warn about the coming behavior change.
- */
- warn_pathless_add();
}
dir->nr = dst - dir->entries;
add_pathspec_matches_against_index(pathspec, seen, specs);
@@ -415,7 +334,6 @@ int cmd_add(int argc, const char **argv, const char *prefix)
int add_new_files;
int require_pathspec;
char *seen = NULL;
- int implicit_dot = 0;
git_config(add_config, NULL);
@@ -435,19 +353,11 @@ int cmd_add(int argc, const char **argv, const char *prefix)
die(_("-A and -u are mutually incompatible"));
if (!show_only && ignore_missing)
die(_("Option --ignore-missing can only be used together with --dry-run"));
- if (addremove) {
- option_with_implicit_dot = "--all";
- short_option_with_implicit_dot = "-A";
- }
- if (take_worktree_changes) {
- option_with_implicit_dot = "--update";
- short_option_with_implicit_dot = "-u";
- }
- if (option_with_implicit_dot && !argc) {
- static const char *here[2] = { ".", NULL };
+
+ if ((addremove || take_worktree_changes) && !argc) {
+ static const char *whole[2] = { ":/", NULL };
argc = 1;
- argv = here;
- implicit_dot = 1;
+ argv = whole;
}
add_new_files = !take_worktree_changes && !refresh_only;
@@ -460,8 +370,7 @@ int cmd_add(int argc, const char **argv, const char *prefix)
(intent_to_add ? ADD_CACHE_INTENT : 0) |
(ignore_add_errors ? ADD_CACHE_IGNORE_ERRORS : 0) |
(!(addremove || take_worktree_changes)
- ? ADD_CACHE_IGNORE_REMOVAL : 0)) |
- (implicit_dot ? ADD_CACHE_IMPLICIT_DOT : 0);
+ ? ADD_CACHE_IGNORE_REMOVAL : 0));
if (require_pathspec && argc == 0) {
fprintf(stderr, _("Nothing specified, nothing added.\n"));
@@ -485,10 +394,9 @@ int cmd_add(int argc, const char **argv, const char *prefix)
}
/* This picks up the paths that are not tracked */
- baselen = fill_directory(&dir, implicit_dot ? NULL : pathspec);
+ baselen = fill_directory(&dir, pathspec);
if (pathspec)
- seen = prune_directory(&dir, pathspec, baselen,
- implicit_dot ? WARN_IMPLICIT_DOT : 0);
+ seen = prune_directory(&dir, pathspec, baselen);
}
if (refresh_only) {
diff --git a/t/t2200-add-update.sh b/t/t2200-add-update.sh
index c317254..4281e18 100755
--- a/t/t2200-add-update.sh
+++ b/t/t2200-add-update.sh
@@ -80,18 +80,13 @@ test_expect_success 'change gets noticed' '
'
-# Note that this is scheduled to change in Git 2.0, when
-# "git add -u" will become full-tree by default.
-test_expect_success 'non-limited update in subdir leaves root alone' '
+test_expect_success 'non-qualified update in subdir updates from the root' '
(
cd dir1 &&
echo even more >>sub2 &&
git add -u
) &&
- cat >expect <<-\EOF &&
- check
- top
- EOF
+ : >expect &&
git diff-files --name-only >actual &&
test_cmp expect actual
'
--
1.8.2.rc3
^ permalink raw reply related
* [PATCH 5/6] add -A: only show pathless 'add -A' warning when changes exist outside cwd
From: Jonathan Nieder @ 2013-03-19 22:51 UTC (permalink / raw)
To: Junio C Hamano
Cc: Matthieu Moy, Jeff King, git,
Nguyễn Thái Ngọc Duy
In-Reply-To: <20130319224400.GA19014@google.com>
In the spirit of the recent similar change for 'git add -u', avoid
pestering users that restrict their attention to a subdirectory and
will not be affected by the coming change in the behavior of pathless
'git add -A'.
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
As before.
builtin/add.c | 21 ++++++++++++++++-----
1 file changed, 16 insertions(+), 5 deletions(-)
diff --git a/builtin/add.c b/builtin/add.c
index 4d8d441..2493493 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -170,7 +170,9 @@ int add_files_to_cache(const char *prefix, const char **pathspec, int flags)
return !!data.add_errors;
}
-static char *prune_directory(struct dir_struct *dir, const char **pathspec, int prefix)
+#define WARN_IMPLICIT_DOT (1u << 0)
+static char *prune_directory(struct dir_struct *dir, const char **pathspec,
+ int prefix, unsigned flag)
{
char *seen;
int i, specs;
@@ -187,6 +189,16 @@ static char *prune_directory(struct dir_struct *dir, const char **pathspec, int
if (match_pathspec(pathspec, entry->name, entry->len,
prefix, seen))
*dst++ = entry;
+ else if (flag & WARN_IMPLICIT_DOT)
+ /*
+ * "git add -A" was run from a subdirectory with a
+ * new file outside that directory.
+ *
+ * "git add -A" will behave like "git add -A :/"
+ * instead of "git add -A ." in the future.
+ * Warn about the coming behavior change.
+ */
+ warn_pathless_add();
}
dir->nr = dst - dir->entries;
add_pathspec_matches_against_index(pathspec, seen, specs);
@@ -433,8 +445,6 @@ int cmd_add(int argc, const char **argv, const char *prefix)
}
if (option_with_implicit_dot && !argc) {
static const char *here[2] = { ".", NULL };
- if (prefix && addremove)
- warn_pathless_add();
argc = 1;
argv = here;
implicit_dot = 1;
@@ -475,9 +485,10 @@ int cmd_add(int argc, const char **argv, const char *prefix)
}
/* This picks up the paths that are not tracked */
- baselen = fill_directory(&dir, pathspec);
+ baselen = fill_directory(&dir, implicit_dot ? NULL : pathspec);
if (pathspec)
- seen = prune_directory(&dir, pathspec, baselen);
+ seen = prune_directory(&dir, pathspec, baselen,
+ implicit_dot ? WARN_IMPLICIT_DOT : 0);
}
if (refresh_only) {
--
1.8.2.rc3
^ permalink raw reply related
* [PATCH 4/6] add -u: only show pathless 'add -u' warning when changes exist outside cwd
From: Jonathan Nieder @ 2013-03-19 22:50 UTC (permalink / raw)
To: Junio C Hamano
Cc: Matthieu Moy, Jeff King, git,
Nguyễn Thái Ngọc Duy
In-Reply-To: <20130319224400.GA19014@google.com>
A common workflow in large projects is to chdir into a subdirectory of
interest and only do work there:
cd src
vi foo.c
make test
git add -u
git commit
The upcoming change to 'git add -u' behavior would not affect such a
workflow: when the only changes present are in the current directory,
'git add -u' will add all changes, and whether that happens via an
implicit "." or implicit ":/" parameter is an unimportant
implementation detail.
The warning about use of 'git add -u' with no pathspec is annoying
because it seemingly serves no purpose in this case. So suppress the
warning unless there are changes outside the cwd that are not being
added.
A previous version of this patch ran two I/O-intensive diff-files
passes: one to find changes outside the cwd, and another to find
changes to add to the index within the cwd. This version runs one
full-tree diff and decides for each change whether to add it or warn
and suppress it in update_callback. As a result, even on very large
repositories "git add -u" will not be significantly slower than the
future default behavior ("git add -u :/"), and the slowdown relative
to "git add -u ." should be a useful clue to users of such
repositories to get into the habit of explicitly passing '.'.
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
Acked-by: Jeff King <peff@peff.net>
Improved-by: Junio C Hamano <gitster@pobox.com>
---
This is the interesting one.
Changes since v1:
* run only one diff-files pass, as explained in the log message
* use strncmp_icase, not match_pathspec, to check if paths are
under cwd
* expand log message with performance implications
* summarized Peff's review with an Ack. I hope that's ok.
Thanks for your help getting this into good shape.
builtin/add.c | 43 +++++++++++++++++++++++++++++++++++++++----
1 file changed, 39 insertions(+), 4 deletions(-)
diff --git a/builtin/add.c b/builtin/add.c
index a424e69..4d8d441 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -26,6 +26,8 @@ static int take_worktree_changes;
struct update_callback_data {
int flags;
int add_errors;
+ const char *implicit_dot;
+ size_t implicit_dot_len;
};
static const char *option_with_implicit_dot;
@@ -94,10 +96,27 @@ static void update_callback(struct diff_queue_struct *q,
{
int i;
struct update_callback_data *data = cbdata;
+ const char *implicit_dot = data->implicit_dot;
+ size_t implicit_dot_len = data->implicit_dot_len;
for (i = 0; i < q->nr; i++) {
struct diff_filepair *p = q->queue[i];
const char *path = p->one->path;
+
+ /*
+ * Check if "git add -A" or "git add -u" was run from a
+ * subdirectory with a modified file outside that directory,
+ * and warn if so.
+ *
+ * "git add -u" will behave like "git add -u :/" instead of
+ * "git add -u ." in the future. This warning prepares for
+ * that change.
+ */
+ if (implicit_dot &&
+ strncmp_icase(path, implicit_dot, implicit_dot_len)) {
+ warn_pathless_add();
+ continue;
+ }
switch (fix_unmerged_status(p, data)) {
default:
die(_("unexpected diff status %c"), p->status);
@@ -121,17 +140,30 @@ static void update_callback(struct diff_queue_struct *q,
}
}
+#define ADD_CACHE_IMPLICIT_DOT 32
int add_files_to_cache(const char *prefix, const char **pathspec, int flags)
{
struct update_callback_data data;
struct rev_info rev;
+
+ memset(&data, 0, sizeof(data));
+ data.flags = flags & ~ADD_CACHE_IMPLICIT_DOT;
+ if ((flags & ADD_CACHE_IMPLICIT_DOT) && prefix) {
+ /*
+ * Check for modified files throughout the worktree so
+ * update_callback has a chance to warn about changes
+ * outside the cwd.
+ */
+ data.implicit_dot = prefix;
+ data.implicit_dot_len = strlen(prefix);
+ pathspec = NULL;
+ }
+
init_revisions(&rev, prefix);
setup_revisions(0, NULL, &rev, NULL);
init_pathspec(&rev.prune_data, pathspec);
rev.diffopt.output_format = DIFF_FORMAT_CALLBACK;
rev.diffopt.format_callback = update_callback;
- data.flags = flags;
- data.add_errors = 0;
rev.diffopt.format_callback_data = &data;
rev.max_count = 0; /* do not compare unmerged paths with stage #2 */
run_diff_files(&rev, DIFF_RACY_IS_MODIFIED);
@@ -371,6 +403,7 @@ int cmd_add(int argc, const char **argv, const char *prefix)
int add_new_files;
int require_pathspec;
char *seen = NULL;
+ int implicit_dot = 0;
git_config(add_config, NULL);
@@ -400,10 +433,11 @@ int cmd_add(int argc, const char **argv, const char *prefix)
}
if (option_with_implicit_dot && !argc) {
static const char *here[2] = { ".", NULL };
- if (prefix)
+ if (prefix && addremove)
warn_pathless_add();
argc = 1;
argv = here;
+ implicit_dot = 1;
}
add_new_files = !take_worktree_changes && !refresh_only;
@@ -416,7 +450,8 @@ int cmd_add(int argc, const char **argv, const char *prefix)
(intent_to_add ? ADD_CACHE_INTENT : 0) |
(ignore_add_errors ? ADD_CACHE_IGNORE_ERRORS : 0) |
(!(addremove || take_worktree_changes)
- ? ADD_CACHE_IGNORE_REMOVAL : 0));
+ ? ADD_CACHE_IGNORE_REMOVAL : 0)) |
+ (implicit_dot ? ADD_CACHE_IMPLICIT_DOT : 0);
if (require_pathspec && argc == 0) {
fprintf(stderr, _("Nothing specified, nothing added.\n"));
--
1.8.2.rc3
^ permalink raw reply related
* [PATCH 3/6] add: make warn_pathless_add() a no-op after first call
From: Jonathan Nieder @ 2013-03-19 22:45 UTC (permalink / raw)
To: Junio C Hamano
Cc: Matthieu Moy, Jeff King, git,
Nguyễn Thái Ngọc Duy
In-Reply-To: <20130319224400.GA19014@google.com>
Make warn_pathless_add() print its warning the first time it is called
and do nothing if called again. This will make it easier to show the
warning on the fly when a relevant condition is detected without
risking showing it multiple times when multiple such conditions hold.
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
As before.
builtin/add.c | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/builtin/add.c b/builtin/add.c
index a4028ee..a424e69 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -33,8 +33,13 @@ static const char *short_option_with_implicit_dot;
static void warn_pathless_add(void)
{
+ static int shown;
assert(option_with_implicit_dot && short_option_with_implicit_dot);
+ if (shown)
+ return;
+ shown = 1;
+
/*
* To be consistent with "git add -p" and most Git
* commands, we should default to being tree-wide, but
--
1.8.2.rc3
^ permalink raw reply related
* [PATCH 2/6] add: make pathless 'add [-u|-A]' warning a file-global function
From: Jonathan Nieder @ 2013-03-19 22:45 UTC (permalink / raw)
To: Junio C Hamano
Cc: Matthieu Moy, Jeff King, git,
Nguyễn Thái Ngọc Duy
In-Reply-To: <20130319224400.GA19014@google.com>
Currently warn_pathless_add() is only called directly by cmd_add(),
but that is about to change. Move its definition higher in the file
and pass the "--update" or "--all" option name used in its message
through globals instead of function arguments to make it easier to
call without passing values that will not change through the call
chain.
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
Unchanged.
builtin/add.c | 69 +++++++++++++++++++++++++++++++----------------------------
1 file changed, 36 insertions(+), 33 deletions(-)
diff --git a/builtin/add.c b/builtin/add.c
index ab1c9e8..a4028ee 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -28,6 +28,41 @@ struct update_callback_data {
int add_errors;
};
+static const char *option_with_implicit_dot;
+static const char *short_option_with_implicit_dot;
+
+static void warn_pathless_add(void)
+{
+ assert(option_with_implicit_dot && short_option_with_implicit_dot);
+
+ /*
+ * To be consistent with "git add -p" and most Git
+ * commands, we should default to being tree-wide, but
+ * this is not the original behavior and can't be
+ * changed until users trained themselves not to type
+ * "git add -u" or "git add -A". For now, we warn and
+ * keep the old behavior. Later, the behavior can be changed
+ * to tree-wide, keeping the warning for a while, and
+ * eventually we can drop the warning.
+ */
+ warning(_("The behavior of 'git add %s (or %s)' with no path argument from a\n"
+ "subdirectory of the tree will change in Git 2.0 and should not be used anymore.\n"
+ "To add content for the whole tree, run:\n"
+ "\n"
+ " git add %s :/\n"
+ " (or git add %s :/)\n"
+ "\n"
+ "To restrict the command to the current directory, run:\n"
+ "\n"
+ " git add %s .\n"
+ " (or git add %s .)\n"
+ "\n"
+ "With the current Git version, the command is restricted to the current directory."),
+ option_with_implicit_dot, short_option_with_implicit_dot,
+ option_with_implicit_dot, short_option_with_implicit_dot,
+ option_with_implicit_dot, short_option_with_implicit_dot);
+}
+
static int fix_unmerged_status(struct diff_filepair *p,
struct update_callback_data *data)
{
@@ -321,35 +356,6 @@ static int add_files(struct dir_struct *dir, int flags)
return exit_status;
}
-static void warn_pathless_add(const char *option_name, const char *short_name) {
- /*
- * To be consistent with "git add -p" and most Git
- * commands, we should default to being tree-wide, but
- * this is not the original behavior and can't be
- * changed until users trained themselves not to type
- * "git add -u" or "git add -A". For now, we warn and
- * keep the old behavior. Later, the behavior can be changed
- * to tree-wide, keeping the warning for a while, and
- * eventually we can drop the warning.
- */
- warning(_("The behavior of 'git add %s (or %s)' with no path argument from a\n"
- "subdirectory of the tree will change in Git 2.0 and should not be used anymore.\n"
- "To add content for the whole tree, run:\n"
- "\n"
- " git add %s :/\n"
- " (or git add %s :/)\n"
- "\n"
- "To restrict the command to the current directory, run:\n"
- "\n"
- " git add %s .\n"
- " (or git add %s .)\n"
- "\n"
- "With the current Git version, the command is restricted to the current directory."),
- option_name, short_name,
- option_name, short_name,
- option_name, short_name);
-}
-
int cmd_add(int argc, const char **argv, const char *prefix)
{
int exit_status = 0;
@@ -360,8 +366,6 @@ int cmd_add(int argc, const char **argv, const char *prefix)
int add_new_files;
int require_pathspec;
char *seen = NULL;
- const char *option_with_implicit_dot = NULL;
- const char *short_option_with_implicit_dot = NULL;
git_config(add_config, NULL);
@@ -392,8 +396,7 @@ int cmd_add(int argc, const char **argv, const char *prefix)
if (option_with_implicit_dot && !argc) {
static const char *here[2] = { ".", NULL };
if (prefix)
- warn_pathless_add(option_with_implicit_dot,
- short_option_with_implicit_dot);
+ warn_pathless_add();
argc = 1;
argv = here;
}
--
1.8.2.rc3
^ permalink raw reply related
* [PATCH 1/6] t2200: check that "add -u" limits itself to subdirectory
From: Jonathan Nieder @ 2013-03-19 22:44 UTC (permalink / raw)
To: Junio C Hamano
Cc: Matthieu Moy, Jeff King, git,
Nguyễn Thái Ngọc Duy
In-Reply-To: <20130319224400.GA19014@google.com>
From: Jeff King <peff@peff.net>
Date: Thu, 14 Mar 2013 02:44:04 -0400
This behavior is due to change in the future, but let's test
it anyway. That helps make sure we do not accidentally
switch the behavior too soon while we are working in the
area, and it means that we can easily verify the change when
we do make it.
Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
Unchanged, only included for reference.
t/t2200-add-update.sh | 16 ++++++++++++++++
1 file changed, 16 insertions(+)
diff --git a/t/t2200-add-update.sh b/t/t2200-add-update.sh
index 4cdebda..c317254 100755
--- a/t/t2200-add-update.sh
+++ b/t/t2200-add-update.sh
@@ -80,6 +80,22 @@ test_expect_success 'change gets noticed' '
'
+# Note that this is scheduled to change in Git 2.0, when
+# "git add -u" will become full-tree by default.
+test_expect_success 'non-limited update in subdir leaves root alone' '
+ (
+ cd dir1 &&
+ echo even more >>sub2 &&
+ git add -u
+ ) &&
+ cat >expect <<-\EOF &&
+ check
+ top
+ EOF
+ git diff-files --name-only >actual &&
+ test_cmp expect actual
+'
+
test_expect_success SYMLINKS 'replace a file with a symlink' '
rm foo &&
--
1.8.2.rc3
^ permalink raw reply related
* [PATCH v2 0/6] make pathless 'add [-u|-A]' warning less noisy
From: Jonathan Nieder @ 2013-03-19 22:44 UTC (permalink / raw)
To: Junio C Hamano
Cc: Matthieu Moy, Jeff King, git,
Nguyễn Thái Ngọc Duy
In-Reply-To: <1362786889-28688-1-git-send-email-gitster@pobox.com>
As promised, here is a rerolled version of the "make pathless 'add
[-u|-A]' warning less noisy", incorporating patches from
jc/add-2.0-u-A-sans-pathspec. Thanks for your help so far.
Just like jc/add-2.0-u-A-sans-pathspec, the only transition in this
series is the "pull the bandaid" kind. That is, there are two steps:
0. the current state, where the warning is a little too noisy
1. the current state but with the warning only firing in cases
where the user will be affected by the change to default
'git add -u' behavior
2. no more warning, 'git add -u' defaulting to 'git add -u :/'
Patch 1 is the same as in jc/add-2.0-u-A-sans-pathspec, included for
reference.
Patches 2-5 correspond to the original patches 1-4. Any changes are
described after the '---' in each patch.
Patch 6 is just the patch from the tip of jc/add-2.0-u-A-sans-pathspec,
rebased. It is meant to be applied in the 2.0 cycle.
Jeff King (1):
t2200: check that "add -u" limits itself to subdirectory
Jonathan Nieder (4):
add: make pathless 'add [-u|-A]' warning a file-global function
add: make warn_pathless_add() a no-op after first call
add -u: only show pathless 'add -u' warning when changes exist outside
cwd
add -A: only show pathless 'add -A' warning when changes exist outside
cwd
Junio C Hamano (1):
git add: -u/-A now affects the entire working tree
Documentation/git-add.txt | 16 +++++++-------
builtin/add.c | 56 ++++++++---------------------------------------
t/t2200-add-update.sh | 11 ++++++++++
3 files changed, 28 insertions(+), 55 deletions(-)
^ permalink raw reply
* [PATCH] sha1_file: remove recursion in packed_object_info
From: Thomas Rast @ 2013-03-19 22:08 UTC (permalink / raw)
To: Junio C Hamano
Cc: Stefan Zager, git, Jeff King,
Nguyễn Thái Ngọc Duy
In-Reply-To: <7v1ubbjmq7.fsf@alter.siamese.dyndns.org>
packed_object_info() and packed_delta_info() were mutually recursive.
The former would handle ordinary types and defer deltas to the latter;
the latter would use the former to resolve the delta base.
This arrangement, however, leads to trouble with threaded index-pack
and long delta chains on platforms where thread stacks are small, as
happened on OS X (512kB thread stacks by default) with the chromium
repo.
The task of the two functions is not all that hard to describe without
any recursion, however. It proceeds in three steps:
- determine the representation type and size, based on the outermost
object (delta or not)
- follow through the delta chain, if any
- determine the object type from what is found at the end of the delta
chain
The only complication stems from the error recovery. If parsing fails
at any step, we want to mark that object (within the pack) as bad and
try getting the corresponding SHA1 from elsewhere. If that also
fails, we want to repeat this process back up the delta chain until we
find a reasonable solution or conclude that there is no way to
reconstruct the object. (This is conveniently checked by t5303.)
To achieve that within the pack, we keep track of the entire delta
chain in a stack. When things go sour, we process that stack from the
top, marking entries as bad and attempting to re-resolve by sha1. To
avoid excessive malloc(), the stack starts out with a small
stack-allocated array. The choice of 64 is based on the default of
pack.depth, which is 50, in the hope that it covers "most" delta
chains without any need for malloc().
It's much harder to make the actual re-resolving by sha1 nonrecursive,
so we skip that. If you can't afford *that* recursion, your
corruption problems are more serious than your stack size problems.
Reported-by: Stefan Zager <szager@google.com>
Signed-off-by: Thomas Rast <trast@student.ethz.ch>
---
Junio C Hamano <gitster@pobox.com> writes:
> Thomas Rast <trast@student.ethz.ch> writes:
>
> > Actually scratch that again. It *is* a stack overflow, except that this
> > is a thread stack, for which the OS X defaults are 512kB apparently, as
> > opposed to 2MB on linux.
> > ...
> > And indeed the following patch fixes it. Sounds like the delta
> > unpacking needs a rewrite to support stackless operation. Sigh.
>
> Yikes. Thanks for digging it to the bottom.
So here's a nonrecursive version. Dijkstra is probably turning over
in his grave as we speak.
I *think* I actually got it right. It passes tests in any case. ;-)
The sad part is, after all the effort it still bombs out in the same
place because of the very similar recursion in unpack_entry(). I'll
save that for another day.
sha1_file.c | 132 +++++++++++++++++++++++++++++++++++++-----------------------
1 file changed, 81 insertions(+), 51 deletions(-)
diff --git a/sha1_file.c b/sha1_file.c
index 16967d3..54e92a2 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -1639,50 +1639,6 @@ static off_t get_delta_base(struct packed_git *p,
return base_offset;
}
-/* forward declaration for a mutually recursive function */
-static int packed_object_info(struct packed_git *p, off_t offset,
- unsigned long *sizep, int *rtype);
-
-static int packed_delta_info(struct packed_git *p,
- struct pack_window **w_curs,
- off_t curpos,
- enum object_type type,
- off_t obj_offset,
- unsigned long *sizep)
-{
- off_t base_offset;
-
- base_offset = get_delta_base(p, w_curs, &curpos, type, obj_offset);
- if (!base_offset)
- return OBJ_BAD;
- type = packed_object_info(p, base_offset, NULL, NULL);
- if (type <= OBJ_NONE) {
- struct revindex_entry *revidx;
- const unsigned char *base_sha1;
- revidx = find_pack_revindex(p, base_offset);
- if (!revidx)
- return OBJ_BAD;
- base_sha1 = nth_packed_object_sha1(p, revidx->nr);
- mark_bad_packed_object(p, base_sha1);
- type = sha1_object_info(base_sha1, NULL);
- if (type <= OBJ_NONE)
- return OBJ_BAD;
- }
-
- /* We choose to only get the type of the base object and
- * ignore potentially corrupt pack file that expects the delta
- * based on a base with a wrong size. This saves tons of
- * inflate() calls.
- */
- if (sizep) {
- *sizep = get_size_from_delta(p, w_curs, curpos);
- if (*sizep == 0)
- type = OBJ_BAD;
- }
-
- return type;
-}
-
int unpack_object_header(struct packed_git *p,
struct pack_window **w_curs,
off_t *curpos,
@@ -1709,6 +1665,25 @@ int unpack_object_header(struct packed_git *p,
return type;
}
+static int retry_bad_packed_offset(struct packed_git *p, off_t obj_offset)
+{
+ int type;
+ struct revindex_entry *revidx;
+ const unsigned char *sha1;
+ revidx = find_pack_revindex(p, obj_offset);
+ if (!revidx)
+ return OBJ_BAD;
+ sha1 = nth_packed_object_sha1(p, revidx->nr);
+ mark_bad_packed_object(p, sha1);
+ type = sha1_object_info(sha1, NULL);
+ if (type <= OBJ_NONE)
+ return OBJ_BAD;
+ return type;
+}
+
+
+#define POI_STACK_PREALLOC 64
+
static int packed_object_info(struct packed_git *p, off_t obj_offset,
unsigned long *sizep, int *rtype)
{
@@ -1716,31 +1691,86 @@ static int packed_object_info(struct packed_git *p, off_t obj_offset,
unsigned long size;
off_t curpos = obj_offset;
enum object_type type;
+ off_t small_poi_stack[POI_STACK_PREALLOC];
+ off_t *poi_stack = small_poi_stack;
+ int poi_stack_nr = 0, poi_stack_alloc = POI_STACK_PREALLOC;
type = unpack_object_header(p, &w_curs, &curpos, &size);
+
if (rtype)
*rtype = type; /* representation type */
+ if (sizep) {
+ if (type == OBJ_OFS_DELTA || type == OBJ_REF_DELTA) {
+ off_t tmp_pos = curpos;
+ get_delta_base(p, &w_curs, &tmp_pos, type, obj_offset);
+ *sizep = get_size_from_delta(p, &w_curs, tmp_pos);
+ if (*sizep == 0) {
+ type = OBJ_BAD;
+ goto out;
+ }
+ } else {
+ *sizep = size;
+ }
+ }
+
+ while (type == OBJ_OFS_DELTA || type == OBJ_REF_DELTA) {
+ off_t base_offset;
+ /* Push the object we're going to leave behind */
+ if (poi_stack_nr >= poi_stack_alloc && poi_stack == small_poi_stack) {
+ poi_stack_alloc = alloc_nr(poi_stack_nr);
+ poi_stack = xmalloc(sizeof(off_t)*poi_stack_alloc);
+ memcpy(poi_stack, small_poi_stack, sizeof(off_t)*poi_stack_nr);
+ } else {
+ ALLOC_GROW(poi_stack, poi_stack_nr+1, poi_stack_alloc);
+ }
+ poi_stack[poi_stack_nr++] = obj_offset;
+ /* If parsing the base offset fails, just unwind */
+ base_offset = get_delta_base(p, &w_curs, &curpos, type, obj_offset);
+ if (!base_offset)
+ goto unwind;
+ curpos = obj_offset = base_offset;
+ type = unpack_object_header(p, &w_curs, &curpos, &size);
+ if (type <= OBJ_NONE) {
+ /* If getting the base itself fails, we first
+ * retry the base, otherwise unwind */
+ type = retry_bad_packed_offset(p, base_offset);
+ if (type > OBJ_NONE)
+ goto out;
+ goto unwind;
+ }
+ }
+
switch (type) {
- case OBJ_OFS_DELTA:
- case OBJ_REF_DELTA:
- type = packed_delta_info(p, &w_curs, curpos,
- type, obj_offset, sizep);
- break;
+ case OBJ_BAD:
case OBJ_COMMIT:
case OBJ_TREE:
case OBJ_BLOB:
case OBJ_TAG:
- if (sizep)
- *sizep = size;
break;
default:
error("unknown object type %i at offset %"PRIuMAX" in %s",
type, (uintmax_t)obj_offset, p->pack_name);
type = OBJ_BAD;
}
+
+out:
+ if (poi_stack != small_poi_stack)
+ free(poi_stack);
unuse_pack(&w_curs);
return type;
+
+unwind:
+ while (poi_stack_nr) {
+ obj_offset = poi_stack[poi_stack_nr--];
+ type = retry_bad_packed_offset(p, obj_offset);
+ if (type > OBJ_NONE)
+ goto out;
+ }
+ if (poi_stack != small_poi_stack)
+ free(poi_stack);
+ unuse_pack(&w_curs);
+ return OBJ_BAD;
}
static void *unpack_compressed_entry(struct packed_git *p,
--
1.8.2.490.g25051b0
^ permalink raw reply related
* RE: FW: Windows. Git, and Dedupe
From: Josh Rowe @ 2013-03-19 21:36 UTC (permalink / raw)
To: René Scharfe; +Cc: git@vger.kernel.org, msysgit@googlegroups.com
In-Reply-To: <5148D3BD.4040608@lsrfire.ath.cx>
Yes, Dedup is in fact a Server-only feature. However, there are lots of people using the Server SKU as development workstations (especially here at Microsoft <g>). There are also some sysadmins that I know of who use git and download sysadmin scripts via git to Servers. Finally, I would hazard a guess that it's possible to mount an NTFS filesystem containing deduped files from a Server machine onto a client SKU and access those files. (I'm not on the NTFS team, and haven't tried it.) So I think there are good reasons to support reparse points on Windows.
The reparse point could be decoded as being a non-symlink reparse item using; in those cases, treating the file as an "ordinary" file would be appropriate.
For example, see the following. The reparse tag value for symlinks is IO_REPARSE_TAG_SYMLINK (0xa000000c) and for deduped files is (IO_REPARSE_TAG_DEDUP) 0x80000013. The value can be discovered from the information at [1].
I admin to not having looked at the git code nor being familiar with mingw. Are native Win32 calls supported in the git codebase?
Jmr
[1] http://msdn.microsoft.com/en-us/library/windows/desktop/aa365740(v=vs.85).aspx
PS I:\temp> cmd /c mklink x y
symbolic link created for x <<===>> y
PS I:\temp> fsutil reparsepoint query x
Reparse Tag Value : 0xa000000c
Tag value: Microsoft
Tag value: Name Surrogate
Tag value: Symbolic Link
Reparse Data Length: 0x00000010
Reparse Data:
0000: 02 00 02 00 00 00 02 00 01 00 00 00 79 00 79 00 ............y.y.
PS I:\temp> fsutil reparsepoint query x.txt
Reparse Tag Value : 0x80000013
Tag value: Microsoft
Reparse Data Length: 0x0000007c
Reparse Data:
0000: 01 02 7c 00 00 00 00 00 66 9c 1a 01 00 00 00 00 ..|.....f.......
0010: 00 00 01 00 00 00 00 00 cb eb c5 00 6a 97 63 4d ............j.cM
0020: 97 9c 13 0c 41 8e ed 8b 40 00 40 00 40 00 00 00 ....A...@.@.@...
0030: d3 b9 a8 d4 e4 c6 cd 01 55 ca 02 00 00 00 05 00 ........U.......
0040: 70 ac 21 04 00 00 05 00 01 00 00 00 88 8d 00 00 p.!.............
0050: c8 30 00 00 00 00 00 00 c8 44 db 94 6c 88 9a d4 .0.......D..l...
0060: 0a a9 01 3a 1f 80 80 8d ea 0d 53 d7 36 49 b9 a4 ...:......S.6I..
0070: 82 a2 b9 4e 2a 16 4b a1 2e d9 f3 dd ...N*.K.....
-----Original Message-----
From: René Scharfe [mailto:rene.scharfe@lsrfire.ath.cx]
Sent: Tuesday, March 19, 2013 2:08 PM
To: Josh Rowe
Cc: git@vger.kernel.org; msysgit@googlegroups.com
Subject: Re: FW: Windows. Git, and Dedupe
Am 18.03.2013 22:20, schrieb Josh Rowe:
> On Windows with an NTFS volume with Deduplication enabled, Git
> believes that deduplicated files are symlinks. It then fails to be
> able to do anything with the file. This can be repro-ed by creating
> an NTFS volume with dedup, creating some duplicate files, verifying
> that a few files are deduped, and trying to add and commit the files
> via git.
Both Single Instance Storage[1] and Data Deduplication[2] (introduced with Windows Server 2012) seem to be server-only features. How about keeping regular git repositories with checked-out files on client disks and use the server only for bare repositories (without working tree)?
When I tried to add a symbolic link created with mklink on Windows 8, the mingw version of git refused because readlink(2) is not supported. This seems to be sufficient to reproduce the issue.
I couldn't test the Cygwin version, though, because http://cygwin.com doesn't respond at the moment.
But a working readlink(2) wouldn't help anyway, I guess. I imagine that the reparse points used for deduplication point into a magic block store which performs garbage collection of content that is no longer referenced -- which probably means that a recreated "symlink"
may point to blocks that have been deleted in the meantime.
Perhaps you need a way to ask git to always follow symlinks instead of trying to store their target specification.
René
[1] http://technet.microsoft.com/en-us/library/dd573308%28v=ws.10%29.aspx
[2] http://msdn.microsoft.com/en-us/library/windows/desktop/hh769303%28v=vs.85%29.aspx
^ permalink raw reply
* Re: [RFC] test-lib.sh: No POSIXPERM for cygwin
From: Junio C Hamano @ 2013-03-19 21:10 UTC (permalink / raw)
To: Torsten Bögershausen; +Cc: Ramsay Jones, git, j6t
In-Reply-To: <7v620nhzle.fsf@alter.siamese.dyndns.org>
Junio C Hamano <gitster@pobox.com> writes:
> Torsten Bögershausen <tboegi@web.de> writes:
>
>> Use a compile switch IGNORECYGWINFSTRICKS to disable the usage
>> of cygwin_lstat_fn() only in path.c
>
> The analysis of the problem and the basic idea to disable the
> fast-but-lying fstricks in the code that matters may be good, but
> the implementation is questionable.
>
> What if we later need to move functions around, etc., so that some
> other calls in path.c still do want to use the fstricks bit while
> the existing ones in the file want the real lstat() information?
>
> Actually, that already is the case. The call to lstat() in
> validate_headref() only cares about the S_ISXXX() type and can
> afford to use the fast-and-lying one, no?
>
> How about doing something like this in the generic codepath, and
> implement your own cygwin_true_mode_bits() function at the cygwin
> compatibility layer, and add
>
> #define true_mode_bits cygwin_true_mode_bits
>
> in the compat/cygwin.h file? The change has the documentation value
> to clarify what each lstat() is used for, too.
>
> Ideally, the "#ifndef true_mode_bits" part may want to become a
> generic helper function if there are lstat() calls in other files
> that cygwin wants to use the real lstat() not the fast-but-lying
> one, but one step at a time.
>
> Hrm?
>
> path.c | 18 ++++++++++++++++--
> 1 file changed, 16 insertions(+), 2 deletions(-)
>
> diff --git a/path.c b/path.c
> index d3d3f8b..d0b31e5 100644
> --- a/path.c
> +++ b/path.c
> @@ -14,6 +14,21 @@
> #include "strbuf.h"
> #include "string-list.h"
>
> +#ifndef true_mode_bits
> +/*
> + * The replacement lstat(2) we use on Cygwin is incomplete and
> + * lies about permission bits; most of the time we do not care,
> + * but the callsites of this wrpper do care.
> + */
> +static int true_mode_bits(const char *path, int *mode)
> +{
> + struct stat st;
> + if (lstat(path, &st) < 0)
> + return -1;
> + return st.st_mode;
Of course this should be more like
*mode = st.st_mode;
return 0;
but I think you got the idea ;-)
> +}
> +#endif
> +
> static char bad_path[] = "/bad-path/";
>
> static char *get_pathname(void)
> @@ -400,9 +415,8 @@ int set_shared_perm(const char *path, int mode)
> return 0;
> }
> if (!mode) {
> - if (lstat(path, &st) < 0)
> + if (true_mode_bits(path, &mode) < 0)
> return -1;
> - mode = st.st_mode;
> orig_mode = mode;
> } else
> orig_mode = 0;
^ permalink raw reply
* Re: FW: Windows. Git, and Dedupe
From: René Scharfe @ 2013-03-19 21:08 UTC (permalink / raw)
To: Josh Rowe; +Cc: git@vger.kernel.org, msysgit
In-Reply-To: <02803146dc74414191fab53b0de33fa1@BLUPR03MB019.namprd03.prod.outlook.com>
Am 18.03.2013 22:20, schrieb Josh Rowe:
> On Windows with an NTFS volume with Deduplication enabled, Git
> believes that deduplicated files are symlinks. It then fails to be
> able to do anything with the file. This can be repro-ed by creating
> an NTFS volume with dedup, creating some duplicate files, verifying
> that a few files are deduped, and trying to add and commit the files
> via git.
Both Single Instance Storage[1] and Data Deduplication[2] (introduced
with Windows Server 2012) seem to be server-only features. How about
keeping regular git repositories with checked-out files on client
disks and use the server only for bare repositories (without working
tree)?
When I tried to add a symbolic link created with mklink on Windows 8,
the mingw version of git refused because readlink(2) is not
supported. This seems to be sufficient to reproduce the issue.
I couldn't test the Cygwin version, though, because http://cygwin.com
doesn't respond at the moment.
But a working readlink(2) wouldn't help anyway, I guess. I imagine
that the reparse points used for deduplication point into a magic
block store which performs garbage collection of content that is no
longer referenced -- which probably means that a recreated "symlink"
may point to blocks that have been deleted in the meantime.
Perhaps you need a way to ask git to always follow symlinks instead
of trying to store their target specification.
René
[1] http://technet.microsoft.com/en-us/library/dd573308%28v=ws.10%29.aspx
[2] http://msdn.microsoft.com/en-us/library/windows/desktop/hh769303%28v=vs.85%29.aspx
^ permalink raw reply
* Re: [RFC] test-lib.sh: No POSIXPERM for cygwin
From: Junio C Hamano @ 2013-03-19 21:03 UTC (permalink / raw)
To: Torsten Bögershausen; +Cc: Ramsay Jones, git, j6t
In-Reply-To: <201303192049.58755.tboegi@web.de>
Torsten Bögershausen <tboegi@web.de> writes:
> Use a compile switch IGNORECYGWINFSTRICKS to disable the usage
> of cygwin_lstat_fn() only in path.c
The analysis of the problem and the basic idea to disable the
fast-but-lying fstricks in the code that matters may be good, but
the implementation is questionable.
What if we later need to move functions around, etc., so that some
other calls in path.c still do want to use the fstricks bit while
the existing ones in the file want the real lstat() information?
Actually, that already is the case. The call to lstat() in
validate_headref() only cares about the S_ISXXX() type and can
afford to use the fast-and-lying one, no?
How about doing something like this in the generic codepath, and
implement your own cygwin_true_mode_bits() function at the cygwin
compatibility layer, and add
#define true_mode_bits cygwin_true_mode_bits
in the compat/cygwin.h file? The change has the documentation value
to clarify what each lstat() is used for, too.
Ideally, the "#ifndef true_mode_bits" part may want to become a
generic helper function if there are lstat() calls in other files
that cygwin wants to use the real lstat() not the fast-but-lying
one, but one step at a time.
Hrm?
path.c | 18 ++++++++++++++++--
1 file changed, 16 insertions(+), 2 deletions(-)
diff --git a/path.c b/path.c
index d3d3f8b..d0b31e5 100644
--- a/path.c
+++ b/path.c
@@ -14,6 +14,21 @@
#include "strbuf.h"
#include "string-list.h"
+#ifndef true_mode_bits
+/*
+ * The replacement lstat(2) we use on Cygwin is incomplete and
+ * lies about permission bits; most of the time we do not care,
+ * but the callsites of this wrpper do care.
+ */
+static int true_mode_bits(const char *path, int *mode)
+{
+ struct stat st;
+ if (lstat(path, &st) < 0)
+ return -1;
+ return st.st_mode;
+}
+#endif
+
static char bad_path[] = "/bad-path/";
static char *get_pathname(void)
@@ -400,9 +415,8 @@ int set_shared_perm(const char *path, int mode)
return 0;
}
if (!mode) {
- if (lstat(path, &st) < 0)
+ if (true_mode_bits(path, &mode) < 0)
return -1;
- mode = st.st_mode;
orig_mode = mode;
} else
orig_mode = 0;
^ permalink raw reply related
* Re: [PATCH 3/4] add -u: only show pathless 'add -u' warning when changes exist outside cwd
From: Jonathan Nieder @ 2013-03-19 20:34 UTC (permalink / raw)
To: Junio C Hamano
Cc: Matthieu Moy, Jeff King, git,
Nguyễn Thái Ngọc Duy
In-Reply-To: <7vd2uvi33w.fsf@alter.siamese.dyndns.org>
Junio C Hamano wrote:
> Incidentally, I am rebuilding the 'next' by kicking many of the
> topics back to 'pu' (essentially, only the ones marked as "Safe" in
> the latest issue of "What's cooking" are kept in 'next'), so perhaps
> we can rebuild the jc/add-2.0-u-A-sans-pathspec topic with your
> series at the bottom, or something? I do not think I have time to
> get around to it myself until later in the evening, though.
Sounds good. I'll reroll with the use-prefix-not-pathspec tweak and
incorporate the current patches from jc/add-2.0-u-A-sans-pathspec.
^ permalink raw reply
* git-code-review
From: Jakub Suder @ 2013-03-19 20:22 UTC (permalink / raw)
To: git
Hi,
I thought someone might be interested here - I wrote a small thingie
in Bash to help me with daily code reviews in my project using git and
optionally GitHub. It basically just creates a mirror of
.git/refs/remotes in .git/review, except it's only updated when you
run the tool, so you can use it to keep track of what was already
reviewed and what wasn't, and review e.g. commits from the last day
every morning (and still be able to run git pull/fetch whenever you
need).
Here's the link: https://github.com/jsuder/git-code-review
--
Kuba Suder
^ permalink raw reply
* Re: [RFC] test-lib.sh: No POSIXPERM for cygwin
From: Torsten Bögershausen @ 2013-03-19 19:49 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Ramsay Jones, git, j6t, tboegi
On Friday 08 February 2013 07:08:14 Torsten Bögershausen wrote:
> On 07.02.13 20:35, Junio C Hamano wrote:
> > Ramsay Jones <ramsay@ramsay1.demon.co.uk> writes:
> >
> >> Torsten Bögershausen wrote:
> >>> t0070 and t1301 fail when running the test suite under cygwin.
> >>> Skip the failing tests by unsetting POSIXPERM.
> >> t1301 does not fail for me. (WIN XP (SP3) on NTFS)
> > Others run Cygwin with vfat or some other filesystem, and some of
> > them do not cope will with POSIXPERM, perhaps?
> >
> > Not having POSIXPERM by default for Cygwin may be a saner default
> > than having one, if we have to pick one.
> >
> > It may be debatable to have this default as platform attribute,
> > though.
> >
> Yes, 1301 passes on cygwin 1.5, but not on 1.7.
> And it breaks on VFAT, for all kind of OS.
>
> Thanks for comments, I'll put more investigations on my todo stack.
> /Torsten
It turns out that cygwin 1.7 handles the read only attribute different then
cygwin 1.5.
Under cygwin 1.5 the "read only" attribute is synchronized with the
"user write" bit from the permissions.
tb@PC /cygdrive/c/temp/pt
$ echo x >x
$ chmod 444 x
$ attrib x
A R C:\temp\pt\x
Cygwin 1.7 does not do the synchronization, which means that some files
seem to be writable when do_stat() is used, but the permissions are r--r--r--
We can improve Git to work for cygwin 1.7, a suggestion for a patch
may look like this:
-- >8 --
Subject: [PATCH] Make core.sharedRepository work under cygwin 1.7
When core.sharedRepository is used, set_shared_perm() in path.c
needs lstat() to return the correct POSIX permissions.
The default for cygwin is core.ignoreCygwinFSTricks = false, which
means that a simplified and fast implementation of lstat() is used.
Especially the file permission bits are wrong in cygwin_lstat_fn():
The read-only attribute of a file is used to calculate
the permissions, resulting in either rw-r--r-- or r--r--r--
Use a compile switch IGNORECYGWINFSTRICKS to disable the usage
of cygwin_lstat_fn() only in path.c
Signed-off-by: Torsten Bögershausen <tboegi@web.de>
---
compat/cygwin.h | 2 ++
path.c | 2 ++
2 files changed, 4 insertions(+)
diff --git a/compat/cygwin.h b/compat/cygwin.h
index a3229f5..984efbe 100644
--- a/compat/cygwin.h
+++ b/compat/cygwin.h
@@ -5,5 +5,7 @@ typedef int (*stat_fn_t)(const char*, struct stat*);
extern stat_fn_t cygwin_stat_fn;
extern stat_fn_t cygwin_lstat_fn;
+#ifndef IGNORECYGWINFSTRICKS
#define stat(path, buf) (*cygwin_stat_fn)(path, buf)
#define lstat(path, buf) (*cygwin_lstat_fn)(path, buf)
+#endif
diff --git a/path.c b/path.c
index d3d3f8b..0acfabf 100644
--- a/path.c
+++ b/path.c
@@ -10,6 +10,8 @@
*
* which is what it's designed for.
*/
+#define IGNORECYGWINFSTRICKS
+
#include "cache.h"
#include "strbuf.h"
#include "string-list.h"
--
1.8.2.341.g543621f
^ permalink raw reply related
* Re: [PATCH 3/4] add -u: only show pathless 'add -u' warning when changes exist outside cwd
From: Junio C Hamano @ 2013-03-19 19:47 UTC (permalink / raw)
To: Jonathan Nieder
Cc: Matthieu Moy, Jeff King, git,
Nguyễn Thái Ngọc Duy
In-Reply-To: <20130319190624.GB3655@google.com>
Jonathan Nieder <jrnieder@gmail.com> writes:
> ... So I
> find myself being trained to ignore the warning.
> ... Running a full-tree diff which slows down
> the code that decides whether to print a warning is a good way to
> train people regarding how long to expect a plain "git add -u" to
> take.
Ok, fair enough.
Incidentally, I am rebuilding the 'next' by kicking many of the
topics back to 'pu' (essentially, only the ones marked as "Safe" in
the latest issue of "What's cooking" are kept in 'next'), so perhaps
we can rebuild the jc/add-2.0-u-A-sans-pathspec topic with your
series at the bottom, or something? I do not think I have time to
get around to it myself until later in the evening, though.
^ permalink raw reply
* Re: [PATCH] git-p4: support exclusively locked files
From: Pete Wyckoff @ 2013-03-19 19:23 UTC (permalink / raw)
To: Danny Thomas; +Cc: git
In-Reply-To: <CD6CC10C.1DE4E%danny.thomas@blackboard.com>
Danny.Thomas@blackboard.com wrote on Mon, 18 Mar 2013 09:26 -0400:
> Interesting. 'Implementing sitewide pessimistic locking with p4 typemap',
> http://www.perforce.com/perforce/doc.current/manuals/p4sag/03_superuser.htm
> l seems to suggest this is all that's needed. I believe we're using the
> default configuration, the exclusive lock on all files behaviour was
> researching the exclusive locking problem,
> http://ericlathrop.com/2012/12/how-to-set-up-git-p4-in-windows/, so I
> thought I'd mention it.
>
> You might be onto something w/ fstat, it doesn't include the exclusive
> indicator:
>
> ... type binary+l
>
> Latest P4 client, and fairly recent server build:
>
> P4/DARWIN90X86_64/2012.2/536738 (2012/10/16)
> P4D/LINUX26X86_64/2012.2/538478 (2012/10/16)
Great, thanks for the pointer and explanation. Do you want to
reroll your patch to use fstat? I'll work on the tests, and
also look into potential failure modes of "git p4 submit" when somebody
else has the exclusive file open.
-- Pete
> On 17/03/2013 20:04, "Pete Wyckoff" <pw@padd.com> wrote:
>
> >Danny.Thomas@blackboard.com wrote on Wed, 13 Mar 2013 13:51 -0400:
> >> By default, newly added binary files are exclusively locked by Perforce:
> >>
> >> 'add default change (binary+l) *exclusive*'
> >>
> >> This results in a 'Could not determine file type' error as the regex
> >> expects
> >> the line to end after the file type matching group. Some repositories
> >>are
> >> also configured to always require exclusive locks, so may be a problem
> >>for
> >> all revisions in some cases.
> >
> >Can you explain how to configure p4d to default everything to
> >binary+l? Also, what version are you using ("p4 info")? I'm
> >trying to write a test case for this.
> >
> >I did find a way to play with typemap to get +l, as:
> >
> > ( p4 typemap -o ; printf "\tbinary+l\t//.../bash*" ) | p4 typemap -i
> >
> >With this, the 2011.1 here just says:
> >0
> > tic-git-test$ p4 opened bash
> > //depot/bash#1 - add default change (binary+l)
> >
> >I've not been able to make it say " *exclusive*" too.
> >
> >> result = p4_read_pipe(["opened", wildcard_encode(file)])
> >> - match = re.match(".*\((.+)\)\r?$", result)
> >> + match = re.match(".*\((.+)\)(?:.+)?\r?$", result)
> >
> >I think this whole function would be less brittle
> >using p4's "-G" output, like:
> >
> > d = p4Cmd(["fstat", "-T", "type", wildcard_encode(file)])
> > # some error checking
> > return d['type']
> >
> >But I'm curious if your p4d includes " *exclusive*" in the
> >type there too, in which case we'll have to strip it.
> >
> >Thanks for starting the patch on this. It's good if we can keep
> >others from running into the same problem.
^ permalink raw reply
* Re: [PATCH 3/4] add -u: only show pathless 'add -u' warning when changes exist outside cwd
From: Jonathan Nieder @ 2013-03-19 19:06 UTC (permalink / raw)
To: Junio C Hamano
Cc: Matthieu Moy, Jeff King, git,
Nguyễn Thái Ngọc Duy
In-Reply-To: <7v620nl99g.fsf@alter.siamese.dyndns.org>
Junio C Hamano wrote:
> Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:
>> No time to review the code now. I thought about implementing something
>> like that, but did not do it because I didn't want the change in the
>> code to be too big. At some point, we'll have to remove the warning and
>> it's easier with my version than with yours. But the "damage" to the
>> code do not seem too big, so that's probably OK and will actually reduce
>> the pain for some users.
>
> Getting these warnings is a *good* thing.
>
> You may happen to have no changed path outside the current directory
> with this particular invocation of "git add -u", or you may do, or
> you may not *even* remember if you touched the paths outside.
>
> Training your fingers to type "git add -u ." without having to even
> think, is primarily to help the last case.
The problem is that these warnings are triggering way too often. It
is like the story of the boy who cried "wolf": instead of training
people to type "git add -u .", we are training them to ignore
warnings.
I personally often find myself in the following situation:
$ cd repowithdeepsubdirs/third_party/git
$ ... hack hack hack ...
$ git add -u
The result is a pile of warning text that I cannot convince myself not
to ignore because I already *knew* that the only changes present were
under the cwd. The old and new "git add -u" behaviors always have the
same effect in this case, so the warning is not relevant to me. So I
find myself being trained to ignore the warning.
Presumably habitual Java hackers that do their work in a
com/long/package/name subdirectory of the toplevel would find this
even more annoying.
One important exception is that if "git add -u :/" is slow, users need
to learn to run "git add -u ." to speed the operation up. But I think
that is intuitive already. Running a full-tree diff which slows down
the code that decides whether to print a warning is a good way to
train people regarding how long to expect a plain "git add -u" to
take.
Hoping that clarifies,
Jonathan
^ permalink raw reply
* Re: [PATCH v3 0/5] nd/branch-show-rebase-bisect-state updates
From: Junio C Hamano @ 2013-03-19 18:37 UTC (permalink / raw)
To: Nguyễn Thái Ngọc Duy; +Cc: git, Matthieu Moy, Jonathan Niedier
In-Reply-To: <1363174973-17597-1-git-send-email-pclouds@gmail.com>
Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:
> This round fixes the design issue of read_and_strip_branch(), makes
> use of jc/reflog-reverse-walk and performs cheaper sha-1 check on
> detached commits.
I was running "git bisect start pu jch" (jch is a branch I do not
push out everywhere that is somewhere in the middle of
"master..pu"), and then after bisection finished, saw this:
$ git branch --with 66887099e096f0258a5ef3b1e7458748597bdffe
* (no branch, bisecting jch)
nd/magic-pathspecs
pu
which is obviously bogus. I only said "I know the tip of pu is bad
and the tip of jch is good". If anything, I was bisecting the
breakage in 'pu', not in 'jch'.
I think the message should better say "before you started to bisect
you were on 'jch'" or something instead.
^ permalink raw reply
* Re: [PATCH v1 39/45] parse_pathspec: make sure the prefix part is wildcard-free
From: Junio C Hamano @ 2013-03-19 18:34 UTC (permalink / raw)
To: Nguyễn Thái Ngọc Duy; +Cc: git
In-Reply-To: <1363327620-29017-40-git-send-email-pclouds@gmail.com>
This seems to break t7300-clean.sh #8
^ permalink raw reply
* Re: [regression?] trailing slash required in .gitattributes
From: Jeff King @ 2013-03-19 18:10 UTC (permalink / raw)
To: git; +Cc: Jean-Noël AVILA
In-Reply-To: <20130319175756.GA13760@sigill.intra.peff.net>
On Tue, Mar 19, 2013 at 01:57:56PM -0400, Jeff King wrote:
> Prior to v1.8.1.1, if I did this:
>
> git init
> echo content >foo &&
> mkdir subdir &&
> echo content >subdir/bar &&
> echo "subdir export-ignore" >.gitattributes
> git add . &&
> git commit -m one &&
> git archive HEAD | tar tf -
>
> my archive would contain only "foo" and ".gitattributes", not subdir. As
> of v1.8.1.1, the attribute on subdir is ignored unless it is written
> with a trailing slash, like:
>
> subdir/ export-ignore
>
> The issue bisects to 94bc671 (Add directory pattern matching to
> attributes, 2012-12-08). That commit actually tests not only that
> "subdir/" matches, but also that just "subdir" does not match.
Sorry, I mis-read the tests. They are not testing that "subdir" does not
work, only that "subdir/" will match only a directory, not a regular
file. Which does make sense.
So I think the regression is accidental. And we would want tests like
this on top (which currently fail):
diff --git a/t/t5002-archive-attr-pattern.sh b/t/t5002-archive-attr-pattern.sh
index 0c847fb..3be809c 100755
--- a/t/t5002-archive-attr-pattern.sh
+++ b/t/t5002-archive-attr-pattern.sh
@@ -27,6 +27,10 @@ test_expect_success 'setup' '
echo ignored-only-if-dir/ export-ignore >>.git/info/attributes &&
git add ignored-only-if-dir &&
+ mkdir -p ignored-without-slash &&
+ echo ignored without slash >ignored-without-slash/foo &&
+ git add ignored-without-slash/foo &&
+ echo ignored-without-slash export-ignore >>.git/info/attributes &&
mkdir -p one-level-lower/two-levels-lower/ignored-only-if-dir &&
echo ignored by ignored dir >one-level-lower/two-levels-lower/ignored-only-if-dir/ignored-by-ignored-dir &&
@@ -49,6 +53,8 @@ test_expect_missing archive/ignored-ony-if-dir/ignored-by-ignored-dir
test_expect_exists archive/not-ignored-dir/
test_expect_missing archive/ignored-only-if-dir/
test_expect_missing archive/ignored-ony-if-dir/ignored-by-ignored-dir
+test_expect_missing archive/ignored-without-slash/ &&
+test_expect_missing archive/ignored-without-slash/foo &&
test_expect_exists archive/one-level-lower/
test_expect_missing archive/one-level-lower/two-levels-lower/ignored-only-if-dir/
test_expect_missing archive/one-level-lower/two-levels-lower/ignored-ony-if-dir/ignored-by-ignored-dir
^ permalink raw reply related
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