[MERGE] Shelf 5 / 5 for reals

Mon Nov 3 21:54:57 GMT 2008

Yes, I noticed and fixed that just after submitting it.

> ^- Usually we leave each one on its own line with a trailing ',' like:
> 
>  ui,
>  workingtree,
>  )

Fixed.

> +try:
> +    from bzrlib.plugins.bzrtools import colordiff
> +except ImportError:
> +    colordiff = None
> 
> 
> ^- I can't say that there is a strictly better way to do this, but it
> does seem a bit odd to have core code depend on an external library.

Agreed.  The only alternative I can think of is to provide something
that bzrtools can hook into, like a shelf_ui.color_differ global.

Is this something you want fixed before this lands?

> Was
> any progress made in getting some sort of colordiff support into core?

Not really.  I didn't want yet another method of performing colordiff
added, without it being a proper replacement of cdiff.  We don't need
three different ways to perform a colour diff.

> ^- I think you have 2 spaces around "list may be  shelved"

Fixed.

> +    def prompt_bool(self, question):
> +        """Prompt the user with a yes/no question.
> +
> +        This may be overridden by self.auto, or auto may be supplied.
> +        It may also *set* self.auto.  It may also raise SystemExit.
> +        :param question: The question to ask the user.
> +        :return: True or False
> 
> ^- You no longer allow "auto" to be passed in, and the docstring just
> needs to be updated.

Good eye.

> +    def handle_modify_text(self, creator, file_id):
> +        """Provide diff hunk selection for modified text.
> +
> +        :param creator: a ShelfCreator
> +        :param file_id: The id of the file to shelve.
> +        :return: number of shelved hunks.
> +        """
> +        target_lines = self.target_tree.get_file_lines(file_id)
> +        work_file = self.work_tree.get_file(file_id)
> +        try:
> +            textfile.text_file(work_file)
> +        finally:
> +            work_file.close()
> +        textfile.check_text_lines(target_lines)
> +        parsed = self.get_parsed_patch(file_id)
> 
> 
> ^- It seems a bit odd to work in terms of both lines of a file, and a
> File object, and to not pass any of that to 'get_parsed_patch' which
> needs to extract it yet again. I'm not sure that there is much you can
> do, but there is a bit of redundancy here.

I agree it's odd.  It's due to API friction; the only code that can
produce a unified diff from top-to-bottom is diff.DiffText.diff(), and
that doesn't accept file lines or anything.

It may be more redundant than you realize, because internal_diff is
checking for binary files too.  But I figured that speed wasn't the most
important consideration for this code, so it was good enough for now.

> ^- You have an extra blank space here

Fixed.

> 
> I'm also thinking we may want to add tests for Unicode filenames as part
> of the UI. I believe we need to set 'outf' to 'exact' because we will be
> writing out diffs, so we need to be careful that we *do* encode
> filenames when we need to. (This can come in after the rest of the
> shelf, code, though.)

Okay.  I'll do that as a follow-on.

> I also notice that you are doing:
> 
> 'Shelve renaming foo => bar? [yNfq]'
> 
> but
> 
> 'Shelve removing file "foo"?  [yNfq]'
> 
> We should probably be consistent with how we quote the filenames we
> display.

Okay, I've quoted that.

> I'll also note that you don't test the display of the filename
> as part of the 'diff' code, though perhaps that is tested elsewhere.

If it's not tested in test_diff, that would definitely be a hole in
testing, but I don't see how it's relevant here.

> Do we want to be raising SystemExit, rather than a more bzr-specific
> exit exception?

Well, it's very direct, and this is commmand-level functionality, so it
felt okay to me.  But I've switched that to raise UserAbort.

Diff attached.

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

iEYEARECAAYFAkkPczEACgkQ0F+nu1YWqI0yNQCdFzkFPit/SrCMG9XpFzv5Rhlt
qmwAoIM8wizF8pUFO04qegACOZQ+zQyr
=PLZe
-----END PGP SIGNATURE-----
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: shelf.diff
Url: https://lists.ubuntu.com/archives/bazaar/attachments/20081103/9e7e4e34/attachment.diff 