* Re: [PATCH 1/5] strbuf API additions and enhancements.
From: Pierre Habouzit @ 2007-09-19 13:36 UTC (permalink / raw)
To: Edgar Toernig; +Cc: Junio C Hamano, Shawn O. Pearce, git
In-Reply-To: <20070919144604.7deca4f7.froese@gmx.de>
[-- Attachment #1: Type: text/plain, Size: 1603 bytes --]
On Wed, Sep 19, 2007 at 12:46:04PM +0000, Edgar Toernig wrote:
> Pierre Habouzit wrote:
> >
> > +void strbuf_addvf(struct strbuf *sb, const char *fmt, va_list ap)
> > +{
> > + int len;
> > +
> > + len = vsnprintf(sb->buf + sb->len, sb->alloc - sb->len, fmt, ap);
> > + if (len < 0) {
> > + len = 0;
> > + }
> > + if (len > strbuf_avail(sb)) {
> > + strbuf_grow(sb, len);
> > + len = vsnprintf(sb->buf + sb->len, sb->alloc - sb->len, fmt, ap);
> > + if (len > strbuf_avail(sb)) {
> > + die("this should not happen, your snprintf is broken");
> > + }
> > + }
> > + strbuf_setlen(sb, sb->len + len);
> > +}
>
> The second vsnprintf won't work as the first one consumed all args
> from va_list ap. You need to va_copy the ap. But iirc va_copy poses
> compatibility issues. Unless va_copy is made available somehow,
> I would suggest to let the caller know that the buffer was too small
> (but isn't any more) and it has to call the function again:
That's what I thought, and then nfvasprintf in trace.c suffers from
the same issue, as I copied the code from there.
> do {
> va_start(ap, fmt);
> again = strbuf_addvf(sb, fmt, ap);
> va_end(ap);
> } while (again);
in fact doing it twice is enough but either way I don't like to impose
that to the caller :/ I mean it's totally stupid to have to do that on a
strbuf. of course we could provide a macro doing that ...
--
·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] Refactor struct transport_ops inlined into struct transport
From: Johannes Schindelin @ 2007-09-19 13:11 UTC (permalink / raw)
To: Shawn O. Pearce; +Cc: Junio C Hamano, git
In-Reply-To: <20070919044931.GB17107@spearce.org>
Hi,
On Wed, 19 Sep 2007, Shawn O. Pearce wrote:
> 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;
> @@ -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 */
> +
That is sneaky. What are the reasons to remove rsync support? I know it
is deprecated, but I'd still like to have it, especially for initial
clones on small-RAMed machines.
Ciao,
Dscho
^ permalink raw reply
* Re: Side-by-side diff and patch visualization
From: Andy Parkins @ 2007-09-19 13:08 UTC (permalink / raw)
To: git; +Cc: Jeff King, Wincent Colaiuta
In-Reply-To: <20070919120956.GA20715@coredump.intra.peff.net>
On Wednesday 2007 September 19, Jeff King wrote:
> Have you tried kompare?
>
> git-diff HEAD~5 | kompare -
You can also throw in the "--unified" switch to see the whole file, rather
than just the bits that have changed (if that's what you like).
git-diff --unified=9999999 HEAD~5 | kompare -
Andy
--
Dr Andy Parkins, M Eng (hons), MIET
andyparkins@gmail.com
^ permalink raw reply
* Re: [PATCH 1/5] strbuf API additions and enhancements.
From: Edgar Toernig @ 2007-09-19 12:46 UTC (permalink / raw)
To: Pierre Habouzit; +Cc: Junio C Hamano, Shawn O. Pearce, git
In-Reply-To: <20070918224119.17650344AB3@madism.org>
Pierre Habouzit wrote:
>
> +void strbuf_addvf(struct strbuf *sb, const char *fmt, va_list ap)
> +{
> + int len;
> +
> + len = vsnprintf(sb->buf + sb->len, sb->alloc - sb->len, fmt, ap);
> + if (len < 0) {
> + len = 0;
> + }
> + if (len > strbuf_avail(sb)) {
> + strbuf_grow(sb, len);
> + len = vsnprintf(sb->buf + sb->len, sb->alloc - sb->len, fmt, ap);
> + if (len > strbuf_avail(sb)) {
> + die("this should not happen, your snprintf is broken");
> + }
> + }
> + strbuf_setlen(sb, sb->len + len);
> +}
The second vsnprintf won't work as the first one consumed all args
from va_list ap. You need to va_copy the ap. But iirc va_copy poses
compatibility issues. Unless va_copy is made available somehow,
I would suggest to let the caller know that the buffer was too small
(but isn't any more) and it has to call the function again:
int strbuf_addvf(struct strbuf *sb, const char *fmt, va_list ap)
{
int len;
len = vsnprintf(sb->buf + sb->len, sb->alloc - sb->len, fmt, ap);
if (len < 0)
return 0;
if (len > strbuf_avail(sb)) {
strbuf_grow(sb, len);
return -1;
}
strbuf_setlen(sb, sb->len + len);
return 0;
}
The caller:
do {
va_start(ap, fmt);
again = strbuf_addvf(sb, fmt, ap);
va_end(ap);
} while (again);
va_copy would be nicer though...
Ciao, ET.
^ permalink raw reply
* Re: Side-by-side diff and patch visualization
From: David Kastrup @ 2007-09-19 12:23 UTC (permalink / raw)
To: git
In-Reply-To: <A92611E8-1035-46A6-AFEF-9C8A6F93AFB1@wincent.com>
Wincent Colaiuta <win@wincent.com> writes:
> Does anybody know of any tools for doing side-by-side visualizations
> of diffs and patches which work well with Git?
>
> So, can anyone recommend a tool which can do this kind of side-by-
> side visualization and plays nicely with Git?
I use M-x ediff-revision RET in Emacs for this. There is also M-x
smerge-ediff RET for resolving a file with merge conflict markers, and
M-x ediff-merge-revisions-with-ancestor RET.
The side-by-side layout is chosen by typing | into the control window
for the merge.
Looking at a patch in respect of two versions can be done with M-x
ediff-patch-file RET.
--
David Kastrup
^ permalink raw reply
* Re: Side-by-side diff and patch visualization
From: Wincent Colaiuta @ 2007-09-19 12:36 UTC (permalink / raw)
To: Jeff King; +Cc: Git Mailing List
In-Reply-To: <20070919120956.GA20715@coredump.intra.peff.net>
El 19/9/2007, a las 14:09, Jeff King escribió:
> On Wed, Sep 19, 2007 at 01:40:17PM +0200, Wincent Colaiuta wrote:
>
>> Does anybody know of any tools for doing side-by-side
>> visualizations of
>> diffs and patches which work well with Git?
>
> Have you tried kompare?
>
> git-diff HEAD~5 | kompare -
>
> -Peff
Ah, didn't know about that one. From the look of it that is exactly
the kind of extremely simple viewer that I was wanting.
Cheers,
Wincent
^ permalink raw reply
* Re: Initial push to remote repository
From: Thomas Glanzmann @ 2007-09-19 12:27 UTC (permalink / raw)
To: Peter Baumann; +Cc: GIT
In-Reply-To: <20070919122558.GA4777@xp.machine.xx>
Hello Peter,
> Try to give it the full ref name and it should work, e.g.
> git push origin refs/heads/master:refs/heads/master
I owe you a beer or two.
(ad027088pc) [~/work/raid_ueberwachung] git push origin refs/heads/master:refs/heads/master
updating 'refs/heads/master'
from 0000000000000000000000000000000000000000
to aca655af03443ef8b4adea269b50471894686b14
Also local refs/remotes/origin/master
Generating pack...
Done counting 18 objects.
Deltifying 18 objects...
100% (18/18) done
Writing 18 objects...
100% (18/18) done
Total 18 (delta 6), reused 0 (delta 0)
refs/heads/master: 0000000000000000000000000000000000000000 -> aca655af03443ef8b4adea269b50471894686b14
Thomas
^ permalink raw reply
* Re: Initial push to remote repository
From: Peter Baumann @ 2007-09-19 12:25 UTC (permalink / raw)
To: Thomas Glanzmann; +Cc: GIT
In-Reply-To: <20070919113557.GA24674@cip.informatik.uni-erlangen.de>
On Wed, Sep 19, 2007 at 01:35:57PM +0200, Thomas Glanzmann wrote:
> Hello,
> I used to publish my local work on a machine with all my git
> repositories using the following commands:
>
> # Publish a local created repository to a remote repository.
> ssh 131.188.30.102 git --git-dir=/home/cip/adm/sithglan/work/repositories/private/astro.git init-db
> git remote add origin 131.188.30.102:/home/cip/adm/sithglan/work/repositories/private/astro.git
> git push origin master:master
> echo >> .git/config <<EOF
> [branch "master"]
> remote = origin
> merge = refs/heads/master
> EOF
> git pull
>
> But since my last update of git it doesn't seem to work that way anymore. Is
> there another way to do what I like to using the git way?
>
> (ad027088pc) [~] mkdir test_initial_push
> (ad027088pc) [~] cd !$
> (ad027088pc) [~/test_initial_push] touch a
> (ad027088pc) [~/test_initial_push] git init
> Initialized empty Git repository in .git/
> (ad027088pc) [~/test_initial_push] git add a
> (ad027088pc) [~/test_initial_push] git commit -m "whatever" a
> Created initial commit 0d408ed: whatever
> 0 files changed, 0 insertions(+), 0 deletions(-)
> create mode 100644 a
> (reverse-i-search)`':
> (ad027088pc) [~/test_initial_push] ssh 131.188.30.102 git --git-dir=/home/cip/adm/sithglan/work/repositories/private/test_initial_push.git init-db
> Initialized empty Git repository in /home/cip/adm/sithglan/work/repositories/private/test_initial_push.git/
> (ad027088pc) [~/test_initial_push] git remote add origin 131.188.30.102:/home/cip/adm/sithglan/work/repositories/private/test_initial_push.git/
> You have new mail in /home/adglth0/Maildir
> (ad027088pc) [~/test_initial_push] git push origin master:master
> error: dst refspec master does not match any existing ref on the remote and does not start with refs/.
> fatal: The remote end hung up unexpectedly
> error: failed to push to '131.188.30.102:/home/cip/adm/sithglan/work/repositories/private/test_initial_push.git/'
Try to give it the full ref name and it should work, e.g.
git push origin refs/heads/master:refs/heads/master
-siprbaum
^ permalink raw reply
* Re: Side-by-side diff and patch visualization
From: Jeff King @ 2007-09-19 12:09 UTC (permalink / raw)
To: Wincent Colaiuta; +Cc: Git Mailing List
In-Reply-To: <A92611E8-1035-46A6-AFEF-9C8A6F93AFB1@wincent.com>
On Wed, Sep 19, 2007 at 01:40:17PM +0200, Wincent Colaiuta wrote:
> Does anybody know of any tools for doing side-by-side visualizations of
> diffs and patches which work well with Git?
Have you tried kompare?
git-diff HEAD~5 | kompare -
-Peff
^ permalink raw reply
* Initial push to remote repository
From: Thomas Glanzmann @ 2007-09-19 11:35 UTC (permalink / raw)
To: GIT
Hello,
I used to publish my local work on a machine with all my git
repositories using the following commands:
# Publish a local created repository to a remote repository.
ssh 131.188.30.102 git --git-dir=/home/cip/adm/sithglan/work/repositories/private/astro.git init-db
git remote add origin 131.188.30.102:/home/cip/adm/sithglan/work/repositories/private/astro.git
git push origin master:master
echo >> .git/config <<EOF
[branch "master"]
remote = origin
merge = refs/heads/master
EOF
git pull
But since my last update of git it doesn't seem to work that way anymore. Is
there another way to do what I like to using the git way?
(ad027088pc) [~] mkdir test_initial_push
(ad027088pc) [~] cd !$
(ad027088pc) [~/test_initial_push] touch a
(ad027088pc) [~/test_initial_push] git init
Initialized empty Git repository in .git/
(ad027088pc) [~/test_initial_push] git add a
(ad027088pc) [~/test_initial_push] git commit -m "whatever" a
Created initial commit 0d408ed: whatever
0 files changed, 0 insertions(+), 0 deletions(-)
create mode 100644 a
(reverse-i-search)`':
(ad027088pc) [~/test_initial_push] ssh 131.188.30.102 git --git-dir=/home/cip/adm/sithglan/work/repositories/private/test_initial_push.git init-db
Initialized empty Git repository in /home/cip/adm/sithglan/work/repositories/private/test_initial_push.git/
(ad027088pc) [~/test_initial_push] git remote add origin 131.188.30.102:/home/cip/adm/sithglan/work/repositories/private/test_initial_push.git/
You have new mail in /home/adglth0/Maildir
(ad027088pc) [~/test_initial_push] git push origin master:master
error: dst refspec master does not match any existing ref on the remote and does not start with refs/.
fatal: The remote end hung up unexpectedly
error: failed to push to '131.188.30.102:/home/cip/adm/sithglan/work/repositories/private/test_initial_push.git/'
(ad027088pc) [~/test_initial_push] git version
git version 1.5.3.1
(ad027088pc) [~/test_initial_push] ssh 131.188.30.102 git version
git version 1.5.2.1
Thomas
^ permalink raw reply
* Side-by-side diff and patch visualization
From: Wincent Colaiuta @ 2007-09-19 11:40 UTC (permalink / raw)
To: Git Mailing List
Does anybody know of any tools for doing side-by-side visualizations
of diffs and patches which work well with Git?
By side-by-side I mean something like what's shown in this screen shot:
<http://wincent.com/images/side-by-side-diff.png>
For simple diffs there is little advantage here over a raw textual
diff, but as patch complexity and size increase the side-by-side diff
can sometimes prove itself to be more useful:
- totally flexible notion of context (you can scroll as far as you
want in either direction)
- this flexibility comes with the same rapid movement between changes
(ie. up/down cursor keys to jump between hunks no matter how far
apart they are)
- for complex diffs, truly comprehending the nature of the changes
may be easier in a side-by-side format
- as icing on cake, the implementation in the screenshot highlights
the removed/added portions within each line
Now, that screenshot is actually of Apple's FileMerge app on Mac OS X
and it would be fairly straightforward to write a dump wrapper script
that either just interpreted the output of git-diff, or could be used
in conjunction with GIT_EXTERNAL_DIFF, to feed files into opendiff
two at a time, but that wouldn't allow you to easily visualize diffs
which touch many paths.
So, can anyone recommend a tool which can do this kind of side-by-
side visualization and plays nicely with Git? Experience with
opendiff and git-mergetool has spoilt me here. For most cases I would
continue to use vanilla git-diff (or gitk), but when I see a change
that I'd like to visualize side-by-side I'd like to be able to use
this alternative diff viewer (or can gitk already do side-by-side
diffs and I just haven't seen how to turn them on?). Bonus points if
I could not only view existing commits, but proposed commits too (ie.
the ability to feed in a patch and see a side-by-side visualization
of what the diff would look like if applied).
If nothing exists I will look at quickly hacking something together
(most likely something that just reads the output of git-diff), but
it will be just that, a quick hack.
Cheers,
Wincent
^ permalink raw reply
* Re: [PATCH] Mention that 'push .. master' is in explicit form master:refs/heads/master
From: Jari Aalto @ 2007-09-19 10:37 UTC (permalink / raw)
To: git
In-Reply-To: <7vfy1bvgn1.fsf@gitster.siamese.dyndns.org>
* Tue 2007-09-18 Junio C Hamano <gitster AT pobox.com>
* Message-Id: 7vfy1bvgn1.fsf AT gitster.siamese.dyndns.org
>> Find a ref that matches `master` in the source repository
>> (most likely, it would find `refs/heads/master`), and update
>> the same ref (e.g. `refs/heads/master`) in `origin` repository
>> - with it.
>> + with it. The following would be exactly same command:
>> +
>> + git push origin master:refs/heads/master
>
> They _might_ be exactly the same.
>
> The reason people often explicitly write
>
> $ git push $URL refs/heads/master:refs/heads/master
>
> in their insns for newbies is because this form would not be
> affected by the random factors at $URL repository (or your
> repository) and will consistently get the same result.
>
> $ git push $URL foo
>
> may push branch head 'foo' or tag 'foo' depending on which one
> you have locally. Having both is not encouraged, but spelling
> the insn out explicitly as refs/heads/foo makes it clear the
> command is talking about the branch even when there is a tag
> with the same name.
Thank you, kindly broaden the current documentation to include this
explanation.
Jari
--
Welcome to FOSS revolution: we fix and modify until it shines
^ permalink raw reply
* Re: [PATCH 4/5] Full rework of quote_c_style and write_name_quoted.
From: Pierre Habouzit @ 2007-09-19 8:47 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <7v1wcvqcsg.fsf@gitster.siamese.dyndns.org>
[-- Attachment #1: Type: text/plain, Size: 2375 bytes --]
On Wed, Sep 19, 2007 at 08:28:47AM +0000, Junio C Hamano wrote:
> 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.
Indeed, in fact I didn't noticed that difference, I'll document that.
>
> > 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?
indeed.
> > + /* 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?
indeed again.
> > /*
> > * 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.
heh, well outfp is still here, but I'll fix the part about outbuf.
--
·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: David Kastrup @ 2007-09-19 8:38 UTC (permalink / raw)
To: git
In-Reply-To: <7vwsunoy3d.fsf@gitster.siamese.dyndns.org>
[Also posted to the list via gmane, so reply there if appropriate]
Junio C Hamano <gitster@pobox.com> writes:
> David Kastrup <dak@gnu.org> writes:
>>
>> Esc x diff-mode RET
>
> Be careful when you edit format-patch output. It seems that
> diff-mode tends to mistake the trailing "signature separator" at
> the end as if it is a removal of a line from the preimage, and
> editing the last hunk ends up miscalculating the number of lines
> in it. It might have been fixed in the latest version but I was
> burned by it number of times.
Would you be able to prepare an example and submit it using
M-x report-emacs-bug RET (probably using an attachment)?
Emacs 22.2 is likely to come out in a few months mainly as a bug fix,
incremental change and maintenance release to 22.1, and this would be
very much the kind of bug that warrants getting fixed, in particular
since it appears likely that Emacs 22.2 will come with git support in
VC.
If there is a good test case known to fail in your Emacs version, it
would be quite easy to verify that it is or gets fixed in 22.2
Thanks,
--
David Kastrup
^ permalink raw reply
* Re: [PATCH] Simplify strbuf uses in archive-tar.c using the proper functions.
From: Pierre Habouzit @ 2007-09-19 8:36 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <7v8x73qdtr.fsf@gitster.siamese.dyndns.org>
[-- Attachment #1: Type: text/plain, Size: 1624 bytes --]
On Wed, Sep 19, 2007 at 08:06:24AM +0000, Junio C Hamano wrote:
> Pierre Habouzit <madcoder@debian.org> writes:
> > + 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.
indeed.
> 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.
I'm a nitwit, the strbuf_reset() should be done _before_ the grow
indeed.
> > + 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);
yeah, I was about to propose the same, I'll add a patch in my current
series to fix that this way.
--
·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: Junio C Hamano @ 2007-09-19 8:31 UTC (permalink / raw)
To: David Kastrup; +Cc: git
In-Reply-To: <86r6kv2h64.fsf@lola.quinscape.zz>
David Kastrup <dak@gnu.org> writes:
> 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.
Be careful when you edit format-patch output. It seems that
diff-mode tends to mistake the trailing "signature separator" at
the end as if it is a removal of a line from the preimage, and
editing the last hunk ends up miscalculating the number of lines
in it. It might have been fixed in the latest version but I was
burned by it number of times.
^ permalink raw reply
* 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
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