On 07/05/2013 00:24, Junio C Hamano wrote:
Kevin Bracey <kevin@xxxxxxxxx> 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html