* Re: segfault for git log --graph --no-walk --grep a
From: Jeff King @ 2013-02-09 0:27 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Thomas Haller, Git List
In-Reply-To: <7vobfuxrns.fsf@alter.siamese.dyndns.org>
On Fri, Feb 08, 2013 at 04:22:15PM -0800, Junio C Hamano wrote:
> Junio C Hamano <gitster@pobox.com> writes:
>
> > Thomas Haller <thom311@gmail.com> writes:
> >
> >> it happens in file revision.c:2306 because "commit->buffer" is zero:
> >>
> >> retval = grep_buffer(&opt->grep_filter,
> >> commit->buffer, strlen(commit->buffer));
> >
> > I think this has been fixed at be5c9fb9049e (logmsg_reencode: lazily
> > load missing commit buffers, 2013-01-26); I haven't merged it to any
> > of the maintenance releases, but the tip of 'master' should have it
> > already.
>
> Ah, no, this shares the same roots as the breakage the said commit
> fixed, and the solution should use the same idea, but not fixed.
Yeah, I think this needs separate treatment. However, this is a perfect
example of the "case-by-case" I mention in the final two paragraphs
there.
What's the right encoding to be grepping in? The original, what we will
output in, or even something else? IOW, I wonder if this should be using
logmsg_reencode in the first place (the obvious reason not to want to do
so is speed, but logmsg_reencode is written to only have an impact in
the case that we do indeed need to reencode).
-Peff
^ permalink raw reply
* Re: segfault for git log --graph --no-walk --grep a
From: Junio C Hamano @ 2013-02-09 0:29 UTC (permalink / raw)
To: Thomas Haller; +Cc: Git List, Jeff King
In-Reply-To: <7vobfuxrns.fsf@alter.siamese.dyndns.org>
Junio C Hamano <gitster@pobox.com> writes:
> Junio C Hamano <gitster@pobox.com> writes:
>
>> Thomas Haller <thom311@gmail.com> writes:
>>
>>> it happens in file revision.c:2306 because "commit->buffer" is zero:
>>>
>>> retval = grep_buffer(&opt->grep_filter,
>>> commit->buffer, strlen(commit->buffer));
>>
>> I think this has been fixed at be5c9fb9049e (logmsg_reencode: lazily
>> load missing commit buffers, 2013-01-26); I haven't merged it to any
>> of the maintenance releases, but the tip of 'master' should have it
>> already.
>
> Ah, no, this shares the same roots as the breakage the said commit
> fixed, and the solution should use the same idea, but not fixed.
Perhaps something along this line...
-- >8 --
Subject: "log --grep": commit's buffer may already have been discarded
Following up on be5c9fb9049e (logmsg_reencode: lazily load missing
commit buffers, 2013-01-26), extract the part that reads the commit
buffer data into a separate helper function, and use it when we
apply the grep filter on the commit during the log walk.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
The reproduction recipe in original bug report looked like this:
git commit -m 'text1' --allow-empty
git commit -m 'text2' --allow-empty
git log --graph --no-walk --grep 'text2'
which does not make any sense to me. We should simply forbid
combination of --graph (which inherently wants a connected history)
and --no-walk (which is a way to tell "This is not about history,
this is about a single point").
But that is a separate issue.
commit.c | 19 +++++++++++++++++++
commit.h | 1 +
pretty.c | 14 ++------------
revision.c | 14 +++++++++++---
4 files changed, 33 insertions(+), 15 deletions(-)
diff --git a/commit.c b/commit.c
index e8eb0ae..c0acf0f 100644
--- a/commit.c
+++ b/commit.c
@@ -335,6 +335,25 @@ int parse_commit(struct commit *item)
return ret;
}
+char *read_commit_object_data(const struct commit *commit, unsigned long *sizep)
+{
+ char *msg;
+ enum object_type type;
+ unsigned long size;
+
+ if (!sizep)
+ sizep = &size;
+
+ msg = read_sha1_file(commit->object.sha1, &type, sizep);
+ if (!msg)
+ die("Cannot read commit object %s",
+ sha1_to_hex(commit->object.sha1));
+ if (type != OBJ_COMMIT)
+ die("Expected commit for '%s', got %s",
+ sha1_to_hex(commit->object.sha1), typename(type));
+ return msg;
+}
+
int find_commit_subject(const char *commit_buffer, const char **subject)
{
const char *eol;
diff --git a/commit.h b/commit.h
index 4138bb4..e314149 100644
--- a/commit.h
+++ b/commit.h
@@ -102,6 +102,7 @@ struct rev_info; /* in revision.h, it circularly uses enum cmit_fmt */
extern char *logmsg_reencode(const struct commit *commit,
const char *output_encoding);
extern void logmsg_free(char *msg, const struct commit *commit);
+extern char *read_commit_object_data(const struct commit *commit, unsigned long *size);
extern void get_commit_format(const char *arg, struct rev_info *);
extern const char *format_subject(struct strbuf *sb, const char *msg,
const char *line_separator);
diff --git a/pretty.c b/pretty.c
index eae57ad..004d16d 100644
--- a/pretty.c
+++ b/pretty.c
@@ -592,18 +592,8 @@ char *logmsg_reencode(const struct commit *commit,
char *msg = commit->buffer;
char *out;
- if (!msg) {
- enum object_type type;
- unsigned long size;
-
- msg = read_sha1_file(commit->object.sha1, &type, &size);
- if (!msg)
- die("Cannot read commit object %s",
- sha1_to_hex(commit->object.sha1));
- if (type != OBJ_COMMIT)
- die("Expected commit for '%s', got %s",
- sha1_to_hex(commit->object.sha1), typename(type));
- }
+ if (!msg)
+ msg = read_commit_object_data(commit, NULL);
if (!output_encoding || !*output_encoding)
return msg;
diff --git a/revision.c b/revision.c
index d7562ee..caf8ef3 100644
--- a/revision.c
+++ b/revision.c
@@ -2279,9 +2279,16 @@ static int commit_match(struct commit *commit, struct rev_info *opt)
strbuf_addch(&buf, '\n');
}
- /* Copy the commit to temporary if we are using "fake" headers */
- if (buf.len)
+ if (!commit->buffer) {
+ /* we may not have commit->buffer */
+ unsigned long size;
+ char *msg = read_commit_object_data(commit, &size);
+ strbuf_add(&buf, msg, size);
+ free(msg);
+ } else if (buf.len) {
+ /* Copy the commit to temporary if we are using "fake" headers */
strbuf_addstr(&buf, commit->buffer);
+ }
if (opt->grep_filter.header_list && opt->mailmap) {
if (!buf.len)
@@ -2302,9 +2309,10 @@ static int commit_match(struct commit *commit, struct rev_info *opt)
/* Find either in the commit object, or in the temporary */
if (buf.len)
retval = grep_buffer(&opt->grep_filter, buf.buf, buf.len);
- else
+ else {
retval = grep_buffer(&opt->grep_filter,
commit->buffer, strlen(commit->buffer));
+ }
strbuf_release(&buf);
return retval;
}
^ permalink raw reply related
* Re: segfault for git log --graph --no-walk --grep a
From: Junio C Hamano @ 2013-02-09 0:39 UTC (permalink / raw)
To: Jeff King; +Cc: Thomas Haller, Git List
In-Reply-To: <20130209002710.GA5570@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> On Fri, Feb 08, 2013 at 04:22:15PM -0800, Junio C Hamano wrote:
>
>> Junio C Hamano <gitster@pobox.com> writes:
>>
>> > Thomas Haller <thom311@gmail.com> writes:
>> >
>> >> it happens in file revision.c:2306 because "commit->buffer" is zero:
>> >>
>> >> retval = grep_buffer(&opt->grep_filter,
>> >> commit->buffer, strlen(commit->buffer));
>> >
>> > I think this has been fixed at be5c9fb9049e (logmsg_reencode: lazily
>> > load missing commit buffers, 2013-01-26); I haven't merged it to any
>> > of the maintenance releases, but the tip of 'master' should have it
>> > already.
>>
>> Ah, no, this shares the same roots as the breakage the said commit
>> fixed, and the solution should use the same idea, but not fixed.
>
> Yeah, I think this needs separate treatment. However, this is a perfect
> example of the "case-by-case" I mention in the final two paragraphs
> there.
>
> What's the right encoding to be grepping in? The original, what we will
> output in, or even something else? IOW, I wonder if this should be using
> logmsg_reencode in the first place (the obvious reason not to want to do
> so is speed, but logmsg_reencode is written to only have an impact in
> the case that we do indeed need to reencode).
Yeah, that actually is a good point. We should be using logmsg_reencode
so that we look for strings in the user's encoding.
^ permalink raw reply
* Re: segfault for git log --graph --no-walk --grep a
From: Jeff King @ 2013-02-09 0:39 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Thomas Haller, Git List
In-Reply-To: <7vk3qixrc8.fsf@alter.siamese.dyndns.org>
On Fri, Feb 08, 2013 at 04:29:11PM -0800, Junio C Hamano wrote:
> Perhaps something along this line...
>
> -- >8 --
> Subject: "log --grep": commit's buffer may already have been discarded
>
> Following up on be5c9fb9049e (logmsg_reencode: lazily load missing
> commit buffers, 2013-01-26), extract the part that reads the commit
> buffer data into a separate helper function, and use it when we
> apply the grep filter on the commit during the log walk.
This obviously makes sense if we don't want to get the route of
re-encoding for grep. Re-encoding would be a user-visible change, but I
wonder if it is the right thing to be doing.
> diff --git a/revision.c b/revision.c
> index d7562ee..caf8ef3 100644
> --- a/revision.c
> +++ b/revision.c
> @@ -2279,9 +2279,16 @@ static int commit_match(struct commit *commit, struct rev_info *opt)
> strbuf_addch(&buf, '\n');
> }
>
> - /* Copy the commit to temporary if we are using "fake" headers */
> - if (buf.len)
> + if (!commit->buffer) {
> + /* we may not have commit->buffer */
> + unsigned long size;
> + char *msg = read_commit_object_data(commit, &size);
> + strbuf_add(&buf, msg, size);
> + free(msg);
> + } else if (buf.len) {
> + /* Copy the commit to temporary if we are using "fake" headers */
> strbuf_addstr(&buf, commit->buffer);
> + }
Hmm. It would be nice to avoid the extra copy when we do not otherwise
need to use the strbuf. I would have expected something more like:
const char *msg = commit->buffer;
if (!msg)
msg = read_commit_object_data(commit, NULL);
[...]
if (buf.len)
retval = grep_buffer(&opt->grep_filter, buf.buf, buf.len);
else
retval = grep_buffer(&opt->grep_filter, msg, strlen(msg));
strbuf_release(&buf);
if (msg != commit->buffer)
free(msg);
return retval;
You would also need to adjust the other uses of commit->buffer
throughout the function to refer to msg.
-Peff
^ permalink raw reply
* Permission denied on home dir results in fatal error as of 1.8.1.1
From: Nick Muerdter @ 2013-02-09 0:40 UTC (permalink / raw)
To: git
As of git 1.8.1.1 and above (tested up to 1.8.1.3), if the home
directory can't be accessed, it results in a fatal error. In git 1.8.1
and below this same setup just resulted in warnings. Was this an
intentional change?
I ran into this in a situation where sudo is being used to execute a
script as root and then git ends up getting executed as a different
user. In this case HOME ends up being /root when git is called as this
different user. This home value is obviously incorrect, so there's an
issue there, but it would be perhaps be nice if this still just
resulted in warnings.
Here's a simple way to reproduce if run as a non-root user:
git 1.8.1.1 (fatal error, doesn't work):
$ env HOME=/root git ls-remote http://github.com/sstephenson/rbenv.git v0.4.0
fatal: unable to access '/root/.config/git/config': Permission denied
git 1.8.1 (warnings, but still works):
$ env HOME=/root git ls-remote http://github.com/sstephenson/rbenv.git v0.4.0
warning: unable to access '/root/.config/git/config': Permission denied
warning: unable to access '/root/.gitconfig': Permission denied
warning: unable to access '/root/.config/git/config': Permission denied
warning: unable to access '/root/.gitconfig': Permission denied
warning: unable to access '/root/.config/git/config': Permission denied
warning: unable to access '/root/.gitconfig': Permission denied
warning: unable to access '/root/.config/git/config': Permission denied
warning: unable to access '/root/.gitconfig': Permission denied
9375e99f921f428849f19efe2a2e500b3295d1a7 refs/tags/v0.4.0
Thanks,
Nick
^ permalink raw reply
* Re: segfault for git log --graph --no-walk --grep a
From: Junio C Hamano @ 2013-02-09 0:47 UTC (permalink / raw)
To: Jeff King; +Cc: Thomas Haller, Git List
In-Reply-To: <7vfw16xqvj.fsf@alter.siamese.dyndns.org>
Junio C Hamano <gitster@pobox.com> writes:
> Jeff King <peff@peff.net> writes:
>
>> On Fri, Feb 08, 2013 at 04:22:15PM -0800, Junio C Hamano wrote:
>>
>>> Junio C Hamano <gitster@pobox.com> writes:
>>>
>>> > Thomas Haller <thom311@gmail.com> writes:
>>> >
>>> >> it happens in file revision.c:2306 because "commit->buffer" is zero:
>>> >>
>>> >> retval = grep_buffer(&opt->grep_filter,
>>> >> commit->buffer, strlen(commit->buffer));
>>> >
>>> > I think this has been fixed at be5c9fb9049e (logmsg_reencode: lazily
>>> > load missing commit buffers, 2013-01-26); I haven't merged it to any
>>> > of the maintenance releases, but the tip of 'master' should have it
>>> > already.
>>>
>>> Ah, no, this shares the same roots as the breakage the said commit
>>> fixed, and the solution should use the same idea, but not fixed.
>>
>> Yeah, I think this needs separate treatment. However, this is a perfect
>> example of the "case-by-case" I mention in the final two paragraphs
>> there.
>>
>> What's the right encoding to be grepping in? The original, what we will
>> output in, or even something else? IOW, I wonder if this should be using
>> logmsg_reencode in the first place (the obvious reason not to want to do
>> so is speed, but logmsg_reencode is written to only have an impact in
>> the case that we do indeed need to reencode).
>
> Yeah, that actually is a good point. We should be using logmsg_reencode
> so that we look for strings in the user's encoding.
Perhaps like this. Just like the previous one (which should be
discarded), this makes the function always use the temporary strbuf,
so doing this upfront actually loses more code than it adds ;-)
revision.c | 29 ++++++++++++-----------------
1 file changed, 12 insertions(+), 17 deletions(-)
diff --git a/revision.c b/revision.c
index d7562ee..07bf728 100644
--- a/revision.c
+++ b/revision.c
@@ -2269,6 +2269,9 @@ static int commit_match(struct commit *commit, struct rev_info *opt)
{
int retval;
struct strbuf buf = STRBUF_INIT;
+ char *message;
+ const char *encoding;
+
if (!opt->grep_filter.pattern_list && !opt->grep_filter.header_list)
return 1;
@@ -2279,32 +2282,24 @@ static int commit_match(struct commit *commit, struct rev_info *opt)
strbuf_addch(&buf, '\n');
}
- /* Copy the commit to temporary if we are using "fake" headers */
- if (buf.len)
- strbuf_addstr(&buf, commit->buffer);
+ encoding = get_log_output_encoding();
+ message = logmsg_reencode(commit, encoding);
+ strbuf_addstr(&buf, message);
+ if (message != commit->buffer)
+ free(message);
if (opt->grep_filter.header_list && opt->mailmap) {
- if (!buf.len)
- strbuf_addstr(&buf, commit->buffer);
-
commit_rewrite_person(&buf, "\nauthor ", opt->mailmap);
commit_rewrite_person(&buf, "\ncommitter ", opt->mailmap);
}
/* Append "fake" message parts as needed */
- if (opt->show_notes) {
- if (!buf.len)
- strbuf_addstr(&buf, commit->buffer);
+ if (opt->show_notes)
format_display_notes(commit->object.sha1, &buf,
- get_log_output_encoding(), 1);
- }
+ encoding, 1);
+
+ retval = grep_buffer(&opt->grep_filter, buf.buf, buf.len);
- /* Find either in the commit object, or in the temporary */
- if (buf.len)
- retval = grep_buffer(&opt->grep_filter, buf.buf, buf.len);
- else
- retval = grep_buffer(&opt->grep_filter,
- commit->buffer, strlen(commit->buffer));
strbuf_release(&buf);
return retval;
}
^ permalink raw reply related
* Re: Permission denied on home dir results in fatal error as of 1.8.1.1
From: Junio C Hamano @ 2013-02-09 0:50 UTC (permalink / raw)
To: Nick Muerdter; +Cc: git
In-Reply-To: <CAECnihxpvtE1XejzHDCRBF=GkyBHmb53WDLa16Suiq=4SeYzvA@mail.gmail.com>
Nick Muerdter <stuff@nickm.org> writes:
> As of git 1.8.1.1 and above (tested up to 1.8.1.3), if the home
> directory can't be accessed, it results in a fatal error. In git 1.8.1
> and below this same setup just resulted in warnings. Was this an
> intentional change?
I think this was done to not just help diagnosing misconfiguration,
but to prevent an unintended misconfiguration from causing problems
(e.g. the user thinks user.name is set up correctly, but forbids Git
from reading it from the configuration files, and ends up creating
commits under wrong names).
Somebody please correct me if this weren't the case...
^ permalink raw reply
* Re: segfault for git log --graph --no-walk --grep a
From: Jeff King @ 2013-02-09 1:05 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Thomas Haller, Git List
In-Reply-To: <7va9rexqii.fsf@alter.siamese.dyndns.org>
On Fri, Feb 08, 2013 at 04:47:01PM -0800, Junio C Hamano wrote:
> > Yeah, that actually is a good point. We should be using logmsg_reencode
> > so that we look for strings in the user's encoding.
>
> Perhaps like this. Just like the previous one (which should be
> discarded), this makes the function always use the temporary strbuf,
> so doing this upfront actually loses more code than it adds ;-)
I like code simplification, but I worry a little about paying for the
extra copy in the common case. I did a best-of-five "git rev-list
--count --grep=foo HEAD" before and after your patch, though, and the
difference was well within the noise. So maybe it's not worth caring
about.
-Peff
^ permalink raw reply
* Re: Permission denied on home dir results in fatal error as of 1.8.1.1
From: Jonathan Nieder @ 2013-02-09 1:05 UTC (permalink / raw)
To: Nick Muerdter; +Cc: Junio C Hamano, git
In-Reply-To: <7v6222xqc4.fsf@alter.siamese.dyndns.org>
Junio C Hamano wrote:
> Nick Muerdter <stuff@nickm.org> writes:
>> As of git 1.8.1.1 and above (tested up to 1.8.1.3), if the home
>> directory can't be accessed, it results in a fatal error. In git 1.8.1
>> and below this same setup just resulted in warnings. Was this an
>> intentional change?
>
> I think this was done to not just help diagnosing misconfiguration,
> but to prevent an unintended misconfiguration from causing problems
> (e.g. the user thinks user.name is set up correctly, but forbids Git
> from reading it from the configuration files, and ends up creating
> commits under wrong names).
Yes, that's right. Sometimes ignoring settings has bad consequences,
so git errors out to let the user intervene and decide whether the
inaccessible settings are important.
Thanks,
Jonathan
^ permalink raw reply
* Re: segfault for git log --graph --no-walk --grep a
From: Jeff King @ 2013-02-09 1:08 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Thomas Haller, Git List
In-Reply-To: <20130209010524.GB5469@sigill.intra.peff.net>
On Fri, Feb 08, 2013 at 08:05:24PM -0500, Jeff King wrote:
> On Fri, Feb 08, 2013 at 04:47:01PM -0800, Junio C Hamano wrote:
>
> > > Yeah, that actually is a good point. We should be using logmsg_reencode
> > > so that we look for strings in the user's encoding.
> >
> > Perhaps like this. Just like the previous one (which should be
> > discarded), this makes the function always use the temporary strbuf,
> > so doing this upfront actually loses more code than it adds ;-)
>
> I like code simplification, but I worry a little about paying for the
> extra copy in the common case. I did a best-of-five "git rev-list
> --count --grep=foo HEAD" before and after your patch, though, and the
> difference was well within the noise. So maybe it's not worth caring
> about.
Oh, hold on, I'm incompetent. I measured the wrong build of git. Here
are the timings for git.git:
[before]
$ best-of-five git rev-list --count --grep=foo HEAD
Attempt 1: 0.503
Attempt 2: 0.5
Attempt 3: 0.501
Attempt 4: 0.502
Attempt 5: 0.5
real 0m0.500s
user 0m0.488s
sys 0m0.008s
[after]
$ best-of-five git rev-list --count --grep=foo HEAD
Attempt 1: 0.514
Attempt 2: 0.525
Attempt 3: 0.517
Attempt 4: 0.523
Attempt 5: 0.518
real 0m0.514s
user 0m0.480s
sys 0m0.028s
So not huge, but measurable.
-Peff
^ permalink raw reply
* Re: [PATCH] user-manual: Rewrite git-gc section for automatic packing
From: Javier Tia @ 2013-02-09 1:13 UTC (permalink / raw)
To: W. Trevor King; +Cc: Git, Junio C Hamano
In-Reply-To: <7ac63ea832711ad4bee636163e277a408cbddda4.1360341577.git.wking@tremily.us>
> +information from taking up too much space on disk or in memory. Some
> +git commands may automatically run linkgit:git-gc[1], so you don't
> +have to worry about running it manually. However, compressing large
> +repositories may take some time, so you might want to disable
> +automatic comression and run it explicitly when you are not doing
^^^^^^^^
You might want to make a little correction by 'compression'.
Regards,
--
Javier
^ permalink raw reply
* git bisect result off by 1 commit
From: Tim Chen @ 2013-02-09 1:54 UTC (permalink / raw)
To: git; +Cc: ak
When I am doing a git bisect to track down a problem commit on the Linux
kernel tree, I found that git bisect actually led me to a patch that's one
before the problem commit.
In particular,
$ git bisect replay bisectlog
Previous HEAD position was d54b1a9... perf script: Remove use of die/exit
HEAD is now at a0d271c... Linux 3.6
Bisecting: 0 revisions left to test after this (roughly 0 steps)
[d54b1a9e0eaca92cde678d19bd82b9594ed00450] perf script: Remove use of die/exit
However, the patch that is problematic is the one before the one git bisect indicated.
[commit 8d3eca20b9f31cf10088e283d704f6a71b9a4ee2].
I am running git 1.7.11.7.
Tim
The bisect record is as follow:
------------bisectlog-----------------
git bisect start
# good: [a0d271cbfed1dd50278c6b06bead3d00ba0a88f9] Linux 3.6
git bisect good a0d271cbfed1dd50278c6b06bead3d00ba0a88f9
# bad: [ddffeb8c4d0331609ef2581d84de4d763607bd37] Linux 3.7-rc1
git bisect bad ddffeb8c4d0331609ef2581d84de4d763607bd37
# bad: [24d7b40a60cf19008334bcbcbd98da374d4d9c64] ARM: OMAP2+: PM: MPU DVFS: use generic CPU device for MPU-SS
git bisect bad 24d7b40a60cf19008334bcbcbd98da374d4d9c64
# bad: [d9a807461fc8cc0d6ba589ea0730d139122af012] Merge tag 'usb-3.6' of git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb
git bisect bad d9a807461fc8cc0d6ba589ea0730d139122af012
# bad: [06d2fe153b9b35e57221e35831a26918f462db68] Merge tag 'driver-core-3.6' of git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/driver-core
git bisect bad 06d2fe153b9b35e57221e35831a26918f462db68
# bad: [7e92daaefa68e5ef1e1732e45231e73adbb724e7] Merge branch 'perf-core-for-linus' of git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip
git bisect bad 7e92daaefa68e5ef1e1732e45231e73adbb724e7
# good: [620e77533f29796df7aff861e79bd72e08554ebb] Merge branch 'core-rcu-for-linus' of git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip
git bisect good 620e77533f29796df7aff861e79bd72e08554ebb
# bad: [8ad7013b252ba683055df19e657eb03d98f4f312] perf test: Add round trip test for sw and hw event names
git bisect bad 8ad7013b252ba683055df19e657eb03d98f4f312
# good: [f47b58b75f5e2a424834eb15f7565a7458a12f44] perf symbols: Fix builds with NO_LIBELF set
git bisect good f47b58b75f5e2a424834eb15f7565a7458a12f44
# good: [e1aa7c30c599e99b4544f9e5b4c275c8a5325bdc] tools lib traceevent: Fix strerror_r() use in pevent_strerror
git bisect good e1aa7c30c599e99b4544f9e5b4c275c8a5325bdc
# good: [ff1a70e75fd005821ab5f2211312a8aa13bbf959] tools lib traceevent: Modify header to work in C++ programs
git bisect good ff1a70e75fd005821ab5f2211312a8aa13bbf959
# bad: [4592281403e74dc4401d5803ec9948d43bbee7ae] perf tools: Remove the node from rblist in strlist__remove
git bisect bad 4592281403e74dc4401d5803ec9948d43bbee7ae
# good: [cc58482133296f52873be909a2795f6d934ecec9] perf help: Remove use of die and handle errors
git bisect good cc58482133296f52873be909a2795f6d934ecec9
# bad: [8d3eca20b9f31cf10088e283d704f6a71b9a4ee2] perf record: Remove use of die/exit
git bisect bad 8d3eca20b9f31cf10088e283d704f6a71b9a4ee2
^ permalink raw reply
* Re: [PATCH v2 2/3] count-objects: report garbage files in pack directory too
From: Duy Nguyen @ 2013-02-09 1:58 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <7vwqui1w84.fsf@alter.siamese.dyndns.org>
On Sat, Feb 9, 2013 at 1:44 AM, Junio C Hamano <gitster@pobox.com> wrote:
>> +static void real_report_pack_garbage(const char *path, int len, const char *name)
>> +{
>
> Don't some callers call this on paths outside objects/pack/
> directory? Is it still report-pack-garbage?
In fact 3/3 uses it to report loose garbage. Will rename.
>> +static const char *known_pack_extensions[] = { ".pack", ".keep", NULL };
>
> This sounds wrong. Isn't ".idx" also known?
I had a comment saying "all known extensions related to a pack, except
.idx" but I dropped it. .idx being used as the pointer file needs to
be handled separately.
>> @@ -1024,14 +1074,37 @@ static void prepare_packed_git_one(char *objdir, int local)
>> int namelen = strlen(de->d_name);
>> struct packed_git *p;
>>
>> - if (!has_extension(de->d_name, ".idx"))
>> + if (len + namelen + 1 > sizeof(path)) {
>> + if (report_pack_garbage)
>> + report_pack_garbage(path, len - 1, de->d_name);
>
> A pack/in/a/very/long/path/pack-0000000000000000000000000000000000000000.pack
> may pass when fed to "git verify-pack", but this will report it as "garbage",
> without reporting what is wrong with it. Wouldn't that confuse users?
If all other git commands do not recognize the pack, then I think it's
still considered garbage. We could either
- make prepare_packed_git_one accept pack/in/a/very/long/path-...
- show the reason why we consider it garbage
Which option do we do? Option 1 may involve chdir in, stat stat stat
and chdir out to get around short PATH_MAX limit on some system.
>> - if (len + namelen + 1 > sizeof(path))
>> + if (!has_extension(de->d_name, ".idx")) {
>> + struct string_list_item *item;
>> + int i, n;
>> + if (!report_pack_garbage)
>> + continue;
>> + if (is_dot_or_dotdot(de->d_name))
>> + continue;
>> + for (i = 0; known_pack_extensions[i]; i++)
>> + if (has_extension(de->d_name,
>> + known_pack_extensions[i]))
>> + break;
>> + if (!known_pack_extensions[i]) {
>> + report_pack_garbage(path, 0, NULL);
>> + continue;
>> + }
>> + n = strlen(path) - strlen(known_pack_extensions[i]);
>> + item = string_list_append_nodup(&garbage,
>> + xstrndup(path, n));
>> + item->util = (void*)known_pack_extensions[i];
>> continue;
>> + }
>
> Why isn't this part more like this?
>
> if (dot-or-dotdot) {
> continue;
> } else if (has_extension(de->d_name, ".idx")) {
> do things for the .idx file;
> } else if (has_extension(de->d_name, ".pack") {
> do things for the .pack file, including
> queuing the name if we haven't seen
> corresponding .idx for later examination;
> } else if (has_extension(de->d_name, ".keep") {
> nothing special for now but we may
> want to add some other checks later
> } else {
> everything else is a garbage
> report_pack_garbage();
> }
Originally I checked known_extensions again in report_pack_garbage()
but after restructuring, yeah we can drop known_extensions and do it
your way. Looks much clearer.
--
Duy
^ permalink raw reply
* Re: git bisect result off by 1 commit
From: Junio C Hamano @ 2013-02-09 2:03 UTC (permalink / raw)
To: Tim Chen; +Cc: git, ak
In-Reply-To: <1360374853.17632.182.camel@schen9-DESK>
Tim Chen <tim.c.chen@linux.intel.com> writes:
> When I am doing a git bisect to track down a problem commit on the Linux
> kernel tree, I found that git bisect actually led me to a patch that's one
> before the problem commit.
>
> In particular,
>
> $ git bisect replay bisectlog
> Previous HEAD position was d54b1a9... perf script: Remove use of die/exit
> HEAD is now at a0d271c... Linux 3.6
> Bisecting: 0 revisions left to test after this (roughly 0 steps)
> [d54b1a9e0eaca92cde678d19bd82b9594ed00450] perf script: Remove use of die/exit
>
> However, the patch that is problematic is the one before the one git bisect indicated.
> [commit 8d3eca20b9f31cf10088e283d704f6a71b9a4ee2].
Looks perfectly normal to me. The message above:
> HEAD is now at a0d271c... Linux 3.6
> Bisecting: 0 revisions left to test after this (roughly 0 steps)
> [d54b1a9e0eaca92cde678d19bd82b9594ed00450] perf script: Remove use of die/exit
is asking you to test the commit at d54b1a9e and tell it the result
of the test. The message says "0 left to test *after* this";
doesn't it mean you still need to do *this*?
A bisecct session ends when it tells you
XXXXXX is the first bad commit
which I do not see in the above. You seem to have stopped before
that happens.
^ permalink raw reply
* Re: inotify to minimize stat() calls
From: Duy Nguyen @ 2013-02-09 2:10 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Ramkumar Ramachandra, Git List
In-Reply-To: <7va9rezaoy.fsf@alter.siamese.dyndns.org>
On Sat, Feb 9, 2013 at 5:45 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Junio C Hamano <gitster@pobox.com> writes:
>
>> Ramkumar Ramachandra <artagnon@gmail.com> writes:
>>
>>> ... Will Git ever
>>> consider using inotify on Linux? What is the downside?
>>
>> I think this has come up from time to time, but my understanding is
>> that nobody thought things through to find a good layer in the
>> codebase to interface to an external daemon that listens to inotify
>> events yet. It is not something like "somebody decreed that we
>> would never consider because of such and such downsides." We are
>> not there yet.
>
> I checked read-cache.c and preload-index.c code. To get the
> discussion rolling, I think something like the outline below may be
> a good starting point and a feasible weekend hack for somebody
> competent:
>
> * At the beginning of preload_index(), instead of spawning the
> worker thread and doing the lstat() check ourselves, we open a
> socket to our daemon (see below) that watches this repository and
Can we replace "open a socket to our daemon" with "open a special file
in .git to get stat data written by our daemon"? TCP/IP socket means
system-wide daemon, not attractive. UNIX socket is not available on
Windows (although there may be named pipe, I don't know).
> make a request for lstat update. The request will contain:
>
> - The SHA1 checksum of the index file we just read (to ensure
> that we and our daemon share the same baseline to
> communicate); and
>
> - the pathspec data.
>
> Our daemon, if it already has a fresh data available, will give
> us a list of <path, lstat result>. Our main process runs a loop
> that is equivalent to what preload_thread() runs but uses the
> lstat() data we obtained from the daemon. If our daemon says it
> does not have a fresh data (or somehow our daemon is dead), we do
> the work ourselves.
>
> * Our daemon watches the index file and the working tree, and
> waits for the above consumer. First it reads the index (and
> remembers what it read), and whenever an inotify event comes,
> does the lstat() and remembers the result. It never writes
> to the index, and does not hold the index lock. Whenever the
> index file changes, it needs to reload the index, and discard
> lstat() data it already has for paths that are lost from the
> updated index.
--
Duy
^ permalink raw reply
* Re: Permission denied on home dir results in fatal error as of 1.8.1.1
From: Nick Muerdter @ 2013-02-09 2:33 UTC (permalink / raw)
To: Jonathan Nieder; +Cc: Junio C Hamano, git
In-Reply-To: <20130209010534.GC8461@google.com>
On Fri, Feb 8, 2013 at 6:05 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:
> Junio C Hamano wrote:
>> Nick Muerdter <stuff@nickm.org> writes:
>
>>> As of git 1.8.1.1 and above (tested up to 1.8.1.3), if the home
>>> directory can't be accessed, it results in a fatal error. In git 1.8.1
>>> and below this same setup just resulted in warnings. Was this an
>>> intentional change?
>>
>> I think this was done to not just help diagnosing misconfiguration,
>> but to prevent an unintended misconfiguration from causing problems
>> (e.g. the user thinks user.name is set up correctly, but forbids Git
>> from reading it from the configuration files, and ends up creating
>> commits under wrong names).
>
> Yes, that's right. Sometimes ignoring settings has bad consequences,
> so git errors out to let the user intervene and decide whether the
> inaccessible settings are important.
Thanks for the quick response.
Just for reference, the specific issue I ran into stems from using
Chef to provision servers. Chef gets run as root but can perform the
git commands as a different user on the system. The way this appears
to be implemented is to fork, set the uid, and then execute git. But
since the HOME environment variable is still set to /root, this leads
to this fatal error. This can obviously be fixed on the script's end
by properly determining and setting the HOME before executing git, but
more care has to be taken, and I'm not sure how common this fork + set
uid + exec approach in other programs might be. But I'll file a bug
with Chef to get it fixed there.
Thanks again,
Nick
^ permalink raw reply
* Re: inotify to minimize stat() calls
From: Junio C Hamano @ 2013-02-09 2:37 UTC (permalink / raw)
To: Duy Nguyen; +Cc: Ramkumar Ramachandra, Git List
In-Reply-To: <CACsJy8DW=tkEy2iOAZxQ+ZyVQ+L11JsPcSxrES5YY7gECmX7UQ@mail.gmail.com>
Duy Nguyen <pclouds@gmail.com> writes:
> Can we replace "open a socket to our daemon" with "open a special file
> in .git to get stat data written by our daemon"? TCP/IP socket means
> system-wide daemon, not attractive. UNIX socket is not available on
> Windows (although there may be named pipe, I don't know).
I do not think TCP/IP socket is too bad (you have to be able to read
the index file to be able to ask questions to the daemon to begin
with, so you must have list of paths already; the answer from the
daemon would not leak anything more sensitive than you can already
know), and UNIX domain socket is not too bad either.
Just like the implementation detail of the daemon itself may differ
on platforms (does Windows have the identical inotify interface? I
doubt it), I expect the RPC mechanism between the daemon and the
client would be platform dependent. So take that "open a socket" as
a generic way to say "have these two communicate with some magic",
nothing more.
^ permalink raw reply
* Re: inotify to minimize stat() calls
From: Junio C Hamano @ 2013-02-09 2:56 UTC (permalink / raw)
To: Git List; +Cc: Duy Nguyen, Ramkumar Ramachandra
In-Reply-To: <7va9rezaoy.fsf@alter.siamese.dyndns.org>
Junio C Hamano <gitster@pobox.com> writes:
> I checked read-cache.c and preload-index.c code. To get the
> discussion rolling, I think something like the outline below may be
> a good starting point and a feasible weekend hack for somebody
> competent:
>
> * At the beginning of preload_index(), instead of spawning the
> worker thread and doing the lstat() check ourselves, we open a
> socket to our daemon (see below) that watches this repository and
> make a request for lstat update. The request will contain:
>
> - The SHA1 checksum of the index file we just read (to ensure
> that we and our daemon share the same baseline to
> communicate); and
>
> - the pathspec data.
>
> Our daemon, if it already has a fresh data available, will give
> us a list of <path, lstat result>. Our main process runs a loop
> that is equivalent to what preload_thread() runs but uses the
> lstat() data we obtained from the daemon. If our daemon says it
> does not have a fresh data (or somehow our daemon is dead), we do
> the work ourselves.
>
> * Our daemon watches the index file and the working tree, and
> waits for the above consumer. First it reads the index (and
> remembers what it read), and whenever an inotify event comes,
> does the lstat() and remembers the result. It never writes
> to the index, and does not hold the index lock. Whenever the
> index file changes, it needs to reload the index, and discard
> lstat() data it already has for paths that are lost from the
> updated index.
I left the details unsaid in thee above because I thought it was
fairly obvious from the nature of the "outline", but let me spend a
few more lines to avoid confusion.
- The way the daemon "watches" the changes to the working tree and
the index may well be very platform dependent. I said "inotify"
above, but the mechanism does not have to be inotify.
- The channel the daemon and the client communicates would also be
system dependent. UNIX domain socket in $GIT_DIR/ with a
well-known name would be one possibility but it does not have to
be the only option.
- The data given from the daemon to the client does not have to
include full lstat() information. They start from the same index
info, and the only thing preload_index() wants to know is for
which paths it should call ce_mark_uptodate(ce), so the answer
given by our daemon can be a list of paths.
^ permalink raw reply
* Re: inotify to minimize stat() calls
From: Robert Zeh @ 2013-02-09 3:36 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Git List, Duy Nguyen, Ramkumar Ramachandra
In-Reply-To: <7vsj56w5y9.fsf@alter.siamese.dyndns.org>
The delay for commands like git status is much worse on Windows than Linux; for my workflow I would be happy with a Windows only implementation.
>From the description so far, I have some question: how does the daemon get started and stopped? Is there one per repository --- this seems to be implied by putting the unix domain socket in $GIT_DIR. Could we automatically reject connections from anything other than localhost when using TCP?
Robert Zeh
On Feb 8, 2013, at 8:56 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Junio C Hamano <gitster@pobox.com> writes:
>
>> I checked read-cache.c and preload-index.c code. To get the
>> discussion rolling, I think something like the outline below may be
>> a good starting point and a feasible weekend hack for somebody
>> competent:
>>
>> * At the beginning of preload_index(), instead of spawning the
>> worker thread and doing the lstat() check ourselves, we open a
>> socket to our daemon (see below) that watches this repository and
>> make a request for lstat update. The request will contain:
>>
>> - The SHA1 checksum of the index file we just read (to ensure
>> that we and our daemon share the same baseline to
>> communicate); and
>>
>> - the pathspec data.
>>
>> Our daemon, if it already has a fresh data available, will give
>> us a list of <path, lstat result>. Our main process runs a loop
>> that is equivalent to what preload_thread() runs but uses the
>> lstat() data we obtained from the daemon. If our daemon says it
>> does not have a fresh data (or somehow our daemon is dead), we do
>> the work ourselves.
>>
>> * Our daemon watches the index file and the working tree, and
>> waits for the above consumer. First it reads the index (and
>> remembers what it read), and whenever an inotify event comes,
>> does the lstat() and remembers the result. It never writes
>> to the index, and does not hold the index lock. Whenever the
>> index file changes, it needs to reload the index, and discard
>> lstat() data it already has for paths that are lost from the
>> updated index.
>
> I left the details unsaid in thee above because I thought it was
> fairly obvious from the nature of the "outline", but let me spend a
> few more lines to avoid confusion.
>
> - The way the daemon "watches" the changes to the working tree and
> the index may well be very platform dependent. I said "inotify"
> above, but the mechanism does not have to be inotify.
>
> - The channel the daemon and the client communicates would also be
> system dependent. UNIX domain socket in $GIT_DIR/ with a
> well-known name would be one possibility but it does not have to
> be the only option.
>
> - The data given from the daemon to the client does not have to
> include full lstat() information. They start from the same index
> info, and the only thing preload_index() wants to know is for
> which paths it should call ce_mark_uptodate(ce), so the answer
> given by our daemon can be a list of paths.
> --
> 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
* Please pull l10n updates for 1.8.2 round 1
From: Jiang Xin @ 2013-02-09 3:55 UTC (permalink / raw)
To: Junio C Hamano
Cc: Ralf Thielow, Thomas Rast, Wang Sheng, Peter Krefting,
Tran Ngoc Quan, Git List
Hi, Junio
The following changes since commit ec3ae6ec46ed48383ae40643990f169b65a563cc:
Merge git://ozlabs.org/~paulus/gitk (2013-01-23 08:35:03 -0800)
are available in the git repository at:
git://github.com/git-l10n/git-po/ master
for you to fetch changes up to 1bbe7c3c124c435b45f87a71516211f5252086f7:
l10n: de.po: translate "reset" as "neu setzen" (2013-02-08 20:43:30 +0100)
----------------------------------------------------------------
Jiang Xin (2):
l10n: Update git.pot (11 new, 7 removed messages)
Merge branch 'master' of git://github.com/ralfth/git-po-de
Peter Krefting (1):
l10n: Update Swedish translation (1983t0f0u)
Ralf Thielow (4):
l10n: de.po: fix some minor issues
l10n: de.po: translate 11 new messages
l10n: de.po: translate "revision" consistently as "Revision"
l10n: de.po: translate "reset" as "neu setzen"
Tran Ngoc Quan (1):
l10n: vi.po: updated Vietnamese translation
Wang Sheng (1):
l10n: zh_CN.po: 800+ new translations on command usages
po/TEAMS | 1 +
po/de.po | 1545 ++++++++++++----------
po/git.pot | 1137 ++++++++--------
po/sv.po | 1164 ++++++++--------
po/vi.po | 1182 +++++++++--------
po/zh_CN.po | 4258 +++++++++++++++++++++++++++++------------------------------
6 files changed, 4728 insertions(+), 4559 deletions(-)
--
Jiang Xin
^ permalink raw reply
* [PATCH v5] Add utf8_fprintf helper that returns correct columns
From: Jiang Xin @ 2013-02-09 6:31 UTC (permalink / raw)
To: Junio C Hamano
Cc: Git List, Torsten Bögershausen,
Nguyễn Thái Ngọc Duy, Jiang Xin
In-Reply-To: <51152575.8090507@web.de>
Since command usages can be translated, they may include utf-8 encoded
strings, and the output in console may not align well any more. This is
because strlen() is different from strwidth() on utf-8 strings. A wrapper
utf8_fprintf() can help to return the correct columns required.
Signed-off-by: Jiang Xin <worldhello.net@gmail.com>
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Reviewed-by: Torsten Bögershausen <tboegi@web.de>
---
parse-options.c | 5 +++--
utf8.c | 21 +++++++++++++++++++++
utf8.h | 1 +
3 files changed, 25 insertions(+), 2 deletions(-)
diff --git a/parse-options.c b/parse-options.c
index 67e98..a6ce9e 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -3,6 +3,7 @@
#include "cache.h"
#include "commit.h"
#include "color.h"
+#include "utf8.h"
static int parse_options_usage(struct parse_opt_ctx_t *ctx,
const char * const *usagestr,
@@ -482,7 +483,7 @@ static int usage_argh(const struct option *opts, FILE *outfile)
s = literal ? "[%s]" : "[<%s>]";
else
s = literal ? " %s" : " <%s>";
- return fprintf(outfile, s, opts->argh ? _(opts->argh) : _("..."));
+ return utf8_fprintf(outfile, s, opts->argh ? _(opts->argh) : _("..."));
}
#define USAGE_OPTS_WIDTH 24
@@ -541,7 +542,7 @@ static int usage_with_options_internal(struct parse_opt_ctx_t *ctx,
if (opts->long_name)
pos += fprintf(outfile, "--%s", opts->long_name);
if (opts->type == OPTION_NUMBER)
- pos += fprintf(outfile, "-NUM");
+ pos += utf8_fprintf(outfile, _("-NUM"));
if ((opts->flags & PARSE_OPT_LITERAL_ARGHELP) ||
!(opts->flags & PARSE_OPT_NOARG))
diff --git a/utf8.c b/utf8.c
index a4ee6..c09dc 100644
--- a/utf8.c
+++ b/utf8.c
@@ -430,6 +430,27 @@ int same_encoding(const char *src, const char *dst)
}
/*
+ * Wrapper for fprintf and returns the total number of columns required
+ * for the printed string, assuming that the string is utf8.
+ */
+int utf8_fprintf(FILE *stream, const char *format, ...)
+{
+ struct strbuf buf = STRBUF_INIT;
+ va_list arg;
+ int columns;
+
+ va_start (arg, format);
+ strbuf_vaddf(&buf, format, arg);
+ va_end (arg);
+
+ columns = fputs(buf.buf, stream);
+ if (0 <= columns) /* keep the error from the I/O */
+ columns = utf8_strwidth(buf.buf);
+ strbuf_release(&buf);
+ return columns;
+}
+
+/*
* Given a buffer and its encoding, return it re-encoded
* with iconv. If the conversion fails, returns NULL.
*/
diff --git a/utf8.h b/utf8.h
index a2142..501b2 100644
--- a/utf8.h
+++ b/utf8.h
@@ -8,6 +8,7 @@ int utf8_strwidth(const char *string);
int is_utf8(const char *text);
int is_encoding_utf8(const char *name);
int same_encoding(const char *, const char *);
+int utf8_fprintf(FILE *, const char *, ...);
void strbuf_add_wrapped_text(struct strbuf *buf,
const char *text, int indent, int indent2, int width);
--
1.8.1.1.370.gf39fc7e
^ permalink raw reply
* Re: Proposal: branch.<name>.remotepush
From: Ramkumar Ramachandra @ 2013-02-09 7:29 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jonathan Nieder, Michael Schubert, Git List
In-Reply-To: <7vhalm1unu.fsf@alter.siamese.dyndns.org>
Junio C Hamano wrote:
> Ramkumar Ramachandra <artagnon@gmail.com> writes:
>
>> Jonathan Nieder wrote:
>>> Ramkumar Ramachandra wrote:
>>>
>>>> And yes, a regular `git push origin refs/for/master` is just retarded.
>>>
>>> The usual incantation is "git push gerrit HEAD:refs/for/master". Is
>>> the code review creation push that uses a different branchname from
>>> the branch the integrator pulls what seems backward, or is it the need
>>> to specify a refname at all on the command line?
>>
>> How else would you design a system to differentiate between a
>> push-for-review, and push-to-update-ref?
>
> You don't have to.
>
> If the reviewed result is merged on the server side and appear on
> 'master', nobody has to push to update refs/heads/master.
I'm sorry, I meant differentiating between push-for-review and
push-for-personal-work.
^ permalink raw reply
* Re: [RFC/PATCH] Introduce branch.<name>.pushremote
From: Ramkumar Ramachandra @ 2013-02-09 7:46 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Git List, Jonathan Nieder, Jeff King
In-Reply-To: <7v8v6y1sih.fsf@alter.siamese.dyndns.org>
Junio C Hamano wrote:
> Ramkumar Ramachandra <artagnon@gmail.com> writes:
>
>> diff --git a/remote.c b/remote.c
>> index e53a6eb..d6fcfc0 100644
>> --- a/remote.c
>> +++ b/remote.c
>> @@ -48,6 +48,7 @@ static int branches_nr;
>>
>> static struct branch *current_branch;
>> static const char *default_remote_name;
>> +static const char *pushremote_name;
>> static int explicit_default_remote_name;
>>
>> static struct rewrites rewrites;
>> @@ -363,6 +364,12 @@ static int handle_config(const char *key, const char *value, void *cb)
>> default_remote_name = branch->remote_name;
>> explicit_default_remote_name = 1;
>> }
>> + } else if (!strcmp(subkey, ".pushremote")) {
>> + if (!value)
>> + return config_error_nonbool(key);
>> + branch->pushremote_name = xstrdup(value);
>
> Perhaps use git_config_string()?
I was just following the style of the surrounding code without
thinking. However, it looks like the surrounding code may be dated,
so I'll include a patch to update it to use git_config_string() before
making the change here.
>> + if (branch == current_branch)
>> + pushremote_name = branch->pushremote_name;
>
> Why is this global only when current_branch is involved?
>
> In other words, does it make sense to read branch.$name.pushremote
> for all the other irrelevant branches?
>
> In yet other words, perhaps adding pushremote_name to the branch
> structure is unneeded, and you only need this single global
> variable?
Frankly, I'm unhappy with this global. Setting a global here and
subsequently reading it in pushremote_get() feels flaky. Why use it
at all when we have branch->remote_name, branch->remote, and (the now
introduced) branch->pushremote_name? I left the pushremote_name field
around, with the expectation that other codepaths that use the
remote_name field might be able to use it.
Thanks.
^ permalink raw reply
* Re: [RFC/PATCH] Introduce remote.pushdefault
From: Ramkumar Ramachandra @ 2013-02-09 7:50 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Git List, Jonathan Nieder, Jeff King
In-Reply-To: <7v4nhm1s85.fsf@alter.siamese.dyndns.org>
Junio C Hamano wrote:
> Ramkumar Ramachandra <artagnon@gmail.com> writes:
>
>> diff --git a/Documentation/config.txt b/Documentation/config.txt
>> index 9b11597..82a4a78 100644
>> --- a/Documentation/config.txt
>> +++ b/Documentation/config.txt
>> @@ -1884,6 +1884,10 @@ receive.updateserverinfo::
>> If set to true, git-receive-pack will run git-update-server-info
>> after receiving data from git-push and updating refs.
>>
>> +remote.pushdefault::
>> + The remote to push to by default. Overrides the
>> + branch-specific configuration `branch.<name>.remote`.
>
> It feels unexpected to see "I may have said while on this branch I
> push there and on that branch I push somewhere else, but no, with
> this single configuration I'm invalidating all these previous
> statements, and all pushes go to this new place".
>
> Shouldn't the default be the default that is to be overridden by
> other configuration that is more specific? That is, "I would
> normally push to this remote and unless I say otherwise that is all
> I have to say, but for this particular branch, I push to somehwere
> else".
Oops, I meant to have it overriden by branch-specific configuration. Fixed now.
>> diff --git a/builtin/push.c b/builtin/push.c
>> index 42b129d..d447a80 100644
>> --- a/builtin/push.c
>> +++ b/builtin/push.c
>> @@ -322,7 +322,7 @@ static int push_with_options(struct transport *transport, int flags)
>> static int do_push(const char *repo, int flags)
>> {
>> int i, errs;
>> - struct remote *remote = remote_get(repo);
>> + struct remote *remote = pushremote_get(repo);
>> const char **url;
>> int url_nr;
>>
>> diff --git a/remote.c b/remote.c
>> index e53a6eb..08bb803 100644
>> --- a/remote.c
>> +++ b/remote.c
>> @@ -48,6 +48,7 @@ static int branches_nr;
>>
>> static struct branch *current_branch;
>> static const char *default_remote_name;
>> +static const char *pushremote_name;
>> static int explicit_default_remote_name;
>>
>> static struct rewrites rewrites;
>> @@ -349,6 +350,14 @@ static int handle_config(const char *key, const char *value, void *cb)
>> const char *subkey;
>> struct remote *remote;
>> struct branch *branch;
>> + if (!prefixcmp(key, "remote.")) {
>> + name = key + 7;
>> + if (!strcmp(name, "pushdefault")) {
>> + if (!value)
>> + return config_error_nonbool(key);
>> + pushremote_name = xstrdup(value);
>> + }
>> + }
>> if (!prefixcmp(key, "branch.")) {
>> name = key + 7;
>> subkey = strrchr(name, '.');
>> @@ -388,8 +397,6 @@ static int handle_config(const char *key, const char *value, void *cb)
>> add_instead_of(rewrite, xstrdup(value));
>> }
>> }
>> - if (prefixcmp(key, "remote."))
>> - return 0;
>
> Why is this no longer needed?
>
> All the remainder of this function is about "remote.*" config and
> this rejects other keys, like "user.name", etc.
I'm sorry. I read that as if (!prefixcmp(key, "remote.")), which is
an entirely different thing. Fixed now.
Thanks.
^ permalink raw reply
* Wishlist: git help bisect should mention "stop" keyword
From: Andreas Mohr @ 2013-02-09 8:44 UTC (permalink / raw)
To: git
Hi,
the man page (git version 1.7.10.4) is a bit non-symmetric
since git bisect has the start param, but when searching for "stop"
(nothing more obvious than that, right?),
one comes up empty --> usability issue.
The appropriate action complementary to start appears to be
git bisect reset, thus its description definitely ought to include a "stop"
keyword.
Description as of 1.7.10.4 is
After a bisect session, to clean up the bisection state and
return to
the original HEAD, issue the following command:
$ git bisect reset
which could be changed into
After a bisect session, to clean up the bisection state and
return to
the original HEAD (in other words, to "stop" bisect),
issue the following command:
$ git bisect reset
Andreas Mohr
--
GNU/Linux. It's not the software that's free, it's you.
^ 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