Merge lp:~parthm/bzr/596785-diff-no-dir-error into lp:bzr

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Parth Malwankar wrote:
> Parth Malwankar has proposed merging lp:~parthm/bzr/596785-diff-no-dir-error into lp:bzr.
>
> Requested reviews:
> bzr-core (bzr-core)
> Related bugs:
> #596785 'bzr diff --old|new PATH' does silently exists if arg does not exist
> https://bugs.launchpad.net/bugs/596785
>
>
> === Fixed Bug #596785 ===
> `bzr diff --old|new PATH` now show an error if PATH is not a dir. Without this patch it exists silently which is confusing (i.e. no diff output).
>
>

old and new can be URLs which will fail your isdir() check, but not fail
to work.

 merge: rejected

If you really want something like this, use a check something like:
  if old is not None and old_tree is None:
    fail

John
=:->

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (Cygwin)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAkwo0REACgkQJdeBCYSNAANIIgCgkW2ttdDtydFdKxdYF9LI5Ard
1EcAoIGBngukt8Y89oOLH31v5C5eC4G6
=ghVe
-----END PGP SIGNATURE-----

> old and new can be URLs which will fail your isdir() check, but not fail
> to work.
>

Good point. I should have thought of that :P

> If you really want something like this, use a check something like:
>  if old is not None and old_tree is None:
>    fail

Interestingly, old_tree is not None even with an invalid path. With
the following patch:

=== modified file 'bzrlib/diff.py'
--- bzrlib/diff.py 2010-05-26 15:58:08 +0000
+++ bzrlib/diff.py 2010-06-28 17:24:13 +0000
@@ -395,12 +395,15 @@
         old_url = default_location
     working_tree, branch, relpath = \
         bzrdir.BzrDir.open_containing_tree_or_branch(old_url)
+ print "wt:", working_tree, "br:", branch, "relpath:", relpath
+ print "has relpath:", branch.control_transport.has(relpath)
     lock_tree_or_branch(working_tree, branch)
     if consider_relpath and relpath != '':
         if working_tree is not None and apply_view:
             views.check_path_in_view(working_tree, relpath)
         specific_files.append(relpath)
     old_tree = _get_tree_to_diff(old_revision_spec, working_tree, branch)
+ print "old_tree:", old_tree
     old_branch = branch

     # Get the new location
@@ -441,6 +444,8 @@
     extra_trees = None
     if working_tree is not None and working_tree not in (old_tree, new_tree):
         extra_trees = (working_tree,)
+ print "old_branch:", old_branch

I get:

wt: <WorkingTree6 of
/storage/parth/src/bzr.dev/596785-diff-no-dir-error> br:
BzrBranch7(file:///storage/parth/src/bzr.dev/596785-diff-no-dir-error/)
relpath: foobar
has relpath: False
old_tree: <DirStateRevisionTree of
<email address hidden> in
DirState(u'/storage/parth/src/bzr.dev/596785-diff-no-dir-error/.bzr/checkout/dirstate')>
old_branch: BzrBranch7(file:///storage/parth/src/bzr.dev/596785-diff-no-dir-error/)

I am thinking that `branch.control_transport.has(relpath)` check could
be used to set old_tree/new_tree to None and check these in
cmd_diff.run as you suggested. Does that seem like a good solution?

Thanks for the review.

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Parth Malwankar wrote:
>> old and new can be URLs which will fail your isdir() check, but not fail
>> to work.
>>
>
> Good point. I should have thought of that :P
>
>> If you really want something like this, use a check something like:
>> if old is not None and old_tree is None:
>> fail
>
> Interestingly, old_tree is not None even with an invalid path. With
> the following patch:
>
> === modified file 'bzrlib/diff.py'
> --- bzrlib/diff.py 2010-05-26 15:58:08 +0000
> +++ bzrlib/diff.py 2010-06-28 17:24:13 +0000
> @@ -395,12 +395,15 @@
> old_url = default_location
> working_tree, branch, relpath = \
> bzrdir.BzrDir.open_containing_tree_or_branch(old_url)
> + print "wt:", working_tree, "br:", branch, "relpath:", relpath
> + print "has relpath:", branch.control_transport.has(relpath)
> lock_tree_or_branch(working_tree, branch)
> if consider_relpath and relpath != '':
> if working_tree is not None and apply_view:
> views.check_path_in_view(working_tree, relpath)
> specific_files.append(relpath)
> old_tree = _get_tree_to_diff(old_revision_spec, working_tree, branch)
> + print "old_tree:", old_tree
> old_branch = branch
>

offhand I would guess that you are getting a containing workingtree. I'm
not sure what should be done about relpath.

At least in my mind '--old' and '--new' are meant to be the exact path
to a branch (or wt), and not have any remainder.

So one option is just:
  if old is not None and (old_tree is None or relpath):
    ..

Is the relpath for the old_url used? If it isn't, then I think my point
stands.

John
=:->

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (Cygwin)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAkwo4AIACgkQJdeBCYSNAAM0MwCgr67dVr1JkSz2HwHDudMFAnuR
j3AAn0TfpwNtD6L/qsFwtYAmuGb034Jg
=QKQr
-----END PGP SIGNATURE-----