Re: cherry-pick and 'log --no-walk' and ordering

From: Martin von Zweigbergk <martin.von.zweigbergk@gmail.com>
Date: Aug 11, 2012, 05:34 UTC
Message-ID: <CAOeW2eHz5un9cNoy-=7Y8=F_G6u-n8kk7kXGHQ+dKrHD8wW6BA@mail.gmail.com>
In-Reply-To: <7vfw7uig13.fsf@alter.siamese.dyndns.org>

On Fri, Aug 10, 2012 at 2:38 PM, Junio C Hamano <gitster@pobox.com> wrote:

Show 13 quoted lines

> Martin von Zweigbergk <martin.von.zweigbergk@gmail.com> writes:
>
>> There is also cherry-pick/revert, which I _think_ does not really want
>> the revisions sorted.
>
> Yes, I think sequencer.c::prepare_revs() is wrong to unconditoinally
> call prepare_revision_walk().
>
> It instead should first check the revs->pending.objects list to see
> if what was given by the caller is a mere collection of individual
> objects or a range expression (i.e. check if any of them is marked
> with UNINTERESTING), and refrain from going into the body of the
> preparation steps, which has to involve sorting.

Do you mean "has to involve sorting" as in "has to involve sorting in order not to break current users of e.g. 'git log --no-walk --branches'" or "revision walking inherently involves sorting"? My current working assumption is that it is the former.

I will make rev_info.no_walk a tri-state {walk, no-walk-sorted, no-walk-unsorted}. The third state would be used from cherry-pick/revert (and maybe git-show, although it should make no difference). I would also expose the third state to rev-list's command line, maybe as --no-walk=unsorted.

Actually, all but command-line parsing is done now and test seem fine,
with quite a small patch:
$ git diff --stat
 builtin/log.c    | 2 +-
 builtin/revert.c | 2 +-
 revision.c       | 5 +++--
 revision.h       | 6 +++++-
 4 files changed, 10 insertions(+), 5 deletions(-)

Did you see a problem with this approach, since you said that sequencer shouldn't unconditionally call prepare_revision_walk()? I can see that git-show needs to go through revs->pending.objects because it handles tags and stuff, but cherry-pick/revert only seem to need the revisions.

Martin