* [PATCH] git-fetch: avoid local fetching from alternate (again)
From: Shawn O. Pearce @ 2007-11-07 2:41 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
Back in e3c6f240fd9c5bdeb33f2d47adc859f37935e2df Junio taught
git-fetch to avoid copying objects when we are fetching from
a repository that is already registered as an alternate object
database. In such a case there is no reason to copy any objects
as we can already obtain them through the alternate.
However we need to ensure the objects are all reachable, so we
run `git rev-list --objects $theirs --not --all` to verify this.
If any object is missing or unreadable then we need to instead copy
the objects from the remote. When a missing object is detected
the git-rev-list process will exit with a non-zero exit status,
making this condition quite easy to detect.
Although git-fetch is currently a builtin (and so is rev-list) we
really cannot invoke the traverse_objects() API at this point in the
transport code. The object walker within traverse_objects() calls
die() as soon as it finds an object it cannot read. If that happens
we want to resume the fetch process by calling do_fetch_pack(),
instead of terminating. To get aroaund this we spawn git-rev-list
into a background process to prevent a die() from killing the
foreground fetch process.
We aren't interested in the output of rev-list (a list of SHA-1
object names that are reachable) or its errors (a "spurious" error
about an object not being found as we need to copy it) so we redirect
both stdout and stderr to /dev/null.
Signed-off-by: Shawn O. Pearce <spearce@spearce.org>
---
run-command.c | 6 ++++--
run-command.h | 1 +
transport.c | 48 +++++++++++++++++++++++++++++++++++++++++++++---
3 files changed, 50 insertions(+), 5 deletions(-)
diff --git a/run-command.c b/run-command.c
index d99a6c4..476d00c 100644
--- a/run-command.c
+++ b/run-command.c
@@ -41,7 +41,7 @@ int start_command(struct child_process *cmd)
cmd->close_out = 1;
}
- need_err = cmd->err < 0;
+ need_err = !cmd->no_stderr && cmd->err < 0;
if (need_err) {
if (pipe(fderr) < 0) {
if (need_in)
@@ -87,7 +87,9 @@ int start_command(struct child_process *cmd)
close(cmd->out);
}
- if (need_err) {
+ if (cmd->no_stderr)
+ dup_devnull(2);
+ else if (need_err) {
dup2(fderr[1], 2);
close_pair(fderr);
}
diff --git a/run-command.h b/run-command.h
index 94e1e9d..1fc781d 100644
--- a/run-command.h
+++ b/run-command.h
@@ -23,6 +23,7 @@ struct child_process {
unsigned close_out:1;
unsigned no_stdin:1;
unsigned no_stdout:1;
+ unsigned no_stderr:1;
unsigned git_cmd:1; /* if this is to be git sub-command */
unsigned stdout_to_stderr:1;
};
diff --git a/transport.c b/transport.c
index f4577b7..8505a84 100644
--- a/transport.c
+++ b/transport.c
@@ -615,17 +615,56 @@ static struct ref *get_refs_via_connect(struct transport *transport)
return refs;
}
+static int fetch_local_nocopy(struct transport *transport,
+ int nr_heads, struct ref **to_fetch)
+{
+ struct stat sb;
+ struct child_process revlist;
+ char **argv;
+ int i, j, err;
+
+ if (stat(transport->url, &sb) || !S_ISDIR(sb.st_mode))
+ return -1;
+
+ i = 0;
+ argv = xmalloc(sizeof(*argv) * (nr_heads + 5));
+ argv[i++] = xstrdup("rev-list");
+ argv[i++] = xstrdup("--objects");
+ for (j = 0; j < nr_heads; j++)
+ argv[i++] = xstrdup(sha1_to_hex(to_fetch[j]->old_sha1));
+ argv[i++] = xstrdup("--not");
+ argv[i++] = xstrdup("--all");
+ argv[i++] = NULL;
+
+ memset(&revlist, 0, sizeof(revlist));
+ revlist.argv = (const char**)argv;
+ revlist.git_cmd = 1;
+ revlist.no_stdin = 1;
+ revlist.no_stdout = 1;
+ revlist.no_stderr = 1;
+ err = start_command(&revlist);
+ if (!err)
+ err |= finish_command(&revlist);
+
+ for (i = 0; argv[i]; i++)
+ free(argv[i]);
+ free(argv);
+ return err;
+}
+
static int fetch_refs_via_pack(struct transport *transport,
int nr_heads, struct ref **to_fetch)
{
struct git_transport_data *data = transport->data;
- char **heads = xmalloc(nr_heads * sizeof(*heads));
- char **origh = xmalloc(nr_heads * sizeof(*origh));
+ char **heads, **origh;
struct ref *refs;
- char *dest = xstrdup(transport->url);
+ char *dest;
struct fetch_pack_args args;
int i;
+ if (!fetch_local_nocopy(transport, nr_heads, to_fetch))
+ return 0;
+
memset(&args, 0, sizeof(args));
args.uploadpack = data->uploadpack;
args.keep_pack = data->keep;
@@ -634,6 +673,9 @@ static int fetch_refs_via_pack(struct transport *transport,
args.verbose = transport->verbose > 0;
args.depth = data->depth;
+ heads = xmalloc(nr_heads * sizeof(*heads));
+ origh = xmalloc(nr_heads * sizeof(*origh));
+ dest = xstrdup(transport->url);
for (i = 0; i < nr_heads; i++)
origh[i] = heads[i] = xstrdup(to_fetch[i]->name);
refs = fetch_pack(&args, dest, nr_heads, heads, &transport->pack_lockfile);
--
1.5.3.5.1590.gfadfad
^ permalink raw reply related
* Re: [PATCH] Documentation: enhanced "git for CVS users" doc about shared repositories
From: Aghiles @ 2007-11-07 1:36 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: Francesco Pretto, Junio C Hamano, git
In-Reply-To: <Pine.LNX.4.64.0711070053320.4362@racer.site>
Hello,
>
> Remember, those who read "git for CVS users" are _unwilling_ to spend the
> time reading git documentation (at least for the most part). If they
> encounter something which is not useful to them, they will not just ignore
> it, they will stop reading.
>
I must disagree with this analysis (although I didn't read the content of the
patch you are commenting). People that are not really interested in git will
find many reasons to stop reading the manual anyway. Those who really
want to migrate (such as we did) are looking for every tiny bit of information.
(We googled git commands and error messages because we didn't
find what we needed in the docs)
The docs are not perfect and somewhat unequal in content but I prefer
more information than less, at this particular stage of git development.
- Aghiles.
^ permalink raw reply
* [PATCH] Disable implicit 'save' argument for 'git stash'
From: Brian Downing @ 2007-11-07 0:26 UTC (permalink / raw)
To: Junio C Hamano; +Cc: tsuna, aghilesk, git, Brian Downing
In-Reply-To: <1194395205-27905-1-git-send-email-bdowning@lavos.net>
Having 'git stash random stuff' actually stash changes is poor
user interface, due to the likelyhood of misspelling another legitimate
argument. Require an explicit 'save' command instead.
Signed-off-by: Brian Downing <bdowning@lavos.net>
---
This commit can be applied on top of the previous whenever it
is decided "enough time" has passed for the hard behavior change
of "git stash" to take place.
git-stash.sh | 16 +++++-----------
t/t3903-stash.sh | 4 ++--
2 files changed, 7 insertions(+), 13 deletions(-)
diff --git a/git-stash.sh b/git-stash.sh
index a8b854a..e900d40 100755
--- a/git-stash.sh
+++ b/git-stash.sh
@@ -219,17 +219,11 @@ create)
help | usage)
usage
;;
-*)
- if test $# -gt 0 && test "$1" = save
- then
- shift
- else
- cat >&2 <<EOF
-'git stash [message...]' is deprecated, please use
-'git stash save [message...]' instead.
-
-EOF
- fi
+save)
+ shift
save_stash "$*" && git-reset --hard
;;
+*)
+ usage
+ ;;
esac
diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
index adfac4b..4896da0 100755
--- a/t/t3903-stash.sh
+++ b/t/t3903-stash.sh
@@ -73,13 +73,13 @@ test_expect_success 'unstashing in a subdirectory' '
git stash apply
'
-test_expect_success 'stash with no args' '
+test_expect_failure 'stash with no args' '
echo 7 > file &&
test_tick &&
git stash
'
-test_expect_success 'stash with bare message' '
+test_expect_failure 'stash with bare message' '
echo 8 > file &&
test_tick &&
git stash "a message"
--
1.5.3.5.1547.gf6d81-dirty
^ permalink raw reply related
* [PATCH] Mark 'git stash [message...]' as deprecated
From: Brian Downing @ 2007-11-07 0:26 UTC (permalink / raw)
To: Junio C Hamano; +Cc: tsuna, aghilesk, git, Brian Downing
In-Reply-To: <20071106085134.GD4435@artemis.corp>
Complain to STDERR unless 'git stash save' is explicitly used.
This is in preparation for completely disabling the "default save"
behavior of the command in the future.
Signed-off-by: Brian Downing <bdowning@lavos.net>
---
Documentation/git-stash.txt | 9 ++++-----
git-stash.sh | 8 +++++++-
t/t3903-stash.sh | 14 +++++++++++++-
3 files changed, 24 insertions(+), 7 deletions(-)
diff --git a/Documentation/git-stash.txt b/Documentation/git-stash.txt
index c0147b9..61cf95d 100644
--- a/Documentation/git-stash.txt
+++ b/Documentation/git-stash.txt
@@ -9,7 +9,7 @@ SYNOPSIS
--------
[verse]
'git-stash' (list | show [<stash>] | apply [<stash>] | clear)
-'git-stash' [save] [message...]
+'git-stash' save [message...]
DESCRIPTION
-----------
@@ -39,8 +39,7 @@ OPTIONS
save::
Save your local modifications to a new 'stash', and run `git-reset
- --hard` to revert them. This is the default action when no
- subcommand is given.
+ --hard` to revert them.
list::
@@ -119,7 +118,7 @@ perform a pull, and then unstash, like this:
$ git pull
...
file foobar not up to date, cannot merge.
-$ git stash
+$ git stash save
$ git pull
$ git stash apply
----------------------------------------------------------------
@@ -147,7 +146,7 @@ You can use `git-stash` to simplify the above, like this:
+
----------------------------------------------------------------
... hack hack hack ...
-$ git stash
+$ git stash save
$ edit emergency fix
$ git commit -a -m "Fix in a hurry"
$ git stash apply
diff --git a/git-stash.sh b/git-stash.sh
index f39bd55..a8b854a 100755
--- a/git-stash.sh
+++ b/git-stash.sh
@@ -1,7 +1,7 @@
#!/bin/sh
# Copyright (c) 2007, Nanako Shiraishi
-USAGE='[ | list | show | apply | clear]'
+USAGE='[save | list | show | apply | clear]'
SUBDIRECTORY_OK=Yes
. git-sh-setup
@@ -223,6 +223,12 @@ help | usage)
if test $# -gt 0 && test "$1" = save
then
shift
+ else
+ cat >&2 <<EOF
+'git stash [message...]' is deprecated, please use
+'git stash save [message...]' instead.
+
+EOF
fi
save_stash "$*" && git-reset --hard
;;
diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
index 9a9a250..adfac4b 100755
--- a/t/t3903-stash.sh
+++ b/t/t3903-stash.sh
@@ -16,7 +16,7 @@ test_expect_success 'stash some dirty working directory' '
git add file &&
echo 3 > file &&
test_tick &&
- git stash &&
+ git stash save &&
git diff-files --quiet &&
git diff-index --cached --quiet HEAD
'
@@ -73,4 +73,16 @@ test_expect_success 'unstashing in a subdirectory' '
git stash apply
'
+test_expect_success 'stash with no args' '
+ echo 7 > file &&
+ test_tick &&
+ git stash
+'
+
+test_expect_success 'stash with bare message' '
+ echo 8 > file &&
+ test_tick &&
+ git stash "a message"
+'
+
test_done
--
1.5.3.5.1547.gf6d81-dirty
^ permalink raw reply related
* [PATCH] Reteach builtin-ls-remote to understand remotes
From: Shawn O. Pearce @ 2007-11-07 1:29 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
Prior to being made a builtin git-ls-remote understood that when
it was given a remote name we wanted it to resolve that to the
pre-configured URL and connect to that location. That changed when
it was converted to a builtin and many of my automation tools broke.
Signed-off-by: Shawn O. Pearce <spearce@spearce.org>
---
builtin-ls-remote.c | 6 +++++-
1 files changed, 5 insertions(+), 1 deletions(-)
diff --git a/builtin-ls-remote.c b/builtin-ls-remote.c
index 003580c..56f3f88 100644
--- a/builtin-ls-remote.c
+++ b/builtin-ls-remote.c
@@ -14,6 +14,7 @@ int cmd_ls_remote(int argc, const char **argv, const char *prefix)
unsigned flags = 0;
const char *uploadpack = NULL;
+ struct remote *remote;
struct transport *transport;
const struct ref *ref;
@@ -52,7 +53,10 @@ int cmd_ls_remote(int argc, const char **argv, const char *prefix)
if (!dest || i != argc - 1)
usage(ls_remote_usage);
- transport = transport_get(NULL, dest);
+ remote = nongit ? NULL : remote_get(dest);
+ if (remote && !remote->url_nr)
+ die("remote %s has no configured URL", dest);
+ transport = transport_get(remote, remote ? remote->url[0] : dest);
if (uploadpack != NULL)
transport_set_option(transport, TRANS_OPT_UPLOADPACK, uploadpack);
--
1.5.3.5.1590.gfadfad
^ permalink raw reply related
* Re: [PATCH] Documentation: enhanced "git for CVS users" doc about shared repositories
From: Francesco Pretto @ 2007-11-07 1:10 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: Junio C Hamano, git
In-Reply-To: <Pine.LNX.4.64.0711070053320.4362@racer.site>
Johannes Schindelin ha scritto:
> Remember, those who read "git for CVS users" are _unwilling_ to spend the
> time reading git documentation (at least for the most part). If they
> encounter something which is not useful to them, they will not just ignore
> it, they will stop reading.
>
That's document isn't for CVS users only. It's referred on the "Git User's Manual"
speaking about shared repositories in general. I hope you agree that the time to
make your eyes jump a little below is less than the time spent googling around
if you don't find what you are looking for.
^ permalink raw reply
* Re: [PATCH] Documentation: enhanced "git for CVS users" doc about shared repositories
From: Johannes Schindelin @ 2007-11-07 0:55 UTC (permalink / raw)
To: Francesco Pretto; +Cc: Junio C Hamano, git
In-Reply-To: <47310ACF.4030103@gmail.com>
Hi,
On Wed, 7 Nov 2007, Francesco Pretto wrote:
> I simply tried to make this document more useful and helpful for a wider
> audience of people that could ever consider of using git in their life.
> And yes, I decided to so because I had trouble myself during initial
> configurations. What's the problem if a document called "git for CVS
> users" is more explicated? What's the problem if it contains as many as
> possible informations to set up git in a viable way and, hopefully, to
> learn something on how it does work?
I refrained from commenting on that patch until now, but alas, you force
me to.
I was pretty unimpressed by the additions, as they seemed large in volume,
but small in content.
Remember, those who read "git for CVS users" are _unwilling_ to spend the
time reading git documentation (at least for the most part). If they
encounter something which is not useful to them, they will not just ignore
it, they will stop reading.
Ciao,
Dscho
^ permalink raw reply
* Re: [PATCH] Documentation: enhanced "git for CVS users" doc about shared repositories
From: Francesco Pretto @ 2007-11-07 0:46 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <7vd4unez2l.fsf@gitster.siamese.dyndns.org>
Junio C Hamano ha scritto:
>
> Honestly speaking, I am not too thrilled about making the
> cvs-migration document much longer than what it currently is.
>
Honestly speaking, you've spent too much time in looking for every possible
objections against these simple additions. At least it should be less than the
time I've spent in measuring every single word of this patch, hoping you could
consider them for inclusion. You gave me lot of attentions (I am grateful of this,
really) so I should probably be surprised of the cleanliness of git code, of the
rigor of the code style, of the clarity of the documentation. But unfortunately,
I am not. I simply tried to make this document more useful and helpful for a
wider audience of people that could ever consider of using git in their life.
And yes, I decided to so because I had trouble myself during initial configurations.
What's the problem if a document called "git for CVS users" is more explicated?
What's the problem if it contains as many as possible informations to set up
git in a viable way and, hopefully, to learn something on how it does work?
I'm sad. Not only because you refused a documentation patch, but because i could
have sent a "Bug: Documentation Sucks!" to the ml and i would have obtained the
same thing: nothing.
Francesco
P.S.:
>> +------------------------------------------------
>> +$ usermod -a -G $group $username
>> +------------------------------------------------
>
> I tend to edit /etc/group with vi ;-) and I suspect these two
> commands are specific to the distro you happen to use.
You were right with usermod (groupadd is ok): that "-a" switch is redhat syntax.
Six years of Linux Standard Base and this is still an unsolved problem...
^ permalink raw reply
* Re: [PATCH] Add Documentation/CodingStyle
From: Junio C Hamano @ 2007-11-07 0:40 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: Ralf Wildenhues, git
In-Reply-To: <Pine.LNX.4.64.0711062317330.4362@racer.site>
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> diff --git a/Documentation/CodingStyle b/Documentation/CodingStyle
> new file mode 100644
> index 0000000..622b80b
> --- /dev/null
> +++ b/Documentation/CodingStyle
> @@ -0,0 +1,87 @@
> +As a popular project, we also have some guidelines to keep to the
> +code. For git in general, two rough rules are:
> +
> + - Most importantly, we never say "It's in POSIX; we'll happily
> + screw your system that does not conform." We live in the
> + real world.
> +
> + - However, we often say "Let's stay away from that construct,
> + it's not even in POSIX".
> +
I am not sure if we want to have CodingStyle document, but the
above are not CodingStyle issues.
If we are to write this down, I'd like to have the more
important third rule, which is:
- In spite of the above two rules, we sometimes say "Although
this is not in POSIX, it (is so convenient | makes the code
much more readable | has other good characteristics) and
practically all the platforms we care about support it, so
let's use it". Again, we live in the real world, and it is
sometimes a judgement call, decided based more on real world
constraints people face than what the paper standard says.
> +For C programs:
> +
> + - Use tabs to increment, and interpret tabs as taking up to 8 spaces
What's the character for decrement? DEL? ;-)
> + Double negation is often harder to understand than no negation at
> + all.
> +
> + Some clever tricks, like using the !! operator with arithmetic
> + constructs, can be extremely confusing to others. Avoid them,
> + unless there is a compelling reason to use them.
I actually think (!!var) idiom is already established in our
codebase.
> + - Use the API. No, really. We have a strbuf (variable length string),
> + several arrays with the ALLOC_GROW() macro, a path_list for sorted
> + string lists, a hash map (mapping struct objects) named
> + "struct decorate", amongst other things.
- When you come up with an API, document it.
> + - if you are planning a new command, consider writing it in shell or
> + perl first, so that changes in semantics can be easily changed and
> + discussed. Many git commands started out like that, and a few are
> + still scripts.
No Python allowed?
^ permalink raw reply
* Re: [PATCH 3/3] pretty=format: Avoid some expensive calculations when not needed
From: Pierre Habouzit @ 2007-11-07 0:14 UTC (permalink / raw)
To: René Scharfe, Junio C Hamano, Johannes Schindelin, git
In-Reply-To: <20071107001112.GD4382@artemis.corp>
[-- Attachment #1: Type: text/plain, Size: 2070 bytes --]
On Wed, Nov 07, 2007 at 12:11:12AM +0000, Pierre Habouzit wrote:
> On Tue, Nov 06, 2007 at 11:17:14PM +0000, René Scharfe wrote:
> > I haven't seen any comments on strbuf_expand. Is it too far out?
> > Here it is again, adjusted for current master and with the changes
> > to strbuf.[ch] coming first:
>
> I have one.
>
> > strbuf.c | 22 +++++
> > strbuf.h | 3
> > pretty.c | 276 ++++++++++++++++++++++++++++++++++-----------------------------
> > 3 files changed, 178 insertions(+), 123 deletions(-)
> >
> > diff --git a/strbuf.c b/strbuf.c
> > index f4201e1..b71da99 100644
> > --- a/strbuf.c
> > +++ b/strbuf.c
> > @@ -129,6 +129,28 @@ void strbuf_addf(struct strbuf *sb, const char *fmt, ...)
> > strbuf_setlen(sb, sb->len + len);
> > }
> >
> > +void strbuf_expand(struct strbuf *sb, const char *fmt,
> > + const char **placeholders, expand_fn_t fn, void *context)
> > +{
> > + char c;
> > + const char **p;
> > +
> > + while ((c = *fmt++)) {
> > + if (c != '%') {
> > + strbuf_addch(sb, c);
> > + continue;
> > + }
>
> strbuf_addch is pretty inneficient as it puts NULs each time. rather
> do that (sketchy) :
>
> {
> for (;;) {
> const char *percent = strchr(fmt, '%');
> if (!percent)
> break;
> strbuf_add(sb, fmt, percent - fmt);
> fmt = percent + 1;
>
> /* do your stuff */
> }
> strbuf_addstr(sb, fmt);
> }
Or if we are at this level of micro-optimization:
{
const char *percent = strchrnul(fmt, '%');
while (*percent) {
strbuf_add(sb, fmt, percent - fmt);
fmt = percent + 1;
/* do your stuff */
percent = strchrnul(fmt, '%');
}
strbuf_add(sb, fmt, percent - fmt);
}
Which would require strchrnul, but it's trivial compat/ material for sure.
--
·O· Pierre Habouzit
··O madcoder@debian.org
OOO http://www.madism.org
[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]
^ permalink raw reply
* Re: [PATCH 3/3] pretty=format: Avoid some expensive calculations when not needed
From: Pierre Habouzit @ 2007-11-07 0:11 UTC (permalink / raw)
To: René Scharfe; +Cc: Junio C Hamano, Johannes Schindelin, git
In-Reply-To: <4730F5FA.3030705@lsrfire.ath.cx>
[-- Attachment #1: Type: text/plain, Size: 1697 bytes --]
On Tue, Nov 06, 2007 at 11:17:14PM +0000, René Scharfe wrote:
> I haven't seen any comments on strbuf_expand. Is it too far out?
> Here it is again, adjusted for current master and with the changes
> to strbuf.[ch] coming first:
I have one.
> strbuf.c | 22 +++++
> strbuf.h | 3
> pretty.c | 276 ++++++++++++++++++++++++++++++++++-----------------------------
> 3 files changed, 178 insertions(+), 123 deletions(-)
>
> diff --git a/strbuf.c b/strbuf.c
> index f4201e1..b71da99 100644
> --- a/strbuf.c
> +++ b/strbuf.c
> @@ -129,6 +129,28 @@ void strbuf_addf(struct strbuf *sb, const char *fmt, ...)
> strbuf_setlen(sb, sb->len + len);
> }
>
> +void strbuf_expand(struct strbuf *sb, const char *fmt,
> + const char **placeholders, expand_fn_t fn, void *context)
> +{
> + char c;
> + const char **p;
> +
> + while ((c = *fmt++)) {
> + if (c != '%') {
> + strbuf_addch(sb, c);
> + continue;
> + }
strbuf_addch is pretty inneficient as it puts NULs each time. rather
do that (sketchy) :
{
for (;;) {
const char *percent = strchr(fmt, '%');
if (!percent)
break;
strbuf_add(sb, fmt, percent - fmt);
fmt = percent + 1;
/* do your stuff */
}
strbuf_addstr(sb, fmt);
}
Of course it's a detail as formats will probably be short. But it's a good
example to show to people wanting to write new strbuf functions.
This nitpicking apart, the timings are impressive.
--
·O· Pierre Habouzit
··O madcoder@debian.org
OOO http://www.madism.org
[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]
^ permalink raw reply
* Re: [PATCH] Add Documentation/CodingStyle
From: Andreas Ericsson @ 2007-11-07 0:04 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: Junio C Hamano, Ralf Wildenhues, git
In-Reply-To: <Pine.LNX.4.64.0711062317330.4362@racer.site>
Johannes Schindelin wrote:
> Even if our code is quite a good documentation for our coding style,
> some people seem to prefer a document describing it.
>
Sweet. You just saved me 40 minutes of writing one just like that for
company use. I owe you a drink, and then you can tell me what movie
you alluded to ;-)
> +
> + - if you are planning a new command, consider writing it in shell or
> + perl first, so that changes in semantics can be easily changed and
> + discussed. Many git commands started out like that, and a few are
> + still scripts.
I'd skip this part though and just add a pointer to contrib/examples/,
saying something along the lines of
- if you're planning a new command, sneak a peak in contrib/examples/
for ample study-material of retired git commands implemented in perl
and shell.
Possibly with s/retired // on that paragraph.
There's nothing particular wrong with writing in C from the start after
all.
--
Andreas Ericsson andreas.ericsson@op5.se
OP5 AB www.op5.se
Tel: +46 8-230225 Fax: +46 8-230231
^ permalink raw reply
* Re: git-rev-list diff options broken
From: Johannes Schindelin @ 2007-11-06 23:55 UTC (permalink / raw)
To: hanwen; +Cc: git
In-Reply-To: <f329bf540711061538l4cdef27co2cb42a9938b2e325@mail.gmail.com>
Hi,
On Tue, 6 Nov 2007, Han-Wen Nienhuys wrote:
> 2007/11/6, Johannes Schindelin <Johannes.Schindelin@gmx.de>:
>
> > Probably you want to use "git log".
> >
> > "git blame -L 585,589 builtin-rev-list.c" indicates that 8c1f0b44(Fix
> > up rev-list option parsing) was responsible, which in turn indicates
> > that it was intentional.
>
> OK. So the man page needs fixing, right?
I guess. Although it seems a bit involved, since the diff-formats.txt are
not included by git-rev-list.txt itself, but by pretty-formats.txt.
Ciao,
Dscho
^ permalink raw reply
* Re: [PATCH 3/3] pretty=format: Avoid some expensive calculations when not needed
From: Johannes Schindelin @ 2007-11-06 23:45 UTC (permalink / raw)
To: René Scharfe; +Cc: Junio C Hamano, git
In-Reply-To: <4730F5FA.3030705@lsrfire.ath.cx>
[-- Attachment #1: Type: TEXT/PLAIN, Size: 1062 bytes --]
Hi,
On Wed, 7 Nov 2007, René Scharfe wrote:
> By the way, the more intrusive surgery required when using strbuf_expand()
> leads to even faster operation. Here my measurements of most of Paul's
> test cases (best of three runs):
>
> [...]
impressive timings. Although I wonder where the time comes from, as the
other substitutions should not be _that_ expensive.
In any case, your approach seems much more sensible, now that we have
strbuf.
> diff --git a/strbuf.h b/strbuf.h
> index cd7f295..95071d5 100644
> --- a/strbuf.h
> +++ b/strbuf.h
> @@ -102,6 +102,9 @@ static inline void strbuf_addbuf(struct strbuf *sb, struct strbuf *sb2) {
> strbuf_add(sb, sb2->buf, sb2->len);
> }
>
> +typedef void (*expand_fn_t) (struct strbuf *sb, const char *placeholder, void *context);
> +extern void strbuf_expand(struct strbuf *sb, const char *fmt, const char **placeholders, expand_fn_t fn, void *context);
I wonder if it would even faster (but maybe not half as readable) if
expand_fd_t got the placeholder_index instead of the placeholder.
Ciao,
Dscho
^ permalink raw reply
* [PATCH 2/2] pretty=format: Avoid some expensive calculations when not needed
From: Johannes Schindelin @ 2007-11-06 23:38 UTC (permalink / raw)
To: René Scharfe; +Cc: Junio C Hamano, git
In-Reply-To: <Pine.LNX.4.64.0711062335150.4362@racer.site>
Use the new function interp_find_active() to avoid calculating the
unique hash names, and other things, when they are not even asked for.
Unfortunately, we cannot reuse the result of that function, which
would be cleaner: there are more users than just git log. Most
notably, git-archive with "$Format:...$" substitution.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
pretty.c | 55 ++++++++++++++++++++++++++++++++++---------------------
1 files changed, 34 insertions(+), 21 deletions(-)
diff --git a/pretty.c b/pretty.c
index 490cede..590de4c 100644
--- a/pretty.c
+++ b/pretty.c
@@ -394,6 +394,8 @@ void format_commit_message(const struct commit *commit,
enum { HEADER, SUBJECT, BODY } state;
const char *msg = commit->buffer;
+ interp_find_active(format, table, ARRAY_SIZE(table));
+
if (ILEFT_RIGHT + 1 != ARRAY_SIZE(table))
die("invalid interp table!");
@@ -407,12 +409,18 @@ void format_commit_message(const struct commit *commit,
/* these depend on the commit */
if (!commit->object.parsed)
parse_object(commit->object.sha1);
- interp_set_entry(table, IHASH, sha1_to_hex(commit->object.sha1));
- interp_set_entry(table, IHASH_ABBREV,
+ if (table[IHASH].active)
+ interp_set_entry(table, IHASH,
+ sha1_to_hex(commit->object.sha1));
+ if (table[IHASH_ABBREV].active)
+ interp_set_entry(table, IHASH_ABBREV,
find_unique_abbrev(commit->object.sha1,
DEFAULT_ABBREV));
- interp_set_entry(table, ITREE, sha1_to_hex(commit->tree->object.sha1));
- interp_set_entry(table, ITREE_ABBREV,
+ if (table[ITREE].active)
+ interp_set_entry(table, ITREE,
+ sha1_to_hex(commit->tree->object.sha1));
+ if (table[ITREE_ABBREV].active)
+ interp_set_entry(table, ITREE_ABBREV,
find_unique_abbrev(commit->tree->object.sha1,
DEFAULT_ABBREV));
interp_set_entry(table, ILEFT_RIGHT,
@@ -422,22 +430,27 @@ void format_commit_message(const struct commit *commit,
? "<"
: ">");
- parents[1] = 0;
- for (i = 0, p = commit->parents;
- p && i < sizeof(parents) - 1;
- p = p->next)
- i += snprintf(parents + i, sizeof(parents) - i - 1, " %s",
- sha1_to_hex(p->item->object.sha1));
- interp_set_entry(table, IPARENTS, parents + 1);
-
- parents[1] = 0;
- for (i = 0, p = commit->parents;
- p && i < sizeof(parents) - 1;
- p = p->next)
- i += snprintf(parents + i, sizeof(parents) - i - 1, " %s",
- find_unique_abbrev(p->item->object.sha1,
- DEFAULT_ABBREV));
- interp_set_entry(table, IPARENTS_ABBREV, parents + 1);
+ if (table[IPARENTS].active) {
+ parents[1] = 0;
+ for (i = 0, p = commit->parents;
+ p && i < sizeof(parents) - 1;
+ p = p->next)
+ i += snprintf(parents + i, sizeof(parents) - i - 1,
+ " %s", sha1_to_hex(p->item->object.sha1));
+ interp_set_entry(table, IPARENTS, parents + 1);
+ }
+
+ if (table[IPARENTS_ABBREV].active) {
+ parents[1] = 0;
+ for (i = 0, p = commit->parents;
+ p && i < sizeof(parents) - 1;
+ p = p->next)
+ i += snprintf(parents + i, sizeof(parents) - i - 1,
+ " %s",
+ find_unique_abbrev(p->item->object.sha1,
+ DEFAULT_ABBREV));
+ interp_set_entry(table, IPARENTS_ABBREV, parents + 1);
+ }
for (i = 0, state = HEADER; msg[i] && state < BODY; i++) {
int eol;
@@ -464,7 +477,7 @@ void format_commit_message(const struct commit *commit,
xmemdupz(msg + i + 9, eol - i - 9);
i = eol;
}
- if (msg[i])
+ if (table[IBODY].active && msg[i])
table[IBODY].value = xstrdup(msg + i);
len = interpolate(sb->buf + sb->len, strbuf_avail(sb),
--
1.5.3.5.1597.g7191
^ permalink raw reply related
* [PATCH 1/2] interpolate.[ch]: Add a function to find which interpolations are active.
From: Johannes Schindelin @ 2007-11-06 23:38 UTC (permalink / raw)
To: René Scharfe; +Cc: Junio C Hamano, git
In-Reply-To: <Pine.LNX.4.64.0711062335150.4362@racer.site>
Some substitutions require pretty expensive operations. So it makes
sense to find out which are needed to begin with. Call
interp_find_active() to set the new "active" field in the interpolation
table.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
This is the patch that Rene proposed, but squashed into my earlier
patch.
interpolate.c | 20 ++++++++++++++++++++
interpolate.h | 3 +++
2 files changed, 23 insertions(+), 0 deletions(-)
diff --git a/interpolate.c b/interpolate.c
index 6ef53f2..bbd89bb 100644
--- a/interpolate.c
+++ b/interpolate.c
@@ -102,3 +102,23 @@ unsigned long interpolate(char *result, unsigned long reslen,
*dest = '\0';
return newlen;
}
+
+void interp_find_active(const char *orig,
+ struct interp *interps, int ninterps)
+{
+ char c;
+ int i;
+
+ for (i = 0; i < ninterps; i++)
+ interps[i].active = 0;
+
+ while ((c = *(orig++)))
+ if (c == '%')
+ /* Try to match an interpolation string. */
+ for (i = 0; i < ninterps; i++)
+ if (!prefixcmp(orig, interps[i].name + 1)) {
+ interps[i].active = 1;
+ orig += strlen(interps[i].name + 1);
+ break;
+ }
+}
diff --git a/interpolate.h b/interpolate.h
index 77407e6..d23531a 100644
--- a/interpolate.h
+++ b/interpolate.h
@@ -14,6 +14,7 @@
struct interp {
const char *name;
char *value;
+ int active:1;
};
extern void interp_set_entry(struct interp *table, int slot, const char *value);
@@ -22,5 +23,7 @@ extern void interp_clear_table(struct interp *table, int ninterps);
extern unsigned long interpolate(char *result, unsigned long reslen,
const char *orig,
const struct interp *interps, int ninterps);
+extern void interp_find_active(const char *orig,
+ struct interp *interps, int ninterps);
#endif /* INTERPOLATE_H */
--
1.5.3.5.1597.g7191
^ permalink raw reply related
* Re: git-rev-list diff options broken
From: Han-Wen Nienhuys @ 2007-11-06 23:38 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git
In-Reply-To: <Pine.LNX.4.64.0711062330220.4362@racer.site>
2007/11/6, Johannes Schindelin <Johannes.Schindelin@gmx.de>:
>
> Probably you want to use "git log".
>
> "git blame -L 585,589 builtin-rev-list.c" indicates that 8c1f0b44(Fix up
> rev-list option parsing) was responsible, which in turn indicates that it
> was intentional.
OK. So the man page needs fixing, right?
--
Han-Wen Nienhuys - hanwen@xs4all.nl - http://www.xs4all.nl/~hanwen
^ permalink raw reply
* Re: [PATCH 3/3] pretty=format: Avoid some expensive calculations when not needed
From: Johannes Schindelin @ 2007-11-06 23:36 UTC (permalink / raw)
To: René Scharfe; +Cc: Junio C Hamano, git
In-Reply-To: <4730EB4E.4080903@lsrfire.ath.cx>
[-- Attachment #1: Type: TEXT/PLAIN, Size: 482 bytes --]
Hi,
On Tue, 6 Nov 2007, René Scharfe wrote:
> -char *interp_find_active(const char *orig,
> - const struct interp *interps, int ninterps)
> +void interp_find_active(const char *orig, struct interp *interps, int ninterps)
> {
> - char *result = xcalloc(1, ninterps);
> char c;
> int i;
>
> + for (i = 0; i < ninterps; i++)
> + interps[i].active = 0;
> +
> [...]
Funny.
I have the _exact_ same change in my repository. I only forgot to send
it, it seems.
Ciao,
Dscho
^ permalink raw reply
* Re: git-rev-list diff options broken
From: Johannes Schindelin @ 2007-11-06 23:33 UTC (permalink / raw)
To: hanwen; +Cc: git
In-Reply-To: <f329bf540711061414k1627521bvaf4a7a06460cc3fd@mail.gmail.com>
Hi,
On Tue, 6 Nov 2007, Han-Wen Nienhuys wrote:
> the git-rev-list manpage says
>
>
> **
> Diff Formatting
> ~~~~~~~~~~~~~~~
>
> Below are listed options that control the formatting of diff output.
> Some of them are specific to gitlink:git-rev-list[1], however other diff
> options may be given. See gitlink:git-diff-files[1] for more options.
> **
>
> however, the source code says
>
>
> if ((!list &&
> (!(revs.tag_objects||revs.tree_objects||revs.blob_objects) &&
> !revs.pending.nr)) ||
> revs.diff)
> usage(rev_list_usage);
>
> so any attempt at showing diffs with git-rev-list will fail. What's
> the deal with this?
Probably you want to use "git log".
"git blame -L 585,589 builtin-rev-list.c" indicates that 8c1f0b44(Fix up
rev-list option parsing) was responsible, which in turn indicates that it
was intentional.
Hth,
Dscho
^ permalink raw reply
* Re: [PATCH 0/5] some shell portability fixes
From: Johannes Schindelin @ 2007-11-06 23:25 UTC (permalink / raw)
To: Mike Hommey; +Cc: Junio C Hamano, Ralf Wildenhues, git
In-Reply-To: <20071106210210.GA32159@glandium.org>
Hi,
On Tue, 6 Nov 2007, Mike Hommey wrote:
> On Tue, Nov 06, 2007 at 12:46:35PM -0800, Junio C Hamano wrote:
> > [5/5] Again, have you covered all of them? I am not opposed to
> > this one, although I am a bit curious who lacks -a/-o in
> > practice.
>
> Solaris's /bin/sh, but it already doesn't support $() and other stuff
> used all over the place in git, so it's not like it's changing anything.
>
> Maybe some other obscure old crappy shell ?
As Junio commented in the part you did not quote, there are better shells
in Solaris. Use those.
Ciao,
Dscho
^ permalink raw reply
* Re: [PATCH] Documentation: enhanced "git for CVS users" doc about shared repositories
From: Junio C Hamano @ 2007-11-06 23:25 UTC (permalink / raw)
To: Francesco Pretto; +Cc: git
In-Reply-To: <4730E056.7080809@gmail.com>
Francesco Pretto <ceztkoml@gmail.com> writes:
> Signed-off-by: Francesco Pretto <ceztkoml@gmail.com>
> ---
> More detailed instructions on how to set up shared repositories.
> Removed an old reference to the need of setting umask of ssh
> users of shared repositories.
> Added a reference to "git for CVS users" doc in git-init manual.
Honestly speaking, I am not too thrilled about making the
cvs-migration document much longer than what it currently is.
Having said that, let's take a look at each hunk.
> @@ -71,7 +71,40 @@ Setting Up a Shared Repository
> We assume you have already created a git repository for your project,
> possibly created from scratch or from a tarball (see the
> link:tutorial.html[tutorial]), or imported from an already existing CVS
> -repository (see the next section).
> +repository (see the next section). Moreover, we assume you can write in a
> +public accessible directory and give other users the permission to do so.
> +You could need or not admin privileges to do so, depending on your
> +system configuration and how you decide to export the repository.
I do not think the above helps anybody. Later sections say
"make it writable by foo group" and such specifically, and from
such descriptions, the reader either (1) understands what are
prerequisite for being able to do so, or (2) is clueless enough
to get puzzled by failure message from "chgrp git foo", and would
not even make the connection to the above text after seeing such
a failure anyway.
> +It's recommended, but not strictly necessary, to create a specific group for
> +every project/repository you'll want to create, so it will be easier to give
> +or prevent access of users to specific repositories.
I'd say this is not git specific nor cvs migrant specific advice
but falls into a common sense category. Better not clutter the
document with such, and keep it short and readable in one
sitting.
> +... With admin privilege launch:
> +
> +------------------------------------------------
> +$ groupadd $group
> +------------------------------------------------
> +
> +If you want to add an user to this group, launch:
> +
> +------------------------------------------------
> +$ usermod -a -G $group $username
> +------------------------------------------------
I tend to edit /etc/group with vi ;-) and I suspect these two
commands are specific to the distro you happen to use.
For something like "cvs migration", I really do not want a set
of step-by-step hand holding instructions. Just telling them to
"pick a group for the project, make repositories belonging to
the project owned by that group, and make them writable by the
group members" should be enough. CVS migrants may not know how
the world works with respect to git, but they are not idiots.
Introductory UNIX command guide is not the goal of the document.
Try to tell them _what_ needs to happen, not _how_, when that
level of the detail is sufficient.
> +In our example, we will store the shared repository in the /pub dir, so the
> +user creating it will need write permission there. There's no problems if you
> +choose another directory, but you'll have to ensure it will be accessible by
> +other users, on local or by remote (this could be not the case of home
> +directories).
Everything up to "by other users" is good, but ", on local or
by..." are unnecessary. If your stress is on shared
repositories, do not even mention "home", unless you are very
convinced that it is a very typical use case, in which case you
should be certain about what you recommend and there is no place
for expression like "this could be ..." for such a sure
recommendation.
> +If you just want to create a directory that is writable by every users that have
> +a local account, launch with privileged credentials:
> +
> +------------------------------------------------
> +$ mkdir /pub
> +$ chmod a+w,+t /pub
> +------------------------------------------------
Unneeded --- again, this is not a UNIX command guide --- and
wrong. You do not necessarily need to "launch with privileged
credentials" to do this anyway. As long as you can chmod the
directory, that is all that is needed.
> Next, give every team member read/write access to this repository. One
> easy way to do this is to give all the team members ssh access to the
> machine where the repository is hosted. If you don't want to give them a
> full shell on the machine, there is a restricted shell which only allows
> users to do git pushes and pulls; see gitlink:git-shell[1].
This part is a very good advice. It is git specific knowledge
new cvs migrants need to learn. Oops, the reason this part is
good is because it is from the original text --- no wonder ;-).
> -Put all the committers in the same group, and make the repository
> -writable by that group:
> +The following two commands will require admin privileges; first, enable
> +git-shell putting it on the trusted shells list of the system:
>
> ------------------------------------------------
> -$ chgrp -R $group /pub/my-repo.git
> +$ echo `which git-shell` >> /etc/shells
> +------------------------------------------------
Saying that /etc/shells may control what shells are allowed as
the login shell on many systems is probably a very good idea.
However, there is no need for an introductory UNIX guide that is
even WRONG. Why "echo `foo`" when just "foo" would work? Why
aren't you checking if /etc/shells already have git-shell
defined?
> +
> +Now, let's create the commit users:
> +
> +------------------------------------------------
> +$ useradd -g $group -s `which git-shell` $username
> ------------------------------------------------
>
> -Make sure committers have a umask of at most 027, so that the directories
> -they create are writable and searchable by other group members.
> +These users will be enabled to push on repositories owned by the group $group.
> +Later, you can give access to other projects simply by adding them to
> +other groups. Similarly, you can prevent access to repositories simply
> +removing those users from related groups.
The original text's point about umask does not apply to modern
git anymore, so the above lines can simply deleted. Almost
everything else you added to this hunk is unnecessary UNIX
guide.
> --- a/Documentation/git-init.txt
> +++ b/Documentation/git-init.txt
> @@ -101,6 +101,13 @@ $ git-add . <2>
> <2> add all existing file to the index
>
>
> +SHARED REPOSITORIES
> +-------------------
> +
> +Please refer to link:cvs-migration.html[git for CVS users], section "Setting Up
> +a Shared Repository", for details on how to set up shared repositories.
> +
> +
> Author
> ------
> Written by Linus Torvalds <torvalds@osdl.org>
This part is a good idea, but instead of putting it at the
bottom, it may make it more prominent to have this at the end of
option description for "--shared".
^ permalink raw reply
* [PATCH] Add Documentation/CodingStyle
From: Johannes Schindelin @ 2007-11-06 23:17 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Ralf Wildenhues, git
In-Reply-To: <7vtznzf5jb.fsf@gitster.siamese.dyndns.org>
Even if our code is quite a good documentation for our coding style,
some people seem to prefer a document describing it.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
I long resisted in adding this, as I really believe our code
base is a very clean one, and a good description of what we
prefer.
But it seems that not everybody has the time to study our
code in depth, beautiful as it is ;-)
BTW the first to catch the allusion to a certain movie
wins a drink with me.
Documentation/CodingStyle | 87 +++++++++++++++++++++++++++++++++++++++++++++
1 files changed, 87 insertions(+), 0 deletions(-)
create mode 100644 Documentation/CodingStyle
diff --git a/Documentation/CodingStyle b/Documentation/CodingStyle
new file mode 100644
index 0000000..622b80b
--- /dev/null
+++ b/Documentation/CodingStyle
@@ -0,0 +1,87 @@
+As a popular project, we also have some guidelines to keep to the
+code. For git in general, two rough rules are:
+
+ - Most importantly, we never say "It's in POSIX; we'll happily
+ screw your system that does not conform." We live in the
+ real world.
+
+ - However, we often say "Let's stay away from that construct,
+ it's not even in POSIX".
+
+As for more concrete guidelines, just imitate the existing code
+(this is a good guideline, no matter which project you are contributing
+to...). But if you must have some list of rules, here they are.
+
+For shell scripts specifically (not exhaustive):
+
+ - We prefer $( ... ) for command substitution; unlike ``, it
+ properly nests. It should have been the way Bourne spelled
+ it from day one, but unfortunately isn't.
+
+ - We use ${parameter-word} and its [-=?+] siblings, and their
+ colon'ed "unset or null" form.
+
+ - We use ${parameter#word} and its [#%] siblings, and their
+ doubled "longest matching" form.
+
+ - We use Arithmetic Expansion $(( ... )).
+
+ - No "Substring Expansion" ${parameter:offset:length}.
+
+ - No shell arrays.
+
+ - No strlen ${#parameter}.
+
+ - No regexp ${parameter/pattern/string}.
+
+ - We do not use Process Substitution <(list) or >(list).
+
+ - We prefer "test" over "[ ... ]".
+
+ - We do not write noiseword "function" in front of shell
+ functions.
+
+For C programs:
+
+ - Use tabs to increment, and interpret tabs as taking up to 8 spaces
+
+ - Try to keep to 80 characters per line
+
+ - When declaring pointers, the star sides with the variable name, i.e.
+ "char *string", not "char* string" or "char * string". This makes
+ it easier to understand "char *string, c;"
+
+ - Do not use curly brackets unnecessarily. I.e.
+
+ if (bla) {
+ x = 1;
+ }
+
+ is frowned upon. A gray area is when the statement extends over a
+ few lines, and/or you have a lengthy comment atop of it.
+
+ - Try to make your code understandable. You may put comments in, but
+ comments invariably tend to stale out when the code they were
+ describing changes. Often splitting a function into two makes the
+ intention of the code much clearer.
+
+ Double negation is often harder to understand than no negation at
+ all.
+
+ Some clever tricks, like using the !! operator with arithmetic
+ constructs, can be extremely confusing to others. Avoid them,
+ unless there is a compelling reason to use them.
+
+ - Use the API. No, really. We have a strbuf (variable length string),
+ several arrays with the ALLOC_GROW() macro, a path_list for sorted
+ string lists, a hash map (mapping struct objects) named
+ "struct decorate", amongst other things.
+
+ - #include system headers in git-compat-util.h. Some headers on some
+ systems show subtle breakages when you change the order, so it is
+ best to keep them in one place.
+
+ - if you are planning a new command, consider writing it in shell or
+ perl first, so that changes in semantics can be easily changed and
+ discussed. Many git commands started out like that, and a few are
+ still scripts.
--
1.5.3.5.1597.g7191
^ permalink raw reply related
* Re: [PATCH 3/3] pretty=format: Avoid some expensive calculations when not needed
From: René Scharfe @ 2007-11-06 23:17 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Johannes Schindelin, git
In-Reply-To: <4730EB4E.4080903@lsrfire.ath.cx>
By the way, the more intrusive surgery required when using strbuf_expand()
leads to even faster operation. Here my measurements of most of Paul's
test cases (best of three runs):
# master
$ time git log --pretty=oneline >/dev/null
real 0m0.390s
user 0m0.340s
sys 0m0.040s
# master
$ time git log --pretty=raw >/dev/null
real 0m0.434s
user 0m0.408s
sys 0m0.016s
# master
$ time git log --pretty="format:%H {%P} %ct" >/dev/null
real 0m1.347s
user 0m0.080s
sys 0m1.256s
# interp_find_active
$ time ./git log --pretty="format:%H {%P} %ct" >/dev/null
real 0m0.694s
user 0m0.020s
sys 0m0.672s
# strbuf_expand
$ time ./git log --pretty="format:%H {%P} %ct" >/dev/null
real 0m0.395s
user 0m0.352s
sys 0m0.028s
I haven't seen any comments on strbuf_expand. Is it too far out?
Here it is again, adjusted for current master and with the changes
to strbuf.[ch] coming first:
strbuf.c | 22 +++++
strbuf.h | 3
pretty.c | 276 ++++++++++++++++++++++++++++++++++-----------------------------
3 files changed, 178 insertions(+), 123 deletions(-)
diff --git a/strbuf.c b/strbuf.c
index f4201e1..b71da99 100644
--- a/strbuf.c
+++ b/strbuf.c
@@ -129,6 +129,28 @@ void strbuf_addf(struct strbuf *sb, const char *fmt, ...)
strbuf_setlen(sb, sb->len + len);
}
+void strbuf_expand(struct strbuf *sb, const char *fmt,
+ const char **placeholders, expand_fn_t fn, void *context)
+{
+ char c;
+ const char **p;
+
+ while ((c = *fmt++)) {
+ if (c != '%') {
+ strbuf_addch(sb, c);
+ continue;
+ }
+
+ for (p = placeholders; *p; p++) {
+ if (!prefixcmp(fmt, *p)) {
+ fn(sb, *p, context);
+ fmt += strlen(*p);
+ break;
+ }
+ }
+ }
+}
+
size_t strbuf_fread(struct strbuf *sb, size_t size, FILE *f)
{
size_t res;
diff --git a/strbuf.h b/strbuf.h
index cd7f295..95071d5 100644
--- a/strbuf.h
+++ b/strbuf.h
@@ -102,6 +102,9 @@ static inline void strbuf_addbuf(struct strbuf *sb, struct strbuf *sb2) {
strbuf_add(sb, sb2->buf, sb2->len);
}
+typedef void (*expand_fn_t) (struct strbuf *sb, const char *placeholder, void *context);
+extern void strbuf_expand(struct strbuf *sb, const char *fmt, const char **placeholders, expand_fn_t fn, void *context);
+
__attribute__((format(printf,2,3)))
extern void strbuf_addf(struct strbuf *sb, const char *fmt, ...);
diff --git a/pretty.c b/pretty.c
index 490cede..ea644bb 100644
--- a/pretty.c
+++ b/pretty.c
@@ -1,6 +1,5 @@
#include "cache.h"
#include "commit.h"
-#include "interpolate.h"
#include "utf8.h"
#include "diff.h"
#include "revision.h"
@@ -283,7 +282,8 @@ static char *logmsg_reencode(const struct commit *commit,
return out;
}
-static void fill_person(struct interp *table, const char *msg, int len)
+static void format_person_part(struct strbuf *sb, char part,
+ const char *msg, int len)
{
int start, end, tz = 0;
unsigned long date;
@@ -295,7 +295,10 @@ static void fill_person(struct interp *table, const char *msg, int len)
start = end + 1;
while (end > 0 && isspace(msg[end - 1]))
end--;
- table[0].value = xmemdupz(msg, end);
+ if (part == 'n') { /* name */
+ strbuf_add(sb, msg, end);
+ return;
+ }
if (start >= len)
return;
@@ -307,7 +310,10 @@ static void fill_person(struct interp *table, const char *msg, int len)
if (end >= len)
return;
- table[1].value = xmemdupz(msg + start, end - start);
+ if (part == 'e') { /* email */
+ strbuf_add(sb, msg + start, end - start);
+ return;
+ }
/* parse date */
for (start = end + 1; start < len && isspace(msg[start]); start++)
@@ -318,7 +324,10 @@ static void fill_person(struct interp *table, const char *msg, int len)
if (msg + start == ep)
return;
- table[5].value = xmemdupz(msg + start, ep - (msg + start));
+ if (part == 't') { /* date, UNIX timestamp */
+ strbuf_add(sb, msg + start, ep - (msg + start));
+ return;
+ }
/* parse tz */
for (start = ep - msg + 1; start < len && isspace(msg[start]); start++)
@@ -329,123 +338,107 @@ static void fill_person(struct interp *table, const char *msg, int len)
tz = -tz;
}
- interp_set_entry(table, 2, show_date(date, tz, DATE_NORMAL));
- interp_set_entry(table, 3, show_date(date, tz, DATE_RFC2822));
- interp_set_entry(table, 4, show_date(date, tz, DATE_RELATIVE));
- interp_set_entry(table, 6, show_date(date, tz, DATE_ISO8601));
+ switch (part) {
+ case 'd': /* date */
+ strbuf_addstr(sb, show_date(date, tz, DATE_NORMAL));
+ return;
+ case 'D': /* date, RFC2822 style */
+ strbuf_addstr(sb, show_date(date, tz, DATE_RFC2822));
+ return;
+ case 'r': /* date, relative */
+ strbuf_addstr(sb, show_date(date, tz, DATE_RELATIVE));
+ return;
+ case 'i': /* date, ISO 8601 */
+ strbuf_addstr(sb, show_date(date, tz, DATE_ISO8601));
+ return;
+ }
}
-void format_commit_message(const struct commit *commit,
- const void *format, struct strbuf *sb)
+static void format_commit_item(struct strbuf *sb, const char *placeholder,
+ void *context)
{
- struct interp table[] = {
- { "%H" }, /* commit hash */
- { "%h" }, /* abbreviated commit hash */
- { "%T" }, /* tree hash */
- { "%t" }, /* abbreviated tree hash */
- { "%P" }, /* parent hashes */
- { "%p" }, /* abbreviated parent hashes */
- { "%an" }, /* author name */
- { "%ae" }, /* author email */
- { "%ad" }, /* author date */
- { "%aD" }, /* author date, RFC2822 style */
- { "%ar" }, /* author date, relative */
- { "%at" }, /* author date, UNIX timestamp */
- { "%ai" }, /* author date, ISO 8601 */
- { "%cn" }, /* committer name */
- { "%ce" }, /* committer email */
- { "%cd" }, /* committer date */
- { "%cD" }, /* committer date, RFC2822 style */
- { "%cr" }, /* committer date, relative */
- { "%ct" }, /* committer date, UNIX timestamp */
- { "%ci" }, /* committer date, ISO 8601 */
- { "%e" }, /* encoding */
- { "%s" }, /* subject */
- { "%b" }, /* body */
- { "%Cred" }, /* red */
- { "%Cgreen" }, /* green */
- { "%Cblue" }, /* blue */
- { "%Creset" }, /* reset color */
- { "%n" }, /* newline */
- { "%m" }, /* left/right/bottom */
- };
- enum interp_index {
- IHASH = 0, IHASH_ABBREV,
- ITREE, ITREE_ABBREV,
- IPARENTS, IPARENTS_ABBREV,
- IAUTHOR_NAME, IAUTHOR_EMAIL,
- IAUTHOR_DATE, IAUTHOR_DATE_RFC2822, IAUTHOR_DATE_RELATIVE,
- IAUTHOR_TIMESTAMP, IAUTHOR_ISO8601,
- ICOMMITTER_NAME, ICOMMITTER_EMAIL,
- ICOMMITTER_DATE, ICOMMITTER_DATE_RFC2822,
- ICOMMITTER_DATE_RELATIVE, ICOMMITTER_TIMESTAMP,
- ICOMMITTER_ISO8601,
- IENCODING,
- ISUBJECT,
- IBODY,
- IRED, IGREEN, IBLUE, IRESET_COLOR,
- INEWLINE,
- ILEFT_RIGHT,
- };
+ const struct commit *commit = context;
struct commit_list *p;
- char parents[1024];
- unsigned long len;
int i;
enum { HEADER, SUBJECT, BODY } state;
const char *msg = commit->buffer;
- if (ILEFT_RIGHT + 1 != ARRAY_SIZE(table))
- die("invalid interp table!");
-
/* these are independent of the commit */
- interp_set_entry(table, IRED, "\033[31m");
- interp_set_entry(table, IGREEN, "\033[32m");
- interp_set_entry(table, IBLUE, "\033[34m");
- interp_set_entry(table, IRESET_COLOR, "\033[m");
- interp_set_entry(table, INEWLINE, "\n");
+ switch (placeholder[0]) {
+ case 'C':
+ switch (placeholder[3]) {
+ case 'd': /* red */
+ strbuf_addstr(sb, "\033[31m");
+ return;
+ case 'e': /* green */
+ strbuf_addstr(sb, "\033[32m");
+ return;
+ case 'u': /* blue */
+ strbuf_addstr(sb, "\033[34m");
+ return;
+ case 's': /* reset color */
+ strbuf_addstr(sb, "\033[m");
+ return;
+ }
+ case 'n': /* newline */
+ strbuf_addch(sb, '\n');
+ return;
+ }
/* these depend on the commit */
if (!commit->object.parsed)
parse_object(commit->object.sha1);
- interp_set_entry(table, IHASH, sha1_to_hex(commit->object.sha1));
- interp_set_entry(table, IHASH_ABBREV,
- find_unique_abbrev(commit->object.sha1,
- DEFAULT_ABBREV));
- interp_set_entry(table, ITREE, sha1_to_hex(commit->tree->object.sha1));
- interp_set_entry(table, ITREE_ABBREV,
- find_unique_abbrev(commit->tree->object.sha1,
- DEFAULT_ABBREV));
- interp_set_entry(table, ILEFT_RIGHT,
- (commit->object.flags & BOUNDARY)
- ? "-"
- : (commit->object.flags & SYMMETRIC_LEFT)
- ? "<"
- : ">");
-
- parents[1] = 0;
- for (i = 0, p = commit->parents;
- p && i < sizeof(parents) - 1;
- p = p->next)
- i += snprintf(parents + i, sizeof(parents) - i - 1, " %s",
- sha1_to_hex(p->item->object.sha1));
- interp_set_entry(table, IPARENTS, parents + 1);
-
- parents[1] = 0;
- for (i = 0, p = commit->parents;
- p && i < sizeof(parents) - 1;
- p = p->next)
- i += snprintf(parents + i, sizeof(parents) - i - 1, " %s",
- find_unique_abbrev(p->item->object.sha1,
- DEFAULT_ABBREV));
- interp_set_entry(table, IPARENTS_ABBREV, parents + 1);
+ switch (placeholder[0]) {
+ case 'H': /* commit hash */
+ strbuf_addstr(sb, sha1_to_hex(commit->object.sha1));
+ return;
+ case 'h': /* abbreviated commit hash */
+ strbuf_addstr(sb, find_unique_abbrev(commit->object.sha1,
+ DEFAULT_ABBREV));
+ return;
+ case 'T': /* tree hash */
+ strbuf_addstr(sb, sha1_to_hex(commit->tree->object.sha1));
+ return;
+ case 't': /* abbreviated tree hash */
+ strbuf_addstr(sb, find_unique_abbrev(commit->tree->object.sha1,
+ DEFAULT_ABBREV));
+ return;
+ case 'P': /* parent hashes */
+ for (p = commit->parents; p; p = p->next) {
+ if (p != commit->parents)
+ strbuf_addch(sb, ' ');
+ strbuf_addstr(sb, sha1_to_hex(p->item->object.sha1));
+ }
+ return;
+ case 'p': /* abbreviated parent hashes */
+ for (p = commit->parents; p; p = p->next) {
+ if (p != commit->parents)
+ strbuf_addch(sb, ' ');
+ strbuf_addstr(sb, find_unique_abbrev(
+ p->item->object.sha1, DEFAULT_ABBREV));
+ }
+ return;
+ case 'm': /* left/right/bottom */
+ strbuf_addch(sb, (commit->object.flags & BOUNDARY)
+ ? '-'
+ : (commit->object.flags & SYMMETRIC_LEFT)
+ ? '<'
+ : '>');
+ return;
+ }
+
+ /* For the rest we have to parse the commit header. */
for (i = 0, state = HEADER; msg[i] && state < BODY; i++) {
int eol;
for (eol = i; msg[eol] && msg[eol] != '\n'; eol++)
; /* do nothing */
if (state == SUBJECT) {
- table[ISUBJECT].value = xmemdupz(msg + i, eol - i);
+ if (placeholder[0] == 's') {
+ strbuf_add(sb, msg + i, eol - i);
+ return;
+ }
i = eol;
}
if (i == eol) {
@@ -453,29 +446,66 @@ void format_commit_message(const struct commit *commit,
/* strip empty lines */
while (msg[eol + 1] == '\n')
eol++;
- } else if (!prefixcmp(msg + i, "author "))
- fill_person(table + IAUTHOR_NAME,
- msg + i + 7, eol - i - 7);
- else if (!prefixcmp(msg + i, "committer "))
- fill_person(table + ICOMMITTER_NAME,
- msg + i + 10, eol - i - 10);
- else if (!prefixcmp(msg + i, "encoding "))
- table[IENCODING].value =
- xmemdupz(msg + i + 9, eol - i - 9);
+ } else if (!prefixcmp(msg + i, "author ")) {
+ if (placeholder[0] == 'a') {
+ format_person_part(sb, placeholder[1],
+ msg + i + 7, eol - i - 7);
+ return;
+ }
+ } else if (!prefixcmp(msg + i, "committer ")) {
+ if (placeholder[0] == 'c') {
+ format_person_part(sb, placeholder[1],
+ msg + i + 10, eol - i - 10);
+ return;
+ }
+ } else if (!prefixcmp(msg + i, "encoding ")) {
+ if (placeholder[0] == 'e') {
+ strbuf_add(sb, msg + i + 9, eol - i - 9);
+ return;
+ }
+ }
i = eol;
}
- if (msg[i])
- table[IBODY].value = xstrdup(msg + i);
-
- len = interpolate(sb->buf + sb->len, strbuf_avail(sb),
- format, table, ARRAY_SIZE(table));
- if (len > strbuf_avail(sb)) {
- strbuf_grow(sb, len);
- interpolate(sb->buf + sb->len, strbuf_avail(sb) + 1,
- format, table, ARRAY_SIZE(table));
- }
- strbuf_setlen(sb, sb->len + len);
- interp_clear_table(table, ARRAY_SIZE(table));
+ if (msg[i] && placeholder[0] == 'b') /* body */
+ strbuf_addstr(sb, msg + i);
+}
+
+void format_commit_message(const struct commit *commit,
+ const void *format, struct strbuf *sb)
+{
+ const char *placeholders[] = {
+ "H", /* commit hash */
+ "h", /* abbreviated commit hash */
+ "T", /* tree hash */
+ "t", /* abbreviated tree hash */
+ "P", /* parent hashes */
+ "p", /* abbreviated parent hashes */
+ "an", /* author name */
+ "ae", /* author email */
+ "ad", /* author date */
+ "aD", /* author date, RFC2822 style */
+ "ar", /* author date, relative */
+ "at", /* author date, UNIX timestamp */
+ "ai", /* author date, ISO 8601 */
+ "cn", /* committer name */
+ "ce", /* committer email */
+ "cd", /* committer date */
+ "cD", /* committer date, RFC2822 style */
+ "cr", /* committer date, relative */
+ "ct", /* committer date, UNIX timestamp */
+ "ci", /* committer date, ISO 8601 */
+ "e", /* encoding */
+ "s", /* subject */
+ "b", /* body */
+ "Cred", /* red */
+ "Cgreen", /* green */
+ "Cblue", /* blue */
+ "Creset", /* reset color */
+ "n", /* newline */
+ "m", /* left/right/bottom */
+ NULL
+ };
+ strbuf_expand(sb, format, placeholders, format_commit_item, (void *)commit);
}
static void pp_header(enum cmit_fmt fmt,
^ permalink raw reply related
* Re: git-log -p --raw broken?
From: Linus Torvalds @ 2007-11-06 23:10 UTC (permalink / raw)
To: hanwen; +Cc: git
In-Reply-To: <f329bf540711061448iab9d4a9q37e13b846dbc5ff1@mail.gmail.com>
On Tue, 6 Nov 2007, Han-Wen Nienhuys wrote:
>
> The manual page suggests that I should be able to do
>
> git log -p --raw COMMIT-RANGE
>
> however, when I try that, I get always get the same style output,
> which is the (to me: useless) human readable output.
>
> Am I missing something?
No, you're not *missing* anything. You have something *too much*.
Please remove the "-p" if you don't want a patch.
This works:
git log --raw COMMIT-RANGE
(actually, most of the time you'd want to use "-r" too when you get raw
output, but git log enables that by default)
Linus
^ permalink raw reply
* Re: git-log -p --raw broken?
From: Han-Wen Nienhuys @ 2007-11-06 23:03 UTC (permalink / raw)
To: git
In-Reply-To: <f329bf540711061448iab9d4a9q37e13b846dbc5ff1@mail.gmail.com>
2007/11/6, Han-Wen Nienhuys <hanwenn@gmail.com>:
> I'm trying to get a list of
>
> git diff-tree -t -r --raw COMMITTISH
>
> [..]
> Am I missing something?
I probably am, never mind this message.
\
--
Han-Wen Nienhuys - hanwen@xs4all.nl - http://www.xs4all.nl/~hanwen
^ 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