Add abs2rel and rel2abs to libutil and use them in ln to provide the -a and -r options

[julian]

Summary

Allow ln to convert absolute symlinks to relative and vise versa

Add a libary to allow that functionality to be added in other places as well
as well as a man page fo rthem

[julian]

Bapt has convinced me to add these functions to libutil instead of keeping them in libpathconv as they originally came.
The Pathconvert package has been incorporated over the years into many projects.
The simplest way to see how to use this is to imagine how difficult it can be sometimes to generate relative symlinks when installing packages or generated binaries when the source and destination are not fixed relative to each other but expressed as two absolute paths. This becomes trivial by adding the -r option and specifying the available absolute paths. I originally started checking this in as libpathconv, before Bapt got to m. I will remove that at the same time I add this.

[julian]

p.s. I'm still considering line 266/267 in ln.c which came with the original patch, but I have not totally understood yet..

[bapt]

Don't forget to bump the date in ln(1)

I am still wondering if 2 commits would be cleaner? one for the move in libutil and one for ln(1) addition but I have no stong feelings about it

For manpages please use make manlint in the source tree

bin/ln/ln.c

282

strlcpy?

lib/libutil/Makefile

10

No need to bump you ad things not alter some

lib/libutil/abs2rel.3

51

No empty lines in mdoc

80

The .bl ligne 69 is never closed

(+ same issues as I spot in the other manpage)

129

Should be a reference ?Xr realpath 3

131

Same

lib/libutil/abs2rel.c

55

strlcpy?

103

strlcpy?

107

missing paranthesis

lib/libutil/rel2abs.3

28

Un expected quotes
Month should not be shorten version

51

No empty lines in mdoc

80

Code blocks should be surrounded by:

.Bd -literal -offset indent
somecode
.Ed

98

Should be Shigio Yamaguchi Mt An [e-mail]

lib/libutil/rel2abs.c

56

strlcpy?

69

strlcpy?

126

strlcpy?

128

missing parenthesis

lib/libutil/tests/abs2rel.c

53

strlcpy?

[rgrimes]

First off in the unix philosophy /we also be adding a relname in the spirit of
basename and dirname that calls this library so that any shell script or
Makefile, etc could easily add this functionality?

IMHO we should not be adding -a and -r type stuff to install and ln, but instead
processing the arguments before invoking the commands.

First glance at man page should reflect that -a | -r are only applicable
as options to -s as it does for -F: [-L -P -s [-a | -r] [-F]]

Line 293 of ln.1 adding , after -F and making option plural is incorrect.
I think you may of had -a and -r here at one time and removed them.

I do not see logic in the program to check that -a or -r are only accepted if -s is used.

I am going to stop here, as I know there is a way to inline my comments but I do
not know how to do that yet in Phabricator

[ngie]

Please add jilles@ to the CR. I suspect I'll need to convert the tests to ATF/TAP proper, the way they're written (they're simple enough to convert).

lib/libutil/abs2rel.c

43

Is this an intuitive name for the lib call?

lib/libutil/rel2abs.c

43

I'm wondering if this is an intuitive lib call name...

46–48

Single line comments per style(9) are:

/* foo */

lib/libutil/tests/Makefile

6–7

• Please sort this list.
• Please add a _test suffix on the end of the test scripts for consistency with the FreeBSD test suite.

lib/libutil/tests/abs2rel.c

33

Sort headers?

lib/libutil/tests/pathconvtest.sh

1

If phabricator was complaining about you adding a +x file without a shebang, you could technically remove the shebang and chmod -x the file, since the install target will add the appropriate +x bits.

[julian]

In D9917#204859, @rgrimes wrote:

First off in the unix philosophy /we also be adding a relname in the spirit of
basename and dirname that calls this library so that any shell script or
Makefile, etc could easily add this functionality?

there are two programs in the tests directory, rel2abs and abs2rel that could be used fo rthis should we decide to promote them from test to real.

IMHO we should not be adding -a and -r type stuff to install and ln, but instead
processing the arguments before invoking the commands.

need the funcitonality else where and it's already part of ln in other systems
see: https://www.gnu.org/software/coreutils/manual/html_node/ln-invocation.html

‘-r’
‘--relative’

Make symbolic links relative to the link location.

Example:

ln -srv /a/file /tmp
'/tmp/file' -> '../a/file'

First glance at man page should reflect that -a | -r are only applicable
as options to -s as it does for -F: [-L -P -s [-a | -r] [-F]]

Line 293 of ln.1 adding , after -F and making option plural is incorrect.
I think you may of had -a and -r here at one time and removed them.

yep
will fix

I do not see logic in the program to check that -a or -r are only accepted if -s is used.

accepted but ignored
if (sflag && (aflag || rflag)) {

I am going to stop here, as I know there is a way to inline my comments but I do
not know how to do that yet in Phabricator

click on the line I think

[ngie]

This comment has been deleted.

lib/libutil/tests/Makefile

6–7

Oh... I missed the fact that these are helper programs used by the tests written below. kyua won't be able to run these programs as-is because they don't conform to any expected formats.

Generally speaking, if I go down this path I do something like this:

# BINDIR= ${TESTSDIR} will need to be defined if it isn't already.

PROGS+= foo_test_helper
TAP_TESTS_SH+= foo_test # or ATF_TESTS_SH or PLAIN_TESTS_SH.

foo_test (in the example above) needs to reference foo_test_helper like "$(dirname "$0")/foo_test_helper".

Also, given that these tests aren't written in ATF or TAP format, they need to use PLAIN_TESTS_SH (PLAIN format is simple -- zero -> pass; non-zero fail... it might not give you the granularity you might like from your test results though).

[ngie]

In D9917#204874, @julian wrote:

In D9917#204859, @rgrimes wrote:

First off in the unix philosophy /we also be adding a relname in the spirit of
basename and dirname that calls this library so that any shell script or
Makefile, etc could easily add this functionality?

there are two programs in the tests directory, rel2abs and abs2rel that could be used fo rthis should we decide to promote them from test to real.

IMHO we should not be adding -a and -r type stuff to install and ln, but instead
processing the arguments before invoking the commands.

need the funcitonality else where and it's already part of ln in other systems
see: https://www.gnu.org/software/coreutils/manual/html_node/ln-invocation.html

‘-r’
‘--relative’

Make symbolic links relative to the link location.

Example:

ln -srv /a/file /tmp
'/tmp/file' -> '../a/file'

Does install -l rs not work as advertised? It seems like it should according to what's being sold in man 1 xinstall.

First glance at man page should reflect that -a | -r are only applicable
as options to -s as it does for -F: [-L -P -s [-a | -r] [-F]]

Line 293 of ln.1 adding , after -F and making option plural is incorrect.
I think you may of had -a and -r here at one time and removed them.

yep
will fix

I do not see logic in the program to check that -a or -r are only accepted if -s is used.

accepted but ignored
if (sflag && (aflag || rflag)) {

I am going to stop here, as I know there is a way to inline my comments but I do
not know how to do that yet in Phabricator

click on the line I think

[rgrimes]

bin/ln/ln.1

44

Change this line to:
.Op Fl L | Fl P | Fl s Op Fl a OP Fl r Op Fl F

45

Remove this line

52

Same edit as @44

166

determines

173

If we are going to add details about implementation perhaps mention ", or creates a loop in the hierarchy" as another unchecked and normally undesirable thing.

317

This is the remains of some other edit, please remove this change

bin/ln/ln.c

66

Is there a white space issue here, or is this an artifact in Phabricator?

116

I would prefer to check if rflag is set here and if so error out, but thats just me wanting binaries to complain about badly constructed input.
case 'a':

if(rflag)  usage();
aflag = 1;

133

Same as at 116

252

add following this

if (aflag | rflag) usage();

408

that should read [-a|-r]

lib/libutil/abs2rel.c

43

What are the names of the functions used to implement this same functionality in install(8), and can this code be refactored and shared?

lib/libutil/rel2abs.c

46–48

/*
 * VERY important single-line comments look like this.
 */

[julian]

will work on this this a bit more this weekend

[asomers]

bin/ln/ln.1

116

s/ralative/relative

bin/ln/ln.c

116

I tend to agree. There's no legitimate use case for using both flags in a single invocation, so it should be an error.

lib/libutil/abs2rel.3

28

You should update the date.

98

A .Sh HISTORY section perhaps? It might be interesting, since this code has apparently been floating around for 20 years.

lib/libutil/rel2abs.c

120

As @bapt would say, "strlcpy?"

lib/libutil/tests/pathconvtest.sh

88

Kyua tests shouldn't write files with absolute paths. They should write files in the pwd instead. Kyua will ensure that it's somewhere under TMPDIR

[wblock]

bin/ln/ln.1

119

(Not yours, yes, I know.)
s/may/can/

It currently reads like it might happen...

173

Needs a comma after the first "specified" and should not use it redundantly but with a different meaning:

option is specified, the link is created exactly as requested without checking that the target exists.

299

(Also not yours, I know.)
s/needs to/must/

"needs to" is vague. Manual pages "need to" make sense, but sometimes use vague and meaningless phrases.

lib/libutil/abs2rel.3

55

Do not use contractions: https://www.freebsd.org/doc/en_US.ISO8859-1/books/fdp-primer/writing-style-guidelines.html
Can also be simpler:

function does not check whether any path exists.

59

Here, it is "path name". Below, it is "pathname". I suggest that the second (compound) version is correct, but whichever is preferred should be used consistently.

98

Agreed. sevan@ has done some work on that and might be able to make suggestions.

106

The blank lines should not be in the source. See note below from bapt about .Bd/.Ed.

132

s/exist/must already exist/

lib/libutil/rel2abs.3

52

The meaning of "may" is not clear here. Probably better as "can".

54

Do not use contractions, "or not" is not helping:

does not check whether any path exists.

58

As above, be consistent with "path name" or "pathname". (Hint: pathname is better, a compound word that has a specific meaning. "Path name" could be "Fred's path", the path he takes when walking to feed the ducks.)

60

If/then sentences have pauses and are kind of halting to read. Many times, the pauses can be removed with clarity remaining as good or even improving by reversing them:

.Dv Null
is returned if an error occurs.

Once again, Perl postfix notation shows the way.

64

Again, "may" implies permission and the meaning here is not clear. I suspect this is not trying to say that rel2abs has permission to fail, but rather that it might fail. So:

The external variable
.Va errno
is set to indicate the error if
.Fn rel2abs
fails.

71

Do not use contractions, be consistent with "pathname".

77

There ought to be a better, more concrete way of saying (length of the pathname) + 1. Here, it can be read as length(pathname+1).

80

Yes. This can be used to remove the blank lines below, also.