DragonFlyBSD

I did mess something up, namely parameter handling of new option in
main() - the kludge around getopt was incomplete, and so, while the
use examples in the previous mail would work, reordering them like
this wouldn't:

undo -d -3 -2

Instead, that became equivalent to:

undo -d -32

This fixed in new patch.

:> This patch does not change the usage notice nor documentation, so this
:> remains to do. (and perhaps stylistic or other changes if the code is
:> not up to your standards or I messed up somewhere)
:
:I did mess something up, namely parameter handling of new option in
:main() - the kludge around getopt was incomplete, and so, while the
:use examples in the previous mail would work, reordering them like
:this wouldn't:
:
:undo d -3 -2
:
:Instead, that became equivalent to:
:
:undo -d -32
:
:This fixed in new patch.
:
:-
: - Joel K. Pettersson

Hmm.

Ok, the -t ts1/ts2 fixes look good.

The 0-9 handling needs some help.  A lot of help.  It's just too messy.
    I think it needs to be handled either by giving it its own option 
    (instead of trying to parse 0-9 in getopt), or by integrating the index
    selection with -t by detecting that the TID values are not in 0x form.

My preference is to simply make -t accept non 0x form transaction ids
    as indices instead of transaction ids.

-Matt

I'm not too surprised.

I guess trying to use immediate number options with multiple
characters is simply not possible to get clean as opposed to merely
working when getopt is used for options. While slightly more verbose
in usage, the code for using a standard option with argument is rather
much nicer, yes.

Should I send such a patch? Alternatively, if you copy the main()
function from before the feature patch, move the initiation of the
"flags" variable to before the option-reading loop, then such code can
simply be inserted for case 't'.

With a simple test for index numbers, though it may be a bit
quick-and-dirty, here's such code:
               case 't':
                       if (ts1.tid == 0 &&
                           !(flags & UNDO_FLAG_SETTID1)) {
                               ts1.tid = parse_delta_time(optarg);
                               if (optarg⁰ >= '0' && optarg⁰ <= '9' &&
                                   optarg¹ != 'x')
                                       flags |= UNDO_FLAG_SETTID1;
                       } else if (ts2.tid == 0 &&
                                  !(flags & UNDO_FLAG_SETTID2)) {
                               ts2.tid = parse_delta_time(optarg);
                               if (optarg⁰ >= '0' && optarg⁰ <= '9' &&
                                   optarg¹ != 'x')
                                       flags |= UNDO_FLAG_SETTID2;
                       } else
                               usage();
                       break;

Where the code for detecting non-TIDs may be replaced, if needed.

:orm.
:I guess trying to use immediate number options with multiple
:characters is simply not possible to get clean as opposed to merely
:working when getopt is used for options. While slightly more verbose
:in usage, the code for using a standard option with argument is rather
:much nicer, yes.
:
:...
:Should I send such a patch? Alternatively, if you copy the main()
:function from before the feature patch, move the initiation of the
:"flags" variable to before the option-reading loop, then such code can
:simply be inserted for case 't'.

Send a complete patch w/ everything, using the new scheme, and I'll
    commit it.

I'm not sure the logic you posted will properly sequence ts1 and
    ts2.  I suggest just tracking the sequencing separately instead
    of using ts1.tid and the flags variable.

Then for updating the flags just pass additional arguments
    to parse_delta_time() and let parse_delta_time() make the
    distinction and adjust the flags  e.g.:

case 't':
    ++count_t;
    if (count_t == 1)
        ts1.tid = parse_delta_time(optarg, &flags, UNDO_FLAG_SETTID1);
    else
        ts2.tid = parse_delta_time(optarg, &flags, UNDO_FLAG_SETTID2);
    else
        usage();
    break;
    ...

Or something like that.

-Matt
                    Matthew Dillon 
                    <dillon@backplane.com>

Here goes. Also includes the changes in the earlier bugfix patch (for
making ts2 be used at all after being set in main()) from the mail
before this whole thread, so if committed, both (fix and feature) are
resolved.

Changed accordingly.

:Here goes. Also includes the changes in the earlier bugfix patch (for
:making ts2 be used at all after being set in main()) from the mail
:before this whole thread, so if committed, both (fix and feature) are
:resolved.
:
: - Joel K. Pettersson

It looks very nice.  I have committed it along with some additional
    augmentation of the documentation.  I also adjusted the code so if
    two -t options are given it automatically defaults to DIFF mode (-d),
    unless overridden.

-Matt

On another note, a second feature addition for the index parameter
occurred to me, which became a very simple matter with the new code -
making a negative index do something, namely go forward from the
oldest revision.

While not as significant from a usability point as the previous
addition, perhaps the best point of it is a quick way of accessing the
oldest, for example diffing it with the newest, as in:

undo -dt-0 filename

This patch, unlike that of the previous addition, has documentation
updates, so should be a quick matter if of interest.

- Joel K. Pettersson

committed in 965778c8e5068e125ea2aa029359fdf88d1e5dfc