Hi Junio,
On Mon, Sep 21, 2020 at 03:18:06PM -0700, Junio C Hamano wrote:
> Denton Liu <liu.denton@xxxxxxxxx> writes:
>
> > This does not happen because at the top of diff_get_merge_base(), we
> > have
> >
> > for (i = 0; i < revs->pending.nr; i++) {
> > struct object *obj = revs->pending.objects[i].item;
> > if (obj->flags)
> > die(_("--merge-base does not work with ranges"));
> > if (obj->type != OBJ_COMMIT)
> > die(_("--merge-base only works with commits"));
> > }
> >
> > which ensures that we don't accept any ranges at all.
>
> I think we should lose that loop, or at least the first test.
>
> If we are not removing the support for "A..B" notation and still
> accept "diff A..B" happily, not accepting "diff --merge-base A..B"
> would appear inconsistent to the users.
I disagree, in the documentation, it clearly states that this option is
only available to the diff modes that accept endpoints, not
ranges:
'git diff' [<options>] --cached [--merge-base] [<commit>] [--] [<path>...]::
and
'git diff' [<options>] [--merge-base] <commit> <commit> [--] [<path>...]::
so it seems perfectly consistent to me. The documentation gives the
impression that the range notations are their own separate mode anyway.
And worst case scenario, if we receive user reports that they believe
the feature is inconsistent, it's 100x easier to change it to allow
ranges than attempting to remove support for ranges in the future.
Thanks,
Denton
