Re: temp file suffixes: mktemp DWIM

[Jim Meyering]

Eric Blake wrote:

> Jim Meyering <jim <at> meyering.net> writes:
>
>> > mktemp a => error, no run of X
>> > mktemp aXX => error, run of X is too short
>> > mktemp XXX => generates 3-character name (if possible)
>> > mktemp aXXX.b => generates 6-character name (if possible)
>> > mktemp --suffix=.b aXXX => longer spelling of the above line
>> > mktemp aXXX --suffix=.b => likewise, if not POSIXLY_CORRECT
>> > mktemp aXXXbXXX => generates aXXXb123 (only the last run changed)
>> > mktemp --suffix=X aXXX => only way to generate a123X instead of a1234
>> > mktemp --suffix=.txt file => error, no run of X
>> > mktemp --suffix=.txt aXXXb => error, explicit --suffix requires trailing X
>>
>> Nice, comprehensive set of examples.
>
> Here's my first cut at implementing this.
>
> Eric Blake (4):
>       [1/4] build: update gnulib submodule to latest, for tempname changes
> Real gnulib commit id TBD, once I push my pending mkostemps series to gnulib
> (http://thread.gmane.org/gmane.comp.lib.gnulib.bugs/19282).  Note that either
> patch 1 or patch 2 in isolation will cause bootstrap failure; so even though
> you normally like to do gnulib submodule updates in isolation, I'm wondering 
> if
> this is a case where the two patches should be squashed together.

This sounds like a good reason indeed to include the gnulib update
with the dependent change.  FWIW, I'm less averse to including a
gnulib submodule update with other changes, now that I've become
more used to dealing with submodules.

>       [2/4] build: reflect gnulib changes to tempname
> Since gnulib added mkostemps support to gen_tempname, we need to reflect the
> extra parameter in gen_tempname_len.  I think the .diff format makes it easier
> to see how we intentionally differ from glibc, rather than writing a flat-out
> replacement file (it's also more robust at catching subsequent gnulib changes
> to the same file, to let us know to resynchronize).

Yes, I was thinking the same thing.
This would be easier to review if there were two deltas:
- the no-semantic-change, convert-to-.diff one
- the add-argument semantics-changing one
but it's not that big a deal.

Yeah, it's a shame about glibc using the incorrect type
for the new parameter.

>       [3/4] tests: enhance mktemp test
> Add more tests of existing behavior, to ensure I didn't break it, and to make
> it obvious what I'm changing.  I could rebase the series to put this first,
> alongside my pending mktemp doc patch, especially if your review of my doc
> patch and the accompanying email turn up any changes we want to existing
> behavior.

Those new tests look fine.

>       [4/4] mktemp: add suffix handling
> The real fun.  Hopefully I added enough tests to cover everything new that
> results from the new code.  Also, is the rearranged output in usage() okay, 
> and
> should I split that into a separate patch?

I'll probably run out of time here, but will start commenting.
AUTHORS: Glad you added your name.

> In rereading this before hitting the send button, I see an out-of-bounds
> reference in patch 4.  I'll know if you reviewed this if you spot the same
> bug ;)  Also, I guess I should also amend my series to mention this bug report
> in the commit message:
>> > http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=548316

Better to stop now.
Will resume here in the morning and then revisit your doc-change message.