Re: [PATCH] revision: add --except option

[Felipe Contreras <felipe.contreras@gmail.com>]

On Fri, Aug 30, 2013 at 1:32 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Felipe Contreras <felipe.contreras@gmail.com> writes:
>
>> So that it's possible to remove certain refs from the list without
>> removing the objects that are referenced by other refs.
>>
>> For example this repository:
>>
>>   * 374e8dd (crap) crap
>>   * 4cbbf7b (test) two
>>   * d025ae0 (HEAD, master) one
>
> Can we make it more clear that your assumption is "crap" is a child
> of "test" which is a child of "master"?  Without that, the "nothing
> will come out" will not follow.
>
>> When using '--branches --except crap':
>>
>>   * 4cbbf7b (test) two
>>   * d025ae0 (HEAD, master) one
>>
>> But when using '--branches --not crap' nothing will come out.
>
> If you have a history where
>
>  - branches "master" and "maint" point at commit A;
>  - branch "next" points at commit B that is a descendant of A; and
>  - there are tags X and Y pointing at commits that are ahead of B
>    or behind A
>
> i.e.
>
>         ----X----A----B----Y
>
> what are the desired semantics for these?
>
>  (1) --branches --except maint
>
>  (2) --all --not --branches --except maint
>
>  (3) ^master next --except maint
>
> "--branches" wants to include "master", "next", and "maint", and the
> "--except" tells us we do not want to take "maint" into account, but
> should that affect what "master" wants to do (either include or
> exclude what are reachable from it)?

No, it should not. '--branches --except main' becomes 'master next'.

> As the way we parse the revisions from the command line is to mark
> "objects", not "refs", as we go, it looks like that the flag SKIP in
> this patch is placed conceptually at a wrong level.

refs are marked as well.

> I agree "--branches --except maint" is a good concept, but to
> implement what this patch wants to do properly, I suspect that the
> revision parser may need to be extended to be a two-phase process;
> the first phase will collect list of objects involved in the range
> expression without marking them with UNINTERESTING mark (that would
> also involve expanding things like --all or --branches), while
> remembering those given with --except, exclude the "except" set from
> the first set, and then finally marking the objects using the
> remainder, or something like that.

That's not necessary, this patch does the trick.

>> +                     ce = &revs->cmdline.rev[i];
>> +                     if ((ce->flags & SKIP) && !strcmp(ce->name, e->name))
>> +                             goto next;
>
> I think this SKIP will not help an object that is already tainted by
> UNINTERESTING; if it is discovered during a traversal from another
> object that will remain in the rev->commits, the travesal will stop
> there, even if a ref that is marked with SKIP will "goto next" here.

No, the traversal will continue just fine. At this point we are still
not traversing anything, simply adding the heads that will need to be
traversed later on to a list. Whether this object has been tainted by
UNINTERESTED or not is irrelevant.

If you do 'master ^maint --except master', handle_commit will return
three commits:

master -> ce->flags will have SKIP, nothing more happens
maint -> ce->flags doesn't have SKIP, processing continues, and it's
added to the list of commits, the commit has UNINTERESTING, but that
doesn't matter
master -> ce->flags will have SKIP, nothing more happens

Essentially it's the same as:

maint -> it's added to the list of commits, the commit has UNINTERESTING

So, it's exactly the same as if you had typed '^maint', which is
exactly what we want.

-- 
Felipe Contreras