* Re: [PATCH 4/5] Full rework of quote_c_style and write_name_quoted.
From: Junio C Hamano @ 2007-09-19 8:28 UTC (permalink / raw)
To: Pierre Habouzit; +Cc: git
In-Reply-To: <20070918224122.2B55D344AB3@madism.org>
Pierre Habouzit <madcoder@debian.org> writes:
> ...
> Signed-off-by: Pierre Habouzit <madcoder@debian.org>
> ---
> builtin-apply.c | 83 +++++--------
> builtin-blame.c | 3 +-
> builtin-check-attr.c | 2 +-
> builtin-checkout-index.c | 4 +-
> builtin-ls-files.c | 13 +--
> builtin-ls-tree.c | 6 +-
> combine-diff.c | 16 +--
> diff.c | 303 +++++++++++++++++-----------------------------
> quote.c | 198 +++++++++++++++++-------------
> quote.h | 8 +-
> 10 files changed, 268 insertions(+), 368 deletions(-)
> ...
> diff --git a/builtin-apply.c b/builtin-apply.c
> index cffbe52..0328863 100644
> --- a/builtin-apply.c
> +++ b/builtin-apply.c
> @@ -1378,61 +1377,50 @@ static const char minuses[]= "--------------------------------------------------
>
> static void show_stats(struct patch *patch)
> {
> - const char *prefix = "";
> - char *name = patch->new_name;
> - char *qname = NULL;
> - int len, max, add, del, total;
> -
> - if (!name)
> - name = patch->old_name;
> + struct strbuf qname;
> + char *cp = patch->new_name ? patch->new_name : patch->old_name;
> + int max, add, del;
>
> - if (0 < (len = quote_c_style(name, NULL, NULL, 0))) {
> - qname = xmalloc(len + 1);
> - quote_c_style(name, qname, NULL, 0);
> - name = qname;
> - }
> + strbuf_init(&qname, 0);
> + quote_c_style(cp, &qname, NULL, 0);
>
> /*
> * "scale" the filename
> */
> - len = strlen(name);
> max = max_len;
> if (max > 50)
> max = 50;
> - if (len > max) {
> - char *slash;
> - prefix = "...";
> - max -= 3;
> - name += len - max;
> - slash = strchr(name, '/');
> - if (slash)
> - name = slash;
> +
> + if (qname.len > max) {
> + cp = strchr(qname.buf + qname.len + 3 - max, '/');
> + if (cp)
> + cp = qname.buf + qname.len + 3 - max;
> + strbuf_splice(&qname, 0, cp - qname.buf, "...", 3);
> + }
At this point, you have max that is larger by 3 than what old
code had. That would make the next two printf() you added as
expected. This affects scaling of add/delete code. Is this
intentional? I _think_ the change is correct (there is no
reason that name display being cliped should affect the length
of the bar graph), but that should have been documented as a
separate bugfix in the commit log.
> diff --git a/quote.c b/quote.c
> index 67c6527..a8a755a 100644
> --- a/quote.c
> +++ b/quote.c
> @@ -114,83 +114,142 @@ char *sq_dequote(char *arg)
> }
> }
>
> +/* 1 means: quote as octal
> + * 0 means: quote as octal if (quote_path_fully)
> + * -1 means: never quote
> + * c: quote as "\\c"
> + */
> +#define X8(x) x, x, x, x, x, x, x, x
> +#define X16(x) X8(x), X8(x)
> +static signed char const sq_lookup[256] = {
> + /* 0 1 2 3 4 5 6 7 */
> + /* 0x00 */ 1, 1, 1, 1, 1, 1, 'a', 1,
Isn't BEL == 0x07, not 0x06?
> + /* 0x08 */ 'b', 't', 'n', 'v', 'f', 'r', 1, 1,
> + /* 0x10 */ X16(1),
> + /* 0x20 */ -1, -1, '"', -1, -1, -1, -1, -1,
> + /* 0x28 */ X16(-1), X16(-1), X16(-1),
> + /* 0x58 */ -1, -1, -1, -1,'\\', -1, -1, -1,
> + /* 0x60 */ X16(-1), X16(-1),
Shouldn't you quote DEL == 0177 here?
> + /* 0x80 */ /* set to 0 */
> +};
> +
> +static inline int sq_must_quote(char c) {
> + return sq_lookup[(unsigned char)c] + quote_path_fully > 0;
> +}
> +
> +/* returns the longest prefix not needing a quote up to maxlen if positive.
> + This stops at the first \0 because it's marked as a character needing an
> + escape */
> +static size_t next_quote_pos(const char *s, ssize_t maxlen)
> +{
> + size_t len;
> + if (maxlen < 0) {
> + for (len = 0; !sq_must_quote(s[len]); len++);
> + } else {
> + for (len = 0; len < maxlen && !sq_must_quote(s[len]); len++);
> + }
> + return len;
> +}
> +
> /*
> * C-style name quoting.
> *
> - * Does one of three things:
> - *
> * (1) if outbuf and outfp are both NULL, inspect the input name and
> * counts the number of bytes that are needed to hold c_style
> * quoted version of name, counting the double quotes around
> * it but not terminating NUL, and returns it. However, if name
> * does not need c_style quoting, it returns 0.
> *
You need to update this comment; you do not have outbuf nor
outfp anymore, you have something else.
^ permalink raw reply
* Re: [PATCH 4/5] Full rework of quote_c_style and write_name_quoted.
From: David Kastrup @ 2007-09-19 8:28 UTC (permalink / raw)
To: git
In-Reply-To: <20070919082111.GB28205@artemis.corp>
Pierre Habouzit <madcoder@debian.org> writes:
> On Wed, Sep 19, 2007 at 08:08:02AM +0000, Andreas Ericsson wrote:
>> Then perhaps a separate patch for this would have been prudent? I'm not
>> against the change per se and I understand the reasoning behind it, but
>> it seems to go against Documentation/SubmittingPatches (submit one change
>> at a time).
>
> Yes, the thing is, I wrote it in one piece, and had a _very_ hard time
> splitting it. The aggregated patches had almost no chunks, and editing
> diffs by hand isn't what I like to do :)
Use Emacs for it. After loading the patch in a file, type
Esc x diff-mode RET
If you now move to a place in the middle of a hunk and type C-c C-s,
the hunk is split at that point into two hunks. C-c C-k kills the
current hunk. C-x C-s saves the file, C-x C-c exits Emacs.
In that manner throwing selected material out of a patch is rather
straightforward, even when it is in the middle of a hunk.
--
David Kastrup
^ permalink raw reply
* Re: [PATCH 2/2] Use xmemdup in many places.
From: Pierre Habouzit @ 2007-09-19 8:26 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <7v6427qdqr.fsf@gitster.siamese.dyndns.org>
[-- Attachment #1: Type: text/plain, Size: 2109 bytes --]
On Wed, Sep 19, 2007 at 08:08:12AM +0000, Junio C Hamano wrote:
> Pierre Habouzit <madcoder@debian.org> writes:
>
> > Signed-off-by: Pierre Habouzit <madcoder@debian.org>
> > ---
> > attr.c | 7 +------
> > builtin-add.c | 8 ++------
> > builtin-apply.c | 11 ++---------
> > builtin-fetch--tool.c | 6 +-----
> > builtin-fmt-merge-msg.c | 17 ++++++-----------
> > builtin-for-each-ref.c | 40 +++++++++-------------------------------
> > builtin-log.c | 12 ++----------
> > builtin-ls-files.c | 9 +--------
> > builtin-mv.c | 5 +----
> > builtin-revert.c | 4 +---
> > builtin-shortlog.c | 11 ++---------
> > commit.c | 16 ++++++----------
> > connect.c | 4 +---
> > convert.c | 7 +------
> > diff.c | 13 ++-----------
> > diffcore-order.c | 7 ++-----
> > fast-import.c | 4 +---
> > http-push.c | 9 ++-------
> > imap-send.c | 20 +++++---------------
> > merge-recursive.c | 19 ++++---------------
> > refs.c | 12 ++++--------
> > sha1_file.c | 12 +++---------
> > tag.c | 4 +---
> > 23 files changed, 60 insertions(+), 197 deletions(-)
> > ...
> > diff --git a/builtin-apply.c b/builtin-apply.c
> > index 05011bb..900d0a7 100644
> > --- a/builtin-apply.c
> > +++ b/builtin-apply.c
> > @@ -293,11 +293,7 @@ static char *find_name(const char *line, char *def, int p_value, int terminate)
> > return def;
> > }
> >
> > - name = xmalloc(len + 1);
> > - memcpy(name, start, len);
> > - name[len] = 0;
> > - free(def);
> > - return name;
> > + return xmemdup(start, len);
> > }
>
> Did we start leaking "def" here?
Hmm I fear we are. that has to be fixed indeed.
--
·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 2/5] sq_quote_argv and add_to_string rework with strbuf's.
From: Pierre Habouzit @ 2007-09-19 8:23 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <7v3axbqdnw.fsf@gitster.siamese.dyndns.org>
[-- Attachment #1: Type: text/plain, Size: 1508 bytes --]
On Wed, Sep 19, 2007 at 08:09:55AM +0000, Junio C Hamano wrote:
> Pierre Habouzit <madcoder@debian.org> writes:
>
> > * sq_quote_buf is made public, and works on a strbuf.
> > * sq_quote_argv also works on a strbuf.
> > * make sq_quote_argv take a "maxlen" argument to check the buffer won't grow
> > too big.
> >
> > Signed-off-by: Pierre Habouzit <madcoder@debian.org>
> > ---
> > connect.c | 21 ++++++--------
> > git.c | 16 +++-------
> > quote.c | 91 ++++++++++++++++---------------------------------------------
> > quote.h | 9 ++----
> > rsh.c | 33 ++++++----------------
> > trace.c | 35 +++++++-----------------
> > 6 files changed, 60 insertions(+), 145 deletions(-)
> > ...
> > diff --git a/quote.c b/quote.c
> > index d88bf75..4df3262 100644
> > --- a/quote.c
> > +++ b/quote.c
> > @@ -20,29 +20,26 @@ static inline int need_bs_quote(char c)
> > return (c == '\'' || c == '!');
> > }
> >
> > -static size_t sq_quote_buf(char *dst, size_t n, const char *src)
> > +void sq_quote_buf(struct strbuf *dst, const char *src)
> > {
>
> You got rid of use of EMIT() macro which is local to this
> function, so you need to remove the #undef/#define in front of
> the function as well.
Isn't it used by the function before ? hmm I'll check then.
--
·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/5] Rework unquote_c_style to work on a strbuf.
From: Pierre Habouzit @ 2007-09-19 8:22 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Shawn O. Pearce, git
In-Reply-To: <7v4phrqdow.fsf@gitster.siamese.dyndns.org>
[-- Attachment #1: Type: text/plain, Size: 2116 bytes --]
On Wed, Sep 19, 2007 at 08:09:19AM +0000, Junio C Hamano wrote:
> Pierre Habouzit <madcoder@debian.org> writes:
>
> > If the gain is not obvious in the diffstat, the resulting code is more
> > readable, _and_ in checkout-index/update-index we now reuse the same buffer
> > to unquote strings instead of always freeing/mallocing.
> >
> > This also is more coherent with the next patch that reworks quoting
> > functions.
> >
> > The quoting function is also made more efficient scanning for backslashes
> > and treating portions of strings without a backslash at once.
> >
> > Signed-off-by: Pierre Habouzit <madcoder@debian.org>
> > ---
> > builtin-apply.c | 125 +++++++++++++++++++++++-----------------------
> > builtin-checkout-index.c | 27 +++++-----
> > builtin-update-index.c | 51 ++++++++++---------
> > fast-import.c | 47 ++++++++---------
> > mktree.c | 25 +++++----
> > quote.c | 92 ++++++++++++++++------------------
> > quote.h | 2 +-
> > 7 files changed, 184 insertions(+), 185 deletions(-)
> > ...
> > diff --git a/quote.c b/quote.c
> > index 4df3262..67c6527 100644
> > --- a/quote.c
> > +++ b/quote.c
> > @@ -201,68 +201,62 @@ int quote_c_style(const char *name, char *outbuf, FILE *outfp, int no_dq)
> > * should free when done. Updates endp pointer to point at
> > * one past the ending double quote if given.
> > */
>
> You need to update the comment above which talks about the input
> and return values. You no longer return an allocated memory
> which the caller should free. You return something else.
Oh my, okay I'll do that. I intend to send an updated series at some
point anyways, because of the two mistakes I already spotted, and
because next conflicts with this series (in not too hard ways to deal
with but still, that would save you some useless merges).
--
·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 4/5] Full rework of quote_c_style and write_name_quoted.
From: Pierre Habouzit @ 2007-09-19 8:21 UTC (permalink / raw)
To: Andreas Ericsson; +Cc: Junio C Hamano, git
In-Reply-To: <46F0D8E2.5090706@op5.se>
[-- Attachment #1: Type: text/plain, Size: 678 bytes --]
On Wed, Sep 19, 2007 at 08:08:02AM +0000, Andreas Ericsson wrote:
> Then perhaps a separate patch for this would have been prudent? I'm not
> against the change per se and I understand the reasoning behind it, but
> it seems to go against Documentation/SubmittingPatches (submit one change
> at a time).
Yes, the thing is, I wrote it in one piece, and had a _very_ hard time
splitting it. The aggregated patches had almost no chunks, and editing
diffs by hand isn't what I like to do :)
--
·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 2/5] sq_quote_argv and add_to_string rework with strbuf's.
From: Junio C Hamano @ 2007-09-19 8:09 UTC (permalink / raw)
To: Pierre Habouzit; +Cc: git
In-Reply-To: <20070918224120.1DC44344AB3@madism.org>
Pierre Habouzit <madcoder@debian.org> writes:
> * sq_quote_buf is made public, and works on a strbuf.
> * sq_quote_argv also works on a strbuf.
> * make sq_quote_argv take a "maxlen" argument to check the buffer won't grow
> too big.
>
> Signed-off-by: Pierre Habouzit <madcoder@debian.org>
> ---
> connect.c | 21 ++++++--------
> git.c | 16 +++-------
> quote.c | 91 ++++++++++++++++---------------------------------------------
> quote.h | 9 ++----
> rsh.c | 33 ++++++----------------
> trace.c | 35 +++++++-----------------
> 6 files changed, 60 insertions(+), 145 deletions(-)
> ...
> diff --git a/quote.c b/quote.c
> index d88bf75..4df3262 100644
> --- a/quote.c
> +++ b/quote.c
> @@ -20,29 +20,26 @@ static inline int need_bs_quote(char c)
> return (c == '\'' || c == '!');
> }
>
> -static size_t sq_quote_buf(char *dst, size_t n, const char *src)
> +void sq_quote_buf(struct strbuf *dst, const char *src)
> {
You got rid of use of EMIT() macro which is local to this
function, so you need to remove the #undef/#define in front of
the function as well.
^ permalink raw reply
* Re: [PATCH 3/5] Rework unquote_c_style to work on a strbuf.
From: Junio C Hamano @ 2007-09-19 8:09 UTC (permalink / raw)
To: Pierre Habouzit; +Cc: Shawn O. Pearce, git
In-Reply-To: <20070918224121.24C3B344AB3@madism.org>
Pierre Habouzit <madcoder@debian.org> writes:
> If the gain is not obvious in the diffstat, the resulting code is more
> readable, _and_ in checkout-index/update-index we now reuse the same buffer
> to unquote strings instead of always freeing/mallocing.
>
> This also is more coherent with the next patch that reworks quoting
> functions.
>
> The quoting function is also made more efficient scanning for backslashes
> and treating portions of strings without a backslash at once.
>
> Signed-off-by: Pierre Habouzit <madcoder@debian.org>
> ---
> builtin-apply.c | 125 +++++++++++++++++++++++-----------------------
> builtin-checkout-index.c | 27 +++++-----
> builtin-update-index.c | 51 ++++++++++---------
> fast-import.c | 47 ++++++++---------
> mktree.c | 25 +++++----
> quote.c | 92 ++++++++++++++++------------------
> quote.h | 2 +-
> 7 files changed, 184 insertions(+), 185 deletions(-)
> ...
> diff --git a/quote.c b/quote.c
> index 4df3262..67c6527 100644
> --- a/quote.c
> +++ b/quote.c
> @@ -201,68 +201,62 @@ int quote_c_style(const char *name, char *outbuf, FILE *outfp, int no_dq)
> * should free when done. Updates endp pointer to point at
> * one past the ending double quote if given.
> */
You need to update the comment above which talks about the input
and return values. You no longer return an allocated memory
which the caller should free. You return something else.
^ permalink raw reply
* Re: [PATCH 2/2] Use xmemdup in many places.
From: Junio C Hamano @ 2007-09-19 8:08 UTC (permalink / raw)
To: Pierre Habouzit; +Cc: git
In-Reply-To: <20070917161142.D3C9A344A49@madism.org>
Pierre Habouzit <madcoder@debian.org> writes:
> Signed-off-by: Pierre Habouzit <madcoder@debian.org>
> ---
> attr.c | 7 +------
> builtin-add.c | 8 ++------
> builtin-apply.c | 11 ++---------
> builtin-fetch--tool.c | 6 +-----
> builtin-fmt-merge-msg.c | 17 ++++++-----------
> builtin-for-each-ref.c | 40 +++++++++-------------------------------
> builtin-log.c | 12 ++----------
> builtin-ls-files.c | 9 +--------
> builtin-mv.c | 5 +----
> builtin-revert.c | 4 +---
> builtin-shortlog.c | 11 ++---------
> commit.c | 16 ++++++----------
> connect.c | 4 +---
> convert.c | 7 +------
> diff.c | 13 ++-----------
> diffcore-order.c | 7 ++-----
> fast-import.c | 4 +---
> http-push.c | 9 ++-------
> imap-send.c | 20 +++++---------------
> merge-recursive.c | 19 ++++---------------
> refs.c | 12 ++++--------
> sha1_file.c | 12 +++---------
> tag.c | 4 +---
> 23 files changed, 60 insertions(+), 197 deletions(-)
> ...
> diff --git a/builtin-apply.c b/builtin-apply.c
> index 05011bb..900d0a7 100644
> --- a/builtin-apply.c
> +++ b/builtin-apply.c
> @@ -293,11 +293,7 @@ static char *find_name(const char *line, char *def, int p_value, int terminate)
> return def;
> }
>
> - name = xmalloc(len + 1);
> - memcpy(name, start, len);
> - name[len] = 0;
> - free(def);
> - return name;
> + return xmemdup(start, len);
> }
Did we start leaking "def" here?
> diff --git a/builtin-for-each-ref.c b/builtin-for-each-ref.c
> index 0afa1c5..287d52a 100644
> --- a/builtin-for-each-ref.c
> +++ b/builtin-for-each-ref.c
> ...
> @@ -305,46 +301,28 @@ static const char *find_wholine(const char *who, int wholen, const char *buf, un
> ...
> static const char *copy_name(const char *buf)
> {
> - const char *eol = strchr(buf, '\n');
> - const char *eoname = strstr(buf, " <");
> - char *line;
> - int len;
> - if (!(eoname && eol && eoname < eol))
> - return "";
> - len = eoname - buf;
> - line = xmalloc(len + 1);
> - memcpy(line, buf, len);
> - line[len] = 0;
> - return line;
> + const char *cp;
> + for (cp = buf; *cp != '\n'; cp++) {
> + if (!strncmp(cp, " <", 2))
> + return xmemdup(buf, cp - buf);
> + }
> + return "";
> }
At least the loop should terminate upon (!*cp); if you do not
have '\n' in the buffer what happens?
^ permalink raw reply
* Re: [PATCH 4/5] Full rework of quote_c_style and write_name_quoted.
From: Andreas Ericsson @ 2007-09-19 8:08 UTC (permalink / raw)
To: Pierre Habouzit, Andreas Ericsson, Junio C Hamano, git
In-Reply-To: <20070919080030.GA28205@artemis.corp>
Pierre Habouzit wrote:
> On Wed, Sep 19, 2007 at 06:37:31AM +0000, Andreas Ericsson wrote:
>> Pierre Habouzit wrote:
>>> diff --git a/builtin-blame.c b/builtin-blame.c
>>> index e364b6c..16c0ca8 100644
>>> --- a/builtin-blame.c
>>> +++ b/builtin-blame.c
>>> @@ -1430,8 +1430,7 @@ static void get_commit_info(struct commit *commit,
>>> static void write_filename_info(const char *path)
>>> {
>>> printf("filename ");
>>> - write_name_quoted(NULL, 0, path, 1, stdout);
>>> - putchar('\n');
>>> + write_name_quoted(path, stdout, '\n');
>>> }
>>>
>> This looks like a candidate for a macro. I'm not sure if gcc optimizes
>> sibling calls in void functions with -O2, and it doesn't inline without
>> -O3.
>
> Well, there is little point. write_name_quoted behaviour changes if
> the last argument is \0 or non-\0 (see patch comment and quote.c code),
> so it does not really matter to inline the "putchar" IMHO.
>
>>> -static void diff_flush_raw(struct diff_filepair *p,
>>> - struct diff_options *options)
>>> +static void diff_flush_raw(struct diff_filepair *p, struct diff_options
>>> *opt)
>> Parameter rename? I'd have thought the patch was big enough as it is ;-)
>
> I'm anal when it comes to code: the rule of the least surprise should
> apply, and consistency is fundamental. And it happens that diff_options
> are always called `opt' in diff.c, except in that place (and it allows
> to write the prototype of the function on one line).
>
Then perhaps a separate patch for this would have been prudent? I'm not
against the change per se and I understand the reasoning behind it, but
it seems to go against Documentation/SubmittingPatches (submit one change
at a time).
--
Andreas Ericsson andreas.ericsson@op5.se
OP5 AB www.op5.se
Tel: +46 8-230225 Fax: +46 8-230231
^ permalink raw reply
* Re: [PATCH] Simplify strbuf uses in archive-tar.c using the proper functions.
From: Junio C Hamano @ 2007-09-19 8:06 UTC (permalink / raw)
To: Pierre Habouzit; +Cc: git
In-Reply-To: <11890199232128-git-send-email-madcoder@debian.org>
Pierre Habouzit <madcoder@debian.org> writes:
> 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
> @@ -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));
Where are you getting the MAX() macro from? On my Linux box
it appears that <sys/params.h> happens to define it but I do not
think that is something we can rely upon portably.
Moreover, isn't this allocation wrong? I thought "grow" was
about "we want this much more in addition to the existing
length", not "reserve at least this much", and this "path" is a
static that will keep the buffer and length from the previous
invocation.
> + strbuf_reset(&path);
> + 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 {
Having said that, I suspect that the preallocation does not
really matter in practice. How about doing something like:
strbuf_reset(&path);
strbuf_grow(&path, PATH_MAX);
strbuf_add(&path, base, baselen);
strbuf_add(&path, filename, filenamelen);
^ permalink raw reply
* Re: [PATCH] Further strbuf re-engineering.
From: Junio C Hamano @ 2007-09-19 8:05 UTC (permalink / raw)
To: Pierre Habouzit; +Cc: git
In-Reply-To: <1189019923943-git-send-email-madcoder@debian.org>
Pierre Habouzit <madcoder@debian.org> writes:
> Signed-off-by: Pierre Habouzit <madcoder@debian.org>
> ---
> builtin-apply.c | 29 ++++++-----------------
> builtin-blame.c | 34 +++++++++-------------------
> builtin-commit-tree.c | 59 ++++++++++--------------------------------------
> diff.c | 25 ++++++--------------
> 4 files changed, 40 insertions(+), 107 deletions(-)
> ...
> diff --git a/builtin-commit-tree.c b/builtin-commit-tree.c
> index ccbcbe3..ee74814 100644
> --- a/builtin-commit-tree.c
> +++ b/builtin-commit-tree.c
> @@ -8,42 +8,13 @@
> ...
> /*
> * FIXME! Share the code with "write-tree.c"
> */
You fixed it. Remove this comment.
^ permalink raw reply
* Re: [PATCH 4/5] Full rework of quote_c_style and write_name_quoted.
From: Pierre Habouzit @ 2007-09-19 8:00 UTC (permalink / raw)
To: Andreas Ericsson; +Cc: Junio C Hamano, git
In-Reply-To: <46F0C3AB.8010801@op5.se>
[-- Attachment #1: Type: text/plain, Size: 1911 bytes --]
On Wed, Sep 19, 2007 at 06:37:31AM +0000, Andreas Ericsson wrote:
> Pierre Habouzit wrote:
> > diff --git a/builtin-blame.c b/builtin-blame.c
> >index e364b6c..16c0ca8 100644
> >--- a/builtin-blame.c
> >+++ b/builtin-blame.c
> >@@ -1430,8 +1430,7 @@ static void get_commit_info(struct commit *commit,
> > static void write_filename_info(const char *path)
> > {
> > printf("filename ");
> >- write_name_quoted(NULL, 0, path, 1, stdout);
> >- putchar('\n');
> >+ write_name_quoted(path, stdout, '\n');
> > }
> >
>
> This looks like a candidate for a macro. I'm not sure if gcc optimizes
> sibling calls in void functions with -O2, and it doesn't inline without
> -O3.
Well, there is little point. write_name_quoted behaviour changes if
the last argument is \0 or non-\0 (see patch comment and quote.c code),
so it does not really matter to inline the "putchar" IMHO.
> > -static void diff_flush_raw(struct diff_filepair *p,
> >- struct diff_options *options)
> >+static void diff_flush_raw(struct diff_filepair *p, struct diff_options
> >*opt)
>
> Parameter rename? I'd have thought the patch was big enough as it is ;-)
I'm anal when it comes to code: the rule of the least surprise should
apply, and consistency is fundamental. And it happens that diff_options
are always called `opt' in diff.c, except in that place (and it allows
to write the prototype of the function on one line).
> Other than that, the diffstat calls this a good patch, and given the
> fact that all your previous series passed all tests, I assume this one
> does too.
Yes, before submitting a series I check the testsuite passes at each
step, so that it doesn't break git-bisect in obvious ways.
--
·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] git-merge: add option --no-ff
From: Lars Hjemli @ 2007-09-19 7:09 UTC (permalink / raw)
To: Peter Baumann, Junio C Hamano; +Cc: git
In-Reply-To: <20070918225134.GA5906@xp.machine.xx>
On 9/19/07, Peter Baumann <waste.manager@gmx.de> wrote:
> This should be quoted, e.g.
> + if test "$no_ff" = 't'
>
Ouch, sorry about that. I can send an updated patch late tonight (it's
now early morning here), but I'm not sure Junio wants/needs it. Junio?
--
larsh
^ permalink raw reply
* Re: [PATCH 4/5] Full rework of quote_c_style and write_name_quoted.
From: Andreas Ericsson @ 2007-09-19 6:37 UTC (permalink / raw)
To: Pierre Habouzit; +Cc: Junio C Hamano, git
In-Reply-To: <20070918224122.2B55D344AB3@madism.org>
Pierre Habouzit wrote:
>
> diff --git a/builtin-blame.c b/builtin-blame.c
> index e364b6c..16c0ca8 100644
> --- a/builtin-blame.c
> +++ b/builtin-blame.c
> @@ -1430,8 +1430,7 @@ static void get_commit_info(struct commit *commit,
> static void write_filename_info(const char *path)
> {
> printf("filename ");
> - write_name_quoted(NULL, 0, path, 1, stdout);
> - putchar('\n');
> + write_name_quoted(path, stdout, '\n');
> }
>
This looks like a candidate for a macro. I'm not sure if gcc optimizes
sibling calls in void functions with -O2, and it doesn't inline without
-O3.
>
> -static void diff_flush_raw(struct diff_filepair *p,
> - struct diff_options *options)
> +static void diff_flush_raw(struct diff_filepair *p, struct diff_options *opt)
Parameter rename? I'd have thought the patch was big enough as it is ;-)
Other than that, the diffstat calls this a good patch, and given the fact that
all your previous series passed all tests, I assume this one does too.
--
Andreas Ericsson andreas.ericsson@op5.se
OP5 AB www.op5.se
Tel: +46 8-230225 Fax: +46 8-230231
^ permalink raw reply
* Re: State of Perforce importing.
From: Simon Hausmann @ 2007-09-19 6:19 UTC (permalink / raw)
To: David Brown; +Cc: Git
In-Reply-To: <20070918233749.GA19533@old.davidb.org>
[-- Attachment #1: Type: text/plain, Size: 993 bytes --]
On Wednesday 19 September 2007 01:37:49 David Brown wrote:
> On Mon, Sep 17, 2007 at 12:30:28PM -0700, David Brown wrote:
> > I'd like to track a lot of code living in a Perforce repository, so I've
> > been playing with 'git-p4.py'. Is the one in the contrib/fast-import
> > directory the latest version, or is there a better place.
> >
> > So far, it is having a couple of problems:
> >
> > - The commit comment is empty. It doesn't seem to grab the Perforce
> > description, and the user seems to be <a@b>.
> >
> > - Every revision seems to check every file out of Perforce. This means
> > that for the directory I want, every revision is going to take about
> > 20 minutes.
>
> An additional problem:
>
> - git-p4 doesn't preserve the execute permission bit from Perforce.
Hmm, can you paste the output of
p4 fstat //path/in/depot/to/file/that/is/imported/incorrectly
? I'm interested in the type of the file that p4 reports.
FWIW it works for me ;-)
Thanks,
Simon
[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 189 bytes --]
^ permalink raw reply
* [PATCH] Don't create .git/branches
From: Pavel Roskin @ 2007-09-19 5:08 UTC (permalink / raw)
To: git
.git/branches was obsoleted by .git/remotes in August 2005, and the
later was obsoleted in December 2006. .git/branches was kept for
compatibility with Cogito. Since Cogito is not developed any longer,
the .git/branches directory is no longer useful, especially in newly
created repositories.
Signed-off-by: Pavel Roskin <proski@gnu.org>
---
templates/branches-- | 1 -
1 files changed, 0 insertions(+), 1 deletions(-)
delete mode 100644 templates/branches--
diff --git a/templates/branches-- b/templates/branches--
deleted file mode 100644
index fae8870..0000000
--- a/templates/branches--
+++ /dev/null
@@ -1 +0,0 @@
-: this is just to ensure the directory exists.
^ permalink raw reply related
* [PATCH 5/5] Fix memory leaks when disconnecting transport instances
From: Shawn O. Pearce @ 2007-09-19 4:49 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
Most transport implementations tend to allocate a data buffer
in the struct transport instance during transport_get() so we
need to free that data buffer when we disconnect it.
Signed-off-by: Shawn O. Pearce <spearce@spearce.org>
---
transport.c | 8 ++++++++
1 files changed, 8 insertions(+), 0 deletions(-)
diff --git a/transport.c b/transport.c
index a1d0a3c..4f9cddc 100644
--- a/transport.c
+++ b/transport.c
@@ -236,6 +236,7 @@ static int close_bundle(struct transport *transport)
struct bundle_transport_data *data = transport->data;
if (data->fd > 0)
close(data->fd);
+ free(data);
return 0;
}
@@ -372,6 +373,12 @@ static int git_transport_push(struct transport *transport, int refspec_nr, const
return !!err;
}
+static int disconnect_git(struct transport *transport)
+{
+ free(transport->data);
+ return 0;
+}
+
static int is_local(const char *url)
{
const char *colon = strchr(url, ':');
@@ -419,6 +426,7 @@ struct transport *transport_get(struct remote *remote, const char *url)
ret->get_refs_list = get_refs_via_connect;
ret->fetch = fetch_refs_via_pack;
ret->push = git_transport_push;
+ ret->disconnect = disconnect_git;
data->thin = 1;
data->uploadpack = "git-upload-pack";
--
1.5.3.1.195.gadd6
^ permalink raw reply related
* [PATCH 4/5] Ensure builtin-fetch honors {fetch,transfer}.unpackLimit
From: Shawn O. Pearce @ 2007-09-19 4:49 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
The only way to configure the unpacking limit is currently through
the .git/config (or ~/.gitconfig) mechanism as we have no existing
command line option interface to control this threshold on a per
invocation basis. This was intentional by design as the storage
policy of the repository should be a repository-wide decision and
should not be subject to variations made on individual command
executions.
Earlier builtin-fetch was bypassing the unpacking limit chosen by
the user through the configuration file as it did not reread the
configuration options through fetch_pack_config if we called the
internal fetch_pack() API directly. We now ensure we always run the
config file through fetch_pack_config at least once in this process,
thereby setting our unpackLimit properly.
Signed-off-by: Shawn O. Pearce <spearce@spearce.org>
---
builtin-fetch-pack.c | 19 +++++++++++++------
transport.c | 9 ---------
transport.h | 3 ---
3 files changed, 13 insertions(+), 18 deletions(-)
diff --git a/builtin-fetch-pack.c b/builtin-fetch-pack.c
index 77eb181..d128915 100644
--- a/builtin-fetch-pack.c
+++ b/builtin-fetch-pack.c
@@ -670,18 +670,24 @@ static int fetch_pack_config(const char *var, const char *value)
static struct lock_file lock;
-int cmd_fetch_pack(int argc, const char **argv, const char *prefix)
+static void fetch_pack_setup()
{
- int i, ret, nr_heads;
- struct ref *ref;
- char *dest = NULL, **heads;
-
+ static int did_setup;
+ if (did_setup)
+ return;
git_config(fetch_pack_config);
-
if (0 <= transfer_unpack_limit)
unpack_limit = transfer_unpack_limit;
else if (0 <= fetch_unpack_limit)
unpack_limit = fetch_unpack_limit;
+ did_setup = 1;
+}
+
+int cmd_fetch_pack(int argc, const char **argv, const char *prefix)
+{
+ int i, ret, nr_heads;
+ struct ref *ref;
+ char *dest = NULL, **heads;
nr_heads = 0;
heads = NULL;
@@ -760,6 +766,7 @@ struct ref *fetch_pack(struct fetch_pack_args *my_args,
struct ref *ref;
struct stat st;
+ fetch_pack_setup();
memcpy(&args, my_args, sizeof(args));
if (args.depth > 0) {
if (stat(git_path("shallow"), &st))
diff --git a/transport.c b/transport.c
index 85f5b1e..a1d0a3c 100644
--- a/transport.c
+++ b/transport.c
@@ -242,11 +242,7 @@ static int close_bundle(struct transport *transport)
struct git_transport_data {
unsigned thin : 1;
unsigned keep : 1;
-
- int unpacklimit;
-
int depth;
-
const char *uploadpack;
const char *receivepack;
};
@@ -267,9 +263,6 @@ static int set_git_option(struct transport *connection,
} else if (!strcmp(name, TRANS_OPT_KEEP)) {
data->keep = !!value;
return 0;
- } else if (!strcmp(name, TRANS_OPT_UNPACKLIMIT)) {
- data->unpacklimit = atoi(value);
- return 0;
} else if (!strcmp(name, TRANS_OPT_DEPTH)) {
if (!value)
data->depth = 0;
@@ -318,7 +311,6 @@ static int fetch_refs_via_pack(struct transport *transport,
args.uploadpack = data->uploadpack;
args.keep_pack = data->keep;
args.lock_pack = 1;
- args.unpacklimit = data->unpacklimit;
args.use_thin_pack = data->thin;
args.verbose = transport->verbose;
args.depth = data->depth;
@@ -435,7 +427,6 @@ struct transport *transport_get(struct remote *remote, const char *url)
data->receivepack = "git-receive-pack";
if (remote && remote->receivepack)
data->receivepack = remote->receivepack;
- data->unpacklimit = -1;
}
return ret;
diff --git a/transport.h b/transport.h
index 3e332ff..6e318e4 100644
--- a/transport.h
+++ b/transport.h
@@ -47,9 +47,6 @@ struct transport *transport_get(struct remote *, const char *);
/* Keep the pack that was transferred if not null */
#define TRANS_OPT_KEEP "keep"
-/* Unpack the objects if fewer than this number of objects are fetched */
-#define TRANS_OPT_UNPACKLIMIT "unpacklimit"
-
/* Limit the depth of the fetch if not null */
#define TRANS_OPT_DEPTH "depth"
--
1.5.3.1.195.gadd6
^ permalink raw reply related
* [PATCH 3/5] Always obtain fetch-pack arguments from struct fetch_pack_args
From: Shawn O. Pearce @ 2007-09-19 4:49 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
Copying the arguments from a fetch_pack_args into static globals
within the builtin-fetch-pack module is error-prone and may lead
rise to cases where arguments supplied via the struct from the
new fetch_pack() API may not be honored by the implementation.
Here we reorganize all of the static globals into a single static
struct fetch_pack_args instance and use memcpy() to move the data
from the caller supplied structure into the globals before we
execute our pack fetching implementation. This strategy is more
robust to additions and deletions of properties.
As keep_pack is a single bit we have also introduced lock_pack to
mean not only download and store the packfile via index-pack but
also to lock it against repacking by creating a .keep file when
the packfile itself is stored. The caller must remove the .keep
file when it is safe to do so.
Signed-off-by: Shawn O. Pearce <spearce@spearce.org>
---
builtin-fetch-pack.c | 111 +++++++++++++++++++++-----------------------------
fetch-pack.h | 9 +++-
transport.c | 9 +---
3 files changed, 55 insertions(+), 74 deletions(-)
diff --git a/builtin-fetch-pack.c b/builtin-fetch-pack.c
index 2977a94..77eb181 100644
--- a/builtin-fetch-pack.c
+++ b/builtin-fetch-pack.c
@@ -8,15 +8,11 @@
#include "sideband.h"
#include "fetch-pack.h"
-static int keep_pack;
static int transfer_unpack_limit = -1;
static int fetch_unpack_limit = -1;
static int unpack_limit = 100;
-static int quiet;
-static int verbose;
-static int fetch_all;
-static int depth;
-static int no_progress;
+static struct fetch_pack_args args;
+
static const char fetch_pack_usage[] =
"git-fetch-pack [--all] [--quiet|-q] [--keep|-k] [--thin] [--upload-pack=<git-upload-pack>] [--depth=<n>] [--no-progress] [-v] [<host>:]<directory> [<refs>...]";
static const char *uploadpack = "git-upload-pack";
@@ -181,7 +177,7 @@ static int find_common(int fd[2], unsigned char *result_sha1,
(use_sideband == 2 ? " side-band-64k" : ""),
(use_sideband == 1 ? " side-band" : ""),
(use_thin_pack ? " thin-pack" : ""),
- (no_progress ? " no-progress" : ""),
+ (args.no_progress ? " no-progress" : ""),
" ofs-delta");
else
packet_write(fd[1], "want %s\n", sha1_to_hex(remote));
@@ -189,13 +185,13 @@ static int find_common(int fd[2], unsigned char *result_sha1,
}
if (is_repository_shallow())
write_shallow_commits(fd[1], 1);
- if (depth > 0)
- packet_write(fd[1], "deepen %d", depth);
+ if (args.depth > 0)
+ packet_write(fd[1], "deepen %d", args.depth);
packet_flush(fd[1]);
if (!fetching)
return 1;
- if (depth > 0) {
+ if (args.depth > 0) {
char line[1024];
unsigned char sha1[20];
int len;
@@ -226,7 +222,7 @@ static int find_common(int fd[2], unsigned char *result_sha1,
retval = -1;
while ((sha1 = get_rev())) {
packet_write(fd[1], "have %s\n", sha1_to_hex(sha1));
- if (verbose)
+ if (args.verbose)
fprintf(stderr, "have %s\n", sha1_to_hex(sha1));
in_vain++;
if (!(31 & ++count)) {
@@ -244,7 +240,7 @@ static int find_common(int fd[2], unsigned char *result_sha1,
do {
ack = get_ack(fd[0], result_sha1);
- if (verbose && ack)
+ if (args.verbose && ack)
fprintf(stderr, "got ack %d %s\n", ack,
sha1_to_hex(result_sha1));
if (ack == 1) {
@@ -263,7 +259,7 @@ static int find_common(int fd[2], unsigned char *result_sha1,
} while (ack);
flushes--;
if (got_continue && MAX_IN_VAIN < in_vain) {
- if (verbose)
+ if (args.verbose)
fprintf(stderr, "giving up\n");
break; /* give up */
}
@@ -271,7 +267,7 @@ static int find_common(int fd[2], unsigned char *result_sha1,
}
done:
packet_write(fd[1], "done\n");
- if (verbose)
+ if (args.verbose)
fprintf(stderr, "done\n");
if (retval != 0) {
multi_ack = 0;
@@ -280,7 +276,7 @@ done:
while (flushes || multi_ack) {
int ack = get_ack(fd[0], result_sha1);
if (ack) {
- if (verbose)
+ if (args.verbose)
fprintf(stderr, "got ack (%d) %s\n", ack,
sha1_to_hex(result_sha1));
if (ack == 1)
@@ -317,7 +313,7 @@ static int mark_complete(const char *path, const unsigned char *sha1, int flag,
static void mark_recent_complete_commits(unsigned long cutoff)
{
while (complete && cutoff <= complete->item->date) {
- if (verbose)
+ if (args.verbose)
fprintf(stderr, "Marking %s as complete\n",
sha1_to_hex(complete->item->object.sha1));
pop_most_recent_commit(&complete, COMPLETE);
@@ -332,7 +328,7 @@ static void filter_refs(struct ref **refs, int nr_match, char **match)
struct ref *ref, *next;
struct ref *fastarray[32];
- if (nr_match && !fetch_all) {
+ if (nr_match && !args.fetch_all) {
if (ARRAY_SIZE(fastarray) < nr_match)
return_refs = xcalloc(nr_match, sizeof(struct ref *));
else {
@@ -348,8 +344,8 @@ static void filter_refs(struct ref **refs, int nr_match, char **match)
if (!memcmp(ref->name, "refs/", 5) &&
check_ref_format(ref->name + 5))
; /* trash */
- else if (fetch_all &&
- (!depth || prefixcmp(ref->name, "refs/tags/") )) {
+ else if (args.fetch_all &&
+ (!args.depth || prefixcmp(ref->name, "refs/tags/") )) {
*newtail = ref;
ref->next = NULL;
newtail = &ref->next;
@@ -365,7 +361,7 @@ static void filter_refs(struct ref **refs, int nr_match, char **match)
free(ref);
}
- if (!fetch_all) {
+ if (!args.fetch_all) {
int i;
for (i = 0; i < nr_match; i++) {
ref = return_refs[i];
@@ -408,7 +404,7 @@ static int everything_local(struct ref **refs, int nr_match, char **match)
}
}
- if (!depth) {
+ if (!args.depth) {
for_each_ref(mark_complete, NULL);
if (cutoff)
mark_recent_complete_commits(cutoff);
@@ -442,7 +438,7 @@ static int everything_local(struct ref **refs, int nr_match, char **match)
o = lookup_object(remote);
if (!o || !(o->flags & COMPLETE)) {
retval = 0;
- if (!verbose)
+ if (!args.verbose)
continue;
fprintf(stderr,
"want %s (%s)\n", sha1_to_hex(remote),
@@ -451,7 +447,7 @@ static int everything_local(struct ref **refs, int nr_match, char **match)
}
hashcpy(ref->new_sha1, local);
- if (!verbose)
+ if (!args.verbose)
continue;
fprintf(stderr,
"already have %s (%s)\n", sha1_to_hex(remote),
@@ -502,14 +498,14 @@ static int get_pack(int xd[2], char **pack_lockfile)
char keep_arg[256];
char hdr_arg[256];
const char **av;
- int do_keep = keep_pack;
+ int do_keep = args.keep_pack;
int keep_pipe[2];
side_pid = setup_sideband(fd, xd);
av = argv;
*hdr_arg = 0;
- if (unpack_limit) {
+ if (!args.keep_pack && unpack_limit) {
struct pack_header header;
if (read_pack_header(fd[0], &header))
@@ -527,11 +523,11 @@ static int get_pack(int xd[2], char **pack_lockfile)
die("fetch-pack: pipe setup failure: %s", strerror(errno));
*av++ = "index-pack";
*av++ = "--stdin";
- if (!quiet && !no_progress)
+ if (!args.quiet && !args.no_progress)
*av++ = "-v";
- if (use_thin_pack)
+ if (args.use_thin_pack)
*av++ = "--fix-thin";
- if (keep_pack > 1 || unpack_limit) {
+ if (args.lock_pack || unpack_limit) {
int s = sprintf(keep_arg,
"--keep=fetch-pack %d on ", getpid());
if (gethostname(keep_arg + s, sizeof(keep_arg) - s))
@@ -541,7 +537,7 @@ static int get_pack(int xd[2], char **pack_lockfile)
}
else {
*av++ = "unpack-objects";
- if (quiet)
+ if (args.quiet)
*av++ = "-q";
}
if (*hdr_arg)
@@ -599,17 +595,17 @@ static struct ref *do_fetch_pack(int fd[2],
if (is_repository_shallow() && !server_supports("shallow"))
die("Server does not support shallow clients");
if (server_supports("multi_ack")) {
- if (verbose)
+ if (args.verbose)
fprintf(stderr, "Server supports multi_ack\n");
multi_ack = 1;
}
if (server_supports("side-band-64k")) {
- if (verbose)
+ if (args.verbose)
fprintf(stderr, "Server supports side-band-64k\n");
use_sideband = 2;
}
else if (server_supports("side-band")) {
- if (verbose)
+ if (args.verbose)
fprintf(stderr, "Server supports side-band\n");
use_sideband = 1;
}
@@ -622,7 +618,7 @@ static struct ref *do_fetch_pack(int fd[2],
goto all_done;
}
if (find_common(fd, sha1, ref) < 0)
- if (keep_pack != 1)
+ if (!args.keep_pack)
/* When cloning, it is not unusual to have
* no common commit.
*/
@@ -674,22 +670,6 @@ static int fetch_pack_config(const char *var, const char *value)
static struct lock_file lock;
-void setup_fetch_pack(struct fetch_pack_args *args)
-{
- uploadpack = args->uploadpack;
- quiet = args->quiet;
- keep_pack = args->keep_pack;
- if (args->unpacklimit >= 0)
- unpack_limit = args->unpacklimit;
- if (args->keep_pack)
- unpack_limit = 0;
- use_thin_pack = args->use_thin_pack;
- fetch_all = args->fetch_all;
- verbose = args->verbose;
- depth = args->depth;
- no_progress = args->no_progress;
-}
-
int cmd_fetch_pack(int argc, const char **argv, const char *prefix)
{
int i, ret, nr_heads;
@@ -710,40 +690,40 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix)
if (*arg == '-') {
if (!prefixcmp(arg, "--upload-pack=")) {
- uploadpack = arg + 14;
+ args.uploadpack = arg + 14;
continue;
}
if (!prefixcmp(arg, "--exec=")) {
- uploadpack = arg + 7;
+ args.uploadpack = arg + 7;
continue;
}
if (!strcmp("--quiet", arg) || !strcmp("-q", arg)) {
- quiet = 1;
+ args.quiet = 1;
continue;
}
if (!strcmp("--keep", arg) || !strcmp("-k", arg)) {
- keep_pack++;
- unpack_limit = 0;
+ args.lock_pack = args.keep_pack;
+ args.keep_pack = 1;
continue;
}
if (!strcmp("--thin", arg)) {
- use_thin_pack = 1;
+ args.use_thin_pack = 1;
continue;
}
if (!strcmp("--all", arg)) {
- fetch_all = 1;
+ args.fetch_all = 1;
continue;
}
if (!strcmp("-v", arg)) {
- verbose = 1;
+ args.verbose = 1;
continue;
}
if (!prefixcmp(arg, "--depth=")) {
- depth = strtol(arg + 8, NULL, 0);
+ args.depth = strtol(arg + 8, NULL, 0);
continue;
}
if (!strcmp("--no-progress", arg)) {
- no_progress = 1;
+ args.no_progress = 1;
continue;
}
usage(fetch_pack_usage);
@@ -756,8 +736,7 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix)
if (!dest)
usage(fetch_pack_usage);
- ref = fetch_pack(dest, nr_heads, heads, NULL);
-
+ ref = fetch_pack(&args, dest, nr_heads, heads, NULL);
ret = !ref;
while (ref) {
@@ -769,7 +748,8 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix)
return ret;
}
-struct ref *fetch_pack(const char *dest,
+struct ref *fetch_pack(struct fetch_pack_args *my_args,
+ const char *dest,
int nr_heads,
char **heads,
char **pack_lockfile)
@@ -780,13 +760,14 @@ struct ref *fetch_pack(const char *dest,
struct ref *ref;
struct stat st;
- if (depth > 0) {
+ memcpy(&args, my_args, sizeof(args));
+ if (args.depth > 0) {
if (stat(git_path("shallow"), &st))
st.st_mtime = 0;
}
pid = git_connect(fd, (char *)dest, uploadpack,
- verbose ? CONNECT_VERBOSE : 0);
+ args.verbose ? CONNECT_VERBOSE : 0);
if (pid < 0)
return NULL;
if (heads && nr_heads)
@@ -809,7 +790,7 @@ struct ref *fetch_pack(const char *dest,
}
}
- if (!ret && depth > 0) {
+ if (!ret && args.depth > 0) {
struct cache_time mtime;
char *shallow = git_path("shallow");
int fd;
diff --git a/fetch-pack.h b/fetch-pack.h
index ad13076..a7888ea 100644
--- a/fetch-pack.h
+++ b/fetch-pack.h
@@ -8,14 +8,17 @@ struct fetch_pack_args
int depth;
unsigned quiet:1,
keep_pack:1,
+ lock_pack:1,
use_thin_pack:1,
fetch_all:1,
verbose:1,
no_progress:1;
};
-void setup_fetch_pack(struct fetch_pack_args *args);
-
-struct ref *fetch_pack(const char *dest, int nr_heads, char **heads, char **pack_lockfile);
+struct ref *fetch_pack(struct fetch_pack_args *args,
+ const char *dest,
+ int nr_heads,
+ char **heads,
+ char **pack_lockfile);
#endif
diff --git a/transport.c b/transport.c
index d8458dc..85f5b1e 100644
--- a/transport.c
+++ b/transport.c
@@ -314,21 +314,18 @@ static int fetch_refs_via_pack(struct transport *transport,
struct fetch_pack_args args;
int i;
+ memset(&args, 0, sizeof(args));
args.uploadpack = data->uploadpack;
- args.quiet = 0;
args.keep_pack = data->keep;
+ args.lock_pack = 1;
args.unpacklimit = data->unpacklimit;
args.use_thin_pack = data->thin;
- args.fetch_all = 0;
args.verbose = transport->verbose;
args.depth = data->depth;
- args.no_progress = 0;
-
- setup_fetch_pack(&args);
for (i = 0; i < nr_heads; i++)
origh[i] = heads[i] = xstrdup(to_fetch[i]->name);
- refs = fetch_pack(dest, nr_heads, heads, &transport->pack_lockfile);
+ refs = fetch_pack(&args, dest, nr_heads, heads, &transport->pack_lockfile);
for (i = 0; i < nr_heads; i++)
free(origh[i]);
--
1.5.3.1.195.gadd6
^ permalink raw reply related
* [PATCH 2/5] Refactor struct transport_ops inlined into struct transport
From: Shawn O. Pearce @ 2007-09-19 4:49 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
Aside from reducing the code by 20 lines this refactoring removes
a level of indirection when trying to access the operations of a
given transport "instance", making the code clearer and easier to
follow.
It also has the nice effect of giving us the benefits of C99 style
struct initialization (namely ".fetch = X") without requiring that
level of language support from our compiler. We don't need to worry
about new operation methods being added as they will now be NULL'd
out automatically by the xcalloc() we use to create the new struct
transport we supply to the caller.
This pattern already exists in struct walker, so we already have
a precedent for it in Git. We also don't really need to worry
about any sort of performance decreases that may occur as a result
of filling out 4-8 op pointers when we make a "struct transport".
The extra few CPU cycles this requires over filling in the "struct
transport_ops" is killed by the time it will take Git to actually
*use* one of those functions, as most transport operations are
going over the wire or will be copying object data locally between
two directories.
Signed-off-by: Shawn O. Pearce <spearce@spearce.org>
---
builtin-fetch.c | 3 +-
transport.c | 62 +++++++++++++++++++++---------------------------------
transport.h | 16 ++++---------
3 files changed, 30 insertions(+), 51 deletions(-)
diff --git a/builtin-fetch.c b/builtin-fetch.c
index 997a8ff..2f639cc 100644
--- a/builtin-fetch.c
+++ b/builtin-fetch.c
@@ -392,8 +392,7 @@ static int do_fetch(struct transport *transport,
if (transport->remote->fetch_tags == -1)
no_tags = 1;
- if (!transport->ops || !transport->ops->get_refs_list ||
- !transport->ops->fetch)
+ if (!transport->get_refs_list || !transport->fetch)
die("Don't know how to fetch from %s", transport->url);
/* if not appending, truncate FETCH_HEAD */
diff --git a/transport.c b/transport.c
index cc76e3f..d8458dc 100644
--- a/transport.c
+++ b/transport.c
@@ -44,8 +44,6 @@ static int disconnect_walker(struct transport *transport)
return 0;
}
-static const struct transport_ops rsync_transport;
-
static int curl_transport_push(struct transport *transport, int refspec_nr, const char **refspec, int flags) {
const char **argv;
int argc;
@@ -199,14 +197,6 @@ static int fetch_objs_via_curl(struct transport *transport,
#endif
-static const struct transport_ops curl_transport = {
- /* set_option */ NULL,
- /* get_refs_list */ get_refs_via_curl,
- /* fetch */ fetch_objs_via_curl,
- /* push */ curl_transport_push,
- /* disconnect */ disconnect_walker
-};
-
struct bundle_transport_data {
int fd;
struct bundle_header header;
@@ -249,14 +239,6 @@ static int close_bundle(struct transport *transport)
return 0;
}
-static const struct transport_ops bundle_transport = {
- /* set_option */ NULL,
- /* get_refs_list */ get_refs_from_bundle,
- /* fetch */ fetch_refs_from_bundle,
- /* push */ NULL,
- /* disconnect */ close_bundle
-};
-
struct git_transport_data {
unsigned thin : 1;
unsigned keep : 1;
@@ -401,13 +383,6 @@ static int git_transport_push(struct transport *transport, int refspec_nr, const
return !!err;
}
-static const struct transport_ops git_transport = {
- /* set_option */ set_git_option,
- /* get_refs_list */ get_refs_via_connect,
- /* fetch */ fetch_refs_via_pack,
- /* push */ git_transport_push
-};
-
static int is_local(const char *url)
{
const char *colon = strchr(url, ':');
@@ -431,18 +406,31 @@ struct transport *transport_get(struct remote *remote, const char *url)
ret->url = url;
if (!prefixcmp(url, "rsync://")) {
- ret->ops = &rsync_transport;
+ /* not supported; don't populate any ops */
+
} else if (!prefixcmp(url, "http://")
|| !prefixcmp(url, "https://")
|| !prefixcmp(url, "ftp://")) {
- ret->ops = &curl_transport;
+ ret->get_refs_list = get_refs_via_curl;
+ ret->fetch = fetch_objs_via_curl;
+ ret->push = curl_transport_push;
+ ret->disconnect = disconnect_walker;
+
} else if (is_local(url) && is_file(url)) {
struct bundle_transport_data *data = xcalloc(1, sizeof(*data));
ret->data = data;
- ret->ops = &bundle_transport;
+ ret->get_refs_list = get_refs_from_bundle;
+ ret->fetch = fetch_refs_from_bundle;
+ ret->disconnect = close_bundle;
+
} else {
struct git_transport_data *data = xcalloc(1, sizeof(*data));
ret->data = data;
+ ret->set_option = set_git_option;
+ ret->get_refs_list = get_refs_via_connect;
+ ret->fetch = fetch_refs_via_pack;
+ ret->push = git_transport_push;
+
data->thin = 1;
data->uploadpack = "git-upload-pack";
if (remote && remote->uploadpack)
@@ -451,7 +439,6 @@ struct transport *transport_get(struct remote *remote, const char *url)
if (remote && remote->receivepack)
data->receivepack = remote->receivepack;
data->unpacklimit = -1;
- ret->ops = &git_transport;
}
return ret;
@@ -460,24 +447,23 @@ struct transport *transport_get(struct remote *remote, const char *url)
int transport_set_option(struct transport *transport,
const char *name, const char *value)
{
- if (transport->ops->set_option)
- return transport->ops->set_option(transport, name, value);
+ if (transport->set_option)
+ return transport->set_option(transport, name, value);
return 1;
}
int transport_push(struct transport *transport,
int refspec_nr, const char **refspec, int flags)
{
- if (!transport->ops->push)
+ if (!transport->push)
return 1;
- return transport->ops->push(transport, refspec_nr, refspec, flags);
+ return transport->push(transport, refspec_nr, refspec, flags);
}
struct ref *transport_get_remote_refs(struct transport *transport)
{
if (!transport->remote_refs)
- transport->remote_refs =
- transport->ops->get_refs_list(transport);
+ transport->remote_refs = transport->get_refs_list(transport);
return transport->remote_refs;
}
@@ -496,7 +482,7 @@ int transport_fetch_refs(struct transport *transport, struct ref *refs)
heads[nr_heads++] = rm;
}
- rc = transport->ops->fetch(transport, nr_heads, heads);
+ rc = transport->fetch(transport, nr_heads, heads);
free(heads);
return rc;
}
@@ -513,8 +499,8 @@ void transport_unlock_pack(struct transport *transport)
int transport_disconnect(struct transport *transport)
{
int ret = 0;
- if (transport->ops->disconnect)
- ret = transport->ops->disconnect(transport);
+ if (transport->disconnect)
+ ret = transport->disconnect(transport);
free(transport);
return ret;
}
diff --git a/transport.h b/transport.h
index 6a95d66..3e332ff 100644
--- a/transport.h
+++ b/transport.h
@@ -5,22 +5,11 @@
#include "remote.h"
struct transport {
- unsigned verbose : 1;
struct remote *remote;
const char *url;
-
void *data;
-
struct ref *remote_refs;
- const struct transport_ops *ops;
- char *pack_lockfile;
-};
-
-#define TRANSPORT_PUSH_ALL 1
-#define TRANSPORT_PUSH_FORCE 2
-
-struct transport_ops {
/**
* Returns 0 if successful, positive if the option is not
* recognized or is inapplicable, and negative if the option
@@ -34,8 +23,13 @@ struct transport_ops {
int (*push)(struct transport *connection, int refspec_nr, const char **refspec, int flags);
int (*disconnect)(struct transport *connection);
+ char *pack_lockfile;
+ unsigned verbose : 1;
};
+#define TRANSPORT_PUSH_ALL 1
+#define TRANSPORT_PUSH_FORCE 2
+
/* Returns a transport suitable for the url */
struct transport *transport_get(struct remote *, const char *);
--
1.5.3.1.195.gadd6
^ permalink raw reply related
* [PATCH 1/5] Rename remote.uri to remote.url within remote handling internals
From: Shawn O. Pearce @ 2007-09-19 4:49 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
Anyplace we talk about the address of a remote repository we always
refer to it as a URL, especially in the configuration file and
.git/remotes where we call it "remote.$n.url" or start the first
line with "URL:". Calling this value a uri within the internal C
code just doesn't jive well with our commonly accepted terms.
Signed-off-by: Shawn O. Pearce <spearce@spearce.org>
---
builtin-fetch.c | 2 +-
builtin-push.c | 8 ++++----
remote.c | 34 +++++++++++++++++-----------------
remote.h | 6 +++---
send-pack.c | 2 +-
5 files changed, 26 insertions(+), 26 deletions(-)
diff --git a/builtin-fetch.c b/builtin-fetch.c
index b9722e5..997a8ff 100644
--- a/builtin-fetch.c
+++ b/builtin-fetch.c
@@ -530,7 +530,7 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)
else
remote = remote_get(argv[i++]);
- transport = transport_get(remote, remote->uri[0]);
+ transport = transport_get(remote, remote->url[0]);
if (verbose >= 2)
transport->verbose = 1;
if (quiet)
diff --git a/builtin-push.c b/builtin-push.c
index 7d7e826..4ee36c2 100644
--- a/builtin-push.c
+++ b/builtin-push.c
@@ -57,9 +57,9 @@ static int do_push(const char *repo, int flags)
refspec_nr = remote->push_refspec_nr;
}
errs = 0;
- for (i = 0; i < remote->uri_nr; i++) {
+ for (i = 0; i < remote->url_nr; i++) {
struct transport *transport =
- transport_get(remote, remote->uri[i]);
+ transport_get(remote, remote->url[i]);
int err;
if (receivepack)
transport_set_option(transport,
@@ -68,14 +68,14 @@ static int do_push(const char *repo, int flags)
transport_set_option(transport, TRANS_OPT_THIN, "yes");
if (verbose)
- fprintf(stderr, "Pushing to %s\n", remote->uri[i]);
+ fprintf(stderr, "Pushing to %s\n", remote->url[i]);
err = transport_push(transport, refspec_nr, refspec, flags);
err |= transport_disconnect(transport);
if (!err)
continue;
- error("failed to push to '%s'", remote->uri[i]);
+ error("failed to push to '%s'", remote->url[i]);
errs++;
}
return !!errs;
diff --git a/remote.c b/remote.c
index 31e2b70..e3c3df5 100644
--- a/remote.c
+++ b/remote.c
@@ -32,13 +32,13 @@ static void add_fetch_refspec(struct remote *remote, const char *ref)
remote->fetch_refspec_nr = nr;
}
-static void add_uri(struct remote *remote, const char *uri)
+static void add_url(struct remote *remote, const char *url)
{
- int nr = remote->uri_nr + 1;
- remote->uri =
- xrealloc(remote->uri, nr * sizeof(char *));
- remote->uri[nr-1] = uri;
- remote->uri_nr = nr;
+ int nr = remote->url_nr + 1;
+ remote->url =
+ xrealloc(remote->url, nr * sizeof(char *));
+ remote->url[nr-1] = url;
+ remote->url_nr = nr;
}
static struct remote *make_remote(const char *name, int len)
@@ -154,7 +154,7 @@ static void read_remotes_file(struct remote *remote)
switch (value_list) {
case 0:
- add_uri(remote, xstrdup(s));
+ add_url(remote, xstrdup(s));
break;
case 1:
add_push_refspec(remote, xstrdup(s));
@@ -206,7 +206,7 @@ static void read_branches_file(struct remote *remote)
} else {
branch = "refs/heads/master";
}
- add_uri(remote, p);
+ add_url(remote, p);
add_fetch_refspec(remote, branch);
remote->fetch_tags = 1; /* always auto-follow */
}
@@ -260,7 +260,7 @@ static int handle_config(const char *key, const char *value)
return 0; /* ignore unknown booleans */
}
if (!strcmp(subkey, ".url")) {
- add_uri(remote, xstrdup(value));
+ add_url(remote, xstrdup(value));
} else if (!strcmp(subkey, ".push")) {
add_push_refspec(remote, xstrdup(value));
} else if (!strcmp(subkey, ".fetch")) {
@@ -347,14 +347,14 @@ struct remote *remote_get(const char *name)
name = default_remote_name;
ret = make_remote(name, 0);
if (name[0] != '/') {
- if (!ret->uri)
+ if (!ret->url)
read_remotes_file(ret);
- if (!ret->uri)
+ if (!ret->url)
read_branches_file(ret);
}
- if (!ret->uri)
- add_uri(ret, name);
- if (!ret->uri)
+ if (!ret->url)
+ add_url(ret, name);
+ if (!ret->url)
return NULL;
ret->fetch = parse_ref_spec(ret->fetch_refspec_nr, ret->fetch_refspec);
ret->push = parse_ref_spec(ret->push_refspec_nr, ret->push_refspec);
@@ -380,11 +380,11 @@ int for_each_remote(each_remote_fn fn, void *priv)
return result;
}
-int remote_has_uri(struct remote *remote, const char *uri)
+int remote_has_url(struct remote *remote, const char *url)
{
int i;
- for (i = 0; i < remote->uri_nr; i++) {
- if (!strcmp(remote->uri[i], uri))
+ for (i = 0; i < remote->url_nr; i++) {
+ if (!strcmp(remote->url[i], url))
return 1;
}
return 0;
diff --git a/remote.h b/remote.h
index b5b558f..05add06 100644
--- a/remote.h
+++ b/remote.h
@@ -4,8 +4,8 @@
struct remote {
const char *name;
- const char **uri;
- int uri_nr;
+ const char **url;
+ int url_nr;
const char **push_refspec;
struct refspec *push;
@@ -32,7 +32,7 @@ struct remote *remote_get(const char *name);
typedef int each_remote_fn(struct remote *remote, void *priv);
int for_each_remote(each_remote_fn fn, void *priv);
-int remote_has_uri(struct remote *remote, const char *uri);
+int remote_has_url(struct remote *remote, const char *url);
struct refspec {
unsigned force : 1;
diff --git a/send-pack.c b/send-pack.c
index f74e66a..4533d1b 100644
--- a/send-pack.c
+++ b/send-pack.c
@@ -420,7 +420,7 @@ int main(int argc, char **argv)
if (remote_name) {
remote = remote_get(remote_name);
- if (!remote_has_uri(remote, dest)) {
+ if (!remote_has_url(remote, dest)) {
die("Destination %s is not a uri for %s",
dest, remote_name);
}
--
1.5.3.1.195.gadd6
^ permalink raw reply related
* [PATCH 0/5] Yet another builtin-fetch round
From: Shawn O. Pearce @ 2007-09-19 4:49 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
Another short series for db/fetch-pack, still in pu. Aside from
optimizing the pipeline on the native transport (so we only invoke
the remote process we need once vs. twice) I'm actually now quite
comfortable with this whole series and think it is ready for next.
I'm certainly running it in production, and will be until it is
merged. The performance difference is too big for me (and at least
some of my coworkers) to not be doing so. If there are any specific
reasons why this topic is not ready for next or is unsuitable for
merging please let me know so I can take the time to correct it.
1/5 Rename remote.uri to remote.url within remote handling internals
2/5 Refactor struct transport_ops inlined into struct transport
3/5 Always obtain fetch-pack arguments from struct fetch_pack_args
These three are basic code cleanups for small issues that
bothered me about the original implementation of builtin-fetch.
Now is just as good of a time as any to cleanup the code and make
it more maintainable. I think the overall total line count is
reduced by these three patches.
* Ensure builtin-fetch honors {fetch,transfer}.unpackLimit
* Fix memory leaks when disconnecting transport instances
Fixes two known (but minor) outstanding bugs. At this point
I do not know of any other bugs in builtin-fetch so I would
really appreciate testing reports from other people, especially
those whose uses cases might stray outside of my workflow. Hah,
I did not tell you my workflow. ;-)
--
Shawn.
^ permalink raw reply
* Re: [PATCH] instaweb: added support Ruby's WEBrick server
From: Johannes Schindelin @ 2007-09-19 1:27 UTC (permalink / raw)
To: Junio C Hamano; +Cc: mike dalessio, git, normalperson
In-Reply-To: <7vodfztviv.fsf@gitster.siamese.dyndns.org>
Hi,
On Tue, 18 Sep 2007, Junio C Hamano wrote:
> I wonder how popular instaweb is and how widely it is used. I've
> actually wondering if we should demote it to contrib/ somewhere, but it
> gets occasional updates so people must be using it...
I have to admit that I found it easier to install gitweb manually,
especially with recent documentation enhancements, _and_ the constraint
that I had to use an existing DocumentRoot.
So I would not be opposed to move this into contrib/.
Ciao,
Dscho
^ permalink raw reply
* Re: [PATCH 4/5] Full rework of quote_c_style and write_name_quoted.
From: Pierre Habouzit @ 2007-09-19 1:14 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <7vabrjtqw7.fsf@gitster.siamese.dyndns.org>
[-- Attachment #1: Type: text/plain, Size: 846 bytes --]
On Wed, Sep 19, 2007 at 12:55:52AM +0000, Junio C Hamano wrote:
> Pierre Habouzit <madcoder@debian.org> writes:
>
> > On Tue, Sep 18, 2007 at 10:00:51PM +0000, Pierre Habouzit wrote:
> >> + cp = strchr(qname.buf + qname.len + 3 - max, '/');
> >> + if (cp)
> >> + cp = qname.buf + qname.len + 3 - max;
> >
> > OMG, this is supposed to be if (!cp) of course...
> >
> > I wonder how this passed the testsuite.
>
> You would need a new test, I guess, before a huge rewrite.
OTOH this is in the code that generates the diffstats, it's not _that_
surprising that we don't have extensive tests about that, as it's not
critical in git afaict ;)
--
·O· Pierre Habouzit
··O madcoder@debian.org
OOO http://www.madism.org
[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]
^ 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