Fix looking for local versions of cached files

Revert changes to GetCachedFile() in r30414, r30910, r31586, and go back to
the behavior before I started messing with it. It turns out it's not worth the
hassle to try and "clean this up"; we depend on the current convoluted
semantics, which is to crawl up the tree from the current dir looking for
codereview.settings files, but to only ever look for PRESUBMIT.py in the
root of the repo. Trying to unify the logic seems to be too painful.

   BUG=none
   R=maruel@chromium.org
   TEST=none


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=33648

[Dirk Pranke, 2009-11-17 02:12:02]

I'm starting to agree with you that this change is more trouble than its worth
...

[M-A Ruel, 2009-11-17 02:25:07]

http://codereview.chromium.org/387067/diff/4001/5001
File gcl.py (right):

http://codereview.chromium.org/387067/diff/4001/5001#newcode157
Line 157: while True:
Actually, there is something weird in that loop:
You check at the same time for a file on the local file system and on the remote
svn server, switching between each test.

Shouldn't you look for one first? Are you sure you know what you are doing? :)

[Dirk Pranke, 2009-11-17 02:56:59]

http://codereview.chromium.org/387067/diff/4001/5001
File gcl.py (right):

http://codereview.chromium.org/387067/diff/4001/5001#newcode157
Line 157: while True:
On 2009/11/17 02:25:07, Marc-Antoine Ruel wrote:
> Actually, there is something weird in that loop:
> You check at the same time for a file on the local file system and on the
remote
> svn server, switching between each test.
> 
> Shouldn't you look for one first? Are you sure you know what you are doing? :)

I'm not sure I follow you. I don't know what the "switching between each test"
is referring to.

I look first for a local file, then I look on the server, then I go up a
directory, and repeat. At least, that's what it's supposed to do. Are you saying
that that isn't what it does, or that that isn't what it should be doing?

It's certainly possible that I don't know what I'm doing (or should be doing
...)

[M-A Ruel, 2009-11-17 15:06:56]

On 2009/11/17 02:56:59, dpranke wrote:
> http://codereview.chromium.org/387067/diff/4001/5001
> File gcl.py (right):
> 
> http://codereview.chromium.org/387067/diff/4001/5001#newcode157
> Line 157: while True:
> On 2009/11/17 02:25:07, Marc-Antoine Ruel wrote:
> > Actually, there is something weird in that loop:
> > You check at the same time for a file on the local file system and on the
> remote
> > svn server, switching between each test.
> > 
> > Shouldn't you look for one first? Are you sure you know what you are doing?
:)
> 
> I'm not sure I follow you. I don't know what the "switching between each test"
> is referring to.

I refer to the fact that you don't have a clear prioritization between local
file system and remote repository. Which one should take over?

> I look first for a local file, then I look on the server, then I go up a
> directory, and repeat. At least, that's what it's supposed to do. Are you
saying
> that that isn't what it does, or that that isn't what it should be doing?
> 
> It's certainly possible that I don't know what I'm doing (or should be doing
> ...)

Do you know in which directory the check starts? You start the check at the
current directory on line 147. What is the current directory at that point? Are
you sure? This is very unclear and we usually try to make gcl work seamlessly as
long as you are inside the checkout root.

[M-A Ruel, 2009-11-23 19:34:48]

http://codereview.chromium.org/387067/diff/8001/9001
File gcl.py (right):

http://codereview.chromium.org/387067/diff/8001/9001#newcode152
gcl.py:152: settings_file = FindUpwardAndReadFile(CODEREVIEW_SETTINGS_FILE)
It won't work if codereview.settings isn't in the checked out tree. It happens.

[M-A Ruel, 2009-11-23 21:50:15]

On Mon, Nov 23, 2009 at 4:46 PM, Dirk Pranke <dpranke@chromium.org> wrote:

> Yeah, I thought about this and couldn't figure out a use case where
> codereview.settings didn't exist locally and that would be a good
> thing (so the design was intentional).
>
> Can you give me an example?
>

svn co svn://svn.chromium.org/chrome/trunk/tools/depot_tools
touch bleh
svn add bleh
gcl change bleh
gcl upload bleh
-> fail.



> -- Dirk
>
> On Mon, Nov 23, 2009 at 11:34 AM,  <maruel@chromium.org> wrote:
> >
> > http://codereview.chromium.org/387067/diff/8001/9001
> > File gcl.py (right):
> >
> > http://codereview.chromium.org/387067/diff/8001/9001#newcode152
> > gcl.py:152: settings_file =
> > FindUpwardAndReadFile(CODEREVIEW_SETTINGS_FILE)
> > It won't work if codereview.settings isn't in the checked out tree. It
> > happens.
> >
> > http://codereview.chromium.org/387067
> >
>

[M-A Ruel, 2009-11-24 01:20:20]

On Mon, Nov 23, 2009 at 5:48 PM, Dirk Pranke <dpranke@chromium.org> wrote:

> I assume in your example there's a "cd depot_tools" in between the svn
> co and the touch. Otherwise, I don't know what the semantics would be
> here.
>
> Okay, your example make sense, I guess I didn't really think about
> people who only check out a part of a tree. In your example, would you
> want to check in trunk/tools as well as trunk? I assume the answer is
> yes.
>

My answer is no. Does that make your day? :)


I.e., GetCachedFile() will only look for trunk/codereview.settings
> (because use_root is now always true).
>

I realized.



> On the other hand, if this is the behavior we want,
> GetCachedFile('PRESUBMIT.py') will only find trunk/PRESUBMIT.py,
> and not trunk/tools/PRESUBMIT.py (if it existed). This means there can
> only be one PRESUBMIT.py per repo. I think this
> is how the code would've worked before. Is that desirable?
>

Yes it is. I think we can live without the path crawl.



> -- Dirk
>
> (and for the record, I clearly should've left things alone without
> ever trying to "fix" this ...)
>

I had warned you. :P

M-A



> On Mon, Nov 23, 2009 at 1:49 PM, Marc-Antoine Ruel <maruel@chromium.org>
> wrote:
> > On Mon, Nov 23, 2009 at 4:46 PM, Dirk Pranke <dpranke@chromium.org>
> wrote:
> >>
> >> Yeah, I thought about this and couldn't figure out a use case where
> >> codereview.settings didn't exist locally and that would be a good
> >> thing (so the design was intentional).
> >>
> >> Can you give me an example?
> >
> > svn co svn://svn.chromium.org/chrome/trunk/tools/depot_tools
> > touch bleh
> > svn add bleh
> > gcl change bleh
> > gcl upload bleh
> > -> fail.
> >
> >
> >>
> >> -- Dirk
> >>
> >> On Mon, Nov 23, 2009 at 11:34 AM,  <maruel@chromium.org> wrote:
> >> >
> >> > http://codereview.chromium.org/387067/diff/8001/9001
> >> > File gcl.py (right):
> >> >
> >> > http://codereview.chromium.org/387067/diff/8001/9001#newcode152
> >> > gcl.py:152: settings_file =
> >> > FindUpwardAndReadFile(CODEREVIEW_SETTINGS_FILE)
> >> > It won't work if codereview.settings isn't in the checked out tree. It
> >> > happens.
> >> >
> >> > http://codereview.chromium.org/387067
> >> >
> >
> >
>

[M-A Ruel, 2009-11-24 01:50:58]

On Mon, Nov 23, 2009 at 8:29 PM, Dirk Pranke <dpranke@chromium.org> wrote:

> Okay, in this case, then I think the right thing to do is if
> FindUpwardsAndReadFile(X) should return GetCachedFile(x) in the
> failure case. Does that sound right?
>

Ok, well, that's what you wanted first.



> I'm actually not sure about not wanting to crawl up the remote tree,
> though. What if I checked out src/webkit/tools/layout_tests? shouldn't
> I look in src, webkit, and tools before skipping up to the top of the
> repo?
>

For your FindUpwardsAndReadFile()? I don't know, it's you that wanted that
behavior! :)


>
> -- Dirk
>
> On Mon, Nov 23, 2009 at 5:20 PM, Marc-Antoine Ruel <maruel@chromium.org>
> wrote:
> > On Mon, Nov 23, 2009 at 5:48 PM, Dirk Pranke <dpranke@chromium.org>
> wrote:
> >>
> >> I assume in your example there's a "cd depot_tools" in between the svn
> >> co and the touch. Otherwise, I don't know what the semantics would be
> >> here.
> >>
> >> Okay, your example make sense, I guess I didn't really think about
> >> people who only check out a part of a tree. In your example, would you
> >> want to check in trunk/tools as well as trunk? I assume the answer is
> >> yes.
> >
> >
> > My answer is no. Does that make your day? :)
> >
> >> I.e., GetCachedFile() will only look for trunk/codereview.settings
> >> (because use_root is now always true).
> >
> > I realized.
> >
> >
> >>
> >> On the other hand, if this is the behavior we want,
> >> GetCachedFile('PRESUBMIT.py') will only find trunk/PRESUBMIT.py,
> >> and not trunk/tools/PRESUBMIT.py (if it existed). This means there can
> >> only be one PRESUBMIT.py per repo. I think this
> >> is how the code would've worked before. Is that desirable?
> >
> > Yes it is. I think we can live without the path crawl.
> >
> >>
> >> -- Dirk
> >>
> >> (and for the record, I clearly should've left things alone without
> >> ever trying to "fix" this ...)
> >
> > I had warned you. :P
> > M-A
> >
> >>
> >> On Mon, Nov 23, 2009 at 1:49 PM, Marc-Antoine Ruel <maruel@chromium.org
> >
> >> wrote:
> >> > On Mon, Nov 23, 2009 at 4:46 PM, Dirk Pranke <dpranke@chromium.org>
> >> > wrote:
> >> >>
> >> >> Yeah, I thought about this and couldn't figure out a use case where
> >> >> codereview.settings didn't exist locally and that would be a good
> >> >> thing (so the design was intentional).
> >> >>
> >> >> Can you give me an example?
> >> >
> >> > svn co svn://svn.chromium.org/chrome/trunk/tools/depot_tools
> >> > touch bleh
> >> > svn add bleh
> >> > gcl change bleh
> >> > gcl upload bleh
> >> > -> fail.
> >> >
> >> >
> >> >>
> >> >> -- Dirk
> >> >>
> >> >> On Mon, Nov 23, 2009 at 11:34 AM,  <maruel@chromium.org> wrote:
> >> >> >
> >> >> > http://codereview.chromium.org/387067/diff/8001/9001
> >> >> > File gcl.py (right):
> >> >> >
> >> >> > http://codereview.chromium.org/387067/diff/8001/9001#newcode152
> >> >> > gcl.py:152: settings_file =
> >> >> > FindUpwardAndReadFile(CODEREVIEW_SETTINGS_FILE)
> >> >> > It won't work if codereview.settings isn't in the checked out tree.
> >> >> > It
> >> >> > happens.
> >> >> >
> >> >> > http://codereview.chromium.org/387067
> >> >> >
> >> >
> >> >
> >
> >
>

[M-A Ruel, 2009-11-24 16:57:44]

Oh, sorry about your head.

That's weird because you are saying exactly the reverse of what I said in
the previous message.

I really said to use use_root=True everywhere. See «Yes it is. I think we
can live without the path crawl.»

M-A


On Mon, Nov 23, 2009 at 9:32 PM, Dirk Pranke <dpranke@chromium.org> wrote:

> My brain hurts.
>
> Okay, I'll take one last stab at this ... I suggest that in all cases,
> you look locally in the current working directory, climb up to
> the top of the checkout, and then look remotely in directories between
> the top of the checkout and the top of the repo.
>
> This would essentially mean we'd treat use_root=False everywhere, not
> just in codereview.settings. Thus, if you had
> /trunk/src/webkit/tools/PRESUBMIT.py , we'd use that over
> /PRESUBMIT.py (today, we'd only ever use /PRESUBMIT.py, and
> we'd ignore the files in the subdirectories).
>
> Does that sound like a reasonable semantic? If we actually want to use
> different search paths for the two different files, then
> I think we can't get a particularly clean semantic and might as well
> roll back the patch.
>
> -- Dirk
>
> On Mon, Nov 23, 2009 at 5:48 PM, Marc-Antoine Ruel <maruel@chromium.org>
> wrote:
> > On Mon, Nov 23, 2009 at 8:29 PM, Dirk Pranke <dpranke@chromium.org>
> wrote:
> >>
> >> Okay, in this case, then I think the right thing to do is if
> >> FindUpwardsAndReadFile(X) should return GetCachedFile(x) in the
> >> failure case. Does that sound right?
> >
> > Ok, well, that's what you wanted first.
> >
> >
> >>
> >> I'm actually not sure about not wanting to crawl up the remote tree,
> >> though. What if I checked out src/webkit/tools/layout_tests? shouldn't
> >> I look in src, webkit, and tools before skipping up to the top of the
> >> repo?
> >
> > For your FindUpwardsAndReadFile()? I don't know, it's you that wanted
> that
> > behavior! :)
> >
> >>
> >> -- Dirk
> >>
> >> On Mon, Nov 23, 2009 at 5:20 PM, Marc-Antoine Ruel <maruel@chromium.org
> >
> >> wrote:
> >> > On Mon, Nov 23, 2009 at 5:48 PM, Dirk Pranke <dpranke@chromium.org>
> >> > wrote:
> >> >>
> >> >> I assume in your example there's a "cd depot_tools" in between the
> svn
> >> >> co and the touch. Otherwise, I don't know what the semantics would be
> >> >> here.
> >> >>
> >> >> Okay, your example make sense, I guess I didn't really think about
> >> >> people who only check out a part of a tree. In your example, would
> you
> >> >> want to check in trunk/tools as well as trunk? I assume the answer is
> >> >> yes.
> >> >
> >> >
> >> > My answer is no. Does that make your day? :)
> >> >
> >> >> I.e., GetCachedFile() will only look for trunk/codereview.settings
> >> >> (because use_root is now always true).
> >> >
> >> > I realized.
> >> >
> >> >
> >> >>
> >> >> On the other hand, if this is the behavior we want,
> >> >> GetCachedFile('PRESUBMIT.py') will only find trunk/PRESUBMIT.py,
> >> >> and not trunk/tools/PRESUBMIT.py (if it existed). This means there
> can
> >> >> only be one PRESUBMIT.py per repo. I think this
> >> >> is how the code would've worked before. Is that desirable?
> >> >
> >> > Yes it is. I think we can live without the path crawl.
> >> >
> >> >>
> >> >> -- Dirk
> >> >>
> >> >> (and for the record, I clearly should've left things alone without
> >> >> ever trying to "fix" this ...)
> >> >
> >> > I had warned you. :P
> >> > M-A
> >> >
> >> >>
> >> >> On Mon, Nov 23, 2009 at 1:49 PM, Marc-Antoine Ruel
> >> >> <maruel@chromium.org>
> >> >> wrote:
> >> >> > On Mon, Nov 23, 2009 at 4:46 PM, Dirk Pranke <dpranke@chromium.org
> >
> >> >> > wrote:
> >> >> >>
> >> >> >> Yeah, I thought about this and couldn't figure out a use case
> where
> >> >> >> codereview.settings didn't exist locally and that would be a good
> >> >> >> thing (so the design was intentional).
> >> >> >>
> >> >> >> Can you give me an example?
> >> >> >
> >> >> > svn co svn://svn.chromium.org/chrome/trunk/tools/depot_tools
> >> >> > touch bleh
> >> >> > svn add bleh
> >> >> > gcl change bleh
> >> >> > gcl upload bleh
> >> >> > -> fail.
> >> >> >
> >> >> >
> >> >> >>
> >> >> >> -- Dirk
> >> >> >>
> >> >> >> On Mon, Nov 23, 2009 at 11:34 AM,  <maruel@chromium.org> wrote:
> >> >> >> >
> >> >> >> > http://codereview.chromium.org/387067/diff/8001/9001
> >> >> >> > File gcl.py (right):
> >> >> >> >
> >> >> >> > http://codereview.chromium.org/387067/diff/8001/9001#newcode152
> >> >> >> > gcl.py:152: settings_file =
> >> >> >> > FindUpwardAndReadFile(CODEREVIEW_SETTINGS_FILE)
> >> >> >> > It won't work if codereview.settings isn't in the checked out
> >> >> >> > tree.
> >> >> >> > It
> >> >> >> > happens.
> >> >> >> >
> >> >> >> > http://codereview.chromium.org/387067
> >> >> >> >
> >> >> >
> >> >> >
> >> >
> >> >
> >
> >
>

[Dirk Pranke, 2009-11-24 21:17:15]

My brain hurts.

Okay, I'll take one last stab at this ... I suggest that in all cases,
you look locally in the current working directory, climb up to
the top of the checkout, and then look remotely in directories between
the top of the checkout and the top of the repo.

This would essentially mean we'd treat use_root=False everywhere, not
just in codereview.settings. Thus, if you had
/trunk/src/webkit/tools/PRESUBMIT.py , we'd use that over
/PRESUBMIT.py (today, we'd only ever use /PRESUBMIT.py, and
we'd ignore the files in the subdirectories).

Does that sound like a reasonable semantic? If we actually want to use
different search paths for the two different files, then
I think we can't get a particularly clean semantic and might as well
roll back the patch.

-- Dirk

On Mon, Nov 23, 2009 at 5:48 PM, Marc-Antoine Ruel <maruel@chromium.org> wrote:
> On Mon, Nov 23, 2009 at 8:29 PM, Dirk Pranke <dpranke@chromium.org> wrote:
>>
>> Okay, in this case, then I think the right thing to do is if
>> FindUpwardsAndReadFile(X) should return GetCachedFile(x) in the
>> failure case. Does that sound right?
>
> Ok, well, that's what you wanted first.
>
>
>>
>> I'm actually not sure about not wanting to crawl up the remote tree,
>> though. What if I checked out src/webkit/tools/layout_tests? shouldn't
>> I look in src, webkit, and tools before skipping up to the top of the
>> repo?
>
> For your FindUpwardsAndReadFile()? I don't know, it's you that wanted that
> behavior! :)
>
>>
>> -- Dirk
>>
>> On Mon, Nov 23, 2009 at 5:20 PM, Marc-Antoine Ruel <maruel@chromium.org>
>> wrote:
>> > On Mon, Nov 23, 2009 at 5:48 PM, Dirk Pranke <dpranke@chromium.org>
>> > wrote:
>> >>
>> >> I assume in your example there's a "cd depot_tools" in between the svn
>> >> co and the touch. Otherwise, I don't know what the semantics would be
>> >> here.
>> >>
>> >> Okay, your example make sense, I guess I didn't really think about
>> >> people who only check out a part of a tree. In your example, would you
>> >> want to check in trunk/tools as well as trunk? I assume the answer is
>> >> yes.
>> >
>> >
>> > My answer is no. Does that make your day? :)
>> >
>> >> I.e., GetCachedFile() will only look for trunk/codereview.settings
>> >> (because use_root is now always true).
>> >
>> > I realized.
>> >
>> >
>> >>
>> >> On the other hand, if this is the behavior we want,
>> >> GetCachedFile('PRESUBMIT.py') will only find trunk/PRESUBMIT.py,
>> >> and not trunk/tools/PRESUBMIT.py (if it existed). This means there can
>> >> only be one PRESUBMIT.py per repo. I think this
>> >> is how the code would've worked before. Is that desirable?
>> >
>> > Yes it is. I think we can live without the path crawl.
>> >
>> >>
>> >> -- Dirk
>> >>
>> >> (and for the record, I clearly should've left things alone without
>> >> ever trying to "fix" this ...)
>> >
>> > I had warned you. :P
>> > M-A
>> >
>> >>
>> >> On Mon, Nov 23, 2009 at 1:49 PM, Marc-Antoine Ruel
>> >> <maruel@chromium.org>
>> >> wrote:
>> >> > On Mon, Nov 23, 2009 at 4:46 PM, Dirk Pranke <dpranke@chromium.org>
>> >> > wrote:
>> >> >>
>> >> >> Yeah, I thought about this and couldn't figure out a use case where
>> >> >> codereview.settings didn't exist locally and that would be a good
>> >> >> thing (so the design was intentional).
>> >> >>
>> >> >> Can you give me an example?
>> >> >
>> >> > svn co svn://svn.chromium.org/chrome/trunk/tools/depot_tools
>> >> > touch bleh
>> >> > svn add bleh
>> >> > gcl change bleh
>> >> > gcl upload bleh
>> >> > -> fail.
>> >> >
>> >> >
>> >> >>
>> >> >> -- Dirk
>> >> >>
>> >> >> On Mon, Nov 23, 2009 at 11:34 AM,  <maruel@chromium.org> wrote:
>> >> >> >
>> >> >> > http://codereview.chromium.org/387067/diff/8001/9001
>> >> >> > File gcl.py (right):
>> >> >> >
>> >> >> > http://codereview.chromium.org/387067/diff/8001/9001#newcode152
>> >> >> > gcl.py:152: settings_file =
>> >> >> > FindUpwardAndReadFile(CODEREVIEW_SETTINGS_FILE)
>> >> >> > It won't work if codereview.settings isn't in the checked out
>> >> >> > tree.
>> >> >> > It
>> >> >> > happens.
>> >> >> >
>> >> >> > http://codereview.chromium.org/387067
>> >> >> >
>> >> >
>> >> >
>> >
>> >
>
>

[Dirk Pranke, 2009-11-24 21:17:15]

Yeah, I thought about this and couldn't figure out a use case where
codereview.settings didn't exist locally and that would be a good
thing (so the design was intentional).

Can you give me an example?

-- Dirk

On Mon, Nov 23, 2009 at 11:34 AM,  <maruel@chromium.org> wrote:
>
> http://codereview.chromium.org/387067/diff/8001/9001
> File gcl.py (right):
>
> http://codereview.chromium.org/387067/diff/8001/9001#newcode152
> gcl.py:152: settings_file =
> FindUpwardAndReadFile(CODEREVIEW_SETTINGS_FILE)
> It won't work if codereview.settings isn't in the checked out tree. It
> happens.
>
> http://codereview.chromium.org/387067
>

[Dirk Pranke, 2009-11-24 21:17:16]

I assume in your example there's a "cd depot_tools" in between the svn
co and the touch. Otherwise, I don't know what the semantics would be
here.

Okay, your example make sense, I guess I didn't really think about
people who only check out a part of a tree. In your example, would you
want to check in trunk/tools as well as trunk? I assume the answer is
yes.

I.e., GetCachedFile() will only look for trunk/codereview.settings
(because use_root is now always true).

On the other hand, if this is the behavior we want,
GetCachedFile('PRESUBMIT.py') will only find trunk/PRESUBMIT.py,
and not trunk/tools/PRESUBMIT.py (if it existed). This means there can
only be one PRESUBMIT.py per repo. I think this
is how the code would've worked before. Is that desirable?

-- Dirk

(and for the record, I clearly should've left things alone without
ever trying to "fix" this ...)

On Mon, Nov 23, 2009 at 1:49 PM, Marc-Antoine Ruel <maruel@chromium.org> wrote:
> On Mon, Nov 23, 2009 at 4:46 PM, Dirk Pranke <dpranke@chromium.org> wrote:
>>
>> Yeah, I thought about this and couldn't figure out a use case where
>> codereview.settings didn't exist locally and that would be a good
>> thing (so the design was intentional).
>>
>> Can you give me an example?
>
> svn co svn://svn.chromium.org/chrome/trunk/tools/depot_tools
> touch bleh
> svn add bleh
> gcl change bleh
> gcl upload bleh
> -> fail.
>
>
>>
>> -- Dirk
>>
>> On Mon, Nov 23, 2009 at 11:34 AM,  <maruel@chromium.org> wrote:
>> >
>> > http://codereview.chromium.org/387067/diff/8001/9001
>> > File gcl.py (right):
>> >
>> > http://codereview.chromium.org/387067/diff/8001/9001#newcode152
>> > gcl.py:152: settings_file =
>> > FindUpwardAndReadFile(CODEREVIEW_SETTINGS_FILE)
>> > It won't work if codereview.settings isn't in the checked out tree. It
>> > happens.
>> >
>> > http://codereview.chromium.org/387067
>> >
>
>

[Dirk Pranke, 2009-11-24 21:17:16]

Okay, in this case, then I think the right thing to do is if
FindUpwardsAndReadFile(X) should return GetCachedFile(x) in the
failure case. Does that sound right?

I'm actually not sure about not wanting to crawl up the remote tree,
though. What if I checked out src/webkit/tools/layout_tests? shouldn't
I look in src, webkit, and tools before skipping up to the top of the
repo?

-- Dirk

On Mon, Nov 23, 2009 at 5:20 PM, Marc-Antoine Ruel <maruel@chromium.org> wrote:
> On Mon, Nov 23, 2009 at 5:48 PM, Dirk Pranke <dpranke@chromium.org> wrote:
>>
>> I assume in your example there's a "cd depot_tools" in between the svn
>> co and the touch. Otherwise, I don't know what the semantics would be
>> here.
>>
>> Okay, your example make sense, I guess I didn't really think about
>> people who only check out a part of a tree. In your example, would you
>> want to check in trunk/tools as well as trunk? I assume the answer is
>> yes.
>
>
> My answer is no. Does that make your day? :)
>
>> I.e., GetCachedFile() will only look for trunk/codereview.settings
>> (because use_root is now always true).
>
> I realized.
>
>
>>
>> On the other hand, if this is the behavior we want,
>> GetCachedFile('PRESUBMIT.py') will only find trunk/PRESUBMIT.py,
>> and not trunk/tools/PRESUBMIT.py (if it existed). This means there can
>> only be one PRESUBMIT.py per repo. I think this
>> is how the code would've worked before. Is that desirable?
>
> Yes it is. I think we can live without the path crawl.
>
>>
>> -- Dirk
>>
>> (and for the record, I clearly should've left things alone without
>> ever trying to "fix" this ...)
>
> I had warned you. :P
> M-A
>
>>
>> On Mon, Nov 23, 2009 at 1:49 PM, Marc-Antoine Ruel <maruel@chromium.org>
>> wrote:
>> > On Mon, Nov 23, 2009 at 4:46 PM, Dirk Pranke <dpranke@chromium.org>
>> > wrote:
>> >>
>> >> Yeah, I thought about this and couldn't figure out a use case where
>> >> codereview.settings didn't exist locally and that would be a good
>> >> thing (so the design was intentional).
>> >>
>> >> Can you give me an example?
>> >
>> > svn co svn://svn.chromium.org/chrome/trunk/tools/depot_tools
>> > touch bleh
>> > svn add bleh
>> > gcl change bleh
>> > gcl upload bleh
>> > -> fail.
>> >
>> >
>> >>
>> >> -- Dirk
>> >>
>> >> On Mon, Nov 23, 2009 at 11:34 AM,  <maruel@chromium.org> wrote:
>> >> >
>> >> > http://codereview.chromium.org/387067/diff/8001/9001
>> >> > File gcl.py (right):
>> >> >
>> >> > http://codereview.chromium.org/387067/diff/8001/9001#newcode152
>> >> > gcl.py:152: settings_file =
>> >> > FindUpwardAndReadFile(CODEREVIEW_SETTINGS_FILE)
>> >> > It won't work if codereview.settings isn't in the checked out tree.
>> >> > It
>> >> > happens.
>> >> >
>> >> > http://codereview.chromium.org/387067
>> >> >
>> >
>> >
>
>

[M-A Ruel, 2009-12-02 21:04:05]

On 2009/11/24 21:17:16, dpranke wrote:
> Okay, in this case, then I think the right thing to do is if
> FindUpwardsAndReadFile(X) should return GetCachedFile(x) in the
> failure case. Does that sound right?
> 
> I'm actually not sure about not wanting to crawl up the remote tree,
> though. What if I checked out src/webkit/tools/layout_tests? shouldn't
> I look in src, webkit, and tools before skipping up to the top of the
> repo?

I just want to have the error currently output about codereview.settings removed
and I'm still not sure locally modified check buys us anything (beside slowing
down gcl operations).

Please update.

[M-A Ruel, 2009-12-03 01:29:53]

lgtm

[Dirk Pranke, 2009-12-03 20:05:23]

Sorry, I've been delayed by other fixes. I'll get this in today.

-- Dirk

On Wed, Dec 2, 2009 at 1:04 PM,  <maruel@chromium.org> wrote:
> On 2009/11/24 21:17:16, dpranke wrote:
>>
>> Okay, in this case, then I think the right thing to do is if
>> FindUpwardsAndReadFile(X) should return GetCachedFile(x) in the
>> failure case. Does that sound right?
>
>> I'm actually not sure about not wanting to crawl up the remote tree,
>> though. What if I checked out src/webkit/tools/layout_tests? shouldn't
>> I look in src, webkit, and tools before skipping up to the top of the
>> repo?
>
> I just want to have the error currently output about codereview.settings
> removed
> and I'm still not sure locally modified check buys us anything (beside
> slowing
> down gcl operations).
>
> Please update.
>
> http://codereview.chromium.org/387067
>