* 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
* [RFC/PATCH 1/3] t9301: Fix testcase covering up a bug 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>
There is a bug in fast-import where the fanout levels of an existing notes
tree being loaded into the fast-import machinery is disregarded. Instead, any
tree loaded is assumed to have a fanout level of 0. If the true fanout level
is deeper, any attempt to remove a note from that tree will silently fail
(as the note will not be found at fanout level 0).
However, this bug was covered up by the way in which the t9301 testcase was
written: When generating the fast-import commands to test mass removal of
notes, we appended these commands to an already existing 'input' file which
happened to already contain the fast-import commands used in the previous
subtest to generate the very same notes tree. This would normally be harmless
(but suboptimal) as the notes created were identical to the notes already
present in the notes tree. But the act of repeating all the notes additions
caused the internal fast-import data structures to recalculate the fanout,
instead of hanging on to the initial (incorrect) fanout (that causes the bug
described above). Thus, the subsequent removal of notes in the same 'input'
file would succeed, thereby covering up the bug described above.
This patch creates a new 'input' file instead of appending to the file from
the previous subtest. Thus, we end up properly testing removal of notes that
were added by a previous fast-import command. As a side effect, the notes
removal can no longer refer to commits using the marks set by the previous
fast-import run, instead the commits names must be referenced directly.
The underlying fast-import bug is still present after this patch, but now we
have at least uncovered it. Therefore, the affected subtests are labeled as
expected failures until the underlying bug is fixed.
Signed-off-by: Johan Herland <johan@herland.net>
---
t/t9301-fast-import-notes.sh | 13 ++++++-------
1 files changed, 6 insertions(+), 7 deletions(-)
diff --git a/t/t9301-fast-import-notes.sh b/t/t9301-fast-import-notes.sh
index 463254c..fd08161 100755
--- a/t/t9301-fast-import-notes.sh
+++ b/t/t9301-fast-import-notes.sh
@@ -507,7 +507,7 @@ test_expect_success 'verify that non-notes are untouched by a fanout change' '
'
remaining_notes=10
test_tick
-cat >>input <<INPUT_END
+cat >input <<INPUT_END
commit refs/notes/many_notes
committer $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE
data <<COMMIT
@@ -516,12 +516,11 @@ COMMIT
from refs/notes/many_notes^0
INPUT_END
-i=$remaining_notes
-while test $i -lt $num_commits
+i=$(($num_commits - $remaining_notes))
+for sha1 in $(git rev-list -n $i refs/heads/many_commits)
do
- i=$(($i + 1))
cat >>input <<INPUT_END
-N 0000000000000000000000000000000000000000 :$i
+N 0000000000000000000000000000000000000000 $sha1
INPUT_END
done
@@ -541,7 +540,7 @@ EXPECT_END
i=$(($i - 1))
done
-test_expect_success 'remove lots of notes' '
+test_expect_failure 'remove lots of notes' '
git fast-import <input &&
GIT_NOTES_REF=refs/notes/many_notes git log refs/heads/many_commits |
@@ -550,7 +549,7 @@ test_expect_success 'remove lots of notes' '
'
-test_expect_success 'verify that removing notes trigger fanout consolidation' '
+test_expect_failure '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
* Re: Incremental use of fast-import may cause conflicting notes
From: Jonathan Nieder @ 2011-11-24 23:09 UTC (permalink / raw)
To: Henrik Grubbström; +Cc: Git Mailing list, Johan Herland, David Barr
In-Reply-To: <Pine.GSO.4.63.1111231137350.5099@shipon.roxen.com>
Hi Henrik,
Henrik Grubbström wrote:
> Background: I have an incremental repository-walker creating a corresponding
> documentation repository from a source repository
> that uses git-notes to store its state, a use for which notes
> seem very suitable.
Nice.
[...]
> when fast-import is restarted
> it won't remember the fanout, and will start writing files in the root
> again. This means that there may be multiple notes-files for the same
> commit, eg both de/adbeef and deadbeef.
[...]
> The problem is probably due to b->num_notes not being initialized properly
> when the old non-empty root commit for the notes branch is loaded in
> parse_from()/parse_new_commit().
Sounds like a bug. Can you suggest a reproduction recipe (ideally as
a patch to t/t9301-fast-import-notes.sh), a fix, or both?
Thanks.
Regards,
Jonathan
^ permalink raw reply
* Re: [PATCH 12/13] credentials: add "store" helper
From: Jeff King @ 2011-11-24 20:09 UTC (permalink / raw)
To: Eric Sunshine; +Cc: git
In-Reply-To: <CAPig+cQ_eP0wG=AFB0rt-SueW-XxrFXKYAXr+4PneYC-dciyuw@mail.gmail.com>
On Thu, Nov 24, 2011 at 09:29:36AM -0500, Eric Sunshine wrote:
> > +When git needs authentication for a particular context URL context,
>
> "context URL context"?
Thanks. That is what I get for proofreading right before bed. :)
I squashed this and your other typo fix into the appropriate patches.
-Peff
^ permalink raw reply
* Re: [PATCH] make git-svn resilient to log.abbrevcommit = true
From: Eric Wong @ 2011-11-24 18:32 UTC (permalink / raw)
To: Thomas Rast; +Cc: Shahid Alam, git, gitster
In-Reply-To: <201111241411.05655.trast@student.ethz.ch>
Thomas Rast <trast@student.ethz.ch> wrote:
> Shahid Alam wrote:
> > Add --no-abbrev-commit arg to working_head_Info()'s invocation
> > of git log.
> [...]
> > @@ -1803,7 +1803,7 @@ sub cmt_sha2rev_batch {
> > sub working_head_info {
> > my ($head, $refs) = @_;
> > my @args = qw/log --no-color --no-decorate --first-parent
> > - --pretty=medium/;
> > + --no-abbrev-commit --pretty=medium/;
>
> Undeniably a bug, but to prevent the same from happening again,
> wouldn't it be better to rewrite it using simply
>
> rev-list --first-parent --pretty=medium
Yes, I've never been happy with using "git log" for any internals since
it's a porcelain. I'll gladly accept a tested patch which uses rev-list
instead. Thanks!
^ permalink raw reply
* Re: [PATCH 10/13] credentials: add "cache" helper
From: Eric Sunshine @ 2011-11-24 14:36 UTC (permalink / raw)
To: Jeff King; +Cc: git
In-Reply-To: <20111124110710.GH8417@sigill.intra.peff.net>
On Thu, Nov 24, 2011 at 6:07 AM, Jeff King <peff@peff.net> wrote:
> diff --git a/Documentation/gitcredentials.txt b/Documentation/gitcredentials.txt
> index 1e54117..a1592ed 100644
> --- a/Documentation/gitcredentials.txt
> +++ b/Documentation/gitcredentials.txt
> @@ -63,11 +63,18 @@ Credential helpers, on the other hand, are external programs from which git can
> [snip]
> +cache::
> +
> + Cache credentials in memory for a short period of time. See
> + linkgit:git-credential-cache[1] for details.
> +
> +You may may also have third-party helpers installed; search for
s/may may/may/
-- ES
^ permalink raw reply
* Re: [PATCH 12/13] credentials: add "store" helper
From: Eric Sunshine @ 2011-11-24 14:29 UTC (permalink / raw)
To: Jeff King; +Cc: git
In-Reply-To: <20111124110756.GJ8417@sigill.intra.peff.net>
On Thu, Nov 24, 2011 at 6:07 AM, Jeff King <peff@peff.net> wrote:
> diff --git a/Documentation/git-credential-store.txt b/Documentation/git-credential-store.txt
> new file mode 100644
> index 0000000..3dcffbd
> --- /dev/null
> +++ b/Documentation/git-credential-store.txt
> @@ -0,0 +1,75 @@
> [snip]
> +The `.git-credentials` file is stored in plaintext. Each credential is
> +stored on its own line as a URL like:
> +
> +------------------------------
> +https://user:pass@example.com
> +------------------------------
> +
> +When git needs authentication for a particular context URL context,
"context URL context"?
-- ES
^ permalink raw reply
* Re: [PATCH] make git-svn resilient to log.abbrevcommit = true
From: Thomas Rast @ 2011-11-24 13:11 UTC (permalink / raw)
To: Shahid Alam; +Cc: git, gitster, Eric Wong
In-Reply-To: <3A78B8F7-803C-4278-8B5F-4A1B02C9FF04@gmail.com>
[add Eric to Cc]
Shahid Alam wrote:
> Add --no-abbrev-commit arg to working_head_Info()'s invocation
> of git log.
[...]
> @@ -1803,7 +1803,7 @@ sub cmt_sha2rev_batch {
> sub working_head_info {
> my ($head, $refs) = @_;
> my @args = qw/log --no-color --no-decorate --first-parent
> - --pretty=medium/;
> + --no-abbrev-commit --pretty=medium/;
Undeniably a bug, but to prevent the same from happening again,
wouldn't it be better to rewrite it using simply
rev-list --first-parent --pretty=medium
?
--
Thomas Rast
trast@{inf,student}.ethz.ch
^ permalink raw reply
* Re: [PATCH 0/13] credential helpers, take two
From: Erik Faye-Lund @ 2011-11-24 12:08 UTC (permalink / raw)
To: Jeff King; +Cc: git
In-Reply-To: <20111124115351.GA29465@sigill.intra.peff.net>
On Thu, Nov 24, 2011 at 12:53 PM, Jeff King <peff@peff.net> wrote:
> On Thu, Nov 24, 2011 at 12:45:39PM +0100, Erik Faye-Lund wrote:
>
>> On Thu, Nov 24, 2011 at 11:58 AM, Jeff King <peff@peff.net> wrote:
>> > Here's a revised version of the http-auth / credential-helper series.
>> >
>>
>> Nice. Do you have the branch somewhere public I can pull it from?
>
> git://github.com/peff/git.git jk/http-auth-simple
>
Thanks.
> But be aware that I do rebase my topic branches frequently.
>
That's fine by me, I just didn't want to have to download each patch
as a raw e-mail and apply it manually ;)
^ permalink raw reply
* Re: [PATCH 0/13] credential helpers, take two
From: Jeff King @ 2011-11-24 11:53 UTC (permalink / raw)
To: Erik Faye-Lund; +Cc: git
In-Reply-To: <CABPQNSaT+4F=EW_agAh_FY0h_ZRgMCzpukdKuvZTdfmz5_Nueg@mail.gmail.com>
On Thu, Nov 24, 2011 at 12:45:39PM +0100, Erik Faye-Lund wrote:
> On Thu, Nov 24, 2011 at 11:58 AM, Jeff King <peff@peff.net> wrote:
> > Here's a revised version of the http-auth / credential-helper series.
> >
>
> Nice. Do you have the branch somewhere public I can pull it from?
git://github.com/peff/git.git jk/http-auth-simple
But be aware that I do rebase my topic branches frequently.
-Peff
^ permalink raw reply
* Re: git-bisect working only from toplevel dir
From: Junio C Hamano @ 2011-11-24 11:50 UTC (permalink / raw)
To: Peter Baumann; +Cc: Jeff King, Adam Borowski, git
In-Reply-To: <20111124070659.GC6291@m62s10.vlinux.de>
Peter Baumann <waste.manager@gmx.de> writes:
> On Wed, Nov 23, 2011 at 12:45:18PM -0800, Junio C Hamano wrote:
> ...
>> Also didn't we make bisect workable in a bare repository recently? So the
>> start-up sequence has to be something more elaborate like...
>> ...
>> and then inside bisect_next() you would check if $prefix exists, and go
>> there to run bisect--helper (or fail to go there and say "cannot test").
>
> But is the "cannot test" aka exit(127) the best we can do in this case?
Yeah, thinking about it a bit more, it may probably be better to make it a
failure. The user explicitly asked "be in _this_ directory and run make;
it should succeed for the bisection test to pass". If the bisection test
criterion the user was interested in was a successful build of the whole
project (not the subpart of the current directory), the user would have
gone up to the top-level and "bisect run make" there.
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox