Re: [PATCH v6] add --summary option to git-push and git-fetch

From: Larry D'Anna <larry@elder-gods.org>
Date: Feb 1, 2010, 00:57 UTC
Message-ID: <20100201005751.GA8322@cthulhu>
In-Reply-To: <7vsk9oysds.fsf@alter.siamese.dyndns.org>

* Junio C Hamano (gitster@pobox.com) [100130 02:16]:

Show 19 quoted lines

> Larry D'Anna <larry@elder-gods.org> writes:
> > +
> > +	object = parse_object(sha1); 
> > +	if (!object)
> > +	    die("bad object %s", arg);
> > +	
> > +	object_deref = deref_tag(object, NULL, 0); 
> > +	if (object_deref && object_deref->type == OBJ_COMMIT)
> > +	    if (flags_to_clear)
> > +		clear_commit_marks((struct commit *) object_deref, flags_to_clear); 
> > +
> > +	object->flags |= flags ^ local_flags; 
> 
> This smells somewhat fishy---what is the reason this "peel and mark" needs
> to be done only in this codepath, and none of the other callers of
> get_reference() need a similar logic, for example?
> 
> In general, why do you need to sprinkle clear-commit-marks all over the
> place?

My idea was to call call clear_commit_marks on the "roots" of the revision arg, and since handle_revision_arg looks up those roots in several different places, i had to put clear_commit_marks in each of those places. the reason the patch is particularly ugly in this spot is that the other places where i put clear_commit_marks, I already had a struct commit *, but here i just had a object that might be a tag.

Show 7 quoted lines

> This is not a rhetorical question (I haven't reviewed all the
> codepath involved for quite some time), but naïvely it appears it would be
> a lot simpler if you can let the existing code to do all the revision
> parsing and preparation to add to the pending object array as usual, and
> clear the flags from them before you let prepare_revision_walk() to start
> traversing the commit, but you probably had some reason why that simpler
> approach would not work and did it this way.  What am I missing?

The "existing code" being the caller of print_summary_for_push_or_fetch? I suppose I just wanted to keep the patches interference with update_local_ref to a minimum, so I had it just grab the existing variable "quickref" out of that function, because that was all the info I really needed to print the summary.

So i guess you're saying that it would be better for update_local_ref and print_summary_for_push_or_fetch to clear the flags, and just pass a rev_info for print_summary_for_push_or_fetch instead of quickref?

   --larry