* [PATCH] git-svn: remove --first-parent, add --upstream
From: Lars Hjemli @ 2007-09-06 16:37 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Eric Wong
In-Reply-To: <20070906075104.GA10192@hand.yhbt.net>
This makes git-svn always issue the --first-parent option to git-log when
trying to establish the "base" subversion branch, so the --first-parent
option to git-svn is no longer needed. Instead a new option, --upstream
<revspec>, is introduced. When this is specified the search for embedded
git-svn-id lines in commit messages starts at the specified revision, if
not specified the search starts at HEAD.
Signed-off-by: Lars Hjemli <hjemli@gmail.com>
---
Documentation/git-svn.txt | 10 +++++-----
git-svn.perl | 18 +++++++++---------
2 files changed, 14 insertions(+), 14 deletions(-)
diff --git a/Documentation/git-svn.txt b/Documentation/git-svn.txt
index 42d7b82..2903777 100644
--- a/Documentation/git-svn.txt
+++ b/Documentation/git-svn.txt
@@ -317,15 +317,15 @@ This is only used with the 'dcommit' command.
Print out the series of git arguments that would show
which diffs would be committed to SVN.
---first-parent::
+--upstream=<revspec>::
This is only used with the 'dcommit', 'rebase', 'log', 'find-rev' and
'show-ignore' commands.
-These commands tries to detect the upstream subversion branch by means of
-the embedded 'git-svn-id' line in commit messages. When --first-parent is
-specified, git-svn only follows the first parent of each commit, effectively
-ignoring commits brought into the current branch through merge-operations.
+These commands tries to detect the upstream subversion branch by traversing
+the first parent of each commit (starting at HEAD), looking for an embedded
+'git-svn-id' line in the commit messages. When --upstream is specified,
+git-svn starts the traversal at the specified commit instead of HEAD.
--
diff --git a/git-svn.perl b/git-svn.perl
index d21eb7f..947a944 100755
--- a/git-svn.perl
+++ b/git-svn.perl
@@ -59,7 +59,7 @@ my ($_stdin, $_help, $_edit,
$_template, $_shared,
$_version, $_fetch_all, $_no_rebase,
$_merge, $_strategy, $_dry_run, $_local,
- $_prefix, $_no_checkout, $_verbose, $_first_parent);
+ $_prefix, $_no_checkout, $_verbose, $_upstream);
$Git::SVN::_follow_parent = 1;
my %remote_opts = ( 'username=s' => \$Git::SVN::Prompt::_username,
'config-dir=s' => \$Git::SVN::Ra::config_dir,
@@ -119,14 +119,14 @@ my %cmd = (
'dry-run|n' => \$_dry_run,
'fetch-all|all' => \$_fetch_all,
'no-rebase' => \$_no_rebase,
- 'first-parent' => \$_first_parent,
+ 'upstream=s' => \$_upstream,
%cmt_opts, %fc_opts } ],
'set-tree' => [ \&cmd_set_tree,
"Set an SVN repository to a git tree-ish",
{ 'stdin|' => \$_stdin, %cmt_opts, %fc_opts, } ],
'show-ignore' => [ \&cmd_show_ignore, "Show svn:ignore listings",
{ 'revision|r=i' => \$_revision,
- 'first-parent' => \$_first_parent
+ 'upstream=s' => \$_upstream
} ],
'multi-fetch' => [ \&cmd_multi_fetch,
"Deprecated alias for $0 fetch --all",
@@ -148,11 +148,11 @@ my %cmd = (
'authors-file|A=s' => \$_authors,
'color' => \$Git::SVN::Log::color,
'pager=s' => \$Git::SVN::Log::pager,
- 'first-parent' => \$_first_parent
+ 'upstream=s' => \$_upstream
} ],
'find-rev' => [ \&cmd_find_rev, "Translate between SVN revision numbers and tree-ish",
{
- 'first-parent' => \$_first_parent
+ 'upstream=s' => \$_upstream
} ],
'rebase' => [ \&cmd_rebase, "Fetch and rebase your working directory",
{ 'merge|m|M' => \$_merge,
@@ -160,7 +160,7 @@ my %cmd = (
'strategy|s=s' => \$_strategy,
'local|l' => \$_local,
'fetch-all|all' => \$_fetch_all,
- 'first-parent' => \$_first_parent,
+ 'upstream=s' => \$_upstream,
%fc_opts } ],
'commit-diff' => [ \&cmd_commit_diff,
'Commit a diff between two trees',
@@ -818,9 +818,9 @@ sub cmt_metadata {
sub working_head_info {
my ($head, $refs) = @_;
- my @args = ('log', '--no-color');
- push @args, '--first-parent' if $_first_parent;
- my ($fh, $ctx) = command_output_pipe(@args, $head);
+ my @args = ('log', '--no-color', '--first-parent');
+ push @args, ($_upstream ? $_upstream : $head);
+ my ($fh, $ctx) = command_output_pipe(@args);
my $hash;
my %max;
while (<$fh>) {
--
1.5.3.1.g0e33-dirty
^ permalink raw reply related
* Re: [PATCH 7/9] Add strbuf_read_path().
From: Johannes Schindelin @ 2007-09-06 16:40 UTC (permalink / raw)
To: Kristian Høgsberg; +Cc: git
In-Reply-To: <11890382262161-git-send-email-krh@redhat.com>
[-- Attachment #1: Type: TEXT/PLAIN, Size: 288 bytes --]
Hi,
On Wed, 5 Sep 2007, Kristian Høgsberg wrote:
> +extern int strbuf_read_path(struct strbuf *sb, const char *path);
May I suggest renaming this to "strbuf_read_file()"? I kind of expected a
function which reads in the absolute path of a file, judging by the
name...
Ciao,
Dscho
^ permalink raw reply
* [PATCH 5/3] archive: rename attribute specfile to export-subst
From: René Scharfe @ 2007-09-06 16:51 UTC (permalink / raw)
To: Junio C Hamano
Cc: Johannes Schindelin, Andreas Ericsson, Git Mailing List,
Michael Gernoth, Thomas Glanzmann
In-Reply-To: <7vzm02klip.fsf@gitster.siamese.dyndns.org>
Junio C Hamano schrieb:
> René Scharfe <rene.scharfe@lsrfire.ath.cx> writes:
>
>>> Maybe we should not so much name it by purpose, but by function. How
>>> about "substformat" for the attribute name, and replacing any
>>> $Format:blablub$ inside those files with something a la
>>> --pretty=format:blablub?
>> I like the $Format:...$ notation. How about naming the attribute
>> "template", as that's what a thus marked file is?
>
> Sounds good, although I suspect "template" might confuse newbies
> that checkout may apply the substitution as well. How about
> something with "export" in it? export-subst, perhaps?
Well, including "export" in the name makes sense, yes. I can't come up
with a better name, let's take this.
--- snip! ---
As suggested by Junio and Johannes, change the name of the former
attribute specfile to export-subst to indicate its function rather
than purpose and to make clear that it is not applied to working tree
files.
Signed-off-by: Rene Scharfe <rene.scharfe@lsrfire.ath.cx>
---
Documentation/gitattributes.txt | 6 +++---
builtin-archive.c | 14 +++++++-------
t/t5000-tar-tree.sh | 18 +++++++++---------
3 files changed, 19 insertions(+), 19 deletions(-)
diff --git a/Documentation/gitattributes.txt b/Documentation/gitattributes.txt
index 37b3be8..d0e951e 100644
--- a/Documentation/gitattributes.txt
+++ b/Documentation/gitattributes.txt
@@ -424,10 +424,10 @@ frotz unspecified
Creating an archive
~~~~~~~~~~~~~~~~~~~
-`specfile`
-^^^^^^^^^^
+`export-subst`
+^^^^^^^^^^^^^^
-If the attribute `specfile` is set for a file then git will expand
+If the attribute `export-subst` is set for a file then git will expand
several placeholders when adding this file to an archive. The
expansion depends on the availability of a commit ID, i.e. if
gitlink:git-archive[1] has been given a tree instead of a commit or a
diff --git a/builtin-archive.c b/builtin-archive.c
index a8a0f01..af14837 100644
--- a/builtin-archive.c
+++ b/builtin-archive.c
@@ -81,8 +81,8 @@ static int run_remote_archiver(const char *remote, int argc,
return !!rv;
}
-static void *format_specfile(const struct commit *commit, const char *format,
- unsigned long *sizep)
+static void *format_subst(const struct commit *commit, const char *format,
+ unsigned long *sizep)
{
unsigned long len = *sizep, result_len = 0;
const char *a = format;
@@ -131,22 +131,22 @@ static void *convert_to_archive(const char *path,
const void *src, unsigned long *sizep,
const struct commit *commit)
{
- static struct git_attr *attr_specfile;
+ static struct git_attr *attr_export_subst;
struct git_attr_check check[1];
if (!commit)
return NULL;
- if (!attr_specfile)
- attr_specfile = git_attr("specfile", 8);
+ if (!attr_export_subst)
+ attr_export_subst = git_attr("export-subst", 12);
- check[0].attr = attr_specfile;
+ check[0].attr = attr_export_subst;
if (git_checkattr(path, ARRAY_SIZE(check), check))
return NULL;
if (!ATTR_TRUE(check[0].value))
return NULL;
- return format_specfile(commit, src, sizep);
+ return format_subst(commit, src, sizep);
}
void *sha1_file_to_archive(const char *path, const unsigned char *sha1,
diff --git a/t/t5000-tar-tree.sh b/t/t5000-tar-tree.sh
index 6e89e07..42e28ab 100755
--- a/t/t5000-tar-tree.sh
+++ b/t/t5000-tar-tree.sh
@@ -28,7 +28,7 @@ commit id embedding:
TAR=${TAR:-tar}
UNZIP=${UNZIP:-unzip}
-SPECFILEFORMAT=%H%n
+SUBSTFORMAT=%H%n
test_expect_success \
'populate workdir' \
@@ -36,7 +36,7 @@ test_expect_success \
echo simple textfile >a/a &&
mkdir a/bin &&
cp /bin/sh a/bin &&
- printf "A\$Format:%s\$O" "$SPECFILEFORMAT" >a/specfile &&
+ printf "A\$Format:%s\$O" "$SUBSTFORMAT" >a/substfile &&
ln -s a a/l1 &&
(p=long_path_to_a_file && cd a &&
for depth in 1 2 3 4 5; do mkdir $p && cd $p; done &&
@@ -108,20 +108,20 @@ test_expect_success \
'diff -r a c/prefix/a'
test_expect_success \
- 'create an archive with a specfile' \
- 'echo specfile specfile >a/.gitattributes &&
+ 'create an archive with a substfile' \
+ 'echo substfile export-subst >a/.gitattributes &&
git archive HEAD >f.tar &&
rm a/.gitattributes'
test_expect_success \
- 'extract specfile' \
+ 'extract substfile' \
'(mkdir f && cd f && $TAR xf -) <f.tar'
test_expect_success \
- 'validate specfile contents' \
- 'git log --max-count=1 "--pretty=format:A${SPECFILEFORMAT}O" HEAD \
- >f/a/specfile.expected &&
- diff f/a/specfile.expected f/a/specfile'
+ 'validate substfile contents' \
+ 'git log --max-count=1 "--pretty=format:A${SUBSTFORMAT}O" HEAD \
+ >f/a/substfile.expected &&
+ diff f/a/substfile.expected f/a/substfile'
test_expect_success \
'git archive --format=zip' \
^ permalink raw reply related
* Re: [PATCH 9/9] Implement git commit as a builtin command.
From: Johannes Schindelin @ 2007-09-06 16:59 UTC (permalink / raw)
To: Kristian Høgsberg; +Cc: git
In-Reply-To: <11890382271931-git-send-email-krh@redhat.com>
[-- Attachment #1: Type: TEXT/PLAIN, Size: 3976 bytes --]
Hi,
On Wed, 5 Sep 2007, Kristian Høgsberg wrote:
> contrib/examples/git-commit.sh | 665 +++++++++++++++++++++++++++++++++++
> git-commit.sh | 665 -----------------------------------
You might want to use "git format-patch -M" next time ;-)
> @@ -357,7 +358,6 @@ BUILTIN_OBJS = \
> builtin-rev-parse.o \
> builtin-revert.o \
> builtin-rm.o \
> - builtin-runstatus.o \
Better keep it; some people's scripts could depend on it.
> +struct option {
> + enum option_type type;
> + const char *long_name;
> + char short_name;
> + void *value;
> +};
> +
> +static int scan_options(const char ***argv, struct option *options)
> +{
I would not (no longer, anyway) be opposed to replacing the option parsing
in git with getopt(); I hear that it is small enough to keep a copy in
compat/getopt.c.
But let's go forward with builtin-commit; getopt() can come later.
> +static char *
> +prepare_index(const char **files, const char *prefix)
> +{
> + int fd;
> + struct tree *tree;
> + struct lock_file *next_index_lock;
> +
> + fd = hold_locked_index(&lock_file, 1);
> + if (read_cache() < 0)
> + die("index file corrupt");
> +
> + if (all) {
> + add_files_to_cache(fd, files, NULL);
> + return lock_file.filename;
> + } else if (also) {
> + add_files_to_cache(fd, files, prefix);
> + return lock_file.filename;
> + }
> +
> + if (interactive)
> + interactive_add();
> +
> + if (*files == NULL) {
> + /* Commit index as-is. */
> + rollback_lock_file(&lock_file);
> + return get_index_file();
> + }
> +
> + /*
> + * FIXME: Warn on unknown files. Shell script does
> + *
> + * commit_only=`git-ls-files --error-unmatch -- "$@"`
> + */
> +
> + /*
> + * FIXME: shell script does
> + *
> + * git-read-tree --index-output="$TMP_INDEX" -i -m HEAD
> + *
> + * which warns about unmerged files in the index.
> + */
> +
> + /* update the user index file */
> + add_files_to_cache(fd, files, prefix);
I suspect this, or ...
> +
> + if (!initial_commit) {
> + tree = parse_tree_indirect(head_sha1);
> + if (!tree)
> + die("failed to unpack HEAD tree object");
> + if (read_tree(tree, 0, NULL))
> + die("failed to read HEAD tree object");
> + }
> +
> + /* Uh oh, abusing lock_file to create a garbage collected file */
> + next_index_lock = xmalloc(sizeof(*next_index_lock));
> + fd = hold_lock_file_for_update(next_index_lock,
> + git_path("next-index-%d", getpid()), 1);
> + add_files_to_cache(fd, files, prefix);
... this, but not both.
> +/* Find out if the message starting at position 'start' in the strbuf
> + * contains only whitespace and Signed-off-by lines. */
> +static int message_is_empty(struct strbuf *sb, int start)
> +{
> + static const char signed_off_by[] = "Signed-off-by: ";
I think you already defined that globally earlier.
In the function message_is_empty() you write:
> + /* See if the template is just a prefix of the message. */
> + strbuf_init(&tmpl);
> + if (template_file && strbuf_read_path(&tmpl, template_file) > 0) {
> + stripspace(&tmpl, 1);
> + if (start + tmpl.len <= sb->len &&
> + memcmp(tmpl.buf, sb->buf + start, tmpl.len) == 0)
> + start += tmpl.len;
Could we not bail out here, if there is no match? In that case, the
message is clearly not empty...
> + /* Check if the rest is just whitespace and Signed-of-by's. */
> + for (i = start; i < sb->len; i++) {
> + nl = memchr(sb->buf + i, '\n', sb->len - i);
> + if (nl)
> + eol = nl - sb->buf;
> + else
> + eol = sb->len;
Why not just "if (isspace(sb->buf[i]) || sb->buf[i] == '\n') continue;"?
This would also catch the cases where people indent their S-O-Bs.
> +
> + if (strlen(signed_off_by) <= eol - i &&
> + !prefixcmp(sb->buf + i, signed_off_by)) {
> + i = eol;
> + continue;
> + }
> + while (i < eol)
> + if (!isspace(sb->buf[i++]))
> + return 0;
> + }
> +
> + return 1;
> +}
I did not review the rest of the code closely yet...
All in all: well done!
Ciao,
Dscho
^ permalink raw reply
* Re: [PATCH 4/3] archive: specfile syntax change: "$Format:%PLCHLDR$" instead of just "%PLCHLDR"
From: Johannes Schindelin @ 2007-09-06 17:11 UTC (permalink / raw)
To: René Scharfe
Cc: Junio C Hamano, Andreas Ericsson, Git Mailing List,
Michael Gernoth, Thomas Glanzmann
In-Reply-To: <46E028B9.2090908@lsrfire.ath.cx>
[-- Attachment #1: Type: TEXT/PLAIN, Size: 1351 bytes --]
Hi,
On Thu, 6 Sep 2007, René Scharfe wrote:
> As suggested by Johannes, --pretty=format: placeholders in specfiles
> need to be wrapped in $Format:...$ now.
Thanks.
> diff --git a/builtin-archive.c b/builtin-archive.c
> index faccce3..a8a0f01 100644
> --- a/builtin-archive.c
> +++ b/builtin-archive.c
> @@ -81,14 +81,58 @@ static int run_remote_archiver(const char *remote, int argc,
> return !!rv;
> }
>
> +static void *format_specfile(const struct commit *commit, const char *format,
> + unsigned long *sizep)
Should this not be "char *buffer" instead of "const char *format"? Or
even better: a "struct strbuf *"?
> +{
> + unsigned long len = *sizep, result_len = 0;
> + const char *a = format;
> + char *result = NULL;
> +
> + for (;;) {
> + const char *b, *c;
> + char *fmt, *formatted = NULL;
> + unsigned long a_len, fmt_len, formatted_len, allocated = 0;
Maybe initialise formatted_len, just to be on the safe side?
> +
> + b = memchr(a, '$', len);
> + if (!b || a + len < b + 9 || memcmp(b + 1, "Format:", 7))
> + break;
Wouldn't memmem(buffer, len, "$Format:", 8) be better here?
A general comment: since you plan to output the result into a file anyway,
it should be even easier to avoid realloc(), and do a
print_formatted_specfile() instead of a format_specfile(), no?
Ciao,
Dscho
^ permalink raw reply
* Re: [PATCH 5/3] archive: rename attribute specfile to export-subst
From: Johannes Schindelin @ 2007-09-06 17:13 UTC (permalink / raw)
To: René Scharfe
Cc: Junio C Hamano, Andreas Ericsson, Git Mailing List,
Michael Gernoth, Thomas Glanzmann
In-Reply-To: <46E02FFF.8090902@lsrfire.ath.cx>
[-- Attachment #1: Type: TEXT/PLAIN, Size: 572 bytes --]
Hi,
On Thu, 6 Sep 2007, René Scharfe wrote:
> As suggested by Junio and Johannes, change the name of the former
> attribute specfile to export-subst to indicate its function rather
> than purpose and to make clear that it is not applied to working tree
> files.
>
> Signed-off-by: Rene Scharfe <rene.scharfe@lsrfire.ath.cx>
ACK!
(Even if I did not really suggest "export-subst", which I like very
much...)
The bigger question is now if these two patches should be folded back into
your original patch series, or stand alone as commits of their own...
Ciao,
Dscho
^ permalink raw reply
* Re: strbuf new API, take 2 for inclusion
From: Pierre Habouzit @ 2007-09-06 17:15 UTC (permalink / raw)
To: Jeff King; +Cc: git
In-Reply-To: <20070906125811.GA32400@coredump.intra.peff.net>
[-- Attachment #1: Type: text/plain, Size: 1214 bytes --]
On Thu, Sep 06, 2007 at 12:58:11PM +0000, Jeff King wrote:
> On Thu, Sep 06, 2007 at 01:20:04PM +0200, Pierre Habouzit wrote:
>
> > I've also stripped as many STRBUF_INIT uses as possible, some people
> > didn't liked it. I've kept its use for "static" strbufs where it's way
> > more convenient that a function call.
>
> The STRBUF_INIT initializer just sets everything to '0' or NULL. Static
> objects already have this done automagically by the compiler, so there's
> no need to use STRBUF_INIT at all there.
Yes, Junio already did that remark. The reason is that it's forward
compatible: if we ever change strbuf's intitial value for some reason,
we would just have to rebuild the code. As junio disliked it (and I'm
not sure I love it either) I've used it where using the _init() function
was impractical.
And yes { 0 } would have worked the same as per C standard. It's just
that I setup my compiler to flag missing C89 initializers (because it
often detects real errors).
Here are the whys'
--
·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: strbuf new API, take 2 for inclusion
From: Jeff King @ 2007-09-06 17:16 UTC (permalink / raw)
To: git
In-Reply-To: <20070906171502.GF8451@artemis.corp>
On Thu, Sep 06, 2007 at 07:15:02PM +0200, Pierre Habouzit wrote:
> Yes, Junio already did that remark. The reason is that it's forward
> compatible: if we ever change strbuf's intitial value for some reason,
> we would just have to rebuild the code. As junio disliked it (and I'm
> not sure I love it either) I've used it where using the _init() function
> was impractical.
OK, I missed that discussion. Thanks for the explanation.
-Peff
^ permalink raw reply
* Re: [PATCH 6/7] Eradicate yet-another-buffer implementation in buitin-rerere.c
From: Pierre Habouzit @ 2007-09-06 17:17 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git
In-Reply-To: <Pine.LNX.4.64.0709061504521.28586@racer.site>
[-- Attachment #1: Type: text/plain, Size: 990 bytes --]
On Thu, Sep 06, 2007 at 02:05:36PM +0000, Johannes Schindelin wrote:
> Hi,
>
> On Thu, 6 Sep 2007, Pierre Habouzit wrote:
>
> > Signed-off-by: Pierre Habouzit <madcoder@debian.org>
> > ---
> > builtin-rerere.c | 56 +++++++++++++++++------------------------------------
> > 1 files changed, 18 insertions(+), 38 deletions(-)
>
> I like that one very much, but ...
>
> > FILE *f = fopen(path, "r");
> > FILE *out;
> >
> > + strbuf_init(&minus);
> > + strbuf_init(&plus);
> > +
>
> You used spaces instead of tabs here.
crap, and I did that in the 5th patch as well. well, I'll maybe send
privately a "fixed" version of the patch to junio then, to avoid
flooding the list with spacing issues.
And I'll also set my vim to use tabs when I'm hacking on git.
--
·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: strbuf new API, take 2 for inclusion
From: Pierre Habouzit @ 2007-09-06 17:19 UTC (permalink / raw)
To: Jeff King; +Cc: git
In-Reply-To: <20070906171621.GA5305@coredump.intra.peff.net>
[-- Attachment #1: Type: text/plain, Size: 748 bytes --]
On Thu, Sep 06, 2007 at 05:16:21PM +0000, Jeff King wrote:
> On Thu, Sep 06, 2007 at 07:15:02PM +0200, Pierre Habouzit wrote:
>
> > Yes, Junio already did that remark. The reason is that it's forward
> > compatible: if we ever change strbuf's intitial value for some reason,
> > we would just have to rebuild the code. As junio disliked it (and I'm
> > not sure I love it either) I've used it where using the _init() function
> > was impractical.
>
> OK, I missed that discussion. Thanks for the explanation.
That was on IRC, maybe that's the why :)
--
·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] Include a git-push example for creating a remote branch
From: Junio C Hamano @ 2007-09-06 17:42 UTC (permalink / raw)
To: Carl Worth; +Cc: Miles Bader, Shawn O. Pearce, git
In-Reply-To: <874pi7dcba.wl%cworth@cworth.org>
Carl Worth <cworth@cworth.org> writes:
> On Wed, 05 Sep 2007 23:30:31 -0700, Junio C Hamano wrote:
>> It is just nobody felt strong enough reason to sugarcoat the
>> normalized syntax with something like:
>>
>> git push --create remote foo v1.2.0
>
> Couldn't we just use an initial + to indicate this as well?
Unfortunately, no, because + has already been taken to mean
something completely different.
^ permalink raw reply
* Re: People unaware of the importance of "git gc"?
From: Junio C Hamano @ 2007-09-06 17:49 UTC (permalink / raw)
To: Johannes Schindelin
Cc: Nicolas Pitre, Nix, Steven Grimm, Linus Torvalds,
Git Mailing List
In-Reply-To: <Pine.LNX.4.64.0709061651550.28586@racer.site>
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> On Wed, 5 Sep 2007, Junio C Hamano wrote:
>
>> @@ -20,6 +20,7 @@ static const char builtin_gc_usage[] = "git-gc [--prune] [--aggressive]";
>>
>> static int pack_refs = 1;
>> static int aggressive_window = -1;
>> +static int gc_auto_threshold = 6700;
>
> Please don't do that.
>
> When you share objects with another git directory, git-gc --auto can get
> rid of the objects when some objects go away in the referenced repository.
I thought the whole point of "gc --auto" was to have something
that does not lose/prune any objects, even the ones that do not
seem to be referenced from anywhere. That is why invocations of
"git gc --auto" do not say --prune as you saw the second patch,
and the repack command "gc --auto" runs is "repack -d -l"
instead of "repack -a -d -l", which means that it does run
git-prune-packed after repacking but not git-prune.
Maybe I am missing something...
^ permalink raw reply
* Re: [PATCH 1/7] Rework strbuf API and semantics.
From: Kristian Høgsberg @ 2007-09-06 17:49 UTC (permalink / raw)
To: Pierre Habouzit; +Cc: git
In-Reply-To: <118907761140-git-send-email-madcoder@debian.org>
On Thu, 2007-09-06 at 13:20 +0200, Pierre Habouzit wrote:
> The gory details are explained in strbuf.h. The change of semantics this
> patch enforces is that the embeded buffer has always a '\0' character after
> its last byte, to always make it a C-string. The offs-by-one changes are all
> related to that very change.
>
> A strbuf can be used to store byte arrays, or as an extended string
> library. The `buf' member can be passed to any C legacy string function,
> because strbuf operations always ensure there is a terminating \0 at the end
> of the buffer, not accounted in the `len' field of the structure.
>
> A strbuf can be used to generate a string/buffer whose final size is not
> really known, and then "strbuf_detach" can be used to get the built buffer,
> and keep the wrapping "strbuf" structure usable for further work again.
>
> Other interesting feature: strbuf_grow(sb, size) ensure that there is
> enough allocated space in `sb' to put `size' new octets of data in the
> buffer. It helps avoiding reallocating data for nothing when the problem the
> strbuf helps to solve has a known typical size.
This looks good, should be very useful.
Kristian
^ permalink raw reply
* Re: [PATCH] git-svn: remove --first-parent, add --upstream
From: Steven Grimm @ 2007-09-06 17:49 UTC (permalink / raw)
To: Lars Hjemli; +Cc: Junio C Hamano, git, Eric Wong
In-Reply-To: <1189096669534-git-send-email-hjemli@gmail.com>
Lars Hjemli wrote:
> This makes git-svn always issue the --first-parent option to git-log when
> trying to establish the "base" subversion branch, so the --first-parent
> option to git-svn is no longer needed. Instead a new option, --upstream
> <revspec>, is introduced. When this is specified the search for embedded
> git-svn-id lines in commit messages starts at the specified revision, if
> not specified the search starts at HEAD.
>
This is a much better solution -- I can't personally imagine a scenario
where I'd want anything other than the --first-parent behavior, so
making it the default is much more convenient. Thanks.
-Steve
^ permalink raw reply
* Re: [RFC] Convert builin-mailinfo.c to use The Better String Library.
From: Linus Torvalds @ 2007-09-06 17:50 UTC (permalink / raw)
To: Dmitry Kakurin; +Cc: Matthieu Moy, Git
In-Reply-To: <4AFD7EAD1AAC4E54A416BA3F6E6A9E52@ntdev.corp.microsoft.com>
On Wed, 5 Sep 2007, Dmitry Kakurin wrote:
>
> When I first looked at Git source code two things struck me as odd:
> 1. Pure C as opposed to C++. No idea why. Please don't talk about portability,
> it's BS.
*YOU* are full of bullshit.
C++ is a horrible language. It's made more horrible by the fact that a lot
of substandard programmers use it, to the point where it's much much
easier to generate total and utter crap with it. Quite frankly, even if
the choice of C were to do *nothing* but keep the C++ programmers out,
that in itself would be a huge reason to use C.
In other words: the choice of C is the only sane choice. I know Miles
Bader jokingly said "to piss you off", but it's actually true. I've come
to the conclusion that any programmer that would prefer the project to be
in C++ over C is likely a programmer that I really *would* prefer to piss
off, so that he doesn't come and screw up any project I'm involved with.
C++ leads to really really bad design choices. You invariably start using
the "nice" library features of the language like STL and Boost and other
total and utter crap, that may "help" you program, but causes:
- infinite amounts of pain when they don't work (and anybody who tells me
that STL and especially Boost are stable and portable is just so full
of BS that it's not even funny)
- inefficient abstracted programming models where two years down the road
you notice that some abstraction wasn't very efficient, but now all
your code depends on all the nice object models around it, and you
cannot fix it without rewriting your app.
In other words, the only way to do good, efficient, and system-level and
portable C++ ends up to limit yourself to all the things that are
basically available in C. And limiting your project to C means that people
don't screw that up, and also means that you get a lot of programmers that
do actually understand low-level issues and don't screw things up with any
idiotic "object model" crap.
So I'm sorry, but for something like git, where efficiency was a primary
objective, the "advantages" of C++ is just a huge mistake. The fact that
we also piss off people who cannot see that is just a big additional
advantage.
If you want a VCS that is written in C++, go play with Monotone. Really.
They use a "real database". They use "nice object-oriented libraries".
They use "nice C++ abstractions". And quite frankly, as a result of all
these design decisions that sound so appealing to some CS people, the end
result is a horrible and unmaintainable mess.
But I'm sure you'd like it more than git.
Linus
^ permalink raw reply
* Re: [PATCH 2/7] Simplify strbuf uses in archive-tar.c using the proper functions.
From: Kristian Høgsberg @ 2007-09-06 17:59 UTC (permalink / raw)
To: Pierre Habouzit; +Cc: git
In-Reply-To: <11890776111843-git-send-email-madcoder@debian.org>
On Thu, 2007-09-06 at 13:20 +0200, Pierre Habouzit wrote:
> This is just cleaner way to deal with strbufs, using its API rather than
> reinventing it in the module (e.g. strbuf_append_string is just the plain
> strbuf_addstr function, and it was used to perform what strbuf_addch does
> anyways).
> ---
> archive-tar.c | 65 ++++++++++++++-------------------------------------------
> 1 files changed, 16 insertions(+), 49 deletions(-)
>
> diff --git a/archive-tar.c b/archive-tar.c
> index a0763c5..c84d7c0 100644
> --- a/archive-tar.c
> +++ b/archive-tar.c
> @@ -78,19 +78,6 @@ static void write_trailer(void)
> }
> }
>
> -static void strbuf_append_string(struct strbuf *sb, const char *s)
> -{
> - int slen = strlen(s);
> - int total = sb->len + slen;
> - if (total + 1 > sb->alloc) {
> - sb->buf = xrealloc(sb->buf, total + 1);
> - sb->alloc = total + 1;
> - }
> - memcpy(sb->buf + sb->len, s, slen);
> - sb->len = total;
> - sb->buf[total] = '\0';
> -}
> -
> /*
> * pax extended header records have the format "%u %s=%s\n". %u contains
> * the size of the whole string (including the %u), the first %s is the
> @@ -100,26 +87,17 @@ static void strbuf_append_string(struct strbuf *sb, const char *s)
> static void strbuf_append_ext_header(struct strbuf *sb, const char *keyword,
> const char *value, unsigned int valuelen)
> {
> - char *p;
> - int len, total, tmp;
> + int len, tmp;
>
> /* "%u %s=%s\n" */
> len = 1 + 1 + strlen(keyword) + 1 + valuelen + 1;
> for (tmp = len; tmp > 9; tmp /= 10)
> len++;
>
> - total = sb->len + len;
> - if (total > sb->alloc) {
> - sb->buf = xrealloc(sb->buf, total);
> - sb->alloc = total;
> - }
> -
> - p = sb->buf;
> - p += sprintf(p, "%u %s=", len, keyword);
> - memcpy(p, value, valuelen);
> - p += valuelen;
> - *p = '\n';
> - sb->len = total;
> + strbuf_grow(sb, len);
> + strbuf_addf(sb, "%u %s=", len, keyword);
> + strbuf_add(sb, value, valuelen);
> + strbuf_addch(sb, '\n');
> }
This entire function can be collapsed to just:
strbuf_addf(sb, "%u %s=%.*s\n", len, keyword, valuelen, value);
>
> static unsigned int ustar_header_chksum(const struct ustar_header *header)
> @@ -153,8 +131,7 @@ static void write_entry(const unsigned char *sha1, struct strbuf *path,
> struct strbuf ext_header;
>
> memset(&header, 0, sizeof(header));
> - ext_header.buf = NULL;
> - ext_header.len = ext_header.alloc = 0;
> + strbuf_init(&ext_header);
Just use your STRBUF_INIT macro?
> if (!sha1) {
> *header.typeflag = TYPEFLAG_GLOBAL_HEADER;
> @@ -225,8 +202,8 @@ static void write_entry(const unsigned char *sha1, struct strbuf *path,
>
> if (ext_header.len > 0) {
> write_entry(sha1, NULL, 0, ext_header.buf, ext_header.len);
> - free(ext_header.buf);
> }
Remove excess braces?
> + strbuf_release(&ext_header);
> write_blocked(&header, sizeof(header));
> if (S_ISREG(mode) && buffer && size > 0)
> write_blocked(buffer, size);
> @@ -235,11 +212,11 @@ static void write_entry(const unsigned char *sha1, struct strbuf *path,
> static void write_global_extended_header(const unsigned char *sha1)
> {
> struct strbuf ext_header;
> - ext_header.buf = NULL;
> - ext_header.len = ext_header.alloc = 0;
> +
> + strbuf_init(&ext_header);
STRBUF_INIT macro?
> strbuf_append_ext_header(&ext_header, "comment", sha1_to_hex(sha1), 40);
> write_entry(NULL, NULL, 0, ext_header.buf, ext_header.len);
> - free(ext_header.buf);
> + strbuf_release(&ext_header);
> }
>
> static int git_tar_config(const char *var, const char *value)
> @@ -260,28 +237,18 @@ static int write_tar_entry(const unsigned char *sha1,
> const char *base, int baselen,
> const char *filename, unsigned mode, int stage)
> {
> - static struct strbuf path;
> + static struct strbuf path = STRBUF_INIT;
> int filenamelen = strlen(filename);
> void *buffer;
> enum object_type type;
> unsigned long size;
>
> - if (!path.alloc) {
> - path.buf = xmalloc(PATH_MAX);
> - path.alloc = PATH_MAX;
> - path.len = path.eof = 0;
> - }
> - if (path.alloc < baselen + filenamelen + 1) {
> - free(path.buf);
> - path.buf = xmalloc(baselen + filenamelen + 1);
> - path.alloc = baselen + filenamelen + 1;
> - }
> - memcpy(path.buf, base, baselen);
> - memcpy(path.buf + baselen, filename, filenamelen);
> - path.len = baselen + filenamelen;
> - path.buf[path.len] = '\0';
> + strbuf_grow(&path, MAX(PATH_MAX, baselen + filenamelen + 1));
> + strbuf_reset(&path);
Does strbuf_reset() do anything here?
> + strbuf_add(&path, base, baselen);
> + strbuf_add(&path, filename, filenamelen);
> if (S_ISDIR(mode) || S_ISGITLINK(mode)) {
> - strbuf_append_string(&path, "/");
> + strbuf_addch(&path, '/');
> buffer = NULL;
> size = 0;
> } else {
^ permalink raw reply
* Re: [PATCH 2/7] Simplify strbuf uses in archive-tar.c using the proper functions.
From: Pierre Habouzit @ 2007-09-06 18:08 UTC (permalink / raw)
To: Kristian Høgsberg; +Cc: git
In-Reply-To: <1189101569.3423.17.camel@hinata.boston.redhat.com>
[-- Attachment #1: Type: text/plain, Size: 2139 bytes --]
On Thu, Sep 06, 2007 at 05:59:29PM +0000, Kristian Høgsberg wrote:
> On Thu, 2007-09-06 at 13:20 +0200, Pierre Habouzit wrote:
> > + strbuf_grow(sb, len);
> > + strbuf_addf(sb, "%u %s=", len, keyword);
> > + strbuf_add(sb, value, valuelen);
> > + strbuf_addch(sb, '\n');
> > }
>
> This entire function can be collapsed to just:
>
> strbuf_addf(sb, "%u %s=%.*s\n", len, keyword, valuelen, value);
yes, but it's less efficient, because %.*s says that sprintf must copy
at most valuelen bytes from value, but it still has to stop if it finds
a \0 before. And the strbuf_grow has sense because the extend policy at
snprintf time is optimistic: we try to write, and if it didn't fit, we
try again. So there is a huge benefit if we have a clue of the final
size.
I would not change a thing.
> > + strbuf_init(&ext_header);
>
> Just use your STRBUF_INIT macro?
Many people dilike it, I'm not sure it's a good idea either, and the
performance hit should be negligible, if it's not, then we can still
make the _init() function an inline.
> > if (ext_header.len > 0) {
> > write_entry(sha1, NULL, 0, ext_header.buf, ext_header.len);
> > - free(ext_header.buf);
> > }
>
> Remove excess braces?
bah, I don't like to strip braces so I won't do that, else you end up
with stupidities like:
if (foo)
// bar();
do_some_very_important_stuff();
Call me paranoid but well, it saved me so many times ...
> > - memcpy(path.buf, base, baselen);
> > - memcpy(path.buf + baselen, filename, filenamelen);
> > - path.len = baselen + filenamelen;
> > - path.buf[path.len] = '\0';
> > + strbuf_grow(&path, MAX(PATH_MAX, baselen + filenamelen + 1));
> > + strbuf_reset(&path);
>
> Does strbuf_reset() do anything here?
>
> > + strbuf_add(&path, base, baselen);
Yes _reset() sets length to 0. so the add here will write at the start
of the buffer again. It definitely is important !
--
·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: Git's database structure
From: Steven Grimm @ 2007-09-06 18:14 UTC (permalink / raw)
To: Johannes Schindelin
Cc: Jon Smirl, Julian Phillips, Andreas Ericsson, Theodore Tso,
Junio C Hamano, Git Mailing List
In-Reply-To: <Pine.LNX.4.64.0709061354180.28586@racer.site>
Johannes Schindelin wrote:
> But you can add _yet another_ index to it, which can be generated on the
> fly, so that Git only has to generate the information once, and then reuse
> it later. As a benefit of this method, the underlying well-tested
> structure needs no change at all.
>
And in fact, you can do this today, without modifying git-blame at all,
by (ab)using its "-S" option (which lets you specify a custom ancestry
chain to search). By coincidence, I was just showing some people at my
office how to do this yesterday. I'll cut-and-paste from the email I
sent them. I am not claiming this is nearly as desirable as a built-in,
auto-updated secondary index, but it proves the concept, anyway.
Fast-to-generate version:
git-rev-list HEAD -- main.c | awk '{if (last) print last " " $0;
last=$0;}' > /tmp/revlist
This speeds things up a lot, because git blame doesn't have to examine
other revisions:
time git blame main.c
1.56s user 0.30s system 99% cpu 1.868 total
time git blame -S /tmp/revlist main.c
0.21s user 0.03s system 96% cpu 0.249 total
The bad news is that generating that revision list is a bit slow, and if
you do it the naive way I suggested above, you can't use the rev list
with the -M option (to follow renames). The good news is that it's
possible to have that too if you generate a list of revisions that
includes the renames:
# Generate a list of all revisions in the right order (only need to do
this once, not once per file)
git rev-list HEAD > /tmp/all-revs
# Generate a list of the revisions that touched this file, following
copies/renames.
# Could do this in fewer commands but this is hopefully easier to follow.
git blame --porcelain -M main.c | \
egrep '^[0-9a-f]{40}' | \
cut -d' ' -f1 | \
fgrep -f - /tmp/all-revs | \
awk '{if (last) print last " " $0; last=$0;}' > /tmp/revlist
Then -M is fast too:
time git blame -M main.c
1.72s user 0.27s system 89% cpu 2.219 total
time git blame -M -S /tmp/revlist main.c
0.29s user 0.03s system 93% cpu 0.341 total
Oddly, if you use the -S option, "git blame -C" actually gets
significantly *slower*. I am not sure why.
-Steve
^ permalink raw reply
* Re: People unaware of the importance of "git gc"?
From: Linus Torvalds @ 2007-09-06 18:15 UTC (permalink / raw)
To: Junio C Hamano
Cc: Johannes Schindelin, Nicolas Pitre, Nix, Steven Grimm,
Git Mailing List
In-Reply-To: <7vk5r3adlx.fsf@gitster.siamese.dyndns.org>
On Thu, 6 Sep 2007, Junio C Hamano wrote:
>
> I thought the whole point of "gc --auto" was to have something
> that does not lose/prune any objects, even the ones that do not
> seem to be referenced from anywhere. That is why invocations of
> "git gc --auto" do not say --prune as you saw the second patch,
> and the repack command "gc --auto" runs is "repack -d -l"
> instead of "repack -a -d -l", which means that it does run
> git-prune-packed after repacking but not git-prune.
I think "repack -d -l" should be ok from a safety perspective, but I'd
also like to say that always running it incrementally is going to largely
suck after a time.
IOW, if you get lots of small incrmental packs, after a while you really
*do* need to do "git gc" to get the real pack generated.
In the case I saw, James really had hundreds of pack-files. That makes all
our object lookups suck. Yes, not having loose objects at all is a big
deal too, and yes, we try to start from the last pack-file we found (for
the locality that we hope is there), but it's still pretty bad from a
cache usage standpoint, and when we create a new object, we'll first
search (in vain) in all the hundreds of pack-files.
So would "git gc --auto" have helped James? I'm sure it would have. But he
already had lots of pack-files from doing "git fetch/pull", and while
doing the "git gc --auto" will likely *delay* the point where you need to
do a full repack, it doesn't make it go away.
We still need to tell people to do a full git gc at some point, or do it
for them. And the longer you delay doing it, the more expensive it's going
to get to do and/or the worse the final packing is going to be (especially
if it ends up reusing non-optimal packing decisions from the smaller
packs).
So I think the --auto stuff is still worth it, but it's really just
pushing the pain somewhat further out.
(In the kernel community, if you fetch my tree daily, you really *are*
going to have hundreds and hundreds of packfiles just from doing that).
So I'd really like us to also remind people to do a *real* and full "git
gc", not just the incremental ones.
Linus
^ permalink raw reply
* Re: [PATCH 2/7] Simplify strbuf uses in archive-tar.c using the proper functions.
From: Kristian Høgsberg @ 2007-09-06 18:18 UTC (permalink / raw)
To: Pierre Habouzit; +Cc: git
In-Reply-To: <20070906180858.GJ8451@artemis.corp>
On Thu, 2007-09-06 at 20:08 +0200, Pierre Habouzit wrote:
> On Thu, Sep 06, 2007 at 05:59:29PM +0000, Kristian Høgsberg wrote:
> > On Thu, 2007-09-06 at 13:20 +0200, Pierre Habouzit wrote:
> > > + strbuf_grow(sb, len);
> > > + strbuf_addf(sb, "%u %s=", len, keyword);
> > > + strbuf_add(sb, value, valuelen);
> > > + strbuf_addch(sb, '\n');
> > > }
> >
> > This entire function can be collapsed to just:
> >
> > strbuf_addf(sb, "%u %s=%.*s\n", len, keyword, valuelen, value);
>
> yes, but it's less efficient, because %.*s says that sprintf must copy
> at most valuelen bytes from value, but it still has to stop if it finds
> a \0 before. And the strbuf_grow has sense because the extend policy at
> snprintf time is optimistic: we try to write, and if it didn't fit, we
> try again. So there is a huge benefit if we have a clue of the final
> size.
Yeah, my strbuf_addf() suggestion doesn't work, since the format seems
to indicate the embedded NULs indeed is allowed (the %u is the length of
the value), so *.% could truncate data.
> > > - memcpy(path.buf, base, baselen);
> > > - memcpy(path.buf + baselen, filename, filenamelen);
> > > - path.len = baselen + filenamelen;
> > > - path.buf[path.len] = '\0';
> > > + strbuf_grow(&path, MAX(PATH_MAX, baselen + filenamelen + 1));
> > > + strbuf_reset(&path);
> >
> > Does strbuf_reset() do anything here?
> >
> > > + strbuf_add(&path, base, baselen);
>
> Yes _reset() sets length to 0. so the add here will write at the start
> of the buffer again. It definitely is important !
But where was length set to non-zero? path is initialized on entry to
the function, and strbuf_grow() just increases the allocation, not
length, right?
Kristian
^ permalink raw reply
* Re: [PATCH 2/7] Simplify strbuf uses in archive-tar.c using the proper functions.
From: Pierre Habouzit @ 2007-09-06 18:27 UTC (permalink / raw)
To: Kristian Høgsberg; +Cc: git
In-Reply-To: <1189102714.3423.22.camel@hinata.boston.redhat.com>
[-- Attachment #1: Type: text/plain, Size: 1378 bytes --]
On Thu, Sep 06, 2007 at 06:18:34PM +0000, Kristian Høgsberg wrote:
> On Thu, 2007-09-06 at 20:08 +0200, Pierre Habouzit wrote:
> > On Thu, Sep 06, 2007 at 05:59:29PM +0000, Kristian Høgsberg wrote:
> > > On Thu, 2007-09-06 at 13:20 +0200, Pierre Habouzit wrote:
> > > > - memcpy(path.buf, base, baselen);
> > > > - memcpy(path.buf + baselen, filename, filenamelen);
> > > > - path.len = baselen + filenamelen;
> > > > - path.buf[path.len] = '\0';
> > > > + strbuf_grow(&path, MAX(PATH_MAX, baselen + filenamelen + 1));
> > > > + strbuf_reset(&path);
> > >
> > > Does strbuf_reset() do anything here?
> > >
> > > > + strbuf_add(&path, base, baselen);
> >
> > Yes _reset() sets length to 0. so the add here will write at the start
> > of the buffer again. It definitely is important !
>
> But where was length set to non-zero? path is initialized on entry to
> the function, and strbuf_grow() just increases the allocation, not
> length, right?
The path is static, hence when you reenter the function you have the
last value in it. The fact that it's static may be questionable, but it
was like it before, I kept it, I've supposed it was for performance
reasons.
--
·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: People unaware of the importance of "git gc"?
From: Steven Grimm @ 2007-09-06 18:29 UTC (permalink / raw)
To: Linus Torvalds
Cc: Junio C Hamano, Johannes Schindelin, Nicolas Pitre, Nix,
Git Mailing List
In-Reply-To: <alpine.LFD.0.999.0709061906010.5626@evo.linux-foundation.org>
Linus Torvalds wrote:
> IOW, if you get lots of small incrmental packs, after a while you really
> *do* need to do "git gc" to get the real pack generated.
>
I wonder if it makes sense to repack just the small incremental packs
into a large (but still incremental) pack, rather than repacking the
entire repository. Presumably that would be a lot faster than a full
"git gc", while still giving you reasonably good packing (at least, if
the threshold is set to a hugh enough number of small packs) and keeping
things fast. That could run as a second phase of "git gc --auto" -- it
should be quick enough to not be too terribly annoying since we're not
running it in the background.
Yeah, if you use the same repo for a long time, you'll accumulate a ton
of medium-sized packs this way, but (a) that's much better than the
situation we have today, and (b) it puts off the performance degradation
for long enough that it becomes more reasonable to expect people to find
out about running the full "git gc" in the meantime, or for git to
further evolve to not need it.
-Steve
^ permalink raw reply
* Re: [PATCH 6/7] Eradicate yet-another-buffer implementation in buitin-rerere.c
From: David Kastrup @ 2007-09-06 20:16 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git
In-Reply-To: <20070906171734.GG8451@artemis.corp>
Pierre Habouzit <madcoder@debian.org> writes:
> On Thu, Sep 06, 2007 at 02:05:36PM +0000, Johannes Schindelin wrote:
>> You used spaces instead of tabs here.
>
> crap, and I did that in the 5th patch as well. well, I'll maybe send
> privately a "fixed" version of the patch to junio then, to avoid
> flooding the list with spacing issues.
>
> And I'll also set my vim to use tabs when I'm hacking on git.
Just use Emacs and
M-x c-set-style RET linux RET
--
David Kastrup, Kriemhildstr. 15, 44793 Bochum
^ permalink raw reply
* Re: [PATCH 4/3] archive: specfile syntax change: "$Format:%PLCHLDR$" instead of just "%PLCHLDR"
From: René Scharfe @ 2007-09-06 20:35 UTC (permalink / raw)
To: Johannes Schindelin
Cc: Junio C Hamano, Andreas Ericsson, Git Mailing List,
Michael Gernoth, Thomas Glanzmann
In-Reply-To: <Pine.LNX.4.64.0709061803590.28586@racer.site>
Johannes Schindelin schrieb:
>> +static void *format_specfile(const struct commit *commit, const char *format,
>> + unsigned long *sizep)
>
> Should this not be "char *buffer" instead of "const char *format"? Or
> even better: a "struct strbuf *"?
const is correct, because the data in the buffer shouldn't be (and
isn't) modified.
"buffer" is a generic term; "format" on the other hand indicates the
meaning of the data within the buffer.
Input and output might indeed be passed along as struct strbuf, even
more so since the new API encourages its use as generic buffer (format
can contain NULs).
>> +{
>> + unsigned long len = *sizep, result_len = 0;
>> + const char *a = format;
>> + char *result = NULL;
>> +
>> + for (;;) {
>> + const char *b, *c;
>> + char *fmt, *formatted = NULL;
>> + unsigned long a_len, fmt_len, formatted_len, allocated = 0;
>
> Maybe initialise formatted_len, just to be on the safe side?
I wish I could use C99 syntax and move its declaration down to its first
use, then it would be obvious that formatted_len is never used without
initialization.
>> +
>> + b = memchr(a, '$', len);
>> + if (!b || a + len < b + 9 || memcmp(b + 1, "Format:", 7))
>> + break;
>
> Wouldn't memmem(buffer, len, "$Format:", 8) be better here?
Oh, that's a nice GNU extension, didn't know it before. We might import
it to compat etc., but I think that's better left for a follow-up patch.
> A general comment: since you plan to output the result into a file anyway,
> it should be even easier to avoid realloc(), and do a
> print_formatted_specfile() instead of a format_specfile(), no?
Hmm, not sure what you mean. At least archive-tar needs the expanded
contents in a buffer (not immediately written to stdout) because it
tries to mimic a real tar and always writes in blocks of 10k and
therefore needs to buffer the output.
Thanks!
René
^ permalink raw reply
* Re: [PATCH 5/3] archive: rename attribute specfile to export-subst
From: René Scharfe @ 2007-09-06 20:38 UTC (permalink / raw)
To: Johannes Schindelin, Junio C Hamano
Cc: Andreas Ericsson, Git Mailing List, Michael Gernoth,
Thomas Glanzmann
In-Reply-To: <Pine.LNX.4.64.0709061811460.28586@racer.site>
Johannes Schindelin schrieb:
> The bigger question is now if these two patches should be folded back
> into your original patch series, or stand alone as commits of their
> own...
Yes, indeed. I submitted them as add-on patches because I noticed the
first three are already in next, but I sure can make a fresh three-part
series based on master. Junio?
Thanks,
René
^ 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