RE: cvs patches & notes

[Rob Saccoccio]

> Sorry, I didn't include his full email,
> which explain his patch quite nicely.  (appended)
>
> In rethinking this, I think we should just add
> a new {u} specifier that would give us the file
> in url-encoded format, including commas.

This would help, but I think the real solution is to not let the shell parse
the arguments.  I don't think there is any reason this is needed.

I don't see why URL-encoding is needed either, when it can be done without
it.  This isn't to say it isn't a reasonable option to provide, I just don't
see it as the proper solution to this problem.

The patch I submitted should handle the commas case although it does take
the code in the wrong direction in terms of funk.

I didn't have any experience with the cvs source and came up with a solution
that worked for my needs.  I think the _right_ answer is to change how the
script is invoked so that the shell isn't used to parse the arguments - but
it wasn't clear to me how to do that without potentially breaking other
things (or spending more time on it than I had).

> In my application, I end up converting it to
> a url form later, because I still have the
> same quoting problem - I'm outputing to a delimited
> file format, so no delimiter is going to be
> perfect.  A {u} specifier would fix it for everyone
> at the source, with only the minor inconvinence
> of url-decoding the file (and directory name)
> later in their application.
>
> And they would only have to do that if they
> needed to, since they could still use the old
> {s} specifier.
>
> I will go ahead an do this, and merge it with
> your {t} specifier code.  May be a couple
> of days before I get to it, tho...

What does the (new) t specifier do?

Any comments on the other two issues I brought up?  The second one I think
has to be addressed.

--robs

>
> Cheers,
> -Russ
>
> (russ.tremain@sun.com)
>
> from:  http://www.mail-archive.com/bug-cvs@gnu.org/msg00975.html
> >From: Rob Saccoccio
> >Subject: cvs patches & notes
> >Date: Fri, 13 Apr 2001 10:03:52 -0700
> >
> >
> >I've identified and fixed (or worked around) some issues with cvs and
> >thought I'd pass 'em along...
> >
> >1. There's a "bogus kludge" in server.c wrapped by "#ifdef sun"
> that applies
> >to pretty old unpatched versions of SunOS.  Maybe an autoconf
> script could
> >be written to test for the condition that causes the problem.  I
> had a look
> >at when the OS patch was first released (and unfortunately don't
> recall the
> >details), but it was years ago.  A reasonable alternative may be
> to assume
> >the kludge isn't needed and suggest that if the problem arises
> the patch be
> >applied or SUNOS_KLUDGE be defined to enable the old behavior.
> >
> >2. When operating in pserver mode, the loginfo scripts are invoked with a
> >file descriptor still open (10) which enables writing back to the client.
> >This is clearly an oversight.  I came across this as I was
> trying to get the
> >server to return without waiting for the loginfo scripts to complete.  In
> >turns out, the server is waiting on the closure of a FD (9) that
> is closed
> >in the helper client, but was passed to its children.  I have a detailed
> >truss if you'd like it.  I couldn't figure out the "right" place to close
> >the FD, so I now close it at the top of my scripts.
> >
> >3. Below is a patch against 1.11 logmsg.c that allows loginfo scripts to
> >properly handle filenames with spaces.  Currently, the arguments are not
> >properly setup to handle this.  The patch is backward compatible.
> >
> >--robs
>
> --- logmsg.c    Fri Apr 13 12:38:43 2001
> +++ new-logmsg.c        Fri Apr 13 12:38:26 2001
> @@ -552,9 +552,9 @@
> * concatenate each filename/version onto str_list
> */
> static int
> -title_proc (p, closure)
> +title_proc (p, quote)
>    Node *p;
> -    void *closure;
> +    void *quote;
> {
>    struct logfile_info *li;
>    char *c;
> @@ -568,8 +568,9 @@
>                You can verify that this assumption is safe by checking the
>                code in add.c (add_directory) and import.c (import). */
>
> -       str_list = xrealloc (str_list, strlen (str_list) + 5);
> +       str_list = xrealloc (str_list, strlen (str_list) + 7);
>         (void) strcat (str_list, " ");
> +       (void) strcat (str_list, (char *)quote);
>
>         if (li->type == T_TITLE)
>         {
> @@ -624,6 +625,8 @@
>                         }
>                 }
>         }
> +
> +       (void) strcat (str_list, (char *)quote);
>    }
>    return (0);
> }
> @@ -711,9 +714,35 @@
>
>        after the format string (we
>
>        might skip a '}') somewhere
>
>        in there... */
> +       char *qp = fmt_percent - 1;     /* a pointer used to find
> quotes */
> +       char *quote = "";
>
>         /* Grab the format string. */
>
> +       /* Backward compatibility kludge.  If the previous non-white space
> +          character is a single quote, its there to cancel out the single
> +          quotes that WERE coded around the repository and changed file
> +          list.  This allowed the shell to break up the args to the
> +          filter program on white space, creating a seperate arg for
> +          each file spec (and the repository).  Since this is the
> +          intended default behaviour, in order to not break existing
> +          scripts we have remove it when its there and put it in
> +          when its not.  This parser really needs to be rewritten.
> +          Another approach would have been to allow the single quote
> +          to be a format character (unrecognized format chars SHOULD
> +          be honored as literals and not ignored), it would break any
> +          existing scripts that were expecting the comma that it is
> +          currently replaced with now. */
> +
> +       /* Walk back from the percent until we hit a non-white
> space char */
> +       while (qp > filter && (*qp == ' ' || *qp == '\t')) --qp;
> +
> +       if (*qp == '\'')
> +       {
> +           quote = "'";
> +           *qp = ' ';
> +       }
> +
>         if ((*(fmt_percent + 1) == ' ') || (*(fmt_percent + 1) == '\0'))
>         {
>                 /* The percent stands alone.  This is an error.  We could
> @@ -760,6 +789,15 @@
>                 fmt_continue = fmt_end;
>         }
>
> +       /* Walk forward from fmt_continue until we hit a non-white space
> char */
> +       qp = fmt_continue;
> +       while (*qp != '\0' && (*qp == ' ' || *qp == '\t')) ++qp;
> +
> +       if (*qp == '\'' && *quote != '\0')
> +       {
> +           *qp = ' ';
> +       }
> +
>         len = fmt_end - fmt_begin;
>         str_list_format = xmalloc (len + 1);
>         strncpy (str_list_format, fmt_begin, len);
> @@ -777,13 +815,13 @@
>         if (str_list_format[0] != '\0')
>         {
>                 type = T_TITLE;
> -           (void) walklist (changes, title_proc, NULL);
> +           (void) walklist (changes, title_proc, quote);
>                 type = T_ADDED;
> -           (void) walklist (changes, title_proc, NULL);
> +           (void) walklist (changes, title_proc, quote);
>                 type = T_MODIFIED;
> -           (void) walklist (changes, title_proc, NULL);
> +           (void) walklist (changes, title_proc, quote);
>                 type = T_REMOVED;
> -           (void) walklist (changes, title_proc, NULL);
> +           (void) walklist (changes, title_proc, quote);
>         }
>
>         free (str_list_format);
> @@ -794,13 +832,15 @@
>
>         prog = xmalloc ((fmt_percent - filter) + strlen (srepos)
>                                         + strlen (str_list) +
> strlen (fmt_continue)
> -                       + 10);
> +                       + 12);
>         (void) strncpy (prog, filter, fmt_percent - filter);
>         prog[fmt_percent - filter] = '\0';
> -       (void) strcat (prog, "'");
> +       if (*quote == '\0') (void) strcat (prog, "'");
> +       (void) strcat (prog, quote);
>         (void) strcat (prog, srepos);
> +       (void) strcat (prog, quote);
>         (void) strcat (prog, str_list);
> -       (void) strcat (prog, "'");
> +       if (*quote == '\0') (void) strcat (prog, "'");
>         (void) strcat (prog, fmt_continue);
>
>         /* To be nice, free up some memory. */
>
> -----
>
>
>