* [PATCH 1/2] ls-files: optionally recurse into submodules
From: Brandon Williams @ 2016-09-21 22:42 UTC (permalink / raw)
To: git; +Cc: Brandon Williams
In-Reply-To: <CAKoko1qch_odsEWba0rtCv-DWO0ABS2yprnwGPCgyT6-7H-LdQ@mail.gmail.com>
Allow ls-files to recognize submodules in order to retrieve a list of
files from a repository's submodules. This is done by forking off a
process to recursively call ls-files on all submodules. Also added a
submodule-prefix command in order to prepend paths to child processes.
Signed-off-by: Brandon Williams <bmwill@google.com>
---
Documentation/git-ls-files.txt | 11 +++-
builtin/ls-files.c | 61 +++++++++++++++++++++
t/t3007-ls-files-recurse-submodules.sh | 99 ++++++++++++++++++++++++++++++++++
3 files changed, 170 insertions(+), 1 deletion(-)
create mode 100755 t/t3007-ls-files-recurse-submodules.sh
diff --git a/Documentation/git-ls-files.txt b/Documentation/git-ls-files.txt
index 0d933ac..09e4449 100644
--- a/Documentation/git-ls-files.txt
+++ b/Documentation/git-ls-files.txt
@@ -18,7 +18,9 @@ SYNOPSIS
[--exclude-per-directory=<file>]
[--exclude-standard]
[--error-unmatch] [--with-tree=<tree-ish>]
- [--full-name] [--abbrev] [--] [<file>...]
+ [--full-name] [--recurse-submodules]
+ [--submodule-prefix=<path>]
+ [--abbrev] [--] [<file>...]
DESCRIPTION
-----------
@@ -137,6 +139,13 @@ a space) at the start of each line:
option forces paths to be output relative to the project
top directory.
+--recurse-submodules::
+ Recursively calls ls-files on each submodule in the repository.
+ Currently there is only support for the --cached mode.
+
+--submodule-prefix=<path>::
+ Prepend the provided path to the output of each file
+
--abbrev[=<n>]::
Instead of showing the full 40-byte hexadecimal object
lines, show only a partial prefix.
diff --git a/builtin/ls-files.c b/builtin/ls-files.c
index 00ea91a..ffd9ea6 100644
--- a/builtin/ls-files.c
+++ b/builtin/ls-files.c
@@ -14,6 +14,7 @@
#include "resolve-undo.h"
#include "string-list.h"
#include "pathspec.h"
+#include "run-command.h"
static int abbrev;
static int show_deleted;
@@ -28,6 +29,8 @@ static int show_valid_bit;
static int line_terminator = '\n';
static int debug_mode;
static int show_eol;
+static const char *submodule_prefix;
+static int recurse_submodules;
static const char *prefix;
static int max_prefix_len;
@@ -68,6 +71,21 @@ static void write_eolinfo(const struct cache_entry *ce, const char *path)
static void write_name(const char *name)
{
/*
+ * NEEDSWORK: To make this thread-safe, full_name would have to be owned
+ * by the caller.
+ *
+ * full_name get reused across output lines to minimize the allocation
+ * churn.
+ */
+ static struct strbuf full_name = STRBUF_INIT;
+ if (submodule_prefix && *submodule_prefix) {
+ strbuf_reset(&full_name);
+ strbuf_addstr(&full_name, submodule_prefix);
+ strbuf_addstr(&full_name, name);
+ name = full_name.buf;
+ }
+
+ /*
* With "--full-name", prefix_len=0; this caller needs to pass
* an empty string in that case (a NULL is good for "").
*/
@@ -152,6 +170,26 @@ static void show_killed_files(struct dir_struct *dir)
}
}
+/**
+ * Recursively call ls-files on a submodule
+ */
+static void show_gitlink(const struct cache_entry *ce)
+{
+ struct child_process cp = CHILD_PROCESS_INIT;
+ int status;
+
+ argv_array_push(&cp.args, "ls-files");
+ argv_array_push(&cp.args, "--recurse-submodules");
+ argv_array_pushf(&cp.args, "--submodule-prefix=%s%s/",
+ submodule_prefix ? submodule_prefix : "",
+ ce->name);
+ cp.git_cmd = 1;
+ cp.dir = ce->name;
+ status = run_command(&cp);
+ if (status)
+ exit(status);
+}
+
static void show_ce_entry(const char *tag, const struct cache_entry *ce)
{
int len = max_prefix_len;
@@ -163,6 +201,10 @@ static void show_ce_entry(const char *tag, const struct cache_entry *ce)
len, ps_matched,
S_ISDIR(ce->ce_mode) || S_ISGITLINK(ce->ce_mode)))
return;
+ if (recurse_submodules && S_ISGITLINK(ce->ce_mode)) {
+ show_gitlink(ce);
+ return;
+ }
if (tag && *tag && show_valid_bit &&
(ce->ce_flags & CE_VALID)) {
@@ -468,6 +510,10 @@ int cmd_ls_files(int argc, const char **argv, const char *cmd_prefix)
{ OPTION_SET_INT, 0, "full-name", &prefix_len, NULL,
N_("make the output relative to the project top directory"),
PARSE_OPT_NOARG | PARSE_OPT_NONEG, NULL },
+ OPT_STRING(0, "submodule-prefix", &submodule_prefix,
+ N_("path"), N_("prepend <path> to each file")),
+ OPT_BOOL(0, "recurse-submodules", &recurse_submodules,
+ N_("recurse through submodules")),
OPT_BOOL(0, "error-unmatch", &error_unmatch,
N_("if any <file> is not in the index, treat this as an error")),
OPT_STRING(0, "with-tree", &with_tree, N_("tree-ish"),
@@ -519,6 +565,21 @@ int cmd_ls_files(int argc, const char **argv, const char *cmd_prefix)
if (require_work_tree && !is_inside_work_tree())
setup_work_tree();
+ if (recurse_submodules &&
+ (show_stage || show_deleted || show_others || show_unmerged ||
+ show_killed || show_modified || show_resolve_undo ||
+ show_valid_bit || show_tag || show_eol))
+ die("ls-files --recurse-submodules can only be used in "
+ "--cached mode");
+
+ if (recurse_submodules && error_unmatch)
+ die("ls-files --recurse-submodules does not support "
+ "--error-unmatch");
+
+ if (recurse_submodules && argc)
+ die("ls-files --recurse-submodules does not support path "
+ "arguments");
+
parse_pathspec(&pathspec, 0,
PATHSPEC_PREFER_CWD |
PATHSPEC_STRIP_SUBMODULE_SLASH_CHEAP,
diff --git a/t/t3007-ls-files-recurse-submodules.sh b/t/t3007-ls-files-recurse-submodules.sh
new file mode 100755
index 0000000..caf3815
--- /dev/null
+++ b/t/t3007-ls-files-recurse-submodules.sh
@@ -0,0 +1,99 @@
+#!/bin/sh
+
+test_description='Test ls-files recurse-submodules feature
+
+This test verifies the recurse-submodules feature correctly lists files from
+submodules.
+'
+
+. ./test-lib.sh
+
+test_expect_success 'setup directory structure and submodules' '
+ echo a >a &&
+ mkdir b &&
+ echo b >b/b &&
+ git add a b &&
+ git commit -m "add a and b" &&
+ git init submodule &&
+ echo c >submodule/c &&
+ git -C submodule add c &&
+ git -C submodule commit -m "add c" &&
+ git submodule add ./submodule &&
+ git commit -m "added submodule"
+'
+
+test_expect_success 'ls-files correctly outputs files in submodule' '
+ cat >expect <<-\EOF &&
+ .gitmodules
+ a
+ b/b
+ submodule/c
+ EOF
+
+ git ls-files --recurse-submodules >actual &&
+ test_cmp expect actual
+'
+
+test_expect_success 'ls-files does not output files not added to a repo' '
+ cat >expect <<-\EOF &&
+ .gitmodules
+ a
+ b/b
+ submodule/c
+ EOF
+
+ echo a >not_added &&
+ echo b >b/not_added &&
+ echo c >submodule/not_added &&
+ git ls-files --recurse-submodules >actual &&
+ test_cmp expect actual
+'
+
+test_expect_success 'ls-files recurses more than 1 level' '
+ cat >expect <<-\EOF &&
+ .gitmodules
+ a
+ b/b
+ submodule/.gitmodules
+ submodule/c
+ submodule/subsub/d
+ EOF
+
+ git init submodule/subsub &&
+ echo d >submodule/subsub/d &&
+ git -C submodule/subsub add d &&
+ git -C submodule/subsub commit -m "add d" &&
+ git -C submodule submodule add ./subsub &&
+ git -C submodule commit -m "added subsub" &&
+ git ls-files --recurse-submodules >actual &&
+ test_cmp expect actual
+'
+
+test_expect_success '--recurse-submodules does not support using path arguments' '
+ test_must_fail git ls-files --recurse-submodules b 2>actual &&
+ test_i18ngrep "does not support path arguments" actual
+'
+
+test_expect_success '--recurse-submodules does not support --error-unmatch' '
+ test_must_fail git ls-files --recurse-submodules --error-unmatch 2>actual &&
+ test_i18ngrep "does not support --error-unmatch" actual
+'
+
+test_incompatible_with_recurse_submodules () {
+ test_expect_success "--recurse-submodules and $1 are incompatible" "
+ test_must_fail git ls-files --recurse-submodules $1 2>actual &&
+ test_i18ngrep 'can only be used in --cached mode' actual
+ "
+}
+
+test_incompatible_with_recurse_submodules -v
+test_incompatible_with_recurse_submodules -t
+test_incompatible_with_recurse_submodules --deleted
+test_incompatible_with_recurse_submodules --modified
+test_incompatible_with_recurse_submodules --others
+test_incompatible_with_recurse_submodules --stage
+test_incompatible_with_recurse_submodules --killed
+test_incompatible_with_recurse_submodules --unmerged
+test_incompatible_with_recurse_submodules --eol
+
+test_done
--
2.8.0.rc3.226.g39d4020
^ permalink raw reply related
* Re: [PATCH 1/2] ls-files: adding support for submodules
From: Brandon Williams @ 2016-09-21 22:38 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <xmqq1t0c7ur2.fsf@gitster.mtv.corp.google.com>
yes you mentioned this and I meant to change that before sending it out.
Looks like it slipped through have slipped through.
^ permalink raw reply
* Re: [PATCH 4/3] http: check curl_multi_remove_handle error code
From: Jeff King @ 2016-09-21 22:31 UTC (permalink / raw)
To: Eric Wong; +Cc: Junio C Hamano, Yaroslav Halchenko, git
In-Reply-To: <20160921222959.GA4781@whir>
On Wed, Sep 21, 2016 at 10:29:59PM +0000, Eric Wong wrote:
> Jeff King <peff@peff.net> wrote:
> > On Wed, Sep 21, 2016 at 09:46:23PM +0000, Eric Wong wrote:
> >
> > > -----------8<-----------
> > > Subject: [PATCH] http: check curl_multi_remove_handle error code
> > >
> > > This should help detect bugs in future changes. While we're at
> > > it, fix a (probably innocuous) bug in our http_cleanup function
> > > for users of older curl.
> > >
> > > curl_multi_remove_handle was not idempotent until curl 7.33.0
> > > with commit 84f3b3dd448399f9548468676e1bd1475dba8de5
> > > ("curl_multi_remove_handle: allow multiple removes"),
> > > so we track the "curlm" membership of the curl easy handle
> > > ourselves with a new "in_multi" flag.
> >
> > Does curl provide a meaningful error here? I'm just wondering if we
> > could simply let curl handle this, and just ignore the error that comes
> > from older versions. We're basically just replicating curl's own state
> > data here.
>
> curl before 7.33.0 returned CURLM_BAD_EASY_HANDLE. This error
> code also happens if we pass a bad/corrupt easy handle;
> so it could be hiding an error on our end.
Hmm, yeah. So we would want to make it conditional on the version
anyway, so that we detected such bugs on more modern systems. That still
doesn't seem unreasonable to me, but your patch is also not very big, so
it's probably fine (the major risk with yours is simply that our state
and curl's state get out of sync).
-Peff
^ permalink raw reply
* Re: [PATCH 4/3] http: check curl_multi_remove_handle error code
From: Eric Wong @ 2016-09-21 22:29 UTC (permalink / raw)
To: Jeff King; +Cc: Junio C Hamano, Yaroslav Halchenko, git
In-Reply-To: <20160921222208.ccgnpgx3gcvbjpfm@sigill.intra.peff.net>
Jeff King <peff@peff.net> wrote:
> On Wed, Sep 21, 2016 at 09:46:23PM +0000, Eric Wong wrote:
>
> > -----------8<-----------
> > Subject: [PATCH] http: check curl_multi_remove_handle error code
> >
> > This should help detect bugs in future changes. While we're at
> > it, fix a (probably innocuous) bug in our http_cleanup function
> > for users of older curl.
> >
> > curl_multi_remove_handle was not idempotent until curl 7.33.0
> > with commit 84f3b3dd448399f9548468676e1bd1475dba8de5
> > ("curl_multi_remove_handle: allow multiple removes"),
> > so we track the "curlm" membership of the curl easy handle
> > ourselves with a new "in_multi" flag.
>
> Does curl provide a meaningful error here? I'm just wondering if we
> could simply let curl handle this, and just ignore the error that comes
> from older versions. We're basically just replicating curl's own state
> data here.
curl before 7.33.0 returned CURLM_BAD_EASY_HANDLE. This error
code also happens if we pass a bad/corrupt easy handle;
so it could be hiding an error on our end.
^ permalink raw reply
* Re: [PATCH 1/2] ls-files: adding support for submodules
From: Junio C Hamano @ 2016-09-21 22:28 UTC (permalink / raw)
To: Brandon Williams; +Cc: git
In-Reply-To: <CAKoko1q7Mb6_cnaA1ecZJ2y1PWUrW4RSu6RiviyN91JV5-QR5g@mail.gmail.com>
Brandon Williams <bmwill@google.com> writes:
> This is another version of the first ls-files patch i sent out in
> order. In this version I fixed the option
> name to be '--submodule-prefix'.
I understand that many internal changes in your work environment are
titled like "DOing X", but our convention around here is to label
them "DO X", as if you are giving an order to somebody else, either
telling the codebase "to be like so", or telling the patch-monkey
maintainer "to make it so". So I'd suggest retitling it to
ls-files: optionally recurse into submodules
or something like that. It is an added advantage of being a lot
more descriptive than "adding support", which does not say what kind
of support it is adding.
^ permalink raw reply
* Re: [PATCH] gitweb: use highlight's shebang detection
From: Ian Kelling @ 2016-09-21 22:24 UTC (permalink / raw)
To: Ian Kelling, git; +Cc: jnareb
In-Reply-To: <20160921221856.27830-1-ian@iankelling.org>
fyi: I mistakenly did not include v2 in the subject of the last message.
^ permalink raw reply
* Re: [PATCH 4/3] http: check curl_multi_remove_handle error code
From: Jeff King @ 2016-09-21 22:22 UTC (permalink / raw)
To: Eric Wong; +Cc: Junio C Hamano, Yaroslav Halchenko, git
In-Reply-To: <20160921214623.GA1919@whir>
On Wed, Sep 21, 2016 at 09:46:23PM +0000, Eric Wong wrote:
> -----------8<-----------
> Subject: [PATCH] http: check curl_multi_remove_handle error code
>
> This should help detect bugs in future changes. While we're at
> it, fix a (probably innocuous) bug in our http_cleanup function
> for users of older curl.
>
> curl_multi_remove_handle was not idempotent until curl 7.33.0
> with commit 84f3b3dd448399f9548468676e1bd1475dba8de5
> ("curl_multi_remove_handle: allow multiple removes"),
> so we track the "curlm" membership of the curl easy handle
> ourselves with a new "in_multi" flag.
Does curl provide a meaningful error here? I'm just wondering if we
could simply let curl handle this, and just ignore the error that comes
from older versions. We're basically just replicating curl's own state
data here.
-Peff
^ permalink raw reply
* [PATCH] gitweb: use highlight's shebang detection
From: Ian Kelling @ 2016-09-21 22:18 UTC (permalink / raw)
To: git; +Cc: jnareb
In-Reply-To: <108ce713-337a-801a-6c3b-089ef25a3883@gmail.com>
The highlight binary can detect language by shebang when we can't tell
the syntax type by the name of the file. In that case, pass the blob
to "highlight --force" and the resulting html will have markup for
highlighting if the language was detected.
Document the feature and improve syntax highlight documentation, add
test to ensure gitweb doesn't crash when language detection is used,
and remove an unused parameter from gitweb_check_feature().
Signed-off-by: Ian Kelling <ian@iankelling.org>
---
Documentation/gitweb.conf.txt | 21 ++++++++++++++-------
gitweb/gitweb.perl | 14 +++++++-------
t/t9500-gitweb-standalone-no-errors.sh | 8 ++++++++
3 files changed, 29 insertions(+), 14 deletions(-)
diff --git a/Documentation/gitweb.conf.txt b/Documentation/gitweb.conf.txt
index a79e350..e632089 100644
--- a/Documentation/gitweb.conf.txt
+++ b/Documentation/gitweb.conf.txt
@@ -246,13 +246,20 @@ $highlight_bin::
Note that 'highlight' feature must be set for gitweb to actually
use syntax highlighting.
+
-*NOTE*: if you want to add support for new file type (supported by
-"highlight" but not used by gitweb), you need to modify `%highlight_ext`
-or `%highlight_basename`, depending on whether you detect type of file
-based on extension (for example "sh") or on its basename (for example
-"Makefile"). The keys of these hashes are extension and basename,
-respectively, and value for given key is name of syntax to be passed via
-`--syntax <syntax>` to highlighter.
+*NOTE*: for a file to be highlighted, its syntax type must be detected
+and that syntax must be supported by "highlight". The default syntax
+detection is minimal, and there are many supported syntax types with no
+detection by default. There are three options for adding syntax
+detection. The first and second priority are `%highlight_basename` and
+`%highlight_ext`, which detect based on basename (the full filename, for
+example "Makefile") and extension (for example "sh"). The keys of these
+hashes are the basename and extension, respectively, and the value for a
+given key is the name of the syntax to be passed via `--syntax <syntax>`
+to "highlight". The last priority is the "highlight" configuration of
+`Shebang` regular expressions to detect the language based on the first
+line in the file, (for example, matching the line "#!/bin/bash"). See
+the highlight documentation and the default config at
+/etc/highlight/filetypes.conf for more details.
+
For example if repositories you are hosting use "phtml" extension for
PHP files, and you want to have correct syntax-highlighting for those
diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 33d701d..44094f4 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -3913,7 +3913,7 @@ sub blob_contenttype {
# guess file syntax for syntax highlighting; return undef if no highlighting
# the name of syntax can (in the future) depend on syntax highlighter used
sub guess_file_syntax {
- my ($highlight, $mimetype, $file_name) = @_;
+ my ($highlight, $file_name) = @_;
return undef unless ($highlight && defined $file_name);
my $basename = basename($file_name, '.in');
return $highlight_basename{$basename}
@@ -3931,15 +3931,16 @@ sub guess_file_syntax {
# or return original FD if no highlighting
sub run_highlighter {
my ($fd, $highlight, $syntax) = @_;
- return $fd unless ($highlight && defined $syntax);
+ return $fd unless ($highlight);
close $fd;
+ my $syntax_arg = (defined $syntax) ? "--syntax $syntax" : "--force";
open $fd, quote_command(git_cmd(), "cat-file", "blob", $hash)." | ".
quote_command($^X, '-CO', '-MEncode=decode,FB_DEFAULT', '-pse',
'$_ = decode($fe, $_, FB_DEFAULT) if !utf8::decode($_);',
'--', "-fe=$fallback_encoding")." | ".
quote_command($highlight_bin).
- " --replace-tabs=8 --fragment --syntax $syntax |"
+ " --replace-tabs=8 --fragment $syntax_arg |"
or die_error(500, "Couldn't open file or run syntax highlighter");
return $fd;
}
@@ -7062,9 +7063,8 @@ sub git_blob {
$have_blame &&= ($mimetype =~ m!^text/!);
my $highlight = gitweb_check_feature('highlight');
- my $syntax = guess_file_syntax($highlight, $mimetype, $file_name);
- $fd = run_highlighter($fd, $highlight, $syntax)
- if $syntax;
+ my $syntax = guess_file_syntax($highlight, $file_name);
+ $fd = run_highlighter($fd, $highlight, $syntax);
git_header_html(undef, $expires);
my $formats_nav = '';
@@ -7117,7 +7117,7 @@ sub git_blob {
$line = untabify($line);
printf qq!<div class="pre"><a id="l%i" href="%s#l%i" class="linenr">%4i</a> %s</div>\n!,
$nr, esc_attr(href(-replay => 1)), $nr, $nr,
- $syntax ? sanitize($line) : esc_html($line, -nbsp=>1);
+ $highlight ? sanitize($line) : esc_html($line, -nbsp=>1);
}
}
close $fd
diff --git a/t/t9500-gitweb-standalone-no-errors.sh b/t/t9500-gitweb-standalone-no-errors.sh
index e94b2f1..576db6d 100755
--- a/t/t9500-gitweb-standalone-no-errors.sh
+++ b/t/t9500-gitweb-standalone-no-errors.sh
@@ -709,6 +709,14 @@ test_expect_success HIGHLIGHT \
git commit -m "Add test.sh" &&
gitweb_run "p=.git;a=blob;f=test.sh"'
+test_expect_success HIGHLIGHT \
+ 'syntax highlighting (highlighter language autodetection)' \
+ 'git config gitweb.highlight yes &&
+ echo "#!/usr/bin/ruby" > test &&
+ git add test &&
+ git commit -m "Add test" &&
+ gitweb_run "p=.git;a=blob;f=test"'
+
# ----------------------------------------------------------------------
# forks of projects
--
2.9.3
^ permalink raw reply related
* Re: [PATCH] gitweb: use highlight's shebang detection
From: Ian Kelling @ 2016-09-21 22:15 UTC (permalink / raw)
To: Jakub Narębski, git
In-Reply-To: <108ce713-337a-801a-6c3b-089ef25a3883@gmail.com>
On Tue, Sep 20, 2016, at 01:22 PM, Jakub Narębski wrote:
> 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...
>
> [1]: http://www.andre-simon.de/doku/highlight/en/highlight.php
Search for the word shebang, it's mentioned twice.
>
> If this feature is available only for some version, or for some
> highlighters, gitweb would have to provide an option to configure
> it. It might be an additional configuration variable, it might
> be a special value in the %highlight_basename or %highlight_ext.
Good question. It was added upstream in 2007, and I tested that it's
functioning in the earliest distros I have easy access to: ubuntu 14.04
and debian wheezy.
>
> > To use highlight's shebang
> > detection, add highlight to the pipeline whenever highlight is enabled.
>
> This describes what this patch does, but the sentence feels
> a bit convoluted, as it is stated.
>
Agreed. I've changed it in v2 of the patch, and perhaps this will make
the rest of the patch clearer too. The new paragraph is:
The highlight binary can detect language by shebang when we can't
tell
the syntax type by the name of the file. In that case, pass the blob
to "highlight --force" and the resulting html will have markup for
highlighting if the language was detected.
> >
> > Document the shebang detection and add a test which exercises it in
> > t/t9500-gitweb-standalone-no-errors.sh.
>
> Nice!
>
> >
> > Signed-off-by: Ian Kelling <ian@iankelling.org>
> > ---
> >
> > Notes:
> > I wondered if adding highlight to the pipeline would make viewing a blob
> > with no highlighting take longer but it did not on my computer. I found
> > no noticeable impact on small files and strangely, on a 159k file, it
> > took 7% less time averaged over several requests.
>
> Strange. I would guess that invoking separate binary and perl would
> always
> add to the time (especially on operation systems where forking / running
> command is expensive... though those are not often used with web servers,
> isn't it).
I dug into this a little more, and I think it's because when we call
highlight, we later call sanitize() instead of esc_html(). sanitize() is
faster and makes up for the extra time highlight takes. I ran a test on
my machine calling sanitize and esc_html on each line of gitweb.perl 100
times: 7.4s for sanitize, 12.4s for esc_html.
>
> >
> > Documentation/gitweb.conf.txt | 21 ++++++++++++++-------
> > gitweb/gitweb.perl | 10 +++++-----
> > t/t9500-gitweb-standalone-no-errors.sh | 18 +++++++++++++-----
> > 3 files changed, 32 insertions(+), 17 deletions(-)
> >
> > diff --git a/Documentation/gitweb.conf.txt b/Documentation/gitweb.conf.txt
> > index a79e350..e632089 100644
> > --- a/Documentation/gitweb.conf.txt
> > +++ b/Documentation/gitweb.conf.txt
> > @@ -246,13 +246,20 @@ $highlight_bin::
> > Note that 'highlight' feature must be set for gitweb to actually
> > use syntax highlighting.
> > +
> > -*NOTE*: if you want to add support for new file type (supported by
> > -"highlight" but not used by gitweb), you need to modify `%highlight_ext`
> > -or `%highlight_basename`, depending on whether you detect type of file
> > -based on extension (for example "sh") or on its basename (for example
> > -"Makefile"). The keys of these hashes are extension and basename,
> > -respectively, and value for given key is name of syntax to be passed via
> > -`--syntax <syntax>` to highlighter.
> > +*NOTE*: for a file to be highlighted, its syntax type must be detected
> > +and that syntax must be supported by "highlight". The default syntax
> > +detection is minimal, and there are many supported syntax types with no
> > +detection by default. There are three options for adding syntax
> > +detection. The first and second priority are `%highlight_basename` and
> > +`%highlight_ext`, which detect based on basename (the full filename, for
> > +example "Makefile") and extension (for example "sh"). The keys of these
> > +hashes are the basename and extension, respectively, and the value for a
> > +given key is the name of the syntax to be passed via `--syntax <syntax>`
> > +to "highlight". The last priority is the "highlight" configuration of
> > +`Shebang` regular expressions to detect the language based on the first
> > +line in the file, (for example, matching the line "#!/bin/bash"). See
> > +the highlight documentation and the default config at
> > +/etc/highlight/filetypes.conf for more details.
>
> All right; in addition to expanding the docs, it also improves them.
Noted in v2 commit log.
>
> > +
> > For example if repositories you are hosting use "phtml" extension for
> > PHP files, and you want to have correct syntax-highlighting for those
> > diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> > index 33d701d..a672181 100755
> > --- a/gitweb/gitweb.perl
> > +++ b/gitweb/gitweb.perl
> > @@ -3931,15 +3931,16 @@ sub guess_file_syntax {
> > # or return original FD if no highlighting
> > sub run_highlighter {
> > my ($fd, $highlight, $syntax) = @_;
> > - return $fd unless ($highlight && defined $syntax);
> > + return $fd unless ($highlight);
>
> Here we would have check if we want / can invoke "highlight".
I think it's right as is. $highlight says the user wants highlighting,
and now we still want to invoke it when we do not know $syntax.
While I was double checking, I noticed there was an unused parameter to
guess_file_syntax(), $mimetype, which could easily make this not
obvious. I removed it in v2.
>
> >
> > close $fd;
> > + my $syntax_arg = (defined $syntax) ? "--syntax $syntax" : "--force";
> > open $fd, quote_command(git_cmd(), "cat-file", "blob", $hash)." | ".
> > quote_command($^X, '-CO', '-MEncode=decode,FB_DEFAULT', '-pse',
> > '$_ = decode($fe, $_, FB_DEFAULT) if !utf8::decode($_);',
> > '--', "-fe=$fallback_encoding")." | ".
> > quote_command($highlight_bin).
> > - " --replace-tabs=8 --fragment --syntax $syntax |"
> > + " --replace-tabs=8 --fragment $syntax_arg |"
> > or die_error(500, "Couldn't open file or run syntax highlighter");
> > return $fd;
> > }
>
> All right (well, except for the question asked at the beginning).
>
> > @@ -7063,8 +7064,7 @@ sub git_blob {
> >
> > my $highlight = gitweb_check_feature('highlight');
> > my $syntax = guess_file_syntax($highlight, $mimetype, $file_name);
> > - $fd = run_highlighter($fd, $highlight, $syntax)
> > - if $syntax;
>
> Hmmm... it looks like the old code checked if there was $syntax defined
> twice: once for truthy value in caller, once for definedness in
> run_highlighter().
>
> > + $fd = run_highlighter($fd, $highlight, $syntax);
>
> All right.
>
> >
> > git_header_html(undef, $expires);
> > my $formats_nav = '';
> > @@ -7117,7 +7117,7 @@ sub git_blob {
> > $line = untabify($line);
> > printf qq!<div class="pre"><a id="l%i" href="%s#l%i" class="linenr">%4i</a> %s</div>\n!,
> > $nr, esc_attr(href(-replay => 1)), $nr, $nr,
> > - $syntax ? sanitize($line) : esc_html($line, -nbsp=>1);
> > + $highlight ? sanitize($line) : esc_html($line, -nbsp=>1);
>
> Oh, well. It looks like checking if highlighter could be run in
> run_highlight() is wrong, as the caller (that is, git_blob()) needs
> to know if it is using "highlight" output (which is HTML) or raw blob
> contents (which needs to be HTML-escaped).
Per previous comment, run_highlight() is right, and we use the same
condition here to know if the highlight binary was used. If highlight
was run with --force and did not detect a language in the shebang, it
still outputs html but without adding the highlight markup.
>
> > }
> > }
> > close $fd
> > diff --git a/t/t9500-gitweb-standalone-no-errors.sh b/t/t9500-gitweb-standalone-no-errors.sh
> > index e94b2f1..9e5fcfe 100755
> > --- a/t/t9500-gitweb-standalone-no-errors.sh
> > +++ b/t/t9500-gitweb-standalone-no-errors.sh
> > @@ -702,12 +702,20 @@ test_expect_success HIGHLIGHT \
> > gitweb_run "p=.git;a=blob;f=file"'
> >
> > test_expect_success HIGHLIGHT \
> > - 'syntax highlighting (highlighted, shell script)' \
> > + 'syntax highlighting (highlighted, shell script shebang)' \
>
> It would be nice to have in test name that it checks if highlighter
> autodetection works, or at least doesn't crash gitweb.
I've updated it to:
syntax highlighting (highlighter language autodetection)
I'm happy to use any suggestion you have.
>
> > 'git config gitweb.highlight yes &&
> > - echo "#!/usr/bin/sh" > test.sh &&
> > - git add test.sh &&
> > - git commit -m "Add test.sh" &&
> > - gitweb_run "p=.git;a=blob;f=test.sh"'
> > + echo "#!/usr/bin/sh" > test &&
> > + git add test &&
> > + git commit -m "Add test" &&
> > + gitweb_run "p=.git;a=blob;f=test"'
> > +
> > +test_expect_success HIGHLIGHT \
> > + 'syntax highlighting (highlighted, header file)' \
>
> Do we check explicit syntax knowledge (based on the extension),
> or autodetect again?
We have explicit syntax knowledge here. My thinking was this would
modify the existing test so that it highlights a different language than
the autodetected one, but the patch is simpler if I just make the
autodetcted one be a different language. I've done that in v2.
>
> > + 'git config gitweb.highlight yes &&
> > + echo "#define ANSWER 42" > test.h &&
> > + git add test.h &&
> > + git commit -m "Add test.h" &&
> > + gitweb_run "p=.git;a=blob;f=test.h"'
> >
> > # ----------------------------------------------------------------------
> > # forks of projects
> >
>
> Thank you for your work on this patch,
> --
> Jakub Narębski
Thank you for reviewing it!
^ permalink raw reply
* Re: [PATCH 1/2] ls-files: adding support for submodules
From: Brandon Williams @ 2016-09-21 22:08 UTC (permalink / raw)
To: git, Junio C Hamano; +Cc: Brandon Williams
In-Reply-To: <1474495472-94190-1-git-send-email-bmwill@google.com>
This is another version of the first ls-files patch i sent out in
order. In this version I fixed the option
name to be '--submodule-prefix'.
^ permalink raw reply
* [PATCH 2/2] ls-files: add pathspec matching for submodules
From: Brandon Williams @ 2016-09-21 22:04 UTC (permalink / raw)
To: git; +Cc: Brandon Williams
In-Reply-To: <1474495472-94190-1-git-send-email-bmwill@google.com>
Pathspecs can be a bit tricky when trying to apply them to submodules.
The main challenge is that the pathspecs will be with respect to the
super module and not with respect to paths in the submodule. The
approach this patch takes is to pass in the identical pathspec from the
super module to the submodule in addition to the submodule-prefix, which
is the path from the root of the super module to the submodule, and then
we can compare an entry in the submodule prepended with the
submodule-prefix to the pathspec in order to determine if there is a
match.
This patch also permits the pathspec logic to perform a prefix match against
submodules since a pathspec could refer to a file inside of a submodule.
Due to limitations in the wildmatch logic, a prefix match is only done
literally. If any wildcard character is encountered we'll simply punt
and produce a false positive match. More accurate matching will be done
once inside the submodule. This is due to the super module not knowing
what files could exist in the submodule.
Signed-off-by: Brandon Williams <bmwill@google.com>
---
builtin/ls-files.c | 132 ++++++++++++++++++++-------------
dir.c | 43 ++++++++++-
dir.h | 4 +
t/t3007-ls-files-recurse-submodules.sh | 114 ++++++++++++++++++++++++++--
4 files changed, 231 insertions(+), 62 deletions(-)
diff --git a/builtin/ls-files.c b/builtin/ls-files.c
index ffd9ea6..fa4029e 100644
--- a/builtin/ls-files.c
+++ b/builtin/ls-files.c
@@ -177,12 +177,34 @@ static void show_gitlink(const struct cache_entry *ce)
{
struct child_process cp = CHILD_PROCESS_INIT;
int status;
+ int i;
argv_array_push(&cp.args, "ls-files");
argv_array_push(&cp.args, "--recurse-submodules");
argv_array_pushf(&cp.args, "--submodule-prefix=%s%s/",
submodule_prefix ? submodule_prefix : "",
ce->name);
+ /* add options */
+ if (show_eol)
+ argv_array_push(&cp.args, "--eol");
+ if (show_valid_bit)
+ argv_array_push(&cp.args, "-v");
+ if (show_stage)
+ argv_array_push(&cp.args, "--stage");
+ if (show_cached)
+ argv_array_push(&cp.args, "--cached");
+ if (debug_mode)
+ argv_array_push(&cp.args, "--debug");
+
+ /*
+ * Pass in the original pathspec args. The submodule will be
+ * responsible for prepending the 'submodule_prefix' prior to comparing
+ * against the pathspec for matches.
+ */
+ argv_array_push(&cp.args, "--");
+ for (i = 0; i < pathspec.nr; i++)
+ argv_array_push(&cp.args, pathspec.items[i].original);
+
cp.git_cmd = 1;
cp.dir = ce->name;
status = run_command(&cp);
@@ -192,57 +214,62 @@ static void show_gitlink(const struct cache_entry *ce)
static void show_ce_entry(const char *tag, const struct cache_entry *ce)
{
+ struct strbuf name = STRBUF_INIT;
int len = max_prefix_len;
+ if (submodule_prefix)
+ strbuf_addstr(&name, submodule_prefix);
+ strbuf_addstr(&name, ce->name);
if (len >= ce_namelen(ce))
die("git ls-files: internal error - cache entry not superset of prefix");
- if (!match_pathspec(&pathspec, ce->name, ce_namelen(ce),
- len, ps_matched,
- S_ISDIR(ce->ce_mode) || S_ISGITLINK(ce->ce_mode)))
- return;
- if (recurse_submodules && S_ISGITLINK(ce->ce_mode)) {
+ if (recurse_submodules && S_ISGITLINK(ce->ce_mode) &&
+ submodule_path_match(&pathspec, name.buf, ps_matched)) {
show_gitlink(ce);
- return;
- }
+ } else if (match_pathspec(&pathspec, name.buf, name.len,
+ len, ps_matched,
+ S_ISDIR(ce->ce_mode) ||
+ S_ISGITLINK(ce->ce_mode))) {
+ if (tag && *tag && show_valid_bit &&
+ (ce->ce_flags & CE_VALID)) {
+ static char alttag[4];
+ memcpy(alttag, tag, 3);
+ if (isalpha(tag[0]))
+ alttag[0] = tolower(tag[0]);
+ else if (tag[0] == '?')
+ alttag[0] = '!';
+ else {
+ alttag[0] = 'v';
+ alttag[1] = tag[0];
+ alttag[2] = ' ';
+ alttag[3] = 0;
+ }
+ tag = alttag;
+ }
- if (tag && *tag && show_valid_bit &&
- (ce->ce_flags & CE_VALID)) {
- static char alttag[4];
- memcpy(alttag, tag, 3);
- if (isalpha(tag[0]))
- alttag[0] = tolower(tag[0]);
- else if (tag[0] == '?')
- alttag[0] = '!';
- else {
- alttag[0] = 'v';
- alttag[1] = tag[0];
- alttag[2] = ' ';
- alttag[3] = 0;
+ if (!show_stage) {
+ fputs(tag, stdout);
+ } else {
+ printf("%s%06o %s %d\t",
+ tag,
+ ce->ce_mode,
+ find_unique_abbrev(ce->sha1,abbrev),
+ ce_stage(ce));
+ }
+ write_eolinfo(ce, ce->name);
+ write_name(ce->name);
+ if (debug_mode) {
+ const struct stat_data *sd = &ce->ce_stat_data;
+
+ printf(" ctime: %d:%d\n", sd->sd_ctime.sec, sd->sd_ctime.nsec);
+ printf(" mtime: %d:%d\n", sd->sd_mtime.sec, sd->sd_mtime.nsec);
+ printf(" dev: %d\tino: %d\n", sd->sd_dev, sd->sd_ino);
+ printf(" uid: %d\tgid: %d\n", sd->sd_uid, sd->sd_gid);
+ printf(" size: %d\tflags: %x\n", sd->sd_size, ce->ce_flags);
}
- tag = alttag;
}
- if (!show_stage) {
- fputs(tag, stdout);
- } else {
- printf("%s%06o %s %d\t",
- tag,
- ce->ce_mode,
- find_unique_abbrev(ce->sha1,abbrev),
- ce_stage(ce));
- }
- write_eolinfo(ce, ce->name);
- write_name(ce->name);
- if (debug_mode) {
- const struct stat_data *sd = &ce->ce_stat_data;
-
- printf(" ctime: %d:%d\n", sd->sd_ctime.sec, sd->sd_ctime.nsec);
- printf(" mtime: %d:%d\n", sd->sd_mtime.sec, sd->sd_mtime.nsec);
- printf(" dev: %d\tino: %d\n", sd->sd_dev, sd->sd_ino);
- printf(" uid: %d\tgid: %d\n", sd->sd_uid, sd->sd_gid);
- printf(" size: %d\tflags: %x\n", sd->sd_size, ce->ce_flags);
- }
+ strbuf_release(&name);
}
static void show_ru_info(void)
@@ -566,27 +593,28 @@ int cmd_ls_files(int argc, const char **argv, const char *cmd_prefix)
setup_work_tree();
if (recurse_submodules &&
- (show_stage || show_deleted || show_others || show_unmerged ||
- show_killed || show_modified || show_resolve_undo ||
- show_valid_bit || show_tag || show_eol))
- die("ls-files --recurse-submodules can only be used in "
- "--cached mode");
+ (show_deleted || show_others || show_unmerged ||
+ show_killed || show_modified || show_resolve_undo))
+ die("ls-files --recurse-submodules unsupported mode");
if (recurse_submodules && error_unmatch)
die("ls-files --recurse-submodules does not support "
"--error-unmatch");
- if (recurse_submodules && argc)
- die("ls-files --recurse-submodules does not support path "
- "arguments");
-
parse_pathspec(&pathspec, 0,
PATHSPEC_PREFER_CWD |
PATHSPEC_STRIP_SUBMODULE_SLASH_CHEAP,
prefix, argv);
- /* Find common prefix for all pathspec's */
- max_prefix = common_prefix(&pathspec);
+ /*
+ * Find common prefix for all pathspec's
+ * This is used as a performance optimization which unfortunately cannot
+ * be done when recursing into submodules
+ */
+ if (recurse_submodules)
+ max_prefix = NULL;
+ else
+ max_prefix = common_prefix(&pathspec);
max_prefix_len = max_prefix ? strlen(max_prefix) : 0;
/* Treat unmatching pathspec elements as errors */
diff --git a/dir.c b/dir.c
index 0ea235f..9df6d36 100644
--- a/dir.c
+++ b/dir.c
@@ -207,8 +207,9 @@ int within_depth(const char *name, int namelen,
return 1;
}
-#define DO_MATCH_EXCLUDE 1
-#define DO_MATCH_DIRECTORY 2
+#define DO_MATCH_EXCLUDE (1<<0)
+#define DO_MATCH_DIRECTORY (1<<1)
+#define DO_MATCH_SUBMODULE (1<<2)
/*
* Does 'match' match the given name?
@@ -283,6 +284,29 @@ static int match_pathspec_item(const struct pathspec_item *item, int prefix,
item->nowildcard_len - prefix))
return MATCHED_FNMATCH;
+ /* Perform checks to see if "name" is a super set of the pathspec */
+ if (flags & DO_MATCH_SUBMODULE) {
+ /* Check if the name is a literal prefix of the pathspec */
+ if ((item->match[namelen] == '/') &&
+ !ps_strncmp(item, match, name, namelen))
+ return MATCHED_RECURSIVELY;
+
+ /*
+ * Here is where we would perform a wildmatch to check if
+ * "name" can be matched as a directory (or a prefix) against
+ * the pathspec. Since wildmatch doesn't have this capability
+ * at the present we have to punt and say that it is a match,
+ * esentially returning a false positive (as long as "name"
+ * matches upto the first wild character).
+ * The submodules themselves will be able to perform more
+ * accurate matching to determine if the pathspec matches.
+ */
+ if (item->nowildcard_len < item->len &&
+ !ps_strncmp(item, match, name,
+ item->nowildcard_len - prefix))
+ return MATCHED_RECURSIVELY;
+ }
+
return 0;
}
@@ -386,6 +410,21 @@ int match_pathspec(const struct pathspec *ps,
return negative ? 0 : positive;
}
+/**
+ * Check if a submodule is a superset of the pathspec
+ */
+int submodule_path_match(const struct pathspec *ps,
+ const char *submodule_name,
+ char *seen)
+{
+ int matched = do_match_pathspec(ps, submodule_name,
+ strlen(submodule_name),
+ 0, seen,
+ DO_MATCH_DIRECTORY |
+ DO_MATCH_SUBMODULE);
+ return matched;
+}
+
int report_path_error(const char *ps_matched,
const struct pathspec *pathspec,
const char *prefix)
diff --git a/dir.h b/dir.h
index da1a858..97c83bb 100644
--- a/dir.h
+++ b/dir.h
@@ -304,6 +304,10 @@ extern int git_fnmatch(const struct pathspec_item *item,
const char *pattern, const char *string,
int prefix);
+extern int submodule_path_match(const struct pathspec *ps,
+ const char *submodule_name,
+ char *seen);
+
static inline int ce_path_match(const struct cache_entry *ce,
const struct pathspec *pathspec,
char *seen)
diff --git a/t/t3007-ls-files-recurse-submodules.sh b/t/t3007-ls-files-recurse-submodules.sh
index caf3815..ca79fda 100755
--- a/t/t3007-ls-files-recurse-submodules.sh
+++ b/t/t3007-ls-files-recurse-submodules.sh
@@ -69,9 +69,111 @@ test_expect_success 'ls-files recurses more than 1 level' '
test_cmp expect actual
'
-test_expect_success '--recurse-submodules does not support using path arguments' '
- test_must_fail git ls-files --recurse-submodules b 2>actual &&
- test_i18ngrep "does not support path arguments" actual
+test_expect_success '--recurse-submodules and pathspecs setup' '
+ echo e >submodule/subsub/e.txt &&
+ git -C submodule/subsub add e.txt &&
+ git -C submodule/subsub commit -m "adding e.txt" &&
+ echo f >submodule/f.TXT &&
+ echo g >submodule/g.txt &&
+ git -C submodule add f.TXT g.txt &&
+ git -C submodule commit -m "add f and g" &&
+ echo h >h.txt &&
+ mkdir sib &&
+ echo sib >sib/file &&
+ git add h.txt sib/file &&
+ git commit -m "add h and sib/file" &&
+ git init sub &&
+ echo sub >sub/file &&
+ git -C sub add file &&
+ git -C sub commit -m "add file" &&
+ git submodule add ./sub &&
+ git commit -m "added sub" &&
+
+ cat >expect <<-\EOF &&
+ .gitmodules
+ a
+ b/b
+ h.txt
+ sib/file
+ sub/file
+ submodule/.gitmodules
+ submodule/c
+ submodule/f.TXT
+ submodule/g.txt
+ submodule/subsub/d
+ submodule/subsub/e.txt
+ EOF
+
+ git ls-files --recurse-submodules >actual &&
+ test_cmp expect actual &&
+ cat actual &&
+ git ls-files --recurse-submodules "*" >actual &&
+ test_cmp expect actual
+'
+
+test_expect_success '--recurse-submodules and pathspecs' '
+ cat >expect <<-\EOF &&
+ h.txt
+ submodule/g.txt
+ submodule/subsub/e.txt
+ EOF
+
+ git ls-files --recurse-submodules "*.txt" >actual &&
+ test_cmp expect actual
+'
+
+test_expect_success '--recurse-submodules and pathspecs' '
+ cat >expect <<-\EOF &&
+ h.txt
+ submodule/f.TXT
+ submodule/g.txt
+ submodule/subsub/e.txt
+ EOF
+
+ git ls-files --recurse-submodules ":(icase)*.txt" >actual &&
+ test_cmp expect actual
+'
+
+test_expect_success '--recurse-submodules and pathspecs' '
+ cat >expect <<-\EOF &&
+ h.txt
+ submodule/f.TXT
+ submodule/g.txt
+ EOF
+
+ git ls-files --recurse-submodules ":(icase)*.txt" ":(exclude)submodule/subsub/*" >actual &&
+ test_cmp expect actual
+'
+
+test_expect_success '--recurse-submodules and pathspecs' '
+ cat >expect <<-\EOF &&
+ sub/file
+ EOF
+
+ git ls-files --recurse-submodules "sub" >actual &&
+ test_cmp expect actual &&
+ git ls-files --recurse-submodules "sub/" >actual &&
+ test_cmp expect actual &&
+ git ls-files --recurse-submodules "sub/file" >actual &&
+ test_cmp expect actual &&
+ git ls-files --recurse-submodules "su*/file" >actual &&
+ test_cmp expect actual &&
+ git ls-files --recurse-submodules "su?/file" >actual &&
+ test_cmp expect actual
+'
+
+test_expect_success '--recurse-submodules and pathspecs' '
+ cat >expect <<-\EOF &&
+ sib/file
+ sub/file
+ EOF
+
+ git ls-files --recurse-submodules "s??/file" >actual &&
+ test_cmp expect actual &&
+ git ls-files --recurse-submodules "s???file" >actual &&
+ test_cmp expect actual &&
+ git ls-files --recurse-submodules "s*file" >actual &&
+ test_cmp expect actual
'
test_expect_success '--recurse-submodules does not support --error-unmatch' '
@@ -82,18 +184,14 @@ test_expect_success '--recurse-submodules does not support --error-unmatch' '
test_incompatible_with_recurse_submodules () {
test_expect_success "--recurse-submodules and $1 are incompatible" "
test_must_fail git ls-files --recurse-submodules $1 2>actual &&
- test_i18ngrep 'can only be used in --cached mode' actual
+ test_i18ngrep 'unsupported mode' actual
"
}
-test_incompatible_with_recurse_submodules -v
-test_incompatible_with_recurse_submodules -t
test_incompatible_with_recurse_submodules --deleted
test_incompatible_with_recurse_submodules --modified
test_incompatible_with_recurse_submodules --others
-test_incompatible_with_recurse_submodules --stage
test_incompatible_with_recurse_submodules --killed
test_incompatible_with_recurse_submodules --unmerged
-test_incompatible_with_recurse_submodules --eol
test_done
--
2.8.0.rc3.226.g39d4020
^ permalink raw reply related
* [PATCH 1/2] ls-files: adding support for submodules
From: Brandon Williams @ 2016-09-21 22:04 UTC (permalink / raw)
To: git; +Cc: Brandon Williams
Allow ls-files to recognize submodules in order to retrieve a list of
files from a repository's submodules. This is done by forking off a
process to recursively call ls-files on all submodules. Also added a
submodule-prefix command in order to prepend paths to child processes.
Signed-off-by: Brandon Williams <bmwill@google.com>
---
Documentation/git-ls-files.txt | 11 +++-
builtin/ls-files.c | 61 +++++++++++++++++++++
t/t3007-ls-files-recurse-submodules.sh | 99 ++++++++++++++++++++++++++++++++++
3 files changed, 170 insertions(+), 1 deletion(-)
create mode 100755 t/t3007-ls-files-recurse-submodules.sh
diff --git a/Documentation/git-ls-files.txt b/Documentation/git-ls-files.txt
index 0d933ac..09e4449 100644
--- a/Documentation/git-ls-files.txt
+++ b/Documentation/git-ls-files.txt
@@ -18,7 +18,9 @@ SYNOPSIS
[--exclude-per-directory=<file>]
[--exclude-standard]
[--error-unmatch] [--with-tree=<tree-ish>]
- [--full-name] [--abbrev] [--] [<file>...]
+ [--full-name] [--recurse-submodules]
+ [--submodule-prefix=<path>]
+ [--abbrev] [--] [<file>...]
DESCRIPTION
-----------
@@ -137,6 +139,13 @@ a space) at the start of each line:
option forces paths to be output relative to the project
top directory.
+--recurse-submodules::
+ Recursively calls ls-files on each submodule in the repository.
+ Currently there is only support for the --cached mode.
+
+--submodule-prefix=<path>::
+ Prepend the provided path to the output of each file
+
--abbrev[=<n>]::
Instead of showing the full 40-byte hexadecimal object
lines, show only a partial prefix.
diff --git a/builtin/ls-files.c b/builtin/ls-files.c
index 00ea91a..ffd9ea6 100644
--- a/builtin/ls-files.c
+++ b/builtin/ls-files.c
@@ -14,6 +14,7 @@
#include "resolve-undo.h"
#include "string-list.h"
#include "pathspec.h"
+#include "run-command.h"
static int abbrev;
static int show_deleted;
@@ -28,6 +29,8 @@ static int show_valid_bit;
static int line_terminator = '\n';
static int debug_mode;
static int show_eol;
+static const char *submodule_prefix;
+static int recurse_submodules;
static const char *prefix;
static int max_prefix_len;
@@ -68,6 +71,21 @@ static void write_eolinfo(const struct cache_entry *ce, const char *path)
static void write_name(const char *name)
{
/*
+ * NEEDSWORK: To make this thread-safe, full_name would have to be owned
+ * by the caller.
+ *
+ * full_name get reused across output lines to minimize the allocation
+ * churn.
+ */
+ static struct strbuf full_name = STRBUF_INIT;
+ if (submodule_prefix && *submodule_prefix) {
+ strbuf_reset(&full_name);
+ strbuf_addstr(&full_name, submodule_prefix);
+ strbuf_addstr(&full_name, name);
+ name = full_name.buf;
+ }
+
+ /*
* With "--full-name", prefix_len=0; this caller needs to pass
* an empty string in that case (a NULL is good for "").
*/
@@ -152,6 +170,26 @@ static void show_killed_files(struct dir_struct *dir)
}
}
+/**
+ * Recursively call ls-files on a submodule
+ */
+static void show_gitlink(const struct cache_entry *ce)
+{
+ struct child_process cp = CHILD_PROCESS_INIT;
+ int status;
+
+ argv_array_push(&cp.args, "ls-files");
+ argv_array_push(&cp.args, "--recurse-submodules");
+ argv_array_pushf(&cp.args, "--submodule-prefix=%s%s/",
+ submodule_prefix ? submodule_prefix : "",
+ ce->name);
+ cp.git_cmd = 1;
+ cp.dir = ce->name;
+ status = run_command(&cp);
+ if (status)
+ exit(status);
+}
+
static void show_ce_entry(const char *tag, const struct cache_entry *ce)
{
int len = max_prefix_len;
@@ -163,6 +201,10 @@ static void show_ce_entry(const char *tag, const struct cache_entry *ce)
len, ps_matched,
S_ISDIR(ce->ce_mode) || S_ISGITLINK(ce->ce_mode)))
return;
+ if (recurse_submodules && S_ISGITLINK(ce->ce_mode)) {
+ show_gitlink(ce);
+ return;
+ }
if (tag && *tag && show_valid_bit &&
(ce->ce_flags & CE_VALID)) {
@@ -468,6 +510,10 @@ int cmd_ls_files(int argc, const char **argv, const char *cmd_prefix)
{ OPTION_SET_INT, 0, "full-name", &prefix_len, NULL,
N_("make the output relative to the project top directory"),
PARSE_OPT_NOARG | PARSE_OPT_NONEG, NULL },
+ OPT_STRING(0, "submodule-prefix", &submodule_prefix,
+ N_("path"), N_("prepend <path> to each file")),
+ OPT_BOOL(0, "recurse-submodules", &recurse_submodules,
+ N_("recurse through submodules")),
OPT_BOOL(0, "error-unmatch", &error_unmatch,
N_("if any <file> is not in the index, treat this as an error")),
OPT_STRING(0, "with-tree", &with_tree, N_("tree-ish"),
@@ -519,6 +565,21 @@ int cmd_ls_files(int argc, const char **argv, const char *cmd_prefix)
if (require_work_tree && !is_inside_work_tree())
setup_work_tree();
+ if (recurse_submodules &&
+ (show_stage || show_deleted || show_others || show_unmerged ||
+ show_killed || show_modified || show_resolve_undo ||
+ show_valid_bit || show_tag || show_eol))
+ die("ls-files --recurse-submodules can only be used in "
+ "--cached mode");
+
+ if (recurse_submodules && error_unmatch)
+ die("ls-files --recurse-submodules does not support "
+ "--error-unmatch");
+
+ if (recurse_submodules && argc)
+ die("ls-files --recurse-submodules does not support path "
+ "arguments");
+
parse_pathspec(&pathspec, 0,
PATHSPEC_PREFER_CWD |
PATHSPEC_STRIP_SUBMODULE_SLASH_CHEAP,
diff --git a/t/t3007-ls-files-recurse-submodules.sh b/t/t3007-ls-files-recurse-submodules.sh
new file mode 100755
index 0000000..caf3815
--- /dev/null
+++ b/t/t3007-ls-files-recurse-submodules.sh
@@ -0,0 +1,99 @@
+#!/bin/sh
+
+test_description='Test ls-files recurse-submodules feature
+
+This test verifies the recurse-submodules feature correctly lists files from
+submodules.
+'
+
+. ./test-lib.sh
+
+test_expect_success 'setup directory structure and submodules' '
+ echo a >a &&
+ mkdir b &&
+ echo b >b/b &&
+ git add a b &&
+ git commit -m "add a and b" &&
+ git init submodule &&
+ echo c >submodule/c &&
+ git -C submodule add c &&
+ git -C submodule commit -m "add c" &&
+ git submodule add ./submodule &&
+ git commit -m "added submodule"
+'
+
+test_expect_success 'ls-files correctly outputs files in submodule' '
+ cat >expect <<-\EOF &&
+ .gitmodules
+ a
+ b/b
+ submodule/c
+ EOF
+
+ git ls-files --recurse-submodules >actual &&
+ test_cmp expect actual
+'
+
+test_expect_success 'ls-files does not output files not added to a repo' '
+ cat >expect <<-\EOF &&
+ .gitmodules
+ a
+ b/b
+ submodule/c
+ EOF
+
+ echo a >not_added &&
+ echo b >b/not_added &&
+ echo c >submodule/not_added &&
+ git ls-files --recurse-submodules >actual &&
+ test_cmp expect actual
+'
+
+test_expect_success 'ls-files recurses more than 1 level' '
+ cat >expect <<-\EOF &&
+ .gitmodules
+ a
+ b/b
+ submodule/.gitmodules
+ submodule/c
+ submodule/subsub/d
+ EOF
+
+ git init submodule/subsub &&
+ echo d >submodule/subsub/d &&
+ git -C submodule/subsub add d &&
+ git -C submodule/subsub commit -m "add d" &&
+ git -C submodule submodule add ./subsub &&
+ git -C submodule commit -m "added subsub" &&
+ git ls-files --recurse-submodules >actual &&
+ test_cmp expect actual
+'
+
+test_expect_success '--recurse-submodules does not support using path arguments' '
+ test_must_fail git ls-files --recurse-submodules b 2>actual &&
+ test_i18ngrep "does not support path arguments" actual
+'
+
+test_expect_success '--recurse-submodules does not support --error-unmatch' '
+ test_must_fail git ls-files --recurse-submodules --error-unmatch 2>actual &&
+ test_i18ngrep "does not support --error-unmatch" actual
+'
+
+test_incompatible_with_recurse_submodules () {
+ test_expect_success "--recurse-submodules and $1 are incompatible" "
+ test_must_fail git ls-files --recurse-submodules $1 2>actual &&
+ test_i18ngrep 'can only be used in --cached mode' actual
+ "
+}
+
+test_incompatible_with_recurse_submodules -v
+test_incompatible_with_recurse_submodules -t
+test_incompatible_with_recurse_submodules --deleted
+test_incompatible_with_recurse_submodules --modified
+test_incompatible_with_recurse_submodules --others
+test_incompatible_with_recurse_submodules --stage
+test_incompatible_with_recurse_submodules --killed
+test_incompatible_with_recurse_submodules --unmerged
+test_incompatible_with_recurse_submodules --eol
+
+test_done
--
2.8.0.rc3.226.g39d4020
^ permalink raw reply related
* Re: [PATCH v4 0/3] Fix a segfault caused by regexec() being called on mmap()ed data
From: Jeff King @ 2016-09-21 22:04 UTC (permalink / raw)
To: Johannes Schindelin
Cc: git, Junio C Hamano, Benjamin Kramer, René Scharfe
In-Reply-To: <cover.1474482164.git.johannes.schindelin@gmx.de>
On Wed, Sep 21, 2016 at 08:23:11PM +0200, Johannes Schindelin wrote:
> 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.
I did a double-take on this, but then read:
> Changes since v3:
> [...]
> - removed fallback when REG_STARTEND is not supported, in favor of
> requiring NO_REGEX.
So I think we are all in agreement. :)
With the exception of a few commit message fixups that Junio already
pointed out, this looks good to me. Thanks.
-Peff
^ permalink raw reply
* Re: [PATCH v4 3/3] regex: use regexec_buf()
From: Jeff King @ 2016-09-21 22:03 UTC (permalink / raw)
To: Johannes Schindelin
Cc: git, Junio C Hamano, Benjamin Kramer, René Scharfe
In-Reply-To: <53f3609d99c865d59d7bfd8219a5334339e9e6bc.1474482164.git.johannes.schindelin@gmx.de>
On Wed, Sep 21, 2016 at 08:24:14PM +0200, Johannes Schindelin wrote:
> 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).
Nice. I probably would have split these into their own patch, but I
think it is OK here.
> @@ -228,18 +227,16 @@ static long ff_regexp(const char *line, long len,
> len--;
> }
>
> - line_buffer = xstrndup(line, len); /* make NUL terminated */
> -
Nice to see this one going away in particular, since it's called quite a
lot. According to perf, "git log -p" on git.git drops about 1.5 million
malloc calls (about 9% of the total). And here are best-of-five results
for that same command:
[before]
real 0m14.676s
user 0m13.988s
sys 0m0.676s
[after]
real 0m14.394s
user 0m13.624s
sys 0m0.760s
Not a _huge_ improvement, but more significant than the run-to-run
noise.
-Peff
^ permalink raw reply
* [PATCH 4/3] http: check curl_multi_remove_handle error code
From: Eric Wong @ 2016-09-21 21:46 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Yaroslav Halchenko, git, Jeff King
In-Reply-To: <xmqqr38nv8ul.fsf@gitster.mtv.corp.google.com>
Junio C Hamano <gitster@pobox.com> wrote:
> Eric Wong <e@80x24.org> writes:
> > The key patch here is 3/3 which seems like an obvious fix to
> > adding the problem of adding a curl easy handle to a curl multi
> > handle repeatedly.
>
> Yeah, sounds like the right thing to do and 2/3 makes it really easy
> to read the resulting code.
>
> > I will investigate those failures in a week or two when I regain
> > regular computer access.
>
> Thanks. Will tentatively queue on 'pu' and wait for updates.
I'm comfortable with the original 3 patch series in 'next'
and being merged to 'master' and 'maint', soon.
I don't think the following 4/3 is strictly necessary now, so
I'd be more comfortable with it being tested in 'pu' or 'next'
for a longer period.
(online today, but not much tomorrow or another few days after)
-----------8<-----------
Subject: [PATCH] http: check curl_multi_remove_handle error code
This should help detect bugs in future changes. While we're at
it, fix a (probably innocuous) bug in our http_cleanup function
for users of older curl.
curl_multi_remove_handle was not idempotent until curl 7.33.0
with commit 84f3b3dd448399f9548468676e1bd1475dba8de5
("curl_multi_remove_handle: allow multiple removes"),
so we track the "curlm" membership of the curl easy handle
ourselves with a new "in_multi" flag.
Tested with curl 7.26.0 and 7.38.0 on Debian 7.x (wheezy) and
Debian 8.x (jessie) respectively.
Signed-off-by: Eric Wong <e@80x24.org>
---
http.c | 12 ++++++++++--
http.h | 1 +
2 files changed, 11 insertions(+), 2 deletions(-)
diff --git a/http.c b/http.c
index 82ed542..9f97749 100644
--- a/http.c
+++ b/http.c
@@ -204,7 +204,12 @@ static void finish_active_slot(struct active_request_slot *slot)
static void xmulti_remove_handle(struct active_request_slot *slot)
{
#ifdef USE_CURL_MULTI
- curl_multi_remove_handle(curlm, slot->curl);
+ CURLMcode code = curl_multi_remove_handle(curlm, slot->curl);
+
+ if (code != CURLM_OK)
+ die("curl_multi_remove_handle failed (%p): %s",
+ slot->curl, curl_multi_strerror(code));
+ slot->in_multi = 0;
#endif
}
@@ -888,7 +893,8 @@ void http_cleanup(void)
while (slot != NULL) {
struct active_request_slot *next = slot->next;
if (slot->curl != NULL) {
- xmulti_remove_handle(slot);
+ if (slot->in_multi)
+ xmulti_remove_handle(slot);
curl_easy_cleanup(slot->curl);
}
free(slot);
@@ -965,6 +971,7 @@ struct active_request_slot *get_active_slot(void)
newslot = xmalloc(sizeof(*newslot));
newslot->curl = NULL;
newslot->in_use = 0;
+ newslot->in_multi = 0;
newslot->next = NULL;
slot = active_queue_head;
@@ -1033,6 +1040,7 @@ int start_active_slot(struct active_request_slot *slot)
slot->in_use = 0;
return 0;
}
+ slot->in_multi = 1;
/*
* We know there must be something to do, since we just added
diff --git a/http.h b/http.h
index 5ab9d9c..3339d70 100644
--- a/http.h
+++ b/http.h
@@ -60,6 +60,7 @@ struct slot_results {
struct active_request_slot {
CURL *curl;
int in_use;
+ int in_multi;
CURLcode curl_result;
long http_code;
int *finished;
--
EW
^ permalink raw reply related
* 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 21:12 UTC (permalink / raw)
To: Johannes Sixt; +Cc: Thomas Gummerer, Git Mailing List
In-Reply-To: <c3aefd9d-b794-21a1-619e-bce6a3c2cf47@kdbg.org>
Johannes Sixt <j6t@kdbg.org> writes:
> But I came to a different conclusion as I said in a message that
> crossed yours. I hope Thomas can pick up the baton again.
Yeah, our mails crossed, apparently, and I do agree with your
reasoning. How about this, then?
-- >8 --
From: Johannes Sixt <j6t@kdbg.org>
Date: Tue, 20 Sep 2016 08:18:25 +0200
Subject: [PATCH] t3700-add: do not check working tree file mode without POSIXPERM
A recently introduced test checks the result of 'git status' after
setting the executable bit on a file. This check does not yield the
expected result when the filesystem does not support the executable
bit.
What we care about is that a file added with "--chmod=+x" has
executable bit in the index and that "--chmod=+x" (or any other
options for that matter) does not muck with working tree files.
The former is tested by other existing tests, so let's check the
latter more explicitly and only under POSIXPERM prerequisite.
Signed-off-by: Johannes Sixt <j6t@kdbg.org>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
t/t3700-add.sh | 6 ++----
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/t/t3700-add.sh b/t/t3700-add.sh
index 16ab2da..924a266 100755
--- a/t/t3700-add.sh
+++ b/t/t3700-add.sh
@@ -361,13 +361,11 @@ 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' '
- echo "AM foo4" >expected &&
+test_expect_success POSIXPERM 'git add --chmod=[+-]x does not change the working tree' '
echo foo >foo4 &&
git add foo4 &&
git add --chmod=+x foo4 &&
- git status -s foo4 >actual &&
- test_cmp expected actual
+ ! test -x foo4
'
test_expect_success 'no file status change if no pathspec is given' '
--
2.10.0-515-g9036219
^ permalink raw reply related
* RE: [PATCH v3] checkout: eliminate unnecessary merge for trivial checkout
From: Ben Peart @ 2016-09-21 18:32 UTC (permalink / raw)
To: Junio C Hamano, Ben Peart; +Cc: pclouds@gmail.com, git@vger.kernel.org
In-Reply-To: <xmqqr38fg6tk.fsf@gitster.mtv.corp.google.com>
I understand the reluctance to change the existing behavior of the "git checkout -b <new-name>" command.
I see this as a tradeoff between taking advantage of the muscle memory for the existing command and coming up with a new shortcut command and training people to use it instead.
The fact that all the use cases we've observed and all the git test cases actually produce the same results but significantly faster with that change in behavior made me hope we could redefine the command to take advantage of the muscle memory.
That said, you're much more on the frontline of receiving negative feedback about doing that than I am. :) How would you like to proceed?
Ben
-----Original Message-----
From: Junio C Hamano [mailto:gitster@pobox.com]
Sent: Monday, September 19, 2016 1:04 PM
To: Ben Peart <peartben@gmail.com>
Cc: pclouds@gmail.com; Ben Peart <Ben.Peart@microsoft.com>; git@vger.kernel.org
Subject: Re: [PATCH v3] checkout: eliminate unnecessary merge for trivial checkout
Junio C Hamano <gitster@pobox.com> writes:
>> "git checkout -b foo" (without -f -m or <start_point>) is defined in
>> the manual as being a shortcut for/equivalent to:
>>
>> (1a) "git branch foo"
>> (1b) "git checkout foo"
>>
>> However, it has been our experience in our observed use cases and all
>> the existing git tests, that it can be treated as equivalent to:
>>
>> (2a) "git branch foo"
>> (2b) "git symbolic-ref HEAD refs/heads/foo"
>> ...
>
> I am still not sure if I like the change of what "checkout -b" is this
> late in the game, though.
Having said all that.
I do see the merit of having a shorthand way to invoke your 2 above.
It is just that I am not convinced that it is the best way to achieve that goal to redefine what "git checkout -b <new-name>" (no other parameters) does.
^ permalink raw reply
* Re: [PATCH tg/add-chmod+x-fix 2/2] t3700-add: protect one --chmod=+x test with POSIXPERM
From: Johannes Sixt @ 2016-09-21 20:58 UTC (permalink / raw)
To: Junio C Hamano, Thomas Gummerer; +Cc: Git Mailing List
In-Reply-To: <xmqqa8f16kup.fsf@gitster.mtv.corp.google.com>
Am 21.09.2016 um 22:47 schrieb Junio C Hamano:
> -test_expect_success 'file status is changed after git add --chmod=+x' '
> - echo "AM foo4" >expected &&
> +test_expect_success 'git add --chmod=[+-]x changes index with newly added file' '
> echo foo >foo4 &&
> git add foo4 &&
> git add --chmod=+x foo4 &&
> - git status -s foo4 >actual &&
> - test_cmp expected actual
> + test_mode_in_index 100755 foo4
> '
No, that's redundant. There are plenty of other test cases that check
for this. You could just remove the case.
But I came to a different conclusion as I said in a message that crossed
yours. I hope Thomas can pick up the baton again.
-- Hannes
^ 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 20:47 UTC (permalink / raw)
To: Thomas Gummerer; +Cc: Johannes Sixt, Git Mailing List
In-Reply-To: <xmqqtwd986ml.fsf@gitster.mtv.corp.google.com>
Junio C Hamano <gitster@pobox.com> writes:
> ... 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.
In other words, how about doing it this way?
-- >8 --
From: Johannes Sixt <j6t@kdbg.org>
Date: Tue, 20 Sep 2016 08:18:25 +0200
Subject: [PATCH] t3700-add: do not rely on executable bit of the working tree file
A recently introduced test checks the result of 'git status' after
setting the executable bit on a file. This check does not yield the
expected result when the filesystem does not support the executable
bit.
The primary thing we care about is that a file added with --chmod=+x
has executable bit in the index, not that it is registered in the
index somehow differently from what is in the working tree, which
is what the "status" output tries to catch. Let's check what we
care about, i.e. the path is marked as an executable in the index.
Signed-off-by: Johannes Sixt <j6t@kdbg.org>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
t/t3700-add.sh | 6 ++----
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/t/t3700-add.sh b/t/t3700-add.sh
index 16ab2da..1a733bb 100755
--- a/t/t3700-add.sh
+++ b/t/t3700-add.sh
@@ -361,13 +361,11 @@ 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' '
- echo "AM foo4" >expected &&
+test_expect_success 'git add --chmod=[+-]x changes index with newly added file' '
echo foo >foo4 &&
git add foo4 &&
git add --chmod=+x foo4 &&
- git status -s foo4 >actual &&
- test_cmp expected actual
+ test_mode_in_index 100755 foo4
'
test_expect_success 'no file status change if no pathspec is given' '
--
2.10.0-515-g9036219
^ permalink raw reply related
* Re: [PATCH tg/add-chmod+x-fix 2/2] t3700-add: protect one --chmod=+x test with POSIXPERM
From: Johannes Sixt @ 2016-09-21 20:47 UTC (permalink / raw)
To: Junio C Hamano, Thomas Gummerer; +Cc: Git Mailing List
In-Reply-To: <xmqqtwd986ml.fsf@gitster.mtv.corp.google.com>
Am 21.09.2016 um 20:12 schrieb Junio C Hamano:
> 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.
OK.
>> A change of that behaviour may make sense for this
>> though.
Hm, git-add is for moving content from the worktree to the index. I
don't think it should change the worktree. I should not have been surprised.
It should probably not even introduce a change in the index that it does
not see in the worktree, but, hey, git-add is a reasonable porcelain
substitute for the --chmod task that otherwise git-update-index would
have to do.
> Perhaps we shouldn't even test this, then?
If I am right that git-add should not change the worktree, it should be
tested. But 'git status -s' is probably the wrong tool for the reasons
you gave (it could accidentally detect a change due to content
difference instead of a file mode difference). At any rate, such a test
must be protected with POSIXPERM.
-- Hannes
^ permalink raw reply
* Re: v2.10.0: ls-tree exit status is always 0, this differs from ls(1)
From: Junio C Hamano @ 2016-09-21 20:39 UTC (permalink / raw)
To: Steffen Nurpmeso; +Cc: git
In-Reply-To: <20160921194004.QOizfyGm8%steffen@sdaoden.eu>
Steffen Nurpmeso <steffen@sdaoden.eu> writes:
> 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>::
Sorry, but I did not notice that there was an attached patch when I
was reading your response for the first time. Risk of using an
attachment to e-mail ;-)
I think this issue does not need a separate bullet point. The
existing text says:
Note that:
- the behaviour is slightly different from that of "/bin/ls" in that the
'<path>' denotes just a list of patterns to match, e.g. so specifying
directory name (without `-r`) will behave differently, and order of the
arguments does not matter.
- the behaviour is similar to that of "/bin/ls" in that the '<path>' is
taken as relative to the current working directory. E.g. when you are
in a directory 'sub' that has a directory 'dir', you can run 'git
ls-tree -r HEAD dir' to list the contents of the tree (that is
'sub/dir' in `HEAD`). You don't want to give a tree that is not at the
root level (e.g. `git ls-tree -r HEAD:sub dir`) in this case, as that
would result in asking for 'sub/sub/dir' in the `HEAD` commit.
However, the current working directory can be ignored by passing
--full-tree option.
and what caused your surprise is already covered by the first bullet
point, if the reader knows what "patterns to match" means in Git's
command line tools; it just needs to be extended to be more
meaningful to those who don't, I think.
How about rewriting the first bullet point like so instead:
- the behaviour is different from that of "/bin/ls" in that the
'<path>' are actually patterns to match, e.g. so specifying
directory name (without `-r`) will behave differently, the order
of the arguments does not matter, and a '<path>' argument that
does not match any path is not an error (i.e. if there is no
path that matches any pattern, nothing is shown in the output).
^ permalink raw reply
* Re: [PATCH 2/3] gitweb: Link to 7-character SHA1SUMS in commit messages
From: Ævar Arnfjörð Bjarmason @ 2016-09-21 20:09 UTC (permalink / raw)
To: Jakub Narębski; +Cc: Git, Junio C Hamano
In-Reply-To: <88635d93-5288-c933-273a-003e5df824af@gmail.com>
On Wed, Sep 21, 2016 at 8:28 PM, Jakub Narębski <jnareb@gmail.com> wrote:
> W dniu 21.09.2016 o 20:04, Ævar Arnfjörð Bjarmason pisze:
>> 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?
No, just experience. With the caveat that this may not matter at all
in this context, C-like code in Perl is slow, if you can offload
things to one big regex operation it's usually 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.
>
> 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).
Thanks!
^ permalink raw reply
* Re: [PATCH v4 3/3] regex: use regexec_buf()
From: Junio C Hamano @ 2016-09-21 20:09 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git, Jeff King, Benjamin Kramer, René Scharfe
In-Reply-To: <xmqqmvj16p06.fsf@gitster.mtv.corp.google.com>
Junio C Hamano <gitster@pobox.com> writes:
> 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).
Also, I think there is nobody that modified read-only buffer.
diffgrep_consume() does say "Yuck -- line ought to be const",
but its "line" parameter is actually a non-const exactly for
this yuckiness (iow, it knew what it was doing).
Perhaps like so?
regex: use regexec_buf()
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(3) may have been mmap(2)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 allocated to construct NUL-terminated strings,
or worse, modified buffers to temporarily insert NULs while calling
regexec(3). By converting them to use regexec_buf() they have become
much cleaner.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
The patch text looked good to me.
Thanks.
^ permalink raw reply
* 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
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