* Re: [PATCH] parse_object: clear "parsed" when freeing buffers
From: Jonathon Mah @ 2013-01-24 23:27 UTC (permalink / raw)
To: Jeff King; +Cc: Junio C Hamano, git
In-Reply-To: <20130124070715.GD610@sigill.intra.peff.net>
On 2013-01-23, at 23:07, Jeff King <peff@peff.net> wrote:
> On Wed, Jan 23, 2013 at 01:25:04PM -0800, Jonathon Mah wrote:
>
>> Several areas of code would free buffers for object structs that
>> contained them ("struct tree" and "struct commit"), but without clearing
>> the "parsed" flag. parse_object would clear the flag for "struct tree",
>> but commits would remain in an invalid state (marked as parsed but with
>> a NULL buffer). Because the objects are uniqued (ccdc6037fee), the
>> invalid objects stay around and can lead to bad behavior.
>>
>> In particular, running pickaxe on all refs in a repository with a cached
>> textconv could segfault. If the textconv cache ref was evaluated first
>> by cmd_log_walk, a subsequent notes_cache_match_validity call would
>> dereference NULL.
>
> Just to be sure I understand, what is going on is something like this?
>
> Log reads all refs, including notes. After showing the notes commit,
> log frees the buffer to save memory. Later, doing the diff on a
> commit causes us to use the notes_cache mechanism. That will look at
> the commit at the tip of the notes ref, which log has already
> processed. It will call parse_commit, but that will do nothing, as the
> parsed flag is already set, but the commit buffer remains NULL.
>
> I wonder if this could be the source of the segfault here, too:
>
> http://thread.gmane.org/gmane.comp.version-control.git/214322/focus=214355
Indeed, that description matches what I see. The other segfault seems to be in the same place too.
The segfault hits with the following stack (optimization off):
0 git-log parse_commit_header + 39 (pretty.c:738)
1 git-log format_commit_one + 3102 (pretty.c:1138)
2 git-log format_commit_item + 177 (pretty.c:1203)
3 git-log strbuf_expand + 172 (strbuf.c:247)
4 git-log format_commit_message + 196 (pretty.c:1263)
5 git-log notes_cache_match_validity + 215 (notes-cache.c:22)
6 git-log notes_cache_init + 212 (notes-cache.c:41)
7 git-log userdiff_get_textconv + 176 (userdiff.c:301)
8 git-log get_textconv + 66 (diff.c:2233)
9 git-log has_changes + 53 (diffcore-pickaxe.c:205)
10 git-log pickaxe + 299 (diffcore-pickaxe.c:40)
11 git-log diffcore_pickaxe_count + 344 (diffcore-pickaxe.c:275)
12 git-log diffcore_pickaxe + 58 (diffcore-pickaxe.c:289)
13 git-log diffcore_std + 162 (diff.c:4673)
14 git-log log_tree_diff_flush + 43 (log-tree.c:721)
15 git-log log_tree_diff + 566 (log-tree.c:826)
16 git-log log_tree_commit + 63 (log-tree.c:847)
17 git-log cmd_log_walk + 172 (log.c:301)
18 git-log cmd_log + 186 (log.c:568)
19 git-log run_builtin + 475 (git.c:273)
20 git-log handle_internal_command + 199 (git.c:434)
21 git-log main + 149 (git.c:523)
22 libdyld.dylib start + 1
commit->message was freed and nulled earlier in a call to cmd_log_walk. This test reproduces it reliably on my machine:
diff --git a/t/t4042-diff-textconv-caching.sh b/t/t4042-diff-textconv-caching.sh
index 91f8198..d7e26ca 100755
--- a/t/t4042-diff-textconv-caching.sh
+++ b/t/t4042-diff-textconv-caching.sh
@@ -106,4 +106,15 @@ test_expect_success 'switching diff driver produces correct results' '
test_cmp expect actual
'
+cat >expect <<EOF
+./helper other (refs/notes/textconv/magic)
+one
+EOF
+# add empty commit on master to make bug more reproducible
+test_expect_success 'pickaxe with cached textconv' '
+ git commit --allow-empty -m another &&
+ git log -S other --pretty=tformat:%s%d --all >actual &&
+ test_cmp expect actual
+'
+
test_done
Jonathon Mah
me@JonathonMah.com
^ permalink raw reply related
* Re: segmentation fault (nullpointer) with git log --submodule -p
From: Junio C Hamano @ 2013-01-24 23:56 UTC (permalink / raw)
To: Jeff King
Cc: Duy Nguyen, Stefan Näwe, Armin, Jonathon Mah,
git@vger.kernel.org
In-Reply-To: <20130124232721.GA16036@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> ... (e.g., how should "log" know that a submodule diff might later want
> to see the same entry? Should we optimistically free and then make it
> easier for the later user to reliably ensure the buffer is primed? Or
> should we err on the side of keeping it in place?).
My knee-jerk reaction is that we should consider that commit->buffer
belongs to the revision traversal machinery. Any other uses bolted
on later can borrow it if buffer still exists (I do not think pretty
code rewrites the buffer contents in place in any way), or they can
ask read_sha1_file() to read it themselves and free when they are
done.
^ permalink raw reply
* Re: segmentation fault (nullpointer) with git log --submodule -p
From: Jeff King @ 2013-01-25 0:55 UTC (permalink / raw)
To: Junio C Hamano
Cc: Duy Nguyen, Stefan Näwe, Armin, Jonathon Mah,
git@vger.kernel.org
In-Reply-To: <7va9ry87a0.fsf@alter.siamese.dyndns.org>
On Thu, Jan 24, 2013 at 03:56:23PM -0800, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
>
> > ... (e.g., how should "log" know that a submodule diff might later want
> > to see the same entry? Should we optimistically free and then make it
> > easier for the later user to reliably ensure the buffer is primed? Or
> > should we err on the side of keeping it in place?).
>
> My knee-jerk reaction is that we should consider that commit->buffer
> belongs to the revision traversal machinery. Any other uses bolted
> on later can borrow it if buffer still exists (I do not think pretty
> code rewrites the buffer contents in place in any way), or they can
> ask read_sha1_file() to read it themselves and free when they are
> done.
Yeah, that is probably the sanest way forward. It at least leaves
ownership in one place, and everybody else is an opportunistic consumer.
We do need to annotate each user site though with something like the
"ensure" function I mentioned.
If they are not owners, then the better pattern is probably something
like:
/*
* Get the commit buffer, either opportunistically using
* the cached version attached to the commit object, or loading it
* from disk if necessary.
*/
const char *use_commit_buffer(struct commit *c)
{
enum object_type type;
unsigned long size;
if (c->buffer)
return c->buffer;
/*
* XXX check type == OBJ_COMMIT?
* XXX die() on NULL as a convenience to callers?
*/
return read_sha1_file(c->object.sha1, &type, &size);
}
void unuse_commit_buffer(const char *buf, struct commit *c)
{
/*
* If we used the cached copy attached to the commit,
* we don't want to free it; it's not our responsibility.
*/
if (buf == c->buffer)
return;
free((char *)buf);
}
I suspect that putting a use/unuse pair inside format_commit_message
would handle most cases.
-Peff
^ permalink raw reply
* Re: [regression] Re: [PATCHv2 10/15] drop length limitations on gecos-derived names and emails
From: Jeff King @ 2013-01-25 1:05 UTC (permalink / raw)
To: Jonathan Nieder; +Cc: Junio C Hamano, Angus Hammond, git, Mihai Rusu
In-Reply-To: <20130124232146.GA17458@google.com>
On Thu, Jan 24, 2013 at 03:21:46PM -0800, Jonathan Nieder wrote:
> This broke /etc/mailname handling. Before:
>
> $ git var GIT_COMMITTER_IDENT
> Jonathan Nieder <jrn@mailname.example.com> 1359069165 -0800
>
> After:
>
> $ git var GIT_COMMITTER_IDENT
> Jonathan Nieder <mailname.example.com> 1359069192 -0800
Ick. I wonder how that slipped through...I know I was testing with
/etc/mailname when developing the series, because I'm on a Debian
system. We do even check this code path in t7502 (if you have the
AUTOIDENT prereq), but of course we can't verify the actual value
automatically, because it could be anything. So I guess I just missed it
during my manual testing, and the automated testing is insufficient to
catch this particular breakage.
> > - if (!fgets(buf, len, mailname)) {
> > + if (strbuf_getline(buf, mailname, '\n') == EOF) {
>
> This clears the strbuf.
Right. Definitely the problem.
> How about something like this as a quick fix?
>
> Reported-by: Mihai Rusu <dizzy@google.com>
> Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
>
> diff --git a/ident.c b/ident.c
> index 73a06a1..cabd73f 100644
> --- a/ident.c
> +++ b/ident.c
> @@ -41,6 +41,7 @@ static void copy_gecos(const struct passwd *w, struct strbuf *name)
> static int add_mailname_host(struct strbuf *buf)
> {
> FILE *mailname;
> + struct strbuf mailnamebuf = STRBUF_INIT;
>
> mailname = fopen("/etc/mailname", "r");
> if (!mailname) {
> @@ -49,14 +50,17 @@ static int add_mailname_host(struct strbuf *buf)
> strerror(errno));
> return -1;
> }
> - if (strbuf_getline(buf, mailname, '\n') == EOF) {
> + if (strbuf_getline(&mailnamebuf, mailname, '\n') == EOF) {
> if (ferror(mailname))
> warning("cannot read /etc/mailname: %s",
> strerror(errno));
> + strbuf_release(&mailnamebuf);
> fclose(mailname);
> return -1;
> }
> /* success! */
> + strbuf_addbuf(buf, &mailnamebuf);
> + strbuf_release(&mailnamebuf);
> fclose(mailname);
> return 0;
> }
I think that is the only reasonable fix. Thanks for figuring it out.
We could expand the test in t7502 to check for "@" in the email, but it
feels weirdly specific to this bug. Either way,
Acked-by: Jeff King <peff@peff.net>
(with a proper commit message, of course).
-Peff
^ permalink raw reply
* Re: [PATCH] t9902: protect test from stray build artifacts
From: Jeff King @ 2013-01-25 1:13 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jean-Noël AVILA, git
In-Reply-To: <7vehha89j5.fsf_-_@alter.siamese.dyndns.org>
On Thu, Jan 24, 2013 at 03:07:42PM -0800, Junio C Hamano wrote:
> When you have random build artifacts in your build directory, left
> behind by running "make" while on another branch, the "git help -a"
> command run by __git_list_all_commands in the completion script that
> is being tested does not have a way to know that they are not part
> of the subcommands this build will ship. Such extra subcommands may
> come from the user's $PATH. They will interfere with the tests that
> expect a certain prefix to uniquely expand to a known completion.
>
> Instrument the completion script and give it a way for us to tell
> what (subset of) subcommands we are going to ship.
>
> Also add a test to "git --help <prefix><TAB>" expansion. It needs
> to show not just commands but some selected documentation pages.
>
> Based on an idea by Jeff King.
>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
> contrib/completion/git-completion.bash | 11 ++++++++++-
> t/t9902-completion.sh | 25 ++++++++++++++++++++++++-
> 2 files changed, 34 insertions(+), 2 deletions(-)
This looks good to me.
The only thing I might add is a test just to double-check that "git help
-a" is parsed correctly. Like:
test_expect_success 'command completion works without test harness' '
GIT_TESTING_COMMAND_COMPLETION= run_completion "git bun" &&
grep "^bundle\$" out
'
(we know we are running bash here, so the one-shot variable is OK to be
used with a function).
-Peff
^ permalink raw reply
* Re: [PATCH 1/7] sha1_file: keep track of where an SHA-1 object comes from
From: Duy Nguyen @ 2013-01-25 1:38 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Jens Lehmann
In-Reply-To: <7va9rycw4t.fsf@alter.siamese.dyndns.org>
On Fri, Jan 25, 2013 at 12:45 AM, Junio C Hamano <gitster@pobox.com> wrote:
>> How about this way instead: we keep track of where objects come from
>> so we can verify object source when we create or update something
>> that contains SHA-1.
>
> The overall approach taken by this series may be worth considering, but
> I do not think the check implemented here is correct.
>
> An object may be found in an alternate odb but we may also have our
> own copy of the same object. You need to prove that a suspicious
> object is visible to us *ONLY* through add_submodule_odb().
The way alt odbs are linked (new odbs area always at the end), if we
have the same copy, their copy will never be read (we check out alt
odbs from head to tail). So those duplicate suspicious objects are
actually invisible to us.
> Once you do add_submodule_odb() to contaminate our object pool, you
> make everything a suspicious object that needs to be checked; that
> is the worst part of the story.
And because we never really touch their alt copy, the returned alt
source is "ours", trusted and not checked. The check should only occur
on objects that we do not have.
--
Duy
^ permalink raw reply
* Re: [PATCH v3 01/10] wildmatch: fix "**" special case
From: Duy Nguyen @ 2013-01-25 1:46 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <7v1udacugj.fsf@alter.siamese.dyndns.org>
On Fri, Jan 25, 2013 at 1:22 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Duy Nguyen <pclouds@gmail.com> writes:
>
>> I don't think we need to support two different sets of wildcards in
>> the long run. I'm thinking of adding ":(glob)" with WM_PATHNAME.
>> Pathspec without :(glob) still uses the current wildcards (i.e. no
>> FNM_PATHNAME). At some point, like 2.0, we either switch the behavior
>> of patterns-without-:(glob) to WM_PATHNAME, or just disable wildcards
>> when :(glob) is not present.
>
> Yeah, I think that is sensible.
>
> I am meaning to merge your retire-fnmatch topic to 'master'.
I thought you wanted it to stay in 'next' longer :-)
> What should the Release Notes say for the upcoming release?
>
> You can build with USE_WILDMATCH=YesPlease to use a replacement
> implementation of pattern matching logic used for pathname-like
> things, e.g. refnames and paths in the repository. The new
> implementation is not expected change the existing behaviour of
> Git at all, except for "git for-each-ref" where you can now say
> "refs/**/master" and match with both refs/heads/master and
> refs/remotes/origin/master. In future versions of Git, we plan
> to use this new implementation in wider places (e.g. "git log
> '**/t*.sh'" may find commits that touch a shell script whose
> name begins with "t" at any level), but we are not there yet.
> By building with USE_WILDMATCH, using the resulting Git daily
> and reporting when you find breakages, you can help us get
> closer to that goal.
That looks ok. You may want to mention that "**" syntax is enabled in
.gitignore and .gitattributes as well (even without USE_WILDMATCH). We
could even stop the behavior change in for-each-ref (FNM_PATHNAME in
general) but I guess because it's a nice regression, nobody would
complain.
--
Duy
^ permalink raw reply
* Re: [PATCH 6/7] read-cache: refuse to create index referring to external objects
From: Duy Nguyen @ 2013-01-25 2:00 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Jens Lehmann
In-Reply-To: <7vpq0ubdec.fsf@alter.siamese.dyndns.org>
On Fri, Jan 25, 2013 at 2:15 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:
>
>> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
>> ---
>> read-cache.c | 20 ++++++++++++++++++++
>> 1 file changed, 20 insertions(+)
>>
>> diff --git a/read-cache.c b/read-cache.c
>> index fda78bc..4579215 100644
>> --- a/read-cache.c
>> +++ b/read-cache.c
>> @@ -1720,6 +1720,26 @@ static int ce_write_entry(git_SHA_CTX *c, int fd, struct cache_entry *ce,
>> ce->name + common, ce_namelen(ce) - common);
>> }
>>
>> + if (object_database_contaminated) {
>> + struct object_info oi;
>> + switch (ce->ce_mode) {
>> + case S_IFGITLINK:
>> + break;
>> + case S_IFDIR:
>> + if (sha1_object_info_extended(ce->sha1, &oi) != OBJ_TREE ||
>
> This case should never happen. Do we store any tree object in the
> index in the first place?
No it was copy/paste mistake (from cache-tree.c, before I deleted it
and added check_sha1_file_for_external_source in 3/7)
>> + (oi.alt && oi.alt->external))
>> + die("cannot create index referring to an external tree %s",
>> + sha1_to_hex(ce->sha1));
>> + break;
>> + default:
>> + if (sha1_object_info_extended(ce->sha1, &oi) != OBJ_BLOB ||
>> + (oi.alt && oi.alt->external))
>> + die("cannot create index referring to an external blob %s",
>> + sha1_to_hex(ce->sha1));
>> + break;
>> + }
>> + }
>> +
>> result = ce_write(c, fd, ondisk, size);
>> free(ondisk);
>> return result;
>
> I think the check you want to add is to the cache-tree code; before
> writing the new index out, if you suspect you might have called
> cache_tree_update() before, invalidate any hierarchy that is
> recorded to produce a tree object that refers to an object that is
> only available through external object store.
cache-tree is covered by check_sha1_file_for_external_source() when it
actually writes a tree (dry-run mode goes through unchecked). Even
when cache-tree is not involved, I do not want the index to point to
an non-existing SHA-1 ("git diff --cached" may fail next time, for
example).
> As to the logic to check if a object is something that we should
> refuse to create a new dependent, I think you should not butcher
> sha1_object_info_extended(), and instead add a new call that checks
> if a given SHA-1 yields an object if you ignore alternate object
> databases that are marked as "external", whose signature may
> resemble:
>
> int has_sha1_file_proper(const unsigned char *sha1);
>
> or something.
Agreed.
> This is a tangent, but in addition, you may want to add an even
> narrower variant that checks the same but ignoring _all_ alternate
> object databases, "external" or not:
>
> int has_sha1_file_local(const unsigned char *sha1);
>
> That may be useful if we want to later make the alternate safer to
> use; instead of letting the codepath to create an object ask
> has_sha1_file() to see if it already exists and refrain from writing
> a new copy, we switch to ask has_sha1_file_locally() and even if an
> alternate object database we borrow from has it, we keep our own
> copy in our repository.
>
> Not tested beyond syntax, but rebasing something like this patch on
> top of your "mark external sources" change may take us closer to
> that.
Thanks, will incorporate in the reroll.
--
Duy
^ permalink raw reply
* Re: segmentation fault (nullpointer) with git log --submodule -p
From: Duy Nguyen @ 2013-01-25 2:05 UTC (permalink / raw)
To: Jeff King
Cc: Junio C Hamano, Stefan Näwe, Armin, Jonathon Mah,
git@vger.kernel.org
In-Reply-To: <20130125005528.GA27325@sigill.intra.peff.net>
On Fri, Jan 25, 2013 at 7:55 AM, Jeff King <peff@peff.net> wrote:
> On Thu, Jan 24, 2013 at 03:56:23PM -0800, Junio C Hamano wrote:
>
>> Jeff King <peff@peff.net> writes:
>>
>> > ... (e.g., how should "log" know that a submodule diff might later want
>> > to see the same entry? Should we optimistically free and then make it
>> > easier for the later user to reliably ensure the buffer is primed? Or
>> > should we err on the side of keeping it in place?).
>>
>> My knee-jerk reaction is that we should consider that commit->buffer
>> belongs to the revision traversal machinery. Any other uses bolted
>> on later can borrow it if buffer still exists (I do not think pretty
>> code rewrites the buffer contents in place in any way), or they can
>> ask read_sha1_file() to read it themselves and free when they are
>> done.
>
> Yeah, that is probably the sanest way forward. It at least leaves
> ownership in one place, and everybody else is an opportunistic consumer.
> We do need to annotate each user site though with something like the
> "ensure" function I mentioned.
>
> If they are not owners, then the better pattern is probably something
> like:
You probably should rename "buffer" (to _buffer or something) and let
the compiler catches all the places commit->buffer illegally used.
>
> /*
> * Get the commit buffer, either opportunistically using
> * the cached version attached to the commit object, or loading it
> * from disk if necessary.
> */
> const char *use_commit_buffer(struct commit *c)
> {
> enum object_type type;
> unsigned long size;
>
> if (c->buffer)
> return c->buffer;
> /*
> * XXX check type == OBJ_COMMIT?
> * XXX die() on NULL as a convenience to callers?
> */
> return read_sha1_file(c->object.sha1, &type, &size);
> }
>
> void unuse_commit_buffer(const char *buf, struct commit *c)
> {
> /*
> * If we used the cached copy attached to the commit,
> * we don't want to free it; it's not our responsibility.
> */
> if (buf == c->buffer)
> return;
>
> free((char *)buf);
> }
>
> I suspect that putting a use/unuse pair inside format_commit_message
> would handle most cases.
>
> -Peff
--
Duy
^ permalink raw reply
* Re: [PATCH] t9902: protect test from stray build artifacts
From: Jonathan Nieder @ 2013-01-25 2:59 UTC (permalink / raw)
To: Jeff King; +Cc: Junio C Hamano, Jean-Noël AVILA, git, Daniel Baumann
In-Reply-To: <20130125011349.GB27657@sigill.intra.peff.net>
Jeff King wrote:
> On Thu, Jan 24, 2013 at 03:07:42PM -0800, Junio C Hamano wrote[1]:
>> Instrument the completion script and give it a way for us to tell
>> what (subset of) subcommands we are going to ship.
[...]
> The only thing I might add is a test just to double-check that "git help
> -a" is parsed correctly. Like:
>
> test_expect_success 'command completion works without test harness' '
> GIT_TESTING_COMMAND_COMPLETION= run_completion "git bun" &&
> grep "^bundle\$" out
> '
Yes. Since there are no other 'git help -a' tests, I think we need
this.
Aside from that, the fix looks good to me.
Jonathan
[1] http://thread.gmane.org/gmane.comp.version-control.git/214167/focus=214469
^ permalink raw reply
* [PATCH] Update renamed file merge-file.h in Makefile
From: Jiang Xin @ 2013-01-25 3:06 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Git List, Jiang Xin
Commit v1.8.1-rc0-3-gfa2364e renamed merge-file.h to merge-blobs.h, but
forgot to update the reference of merge-file.h in Makefile. This would
break the build of "po/git.pot", which depends on $(LOCALIZED_C), then
fallback to the missing file "merge-file.h".
Signed-off-by: Jiang Xin <worldhello.net@gmail.com>
---
Makefile | 2 +-
1 个文件被修改，插入 1 行(+)，删除 1 行(-)
diff --git a/Makefile b/Makefile
index 1b30d7b..a786d4c 100644
--- a/Makefile
+++ b/Makefile
@@ -649,7 +649,7 @@ LIB_H += list-objects.h
LIB_H += ll-merge.h
LIB_H += log-tree.h
LIB_H += mailmap.h
-LIB_H += merge-file.h
+LIB_H += merge-blobs.h
LIB_H += merge-recursive.h
LIB_H += mergesort.h
LIB_H += notes-cache.h
--
1.8.1.1
^ permalink raw reply related
* Re: [PATCH 1/7] sha1_file: keep track of where an SHA-1 object comes from
From: Junio C Hamano @ 2013-01-25 3:58 UTC (permalink / raw)
To: Duy Nguyen; +Cc: git, Jens Lehmann
In-Reply-To: <CACsJy8Ag6v7wUnupRwGid26AUzgZ=WbdA5F-MpjUv5ktaj5Asg@mail.gmail.com>
Duy Nguyen <pclouds@gmail.com> writes:
> On Fri, Jan 25, 2013 at 12:45 AM, Junio C Hamano <gitster@pobox.com> wrote:
>>> How about this way instead: we keep track of where objects come from
>>> so we can verify object source when we create or update something
>>> that contains SHA-1.
>>
>> The overall approach taken by this series may be worth considering, but
>> I do not think the check implemented here is correct.
>>
>> An object may be found in an alternate odb but we may also have our
>> own copy of the same object. You need to prove that a suspicious
>> object is visible to us *ONLY* through add_submodule_odb().
>
> The way alt odbs are linked (new odbs area always at the end), if we
> have the same copy, their copy will never be read (we check out alt
> odbs from head to tail). So those duplicate suspicious objects are
> actually invisible to us.
The way I read find_pack_entry() is that our heuristics to start
our search by looking at the pack in which we found an object the
last time will invalidate your assumption above. Am I mistaken?
^ permalink raw reply
* Re: segmentation fault (nullpointer) with git log --submodule -p
From: Junio C Hamano @ 2013-01-25 3:59 UTC (permalink / raw)
To: Jeff King
Cc: Duy Nguyen, Stefan Näwe, Armin, Jonathon Mah,
git@vger.kernel.org
In-Reply-To: <7va9ry87a0.fsf@alter.siamese.dyndns.org>
Junio C Hamano <gitster@pobox.com> writes:
> Jeff King <peff@peff.net> writes:
>
>> ... (e.g., how should "log" know that a submodule diff might later want
>> to see the same entry? Should we optimistically free and then make it
>> easier for the later user to reliably ensure the buffer is primed? Or
>> should we err on the side of keeping it in place?).
>
> My knee-jerk reaction is that we should consider that commit->buffer
> belongs to the revision traversal machinery. Any other uses bolted
> on later can borrow it if buffer still exists (I do not think pretty
> code rewrites the buffer contents in place in any way), or they can
> ask read_sha1_file() to read it themselves and free when they are
> done.
I've been toying with an idea along this line.
commit.h | 16 ++++++++++++++++
builtin/blame.c | 27 ++++++++-------------------
commit.c | 20 ++++++++++++++++++++
3 files changed, 44 insertions(+), 19 deletions(-)
diff --git a/commit.h b/commit.h
index c16c8a7..b559535 100644
--- a/commit.h
+++ b/commit.h
@@ -226,4 +226,20 @@ extern void print_commit_list(struct commit_list *list,
const char *format_cur,
const char *format_last);
+extern int ensure_commit_buffer(struct commit *);
+extern void discard_commit_buffer(struct commit *);
+
+#define with_commit_buffer(commit) \
+ do { \
+ int had_buffer_ = !!commit->buffer; \
+ if (!had_buffer_) \
+ ensure_commit_buffer(commit); \
+ do
+
+#define done_with_commit_buffer(commit) \
+ while (0); \
+ if (!had_buffer_) \
+ discard_commit_buffer(commit); \
+ } while (0)
+
#endif /* COMMIT_H */
diff --git a/builtin/blame.c b/builtin/blame.c
index b431ba3..8b2e4a5 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -1424,25 +1424,14 @@ static void get_commit_info(struct commit *commit,
commit_info_init(ret);
- /*
- * We've operated without save_commit_buffer, so
- * we now need to populate them for output.
- */
- if (!commit->buffer) {
- enum object_type type;
- unsigned long size;
- commit->buffer =
- read_sha1_file(commit->object.sha1, &type, &size);
- if (!commit->buffer)
- die("Cannot read commit %s",
- sha1_to_hex(commit->object.sha1));
- }
- encoding = get_log_output_encoding();
- reencoded = logmsg_reencode(commit, encoding);
- message = reencoded ? reencoded : commit->buffer;
- get_ac_line(message, "\nauthor ",
- &ret->author, &ret->author_mail,
- &ret->author_time, &ret->author_tz);
+ with_commit_buffer(commit) {
+ encoding = get_log_output_encoding();
+ reencoded = logmsg_reencode(commit, encoding);
+ message = reencoded ? reencoded : commit->buffer;
+ get_ac_line(message, "\nauthor ",
+ &ret->author, &ret->author_mail,
+ &ret->author_time, &ret->author_tz);
+ } done_with_commit_buffer(commit);
if (!detailed) {
free(reencoded);
diff --git a/commit.c b/commit.c
index e8eb0ae..a627eea 100644
--- a/commit.c
+++ b/commit.c
@@ -1357,3 +1357,23 @@ void print_commit_list(struct commit_list *list,
printf(format, sha1_to_hex(list->item->object.sha1));
}
}
+
+int ensure_commit_buffer(struct commit *commit)
+{
+ enum object_type type;
+ unsigned long size;
+
+ if (commit->buffer)
+ return 0;
+ commit->buffer = read_sha1_file(commit->object.sha1, &type, &size);
+ if (commit->buffer)
+ return -1;
+ else
+ return 0;
+}
+
+void discard_commit_buffer(struct commit *commit)
+{
+ free(commit->buffer);
+ commit->buffer = NULL;
+}
^ permalink raw reply related
* Re: [PATCH 1/7] sha1_file: keep track of where an SHA-1 object comes from
From: Duy Nguyen @ 2013-01-25 4:02 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Jens Lehmann
In-Reply-To: <7v4ni59ano.fsf@alter.siamese.dyndns.org>
On Fri, Jan 25, 2013 at 10:58 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Duy Nguyen <pclouds@gmail.com> writes:
>
>> On Fri, Jan 25, 2013 at 12:45 AM, Junio C Hamano <gitster@pobox.com> wrote:
>>>> How about this way instead: we keep track of where objects come from
>>>> so we can verify object source when we create or update something
>>>> that contains SHA-1.
>>>
>>> The overall approach taken by this series may be worth considering, but
>>> I do not think the check implemented here is correct.
>>>
>>> An object may be found in an alternate odb but we may also have our
>>> own copy of the same object. You need to prove that a suspicious
>>> object is visible to us *ONLY* through add_submodule_odb().
>>
>> The way alt odbs are linked (new odbs area always at the end), if we
>> have the same copy, their copy will never be read (we check out alt
>> odbs from head to tail). So those duplicate suspicious objects are
>> actually invisible to us.
>
> The way I read find_pack_entry() is that our heuristics to start
> our search by looking at the pack in which we found an object the
> last time will invalidate your assumption above. Am I mistaken?
No, you are right. Loose objects are always searched from the start of
alt odb list. Packs are searched till the end then back to head again.
--
Duy
^ permalink raw reply
* Re: segmentation fault (nullpointer) with git log --submodule -p
From: Jeff King @ 2013-01-25 4:08 UTC (permalink / raw)
To: Junio C Hamano
Cc: Duy Nguyen, Stefan Näwe, Armin, Jonathon Mah,
git@vger.kernel.org
In-Reply-To: <7vzjzx7w01.fsf@alter.siamese.dyndns.org>
On Thu, Jan 24, 2013 at 07:59:58PM -0800, Junio C Hamano wrote:
> Junio C Hamano <gitster@pobox.com> writes:
>
> > Jeff King <peff@peff.net> writes:
> >
> >> ... (e.g., how should "log" know that a submodule diff might later want
> >> to see the same entry? Should we optimistically free and then make it
> >> easier for the later user to reliably ensure the buffer is primed? Or
> >> should we err on the side of keeping it in place?).
> >
> > My knee-jerk reaction is that we should consider that commit->buffer
> > belongs to the revision traversal machinery. Any other uses bolted
> > on later can borrow it if buffer still exists (I do not think pretty
> > code rewrites the buffer contents in place in any way), or they can
> > ask read_sha1_file() to read it themselves and free when they are
> > done.
>
> I've been toying with an idea along this line.
>
> commit.h | 16 ++++++++++++++++
> builtin/blame.c | 27 ++++++++-------------------
> commit.c | 20 ++++++++++++++++++++
> 3 files changed, 44 insertions(+), 19 deletions(-)
I think we are on the same page as far as what needs to happen at the
call sites.
My suggested implementation had a separate buffer, but you are right
that we may need to actually set "commit->buffer" because sub-functions
expect to find it there (the alternative might be cleaning up the
sub-function interfaces). I haven't looked at the call-sites yet.
This:
> +extern int ensure_commit_buffer(struct commit *);
> +extern void discard_commit_buffer(struct commit *);
> +
> +#define with_commit_buffer(commit) \
> + do { \
> + int had_buffer_ = !!commit->buffer; \
> + if (!had_buffer_) \
> + ensure_commit_buffer(commit); \
> + do
> +
> +#define done_with_commit_buffer(commit) \
> + while (0); \
> + if (!had_buffer_) \
> + discard_commit_buffer(commit); \
> + } while (0)
is pretty nasty, though. I know it gets the job done, but in my
experience, macros which do not behave syntactically like functions are
usually a good sign that you are doing something gross and
unmaintainable.
I dunno.
-Peff
^ permalink raw reply
* Re: [PATCH] t9902: protect test from stray build artifacts
From: Junio C Hamano @ 2013-01-25 4:11 UTC (permalink / raw)
To: Jeff King; +Cc: Jean-Noël AVILA, git
In-Reply-To: <20130125011349.GB27657@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> This looks good to me.
>
> The only thing I might add is a test just to double-check that "git help
> -a" is parsed correctly. Like:
>
> test_expect_success 'command completion works without test harness' '
> GIT_TESTING_COMMAND_COMPLETION= run_completion "git bun" &&
> grep "^bundle\$" out
> '
>
> (we know we are running bash here, so the one-shot variable is OK to be
> used with a function).
I think you meant "^bundle $" there, but don't we have the same
problem when there is an end-user subcommand "git bunny"?
Ahh, ok, we show one element per line and just make sure "bundle"
is there, and we do not care what other buns appear in the output.
Will squash in, then.
^ permalink raw reply
* Re: [PATCH] t9902: protect test from stray build artifacts
From: Jeff King @ 2013-01-25 4:13 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jean-Noël AVILA, git
In-Reply-To: <7vvcal7vhg.fsf@alter.siamese.dyndns.org>
On Thu, Jan 24, 2013 at 08:11:07PM -0800, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
>
> > This looks good to me.
> >
> > The only thing I might add is a test just to double-check that "git help
> > -a" is parsed correctly. Like:
> >
> > test_expect_success 'command completion works without test harness' '
> > GIT_TESTING_COMMAND_COMPLETION= run_completion "git bun" &&
> > grep "^bundle\$" out
> > '
> >
> > (we know we are running bash here, so the one-shot variable is OK to be
> > used with a function).
>
> I think you meant "^bundle $" there, but don't we have the same
> problem when there is an end-user subcommand "git bunny"?
>
> Ahh, ok, we show one element per line and just make sure "bundle"
> is there, and we do not care what other buns appear in the output.
Exactly. At least that was the intent; I typed it straight into my MUA. :)
-Peff
^ permalink raw reply
* Re: [PATCH] t9902: protect test from stray build artifacts
From: Junio C Hamano @ 2013-01-25 4:19 UTC (permalink / raw)
To: Jeff King; +Cc: Jean-Noël AVILA, git
In-Reply-To: <7vvcal7vhg.fsf@alter.siamese.dyndns.org>
Junio C Hamano <gitster@pobox.com> writes:
> Jeff King <peff@peff.net> writes:
>
>> This looks good to me.
>>
>> The only thing I might add is a test just to double-check that "git help
>> -a" is parsed correctly. Like:
>>
>> test_expect_success 'command completion works without test harness' '
>> GIT_TESTING_COMMAND_COMPLETION= run_completion "git bun" &&
>> grep "^bundle\$" out
>> '
>>
>> (we know we are running bash here, so the one-shot variable is OK to be
>> used with a function).
>
> I think you meant "^bundle $" there, but don't we have the same
> problem when there is an end-user subcommand "git bunny"?
>
> Ahh, ok, we show one element per line and just make sure "bundle"
> is there, and we do not care what other buns appear in the output.
>
> Will squash in, then.
Not so quick, though. The lower level "read from help -a" is only
run once and its output kept in a two-level cache hierarchy; we need
to reset both.
It starts to look a bit too intimately tied to the implementation of
what is being tested for my taste, though.
t/t9902-completion.sh | 10 ++++++++++
1 file changed, 10 insertions(+)
diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
index adc1372..bb6ee1a 100755
--- a/t/t9902-completion.sh
+++ b/t/t9902-completion.sh
@@ -286,4 +286,14 @@ test_expect_success 'send-email' '
test_completion "git send-email ma" "master "
'
+# This is better to be at the end, as it resets the state by tweaking
+# the internals.
+test_expect_success 'help -a read correctly by command list generator' '
+ __git_all_commands= &&
+ __git_porcelain_commands= &&
+ GIT_TESTING_COMMAND_COMPLETION= &&
+ run_completion "git bun" &&
+ grep "^bundle $" out
+'
+
test_done
--
1.8.1.1.525.gdace530
^ permalink raw reply related
* Re: segmentation fault (nullpointer) with git log --submodule -p
From: Junio C Hamano @ 2013-01-25 4:21 UTC (permalink / raw)
To: Jeff King
Cc: Duy Nguyen, Stefan Näwe, Armin, Jonathon Mah,
git@vger.kernel.org
In-Reply-To: <20130125040856.GA30533@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> ... I know it gets the job done, but in my
> experience, macros which do not behave syntactically like functions are
> usually a good sign that you are doing something gross and
> unmaintainable.
Yeah, it is a bit too cute for my taste, too, even though it was fun
;-)
^ permalink raw reply
* Re: [PATCH] t9902: protect test from stray build artifacts
From: Jeff King @ 2013-01-25 4:23 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jean-Noël AVILA, git
In-Reply-To: <7vr4l97v3h.fsf@alter.siamese.dyndns.org>
On Thu, Jan 24, 2013 at 08:19:30PM -0800, Junio C Hamano wrote:
> > Ahh, ok, we show one element per line and just make sure "bundle"
> > is there, and we do not care what other buns appear in the output.
> >
> Not so quick, though. The lower level "read from help -a" is only
> run once and its output kept in a two-level cache hierarchy; we need
> to reset both.
Ugh, I didn't even think about that.
I wonder if it would be simpler if the completion tests actually ran a
new bash for each test. That would be slower, but it somehow seems
cleaner.
> It starts to look a bit too intimately tied to the implementation of
> what is being tested for my taste, though.
> [...]
> +test_expect_success 'help -a read correctly by command list generator' '
> + __git_all_commands= &&
> + __git_porcelain_commands= &&
> + GIT_TESTING_COMMAND_COMPLETION= &&
> + run_completion "git bun" &&
> + grep "^bundle $" out
> +'
Agreed. I could take or leave it at this point. It's nice to check that
changes to "help -a" will not break it, but ultimately it feels a bit
too contrived to catch anything useful.
-Peff
^ permalink raw reply
* Re: [PATCH v4 0/3] Finishing touches to "push" advises
From: Chris Rorvick @ 2013-01-25 4:31 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Jeff King
In-Reply-To: <1358978130-12216-1-git-send-email-gitster@pobox.com>
On Wed, Jan 23, 2013 at 3:55 PM, Junio C Hamano <gitster@pobox.com> wrote:
> This builds on Chris Rorvick's earlier effort to forbid unforced
> updates to refs/tags/ hierarchy and giving sensible error and advise
> messages for that case (we are not rejecting such a push due to fast
> forwardness, and suggesting to fetch and integrate before pushing
> again does not make sense).
FWIW, these changes look good to me. The logic in
set_ref_status_for_push() is easier to follow and the additional error
statuses (and associated advice) make things much clearer.
Had I written the the "already exists" advice in the context of these
additional statuses I would have said "the destination *tag* reference
already exists", or maybe even just "the destination *tag* already
exists". It's probably fine the way it is, but I only avoided using
"tag" in the advice because I was abusing it.
Thanks,
Chris
^ permalink raw reply
* Git 1.8.2 l10n round 1
From: Jiang Xin @ 2013-01-25 4:54 UTC (permalink / raw)
To: Byrial Jensen, Ralf Thielow,
Ævar Arnfjörð Bjarmason, Marco Paolone,
Vincent van Ravesteijn, Marco Sousa, Peter Krefting,
Trần Ngọc Quân, David Hrbáč
Cc: Git List
Dear l10n team members,
New "git.pot" is generated from v1.8.1-476-gec3ae6e in the master branch.
l10n: Update git.pot (11 new, 7 removed messages)
Generate po/git.pot from v1.8.1-476-gec3ae6e, and there are 11 new and 7
removed messages.
Signed-off-by: Jiang Xin <worldhello.net@gmail.com>
This update is for the l10n of upcoming git 1.8.2. You can get it from
the usual place:
https://github.com/git-l10n/git-po/
--
Jiang Xin
^ permalink raw reply
* Re: [PATCH v3 01/10] wildmatch: fix "**" special case
From: Junio C Hamano @ 2013-01-25 4:55 UTC (permalink / raw)
To: Duy Nguyen; +Cc: git
In-Reply-To: <CACsJy8Bq8aQ69twWtwHyNzez2OwTN1wHxqHwb_AM-Qo4TUuv8Q@mail.gmail.com>
Duy Nguyen <pclouds@gmail.com> writes:
> On Fri, Jan 25, 2013 at 1:22 AM, Junio C Hamano <gitster@pobox.com> wrote:
>> Duy Nguyen <pclouds@gmail.com> writes:
>>
>>> I don't think we need to support two different sets of wildcards in
>>> the long run. I'm thinking of adding ":(glob)" with WM_PATHNAME.
>>> Pathspec without :(glob) still uses the current wildcards (i.e. no
>>> FNM_PATHNAME). At some point, like 2.0, we either switch the behavior
>>> of patterns-without-:(glob) to WM_PATHNAME, or just disable wildcards
>>> when :(glob) is not present.
>>
>> Yeah, I think that is sensible.
>>
>> I am meaning to merge your retire-fnmatch topic to 'master'.
>
> I thought you wanted it to stay in 'next' longer :-)
I only said "a bit longer than other topics", and the topic has been
cooking on 'next' for three weeks by now, which is a lot longer. I
haven't been keeping exact tallies, but trivial ones graduate from
'next' to 'master' in 3 to 5 days, ones with medium complexity in 7
to 10 days on average, I think.
> That looks ok. You may want to mention that "**" syntax is enabled in
> .gitignore and .gitattributes as well (even without USE_WILDMATCH).
Yeah, I forgot about that behaviour, and it is a bit confusing
story. You did that in 237ec6e (Support "**" wildcard in .gitignore
and .gitattributes, 2012-10-15).
c41244e (wildmatch: support "no FNM_PATHNAME" mode, 2013-01-01) that
is part of the retire-fnmatch topic, changes the behaviour of
wildmatch() with respect to /**/ magic drastically, but everything
cancels out in the end:
- With the current master without nd/retire-fnmatch, wildmatch()
always works in WM_PATHNAME mode wrt '/**/'; match_pathname()
that is used for attr/ignore matching uses wildmatch() so a
pattern "**/Makefile" matches "Makefile" at the top, affected by
the "**/" magic;
- After merging nd/retire-fnmatch, wildmatch() needs WM_PATHNAME
passed in order to grok '/**/' magic, but match_pathname() is
changed in the same commit to pass WM_PATHNAME, so the "**/"
magic is retained for ignore/attr matching.
> We
> could even stop the behavior change in for-each-ref (FNM_PATHNAME in
> general) but I guess because it's a nice regression, nobody would
> complain.
That is not a "regression" (whose definition is "we inadvertently
changed the behaviour and fixed that once, but the breakage came
back"). It is a behaviour change that is backward incompatible.
I would agree that it is a nice behaviour change, though ;-)
Thanks.
^ permalink raw reply
* Re: What's cooking in git.git (Jan 2013, #08; Tue, 22)
From: Chris Rorvick @ 2013-01-25 4:55 UTC (permalink / raw)
To: John Keeping; +Cc: Junio C Hamano, git, Eric S. Raymond
In-Reply-To: <20130123211237.GR7498@serenity.lan>
On Wed, Jan 23, 2013 at 3:12 PM, John Keeping <john@keeping.me.uk> wrote:
> The existing script (git-cvsimport.perl) won't ever work with cvsps-3
> since features it relies on have been removed.
Not reporting the ancestry branch seems to be the big one. Are there
others? I had a version of the Perl script sort of working, but only
well enough to pass the t9600 and t9604 tests.
Chris
^ permalink raw reply
* Re: [PATCH] Update renamed file merge-file.h in Makefile
From: Jiang Xin @ 2013-01-25 4:59 UTC (permalink / raw)
To: Git List
In-Reply-To: <1359083188-31866-1-git-send-email-worldhello.net@gmail.com>
Oops, I find it is already fixed in commit
a60521bc6099ce89d05ef2160d2e3c30a106fda7.
commit a60521bc6099ce89d05ef2160d2e3c30a106fda7
Author: Ramsay Jones <ramsay@ramsay1.demon.co.uk>
Date: Tue Jan 22 16:47:47 2013 +0000
Makefile: Replace merge-file.h with merge-blobs.h in LIB_H
Commit fa2364ec ("Which merge_file() function do you mean?", 06-12-2012)
renamed the files merge-file.[ch] to merge-blobs.[ch], but forgot to
rename the header file in the definition of the LIB_H macro.
Signed-off-by: Ramsay Jones <ramsay@ramsay1.demon.co.uk>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
(kick back to the list)
2013/1/25 Jiang Xin <worldhello.net@gmail.com>:
> Commit v1.8.1-rc0-3-gfa2364e renamed merge-file.h to merge-blobs.h, but
> forgot to update the reference of merge-file.h in Makefile. This would
> break the build of "po/git.pot", which depends on $(LOCALIZED_C), then
> fallback to the missing file "merge-file.h".
>
> Signed-off-by: Jiang Xin <worldhello.net@gmail.com>
> ---
> Makefile | 2 +-
> 1 个文件被修改，插入 1 行(+)，删除 1 行(-)
>
> diff --git a/Makefile b/Makefile
> index 1b30d7b..a786d4c 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -649,7 +649,7 @@ LIB_H += list-objects.h
> LIB_H += ll-merge.h
> LIB_H += log-tree.h
> LIB_H += mailmap.h
> -LIB_H += merge-file.h
> +LIB_H += merge-blobs.h
> LIB_H += merge-recursive.h
> LIB_H += mergesort.h
> LIB_H += notes-cache.h
> --
> 1.8.1.1
>
^ 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