* Re: git branch -M" regression in 1.7.7?
From: Conrad Irwin @ 2011-11-26 7:05 UTC (permalink / raw)
To: Jonathan Nieder
Cc: ☂Josh Chia (谢任中), git,
Soeren Sonnenburg
In-Reply-To: <20111126023002.GA17652@elie.hsd1.il.comcast.net>
On Fri, Nov 25, 2011 at 6:30 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:
> A reproduction recipe (preferrably in the form of a patch to
> t/t3200-branch.sh would be welcome.
Sent in a separate email. Feel free to add a "Tested-by:" header to
your patch if you want :).
>
> -- >8 --
> Subject: treat "git branch -M master master" as a no-op again
>
> Before v1.7.7-rc2~1^2~2 (Prevent force-updating of the current branch,
> 2011-08-20), commands like "git branch -M topic master" could be used
> even when "master" was the current branch, with the somewhat
> counterintuitive result that HEAD would point to some place new while
> the index and worktree kept the content of the old commit. This is
> not a very sensible operation and the result is what almost nobody
> would expect, so erroring out in this case was a good change.
>
> However, there is one exception to the "it's usually not obvious what
> it would mean to overwrite the current branch by another one" rule.
> Namely:
>
> git branch -M master master
>
> is clearly meant to be a no-op, even if you are on the master branch.
Agreed. I thought after reading your patch about making it just do:
if (!strcmp(oldname, newname))
exit(0);
but I guess it would then not mark an entry in the reflog that people
could be relying on...
> + clobber_head_ok = !strcmp(oldname, newname);
> +
> + validate_new_branchname(newname, &newref, force, clobber_head_ok);
This looks ok, and will be improvable if the NEEDSWORK in branch.h is done.
The other thing I wonder is whether "git checkout -B master HEAD" or
"git branch -f master master" should have the same short-cut?
Conrad
^ permalink raw reply
* Re: [PATCH] Test renaming a branch to itself
From: Jonathan Nieder @ 2011-11-26 6:59 UTC (permalink / raw)
To: Conrad Irwin
Cc: git, Soeren Sonnenburg,
☂Josh Chia (谢任中)
In-Reply-To: <1322290364-16207-1-git-send-email-conrad.irwin@gmail.com>
Conrad Irwin wrote:
> Test for a regression introduced in v1.7.7-rc2~1^2~2.
Thanks! I take it that means you like the patch. :)
The tests look sane fwiw (and it looks like the tests you wrote before
cover the "branch -M" safety valve pretty well).
^ permalink raw reply
* [PATCH] Test renaming a branch to itself
From: Conrad Irwin @ 2011-11-26 6:52 UTC (permalink / raw)
To: Jonathan Nieder
Cc: git, Soeren Sonnenburg,
☂Josh Chia (谢任中), Conrad Irwin
In-Reply-To: <20111126023002.GA17652@elie.hsd1.il.comcast.net>
Test for a regression introduced in v1.7.7-rc2~1^2~2.
Requested-by: Jonathan Nieder <jrnieder@gmail.com>
Signed-off-by: Conrad Irwin <conrad.irwin@gmail.com>
---
t/t3200-branch.sh | 16 ++++++++++++++++
1 files changed, 16 insertions(+), 0 deletions(-)
diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
index bc73c20..7690332 100755
--- a/t/t3200-branch.sh
+++ b/t/t3200-branch.sh
@@ -115,6 +115,22 @@ test_expect_success 'git branch -M baz bam should succeed when baz is checked ou
git branch -M baz bam
'
+test_expect_success 'git branch -M master should work when master is checked out' '
+ git checkout master &&
+ git branch -M master
+'
+
+test_expect_success 'git branch -M master master should work when master is checked out' '
+ git checkout master &&
+ git branch -M master master
+'
+
+test_expect_success 'git branch -M master2 master2 should work when master is checked out' '
+ git checkout master &&
+ git branch master2 &&
+ git branch -M master2 master2
+'
+
test_expect_success 'git branch -v -d t should work' '
git branch t &&
test_path_is_file .git/refs/heads/t &&
--
1.7.7.1.433.ga2d04a
^ permalink raw reply related
* Re: git branch -M" regression in 1.7.7?
From: Jonathan Nieder @ 2011-11-26 2:30 UTC (permalink / raw)
To: ☂Josh Chia (谢任中)
Cc: git, Soeren Sonnenburg, Conrad Irwin
In-Reply-To: <CALxtSbRbwkVDKJcXiKY9rHYCjA3XGgCytbXQnRhQvbEnY8SpjA@mail.gmail.com>
Hi,
Josh Chia (谢任中) wrote:
> On git 1.7.7.3, when I try to "git branch -M master" when I'm already
> on a branch 'master', I get this error message:
> Cannot force update the current branch
>
> On 1.7.6.4, the command succeeds.
>
> Is this intended?
Yes, but it probably wasn't a good idea. How about this patch?
A reproduction recipe (preferrably in the form of a patch to
t/t3200-branch.sh would be welcome.
-- >8 --
Subject: treat "git branch -M master master" as a no-op again
Before v1.7.7-rc2~1^2~2 (Prevent force-updating of the current branch,
2011-08-20), commands like "git branch -M topic master" could be used
even when "master" was the current branch, with the somewhat
counterintuitive result that HEAD would point to some place new while
the index and worktree kept the content of the old commit. This is
not a very sensible operation and the result is what almost nobody
would expect, so erroring out in this case was a good change.
However, there is one exception to the "it's usually not obvious what
it would mean to overwrite the current branch by another one" rule.
Namely:
git branch -M master master
is clearly meant to be a no-op, even if you are on the master branch.
And in the latter case, it can be abbreviated:
git branch -M master
This seems like a valuable exception to allow, because then "git
branch -M foo" would _always_ be allowed --- either 'foo' is not the
current branch, and it does the obvious thing, or 'foo' is the current
branch, and nothing happens.
Buildbot uses this idiom and was broken in 1.7.7 (it would emit the
message "Cannot force update the current branch").
Reported-by: Soeren Sonnenburg <sonne@debian.org>
Reported-by: Josh Chia (谢任中)
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
builtin/branch.c | 9 ++++++++-
1 files changed, 8 insertions(+), 1 deletions(-)
diff --git a/builtin/branch.c b/builtin/branch.c
index 51ca6a02..24f33b24 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -568,6 +568,7 @@ static void rename_branch(const char *oldname, const char *newname, int force)
unsigned char sha1[20];
struct strbuf oldsection = STRBUF_INIT, newsection = STRBUF_INIT;
int recovery = 0;
+ int clobber_head_ok;
if (!oldname)
die(_("cannot rename the current branch while not on any."));
@@ -583,7 +584,13 @@ static void rename_branch(const char *oldname, const char *newname, int force)
die(_("Invalid branch name: '%s'"), oldname);
}
- validate_new_branchname(newname, &newref, force, 0);
+ /*
+ * A command like "git branch -M currentbranch currentbranch" cannot
+ * cause the worktree to become inconsistent with HEAD, so allow it.
+ */
+ clobber_head_ok = !strcmp(oldname, newname);
+
+ validate_new_branchname(newname, &newref, force, clobber_head_ok);
strbuf_addf(&logmsg, "Branch: renamed %s to %s",
oldref.buf, newref.buf);
--
1.7.8.rc3
^ permalink raw reply related
* git branch -M" regression in 1.7.7?
From: ☂Josh Chia (谢任中) @ 2011-11-26 0:36 UTC (permalink / raw)
To: git
On git 1.7.7.3, when I try to "git branch -M master" when I'm already
on a branch 'master', I get this error message:
Cannot force update the current branch
On 1.7.6.4, the command succeeds.
Is this intended?
^ permalink raw reply
* Re: Infinite loop in cascade_filter_fn()
From: Carlos Martín Nieto @ 2011-11-25 17:02 UTC (permalink / raw)
To: Henrik Grubbström; +Cc: Git Mailing list, Junio C Hamano
In-Reply-To: <Pine.GSO.4.63.1111251705330.22588@shipon.roxen.com>
[-- Attachment #1: Type: text/plain, Size: 2949 bytes --]
On Fri, Nov 25, 2011 at 05:14:17PM +0100, Henrik Grubbström wrote:
> On Fri, 25 Nov 2011, Carlos Martín Nieto wrote:
>
> >This patch fixes this, but I think it would still break if the LF is
> >at the end of the file. Changing the `if (!input)` to put the LF in
> >the output buffer may or may not be the right soulution. I feel like
> >this should be handled by cascade_filter_fn rather than the actual
> >filter somehow, but Junio's comment (4ae66704 'stream filter: add "no
> >more input" to the filters') suggests otherwise.
> >
> >I'm working on a cleaner patch that takes care of a bit of state, but
> >this is the general idea.
>
> Looks good to me (and seems to work in my case).
That patch would give wrong output if the same happened at the end of
a file. The attached patch should also cover this case.
> Typo in the commit subject though.
>
> > cmn
> >--- 8< ---
> >Subject: [PATCH] convert: don't loop indefintely if at LF-to-CRLF streaming
> ^^^^^^^^^^^
> This should be either "infinitely", or "indefinitely", but since we
> know that the loop won't terminate "infinitely" is to be preferred.
Thanks for noticing. I went with a different title in the end. Junio,
could you consider this one for inclusion in the next RC?
--- 8< ---
Subject: [PATCH] convert: track state in LF-to-CRLF filter
There may not be enough space to store CRLF in the output. If we don't
fill the buffer, then the filter will keep getting called with the same
short buffer and will loop forever.
Instead, always store the CR and record there's a missing LF if
necessary it so we store it in the output buffer the next time the
function gets called.
Reported-by: Henrik Grubbström <grubba@roxen.com>
Signed-off-by: Carlos Martín Nieto <cmn@elego.de>
---
convert.c | 23 ++++++++++++++++-------
1 files changed, 16 insertions(+), 7 deletions(-)
diff --git a/convert.c b/convert.c
index 86e9c29..c050b86 100644
--- a/convert.c
+++ b/convert.c
@@ -880,20 +880,29 @@ static int lf_to_crlf_filter_fn(struct stream_filter *filter,
const char *input, size_t *isize_p,
char *output, size_t *osize_p)
{
- size_t count;
+ size_t count, o = 0;
+ static int want_lf = 0;
+
+ /* Output a pending LF if we need to */
+ if (want_lf) {
+ output[o++] = '\n';
+ want_lf = 0;
+ }
if (!input)
- return 0; /* we do not keep any states */
+ return 0; /* We've already dealt with the state */
+
count = *isize_p;
if (count) {
- size_t i, o;
- for (i = o = 0; o < *osize_p && i < count; i++) {
+ size_t i;
+ for (i = 0; o < *osize_p && i < count; i++) {
char ch = input[i];
if (ch == '\n') {
- if (o + 1 < *osize_p)
- output[o++] = '\r';
- else
+ output[o++] = '\r';
+ if (o >= *osize_p) {
+ want_lf = 1;
break;
+ }
}
output[o++] = ch;
}
--
1.7.8.rc3.31.g017d1
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 490 bytes --]
^ permalink raw reply related
* Re: [PATCH] grep: load funcname patterns for -W
From: René Scharfe @ 2011-11-25 16:32 UTC (permalink / raw)
To: Thomas Rast; +Cc: git, Junio C Hamano
In-Reply-To: <5e3bcf651b31b299ca411296e6e7c4d11f6ae617.1322232319.git.trast@student.ethz.ch>
Am 25.11.2011 15:46, schrieb Thomas Rast:
> git-grep avoids loading the funcname patterns unless they are needed.
> ba8ea74 (grep: add option to show whole function as context,
> 2011-08-01) forgot to extend this test also to the new funcbody
> feature. Do so.
>
> The catch is that we also have to disable threading when using
> userdiff, as explained in grep_threads_ok(). So we must be careful to
> introduce the same test there.
Oops. Thanks for catching this.
That reminds me to look into adding a thread-safe way to access
attributes again..
Thanks,
René
^ permalink raw reply
* Re: what are the chances of a 'pre-upload' hook?
From: Sitaram Chamarty @ 2011-11-25 16:18 UTC (permalink / raw)
To: Andreas Ericsson; +Cc: Git Mailing List
In-Reply-To: <4ECF939D.8020706@op5.se>
On Fri, Nov 25, 2011 at 6:39 PM, Andreas Ericsson <ae@op5.se> wrote:
> People who fetch but don't push is, by far, the vast majority of git users.
> Think of everyone fetching from any public software repository without
> having write access to it. If you think of git.git and linux.git alone
> I think it's safe to assume the number of "fetch-no-push" outnumber the
> "push-and-whatnot" group by some quarter million to one.
But in those environments the person pulling does not even have an ID
so how is he at risk with the hook running?
>> I may be wrong but I imagine shared environments are those where
>> almost everyone will push at least once in a while. It's a closed
>> group of people, probably all developers, etc etc etc...
>>
>
> Not really. We fetch from each other quite a lot at work, and from
> each others semi-public repositories on a shared server where we've
> all got accounts (ie, write access), but we very, very rarely push
> into each others repositories. The sharepoint is the "official" repo
> on the repo-server, which the buildbots gets its code from and where
> everything to be released, maintained or handled in some way in the
> future resides.
Yes, and this is the only situation where it does have the issue. I'm
just not sure how common this is.
It's fine if you tell me I'm wrong and that this *is* still very
common. I'll back off.
But everyone seems to be bringing in github and public repos as part
of the argument, and I don't see how they're relevant to the original
security issue of the guy who pulls having his account compromised.
> Anyways. Shooting down the arguments *against* pre-upload hooks are
> quite silly if it's not combined with some fresh arguments *for* such
> a hook.
>
> So... What usecase do you envision where you'd need one?
I'm writing a caching proxy that helps with bandwidth issues when too
many people in a bad-WAN site want to clone some huge repo from its
canonical site. The only one I found by googling fiddles with the git
protocol itself, and I hate doing things like that.
Ignoring all the details, the pre-upload hook would have checked some
conditions and fired off a fetch from the remote site if those checks
passed.
It's easy enough to do it from cron but it would have been more
elegant this way.
^ permalink raw reply
* Re: Infinite loop in cascade_filter_fn()
From: Henrik Grubbström @ 2011-11-25 16:14 UTC (permalink / raw)
To: Carlos Martín Nieto; +Cc: Git Mailing list, Junio C Hamano
In-Reply-To: <20111125153829.GB10417@beez.lab.cmartin.tk>
[-- Attachment #1: Type: TEXT/PLAIN, Size: 1007 bytes --]
On Fri, 25 Nov 2011, Carlos Martín Nieto wrote:
> This patch fixes this, but I think it would still break if the LF is
> at the end of the file. Changing the `if (!input)` to put the LF in
> the output buffer may or may not be the right soulution. I feel like
> this should be handled by cascade_filter_fn rather than the actual
> filter somehow, but Junio's comment (4ae66704 'stream filter: add "no
> more input" to the filters') suggests otherwise.
>
> I'm working on a cleaner patch that takes care of a bit of state, but
> this is the general idea.
Looks good to me (and seems to work in my case).
Typo in the commit subject though.
> cmn
> --- 8< ---
> Subject: [PATCH] convert: don't loop indefintely if at LF-to-CRLF streaming
^^^^^^^^^^^
This should be either "infinitely", or "indefinitely", but since we know
that the loop won't terminate "infinitely" is to be preferred.
Thanks,
--
Henrik Grubbström grubba@roxen.com
Roxen Internet Software AB
^ permalink raw reply
* Re: Infinite loop in cascade_filter_fn()
From: Henrik Grubbström @ 2011-11-25 15:59 UTC (permalink / raw)
To: Carlos Martín Nieto; +Cc: Git Mailing list, Junio C Hamano
In-Reply-To: <20111125155301.GC10417@beez.lab.cmartin.tk>
[-- Attachment #1: Type: TEXT/PLAIN, Size: 692 bytes --]
On Fri, 25 Nov 2011, Carlos Martín Nieto wrote:
> On Fri, Nov 25, 2011 at 04:43:41PM +0100, Henrik Grubbström wrote:
>>
>> The bug is probably that lf_to_crlf_filter_fn() should return
>> non-zero in this case (ie o and/or i being zero).
>
> non-zero? That would cause the filter to abort, which definitely not
> what we want. Have you seen my other e-mails regarding this? I'm
> trying to figure out which is the best way to go about this. The
> solution is to keep track of the fact that we're missing a LF in the
> output buffer.
True, I misread the code.
Keeping track of the filter state is the way to go.
> cmn
--
Henrik Grubbström grubba@roxen.com
Roxen Internet Software AB
^ permalink raw reply
* Re: Infinite loop in cascade_filter_fn()
From: Carlos Martín Nieto @ 2011-11-25 15:53 UTC (permalink / raw)
To: Henrik Grubbström; +Cc: Git Mailing list, Junio C Hamano
In-Reply-To: <Pine.GSO.4.63.1111251629500.22588@shipon.roxen.com>
[-- Attachment #1: Type: text/plain, Size: 1277 bytes --]
On Fri, Nov 25, 2011 at 04:43:41PM +0100, Henrik Grubbström wrote:
> On Wed, 23 Nov 2011, Henrik Grubbström wrote:
>
> >Hi.
> >
> >My git repository walker just got bitten by what seems to be a
> >reasonably new bug in convert.c:cascade_filter_fn() (git 1.7.8.rc3
> >(gentoo)).
>
> After some tracing, the problem is triggered by the variable "remaining"
> being set to 1 in the beginning of the cascade_filter_fn() loop,
> which causes filter "two" to be called with an output buffer size of
> 1.
> Filter "two" in this case is lf_to_crlf_filter_fn(), and the next
> input character is a "\n". lf_to_crlf_filter_fn() wants to convert
> this to "\r\n", but that doesn't fit into the buffer, so it breaks
> out and returns zero. Upon seing the zero cascade_filter_fn() thinks
> all is well, even though nothing has happened, and loops.
>
> The bug is probably that lf_to_crlf_filter_fn() should return
> non-zero in this case (ie o and/or i being zero).
non-zero? That would cause the filter to abort, which definitely not
what we want. Have you seen my other e-mails regarding this? I'm
trying to figure out which is the best way to go about this. The
solution is to keep track of the fact that we're missing a LF in the
output buffer.
cmn
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 490 bytes --]
^ permalink raw reply
* Re: Infinite loop in cascade_filter_fn()
From: Henrik Grubbström @ 2011-11-25 15:43 UTC (permalink / raw)
To: Git Mailing list; +Cc: Junio C Hamano
In-Reply-To: <Pine.GSO.4.63.1111231801580.5099@shipon.roxen.com>
[-- Attachment #1: Type: TEXT/PLAIN, Size: 938 bytes --]
On Wed, 23 Nov 2011, Henrik Grubbström wrote:
> Hi.
>
> My git repository walker just got bitten by what seems to be a reasonably new
> bug in convert.c:cascade_filter_fn() (git 1.7.8.rc3 (gentoo)).
After some tracing, the problem is triggered by the variable "remaining"
being set to 1 in the beginning of the cascade_filter_fn() loop, which
causes filter "two" to be called with an output buffer size of 1.
Filter "two" in this case is lf_to_crlf_filter_fn(), and the next input
character is a "\n". lf_to_crlf_filter_fn() wants to convert this to
"\r\n", but that doesn't fit into the buffer, so it breaks out and returns
zero. Upon seing the zero cascade_filter_fn() thinks all is well, even
though nothing has happened, and loops.
The bug is probably that lf_to_crlf_filter_fn() should return non-zero in
this case (ie o and/or i being zero).
> Thanks,
--
Henrik Grubbström grubba@roxen.com
Roxen Internet Software AB
^ permalink raw reply
* Re: Infinite loop in cascade_filter_fn()
From: Carlos Martín Nieto @ 2011-11-25 15:38 UTC (permalink / raw)
To: Henrik Grubbström; +Cc: Git Mailing list, Junio C Hamano
In-Reply-To: <Pine.GSO.4.63.1111231801580.5099@shipon.roxen.com>
[-- Attachment #1: Type: text/plain, Size: 2907 bytes --]
On Wed, Nov 23, 2011 at 06:40:47PM +0100, Henrik Grubbström wrote:
> Hi.
>
> My git repository walker just got bitten by what seems to be a
> reasonably new bug in convert.c:cascade_filter_fn() (git 1.7.8.rc3
> (gentoo)).
>
> How to reproduce:
>
> git clone git@github.com:pikelang/Pike.git
>
> git checkout -f 0e2080f838c6f0bc7d670ac7549676a353451dca^
>
> git checkout -f 0e2080f838c6f0bc7d670ac7549676a353451dca
>
> The first two commands complete as expected, while the last hangs forever.
> Performing the same with git 1.7.6.4 works as expected.
>
> The problematic file seems to be
> /src/modules/_Crypto/rijndael_ecb_vt.txt which has the attributes:
> text ident eol=crlf
It looks like you won the lottery. The problem was that the output
buffer only has one byte available when we see a LF. We check whether
there is enough space (two bytes) to store CRLF in the output buffer,
see that there isn't and return. cascade_filter_fn sees that the
buffer hasn't been written fully and calls lf_to_crlf_filter_fn with
the same output buffer, which we still can't fill, because it's too
short.
This patch fixes this, but I think it would still break if the LF is
at the end of the file. Changing the `if (!input)` to put the LF in
the output buffer may or may not be the right soulution. I feel like
this should be handled by cascade_filter_fn rather than the actual
filter somehow, but Junio's comment (4ae66704 'stream filter: add "no
more input" to the filters') suggests otherwise.
I'm working on a cleaner patch that takes care of a bit of state, but
this is the general idea.
cmn
--- 8< ---
Subject: [PATCH] convert: don't loop indefintely if at LF-to-CRLF streaming
If we find a LF when the output buffer is only has one byte remaining,
cascade_filter_fn won't notice that we need more input and won't drain
the output buffer.
In such a case, store whether we've outputted the CR so we can retake
it from there.
Signed-off-by: Carlos Martín Nieto <cmn@elego.de>
---
convert.c | 11 ++++++++---
1 files changed, 8 insertions(+), 3 deletions(-)
diff --git a/convert.c b/convert.c
index 86e9c29..4218f40 100644
--- a/convert.c
+++ b/convert.c
@@ -881,6 +881,7 @@ static int lf_to_crlf_filter_fn(struct stream_filter *filter,
char *output, size_t *osize_p)
{
size_t count;
+ static int put_cr = 0;
if (!input)
return 0; /* we do not keep any states */
@@ -890,10 +891,14 @@ static int lf_to_crlf_filter_fn(struct stream_filter *filter,
for (i = o = 0; o < *osize_p && i < count; i++) {
char ch = input[i];
if (ch == '\n') {
- if (o + 1 < *osize_p)
+ if (put_cr) {
+ put_cr = 0;
+ } else {
output[o++] = '\r';
- else
- break;
+ put_cr = 1;
+ i--;
+ continue;
+ }
}
output[o++] = ch;
}
--
1.7.8.rc3.31.g017d1
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 490 bytes --]
^ permalink raw reply related
* [PATCH] grep: load funcname patterns for -W
From: Thomas Rast @ 2011-11-25 14:46 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, René Scharfe
git-grep avoids loading the funcname patterns unless they are needed.
ba8ea74 (grep: add option to show whole function as context,
2011-08-01) forgot to extend this test also to the new funcbody
feature. Do so.
The catch is that we also have to disable threading when using
userdiff, as explained in grep_threads_ok(). So we must be careful to
introduce the same test there.
---
grep.c | 7 ++++---
t/t7810-grep.sh | 14 ++++++++++++++
2 files changed, 18 insertions(+), 3 deletions(-)
diff --git a/grep.c b/grep.c
index b29d09c..7a070e9 100644
--- a/grep.c
+++ b/grep.c
@@ -948,8 +948,8 @@ int grep_threads_ok(const struct grep_opt *opt)
* machinery in grep_buffer_1. The attribute code is not
* thread safe, so we disable the use of threads.
*/
- if (opt->funcname && !opt->unmatch_name_only && !opt->status_only &&
- !opt->name_only)
+ if ((opt->funcname || opt->funcbody)
+ && !opt->unmatch_name_only && !opt->status_only && !opt->name_only)
return 0;
return 1;
@@ -1008,7 +1008,8 @@ static int grep_buffer_1(struct grep_opt *opt, const char *name,
}
memset(&xecfg, 0, sizeof(xecfg));
- if (opt->funcname && !opt->unmatch_name_only && !opt->status_only &&
+ if ((opt->funcname || opt->funcbody)
+ && !opt->unmatch_name_only && !opt->status_only &&
!opt->name_only && !binary_match_only && !collect_hits) {
struct userdiff_driver *drv = userdiff_find_by_path(name);
if (drv && drv->funcname.pattern) {
diff --git a/t/t7810-grep.sh b/t/t7810-grep.sh
index 81263b7..7ba5b16 100755
--- a/t/t7810-grep.sh
+++ b/t/t7810-grep.sh
@@ -523,6 +523,20 @@ test_expect_success 'grep -W' '
test_cmp expected actual
'
+cat >expected <<EOF
+hello.c= printf("Hello world.\n");
+hello.c: return 0;
+hello.c- /* char ?? */
+EOF
+
+test_expect_success 'grep -W with userdiff' '
+ test_when_finished "rm -f .gitattributes" &&
+ git config diff.custom.xfuncname "(printf.*|})$" &&
+ echo "hello.c diff=custom" >.gitattributes &&
+ git grep -W return >actual &&
+ test_cmp expected actual
+'
+
test_expect_success 'grep from a subdirectory to search wider area (1)' '
mkdir -p s &&
(
--
1.7.8.rc3.415.gbcbb2
^ permalink raw reply related
* Re: what are the chances of a 'pre-upload' hook?
From: Jeff King @ 2011-11-25 14:40 UTC (permalink / raw)
To: Sitaram Chamarty; +Cc: Git Mailing List
In-Reply-To: <CAMK1S_gh_CsWc-DnbOuUwn+H1i3skm99xzDbWe-wxsKKS0Qw-w@mail.gmail.com>
On Fri, Nov 25, 2011 at 09:43:14AM +0530, Sitaram Chamarty wrote:
> The quick summary, in the words of Jeff (second post in that link) is:
> "Because [upload]-pack runs as the user who is [fetching], not as the
> repository owner. So by convincing you to [fetch from] my repository
> in a multi-user environment, I convince you to run some arbitrary code
> of mine."
>
> My contention (today) is:
>
> - you're already taking that risk for push
> - so this would add a new risk only for people who fetch but don't push
> - which, I submit, is a very small (if not almost empty) set of people
>
> I may be wrong but I imagine shared environments are those where
> almost everyone will push at least once in a while. It's a closed
> group of people, probably all developers, etc etc etc...
One thing to note is that this is _only_ an issue on systems where the
user running upload-pack (or receive-pack, for push) is not the same as
the user who owns the hooks directory. So basically two situations:
1. Alice and Bob are developers on a multi-user system with ssh
access. Alice will run "git fetch ~bob/project.git" or "git fetch
alice@server:~bob/project.git". She executes Bob's hooks as
herself on the server.
2. Somebody runs a git:// server, locked down under a "git" user (or a
smart-http server, under a "www" account). If users of the service
can write their own hooks, they will run as the "git" user.
But there are many situations that don't fall under this umbrella. In
(1), maybe Alice and Bob decide that they trust each other enough, and
the hook is more important than the security issue. In (2), users of the
service might not be able to write their own hooks (this is the case for
GitHub, and I assume also for Gerrit).
There should be a way for people who aren't in one of those dangerous
situations to be able to use a hook. The important thing is to set the
defaults in such a way that the people who _are_ in that situation
aren't left in a vulnerable situation.
The easiest way would be something like a "trust remote hooks"
environment variable, off by default. Admins in situation (2) could set
it for their git-daemon (or webserver, or whatever, or
gitolite-over-ssh), once they decided it was safe.
It could also help with (1), but I think you would need to take special
steps to propagate the variable in the "git fetch server:project.git"
case.
-Peff
^ permalink raw reply
* Re: Infinite loop in cascade_filter_fn()
From: Carlos Martín Nieto @ 2011-11-25 14:31 UTC (permalink / raw)
To: Henrik Grubbström; +Cc: Git Mailing list, Junio C Hamano
In-Reply-To: <Pine.GSO.4.63.1111231801580.5099@shipon.roxen.com>
[-- Attachment #1: Type: text/plain, Size: 1326 bytes --]
On Wed, Nov 23, 2011 at 06:40:47PM +0100, Henrik Grubbström wrote:
> Hi.
>
> My git repository walker just got bitten by what seems to be a
> reasonably new bug in convert.c:cascade_filter_fn() (git 1.7.8.rc3
> (gentoo)).
It looks like it's a bug between cascade_filter_fn and the actual
filter function lf_to_crlf_filter_fn that gets triggered when the
output buffer is too small. In this particular case, *isize_p=378 and
*osize_p=1 which causes cascade_filter_fn to feed the filter data
which it can't process because it doesn't have anywhere to put it.
I think that the function assumes that the output buffer is always
large enough, but there are many indirections, so it might be an
off-by-one.
>
> How to reproduce:
>
> git clone git@github.com:pikelang/Pike.git
>
> git checkout -f 0e2080f838c6f0bc7d670ac7549676a353451dca^
>
> git checkout -f 0e2080f838c6f0bc7d670ac7549676a353451dca
>
> The first two commands complete as expected, while the last hangs forever.
> Performing the same with git 1.7.6.4 works as expected.
>
> The problematic file seems to be
> /src/modules/_Crypto/rijndael_ecb_vt.txt which has the attributes:
> text ident eol=crlf
>
> Thanks,
>
> --
> Henrik Grubbström grubba@grubba.org
> Roxen Internet Software AB grubba@roxen.com
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 490 bytes --]
^ permalink raw reply
* Re: what are the chances of a 'pre-upload' hook?
From: Andreas Ericsson @ 2011-11-25 13:09 UTC (permalink / raw)
To: Sitaram Chamarty; +Cc: Git Mailing List
In-Reply-To: <CAMK1S_gh_CsWc-DnbOuUwn+H1i3skm99xzDbWe-wxsKKS0Qw-w@mail.gmail.com>
On 11/25/2011 05:13 AM, Sitaram Chamarty wrote:
> On Fri, Nov 25, 2011 at 8:46 AM, Sitaram Chamarty<sitaramc@gmail.com> wrote:
>> (...and/or a post-upload hook)
>>
>> Has this ever come up?
>
> Sorry for the google-fu fail and for replying to my own post.
> http://git.661346.n2.nabble.com/Removal-of-post-upload-hook-td4394312.html
> is the longest thread I (later) found.
>
> The quick summary, in the words of Jeff (second post in that link) is:
> "Because [upload]-pack runs as the user who is [fetching], not as the
> repository owner. So by convincing you to [fetch from] my repository
> in a multi-user environment, I convince you to run some arbitrary code
> of mine."
>
> My contention (today) is:
>
> - you're already taking that risk for push
> - so this would add a new risk only for people who fetch but don't push
> - which, I submit, is a very small (if not almost empty) set of people
>
People who fetch but don't push is, by far, the vast majority of git users.
Think of everyone fetching from any public software repository without
having write access to it. If you think of git.git and linux.git alone
I think it's safe to assume the number of "fetch-no-push" outnumber the
"push-and-whatnot" group by some quarter million to one.
> I may be wrong but I imagine shared environments are those where
> almost everyone will push at least once in a while. It's a closed
> group of people, probably all developers, etc etc etc...
>
Not really. We fetch from each other quite a lot at work, and from
each others semi-public repositories on a shared server where we've
all got accounts (ie, write access), but we very, very rarely push
into each others repositories. The sharepoint is the "official" repo
on the repo-server, which the buildbots gets its code from and where
everything to be released, maintained or handled in some way in the
future resides.
Anyways. Shooting down the arguments *against* pre-upload hooks are
quite silly if it's not combined with some fresh arguments *for* such
a hook.
So... What usecase do you envision where you'd need one?
--
Andreas Ericsson andreas.ericsson@op5.se
OP5 AB www.op5.se
Tel: +46 8-230225 Fax: +46 8-230231
Considering the successes of the wars on alcohol, poverty, drugs and
terror, I think we should give some serious thought to declaring war
on peace.
^ permalink raw reply
* Re: [PATCH] Fix https interactive authentication problem
From: Ruedi Steinmann @ 2011-11-25 12:10 UTC (permalink / raw)
To: git
In-Reply-To: <20111123225121.GA23357@sigill.intra.peff.net>
Jeff King <peff <at> peff.net> writes:
>
> Hmm. What version of git are you using?
>
Hi Jeff,
I'm a little short of time at the moment, I'll come to this back later. So just
the quick infos. I discovered the error in my preinstalled version of git
(1.7.5.4). I reproduced the error with the version of git I have the source code
of: (which I should have done before, sorry)
../git/git/git clone https://n.ethz.ch/...
Cloning into 'masterthesis'...
warning: templates not found /home/ruedi/share/git-core/templates
Username for 'n.ethz.ch':
Password for 'n.ethz.ch':
error: The requested URL returned error: 401 (curl_result = 22, http_code = 401,
sha1 = 4e69f4215f0d8c02ec1c29aadcd4ea39bb4e7c5e)
error: Unable to find 4e69f4215f0d8c02ec1c29aadcd4ea39bb4e7c5e under
https://n.ethz.ch/.../masterthesis.git
Cannot obtain needed commit 4e69f4215f0d8c02ec1c29aadcd4ea39bb4e7c5e
while processing commit 0fa41a9d4b3282891173ec54c04e0e3763867807.
error: Fetch failed.
I am working with the git repository from https://github.com/gitster/git.git.
>From the first few lines of my commit from gitk:
Parent: 017d1e134545db0d162908f3538077eaa1f34fb6 (Update 1.7.8 draft release
notes in preparation for rc4)
Branch: master
Follows: v1.7.7.4, v1.7.8-rc3
Hope this helps
Cheers
Ruedi
^ permalink raw reply
* Re: what are the chances of a 'pre-upload' hook?
From: Sitaram Chamarty @ 2011-11-25 4:13 UTC (permalink / raw)
To: Git Mailing List
In-Reply-To: <CAMK1S_jaEWV=F6iHKZw_6u5ncDW0bPosNx-03W9bOLOfEEEY1Q@mail.gmail.com>
On Fri, Nov 25, 2011 at 8:46 AM, Sitaram Chamarty <sitaramc@gmail.com> wrote:
> (...and/or a post-upload hook)
>
> Has this ever come up?
Sorry for the google-fu fail and for replying to my own post.
http://git.661346.n2.nabble.com/Removal-of-post-upload-hook-td4394312.html
is the longest thread I (later) found.
The quick summary, in the words of Jeff (second post in that link) is:
"Because [upload]-pack runs as the user who is [fetching], not as the
repository owner. So by convincing you to [fetch from] my repository
in a multi-user environment, I convince you to run some arbitrary code
of mine."
My contention (today) is:
- you're already taking that risk for push
- so this would add a new risk only for people who fetch but don't push
- which, I submit, is a very small (if not almost empty) set of people
I may be wrong but I imagine shared environments are those where
almost everyone will push at least once in a while. It's a closed
group of people, probably all developers, etc etc etc...
Thanks for listening.
^ permalink raw reply
* Re: what are the chances of a 'pre-upload' hook?
From: Martin Fick @ 2011-11-25 3:22 UTC (permalink / raw)
To: Sitaram Chamarty, Git Mailing List, Sitaram Chamarty,
Git Mailing List
In-Reply-To: <dbf5142a-184b-46a2-9815-9ed82c95dfa7@email.android.com>
>Martin Fick <mfick@codeaurora.org> wrote:
>Sitaram Chamarty <sitaramc@gmail.com> wrote:
>
>>(...and/or a post-upload hook)
>>
>>Has this ever come up?
>
>Yes, previously it was seen as problematic (potentially too slow), but
>we are now open to having one. Patches welcome. :)
Doh sorry, I thought this was a Gerrit question! (Wrong mailing list)
Employee of Qualcomm Innovation Center,Inc. which is a member of Code Aurora Forum
^ permalink raw reply
* Re: what are the chances of a 'pre-upload' hook?
From: Martin Fick @ 2011-11-25 3:18 UTC (permalink / raw)
To: Sitaram Chamarty, Git Mailing List
In-Reply-To: <CAMK1S_jaEWV=F6iHKZw_6u5ncDW0bPosNx-03W9bOLOfEEEY1Q@mail.gmail.com>
Sitaram Chamarty <sitaramc@gmail.com> wrote:
>(...and/or a post-upload hook)
>
>Has this ever come up?
>
Yes, previously it was seen as problematic (potentially too slow), but we are now open to having one. Patches welcome. :)
Employee of Qualcomm Innovation Center,Inc. which is a member of Code Aurora Forum
^ permalink raw reply
* what are the chances of a 'pre-upload' hook?
From: Sitaram Chamarty @ 2011-11-25 3:16 UTC (permalink / raw)
To: Git Mailing List
(...and/or a post-upload hook)
Has this ever come up?
--
Sitaram
^ permalink raw reply
* [RFC/PATCH 2/3] t9301: Add 2nd testcase exposing bugs in fast-import's notes fanout handling
From: Johan Herland @ 2011-11-25 0:09 UTC (permalink / raw)
To: grubba; +Cc: git, jrnieder, johan
In-Reply-To: <1322179787-4422-1-git-send-email-johan@herland.net>
The previous patch exposed a bug in fast-import where _removing_ an existing
note fails (when that note resides on a non-zero fanout level, and was added
prior to this fast-import run).
This patch demostrates the same issue when _changing_ an existing note
(subject to the same circumstances).
Discovered-by: Henrik Grubbström <grubba@roxen.com>
Signed-off-by: Johan Herland <johan@herland.net>
---
t/t9301-fast-import-notes.sh | 54 ++++++++++++++++++++++++++++++++++++++++++
1 files changed, 54 insertions(+), 0 deletions(-)
diff --git a/t/t9301-fast-import-notes.sh b/t/t9301-fast-import-notes.sh
index fd08161..57d85a6 100755
--- a/t/t9301-fast-import-notes.sh
+++ b/t/t9301-fast-import-notes.sh
@@ -505,6 +505,60 @@ test_expect_success 'verify that non-notes are untouched by a fanout change' '
test_cmp expect_non-note3 actual
'
+
+# Change the notes for the three top commits
+test_tick
+cat >input <<INPUT_END
+commit refs/notes/many_notes
+committer $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE
+data <<COMMIT
+changing notes for the top three commits
+COMMIT
+from refs/notes/many_notes^0
+INPUT_END
+
+rm expect
+i=$num_commits
+j=0
+while test $j -lt 3
+do
+ cat >>input <<INPUT_END
+N inline refs/heads/many_commits~$j
+data <<EOF
+changed note for commit #$i
+EOF
+INPUT_END
+ cat >>expect <<EXPECT_END
+ commit #$i
+ changed note for commit #$i
+EXPECT_END
+ i=$(($i - 1))
+ j=$(($j + 1))
+done
+
+test_expect_failure 'change a few existing notes' '
+
+ git fast-import <input &&
+ GIT_NOTES_REF=refs/notes/many_notes git log -n3 refs/heads/many_commits |
+ grep "^ " > actual &&
+ test_cmp expect actual
+
+'
+
+test_expect_failure 'verify that changing notes respect existing fanout' '
+
+ # None of the entries in the top-level notes tree should be a full SHA1
+ git ls-tree --name-only refs/notes/many_notes |
+ while read path
+ do
+ if test $(expr length "$path") -ge 40
+ then
+ return 1
+ fi
+ done
+
+'
+
remaining_notes=10
test_tick
cat >input <<INPUT_END
--
1.7.5.rc1.3.g4d7b
^ permalink raw reply related
* [RFC/PATCH 0/3] fast-import: Fix incremental use of notes
From: Johan Herland @ 2011-11-25 0:09 UTC (permalink / raw)
To: grubba; +Cc: git, jrnieder, johan
In-Reply-To: <Pine.GSO.4.63.1111231137350.5099@shipon.roxen.com>
Hi,
This is a first attempt at fixing the bug reported by Henrik.
The first two patches provide testcases for the bug, and the last patch
provides a fix.
Have fun! :)
...Johan
Johan Herland (3):
t9301: Fix testcase covering up a bug in fast-import's notes fanout handling
t9301: Add 2nd testcase exposing bugs in fast-import's notes fanout handling
fast-import: Fix incorrect fanout level when modifying existing notes refs
fast-import.c | 28 ++++++++++++++++--
t/t9301-fast-import-notes.sh | 63 ++++++++++++++++++++++++++++++++++++++---
2 files changed, 83 insertions(+), 8 deletions(-)
--
1.7.5.rc1.3.g4d7b
^ permalink raw reply
* [RFC/PATCH 3/3] fast-import: Fix incorrect fanout level when modifying existing notes refs
From: Johan Herland @ 2011-11-25 0:09 UTC (permalink / raw)
To: grubba; +Cc: git, jrnieder, johan
In-Reply-To: <1322179787-4422-1-git-send-email-johan@herland.net>
This fixes the bug uncovered by the tests added in the previous two patches.
When an existing notes ref was loaded into the fast-import machinery, the
num_notes counter associated with that ref remained == 0, even though the
true number of notes in the loaded ref was higher. This caused a fanout
level of 0 to be used, although the actual fanout of the tree could be > 0.
Manipulating the notes tree at an incorrect fanout level causes removals to
silently fail, and modifications of existing notes to instead produce an
additional note (leaving the old object in place at a different fanout level).
This patch fixes the bug by explicitly counting the number of notes in the
notes tree whenever it looks like the num_notes counter could be wrong (when
num_notes == 0). There may be false positives (i.e. triggering the counting
when the notes tree is truly empty), but in those cases, the counting should
not take long.
Signed-off-by: Johan Herland <johan@herland.net>
---
fast-import.c | 28 +++++++++++++++++++++++++---
t/t9301-fast-import-notes.sh | 8 ++++----
2 files changed, 29 insertions(+), 7 deletions(-)
diff --git a/fast-import.c b/fast-import.c
index 8d8ea3c..f4bfe0f 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -2173,6 +2173,11 @@ static uintmax_t do_change_note_fanout(
if (tmp_hex_sha1_len == 40 && !get_sha1_hex(hex_sha1, sha1)) {
/* This is a note entry */
+ if (fanout == 0xff) {
+ /* Counting mode, no rename */
+ num_notes++;
+ continue;
+ }
construct_path_with_fanout(hex_sha1, fanout, realpath);
if (!strcmp(fullpath, realpath)) {
/* Note entry is in correct location */
@@ -2379,7 +2384,7 @@ static void file_change_cr(struct branch *b, int rename)
leaf.tree);
}
-static void note_change_n(struct branch *b, unsigned char old_fanout)
+static void note_change_n(struct branch *b, unsigned char *old_fanout)
{
const char *p = command_buf.buf + 2;
static struct strbuf uq = STRBUF_INIT;
@@ -2390,6 +2395,23 @@ static void note_change_n(struct branch *b, unsigned char old_fanout)
uint16_t inline_data = 0;
unsigned char new_fanout;
+ /*
+ * When loading a branch, we don't traverse its tree to count the real
+ * number of notes (too expensive to do this for all non-note refs).
+ * This means that recently loaded notes refs might incorrectly have
+ * b->num_notes == 0, and consequently, old_fanout might be wrong.
+ *
+ * Fix this by traversing the tree and counting the number of notes
+ * when b->num_notes == 0. If the notes tree is truly empty, the
+ * calculation should not take long.
+ */
+ if (b->num_notes == 0 && *old_fanout == 0) {
+ /* Invoke change_note_fanout() in "counting mode". */
+ b->num_notes = change_note_fanout(&b->branch_tree, 0xff);
+ *old_fanout = convert_num_notes_to_fanout(b->num_notes);
+ }
+
+ /* Now parse the notemodify command. */
/* <dataref> or 'inline' */
if (*p == ':') {
char *x;
@@ -2450,7 +2472,7 @@ static void note_change_n(struct branch *b, unsigned char old_fanout)
typename(type), command_buf.buf);
}
- construct_path_with_fanout(sha1_to_hex(commit_sha1), old_fanout, path);
+ construct_path_with_fanout(sha1_to_hex(commit_sha1), *old_fanout, path);
if (tree_content_remove(&b->branch_tree, path, NULL))
b->num_notes--;
@@ -2637,7 +2659,7 @@ static void parse_new_commit(void)
else if (!prefixcmp(command_buf.buf, "C "))
file_change_cr(b, 0);
else if (!prefixcmp(command_buf.buf, "N "))
- note_change_n(b, prev_fanout);
+ note_change_n(b, &prev_fanout);
else if (!strcmp("deleteall", command_buf.buf))
file_change_deleteall(b);
else if (!prefixcmp(command_buf.buf, "ls "))
diff --git a/t/t9301-fast-import-notes.sh b/t/t9301-fast-import-notes.sh
index 57d85a6..83acf68 100755
--- a/t/t9301-fast-import-notes.sh
+++ b/t/t9301-fast-import-notes.sh
@@ -536,7 +536,7 @@ EXPECT_END
j=$(($j + 1))
done
-test_expect_failure 'change a few existing notes' '
+test_expect_success 'change a few existing notes' '
git fast-import <input &&
GIT_NOTES_REF=refs/notes/many_notes git log -n3 refs/heads/many_commits |
@@ -545,7 +545,7 @@ test_expect_failure 'change a few existing notes' '
'
-test_expect_failure 'verify that changing notes respect existing fanout' '
+test_expect_success 'verify that changing notes respect existing fanout' '
# None of the entries in the top-level notes tree should be a full SHA1
git ls-tree --name-only refs/notes/many_notes |
@@ -594,7 +594,7 @@ EXPECT_END
i=$(($i - 1))
done
-test_expect_failure 'remove lots of notes' '
+test_expect_success 'remove lots of notes' '
git fast-import <input &&
GIT_NOTES_REF=refs/notes/many_notes git log refs/heads/many_commits |
@@ -603,7 +603,7 @@ test_expect_failure 'remove lots of notes' '
'
-test_expect_failure 'verify that removing notes trigger fanout consolidation' '
+test_expect_success 'verify that removing notes trigger fanout consolidation' '
# All entries in the top-level notes tree should be a full SHA1
git ls-tree --name-only -r refs/notes/many_notes |
--
1.7.5.rc1.3.g4d7b
^ 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