* 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
* [PATCH 5/7] pickaxe: pass diff_options to contains and 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>
Remove the unused parameter needle from contains() and has_changes().
Also replace the parameter len with a pointer to the diff_options. We
can use its member pickaxe to check if the needle is an empty string
and use the kwsmatch structure to find out the length of the match
instead.
This change is done as a preparation to unify the signatures of
has_changes() and diff_grep(), which will be used in the patch after
the next one to factor out common code.
Signed-off-by: Rene Scharfe <rene.scharfe@lsrfire.ath.cx>
---
diffcore-pickaxe.c | 28 ++++++++++++++--------------
1 files changed, 14 insertions(+), 14 deletions(-)
diff --git a/diffcore-pickaxe.c b/diffcore-pickaxe.c
index 4347ec1..4d66ba9 100644
--- a/diffcore-pickaxe.c
+++ b/diffcore-pickaxe.c
@@ -144,14 +144,13 @@ static void diffcore_pickaxe_grep(struct diff_options *o)
return;
}
-static unsigned int contains(struct diff_filespec *one,
- const char *needle, unsigned long len,
+static unsigned int contains(struct diff_filespec *one, struct diff_options *o,
regex_t *regexp, kwset_t kws)
{
unsigned int cnt;
unsigned long sz;
const char *data;
- if (!len)
+ if (!o->pickaxe[0])
return 0;
if (diff_populate_filespec(one, 0))
return 0;
@@ -175,14 +174,15 @@ static unsigned int contains(struct diff_filespec *one,
} else { /* Classic exact string match */
while (sz) {
- size_t offset = kwsexec(kws, data, sz, NULL);
+ struct kwsmatch kwsm;
+ size_t offset = kwsexec(kws, data, sz, &kwsm);
const char *found;
if (offset == -1)
break;
else
found = data + offset;
- sz -= found - data + len;
- data = found + len;
+ sz -= found - data + kwsm.size[0];
+ data = found + kwsm.size[0];
cnt++;
}
}
@@ -190,20 +190,20 @@ static unsigned int contains(struct diff_filespec *one,
return cnt;
}
-static int has_changes(struct diff_filepair *p, const char *needle,
- unsigned long len, regex_t *regexp, kwset_t kws)
+static int has_changes(struct diff_filepair *p, struct diff_options *o,
+ regex_t *regexp, kwset_t kws)
{
if (!DIFF_FILE_VALID(p->one)) {
if (!DIFF_FILE_VALID(p->two))
return 0; /* ignore unmerged */
/* created */
- return contains(p->two, needle, len, regexp, kws) != 0;
+ return contains(p->two, o, regexp, kws) != 0;
}
if (!DIFF_FILE_VALID(p->two))
- return contains(p->one, needle, len, regexp, kws) != 0;
+ return contains(p->one, o, regexp, kws) != 0;
if (!diff_unmodified_pair(p)) {
- return contains(p->one, needle, len, regexp, kws) !=
- contains(p->two, needle, len, regexp, kws);
+ return contains(p->one, o, regexp, kws) !=
+ contains(p->two, o, regexp, kws);
}
return 0;
}
@@ -241,7 +241,7 @@ static void diffcore_pickaxe_count(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 (has_changes(p, needle, len, regexp, kws))
+ if (has_changes(p, o, regexp, kws))
goto out; /* do not munge the queue */
}
@@ -257,7 +257,7 @@ static void diffcore_pickaxe_count(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 (has_changes(p, needle, len, regexp, kws))
+ if (has_changes(p, o, regexp, kws))
diff_q(&outq, p);
else
diff_free_filepair(p);
--
1.7.7
^ permalink raw reply related
* Re: Git Bug report
From: Phil Hord @ 2011-10-06 16:48 UTC (permalink / raw)
To: SZEDER Gábor
Cc: Junio C Hamano, Fredrik Gustafsson, Federico Lucifredi, git
In-Reply-To: <20111006010940.GR2208@goldbirke>
On 10/5/11, 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.
Phil
^ permalink raw reply
* [PATCH 4/7] pickaxe: factor out has_changes
From: René Scharfe @ 2011-10-06 16:26 UTC (permalink / raw)
To: Git Mailing List; +Cc: Junio C Hamano
In-Reply-To: <4E8DD065.3040607@lsrfire.ath.cx>
Move duplicate if/else construct into its own helper function.
Signed-off-by: Rene Scharfe <rene.scharfe@lsrfire.ath.cx>
---
diffcore-pickaxe.c | 57 +++++++++++++++++++--------------------------------
1 files changed, 21 insertions(+), 36 deletions(-)
diff --git a/diffcore-pickaxe.c b/diffcore-pickaxe.c
index c367d8d..4347ec1 100644
--- a/diffcore-pickaxe.c
+++ b/diffcore-pickaxe.c
@@ -190,13 +190,31 @@ static unsigned int contains(struct diff_filespec *one,
return cnt;
}
+static int has_changes(struct diff_filepair *p, const char *needle,
+ unsigned long len, regex_t *regexp, kwset_t kws)
+{
+ if (!DIFF_FILE_VALID(p->one)) {
+ if (!DIFF_FILE_VALID(p->two))
+ return 0; /* ignore unmerged */
+ /* created */
+ return contains(p->two, needle, len, regexp, kws) != 0;
+ }
+ if (!DIFF_FILE_VALID(p->two))
+ return contains(p->one, needle, len, regexp, kws) != 0;
+ if (!diff_unmodified_pair(p)) {
+ return contains(p->one, needle, len, regexp, kws) !=
+ contains(p->two, needle, len, regexp, kws);
+ }
+ return 0;
+}
+
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, has_changes;
+ int i;
regex_t regex, *regexp = NULL;
kwset_t kws = NULL;
struct diff_queue_struct outq;
@@ -223,22 +241,7 @@ static void diffcore_pickaxe_count(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_FILE_VALID(p->one)) {
- if (!DIFF_FILE_VALID(p->two))
- continue; /* ignore unmerged */
- /* created */
- if (contains(p->two, needle, len, regexp, kws))
- has_changes++;
- }
- else if (!DIFF_FILE_VALID(p->two)) {
- if (contains(p->one, needle, len, regexp, kws))
- has_changes++;
- }
- else if (!diff_unmodified_pair(p) &&
- contains(p->one, needle, len, regexp, kws) !=
- contains(p->two, needle, len, regexp, kws))
- has_changes++;
- if (has_changes)
+ if (has_changes(p, needle, len, regexp, kws))
goto out; /* do not munge the queue */
}
@@ -254,25 +257,7 @@ static void diffcore_pickaxe_count(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];
- has_changes = 0;
- if (!DIFF_FILE_VALID(p->one)) {
- if (!DIFF_FILE_VALID(p->two))
- ; /* ignore unmerged */
- /* created */
- else if (contains(p->two, needle, len, regexp,
- kws))
- has_changes = 1;
- }
- else if (!DIFF_FILE_VALID(p->two)) {
- if (contains(p->one, needle, len, regexp, kws))
- has_changes = 1;
- }
- else if (!diff_unmodified_pair(p) &&
- contains(p->one, needle, len, regexp, kws) !=
- contains(p->two, needle, len, regexp, kws))
- has_changes = 1;
-
- if (has_changes)
+ if (has_changes(p, needle, len, regexp, kws))
diff_q(&outq, p);
else
diff_free_filepair(p);
--
1.7.7
^ permalink raw reply related
* Re: Git Bug report
From: Matthieu Moy @ 2011-10-06 16:26 UTC (permalink / raw)
To: Junio C Hamano
Cc: Phil Hord, SZEDER Gábor, git, Fredrik Gustafsson,
Federico Lucifredi
In-Reply-To: <7vy5wy145q.fsf@alter.siamese.dyndns.org>
Junio C Hamano <gitster@pobox.com> writes:
> 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.
Plus, the user may have removed the permission on the .git directory by
mistake, and it would be very surprising behavior if git ran without
complaining using a higher level GIT_DIR (i.e. a more or less arbitrary
repo as far as the user is concerned ...)
> How's that sound as a guideline?
Sounds reasonable, yes.
--
Matthieu Moy
http://www-verimag.imag.fr/~moy/
^ permalink raw reply
* [PATCH 3/7] pickaxe: plug regex/kws leak
From: René Scharfe @ 2011-10-06 16:23 UTC (permalink / raw)
To: Git Mailing List; +Cc: Junio C Hamano
In-Reply-To: <4E8DD065.3040607@lsrfire.ath.cx>
With -S... --pickaxe-all, free the regex or the kws before returning
even if we found a match. Also get rid of the variable has_changes,
as we can simply break out of the loop.
Signed-off-by: Rene Scharfe <rene.scharfe@lsrfire.ath.cx>
---
diffcore-pickaxe.c | 13 +++++++------
1 files changed, 7 insertions(+), 6 deletions(-)
diff --git a/diffcore-pickaxe.c b/diffcore-pickaxe.c
index 96f7ea6..c367d8d 100644
--- a/diffcore-pickaxe.c
+++ b/diffcore-pickaxe.c
@@ -221,7 +221,7 @@ static void diffcore_pickaxe_count(struct diff_options *o)
if (opts & DIFF_PICKAXE_ALL) {
/* Showing the whole changeset if needle exists */
- for (i = has_changes = 0; !has_changes && i < q->nr; i++) {
+ for (i = 0; i < q->nr; i++) {
struct diff_filepair *p = q->queue[i];
if (!DIFF_FILE_VALID(p->one)) {
if (!DIFF_FILE_VALID(p->two))
@@ -238,9 +238,9 @@ static void diffcore_pickaxe_count(struct diff_options *o)
contains(p->one, needle, len, regexp, kws) !=
contains(p->two, needle, len, regexp, kws))
has_changes++;
+ if (has_changes)
+ goto out; /* do not munge the queue */
}
- if (has_changes)
- return; /* not munge the queue */
/* otherwise we will clear the whole queue
* by copying the empty outq at the end of this
@@ -278,13 +278,14 @@ static void diffcore_pickaxe_count(struct diff_options *o)
diff_free_filepair(p);
}
+ free(q->queue);
+ *q = outq;
+
+ out:
if (opts & DIFF_PICKAXE_REGEX)
regfree(®ex);
else
kwsfree(kws);
-
- free(q->queue);
- *q = outq;
return;
}
--
1.7.7
^ permalink raw reply related
* Re: Git Bug report
From: Junio C Hamano @ 2011-10-06 16:22 UTC (permalink / raw)
To: Phil Hord; +Cc: SZEDER Gábor, git, Fredrik Gustafsson, Federico Lucifredi
In-Reply-To: <CABURp0qCsKG2oOxLw4BfU8UM=9V+pigd69ZK=TZVwetBPqjuiA@mail.gmail.com>
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?
^ permalink raw reply
* [PATCH 2/7] pickaxe: plug regex leak
From: René Scharfe @ 2011-10-06 16:14 UTC (permalink / raw)
To: Git Mailing List; +Cc: Junio C Hamano
In-Reply-To: <4E8DD065.3040607@lsrfire.ath.cx>
With -G... --pickaxe-all, free the regex before returning even if we
found a match. Also get rid of the variable has_changes, as we can
simply break out of the loop.
Signed-off-by: Rene Scharfe <rene.scharfe@lsrfire.ath.cx>
---
diffcore-pickaxe.c | 13 ++++++-------
1 files changed, 6 insertions(+), 7 deletions(-)
diff --git a/diffcore-pickaxe.c b/diffcore-pickaxe.c
index 0835a3b..96f7ea6 100644
--- a/diffcore-pickaxe.c
+++ b/diffcore-pickaxe.c
@@ -96,7 +96,7 @@ static int diff_grep(struct diff_filepair *p, regex_t *regexp, struct diff_optio
static void diffcore_pickaxe_grep(struct diff_options *o)
{
struct diff_queue_struct *q = &diff_queued_diff;
- int i, has_changes, err;
+ int i, err;
regex_t regex;
struct diff_queue_struct outq;
outq.queue = NULL;
@@ -112,13 +112,11 @@ static void diffcore_pickaxe_grep(struct diff_options *o)
if (o->pickaxe_opts & DIFF_PICKAXE_ALL) {
/* Showing the whole changeset if needle exists */
- for (i = has_changes = 0; !has_changes && i < q->nr; i++) {
+ for (i = 0; i < q->nr; i++) {
struct diff_filepair *p = q->queue[i];
if (diff_grep(p, ®ex, o))
- has_changes++;
+ goto out; /* do not munge the queue */
}
- if (has_changes)
- return; /* do not munge the queue */
/*
* Otherwise we will clear the whole queue by copying
@@ -138,10 +136,11 @@ static void diffcore_pickaxe_grep(struct diff_options *o)
}
}
- regfree(®ex);
-
free(q->queue);
*q = outq;
+
+ out:
+ regfree(®ex);
return;
}
--
1.7.7
^ permalink raw reply related
* Re: [PATCH 2/4] cleanup: use internal memory allocation wrapper functions everywhere
From: Brandon Casey @ 2011-10-06 16:14 UTC (permalink / raw)
To: Johannes Sixt
Cc: peff@peff.net, git@vger.kernel.org, gitster@pobox.com,
sunshine@sunshineco.com, bharrosh@panasas.com,
trast@student.ethz.ch
In-Reply-To: <4E8D4812.9090102@viscovery.net>
[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.
Thanks,
-Brandon
^ permalink raw reply
* Re: git commit -a reports untracked files after a clone
From: Jeff King @ 2011-10-06 16:06 UTC (permalink / raw)
To: Joerg Rosenkranz; +Cc: Junio C Hamano, git
In-Reply-To: <loom.20111005T161522-357@post.gmane.org>
On Wed, Oct 05, 2011 at 02:26:30PM +0000, Joerg Rosenkranz wrote:
> > On Fri, May 27, 2011 at 02:00:45PM -0400, Jeff King wrote:
> > 1. We load the index, and for each entry, insert it into the index's
> > name_hash. In addition, if ignorecase is turned on, we make an
> > entry in the name_hash for the directory (e.g., "contrib/"), which
> > uses the following code from 5102c61's hash_index_entry_directories:
>
> Sorry for reactivating this old thread.
> We are running in this problem too. The behavior is the same in msysgit and on
> Linux. Your patch resolves that problem for us.
>
> Is there any chance to drive this patch forward?
Thanks for prodding. I wrote a big analysis at the end of that thread,
but didn't get much response. I've been meaning to pick it up again.
So I spent a few minutes revisiting the topic today. I think it's
definitely a bug, and the fix is definitely correct. The patch is below,
with what I hope is a coherent analysis adapted from the previous
thread.
The only question is whether or not it's OK to add a few bytes to
"struct cache_entry" to cater to an uncommon case (see the comments at
the end of the commit message). Junio might have thoughts on that.
-- >8 --
Subject: [PATCH] fix phantom untracked files when core.ignorecase is set
When core.ignorecase is turned on and there are stale index
entries, "git commit" can sometimes report directories as
untracked, even though they contain tracked files.
You can see an example of this with:
# make a case-insensitive repo
git init repo && cd repo &&
git config core.ignorecase true &&
# with some tracked files in a subdir
mkdir subdir &&
> subdir/one &&
> subdir/two &&
git add . &&
git commit -m base &&
# now make the index entries stale
touch subdir/* &&
# and then ask commit to update those entries and show
# us the status template
git commit -a
which will report "subdir/" as untracked, even though it
clearly contains two tracked files. What is happening in the
commit program is this:
1. We load the index, and for each entry, insert it into the index's
name_hash. In addition, if ignorecase is turned on, we make an
entry in the name_hash for the directory (e.g., "contrib/"), which
uses the following code from 5102c61's hash_index_entry_directories:
hash = hash_name(ce->name, ptr - ce->name);
if (!lookup_hash(hash, &istate->name_hash)) {
pos = insert_hash(hash, &istate->name_hash);
if (pos) {
ce->next = *pos;
*pos = ce;
}
}
Note that we only add the directory entry if there is not already an
entry.
2. We run add_files_to_cache, which gets updated information for each
cache entry. It helpfully inserts this information into the cache,
which calls replace_index_entry. This in turn calls
remove_name_hash() on the old entry, and add_name_hash() on the new
one. But remove_name_hash doesn't actually remove from the hash, it
only marks it as "no longer interesting" (from cache.h):
/*
* We don't actually *remove* it, we can just mark it invalid so that
* we won't find it in lookups.
*
* Not only would we have to search the lists (simple enough), but
* we'd also have to rehash other hash buckets in case this makes the
* hash bucket empty (common). So it's much better to just mark
* it.
*/
static inline void remove_name_hash(struct cache_entry *ce)
{
ce->ce_flags |= CE_UNHASHED;
}
This is OK in the specific-file case, since the entries in the hash
form a linked list, and we can just skip the "not here anymore"
entries during lookup.
But for the directory hash entry, we will _not_ write a new entry,
because there is already one there: the old one that is actually no
longer interesting!
3. While traversing the directories, we end up in the
directory_exists_in_index_icase function to see if a directory is
interesting. This in turn checks index_name_exists, which will
look up the directory in the index's name_hash. We see the old,
deleted record, and assume there is nothing interesting. The
directory gets marked as untracked, even though there are index
entries in it.
The problem is in the code I showed above:
hash = hash_name(ce->name, ptr - ce->name);
if (!lookup_hash(hash, &istate->name_hash)) {
pos = insert_hash(hash, &istate->name_hash);
if (pos) {
ce->next = *pos;
*pos = ce;
}
}
Having a single cache entry that represents the directory is
not enough; that entry may go away if the index is changed.
It may be tempting to say that the problem is in our removal
method; if we removed the entry entirely instead of simply
marking it as "not here anymore", then we would know we need
to insert a new entry. But that only covers this particular
case of remove-replace. In the more general case, consider
something like this:
1. We add "foo/bar" and "foo/baz" to the index. Each gets
their own entry in name_hash, plus we make a "foo/"
entry that points to "foo/bar".
2. We remove the "foo/bar" entry from the index, and from
the name_hash.
3. We ask if "foo/" exists, and see no entry, even though
"foo/baz" exists.
So we need that directory entry to have the list of _all_
cache entries that indicate that the directory is tracked.
So that implies making a linked list as we do for other
entries, like:
hash = hash_name(ce->name, ptr - ce->name);
pos = insert_hash(hash, &istate->name_hash);
if (pos) {
ce->next = *pos;
*pos = ce;
}
But that's not right either. In fact, it shows a second bug
in the current code, which is that the "ce->next" pointer is
supposed to be linking entries for a specific filename
entry, but here we are overwriting it for the directory
entry. So the same cache entry ends up in two linked lists,
but they share the same "next" pointer.
As it turns out, this second bug can't be triggered in the
current code. The "if (pos)" conditional is totally dead
code; pos will only be non-NULL if there was an existing
hash entry, and we already checked that there wasn't one
through our call to lookup_hash.
But fixing the first bug means taking out that call to
lookup_hash, which is going to activate the buggy dead code,
and we'll end up splicing the two linked lists together.
So we need to have a separate next pointer for the list in
the directory bucket, and we need to traverse that list in
index_name_exists when we are looking up a directory.
This bloats "struct cache_entry" by a few bytes. Which is
annoying, because it's only necessary when core.ignorecase
is enabled. There's not an easy way around it, short of
separating out the "next" pointers from cache_entry entirely
(i.e., having a separate "cache_entry_list" struct that gets
stored in the name_hash). In practice, it probably doesn't
matter; we have thousands of cache entries, compared to the
millions of objects (where adding 4 bytes to the struct
actually does impact performance).
Signed-off-by: Jeff King <peff@peff.net>
---
cache.h | 1 +
name-hash.c | 15 ++++++++-------
2 files changed, 9 insertions(+), 7 deletions(-)
diff --git a/cache.h b/cache.h
index 607c2ea..1334fbf 100644
--- a/cache.h
+++ b/cache.h
@@ -168,6 +168,7 @@ struct cache_entry {
unsigned int ce_flags;
unsigned char sha1[20];
struct cache_entry *next;
+ struct cache_entry *dir_next;
char name[FLEX_ARRAY]; /* more */
};
diff --git a/name-hash.c b/name-hash.c
index c6b6a3f..225dd76 100644
--- a/name-hash.c
+++ b/name-hash.c
@@ -57,12 +57,10 @@ static void hash_index_entry_directories(struct index_state *istate, struct cach
if (*ptr == '/') {
++ptr;
hash = hash_name(ce->name, ptr - ce->name);
- if (!lookup_hash(hash, &istate->name_hash)) {
- pos = insert_hash(hash, ce, &istate->name_hash);
- if (pos) {
- ce->next = *pos;
- *pos = ce;
- }
+ pos = insert_hash(hash, ce, &istate->name_hash);
+ if (pos) {
+ ce->dir_next = *pos;
+ *pos = ce;
}
}
}
@@ -166,7 +164,10 @@ struct cache_entry *index_name_exists(struct index_state *istate, const char *na
if (same_name(ce, name, namelen, icase))
return ce;
}
- ce = ce->next;
+ if (icase && name[namelen - 1] == '/')
+ ce = ce->dir_next;
+ else
+ ce = ce->next;
}
/*
--
1.7.7.37.g0e376
^ 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