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

From: Sverre Rabbelier <srabbelier@gmail.com>
Date: Aug 8, 2011, 21:27 UTC
Message-ID: <CAGdFq_hLy6_AW-Yh_9fi318Z6jdkFWw5+cYrwMtOitDkGQorFA@mail.gmail.com>
In-Reply-To: <7vfwlbztfg.fsf@alter.siamese.dyndns.org>

Heya,

On Mon, Aug 8, 2011 at 19:47, Junio C Hamano <gitster@pobox.com> wrote:

Show 9 quoted lines

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

Show 6 quoted lines

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

Show 5 quoted lines

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

Show 8 quoted lines

> 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