* Re: [patch] color of branches in git status -sb
From: Nicolas Dudebout @ 2011-11-17 2:25 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <7v4ny3pn4v.fsf@alter.siamese.dyndns.org>
Please disregard this as a patch. I do not have the time to understand
how they have to be properly formatted. I just pasted the output of my
git client.
I am just letting you know that there is a problem that can be easily
fixed. I can resend a separate email if that makes it easier for your
bug tracking.
On Wed, Nov 16, 2011 at 8:13 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Nicolas Dudebout <nicolas.dudebout@gmail.com> writes:
>
>> The following patch fixes the fact that two colors of the status
>> output could not be configured in .git/config.
>>
>> The color of the current branch could not be modified because of the
>> name WT_STATUS_ONBRANCH having been changed to WT_STATUS_LOCAL_BRANCH.
>>
>> The color of the remote branch was not defined at all.
>>
>> By the way, when I do 'git status' instead of git 'status -sb' the
>> local and remote branch do not get colored. Is this a desired
>> behavior?
>>
>> Nicolas
>
> Please follow Documentation/SubmittingPatches.
>
> Also expect that patches to add new feature this deep in the pre-release
> feature freeze period is not likely to get reviewed by regular list
> members.
>
>>
>> Modified Documentation/config.txt
>
> Don't do this. We can tell what is modified from "diff --git" lines.
>
>> diff --git a/Documentation/config.txt b/Documentation/config.txt
>> index 5a841da..7a0ddd6 100644
>> --- a/Documentation/config.txt
>> +++ b/Documentation/config.txt
>> @@ -809 +809,2 @@ color.status.<slot>::
>> - `branch` (the current branch), or
>> + `branch` (the current branch),
>> + `remote` (the remote branch) or
>
> Don't do this. Proper patches have context lines for good reasons.
>
>> Modified builtin/commit.c
>> diff --git a/builtin/commit.c b/builtin/commit.c
>> index c46f2d1..4674600 100644
>> --- a/builtin/commit.c
>> +++ b/builtin/commit.c
>> @@ -1118 +1118,3 @@ static int parse_status_slot(const char *var, int offset)
>> - return WT_STATUS_ONBRANCH;
>> + return WT_STATUS_LOCAL_BRANCH;
>> + if (!strcasecmp(var+offset, "remote"))
>> + return WT_STATUS_REMOTE_BRANCH;
>
^ permalink raw reply
* Re: git clone --reference not working
From: Michael Haggerty @ 2011-11-17 4:49 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Andrea Arcangeli, git
In-Reply-To: <7vobwbpnzr.fsf@alter.siamese.dyndns.org>
On 11/17/2011 01:54 AM, Junio C Hamano wrote:
> Andrea Arcangeli <aarcange@redhat.com> writes:
>
>> latest git.git won't clone linux upstream with --reference. Those
>> v*^{} tags breaks it. What's that stuff anyway, looks totally ugly
>> (two commits with same data contents and header) bah.
>
> They point at commits they tag, and are essential for auto-following. They
> have been there forever in ls-remote output and they are not the real
> problem.
>
> A recent topic that was merged at 9bd5000 tightened the refname checking
> code without thinking and started to needlessly barf upon seeing them. I
> think we have discussed about the issue on the list, but I do not think
> there were fixes yet.
>
> Thanks for reminding.
>
> Michael, how does this look?
>
> -- >8 --
> Subject: refs: loosen over-strict "format" check
> [...]
I reviewed the patch (and ran the test suite here for good measure).
Looks good.
>From SubmittingPatches it looks like I should authorize
Reviewed-by: Michael Haggerty <mhagger@alum.mit.edu>
Is there a standard way to do so?
Michael
--
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/
^ permalink raw reply
* Suggestions for gitk
From: Michael Haggerty @ 2011-11-17 5:53 UTC (permalink / raw)
To: git discussion list, Paul Mackerras
I use gitk every day and love it. I have a few suggestions that IMO
would make it even better. Please note that I've been collecting these
ideas for a while, and they are only small niggles about an excellent tool.
1. Add a "HEAD" button
Frequently I want to jump to the HEAD revision after having moved around
in the history for a while. Since HEAD is not always at the top of the
revision list, I do this by typing "HEAD" into the "SHA1 ID" field. Is
there a better way already? If not, I think it would be very convenient
to have a small "HEAD" button near that field that causes gitk to jump
there.
2. Option to check out new branch
After I "Create new branch", I often want to check out the new branch
(the equivalent of "git checkout -b BRANCH REV"). So it would be
convenient if the dialog opened by right-clicking on a commit and select
"Create new branch" offered the option to check out the newly-created
branch. For example, it could have a third confirmation button "Create
and check out", or it could have a tick box "Check out new branch".
3. Allow "Branches: many (N)" to be expanded
There is some limit above which the commit window doesn't list the
branches that contain the commit, but instead displays "Branches: many
(N)". But sometimes one wants to know anyway. For example, the field
could be a link that one could click on to open a dialog box listing all
of the branches containing the commit.
In fact, a tabular format would often be more convenient than the
current format (which is hard to skim through due to its horizontal
layout and often scrolls off the right side of the screen) even if there
are only a few branches, and the same applies to "Tags", "Precedes",
etc. So perhaps this dialog could be made available in all cases, even
if the branches/tags/etc are expanded in the commit summary.
4. Hide "commit" part of window
When I'm trying to get an overview of the history, I am often not
interested in the commit summary. So it would be great if there were a
hotkey to hide the "commit" part of the window and allow the "log" part
of the window to use the whole surface. This would be like
Thunderbird's "F8" key (which I also use all the time), so if this
feature is implemented you might consider using "F8" for it.
5. Tab completion in "SHA1 ID" field
It would be wonderful if the SHA1 ID field supported some kind of tab
completion for branch names, similar to that offered by the git mode for
bash. Currently it is painful to type a long branch name into this field.
6. Display "git log" command line
I assume that gitk is using something like "git log" to generate the
list of commits that it displays. The "git log" command line can be
configured quite flexibly using the "F4" dialog. So flexibly, in fact,
that I often wonder what options it is passing to "git log". I think it
would be cool if gitk would display the "git log" command line that
corresponds to the options currently selected in the "F4" dialog.
7. Tags squeeze out commit message in the "log" window
If a commit has a tag, the tag is listed next in the first column of the
"log" window, pushing the first line of the commit message off to the
right. If a commit has multiple tags, it can easily happen that the
tags push the commit's log message completely out of the window. There
is no scrollbar, so there is no way to see the commit message in this
situation.
The inability to read the commit message is not such a problem, because
one can select the commit and see the commit message in the bottom part
of the window. More frustrating is that in this situation, it is
impossible to get to the menu that is normally accessed by
right-clicking on the commit message.
So perhaps the menu could also be made available elsewhere, for example
by right-clicking on the blue dot on the "graph" part of the display,
and/or by right clicking on author and date associated with the commit.
I don't know tcl/tk; otherwise I'd try to make some of these changes by
myself...
Thanks for listening,
Michael
--
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/
^ permalink raw reply
* Re: git clone --reference not working
From: Junio C Hamano @ 2011-11-17 5:55 UTC (permalink / raw)
To: Michael Haggerty; +Cc: Andrea Arcangeli, git
In-Reply-To: <4EC4926D.5050004@alum.mit.edu>
Michael Haggerty <mhagger@alum.mit.edu> writes:
> On 11/17/2011 01:54 AM, Junio C Hamano wrote:
> ...
> Looks good.
>
>>From SubmittingPatches it looks like I should authorize
>
> Reviewed-by: Michael Haggerty <mhagger@alum.mit.edu>
>
> Is there a standard way to do so?
That's perfect. I asked for an extra set of eyeballs from somebody who is
familiar with the codepath that had a problem, and that person eyeballed
and found the change to be an appropriate fix to the problem.
Either Acked-by or Reviewed-by is fine by me.
As a tentative measure, for tonight's pushout, I am inclined to queue an
equivalent of this patch on top of both mh/ref-api-2 and mh/ref-api-3
topic and merge them to 'next' and 'pu'. I'd appreciate if you can double
check the two merges on master..pu after I push them out in a few hours.
Thanks.
^ permalink raw reply
* [PATCH] pack-object: tolerate broken packs that have duplicated objects
From: Junio C Hamano @ 2011-11-17 6:04 UTC (permalink / raw)
To: git; +Cc: Shawn O. Pearce
When --reuse-delta is in effect (which is the default), and an existing
pack in the repository has the same object registered twice (e.g. one copy
in a non-delta format and the other copy in a delta against some other
object), an attempt to repack the repository can result in a cyclic delta
dependency, causing write_one() function to infinitely recurse into
itself.
Detect such a case and break the loopy dependency by writing out an object
that is involved in such a loop in the non-delta format.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
* This may need to be cherry-picked to earlier maintenance series. The
topic I queued tonight is built on v1.7.6.4.
builtin/pack-objects.c | 55 +++++++++++++++++++++++++++++++++++++----------
1 files changed, 43 insertions(+), 12 deletions(-)
diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index c6e2d87..5ae808b 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -404,25 +404,56 @@ static unsigned long write_object(struct sha1file *f,
return hdrlen + datalen;
}
-static int write_one(struct sha1file *f,
- struct object_entry *e,
- off_t *offset)
+enum write_one_status {
+ WRITE_ONE_SKIP = -1, /* already written */
+ WRITE_ONE_BREAK = 0, /* writing this will bust the limit; not written */
+ WRITE_ONE_WRITTEN = 1, /* normal */
+ WRITE_ONE_RECURSIVE = 2 /* already scheduled to be written */
+};
+
+static enum write_one_status write_one(struct sha1file *f,
+ struct object_entry *e,
+ off_t *offset)
{
unsigned long size;
+ int recursing;
- /* offset is non zero if object is written already. */
- if (e->idx.offset || e->preferred_base)
- return -1;
+ /*
+ * we set offset to 1 (which is an impossible value) to mark
+ * the fact that this object is involved in "write its base
+ * first before writing a deltified object" recursion.
+ */
+ recursing = (e->idx.offset == 1);
+ if (recursing) {
+ warning("recursive delta detected for object %s",
+ sha1_to_hex(e->idx.sha1));
+ return WRITE_ONE_RECURSIVE;
+ } else if (e->idx.offset || e->preferred_base) {
+ /* offset is non zero if object is written already. */
+ return WRITE_ONE_SKIP;
+ }
/* if we are deltified, write out base object first. */
- if (e->delta && !write_one(f, e->delta, offset))
- return 0;
+ if (e->delta) {
+ e->idx.offset = 1; /* now recurse */
+ switch (write_one(f, e->delta, offset)) {
+ case WRITE_ONE_RECURSIVE:
+ /* we cannot depend on this one */
+ e->delta = NULL;
+ break;
+ default:
+ break;
+ case WRITE_ONE_BREAK:
+ e->idx.offset = recursing;
+ return WRITE_ONE_BREAK;
+ }
+ }
e->idx.offset = *offset;
size = write_object(f, e, *offset);
if (!size) {
- e->idx.offset = 0;
- return 0;
+ e->idx.offset = recursing;
+ return WRITE_ONE_BREAK;
}
written_list[nr_written++] = &e->idx;
@@ -430,7 +461,7 @@ static int write_one(struct sha1file *f,
if (signed_add_overflows(*offset, size))
die("pack too large for current definition of off_t");
*offset += size;
- return 1;
+ return WRITE_ONE_WRITTEN;
}
static void write_pack_file(void)
@@ -468,7 +499,7 @@ static void write_pack_file(void)
offset = sizeof(hdr);
nr_written = 0;
for (; i < nr_objects; i++) {
- if (!write_one(f, objects + i, &offset))
+ if (write_one(f, objects + i, &offset) == WRITE_ONE_BREAK)
break;
display_progress(progress_state, written);
}
--
1.7.8.rc2.109.g72037
^ permalink raw reply related
* [PATCH] receive-pack, fetch-pack: reject bogus pack that records objects twice
From: Junio C Hamano @ 2011-11-17 6:04 UTC (permalink / raw)
To: git; +Cc: Shawn O. Pearce
When receive-pack & fetch-pack are run and store the pack obtained over
the wire to a local repository, they internally run the index-pack command
with the --strict option. Make sure that we reject incoming packfile that
records objects twice to avoid spreading such a damage.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
* Passing --strict from fetch-pack actually is a recent invention, so
this will be only useful to 1.7.8 and later.
builtin/index-pack.c | 4 +++-
object.c | 2 ++
pack-write.c | 4 ++++
pack.h | 3 ++-
4 files changed, 11 insertions(+), 2 deletions(-)
diff --git a/builtin/index-pack.c b/builtin/index-pack.c
index 0945adb..98025da 100644
--- a/builtin/index-pack.c
+++ b/builtin/index-pack.c
@@ -1122,8 +1122,10 @@ int cmd_index_pack(int argc, const char **argv, const char *prefix)
if (!index_name)
die("--verify with no packfile name given");
read_idx_option(&opts, index_name);
- opts.flags |= WRITE_IDX_VERIFY;
+ opts.flags |= WRITE_IDX_VERIFY | WRITE_IDX_STRICT;
}
+ if (strict)
+ opts.flags |= WRITE_IDX_STRICT;
curr_pack = open_pack_file(pack_name);
parse_pack_header();
diff --git a/object.c b/object.c
index 31976b5..d8d09f9 100644
--- a/object.c
+++ b/object.c
@@ -149,6 +149,8 @@ struct object *parse_object_buffer(const unsigned char *sha1, enum object_type t
struct tree *tree = lookup_tree(sha1);
if (tree) {
obj = &tree->object;
+ if (!tree->buffer)
+ tree->object.parsed = 0;
if (!tree->object.parsed) {
if (parse_tree_buffer(tree, buffer, size))
return NULL;
diff --git a/pack-write.c b/pack-write.c
index 9cd3bfb..f84adde 100644
--- a/pack-write.c
+++ b/pack-write.c
@@ -129,6 +129,10 @@ const char *write_idx_file(const char *index_name, struct pack_idx_entry **objec
}
sha1write(f, obj->sha1, 20);
git_SHA1_Update(&ctx, obj->sha1, 20);
+ if ((opts->flags & WRITE_IDX_STRICT) &&
+ (i && !hashcmp(list[-2]->sha1, obj->sha1)))
+ die("The same object %s appears twice in the pack",
+ sha1_to_hex(obj->sha1));
}
if (index_version >= 2) {
diff --git a/pack.h b/pack.h
index 722a54e..aca4739 100644
--- a/pack.h
+++ b/pack.h
@@ -37,7 +37,8 @@ struct pack_header {
struct pack_idx_option {
unsigned flags;
/* flag bits */
-#define WRITE_IDX_VERIFY 01
+#define WRITE_IDX_VERIFY 01 /* verify only, do not write the idx file */
+#define WRITE_IDX_STRICT 02
uint32_t version;
uint32_t off32_limit;
--
1.7.8.rc2.109.g72037
^ permalink raw reply related
* Re: Git 1.7.5 problem with HTTPS
From: Dmitry Smirnov @ 2011-11-17 6:36 UTC (permalink / raw)
To: git
In-Reply-To: <CACf55T6BGds_D=nbb8G=m+Jwr+bHFruCs-Q0+FOO+WXitXEJ-g@mail.gmail.com>
I had fixed the problem by manually installing the most recent version
of the libcurl3-gnutls for Ubuntu (from precise):
http://packages.ubuntu.com/precise/libcurl3-gnutls
It will require also most recent libgnutls:
http://packages.ubuntu.com/precise/libgnutls26
Dmitry
^ permalink raw reply
* What's cooking in git.git (Nov 2011, #04; Wed, 16)
From: Junio C Hamano @ 2011-11-17 6:46 UTC (permalink / raw)
To: git
Here are the topics that have been cooking. Commits prefixed with '-' are
only in 'pu' (proposed updates) while commits prefixed with '+' are in 'next'.
One nasty regression killed, hopefully we are good to go for the third
release candidate tomorrow.
Here are the repositories that have my integration branches:
With maint, master, next, pu, todo:
git://git.kernel.org/pub/scm/git/git.git
git://repo.or.cz/alt-git.git
https://code.google.com/p/git-core/
https://github.com/git/git
With only maint and master:
git://git.sourceforge.jp/gitroot/git-core/git.git
git://git-core.git.sourceforge.net/gitroot/git-core/git-core
With all the topics and integration branches:
https://github.com/gitster/git
The preformatted documentation in HTML and man format are found in:
git://git.kernel.org/pub/scm/git/git-{htmldocs,manpages}.git/
git://repo.or.cz/git-{htmldocs,manpages}.git/
https://code.google.com/p/git-{htmldocs,manpages}.git/
https://github.com/gitster/git-{htmldocs,manpages}.git/
--------------------------------------------------
[New Topics]
* jk/refresh-porcelain-output (2011-11-14) 5 commits
- perhaps squash? show intent-to-add entries as "A" in verbose refresh
- refresh_index: notice typechanges in output
- read-cache: let refresh_cache_ent pass up changed flags
- refresh_index: mark deletions in porcelain output
- refresh_index: rename format variables
The tip two commits need to be squashed into one.
* ab/enable-i18n (2011-11-14) 1 commit
- i18n: add infrastructure for translating Git with gettext
* gh/userdiff-matlab (2011-11-15) 1 commit
- Add built-in diff patterns for MATLAB code
* jc/maint-pack-object-cycle (2011-11-16) 1 commit
- pack-object: tolerate broken packs that have duplicated objects
Make the client side more robust against bogus pack stream; the problem
was discovered by accident while repacking a clone obtained from somewhat
buggy test server.
* jc/index-pack-reject-dups (2011-11-16) 1 commit
- receive-pack, fetch-pack: reject bogus pack that records objects twice
And this is the prevention to reject such pack stream in the first place.
* jc/maint-name-rev-all (2011-11-15) 1 commit
- name-rev --all: do not even attempt to describe non-commit object
Fix for a corner-case bug? that was present from day one of the command.
* ml/mailmap (2011-11-16) 1 commit
- mailmap: xcalloc mailmap_info
An obvious(ly correct) cleanup.
* rr/misc-fixes (2011-11-15) 4 commits
- git-compat-util: don't assume value for undefined variable
- sha1_file: don't mix enum with int
- convert: don't mix enum with int
- http: remove unused function hex()
Obvious(ly correct) cleanups.
--------------------------------------------------
[Stalled]
* hv/submodule-merge-search (2011-10-13) 4 commits
- submodule.c: make two functions static
- allow multiple calls to submodule merge search for the same path
- push: Don't push a repository with unpushed submodules
- push: teach --recurse-submodules the on-demand option
What the topic aims to achieve may make sense, but the implementation
looked somewhat suboptimal.
* sg/complete-refs (2011-10-21) 9 commits
(merged to 'next' on 2011-10-26 at d65e2b4)
+ completion: remove broken dead code from __git_heads() and __git_tags()
+ completion: fast initial completion for config 'remote.*.fetch' value
+ completion: improve ls-remote output filtering in __git_refs_remotes()
+ completion: query only refs/heads/ in __git_refs_remotes()
+ completion: support full refs from remote repositories
+ completion: improve ls-remote output filtering in __git_refs()
+ completion: make refs completion consistent for local and remote repos
+ completion: optimize refs completion
+ completion: document __gitcomp()
Will keep in 'next' during this cycle.
Needs an Ack or two from completion folks before going forward.
* sr/transport-helper-fix-rfc (2011-07-19) 2 commits
- t5800: point out that deleting branches does not work
- t5800: document inability to push new branch with old content
See comments on sr/fix-fast-export-tips topic.
* sr/fix-fast-export-tips (2011-11-05) 3 commits
- fast-export: output reset command for commandline revs
- fast-export: do not refer to non-existing marks
- t9350: point out that refs are not updated correctly
The bottom commit from the stalled sr/transport-helper-fix-rfc topic is
fixed with this. It may make sense to drop the other topic and include
that commit in this series.
The command line parser is still too lax and accepts malformed input, but
this is a step in the right direction and tightening the command line now
should be doable without a low level surgery that touches codepaths that
are unrelated to the command line processing like the previous attempt
used to do.
* jc/lookup-object-hash (2011-08-11) 6 commits
- object hash: replace linear probing with 4-way cuckoo hashing
- object hash: we know the table size is a power of two
- object hash: next_size() helper for readability
- pack-objects --count-only
- object.c: remove duplicated code for object hashing
- object.c: code movement for readability
I do not think there is anything fundamentally wrong with this series, but
the risk of breakage far outweighs observed performance gain in one
particular workload.
* jc/verbose-checkout (2011-10-16) 2 commits
- checkout -v: give full status output after switching branches
- checkout: move the local changes report to the end
This is just to leave a record that the reason why we do not do this not
because we are incapable of coding this, but because it is not a good idea
to do this. I suspect people who are new to git that might think they need
it would soon realize the don't.
Will keep in 'pu' as a showcase for a while and then will drop.
* eh/grep-scale-to-cpunum (2011-11-05) 1 commit
- grep: detect number of CPUs for thread spawning
Kills I/O parallelism and needs to be improved or discarded.
* jc/commit-tree-extra (2011-11-12) 2 commits
- commit-tree: teach -C <extra-commit>
- commit-tree: teach -x <extra>
(this branch uses jc/pull-signed-tag; is tangled with jc/signed-commit.)
Not absolutely needed; parked in 'pu' but may drop.
--------------------------------------------------
[Cooking]
* nd/resolve-ref (2011-11-13) 2 commits
- Copy resolve_ref() return value for longer use
- Convert many resolve_ref() calls to read_ref*() and ref_exists()
A fix for an error-reporting codepath that is not a regression, that
turned into a patch that touches many callsite.
* vr/msvc (2011-10-31) 3 commits
(merged to 'next' on 2011-11-14 at f46d021)
+ MSVC: Remove unneeded header stubs
+ Compile fix for MSVC: Include <io.h>
+ Compile fix for MSVC: Do not include sys/resources.h
Will keep in 'next' during this cycle.
* na/strtoimax (2011-11-05) 3 commits
(merged to 'next' on 2011-11-14 at b64e1cb)
+ Support sizes >=2G in various config options accepting 'g' sizes.
+ Compatibility: declare strtoimax() under NO_STRTOUMAX
+ Add strtoimax() compatibility function.
Will keep in 'next' during this cycle.
* jc/signed-commit (2011-11-12) 4 commits
- pretty: %G[?GS] placeholders
- test "commit -S" and "log --show-signature"
- log: --show-signature
- commit: teach --gpg-sign option
(this branch uses jc/pull-signed-tag; is tangled with jc/commit-tree-extra.)
Rebased on top of jc/pull-signed-tag topic, after reverting the old one
out of 'next'.
* jc/pull-signed-tag (2011-11-12) 15 commits
(merged to 'next' on 2011-11-14 at 25e8838)
+ commit-tree: teach -m/-F options to read logs from elsewhere
+ commit-tree: update the command line parsing
+ commit: teach --amend to carry forward extra headers
+ merge: force edit and no-ff mode when merging a tag object
+ commit: copy merged signed tags to headers of merge commit
+ merge: record tag objects without peeling in MERGE_HEAD
+ merge: make usage of commit->util more extensible
+ fmt-merge-msg: Add contents of merged tag in the merge message
+ fmt-merge-msg: package options into a structure
+ fmt-merge-msg: avoid early returns
+ refs DWIMmery: use the same rule for both "git fetch" and others
+ fetch: allow "git fetch $there v1.0" to fetch a tag
+ merge: notice local merging of tags and keep it unwrapped
+ fetch: do not store peeled tag object names in FETCH_HEAD
+ Split GPG interface into its own helper library
(this branch is used by jc/commit-tree-extra and jc/signed-commit.)
Further updated to allow "commit --amend" to retain the mergetag
headers. I think this is ready for the cycle after upcoming 1.7.8.
Will keep in 'next' during this cycle.
* jc/request-pull-show-head-4 (2011-11-09) 12 commits
(merged to 'next' on 2011-11-13 at e473fd2)
+ request-pull: use the annotated tag contents
(merged to 'next' on 2011-10-15 at 7e340ff)
+ fmt-merge-msg.c: Fix an "dubious one-bit signed bitfield" sparse error
(merged to 'next' on 2011-10-10 at 092175e)
+ environment.c: Fix an sparse "symbol not declared" warning
+ builtin/log.c: Fix an "Using plain integer as NULL pointer" warning
(merged to 'next' on 2011-10-07 at fcaeca0)
+ fmt-merge-msg: use branch.$name.description
(merged to 'next' on 2011-10-06 at fa5e0fe)
+ request-pull: use the branch description
+ request-pull: state what commit to expect
+ request-pull: modernize style
+ branch: teach --edit-description option
+ format-patch: use branch description in cover letter
+ branch: add read_branch_desc() helper function
+ Merge branch 'bk/ancestry-path' into jc/branch-desc
Allow setting "description" for branches and use it to help communications
between humans in various workflow elements. It also allows requesting for
a signed tag to be pulled and shows the tag message in the generated message.
Will keep in 'next' during this cycle.
* ab/clang-lints (2011-11-06) 2 commits
(merged to 'next' on 2011-11-13 at a573aec)
+ cast variable in call to free() in builtin/diff.c and submodule.c
+ apply: get rid of useless x < 0 comparison on a size_t type
Will keep in 'next' during this cycle.
* ab/pull-rebase-config (2011-11-07) 1 commit
(merged to 'next' on 2011-11-13 at 72bb2d5)
+ pull: introduce a pull.rebase option to enable --rebase
Will keep in 'next' during this cycle.
* nd/fsck-progress (2011-11-06) 4 commits
(merged to 'next' on 2011-11-13 at 8831811)
+ fsck: print progress
+ fsck: avoid reading every object twice
+ verify_packfile(): check as many object as possible in a pack
+ fsck: return error code when verify_pack() goes wrong
Will keep in 'next' during this cycle.
* nd/prune-progress (2011-11-07) 3 commits
(merged to 'next' on 2011-11-13 at c5722ac)
+ reachable: per-object progress
+ prune: handle --progress/no-progress
+ prune: show progress while marking reachable objects
Will keep in 'next' during this cycle.
* jc/stream-to-pack (2011-11-03) 4 commits
- Bulk check-in
- finish_tmp_packfile(): a helper function
- create_tmp_packfile(): a helper function
- write_pack_header(): a helper function
Teaches "git add" to send large-ish blob data straight to a packfile.
This is a continuation to the "large file support" topic. I think this
codepath to move data from worktree to repository needs to become aware of
streaming, just like the checkout codepath that goes the other way, which
was done in the previous "large file support" topic in the 1.7.7 cycle.
* jn/gitweb-side-by-side-diff (2011-10-31) 8 commits
- gitweb: Add navigation to select side-by-side diff
- gitweb: Use href(-replay=>1,...) for formats links in "commitdiff"
- t9500: Add basic sanity tests for side-by-side diff in gitweb
- t9500: Add test for handling incomplete lines in diff by gitweb
- gitweb: Give side-by-side diff extra CSS styling
- gitweb: Add a feature to show side-by-side diff
- gitweb: Extract formatting of diff chunk header
- gitweb: Refactor diff body line classification
Replaces a series from Kato Kazuyoshi on the same topic.
* mf/curl-select-fdset (2011-11-04) 4 commits
(merged to 'next' on 2011-11-06 at a49516f)
+ http: drop "local" member from request struct
+ http.c: Rely on select instead of tracking whether data was received
+ http.c: Use timeout suggested by curl instead of fixed 50ms timeout
+ http.c: Use curl_multi_fdset to select on curl fds instead of just sleeping
Reduces unnecessary waits.
Will keep in 'next' during this cycle.
* nd/misc-cleanups (2011-10-27) 6 commits
(merged to 'next' on 2011-10-28 at 2527a49)
+ unpack_object_header_buffer(): clear the size field upon error
+ tree_entry_interesting: make use of local pointer "item"
+ tree_entry_interesting(): give meaningful names to return values
+ read_directory_recursive: reduce one indentation level
+ get_tree_entry(): do not call find_tree_entry() on an empty tree
+ tree-walk.c: do not leak internal structure in tree_entry_len()
These are unquestionably good parts taken out of a larger series, so that
we can focus more on the other changes in later rounds of review.
Will keep in 'next' during this cycle.
* rs/allocate-cache-entry-individually (2011-10-26) 2 commits
(merged to 'next' on 2011-10-27 at 2e4acd6)
+ cache.h: put single NUL at end of struct cache_entry
+ read-cache.c: allocate index entries individually
Will keep in 'next' during this cycle.
* mh/ref-api-3 (2011-11-16) 26 commits
(merged to 'next' on 2011-11-16 at cc76151)
+ refs: loosen over-strict "format" check
(merged to 'next' on 2011-10-23 at 92e2d35)
+ is_refname_available(): reimplement using do_for_each_ref_in_array()
+ names_conflict(): simplify implementation
+ names_conflict(): new function, extracted from is_refname_available()
+ repack_without_ref(): reimplement using do_for_each_ref_in_array()
+ do_for_each_ref_in_array(): new function
+ do_for_each_ref(): correctly terminate while processesing extra_refs
+ add_ref(): take a (struct ref_entry *) parameter
+ create_ref_entry(): extract function from add_ref()
+ parse_ref_line(): add a check that the refname is properly formatted
+ repack_without_ref(): remove temporary
+ Rename another local variable name -> refname
(merged to 'next' on 2011-10-19 at cc89f0e)
+ resolve_gitlink_ref_recursive(): change to work with struct ref_cache
+ Pass a (ref_cache *) to the resolve_gitlink_*() helper functions
+ resolve_gitlink_ref(): improve docstring
+ get_ref_dir(): change signature
+ refs: change signatures of get_packed_refs() and get_loose_refs()
+ is_dup_ref(): extract function from sort_ref_array()
+ add_ref(): add docstring
+ parse_ref_line(): add docstring
+ is_refname_available(): remove the "quiet" argument
+ clear_ref_array(): rename from free_ref_array()
+ refs: rename parameters result -> sha1
+ refs: rename "refname" variables
+ struct ref_entry: document name member
+ cache.h: add comments for git_path() and git_path_submodule()
(this branch is tangled with mh/ref-api-2.)
Will keep in 'next' during this cycle.
* rr/revert-cherry-pick (2011-10-23) 5 commits
(merged to 'next' on 2011-10-26 at 27b7496)
+ revert: simplify communicating command-line arguments
+ revert: allow mixed pick and revert instructions
+ revert: make commit subjects in insn sheet optional
+ revert: simplify getting commit subject in format_todo()
+ revert: free msg in format_todo()
The internals of "git revert/cherry-pick" has been further refactored to
serve as the basis for the sequencer.
Will keep in 'next' during this cycle.
* cb/daemon-permission-errors (2011-10-17) 2 commits
- daemon: report permission denied error to clients
- daemon: add tests
The tip commit might be loosening things a bit too much.
Will keep in 'pu' until hearing a convincing argument for the patch.
* mh/ref-api-2 (2011-11-16) 15 commits
(merged to 'next' on 2011-11-16 at 511457f)
+ refs: loosen over-strict "format" check
(merged to 'next' on 2011-10-19 at cc89f0e)
+ resolve_gitlink_ref_recursive(): change to work with struct ref_cache
+ Pass a (ref_cache *) to the resolve_gitlink_*() helper functions
+ resolve_gitlink_ref(): improve docstring
+ get_ref_dir(): change signature
+ refs: change signatures of get_packed_refs() and get_loose_refs()
+ is_dup_ref(): extract function from sort_ref_array()
+ add_ref(): add docstring
+ parse_ref_line(): add docstring
+ is_refname_available(): remove the "quiet" argument
+ clear_ref_array(): rename from free_ref_array()
+ refs: rename parameters result -> sha1
+ refs: rename "refname" variables
+ struct ref_entry: document name member
+ cache.h: add comments for git_path() and git_path_submodule()
(this branch is tangled with mh/ref-api-3.)
Will keep in 'next' during this cycle.
^ permalink raw reply
* Re: [PATCH 3/3] rename git_path() to git_path_unsafe()
From: Jonathan Nieder @ 2011-11-17 7:03 UTC (permalink / raw)
To: Junio C Hamano
Cc: Ramkumar Ramachandra, Git List,
Nguyễn Thái Ngọc Duy
In-Reply-To: <7vzkfvo88i.fsf@alter.siamese.dyndns.org>
Junio C Hamano wrote:
> In any case, I do not like seeing many list regulars throwing too many
> non-regression-fix patches during prerelease freeze period on the
> list.
Fine, but what about the buffer overflow (not an incredibly recent
regression, but certainly a fix) addressed in patch 2 of the series?
^ permalink raw reply
* BUG. git rebase -i successfully continues (and also skips rewording) when pre-commit hook fails (exits with non-zero code)
From: Alexey Shumkin @ 2011-11-17 8:58 UTC (permalink / raw)
To: git
For a project I have a pre-commit hook that monitors
whether files in a folder (scripts of DB) changed or not
and fails if another special file (DB version) did not changed, too.
So, I did some commits and then I decided to change the order of them.
Of course, I used a lovely "git rebase -i" command. I changed the order
of the commits, then rebasing went ok. But I noticed that my pre-commit
hook output failure message (one of the commits did not meet
above-mentioned condition). It's not too bad but ugly. But when I
decided to correct a message of that specific commit I ran
"git rebase -i" again, marked that commit for rewording, rewording did
not start (because pre-commit hook failed, obviously) and rebasing went
on (commit had an unchanged message) and successfully finished. That is
not what I expected.
I guess if any of hooks fail (which usually fail the commit), rebasing
have to be interrupted (as when there are conflicts)
Here is a sample to reproduce the error
git init .
echo content > file
git add -fv file
git commit -a -m 'first commit'
echo line 2 >> file
git commit -a -m 'secont commit' # note a typo ;)
echo '#!/bin/bash
echo commit failed
exit 1' > .git/hooks/pre-commit
chmod +x .git/hooks/pre-commit
echo fail >> file
git commit -a -m 'failed commit' # to show that pre-commit hook fails
# and outputs "commit failed"
git reset --hard
git rebase -i HEAD^
# mark commit for rewording and exit an editor
note following output after all this:
>commit fail/1)
>Successfully rebased and updated refs/heads/master
^ permalink raw reply
* [PATCH 0/8] nd/resolve-ref v2
From: Nguyễn Thái Ngọc Duy @ 2011-11-17 9:32 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, Michael Haggerty, Jonathan Nieder,
Ramkumar Ramachandra, Nguyễn Thái Ngọc Duy
The first part of actually nd/resolve-ref v2.
The last two patches are an attempt to catch overwriting faults in
future. git_pathname() and resolve_ref_unsafe() are guarded.
(Un)fortunately I ran "make memcheck" but found no new segfaults.
Either test coverage is insufficient, or we have done a very good
job of reviewing/testing git.git
Nguyễn Thái Ngọc Duy (8):
Convert many resolve_ref() calls to read_ref*() and ref_exists()
Rename resolve_ref() to resolve_ref_unsafe()
Re-add resolve_ref() that always returns an allocated buffer
cmd_merge: convert to single exit point
Use resolve_ref() instead of resolve_ref_unsafe()
Convert resolve_ref_unsafe+xstrdup to resolve_ref
Guard memory overwriting in resolve_ref_unsafe's static buffer
Enable GIT_DEBUG_MEMCHECK on git_pathname()
Makefile | 3 ++
branch.c | 2 +-
builtin/branch.c | 11 +++----
builtin/checkout.c | 17 ++++++------
builtin/commit.c | 3 +-
builtin/fmt-merge-msg.c | 8 ++++-
builtin/for-each-ref.c | 7 +---
builtin/fsck.c | 2 +-
builtin/merge.c | 56 +++++++++++++++++++++++++----------------
builtin/notes.c | 8 ++++-
builtin/receive-pack.c | 5 ++-
builtin/remote.c | 10 +++----
builtin/replace.c | 4 +-
builtin/show-branch.c | 6 +---
builtin/show-ref.c | 2 +-
builtin/symbolic-ref.c | 2 +-
builtin/tag.c | 4 +-
bundle.c | 2 +-
cache.h | 17 +++++++++---
git-compat-util.h | 9 ++++++
notes-merge.c | 2 +-
path.c | 28 ++++++++++++++------
reflog-walk.c | 13 +++++----
refs.c | 63 +++++++++++++++++++++++++++++++---------------
remote.c | 10 +++---
transport.c | 2 +-
wrapper.c | 21 +++++++++++++++
wt-status.c | 4 +--
28 files changed, 203 insertions(+), 118 deletions(-)
--
1.7.4.74.g639db
^ permalink raw reply
* [PATCH 1/8] Convert many resolve_ref() calls to read_ref*() and ref_exists()
From: Nguyễn Thái Ngọc Duy @ 2011-11-17 9:32 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, Michael Haggerty, Jonathan Nieder,
Ramkumar Ramachandra, Nguyễn Thái Ngọc Duy
In-Reply-To: <1321522335-24193-1-git-send-email-pclouds@gmail.com>
resolve_ref() may return a pointer to a static buffer, which is not
safe for long-term use because if another resolve_ref() call happens,
the buffer may be changed. Many call sites though do not care about
this buffer. They simply check if the return value is NULL or not.
Convert all these call sites to new wrappers to reduce resolve_ref()
calls from 57 to 34. If we change resolve_ref() prototype later on
to avoid passing static buffer out, this helps reduce changes.
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
builtin/branch.c | 5 ++---
builtin/checkout.c | 4 ++--
builtin/merge.c | 4 ++--
builtin/remote.c | 8 +++-----
builtin/replace.c | 4 ++--
builtin/show-ref.c | 2 +-
builtin/tag.c | 4 ++--
bundle.c | 2 +-
cache.h | 2 ++
notes-merge.c | 2 +-
refs.c | 27 ++++++++++++++++-----------
remote.c | 4 ++--
12 files changed, 36 insertions(+), 32 deletions(-)
diff --git a/builtin/branch.c b/builtin/branch.c
index 51ca6a0..0fe9c4d 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -186,7 +186,7 @@ static int delete_branches(int argc, const char **argv, int force, int kinds)
free(name);
name = xstrdup(mkpath(fmt, bname.buf));
- if (!resolve_ref(name, sha1, 1, NULL)) {
+ if (read_ref(name, sha1)) {
error(_("%sbranch '%s' not found."),
remote, bname.buf);
ret = 1;
@@ -565,7 +565,6 @@ static int print_ref_list(int kinds, int detached, int verbose, int abbrev, stru
static void rename_branch(const char *oldname, const char *newname, int force)
{
struct strbuf oldref = STRBUF_INIT, newref = STRBUF_INIT, logmsg = STRBUF_INIT;
- unsigned char sha1[20];
struct strbuf oldsection = STRBUF_INIT, newsection = STRBUF_INIT;
int recovery = 0;
@@ -577,7 +576,7 @@ static void rename_branch(const char *oldname, const char *newname, int force)
* Bad name --- this could be an attempt to rename a
* ref that we used to allow to be created by accident.
*/
- if (resolve_ref(oldref.buf, sha1, 1, NULL))
+ if (ref_exists(oldref.buf))
recovery = 1;
else
die(_("Invalid branch name: '%s'"), oldname);
diff --git a/builtin/checkout.c b/builtin/checkout.c
index 2a80772..beeaee4 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -288,7 +288,7 @@ static int checkout_paths(struct tree *source_tree, const char **pathspec,
commit_locked_index(lock_file))
die(_("unable to write new index file"));
- resolve_ref("HEAD", rev, 0, &flag);
+ read_ref_full("HEAD", rev, 0, &flag);
head = lookup_commit_reference_gently(rev, 1);
errs |= post_checkout_hook(head, head, 0);
@@ -866,7 +866,7 @@ static int parse_branchname_arg(int argc, const char **argv,
setup_branch_path(new);
if (!check_refname_format(new->path, 0) &&
- resolve_ref(new->path, branch_rev, 1, NULL))
+ !read_ref(new->path, branch_rev))
hashcpy(rev, branch_rev);
else
new->path = NULL; /* not an existing branch */
diff --git a/builtin/merge.c b/builtin/merge.c
index dffd5ec..42b4f9e 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -420,7 +420,7 @@ static struct object *want_commit(const char *name)
static void merge_name(const char *remote, struct strbuf *msg)
{
struct object *remote_head;
- unsigned char branch_head[20], buf_sha[20];
+ unsigned char branch_head[20];
struct strbuf buf = STRBUF_INIT;
struct strbuf bname = STRBUF_INIT;
const char *ptr;
@@ -479,7 +479,7 @@ static void merge_name(const char *remote, struct strbuf *msg)
strbuf_addstr(&truname, "refs/heads/");
strbuf_addstr(&truname, remote);
strbuf_setlen(&truname, truname.len - len);
- if (resolve_ref(truname.buf, buf_sha, 1, NULL)) {
+ if (ref_exists(truname.buf)) {
strbuf_addf(msg,
"%s\t\tbranch '%s'%s of .\n",
sha1_to_hex(remote_head->sha1),
diff --git a/builtin/remote.c b/builtin/remote.c
index c810643..407abfb 100644
--- a/builtin/remote.c
+++ b/builtin/remote.c
@@ -343,8 +343,7 @@ static int get_ref_states(const struct ref *remote_refs, struct ref_states *stat
states->tracked.strdup_strings = 1;
states->stale.strdup_strings = 1;
for (ref = fetch_map; ref; ref = ref->next) {
- unsigned char sha1[20];
- if (!ref->peer_ref || read_ref(ref->peer_ref->name, sha1))
+ if (!ref->peer_ref || !ref_exists(ref->peer_ref->name))
string_list_append(&states->new, abbrev_branch(ref->name));
else
string_list_append(&states->tracked, abbrev_branch(ref->name));
@@ -710,7 +709,7 @@ static int mv(int argc, const char **argv)
int flag = 0;
unsigned char sha1[20];
- resolve_ref(item->string, sha1, 1, &flag);
+ read_ref_full(item->string, sha1, 1, &flag);
if (!(flag & REF_ISSYMREF))
continue;
if (delete_ref(item->string, NULL, REF_NODEREF))
@@ -1220,10 +1219,9 @@ static int set_head(int argc, const char **argv)
usage_with_options(builtin_remote_sethead_usage, options);
if (head_name) {
- unsigned char sha1[20];
strbuf_addf(&buf2, "refs/remotes/%s/%s", argv[0], head_name);
/* make sure it's valid */
- if (!resolve_ref(buf2.buf, sha1, 1, NULL))
+ if (!ref_exists(buf2.buf))
result |= error("Not a valid ref: %s", buf2.buf);
else if (create_symref(buf.buf, buf2.buf, "remote set-head"))
result |= error("Could not setup %s", buf.buf);
diff --git a/builtin/replace.c b/builtin/replace.c
index 517fa10..4a8970e 100644
--- a/builtin/replace.c
+++ b/builtin/replace.c
@@ -58,7 +58,7 @@ static int for_each_replace_name(const char **argv, each_replace_name_fn fn)
had_error = 1;
continue;
}
- if (!resolve_ref(ref, sha1, 1, NULL)) {
+ if (read_ref(ref, sha1)) {
error("replace ref '%s' not found.", *p);
had_error = 1;
continue;
@@ -97,7 +97,7 @@ static int replace_object(const char *object_ref, const char *replace_ref,
if (check_refname_format(ref, 0))
die("'%s' is not a valid ref name.", ref);
- if (!resolve_ref(ref, prev, 1, NULL))
+ if (read_ref(ref, prev))
hashclr(prev);
else if (!force)
die("replace ref '%s' already exists", ref);
diff --git a/builtin/show-ref.c b/builtin/show-ref.c
index fafb6dd..3911661 100644
--- a/builtin/show-ref.c
+++ b/builtin/show-ref.c
@@ -225,7 +225,7 @@ int cmd_show_ref(int argc, const char **argv, const char *prefix)
unsigned char sha1[20];
if (!prefixcmp(*pattern, "refs/") &&
- resolve_ref(*pattern, sha1, 1, NULL)) {
+ !read_ref(*pattern, sha1)) {
if (!quiet)
show_one(*pattern, sha1);
}
diff --git a/builtin/tag.c b/builtin/tag.c
index 9b6fd95..439249d 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -174,7 +174,7 @@ static int for_each_tag_name(const char **argv, each_tag_name_fn fn)
had_error = 1;
continue;
}
- if (!resolve_ref(ref, sha1, 1, NULL)) {
+ if (read_ref(ref, sha1)) {
error(_("tag '%s' not found."), *p);
had_error = 1;
continue;
@@ -518,7 +518,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
if (strbuf_check_tag_ref(&ref, tag))
die(_("'%s' is not a valid tag name."), tag);
- if (!resolve_ref(ref.buf, prev, 1, NULL))
+ if (read_ref(ref.buf, prev))
hashclr(prev);
else if (!force)
die(_("tag '%s' already exists"), tag);
diff --git a/bundle.c b/bundle.c
index 08020bc..4742f27 100644
--- a/bundle.c
+++ b/bundle.c
@@ -320,7 +320,7 @@ int create_bundle(struct bundle_header *header, const char *path,
continue;
if (dwim_ref(e->name, strlen(e->name), sha1, &ref) != 1)
continue;
- if (!resolve_ref(e->name, sha1, 1, &flag))
+ if (read_ref_full(e->name, sha1, 1, &flag))
flag = 0;
display_ref = (flag & REF_ISSYMREF) ? e->name : ref;
diff --git a/cache.h b/cache.h
index 2e6ad36..5badece 100644
--- a/cache.h
+++ b/cache.h
@@ -832,6 +832,8 @@ static inline int get_sha1_with_context(const char *str, unsigned char *sha1, st
extern int get_sha1_hex(const char *hex, unsigned char *sha1);
extern char *sha1_to_hex(const unsigned char *sha1); /* static buffer result! */
+extern int read_ref_full(const char *filename, unsigned char *sha1,
+ int reading, int *flags);
extern int read_ref(const char *filename, unsigned char *sha1);
/*
diff --git a/notes-merge.c b/notes-merge.c
index e9e4199..e33c2c9 100644
--- a/notes-merge.c
+++ b/notes-merge.c
@@ -568,7 +568,7 @@ int notes_merge(struct notes_merge_options *o,
o->local_ref, o->remote_ref);
/* Dereference o->local_ref into local_sha1 */
- if (!resolve_ref(o->local_ref, local_sha1, 0, NULL))
+ if (read_ref_full(o->local_ref, local_sha1, 0, NULL))
die("Failed to resolve local notes ref '%s'", o->local_ref);
else if (!check_refname_format(o->local_ref, 0) &&
is_null_sha1(local_sha1))
diff --git a/refs.c b/refs.c
index e69ba26..44c1c86 100644
--- a/refs.c
+++ b/refs.c
@@ -334,7 +334,7 @@ static void get_ref_dir(const char *submodule, const char *base,
flag |= REF_ISBROKEN;
}
} else
- if (!resolve_ref(ref, sha1, 1, &flag)) {
+ if (read_ref_full(ref, sha1, 1, &flag)) {
hashclr(sha1);
flag |= REF_ISBROKEN;
}
@@ -612,13 +612,18 @@ struct ref_filter {
void *cb_data;
};
-int read_ref(const char *ref, unsigned char *sha1)
+int read_ref_full(const char *ref, unsigned char *sha1, int reading, int *flags)
{
- if (resolve_ref(ref, sha1, 1, NULL))
+ if (resolve_ref(ref, sha1, reading, flags))
return 0;
return -1;
}
+int read_ref(const char *ref, unsigned char *sha1)
+{
+ return read_ref_full(ref, sha1, 1, NULL);
+}
+
#define DO_FOR_EACH_INCLUDE_BROKEN 01
static int do_one_ref(const char *base, each_ref_fn fn, int trim,
int flags, void *cb_data, struct ref_entry *entry)
@@ -663,7 +668,7 @@ int peel_ref(const char *ref, unsigned char *sha1)
goto fallback;
}
- if (!resolve_ref(ref, base, 1, &flag))
+ if (read_ref_full(ref, base, 1, &flag))
return -1;
if ((flag & REF_ISPACKED)) {
@@ -746,7 +751,7 @@ static int do_head_ref(const char *submodule, each_ref_fn fn, void *cb_data)
return 0;
}
- if (resolve_ref("HEAD", sha1, 1, &flag))
+ if (!read_ref_full("HEAD", sha1, 1, &flag))
return fn("HEAD", sha1, flag, cb_data);
return 0;
@@ -826,7 +831,7 @@ int head_ref_namespaced(each_ref_fn fn, void *cb_data)
int flag;
strbuf_addf(&buf, "%sHEAD", get_git_namespace());
- if (resolve_ref(buf.buf, sha1, 1, &flag))
+ if (!read_ref_full(buf.buf, sha1, 1, &flag))
ret = fn(buf.buf, sha1, flag, cb_data);
strbuf_release(&buf);
@@ -1022,7 +1027,7 @@ int refname_match(const char *abbrev_name, const char *full_name, const char **r
static struct ref_lock *verify_lock(struct ref_lock *lock,
const unsigned char *old_sha1, int mustexist)
{
- if (!resolve_ref(lock->ref_name, lock->old_sha1, mustexist, NULL)) {
+ if (read_ref_full(lock->ref_name, lock->old_sha1, mustexist, NULL)) {
error("Can't verify ref %s", lock->ref_name);
unlock_ref(lock);
return NULL;
@@ -1377,7 +1382,8 @@ int rename_ref(const char *oldref, const char *newref, const char *logmsg)
goto rollback;
}
- if (resolve_ref(newref, sha1, 1, &flag) && delete_ref(newref, sha1, REF_NODEREF)) {
+ if (!read_ref_full(newref, sha1, 1, &flag) &&
+ delete_ref(newref, sha1, REF_NODEREF)) {
if (errno==EISDIR) {
if (remove_empty_directories(git_path("%s", newref))) {
error("Directory not empty: %s", newref);
@@ -1929,7 +1935,7 @@ static int do_for_each_reflog(const char *base, each_ref_fn fn, void *cb_data)
retval = do_for_each_reflog(log, fn, cb_data);
} else {
unsigned char sha1[20];
- if (!resolve_ref(log, sha1, 0, NULL))
+ if (read_ref_full(log, sha1, 0, NULL))
retval = error("bad ref for %s", log);
else
retval = fn(log, sha1, 0, cb_data);
@@ -2072,7 +2078,6 @@ char *shorten_unambiguous_ref(const char *ref, int strict)
*/
for (j = 0; j < rules_to_fail; j++) {
const char *rule = ref_rev_parse_rules[j];
- unsigned char short_objectname[20];
char refname[PATH_MAX];
/* skip matched rule */
@@ -2086,7 +2091,7 @@ char *shorten_unambiguous_ref(const char *ref, int strict)
*/
mksnpath(refname, sizeof(refname),
rule, short_name_len, short_name);
- if (!read_ref(refname, short_objectname))
+ if (ref_exists(refname))
break;
}
diff --git a/remote.c b/remote.c
index e2ef991..6655bb0 100644
--- a/remote.c
+++ b/remote.c
@@ -1507,13 +1507,13 @@ int stat_tracking_info(struct branch *branch, int *num_ours, int *num_theirs)
* nothing to report.
*/
base = branch->merge[0]->dst;
- if (!resolve_ref(base, sha1, 1, NULL))
+ if (read_ref(base, sha1))
return 0;
theirs = lookup_commit_reference(sha1);
if (!theirs)
return 0;
- if (!resolve_ref(branch->refname, sha1, 1, NULL))
+ if (read_ref(branch->refname, sha1))
return 0;
ours = lookup_commit_reference(sha1);
if (!ours)
--
1.7.4.74.g639db
^ permalink raw reply related
* [PATCH 2/8] Rename resolve_ref() to resolve_ref_unsafe()
From: Nguyễn Thái Ngọc Duy @ 2011-11-17 9:32 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, Michael Haggerty, Jonathan Nieder,
Ramkumar Ramachandra, Nguyễn Thái Ngọc Duy
In-Reply-To: <1321522335-24193-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 this 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 | 6 +++---
builtin/checkout.c | 2 +-
builtin/commit.c | 2 +-
builtin/fmt-merge-msg.c | 2 +-
builtin/for-each-ref.c | 2 +-
builtin/fsck.c | 2 +-
builtin/merge.c | 2 +-
builtin/notes.c | 2 +-
builtin/receive-pack.c | 4 ++--
builtin/remote.c | 2 +-
builtin/show-branch.c | 4 ++--
builtin/symbolic-ref.c | 2 +-
cache.h | 2 +-
reflog-walk.c | 4 ++--
refs.c | 20 ++++++++++----------
remote.c | 6 +++---
transport.c | 2 +-
wt-status.c | 2 +-
19 files changed, 35 insertions(+), 35 deletions(-)
diff --git a/branch.c b/branch.c
index d809876..243355b 100644
--- a/branch.c
+++ b/branch.c
@@ -151,7 +151,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 0fe9c4d..6903b0d 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -115,7 +115,7 @@ 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)
+ resolve_ref_unsafe(branch->merge[0]->dst, sha1, 1, NULL)) != NULL)
reference_rev = lookup_commit_reference(sha1);
}
if (!reference_rev)
@@ -250,7 +250,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)))
@@ -688,7 +688,7 @@ 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_ref_unsafe("HEAD", head_sha1, 0, NULL);
if (!head)
die(_("Failed to resolve HEAD as a valid ref."));
head = xstrdup(head);
diff --git a/builtin/checkout.c b/builtin/checkout.c
index beeaee4..2b8e73b 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -699,7 +699,7 @@ static int switch_branches(struct checkout_opts *opts, struct branch_info *new)
unsigned char rev[20];
int flag;
memset(&old, 0, sizeof(old));
- old.path = xstrdup(resolve_ref("HEAD", rev, 0, &flag));
+ old.path = xstrdup(resolve_ref_unsafe("HEAD", rev, 0, &flag));
old.commit = lookup_commit_reference_gently(rev, 1);
if (!(flag & REF_ISSYMREF)) {
free((char *)old.path);
diff --git a/builtin/commit.c b/builtin/commit.c
index c46f2d1..0be3b45 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -1259,7 +1259,7 @@ static void print_summary(const char *prefix, const unsigned char *sha1,
struct commit *commit;
struct strbuf format = STRBUF_INIT;
unsigned char junk_sha1[20];
- const char *head = resolve_ref("HEAD", junk_sha1, 0, NULL);
+ const char *head = resolve_ref_unsafe("HEAD", junk_sha1, 0, NULL);
struct pretty_print_context pctx = {0};
struct strbuf author_ident = STRBUF_INIT;
struct strbuf committer_ident = STRBUF_INIT;
diff --git a/builtin/fmt-merge-msg.c b/builtin/fmt-merge-msg.c
index 7e2f225..5c9b40e 100644
--- a/builtin/fmt-merge-msg.c
+++ b/builtin/fmt-merge-msg.c
@@ -263,7 +263,7 @@ static int do_fmt_merge_msg(int merge_title, struct strbuf *in,
const char *current_branch;
/* get current branch */
- current_branch = resolve_ref("HEAD", head_sha1, 1, NULL);
+ current_branch = resolve_ref_unsafe("HEAD", head_sha1, 1, NULL);
if (!current_branch)
die("No current branch");
if (!prefixcmp(current_branch, "refs/heads/"))
diff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c
index d90e5d2..b954ca8 100644
--- a/builtin/for-each-ref.c
+++ b/builtin/for-each-ref.c
@@ -629,7 +629,7 @@ 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);
+ symref = resolve_ref_unsafe(ref->refname, unused1, 1, NULL);
if (symref)
ref->symref = xstrdup(symref);
else
diff --git a/builtin/fsck.c b/builtin/fsck.c
index df1a88b..0e0e17a 100644
--- a/builtin/fsck.c
+++ b/builtin/fsck.c
@@ -532,7 +532,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/merge.c b/builtin/merge.c
index 42b4f9e..519e3c5 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -1095,7 +1095,7 @@ 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);
+ branch = resolve_ref_unsafe("HEAD", head_sha1, 0, &flag);
if (branch && !prefixcmp(branch, "refs/heads/"))
branch += 11;
if (!branch || is_null_sha1(head_sha1))
diff --git a/builtin/notes.c b/builtin/notes.c
index f8e437d..e191ce6 100644
--- a/builtin/notes.c
+++ b/builtin/notes.c
@@ -825,7 +825,7 @@ 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 = resolve_ref_unsafe("NOTES_MERGE_REF", sha1, 0, NULL);
if (!o->local_ref)
die("Failed to resolve NOTES_MERGE_REF");
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 7ec68a1..333f2b0 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))
@@ -695,7 +695,7 @@ static void execute_commands(struct command *commands, const char *unpacker_erro
check_aliased_updates(commands);
- head_name = resolve_ref("HEAD", sha1, 0, NULL);
+ head_name = resolve_ref_unsafe("HEAD", sha1, 0, NULL);
for (cmd = commands; cmd; cmd = cmd->next)
if (!cmd->skip_update)
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 4b480d7..1e7bd31 100644
--- a/builtin/show-branch.c
+++ b/builtin/show-branch.c
@@ -728,7 +728,7 @@ int cmd_show_branch(int ac, const char **av, const char *prefix)
static const char *fake_av[2];
const char *refname;
- refname = resolve_ref("HEAD", sha1, 1, NULL);
+ refname = resolve_ref_unsafe("HEAD", sha1, 1, NULL);
fake_av[0] = xstrdup(refname);
fake_av[1] = NULL;
av = fake_av;
@@ -791,7 +791,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 5badece..61f023a 100644
--- a/cache.h
+++ b/cache.h
@@ -866,7 +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 const char *resolve_ref_unsafe(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 5d81d39..b9b2453 100644
--- a/reflog-walk.c
+++ b/reflog-walk.c
@@ -50,7 +50,7 @@ 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);
+ const char *name = resolve_ref_unsafe(ref, sha1, 1, NULL);
if (name)
for_each_reflog_ent(name, read_one_reflog, reflogs);
}
@@ -168,7 +168,7 @@ 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);
+ const char *head = resolve_ref_unsafe("HEAD", sha1, 0, NULL);
if (!head)
die ("No current branch");
free(branch);
diff --git a/refs.c b/refs.c
index 44c1c86..9e42e36 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;
@@ -497,7 +497,7 @@ 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_unsafe(const char *ref, unsigned char *sha1, int reading, int *flag)
{
int depth = MAXDEPTH;
ssize_t len;
@@ -614,7 +614,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;
}
@@ -1118,7 +1118,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);
@@ -1148,7 +1148,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) &&
@@ -1184,7 +1184,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;
@@ -1197,7 +1197,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;
@@ -1360,7 +1360,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);
@@ -1649,7 +1649,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);
@@ -1986,7 +1986,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/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;
diff --git a/wt-status.c b/wt-status.c
index 70fdb76..cc6dad5 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -119,7 +119,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);
+ head = resolve_ref_unsafe("HEAD", sha1, 0, NULL);
s->branch = head ? xstrdup(head) : NULL;
s->reference = "HEAD";
s->fp = stdout;
--
1.7.4.74.g639db
^ permalink raw reply related
* [PATCH 3/8] Re-add resolve_ref() that always returns an allocated buffer
From: Nguyễn Thái Ngọc Duy @ 2011-11-17 9:32 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, Michael Haggerty, Jonathan Nieder,
Ramkumar Ramachandra, Nguyễn Thái Ngọc Duy
In-Reply-To: <1321522335-24193-1-git-send-email-pclouds@gmail.com>
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
cache.h | 1 +
refs.c | 6 ++++++
2 files changed, 7 insertions(+), 0 deletions(-)
diff --git a/cache.h b/cache.h
index 61f023a..6b8ac8b 100644
--- a/cache.h
+++ b/cache.h
@@ -867,6 +867,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_unsafe(const char *ref, unsigned char *sha1, int reading, int *flag);
+extern char *resolve_ref(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/refs.c b/refs.c
index 9e42e36..28496ed 100644
--- a/refs.c
+++ b/refs.c
@@ -605,6 +605,12 @@ const char *resolve_ref_unsafe(const char *ref, unsigned char *sha1, int reading
return ref;
}
+char *resolve_ref(const char *ref, unsigned char *sha1, int reading, int *flag)
+{
+ const char *ret = resolve_ref_unsafe(ref, sha1, reading, flag);
+ return ret ? xstrdup(ret) : NULL;
+}
+
/* The argument to filter_refs */
struct ref_filter {
const char *pattern;
--
1.7.4.74.g639db
^ permalink raw reply related
* [PATCH 4/8] cmd_merge: convert to single exit point
From: Nguyễn Thái Ngọc Duy @ 2011-11-17 9:32 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, Michael Haggerty, Jonathan Nieder,
Ramkumar Ramachandra, Nguyễn Thái Ngọc Duy
In-Reply-To: <1321522335-24193-1-git-send-email-pclouds@gmail.com>
This makes post-processing easier.
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
builtin/merge.c | 48 +++++++++++++++++++++++++++++-------------------
1 files changed, 29 insertions(+), 19 deletions(-)
diff --git a/builtin/merge.c b/builtin/merge.c
index 519e3c5..0d597b3 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -1082,7 +1082,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
struct commit *head_commit;
struct strbuf buf = STRBUF_INIT;
const char *head_arg;
- int flag, i;
+ int flag, i, ret = 0;
int best_cnt = -1, merge_was_ok = 0, automerge_was_ok = 0;
struct commit_list *common = NULL;
const char *best_strategy = NULL, *wt_strategy = NULL;
@@ -1121,7 +1121,8 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
die(_("There is no merge to abort (MERGE_HEAD missing)."));
/* Invoke 'git reset --merge' */
- return cmd_reset(nargc, nargv, prefix);
+ ret = cmd_reset(nargc, nargv, prefix);
+ goto done;
}
if (read_cache_unmerged())
@@ -1205,7 +1206,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
read_empty(remote_head->sha1, 0);
update_ref("initial pull", "HEAD", remote_head->sha1, NULL, 0,
DIE_ON_ERR);
- return 0;
+ goto done;
} else {
struct strbuf merge_names = STRBUF_INIT;
@@ -1292,7 +1293,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
* but first the most common case of merging one remote.
*/
finish_up_to_date("Already up-to-date.");
- return 0;
+ goto done;
} else if (allow_fast_forward && !remoteheads->next &&
!common->next &&
!hashcmp(common->item->object.sha1, head_commit->object.sha1)) {
@@ -1313,15 +1314,16 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
strbuf_addstr(&msg,
" (no commit created; -m option ignored)");
o = want_commit(sha1_to_hex(remoteheads->item->object.sha1));
- if (!o)
- return 1;
-
- if (checkout_fast_forward(head_commit->object.sha1, remoteheads->item->object.sha1))
- return 1;
+ if (!o ||
+ checkout_fast_forward(head_commit->object.sha1,
+ remoteheads->item->object.sha1)) {
+ ret = 1;
+ goto done;
+ }
finish(head_commit, o->sha1, msg.buf);
drop_save();
- return 0;
+ goto done;
} else if (!remoteheads->next && common->next)
;
/*
@@ -1339,8 +1341,11 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
git_committer_info(IDENT_ERROR_ON_NO_NAME);
printf(_("Trying really trivial in-index merge...\n"));
if (!read_tree_trivial(common->item->object.sha1,
- head_commit->object.sha1, remoteheads->item->object.sha1))
- return merge_trivial(head_commit);
+ head_commit->object.sha1,
+ remoteheads->item->object.sha1)) {
+ ret = merge_trivial(head_commit);
+ goto done;
+ }
printf(_("Nope.\n"));
}
} else {
@@ -1368,7 +1373,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
}
if (up_to_date) {
finish_up_to_date("Already up-to-date. Yeeah!");
- return 0;
+ goto done;
}
}
@@ -1450,9 +1455,11 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
* If we have a resulting tree, that means the strategy module
* auto resolved the merge cleanly.
*/
- if (automerge_was_ok)
- return finish_automerge(head_commit, common, result_tree,
- wt_strategy);
+ if (automerge_was_ok) {
+ ret = finish_automerge(head_commit, common, result_tree,
+ wt_strategy);
+ goto done;
+ }
/*
* Pick the result from the best strategy and have the user fix
@@ -1466,7 +1473,8 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
else
fprintf(stderr, _("Merge with strategy %s failed.\n"),
use_strategies[0]->name);
- return 2;
+ ret = 2;
+ goto done;
} else if (best_strategy == wt_strategy)
; /* We already have its result in the working tree. */
else {
@@ -1485,7 +1493,9 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
if (merge_was_ok) {
fprintf(stderr, _("Automatic merge went well; "
"stopped before committing as requested\n"));
- return 0;
} else
- return suggest_conflicts(option_renormalize);
+ ret = suggest_conflicts(option_renormalize);
+
+done:
+ return ret;
}
--
1.7.4.74.g639db
^ permalink raw reply related
* [PATCH 5/8] Use resolve_ref() instead of resolve_ref_unsafe()
From: Nguyễn Thái Ngọc Duy @ 2011-11-17 9:32 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, Michael Haggerty, Jonathan Nieder,
Ramkumar Ramachandra, Nguyễn Thái Ngọc Duy
In-Reply-To: <1321522335-24193-1-git-send-email-pclouds@gmail.com>
resolve_ref_unsaf() may return a pointer to a static buffer. Callers
that use this value longer than a couple of statements should copy the
value to avoid some hidden resolve_ref() call that may change the
static buffer's value.
The bug found by Tony Wang <wwwjfy@gmail.com> in builtin/merge.c
demonstrates this. The first call is in cmd_merge()
branch = resolve_ref_unsafe("HEAD", head_sha1, 0, &flag);
Then deep in lookup_commit_or_die() a few lines after, resolve_ref()
may be called again and destroy "branch".
lookup_commit_or_die
lookup_commit_reference
lookup_commit_reference_gently
parse_object
lookup_replace_object
do_lookup_replace_object
prepare_replace_object
for_each_replace_ref
do_for_each_ref
get_loose_refs
get_ref_dir
read_ref_full
resolve_ref_unsafe
Convert all resolve_ref_unsafe() call sites, where the return value may
be held across many function calls, to resolve_ref() and free buffer
after use.
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
builtin/branch.c | 5 +++--
builtin/commit.c | 3 ++-
builtin/fmt-merge-msg.c | 8 ++++++--
builtin/merge.c | 4 +++-
builtin/notes.c | 8 ++++++--
builtin/receive-pack.c | 5 +++--
reflog-walk.c | 6 ++++--
7 files changed, 27 insertions(+), 12 deletions(-)
diff --git a/builtin/branch.c b/builtin/branch.c
index 6903b0d..633b56e 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,7 +115,7 @@ static int branch_merged(int kind, const char *name,
branch->merge[0] &&
branch->merge[0]->dst &&
(reference_name =
- resolve_ref_unsafe(branch->merge[0]->dst, sha1, 1, NULL)) != NULL)
+ resolve_ref(branch->merge[0]->dst, sha1, 1, NULL)) != NULL)
reference_rev = lookup_commit_reference(sha1);
}
if (!reference_rev)
@@ -141,6 +141,7 @@ static int branch_merged(int kind, const char *name,
" '%s', even though it is merged to HEAD."),
name, reference_name);
}
+ free(reference_name);
return merged;
}
diff --git a/builtin/commit.c b/builtin/commit.c
index 0be3b45..f3a6ed2 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -1259,7 +1259,7 @@ static void print_summary(const char *prefix, const unsigned char *sha1,
struct commit *commit;
struct strbuf format = STRBUF_INIT;
unsigned char junk_sha1[20];
- const char *head = resolve_ref_unsafe("HEAD", junk_sha1, 0, NULL);
+ const char *head;
struct pretty_print_context pctx = {0};
struct strbuf author_ident = STRBUF_INIT;
struct strbuf committer_ident = STRBUF_INIT;
@@ -1304,6 +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);
printf("[%s%s ",
!prefixcmp(head, "refs/heads/") ?
head + 11 :
diff --git a/builtin/fmt-merge-msg.c b/builtin/fmt-merge-msg.c
index 5c9b40e..dd94c3d 100644
--- a/builtin/fmt-merge-msg.c
+++ b/builtin/fmt-merge-msg.c
@@ -261,9 +261,10 @@ static int do_fmt_merge_msg(int merge_title, struct strbuf *in,
int i = 0, pos = 0;
unsigned char head_sha1[20];
const char *current_branch;
+ char *ref;
/* get current branch */
- current_branch = resolve_ref_unsafe("HEAD", head_sha1, 1, NULL);
+ current_branch = ref = resolve_ref("HEAD", head_sha1, 1, NULL);
if (!current_branch)
die("No current branch");
if (!prefixcmp(current_branch, "refs/heads/"))
@@ -283,8 +284,10 @@ static int do_fmt_merge_msg(int merge_title, struct strbuf *in,
die ("Error in line %d: %.*s", i, len, p);
}
- if (!srcs.nr)
+ if (!srcs.nr) {
+ free(ref);
return 0;
+ }
if (merge_title)
do_fmt_merge_msg_title(out, current_branch);
@@ -306,6 +309,7 @@ static int do_fmt_merge_msg(int merge_title, struct strbuf *in,
shortlog(origins.items[i].string, origins.items[i].util,
head, &rev, shortlog_len, out);
}
+ free(ref);
return 0;
}
diff --git a/builtin/merge.c b/builtin/merge.c
index 0d597b3..8be0594 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -1087,6 +1087,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);
@@ -1095,7 +1096,7 @@ 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_unsafe("HEAD", head_sha1, 0, &flag);
+ branch = branch_ref = resolve_ref("HEAD", head_sha1, 0, &flag);
if (branch && !prefixcmp(branch, "refs/heads/"))
branch += 11;
if (!branch || is_null_sha1(head_sha1))
@@ -1497,5 +1498,6 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
ret = suggest_conflicts(option_renormalize);
done:
+ free(branch_ref);
return ret;
}
diff --git a/builtin/notes.c b/builtin/notes.c
index e191ce6..725a701 100644
--- a/builtin/notes.c
+++ b/builtin/notes.c
@@ -804,6 +804,8 @@ 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;
/*
* Read partial merge result from .git/NOTES_MERGE_PARTIAL,
@@ -825,7 +827,7 @@ 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_unsafe("NOTES_MERGE_REF", sha1, 0, NULL);
+ o->local_ref = ref = resolve_ref("NOTES_MERGE_REF", sha1, 0, NULL);
if (!o->local_ref)
die("Failed to resolve NOTES_MERGE_REF");
@@ -843,7 +845,9 @@ static int merge_commit(struct notes_merge_options *o)
free_notes(t);
strbuf_release(&msg);
- return merge_abort(o);
+ ret = merge_abort(o);
+ free(ref);
+ return ret;
}
static int merge(int argc, const char **argv, const char *prefix)
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 333f2b0..d41884a 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,7 +695,8 @@ static void execute_commands(struct command *commands, const char *unpacker_erro
check_aliased_updates(commands);
- head_name = resolve_ref_unsafe("HEAD", sha1, 0, NULL);
+ free(head_name);
+ head_name = resolve_ref("HEAD", sha1, 0, NULL);
for (cmd = commands; cmd; cmd = cmd->next)
if (!cmd->skip_update)
diff --git a/reflog-walk.c b/reflog-walk.c
index b9b2453..2d5aee0 100644
--- a/reflog-walk.c
+++ b/reflog-walk.c
@@ -50,9 +50,11 @@ 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_unsafe(ref, sha1, 1, NULL);
- if (name)
+ char *name = resolve_ref(ref, sha1, 1, NULL);
+ if (name) {
for_each_reflog_ent(name, read_one_reflog, reflogs);
+ free(name);
+ }
}
if (reflogs->nr == 0) {
int len = strlen(ref);
--
1.7.4.74.g639db
^ permalink raw reply related
* [PATCH 6/8] Convert resolve_ref_unsafe+xstrdup to resolve_ref
From: Nguyễn Thái Ngọc Duy @ 2011-11-17 9:32 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, Michael Haggerty, Jonathan Nieder,
Ramkumar Ramachandra, Nguyễn Thái Ngọc Duy
In-Reply-To: <1321522335-24193-1-git-send-email-pclouds@gmail.com>
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
builtin/branch.c | 3 +--
builtin/checkout.c | 13 +++++++------
builtin/for-each-ref.c | 7 ++-----
builtin/show-branch.c | 4 +---
reflog-walk.c | 7 +++----
wt-status.c | 4 +---
6 files changed, 15 insertions(+), 23 deletions(-)
diff --git a/builtin/branch.c b/builtin/branch.c
index 633b56e..a254898 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -689,10 +689,9 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
track = git_branch_track;
- head = resolve_ref_unsafe("HEAD", head_sha1, 0, NULL);
+ head = resolve_ref("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 2b8e73b..6efb1cf 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -696,15 +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 = xstrdup(resolve_ref_unsafe("HEAD", rev, 0, &flag));
+ old.path = path = resolve_ref("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/");
@@ -718,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);
@@ -727,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/for-each-ref.c b/builtin/for-each-ref.c
index b954ca8..dc19f3c 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_unsafe(ref->refname, unused1, 1, NULL);
- if (symref)
- ref->symref = xstrdup(symref);
- else
+ ref->symref = resolve_ref(ref->refname, unused1, 1, NULL);
+ if (!ref->symref)
ref->symref = "";
}
diff --git a/builtin/show-branch.c b/builtin/show-branch.c
index 1e7bd31..9e849c7 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_unsafe("HEAD", sha1, 1, NULL);
- fake_av[0] = xstrdup(refname);
+ fake_av[0] = resolve_ref("HEAD", sha1, 1, NULL);
fake_av[1] = NULL;
av = fake_av;
ac = 1;
diff --git a/reflog-walk.c b/reflog-walk.c
index 2d5aee0..fd17e71 100644
--- a/reflog-walk.c
+++ b/reflog-walk.c
@@ -170,11 +170,10 @@ int add_reflog_for_walk(struct reflog_walk_info *info,
else {
if (*branch == '\0') {
unsigned char sha1[20];
- const char *head = resolve_ref_unsafe("HEAD", sha1, 0, NULL);
- if (!head)
- die ("No current branch");
free(branch);
- branch = xstrdup(head);
+ branch = resolve_ref("HEAD", sha1, 0, NULL);
+ if (!branch)
+ die ("No current branch");
}
reflogs = read_complete_reflog(branch);
if (!reflogs || reflogs->nr == 0) {
diff --git a/wt-status.c b/wt-status.c
index cc6dad5..77adf64 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_unsafe("HEAD", sha1, 0, NULL);
- s->branch = head ? xstrdup(head) : NULL;
+ s->branch = resolve_ref("HEAD", sha1, 0, NULL);
s->reference = "HEAD";
s->fp = stdout;
s->index_file = get_index_file();
--
1.7.4.74.g639db
^ permalink raw reply related
* [PATCH 7/8] Guard memory overwriting in resolve_ref_unsafe's static buffer
From: Nguyễn Thái Ngọc Duy @ 2011-11-17 9:32 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, Michael Haggerty, Jonathan Nieder,
Ramkumar Ramachandra, Nguyễn Thái Ngọc Duy
In-Reply-To: <1321522335-24193-1-git-send-email-pclouds@gmail.com>
There is a potential problem with resolve_ref_unsafe() and some other
functions in git. The return value returned by resolve_ref_unsafe() 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_unsafe() 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_unsafe() 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>
---
GIT_DEBUG_MEMCHECK should probably be ignored in Windows.
Makefile | 3 +++
cache.h | 3 ++-
git-compat-util.h | 9 +++++++++
refs.c | 14 ++++++++++++--
wrapper.c | 21 +++++++++++++++++++++
5 files changed, 47 insertions(+), 3 deletions(-)
diff --git a/Makefile b/Makefile
index ee34eab..d671c64 100644
--- a/Makefile
+++ b/Makefile
@@ -2220,6 +2220,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 6b8ac8b..feb44a5 100644
--- a/cache.h
+++ b/cache.h
@@ -866,7 +866,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_unsafe(const char *ref, unsigned char *sha1, int reading, int *flag);
+#define resolve_ref_unsafe(ref, sha1, reading, flag) resolve_ref_unsafe_real(ref, sha1, reading, flag, __FUNCTION__, __LINE__)
+extern const char *resolve_ref_unsafe_real(const char *ref, unsigned char *sha1, int reading, int *flag, const char *file, int line);
extern char *resolve_ref(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 5ef8ff7..d00c9c6 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -432,6 +432,15 @@ 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. When the memory
+ * area is no longer used, ban all access to it so any illegal access
+ * can be caught. xfree_mmap() does not really free memory.
+ */
+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 28496ed..62d8a37 100644
--- a/refs.c
+++ b/refs.c
@@ -497,12 +497,22 @@ static int get_packed_ref(const char *ref, unsigned char *sha1)
return -1;
}
-const char *resolve_ref_unsafe(const char *ref, unsigned char *sha1, int reading, int *flag)
+const char *resolve_ref_unsafe_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/wrapper.c b/wrapper.c
index 85f09df..3120d97 100644
--- a/wrapper.c
+++ b/wrapper.c
@@ -60,6 +60,27 @@ void *xmallocz(size_t size)
return ret;
}
+void *xmalloc_mmap(size_t size, const char *file, int line)
+{
+ int *ret = mmap(NULL, size + sizeof(int*) * 3,
+ PROT_READ | PROT_WRITE, MAP_ANONYMOUS | MAP_PRIVATE,
+ -1, 0);
+ if (ret == (int*)-1)
+ die_errno("unable to mmap %lu bytes anonymously",
+ (unsigned long)size);
+
+ ret[0] = (int)file;
+ ret[1] = line;
+ ret[2] = size;
+ return ret + 3;
+}
+
+void xfree_mmap(void *p)
+{
+ if (p && mprotect(((int*)p) - 3, ((int*)p)[-1], 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.4.74.g639db
^ permalink raw reply related
* [PATCH 8/8] Enable GIT_DEBUG_MEMCHECK on git_pathname()
From: Nguyễn Thái Ngọc Duy @ 2011-11-17 9:32 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, Michael Haggerty, Jonathan Nieder,
Ramkumar Ramachandra, Nguyễn Thái Ngọc Duy
In-Reply-To: <1321522335-24193-1-git-send-email-pclouds@gmail.com>
Make git_pathname() use xmalloc_mmap/xfree_mmap to catch invalid access
to old buffer when it's already overwritten.
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
cache.h | 11 +++++++----
path.c | 28 +++++++++++++++++++---------
2 files changed, 26 insertions(+), 13 deletions(-)
diff --git a/cache.h b/cache.h
index feb44a5..66365fb 100644
--- a/cache.h
+++ b/cache.h
@@ -661,10 +661,13 @@ extern char *git_pathdup(const char *fmt, ...)
__attribute__((format (printf, 1, 2)));
/* Return a statically allocated filename matching the sha1 signature */
-extern char *mkpath(const char *fmt, ...) __attribute__((format (printf, 1, 2)));
-extern char *git_path(const char *fmt, ...) __attribute__((format (printf, 1, 2)));
-extern char *git_path_submodule(const char *path, const char *fmt, ...)
- __attribute__((format (printf, 2, 3)));
+#define mkpath(...) mkpath_real(__FUNCTION__, __LINE__, __VA_ARGS__)
+extern char *mkpath_real(const char *file, int line, const char *fmt, ...) __attribute__((format (printf, 3, 4)));
+#define git_path( ...) git_path_real(__FUNCTION__, __LINE__, __VA_ARGS__)
+extern char *git_path_real(const char *file, int line, const char *fmt, ...) __attribute__((format (printf, 3, 4)));
+#define git_path_submodule(path, ...) git_path_submodule_real(__FUNCTION__, __LINE__, path, __VA_ARGS__)
+extern char *git_path_submodule_real(const char *file, int line, const char *path, const char *fmt, ...)
+ __attribute__((format (printf, 4, 5)));
extern char *sha1_file_name(const unsigned char *sha1);
extern char *sha1_pack_name(const unsigned char *sha1);
diff --git a/path.c b/path.c
index b6f71d1..d2aa941 100644
--- a/path.c
+++ b/path.c
@@ -15,11 +15,21 @@
static char bad_path[] = "/bad-path/";
-static char *get_pathname(void)
+static char *get_pathname(const char *file, int line)
{
- static char pathname_array[4][PATH_MAX];
+ static char real_pathname_array[4][PATH_MAX];
+ static char *pathname_array[4];
static int index;
- return pathname_array[3 & ++index];
+ int idx = 3 & ++index;
+
+ if (!pathname_array[idx] && !getenv("GIT_DEBUG_MEMCHECK"))
+ pathname_array[idx] = real_pathname_array[idx];
+
+ if (pathname_array[idx] != real_pathname_array[idx]) {
+ xfree_mmap(pathname_array[idx]);
+ pathname_array[idx] = xmalloc_mmap(PATH_MAX, file, line);
+ }
+ return pathname_array[idx];
}
static char *cleanup_path(char *path)
@@ -87,11 +97,11 @@ char *git_pathdup(const char *fmt, ...)
return xstrdup(path);
}
-char *mkpath(const char *fmt, ...)
+char *mkpath_real(const char *file, int line, const char *fmt, ...)
{
va_list args;
unsigned len;
- char *pathname = get_pathname();
+ char *pathname = get_pathname(file, line);
va_start(args, fmt);
len = vsnprintf(pathname, PATH_MAX, fmt, args);
@@ -101,10 +111,10 @@ char *mkpath(const char *fmt, ...)
return cleanup_path(pathname);
}
-char *git_path(const char *fmt, ...)
+char *git_path_real(const char *file, int line, const char *fmt, ...)
{
const char *git_dir = get_git_dir();
- char *pathname = get_pathname();
+ char *pathname = get_pathname(file, line);
va_list args;
unsigned len;
@@ -122,9 +132,9 @@ char *git_path(const char *fmt, ...)
return cleanup_path(pathname);
}
-char *git_path_submodule(const char *path, const char *fmt, ...)
+char *git_path_submodule_real(const char *file, int line, const char *path, const char *fmt, ...)
{
- char *pathname = get_pathname();
+ char *pathname = get_pathname(file, line);
struct strbuf buf = STRBUF_INIT;
const char *git_dir;
va_list args;
--
1.7.4.74.g639db
^ permalink raw reply related
* Re: [PATCH] i18n: add infrastructure for translating Git with gettext
From: Ævar Arnfjörð Bjarmason @ 2011-11-17 10:07 UTC (permalink / raw)
To: Jonathan Nieder
Cc: Junio C Hamano, git, Jakub Narebski, Jeff Epler, Johannes Sixt,
Erik Faye-Lund, Thomas Rast, Peter Krefting
In-Reply-To: <20111116103319.GB30753@elie.hsd1.il.comcast.net>
On Wed, Nov 16, 2011 at 11:33, Jonathan Nieder <jrnieder@gmail.com> wrote:
> Ævar Arnfjörð Bjarmason wrote:
>
>> Here's what we do with the sharedir currently:
>>
>> * It's used as the gitwebdir already, and is used also as the
>> localedir with this patch:
>>
>> sharedir = $(prefix)/share
>> gitwebdir = $(sharedir)/gitweb
>> localedir = $(sharedir)/locale
>
> This has $(prefix)/ (e.g., /usr/) at the start.
Yes I'm an idiot. I confused the installation part with stuff we
actually make in the git compilation directory.
The i18n code is indeed the onle thing that uses "share" in the
compilation directory.
Thanks for spotting that.
^ permalink raw reply
* Re: [PATCH 6/8] Convert resolve_ref_unsafe+xstrdup to resolve_ref
From: Ramkumar Ramachandra @ 2011-11-17 10:22 UTC (permalink / raw)
To: Nguyễn Thái Ngọc Duy
Cc: git, Junio C Hamano, Jeff King, Michael Haggerty, Jonathan Nieder
In-Reply-To: <1321522335-24193-7-git-send-email-pclouds@gmail.com>
Hi Nguyễn,
Nguyễn Thái Ngọc Duy wrote:
> diff --git a/builtin/checkout.c b/builtin/checkout.c
> index 2b8e73b..6efb1cf 100644
> --- a/builtin/checkout.c
> +++ b/builtin/checkout.c
> @@ -696,15 +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 = xstrdup(resolve_ref_unsafe("HEAD", rev, 0, &flag));
> + old.path = path = resolve_ref("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/");
This caught my eye immediately: you introduced a new "path" variable.
Let's scroll ahead and see why.
> @@ -718,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);
> @@ -727,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;
> }
Before application of your patch, if !(flag & REF_ISSYMREF) then
old.path is set to NULL and this free() would've read free(NULL). Was
this codepath ever reached, and did you fix a bug by introducing the
new "path" variable, or was it never reached but you introduced the
new variable for clarity anyway? Either case is worth mentioning in
the commit message, I think.
> diff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c
> index b954ca8..dc19f3c 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_unsafe(ref->refname, unused1, 1, NULL);
> - if (symref)
> - ref->symref = xstrdup(symref);
> - else
> + ref->symref = resolve_ref(ref->refname, unused1, 1, NULL);
> + if (!ref->symref)
> ref->symref = "";
> }
> [...]
This bit along with the others follow a common pattern: one temporary
variable was used to capture the value of resolve_ref_unsafe(), before
using xstrdup() on it to perform the actual assignment. You changed
the resolve_ref_unsafe() to resolve_ref() and got rid of that extra
variable.
Thanks.
-- Ram
^ permalink raw reply
* Re: [PATCH 8/8] Enable GIT_DEBUG_MEMCHECK on git_pathname()
From: Ramkumar Ramachandra @ 2011-11-17 10:35 UTC (permalink / raw)
To: Nguyễn Thái Ngọc Duy
Cc: git, Junio C Hamano, Jeff King, Michael Haggerty, Jonathan Nieder
In-Reply-To: <1321522335-24193-9-git-send-email-pclouds@gmail.com>
Hi Nguyễn,
Nguyễn Thái Ngọc Duy writes:
> diff --git a/cache.h b/cache.h
> index feb44a5..66365fb 100644
> --- a/cache.h
> +++ b/cache.h
> @@ -661,10 +661,13 @@ extern char *git_pathdup(const char *fmt, ...)
> __attribute__((format (printf, 1, 2)));
>
> /* Return a statically allocated filename matching the sha1 signature */
> -extern char *mkpath(const char *fmt, ...) __attribute__((format (printf, 1, 2)));
> -extern char *git_path(const char *fmt, ...) __attribute__((format (printf, 1, 2)));
> -extern char *git_path_submodule(const char *path, const char *fmt, ...)
> - __attribute__((format (printf, 2, 3)));
> +#define mkpath(...) mkpath_real(__FUNCTION__, __LINE__, __VA_ARGS__)
> +extern char *mkpath_real(const char *file, int line, const char *fmt, ...) __attribute__((format (printf, 3, 4)));
> +#define git_path( ...) git_path_real(__FUNCTION__, __LINE__, __VA_ARGS__)
> +extern char *git_path_real(const char *file, int line, const char *fmt, ...) __attribute__((format (printf, 3, 4)));
> +#define git_path_submodule(path, ...) git_path_submodule_real(__FUNCTION__, __LINE__, path, __VA_ARGS__)
> +extern char *git_path_submodule_real(const char *file, int line, const char *path, const char *fmt, ...)
> + __attribute__((format (printf, 4, 5)));
The macros __FILE__, __LINE__ and __VA_ARGS__ are gcc-specific
extensions, no? I was curious to see if some other parts of Git are
using this: a quick grep returns mailmap.c and notes-merge.c. They
both use __VA_ARGS__ it for debugging purposes. So, nothing new.
What happens if GIT_DEBUG_MEMCHECK is set, but I'm not using gcc?
Also, it's probably worth mentioning in the commit message that this
debugging trick is gcc-specific.
Thanks for working on this.
-- Ram
^ permalink raw reply
* Re: [PATCH 4/8] cmd_merge: convert to single exit point
From: Ramkumar Ramachandra @ 2011-11-17 10:39 UTC (permalink / raw)
To: Nguyễn Thái Ngọc Duy
Cc: git, Junio C Hamano, Jeff King, Michael Haggerty, Jonathan Nieder
In-Reply-To: <1321522335-24193-5-git-send-email-pclouds@gmail.com>
Hi again,
Nguyễn Thái Ngọc Duy wrote:
> [Subject: [PATCH 4/8] cmd_merge: convert to single exit point]
>
> This makes post-processing easier.
>
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
> builtin/merge.c | 48 +++++++++++++++++++++++++++++-------------------
> 1 files changed, 29 insertions(+), 19 deletions(-)
Um, (how) does this seemingly unrelated patch belong to the series?
Thanks.
-- Ram
^ permalink raw reply
* Re: [PATCH 0/8] nd/resolve-ref v2
From: Jonathan Nieder @ 2011-11-17 10:39 UTC (permalink / raw)
To: Nguyễn Thái Ngọc Duy
Cc: git, Junio C Hamano, Jeff King, Michael Haggerty,
Ramkumar Ramachandra
In-Reply-To: <1321522335-24193-1-git-send-email-pclouds@gmail.com>
Hi,
Nguyễn Thái Ngọc Duy wrote:
> Nguyễn Thái Ngọc Duy (8):
Here are some comments from a quick glance over the series, to avoid
too much noise that would distract from reports about the upcoming
release.
> Convert many resolve_ref() calls to read_ref*() and ref_exists()
> Rename resolve_ref() to resolve_ref_unsafe()
I like the general direction of the series (and especially patch 1/8).
As Junio nicely explains at [1], it is too tempting to keep and pass
around the canonical representation of a refname returned by
resolve_ref() without remembering to copy it.
> Re-add resolve_ref() that always returns an allocated buffer
Makes me nervous, since it would introduce memory leaks if some other
patch in flight calls resolve_ref(). Why not call it ref_resolve() or
something?
> cmd_merge: convert to single exit point
> Use resolve_ref() instead of resolve_ref_unsafe()
The print_summary() change introduces a leak.
[...]
> Guard memory overwriting in resolve_ref_unsafe's static buffer
> Enable GIT_DEBUG_MEMCHECK on git_pathname()
__VA_ARGS__ was introduced in C99. I suspect some compilers that
currently can compile git don't support it. But if you need to use
it, that wouldn't rule out doing so in some corner guarded with an
#ifdef.
Looks pleasant overall. I look forward to looking more closely at
this later.
Ciao,
Jonathan
[1] http://thread.gmane.org/gmane.comp.version-control.git/185446/focus=185519
^ permalink raw reply
* Re: [PATCH 4/8] cmd_merge: convert to single exit point
From: Jonathan Nieder @ 2011-11-17 10:44 UTC (permalink / raw)
To: Ramkumar Ramachandra
Cc: Nguyễn Thái Ngọc Duy, git, Junio C Hamano,
Jeff King, Michael Haggerty
In-Reply-To: <CALkWK0kME4fgLK0S+sFRXmDX1uj_N5+PZnvLFJp33qNssPptWQ@mail.gmail.com>
Ramkumar Ramachandra wrote:
> Nguyễn Thái Ngọc Duy wrote:
>> [Subject: [PATCH 4/8] cmd_merge: convert to single exit point]
>>
>> This makes post-processing easier.
>>
>> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
[...]
> Um, (how) does this seemingly unrelated patch belong to the series?
It's as Duy says --- it makes post-processing, for example to free()
a variable before returning, easier. Which simplifies the next patch.
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox