* Re: v2.10.0: ls-tree exit status is always 0, this differs from ls(1)
From: Steffen Nurpmeso @ 2016-09-21 19:40 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <xmqqr38d9ova.fsf@gitster.mtv.corp.google.com>
[-- Attachment #1: Type: text/plain, Size: 648 bytes --]
Hello.
Junio C Hamano <gitster@pobox.com> wrote:
|Steffen Nurpmeso <steffen@sdaoden.eu> writes:
|> I think this behaviour contradicts the manual which strongly links
|> ls-tree to ls(1):
|
|Patches to the documentation is very much welcomed.
The below could serve this purpose.
|Somewhere the similarity must end, and actually it ends a lot
|earlier, as "/bin/ls" takes exact paths while "ls-tree" (or any
|other Git command for that matter) takes a pathspec pattern,
|and not having a path that matches the pathspec pattern is not
|an error condition.
I was just surprised to see nothing and get no feedback at all.
Ciao!
--steffen
[-- Attachment #2: git-ls-tree-doc.diff --]
[-- Type: text/x-diff, Size: 539 bytes --]
diff --git a/Documentation/git-ls-tree.txt b/Documentation/git-ls-tree.txt
index dbc91f9..8ebeced 100644
--- a/Documentation/git-ls-tree.txt
+++ b/Documentation/git-ls-tree.txt
@@ -33,6 +33,10 @@ in the current working directory. Note that:
However, the current working directory can be ignored by passing
--full-tree option.
+ - the behaviour is different to that of "/bin/ls" in sofar as non-existing
+ '<path>' arguments are silently ignored and not reflected in the exit
+ status code.
+
OPTIONS
-------
<tree-ish>::
^ permalink raw reply related
* Re: [PATCH v4 3/3] regex: use regexec_buf()
From: Junio C Hamano @ 2016-09-21 19:18 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git, Jeff King, Benjamin Kramer, René Scharfe
In-Reply-To: <53f3609d99c865d59d7bfd8219a5334339e9e6bc.1474482164.git.johannes.schindelin@gmx.de>
Johannes Schindelin <johannes.schindelin@gmx.de> writes:
> The new regexec_buf() function operates on buffers with an explicitly
> specified length, rather than NUL-terminated strings.
>
> We need to use this function whenever the buffer we want to pass to
> regexec() may have been mmap()ed (and is hence not NUL-terminated).
>
> Note: the original motivation for this patch was to fix a bug where
> `git diff -G <regex>` would crash. This patch converts more callers,
> though, some of which explicitly allocated and constructed
> NUL-terminated strings (or worse: modified read-only buffers to insert
> NULs).
>
> Some of the buffers actually may be NUL-terminated. As regexec_buf()
> uses REG_STARTEND where available, but has to fall back to allocating
> and constructing NUL-terminated strings where REG_STARTEND is not
> available, this makes the code less efficient in the latter case.
>
> However, given the widespread support for REG_STARTEND, combined with
> the improved ease of code maintenance, we strike the balance in favor
> of REG_STARTEND.
The last paragraph can go (2/3 was already justified separately),
and the paragraph before that needs rewording, as you no longer do
the "duplicate, run regexec, and free" dance.
Will comment on the patch text itself later.
Thanks for following it through. This topic actually fell under my
radar until now.
^ permalink raw reply
* Re: [PATCH v4 2/3] regex: add regexec_buf() that can work on a non NUL-terminated string
From: Junio C Hamano @ 2016-09-21 19:17 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git, Jeff King, Benjamin Kramer, René Scharfe
In-Reply-To: <270cea11c4d8bfb332a6c014a11673b7f4666ee4.1474482164.git.johannes.schindelin@gmx.de>
Johannes Schindelin <johannes.schindelin@gmx.de> writes:
> ...
> Happily, there is an extension to regexec() introduced by the NetBSD
> project and present in all major regex implementation including
> Linux', MacOSX' and the one Git includes in compat/regex/: by using
> the (non-POSIX) REG_STARTEND flag, it is possible to tell the
> regexec() function that it should only look at the offsets between
> pmatch[0].rm_so and pmatch[0].rm_eo.
>
> That is exactly what we need.
Wonderful.
> Since support for REG_STARTEND is so widespread by now, let's just
> introduce a helper function that uses it, and fall back to allocating
> and constructing a NUL-terminated when REG_STARTEND is not available.
I'd somehow reword the last paragraph here, though ;-)
Since support for REG_STARTEND is so widespread by now, let's just
introduce a helper function that always uses it, and tell people
on a platform whose regex library does not support it to use the
one from our compat/regex/ directory.
The patch itself looks very sane. Thanks.
^ permalink raw reply
* Re: [PATCH 3/3] gitweb: Link to "git describe"'d commits in log messages
From: Jakub Narębski @ 2016-09-21 19:13 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason; +Cc: Git, Junio C Hamano
In-Reply-To: <CACBZZX5QF7FztmU-mqOEx40kC-PpQ9SqcL8+3HtuntcRZ1tWzA@mail.gmail.com>
W dniu 21.09.2016 o 19:58, Ævar Arnfjörð Bjarmason pisze:
> On Wed, Sep 21, 2016 at 7:09 PM, Jakub Narębski <jnareb@gmail.com> wrote:
>> W dniu 21.09.2016 o 13:44, Ævar Arnfjörð Bjarmason napisał:
>>> + (?<!-) # see strbuf_check_tag_ref(). Tags can't start with -
>>> + [A-Za-z0-9.-]+
>>> + (?!\.) # refs can't end with ".", see check_refname_format()
>>
>> If we can assume that tag name is at least two characters (instead of
>> at least one character), we could get rid of those extended regexp
>> lookaround assertions:
>>
>> (?<!pattern) - zero-width negative lookbehind assertion
>> (?!pattern) - zero-width negative lookahead assertion
>>
>> That is:
>>
>> + [A-Za-z0-9.] # see strbuf_check_tag_ref(). Tags can't start with -
>> + [A-Za-z0-9.-]*
>> + [A-Za-z0-9-] # refs can't end with ".", see check_refname_format()
>
> Why get rid of them? I'm all for improving the regex, there's bound to
> be lots of bugs in it, but since it's perl we can freely use its
> extended features.
Ah, all right. I was wondering how zero width assertions / patterns
interact with each other, but zero-width negative lookaround assertions
are really quite simple.
>
>> Also, the canonical documentation for what is allowed in refnames
>> is git-check-ref-format(1)... though it does not look like it includes
>> "tags cannot start with '-'".
>
> Yeah, looks like that manpage needs to be patched.
Right.
>
>> Anyway, perhaps 'is it valid refname' could be passed to a subroutine,
>> or a named regexp (which might be more involved, like disallowing two
>> consecutive dots, e.g. "(?!.*\.{2})" at beginning).
I wonder if rules for valid tag name can be described in extended
regexp, and if it is, how readable would it be.
Regards,
--
Jakub Narębski
^ permalink raw reply
* Re: [PATCH v2 2/3] init: do not set core.worktree more often than necessary
From: Junio C Hamano @ 2016-09-21 18:44 UTC (permalink / raw)
To: Nguyễn Thái Ngọc Duy; +Cc: git, git, max.nordlund
In-Reply-To: <20160921112939.3444-3-pclouds@gmail.com>
Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:
> When "git init" is called with GIT_WORK_TREE environment set, we want to
> keep this worktree's location in core.worktree so the user does not have
> to set the environment again and again. See ef6f0af (git-init: set
> core.worktree if GIT_WORK_TREE is specified - 2007-07-04)
>
> We detect that by this logic (in needs_work_tree_config): normally
> worktree's top dir would contains ".git" directory, if this is not true,
s/contains/contain/;
> worktree is probably set to elsewhere by the user.
>
> Unfortunately when it calls get_git_dir() it does not take ".git" files
> into account. When we find a .git file, we immediately follow the file
> until we find the real ".git" directory. The location of this first
> ".git" file is lost.
>
> The .git file would satisfy the logic above and not create
> core.worktree (correct). But because the final .git's location is used,
> needs_work_tree_config() is misled and creates core.worktree anyway.
The above explanation makes it sound like the correct fix belongs to
needs_work_tree_config(), though.
I am starting to wonder if what ef6f0af did was misguided and we are
better off without setting core.worktree ourselves, but that is a
different issue.
> diff --git a/builtin/init-db.c b/builtin/init-db.c
> index d5d7558..0d5cc76 100644
> --- a/builtin/init-db.c
> +++ b/builtin/init-db.c
> @@ -23,6 +23,7 @@ static int init_is_bare_repository = 0;
> static int init_shared_repository = -1;
> static const char *init_db_template_dir;
> static const char *git_link;
> +static const char *original_git_dir;
>
> static void copy_templates_1(struct strbuf *path, struct strbuf *template,
> DIR *dir)
> @@ -263,7 +264,7 @@ static int create_default_files(const char *template_path)
> /* allow template config file to override the default */
> if (log_all_ref_updates == -1)
> git_config_set("core.logallrefupdates", "true");
> - if (needs_work_tree_config(get_git_dir(), work_tree))
> + if (needs_work_tree_config(original_git_dir, work_tree))
> git_config_set("core.worktree", work_tree);
> }
>
> @@ -314,6 +315,8 @@ static void create_object_directory(void)
> int set_git_dir_init(const char *git_dir, const char *real_git_dir,
> int exist_ok)
> {
> + original_git_dir = xstrdup(real_path(git_dir));
> +
> if (real_git_dir) {
> struct stat st;
The function being extern bothers me. The create_default_files()
function, which is the only thing consumes this variable, is called
only from init_db(), and I'd prefer to see some way to guarantee
that everybody who calls init_db() calls set_git_dir_init()
beforehand. Right now, cmd_init_db() and cmd_clone() are the only
ones that call init_db() and they both call set_dir_git_init(); if a
new caller starts calling init_db() and forgets to call the other
one, that caller will be buggy.
Perhaps a comment before init_db() to tell callers to always call
the other one is the least thing necessary?
> diff --git a/t/t0001-init.sh b/t/t0001-init.sh
> index 488564b..b8fc588 100755
> --- a/t/t0001-init.sh
> +++ b/t/t0001-init.sh
> @@ -400,9 +400,11 @@ test_expect_success 're-init from a linked worktree' '
> test_commit first &&
> git worktree add ../linked-worktree &&
> mv .git/info/exclude expected-exclude &&
> + cp .git/config expected-config &&
> find .git/worktrees -print | sort >expected &&
> git -C ../linked-worktree init &&
> test_cmp expected-exclude .git/info/exclude &&
> + test_cmp expected-config .git/config &&
> find .git/worktrees -print | sort >actual &&
> test_cmp expected actual
> )
^ permalink raw reply
* [PATCH v4 1.5/3] fixup! regex: -G<pattern> feeds a non NUL-terminated string to regexec() and fails
From: Johannes Schindelin @ 2016-09-21 18:34 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jeff King, Benjamin Kramer, René Scharfe
Sorry, I should have squashed that in, but I *just* noticed the conflict
with 433860f (diff: improve positioning of add/delete blocks in diffs,
2016-09-05).
---
t/{t4061-diff-pickaxe.sh => t4062-diff-pickaxe.sh} | 0
1 file changed, 0 insertions(+), 0 deletions(-)
rename t/{t4061-diff-pickaxe.sh => t4062-diff-pickaxe.sh} (100%)
diff --git a/t/t4061-diff-pickaxe.sh b/t/t4062-diff-pickaxe.sh
similarity index 100%
rename from t/t4061-diff-pickaxe.sh
rename to t/t4062-diff-pickaxe.sh
--
2.10.0.windows.1.10.g803177d
^ permalink raw reply
* Re: [PATCH 2/3] gitweb: Link to 7-character SHA1SUMS in commit messages
From: Jakub Narębski @ 2016-09-21 18:28 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason; +Cc: Git, Junio C Hamano
In-Reply-To: <CACBZZX5mYuDUMHfurKEbCqodZkhYygQz+-G6VmKW+AMnCycm9g@mail.gmail.com>
W dniu 21.09.2016 o 20:04, Ævar Arnfjörð Bjarmason pisze:
> On Wed, Sep 21, 2016 at 6:26 PM, Jakub Narębski <jnareb@gmail.com> wrote:
>
>> P.S. I have reworking of commit message parsing and enhancement in my
>> long, long and dated gitweb TODO list :-(
>
> Anything specific you could share?
Some of TODO I would have to bring from backups, as the computer on
which I did majority of gitweb development has since died (from old
age).
The list includes:
- implement caching of gitweb output
- revamp handling of encoding (UTF-8 with fallback encoding)
- split gitweb into modules, while maintaining ease of install
- refactor handling of diffs
- better handling of config files
- document URI structure, perhaps revamp URI parsing and generation
- make commit message transformation generic
(see below)
>
> One thing that would be a lot faster in Perl is if we didn't have to
> pass the log around as split-up lines and could just operate on it as
> one big string.
Well, there are a few transformations that commit message undergoes
in gitweb, including linking SHA1, optional linking of bug numbers
to bug tracker, and syntax highlighting of signoff lines (trailer
lines).
I would like to have this cleaned up, and refactored. With all
those transformations we would need to keep account which parts
are HTML, and which not and need escaping (note: URI escape !=
HTML escape).
>
> It would make some code like git_print_log() a bit more complex /
> fragile, since it would have to work on multi-line strings, but
> anything that needed to do a regex match / replacement would be much
> faster.
Would it? Did you perform any synthetic micro-benchmark?
>
> But OTOH I think perhaps we're worrying about nothing when it comes to
> the performance. I haven't been able to make gitweb display more than
> a 100 or so commits at a time (haven't found where exactly in the code
> these limits are), any munging we do on the log messages would have to
> be pretty damn slow to matter.
sub git_log_generic {
# [...]
my @commitlist =
parse_commits($commit_hash, 101, (100 * $page),
defined $file_name ? ($file_name, "--full-history") : ());
Here you have it (it probably should be a constant; this number can be
found in a few other places).
Best,
--
Jakub Narębski
^ permalink raw reply
* [PATCH v4 3/3] regex: use regexec_buf()
From: Johannes Schindelin @ 2016-09-21 18:24 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jeff King, Benjamin Kramer, René Scharfe
In-Reply-To: <cover.1474482164.git.johannes.schindelin@gmx.de>
The new regexec_buf() function operates on buffers with an explicitly
specified length, rather than NUL-terminated strings.
We need to use this function whenever the buffer we want to pass to
regexec() may have been mmap()ed (and is hence not NUL-terminated).
Note: the original motivation for this patch was to fix a bug where
`git diff -G <regex>` would crash. This patch converts more callers,
though, some of which explicitly allocated and constructed
NUL-terminated strings (or worse: modified read-only buffers to insert
NULs).
Some of the buffers actually may be NUL-terminated. As regexec_buf()
uses REG_STARTEND where available, but has to fall back to allocating
and constructing NUL-terminated strings where REG_STARTEND is not
available, this makes the code less efficient in the latter case.
However, given the widespread support for REG_STARTEND, combined with
the improved ease of code maintenance, we strike the balance in favor
of REG_STARTEND.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
diff.c | 3 ++-
diffcore-pickaxe.c | 18 ++++++++----------
grep.c | 14 ++------------
t/t4061-diff-pickaxe.sh | 2 +-
xdiff-interface.c | 13 ++++---------
5 files changed, 17 insertions(+), 33 deletions(-)
diff --git a/diff.c b/diff.c
index c6da383..fb99235 100644
--- a/diff.c
+++ b/diff.c
@@ -952,7 +952,8 @@ static int find_word_boundaries(mmfile_t *buffer, regex_t *word_regex,
{
if (word_regex && *begin < buffer->size) {
regmatch_t match[1];
- if (!regexec(word_regex, buffer->ptr + *begin, 1, match, 0)) {
+ if (!regexec_buf(word_regex, buffer->ptr + *begin,
+ buffer->size - *begin, 1, match, 0)) {
char *p = memchr(buffer->ptr + *begin + match[0].rm_so,
'\n', match[0].rm_eo - match[0].rm_so);
*end = p ? p - buffer->ptr : match[0].rm_eo + *begin;
diff --git a/diffcore-pickaxe.c b/diffcore-pickaxe.c
index 55067ca..9795ca1 100644
--- a/diffcore-pickaxe.c
+++ b/diffcore-pickaxe.c
@@ -23,7 +23,6 @@ static void diffgrep_consume(void *priv, char *line, unsigned long len)
{
struct diffgrep_cb *data = priv;
regmatch_t regmatch;
- int hold;
if (line[0] != '+' && line[0] != '-')
return;
@@ -33,11 +32,8 @@ static void diffgrep_consume(void *priv, char *line, unsigned long len)
* caller early.
*/
return;
- /* Yuck -- line ought to be "const char *"! */
- hold = line[len];
- line[len] = '\0';
- data->hit = !regexec(data->regexp, line + 1, 1, ®match, 0);
- line[len] = hold;
+ data->hit = !regexec_buf(data->regexp, line + 1, len - 1, 1,
+ ®match, 0);
}
static int diff_grep(mmfile_t *one, mmfile_t *two,
@@ -50,9 +46,11 @@ static int diff_grep(mmfile_t *one, mmfile_t *two,
xdemitconf_t xecfg;
if (!one)
- return !regexec(regexp, two->ptr, 1, ®match, 0);
+ return !regexec_buf(regexp, two->ptr, two->size,
+ 1, ®match, 0);
if (!two)
- return !regexec(regexp, one->ptr, 1, ®match, 0);
+ return !regexec_buf(regexp, one->ptr, one->size,
+ 1, ®match, 0);
/*
* We have both sides; need to run textual diff and see if
@@ -83,8 +81,8 @@ static unsigned int contains(mmfile_t *mf, regex_t *regexp, kwset_t kws)
regmatch_t regmatch;
int flags = 0;
- assert(data[sz] == '\0');
- while (*data && !regexec(regexp, data, 1, ®match, flags)) {
+ while (*data &&
+ !regexec_buf(regexp, data, sz, 1, ®match, flags)) {
flags |= REG_NOTBOL;
data += regmatch.rm_eo;
if (*data && regmatch.rm_so == regmatch.rm_eo)
diff --git a/grep.c b/grep.c
index d7d00b8..1194d35 100644
--- a/grep.c
+++ b/grep.c
@@ -898,17 +898,6 @@ static int fixmatch(struct grep_pat *p, char *line, char *eol,
}
}
-static int regmatch(const regex_t *preg, char *line, char *eol,
- regmatch_t *match, int eflags)
-{
-#ifdef REG_STARTEND
- match->rm_so = 0;
- match->rm_eo = eol - line;
- eflags |= REG_STARTEND;
-#endif
- return regexec(preg, line, 1, match, eflags);
-}
-
static int patmatch(struct grep_pat *p, char *line, char *eol,
regmatch_t *match, int eflags)
{
@@ -919,7 +908,8 @@ static int patmatch(struct grep_pat *p, char *line, char *eol,
else if (p->pcre_regexp)
hit = !pcrematch(p, line, eol, match, eflags);
else
- hit = !regmatch(&p->regexp, line, eol, match, eflags);
+ hit = !regexec_buf(&p->regexp, line, eol - line, 1, match,
+ eflags);
return hit;
}
diff --git a/t/t4061-diff-pickaxe.sh b/t/t4061-diff-pickaxe.sh
index 5929f2e..f0bf50b 100755
--- a/t/t4061-diff-pickaxe.sh
+++ b/t/t4061-diff-pickaxe.sh
@@ -14,7 +14,7 @@ test_expect_success setup '
test_tick &&
git commit -m "A 4k file"
'
-test_expect_failure '-G matches' '
+test_expect_success '-G matches' '
git diff --name-only -G "^0{4096}$" HEAD^ >out &&
test 4096-zeroes.txt = "$(cat out)"
'
diff --git a/xdiff-interface.c b/xdiff-interface.c
index 3bfc69c..060038c 100644
--- a/xdiff-interface.c
+++ b/xdiff-interface.c
@@ -214,11 +214,10 @@ struct ff_regs {
static long ff_regexp(const char *line, long len,
char *buffer, long buffer_size, void *priv)
{
- char *line_buffer;
struct ff_regs *regs = priv;
regmatch_t pmatch[2];
int i;
- int result = -1;
+ int result;
/* Exclude terminating newline (and cr) from matching */
if (len > 0 && line[len-1] == '\n') {
@@ -228,18 +227,16 @@ static long ff_regexp(const char *line, long len,
len--;
}
- line_buffer = xstrndup(line, len); /* make NUL terminated */
-
for (i = 0; i < regs->nr; i++) {
struct ff_reg *reg = regs->array + i;
- if (!regexec(®->re, line_buffer, 2, pmatch, 0)) {
+ if (!regexec_buf(®->re, line, len, 2, pmatch, 0)) {
if (reg->negate)
- goto fail;
+ return -1;
break;
}
}
if (regs->nr <= i)
- goto fail;
+ return -1;
i = pmatch[1].rm_so >= 0 ? 1 : 0;
line += pmatch[i].rm_so;
result = pmatch[i].rm_eo - pmatch[i].rm_so;
@@ -248,8 +245,6 @@ static long ff_regexp(const char *line, long len,
while (result > 0 && (isspace(line[result - 1])))
result--;
memcpy(buffer, line, result);
- fail:
- free(line_buffer);
return result;
}
--
2.10.0.windows.1.10.g803177d
^ permalink raw reply related
* [PATCH v4 2/3] regex: add regexec_buf() that can work on a non NUL-terminated string
From: Johannes Schindelin @ 2016-09-21 18:24 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jeff King, Benjamin Kramer, René Scharfe
In-Reply-To: <cover.1474482164.git.johannes.schindelin@gmx.de>
We just introduced a test that demonstrates that our sloppy use of
regexec() on a mmap()ed area can result in incorrect results or even
hard crashes.
So what we need to fix this is a function that calls regexec() on a
length-delimited, rather than a NUL-terminated, string.
Happily, there is an extension to regexec() introduced by the NetBSD
project and present in all major regex implementation including
Linux', MacOSX' and the one Git includes in compat/regex/: by using
the (non-POSIX) REG_STARTEND flag, it is possible to tell the
regexec() function that it should only look at the offsets between
pmatch[0].rm_so and pmatch[0].rm_eo.
That is exactly what we need.
Since support for REG_STARTEND is so widespread by now, let's just
introduce a helper function that uses it, and fall back to allocating
and constructing a NUL-terminated when REG_STARTEND is not available.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
Makefile | 3 ++-
git-compat-util.h | 13 +++++++++++++
2 files changed, 15 insertions(+), 1 deletion(-)
diff --git a/Makefile b/Makefile
index df4f86b..c6f7f66 100644
--- a/Makefile
+++ b/Makefile
@@ -301,7 +301,8 @@ all::
# crashes due to allocation and free working on different 'heaps'.
# It's defined automatically if USE_NED_ALLOCATOR is set.
#
-# Define NO_REGEX if you have no or inferior regex support in your C library.
+# Define NO_REGEX if your C library lacks regex support with REG_STARTEND
+# feature.
#
# Define HAVE_DEV_TTY if your system can open /dev/tty to interact with the
# user.
diff --git a/git-compat-util.h b/git-compat-util.h
index 37cce07..8aab0c3 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -977,6 +977,19 @@ void git_qsort(void *base, size_t nmemb, size_t size,
#define qsort git_qsort
#endif
+#ifndef REG_STARTEND
+#error "Git requires REG_STARTEND support. Compile with NO_REGEX=NeedsStartEnd"
+#endif
+
+static inline int regexec_buf(const regex_t *preg, const char *buf, size_t size,
+ size_t nmatch, regmatch_t pmatch[], int eflags)
+{
+ assert(nmatch > 0 && pmatch);
+ pmatch[0].rm_so = 0;
+ pmatch[0].rm_eo = size;
+ return regexec(preg, buf, nmatch, pmatch, eflags | REG_STARTEND);
+}
+
#ifndef DIR_HAS_BSD_GROUP_SEMANTICS
# define FORCE_DIR_SET_GID S_ISGID
#else
--
2.10.0.windows.1.10.g803177d
^ permalink raw reply related
* [PATCH v4 1/3] regex: -G<pattern> feeds a non NUL-terminated string to regexec() and fails
From: Johannes Schindelin @ 2016-09-21 18:23 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jeff King, Benjamin Kramer, René Scharfe
In-Reply-To: <cover.1474482164.git.johannes.schindelin@gmx.de>
When our pickaxe code feeds file contents to regexec(), it implicitly
assumes that the file contents are read into implicitly NUL-terminated
buffers (i.e. that we overallocate by 1, appending a single '\0').
This is not so.
In particular when the file contents are simply mmap()ed, we can be
virtually certain that the buffer is preceding uninitialized bytes, or
invalid pages.
Note that the test we add here is known to be flakey: we simply cannot
know whether the byte following the mmap()ed ones is a NUL or not.
Typically, on Linux the test passes. On Windows, it fails virtually
every time due to an access violation (that's a segmentation fault for
you Unix-y people out there). And Windows would be correct: the
regexec() call wants to operate on a regular, NUL-terminated string,
there is no NUL in the mmap()ed memory range, and it is undefined
whether the next byte is even legal to access.
When run with --valgrind it demonstrates quite clearly the breakage, of
course.
Being marked with `test_expect_failure`, this test will sometimes be
declare "TODO fixed", even if it only passes by mistake.
This test case represents a Minimal, Complete and Verifiable Example of
a breakage reported by Chris Sidi.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
t/t4061-diff-pickaxe.sh | 22 ++++++++++++++++++++++
1 file changed, 22 insertions(+)
create mode 100755 t/t4061-diff-pickaxe.sh
diff --git a/t/t4061-diff-pickaxe.sh b/t/t4061-diff-pickaxe.sh
new file mode 100755
index 0000000..5929f2e
--- /dev/null
+++ b/t/t4061-diff-pickaxe.sh
@@ -0,0 +1,22 @@
+#!/bin/sh
+#
+# Copyright (c) 2016 Johannes Schindelin
+#
+
+test_description='Pickaxe options'
+
+. ./test-lib.sh
+
+test_expect_success setup '
+ test_commit initial &&
+ printf "%04096d" 0 >4096-zeroes.txt &&
+ git add 4096-zeroes.txt &&
+ test_tick &&
+ git commit -m "A 4k file"
+'
+test_expect_failure '-G matches' '
+ git diff --name-only -G "^0{4096}$" HEAD^ >out &&
+ test 4096-zeroes.txt = "$(cat out)"
+'
+
+test_done
--
2.10.0.windows.1.10.g803177d
^ permalink raw reply related
* [PATCH v4 0/3] Fix a segfault caused by regexec() being called on mmap()ed data
From: Johannes Schindelin @ 2016-09-21 18:23 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jeff King, Benjamin Kramer, René Scharfe
In-Reply-To: <cover.1473321437.git.johannes.schindelin@gmx.de>
[-- Attachment #1: Type: text/plain, Size: 4565 bytes --]
[Cc:ing Benjamin Kramer & René Scharfe because they both worked on
the REG_STARTEND code in grep.c that I replace in this iteration of the
patch series]
This patch series addresses a problem where `git diff` is called using
`-G` or `-S --pickaxe-regex` on new-born files that are configured
without user diff drivers, and that hence get mmap()ed into memory.
The problem with that: mmap()ed memory is *not* NUL-terminated, yet the
pickaxe code calls regexec() on it just the same.
This problem has been reported by my colleague Chris Sidi.
We solve this by introducing a helper, regexec_buf(), that takes a
pointer and a length instead of a NUL-terminated string.
This helper then uses REG_STARTEND where available, and falls back to
allocating and constructing a NUL-terminated string. Given the
wide-spread support for REG_STARTEND (Linux has it, MacOSX has it, Git
for Windows has it because it uses compat/regex/ that has it), I think
this is a fair trade-off.
Changes since v3:
- reworded the onelines as per Junio's suggestions.
- removed fallback when REG_STARTEND is not supported, in favor of
requiring NO_REGEX.
- removed the regmatch() function from grep.c, in favor of using
regexec_buf().
Johannes Schindelin (3):
regex: -G<pattern> feeds a non NUL-terminated string to regexec() and
fails
regex: add regexec_buf() that can work on a non NUL-terminated string
regex: use regexec_buf()
Makefile | 3 ++-
diff.c | 3 ++-
diffcore-pickaxe.c | 18 ++++++++----------
git-compat-util.h | 13 +++++++++++++
grep.c | 14 ++------------
t/t4061-diff-pickaxe.sh | 22 ++++++++++++++++++++++
xdiff-interface.c | 13 ++++---------
7 files changed, 53 insertions(+), 33 deletions(-)
create mode 100755 t/t4061-diff-pickaxe.sh
Published-As: https://github.com/dscho/git/releases/tag/mmap-regexec-v4
Fetch-It-Via: git fetch https://github.com/dscho/git mmap-regexec-v4
Interdiff vs v3:
diff --git a/Makefile b/Makefile
index df4f86b..c6f7f66 100644
--- a/Makefile
+++ b/Makefile
@@ -301,7 +301,8 @@ all::
# crashes due to allocation and free working on different 'heaps'.
# It's defined automatically if USE_NED_ALLOCATOR is set.
#
-# Define NO_REGEX if you have no or inferior regex support in your C library.
+# Define NO_REGEX if your C library lacks regex support with REG_STARTEND
+# feature.
#
# Define HAVE_DEV_TTY if your system can open /dev/tty to interact with the
# user.
diff --git a/git-compat-util.h b/git-compat-util.h
index 627ec5f..8aab0c3 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -977,25 +977,17 @@ void git_qsort(void *base, size_t nmemb, size_t size,
#define qsort git_qsort
#endif
+#ifndef REG_STARTEND
+#error "Git requires REG_STARTEND support. Compile with NO_REGEX=NeedsStartEnd"
+#endif
+
static inline int regexec_buf(const regex_t *preg, const char *buf, size_t size,
size_t nmatch, regmatch_t pmatch[], int eflags)
{
-#ifdef REG_STARTEND
assert(nmatch > 0 && pmatch);
pmatch[0].rm_so = 0;
pmatch[0].rm_eo = size;
return regexec(preg, buf, nmatch, pmatch, eflags | REG_STARTEND);
-#else
- char *buf2 = xmalloc(size + 1);
- int ret;
-
- memcpy(buf2, buf, size);
- buf2[size] = '\0';
- ret = regexec(preg, buf2, nmatch, pmatch, eflags);
- free(buf2);
-
- return ret;
-#endif
}
#ifndef DIR_HAS_BSD_GROUP_SEMANTICS
diff --git a/grep.c b/grep.c
index d7d00b8..1194d35 100644
--- a/grep.c
+++ b/grep.c
@@ -898,17 +898,6 @@ static int fixmatch(struct grep_pat *p, char *line, char *eol,
}
}
-static int regmatch(const regex_t *preg, char *line, char *eol,
- regmatch_t *match, int eflags)
-{
-#ifdef REG_STARTEND
- match->rm_so = 0;
- match->rm_eo = eol - line;
- eflags |= REG_STARTEND;
-#endif
- return regexec(preg, line, 1, match, eflags);
-}
-
static int patmatch(struct grep_pat *p, char *line, char *eol,
regmatch_t *match, int eflags)
{
@@ -919,7 +908,8 @@ static int patmatch(struct grep_pat *p, char *line, char *eol,
else if (p->pcre_regexp)
hit = !pcrematch(p, line, eol, match, eflags);
else
- hit = !regmatch(&p->regexp, line, eol, match, eflags);
+ hit = !regexec_buf(&p->regexp, line, eol - line, 1, match,
+ eflags);
return hit;
}
--
2.10.0.windows.1.10.g803177d
base-commit: f6727b0509ec3417a5183ba6e658143275a734f5
^ permalink raw reply
* Re: What's cooking in git.git (Sep 2016, #05; Mon, 19)
From: Johannes Schindelin @ 2016-09-21 18:22 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <xmqq8tulclcu.fsf@gitster.mtv.corp.google.com>
Hi Junio,
On Wed, 21 Sep 2016, Junio C Hamano wrote:
> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>
> >> * jk/rebase-i-drop-ident-check (2016-07-29) 1 commit
> >> (merged to 'next' on 2016-08-14 at 6891bcd)
> >> + rebase-interactive: drop early check for valid ident
> >>
> >> Even when "git pull --rebase=preserve" (and the underlying "git
> >> rebase --preserve") can complete without creating any new commit
> >> (i.e. fast-forwards), it still insisted on having a usable ident
> >> information (read: user.email is set correctly), which was less
> >> than nice. As the underlying commands used inside "git rebase"
> >> would fail with a more meaningful error message and advice text
> >> when the bogus ident matters, this extra check was removed.
> >>
> >> Will hold to see if people scream.
> >> cf. <20160729224944.GA23242@sigill.intra.peff.net>
> >
> > Let's do this.
>
> We have already been doing it (i.e. "hold to see if people scream")
> for some time.
I meant: let's merge this to `master`.
> Does it conflict with your effort to reimplement "rebase -i" in C
I do not think so.
> to keep this in 'next'? Do you want it to move to 'master'? I was
> under the impression that it would not make a difference to have or not
> have this patch once your reimplementation gets merged (meaning: the
> removal of the three lines will be done by wholesale removal of
> git-rebase--interactive.sh done the endgame of your series), so...
Oh, I failed to make clear that my patch series do *not* remove
git-rebase--interactive.sh. I just barely started to work to that end.
While the speed improvements are quite noticable, the rebase--helper
command still only implements the performance-critical code paths in C.
There is quite a bit of work left to do before git-rebase--interactive.sh
can be retired:
- --root is not handled via the sequencer yet,
- --preserve-merges is not handled either [*1*],
- the shell script still sets up the state directory,
- option parsing is still all-shell,
- probably more tasks I forgot.
The good news is that these parts can be converted independently from each
other, and even by independent developers (hint, hint ;-)).
Ciao,
Dscho
Footnote *1*: I am not sure that I want to port -p to C: in my view, this
is a failed experiment, to be replaced with a design based on my Git
garden shears. I tend to think that that part should be moved to a new
shell script ("git-rebase--preserve-merges.sh"?) unless some developer
other than me feels strongly enough to put their money where their mouth
is and teach the sequencer about it.
^ permalink raw reply
* Re: [PATCH v2 0/3] Fix git-init in linked worktrees
From: Junio C Hamano @ 2016-09-21 18:18 UTC (permalink / raw)
To: Nguyễn Thái Ngọc Duy; +Cc: git, git, max.nordlund
In-Reply-To: <20160921112939.3444-1-pclouds@gmail.com>
Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:
> v2 requires jk/setup-sequence-update so I could kill my workaround
> patch and avoid conflicts in t0001. And:
>
> - 1/3 has a few missing words back in its commit message
> - 2/3, which was 3/3 in v1, no longer has the ugly hacky
> get_first_git_dir()
> - 3/3 is a new tiny code improvement after the new 2/3
>
> Nguyễn Thái Ngọc Duy (3):
> init: correct re-initialization from a linked worktree
> init: do not set core.worktree more often than necessary
> init: reuse original_git_dir in set_git_dir_init()
Thanks. Will take a look.
^ permalink raw reply
* Re: git-gui, was Re: [PATCH v2 6/6] git-gui: Update Japanese information
From: Junio C Hamano @ 2016-09-21 18:16 UTC (permalink / raw)
To: Vasco Almeida; +Cc: git, Pat Thoyts
In-Reply-To: <1474378663.1884.41.camel@sapo.pt>
Vasco Almeida <vascomalmeida@sapo.pt> writes:
> I have sent some git-gui patches on May this year and I think it will
> add value to accepted them at some point:
Yeah, they may be of value, but the thing is, I am not really in the
position to review or apply them (I don't do git-gui).
If Pat is not going to return, we would need to find volunteers to
be maintainers of "git-gui" first.
Thanks. I may get to these patches when/if I find time, but it is
not likely to happen very soon.
^ permalink raw reply
* Re: [PATCH] git-check-ref-format.txt: fixup documentation
From: Junio C Hamano @ 2016-09-21 18:12 UTC (permalink / raw)
To: Elia Pinto; +Cc: git
In-Reply-To: <20160920073314.22485-1-gitter.spiros@gmail.com>
Elia Pinto <gitter.spiros@gmail.com> writes:
> die is not a standard shell function. Use
> a different shell code for the example.
Thanks.
^ permalink raw reply
* Re: [PATCH tg/add-chmod+x-fix 2/2] t3700-add: protect one --chmod=+x test with POSIXPERM
From: Junio C Hamano @ 2016-09-21 18:12 UTC (permalink / raw)
To: Thomas Gummerer; +Cc: Johannes Sixt, Git Mailing List
In-Reply-To: <20160920193444.GG8254@hank>
Thomas Gummerer <t.gummerer@gmail.com> writes:
>> I am surprised that add --chmod=+x changes only the index, but not
>> the file on disk!?!
>
> I *think* --chmod is mainly thought of as a convenience for git users
> on a filesystem that doesn't have an executable flag. So it was
> introduced this way as the permissions on the file system don't matter
> in that case. A change of that behaviour may make sense for this
> though.
Perhaps we shouldn't even test this, then? I can see future people
wanting to change this behaviour, while I can see argument for not
touching the working tree file, too. It is essential for the main
purpose of the command (i.e. "I want to flip the executable bit for
the path in the index") to make sure that the bit in the index is
changed. Comparing the index with the working tree using "status"
is probably not how you would want to do so. A future breakage may
cause the indexed blob name to change by mistake, and status would
happily report difference but you would not notice its output is
saying "Hey, they are different between the index and the working
tree", while you are expecting ONLY the change in the executable bit.
How about doing
git add foo4 &&
git add --chmod=+x foo4 &&
test_mode_in_index 100755 foo4
instead?
>
>> t/t3700-add.sh | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/t/t3700-add.sh b/t/t3700-add.sh
>> index 16ab2da..13e0dd2 100755
>> --- a/t/t3700-add.sh
>> +++ b/t/t3700-add.sh
>> @@ -361,7 +361,7 @@ test_expect_success 'git add --chmod=[+-]x changes index with already added file
>> test_mode_in_index 100644 xfoo3
>> '
>>
>> -test_expect_success 'file status is changed after git add --chmod=+x' '
>> +test_expect_success POSIXPERM 'file status is changed after git add --chmod=+x' '
>> echo "AM foo4" >expected &&
>> echo foo >foo4 &&
>> git add foo4 &&
>> --
>> 2.10.0.85.gea34e30
>>
^ permalink raw reply
* Re: [PATCH 2/3] gitweb: Link to 7-character SHA1SUMS in commit messages
From: Ævar Arnfjörð Bjarmason @ 2016-09-21 18:04 UTC (permalink / raw)
To: Jakub Narębski; +Cc: Git, Junio C Hamano
In-Reply-To: <64389bcb-c6e4-1d19-54a1-650868b9acb5@gmail.com>
On Wed, Sep 21, 2016 at 6:26 PM, Jakub Narębski <jnareb@gmail.com> wrote:
> P.S. I have reworking of commit message parsing and enhancement in my
> long, long and dated gitweb TODO list :-(
Anything specific you could share?
One thing that would be a lot faster in Perl is if we didn't have to
pass the log around as split-up lines and could just operate on it as
one big string.
It would make some code like git_print_log() a bit more complex /
fragile, since it would have to work on multi-line strings, but
anything that needed to do a regex match / replacement would be much
faster.
But OTOH I think perhaps we're worrying about nothing when it comes to
the performance. I haven't been able to make gitweb display more than
a 100 or so commits at a time (haven't found where exactly in the code
these limits are), any munging we do on the log messages would have to
be pretty damn slow to matter.
> P.P.S. Kay Sievers no longer works on gitweb, and I think no longer
> works at SuSE but at RedHat.
Yup, been getting bounces from his address.
^ permalink raw reply
* Re: [PATCH 3/3] gitweb: Link to "git describe"'d commits in log messages
From: Junio C Hamano @ 2016-09-21 18:01 UTC (permalink / raw)
To: Jakub Narębski; +Cc: Ævar Arnfjörð Bjarmason, git
In-Reply-To: <ba9d9c82-4b5e-4d62-95f1-a1c56cfc70af@gmail.com>
Jakub Narębski <jnareb@gmail.com> writes:
>> When I saw 2/3 I wondered about one thing and 3/3 shares the same,
>> which is that we only use regex match and do not validate for a
>> false match. Would it be too expensive...
>
> It's a matter of balance between false positives (and unresolving
> links) and performance...
Yes, and that is why I asked a simple yes-or-no question. Would it
be too expensive? Your answer seems to be yes.
Have we measured? Is that really a bottleneck? Would it help to
update parse_commits to call a new command "gitweb--helper" that
produces the result of what git_print_log would have done to its
$log argument, for example?
^ permalink raw reply
* Re: [PATCH 3/3] gitweb: Link to "git describe"'d commits in log messages
From: Ævar Arnfjörð Bjarmason @ 2016-09-21 17:58 UTC (permalink / raw)
To: Jakub Narębski; +Cc: Git, Junio C Hamano
In-Reply-To: <fadd75f3-3737-1eaf-30f3-46a2ef132b27@gmail.com>
On Wed, Sep 21, 2016 at 7:09 PM, Jakub Narębski <jnareb@gmail.com> wrote:
> W dniu 21.09.2016 o 13:44, Ævar Arnfjörð Bjarmason napisał:
>
>> Change the log formatting function to know about "git describe" output
>> like v2.8.0-4-g867ad08 in addition to just plain 867ad08.
>
> All right, that is a good plan.
>
>>
>> This also fixes a micro-regression in my change of the minimum SHA1
>> length from 8 to 7, which is that dated tags like
>> hadoop-20160921-113441-20-g094fb7d would start thinking the "20160921"
>> part was a commit.
>
> Actually 20160921 is 8 characters, so assuming that '-' is treated
> as word boundary by Perl, it is not a regression; this false positive
> was there. The new feature would help, instead of linking false match
> it links whole git-describe output.
>
> So this paragraph needs to be changed wrt. the above.
Yeah I just miscounted there, will change that.
> Note that there are quite a bit of shortened SHA-1 that are composed
> entirely from digits, without a-f characters.
*nod*
>>
>> There are still many valid refnames that we don't link to
>> e.g. v2.10.0-rc1~2^2~1 is also a valid way to refer to
>> v2.8.0-4-g867ad08, but I'm not supporting that with this commit,
>> similarly it's trivially possible to create some refnames like
>> "æ/var-gf6727b0" or whatever which won't be picked up by this regex.
>
> Hopefully hierarchical tags are rare. We need to reduce false
> positives.
>
>>
>> There's surely room for improvement here, but I just wanted to address
>> the very common case of sticking "git describe" output into commit
>> messages without trying to link to all possible refnames, that's going
>> to be a rather futile exercise given that this is free text, and it
>> would be prohibitively expensive to look up whether the references in
>> question exist in our repository.
>
> Note that we do not ask Git at the time of displaying commit message
> if the link is valid for performance reasons; we link it, and the link
> may be invalid if it was a false positive.
>
> Note that recommended way to refer to other commit in commit mesages
> is (see Documentation/SubmittingPatches):
>
> If you want to reference a previous commit in the history of a stable
> branch, use the format "abbreviated sha1 (subject, date)",
> with the subject enclosed in a pair of double-quotes, like this:
>
> Commit f86a374 ("pack-bitmap.c: fix a memleak", 2015-03-30)
> noticed that ...
>
> Hmmm... this makes previous commit even more important.
>
>> ---
>> gitweb/gitweb.perl | 18 ++++++++++++++++--
>> 1 file changed, 16 insertions(+), 2 deletions(-)
>>
>> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
>> index 101dbc0..3a52bc7 100755
>> --- a/gitweb/gitweb.perl
>> +++ b/gitweb/gitweb.perl
>> @@ -2036,10 +2036,24 @@ sub format_log_line_html {
>> my $line = shift;
>>
>> $line = esc_html($line, -nbsp=>1);
>> - $line =~ s{\b([0-9a-fA-F]{7,40})\b}{
>> + $line =~ s{
>> + \b
>> + (
>> + # The output of "git describe", e.g. v2.10.0-297-gf6727b0
>> + # or hadoop-20160921-113441-20-g094fb7d
>
> All right, for more complex regular expressions using in-line comments
> (extended regexp in Perl) is a good idea.
>
>> + (?<!-) # see strbuf_check_tag_ref(). Tags can't start with -
>> + [A-Za-z0-9.-]+
>> + (?!\.) # refs can't end with ".", see check_refname_format()
>
> If we can assume that tag name is at least two characters (instead of
> at least one character), we could get rid of those extended regexp
> lookaround assertions:
>
> (?<!pattern) - zero-width negative lookbehind assertion
> (?!pattern) - zero-width negative lookahead assertion
>
> That is:
>
> + [A-Za-z0-9.] # see strbuf_check_tag_ref(). Tags can't start with -
> + [A-Za-z0-9.-]*
> + [A-Za-z0-9-] # refs can't end with ".", see check_refname_format()
Why get rid of them? I'm all for improving the regex, there's bound to
be lots of bugs in it, but since it's perl we can freely use its
extended features.
> Also, the canonical documentation for what is allowed in refnames
> is git-check-ref-format(1)... though it does not look like it includes
> "tags cannot start with '-'".
Yeah, looks like that manpage needs to be patched.
> Anyway, perhaps 'is it valid refname' could be passed to a subroutine,
> or a named regexp (which might be more involved, like disallowing two
> consecutive dots, e.g. "(?!.*\.{2})" at beginning).
>
>> + -g[0-9a-fA-F]{7,40}
>
> If we are limiting to git-describe output, we can get rid of A-F here.
Indeed.
>> + |
>> + # Just a normal looking Git SHA1
>> + [0-9a-fA-F]{7,40}
>> + )
>> + \b
>> + }{
>> $cgi->a({-href => href(action=>"object", hash=>$1),
>> -class => "text"}, $1);
>> - }eg;
>> + }egx;
>>
>> return $line;
>> }
>>
>
> Good work.
>
> I assume that you are using git-describe output in commit messages
> a lot, isn't it?
Yeah, and I got tired of gitweb not linking to any of the commits I
was referencing.
^ permalink raw reply
* Re: v2.9.3 and v2.10.0: `name-ref' HEAD gives wrong branch name
From: Jakub Narębski @ 2016-09-21 17:54 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Bryan Turner, Steffen Nurpmeso, Git Users
In-Reply-To: <xmqq8tulb45n.fsf@gitster.mtv.corp.google.com>
W dniu 21.09.2016 o 18:37, Junio C Hamano pisze:
> Jakub Narębski <jnareb@gmail.com> writes:
>>> Have you tried "git symbolic-ref HEAD"?
>>>
>>> $ git symbolic-ref HEAD
>>> refs/heads/master
>>>
>>> If you don't want the fully-qualified ref, you can add --short:
>>>
>>> $ git symbolic-ref --short HEAD
>>> master
>>
>> This does not work for detached HEAD, but perhaps you don't need
>> to worry about this.
>
> I am not sure what you mean by "does not work". Asking what ref
> HEAD points at to symbolic-ref will tell you it does not point at
> anything by exiting with non-zero status and that can be relied
> upon.
>
> Asking "symbolic-ref HEAD" has been the way how "git branch" and
> other commands find out what branch is currently checked out for
> almost eternity ("git symbolic-ref" appeared in Git v0.99.8).
I'm sorry, I was wrong saying "does not work". The problem is not
that it does not work, but that you need to take care of exit
condition to correctly support detached HEAD state, and not end
with empty name for a branch, for example...
But if handled correctly, git-symbolic-ref is as good as git-rev-parse
as a plumbing to resolve current branch name.
Best,
--
Jakub Narębski
^ permalink raw reply
* Re: [PATCH] gitweb: use highlight's shebang detection
From: Jakub Narębski @ 2016-09-21 17:51 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Ian Kelling, git
In-Reply-To: <xmqq4m59b43v.fsf@gitster.mtv.corp.google.com>
W dniu 21.09.2016 o 18:38, Junio C Hamano pisze:
> Jakub Narębski <jnareb@gmail.com> writes:
>> W dniu 06.09.2016 o 21:00, Ian Kelling pisze:
>>
>>> The highlight binary can detect language by shebang when we can't tell
>>> the syntax type by the name of the file.
>>
>> Was it something always present among highlight[1] binary capabilities,
>> or is it something present only in new enough highlight app? Or only
>> in some specific fork / specific binary? I couldn't find language
>> detection in highlight[1] documentation...
>> ...
>> Thank you for your work on this patch,
>
> Thanks for reviewing. It seems that there will be further exchange
> needed before I can pick it up?
Yes, I think so.
--
Jakub Narębski
^ permalink raw reply
* Re: [PATCH v2] ls-files: add pathspec matching for submodules
From: Junio C Hamano @ 2016-09-21 17:49 UTC (permalink / raw)
To: Brandon Williams; +Cc: git, Nguyễn Thái Ngọc Duy
In-Reply-To: <xmqqmvj19nyp.fsf@gitster.mtv.corp.google.com>
Junio C Hamano <gitster@pobox.com> writes:
> Brandon Williams <bmwill@google.com> writes:
>
>> On a similar but slightly different note. In general do we want
>> the pathspec '??b' to match against the sib/ directory and
>> subsequently have ls-files print all entries inside of the sib/
>> directory? (this is in the non-recursive case)
>
> I'd need to find time to dig a bit of history before I can give a
> firm opinion on this, but here is a knee-jerk version of my reaction.
In the context of what you are doing, i.e. "ls-files that recurses
into submodules", my opinion is that "ls-files --recurse-submodules"
should behave wrt pathspecs AS IF all the submodule contents are
flattened into a single index of the superproject.
In the sample scenario under discussion, i.e.
In the superproject we have these
$ git ls-files -s
100644 c489803d5bdec1755f650854fe7ef5ab7a3ee58d 0 .gitmodules
100644 e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 0 sib/file
160000 1f5a0695289c500f25e7fa55e3ad27e394d1206b 0 sub
In 'sub' submodule we have this
100644 e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 0 file
such a flattend index would look like this:
100644 c489803d5bdec1755f650854fe7ef5ab7a3ee58d 0 .gitmodules
100644 e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 0 sib/file
100644 e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 0 sub/file
i.e. removing 'sub' submodule entry from the index of the
superproject and overlay everything in the submodule with sub/
prefixed to its path.
And with such an index, if and only if a path matches a pathspec,
"git ls-files --recurse-submodules" run at the toplevel with the
same pathspec should show the path. That means
$ git ls-files --recurse-submodules '??b'
would show nothing (not even 'sub'), while
$ git ls-files --recurse-submodules '??b*'
should show sib/file and sub/file. That is because that is how the
command without "--recurse-submodules" working on that flat index
would produce.
The "we have historically two kinds of pathspecs and they differ how
they work with wildcard" is a separate issue, I would think, even
though the result would affect what should happen in the above
example (i.e. if we said "either a pattern match or a literal match
to a leading directory path should make everything underneath
match", '??b' would make sib/<anything> and sub/<anything> to be
shown).
^ permalink raw reply
* Re: [PATCH 3/3] gitweb: Link to "git describe"'d commits in log messages
From: Jakub Narębski @ 2016-09-21 17:49 UTC (permalink / raw)
To: Junio C Hamano, Ævar Arnfjörð Bjarmason; +Cc: git
In-Reply-To: <xmqqvaxp9oyp.fsf@gitster.mtv.corp.google.com>
W dniu 21.09.2016 o 18:50, Junio C Hamano pisze:
> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
>
>> There's surely room for improvement here, but I just wanted to address
>> the very common case of sticking "git describe" output into commit
>> messages without trying to link to all possible refnames, that's going
>> to be a rather futile exercise given that this is free text, and it
>> would be prohibitively expensive to look up whether the references in
>> question exist in our repository.
>
> When I saw 2/3 I wondered about one thing and 3/3 shares the same,
> which is that we only use regex match and do not validate for a
> false match. Would it be too expensive to pick up what _looks_ like
> a rev (e.g. hex or g(refname regexp)-hex) then validate it with
> "rev-parse --verify --quiet" to make sure it is a rev, before
> actually making it a link? Even if are we trying to account for
> people referring to commits that do not exist in this repository
> (e.g. some other project, in a submodule repository, or just an
> earlier incarnation of rebasing that has since been lost), it seems
> to me that it does not help to mark them with a link that won't
> resolve.
I think it could be a good *option*, but revision verification
could be costly, for example in the 'log' view with multiple commits
and multiple revision-like looking candidates, even if we were able
to do it with one command.
Also, "git rev-parse --verify [--quiet]" can verify only one
revision at once, isn't it? Maybe something like 'git cat-file
--batch-check' would be better (one fork)?
It's a matter of balance between false positives (and unresolving
links) and performance...
Best,
--
Jakub Narębski
^ permalink raw reply
* Re: What's cooking in git.git (Sep 2016, #05; Mon, 19)
From: Kevin Daudt @ 2016-09-21 17:45 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <xmqq7fa59mti.fsf@gitster.mtv.corp.google.com>
On Wed, Sep 21, 2016 at 10:36:57AM -0700, Junio C Hamano wrote:
> Kevin Daudt <me@ikke.info> writes:
>
> > On Mon, Sep 19, 2016 at 04:30:34PM -0700, Junio C Hamano wrote:
> >>
> >> * kd/mailinfo-quoted-string (2016-09-19) 2 commits
> >> - mailinfo: unescape quoted-pair in header fields
> >> - t5100-mailinfo: replace common path prefix with variable
> >
> > Is this good enough, or do you want me to look into the feedback from
> > jeff?
>
> If you are talking about the simplified loop that deliberately sets
> a rule that is looser than RFC, yes, I'd like to see you at least
> consider the pros and cons of his approach, which looked nicer to my
> brief reading of it.
>
> It is perfectly OK by me (it may not be so if you ask Peff) if you
> decide that your version is better after doing so, though.
>
> Thanks.
Alright, I'll look into it.
^ permalink raw reply
* Re: What's cooking in git.git (Sep 2016, #05; Mon, 19)
From: Junio C Hamano @ 2016-09-21 17:36 UTC (permalink / raw)
To: Kevin Daudt; +Cc: git
In-Reply-To: <20160921162628.GA27363@ikke.info>
Kevin Daudt <me@ikke.info> writes:
> On Mon, Sep 19, 2016 at 04:30:34PM -0700, Junio C Hamano wrote:
>>
>> * kd/mailinfo-quoted-string (2016-09-19) 2 commits
>> - mailinfo: unescape quoted-pair in header fields
>> - t5100-mailinfo: replace common path prefix with variable
>
> Is this good enough, or do you want me to look into the feedback from
> jeff?
If you are talking about the simplified loop that deliberately sets
a rule that is looser than RFC, yes, I'd like to see you at least
consider the pros and cons of his approach, which looked nicer to my
brief reading of it.
It is perfectly OK by me (it may not be so if you ask Peff) if you
decide that your version is better after doing so, though.
Thanks.
^ 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