# base85: Two tiny fixes 11 messages from 2010-01-07 to 2010-01-08. Participants: Andreas Gruenbacher, Nicolas Pitre, Michael J Gruber, Junio C Hamano, A Large Angry SCM. Thread: https://gitlist.dev/t/22113 ## Andreas Gruenbacher, 2010-01-07 14:58 Subject: base85: Two tiny fixes Message-ID: <201001071558.30065.agruen@suse.de> URL: https://gitlist.dev/e/201001071558.30065.agruen%40suse.de ``` While looking at the base85 code I found a bug in the debug code and an unnecessary call. You may want to have a look at the two fixes here: http://www.kernel.org/pub/scm/linux/kernel/git/agruen/git.git There is another little oddity in the way the de85 table is set up: 0 indicates an invalid entry; to avoid this from clashing with a valid entry, valid entries are incremented by one and decremented again while decoding. This leads to slightly worse code than using a negative number to indicate invalid values (and avoiding to increment/decrement). Andreas ``` ## Nicolas Pitre, 2010-01-07 17:58 Subject: Re: base85: Two tiny fixes Message-ID: URL: https://gitlist.dev/e/alpine.LFD.2.00.1001071253400.21025%40xanadu.home In-Reply-To: <201001071558.30065.agruen@suse.de> ``` On Thu, 7 Jan 2010, Andreas Gruenbacher wrote: > While looking at the base85 code I found a bug in the debug code and an > unnecessary call. You may want to have a look at the two fixes here: > > http://www.kernel.org/pub/scm/linux/kernel/git/agruen/git.git ACK. Please post them to this list. > There is another little oddity in the way the de85 table is set up: 0 > indicates an invalid entry; to avoid this from clashing with a valid entry, > valid entries are incremented by one and decremented again while decoding. > This leads to slightly worse code than using a negative number to indicate > invalid values (and avoiding to increment/decrement). You can make a patch to modify that as well if you wish. And in that case don't forget to make de85 explicitly signed as a char is unsigned by default on some platforms. Nicolas ``` ## Andreas Gruenbacher, 2010-01-08 13:02 Subject: Re: base85: Two tiny fixes Message-ID: <201001081402.02476.agruen@suse.de> URL: https://gitlist.dev/e/201001081402.02476.agruen%40suse.de In-Reply-To: ``` On Thursday 07 January 2010 18:58:01 Nicolas Pitre wrote: > ACK. Please post them to this list. Okay, done. > On Thu, 7 Jan 2010, Andreas Gruenbacher wrote: > > There is another little oddity in the way the de85 table is set up: 0 > > indicates an invalid entry; to avoid this from clashing with a valid entry, > > valid entries are incremented by one and decremented again while decoding. > > This leads to slightly worse code than using a negative number to indicate > > invalid values (and avoiding to increment/decrement). > > You can make a patch to modify that as well if you wish. Nah, it's not worth the noise. > And in that case don't forget to make de85 explicitly signed as a char is > unsigned by default on some platforms. I would have forgotten this; thanks for pointing it out! Thanks, Andreas ``` ## Andreas Gruenbacher, 2010-01-08 13:39 Subject: [PATCH 1/3] base85 debug code: Fix length byte calculation Message-ID: <1262958000-27181-1-git-send-email-agruen@suse.de> URL: https://gitlist.dev/e/1262958000-27181-1-git-send-email-agruen%40suse.de In-Reply-To: ``` Signed-off-by: Andreas Gruenbacher --- base85.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/base85.c b/base85.c index b417a15..1d165d9 100644 --- a/base85.c +++ b/base85.c @@ -118,7 +118,7 @@ int main(int ac, char **av) int len = strlen(av[2]); encode_85(buf, av[2], len); if (len <= 26) len = len + 'A' - 1; - else len = len + 'a' - 26 + 1; + else len = len + 'a' - 26 - 1; printf("encoded: %c%s\n", len, buf); return 0; } -- 1.6.6.75.g37bae ``` ## Andreas Gruenbacher, 2010-01-08 13:39 Subject: [PATCH 2/3] base85: No need to initialize the decode table in encode_85 Message-ID: <1262958000-27181-2-git-send-email-agruen@suse.de> URL: https://gitlist.dev/e/1262958000-27181-2-git-send-email-agruen%40suse.de In-Reply-To: ``` Signed-off-by: Andreas Gruenbacher --- base85.c | 2 -- 1 files changed, 0 insertions(+), 2 deletions(-) diff --git a/base85.c b/base85.c index 1d165d9..7204ce2 100644 --- a/base85.c +++ b/base85.c @@ -84,8 +84,6 @@ int decode_85(char *dst, const char *buffer, int len) void encode_85(char *buf, const unsigned char *data, int bytes) { - prep_base85(); - say("encode 85"); while (bytes) { unsigned acc = 0; -- 1.6.6.75.g37bae ``` ## Andreas Gruenbacher, 2010-01-08 13:40 Subject: [PATCH 3/3] base85: Make the code more obvious instead of explaining the non-obvious Message-ID: <1262958000-27181-3-git-send-email-agruen@suse.de> URL: https://gitlist.dev/e/1262958000-27181-3-git-send-email-agruen%40suse.de In-Reply-To: ``` Here is another cleanup ... Signed-off-by: Andreas Gruenbacher --- base85.c | 10 ++-------- 1 files changed, 2 insertions(+), 8 deletions(-) diff --git a/base85.c b/base85.c index 7204ce2..e459fee 100644 --- a/base85.c +++ b/base85.c @@ -57,14 +57,8 @@ int decode_85(char *dst, const char *buffer, int len) de = de85[ch]; if (--de < 0) return error("invalid base85 alphabet %c", ch); - /* - * Detect overflow. The largest - * 5-letter possible is "|NsC0" to - * encode 0xffffffff, and "|NsC" gives - * 0x03030303 at this point (i.e. - * 0xffffffff = 0x03030303 * 85). - */ - if (0x03030303 < acc || + /* Detect overflow. */ + if (0xffffffff / 85 < acc || 0xffffffff - de < (acc *= 85)) return error("invalid base85 sequence %.5s", buffer-5); acc += de; -- 1.6.6.75.g37bae ``` ## Michael J Gruber, 2010-01-08 15:46 Subject: Re: [PATCH 2/3] base85: No need to initialize the decode table in encode_85 Message-ID: <4B475361.60506@drmicha.warpmail.net> URL: https://gitlist.dev/e/4B475361.60506%40drmicha.warpmail.net In-Reply-To: <1262958000-27181-2-git-send-email-agruen@suse.de> ``` Andreas Gruenbacher venit, vidit, dixit 08.01.2010 14:39: > Signed-off-by: Andreas Gruenbacher > --- For the less informed it may be worthwhile to have an explanation in the commit message why encode_85() does not need to initialize the table. (I strongly suspect it's a matter of de vs. en, i.e. "because it only encodes but does not decode."...) > base85.c | 2 -- > 1 files changed, 0 insertions(+), 2 deletions(-) > > diff --git a/base85.c b/base85.c > index 1d165d9..7204ce2 100644 > --- a/base85.c > +++ b/base85.c > @@ -84,8 +84,6 @@ int decode_85(char *dst, const char *buffer, int len) > > void encode_85(char *buf, const unsigned char *data, int bytes) > { > - prep_base85(); > - > say("encode 85"); > while (bytes) { > unsigned acc = 0; ``` ## Junio C Hamano, 2010-01-08 15:55 Subject: Re: [PATCH 2/3] base85: No need to initialize the decode table in encode_85 Message-ID: <7v3a2giobm.fsf@alter.siamese.dyndns.org> URL: https://gitlist.dev/e/7v3a2giobm.fsf%40alter.siamese.dyndns.org In-Reply-To: <4B475361.60506@drmicha.warpmail.net> ``` Michael J Gruber writes: > Andreas Gruenbacher venit, vidit, dixit 08.01.2010 14:39: >> Signed-off-by: Andreas Gruenbacher >> --- > > For the less informed it may be worthwhile to have an explanation in the > commit message why encode_85() does not need to initialize the table. (I > strongly suspect it's a matter of de vs. en, i.e. "because it only > encodes but does not decode."...) The title can be reworded to base85: encode85() does not use the decode table ``` ## Andreas Gruenbacher, 2010-01-08 16:17 Subject: [PATCH] base85: encode85() does not use the decode table Message-ID: <1262967470-31782-1-git-send-email-agruen@suse.de> URL: https://gitlist.dev/e/1262967470-31782-1-git-send-email-agruen%40suse.de In-Reply-To: <7v3a2giobm.fsf@alter.siamese.dyndns.org> ``` Signed-off-by: Andreas Gruenbacher --- base85.c | 2 -- 1 files changed, 0 insertions(+), 2 deletions(-) diff --git a/base85.c b/base85.c index 1d165d9..7204ce2 100644 --- a/base85.c +++ b/base85.c @@ -84,8 +84,6 @@ int decode_85(char *dst, const char *buffer, int len) void encode_85(char *buf, const unsigned char *data, int bytes) { - prep_base85(); - say("encode 85"); while (bytes) { unsigned acc = 0; -- 1.6.6.75.g37bae ``` ## Andreas Gruenbacher, 2010-01-08 16:22 Subject: [PATCH] base85: encode_85() does not use the decode table Message-ID: <1262967738-31996-1-git-send-email-agruen@suse.de> URL: https://gitlist.dev/e/1262967738-31996-1-git-send-email-agruen%40suse.de In-Reply-To: <7v3a2giobm.fsf@alter.siamese.dyndns.org> ``` Signed-off-by: Andreas Gruenbacher --- base85.c | 2 -- 1 files changed, 0 insertions(+), 2 deletions(-) diff --git a/base85.c b/base85.c index 1d165d9..7204ce2 100644 --- a/base85.c +++ b/base85.c @@ -84,8 +84,6 @@ int decode_85(char *dst, const char *buffer, int len) void encode_85(char *buf, const unsigned char *data, int bytes) { - prep_base85(); - say("encode 85"); while (bytes) { unsigned acc = 0; -- 1.6.6.75.g37bae ``` ## A Large Angry SCM, 2010-01-08 23:15 Subject: Re: [PATCH 3/3] base85: Make the code more obvious instead of explaining the non-obvious Message-ID: <4B47BC9C.1020807@gmail.com> URL: https://gitlist.dev/e/4B47BC9C.1020807%40gmail.com In-Reply-To: <1262958000-27181-3-git-send-email-agruen@suse.de> ``` Andreas Gruenbacher wrote: > Here is another cleanup ... > I LOVE the subject line of this commit! ```