Merge lp:~gagern/bzr/bug527878-directoryCommonOption into lp:bzr

[Martin von Gagern]

Add --directory (-d) option to a number of commands, as requested by bug #527878. Still not all commands adjusted, but I'd like to see these simple ones merged before I tackle harder ones.

[Vincent Ladeuil (vila)]

Thanks for working on that, this is an often required feature.

16 tests are failing for this patch which demonstrate that we have some coverage here.

Yet, I'd like at least one blackbox test by modified command so that:
- we check that the new parameter is recognized,
- we check that it's taken into account

8 +def _open_tree_branch_or_directory(filename, directory):

Shouldn't that be: _open_containing_tree_or_branch_for_directory to:
- remind the enclosed _open_containing call,
- make it clear we override it for the 'directory' parameter if specified

I appreciate that you tried to reduce your patch size by tackling the easiest ones first.

review: Needs Fixing

[Martin von Gagern (gagern)]

> Yet, I'd like at least one blackbox test by modified command so that:
> - we check that the new parameter is recognized,
> - we check that it's taken into account

Added blackbox tests for all commands for which I added the --directory
option. That's a lot more work than the actual code modification... :-(
But probably justified: exposed an error in the added command, which I
fixed in a separate commit. Please have a close look at that fix, as I'm
not sure what the point of this os.access check is in the first place.

> 8 +def _open_tree_branch_or_directory(filename, directory):
>
> Shouldn't that be: _open_containing_tree_or_branch_for_directory to:
> - remind the enclosed _open_containing call,
> - make it clear we override it for the 'directory' parameter if specified

Not sure that's a good choice either: the code I implemented requires
the argument to --directory to denote the root directory of the branch.
Passing a directory within the branch would be illegal. Therefore it's
not really "containing branch for directory", as we don't use the branch
containing the directory, but expect the directory to be the branch.

What I wanted the name to express would be "(containing tree or branch)
or --directory", or perhaps the other way round, if you have python
short-circuit evaluation in mind. As verbose names expressing this idea
tend to be rather long, some kind of abbreviatuon seemed sensible.
That's what made me think about "tree, branch or directory", thus the
name I chose.

So first question is, do you agree with my semantics of requiring the
directory option to specify the root of a branch for all cases where a
branch without a tree is allowed? I feel that's cleaner than doing
containing checks and allowing arbitrary distribution of subdirs among
the option and the other arguments.

If you agree, then what options do we have to choose a better name?
1. _open_directory_option_or_containing_tree_or_branch
2. _open_directory_or_containing_tree_or_branch
3. _open_containing_tree_or_branch_for_directory (your suggestion)
4. _open_dir_or_containing_tree_or_branch
5. _open_containing_tree_or_branch_or_directory
6. _open_containing_tree_or_branch
7. _open_tree_branch_or_directory (current form)

[Vincent Ladeuil (vila)]

>>>>> Martin von Gagern <email address hidden> writes:

    >> Yet, I'd like at least one blackbox test by modified command so that:
    >> - we check that the new parameter is recognized,
    >> - we check that it's taken into account

    > Added blackbox tests for all commands for which I added the
    > --directory option. That's a lot more work than the actual code
    > modification... :-(

Thanks for that. Yes, sometimes you need to catch up... on the other
hand, these tests may be factorized/parametrized differently, but
nothing obvious at first glance... but see[1]

    > But probably justified: exposed an error in the added command,
    > which I fixed in a separate commit.

And added a test ! Great.

    > Please have a close look at that fix, as I'm not sure what the
    > point of this os.access check is in the first place.

Urgh, this os.access is a ugly, but we want to get rid of all these
variations in favor of the unique 'ls' command anyway.

You may replace it by 'wt.has(path)' which does exactly that (grepping
os.access will tell you that this *is* the only place where os.access is
used wrongly).

I think the fix was indeed needed and is correct.

    >> 8 +def _open_tree_branch_or_directory(filename, directory):
    >>
    >> Shouldn't that be: _open_containing_tree_or_branch_for_directory to:
    >> - remind the enclosed _open_containing call,
    >> - make it clear we override it for the 'directory' parameter if specified

    > Not sure that's a good choice either: the code I implemented
    > requires the argument to --directory to denote the root directory
    > of the branch.

Right, but it can still be None.

    > Passing a directory within the branch would be illegal. Therefore
    > it's not really "containing branch for directory", as we don't use
    > the branch containing the directory, but expect the directory to
    > be the branch.

Yes, as mentioned on IRC, I think the root problem here is to be able to
override the branch root guessing but that can come later.

    > What I wanted the name to express would be "(containing tree or
    > branch) or --directory", or perhaps the other way round, if you
    > have python short-circuit evaluation in mind. As verbose names
    > expressing this idea tend to be rather long, some kind of
    > abbreviatuon seemed sensible. That's what made me think about
    > "tree, branch or directory", thus the name I chose.

    > So first question is, do you agree with my semantics of requiring
    > the directory option to specify the root of a branch
    > for all cases where a branch without a tree is allowed?

I agree 100% until here.

    > I feel that's cleaner than doing containing checks and allowing
    > arbitrary distribution of subdirs among the option and the other
    > arguments.

This part is less clear, I a vague souvenir of some commands allowing
files from different branches, which can be messy and I'm not sure that
we want to keep that.

But anyway, even without that edge case, I think some commands didn't
anticipate this option and will need to be carefully inspected (as you
did very well with 'bzr added').

The whole idea with open_containing is t...

review: Approve

[Gary van der Merwe (garyvdm)]

Everything looks good. I hack up some code to check the every command that has a directory option, has a test that calls it with --directory, just in case one was missed, and the all do.

review: Approve

[lp:~gagern/bzr/bug527878-directoryCommonOption]

5181. By Martin von Gagern on 2010-05-03

Added NEWS entry.

5182. By Martin von Gagern on 2010-05-03

merge from trunk.