* Re: best way to fastforward all tracking branches after a fetch
From: Stefan Haller @ 2011-12-12 9:19 UTC (permalink / raw)
To: Jeff King; +Cc: Hallvard B Furuseth, Gelonida N, git
In-Reply-To: <20111212082526.GC16511@sigill.intra.peff.net>
Jeff King <peff@peff.net> wrote:
> On Mon, Dec 12, 2011 at 08:33:15AM +0100, Stefan Haller wrote:
>
> > > Local branches can track each other. So the script needs to toposort
> > > the branches, or to loop until either nothing was done or an error
> > > happened. (The latter to prevent an eternal loop on error.)
> >
> > Is this just theoretical, or are there real use cases for this? What
> > would be a workflow with such a local tracking branch?
>
> I use this all the time.
>
> In git.git, we use a topic branch workflow (i.e., every feature gets its
> own topic branch, and topics graduate independently to master as they
> are deemed stable). And we use a patch-submission workflow, which means
> it's OK for me to rebase my topics locally, because the end-product is a
> series of patches sent to the list.
>
> Typically I branch off of "origin/master", so the topic is independent
> of anything else. For example, the "jk/credentials" branch in my git
> repo is branched from "origin/master" (Junio's master). But sometimes
> there is a topic that depends on another topic, but should not be part
> of the same series (because the the first topic can graduate to master,
> but the second one may still need more time for discussion and cooking).
> In that case, I'll set the upstream to the other local topic branch. An
> example of this is the "jk/prompt" series, which depends on
> "jk/credentials" for infrastructure, but is really a separate issue.
>
> Having the upstream set is convenient, because I can get _just_ the
> commits in jk/prompt with "git log @{u}..". Or I can rebase _just_ the
> commits in that topic with "git rebase -i". If my upstream were set to
> origin, I would accidentally also rebase all of the commits pulled in
> from jk/credentials, too.
I see, thanks. For my script, I'm wondering then if the most sensible
thing to do is to just skip any branch whose upstream doesn't start with
refs/remotes/.
For a future "git pull --all" feature, it would probably only work on
those branches whose upstream is on the remote being pulled from,
anyway.
--
Stefan Haller
Berlin, Germany
http://www.haller-berlin.de/
^ permalink raw reply
* Re: Access to git repository through squid proxy: The remote end hung up unexpectedly
From: Mathieu Peltier @ 2011-12-12 9:25 UTC (permalink / raw)
To: Konstantin Khomoutov; +Cc: git
In-Reply-To: <20111210225818.GM2525@localhost.localdomain>
> Looks like either your socat supplies incorrect credentials or proxy
> does not ask socat to actually authenticate and just denies the request.
The problem is solved. Sorry in fact it was just a problem with the
proxy configuration: the squid proxy was configured to use another
proxy server which was denying to use HTTP connect on 9418 port...
Thanks for you help.
Mathieu
^ permalink raw reply
* Re: Breakage (?) in configure and git_vsnprintf()
From: Andreas Schwab @ 2011-12-12 9:30 UTC (permalink / raw)
To: Jeff King
Cc: Michael Haggerty, Junio C Hamano, git discussion list,
Michal Rokos, Brandon Casey
In-Reply-To: <20111212064305.GA16511@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> if (maxsize > 0) {
> - ret = vsnprintf(str, maxsize-SNPRINTF_SIZE_CORR, format, ap);
> + va_copy(cp, ap);
> + ret = vsnprintf(str, maxsize-SNPRINTF_SIZE_CORR, format, cp);
You also need to call va_end on the copy.
Andreas.
--
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756 01D3 44D5 214B 8276 4ED5
"And now for something completely different."
^ permalink raw reply
* Re: best way to fastforward all tracking branches after a fetch
From: Gelonida N @ 2011-12-12 10:13 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <7vmxay5h0g.fsf@alter.siamese.dyndns.org>
Thanks for this rather long answer,
On 12/12/2011 09:09 AM, Junio C Hamano wrote:
> Gelonida N <gelonida@gmail.com> writes:
>
>> What is the best way to fastforward all fastforwardable tracking
>> branches after a git fetch?
>
> This lacks context and invites too many tangents, so I'll only touch a few
> of them.
>
> First of all, why do you want to do this?
>
To explain the scenario:
- small project
- every person works on master and multiple topic branches
and might alternate rather often
- sometimes several persons work on the same topic branch
but most of the time not in parallel.
- one person is working from several machines (starting work on
one and continuing on another)
- additionally we do many pushed in order to be sure,
that our data is backed up in case of disk failures.
- sometimes I just want to 'build' from a branch, that I am not
working on. but there I create mostly not even a tracking branch
before changing a machine I want to be sure to have pushed everything. I
wanted to get rid of the warning, that some branches cannot be pushed,
because they aren't fastforwarded
when checking out a branch I want to avoid, that I have to pull manually.
> In other words, wouldn't a post-checkout hook be a better place to do
> this kind of thing, perhaps like this (completely untested)?
>
> #!/bin/sh
> old=$1 new=$2 kind=$3
>
> # did we checkout a branch?
> test "$kind" = 1 || exit 0
>
> # what did we check out?
> branch=$(git symbolic-ref HEAD 2>/dev/null) || exit 0
>
> # does it track anything? otherwise nothing needs to be done
> upstream=$(git for-each-ref --format='%(upstream)' "$branch")
> test -z "$upstream" || exit 0
>
> # are we up-to-date? if so no need to do anything
> test 0 = $(git rev-list "..$upstream" | wc -l) && exit 0
>
> # do we have something we made? if so no point trying to fast-forward
> test 0 = $(git rev-list "$upstream.." | wc -l) || exit 0
>
> # attempt a fast-forward merge with it
> git merge --ff-only @{upstream}
>
This is a solution, I wouldn't get rid of the warnings though when
running git push.
^ permalink raw reply
* Re: best way to fastforward all tracking branches after a fetch
From: Gelonida N @ 2011-12-12 10:24 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano
In-Reply-To: <4EE5D3CD.6020604@gmail.com>
I forgot one other use case:
- wanting to pull from tracking branches without fastforwarding is not
such a smart idea.
of course I can do
git merge from remotes/origin/branch
but this is more to type and would vary depending on whether 'd like to
pull from an unpushed tracking branch or from a freshly fetched tracking
branch.
On 12/12/2011 11:13 AM, Gelonida N wrote:
> Thanks for this rather long answer,
>
> On 12/12/2011 09:09 AM, Junio C Hamano wrote:
>> Gelonida N <gelonida@gmail.com> writes:
>>
>>> What is the best way to fastforward all fastforwardable tracking
>>> branches after a git fetch?
>>
>> This lacks context and invites too many tangents, so I'll only touch a few
>> of them.
>>
>> First of all, why do you want to do this?
>>
>
> To explain the scenario:
> - small project
> - every person works on master and multiple topic branches
> and might alternate rather often
> - sometimes several persons work on the same topic branch
> but most of the time not in parallel.
> - one person is working from several machines (starting work on
> one and continuing on another)
> - additionally we do many pushed in order to be sure,
> that our data is backed up in case of disk failures.
> - sometimes I just want to 'build' from a branch, that I am not
> working on. but there I create mostly not even a tracking branch
>
> before changing a machine I want to be sure to have pushed everything. I
> wanted to get rid of the warning, that some branches cannot be pushed,
> because they aren't fastforwarded
>
> when checking out a branch I want to avoid, that I have to pull manually.
>
>
>
>> In other words, wouldn't a post-checkout hook be a better place to do
>> this kind of thing, perhaps like this (completely untested)?
>>
>> #!/bin/sh
>> old=$1 new=$2 kind=$3
>>
>> # did we checkout a branch?
>> test "$kind" = 1 || exit 0
>>
>> # what did we check out?
>> branch=$(git symbolic-ref HEAD 2>/dev/null) || exit 0
>>
>> # does it track anything? otherwise nothing needs to be done
>> upstream=$(git for-each-ref --format='%(upstream)' "$branch")
>> test -z "$upstream" || exit 0
>>
>> # are we up-to-date? if so no need to do anything
>> test 0 = $(git rev-list "..$upstream" | wc -l) && exit 0
>>
>> # do we have something we made? if so no point trying to fast-forward
>> test 0 = $(git rev-list "$upstream.." | wc -l) || exit 0
>>
>> # attempt a fast-forward merge with it
>> git merge --ff-only @{upstream}
>>
>
> This is a solution, I wouldn't get rid of the warnings though when
> running git push.
>
^ permalink raw reply
* Re: Breakage (?) in configure and git_vsnprintf()
From: Michael Haggerty @ 2011-12-12 10:25 UTC (permalink / raw)
To: Jeff King
Cc: Junio C Hamano, git discussion list, Michal Rokos, Brandon Casey
In-Reply-To: <20111212064305.GA16511@sigill.intra.peff.net>
On 12/12/2011 07:43 AM, Jeff King wrote:
> On Sun, Dec 11, 2011 at 07:42:03PM +0100, Michael Haggerty wrote:
>> 2. The configure problem causes git_vsnprintf() to be wrapped around the
>> C library version. This leads to many failures in the test suite. I
>> suppose that git_vsnprintf() is broken in some way.
>
> I enabled SNPRINTF_RETURNS_BOGUS manually and was able to see the test
> suite failures. Very oddly, I could get them while running the full
> suite in parallel, but when I ran individual scripts, the problem went
> away. Which makes no sense to me at all.
>
> However, I peeked at the git_vsnprintf function, and one obvious error
> is that it calls vsnprintf multiple times on the same va_list.
Thanks for the quick response! Yes, I think you've hit the nail on the
head. (Though I think Andreas is correct that va_end() needs to be
called on the copies.) Either with or without va_end(), your patch
fixes the test suite failures for me.
> I'll leave the issue of "-std=c89" triggering SNPRINTF_RETURNS_BOGUS to
> people who know and care about autoconf. My gut is to say "don't do
> that". Git is not actually pure c89. [...]
OK, I can live with that. Poor Junio will probably be stuck correcting
my non-c89isms again, though :-(
Michael
--
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/
^ permalink raw reply
* [PATCH 1/4] revert: convert resolve_ref() to read_ref_full()
From: Nguyễn Thái Ngọc Duy @ 2011-12-12 11:20 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jonathan Nieder,
Nguyễn Thái Ngọc Duy
This is the follow up of c689332 (Convert many resolve_ref() calls to
read_ref*() and ref_exists() - 2011-11-13). See the said commit for
rationale.
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
builtin/revert.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/builtin/revert.c b/builtin/revert.c
index 1ea525c..0c52a83 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -901,7 +901,7 @@ static int rollback_single_pick(void)
if (!file_exists(git_path("CHERRY_PICK_HEAD")) &&
!file_exists(git_path("REVERT_HEAD")))
return error(_("no cherry-pick or revert in progress"));
- if (!resolve_ref("HEAD", head_sha1, 0, NULL))
+ if (read_ref_full("HEAD", head_sha1, 0, NULL))
return error(_("cannot resolve HEAD"));
if (is_null_sha1(head_sha1))
return error(_("cannot abort from a branch yet to be born"));
--
1.7.8.36.g69ee2
^ permalink raw reply related
* [PATCH 2/4] Convert resolve_ref+xstrdup to new resolve_refdup function
From: Nguyễn Thái Ngọc Duy @ 2011-12-12 11:20 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jonathan Nieder,
Nguyễn Thái Ngọc Duy
In-Reply-To: <1323688832-32016-1-git-send-email-pclouds@gmail.com>
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
builtin/branch.c | 11 ++++-------
builtin/checkout.c | 15 +++++++--------
builtin/fmt-merge-msg.c | 6 +++---
builtin/for-each-ref.c | 7 ++-----
builtin/merge.c | 12 +++++-------
builtin/notes.c | 6 +++---
builtin/receive-pack.c | 8 +++-----
builtin/show-branch.c | 4 +---
cache.h | 1 +
reflog-walk.c | 13 ++++++-------
refs.c | 6 ++++++
wt-status.c | 4 +---
12 files changed, 42 insertions(+), 51 deletions(-)
diff --git a/builtin/branch.c b/builtin/branch.c
index e1e486e..72c4c31 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -103,7 +103,7 @@ static int branch_merged(int kind, const char *name,
* safely to HEAD (or the other branch).
*/
struct commit *reference_rev = NULL;
- const char *reference_name = NULL;
+ char *reference_name = NULL;
int merged;
if (kind == REF_LOCAL_BRANCH) {
@@ -115,10 +115,8 @@ static int branch_merged(int kind, const char *name,
branch->merge[0] &&
branch->merge[0]->dst &&
(reference_name =
- resolve_ref(branch->merge[0]->dst, sha1, 1, NULL)) != NULL) {
- reference_name = xstrdup(reference_name);
+ resolve_refdup(branch->merge[0]->dst, sha1, 1, NULL)) != NULL)
reference_rev = lookup_commit_reference(sha1);
- }
}
if (!reference_rev)
reference_rev = head_rev;
@@ -143,7 +141,7 @@ static int branch_merged(int kind, const char *name,
" '%s', even though it is merged to HEAD."),
name, reference_name);
}
- free((char *)reference_name);
+ free(reference_name);
return merged;
}
@@ -731,10 +729,9 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
track = git_branch_track;
- head = resolve_ref("HEAD", head_sha1, 0, NULL);
+ head = resolve_refdup("HEAD", head_sha1, 0, NULL);
if (!head)
die(_("Failed to resolve HEAD as a valid ref."));
- head = xstrdup(head);
if (!strcmp(head, "HEAD")) {
detached = 1;
} else {
diff --git a/builtin/checkout.c b/builtin/checkout.c
index b7c6302..a66d3eb 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -696,17 +696,14 @@ static int switch_branches(struct checkout_opts *opts, struct branch_info *new)
{
int ret = 0;
struct branch_info old;
+ char *path;
unsigned char rev[20];
int flag;
memset(&old, 0, sizeof(old));
- old.path = resolve_ref("HEAD", rev, 0, &flag);
- if (old.path)
- old.path = xstrdup(old.path);
+ old.path = path = resolve_refdup("HEAD", rev, 0, &flag);
old.commit = lookup_commit_reference_gently(rev, 1);
- if (!(flag & REF_ISSYMREF)) {
- free((char *)old.path);
+ if (!(flag & REF_ISSYMREF))
old.path = NULL;
- }
if (old.path && !prefixcmp(old.path, "refs/heads/"))
old.name = old.path + strlen("refs/heads/");
@@ -720,8 +717,10 @@ static int switch_branches(struct checkout_opts *opts, struct branch_info *new)
}
ret = merge_working_tree(opts, &old, new);
- if (ret)
+ if (ret) {
+ free(path);
return ret;
+ }
if (!opts->quiet && !old.path && old.commit && new->commit != old.commit)
orphaned_commit_warning(old.commit);
@@ -729,7 +728,7 @@ static int switch_branches(struct checkout_opts *opts, struct branch_info *new)
update_refs_for_switch(opts, &old, new);
ret = post_checkout_hook(old.commit, new->commit, 1);
- free((char *)old.path);
+ free(path);
return ret || opts->writeout_error;
}
diff --git a/builtin/fmt-merge-msg.c b/builtin/fmt-merge-msg.c
index bdfa0ea..a27efcd 100644
--- a/builtin/fmt-merge-msg.c
+++ b/builtin/fmt-merge-msg.c
@@ -372,14 +372,14 @@ int fmt_merge_msg(struct strbuf *in, struct strbuf *out,
int i = 0, pos = 0;
unsigned char head_sha1[20];
const char *current_branch;
+ char *ref;
/* get current branch */
- current_branch = resolve_ref("HEAD", head_sha1, 1, NULL);
+ current_branch = ref = resolve_refdup("HEAD", head_sha1, 1, NULL);
if (!current_branch)
die("No current branch");
if (!prefixcmp(current_branch, "refs/heads/"))
current_branch += 11;
- current_branch = xstrdup(current_branch);
/* get a line */
while (pos < in->len) {
@@ -421,7 +421,7 @@ int fmt_merge_msg(struct strbuf *in, struct strbuf *out,
}
strbuf_complete_line(out);
- free((char *)current_branch);
+ free(ref);
return 0;
}
diff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c
index d90e5d2..b01d76a 100644
--- a/builtin/for-each-ref.c
+++ b/builtin/for-each-ref.c
@@ -628,11 +628,8 @@ static void populate_value(struct refinfo *ref)
if (need_symref && (ref->flag & REF_ISSYMREF) && !ref->symref) {
unsigned char unused1[20];
- const char *symref;
- symref = resolve_ref(ref->refname, unused1, 1, NULL);
- if (symref)
- ref->symref = xstrdup(symref);
- else
+ ref->symref = resolve_refdup(ref->refname, unused1, 1, NULL);
+ if (!ref->symref)
ref->symref = "";
}
diff --git a/builtin/merge.c b/builtin/merge.c
index a1c8534..6437db2 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -1096,6 +1096,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
struct commit_list *common = NULL;
const char *best_strategy = NULL, *wt_strategy = NULL;
struct commit_list **remotes = &remoteheads;
+ char *branch_ref;
if (argc == 2 && !strcmp(argv[1], "-h"))
usage_with_options(builtin_merge_usage, builtin_merge_options);
@@ -1104,12 +1105,9 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
* Check if we are _not_ on a detached HEAD, i.e. if there is a
* current branch.
*/
- branch = resolve_ref("HEAD", head_sha1, 0, &flag);
- if (branch) {
- if (!prefixcmp(branch, "refs/heads/"))
- branch += 11;
- branch = xstrdup(branch);
- }
+ branch = branch_ref = resolve_refdup("HEAD", head_sha1, 0, &flag);
+ if (branch && !prefixcmp(branch, "refs/heads/"))
+ branch += 11;
if (!branch || is_null_sha1(head_sha1))
head_commit = NULL;
else
@@ -1520,6 +1518,6 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
ret = suggest_conflicts(option_renormalize);
done:
- free((char *)branch);
+ free(branch_ref);
return ret;
}
diff --git a/builtin/notes.c b/builtin/notes.c
index 10b8bc7..816c998 100644
--- a/builtin/notes.c
+++ b/builtin/notes.c
@@ -804,6 +804,7 @@ static int merge_commit(struct notes_merge_options *o)
struct notes_tree *t;
struct commit *partial;
struct pretty_print_context pretty_ctx;
+ char *ref;
int ret;
/*
@@ -826,10 +827,9 @@ static int merge_commit(struct notes_merge_options *o)
t = xcalloc(1, sizeof(struct notes_tree));
init_notes(t, "NOTES_MERGE_PARTIAL", combine_notes_overwrite, 0);
- o->local_ref = resolve_ref("NOTES_MERGE_REF", sha1, 0, NULL);
+ o->local_ref = ref = resolve_refdup("NOTES_MERGE_REF", sha1, 0, NULL);
if (!o->local_ref)
die("Failed to resolve NOTES_MERGE_REF");
- o->local_ref = xstrdup(o->local_ref);
if (notes_merge_commit(o, t, partial, sha1))
die("Failed to finalize notes merge");
@@ -846,7 +846,7 @@ static int merge_commit(struct notes_merge_options *o)
free_notes(t);
strbuf_release(&msg);
ret = merge_abort(o);
- free((char *)o->local_ref);
+ free(ref);
return ret;
}
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index b6d957c..5cd6fc7 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -36,7 +36,7 @@ static int use_sideband;
static int prefer_ofs_delta = 1;
static int auto_update_server_info;
static int auto_gc = 1;
-static const char *head_name;
+static char *head_name;
static int sent_capabilities;
static enum deny_action parse_deny_action(const char *var, const char *value)
@@ -695,10 +695,8 @@ static void execute_commands(struct command *commands, const char *unpacker_erro
check_aliased_updates(commands);
- free((char *)head_name);
- head_name = resolve_ref("HEAD", sha1, 0, NULL);
- if (head_name)
- head_name = xstrdup(head_name);
+ free(head_name);
+ head_name = resolve_refdup("HEAD", sha1, 0, NULL);
for (cmd = commands; cmd; cmd = cmd->next)
if (!cmd->skip_update)
diff --git a/builtin/show-branch.c b/builtin/show-branch.c
index 4b480d7..a1f148e 100644
--- a/builtin/show-branch.c
+++ b/builtin/show-branch.c
@@ -726,10 +726,8 @@ int cmd_show_branch(int ac, const char **av, const char *prefix)
if (ac == 0) {
static const char *fake_av[2];
- const char *refname;
- refname = resolve_ref("HEAD", sha1, 1, NULL);
- fake_av[0] = xstrdup(refname);
+ fake_av[0] = resolve_refdup("HEAD", sha1, 1, NULL);
fake_av[1] = NULL;
av = fake_av;
ac = 1;
diff --git a/cache.h b/cache.h
index 8c98d05..4887a3e 100644
--- a/cache.h
+++ b/cache.h
@@ -866,6 +866,7 @@ extern int read_ref(const char *filename, unsigned char *sha1);
* errno is sometimes set on errors, but not always.
*/
extern const char *resolve_ref(const char *ref, unsigned char *sha1, int reading, int *flag);
+extern char *resolve_refdup(const char *ref, unsigned char *sha1, int reading, int *flag);
extern int dwim_ref(const char *str, int len, unsigned char *sha1, char **ref);
extern int dwim_log(const char *str, int len, unsigned char *sha1, char **ref);
diff --git a/reflog-walk.c b/reflog-walk.c
index da71a85..5572b42 100644
--- a/reflog-walk.c
+++ b/reflog-walk.c
@@ -50,11 +50,10 @@ static struct complete_reflogs *read_complete_reflog(const char *ref)
for_each_reflog_ent(ref, read_one_reflog, reflogs);
if (reflogs->nr == 0) {
unsigned char sha1[20];
- const char *name = resolve_ref(ref, sha1, 1, NULL);
+ char *name = resolve_refdup(ref, sha1, 1, NULL);
if (name) {
- name = xstrdup(name);
for_each_reflog_ent(name, read_one_reflog, reflogs);
- free((char *)name);
+ free(name);
}
}
if (reflogs->nr == 0) {
@@ -171,11 +170,11 @@ int add_reflog_for_walk(struct reflog_walk_info *info,
else {
if (*branch == '\0') {
unsigned char sha1[20];
- const char *head = resolve_ref("HEAD", sha1, 0, NULL);
- if (!head)
- die ("No current branch");
free(branch);
- branch = xstrdup(head);
+ branch = resolve_refdup("HEAD", sha1, 0, NULL);
+ if (!branch)
+ die ("No current branch");
+
}
reflogs = read_complete_reflog(branch);
if (!reflogs || reflogs->nr == 0) {
diff --git a/refs.c b/refs.c
index f5cb297..8ffb32f 100644
--- a/refs.c
+++ b/refs.c
@@ -605,6 +605,12 @@ const char *resolve_ref(const char *ref, unsigned char *sha1, int reading, int *
return ref;
}
+char *resolve_refdup(const char *ref, unsigned char *sha1, int reading, int *flag)
+{
+ const char *ret = resolve_ref(ref, sha1, reading, flag);
+ return ret ? xstrdup(ret) : NULL;
+}
+
/* The argument to filter_refs */
struct ref_filter {
const char *pattern;
diff --git a/wt-status.c b/wt-status.c
index 70fdb76..9ffc535 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -111,7 +111,6 @@ void status_printf_more(struct wt_status *s, const char *color,
void wt_status_prepare(struct wt_status *s)
{
unsigned char sha1[20];
- const char *head;
memset(s, 0, sizeof(*s));
memcpy(s->color_palette, default_wt_status_colors,
@@ -119,8 +118,7 @@ void wt_status_prepare(struct wt_status *s)
s->show_untracked_files = SHOW_NORMAL_UNTRACKED_FILES;
s->use_color = -1;
s->relative_paths = 1;
- head = resolve_ref("HEAD", sha1, 0, NULL);
- s->branch = head ? xstrdup(head) : NULL;
+ s->branch = resolve_refdup("HEAD", sha1, 0, NULL);
s->reference = "HEAD";
s->fp = stdout;
s->index_file = get_index_file();
--
1.7.8.36.g69ee2
^ permalink raw reply related
* [PATCH 3/4] Guard memory overwriting in resolve_ref's static buffer
From: Nguyễn Thái Ngọc Duy @ 2011-12-12 11:20 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jonathan Nieder,
Nguyễn Thái Ngọc Duy
In-Reply-To: <1323688832-32016-1-git-send-email-pclouds@gmail.com>
There is a potential problem with resolve_ref() and some other
functions in git. The return value returned by resolve_ref() may
be changed when the function is called again. Callers must make sure the
next call won't happen as long as the value is still being used.
It's usually hard to track down this kind of problem. Michael Haggerty
has an idea [1] that, instead of passing the same static buffer to
caller every time the function is called, we free the old buffer and
allocate the new one. This way access to the old (now invalid) buffer
may be caught.
This patch applies the same principle for resolve_ref() with a
few modifications:
- This behavior is enabled when GIT_DEBUG_MEMCHECK is set. The ability
is always available. We may be able to ask users to rerun with this
flag on in suspicious cases.
- Rely on mmap/mprotect to catch illegal access. We need valgrind or
some other memory tracking tool to reliably catch this in Michael's
approach.
- Because mprotect is used instead of munmap, we definitely leak
memory. Hopefully callers will not put resolve_ref() in a
loop that runs 1 million times.
- Save caller location in the allocated buffer so we know who made this
call in the core dump.
Also introduce a new target, "make memcheck", that runs tests with this
flag on.
[1] http://comments.gmane.org/gmane.comp.version-control.git/182209
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
Changes include:
- __FUNCTION__ to __FILE__ for compiler compatibility
- x{malloc,free}_mmap() now put call site information in a struct,
it's clearer this way and hopefully will avoid platform issues
- update t0071 to follow '&&' convention
- add notes where to get caller site info in git-compat-util.h
.gitignore | 1 +
Makefile | 4 ++++
cache.h | 3 ++-
git-compat-util.h | 20 ++++++++++++++++++++
refs.c | 13 +++++++++++--
t/t0071-memcheck.sh | 11 +++++++++++
test-resolve-ref.c | 18 ++++++++++++++++++
wrapper.c | 27 +++++++++++++++++++++++++++
8 files changed, 94 insertions(+), 3 deletions(-)
create mode 100755 t/t0071-memcheck.sh
create mode 100644 test-resolve-ref.c
diff --git a/.gitignore b/.gitignore
index 8572c8c..470e452 100644
--- a/.gitignore
+++ b/.gitignore
@@ -179,6 +179,7 @@
/test-obj-pool
/test-parse-options
/test-path-utils
+/test-resolve-ref
/test-run-command
/test-sha1
/test-sigchain
diff --git a/Makefile b/Makefile
index ed82320..d71cf04 100644
--- a/Makefile
+++ b/Makefile
@@ -444,6 +444,7 @@ TEST_PROGRAMS_NEED_X += test-obj-pool
TEST_PROGRAMS_NEED_X += test-parse-options
TEST_PROGRAMS_NEED_X += test-path-utils
TEST_PROGRAMS_NEED_X += test-run-command
+TEST_PROGRAMS_NEED_X += test-resolve-ref
TEST_PROGRAMS_NEED_X += test-sha1
TEST_PROGRAMS_NEED_X += test-sigchain
TEST_PROGRAMS_NEED_X += test-string-pool
@@ -2241,6 +2242,9 @@ export NO_SVN_TESTS
test: all
$(MAKE) -C t/ all
+memcheck: all
+ GIT_DEBUG_MEMCHECK=1 $(MAKE) -C t/ all
+
test-ctype$X: ctype.o
test-date$X: date.o ctype.o
diff --git a/cache.h b/cache.h
index 4887a3e..ba5e911 100644
--- a/cache.h
+++ b/cache.h
@@ -865,7 +865,8 @@ extern int read_ref(const char *filename, unsigned char *sha1);
*
* errno is sometimes set on errors, but not always.
*/
-extern const char *resolve_ref(const char *ref, unsigned char *sha1, int reading, int *flag);
+#define resolve_ref(ref, sha1, reading, flag) resolve_ref_real(ref, sha1, reading, flag, __FILE__, __LINE__)
+extern const char *resolve_ref_real(const char *ref, unsigned char *sha1, int reading, int *flag, const char *file, int line);
extern char *resolve_refdup(const char *ref, unsigned char *sha1, int reading, int *flag);
extern int dwim_ref(const char *str, int len, unsigned char *sha1, char **ref);
diff --git a/git-compat-util.h b/git-compat-util.h
index 77062ed..0cb6e34 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -433,6 +433,26 @@ extern char *xstrndup(const char *str, size_t len);
extern void *xrealloc(void *ptr, size_t size);
extern void *xcalloc(size_t nmemb, size_t size);
extern void *xmmap(void *start, size_t length, int prot, int flags, int fd, off_t offset);
+
+/*
+ * These functions are used to allocate new memory blocks and catch
+ * invalid use after they are released (though the memory is never
+ * returned to system, so do not allocate too much this way).
+ *
+ * mprotect() is used to remove all access to memory when xfree_mmap()
+ * is called. Invalid access will cause sigsegv. The memory block is
+ * preceded by struct alloc_header, describing where it is
+ * allocated. This information can be found in the core dump.
+ */
+struct alloc_header {
+ const char *file;
+ int line;
+ int size;
+ char buf[FLEX_ARRAY];
+};
+extern void *xmalloc_mmap(size_t, const char *file, int line);
+extern void xfree_mmap(void *);
+
extern ssize_t xread(int fd, void *buf, size_t len);
extern ssize_t xwrite(int fd, const void *buf, size_t len);
extern int xdup(int fd);
diff --git a/refs.c b/refs.c
index 8ffb32f..cf8dfcc 100644
--- a/refs.c
+++ b/refs.c
@@ -497,12 +497,21 @@ static int get_packed_ref(const char *ref, unsigned char *sha1)
return -1;
}
-const char *resolve_ref(const char *ref, unsigned char *sha1, int reading, int *flag)
+const char *resolve_ref_real(const char *ref, unsigned char *sha1,
+ int reading, int *flag, const char *file, int line)
{
int depth = MAXDEPTH;
ssize_t len;
char buffer[256];
- static char ref_buffer[256];
+ static char real_ref_buffer[256];
+ static char *ref_buffer;
+
+ if (!ref_buffer && !getenv("GIT_DEBUG_MEMCHECK"))
+ ref_buffer = real_ref_buffer;
+ if (ref_buffer != real_ref_buffer) {
+ xfree_mmap(ref_buffer);
+ ref_buffer = xmalloc_mmap(256, file, line);
+ }
if (flag)
*flag = 0;
diff --git a/t/t0071-memcheck.sh b/t/t0071-memcheck.sh
new file mode 100755
index 0000000..8904369
--- /dev/null
+++ b/t/t0071-memcheck.sh
@@ -0,0 +1,11 @@
+#!/bin/sh
+
+test_description='test that GIT_DEBUG_MEMCHECK works correctly'
+. ./test-lib.sh
+
+test_expect_success 'test-resolve-ref must crash' '
+ test-resolve-ref &&
+ GIT_DEBUG_MEMCHECK=1 test_expect_code 139 test-resolve-ref
+'
+
+test_done
diff --git a/test-resolve-ref.c b/test-resolve-ref.c
new file mode 100644
index 0000000..b772038
--- /dev/null
+++ b/test-resolve-ref.c
@@ -0,0 +1,18 @@
+#include "cache.h"
+
+int main(int argc, char **argv)
+{
+ unsigned char sha1[20];
+ const char *ref1, *ref2;
+ setup_git_directory();
+
+ /*
+ * This is an invalid use of resolve_ref_unsafe(). This
+ * function returns a shared buffer, so by the time the second
+ * call is made, ref1 must _not_ be accessed any more.
+ */
+ ref1 = resolve_ref("HEAD", sha1, 0, NULL);
+ ref2 = resolve_ref("HEAD", sha1, 0, NULL);
+ printf("ref1 = %s\nref2 = %s\n", ref1, ref2);
+ return 0;
+}
diff --git a/wrapper.c b/wrapper.c
index 85f09df..02b6c81 100644
--- a/wrapper.c
+++ b/wrapper.c
@@ -60,6 +60,33 @@ void *xmallocz(size_t size)
return ret;
}
+void *xmalloc_mmap(size_t size, const char *file, int line)
+{
+ struct alloc_header *block;
+ size += offsetof(struct alloc_header,buf);
+ block = mmap(NULL, size, PROT_READ | PROT_WRITE,
+ MAP_ANONYMOUS | MAP_PRIVATE, -1, 0);
+ if (block == (struct alloc_header*)-1)
+ die_errno("unable to mmap %lu bytes anonymously",
+ (unsigned long)size);
+
+ block->file = file;
+ block->line = line;
+ block->size = size;
+ return block->buf;
+}
+
+void xfree_mmap(void *p)
+{
+ struct alloc_header *block;
+
+ if (!p)
+ return;
+ block = (struct alloc_header *)((char*)p - offsetof(struct alloc_header,buf));
+ if (mprotect(block, block->size, PROT_NONE) == -1)
+ die_errno("unable to remove memory access");
+}
+
/*
* xmemdupz() allocates (len + 1) bytes of memory, duplicates "len" bytes of
* "data" to the allocated memory, zero terminates the allocated memory,
--
1.7.8.36.g69ee2
^ permalink raw reply related
* [PATCH 4/4] Rename resolve_ref() to resolve_ref_unsafe()
From: Nguyễn Thái Ngọc Duy @ 2011-12-12 11:20 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jonathan Nieder,
Nguyễn Thái Ngọc Duy
In-Reply-To: <1323688832-32016-1-git-send-email-pclouds@gmail.com>
resolve_ref() may return a pointer to a shared buffer and can be
overwritten by the next resolve_ref() calls. Callers need to
pay attention, not to keep the pointer when the next call happens.
Rename with "_unsafe" suffix to warn developers (or reviewers) before
introducing new call sites.
This patch is generated using the following command
git grep -l 'resolve_ref(' -- '*.[ch]'|xargs sed -i 's/resolve_ref(/resolve_ref_unsafe(/g'
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
branch.c | 2 +-
builtin/branch.c | 2 +-
builtin/commit.c | 2 +-
builtin/fsck.c | 2 +-
builtin/log.c | 4 ++--
builtin/receive-pack.c | 2 +-
builtin/remote.c | 2 +-
builtin/show-branch.c | 2 +-
builtin/symbolic-ref.c | 2 +-
cache.h | 2 +-
refs.c | 20 ++++++++++----------
remote.c | 6 +++---
test-resolve-ref.c | 4 ++--
transport.c | 2 +-
14 files changed, 27 insertions(+), 27 deletions(-)
diff --git a/branch.c b/branch.c
index d91a099..772a4c2 100644
--- a/branch.c
+++ b/branch.c
@@ -182,7 +182,7 @@ int validate_new_branchname(const char *name, struct strbuf *ref,
const char *head;
unsigned char sha1[20];
- head = resolve_ref("HEAD", sha1, 0, NULL);
+ head = resolve_ref_unsafe("HEAD", sha1, 0, NULL);
if (!is_bare_repository() && head && !strcmp(head, ref->buf))
die("Cannot force update the current branch.");
}
diff --git a/builtin/branch.c b/builtin/branch.c
index 72c4c31..641bee1 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -251,7 +251,7 @@ static char *resolve_symref(const char *src, const char *prefix)
int flag;
const char *dst, *cp;
- dst = resolve_ref(src, sha1, 0, &flag);
+ dst = resolve_ref_unsafe(src, sha1, 0, &flag);
if (!(dst && (flag & REF_ISSYMREF)))
return NULL;
if (prefix && (cp = skip_prefix(dst, prefix)))
diff --git a/builtin/commit.c b/builtin/commit.c
index e36e9ad..4d39d25 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -1304,7 +1304,7 @@ static void print_summary(const char *prefix, const unsigned char *sha1,
rev.diffopt.break_opt = 0;
diff_setup_done(&rev.diffopt);
- head = resolve_ref("HEAD", junk_sha1, 0, NULL);
+ head = resolve_ref_unsafe("HEAD", junk_sha1, 0, NULL);
printf("[%s%s ",
!prefixcmp(head, "refs/heads/") ?
head + 11 :
diff --git a/builtin/fsck.c b/builtin/fsck.c
index 30d0dc8..8c479a7 100644
--- a/builtin/fsck.c
+++ b/builtin/fsck.c
@@ -563,7 +563,7 @@ static int fsck_head_link(void)
if (verbose)
fprintf(stderr, "Checking HEAD link\n");
- head_points_at = resolve_ref("HEAD", head_sha1, 0, &flag);
+ head_points_at = resolve_ref_unsafe("HEAD", head_sha1, 0, &flag);
if (!head_points_at)
return error("Invalid HEAD");
if (!strcmp(head_points_at, "HEAD"))
diff --git a/builtin/log.c b/builtin/log.c
index 4395f3e..89d0cc0 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -1040,7 +1040,7 @@ static char *find_branch_name(struct rev_info *rev)
if (positive < 0)
return NULL;
strbuf_addf(&buf, "refs/heads/%s", rev->cmdline.rev[positive].name);
- branch = resolve_ref(buf.buf, branch_sha1, 1, NULL);
+ branch = resolve_ref_unsafe(buf.buf, branch_sha1, 1, NULL);
if (!branch ||
prefixcmp(branch, "refs/heads/") ||
hashcmp(rev->cmdline.rev[positive].item->sha1, branch_sha1))
@@ -1268,7 +1268,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
rev.pending.objects[0].item->flags |= UNINTERESTING;
add_head_to_pending(&rev);
- ref = resolve_ref("HEAD", sha1, 1, NULL);
+ ref = resolve_ref_unsafe("HEAD", sha1, 1, NULL);
if (ref && !prefixcmp(ref, "refs/heads/"))
branch_name = xstrdup(ref + strlen("refs/heads/"));
else
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 5cd6fc7..a1a4b09 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -571,7 +571,7 @@ static void check_aliased_update(struct command *cmd, struct string_list *list)
int flag;
strbuf_addf(&buf, "%s%s", get_git_namespace(), cmd->ref_name);
- dst_name = resolve_ref(buf.buf, sha1, 0, &flag);
+ dst_name = resolve_ref_unsafe(buf.buf, sha1, 0, &flag);
strbuf_release(&buf);
if (!(flag & REF_ISSYMREF))
diff --git a/builtin/remote.c b/builtin/remote.c
index 407abfb..583eec9 100644
--- a/builtin/remote.c
+++ b/builtin/remote.c
@@ -573,7 +573,7 @@ static int read_remote_branches(const char *refname,
strbuf_addf(&buf, "refs/remotes/%s/", rename->old);
if (!prefixcmp(refname, buf.buf)) {
item = string_list_append(rename->remote_branches, xstrdup(refname));
- symref = resolve_ref(refname, orig_sha1, 1, &flag);
+ symref = resolve_ref_unsafe(refname, orig_sha1, 1, &flag);
if (flag & REF_ISSYMREF)
item->util = xstrdup(symref);
else
diff --git a/builtin/show-branch.c b/builtin/show-branch.c
index a1f148e..a59e088 100644
--- a/builtin/show-branch.c
+++ b/builtin/show-branch.c
@@ -789,7 +789,7 @@ int cmd_show_branch(int ac, const char **av, const char *prefix)
}
}
- head_p = resolve_ref("HEAD", head_sha1, 1, NULL);
+ head_p = resolve_ref_unsafe("HEAD", head_sha1, 1, NULL);
if (head_p) {
head_len = strlen(head_p);
memcpy(head, head_p, head_len + 1);
diff --git a/builtin/symbolic-ref.c b/builtin/symbolic-ref.c
index dea849c..2ef5962 100644
--- a/builtin/symbolic-ref.c
+++ b/builtin/symbolic-ref.c
@@ -12,7 +12,7 @@ static void check_symref(const char *HEAD, int quiet)
{
unsigned char sha1[20];
int flag;
- const char *refs_heads_master = resolve_ref(HEAD, sha1, 0, &flag);
+ const char *refs_heads_master = resolve_ref_unsafe(HEAD, sha1, 0, &flag);
if (!refs_heads_master)
die("No such ref: %s", HEAD);
diff --git a/cache.h b/cache.h
index ba5e911..051e7ee 100644
--- a/cache.h
+++ b/cache.h
@@ -865,7 +865,7 @@ extern int read_ref(const char *filename, unsigned char *sha1);
*
* errno is sometimes set on errors, but not always.
*/
-#define resolve_ref(ref, sha1, reading, flag) resolve_ref_real(ref, sha1, reading, flag, __FILE__, __LINE__)
+#define resolve_ref_unsafe(ref, sha1, reading, flag) resolve_ref_real(ref, sha1, reading, flag, __FILE__, __LINE__)
extern const char *resolve_ref_real(const char *ref, unsigned char *sha1, int reading, int *flag, const char *file, int line);
extern char *resolve_refdup(const char *ref, unsigned char *sha1, int reading, int *flag);
diff --git a/refs.c b/refs.c
index cf8dfcc..010bb72 100644
--- a/refs.c
+++ b/refs.c
@@ -361,7 +361,7 @@ static int warn_if_dangling_symref(const char *refname, const unsigned char *sha
if (!(flags & REF_ISSYMREF))
return 0;
- resolves_to = resolve_ref(refname, junk, 0, NULL);
+ resolves_to = resolve_ref_unsafe(refname, junk, 0, NULL);
if (!resolves_to || strcmp(resolves_to, d->refname))
return 0;
@@ -616,7 +616,7 @@ const char *resolve_ref_real(const char *ref, unsigned char *sha1,
char *resolve_refdup(const char *ref, unsigned char *sha1, int reading, int *flag)
{
- const char *ret = resolve_ref(ref, sha1, reading, flag);
+ const char *ret = resolve_ref_unsafe(ref, sha1, reading, flag);
return ret ? xstrdup(ret) : NULL;
}
@@ -629,7 +629,7 @@ struct ref_filter {
int read_ref_full(const char *ref, unsigned char *sha1, int reading, int *flags)
{
- if (resolve_ref(ref, sha1, reading, flags))
+ if (resolve_ref_unsafe(ref, sha1, reading, flags))
return 0;
return -1;
}
@@ -1126,7 +1126,7 @@ int dwim_ref(const char *str, int len, unsigned char *sha1, char **ref)
this_result = refs_found ? sha1_from_ref : sha1;
mksnpath(fullref, sizeof(fullref), *p, len, str);
- r = resolve_ref(fullref, this_result, 1, &flag);
+ r = resolve_ref_unsafe(fullref, this_result, 1, &flag);
if (r) {
if (!refs_found++)
*ref = xstrdup(r);
@@ -1156,7 +1156,7 @@ int dwim_log(const char *str, int len, unsigned char *sha1, char **log)
const char *ref, *it;
mksnpath(path, sizeof(path), *p, len, str);
- ref = resolve_ref(path, hash, 1, NULL);
+ ref = resolve_ref_unsafe(path, hash, 1, NULL);
if (!ref)
continue;
if (!stat(git_path("logs/%s", path), &st) &&
@@ -1192,7 +1192,7 @@ static struct ref_lock *lock_ref_sha1_basic(const char *ref, const unsigned char
lock = xcalloc(1, sizeof(struct ref_lock));
lock->lock_fd = -1;
- ref = resolve_ref(ref, lock->old_sha1, mustexist, &type);
+ ref = resolve_ref_unsafe(ref, lock->old_sha1, mustexist, &type);
if (!ref && errno == EISDIR) {
/* we are trying to lock foo but we used to
* have foo/bar which now does not exist;
@@ -1205,7 +1205,7 @@ static struct ref_lock *lock_ref_sha1_basic(const char *ref, const unsigned char
error("there are still refs under '%s'", orig_ref);
goto error_return;
}
- ref = resolve_ref(orig_ref, lock->old_sha1, mustexist, &type);
+ ref = resolve_ref_unsafe(orig_ref, lock->old_sha1, mustexist, &type);
}
if (type_p)
*type_p = type;
@@ -1368,7 +1368,7 @@ int rename_ref(const char *oldref, const char *newref, const char *logmsg)
if (log && S_ISLNK(loginfo.st_mode))
return error("reflog for %s is a symlink", oldref);
- symref = resolve_ref(oldref, orig_sha1, 1, &flag);
+ symref = resolve_ref_unsafe(oldref, orig_sha1, 1, &flag);
if (flag & REF_ISSYMREF)
return error("refname %s is a symbolic ref, renaming it is not supported",
oldref);
@@ -1657,7 +1657,7 @@ int write_ref_sha1(struct ref_lock *lock,
unsigned char head_sha1[20];
int head_flag;
const char *head_ref;
- head_ref = resolve_ref("HEAD", head_sha1, 1, &head_flag);
+ head_ref = resolve_ref_unsafe("HEAD", head_sha1, 1, &head_flag);
if (head_ref && (head_flag & REF_ISSYMREF) &&
!strcmp(head_ref, lock->ref_name))
log_ref_write("HEAD", lock->old_sha1, sha1, logmsg);
@@ -1994,7 +1994,7 @@ int update_ref(const char *action, const char *refname,
int ref_exists(const char *refname)
{
unsigned char sha1[20];
- return !!resolve_ref(refname, sha1, 1, NULL);
+ return !!resolve_ref_unsafe(refname, sha1, 1, NULL);
}
struct ref *find_ref_by_name(const struct ref *list, const char *name)
diff --git a/remote.c b/remote.c
index 6655bb0..73a3809 100644
--- a/remote.c
+++ b/remote.c
@@ -482,7 +482,7 @@ static void read_config(void)
return;
default_remote_name = xstrdup("origin");
current_branch = NULL;
- head_ref = resolve_ref("HEAD", sha1, 0, &flag);
+ head_ref = resolve_ref_unsafe("HEAD", sha1, 0, &flag);
if (head_ref && (flag & REF_ISSYMREF) &&
!prefixcmp(head_ref, "refs/heads/")) {
current_branch =
@@ -1007,7 +1007,7 @@ static char *guess_ref(const char *name, struct ref *peer)
struct strbuf buf = STRBUF_INIT;
unsigned char sha1[20];
- const char *r = resolve_ref(peer->name, sha1, 1, NULL);
+ const char *r = resolve_ref_unsafe(peer->name, sha1, 1, NULL);
if (!r)
return NULL;
@@ -1058,7 +1058,7 @@ static int match_explicit(struct ref *src, struct ref *dst,
unsigned char sha1[20];
int flag;
- dst_value = resolve_ref(matched_src->name, sha1, 1, &flag);
+ dst_value = resolve_ref_unsafe(matched_src->name, sha1, 1, &flag);
if (!dst_value ||
((flag & REF_ISSYMREF) &&
prefixcmp(dst_value, "refs/heads/")))
diff --git a/test-resolve-ref.c b/test-resolve-ref.c
index b772038..59d04e0 100644
--- a/test-resolve-ref.c
+++ b/test-resolve-ref.c
@@ -11,8 +11,8 @@ int main(int argc, char **argv)
* function returns a shared buffer, so by the time the second
* call is made, ref1 must _not_ be accessed any more.
*/
- ref1 = resolve_ref("HEAD", sha1, 0, NULL);
- ref2 = resolve_ref("HEAD", sha1, 0, NULL);
+ ref1 = resolve_ref_unsafe("HEAD", sha1, 0, NULL);
+ ref2 = resolve_ref_unsafe("HEAD", sha1, 0, NULL);
printf("ref1 = %s\nref2 = %s\n", ref1, ref2);
return 0;
}
diff --git a/transport.c b/transport.c
index 51814b5..e9797c0 100644
--- a/transport.c
+++ b/transport.c
@@ -163,7 +163,7 @@ static void set_upstreams(struct transport *transport, struct ref *refs,
/* Follow symbolic refs (mainly for HEAD). */
localname = ref->peer_ref->name;
remotename = ref->name;
- tmp = resolve_ref(localname, sha, 1, &flag);
+ tmp = resolve_ref_unsafe(localname, sha, 1, &flag);
if (tmp && flag & REF_ISSYMREF &&
!prefixcmp(tmp, "refs/heads/"))
localname = tmp;
--
1.7.8.36.g69ee2
^ permalink raw reply related
* Re: [PATCH v2 08/51] is_dup_ref(): extract function from sort_ref_array()
From: Michael Haggerty @ 2011-12-12 11:44 UTC (permalink / raw)
To: Jeff King
Cc: Junio C Hamano, git, Drew Northup, Jakub Narebski, Heiko Voigt,
Johan Herland, Julian Phillips
In-Reply-To: <20111212083345.GA16106@sigill.intra.peff.net>
On 12/12/2011 09:33 AM, Jeff King wrote:
> On Mon, Dec 12, 2011 at 06:38:15AM +0100, mhagger@alum.mit.edu wrote:
>
>> +/*
>> + * Emit a warning and return true iff ref1 and ref2 have the same name
>> + * and the same sha1. Die if they have the same name but different
>> + * sha1s.
>> + */
>> +static int is_dup_ref(const struct ref_entry *ref1, const struct ref_entry *ref2)
>> +{
>> + if (!strcmp(ref1->name, ref2->name)) {
>> + /* Duplicate name; make sure that the SHA1s match: */
>> + if (hashcmp(ref1->sha1, ref2->sha1))
>> + die("Duplicated ref, and SHA1s don't match: %s",
>> + ref1->name);
>> + warning("Duplicated ref: %s", ref1->name);
>> + return 1;
>> + } else {
>> + return 0;
>> + }
>> +}
>
> As a user, I'm not sure what this message means. If I understand the
> context right, this happens when you have duplicate entries in your
> packed-refs file?
It could be caused by dups in any of the three ref caches: packed refs,
loose refs, or extra refs.
Duplicates in the packed-refs file could be caused by any garden-variety
data corruption external to git. Or they could be caused by a bug in
git that caused it to write duplicates. But the code that writes packed
refs iterates over the refs right after they are sorted, and sorting
eliminates duplicates, so this would be an unlikely bug the way the code
is currently organized. I suppose they could also be caused by the
simultaneous writing of the packed-refs file by another process that
doesn't respect git's lock file; perhaps an ill-timed rsync or something.
Normally there should be no duplicate loose refs, because their names
are taken directly from the filesystem and duplicate filenames are not
allowed. (Though I don't know if readdir() guarantees an atomic
snapshot of a directory; if not, then a dup could perhaps be created by
having another process add and remove files while git is reading a loose
ref directory.)
Extra refs are created locally by other git code that I am not familiar
with, so duplicate extra refs could only be created by a bug in git.
So in summary, duplicates could be caused by a git bug, by a corrupt
packed-refs file, or conceivably by a race condition with another
process that is mutating the packed-refs file or the loose reference
directories.
> This would indicate a bug in git, so should this perhaps say "BUG:" in
> front, or maybe give some more context so that a user understands it is
> not their error, or even a random error on this run, but that git has
> accidentally corrupted the packed-refs file (and their best bet is
> probably to report the bug to us).
>
> This is obviously not an issue introduced by your patch, as you are
> just moving these error messages around. But maybe while the topic is
> fresh in your mind it is a good time to improve it. I dunno. AFAICT
> nobody has ever actually hit this message, so maybe it doesn't matter.
> :)
The first of Google search results for the fatal error message only
shows one instance where the error was observed [1]. This was a case of
cloning using the rsync protocol, which I believe is now deprecated
(probably for exactly this reason).
I think that the gold-plated way to improve the error message would be
to pass the error back to the caller, who would have more context to
decide the most likely cause of the duplicate and give an appropriate
warning or error message. But I think we can afford to wait until more
error reports appear before bothering with this.
Michael
[1] http://kerneltrap.org/mailarchive/git/2008/12/17/4438764
--
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/
^ permalink raw reply
* Re: [PATCH] tag deletions not rejected with receive.denyDeletes= true
From: Jerome DE VIVIE @ 2011-12-12 14:06 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <6271653.2751323698446271.JavaMail.root@promailix.prometil.com>
Junio C Hamano <gitster@pobox.com> writes :
> Our documentation can also use some updates, as it dates to the days back
> when we more liberally used "refs" and "branches" interchangeably.
Ok, I have turned the patch below for documentation.
For protecting tags, I can do it with triggers but its painful with lots of repositories. I propose to extend receive.denyDeletes with these values:
- "false"/"none" (existing behavior)
- "true"/"branches" (existing behavior)
- "tags": protect tags only
- "all": protect both tags and branches
Your opinion ?
BR
Jérôme
Signed-off-by: Jerome de Vivie <jedevivie-ext@airfrance.fr>
---
Documentation/config.txt | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/Documentation/config.txt b/Documentation/config.txt
index 5a841da..9c7c7fe 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1642,7 +1642,7 @@ receive.unpackLimit::
receive.denyDeletes::
If set to true, git-receive-pack will deny a ref update that deletes
- the ref. Use this to prevent such a ref deletion via a push.
+ a branch. Use this to prevent such a branch deletion via a push.
receive.denyDeleteCurrent::
If set to true, git-receive-pack will deny a ref update that
--
1.7.6.msysgit.0
^ permalink raw reply related
* Re: [RFC/PATCH] show tracking branches with their associated branch names
From: Santhosh @ 2011-12-12 14:19 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Vincent van Ravesteijn, git
In-Reply-To: <7vsjkq5h0i.fsf@alter.siamese.dyndns.org>
On Mon, Dec 12, 2011 at 12:44 PM, Junio C Hamano <gitster@pobox.com> wrote:
>
> It makes sense to make the various information available, but it does not
> mean it makes sense to add it by default for everybody at all. Given that
> against all common sense, many newbie web-tips and third-party documents
> suggest "git branch | sed -ne 's/^\* //p'" as a way to find the current
> branch in scripts, I am sure such a change will cause trouble to many
> while only helping a few.
>
You certainly have a point.
> I wouldn't mind a new option --verbose-format=... that takes various
> formatting letters similar to how --pretty=format:... does, though.
>
I will explore this option and see if it is worth the trouble.
Probably I can accomplish this locally with an alias.
~Santhosh.
^ permalink raw reply
* Re: Breakage (?) in configure and git_vsnprintf()
From: Jeff King @ 2011-12-12 14:25 UTC (permalink / raw)
To: Andreas Schwab
Cc: Michael Haggerty, Junio C Hamano, git discussion list,
Michal Rokos, Brandon Casey
In-Reply-To: <m262hmgmrw.fsf@igel.home>
On Mon, Dec 12, 2011 at 10:30:27AM +0100, Andreas Schwab wrote:
> Jeff King <peff@peff.net> writes:
>
> > if (maxsize > 0) {
> > - ret = vsnprintf(str, maxsize-SNPRINTF_SIZE_CORR, format, ap);
> > + va_copy(cp, ap);
> > + ret = vsnprintf(str, maxsize-SNPRINTF_SIZE_CORR, format, cp);
>
> You also need to call va_end on the copy.
Silly me. Thanks for catching.
Junio, here's a corrected version.
-- >8 --
Subject: [PATCH] compat/snprintf: don't look at va_list twice
If you define SNPRINTF_RETURNS_BOGUS, we use a special
git_vsnprintf wrapper assumes that vsnprintf returns "-1"
instead of the number of characters that you would need to
store the result.
To do this, it invokes vsnprintf multiple times, growing a
heap buffer until we have enough space to hold the result.
However, this means we evaluate the va_list parameter
multiple times, which is generally a bad thing (it may be
modified by calls to vsnprintf, yielding undefined
behavior).
Instead, we must va_copy it and hand the copy to vsnprintf,
so we always have a pristine va_list.
Signed-off-by: Jeff King <peff@peff.net>
---
compat/snprintf.c | 9 +++++++--
1 files changed, 7 insertions(+), 2 deletions(-)
diff --git a/compat/snprintf.c b/compat/snprintf.c
index e1e0e75..42ea1ac 100644
--- a/compat/snprintf.c
+++ b/compat/snprintf.c
@@ -19,11 +19,14 @@
#undef vsnprintf
int git_vsnprintf(char *str, size_t maxsize, const char *format, va_list ap)
{
+ va_list cp;
char *s;
int ret = -1;
if (maxsize > 0) {
- ret = vsnprintf(str, maxsize-SNPRINTF_SIZE_CORR, format, ap);
+ va_copy(cp, ap);
+ ret = vsnprintf(str, maxsize-SNPRINTF_SIZE_CORR, format, cp);
+ va_end(cp);
if (ret == maxsize-1)
ret = -1;
/* Windows does not NUL-terminate if result fills buffer */
@@ -42,7 +45,9 @@ int git_vsnprintf(char *str, size_t maxsize, const char *format, va_list ap)
if (! str)
break;
s = str;
- ret = vsnprintf(str, maxsize-SNPRINTF_SIZE_CORR, format, ap);
+ va_copy(cp, ap);
+ ret = vsnprintf(str, maxsize-SNPRINTF_SIZE_CORR, format, cp);
+ va_end(cp);
if (ret == maxsize-1)
ret = -1;
}
--
1.7.8.13.g74677
^ permalink raw reply related
* Git blame only current branch
From: Stephen Bash @ 2011-12-12 15:24 UTC (permalink / raw)
To: git discussion list
In-Reply-To: <e9e35956-a091-4143-8fd4-3516b54263a6@mail>
Hi all-
I'm curious if there's a method to make git blame merge commits that introduce code to the given branch rather than commits on the original (topic) branch? For example:
A--B--C---M--D master
\ /
1--2--3 topicA
I'd like a mode where 'git blame master ...' shows commit M for lines changed by topicA so I can easily do 'git blame M^ ...' to see changes (on master) prior to the merge of topicA. Unfortunately 'git blame master ^topicA ...' blames all the changes of topicA to 3 (which reading the docs appears to be the "correct" behavior).
Thanks!
Stephen
^ permalink raw reply
* Re: [RFC/PATCH] add update to branch support for "floating submodules"
From: Andreas T.Auer @ 2011-12-12 15:34 UTC (permalink / raw)
To: Leif Gruenwoldt; +Cc: Junio C Hamano, git
In-Reply-To: <CALFF=ZQKRgx_AodBQH17T9cSe_JFtoKie7DoMMfkTXCyCFospw@mail.gmail.com>
On 10.12.2011 16:27 Leif Gruenwoldt wrote:
> On Sat, Dec 10, 2011 at 1:30 AM, Junio C Hamano <gitster@pobox.com>
> wrote:
>
> > So that use case does not sound like a good rationale to require
> > addition of floating submodules.
>
> Ok I will try another scenario :)
>
> Imagine again products A, B and C and a common library. The products
> are in a stable state of development and track a stable branch of
> the common lib. Then imagine an important security fix gets made to
> the common library. On the next pull of products A, B, and C they get
> this fix for free because they were floating. They didn't need to
> communicate with the maintainer of the common repo to know this. In
> fact they don't really care. They just want the latest stable code
> for that release branch.
So you don't want to have a stale submodule as Junio suggested, which is
older than the gitlinked commit in the superproject, but you want to
have the newest stable version, which is not yet gitlinked in the
superproject, right?
Wouldn't ( cd commonlib ; git pull stable ) instead of
git submodule update commonlib
work as you want?
To be able to configure this update behavior in .gitmodules for _some_
submodules, could be helpful in this case.
So you don't want to add a new commit to the products A, B and C repos
whenever the stable branch of the submodule changes, but on the other
hand when you commit changes to the products it would still make sense
to update the gitlink to the current commonlib version together with
your changes, too, right?
> This is how package management on many linux systems works.
> Dependencies get updated and all products reap the benefit (or
> catastrophe) automatically.
If I have e.g. the Debian testing distro, which is more floating than
the most other Linux distro releases, then I still get only those
versions of the packages that are referenced by this "Debian testing"
superproject, unless I specify a different superproject (e.g. "Debian
unstable") to get a newer version, but they are still tracked in some
superproject. I'm not aware of a way to get the newest version of a
package before it is in some "superproject", except downloading it
explictily somewhere else. But I don't think this is what you want.
^ permalink raw reply
* Re: Question about commit message wrapping
From: Holger Hellmuth @ 2011-12-12 16:37 UTC (permalink / raw)
To: Frans Klaver; +Cc: Andrew Ardill, Jakub Narebski, Sidney San Martín, git
In-Reply-To: <CAH6sp9NwyxZi6KR4U96=sWdiqCseyTLEDoHdw=y9hUx2kHwOpg@mail.gmail.com>
On 12.12.2011 09:41, Frans Klaver wrote:
>> Emails on this list are almost exclusively sent pre-wrapped to 80
>> character line lengths.
>> The result is exactly the kind of ragged output you used in your
>> example. Changing this behaviour may break backwards compatibility,
>> but it is already broken for 'future' compatibility.
I don't really see backwards compatibility broken either. At the moment
commit messages are usually pre-wrapped at 72 columns, which looks
perfect only on 80 column displays, ok on wider displays and bad on
narrow displays.
If the requirement to pre-wrap would fall and either 'git log' or 'less'
doing the wrap, old commit messages would still look perfect on 80
column, ok on wider and bad on narrow displays. Newer commit messages
would look good everywhere.
The only breakage would be that new long commit messages would look bad
on older git versions. Because of that the auto-wrap should be
implemented first and the "requirement" for 72 columns should fall in a
later version.
> I am starting to think that we need to somehow keep the current
> behavior, but override at smaller widths. Maybe even use format=flowed
> in format-patch.
Could you explain what overriding at smaller widths would achieve?
Supporting format=flowed would be nice though.
> On the other hand, the fundamental use with git is to
> communicate code, and I'm not sure how that [cw]ould be handled.
I prefer wrapped code to code that is cut of at a specific column.
Wrapped code has much less possibility for misinterpretation. Python
programmers might disagree?
I see your proposal mainly as an improvement to the display of texts,
not code.
^ permalink raw reply
* Re: Git blame only current branch
From: Jeff King @ 2011-12-12 16:55 UTC (permalink / raw)
To: Stephen Bash; +Cc: git discussion list
In-Reply-To: <d615954f-bed8-482d-a2e3-e1e741d6dd23@mail>
On Mon, Dec 12, 2011 at 10:24:47AM -0500, Stephen Bash wrote:
> I'm curious if there's a method to make git blame merge commits that
> introduce code to the given branch rather than commits on the original
> (topic) branch? For example:
Usually when you are interested in seeing merges like this in git-log,
you would use one of "--first-parent" or "--merges". However, though
"git blame" takes revision arguments, it does its own traversal of the
graph that does not respect those options.
Modifying it to do --first-parent is pretty easy:
diff --git a/builtin/blame.c b/builtin/blame.c
index 80febbe..c19a8cd 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -1191,6 +1191,8 @@ static int num_scapegoats(struct rev_info *revs, struct commit *commit)
{
int cnt;
struct commit_list *l = first_scapegoat(revs, commit);
+ if (revs->first_parent_only)
+ return l ? 1 : 0;
for (cnt = 0; l; l = l->next)
cnt++;
return cnt;
With that, "git blame --first-parent" produces reasonable results for
me. But of course I didn't do more than 30 seconds of testing, so it is
entirely possible there are corner cases or unforeseen side effects.
Handling --merges is probably a little trickier, as you need to consider
only some commits as scapegoats, but still traverse through everything
to find the merges.
-Peff
^ permalink raw reply related
* Re: Git blame only current branch
From: Stephen Bash @ 2011-12-12 17:05 UTC (permalink / raw)
To: Jeff King; +Cc: git discussion list
In-Reply-To: <20111212165542.GA4802@sigill.intra.peff.net>
----- Original Message -----
> From: "Jeff King" <peff@peff.net>
> Sent: Monday, December 12, 2011 11:55:42 AM
> Subject: Re: Git blame only current branch
>
> On Mon, Dec 12, 2011 at 10:24:47AM -0500, Stephen Bash wrote:
>
> > I'm curious if there's a method to make git blame merge commits
> > that introduce code to the given branch rather than commits on
> > the original (topic) branch? For example:
>
> Usually when you are interested in seeing merges like this in
> git-log, you would use one of "--first-parent" or "--merges".
> However, though "git blame" takes revision arguments, it does
> its own traversal of the graph that does not respect those
> options.
My first thought was --first-parent, and was disappointed when I didn't find it in the blame documentation :) I think for my purposes --first-parent is better than --merges because there are non-merge commits on the branch(es) of interest (and thus I think the problem would become ill-posed in the --merges case).
> Modifying it to do --first-parent is pretty easy:
> ... snip ...
That's pretty simple... I'll try to do a little testing this afternoon.
Thanks!
Stephen
^ permalink raw reply
* Re: [PATCH v2 08/51] is_dup_ref(): extract function from sort_ref_array()
From: Junio C Hamano @ 2011-12-12 17:14 UTC (permalink / raw)
To: Michael Haggerty
Cc: Jeff King, git, Drew Northup, Jakub Narebski, Heiko Voigt,
Johan Herland, Julian Phillips
In-Reply-To: <4EE5E918.8090104@alum.mit.edu>
Michael Haggerty <mhagger@alum.mit.edu> writes:
> Extra refs are created locally by other git code that I am not familiar
> with, so duplicate extra refs could only be created by a bug in git.
I think we already discussed this, but just in case you forgot. They are
used to tell git to treat as if objects in histories leading to certain
commits are complete and available, i.e. only their object names matter
and the refname field is primarily for debugging. It would be a grave
regression if you errored out seeing duplicate names in them.
^ permalink raw reply
* Re: [PATCH] tag deletions not rejected with receive.denyDeletes= true
From: Junio C Hamano @ 2011-12-12 17:16 UTC (permalink / raw)
To: Jerome DE VIVIE; +Cc: Junio C Hamano, git
In-Reply-To: <12967682.2821323698766430.JavaMail.root@promailix.prometil.com>
Jerome DE VIVIE <j.devivie@prometil.com> writes:
> Junio C Hamano <gitster@pobox.com> writes :
>> Our documentation can also use some updates, as it dates to the days back
>> when we more liberally used "refs" and "branches" interchangeably.
>
> Ok, I have turned the patch below for documentation.
Err,.. what I meant by "documentation update" is more like this.
Documentation/config.txt | 8 ++++----
1 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/Documentation/config.txt b/Documentation/config.txt
index 8a7d2d4..8eda8e4 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1653,15 +1653,15 @@ receive.unpackLimit::
`transfer.unpackLimit` is used instead.
receive.denyDeletes::
- If set to true, git-receive-pack will deny a ref update that deletes
- the ref. Use this to prevent such a ref deletion via a push.
+ If set to true, git-receive-pack will deny an update that deletes
+ the branch. Use this to prevent a push from deleting a branch.
receive.denyDeleteCurrent::
- If set to true, git-receive-pack will deny a ref update that
+ If set to true, git-receive-pack will deny an update that
deletes the currently checked out branch of a non-bare repository.
receive.denyCurrentBranch::
- If set to true or "refuse", git-receive-pack will deny a ref update
+ If set to true or "refuse", git-receive-pack will deny an update
to the currently checked out branch of a non-bare repository.
Such a push is potentially dangerous because it brings the HEAD
out of sync with the index and working tree. If set to "warn",
^ permalink raw reply related
* Re: Git blame only current branch
From: andreas.t.auer_gtml_37453 @ 2011-12-12 17:19 UTC (permalink / raw)
To: Stephen Bash; +Cc: Jeff King, git discussion list
In-Reply-To: <5e2440c1-8d11-4d92-b42f-14169a62ced1@mail>
On 12.12.2011 18:05 Stephen Bash wrote:
> ----- Original Message -----
> > From: "Jeff King" <peff@peff.net> Sent: Monday, December 12, 2011
> > 11:55:42 AM Subject: Re: Git blame only current branch
> >
> > On Mon, Dec 12, 2011 at 10:24:47AM -0500, Stephen Bash wrote:
> >
> > Usually when you are interested in seeing merges like this in
> > git-log, you would use one of "--first-parent" or "--merges".
> > However, though "git blame" takes revision arguments, it does its
> > own traversal of the graph that does not respect those options.
>
> My first thought was --first-parent, and was disappointed when I
> didn't find it in the blame documentation :) I think for my purposes
> --first-parent is better than --merges because there are non-merge
> commits on the branch(es) of interest (and thus I think the problem
> would become ill-posed in the --merges case).
>
> > Modifying it to do --first-parent is pretty easy: ... snip ...
>
> That's pretty simple... I'll try to do a little testing this
> afternoon.
You might need to consider that if the master branch was first merged
into topicA before topicA was merged back to the master that the master
would only be fast-forwarded and so the first parent of M would be 3 not
C. So depending how the developers merged you might get different results.
^ permalink raw reply
* Re: [RFC/PATCH] add update to branch support for "floating submodules"
From: Leif Gruenwoldt @ 2011-12-12 18:04 UTC (permalink / raw)
To: Andreas T.Auer; +Cc: Junio C Hamano, git
In-Reply-To: <4EE61EED.50604@ursus.ath.cx>
On Mon, Dec 12, 2011 at 10:34 AM, Andreas T.Auer
<andreas.t.auer_gtml_37453@ursus.ath.cx> wrote:
> So you don't want to have a stale submodule as Junio suggested, which is
> older than the gitlinked commit in the superproject, but you want to have
> the newest stable version, which is not yet gitlinked in the superproject,
> right?
Right.
> Wouldn't ( cd commonlib ; git pull stable ) instead of
> git submodule update commonlib
> work as you want?
Yes that's how we perform business now.
> To be able to configure this update behavior in .gitmodules for _some_
> submodules, could be helpful in this case.
Yes my thoughts exactly.
> So you don't want to add a new commit to the products A, B and C repos
> whenever the stable branch of the submodule changes, but on the other hand
> when you commit changes to the products it would still make sense to update
> the gitlink to the current commonlib version together with your changes,
> too, right?
Hmm I supose that does make sense. If the commonlib version was auto recorded
during a commit of the product it would be nice. Then if/when the user
reconfigured
the submodule from "floating" to "strict" mode it would then have a
submodule sha1
reference. I like how this sounds.
^ permalink raw reply
* Re: [PATCH 2/4] Convert resolve_ref+xstrdup to new resolve_refdup function
From: Junio C Hamano @ 2011-12-12 18:07 UTC (permalink / raw)
To: Nguyễn Thái Ngọc Duy; +Cc: git, Jonathan Nieder
In-Reply-To: <1323688832-32016-2-git-send-email-pclouds@gmail.com>
Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
Thanks.
The patch looks correct but I have a slight maintainability concern and a
suggestion for possible improvement.
> ...
> diff --git a/builtin/checkout.c b/builtin/checkout.c
> index b7c6302..a66d3eb 100644
> --- a/builtin/checkout.c
> +++ b/builtin/checkout.c
> @@ -696,17 +696,14 @@ static int switch_branches(struct checkout_opts *opts, struct branch_info *new)
> {
> int ret = 0;
> struct branch_info old;
> + char *path;
> unsigned char rev[20];
> int flag;
> memset(&old, 0, sizeof(old));
> - old.path = resolve_ref("HEAD", rev, 0, &flag);
> - if (old.path)
> - old.path = xstrdup(old.path);
> + old.path = path = resolve_refdup("HEAD", rev, 0, &flag);
This uses "one 'const char *' pointer that is used for reading data from
and an extra 'char *' pointer that is used only for freeing" approach,
which has two advantages and one disadvantage:
+ Obviously, we do not have to cast away constness when freeing.
+ A caller that follows the pattern could move the "for-data" pointer
without having to worry about affecting "for-freeing" pointer. It could
even do something like this:
char *for_freeing;
const char *for_data;
for_data = for_freeing = resolve_refdup(...);
if (prefixcmp(for_data, "refs/heads/"))
for_data = skip_prefix(for_data, "refs/heads/");
... do other things using for_data pointer ...
free(for_freeing);
- If the for-freeing and for-data pointers are named too similarly, the
code becomes harder to read. It is easy for a person who is new to the
codebase to start using the for-freeing pointer to manipulate the data
(mostly harmless) or even advance the pointer by mistake (bad).
A handful of places in the existing codebase use this "two pointers"
approach primarily for the second advantage. The way they avoid the
disadvantage is by naming the other pointer "$something_to_free" (and the
"$something_" part is optional if there is only one such variable in the
context) to make it clear that the variable is not meant to be looked at
in the code and its primary purpose is for cleaning up at the end.
Perhaps renaming this "path" to "to_free" (or "path_to_free" if you want
to hint the "path-ness" to the readers, but see below) would make it less
likely that future tweakers mistakenly look at, modify the string thru, or
worse yet move the pointer itself.
> diff --git a/builtin/merge.c b/builtin/merge.c
> index a1c8534..6437db2 100644
> --- a/builtin/merge.c
> +++ b/builtin/merge.c
> @@ -1096,6 +1096,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
> struct commit_list *common = NULL;
> const char *best_strategy = NULL, *wt_strategy = NULL;
> struct commit_list **remotes = &remoteheads;
> + char *branch_ref;
>
> if (argc == 2 && !strcmp(argv[1], "-h"))
> usage_with_options(builtin_merge_usage, builtin_merge_options);
> @@ -1104,12 +1105,9 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
> * Check if we are _not_ on a detached HEAD, i.e. if there is a
> * current branch.
> */
> - branch = resolve_ref("HEAD", head_sha1, 0, &flag);
> - if (branch) {
> - if (!prefixcmp(branch, "refs/heads/"))
> - branch += 11;
> - branch = xstrdup(branch);
> - }
> + branch = branch_ref = resolve_refdup("HEAD", head_sha1, 0, &flag);
Without s/branch_ref/to_free/ this part is unreadable and unmaintainable,
as it invites the "which variable should I use?" question.
It is more important to clarify that the "branch" variable is what the
code should look at and manipulate by *not* calling this for-freeing
pointer after what it contains (i.e. "branch").
When naming a "for-freeing" pointer variable, the kind of data the area of
memory happens to contain is of secondary importance. Being deliberately
vague about what the area of memory may contain is a good thing, because
it actively discourages the program from looking at the area via the
pointer if the variable is named "to_free" or something that does not
specify what it contains.
Other places in this patch that call the for-freeing variable "ref" share
the same issue but they are less specific than their for-data variable
counterpart, which makes it slightly less risky than this instance.
^ permalink raw reply
* [PATCH 1/3] test-terminal: give the child an empty stdin TTY
From: Thomas Rast @ 2011-12-12 18:09 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jeff King, Jonathan Nieder, Michael Haggerty
So far, test-terminal.perl did not care at all about the stdin (that
is, leave it as-is). This mostly works well, but git-shortlog is a
problem:
* It takes decisions based on isatty(0). (No test checks this, but
compare 'git shortlog </dev/null' with 'git shortlog' in a
terminal.)
* It reads all of stdin if !isatty(0) and no arguments were passed.
Because of the latter, t7006.58ff cause unexpected results if you do:
git rev-list <range> |
while read sha; do
git checkout sha
make test
done
If t7006 runs during any 'make test' run, the next 'read sha' will
fail (git-shortlog ate all of stdin already) and the while loop stops
immediately. In particular, this loop will only ever successfully
test *one* revision.
To fix this, change test-terminal.perl to open a third PTY for stdin,
send an EOF (Ctrl-D) immediately and close it later. Since this may
not be portable, we use POSIX::Termios to set it to Ctrl-D.
Noticed-by: Michael Haggerty <mhagger@alum.mit.edu>
Signed-off-by: Thomas Rast <trast@student.ethz.ch>
---
t/test-terminal.perl | 22 +++++++++++++++++++---
1 files changed, 19 insertions(+), 3 deletions(-)
diff --git a/t/test-terminal.perl b/t/test-terminal.perl
index ee01eb9..87b5a8c 100755
--- a/t/test-terminal.perl
+++ b/t/test-terminal.perl
@@ -4,14 +4,16 @@
use warnings;
use IO::Pty;
use File::Copy;
+use POSIX ();
-# Run @$argv in the background with stdio redirected to $out and $err.
+# Run @$argv in the background with stdio redirected from $in and to $out and $err.
sub start_child {
- my ($argv, $out, $err) = @_;
+ my ($argv, $in, $out, $err) = @_;
my $pid = fork;
if (not defined $pid) {
die "fork failed: $!"
} elsif ($pid == 0) {
+ open STDIN, "<&", $in;
open STDOUT, ">&", $out;
open STDERR, ">&", $err;
close $out;
@@ -64,13 +66,27 @@ sub copy_stdio {
or exit 1;
}
+sub set_default_eof_char {
+ my $fd = fileno shift;
+ my $termios = POSIX::Termios->new;
+ $termios->getattr($fd);
+ $termios->setcc(&POSIX::VEOF, 4);
+ $termios->setattr($fd, &POSIX::TCSANOW)
+ or die "Termios::setattr failed: $!";
+}
+
if ($#ARGV < 1) {
die "usage: test-terminal program args";
}
+my $master_in = new IO::Pty;
my $master_out = new IO::Pty;
my $master_err = new IO::Pty;
-my $pid = start_child(\@ARGV, $master_out->slave, $master_err->slave);
+set_default_eof_char($master_in->slave);
+my $pid = start_child(\@ARGV, $master_in->slave, $master_out->slave, $master_err->slave);
+close $master_in->slave;
close $master_out->slave;
close $master_err->slave;
+print $master_in "\cD";
copy_stdio($master_out, $master_err);
+close $master_in;
exit(finish_child($pid));
--
1.7.8.431.g2abf2
^ 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