fix for symlink-in-PWD problem

[ In reference to
http://maillist.perforce.com/pipermail/p4ruby/20
08q1/000115.html ]

I finally got P4Win cooperating with cygwin-p4ruby via
listing my
cygwin path in AltRoots, however I ran into a bit of
confusion along
the way.  My client root is /home/me/some/long/path, which I
have
symlinked to /home/me/work.  Apparently this causes a glitch
since the
shell sets PWD to /home/me/work instead of the full path:

http://kb.perforce.com/UserTasks/ConfiguringP4/Symbo
licLinks

Note the Perforce authors decided not to solve the problem:
"However,
this doesn't necessarily fix the issue of symlinks under the
Perforce
client root. For that, you will still need to either set and
export
the $PWD environment variable, or use the -d argument as
necessary."

May I suggest the following patch to workaround it:

require 'p4'
require 'pathname'

class P4
   class << self
      alias_method :previous_new, :new
      def new(*args, &block)
         previous_pwd = ENV["PWD"]
         begin
            ENV["PWD"] =
Pathname.new(previous_pwd).realpath
            previous_new(*args, &block)
         ensure
            ENV["PWD"] = previous_pwd
         end
      end
   end
end

I am using p4ruby-2007.3 with cygwin p4api-2007.3.143793.

IMO Perforce should expand away the symlink, but until then
at least
P4Ruby can do it.  Please spare others from this needless
pain!
_______________________________________________
p4ruby mailing list
p4rubyperforce.com

http://maillist.perforce.com/mailman/listinfo/p4ruby

The easier workaround is:

	ENV.delete( "PWD" )

If PWD isn't present in the environment, then the Perforce
API will fall back 
on using the result of getcwd(), which expands your symlink
as required.

Note that the PWD environment variable is intended to
supress expansion of 
symlinks. Back in the early 90's it used to drive me nuts
when I cd'd across 
a symlink and couldn't get back using 'cd ..'. PWD was the
solution that the 
shells adopted for this and Perforce is simply following the
convention.

Most of the time that's what's wanted. When it isn't,
deleting PWD from the 
environment usually solves the problem.

Hope that helps!

Tony

On Thursday 06 March 2008 14:29:24 bob p4 wrote:
> [ In reference to
> http://maillist.perforce.com/pipermail/p4ruby/20
08q1/000115.html ]
>
> I finally got P4Win cooperating with cygwin-p4ruby via
listing my
> cygwin path in AltRoots, however I ran into a bit of
confusion along
> the way.  My client root is /home/me/some/long/path,
which I have
> symlinked to /home/me/work.  Apparently this causes a
glitch since the
> shell sets PWD to /home/me/work instead of the full
path:
>
> http://kb.perforce.com/UserTasks/ConfiguringP4/Symbo
licLinks
>
> Note the Perforce authors decided not to solve the
problem: "However,
> this doesn't necessarily fix the issue of symlinks
under the Perforce
> client root. For that, you will still need to either
set and export
> the $PWD environment variable, or use the -d argument
as necessary."
>
> May I suggest the following patch to workaround it:
>
> require 'p4'
> require 'pathname'
>
> class P4
>    class << self
>       alias_method :previous_new, :new
>       def new(*args, &block)
>          previous_pwd = ENV["PWD"]
>          begin
>             ENV["PWD"] =
Pathname.new(previous_pwd).realpath
>             previous_new(*args, &block)
>          ensure
>             ENV["PWD"] = previous_pwd
>          end
>       end
>    end
> end
>
> I am using p4ruby-2007.3 with cygwin
p4api-2007.3.143793.
>
> IMO Perforce should expand away the symlink, but until
then at least
> P4Ruby can do it.  Please spare others from this
needless pain!
> _______________________________________________
> p4ruby mailing list
> p4rubyperforce.com
> 
http://maillist.perforce.com/mailman/listinfo/p4ruby
>
> !DSPAM:47d0000351257730779061!

_______________________________________________
p4ruby mailing list
p4rubyperforce.com

http://maillist.perforce.com/mailman/listinfo/p4ruby

On Fri, Mar 7, 2008 at 8:46 AM, Tony Smith <tonysmee.org> wrote:
> The easier workaround is:
>
>         ENV.delete( "PWD" )
>
>  If PWD isn't present in the environment, then the
Perforce API will fall back
>  on using the result of getcwd(), which expands your
symlink as required.
>
>  Note that the PWD environment variable is intended to
supress expansion of
>  symlinks. Back in the early 90's it used to drive me
nuts when I cd'd across
>  a symlink and couldn't get back using 'cd ..'. PWD was
the solution that the
>  shells adopted for this and Perforce is simply
following the convention.
>
>  Most of the time that's what's wanted. When it isn't,
deleting PWD from the
>  environment usually solves the problem.

Deleting PWD makes the implementation slightly easier, but
the
principle is the same.  The workaround becomes:

class P4
   class << self
      alias_method :previous_new, :new
      def new(*args, &block)
         previous_pwd = ENV["PWD"]
         begin
            ENV.delete("PWD")
            previous_new(*args, &block)
         ensure
            if previous_pwd
               ENV["PWD"] = previous_pwd
            end
         end
      end
   end
end

Do we have a philosophical disagreement?  I want to fix this
problem
for Joe Programmer once and for all.  Would you rather have
Joe
Programmer to run into the problem, become annoyed and spend
time
googling, after which he may POSSIBLY find the magic
solution of
ENV.delete("PWD")?  Joe Programmer includes me.  I
want to help my
future self.  Do you?

The above fix also has the advantage of not polluting ENV
with a
deleted PWD, which may be used by some other component.

--Jeff
_______________________________________________
p4ruby mailing list
p4rubyperforce.com

http://maillist.perforce.com/mailman/listinfo/p4ruby

On Friday 07 March 2008 19:36:49 bob p4 wrote:
> On Fri, Mar 7, 2008 at 8:46 AM, Tony Smith <tonysmee.org> wrote:
> >  Most of the time that's what's wanted. When it
isn't, deleting PWD from
> > the environment usually solves the problem.

<snip>

> Do we have a philosophical disagreement?  I want to fix
this problem
> for Joe Programmer once and for all.  Would you rather
have Joe
> Programmer to run into the problem, become annoyed and
spend time
> googling, after which he may POSSIBLY find the magic
solution of
> ENV.delete("PWD")?  Joe Programmer includes
me.  I want to help my
> future self.  Do you?

I don't think this is a problem that can be solved once, for
everyone; if it 
was, I'd have done so long ago. 

As I said in my previous response, most of the time people
_don't_ want 
symlinks expanded (and would complain if we did). Perhaps if
others feel the 
same 

> The above fix also has the advantage of not polluting
ENV with a
> deleted PWD, which may be used by some other
component.

I
_______________________________________________
p4ruby mailing list
p4rubyperforce.com

http://maillist.perforce.com/mailman/listinfo/p4ruby

Let's try that again, a wrong keypress sent the last
response too soon.

On Friday 07 March 2008 19:36:49 bob p4 wrote:
> On Fri, Mar 7, 2008 at 8:46 AM, Tony Smith <tonysmee.org> wrote:
> >  Most of the time that's what's wanted. When it
isn't, deleting PWD from
> > the environment usually solves the problem.

<snip>

> Do we have a philosophical disagreement?  I want to fix
this problem
> for Joe Programmer once and for all.  Would you rather
have Joe
> Programmer to run into the problem, become annoyed and
spend time
> googling, after which he may POSSIBLY find the magic
solution of
> ENV.delete("PWD")?  Joe Programmer includes
me.  I want to help my
> future self.  Do you?

I don't think this is a problem that can be solved once, for
everyone; if it 
was, I'd have done so long ago. 

As I said in my previous response, most of the time people
_don't_ want 
symlinks expanded (and would complain if we did).  

> The above fix also has the advantage of not polluting
ENV with a
> deleted PWD, which may be used by some other
component.

I'm not a fan of the fix as it's been coded, but I'll take
another look at the 
problem and see if any better solutions come to light.

Tony
_______________________________________________
p4ruby mailing list
p4rubyperforce.com

http://maillist.perforce.com/mailman/listinfo/p4ruby

On Mon, Mar 10, 2008 at 10:31 AM, Tony Smith <tonysmee.org> wrote:
> Let's try that again, a wrong keypress sent the last
response too soon.
>
>
>  On Friday 07 March 2008 19:36:49 bob p4 wrote:
>
> > On Fri, Mar 7, 2008 at 8:46 AM, Tony Smith
<tonysmee.org> wrote:
>
> > >  Most of the time that's what's wanted. When
it isn't, deleting PWD from
>  > > the environment usually solves the problem.
>
>  <snip>
>
>
>  > Do we have a philosophical disagreement?  I want
to fix this problem
>  > for Joe Programmer once and for all.  Would you
rather have Joe
>  > Programmer to run into the problem, become
annoyed and spend time
>  > googling, after which he may POSSIBLY find the
magic solution of
>  > ENV.delete("PWD")?  Joe Programmer
includes me.  I want to help my
>  > future self.  Do you?
>
>
> I don't think this is a problem that can be solved
once, for everyone; if it
>  was, I'd have done so long ago.

Why do you think that?  We clearly agree that deleting PWD
works.
What is the problem?  If you were omniscient then you would
have done
so long ago, but of course none of us are.

> > The above fix also has the advantage of not
polluting ENV with a
>  > deleted PWD, which may be used by some other
component.
>
>  I'm not a fan of the fix as it's been coded, but I'll
take another look at the
>  problem and see if any better solutions come to
light.

The "fix," "as it's been coded," is
obviously a workaround for an end
user that wants the correct behavior immediately, without
recompiling
P4.so.  The actual fix would be to p4.cpp.  However the
workaround
shows exactly what to do: delete PWD from the environment,
call into
perforce, then restore PWD if it previously existed.  If
there is
something wrong with this plan then please tell us.

--Jeff
_______________________________________________
p4ruby mailing list
p4rubyperforce.com

http://maillist.perforce.com/mailman/listinfo/p4ruby

On Monday 10 March 2008 15:57:36 bob p4 wrote:
> > I don't think this is a problem that can be solved
once, for everyone; if
> > it was, I'd have done so long ago.
>
> Why do you think that?  We clearly agree that deleting
PWD works.

Yes, absolutely.

> What is the problem? 

The problem is that not everyone wants what you want. In
fact, other customers 
are _relying_ on the default behaviour, and we simply don't
want to break 
their scripts.

> If you were omniscient then you would have done 
> so long ago, but of course none of us are.
>
> > > The above fix also has the advantage of not
polluting ENV with a
> > >
> >  > deleted PWD, which may be used by some other
component.
> >
> >  I'm not a fan of the fix as it's been coded, but
I'll take another look
> > at the problem and see if any better solutions
come to light.
>
> The "fix," "as it's been coded," is
obviously a workaround for an end
> user that wants the correct behavior immediately,
without recompiling
> P4.so.  The actual fix would be to p4.cpp.  However the
workaround
> shows exactly what to do: delete PWD from the
environment, call into
> perforce, then restore PWD if it previously existed. 
If there is
> something wrong with this plan then please tell us.

It's ugly, that's all. It's not 'wrong' as it achieves the
right result, but 
to my eyes it's ugly. So we're looking for a better
solution. For example, a 
little investigation shows that the following also works in
P4Ruby:

	p4 = P4.new
	p4.cwd = Dir.getwd
	p4.connect

Does that help? 

Tony
_______________________________________________
p4ruby mailing list
p4rubyperforce.com

http://maillist.perforce.com/mailman/listinfo/p4ruby

On Mon, Mar 10, 2008 at 12:54 PM, Tony Smith <tonysmee.org> wrote:
> On Monday 10 March 2008 15:57:36 bob p4 wrote:
>
> > > I don't think this is a problem that can be
solved once, for everyone; if
>  > > it was, I'd have done so long ago.
>  >
>  > Why do you think that?  We clearly agree that
deleting PWD works.
>
>
> Yes, absolutely.
>
>  > What is the problem?
>
>  The problem is that not everyone wants what you want.
In fact, other customers
>  are _relying_ on the default behaviour, and we simply
don't want to break
>  their scripts.

I claim the fix is compatible with the default behavior;
that is why I
suggested it.  I may be wrong, but that remains to be shown.
 Can you
give an example where this would break existing code?

>  >
>  > The "fix," "as it's been
coded," is obviously a workaround for an end
>  > user that wants the correct behavior immediately,
without recompiling
>  > P4.so.  The actual fix would be to p4.cpp. 
However the workaround
>  > shows exactly what to do: delete PWD from the
environment, call into
>  > perforce, then restore PWD if it previously
existed.  If there is
>  > something wrong with this plan then please tell
us.
>
>  It's ugly, that's all. It's not 'wrong' as it achieves
the right result, but
> to my eyes it's ugly. So we're looking for a better
solution. For example, a
>  little investigation shows that the following also
works in P4Ruby:
>
>         p4 = P4.new
>         p4.cwd = Dir.getwd
>         p4.connect
>
>  Does that help?

First and foremost, my motivation is to fix the problem
permanently.
Save Joe Programmer the hassle of discovering, isolating,
and
searching for solutions to this problem.  I'm satisfied with
whatever
fixes it; and, of course, the simpler the better.

So when you ask, "Does the above help?" it depends
upon what you mean.
 If you mean that Joe Programmer should be advised to add
"p4.cwd =
Dir.getwd" when he runs into the problem, then this
does not help.
Joe Programmer's code already contains a bug, and his
googling efforts
may or may not locate this advice.

On the other hand, if you intend to integrate this fix into
P4Ruby,
then it does help.

You have said, "The problem is that not everyone wants
what you want."
 But everybody *does* want what I want, which is for this
bug to be
fixed.

--Jamie
_______________________________________________
p4ruby mailing list
p4rubyperforce.com

http://maillist.perforce.com/mailman/listinfo/p4ruby

On Monday 10 March 2008 18:11:06 bob p4 wrote:
> I claim the fix is compatible with the default
behavior; that is why I
> suggested it.  I may be wrong, but that remains to be
shown.  Can you
> give an example where this would break existing code?

Anytime there's a symlink in the cwd and it's part of the
client root/view, 
but the linked location isn't, existing code using local
syntax would break.

For example:

tonybarney:~/p4tmp/client$ p4 client -o

Client: barney
Root:   /home/tony/p4tmp/client
View:
        //depot/... //barney/...

tonybarney:~/p4tmp/client$ p4 where
//depot/... //barney/... /home/tony/p4tmp/client/...

tonybarney:~/p4tmp/client$ ls -l
lrwxrwxrwx 1 tony tony 16 2008-03-11 11:08 symlink ->
../not-in-client

tonybarney:~/p4tmp/client$ cd symlink
tonybarney:~/p4tmp/client/symlink$ irb
irb(main):001:0> require "P4"
=> true
irb(main):002:0> p4 = P4.new
=> #<P4:0x2b8104872d90>
irb(main):003:0> p4.connect
=> true
irb(main):004:0> p4.run_where
=> 
[{"clientFile"=>"//barney/symlink/..."
;,
"path"=>"/home/tony/p4tmp/client/symlink/.
..",
"depotFile"=>"//depot/symlink/..."}]


Now, with your change:

tonybarney:~/p4tmp/client/symlink$ irb
irb(main):001:0> require "P4"
=> true
irb(main):002:0> class P4
irb(main):003:1>    class << self
irb(main):004:2>       alias_method :previous_new, :new
irb(main):005:2>       def new(*args, &block)
irb(main):006:3>          previous_pwd =
ENV["PWD"]
irb(main):007:3>          begin
irb(main):008:4*             ENV.delete("PWD")
irb(main):009:4>             previous_new(*args,
&block)
irb(main):010:4>          ensure
irb(main):011:4*             if previous_pwd
irb(main):012:5>                ENV["PWD"] =
previous_pwd
irb(main):013:5>             end
irb(main):014:4>          end
irb(main):015:3>       end
irb(main):016:2>    end
irb(main):017:1> end
=> nil
irb(main):018:0> p4 = P4.new
=> #<P4:0x2af8abc51ee8>
irb(main):019:0> p4.connect
=> true
irb(main):020:0> p4.run_where
P4Exception: [P4#run] Errors during command execution(
"p4 where" )

        [Error]: Path '/home/tony/p4tmp/not-in-client/...'
is not under 
client's root '/home/tony/p4tmp/client'.


        from /usr/local/lib/site_ruby/1.8/P4.rb:67:in `run'
        from /usr/local/lib/site_ruby/1.8/P4.rb:67:in
`method_missing'
        from (irb):20
irb(main):021:0>

> You have said, "The problem is that not everyone
wants what you want."
>  But everybody *does* want what I want, which is for
this bug to be
> fixed.

This really isn't a bug; it's just the way that ALL Perforce
clients handle 
symlinks, and as far as I know, it's always been this way.
See also:

http://kb.perforce.com/UserTasks/ConfiguringP4/Symbo
licLinks

I think we've covered this subject in sufficient depth here,
but if you still 
feel strongly that it's a bug, or you'd like to discuss it
further, please 
log a support call by sending a message to supportperforce.com including the 
output of 'p4 info'.

Thanks,

Tony
_______________________________________________
p4ruby mailing list
p4rubyperforce.com

http://maillist.perforce.com/mailman/listinfo/p4ruby

On Tue, Mar 11, 2008 at 7:28 AM, Tony Smith <tonysmee.org> wrote:
>
>  On Monday 10 March 2008 18:11:06 bob p4 wrote:
>  > I claim the fix is compatible with the default
behavior; that is why I
>  > suggested it.  I may be wrong, but that remains
to be shown.  Can you
>  > give an example where this would break existing
code?
>
>  Anytime there's a symlink in the cwd and it's part of
the client root/view,
>  but the linked location isn't, existing code using
local syntax would break.

Your example is a good one.  It's obvious in hindsight
because it is
the converse of my case.  I was thinking that perforce
should be
smarter when PWD doesn't match the depot root --- why not
expand PWD
to see if it matches?  If it does, then it's safe to say
that is the
client root I intended.

In your case, the PWD does match the depot, but if you
expand it we
get something not in the client root.  I don't immediately
see the
possible benefit of this trickery, but I won't rule it out.

So I will revise my suggestion.  If and only if PWD is not
in the
client root, then perforce should try expanding PWD.  This
covers both
cases, I believe.

P4ruby is only mirroring perforce, so I can't exactly fault
p4ruby.  I
do believe perforce is bordering on incorrect behavior here,
so I
guess I'll take it up with them.  In the meantime I can
patch p4ruby
on my side to "fix" perforce.

Theoretically speaking, though, what are the possible
downsides of
p4ruby expanding PWD when it is not in the client root?  The
upside,
of course, is that it there is one less "wtf?"
situation for
programmers to encounter.

--Jeff
_______________________________________________
p4ruby mailing list
p4rubyperforce.com

http://maillist.perforce.com/mailman/listinfo/p4ruby