Fix serving of files when the root dir is './'

Fix serving of files when the root dir is './'

[imirkin_alum.mit.edu, 2012-05-06 18:01:26]

Grrr... I never pushed the "message" thing up to upstream rietveld, so
it ate my comments =/

Basically I ran into this only with soy files, for some reason. But
the root is very clearly set to './' in that case, so we definitely
need to de-uglify after. I also kept the first de-uglify since I don't
want to break existing stuff, and this is not a lot of fun to debug.

On Sun, May 6, 2012 at 1:53 PM,  <ibmirkin@gmail.com> wrote:
> Reviewers: bolinfest,
>
> Description:
> Fix serving of files when the root dir is './'
>
> Please review this at http://codereview.appspot.com/6195049/
>
> Affected files:
>  M src/org/plovr/Manifest.java
>
>
> Index: src/org/plovr/Manifest.java
> ===================================================================
> --- a/src/org/plovr/Manifest.java
> +++ b/src/org/plovr/Manifest.java
> @@ -421,15 +421,19 @@
>         // so strip the leading "./" from the JsInput name in this case.
>         String name;
>         if (file.equals(rootOfSearch.getFile())) {
> -          name = rootOfSearch.getName();
> +          name = "";
>         } else {
>           name = getRelativePath.apply(file);
> -          final String uglyPrefix = "./";
> -          if (name.startsWith(uglyPrefix)) {
> -            name = name.substring(uglyPrefix.length());
> -          }
> -          name = rootOfSearch.getName() + name;
>         }
> +        final String uglyPrefix = "./";
> +        if (name.startsWith(uglyPrefix)) {
> +          name = name.substring(uglyPrefix.length());
> +        }
> +        name = rootOfSearch.getName() + name;
> +        if (name.startsWith(uglyPrefix)) {
> +          name = name.substring(uglyPrefix.length());
> +        }
> +
>         JsInput input = LocalFileJsInput.createForFileWithName(file, name,
>             soyFileOptions);
>         logger.config("Dependency: " + input);
>
>

[bolinfest, 2012-05-07 02:52:10]

Is there an easy way you can add a test for this?

Sent from my iPhone

On May 6, 2012, at 2:01 PM, Ilia Mirkin <imirkin@alum.mit.edu> wrote:

> Grrr... I never pushed the "message" thing up to upstream rietveld, so
> it ate my comments =/
> 
> Basically I ran into this only with soy files, for some reason. But
> the root is very clearly set to './' in that case, so we definitely
> need to de-uglify after. I also kept the first de-uglify since I don't
> want to break existing stuff, and this is not a lot of fun to debug.
> 
> On Sun, May 6, 2012 at 1:53 PM,  <ibmirkin@gmail.com> wrote:
>> Reviewers: bolinfest,
>> 
>> Description:
>> Fix serving of files when the root dir is './'
>> 
>> Please review this at http://codereview.appspot.com/6195049/
>> 
>> Affected files:
>>  M src/org/plovr/Manifest.java
>> 
>> 
>> Index: src/org/plovr/Manifest.java
>> ===================================================================
>> --- a/src/org/plovr/Manifest.java
>> +++ b/src/org/plovr/Manifest.java
>> @@ -421,15 +421,19 @@
>>         // so strip the leading "./" from the JsInput name in this case.
>>         String name;
>>         if (file.equals(rootOfSearch.getFile())) {
>> -          name = rootOfSearch.getName();
>> +          name = "";
>>         } else {
>>           name = getRelativePath.apply(file);
>> -          final String uglyPrefix = "./";
>> -          if (name.startsWith(uglyPrefix)) {
>> -            name = name.substring(uglyPrefix.length());
>> -          }
>> -          name = rootOfSearch.getName() + name;
>>         }
>> +        final String uglyPrefix = "./";
>> +        if (name.startsWith(uglyPrefix)) {
>> +          name = name.substring(uglyPrefix.length());
>> +        }
>> +        name = rootOfSearch.getName() + name;
>> +        if (name.startsWith(uglyPrefix)) {
>> +          name = name.substring(uglyPrefix.length());
>> +        }
>> +
>>         JsInput input = LocalFileJsInput.createForFileWithName(file, name,
>>             soyFileOptions);
>>         logger.config("Dependency: " + input);
>> 
>> 

[imirkin_alum.mit.edu, 2012-05-07 02:58:45]

I'll look... maybe. This was just something I ran into while testing
out the translations thing.

On Sun, May 6, 2012 at 10:52 PM, Michael Bolin <bolinfest@gmail.com> wrote:
> Is there an easy way you can add a test for this?
>
> Sent from my iPhone
>
> On May 6, 2012, at 2:01 PM, Ilia Mirkin <imirkin@alum.mit.edu> wrote:
>
>> Grrr... I never pushed the "message" thing up to upstream rietveld, so
>> it ate my comments =/
>>
>> Basically I ran into this only with soy files, for some reason. But
>> the root is very clearly set to './' in that case, so we definitely
>> need to de-uglify after. I also kept the first de-uglify since I don't
>> want to break existing stuff, and this is not a lot of fun to debug.
>>
>> On Sun, May 6, 2012 at 1:53 PM,  <ibmirkin@gmail.com> wrote:
>>> Reviewers: bolinfest,
>>>
>>> Description:
>>> Fix serving of files when the root dir is './'
>>>
>>> Please review this at http://codereview.appspot.com/6195049/
>>>
>>> Affected files:
>>>  M src/org/plovr/Manifest.java
>>>
>>>
>>> Index: src/org/plovr/Manifest.java
>>> ===================================================================
>>> --- a/src/org/plovr/Manifest.java
>>> +++ b/src/org/plovr/Manifest.java
>>> @@ -421,15 +421,19 @@
>>>         // so strip the leading "./" from the JsInput name in this case.
>>>         String name;
>>>         if (file.equals(rootOfSearch.getFile())) {
>>> -          name = rootOfSearch.getName();
>>> +          name = "";
>>>         } else {
>>>           name = getRelativePath.apply(file);
>>> -          final String uglyPrefix = "./";
>>> -          if (name.startsWith(uglyPrefix)) {
>>> -            name = name.substring(uglyPrefix.length());
>>> -          }
>>> -          name = rootOfSearch.getName() + name;
>>>         }
>>> +        final String uglyPrefix = "./";
>>> +        if (name.startsWith(uglyPrefix)) {
>>> +          name = name.substring(uglyPrefix.length());
>>> +        }
>>> +        name = rootOfSearch.getName() + name;
>>> +        if (name.startsWith(uglyPrefix)) {
>>> +          name = name.substring(uglyPrefix.length());
>>> +        }
>>> +
>>>         JsInput input = LocalFileJsInput.createForFileWithName(file, name,
>>>             soyFileOptions);
>>>         logger.config("Dependency: " + input);
>>>
>>>

[imirkin_alum.mit.edu, 2012-05-07 05:27:48]

Another alternative all this hackery, btw, is to do the same thing you
did for .. -- use $ to replace the . in the InputFileHandler. I think
that might actually be a better way to go. Then we can get rid of the
whole ./ thing in Manifest. Thoughts?

On Sun, May 6, 2012 at 10:58 PM, Ilia Mirkin <imirkin@alum.mit.edu> wrote:
> I'll look... maybe. This was just something I ran into while testing
> out the translations thing.
>
> On Sun, May 6, 2012 at 10:52 PM, Michael Bolin <bolinfest@gmail.com> wrote:
>> Is there an easy way you can add a test for this?
>>
>> Sent from my iPhone
>>
>> On May 6, 2012, at 2:01 PM, Ilia Mirkin <imirkin@alum.mit.edu> wrote:
>>
>>> Grrr... I never pushed the "message" thing up to upstream rietveld, so
>>> it ate my comments =/
>>>
>>> Basically I ran into this only with soy files, for some reason. But
>>> the root is very clearly set to './' in that case, so we definitely
>>> need to de-uglify after. I also kept the first de-uglify since I don't
>>> want to break existing stuff, and this is not a lot of fun to debug.
>>>
>>> On Sun, May 6, 2012 at 1:53 PM,  <ibmirkin@gmail.com> wrote:
>>>> Reviewers: bolinfest,
>>>>
>>>> Description:
>>>> Fix serving of files when the root dir is './'
>>>>
>>>> Please review this at http://codereview.appspot.com/6195049/
>>>>
>>>> Affected files:
>>>>  M src/org/plovr/Manifest.java
>>>>
>>>>
>>>> Index: src/org/plovr/Manifest.java
>>>> ===================================================================
>>>> --- a/src/org/plovr/Manifest.java
>>>> +++ b/src/org/plovr/Manifest.java
>>>> @@ -421,15 +421,19 @@
>>>>         // so strip the leading "./" from the JsInput name in this case.
>>>>         String name;
>>>>         if (file.equals(rootOfSearch.getFile())) {
>>>> -          name = rootOfSearch.getName();
>>>> +          name = "";
>>>>         } else {
>>>>           name = getRelativePath.apply(file);
>>>> -          final String uglyPrefix = "./";
>>>> -          if (name.startsWith(uglyPrefix)) {
>>>> -            name = name.substring(uglyPrefix.length());
>>>> -          }
>>>> -          name = rootOfSearch.getName() + name;
>>>>         }
>>>> +        final String uglyPrefix = "./";
>>>> +        if (name.startsWith(uglyPrefix)) {
>>>> +          name = name.substring(uglyPrefix.length());
>>>> +        }
>>>> +        name = rootOfSearch.getName() + name;
>>>> +        if (name.startsWith(uglyPrefix)) {
>>>> +          name = name.substring(uglyPrefix.length());
>>>> +        }
>>>> +
>>>>         JsInput input = LocalFileJsInput.createForFileWithName(file, name,
>>>>             soyFileOptions);
>>>>         logger.config("Dependency: " + input);
>>>>
>>>>

[Ilia Mirkin, 2012-06-01 16:47:00]

ping

see plovr mailing list, apparently this affects more than just soy files in some
conditions.

let me know how you want to proceed.

[bolinfest, 2012-06-04 17:28:27]

http://codereview.appspot.com/6195049/diff/1/src/org/plovr/Manifest.java
File src/org/plovr/Manifest.java (right):

http://codereview.appspot.com/6195049/diff/1/src/org/plovr/Manifest.java#newc...
src/org/plovr/Manifest.java:424: name = "";
So if I have:

"inputs": "foo.js"

will this condition be true? Shouldn't name be "foo.js" rather than ""?

Also, I realize that files are hard to mock out, but is it possible to provide a
test case for this change?

[imirkin_alum.mit.edu, 2012-06-04 17:29:28]

Did you have any opinion on whether I should extend the hack (as I've
done in this change) or switch all "." to "$" as you do for ..?

  -ilia

On Mon, Jun 4, 2012 at 1:28 PM,  <bolinfest@gmail.com> wrote:
>
> http://codereview.appspot.com/6195049/diff/1/src/org/plovr/Manifest.java
> File src/org/plovr/Manifest.java (right):
>
>
http://codereview.appspot.com/6195049/diff/1/src/org/plovr/Manifest.java#newc...
> src/org/plovr/Manifest.java:424: name = "";
> So if I have:
>
> "inputs": "foo.js"
>
> will this condition be true? Shouldn't name be "foo.js" rather than ""?
>
> Also, I realize that files are hard to mock out, but is it possible to
> provide a test case for this change?
>
> http://codereview.appspot.com/6195049/

[bolinfest, 2012-06-04 17:31:22]

I would rather try to eliminate the . where appropriate. Using the $$ makes
URLs harder to read, but I didn't have another way around that one.

On Mon, Jun 4, 2012 at 10:29 AM, Ilia Mirkin <imirkin@alum.mit.edu> wrote:

> Did you have any opinion on whether I should extend the hack (as I've
> done in this change) or switch all "." to "$" as you do for ..?
>
>  -ilia
>
> On Mon, Jun 4, 2012 at 1:28 PM,  <bolinfest@gmail.com> wrote:
> >
> > http://codereview.appspot.com/6195049/diff/1/src/org/plovr/Manifest.java
> > File src/org/plovr/Manifest.java (right):
> >
> >
>
http://codereview.appspot.com/6195049/diff/1/src/org/plovr/Manifest.java#newc...
> > src/org/plovr/Manifest.java:424: name = "";
> > So if I have:
> >
> > "inputs": "foo.js"
> >
> > will this condition be true? Shouldn't name be "foo.js" rather than ""?
> >
> > Also, I realize that files are hard to mock out, but is it possible to
> > provide a test case for this change?
> >
> > http://codereview.appspot.com/6195049/
>

[bolinfest, 2012-06-04 17:33:01]

Also, what is the subject of the message on this on the mailing list?

On Mon, Jun 4, 2012 at 10:31 AM, Michael Bolin <bolinfest@gmail.com> wrote:

> I would rather try to eliminate the . where appropriate. Using the $$
> makes URLs harder to read, but I didn't have another way around that one.
>
>
> On Mon, Jun 4, 2012 at 10:29 AM, Ilia Mirkin <imirkin@alum.mit.edu> wrote:
>
>> Did you have any opinion on whether I should extend the hack (as I've
>> done in this change) or switch all "." to "$" as you do for ..?
>>
>>  -ilia
>>
>> On Mon, Jun 4, 2012 at 1:28 PM,  <bolinfest@gmail.com> wrote:
>> >
>> >
>> http://codereview.appspot.com/6195049/diff/1/src/org/plovr/Manifest.java
>> > File src/org/plovr/Manifest.java (right):
>> >
>> >
>>
http://codereview.appspot.com/6195049/diff/1/src/org/plovr/Manifest.java#newc...
>> > src/org/plovr/Manifest.java:424: name = "";
>> > So if I have:
>> >
>> > "inputs": "foo.js"
>> >
>> > will this condition be true? Shouldn't name be "foo.js" rather than ""?
>> >
>> > Also, I realize that files are hard to mock out, but is it possible to
>> > provide a test case for this change?
>> >
>> > http://codereview.appspot.com/6195049/
>>
>
>

[imirkin_alum.mit.edu, 2012-06-04 17:33:47]

Something completely obscure...
https://groups.google.com/forum/?fromgroups#!topic/plovr/LFIxO-gJaIQ

On Mon, Jun 4, 2012 at 1:33 PM, Michael Bolin <bolinfest@gmail.com> wrote:
> Also, what is the subject of the message on this on the mailing list?
>
>
> On Mon, Jun 4, 2012 at 10:31 AM, Michael Bolin <bolinfest@gmail.com> wrote:
>>
>> I would rather try to eliminate the . where appropriate. Using the $$
>> makes URLs harder to read, but I didn't have another way around that one.
>>
>>
>> On Mon, Jun 4, 2012 at 10:29 AM, Ilia Mirkin <imirkin@alum.mit.edu> wrote:
>>>
>>> Did you have any opinion on whether I should extend the hack (as I've
>>> done in this change) or switch all "." to "$" as you do for ..?
>>>
>>>  -ilia
>>>
>>> On Mon, Jun 4, 2012 at 1:28 PM,  <bolinfest@gmail.com> wrote:
>>> >
>>> >
>>> > http://codereview.appspot.com/6195049/diff/1/src/org/plovr/Manifest.java
>>> > File src/org/plovr/Manifest.java (right):
>>> >
>>> >
>>> >
http://codereview.appspot.com/6195049/diff/1/src/org/plovr/Manifest.java#newc...
>>> > src/org/plovr/Manifest.java:424: name = "";
>>> > So if I have:
>>> >
>>> > "inputs": "foo.js"
>>> >
>>> > will this condition be true? Shouldn't name be "foo.js" rather than ""?
>>> >
>>> > Also, I realize that files are hard to mock out, but is it possible to
>>> > provide a test case for this change?
>>> >
>>> > http://codereview.appspot.com/6195049/
>>
>>
>

[Ilia Mirkin, 2012-06-22 18:54:26]

http://codereview.appspot.com/6195049/diff/1/src/org/plovr/Manifest.java
File src/org/plovr/Manifest.java (right):

http://codereview.appspot.com/6195049/diff/1/src/org/plovr/Manifest.java#newc...
src/org/plovr/Manifest.java:424: name = "";
On 2012/06/04 17:28:28, bolinfest wrote:
> So if I have:
> 
> "inputs": "foo.js"
> 
> will this condition be true? Shouldn't name be "foo.js" rather than ""?

Hmmmm.... perhaps. I will try to figure that out. Unfortunately last weekend was
shot, and this weekend isn't looking much better. Maybe tonight.

> 
> Also, I realize that files are hard to mock out, but is it possible to provide
a
> test case for this change?

Yeah, I looked around, unsuccessfully. However right now
testdata/example/test-raw.html breaks for me, and with this change it works.
(IIRC, I did all this a while ago.)