Installation failure caused by CDPATH environment variable

Installation failure caused by CDPATH environment variable

[Wincent Colaiuta]

This week I've installed Git on two different machines and one of  
them mysteriously failed making the "install" target in the  
"templates/Makefile". Specifically, the problem was occuring here:

         (cd blt && $(TAR) cf - .) | \
         (cd '$(DESTDIR_SQ)$(template_dir_SQ)' && $(TAR) xf -)

And the error message was:

tar: This does not look like a tar archive
tar: Skipping to next header
tar: Error exit delayed from previous errors

Upon investigation, I discovered that the cause was that on the  
failing machine, the CDPATH environment variable was set and included  
"." (the current directory). This meant that upon executing:

         (cd blt && $(TAR) cf - .)

The data from tar was getting prepended with garbage because "cd blt"  
was echoing the path to the directory.

The workaround is to "unset CDPATH" or change the value of CDPATH so  
that it doesn't include the "." (although the latter is not very  
feasible because without "." at the front of CDPATH changing into a  
directory relative to the current directory can be quite painful).

What do you think about altering the templates/Makefile to make it  
more robust against this kind of environment?

Here's a possible patch:

 From 057630bdcfeee63b90468d1a69153171b15780c0 Mon Sep 17 00:00:00 2001
From: Wincent Colaiuta <win@wincent.com>
Date: Wed, 11 Jul 2007 18:43:59 +0200

[PATCH] Proof Makefile against possible problems with CDPATH environment

If the CDPATH environment variable is set and contains a period
Bash may echo the current directory name while performing a cd,
and when this extra output is piped to tar as part of template
installation it can cause tar to abort with an error. This patch
avoids this problem by invoking cd as a separate step prior to
invoking tar.

Signed-off-by: Wincent Colaiuta <win@wincent.com>
---
  templates/Makefile |    3 ++-
  1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/templates/Makefile b/templates/Makefile
index aaa39d3..3457ccb 100644
--- a/templates/Makefile
+++ b/templates/Makefile
@@ -46,5 +46,6 @@ clean:

  install: all
         $(INSTALL) -d -m755 '$(DESTDIR_SQ)$(template_dir_SQ)'
-       (cd blt && $(TAR) cf - .) | \
+       cd blt && $(TAR) cf - . | \
         (cd '$(DESTDIR_SQ)$(template_dir_SQ)' && $(TAR) xf -)
+       cd -
-- 
1.5.2.3

Re: Installation failure caused by CDPATH environment variable

[Johannes Schindelin]

Hi,

On Wed, 11 Jul 2007, Wincent Colaiuta wrote:

> [describes the typical CDPATH problem]

You exported CDPATH.  You're guaranteed to run into problems with that.  I 
doubt that your patch catches all problems in git, and even if we tried to 
avoid breakage, you can only do so much about that.

It is _wrong_ to export CDPATH.

Hth,
Dscho

Re: Installation failure caused by CDPATH environment variable

[Junio C Hamano]

Wincent Colaiuta <win@wincent.com> writes:

>  install: all
>         $(INSTALL) -d -m755 '$(DESTDIR_SQ)$(template_dir_SQ)'
> -       (cd blt && $(TAR) cf - .) | \
> +       cd blt && $(TAR) cf - . | \
>         (cd '$(DESTDIR_SQ)$(template_dir_SQ)' && $(TAR) xf -)
> +       cd -

Actually I suspect replacing the "cd blt" in the original with
"cd ./blt", without changing anything else, would squelch the
CDPATH problem.

We could write it as "$(TAR) Ccf blt - ." if we can rely on the
'C' option, but I suspect it is a GNU extension.  Does anybody
have POSIX.1 handy?

Re: Installation failure caused by CDPATH environment variable

[David Kastrup]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> On Wed, 11 Jul 2007, Wincent Colaiuta wrote:
>
>> [describes the typical CDPATH problem]
>
> You exported CDPATH.  You're guaranteed to run into problems with that.  I 
> doubt that your patch catches all problems in git, and even if we tried to 
> avoid breakage, you can only do so much about that.
>
> It is _wrong_ to export CDPATH.

But it is not our job to educate people about that.  So I'd just add
something like

[ "X" = "X$CDPATH" ] || unset CDPATH # ignore braindamaged exports

to the top of possibly affected scripts and be done.

After the first few dozen bug reports (which are just the tip of an
iceberg of people who say "git is crap and fails in strange ways") you
just recognize that you should just make that somebody else's problem.

I've done my fair amount of such educational tasks myself with
packages of mine.  It just gets you annoyed more and more each time,
it is tedious, it gets people get a bad opinion of both your software
_and_ your person (try being friendly after the 20th actually
technically completely unnecessary such request, while basically
explaining that you won't fix it because it is a Darwinian trap for
stupid people and you want evolution to run its course).

A one-liner that makes this somebody else's problem (if at all) is
worth its weight in gold.

Trust me on that.

My personal lowlight in that area are dozens of university and company
document classes derived from some old AMSLaTeX class in prehistoric
times that redefined \endfigure in a way incompatible with quite a few
packages.  Among them preview.sty written by me.

After about 5 bug reports about 5 different classes, explaining what
was wrong and why it broke quite a few packages, not just
preview-latex, and how people should fix it and how they should
complain to the people giving them the package...  preview.sty just
patches the problematic definitions if it has a reasonable guess that
they are in place, and blows out a stern and verbose warning
explaining where to complain and why.

Of course, nobody ever does.

Don't educate people.  Just don't trigger their problems.  Of course,
there are millions of ways of shooting oneself in the foot, but in
this case the same foot has been hit several times already.

-- 
David Kastrup

Re: Installation failure caused by CDPATH environment variable

[Junio C Hamano]

David Kastrup <dak@gnu.org> writes:

> Don't educate people.  Just don't trigger their problems.  Of course,
> there are millions of ways of shooting oneself in the foot, but in
> this case the same foot has been hit several times already.

Yup.  We do exactly that in git-clone, git-sh-setup and
t/test-lib to avoid getting bugged by this stupidity.

Re: Installation failure caused by CDPATH environment variable

[Wincent Colaiuta]

El 12/7/2007, a las 10:34, Junio C Hamano escribió:

> David Kastrup <dak@gnu.org> writes:
>
>> Don't educate people.  Just don't trigger their problems.  Of course,
>> there are millions of ways of shooting oneself in the foot, but in
>> this case the same foot has been hit several times already.
>
> Yup.  We do exactly that in git-clone, git-sh-setup and
> t/test-lib to avoid getting bugged by this stupidity.

El 12/7/2007, a las 9:51, David Kastrup escribió:

> [ "X" = "X$CDPATH" ] || unset CDPATH # ignore braindamaged exports

Whatever decision is taken in the end, I think we should avoid terms  
like "stupidity" and "braindamaged" to avoid causing possible  
offense. Exporting CDPATH is a simple mistake that can be made  
inadvertently or unwittingly, but very easily (Googling for "export  
CDPATH" yields 18,000+ results, many of them purporting to be Bash  
"tutorials").

Cheers,
Wincent

Re: Installation failure caused by CDPATH environment variable

[Johannes Schindelin]

Hi,

[please Cc' me if you already quote me and answer directly to my message]

On Thu, 12 Jul 2007, David Kastrup wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> > On Wed, 11 Jul 2007, Wincent Colaiuta wrote:
> >
> >> [describes the typical CDPATH problem]
> >
> > You exported CDPATH.  You're guaranteed to run into problems with 
> > that.  I doubt that your patch catches all problems in git, and even 
> > if we tried to avoid breakage, you can only do so much about that.
> >
> > It is _wrong_ to export CDPATH.
> 
> But it is not our job to educate people about that.  So I'd just add
> something like
> 
> [ "X" = "X$CDPATH" ] || unset CDPATH # ignore braindamaged exports
> 
> to the top of possibly affected scripts and be done.

We have that already, and we're not done.

> A one-liner that makes this somebody else's problem (if at all) is worth 
> its weight in gold.

That is right.  And a one-liner that pretends to be a solve-all, but is 
not, is worth its weight in lead.

I bet that in ten years we find yet another instance where exporting 
CDPATH, which you should not have done in the first place, breaks Git.

Ciao,
Dscho "who chants wrong, wrong, wrong"

Re: Installation failure caused by CDPATH environment variable

[Johannes Schindelin]

Hi,

On Thu, 12 Jul 2007, Wincent Colaiuta wrote:

> El 12/7/2007, a las 10:34, Junio C Hamano escribi?:
> 
> > David Kastrup <dak@gnu.org> writes:
> > 
> > > Don't educate people.  Just don't trigger their problems.  Of 
> > > course, there are millions of ways of shooting oneself in the foot, 
> > > but in this case the same foot has been hit several times already.
> > 
> > Yup.  We do exactly that in git-clone, git-sh-setup and t/test-lib to 
> > avoid getting bugged by this stupidity.
> 
> El 12/7/2007, a las 9:51, David Kastrup escribi?:
> 
> > [ "X" = "X$CDPATH" ] || unset CDPATH # ignore braindamaged exports
> 
> Whatever decision is taken in the end, I think we should avoid terms 
> like "stupidity" and "braindamaged" to avoid causing possible offense. 
> Exporting CDPATH is a simple mistake that can be made inadvertently or 
> unwittingly, but very easily (Googling for "export CDPATH" yields 
> 18,000+ results, many of them purporting to be Bash "tutorials").

Sorry, my words were indeed to strong.  I apologise.  My only excuse is 
that this crops up ever so often, and it does get slightly annoying.  Of 
course, you did not read all those mails, and so my words were unmerited.  

For an example, see

http://article.gmane.org/gmane.comp.version-control.git/13736/match=cdpath

Ciao,
Dscho

Re: Installation failure caused by CDPATH environment variable

[Uwe Kleine-König]

Hello,

Junio C Hamano wrote:
> We could write it as "$(TAR) Ccf blt - ." if we can rely on the
> 'C' option, but I suspect it is a GNU extension.  Does anybody
> have POSIX.1 handy?
I don't have POSIX.1 handy, but on Solaris 10, you need to say:

	tar cf - -C blt .

(That is, Solaris' tar has the 'C' option, which is quite a good
indication, that it's included in POSIX :-)

Best regards
Uwe

-- 
Uwe Kleine-König

http://www.google.com/search?q=gigabyte+in+bit

Re: Installation failure caused by CDPATH environment variable

[Junio C Hamano]

Uwe Kleine-König <ukleinek@informatik.uni-freiburg.de> writes:

> Junio C Hamano wrote:
>> We could write it as "$(TAR) Ccf blt - ." if we can rely on the
>> 'C' option, but I suspect it is a GNU extension.  Does anybody
>> have POSIX.1 handy?
> I don't have POSIX.1 handy, but on Solaris 10, you need to say:
>
> 	tar cf - -C blt .
>
> (That is, Solaris' tar has the 'C' option, which is quite a good
> indication, that it's included in POSIX :-)

Oh, I cannot resist ;-)

Solaris has unlink(2) capable of removing a directory, but I do
not think it is included in POSIX.

Re: Installation failure caused by CDPATH environment variable

[Wincent Colaiuta]

El 17/7/2007, a las 20:37, Junio C Hamano escribió:

> Uwe Kleine-König <ukleinek@informatik.uni-freiburg.de> writes:
>
>> Junio C Hamano wrote:
>>> We could write it as "$(TAR) Ccf blt - ." if we can rely on the
>>> 'C' option, but I suspect it is a GNU extension.  Does anybody
>>> have POSIX.1 handy?
>> I don't have POSIX.1 handy, but on Solaris 10, you need to say:
>>
>> 	tar cf - -C blt .
>>
>> (That is, Solaris' tar has the 'C' option, which is quite a good
>> indication, that it's included in POSIX :-)
>
> Oh, I cannot resist ;-)
>
> Solaris has unlink(2) capable of removing a directory, but I do
> not think it is included in POSIX.

I'd say the answer is "no", "C" isn't in the POSIX tar specification.

The latest, Version 3, says "the Shell and Utilities volume of IEEE  
Std 1003.1-2001 no longer contains the tar utility" (<http:// 
www.opengroup.org/onlinepubs/009695399/basedefs/tar.h.html>).

And Version 2, which does contain tar, does not list "C" as an option  
(<http://www.opengroup.org/onlinepubs/7990989775/xcu/tar.html>).

In fact, the example they give for moving directory hierarchies is  
exactly this:

   (cd fromdir; tar cf - . ) | (cd todir; tar xf -)

Cheers,
Wincent

Re: Installation failure caused by CDPATH environment variable

[Uwe Kleine-König]

Hello Junio,

Junio C Hamano wrote:
> Uwe Kleine-König <ukleinek@informatik.uni-freiburg.de> writes:
> > (That is, Solaris' tar has the 'C' option, which is quite a good
> > indication, that it's included in POSIX :-)
> 
> Oh, I cannot resist ;-)
> 
> Solaris has unlink(2) capable of removing a directory, but I do
> not think it is included in POSIX.
I don't have access to the POSIX documents, but it would surprise me if
they forbid unlink(2) being able to unlink directories. :-)

You need some effort to support tar -C and to catch directories in
unlink.  So Sun put the effort in tar, because it may be needed to be
conformant to POSIX or some other standard.

Best regards
Uwe

-- 
Uwe Kleine-König

<script>alert("This is a virus for Outlook")</script>