Re: [PATCH] revisions: refactor init_revisions and setup_revisions.

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 10, 2008, 19:36 UTC
Message-ID: <7vwsoaid6z.fsf@gitster.siamese.dyndns.org>
In-Reply-To: <20080310084920.GA2798@artemis.madism.org>

Pierre Habouzit <madcoder@debian.org> writes:

Show 5 quoted lines

>   We could of course have a .def member in the struct rev_info, and use
> the one passed to setup_revisions then if it's still NULL, but it
> doesn't really makes sense to me, and I don't really see a problem with
> saying at init time that you'll default to "HEAD". Though if you really
> dislike it that much, I squash a patch that does that on top of it.

Well, it was not liking or disliking. Although I thought "default" that sets a value to the default after the parser finds that the user did not give anything (the approach you described in the above quoted paragraph) is a natural implementation, probably more so than what you did, I do not have strong preference either way.

When I see a change where I do not see a reason to, I get suspicious, wondering if I am missing some bigger reason (e.g. "by moving it there this and that would become much easier and cleaner, even though it now forces callers of cmd_log_init() to duplicate the default values"). There must be an obvious justification you had when you changed it, which I am not seeing. Hence that question.

Show 9 quoted lines

>> Applying this to 'master' and then merging 'pu' shows that there are a few
>> topics that are cooking that would conflict with this change.  Merging
>> 'next' seems to go cleanly (I haven't checked the result), so it is not
>> too bad for me to carrry this at this moment, if we were not this close to
>> the rc freeze.  I dunno.
>
>   Well I can wait longer, I'd just like to see it merged in a not too
> far future, because I have to check for new places that would need
> conversions at each reabase :)

Yeah, that burden can be shifted to me, in other words ;-)