Merge-base alias for git vc-diff

Merge-base alias for git vc-diff

[Juri Linkov]

I can't find how vc could compare the HEAD of the branch with its merge base.

The documentation recommends using `git diff topic...master' to find
changes that occurred on the master branch since when the topic branch
was started off it.

But it seems that vc-diff uses neither `..' nor `...'

Even `vc-git-log-incoming' and `vc-git-log-outgoing' use only `..'

Since "git diff A...B" is equivalent to "git diff $(git-merge-base A B) B",
doing the same means adding a new alias "MERGE-BASE" to the completions of
`C-u C-x v D' (vc-root-diff) to complement the existing alias "HEAD".

Does this make sense or there is a simpler way to do the same?

Re: Merge-base alias for git vc-diff

[Dmitry Gutov]

On 27.12.2018 0:32, Juri Linkov wrote:

> Since "git diff A...B" is equivalent to "git diff $(git-merge-base A B) B",
> doing the same means adding a new alias "MERGE-BASE" to the completions of
> `C-u C-x v D' (vc-root-diff) to complement the existing alias "HEAD".

Do we have aliases like that in VC? HEAD is simply a ref that Git 
recognizes, but MERGE-BASE is not (*). Not sure how to handle it best.

Maybe you'll want to both extend the 'diff' backend command and add a 
new boolean argument to the end.

And also add a new command that would use it, similar to 
vc-root-version-diff (or a new meaning to some value of the prefix 
argument).

(*) Also: merge-base with what? master is not always the base.

Re: Merge-base alias for git vc-diff

[Juri Linkov]

>> Since "git diff A...B" is equivalent to "git diff $(git-merge-base A B) B",
>> doing the same means adding a new alias "MERGE-BASE" to the completions of
>> `C-u C-x v D' (vc-root-diff) to complement the existing alias "HEAD".
>
> Do we have aliases like that in VC? HEAD is simply a ref that Git
> recognizes, but MERGE-BASE is not (*). Not sure how to handle it best.

MERGE-BASE could be a VC alias, not Git alias.  So it could be handled
specially by ‘vc-git-diff’, ‘vc-git-revision-table’, etc.

> Maybe you'll want to both extend the 'diff' backend command and add a new
> boolean argument to the end.

This is the easiest part of the task.

> And also add a new command that would use it, similar to
> vc-root-version-diff (or a new meaning to some value of the prefix
> argument).

Adding a new command like ‘vc-merge-base-diff’ is easy, but first
I'd like to try to extend the existing command because it is bound
to a short key sequence: ‘C-u C-x v D’ then type ‘TAB’ for completion
and select e.g. ‘MERGE-BASE’.

> (*) Also: merge-base with what? master is not always the base.

This is another problem, then we need to add the prefix MERGE-BASE
to all branches listed in the completions: MERGE-BASE:master,
MERGE-BASE:branch-a, MERGE-BASE:branch-b, ...  Too bad.

Re: Merge-base alias for git vc-diff

[Dmitry Gutov]

On 27.12.2018 22:02, Juri Linkov wrote:

> This is another problem, then we need to add the prefix MERGE-BASE
> to all branches listed in the completions: MERGE-BASE:master,
> MERGE-BASE:branch-a, MERGE-BASE:branch-b, ...  Too bad.

Yeah, seems non-obvious. Maybe create a whitelist for branch names?

Re: Merge-base alias for git vc-diff

[Stefan Monnier]

> This is another problem, then we need to add the prefix MERGE-BASE
> to all branches listed in the completions: MERGE-BASE:master,
> MERGE-BASE:branch-a, MERGE-BASE:branch-b, ...  Too bad.

Maybe we should allow the "old revision" to be of the form "<REV>..."
and/or the "new revision" to be of the form "...<REV>".


        Stefan

Re: Merge-base alias for git vc-diff

[Juri Linkov]

>> This is another problem, then we need to add the prefix MERGE-BASE
>> to all branches listed in the completions: MERGE-BASE:master,
>> MERGE-BASE:branch-a, MERGE-BASE:branch-b, ...  Too bad.
>
> Maybe we should allow the "old revision" to be of the form "<REV>..."
> and/or the "new revision" to be of the form "...<REV>".

This is the most natural representation, but the only problem is its
discoverability - the users have to learn this syntax.  OTOH, after
typing TAB TAB we can show the virtual revision name MERGE-BASE
in the list of completions, but its drawback is that after selecting
MERGE-BASE we have to show another minibuffer asking for the branch name
where to look for the common ancestor.  Or we could support both ways.

Re: Merge-base alias for git vc-diff

[Juri Linkov]

>> And also add a new command that would use it, similar to
>> vc-root-version-diff (or a new meaning to some value of the prefix
>> argument).
>
> Adding a new command like ‘vc-merge-base-diff’ is easy, but first
> I'd like to try to extend the existing command because it is bound
> to a short key sequence: ‘C-u C-x v D’ then type ‘TAB’ for completion
> and select e.g. ‘MERGE-BASE’.

Since there is no good solution to reuse the existing command,
bug#33950 implements two new commands:

* vc-log-mergebase (bound to ‘C-x v M L’) shows the same output as
  GitLab shows in the “Commits” tab of the merge request.

* vc-diff-mergebase (bound to ‘C-x v M D’) shows the same output as
  GitLab shows in the “Changes” tab of the merge request.

Re: Merge-base alias for git vc-diff

[Stefan Monnier]

> Since there is no good solution to reuse the existing command,
> bug#33950 implements two new commands:
>
> * vc-log-mergebase (bound to ‘C-x v M L’) shows the same output as
>   GitLab shows in the “Commits” tab of the merge request.
>
> * vc-diff-mergebase (bound to ‘C-x v M D’) shows the same output as
>   GitLab shows in the “Changes” tab of the merge request.

I must say I find this solution very unsatisfactory.


        Stefan

Re: Merge-base alias for git vc-diff

[João Távora]

On Wed, Jan 2, 2019 at 2:08 AM Stefan Monnier <monnier@iro.umontreal.ca> wrote:
>
> > Since there is no good solution to reuse the existing command,
> > bug#33950 implements two new commands:
> >
> > * vc-log-mergebase (bound to ‘C-x v M L’) shows the same output as
> >   GitLab shows in the “Commits” tab of the merge request.
> >
> > * vc-diff-mergebase (bound to ‘C-x v M D’) shows the same output as
> >   GitLab shows in the “Changes” tab of the merge request.
>
> I must say I find this solution very unsatisfactory.

Me too.  I don't mind adding those commands, but I think
the "..." syntax should be supported, too.

One way to enhance dicoverability is to add a short help in
the first few lines of the minibuffer.

João


-- 
João Távora

Re: Merge-base alias for git vc-diff

[Juri Linkov]

>> Since there is no good solution to reuse the existing command,
>> bug#33950 implements two new commands:
>>
>> * vc-log-mergebase (bound to ‘C-x v M L’) shows the same output as
>>   GitLab shows in the “Commits” tab of the merge request.
>>
>> * vc-diff-mergebase (bound to ‘C-x v M D’) shows the same output as
>>   GitLab shows in the “Changes” tab of the merge request.
>
> I must say I find this solution very unsatisfactory.

The reason I proposed separate commands is because I can't find
a good solution how to combine this with the existing commands.
Unsolved problems:

1. I don't know how to change the *Completions* buffer header from
   “Possible completions are:” to
   “Possible completions are (<REV>... means diff with common ancestor):”

2. How to support inserting a completion from the *Completions* buffer
   into the minibuffer but not exiting the minibuffer after choose-completion.
   Then after selecting a completion the user could add “...” to the end.

3. ‘C-u C-x v L’ asks for the limit only.  Maybe adding a new prefix arg
   ‘C-u C-u C-x v L’ could ask for two revisions: start rev and end rev.

Then ‘C-u C-u C-x v D’ could also interpret its double prefix arg as diff
with the common ancestor.

Re: Merge-base alias for git vc-diff

[Dmitry Gutov]

On 02.01.2019 18:34, João Távora wrote:

> Me too.  I don't mind adding those commands, but I think
> the "..." syntax should be supported, too.

I think it's not terrible. And if we were choosing between new commands 
and new revision syntax, the former is probably easier to discover.

If we used the same revision syntax in more contexts (commands), that 
might be more important.

> One way to enhance dicoverability is to add a short help in
> the first few lines of the minibuffer.

Isn't minibuffer normally just one line tall?

Re: Merge-base alias for git vc-diff

[João Távora]

On Wed, Jan 2, 2019 at 10:26 PM Dmitry Gutov <dgutov@yandex.ru> wrote:
>
> On 02.01.2019 18:34, João Távora wrote:
>
> > Me too.  I don't mind adding those commands, but I think
> > the "..." syntax should be supported, too.
>
> I think it's not terrible. And if we were choosing between new commands
> and new revision syntax, the former is probably easier to discover.

That depends on how one uses Emacs.  I personally prefer hints,
completion and C-u modifiers to new commands.

> Isn't minibuffer normally just one line tall?

Don't know. Is there a hard rule against printing more lines?  Or
a real disadvantage?

João

Re: Merge-base alias for git vc-diff

[Dmitry Gutov]

On 03.01.2019 1:48, João Távora wrote:

> That depends on how one uses Emacs.  I personally prefer hints,
> completion and C-u modifiers to new commands.

If we can reuse one of modifiers like that, yeah. But there's a limited 
amount of prefix modifiers we can use and, again, it's not very 
discoverable.

That's not to say that you couldn't integrate the new commands to be 
delegated to with certain prefix values, like vc-root-diff does.

To be clear, I'm not wild about adding new commands, but if we add 
functionality, it must go somewhere.

> Don't know. Is there a hard rule against printing more lines?  Or
> a real disadvantage?

Windows are jumping, it can be annoying. And in the end it might 
redistribute heights between windows in a top-bottom split.

Re: Merge-base alias for git vc-diff

[Stefan Monnier]

> That depends on how one uses Emacs.  I personally prefer hints,
> completion and C-u modifiers to new commands.

So do I.  Especially since we'd then also have to add new
log-view-merge-diff commands and possibly others.

>> Isn't minibuffer normally just one line tall?
> Don't know. Is there a hard rule against printing more lines?
> Or a real disadvantage?

FWIW, my minibuffer has 1 line and can't grow because it's in
a minibuffer-only frame.


        Stefan

Re: Merge-base alias for git vc-diff

[Stefan Monnier]

> The reason I proposed separate commands is because I can't find
> a good solution how to combine this with the existing commands.

You'd have to change the notion of "revision" in vc-git.el to allow
a leading or trailing "...": remember, this is specific to Git so it can
only belong in vc-git.el.

It also means that revision completion in C-x v ~ will also allow those
"..." even tho they don't make any sense there.


        Stefan

Re: Merge-base alias for git vc-diff

[João Távora]

Stefan Monnier <monnier@IRO.UMontreal.CA> writes:

> FWIW, my minibuffer has 1 line and can't grow because it's in
> a minibuffer-only frame.

Kinky :) I don't think a rigid minibuffer is a great idea, but scratch
the multi-line minibuffer approach, just add a C-h shortcut that
displays some contextual help like `save-some-buffers'.

Or add some hint to the beginning of the line, there's plenty of space.

Here's two other ideas:

1. Isn't merge-base suitable for a toggle switch?  Could sth like
   vc-git-prefer-merge-base be useful?  Maybe I'd set that to t, and it
   shows nicely in a checkbox in the VC menu for discoverability.  But I
   like the next idea better.

2. A diff is always a comparison of two revisions, right?  For vc-dir,
   isn't a UI like query-replace's suitable here?  You know, that thing
   where you type the pattern and the replacement interactively in the
   same minibuffer prompt, separated by the arrow.  You can M-p, C-r,
   etc, flawlessly. I really like that UI, it's really intuitive (kudos
   to whoever participated, btw).
    
   So here, instead of pattern and replacement, two revisions.  All one
   would need, apparently, is to turn the arrow into a ".." and make it
   toggleable to "..." with say, C-M-y.  I think it would simplify the
   completion problem: on both sides of the ".." or "..." you just
   complete a revision using whatever system we presumably already have
   in place for that.

João

Re: Merge-base alias for git vc-diff

[Juri Linkov]

> 2. A diff is always a comparison of two revisions, right?  For vc-dir,
>    isn't a UI like query-replace's suitable here?  You know, that thing
>    where you type the pattern and the replacement interactively in the
>    same minibuffer prompt, separated by the arrow.  You can M-p, C-r,
>    etc, flawlessly. I really like that UI, it's really intuitive (kudos
>    to whoever participated, btw).
>
>    So here, instead of pattern and replacement, two revisions.  All one
>    would need, apparently, is to turn the arrow into a ".." and make it
>    toggleable to "..." with say, C-M-y.  I think it would simplify the
>    completion problem: on both sides of the ".." or "..." you just
>    complete a revision using whatever system we presumably already have
>    in place for that.

Good idea.  This brings the minibuffer input syntax closer to the real
command line syntax.  The only problem is that then we have to use
different amount of dots for vc-root-diff and vc-print-root-log because
in git: ‘git diff A...B’ corresponds to ‘git log A..B’
whereas ‘git diff A..B’ corresponds to ‘git log A...B’

Re: Merge-base alias for git vc-diff

[João Távora]

Juri Linkov <juri@linkov.net> writes:

>> 2. A diff is always a comparison of two revisions, right?  For vc-dir,
>>    isn't a UI like query-replace's suitable here?  You know, that thing
>>    where you type the pattern and the replacement interactively in the
>>    same minibuffer prompt, separated by the arrow.  You can M-p, C-r,
>>    etc, flawlessly. I really like that UI, it's really intuitive (kudos
>>    to whoever participated, btw).
>>
>>    So here, instead of pattern and replacement, two revisions.  All one
>>    would need, apparently, is to turn the arrow into a ".." and make it
>>    toggleable to "..." with say, C-M-y.  I think it would simplify the
>>    completion problem: on both sides of the ".." or "..." you just
>>    complete a revision using whatever system we presumably already have
>>    in place for that.
>
> Good idea.

Cool. I'd say go for it! (unless someone seriously objects).

For a first stab at this, most of the UI work would be in
vc-diff-build-argument-list-internal which would, if passed an optional
argument, say CHOOSE-REVISION-RELATIONSHIP, use the new UI and return an
extra element at the end saying the merge-base should be used.  It
should be a question of grabbing this element in vc-version-diff and
passing it this down to vc-<backend>-diff.

The Git backend would know how to handle it (as could presumably other
backends).  The value of CHOOSE-REVISION-RELATIONSHIP passed in
vc-version-diff could be a backend-specific decision.

> The only problem is that then we have to use different amount of dots
> for vc-root-diff and vc-print-root-log because in git: ‘git diff
> A...B’ corresponds to ‘git log A..B’ whereas ‘git diff A..B’
> corresponds to ‘git log A...B’

Really? Ugh, that unfortunate.  Well we could just live with it or use
different symbols to abstract it away.

João

Re: Merge-base alias for git vc-diff

[Juri Linkov]

>> The only problem is that then we have to use different amount of dots
>> for vc-root-diff and vc-print-root-log because in git: ‘git diff
>> A...B’ corresponds to ‘git log A..B’ whereas ‘git diff A..B’
>> corresponds to ‘git log A...B’
>
> Really? Ugh, that unfortunate.  Well we could just live with it or use
> different symbols to abstract it away.

Another question is whether to use the same Git syntax for other dVCSes?
It seems that finding a common ancestor is a common task among them:
https://stackoverflow.com/questions/6743068/merge-base-analog-for-mercurial-and-bzr-to-find-common-ancestors-as-possible-fo

Re: Merge-base alias for git vc-diff

[Juri Linkov]

>> The only problem is that then we have to use different amount of dots
>> for vc-root-diff and vc-print-root-log because in git: ‘git diff
>> A...B’ corresponds to ‘git log A..B’ whereas ‘git diff A..B’
>> corresponds to ‘git log A...B’
>
> Really? Ugh, that unfortunate.  Well we could just live with it or use
> different symbols to abstract it away.

I still have no idea how to read a start/end revision in
‘C-u C-x v L’.  Currently it accepts a limit as a number.
I wonder is it possible to parse its minibuffer's input
and to distinguish such number from the revision?
Maybe using a regexp?

Re: Merge-base alias for git vc-diff

[Stefan Monnier]

> I still have no idea how to read a start/end revision in
> ‘C-u C-x v L’.

I think you need to use vc-print-branch-log.  All other VC print-log
commands show their age (they were designed back when branches were
rare, so you'd just "get *the* log" and then search for the revision of
interest).


        Stefan

Re: Merge-base alias for git vc-diff

[Juri Linkov]

>> I still have no idea how to read a start/end revision in
>> ‘C-u C-x v L’.
>
> I think you need to use vc-print-branch-log.  All other VC print-log
> commands show their age (they were designed back when branches were
> rare, so you'd just "get *the* log" and then search for the revision of
> interest).

I wonder how it could be used to limit its output to the merge base?
Maybe

  M-x vc-print-branch-log RET master..branch

Re: Merge-base alias for git vc-diff

[Juri Linkov]

>> The reason I proposed separate commands is because I can't find
>> a good solution how to combine this with the existing commands.
>
> You'd have to change the notion of "revision" in vc-git.el to allow
> a leading or trailing "...": remember, this is specific to Git so it can
> only belong in vc-git.el.
>
> It also means that revision completion in C-x v ~ will also allow those
> "..." even tho they don't make any sense there.

After trying to implement this with

diff --git a/lisp/vc/vc-git.el b/lisp/vc/vc-git.el
index f9ea4def15..8f8e151549 100644
--- a/lisp/vc/vc-git.el
+++ b/lisp/vc/vc-git.el
@@ -1247,4 +1249,10 @@ vc-git-diff
       (setq command "diff-index")
       (unless rev1 (setq rev1 "HEAD")))
+    (when (string-match "\\`\\(.*\\)\\.\\.\\.\\'" rev1)
+      (setq rev1 (string-trim-right (or (vc-git--run-command-string
+                                         nil "merge-base"
+                                         (match-string 1 rev1)
+                                         rev2)
+                                        (match-string 1 rev1)))))
     (if vc-git-diff-switches
         (apply #'vc-git-command (or buffer "*vc-diff*")

and using with `C-u C-x v D master... RET branch RET'
exposed an unexpected problem: vc-diff-internal sets
diff-vc-revisions to the list ("master..." "branch")

But diff-mode fails to use diff-vc-revisions for such features as
jumping to the corresponding source line with diff-goto-source that
signals an error in vc-git-find-revision:

  (error "Failed (status 128): git --no-pager cat-file blob master...:lisp/vc/vc-git.el .")

and leaves the buffer with an error message in it:

  fatal: Not a valid object name master...:lisp/vc/vc-git.el

One way to resolve this is to interpret "master...branch" as a syntax
to denote the merge base, then set diff-vc-revisions instead of

  ("master..." "branch")

to

  ("master...branch" "branch")

This means that this feature should be used only this way:

  C-u C-x v D master...branch RET branch RET

Re: Merge-base alias for git vc-diff

[Dmitry Gutov]

On 11.01.2019 00:25, Juri Linkov wrote:
> This means that this feature should be used only this way:
> 
>    C-u C-x v D master...branch RET branch RET

We could use a more special syntax for this, in order for the user not 
to be confused regarding master...branch..branch.

For example:

C-u C-x v D master^branch RET branch RET

Hopefully not many branches have a ^ in their names.

Re: Merge-base alias for git vc-diff

[Stefan Monnier]

>   (error "Failed (status 128): git --no-pager cat-file blob master...:lisp/vc/vc-git.el .")

I think this can/should be solved by making vc-git-find-revision throw
away the "..."


        Stefan

Re: Merge-base alias for git vc-diff

[Juri Linkov]

>>   (error "Failed (status 128): git --no-pager cat-file blob master...:lisp/vc/vc-git.el .")
>
> I think this can/should be solved by making vc-git-find-revision throw
> away the "..."

Sorry, I don't understand.  "git cat-file blob master:lisp/vc/vc-git.el"
will get the revision from the master's HEAD, but what is needed is
a revision of the merge base between master and branch, i.e. what
is returned by "git-merge-base master branch".

Re: Merge-base alias for git vc-diff

[Juri Linkov]

>> This means that this feature should be used only this way:
>>
>>    C-u C-x v D master...branch RET branch RET
>
> We could use a more special syntax for this, in order for the user not to
> be confused regarding master...branch..branch.
>
> For example:
>
> C-u C-x v D master^branch RET branch RET
>
> Hopefully not many branches have a ^ in their names.

This is too ad-hoc syntax that might conflict with some VCS special syntax
like HEAD^2.

Let me remind that "git diff A...B" is equivalent to
"git diff $(git-merge-base A B) B".

So we have at least 3 possible solutions:

  C-u C-x v D master...branch RET branch RET  -- where master...branch
                                              -- internally is substituted
                                              -- by the merge-base revision
                                              -- found by "git-merge-base master branch"

  C-u C-x v D master...branch RET RET   -- shortcut of the above, but the problem is
                                        -- how to get "branch" from rev1
                                        -- when rev2 is empty

  C-x v M D master RET branch RET  -- special command for merge-base revisions