* Re: [PATCHv2 0/13] credential helpers
From: Junio C Hamano @ 2011-12-08 21:34 UTC (permalink / raw)
To: Jeff King; +Cc: git
In-Reply-To: <20111207064231.GA499@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> Because the pattern takes 0 or more lines and no terminator, we can't
> distinguish between empty or truncated input and the empty pattern.
I agree that such a positive "Ok here is the end of specification" marker
is a good idea, even if we do not worry about "an empty set".
When the requestor wants to specify the credentials with host and user,
but the wire is cut after host is communicated but before user is, we do
want to notice the communication error, instead of silently erasing all
the credentials on the host regardless of the user.
Thanks.
^ permalink raw reply
* Re: [PATCH 1/2] run-command: Add checks after execvp fails with EACCES
From: Frans Klaver @ 2011-12-08 21:44 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <7vpqg1e3au.fsf@alter.siamese.dyndns.org>
On Tue, 06 Dec 2011 23:35:53 +0100, Junio C Hamano <gitster@pobox.com>
wrote:
> 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 just had a good look through the man page of access(2), and I think it
depends. access works for the real uid, which is what I attempted to
implement in the above check as well. However, do we actually need to use
the real uid or do we need the set uid (geteuid(2))? Would it be safe to
assume we don't setuid?
> 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.
Will fix if we can't use access(2) due to what I mentioned above.
>> + 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?
Looking back on it, I think I actually wanted to use warning() from
usage.c. I'll still have to check if that does the multi-line thing as I
expect it to.
^ permalink raw reply
* Re: [PATCH 5/7] add generic terminal prompt function
From: Jakub Narebski @ 2011-12-08 21:48 UTC (permalink / raw)
To: Jeff King; +Cc: git, Erik Faye-Lund, Junio C Hamano
In-Reply-To: <20111208083323.GE26409@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> diff --git a/Makefile b/Makefile
> index b024e2f..2a9bb3d 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -227,6 +227,9 @@ all::
> +# Define HAVE_DEV_TTY if your system can open /dev/tty to interact with the
> +# user.
> @@ -833,6 +838,7 @@ ifeq ($(uname_S),Linux)
> NO_STRLCPY = YesPlease
> NO_MKSTEMPS = YesPlease
> HAVE_PATHS_H = YesPlease
> + HAVE_DEV_TTY = YesPlease
> endif
Here you use HAVE_DEV_TTY (by the way, I wonder if it could be
automatically detected by ./configure script)...
> diff --git a/compat/terminal.c b/compat/terminal.c
> new file mode 100644
> index 0000000..49f16ca
> --- /dev/null
> +++ b/compat/terminal.c
> @@ -0,0 +1,81 @@
> +#include "git-compat-util.h"
> +#include "compat/terminal.h"
> +#include "sigchain.h"
> +#include "strbuf.h"
> +
> +#ifndef NO_DEV_TTY
> +#endif
+#endif /* NO_DEV_TTY */
...and here you have NO_DEV_TTY
--
Jakub Narębski
^ permalink raw reply
* Re: [PATCH 5/7] add generic terminal prompt function
From: Jeff King @ 2011-12-08 21:52 UTC (permalink / raw)
To: Jakub Narebski; +Cc: git, Erik Faye-Lund, Junio C Hamano
In-Reply-To: <m38vmmivkc.fsf@localhost.localdomain>
On Thu, Dec 08, 2011 at 01:48:33PM -0800, Jakub Narebski wrote:
> > @@ -833,6 +838,7 @@ ifeq ($(uname_S),Linux)
> > NO_STRLCPY = YesPlease
> > NO_MKSTEMPS = YesPlease
> > HAVE_PATHS_H = YesPlease
> > + HAVE_DEV_TTY = YesPlease
> > endif
>
> Here you use HAVE_DEV_TTY (by the way, I wonder if it could be
> automatically detected by ./configure script)...
> [...]
> ...and here you have NO_DEV_TTY
Whoops. Thanks for catching. I converted it to NO_DEV_TTY, which would
turn this code on by default (because I think _most_ platforms we use
are going to want this), but then I decided to go the conservative route
and let platforms opt into it.
And of course since my platform is the one that enables it, I didn't
notice during my testing.
I'll fix it for the next re-roll.
-Peff
^ permalink raw reply
* Re: [PATCH 2/2] bundle: rewrite builtin to use parse-options
From: Junio C Hamano @ 2011-12-08 23:48 UTC (permalink / raw)
To: Ramkumar Ramachandra; +Cc: Jonathan Nieder, Matthieu Moy, Git List
In-Reply-To: <CALkWK0nCuFgS8PKkQLMzqpBsOWouSs5y=CEKS1r0x0=LkhqC9A@mail.gmail.com>
Ramkumar Ramachandra <artagnon@gmail.com> writes:
> That being said, do you see value in lifting the restriction on
> opts->long_name and PARSE_OPTS_NODASH not allowed together? The
> restriction seems quite arbitrary, but I can't justify lifting it
> unless I can show some valid usecase.
True and true.
As to the first "true", it is because the nodash was introduced only to
parse "find ... \( ... \)" style parentheses as if they are part of option
sequence, and that use case only needed a single letter.
As to the second "true", it is because so far we didn't need anything
longer.
I do not think the name of a subcommand is not a good use case example for
it, by the way. Unlike parentheses on the command line of "find" that can
come anywhere and there can be more than one, the subcommand must be the
first thing on the command line and only one subcommand is given at one
command invocation.
^ permalink raw reply
* Re: [PATCH 5/6] t1510 (worktree): fix '&&' chaining
From: Junio C Hamano @ 2011-12-09 0:00 UTC (permalink / raw)
To: Jonathan Nieder; +Cc: Ramkumar Ramachandra, Matthieu Moy, Git List
In-Reply-To: <20111208171213.GF2394@elie.hsd1.il.comcast.net>
Jonathan Nieder <jrnieder@gmail.com> writes:
> This is also not "Additionally", as in "As a separate change that
> maybe should have been another patch but I am too lazy". Rather, it
> is a necessary change that is part of the same task. So I would
> write:
>
> Breaks in a test assertion's && chain can potentially hide failures
> from earlier commands in the chain. Fix these breaks.
>
> 'unset' returns non-zero status when the variable passed was already
> unset on some shells, so now that the status is tested we need to
> change these instances to 'safe_unset'.
>
> Erm, sane_unset, not safe_unset. Did you even test this?
Thanks for careful reading and many constructive review comments.
^ permalink raw reply
* Re: Debugging git-commit slowness on a large repo
From: Joshua Redstone @ 2011-12-09 0:09 UTC (permalink / raw)
To: Nguyen Thai Ngoc Duy
Cc: Carlos Martín Nieto, Tomas Carnecky, Junio C Hamano,
git@vger.kernel.org
In-Reply-To: <CACsJy8DiWWr7eo86gzb-XcqfDv4_ENkqWxswTNb-k84xO18c=A@mail.gmail.com>
On 12/7/11 5:39 PM, "Nguyen Thai Ngoc Duy" <pclouds@gmail.com> wrote:
>On Thu, Dec 8, 2011 at 5:48 AM, Joshua Redstone <joshua.redstone@fb.com>
>wrote:
>> Hi Duy,
>> Thanks for the documentation link.
>>
>> git ls-files shows 100k files, which matches # of files in the working
>> tree ('find . -type f -print | wc -l').
>
>Any chance you can split it into smaller repositories, or remove files
>from working directory (e.g. if you store logs, you don't have to keep
>logs from all time in working directory, they can be retrieved from
>history).
It's not really feasible to split it into smaller repositories. In fact,
we're expecting it to grow between 3x and 5x in number of files and number
of commits.
>
>> I added a 'git read-tree HEAD' before the git-add, and a 'git
>>write-tree'
>> after the add. With that, the commit time slowed down to 8 seconds per
>> commit, plus 4 more seconds for the read-tree/add/write-tree ops. The
>> read-tree/add/write-tree each took about a second.
>
>read-tree destroys stat info in index, refreshing 100k entries in
>index in this case may take some time. Try this to see if commit time
>reduces and how much time update-index takes
>
>read-tree HEAD
>update-index --refresh
>add ....
>write-tree
>commit -q
I added the "update-index --refresh" and the time for commit became more
like 0.6 seconds.
In this setup: read-tree takes ~2 seconds, update-index takes ~8 seconds,
git-add takes 1 to 4 seconds, and write-tree takes less than 1 second.
>
>> As an experiment, I also tried removing the 'git read-tree' and just
>> having the git-write-tree. That sped up commits to 0.6 seconds, but the
>> overall time for add/write-tree/commit was still 3 to 6 seconds.
>
>overall time is not really important because we duplicate work here
>(write-tree is done as part of commit again). What I'm trying to do is
>to determine how much time each operation in commit may take.
>--
>Duy
^ permalink raw reply
* Re: Debugging git-commit slowness on a large repo
From: Joshua Redstone @ 2011-12-09 0:17 UTC (permalink / raw)
To: Nguyen Thai Ngoc Duy
Cc: Carlos Martín Nieto, Tomas Carnecky, Junio C Hamano,
git@vger.kernel.org
In-Reply-To: <CB069000.2C9C6%joshua.redstone@fb.com>
Btw, I also tried doing some very poor-man's profiling on "git commit"
without any of the readtree/writetree/updateindex commands.
Around 50% of the time was in (bottom few frames may have varied)
#1 0x00000000004c467e in find_pack_entry (sha1=0x1475a44 ,
e=0x7fff2621f070) at sha1_file.c:2027
#2 0x00000000004c57b0 in has_sha1_file (sha1=0x7fe2cd9c7900 "00") at
sha1_file.c:2567
#3 0x000000000046e4af in update_one (it=<value optimized out>,
cache=<value optimized out>, entries=<value optimized out>, base=<value
optimized out>, baselen=<value optimized out>, missing_ok=<value optimized
out>, dryrun=0) at cache-\
tree.c:333
#4 0x000000000046e278 in update_one (it=<value optimized out>,
cache=<value optimized out>, entries=<value optimized out>, base=<value
optimized out>, baselen=<value optimized out>, missing_ok=<value optimized
out>, dryrun=0) at cache-\
tree.c:285
#5 0x000000000046e278 in update_one (it=<value optimized out>,
cache=<value optimized out>, entries=<value optimized out>, base=<value
optimized out>, baselen=<value optimized out>, missing_ok=<value optimized
out>, dryrun=0) at cache-\
tree.c:285
#6 0x000000000046e278 in update_one (it=<value optimized out>,
cache=<value optimized out>, entries=<value optimized out>, base=<value
optimized out>, baselen=<value optimized out>, missing_ok=<value optimized
out>, dryrun=0) at cache-\
tree.c:285
#7 0x000000000046e278 in update_one (it=<value optimized out>,
cache=<value optimized out>, entries=<value optimized out>, base=<value
optimized out>, baselen=<value optimized out>, missing_ok=<value optimized
out>, dryrun=0) at cache-\
tree.c:285
#8 0x000000000046e278 in update_one (it=<value optimized out>,
cache=<value optimized out>, entries=<value optimized out>, base=<value
optimized out>, baselen=<value optimized out>, missing_ok=<value optimized
out>, dryrun=0) at cache-\
tree.c:285
#9 0x000000000046e869 in cache_tree_update (it=<value optimized out>,
cache=<value optimized out>, entries=dwarf2_read_address: Corrupted DWARF
expression.
) at cache-tree.c:379
#10 0x000000000041cade in prepare_to_commit (index_file=0x781740
".git/index", prefix=<value optimized out>, current_head=<value optimized
out>, s=0x7fff26220d00, author_ident=<value optimized out>) at
builtin/commit.c:866
#11 0x000000000041d891 in cmd_commit (argc=0, argv=0x7fff262213a0,
prefix=0x0) at builtin/commit.c:1407
#12 0x0000000000404bf7 in handle_internal_command (argc=4,
argv=0x7fff262213a0) at git.c:308
#13 0x0000000000404e2f in main (argc=4, argv=0x7fff262213a0) at git.c:512
And 30% of the time was in:
#0 0x00000034af2c34a5 in _lxstat () from /lib64/libc.so.6
#1 0x00000000004abe0f in refresh_cache_ent (istate=0x780940,
ce=0x7f8462a34e40, options=0, err=0x7fff6dd9f588) at
/usr/include/sys/stat.h:443
#2 0x00000000004ac1a0 in refresh_index (istate=0x780940, flags=<value
optimized out>, pathspec=<value optimized out>, seen=<value optimized
out>, header_msg=0x0) at read-cache.c:1133
#3 0x000000000041b60a in refresh_cache_or_die (refresh_flags=<value
optimized out>) at builtin/commit.c:331
#4 0x000000000041bc39 in prepare_index (argc=0, argv=0x7fff6dda0310,
prefix=0x0, current_head=<value optimized out>, is_status=<value optimized
out>) at builtin/commit.c:414
#5 0x000000000041d878 in cmd_commit (argc=0, argv=0x7fff6dda0310,
prefix=0x0) at builtin/commit.c:1403
Josh
On 12/8/11 4:09 PM, "Joshua Redstone" <joshua.redstone@fb.com> wrote:
>On 12/7/11 5:39 PM, "Nguyen Thai Ngoc Duy" <pclouds@gmail.com> wrote:
>
>>On Thu, Dec 8, 2011 at 5:48 AM, Joshua Redstone <joshua.redstone@fb.com>
>>wrote:
>>> Hi Duy,
>>> Thanks for the documentation link.
>>>
>>> git ls-files shows 100k files, which matches # of files in the working
>>> tree ('find . -type f -print | wc -l').
>>
>>Any chance you can split it into smaller repositories, or remove files
>>from working directory (e.g. if you store logs, you don't have to keep
>>logs from all time in working directory, they can be retrieved from
>>history).
>
>It's not really feasible to split it into smaller repositories. In fact,
>we're expecting it to grow between 3x and 5x in number of files and number
>of commits.
>
>>
>>> I added a 'git read-tree HEAD' before the git-add, and a 'git
>>>write-tree'
>>> after the add. With that, the commit time slowed down to 8 seconds per
>>> commit, plus 4 more seconds for the read-tree/add/write-tree ops. The
>>> read-tree/add/write-tree each took about a second.
>>
>>read-tree destroys stat info in index, refreshing 100k entries in
>>index in this case may take some time. Try this to see if commit time
>>reduces and how much time update-index takes
>>
>>read-tree HEAD
>>update-index --refresh
>>add ....
>>write-tree
>>commit -q
>
>I added the "update-index --refresh" and the time for commit became more
>like 0.6 seconds.
>In this setup: read-tree takes ~2 seconds, update-index takes ~8 seconds,
>git-add takes 1 to 4 seconds, and write-tree takes less than 1 second.
>
>>
>>> As an experiment, I also tried removing the 'git read-tree' and just
>>> having the git-write-tree. That sped up commits to 0.6 seconds, but
>>>the
>>> overall time for add/write-tree/commit was still 3 to 6 seconds.
>>
>>overall time is not really important because we duplicate work here
>>(write-tree is done as part of commit again). What I'm trying to do is
>>to determine how much time each operation in commit may take.
>>--
>>Duy
>
^ permalink raw reply
* Question about commit message wrapping
From: Sidney San Martín @ 2011-12-09 1:59 UTC (permalink / raw)
To: git
Hey, I want to ask about the practice of wrapping commit messages to 70-something charaters.
The webpage most cited about it, which I otherwise really like, is
http://tbaggery.com/2008/04/19/a-note-about-git-commit-messages.html
*Nothing else* in my everyday life works this way anymore. Line wrapping gets done on the display end in my email client, my web browser, my ebook reader entirely automatically, and it adapts to the size of the window.
That article gives two reasons why commits should be wrapped to 72 columns. First:
> git log doesn’t do any special special wrapping of the commit messages. With the default pager of less -S, this means your paragraphs flow far off the edge of the screen, making them difficult to read. On an 80 column terminal, if we subtract 4 columns for the indent on the left and 4 more for symmetry on the right, we’re left with 72 columns.
Here, I put a patch at the bottom of this email that wraps commit messages to, right now, 80 columns when they're displayed. (It’s a quick one, probably needs configurability. Also, beware, I don’t program in C too much.)
Second:
> git format-patch --stdout converts a series of commits to a series of emails, using the messages for the message body. Good email netiquette dictates we wrap our plain text emails such that there’s room for a few levels of nested reply indicators without overflow in an 80 column terminal. (The current rails.git workflow doesn’t include email, but who knows what the future will bring.)
There's been a standard for flowed plain text emails (which don't have to wrap at 80 columns) for well over ten years, RFC-2646 and is widely supported. Besides, code in diffs is often longer than 7x characters, and wrapping, like `git log`, could be done inside git. FWIW, there are a bunch of merge commits with lines longer than 80 characters in the git repo itself.
Finally, people read commits these days in many other places than `git log` (and make commits in many other places than a text editor configured to wrap). Most every GUI and already word wraps commit messages just fine. As a result, there are commits in popular repos much longer than the 72-column standard and no one notices. Instead, properly-formatted commit messages end up looking cramped when you see them in anywhere wider than 80 columns.
Am I crazy? If this makes sense to anyone else, I'd be happy to help massage this into something git-worthy, with some help (never worked on Git before).
- - -
From a93b390d1506652d4ad41d1cbd987ba98a8deca0 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Sidney=20San=20Marti=CC=81n?= <s@sidneysm.com>
Date: Thu, 8 Dec 2011 20:26:23 -0500
Subject: [PATCH] Wrap commit messages on display
- Wrap to 80 characters minus the indent
- Use a hanging indent for lines which begin with "- "
- Do not wrap lines which begin with whitespace
---
pretty.c | 10 ++++++++--
1 files changed, 8 insertions(+), 2 deletions(-)
diff --git a/pretty.c b/pretty.c
index 230fe1c..15804ce 100644
--- a/pretty.c
+++ b/pretty.c
@@ -1243,8 +1243,14 @@ void pp_remainder(const struct pretty_print_context *pp,
memset(sb->buf + sb->len, ' ', indent);
strbuf_setlen(sb, sb->len + indent);
}
- strbuf_add(sb, line, linelen);
- strbuf_addch(sb, '\n');
+ if (line[0] == ' ' || line[0] == '\t') {
+ strbuf_add(sb, line, linelen);
+ } else {
+ struct strbuf wrapped = STRBUF_INIT;
+ strbuf_add(&wrapped, line, linelen);
+ strbuf_add_wrapped_text(sb, wrapped.buf, 0, indent + (line[0] == '-' && line[1] == ' ' ? 2 : 0), 80 - indent);
+ strbuf_addch(sb, '\n');
+ }
}
}
--
1.7.8
^ permalink raw reply related
* Re: [PATCHv2 0/13] credential helpers
From: Jeff King @ 2011-12-09 2:29 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <7vmxb2hhne.fsf@alter.siamese.dyndns.org>
On Thu, Dec 08, 2011 at 01:34:29PM -0800, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
>
> > Because the pattern takes 0 or more lines and no terminator, we can't
> > distinguish between empty or truncated input and the empty pattern.
>
> I agree that such a positive "Ok here is the end of specification" marker
> is a good idea, even if we do not worry about "an empty set".
>
> When the requestor wants to specify the credentials with host and user,
> but the wire is cut after host is communicated but before user is, we do
> want to notice the communication error, instead of silently erasing all
> the credentials on the host regardless of the user.
OK, I've tweaked the series to require an end-of-credential marker (a
blank line) both in input and output.
In addition, I've changed the code that runs helpers to make reading
from the helpers an all-or-nothing thing (instead of incrementally
ovewriting our credential as we read from it). Before, if a helper
exited with error, we would happily use its partial result. Instead, we
now read its response into a holding area, and only copy it into our
credential when we get a successful exit code. This lets us detect
truncation when reading from the helper, too.
It works, and it detects truncated output both ways properly (I know
because I had to update every test, since the old output was missing the
end-of-credential marker).
It makes me a little sad, because the original format (relying on EOF)
was so Unix-y. You could make a helper like this:
echo password=`gpg -qd ~/.secret.gpg`
and now you must remember to tack an extra "echo" at the end. Not a big
deal, but it somehow just feels less elegant to my gut. OTOH, classic
Unix constructs have always been a nightmare for robustness and error
checking[1], so this is certainly nothing new.
The diff from this tip to the old tip is below to give you a sense of
the magnitude of the change (the individual changes are squashed into
their respective patches for the next re-roll, of course). I'll hold off
on posting the whole series to see if we get any more comments.
-Peff
[1] I mean things like:
grep foo bar | sed 's/some/transformation/'
where we totally ignore errors from grep, and where a truncated
output on the pipe would just subtly generate wrong answers.
---
Documentation/technical/api-credentials.txt | 2 +-
credential-cache--daemon.c | 1 +
credential-cache.c | 2 +
credential-store.c | 1 +
credential.c | 39 +++++++++++++++++++++++---
t/lib-credential.sh | 1 +
t/t0300-credentials.sh | 3 ++
t/t5550-http-fetch.sh | 1 +
8 files changed, 44 insertions(+), 6 deletions(-)
diff --git a/Documentation/technical/api-credentials.txt b/Documentation/technical/api-credentials.txt
index 21ca6a2..0aac899 100644
--- a/Documentation/technical/api-credentials.txt
+++ b/Documentation/technical/api-credentials.txt
@@ -199,7 +199,7 @@ followed by a newline. The key may contain any bytes except `=`,
newline, or NUL. The value may contain any bytes except newline or NUL.
In both cases, all bytes are treated as-is (i.e., there is no quoting,
and one cannot transmit a value with newline or NUL in it). The list of
-attributes is terminated by a blank line or end-of-file.
+attributes is terminated by a blank line.
Git will send the following attributes (but may not send all of
them for a given credential; for example, a `host` attribute makes no
diff --git a/credential-cache--daemon.c b/credential-cache--daemon.c
index 390f194..38403645 100644
--- a/credential-cache--daemon.c
+++ b/credential-cache--daemon.c
@@ -138,6 +138,7 @@ static void serve_one_client(FILE *in, FILE *out)
if (e) {
fprintf(out, "username=%s\n", e->item.username);
fprintf(out, "password=%s\n", e->item.password);
+ fprintf(out, "\n");
}
}
else if (!strcmp(action.buf, "exit"))
diff --git a/credential-cache.c b/credential-cache.c
index dc98372..5b8d8c9 100644
--- a/credential-cache.c
+++ b/credential-cache.c
@@ -70,6 +70,8 @@ static void do_cache(const char *socket, const char *action, int timeout,
if (strbuf_read(&buf, 0, 0) < 0)
die_errno("unable to relay credential");
}
+ else
+ strbuf_addch(&buf, '\n');
if (!send_request(socket, &buf))
return;
diff --git a/credential-store.c b/credential-store.c
index ed58768..00e38f0 100644
--- a/credential-store.c
+++ b/credential-store.c
@@ -43,6 +43,7 @@ static void print_entry(struct credential *c)
{
printf("username=%s\n", c->username);
printf("password=%s\n", c->password);
+ printf("\n");
}
static void print_line(struct strbuf *buf)
diff --git a/credential.c b/credential.c
index a17eafe..6d2a37d 100644
--- a/credential.c
+++ b/credential.c
@@ -147,8 +147,10 @@ int credential_read(struct credential *c, FILE *fp)
char *key = line.buf;
char *value = strchr(key, '=');
- if (!line.len)
- break;
+ if (!line.len) {
+ strbuf_release(&line);
+ return 0;
+ }
if (!value) {
warning("invalid credential line: %s", key);
@@ -181,7 +183,7 @@ int credential_read(struct credential *c, FILE *fp)
}
strbuf_release(&line);
- return 0;
+ return -1;
}
static void credential_write_item(FILE *fp, const char *key, const char *value)
@@ -198,6 +200,26 @@ static void credential_write(const struct credential *c, FILE *fp)
credential_write_item(fp, "path", c->path);
credential_write_item(fp, "username", c->username);
credential_write_item(fp, "password", c->password);
+ putc('\n', fp);
+}
+
+static void credential_merge_one(char **dst, char **src)
+{
+ if (!*src)
+ return;
+ free(*dst);
+ *dst = *src;
+ *src = NULL;
+}
+
+static void credential_merge(struct credential *dst,
+ struct credential *src)
+{
+ credential_merge_one(&dst->protocol, &src->protocol);
+ credential_merge_one(&dst->host, &src->host);
+ credential_merge_one(&dst->path, &src->path);
+ credential_merge_one(&dst->username, &src->username);
+ credential_merge_one(&dst->password, &src->password);
}
static int run_credential_helper(struct credential *c,
@@ -206,6 +228,7 @@ static int run_credential_helper(struct credential *c,
{
struct child_process helper;
const char *argv[] = { NULL, NULL };
+ struct credential response = CREDENTIAL_INIT;
FILE *fp;
memset(&helper, 0, sizeof(helper));
@@ -227,17 +250,23 @@ static int run_credential_helper(struct credential *c,
if (want_output) {
int r;
+
fp = xfdopen(helper.out, "r");
- r = credential_read(c, fp);
+ r = credential_read(&response, fp);
fclose(fp);
if (r < 0) {
+ credential_clear(&response);
finish_command(&helper);
return -1;
}
}
- if (finish_command(&helper))
+ if (finish_command(&helper)) {
+ credential_clear(&response);
return -1;
+ }
+
+ credential_merge(c, &response);
return 0;
}
diff --git a/t/lib-credential.sh b/t/lib-credential.sh
index fc34447..c0de4e9 100755
--- a/t/lib-credential.sh
+++ b/t/lib-credential.sh
@@ -5,6 +5,7 @@
# separated by "--".
check() {
read_chunk >stdin &&
+ echo >>stdin &&
read_chunk >expect-stdout &&
read_chunk >expect-stderr &&
test-credential "$@" <stdin >stdout 2>stderr &&
diff --git a/t/t0300-credentials.sh b/t/t0300-credentials.sh
index 885af8f..f0e77dc 100755
--- a/t/t0300-credentials.sh
+++ b/t/t0300-credentials.sh
@@ -9,6 +9,7 @@ test_expect_success 'setup helper scripts' '
whoami=`echo $0 | sed s/.*git-credential-//`
echo >&2 "$whoami: $*"
while IFS== read key value; do
+ test -z "$key" && break
echo >&2 "$whoami: $key=$value"
eval "$key=$value"
done
@@ -28,6 +29,7 @@ test_expect_success 'setup helper scripts' '
. ./dump
test -z "$user" || echo username=$user
test -z "$pass" || echo password=$pass
+ echo
EOF
chmod +x git-credential-verbatim &&
@@ -196,6 +198,7 @@ HELPER="!f() {
cat >/dev/null
echo username=foo
echo password=bar
+ echo
}; f"
test_expect_success 'respect configured credentials' '
test_config credential.helper "$HELPER" &&
diff --git a/t/t5550-http-fetch.sh b/t/t5550-http-fetch.sh
index 95a133d..b817c69 100755
--- a/t/t5550-http-fetch.sh
+++ b/t/t5550-http-fetch.sh
@@ -106,6 +106,7 @@ test_expect_success 'http auth respects credential helper config' '
cat >/dev/null
echo username=user@host
echo password=user@host
+ echo
}; f" &&
>askpass-query &&
echo wrong >askpass-response &&
^ permalink raw reply related
* Re: Query on git commit amend
From: Viresh Kumar @ 2011-12-09 4:49 UTC (permalink / raw)
To: Junio C Hamano
Cc: Björn Steinbrink, Vijay Lakshminarayanan,
git@vger.kernel.org, Shiraz HASHIM, Vipin KUMAR
In-Reply-To: <7vvcprar3v.fsf@alter.siamese.dyndns.org>
On 12/8/2011 11:22 PM, Junio C Hamano wrote:
> So by saying "--amend -C HEAD" you are saying "I want to reuse the log
> message of the commit I am amending,... eh, scratch that, I instead want
> to use the log message of the HEAD commit", as if the commit you are
> amending and HEAD are two different things. That is idiotic.
>
> Of course, if "git commit --amend" honoured "--no-edit", that is even more
> direct, straightforward and intuitive way to say so ;-)
Got your point. That's correct.
--
viresh
^ permalink raw reply
* Re: [PATCH v2 0/6] Fix '&&' chaining in tests
From: Ramkumar Ramachandra @ 2011-12-09 5:23 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jonathan Nieder, Matthieu Moy, Git List
In-Reply-To: <7vzkf2hm94.fsf@alter.siamese.dyndns.org>
Hi,
Junio C Hamano wrote:
> Ramkumar Ramachandra <artagnon@gmail.com> writes:
>
>> This follows-up $gmane/186481.
>
> I take that you meant "replaces". It was confusing especially because you
> seem to have included a few unrelated patches in the thread.
Yeah. Sorry- stray files were lying around from the previous 'git
format-patch' invocation. Which brings me to: I wonder if it makes
sense to (optionally) check that the directory is empty when executing
'git format-patch -o <dir>'. I sometimes forget to do it by hand.
-- Ram
^ permalink raw reply
* Re: [PATCH 4/5] revert: allow mixed pick and revert instructions
From: Ramkumar Ramachandra @ 2011-12-09 5:34 UTC (permalink / raw)
To: Jonathan Nieder; +Cc: Junio C Hamano, Git List
In-Reply-To: <20111207074501.GE11737@elie.hsd1.il.comcast.net>
Hi,
Jonathan Nieder wrote:
> Ramkumar Ramachandra wrote:
>
>> This patch lays the foundation for extending the parser to support
>> more actions so 'git rebase -i' can reuse this machinery in the
>> future. While at it, also improve the error messages reported by the
>> insn sheet parser.
>
> I don't know what this part is about...
I'll separate out these changes in the re-roll. Thanks.
>> @@ -733,37 +739,29 @@ static struct commit *parse_insn_line(char *bol, char *eol, struct replay_opts *
> [...]
>> status = get_sha1(bol, commit_sha1);
>> *end_of_object_name = saved;
> [...]
>> if (status < 0)
>> - return NULL;
>> + return error(_("Line %d: Malformed object name: %s"), lineno, bol);
>
> (Not about this patch, but an earlier one) What is this idiom about?
> Why not
>
> if (get_sha1(bol, commit_sha1))
> return error(_(...));
>
> ?
I'm first detecting the end of the object name and terminating it with
a '\0', before using using get_sha1() to read out the object name.
Then, I restore the instruction sheet to the original state by
restoring the character from 'saved'. If I use the idiom you
suggested, I'll leave midway after editing the instruction sheet, and
this is undesirable.
-- Ram
^ permalink raw reply
* Re: [PATCH 2/5] revert: make commit subjects in insn sheet optional
From: Ramkumar Ramachandra @ 2011-12-09 5:57 UTC (permalink / raw)
To: Jonathan Nieder; +Cc: Junio C Hamano, Git List
In-Reply-To: <20111207070223.GC11737@elie.hsd1.il.comcast.net>
Hi again,
Jonathan Nieder wrote:
> Ramkumar Ramachandra wrote:
>
>> [...]
>> While at it, also fix a bug: currently, we use a commit-id-shaped
>> buffer to store the word after "pick" in '.git/sequencer/todo'. This
>> is both wasteful and wrong because it places an artificial limit on
>> the line length. Eliminate the need for the buffer altogether, and
>> add a test demonstrating this.
>
> Reading the above does not make it at all obvious that I should want
> to apply this patch because otherwise my prankster friend can cause my
> copy of git to crash or run arbitrary code by putting a long commit
Working backwards:
get_sha1() is what will finally misbehave: how? It uses strlen() and
let's assume that the number returned by it is too big to fit in a
size_t. Surely, this means that we should only use get_sha1() on
something whose length is bounded. So, do we ever try to get to the
end of the line? Yes! Let's assume that the problem starts when
end_of_object_name calls strcspn which returns something too big to
fit in a size_t. From the manpage it has no standard way of reporting
failure. I'm not sure what to do; I think I have two choices:
1. Implement the strscpn() using two strchrnul() calls.
2. Drop this patch and use strbuf to replace the fixed-size buffer.
I think I'll go with the second option. What do you think?
-- Ram
^ permalink raw reply
* Re: [PATCH 2/5] revert: make commit subjects in insn sheet optional
From: Ramkumar Ramachandra @ 2011-12-09 6:12 UTC (permalink / raw)
To: Jonathan Nieder; +Cc: Junio C Hamano, Git List
In-Reply-To: <CALkWK0nkPB5WptJ9nSkSOif5_0R_f39Dg_HR3Rmg02hG_4Q1Tg@mail.gmail.com>
Hi,
Ramkumar Ramachandra wrote:
> 1. Implement the strscpn() using two strchrnul() calls.
> 2. Drop this patch and use strbuf to replace the fixed-size buffer.
>
> I think I'll go with the second option. What do you think?
> [...]
Er, I have an 'eol' which I'm not using. Making sure that 'eol' is
valid is good enough to avoid unexpected failures: if strchrnul() is
able to find a '\n', strcspn() should have no trouble finding either a
' ' or '\n' (whichever comes first).
Sorry for the nonsense.
-- Ram
^ permalink raw reply
* Re: Question about commit message wrapping
From: Frans Klaver @ 2011-12-09 7:05 UTC (permalink / raw)
To: git, Sidney San Martín
In-Reply-To: <35A5A513-91FD-4EF9-B890-AB3D1550D63F@sidneysm.com>
On Fri, 09 Dec 2011 02:59:06 +0100, Sidney San Martín <s@sidneysm.com>
wrote:
> Hey, I want to ask about the practice of wrapping commit messages to
> 70-something charaters.
>
> The webpage most cited about it, which I otherwise really like, is
>
> http://tbaggery.com/2008/04/19/a-note-about-git-commit-messages.html
>
> *Nothing else* in my everyday life works this way anymore. Line wrapping
> gets done on the display end in my email client, my web browser, my
> ebook reader entirely automatically, and it adapts to the size of the
> window.
Actually, opera-mail autowraps at 72 characters but sets the text format
to flowed. It also wraps the quoted text when you reply. But there's a
reasonable chance that you don't use opera in your daily life. On the
other hand I would not be surprised if most decent e-mail clients worked
that way.
Nobody's forcing you to use the same practice in your own projects anyway.
>
> That article gives two reasons why commits should be wrapped to 72
> columns. First:
>
>> git log doesn’t do any special special wrapping of the commit messages.
>> With the default pager of less -S, this means your paragraphs flow far
>> off the edge of the screen, making them difficult to read. On an 80
>> column terminal, if we subtract 4 columns for the indent on the left
>> and 4 more for symmetry on the right, we’re left with 72 columns.
>
> Here, I put a patch at the bottom of this email that wraps commit
> messages to, right now, 80 columns when they're displayed. (It’s a quick
> one, probably needs configurability. Also, beware, I don’t program in C
> too much.)
Hm. Saying "that's how the tool works" is not a good reason in my opinion.
There might be tons of other reasons for wrapping at 80 characters.
Readability is one that comes to mind for me.
>
> Second:
>
>> git format-patch --stdout converts a series of commits to a series of
>> emails, using the messages for the message body. Good email netiquette
>> dictates we wrap our plain text emails such that there’s room for a few
>> levels of nested reply indicators without overflow in an 80 column
>> terminal. (The current rails.git workflow doesn’t include email, but
>> who knows what the future will bring.)
>
> There's been a standard for flowed plain text emails (which don't have
> to wrap at 80 columns) for well over ten years, RFC-2646 and is widely
> supported. Besides, code in diffs is often longer than 7x characters,
> and wrapping, like `git log`, could be done inside git. FWIW, there are
> a bunch of merge commits with lines longer than 80 characters in the git
> repo itself.
Yes, that standard allows e-mail clients to display the text more fluidly,
even if the source text is word-wrapped. While git uses e-mail format, it
isn't an e-mail client. I always interpreted this whole thing as git
basically creating plain-text e-mails. You're actually writing the source
of the e-mail in your commit message. If you care about actual use in
e-mail (like we do here on the list) you might want to add the relevant
header to the mails. That said, Apple Mail (the client you used to send
your mail) doesn't even use the RFC you quote in the sent message. That
mail is going to be a pain in the butt to read in mutt from work ;).
>
> Finally, people read commits these days in many other places than `git
> log` (and make commits in many other places than a text editor
> configured to wrap). Most every GUI and already word wraps commit
> messages just fine. As a result, there are commits in popular repos much
> longer than the 72-column standard and no one notices. Instead,
> properly-formatted commit messages end up looking cramped when you see
> them in anywhere wider than 80 columns.
Cramped? I think it's compact and actually I prefer it over long lines.
> Am I crazy?
Probably not. Don't take my word for it. I'm not a psychiatrist.
> If this makes sense to anyone else, I'd be happy to help massage this
> into something git-worthy, with some help (never worked on Git before).
>
> - - -
>
> From a93b390d1506652d4ad41d1cbd987ba98a8deca0 Mon Sep 17 00:00:00 2001
> From: =?UTF-8?q?Sidney=20San=20Marti=CC=81n?= <s@sidneysm.com>
> Date: Thu, 8 Dec 2011 20:26:23 -0500
> Subject: [PATCH] Wrap commit messages on display
>
> - Wrap to 80 characters minus the indent
> - Use a hanging indent for lines which begin with "- "
> - Do not wrap lines which begin with whitespace
> ---
> pretty.c | 10 ++++++++--
> 1 files changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/pretty.c b/pretty.c
> index 230fe1c..15804ce 100644
> --- a/pretty.c
> +++ b/pretty.c
> @@ -1243,8 +1243,14 @@ void pp_remainder(const struct
> pretty_print_context *pp,
> memset(sb->buf + sb->len, ' ', indent);
> strbuf_setlen(sb, sb->len + indent);
> }
> - strbuf_add(sb, line, linelen);
> - strbuf_addch(sb, '\n');
> + if (line[0] == ' ' || line[0] == '\t') {
> + strbuf_add(sb, line, linelen);
> + } else {
> + struct strbuf wrapped = STRBUF_INIT;
> + strbuf_add(&wrapped, line, linelen);
> + strbuf_add_wrapped_text(sb, wrapped.buf, 0, indent + (line[0] == '-'
> && line[1] == ' ' ? 2 : 0), 80 - indent);
While on the subject, In my mail view, the new line started with the [1]
from line[1], in the quote the line looks entirely different. Now this is
code we're talking about, so it makes slightly more sense to have a proper
wrapping hard-coded. Compare the above with the following:
+ int hanging_indent = ((line[0] == '-' && line[1] == ' ') ? 2 : 0);
[...]
+ strbuf_add_wrapped_text(sb, wrapped.buf, 0,
+ indent + hanging_indent,
+ 80 - indent);
Much clearer, no? I personally usually have two or three terminals tucked
next to each other, so I can look at two or three things at the same time.
80 characters limit is a nice feature then.
> + strbuf_addch(sb, '\n');
> + }
> }
> }
>
Cheers,
Frans
^ permalink raw reply
* [PATCH] am: don't persist keepcr flag
From: Martin von Zweigbergk @ 2011-12-09 7:30 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Martin von Zweigbergk
The keepcr flag is only used in the split_patches function, which is
only called before a patch application has to stopped for user input,
not after resuming. It is therefore unnecessary to persist the
flag. This seems to have been the case since it was introduced in
ad2c928 (git-am: Add command line parameter `--keep-cr` passing it to
git-mailsplit, 2010-02-27).
Signed-off-by: Martin von Zweigbergk <martin.von.zweigbergk@gmail.com>
---
git-am.sh | 7 -------
1 files changed, 0 insertions(+), 7 deletions(-)
diff --git a/git-am.sh b/git-am.sh
index 9042432..1c13b13 100755
--- a/git-am.sh
+++ b/git-am.sh
@@ -530,7 +530,6 @@ else
echo "$sign" >"$dotest/sign"
echo "$utf8" >"$dotest/utf8"
echo "$keep" >"$dotest/keep"
- echo "$keepcr" >"$dotest/keepcr"
echo "$scissors" >"$dotest/scissors"
echo "$no_inbody_headers" >"$dotest/no_inbody_headers"
echo "$GIT_QUIET" >"$dotest/quiet"
@@ -576,12 +575,6 @@ if test "$(cat "$dotest/keep")" = t
then
keep=-k
fi
-case "$(cat "$dotest/keepcr")" in
-t)
- keepcr=--keep-cr ;;
-f)
- keepcr=--no-keep-cr ;;
-esac
case "$(cat "$dotest/scissors")" in
t)
scissors=--scissors ;;
--
1.7.8.237.gcc4e3
^ permalink raw reply related
* Re: Question about commit message wrapping
From: Frans Klaver @ 2011-12-09 7:51 UTC (permalink / raw)
To: git, Sidney San Martín
In-Reply-To: <op.v57na7120aolir@keputer>
On Fri, Dec 9, 2011 at 8:05 AM, Frans Klaver <fransklaver@gmail.com> wrote:
>> *Nothing else* in my everyday life works this way anymore. Line wrapping
>> gets done on the display end in my email client, my web browser, my ebook
>> reader entirely automatically, and it adapts to the size of the window.
It appears none of the books I have available here have more than 80
characters on one line. It's a typography thing. And typographers for
some reason rarely get it wrong. Why not make use of that knowledge?
^ permalink raw reply
* Re: [PATCH 1/6] t3040 (subprojects-basic): modernize style
From: Ramkumar Ramachandra @ 2011-12-09 7:56 UTC (permalink / raw)
To: Jonathan Nieder; +Cc: Junio C Hamano, Matthieu Moy, Git List
In-Reply-To: <20111208163448.GA2394@elie.hsd1.il.comcast.net>
Hi,
Jonathan Nieder wrote:
> Ramkumar Ramachandra wrote:
>
>> Put the opening quote starting each test on the same line as the
>> test_expect_* invocation. While at it:
>
> I suspect the above description, while it does describe your patch,
> does not describe the _reason_ that the patch exists or that someone
> would want to apply it. Isn't it something more like:
> [...]
Right, fixed.
> I didn't read over the patch again. Has it changed since v1?
No. I refrained from making other style changes and/ or combining tests.
-- Ram
^ permalink raw reply
* [POC PATCH 1/5] Turn grep's use_threads into a global flag
From: Thomas Rast @ 2011-12-09 8:39 UTC (permalink / raw)
To: git; +Cc: René Scharfe, Junio C Hamano, Eric Herman
In-Reply-To: <cover.1323419666.git.trast@student.ethz.ch>
In preparation for further work on this, turn use_threads into a flag
shared across the whole code base. The supporting (un)lock_if_threaded()
functions are to be used for locking; they return immediately when not
threading.
Signed-off-by: Thomas Rast <trast@student.ethz.ch>
---
builtin/grep.c | 20 ++++++++------------
thread-utils.c | 16 ++++++++++++++++
thread-utils.h | 16 ++++++++++++++++
3 files changed, 40 insertions(+), 12 deletions(-)
diff --git a/builtin/grep.c b/builtin/grep.c
index 988ea1d..76f2c4f 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -24,8 +24,6 @@
NULL
};
-static int use_threads = 1;
-
#ifndef NO_PTHREADS
#define THREADS 8
static pthread_t threads[THREADS];
@@ -76,14 +74,12 @@ struct work_item {
static inline void grep_lock(void)
{
- if (use_threads)
- pthread_mutex_lock(&grep_mutex);
+ lock_if_threaded(&grep_mutex);
}
static inline void grep_unlock(void)
{
- if (use_threads)
- pthread_mutex_unlock(&grep_mutex);
+ unlock_if_threaded(&grep_mutex);
}
/* Used to serialize calls to read_sha1_file. */
@@ -91,14 +87,12 @@ static inline void grep_unlock(void)
static inline void read_sha1_lock(void)
{
- if (use_threads)
- pthread_mutex_lock(&read_sha1_mutex);
+ lock_if_threaded(&read_sha1_mutex);
}
static inline void read_sha1_unlock(void)
{
- if (use_threads)
- pthread_mutex_unlock(&read_sha1_mutex);
+ unlock_if_threaded(&read_sha1_mutex);
}
/* Signalled when a new work_item is added to todo. */
@@ -984,6 +978,10 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
argc--;
}
+#ifndef NO_PTHREADS
+ use_threads = 1;
+#endif
+
if (show_in_pager == default_pager)
show_in_pager = git_pager(1);
if (show_in_pager) {
@@ -1011,8 +1009,6 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
skip_first_line = 1;
start_threads(&opt);
}
-#else
- use_threads = 0;
#endif
compile_grep_patterns(&opt);
diff --git a/thread-utils.c b/thread-utils.c
index 7f4b76a..fb75a29 100644
--- a/thread-utils.c
+++ b/thread-utils.c
@@ -1,6 +1,8 @@
#include "cache.h"
#include "thread-utils.h"
+int use_threads;
+
#if defined(hpux) || defined(__hpux) || defined(_hpux)
# include <sys/pstat.h>
#endif
@@ -59,3 +61,17 @@ int init_recursive_mutex(pthread_mutex_t *m)
}
return ret;
}
+
+#ifndef NO_PTHREADS
+void lock_if_threaded(pthread_mutex_t *m)
+{
+ if (use_threads)
+ pthread_mutex_lock(m);
+}
+
+void unlock_if_threaded(pthread_mutex_t *m)
+{
+ if (use_threads)
+ pthread_mutex_unlock(m);
+}
+#endif
diff --git a/thread-utils.h b/thread-utils.h
index 6fb98c3..9a780a2 100644
--- a/thread-utils.h
+++ b/thread-utils.h
@@ -1,11 +1,27 @@
#ifndef THREAD_COMPAT_H
#define THREAD_COMPAT_H
+/*
+ * This variable is used by commands to globally tell affected
+ * subsystems that they must use thread-safe mechanisms.
+ */
+extern int use_threads;
+
#ifndef NO_PTHREADS
#include <pthread.h>
extern int online_cpus(void);
extern int init_recursive_mutex(pthread_mutex_t*);
+/* These functions do nothing if use_threads==0 or NO_PTHREADS */
+extern void lock_if_threaded(pthread_mutex_t*);
+extern void unlock_if_threaded(pthread_mutex_t*);
+
+#else
+
+#define lock_if_threaded(lock)
+#define unlock_if_threaded(lock)
+
#endif
+
#endif /* THREAD_COMPAT_H */
--
1.7.8.431.g2abf2
^ permalink raw reply related
* [POC PATCH 0/5] Threaded loose object and pack access
From: Thomas Rast @ 2011-12-09 8:39 UTC (permalink / raw)
To: git; +Cc: René Scharfe, Junio C Hamano, Eric Herman
Well, just to make sure we're all left in a confused mess of partly
conflicting patches, here's another angle on the same thing:
Jeff King wrote:
> Wow, that's horrible. Leaving aside the parallelism, it's just terrible
> that reading from the cache is 20 times slower than the worktree. I get
> similar results on my quad-core machine.
By poking around in sha1_file.c I got that down to about 10. It's not
great yet, but it seems a start.
The goal would be to improve it to the point where a patch lookup that
already has all relevant packs open and windows mapped can proceed
without locking. I'm not sure that's doable short of duplicating the
whole pack state (including fds and windows) across threads, but I'll
give it some more thought before going that route.
Thomas Rast (5):
Turn grep's use_threads into a global flag
grep: push locking into read_sha1_*
sha1_file_name_buf(): sha1_file_name in caller's buffer
sha1_file: stuff various pack reading variables into a struct
sha1_file: make the pack machinery thread-safe
builtin/grep.c | 60 +++++-----------
cache.h | 1 +
replace_object.c | 5 +-
sha1_file.c | 213 +++++++++++++++++++++++++++++++++++++++++-------------
thread-utils.c | 30 ++++++++
thread-utils.h | 23 ++++++
6 files changed, 240 insertions(+), 92 deletions(-)
--
1.7.8.431.g2abf2
^ permalink raw reply
* [POC PATCH 3/5] sha1_file_name_buf(): sha1_file_name in caller's buffer
From: Thomas Rast @ 2011-12-09 8:39 UTC (permalink / raw)
To: git; +Cc: René Scharfe, Junio C Hamano, Eric Herman
In-Reply-To: <cover.1323419666.git.trast@student.ethz.ch>
sha1_file_name is non-reentrant because of its use of a static buffer.
Split it into just the buffer writing (which can be called even from
threads as long as the buffer is stack'd) and a small wrapper that
uses the static buffer as before.
Signed-off-by: Thomas Rast <trast@student.ethz.ch>
---
sha1_file.c | 29 +++++++++++++++++++----------
1 files changed, 19 insertions(+), 10 deletions(-)
diff --git a/sha1_file.c b/sha1_file.c
index c3595b3..18648c3 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -153,18 +153,11 @@ static void fill_sha1_path(char *pathbuf, const unsigned char *sha1)
}
/*
- * NOTE! This returns a statically allocated buffer, so you have to be
- * careful about using it. Do an "xstrdup()" if you need to save the
- * filename.
- *
- * Also note that this returns the location for creating. Reading
- * SHA1 file can happen from any alternate directory listed in the
- * DB_ENVIRONMENT environment variable if it is not found in
- * the primary object database.
+ * Similar to sha1_file_name but you provide a buffer of size at least
+ * PATH_MAX.
*/
-char *sha1_file_name(const unsigned char *sha1)
+void sha1_file_name_buf(char *buf, const unsigned char *sha1)
{
- static char buf[PATH_MAX];
const char *objdir;
int len;
@@ -179,6 +172,22 @@ char *sha1_file_name(const unsigned char *sha1)
buf[len+3] = '/';
buf[len+42] = '\0';
fill_sha1_path(buf + len + 1, sha1);
+}
+
+/*
+ * NOTE! This returns a statically allocated buffer, so you have to be
+ * careful about using it. Do an "xstrdup()" if you need to save the
+ * filename.
+ *
+ * Also note that this returns the location for creating. Reading
+ * SHA1 file can happen from any alternate directory listed in the
+ * DB_ENVIRONMENT environment variable if it is not found in
+ * the primary object database.
+ */
+char *sha1_file_name(const unsigned char *sha1)
+{
+ static char buf[PATH_MAX];
+ sha1_file_name_buf(buf, sha1);
return buf;
}
--
1.7.8.431.g2abf2
^ permalink raw reply related
* [POC PATCH 5/5] sha1_file: make the pack machinery thread-safe
From: Thomas Rast @ 2011-12-09 8:39 UTC (permalink / raw)
To: git; +Cc: René Scharfe, Junio C Hamano, Eric Herman
In-Reply-To: <cover.1323419666.git.trast@student.ethz.ch>
More precisely speaking, this pushes the locking down from
read_object() into bits of the pack machinery that cannot (yet) run in
parallel.
There are several hacks here:
a) prepare_packed_git() must be called before any parallel accesses
happen. It now unconditionally opens and maps all index files.
b) similarly, prepare_replace_object() must be called before any
parallel read_sha1_file() happens
This simplification lets us avoid locking outright to guard the index
accesses; locking is then mainly required for open_packed_git(),
[un]use_pack(), and such.
The ultimate goal would of course be to let at least _some_ pack
accesses happen without any locking whatsoever. But grep already
benefits from it with a nice speed boost on non-worktree greps.
Signed-off-by: Thomas Rast <trast@student.ethz.ch>
---
builtin/grep.c | 9 ++++++
cache.h | 1 +
replace_object.c | 5 ++-
sha1_file.c | 81 +++++++++++++++++++++++++++++++++++++++++++++++------
thread-utils.c | 9 ++++--
thread-utils.h | 3 +-
6 files changed, 93 insertions(+), 15 deletions(-)
diff --git a/builtin/grep.c b/builtin/grep.c
index 6c5bdfa..212497d 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -980,6 +980,15 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
if (opt.pre_context || opt.post_context || opt.file_break ||
opt.funcbody)
skip_first_line = 1;
+ /*
+ * This does the non-threadsafe work early. FIXME:
+ * grep shouldn't have to know about this mess.
+ */
+ use_threads = 0;
+ prepare_replace_object();
+ prepare_packed_git();
+ use_threads = 1;
+
start_threads(&opt);
}
#endif
diff --git a/cache.h b/cache.h
index 8c98d05..379dd44 100644
--- a/cache.h
+++ b/cache.h
@@ -764,6 +764,7 @@ static inline const unsigned char *lookup_replace_object(const unsigned char *sh
return sha1;
return do_lookup_replace_object(sha1);
}
+extern void prepare_replace_object(void);
/* Read and unpack a sha1 file into memory, write memory to a sha1 file */
extern int sha1_object_info(const unsigned char *, unsigned long *);
diff --git a/replace_object.c b/replace_object.c
index d0b1548..b303392 100644
--- a/replace_object.c
+++ b/replace_object.c
@@ -2,6 +2,7 @@
#include "sha1-lookup.h"
#include "refs.h"
#include "commit.h"
+#include "thread-utils.h"
static struct replace_object {
unsigned char sha1[2][20];
@@ -76,13 +77,15 @@ static int register_replace_ref(const char *refname,
return 0;
}
-static void prepare_replace_object(void)
+void prepare_replace_object(void)
{
static int replace_object_prepared;
if (replace_object_prepared)
return;
+ assert(!use_threads);
+
for_each_replace_ref(register_replace_ref, NULL);
replace_object_prepared = 1;
if (!replace_object_nr)
diff --git a/sha1_file.c b/sha1_file.c
index 7c367f9..b61692e 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -429,7 +429,8 @@ void prepare_alt_odb(void)
static int has_loose_object_local(const unsigned char *sha1)
{
- char *name = sha1_file_name(sha1);
+ char name[PATH_MAX];
+ sha1_file_name_buf(name, sha1);
return !access(name, F_OK);
}
@@ -650,9 +651,12 @@ static int unuse_one_window(struct packed_git *current, int keep_fd)
void release_pack_memory(size_t need, int fd)
{
- size_t cur = pack_mapped;
+ size_t cur;
+ lock_if_threaded(&pack_access_mutex);
+ cur = pack_mapped;
while (need >= (cur - pack_mapped) && unuse_one_window(NULL, fd))
; /* nothing */
+ unlock_if_threaded(&pack_access_mutex);
}
void *xmmap(void *start, size_t length,
@@ -689,9 +693,12 @@ void close_pack_windows(struct packed_git *p)
void unuse_pack(struct pack_window **w_cursor)
{
struct pack_window *w = *w_cursor;
+
if (w) {
+ lock_if_threaded(&pack_access_mutex);
w->inuse_cnt--;
*w_cursor = NULL;
+ unlock_if_threaded(&pack_access_mutex);
}
}
@@ -712,10 +719,13 @@ void close_pack_index(struct packed_git *p)
* must subsist at this point. If ever objects from this pack are requested
* again, the new version of the pack will be reinitialized through
* reprepare_packed_git().
+ *
+ * NOT THREAD-SAFE
*/
void free_pack_by_name(const char *pack_name)
{
struct packed_git *p, **pp = &packed_git;
+ assert(!use_threads);
while (*pp) {
p = *pp;
@@ -821,13 +831,33 @@ static int open_packed_git_1(struct packed_git *p)
static int open_packed_git(struct packed_git *p)
{
- if (!open_packed_git_1(p))
+ lock_if_threaded(&pack_access_mutex);
+ /*
+ * is_pack_valid() took the easy route and did not
+ * lock. This is probably okay; if the pack was
+ * *ever* open, it was valid unless another process is
+ * actively trying to corrupt it, in which case:
+ * meh.
+ *
+ * However, a concurrent open_packed_git() may already have
+ * opened it before we get here. So we test again in a locked
+ * section. If it beat us to it, then we have no work left to
+ * do.
+ */
+ if (p->pack_fd != -1) {
+ unlock_if_threaded(&pack_access_mutex);
return 0;
+ }
+ if (!open_packed_git_1(p)) {
+ unlock_if_threaded(&pack_access_mutex);
+ return 0;
+ }
if (p->pack_fd != -1) {
close(p->pack_fd);
pack_open_fds--;
p->pack_fd = -1;
}
+ unlock_if_threaded(&pack_access_mutex);
return -1;
}
@@ -858,6 +888,9 @@ unsigned char *use_pack(struct packed_git *p,
*/
if (!p->pack_size && p->pack_fd == -1 && open_packed_git(p))
die("packfile %s cannot be accessed", p->pack_name);
+
+ lock_if_threaded(&pack_access_mutex);
+
if (offset > (p->pack_size - 20))
die("offset beyond end of packfile (truncated pack?)");
@@ -916,6 +949,9 @@ unsigned char *use_pack(struct packed_git *p,
offset -= win->offset;
if (left)
*left = win->len - xsize_t(offset);
+
+ unlock_if_threaded(&pack_access_mutex);
+
return win->base + offset;
}
@@ -1044,6 +1080,7 @@ static void prepare_packed_git_one(char *objdir, int local)
if (!p)
continue;
install_packed_git(p);
+ open_pack_index(p);
}
closedir(dir);
}
@@ -1102,6 +1139,12 @@ static void rearrange_packed_git(void)
free(ary);
}
+/*
+ * NOT THREAD-SAFE
+ *
+ * However, it's ok if you run this early, before starting threads,
+ * and then use the pack machinery from threads.
+ */
static int prepare_packed_git_run_once = 0;
void prepare_packed_git(void)
{
@@ -1109,6 +1152,7 @@ void prepare_packed_git(void)
if (prepare_packed_git_run_once)
return;
+ assert (!use_threads);
prepare_packed_git_one(get_object_directory(), 1);
prepare_alt_odb();
for (alt = alt_odb_list; alt; alt = alt->next) {
@@ -1180,8 +1224,10 @@ static int git_open_noatime(const char *name)
static int open_sha1_file(const unsigned char *sha1)
{
int fd;
- char *name = sha1_file_name(sha1);
+ char namebuf[PATH_MAX];
+ char *name = namebuf;
struct alternate_object_database *alt;
+ sha1_file_name_buf(name, sha1);
fd = git_open_noatime(name);
if (fd >= 0)
@@ -1698,7 +1744,22 @@ static struct pack_context *pack_context_alloc(void)
return ctx;
}
+#ifdef NO_PTHREADS
#define get_thread_pack_context() (&main_pack_context)
+#else
+static struct pack_context *get_thread_pack_context(void)
+{
+ struct pack_context *ctx;
+ if (!use_threads)
+ return &main_pack_context;
+ ctx = pthread_getspecific(pack_context_key);
+ if (ctx)
+ return ctx;
+ ctx = pack_context_alloc();
+ pthread_setspecific(pack_context_key, ctx);
+ return ctx;
+}
+#endif
static unsigned long pack_entry_hash(struct packed_git *p, off_t base_offset)
{
@@ -2219,6 +2280,10 @@ static void *read_packed_sha1(const unsigned char *sha1,
return data;
}
+/*
+ * WARNING: must never be called concurrently with read_sha1_file and
+ * friends! They do lookups in the cached_objects without locking.
+ */
int pretend_sha1_file(void *buf, unsigned long len, enum object_type type,
unsigned char *sha1)
{
@@ -2280,19 +2345,15 @@ void *read_sha1_file_extended(const unsigned char *sha1,
unsigned flag)
{
void *data;
- char *path;
const struct packed_git *p;
const unsigned char *repl;
- lock_if_threaded(&read_sha1_mutex);
-
repl = (flag & READ_SHA1_FILE_REPLACE)
? lookup_replace_object(sha1) : sha1;
errno = 0;
data = read_object(repl, type, size);
if (data) {
- unlock_if_threaded(&read_sha1_mutex);
return data;
}
@@ -2305,7 +2366,8 @@ void *read_sha1_file_extended(const unsigned char *sha1,
sha1_to_hex(repl), sha1_to_hex(sha1));
if (has_loose_object(repl)) {
- path = sha1_file_name(sha1);
+ char path[PATH_MAX];
+ sha1_file_name_buf(path, sha1);
die("loose object %s (stored in %s) is corrupt",
sha1_to_hex(repl), path);
}
@@ -2314,7 +2376,6 @@ void *read_sha1_file_extended(const unsigned char *sha1,
die("packed object %s (stored in %s) is corrupt",
sha1_to_hex(repl), p->pack_name);
- unlock_if_threaded(&read_sha1_mutex);
return NULL;
}
diff --git a/thread-utils.c b/thread-utils.c
index 70af3f9..0da2b65 100644
--- a/thread-utils.c
+++ b/thread-utils.c
@@ -62,15 +62,18 @@ int init_recursive_mutex(pthread_mutex_t *m)
return ret;
}
-pthread_mutex_t read_sha1_mutex;
+pthread_mutex_t pack_access_mutex;
+pthread_key_t pack_context_key;
void init_subsystem_locks(void)
{
- pthread_mutex_init(&read_sha1_mutex, NULL);
+ init_recursive_mutex(&pack_access_mutex);
+ pthread_key_create(&pack_context_key, NULL);
}
void destroy_subsystem_locks(void)
{
- pthread_mutex_destroy(&read_sha1_mutex);
+ pthread_mutex_destroy(&pack_access_mutex);
+ pthread_key_delete(pack_context_key);
}
#ifndef NO_PTHREADS
diff --git a/thread-utils.h b/thread-utils.h
index 3906753..7d3cc0a 100644
--- a/thread-utils.h
+++ b/thread-utils.h
@@ -17,7 +17,8 @@
extern void lock_if_threaded(pthread_mutex_t*);
extern void unlock_if_threaded(pthread_mutex_t*);
-extern pthread_mutex_t read_sha1_mutex;
+extern pthread_mutex_t pack_access_mutex;
+extern pthread_key_t pack_context_key;
extern void init_subsystem_locks(void);
extern void destroy_subsystem_locks(void);
--
1.7.8.431.g2abf2
^ permalink raw reply related
* [POC PATCH 4/5] sha1_file: stuff various pack reading variables into a struct
From: Thomas Rast @ 2011-12-09 8:39 UTC (permalink / raw)
To: git; +Cc: René Scharfe, Junio C Hamano, Eric Herman
In-Reply-To: <cover.1323419666.git.trast@student.ethz.ch>
In preparation for making these variables thread-local, put various
delta-cache related bits of pack reading state into a struct. For now
the accessor function is a dummy that always returns a static instance
of this struct.
Signed-off-by: Thomas Rast <trast@student.ethz.ch>
---
sha1_file.c | 99 ++++++++++++++++++++++++++++++++++++++++-------------------
1 files changed, 67 insertions(+), 32 deletions(-)
diff --git a/sha1_file.c b/sha1_file.c
index 18648c3..7c367f9 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -1655,21 +1655,50 @@ static void *unpack_compressed_entry(struct packed_git *p,
#define MAX_DELTA_CACHE (256)
-static size_t delta_base_cached;
-
-static struct delta_base_cache_lru_list {
+struct delta_base_cache_lru_list {
struct delta_base_cache_lru_list *prev;
struct delta_base_cache_lru_list *next;
-} delta_base_cache_lru = { &delta_base_cache_lru, &delta_base_cache_lru };
+};
+
+static struct delta_base_cache_lru_list main_delta_base_cache_lru = {
+ &main_delta_base_cache_lru, &main_delta_base_cache_lru
+};
-static struct delta_base_cache_entry {
+struct delta_base_cache_entry {
struct delta_base_cache_lru_list lru;
void *data;
struct packed_git *p;
off_t base_offset;
unsigned long size;
enum object_type type;
-} delta_base_cache[MAX_DELTA_CACHE];
+};
+
+static struct delta_base_cache_entry main_delta_base_cache[MAX_DELTA_CACHE];
+
+struct pack_context {
+ size_t delta_base_cached;
+ struct delta_base_cache_entry *delta_base_cache;
+ struct delta_base_cache_lru_list *delta_base_cache_lru;
+ struct packed_git *last_found;
+};
+
+static struct pack_context main_pack_context = {
+ 0, main_delta_base_cache, &main_delta_base_cache_lru, (void*)1
+};
+
+static struct pack_context *pack_context_alloc(void)
+{
+ struct pack_context *ctx = xmalloc(sizeof(struct pack_context));
+ ctx->delta_base_cached = 0;
+ ctx->delta_base_cache_lru = xmalloc(sizeof(struct delta_base_cache_lru_list));
+ ctx->delta_base_cache_lru->prev = ctx->delta_base_cache_lru;
+ ctx->delta_base_cache_lru->next = ctx->delta_base_cache_lru;
+ ctx->delta_base_cache = xcalloc(MAX_DELTA_CACHE, sizeof(struct delta_base_cache_entry));
+ ctx->last_found = (void*)1;
+ return ctx;
+}
+
+#define get_thread_pack_context() (&main_pack_context)
static unsigned long pack_entry_hash(struct packed_git *p, off_t base_offset)
{
@@ -1683,7 +1712,8 @@ static unsigned long pack_entry_hash(struct packed_git *p, off_t base_offset)
static int in_delta_base_cache(struct packed_git *p, off_t base_offset)
{
unsigned long hash = pack_entry_hash(p, base_offset);
- struct delta_base_cache_entry *ent = delta_base_cache + hash;
+ struct delta_base_cache_entry *ent
+ = get_thread_pack_context()->delta_base_cache + hash;
return (ent->data && ent->p == p && ent->base_offset == base_offset);
}
@@ -1692,7 +1722,8 @@ static void *cache_or_unpack_entry(struct packed_git *p, off_t base_offset,
{
void *ret;
unsigned long hash = pack_entry_hash(p, base_offset);
- struct delta_base_cache_entry *ent = delta_base_cache + hash;
+ struct pack_context *ctx = get_thread_pack_context();
+ struct delta_base_cache_entry *ent = ctx->delta_base_cache + hash;
ret = ent->data;
if (!ret || ent->p != p || ent->base_offset != base_offset)
@@ -1702,7 +1733,7 @@ static void *cache_or_unpack_entry(struct packed_git *p, off_t base_offset,
ent->data = NULL;
ent->lru.next->prev = ent->lru.prev;
ent->lru.prev->next = ent->lru.next;
- delta_base_cached -= ent->size;
+ ctx->delta_base_cached -= ent->size;
} else {
ret = xmemdupz(ent->data, ent->size);
}
@@ -1711,48 +1742,52 @@ static void *cache_or_unpack_entry(struct packed_git *p, off_t base_offset,
return ret;
}
-static inline void release_delta_base_cache(struct delta_base_cache_entry *ent)
+static inline void release_delta_base_cache(struct pack_context *ctx,
+ struct delta_base_cache_entry *ent)
{
if (ent->data) {
free(ent->data);
ent->data = NULL;
ent->lru.next->prev = ent->lru.prev;
ent->lru.prev->next = ent->lru.next;
- delta_base_cached -= ent->size;
+ ctx->delta_base_cached -= ent->size;
}
}
void clear_delta_base_cache(void)
{
unsigned long p;
+ struct pack_context *ctx = get_thread_pack_context();
+ struct delta_base_cache_entry *delta_base_cache = ctx->delta_base_cache;
for (p = 0; p < MAX_DELTA_CACHE; p++)
- release_delta_base_cache(&delta_base_cache[p]);
+ release_delta_base_cache(ctx, &delta_base_cache[p]);
}
static void add_delta_base_cache(struct packed_git *p, off_t base_offset,
void *base, unsigned long base_size, enum object_type type)
{
unsigned long hash = pack_entry_hash(p, base_offset);
- struct delta_base_cache_entry *ent = delta_base_cache + hash;
+ struct pack_context *ctx = get_thread_pack_context();
+ struct delta_base_cache_entry *ent = ctx->delta_base_cache + hash;
struct delta_base_cache_lru_list *lru;
- release_delta_base_cache(ent);
- delta_base_cached += base_size;
+ release_delta_base_cache(ctx, ent);
+ ctx->delta_base_cached += base_size;
- for (lru = delta_base_cache_lru.next;
- delta_base_cached > delta_base_cache_limit
- && lru != &delta_base_cache_lru;
+ for (lru = ctx->delta_base_cache_lru->next;
+ ctx->delta_base_cached > delta_base_cache_limit
+ && lru != ctx->delta_base_cache_lru;
lru = lru->next) {
struct delta_base_cache_entry *f = (void *)lru;
if (f->type == OBJ_BLOB)
- release_delta_base_cache(f);
+ release_delta_base_cache(ctx, f);
}
- for (lru = delta_base_cache_lru.next;
- delta_base_cached > delta_base_cache_limit
- && lru != &delta_base_cache_lru;
+ for (lru = ctx->delta_base_cache_lru->next;
+ ctx->delta_base_cached > delta_base_cache_limit
+ && lru != ctx->delta_base_cache_lru;
lru = lru->next) {
struct delta_base_cache_entry *f = (void *)lru;
- release_delta_base_cache(f);
+ release_delta_base_cache(ctx, f);
}
ent->p = p;
@@ -1760,10 +1795,10 @@ static void add_delta_base_cache(struct packed_git *p, off_t base_offset,
ent->type = type;
ent->data = base;
ent->size = base_size;
- ent->lru.next = &delta_base_cache_lru;
- ent->lru.prev = delta_base_cache_lru.prev;
- delta_base_cache_lru.prev->next = &ent->lru;
- delta_base_cache_lru.prev = &ent->lru;
+ ent->lru.next = ctx->delta_base_cache_lru;
+ ent->lru.prev = ctx->delta_base_cache_lru->prev;
+ ctx->delta_base_cache_lru->prev->next = &ent->lru;
+ ctx->delta_base_cache_lru->prev = &ent->lru;
}
static void *read_object(const unsigned char *sha1, enum object_type *type,
@@ -2021,14 +2056,14 @@ int is_pack_valid(struct packed_git *p)
static int find_pack_entry(const unsigned char *sha1, struct pack_entry *e)
{
- static struct packed_git *last_found = (void *)1;
struct packed_git *p;
off_t offset;
+ struct pack_context *ctx = get_thread_pack_context();
prepare_packed_git();
if (!packed_git)
return 0;
- p = (last_found == (void *)1) ? packed_git : last_found;
+ p = (ctx->last_found == (void *)1) ? packed_git : ctx->last_found;
do {
if (p->num_bad_objects) {
@@ -2055,16 +2090,16 @@ static int find_pack_entry(const unsigned char *sha1, struct pack_entry *e)
e->offset = offset;
e->p = p;
hashcpy(e->sha1, sha1);
- last_found = p;
+ ctx->last_found = p;
return 1;
}
next:
- if (p == last_found)
+ if (p == ctx->last_found)
p = packed_git;
else
p = p->next;
- if (p == last_found)
+ if (p == ctx->last_found)
p = p->next;
} while (p);
return 0;
--
1.7.8.431.g2abf2
^ permalink raw reply related
* [POC PATCH 2/5] grep: push locking into read_sha1_*
From: Thomas Rast @ 2011-12-09 8:39 UTC (permalink / raw)
To: git; +Cc: René Scharfe, Junio C Hamano, Eric Herman
In-Reply-To: <cover.1323419666.git.trast@student.ethz.ch>
Move the locking away from grep (the user) and into read_sha1_* and
read_object_* (the subsystem). This will allow future work on the
locking granularity there.
Signed-off-by: Thomas Rast <trast@student.ethz.ch>
---
builtin/grep.c | 35 ++++-------------------------------
sha1_file.c | 12 ++++++++++--
thread-utils.c | 11 +++++++++++
thread-utils.h | 6 ++++++
4 files changed, 31 insertions(+), 33 deletions(-)
diff --git a/builtin/grep.c b/builtin/grep.c
index 76f2c4f..6c5bdfa 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -82,19 +82,6 @@ static inline void grep_unlock(void)
unlock_if_threaded(&grep_mutex);
}
-/* Used to serialize calls to read_sha1_file. */
-static pthread_mutex_t read_sha1_mutex;
-
-static inline void read_sha1_lock(void)
-{
- lock_if_threaded(&read_sha1_mutex);
-}
-
-static inline void read_sha1_unlock(void)
-{
- unlock_if_threaded(&read_sha1_mutex);
-}
-
/* Signalled when a new work_item is added to todo. */
static pthread_cond_t cond_add;
@@ -248,8 +235,8 @@ static void start_threads(struct grep_opt *opt)
{
int i;
+ init_subsystem_locks();
pthread_mutex_init(&grep_mutex, NULL);
- pthread_mutex_init(&read_sha1_mutex, NULL);
pthread_cond_init(&cond_add, NULL);
pthread_cond_init(&cond_write, NULL);
pthread_cond_init(&cond_result, NULL);
@@ -296,16 +283,14 @@ static int wait_all(void)
}
pthread_mutex_destroy(&grep_mutex);
- pthread_mutex_destroy(&read_sha1_mutex);
pthread_cond_destroy(&cond_add);
pthread_cond_destroy(&cond_write);
pthread_cond_destroy(&cond_result);
+ destroy_subsystem_locks();
return hit;
}
#else /* !NO_PTHREADS */
-#define read_sha1_lock()
-#define read_sha1_unlock()
static int wait_all(void)
{
@@ -363,21 +348,11 @@ static int grep_config(const char *var, const char *value, void *cb)
return 0;
}
-static void *lock_and_read_sha1_file(const unsigned char *sha1, enum object_type *type, unsigned long *size)
-{
- void *data;
-
- read_sha1_lock();
- data = read_sha1_file(sha1, type, size);
- read_sha1_unlock();
- return data;
-}
-
static void *load_sha1(const unsigned char *sha1, unsigned long *size,
const char *name)
{
enum object_type type;
- void *data = lock_and_read_sha1_file(sha1, &type, size);
+ void *data = read_sha1_file(sha1, &type, size);
if (!data)
error(_("'%s': unable to read %s"), name, sha1_to_hex(sha1));
@@ -578,7 +553,7 @@ static int grep_tree(struct grep_opt *opt, const struct pathspec *pathspec,
void *data;
unsigned long size;
- data = lock_and_read_sha1_file(entry.sha1, &type, &size);
+ data = read_sha1_file(entry.sha1, &type, &size);
if (!data)
die(_("unable to read tree (%s)"),
sha1_to_hex(entry.sha1));
@@ -608,10 +583,8 @@ static int grep_object(struct grep_opt *opt, const struct pathspec *pathspec,
struct strbuf base;
int hit, len;
- read_sha1_lock();
data = read_object_with_reference(obj->sha1, tree_type,
&size, NULL);
- read_sha1_unlock();
if (!data)
die(_("unable to read tree (%s)"), sha1_to_hex(obj->sha1));
diff --git a/sha1_file.c b/sha1_file.c
index 956422b..c3595b3 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -18,6 +18,7 @@
#include "refs.h"
#include "pack-revindex.h"
#include "sha1-lookup.h"
+#include "thread-utils.h"
#ifndef O_NOATIME
#if defined(__linux__) && (defined(__i386__) || defined(__PPC__))
@@ -2237,13 +2238,19 @@ void *read_sha1_file_extended(const unsigned char *sha1,
void *data;
char *path;
const struct packed_git *p;
- const unsigned char *repl = (flag & READ_SHA1_FILE_REPLACE)
+ const unsigned char *repl;
+
+ lock_if_threaded(&read_sha1_mutex);
+
+ repl = (flag & READ_SHA1_FILE_REPLACE)
? lookup_replace_object(sha1) : sha1;
errno = 0;
data = read_object(repl, type, size);
- if (data)
+ if (data) {
+ unlock_if_threaded(&read_sha1_mutex);
return data;
+ }
if (errno && errno != ENOENT)
die_errno("failed to read object %s", sha1_to_hex(sha1));
@@ -2263,6 +2270,7 @@ void *read_sha1_file_extended(const unsigned char *sha1,
die("packed object %s (stored in %s) is corrupt",
sha1_to_hex(repl), p->pack_name);
+ unlock_if_threaded(&read_sha1_mutex);
return NULL;
}
diff --git a/thread-utils.c b/thread-utils.c
index fb75a29..70af3f9 100644
--- a/thread-utils.c
+++ b/thread-utils.c
@@ -62,6 +62,17 @@ int init_recursive_mutex(pthread_mutex_t *m)
return ret;
}
+pthread_mutex_t read_sha1_mutex;
+void init_subsystem_locks(void)
+{
+ pthread_mutex_init(&read_sha1_mutex, NULL);
+}
+
+void destroy_subsystem_locks(void)
+{
+ pthread_mutex_destroy(&read_sha1_mutex);
+}
+
#ifndef NO_PTHREADS
void lock_if_threaded(pthread_mutex_t *m)
{
diff --git a/thread-utils.h b/thread-utils.h
index 9a780a2..3906753 100644
--- a/thread-utils.h
+++ b/thread-utils.h
@@ -17,10 +17,16 @@
extern void lock_if_threaded(pthread_mutex_t*);
extern void unlock_if_threaded(pthread_mutex_t*);
+extern pthread_mutex_t read_sha1_mutex;
+extern void init_subsystem_locks(void);
+extern void destroy_subsystem_locks(void);
+
#else
#define lock_if_threaded(lock)
#define unlock_if_threaded(lock)
+#define init_subsystem_locks()
+#define destroy_subsystem_locks()
#endif
--
1.7.8.431.g2abf2
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox