* Re: done feature in remote-helpers (was Re: [PATCH 2/2] Add a remote helper to interact with mediawiki (fetch & push))
From: Matthieu Moy @ 2011-08-31 13:12 UTC (permalink / raw)
To: Sverre Rabbelier
Cc: Junio C Hamano, Jonathan Nieder, git, Jeremie Nikaes,
Arnaud Lacurie, Claire Fousse, David Amouyal
In-Reply-To: <CAGdFq_hFPBeogpX0Qp8Knfssw8QzV+GwCAyH+zZhM2iEJfFYMw@mail.gmail.com>
Sverre Rabbelier <srabbelier@gmail.com> writes:
> You can have multiple imports in the current system, you just need to
> remember to add the trailing newline.
>
> import refs/heads/master
> \n
> import refs/heads/next
> \n
Then I'm lost. Isn't \n supposed to mean that the list of commands is
over, and that the remote-helper should terminate?
Also, who is "you" in your sentence? It can't be the remote helper
(which reads this sequence), so it has to be Git's transport-helper. Are
you saying that the transport-helper should be modified to add \n after
sending an import command?
--
Matthieu Moy
http://www-verimag.imag.fr/~moy/
^ permalink raw reply
* Re: done feature in remote-helpers (was Re: [PATCH 2/2] Add a remote helper to interact with mediawiki (fetch & push))
From: Sverre Rabbelier @ 2011-08-31 12:58 UTC (permalink / raw)
To: Matthieu Moy
Cc: Junio C Hamano, Jonathan Nieder, git, Jeremie Nikaes,
Arnaud Lacurie, Claire Fousse, David Amouyal
In-Reply-To: <vpqk49tviza.fsf@bauges.imag.fr>
Heya,
On Wed, Aug 31, 2011 at 14:55, Matthieu Moy
<Matthieu.Moy@grenoble-inp.fr> wrote:
> Err, no, it isn't. From git-remote-testgit.py:
>
> def do_import(repo, args):
> [...]
> while True:
> line = sys.stdin.readline()
> [...]
> # strip of leading 'import '
> ref = line[7:].strip()
> refs.append(ref)
>
> repo = update_local_repo(repo)
> repo.exporter.export_repo(repo.gitdir, refs)
>
> print "done"
>
> What it does is that it reads multiple "import" commands, and process
> them all at once, with a single "print done" at the end. Actually,
> testgit would die("Expected import line.") if Git sent another command
> after "import".
Ah, see, this is why in my original version the syntax was:
import
refs/heads/master
refs/heads/next
\n
Instead of the current:
import refs/heads/master
import refs/heads/next
\n
You can have multiple imports in the current system, you just need to
remember to add the trailing newline.
import refs/heads/master
\n
import refs/heads/next
\n
In the above case you'll have to have two done commands.
--
Cheers,
Sverre Rabbelier
^ permalink raw reply
* Re: done feature in remote-helpers (was Re: [PATCH 2/2] Add a remote helper to interact with mediawiki (fetch & push))
From: Matthieu Moy @ 2011-08-31 12:55 UTC (permalink / raw)
To: Sverre Rabbelier
Cc: Junio C Hamano, Jonathan Nieder, git, Jeremie Nikaes,
Arnaud Lacurie, Claire Fousse, David Amouyal
In-Reply-To: <CAGdFq_jyVK3_THYXzCOLDpNww0Npn2qzZ1qv-BMuLbg1vgVjZw@mail.gmail.com>
Sverre Rabbelier <srabbelier@gmail.com> writes:
> Heya,
>
> On Wed, Aug 31, 2011 at 14:05, Matthieu Moy
> <Matthieu.Moy@grenoble-inp.fr> wrote:
>> So, is this the expected behavior? Wouldn't it be more sensible to allow
>> the remote-helper to issue a "done" after each "import" command? Right
>> now, my understanding is that after an "import" command is issued, it's
>> no longer possible to output anything other than fast-import stream on
>> stdout, and I guess it'd be more future-proof to allow closing the
>> fast-import with a "done", and allow any dialog between git and the
>> remote helper afterwards.
>
> Wow, no that's not the intended behavior. We meant to make it exactly
> as you describe, after each import command you end with a done. This
> is (should) also be what the testgit implementation does currently,
Err, no, it isn't. From git-remote-testgit.py:
def do_import(repo, args):
[...]
while True:
line = sys.stdin.readline()
[...]
# strip of leading 'import '
ref = line[7:].strip()
refs.append(ref)
repo = update_local_repo(repo)
repo.exporter.export_repo(repo.gitdir, refs)
print "done"
What it does is that it reads multiple "import" commands, and process
them all at once, with a single "print done" at the end. Actually,
testgit would die("Expected import line.") if Git sent another command
after "import".
--
Matthieu Moy
http://www-verimag.imag.fr/~moy/
^ permalink raw reply
* Clean termination of remote-helpers (was Re: [PATCH 2/2] Add a remote helper to interact with mediawiki (fetch & push))
From: Matthieu Moy @ 2011-08-31 12:33 UTC (permalink / raw)
To: git; +Cc: gitster, Jeremie Nikaes, Arnaud Lacurie, Claire Fousse,
David Amouyal
In-Reply-To: <1314378689-8997-2-git-send-email-Matthieu.Moy@imag.fr>
Matthieu Moy <Matthieu.Moy@imag.fr> writes:
> +BEGIN { $| = 1 };
> +if (!eof(STDIN)) {
> + # Wait for Git to terminate. If we don't, git fetch
> + # (transport-helper.c's sendline function) will try to write
> + # to our stdin, which will be closed, and git fetch will be
> + # killed. That's probably a bug in transport-helper.c, but in
> + # the meantime ...
> + sleep .1;
> +};
I was expecting this part to be more controversial, so I'm just
repeating it to draw more attention ;-).
I just found a "cleaner" way to terminate, but I still don't find it
really satisfactory:
# Inform Git that we're done, otherwise Git won't close it's stdin,
# and the next loop will be infinite.
close(STDOUT);
# Flush stdin before we terminate. If we don't, git fetch
# (transport-helper.c's sendline function) will try to write to our
# stdin, which may be closed, and git fetch will be killed. That's
# probably a bug in transport-helper.c, but in the meantime ...
while (<STDIN>) {};
This seems reliable (just did 100+ imports without crash).
If I comment-out the "close(STDOUT)", then the while loop is indeed
infinite. If I comment-out the "while (<STDIN>) {};", then the import
unreliably fails (for example, I just did 10 clones of a 1-page wiki,
and got one failure). No error message, just a non-zero exit status, and
in the case of "clone", the newly created repository is deleted before
the command terminates.
With debug activated in transport-helper.c, the last messages are just:
Debug: Disconnecting.
Debug: Remote helper: ->
and gdb says:
Program received signal SIGPIPE, Broken pipe.
0xb7fe2424 in __kernel_vsyscall ()
(gdb) bt
#0 0xb7fe2424 in __kernel_vsyscall ()
#1 0xb7e66ff3 in __write_nocancel () at ../sysdeps/unix/syscall-template.S:82
#2 0x0811dc08 in xwrite (fd=8, buf=0x81bd840, len=1) at wrapper.c:137
#3 0x0811dc67 in write_in_full (fd=8, buf=0x81bd840, count=1) at wrapper.c:169
#4 0x08115035 in sendline (helper=0x81a2d00, buffer=0xbfffe674) at transport-helper.c:41
#5 0x081158f3 in disconnect_helper (transport=0x81a2cc0) at transport-helper.c:231
#6 release_helper (transport=0x81a2cc0) at transport-helper.c:324
#7 0x08111f1d in transport_disconnect (transport=0x81a2cc0) at transport.c:1144
#8 0x08061711 in cmd_clone (argc=3, argv=0xbfffeb58, prefix=0x0) at builtin/clone.c:739
#9 0x0804ba27 in run_builtin (argc=<value optimized out>, argv=<value optimized out>) at git.c:308
#10 handle_internal_command (argc=<value optimized out>, argv=<value optimized out>) at git.c:466
#11 0x0804bc33 in run_argv (argc=3, argv=0xbfffeb58) at git.c:512
#12 main (argc=3, argv=0xbfffeb58) at git.c:585
Any idea how to fix this?
--
Matthieu Moy
http://www-verimag.imag.fr/~moy/
^ permalink raw reply
* Re: done feature in remote-helpers (was Re: [PATCH 2/2] Add a remote helper to interact with mediawiki (fetch & push))
From: Sverre Rabbelier @ 2011-08-31 12:17 UTC (permalink / raw)
To: Matthieu Moy
Cc: Junio C Hamano, Jonathan Nieder, git, Jeremie Nikaes,
Arnaud Lacurie, Claire Fousse, David Amouyal
In-Reply-To: <vpq1uw13hx3.fsf_-_@bauges.imag.fr>
Heya,
On Wed, Aug 31, 2011 at 14:05, Matthieu Moy
<Matthieu.Moy@grenoble-inp.fr> wrote:
> So, is this the expected behavior? Wouldn't it be more sensible to allow
> the remote-helper to issue a "done" after each "import" command? Right
> now, my understanding is that after an "import" command is issued, it's
> no longer possible to output anything other than fast-import stream on
> stdout, and I guess it'd be more future-proof to allow closing the
> fast-import with a "done", and allow any dialog between git and the
> remote helper afterwards.
Wow, no that's not the intended behavior. We meant to make it exactly
as you describe, after each import command you end with a done. This
is (should) also be what the testgit implementation does currently,
and it's what my remote-hg helper does as well. I'm not sure why it's
not working for you, but if it's not then that is definitely a bug,
and not intended behavior.
--
Cheers,
Sverre Rabbelier
^ permalink raw reply
* done feature in remote-helpers (was Re: [PATCH 2/2] Add a remote helper to interact with mediawiki (fetch & push))
From: Matthieu Moy @ 2011-08-31 12:05 UTC (permalink / raw)
To: Sverre Rabbelier
Cc: Junio C Hamano, Jonathan Nieder, git, Jeremie Nikaes,
Arnaud Lacurie, Claire Fousse, David Amouyal
In-Reply-To: <CAGdFq_jwLGZ+tLKramRrLJmwyY_uDtj7JXUwYBO9pSqOmZ20xQ@mail.gmail.com>
Sverre Rabbelier <srabbelier@gmail.com> writes:
> Heya,
>
> On Mon, Aug 29, 2011 at 08:05, Junio C Hamano <gitster@pobox.com> wrote:
>> Does this exchange suggest that at least we would need an update to
>> documentation around "done", as Matthieu's "why is 'done' needed even
>> though I am not calling with --done?" sounds like a very fair question.
>
> No I think the documentation for fast-import is correct. If you pass
> --use-done-feature or print 'feature done' in the stream the use of
> 'done' is required, otherwise it isn't. We did recently changed git to
> pass '--use-done-feature' to the fast-import process though :).
That doesn't help much someone writting a remote helper.
The documentation for remote-helpers neither talks about "done" nor
about "--use-done-feature" or whatever way Git uses this feature when
using remote-helpers.
The current state is particularly confusing: git seems to expect one and
only one "done" feature, even when multiple "import" commands are
issued. That's very strange, and I'm not sure whether it's the expected
behavior (I can try a documentation patch, but I need to understand
better what's expected and what's not).
It would be natural to write remote-helpers like
while ($cmd = <read command>) {
if ($cmd eq "import") {
<write fast-import stream>
print "done\n";
} ...
}
but in the current state, it doesn't work since we'll get a first
"import HEAD", issue a "done", then get a "import refs/heads/master" and
write to a dead pipe.
Then, it would be very tempting to write it like
while ($cmd = <read command>) {
if ($cmd eq "import") {
<write fast-import stream>
} ...
}
print "done\n";
but this doesn't work either, because when calling "git push", no
"import" command is involved, no fast-import is started, and the "done"
breaks everything.
That's why I had to make it like
my $import_started;
while ($cmd = <read command>) {
if ($cmd eq "import") {
$import_started = 1;
<write fast-import stream>
} ...
}
if ($import_started) {
print "done\n";
}
and I really had the feeling I was working around a mis-feature of Git
here.
So, is this the expected behavior? Wouldn't it be more sensible to allow
the remote-helper to issue a "done" after each "import" command? Right
now, my understanding is that after an "import" command is issued, it's
no longer possible to output anything other than fast-import stream on
stdout, and I guess it'd be more future-proof to allow closing the
fast-import with a "done", and allow any dialog between git and the
remote helper afterwards.
Thanks,
--
Matthieu Moy
http://www-verimag.imag.fr/~moy/
^ permalink raw reply
* Re: [PATCH 2/2] Add a remote helper to interact with mediawiki (fetch & push)
From: Matthieu Moy @ 2011-08-31 11:54 UTC (permalink / raw)
To: Jonathan Nieder
Cc: Sverre Rabbelier, Junio C Hamano, git, Jeremie Nikaes,
Arnaud Lacurie, Claire Fousse, David Amouyal,
Ramkumar Ramachandra, Dmitry Ivankov
In-Reply-To: <20110830035608.GB6647@elie.gateway.2wire.net>
Jonathan Nieder <jrnieder@gmail.com> writes:
> Here's an old attempt to make the documentation a little easier to read,
> and hopefully also to add to.
Thanks, that helps. I wish we had all this earlier ;-).
> +Git sends the remote helper a list of commands on standard input, one
> +per line. The first command is always the 'capabilities' command,
Do we want to set this in stone? Wouldn't a Git implementation calling
"option" before "capabilities" be correct?
> +Capabilities
> +~~~~~~~~~~~~
(perhaps name the section "Overview of Capabilities"?)
[...]
> +'refspec' <refspec>::
> + This modifies the 'import' capability, allowing the produced
> + fast-import stream to modify refs in a private namespace
> + instead of writing to refs/heads or refs/remotes directly.
> + It is recommended that all importers providing the 'import'
> + capability use this.
> ++
> +A helper advertising the capability
> +`refspec refs/heads/{asterisk}:refs/svn/origin/branches/{asterisk}`
> +is saying that, when it is asked to `import refs/heads/topic`, the
> +stream it outputs will update the `refs/svn/origin/branches/topic`
> +ref.
> ++
> +This capability can be advertised multiple times. The first
> +applicable refspec takes precedence. The left-hand of refspecs
> +advertised with this capability must cover all refs reported by
> +the list command. If no 'refspec' capability is advertised,
> +there is an implied `refspec {asterisk}:{asterisk}`.
Since this "Capabilities" section is meant to be an overview, I'd
shorten this to
+'refspec' <refspec>::
+ This modifies the 'import' capability, allowing the produced
+ fast-import stream to modify refs in a private namespace
+ instead of writing to refs/heads or refs/remotes directly.
and drop the detailed explanation here.
> +Capabilities for Fetching
> +~~~~~~~~~~~~~~~~~~~~~~~~~
[...]
> +'refspec' <refspec>::
> + This modifies the 'import' capability.
Since this would be the "detailed explanation" part, this is the one
readers will read more carefully, so I'd put the recommandation right
here:
+ It is recommended that all importers providing the 'import'
+ capability use this.
and of course, keep this:
> ++
> +A helper advertising
> +`refspec refs/heads/{asterisk}:refs/svn/origin/branches/{asterisk}`
> +in its capabilities is saying that, when it handles
> +`import refs/heads/topic`, the stream it outputs will update the
> +`refs/svn/origin/branches/topic` ref.
> ++
> +This capability can be advertised multiple times. The first
> +applicable refspec takes precedence. The left-hand of refspecs
> +advertised with this capability must cover all refs reported by
> +the list command. If no 'refspec' capability is advertised,
> +there is an implied `refspec {asterisk}:{asterisk}`.
> +
--
Matthieu Moy
http://www-verimag.imag.fr/~moy/
^ permalink raw reply
* [PATCH] for-each-ref: add split message parts to %(contents:*).
From: Michał Górny @ 2011-08-31 9:11 UTC (permalink / raw)
To: git; +Cc: Jeff King, Michael J Gruber, Michał Górny
In-Reply-To: <4E5CB0D0.7000905@drmicha.warpmail.net>
Now %(contents:subject) contains the message subject, %(contents:body)
main body part and %(contents:signature) GPG signature.
---
Documentation/git-for-each-ref.txt | 7 ++++---
builtin/for-each-ref.c | 22 +++++++++++++++++-----
2 files changed, 21 insertions(+), 8 deletions(-)
diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt
index 152e695..c872b88 100644
--- a/Documentation/git-for-each-ref.txt
+++ b/Documentation/git-for-each-ref.txt
@@ -101,9 +101,10 @@ Fields that have name-email-date tuple as its value (`author`,
`committer`, and `tagger`) can be suffixed with `name`, `email`,
and `date` to extract the named component.
-The first line of the message in a commit and tag object is
-`subject`, the remaining lines are `body`. The whole message
-is `contents`.
+The complete message in a commit and tag object is `contents`.
+Its first line is `contents:subject`, the remaining lines
+are `contents:body` and the optional GPG signature
+is `contents:signature`.
For sorting purposes, fields with numeric values sort in numeric
order (`objectsize`, `authordate`, `committerdate`, `taggerdate`).
diff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c
index 89e75c6..4854ab4 100644
--- a/builtin/for-each-ref.c
+++ b/builtin/for-each-ref.c
@@ -69,6 +69,9 @@ static struct {
{ "subject" },
{ "body" },
{ "contents" },
+ { "contents:subject" },
+ { "contents:body" },
+ { "contents:signature" },
{ "upstream" },
{ "symref" },
{ "flag" },
@@ -458,7 +461,7 @@ static void grab_person(const char *who, struct atom_value *val, int deref, stru
}
}
-static void find_subpos(const char *buf, unsigned long sz, const char **sub, const char **body)
+static void find_subpos(const char *buf, unsigned long sz, const char **sub, const char **body, const char **signature)
{
while (*buf) {
const char *eol = strchr(buf, '\n');
@@ -478,18 +481,20 @@ static void find_subpos(const char *buf, unsigned long sz, const char **sub, con
buf = strchr(buf, '\n');
if (!buf) {
*body = "";
+ *signature = *body;
return; /* no body */
}
while (*buf == '\n')
buf++; /* skip blank between subject and body */
*body = buf;
+ *signature = buf + parse_signature(buf, strlen(buf));
}
/* See grab_values */
static void grab_sub_body_contents(struct atom_value *val, int deref, struct object *obj, void *buf, unsigned long sz)
{
int i;
- const char *subpos = NULL, *bodypos = NULL;
+ const char *subpos = NULL, *bodypos = NULL, *sigpos = NULL;
for (i = 0; i < used_atom_cnt; i++) {
const char *name = used_atom[i];
@@ -500,19 +505,26 @@ static void grab_sub_body_contents(struct atom_value *val, int deref, struct obj
name++;
if (strcmp(name, "subject") &&
strcmp(name, "body") &&
- strcmp(name, "contents"))
+ strcmp(name, "contents") &&
+ strcmp(name, "contents:subject") &&
+ strcmp(name, "contents:body") &&
+ strcmp(name, "contents:signature"))
continue;
if (!subpos)
- find_subpos(buf, sz, &subpos, &bodypos);
+ find_subpos(buf, sz, &subpos, &bodypos, &sigpos);
if (!subpos)
return;
- if (!strcmp(name, "subject"))
+ if (!strcmp(name, "subject") || !strcmp(name, "contents:subject"))
v->s = copy_line(subpos);
else if (!strcmp(name, "body"))
v->s = xstrdup(bodypos);
else if (!strcmp(name, "contents"))
v->s = xstrdup(subpos);
+ else if (!strcmp(name, "contents:body"))
+ v->s = xstrndup(bodypos, sigpos - bodypos);
+ else if (!strcmp(name, "contents:signature"))
+ v->s = xstrdup(sigpos);
}
}
--
1.7.6.1
^ permalink raw reply related
* [PATCH] sha1_file: Remove relative entries limitation
From: Hui Wang @ 2011-08-31 6:41 UTC (permalink / raw)
To: gitster, git
link_alt_odb_entries() will be called recursively if alternates has
valid object store paths, and to avoid nesting too deep, the
recursive depth is limited to 5, this limitation is reasonable and
safe for dead-loop reference situation.
There is another limitation in this function to only permit the 1st
level alternates has relative paths, but there is no foreseeable
greater risk using relative paths in 2nd/3rd... level alternates than
using absolute paths, in addition to we already have max depth 5
limitation, we can safely remove this limitation.
Moreover removing this limitation will make below two usage workable.
usage1: base-repos has relative path in the alternates
%>git clone --reference base-repos src dest
usage2: src2 has relative path to point src1, src1 has relative path
to point src
%>git clone src2 dest
Signed-off-by: Hui Wang <jason77.wang@gmail.com>
---
sha1_file.c | 13 ++++---------
1 files changed, 4 insertions(+), 9 deletions(-)
diff --git a/sha1_file.c b/sha1_file.c
index f7c3408..4130ca0 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -332,15 +332,10 @@ static void link_alt_odb_entries(const char *alt, const char *ep, int sep,
}
while (cp < ep && *cp != sep)
cp++;
- if (last != cp) {
- if (!is_absolute_path(last) && depth) {
- error("%s: ignoring relative alternate object store %s",
- relative_base, last);
- } else {
- link_alt_odb_entry(last, cp - last,
- relative_base, depth);
- }
- }
+ if (last != cp)
+ link_alt_odb_entry(last, cp - last,
+ relative_base, depth);
+
while (cp < ep && *cp == sep)
cp++;
last = cp;
--
1.7.6
^ permalink raw reply related
* Re: [PATCH] grep: Fix race condition in delta_base_cache
From: Nicolas Morey-Chaisemartin @ 2011-08-31 6:32 UTC (permalink / raw)
To: Jeff King; +Cc: Junio C Hamano, git
In-Reply-To: <20110831015936.GB2519@sigill.intra.peff.net>
On 08/31/2011 03:59 AM, Jeff King wrote:
>
> I notice there are some other code paths that end up in xmalloc without
> locking, too (e.g., load_file, and some strbuf_* calls). Don't those
> need locking, too, as malloc may try to release packfile memory?
>
After some consideration I think they do.
Grep threads definitly get the try_to_free function registered while the main one does at least some xreallock out of sha1_file.c (so not owning the lock).
I guess it requires the same kind of lock pack-objects.c uses (meaning we need to set the try_to_free function in grep.c too).
>
> [1] Actually, it looks like the "try_to_free" routine starts as nothing,
> and then add_packed_git sets it lazily to try_to_free_pack_memory.
> But what builtin/pack-objects tries to do is overwrite that with a
> version of try_to_free_pack_memory that does locking. So it's
> possible that we would not have read any packed objects while
> setting up the threads, and add_packed_git will overwrite our
> careful, locking version of try_to_free_pack_memory.
>
> I _think_ pack-objects is probably OK, because it will have already
> done the complete "counting objects" phase, which would look in any
> packs. But it may be harder for grep.
I'm not expert enough in git pathways to be sure about pack_objects but I agree it looks "risky".
Maybe add_packed_git should check whether there already is a free routine (other than do_nothing) instead of simply setting it up the first time.
Nicolas Morey-Chaisemartin
^ permalink raw reply
* [PATCH] xdiff/xprepare: initialise xdlclassifier_t cf in xdl_prepare_env()
From: Tay Ray Chuan @ 2011-08-31 4:48 UTC (permalink / raw)
To: Git Mailing List; +Cc: Junio C Hamano
Ensure that the xdl_free_classifier() call on xdlclassifier_t cf is safe
even if xdl_init_classifier() isn't called. This may occur in the case
where diff is run with --histogram and a call to, say, xdl_prepare_ctx()
fails.
Signed-off-by: Tay Ray Chuan <rctay89@gmail.com>
---
This should go into 'rc/histogram-diff' in 'next'.
xdiff/xprepare.c | 3 +++
1 files changed, 3 insertions(+), 0 deletions(-)
diff --git a/xdiff/xprepare.c b/xdiff/xprepare.c
index 620fc9a..4323596 100644
--- a/xdiff/xprepare.c
+++ b/xdiff/xprepare.c
@@ -239,6 +239,9 @@ int xdl_prepare_env(mmfile_t *mf1, mmfile_t *mf2, xpparam_t const *xpp,
long enl1, enl2, sample;
xdlclassifier_t cf;
+ cf.rchash = NULL;
+ cf.ncha.head = NULL;
+
/*
* For histogram diff, we can afford a smaller sample size and
* thus a poorer estimate of the number of lines, as the hash
--
1.7.6.1.706.gaa5cf
^ permalink raw reply related
* git credential helper design [was: What's cooking in git.git (Aug 2011, #07; Wed, 24)]
From: Jeff King @ 2011-08-31 2:38 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Lukas Sandström, Ted Zlatanov, git
In-Reply-To: <7vhb55i11i.fsf@alter.siamese.dyndns.org>
On Thu, Aug 25, 2011 at 03:22:49PM -0700, Junio C Hamano wrote:
> > I'm OK with holding this off for another round. I'd really like to get
> > more feedback from third-party helper writers. ...
>
> I actually do not think the lack of finer-than-host level granularity a
> problem we need to solve before moving forward. IIRC, when accessing
> "http://github.com/frotz" and "http://github.com/nitfol", you would key
> the authentication material with something like "http://github.com" (or
> was it "http:github.com"? the details do not matter for the purpose of
> this discussion).
It's "http:github.com", which has always looked a bit ugly to me. I had
hoped they would just be opaque blobs and nobody would need to look at
them. But when you get into things like setting the username via the
config, then users see them, and they need to look sane. Making them
look more like a canonicalized URL is probably a good thing.
After seeing the helper that Lukas posted recently on the list, I am
wondering if they should include the username, too. I had left it
separate, because I wanted helpers to be able to index "foo@example.com"
and "example.com" in the same slot. I.e., to realize that the latter
could use the same credentials cached for the former. But it also
complicates the helpers; instead of doing:
credential = secure_storage_lookup(unique_token);
return credential /* could be NULL */
they have to do:
for credential in secure_storage_lookup(unique_token) {
if (!username)
return credential; /* take first one arbitrarily */
else if (username == credential.username)
return credential; /* ok, matched preferred username */
}
return NULL;
which implies that the secure storage can even store a list indexed
under the token.
So perhaps a better model is to give the helper some canonicalized URL,
like:
https://foo@example.com
(where the canonicalization is important, because we want the helper to
be able to just treat it like a string of bytes if it wants). And then
we can naturally extend that to:
https://foo@example.com/specific-repo.git
if the user wants a repo-specific authentication context.
> We can consider what you already have as the default case for a more
> general "we cut off at the hostname and take that as the auth-domain
> boundary unless told otherwise". We may not have the way to "tell
> otherwise" yet, but as long as we are reasonably confident that we know
> how to extend the system in a backward compatible way, it is not a
> show-stopper.
I think in either case it gets tacked onto the auth token. But it
probably makes sense now to choose a nice syntax for the token that will
look good when we extend it later.
Ted wrote:
> How about a config variable with regular expressions like
>
> auth-domain.xyz.url = https://(.*@)?github.com/.*
I like this. In fact, perhaps it makes sense for git to generate the
maximal token, like:
https://user@host.example.com/path/to/repo.git
and then provide the user with configuration like this to narrow it down
as they see fit. Perhaps even do a substitution regexp to let them
rewrite it arbitrarily. And then if we want to be more permissive by
default, provide some backup regexps to be used when they don't provide
their own, like cutting out the pathname portion.
-Peff
^ permalink raw reply
* Re: What's cooking in git.git (Aug 2011, #07; Wed, 24)
From: Jeff King @ 2011-08-31 2:20 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <7vhb55i11i.fsf@alter.siamese.dyndns.org>
On Thu, Aug 25, 2011 at 03:22:49PM -0700, Junio C Hamano wrote:
> > On Wed, Aug 24, 2011 at 05:09:09PM -0700, Junio C Hamano wrote:
> >
> >> * jk/add-i-hunk-filter (2011-07-27) 5 commits
> >> (merged to 'next' on 2011-08-11 at 8ff9a56)
> >> + add--interactive: add option to autosplit hunks
> >> + add--interactive: allow negatation of hunk filters
> >> + add--interactive: allow hunk filtering on command line
> >> + add--interactive: factor out regex error handling
> >> + add--interactive: refactor patch mode argument processing
> >>
> >> Needs documentation updates.
> >
> > I think Duy already mentioned this, but you may want to update your
> > "what's cooking" note: it needs not just doc updates, but code to
> > actually pass the options along from real git commands that use
> > add--interactive, like add, checkout, reset, and stash.
>
> Thanks. Also tests are lacking, too. Although I do not necessarily see the
> lack of integration with anything but "add" a show-stopper (I consider
> "-p" to chekout, reset and stash are "nice to have"), [...]
It is less ready than that. You cannot even use it from "git add" at
this point. It is _only_ the perl bits, as I was just providing them to
Duy, so he could write the C bits. So the patches as they are are
useless. Hence no tests, since you can't even trigger the code without
artifically calling add--interactive directly with the new options.
So it probably makes sense to just drop them (or just leave them in pu)
for the next cycle until the other half materializes.
> you are correct that "add -i" and then choosing '[p]atch' gets very
> confused with
Hmm, that is a regression probably caused by my refactoring. Thanks for
pointing it out. I'll take a look.
> >> The initial "tag --contains" de-pessimization without need for generation
> >> numbers is already in; backburnered.
> >
> > So...what next? I don't really like leaving the contains traversal
> > as-is.
>
> Hmm, honestly speaking, I do not see much problem with it. My knee-jerk
> reaction is to go with 1.a and if we really want to do something 1.b
> perhaps but I suspect "these are bogus" cache wouldn't be so useful by
> itself and we may need a bit more information.
OK. I'll clean up and submit a patch for that, but I'll wait for
post-1.7.7.
-Peff
^ permalink raw reply
* Re: [PATCH] grep: Fix race condition in delta_base_cache
From: Jeff King @ 2011-08-31 1:59 UTC (permalink / raw)
To: Nicolas Morey-Chaisemartin; +Cc: Junio C Hamano, git
In-Reply-To: <4E5CE982.7080200@morey-chaisemartin.com>
On Tue, Aug 30, 2011 at 03:45:38PM +0200, Nicolas Morey-Chaisemartin wrote:
> According to gdb the problem originate from release_delta_cash (sha1_file.c:1703)
> free(ent->data);
>
> From my analysis it seems that git grep threads do acquire lock before
> calling read_sha1_file but not before calling
> read_object_with_reference who ends up calling read_sha1_file too.
Yeah, I think this is necessary, and the patch looks good.
I notice there are some other code paths that end up in xmalloc without
locking, too (e.g., load_file, and some strbuf_* calls). Don't those
need locking, too, as malloc may try to release packfile memory?
builtin/pack-objects.c dealt with this already by setting a new
"try_to_free" routine that locks[1], which we should also do. It
probably comes up less frequently, because it only happens when we're
under memory pressure.
-Peff
[1] Actually, it looks like the "try_to_free" routine starts as nothing,
and then add_packed_git sets it lazily to try_to_free_pack_memory.
But what builtin/pack-objects tries to do is overwrite that with a
version of try_to_free_pack_memory that does locking. So it's
possible that we would not have read any packed objects while
setting up the threads, and add_packed_git will overwrite our
careful, locking version of try_to_free_pack_memory.
I _think_ pack-objects is probably OK, because it will have already
done the complete "counting objects" phase, which would look in any
packs. But it may be harder for grep.
^ permalink raw reply
* Re: [PATCH] Add a credential-helper for KDE
From: Jeff King @ 2011-08-31 1:42 UTC (permalink / raw)
To: Lukas Sandström; +Cc: Git Mailing List
In-Reply-To: <4E594B5A.6070902@gmail.com>
On Sat, Aug 27, 2011 at 09:54:02PM +0200, Lukas Sandström wrote:
> This Python script plugs into the credentials API
> of Git to ask the user for passwords with a nice
> KDE password dialog.
Thanks for working on this.
> .../git-kde-credentials-helper.py | 122 ++++++++++++++++++++
Can we call it git-credential-kdewallet or similar? Then users can just
do:
git config credential.helper kdewallet
(where "kdewallet" can be whatever you think is most appropriate; the
key is naming it git-credential-*).
> 1 files changed, 122 insertions(+), 0 deletions(-)
> create mode 100755 contrib/kde-credetials-helper/git-kde-credentials-helper.py
Minor typo in directory name.
> + def check_wallet(self):
> + (res, data) = self.wallet.readMap(self.token)
> + if res != 0:
> + return None
> + try:
> + self.username = data[QString("username")]
> + self.password = data[QString("password")]
> + except KeyError:
> + return None
> + return self.username and self.password
If I am reading this correctly, you look up based purely on the context
token. Which means that if I do something like this:
$ git push https://host.com/repo.git
[enter username: user1, password: foo]
$ git push https://user2@host.com/other-repo.git
We will invoke the helper as:
git credential-kdewallet --unique=https:host.com --username=user2
but the helper will ignore the "user2" bit, and return "user1 / foo".
The "cache" helper I wrote handles this situation better, by indexing
both on the token and the username. I wonder if the username should
become part of the token. Or if the token should really just become a
canonicalized URL, minus the actual path. So the first one would get:
--unique=https://host.com
and the second would get:
--unique=https://user2@host.com
Then helpers wouldn't need to worry about doing anything special.
What do you think? Also, any comments in general on writing a helper?
You are the first one besides me to do so. Did you find anything in the
interface or the documentation confusing? Suggestions are very welcome,
as nothing has been released yet and we're free to tweak as much as we
want.
-Peff
^ permalink raw reply
* Re: [PATCH 1/3] traverse_trees(): allow pruning with pathspec
From: Nguyen Thai Ngoc Duy @ 2011-08-31 1:35 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Linus Torvalds
In-Reply-To: <7v39gi3ib6.fsf@alter.siamese.dyndns.org>
On Wed, Aug 31, 2011 at 12:44 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Nguyen Thai Ngoc Duy <pclouds@gmail.com> writes:
>
>>> @@ -376,16 +396,22 @@ int traverse_trees(int n, struct tree_desc *t, struct traverse_info *info)
>>> mask |= 1ul << i;
>>> if (S_ISDIR(entry[i].mode))
>>> dirmask |= 1ul << i;
>>> + e = &entry[i];
>>> }
>>
>> Why? "e" is not used in that loop or anywhere after that.
>
> This is trying to find _a_ surviving entry to be fed to prune_traversal()
> which in turn uses tree_entry_interesting(). At this point in the code, we
> are stuffing the entries of the same name from the input trees (and if one
> tree is missing an entry of the chosen name, it will have NULL there), so
> any non-empty entry would do. It corresponds to "first" but that is just a
> simple string and not a name_entry tree_entry_interesting() wants.
Ah yes. I only searched in old code, "e" is used in the new
prune_traversal() call.
>>> if (!mask)
>>> break;
>>> - ret = info->fn(n, mask, dirmask, entry, info);
>>> - if (ret < 0) {
>>> - error = ret;
>>> - if (!info->show_all_errors)
>>> - break;
>>> + interesting = prune_traversal(e, info, &base, interesting);
>>> + if (interesting < 0)
>>> + break;
>>
>> I don't really understand this function to comment. But I guess when
>> interesting < 0, we only skip info->fn() and assume it returns "mask"
>> (its user unpack_callback() only returns either "mask" or -1).
>
> We consume the entries we have used in merging (which is actually
> "everything in entry[] array" as info->fn() returns "mask" itself) by
> saying "update_extended_entry()" and the purpose of doing so is to prepare
> to process the next entry of the tree we are traversing.
>
> When tree_entry_interesting() returns negative, it tells us "no, and no
> subsequent entries will be either", meaning "we are done with this tree".
> As we are done, there is nothing to prepare for the next round; we are not
> walking the remaining entries in the trees we are looking at. Is there any
> point in calling update_extended_entry() I am missing?
No you're right again. Somehow I thought there would be another round
in the loop (ie. continue, not break). My bad.
--
Duy
^ permalink raw reply
* Not yet 1.7.7-rc1 but... (take 2)
From: Junio C Hamano @ 2011-08-31 0:59 UTC (permalink / raw)
To: git
In-Reply-To: <7v62lf4ok0.fsf_-_@alter.siamese.dyndns.org>
Junio C Hamano <gitster@pobox.com> writes:
... here is an updated preview of the "New Topics" section of the next
issue of "What's cooking". People sent in fixes to bugs several topics
that are new in 1.7.7-rc0 have introduced, and they are being fast
tracked for 1.7.7-rc1, which probably will happen tomorrow.
Thanks.
-- >8 --
[New Topics]
* mg/maint-notes-C-doc (2011-08-25) 1 commit
(merged to 'next' on 2011-08-30 at 6f3281a)
+ git-notes.txt: clarify -C vs. copy and -F
Will merge to "master".
* jc/clean-exclude-doc (2011-08-28) 1 commit
(merged to 'next' on 2011-08-30 at 79dd173)
+ Documentation: clarify "git clean -e <pattern>"
Will merge to "master".
* mh/check-ref-format-print-normalize (2011-08-27) 2 commits
(merged to 'next' on 2011-08-30 at e827410)
+ Forbid DEL characters in reference names
+ check-ref-format --print: Normalize refnames that start with slashes
Will merge to "master".
* bk/ancestry-path (2011-08-25) 3 commits
- revision: do not include sibling history in --ancestry-path output
- revision: keep track of the end-user input from the command line
- rev-list: Demonstrate breakage with --ancestry-path --all
The topic came up a bit too late in the cycle.
Will cook for a while.
* mg/branch-list (2011-08-28) 5 commits
- branch: allow pattern arguments
- branch: introduce --list option
- git-branch: introduce missing long forms for the options
- git-tag: introduce long forms for the options
- t6040: test branch -vv
Not urgent; the topic came up a bit too late in the cycle.
* mm/rebase-i-exec-edit (2011-08-26) 2 commits
- rebase -i: notice and warn if "exec $cmd" modifies the index or the working tree
- rebase -i: clean error message for --continue after failed exec
Not urgent; the topic came up a bit too late in the cycle.
* jk/default-attr (2011-08-26) 1 commit
- attr: map builtin userdiff drivers to well-known extensions
Not urgent; I fixed up the test breakage just for fun.
* hv/submodule-merge-search (2011-08-26) 4 commits
- submodule: Search for merges only at end of recursive merge
- allow multiple calls to submodule merge search for the same path
- submodule: Demonstrate known breakage during recursive merge
- push: Don't push a repository with unpushed submodules
(this branch uses fg/submodule-ff-check-before-push.)
Not urgent; the topic came up a bit too late in the cycle.
The bottom one needs to be replaced with a properly written commit log message.
* mm/mediawiki-as-a-remote (2011-08-26) 1 commit
- Add a remote helper to interact with mediawiki (fetch & push)
Fun.
Not urgent; the topic came up a bit too late in the cycle.
* nd/maint-autofix-tag-in-head (2011-08-26) 3 commits
- Accept tags in HEAD or MERGE_HEAD
- merge: remove global variable head[]
- merge: keep stash[] a local variable
Probably needs a re-roll to aim a bit higher.
Not urgent; will not be in 1.7.7.
* bc/unstash-clean-crufts (2011-08-27) 4 commits
- git-stash: remove untracked/ignored directories when stashed
- t/t3905: add missing '&&' linkage
- git-stash.sh: fix typo in error message
- t/t3905: use the name 'actual' for test output, swap arguments to test_cmp
Not urgent; the topic came up a bit too late in the cycle.
* bg/t5540-osx-grep (2011-08-28) 1 commit
(merged to 'next' on 2011-08-30 at 894af05)
+ t5540-http-test: shorten grep pattern
Will merge to "master".
* cb/maint-ls-files-error-report (2011-08-28) 1 commit
(merged to 'next' on 2011-08-30 at b606e19)
+ t3005: do not assume a particular order of stdout and stderr of git-ls-files
Will merge to "master".
* da/make-auto-header-dependencies (2011-08-30) 1 commit
- Makefile: Improve compiler header dependency check
(this branch uses fk/make-auto-header-dependencies.)
Not urgent; will not be in 1.7.7.
* ms/daemon-timeout-is-in-seconds (2011-08-28) 1 commit
(merged to 'next' on 2011-08-30 at 2462eb0)
+ git-daemon.txt: specify --timeout in seconds
Will merge to "master".
* bc/bisect-test-use-shell-path (2011-08-30) 1 commit
(merged to 'next' on 2011-08-30 at c090151)
+ t6030: use $SHELL_PATH to invoke user's preferred shell instead of bare sh
Will merge to "master".
* gb/am-hg-patch (2011-08-29) 1 commit
- am: preliminary support for hg patches
Not urgent; the topic came up a bit too late in the cycle.
* gb/maint-am-patch-format-error-message (2011-08-29) 1 commit
(merged to 'next' on 2011-08-30 at ed01a1d)
+ am: format is in $patch_format, not parse_patch
Will merge to "master".
* gb/maint-am-stgit-author-to-from-fix (2011-08-29) 1 commit
(merged to 'next' on 2011-08-30 at 0740010)
+ am: fix stgit patch mangling
Will merge to "master".
* jc/diff-index-unpack (2011-08-29) 3 commits
- diff-index: pass pathspec down to unpack-trees machinery
- unpack-trees: allow pruning with pathspec
- traverse_trees(): allow pruning with pathspec
Will cook for a while.
* tr/maint-format-patch-empty-output (2011-08-29) 4 commits
(merged to 'next' on 2011-08-30 at 078c522)
+ Document negated forms of format-patch --to --cc --add-headers
+ t4014: "no-add-headers" is actually called "no-add-header"
+ t4014: invoke format-patch with --stdout where intended
+ t4014: check for empty files from git format-patch --stdout
Will merge to "master".
* tr/maint-ident-to-git-memmove (2011-08-29) 1 commit
(merged to 'next' on 2011-08-30 at 9395a9b)
+ Use memmove in ident_to_git
Will merge to "master".
* tr/maint-strbuf-grow-nul-termination (2011-08-29) 1 commit
(merged to 'next' on 2011-08-30 at dc87192)
+ strbuf_grow(): maintain nul-termination even for new buffer
Will merge to "master".
* tr/maint-t3903-misquoted-command (2011-08-30) 1 commit
(merged to 'next' on 2011-08-30 at f533857)
+ t3903: fix misquoted rev-parse invocation
Will merge to "master".
* va/p4-branch-import-test-update (2011-08-29) 1 commit
(merged to 'next' on 2011-08-30 at 5c54fb3)
+ git-p4: simple branch tests edits
Will merge to "master".
* jn/remote-helpers-doc (2011-08-29) 1 commit
- Documentation/remote-helpers: explain capabilities first
Not urgent; it would however be a good starting point.
* nm/grep-object-sha1-lock (2011-08-30) 1 commit
- grep: Fix race condition in delta_base_cache
Not urgent; the topic came up a bit too late in the cycle.
* tr/mergetool-valgrind (2011-08-30) 1 commit
- Symlink mergetools scriptlets into valgrind wrappers
Not urgent; the topic came up a bit too late in the cycle.
* js/i18n-scripts-2 (2011-08-30) 1 commit
(merged to 'next' on 2011-08-30 at 5a144a2)
+ bisect: take advantage of gettextln, eval_gettextln.
Will merge to "master".
^ permalink raw reply
* Re: [PATCH 0/3] Un-pessimize "diff-index $commit -- $pathspec"
From: Junio C Hamano @ 2011-08-31 0:18 UTC (permalink / raw)
To: Marat Radchenko; +Cc: git
In-Reply-To: <loom.20110830T081442-547@post.gmane.org>
Marat Radchenko <marat@slonopotamus.org> writes:
> Junio C Hamano <gitster <at> pobox.com> writes:
>> Marat, if/when you have a chance could you try a patched git on your
>> original use case and see if it produces correct output with shorter
>> amount of time?
>
> 30s without patch and 0.3s with it.
Thanks.
^ permalink raw reply
* Re: [PATCH v2] Make use of git status when autocompleting git add, rm, checkout --, and reset HEAD
From: Junio C Hamano @ 2011-08-30 23:14 UTC (permalink / raw)
To: Ron Panduwana; +Cc: git, Shawn O. Pearce, Lee Marlow, Thomas Rast
In-Reply-To: <1314740583-14567-1-git-send-email-panduwana@gmail.com>
Ron Panduwana <panduwana@gmail.com> writes:
> Signed-off-by: Ron Panduwana <panduwana@gmail.com>
"Make use of" is something anybody can read from the patch. What we need
from the proposed commit log message above S-o-b: line is to justify why
it is a good change. Does it make the code simpler to follow by making it
shorter? Does it make it faster to complete, and if so by how much faster?
Does it make it easier to use by not including paths that it used to show,
and if so what are the differences the end users would see, and why is it
justified to omit these paths from the candidate set?
^ permalink raw reply
* [PATCH] bisect: take advantage of gettextln, eval_gettextln.
From: Jon Seymour @ 2011-08-30 23:09 UTC (permalink / raw)
To: git; +Cc: gitster, Jon Seymour
Signed-off-by: Jon Seymour <jon.seymour@gmail.com>
---
git-bisect.sh | 49 ++++++++++++++-----------------------------------
1 files changed, 14 insertions(+), 35 deletions(-)
Now that both dependencies have been merged into master, this patch is ready
to be merged.
diff --git a/git-bisect.sh b/git-bisect.sh
index e0ca3fb..2524060 100755
--- a/git-bisect.sh
+++ b/git-bisect.sh
@@ -45,10 +45,7 @@ bisect_head()
bisect_autostart() {
test -s "$GIT_DIR/BISECT_START" || {
- (
- gettext "You need to start by \"git bisect start\"" &&
- echo
- ) >&2
+ gettextln "You need to start by \"git bisect start\"" >&2
if test -t 0
then
# TRANSLATORS: Make sure to include [Y] and [n] in your
@@ -272,10 +269,7 @@ bisect_next_check() {
t,,good)
# have bad but not good. we could bisect although
# this is less optimum.
- (
- gettext "Warning: bisecting only with a bad commit." &&
- echo
- ) >&2
+ gettextln "Warning: bisecting only with a bad commit." >&2
if test -t 0
then
# TRANSLATORS: Make sure to include [Y] and [n] in your
@@ -291,18 +285,12 @@ bisect_next_check() {
if test -s "$GIT_DIR/BISECT_START"
then
- (
- gettext "You need to give me at least one good and one bad revisions.
-(You can use \"git bisect bad\" and \"git bisect good\" for that.)" &&
- echo
- ) >&2
+ gettextln "You need to give me at least one good and one bad revisions.
+(You can use \"git bisect bad\" and \"git bisect good\" for that.)" >&2
else
- (
- gettext "You need to start by \"git bisect start\".
+ gettextln "You need to start by \"git bisect start\".
You then need to give me at least one good and one bad revisions.
-(You can use \"git bisect bad\" and \"git bisect good\" for that.)" &&
- echo
- ) >&2
+(You can use \"git bisect bad\" and \"git bisect good\" for that.)" >&2
fi
exit 1 ;;
esac
@@ -355,7 +343,7 @@ bisect_visualize() {
bisect_reset() {
test -s "$GIT_DIR/BISECT_START" || {
- gettext "We are not bisecting."; echo
+ gettextln "We are not bisecting."
return
}
case "$#" in
@@ -428,18 +416,15 @@ bisect_run () {
while true
do
command="$@"
- eval_gettext "running \$command"; echo
+ eval_gettextln "running \$command"
"$@"
res=$?
# Check for really bad run error.
if [ $res -lt 0 -o $res -ge 128 ]
then
- (
- eval_gettext "bisect run failed:
-exit code \$res from '\$command' is < 0 or >= 128" &&
- echo
- ) >&2
+ eval_gettextln "bisect run failed:
+exit code \$res from '\$command' is < 0 or >= 128" >&2
exit $res
fi
@@ -464,26 +449,20 @@ exit code \$res from '\$command' is < 0 or >= 128" &&
if sane_grep "first bad commit could be any of" "$GIT_DIR/BISECT_RUN" \
> /dev/null
then
- (
- gettext "bisect run cannot continue any more" &&
- echo
- ) >&2
+ gettextln "bisect run cannot continue any more" >&2
exit $res
fi
if [ $res -ne 0 ]
then
- (
- eval_gettext "bisect run failed:
-'bisect_state \$state' exited with error code \$res" &&
- echo
- ) >&2
+ eval_gettextln "bisect run failed:
+'bisect_state \$state' exited with error code \$res" >&2
exit $res
fi
if sane_grep "is the first bad commit" "$GIT_DIR/BISECT_RUN" > /dev/null
then
- gettext "bisect run success"; echo
+ gettextln "bisect run success"
exit 0;
fi
--
1.7.6.44.gdbb64d
^ permalink raw reply related
* [PATCH] grep: Fix race condition in delta_base_cache
From: Nicolas Morey-Chaisemartin @ 2011-08-30 13:45 UTC (permalink / raw)
To: Junio C Hamano, git
[-- Attachment #1: Type: text/plain, Size: 825 bytes --]
When running large git grep (ie: git grep regexp $(git rev-list --all)), glibc error sometimes occur:
*** glibc detected *** git: double free or corruption (!prev): 0x00000000010abdf0 ***
According to gdb the problem originate from release_delta_cash (sha1_file.c:1703)
free(ent->data);
>From my analysis it seems that git grep threads do acquire lock before calling read_sha1_file but not before calling
read_object_with_reference who ends up calling read_sha1_file too.
Adding the lock around read_object_with_reference seems to fix the issue for me.
I've ran git grep about a dozen time and seen no more error while
it usually happened half the time before.
Signed-off-by: Nicolas Morey-Chaisemartin <nicolas@morey-chaisemartin.com>
---
builtin/grep.c | 2 ++
1 files changed, 2 insertions(+), 0 deletions(-)
[-- Attachment #2: 0001-grep-Fix-race-condition-in-delta_base_cache.patch --]
[-- Type: text/x-patch, Size: 465 bytes --]
diff --git a/builtin/grep.c b/builtin/grep.c
index 1c359c2..56398d5 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -598,8 +598,10 @@ 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));
^ permalink raw reply related
* [PATCH v2] Make use of git status when autocompleting git add, rm, checkout --, and reset HEAD
From: Ron Panduwana @ 2011-08-30 21:43 UTC (permalink / raw)
To: git; +Cc: Shawn O. Pearce, Junio C Hamano, Lee Marlow, Thomas Rast,
Ron Panduwana
Signed-off-by: Ron Panduwana <panduwana@gmail.com>
---
On Fri, Aug 19, 2011 at 5:10 PM, Thomas Rast <trast@student.ethz.ch> wrote:
> Some thoughts:
>
> * running git-status for . has some issues: it doesn't work in the
> Â case of
>
> Â Â cd subdir
> Â Â git add ../some/file[TAB]
>
> Â It's also inefficient if you are at the top level and
>
> Â Â git add path/to/file/a/few/levels/down[TAB]
>
> Â since it wouldn't actually have to look for untracked files in the
> Â entire repo.
Fixed by running git-status for $cur if $cur is a directory. Otherwise run on .
> * -uall is not required unless you are looking for untracked files.
> Â For the other commands you could speed up completion by passing
> Â -uno instead.
Fixed by adding second parameter to __git_files_having_status
contrib/completion/git-completion.bash | 84 ++++++++++++++++++++-----------
1 files changed, 54 insertions(+), 30 deletions(-)
diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 8648a36..9d44501 100755
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -1010,6 +1010,16 @@ __git_has_doubledash ()
return 1
}
+# __git_files_having_status requires 2 arguments
+__git_files_having_status ()
+{
+ local dir="."
+ if [ -d "$cur" ]; then
+ dir="$cur"
+ fi
+ echo "$(git status $2 -s "$dir" 2>/dev/null | egrep "^$1" | cut -c4-)"
+}
+
__git_whitespacelist="nowarn warn error error-all fix"
_git_am ()
@@ -1058,17 +1068,17 @@ _git_apply ()
_git_add ()
{
- __git_has_doubledash && return
-
- case "$cur" in
- --*)
- __gitcomp "
- --interactive --refresh --patch --update --dry-run
- --ignore-errors --intent-to-add
- "
- return
- esac
- COMPREPLY=()
+ if ! __git_has_doubledash; then
+ case "$cur" in
+ --*)
+ __gitcomp "
+ --interactive --refresh --patch --update --dry-run
+ --ignore-errors --intent-to-add
+ "
+ return
+ esac
+ fi
+ __gitcomp "$(__git_files_having_status "(.[MAU]|UD|\?\?)" -uall)"
}
_git_archive ()
@@ -1171,7 +1181,12 @@ _git_bundle ()
_git_checkout ()
{
- __git_has_doubledash && return
+ if __git_has_doubledash; then
+ if [[ ${words[2]} = "--" ]]; then
+ __gitcomp "$(__git_files_having_status ".[MD]" -uno)"
+ fi
+ return
+ fi
case "$cur" in
--conflict=*)
@@ -1469,7 +1484,7 @@ _git_help ()
__gitcomp "$__git_all_commands $(__git_aliases)
attributes cli core-tutorial cvs-migration
diffcore gitk glossary hooks ignore modules
- namespaces repository-layout tutorial tutorial-2
+ repository-layout tutorial tutorial-2
workflows
"
}
@@ -2313,14 +2328,18 @@ _git_replace ()
_git_reset ()
{
- __git_has_doubledash && return
-
- case "$cur" in
- --*)
- __gitcomp "--merge --mixed --hard --soft --patch"
+ if ! __git_has_doubledash; then
+ case "$cur" in
+ --*)
+ __gitcomp "--merge --mixed --hard --soft --patch"
+ return
+ ;;
+ esac
+ fi
+ if [[ ${words[2]} = "HEAD" ]]; then
+ __gitcomp "$(__git_files_having_status "[ADM]." -uno)"
return
- ;;
- esac
+ fi
__gitcomp "$(__git_refs)"
}
@@ -2337,15 +2356,20 @@ _git_revert ()
_git_rm ()
{
- __git_has_doubledash && return
-
- case "$cur" in
- --*)
- __gitcomp "--cached --dry-run --ignore-unmatch --quiet"
- return
- ;;
- esac
- COMPREPLY=()
+ if ! __git_has_doubledash; then
+ case "$cur" in
+ --*)
+ __gitcomp "--cached --dry-run --ignore-unmatch --quiet"
+ return
+ ;;
+ esac
+ fi
+ # check if --cached was specified
+ if [ "$(__git_find_on_cmdline "--cached")" ]; then
+ COMPREPLY=()
+ else
+ __gitcomp "$(__git_files_having_status "(.D|DU|UA)" -uno)"
+ fi
}
_git_shortlog ()
@@ -2640,7 +2664,6 @@ _git ()
--exec-path
--html-path
--work-tree=
- --namespace=
--help
"
;;
@@ -2737,3 +2760,4 @@ else
shopt "$@"
}
fi
+
--
1.7.1
^ permalink raw reply related
* Re: [PATCH] t6030: use $SHELL_PATH to invoke user's preferred shell instead of bare sh
From: Brandon Casey @ 2011-08-30 20:55 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Brandon Casey, git
In-Reply-To: <7vaaaq1w6x.fsf@alter.siamese.dyndns.org>
On 08/30/2011 03:27 PM, Junio C Hamano wrote:
> Junio C Hamano <gitster@pobox.com> writes:
>
>>> + git bisect run \"$SHELL_PATH\" -c '
>>> GOOD=\$(git for-each-ref \"--format=%(objectname)\" refs/bisect/good-*) &&
>>> git rev-list --objects BISECT_HEAD --not \$GOOD >tmp.\$\$ &&
>>> git pack-objects --stdout >/dev/null < tmp.\$\$
>>
>> Hmm, shouldn't we also be quoting '$' in front of SHELL_PATH to make the
>> test_expect_success interpolate the value of SHELL_PATH, instead of hoping
>> that SHELL_PATH does not have double-quote in it when the shell that forms
>> parameters given to test_expect_success interpolates it?
Yep, makes sense. Good catch. Thanks.
-Brandon
> By doing this silly thing:
>
> $ mkdir '/var/tmp/a"b c/'
> $ ln -s /bin/sh '/var/tmp/a"b c/shell'
>
> and then adding
>
> SHELL_PATH='/var/tmp/a"b c/shell'
>
> after we source "test-lib.sh" in t6030, I see this breaks when the shell
> parses to figure out what parameters to call test_expect_success with.
>
> t/t6030-bisect-porcelain.sh | 2 +-
> 1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/t/t6030-bisect-porcelain.sh b/t/t6030-bisect-porcelain.sh
> index 68c0e97..cd9de63 100755
> --- a/t/t6030-bisect-porcelain.sh
> +++ b/t/t6030-bisect-porcelain.sh
> @@ -701,7 +701,7 @@ test_expect_success 'bisect: demonstrate identification of damage boundary' "
> git bisect reset &&
> git checkout broken &&
> git bisect start broken master --no-checkout &&
> - git bisect run \"$SHELL_PATH\" -c '
> + git bisect run \"\$SHELL_PATH\" -c '
> GOOD=\$(git for-each-ref \"--format=%(objectname)\" refs/bisect/good-*) &&
> git rev-list --objects BISECT_HEAD --not \$GOOD >tmp.\$\$ &&
> git pack-objects --stdout >/dev/null < tmp.\$\$
^ permalink raw reply
* Re: [PATCH 3/5] tree-walk: micro-optimization in tree_entry_interesting
From: Junio C Hamano @ 2011-08-30 20:40 UTC (permalink / raw)
To: Dan McGee; +Cc: git
In-Reply-To: <CAEik5nNaDkAa2+63g1z3c1JUB8sLuTLfYP3jLKZJg2=yKqyzDg@mail.gmail.com>
Dan McGee <dpmcgee@gmail.com> writes:
>> dmcgee@galway ~/projects/linux-2.6 (master)
>> $ time ../git/git-log -- zzzzz_not_exist > /dev/null
>>
>> real 0m0.945s
>> user 0m0.857s
>> sys 0m0.083s
>>
>>> There is nothing wrong in the patch per-se, but I really wish we didn't
>>> have to do this; it feels like the compiler should be helping us in this
>>> case.
>
> If I resurrect this with an updated commit message reflecting concerns
> raised, can it be merged? Given that it is a noticeable performance
> boost on real-life repositories and I can show it has little (<1%) to
> no impact on most repos, it is a definite win.
I do not see anything wrong in this particular patch per-se, but I really
wish we didn't have to do this.
Please include a few lines of benchmarking result in the updated commit
log message as well if you are rerolling this patch, perhaps like I did
in:
http://thread.gmane.org/gmane.comp.version-control.git/179926/focus=180361
i.e., before and after comparison.
Thanks.
^ permalink raw reply
* Re: [PATCH] t6030: use $SHELL_PATH to invoke user's preferred shell instead of bare sh
From: Junio C Hamano @ 2011-08-30 20:27 UTC (permalink / raw)
To: Brandon Casey, Brandon Casey; +Cc: git
In-Reply-To: <7vfwki1wqo.fsf@alter.siamese.dyndns.org>
Junio C Hamano <gitster@pobox.com> writes:
>> + git bisect run \"$SHELL_PATH\" -c '
>> GOOD=\$(git for-each-ref \"--format=%(objectname)\" refs/bisect/good-*) &&
>> git rev-list --objects BISECT_HEAD --not \$GOOD >tmp.\$\$ &&
>> git pack-objects --stdout >/dev/null < tmp.\$\$
>
> Hmm, shouldn't we also be quoting '$' in front of SHELL_PATH to make the
> test_expect_success interpolate the value of SHELL_PATH, instead of hoping
> that SHELL_PATH does not have double-quote in it when the shell that forms
> parameters given to test_expect_success interpolates it?
By doing this silly thing:
$ mkdir '/var/tmp/a"b c/'
$ ln -s /bin/sh '/var/tmp/a"b c/shell'
and then adding
SHELL_PATH='/var/tmp/a"b c/shell'
after we source "test-lib.sh" in t6030, I see this breaks when the shell
parses to figure out what parameters to call test_expect_success with.
t/t6030-bisect-porcelain.sh | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/t/t6030-bisect-porcelain.sh b/t/t6030-bisect-porcelain.sh
index 68c0e97..cd9de63 100755
--- a/t/t6030-bisect-porcelain.sh
+++ b/t/t6030-bisect-porcelain.sh
@@ -701,7 +701,7 @@ test_expect_success 'bisect: demonstrate identification of damage boundary' "
git bisect reset &&
git checkout broken &&
git bisect start broken master --no-checkout &&
- git bisect run \"$SHELL_PATH\" -c '
+ git bisect run \"\$SHELL_PATH\" -c '
GOOD=\$(git for-each-ref \"--format=%(objectname)\" refs/bisect/good-*) &&
git rev-list --objects BISECT_HEAD --not \$GOOD >tmp.\$\$ &&
git pack-objects --stdout >/dev/null < tmp.\$\$
^ 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