* Re: regression in multi-threaded git-pack-index
From: Thomas Rast @ 2013-03-19 10:29 UTC (permalink / raw)
To: Jeff King
Cc: Thomas Rast, Stefan Zager, git,
Nguyễn Thái Ngọc Duy
In-Reply-To: <20130319102422.GB6341@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> On Tue, Mar 19, 2013 at 06:08:00AM -0400, Jeff King wrote:
>
>> @@ -538,6 +539,8 @@ static void resolve_delta(struct object_entry *delta_obj,
>>
>> delta_obj->real_type = base->obj->real_type;
>> delta_obj->delta_depth = base->obj->delta_depth + 1;
>> + if (deepest_delta < delta_obj->delta_depth)
>> + deepest_delta = delta_obj->delta_depth;
>> delta_obj->base_object_no = base->obj - objects;
>> delta_data = get_data_from_pack(delta_obj);
>> base_data = get_base_data(base);
>>
>> and valgrind reports an uninitialized value in the conditional. But we
>> can see that deepest_delta is static, and therefore always has some
>> value. And delta_obj->delta_depth is set in the line above. So both
>> should have some known value, unless they are computed from unknown
>> values. In that case, shouldn't valgrind have previously noticed when we
>> accessed those unknown values?
>
> Ah, indeed. Putting:
>
> fprintf(stderr, "%lu\n", base->obj->delta_depth);
>
> before the conditional reveals that base->obj->delta_depth is
> uninitialized, which is the real problem. I'm sure there is some
> perfectly logical explanation for why valgrind can't detect its use
> during the assignment, but I'm not sure what it is.
That's simply because you would get far too much noise. It only reports
an uninitialized value when it actually gets used in a conditional or
for output (syscalls), which is when they matter.
You can use --track-origins=yes to see where the undefined value came
from, but it's veeeery slow.
> It looks like the delta_depth value was
> introduced in 38a4556 (index-pack: start learning to emulate
> "verify-pack -v", 2011-06-03), and it used only for showing the chain
> depths with --verify-stat. So it is almost certainly not related to
> Stefan's original problem, but it does mean we've probably been
> computing bogus chain lengths.
Nice catch!
--
Thomas Rast
trast@{inf,student}.ethz.ch
^ permalink raw reply
* Re: regression in multi-threaded git-pack-index
From: Jeff King @ 2013-03-19 10:33 UTC (permalink / raw)
To: Thomas Rast; +Cc: Stefan Zager, git, Nguyễn Thái Ngọc Duy
In-Reply-To: <87obef8yy7.fsf@pctrast.inf.ethz.ch>
On Tue, Mar 19, 2013 at 11:29:36AM +0100, Thomas Rast wrote:
> > Ah, indeed. Putting:
> >
> > fprintf(stderr, "%lu\n", base->obj->delta_depth);
> >
> > before the conditional reveals that base->obj->delta_depth is
> > uninitialized, which is the real problem. I'm sure there is some
> > perfectly logical explanation for why valgrind can't detect its use
> > during the assignment, but I'm not sure what it is.
>
> That's simply because you would get far too much noise. It only reports
> an uninitialized value when it actually gets used in a conditional or
> for output (syscalls), which is when they matter.
Would it? I would think any computation you start with an undefined
value would be suspect (and you would want to know about it as soon as
possible, before the tainted value gets output). I was assuming it was a
performance issue or something.
> You can use --track-origins=yes to see where the undefined value came
> from, but it's veeeery slow.
It's pretty slow either way. :) I do have --track-origins on, but the
origin is this:
objects = xrealloc(objects,
(nr_objects + nr_unresolved + 1)
* sizeof(*objects));
which is not very helpful. It's _somewhere_ in the list of objects...:)
-Peff
^ permalink raw reply
* [PATCH] t7811 (grep-open): remove test_config() redefinition
From: Ramkumar Ramachandra @ 2013-03-19 10:43 UTC (permalink / raw)
To: Git List
test_config() is already a well-defined function in
test-lib-functions.sh. Don't duplicate it unnecessarily.
Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
Found this while randomly grep'ping around.
t/t7811-grep-open.sh | 5 -----
1 file changed, 5 deletions(-)
diff --git a/t/t7811-grep-open.sh b/t/t7811-grep-open.sh
index a895778..e1951a5 100755
--- a/t/t7811-grep-open.sh
+++ b/t/t7811-grep-open.sh
@@ -125,11 +125,6 @@ test_expect_success 'modified file' '
test_cmp empty out
'
-test_config() {
- git config "$1" "$2" &&
- test_when_finished "git config --unset $1"
-}
-
test_expect_success 'copes with color settings' '
rm -f actual &&
echo grep.h >expect &&
--
1.8.2
^ permalink raw reply related
* Re: Memory corruption when rebasing with git version 1.8.1.5 on arch
From: Bernhard Posselt @ 2013-03-19 10:42 UTC (permalink / raw)
To: Jeff King; +Cc: git
In-Reply-To: <20130311051840.GB13510@sigill.intra.peff.net>
Ok, sorrry for not responsding for quite a while, we had the 5.0 release and had too much to do.
I found out why it segfaults: I had a .gitconfig file (sry must have somehow missed that no idea why actually) with the following contents:
http://dpaste.com/1027662/
it seems that the memory corruption does not happen anymore when i change
[apply]
whitespace = fix
to
[apply]
#whitespace = fix
so fixing whitespaces may be the culprit
On 03/11/2013 06:18 AM, Jeff King wrote:
> On Sun, Mar 10, 2013 at 12:45:43PM +0100, Bernhard Posselt wrote:
>
>>> valgrind -q --trace-children=yes --log-file=/tmp/valgrind.out \
>>> git pull --rebase https://github.com/Raydiation/core
>> The log file was empty and it seemed to apply everything nice when
>> running valgrind. When i tried to run it without valgrind it failed
>> with memory corruption.
> Thanks, we are maybe getting closer. It's weird that it works OK with
> valgrind. If the valgrind log is empty, though, I'm confused about where
> the output you pasted below came from.
>
>> ==22291== Invalid write of size 1
>> ==22291== at 0x4C2DB93: memcpy@@GLIBC_2.14 (in
>> /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
>> ==22291== by 0x4076B1: update_pre_post_images (in /usr/lib/git-core/git)
>> ==22291== by 0x40A60F: apply_fragments (in /usr/lib/git-core/git)
>> ==22291== by 0x40C29F: check_patch_list (in /usr/lib/git-core/git)
>> ==22291== by 0x40CC35: apply_patch (in /usr/lib/git-core/git)
>> ==22291== by 0x40F584: cmd_apply (in /usr/lib/git-core/git)
>> ==22291== by 0x4058E7: handle_internal_command (in /usr/lib/git-core/git)
>> ==22291== by 0x404DD1: main (in /usr/lib/git-core/git)
> Hmm, it would be nice to have line numbers. Can you try compiling with
> "-g -O0"?
>
> The function where the problem is deals with whitespace munging. Just a
> guess, but do you have any whitespace config options set (e.g.,
> apply.whitespace)?
>
> -Peff
^ permalink raw reply
* Re: regression in multi-threaded git-pack-index
From: Thomas Rast @ 2013-03-19 10:45 UTC (permalink / raw)
To: Jeff King; +Cc: Stefan Zager, git, Nguyễn Thái Ngọc Duy
In-Reply-To: <20130319103306.GA9490@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> On Tue, Mar 19, 2013 at 11:29:36AM +0100, Thomas Rast wrote:
>
>> > Ah, indeed. Putting:
>> >
>> > fprintf(stderr, "%lu\n", base->obj->delta_depth);
>> >
>> > before the conditional reveals that base->obj->delta_depth is
>> > uninitialized, which is the real problem. I'm sure there is some
>> > perfectly logical explanation for why valgrind can't detect its use
>> > during the assignment, but I'm not sure what it is.
>>
>> That's simply because you would get far too much noise. It only reports
>> an uninitialized value when it actually gets used in a conditional or
>> for output (syscalls), which is when they matter.
>
> Would it? I would think any computation you start with an undefined
> value would be suspect (and you would want to know about it as soon as
> possible, before the tainted value gets output). I was assuming it was a
> performance issue or something.
Now consider
// somewhere on the stack
struct foo {
char c;
int i;
} a, b;
a.c = a.i = 0;
memcpy(&b, &a, sizeof(struct foo));
The compiler could legitimately leave the padding between c and i
uninitialized, and with your proposed "early" reporting the memcpy would
complain.
--
Thomas Rast
trast@{inf,student}.ethz.ch
^ permalink raw reply
* Re: regression in multi-threaded git-pack-index
From: Jeff King @ 2013-03-19 10:47 UTC (permalink / raw)
To: Thomas Rast; +Cc: Stefan Zager, git, Nguyễn Thái Ngọc Duy
In-Reply-To: <871ubb8y6z.fsf@pctrast.inf.ethz.ch>
On Tue, Mar 19, 2013 at 11:45:56AM +0100, Thomas Rast wrote:
> Now consider
>
> // somewhere on the stack
> struct foo {
> char c;
> int i;
> } a, b;
> a.c = a.i = 0;
>
> memcpy(&b, &a, sizeof(struct foo));
>
> The compiler could legitimately leave the padding between c and i
> uninitialized, and with your proposed "early" reporting the memcpy would
> complain.
Ah, good point. And valgrind does not have any way of knowing what is
padding and what is not, since it sees only the compiled contents.
Probably llvm's memory checker support could do a better job there.
-Peff
^ permalink raw reply
* Re: [PATCH 0/8] Improve git-status --ignored
From: Karsten Blees @ 2013-03-19 10:48 UTC (permalink / raw)
To: Duy Nguyen
Cc: Junio C Hamano, Git List, Erik Faye-Lund, Ramkumar Ramachandra,
Robert Zeh, Antoine Pelisse, Adam Spiers
In-Reply-To: <CACsJy8DShce6bXfyWyHk7pqg4PA-cAn1bKh0hgFYX=s486nwaA@mail.gmail.com>
Am 19.03.2013 06:20, schrieb Duy Nguyen:
> On Tue, Mar 19, 2013 at 11:08 AM, Junio C Hamano <gitster@pobox.com> wrote:
>> Karsten Blees <karsten.blees@gmail.com> writes:
>>
>>> This patch series addresses several bugs and performance issues in
>>> .gitignore processing that came up in the inotify discussion.
>>
>> Thanks.
>>
>> How does this interact with the nd/read-directory-recursive-optim
>> topic that has been cooking for a while?
>
> I think 8/8 is another version of nd/read-directory-recursive-optim
>
Yes. When reviewing Duy's patch, I wondered why we would need all those special cases (or: why is treat_file so damn complicated). It turned out that it gets much simpler after fixing some bugs and eliminating the is_excluded / is_path_excluded discrepancy. This variant also optimizes git-status --ignored.
I tried to express my ideas here [1], but I guess this was a bit unstructured, sorry :-)
Note that we could skip excluded checks for tracked directories as well if it weren't for the current notion of ignored tracked directories.
I'd still like to eliminate the second directory scan in git-status --ignored (i.e. don't call fill_directory twice, which would save ~50ms for linux and ~300ms for WebKit), but that's a bit more involved...
[1] http://thread.gmane.org/gmane.comp.version-control.git/217111
^ permalink raw reply
* Re: [PATCH v1 27/45] Convert run_add_interactive to use struct pathspec
From: John Keeping @ 2013-03-19 10:58 UTC (permalink / raw)
To: Duy Nguyen; +Cc: git, Junio C Hamano
In-Reply-To: <CACsJy8AMcfwx5UurNQs1-uJLw_Xt+PAHFU_Tjizf2zOsYECyvg@mail.gmail.com>
On Tue, Mar 19, 2013 at 08:58:23AM +0700, Duy Nguyen wrote:
> On Tue, Mar 19, 2013 at 1:26 AM, John Keeping <john@keeping.me.uk> wrote:
> > On Fri, Mar 15, 2013 at 01:06:42PM +0700, Nguyễn Thái Ngọc Duy wrote:
> >> This passes the pathspec, more or less unmodified, to
> >> git-add--interactive. The command itself does not process pathspec. It
> >> simply passes the pathspec to other builtin commands. So if all those
> >> commands support pathspec, we're good.
> >
> > This breaks "git reset --keep" in a subdirectory for me.
> >
> > I ran "git reset --keep <branch>" in a subdirectory and got:
> >
> > fatal: BUG: parse_pathspec cannot take no argument in this case
> >
> > Bisecting points to this commit.
> >
> > The simplest test case is:
> >
> > ( cd t && ../bin-wrappers/git reset --keep HEAD )
> >
> > which works on master but not pu.
>
> Beautiful. I got messed up with C operator precedence. This should fix
> it. I'll check the rest of parse_pathspec calls later.
Yes, this fixes it. Thanks.
> diff --git a/builtin/reset.c b/builtin/reset.c
> index ab3917d..b665218 100644
> --- a/builtin/reset.c
> +++ b/builtin/reset.c
> @@ -219,7 +219,7 @@ static void parse_args(struct pathspec *pathspec,
> *rev_ret = rev;
> parse_pathspec(pathspec, 0,
> PATHSPEC_PREFER_FULL |
> - patch_mode ? PATHSPEC_PREFIX_ORIGIN : 0,
> + (patch_mode ? PATHSPEC_PREFIX_ORIGIN : 0),
> prefix, argv);
> }
>
> --
> Duy
^ permalink raw reply
* [PATCH] index-pack: always zero-initialize object_entry list
From: Jeff King @ 2013-03-19 10:58 UTC (permalink / raw)
To: Thomas Rast
Cc: Junio C Hamano, Stefan Zager, git,
Nguyễn Thái Ngọc Duy
In-Reply-To: <20130319102422.GB6341@sigill.intra.peff.net>
Commit 38a4556 (index-pack: start learning to emulate
"verify-pack -v", 2011-06-03) added a "delta_depth" counter
to each "struct object_entry". Initially, all object entries
have their depth set to 0; in resolve_delta, we then set the
depth of each delta to "base + 1". Base entries never have
their depth touched, and remain at 0.
To ensure that all depths start at 0, that commit changed
calls to xmalloc the object_entry list into calls to
xcalloc. However, it forgot that we grow the list with
xrealloc later. These extra entries are used when we add an
object from elsewhere pack to complete a thin pack. If we
add a non-delta object, its depth value will just be
uninitialized heap data.
This patch fixes it by zero-initializing entries we add to
the objects list via the xrealloc.
Signed-off-by: Jeff King <peff@peff.net>
---
Another solution would be to say "only look at delta_depth
if the object is a delta"; we follow that rule already in
the output histogram code path, but just do not when
checking a delta's base. So it would similarly be a
one-liner. But I think given the switch to xcalloc in the
original patch, the intent was to just always zero each
object, as I described above.
This would be more readable if we had an "xrecalloc" or
similar, which realloc'd a pointer and set just the _new_
space to zeros. I do not recall ever hearing of such a
function, though. I figured since it is a one-off, it is
simpler to just say what we mean with memset here than
invent a new allocation function that will leave people
scratching their heads about its semantics.
builtin/index-pack.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/builtin/index-pack.c b/builtin/index-pack.c
index 43d364b..ca62443 100644
--- a/builtin/index-pack.c
+++ b/builtin/index-pack.c
@@ -1107,6 +1107,8 @@ static void conclude_pack(int fix_thin_pack, const char *curr_pack, unsigned cha
objects = xrealloc(objects,
(nr_objects + nr_unresolved + 1)
* sizeof(*objects));
+ memset(objects + nr_objects, 0,
+ (nr_unresolved + 1) * sizeof(*objects));
f = sha1fd(output_fd, curr_pack);
fix_unresolved_deltas(f, nr_unresolved);
sprintf(msg, _("completed with %d local objects"),
--
1.8.2.4.g2ed830d
^ permalink raw reply related
* Re: [ITCH] Specify refspec without remote
From: Duy Nguyen @ 2013-03-19 11:33 UTC (permalink / raw)
To: Jeff King; +Cc: Ramkumar Ramachandra, Git List
In-Reply-To: <20130318170804.GA15924@sigill.intra.peff.net>
On Tue, Mar 19, 2013 at 12:08 AM, Jeff King <peff@peff.net> wrote:
>> Is there a reason for the remote not being optional, or are we just
>> waiting for a patch? The only problem I can foresee is very minor:
>> there is a ref with the same name as a remote; in this case, we'd have
>> to specify both the remote and the ref.
>
> I think the ambiguity is a little more complex than that, because we
> cannot enumerate the universe of all remotes. Keep in mind that we can
> take either a configured remote or a URL (or ssh host). So what does:
>
> git push foo:bar
>
> mean? Is it pushing "refs/heads/foo" to "refs/heads/bar" on "origin"? Or
> is it using the default refspecs to push to the "bar" repo on the host
> "foo" over ssh?
>
> So you would need some heuristics based on whether something was a valid
> refspec, or could be a valid remote name or URL.
Assume that we agree on what remote is implied, we could simplify
parsing by specifying the remote with "." (or something short and
unambiguous). So the above command would become
git push . foo:bar
Not too much to type
--
Duy
^ permalink raw reply
* Re: [ITCH] Specify refspec without remote
From: Ramkumar Ramachandra @ 2013-03-19 11:53 UTC (permalink / raw)
To: Duy Nguyen; +Cc: Jeff King, Git List
In-Reply-To: <CACsJy8Ad7rKtMd-6BoBtbVa70F0AaJ+OUjEykNh344tPw7F7Vg@mail.gmail.com>
Duy Nguyen wrote:
> On Tue, Mar 19, 2013 at 12:08 AM, Jeff King <peff@peff.net> wrote:
>>> Is there a reason for the remote not being optional, or are we just
>>> waiting for a patch? The only problem I can foresee is very minor:
>>> there is a ref with the same name as a remote; in this case, we'd have
>>> to specify both the remote and the ref.
>>
>> I think the ambiguity is a little more complex than that, because we
>> cannot enumerate the universe of all remotes. Keep in mind that we can
>> take either a configured remote or a URL (or ssh host). So what does:
>>
>> git push foo:bar
>>
>> mean? Is it pushing "refs/heads/foo" to "refs/heads/bar" on "origin"? Or
>> is it using the default refspecs to push to the "bar" repo on the host
>> "foo" over ssh?
>>
>> So you would need some heuristics based on whether something was a valid
>> refspec, or could be a valid remote name or URL.
>
> Assume that we agree on what remote is implied, we could simplify
> parsing by specifying the remote with "." (or something short and
> unambiguous). So the above command would become
>
> git push . foo:bar
A URL may be a path to a git repository, and '.' is a valid path.
Currently, 'git push .' seems to push to the current repository (what
does that even mean?). For something truly unambiguous, we'll have to
use a character that's disallowed in URLs and isn't interpreted by the
shell- I can't seem to think of one. Otherwise, we'll have to
fallback to using heuristics anyway.
^ permalink raw reply
* Re: [ITCH] Specify refspec without remote
From: Holger Hellmuth (IKS) @ 2013-03-19 11:58 UTC (permalink / raw)
To: Duy Nguyen; +Cc: Jeff King, Ramkumar Ramachandra, Git List
In-Reply-To: <CACsJy8Ad7rKtMd-6BoBtbVa70F0AaJ+OUjEykNh344tPw7F7Vg@mail.gmail.com>
Am 19.03.2013 12:33, schrieb Duy Nguyen:
> git push . foo:bar
'.' has more like a "here" semantic, '..' might be a more fitting
mnemonic here.
^ permalink raw reply
* Re: [ITCH] Specify refspec without remote
From: Duy Nguyen @ 2013-03-19 12:15 UTC (permalink / raw)
To: Ramkumar Ramachandra; +Cc: Jeff King, Git List
In-Reply-To: <CALkWK0nhmks6LqoALA8hrwkR00NjweyqV2RJ9-9V3q-bjgpsCg@mail.gmail.com>
On Tue, Mar 19, 2013 at 6:53 PM, Ramkumar Ramachandra
<artagnon@gmail.com> wrote:
>> git push . foo:bar
>
> A URL may be a path to a git repository, and '.' is a valid path.
> Currently, 'git push .' seems to push to the current repository (what
> does that even mean?). For something truly unambiguous, we'll have to
> use a character that's disallowed in URLs and isn't interpreted by the
> shell- I can't seem to think of one. Otherwise, we'll have to
> fallback to using heuristics anyway.
Yeah that was a stupid suggestion. There's also "-". Right now git
push accepts it as a remote name, but I think in general "-" alone has
always had a special meaning in UNIX world. We could put a new meaning
to it (literal "-" directory can be specified with "./-"). Not totally
sure if "-" is allowed in refspec though.
--
Duy
^ permalink raw reply
* Re: regression in multi-threaded git-pack-index
From: Duy Nguyen @ 2013-03-19 12:35 UTC (permalink / raw)
To: Thomas Rast; +Cc: Jeff King, Stefan Zager, git
In-Reply-To: <87fvzrajmr.fsf@pctrast.inf.ethz.ch>
On Tue, Mar 19, 2013 at 3:17 PM, Thomas Rast <trast@student.ethz.ch> wrote:
>> but the line in question is:
>>
>> if (deepest_delta < delta_obj->delta_depth)
>>
>...
>
> Duy, what was the reasoning why resolve_delta() does not need to hold
> locks when it looks when it looks at deepest_delta? My coffee levels
> aren't up to this task yet. It certainly seems extremely dubious to me,
> as the code uses the global deepest_delta in threaded sections. You can
> probably argue that the load/store is atomic on most(?) platforms, but
> you get no guarantees that deepest_delta at any time in fact holds the
> maximum value of delta_obj->delta_depth.
Now that I have had dinner (and energy restored), the explanation
might be because I missed it. resolve_delta() deals with data in
delta_obj and does not share global state, except this one and
nr_resolved_deltas. The latter is protected. I guess we could protect
this one with a mutex. But only do so when "--verify" is specified
if (stat) {
lock();
if (deepest_delta < delta_obj->delta_depth)
deepest_delta = delta_obj->delta_depth;
unlock();
}
so that we don't need to hold/release lock when index-pack is run.
--
Duy
^ permalink raw reply
* [PATCH] index-pack: protect deepest_delta in multithread code
From: Nguyễn Thái Ngọc Duy @ 2013-03-19 13:01 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, Thomas Rast, Stefan Zager,
Nguyễn Thái Ngọc Duy
In-Reply-To: <CACsJy8DgQZFewPjLSXSkdHHWqhQDqExoVq-pBGpKr1G8w06uvQ@mail.gmail.com>
deepest_delta is a global variable but is updated without protection
in resolve_delta(), a multithreaded function. Add a new mutex for it,
but only protect and update when it's actually used (i.e. show_stat is
non-zero).
Another variable that will not be updated is delta_depth in "struct
object_entry" as it's only useful when show_stat is 1. Putting it in
"if (show_stat)" makes it clearer.
The local variable "stat" is renamed to "show_stat" after moving to
global scope because the name "stat" conflicts with stat(2) syscall.
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
builtin/index-pack.c | 30 +++++++++++++++++++++++-------
1 file changed, 23 insertions(+), 7 deletions(-)
diff --git a/builtin/index-pack.c b/builtin/index-pack.c
index 43d364b..9cfd6e7 100644
--- a/builtin/index-pack.c
+++ b/builtin/index-pack.c
@@ -78,6 +78,7 @@ static int nr_threads;
static int from_stdin;
static int strict;
static int verbose;
+static int show_stat;
static struct progress *progress;
@@ -108,6 +109,10 @@ static pthread_mutex_t work_mutex;
#define work_lock() lock_mutex(&work_mutex)
#define work_unlock() unlock_mutex(&work_mutex)
+static pthread_mutex_t deepest_delta_mutex;
+#define deepest_delta_lock() lock_mutex(&deepest_delta_mutex)
+#define deepest_delta_unlock() unlock_mutex(&deepest_delta_mutex)
+
static pthread_key_t key;
static inline void lock_mutex(pthread_mutex_t *mutex)
@@ -130,6 +135,8 @@ static void init_thread(void)
init_recursive_mutex(&read_mutex);
pthread_mutex_init(&counter_mutex, NULL);
pthread_mutex_init(&work_mutex, NULL);
+ if (show_stat)
+ pthread_mutex_init(&deepest_delta_mutex, NULL);
pthread_key_create(&key, NULL);
thread_data = xcalloc(nr_threads, sizeof(*thread_data));
threads_active = 1;
@@ -143,6 +150,8 @@ static void cleanup_thread(void)
pthread_mutex_destroy(&read_mutex);
pthread_mutex_destroy(&counter_mutex);
pthread_mutex_destroy(&work_mutex);
+ if (show_stat)
+ pthread_mutex_destroy(&deepest_delta_mutex);
pthread_key_delete(key);
free(thread_data);
}
@@ -158,6 +167,9 @@ static void cleanup_thread(void)
#define work_lock()
#define work_unlock()
+#define deepest_delta_lock()
+#define deepest_delta_unlock()
+
#endif
@@ -833,9 +845,13 @@ static void resolve_delta(struct object_entry *delta_obj,
void *base_data, *delta_data;
delta_obj->real_type = base->obj->real_type;
- delta_obj->delta_depth = base->obj->delta_depth + 1;
- if (deepest_delta < delta_obj->delta_depth)
- deepest_delta = delta_obj->delta_depth;
+ if (show_stat) {
+ delta_obj->delta_depth = base->obj->delta_depth + 1;
+ deepest_delta_lock();
+ if (deepest_delta < delta_obj->delta_depth)
+ deepest_delta = delta_obj->delta_depth;
+ deepest_delta_unlock();
+ }
delta_obj->base_object_no = base->obj - objects;
delta_data = get_data_from_pack(delta_obj);
base_data = get_base_data(base);
@@ -1462,7 +1478,7 @@ static void show_pack_info(int stat_only)
int cmd_index_pack(int argc, const char **argv, const char *prefix)
{
- int i, fix_thin_pack = 0, verify = 0, stat_only = 0, stat = 0;
+ int i, fix_thin_pack = 0, verify = 0, stat_only = 0;
const char *curr_pack, *curr_index;
const char *index_name = NULL, *pack_name = NULL;
const char *keep_name = NULL, *keep_msg = NULL;
@@ -1495,10 +1511,10 @@ int cmd_index_pack(int argc, const char **argv, const char *prefix)
verify = 1;
} else if (!strcmp(arg, "--verify-stat")) {
verify = 1;
- stat = 1;
+ show_stat = 1;
} else if (!strcmp(arg, "--verify-stat-only")) {
verify = 1;
- stat = 1;
+ show_stat = 1;
stat_only = 1;
} else if (!strcmp(arg, "--keep")) {
keep_msg = "";
@@ -1606,7 +1622,7 @@ int cmd_index_pack(int argc, const char **argv, const char *prefix)
if (strict)
check_objects();
- if (stat)
+ if (show_stat)
show_pack_info(stat_only);
idx_objects = xmalloc((nr_objects) * sizeof(struct pack_idx_entry *));
--
1.8.2.83.gc99314b
^ permalink raw reply related
* Re: [ITCH] Specify refspec without remote
From: Holger Hellmuth (IKS) @ 2013-03-19 13:03 UTC (permalink / raw)
To: Duy Nguyen; +Cc: Ramkumar Ramachandra, Jeff King, Git List
In-Reply-To: <CACsJy8D10yXoxynOscWjCyAq8qxeXOCMJLqkFYzSRUokUgYF8A@mail.gmail.com>
Would it make sense to allow abbreviation similar to how git objects can
be abbreviated? This would mean origin usually could be spelled just o
^ permalink raw reply
* Re: [PATCH] index-pack: protect deepest_delta in multithread code
From: Jeff King @ 2013-03-19 13:25 UTC (permalink / raw)
To: Nguyễn Thái Ngọc Duy
Cc: git, Junio C Hamano, Thomas Rast, Stefan Zager
In-Reply-To: <1363698075-12452-1-git-send-email-pclouds@gmail.com>
On Tue, Mar 19, 2013 at 08:01:15PM +0700, Nguyen Thai Ngoc Duy wrote:
> deepest_delta is a global variable but is updated without protection
> in resolve_delta(), a multithreaded function. Add a new mutex for it,
> but only protect and update when it's actually used (i.e. show_stat is
> non-zero).
This makes sense to me.
> Another variable that will not be updated is delta_depth in "struct
> object_entry" as it's only useful when show_stat is 1. Putting it in
> "if (show_stat)" makes it clearer.
Having just read through this code for the first time, I agree that
having the "if (show_stat)" would have made it a lot more clear under
what conditions and for what purpose the delta_depth flag was being
used.
> builtin/index-pack.c | 30 +++++++++++++++++++++++-------
> 1 file changed, 23 insertions(+), 7 deletions(-)
Patch looks good to me. Thanks.
-Peff
^ permalink raw reply
* Re: Memory corruption when rebasing with git version 1.8.1.5 on arch
From: Jeff King @ 2013-03-19 13:44 UTC (permalink / raw)
To: Bernhard Posselt; +Cc: Junio C Hamano, git
In-Reply-To: <51484125.9000307@bernhard-posselt.com>
On Tue, Mar 19, 2013 at 11:42:45AM +0100, Bernhard Posselt wrote:
> it seems that the memory corruption does not happen anymore when i change
>
> [apply]
> whitespace = fix
>
> to
>
> [apply]
> #whitespace = fix
>
> so fixing whitespaces may be the culprit
Thanks, I'm able to reproduce with the config you showed. The other key
element seems to be using tab-in-indent. I am not too familiar with
this code, but I was able to get a much smaller reproduction recipe:
-- >8 --
# make tabs more obvious by using "Q" instead
q_to_tab() {
perl -lpe 's/Q/\t/g'
}
q_to_tab >preimage <<\EOF
QQa
QQb
QQc
d
QQe
QQf
QQg
EOF
q_to_tab >patch <<\EOF
diff --git a/preimage b/preimage
--- a/preimage
+++ b/preimage
@@ -1,7 +1,6 @@ public static function store($filename) {
QQa
QQb
QQc
-QQd
QQe
QQf
QQg
EOF
valgrind \
git -c core.whitespace=tab-in-indent apply --whitespace=fix patch
-- 8< --
which yields:
==7112== Invalid write of size 2
==7112== at 0x4C2C023: memcpy (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==7112== by 0x40C365: update_pre_post_images (apply.c:2165)
==7112== by 0x40CC52: match_fragment (apply.c:2402)
[...]
==7112== Address 0x6e57a5e is 0 bytes after a block of size 94 alloc'd
==7112== at 0x4C2A26B: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==7112== by 0x4C2A51F: realloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==7112== by 0x535193: xrealloc (wrapper.c:100)
==7112== by 0x51C322: strbuf_grow (strbuf.c:74)
==7112== by 0x51C10C: strbuf_init (strbuf.c:34)
==7112== by 0x40D329: apply_one_fragment (apply.c:2602)
[...]
and so on. I haven't quite figured out what is going on. It looks like
we call update_pre_post_images with postlen==0, which causes it to just
write the fixed postimage into the existing buffer. But of course the
fixed version is bigger, because we are expanding the tabs into 8
spaces (but it _doesn't_ break if each line starts with only one tab,
which confuses me).
I'm not too familiar with this code. Maybe Junio can say more.
-Peff
^ permalink raw reply
* Re: [PATCH] index-pack: protect deepest_delta in multithread code
From: Thomas Rast @ 2013-03-19 13:50 UTC (permalink / raw)
To: Nguyễn Thái Ngọc Duy
Cc: git, Junio C Hamano, Jeff King, Thomas Rast, Stefan Zager
In-Reply-To: <1363698075-12452-1-git-send-email-pclouds@gmail.com>
Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:
> deepest_delta is a global variable but is updated without protection
> in resolve_delta(), a multithreaded function. Add a new mutex for it,
> but only protect and update when it's actually used (i.e. show_stat is
> non-zero).
>
> Another variable that will not be updated is delta_depth in "struct
> object_entry" as it's only useful when show_stat is 1. Putting it in
> "if (show_stat)" makes it clearer.
>
> The local variable "stat" is renamed to "show_stat" after moving to
> global scope because the name "stat" conflicts with stat(2) syscall.
Looks good to me, too. However, I think it would still be less magical
if we had the below too. It also silences helgrind.
-- >8 --
Subject: [PATCH] index-pack: guard nr_resolved_deltas reads by lock
The threaded parts of index-pack increment the number of resolved
deltas in nr_resolved_deltas guarded by counter_mutex. However, the
per-thread outer loop accessed nr_resolved_deltas without any locks.
This is not wrong as such, since it doesn't matter all that much
whether we get an outdated value. However, unless someone proves that
this one lock makes all the performance difference, it would be much
cleaner to guard _all_ accesses to the variable with the lock.
Signed-off-by: Thomas Rast <trast@student.ethz.ch>
---
builtin/index-pack.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/builtin/index-pack.c b/builtin/index-pack.c
index b3fee45..6be99e2 100644
--- a/builtin/index-pack.c
+++ b/builtin/index-pack.c
@@ -968,7 +968,9 @@ static void *threaded_second_pass(void *data)
for (;;) {
int i;
work_lock();
+ counter_lock();
display_progress(progress, nr_resolved_deltas);
+ counter_unlock();
while (nr_dispatched < nr_objects &&
is_delta_type(objects[nr_dispatched].type))
nr_dispatched++;
--
1.8.2.487.g725f6bb
--
Thomas Rast
trast@{inf,student}.ethz.ch
^ permalink raw reply related
* Re: [PATCH] index-pack: protect deepest_delta in multithread code
From: Duy Nguyen @ 2013-03-19 14:07 UTC (permalink / raw)
To: Thomas Rast; +Cc: git, Junio C Hamano, Jeff King, Stefan Zager
In-Reply-To: <87d2uv7b31.fsf@pctrast.inf.ethz.ch>
On Tue, Mar 19, 2013 at 8:50 PM, Thomas Rast <trast@student.ethz.ch> wrote:
> -- >8 --
> Subject: [PATCH] index-pack: guard nr_resolved_deltas reads by lock
>
> The threaded parts of index-pack increment the number of resolved
> deltas in nr_resolved_deltas guarded by counter_mutex. However, the
> per-thread outer loop accessed nr_resolved_deltas without any locks.
>
> This is not wrong as such, since it doesn't matter all that much
> whether we get an outdated value. However, unless someone proves that
> this one lock makes all the performance difference, it would be much
> cleaner to guard _all_ accesses to the variable with the lock.
>
> Signed-off-by: Thomas Rast <trast@student.ethz.ch>
> ---
> builtin/index-pack.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/builtin/index-pack.c b/builtin/index-pack.c
> index b3fee45..6be99e2 100644
> --- a/builtin/index-pack.c
> +++ b/builtin/index-pack.c
> @@ -968,7 +968,9 @@ static void *threaded_second_pass(void *data)
> for (;;) {
> int i;
> work_lock();
> + counter_lock();
> display_progress(progress, nr_resolved_deltas);
> + counter_unlock();
> while (nr_dispatched < nr_objects &&
> is_delta_type(objects[nr_dispatched].type))
> nr_dispatched++;
I'm pretty sure futex will make this cheap. The only thing I don't
like here is the double locking (work_lock then counter_lock) is an
invitation for potential deadlocks (not now, but who now what can
change later). I think you could move work_lock(); down after
counter_unlock() so we hold one lock at a time.
--
Duy
^ permalink raw reply
* [PATCH v2] index-pack: guard nr_resolved_deltas reads by lock
From: Thomas Rast @ 2013-03-19 14:16 UTC (permalink / raw)
To: Nguyễn Thái Ngọc Duy
Cc: git, Junio C Hamano, Jeff King, Stefan Zager
In-Reply-To: <CACsJy8C0AYvAEm6fJFv+JLWpg3HuFG0erKXnq3NxpkYAy=qb_w@mail.gmail.com>
The threaded parts of index-pack increment the number of resolved
deltas in nr_resolved_deltas guarded by counter_mutex. However, the
per-thread outer loop accessed nr_resolved_deltas without any locks.
This is not wrong as such, since it doesn't matter all that much
whether we get an outdated value. However, unless someone proves that
this one lock makes all the performance difference, it would be much
cleaner to guard _all_ accesses to the variable with the lock.
The only such use is display_progress() in the threaded section (all
others are in the conclude_pack() callchain outside the threaded
part). To make it obvious that it cannot deadlock, move it out of
work_mutex.
Signed-off-by: Thomas Rast <trast@student.ethz.ch>
---
> The only thing I don't
> like here is the double locking (work_lock then counter_lock) is an
> invitation for potential deadlocks (not now, but who now what can
> change later). I think you could move work_lock(); down after
> counter_unlock() so we hold one lock at a time.
Good point.
builtin/index-pack.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/builtin/index-pack.c b/builtin/index-pack.c
index b3fee45..a481f54 100644
--- a/builtin/index-pack.c
+++ b/builtin/index-pack.c
@@ -967,8 +967,10 @@ static void *threaded_second_pass(void *data)
set_thread_data(data);
for (;;) {
int i;
- work_lock();
+ counter_lock();
display_progress(progress, nr_resolved_deltas);
+ counter_unlock();
+ work_lock();
while (nr_dispatched < nr_objects &&
is_delta_type(objects[nr_dispatched].type))
nr_dispatched++;
--
1.8.2.490.gc3bbe62
^ permalink raw reply related
* Re: [BUG?] rebase -i: edit versus unmerged changes
From: Andrew Wong @ 2013-03-19 14:21 UTC (permalink / raw)
To: Ramkumar Ramachandra; +Cc: Git List
In-Reply-To: <CALkWK0n=fb_kq+Ed3rd9MhQUCHY3+_nj=zw4M+GmjbzEO56SOg@mail.gmail.com>
On 3/19/13, Ramkumar Ramachandra <artagnon@gmail.com> wrote:
> I know that this is expected behavior, but is there an easy way to get
> rid of this inconsistency?
You can actually rely on "rebase" to run the appropriate command. In
the first edit commit (the_
no conflict one), I usually run only "git add" to add my changes, then
just run "git rebase --cont"._
And "rebase" will recognize that I'm doing an "edit" and run "git
commit --amend" for me. For the
"unmerged case", I'd do the same "add and continue", and "rebase" will
run "git commit" for__
me.
By doing that, the two scenarios feel a bit more consistent.
^ permalink raw reply
* Re: [PATCH 0/8] Improve git-status --ignored
From: Junio C Hamano @ 2013-03-19 14:48 UTC (permalink / raw)
To: Duy Nguyen
Cc: Karsten Blees, Git List, Erik Faye-Lund, Ramkumar Ramachandra,
Robert Zeh, Antoine Pelisse, Adam Spiers
In-Reply-To: <CACsJy8DShce6bXfyWyHk7pqg4PA-cAn1bKh0hgFYX=s486nwaA@mail.gmail.com>
Duy Nguyen <pclouds@gmail.com> writes:
> On Tue, Mar 19, 2013 at 11:08 AM, Junio C Hamano <gitster@pobox.com> wrote:
>> Karsten Blees <karsten.blees@gmail.com> writes:
>>
>>> This patch series addresses several bugs and performance issues in
>>> .gitignore processing that came up in the inotify discussion.
>>
>> Thanks.
>>
>> How does this interact with the nd/read-directory-recursive-optim
>> topic that has been cooking for a while?
>
> I think 8/8 is another version of nd/read-directory-recursive-optim
Yeah, it seems so; even though the series is much larger, overall it
looks to me a cleaner solution without a specific special case.
Would we we better off kicking nd/read-directory-recursive-optim
back to 'pu' (and eventually ejecting it) and replacing it with a
reroll of Karsten's series when it comes, perhaps?
^ permalink raw reply
* git merge <tag> behavior
From: Yann Droneaud @ 2013-03-19 14:55 UTC (permalink / raw)
To: Git
Hi,
While trying to reproduce/understand the problems[1][2] I was facing
when using Google's Git repo tool[3], I've found minor problems in Git:
1) there's no mention of the git merge <tag> behavior in git-merge.1
When asking Git to merge a tag (such as a signed tag or annotated tag),
it will always create a merge commit even if fast-forward was possible.
It's like having --no-ff present on the command line.
It's a difference from the default behavior described in git-merge.1[4].
It should be documented as an exception of "FAST-FORWARD MERGE" section
and "--ff" option description.
2) git merge <tag> VS git merge <object-id>
If <tag> is an object (not a lightweight/reference tag), git merge <tag>
will by default create a merge commit with the tag message.
Additionally, the signature check will be reported as comment, for
example:
Merge tag 'v1.12.2' into branch-v1.12.2
repo 1.12.2
# gpg: Signature made Fri Mar 1 18:36:42 2013 CET using DSA key ID 920F5C65
# gpg: Good signature from "Repo Maintainer <repo@android.kernel.org>"
# gpg: WARNING: This key is not certified with a trusted signature!
# gpg: There is no indication that the signature belongs to the owner.
# Primary key fingerprint: 8BB9 AD79 3E8E 6153 AF0F 9A44 1653 0D5E 920F 5C65
But, if you use the tag object-id instead of its name, for example using
git merge `git show-ref <tag>`, the tag is not recognized and the
signature is not checked. Git still create a merge commit, but doesn't
prepare a commit message with the tag message and the signature:
Merge commit 'ac22c7ae2e652f63366b65ee23122292d3564fff' into
branch-ac22c7ae2e652f63366b65ee23122292d3564fff
It would be great to have Git using the tag message and check the
signature.
3) Merge options can't be overridden.
If I modify .git/config to set a merge option, for example forcing
fast-forward merge, this option cannot be overridden on command line:
Example 1:
$ cat .git/config:
[branch "master"]
mergeoptions = --ff-only
$ git merge --no-ff <tag>
fatal: You cannot combine --no-ff with --ff-only
Example 2:
$ cat .git/config:
[merge]
ff = only
$ git merge --no-ff <tag>
fatal: You cannot combine --no-ff with --ff-only
Setting the merge options in config should overridden by command line.
Regards.
[1] issue 135: repo: repo sync should force fast-forward merge
https://code.google.com/p/git-repo/issues/detail?id=135
[2] Issue 136: repo: repo sync should use the tag name instead of object identifier of the tag
https://code.google.com/p/git-repo/issues/detail?id=136
[3] git-repo - repo - The multiple repository tool
http://code.google.com/p/git-repo/
[4] git-merge(1) Manual Page
https://www.kernel.org/pub/software/scm/git/docs/git-merge.html
--
Yann Droneaud
OPTEYA
^ permalink raw reply
* Re: [PATCH 3/4] add -u: only show pathless 'add -u' warning when changes exist outside cwd
From: Junio C Hamano @ 2013-03-19 14:57 UTC (permalink / raw)
To: Jonathan Nieder
Cc: Jeff King, Matthieu Moy, git,
Nguyễn Thái Ngọc Duy
In-Reply-To: <20130319052805.GO5062@elie.Belkin>
Jonathan Nieder <jrnieder@gmail.com> writes:
> Yes, that can work, for example like this (replacing the patch you're
> replying to).
I think that would be a better approach if we were to do this. I
still have the same reservation that "this is fundamentally not
worse but still hurts the users more".
> + /*
> + * Check if "git add -A" or "git add -u" was run from a
> + * subdirectory with a modified file outside that directory,
> + * and warn if so.
> + *
> + * "git add -u" will behave like "git add -u :/" instead of
> + * "git add -u ." in the future. This warning prepares for
> + * that change.
> + */
> + if (implicit_dot &&
> + !match_pathspec(implicit_dot, path, strlen(path), 0, NULL)) {
This one really should *not* use match_pathspec(), I think.
It is a special case where we were asked to limit to our directory
but decided to grab everything instead and filtering the outcome
outselves. We should have a "path to the starting directory" aka
"prefix" in implicit_dot and check if path is covered by the prefix
instead.
> + warn_pathless_add();
> + continue;
> + }
> switch (fix_unmerged_status(p, data)) {
> default:
> die(_("unexpected diff status %c"), p->status);
^ 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