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

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

Hi,

On Sun, 24 Jul 2011, Junio C Hamano wrote:

> 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 the excuse is "this is only to help fast-export and other callers of 
> add_pending_object() does not care", that is a sloppy attitude that 
> breaks maintainability of the code (because it forgets to add "in the 
> current code nobody looks at the new 'flags' field" to that excuse, and 
> also does not have any comments around this code that says so); it is 
> questionable if such a hack belongs to a patch that touches object.h.

I hope you appreciate our effort not to hack this into the 'mode' 
variable. That variable is exactly in the spirit you mentioned, so I 
figured we'd be fine with a similar approach.

Now that you have made clear that it is not fine, would you please care to 
enlighten me how you would prefer the change to look like that makes the 
argument parsing in revision.c remember which _refs_ (as opposed to 
_commits_) were marked uninteresting?

Thank you very much,
Johannes