( )⚙ D6848 narrow: add option for automatically removing unused includes

This is an archive of the discontinued Mercurial Phabricator instance.

narrow: add option for automatically removing unused includes
ClosedPublic

Authored by martinvonz on Sep 13 2019, 1:42 AM.

Details

Summary

It's been a somewhat common request among our users to have Mercurial
automatically pick includes to remove. This patch adds an option for
that: hg tracked --auto-remove-includes. I'm not sure if this is the
right name and semantics for it. Perhaps the feature should also add
excludes of large subdirectories even if other files in the include
are needed? Narrow clones are experimental, so we can change the name
and/or semantics later if necessary.

Diff Detail

Repository
rHG Mercurial
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

martinvonz created this revision.Sep 13 2019, 1:42 AM
rdamazio added inline comments.
tests/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.

martinvonz added inline comments.Sep 13 2019, 5:16 PM
tests/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...

rdamazio added inline comments.Sep 13 2019, 8:40 PM
tests/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.

martinvonz added inline comments.Sep 13 2019, 8:54 PM
tests/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?

martinvonz updated this revision to Diff 16532.Sep 13 2019, 8:54 PM
martinvonz added inline comments.Sep 14 2019, 1:45 AM
tests/test-narrow.t
457

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.

pulkit added a subscriber: pulkit.Sep 17 2019, 1:44 PM
pulkit added inline comments.
hgext/narrow/narrowcommands.py
332

Can you add some help text below on how these unused includes are calculated?

martinvonz added inline comments.Sep 17 2019, 1:45 PM
hgext/narrow/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?

pulkit added inline comments.Sep 17 2019, 1:50 PM
hgext/narrow/narrowcommands.py
331

Following pattern of other flags, auto-removeinclude(s) seems better.

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.

martinvonz marked 2 inline comments as done.Sep 17 2019, 1:58 PM
martinvonz edited the summary of this revision. (Show Details)
martinvonz updated this revision to Diff 16563.
martinvonz added inline comments.Sep 17 2019, 1:58 PM
hgext/narrow/narrowcommands.py
331

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.

332

I decided to drop EXPERIMENTAL.

pulkit added inline comments.Sep 17 2019, 2:14 PM
hgext/narrow/narrowcommands.py
369

sorry, but currently visible commits sounds like not correct as this can be used in case of non-ellipses repository also.

martinvonz updated this revision to Diff 16564.Sep 17 2019, 2:18 PM
martinvonz marked an inline comment as done.Sep 17 2019, 2:19 PM
martinvonz added inline comments.
hgext/narrow/narrowcommands.py
369

Good point. I've updated this a bit. See how this sounds.

pulkit accepted this revision.Sep 17 2019, 3:33 PM
This revision is now accepted and ready to land.Sep 17 2019, 3:33 PM
martinvonz marked an inline comment as done.Sep 17 2019, 3:57 PM
This revision was automatically updated to reflect the committed changes.