* [PATCH] missing return
From: Jerald Fitzjerald @ 2007-05-30 12:39 UTC (permalink / raw)
To: git; +Cc: jfj
OK for mainline?
---
diff --git a/base85.c b/base85.c
--- a/base85.c
+++ b/base85.c
@@ -66,7 +66,7 @@ int decode_85(char *dst, const char *buf
*/
if (0x03030303 < acc ||
0xffffffff - de < (acc *= 85))
- error("invalid base85 sequence %.5s", buffer-5);
+ return error("invalid base85 sequence %.5s", buffer-5);
acc += de;
say1(" %08x", acc);
^ permalink raw reply
* Re: [PATCH 4/4] Don't link with libsocket in MINGW port
From: Nguyen Thai Ngoc Duy @ 2007-05-30 13:08 UTC (permalink / raw)
To: Johannes Sixt; +Cc: git
In-Reply-To: <465D6796.27B65043@eudaptics.com>
On 5/30/07, Johannes Sixt <J.Sixt@eudaptics.com> wrote:
> This looks wrong. NEEDS_SOCKET is set in the architecture sections of
> Makefile where necessary, but the MinGW section doesn't want it and
> doesn't set it. If ./configure sets it, then you should fix
> configure.ac, not work around in Makefile.
I did so because I didn't have SunOS to test so I'd rather not touch
the code detecting NEEDS_SOCKET in configure.ac. How about this patch:
diff --git a/configure.ac b/configure.ac
index 4a12cb0..0516fdb 100644
--- a/configure.ac
+++ b/configure.ac
@@ -173,6 +173,12 @@ test -n "$NEEDS_LIBICONV" && LIBS="$LIBS -liconv"
AC_CHECK_LIB([c], [socket],
[NEEDS_SOCKET=],
[NEEDS_SOCKET=YesPlease])
+if test "$NEEDS_SOCKET" = YesPlease; then # more check
+ AC_CHECK_LIB([socket], [socket],
+ [LIBSOCKET_HAS_SOCKET=],
+ [LIBSOCKET_HAS_SOCKET=1])
+ test -n "$LIBSOCKET_HAS_SOCKET" && NEEDS_SOCKET=
+fi
AC_SUBST(NEEDS_SOCKET)
test -n "$NEEDS_SOCKET" && LIBS="$LIBS -lsocket"
If someone has a SunOS, please test the above patch. Thanks
--
Duy
^ permalink raw reply related
* Re: [RFH] QGit: how to cram a patch in a crowded screen
From: Andy Parkins @ 2007-05-30 13:08 UTC (permalink / raw)
To: git; +Cc: Marco Costalba, Pavel Roskin
In-Reply-To: <e5bfff550705261344l604543e7h64c20c1fc0979f20@mail.gmail.com>
On Saturday 2007 May 26, Marco Costalba wrote:
I've been using the latest qgit for a couple of days. Thought I'd comment on
the changes.
> > All visually distinct to improve searching by eye (perhaps including
> > clear separators between files patched). Then the file list could
> > include a "<header>" psuedo-file that would jump back to the top of the
> > viewer.
>
> This seems really gitk like. Not that I don' t like it, but _if_ it's
> possible I would prefer something a little bit more original.
I thought of a reason why I like qgit's separation of log and patch. Some
revisions have very large patches. In those cases, it's nice to have the
choice not to pay for the expensive patch display when all you want is to
view the log message.
My vote is definitely for keeping separate log and patch viewer.
> Now user can toggle between revision commit message and patch content
> with the menu entry 'View->toggle message/diff', or with shortcut 'm'
> or also with mouse clicking on top right links 'Diff->' and 'Log->'.
I like the principle; but I'm not sure about the floating "Diff->"
and "Log->"; it seems to me that the GUI metaphor for exactly this sort of
thing is well established - the tabbed display. Why invent yet another way
of switching a window between different views?
> There is also a check box in 'Edit->settings' called 'Show always
> revision message as first' that, if set, let automatically to switch
> to revision message when browsing on a new revision.
I have no objection to this. I can imagine cases when both automatic-switch
and leave-it-as-it-is would be what I want - so it has to be an option.
> With this, browsing on a repo reading both log message and patch it's
> a matter of 2 clicks or 2 shortcuts per revision, one for changing
> revision (and read the log message) and another to view the patch
> content.
You don't mention it here, but the change I'm not keen on is the scroll-down
to switch feature. Often, I over-scroll to be sure that I have hit the
bottom of a scroll box. When I do that with the scroll-to-switch feature,
suddenly the view changes. I'm finding it quite disconcerting.
> Comments? like/dislike?
Apart from the minor niggles I've mentioned above; I do like it.
- It feels more stable - in my day-to-day usage, far less of the screen
changes, so you don't feel that you're jumping between different modes
- Being able to see the patch and the revision list at the same time is
very nice.
While we're on wishes, here's a couple:
- It's hard to spot the boundaries between files in the the patch viewer.
It would be great if the
diff --git a/vserver/Makefile b/vserver/Makefile
index 2581254..17d8934 100644
lines had a different coloured background.
- The margin at the front of the log message lines puts the header block and
log text at different horizontal positions on the screen. I think it would
be nicer if they were at the same position.
Maybe I'll scratch those itches myself; but thought I'd mention them in case
you get to them before me :-)
Andy
--
Dr Andy Parkins, M Eng (hons), MIET
andyparkins@gmail.com
^ permalink raw reply
* Re: [PATCH 0/4] Make MINGW port cross-compilable from Linux
From: Johannes Schindelin @ 2007-05-30 13:11 UTC (permalink / raw)
To: Johannes Sixt
Cc: Nguyá»…n Thái Ngá»c Duy,
git
In-Reply-To: <465D1EDD.658483B4@eudaptics.com>
[-- Attachment #1: Type: TEXT/PLAIN, Size: 521 bytes --]
Hi,
On Wed, 30 May 2007, Johannes Sixt wrote:
> "Nguyá»…n Thái Ngá»c Duy" wrote:
> >
> > The following patches allow to cross compile mingw port from Linux less painful.
>
> Thank you. Could you please push this to the mob branch?
>
> git push mob@repo.or.cz:/srv/git/git/mingw.git +mybranch:mob
>
> The series should be based on mingw.git's master branch (not devel)!
Is there any good reason why you do that in such a secretive manner? I
have not seen this series on the list, just your replies.
Ciao,
Dscho
^ permalink raw reply
* Re: [PATCH] cvsimport: add support for new style remote layout
From: James Bowes @ 2007-05-30 13:24 UTC (permalink / raw)
To: Andy Whitcroft; +Cc: git
In-Reply-To: <3a477a76f6c5396feee452678fb8110c@pinky>
On 5/29/07, Andy Whitcroft <apw@shadowen.org> wrote:
> Add a new option '-r <remote>' to set the remote name for
> this import. When this option is specified branches are named
> refs/remotes/<remote>/branch, with HEAD named as master matching
> git-clone separate remotes layout. Without branches are placed
> ion refs/heads, with HEAD named origin as before.
I love this. It's really nice to have 'git branch' not cluttered with
remote branches.
-James
^ permalink raw reply
* Re: [PATCH 0/4] Make MINGW port cross-compilable from Linux
From: Johannes Schindelin @ 2007-05-30 13:43 UTC (permalink / raw)
To: Nguyen Thai Ngoc Duy; +Cc: Johannes Sixt, git
In-Reply-To: <fcaeb9bf0705300618s712be8auf29ba7bd3579bd16@mail.gmail.com>
[-- Attachment #1: Type: TEXT/PLAIN, Size: 1017 bytes --]
Hi,
On Wed, 30 May 2007, Nguyen Thai Ngoc Duy wrote:
> On 5/30/07, Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
> >
> > On Wed, 30 May 2007, Johannes Sixt wrote:
> >
> > > "Nguyễn Thái Ngọc Duy" wrote:
> > > >
> > > > The following patches allow to cross compile mingw port from Linux less
> > painful.
> > >
> > > Thank you. Could you please push this to the mob branch?
> > >
> > > git push mob@repo.or.cz:/srv/git/git/mingw.git +mybranch:mob
> > >
> > > The series should be based on mingw.git's master branch (not devel)!
> >
> > Is there any good reason why you do that in such a secretive manner? I
> > have not seen this series on the list, just your replies.
>
> It's vger that should be blamed, not me :) I sent the series to
> Johannes Sixt and CC git@vger but they never came.
That explains it. I notice that your mail did not come through again...
Maybe vger should grow a facility where you can check _why_ a mail does
not go through. Sort of frustrating otherwise.
Ciao,
Dscho
^ permalink raw reply
* [MinGW port] Unable to repack on Clearcase dynamic views
From: Nguyen Thai Ngoc Duy @ 2007-05-30 14:42 UTC (permalink / raw)
To: Git Mailing List
Hi,
On clearcase dynamic views (kind of virtual drives), "mv -f a b" won't
work if "a" is read-only. Because git-repack.sh removes all write
permission before moving packs, it fails on clearcase dynamic views.
My approach is rather ugly. Does anyone have a better solution?
diff --git a/git-repack.sh b/git-repack.sh
index acb78ba..76a9525 100755
--- a/git-repack.sh
+++ b/git-repack.sh
@@ -86,10 +86,22 @@ else
mv -f "$PACKTMP-$name.idx" "$PACKDIR/pack-$name.idx" &&
test -f "$PACKDIR/pack-$name.pack" &&
test -f "$PACKDIR/pack-$name.idx" || {
- echo >&2 "Couldn't replace the existing pack with updated one."
- echo >&2 "The original set of packs have been saved as"
- echo >&2 "old-pack-$name.{pack,idx} in $PACKDIR."
- exit 1
+ # Clearcase dynamic views do not allow to move file without write permission
+ # Try the second time with write allowed
+ chmod u+w "$PACKTMP-$name.pack"
+ chmod u+w "$PACKTMP-$name.idx"
+
+ mv -f "$PACKTMP-$name.pack" "$PACKDIR/pack-$name.pack" &&
+ mv -f "$PACKTMP-$name.idx" "$PACKDIR/pack-$name.idx" &&
+ test -f "$PACKDIR/pack-$name.pack" &&
+ test -f "$PACKDIR/pack-$name.idx" || {
+ echo >&2 "Couldn't replace the existing pack with updated one."
+ echo >&2 "The original set of packs have been saved as"
+ echo >&2 "old-pack-$name.{pack,idx} in $PACKDIR."
+ exit 1
+ }
+ chmod a-w "$PACKDIR/pack-$name.pack"
+ chmod a-w "$PACKDIR/pack-$name.idx"
}
rm -f "$PACKDIR/old-pack-$name.pack" "$PACKDIR/old-pack-$name.idx"
fi
--
Duy
^ permalink raw reply related
* [PATCH] keep .mailmap sorted
From: Nicolas Pitre @ 2007-05-30 15:07 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
[-- Attachment #1: Type: TEXT/PLAIN, Size: 608 bytes --]
Signed-off-by: Nicolas Pitre <nico@cam.org>
---
diff --git a/.mailmap b/.mailmap
index 4e0615e..fb93758 100644
--- a/.mailmap
+++ b/.mailmap
@@ -7,6 +7,7 @@
Aneesh Kumar K.V <aneesh.kumar@gmail.com>
Chris Shoemaker <c.shoemaker@cox.net>
+Dana L. How <how@deathvalley.cswitch.com>
Daniel Barkalow <barkalow@iabervon.org>
David Kågedal <davidk@lysator.liu.se>
Fredrik Kuivinen <freku045@student.liu.se>
@@ -42,4 +43,3 @@ Ville Skyttä <scop@xemacs.org>
YOSHIFUJI Hideaki <yoshfuji@linux-ipv6.org>
anonymous <linux@horizon.com>
anonymous <linux@horizon.net>
-Dana L. How <how@deathvalley.cswitch.com>
^ permalink raw reply related
* Re: [MinGW port] Unable to repack on Clearcase dynamic views
From: Paolo Teti @ 2007-05-30 15:11 UTC (permalink / raw)
To: Nguyen Thai Ngoc Duy; +Cc: Git Mailing List
In-Reply-To: <fcaeb9bf0705300742u22b54c78vccbc037fb553141f@mail.gmail.com>
2007/5/30, Nguyen Thai Ngoc Duy <pclouds@gmail.com>:
> Hi,
>
> On clearcase dynamic views (kind of virtual drives), "mv -f a b" won't
> work if "a" is read-only. Because git-repack.sh removes all write
> permission before moving packs, it fails on clearcase dynamic views.
>
> My approach is rather ugly. Does anyone have a better solution?
I hate clearcase, but honestly I have used it forced by a customer..
The real problem is why you need to do a repack in a CC view, but..
to move a file in a Clearcase view you have to use 'cleartool mv ...'
(or 'cleartool relocate') and not the UNIX mv command...
..and he directory where the element to be moved or renamed resides
must be checked out.
^ permalink raw reply
* Re: [MinGW port] Unable to repack on Clearcase dynamic views
From: Johannes Sixt @ 2007-05-30 15:28 UTC (permalink / raw)
To: git
In-Reply-To: <fcaeb9bf0705300742u22b54c78vccbc037fb553141f@mail.gmail.com>
Nguyen Thai Ngoc Duy wrote:
> On clearcase dynamic views (kind of virtual drives), "mv -f a b" won't
> work if "a" is read-only. Because git-repack.sh removes all write
> permission before moving packs, it fails on clearcase dynamic views.
>
> My approach is rather ugly. Does anyone have a better solution?
If you do two 'git repack -a -d' in a row, the second one fails even
with this patch, right? To fix it, you must 'chmod u+w' in the for-loop
right above this hunk, too.
Since you 'chmod a-w' after the mv, why not just remove the first ones
before the first mv? Then you can get rid of the second try.
-- Hannes
>
> diff --git a/git-repack.sh b/git-repack.sh
> index acb78ba..76a9525 100755
> --- a/git-repack.sh
> +++ b/git-repack.sh
> @@ -86,10 +86,22 @@ else
> mv -f "$PACKTMP-$name.idx" "$PACKDIR/pack-$name.idx" &&
> test -f "$PACKDIR/pack-$name.pack" &&
> test -f "$PACKDIR/pack-$name.idx" || {
> - echo >&2 "Couldn't replace the existing pack with updated one."
> - echo >&2 "The original set of packs have been saved as"
> - echo >&2 "old-pack-$name.{pack,idx} in $PACKDIR."
> - exit 1
> + # Clearcase dynamic views do not allow to move file without write permission
> + # Try the second time with write allowed
> + chmod u+w "$PACKTMP-$name.pack"
> + chmod u+w "$PACKTMP-$name.idx"
> +
> + mv -f "$PACKTMP-$name.pack" "$PACKDIR/pack-$name.pack" &&
> + mv -f "$PACKTMP-$name.idx" "$PACKDIR/pack-$name.idx" &&
> + test -f "$PACKDIR/pack-$name.pack" &&
> + test -f "$PACKDIR/pack-$name.idx" || {
> + echo >&2 "Couldn't replace the existing pack with updated one."
> + echo >&2 "The original set of packs have been saved as"
> + echo >&2 "old-pack-$name.{pack,idx} in $PACKDIR."
> + exit 1
> + }
> + chmod a-w "$PACKDIR/pack-$name.pack"
> + chmod a-w "$PACKDIR/pack-$name.idx"
> }
> rm -f "$PACKDIR/old-pack-$name.pack" "$PACKDIR/old-pack-$name.idx"
> fi
>
> --
> Duy
^ permalink raw reply
* Re: [MinGW port] Unable to repack on Clearcase dynamic views
From: Nguyen Thai Ngoc Duy @ 2007-05-30 15:33 UTC (permalink / raw)
To: Paolo Teti; +Cc: Git Mailing List, Matthias Lederhofer, Junio C Hamano
In-Reply-To: <34a7ae040705300811o22a7d9e9y1ca6e52c4bf23fcc@mail.gmail.com>
On 5/30/07, Paolo Teti <paolo.teti@gmail.com> wrote:
> 2007/5/30, Nguyen Thai Ngoc Duy <pclouds@gmail.com>:
> > Hi,
> >
> > On clearcase dynamic views (kind of virtual drives), "mv -f a b" won't
> > work if "a" is read-only. Because git-repack.sh removes all write
> > permission before moving packs, it fails on clearcase dynamic views.
> >
> > My approach is rather ugly. Does anyone have a better solution?
>
> I hate clearcase, but honestly I have used it forced by a customer..
> The real problem is why you need to do a repack in a CC view, but..
I don't like it either :) But you remind me the Matthias' workdir
patchset. With that I can move out .git to a real drive/filesystem
(which should also give better performance).
I think it's a valid use case. Anyone remember why Matthias' patchset
was dropped?
It was last mentioned in
http://article.gmane.org/gmane.comp.version-control.git/43041
Junio, Matthias? May I help?
>
> to move a file in a Clearcase view you have to use 'cleartool mv ...'
> (or 'cleartool relocate') and not the UNIX mv command...
'cleartool mv' is used for clearcase-manages files. My git repository
are private files.
>
> ..and he directory where the element to be moved or renamed resides
> must be checked out.
>
--
Duy
^ permalink raw reply
* Re: [PATCH] Don't ignore write failure from git-diff, git-log, etc.
From: Linus Torvalds @ 2007-05-30 15:40 UTC (permalink / raw)
To: Jim Meyering; +Cc: git
In-Reply-To: <87k5uqqz0y.fsf@rho.meyering.net>
On Wed, 30 May 2007, Jim Meyering wrote:
>
> No. I'm glad to see that perhaps even you are surprised by your words.
If you thought I was a polite person and am surpised by calling things
crap after the fact, I'm afraid you have a few shocking moments coming to
you. My reply was fairly polite by my standards. My tag-line is often "On
the internet, nobody can hear you being subtle".
So let me rephrase my message to you:
I think your INTERMEDIATE PATCH WAS TOTAL AND UTTER CRAP.
EVERY SINGLE HUNK WAS SH*T.
You expressly IGNORED my point that some errors aren't errors, and
MADE GIT WORSE.
Are we on the same page now?
In contrast, your final patch was fine. The one where you finally fixed
the issue that I complained about FROM THE VERY BEGINNING.
Comprende?
> The only editing was to capitalize WHOLE. Here's what you wrote:
>
> > I think this patch is fundamentally WRONG. This fragment is just a prime
> > example of why the whole patch is crap. The old code was correct, and you
> > broke it.
>
> Umm... are the above three lines the only part of my message you're
> prepared to talk about? You haven't addressed any of the interesting
> (technical) parts.
Umm. Your final patch was a few trivial lines. I addressed all interesting
technical parts IN MY ORIGINAL REPLY WHEN YOU FIRST POSTED IT.
Which you ignored (or rather, explicitly chose to disagree with, and
added MORE crap to the patch).
Go away. I'm not interested in flaming you any more. The patch wasn't that
interesting to begin with, and you have shown that you're more interested
in being contrary than to actually fix the problems that were pointed out
to you _immediately_ and without any flames.
Linus
^ permalink raw reply
* Fwd: [MinGW port] Unable to repack on Clearcase dynamic views
From: Nguyen Thai Ngoc Duy @ 2007-05-30 15:43 UTC (permalink / raw)
To: Git Mailing List
In-Reply-To: <fcaeb9bf0705300840w221c46c1y742388fc9e61c2fa@mail.gmail.com>
I'm sorry, forgot to CC the list.
---------- Forwarded message ----------
From: Nguyen Thai Ngoc Duy <pclouds@gmail.com>
Date: May 30, 2007 11:40 AM
Subject: Re: [MinGW port] Unable to repack on Clearcase dynamic views
To: Johannes Sixt <J.Sixt@eudaptics.com>
On 5/30/07, Johannes Sixt <J.Sixt@eudaptics.com> wrote:
> Nguyen Thai Ngoc Duy wrote:
> > On clearcase dynamic views (kind of virtual drives), "mv -f a b" won't
> > work if "a" is read-only. Because git-repack.sh removes all write
> > permission before moving packs, it fails on clearcase dynamic views.
> >
> > My approach is rather ugly. Does anyone have a better solution?
>
> If you do two 'git repack -a -d' in a row, the second one fails even
> with this patch, right? To fix it, you must 'chmod u+w' in the for-loop
> right above this hunk, too.
Well, I'm not sure I understand it. git-repack with the patch applied
runs fine for me. Can you explain it more detail please?
>
> Since you 'chmod a-w' after the mv, why not just remove the first ones
> before the first mv? Then you can get rid of the second try.
Because I'm not sure what that command is for :) I suspect that packs
(or anything inside .git/objects) are read-only and the author want to
make sure no one can write to the packs as soon as they are created.
The other way to do is to move 'chmod a-w' down several lines -- we
only make those files read-only after moving them to
.git/objects/packs.
>
> -- Hannes
>
> >
> > diff --git a/git-repack.sh b/git-repack.sh
> > index acb78ba..76a9525 100755
> > --- a/git-repack.sh
> > +++ b/git-repack.sh
> > @@ -86,10 +86,22 @@ else
> > mv -f "$PACKTMP-$name.idx" "$PACKDIR/pack-$name.idx" &&
> > test -f "$PACKDIR/pack-$name.pack" &&
> > test -f "$PACKDIR/pack-$name.idx" || {
> > - echo >&2 "Couldn't replace the existing pack with updated one."
> > - echo >&2 "The original set of packs have been saved as"
> > - echo >&2 "old-pack-$name.{pack,idx} in $PACKDIR."
> > - exit 1
> > + # Clearcase dynamic views do not allow to move file without write permission
> > + # Try the second time with write allowed
> > + chmod u+w "$PACKTMP-$name.pack"
> > + chmod u+w "$PACKTMP-$name.idx"
> > +
> > + mv -f "$PACKTMP-$name.pack" "$PACKDIR/pack-$name.pack" &&
> > + mv -f "$PACKTMP-$name.idx" "$PACKDIR/pack-$name.idx" &&
> > + test -f "$PACKDIR/pack-$name.pack" &&
> > + test -f "$PACKDIR/pack-$name.idx" || {
> > + echo >&2 "Couldn't replace the existing pack with updated one."
> > + echo >&2 "The original set of packs have been saved as"
> > + echo >&2 "old-pack-$name.{pack,idx} in $PACKDIR."
> > + exit 1
> > + }
> > + chmod a-w "$PACKDIR/pack-$name.pack"
> > + chmod a-w "$PACKDIR/pack-$name.idx"
> > }
> > rm -f "$PACKDIR/old-pack-$name.pack" "$PACKDIR/old-pack-$name.idx"
> > fi
> >
> > --
> > Duy
>
>
--
Duy
--
Duy
^ permalink raw reply
* Re: [PATCH] Don't ignore write failure from git-diff, git-log, etc.
From: Jim Meyering @ 2007-05-30 16:12 UTC (permalink / raw)
To: Linus Torvalds; +Cc: git
In-Reply-To: <alpine.LFD.0.98.0705300832410.26602@woody.linux-foundation.org>
Linus Torvalds <torvalds@linux-foundation.org> wrote:
[...big flame!...]
Wow! This is like the scene from the Wizard of Oz:
I've just raised the curtain and seen who's behind it.
Jim
^ permalink raw reply
* Re: [MinGW port] Unable to repack on Clearcase dynamic views
From: Paolo Teti @ 2007-05-30 16:30 UTC (permalink / raw)
To: Nguyen Thai Ngoc Duy
Cc: Git Mailing List, Matthias Lederhofer, Junio C Hamano
In-Reply-To: <fcaeb9bf0705300833t6f0639ddx2f507996e48d838c@mail.gmail.com>
2007/5/30, Nguyen Thai Ngoc Duy <pclouds@gmail.com>:
> On 5/30/07, Paolo Teti <paolo.teti@gmail.com> wrote:
> > to move a file in a Clearcase view you have to use 'cleartool mv ...'
> > (or 'cleartool relocate') and not the UNIX mv command...
>
> 'cleartool mv' is used for clearcase-manages files. My git repository
> are private files.
ok, but also cleartool mv works for private file..
Take a look at:
http://www-1.ibm.com/support/docview.wss?rs=984&context=SSSH27&context=SSCGQ7D&q1=mv+read-only+files&uid=swg1IC46584&loc=en_US&cs=utf-8&lang=en
should be a clearcase bug.
^ permalink raw reply
* [PATCH] fix signed range problems with hex conversions
From: Nicolas Pitre @ 2007-05-30 17:09 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
The get_sha1_hex() function is defined as accepting a char array.
Because the char type is signed by default on many architectures,
get_sha1_hex() can be passed a pointer to negative chars. This can
happen with user input containing chars with the top bit set.
Then those chars are passed to hexval() which is defined as accepting an
unsigned int value. Whenever a signed char is promoted to an int, the
promotion is always signed and then the result is stored in the unsigned
int variable. In the negative char case that means really large
unsigned int values will result, and then the hexval_table is happily
indexed with that value.
On 32-bit architectures the large int value will create a wrap-around
and a byte located somewhere before the hexval_table array in memory
will be fetched. Depending on that byte value a bogus SHA1 value could
be returned.
On 64-bit architectures the large int value will most probably cause a
segmentation fault.
This patch adds a range test to hexval() in order to prevent this. Also
let's index the hexval_table array directly in get_sha1_hex() using
explicitly unsigned chars to avoid the range test producing faster
code.
While at it, make hexval_table const.
Signed-off-by: Nicolas Pitre <nico@cam.org>
---
diff --git a/cache.h b/cache.h
index f675223..30fcaa9 100644
--- a/cache.h
+++ b/cache.h
@@ -359,10 +359,10 @@ extern void *map_sha1_file(const unsigned char *sha1, unsigned long *);
extern int has_pack_file(const unsigned char *sha1);
extern int has_pack_index(const unsigned char *sha1);
-extern signed char hexval_table[256];
+extern const signed char hexval_table[256];
static inline unsigned int hexval(unsigned int c)
{
- return hexval_table[c];
+ return (c & ~0xff) ? -1 : hexval_table[c];
}
/* Convert to/from hex/sha1 representation */
diff --git a/sha1_file.c b/sha1_file.c
index a3637d7..e10fb4b 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -33,7 +33,7 @@ const unsigned char null_sha1[20];
static unsigned int sha1_file_open_flag = O_NOATIME;
-signed char hexval_table[256] = {
+const signed char hexval_table[256] = {
-1, -1, -1, -1, -1, -1, -1, -1, /* 00-07 */
-1, -1, -1, -1, -1, -1, -1, -1, /* 08-0f */
-1, -1, -1, -1, -1, -1, -1, -1, /* 10-17 */
@@ -72,11 +72,12 @@ int get_sha1_hex(const char *hex, unsigned char *sha1)
{
int i;
for (i = 0; i < 20; i++) {
- unsigned int val = (hexval(hex[0]) << 4) | hexval(hex[1]);
+ unsigned char c0 = *hex++;
+ unsigned char c1 = *hex++;
+ unsigned int val = (hexval_table[c0] << 4) | hexval_table[c1];
if (val & ~0xff)
return -1;
*sha1++ = val;
- hex += 2;
}
return 0;
}
^ permalink raw reply related
* Re: [PATCH] missing return
From: Junio C Hamano @ 2007-05-30 17:24 UTC (permalink / raw)
To: Jerald Fitzjerald; +Cc: git
In-Reply-To: <465D709E.7040402@freemail.gr>
Jerald Fitzjerald <jfj@freemail.gr> writes:
> OK for mainline?
Sure, with a signoff please.
I realize that I've seen this pointed out by somebody already.
Thanks for reminder.
^ permalink raw reply
* Re: [PATCH] fix signed range problems with hex conversions
From: Linus Torvalds @ 2007-05-30 17:32 UTC (permalink / raw)
To: Nicolas Pitre; +Cc: Junio C Hamano, Git Mailing List
In-Reply-To: <alpine.LFD.0.99.0705301213450.11491@xanadu.home>
On Wed, 30 May 2007, Nicolas Pitre wrote:
>
> This patch adds a range test to hexval() in order to prevent this. Also
> let's index the hexval_table array directly in get_sha1_hex() using
> explicitly unsigned chars to avoid the range test producing faster
> code.
Please just make hexval() take a "unsigned char" instead, solving the
problem in a manner that makes it much easier for the compiler to realize
that it never needs to sign-extend the value or test the end result..
Ie I think your patch would be better off something like the following
instead (it would be a one-liner, but I agree that marking hexval_table
"const" is also a good idea).
With this, gcc can just generate:
movzbl (%rdi), %eax
movsbl hexval_table(%rax),%edx
movzbl 1(%rdi), %eax
movsbl hexval_table(%rax),%eax
sall $4, %edx
orl %eax, %edx
for the code to generate a byte from two hex characters.
Linus
----
diff --git a/cache.h b/cache.h
index ec85d93..0da7070 100644
--- a/cache.h
+++ b/cache.h
@@ -359,8 +359,8 @@ extern void *map_sha1_file(const unsigned char *sha1, unsigned long *);
extern int has_pack_file(const unsigned char *sha1);
extern int has_pack_index(const unsigned char *sha1);
-extern signed char hexval_table[256];
-static inline unsigned int hexval(unsigned int c)
+extern const signed char hexval_table[256];
+static inline unsigned int hexval(unsigned char c)
{
return hexval_table[c];
}
diff --git a/sha1_file.c b/sha1_file.c
index 12d2ef2..f959fe5 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -33,7 +33,7 @@ const unsigned char null_sha1[20];
static unsigned int sha1_file_open_flag = O_NOATIME;
-signed char hexval_table[256] = {
+const signed char hexval_table[256] = {
-1, -1, -1, -1, -1, -1, -1, -1, /* 00-07 */
-1, -1, -1, -1, -1, -1, -1, -1, /* 08-0f */
-1, -1, -1, -1, -1, -1, -1, -1, /* 10-17 */
^ permalink raw reply related
* Re: [PATCH] fix signed range problems with hex conversions
From: Linus Torvalds @ 2007-05-30 17:42 UTC (permalink / raw)
To: Nicolas Pitre; +Cc: Junio C Hamano, Git Mailing List
In-Reply-To: <alpine.LFD.0.98.0705301025170.26602@woody.linux-foundation.org>
On Wed, 30 May 2007, Linus Torvalds wrote:
>
> With this, gcc can just generate:
>
> movzbl (%rdi), %eax
> ...
Btw, in the kernel we have a rule for *.c -> *.s files exactly because
it's nice to be able to easily say "ok, what does that generate".
Here's a patch to add such a rule to git too, in case anybody is
interested. It makes it much simpler to just do
make sha1_file.s
and look at the compiler-generated output that way, rather than having to
fire up gdb on the resulting binary.
(Add -fverbose-asm or something if you want to, it can make the result
even more readable)
Linus
---
diff --git a/Makefile b/Makefile
index 7527734..7ffb803 100644
--- a/Makefile
+++ b/Makefile
@@ -846,6 +846,8 @@ git$X git.spec \
%.o: %.c GIT-CFLAGS
$(QUIET_CC)$(CC) -o $*.o -c $(ALL_CFLAGS) $<
+%.s: %.c GIT-CFLAGS
+ $(QUIET_CC)$(CC) -S $(ALL_CFLAGS) $<
%.o: %.S
$(QUIET_CC)$(CC) -o $*.o -c $(ALL_CFLAGS) $<
^ permalink raw reply related
* Re: [PATCH] Don't ignore write failure from git-diff, git-log, etc.
From: Junio C Hamano @ 2007-05-30 17:51 UTC (permalink / raw)
To: Jim Meyering; +Cc: git
In-Reply-To: <87myzmr152.fsf@rho.meyering.net>
Jim Meyering <jim@meyering.net> writes:
> Junio C Hamano <junkio@cox.net> wrote:
>> Jim Meyering <jim@meyering.net> writes:
>>
>>> Of course error messages are annoying when your short-pipe-read is
>>> _deliberate_ (tho, most real uses of git tools will actually get no
>>> message to be annoyed about[*]), but what if there really *is* a mistake?
>>> Try this:
>>>
>>> # You want to force git to ignore the error.
>>> $ trap '' PIPE; git-rev-list HEAD | sync
>>> $
>>
>> It is perfectly valid (although it is stupid) for a Porcelain
>> script to do this:
>>
>> latest_by_jim=$(git log --pretty=oneline --author='Jim' | head -n 1)
>> case "$latest_by_jim" in
>> '') echo "No commit by Jim" ;;
>> *) # do something interesting on the commit
>> ;;;
>> esac
>
> Hi Junio,
>
> The above snippet (prepending a single #!/bin/bash line) doesn't provoke
> an EPIPE diagnostic from my patched git. In fact, even if you're using
> an old, unpatched version of bash, it provokes *no* diagnostic at all.
> To provoke a diagnostic (from bash, not git), using old unpatched bash,
> you need a script doing output from a subshell, e.g.:
>
> #!/tmp/bash-3.0/bash
> for x in 1; do
> git-log
> done | head -1
I haven't thought it through, but isn't the above example only
talking about the "Broken pipe" message? Surely, you would get
that message from older Bash if you have a shell loop on the
upstream side of the pipe no matter what we (the command that is
run by the shell loop) do, and trap is needed to squelch it.
But I do not see how this pipeline, where git-rev-list produces
more than what fits in the in-kernel pipe buffer:
"git-rev-list a lot of data | head -n 1"
would not catch EPIPE and say "Broken Pipe" with your patch.
Especially if the downstream is sufficiently slow (say, replace
it with "(sleep 10 && head -n 1)", perhaps), wouldn't the
upstream produce enough without being read, gets stuck on write,
and when the downstream exits, it would notice its write(2)
failed with EPIPE, wouldn't it?
Maybe you are talking about your updated patch?
> ...[patch to make git/EPIPE exit nonzero, but with no diagnostic]
>
> Thank you for taking the time to reply and to come up with a compromise.
> At first I thought this would be a step in the right direction, but,
> now that I understand how infrequently EPIPE actually comes into play,
> I think it'd be better to avoid a half-measure fix, since that would
> just perpetuate the idea that EPIPE is worth handling specially.
After having read what Linus said about how the "fixed" one
would behave differently, depending on the amount of data we
produce before the consumer says "I've seen enough" and
depending on the amount of data that would fit in the in-kernel
pipe buffer, I no longer think the compromise patch you mention
above is improvement anymore.
^ permalink raw reply
* Re: [RFH] QGit: how to cram a patch in a crowded screen
From: Marco Costalba @ 2007-05-30 18:18 UTC (permalink / raw)
To: Andy Parkins; +Cc: git, Pavel Roskin
In-Reply-To: <200705301408.37929.andyparkins@gmail.com>
On 5/30/07, Andy Parkins <andyparkins@gmail.com> wrote:
> On Saturday 2007 May 26, Marco Costalba wrote:
>
> I've been using the latest qgit for a couple of days. Thought I'd comment on
> the changes.
>
Thanks!
>
> > Now user can toggle between revision commit message and patch content
> > with the menu entry 'View->toggle message/diff', or with shortcut 'm'
> > or also with mouse clicking on top right links 'Diff->' and 'Log->'.
>
> I like the principle; but I'm not sure about the floating "Diff->"
> and "Log->"; it seems to me that the GUI metaphor for exactly this sort of
> thing is well established - the tabbed display. Why invent yet another way
> of switching a window between different views?
>
This 'Diff->' and "Log"thing are going to change a lot this week-end!
My (crazy) idea is:
- Let switch from message to diff content scrolling down after the end
of message.
- Let switch from diff to message content scrolling up before the
start of diff content
- Let browse from one revision to the previous one (going down by one)
scrolling down after the end of diff content.
- Let browse from one revision to the next one (going up by one)
scrolling up before the start of message content.
So I imagine two labels for each content type:
- for message content a top right label called "Up" and one in bottom
right position called "Diff"
- for diff content a top right label called "Log" and one in bottom
right position called "Down"
I plan also to change the labels in something more intuitive with
scroll action, as example adding an up and down direction little
arrows next to them.
But that's not the end of the story: my mind went really mad while
thinking about this!
Because I normally browse new patches going from the oldest to the
earliest (going up), and for each patch I look before the message and
then (optionally) the content I 've wondered a nightmare like this:
Revision message pane:
-Up right position: one link label called "Up"
-Down right position: two links called "Diff"and (in small font) "Down"
Diff content pane:
-Up right position: two links called "Log"and (in small font) "Up"
-Down right position: two links called "Up"and (in small font) "Down"
Scrolling action defaults to bigger font label for either scroll direction.
It is possible to quickly switch each label couple: the big font label
became the smallest so to change scroll action in case of changing
repository browsing direction.
Directly clicking on each label of course produces the correspondent effect.
To much smoke??
>
> You don't mention it here, but the change I'm not keen on is the scroll-down
> to switch feature. Often, I over-scroll to be sure that I have hit the
> bottom of a scroll box. When I do that with the scroll-to-switch feature,
> suddenly the view changes. I'm finding it quite disconcerting.
>
That's a problem of finding a suitable filter. There is already one
implemented that let you switch only once, you can keep scrolling but
the content will not change a second time, you need to stop scrolling
for a while to "rearm" the scroll action.
> While we're on wishes, here's a couple:
> - It's hard to spot the boundaries between files in the the patch viewer.
> It would be great if the
> diff --git a/vserver/Makefile b/vserver/Makefile
> index 2581254..17d8934 100644
> lines had a different coloured background.
Yes. I agree. Thanks.
> - The margin at the front of the log message lines puts the header block and
> log text at different horizontal positions on the screen. I think it would
> be nicer if they were at the same position.
>
Yes.
> Maybe I'll scratch those itches myself; but thought I'd mention them in case
> you get to them before me :-)
>
>
Please, fell free to go head, I will be involved in implementing this
crappy scrolling thing toy this week end :-)
Thanks
Marco
P.S: In case someone wonders what's the goal of this label madness. It
is to be able to browse a repo in either both sequential directions,
up or down, using only the mouse wheel.
^ permalink raw reply
* Re: [PATCH] fix signed range problems with hex conversions
From: Nicolas Pitre @ 2007-05-30 18:20 UTC (permalink / raw)
To: Linus Torvalds; +Cc: Junio C Hamano, Git Mailing List
In-Reply-To: <alpine.LFD.0.98.0705301025170.26602@woody.linux-foundation.org>
On Wed, 30 May 2007, Linus Torvalds wrote:
> On Wed, 30 May 2007, Nicolas Pitre wrote:
> >
> > This patch adds a range test to hexval() in order to prevent this. Also
> > let's index the hexval_table array directly in get_sha1_hex() using
> > explicitly unsigned chars to avoid the range test producing faster
> > code.
>
> Please just make hexval() take a "unsigned char" instead, solving the
> problem in a manner that makes it much easier for the compiler to realize
> that it never needs to sign-extend the value or test the end result..
Well, I did it the way I did for two reasons, the first being that
random hexval() usage won't mask a bad value if it is passed an int
which happens to be out of range (think EOF, or better yet -208, -207,
etc).
Yet gcc appears to be smart enough to skip the test if it is passed a
value that cannot exceed the test range, like an unsigned char.
> Ie I think your patch would be better off something like the following
> instead (it would be a one-liner, but I agree that marking hexval_table
> "const" is also a good idea).
>
> With this, gcc can just generate:
>
> movzbl (%rdi), %eax
> movsbl hexval_table(%rax),%edx
> movzbl 1(%rdi), %eax
> movsbl hexval_table(%rax),%eax
> sall $4, %edx
> orl %eax, %edx
>
> for the code to generate a byte from two hex characters.
My patch already produces code that looks like that for get_sha1_hex()
on i386. But my second reason for the patch is that on ARM it
should becomes:
/* r4 = hexval_table, ip = hex */
ldrb r1, [ip], #1
ldrb r2, [ip], #1
ldrsb r0, [r4, r1]
ldrsb r3, [r4, r2]
orr r0, r3, r0, asl #4
I.e. the compiler has a greater chance to fold the post increment with
the load byte instruction as above without the need for an extra
add instruction.
Nicolas
^ permalink raw reply
* Re: [MinGW port] Unable to repack on Clearcase dynamic views
From: Robin Rosenberg @ 2007-05-30 18:28 UTC (permalink / raw)
To: Nguyen Thai Ngoc Duy
Cc: Paolo Teti, Git Mailing List, Matthias Lederhofer, Junio C Hamano
In-Reply-To: <fcaeb9bf0705300833t6f0639ddx2f507996e48d838c@mail.gmail.com>
onsdag 30 maj 2007 skrev Nguyen Thai Ngoc Duy:
> On 5/30/07, Paolo Teti <paolo.teti@gmail.com> wrote:
> > 2007/5/30, Nguyen Thai Ngoc Duy <pclouds@gmail.com>:
> > > Hi,
> > >
> > > On clearcase dynamic views (kind of virtual drives), "mv -f a b" won't
> > > work if "a" is read-only. Because git-repack.sh removes all write
> > > permission before moving packs, it fails on clearcase dynamic views.
> > >
> > > My approach is rather ugly. Does anyone have a better solution?
> >
> > I hate clearcase, but honestly I have used it forced by a customer..
> > The real problem is why you need to do a repack in a CC view, but..
>
> I don't like it either :) But you remind me the Matthias' workdir
> patchset. With that I can move out .git to a real drive/filesystem
> (which should also give better performance).
>
> I think it's a valid use case. Anyone remember why Matthias' patchset
> was dropped?
> It was last mentioned in
> http://article.gmane.org/gmane.comp.version-control.git/43041
>
> Junio, Matthias? May I help?
Wasn't because it's better to make .git a link to a repository somewhere
else? Just a guess.
ln -s /somewhere/repo/.git .git
Works fine for me (yes, in a clearcase dynamic view).
BTW, Does anyone have something like git-cvsexportcommit for clearcase?
-- robin
^ permalink raw reply
* Re: [MinGW port] Unable to repack on Clearcase dynamic views
From: Johannes Schindelin @ 2007-05-30 18:44 UTC (permalink / raw)
To: Robin Rosenberg
Cc: Nguyen Thai Ngoc Duy, Paolo Teti, Git Mailing List,
Matthias Lederhofer, Junio C Hamano
In-Reply-To: <200705302028.15549.robin.rosenberg.lists@dewire.com>
Hi,
On Wed, 30 May 2007, Robin Rosenberg wrote:
> Wasn't because it's better to make .git a link to a repository somewhere
> else? Just a guess.
>
> ln -s /somewhere/repo/.git .git
>
> Works fine for me (yes, in a clearcase dynamic view).
MinGW does not know symbolic links (at least not on the common
filesystems).
Ciao,
Dscho
^ permalink raw reply
* Re: [MinGW port] Unable to repack on Clearcase dynamic views
From: Nguyen Thai Ngoc Duy @ 2007-05-30 19:08 UTC (permalink / raw)
To: Robin Rosenberg
Cc: Paolo Teti, Git Mailing List, Matthias Lederhofer, Junio C Hamano
In-Reply-To: <200705302028.15549.robin.rosenberg.lists@dewire.com>
On 5/30/07, Robin Rosenberg <robin.rosenberg.lists@dewire.com> wrote:
> onsdag 30 maj 2007 skrev Nguyen Thai Ngoc Duy:
> > On 5/30/07, Paolo Teti <paolo.teti@gmail.com> wrote:
> > > 2007/5/30, Nguyen Thai Ngoc Duy <pclouds@gmail.com>:
> > > > Hi,
> > > >
> > > > On clearcase dynamic views (kind of virtual drives), "mv -f a b" won't
> > > > work if "a" is read-only. Because git-repack.sh removes all write
> > > > permission before moving packs, it fails on clearcase dynamic views.
> > > >
> > > > My approach is rather ugly. Does anyone have a better solution?
> > >
> > > I hate clearcase, but honestly I have used it forced by a customer..
> > > The real problem is why you need to do a repack in a CC view, but..
> >
> > I don't like it either :) But you remind me the Matthias' workdir
> > patchset. With that I can move out .git to a real drive/filesystem
> > (which should also give better performance).
> >
> > I think it's a valid use case. Anyone remember why Matthias' patchset
> > was dropped?
> > It was last mentioned in
> > http://article.gmane.org/gmane.comp.version-control.git/43041
> >
> > Junio, Matthias? May I help?
>
> Wasn't because it's better to make .git a link to a repository somewhere
> else? Just a guess.
>
> ln -s /somewhere/repo/.git .git
>
> Works fine for me (yes, in a clearcase dynamic view).
Clearcase symlinks require checkout/checkin stuffs that I really don't
want to mess up with. Moreover, it seems not work if the link
destination is out of clearcase view (which is what I wanted).
--
Duy
^ 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