Re: [JGIT PATCH v2 14/24] Added the class IgnoreRuleListFactory.

From: Shawn O. Pearce <spearce@spearce.org>
Date: May 14, 2008, 01:06 UTC
Message-ID: <20080514010600.GK29038@spearce.org>
In-Reply-To: <48296B41.9020105@web.de>

Florian Kberle <FloriansKarten@web.de> wrote:

Show 13 quoted lines

> Shawn O. Pearce wrote:
> >
> >Why go through all this work to buffer the lines we don't care about
> >(starting with # or are blank) when we could just discard them in the
> >inside of createIgnoreRuleList and then create the rule right away?
>
> I did this to reduce complexity and to increase modularity.
> The method createIgnoreRuleList(Iterable<String> lineIterable) can now 
> be tested without the need to create files. Also it is so possible to 
> read the patterns from different sources.
> 
> If you worry about memory usuage I could create some kind of 
> RulesBuilder class:

OK. I guess I can see that. I wasn't worried about memory, it just seemed odd to me that the "parser" (splitting the file into lines) was also not skipping the lines that were not interesting.

Given your explanation I don't see a reason to change it, it just
struck me as odd.

Show 13 quoted lines

> >I suspect this code would be easier to follow if you just accepted
> >changing the method parameter, such as:
> >
> >	private Rule createRule(String pattern) {
> >		boolean exclude = true;
> >		if (pattern.startsWith("!)) {
> >			pattern = pattern.substring(1);
> >			exclude = false;
> >		}
>
> If I remember correctly Intellij IDEA marks per default cases where you 
> change a parameter. So it looks like that there are style guides which 
> are against the practice of changing the values of parameters.

Yes, there are style guides floating around that say "do not modify parameters". There is also a Java keyword that means "do not modify this variable in scope", its called "final".

Personally I think whether or not something should be final in a scope depends on the size of the scope and what is clearer to read to modify the existing variable, or to introduce a copy with a different name and a potentially different value. Forcing everything to not modify parameters by compiler edict is nuts.

In the code I was talking about you had two different variables for the pattern, just to avoid not modifying the parameter. Yet within the scope of the method once the "!" is cut off the original pattern string is worthless. In my very humble opinion it is more risky to have two different variables (one whose scope has expired the moment the method starts and one that is useful) then to have a mutable parameter.

But as I said, its only my humble opinion.  Since I'm American my
$0.02 is worth what, 0.00000002 rupees these days?  ;-)

> I implemented it that way, becasue git behave the same way:
> * create the follwing file: a/a/b/test.txt
> * add the line "a/b" to .gitignore
> * add a and notice that it adds the test.txt file.

OK. You are correct. If the ignore rule contains "/" it appears to only apply from the top level of the repository. That wasn't my understanding of how it worked, but I stand corrected. Thank you.

-- 
Shawn.