[PATCH] subrepos: support adding files in git subrepos

[Mathias De Maré]

On Wed, Feb 25, 2015 at 2:46 AM, Matt Harbison <mharbison72 at gmail.com>
wrote:

> On Tue, 24 Feb 2015 02:49:49 -0500, Mathias De Maré <
> mathias.demare at gmail.com> wrote:
>
>  # HG changeset patch
>> # User Mathias De Maré <mathias.demare at gmail.com>
>> # Date 1424764162 -3600
>> #      Tue Feb 24 08:49:22 2015 +0100
>> # Node ID 391c33821c0399e40979ef2440289844f8e12756
>> # Parent  ff5caa8dfd993680d9602ca6ebb14da9de10d5f4
>> subrepos: support adding files in git subrepos
>>
>> This support includes correct matching, so includes,
>> excludes and patterns are all supported.
>>
>> diff --git a/mercurial/subrepo.py b/mercurial/subrepo.py
>> --- a/mercurial/subrepo.py
>> +++ b/mercurial/subrepo.py
>> @@ -1524,6 +1524,24 @@
>>              return False
>>     @annotatesubrepoerror
>> +    def add(self, ui, match, prefix, explicitonly, **opts):
>> +        rev = self._state[1]
>> +        if match.files():
>> +            files = match.files()
>> +        elif match.always():
>> +            self._gitcommand(["add", "--all"])
>> +            return []
>> +        else:
>> +            (modified, added, removed,
>> +             deleted, unknown, ignored, clean) = self.status(None)
>> +            files = unknown
>> +        if match:
>> +            files = [f for f in files if match(f)]
>>
>
> Will match ever be None, since .files() was called on it first?
>
Hm, I'll go over the code calling this, I'm unsure.

>
>  +        for f in files:
>> +            self._gitcommand(["add", f])
>> +        return []
>> +
>> +    @annotatesubrepoerror
>>      def remove(self):
>>          if self._gitmissing():
>>              return
>>
>
> Do we need this _gitmissing() check for add() too?  Don't forget to update
> mercurial/help/subrepos.txt.
>
I'll have a look as well :-) Note that 'remove' is not for removing git
files, it's for removing the entire subrepository. 'forget' is used for
removing files (but is not yet implemented for git).

>
> I was surprised that the matcher has all of the files in it, but your
> tests seem to prove it does.  Looking at the code, it looks like it goes
> cmdutil.add() -> wctx.walk(match) -> dirstate.walk(), which I guess picks
> up the (non-hg?) subrepo files.  But that dirstate walk is called in a way
> to ignore the files in the parent repo's .hgignore.  And since it doesn't
> know git's ignore file, will it end up adding files git wants to ignore if
> a pattern _other_ than an exact file is supplied, like a glob?  (IDK if git
> will add ignored files if explicitly named like hg.  Maybe it isn't an
> issue.)
>
'git help add' says:

>    -f, --force
>        Allow adding otherwise ignored files.
>
> So without using that option, it should apparently be ok (though I wonder
if it would give an error in that case, I'll test).

>
> The .gitignore issue you can probably solve by always calling
> git.status(), and only acting on the intersection of unknown and
> match.files(), unless a file is match.exact() (or match.always()).  I'm not
> sure what to suggest about bypassing .hgignore, other than to walk again
> with the correct parameter?  Maybe _always_ iterating over unknown from
> git.status() and calling the matcher on each entry?  That may fix both
> cases, but opens the door to silently ignoring the error for adding an
> already tracked file.  That may not be worth worrying about.
>
I'll look into this in a bit more detail. Explicitly using the git status()
seems like the best fix, but I'll add some additional tests, both with a
.hgignore and with a .gitignore.

Thanks for the feedback!

Greetings,
Mathias

>
> --Matt
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://selenic.com/pipermail/mercurial-devel/attachments/20150225/ad6d8d98/attachment.html>