Johannes Schindelin <Johannes.Schindelin@xxxxxx> writes:
> Hi Junio,
>
> On Wed, 3 Aug 2011, Junio C Hamano wrote:
>
>> Sverre Rabbelier <srabbelier@xxxxxxxxx> writes:
>>
>> > On Sun, Jul 24, 2011 at 21:23, Junio C Hamano <gitster@xxxxxxxxx> wrote:
>> >> Sverre Rabbelier <srabbelier@xxxxxxxxx> 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);
>> >>> }
> ...
> If you do that, you're back to start. Since obj has not the faintest clue
> whether the pending object was added from a negative or a positive ref.
But the point is that this codepath does not have a faintest clue whether
the "obj" parameter is something the end user actively asked for (which
might have been marked as uninteresting for other reasons, namely, because
it is reachable from other negative refs). So passing unconditional 0 is
just as bad.
>> Wouldn't it be wonderful if the revision machinery left richer clue in
>> each element of the pending object array while parsing, so that the caller
>> does not have to guess?
>
> That is what we are trying to solve, and rather than to reuse the "mode" I
> thought that it'd be wiser to add new "flags".
I never thought about nor suggested touching "mode" ;-) It does not have
enough width for the necessary information.
> Many of the richer clues you refer to could be expressed as such flags,
> including the problem I need to address.
>
>> In addition to a single "mode" integer, which says if it is supposed to
>> be a tree or a blob, we could allocate a single structure that records...
>
> Is this not a little bit of a big, huge, tremendous overkill?
As long as you can show your "flags" can (be extended to) express the same
richness to solve sample problems I mentioned in my response, as well as
your immediate issue, I wouldn't insist implementing a parsed struct/union
that may be a more (and unnecessarily) verbose way to say the same thing.
> Or in other words: I'd rather stay with a simple, elegant, minimal patch
> that solves the problem at hand while not preventing future enhancements.
We are on the same page, but what I read from the patch didn't show a
clear way forward to extend the "flags" to allow the stuff I mentioned
(and the stuff I didn't but obviously fall into the same category of "we
wish revision parsing machinery left us richer information").
> Also remember that the pending objects array is used for the complete
> object traversal.
My reading and rememberance of the code around add_parents_to_list() must
be quite different from yours. It is not 2006 anymore.
We didn't even have a separate pending object list type until 1f1e895 (Add
"named object array" concept, 2006-06-19) and used the same object_list,
whose element size _did_ matter as you mention. But that commit allowed us
to give elements on the pending list that came directly from the end user
richer semantics than those of the object_list that we discover during the
traversal, and that is why back at e5709a4 (add add_object_array_with_mode,
2007-04-22) we gave more information to it without having to fatten
object_list elements.
> The only way to convince me to do that complicated stuff is by
> blackmailing me, stating that I cannot have a working remote-hg without
> doing all this nested/union/self-referential struct work I don't need.
I would be reluctant to accept a myopic hack that is only good for one
caller and that needs to be ripped out and re-done, especially when we
already know other issues that can be solved cleanly if you go a little
further in the initial round.
As I said, I am not married to the verbose struct/union representation
(the only reason I showed that verbosity was because it allowed me to do
away without having to enumerate all the syntax sugars we already
support); if your "flags" can express the same thing (it may needs to
become a bitfield with enough width, but I highly suspect that you would
also need at least a component that says "this is the string the user gave
us --- the user said 'master', not 'master^0', for example) and is a lot
more compact, that is definitely we want to go with.
Thanks.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html