Bug #1319

another undo patch: a feature request and implementation, should it be of interest

Added by joelkp over 5 years ago. Updated over 5 years ago.

Status:ClosedStart date:
Priority:NormalDue date:
Assignee:-% Done:

0%

Category:-
Target version:-

Description

The previous patch I sent came about after fiddling with the undo
utility source for another reason, this being a feature I wanted to
try implementing - no idea if of interest to you; if not, I can always
continue to use it regardless.

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)

The feature (the point of it being usability) is a new option, a dash
followed immediately by one or more digits, this number being the
number of revisions to go back from the present (so -1 matches the
default, -0 the present revision). Each such argument replaces a [-t
transaction-id] argument, so you can use one of either, one of each,
or two of one of them.

So for instance, if you want to diff the revision three versions back
with the one two versions back, you can for example use one of:

undo -3 -d -2 filename
undo -3d2 filename
undo -2D3 filename

These giving the same result. (this - two of these numbers for a
comparison - however, requires the previously sent patch, since it
fixes what seems a bug, namely that a second transaction id -
alternatively number as above, given this patch - is left unused after
being set in main()) This seems to me rather quicker and simpler to do
than using

undo -i

to get a list of transaction ids, then finding and using the relevant
one or two, if accessing revision(s) a given number of versions back
is all you want to do.

undo.patch Magnifier (3.45 KB) joelkp, 03/22/2009 08:17 PM

undo.patch Magnifier (3.6 KB) joelkp, 03/23/2009 01:28 AM

undo.patch Magnifier (4.73 KB) joelkp, 03/29/2009 03:40 AM

undo.patch Magnifier (3.13 KB) joelkp, 03/29/2009 02:50 PM

History

#1 Updated by joelkp over 5 years ago

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.

#2 Updated by dillon over 5 years ago

:> 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

#3 Updated by joelkp over 5 years ago

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] >= '0' && optarg[0] <= '9' &&
                                   optarg[1] != 'x')
                                       flags |= UNDO_FLAG_SETTID1;
                       } else if (ts2.tid == 0 &&
                                  !(flags & UNDO_FLAG_SETTID2)) {
                               ts2.tid = parse_delta_time(optarg);
                               if (optarg[0] >= '0' && optarg[0] <= '9' &&
                                   optarg[1] != 'x')
                                       flags |= UNDO_FLAG_SETTID2;
                       } else
                               usage();
                       break;

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

#4 Updated by dillon over 5 years ago

: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
<>

#5 Updated by joelkp over 5 years ago

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.

#6 Updated by dillon over 5 years ago

: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

#7 Updated by joelkp over 5 years ago

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

#8 Updated by corecode over 5 years ago

committed in 965778c8e5068e125ea2aa029359fdf88d1e5dfc

Also available in: Atom PDF