Submitter | phabricator |
---|---|
Date | Sept. 13, 2019, 5:42 a.m. |
Message ID | <differential-rev-PHID-DREV-yc35ozcqzdryeibjfwng-req@mercurial-scm.org> |
Download | mbox | patch |
Permalink | /patch/41655/ |
State | Superseded |
Headers | show |
Comments
rdamazio added inline comments. INLINE COMMENTS > test-narrow.t:457 > + $ echo a >> d0/f > + $ hg ci -m 'local change to d0' > + $ hg co '.^' This obsolete commit is completely lost afterwards, which kind of violates the principle of "all changes are kept forever" - should it dump this somewhere for safekeeping? Also, what if you just don't get rid of the files, and simply remove it from the narrowspec? It'll stop processing/pulling/pushing the directory, but no data is lost. REPOSITORY rHG Mercurial CHANGES SINCE LAST ACTION https://phab.mercurial-scm.org/D6848/new/ REVISION DETAIL https://phab.mercurial-scm.org/D6848 To: martinvonz, durin42, #hg-reviewers Cc: rdamazio, mercurial-devel
martinvonz added inline comments. INLINE COMMENTS > rdamazio wrote in test-narrow.t:457 > This obsolete commit is completely lost afterwards, which kind of violates the principle of "all changes are kept forever" - should it dump this somewhere for safekeeping? > Also, what if you just don't get rid of the files, and simply remove it from the narrowspec? It'll stop processing/pulling/pushing the directory, but no data is lost. > This obsolete commit is completely lost afterwards, which kind of violates the principle of "all changes are kept forever" - should it dump this somewhere for safekeeping? Yes, it does dump it to a backup bundle (see output on line 474). > Also, what if you just don't get rid of the files, and simply remove it from the narrowspec? It'll stop processing/pulling/pushing the directory, but no data is lost. That's just about the working directory, right? This file is unchanged in the working directory at the time of `hg tracked --auto-remove-includes`... REPOSITORY rHG Mercurial CHANGES SINCE LAST ACTION https://phab.mercurial-scm.org/D6848/new/ REVISION DETAIL https://phab.mercurial-scm.org/D6848 To: martinvonz, durin42, #hg-reviewers Cc: rdamazio, mercurial-devel
rdamazio added inline comments. INLINE COMMENTS > martinvonz wrote in test-narrow.t:457 > > This obsolete commit is completely lost afterwards, which kind of violates the principle of "all changes are kept forever" - should it dump this somewhere for safekeeping? > > Yes, it does dump it to a backup bundle (see output on line 474). > > > Also, what if you just don't get rid of the files, and simply remove it from the narrowspec? It'll stop processing/pulling/pushing the directory, but no data is lost. > > That's just about the working directory, right? This file is unchanged in the working directory at the time of `hg tracked --auto-remove-includes`... > That's just about the working directory, right? This file is unchanged in the working directory at the time of hg tracked --auto-remove-includes... No, I meant what if you don't get rid of the .i files. But you raise a good point - shouldn't you also clean up the working directory? Alternatively, or in addition, maybe a confirmation prompt pointing out which commits and narrowspec entries will be dropped may be useful. REPOSITORY rHG Mercurial CHANGES SINCE LAST ACTION https://phab.mercurial-scm.org/D6848/new/ REVISION DETAIL https://phab.mercurial-scm.org/D6848 To: martinvonz, durin42, #hg-reviewers Cc: rdamazio, mercurial-devel
martinvonz added inline comments. INLINE COMMENTS > rdamazio wrote in test-narrow.t:457 > > That's just about the working directory, right? This file is unchanged in the working directory at the time of hg tracked --auto-remove-includes... > > No, I meant what if you don't get rid of the .i files. > But you raise a good point - shouldn't you also clean up the working directory? > > Alternatively, or in addition, maybe a confirmation prompt pointing out which commits and narrowspec entries will be dropped may be useful. > No, I meant what if you don't get rid of the .i files. This is not changing which .i files we get rid of; the behavior should be the same as if you had typed `hg tracked --removeinclude path:d0 --removeinclude path:d2`. > But you raise a good point - shouldn't you also clean up the working directory? Yes, it should be, but it doesn't print those files if they were clean. I've added a call to `hg files` to the test so you can see that they are cleaned up. > Alternatively, or in addition, maybe a confirmation prompt pointing out which commits and > narrowspec entries will be dropped may be useful. I considered that but I wasn't sure if it would be more annoying than helpful. Want me to add that? REPOSITORY rHG Mercurial CHANGES SINCE LAST ACTION https://phab.mercurial-scm.org/D6848/new/ REVISION DETAIL https://phab.mercurial-scm.org/D6848 To: martinvonz, durin42, #hg-reviewers Cc: rdamazio, mercurial-devel
martinvonz added inline comments. INLINE COMMENTS > martinvonz wrote in test-narrow.t:457 > > No, I meant what if you don't get rid of the .i files. > > This is not changing which .i files we get rid of; the behavior should be the same as if you had typed `hg tracked --removeinclude path:d0 --removeinclude path:d2`. > > > But you raise a good point - shouldn't you also clean up the working directory? > > Yes, it should be, but it doesn't print those files if they were clean. I've added a call to `hg files` to the test so you can see that they are cleaned up. > > > Alternatively, or in addition, maybe a confirmation prompt pointing out which commits and > > narrowspec entries will be dropped may be useful. > > I considered that but I wasn't sure if it would be more annoying than helpful. Want me to add that? I added that prompt. We also talked about adding a `--dry-run` option instead of the prompt, but that seemed harder because it would need to be handled in many different cases, including `--update-working-copy`, even if we just decide to error out. REPOSITORY rHG Mercurial CHANGES SINCE LAST ACTION https://phab.mercurial-scm.org/D6848/new/ REVISION DETAIL https://phab.mercurial-scm.org/D6848 To: martinvonz, durin42, #hg-reviewers Cc: rdamazio, mercurial-devel
pulkit added inline comments. INLINE COMMENTS > narrowcommands.py:332 > + ('', 'auto-remove-includes', False, > + _('automatically choose unused includes to remove (EXPERIMENTAL)')), > ('', 'addexclude', [], _('new paths to exclude')), Can you add some help text below on how these unused includes are calculated? REPOSITORY rHG Mercurial CHANGES SINCE LAST ACTION https://phab.mercurial-scm.org/D6848/new/ REVISION DETAIL https://phab.mercurial-scm.org/D6848 To: martinvonz, durin42, #hg-reviewers Cc: pulkit, rdamazio, mercurial-devel
martinvonz added inline comments. INLINE COMMENTS > pulkit wrote in narrowcommands.py:332 > Can you add some help text below on how these unused includes are calculated? Even though it's marked EXPERIMENTAL? Or maybe I should do that and just drop the EXPERIMENTAL since the whole feature (narrow clones) is experimental anyway? REPOSITORY rHG Mercurial CHANGES SINCE LAST ACTION https://phab.mercurial-scm.org/D6848/new/ REVISION DETAIL https://phab.mercurial-scm.org/D6848 To: martinvonz, durin42, #hg-reviewers Cc: pulkit, rdamazio, mercurial-devel
pulkit added inline comments. INLINE COMMENTS > narrowcommands.py:331 > ('', 'removeinclude', [], _('old paths to no longer include')), > + ('', 'auto-remove-includes', False, > + _('automatically choose unused includes to remove (EXPERIMENTAL)')), Following pattern of other flags, `auto-removeinclude(s)` seems better. > martinvonz wrote in narrowcommands.py:332 > Even though it's marked EXPERIMENTAL? Or maybe I should do that and just drop the EXPERIMENTAL since the whole feature (narrow clones) is experimental anyway? I don't have a strong opinion on having or dropping EXPERIMENTAL, but we should have some help text. It can be added under verbose section. REPOSITORY rHG Mercurial CHANGES SINCE LAST ACTION https://phab.mercurial-scm.org/D6848/new/ REVISION DETAIL https://phab.mercurial-scm.org/D6848 To: martinvonz, durin42, #hg-reviewers Cc: pulkit, rdamazio, mercurial-devel
martinvonz added inline comments. INLINE COMMENTS > pulkit wrote in narrowcommands.py:331 > Following pattern of other flags, `auto-removeinclude(s)` seems better. I was also wondering about that. Maybe those should be changed instead? I'm not sure, but I think we're slowly transitioning option names to use hyphens just like we do with config names? I don't care much, so let me know if you'd still prefer to drop the second hyphen. > pulkit wrote in narrowcommands.py:332 > I don't have a strong opinion on having or dropping EXPERIMENTAL, but we should have some help text. It can be added under verbose section. I decided to drop EXPERIMENTAL. REPOSITORY rHG Mercurial CHANGES SINCE LAST ACTION https://phab.mercurial-scm.org/D6848/new/ REVISION DETAIL https://phab.mercurial-scm.org/D6848 To: martinvonz, durin42, #hg-reviewers Cc: pulkit, rdamazio, mercurial-devel
pulkit added inline comments. INLINE COMMENTS > narrowcommands.py:369 > + If --auto-remove-includes is specified, then those includes that don't match > + any files modified by currently visible commits will be added to the set of > + explicitly specified includes to remove. sorry, but `currently visible commits` sounds like not correct as this can be used in case of non-ellipses repository also. REPOSITORY rHG Mercurial CHANGES SINCE LAST ACTION https://phab.mercurial-scm.org/D6848/new/ REVISION DETAIL https://phab.mercurial-scm.org/D6848 To: martinvonz, durin42, #hg-reviewers Cc: pulkit, rdamazio, mercurial-devel
martinvonz added inline comments. martinvonz marked an inline comment as done. INLINE COMMENTS > pulkit wrote in narrowcommands.py:369 > sorry, but `currently visible commits` sounds like not correct as this can be used in case of non-ellipses repository also. Good point. I've updated this a bit. See how this sounds. REPOSITORY rHG Mercurial CHANGES SINCE LAST ACTION https://phab.mercurial-scm.org/D6848/new/ REVISION DETAIL https://phab.mercurial-scm.org/D6848 To: martinvonz, durin42, #hg-reviewers Cc: pulkit, rdamazio, mercurial-devel
Patch
diff --git a/tests/test-narrow.t b/tests/test-narrow.t --- a/tests/test-narrow.t +++ b/tests/test-narrow.t @@ -447,3 +447,37 @@ abort: local changes found (use --force-delete-local-changes to ignore) [255] + $ cd .. + +Test --auto-remove-includes + $ hg clone --narrow ssh://user@dummy/master narrow-auto-remove -q \ + > --include d0 --include d1 --include d2 + $ cd narrow-auto-remove + $ echo a >> d0/f + $ hg ci -m 'local change to d0' + $ hg co '.^' + 1 files updated, 0 files merged, 0 files removed, 0 files unresolved + $ echo a >> d1/f + $ hg ci -m 'local change to d1' + created new head + $ hg debugobsolete $(hg log -T '{node}' -r 'desc("local change to d0")') + 1 new obsolescence markers + obsoleted 1 changesets + $ hg tracked --auto-remove-includes + comparing with ssh://user@dummy/master + searching for changes + looking for unused includes to remove + removing these unused includes: + path:d0 + path:d2 + looking for local changes to affected paths + saved backup bundle to $TESTTMP/narrow-auto-remove/.hg/strip-backup/*-narrow.hg (glob) + deleting data/d0/f.i + deleting data/d2/f.i + deleting meta/d0/00manifest.i (tree !) + deleting meta/d2/00manifest.i (tree !) + $ hg tracked --auto-remove-includes + comparing with ssh://user@dummy/master + searching for changes + looking for unused includes to remove + found no unused includes diff --git a/hgext/narrow/narrowcommands.py b/hgext/narrow/narrowcommands.py --- a/hgext/narrow/narrowcommands.py +++ b/hgext/narrow/narrowcommands.py @@ -328,6 +328,8 @@ @command('tracked', [('', 'addinclude', [], _('new paths to include')), ('', 'removeinclude', [], _('old paths to no longer include')), + ('', 'auto-remove-includes', False, + _('automatically choose unused includes to remove (EXPERIMENTAL)')), ('', 'addexclude', [], _('new paths to exclude')), ('', 'import-rules', '', _('import narrowspecs from a file')), ('', 'removeexclude', [], _('old paths to no longer exclude')), @@ -398,10 +400,12 @@ removedincludes = narrowspec.parsepatterns(opts['removeinclude']) addedexcludes = narrowspec.parsepatterns(opts['addexclude']) removedexcludes = narrowspec.parsepatterns(opts['removeexclude']) + autoremoveincludes = opts['auto_remove_includes'] update_working_copy = opts['update_working_copy'] only_show = not (addedincludes or removedincludes or addedexcludes or - removedexcludes or newrules or update_working_copy) + removedexcludes or newrules or autoremoveincludes or + update_working_copy) oldincludes, oldexcludes = repo.narrowpats @@ -436,7 +440,7 @@ narrowspec.copytoworkingcopy(repo) return 0 - if not widening and not narrowing: + if not (widening or narrowing or autoremoveincludes): ui.status(_("nothing to widen or narrow\n")) return 0 @@ -459,6 +463,27 @@ commoninc = discovery.findcommonincoming(repo, remote) + if autoremoveincludes: + outgoing = discovery.findcommonoutgoing(repo, remote, + commoninc=commoninc) + ui.status(_('looking for unused includes to remove\n')) + localfiles = set() + for n in itertools.chain(outgoing.missing, outgoing.excluded): + localfiles.update(repo[n].files()) + suggestedremovals = [] + for include in sorted(oldincludes): + match = narrowspec.match(repo.root, [include], oldexcludes) + if not any(match(f) for f in localfiles): + suggestedremovals.append(include) + if suggestedremovals: + ui.status(_('removing these unused includes:\n')) + for s in suggestedremovals: + ui.status('%s\n' % s) + removedincludes.update(suggestedremovals) + narrowing = True + else: + ui.status(_('found no unused includes\n')) + if narrowing: newincludes = oldincludes - removedincludes newexcludes = oldexcludes | addedexcludes