* Re: [PATCH] revert.c: defer writing CHERRY_PICK_HEAD till it is safe to do so
From: Jay Soffian @ 2011-10-06 17:33 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, nicolas.dichtel, Jeff King
In-Reply-To: <CAG+J_DxqpDAm-qPR-Udkr_b1gc=Y+LoKbsQdmiCi6ztNKz0_Mg@mail.gmail.com>
On Thu, Oct 6, 2011 at 1:28 PM, Jay Soffian <jaysoffian@gmail.com> wrote:
> On Thu, Oct 6, 2011 at 1:02 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> I think do_recursive_merge() would die() when the merge cannot even start
>> (i.e. the local changes after the cherry-pick exits are the ones from the
>> time before such a failed cherry-pick started), but I suspect that the
>> other codepath uses try_merge_command() to drive strategies other than
>> recursive and it does not die() there in such a case. Can you make sure
>> this patch is sufficient in such a case as well?
>
> It's wrong to write out CHERRY_PICK_HEAD if we couldn't start the
> merge, but using cherry-pick with a strategy other than recursive
> seems broken in that case in other ways as well:
>
> $ git cherry-pick --strategy=resolve side
> error: Untracked working tree file 'bar' would be overwritten by merge.
> error: could not apply a77535c9ac... bar
> hint: after resolving the conflicts, mark the corrected paths
> hint: with 'git add <paths>' or 'git rm <paths>'
> hint: and commit the result with 'git commit'
>
> The "hint" advice is completely wrong.
For that matter, I don't see how to distinguish "the merge did not
even start" from "the merge had conflicts" when using
try_merge_command(). In the former case we do NOT want
CHERRY_PICK_HEAD left behind (nor to print the wrong advice above),
while in the latter case we do.
j.
^ permalink raw reply
* Re: [PATCH] mingw: avoid using strbuf in syslog
From: Erik Faye-Lund @ 2011-10-06 17:32 UTC (permalink / raw)
To: Brandon Casey; +Cc: git, j.sixt, gitster
In-Reply-To: <CA+sFfMediaCnzFH6uKfVc_Bb+W+AA1nO+OdvB8vWjjrsD1kh9w@mail.gmail.com>
On Thu, Oct 6, 2011 at 7:27 PM, Brandon Casey <drafnel@gmail.com> wrote:
> On Thu, Oct 6, 2011 at 11:57 AM, Erik Faye-Lund <kusmabite@gmail.com> wrote:
>> strbuf can call die, which again can call syslog from git-daemon.
>>
>> Endless recursion is no fun; fix it by hand-rolling the logic.
>>
>> Signed-off-by: Erik Faye-Lund <kusmabite@gmail.com>
>> ---
>> Fixes the problem Brandon pointed out...
>
> Actually, Johannes should get that credit. He brought up the whole
> recursion issue.
>
OK, I didn't spot that one. But it's not in the commit message,
though. Perhaps it should be?
>> compat/win32/syslog.c | 26 ++++++++++++++------------
>> 1 files changed, 14 insertions(+), 12 deletions(-)
>>
>> diff --git a/compat/win32/syslog.c b/compat/win32/syslog.c
>> index 42b95a9..3b8e2b7 100644
>> --- a/compat/win32/syslog.c
>> +++ b/compat/win32/syslog.c
>> @@ -1,5 +1,4 @@
>> #include "../../git-compat-util.h"
>> -#include "../../strbuf.h"
>>
>> static HANDLE ms_eventlog;
>>
>> @@ -16,13 +15,8 @@ void openlog(const char *ident, int logopt, int facility)
>>
>> void syslog(int priority, const char *fmt, ...)
>> {
>> - struct strbuf sb = STRBUF_INIT;
>> - struct strbuf_expand_dict_entry dict[] = {
>> - {"1", "% 1"},
>> - {NULL, NULL}
>> - };
>> WORD logtype;
>> - char *str;
>> + char *str, *pos;
>> int str_len;
>> va_list ap;
>>
>> @@ -39,11 +33,20 @@ void syslog(int priority, const char *fmt, ...)
>> }
>>
>> str = malloc(str_len + 1);
>> + if (!str)
>> + return; /* no chance to report error */
>> +
>
> Just above this, warning() is used to at least print a message to
> stderr if vsnprintf() fails. It could be used here, and below when
> realloc fails.
>
Good point, so this should be squashed in:
---
diff --git a/compat/win32/syslog.c b/compat/win32/syslog.c
index 3b8e2b7..d015e43 100644
--- a/compat/win32/syslog.c
+++ b/compat/win32/syslog.c
@@ -33,8 +33,10 @@ void syslog(int priority, const char *fmt, ...)
}
str = malloc(str_len + 1);
- if (!str)
- return; /* no chance to report error */
+ if (!str) {
+ warning("malloc failed: '%s'", strerror(errno));
+ return;
+ }
va_start(ap, fmt);
vsnprintf(str, str_len + 1, fmt, ap);
@@ -42,8 +44,10 @@ void syslog(int priority, const char *fmt, ...)
while ((pos = strstr(str, "%1")) != NULL) {
str = realloc(str, ++str_len + 1);
- if (!str)
+ if (!str) {
+ warning("realloc failed: '%s'", strerror(errno));
return;
+ }
memmove(pos + 2, pos + 1, strlen(pos));
pos[1] = ' ';
}
^ permalink raw reply related
* Re: What's cooking in git.git (Oct 2011, #01; Tue, 4)
From: Pascal Obry @ 2011-10-06 17:32 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <7vlisy119i.fsf@alter.siamese.dyndns.org>
Le 06/10/2011 19:24, Junio C Hamano a écrit :
> The impression I got from the discussion was quite different, which was
> that the patch was not even "good" by making certain things impossible to
> do by catering to a narrow corner case.
I have understood that this was not completely fixing all
slash/backslash issues (is it even possible?) but in no way there was
regressions pointed out. At least a specific error *was* fixed. But I
may have missed something, in that case sorry.
Pascal.
--
--|------------------------------------------------------
--| Pascal Obry Team-Ada Member
--| 45, rue Gabriel Peri - 78114 Magny Les Hameaux FRANCE
--|------------------------------------------------------
--| http://www.obry.net - http://v2p.fr.eu.org
--| "The best way to travel is by means of imagination"
--|
--| gpg --keyserver keys.gnupg.net --recv-key F949BD3B
^ permalink raw reply
* Re: [PATCH] revert.c: defer writing CHERRY_PICK_HEAD till it is safe to do so
From: Junio C Hamano @ 2011-10-06 17:28 UTC (permalink / raw)
To: Jay Soffian; +Cc: git, nicolas.dichtel, Jeff King
In-Reply-To: <CAG+J_Dzh2Njjwykt-f4w_YqpftrJEpQfUW2OvSRs_MSPcNFQnw@mail.gmail.com>
Jay Soffian <jaysoffian@gmail.com> writes:
> For that matter, why does revert.c have it's own implementation of
> recursive instead of just calling try_merge_command("recursive", ...)?
I think the people who did this part wanted not to fork.
^ permalink raw reply
* Re: [PATCH] revert.c: defer writing CHERRY_PICK_HEAD till it is safe to do so
From: Jay Soffian @ 2011-10-06 17:28 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, nicolas.dichtel, Jeff King
In-Reply-To: <7vty7m12b2.fsf@alter.siamese.dyndns.org>
On Thu, Oct 6, 2011 at 1:02 PM, Junio C Hamano <gitster@pobox.com> wrote:
> I think do_recursive_merge() would die() when the merge cannot even start
> (i.e. the local changes after the cherry-pick exits are the ones from the
> time before such a failed cherry-pick started), but I suspect that the
> other codepath uses try_merge_command() to drive strategies other than
> recursive and it does not die() there in such a case. Can you make sure
> this patch is sufficient in such a case as well?
It's wrong to write out CHERRY_PICK_HEAD if we couldn't start the
merge, but using cherry-pick with a strategy other than recursive
seems broken in that case in other ways as well:
$ git cherry-pick --strategy=resolve side
error: Untracked working tree file 'bar' would be overwritten by merge.
error: could not apply a77535c9ac... bar
hint: after resolving the conflicts, mark the corrected paths
hint: with 'git add <paths>' or 'git rm <paths>'
hint: and commit the result with 'git commit'
The "hint" advice is completely wrong.
j.
^ permalink raw reply
* Re: [PATCH] mingw: avoid using strbuf in syslog
From: Brandon Casey @ 2011-10-06 17:27 UTC (permalink / raw)
To: Erik Faye-Lund; +Cc: git, j.sixt, gitster
In-Reply-To: <1317920244-6320-1-git-send-email-kusmabite@gmail.com>
On Thu, Oct 6, 2011 at 11:57 AM, Erik Faye-Lund <kusmabite@gmail.com> wrote:
> strbuf can call die, which again can call syslog from git-daemon.
>
> Endless recursion is no fun; fix it by hand-rolling the logic.
>
> Signed-off-by: Erik Faye-Lund <kusmabite@gmail.com>
> ---
> Fixes the problem Brandon pointed out...
Actually, Johannes should get that credit. He brought up the whole
recursion issue.
> compat/win32/syslog.c | 26 ++++++++++++++------------
> 1 files changed, 14 insertions(+), 12 deletions(-)
>
> diff --git a/compat/win32/syslog.c b/compat/win32/syslog.c
> index 42b95a9..3b8e2b7 100644
> --- a/compat/win32/syslog.c
> +++ b/compat/win32/syslog.c
> @@ -1,5 +1,4 @@
> #include "../../git-compat-util.h"
> -#include "../../strbuf.h"
>
> static HANDLE ms_eventlog;
>
> @@ -16,13 +15,8 @@ void openlog(const char *ident, int logopt, int facility)
>
> void syslog(int priority, const char *fmt, ...)
> {
> - struct strbuf sb = STRBUF_INIT;
> - struct strbuf_expand_dict_entry dict[] = {
> - {"1", "% 1"},
> - {NULL, NULL}
> - };
> WORD logtype;
> - char *str;
> + char *str, *pos;
> int str_len;
> va_list ap;
>
> @@ -39,11 +33,20 @@ void syslog(int priority, const char *fmt, ...)
> }
>
> str = malloc(str_len + 1);
> + if (!str)
> + return; /* no chance to report error */
> +
Just above this, warning() is used to at least print a message to
stderr if vsnprintf() fails. It could be used here, and below when
realloc fails.
> va_start(ap, fmt);
> vsnprintf(str, str_len + 1, fmt, ap);
> va_end(ap);
> - strbuf_expand(&sb, str, strbuf_expand_dict_cb, &dict);
> - free(str);
> +
> + while ((pos = strstr(str, "%1")) != NULL) {
> + str = realloc(str, ++str_len + 1);
> + if (!str)
> + return;
> + memmove(pos + 2, pos + 1, strlen(pos));
> + pos[1] = ' ';
> + }
>
> switch (priority) {
> case LOG_EMERG:
-Brandon
^ permalink raw reply
* Re: What's cooking in git.git (Oct 2011, #01; Tue, 4)
From: Junio C Hamano @ 2011-10-06 17:27 UTC (permalink / raw)
To: Brad King; +Cc: git
In-Reply-To: <4E8DCF79.50200@kitware.com>
Brad King <brad.king@kitware.com> writes:
>> + push: teach --recurse-submodules the on-demand option
>> (this branch is tangled with fg/submodule-auto-push.)
>>
>> The second from the bottom one needs to be replaced with a properly
>> written commit log message.
>
> are a separate topic.
Thanks for reading the list carefully. What you described is exactly what
"tangled with" means.
^ permalink raw reply
* Re: What's cooking in git.git (Oct 2011, #01; Tue, 4)
From: Junio C Hamano @ 2011-10-06 17:24 UTC (permalink / raw)
To: pascal; +Cc: git
In-Reply-To: <4E8DC01A.8060406@obry.net>
Pascal Obry <pascal@obry.net> writes:
>> Incomplete with respect to backslash processing in prefix_filename(), and
>> also loses the ability to escape glob specials.
>> Will discard.
>
> Sorry but this is letting best be enemy of good!
The impression I got from the discussion was quite different, which was
that the patch was not even "good" by making certain things impossible to
do by catering to a narrow corner case.
^ permalink raw reply
* "Use of uninitialized value" running "git svn clone"
From: Luiz-Otavio Zorzella @ 2011-10-06 17:23 UTC (permalink / raw)
To: git
I'm trying to convert a project (hosted in googlecode.com) from svn to
git, using the "git svn clone" command, and I'm getting an "Use of
uninitialized value" error. Here's the truncated output:
$ git svn clone https://test-libraries-for-java.googlecode.com/svn
--no-metadata -A ~/tmp/authors.txt -t tags -b branches -T trunk
test-libraries-for-java
r1 = c3adafa93a420f19b1bcfb6765fe0eb90aaa751c (refs/remotes/trunk)
A .classpath
A .project
A COPYING
A build.properties
A build.xml
W: +empty_dir: trunk/src
[...]
r10 = 8d5d7fdebdb7f822388fd3e4f4061abbfd1fb0cf (refs/remotes/trunk)
M test/com/google/common/testing/junit3/JUnitAssertsTest.java
r11 = 4c8a77660bf353ed55c9d583b39e263203c685a4 (refs/remotes/trunk)
Found possible branch point:
https://test-libraries-for-java.googlecode.com/svn/trunk =>
https://test-libraries-for-java.googlecode.com/svn/tags/release-1.0,
11
Use of uninitialized value $u in substitution (s///) at
/usr/lib/git-core/git-svn line 1731.
Use of uninitialized value $u in concatenation (.) or string at
/usr/lib/git-core/git-svn line 1731.
refs/remotes/trunk:
'https://test-libraries-for-java.googlecode.com/svn' not found in ''
For completeness, here's the authors.txt file I'm using:
$ cat ~/tmp/authors.txt
zorzella = Luiz-Otavio 'Z' Zorzella <zorzella@gmail.com>
(no author) = Luiz-Otavio 'Z' Zorzella <zorzella@gmail.com>
**************
Thanks in advance,
Z
^ permalink raw reply
* Re: [PATCH] commit: teach --gpg-sign option
From: Matthieu Moy @ 2011-10-06 17:22 UTC (permalink / raw)
To: Jonathan Nieder; +Cc: Shawn Pearce, Junio C Hamano, git
In-Reply-To: <20111006171107.GA10973@elie>
Jonathan Nieder <jrnieder@gmail.com> writes:
> I probably missed some earlier discussion (so please forgive me this),
(same here)
> What happens if my old key is compromised and I want to throw away the
> signatures and replace them with signatures using my new key?
With the patch we're discussing, signatures are part of history, hence
can't be modified after the fact without rewritting them.
*But*, by design, unless sha1 itself is compromized (in which case Git
would need to change to another hash function, that would be no fun),
signing the tip of every branch is sufficient to sign the whole history.
So, your old signatures would remain there, and your new signature, for
new commits, would be added on top.
> How does this relate to the "push certificate" use case, which seemed
> to be mostly about authenticating published branch tips with
> signatures that are not necessarily important in the long term?
I'm wondering how this feature would fit in a typical flow, indeed.
Usually, I hack for a while, and when I'm happy enough, I push. But I
don't take the decision of what to push at commit time, so if the idea
is to sign only a few commits (i.e. the ones you push), then you should
decide this at commit time ("hmm, I should commit --gpg-sign this time
because I'm going to push this one").
If the idea is to sign every commit, then there should be a config
option so that we don't have to type it every time.
--
Matthieu Moy
http://www-verimag.imag.fr/~moy/
^ permalink raw reply
* Re: [PATCH WIP 0/3] git log --exclude
From: Junio C Hamano @ 2011-10-06 17:22 UTC (permalink / raw)
To: Jeff King
Cc: Junio C Hamano, Nguyễn Thái Ngọc Duy, git,
Jonathan Nieder
In-Reply-To: <20111006143441.GA21558@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> The current protocol relies on certain repository properties on the
> remote end that narrow clone will violate. I don't see a way around that
> without a protocol extension to communicate the narrowness. What will
> that extension look like?
It would have to involve exchanging "I have/am interested in the paths
that match these pathspecs". I do not mind if our initial implementation
did not support anything other than fetching into a narrow from full and
pushing from a narrow to full. The second iteration could add fetching and
pushing between two narrows with the same set of narrowing pathspecs.
As to widening the area after a clone is made, I do not mind and if our
initial impementation only supported widening the area in a stupid way
(e.g. semi-equivalent of initial clone with the widened set of narrowing
pathspecs).
^ permalink raw reply
* Re: [PATCH] revert.c: defer writing CHERRY_PICK_HEAD till it is safe to do so
From: Jay Soffian @ 2011-10-06 17:21 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, nicolas.dichtel, Jeff King
In-Reply-To: <7vty7m12b2.fsf@alter.siamese.dyndns.org>
On Thu, Oct 6, 2011 at 1:02 PM, Junio C Hamano <gitster@pobox.com> wrote:
> I think do_recursive_merge() would die() when the merge cannot even start
> (i.e. the local changes after the cherry-pick exits are the ones from the
> time before such a failed cherry-pick started), but I suspect that the
> other codepath uses try_merge_command() to drive strategies other than
> recursive and it does not die() there in such a case. Can you make sure
> this patch is sufficient in such a case as well?
Why does recursive die with a dirty tree but not other strategies?
For that matter, why does revert.c have it's own implementation of
recursive instead of just calling try_merge_command("recursive", ...)?
j.
^ permalink raw reply
* [PATCH/RFC jn/ident-from-etc-mailname] ident: do not retrieve default ident when unnecessary
From: Jonathan Nieder @ 2011-10-06 17:17 UTC (permalink / raw)
To: Junio C Hamano
Cc: Johannes Sixt, git, Matt Kraai, Gerrit Pape, Ian Jackson,
Linus Torvalds
In-Reply-To: <7vty7pga7y.fsf@alter.siamese.dyndns.org>
Avoid a getpwuid() call (which contacts the network if the password
database is not local), read of /etc/mailname, gethostname() call, and
reverse DNS lookup if the user has already chosen a name and email
through configuration, the environment, or the command line.
This should slightly speed up commands like "git commit". More
importantly, it improves error reporting when computation of the
default ident string does not go smoothly. For example, after
detecting a problem (e.g., "warning: cannot open /etc/mailname:
Permission denied") in retrieving the default committer identity:
touch /etc/mailname; # as root
chmod -r /etc/mailname; # as root
git commit -m 'test commit'
you can squelch the warning while waiting for your sysadmin to fix the
permissions problem.
echo '[user] email = me@example.com' >>~/.gitconfig
Inspired-by: Johannes Sixt <j6t@kdgb.org>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
Junio C Hamano wrote:
> --- a/ident.c
> +++ b/ident.c
> @@ -239,6 +239,10 @@ const char *fmt_ident(const char *name, const char *email,
> int warn_on_no_name = (flag & IDENT_WARN_ON_NO_NAME);
> int name_addr_only = (flag & IDENT_NO_DATE);
>
> + if (name && !git_default_name[0])
> + strcpy(git_default_name, name);
> + if (email && !git_default_email[0])
> + strcpy(git_default_email, email);
That poisons the "git_default_foo" variables for the next fmt_ident
call. But something similar should work.
ident.c | 16 ++++++++--------
1 files changed, 8 insertions(+), 8 deletions(-)
diff --git a/ident.c b/ident.c
index edb43144..f619619b 100644
--- a/ident.c
+++ b/ident.c
@@ -117,19 +117,21 @@ static void copy_email(const struct passwd *pw)
sizeof(git_default_email) - len);
}
-static void setup_ident(void)
+static void setup_ident(const char **name, const char **emailp)
{
struct passwd *pw = NULL;
/* Get the name ("gecos") */
- if (!git_default_name[0]) {
+ if (!*name && !git_default_name[0]) {
pw = getpwuid(getuid());
if (!pw)
die("You don't exist. Go away!");
copy_gecos(pw, git_default_name, sizeof(git_default_name));
}
+ if (!*name)
+ *name = git_default_name;
- if (!git_default_email[0]) {
+ if (!*emailp && !git_default_email[0]) {
const char *email = getenv("EMAIL");
if (email && email[0]) {
@@ -144,6 +146,8 @@ static void setup_ident(void)
copy_email(pw);
}
}
+ if (!*emailp)
+ *emailp = git_default_email;
/* And set the default date */
if (!git_default_date[0])
@@ -239,11 +243,7 @@ const char *fmt_ident(const char *name, const char *email,
int warn_on_no_name = (flag & IDENT_WARN_ON_NO_NAME);
int name_addr_only = (flag & IDENT_NO_DATE);
- setup_ident();
- if (!name)
- name = git_default_name;
- if (!email)
- email = git_default_email;
+ setup_ident(&name, &email);
if (!*name) {
struct passwd *pw;
--
1.7.7.rc1
^ permalink raw reply related
* Re: [PATCH 2/4] cleanup: use internal memory allocation wrapper functions everywhere
From: Brandon Casey @ 2011-10-06 17:17 UTC (permalink / raw)
To: kusmabite
Cc: Johannes Sixt, peff@peff.net, git@vger.kernel.org,
gitster@pobox.com, sunshine@sunshineco.com, bharrosh@panasas.com,
trast@student.ethz.ch
In-Reply-To: <CABPQNSak_jDbNQhzMoSN=NdWmyby3xJRwED54Ck5H1Y12HoGCQ@mail.gmail.com>
On Thu, Oct 6, 2011 at 11:50 AM, Erik Faye-Lund <kusmabite@gmail.com> wrote:
> On Thu, Oct 6, 2011 at 6:14 PM, Brandon Casey <drafnel@gmail.com> wrote:
>> [removed Alexey Shumkin from cc]
>>
>> On Thu, Oct 6, 2011 at 1:17 AM, Johannes Sixt <j.sixt@viscovery.net> wrote:
>>> Am 10/6/2011 4:00, schrieb Brandon Casey:
>>>> [resend without html bits added by "gmail offline"]
>>>> On Wed, Oct 5, 2011 at 7:53 PM, Brandon Casey <drafnel@gmail.com> wrote:
>>>>> On Thursday, September 15, 2011, Brandon Casey wrote:
>>>>>>
>>>>>> On Thu, Sep 15, 2011 at 1:52 AM, Johannes Sixt <j.sixt@viscovery.net>
>>>>>>> There is a danger that the high-level die() routine (which is used by
>>>>>>> the
>>>>>>> x-wrappers) uses one of the low-level compat/ routines. IOW, in the case
>>>>>>> of errors, recursion might occur. Therefore, I would prefer that the
>>>>>>> compat/ routines do their own error reporting (preferably via return
>>>>>>> values and errno).
>>>>>>
>>>>>> Thanks. Will do.
>>>>>
>>>>> Hi Johannes,
>>>>> I have taken a closer look at the possibility of recursion with respect to
>>>>> die() and the functions in compat/. It appears the risk is only related to
>>>>> vsnprintf/snprintf at the moment. So as long as we avoid calling xmalloc et
>>>>> al from within snprintf.c, I think we should be safe from recursion.
>>>>> I'm inclined to keep the additions to mingw.c and win32/syslog.c since they
>>>>> both already use the x-wrappers or strbuf, even though they could easily be
>>>>> worked around. The other file that was touched is compat/qsort, which
>>>>> returns void and doesn't have a good alternative error handling path. So,
>>>>> I'm inclined to keep that one too.
>>>
>>> I'm fine with keeping the change to mingw.c (getaddrinfo related) and
>>> qsort: both are unlikely to be called from die().
>>>
>>> But syslog() *is* called from die() in git-daemon, and it would be better
>>> to back out the other offenders instead of adding to them.
>>
>> Ah. Yes, you're right. x-wrappers should not be used in syslog.c and
>> the use of strbuf's should be replaced.
>
> Good point. The patch for this looks something like this:
>
> diff --git a/compat/win32/syslog.c b/compat/win32/syslog.c
> index 42b95a9..243538c 100644
> --- a/compat/win32/syslog.c
> +++ b/compat/win32/syslog.c
> @@ -1,5 +1,4 @@
> #include "../../git-compat-util.h"
> -#include "../../strbuf.h"
>
> static HANDLE ms_eventlog;
>
> @@ -16,13 +15,8 @@ void openlog(const char *ident, int logopt, int facility)
>
> void syslog(int priority, const char *fmt, ...)
> {
> - struct strbuf sb = STRBUF_INIT;
> - struct strbuf_expand_dict_entry dict[] = {
> - {"1", "% 1"},
> - {NULL, NULL}
> - };
> WORD logtype;
> - char *str;
> + char *str, *pos;
> int str_len;
> va_list ap;
>
> @@ -39,11 +33,20 @@ void syslog(int priority, const char *fmt, ...)
> }
>
> str = malloc(str_len + 1);
> + if (!str)
> + return; /* no chance to report error */
> +
> va_start(ap, fmt);
> vsnprintf(str, str_len + 1, fmt, ap);
> va_end(ap);
> - strbuf_expand(&sb, str, strbuf_expand_dict_cb, &dict);
> - free(str);
> +
> + while ((pos = strstr(str, "%1")) != NULL) {
> + str = realloc(str, ++str_len + 1);
> + if (!str)
> + return;
> + memmove(pos + 2, pos + 1, strlen(pos));
> + pos[1] = ' ';
> + }
Is there any reason not to just use a different character than '%'
when replacing '%n'? Like '@'? Then the replacement could be done
in-place.
e.g. convert this:
fe80::3%1
into this:
fe80::3@1
I'm not usually on a windows platform, so maybe adding the space is
the common thing to do when trying to write an ipv6 address to the
event log using ReportEvent(). If not, then I think people would
probably figure out easily enough that '@n' referred to interface 'n'.
-Brandon
^ permalink raw reply
* Re: [PATCH] commit: teach --gpg-sign option
From: Jonathan Nieder @ 2011-10-06 17:11 UTC (permalink / raw)
To: Shawn Pearce; +Cc: Junio C Hamano, git
In-Reply-To: <CAJo=hJvWbjEM9E5AjPHgmQ=eY8xf=Q=xtukeu2Ur7auUqeabDg@mail.gmail.com>
Shawn Pearce wrote:
> On Wed, Oct 5, 2011 at 17:56, Junio C Hamano <gitster@pobox.com> wrote:
>> And this uses the gpg-interface.[ch] to allow signing the commit, i.e.
>>
>> $ git commit --gpg-sign -m foo
>> You need a passphrase to unlock the secret key for
>> user: "Junio C Hamano <gitster@pobox.com>"
>> 4096-bit RSA key, ID 96AFE6CB, created 2011-10-03 (main key ID 713660A7)
[...]
> I like this approach better than the prior "push certificate" idea.
> The signature information is part of the history graph
I probably missed some earlier discussion (so please forgive me this),
but how is it intended to be used? Would projects
a. require as a matter of policy that all commits be signed
b. just sign releases as usual, but as commits in the history graph
instead of tags
c. sign the occasional especially interesting commit
What happens if my old key is compromised and I want to throw away the
signatures and replace them with signatures using my new key? How
does this relate to the "push certificate" use case, which seemed to
be mostly about authenticating published branch tips with signatures
that are not necessarily important in the long term?
In other words, something like this feature sounds like a sensible way
to commit the equivalent of a GPG-signed patch, but it doesn't seem
like a good fit for the "push certificate" use cases.
^ permalink raw reply
* Re: [PATCH] revert.c: defer writing CHERRY_PICK_HEAD till it is safe to do so
From: Junio C Hamano @ 2011-10-06 17:02 UTC (permalink / raw)
To: Jay Soffian; +Cc: git, nicolas.dichtel, Jeff King
In-Reply-To: <1317911641-47660-1-git-send-email-jaysoffian@gmail.com>
Jay Soffian <jaysoffian@gmail.com> writes:
> do_pick_commit() writes out CHERRY_PICK_HEAD before invoking merge (either
> via do_recursive_merge() or try_merge_command()) on the assumption that if
> the merge fails it is due to conflict. However, if the tree is dirty, the
> merge may not even start, aborting before do_pick_commit() can remove
> CHERRY_PICK_HEAD.
>
> Instead, defer writing CHERRY_PICK_HEAD till after merge has returned.
> At this point we know the merge has either succeeded or failed due
> to conflict. In either case, we want CHERRY_PICK_HEAD to be written
> so that it may be picked up by the subsequent invocation of commit.
I think do_recursive_merge() would die() when the merge cannot even start
(i.e. the local changes after the cherry-pick exits are the ones from the
time before such a failed cherry-pick started), but I suspect that the
other codepath uses try_merge_command() to drive strategies other than
recursive and it does not die() there in such a case. Can you make sure
this patch is sufficient in such a case as well?
Other than that I think this is better than the previous rounds.
Thanks.
> Signed-off-by: Jay Soffian <jaysoffian@gmail.com>
> ---
> builtin/revert.c | 6 +++++-
> t/t3507-cherry-pick-conflict.sh | 7 +++++++
> 2 files changed, 12 insertions(+), 1 deletions(-)
>
> diff --git a/builtin/revert.c b/builtin/revert.c
> index 3117776c2c..48526e1ef1 100644
> --- a/builtin/revert.c
> +++ b/builtin/revert.c
> @@ -384,6 +384,7 @@ static int do_pick_commit(void)
> char *defmsg = NULL;
> struct strbuf msgbuf = STRBUF_INIT;
> int res;
> + int record_cherry_pick_head = 0;
>
> if (no_commit) {
> /*
> @@ -477,7 +478,7 @@ static int do_pick_commit(void)
> strbuf_addstr(&msgbuf, ")\n");
> }
> if (!no_commit)
> - write_cherry_pick_head();
> + record_cherry_pick_head = 1;
> }
>
> if (!strategy || !strcmp(strategy, "recursive") || action == REVERT) {
> @@ -498,6 +499,9 @@ static int do_pick_commit(void)
> free_commit_list(remotes);
> }
>
> + if (record_cherry_pick_head)
> + write_cherry_pick_head();
> +
> if (res) {
> error(action == REVERT
> ? _("could not revert %s... %s")
> diff --git a/t/t3507-cherry-pick-conflict.sh b/t/t3507-cherry-pick-conflict.sh
> index 212ec54aaf..29890bf5ac 100755
> --- a/t/t3507-cherry-pick-conflict.sh
> +++ b/t/t3507-cherry-pick-conflict.sh
> @@ -77,6 +77,13 @@ test_expect_success 'cherry-pick --no-commit does not set CHERRY_PICK_HEAD' '
> test_must_fail git rev-parse --verify CHERRY_PICK_HEAD
> '
>
> +test_expect_success 'cherry-pick w/dirty tree does not set CHERRY_PICK_HEAD' '
> + pristine_detach initial &&
> + echo foo > foo &&
> + test_must_fail git cherry-pick base &&
> + test_must_fail git rev-parse --verify CHERRY_PICK_HEAD
> +'
> +
> test_expect_success 'GIT_CHERRY_PICK_HELP suppresses CHERRY_PICK_HEAD' '
> pristine_detach initial &&
> (
^ permalink raw reply
* [PATCH] mingw: avoid using strbuf in syslog
From: Erik Faye-Lund @ 2011-10-06 16:57 UTC (permalink / raw)
To: git; +Cc: drafnel, j.sixt, gitster
strbuf can call die, which again can call syslog from git-daemon.
Endless recursion is no fun; fix it by hand-rolling the logic.
Signed-off-by: Erik Faye-Lund <kusmabite@gmail.com>
---
Fixes the problem Brandon pointed out...
compat/win32/syslog.c | 26 ++++++++++++++------------
1 files changed, 14 insertions(+), 12 deletions(-)
diff --git a/compat/win32/syslog.c b/compat/win32/syslog.c
index 42b95a9..3b8e2b7 100644
--- a/compat/win32/syslog.c
+++ b/compat/win32/syslog.c
@@ -1,5 +1,4 @@
#include "../../git-compat-util.h"
-#include "../../strbuf.h"
static HANDLE ms_eventlog;
@@ -16,13 +15,8 @@ void openlog(const char *ident, int logopt, int facility)
void syslog(int priority, const char *fmt, ...)
{
- struct strbuf sb = STRBUF_INIT;
- struct strbuf_expand_dict_entry dict[] = {
- {"1", "% 1"},
- {NULL, NULL}
- };
WORD logtype;
- char *str;
+ char *str, *pos;
int str_len;
va_list ap;
@@ -39,11 +33,20 @@ void syslog(int priority, const char *fmt, ...)
}
str = malloc(str_len + 1);
+ if (!str)
+ return; /* no chance to report error */
+
va_start(ap, fmt);
vsnprintf(str, str_len + 1, fmt, ap);
va_end(ap);
- strbuf_expand(&sb, str, strbuf_expand_dict_cb, &dict);
- free(str);
+
+ while ((pos = strstr(str, "%1")) != NULL) {
+ str = realloc(str, ++str_len + 1);
+ if (!str)
+ return;
+ memmove(pos + 2, pos + 1, strlen(pos));
+ pos[1] = ' ';
+ }
switch (priority) {
case LOG_EMERG:
@@ -66,7 +69,6 @@ void syslog(int priority, const char *fmt, ...)
}
ReportEventA(ms_eventlog, logtype, 0, 0, NULL, 1, 0,
- (const char **)&sb.buf, NULL);
-
- strbuf_release(&sb);
+ (const char **)&str, NULL);
+ free(str);
}
--
1.7.6.msysgit.0.579.ga3d6f
^ permalink raw reply related
* [PATCH 1/2] fetch: free all the additional refspecs
From: Carlos Martín Nieto @ 2011-10-06 16:56 UTC (permalink / raw)
To: git
In-Reply-To: <1317920187-17389-1-git-send-email-cmn@elego.de>
Signed-off-by: Carlos Martín Nieto <cmn@elego.de>
---
builtin/fetch.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/builtin/fetch.c b/builtin/fetch.c
index 7a4e41c..30b485e 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -883,7 +883,7 @@ static int fetch_one(struct remote *remote, int argc, const char **argv)
atexit(unlock_pack);
refspec = parse_fetch_refspec(ref_nr, refs);
exit_code = do_fetch(transport, refspec, ref_nr);
- free(refspec);
+ free_refspec(ref_nr, refspec);
transport_disconnect(transport);
transport = NULL;
return exit_code;
--
1.7.5.2.354.g349bf
^ permalink raw reply related
* [PATCH 2/2] fetch: honor the user-provided refspecs when pruning refs
From: Carlos Martín Nieto @ 2011-10-06 16:56 UTC (permalink / raw)
To: git
In-Reply-To: <1317920187-17389-1-git-send-email-cmn@elego.de>
If the user gave us refspecs on the command line, we should use those
when deciding whether to prune a ref instead of relying on the
refspecs in the config.
Previously, running
git fetch --prune origin refs/heads/master:refs/remotes/origin/master
would delete every other tag under the origin namespace because we
were using the refspec to filter the available refs but using the
configured refspec to figure out if a ref had been deleted on the
remote. This is clearly the wrong thing to do.
Teach get_stale_heads about user-provided refspecs and use them if
they're available.
Signed-off-by: Carlos Martín Nieto <cmn@elego.de>
---
builtin/fetch.c | 6 ++--
builtin/remote.c | 2 +-
remote.c | 74 ++++++++++++++++++++++++++++++++++++++++++++++++-----
remote.h | 3 +-
4 files changed, 73 insertions(+), 12 deletions(-)
diff --git a/builtin/fetch.c b/builtin/fetch.c
index 30b485e..b937d71 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -505,10 +505,10 @@ static int fetch_refs(struct transport *transport, struct ref *ref_map)
return ret;
}
-static int prune_refs(struct transport *transport, struct ref *ref_map)
+static int prune_refs(struct transport *transport, struct refspec *refs, int n, struct ref *ref_map)
{
int result = 0;
- struct ref *ref, *stale_refs = get_stale_heads(transport->remote, ref_map);
+ struct ref *ref, *stale_refs = get_stale_heads(transport->remote, ref_map, refs, n);
const char *dangling_msg = dry_run
? _(" (%s will become dangling)\n")
: _(" (%s has become dangling)\n");
@@ -700,7 +700,7 @@ static int do_fetch(struct transport *transport,
return 1;
}
if (prune)
- prune_refs(transport, ref_map);
+ prune_refs(transport, refs, ref_count, ref_map);
free_refs(ref_map);
/* if neither --no-tags nor --tags was specified, do automated tag
diff --git a/builtin/remote.c b/builtin/remote.c
index b25dfb4..91a2148 100644
--- a/builtin/remote.c
+++ b/builtin/remote.c
@@ -349,7 +349,7 @@ static int get_ref_states(const struct ref *remote_refs, struct ref_states *stat
else
string_list_append(&states->tracked, abbrev_branch(ref->name));
}
- stale_refs = get_stale_heads(states->remote, fetch_map);
+ stale_refs = get_stale_heads(states->remote, fetch_map, NULL, 0);
for (ref = stale_refs; ref; ref = ref->next) {
struct string_list_item *item =
string_list_append(&states->stale, abbrev_branch(ref->name));
diff --git a/remote.c b/remote.c
index 7840d2f..72a26d3 100644
--- a/remote.c
+++ b/remote.c
@@ -1684,26 +1684,84 @@ struct stale_heads_info {
struct remote *remote;
struct string_list *ref_names;
struct ref **stale_refs_tail;
+ struct refspec *refs;
+ int ref_count;
};
+/*
+ * Find a refspec to a remote's
+ * Returns 0 on success, -1 if it couldn't find a the refspec
+ */
+static int find_in_refs(struct refspec *refs, int ref_count, struct refspec *query)
+{
+ int i;
+ struct refspec *refspec;
+
+ for (i = 0; i < ref_count; ++i) {
+ refspec = &refs[i];
+
+ /*
+ * No '*' means that it must match exactly. If it does
+ * have it, try to match it against the pattern. If
+ * the refspec matches, store the ref name as it would
+ * appear in the server in query->src.
+ */
+ if (!strchr(refspec->dst, '*')) {
+ if (!strcmp(query->dst, refspec->dst)) {
+ query->src = xstrdup(refspec->src);
+ return 0;
+ }
+ } else {
+ if (match_name_with_pattern(refspec->dst, query->dst,
+ refspec->src, &query->src)) {
+ return 0;
+ }
+ }
+ }
+
+ return -1;
+}
+
static int get_stale_heads_cb(const char *refname,
const unsigned char *sha1, int flags, void *cb_data)
{
struct stale_heads_info *info = cb_data;
struct refspec refspec;
+ int ret;
memset(&refspec, 0, sizeof(refspec));
refspec.dst = (char *)refname;
- if (!remote_find_tracking(info->remote, &refspec)) {
- if (!((flags & REF_ISSYMREF) ||
- string_list_has_string(info->ref_names, refspec.src))) {
- struct ref *ref = make_linked_ref(refname, &info->stale_refs_tail);
- hashcpy(ref->new_sha1, sha1);
- }
+
+ /*
+ * If the user speicified refspecs on the command line, we
+ * should only use those to check. Otherwise, look in the
+ * remote's configuration for the branch.
+ */
+ if (info->ref_count)
+ ret = find_in_refs(info->refs, info->ref_count, &refspec);
+ else
+ ret = remote_find_tracking(info->remote, &refspec);
+
+ /* No matches */
+ if (ret)
+ return 0;
+
+ /*
+ * If we did find a suitable refspec and it's not a symref and
+ * it's not in the list of refs that currently exist in that
+ * remote we consider it to be stale.
+ */
+ if (!((flags & REF_ISSYMREF) ||
+ string_list_has_string(info->ref_names, refspec.src))) {
+ struct ref *ref = make_linked_ref(refname, &info->stale_refs_tail);
+ hashcpy(ref->new_sha1, sha1);
}
+
+ free(refspec.src);
return 0;
}
-struct ref *get_stale_heads(struct remote *remote, struct ref *fetch_map)
+struct ref *get_stale_heads(struct remote *remote, struct ref *fetch_map,
+ struct refspec *refs, int ref_count)
{
struct ref *ref, *stale_refs = NULL;
struct string_list ref_names = STRING_LIST_INIT_NODUP;
@@ -1711,6 +1769,8 @@ struct ref *get_stale_heads(struct remote *remote, struct ref *fetch_map)
info.remote = remote;
info.ref_names = &ref_names;
info.stale_refs_tail = &stale_refs;
+ info.refs = refs;
+ info.ref_count = ref_count;
for (ref = fetch_map; ref; ref = ref->next)
string_list_append(&ref_names, ref->name);
sort_string_list(&ref_names);
diff --git a/remote.h b/remote.h
index 9a30a9d..2f753a0 100644
--- a/remote.h
+++ b/remote.h
@@ -164,6 +164,7 @@ struct ref *guess_remote_head(const struct ref *head,
int all);
/* Return refs which no longer exist on remote */
-struct ref *get_stale_heads(struct remote *remote, struct ref *fetch_map);
+struct ref *get_stale_heads(struct remote *remote, struct ref *fetch_map,
+ struct refspec *refs, int ref_count);
#endif
--
1.7.5.2.354.g349bf
^ permalink raw reply related
* [WIP PATCH 0/2] Be more careful when prunning
From: Carlos Martín Nieto @ 2011-10-06 16:56 UTC (permalink / raw)
To: git
In-Reply-To: <20111004103624.GA11863@sigill.intra.peff.net>
[I turns out that my system was slightly misconfigured, so vger didn't
want to accept my mail, this a re-send to the list only]
Hello,
I consider this WIP because I haven't addressed the issue when --tags
is given. It's likely the most common issue (and what sparked this),
but the second patch sets it up so it becomes easier.
The first patch is not that big a deal, but it's better if we're
freeing the refspecs, we might as well free all of them.
The second patch teaches get_stale_heads to use the user-provided
refspecs instead of the ones in the config. For example, running
git fetch --prune origin refs/heads/master:refs/heads/master
doesn't remove the other branches anymore. For a more interesting (and
believable) example, let's take
git fetch --prune origin refs/heads/b/*:refs/heads/b/*
because you want to prune the refs inside the b/ namespace
only. Currently git will delete all the refs that aren't under that
namespace. With the second patch applied, git won't remove any refs
outside the b/ namespace.
Now comes the interesting part: when --tags is given, there is no
refspec set up, fetch just sets up a global variable. What I'm
thinking (and going to implement after dinner, unless people cry out
against it) is this: just before calling prune_refs, add a refspec to
the user-provided list with the refspec refs/tags/*:refs/tags/*
similar to what fetch_one does if you gave it "tag v1.5.6". This would
cause us to ignore the configured refspec and keep the branches. The
lack of '+' is most certainly on purpose. Is there anything
fundamentally wrong with that idea?
Cheers,
cmn
Carlos Martín Nieto (2):
fetch: free all the additional refspecs
fetch: honor the user-provided refspecs when pruning refs
builtin/fetch.c | 8 +++---
builtin/remote.c | 2 +-
remote.c | 74 ++++++++++++++++++++++++++++++++++++++++++++++++-----
remote.h | 3 +-
4 files changed, 74 insertions(+), 13 deletions(-)
--
1.7.5.2.354.g349bf
^ permalink raw reply
* Re: Git Bug report
From: Phil Hord @ 2011-10-06 16:54 UTC (permalink / raw)
To: Junio C Hamano
Cc: SZEDER Gábor, git, Fredrik Gustafsson, Federico Lucifredi
In-Reply-To: <7vy5wy145q.fsf@alter.siamese.dyndns.org>
On Thu, Oct 6, 2011 at 12:22 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Phil Hord <phil.hord@gmail.com> writes:
>
>> On Oct 5, 2011 9:14 PM, "SZEDER Gábor" <szeder@ira.uka.de> wrote:
>>>
>>> On Wed, Oct 05, 2011 at 05:44:54PM -0700, Junio C Hamano wrote:
>>> > SZEDER Gábor <szeder@ira.uka.de> writes:
>>> >
>>> > > And what about unreadable .git files?
>>> >
>>> > Having then inside a working tree is so sick that I do not think it
>>> > deserves consideration.
>>>
>>> I'm not sure why is this any different than having a .git directory
>>> that is not a repository inside a working tree.
>>
>> What should happen here? Ignore it and keep searching? Or fail?
>>
>> I just added some common gitfile detection code and I noticed that the
>> oddball case now is the one that dies on error rather than continuing to
>> search for alternate explanations. I left the oddball behavior assuming it
>> is desireable, but now you have me rethinking it.
>
> Yeah, after thinking about it a bit more, whenever we see ".git" during
> the upward discovery process, we should always warn if we know it is _not_
> a GIT_DIR before looking for another ".git" at higher levels, as anything
> in that directory cannot be added. If we cannot tell if it is or is not
> a GIT_DIR, we should error out---the reason we cannot tell most likely is
> because we cannot read it, and such a file, if it is not a GIT_DIR, cannot
> be tracked in the real GIT_DIR at a higher level, and if it is a GIT_DIR,
> we cannot use it to record updates or inspect existing history.
>
> How's that sound as a guideline?
Ok. Three cases, then:
if .git is valid, we use it.
If .git is bogus, we warn about it and keep searching.
If .git is unverifiable (permissions, IO-fail, etc.), we die.
Phil
^ permalink raw reply
* Re: [PATCH 2/4] cleanup: use internal memory allocation wrapper functions everywhere
From: Erik Faye-Lund @ 2011-10-06 16:52 UTC (permalink / raw)
To: Brandon Casey
Cc: Johannes Sixt, peff@peff.net, git@vger.kernel.org,
gitster@pobox.com, sunshine@sunshineco.com, bharrosh@panasas.com,
trast@student.ethz.ch
In-Reply-To: <CABPQNSak_jDbNQhzMoSN=NdWmyby3xJRwED54Ck5H1Y12HoGCQ@mail.gmail.com>
On Thu, Oct 6, 2011 at 6:50 PM, Erik Faye-Lund <kusmabite@gmail.com> wrote:
> On Thu, Oct 6, 2011 at 6:14 PM, Brandon Casey <drafnel@gmail.com> wrote:
>> Ah. Yes, you're right. x-wrappers should not be used in syslog.c and
>> the use of strbuf's should be replaced.
>
> Good point. The patch for this looks something like this:
>
> diff --git a/compat/win32/syslog.c b/compat/win32/syslog.c
> index 42b95a9..243538c 100644
> --- a/compat/win32/syslog.c
> +++ b/compat/win32/syslog.c
> @@ -1,5 +1,4 @@
> #include "../../git-compat-util.h"
> -#include "../../strbuf.h"
>
> static HANDLE ms_eventlog;
>
> @@ -16,13 +15,8 @@ void openlog(const char *ident, int logopt, int facility)
>
> void syslog(int priority, const char *fmt, ...)
> {
> - struct strbuf sb = STRBUF_INIT;
> - struct strbuf_expand_dict_entry dict[] = {
> - {"1", "% 1"},
> - {NULL, NULL}
> - };
> WORD logtype;
> - char *str;
> + char *str, *pos;
> int str_len;
> va_list ap;
>
> @@ -39,11 +33,20 @@ void syslog(int priority, const char *fmt, ...)
> }
>
> str = malloc(str_len + 1);
> + if (!str)
> + return; /* no chance to report error */
> +
> va_start(ap, fmt);
> vsnprintf(str, str_len + 1, fmt, ap);
> va_end(ap);
> - strbuf_expand(&sb, str, strbuf_expand_dict_cb, &dict);
> - free(str);
> +
> + while ((pos = strstr(str, "%1")) != NULL) {
> + str = realloc(str, ++str_len + 1);
> + if (!str)
> + return;
> + memmove(pos + 2, pos + 1, strlen(pos));
> + pos[1] = ' ';
> + }
>
> switch (priority) {
> case LOG_EMERG:
> @@ -66,7 +69,5 @@ void syslog(int priority, const char *fmt, ...)
> }
>
> ReportEventA(ms_eventlog, logtype, 0, 0, NULL, 1, 0,
> - (const char **)&sb.buf, NULL);
> -
> - strbuf_release(&sb);
> + (const char **)&str, NULL);
> }
>
...aaand this on top to avoid a leak, of course:
diff --git a/compat/win32/syslog.c b/compat/win32/syslog.c
index 243538c..3b8e2b7 100644
--- a/compat/win32/syslog.c
+++ b/compat/win32/syslog.c
@@ -70,4 +70,5 @@ void syslog(int priority, const char *fmt, ...)
ReportEventA(ms_eventlog, logtype, 0, 0, NULL, 1, 0,
(const char **)&str, NULL);
+ free(str);
}
^ permalink raw reply related
* Re: [PATCH 2/4] cleanup: use internal memory allocation wrapper functions everywhere
From: Erik Faye-Lund @ 2011-10-06 16:50 UTC (permalink / raw)
To: Brandon Casey
Cc: Johannes Sixt, peff@peff.net, git@vger.kernel.org,
gitster@pobox.com, sunshine@sunshineco.com, bharrosh@panasas.com,
trast@student.ethz.ch
In-Reply-To: <CA+sFfMf8_7ccC9kjEq=2NrehVgS=ahnQA8VibEF10VaULot7=A@mail.gmail.com>
On Thu, Oct 6, 2011 at 6:14 PM, Brandon Casey <drafnel@gmail.com> wrote:
> [removed Alexey Shumkin from cc]
>
> On Thu, Oct 6, 2011 at 1:17 AM, Johannes Sixt <j.sixt@viscovery.net> wrote:
>> Am 10/6/2011 4:00, schrieb Brandon Casey:
>>> [resend without html bits added by "gmail offline"]
>>> On Wed, Oct 5, 2011 at 7:53 PM, Brandon Casey <drafnel@gmail.com> wrote:
>>>> On Thursday, September 15, 2011, Brandon Casey wrote:
>>>>>
>>>>> On Thu, Sep 15, 2011 at 1:52 AM, Johannes Sixt <j.sixt@viscovery.net>
>>>>>> There is a danger that the high-level die() routine (which is used by
>>>>>> the
>>>>>> x-wrappers) uses one of the low-level compat/ routines. IOW, in the case
>>>>>> of errors, recursion might occur. Therefore, I would prefer that the
>>>>>> compat/ routines do their own error reporting (preferably via return
>>>>>> values and errno).
>>>>>
>>>>> Thanks. Will do.
>>>>
>>>> Hi Johannes,
>>>> I have taken a closer look at the possibility of recursion with respect to
>>>> die() and the functions in compat/. It appears the risk is only related to
>>>> vsnprintf/snprintf at the moment. So as long as we avoid calling xmalloc et
>>>> al from within snprintf.c, I think we should be safe from recursion.
>>>> I'm inclined to keep the additions to mingw.c and win32/syslog.c since they
>>>> both already use the x-wrappers or strbuf, even though they could easily be
>>>> worked around. The other file that was touched is compat/qsort, which
>>>> returns void and doesn't have a good alternative error handling path. So,
>>>> I'm inclined to keep that one too.
>>
>> I'm fine with keeping the change to mingw.c (getaddrinfo related) and
>> qsort: both are unlikely to be called from die().
>>
>> But syslog() *is* called from die() in git-daemon, and it would be better
>> to back out the other offenders instead of adding to them.
>
> Ah. Yes, you're right. x-wrappers should not be used in syslog.c and
> the use of strbuf's should be replaced.
Good point. The patch for this looks something like this:
diff --git a/compat/win32/syslog.c b/compat/win32/syslog.c
index 42b95a9..243538c 100644
--- a/compat/win32/syslog.c
+++ b/compat/win32/syslog.c
@@ -1,5 +1,4 @@
#include "../../git-compat-util.h"
-#include "../../strbuf.h"
static HANDLE ms_eventlog;
@@ -16,13 +15,8 @@ void openlog(const char *ident, int logopt, int facility)
void syslog(int priority, const char *fmt, ...)
{
- struct strbuf sb = STRBUF_INIT;
- struct strbuf_expand_dict_entry dict[] = {
- {"1", "% 1"},
- {NULL, NULL}
- };
WORD logtype;
- char *str;
+ char *str, *pos;
int str_len;
va_list ap;
@@ -39,11 +33,20 @@ void syslog(int priority, const char *fmt, ...)
}
str = malloc(str_len + 1);
+ if (!str)
+ return; /* no chance to report error */
+
va_start(ap, fmt);
vsnprintf(str, str_len + 1, fmt, ap);
va_end(ap);
- strbuf_expand(&sb, str, strbuf_expand_dict_cb, &dict);
- free(str);
+
+ while ((pos = strstr(str, "%1")) != NULL) {
+ str = realloc(str, ++str_len + 1);
+ if (!str)
+ return;
+ memmove(pos + 2, pos + 1, strlen(pos));
+ pos[1] = ' ';
+ }
switch (priority) {
case LOG_EMERG:
@@ -66,7 +69,5 @@ void syslog(int priority, const char *fmt, ...)
}
ReportEventA(ms_eventlog, logtype, 0, 0, NULL, 1, 0,
- (const char **)&sb.buf, NULL);
-
- strbuf_release(&sb);
+ (const char **)&str, NULL);
}
^ permalink raw reply related
* [PATCH 7/7] pickaxe: factor out pickaxe
From: René Scharfe @ 2011-10-06 16:50 UTC (permalink / raw)
To: Git Mailing List; +Cc: Junio C Hamano
In-Reply-To: <4E8DD065.3040607@lsrfire.ath.cx>
Move the duplicate diff queue loop into its own function that accepts
a match function: has_changes() for -S and diff_grep() for -G.
Signed-off-by: Rene Scharfe <rene.scharfe@lsrfire.ath.cx>
---
diffcore-pickaxe.c | 110 ++++++++++++++++++++-------------------------------
1 files changed, 43 insertions(+), 67 deletions(-)
diff --git a/diffcore-pickaxe.c b/diffcore-pickaxe.c
index 226fa0c..380a837 100644
--- a/diffcore-pickaxe.c
+++ b/diffcore-pickaxe.c
@@ -8,6 +8,46 @@
#include "xdiff-interface.h"
#include "kwset.h"
+typedef int (*pickaxe_fn)(struct diff_filepair *p, struct diff_options *o, regex_t *regexp, kwset_t kws);
+
+static void pickaxe(struct diff_queue_struct *q, struct diff_options *o,
+ regex_t *regexp, kwset_t kws, pickaxe_fn fn)
+{
+ int i;
+ struct diff_queue_struct outq;
+
+ DIFF_QUEUE_CLEAR(&outq);
+
+ if (o->pickaxe_opts & DIFF_PICKAXE_ALL) {
+ /* Showing the whole changeset if needle exists */
+ for (i = 0; i < q->nr; i++) {
+ struct diff_filepair *p = q->queue[i];
+ if (fn(p, o, regexp, kws))
+ return; /* do not munge the queue */
+ }
+
+ /*
+ * Otherwise we will clear the whole queue by copying
+ * the empty outq at the end of this function, but
+ * first clear the current entries in the queue.
+ */
+ for (i = 0; i < q->nr; i++)
+ diff_free_filepair(q->queue[i]);
+ } else {
+ /* Showing only the filepairs that has the needle */
+ for (i = 0; i < q->nr; i++) {
+ struct diff_filepair *p = q->queue[i];
+ if (fn(p, o, regexp, kws))
+ diff_q(&outq, p);
+ else
+ diff_free_filepair(p);
+ }
+ }
+
+ free(q->queue);
+ *q = outq;
+}
+
struct diffgrep_cb {
regex_t *regexp;
int hit;
@@ -96,12 +136,8 @@ static int diff_grep(struct diff_filepair *p, struct diff_options *o,
static void diffcore_pickaxe_grep(struct diff_options *o)
{
- struct diff_queue_struct *q = &diff_queued_diff;
- int i, err;
+ int err;
regex_t regex;
- struct diff_queue_struct outq;
- outq.queue = NULL;
- outq.nr = outq.alloc = 0;
err = regcomp(®ex, o->pickaxe, REG_EXTENDED | REG_NEWLINE);
if (err) {
@@ -111,36 +147,8 @@ static void diffcore_pickaxe_grep(struct diff_options *o)
die("invalid log-grep regex: %s", errbuf);
}
- if (o->pickaxe_opts & DIFF_PICKAXE_ALL) {
- /* Showing the whole changeset if needle exists */
- for (i = 0; i < q->nr; i++) {
- struct diff_filepair *p = q->queue[i];
- if (diff_grep(p, o, ®ex, NULL))
- goto out; /* do not munge the queue */
- }
+ pickaxe(&diff_queued_diff, o, ®ex, NULL, diff_grep);
- /*
- * Otherwise we will clear the whole queue by copying
- * the empty outq at the end of this function, but
- * first clear the current entries in the queue.
- */
- for (i = 0; i < q->nr; i++)
- diff_free_filepair(q->queue[i]);
- } else {
- /* Showing only the filepairs that has the needle */
- for (i = 0; i < q->nr; i++) {
- struct diff_filepair *p = q->queue[i];
- if (diff_grep(p, o, ®ex, NULL))
- diff_q(&outq, p);
- else
- diff_free_filepair(p);
- }
- }
-
- free(q->queue);
- *q = outq;
-
- out:
regfree(®ex);
return;
}
@@ -213,13 +221,9 @@ static void diffcore_pickaxe_count(struct diff_options *o)
{
const char *needle = o->pickaxe;
int opts = o->pickaxe_opts;
- struct diff_queue_struct *q = &diff_queued_diff;
unsigned long len = strlen(needle);
- int i;
regex_t regex, *regexp = NULL;
kwset_t kws = NULL;
- struct diff_queue_struct outq;
- DIFF_QUEUE_CLEAR(&outq);
if (opts & DIFF_PICKAXE_REGEX) {
int err;
@@ -238,36 +242,8 @@ static void diffcore_pickaxe_count(struct diff_options *o)
kwsprep(kws);
}
- if (opts & DIFF_PICKAXE_ALL) {
- /* Showing the whole changeset if needle exists */
- for (i = 0; i < q->nr; i++) {
- struct diff_filepair *p = q->queue[i];
- if (has_changes(p, o, regexp, kws))
- goto out; /* do not munge the queue */
- }
-
- /* otherwise we will clear the whole queue
- * by copying the empty outq at the end of this
- * function, but first clear the current entries
- * in the queue.
- */
- for (i = 0; i < q->nr; i++)
- diff_free_filepair(q->queue[i]);
- }
- else
- /* Showing only the filepairs that has the needle */
- for (i = 0; i < q->nr; i++) {
- struct diff_filepair *p = q->queue[i];
- if (has_changes(p, o, regexp, kws))
- diff_q(&outq, p);
- else
- diff_free_filepair(p);
- }
-
- free(q->queue);
- *q = outq;
+ pickaxe(&diff_queued_diff, o, regexp, kws, has_changes);
- out:
if (opts & DIFF_PICKAXE_REGEX)
regfree(®ex);
else
--
1.7.7
^ permalink raw reply related
* [PATCH 6/7] pickaxe: give diff_grep the same signature as has_changes
From: René Scharfe @ 2011-10-06 16:50 UTC (permalink / raw)
To: Git Mailing List; +Cc: Junio C Hamano
In-Reply-To: <4E8DD065.3040607@lsrfire.ath.cx>
Change diff_grep() to match the signature of has_changes() as a
preparation for the next patch that will use function pointers to
the two.
Signed-off-by: Rene Scharfe <rene.scharfe@lsrfire.ath.cx>
---
diffcore-pickaxe.c | 7 ++++---
1 files changed, 4 insertions(+), 3 deletions(-)
diff --git a/diffcore-pickaxe.c b/diffcore-pickaxe.c
index 4d66ba9..226fa0c 100644
--- a/diffcore-pickaxe.c
+++ b/diffcore-pickaxe.c
@@ -45,7 +45,8 @@ static void fill_one(struct diff_filespec *one,
}
}
-static int diff_grep(struct diff_filepair *p, regex_t *regexp, struct diff_options *o)
+static int diff_grep(struct diff_filepair *p, struct diff_options *o,
+ regex_t *regexp, kwset_t kws)
{
regmatch_t regmatch;
struct userdiff_driver *textconv_one = NULL;
@@ -114,7 +115,7 @@ static void diffcore_pickaxe_grep(struct diff_options *o)
/* Showing the whole changeset if needle exists */
for (i = 0; i < q->nr; i++) {
struct diff_filepair *p = q->queue[i];
- if (diff_grep(p, ®ex, o))
+ if (diff_grep(p, o, ®ex, NULL))
goto out; /* do not munge the queue */
}
@@ -129,7 +130,7 @@ static void diffcore_pickaxe_grep(struct diff_options *o)
/* Showing only the filepairs that has the needle */
for (i = 0; i < q->nr; i++) {
struct diff_filepair *p = q->queue[i];
- if (diff_grep(p, ®ex, o))
+ if (diff_grep(p, o, ®ex, NULL))
diff_q(&outq, p);
else
diff_free_filepair(p);
--
1.7.7
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox