Add UserHomeContract as inverse of UserHomeExpand

[fingolfin]

AI assistance: Codex implemented the function, tests, and docs.

[reiniscirpons]

Hi Max, looks good. One minor improvement below and one minor concern: what if the GAPInfo.UserHome variable had a trailing slash? I think with the current implementation we are in trouble:

gap> GAPInfo.UserHome := "/tmp/gap-home/";;
gap> UserHomeContract("/tmp/gap-home/.gap");
~.gap

This is of course not a concern for the expand function since slashes get undoubled in paths.

I can't seem to find any documentation on GAPInfo.UserHome on whether or not trailing slashes are permitted. Certainly gap doesn't complain if I set my userhome with a trailing slash. It seems they are uncommon for the $HOME env variable, but also not strictly prohibited. Probably best to add a check in this function just in case, also I don't know if this is a worry at all on Windows.

[fingolfin]

All excellent points, thanks @reiniscirpons. I also think the existing UserHomeExpand is somewhat incomplete, it does not handle ~USERNAME/path correctly.

[fingolfin]

@reiniscirpons PR updated

[fieker]

As a side remark: UserHomeContract is a very unfortunate name... maybe
use UserHomeCompress or add a directory (UserHomeDirectoryCompress...).
I was wondering what contractual issues Gap might have with me...

My 2 cents

[fingolfin]

Well it was meant to be the "opposite of UserHomeExpand, and also was inspired by Julia's contractuser (which is slightly better due to the VERB-NOUN order).

But I tend to agree...

[fingolfin]

BTW UserHomeCompress is likewise unfortunate ("wait, will this compress my home dir? neat! or ... dangerous???")

Maybe should just call this ContractUserHomeInPathand rename the old one toExpandUserHomeInPath` (with a old name kept around as a synonym forever)

[reiniscirpons]

Not sure if all the changes are necessary (e.g. I don't know if we need to canonicalize all the paths), also some minor code improvements with respect to reducing the number of list manipulations.

[reiniscirpons]

Do we need all this logic for expand? You mention that

I also think the existing UserHomeExpand is somewhat incomplete, it does not handle ~USERNAME/path correctly.

But this change is orthogonal, right?

Multiple slashes are already treated as a single slash by unix-based systems (see https://unix.stackexchange.com/a/1919 for an overview and references to the spec), so this seems a bit of a redundant change.

[reiniscirpons]

Its not clear we need to "canonicalize" in the suffix since repeated separators are fine.

Even so, ideally we would do this without repeatedly removing from the front, perhaps something like (modulo off by one errors):

Suggested change

	# Canonicalize redundant separators right after the home-directory boundary.
	suffix := str{[homeLen + 2..Length(str)]};
	while Length(suffix) > 0 and suffix[1] = '/' do
	Remove(suffix, 1);
	od;
	# Canonicalize redundant separators right after the home-directory boundary.
	suffix := "";
	for idx in [homeLen+2 .. Length(str)] do
	if str[idx] <> '/' then
	suffix := str{[idx..Length(str)]};
	break;
	fi;
	od;

Maybe factor out into a dedicated "CanonicalizePath" function, which could handle other quirks of the path specification too? But I think this might be too much effort for not that much gain.

[fingolfin]

Well, yeah, this worries about e.g. replacing $HOME// by just ~ instead of ~// (just like $HOME and $HOME/ both become ~). But since in the end both ~// and ~ refer to the same dir, I don't have a problem with not "normalizing" here.

[reiniscirpons]

Suggested change

	## A trailing slash in <C>GAPInfo.UserHome</C> is ignored when contracting
	## Any trailing slashes in <C>GAPInfo.UserHome</C> are ignored when contracting

[reiniscirpons]

Suggested change

	## A trailing slash in <C>GAPInfo.UserHome</C> is ignored when expanding
	## Any trailing slashes in <C>GAPInfo.UserHome</C> are ignored when expanding

but I'm not sure we need to remove the trailing slashes here, see comment below.

[reiniscirpons]

Minor comment, but do we need to do a copy and repeated remove here? We could do this by just counting down from the end to get the last index without a '/' and then do a copy at the end, e.g.:

Suggested change

	home := ShallowCopy(GAPInfo.UserHome);
	while Length(home) > 1 and Last(home) = '/' do
	Remove(home);
	od;
	homeLen := Length(home);
	homeLen := Length(GAPInfo.UserHome);
	while homeLen > 1 and GAPInfo.UserHome[homeLen] = '/' do
	homeLen := homeLen - 1;
	od;
	home := GAPInfo.UserHome{[1 .. homeLen]};

Of course, this is unlikely to be a performance bottleneck, but good to avoid unnecessary work where possible.

[fingolfin]

Removing from the end is super fast, it basically only needs to decrement the string length. Besides, as you point out, usually this only runs 0 or 1 times.

Alternatively we could also decide to just store the UserHome in "canonical" or "normalized" format, i.e., with trailing slashes removed (strictly speaking, one would expect more than that for a true "canonical" name; so I'd not call it that. I'd just write that it is always without a trailing slash, unless it is / itself (which... really shouldn't be the UserHome, ever... ugh)

[reiniscirpons]

Good catch! I was a bit confused about what it was doing at first, maybe add a comment:

Suggested change

	if str[homeLen + 1] <> '/' then
	return str;
	fi;
	# Check that the string starts with Concatenation(GAPInfo.UserHome, "/")
	# not just GAPInfo.UserHome which could alias in case it is a prefix of another directory
	if str[homeLen + 1] <> '/' then
	return str;
	fi;

[fingolfin]

@reiniscirpons like this maybe:

Suggested change

	if str[homeLen + 1] <> '/' then
	return str;
	fi;
	# Check that the string starts with Concatenation(GAPInfo.UserHome, "/") not
	# just GAPInfo.UserHome. Otherwise if `GAPInfo.UserHome` is for example
	# `/home/john` but str is `/home/johnny` we'd end up with `~ny`).
	if str[homeLen + 1] <> '/' then
	return str;
	fi;

[fingolfin]

@reiniscirpons so we'd basically just remove this:

Suggested change

	while Length(suffix) > 0 and suffix[1] = '/' do
	Remove(suffix, 1);
	od;
	if Length(suffix) = 0 then
	return "~";
	fi;