Page MenuHomeFreeBSD
Differential D3200

Replace GNU RCS ident with a BSD license ident
ClosedPublic
Actions

Authored by bapt on Jul 25 2015, 3:17 PM.
Tags
None
Referenced Files
Unknown Object (File)
Sun, Apr 21, 9:47 PM
Unknown Object (File)
Sun, Apr 21, 9:47 PM
Unknown Object (File)
Sun, Apr 21, 9:47 PM
Unknown Object (File)
Sun, Apr 21, 9:47 PM
Unknown Object (File)
Sun, Apr 21, 9:47 PM
Unknown Object (File)
Sun, Apr 21, 9:47 PM
Unknown Object (File)
Apr 18 2024, 3:54 PM
Unknown Object (File)
Dec 28 2023, 9:16 AM
View All Files
Subscribers
eadler
imp
wblock

Details

Reviewers
emaste
pfg
delphij
Group Reviewers
manpages
Commits
rS285890: Replace GNU RCS ident with a BSD license ident
Summary

100% compatible with GNU RCS ident version in base with the exception of the -V
version which output nothing in the new version)
100% compatible with GNU RCS 5.9.4 with the exception of long options which has
not been added.
Passes all regression tests from GNU RCS 5.9.4
Support svn extension for the Keyword id (double dots and # before last $)

Rationale: lots of scripts are using ident(1) but if base is built WITHOUT_RCS
then those scripts become unusable.

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint Warnings
SeverityLocationCodeMessage
Warningusr.bin/ident/tests/ident.sh:CHMOD1Invalid Executable
Unit
No Test Coverage

Event Timeline

bapt updated this revision to Diff 7297.Jul 25 2015, 3:17 PM
bapt retitled this revision from to Replace GNU RCS ident with a BSD license ident.
bapt updated this object.
bapt edited the test plan for this revision. (Show Details)
bapt added reviewers: emaste, delphij.
Herald added a reviewer: manpages. · View Herald TranscriptJul 25 2015, 3:17 PM
Herald added a subscriber: imp. · View Herald Transcript
emaste added inline comments.Jul 25 2015, 3:28 PM
usr.bin/ident/ident.1
47

s/it/are/

52

"composed only" and "in the C locale"

bapt updated this revision to Diff 7298.Jul 25 2015, 3:32 PM
bapt edited edge metadata.

fix manpage

bapt marked 2 inline comments as done.Jul 25 2015, 3:32 PM

@emaste comments addressed

delphij requested changes to this revision.EditedJul 25 2015, 7:07 PM
delphij edited edge metadata.

Please see my inline comments. The first one is more important (although I don't see it a big problem in practice), the other two are cosmetic.

In additional to that, looking at your code I found another bug and here is the test case:

# $$FreeBSD: head/COPYRIGHT 276462 2014-12-31 10:00:43Z bz $
usr.bin/ident/ident.c
105

I think this is wrong (although the impact is probably small). If we have more than INTMAX worth of ID's, eventually we would get an overflow and therefore get wrong result.

If I was you I would use a boolean or consider nbid as a boolean here, initialize as false, and |= each time, because it's all we care for the purpose of -q and we don't care the count.

113

Maybe errx?

139

It's not common for system utilities to use errx to give usage information. Is this to match GNU behavior?

(Note that new GNU RCS (5.9.4) only gives the standard invalid option and not usage.)

This revision now requires changes to proceed.Jul 25 2015, 7:07 PM
eadler added a subscriber: eadler.EditedJul 25 2015, 7:13 PM

meta comment: please support long options. we don't have to block the import on them, but we shouldn't remove that functionality

bapt updated this revision to Diff 7325.Jul 25 2015, 7:52 PM
bapt edited edge metadata.

Avoid integer overflow
Handle $ in the keyword as a reinitialisation of the keyword to match GNU ident behaviour

bapt marked an inline comment as done.Jul 25 2015, 7:55 PM
bapt added inline comments.
usr.bin/ident/ident.c
115

Nope because that will make a GNU ident regression test fail because in this particular case
GNU ident has no colon separation between the program name and the warning word

141

Yes I figured that one out, but did not know which one I should mimic: newer version or the one we have in base

bapt added a comment.Jul 25 2015, 7:57 PM
In D3200#64100, @eadler wrote:

meta comment: please support long options. we don't have to block the import on them, but we shouldn't remove that functionality

Long option support is undocumented upstream, neither available in base version. only because of --quiet and --help will not imho really make sense.

Note that in recent rcs version -V is deprecated and replaced by --version but none of this option make sense in base

wblock added a subscriber: wblock.Jul 26 2015, 12:57 AM
wblock added inline comments.
usr.bin/ident/ident.1
38

Argument name is missing:

.Op Ar files

44

Does not need "the":

in

47

Capitalize "if":

If no arguments are passed, then

49

passive->active:

parses the standard input.

52

Too many "only"s:

must only be composed of alphanumeric values in the C locale, followed by

54

No "then":

a space.

56

Duplicate .Pp, remove one.

57

s/The following/These/

62

Do the same thing as the previous sentence:

Do nothing, added for compatibility with GNU ident.

bapt updated this revision to Diff 7337.Jul 26 2015, 1:10 AM
bapt edited edge metadata.

Address @wblock remarks

bapt marked 7 inline comments as done.Jul 26 2015, 1:11 AM
bapt added inline comments.
usr.bin/ident/ident.1
38

Nope this is magic :)

.Op Ar
Will generate:
[file ...]

automagically

54

No it will be "followed by ':' then a space.

bapt added a reviewer: pfg.Jul 26 2015, 1:27 AM
wblock added inline comments.Jul 26 2015, 1:53 AM
usr.bin/ident/ident.1
39

But later you refer to this argument as files. There might be a precedent for how to handle that. On line 44 below, this markup might work:

.Ar file Ns s .

55

Okay, replace "then" with "and":

and a space.

Rendering as:
must only be composed of alphanumeric values in the C locale, followed by `:' and a space.

pfg accepted this revision.Jul 26 2015, 3:23 AM
pfg edited edge metadata.

I like it, thanks!
I agree the long options are unnecessary.

FWIW, the OpenBSD version uses these defines, but I don't see that as a showstopper.

#define KEYDELIM	'$'	/* keywords delimiter */
#define VALDELIM	':'	/* values delimiter */
Closed by commit rS285890: Replace GNU RCS ident with a BSD license ident (authored by bapt). · Explain WhyJul 26 2015, 11:21 AM
This revision was automatically updated to reflect the committed changes.
swills added a subscriber: swills.Jul 26 2015, 11:58 AM
swills removed a subscriber: swills.

Revision Contents
Changeset List

PathSize
gnu/
usr.bin/
rcs/
2 lines
tools/
build/
mk/
2 lines
usr.bin/
1 line
13 lines
69 lines
163 lines
11 lines
14 lines
12 lines
3 lines

Diff 7298

View Options

gnu/usr.bin/rcs/Makefile

Loading...
View Options

gnu/usr.bin/rcs/ident/

Loading...
View Options

gnu/usr.bin/rcs/ident/Makefile

Loading...
View Options

gnu/usr.bin/rcs/ident/Makefile.depend

Loading...
View Options

gnu/usr.bin/rcs/ident/ident.1

Loading...
View Options

gnu/usr.bin/rcs/ident/ident.c

Loading...
View Options

tools/build/mk/OptionalObsoleteFiles.inc

Loading...
View Options

usr.bin/Makefile

Loading...
View Options

usr.bin/ident/

Loading...
View Options

usr.bin/ident/Makefile

Loading...
View Options

usr.bin/ident/ident.1

Loading...
View Options

usr.bin/ident/ident.c

Loading...
View Options

usr.bin/ident/tests/

Loading...
View Options

usr.bin/ident/tests/Makefile

Loading...
View Options

usr.bin/ident/tests/ident.sh

Loading...
View Options

usr.bin/ident/tests/test.in

Loading...
View Options

usr.bin/ident/tests/test.out

Loading...
View Options

usr.bin/ident/tests/testnoid

Loading...