* 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
* [PATCH 4/2] grep: turn off threading for non-worktree
From: René Scharfe @ 2011-12-06 23:01 UTC (permalink / raw)
To: Thomas Rast; +Cc: git, Eric Herman, Junio C Hamano
In-Reply-To: <4EDE9BBA.2010409@lsrfire.ath.cx>
Reading of git objects needs to be protected by an exclusive lock
and cannot be parallelized. Searching the read buffers can be done
in parallel, but for simple expressions threading is a net loss due
to its overhead, as measured by Thomas. Turn it off unless we're
searching in the worktree.
Once the object store can be read safely by multiple threads in
parallel this patch should be reverted.
Signed-off-by: Rene Scharfe <rene.scharfe@lsrfire.ath.cx>
---
Goes on top of my earlier patch. Could use a better commit message
with your (cleaned up) performance numbers.
Documentation/git-grep.txt | 5 +++--
builtin/grep.c | 2 +-
2 files changed, 4 insertions(+), 3 deletions(-)
diff --git a/Documentation/git-grep.txt b/Documentation/git-grep.txt
index 47ac188..e981a9b 100644
--- a/Documentation/git-grep.txt
+++ b/Documentation/git-grep.txt
@@ -228,8 +228,9 @@ OPTIONS
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.
+ Run <n> search threads in parallel. Default is 8 when searching
+ the worktree and 0 otherwise. This option is ignored if git was
+ built without support for POSIX threads.
<tree>...::
Instead of searching tracked files in the working tree, search
diff --git a/builtin/grep.c b/builtin/grep.c
index 0bda900..f698642 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -1048,7 +1048,7 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
nr_threads = 0;
#else
if (nr_threads == -1)
- nr_threads = (online_cpus() > 1) ? THREADS : 0;
+ nr_threads = (online_cpus() > 1 && !list.nr) ? THREADS : 0;
if (nr_threads > 0) {
opt.use_threads = 1;
--
1.7.8
^ permalink raw reply related
* Re: [PATCH 5/5] reset: update cache-tree data when appropriate
From: Junio C Hamano @ 2011-12-06 23:13 UTC (permalink / raw)
To: Thomas Rast; +Cc: git, Carlos Martín Nieto
In-Reply-To: <1385c10084ae41ae4543ef3ccaa1d6c8182b2204.1323191497.git.trast@student.ethz.ch>
Thomas Rast <trast@student.ethz.ch> writes:
> In the case of --mixed and --hard, we throw away the old index and
> rebuild everything from the tree argument (or HEAD). So we have an
> opportunity here to fill in the cache-tree data, just as read-tree
> did.
>
> Signed-off-by: Thomas Rast <trast@student.ethz.ch>
> ---
> builtin/reset.c | 7 +++++++
> t/t0090-cache-tree.sh | 4 ++--
> 2 files changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/builtin/reset.c b/builtin/reset.c
> index 811e8e2..8c2c1d5 100644
> --- a/builtin/reset.c
> +++ b/builtin/reset.c
> @@ -43,6 +43,7 @@ static int reset_index_file(const unsigned char *sha1, int reset_type, int quiet
> int nr = 1;
> int newfd;
> struct tree_desc desc[2];
> + struct tree *tree;
> struct unpack_trees_options opts;
> struct lock_file *lock = xcalloc(1, sizeof(struct lock_file));
>
> @@ -84,6 +85,12 @@ static int reset_index_file(const unsigned char *sha1, int reset_type, int quiet
> return error(_("Failed to find tree of %s."), sha1_to_hex(sha1));
> if (unpack_trees(nr, desc, &opts))
> return -1;
> +
> + if (reset_type == MIXED || reset_type == HARD) {
> + tree = parse_tree_indirect(sha1);
> + prime_cache_tree(&active_cache_tree, tree);
> + }
The basic idea that MIXED or HARD should result in a cache-tree that match
the tree we just read is sound, but how expensive is prime_cache_tree()? I
think it reads the same tree once again. Admittedly, the data needed to
reconstruct the tree is likely to be hot in core, but it may be necessary
to measure before deciding if this is a good change.
^ permalink raw reply
* Re: How to make devs write better commit messages
From: Michael Schubert @ 2011-12-06 23:18 UTC (permalink / raw)
To: Joseph Huttner; +Cc: git
In-Reply-To: <CAOJsP-X0ZWT5HLHcBc2FmhoMpWFOvEFADiM9jGZ9R1ctqHDJ9w@mail.gmail.com>
On 12/06/2011 11:55 PM, Joseph Huttner wrote:
> 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?
If it's no social issue but just due to lack of a reminder you
could provide a template for commit.template. Either way: you
still would have to force people to set it.?
^ permalink raw reply
* Re: [PATCHv2 06/13] credential: apply helper config
From: Junio C Hamano @ 2011-12-06 23:58 UTC (permalink / raw)
To: Jeff King; +Cc: git
In-Reply-To: <20111206062247.GF29233@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> diff --git a/t/t0300-credentials.sh b/t/t0300-credentials.sh
> index 81a455f..e3f61f4 100755
> --- a/t/t0300-credentials.sh
> +++ b/t/t0300-credentials.sh
> @@ -192,4 +192,46 @@ test_expect_success 'internal getpass does not ask for known username' '
> EOF
> '
>
> +HELPER="f() {
> + cat >/dev/null
> + echo username=foo
> + echo password=bar
> + }; f"
> +test_expect_success 'respect configured credentials' '
> + test_config credential.helper "$HELPER" &&
> + check fill <<-\EOF
> + --
> + username=foo
> + password=bar
> + --
> + EOF
> +'
Hmm, why do I get ask-ass-{username,password} from this one?
> +test_expect_success 'match configured credential' '
> + test_config credential.https://example.com.helper "$HELPER" &&
> + check fill <<-\EOF
> + protocol=https
> + host=example.com
> + path=repo.git
> + --
> + username=foo
> + password=bar
> + --
> + EOF
> +'
And this one, too...
^ permalink raw reply
* Re: How to make devs write better commit messages
From: Hilco Wijbenga @ 2011-12-07 0:08 UTC (permalink / raw)
To: Joseph Huttner; +Cc: git, Michael Schubert
In-Reply-To: <CAOJsP-X0ZWT5HLHcBc2FmhoMpWFOvEFADiM9jGZ9R1ctqHDJ9w@mail.gmail.com>
On 6 December 2011 14:55, Joseph Huttner <huttnified@gmail.com> wrote:
> 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?
Great idea! These templates would be stored in the Git repo, I assume?
Btw, there is 'commit.template' which you can use locally.
I was wondering if it might be possible to somehow add project config
defaults to one's Git repo. It would be great to have something like
'commit.template' point to a file in the Git repo by default.
Currently, it doesn't seem possible to have a config parameter "point
to" a file or directory in the Git repo. Nor do I know of a way to
have the Git repo set a config parameter to a default value. Or is
this possible after all?
^ permalink raw reply
* Re: [RFD] Handling of non-UTF8 data in gitweb
From: Junio C Hamano @ 2011-12-07 0:37 UTC (permalink / raw)
To: Jakub Narebski; +Cc: git, Jürgen Kreileder, John Hawley
In-Reply-To: <201112041709.32212.jnareb@gmail.com>
Jakub Narebski <jnareb@gmail.com> writes:
> But doing this would change gitweb behavior. Currently when
> encountering something (usually line of output) that is not valid
> UTF-8, we decode it (to UTF-8) using $fallback_encoding, by default
> 'latin1'. Note however that this value is per gitweb installation,
> not per repository.
I think we added and you acked 00f429a (gitweb: Handle non UTF-8 text
better, 2007-06-03) for a good reason, and I think the above argues that
whatever issue the commit tried to address is a non-issue. Is it really
true?
> ... I guess
> it could be emulated by defining our own 'utf-8-with-fallback'
> encoding, or by defining our own PerlIO layer with PerlIO::via.
> But it no longer be simple solution (though still automatic).
Between the current "everybody who read from the input must remember to
call to_utf8" and "everybody gets to read utf8 automatically for internal
processing", even though the latter may involve one-time pain to set up
the framework to do so, the pros-and-cons feels obvious to me.
^ permalink raw reply
* Re: [PATCHv2 06/13] credential: apply helper config
From: Jeff King @ 2011-12-07 0:45 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <7vsjkxckwk.fsf@alter.siamese.dyndns.org>
On Tue, Dec 06, 2011 at 03:58:35PM -0800, Junio C Hamano wrote:
> > +test_expect_success 'respect configured credentials' '
> > + test_config credential.helper "$HELPER" &&
> > + check fill <<-\EOF
> > + --
> > + username=foo
> > + password=bar
> > + --
> > + EOF
> > +'
>
> Hmm, why do I get ask-ass-{username,password} from this one?
Ugh. Because apparently one of my re-roll tweaks from patch 03 regressed
this. Sorry, I should have been more careful about running the full
suite, not just the tests in the commits I tweaked.
Let me investigate and get back to you.
-Peff
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox