* Re: [PATCH 0/9] respect binary attribute in grep
From: Jeff King @ 2012-02-02 11:07 UTC (permalink / raw)
To: Thomas Rast
Cc: Junio C Hamano, Thomas Rast, Conrad Irwin, git,
Nguyen Thai Ngoc Duy, Dov Grobgeld
In-Reply-To: <87vcnp5wkg.fsf@thomas.inf.ethz.ch>
On Thu, Feb 02, 2012 at 12:00:47PM +0100, Thomas Rast wrote:
> My original plan was to make use_threads git-global, instead of
> grep-global (and shift responsibility to the subsystems instead of their
> users), but that's just me and the patches aren't ready yet.
Yeah, having just dug into the threading code in grep a bit, I agree
that would be a saner approach. The locking is all bolted-on, so you end
up with these weird contracts between code, like the low-level grep code
asking anybody who might be multi-threading it to initialize the mutexes
to cover access to a totally different subsystem. I'd much rather each
subsystem just take care of itself.
-Peff
^ permalink raw reply
* [PATCH 3/3] vcs-svn: suppress a -Wtype-limits warning
From: Jonathan Nieder @ 2012-02-02 11:06 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Ramsay Jones, David Barr, GIT Mailing-list, Dmitry Ivankov
In-Reply-To: <20120202104128.GG3823@burratino>
On 32-bit architectures with 64-bit file offsets, gcc 4.3 and earlier
produce the following warning:
CC vcs-svn/sliding_window.o
vcs-svn/sliding_window.c: In function `check_overflow':
vcs-svn/sliding_window.c:36: warning: comparison is always false \
due to limited range of data type
The warning appears even when gcc is run without any warning flags
(this is gcc bug 12963). In later versions the same warning can be
reproduced with -Wtype-limits, which is implied by -Wextra.
On 64-bit architectures it really is possible for a size_t not to be
representable as an off_t so the check this is warning about is not
actually redundant. But even false positives are distracting. Avoid
the warning by making the "len" argument to check_overflow a
uintmax_t; no functional change intended.
Reported-by: Ramsay Jones <ramsay@ramsay1.demon.co.uk>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
That's the end of the series. I hope it was entertaining.
Thoughts of all kinds welcome, as usual.
Jonathan
vcs-svn/sliding_window.c | 6 +++---
1 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/vcs-svn/sliding_window.c b/vcs-svn/sliding_window.c
index fafa4a63..2f4ae60f 100644
--- a/vcs-svn/sliding_window.c
+++ b/vcs-svn/sliding_window.c
@@ -31,15 +31,15 @@ static int read_to_fill_or_whine(struct line_buffer *file,
return 0;
}
-static int check_overflow(off_t offset, size_t len)
+static int check_overflow(off_t offset, uintmax_t len)
{
if (len > maximum_signed_value_of_type(off_t))
return error("unrepresentable length in delta: "
- "%"PRIuMAX" > OFF_MAX", (uintmax_t) len);
+ "%"PRIuMAX" > OFF_MAX", len);
if (signed_add_overflows(offset, (off_t) len))
return error("unrepresentable offset in delta: "
"%"PRIuMAX" + %"PRIuMAX" > OFF_MAX",
- (uintmax_t) offset, (uintmax_t) len);
+ (uintmax_t) offset, len);
return 0;
}
--
1.7.9
^ permalink raw reply related
* Re: [PATCH 1/3] vcs-svn: rename check_overflow arguments for clarity
From: Dmitry Ivankov @ 2012-02-02 11:05 UTC (permalink / raw)
To: Jonathan Nieder
Cc: Junio C Hamano, Ramsay Jones, David Barr, GIT Mailing-list
In-Reply-To: <20120202105923.GJ3823@burratino>
Hi
On Thu, Feb 2, 2012 at 4:59 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:
> From: Ramsay Jones <ramsay@ramsay1.demon.co.uk>
>
> Code using the argument names a and b just doesn't look right (not
> sure why!). Use more explicit names "offset" and "len" to make their
> type and function clearer.
Well, it's still not clear. Given off_t a, size_t b, check that a+b
fits into type... which type?
"offset" and "length" don't imply that it's "type of offset" or maybe
"type of length".
>
> Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
> ---
> Split out from Ramsay's patch.
>
> vcs-svn/sliding_window.c | 10 +++++-----
> 1 files changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/vcs-svn/sliding_window.c b/vcs-svn/sliding_window.c
> index 1bac7a4c..fafa4a63 100644
> --- a/vcs-svn/sliding_window.c
> +++ b/vcs-svn/sliding_window.c
> @@ -31,15 +31,15 @@ static int read_to_fill_or_whine(struct line_buffer *file,
> return 0;
> }
>
> -static int check_overflow(off_t a, size_t b)
> +static int check_overflow(off_t offset, size_t len)
> {
> - if (b > maximum_signed_value_of_type(off_t))
> + if (len > maximum_signed_value_of_type(off_t))
> return error("unrepresentable length in delta: "
> - "%"PRIuMAX" > OFF_MAX", (uintmax_t) b);
> + "%"PRIuMAX" > OFF_MAX", (uintmax_t) len);
> - if (signed_add_overflows(a, (off_t) b))
> + if (signed_add_overflows(offset, (off_t) len))
> return error("unrepresentable offset in delta: "
> "%"PRIuMAX" + %"PRIuMAX" > OFF_MAX",
> - (uintmax_t) a, (uintmax_t) b);
> + (uintmax_t) offset, (uintmax_t) len);
> return 0;
> }
>
> --
> 1.7.9
>
^ permalink raw reply
* [PATCH 2/3] vcs-svn: allow import of > 4GiB files
From: Jonathan Nieder @ 2012-02-02 11:03 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Ramsay Jones, David Barr, GIT Mailing-list, Dmitry Ivankov
In-Reply-To: <20120202104128.GG3823@burratino>
There is no reason in principle that an svn-format dump would not be
able to represent a file whose length does not fit in a 32-bit
integer. Use off_t consistently to represent file lengths (in place
of using uint32_t in some contexts) so we can handle that.
Most svn-fe code is already ready to do that without this patch and
passes values of type off_t around. The type mismatch from stragglers
was noticed with gcc -Wtype-limits.
While at it, tighten the parsing of the Text-content-length field to
make sure it is a number and does not overflow, and tighten other
overflow checks as that value is passed around and manipulated.
Inspired-by: Ramsay Jones <ramsay@ramsay1.demon.co.uk>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
vcs-svn/fast_export.c | 15 +++++++++------
vcs-svn/fast_export.h | 4 ++--
vcs-svn/svndump.c | 21 +++++++++++++++------
3 files changed, 26 insertions(+), 14 deletions(-)
diff --git a/vcs-svn/fast_export.c b/vcs-svn/fast_export.c
index 19d7c34c..b823b851 100644
--- a/vcs-svn/fast_export.c
+++ b/vcs-svn/fast_export.c
@@ -227,15 +227,18 @@ static long apply_delta(off_t len, struct line_buffer *input,
return ret;
}
-void fast_export_data(uint32_t mode, uint32_t len, struct line_buffer *input)
+void fast_export_data(uint32_t mode, off_t len, struct line_buffer *input)
{
+ assert(len >= 0);
if (mode == REPO_MODE_LNK) {
/* svn symlink blobs start with "link " */
+ if (len < 5)
+ die("invalid dump: symlink too short for \"link\" prefix");
len -= 5;
if (buffer_skip_bytes(input, 5) != 5)
die_short_read(input);
}
- printf("data %"PRIu32"\n", len);
+ printf("data %"PRIuMAX"\n", (uintmax_t) len);
if (buffer_copy_bytes(input, len) != len)
die_short_read(input);
fputc('\n', stdout);
@@ -297,12 +300,12 @@ int fast_export_ls(const char *path, uint32_t *mode, struct strbuf *dataref)
void fast_export_blob_delta(uint32_t mode,
uint32_t old_mode, const char *old_data,
- uint32_t len, struct line_buffer *input)
+ off_t len, struct line_buffer *input)
{
long postimage_len;
- if (len > maximum_signed_value_of_type(off_t))
- die("enormous delta");
- postimage_len = apply_delta((off_t) len, input, old_data, old_mode);
+
+ assert(len >= 0);
+ postimage_len = apply_delta(len, input, old_data, old_mode);
if (mode == REPO_MODE_LNK) {
buffer_skip_bytes(&postimage, strlen("link "));
postimage_len -= strlen("link ");
diff --git a/vcs-svn/fast_export.h b/vcs-svn/fast_export.h
index 43d05b65..aa629f54 100644
--- a/vcs-svn/fast_export.h
+++ b/vcs-svn/fast_export.h
@@ -14,10 +14,10 @@ void fast_export_begin_commit(uint32_t revision, const char *author,
const struct strbuf *log, const char *uuid,
const char *url, unsigned long timestamp);
void fast_export_end_commit(uint32_t revision);
-void fast_export_data(uint32_t mode, uint32_t len, struct line_buffer *input);
+void fast_export_data(uint32_t mode, off_t len, struct line_buffer *input);
void fast_export_blob_delta(uint32_t mode,
uint32_t old_mode, const char *old_data,
- uint32_t len, struct line_buffer *input);
+ off_t len, struct line_buffer *input);
/* If there is no such file at that rev, returns -1, errno == ENOENT. */
int fast_export_ls_rev(uint32_t rev, const char *path,
diff --git a/vcs-svn/svndump.c b/vcs-svn/svndump.c
index ca63760f..644fdc71 100644
--- a/vcs-svn/svndump.c
+++ b/vcs-svn/svndump.c
@@ -40,7 +40,8 @@
static struct line_buffer input = LINE_BUFFER_INIT;
static struct {
- uint32_t action, propLength, textLength, srcRev, type;
+ uint32_t action, propLength, srcRev, type;
+ off_t text_length;
struct strbuf src, dst;
uint32_t text_delta, prop_delta;
} node_ctx;
@@ -61,7 +62,7 @@ static void reset_node_ctx(char *fname)
node_ctx.type = 0;
node_ctx.action = NODEACT_UNKNOWN;
node_ctx.propLength = LENGTH_UNKNOWN;
- node_ctx.textLength = LENGTH_UNKNOWN;
+ node_ctx.text_length = -1;
strbuf_reset(&node_ctx.src);
node_ctx.srcRev = 0;
strbuf_reset(&node_ctx.dst);
@@ -209,7 +210,7 @@ static void handle_node(void)
{
const uint32_t type = node_ctx.type;
const int have_props = node_ctx.propLength != LENGTH_UNKNOWN;
- const int have_text = node_ctx.textLength != LENGTH_UNKNOWN;
+ const int have_text = node_ctx.text_length != -1;
/*
* Old text for this node:
* NULL - directory or bug
@@ -291,12 +292,12 @@ static void handle_node(void)
}
if (!node_ctx.text_delta) {
fast_export_modify(node_ctx.dst.buf, node_ctx.type, "inline");
- fast_export_data(node_ctx.type, node_ctx.textLength, &input);
+ fast_export_data(node_ctx.type, node_ctx.text_length, &input);
return;
}
fast_export_modify(node_ctx.dst.buf, node_ctx.type, "inline");
fast_export_blob_delta(node_ctx.type, old_mode, old_data,
- node_ctx.textLength, &input);
+ node_ctx.text_length, &input);
}
static void begin_revision(void)
@@ -409,7 +410,15 @@ void svndump_read(const char *url)
break;
case sizeof("Text-content-length"):
if (!constcmp(t, "Text-content-length")) {
- node_ctx.textLength = atoi(val);
+ char *end;
+ uintmax_t textlen;
+
+ textlen = strtoumax(val, &end, 10);
+ if (!isdigit(*val) || *end)
+ die("invalid dump: non-numeric length %s", val);
+ if (textlen > maximum_signed_value_of_type(off_t))
+ die("unrepresentable length in dump: %s", val);
+ node_ctx.text_length = (off_t) textlen;
break;
}
if (constcmp(t, "Prop-content-length"))
--
1.7.9
^ permalink raw reply related
* Re: [PATCH 0/9] respect binary attribute in grep
From: Thomas Rast @ 2012-02-02 11:00 UTC (permalink / raw)
To: Jeff King
Cc: Junio C Hamano, Thomas Rast, Conrad Irwin, git,
Nguyen Thai Ngoc Duy, Dov Grobgeld
In-Reply-To: <20120202081747.GA10271@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> [+cc Thomas, as I am mangling some of his recent work with my
> refactoring]
Mangling? I think it all looks very good.
My original plan was to make use_threads git-global, instead of
grep-global (and shift responsibility to the subsystems instead of their
users), but that's just me and the patches aren't ready yet.
--
Thomas Rast
trast@{inf,student}.ethz.ch
^ permalink raw reply
* Re: [PATCH v3 1/4] completion: be nicer with zsh
From: Felipe Contreras @ 2012-02-02 11:00 UTC (permalink / raw)
To: Thomas Rast; +Cc: Jonathan Nieder, git, SZEDER Gábor, Junio C Hamano
In-Reply-To: <8739at8qw6.fsf@thomas.inf.ethz.ch>
On Thu, Feb 2, 2012 at 12:35 PM, Thomas Rast <trast@inf.ethz.ch> wrote:
> Felipe Contreras <felipe.contreras@gmail.com> writes:
>
>> On Thu, Feb 2, 2012 at 10:48 AM, Jonathan Nieder <jrnieder@gmail.com> wrote:
>>
>> Exactly, and "completion: avoid default value assignment on : true
>> command" tells *nothing* to most people. Why is this patch needed? Do
>> I care about it?
>>
>> OTOH "completion: be nicer with zsh" explains the purpose of the patch
>> and people that don't care about zsh can happily ignore it if they
>> want, and the ones that care about zsh might want to back port it, or
>> whatever.
>
> Perhaps you could compromise on
>
> completion: work around zsh word splitting bug in : ${foo:=$(bar)}
Yes, that sounds better, perhaps:
completion: work around zsh option propagation bug
>> | tl;dr: $__git_porcelain_commands = $__git_all_commands
>>
>> Wrapping it up, to make clear what happens.
>
> I think this is not good style for a commit message. Apart from the
> very trendy use of tl;dr, it doesn't even properly summarize the cause
> *or* the user-visible symptom. It just states how the confusion
> propagates somewhere in the middle of the code.
I don't think the cause or the user-visible symptom need any summary.
I was trying to summarize this:
---
This is because in zsh the following code:
for i in $__git_all_commands
would evaluate $__git_all_commands as a single word (with spaces),
${=__git_all_commands} should be used to do word splitting expansion
(unless SH_WORD_SPLIT is used).
sh emulation should take care of that, but the command expantion is
messing up with that.
---
--
Felipe Contreras
^ permalink raw reply
* [PATCH 1/3] vcs-svn: rename check_overflow arguments for clarity
From: Jonathan Nieder @ 2012-02-02 10:59 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Ramsay Jones, David Barr, GIT Mailing-list, Dmitry Ivankov
In-Reply-To: <20120202104128.GG3823@burratino>
From: Ramsay Jones <ramsay@ramsay1.demon.co.uk>
Code using the argument names a and b just doesn't look right (not
sure why!). Use more explicit names "offset" and "len" to make their
type and function clearer.
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
Split out from Ramsay's patch.
vcs-svn/sliding_window.c | 10 +++++-----
1 files changed, 5 insertions(+), 5 deletions(-)
diff --git a/vcs-svn/sliding_window.c b/vcs-svn/sliding_window.c
index 1bac7a4c..fafa4a63 100644
--- a/vcs-svn/sliding_window.c
+++ b/vcs-svn/sliding_window.c
@@ -31,15 +31,15 @@ static int read_to_fill_or_whine(struct line_buffer *file,
return 0;
}
-static int check_overflow(off_t a, size_t b)
+static int check_overflow(off_t offset, size_t len)
{
- if (b > maximum_signed_value_of_type(off_t))
+ if (len > maximum_signed_value_of_type(off_t))
return error("unrepresentable length in delta: "
- "%"PRIuMAX" > OFF_MAX", (uintmax_t) b);
+ "%"PRIuMAX" > OFF_MAX", (uintmax_t) len);
- if (signed_add_overflows(a, (off_t) b))
+ if (signed_add_overflows(offset, (off_t) len))
return error("unrepresentable offset in delta: "
"%"PRIuMAX" + %"PRIuMAX" > OFF_MAX",
- (uintmax_t) a, (uintmax_t) b);
+ (uintmax_t) offset, (uintmax_t) len);
return 0;
}
--
1.7.9
^ permalink raw reply related
* Re: [PATCH v3 1/4] completion: be nicer with zsh
From: Jonathan Nieder @ 2012-02-02 10:55 UTC (permalink / raw)
To: Thomas Rast; +Cc: Felipe Contreras, git, SZEDER Gábor, Junio C Hamano
In-Reply-To: <20120202105006.GH3823@burratino>
Jonathan Nieder wrote:
> Thomas Rast wrote:
>> Perhaps you could compromise on
>>
>> completion: work around zsh word splitting bug in : ${foo:=$(bar)}
>
> Thanks, that looks like a good subject line to me.
Ah, except it's not a word splitting bug. It's an option propagation
bug.
^ permalink raw reply
* Re: [PATCH v3 1/4] completion: be nicer with zsh
From: Jonathan Nieder @ 2012-02-02 10:50 UTC (permalink / raw)
To: Thomas Rast; +Cc: Felipe Contreras, git, SZEDER Gábor, Junio C Hamano
In-Reply-To: <8739at8qw6.fsf@thomas.inf.ethz.ch>
Thomas Rast wrote:
> Perhaps you could compromise on
>
> completion: work around zsh word splitting bug in : ${foo:=$(bar)}
Thanks, that looks like a good subject line to me. It gives a hint of
what the patch is trying to do, and it does not try to fool me into
thinking that if I use bash the patch does not affect me.
Felipe, I'm not going to respond to the rest of your message. Perhaps
someone more patient than I am will, or if someone has specific
questions for me, I'll be glad to help.
^ permalink raw reply
* [PATCH/RFC 0/3] Re: [PATCH] vcs-svn: Fix some compiler warnings
From: Jonathan Nieder @ 2012-02-02 10:41 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Ramsay Jones, David Barr, GIT Mailing-list, Dmitry Ivankov
In-Reply-To: <7vipjpzxav.fsf@alter.siamese.dyndns.org>
Junio C Hamano wrote:
> I'd hold the branch in 'next' for now, until this gets resolved (one
> possible resolution is to declare Ramsey's patch is good enough for now,
> and do the follow-up later).
Sounds sensible. How about the following? Intended to replace
Ramsay's patch. Not well tested yet.
By the way, wouldn't the (p->field < 0) test in grep.c line 330
trigger the same compiler bug?
Jonathan Nieder (2):
vcs-svn: allow import of > 4GiB files
vcs-svn: suppress a -Wtype-limits warning
Ramsay Allan Jones (1):
vcs-svn: rename check_overflow arguments for clarity
vcs-svn/fast_export.c | 15 +++++++++------
vcs-svn/fast_export.h | 4 ++--
vcs-svn/sliding_window.c | 10 +++++-----
vcs-svn/svndump.c | 21 +++++++++++++++------
4 files changed, 31 insertions(+), 19 deletions(-)
^ permalink raw reply
* Re: [PATCH v3 1/4] completion: be nicer with zsh
From: Thomas Rast @ 2012-02-02 10:35 UTC (permalink / raw)
To: Felipe Contreras; +Cc: Jonathan Nieder, git, SZEDER Gábor, Junio C Hamano
In-Reply-To: <CAMP44s0w1eXWWaMT3WAfLxFHPQvc9dp33cyJ=T2im6g7rsrKhw@mail.gmail.com>
Felipe Contreras <felipe.contreras@gmail.com> writes:
> On Thu, Feb 2, 2012 at 10:48 AM, Jonathan Nieder <jrnieder@gmail.com> wrote:
>
> Exactly, and "completion: avoid default value assignment on : true
> command" tells *nothing* to most people. Why is this patch needed? Do
> I care about it?
>
> OTOH "completion: be nicer with zsh" explains the purpose of the patch
> and people that don't care about zsh can happily ignore it if they
> want, and the ones that care about zsh might want to back port it, or
> whatever.
Perhaps you could compromise on
completion: work around zsh word splitting bug in : ${foo:=$(bar)}
?
> | tl;dr: $__git_porcelain_commands = $__git_all_commands
>
> Wrapping it up, to make clear what happens.
I think this is not good style for a commit message. Apart from the
very trendy use of tl;dr, it doesn't even properly summarize the cause
*or* the user-visible symptom. It just states how the confusion
propagates somewhere in the middle of the code.
--
Thomas Rast
trast@{inf,student}.ethz.ch
^ permalink raw reply
* Re: General support for ! in git-config values
From: demerphq @ 2012-02-02 10:21 UTC (permalink / raw)
To: Jeff King; +Cc: Junio C Hamano, Ævar Arnfjörð, Git Mailing List
In-Reply-To: <20120202095432.GA19356@sigill.intra.peff.net>
On 2 February 2012 10:54, Jeff King <peff@peff.net> wrote:
> On Thu, Feb 02, 2012 at 10:44:05AM +0100, demerphq wrote:
>
>> The general design of git seems to me to be based around providing
>> building blocks that people can use to build new and interesting tools
>> on top of, and so it seems counter to that philosophy to reject an
>> feature based on speculative security issues that really can't be
>> decided in advance but must instead be decided on a case by case
>> basis.
>
> I can't speak for Junio, but I am certainly not rejecting it. Only
> saying that it needs to be thought through, and the utility weighed
> against the costs.
Of course. I totally understand. I have written mails saying stuff
like this myself. :-)
> So far I haven't seen an actual patch to comment on
> (or even a proposed syntax beyond starting a string with "!", which I
> think is a non-starter due to conflicting with existing uses),
I understand. I think we will probably use backtick quoting in git-deploy. So
deploy.prefix=`cat /etc/SERVER_ROLE`
will execute cat /etc/SERVER_ROLE and use the results as the value of
the config option.
> nor have
> I seen a concrete use case (you mentioned pulling the name/email from
> ldap, but you also mentioned that there are lots of other ways of
> solving that particular problem, so it's not especially compelling).
One place that it would be useful for us in git-deploy would be to
detect the tag prefix for the rollout we are doing. Every staging
server already has a file that contains this value. We would like to
make it easy for people to configure the tool to either use the value
provided, or to use something like `cat /etc/SERVER_ROLE` instead.
Anyway, from that POV I could totally understand "so do that in
git-deploy". Since the tool is written in perl we have to wrap
git-config anyway, so it easy to add a special case for ourselves.
But I still think the general idea is pretty useful, the ldap example
is IMO a cleaner solution than the alternatives, and a variant that I
think is much harder to do currently come to mind right away: setting
the user.email automatically depending on where in your tree a git
repo was located, so that when I work on repo underneath /CPAN/ it
uses my CPAN address, and when I work in my /work/ tree it uses my
$work address, etc, without me having to configure it repo by repo.
(This has bitten more than once in the past)
> I'd be happy to hear a more concrete proposal.
I will be mostly afk the next week so I will leave that to Avar if he
wants to pursue it.
cheers,
Yves
--
perl -Mre=debug -e "/just|another|perl|hacker/"
^ permalink raw reply
* Re: [PATCH v3 1/4] completion: be nicer with zsh
From: Felipe Contreras @ 2012-02-02 10:18 UTC (permalink / raw)
To: Jonathan Nieder; +Cc: git, SZEDER Gábor
In-Reply-To: <20120202094624.GF3823@burratino>
On Thu, Feb 2, 2012 at 11:46 AM, Jonathan Nieder <jrnieder@gmail.com> wrote:
> Felipe Contreras wrote:
>> On Thu, Feb 2, 2012 at 11:10 AM, Jonathan Nieder <jrnieder@gmail.com> wrote:
>
>>> See [1] for details. If there's no obvious explanation, I'd suggest
>>> contacting the postmaster.
>>
>> But there's nothing like the taboo words in the mail:
>> http://vger.kernel.org/majordomo-taboos.txt
>
> Why are you telling me? I am not the postmaster. I can't do anything
> to investigate it or fix it.
I'm not telling you, this is a conversation on a public mailing list.
Other people might know something about, or they might hit the same
issue in the future. It's good to state things on record.
And FTR, I had already contacted the postmaster, which has been not
exactly helpful in previous occasions, but lets see.
--
Felipe Contreras
^ permalink raw reply
* Re: [PATCH v3 1/4] completion: be nicer with zsh
From: Felipe Contreras @ 2012-02-02 10:12 UTC (permalink / raw)
To: Jonathan Nieder; +Cc: git, SZEDER Gábor, Junio C Hamano
In-Reply-To: <20120202084859.GC3823@burratino>
On Thu, Feb 2, 2012 at 10:48 AM, Jonathan Nieder <jrnieder@gmail.com> wrote:
> Felipe Contreras wrote:
>
>> Felipe Contreras (4):
>> completion: be nicer with zsh
>
> Since I can't find this patch in the mail archive, I'll reply here.
> Luckily the most important bit is above already.
>
> I think I mentioned before that this subject line is what will appear
> in the shortlog and the shortlog is all that some people will see of
> the changelog, so it should include a self-contained description of
> the impact of the patch.
Exactly, and "completion: avoid default value assignment on : true
command" tells *nothing* to most people. Why is this patch needed? Do
I care about it?
OTOH "completion: be nicer with zsh" explains the purpose of the patch
and people that don't care about zsh can happily ignore it if they
want, and the ones that care about zsh might want to back port it, or
whatever.
Also, if you are looking at a shortlog and looking for patches related
to zsh, you would easily see this.
"completion: avoid default value assignment on : true command" almost
ensures that the people reading that summary would need to read
further to see *why*.
> -- >8 --
> From: Felipe Contreras <felipe.contreras@gmail.com>
> Date: Thu, 2 Feb 2012 03:15:17 +0200
> Subject: completion: avoid default value assignment on : true command
No useful information provided yet. Should I care about this? I guess
I need to open the mail and see.
> zsh versions from 4.3.0 to present (4.3.15) do not correctly propagate
> the SH_WORD_SPLIT option into the subshell in ${foo:=$(bar)}
> expressions. For example, after running
Ok, a bug in zsh, I can skip this. Or if I care about zsh, then I
still have to read further, because I still don't know what's the
problem.
And BTW, this is extremely detailed. Where's the evidence? Is there a
bug report? How can I follow this to find out when it's fixed. (hint:
missing link)
> emulate sh
> fn () {
> var='one two'
> printf '%s\n' $var
> }
> x=$(fn)
> : ${y=$(fn)}
>
> printing "$x" results in two lines as expected, but printing "$y"
> results in a single line because $var is expanded as a single word
> when evaluating fn to compute y.
Ok, still reading.
> So avoid the construct, and use an explicit 'test -n "$foo" ||
> foo=$(bar)' instead.
Yeah...
> This fixes a bug tht caused all commands to be
> treated as porcelain and show up in "git <TAB><TAB>" completion,
> because the list of all commands was treated as a single word in
> __git_list_porcelain_commands and did not match any of the patterns
> that would usually cause plumbing to be excluded.
Aha! Finally we arrive to the important part.
I guess we have different styles of writing. Personally, I don't see
the point of forcing the readers to go through the whole thing.
Compare this to mine:
| Subject: [PATCH v3 1/4] completion: be nicer with zsh
At this point most people can skip this.
| And yet another bug in zsh[1] causes a mismatch; zsh seems to have
| problem emulating wordspliting, but only on the command substitution.
|
| Let's avoid it.
A bug in zsh in certain situations... got it.
| I found this issue because __git_compute_porcelain_commands listed all
| commands (not only porcelain).
At this point it might have been better to mention "git <TAB><TAB>"
instead, as you did. But not a big deal, the problem is clear; all
commands are listed.
Almost all people would stop at this point; either they don't care
about zsh, or they care, and they want the patch.
| This is because in zsh the following code:
|
| for i in $__git_all_commands
|
| would evaluate $__git_all_commands as a single word (with spaces),
| ${=__git_all_commands} should be used to do word splitting expansion
| (unless SH_WORD_SPLIT is used).
|
| sh emulation should take care of that, but the command expantion is
| messing up with that.
And more details for the skeptics, or the ones that want to learn more.
| tl;dr: $__git_porcelain_commands = $__git_all_commands
Wrapping it up, to make clear what happens.
| [1] http://article.gmane.org/gmane.comp.shells.zsh.devel/24296
And then a link to further details, discussion, and possible fixes.
Cheers.
--
Felipe Contreras
^ permalink raw reply
* Re: How to find and analyze bad merges?
From: norbert.nemec @ 2012-02-02 10:05 UTC (permalink / raw)
To: git
In-Reply-To: <jgdgcv$h8n$1@dough.gmane.org>
Thinking about a possible solution:
Is there a way to re-do a merge-commit and diff the result against the
recorded merge without touching the working tree? This would be the
killer-feature to analyze a recorded merge-commit.
Am 02.02.12 09:10, schrieb norbert.nemec:
> Hi there,
>
> a colleague of mine happened to produce a bad merge by unintenionally
> picking the version of the remote branch ("R") for all conflicting
> files. Effectively, he eliminated a whole bunch of bugfixes that were
> already on his local branch ("L").
>
> Obviously this was a mistake on his side, but hey: everyone makes
> mistakes. The real problem is to find this problem afterwards, possibly
> weeks later, when you suddenly realize that a bug that you had fixed
> suddenly reappears.
>
> A "git log" on the whole repository shows both branches R and L.
> A "git show" on the bugfix commit shows the bugfix as you expect it.
>
> BUT:
> A "git log" on the file itself shows neither the problematic merge nor
> the bugfix commit. Git considers the merge of this file trivial because
> the content is identical to that of parent R. Therefore, whatever
> happened on branch L is not considered relevant history of the file.
>
> FURTHERMORE:
> A "git show" of the merge itself does not show the conflicting file
> either. Obviously, "git show" on a merge decides which files are
> relevant not based on conflicts but based on resolutions.
>
> To sort out what happened, you first need to have a suspicion and then
> dig fairly deep in the manuals to set the correct options to show what
> happened.
>
> I think, both "git log" and "git show" should by default be a bit more
> conservative in hiding "insignificant" merges:
> * In "git log" a branch should only be hidden if it never touched the file.
> * In "git show" a merge should display all files that did have a
> conflict independent of the resolution. (I am open to discuss whether
> auto-resolvable conflicts should be displayed by default. Non-trivial
> conflicts definitely should)
>
> Greetings,
> Norbert
>
^ permalink raw reply
* Re: [PATCH] i18n: po for zh_cn
From: Frederik Schwarzer @ 2012-02-02 10:04 UTC (permalink / raw)
To: git
Am Donnerstag, 2. Februar 2012, 09:15:38 schrieben Sie:
(damn, my mail programme always answers to the sender only ...)
Hi,
> Jiang Xin <worldhello.net@gmail.com> writes:
> > 2012/2/2 Junio C Hamano <gitster@pobox.com>:
> > ...
> >
> >> (6) From time to time, the l10n coordinator will pull from
> >> "git.git" when
> >>
> >> meaningful number of changes are made to the translatable
> >> strings. I hope this would happen much less often than
> >> once per week, preferably much less frequently. The l10n
> >> coordinator updates po/git.pot and makes a commit, and
> >> notifies the l10n teams.
> >
> > notifies the l10n teams using another mailing list maybe.
>
> I personally think using the regular git@vger.kernel.org list would
> be preferrable for this. People who translate would want to learn
> the reasoning that led to the final phrasing of the messages to
> come up with usable translation, and following the main list would
> be one way to do so. Having to follow two separate list will be an
> unnecessary burden.
>
> But the choice of how to coordinate his or her work with the l10n
> teams is entirely up to the l10n coordinator.
As I see it there are two kinds of people doing translations.
Developers who also translate and people who want to translate
software but are not involved in the development.
Translations of the former group are in many cases suboptimal. But the
latter group will not follow this mailing list. They are not
interested in all the details. They live in their room, get a piece of
paper and translate it. In case something is unclear, they aks. But as
it is now, it would look something like this:
Translators receive 100+ emails per day and are supposed to filter out
the one email every 10 days that carries useful information for their
work. In practice I guess interested Translators (who are not
interested in every code detail) will unsubscribe after a few days and
then miss all the fun.
A git-i18n mailing list could coordinate that. It would not be a list
for l10n teams to do their internal coordination, but for the i18n
coordinator to notify l10n teams about updated POT files (he might
even merge PO files) and for l10n teams to ask about strings they are
unsure about. These questions would then be digested by the i18n
coordinator and brought to the attention of the developers if needed.
How does that sound?
Regards
PS: I would have even put all the POT/PO stuff in an extra Git
repository. Working with translation is decoupled from development
cleanly by the msginit/msgmerge process but well, it might not be woth
that separation for a single project. :)
^ permalink raw reply
* Re: General support for ! in git-config values
From: Jeff King @ 2012-02-02 9:54 UTC (permalink / raw)
To: demerphq; +Cc: Junio C Hamano, Ævar Arnfjörð, Git Mailing List
In-Reply-To: <CANgJU+X2dRP__PFAywGEisDS3xyF7fSszSQG6BO61j2TMKL3Qg@mail.gmail.com>
On Thu, Feb 02, 2012 at 10:44:05AM +0100, demerphq wrote:
> The general design of git seems to me to be based around providing
> building blocks that people can use to build new and interesting tools
> on top of, and so it seems counter to that philosophy to reject an
> feature based on speculative security issues that really can't be
> decided in advance but must instead be decided on a case by case
> basis.
I can't speak for Junio, but I am certainly not rejecting it. Only
saying that it needs to be thought through, and the utility weighed
against the costs. So far I haven't seen an actual patch to comment on
(or even a proposed syntax beyond starting a string with "!", which I
think is a non-starter due to conflicting with existing uses), nor have
I seen a concrete use case (you mentioned pulling the name/email from
ldap, but you also mentioned that there are lots of other ways of
solving that particular problem, so it's not especially compelling).
I'd be happy to hear a more concrete proposal.
-Peff
^ permalink raw reply
* Re: [PATCH v3 1/4] completion: be nicer with zsh
From: Jonathan Nieder @ 2012-02-02 9:46 UTC (permalink / raw)
To: Felipe Contreras; +Cc: git, SZEDER Gábor
In-Reply-To: <CAMP44s0nD4p9=fwpLwchmGJ123onLmRaSPmOL+cYpTFCJ-jwXw@mail.gmail.com>
Felipe Contreras wrote:
> On Thu, Feb 2, 2012 at 11:10 AM, Jonathan Nieder <jrnieder@gmail.com> wrote:
>> See [1] for details. If there's no obvious explanation, I'd suggest
>> contacting the postmaster.
>
> But there's nothing like the taboo words in the mail:
> http://vger.kernel.org/majordomo-taboos.txt
Why are you telling me? I am not the postmaster. I can't do anything
to investigate it or fix it.
^ permalink raw reply
* Re: General support for ! in git-config values
From: demerphq @ 2012-02-02 9:44 UTC (permalink / raw)
To: Jeff King; +Cc: Junio C Hamano, Ævar Arnfjörð, Git Mailing List
In-Reply-To: <20120202023857.GA11745@sigill.intra.peff.net>
On 2 February 2012 03:38, Jeff King <peff@peff.net> wrote:
> On Thu, Feb 02, 2012 at 02:57:14AM +0100, demerphq wrote:
>
>> > Not really.
>> >
>> > I do not think whatever "utility" value outweighs the hassle of having to
>> > think through the ramifications (including but not limited to security) of
>> > running arbitrary user command every time a value is looked up.
>>
>> Why is that your problem? If I have to enable it then isn't that my choice?
>
> From a security perspective, you want to make sure that people who
> aren't interested in your feature don't accidentally trigger it. E.g.,
> imagine I currently run a locked-down git repo but execute some commands
> on your behalf, and I allow you to set a few "known safe" config options
> like user.email. Even though I am not interested in your feature,
> respecting "!rm -rf /" in the user.email you give me would be a bad
> thing.
Like I said, I do not think this should be enabled by default, I think
it should be possible to enable it config wide. So unless this
scenario involves getting the owner of the locked down repo to enable
a config option they know nothing about, in which case I would say
there are easier attacks -- someone that stupid probably could be
talked into telling you their root password. :-)
> It's not an insurmountable problem. There could be options to turn it
> on, or turn it off, or whatever.
My thought exactly. Anyone paranoid about security would never enable
this feature. Those who are comfortable with the security issues
could.
> Or we could shrug and say that config
> is already dangerous to let other people set (which it is already, but
> only for some options).
I think that since it could be set up to be determined by the user,
that it would be no more dangerous than any other option.
> But those are the sorts of ramifications that
> need to be thought through.
I understand that. All I can say is $work uses git on a pretty large
scale, 100+ devs etc, and we use it to manage our rollout processes
which we use a lot (I cant say how often but a lot). So if it would be
useful to us it probably would be useful to others.
> (Another one is that with our current strategy, we actually read and
> parse the config files multiple times. Should your program get run many
> times?).
Again I would say this is not git's problem. If it should not be run
multiple times it is up to the user to figure out an alternative.
The general design of git seems to me to be based around providing
building blocks that people can use to build new and interesting tools
on top of, and so it seems counter to that philosophy to reject an
feature based on speculative security issues that really can't be
decided in advance but must instead be decided on a case by case
basis.
cheers,
Yves
--
perl -Mre=debug -e "/just|another|perl|hacker/"
^ permalink raw reply
* Re: [PATCH v3 1/4] completion: be nicer with zsh
From: Felipe Contreras @ 2012-02-02 9:38 UTC (permalink / raw)
To: Jonathan Nieder; +Cc: git, SZEDER Gábor
In-Reply-To: <20120202091059.GE3823@burratino>
On Thu, Feb 2, 2012 at 11:10 AM, Jonathan Nieder <jrnieder@gmail.com> wrote:
> (dropping Shawn from cc list, since I don't think he's touched the
> completion code for years)
> Felipe Contreras wrote:
>> On Thu, Feb 2, 2012 at 10:16 AM, Jonathan Nieder <jrnieder@gmail.com> wrote:
>
>>> Patches didn't hit the list again. Any idea why?
>>
>> No. A bug in list software?
>>
>> I didn't get any warning or error.
>
> Except in response to HTML attachments, I've never seen vger return any
> explanation when it decides a message is spam. It just discards them.
Which is wrong, because when it discards them wrongly nobody knows why.
> See [1] for details. If there's no obvious explanation, I'd suggest
> contacting the postmaster.
But there's nothing like the taboo words in the mail:
http://vger.kernel.org/majordomo-taboos.txt
--
Felipe Contreras
^ permalink raw reply
* Re: [PATCH v3 1/4] completion: be nicer with zsh
From: Jonathan Nieder @ 2012-02-02 9:10 UTC (permalink / raw)
To: Felipe Contreras; +Cc: git, SZEDER Gábor
In-Reply-To: <CAMP44s3FxUmnpQevoV2ARJpWK9CJ16zXDmpJRDOLHNW6RdSc5Q@mail.gmail.com>
(dropping Shawn from cc list, since I don't think he's touched the
completion code for years)
Felipe Contreras wrote:
> On Thu, Feb 2, 2012 at 10:16 AM, Jonathan Nieder <jrnieder@gmail.com> wrote:
>> Patches didn't hit the list again. Any idea why?
>
> No. A bug in list software?
>
> I didn't get any warning or error.
Except in response to HTML attachments, I've never seen vger return any
explanation when it decides a message is spam. It just discards them.
See [1] for details. If there's no obvious explanation, I'd suggest
contacting the postmaster.
Hope that helps, and sorry for the fuss,
Jonathan
[1] http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH v3 2/4] completion: simplify __git_remotes
From: Jonathan Nieder @ 2012-02-02 9:05 UTC (permalink / raw)
To: Felipe Contreras; +Cc: git, SZEDER Gábor, Junio C Hamano
In-Reply-To: <1328145320-14071-1-git-send-email-felipe.contreras@gmail.com>
Felipe Contreras wrote:
> completion: simplify __git_remotes
> completion: remove unused code
In the same spirit of stealing potential shortlog lines from Junio:
-- >8 --
From: Felipe Contreras <felipe.contreras@gmail.com>
Subject: completion: use ls -1 instead of rolling a loop to do that ourselves
This simplifies the code a great deal. In particular, it allows us to
get rid of __git_shopt, which is used only in this fuction to enable
'nullglob' in zsh.
[jn: squashed with a patch that actually gets rid of __git_shopt]
Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
The diffstat says it all.
contrib/completion/git-completion.bash | 37 +-------------------------------
1 files changed, 1 insertions(+), 36 deletions(-)
diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 78be1958..4612dde9 100755
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -644,12 +644,7 @@ __git_refs_remotes ()
__git_remotes ()
{
local i ngoff IFS=$'\n' d="$(__gitdir)"
- __git_shopt -q nullglob || ngoff=1
- __git_shopt -s nullglob
- for i in "$d/remotes"/*; do
- echo ${i#$d/remotes/}
- done
- [ "$ngoff" ] && __git_shopt -u nullglob
+ test -d "$d/remotes" && ls -1 "$d/remotes"
for i in $(git --git-dir="$d" config --get-regexp 'remote\..*\.url' 2>/dev/null); do
i="${i#remote.}"
echo "${i/.url*/}"
@@ -2733,33 +2728,3 @@ if [ Cygwin = "$(uname -o 2>/dev/null)" ]; then
complete -o bashdefault -o default -o nospace -F _git git.exe 2>/dev/null \
|| complete -o default -o nospace -F _git git.exe
fi
-
-if [[ -n ${ZSH_VERSION-} ]]; then
- __git_shopt () {
- local option
- if [ $# -ne 2 ]; then
- echo "USAGE: $0 (-q|-s|-u) <option>" >&2
- return 1
- fi
- case "$2" in
- nullglob)
- option="$2"
- ;;
- *)
- echo "$0: invalid option: $2" >&2
- return 1
- esac
- case "$1" in
- -q) setopt | grep -q "$option" ;;
- -u) unsetopt "$option" ;;
- -s) setopt "$option" ;;
- *)
- echo "$0: invalid flag: $1" >&2
- return 1
- esac
- }
-else
- __git_shopt () {
- shopt "$@"
- }
-fi
--
1.7.9
^ permalink raw reply related
* Re: How to find and analyze bad merges?
From: norbert.nemec @ 2012-02-02 9:01 UTC (permalink / raw)
To: git
In-Reply-To: <7vd39xy7it.fsf@alter.siamese.dyndns.org>
Am 02.02.12 09:16, schrieb Junio C Hamano:
> "norbert.nemec"<norbert.nemec@native-instruments.de> writes:
>
>> a colleague of mine happened to produce a bad merge by unintenionally
>> picking the version of the remote branch ("R") for all conflicting
>> files. Effectively, he eliminated a whole bunch of bugfixes that were
>> already on his local branch ("L").
>>
>> Obviously this was a mistake on his side, but hey: everyone makes
>> mistakes. The real problem is to find this problem afterwards,
>> possibly weeks later, when you suddenly realize that a bug that you
>> had fixed suddenly reappears.
>
> Bisect?
This is not the point: My colleague knew exactly which commit contained
the bugfix. The trouble was finding out why this bugfix disappeared even
though everything indicated that it was cleanly merged into the current
branch.
^ permalink raw reply
* Re: [PATCH v3 1/4] completion: be nicer with zsh
From: Jonathan Nieder @ 2012-02-02 8:48 UTC (permalink / raw)
To: Felipe Contreras; +Cc: git, SZEDER Gábor, Junio C Hamano
In-Reply-To: <1328145320-14071-1-git-send-email-felipe.contreras@gmail.com>
Hi,
Felipe Contreras wrote:
> Felipe Contreras (4):
> completion: be nicer with zsh
Since I can't find this patch in the mail archive, I'll reply here.
Luckily the most important bit is above already.
I think I mentioned before that this subject line is what will appear
in the shortlog and the shortlog is all that some people will see of
the changelog, so it should include a self-contained description of
the impact of the patch.
However, clearly I did not say it clearly enough. :) I guess it's
better to take a cue from storytellers and show rather than tell.
(Please don't take this as a precedent --- I will not always be doing
the style fixes myself, and sometimes will consider a patch to scratch
someone else's itch not worth the trouble and work on something else.)
-- >8 --
From: Felipe Contreras <felipe.contreras@gmail.com>
Date: Thu, 2 Feb 2012 03:15:17 +0200
Subject: completion: avoid default value assignment on : true command
zsh versions from 4.3.0 to present (4.3.15) do not correctly propagate
the SH_WORD_SPLIT option into the subshell in ${foo:=$(bar)}
expressions. For example, after running
emulate sh
fn () {
var='one two'
printf '%s\n' $var
}
x=$(fn)
: ${y=$(fn)}
printing "$x" results in two lines as expected, but printing "$y"
results in a single line because $var is expanded as a single word
when evaluating fn to compute y.
So avoid the construct, and use an explicit 'test -n "$foo" ||
foo=$(bar)' instead. This fixes a bug tht caused all commands to be
treated as porcelain and show up in "git <TAB><TAB>" completion,
because the list of all commands was treated as a single word in
__git_list_porcelain_commands and did not match any of the patterns
that would usually cause plumbing to be excluded.
[jn: clarified commit message, indentation style fix]
Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
contrib/completion/git-completion.bash | 9 ++++++---
1 files changed, 6 insertions(+), 3 deletions(-)
diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 78be1958..d7965daf 100755
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -676,7 +676,8 @@ __git_merge_strategies=
# is needed.
__git_compute_merge_strategies ()
{
- : ${__git_merge_strategies:=$(__git_list_merge_strategies)}
+ [ -n "$__git_merge_strategies" ] ||
+ __git_merge_strategies=$(__git_list_merge_strategies)
}
__git_complete_revlist_file ()
@@ -854,7 +855,8 @@ __git_list_all_commands ()
__git_all_commands=
__git_compute_all_commands ()
{
- : ${__git_all_commands:=$(__git_list_all_commands)}
+ [ -n "$__git_all_commands" ] ||
+ __git_all_commands=$(__git_list_all_commands)
}
__git_list_porcelain_commands ()
@@ -947,7 +949,8 @@ __git_porcelain_commands=
__git_compute_porcelain_commands ()
{
__git_compute_all_commands
- : ${__git_porcelain_commands:=$(__git_list_porcelain_commands)}
+ [ -n "$__git_porcelain_commands" ] ||
+ __git_porcelain_commands=$(__git_list_porcelain_commands)
}
__git_pretty_aliases ()
--
1.7.9
^ permalink raw reply related
* Re: [PATCH v3 1/4] completion: be nicer with zsh
From: Felipe Contreras @ 2012-02-02 8:34 UTC (permalink / raw)
To: Jonathan Nieder; +Cc: git, SZEDER Gábor, Shawn O. Pearce
In-Reply-To: <20120202081622.GB3823@burratino>
On Thu, Feb 2, 2012 at 10:16 AM, Jonathan Nieder <jrnieder@gmail.com> wrote:
> Felipe Contreras wrote:
>
>> And yet another bug in zsh[1] causes a mismatch; zsh seems to have
>> problem emulating wordspliting, but only on the command substitution.
>
> Patches didn't hit the list again. Any idea why?
No. A bug in list software?
I didn't get any warning or error.
--
Felipe Contreras
^ 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