[PATCH] addremove: print relative paths when called with -I/-X

Matt Harbison mharbison72 at gmail.com
Thu Dec 4 21:48:20 CST 2014


On Thu, 04 Dec 2014 16:27:30 -0500, Martin von Zweigbergk  
<martinvonz at google.com> wrote:

> On Thu Dec 04 2014 at 9:59:20 AM Matt Harbison <mharbison72 at gmail.com>
> wrote:
>
>>
>>
>> On Thu, Dec 4, 2014 at 12:40 PM, Martin von Zweigbergk <
>> martinvonz at google.com> wrote:
>>
>>>
>>>
>>> [snip]
>>>
>>>
>>>>
>>>>
>>>>>   I didn't run the
>>>>> >> tests to see if that breaks anything.  (I'm not sure why it didn't
>>>>> also
>>>>> >> list the directory I was in as a relpath too.)
>>>>> >>
>>>>> >
>>>>> > Without having spent much thought to it, does 'not m.always()' work
>>>>> > better?
>>>>> > I ask because that's what it seemed like status and locate wanted
>>>>> when I
>>>>> > checked. It's just mostly the same for addremove since exact() was
>>>>> > already
>>>>> > discounted.
>>>>>
>>>>> I'm not sure.  My concern is that largefiles creates matchers with  
>>>>> the
>>>>> _always field set to False.  See composenormalfilematcher() in
>>>>> largefiles/overrides.py.  I haven't sent the second half of the  
>>>>> series
>>>>> yet, but that matcher will be passed to this code, and it basically
>>>>> eliminates the conditional when it is hardcoded to False.  It makes
>>>>> sense
>>>>> that the largefiles matcher is hardcoded, since it is excluding some
>>>>> files.  So this idea seems close, but a step in the wrong direction.
>>>>>
>>>>> It seems surprising to me that a list called patterns can be given to
>>>>> the
>>>>> matcher, and then the matcher reports that no patterns were given.
>>>>> Maybe
>>>>> it is just an unfortunate similarity in the names, but it seems like
>>>>> that
>>>>> needs to be fixed (or better documented what the differences are).
>>>>>
>>>>> --Matt
>>>>>
>>>>
>>>> Does it seem fair to say that we want to use relative paths in the UI  
>>>> in
>>>> a few cases (addremove, status, locate) if the user has not  
>>>> restricted the
>>>> paths in any way? If it is, it seems like we would need a flag on the
>>>> matcher that is disconnected from what the matcher actually matches.  
>>>> That
>>>> might be your _anyfiles, but modified to be "bool(patterns or include
>>>> or exclude)" and maybe renamed to _userelativepathsinui (but  
>>>> shortened).
>>>> *sigh*
>>>>
>>> But in that case it seems better to just keep the knowledge outside of
>>> matcher. Does the need to pass it around in the matcher arise from  
>>> subrepo
>>> use cases? I guess I didn't get that part from your patches.
>>>
>>
>> I'm not sure I understand.  I replaced the patterns list with a matcher,
>> because it seems like a matcher is more functional for subrepos.  E.g.  
>> the
>> pattern list isn't narrowed if I kept passing it along to subrepos, so  
>> it
>> is both (kind of) redundant with the matcher and wrong inside a subrepo.
>> The only thing it is good for at that point is to decide abs or rel.
>>
>
> I'm not sure what the right thing to do is. Do you think it makes sense  
> to
> add simply add "_pathrestricted = bool(patterns or include or exclude)"
> (feel free to pick another name) and the other things you added in your
> patches?

Yes, that's probably the simplest (and externally cleanest) thing to do.   
You were right about not modifying anypats()- commit calls this to make  
sure that -I/-X isn't used on commit, but does allow committing specific  
files, and that would have broke.


>> It might be nice to move the ((pats and rel) or abs) logic into a  
>> matcher
>> function to hide that logic, and then however we store that info doesn't
>> have to be public on the matcher.  I just don't know if the pattern is  
>> used
>> enough to justify that move, and nobody chimed in about why some methods
>> use abs vs rel and others don't.  Passing along an external 'what print
>> style should is use' parameter seems ugly to me.
>>
>
> Maybe pathrestricted() needs to be checked except where we print the  
> path.
> If that's the case, you can even skip adding the method and just have a
> 'def uipath(f): return self._pathrestricted and self.rel(f) or f'.

I agree with the latter, and like that name.  This worked with the entire  
series, so now I'm running the full test suite on Windows, and hopefully  
will submit it tonight.  (It takes almost 2 hours to run the full thing.)   
If accepted, I'll rebase, submit v3 of the series and carry on.

Thanks for brainstorming out loud on this.

--Matt


More information about the Mercurial-devel mailing list