* Re: pack.packSizeLimit, safety checks
From: Shawn O. Pearce @ 2010-02-01 16:28 UTC (permalink / raw)
To: Johannes Sixt; +Cc: Nicolas Pitre, Sergio, git
In-Reply-To: <4B6700CF.1090106@viscovery.net>
Johannes Sixt <j.sixt@viscovery.net> wrote:
> Nicolas Pitre schrieb:
> > Grrrrr. This is a terrible discrepency given that all the other
> > arguments in Git are always byte based, with the optional k/m/g suffix,
> > by using git_parse_ulong(). So IMHO I'd just change --max-pack-size to
> > be in line with all the rest and have it accept bytes instead of MB.
> > And of course I'd push such a change to be included in v1.7.0 along with
> > the other incompatible fixes.
>
> While at it, also change --big-file-threshold that fast-import learnt the
> other day...
Yup. WTF was I thinking when I did megabytes as the default unit
on the command line...
--
Shawn.
^ permalink raw reply
* Re: pack.packSizeLimit, safety checks
From: Johannes Sixt @ 2010-02-01 16:26 UTC (permalink / raw)
To: Nicolas Pitre; +Cc: Sergio, git
In-Reply-To: <alpine.LFD.2.00.1002011100550.1681@xanadu.home>
Nicolas Pitre schrieb:
> Grrrrr. This is a terrible discrepency given that all the other
> arguments in Git are always byte based, with the optional k/m/g suffix,
> by using git_parse_ulong(). So IMHO I'd just change --max-pack-size to
> be in line with all the rest and have it accept bytes instead of MB.
> And of course I'd push such a change to be included in v1.7.0 along with
> the other incompatible fixes.
While at it, also change --big-file-threshold that fast-import learnt the
other day...
-- Hannes
^ permalink raw reply
* Re: Delivery Status Notification (Failure)
From: Laszlo Papp @ 2010-02-01 16:16 UTC (permalink / raw)
To: Frank Li; +Cc: git, normalperson
In-Reply-To: <1976ea661001290704h3ed4761dscf95ba848b373555@mail.gmail.com>
On Fri, Jan 29, 2010 at 4:04 PM, Frank Li <lznuaa@gmail.com> wrote:
>>
>> If I use --username option, Frank Li said me it's not enough in
>> TortoiseGIT to provide a popup facility to type the password related
>> to the set --username option.
>>
>
> I prefer git-svn can provide environment to launch a external
> application to input password like open ssh.
>
No perl guru among the volunteers if the original author is not available ? :(
Best Regards,
Laszlo Papp
^ permalink raw reply
* Re: pack.packSizeLimit, safety checks
From: Nicolas Pitre @ 2010-02-01 16:11 UTC (permalink / raw)
To: Sergio; +Cc: git
In-Reply-To: <loom.20100201T101056-232@post.gmane.org>
On Mon, 1 Feb 2010, Sergio wrote:
> Hi,
>
> documentation about pack.packSizeLimit
>
> says:
>
> The default maximum size of a pack. This setting only affects packing to a file,
> i.e. the git:// protocol is unaffected. It can be overridden by the
> --max-pack-size option of git-repack(1).
>
> I would suggest clarifying it into
>
> The default maximum size of a pack in bytes. This setting only affects packing
> to a file, i.e. the git:// protocol is unaffected. It can be overridden by the
> --max-pack-size option of git-repack(1).
>
> Since --max-pack-size takes MB and one might be tempted to assume that the same
> is valid for pack.packSizeLimit.
Grrrrr. This is a terrible discrepency given that all the other
arguments in Git are always byte based, with the optional k/m/g suffix,
by using git_parse_ulong(). So IMHO I'd just change --max-pack-size to
be in line with all the rest and have it accept bytes instead of MB.
And of course I'd push such a change to be included in v1.7.0 along with
the other incompatible fixes.
Your suggested precision above is still worth it of course.
> Also note that some safety check on pack.packSizeLimit could probably be
> desirable to avoid an unreasonably small limit. For instance:
>
> Assume that pack.packSizeLimit is set to 1 (believing it would be 1MB, but it is
> in fact 1B). With this at the first git gc every object goes in its own pack.
> You realize the mistake, you fix pack.packSizeLimit to 1000000, but at this
> point you cannot go back since git gc cannot run anymore (too many open files).
That's a totally orthogonal issue. There are other ways to get into
trouble with too many open files and that deserves a fix of its own
(such as limiting the number of simultaneous opened packs).
Nicolas
^ permalink raw reply
* Re: [PATCH 0/2] upload-pack: pre- and post- hooks
From: Shawn O. Pearce @ 2010-02-01 16:01 UTC (permalink / raw)
To: Arun Raghavan; +Cc: git
In-Reply-To: <6f8b45101002010750t5541faefv5b4640dfb9949306@mail.gmail.com>
Arun Raghavan <ford_prefect@gentoo.org> wrote:
> On 1 February 2010 20:50, Shawn O. Pearce <spearce@spearce.org> wrote:
> > Arun Raghavan <ford_prefect@gentoo.org> wrote:
> >> This patch set reintroduces the post-upload-pack hook and adds a
> >> pre-upload-pack hook. These are now only built if 'ALLOW_INSECURE_HOOKS' is set
> >> at build time. The idea is that only system administrators who need this
> >> functionality and are sure the potential insecurity is not relevant to their
> >> system will enable it.
> >
> > *sigh*
> >
> > I guess this is better, having it off by default, but allowing an
> > administrator who needs this feature to build a custom package.
> >
> > Unfortunately... I'm sure some distro out there is going to think
> > they know how to compile Git better than we do, and enable this by
> > default, exposing their users to a security hole. ?Ask the OpenSSL
> > project about how well distros package code... ?:-\
> >
> > I'd like a bit more than just a compile time flag.
>
> I was hoping the all-caps INSECURE in the name would give
> distributors pause. :)
>
> Suggestions on what else might work?
At one point we were talking about checking the owner of the hook
script itself. If it was uid 0 or the current actual user uid,
then we run the hook, otherwise we don't.
That only really works on POSIX platforms, but it does make some
sense. Root can already screw with you by replacing the binary
you are executing, so any hook they own is no more risky than the
git-upload-pack you just started.
If its the actual user uid, then systems like gitosis can still
make use of the hook by making the hook owned by the "git" user
that gitosis is executing all sessions under.
But mixed user systems, the hook would only run for the user who
created it, and be skipped for everyone else.
I'm not really sure what to do on Win32 here. Everyone is usually
Administrator these days which makes the test for "root" there
somewhat pointless. Maybe its just not enabled on Win32.
> >> At some point if the future, if needed, this could also be made a part of the
> >> negotiation between the client and server.
> >
> > I'm not sure I follow.
> >
> > Are you proposing the server advertises that it wants to run hooks,
> > and lets the client decide whether or not they should be executed?
>
> Something like that. I was thinking the client could always advertise
> whether the it wants to allow the hooks to be executed or not (which
> would override the default value of the global variable I introduced).
> Either approach would work, though the second is simpler but also
> dumber.
>
> Again, this might be over-complicating things, which is why I did not
> implement it. I just wanted to make a note of the fact that this could
> be done if the need is felt.
My concern with this is, users might disable the hook all of the
time, and then servers that actually want the hook (e.g. gentoo's
use of the pre-upload-pack to avoid initial clones over git://)
would be stuck, just like they are today.
No, its just not sane to give the user a choice whether or not they
should execute something remotely.
--
Shawn.
^ permalink raw reply
* Re: [PATCH 0/2] upload-pack: pre- and post- hooks
From: Arun Raghavan @ 2010-02-01 15:50 UTC (permalink / raw)
To: Shawn O. Pearce; +Cc: git
In-Reply-To: <20100201152010.GC8916@spearce.org>
On 1 February 2010 20:50, Shawn O. Pearce <spearce@spearce.org> wrote:
> Arun Raghavan <ford_prefect@gentoo.org> wrote:
>> This patch set reintroduces the post-upload-pack hook and adds a
>> pre-upload-pack hook. These are now only built if 'ALLOW_INSECURE_HOOKS' is set
>> at build time. The idea is that only system administrators who need this
>> functionality and are sure the potential insecurity is not relevant to their
>> system will enable it.
>
> *sigh*
>
> I guess this is better, having it off by default, but allowing an
> administrator who needs this feature to build a custom package.
>
> Unfortunately... I'm sure some distro out there is going to think
> they know how to compile Git better than we do, and enable this by
> default, exposing their users to a security hole. Ask the OpenSSL
> project about how well distros package code... :-\
>
> I'd like a bit more than just a compile time flag.
I was hoping the all-caps INSECURE in the name would give distributors pause. :)
Suggestions on what else might work?
>> At some point if the future, if needed, this could also be made a part of the
>> negotiation between the client and server.
>
> I'm not sure I follow.
>
> Are you proposing the server advertises that it wants to run hooks,
> and lets the client decide whether or not they should be executed?
Something like that. I was thinking the client could always advertise
whether the it wants to allow the hooks to be executed or not (which
would override the default value of the global variable I introduced).
Either approach would work, though the second is simpler but also
dumber.
Again, this might be over-complicating things, which is why I did not
implement it. I just wanted to make a note of the fact that this could
be done if the need is felt.
Cheers,
--
Arun Raghavan
http://arunraghavan.net/
(Ford_Prefect | Gentoo) & (arunsr | GNOME)
^ permalink raw reply
* [PATCH] fast-import: Document the core.bigFileThreshold configuration setting
From: Shawn O. Pearce @ 2010-02-01 15:41 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Sverre Rabbelier, git, Nicolas Pitre
In-Reply-To: <7v3a1oyrkp.fsf@alter.siamese.dyndns.org>
Signed-off-by: Shawn O. Pearce <spearce@spearce.org>
---
Junio C Hamano <gitster@pobox.com> wrote:
> You may want to add the new option to the output from "cmd -h" and
> probably description of the configuration in the doc before any of this
> gets official.
Doc patch. :-)
Feel free to squash, or to apply as its own change, I don't care
either way.
Documentation/config.txt | 14 ++++++++++++++
fast-import.c | 2 +-
2 files changed, 15 insertions(+), 1 deletions(-)
diff --git a/Documentation/config.txt b/Documentation/config.txt
index f7728ec..b16a20b 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -409,6 +409,20 @@ You probably do not need to adjust this value.
+
Common unit suffixes of 'k', 'm', or 'g' are supported.
+core.bigFileThreshold::
+ Files larger than this size are stored deflated, without
+ attempting delta compression. Storing large files without
+ delta compression avoids excessive memory usage, at the
+ slight expense of increased disk usage.
++
+Default is 512 MiB on all platforms. This should be reasonable
+for most projects as source code and other text files can still
+be delta compressed, but larger binary media files won't be.
++
+Common unit suffixes of 'k', 'm', or 'g' are supported.
++
+Currently only linkgit:git-fast-import[1] honors this setting.
+
core.excludesfile::
In addition to '.gitignore' (per-directory) and
'.git/info/exclude', git looks into this file for patterns
diff --git a/fast-import.c b/fast-import.c
index 8055f73..1bcf3bb 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -2613,7 +2613,7 @@ static int git_pack_config(const char *k, const char *v, void *cb)
}
static const char fast_import_usage[] =
-"git fast-import [--date-format=f] [--max-pack-size=n] [--depth=n] [--active-branches=n] [--export-marks=marks.file]";
+"git fast-import [--date-format=f] [--max-pack-size=n] [--big-file-threshold=n] [--depth=n] [--active-branches=n] [--export-marks=marks.file]";
int main(int argc, const char **argv)
{
--
1.7.0.rc0.170.g7207c
--
Shawn.
^ permalink raw reply related
* Re: [PATCH] fast-import: Stream very large blobs directly to pack
From: Shawn O. Pearce @ 2010-02-01 15:28 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Sverre Rabbelier, git, Nicolas Pitre
In-Reply-To: <7v3a1oyrkp.fsf@alter.siamese.dyndns.org>
Junio C Hamano <gitster@pobox.com> wrote:
> I'll queued this with a result of my conflict resolution to 'pu' for now
> but please double check after I push it out.
The strtoumax call got messed up. Squash this into your merge:
diff --git a/fast-import.c b/fast-import.c
index e6ebcf6..9c65a24 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -2800,7 +2800,7 @@ static int parse_one_option(const char *option)
if (!prefixcmp(option, "max-pack-size=")) {
option_max_pack_size(option + 14);
} else if (!prefixcmp(option, "big-file-threshold=")) {
- big_file_threshold = strtoumax(option + 21, NULL, 0) * 1024 * 1024;
+ big_file_threshold = strtoumax(option + 19, NULL, 0) * 1024 * 1024;
} else if (!prefixcmp(option, "depth=")) {
option_depth(option + 6);
} else if (!prefixcmp(option, "active-branches=")) {
> You may want to add the new option to the output from "cmd -h" and
> probably description of the configuration in the doc before any of this
> gets official.
I'll send an additional patch in a minute with these documentation
related updates.
--
Shawn.
^ permalink raw reply related
* Re: [PATCH] fast-import: Stream very large blobs directly to pack
From: Shawn O. Pearce @ 2010-02-01 15:23 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Sverre Rabbelier, git, Nicolas Pitre
In-Reply-To: <7vmxzw15dt.fsf@alter.siamese.dyndns.org>
Junio C Hamano <gitster@pobox.com> wrote:
> Junio C Hamano <gitster@pobox.com> writes:
> > "Shawn O. Pearce" <spearce@spearce.org> writes:
> >
> >> I was intentionally slating this for maint, to fix a bug a user
> >> reported when handling large streams.
> >
> > I personally see that as adding a new feature (especially with new option
> > and config).
>
> Sorry, but I take it back. The new codepath triggers even without any
> explicit request and _fixes_ the situation where old code simply failed,
> so it is worth queuing for the maintenance track.
Thanks.
> Do you want to do the deflatebound thing, or are we Ok without?
No, I think we're OK without it. The pack size limit stuff is only
a rough guess anyway. I never meant for us to be sticking to it to
within a byte. It was only meant to keep us from shoving a 100 MiB
blob onto the end of a pack once we got close to the size requested
by the user.
--
Shawn.
^ permalink raw reply
* Re: [PATCH 0/2] upload-pack: pre- and post- hooks
From: Shawn O. Pearce @ 2010-02-01 15:20 UTC (permalink / raw)
To: Arun Raghavan; +Cc: git
In-Reply-To: <1265013127-12589-1-git-send-email-ford_prefect@gentoo.org>
Arun Raghavan <ford_prefect@gentoo.org> wrote:
> This patch set reintroduces the post-upload-pack hook and adds a
> pre-upload-pack hook. These are now only built if 'ALLOW_INSECURE_HOOKS' is set
> at build time. The idea is that only system administrators who need this
> functionality and are sure the potential insecurity is not relevant to their
> system will enable it.
*sigh*
I guess this is better, having it off by default, but allowing an
administrator who needs this feature to build a custom package.
Unfortunately... I'm sure some distro out there is going to think
they know how to compile Git better than we do, and enable this by
default, exposing their users to a security hole. Ask the OpenSSL
project about how well distros package code... :-\
I'd like a bit more than just a compile time flag.
> At some point if the future, if needed, this could also be made a part of the
> negotiation between the client and server.
I'm not sure I follow.
Are you proposing the server advertises that it wants to run hooks,
and lets the client decide whether or not they should be executed?
--
Shawn.
^ permalink raw reply
* git-gui translation updates needed
From: Shawn O. Pearce @ 2010-02-01 15:16 UTC (permalink / raw)
To: git
git-gui has picked up a few new strings. Junio and I would like
to see the new version in git 1.7.0, which means we need to get
the new messages translated.
If you maintain a translation file for git-gui, please grab the
latest sources and send a patch for your .po file:
git://git.spearce.org/git-gui.git master
Thus far I think I have German, Italian and Swedish (thanks Christian
Stimming, Christian Stimming, Christian Stimming).
--
Shawn.
^ permalink raw reply
* Re: [RFC/PATCH 5/6] revert: add --ff option to allow fast forward when cherry-picking
From: Paolo Bonzini @ 2010-02-01 14:29 UTC (permalink / raw)
To: Christian Couder
Cc: Christian Couder, Junio C Hamano, git, Linus Torvalds,
Johannes Schindelin, Stephan Beyer, Daniel Barkalow
In-Reply-To: <c07716ae1002010443p4e0443feke14e5f877584640f@mail.gmail.com>
On 02/01/2010 01:43 PM, Christian Couder wrote:
> Maybe it could be the default, but in this case it should be made
> compatible with -n option (and perhaps other options) for backward
> compatibility, and this would probably need more involved changes.
A better objection is that GIT_COMMITTER_* is respected by |git
cherry-pick" but not by "git cherry-pick --ff", and that even without
setting the variables, "git cherry-pick" will pick a new commit date but
"git cherry-pick --ff" wouldn't. The latter, I think is the only
difference that is worth pondering further.
My impression is that a user would never have any problem with
fast-forwarding. For scripts probably the same is true (the typical
scenario for scripts is probably very similar to what "git rebase -i"
does), but it can still be a potential backwards-incompatibility in case
the script is *expecting* cherry-picking to generate a new SHA1. How
broken can we consider this expectation?
That said, to reply to your question, of course -n would disable it, and
so would -x, -s and possibly -e. But then, the same applies to --ff:
your patch forbids "-n --ff", but -x, -s and -e would need the same
treatment.
Note that "-e --ff" would error out; however if --ff would be the
default, "-e" would probably choose between fast-forward and
non-fast-forward depending on whether the commit message was edited.
Paolo
^ permalink raw reply
* Re: Problem listing GIT repository with accents
From: Jeff King @ 2010-02-01 13:44 UTC (permalink / raw)
To: Johannes Sixt
Cc: Junio C Hamano, Ellié Computing Open Source Program,
Pierre Habouzit, git
In-Reply-To: <20100201133903.GA923@coredump.intra.peff.net>
On Mon, Feb 01, 2010 at 08:39:03AM -0500, Jeff King wrote:
> If you want to keep the "!p[len]" condition, then yes. If we want to
> properly quote internal NULs (which again, I am not sure is of practical
> use), then you would also have to make that condition "(maxlen >= 0 &&
> !p[len])". Which is really not that bad, but I was trying to make it
> easier to read. I am OK with any of the three combinations of fixes.
>
> And the fact that I am using the word "combination" probably means it
> should be a separate patch anyway.
>
> So here is the minimal patch to go on 'maint', for Junio's convenience.
And here is what the "quote embedded NULs" patch would look like on top.
It's actually pretty straightforward, but the more I think of it, the
more I think it is probably not worthwhile. Not only are we quoting
paths, which should not have embedded NULs, but it requires that the
caller always pass the length explicitly, and clearly we are not doing
that all or even most of the time. So while this would fix the low-level
"this function quotes an arbitrary string" case, for it to be of any use
all of the code paths leading to it would need to be audited to handle
NUL-embedded strings.
---
diff --git a/quote.c b/quote.c
index 723bb4f..fc93435 100644
--- a/quote.c
+++ b/quote.c
@@ -213,7 +213,7 @@ static size_t quote_c_style_counted(const char *name, ssize_t maxlen,
int ch;
len = next_quote_pos(p, maxlen);
- if (len == maxlen || !p[len])
+ if (len == maxlen || (maxlen < 0 && !p[len]))
break;
if (!no_dq && p == name)
^ permalink raw reply related
* Re: Problem listing GIT repository with accents
From: Jeff King @ 2010-02-01 13:39 UTC (permalink / raw)
To: Johannes Sixt
Cc: Junio C Hamano, Ellié Computing Open Source Program,
Pierre Habouzit, git
In-Reply-To: <4B66CD81.3010005@viscovery.net>
On Mon, Feb 01, 2010 at 01:48:01PM +0100, Johannes Sixt wrote:
> Jeff King schrieb:
> > @@ -209,11 +209,14 @@ static size_t quote_c_style_counted(const char *name, ssize_t maxlen,
> > size_t len, count = 0;
> > const char *p = name;
> >
> > + if (maxlen < 0)
> > + maxlen = strlen(name);
> > +
> > for (;;) {
> > int ch;
> >
> > len = next_quote_pos(p, maxlen);
> > - if (len == maxlen || !p[len])
> > + if (len == maxlen)
> > break;
> >
> > if (!no_dq && p == name)
> > @@ -223,6 +226,7 @@ static size_t quote_c_style_counted(const char *name, ssize_t maxlen,
> > EMIT('\\');
> > p += len;
> > ch = (unsigned char)*p++;
> > + maxlen -= len + 1;
>
> Couldn't you just write
>
> if (0 <= maxlen)
> maxlen -= len + 1;
>
> to avoid the strlen(), because the rest of the loop is actually OK when
> maxlen == -1, isn't it?
If you want to keep the "!p[len]" condition, then yes. If we want to
properly quote internal NULs (which again, I am not sure is of practical
use), then you would also have to make that condition "(maxlen >= 0 &&
!p[len])". Which is really not that bad, but I was trying to make it
easier to read. I am OK with any of the three combinations of fixes.
And the fact that I am using the word "combination" probably means it
should be a separate patch anyway.
So here is the minimal patch to go on 'maint', for Junio's convenience.
-- >8 --
Subject: [PATCH] fix invalid read in quote_c_style_counted
This function did not work on strings that were not
NUL-terminated. It reads through a length-bounded string,
searching for characters in need of quoting. After we find
one, we output the quoted character, then advance our
pointer to find the next one. However, we never decremented
the length, meaning we ended up looking at whatever random
junk was stored after the string.
This bug was not found by the existing tests because most
code paths feed a NUL-terminated string. The notable
exception is a directory name being fed by ls-tree.
Signed-off-by: Jeff King <peff@peff.net>
---
quote.c | 2 ++
t/t3902-quoted.sh | 19 ++++++++++++++++++-
2 files changed, 20 insertions(+), 1 deletions(-)
diff --git a/quote.c b/quote.c
index acb6bf9..723bb4f 100644
--- a/quote.c
+++ b/quote.c
@@ -223,6 +223,8 @@ static size_t quote_c_style_counted(const char *name, ssize_t maxlen,
EMIT('\\');
p += len;
ch = (unsigned char)*p++;
+ if (maxlen >= 0)
+ maxlen -= len + 1;
if (sq_lookup[ch] >= ' ') {
EMIT(sq_lookup[ch]);
} else {
diff --git a/t/t3902-quoted.sh b/t/t3902-quoted.sh
index 5868052..14da45f 100755
--- a/t/t3902-quoted.sh
+++ b/t/t3902-quoted.sh
@@ -25,7 +25,7 @@ for_each_name () {
for name in \
Name "Name and a${LF}LF" "Name and an${HT}HT" "Name${DQ}" \
"$FN$HT$GN" "$FN$LF$GN" "$FN $GN" "$FN$GN" "$FN$DQ$GN" \
- "With SP in it"
+ "With SP in it" "caractère spécial/file"
do
eval "$1"
done
@@ -33,6 +33,7 @@ for_each_name () {
test_expect_success setup '
+ mkdir "caractère spécial" &&
for_each_name "echo initial >\"\$name\""
git add . &&
git commit -q -m Initial &&
@@ -50,6 +51,7 @@ Name
"Name and an\tHT"
"Name\""
With SP in it
+"caract\303\250re sp\303\251cial/file"
"\346\277\261\351\207\216\t\347\264\224"
"\346\277\261\351\207\216\n\347\264\224"
"\346\277\261\351\207\216 \347\264\224"
@@ -63,6 +65,7 @@ Name
"Name and an\tHT"
"Name\""
With SP in it
+caractère spécial/file
"濱野\t純"
"濱野\n純"
濱野 純
@@ -97,6 +100,13 @@ test_expect_success 'check fully quoted output from diff-tree' '
'
+test_expect_success 'check fully quoted output from ls-tree' '
+
+ git ls-tree --name-only -r HEAD >current &&
+ test_cmp expect.quoted current
+
+'
+
test_expect_success 'setting core.quotepath' '
git config --bool core.quotepath false
@@ -130,4 +140,11 @@ test_expect_success 'check fully quoted output from diff-tree' '
'
+test_expect_success 'check fully quoted output from ls-tree' '
+
+ git ls-tree --name-only -r HEAD >current &&
+ test_cmp expect.raw current
+
+'
+
test_done
--
1.7.0.rc1.16.g21332.dirty
^ permalink raw reply related
* Re: Bug in git-filter-branch example
From: Johannes Sixt @ 2010-02-01 12:57 UTC (permalink / raw)
To: Ivo Anjo; +Cc: git
In-Reply-To: <557ea2711002010348m57aa31fesd1047cbe3f01cb0b@mail.gmail.com>
Ivo Anjo schrieb:
> I've been working on importing my svn repo into git, and while moving
> some things around using git-filter-branch I ran into a bug in the
> example provided on the manpage:
Thanks for the report, but: Examples are just that: examples. IMO, it is
OK that there are implicit assumptions (such as that only "common" file
names are used in the repo).
Making the example universally applicable would greatly obfuscate the
important messages.
> I ended up using git filter-branch to remove the offending file ("if
> all you have is a hammer..."), and re-adding it after the move.
It seems that the example has met its objective: It showed you how to use
the hammer, and with your creativity mixed in, it enabled you to use the
hammer in new ways.
-- Hannes
^ permalink raw reply
* Re: Problem listing GIT repository with accents
From: Johannes Sixt @ 2010-02-01 12:48 UTC (permalink / raw)
To: Jeff King
Cc: Junio C Hamano, Ellié Computing Open Source Program,
Pierre Habouzit, git
In-Reply-To: <20100201121933.GA9995@coredump.intra.peff.net>
Jeff King schrieb:
> @@ -209,11 +209,14 @@ static size_t quote_c_style_counted(const char *name, ssize_t maxlen,
> size_t len, count = 0;
> const char *p = name;
>
> + if (maxlen < 0)
> + maxlen = strlen(name);
> +
> for (;;) {
> int ch;
>
> len = next_quote_pos(p, maxlen);
> - if (len == maxlen || !p[len])
> + if (len == maxlen)
> break;
>
> if (!no_dq && p == name)
> @@ -223,6 +226,7 @@ static size_t quote_c_style_counted(const char *name, ssize_t maxlen,
> EMIT('\\');
> p += len;
> ch = (unsigned char)*p++;
> + maxlen -= len + 1;
Couldn't you just write
if (0 <= maxlen)
maxlen -= len + 1;
to avoid the strlen(), because the rest of the loop is actually OK when
maxlen == -1, isn't it?
-- Hannes
^ permalink raw reply
* Re: [RFC/PATCH 5/6] revert: add --ff option to allow fast forward when cherry-picking
From: Christian Couder @ 2010-02-01 12:43 UTC (permalink / raw)
To: Paolo Bonzini
Cc: Christian Couder, Junio C Hamano, git, Linus Torvalds,
Johannes Schindelin, Stephan Beyer, Daniel Barkalow
In-Reply-To: <4B66B68C.7050505@gnu.org>
On Mon, Feb 1, 2010 at 12:10 PM, Paolo Bonzini <bonzini@gnu.org> wrote:
>> + OPT_BOOLEAN(0, "ff",&ff_ok, "allow fast forward"),
>
> Why should this not be the default?
Maybe it could be the default, but in this case it should be made
compatible with -n option
(and perhaps other options) for backward compatibility, and this would
probably need more
involved changes.
Thanks,
Christian.
> Instead, you'd add --no-ff. This would
> simplify 6/6 further, like
>
> eval sha1=\$$#
> ...
> output git cherry-pick "$@"
>
> Paolo
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
^ permalink raw reply
* Re: Bug in git-filter-branch example
From: Jeff King @ 2010-02-01 12:43 UTC (permalink / raw)
To: Ivo Anjo; +Cc: Junio C Hamano, git
In-Reply-To: <557ea2711002010348m57aa31fesd1047cbe3f01cb0b@mail.gmail.com>
On Mon, Feb 01, 2010 at 11:48:27AM +0000, Ivo Anjo wrote:
> To move the whole tree into a subdirectory, or remove it from there:
>
> git filter-branch --index-filter \
> 'git ls-files -s | sed "s-\t-&newsubdir/-" |
> GIT_INDEX_FILE=$GIT_INDEX_FILE.new \
> git update-index --index-info &&
> mv $GIT_INDEX_FILE.new $GIT_INDEX_FILE' HEAD
>
>
> The problem is with filenames that use complex utf8 (non-ascii?) chars:
Yes, that is definitely the problem. update-index unquotes its input, as
you would expect, but the sed munging does not take the quote into
account. You can fix it by doing:
sed "s-\t\"*-&newsubdir/-"
instead. Here is a patch to fix the documentation. I am slightly unsure
of whether it should be applied. These examples are supposed to be
simple and readable to help the user understand what the filters can do.
And this makes it somewhat less simple for the sake of a special case.
But at the same time, users are going to cut-and-paste these examples (I
know I have), and the special case is not _that_ special, especially for
non-English speakers. And having it in the example helps make people
aware that quoted paths are a reality.
So I think on balance it is probably better to fix it.
Note also that another way of "fixing" this would be to set
core.quotepath to false (which is something you probably want to do
anyway if you are using utf8 characters in your filenames). And I put
"fix" in quotes because you still may run across quoted paths, but they
will be much less common; you will only see them if you have control
characters or other insanity in your paths.
-- >8 --
Subject: [PATCH] docs: fix filter-branch example for quoted paths
If there is a quoted path, update-index will correctly
unquote it. However, we must take care to put our new prefix
inside the double-quote.
Signed-off-by: Jeff King <peff@peff.net>
---
Documentation/git-filter-branch.txt | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/Documentation/git-filter-branch.txt b/Documentation/git-filter-branch.txt
index cfaba2a..020028c 100644
--- a/Documentation/git-filter-branch.txt
+++ b/Documentation/git-filter-branch.txt
@@ -358,7 +358,7 @@ To move the whole tree into a subdirectory, or remove it from there:
---------------------------------------------------------------
git filter-branch --index-filter \
- 'git ls-files -s | sed "s-\t-&newsubdir/-" |
+ 'git ls-files -s | sed "s-\t\"*-&newsubdir/-" |
GIT_INDEX_FILE=$GIT_INDEX_FILE.new \
git update-index --index-info &&
mv $GIT_INDEX_FILE.new $GIT_INDEX_FILE' HEAD
--
1.7.0.rc1.16.g21332.dirty
^ permalink raw reply related
* Re: Problem listing GIT repository with accents
From: Jeff King @ 2010-02-01 12:19 UTC (permalink / raw)
To: Junio C Hamano
Cc: Ellié Computing Open Source Program, Pierre Habouzit, git
In-Reply-To: <20100201113213.GA22663@coredump.intra.peff.net>
On Mon, Feb 01, 2010 at 06:32:13AM -0500, Jeff King wrote:
> The patch below fixes it for me. This is the first time I've ever looked
> at this code, though, so an extra set of eyes is appreciated. I'm also
> not sure of the "!p[len]" termination that the loop uses (quoted in the
> context below). The string is explicitly not NUL-terminated, so why
> would that matter? I think that may have been covering up the bug in
> some cases.
OK, I see now. Callers can pass "-1" for the length to indicate that the
name is NUL-terminated. So my initial patch does work, but it ends up
decrementing the "-1", which happens to work because next_quote_pos just
checks that it is negative.
Here is an updated patch. The two changes are:
1. A test in t3902.
2. It converts the "-1" into strlen(name) before doing any work, which
means we can lose the test for a NUL-terminator. I think this makes
it much easier to see what is going on. And arguably we are fixing
a "bug" where embedded NULs in a length-bounded string erroneously
marked the end-of-string. In practice, I don't think it matters as
we are quoting paths, and NUL is not a valid character there.
It does come at the slight expense of running an extra strlen()
instead of discovering the length as we progress through the loop.
It's possible to fix that, but it makes the code a bit harder to
read.
Junio, I think this bug goes back to v1.5.4, which makes the fix
appropriate for 'maint'.
-- >8 --
Subject: [PATCH] fix invalid read in quote_c_style_counted
This function did not work on strings that were not
NUL-terminated. It reads through a length-bounded string,
searching for characters in need of quoting. After we find
one, we output the quoted character, then advance our
pointer to find the next one. However, we never decremented
the length, meaning we ended up looking at whatever random
junk was stored after the string.
On top of this, callers can pass in "-1" as the length to
indicate a NUL-terminated string, and we would also end our
loop upon seeing a NUL. To keep the code simple, let's just
convert "-1" cases into "strlen(name)".
This bug was not found by the existing tests because most
code paths feed a NUL-terminated string. The notable
exception is a directory name being fed by ls-tree.
Signed-off-by: Jeff King <peff@peff.net>
---
quote.c | 6 +++++-
t/t3902-quoted.sh | 19 ++++++++++++++++++-
2 files changed, 23 insertions(+), 2 deletions(-)
diff --git a/quote.c b/quote.c
index acb6bf9..e35ae50 100644
--- a/quote.c
+++ b/quote.c
@@ -209,11 +209,14 @@ static size_t quote_c_style_counted(const char *name, ssize_t maxlen,
size_t len, count = 0;
const char *p = name;
+ if (maxlen < 0)
+ maxlen = strlen(name);
+
for (;;) {
int ch;
len = next_quote_pos(p, maxlen);
- if (len == maxlen || !p[len])
+ if (len == maxlen)
break;
if (!no_dq && p == name)
@@ -223,6 +226,7 @@ static size_t quote_c_style_counted(const char *name, ssize_t maxlen,
EMIT('\\');
p += len;
ch = (unsigned char)*p++;
+ maxlen -= len + 1;
if (sq_lookup[ch] >= ' ') {
EMIT(sq_lookup[ch]);
} else {
diff --git a/t/t3902-quoted.sh b/t/t3902-quoted.sh
index 5868052..14da45f 100755
--- a/t/t3902-quoted.sh
+++ b/t/t3902-quoted.sh
@@ -25,7 +25,7 @@ for_each_name () {
for name in \
Name "Name and a${LF}LF" "Name and an${HT}HT" "Name${DQ}" \
"$FN$HT$GN" "$FN$LF$GN" "$FN $GN" "$FN$GN" "$FN$DQ$GN" \
- "With SP in it"
+ "With SP in it" "caractère spécial/file"
do
eval "$1"
done
@@ -33,6 +33,7 @@ for_each_name () {
test_expect_success setup '
+ mkdir "caractère spécial" &&
for_each_name "echo initial >\"\$name\""
git add . &&
git commit -q -m Initial &&
@@ -50,6 +51,7 @@ Name
"Name and an\tHT"
"Name\""
With SP in it
+"caract\303\250re sp\303\251cial/file"
"\346\277\261\351\207\216\t\347\264\224"
"\346\277\261\351\207\216\n\347\264\224"
"\346\277\261\351\207\216 \347\264\224"
@@ -63,6 +65,7 @@ Name
"Name and an\tHT"
"Name\""
With SP in it
+caractère spécial/file
"濱野\t純"
"濱野\n純"
濱野 純
@@ -97,6 +100,13 @@ test_expect_success 'check fully quoted output from diff-tree' '
'
+test_expect_success 'check fully quoted output from ls-tree' '
+
+ git ls-tree --name-only -r HEAD >current &&
+ test_cmp expect.quoted current
+
+'
+
test_expect_success 'setting core.quotepath' '
git config --bool core.quotepath false
@@ -130,4 +140,11 @@ test_expect_success 'check fully quoted output from diff-tree' '
'
+test_expect_success 'check fully quoted output from ls-tree' '
+
+ git ls-tree --name-only -r HEAD >current &&
+ test_cmp expect.raw current
+
+'
+
test_done
--
1.7.0.rc1.16.g21332.dirty
^ permalink raw reply related
* Re: master^ is not a local branch -- huh?!?
From: Steve Diver @ 2010-02-01 11:52 UTC (permalink / raw)
To: Junio C Hamano
Cc: Nicolas Pitre, Mark Lodato, Sverre Rabbelier, Git List, Ron1,
Jacob Helwig
On 30/01/2010 06:03, Junio C Hamano wrote:
>Nicolas Pitre <nico@fluxnic.net> writes:
>>First, I'm afraid that "Checking out commit 'foobar'" might be confusing
>>as this may happen through either a remote branch, a tag, or any random
>>commit. It seems to me that "Checking out 'v2.5'" is less confusing
>>than "Checking out commit 'v2.5'". But that's a minor detail and
>>probably a personal preference.
...
>>To the contrary: this "detached HEAD" is exactly what you need if you
>>want to relate to any documentation or perform a search for more
>>information. Like it or not, this detached HEAD term is exactly what
>>this Git concept is all about and how it is designated everywhere. The
>>sooner Git users see and learn about it the better.
>As I am not good at keeping track of different proposals to change this
>word here and that word there, I expect this will probably need at least
>few rotations of earth to get input from people in different timezones,
>and I think this is post 1.7.0 item anyway, I'll queue the attached draft
>in 'pu' and keep it there, to make it easier for others to tweak the
>message.
Would it be a safe assumption to describe a 'detached HEAD' state as
being synonymous with a (local) personal scratchpad or temporary
workspace based on and from the original committed object?
If this assumption is correct, then maybe this notion of a scratchpad
may be more intuitive and conceptual to new users without getting
bogged down with the necessary semantics of the terms used, but can
also preserve references to 'detached HEAD' in the documentation for a
fuller explanation.
A scratchpad or temporary workspace description alludes to its semi
permanent nature, and can warn that the subsequent commits may be lost
through aging and garbage collection until the user "commits" to
saving their progress through the creation a new branch, and thereby
making them permanent.
I must say the explanations presented in this thread have shed some
light on what is on the face of it a common trap that leaves new users
wondering "What happened to my work!" and I thank you all.
Steve
^ permalink raw reply
* Bug in git-filter-branch example
From: Ivo Anjo @ 2010-02-01 11:48 UTC (permalink / raw)
To: git
I've been working on importing my svn repo into git, and while moving
some things around using git-filter-branch I ran into a bug in the
example provided on the manpage:
To move the whole tree into a subdirectory, or remove it from there:
git filter-branch --index-filter \
'git ls-files -s | sed "s-\t-&newsubdir/-" |
GIT_INDEX_FILE=$GIT_INDEX_FILE.new \
git update-index --index-info &&
mv $GIT_INDEX_FILE.new $GIT_INDEX_FILE' HEAD
The problem is with filenames that use complex utf8 (non-ascii?) chars:
> ls -lah Papers/*Lea*
-rw-r--r-- 1 knuckles users 109K 2010-01-31 18:57 Papers/A Java Fork ⁄
Join Framework -- Lea.pdf
as you can see, I'm using '⁄' as a replacement for '/' on that
filename, since I obviously can't use '/' on a filename. The problem
is, git ls-files -s lists that file wrapped in quotes and with the
char value escaped
100644 c09309342037fa7d91f37651e2f16e981e4d739a 0 "Papers/A Java
Fork \342\201\204 Join Framework -- Lea.pdf"
while other files aren't, so the simple sed line provided won't work,
as it will output the new filename as newsubdir/"Papers...
I ended up using git filter-branch to remove the offending file ("if
all you have is a hammer..."), and re-adding it after the move.
I'm also left wondering how many uses of git ls-files out there on the
"internets" survive working on a filename such as this one. Maybe git
ls-files should have an option to output the real utf8 file name
instead of escaping the bytes...?
Ivo Anjo
^ permalink raw reply
* Re: Problem listing GIT repository with accents
From: Jeff King @ 2010-02-01 11:32 UTC (permalink / raw)
To: Ellié Computing Open Source Program
Cc: Pierre Habouzit, Junio C Hamano, git
In-Reply-To: <7E88665723814E46BCBA1A39E84C27A5@elrond>
On Mon, Feb 01, 2010 at 11:48:29AM +0100, Ellié Computing Open Source Program wrote:
> C:\temp\scc-tests\git>git ls-tree -r HEAD "caractère spécial"
> 100644 blob bf10a8b39e72c754ee1872fcdb13662cba6a8880 "caract\350re
> sp\351cial/\272plouf.txt"
>
> Note the spurious \272 which comes in the listing :(
> Trying again the same commands may give other spurious characters
> (each time we tried we get different _bad_ responses)
Looks like a bug. I was able to replicate it here, and valgrind notices
it, too:
==22720== Invalid read of size 1
==22720== at 0x80E77FF: next_quote_pos (quote.c:174)
==22720== by 0x80E783A: quote_c_style_counted (quote.c:215)
==22720== by 0x80E7D14: write_name_quotedpfx (quote.c:286)
==22720== by 0x80808F3: show_tree (builtin-ls-tree.c:114)
==22720== by 0x811000D: read_tree_recursive (tree.c:114)
==22720== by 0x81100E7: read_tree_recursive (tree.c:131)
==22720== by 0x8080CC2: cmd_ls_tree (builtin-ls-tree.c:173)
==22720== by 0x804B7FA: run_builtin (git.c:257)
==22720== by 0x804B958: handle_internal_command (git.c:412)
==22720== by 0x804BA2F: run_argv (git.c:454)
==22720== by 0x804BB97: main (git.c:525)
==22720== Address 0x43405b4 is 0 bytes after a block of size 20 alloc'd
==22720== at 0x4024C4C: malloc (vg_replace_malloc.c:195)
==22720== by 0x8115739: xmalloc (wrapper.c:20)
==22720== by 0x811005E: read_tree_recursive (tree.c:127)
==22720== by 0x8080CC2: cmd_ls_tree (builtin-ls-tree.c:173)
==22720== by 0x804B7FA: run_builtin (git.c:257)
==22720== by 0x804B958: handle_internal_command (git.c:412)
==22720== by 0x804BA2F: run_argv (git.c:454)
==22720== by 0x804BB97: main (git.c:525)
The patch below fixes it for me. This is the first time I've ever looked
at this code, though, so an extra set of eyes is appreciated. I'm also
not sure of the "!p[len]" termination that the loop uses (quoted in the
context below). The string is explicitly not NUL-terminated, so why
would that matter? I think that may have been covering up the bug in
some cases.
-- >8 --
Subject: [PATCH] fix invalid read in quote_c_style_counted
We progress through a length-bounded string, looking for
characters in need of quoting. After each character is
found, we output everything up until that character
literally, then the quoted character. We then advance our
string and look again. However, we never actually
decremented the length, meaning we ended up looking at
whatever random junk was stored after the string.
Signed-off-by: Jeff King <peff@peff.net>
---
quote.c | 1 +
1 files changed, 1 insertions(+), 0 deletions(-)
diff --git a/quote.c b/quote.c
index acb6bf9..392006d 100644
--- a/quote.c
+++ b/quote.c
@@ -216,20 +216,21 @@ static size_t quote_c_style_counted(const char *name, ssize_t maxlen,
if (len == maxlen || !p[len])
break;
if (!no_dq && p == name)
EMIT('"');
EMITBUF(p, len);
EMIT('\\');
p += len;
ch = (unsigned char)*p++;
+ maxlen -= len + 1;
if (sq_lookup[ch] >= ' ') {
EMIT(sq_lookup[ch]);
} else {
EMIT(((ch >> 6) & 03) + '0');
EMIT(((ch >> 3) & 07) + '0');
EMIT(((ch >> 0) & 07) + '0');
}
}
EMITBUF(p, len);
--
1.7.0.rc1.16.g21332.dirty
^ permalink raw reply related
* Re: [RFC/PATCH 5/6] revert: add --ff option to allow fast forward when cherry-picking
From: Paolo Bonzini @ 2010-02-01 11:10 UTC (permalink / raw)
To: Christian Couder
Cc: Junio C Hamano, git, Linus Torvalds, Johannes Schindelin,
Stephan Beyer, Daniel Barkalow
In-Reply-To: <20100201075542.3929.35967.chriscool@tuxfamily.org>
> + OPT_BOOLEAN(0, "ff",&ff_ok, "allow fast forward"),
Why should this not be the default? Instead, you'd add --no-ff. This
would simplify 6/6 further, like
eval sha1=\$$#
...
output git cherry-pick "$@"
Paolo
^ permalink raw reply
* Problem listing GIT repository with accents
From: Ellié Computing Open Source Program @ 2010-02-01 10:48 UTC (permalink / raw)
To: git
Dear all,
I'm writing a tool (ECMerge) which use the git command line to list folders
content (as of specific commits, tags and so on). This way our users can
browse a GIT repository in our GUI.
We are in front of the following problem: when a GIT repository contains a
folder with accents, the files names for its content are often prepended
with random characters. Here is a transcript of a list of operation
exhibiting the problem:
C:\temp\scc-tests\git>git ls-tree HEAD .
100644 blob 443d8625f771c421efd86c129483f9a139a4e85f "p\351p\351.txt"
C:\temp\scc-tests\git>mkdir "caractère spécial"
C:\temp\scc-tests\git>echo plouf > "caractère spécial\plouf.txt"
C:\temp\scc-tests\git>git add "caractère spécial"
C:\temp\scc-tests\git>git commit -m plouf
[master b94d9cb] plouf
1 files changed, 1 insertions(+), 0 deletions(-)
create mode 100644 "caract\350re sp\351cial/plouf.txt"
C:\temp\scc-tests\git>git ls-tree HEAD .
040000 tree d2b614bbfb4c5f39a32eb1309654262df113f605 "caract\350re
sp\351cial"
100644 blob 443d8625f771c421efd86c129483f9a139a4e85f "p\351p\351.txt"
C:\temp\scc-tests\git>git ls-tree -r HEAD .
100644 blob bf10a8b39e72c754ee1872fcdb13662cba6a8880 "caract\350re
sp\351cial/\272plouf.txt"
100644 blob 443d8625f771c421efd86c129483f9a139a4e85f "p\351p\351.txt"
C:\temp\scc-tests\git>git ls-tree -r HEAD "caractère spécial"
100644 blob bf10a8b39e72c754ee1872fcdb13662cba6a8880 "caract\350re
sp\351cial/\272plouf.txt"
Note the spurious \272 which comes in the listing :(
Trying again the same commands may give other spurious characters (each time
we tried we get different _bad_ responses)
Hope you can do somehing for that,
Best regards
Armel Asselin - from Ellié Computing
^ permalink raw reply
* Re: [RFC/PATCH 1/6] revert: libify cherry-pick and revert functionnality
From: Johannes Schindelin @ 2010-02-01 10:26 UTC (permalink / raw)
To: Christian Couder
Cc: Junio C Hamano, git, Linus Torvalds, Stephan Beyer,
Daniel Barkalow
In-Reply-To: <20100201075542.3929.38404.chriscool@tuxfamily.org>
Hi,
On Mon, 1 Feb 2010, Christian Couder wrote:
> From: Stephan Beyer <s-beyer@gmx.net>
>
> The goal of this commit is to abstract out "git cherry-pick" and
> "git revert" functionnality into a new pick_commit() function made
> of code from "builtin-revert.c".
>
> The new pick_commit() function is in a new "pick.c" file with an
> associated "pick.h".
>
> This function starts from the current index (not HEAD), and allow
> the effect of one commit replayed (either forward or backward) to
> that state, leaving the result in the index. So it makes it
> possible to replay many commits to the index in sequence without
> commiting in between.
>
> This commit is made of code from the sequencer GSoC project:
>
> git://repo.or.cz/git/sbeyer.git
>
> (at commit 5a78908b70ceb5a4ea9fd4b82f07ceba1f019079)
>
> And it contains some changes and comments suggested by Junio.
>
> The original commit in the sequencer project that introduced
> this change is: 94a568a78d243d7a6c13778bc6b7ac1eb46e48cc
>
> Mentored-by: Daniel Barkalow <barkalow@iabervon.org>
> Mentored-by: Christian Couder <chriscool@tuxfamily.org>
> Signed-off-by: Stephan Beyer <s-beyer@gmx.net>
> Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
> ---
I guess that this patch would look nicer using --patience. In its current
form, it would put too big a dent in my Git time budget, sorry, I will not
review it.
Ciao,
Dscho
^ 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