Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

cli: add trees command #599

Closed
wants to merge 1 commit into from
Closed

cli: add trees command #599

wants to merge 1 commit into from

Conversation

edvbld
Copy link
Member

@edvbld edvbld commented Apr 24, 2020

Hi all,

please review this patch that adds
trees functionality to Skara.
This is mainly for those who want to use Skara for working with Mercurial today
and are missing the trees functionality, but I also added a couple of trees
"commands" like tstatus. tpull etc. to skara.gitconfig.

Testing:

  • Manual testing of both Mercurial and Git trees commands

Thanks,
Erik


Progress

  • Change must not contain extraneous whitespace
  • Change must be properly reviewed

Reviewers

Download

$ git fetch https://git.openjdk.java.net/skara pull/599/head:pull/599
$ git checkout pull/599

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Apr 24, 2020

👋 Welcome back ehelin! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request.

@mlbridge
Copy link

@mlbridge mlbridge bot commented Apr 24, 2020

Webrevs

Copy link
Member

@rwestberg rwestberg left a comment

Good stuff!

@openjdk
Copy link

@openjdk openjdk bot commented Apr 24, 2020

@edvbld This change now passes all automated pre-integration checks. When the change also fulfills all project specific requirements, type /integrate in a new comment to proceed. After integration, the commit message will be:

cli: add trees command

Reviewed-by: rwestberg
  • If you would like to add a summary, use the /summary command.
  • To credit additional contributors, use the /contributor command.
  • To add additional solved issues, use the /solves command.

Since the source branch of this PR was last updated there have been 3 commits pushed to the master branch. As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid automatic rebasing, please merge master into your branch, and then specify the current head hash when integrating, like this: /integrate bd3b96dae833919b8af63dea3966d7151520e974.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

@openjdk openjdk bot added the ready label Apr 24, 2020
@edvbld
Copy link
Member Author

@edvbld edvbld commented Apr 24, 2020

/integrate

@openjdk openjdk bot closed this Apr 24, 2020
@openjdk openjdk bot added integrated and removed ready labels Apr 24, 2020
@openjdk
Copy link

@openjdk openjdk bot commented Apr 24, 2020

@edvbld The following commits have been pushed to master since your change was applied:

  • bd3b96d: 378: Create merge conflict webrevs for "merge style" PRs without merge commit
  • 4984c98: 377: Remove special handling of failed-auto-merge label
  • 341fcfe: skara: add hint about help for commands

Your commit was automatically rebased without conflicts.

Pushed as commit 8d9b6e9.

@openjdk openjdk bot removed the rfr label Apr 24, 2020
@mlbridge
Copy link

@mlbridge mlbridge bot commented Apr 24, 2020

Mailing list message from Erik Helin on skara-dev:

Changeset: 8d9b6e9
Author: Erik Helin <ehelin at openjdk.org>
Date: 2020-04-24 11:26:24 +0000
URL: https://git.openjdk.java.net/skara/commit/8d9b6e98

cli: add trees command

Reviewed-by: rwestberg

! cli/build.gradle
! cli/src/main/java/org/openjdk/skara/cli/GitSkara.java
+ cli/src/main/java/org/openjdk/skara/cli/GitTrees.java
! skara.gitconfig
! skara.py

@mlbridge
Copy link

@mlbridge mlbridge bot commented Apr 24, 2020

Mailing list message from Magnus Ihse Bursie on skara-dev:

Interesting idea. I think you missed a proper implementation of tconfig,
though, if you want backwards compatibility:

magnusi at sthihse:/localhome/hg/jdk-ALT$ hg tconfig --help
hg tconfig [OPTION]... [SUBTREE]...

list or change the subtrees configuration

??? One of five operations can be selected:

??? --list :??? list the configured subtrees; this is the default if no
other
??????????????? operation is selected.
??? --add :???? add the specified subtrees to the configuration.
??? --del :???? delete the specified subtrees from the configuration. Use
??????????????? --del --all to delete all subtrees.
??? --set :???? set the subtree configuration to the specified
subtrees. Use
??????????????? --set --walk to walk the filesystem rooted at REPO and set
??????????????? the subtree configuration to the discovered repos. Use
??????????????? --depth to write the subtree configuration depth-most, so
??????????????? that each subtree is defined within the nearest enclosing
??????????????? repository.? Note that --walk and --depth may be used
??????????????? together.
??? --expand :? list the value of config items from the [trees]
section. Items
??????????????? in the [trees] section can be defined in terms of other
items
??????????????? in the [trees] section; tconfig --expand shows the
??????????????? recursively expanded value.? It returns 0 if at least one
??????????????? config item was found; otherwise it returns 1.

??? Note that with the slight exception of --set --depth, this command does
??? not recurse into subtrees; it operates only on the current repository.
??? (To recursively list subtrees, use the tlist command.)

options:

?-a --add?????????? add the specified SUBTREEs to config
??? --all?????????? with --del, delete all subtrees from config
?-d --del?????????? delete the specified SUBTREEs from config
?-e --expand??????? recursively expand config items in the [trees] section
?-l --list????????? list the configured subtrees
??? --depth???????? store subtree configuration depth-most
?-s --set?????????? set the subtree config to SUBTREEs
??? --tns NAMESPACE trees namespace to use
?-w --walk????????? walk the filesystem to discover subtrees
??? --mq??????????? operate on patch repository

(some details hidden, use --verbose to show complete help)

Your code seems to do automatic configuration, which is probably an
improvment :-) (but not backwards compatibility).

It's also unclear what the "tconfig = tree config" alias means.

It's not at all clear that we want or need backwards compatibility here.
But if not, then the tconfig command should probably be removed.

/Magnus

On 2020-04-24 11:27, Erik Helin wrote:

Hi all,

please review this patch that adds
[trees](https://hg.openjdk.java.net/code-tools/trees) functionality to Skara.
This is mainly for those who want to use Skara for working with Mercurial today
and are missing the trees functionality, but I also added a couple of trees
"commands" like `tstatus`. `tpull` etc. to `skara.gitconfig`.

Testing:
- Manual testing of both Mercurial and Git trees commands

Thanks,
Erik

-------------

Commit messages:
- cli: add trees command

Changes: https://git.openjdk.java.net/skara/pull/599/files
Webrev: https://webrevs.openjdk.java.net/skara/599/webrev.00
Stats: 222 lines in 5 files changed: 210 ins; 0 del; 12 mod
Patch: https://git.openjdk.java.net/skara/pull/599.diff
Fetch: git fetch https://git.openjdk.java.net/skara pull/599/head:pull/599

PR: https://git.openjdk.java.net/skara/pull/599

@mlbridge
Copy link

@mlbridge mlbridge bot commented Apr 24, 2020

Mailing list message from Erik Helin on skara-dev:

On 4/24/20 2:41 PM, Magnus Ihse Bursie wrote:

Interesting idea. I think you missed a proper implementation of tconfig,
though, if you want backwards compatibility:

Your code seems to do automatic configuration, which is probably an
improvment :-) (but not backwards compatibility).

Yes, I should have mentioned this in the RFR email. To me there are two
problems with original trees.py "tconfig" command:

1. It removes the ability to run the Mercurial command "config" for a
set of trees, since the name "tconfig" is already taken by trees.py
own "tconfig" command.
2. I don't see any reason why trees.py forces the user to run tconfig
prior to running any other trees command (such as "tstatus"). If a
user runs "tstatus" then it is obvious that they want to run the
command "status" for all trees, so we might as well find all the
trees by then if we haven't done it before.

It's also unclear what the "tconfig = tree config" alias means.

It means that `tconfig` is an alias that runs the command `trees config`
:) "config" is a command in Git, just as it is in Mercurial, so
"tconfig" here simply means "run the command 'config' in all trees".

It's not at all clear that we want or need backwards compatibility here.
But if not, then the tconfig command should probably be removed.

For the reasons listed above I think we should break backwards
compatibility here, particularly since the workaround is very easy: you
no longer have to run tconfig --set --walk :)

Or did I miss something?

Thanks,
Erik

/Magnus

On 2020-04-24 11:27, Erik Helin wrote:

Hi all,

please review this patch that adds
[trees](https://hg.openjdk.java.net/code-tools/trees) functionality to
Skara.
This is mainly for those who want to use Skara for working with
Mercurial today
and are missing the trees functionality, but I also added a couple of
trees
"commands" like `tstatus`. `tpull` etc. to `skara.gitconfig`.

Testing:
- Manual testing of both Mercurial and Git trees commands

Thanks,
Erik

-------------

Commit messages:
? - cli: add trees command

Changes: https://git.openjdk.java.net/skara/pull/599/files
? Webrev: https://webrevs.openjdk.java.net/skara/599/webrev.00
?? Stats: 222 lines in 5 files changed: 210 ins; 0 del; 12 mod
?? Patch: https://git.openjdk.java.net/skara/pull/599.diff
?? Fetch: git fetch https://git.openjdk.java.net/skara
pull/599/head:pull/599

PR: https://git.openjdk.java.net/skara/pull/599

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
2 participants