Subject: Re: [PATCH v3 10/9] revision.c: treat A...B merge bases as if manually specified

From: Kevin Bracey <[email-masked]>
Date: Tue, 07 May 2013 18:52:13 +0300

On 07/05/2013 00:24, Junio C Hamano wrote:
> Kevin Bracey <[email-masked]> writes:
>
>> The documentation assures users that "A...B" is defined as 'r1 r2 --not
>> $(git merge-base --all r1 r2)'. This isn't in fact quite true, because
>> the calculated merge bases are not sent to add_rev_cmdline().
> We want the commands to be able to tell which ones in revs->pending
> and revs->commits were specified by the end user and how.  While I
> think it makes sense to mark these negative merge bases with "These
> came from the command line with A...B syntax", I am not sure if it
> is the best way to do so in add_pending_commit_list().
Layering violation? The callers of add_pending_commit_list() should be 
doing it themselves? Yes, I suppose it was a bit of a convenience hack, 
having add_pending_commit_list do it.
>
> By the way, why does this have anything to do with the history
> traversal series in the first place?

The answer's in the test, but it's not that clear in this series 
addendum, with the test newly appearing and passing. The next version of 
the series will have the test in initially as a failure.

Without this patch, "git log E...F file" will unnecessarily show D, as 
it has 2 differing non-priority parents B and C.

Whereas "git log E F ^B file" doesn't show D. So we have a behaviour 
difference between two allegedly equivalent commands.

When querying E...F, C is a side branch between the merge base B and F, 
so D should be removable, just as for "B..F" or "E F ^B". So we need to 
give B the BOTTOM marker to make that work, as if it had been specified 
"E F ^B".

>
> When there is anythning marked UNINTERESTING on the rev->pending
> before calling prepare_revision_walk(), you have a history with some
> bottom boundaries, and when there isn't, your bottom boundaries are
> root commits.  If you want to behave differently depending on how
> the user gave us the revision range from the command line, e.g.
> acting differently between "A ^B" and "B..A", cmdline is a good
> place to learn the exact form, but at the history traversal level, I
> do not think you should even care.  Why does the code even look at
> the cmdline, and not rev->pending?
>

Well, on this first pass I wanted to be sure I was using the same 
definition of "bottom" as ancestry-path, so I hacked the flag setting in 
there, and I believe the answer to "why not just look at UNINTERESTING" 
lies in commit 281eee4. If I understand that correctly, it goes wrong 
when you have multiple negative specifications - some initial walking is 
done, meaning we can't distinguish between specified bottoms and other 
walk results.

Now, the current structure of the code is clearly silly, and my "hack" 
comment acknowledged that - if we have the BOTTOM flag, we could just 
set that immediately during command parsing, and neither this nor 
ancestry-path would need to look back at the command line to identify 
what the bottom commits were. But I didn't want to start work on that 
without further discussion about the merits of the BOTTOM flag.

(Actually, no, I'm not looking back at the command line, I just know 
that ancestry-path does, so I make sure I arrange it so that my 
inspection of rev->pending shows the same bottom commits as its 
inspection of the command line).

Kevin