Re: [PATCH v3 1/3] gitweb: fix #patchNN anchors when path_info is enabled

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 18, 2011, 16:57 UTC
Message-ID: <7v7hbwxt7e.fsf@alter.siamese.dyndns.org>
In-Reply-To: <201103181359.46600.jnareb@gmail.com>

Jakub Narebski <jnareb@gmail.com> writes:

Show 9 quoted lines

> I like v2 version, and had only small corrections.  For you to not have
> to resend this patch once again, I have added those corrections and sent
> them as a patch myself.
>
> Changes from original v2 version from Kevin Cernekee:
>
> * Fix (re-add) accidentally lost in v2 '"<td class=\"link\">"' in first
>   chunk using href(-anchor => ...)
> * Reword commit message (hopefully make it better, and not only longer)

Thanks.

> * Move code that sets implicit -replay when -anchor is the only parameter
>   lower, and change order of subexpression, for better possible future
>   extendability, if there would be other cases when we would want to 
>   automatically turn on -replay.

I think you meant this part:

	# implicit -replay
	$params{-replay} = 1 if (keys %params == 1 && $params{-anchor});

But I don't think it is that much of an improvement as long as it uses the statement modifier syntax ("A if B") for a thing like this.

I will not bother rewriting the commit I made out of this patch myself, but the next person who wants to add the auto-replay for his option will first have to rewrite the above into:

	if (key %params == 1 && $params{-anchor}) {
		$params{-replay} = 1;
	}

before turning it into:

	if ((key %params == 1 && $params{-anchor}) ||
	    ("here comes my new condition")) {
		$params{-replay} = 1;
	}

anyway. If your rewrite were to a plain-vanilla "if () { ... }", it would have been a real improvement.

Besides, I find it a bad taste to use statement modifier syntax unless the modifying condition ("if B" part) is much more likely to hold true than false.

If you limit your use of statement modifiers for conditions that almost always hold true, it will become much easier to scan the code for the first time, because you can skim through and almost ignore the condition part to follow the logic for the normal case. But turning 'replay' on in a specific narrow case (and later in a specific set of narrow cases) is a complete opposite of normal codeflow.

Anyway, thanks for the clean-up.  Applied.