Re: [PATCH 3/5] setup_revisions: remember whether a ref was positive or not

From: Sverre Rabbelier <srabbelier@gmail.com>
Date: Aug 3, 2011, 13:52 UTC
Message-ID: <CAGdFq_ghxFdpjxCgTNbqXWGpt0rpJaGZ1_h+ZC71PzaPzbQ-0A@mail.gmail.com>
In-Reply-To: <7vr55fs1z0.fsf@alter.siamese.dyndns.org>

Heya,

On Mon, Jul 25, 2011 at 01:17, Junio C Hamano <gitster@pobox.com> wrote:

Show 7 quoted lines

> In other words, I am not opposed to an effort to give the callers to the
> "pending objects" machinery a better way to discover what the user told us
> from the command line, giving them more than just "at the end of the
> UNINTERESTING marking here are the objects listed on the command line and
> you can look at their flags".  For example, some commands may want to tell
> "a..b" and "^a b" apart, and other commands may want to tell what "a" was
> when the user asked for exotic things like "a^@" or "a^!".

I agree that such might be useful, but since we currently do not need something as advanced as that, I'm hesitant to implement something (far) more advanced than what we need (YAGNI [0]). So I'm thinking that perhaps there's a suitable middle ground where we stick with the current flags approach until someone needs something more advanced so as to make sure that it's implemented in a way that meets an actual implementer need?

On Sun, Jul 24, 2011 at 21:23, Junio C Hamano <gitster@pobox.com> wrote:

Show 12 quoted lines

> Sverre Rabbelier <srabbelier@gmail.com> writes:
>
>>  void add_pending_object(struct rev_info *revs, struct object *obj, const char *name)
>>  {
>> -     add_pending_object_with_mode(revs, obj, name, S_IFINVALID);
>> +     add_pending_object_with_mode(revs, obj, name, S_IFINVALID, 0);
>>  }
>
> This seems utterly broken.  For example, fmt-merge-msg.c adds "^HEAD" and
> of course the flags on the object is UNINTERESTING. Has all the callers of
> add_pending_object() been verified? Why is it passing an unconditional 0
> and not !!(obj->flags & UNINTERESTING) or something?

If I understand correctly (and it's not unlikely that I don't), the 'flags' field is used to store the actual flags (not just a boolean). Would the following be appropriate?

+     add_pending_object_with_mode(revs, obj, name, S_IFINVALID, obj->flags);

Hopefully Dscho remembers enough of our hacking to be able to answer that question.

If it is, do you still think it's "utterly broken" or would that be something we can work with?

[0] http://c2.com/xp/YouArentGonnaNeedIt.html

-- 
Cheers,

Sverre Rabbelier