* [PATCH v3 07/13] exclude: avoid calling prep_exclude on entries of the same directory
From: Nguyễn Thái Ngọc Duy @ 2013-03-12 13:04 UTC (permalink / raw)
To: git; +Cc: Nguyễn Thái Ngọc Duy
In-Reply-To: <1363093500-16796-1-git-send-email-pclouds@gmail.com>
prep_exclude is only necessary when we enter or leave a directory. Now
it's called for every entry in a directory. With this patch, the
number of prep_exclude calls in webkit.git goes from 188k down to
11k. This patch does not make exclude any faster, but it prepares for
making prep_exclude heavier in terms of computation, where a large
number of calls may have bigger impacts.
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
dir.c | 10 +++++++++-
dir.h | 1 +
2 files changed, 10 insertions(+), 1 deletion(-)
diff --git a/dir.c b/dir.c
index 58739f3..f8f7a7e 100644
--- a/dir.c
+++ b/dir.c
@@ -804,7 +804,10 @@ static struct exclude *last_exclude_matching(struct dir_struct *dir,
basename = (basename) ? basename+1 : pathname;
START_CLOCK();
- prep_exclude(dir, pathname, basename-pathname);
+ if (!dir->exclude_prepared) {
+ prep_exclude(dir, pathname, basename-pathname);
+ dir->exclude_prepared = 1;
+ }
STOP_CLOCK(tv_prep_exclude);
START_CLOCK();
@@ -894,6 +897,7 @@ struct exclude *last_exclude_matching_path(struct path_exclude_check *check,
if (ch == '/') {
int dt = DT_DIR;
+ check->dir->exclude_prepared = 0;
exclude = last_exclude_matching(check->dir,
path->buf, path->len,
&dt);
@@ -908,6 +912,7 @@ struct exclude *last_exclude_matching_path(struct path_exclude_check *check,
/* An entry in the index; cannot be a directory with subentries */
strbuf_setlen(path, 0);
+ check->dir->exclude_prepared = 0;
return last_exclude_matching(check->dir, name, namelen, dtype);
}
@@ -1394,6 +1399,7 @@ static int read_directory_recursive(struct dir_struct *dir,
if (!fdir)
goto out;
+ dir->exclude_prepared = 0;
while ((de = readdir(fdir)) != NULL) {
switch (treat_path(dir, de, &path, baselen, simplify)) {
case path_recurse:
@@ -1415,6 +1421,7 @@ static int read_directory_recursive(struct dir_struct *dir,
}
closedir(fdir);
out:
+ dir->exclude_prepared = 0;
strbuf_release(&path);
return contents;
@@ -1486,6 +1493,7 @@ static int treat_leading_path(struct dir_struct *dir,
break;
if (simplify_away(sb.buf, sb.len, simplify))
break;
+ dir->exclude_prepared = 0;
if (treat_one_path(dir, &sb, simplify,
DT_DIR, NULL) == path_ignored)
break; /* do not recurse into it */
diff --git a/dir.h b/dir.h
index 560ade4..0748407 100644
--- a/dir.h
+++ b/dir.h
@@ -86,6 +86,7 @@ struct dir_struct {
/* Exclude info */
const char *exclude_per_dir;
+ int exclude_prepared;
/*
* We maintain three groups of exclude pattern lists:
--
1.8.1.2.536.gf441e6d
^ permalink raw reply related
* [PATCH v3 08/13] exclude: record baselen in the pattern
From: Nguyễn Thái Ngọc Duy @ 2013-03-12 13:04 UTC (permalink / raw)
To: git; +Cc: Nguyễn Thái Ngọc Duy
In-Reply-To: <1363093500-16796-1-git-send-email-pclouds@gmail.com>
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
attr.c | 4 +++-
dir.c | 19 ++++++++++++++-----
dir.h | 6 +++++-
3 files changed, 22 insertions(+), 7 deletions(-)
diff --git a/attr.c b/attr.c
index 1818ba5..b89da33 100644
--- a/attr.c
+++ b/attr.c
@@ -249,12 +249,14 @@ static struct match_attr *parse_attr_line(const char *line, const char *src,
res->u.attr = git_attr_internal(name, namelen);
else {
char *p = (char *)&(res->state[num_attr]);
+ int pattern_baselen;
memcpy(p, name, namelen);
res->u.pat.pattern = p;
parse_exclude_pattern(&res->u.pat.pattern,
&res->u.pat.patternlen,
&res->u.pat.flags,
- &res->u.pat.nowildcardlen);
+ &res->u.pat.nowildcardlen,
+ &pattern_baselen);
if (res->u.pat.flags & EXC_FLAG_MUSTBEDIR)
res->u.pat.patternlen++;
if (res->u.pat.flags & EXC_FLAG_NEGATIVE) {
diff --git a/dir.c b/dir.c
index f8f7a7e..932fd2f 100644
--- a/dir.c
+++ b/dir.c
@@ -390,10 +390,11 @@ static int no_wildcard(const char *string)
void parse_exclude_pattern(const char **pattern,
int *patternlen,
int *flags,
- int *nowildcardlen)
+ int *nowildcardlen,
+ int *pattern_baselen)
{
const char *p = *pattern;
- size_t i, len;
+ int i, len;
*flags = 0;
if (*p == '!') {
@@ -405,12 +406,15 @@ void parse_exclude_pattern(const char **pattern,
len--;
*flags |= EXC_FLAG_MUSTBEDIR;
}
- for (i = 0; i < len; i++) {
+ for (i = len - 1; i >= 0; i--) {
if (p[i] == '/')
break;
}
- if (i == len)
+ if (i < 0) {
*flags |= EXC_FLAG_NODIR;
+ *pattern_baselen = -1;
+ } else
+ *pattern_baselen = i;
*nowildcardlen = simple_length(p);
/*
* we should have excluded the trailing slash from 'p' too,
@@ -421,6 +425,8 @@ void parse_exclude_pattern(const char **pattern,
*nowildcardlen = len;
if (*p == '*' && no_wildcard(p + 1))
*flags |= EXC_FLAG_ENDSWITH;
+ else if (*nowildcardlen != len)
+ *pattern_baselen = -1;
*pattern = p;
*patternlen = len;
}
@@ -432,8 +438,10 @@ void add_exclude(const char *string, const char *base,
int patternlen;
int flags;
int nowildcardlen;
+ int pattern_baselen;
- parse_exclude_pattern(&string, &patternlen, &flags, &nowildcardlen);
+ parse_exclude_pattern(&string, &patternlen, &flags,
+ &nowildcardlen, &pattern_baselen);
if (flags & EXC_FLAG_MUSTBEDIR) {
char *s;
x = xmalloc(sizeof(*x) + patternlen + 1);
@@ -449,6 +457,7 @@ void add_exclude(const char *string, const char *base,
x->nowildcardlen = nowildcardlen;
x->base = base;
x->baselen = baselen;
+ x->pattern_baselen = pattern_baselen;
x->flags = flags;
x->srcpos = srcpos;
ALLOC_GROW(el->excludes, el->nr + 1, el->alloc);
diff --git a/dir.h b/dir.h
index 0748407..cb50a85 100644
--- a/dir.h
+++ b/dir.h
@@ -44,6 +44,7 @@ struct exclude_list {
int nowildcardlen;
const char *base;
int baselen;
+ int pattern_baselen;
int flags;
/*
@@ -172,7 +173,10 @@ extern struct exclude_list *add_exclude_list(struct dir_struct *dir,
extern int add_excludes_from_file_to_list(const char *fname, const char *base, int baselen,
struct exclude_list *el, int check_index);
extern void add_excludes_from_file(struct dir_struct *, const char *fname);
-extern void parse_exclude_pattern(const char **string, int *patternlen, int *flags, int *nowildcardlen);
+extern void parse_exclude_pattern(const char **string,
+ int *patternlen, int *flags,
+ int *nowildcardlen,
+ int *pattern_baselen);
extern void add_exclude(const char *string, const char *base,
int baselen, struct exclude_list *el, int srcpos);
extern void clear_exclude_list(struct exclude_list *el);
--
1.8.1.2.536.gf441e6d
^ permalink raw reply related
* [PATCH v3 09/13] exclude: filter out patterns not applicable to the current directory
From: Nguyễn Thái Ngọc Duy @ 2013-03-12 13:04 UTC (permalink / raw)
To: git; +Cc: Nguyễn Thái Ngọc Duy
In-Reply-To: <1363093500-16796-1-git-send-email-pclouds@gmail.com>
.gitignore files are spread over directories (*) so that when we check
for ignored files at foo, we are not bothered by foo/bar/.gitignore,
which contains ignore rules for foo/bar only.
This is not enough. foo/.gitignore can contain the pattern
"foo/bar/*.c". When we stay at foo, we know that the pattern cannot
match anything. Similarly, the pattern "/autom4te.cache" at root
directory cannot match anything in foo. This patch attempts to filter
out such patterns to drive down matching cost.
The algorithm implemented here is a naive one. Patterns can be either
active or passive:
- When we enter a new directory (e.g. from root to foo), currently
active patterns may no longer be applicable and can be turned to
passive.
- On the opposite, when we leave a directory (foo back to roo),
passive patterns may come alive again.
We could do smarter things. But this implementation cuts a big portion
of cost already (and solves the "root .gitignore is evil" problem).
There's probably no need to be smart.
(*) this design forces us to try to find .gitignore at every
directory. On webkit.git that equals to 6k open syscalls. It feels
like ".svn on every directory" again. I suggest we add
~/.gitignore.master, containing the list .gitignore files in
worktree. If this file exists, we don't poke at every directory for
.gitignore.
treat_leading_path: 0.000 0.000
read_directory: 3.455 2.879
+treat_one_path: 2.203 1.620
++is_excluded: 2.000 1.416
+++prep_exclude: 0.171 0.198
+++matching: 1.509 0.904
++dir_exist: 0.036 0.035
++index_name_exists: 0.292 0.289
lazy_init_name_hash: 0.257 0.257
+simplify_away: 0.084 0.085
+dir_add_name: 0.446 0.446
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
dir.c | 93 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++--
dir.h | 1 +
2 files changed, 92 insertions(+), 2 deletions(-)
diff --git a/dir.c b/dir.c
index 932fd2f..c57bf06 100644
--- a/dir.c
+++ b/dir.c
@@ -458,7 +458,7 @@ void add_exclude(const char *string, const char *base,
x->base = base;
x->baselen = baselen;
x->pattern_baselen = pattern_baselen;
- x->flags = flags;
+ x->flags = flags | EXC_FLAG_ACTIVE;
x->srcpos = srcpos;
ALLOC_GROW(el->excludes, el->nr + 1, el->alloc);
el->excludes[el->nr++] = x;
@@ -591,6 +591,87 @@ void add_excludes_from_file(struct dir_struct *dir, const char *fname)
die("cannot use %s as an exclude file", fname);
}
+static int pattern_match_base(struct dir_struct *dir,
+ const char *base, int baselen,
+ const struct exclude *exc)
+{
+ const char *pattern;
+
+ /*
+ * TODO: if a patterns come from a .gitignore, exc->base would
+ * be the same for all of them. We could compare once and
+ * reuse the result, instead of perform the comparison per
+ * pattern like this.
+ */
+ if (exc->baselen) {
+ if (baselen < exc->baselen + 1)
+ return 0;
+
+ if (base[exc->baselen] != '/' ||
+ memcmp(base, exc->base, exc->baselen))
+ return 0;
+
+ base += exc->baselen + 1;
+ baselen -= exc->baselen + 1;
+ }
+
+ if (baselen != exc->pattern_baselen)
+ return 0;
+
+ if (exc->pattern_baselen) {
+ pattern = exc->pattern;
+ if (*pattern == '/')
+ pattern++;
+ if (memcmp(base, pattern, exc->pattern_baselen))
+ return 0;
+ }
+
+ return 1;
+}
+
+/*
+ * If pushed is non-zero, we have entered a new directory. Some
+ * pathname patterns may no longer applicable. Go over all active
+ * patterns and disable them if so.
+ *
+ * If popped is non-zero, we have left a directory. Inactive patterns
+ * may be applicable again. Go over them and re-enable if so.
+ */
+static void scan_patterns(struct dir_struct *dir,
+ const char *base, int baselen,
+ int pushed, int popped)
+{
+ int i, j, k;
+
+ for (i = EXC_CMDL; i <= EXC_FILE; i++) {
+ struct exclude_list_group *group = &dir->exclude_list_group[i];
+ for (j = group->nr - 1; j >= 0; j--) {
+ struct exclude_list *list = &group->el[j];
+ for (k = 0; k < list->nr; k++) {
+ struct exclude *exc = list->excludes[k];
+
+ /*
+ * No base (i.e. EXC_FLAG_NODIR) or
+ * applicable to many bases ("**"
+ * patterns)
+ */
+ if (exc->pattern_baselen == -1)
+ continue;
+
+ if (exc->flags & EXC_FLAG_ACTIVE) {
+ if (pushed &&
+ !pattern_match_base(dir, base, baselen, exc))
+ exc->flags &= ~EXC_FLAG_ACTIVE;
+ } else {
+ if (popped &&
+ pattern_match_base(dir, base, baselen, exc))
+ exc->flags |= EXC_FLAG_ACTIVE;
+ }
+ }
+ }
+ }
+}
+
/*
* Loads the per-directory exclude list for the substring of base
* which has a char length of baselen.
@@ -600,7 +681,7 @@ static void prep_exclude(struct dir_struct *dir, const char *base, int baselen)
struct exclude_list_group *group;
struct exclude_list *el;
struct exclude_stack *stk = NULL;
- int current;
+ int current, popped = 0, pushed = 0;
if ((!dir->exclude_per_dir) ||
(baselen + strlen(dir->exclude_per_dir) >= PATH_MAX))
@@ -621,6 +702,7 @@ static void prep_exclude(struct dir_struct *dir, const char *base, int baselen)
clear_exclude_list(el);
free(stk);
group->nr--;
+ popped++;
}
/* Read from the parent directories and push them down. */
@@ -659,8 +741,12 @@ static void prep_exclude(struct dir_struct *dir, const char *base, int baselen)
el, 1);
dir->exclude_stack = stk;
current = stk->baselen;
+ pushed++;
}
dir->basebuf[baselen] = '\0';
+
+ if (pushed | popped)
+ scan_patterns(dir, base, baselen, pushed, popped);
}
int match_basename(const char *basename, int basenamelen,
@@ -755,6 +841,9 @@ static struct exclude *last_exclude_matching_from_list(const char *pathname,
const char *exclude = x->pattern;
int prefix = x->nowildcardlen;
+ if (!(x->flags & EXC_FLAG_ACTIVE))
+ continue;
+
if (x->flags & EXC_FLAG_MUSTBEDIR) {
if (*dtype == DT_UNKNOWN)
*dtype = get_dtype(NULL, pathname, pathlen);
diff --git a/dir.h b/dir.h
index cb50a85..247bfda 100644
--- a/dir.h
+++ b/dir.h
@@ -14,6 +14,7 @@ struct dir_entry {
#define EXC_FLAG_ENDSWITH 4
#define EXC_FLAG_MUSTBEDIR 8
#define EXC_FLAG_NEGATIVE 16
+#define EXC_FLAG_ACTIVE 32
/*
* Each excludes file will be parsed into a fresh exclude_list which
--
1.8.1.2.536.gf441e6d
^ permalink raw reply related
* [PATCH v3 10/13] read_directory: avoid invoking exclude machinery on tracked files
From: Nguyễn Thái Ngọc Duy @ 2013-03-12 13:04 UTC (permalink / raw)
To: git; +Cc: Nguyễn Thái Ngọc Duy, Junio C Hamano
In-Reply-To: <1363093500-16796-1-git-send-email-pclouds@gmail.com>
read_directory() (and its friendly wrapper fill_directory) collects
untracked/ignored files by traversing through the whole worktree,
feeding every entry to treat_one_path(), where each entry is checked
against .gitignore patterns.
One may see that tracked files can't be excluded and we do not need to
run them through exclude machinery. On repos where there are many
.gitignore patterns and/or a lot of tracked files, this unnecessary
processing can become expensive.
This patch avoids it mostly for normal cases. Directories are still
processed as before. DIR_SHOW_IGNORED and DIR_COLLECT_IGNORED are not
normally used unless some options are given (e.g. "checkout
--overwrite-ignore", "add -f"...)
treat_one_path's behavior changes when taking this shortcut. With
current code, when a non-directory path is not excluded,
treat_one_path calls treat_file, which returns the initial value of
exclude_file and causes treat_one_path to return path_handled. With
this patch, on the same conditions, treat_one_path returns
path_ignored.
read_directory_recursive() cares about this difference. Check out the
snippet:
while (...) {
switch (treat_path(...)) {
case path_ignored:
continue;
case path_handled:
break;
}
contents++;
if (check_only)
break;
dir_add_name(dir, path.buf, path.len);
}
If path_handled is returned, contents goes up. And if check_only is
true, the loop could be broken early. These will not happen when
treat_one_path (and its wrapper treat_path) returns
path_ignored. dir_add_name internally does a cache_name_exists() check
so it makes no difference.
To avoid this behavior change, treat_one_path is instructed to skip
the optimization when check_only or contents is used.
Finally some numbers (best of 20 runs) that shows why it's worth all
the hassle:
git status | webkit linux-2.6 libreoffice-core gentoo-x86
-------------+----------------------------------------------
before | 1.097s 0.208s 0.399s 0.539s
after | 0.736s 0.159s 0.248s 0.501s
nr. patterns | 89 376 19 0
nr. tracked | 182k 40k 63k 101k
treat_leading_path: 0.000 0.000
read_directory: 2.879 1.299
+treat_one_path: 1.620 0.599
++is_excluded: 1.416 0.103
+++prep_exclude: 0.198 0.040
+++matching: 0.904 0.036
++dir_exist: 0.035 0.036
++index_name_exists: 0.289 0.291
lazy_init_name_hash: 0.257 0.257
+simplify_away: 0.085 0.082
+dir_add_name: 0.446 0.000
Tracked-down-by: Karsten Blees <karsten.blees@gmail.com>
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
dir.c | 80 ++++++++++++++++++++++++++++++++++++++++++++-----------------------
1 file changed, 53 insertions(+), 27 deletions(-)
diff --git a/dir.c b/dir.c
index c57bf06..6809dd2 100644
--- a/dir.c
+++ b/dir.c
@@ -43,8 +43,11 @@ struct path_simplify {
const char *path;
};
-static int read_directory_recursive(struct dir_struct *dir, const char *path, int len,
- int check_only, const struct path_simplify *simplify);
+static void read_directory_recursive(struct dir_struct *dir,
+ const char *path, int len,
+ int check_only,
+ const struct path_simplify *simplify,
+ int *contents);
static int get_dtype(struct dirent *de, const char *path, int len);
static inline int memequal_icase(const char *a, const char *b, int n)
@@ -1184,7 +1187,7 @@ static enum directory_treatment treat_directory(struct dir_struct *dir,
const char *dirname, int len, int exclude,
const struct path_simplify *simplify)
{
- int ret;
+ int contents = 0, ret;
START_CLOCK();
/* The "len-1" is to strip the final '/' */
ret = directory_exists_in_index(dirname, len-1);
@@ -1219,19 +1222,19 @@ static enum directory_treatment treat_directory(struct dir_struct *dir,
* check if it contains only ignored files
*/
if ((dir->flags & DIR_SHOW_IGNORED) && !exclude) {
- int ignored;
dir->flags &= ~DIR_SHOW_IGNORED;
dir->flags |= DIR_HIDE_EMPTY_DIRECTORIES;
- ignored = read_directory_recursive(dir, dirname, len, 1, simplify);
+ read_directory_recursive(dir, dirname, len, 1, simplify, &contents);
dir->flags &= ~DIR_HIDE_EMPTY_DIRECTORIES;
dir->flags |= DIR_SHOW_IGNORED;
- return ignored ? ignore_directory : show_directory;
+ return contents ? ignore_directory : show_directory;
}
if (!(dir->flags & DIR_SHOW_IGNORED) &&
!(dir->flags & DIR_HIDE_EMPTY_DIRECTORIES))
return show_directory;
- if (!read_directory_recursive(dir, dirname, len, 1, simplify))
+ read_directory_recursive(dir, dirname, len, 1, simplify, &contents);
+ if (!contents)
return ignore_directory;
return show_directory;
}
@@ -1398,10 +1401,26 @@ enum path_treatment {
static enum path_treatment treat_one_path(struct dir_struct *dir,
struct strbuf *path,
const struct path_simplify *simplify,
- int dtype, struct dirent *de)
+ int dtype, struct dirent *de,
+ int exclude_shortcut_ok)
{
int exclude;
+ if (dtype == DT_UNKNOWN)
+ dtype = get_dtype(de, path->buf, path->len);
+
+ if (exclude_shortcut_ok &&
+ !(dir->flags & DIR_SHOW_IGNORED) &&
+ !(dir->flags & DIR_COLLECT_IGNORED) &&
+ dtype != DT_DIR) {
+ struct cache_entry *ce;
+ START_CLOCK();
+ ce = cache_name_exists(path->buf, path->len, ignore_case);
+ STOP_CLOCK(tv_index_name_exists);
+ if (ce)
+ return path_ignored;
+ }
+
START_CLOCK();
exclude = is_excluded(dir, path->buf, path->len, &dtype);
STOP_CLOCK(tv_is_excluded);
@@ -1417,9 +1436,6 @@ static enum path_treatment treat_one_path(struct dir_struct *dir,
if (exclude && !(dir->flags & DIR_SHOW_IGNORED))
return path_ignored;
- if (dtype == DT_UNKNOWN)
- dtype = get_dtype(de, path->buf, path->len);
-
switch (dtype) {
default:
return path_ignored;
@@ -1451,7 +1467,8 @@ static enum path_treatment treat_path(struct dir_struct *dir,
struct dirent *de,
struct strbuf *path,
int baselen,
- const struct path_simplify *simplify)
+ const struct path_simplify *simplify,
+ int exclude_shortcut_ok)
{
int dtype, ret;
@@ -1467,7 +1484,7 @@ static enum path_treatment treat_path(struct dir_struct *dir,
dtype = DTYPE(de);
START_CLOCK();
- ret = treat_one_path(dir, path, simplify, dtype, de);
+ ret = treat_one_path(dir, path, simplify, dtype, de, exclude_shortcut_ok);
STOP_CLOCK(tv_treat_one_path);
return ret;
}
@@ -1481,13 +1498,13 @@ static enum path_treatment treat_path(struct dir_struct *dir,
* Also, we ignore the name ".git" (even if it is not a directory).
* That likely will not change.
*/
-static int read_directory_recursive(struct dir_struct *dir,
- const char *base, int baselen,
- int check_only,
- const struct path_simplify *simplify)
+static void read_directory_recursive(struct dir_struct *dir,
+ const char *base, int baselen,
+ int check_only,
+ const struct path_simplify *simplify,
+ int *contents)
{
DIR *fdir;
- int contents = 0;
struct dirent *de;
struct strbuf path = STRBUF_INIT;
@@ -1499,18 +1516,29 @@ static int read_directory_recursive(struct dir_struct *dir,
dir->exclude_prepared = 0;
while ((de = readdir(fdir)) != NULL) {
- switch (treat_path(dir, de, &path, baselen, simplify)) {
+ switch (treat_path(dir, de, &path, baselen,
+ simplify,
+ !check_only && !contents)) {
case path_recurse:
- contents += read_directory_recursive(dir, path.buf,
- path.len, 0,
- simplify);
+ read_directory_recursive(dir, path.buf,
+ path.len, 0,
+ simplify,
+ contents);
continue;
case path_ignored:
continue;
case path_handled:
break;
}
- contents++;
+ /*
+ * Update the last argument to treat_path if anything
+ * else is done after this point. This is because if
+ * treat_path's exclude_shortcut_ok is true, it may
+ * incorrectly return path_ignored (and never reaches
+ * this part) instead of path_handled.
+ */
+ if (contents)
+ (*contents)++;
if (check_only)
break;
START_CLOCK();
@@ -1521,8 +1549,6 @@ static int read_directory_recursive(struct dir_struct *dir,
out:
dir->exclude_prepared = 0;
strbuf_release(&path);
-
- return contents;
}
static int cmp_name(const void *p1, const void *p2)
@@ -1593,7 +1619,7 @@ static int treat_leading_path(struct dir_struct *dir,
break;
dir->exclude_prepared = 0;
if (treat_one_path(dir, &sb, simplify,
- DT_DIR, NULL) == path_ignored)
+ DT_DIR, NULL, 0) == path_ignored)
break; /* do not recurse into it */
if (len <= baselen) {
rc = 1;
@@ -1621,7 +1647,7 @@ int read_directory(struct dir_struct *dir, const char *path, int len, const char
STOP_CLOCK(tv_lazy_init_name_hash);
#endif
START_CLOCK();
- read_directory_recursive(dir, path, len, 0, simplify);
+ read_directory_recursive(dir, path, len, 0, simplify, NULL);
STOP_CLOCK(tv_read_directory);
}
#ifdef MEASURE_EXCLUDE
--
1.8.1.2.536.gf441e6d
^ permalink raw reply related
* [PATCH v3 11/13] Preallocate hash tables when the number of inserts are known in advance
From: Nguyễn Thái Ngọc Duy @ 2013-03-12 13:04 UTC (permalink / raw)
To: git; +Cc: Nguyễn Thái Ngọc Duy
In-Reply-To: <1363093500-16796-1-git-send-email-pclouds@gmail.com>
This avoids unnecessary allocations and reinsertions. On webkit.git
(i.e. about 100k inserts to the name hash table), this reduces about
100ms out of 3s user time.
treat_leading_path: 0.000 0.000
read_directory: 1.299 1.305
+treat_one_path: 0.599 0.599
++is_excluded: 0.103 0.103
+++prep_exclude: 0.040 0.040
+++matching: 0.036 0.035
++dir_exist: 0.036 0.035
++index_name_exists: 0.291 0.292
lazy_init_name_hash: 0.257 0.155
+simplify_away: 0.082 0.083
+dir_add_name: 0.000 0.000
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
diffcore-rename.c | 1 +
hash.h | 7 +++++++
name-hash.c | 1 +
3 files changed, 9 insertions(+)
diff --git a/diffcore-rename.c b/diffcore-rename.c
index 512d0ac..8d3d9bb 100644
--- a/diffcore-rename.c
+++ b/diffcore-rename.c
@@ -389,6 +389,7 @@ static int find_exact_renames(struct diff_options *options)
struct hash_table file_table;
init_hash(&file_table);
+ preallocate_hash(&file_table, (rename_src_nr + rename_dst_nr) * 2);
for (i = 0; i < rename_src_nr; i++)
insert_file_table(&file_table, -1, i, rename_src[i].p->one);
diff --git a/hash.h b/hash.h
index b875ce6..244d1fe 100644
--- a/hash.h
+++ b/hash.h
@@ -40,4 +40,11 @@ static inline void init_hash(struct hash_table *table)
table->array = NULL;
}
+static inline void preallocate_hash(struct hash_table *table, unsigned int size)
+{
+ assert(table->size == 0 && table->nr == 0 && table->array == NULL);
+ table->size = size;
+ table->array = xcalloc(sizeof(struct hash_table_entry), size);
+}
+
#endif
diff --git a/name-hash.c b/name-hash.c
index 942c459..1287a19 100644
--- a/name-hash.c
+++ b/name-hash.c
@@ -92,6 +92,7 @@ static void lazy_init_name_hash(struct index_state *istate)
if (istate->name_hash_initialized)
return;
+ preallocate_hash(&istate->name_hash, istate->cache_nr * 2);
for (nr = 0; nr < istate->cache_nr; nr++)
hash_index_entry(istate, istate->cache[nr]);
istate->name_hash_initialized = 1;
--
1.8.1.2.536.gf441e6d
^ permalink raw reply related
* [PATCH v3 12/13] name-hash: allow to lookup a name with precalculated base hash
From: Nguyễn Thái Ngọc Duy @ 2013-03-12 13:04 UTC (permalink / raw)
To: git; +Cc: Nguyễn Thái Ngọc Duy
In-Reply-To: <1363093500-16796-1-git-send-email-pclouds@gmail.com>
index_name_exists_base() differs from index_name_exists() in how the
hash is calculated. It uses the base hash as the seed and skips the
baselen part.
The intention is to reduce hashing cost during directory traversal,
where the hash of the directory is calculated once, and used as base
hash for all entries inside.
This poses a constraint in hash function. The function has not changed
since 2008. With luck it'll never change again. If resuming hashing at
any characters are not feasible with a new (better) hash function,
maybe we could stop at directory boundary.
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
Makefile | 1 +
cache.h | 2 --
dir.c | 1 +
name-hash.c | 48 ++++++++++++++++++++++--------------------------
name-hash.h (new) | 45 +++++++++++++++++++++++++++++++++++++++++++++
read-cache.c | 1 +
unpack-trees.c | 1 +
7 files changed, 71 insertions(+), 28 deletions(-)
create mode 100644 name-hash.h
diff --git a/Makefile b/Makefile
index 26d3332..8d753af 100644
--- a/Makefile
+++ b/Makefile
@@ -690,6 +690,7 @@ LIB_H += mailmap.h
LIB_H += merge-blobs.h
LIB_H += merge-recursive.h
LIB_H += mergesort.h
+LIB_H += name-hash.h
LIB_H += notes-cache.h
LIB_H += notes-merge.h
LIB_H += notes.h
diff --git a/cache.h b/cache.h
index e493563..cf33c67 100644
--- a/cache.h
+++ b/cache.h
@@ -313,7 +313,6 @@ static inline void remove_name_hash(struct cache_entry *ce)
#define refresh_cache(flags) refresh_index(&the_index, (flags), NULL, NULL, NULL)
#define ce_match_stat(ce, st, options) ie_match_stat(&the_index, (ce), (st), (options))
#define ce_modified(ce, st, options) ie_modified(&the_index, (ce), (st), (options))
-#define cache_name_exists(name, namelen, igncase) index_name_exists(&the_index, (name), (namelen), (igncase))
#define cache_name_is_other(name, namelen) index_name_is_other(&the_index, (name), (namelen))
#define resolve_undo_clear() resolve_undo_clear_index(&the_index)
#define unmerge_cache_entry_at(at) unmerge_index_entry_at(&the_index, at)
@@ -442,7 +441,6 @@ extern int write_index(struct index_state *, int newfd);
extern int discard_index(struct index_state *);
extern int unmerged_index(const struct index_state *);
extern int verify_path(const char *path);
-extern struct cache_entry *index_name_exists(struct index_state *istate, const char *name, int namelen, int igncase);
extern int index_name_pos(const struct index_state *, const char *name, int namelen);
#define ADD_CACHE_OK_TO_ADD 1 /* Ok to add */
#define ADD_CACHE_OK_TO_REPLACE 2 /* Ok to replace file/directory */
diff --git a/dir.c b/dir.c
index 6809dd2..5fda5af 100644
--- a/dir.c
+++ b/dir.c
@@ -11,6 +11,7 @@
#include "dir.h"
#include "refs.h"
#include "wildmatch.h"
+#include "name-hash.h"
#ifdef MEASURE_EXCLUDE
static uint32_t tv_treat_leading_path;
diff --git a/name-hash.c b/name-hash.c
index 1287a19..572d232 100644
--- a/name-hash.c
+++ b/name-hash.c
@@ -7,30 +7,7 @@
*/
#define NO_THE_INDEX_COMPATIBILITY_MACROS
#include "cache.h"
-
-/*
- * This removes bit 5 if bit 6 is set.
- *
- * That will make US-ASCII characters hash to their upper-case
- * equivalent. We could easily do this one whole word at a time,
- * but that's for future worries.
- */
-static inline unsigned char icase_hash(unsigned char c)
-{
- return c & ~((c & 0x40) >> 1);
-}
-
-static unsigned int hash_name(const char *name, int namelen)
-{
- unsigned int hash = 0x123;
-
- while (namelen--) {
- unsigned char c = *name++;
- c = icase_hash(c);
- hash = hash*101 + c;
- }
- return hash;
-}
+#include "name-hash.h"
static void hash_index_entry_directories(struct index_state *istate, struct cache_entry *ce)
{
@@ -152,9 +129,11 @@ static int same_name(const struct cache_entry *ce, const char *name, int namelen
return slow_same_name(name, namelen, ce->name, namelen < len ? namelen : len);
}
-struct cache_entry *index_name_exists(struct index_state *istate, const char *name, int namelen, int icase)
+static inline struct cache_entry *find_entry_by_hash(struct index_state *istate,
+ const char *name, int namelen,
+ unsigned int hash,
+ int icase)
{
- unsigned int hash = hash_name(name, namelen);
struct cache_entry *ce;
lazy_init_name_hash(istate);
@@ -189,3 +168,20 @@ struct cache_entry *index_name_exists(struct index_state *istate, const char *na
}
return NULL;
}
+
+struct cache_entry *index_name_exists(struct index_state *istate,
+ const char *name, int namelen,
+ int icase)
+{
+ unsigned int hash = hash_name(name, namelen);
+ return find_entry_by_hash(istate, name, namelen, hash, icase);
+}
+
+struct cache_entry *index_name_exists_base(struct index_state *istate,
+ unsigned int basehash, int baselen,
+ const char *name, int namelen,
+ int icase)
+{
+ unsigned int hash = hash_name_from(basehash, name + baselen, namelen - baselen);
+ return find_entry_by_hash(istate, name, namelen, hash, icase);
+}
diff --git a/name-hash.h b/name-hash.h
new file mode 100644
index 0000000..997de30
--- /dev/null
+++ b/name-hash.h
@@ -0,0 +1,45 @@
+#ifndef NAME_HASH_H
+#define NAME_HASH_H
+
+extern struct cache_entry *index_name_exists(struct index_state *istate,
+ const char *name, int namelen,
+ int igncase);
+extern struct cache_entry *index_name_exists_base(struct index_state *istate,
+ unsigned int basehash, int baselen,
+ const char *name, int namelen,
+ int igncase);
+
+static inline struct cache_entry *cache_name_exists(const char *name, int namelen, int igncase)
+{
+ return index_name_exists(&the_index, name, namelen, igncase);
+}
+
+/*
+ * This removes bit 5 if bit 6 is set.
+ *
+ * That will make US-ASCII characters hash to their upper-case
+ * equivalent. We could easily do this one whole word at a time,
+ * but that's for future worries.
+ */
+static inline unsigned char icase_hash(unsigned char c)
+{
+ return c & ~((c & 0x40) >> 1);
+}
+
+static inline unsigned int hash_name_from(unsigned int hash,
+ const char *name, int namelen)
+{
+ while (namelen--) {
+ unsigned char c = *name++;
+ c = icase_hash(c);
+ hash = hash*101 + c;
+ }
+ return hash;
+}
+
+static inline unsigned int hash_name(const char *name, int namelen)
+{
+ return hash_name_from(0x123, name, namelen);
+}
+
+#endif
diff --git a/read-cache.c b/read-cache.c
index 827ae55..8bd3ec8 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -14,6 +14,7 @@
#include "resolve-undo.h"
#include "strbuf.h"
#include "varint.h"
+#include "name-hash.h"
static struct cache_entry *refresh_cache_entry(struct cache_entry *ce, int really);
diff --git a/unpack-trees.c b/unpack-trees.c
index 09e53df..60fa5d4 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -8,6 +8,7 @@
#include "progress.h"
#include "refs.h"
#include "attr.h"
+#include "name-hash.h"
/*
* Error messages expected by scripts out of plumbing commands such as
--
1.8.1.2.536.gf441e6d
^ permalink raw reply related
* [PATCH v3 13/13] read_directory: calculate name hashes incrementally
From: Nguyễn Thái Ngọc Duy @ 2013-03-12 13:05 UTC (permalink / raw)
To: git; +Cc: Nguyễn Thái Ngọc Duy
In-Reply-To: <1363093500-16796-1-git-send-email-pclouds@gmail.com>
Instead of index_name_exists() calculating a hash for full pathname
for every entry, we calculate partial hash per directory, use it as a
seed. The number of characters that icase_hash has to chew will
reduce.
treat_leading_path: 0.000 0.000
read_directory: 1.296 1.235
+treat_one_path: 0.599 0.531
++is_excluded: 0.102 0.102
+++prep_exclude: 0.040 0.040
+++matching: 0.035 0.035
++dir_exist: 0.035 0.035
++index_name_exists: 0.292 0.225
lazy_init_name_hash: 0.155 0.155
+simplify_away: 0.082 0.083
+dir_add_name: 0.000 0.000
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
dir.c | 44 +++++++++++++++++++++++++++++++++-----------
1 file changed, 33 insertions(+), 11 deletions(-)
diff --git a/dir.c b/dir.c
index 5fda5af..8638dcd 100644
--- a/dir.c
+++ b/dir.c
@@ -46,6 +46,7 @@ struct path_simplify {
static void read_directory_recursive(struct dir_struct *dir,
const char *path, int len,
+ unsigned int hash,
int check_only,
const struct path_simplify *simplify,
int *contents);
@@ -1044,12 +1045,17 @@ static struct dir_entry *dir_entry_new(const char *pathname, int len)
return ent;
}
-static struct dir_entry *dir_add_name(struct dir_struct *dir, const char *pathname, int len)
+static struct dir_entry *dir_add_name(struct dir_struct *dir,
+ const char *pathname, int len,
+ unsigned int hash, int baselen)
{
if (!(dir->flags & DIR_SHOW_IGNORED)) {
struct cache_entry *ce;
START_CLOCK();
- ce = cache_name_exists(pathname, len, ignore_case);
+ ce = index_name_exists_base(&the_index,
+ hash, baselen,
+ pathname, len,
+ ignore_case);
STOP_CLOCK(tv_index_name_exists);
if (ce)
return NULL;
@@ -1225,7 +1231,9 @@ static enum directory_treatment treat_directory(struct dir_struct *dir,
if ((dir->flags & DIR_SHOW_IGNORED) && !exclude) {
dir->flags &= ~DIR_SHOW_IGNORED;
dir->flags |= DIR_HIDE_EMPTY_DIRECTORIES;
- read_directory_recursive(dir, dirname, len, 1, simplify, &contents);
+ read_directory_recursive(dir, dirname, len,
+ hash_name(dirname, len),
+ 1, simplify, &contents);
dir->flags &= ~DIR_HIDE_EMPTY_DIRECTORIES;
dir->flags |= DIR_SHOW_IGNORED;
@@ -1234,7 +1242,9 @@ static enum directory_treatment treat_directory(struct dir_struct *dir,
if (!(dir->flags & DIR_SHOW_IGNORED) &&
!(dir->flags & DIR_HIDE_EMPTY_DIRECTORIES))
return show_directory;
- read_directory_recursive(dir, dirname, len, 1, simplify, &contents);
+ read_directory_recursive(dir, dirname, len,
+ hash_name(dirname, len),
+ 1, simplify, &contents);
if (!contents)
return ignore_directory;
return show_directory;
@@ -1401,6 +1411,8 @@ enum path_treatment {
static enum path_treatment treat_one_path(struct dir_struct *dir,
struct strbuf *path,
+ unsigned int hash,
+ int baselen,
const struct path_simplify *simplify,
int dtype, struct dirent *de,
int exclude_shortcut_ok)
@@ -1416,7 +1428,8 @@ static enum path_treatment treat_one_path(struct dir_struct *dir,
dtype != DT_DIR) {
struct cache_entry *ce;
START_CLOCK();
- ce = cache_name_exists(path->buf, path->len, ignore_case);
+ ce = index_name_exists_base(&the_index, hash, baselen,
+ path->buf, path->len, ignore_case);
STOP_CLOCK(tv_index_name_exists);
if (ce)
return path_ignored;
@@ -1467,6 +1480,7 @@ static enum path_treatment treat_one_path(struct dir_struct *dir,
static enum path_treatment treat_path(struct dir_struct *dir,
struct dirent *de,
struct strbuf *path,
+ unsigned int hash,
int baselen,
const struct path_simplify *simplify,
int exclude_shortcut_ok)
@@ -1485,7 +1499,8 @@ static enum path_treatment treat_path(struct dir_struct *dir,
dtype = DTYPE(de);
START_CLOCK();
- ret = treat_one_path(dir, path, simplify, dtype, de, exclude_shortcut_ok);
+ ret = treat_one_path(dir, path, hash, baselen,
+ simplify, dtype, de, exclude_shortcut_ok);
STOP_CLOCK(tv_treat_one_path);
return ret;
}
@@ -1501,6 +1516,7 @@ static enum path_treatment treat_path(struct dir_struct *dir,
*/
static void read_directory_recursive(struct dir_struct *dir,
const char *base, int baselen,
+ unsigned int hash,
int check_only,
const struct path_simplify *simplify,
int *contents)
@@ -1517,12 +1533,16 @@ static void read_directory_recursive(struct dir_struct *dir,
dir->exclude_prepared = 0;
while ((de = readdir(fdir)) != NULL) {
- switch (treat_path(dir, de, &path, baselen,
+ switch (treat_path(dir, de, &path, hash, baselen,
simplify,
!check_only && !contents)) {
case path_recurse:
read_directory_recursive(dir, path.buf,
- path.len, 0,
+ path.len,
+ hash_name_from(hash,
+ path.buf + baselen,
+ path.len - baselen),
+ 0,
simplify,
contents);
continue;
@@ -1543,7 +1563,7 @@ static void read_directory_recursive(struct dir_struct *dir,
if (check_only)
break;
START_CLOCK();
- dir_add_name(dir, path.buf, path.len);
+ dir_add_name(dir, path.buf, path.len, hash, baselen);
STOP_CLOCK(tv_dir_add_name);
}
closedir(fdir);
@@ -1619,7 +1639,7 @@ static int treat_leading_path(struct dir_struct *dir,
if (simplify_away(sb.buf, sb.len, simplify))
break;
dir->exclude_prepared = 0;
- if (treat_one_path(dir, &sb, simplify,
+ if (treat_one_path(dir, &sb, 0, 0, simplify,
DT_DIR, NULL, 0) == path_ignored)
break; /* do not recurse into it */
if (len <= baselen) {
@@ -1648,7 +1668,9 @@ int read_directory(struct dir_struct *dir, const char *path, int len, const char
STOP_CLOCK(tv_lazy_init_name_hash);
#endif
START_CLOCK();
- read_directory_recursive(dir, path, len, 0, simplify, NULL);
+ read_directory_recursive(dir, path, len,
+ hash_name(path, len),
+ 0, simplify, NULL);
STOP_CLOCK(tv_read_directory);
}
#ifdef MEASURE_EXCLUDE
--
1.8.1.2.536.gf441e6d
^ permalink raw reply related
* Updating not actual branch
From: Jan Pešta @ 2013-03-12 13:22 UTC (permalink / raw)
To: git
Hi All,
I have a question if there is a posibility tu update a branch which is not
actual working copy.
I have following situation:
A - B - C - I - J master
\ - D - E - F feature 1
\ G - H feature 2 (working copy)
I would like tu update whole tree with latest changes in master
A - B - C - I - J master
\ - D* - E* - F* feature 1
\ G* - H* feature 2
(working copy)
Is there some way how to do it without swithing to each branch and update
them manually?
Thanks,
Jan
^ permalink raw reply
* Re: [PATCH 1/2] require pathspec for "git add -u/-A"
From: Matthieu Moy @ 2013-03-12 13:58 UTC (permalink / raw)
To: Jeff King; +Cc: Junio C Hamano, git
In-Reply-To: <20130312112840.GA13186@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> PS I wonder how others are finding the warning? I'm finding it slightly
> annoying. Perhaps I just haven't retrained my fingers yet. But one
> problem with that retraining is that I type "git add -u" from the
> root _most_ of the time, and it just works. But occasionally, I get
> the "hey, do not do that" warning, because I'm in a subdir, even
> though I would be happy to have the full-tree behavior.
Same here. Not terribly disturbing, but I did get the warning
occasionally, even though I'm the author of the patch introducing
it ;-).
> I guess we already rejected the idea of being able to shut off the
> warning and just get the new behavior, in favor of having people
> specify it manually each time?
Somehow, but we may find a way to do so, as long as it temporary (i.e.
something that will have no effect after the transition period), and
that is is crystal clear that it's temporary.
All proposals up to now were rejected because of the risk of confusing
users (either shutting the warning blindly, or letting people think they
could keep the current behavior after Git 2.0).
--
Matthieu Moy
http://www-verimag.imag.fr/~moy/
^ permalink raw reply
* Re: Updating not actual branch
From: Konstantin Khomoutov @ 2013-03-12 15:19 UTC (permalink / raw)
To: Jan Pešta; +Cc: git
In-Reply-To: <006501ce1f24$94636a30$bd2a3e90$@certicon.cz>
On Tue, 12 Mar 2013 14:22:00 +0100
Jan Pešta <jan.pesta@certicon.cz> wrote:
> I have a question if there is a posibility tu update a branch which
> is not actual working copy.
>
> I have following situation:
>
> A - B - C - I - J master
> \ - D - E - F feature 1
> \ G - H feature 2 (working copy)
>
> I would like tu update whole tree with latest changes in master
>
> A - B - C - I - J master
> \ - D* - E* - F* feature 1
> \ G* - H*
> feature 2 (working copy)
>
>
> Is there some way how to do it without swithing to each branch and
> update them manually?
It's partially possible to do: you are able to forcibly fetch a remote
object into any local branch provided it's not checked out. Hence, in
your case you'll be able to update any branch excapt for "feature 2".
To do this, you could use the explicit refspec when fetching, like this:
$ git fetch origin +src1:dst1 '+blooper 1:feature 1'
(Consult the `git fetch` manual for more info on using refspecs.)
One possible downside is that I'm not sure this approach would play
nicely with remote branches, if you have any. I mean, direct fetching
into local branches will unlikely to update their matching "upstream"
remote branches.
So supposedly a better way to go would be to write a script which would
go like this:
1) Do a simple one-argument `git fetch <remotename>` to fetch from
the specified remote and update its remote branches.
2) Run `git for-each-ref`, and for each local branch found first check
whether it's currently checked out (that is, HEAD points at it)
and ignore the branch if it is; otherwise do `git update-ref`
updating the branch pointer from its upstream branch -- referring to
that using the <ref>@{upstream} syntax.
(Consult the gitrevisions manual for more info on this syntax.)
^ permalink raw reply
* Re: [PATCH 14/19] Document pull-all and push-all
From: Holger Hellmuth (IKS) @ 2013-03-12 15:12 UTC (permalink / raw)
To: Paul Campbell
Cc: git, David Michael Barr, Kindjal, bibendi, Herman van Rink,
mhoffman, Nate Jones
In-Reply-To: <CALeLG_kdXMb8wAyAL7T9jXk3sT85uJeiNh+v3jz9tKcf25VA9A@mail.gmail.com>
Am 09.03.2013 20:28, schrieb Paul Campbell:
> From 7dcd40ab8687a588b7b0c6ff914a7cfb601b6774 Mon Sep 17 00:00:00 2001
> From: Herman van Rink <rink@initfour.nl>
> Date: Tue, 27 Mar 2012 13:59:16 +0200
> Subject: [PATCH 14/19] Document pull-all and push-all
>
> ---
> contrib/subtree/git-subtree.txt | 8 +++++++-
> 1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/contrib/subtree/git-subtree.txt b/contrib/subtree/git-subtree.txt
> index e0957ee..c8fc103 100644
> --- a/contrib/subtree/git-subtree.txt
> +++ b/contrib/subtree/git-subtree.txt
> @@ -92,13 +92,19 @@ pull::
> Exactly like 'merge', but parallels 'git pull' in that
> it fetches the given commit from the specified remote
> repository.
> -
> +
> +pull-all::
> + Perform a pull operation on all in .gittrees registered subtrees.
> +
> push::
> Does a 'split' (see below) using the <prefix> supplied
> and then does a 'git push' to push the result to the
> repository and refspec. This can be used to push your
> subtree to different branches of the remote repository.
>
> +push-all::
> + Perform a pull operation on all in .gittrees registered subtrees.
-----
pull->push
> +
> split::
> Extract a new, synthetic project history from the
> history of the <prefix> subtree. The new history
>
^ permalink raw reply
* Re: [PATCH v4] submodule: add 'deinit' command
From: Phil Hord @ 2013-03-12 15:39 UTC (permalink / raw)
To: Jens Lehmann
Cc: Git Mailing List, Junio C Hamano, Heiko Voigt, Michael J Gruber,
Marc Branchaud, W. Trevor King
In-Reply-To: <5112C6F6.4030607@web.de>
On Wed, Feb 6, 2013 at 4:11 PM, Jens Lehmann <Jens.Lehmann@web.de> wrote:
> With "git submodule init" the user is able to tell git he cares about one
> or more submodules and wants to have it populated on the next call to "git
> submodule update". But currently there is no easy way he could tell git he
> does not care about a submodule anymore and wants to get rid of his local
> work tree (except he knows a lot about submodule internals and removes the
> "submodule.$name.url" setting from .git/config together with the work tree
> himself).
>
> Help those users by providing a 'deinit' command. This removes the whole
> submodule.<name> section from .git/config either for the given
> submodule(s) or for all those which have been initialized if '.' is
> given. Fail if the current work tree contains modifications unless
> forced. Complain when for a submodule given on the command line the url
> setting can't be found in .git/config, but nonetheless don't fail.
>
> Add tests and link the man pages of "git submodule deinit" and "git rm"
> to assist the user in deciding whether removing or unregistering the
> submodule is the right thing to do for him.
>
> Signed-off-by: Jens Lehmann <Jens.Lehmann@web.de>
Probably because I was new to this command, I was confused by this output.
$ git submodule deinit submodule
rm 'submodule'
Submodule 'submodule' (gerrit:foo/submodule) unregistered for path 'submodule'
$ git rm submodule
rm 'submodule'
This line is confusing to me in the deinit command:
rm 'submodule'
It doesn't mean what git usually means when it says this to me. See
how the 'git rm' command says the same thing but means something
different in the next command.
In the deinit case, git removes the workdir contents of 'submodule'
and it reports "rm 'submodule'". In the rm case, git removes the
submodule link from the tree and rmdirs the empty 'submodule'
directory.
I think this would be clearer if 'git deinit' said
rm 'submodule/*'
or maybe
Removed workdir for 'submodule'
Is it just me?
Phil
^ permalink raw reply
* Re: [PATCH v2 1/4] config: factor out config file stack management
From: Heiko Voigt @ 2013-03-12 15:44 UTC (permalink / raw)
To: Jeff King; +Cc: Junio C Hamano, git, Jens Lehmann, Ramsay Jones
In-Reply-To: <20130312105200.GC11340@sigill.intra.peff.net>
On Tue, Mar 12, 2013 at 06:52:00AM -0400, Jeff King wrote:
> On Sun, Mar 10, 2013 at 05:57:53PM +0100, Heiko Voigt wrote:
>
> > Because a config callback may start parsing a new file, the
> > global context regarding the current config file is stored
> > as a stack. Currently we only need to manage that stack from
> > git_config_from_file. Let's factor it out to allow new
> > sources of config data.
> >
> > [...]
> >
> > +static int do_config_from(struct config_file *top, config_fn_t fn, void *data)
> > +{
> > + int ret;
> > +
> > + /* push config-file parsing state stack */
> > + top->prev = cf;
> > + top->linenr = 1;
> > + top->eof = 0;
> > + strbuf_init(&top->value, 1024);
> > + strbuf_init(&top->var, 1024);
> > + cf = top;
> > +
> > + ret = git_parse_file(fn, data);
> > +
> > + /* pop config-file parsing state stack */
> > + strbuf_release(&top->value);
> > + strbuf_release(&top->var);
> > + cf = top->prev;
> > +
> > + return ret;
> > +}
>
> Can we throw in a comment at the top here with the expected usage? In
> particular, do_config_from is expecting the caller to have filled in
> certain fields (at this point, top->f and top->name), but there is
> nothing to make that clear.
Of course. Will do that in the next iteration. How about I squash this in:
diff --git a/config.c b/config.c
index b8c8640..b7632c9 100644
--- a/config.c
+++ b/config.c
@@ -948,6 +954,9 @@ int git_default_config(const char *var, const char *value, v
return 0;
}
+/* The fields data, name and the source specific callbacks of top need
+ * to be initialized before calling this function.
+ */
static int do_config_from_source(struct config_source *top, config_fn_t fn, voi
{
int ret;
I would add that to the third patch:
config: make parsing stack struct independent from actual data source
because that contains the final modification to config_file/config_source.
Cheers Heiko
^ permalink raw reply related
* Re: [PATCH v2 2/4] config: drop file pointer validity check in get_next_char()
From: Heiko Voigt @ 2013-03-12 16:00 UTC (permalink / raw)
To: Jeff King; +Cc: Junio C Hamano, git, Jens Lehmann, Ramsay Jones
In-Reply-To: <20130312110003.GD11340@sigill.intra.peff.net>
On Tue, Mar 12, 2013 at 07:00:03AM -0400, Jeff King wrote:
> On Sun, Mar 10, 2013 at 05:58:57PM +0100, Heiko Voigt wrote:
>
> > The only location where cf is set in this file is in do_config_from().
> > This function has only one callsite which is config_from_file(). In
> > config_from_file() its ensured that the f member is set to non-zero.
> > [...]
> > - if (cf && ((f = cf->f) != NULL)) {
> > + if (cf) {
>
> I still think we can drop this conditional entirely. The complete call
> graph looks like:
>
> git_config_from_file
> -> git_parse_file
> -> get_next_char
> -> get_value
> -> get_next_char
> -> parse_value
> -> get_next_char
> -> get_base_var
> -> get_next_char
> -> get_extended_base_var
> -> get_next_char
>
> That is, every path to get_next_char happens while we are in
> git_config_from_file, and that function guarantees that cf = &top, and
> that top.f != NULL. We do not have to even do any analysis of the
> conditions for each call, because we never change "cf" nor "top.f"
> except when we set them in git_config_from_file.
Ok if you say so I will do that :-). I was thinking about adding a patch
that would remove cf as a global variable and explicitely pass it down
to get_next_char. That makes it more obvious that it actually is != NULL.
Looking at your callgraph I do not think its that much work. What do you
think?
BTW, how did you generate this callgraph? Do you have a nice tool for that?
Cheers Heiko
^ permalink raw reply
* Re: [PATCH/RFC] Changing submodule foreach --recursive to be depth-first, --parent option to execute command in supermodule as well
From: Phil Hord @ 2013-03-12 16:01 UTC (permalink / raw)
To: Jens Lehmann
Cc: Junio C Hamano, Eric Cousineau, Heiko Voigt, git@vger.kernel.org
In-Reply-To: <513B7D08.20406@web.de>
On Sat, Mar 9, 2013 at 1:18 PM, Jens Lehmann <Jens.Lehmann@web.de> wrote:
> Am 05.03.2013 22:17, schrieb Phil Hord:
>> On Tue, Mar 5, 2013 at 3:51 PM, Jens Lehmann <Jens.Lehmann@web.de> wrote:
>>> Am 05.03.2013 19:34, schrieb Junio C Hamano:
>>>> Eric Cousineau <eacousineau@gmail.com> writes:
>>>>> ...
>>>> I am not entirely convinced we would want --include-super in the
>>>> first place, though. It does not belong to "submodule foreach";
>>>> it is doing something _outside_ the submoudules.
>>>
>>> I totally agree with that. First, adding --include-super does not
>>> belong into the --post-order patch at all, as that is a different
>>> topic (even though it belongs to the same use case Eric has). Also
>>> the reason why we are thinking about adding the --post-order option
>>> IMO cuts the other way for --include-super: It is so easy to do
>>> that yourself I'm not convinced we should add an extra option to
>>> foreach for that, especially as it has nothing to do with submodules.
>>> So I think we should just drop --include-super.
>>
>> I agree it should not be part of this commit, but I've often found
>> myself in need of an --include-super switch. To me,
>> git-submodule-foreach means "visit all my .git repos in this project
>> and execute $cmd". It's a pity that the super-project is considered a
>> second-class citizen in this regard.
>
> Hmm, for me the super-project is a very natural second-class citizen
> to "git *submodule* foreach". But also I understand that sometimes the
> user wants to apply a command to superproject and submodules alike (I
> just recently did exactly that with "git gc" on our build server).
>
>> I have to do this sometimes:
>>
>> ${cmd} && git submodule foreach --recursive '${cmd}'
>>
>> I often forget the first part in scripts, though, and I've seen others
>> do it too. I usually create a function for it in git-heavy scripts.
>>
>> In a shell, it usually goes like this:
>>
>> git submodule foreach --recursive '${cmd}'
>> <up><home><del>{30-ish}<end><backspace><enter>
>>
>> It'd be easier if I could just include a switch for this, and maybe
>> even create an alias for it. But maybe this is different command
>> altogether.
>
> Are you sure you wouldn't forget to provide such a switch too? ;-)
No. However, when I remember to add the switch, my shell history will
remember it for me. This does not happen naturally for me in the
"<up><home><del>{30-ish}..." workflow.
I also hope this switch grows up into a configuration option someday.
Or maybe a completely different command, like I said before; because I
actually think it could be dangerous as a configuration option since
it would have drastic consequences for users executing scripts or
commands in other users' environments.
> I'm still not convinced we should add a new switch, as it can easily
> be achieved by adding "${cmd} &&" to your scripts. And on the command
> line you could use an alias like this one to achieve that:
>
> [alias]
> recurse = !sh -c \"$@ && git submodule foreach --recursive $@\"
Yes, making the feature itself a 2nd-class citizen. :-)
But this alias also denies me the benefit of the --post-order option.
For 'git recurse git push', for example, I wouldn't want the
superproject push to occur first; I would want it to occur last after
the submodules have been successfully pushed.
I agree this should go in some other commit, but I do not think it is
so trivial it should never be considered as a feature for git. That's
all I'm trying to say.
Phil
^ permalink raw reply
* Re: Updating not actual branch
From: Junio C Hamano @ 2013-03-12 16:03 UTC (permalink / raw)
To: Jan Pešta; +Cc: git
In-Reply-To: <006501ce1f24$94636a30$bd2a3e90$@certicon.cz>
Jan Pešta <jan.pesta@certicon.cz> writes:
> I have following situation:
>
> A - B - C - I - J master
> \ - D - E - F feature 1
> \ G - H feature 2 (working copy)
>
> I would like tu update whole tree with latest changes in master
>
> A - B - C - I - J master
> \ - D* - E* - F* feature 1
> \ G* - H* feature 2
> (working copy)
>
>
> Is there some way how to do it without swithing to each branch and update
> them manually?
With these asterisks I would assume that you are rebasing feature #n
on top of updated master. As rebasing requires you to have a
working tree, so that you can resolve potential conflicts between
your work and work done on the updated upstream, you fundamentally
would need to check out the branch you work on.
In the case you depicted where feature-1 is a complete subset of
feature-2, you are rebasing both of them, and you do not end up in
a nasty conflict, you could start from this state:
A---B---C master
\
D---E---F feature-1
\
G---H feature-2
update the master from the upstream:
$ git checkout master ; git pull
A---B---C---I---J master
\
D---E---F feature-1
\
G---H feature-2
rebase feature-2 on top of the updated master:
$ git rebase master feature-2
G'--H' feature-2
/
D'--E'--F'
/
A---B---C---I---J master
\
D---E---F feature-1
\
G---H
and finally repoint feature-1 to its updated version:
$ git branch -f feature-1 F'
G'--H' feature-2
/
D'--E'--F' feature-1
/
A---B---C---I---J master
\
D---E---F
\
G---H
Depending on the interaction between commits C..J and C..F, your
rebasing of feature-2 may end up not losing any of D', E' or F'.
Imagine the case where J was committed on the upstream by applying
the same patch as the original E; E' will become redundant and the
result of your "git rebase master feature-2" may look like this
instead:
G'--H' feature-2
/
D'--F'
/
A---B---C---I---J master
\
D---E---F feature-1
\
G---H
Or J could remove something E depends on, in which case you may have
to add it back with a new commit X when you rebase feature-2, like
so:
G'--H' feature-2
/
D'--X---E'--F'
/
A---B---C---I---J master
\
D---E---F feature-1
\
G---H
Because you cannot mechanically decide where the tip of updated
feature-1 has to be, you would need to use your brain to decide
where to repoint the tip of feature-1 in the last step.
^ permalink raw reply
* Re: [PATCH v2 2/4] config: drop file pointer validity check in get_next_char()
From: Heiko Voigt @ 2013-03-12 16:16 UTC (permalink / raw)
To: Jeff King; +Cc: Junio C Hamano, git, Jens Lehmann, Ramsay Jones
In-Reply-To: <20130312160056.GB4472@sandbox-ub.fritz.box>
On Tue, Mar 12, 2013 at 05:00:56PM +0100, Heiko Voigt wrote:
> On Tue, Mar 12, 2013 at 07:00:03AM -0400, Jeff King wrote:
> > On Sun, Mar 10, 2013 at 05:58:57PM +0100, Heiko Voigt wrote:
> >
> > > The only location where cf is set in this file is in do_config_from().
> > > This function has only one callsite which is config_from_file(). In
> > > config_from_file() its ensured that the f member is set to non-zero.
> > > [...]
> > > - if (cf && ((f = cf->f) != NULL)) {
> > > + if (cf) {
> >
> > I still think we can drop this conditional entirely. The complete call
> > graph looks like:
> >
> > git_config_from_file
> > -> git_parse_file
> > -> get_next_char
> > -> get_value
> > -> get_next_char
> > -> parse_value
> > -> get_next_char
> > -> get_base_var
> > -> get_next_char
> > -> get_extended_base_var
> > -> get_next_char
> >
> > That is, every path to get_next_char happens while we are in
> > git_config_from_file, and that function guarantees that cf = &top, and
> > that top.f != NULL. We do not have to even do any analysis of the
> > conditions for each call, because we never change "cf" nor "top.f"
> > except when we set them in git_config_from_file.
>
> Ok if you say so I will do that :-). I was thinking about adding a patch
> that would remove cf as a global variable and explicitely pass it down
> to get_next_char. That makes it more obvious that it actually is != NULL.
> Looking at your callgraph I do not think its that much work. What do you
> think?
I just had a look and unfortunately there are other functions that use
this variable (namely handle_path_include) for which its not that easy
to pass this in. So I will just drop the check here.
Cheers Heiko
^ permalink raw reply
* Re: [PATCH v4] submodule: add 'deinit' command
From: Junio C Hamano @ 2013-03-12 16:22 UTC (permalink / raw)
To: Phil Hord
Cc: Jens Lehmann, Git Mailing List, Heiko Voigt, Michael J Gruber,
Marc Branchaud, W. Trevor King
In-Reply-To: <CABURp0pC2FELxM5aUwxuTqS1roZm+fwkCQA+BoXjrd0+yQMmbg@mail.gmail.com>
Phil Hord <phil.hord@gmail.com> writes:
> I think this would be clearer if 'git deinit' said
>
> rm 'submodule/*'
>
> or maybe
>
> Removed workdir for 'submodule'
>
> Is it just me?
The latter may probably be better.
After cloning the superproject, you show interest in individual
submodules by saying "git submodule init <that submodule>" and until
then your .git/config in the superproject does not indicate that you
are interested in that submodule, and you won't have a submodule
checkout.
"deinit" is a way to revert back to that original state; recording
that you are no longer interested in the submodule is the primary
effect, and removal of its checkout is a mere side effect of it.
^ permalink raw reply
* Re: [PATCH v2 3/4] config: make parsing stack struct independent from actual data source
From: Heiko Voigt @ 2013-03-12 16:27 UTC (permalink / raw)
To: Jeff King; +Cc: Junio C Hamano, git, Jens Lehmann, Ramsay Jones
In-Reply-To: <20130312110355.GE11340@sigill.intra.peff.net>
On Tue, Mar 12, 2013 at 07:03:55AM -0400, Jeff King wrote:
> On Sun, Mar 10, 2013 at 05:59:40PM +0100, Heiko Voigt wrote:
>
> > diff --git a/config.c b/config.c
> > index f55c43d..fe1c0e5 100644
> > --- a/config.c
> > +++ b/config.c
> > @@ -10,20 +10,42 @@
> > #include "strbuf.h"
> > #include "quote.h"
> >
> > -typedef struct config_file {
> > - struct config_file *prev;
> > - FILE *f;
> > +struct config_source {
> > + struct config_source *prev;
> > + void *data;
>
> Would a union be more appropriate here? We do not ever have to pass it
> directly as a parameter, since we pass the "struct config_source" to the
> method functions.
>
> It's still possible to screw up using a union, but it's slightly harder
> than screwing up using a void pointer. And I do not think we need the
> run-time flexibility offered by the void pointer in this case.
No we do not need the void pointer flexibility. But that means every new
source would need to add to this union. Junio complained about that fact
when I first added the extra members directly to the struct. A union
does not waste that much space and we get some seperation using the
union members. Since this struct is local only I think that should be
ok.
> > +static int config_file_fgetc(struct config_source *conf)
> > +{
> > + FILE *f = conf->data;
> > + return fgetc(f);
> > +}
>
> This could become just:
>
> return fgetc(conf->u.f);
>
> and so forth (might it make sense to give "f" a more descriptive name,
> as we are adding other sources?).
Will change that.
Cheers Heiko
^ permalink raw reply
* Re: [PATCH v2 4/4] teach config parsing to read from strbuf
From: Heiko Voigt @ 2013-03-12 16:42 UTC (permalink / raw)
To: Jeff King; +Cc: Junio C Hamano, git, Jens Lehmann, Ramsay Jones
In-Reply-To: <20130312111806.GF11340@sigill.intra.peff.net>
On Tue, Mar 12, 2013 at 07:18:06AM -0400, Jeff King wrote:
> On Sun, Mar 10, 2013 at 06:00:52PM +0100, Heiko Voigt wrote:
>
> > This can be used to read configuration values directly from gits
> > database.
> >
> > Signed-off-by: Heiko Voigt <hvoigt@hvoigt.net>
>
> This is lacking motivation. IIRC, the rest of the story is something
> like "...so we can read .gitmodules directly from the repo" or something
> like that?
Will add some more here.
> > +struct config_strbuf {
> > + struct strbuf *strbuf;
> > + int pos;
> > +};
> >
> > +static int config_strbuf_fgetc(struct config_source *conf)
> > +{
> > + struct config_strbuf *str = conf->data;
>
> Yuck. If you used a union in the previous patch, then this could just go
> inline into the "struct config_source".
>
> > +int git_config_from_strbuf(config_fn_t fn, const char *name, struct strbuf *strbuf, void *data)
>
> Should this be a "const struct strbuf *strbuf"? For that matter, is
> there any reason not to take a bare pointer/len combination? It seems
> likely that callers would get the data from read_sha1_file, which means
> they have to stuff it into a strbuf for no good reason.
pointer/len should be fine too. I just used strbuf since when you find
out later that you need to modify the string its easier to handle. A
config parser should not need to do that so I will change that.
> > diff --git a/test-config.c b/test-config.c
> > new file mode 100644
> > index 0000000..c650837
> > --- /dev/null
> > +++ b/test-config.c
> > @@ -0,0 +1,40 @@
>
> I'm slightly "meh" on this test-config program. Having to add a C test
> harness like this is a good indication that we are short-changing users
> of the shell API in favor of builtin C code.
I mainly did this because I needed some test for the config part while
developing the "fetch renamed submodules" series.
> Your series does not actually add any callers of the new function. The
> obvious "patch 5/4" would be to plumb it into "git config --blob", and
> then we can just directly test it there (there could be other callers
> besides reading from a blob, of course, but I think the point of the
> series is to head in that direction).
Since this is a split of the series mentioned above there are no real
callers yet. The main reason for the split was that I wanted to reduce
the review burden of one big series into multiple reviews of smaller
chunks. If you think it is useful to add the --blob option I can also
test from there. It could actually be useful to look at certain
.gitmodules options from the submodule script.
Cheers Heiko
^ permalink raw reply
* Re: linux-next: unneeded merge in the security tree
From: Linus Torvalds @ 2013-03-12 17:13 UTC (permalink / raw)
To: Theodore Ts'o, James Morris, Stephen Rothwell, Linus,
linux-next, Linux Kernel Mailing List, Git Mailing List,
Junio C Hamano
In-Reply-To: <20130312041641.GE18595@thunk.org>
[ Added Junio and git to the recipients, and leaving a lot of stuff
quoted due to that... ]
On Mon, Mar 11, 2013 at 9:16 PM, Theodore Ts'o <tytso@mit.edu> wrote:
> On Tue, Mar 12, 2013 at 03:10:53PM +1100, James Morris wrote:
>> On Tue, 12 Mar 2013, Stephen Rothwell wrote:
>> > The top commit in the security tree today is a merge of v3.9-rc2. This
>> > is a completely unnecessary merge as the tree before the merge was a
>> > subset of v3.9-rc1 and so if the merge had been done using anything but
>> > the tag, it would have just been a fast forward. I know that this is now
>> > deliberate behaviour on git's behalf, but isn't there some way we can
>> > make this easier on maintainers who are just really just trying to pick a
>> > new starting point for their trees after a release? (at least I assume
>> > that is what James was trying to do)
>>
>> Yes, and I was merging to a tag as required by Linus.
Now, quite frankly, I'd prefer people not merge -rc tags either, just
real releases. -rc tags are certainly *much* better than merging
random daily stuff, but the basic rule should be "don't back-merge AT
ALL" rather than "back-merge tags".
That said, you didn't really want a merge at all, you just wanted to
sync up and start development. Which is different (but should still
prefer real releases, and only use rc tags if it's fixing stuff that
happened in the merge window - which may be the case here).
> Why not just force the head of the security tree to be v3.9-rc2? Then
> you don't end up creating a completely unnecessary merge commit, and
> users who were at the previous head of the security tree will
> experience a fast forward when they pull your new head.
So I think that may *technically* be the right solution, but it's a
rather annoying UI issue, partly because you can't just do it in a
single operation (you can't do a pull of the tag to both fetch and
fast-forward it), but partly because "git reset --hard" is also an
operation that can lose history, so it's something that people should
be nervous about, and shouldn't use as some kind of standard "let's
just fast-forward to Linus' tree" thing.
At the same time, it's absolutely true that when *I* pull a signed tag
from a downstream developer, I don't want a fast-forward, because then
I'd lose the signature. So when a maintainer pulls a submaintainer
tree, you want the signature to come upstream, but when a
submaintainer wants to just sync up with upstream, you don't want to
generate the pointless signed merge commit, because the signature is
already upstream because it's a public tag. So gthe behavior of "git
pull" is fundamentally ambiguous.
But git doesn't know the difference between "official public upstream
tag" and "signed tag used to verify the pull request".
I'm adding the git list just to get this issue out there and see if
people have any ideas. I've got a couple of workarounds, but they
aren't wonderful..
One is simple:
git config alias.sync="pull --ff-only"
which works fine, but forces submaintainers to be careful when doing
things like this, and using a special command to do back-merges.
And maybe that's the right thing to do? Back-merges *are* special,
after all. But the above alias is particularly fragile, in that
there's both "pull" and "merge" that people want to use this for, and
it doesn't really handle both. And --ff-only will obviously fail if
you actually have some work in your tree, and want to do a real merge,
so then you have to do that differently. So I'm mentioning this as a
better model than "git reset", but not really a *solution*.
That said, the fact that "--ff-only" errors out if you have local
development may actually be a big bonus - because you really shouldn't
do merges at all if you have local development, but syncing up to my
tree if you don't have it (and are going to start it) may be something
reasonable.
Now, the other approach - and perhaps preferable, but requiring actual
changes to git itself - is to do the non-fast-forward merge *only* for
FETCH_HEAD, which already has magic semantics in other ways. So if
somebody does
git fetch linus
git merge v3.8
to sync with me, they would *not* get a merge commit with a signature,
just a fast-forward. But if you do
git pull linus v3.8
or a
git fetch linus v3.8
git merge FETCH_HEAD
it would look like a "maintainer merge" and stash the signature in the
merge commit rather than fast-forward. It would probably work in
practice.
The final approach might be to make it like the "merge summary" and
simply make it configurable _and_ have a command line flag for it,
defaulting to our current behavior or to the above suggested "default
on for FETCH_HEAD, off for anything else".
Hmm?
Linus
^ permalink raw reply
* Re: [PATCH v3 04/13] match_basename: use strncmp instead of strcmp
From: Antoine Pelisse @ 2013-03-12 17:40 UTC (permalink / raw)
To: Nguyễn Thái Ngọc Duy; +Cc: git
In-Reply-To: <1363093500-16796-5-git-send-email-pclouds@gmail.com>
> --- a/dir.c
> +++ b/dir.c
> @@ -636,12 +636,14 @@ int match_basename(const char *basename, int basenamelen,
> int flags)
> {
> if (prefix == patternlen) {
> - if (!strcmp_icase(pattern, basename))
> + if (patternlen == basenamelen &&
> + !strncmp_icase(pattern, basename, patternlen))
> return 1;
> } else if (flags & EXC_FLAG_ENDSWITH) {
> if (patternlen - 1 <= basenamelen &&
> - !strcmp_icase(pattern + 1,
> - basename + basenamelen - patternlen + 1))
> + !strncmp_icase(pattern + 1,
> + basename + basenamelen - patternlen + 1,
> + patternlen - 1))
> return 1;
I can see that you kept strncmp(), was it worse with memcmp() ?
^ permalink raw reply
* Re: linux-next: unneeded merge in the security tree
From: Geert Uytterhoeven @ 2013-03-12 17:51 UTC (permalink / raw)
To: Linus Torvalds
Cc: Theodore Ts'o, James Morris, Stephen Rothwell, linux-next,
Linux Kernel Mailing List, Git Mailing List, Junio C Hamano
In-Reply-To: <CA+55aFzFLDcN-1GKae6Xqrns59K1xOD_HPzuv2Lv1__fZpqFMw@mail.gmail.com>
On Tue, Mar 12, 2013 at 6:13 PM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>> Why not just force the head of the security tree to be v3.9-rc2? Then
>> you don't end up creating a completely unnecessary merge commit, and
>> users who were at the previous head of the security tree will
>> experience a fast forward when they pull your new head.
>
> So I think that may *technically* be the right solution, but it's a
> rather annoying UI issue, partly because you can't just do it in a
> single operation (you can't do a pull of the tag to both fetch and
> fast-forward it), but partly because "git reset --hard" is also an
> operation that can lose history, so it's something that people should
> be nervous about, and shouldn't use as some kind of standard "let's
> just fast-forward to Linus' tree" thing.
In many cases, "git rebase x" does the exact same thing as
"git reset --hard x", with an added safeguard: if you forgot to upstream
something, it'll boil up on top of "x".
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
^ permalink raw reply
* Re: difftool -d symlinks, under what conditions
From: Matt McClure @ 2013-03-12 18:12 UTC (permalink / raw)
To: David Aguilar; +Cc: git@vger.kernel.org, Tim Henigan
In-Reply-To: <CAJELnLEL8y0G3MBGkW+YDKtVxX4n4axJG7p0oPsXsV4_FRyGDg@mail.gmail.com>
On Tue, Nov 27, 2012 at 7:41 AM, Matt McClure <matthewlmcclure@gmail.com> wrote:
>
> On Tuesday, November 27, 2012, David Aguilar wrote:
>>
>> It seems that there is an edge case here that we are not
>> accounting for: unmodified worktree paths, when checked out
>> into the temporary directory, can be edited by the tool when
>> comparing against older commits. These edits will be lost.
>
>
> Yes. That is exactly my desired use case. I want to make edits while I'm reviewing the diff.
I took a crack at implementing the change to make difftool -d use
symlinks more aggressively. I've tested it lightly, and it works for
the limited cases I've tried. This is my first foray into the Git
source code, so it's entirely possible that there are unintended side
effects and regressions if other features depend on the same code path
and make different assumptions.
https://github.com/matthewlmcclure/git/compare/difftool-directory-symlink-work-tree
Your thoughts on the change?
--
Matt McClure
http://www.matthewlmcclure.com
http://www.mapmyfitness.com/profile/matthewlmcclure
^ permalink raw reply
* Re: [PATCH v2 1/4] config: factor out config file stack management
From: Jeff King @ 2013-03-12 19:04 UTC (permalink / raw)
To: Heiko Voigt; +Cc: Junio C Hamano, git, Jens Lehmann, Ramsay Jones
In-Reply-To: <20130312154434.GB3021@sandbox-ub.fritz.box>
On Tue, Mar 12, 2013 at 04:44:35PM +0100, Heiko Voigt wrote:
> > Can we throw in a comment at the top here with the expected usage? In
> > particular, do_config_from is expecting the caller to have filled in
> > certain fields (at this point, top->f and top->name), but there is
> > nothing to make that clear.
>
> Of course. Will do that in the next iteration. How about I squash this in:
> [...]
> +/* The fields data, name and the source specific callbacks of top need
> + * to be initialized before calling this function.
> + */
> static int do_config_from_source(struct config_source *top, config_fn_t fn, voi
I think that is OK, but it may be even better to list the fields by
name. Also, our multi-line comment style is:
/*
* Multi-line comment.
*/
> I would add that to the third patch:
>
> config: make parsing stack struct independent from actual data source
>
> because that contains the final modification to config_file/config_source.
It does not matter to the end result, but I find it helps with reviewing
when the comment is added along with the function, and then expanded as
the function is changed. It helps to understand the effects of later
patches if they need to tweak comments.
I do not care that much in this instance, since we have already
discussed it, and I know what is going on, though.
-Peff
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox