[Sugar-devel] Analysing code changes, git diff

[Simon Schampijer]

On 09/09/2010 02:08 PM, Sascha Silbe wrote:
> Excerpts from James Cameron's message of Thu Sep 09 03:24:36 +0200 2010:
>
>> Sorry, I used tabify in emacs, so as to force the diff to show the
>> entire function as changed.  I had said I "replaced tabs with spaces",
>> but I should have said "replaced spaces with tabs".  This is only
>> because I don't know how to drive git diff well enough.
>
> This brings up something I have been brooding about for some time now:
>
> The default output of "git diff" often isn't very useful for
> understanding a patch or even just merging changes. It's a low-level
> representation and, when comparing Python code, often not even a good
> one.
>
> I know git can be configured to use external "diff drivers" and "merge
> drivers" (see also [6]), so maybe we can improve this. That's why I
> started a new thread: Others might be aware of tools that can be used,
> either as-is or after some integration work (or at least on their own).
>
>
> There are two different things that bug me about "git diff" when applied
> to the Sugar source code:
>
>
> 1. The built-in algorithm is generic (good) and always language-
>     agnostic (bad). For Python code, this often results in:
>
>     I. Choosing unsuitable borders.
>        Example:
>
> @@ -81,15 +82,19 @@ def _async_copy(self, file_path, destination_path, completion_cb,
>                   unlink_src)
>           async_copy.start()
>
> -    def retrieve(self, uid, user_id, extension):
> +    def has_data(self, object_id):
> +        file_path = layoutmanager.get_instance().get_data_path(object_id)
> +        return os.path.exists(file_path)
> +
> +    def retrieve(self, object_id, user_id, extension):
>           """Place the file associated to a given entry into a directory
>              where the user can read it. The caller is reponsible for
>              deleting this file.
>
>           """
>
>        This mixes up the changes to retrieve (only signature changed) and
>        adding a new method has_data(). When handling conflicts, the above
>        output is unnecessarily hard to understand and resolve.
>        What I would have liked to see instead is:
>
> +    def has_data(self, object_id):
> +        file_path = layoutmanager.get_instance().get_data_path(object_id)
> +        return os.path.exists(file_path)
> +
> -    def retrieve(self, uid, user_id, extension):
> +    def retrieve(self, object_id, user_id, extension):
>           """Place the file associated to a given entry into a directory
>              where the user can read it. The caller is reponsible for
>              deleting this file.
>
>           """

FWIW, I understand your example and the frustration well. Have been put 
off by it myself. I often use the diff and the copy of the original code 
and/or final code near by to understand bigger refactorings. Which is 
not very handy of course.

So far, I have not come across better tools but will let you know if I 
see better.

Regards,
    Simon