2012/3/10 Pádraig Brady <[email protected]>: > On 03/01/2012 07:44 PM, Jérémy Compostella wrote: >> Hi Pádraig, >> >> Finally, I found time to work again on this patch. I provided it as >> attachment. >> >> I'm not completely satisfied with the documentation part. I've tried to >> be more specific but it becomes quickly complicated. So I get back the >> original explanation which does not completely satisfied me since it >> does not explain the following point: >> - When -a is not specified: >> - output file names are considered exhausted when the first suffix >> character will become 'z'. >> -'z[a-z]' suffixes are never used >> - the fact that suffix length is increased each time the suffix are >> "exhausted". >> But maybe it's not relevant to be so specific. >> >> What is your opinion about this point ? > > Well documentation can easily include too much detail. > But one should try to clarify any ambiguities > a user might be considering. > > I adjusted that documentation as follows: > > @@ -2990,11 +2990,15 @@ The output files' names consist of @var{prefix} > (@samp{> > followed by a group of characters (@samp{aa}, @samp{ab}, @dots{} by > default), such that concatenating the output files in traditional > sorted order by file name produces the original input file (except > -@option{-nr/@var{n}}). If more than 676 output files are required and > -@samp{-a} is not specified, @command{split} uses @samp{zaaa}, > -@samp{zaab}, etc. If the @option{-a} is specified and the output file > -names are exhausted, @command{split} reports an error without deleting > -the output files that it did create. > +@option{-nr/@var{n}}). By default split will initially create files > +with two generated suffix characters, and will increase this width by two > +when the next most significant position reaches the last character. > +(@samp{yz}, @samp{zaaa}, @dots{}). In this way an arbitrary number > +of output files are supported which sort as described above, > +even in the presence of an @option{--additional-suffix} option. > +If the @option{-a} option is specified and the output file names are > exhausted, > +@command{split} reports an error without deleting the output files > +that it did create. I like it.
> I made a few adjustments to the code also. > > Disable this feature if --numeric=FROM is specified, > because these suffixes are not all consecutive > and hence we can't support arbitrary FROM values. > > Disable this feature if a specific number of > files is specified with the -n option. > We already auto select the width appropriately. > > I could find no logic flaws in the implementation in new_file_name(). > Well there was one nit with: > > sufindex = xrealloc (sufindex, suffix_length * sizeof *sufindex); > > That should be as follows to avoid overflow issues: > > sufindex = xnrealloc (sufindex, suffix_length, sizeof *sufindex); Fair point. > While looking at that I noticed a fair bit of overlap > from the existing fixed width suffix initialization, > so I merged the two with a goto. That brought the > new implementation down from 15 to 8 significant lines, > and those 8 are now mostly simple integer width adjustments. > Also by doing this we get to take advantage of the _PC_NAME_MAX > check in this section. I've been formatted by "Please, never use goto keyword in your C program" at university and work too. Indeed, I've thought to a solution like that but I didn't find a really smart solution without the goto keyword. > I also added a test. > > Full patch is attached. > I hope to apply this sometime tomorrow. BTW, reviewing your changes I discovered that when a user specifies a suffix length zero the old behavior was to use the default length of 2 and the new is, of course, the suffix auto length way. However, IMHO, it's dangerous to not raise an error. If a user for example make a script which use split with a shell variable for the suffix_length option it could have the feeling that his script works fine although it provides a suffix length zero. I attached an independent patch since there is no semantic relationship with the current one. Feel free to merge, review or refuse it. Cheers, Jérémy
From 6b6cf2246f400b7f8afbdc8199bf9f892f7167d6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=A9r=C3=A9my=20Compostella?= <[email protected]> Date: Sat, 10 Mar 2012 11:58:19 +0100 Subject: [PATCH] split: suffix length zero is not permitted * src/split.c (main): Raise an error when a user specifies a suffix length of zero. --- src/split.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/src/split.c b/src/split.c index ca5be9f..c50128b 100644 --- a/src/split.c +++ b/src/split.c @@ -1101,7 +1101,7 @@ main (int argc, char **argv) { unsigned long tmp; if (xstrtoul (optarg, NULL, 10, &tmp, "") != LONGINT_OK - || SIZE_MAX / sizeof (size_t) < tmp) + || SIZE_MAX / sizeof (size_t) < tmp || !tmp) { error (0, 0, _("%s: invalid suffix length"), optarg); usage (EXIT_FAILURE); -- 1.7.2.5
