D4312: New bookflow extension for bookmark-based branching

[pulkit (Pulkit Goyal)]

pulkit added a comment.


  I have looked at the things more thoroughly and commented things inline. Certain things can be treated as opinions and it's perfectly fine to now fix them.
  
  I found following cases missing in tests:
  
  - Testing of creation of bookmark name which already exists
  - Testing active bookmark is not moved on update (I might be wrong)
  
  Also, mercurial core tests mainly test-check-code.t and test-check-commit.t fails with this patch. Please make them happy.
  
  Also, the functionality this extension implemented is somewhat similar to https://www.mercurial-scm.org/wiki/BookmarkUpdatePlan.

INLINE COMMENTS

> bookflow.py:2
> +"""implements bookmark-based branching
> +
> + - Disables creation of new branches (config: enable_branches=False).

Can we add some help text here that this extension helps implement feature branches workflow using bookamrks by providing config knobs to change the default behavior.

> bookflow.py:6
> + - Doesn't move the active bookmark on update, only on commit.
> + - Requires '--rev' for moving an existing bookmark.
> + - Protects special bookmarks (config: protect=@).

Since the above two are enabled by default when we enabled this extension, they deserve a separate paragraph before the config options.

> bookflow.py:14
> +    :hg up|co MARK: switch to bookmark
> +    :hg push -B .: push active bookmark
> +"""

Can we switch the way help is described here. From 'cmdname: help' to

`To create a new bookmark: hg book mark`.

> bookflow.py:41
> +        if active in ui.configlist(MY_NAME, 'protect'):
> +            raise error.Abort(_('can\'t commit, bookmark {} is protected').format(active))
> +        if not cwd_at_bookmark(repo, active):

We generally use `cannot` instead of `can't` in core messages.

> bookflow.py:43
> +        if not cwd_at_bookmark(repo, active):
> +            raise error.Abort(_('can\'t commit, working directory out of sync with active bookmark.\nRun: hg up {}').format(active))
> +    elif ui.configbool(MY_NAME, 'require_bookmark', True):

The `Run: hg up {}` part should be in the help text. Also `run 'hg up {}'` matches the core style more.

> bookflow.py:72
> +    if active and not cwd_at_bookmark(repo, active):
> +        ui.warn(_("working directory out of sync with active bookmark.\nRun: hg up {}\n").format(active))
> +    return rc

We can better say that "the active bookmark is updated to <commit>. use hg update to update to it."

> bookflow.py:82
> +    if label and not opts.get(r'clean') and not opts.get(r'rev'):
> +        raise error.Abort(_("branching should be done using bookmarks:\nhg bookmark {}").format(label))
> +    return orig(ui, repo, label, **opts)

This can be improved to "creating named branches is disabled and you should use bookmarks. see `hg help bookmarks`."

Also maybe since we are defining a workflow using this extension, documenting that workflow will be great!

> bookflow.py:87
> +    extensions.wrapfunction(bookmarks, 'update', bookmarks_update)
> +    extensions.wrapfunction(bookmarks, 'addbookmarks', bookmarks_addbookmarks)
> +    # commit hook conflicts with shelving

We should move this couple of wrapfunctions call to uisetup() too.

> bookflow.py:89
> +    # commit hook conflicts with shelving
> +    # ui.setconfig('hooks', 'pretxncommit.' + MY_NAME, commit_hook, source=MY_NAME)
> +

If I understand correctly, the above couple of lines can be deleted?

> test-bookflow.t:2
> +initialize
> +  $ make_changes() { d=`pwd`; [ ! -z $1 ] && cd $1; echo "test $(basename `pwd`)" >> test; hg commit -Am"${2:-test}"; r=$?; cd $d; return $r; }
> +  $ assert_clean() { ls -1 $1 | grep -v "test$" | cat;}

I am not good at bash I am certain you make it simpler by removing the logic related to pwd, I am not sure why do we need that.

> test-bookflow.t:3
> +  $ make_changes() { d=`pwd`; [ ! -z $1 ] && cd $1; echo "test $(basename `pwd`)" >> test; hg commit -Am"${2:-test}"; r=$?; cd $d; return $r; }
> +  $ assert_clean() { ls -1 $1 | grep -v "test$" | cat;}
> +  $ ls -1a

sorry, but is it like `hg status`?

> test-bookflow.t:29
> +  $ make_changes
> +  $ hg push ../a > /dev/null
> +

output of push to /dev/null, that seems a bit weird, let's not do that.

> test-bookflow.t:110
> +# pull, this should move the bookmark forward, because it was changed remotely
> +  $ hg pull -u | grep "updating to active bookmark X"
> +  updating to active bookmark X

I suggest printing the complete output of `hg pull` and remove the grep.

> test-bookflow.t:123
> +  $ assert_clean ../a
> +  $ assert_clean ../b
> +  $ hg --cwd ../a bookmarks

Opinion: these assert_clean and make_changes are not clean ways. Maybe the global -R flag can be used to commit without changing directories.

> test-bookflow.t:255
> +  $ make_changes ../a
> +  $ hg --cwd ../a book | grep X
> +   \* X                         \d+:f73a71c992b8 (re)

nit: -R flag can be used.

> test-bookflow.t:258
> +  $ cd ../b
> +  $ hg pull  2>&1 | grep -v add|grep -v pulling|grep -v searching|grep -v changeset
> +  updating bookmark X

Can we just show the whole output. We are completely fine with more redundant lines.

> test-bookflow.t:271
> +  [255]
> +  $ hg up -C -r . > /dev/null # cleanup local changes
> +  $ assert_clean

Same, let's not push output to /dev/null.

> test-bookflow.t:275
> +  36a6e592ec06
> +  $ hg up X > /dev/null
> +  $ hg id -i # now we're on X

Same, let's not push output to /dev/null.

REVISION DETAIL
  https://phab.mercurial-scm.org/D4312

To: idlsoft, #hg-reviewers
Cc: pulkit, mercurial-devel