* Re: Improve 'git help' with basic user guide linkss
From: Junio C Hamano @ 2013-02-08 20:53 UTC (permalink / raw)
To: Philip Oakley; +Cc: Git List
In-Reply-To: <6D91D31A093D46869F43DD8D1012F0FB@PhilipOakley>
"Philip Oakley" <philipoakley@iee.org> writes:
> I'm looking at extending the 'git help' to include some information
> for the basic user who isn't ready for the extensive man page
> documentation for the various commands.
We have pointers at the beginning of "git(1)" for that exact reason.
I am not saying the documents pointed at from there are perfect, but
shouldn't that approach work?
> My real question is on the right approach to generating a list of
> guides and including them into the git help options. I'm planning on
> extending the command-list.txt file to include 'guides' and then
> extending the generate-cmdlist.sh to generate a guides array in
> common-cmds.h.
Having a catalog of guide documents in help.o sounds like a good way
to go, but I doubt "command-list" is a good place to store it. It
is about git subcommands, "git help -a" uses it to show the list of
them, and the bash completion support uses the list via "git help -a".
The common-cmds.h does not have to be the only avenue to add your
catalog of guide documents to help.o. As a part of the build
procedure, you can list Documentation/guides/ and generate an array
definition into "guides.h", and add #include "guides.h" in help.c,
for example.
> I'm thinking of adding -g --guides and -c --commands options to
> complement the existing -a --all (becomes both commands and guides)
Complement is fine. Contaminating -a with guides is probably not.
> I was expecting to update the user-manual. to become
> gituser-manual.txt so that the existing 'git help user-manual' scheme
> would discover it. The Tutorial and the User manual obviously(?) being
> the first port of call for the confused user.
Again, we do have pointer to tutorial fairly prominently at the
beginning of "git(1)". Perhaps we want index.html that redirects to
git.html or something?
^ permalink raw reply
* Re: [PATCH] git-mergetool: print filename when it contains %
From: Asheesh Laroia @ 2013-02-08 20:58 UTC (permalink / raw)
To: git
In-Reply-To: <7vhalm3e54.fsf@alter.siamese.dyndns.org>
On Fri, 8 Feb 2013, Junio C Hamano wrote:
> Asheesh Laroia <asheesh@asheesh.org> writes:
>
>> Before this change, if git-mergetool was invoked with regard to
>
> Drop "before this change,"; it is clear (and it is a recommended
> practice) you are first describing what problem you are addressing.
>
Junio, thanks for the quick reply! I agree with your suggestions, and will
take a look at addressing them, hopefully by Tuesday or so.
-- Asheesh.
^ permalink raw reply
* Re: [PATCH] git-mergetool: print filename when it contains %
From: Junio C Hamano @ 2013-02-08 21:09 UTC (permalink / raw)
To: Asheesh Laroia; +Cc: git
In-Reply-To: <alpine.DEB.2.02.1302081558130.23245@rose.makesad.us>
Asheesh Laroia <asheesh@asheesh.org> writes:
> Junio, thanks for the quick reply! I agree with your suggestions, and
> will take a look at addressing them, hopefully by Tuesday or so.
FYI, here is what I queued for now.
-- >8 --
From: Asheesh Laroia <asheesh@asheesh.org>
Date: Thu, 7 Feb 2013 17:16:24 -0800
Subject: [PATCH] git-mergetool: print filename when it contains %
If git-mergetool was invoked with files with a percent sign (%) in
their names, it would print an error. For example, if you were
calling mergetool on a file called "%2F":
printf: %2F: invalid directive
Do not pass random string to printf as if it were a valid format.
Use format string "%s" and pass the string as a data to be formatted
instead.
Signed-off-by: Asheesh Laroia <asheesh@asheesh.org>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
git-mergetool.sh | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/git-mergetool.sh b/git-mergetool.sh
index c50e18a..012afa5 100755
--- a/git-mergetool.sh
+++ b/git-mergetool.sh
@@ -440,7 +440,7 @@ then
fi
printf "Merging:\n"
-printf "$files\n"
+printf "%s\n" "$files"
IFS='
'
--
1.8.1.3.617.gb5c8e72
^ permalink raw reply related
* inotify to minimize stat() calls
From: Ramkumar Ramachandra @ 2013-02-08 21:10 UTC (permalink / raw)
To: Git List
Hi,
For large repositories, many simple git commands like `git status`
take a while to respond. I understand that this is because of large
number of stat() calls to figure out which files were changed. I
overheard that Mercurial wants to solve this problem using itnotify,
but the idea bothers me because it's not portable. Will Git ever
consider using inotify on Linux? What is the downside?
Ram
^ permalink raw reply
* Re: Improve 'git help' with basic user guide linkss
From: Philip Oakley @ 2013-02-08 21:43 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Git List
In-Reply-To: <7vr4kqzfw5.fsf@alter.siamese.dyndns.org>
From: "Junio C Hamano" <gitster@pobox.com>
Sent: Friday, February 08, 2013 8:53 PM
> "Philip Oakley" <philipoakley@iee.org> writes:
>
>> I'm looking at extending the 'git help' to include some information
>> for the basic user who isn't ready for the extensive man page
>> documentation for the various commands.
>
> We have pointers at the beginning of "git(1)" for that exact reason.
> I am not saying the documents pointed at from there are perfect, but
> shouldn't that approach work?
The problem for the new user is to find that page you need to know the
command 'git help git' which is unlikely. We know where to find it, they
don't.
My initial https://github.com/PhilipOakley/git/commit/e6217d simply
updates
- N_("See 'git help <command>' for more information on a specific
command.");
+ N_("See 'git help <command>' for more information on a specific
command.\n"
+ "Or 'git help <guide>', such as 'tutorial' for an introduction to
Git.");
as a starter for the new users.
It's then a case of providing an option to show the common and other
guides.
>
>> My real question is on the right approach to generating a list of
>> guides and including them into the git help options. I'm planning on
>> extending the command-list.txt file to include 'guides' and then
>> extending the generate-cmdlist.sh to generate a guides array in
>> common-cmds.h.
>
> Having a catalog of guide documents in help.o sounds like a good way
> to go, but I doubt "command-list" is a good place to store it. It
> is about git subcommands, "git help -a" uses it to show the list of
> them, and the bash completion support uses the list via "git help -a".
>
> The common-cmds.h does not have to be the only avenue to add your
> catalog of guide documents to help.o. As a part of the build
> procedure, you can list Documentation/guides/ and generate an array
> definition into "guides.h", and add #include "guides.h" in help.c,
> for example.
Agreed in the sense I'd been thinking of doubling up the code but making
use of the "command list" with its categories did appear to kill two
birds with one stone. It could be that the shell script could simply
generate the two .h files if appropriate.
>
>> I'm thinking of adding -g --guides and -c --commands options to
>> complement the existing -a --all (becomes both commands and guides)
>
> Complement is fine. Contaminating -a with guides is probably not.
My view is that help --all (-a) is essentially incomplete as it
currently doesn't provide all the help.
The -c option would provide just the commands for the command completion
case.
>
>> I was expecting to update the user-manual. to become
>> gituser-manual.txt so that the existing 'git help user-manual' scheme
>> would discover it. The Tutorial and the User manual obviously(?)
>> being
>> the first port of call for the confused user.
>
> Again, we do have pointer to tutorial fairly prominently at the
> beginning of "git(1)". Perhaps we want index.html that redirects to
> git.html or something?
^ permalink raw reply
* Re: inotify to minimize stat() calls
From: Junio C Hamano @ 2013-02-08 22:15 UTC (permalink / raw)
To: Ramkumar Ramachandra; +Cc: Git List
In-Reply-To: <CALkWK0=EP0Lv1F_BArub7SpL9rgFhmPtpMOCgwFqfJmVE=oa=A@mail.gmail.com>
Ramkumar Ramachandra <artagnon@gmail.com> writes:
> ... Will Git ever
> consider using inotify on Linux? What is the downside?
I think this has come up from time to time, but my understanding is
that nobody thought things through to find a good layer in the
codebase to interface to an external daemon that listens to inotify
events yet. It is not something like "somebody decreed that we
would never consider because of such and such downsides." We are
not there yet.
^ permalink raw reply
* Re: inotify to minimize stat() calls
From: Junio C Hamano @ 2013-02-08 22:45 UTC (permalink / raw)
To: Ramkumar Ramachandra; +Cc: Git List
In-Reply-To: <7vehgqzc2p.fsf@alter.siamese.dyndns.org>
Junio C Hamano <gitster@pobox.com> writes:
> Ramkumar Ramachandra <artagnon@gmail.com> writes:
>
>> ... Will Git ever
>> consider using inotify on Linux? What is the downside?
>
> I think this has come up from time to time, but my understanding is
> that nobody thought things through to find a good layer in the
> codebase to interface to an external daemon that listens to inotify
> events yet. It is not something like "somebody decreed that we
> would never consider because of such and such downsides." We are
> not there yet.
I checked read-cache.c and preload-index.c code. To get the
discussion rolling, I think something like the outline below may be
a good starting point and a feasible weekend hack for somebody
competent:
* At the beginning of preload_index(), instead of spawning the
worker thread and doing the lstat() check ourselves, we open a
socket to our daemon (see below) that watches this repository and
make a request for lstat update. The request will contain:
- The SHA1 checksum of the index file we just read (to ensure
that we and our daemon share the same baseline to
communicate); and
- the pathspec data.
Our daemon, if it already has a fresh data available, will give
us a list of <path, lstat result>. Our main process runs a loop
that is equivalent to what preload_thread() runs but uses the
lstat() data we obtained from the daemon. If our daemon says it
does not have a fresh data (or somehow our daemon is dead), we do
the work ourselves.
* Our daemon watches the index file and the working tree, and
waits for the above consumer. First it reads the index (and
remembers what it read), and whenever an inotify event comes,
does the lstat() and remembers the result. It never writes
to the index, and does not hold the index lock. Whenever the
index file changes, it needs to reload the index, and discard
lstat() data it already has for paths that are lost from the
updated index.
^ permalink raw reply
* Re: Improve 'git help' with basic user guide linkss
From: Junio C Hamano @ 2013-02-08 22:54 UTC (permalink / raw)
To: Philip Oakley; +Cc: Git List
In-Reply-To: <6BC280F5827C4098BCB6276232DDE8E4@PhilipOakley>
"Philip Oakley" <philipoakley@iee.org> writes:
> My initial https://github.com/PhilipOakley/git/commit/e6217d simply
> updates
> - N_("See 'git help <command>' for more information on a specific
> command.");
> + N_("See 'git help <command>' for more information on a specific
> command.\n"
> + "Or 'git help <guide>', such as 'tutorial' for an introduction
> to Git.");
> as a starter for the new users.
Yeah, that would be a good change to make to "git help<RETURN>"
output.
> My view is that help --all (-a) is essentially incomplete as it
> currently doesn't provide all the help.
It has always been about "tell me all subcommands", not about "give
me all the help you could give me". You are not adding a "help"
subcommand to a system you wrote last week. Changing the semantics
this late feels, eh, too late.
^ permalink raw reply
* Re: [PATCH] user-manual: Rewrite git-gc section for automatic packing
From: Junio C Hamano @ 2013-02-08 23:04 UTC (permalink / raw)
To: W. Trevor King; +Cc: Git
In-Reply-To: <20130208183546.GC3616@odin.tremily.us>
"W. Trevor King" <wking@tremily.us> writes:
> I just read through the manual cover to cover, so I have a number of
> other fixes in the pipe (from which I've already submitted the
> receive.denyCurrentBranch patch).
Wonderful.
> ... Should I bundle them all into a
> single series to reduce clutter on the list,...
I do think it makes much difference between a single series that
consists of 47 separate patches and a flood of 47 unrelated patches.
As long as it is not a single patch with 200 hunks, some of which
has to be redone repeatedly, I think it is fine either way.
Many of them may be a no-brainer to accept on the first try, while
some may have to be improved with the input from the list and will
be rerolled. I would imagine the initial round would be:
[PATCH v1 00/47] User manual updates
[PATCH v1 01/47] user-manual: update description of 'xyzzy'
[PATCH v1 02/47] user-manual: update description of 'frotz'
...
[PATCH v1 47/47] user-manual: update description of 'nitfol'
and after reviewing, some of them need to be redone in v2; the cover
letter for v2 would say something like
[PATCH v2 00/52] User manual updates
The patches 01-17, 19, 22-36, 39, 42-47 are the same as in
v1; 48-52 are new.
And people who missed the v1 review cycle may have a chance to look
at and respond to [PATCH v2 06/52] which may result in an update of
that patch to address issues that reviewers of the initial round may
have missed.
^ permalink raw reply
* Re: [PATCH] user-manual: Rewrite git-gc section for automatic packing
From: Junio C Hamano @ 2013-02-08 23:04 UTC (permalink / raw)
To: W. Trevor King; +Cc: Git
In-Reply-To: <20130208183546.GC3616@odin.tremily.us>
"W. Trevor King" <wking@tremily.us> writes:
> I just read through the manual cover to cover, so I have a number of
> other fixes in the pipe (from which I've already submitted the
> receive.denyCurrentBranch patch).
Wonderful.
> ... Should I bundle them all into a
> single series to reduce clutter on the list,...
I do not think it makes much difference between a single series that
consists of 47 separate patches and a flood of 47 unrelated patches.
As long as it is not a single patch with 200 hunks, some of which
has to be redone repeatedly, I think it is fine either way.
Hopefully, many of them may be a no-brainer to accept on the first
try, while some may have to be improved with the input from the list
and will be rerolled. I would imagine the initial round would be:
[PATCH v1 00/47] User manual updates
[PATCH v1 01/47] user-manual: update description of 'xyzzy'
[PATCH v1 02/47] user-manual: update description of 'frotz'
...
[PATCH v1 47/47] user-manual: update description of 'nitfol'
and after reviewing, some of them need to be redone in v2; the cover
letter for v2 would say something like
[PATCH v2 00/52] User manual updates
The patches 01-17, 19, 22-36, 39, 42-47 are the same as in
v1; 48-52 are new.
And people who missed the v1 review cycle may have a chance to look
at and respond to [PATCH v2 06/52] which may result in an update of
that patch to address issues that reviewers of the initial round may
have missed.
^ permalink raw reply
* Re: Improve 'git help' with basic user guide linkss
From: Philip Oakley @ 2013-02-08 23:16 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Git List
In-Reply-To: <7v6222za9x.fsf@alter.siamese.dyndns.org>
From: "Junio C Hamano" <gitster@pobox.com>
Sent: Friday, February 08, 2013 10:54 PM
> "Philip Oakley" <philipoakley@iee.org> writes:
>
>> My initial https://github.com/PhilipOakley/git/commit/e6217d simply
>> updates
>> - N_("See 'git help <command>' for more information on a specific
>> command.");
>> + N_("See 'git help <command>' for more information on a specific
>> command.\n"
>> + "Or 'git help <guide>', such as 'tutorial' for an introduction
>> to Git.");
>> as a starter for the new users.
>
> Yeah, that would be a good change to make to "git help<RETURN>"
> output.
I'll sort some patches early next week (the weekend's committed
elsewhere)
>
>> My view is that help --all (-a) is essentially incomplete as it
>> currently doesn't provide all the help.
>
> It has always been about "tell me all subcommands", not about "give
> me all the help you could give me". You are not adding a "help"
> subcommand to a system you wrote last week. Changing the semantics
> this late feels, eh, too late.
OK, I'll limit the the follow-ons to just an extra --guides option
(probably just a list of the common guides initially), and
leave --all(-a) for just the commands.
^ permalink raw reply
* segfault for git log --graph --no-walk --grep a
From: Thomas Haller @ 2013-02-08 23:52 UTC (permalink / raw)
To: Git List
[-- Attachment #1: Type: Text/Plain, Size: 2062 bytes --]
Hallo,
I just found that git crashes with a segmentation fault when calling
$ git log --graph --no-walk --grep pattern
It happens both for version 1.7.10.4 from Debian (wheezy,amd64) and
a fresh compiled git from github (git.git repository, master).
For the error to occure, the pattern must match the commit message in HEAD.
How to reproduce:
git init .
git commit -m 'text1' --allow-empty
git commit -m 'text2' --allow-empty
git log --graph --no-walk --grep 'text2'
And here is coredump I got:
Core was generated by `git log --graph --no-walk --grep text2'.
Program terminated with signal 11, Segmentation fault.
#0 __strlen_sse42 () at ../sysdeps/x86_64/multiarch/strlen-sse4.S:32
#1 0x00000000004cc13e in commit_match (opt=0x7fffbd0ee500, commit=0x216d1a8) at revision.c:2306
#2 get_commit_action (revs=0x7fffbd0ee500, commit=0x216d1a8) at revision.c:2338
#3 0x00000000004984b4 in graph_is_interesting (commit=<optimized out>, graph=<error reading variable: Unhandled dwarf expression opcode 0xfa>) at graph.c:330
#4 0x0000000000498569 in first_interesting_parent (graph=graph@entry=0x21629c0) at graph.c:369
#5 0x000000000049965e in graph_update (graph=0x21629c0, commit=<optimized out>) at graph.c:593
#6 0x00000000004cc7a9 in get_revision (revs=revs@entry=0x7fffbd0ee500) at revision.c:2580
#7 0x000000000043988a in cmd_log_walk (rev=rev@entry=0x7fffbd0ee500) at builtin/log.c:309
#8 0x000000000043a398 in cmd_log (argc=9, argv=0x2162930, prefix=0x0) at builtin/log.c:582
#9 0x0000000000405988 in run_builtin (argv=0x2162930, argc=9, p=0x751438) at git.c:281
#10 handle_internal_command (argc=9, argv=0x2162930) at git.c:443
#11 0x0000000000404df2 in run_argv (argv=0x7fffbd0eec00, argcp=0x7fffbd0eec0c) at git.c:489
#12 main (argc=9, argv=0x2162930) at git.c:564
it happens in file revision.c:2306 because "commit->buffer" is zero:
retval = grep_buffer(&opt->grep_filter,
commit->buffer, strlen(commit->buffer));
thank you all, for this awesome software.
Thomas
[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 490 bytes --]
^ permalink raw reply
* Re: segfault for git log --graph --no-walk --grep a
From: Junio C Hamano @ 2013-02-09 0:05 UTC (permalink / raw)
To: Thomas Haller; +Cc: Git List, Jeff King
In-Reply-To: <201302090052.22053.thom311@gmail.com>
Thomas Haller <thom311@gmail.com> writes:
> it happens in file revision.c:2306 because "commit->buffer" is zero:
>
> retval = grep_buffer(&opt->grep_filter,
> commit->buffer, strlen(commit->buffer));
I think this has been fixed at be5c9fb9049e (logmsg_reencode: lazily
load missing commit buffers, 2013-01-26); I haven't merged it to any
of the maintenance releases, but the tip of 'master' should have it
already.
^ permalink raw reply
* Re: segfault for git log --graph --no-walk --grep a
From: Junio C Hamano @ 2013-02-09 0:22 UTC (permalink / raw)
To: Thomas Haller; +Cc: Git List, Jeff King
In-Reply-To: <7vsj56xsg5.fsf@alter.siamese.dyndns.org>
Junio C Hamano <gitster@pobox.com> writes:
> Thomas Haller <thom311@gmail.com> writes:
>
>> it happens in file revision.c:2306 because "commit->buffer" is zero:
>>
>> retval = grep_buffer(&opt->grep_filter,
>> commit->buffer, strlen(commit->buffer));
>
> I think this has been fixed at be5c9fb9049e (logmsg_reencode: lazily
> load missing commit buffers, 2013-01-26); I haven't merged it to any
> of the maintenance releases, but the tip of 'master' should have it
> already.
Ah, no, this shares the same roots as the breakage the said commit
fixed, and the solution should use the same idea, but not fixed.
^ permalink raw reply
* Re: segfault for git log --graph --no-walk --grep a
From: Jeff King @ 2013-02-09 0:27 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Thomas Haller, Git List
In-Reply-To: <7vobfuxrns.fsf@alter.siamese.dyndns.org>
On Fri, Feb 08, 2013 at 04:22:15PM -0800, Junio C Hamano wrote:
> Junio C Hamano <gitster@pobox.com> writes:
>
> > Thomas Haller <thom311@gmail.com> writes:
> >
> >> it happens in file revision.c:2306 because "commit->buffer" is zero:
> >>
> >> retval = grep_buffer(&opt->grep_filter,
> >> commit->buffer, strlen(commit->buffer));
> >
> > I think this has been fixed at be5c9fb9049e (logmsg_reencode: lazily
> > load missing commit buffers, 2013-01-26); I haven't merged it to any
> > of the maintenance releases, but the tip of 'master' should have it
> > already.
>
> Ah, no, this shares the same roots as the breakage the said commit
> fixed, and the solution should use the same idea, but not fixed.
Yeah, I think this needs separate treatment. However, this is a perfect
example of the "case-by-case" I mention in the final two paragraphs
there.
What's the right encoding to be grepping in? The original, what we will
output in, or even something else? IOW, I wonder if this should be using
logmsg_reencode in the first place (the obvious reason not to want to do
so is speed, but logmsg_reencode is written to only have an impact in
the case that we do indeed need to reencode).
-Peff
^ permalink raw reply
* Re: segfault for git log --graph --no-walk --grep a
From: Junio C Hamano @ 2013-02-09 0:29 UTC (permalink / raw)
To: Thomas Haller; +Cc: Git List, Jeff King
In-Reply-To: <7vobfuxrns.fsf@alter.siamese.dyndns.org>
Junio C Hamano <gitster@pobox.com> writes:
> Junio C Hamano <gitster@pobox.com> writes:
>
>> Thomas Haller <thom311@gmail.com> writes:
>>
>>> it happens in file revision.c:2306 because "commit->buffer" is zero:
>>>
>>> retval = grep_buffer(&opt->grep_filter,
>>> commit->buffer, strlen(commit->buffer));
>>
>> I think this has been fixed at be5c9fb9049e (logmsg_reencode: lazily
>> load missing commit buffers, 2013-01-26); I haven't merged it to any
>> of the maintenance releases, but the tip of 'master' should have it
>> already.
>
> Ah, no, this shares the same roots as the breakage the said commit
> fixed, and the solution should use the same idea, but not fixed.
Perhaps something along this line...
-- >8 --
Subject: "log --grep": commit's buffer may already have been discarded
Following up on be5c9fb9049e (logmsg_reencode: lazily load missing
commit buffers, 2013-01-26), extract the part that reads the commit
buffer data into a separate helper function, and use it when we
apply the grep filter on the commit during the log walk.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
The reproduction recipe in original bug report looked like this:
git commit -m 'text1' --allow-empty
git commit -m 'text2' --allow-empty
git log --graph --no-walk --grep 'text2'
which does not make any sense to me. We should simply forbid
combination of --graph (which inherently wants a connected history)
and --no-walk (which is a way to tell "This is not about history,
this is about a single point").
But that is a separate issue.
commit.c | 19 +++++++++++++++++++
commit.h | 1 +
pretty.c | 14 ++------------
revision.c | 14 +++++++++++---
4 files changed, 33 insertions(+), 15 deletions(-)
diff --git a/commit.c b/commit.c
index e8eb0ae..c0acf0f 100644
--- a/commit.c
+++ b/commit.c
@@ -335,6 +335,25 @@ int parse_commit(struct commit *item)
return ret;
}
+char *read_commit_object_data(const struct commit *commit, unsigned long *sizep)
+{
+ char *msg;
+ enum object_type type;
+ unsigned long size;
+
+ if (!sizep)
+ sizep = &size;
+
+ msg = read_sha1_file(commit->object.sha1, &type, sizep);
+ if (!msg)
+ die("Cannot read commit object %s",
+ sha1_to_hex(commit->object.sha1));
+ if (type != OBJ_COMMIT)
+ die("Expected commit for '%s', got %s",
+ sha1_to_hex(commit->object.sha1), typename(type));
+ return msg;
+}
+
int find_commit_subject(const char *commit_buffer, const char **subject)
{
const char *eol;
diff --git a/commit.h b/commit.h
index 4138bb4..e314149 100644
--- a/commit.h
+++ b/commit.h
@@ -102,6 +102,7 @@ struct rev_info; /* in revision.h, it circularly uses enum cmit_fmt */
extern char *logmsg_reencode(const struct commit *commit,
const char *output_encoding);
extern void logmsg_free(char *msg, const struct commit *commit);
+extern char *read_commit_object_data(const struct commit *commit, unsigned long *size);
extern void get_commit_format(const char *arg, struct rev_info *);
extern const char *format_subject(struct strbuf *sb, const char *msg,
const char *line_separator);
diff --git a/pretty.c b/pretty.c
index eae57ad..004d16d 100644
--- a/pretty.c
+++ b/pretty.c
@@ -592,18 +592,8 @@ char *logmsg_reencode(const struct commit *commit,
char *msg = commit->buffer;
char *out;
- if (!msg) {
- enum object_type type;
- unsigned long size;
-
- msg = read_sha1_file(commit->object.sha1, &type, &size);
- if (!msg)
- die("Cannot read commit object %s",
- sha1_to_hex(commit->object.sha1));
- if (type != OBJ_COMMIT)
- die("Expected commit for '%s', got %s",
- sha1_to_hex(commit->object.sha1), typename(type));
- }
+ if (!msg)
+ msg = read_commit_object_data(commit, NULL);
if (!output_encoding || !*output_encoding)
return msg;
diff --git a/revision.c b/revision.c
index d7562ee..caf8ef3 100644
--- a/revision.c
+++ b/revision.c
@@ -2279,9 +2279,16 @@ static int commit_match(struct commit *commit, struct rev_info *opt)
strbuf_addch(&buf, '\n');
}
- /* Copy the commit to temporary if we are using "fake" headers */
- if (buf.len)
+ if (!commit->buffer) {
+ /* we may not have commit->buffer */
+ unsigned long size;
+ char *msg = read_commit_object_data(commit, &size);
+ strbuf_add(&buf, msg, size);
+ free(msg);
+ } else if (buf.len) {
+ /* Copy the commit to temporary if we are using "fake" headers */
strbuf_addstr(&buf, commit->buffer);
+ }
if (opt->grep_filter.header_list && opt->mailmap) {
if (!buf.len)
@@ -2302,9 +2309,10 @@ static int commit_match(struct commit *commit, struct rev_info *opt)
/* Find either in the commit object, or in the temporary */
if (buf.len)
retval = grep_buffer(&opt->grep_filter, buf.buf, buf.len);
- else
+ else {
retval = grep_buffer(&opt->grep_filter,
commit->buffer, strlen(commit->buffer));
+ }
strbuf_release(&buf);
return retval;
}
^ permalink raw reply related
* Re: segfault for git log --graph --no-walk --grep a
From: Junio C Hamano @ 2013-02-09 0:39 UTC (permalink / raw)
To: Jeff King; +Cc: Thomas Haller, Git List
In-Reply-To: <20130209002710.GA5570@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> On Fri, Feb 08, 2013 at 04:22:15PM -0800, Junio C Hamano wrote:
>
>> Junio C Hamano <gitster@pobox.com> writes:
>>
>> > Thomas Haller <thom311@gmail.com> writes:
>> >
>> >> it happens in file revision.c:2306 because "commit->buffer" is zero:
>> >>
>> >> retval = grep_buffer(&opt->grep_filter,
>> >> commit->buffer, strlen(commit->buffer));
>> >
>> > I think this has been fixed at be5c9fb9049e (logmsg_reencode: lazily
>> > load missing commit buffers, 2013-01-26); I haven't merged it to any
>> > of the maintenance releases, but the tip of 'master' should have it
>> > already.
>>
>> Ah, no, this shares the same roots as the breakage the said commit
>> fixed, and the solution should use the same idea, but not fixed.
>
> Yeah, I think this needs separate treatment. However, this is a perfect
> example of the "case-by-case" I mention in the final two paragraphs
> there.
>
> What's the right encoding to be grepping in? The original, what we will
> output in, or even something else? IOW, I wonder if this should be using
> logmsg_reencode in the first place (the obvious reason not to want to do
> so is speed, but logmsg_reencode is written to only have an impact in
> the case that we do indeed need to reencode).
Yeah, that actually is a good point. We should be using logmsg_reencode
so that we look for strings in the user's encoding.
^ permalink raw reply
* Re: segfault for git log --graph --no-walk --grep a
From: Jeff King @ 2013-02-09 0:39 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Thomas Haller, Git List
In-Reply-To: <7vk3qixrc8.fsf@alter.siamese.dyndns.org>
On Fri, Feb 08, 2013 at 04:29:11PM -0800, Junio C Hamano wrote:
> Perhaps something along this line...
>
> -- >8 --
> Subject: "log --grep": commit's buffer may already have been discarded
>
> Following up on be5c9fb9049e (logmsg_reencode: lazily load missing
> commit buffers, 2013-01-26), extract the part that reads the commit
> buffer data into a separate helper function, and use it when we
> apply the grep filter on the commit during the log walk.
This obviously makes sense if we don't want to get the route of
re-encoding for grep. Re-encoding would be a user-visible change, but I
wonder if it is the right thing to be doing.
> diff --git a/revision.c b/revision.c
> index d7562ee..caf8ef3 100644
> --- a/revision.c
> +++ b/revision.c
> @@ -2279,9 +2279,16 @@ static int commit_match(struct commit *commit, struct rev_info *opt)
> strbuf_addch(&buf, '\n');
> }
>
> - /* Copy the commit to temporary if we are using "fake" headers */
> - if (buf.len)
> + if (!commit->buffer) {
> + /* we may not have commit->buffer */
> + unsigned long size;
> + char *msg = read_commit_object_data(commit, &size);
> + strbuf_add(&buf, msg, size);
> + free(msg);
> + } else if (buf.len) {
> + /* Copy the commit to temporary if we are using "fake" headers */
> strbuf_addstr(&buf, commit->buffer);
> + }
Hmm. It would be nice to avoid the extra copy when we do not otherwise
need to use the strbuf. I would have expected something more like:
const char *msg = commit->buffer;
if (!msg)
msg = read_commit_object_data(commit, NULL);
[...]
if (buf.len)
retval = grep_buffer(&opt->grep_filter, buf.buf, buf.len);
else
retval = grep_buffer(&opt->grep_filter, msg, strlen(msg));
strbuf_release(&buf);
if (msg != commit->buffer)
free(msg);
return retval;
You would also need to adjust the other uses of commit->buffer
throughout the function to refer to msg.
-Peff
^ permalink raw reply
* Permission denied on home dir results in fatal error as of 1.8.1.1
From: Nick Muerdter @ 2013-02-09 0:40 UTC (permalink / raw)
To: git
As of git 1.8.1.1 and above (tested up to 1.8.1.3), if the home
directory can't be accessed, it results in a fatal error. In git 1.8.1
and below this same setup just resulted in warnings. Was this an
intentional change?
I ran into this in a situation where sudo is being used to execute a
script as root and then git ends up getting executed as a different
user. In this case HOME ends up being /root when git is called as this
different user. This home value is obviously incorrect, so there's an
issue there, but it would be perhaps be nice if this still just
resulted in warnings.
Here's a simple way to reproduce if run as a non-root user:
git 1.8.1.1 (fatal error, doesn't work):
$ env HOME=/root git ls-remote http://github.com/sstephenson/rbenv.git v0.4.0
fatal: unable to access '/root/.config/git/config': Permission denied
git 1.8.1 (warnings, but still works):
$ env HOME=/root git ls-remote http://github.com/sstephenson/rbenv.git v0.4.0
warning: unable to access '/root/.config/git/config': Permission denied
warning: unable to access '/root/.gitconfig': Permission denied
warning: unable to access '/root/.config/git/config': Permission denied
warning: unable to access '/root/.gitconfig': Permission denied
warning: unable to access '/root/.config/git/config': Permission denied
warning: unable to access '/root/.gitconfig': Permission denied
warning: unable to access '/root/.config/git/config': Permission denied
warning: unable to access '/root/.gitconfig': Permission denied
9375e99f921f428849f19efe2a2e500b3295d1a7 refs/tags/v0.4.0
Thanks,
Nick
^ permalink raw reply
* Re: segfault for git log --graph --no-walk --grep a
From: Junio C Hamano @ 2013-02-09 0:47 UTC (permalink / raw)
To: Jeff King; +Cc: Thomas Haller, Git List
In-Reply-To: <7vfw16xqvj.fsf@alter.siamese.dyndns.org>
Junio C Hamano <gitster@pobox.com> writes:
> Jeff King <peff@peff.net> writes:
>
>> On Fri, Feb 08, 2013 at 04:22:15PM -0800, Junio C Hamano wrote:
>>
>>> Junio C Hamano <gitster@pobox.com> writes:
>>>
>>> > Thomas Haller <thom311@gmail.com> writes:
>>> >
>>> >> it happens in file revision.c:2306 because "commit->buffer" is zero:
>>> >>
>>> >> retval = grep_buffer(&opt->grep_filter,
>>> >> commit->buffer, strlen(commit->buffer));
>>> >
>>> > I think this has been fixed at be5c9fb9049e (logmsg_reencode: lazily
>>> > load missing commit buffers, 2013-01-26); I haven't merged it to any
>>> > of the maintenance releases, but the tip of 'master' should have it
>>> > already.
>>>
>>> Ah, no, this shares the same roots as the breakage the said commit
>>> fixed, and the solution should use the same idea, but not fixed.
>>
>> Yeah, I think this needs separate treatment. However, this is a perfect
>> example of the "case-by-case" I mention in the final two paragraphs
>> there.
>>
>> What's the right encoding to be grepping in? The original, what we will
>> output in, or even something else? IOW, I wonder if this should be using
>> logmsg_reencode in the first place (the obvious reason not to want to do
>> so is speed, but logmsg_reencode is written to only have an impact in
>> the case that we do indeed need to reencode).
>
> Yeah, that actually is a good point. We should be using logmsg_reencode
> so that we look for strings in the user's encoding.
Perhaps like this. Just like the previous one (which should be
discarded), this makes the function always use the temporary strbuf,
so doing this upfront actually loses more code than it adds ;-)
revision.c | 29 ++++++++++++-----------------
1 file changed, 12 insertions(+), 17 deletions(-)
diff --git a/revision.c b/revision.c
index d7562ee..07bf728 100644
--- a/revision.c
+++ b/revision.c
@@ -2269,6 +2269,9 @@ static int commit_match(struct commit *commit, struct rev_info *opt)
{
int retval;
struct strbuf buf = STRBUF_INIT;
+ char *message;
+ const char *encoding;
+
if (!opt->grep_filter.pattern_list && !opt->grep_filter.header_list)
return 1;
@@ -2279,32 +2282,24 @@ static int commit_match(struct commit *commit, struct rev_info *opt)
strbuf_addch(&buf, '\n');
}
- /* Copy the commit to temporary if we are using "fake" headers */
- if (buf.len)
- strbuf_addstr(&buf, commit->buffer);
+ encoding = get_log_output_encoding();
+ message = logmsg_reencode(commit, encoding);
+ strbuf_addstr(&buf, message);
+ if (message != commit->buffer)
+ free(message);
if (opt->grep_filter.header_list && opt->mailmap) {
- if (!buf.len)
- strbuf_addstr(&buf, commit->buffer);
-
commit_rewrite_person(&buf, "\nauthor ", opt->mailmap);
commit_rewrite_person(&buf, "\ncommitter ", opt->mailmap);
}
/* Append "fake" message parts as needed */
- if (opt->show_notes) {
- if (!buf.len)
- strbuf_addstr(&buf, commit->buffer);
+ if (opt->show_notes)
format_display_notes(commit->object.sha1, &buf,
- get_log_output_encoding(), 1);
- }
+ encoding, 1);
+
+ retval = grep_buffer(&opt->grep_filter, buf.buf, buf.len);
- /* Find either in the commit object, or in the temporary */
- if (buf.len)
- retval = grep_buffer(&opt->grep_filter, buf.buf, buf.len);
- else
- retval = grep_buffer(&opt->grep_filter,
- commit->buffer, strlen(commit->buffer));
strbuf_release(&buf);
return retval;
}
^ permalink raw reply related
* Re: Permission denied on home dir results in fatal error as of 1.8.1.1
From: Junio C Hamano @ 2013-02-09 0:50 UTC (permalink / raw)
To: Nick Muerdter; +Cc: git
In-Reply-To: <CAECnihxpvtE1XejzHDCRBF=GkyBHmb53WDLa16Suiq=4SeYzvA@mail.gmail.com>
Nick Muerdter <stuff@nickm.org> writes:
> As of git 1.8.1.1 and above (tested up to 1.8.1.3), if the home
> directory can't be accessed, it results in a fatal error. In git 1.8.1
> and below this same setup just resulted in warnings. Was this an
> intentional change?
I think this was done to not just help diagnosing misconfiguration,
but to prevent an unintended misconfiguration from causing problems
(e.g. the user thinks user.name is set up correctly, but forbids Git
from reading it from the configuration files, and ends up creating
commits under wrong names).
Somebody please correct me if this weren't the case...
^ permalink raw reply
* Re: segfault for git log --graph --no-walk --grep a
From: Jeff King @ 2013-02-09 1:05 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Thomas Haller, Git List
In-Reply-To: <7va9rexqii.fsf@alter.siamese.dyndns.org>
On Fri, Feb 08, 2013 at 04:47:01PM -0800, Junio C Hamano wrote:
> > Yeah, that actually is a good point. We should be using logmsg_reencode
> > so that we look for strings in the user's encoding.
>
> Perhaps like this. Just like the previous one (which should be
> discarded), this makes the function always use the temporary strbuf,
> so doing this upfront actually loses more code than it adds ;-)
I like code simplification, but I worry a little about paying for the
extra copy in the common case. I did a best-of-five "git rev-list
--count --grep=foo HEAD" before and after your patch, though, and the
difference was well within the noise. So maybe it's not worth caring
about.
-Peff
^ permalink raw reply
* Re: Permission denied on home dir results in fatal error as of 1.8.1.1
From: Jonathan Nieder @ 2013-02-09 1:05 UTC (permalink / raw)
To: Nick Muerdter; +Cc: Junio C Hamano, git
In-Reply-To: <7v6222xqc4.fsf@alter.siamese.dyndns.org>
Junio C Hamano wrote:
> Nick Muerdter <stuff@nickm.org> writes:
>> As of git 1.8.1.1 and above (tested up to 1.8.1.3), if the home
>> directory can't be accessed, it results in a fatal error. In git 1.8.1
>> and below this same setup just resulted in warnings. Was this an
>> intentional change?
>
> I think this was done to not just help diagnosing misconfiguration,
> but to prevent an unintended misconfiguration from causing problems
> (e.g. the user thinks user.name is set up correctly, but forbids Git
> from reading it from the configuration files, and ends up creating
> commits under wrong names).
Yes, that's right. Sometimes ignoring settings has bad consequences,
so git errors out to let the user intervene and decide whether the
inaccessible settings are important.
Thanks,
Jonathan
^ permalink raw reply
* Re: segfault for git log --graph --no-walk --grep a
From: Jeff King @ 2013-02-09 1:08 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Thomas Haller, Git List
In-Reply-To: <20130209010524.GB5469@sigill.intra.peff.net>
On Fri, Feb 08, 2013 at 08:05:24PM -0500, Jeff King wrote:
> On Fri, Feb 08, 2013 at 04:47:01PM -0800, Junio C Hamano wrote:
>
> > > Yeah, that actually is a good point. We should be using logmsg_reencode
> > > so that we look for strings in the user's encoding.
> >
> > Perhaps like this. Just like the previous one (which should be
> > discarded), this makes the function always use the temporary strbuf,
> > so doing this upfront actually loses more code than it adds ;-)
>
> I like code simplification, but I worry a little about paying for the
> extra copy in the common case. I did a best-of-five "git rev-list
> --count --grep=foo HEAD" before and after your patch, though, and the
> difference was well within the noise. So maybe it's not worth caring
> about.
Oh, hold on, I'm incompetent. I measured the wrong build of git. Here
are the timings for git.git:
[before]
$ best-of-five git rev-list --count --grep=foo HEAD
Attempt 1: 0.503
Attempt 2: 0.5
Attempt 3: 0.501
Attempt 4: 0.502
Attempt 5: 0.5
real 0m0.500s
user 0m0.488s
sys 0m0.008s
[after]
$ best-of-five git rev-list --count --grep=foo HEAD
Attempt 1: 0.514
Attempt 2: 0.525
Attempt 3: 0.517
Attempt 4: 0.523
Attempt 5: 0.518
real 0m0.514s
user 0m0.480s
sys 0m0.028s
So not huge, but measurable.
-Peff
^ permalink raw reply
* Re: [PATCH] user-manual: Rewrite git-gc section for automatic packing
From: Javier Tia @ 2013-02-09 1:13 UTC (permalink / raw)
To: W. Trevor King; +Cc: Git, Junio C Hamano
In-Reply-To: <7ac63ea832711ad4bee636163e277a408cbddda4.1360341577.git.wking@tremily.us>
> +information from taking up too much space on disk or in memory. Some
> +git commands may automatically run linkgit:git-gc[1], so you don't
> +have to worry about running it manually. However, compressing large
> +repositories may take some time, so you might want to disable
> +automatic comression and run it explicitly when you are not doing
^^^^^^^^
You might want to make a little correction by 'compression'.
Regards,
--
Javier
^ 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