How should we handle diff commands that only have access to one of the two files?

Ok, I can admit to having a shrine to the "Blessed
Mother of fdisk" in 
my office and whether smart or not, I'm normally chant the
"fresh 
install" mantra. It's how I've always handle system
upgrades for years 
and since I only ever needed few packages, keeping packages
up to date 
has been done with my dear friends pkg_delete and pkg_add.
Now that 
I've started using OpenBSD as my daily desktop, it's time to
learn the 
real magic hidden in the OpenBSD package system...
-updating!

Since xine-lib in the 4.0-stable tree has been updated, new
packages are 
up on the mirrors and I have xine-lib-1.1.1p0 installed from
CD, it 
seemed like a good place to practice. After a quick read of
the 
pkg_add(1) man page, I gave it a shot...

# pkg_add -u xine-lib
Candidates for updating xine-lib-1.1.1p0 ->
xine-lib-1.1.1p0 
xine-lib-1.1.2p4
Ambiguous: xine-lib-1.1.1p0 could be xine-lib-1.1.1p0
xine-lib-1.1.2p4
Looking for updates: complete
There are some ambiguities. Please run in interactive mode
again.
#

My question is how the heck is "xine-lib-1.1.1p0"
considered a candidate 
for upgrading itself? -Is this a bug or am I missing
something obvious?

Anyhow, I followed the additional instructions and used
interactive 
mode, selected the desired package to upgrade to
(xine-lib-1.1.2p4) and 
all went well... well sort of...

# pkg_add -iu xine-lib
Candidates for updating xine-lib-1.1.1p0 ->
xine-lib-1.1.1p0 
xine-lib-1.1.2p4
Ambiguous: xine-lib-1.1.1p0 could be xine-lib-1.1.1p0
xine-lib-1.1.2p4
Choose one package
         0: <None>
         1: xine-lib-1.1.1p0
         2: xine-lib-1.1.2p4
Your choice: 2
Looking for updates: complete
Running the equivalent of pkg_add -r xine-lib-1.1.2p4
xine-lib-1.1.2p4 (extracting): complete
xine-lib-1.1.1p0 (deleting): complete
xine-lib-1.1.2p4 (installing): complete
Clean shared items: complete
#

When I saw this line in the output:

  Running the equivalent of pkg_add -r xine-lib-1.1.2p4

I was really worried. How the heck could it be -r(eplacing)
a package 
which is not installed? 

Of course, the xine-lib-1.1.1p0 version was installed but
the reported 
xine-lib-1.1.2p4 version was not. 

I was supposed to be -r(eplacing) the installed
xine-lib-1.1.1p0 with 
the new xine-lib-1.1.2p4

The above is really an English usage issue, and not really a
big deal 
but most people think, "replace one-thing with
another-thing."

Another issue which is less than obvious in the output above
is that the 
server (ftp3.usa.openbsd.org) actually had four xine-lib
packages 
available but as  you can see, it only gave me two choices
in 
interactive mode.

  xine-lib-1.1.1p0
  xine-lib-1.1.2
  xine-lib-1.1.2p3
  xine-lib-1.1.2p4

If someone is running in interactive mode, it might be
better to give 
them a full list of available choices.

Are the multiple choices the reason why I got the dreaded
"ambiguous" on 
my first attempt?  -And if so, why where they not listed as
candidates?

An issue which is a big deal is the fact that
 
  # pkg_add -r not_installed_package_name

will install a completely new package rather than fail to
replace a 
package which is not installed. If the given package is not
currently 
installed, then -r(eplace) should fail.

Is it better to augment the man pages, or change the way
things work?

yes, I know... shutup and send patches...  -But it just
seemed best to 
ask first on these things to see what others think.  

Kind Regards,
JCR

On Jan 13, 2007, at 12:37, Carsten Koch wrote:

> Barry Scott wrote:
> ...
>>> IMHO, it would be best to always grey out
commands that do not  
>>> apply.
>>>
>>
>> Indeed this is what is supposed to happen. Any
command that fails for
>> predictable reasons must be disabled and if its not
its a bug.
>
> I agree.
>
> For the cases where a local file does not exist,
> that should be relatively easy to fix.

>
> For the cases where a repository file does not exist,
> I believe it will not be acceptable to have a long
delay
> between the mouse click and the appearing of the
context menu.

You have to try the operation optimistically. Probing the
repo is not
accpetable.

You can trap the error from the diff logic and translate it
into a  
friendly
message for the user. Even if you have to issue another
command to
find out the info you need for a clear message.

Barry



------------------------------------------------------------
---------
To unsubscribe, e-mail: dev-unsubscribepysvn.tigris.org
For additional commands, e-mail: dev-helppysvn.tigris.org

On Jan 13, 2007, at 12:37, Carsten Koch wrote:

> Barry Scott wrote:
> ...
>>> IMHO, it would be best to always grey out
commands that do not  
>>> apply.
>>>
>>
>> Indeed this is what is supposed to happen. Any
command that fails for
>> predictable reasons must be disabled and if its not
its a bug.
>
> I agree.
>
> For the cases where a local file does not exist,
> that should be relatively easy to fix.
>
> For the cases where a repository file does not exist,
> I believe it will not be acceptable to have a long
delay
> between the mouse click and the appearing of the
context menu.
>
> When thinking this over, I first thought we could check
whether
> the "HEAD", "branch origin BASE"
and "branch origin HEAD" versions
> exist when generating the list. However, this will work
100% reliably
> only for "branch origin BASE", since both
head versions can change
> between the time we create a list and the time a user
right-clicks
> on a list item.
>
> So I currently do not see a reliable solution for the
cases
> where a repository file does not exist.

Would diff_summarize help you get the info you need to tell
the
use what is going on?

Barry

------------------------------------------------------------
---------
To unsubscribe, e-mail: dev-unsubscribepysvn.tigris.org
For additional commands, e-mail: dev-helppysvn.tigris.org

Carsten Koch wrote:
> Barry Scott wrote:
> ...
>>> IMHO, it would be best to always grey out
commands that do not apply.
>>>
>> Indeed this is what is supposed to happen. Any
command that fails for
>> predictable reasons must be disabled and if its not
its a bug.
> 
> I agree.
> 
> For the cases where a local file does not exist,
> that should be relatively easy to fix.

I took a look today and found it much harder than I had
anticipated.

For the normal list in the main window, I made the following
change:

============================================================
=======
--- wb_frame.py (revision 44059)
+++ wb_frame.py (working copy)
 -865,7
+865,7 

     def OnUpdateUiSpDiffWorkHead( self, event ):
         self.getUpdateUiState()
-        if self.ui_state_focus.need_checkout:
+        if self.ui_state_focus.need_checkout or not
self.ui_state_focus.file_exists:
             event.Enable( False )
         else:
             if self.ui_state_list is self.ui_state_focus:
 -881,14
+881,14 

     def OnUpdateUiSpDiffWorkBranchOriginBase( self, event
):
         self.getUpdateUiState()
-        if self.ui_state_focus.need_checkout:
+        if self.ui_state_focus.need_checkout or not
self.ui_state_focus.file_exists:
             event.Enable( False )
         else:
             event.Enable( self.ui_state_list.versioned and
not self.ui_state_list.new_versioned )

     def OnUpdateUiSpDiffWorkBranchOriginHead( self, event
):
         self.getUpdateUiState()
-        if self.ui_state_focus.need_checkout:
+        if self.ui_state_focus.need_checkout or not
self.ui_state_focus.file_exists:
             event.Enable( False )
         else:
             event.Enable( self.ui_state_list.versioned and
not self.ui_state_list.new_versioned )

That works as intended if I select a single file:
If the file I select exists in my working copy,
the diff items are active.
If it does not exists, diff items are inactive.
So far so good.

If I select multiple files, the behaviour becomes somewhat
strange.
Selecting a range that includes a missing file sometimes has
the
diff items enabled, sometimes they are disabled.

So I guess enabling and disabling of the context menu
entries
will not really work.

Before I realized that, I also took a look at
wb_subversion_report_branch_changes.py. Of course, that one
does
not have the self.getUpdateUiState() infrastructure in place
and
I could not quickly figure out how to make it work.

So, in summary, I am ready to give up on the "disable
context menu
entries that do not apply" path and only follow up on
the
"trap the error from the diff logic and translate it
into a friendly
message for the user" path.

Am I making sense here?
Is this the right way to go?

Carsten.

------------------------------------------------------------
---------
To unsubscribe, e-mail: dev-unsubscribepysvn.tigris.org
For additional commands, e-mail: dev-helppysvn.tigris.org

Barry Scott wrote:
...
> You can trap the error from the diff logic and
translate it into a friendly
> message for the user. Even if you have to issue another
command to
> find out the info you need for a clear message.

OK, here is what I came up with:

Index: wb_subversion_diff.py
============================================================
=======
--- wb_subversion_diff.py       (revision 44059)
+++ wb_subversion_diff.py       (working copy)
 -89,6
+89,34 
     if mode == 'svn-diff' and (old_path_info.revision is
None or new_path_info.revision is None):
         mode = 'built-in'

+    # Check for possible errors in advance to give the user
a clear message.
+    for path_info in (old_path_info, new_path_info):
+        if path_info.revision is None or
path_info.revision.kind == pysvn.opt_revision_kind.working:
+            if not os.path.exists( path_info.path ):
+                app.log_error( '"%s" does not
exist.' % path_info.path )
+                return
+            if os.path.isdir( path_info.path ):
+                app.log_error( '"%s" refers to a
directory.' % path_info.path )
+                return
+            if not os.access( path_info.path, os.R_OK ):
+                app.log_error( '"%s" cannot be
read.' % path_info.path )
+                return
+        else:
+           yield app.backgroundProcess
+           try:
+               project_info.client_bg.info2(
path_info.path, path_info.revision, recurse=False )
+           except pysvn.ClientError, e:
+               yield app.foregroundProcess
+               if path_info.revision.kind ==
pysvn.opt_revision_kind.base:
+                  revision_description = 'BASE'
+               elif path_info.revision.kind ==
pysvn.opt_revision_kind.head:
+                  revision_description = 'HEAD'
+               else:
+                  revision_description = str(
path_info.revision.number )
+               app.log_error( '"%s" revision %s
cannot be accessed.' % (path_info.path,
revision_description) )
+               return
+           yield app.foregroundProcess
+
     if mode == 'svn-diff':
         # svn will do all the work
         yield app.backgroundProcess


It is not what I originally thought would be ideal,
but I guess it is close to the best we can realistically
achieve.

Let me know what you think.

Carsten.

------------------------------------------------------------
---------
To unsubscribe, e-mail: dev-unsubscribepysvn.tigris.org
For additional commands, e-mail: dev-helppysvn.tigris.org

Barry Scott wrote:
...
> You can trap the error from the diff logic and
translate it into a friendly
> message for the user. Even if you have to issue another
command to
> find out the info you need for a clear message.

The attached patch has been in use at ICEM since January
30th,
so it has been well tested.
If you approve, please commit.

Thanks and Cheers,
Carsten.

------------------------------------------------------------
---------
To unsubscribe, e-mail: dev-unsubscribepysvn.tigris.org
For additional commands, e-mail: dev-helppysvn.tigris.org

On Apr 26, 2007, at 18:35, Carsten Koch wrote:

> Barry Scott wrote:
> ...
>> You can trap the error from the diff logic and
translate it into a  
>> friendly
>> message for the user. Even if you have to issue
another command to
>> find out the info you need for a clear message.
>
> The attached patch has been in use at ICEM since
January 30th,
> so it has been well tested.
> If you approve, please commit.

Committed as r845.

Barry



------------------------------------------------------------
---------
To unsubscribe, e-mail: dev-unsubscribepysvn.tigris.org
For additional commands, e-mail: dev-helppysvn.tigris.org

Barry Scott wrote:
...
> You can trap the error from the diff logic and
translate it into a friendly
> message for the user. Even if you have to issue another
command to
> find out the info you need for a clear message.

My users requested that we simply create an empty file
when one of the two is not accessible for whatever reason.
This way, the diff program will automatically process
added and deleted files "right" (i.e. display the
file
in one half of the window and nothing in the other).

The attached patch has been in use at ICEM for a while
now and everyone appears to be happy with it.

So, if you approve, please commit.

Thanks and Cheers,
Carsten.

------------------------------------------------------------
---------
To unsubscribe, e-mail: dev-unsubscribepysvn.tigris.org
For additional commands, e-mail: dev-helppysvn.tigris.org