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

From: Junio C Hamano <gitster@pobox.com>
Date: Jul 24, 2011, 19:23 UTC
Message-ID: <7vy5znscst.fsf@alter.siamese.dyndns.org>
In-Reply-To: <1311517282-24831-4-git-send-email-srabbelier@gmail.com>

Sverre Rabbelier <srabbelier@gmail.com> writes:

Show 5 quoted lines

>  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.

Show 16 quoted lines

> @@ -1073,7 +1074,8 @@ int handle_revision_arg(const char *arg, struct rev_info *revs,
>  			} else
>  				a->object.flags |= flags_exclude;
>  			b->object.flags |= flags;
> -			add_pending_object(revs, &a->object, this);
> +			add_pending_object_with_mode(revs, &a->object, this,
> +						     S_IFINVALID, flags_exclude);
> ...
> @@ -1103,7 +1105,7 @@ int handle_revision_arg(const char *arg, struct rev_info *revs,
>  	if (!cant_be_filename)
>  		verify_non_filename(revs->prefix, arg);
>  	object = get_reference(revs, arg, sha1, flags ^ local_flags);
> -	add_pending_object_with_mode(revs, object, arg, mode);
> +	add_pending_object_with_mode(revs, object, arg, mode, local_flags);
>  	return 0;
>  }

Questionable.  Did the user mean to say Z is positive when he said

	$ git rev-list A B ^C ... --not G H ... ^Z