Page MenuHomeFreeBSD

bin/cp: make cp src/ equivalent to cp src
Needs ReviewPublic

Authored by alex_kaworu.ch on Tue, Sep 8, 12:14 PM.

Details

Reviewers
bapt
emaste
Group Reviewers
manpages
Summary

Before this patch, cp -R src/ dst is equivalent to cp -R src/. dst. This patch make it so cp -R src/ dst is now equivalent to cp -R src dst.

Historically OpenBSD has opted this patch behaviour a long time ago, we elected to document the trailing slash, and NetBSD has changed to this patch behaviour as well.

Because other implementations have opted for the proposed behaviour, and shells (e.g. bash) may autocomplete directories with a trailing slash, the current implementation may cause both scripting portability issues and interactive misuse.

In all the src/ tree only two instances of cp -R needed to be adapted.

Diff Detail

Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

alex_kaworu.ch created this revision.Tue, Sep 8, 12:14 PM
alex_kaworu.ch requested review of this revision.Tue, Sep 8, 12:14 PM
This comment was removed by alex_kaworu.ch.

Update date in cp.1

bapt added a reviewer: emaste.Tue, Sep 8, 12:26 PM
danfe added a subscriber: danfe.Tue, Sep 8, 12:33 PM

This patch make it so cp -R src/ dst is now equivalent to cp -R src dst.

I find the rationale for this change pretty weak and dubious. I often use both forms precisely because they work differently. I'd also expect that lots of ports would get broken by this change.

What does cp in Linux distros do?
If this goes in we should probably also call out the change in the man page - something along the lines of "FreeBSD versions prior to ... behaved ..."

What does cp in Linux distros do?
If this goes in we should probably also call out the change in the man page - something along the lines of "FreeBSD versions prior to ... behaved ..."

GNU cp behave as the proposed patch. Will do the manpage change if this is accepted.

alex_kaworu.ch added a comment.EditedTue, Sep 8, 8:17 PM

This patch make it so cp -R src/ dst is now equivalent to cp -R src dst.

I find the rationale for this change pretty weak and dubious. I often use both forms precisely because they work differently. [...]

There is no loss of functionality as cp src/. dst is still possible, documented, and supported by other implementations. Thus, one can still use both form after this patch although it'll sometimes require to type one more character (which sounds reasonable).

[...] I'd also expect that lots of ports would get broken by this change.

This is an interesting claim.

I think we can agree that no program ported to FreeBSD is aware of how our cp differ from other implementations in that regards. Even if they did, what would they do about it? cp -R src/. dst on others OSes and cp -R src/ dst on FreeBSD make no sense as the former works on FreeBSD as well. cp -R src/ dst on others OSes and cp -R src dst on FreeBSD make no sense as the latter works on other implementations. Most likely, cp -R src/* dst and respectfully cp -R src dst are used instead, both unaffected by this patch. On the other hand, ported applications might use cp -R src/ dst (wrongly) expecting to behave the same on all implementations. So I would argue that, from a ports perspective, this patch is actually an improvement over the status quo because of the increased compatibility.

Now granted this patch might break some port compat goo that has been written for FreeBSD. Running git grep 'cp.*-[Rr]' on the ports tree shows this:


I've filtered manually the false positive and here are the cp -R|-r invocations:

We see that, as expected earlier, the vast majority are either cp -R src dst or cp -R src/* dst. Only a few exceptions stand out:

  1. This one is using the cp -R src/. dst form, it won't be affected by this diff:
devel/log4cpp/files/patch-doc_Makefile.in:-	cp -r html/. $(docdir)
devel/log4cpp/files/patch-doc_Makefile.in:+	cp -r html/. $(DESTDIR)$(docdir)
  1. Both theses files were written for FreeBSD and use the cp -R src/ dst form. Updating them as cp -R src/. dst will be trivial:
sysutils/cpupdate/files/cpupdate.in:    cp -Rp $_d/ $_dst
sysutils/cpupdate/files/cpupdate.in:    [ -d $_d-with-caveats ] && cp -Rp $_d-with-caveats/ $_dst
textproc/s5/files/s5.in:	${NOOP} cp -Rp ${VERBOSE:+'-v'} "${S5_TEMPLATE}"/ "$1"/
  1. Finally, here are examples where the ported applications expect the behaviour proposed by this diff, and we have to maintain patches for them:
multimedia/lives/Makefile:	@${REINPLACE_CMD} '/cp -rf/ s|data/|data|g; /cp -rf/ s|icons/|icons|g' \
multimedia/lives/files/patch-lives-plugins_weed-plugins_gdk_Makefile.am:-	cp -rf data/ "$(DESTDIR)$(fxpluginslibdir)" && chmod -R a+r "$(DESTDIR)$(fxpluginslibdir)/data/"
multimedia/lives/files/patch-lives-plugins_weed-plugins_gdk_Makefile.am:-	cp -rf icons/ "$(DESTDIR)$(fxpluginslibdir)" && chmod -R a+r "$(DESTDIR)$(fxpluginslibdir)/icons/"
multimedia/lives/files/patch-lives-plugins_weed-plugins_gdk_Makefile.am:+	cp -rf data "$(DESTDIR)$(fxpluginslibdir)" && chmod -R a+r "$(DESTDIR)$(fxpluginslibdir)/data"
multimedia/lives/files/patch-lives-plugins_weed-plugins_gdk_Makefile.am:+	cp -rf icons "$(DESTDIR)$(fxpluginslibdir)" && chmod -R a+r "$(DESTDIR)$(fxpluginslibdir)/icons"
x11-themes/plasma5-breeze-gtk/files/patch-src_build__theme.sh:-cp -r assets/ "${INSTALL_TARGET}"
x11-themes/plasma5-breeze-gtk/files/patch-src_build__theme.sh:+cp -r assets "${INSTALL_TARGET}"

For completness, here are the same files for the src tree:


The two interesting cases are adressed by this diff already:

libexec/rc/rc.initdiskless:	    cp -Rp $j/ /$subdir
tools/regression/msdosfs/msdosfstest.sh:cp -R /usr/src/bin/ /tmp/msdosfstest/