Discussion:
RFC: New Response type to aid rlog reponse parsing
patrick keshishian
2017-11-04 05:48:01 UTC
Permalink
Greetings,

I am hoping you would be open to adding a new (optional) response
type. Motivation behind this addition is to aid clients distinguish
the free-form log/commit messages in response to an rlog
query/request.

The new response type I am proposing is LOGM. A server recognizing it
in Valid-responses list, will prepend "LOGM " to each log/commit
message line (rather than "M ") while servicing rlog requests. Servers
not recognizing it will work as they do today.

With this change in effect, all guess-work over where a log/commit
message begins and ends is eliminated.


I ran into at least three different log/commit message forms which
confused CVSps and then in turn Git-cvsimport.

I have documented this [1] for interested parties. It is a rather
long-ish document. I have done my best to organize the information for
easy digestion and reference.


Attached is a patch implementing LOGM change against cvs-1.11.23
downloaded from your site (unfortunately I couldn't get sources
through anoncvs).

The changes are fairly simple, and the best I can tell, backward
compatible with unpatched components.

Given this, I hope you are open to accepting these changes.

Cheers,
--patrick

[1] http://sidster.com/code/cvs2git/
Thorsten Glaser
2017-11-04 13:51:16 UTC
Permalink
Hi,
Post by patrick keshishian
I have documented this [1] for interested parties. It is a rather
long-ish document. I have done my best to organize the information for
easy digestion and reference.
how interesting. I=E2=80=99ve added a =E2=80=9Ccvs suck=E2=80=9D command to=
the 1.12 flavour
shipped with MirBSD and Debian (current de-facto =E2=80=9Cupstream=E2=80=9D=
of GNU
CVS as I seem to be the only one working on it) to download individual
,v files, but that would of course only work if the server supports it.
Post by patrick keshishian
Attached is a patch implementing LOGM change against cvs-1.11.23
I=E2=80=99ve looked at it and it=E2=80=99s trivial and probably correct eno=
ugh,
so I=E2=80=99ll patch my 1.12 branch similarily and add to the (in)=E2=80=
=9Csanity=E2=80=9D
testsuite. Then you=E2=80=99ll only have to convince the OpenCVS maintainer=
s
to adapt (if it still exists, that is, haven=E2=80=99t checked) and everyon=
e
is compatible.

Thanks,
//mirabilos
PS: Due to Google=E2=80=99s inability to honour basic Internet standards,
direct messages from Googlemail to my eMail address are likely
to not reach me, or only delayed by up to several weeks.
--=20
Post by patrick keshishian
Wish I had pine to hand :-( I'll give lynx a try, thanks.
Michael Schmitz on nntp://news.gmane.org/gmane.linux.debian.ports.68k
a.k.a. {news.gmane.org/nntp}#news.gmane.linux.debian.ports.68k in pine
patrick keshishian
2017-11-04 17:24:45 UTC
Permalink
Hi Thorsten,
Hi,
Post by patrick keshishian
I have documented this [1] for interested parties. It is a rather
long-ish document. I have done my best to organize the information for
easy digestion and reference.
how interesting. I’ve added a “cvs suck” command to the 1.12 flavour
shipped with MirBSD and Debian (current de-facto “upstream” of GNU
CVS as I seem to be the only one working on it) to download individual
,v files, but that would of course only work if the server supports it.
Post by patrick keshishian
Attached is a patch implementing LOGM change against cvs-1.11.23
I’ve looked at it and it’s trivial and probably correct enough,
so I’ll patch my 1.12 branch similarily and add to the (in)“sanity”
testsuite. Then you’ll only have to convince the OpenCVS maintainers
to adapt (if it still exists, that is, haven’t checked) and everyone
is compatible.
Thanks for the quick response and vote of confidence.

As far as I can tell OpenCVS has been pretty dormant. I was going to
give them a heads up after getting a feel for the reception of this
change by CVS proper.

Cheers,
--patrick

p.s., sorry about my gmail address :(
Thanks,
//mirabilos
PS: Due to Google’s inability to honour basic Internet standards,
direct messages from Googlemail to my eMail address are likely
to not reach me, or only delayed by up to several weeks.
--
Post by patrick keshishian
Wish I had pine to hand :-( I'll give lynx a try, thanks.
Michael Schmitz on nntp://news.gmane.org/gmane.linux.debian.ports.68k
a.k.a. {news.gmane.org/nntp}#news.gmane.linux.debian.ports.68k in pine
Thorsten Glaser
2017-11-04 18:44:49 UTC
Permalink
Hi patrick,
Post by patrick keshishian
As far as I can tell OpenCVS has been pretty dormant. I was going to
give them a heads up after getting a feel for the reception of this
change by CVS proper.
as far as I can tell, nobody develops =E2=80=9CCVS proper=E2=80=9D any more=
=2E

I=E2=80=99m pretty much doing my own thing with the 1.12 branch only,
merging patches thrown into my general direction and trying
to take over distro packages, but I=E2=80=99m not a full upstream
maintainer (yet).

bye,
//mirabilos
--=20
St=C3=A9phane, I actually don=E2=80=99t block Googlemail, they=E2=80=99re j=
ust too utterly
stupid to successfully deliver to me (or anyone else using Greylisting
and not whitelisting their ranges). Same for a few other providers such
as Hotmail. Some spammers (Yahoo) I do block.
Thorsten Glaser
2017-11-18 23:56:07 UTC
Permalink
Post by patrick keshishian
Attached is a patch implementing LOGM change against cvs-1.11.23
I=E2=80=99ve implemented it differently, especially in order to never
send partial lines to the client (in a network packet). If you
wish, have a look at / review the following patch.

I=E2=80=99m currently sanity.sh-testing it, this will take the night.

--- src/gnu/usr.bin/cvs/src/buffer.c:1.2=09Sat Oct 22 14:33:29 2016
+++ src/gnu/usr.bin/cvs/src/buffer.c=09Sat Nov 18 22:05:10 2017
@@ -1052,7 +1055,14 @@ buf_copy_lines (struct buffer *outbuf, s
=09}
=20
=09/* Put in the command. */
-=09buf_append_char (outbuf, command);
+=09switch (command) {
+=09case CVS_OUTPUT_EX_LOGM:
+=09=09buf_output0 (outbuf, "LOGM");
+=09=09break;
+=09default:
+=09=09buf_append_char (outbuf, command);
+=09=09break;
+=09}
=09buf_append_char (outbuf, ' ');
=20
=09if (inbuf->data !=3D nldata)
--- src/gnu/usr.bin/cvs/src/client.c:1.8=09Sat Aug 12 01:51:22 2017
+++ src/gnu/usr.bin/cvs/src/client.c=09Sat Nov 18 22:05:11 2017
@@ -3079,6 +3079,7 @@ struct response responses[] =3D
rs_optional),
RSP_LINE("M", handle_m, response_type_normal, rs_essential),
RSP_LINE("Mbinary", handle_mbinary, response_type_normal, rs_optional)=
,
+ RSP_LINE("LOGM", handle_m, response_type_normal, rs_optional),
RSP_LINE("E", handle_e, response_type_normal, rs_essential),
RSP_LINE("F", handle_f, response_type_normal, rs_optional),
RSP_LINE("MT", handle_mt, response_type_normal, rs_optional),
--- src/gnu/usr.bin/cvs/src/cvs.h:1.7=09Fri Oct 21 16:41:16 2016
+++ src/gnu/usr.bin/cvs/src/cvs.h=09Sat Nov 18 22:05:12 2017
@@ -920,11 +921,15 @@ void tag_check_valid (const char *, int,
=20
/* From server.c and documented there. */
void cvs_output (const char *, size_t);
+void cvs_output_ex (const char *, size_t, int);
void cvs_output_binary (char *, size_t);
void cvs_outerr (const char *, size_t);
void cvs_flusherr (void);
void cvs_flushout (void);
void cvs_output_tagged (const char *, const char *);
+int supported_response (const char *);
+
+#define CVS_OUTPUT_EX_LOGM=090x80000001
=20
extern const char *global_session_id;
=20
--- src/gnu/usr.bin/cvs/src/log.c:1.1.101.2=09Tue Apr 19 20:33:18 2005
+++ src/gnu/usr.bin/cvs/src/log.c=09Sat Nov 18 22:05:12 2017
@@ -145,6 +148,7 @@ static void log_version (struct log_data
=09=09=09=09RCSNode *, RCSVers *, int);
static int log_branch (Node *, void *);
static int version_compare (const char *, const char *, int);
+static void logm_output (const char *);
=20
static struct log_data log_data;
static int is_rlog;
@@ -1681,11 +1685,10 @@ log_version (struct log_data *log_data,=20
=09cvs_output ("*** empty log message ***\n", 0);
else
{
+=09/* assert: last thing cvs_output=E2=80=99ed was a newline */
=09/* FIXME: Technically, the log message could contain a null
byte. */
-=09cvs_output (p->data, 0);
-=09if (((char *)p->data)[strlen (p->data) - 1] !=3D '\n')
-=09 cvs_output ("\n", 1);
+=09logm_output(p->data);
}
}
=20
@@ -1780,3 +1783,23 @@ version_compare (const char *v1, const c
=09 ++v2;
}
}
+
+static void
+logm_output(const char *str)
+{
+=09/* assert: str is not empty */
+=09size_t len =3D strlen(str);
+=09int buftag =3D 'M';
+=09static char has_logm =3D 0;
+
+=09if (server_active) {
+=09=09if (!has_logm)
+=09=09=09has_logm =3D supported_response("LOGM") ? 1 : 2;
+=09=09if (has_logm =3D=3D 1)
+=09=09=09buftag =3D CVS_OUTPUT_EX_LOGM;
+=09}
+
+=09cvs_output_ex(str, len, buftag);
+=09if (/*len > 0 &&*/ str[len - 1] !=3D '\n')
+=09=09cvs_output_ex("\n", 1, buftag);
+}
--- src/gnu/usr.bin/cvs/src/server.c:1.14=09Sat Aug 12 01:08:25 2017
+++ src/gnu/usr.bin/cvs/src/server.c=09Sat Nov 18 22:05:12 2017
@@ -548,8 +553,8 @@ alloc_pending_warning (size_t size)
=20
=20
=20
-static int
-supported_response (char *name)
+int
+supported_response (const char *name)
{
struct response *rs;
=20
@@ -7703,6 +7708,12 @@ krb_encrypt_buffer_initialize( struct bu
void
cvs_output (const char *str, size_t len)
{
+ cvs_output_ex (str, len, 'M');
+}
+
+void
+cvs_output_ex (const char *str, size_t len, int buftag)
+{
if (len =3D=3D 0)
=09len =3D strlen (str);
#ifdef SERVER_SUPPORT
@@ -7711,7 +7722,7 @@ cvs_output (const char *str, size_t len)
=09if (buf_to_net)
=09{
=09 buf_output (saved_output, str, len);
-=09 buf_copy_lines (buf_to_net, saved_output, 'M');
+=09 buf_copy_lines (buf_to_net, saved_output, buftag);
=09}
# if HAVE_SYSLOG_H
=09else
@@ -7726,7 +7737,7 @@ cvs_output (const char *str, size_t len)
=09if (protocol)
=09{
=09 buf_output (saved_output, str, len);
-=09 buf_copy_lines (protocol, saved_output, 'M');
+=09 buf_copy_lines (protocol, saved_output, buftag);
=09 buf_send_counted (protocol);
=09}
# if HAVE_SYSLOG_H


bye,
//mirabilos
--=20
13:22=E2=8E=9C=C2=ABneurodamage=C2=BB mira, what's up man? I have a CVS que=
stion for you in #cvs
13:22=E2=8E=9C=C2=ABneurodamage=C2=BB since you're so good w. it =E2=94=82 =
=C2=ABneurodamage:#cvs=C2=BB i love you
13:28=E2=8E=9C=C2=ABneurodamage:#cvs=C2=BB you're a handy guy to have aroun=
d for systems stuff =E2=98=BA
16:06=E2=8E=9C<Draget:#cvs> Thank god I found you =3D) 20:03=E2=94=82=C2=
=ABbioe007:#cvs=C2=BB mira2k: ty
17:14=E2=8E=9C<ldiain:#cvs> Thanks big help you are :-) <bioe007> mira|nw=
t: ty again
18:35=E2=8E=9C=C2=ABalturiak:#cvs=C2=BB mirabilos: aw, nice. thanks :o
18:36=E2=8E=9C=C2=ABThunderChicken:#cvs=C2=BB mirabilos FTW! 23:03=E2=8E=
=9C=C2=ABmithraic:#cvs=C2=BB aaah. thanks
18:41=E2=8E=9C=C2=ABalturiak:#cvs=C2=BB phew. thanks a bunch, guys. you jus=
t made my weekend :-)
18:10=E2=8E=9C=C2=ABsumit:#cvs=C2=BB mirabilos: oh ok.. thanks for that
21:57=E2=8E=9C<bhuey:#cvs> yeah, I really appreciate help
18:50=E2=8E=9C=C2=ABgrndlvl:#cvs=C2=BB thankyou 18:50=E2=8E=9C=
=C2=ABgrndlvl:#cvs=C2=BB worked perfectly
20:50=E2=8E=9C<paolo:#cvs> i see. mirabilos, thnks for your support
00:36=E2=8E=9C=C2=ABhalirutan:#cvs=C2=BB ok, the obvious way:-) thx
18:44=E2=8E=9C=C2=ABarcfide:#cvs=C2=BB mirabilos, I am running OpenBSD. =
18:59=E2=8E=9C=C2=ABarcfide:#cvs=C2=BB
Hrm, yes, I see what you mean. 19:01=E2=8E=9C=C2=ABarcfide:#cvs=C2=BB Yeah,=
thanks for the help.
21:33=E2=8E=9C=C2=ABCardinalFang:#cvs=C2=BB Ugh. Okay. Sorry for the dumb=
question. Thank you
21:34=E2=8E=9C<centosian:#cvs> mirabilos: whoa that's sweet
21:52=E2=8E=9C=C2=ABgarrett__:#cvs=C2=BB much appreciated =C2=ABgarrett__:=
#cvs=C2=BB thanks for your time
23:39=E2=8E=9C<symons:#cvs> this worked, thank you very much 16:26=E2=8E=9C=
<schweizer:#cvs> ok
thx, i'll try that 20:00=E2=8E=9C=C2=ABstableable:#cvs=C2=BB Thank you.=
20:50=E2=8E=9C=C2=ABs833:#cvs=C2=BB
mirabilos: thanks a lot. 19:34=E2=8E=9C<bobbytek:#cvs> Thanks for co=
nfirming :)
20:08=E2=8E=9C<tsolox:#cvs> ...works like a charm.. thanks mirabilos
patrick keshishian
2017-11-21 00:57:22 UTC
Permalink
Hi Thorsten,

Sorry for the delayed reply (project I'm working on is taking
far too many cycles).
---------- Forwarded message ----------
Date: Sat, 18 Nov 2017 23:56:07 +0000 (UTC)
Subject: Re: RFC: New Response type to aid rlog reponse parsing
Post by patrick keshishian
Attached is a patch implementing LOGM change against cvs-1.11.23
I've implemented it differently, especially in order to never
send partial lines to the client (in a network packet). If you
wish, have a look at / review the following patch.
Just from reading the diff it looks OK. However, I didn't comprehend
the sending "partial lines to the client" bit.
I'm currently sanity.sh-testing it, this will take the night.
Hopefully your tests don't show any regression.

I can patch my CVS server with your diff when I break away from
my work.

Thanks for the update!

Happy Thanksgiving!

--patrick

p.s., sending from a different account (not subscribed to CVS list)
hopefully it'll make it.
--- src/gnu/usr.bin/cvs/src/buffer.c:1.2 Sat Oct 22 14:33:29 2016
+++ src/gnu/usr.bin/cvs/src/buffer.c Sat Nov 18 22:05:10 2017
@@ -1052,7 +1055,14 @@ buf_copy_lines (struct buffer *outbuf, s
}
/* Put in the command. */
- buf_append_char (outbuf, command);
+ switch (command) {
+ buf_output0 (outbuf, "LOGM");
+ break;
+ buf_append_char (outbuf, command);
+ break;
+ }
buf_append_char (outbuf, ' ');
if (inbuf->data != nldata)
--- src/gnu/usr.bin/cvs/src/client.c:1.8 Sat Aug 12 01:51:22 2017
+++ src/gnu/usr.bin/cvs/src/client.c Sat Nov 18 22:05:11 2017
@@ -3079,6 +3079,7 @@ struct response responses[] =
rs_optional),
RSP_LINE("M", handle_m, response_type_normal, rs_essential),
RSP_LINE("Mbinary", handle_mbinary, response_type_normal, rs_optional),
+ RSP_LINE("LOGM", handle_m, response_type_normal, rs_optional),
RSP_LINE("E", handle_e, response_type_normal, rs_essential),
RSP_LINE("F", handle_f, response_type_normal, rs_optional),
RSP_LINE("MT", handle_mt, response_type_normal, rs_optional),
--- src/gnu/usr.bin/cvs/src/cvs.h:1.7 Fri Oct 21 16:41:16 2016
+++ src/gnu/usr.bin/cvs/src/cvs.h Sat Nov 18 22:05:12 2017
@@ -920,11 +921,15 @@ void tag_check_valid (const char *, int,
/* From server.c and documented there. */
void cvs_output (const char *, size_t);
+void cvs_output_ex (const char *, size_t, int);
void cvs_output_binary (char *, size_t);
void cvs_outerr (const char *, size_t);
void cvs_flusherr (void);
void cvs_flushout (void);
void cvs_output_tagged (const char *, const char *);
+int supported_response (const char *);
+
+#define CVS_OUTPUT_EX_LOGM 0x80000001
extern const char *global_session_id;
--- src/gnu/usr.bin/cvs/src/log.c:1.1.101.2 Tue Apr 19 20:33:18 2005
+++ src/gnu/usr.bin/cvs/src/log.c Sat Nov 18 22:05:12 2017
@@ -145,6 +148,7 @@ static void log_version (struct log_data
RCSNode *, RCSVers *, int);
static int log_branch (Node *, void *);
static int version_compare (const char *, const char *, int);
+static void logm_output (const char *);
static struct log_data log_data;
static int is_rlog;
@@ -1681,11 +1685,10 @@ log_version (struct log_data *log_data,
cvs_output ("*** empty log message ***\n", 0);
else
{
+ /* assert: last thing cvs_output???ed was a newline */
/* FIXME: Technically, the log message could contain a null
byte. */
- cvs_output (p->data, 0);
- if (((char *)p->data)[strlen (p->data) - 1] != '\n')
- cvs_output ("\n", 1);
+ logm_output(p->data);
}
}
@@ -1780,3 +1783,23 @@ version_compare (const char *v1, const c
++v2;
}
}
+
+static void
+logm_output(const char *str)
+{
+ /* assert: str is not empty */
+ size_t len = strlen(str);
+ int buftag = 'M';
+ static char has_logm = 0;
+
+ if (server_active) {
+ if (!has_logm)
+ has_logm = supported_response("LOGM") ? 1 : 2;
+ if (has_logm == 1)
+ buftag = CVS_OUTPUT_EX_LOGM;
+ }
+
+ cvs_output_ex(str, len, buftag);
+ if (/*len > 0 &&*/ str[len - 1] != '\n')
+ cvs_output_ex("\n", 1, buftag);
+}
--- src/gnu/usr.bin/cvs/src/server.c:1.14 Sat Aug 12 01:08:25 2017
+++ src/gnu/usr.bin/cvs/src/server.c Sat Nov 18 22:05:12 2017
@@ -548,8 +553,8 @@ alloc_pending_warning (size_t size)
-static int
-supported_response (char *name)
+int
+supported_response (const char *name)
{
struct response *rs;
@@ -7703,6 +7708,12 @@ krb_encrypt_buffer_initialize( struct bu
void
cvs_output (const char *str, size_t len)
{
+ cvs_output_ex (str, len, 'M');
+}
+
+void
+cvs_output_ex (const char *str, size_t len, int buftag)
+{
if (len == 0)
len = strlen (str);
#ifdef SERVER_SUPPORT
@@ -7711,7 +7722,7 @@ cvs_output (const char *str, size_t len)
if (buf_to_net)
{
buf_output (saved_output, str, len);
- buf_copy_lines (buf_to_net, saved_output, 'M');
+ buf_copy_lines (buf_to_net, saved_output, buftag);
}
# if HAVE_SYSLOG_H
else
@@ -7726,7 +7737,7 @@ cvs_output (const char *str, size_t len)
if (protocol)
{
buf_output (saved_output, str, len);
- buf_copy_lines (protocol, saved_output, 'M');
+ buf_copy_lines (protocol, saved_output, buftag);
buf_send_counted (protocol);
}
# if HAVE_SYSLOG_H
bye,
//mirabilos
Thorsten Glaser
2017-11-21 20:19:15 UTC
Permalink
Hi patrick,
Post by patrick keshishian
Sorry for the delayed reply (project I'm working on is taking
far too many cycles).
don=E2=80=99t worry, I know all about that.
Post by patrick keshishian
Just from reading the diff it looks OK. However, I didn't comprehend
the sending "partial lines to the client" bit.
There was some comment in cvs_output() about being careful to
only write to the output stream when a full line has been
accumulated.
Post by patrick keshishian
I'm currently sanity.sh-testing it, this will take the night.
Hopefully your tests don't show any regression.
It didn=E2=80=99t. I also manually tested the server by interacting
with 'cvs server' from the commandline, and it looked fine.
Post by patrick keshishian
I can patch my CVS server with your diff when I break away from
my work.
Mind that this diff is based on continued development from
the 1.12.13 release, not the ancient 1.11.x branch, though.
Post by patrick keshishian
Thanks for the update!
You=E2=80=99re welcome!
Post by patrick keshishian
Happy Thanksgiving!
Erm, not here (Europe), but thanks anyway, and same to you.
Post by patrick keshishian
p.s., sending from a different account (not subscribed to CVS list)
hopefully it'll make it.
It made it, both over the list and separately.

bye,
//mirabilos
--=20
Support mksh as /bin/sh and RoQA dash NOW!
=E2=80=A3 src:bash (358 (380) bugs: 1 RC, 246 (262) I&N, 111 (117) M&W, 0 F=
&P)
=E2=80=A3 src:dash (110 (129) bugs: 0 RC, 61 (67) I&N, 49 (62) M&W, 0 F&P)
=E2=80=A3 src:mksh (0 bugs: 0 RC, 0 I&N, 0 M&W, 0 F&P)
dash has two RC bugs they just closed because they don=E2=80=99t care about=
quality=E2=80=A6

Loading...