* git-rev-list diff options broken
From: Han-Wen Nienhuys @ 2007-11-06 22:14 UTC (permalink / raw)
To: git
Hi,
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?
--
Han-Wen Nienhuys - hanwen@xs4all.nl - http://www.xs4all.nl/~hanwen
^ permalink raw reply
* Re: git-rev-list diff options broken
From: Han-Wen Nienhuys @ 2007-11-06 22:15 UTC (permalink / raw)
To: git
In-Reply-To: <f329bf540711061414k1627521bvaf4a7a06460cc3fd@mail.gmail.com>
2007/11/6, Han-Wen Nienhuys <hanwenn@gmail.com>:
> so any attempt at showing diffs with git-rev-list will fail. What's
> the deal with this?
this is with
git version 1.5.3.5.576.gfe6193
--
Han-Wen Nienhuys - hanwen@xs4all.nl - http://www.xs4all.nl/~hanwen
^ permalink raw reply
* Re: [PATCH] git-revert is one of the most misunderstood command in git, help users out.
From: Johannes Schindelin @ 2007-11-06 22:21 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Pierre Habouzit, Steven Grimm, git
In-Reply-To: <7vir4fgnz1.fsf@gitster.siamese.dyndns.org>
Hi,
On Tue, 6 Nov 2007, Junio C Hamano wrote:
> Pierre Habouzit <madcoder@debian.org> writes:
>
> > On Tue, Nov 06, 2007 at 06:27:54PM +0000, Johannes Schindelin wrote:
> >
> >> On Tue, 6 Nov 2007, Junio C Hamano wrote:
> >>
> >> > Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> >> >
> >> > > Well, I think that _if_ we allow "git revert <path>" to mean
> >> > > "revert the changes to <path>, relative to the index" (which
> >> > > would be the same as "git checkout <path>"), then committing that
> >> > > change just does not make sense.
> >> > >
> >> > > And it is this behaviour that people are seeking, not "git revert
> >> > > <commit> <path>".
> >> >
> >> > Heh, I found this in the recent log somewhere.
> >> >
> >> > <gitte> Really, I wonder how difficult git is for people who are not
> >> > brainwashed by cvs/svn, and unfortunately enough, partly by bzr
> >> > and hg.
> >> > <gitte> From a user perspective, you might be correct. But then we
> >> > have to add 1000 commands to reflect the English language.
> >> > <gitte> Not what I want. [06:46]
> >> >
> >> > I am wondering who said it ;-).
> >>
> >> Now, that is not fair, using my own words against me ;-)
> >
> > That's very funny actually :]
>
> Yeah, it was doubly funny after I saw you posted a list of "$scm revert"
> and Dscho still sided with you in that thread.
Hey, I had my nice 5 minutes for the day, so give me a break!
;-)
Ciao,
Dscho
^ permalink raw reply
* Re: [PATCH] git-revert is one of the most misunderstood command in git, help users out.
From: Johannes Schindelin @ 2007-11-06 22:25 UTC (permalink / raw)
To: Robin Rosenberg
Cc: Mike Hommey, Junio C Hamano, Steven Grimm, Pierre Habouzit, git
In-Reply-To: <200711062221.58475.robin.rosenberg.lists@dewire.com>
Hi,
On Tue, 6 Nov 2007, Robin Rosenberg wrote:
> tisdag 06 november 2007 skrev Mike Hommey:
> > Maybe the documentation could emphasise on how to undo things when the
> > user makes mistakes.
> > Sometimes, saving your repo can be as simple as git reset --hard HEAD@{1}.
> > This is not, unfortunately, a works-for-all-cases command.
>
> Yea, git-undo(7).
In related news, I know a few users who need an un-rm-rf. Anyone?
Ciao,
Dscho
^ permalink raw reply
* Re: [PATCH 3/3] pretty=format: Avoid some expensive calculations when not needed
From: René Scharfe @ 2007-11-06 22:31 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Johannes Schindelin, git
In-Reply-To: <7vejf4kwry.fsf@gitster.siamese.dyndns.org>
Junio C Hamano schrieb:
> René Scharfe <rene.scharfe@lsrfire.ath.cx> writes:
>
>> Junio C Hamano schrieb: ...
>>> Instead of allocating a separate array and freeing at the end,
>>> wouldn't it make more sense to have a bitfield that records what
>>> is used by the format string inside the array elements?
>> How about (ab)using the value field? Let interp_find_active() mark
>> unneeded entries with NULL, and the rest with some cookie. All
>> table entries with non-NULL values need to be initialized.
>> interp_set_entry() needs to be aware of this cookie, as it mustn't
>> free() it. The cookie could be the address of a static char* in
>> interpolate.c.
>
> Sorry, where is this aversion to making the struct a bit larger
> coming from?
Not from the rational part of my brain, for sure. The following on
top of Dscho's second patch? (A char would be smaller, but a bitfield
documents the intent better.)
diff --git a/interpolate.c b/interpolate.c
index 80eeb36..1e4ccaf 100644
--- a/interpolate.c
+++ b/interpolate.c
@@ -103,22 +103,21 @@ unsigned long interpolate(char *result, unsigned long reslen,
return newlen;
}
-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;
+
while ((c = *(orig++)))
if (c == '%')
/* Try to match an interpolation string. */
for (i = 0; i < ninterps; i++)
if (!prefixcmp(orig, interps[i].name + 1)) {
- result[i] = 1;
+ interps[i].active = 1;
orig += strlen(interps[i].name + 1);
break;
}
-
- return result;
}
diff --git a/interpolate.h b/interpolate.h
index 2d197c5..a8ee6b9 100644
--- a/interpolate.h
+++ b/interpolate.h
@@ -14,6 +14,7 @@
struct interp {
const char *name;
char *value;
+ unsigned active:1;
};
extern void interp_set_entry(struct interp *table, int slot, const char *value);
@@ -22,7 +23,6 @@ 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 char *interp_find_active(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 */
^ permalink raw reply related
* Re: [PATCH] Give git-am back the ability to add Signed-off-by lines.
From: Pierre Habouzit @ 2007-11-06 22:35 UTC (permalink / raw)
To: Johannes Sixt; +Cc: Junio C Hamano, git
In-Reply-To: <200711062133.58903.johannes.sixt@telecom.at>
[-- Attachment #1: Type: text/plain, Size: 888 bytes --]
On Tue, Nov 06, 2007 at 08:33:58PM +0000, Johannes Sixt wrote:
> This was lost in the migration to git-rev-parse --parseopt by commit
> 78443d90491c1b82afdffc3d5d2ab8c1a58928b5.
>
> Signed-off-by: Johannes Sixt <johannes.sixt@telecom.at>
> ---
> git-am.sh | 2 +-
> 1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/git-am.sh b/git-am.sh
> index 876b973..e5af955 100755
> --- a/git-am.sh
> +++ b/git-am.sh
> @@ -131,7 +131,7 @@ do
> binary=t ;;
> -3|--3way)
> threeway=t ;;
> - -s--signoff)
> + -s|--signoff)
Gaaaah. One more case of t/f finger-slip
> sign=t ;;
> -u|--utf8)
> utf8=t ;; # this is now default
> --
> 1.5.3.4.315.g2ce38
>
--
·O· Pierre Habouzit
··O madcoder@debian.org
OOO http://www.madism.org
[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]
^ permalink raw reply
* git-log -p --raw broken?
From: Han-Wen Nienhuys @ 2007-11-06 22:48 UTC (permalink / raw)
To: git
Hi,
I'm trying to get a list of
git diff-tree -t -r --raw COMMITTISH
where COMMITTISH comes from git-rev-list, using a limited number of
processes. 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?
--
Han-Wen Nienhuys - hanwen@xs4all.nl - http://www.xs4all.nl/~hanwen
^ 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
* 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: [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
* [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] 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
* 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: 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 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: 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
* [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
* [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
* 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
* 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] 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: [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 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] 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] 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
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