* Re: [PATCH] userdiff: allow * between cpp funcname words
From: Jeff King @ 2011-12-06 20:19 UTC (permalink / raw)
To: Thomas Rast; +Cc: Junio C Hamano, git
In-Reply-To: <201112062117.57690.trast@student.ethz.ch>
On Tue, Dec 06, 2011 at 09:17:56PM +0100, Thomas Rast wrote:
> > Looks reasonable to me. You can see the difference, for instance, with:
> >
> > git show -U1 3c73a1d
> >
> > (The -U1 is because of the annoying "we will start looking for the
> > header at the top of context, not the top of changes" behavior I
> > mentioned last week).
>
> Actually (sadly) I'll have to revise it. It doesn't match much of C++
> either, and I haven't yet come up with a reasonable regex that
> matches, say,
>
> foo::Bar<int>::t& Baz::operator<<(
>
> which I would call ludicrous, but it's valid C++.
Ick, yeah. Maybe it is worth doing the "*" thing for now, and then
worrying about advanced C++ stuff on top as another patch. AFAICT, your
original patch is a strict improvement.
-Peff
^ permalink raw reply
* Re: What's cooking in git.git (Dec 2011, #02; Mon, 5)
From: Ramkumar Ramachandra @ 2011-12-06 20:30 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Jonathan Nieder
In-Reply-To: <7vliqph8a6.fsf@alter.siamese.dyndns.org>
Hi Junio,
Junio C Hamano wrote:
> A few comments and thoughts, all minor.
>
> * On "revert: simplify getting commit subject in format_todo()"
>
> The old code used to use get_message() on cur->item, checked its return
> value (if cur->item is not parsed, or has already been used and its buffer
> cleared, cur->item->buffer would be NULL and get_message() returns -1) and
> issued an error. The new code uses find_commit_subject without first
> checking if cur->item->buffer is NULL.
>
> Does this mean the old code was overly defensive, or is the new code too
> lax?
>
> I understand that parse_insn_line() uses lookup_commit_reference() which
> calls parse_object() on the object name (and if it is a tag, deref_tag()
> will parse the tagged object until we see something that is not a tag), so
> we know cur->item is parsed before we see it, so I suspect you merely were
> being overly defensive, but I may be missing something.
Right. Actually, I was being overly defensive because I was being
lazy about having to deal the an empty-commit-subject case in the
parser. With "revert: make commit subjects in insn sheet optional",
that's no more a concern- I'll reorder these two patches, and explain
this detail in the commit message in the re-roll.
> * On "revert: make commit subjects in insn sheet optional"
>
> After finding the verb and advancing the pointer "bol" at the beginning of
> the object name, end_of_object_name variable points at the first SP or LF
> using strcspn(bol, " \n"), but I wonder why we are not grabbing a run of
> hexdigits instead, i.e. strspn(bol, "0123456789abcdef"). Is this so that
> we can allow something like "pick rr/revert-cherry-pick~3"?
Yes, it is exactly for that reason :)
> I also wonder if this should be (sorry for pcre) "(pick|revert)\s+(\S+)\s"
> instead, i.e. allow users with fat fingers to use one or more SP or even HT
> to separate the verb and the operand.
Hm, I'm not too enthusiastic about this change, because we don't
advertise hand-editing the instruction sheet yet: it's just some extra
parsing cruft along with guardian buffer-overflow code that buys us no
immediate benefits.
> The last test you added to 3510 in this patch runs test_line_count
> unconditionally, by the way.
Good catch. Missing the chaining '&&' seems to be quite a common
error: I vaguely remember seeing a patch to detect this sometime ago.
Jonathan?
> * On "revert: allow mixed pick and revert instructions"
>
> Reporting what we did not understand from parse_insn_line() is a good
> idea, but I think the line number should be reported at the beginning
> of the same line.
Makes sense. Do you like this?
diff --git a/builtin/revert.c b/builtin/revert.c
index e447449..cfa770e 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -715,7 +715,8 @@ static int format_todo(struct strbuf *buf, struct
replay_insn_list *todo_list)
return 0;
}
-static int parse_insn_line(char *bol, char *eol, struct replay_insn_list *item)
+static int parse_insn_line(char *bol, char *eol,
+ struct replay_insn_list *item, int lineno)
{
unsigned char commit_sha1[20];
char *end_of_object_name;
@@ -731,7 +732,8 @@ static int parse_insn_line(char *bol, char *eol,
struct replay_insn_list *item)
size_t len = strchrnul(bol, '\n') - bol;
if (len > 255)
len = 255;
- return error(_("Unrecognized action: %.*s"), (int)len, bol);
+ return error(_("Line %d: Unrecognized action: %.*s"),
+ lineno, (int)len, bol);
}
end_of_object_name = bol + strcspn(bol, " \n");
@@ -741,11 +743,11 @@ static int parse_insn_line(char *bol, char *eol,
struct replay_insn_list *item)
*end_of_object_name = saved;
if (status < 0)
- return error(_("Malformed object name: %s"), bol);
+ return error(_("Line %d: Malformed object name: %s"), lineno, bol);
item->operand = lookup_commit_reference(commit_sha1);
if (!item->operand)
- return error(_("Not a valid commit: %s"), bol);
+ return error(_("Line %d: Not a valid commit: %s"), lineno, bol);
item->next = NULL;
return 0;
@@ -760,8 +762,8 @@ static int parse_insn_buffer(char *buf, struct
replay_insn_list **todo_list)
for (i = 1; *p; i++) {
char *eol = strchrnul(p, '\n');
- if (parse_insn_line(p, eol, &item) < 0)
- return error(_("on line %d."), i);
+ if (parse_insn_line(p, eol, &item, lineno) < 0)
+ return -1;
next = replay_insn_list_append(item.action, item.operand, next);
p = *eol ? eol + 1 : eol;
}
--
> I'd say that I'd prefer queuing re-rolled patches posted on the list; I'll
> discard the rr/revert-cherry-pick (6a156fd) from my tree.
Thanks for letting me know -- will post shortly.
-- Ram
^ permalink raw reply related
* ANNOUNCE: Git for Windows 1.7.8
From: Pat Thoyts @ 2011-12-06 20:32 UTC (permalink / raw)
To: msysGit, Git Mailing List
This release brings the latest release of Git to Windows users.
Pre-built installers are available from
http://code.google.com/p/msysgit/downloads/list
^ permalink raw reply
* Re: [PATCH] userdiff: allow * between cpp funcname words
From: Johannes Sixt @ 2011-12-06 20:52 UTC (permalink / raw)
To: Jeff King; +Cc: Thomas Rast, Junio C Hamano, git
In-Reply-To: <20111206201944.GB27930@sigill.intra.peff.net>
Am 06.12.2011 21:19, schrieb Jeff King:
> On Tue, Dec 06, 2011 at 09:17:56PM +0100, Thomas Rast wrote:
>
>>> Looks reasonable to me. You can see the difference, for instance, with:
>>>
>>> git show -U1 3c73a1d
>>>
>>> (The -U1 is because of the annoying "we will start looking for the
>>> header at the top of context, not the top of changes" behavior I
>>> mentioned last week).
>>
>> Actually (sadly) I'll have to revise it. It doesn't match much of C++
>> either, and I haven't yet come up with a reasonable regex that
>> matches, say,
>>
>> foo::Bar<int>::t& Baz::operator<<(
>>
>> which I would call ludicrous, but it's valid C++.
>
> Ick, yeah. Maybe it is worth doing the "*" thing for now, and then
> worrying about advanced C++ stuff on top as another patch. AFAICT, your
> original patch is a strict improvement.
Excuse me, where's the problem? The above example shows this
@@ -105,8 +105,8 @@ char *url_decode(const char *url)
struct strbuf out = STRBUF_INIT;
- const char *slash = strchr(url, '/');
+ const char *colon = strchr(url, ':');
...
with current 4cb5d10b. This looks quite correct, no?
-- Hannes
^ permalink raw reply
* Re: Query on git commit amend
From: Junio C Hamano @ 2011-12-06 21:00 UTC (permalink / raw)
To: git; +Cc: Vijay Lakshminarayanan, Viresh Kumar, Shiraz HASHIM
In-Reply-To: <7vobvlfowk.fsf@alter.siamese.dyndns.org>
Junio C Hamano <gitster@pobox.com> writes:
> I almost always use
>
> $ EDITOR=: git commit --amend
>
> when rewriting the contents without updating the message, but I think
> we should allow people to say:
>
> $ git commit --amend --no-edit
>
> which is accepted from the command line but is not honoured.
And this should fix it (only lightly tested).
-- >8 --
Subject: [PATCH] commit: honor --no-edit
After making fixes to the contents to be committed, it is not unusual to
update the current commit without rewording the message. Idioms to do
tell "commit --amend" that we do not need an editor have been:
$ EDITOR=: git commit --amend
$ git commit --amend -C HEAD
but that was only because a more natural
$ git commit --amend --no-edit
did not honour "--no-edit" option.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
builtin/commit.c | 9 +++++----
1 files changed, 5 insertions(+), 4 deletions(-)
diff --git a/builtin/commit.c b/builtin/commit.c
index 8f2bebe..48bea8f 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -81,7 +81,8 @@ static const char *template_file;
static const char *author_message, *author_message_buffer;
static char *edit_message, *use_message;
static char *fixup_message, *squash_message;
-static int all, edit_flag, also, interactive, patch_interactive, only, amend, signoff;
+static int all, also, interactive, patch_interactive, only, amend, signoff;
+static int edit_flag = -1; /* unspecified */
static int quiet, verbose, no_verify, allow_empty, dry_run, renew_authorship;
static int no_post_rewrite, allow_empty_message;
static char *untracked_files_arg, *force_date, *ignore_submodule_arg;
@@ -141,7 +142,7 @@ static struct option builtin_commit_options[] = {
OPT_BOOLEAN(0, "reset-author", &renew_authorship, "the commit is authored by me now (used with -C-c/--amend)"),
OPT_BOOLEAN('s', "signoff", &signoff, "add Signed-off-by:"),
OPT_FILENAME('t', "template", &template_file, "use specified template file"),
- OPT_BOOLEAN('e', "edit", &edit_flag, "force edit of commit"),
+ OPT_BOOL('e', "edit", &edit_flag, "force edit of commit"),
OPT_STRING(0, "cleanup", &cleanup_arg, "default", "how to strip spaces and #comments from message"),
OPT_BOOLEAN(0, "status", &include_status, "include status in commit message template"),
/* end commit message options */
@@ -1020,8 +1021,8 @@ static int parse_and_validate_options(int argc, const char *argv[],
if (logfile || message.len || use_message || fixup_message)
use_editor = 0;
- if (edit_flag)
- use_editor = 1;
+ if (0 <= edit_flag)
+ use_editor = edit_flag;
if (!use_editor)
setenv("GIT_EDITOR", ":", 1);
--
1.7.8.157.g03e55
^ permalink raw reply related
* Re: ANNOUNCE: Git for Windows 1.7.8
From: Erik Faye-Lund @ 2011-12-06 21:03 UTC (permalink / raw)
To: Pat Thoyts; +Cc: msysGit, Git Mailing List
In-Reply-To: <CABNJ2GJ_1Nf9rWev6BKE9zcqt5yrgq6PbaMLVRD705UapLHf0w@mail.gmail.com>
On Tue, Dec 6, 2011 at 9:32 PM, Pat Thoyts <patthoyts@gmail.com> wrote:
> This release brings the latest release of Git to Windows users.
>
> Pre-built installers are available from
> http://code.google.com/p/msysgit/downloads/list
Great work, thanks a lot!
^ permalink raw reply
* Re: [PATCH] userdiff: allow * between cpp funcname words
From: René Scharfe @ 2011-12-06 21:07 UTC (permalink / raw)
To: Johannes Sixt; +Cc: Jeff King, Thomas Rast, Junio C Hamano, git
In-Reply-To: <4EDE8086.9080303@kdbg.org>
Am 06.12.2011 21:52, schrieb Johannes Sixt:
> Am 06.12.2011 21:19, schrieb Jeff King:
>> On Tue, Dec 06, 2011 at 09:17:56PM +0100, Thomas Rast wrote:
>>
>>>> Looks reasonable to me. You can see the difference, for instance, with:
>>>>
>>>> git show -U1 3c73a1d
>>>>
>>>> (The -U1 is because of the annoying "we will start looking for the
>>>> header at the top of context, not the top of changes" behavior I
>>>> mentioned last week).
>>>
>>> Actually (sadly) I'll have to revise it. It doesn't match much of C++
>>> either, and I haven't yet come up with a reasonable regex that
>>> matches, say,
>>>
>>> foo::Bar<int>::t& Baz::operator<<(
>>>
>>> which I would call ludicrous, but it's valid C++.
>>
>> Ick, yeah. Maybe it is worth doing the "*" thing for now, and then
>> worrying about advanced C++ stuff on top as another patch. AFAICT, your
>> original patch is a strict improvement.
>
> Excuse me, where's the problem? The above example shows this
>
> @@ -105,8 +105,8 @@ char *url_decode(const char *url)
> struct strbuf out = STRBUF_INIT;
> - const char *slash = strchr(url, '/');
> + const char *colon = strchr(url, ':');
> ...
>
> with current 4cb5d10b. This looks quite correct, no?
That's with the default heuristic; try something like this first to turn
on userdiff:
$ echo url.c diff=cpp >>.gitattributes
René
^ permalink raw reply
* Re: What's cooking in git.git (Dec 2011, #02; Mon, 5)
From: Luke Diamand @ 2011-12-06 21:12 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <7v8vmqi98f.fsf@alter.siamese.dyndns.org>
On 06/12/11 05:01, Junio C Hamano wrote:
> Here are the topics that have been cooking. Commits prefixed with '-' are
> only in 'pu' (proposed updates) while commits prefixed with '+' are in
> 'next'.
>
>
> Will merge to 'next'.
>
> * ld/p4-labels-branches (2011-11-30) 4 commits
> - git-p4: importing labels should cope with missing owner
> - git-p4: add test for p4 labels
> - git-p4: cope with labels with empty descriptions
> - git-p4: handle p4 branches and labels containing shell chars
>
> I understand this has been retracted---please correct me otherwise.
> Will discard, expecting a reroll.
Yes, discard this one and I'll re-roll it with fixes for the other label
issues.
Thanks
Luke
^ permalink raw reply
* Re: [PATCH] userdiff: allow * between cpp funcname words
From: Junio C Hamano @ 2011-12-06 21:15 UTC (permalink / raw)
To: Thomas Rast; +Cc: Jeff King, git
In-Reply-To: <201112062117.57690.trast@student.ethz.ch>
Thomas Rast <trast@student.ethz.ch> writes:
> Actually (sadly) I'll have to revise it. It doesn't match much of C++
> either, and I haven't yet come up with a reasonable regex that
> matches, say,
>
> foo::Bar<int>::t& Baz::operator<<(
>
> which I would call ludicrous, but it's valid C++.
Heh, I'd rather not see us go that route, which would either end up
implementing a C++ parser or reverting the heuristics back to "non-blank
at the beginning of the line" that was already reasonably useful.
^ permalink raw reply
* Re: Auto update submodules after merge and reset
From: Jens Lehmann @ 2011-12-06 21:32 UTC (permalink / raw)
To: Max Krasnyansky; +Cc: git
In-Reply-To: <4EDD6A8C.40008@qualcomm.com>
Am 06.12.2011 02:06, schrieb Max Krasnyansky:
> On 11/30/2011 12:31 AM, Jens Lehmann wrote:
>> I'm working on a patch series to teach Git to optionally update the submodules work trees on checkout, reset merge and so on, but I'm not there yet.
> Everything you suggested sounds great. We're on the same page (config option, etc).
> How far along are you? Do you have a tree I could pull from to play with things?
> I could help with testing, bug fixes and/or implementing parts of it. Let me know.
Great to hear that! Please see my GitHub repo for the current state:
https://github.com/jlehmann/git-submod-enhancements
It has two interesting branches:
git-checkout-recurse-submodules:
This was my first attempt to tell unpack_trees() to checkout submodules
and works quite well. Porcelain checks out submodules by default while
plumbing learned the --recurse-submodules option to do that (and git gui
and gitk use that option so stuff like "Revert Changes" does work on
submodules :-). I use it at work for some time and it works quite well,
but doesn't handle new or deleted submodules. And unfortunately the way
I added the flag to control submodule checkout doesn't allow to add a
per-submodule configuration option.
recursive_submodule_checkout:
This is where new development happens. I added the basic infrastructure
to have global and per-submodule configuration controlling the checkout
and ported the unpack_trees() changes from git-checkout-recurse-submodules
here. I also added removal and creation of submodules based on the now
moved gitdir. This branch has rudimentary tests but still needs quite some
work.
I expect to have some time around the end of year to move things forward.
It'd be cool if you could check the current state, after that we can
decide how to move the topic forward together.
> For now I implemented automatic submodules update using 'post-merge' hook. But obviously it does
> not handle reset and things. I'm thinking of adding 'post-reset' and 'pre-merge' that would be useful
> for this and maybe other things.
I doubt hooks can be more than a band aid for submodule checkout. I thought
about doing that too and came to the conclusion it will only handle some of
the issues. And you'll have to provide a real life use case to get a new
hook accepted ;-)
^ permalink raw reply
* [PATCH 0/2] run-command: Add EACCES diagnostics
From: Frans Klaver @ 2011-12-06 21:38 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano
In-Reply-To: <op.v5e8mgbc0aolir@keputer>
So here's a couple of patches that introduce some more elaborate investigation
into what went wrong when receiving EACCES. This is probably something that
could be expanded in the future, as running a command doesn't always produce
equally obvious error messages.
"run-command: Add checks after execvp fails with EACCES" provides some basic checks
on the permissions in PATH, and gives just a warning that none of its checks
indicate a problem, so the user should check at least the interpreter permissions.
"run-command: Add interpreter permissions check" actually adds interpreter checking.
---
run-command.c | 172 ++++++++++++++++++++++++++++++++++++++++++++++++
t/t0061-run-command.sh | 38 ++++++++++-
2 files changed, 209 insertions(+), 1 deletions(-)
^ permalink raw reply
* [PATCH 1/2] run-command: Add checks after execvp fails with EACCES
From: Frans Klaver @ 2011-12-06 21:38 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Frans Klaver
In-Reply-To: <1323207503-26581-1-git-send-email-fransklaver@gmail.com>
execvp returns ENOENT if a command was not found after searching PATH.
If path contains a directory that current user has insufficient
privileges to, EACCES is returned. This may still mean the program
wasn't found and may cause confusion to the user, especially when the
file mentioned doesn't exist -- that is, the user would expect NOENT to
be returned -- and the user was actually hoping for an alias to be executed.
To help users track down the core issue more easily, perform some checks
on the path and file permissions involved. Output errors when paths or
files don't have enough permissions.
Signed-off-by: Frans Klaver <fransklaver@gmail.com>
---
run-command.c | 118 ++++++++++++++++++++++++++++++++++++++++++++++++
t/t0061-run-command.sh | 16 ++++++-
2 files changed, 133 insertions(+), 1 deletions(-)
diff --git a/run-command.c b/run-command.c
index 1c51043..5e38c5a 100644
--- a/run-command.c
+++ b/run-command.c
@@ -2,6 +2,7 @@
#include "run-command.h"
#include "exec_cmd.h"
#include "argv-array.h"
+#include "dir.h"
static inline void close_pair(int fd[2])
{
@@ -134,6 +135,119 @@ static int wait_or_whine(pid_t pid, const char *argv0, int silent_exec_failure)
return code;
}
+#ifndef WIN32
+static int is_in_group(gid_t gid)
+{
+ gid_t *groups;
+ int ngroups, gc;
+ int yes;
+
+ if (gid == getgid())
+ return 1;
+
+ groups = NULL;
+ ngroups = getgroups(0, NULL);
+ if (ngroups > 0) {
+ groups = (gid_t *)xmalloc(ngroups * sizeof(gid_t));
+ if (getgroups(ngroups, groups) < 0) {
+ free(groups);
+ return 0;
+ }
+ }
+
+ yes = 0;
+ for (gc = 0; gc < ngroups; gc++)
+ if (groups[gc] == gid)
+ yes = 1;
+
+ free(groups);
+ return yes;
+}
+
+static int have_read_execute_permissions(const char *path)
+{
+ struct stat s;
+ trace_printf("checking '%s'\n", path);
+
+ if (stat(path, &s) < 0) {
+ trace_printf("could not stat '%s': %s\n",
+ path, strerror(errno));
+ return 0;
+ }
+ trace_printf("uid: %d, gid: %d\n", s.st_uid, s.st_gid);
+ trace_printf("mode: %o\n", s.st_mode);
+
+ /* check world permissions */
+ if ((s.st_mode&(S_IXOTH|S_IROTH)) == (S_IXOTH|S_IROTH))
+ return 1;
+
+ /* check group permissions & membership */
+ if ((s.st_mode&(S_IXGRP|S_IRGRP)) == (S_IXGRP|S_IRGRP) &&
+ is_in_group(s.st_gid))
+ return 1;
+
+ /* check owner permissions & ownership */
+ if ((s.st_mode&(S_IXUSR|S_IRUSR)) == (S_IXUSR|S_IRUSR) &&
+ s.st_uid == getuid())
+ return 1;
+
+ return 0;
+}
+
+static void diagnose_execvp_eacces(const char *cmd, const char **argv)
+{
+ /* man 2 execve states that EACCES is returned for:
+ * - Search permission is denied on a component of the path prefix
+ * of cmd or the name of a script interpreter
+ * - The file or script interpreter is not a regular file
+ * - Execute permission is denied for the file, script or ELF
+ * interpreter
+ * - The file system is mounted noexec
+ */
+ struct strbuf sb = STRBUF_INIT;
+ char *path = getenv("PATH");
+ char *next;
+
+ if (strchr(cmd, '/')) {
+ if (!have_read_execute_permissions(cmd))
+ error("no read/execute permissions on '%s'\n", cmd);
+ return;
+ }
+
+ for (;;) {
+ next = strchrnul(path, ':');
+ if (path < next)
+ strbuf_add(&sb, path, next - path);
+ else
+ strbuf_addch(&sb, '.');
+
+ if (!have_read_execute_permissions(sb.buf))
+ error("no read/execute permissions on '%s'\n", sb.buf);
+
+ if (sb.len && sb.buf[sb.len - 1] != '/')
+ strbuf_addch(&sb, '/');
+ strbuf_addstr(&sb, cmd);
+
+ if (file_exists(sb.buf)) {
+ if (!have_read_execute_permissions(sb.buf))
+ error("no read/execute permissions on '%s'\n",
+ sb.buf);
+ else
+ warn("file '%s' exists and permissions "
+ "seem OK.\nIf this is a script, see if you "
+ "have sufficient privileges to run the "
+ "interpreter", sb.buf);
+ }
+
+ strbuf_release(&sb);
+
+ if (!*next)
+ break;
+ path = next + 1;
+ }
+}
+#endif
+
int start_command(struct child_process *cmd)
{
int need_in, need_out, need_err;
@@ -285,6 +399,10 @@ fail_pipe:
error("cannot run %s: %s", cmd->argv[0],
strerror(ENOENT));
exit(127);
+ } else if (errno == EACCES) {
+ diagnose_execvp_eacces(cmd->argv[0], cmd->argv);
+ die("cannot exec '%s': %s", cmd->argv[0],
+ strerror(EACCES));
} else {
die_errno("cannot exec '%s'", cmd->argv[0]);
}
diff --git a/t/t0061-run-command.sh b/t/t0061-run-command.sh
index 8d4938f..b39bd16 100755
--- a/t/t0061-run-command.sh
+++ b/t/t0061-run-command.sh
@@ -26,7 +26,7 @@ test_expect_success 'run_command can run a command' '
test_cmp empty err
'
-test_expect_success POSIXPERM 'run_command reports EACCES' '
+test_expect_success POSIXPERM 'run_command reports EACCES, file permissions' '
cat hello-script >hello.sh &&
chmod -x hello.sh &&
test_must_fail test-run-command run-command ./hello.sh 2>err &&
@@ -34,4 +34,18 @@ test_expect_success POSIXPERM 'run_command reports EACCES' '
grep "fatal: cannot exec.*hello.sh" err
'
+test_expect_success POSIXPERM 'run_command reports EACCES, search path permisions' '
+ mkdir -p inaccessible &&
+ PATH=$(pwd)/inaccessible:$PATH &&
+ export PATH &&
+
+ cat hello-script >inaccessible/hello.sh &&
+ chmod 400 inaccessible &&
+ test_must_fail test-run-command run-command hello.sh 2>err &&
+ chmod 755 inaccessible &&
+
+ grep "fatal: cannot exec.*hello.sh" err &&
+ grep "no read/execute permissions on" err
+'
+
test_done
--
1.7.8
^ permalink raw reply related
* [PATCH 2/2] run-command: Add interpreter permissions check
From: Frans Klaver @ 2011-12-06 21:38 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Frans Klaver
In-Reply-To: <1323207503-26581-1-git-send-email-fransklaver@gmail.com>
If a script is started and the interpreter of that script given in the
shebang cannot be started due to permissions, we can get a rather
obscure situation. All permission checks pass for the script itself,
but we still get EACCES from execvp.
Try to find out if the above is the case and warn the user about it.
Signed-off-by: Frans Klaver <fransklaver@gmail.com>
---
run-command.c | 66 +++++++++++++++++++++++++++++++++++++++++++----
t/t0061-run-command.sh | 22 ++++++++++++++++
2 files changed, 82 insertions(+), 6 deletions(-)
diff --git a/run-command.c b/run-command.c
index 5e38c5a..b8cf8d4 100644
--- a/run-command.c
+++ b/run-command.c
@@ -194,6 +194,63 @@ static int have_read_execute_permissions(const char *path)
return 0;
}
+static void check_interpreter(const char *cmd)
+{
+ FILE *f;
+ struct strbuf sb = STRBUF_INIT;
+ /* bash reads an 80 character line when determining the interpreter.
+ * BSD apparently only allows 32 characters, as it is the size of
+ * your average binary executable header.
+ */
+ char firstline[80];
+ char *interpreter = NULL;
+ size_t s, i;
+
+ f = fopen(cmd, "r");
+ if (!f) {
+ error("cannot open file '%s': %s\n", cmd, strerror(errno));
+ return;
+ }
+
+ s = fread(firstline, 1, sizeof(firstline), f);
+ if (s < 2) {
+ trace_printf("cannot determine file type");
+ fclose(f);
+ return;
+ }
+
+ if (firstline[0] != '#' || firstline[1] != '!') {
+ trace_printf("file '%s' is not a script or"
+ " is a script without '#!'", cmd);
+ fclose(f);
+ return;
+ }
+
+ /* see if the given path has the executable bit set */
+ for (i = 2; i < s; i++) {
+ if (!interpreter && firstline[i] != ' ' && firstline[i] != '\t')
+ interpreter = firstline + i;
+
+ if (interpreter && (firstline[i] == ' ' ||
+ firstline[i] == '\n')) {
+ strbuf_add(&sb, interpreter,
+ (firstline + i) - interpreter);
+ break;
+ }
+ }
+ if (!sb.len) {
+ error("could not determine interpreter");
+ strbuf_release(&sb);
+ return;
+ }
+
+ if (!have_read_execute_permissions(sb.buf))
+ error("bad interpreter: no read/execute permissions on '%s'\n",
+ sb.buf);
+
+ strbuf_release(&sb);
+}
+
static void diagnose_execvp_eacces(const char *cmd, const char **argv)
{
/* man 2 execve states that EACCES is returned for:
@@ -209,8 +266,8 @@ static void diagnose_execvp_eacces(const char *cmd, const char **argv)
char *next;
if (strchr(cmd, '/')) {
- if (!have_read_execute_permissions(cmd))
- error("no read/execute permissions on '%s'\n", cmd);
+ if (have_read_execute_permissions(cmd))
+ check_interpreter(cmd);
return;
}
@@ -233,10 +290,7 @@ static void diagnose_execvp_eacces(const char *cmd, const char **argv)
error("no read/execute permissions on '%s'\n",
sb.buf);
else
- warn("file '%s' exists and permissions "
- "seem OK.\nIf this is a script, see if you "
- "have sufficient privileges to run the "
- "interpreter", sb.buf);
+ check_interpreter(sb.buf);
}
strbuf_release(&sb);
diff --git a/t/t0061-run-command.sh b/t/t0061-run-command.sh
index b39bd16..39bfaef 100755
--- a/t/t0061-run-command.sh
+++ b/t/t0061-run-command.sh
@@ -13,6 +13,18 @@ cat >hello-script <<-EOF
EOF
>empty
+cat >someinterpreter <<-EOF
+ #!$SHELL_PATH
+ cat hello-script
+EOF
+>empty
+
+cat >incorrect-interpreter-script <<-EOF
+ #!someinterpreter
+ cat hello-script
+EOF
+>empty
+
test_expect_success 'start_command reports ENOENT' '
test-run-command start-command-ENOENT ./does-not-exist
'
@@ -48,4 +60,14 @@ test_expect_success POSIXPERM 'run_command reports EACCES, search path permision
grep "no read/execute permissions on" err
'
+test_expect_success POSIXPERM 'run_command reports EACCES, interpreter fails' '
+ cat incorrect-interpreter-script >hello.sh &&
+ chmod +x hello.sh &&
+ chmod -x someinterpreter &&
+ test_must_fail test-run-command run-command ./hello.sh 2>err &&
+
+ grep "fatal: cannot exec.*hello.sh" err &&
+ grep "bad interpreter" err
+'
+
test_done
--
1.7.8
^ permalink raw reply related
* Re: [PATCHv2 0/13] credential helpers
From: Junio C Hamano @ 2011-12-06 21:40 UTC (permalink / raw)
To: Jeff King; +Cc: git
In-Reply-To: <20111206062127.GA29046@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> ... You can now
> do: "git credential-store erase </dev/null" to erase everything
> (since you have provided no restrictions, it matches everything).
That "justification" does not sound so true to me but perhaps that is
because it is unclear what "erase" means and what it means to give the
operation parameters.
When I see "erase $foo", I would find it natural if $foo meant "if there
is something that matches $foo, then please remove it, but keep everything
else intact", and not the other way around "Match the existing entries
against a pattern (or a set of matching patterns) I am giving you, and
drop all the rest". So if I happen to give you an empty set, I would
expect nothing is removed.
Perhaps the root cause of the issue is that you are treating the input as
"restriction" instead of something that produces "positive matches"?
Confused.
^ permalink raw reply
* Odd issue - The Diffs That WILL NOT DIE.
From: Chris Patti @ 2011-12-06 21:43 UTC (permalink / raw)
To: git
I have a Homebrew installed version if Git 1.7.8 running on OSX Lion.
I'm seeing a very odd issue where these diffs I didn't create keep
recurring in a particular repository.
I've tried:
* Nuking the repo and re-cloning, cloning into a totally different
containing directory
* git reset --hard, git checkout -- of the offending file supposedly
containing the diffs
Is there some sort of uber persistent local cache that's bound to the
remote repository?
Thanks in advance,
-Chris
--
Christopher Patti - Geek At Large | GTalk: cpatti@gmail.com | AIM:
chrisfeohpatti | P: (260) 54PATTI
"Technology challenges art, art inspires technology." - John Lasseter, Pixar
^ permalink raw reply
* Re: Odd issue - The Diffs That WILL NOT DIE.
From: Frans Klaver @ 2011-12-06 21:47 UTC (permalink / raw)
To: git, Chris Patti
In-Reply-To: <CAJ8P3RBm=RhNf6LKLqprqX6Rqx0OgRnJR+=+-Qhg4PvpeqaUDg@mail.gmail.com>
On Tue, 06 Dec 2011 22:43:50 +0100, Chris Patti <cpatti@gmail.com> wrote:
> I have a Homebrew installed version if Git 1.7.8 running on OSX Lion.
>
> I'm seeing a very odd issue where these diffs I didn't create keep
> recurring in a particular repository.
Could you be a little more specific about the nature of the diffs? Is it
reproducible on another system? It sounds like newlines or whitespace. Does
$ git diff -w
produce the same result?
>
> I've tried:
>
> * Nuking the repo and re-cloning, cloning into a totally different
> containing directory
> * git reset --hard, git checkout -- of the offending file supposedly
> containing the diffs
>
> Is there some sort of uber persistent local cache that's bound to the
> remote repository?
>
> Thanks in advance,
> -Chris
>
^ permalink raw reply
* Re: Odd issue - The Diffs That WILL NOT DIE.
From: Carlos Martín Nieto @ 2011-12-06 21:51 UTC (permalink / raw)
To: Chris Patti; +Cc: git
In-Reply-To: <CAJ8P3RBm=RhNf6LKLqprqX6Rqx0OgRnJR+=+-Qhg4PvpeqaUDg@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 895 bytes --]
On Tue, Dec 06, 2011 at 04:43:50PM -0500, Chris Patti wrote:
> I have a Homebrew installed version if Git 1.7.8 running on OSX Lion.
>
> I'm seeing a very odd issue where these diffs I didn't create keep
> recurring in a particular repository.
Which diffs? You haven't given us any? What files does this happen
with? Do they have any peculiarities?
If these are files with non-ASCII filenames, then you're hitting a
misfeature of the HFS+ filesystem (it lies when git asks it about
files).
>
> I've tried:
>
> * Nuking the repo and re-cloning, cloning into a totally different
> containing directory
> * git reset --hard, git checkout -- of the offending file supposedly
> containing the diffs
>
> Is there some sort of uber persistent local cache that's bound to the
> remote repository?
The remote repository shouldn't have anything to do with this.
cmn
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 490 bytes --]
^ permalink raw reply
* Re: [PATCHv2 12/13] credentials: add "store" helper
From: Junio C Hamano @ 2011-12-06 21:50 UTC (permalink / raw)
To: Jeff King; +Cc: git
In-Reply-To: <20111206062305.GL29233@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> +static void store_credential(const char *fn, struct credential *c)
> +{
> + struct strbuf buf = STRBUF_INIT;
> +
> + /*
> + * Sanity check that what we are storing is actually sensible.
> + * In particular, we can't make a URL without a protocol field.
> + * Without either a host or pathname (depending on the scheme),
> + * we have no primary key. And without a username and password,
> + * we are not actually storing a credential.
> + */
> + if (!c->protocol || !(c->host || c->path) ||
> + !c->username || !c->password)
> + return;
Very nicely explained. I wish all our patches had comments like this to
explain tricky bit that looks as if the choice was arbitrarily made but
in fact the logic was carefully constructed.
Thanks.
^ permalink raw reply
* Re: [PATCHv2 13/13] t: add test harness for external credential helpers
From: Junio C Hamano @ 2011-12-06 21:51 UTC (permalink / raw)
To: Jeff King; +Cc: git
In-Reply-To: <20111206062313.GM29233@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> diff --git a/t/t0303-credential-external.sh b/t/t0303-credential-external.sh
> new file mode 100755
> index 0000000..79b046f
> --- /dev/null
> +++ b/t/t0303-credential-external.sh
> @@ -0,0 +1,23 @@
> ...
> +else
> + helper_test_clean "$GIT_TEST_CREDENTIAL_HELPER"
> + helper_test "$GIT_TEST_CREDENTIAL_HELPER"
> +# helper_test_clean "$GIT_TEST_CREDENTIAL_HELPER"
> +fi
Huh? Leftover debugging cruft?
^ permalink raw reply
* Re: [PATCHv2 13/13] t: add test harness for external credential helpers
From: Jeff King @ 2011-12-06 22:08 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <7vvcpte5cg.fsf@alter.siamese.dyndns.org>
On Tue, Dec 06, 2011 at 01:51:43PM -0800, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
>
> > diff --git a/t/t0303-credential-external.sh b/t/t0303-credential-external.sh
> > new file mode 100755
> > index 0000000..79b046f
> > --- /dev/null
> > +++ b/t/t0303-credential-external.sh
> > @@ -0,0 +1,23 @@
> > ...
> > +else
> > + helper_test_clean "$GIT_TEST_CREDENTIAL_HELPER"
> > + helper_test "$GIT_TEST_CREDENTIAL_HELPER"
> > +# helper_test_clean "$GIT_TEST_CREDENTIAL_HELPER"
> > +fi
>
> Huh? Leftover debugging cruft?
Oops, yes. It should be:
clean
do_the_test
clean
The first clean is "remove any cruft accidentally leftover from a
previous run, so we can do our test". The second clean is "be a good
citizen and get rid of cruft we just accumulated".
As part of my testing, I commented out the second clean momentarily so
that I could verify that the cruft was left without the clean, but gone
with the clean. And then I accidentally committed with the "#" left in.
Here's the correct version, with some extra comments about the
clean-test-clean cycle:
-- >8 --
Subject: [PATCHv2 13/13] t: add test harness for external credential helpers
We already have tests for the internal helpers, but it's
nice to give authors of external tools an easy way to
sanity-check their helpers.
If you have written the "git-credential-foo" helper, you can
do so with:
GIT_TEST_CREDENTIAL_HELPER=foo \
make t0303-credential-external.sh
This assumes that your helper is capable of both storing and
retrieving credentials (some helpers may be read-only, and
they will fail these tests).
If your helper supports time-based expiration with a
configurable timeout, you can test that feature like this:
GIT_TEST_CREDENTIAL_HELPER_TIMEOUT="foo --timeout=1" \
make t0303-credential-external.sh
Signed-off-by: Jeff King <peff@peff.net>
---
t/t0303-credential-external.sh | 30 ++++++++++++++++++++++++++++++
1 files changed, 30 insertions(+), 0 deletions(-)
create mode 100755 t/t0303-credential-external.sh
diff --git a/t/t0303-credential-external.sh b/t/t0303-credential-external.sh
new file mode 100755
index 0000000..092dd3c
--- /dev/null
+++ b/t/t0303-credential-external.sh
@@ -0,0 +1,30 @@
+#!/bin/sh
+
+test_description='external credential helper tests'
+. ./test-lib.sh
+. "$TEST_DIRECTORY"/lib-credential.sh
+
+if test -z "$GIT_TEST_CREDENTIAL_HELPER"; then
+ say "# skipping external helper tests (set GIT_TEST_CREDENTIAL_HELPER)"
+else
+ # clean before the test in case there is cruft left
+ # over from a previous run that would impact results
+ helper_test_clean "$GIT_TEST_CREDENTIAL_HELPER"
+
+ helper_test "$GIT_TEST_CREDENTIAL_HELPER"
+
+ # then clean afterwards so that we are good citizens
+ # and don't leave cruft in the helper's storage, which
+ # might be long-term system storage
+ helper_test_clean "$GIT_TEST_CREDENTIAL_HELPER"
+fi
+
+if test -z "$GIT_TEST_CREDENTIAL_HELPER_TIMEOUT"; then
+ say "# skipping external helper timeout tests"
+else
+ helper_test_clean "$GIT_TEST_CREDENTIAL_HELPER_TIMEOUT"
+ helper_test_timeout "$GIT_TEST_CREDENTIAL_HELPER_TIMEOUT"
+ helper_test_clean "$GIT_TEST_CREDENTIAL_HELPER_TIMEOUT"
+fi
+
+test_done
--
1.7.8.rc2.8.gf076c
^ permalink raw reply related
* Re: [PATCH 1/2] run-command: Add checks after execvp fails with EACCES
From: Junio C Hamano @ 2011-12-06 22:35 UTC (permalink / raw)
To: Frans Klaver; +Cc: git, Junio C Hamano
In-Reply-To: <1323207503-26581-2-git-send-email-fransklaver@gmail.com>
Frans Klaver <fransklaver@gmail.com> writes:
> +#ifndef WIN32
> +static int is_in_group(gid_t gid)
> ...
> +static int have_read_execute_permissions(const char *path)
> +{
> + struct stat s;
> + trace_printf("checking '%s'\n", path);
> +
> + if (stat(path, &s) < 0) {
> + ...
> + /* check world permissions */
> + if ((s.st_mode&(S_IXOTH|S_IROTH)) == (S_IXOTH|S_IROTH))
> + return 1;
Hmm, do you need to do this with stat(2)?
Wouldn't access(2) with R_OK|X_OK give you exactly what you want without
this much trouble?
I also think that your permission check is incorrectly implemented.
$ cd /var/tmp && date >j && chmod 044 j && ls -l j
----r--r-- 1 junio junio 29 Dec 6 14:32 j
$ cat j
cat: j: Permission denied
$ su pogo
Password:
$ cat j
Tue Dec 6 14:32:23 PST 2011
That's a world-readable but unreadable-only-to-me file.
> +static void diagnose_execvp_eacces(const char *cmd, const char **argv)
> +{
> + /* man 2 execve states that EACCES is returned for:
/*
* Just a style, but we tend to write multi-line comment like
* this, without anything else on opening and closing lines of
* the comment block.
*/
> + * - The file system is mounted noexec
> + */
> + struct strbuf sb = STRBUF_INIT;
> + char *path = getenv("PATH");
> + char *next;
> +
> + if (strchr(cmd, '/')) {
> + if (!have_read_execute_permissions(cmd))
> + error("no read/execute permissions on '%s'\n", cmd);
> + return;
> + }
Ok, execvp() failed and "cmd" has at least one slash, so we know we did
not look for it in $PATH. We check only one and return (did you need
getenv() in that case?).
> + for (;;) {
> + next = strchrnul(path, ':');
> + if (path < next)
> + strbuf_add(&sb, path, next - path);
> + else
> + strbuf_addch(&sb, '.');
Nice touch that you did not forget an empty component on $PATH.
> + if (!have_read_execute_permissions(sb.buf))
> + error("no read/execute permissions on '%s'\n", sb.buf);
Don't you want to continue here upon error, after resetting sb? You just
saw the directory is unreadble, so you know next file_exists() will fail
before you try it.
> + if (sb.len && sb.buf[sb.len - 1] != '/')
> + strbuf_addch(&sb, '/');
> + strbuf_addstr(&sb, cmd);
> +
> + if (file_exists(sb.buf)) {
> + if (!have_read_execute_permissions(sb.buf))
> + error("no read/execute permissions on '%s'\n",
> + sb.buf);
> + else
> + warn("file '%s' exists and permissions "
> + "seem OK.\nIf this is a script, see if you "
> + "have sufficient privileges to run the "
> + "interpreter", sb.buf);
Does "warn()" do the right thing for multi-line strings like this?
^ permalink raw reply
* Re: [PATCH 2/2] run-command: Add interpreter permissions check
From: Junio C Hamano @ 2011-12-06 22:47 UTC (permalink / raw)
To: Frans Klaver; +Cc: git, Junio C Hamano
In-Reply-To: <1323207503-26581-3-git-send-email-fransklaver@gmail.com>
Frans Klaver <fransklaver@gmail.com> writes:
> If a script is started and the interpreter of that script given in the
> shebang cannot be started due to permissions, we can get a rather
> obscure situation. All permission checks pass for the script itself,
> but we still get EACCES from execvp.
>
> Try to find out if the above is the case and warn the user about it.
>
> Signed-off-by: Frans Klaver <fransklaver@gmail.com>
> ---
> run-command.c | 66 +++++++++++++++++++++++++++++++++++++++++++----
> t/t0061-run-command.sh | 22 ++++++++++++++++
> 2 files changed, 82 insertions(+), 6 deletions(-)
>
> diff --git a/run-command.c b/run-command.c
> index 5e38c5a..b8cf8d4 100644
> --- a/run-command.c
> +++ b/run-command.c
> @@ -194,6 +194,63 @@ static int have_read_execute_permissions(const char *path)
> return 0;
> }
>
> +static void check_interpreter(const char *cmd)
> +{
> + FILE *f;
> + struct strbuf sb = STRBUF_INIT;
> + /* bash reads an 80 character line when determining the interpreter.
> + * BSD apparently only allows 32 characters, as it is the size of
> + * your average binary executable header.
> + */
> + char firstline[80];
> + char *interpreter = NULL;
> + size_t s, i;
> +
> + f = fopen(cmd, "r");
> + if (!f) {
> + error("cannot open file '%s': %s\n", cmd, strerror(errno));
> + return;
> + }
> +
> + s = fread(firstline, 1, sizeof(firstline), f);
> + if (s < 2) {
> + trace_printf("cannot determine file type");
> + fclose(f);
> + return;
> + }
> +
> + if (firstline[0] != '#' || firstline[1] != '!') {
> + trace_printf("file '%s' is not a script or"
> + " is a script without '#!'", cmd);
> + fclose(f);
> + return;
> + }
Nice touches to silently pass scripts that do not begin with she-bang.
> +
> + /* see if the given path has the executable bit set */
> + for (i = 2; i < s; i++) {
> + if (!interpreter && firstline[i] != ' ' && firstline[i] != '\t')
> + interpreter = firstline + i;
> +
> + if (interpreter && (firstline[i] == ' ' ||
> + firstline[i] == '\n')) {
Curious.
"#!<TAB>/bin/bash<TAB><LF>" would cause you to check "/bin/bash<TAB>"?
> + strbuf_add(&sb, interpreter,
> + (firstline + i) - interpreter);
> + break;
> + }
Wouldn't strcspn() work better instead of this loop?
> + }
> + if (!sb.len) {
> + error("could not determine interpreter");
> + strbuf_release(&sb);
> + return;
> + }
> +
> + if (!have_read_execute_permissions(sb.buf))
> + error("bad interpreter: no read/execute permissions on '%s'\n",
> + sb.buf);
> +
> + strbuf_release(&sb);
> +}
> +
> static void diagnose_execvp_eacces(const char *cmd, const char **argv)
> {
> /* man 2 execve states that EACCES is returned for:
> @@ -209,8 +266,8 @@ static void diagnose_execvp_eacces(const char *cmd, const char **argv)
> char *next;
>
> if (strchr(cmd, '/')) {
> - if (!have_read_execute_permissions(cmd))
> - error("no read/execute permissions on '%s'\n", cmd);
> + if (have_read_execute_permissions(cmd))
> + check_interpreter(cmd);
I would have expected the overall logic to be more like this:
if we cannot read and execute it then
that in itself is an error (i.e. the error message from [1/2])
else if we can read it then
let's see if there is an error in the interpreter.
It is unnatural to see "if we can read and execute, then see if there is
anything wrong with the interpreter" and _nothing else_ here. If you made
the "have_read_execute_permissions()" to issue the error message you used
to give in your [1/2] patch here, that is OK from the point of view of the
overall code structure, but then the function is no longer "do we have
permissions" boolean check and needs to be renamed. And if you didn't,
then I have to wonder why we do not need the error message you added in
your [1/2].
> @@ -233,10 +290,7 @@ static void diagnose_execvp_eacces(const char *cmd, const char **argv)
> error("no read/execute permissions on '%s'\n",
> sb.buf);
> else
> - warn("file '%s' exists and permissions "
> - "seem OK.\nIf this is a script, see if you "
> - "have sufficient privileges to run the "
> - "interpreter", sb.buf);
> + check_interpreter(sb.buf);
> }
>
> strbuf_release(&sb);
> diff --git a/t/t0061-run-command.sh b/t/t0061-run-command.sh
> index b39bd16..39bfaef 100755
> --- a/t/t0061-run-command.sh
> +++ b/t/t0061-run-command.sh
> @@ -13,6 +13,18 @@ cat >hello-script <<-EOF
> EOF
> >empty
>
> +cat >someinterpreter <<-EOF
> + #!$SHELL_PATH
> + cat hello-script
> +EOF
> +>empty
> +
> +cat >incorrect-interpreter-script <<-EOF
> + #!someinterpreter
> + cat hello-script
> +EOF
> +>empty
> +
> test_expect_success 'start_command reports ENOENT' '
> test-run-command start-command-ENOENT ./does-not-exist
> '
> @@ -48,4 +60,14 @@ test_expect_success POSIXPERM 'run_command reports EACCES, search path permision
> grep "no read/execute permissions on" err
> '
>
> +test_expect_success POSIXPERM 'run_command reports EACCES, interpreter fails' '
> + cat incorrect-interpreter-script >hello.sh &&
> + chmod +x hello.sh &&
> + chmod -x someinterpreter &&
> + test_must_fail test-run-command run-command ./hello.sh 2>err &&
> +
> + grep "fatal: cannot exec.*hello.sh" err &&
> + grep "bad interpreter" err
> +'
> +
> test_done
^ permalink raw reply
* Re: [PATCH v2 3/3] grep: disable threading in all but worktree case
From: René Scharfe @ 2011-12-06 22:48 UTC (permalink / raw)
To: git; +Cc: Eric Herman, git, Junio C Hamano
In-Reply-To: <4ED8F9AE.8030605@lsrfire.ath.cx>
Am 02.12.2011 17:15, schrieb René Scharfe:
> How about adding a parameter to control the number of threads
> (--threads?) instead that defaults to eight (or five) for the worktree
> and one for the rest? That would also make benchmarking easier.
Like this:
-- >8 --
Subject: grep: add parameter --threads
Allow the number of threads to be specified by the user. This makes
benchmarking the performance impact of different numbers of threads
much easier.
Move the code for thread handling after argument parsing. This allows
to change the default number of threads based on the kind of search
(worktree etc.) later on.
Signed-off-by: Rene Scharfe <rene.scharfe@lsrfire.ath.cx>
---
Applies on top of your second patch.
Documentation/git-grep.txt | 4 ++
builtin/grep.c | 75 +++++++++++++++++++++++--------------------
2 files changed, 44 insertions(+), 35 deletions(-)
diff --git a/Documentation/git-grep.txt b/Documentation/git-grep.txt
index 15d6711..47ac188 100644
--- a/Documentation/git-grep.txt
+++ b/Documentation/git-grep.txt
@@ -227,6 +227,10 @@ OPTIONS
Do not output matched lines; instead, exit with status 0 when
there is a match and with non-zero status when there isn't.
+--threads <n>::
+ Run <n> search threads in parallel. Default is 8. This option
+ is ignored if git was built without support for POSIX threads.
+
<tree>...::
Instead of searching tracked files in the working tree, search
blobs in the given trees.
diff --git a/builtin/grep.c b/builtin/grep.c
index 65b1ffe..0bda900 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -24,11 +24,10 @@ static char const * const grep_usage[] = {
NULL
};
-static int use_threads = 1;
-
#ifndef NO_PTHREADS
#define THREADS 8
-static pthread_t threads[THREADS];
+static pthread_t *threads;
+static int nr_threads = -1;
static void *load_sha1(const unsigned char *sha1, unsigned long *size,
const char *name);
@@ -76,13 +75,13 @@ static pthread_mutex_t grep_mutex;
static inline void grep_lock(void)
{
- if (use_threads)
+ if (nr_threads > 0)
pthread_mutex_lock(&grep_mutex);
}
static inline void grep_unlock(void)
{
- if (use_threads)
+ if (nr_threads > 0)
pthread_mutex_unlock(&grep_mutex);
}
@@ -91,13 +90,13 @@ static pthread_mutex_t read_sha1_mutex;
static inline void read_sha1_lock(void)
{
- if (use_threads)
+ if (nr_threads > 0)
pthread_mutex_lock(&read_sha1_mutex);
}
static inline void read_sha1_unlock(void)
{
- if (use_threads)
+ if (nr_threads > 0)
pthread_mutex_unlock(&read_sha1_mutex);
}
@@ -254,6 +253,8 @@ static void start_threads(struct grep_opt *opt)
{
int i;
+ threads = xcalloc(nr_threads, sizeof(pthread_t));
+
pthread_mutex_init(&grep_mutex, NULL);
pthread_mutex_init(&read_sha1_mutex, NULL);
pthread_mutex_init(&grep_attr_mutex, NULL);
@@ -265,7 +266,7 @@ static void start_threads(struct grep_opt *opt)
strbuf_init(&todo[i].out, 0);
}
- for (i = 0; i < ARRAY_SIZE(threads); i++) {
+ for (i = 0; i < nr_threads; i++) {
int err;
struct grep_opt *o = grep_opt_dup(opt);
o->output = strbuf_out;
@@ -296,7 +297,7 @@ static int wait_all(void)
pthread_cond_broadcast(&cond_add);
grep_unlock();
- for (i = 0; i < ARRAY_SIZE(threads); i++) {
+ for (i = 0; i < nr_threads; i++) {
void *h;
pthread_join(threads[i], &h);
hit |= (int) (intptr_t) h;
@@ -309,6 +310,8 @@ static int wait_all(void)
pthread_cond_destroy(&cond_write);
pthread_cond_destroy(&cond_result);
+ free(threads);
+
return hit;
}
#else /* !NO_PTHREADS */
@@ -410,7 +413,7 @@ static int grep_sha1(struct grep_opt *opt, const unsigned char *sha1,
name = strbuf_detach(&pathbuf, NULL);
#ifndef NO_PTHREADS
- if (use_threads) {
+ if (nr_threads > 0) {
grep_sha1_async(opt, name, sha1);
return 0;
} else
@@ -472,7 +475,7 @@ static int grep_file(struct grep_opt *opt, const char *filename)
name = strbuf_detach(&buf, NULL);
#ifndef NO_PTHREADS
- if (use_threads) {
+ if (nr_threads > 0) {
grep_file_async(opt, name, filename);
return 0;
} else
@@ -895,6 +898,13 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
PARSE_OPT_OPTARG, NULL, (intptr_t)default_pager },
OPT_BOOLEAN(0, "ext-grep", &external_grep_allowed__ignored,
"allow calling of grep(1) (ignored by this build)"),
+#ifdef NO_PTHREADS
+ OPT_INTEGER(0, "threads", &nr_threads,
+ "handle <n> files in parallel (ignored by this build)"),
+#else
+ OPT_INTEGER(0, "threads", &nr_threads,
+ "handle <n> files in parallel"),
+#endif
{ OPTION_CALLBACK, 0, "help-all", &options, NULL, "show usage",
PARSE_OPT_HIDDEN | PARSE_OPT_NOARG, help_callback },
OPT_END()
@@ -995,7 +1005,7 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
opt.output_priv = &path_list;
opt.output = append_path;
string_list_append(&path_list, show_in_pager);
- use_threads = 0;
+ nr_threads = 0;
}
if (!opt.pattern_list)
@@ -1003,28 +1013,6 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
if (!opt.fixed && opt.ignore_case)
opt.regflags |= REG_ICASE;
-#ifndef NO_PTHREADS
- if (online_cpus() == 1)
- use_threads = 0;
-#else
- use_threads = 0;
-#endif
-
- opt.use_threads = use_threads;
-
-#ifndef NO_PTHREADS
- if (use_threads) {
- if (opt.pre_context || opt.post_context || opt.file_break ||
- opt.funcbody)
- skip_first_line = 1;
- start_threads(&opt);
- }
-#else
- use_threads = 0;
-#endif
-
- compile_grep_patterns(&opt);
-
/* Check revs and then paths */
for (i = 0; i < argc; i++) {
const char *arg = argv[i];
@@ -1056,6 +1044,23 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
pathspec.max_depth = opt.max_depth;
pathspec.recursive = 1;
+#ifdef NO_PTHREADS
+ nr_threads = 0;
+#else
+ if (nr_threads == -1)
+ nr_threads = (online_cpus() > 1) ? THREADS : 0;
+
+ if (nr_threads > 0) {
+ opt.use_threads = 1;
+ if (opt.pre_context || opt.post_context || opt.file_break ||
+ opt.funcbody)
+ skip_first_line = 1;
+ start_threads(&opt);
+ }
+#endif
+
+ compile_grep_patterns(&opt);
+
if (show_in_pager && (cached || list.nr))
die(_("--open-files-in-pager only works on the worktree"));
@@ -1100,7 +1105,7 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
hit = grep_objects(&opt, &pathspec, &list);
}
- if (use_threads)
+ if (nr_threads > 0)
hit |= wait_all();
if (hit && show_in_pager)
run_pager(&opt, prefix);
--
1.7.8
^ permalink raw reply related
* Re: [PATCH 1/5] Add test-scrap-cache-tree
From: Junio C Hamano @ 2011-12-06 22:51 UTC (permalink / raw)
To: Thomas Rast; +Cc: git, Carlos Martín Nieto
In-Reply-To: <534454506f92428863de8fe0638f4129e962a073.1323191497.git.trast@student.ethz.ch>
Thomas Rast <trast@student.ethz.ch> writes:
> A simple utility that invalidates all existing cache-tree data. We
> need this for tests. (We don't need a tool to rebuild the cache-tree
> data; git read-tree HEAD works for that.)
"git read-tree -m HEAD HEAD" would work for that, I suspect ;-)
But that can be improved later, so it is very sensible to add a test tool
like this.
^ permalink raw reply
* How to make devs write better commit messages
From: Joseph Huttner @ 2011-12-06 22:55 UTC (permalink / raw)
To: git
So I know that there is a somewhat standard format of commit messages
in Git, which Linus outlines here:
https://github.com/torvalds/subsurface/blob/master/README#L164
Trouble is, when most people go to commit, the file that the editor
opens has no reminder of how to write a proper commit message. Often
I find myself having to go back through the commit log, or consulting
the above link.
I propose two things:
1. An optional flag in the Git config that, if set, shows the format
of a typical commit message in your commit message template.
2. The ability to modify this commit message template. Many teams
use automated tools to read commit messages and then do automated
tasks based on that data, like comment an RT ticket. Thus, developers
need to be reminded of these team-specific settings as well.
What are your thoughts?
The bottom line is that good commit messages are really important, so
we should make it as easy as possible for developers to go ahead and
write a perfect commit message every time they commit code.
E.g.
# Please enter the commit message for your changes. Lines starting
# with '#' will be ignored, and an empty message aborts the commit
# On branch master
# Changes to be committed:
# (use "git reset HEAD <file>..." to unstage)
#
# modified: application/views/layouts/layout.phtml
#
# Untracked files:
# (use "git add <file>..." to include in what will be committed)
#
# public/js/databases/
#
# How to properly format your commit message:
#
# Header line: explaining the commit in one line
#
# Body of commit message is a few lines of text, explaining things
# in more detail, possibly giving some background about the issue
# being fixed, etc etc.
#
# The body of the commit message can be several paragraphs, and
# please do proper word-wrap and keep columns shorter than about
# 74 characters or so. That way "git log" will show things
# nicely even when it's indented.
#
# RT: 123, 456 [a comma-separated list of RT tickets this commit refers to]
#
^ 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