This is an archive of the discontinued Mercurial Phabricator instance.

sparse: add a --cwd-list option
ClosedPublic

Authored by shravyak on Sep 18 2017, 10:17 PM.
Tags
None
Subscribers

Details

Reviewers
durham
akushner
mitrandir
Group Reviewers
Restricted Project
Commits
rFBHGX70f4fed0e5dd: sparse: add a --cwd-list option
Summary

This diff adds an option --cwd-list to hg sparse. This will return the
contents of the current directory. The files that are in the sparse profile are
annotated with a '-' indicator.

Test Plan

Tested by running the command 'hg sparse --cwd-list' in various folders.

Diff Detail

Repository
rFBHGX Facebook Mercurial Extensions
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

shravyak created this revision.Sep 18 2017, 10:17 PM
Herald added a reviewer: Restricted Project. · View Herald TranscriptSep 18 2017, 10:17 PM
shravyak updated this revision to Diff 1891.Sep 18 2017, 10:22 PM
shravyak edited the test plan for this revision. (Show Details)Sep 18 2017, 10:24 PM
shravyak added a reviewer: durham.
shravyak edited the test plan for this revision. (Show Details)Sep 18 2017, 11:02 PM
akushner requested changes to this revision.Sep 19 2017, 12:49 AM
akushner added a subscriber: akushner.
akushner added inline comments.
hgext3rd/sparse.py
710–711

How are the files in the profile indicated compared to the ones that aren't in the profile? This isn't clear to someone reading how the output is annotated.

1062

Comments should tell the "why" not "what". The code tells that you are getting the "root".

The "why" is used to explain something that might not be obvious to the reader or why a specific decision was made.

1075–1076

These comments aren't needed.

1077

if len(cwd)+1 is a constant, generally best left outside a loop (although in a short list of files, probably not noticeable.

1078

Will this work on WIndows?

1086

perhaps

for folder in sorted(all_folders):

When you type 'ls', notice how the default is alphabetic sorting? If someone is going to compare the output of this command with what's in the directory, it should be in the same sorted order.

I can even see a case where all the '-' annotated ones are at the top or bottom, but interspersed with no ordering makes this tool hard to use.

1087

This is shorter and keeps alignment

marker = ' ' if folder in checkedout_folders else '-'
repo.ui.status("%s %s\n" % (marker, folder)
This revision now requires changes to proceed.Sep 19 2017, 12:49 AM

In the third screenshot it looks like the first character is cut off every entry?

durham requested changes to this revision.Sep 19 2017, 12:23 PM
durham added inline comments.
hgext3rd/sparse.py
667

I think --cwd-list might be a better name for the flag. find implies searching for something, which isn't quite what this is doing.

1054

Mercurial variable naming avoids mid-variable underscores (they differ from python on this), so I'd rename this _cwdfind (or _cwdlist).

1060

Since files is only ever iterated over, and since mf will never have duplicates, you don't need to create the set() here. Just iterate of 'mf' instead (which is what set() is doing here to create the set).

1061

I think we need to call cwd = util.normpath(cwd) on this to normalize it. That will turn windows paths into unix looking paths, to address Aaron's question about windows below.

1068

I think there needs to be a space after 'begin'. I'd also lowercase the initial 'the', since that's mercurial style.

1072

I'd drop the underscores, for style consistency.

1077

I think this +1 is the reason the first character is missing when listing the root directory. So len(cwd) is constant, but the +1 might not be.

1088

I'd definitely have two spaces before the normal directories. So they align nicely with the '- ' directories in the output. Aaron's proposal does that as well

shravyak marked 10 inline comments as done.Sep 19 2017, 6:35 PM
shravyak updated this revision to Diff 1902.
shravyak added inline comments.Sep 19 2017, 6:36 PM
hgext3rd/sparse.py
1061

Makes sense, thanks!

1062

Modified the comment.

1077

Right. Changed it accordingly.

shravyak edited the test plan for this revision. (Show Details)Sep 19 2017, 6:36 PM
pulkit added a subscriber: pulkit.Sep 19 2017, 6:56 PM

Looks like you forgot to update the commit message.

shravyak edited the summary of this revision. (Show Details)Sep 19 2017, 7:42 PM
shravyak edited the test plan for this revision. (Show Details)Sep 20 2017, 1:25 PM
shravyak retitled this revision from [hgparse] add a --cwd-find option to hg parse to [hgparse] add a --cwd-list option to hg parse.Sep 20 2017, 1:58 PM
shravyak edited the test plan for this revision. (Show Details)Sep 20 2017, 2:04 PM
akushner added inline comments.Sep 20 2017, 11:44 PM
hgext3rd/sparse.py
667

Really should be something like:

annotate files that aren't in the sparse profile
1055–1056

Actually, the files that aren't in the sparse profile get annotated.

1078

looks like this should be removed.

mitrandir resigned from this revision.Sep 21 2017, 8:27 AM

It looks pretty good. The last thing would be to add a test. There are already some sparse tests in the tests directory, so it should be easy to add some new lines there. Let me know if you'll be busy on other tasks, and I can chip in tests to make sure this gets landed.

hgext3rd/sparse.py
1059

The appropriate name for this is still mf, since it is a manifest instance.

durham requested changes to this revision.Sep 21 2017, 2:42 PM

Requesting changes for the test

This revision now requires changes to proceed.Sep 21 2017, 2:42 PM
shravyak edited the summary of this revision. (Show Details)Sep 21 2017, 7:43 PM
shravyak edited the test plan for this revision. (Show Details)
shravyak retitled this revision from [hgparse] add a --cwd-list option to hg parse to [hgparse] add a --cwd-find option to hg parse.
shravyak marked 4 inline comments as done.Sep 21 2017, 7:43 PM
durham retitled this revision from [hgparse] add a --cwd-find option to hg parse to hgsparse: add a --cwd-find option to hg parse.Sep 27 2017, 4:40 AM
durham edited the test plan for this revision. (Show Details)
ryanmce retitled this revision from hgsparse: add a --cwd-find option to hg parse to sparse: add a --cwd-find option.Sep 27 2017, 4:51 AM
durham edited the summary of this revision. (Show Details)Sep 27 2017, 12:43 PM
durham edited the test plan for this revision. (Show Details)
durham retitled this revision from sparse: add a --cwd-find option to sparse: add a --cwd-list option.
durham updated this revision to Diff 2127.
durham accepted this revision.Sep 27 2017, 12:44 PM

Updated with a test

Successfully pushed the code to the repo - https://phabricator.intern.facebook.com/P58345906. Closing the task.

This revision was automatically updated to reflect the committed changes.