From: Larry D'Anna <larry@elder-gods.org>
Date: Mon, 01 Feb 2010 00:57:51 GMT
Subject: Re: [PATCH v6] add --summary option to git-push and git-fetch
Message-ID: <20100201005751.GA8322@cthulhu>
In-Reply-To: <7vsk9oysds.fsf@alter.siamese.dyndns.org>

* Junio C Hamano (gitster@pobox.com) [100130 02:16]:
> 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.

> 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