* Re: RFE: Discard hunks during `git add -p`
From: Konstantin Khomoutov @ 2016-11-02 14:07 UTC (permalink / raw)
To: Jan Engelhardt; +Cc: git
In-Reply-To: <alpine.LSU.2.20.1611021435280.21207@nerf40.vanv.qr>
On Wed, 2 Nov 2016 14:46:04 +0100 (CET)
Jan Engelhardt <jengelh@inai.de> wrote:
> Current version: 2.10.2
> Example workflow:
>
> * I would do a global substitution across a source tree, e.g. `perl
> -i -pe 's{OLD_FOO\(x\)}{NEW_BAR(x, 0)}' *.c`
> * Using `git add -p`, I would verify each of the substitutions that
> they make sense in their respective locations, and, based on that,
> answer "y" or "n" to the interactive prompting to stage good hunks.
> * When done with add-p, I would commit the so-staged hunks,
> and then use `git reset --hard` to discard all changes that were
> not acknowledged during add-p.
>
> Being able to discard hunks (reset working copy to index contents)
> during add-p would alleviate the (quite broad) hard reset.
Couldn't you just do
git checkout -- .
after staging your approved changes?
To selectively zap uncommitted changes from your working tree, you could
do
git checkout --patch -- .
I'm not sure overloading `git add` with a "reverse" action is a good
idea. I'm actually prefer pragmatism over conceptual purity but I'm
not sure the prospective gain here is clear.
^ permalink raw reply
* RFE: Discard hunks during `git add -p`
From: Jan Engelhardt @ 2016-11-02 13:46 UTC (permalink / raw)
To: git
Current version: 2.10.2
Example workflow:
* I would do a global substitution across a source tree, e.g. `perl -i
-pe 's{OLD_FOO\(x\)}{NEW_BAR(x, 0)}' *.c`
* Using `git add -p`, I would verify each of the substitutions that they
make sense in their respective locations, and, based on that,
answer "y" or "n" to the interactive prompting to stage good hunks.
* When done with add-p, I would commit the so-staged hunks,
and then use `git reset --hard` to discard all changes that were
not acknowledged during add-p.
Being able to discard hunks (reset working copy to index contents)
during add-p would alleviate the (quite broad) hard reset.
Similar approach:
* global substitution
* Using `git add -p`, some hunks may warrant some more editing, doable
with the "e" command. The index would be updated with the extra
change, but the working copy be left as-is.
* When rerunning `git add -p` in such a state, a difference is shown
again for such edited spots, which I would like to discard (bring
the working copy into sync with index).
^ permalink raw reply
* [PATCH 5/5] exclude: do not respect symlinks for in-tree .gitignore
From: Jeff King @ 2016-11-02 13:09 UTC (permalink / raw)
To: git
In-Reply-To: <20161102130432.d3zprdul4sqgcfwu@sigill.intra.peff.net>
Like .gitattributes, we would like to make sure that
.gitignore files are handled consistently whether read from
the index or from the filesystem. We can do so by using
O_NOFOLLOW when opening the files.
Signed-off-by: Jeff King <peff@peff.net>
---
dir.c | 9 +++++++--
t/t0008-ignores.sh | 29 +++++++++++++++++++++++++++++
2 files changed, 36 insertions(+), 2 deletions(-)
diff --git a/dir.c b/dir.c
index 4fa1ca109..cf3fde005 100644
--- a/dir.c
+++ b/dir.c
@@ -693,6 +693,7 @@ static void invalidate_directory(struct untracked_cache *uc,
/* Flags for add_excludes() */
#define EXCLUDE_CHECK_INDEX (1<<0)
+#define EXCLUDE_NOFOLLOW (1<<1)
/*
* Given a file with name "fname", read it (either from disk, or from
@@ -713,7 +714,11 @@ static int add_excludes(const char *fname, const char *base, int baselen,
size_t size = 0;
char *buf, *entry;
- fd = open(fname, O_RDONLY);
+ if (flags & EXCLUDE_NOFOLLOW)
+ fd = open_nofollow(fname, O_RDONLY);
+ else
+ fd = open(fname, O_RDONLY);
+
if (fd < 0 || fstat(fd, &st) < 0) {
if (errno != ENOENT)
warn_on_inaccessible(fname);
@@ -1130,7 +1135,7 @@ static void prep_exclude(struct dir_struct *dir, const char *base, int baselen)
strbuf_addstr(&sb, dir->exclude_per_dir);
el->src = strbuf_detach(&sb, NULL);
add_excludes(el->src, el->src, stk->baselen, el,
- EXCLUDE_CHECK_INDEX,
+ EXCLUDE_CHECK_INDEX | EXCLUDE_NOFOLLOW,
untracked ? &sha1_stat : NULL);
}
/*
diff --git a/t/t0008-ignores.sh b/t/t0008-ignores.sh
index d27f438bf..7348b8e6a 100755
--- a/t/t0008-ignores.sh
+++ b/t/t0008-ignores.sh
@@ -841,4 +841,33 @@ test_expect_success 'info/exclude trumps core.excludesfile' '
test_cmp expect actual
'
+test_expect_success SYMLINKS 'set up ignore file for symlink tests' '
+ echo "*" >ignore
+'
+
+test_expect_success SYMLINKS 'symlinks respected in core.excludesFile' '
+ test_when_finished "rm symlink" &&
+ ln -s ignore symlink &&
+ test_config core.excludesFile "$(pwd)/symlink" &&
+ echo file >expect &&
+ git check-ignore file >actual &&
+ test_cmp expect actual
+'
+
+test_expect_success SYMLINKS 'symlinks respected in info/exclude' '
+ test_when_finished "rm .git/info/exclude" &&
+ ln -sf ../../ignore .git/info/exclude &&
+ echo file >expect &&
+ git check-ignore file >actual &&
+ test_cmp expect actual
+'
+
+test_expect_success SYMLINKS 'symlinks not respected in-tree' '
+ test_when_finished "rm .gitignore" &&
+ ln -sf ignore .gitignore &&
+ >expect &&
+ test_must_fail git check-ignore file >actual &&
+ test_cmp expect actual
+'
+
test_done
--
2.11.0.rc0.258.gf434c15
^ permalink raw reply related
* [PATCH 4/5] attr: do not respect symlinks for in-tree .gitattributes
From: Jeff King @ 2016-11-02 13:08 UTC (permalink / raw)
To: git
In-Reply-To: <20161102130432.d3zprdul4sqgcfwu@sigill.intra.peff.net>
The attributes system may sometimes read in-tree files from
the filesystem, and sometimes from the index. In the latter
case, we do not resolve symbolic links (and are not likely
to ever start doing so). Let's open filesystem links with
O_NOFOLLOW so that the two cases behave consistently.
As a bonus, this means that git will not follow such
symlinks to read and parse out-of-tree paths. It's unlikely
that this could have any security implications (a malicious
repository can already feed arbitrary content to the
attribute parser, and any disclosure of the out-of-tree
contents happens only to stderr). But it's one less oddball
thing to worry about.
Note that O_NOFOLLOW only prevents following links for the
path itself, not intermediate directories in the path. At
first glance, it seems like
ln -s /some/path in-repo
might still look at "in-repo/.gitattributes", following the
symlink to "/some/path/.gitattributes". However, if
"in-repo" is a symbolic link, then we know that it has no
git paths below it, and will never look at its
.gitattributes file.
We will continue to support out-of-tree symbolic links
(e.g., in $GIT_DIR/info/attributes); this just affects
in-tree links. When a symbolic link is encountered, the
contents are ignored and a warning is printed. POSIX
specifies ELOOP in this case, so the user would generally
see something like:
warning: unable to access '.gitattributes': Too many levels of symbolic links
Signed-off-by: Jeff King <peff@peff.net>
---
attr.c | 17 +++++++++++++----
t/t0003-attributes.sh | 31 +++++++++++++++++++++++++++++++
2 files changed, 44 insertions(+), 4 deletions(-)
diff --git a/attr.c b/attr.c
index 79bd89226..abf375c8a 100644
--- a/attr.c
+++ b/attr.c
@@ -153,6 +153,7 @@ static const char blank[] = " \t\r\n";
/* Flags usable in read_attr() and parse_attr_line() family of functions. */
#define READ_ATTR_MACRO_OK (1<<0)
+#define READ_ATTR_NOFOLLOW (1<<1)
/*
* Parse a whitespace-delimited attribute state (i.e., "attr",
@@ -371,16 +372,24 @@ static struct index_state *use_index;
static struct attr_stack *read_attr_from_file(const char *path, unsigned flags)
{
- FILE *fp = fopen(path, "r");
+ int fd;
+ FILE *fp;
struct attr_stack *res;
char buf[2048];
int lineno = 0;
- if (!fp) {
+ if (flags & READ_ATTR_NOFOLLOW)
+ fd = open_nofollow(path, O_RDONLY);
+ else
+ fd = open(path, O_RDONLY);
+
+ if (fd < 0) {
if (errno != ENOENT && errno != ENOTDIR)
warn_on_inaccessible(path);
return NULL;
}
+ fp = xfdopen(fd, "r");
+
res = xcalloc(1, sizeof(*res));
while (fgets(buf, sizeof(buf), fp)) {
char *bufp = buf;
@@ -528,7 +537,7 @@ static void bootstrap_attr_stack(void)
}
if (!is_bare_repository() || direction == GIT_ATTR_INDEX) {
- elem = read_attr(GITATTRIBUTES_FILE, flags);
+ elem = read_attr(GITATTRIBUTES_FILE, flags | READ_ATTR_NOFOLLOW);
elem->origin = xstrdup("");
elem->originlen = 0;
elem->prev = attr_stack;
@@ -620,7 +629,7 @@ static void prepare_attr_stack(const char *path, int dirlen)
strbuf_add(&pathbuf, path, cp - path);
strbuf_addch(&pathbuf, '/');
strbuf_addstr(&pathbuf, GITATTRIBUTES_FILE);
- elem = read_attr(pathbuf.buf, 0);
+ elem = read_attr(pathbuf.buf, READ_ATTR_NOFOLLOW);
strbuf_setlen(&pathbuf, cp - path);
elem->origin = strbuf_detach(&pathbuf, &elem->originlen);
elem->prev = attr_stack;
diff --git a/t/t0003-attributes.sh b/t/t0003-attributes.sh
index f0fbb4255..bd01945e5 100755
--- a/t/t0003-attributes.sh
+++ b/t/t0003-attributes.sh
@@ -297,4 +297,35 @@ test_expect_success 'bare repository: test info/attributes' '
)
'
+check_symlink () {
+ echo "$1: symlink: $2" >expect &&
+ git check-attr symlink "$1" >actual &&
+ test_cmp expect actual
+}
+
+test_expect_success SYMLINKS 'set up attr file for symlink tests' '
+ echo "* symlink" >attr
+'
+
+test_expect_success SYMLINKS 'symlinks respected in core.attributesFile' '
+ test_when_finished "rm symlink" &&
+ ln -s attr symlink &&
+ test_config core.attributesFile "$(pwd)/symlink" &&
+ check_symlink file set
+'
+
+test_expect_success SYMLINKS 'symlinks respected in info/attributes' '
+ test_when_finished "rm .git/info/attributes" &&
+ ln -s ../../attr .git/info/attributes &&
+ check_symlink file set
+'
+
+test_expect_success SYMLINKS 'symlinks not respected in-tree' '
+ test_when_finished "rm .gitattributes" &&
+ ln -sf attr .gitattributes &&
+ mkdir subdir &&
+ ln -sf ../attr subdir/.gitattributes &&
+ check_symlink subdir/file unspecified
+'
+
test_done
--
2.11.0.rc0.258.gf434c15
^ permalink raw reply related
* [PATCH 3/5] exclude: convert "check_index" into a flags field
From: Jeff King @ 2016-11-02 13:07 UTC (permalink / raw)
To: git
In-Reply-To: <20161102130432.d3zprdul4sqgcfwu@sigill.intra.peff.net>
We pass the "check_index" flag through the variants of
add_excludes(). Let's turn this into a full flags bit-field,
so that we can add more flags to it without affecting the
function signature.
Note that only one caller actually needs to use the new flag
name, as the rest all were passing "0" already.
Signed-off-by: Jeff King <peff@peff.net>
---
dir.c | 13 +++++++++----
dir.h | 2 +-
2 files changed, 10 insertions(+), 5 deletions(-)
diff --git a/dir.c b/dir.c
index bfa8c8a9a..4fa1ca109 100644
--- a/dir.c
+++ b/dir.c
@@ -691,6 +691,9 @@ static void invalidate_directory(struct untracked_cache *uc,
dir->dirs[i]->recurse = 0;
}
+/* Flags for add_excludes() */
+#define EXCLUDE_CHECK_INDEX (1<<0)
+
/*
* Given a file with name "fname", read it (either from disk, or from
* the index if "check_index" is non-zero), parse it and store the
@@ -701,9 +704,10 @@ static void invalidate_directory(struct untracked_cache *uc,
* ss_valid is non-zero, "ss" must contain good value as input.
*/
static int add_excludes(const char *fname, const char *base, int baselen,
- struct exclude_list *el, int check_index,
+ struct exclude_list *el, unsigned flags,
struct sha1_stat *sha1_stat)
{
+ int check_index = !!(flags & EXCLUDE_CHECK_INDEX);
struct stat st;
int fd, i, lineno = 1;
size_t size = 0;
@@ -787,9 +791,9 @@ static int add_excludes(const char *fname, const char *base, int baselen,
int add_excludes_from_file_to_list(const char *fname, const char *base,
int baselen, struct exclude_list *el,
- int check_index)
+ unsigned flags)
{
- return add_excludes(fname, base, baselen, el, check_index, NULL);
+ return add_excludes(fname, base, baselen, el, flags, NULL);
}
struct exclude_list *add_exclude_list(struct dir_struct *dir,
@@ -1125,7 +1129,8 @@ static void prep_exclude(struct dir_struct *dir, const char *base, int baselen)
strbuf_addbuf(&sb, &dir->basebuf);
strbuf_addstr(&sb, dir->exclude_per_dir);
el->src = strbuf_detach(&sb, NULL);
- add_excludes(el->src, el->src, stk->baselen, el, 1,
+ add_excludes(el->src, el->src, stk->baselen, el,
+ EXCLUDE_CHECK_INDEX,
untracked ? &sha1_stat : NULL);
}
/*
diff --git a/dir.h b/dir.h
index 97c83bb38..ba7eb924c 100644
--- a/dir.h
+++ b/dir.h
@@ -239,7 +239,7 @@ extern int is_excluded(struct dir_struct *dir, const char *name, int *dtype);
extern struct exclude_list *add_exclude_list(struct dir_struct *dir,
int group_type, const char *src);
extern int add_excludes_from_file_to_list(const char *fname, const char *base, int baselen,
- struct exclude_list *el, int check_index);
+ struct exclude_list *el, unsigned flags);
extern void add_excludes_from_file(struct dir_struct *, const char *fname);
extern void parse_exclude_pattern(const char **string, int *patternlen, unsigned *flags, int *nowildcardlen);
extern void add_exclude(const char *string, const char *base,
--
2.11.0.rc0.258.gf434c15
^ permalink raw reply related
* [PATCH 2/5] attr: convert "macro_ok" into a flags field
From: Jeff King @ 2016-11-02 13:06 UTC (permalink / raw)
To: git
In-Reply-To: <20161102130432.d3zprdul4sqgcfwu@sigill.intra.peff.net>
The attribute code can have a rather deep callstack, through
which we have to pass the "macro_ok" flag. In anticipation
of adding other flags, let's convert this to a generic
bit-field.
Signed-off-by: Jeff King <peff@peff.net>
---
attr.c | 43 ++++++++++++++++++++++++-------------------
1 file changed, 24 insertions(+), 19 deletions(-)
diff --git a/attr.c b/attr.c
index 1fcf042b8..79bd89226 100644
--- a/attr.c
+++ b/attr.c
@@ -151,6 +151,9 @@ struct match_attr {
static const char blank[] = " \t\r\n";
+/* Flags usable in read_attr() and parse_attr_line() family of functions. */
+#define READ_ATTR_MACRO_OK (1<<0)
+
/*
* Parse a whitespace-delimited attribute state (i.e., "attr",
* "-attr", "!attr", or "attr=value") from the string starting at src.
@@ -200,7 +203,7 @@ static const char *parse_attr(const char *src, int lineno, const char *cp,
}
static struct match_attr *parse_attr_line(const char *line, const char *src,
- int lineno, int macro_ok)
+ int lineno, unsigned flags)
{
int namelen;
int num_attr, i;
@@ -215,7 +218,7 @@ static struct match_attr *parse_attr_line(const char *line, const char *src,
namelen = strcspn(name, blank);
if (strlen(ATTRIBUTE_MACRO_PREFIX) < namelen &&
starts_with(name, ATTRIBUTE_MACRO_PREFIX)) {
- if (!macro_ok) {
+ if (!(flags & READ_ATTR_MACRO_OK)) {
fprintf(stderr, "%s not allowed: %s:%d\n",
name, src, lineno);
return NULL;
@@ -339,11 +342,11 @@ static void handle_attr_line(struct attr_stack *res,
const char *line,
const char *src,
int lineno,
- int macro_ok)
+ unsigned flags)
{
struct match_attr *a;
- a = parse_attr_line(line, src, lineno, macro_ok);
+ a = parse_attr_line(line, src, lineno, flags);
if (!a)
return;
ALLOC_GROW(res->attrs, res->num_matches + 1, res->alloc);
@@ -358,14 +361,15 @@ static struct attr_stack *read_attr_from_array(const char **list)
res = xcalloc(1, sizeof(*res));
while ((line = *(list++)) != NULL)
- handle_attr_line(res, line, "[builtin]", ++lineno, 1);
+ handle_attr_line(res, line, "[builtin]", ++lineno,
+ READ_ATTR_MACRO_OK);
return res;
}
static enum git_attr_direction direction;
static struct index_state *use_index;
-static struct attr_stack *read_attr_from_file(const char *path, int macro_ok)
+static struct attr_stack *read_attr_from_file(const char *path, unsigned flags)
{
FILE *fp = fopen(path, "r");
struct attr_stack *res;
@@ -382,13 +386,13 @@ static struct attr_stack *read_attr_from_file(const char *path, int macro_ok)
char *bufp = buf;
if (!lineno)
skip_utf8_bom(&bufp, strlen(bufp));
- handle_attr_line(res, bufp, path, ++lineno, macro_ok);
+ handle_attr_line(res, bufp, path, ++lineno, flags);
}
fclose(fp);
return res;
}
-static struct attr_stack *read_attr_from_index(const char *path, int macro_ok)
+static struct attr_stack *read_attr_from_index(const char *path, unsigned flags)
{
struct attr_stack *res;
char *buf, *sp;
@@ -406,34 +410,34 @@ static struct attr_stack *read_attr_from_index(const char *path, int macro_ok)
;
more = (*ep == '\n');
*ep = '\0';
- handle_attr_line(res, sp, path, ++lineno, macro_ok);
+ handle_attr_line(res, sp, path, ++lineno, flags);
sp = ep + more;
}
free(buf);
return res;
}
-static struct attr_stack *read_attr(const char *path, int macro_ok)
+static struct attr_stack *read_attr(const char *path, unsigned flags)
{
struct attr_stack *res;
if (direction == GIT_ATTR_CHECKOUT) {
- res = read_attr_from_index(path, macro_ok);
+ res = read_attr_from_index(path, flags);
if (!res)
- res = read_attr_from_file(path, macro_ok);
+ res = read_attr_from_file(path, flags);
}
else if (direction == GIT_ATTR_CHECKIN) {
- res = read_attr_from_file(path, macro_ok);
+ res = read_attr_from_file(path, flags);
if (!res)
/*
* There is no checked out .gitattributes file there, but
* we might have it in the index. We allow operation in a
* sparsely checked out work tree, so read from it.
*/
- res = read_attr_from_index(path, macro_ok);
+ res = read_attr_from_index(path, flags);
}
else
- res = read_attr_from_index(path, macro_ok);
+ res = read_attr_from_index(path, flags);
if (!res)
res = xcalloc(1, sizeof(*res));
return res;
@@ -493,6 +497,7 @@ static GIT_PATH_FUNC(git_path_info_attributes, INFOATTRIBUTES_FILE)
static void bootstrap_attr_stack(void)
{
struct attr_stack *elem;
+ unsigned flags = READ_ATTR_MACRO_OK;
if (attr_stack)
return;
@@ -503,7 +508,7 @@ static void bootstrap_attr_stack(void)
attr_stack = elem;
if (git_attr_system()) {
- elem = read_attr_from_file(git_etc_gitattributes(), 1);
+ elem = read_attr_from_file(git_etc_gitattributes(), flags);
if (elem) {
elem->origin = NULL;
elem->prev = attr_stack;
@@ -514,7 +519,7 @@ static void bootstrap_attr_stack(void)
if (!git_attributes_file)
git_attributes_file = xdg_config_home("attributes");
if (git_attributes_file) {
- elem = read_attr_from_file(git_attributes_file, 1);
+ elem = read_attr_from_file(git_attributes_file, flags);
if (elem) {
elem->origin = NULL;
elem->prev = attr_stack;
@@ -523,7 +528,7 @@ static void bootstrap_attr_stack(void)
}
if (!is_bare_repository() || direction == GIT_ATTR_INDEX) {
- elem = read_attr(GITATTRIBUTES_FILE, 1);
+ elem = read_attr(GITATTRIBUTES_FILE, flags);
elem->origin = xstrdup("");
elem->originlen = 0;
elem->prev = attr_stack;
@@ -532,7 +537,7 @@ static void bootstrap_attr_stack(void)
}
if (startup_info->have_repository)
- elem = read_attr_from_file(git_path_info_attributes(), 1);
+ elem = read_attr_from_file(git_path_info_attributes(), flags);
else
elem = NULL;
--
2.11.0.rc0.258.gf434c15
^ permalink raw reply related
* [PATCH 1/5] add open_nofollow() helper
From: Jeff King @ 2016-11-02 13:06 UTC (permalink / raw)
To: git
In-Reply-To: <20161102130432.d3zprdul4sqgcfwu@sigill.intra.peff.net>
Some callers of open() would like to optionally use
O_NOFOLLOW, but it is not available on all platforms. We
could abstract this by publicly defining O_NOFOLLOW to 0 on
those platforms, but that leaves us no room for any
workarounds (e.g., by checking the file type via lstat()).
Instead, let's abstract it into its own function. We don't
implement any workarounds here, but it it would be easy to
add them later.
Signed-off-by: Jeff King <peff@peff.net>
---
I didn't add the workaround because I think the current callers are OK
with "best effort", and doing it non-racily is quite tricky (though we
might also be OK with a racy version; we are not trying to beat
/tmp races, but just making sure a checkout that we did is sane).
git-compat-util.h | 3 +++
wrapper.c | 8 ++++++++
2 files changed, 11 insertions(+)
diff --git a/git-compat-util.h b/git-compat-util.h
index 87237b092..a2cc33ebc 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -1080,6 +1080,9 @@ int access_or_die(const char *path, int mode, unsigned flag);
/* Warn on an inaccessible file that ought to be accessible */
void warn_on_inaccessible(const char *path);
+/* Open with O_NOFOLLOW, if available on this platform */
+int open_nofollow(const char *path, int flags);
+
#ifdef GMTIME_UNRELIABLE_ERRORS
struct tm *git_gmtime(const time_t *);
struct tm *git_gmtime_r(const time_t *, struct tm *);
diff --git a/wrapper.c b/wrapper.c
index e7f197996..6bc7f24f5 100644
--- a/wrapper.c
+++ b/wrapper.c
@@ -679,3 +679,11 @@ void sleep_millisec(int millisec)
{
poll(NULL, 0, millisec);
}
+
+#ifndef O_NOFOLLOW
+#define O_NOFOLLOW 0
+#endif
+int open_nofollow(const char *path, int flags)
+{
+ return open(path, flags | O_NOFOLLOW);
+}
--
2.11.0.rc0.258.gf434c15
^ permalink raw reply related
* [PATCH 0/5] disallow symlinks for .gitignore and .gitattributes
From: Jeff King @ 2016-11-02 13:04 UTC (permalink / raw)
To: git
I noticed in a nearby discussion that we will follow in-filesystem
symlinks for in-tree .gitignore and .gitattributes files, but not when
those files are read out of the index (e.g., when switching branches).
This series teaches git to open those files with O_NOFOLLOW (when it is
available) to get more consistent behavior. Note that this only applies
to the in-tree versions; you can still symlink $GIT_DIR/info/attributes,
etc.
I stopped short of warning about symlinked entries in git-fsck, but
perhaps we would want to do that as well (doing it completely is tricky
because of all of the case-folding issues around matching pathnames).
[1/5]: add open_nofollow() helper
[2/5]: attr: convert "macro_ok" into a flags field
[3/5]: exclude: convert "check_index" into a flags field
[4/5]: attr: do not respect symlinks for in-tree .gitattributes
[5/5]: exclude: do not respect symlinks for in-tree .gitignore
attr.c | 58 ++++++++++++++++++++++++++++++++-------------------
dir.c | 20 +++++++++++++-----
dir.h | 2 +-
git-compat-util.h | 3 +++
t/t0003-attributes.sh | 31 +++++++++++++++++++++++++++
t/t0008-ignores.sh | 29 ++++++++++++++++++++++++++
wrapper.c | 8 +++++++
7 files changed, 123 insertions(+), 28 deletions(-)
-Peff
^ permalink raw reply
* Re: What's cooking in git.git (Oct 2016, #09; Mon, 31)
From: Junio C Hamano @ 2016-11-02 12:57 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git
In-Reply-To: <alpine.DEB.2.20.1611021054030.3264@virtualbox>
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> On Mon, 31 Oct 2016, Junio C Hamano wrote:
>
>> * jc/git-open-cloexec (2016-10-31) 3 commits
>> - sha1_file: stop opening files with O_NOATIME
>> - git_open_cloexec(): use fcntl(2) w/ FD_CLOEXEC fallback
>> - git_open(): untangle possible NOATIME and CLOEXEC interactions
>>
>> The codeflow of setting NOATIME and CLOEXEC on file descriptors Git
>> opens has been simplified.
>
> This branch (in particular, the "use fcntl(2)" one) breaks the build on
> Windows. Until this breakage is resolved, I won't be able to see (nor
> report) any test breakages in `pu`.
Thanks for a heads-up. Anything in 'pu' won't be of importance
until 2.11 final, so please don't worry too much about it. Instead
please do make sure the tip of 'master' is OK.
Having said that, an incremental update or replacement to help
preparing it for post 2.11 final is appreciated ;-)
FYI, I may be offline for a few days.
^ permalink raw reply
* Re: What's cooking in git.git (Oct 2016, #09; Mon, 31)
From: Junio C Hamano @ 2016-11-02 12:55 UTC (permalink / raw)
To: Torsten Bögershausen; +Cc: git
In-Reply-To: <20161102071646.GA5094@tb-raspi>
Torsten Bögershausen <tboegi@web.de> writes:
>> * tb/convert-stream-check (2016-10-27) 2 commits
>> - convert.c: stream and fast search for binary
>> - read-cache: factor out get_sha1_from_index() helper
>>
>
> It looks to be a good "proof of concept".
>
> The current series is far away from being ready, so please kick it
> out of pu and feel free to discard.
Thanks for a heads-up. Anything in 'pu' won't be of importance
until 2.11 final, so please don't worry too much about it.
^ permalink raw reply
* Re: What's cooking in git.git (Oct 2016, #09; Mon, 31)
From: Johannes Schindelin @ 2016-11-02 9:57 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <xmqqwpgoqjct.fsf@gitster.mtv.corp.google.com>
Hi Junio,
On Mon, 31 Oct 2016, Junio C Hamano wrote:
> * jc/git-open-cloexec (2016-10-31) 3 commits
> - sha1_file: stop opening files with O_NOATIME
> - git_open_cloexec(): use fcntl(2) w/ FD_CLOEXEC fallback
> - git_open(): untangle possible NOATIME and CLOEXEC interactions
>
> The codeflow of setting NOATIME and CLOEXEC on file descriptors Git
> opens has been simplified.
This branch (in particular, the "use fcntl(2)" one) breaks the build on
Windows. Until this breakage is resolved, I won't be able to see (nor
report) any test breakages in `pu`.
Ciao,
Dscho
^ permalink raw reply
* Re: What's cooking in git.git (Oct 2016, #09; Mon, 31)
From: Torsten Bögershausen @ 2016-11-02 7:16 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <xmqqwpgoqjct.fsf@gitster.mtv.corp.google.com>
>
> * tb/convert-stream-check (2016-10-27) 2 commits
> - convert.c: stream and fast search for binary
> - read-cache: factor out get_sha1_from_index() helper
>
> End-of-line conversion sometimes needs to see if the current blob
> in the index has NULs and CRs to base its decision. We used to
> always get a full statistics over the blob, but in many cases we
> can return early when we have seen "enough" (e.g. if we see a
> single NUL, the blob will be handled as binary). The codepaths
> have been optimized by using streaming interface.
>
> The tip seems to do too much in a single commit and may be better split.
> cf. <20161012134724.28287-1-tboegi@web.de>
> cf. <xmqqd1il5w4e.fsf@gitster.mtv.corp.google.com>
Reviews have been done, thanks everybody.
It looks to be a good "proof of concept".
The current series is far away from being ready, so please kick it
out of pu and feel free to discard.
^ permalink raw reply
* Re: [PATCH 3/3] abbrev: auto size the default abbreviation
From: Jeff King @ 2016-11-02 2:12 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Linus Torvalds
In-Reply-To: <xmqqmvhimzrh.fsf@gitster.mtv.corp.google.com>
On Tue, Nov 01, 2016 at 06:33:38PM -0700, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
>
> > On Fri, Sep 30, 2016 at 05:19:37PM -0700, Junio C Hamano wrote:
> >
> >> Introduce a mechanism, where we estimate the number of objects in
> >> the repository upon the first request to abbreviate an object name
> >> with the default setting and come up with a sane default for the
> >> repository. Based on the expectation that we would see collision in
> >> a repository with 2^(2N) objects when using object names shortened
> >> to first N bits, use sufficient number of hexdigits to cover the
> >> number of objects in the repository. Each hexdigit (4-bits) we add
> >> to the shortened name allows us to have four times (2-bits) as many
> >> objects in the repository.
>
> I was idly browsing the draft release notes and then documentation
> and noticed that, even though the new default is to auto-scale,
> there is no mention of that in the documentation and there is no way
> to explicitly ask for auto-scaling.
>
> I wonder if we want to have something like this. I actually am
> inclined to drop the change to config.c and remove the new mention
> of "auto" in the documentation.
I doubt anybody cares that much either way, but in theory
core.abbrev=auto is a way to override core.abbrev=10 in /etc/gitconfig
or something. Though I'm having trouble envisioning a case where anybody
would set it in /etc/gitconfig, or why somebody would then want to
override that back to auto.
So I think it is fine either way (but I do agree that the core.abbrev
needs _some_ update to mention the auto-scaling behavior).
> diff --git a/config.c b/config.c
> index 83fdecb1bc..c363cca4a9 100644
> --- a/config.c
> +++ b/config.c
> @@ -834,10 +834,16 @@ static int git_default_core_config(const char *var, const char *value)
> }
>
> if (!strcmp(var, "core.abbrev")) {
> - int abbrev = git_config_int(var, value);
> - if (abbrev < minimum_abbrev || abbrev > 40)
> - return -1;
> - default_abbrev = abbrev;
> + if (!value)
> + return config_error_nonbool(var);
> + if (!strcasecmp(value, "auto"))
> + default_abbrev = -1;
> + else {
> + int abbrev = git_config_int(var, value);
> + if (abbrev < minimum_abbrev || abbrev > 40)
> + return -1;
> + default_abbrev = abbrev;
> + }
This isn't a new problem you added, but that "return -1" would probably
leave people confused:
$ git -c core.abbrev=2 log
fatal: unable to parse 'core.abbrev' from command-line config
Probably something like:
return error("abbrev length out of range: %d", abbrev);
would be more descriptive. I doubt it's a big deal in practice, though
(why would you set core.abbrev to something silly in the first place?).
-Peff
^ permalink raw reply
* Re: [PATCH v1 05/19] update-index: warn in case of split-index incoherency
From: Junio C Hamano @ 2016-11-02 1:37 UTC (permalink / raw)
To: Christian Couder
Cc: Duy Nguyen, Git Mailing List,
Ævar Arnfjörð Bjarmason, Christian Couder
In-Reply-To: <CAP8UFD18gcMJY7zjXw+ry6Kq2Foug9r0e=OVgZ_hcFkEVfnChA@mail.gmail.com>
Christian Couder <christian.couder@gmail.com> writes:
>>> Wrap this string and the one below with _() so they can be translated.
>>
>> True.
>>
>> I further wonder if a natural reaction from users after seeing this
>> message is "I do want to--what else would I use that option to run
>> you for? Just do as you are told, instead of telling me what to
>> do!". Is this warning really a good idea, or shouldn't these places
>> be setting the configuration?
>
> In 435ec090ec (config: add core.untrackedCache, 2016-01-27) we decided
> to just use warning() after discussing if we should instead set the
> configuration.
Yeah that seems to be the version that was committed. I then wonder
if the users would naturally have a similar raction to that warning
as well.
^ permalink raw reply
* Re: [PATCH 3/3] abbrev: auto size the default abbreviation
From: Junio C Hamano @ 2016-11-02 1:33 UTC (permalink / raw)
To: Jeff King; +Cc: git, Linus Torvalds
In-Reply-To: <20161003222701.za5njew33rqc5b6g@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> On Fri, Sep 30, 2016 at 05:19:37PM -0700, Junio C Hamano wrote:
>
>> Introduce a mechanism, where we estimate the number of objects in
>> the repository upon the first request to abbreviate an object name
>> with the default setting and come up with a sane default for the
>> repository. Based on the expectation that we would see collision in
>> a repository with 2^(2N) objects when using object names shortened
>> to first N bits, use sufficient number of hexdigits to cover the
>> number of objects in the repository. Each hexdigit (4-bits) we add
>> to the shortened name allows us to have four times (2-bits) as many
>> objects in the repository.
I was idly browsing the draft release notes and then documentation
and noticed that, even though the new default is to auto-scale,
there is no mention of that in the documentation and there is no way
to explicitly ask for auto-scaling.
I wonder if we want to have something like this. I actually am
inclined to drop the change to config.c and remove the new mention
of "auto" in the documentation.
Documentation/config.txt | 9 +++++----
config.c | 14 ++++++++++----
2 files changed, 15 insertions(+), 8 deletions(-)
diff --git a/Documentation/config.txt b/Documentation/config.txt
index a0ab66aae7..b02f8a4025 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -783,10 +783,11 @@ core.sparseCheckout::
linkgit:git-read-tree[1] for more information.
core.abbrev::
- Set the length object names are abbreviated to. If unspecified,
- many commands abbreviate to 7 hexdigits, which may not be enough
- for abbreviated object names to stay unique for sufficiently long
- time.
+ Set the length object names are abbreviated to. If
+ unspecified or set to "auto", an appropriate value is
+ computed based on the approximate number of packed objects
+ in your repository, which hopefully is enough for
+ abbreviated object names to stay unique for some time.
add.ignoreErrors::
add.ignore-errors (deprecated)::
diff --git a/config.c b/config.c
index 83fdecb1bc..c363cca4a9 100644
--- a/config.c
+++ b/config.c
@@ -834,10 +834,16 @@ static int git_default_core_config(const char *var, const char *value)
}
if (!strcmp(var, "core.abbrev")) {
- int abbrev = git_config_int(var, value);
- if (abbrev < minimum_abbrev || abbrev > 40)
- return -1;
- default_abbrev = abbrev;
+ if (!value)
+ return config_error_nonbool(var);
+ if (!strcasecmp(value, "auto"))
+ default_abbrev = -1;
+ else {
+ int abbrev = git_config_int(var, value);
+ if (abbrev < minimum_abbrev || abbrev > 40)
+ return -1;
+ default_abbrev = abbrev;
+ }
return 0;
}
^ permalink raw reply related
* Re: [ANNOUNCE] Git v2.11.0-rc0
From: Jacob Keller @ 2016-11-02 1:21 UTC (permalink / raw)
To: Stefan Beller
Cc: Jeff King, Junio C Hamano, Michael Haggerty, git@vger.kernel.org
In-Reply-To: <CAGZ79ka6un7nHaNk3F8yp3vFSnB-iGapqLcZ-ZC3EvcKE4DMNQ@mail.gmail.com>
On Tue, Nov 1, 2016 at 2:10 PM, Stefan Beller <sbeller@google.com> wrote:
> On Tue, Nov 1, 2016 at 1:59 PM, Jeff King <peff@peff.net> wrote:
>> On Tue, Nov 01, 2016 at 01:56:34PM -0700, Junio C Hamano wrote:
>>
>>> > As of -rc0, we have both --indent-heuristic and --compaction-heuristic
>>> > (along with matching config), and they are mutually exclusive.
>>> >
>>> > In [1], Stefan suggested just replacing the compaction heuristic
>>> > entirely with the new one (and you seemed to agree). If we're going to
>>> > do that, it makes sense to do so before the release, so that we don't
>>> > get stuck supporting --indent-heuristic forever.
>>>
>>> You meant "compaction" in the last part? I think it is probably a
>>> good idea.
>>
>> I thought the plan mentioned in the mail I linked was to keep the indent
>> heuristic, but simply _call_ it the compaction heuristic. IOW, to swap
>> out the implementation under the hood for something we know is better.
>
> AFAICT Michaels design is better in every aspect than what I did initially,
> so it supersedes the work I did there. I would support the swap in names.
>
Agreed, it's much better than the original idea, and results in better
diffs in every single case we could find.
>>
>> We've already released a version with --compaction-heuristic, so we are
>> stuck keeping it forever either way.
>
> IIRC the release notes specifically noted this flag to be experimental and
> may be removed in future versions.
I agree, I think that we specifically spelled out that this might go
away, and so I don't think we're stuck supporting it forever. We don't
even really need a deprecation time frame either.
>
> When not doing the swap of the implementation, but rather remove the
> experimental feature of compaction-heuristic and introducing a *new*
> experimental --indent-heuristic, this may drive the point across that
> these names are actually experimental.
I think we should swap names as "compaction heuristic" is more generic.
Thanks,
Jake
^ permalink raw reply
* should git-subtree ignore submodules?
From: Kirill Katsnelson @ 2016-11-01 19:41 UTC (permalink / raw)
To: git, apenwarr
"git-subtree add" fails to add the subtree into a repository with
submodules, reporting that the worktree is not clean, while `git status`
says that everything is clean (including submodules). I tracked the
problem down to the command "git index HEAD" returning all submodules as
having the M status:
$ git diff-index HEAD
:160000 160000 d3812c9318c4d0336897fd2d666be908fa1a7953
d3812c9318c4d0336897fd2d666be908fa1a7953 M ext/grpc
<snip more submodules>
$ git --version
git version 2.9.2.windows.1
I worked around the problem in my local copy of git-subtree shell script
by adding "--ignore-submodules=all" to the two invocations of `git
diff-index` in the ensure_clean() function (direct link
<https://github.com/git/git/blob/next/contrib/subtree/git-subtree.sh#L586>).
I am wondering, is this a defect in git-subtree? To my understanding,
the command should not care about submodules more than ensuring their
worktree is not in the way of new prefix, and that's a separate check.
So *even if* the submodule is modified, this should not be a show
stopper for "git-subtree add". Or am I missing some subtleties?
-kkm
^ permalink raw reply
* Re: [PATCH v1 05/19] update-index: warn in case of split-index incoherency
From: Christian Couder @ 2016-11-01 23:00 UTC (permalink / raw)
To: Junio C Hamano
Cc: Duy Nguyen, Git Mailing List,
Ævar Arnfjörð Bjarmason, Christian Couder
In-Reply-To: <xmqqlgx3owbg.fsf@gitster.mtv.corp.google.com>
On Tue, Nov 1, 2016 at 8:05 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Duy Nguyen <pclouds@gmail.com> writes:
>
>>> diff --git a/builtin/update-index.c b/builtin/update-index.c
>>> index b75ea03..a14dbf2 100644
>>> --- a/builtin/update-index.c
>>> +++ b/builtin/update-index.c
>>> @@ -1098,12 +1098,21 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)
>>> }
>>>
>>> if (split_index > 0) {
>>> + if (git_config_get_split_index() == 0)
>>> + warning("core.splitIndex is set to false; "
>>> + "remove or change it, if you really want to "
>>> + "enable split index");
>>
>> Wrap this string and the one below with _() so they can be translated.
>
> True.
>
> I further wonder if a natural reaction from users after seeing this
> message is "I do want to--what else would I use that option to run
> you for? Just do as you are told, instead of telling me what to
> do!". Is this warning really a good idea, or shouldn't these places
> be setting the configuration?
In 435ec090ec (config: add core.untrackedCache, 2016-01-27) we decided
to just use warning() after discussing if we should instead set the
configuration.
^ permalink raw reply
* Re: Submodules: two confusing situations
From: Stefan Beller @ 2016-11-01 22:56 UTC (permalink / raw)
To: David Turner; +Cc: git@vger.kernel.org
In-Reply-To: <6533af94ebd74952b75e51c5609d8c20@exmbdft7.ad.twosigma.com>
On Tue, Nov 1, 2016 at 3:13 PM, David Turner <David.Turner@twosigma.com> wrote:
> Consider the following two cases:
>
> We have commit X and commit Y. X is an ancestor of Y.
>
> We're at X and doing get checkout Y -- or we're at Y and we're doing
> git checkout X.
>
> Case 1: Between X and Y, we have deleted a submodule.
> In order to move from X to Y, git removes the submodule
> from the working tree.
>
> Case 2: Between X and Y, we have instead added a submodule. In order
> to move from Y to X (that is, the opposite direction), git *does not*
> remove the submodule from the tree; instead, it gives a warning and
> leaves the submodule behind.
>
> I don't understand why these two cases are not symmetric.
Because you are using git-submodule.sh that is only an approximation of
what is supposed to happen. ("git checkout [X | Y] && git submodule update"
I'd guess).
"git submodule update" only *updates* the submodules and doesn't *delete*
them at all. I think this originated from the historic behavior of
having the submodules
.git directory inside the submodule and not in the superprojects .git/module.
When having the .git dir inside the submodule, you cannot delete the submodule
as you'd potentially loose information (local commits) from the submodule.
I am currently working on implementing a flag --recurse-submodules for
git-checkout
that would be symmetrical from X -> Y and back, but that feature hasn't seen
the mailing list yet as I discovered yet another bug locally.
I think I found a reviewer though. ;)
The problem with doing a propoer checkout, i.e. update and deletion of
a submodule,
you need to be sure the submodule in its state can go away:
* no untracked files that are lost (except for gitignored files)
* clean index in the submodule and
* the submodule points at the recorded sha1 (i.e. there are no
commits that would be dangling)
>
> Perhaps relatedly, consider the attached shell-script, which I have not yet made into a full git test because I'm not sure I understand the issues fully.
>
> It creates three commits:
>
> Commit 1 adds a submodule
> Commit 2 removes that submodule and re-adds it into a subdirectory (sub1 to sub1/sub1).
> Commit 3 adds an unrelated file.
>
> Then it checks out commit 1 (first deinitializing the submodule to avoid case 2 above), and attempts to cherry-pick commit 3. This seems like it ought to work, based on my understanding of cherry-pick. But in fact it gives a conflict on the stuff from commit 2 (which is unrelated to commit 3).
That sounds like a bug indeed.
>
> This is confusing to me, and looks like a bug. What am I missing?
>
^ permalink raw reply
* Submodules: two confusing situations
From: David Turner @ 2016-11-01 22:13 UTC (permalink / raw)
To: git@vger.kernel.org
[-- Attachment #1: Type: text/plain, Size: 1341 bytes --]
Consider the following two cases:
We have commit X and commit Y. X is an ancestor of Y.
We're at X and doing get checkout Y -- or we're at Y and we're doing
git checkout X.
Case 1: Between X and Y, we have deleted a submodule.
In order to move from X to Y, git removes the submodule
from the working tree.
Case 2: Between X and Y, we have instead added a submodule. In order
to move from Y to X (that is, the opposite direction), git *does not*
remove the submodule from the tree; instead, it gives a warning and
leaves the submodule behind.
I don't understand why these two cases are not symmetric.
--
Perhaps relatedly, consider the attached shell-script, which I have not yet made into a full git test because I'm not sure I understand the issues fully.
It creates three commits:
Commit 1 adds a submodule
Commit 2 removes that submodule and re-adds it into a subdirectory (sub1 to sub1/sub1).
Commit 3 adds an unrelated file.
Then it checks out commit 1 (first deinitializing the submodule to avoid case 2 above), and attempts to cherry-pick commit 3. This seems like it ought to work, based on my understanding of cherry-pick. But in fact it gives a conflict on the stuff from commit 2 (which is unrelated to commit 3).
This is confusing to me, and looks like a bug. What am I missing?
[-- Attachment #2: submodule-merge.sh --]
[-- Type: application/octet-stream, Size: 1021 bytes --]
#!/bin/bash
set -euo pipefail
mkdir demo
cd demo
git init main
(
git init sub1 &&
cd sub1 &&
dd if=/dev/urandom of=f bs=40 count=1 &&
git add f &&
git commit -m f
)
(
cd main &&
git submodule add ../sub1 sub1 &&
> sub2 && git add sub2 &&
git commit -m 'add both submodules' &&
git tag start
)
#make a commit that replaces sub1 in a l->d transition
(
cd main &&
git rm sub1 &&
mkdir sub1 &&
git submodule add ../sub1 sub1/sub1 &&
git commit -m 'f to d' &&
#And another commit that just adds a random file.
>foo &&
git add foo &&
git commit -m 'foo' &&
git tag l-to-d
# Now we want to get back to the start state
git submodule deinit sub1/sub1 &&
git checkout -b sub2 start &&
# Finally, we want to cherry-pick an innocuous-looking commit from a branch
# where we have previously made the l->d change
# This should not fail, but does.
git cherry-pick l-to-d
)
^ permalink raw reply
* Re: [ANNOUNCE] Git v2.11.0-rc0
From: Junio C Hamano @ 2016-11-01 22:06 UTC (permalink / raw)
To: Stefan Beller
Cc: Jeff King, Michael Haggerty, Jacob Keller, git@vger.kernel.org
In-Reply-To: <CAGZ79kZHajTxFRbOftH==UAXhbH7RSA_jYWO-aQXhW2aSRdUFA@mail.gmail.com>
Stefan Beller <sbeller@google.com> writes:
>> -diff.indentHeuristic::
>> diff.compactionHeuristic::
>> Set one of these options to `true` to enable one of two
>> experimental heuristics that shift diff hunk boundaries to
>
> We would need to reword this as well, as there will be only one heuristic left?
>> + } else if (flags & XDF_COMPACTION_HEURISTIC) {
>> /*
>> * Indent heuristic: a group of pure add/delete lines
>
> This comment may need adjustment as well (though we could go without)
Thanks. I've queued this as "SQUASH???" on top.
diff --git a/Documentation/diff-config.txt b/Documentation/diff-config.txt
index 0c79e48d9d..39fff3aef9 100644
--- a/Documentation/diff-config.txt
+++ b/Documentation/diff-config.txt
@@ -172,9 +172,8 @@ diff.tool::
include::mergetools-diff.txt[]
diff.compactionHeuristic::
- Set one of these options to `true` to enable one of two
- experimental heuristics that shift diff hunk boundaries to
- make patches easier to read.
+ Set this option to `true` to enable experimental heuristics
+ that shift diff hunk boundaries to make patches easier to read.
diff.algorithm::
Choose a diff algorithm. The variants are as follows:
diff --git a/xdiff/xdiffi.c b/xdiff/xdiffi.c
index 6cb96219cb..31ff519232 100644
--- a/xdiff/xdiffi.c
+++ b/xdiff/xdiffi.c
@@ -908,7 +908,9 @@ int xdl_change_compact(xdfile_t *xdf, xdfile_t *xdfo, long flags) {
}
} else if (flags & XDF_COMPACTION_HEURISTIC) {
/*
- * Indent heuristic: a group of pure add/delete lines
+ * Heuristic based on the indentation level.
+ *
+ * A group of pure add/delete lines
* implies two splits, one between the end of the "before"
* context and the start of the group, and another between
* the end of the group and the beginning of the "after"
^ permalink raw reply related
* Re: [ANNOUNCE] Git v2.11.0-rc0
From: Jeff King @ 2016-11-01 21:50 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Michael Haggerty, Stefan Beller, Jacob Keller, git
In-Reply-To: <xmqqvaw6nac0.fsf@gitster.mtv.corp.google.com>
On Tue, Nov 01, 2016 at 02:45:19PM -0700, Junio C Hamano wrote:
> Junio C Hamano <gitster@pobox.com> writes:
>
> > And here is _with_ renaming. I think "compaction heuristics" is a
> > much better phrase to call "heuristics used during the diff hunk
> > compaction process" without specifying how that heuristics work
> > (like taking hints in the indentation levels). If we are to retire
> > one while keeping the other, compaction-heuristics should be the
> > name we give and keep for the surviving one.
> >
> > I have not much confidence in the conversion result, though.
>
> I'll queue it on 'pu' for now, before I go offline for a few days,
> with the following log message:
Thanks. I think this is good, with the exception of the "One of these"
bits that remains in diff-config.txt.
I think Michael is OK with this approach based on previous discussions,
but we should get his Ack, too.
-Peff
^ permalink raw reply
* Re: [ANNOUNCE] Git v2.11.0-rc0
From: Junio C Hamano @ 2016-11-01 21:45 UTC (permalink / raw)
To: Jeff King; +Cc: Michael Haggerty, Stefan Beller, Jacob Keller, git
In-Reply-To: <xmqqzilinanp.fsf@gitster.mtv.corp.google.com>
Junio C Hamano <gitster@pobox.com> writes:
> And here is _with_ renaming. I think "compaction heuristics" is a
> much better phrase to call "heuristics used during the diff hunk
> compaction process" without specifying how that heuristics work
> (like taking hints in the indentation levels). If we are to retire
> one while keeping the other, compaction-heuristics should be the
> name we give and keep for the surviving one.
>
> I have not much confidence in the conversion result, though.
I'll queue it on 'pu' for now, before I go offline for a few days,
with the following log message:
diff: retire the original experimental "compaction" heuristics
This retires the experimental "compaction" heuristics but with a
twist. It removes the mention of "indent" heuristics, which was a
competing experiment, from everywhere, guts the core logic of the
original "compaction" heuristics out and replaces it with the logic
used by the "indent" heuristics.
The externally visible effect of this change is that people who have
been experimenting by setting diff.compactionHeuristic configuration
or giving the command line option --compaction-heuristic will start
getting the behaviour based on the improved heuristics that used to
be called "indent" heuristics.
Suggested-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
^ permalink raw reply
* Re: [ANNOUNCE] Git v2.11.0-rc0
From: Stefan Beller @ 2016-11-01 21:41 UTC (permalink / raw)
To: Junio C Hamano
Cc: Jeff King, Michael Haggerty, Jacob Keller, git@vger.kernel.org
In-Reply-To: <xmqqzilinanp.fsf@gitster.mtv.corp.google.com>
On Tue, Nov 1, 2016 at 2:38 PM, Junio C Hamano <gitster@pobox.com> wrote:
>
> -diff.indentHeuristic::
> diff.compactionHeuristic::
> Set one of these options to `true` to enable one of two
> experimental heuristics that shift diff hunk boundaries to
We would need to reword this as well, as there will be only one heuristic left?
> --- a/xdiff/xdiffi.c
> +++ b/xdiff/xdiffi.c
> @@ -906,22 +906,7 @@ int xdl_change_compact(xdfile_t *xdf, xdfile_t *xdfo, long flags) {
> if (group_previous(xdfo, &go))
> xdl_bug("group sync broken sliding to match");
> }
> - } else if ((flags & XDF_COMPACTION_HEURISTIC) && blank_lines) {
> - /*
> - * Compaction heuristic: if it is possible to shift the
> - * group to make its bottom line a blank line, do so.
> - *
> - * As we already shifted the group forward as far as
> - * possible in the earlier loop, we only need to handle
> - * backward shifts, not forward ones.
> - */
> - while (!is_blank_line(xdf->recs[g.end - 1], flags)) {
> - if (group_slide_up(xdf, &g, flags))
> - xdl_bug("blank line disappeared");
> - if (group_previous(xdfo, &go))
> - xdl_bug("group sync broken sliding to blank line");
> - }
> - } else if (flags & XDF_INDENT_HEURISTIC) {
> + } else if (flags & XDF_COMPACTION_HEURISTIC) {
> /*
> * Indent heuristic: a group of pure add/delete lines
This comment may need adjustment as well (though we could go without)
> * implies two splits, one between the end of the "before"
^ permalink raw reply
* Re: [ANNOUNCE] Git v2.11.0-rc0
From: Junio C Hamano @ 2016-11-01 21:38 UTC (permalink / raw)
To: Jeff King; +Cc: Michael Haggerty, Stefan Beller, Jacob Keller, git
In-Reply-To: <xmqqh97qoqq2.fsf@gitster.mtv.corp.google.com>
Junio C Hamano <gitster@pobox.com> writes:
> If it involves renaming and swapping, however, we may be talking
> about a change that we cannot deliver with confidence in less than a
> week before -rc1, which sounds, eh, suboptimal.
>
> FWIW, here is how the removal of compaction without renaming looks
> like.
And here is _with_ renaming. I think "compaction heuristics" is a
much better phrase to call "heuristics used during the diff hunk
compaction process" without specifying how that heuristics work
(like taking hints in the indentation levels). If we are to retire
one while keeping the other, compaction-heuristics should be the
name we give and keep for the surviving one.
I have not much confidence in the conversion result, though.
Documentation/diff-config.txt | 1 -
Documentation/diff-heuristic-options.txt | 2 --
builtin/blame.c | 3 +--
diff.c | 25 ++++-----------------
git-add--interactive.perl | 5 +----
t/t4061-diff-indent.sh | 38 ++++++++++++++++----------------
xdiff/xdiff.h | 1 -
xdiff/xdiffi.c | 17 +-------------
8 files changed, 26 insertions(+), 66 deletions(-)
diff --git a/Documentation/diff-config.txt b/Documentation/diff-config.txt
index 58f4bd6afa..0c79e48d9d 100644
--- a/Documentation/diff-config.txt
+++ b/Documentation/diff-config.txt
@@ -171,7 +171,6 @@ diff.tool::
include::mergetools-diff.txt[]
-diff.indentHeuristic::
diff.compactionHeuristic::
Set one of these options to `true` to enable one of two
experimental heuristics that shift diff hunk boundaries to
diff --git a/Documentation/diff-heuristic-options.txt b/Documentation/diff-heuristic-options.txt
index 36cb549df9..3cb024aa22 100644
--- a/Documentation/diff-heuristic-options.txt
+++ b/Documentation/diff-heuristic-options.txt
@@ -1,5 +1,3 @@
---indent-heuristic::
---no-indent-heuristic::
--compaction-heuristic::
--no-compaction-heuristic::
These are to help debugging and tuning experimental heuristics
diff --git a/builtin/blame.c b/builtin/blame.c
index 4ddfadb71f..395d4011fb 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -2596,7 +2596,6 @@ int cmd_blame(int argc, const char **argv, const char *prefix)
* and are only included here to get included in the "-h"
* output:
*/
- { OPTION_LOWLEVEL_CALLBACK, 0, "indent-heuristic", NULL, NULL, N_("Use an experimental indent-based heuristic to improve diffs"), PARSE_OPT_NOARG, parse_opt_unknown_cb },
{ OPTION_LOWLEVEL_CALLBACK, 0, "compaction-heuristic", NULL, NULL, N_("Use an experimental blank-line-based heuristic to improve diffs"), PARSE_OPT_NOARG, parse_opt_unknown_cb },
OPT_BIT(0, "minimal", &xdl_opts, N_("Spend extra cycles to find better match"), XDF_NEED_MINIMAL),
@@ -2645,7 +2644,7 @@ int cmd_blame(int argc, const char **argv, const char *prefix)
}
parse_done:
no_whole_file_rename = !DIFF_OPT_TST(&revs.diffopt, FOLLOW_RENAMES);
- xdl_opts |= revs.diffopt.xdl_opts & (XDF_COMPACTION_HEURISTIC | XDF_INDENT_HEURISTIC);
+ xdl_opts |= revs.diffopt.xdl_opts & XDF_COMPACTION_HEURISTIC;
DIFF_OPT_CLR(&revs.diffopt, FOLLOW_RENAMES);
argc = parse_options_end(&ctx);
diff --git a/diff.c b/diff.c
index 8981477c43..f1b01f5b1e 100644
--- a/diff.c
+++ b/diff.c
@@ -27,7 +27,6 @@
#endif
static int diff_detect_rename_default;
-static int diff_indent_heuristic; /* experimental */
static int diff_compaction_heuristic; /* experimental */
static int diff_rename_limit_default = 400;
static int diff_suppress_blank_empty;
@@ -223,16 +222,8 @@ void init_diff_ui_defaults(void)
int git_diff_heuristic_config(const char *var, const char *value, void *cb)
{
- if (!strcmp(var, "diff.indentheuristic")) {
- diff_indent_heuristic = git_config_bool(var, value);
- if (diff_indent_heuristic)
- diff_compaction_heuristic = 0;
- }
- if (!strcmp(var, "diff.compactionheuristic")) {
+ if (!strcmp(var, "diff.compactionheuristic"))
diff_compaction_heuristic = git_config_bool(var, value);
- if (diff_compaction_heuristic)
- diff_indent_heuristic = 0;
- }
return 0;
}
@@ -3378,9 +3369,7 @@ void diff_setup(struct diff_options *options)
options->use_color = diff_use_color_default;
options->detect_rename = diff_detect_rename_default;
options->xdl_opts |= diff_algorithm;
- if (diff_indent_heuristic)
- DIFF_XDL_SET(options, INDENT_HEURISTIC);
- else if (diff_compaction_heuristic)
+ if (diff_compaction_heuristic)
DIFF_XDL_SET(options, COMPACTION_HEURISTIC);
options->orderfile = diff_order_file_cfg;
@@ -3876,15 +3865,9 @@ int diff_opt_parse(struct diff_options *options,
DIFF_XDL_SET(options, IGNORE_WHITESPACE_AT_EOL);
else if (!strcmp(arg, "--ignore-blank-lines"))
DIFF_XDL_SET(options, IGNORE_BLANK_LINES);
- else if (!strcmp(arg, "--indent-heuristic")) {
- DIFF_XDL_SET(options, INDENT_HEURISTIC);
- DIFF_XDL_CLR(options, COMPACTION_HEURISTIC);
- } else if (!strcmp(arg, "--no-indent-heuristic"))
- DIFF_XDL_CLR(options, INDENT_HEURISTIC);
- else if (!strcmp(arg, "--compaction-heuristic")) {
+ else if (!strcmp(arg, "--compaction-heuristic"))
DIFF_XDL_SET(options, COMPACTION_HEURISTIC);
- DIFF_XDL_CLR(options, INDENT_HEURISTIC);
- } else if (!strcmp(arg, "--no-compaction-heuristic"))
+ else if (!strcmp(arg, "--no-compaction-heuristic"))
DIFF_XDL_CLR(options, COMPACTION_HEURISTIC);
else if (!strcmp(arg, "--patience"))
options->xdl_opts = DIFF_WITH_ALG(options, PATIENCE_DIFF);
diff --git a/git-add--interactive.perl b/git-add--interactive.perl
index ee3d812695..642cce1ac6 100755
--- a/git-add--interactive.perl
+++ b/git-add--interactive.perl
@@ -45,7 +45,6 @@
my $normal_color = $repo->get_color("", "reset");
my $diff_algorithm = $repo->config('diff.algorithm');
-my $diff_indent_heuristic = $repo->config_bool('diff.indentheuristic');
my $diff_compaction_heuristic = $repo->config_bool('diff.compactionheuristic');
my $diff_filter = $repo->config('interactive.difffilter');
@@ -751,9 +750,7 @@ sub parse_diff {
if (defined $diff_algorithm) {
splice @diff_cmd, 1, 0, "--diff-algorithm=${diff_algorithm}";
}
- if ($diff_indent_heuristic) {
- splice @diff_cmd, 1, 0, "--indent-heuristic";
- } elsif ($diff_compaction_heuristic) {
+ if ($diff_compaction_heuristic) {
splice @diff_cmd, 1, 0, "--compaction-heuristic";
}
if (defined $patch_mode_revision) {
diff --git a/t/t4061-diff-indent.sh b/t/t4061-diff-indent.sh
index 556450609b..30f809d0d3 100755
--- a/t/t4061-diff-indent.sh
+++ b/t/t4061-diff-indent.sh
@@ -1,6 +1,6 @@
#!/bin/sh
-test_description='Test diff indent heuristic.
+test_description='Test diff compaction heuristic.
'
. ./test-lib.sh
@@ -157,28 +157,28 @@ test_expect_success 'diff: ugly spaces' '
compare_diff spaces-expect out
'
-test_expect_success 'diff: nice spaces with --indent-heuristic' '
- git diff --indent-heuristic old new -- spaces.txt >out-compacted &&
+test_expect_success 'diff: nice spaces with --compaction-heuristic' '
+ git diff --compaction-heuristic old new -- spaces.txt >out-compacted &&
compare_diff spaces-compacted-expect out-compacted
'
-test_expect_success 'diff: nice spaces with diff.indentHeuristic' '
- git -c diff.indentHeuristic=true diff old new -- spaces.txt >out-compacted2 &&
+test_expect_success 'diff: nice spaces with diff.compactionHeuristic' '
+ git -c diff.compactionHeuristic=true diff old new -- spaces.txt >out-compacted2 &&
compare_diff spaces-compacted-expect out-compacted2
'
-test_expect_success 'diff: --no-indent-heuristic overrides config' '
- git -c diff.indentHeuristic=true diff --no-indent-heuristic old new -- spaces.txt >out2 &&
+test_expect_success 'diff: --no-compaction-heuristic overrides config' '
+ git -c diff.compactionHeuristic=true diff --no-compaction-heuristic old new -- spaces.txt >out2 &&
compare_diff spaces-expect out2
'
-test_expect_success 'diff: --indent-heuristic with --patience' '
- git diff --indent-heuristic --patience old new -- spaces.txt >out-compacted3 &&
+test_expect_success 'diff: --compaction-heuristic with --patience' '
+ git diff --compaction-heuristic --patience old new -- spaces.txt >out-compacted3 &&
compare_diff spaces-compacted-expect out-compacted3
'
-test_expect_success 'diff: --indent-heuristic with --histogram' '
- git diff --indent-heuristic --histogram old new -- spaces.txt >out-compacted4 &&
+test_expect_success 'diff: --compaction-heuristic with --histogram' '
+ git diff --compaction-heuristic --histogram old new -- spaces.txt >out-compacted4 &&
compare_diff spaces-compacted-expect out-compacted4
'
@@ -187,8 +187,8 @@ test_expect_success 'diff: ugly functions' '
compare_diff functions-expect out
'
-test_expect_success 'diff: nice functions with --indent-heuristic' '
- git diff --indent-heuristic old new -- functions.c >out-compacted &&
+test_expect_success 'diff: nice functions with --compaction-heuristic' '
+ git diff --compaction-heuristic old new -- functions.c >out-compacted &&
compare_diff functions-compacted-expect out-compacted
'
@@ -197,18 +197,18 @@ test_expect_success 'blame: ugly spaces' '
compare_blame spaces-expect out-blame
'
-test_expect_success 'blame: nice spaces with --indent-heuristic' '
- git blame --indent-heuristic old..new -- spaces.txt >out-blame-compacted &&
+test_expect_success 'blame: nice spaces with --compaction-heuristic' '
+ git blame --compaction-heuristic old..new -- spaces.txt >out-blame-compacted &&
compare_blame spaces-compacted-expect out-blame-compacted
'
-test_expect_success 'blame: nice spaces with diff.indentHeuristic' '
- git -c diff.indentHeuristic=true blame old..new -- spaces.txt >out-blame-compacted2 &&
+test_expect_success 'blame: nice spaces with diff.compactionHeuristic' '
+ git -c diff.compactionHeuristic=true blame old..new -- spaces.txt >out-blame-compacted2 &&
compare_blame spaces-compacted-expect out-blame-compacted2
'
-test_expect_success 'blame: --no-indent-heuristic overrides config' '
- git -c diff.indentHeuristic=true blame --no-indent-heuristic old..new -- spaces.txt >out-blame2 &&
+test_expect_success 'blame: --no-compaction-heuristic overrides config' '
+ git -c diff.compactionHeuristic=true blame --no-compaction-heuristic old..new -- spaces.txt >out-blame2 &&
git blame old..new -- spaces.txt >out-blame &&
compare_blame spaces-expect out-blame2
'
diff --git a/xdiff/xdiff.h b/xdiff/xdiff.h
index 8db16d4ae6..7423f77fc8 100644
--- a/xdiff/xdiff.h
+++ b/xdiff/xdiff.h
@@ -42,7 +42,6 @@ extern "C" {
#define XDF_IGNORE_BLANK_LINES (1 << 7)
#define XDF_COMPACTION_HEURISTIC (1 << 8)
-#define XDF_INDENT_HEURISTIC (1 << 9)
#define XDL_EMIT_FUNCNAMES (1 << 0)
#define XDL_EMIT_FUNCCONTEXT (1 << 2)
diff --git a/xdiff/xdiffi.c b/xdiff/xdiffi.c
index 760fbb6db7..6cb96219cb 100644
--- a/xdiff/xdiffi.c
+++ b/xdiff/xdiffi.c
@@ -906,22 +906,7 @@ int xdl_change_compact(xdfile_t *xdf, xdfile_t *xdfo, long flags) {
if (group_previous(xdfo, &go))
xdl_bug("group sync broken sliding to match");
}
- } else if ((flags & XDF_COMPACTION_HEURISTIC) && blank_lines) {
- /*
- * Compaction heuristic: if it is possible to shift the
- * group to make its bottom line a blank line, do so.
- *
- * As we already shifted the group forward as far as
- * possible in the earlier loop, we only need to handle
- * backward shifts, not forward ones.
- */
- while (!is_blank_line(xdf->recs[g.end - 1], flags)) {
- if (group_slide_up(xdf, &g, flags))
- xdl_bug("blank line disappeared");
- if (group_previous(xdfo, &go))
- xdl_bug("group sync broken sliding to blank line");
- }
- } else if (flags & XDF_INDENT_HEURISTIC) {
+ } else if (flags & XDF_COMPACTION_HEURISTIC) {
/*
* Indent heuristic: a group of pure add/delete lines
* implies two splits, one between the end of the "before"
^ 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