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

[Sverre Rabbelier <srabbelier@gmail.com>]

Heya,

On Mon, Aug 8, 2011 at 19:47, Junio C Hamano <gitster@pobox.com> wrote:
> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>> 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.

Doesn't passing 0 indicate that we _did not_ receive any explicit user
input on this ref, which is exactly what we want to record? If the
user passed us any explicit input on the ref, we record it at the
other call-sites, here, the user told us nothing, so we record exactly
that, nothing.

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

I cannot recall you ever asking somebody to implement some feature
_nobody needs right now_ while trying to fix a _bug_, why now? I do
not know this code well enough to implement it, and Dscho doesn't have
the time to do it.

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

Nobody needs the stuff you mentioned right now. We do need this to fix
this bug. If someone else does want it at a later date, replacing it
will be exactly as much work with or without this patch.

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

While I understand this reluctance, remember that this "one caller" is
required to fix a bug in the current code. If you had a similar
complaint about the remote-hg.py patches that I haven't sent yet, I
would be more than willing to invest the extra time in addressing
those concerns, since I'm adding new functionality anyway, this is
different.

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

Don't we already store that in the name field?

-- 
Cheers,

Sverre Rabbelier