New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

dag: add diff tool option #473

Merged
merged 3 commits into from Aug 12, 2015

Conversation

Projects
None yet
2 participants
@AndiDog
Contributor

AndiDog commented Jul 13, 2015

This adds a context menu option for each file when viewing commit(s) in the DAG. It opens the external diff tool for

  • one commit selected: ..
  • more commits selected: ..

I didn't manage to add Ctrl+D shortcut for that because I don't have that much time (got an error "QAction::eventFilter: Ambiguous shortcut overload: Ctrl+D").

Please check carefully since I don't know the code base / style at all.

dag: add diff tool option
Signed-off-by: Andreas Sommer <andreas.sommer87@googlemail.com>
Show outdated Hide outdated cola/widgets/dag.py
@AndiDog

This comment has been minimized.

Show comment
Hide comment
@AndiDog

AndiDog Jul 15, 2015

Contributor

I chose not to use ^! because e.g. a merge commit may have two parents. Then the "least surprise" action would be to diff against the commit which is shown below the selected commit, instead of letting Git choose against which parent commit to diff. It looks like ^! will always choose the first parent commit as shown in git log (a merge commit e.g. has the log line "Merge: 1234567 7654321", and the first sha1 – here 1234567 ­– is used for the diff).

Does that reasoning make sense to you?

Contributor

AndiDog commented Jul 15, 2015

I chose not to use ^! because e.g. a merge commit may have two parents. Then the "least surprise" action would be to diff against the commit which is shown below the selected commit, instead of letting Git choose against which parent commit to diff. It looks like ^! will always choose the first parent commit as shown in git log (a merge commit e.g. has the log line "Merge: 1234567 7654321", and the first sha1 – here 1234567 ­– is used for the diff).

Does that reasoning make sense to you?

@davvid

This comment has been minimized.

Show comment
Hide comment
@davvid

davvid Jul 16, 2015

Member

The order is what --topo-order gives, and I don't think we can expect it to make sense in all cases. The history is a dag, but the list is flattening and losing information.

This reminds me of the same problem we had to solve in sha1_diff() in cola/gitcmds.py. The comment there mentions,

def sha1_diff(git, sha1, filename=None):
    """Return the diff for a sha1"""
    # Naively "$sha1^!" is what we'd like to use but that doesn't
    # give the correct result for merges--the diff is reversed.
    # Be explicit and compare sha1 against its first parent.
    args = [sha1 + '~', sha1]
    opts = common_diff_opts()
    _add_filename(args, filename)
    status, out, err = git.diff(*args, **opts)
    if status != 0:
        # We probably don't have "$sha1~" because this is the root commit.
        # "git show" is clever enough to handle the root commit.
        args = [sha1 + '^!']
        _add_filename(args, filename)
        status, out, err = git.show(pretty='format:', *args, **opts)
        out = out.lstrip()
    return out

I think it's okay to ignore root commits for now, since we're launching a difftool. The rationale above is probably what I was thinking about. For a merge, we want to compare the first parent vs. the merge commit. I think git difftool $sha1~ $sha1 is what we should use universally for showing the effect of a commit on the project.

The root commit won't work with this approach but that's fine IMO. It's simpler to just disable the button when the root commit is selected (the parents field should be empty).

Later, maybe we can special-case the root commit. The way to do that would be to detect that it's a root and then diff it against the empty tree, which is a built-in git constant sha1. Hopefully the resulting code is simpler for this initial version since we only need to know $sha1.

What do you think?

Member

davvid commented Jul 16, 2015

The order is what --topo-order gives, and I don't think we can expect it to make sense in all cases. The history is a dag, but the list is flattening and losing information.

This reminds me of the same problem we had to solve in sha1_diff() in cola/gitcmds.py. The comment there mentions,

def sha1_diff(git, sha1, filename=None):
    """Return the diff for a sha1"""
    # Naively "$sha1^!" is what we'd like to use but that doesn't
    # give the correct result for merges--the diff is reversed.
    # Be explicit and compare sha1 against its first parent.
    args = [sha1 + '~', sha1]
    opts = common_diff_opts()
    _add_filename(args, filename)
    status, out, err = git.diff(*args, **opts)
    if status != 0:
        # We probably don't have "$sha1~" because this is the root commit.
        # "git show" is clever enough to handle the root commit.
        args = [sha1 + '^!']
        _add_filename(args, filename)
        status, out, err = git.show(pretty='format:', *args, **opts)
        out = out.lstrip()
    return out

I think it's okay to ignore root commits for now, since we're launching a difftool. The rationale above is probably what I was thinking about. For a merge, we want to compare the first parent vs. the merge commit. I think git difftool $sha1~ $sha1 is what we should use universally for showing the effect of a commit on the project.

The root commit won't work with this approach but that's fine IMO. It's simpler to just disable the button when the root commit is selected (the parents field should be empty).

Later, maybe we can special-case the root commit. The way to do that would be to detect that it's a root and then diff it against the empty tree, which is a built-in git constant sha1. Hopefully the resulting code is simpler for this initial version since we only need to know $sha1.

What do you think?

@davvid

This comment has been minimized.

Show comment
Hide comment
@davvid

davvid Jul 16, 2015

Member

The sha1 for an empty tree is 4b825dc642cb6eb9a060e54bf8d69288fbee4904. Special-case for the root commit would be, git difftool 4b825dc642cb6eb9a060e54bf8d69288fbee4904 $sha1.

Member

davvid commented Jul 16, 2015

The sha1 for an empty tree is 4b825dc642cb6eb9a060e54bf8d69288fbee4904. Special-case for the root commit would be, git difftool 4b825dc642cb6eb9a060e54bf8d69288fbee4904 $sha1.

@AndiDog

This comment has been minimized.

Show comment
Hide comment
@AndiDog

AndiDog Jul 17, 2015

Contributor

Good catches! Then I will adapt to compare sha1~ vs. sha1 and handle the root commit by disabling the option. Might take a few days to update.

Contributor

AndiDog commented Jul 17, 2015

Good catches! Then I will adapt to compare sha1~ vs. sha1 and handle the root commit by disabling the option. Might take a few days to update.

@AndiDog

This comment has been minimized.

Show comment
Hide comment
@AndiDog

AndiDog Jul 21, 2015

Contributor

Hmm I also noticed that when selecting multiple commits in the DAG Log, only the changed files from the topmost commit are shown in "Files". What do you think about changing that to showing all files that changed within that commit range? Then my context menu option for the difftool actually makes sense to use.

Contributor

AndiDog commented Jul 21, 2015

Hmm I also noticed that when selecting multiple commits in the DAG Log, only the changed files from the topmost commit are shown in "Files". What do you think about changing that to showing all files that changed within that commit range? Then my context menu option for the difftool actually makes sense to use.

@davvid

This comment has been minimized.

Show comment
Hide comment
@davvid

davvid Jul 21, 2015

Member

@AndiDog that would be a very welcome change. I think the current code basically just took a shortcut by only looking at the first selected item. Having it honor the range would be really awesome.

Member

davvid commented Jul 21, 2015

@AndiDog that would be a very welcome change. I think the current code basically just took a shortcut by only looking at the first selected item. Having it honor the range would be really awesome.

@AndiDog

This comment has been minimized.

Show comment
Hide comment
@AndiDog

AndiDog Aug 11, 2015

Contributor

Please have a look if that solution works for you.

I'll implement showing all files for the selected range (comment above: #473 (comment)) when this is done.

Contributor

AndiDog commented Aug 11, 2015

Please have a look if that solution works for you.

I'll implement showing all files for the selected range (comment above: #473 (comment)) when this is done.

@davvid

This comment has been minimized.

Show comment
Hide comment
@davvid

davvid Aug 12, 2015

Member

Thanks @AndiDog I'll merge this now.

I may break some of the one-liner expressions foo = bar if baz else quux into multiple lines, but that's a trivial style thing and no need to bother you with that. Nicely done.

Member

davvid commented Aug 12, 2015

Thanks @AndiDog I'll merge this now.

I may break some of the one-liner expressions foo = bar if baz else quux into multiple lines, but that's a trivial style thing and no need to bother you with that. Nicely done.

davvid added a commit that referenced this pull request Aug 12, 2015

Merge pull request #473 from AndiDog/feature/dag-difftool-option
dag: add diff tool option

Signed-off-by: David Aguilar <davvid@gmail.com>

@davvid davvid merged commit 638c236 into git-cola:master Aug 12, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@AndiDog AndiDog deleted the AndiDog:feature/dag-difftool-option branch Oct 2, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment