Re: [Monotone-devel] the changelog editor branch is ready for review

[Thomas Keller]

Am 19.04.10 07:21, schrieb Derek Scherger:
> 2010/4/15 Derek Scherger <address@hidden>
>> 2010/4/14 Stéphane Gimenez <address@hidden>
>>
>>> First remark: mtn status ask for my password and it's rather
>>> inconvenient (I'm not using ssh agent). Same for mtn commit just after
>>> it's invocation and prior to the real commit.
>>
>> Hrm.. I hadn't noticed that but I'll take a look.
> 
> This should be fixed in c66c53da6285693772f76e3f7cfa3b99a34f8b04 but I'm not
> sure if the change I've made there is a good one or not.
> 
> Thomas and Tim,  can you please have a look at this revision and see if you
> have a better approach? I don't see any obvious way of getting the signing
> key without attempting to decrypt it.

I guess we can't do much better right now. We have lots of places where
static functions fiddle / evaluate global state and change other global
state / calculate some value and I'd consider this bad practice, but
hey, there is no way to fix that without rewriting lots of code, right?

In the meantime what I would really really want to get rid of are
boolean function arguments - so maybe you could start and make the code
a little easier to read by passing a more describable enum value...?


>>> About this, I find 'ChangeSet: xxxx' really confusing, one could
>>> think of 'xxxx' as an id for the changeset. The good old 'Changes against
>>> parent xxxx' looks better to me.
>>>
>>
> I tend to agree here and I've reverted it to use the "Changes against
> parent..." however I'm now wondering whether displaying the branch names
> associated with each parent in this section might be useful which makes me
> wonder again about using Parent: ... and Branch: headers here.

This might surely make sense, but where do we stop? If we start listing
cert values of *parents* we might start listing the parents of these
parents and their certs and... endless loop :)

No, I think the parent revision id is enough, really. Everything else
should be part of that revision's log output.

>>> Since you want to know about what users think... See below for what
>>> I'd like to see in my editor. I think it's pretty self explanatory
>>> and compatible with most of the ideas you came up with.
>>>
>>
>  Here's what I have now:
> 
> $ mtn commit
> ######################################################
> Enter a description of this change following the Changelog line.
> The values of Author, Date and Branch may be modified as required.
> Any other modifications will cause the commit to fail.
> 
> *** REMOVE THIS LINE TO CANCEL THE COMMIT ***
> ----------------------------------------------------------------------
> Revision: c66c53da6285693772f76e3f7cfa3b99a34f8b04
> Parent:   f016f1e3e91e181e1fee2320ad537d99ce236d7d
> Author:   address@hidden
> Date:     Sun Apr 18 10:59:06 PM 2010
> Branch:   net.venge.monotone.experiment.changelog-editor
> 
> Changelog:
> 
> avoid requiring key passphrase for status and before editing commit message
> 
>  * cmd_ws_commit.cc (status,commit): call get_user_key with new
> don't-cache-key
>    flag; pass key_store object into complete_key_identity so that keys that
>    don't exist in the database are still available
> 
>  * keys.{hh,cc} (get_user_key): add new "cache" boolean controlling whether
>    check_and_save_chosen_key is called to decrypt the private key
> 
>  * tests/wrong_options_override_workspace_options/__driver__.lua: revert
> changes
>    made on this branch that are no longer necessary; test now matches that
> on
>    net.venge.monotone
> 
> ----------------------------------------------------------------------
> Changes against parent f016f1e3e91e181e1fee2320ad537d99ce236d7d
> 
>   patched  cmd_ws_commit.cc
>   patched  keys.cc
>   patched  keys.hh
>   patched  tests/wrong_options_override_workspace_options/__driver__.lua
> ######################################################
> 
> Which includes a --- separator line after the Changelog section in the
> commit editor. I've left the Revision: and Parent: lines between the ---
> separator lines alone. I'm assuming that people won't have any reason to
> edit these as they won't have any sensible values to put in them so they
> aren't likely to touch them.

Ok, I got my separator lines - fair 'nuff...

> $ mtn status
> ----------------------------------------------------------------------
> Revision: aa124b3ff5c488a0aeba8754821d00a374c61440
> Parent:   c66c53da6285693772f76e3f7cfa3b99a34f8b04
> Author:   address@hidden
> Date:     Sun Apr 18 11:12:45 PM 2010
> Branch:   net.venge.monotone.experiment.changelog-editor.foo
> 
> ----------------------------------------------------------------------
> This revision will create a new branch.
> Old Branch: net.venge.monotone.experiment.changelog-editor
> New Branch: net.venge.monotone.experiment.changelog-editor.foo
> 
> Changes against parent c66c53da6285693772f76e3f7cfa3b99a34f8b04
> 
> no changes

Now this is something which I'd slightly change - I'd love to see this
information in the editable area and the hint that the branch changes
above the editable area - because its important and might otherwise get
easily forgotten, so something like:

$ mtn status
ATTENTION: This revision will create a new branch.
----------------------------------------------------------------------
Revision:   aa124b3ff5c488a0aeba8754821d00a374c61440
Parent:     c66c53da6285693772f76e3f7cfa3b99a34f8b04
Author:     address@hidden
Date:       Sun Apr 18 11:12:45 PM 2010
Old branch: net.venge.monotone.experiment.changelog-editor
Branch:     net.venge.monotone.experiment.changelog-editor.foo

----------------------------------------------------------------------
Changes against parent c66c53da6285693772f76e3f7cfa3b99a34f8b04

no changes

and for commit the header could look like so

$ mtn commit
Enter a description of this change following the Changelog line.
The values of Author, Date and Branch may be modified as required.
Any other modifications will cause the commit to fail.

*** REMOVE THIS LINE TO CANCEL THE COMMIT ***

ATTENTION: This revision will create a new branch.
----------------------------------------------------------------------

>>> Also, it would be nice if the editor was invoked with option '+13' to
>>> position the cursor just after the ChangeLog line. It seems to work
>>> with vim, emacs and nano.
>>
>>
> You can set EDITOR='emacsclient +15' to get what you want but this is
> probably not what you want EDITOR set to in general. We'd need to do some
> more work in the edit_comment hook to get this right. This would be nice to
> do but I don't think it's critical for getting this branch finished.

Instead of giving the editor a "jump point" (which could be error-prone
f.e. if you think about i18n) I'd try to decrease the verbosity of the
entry line mainly by removing the warning in line three:

$ mtn commit
Enter a description of this change after the Changelog line.
Author, Date and Branch may be modified as required.

*** REMOVE THIS LINE TO CANCEL THE COMMIT ***
----------------------------------------------------------------------

And of course I'd remove Revision: and Parent: from the editable area to
make it even less verbose, but we've had this discussion before and as I
said I'm happy already that my separators made it in ;)

> I'm not in any particular hurry to merge this but I think it's pretty much
> ready. Are there any fundamental objections at this point? Do we want to
> have it in for 0.48?

I think we definitely want to have it for 0.48. And I'm also voting for
not discussing this useful feature to death (because then it won't get
included at all) - so I'm shutting up now :)

Thomas.

-- 
GPG-Key 0x160D1092 | address@hidden | http://thomaskeller.biz
Please note that according to the EU law on data retention, information
on every electronic information exchange might be retained for a period
of six months or longer: http://www.vorratsdatenspeicherung.de/?lang=en

signature.asc
Description: OpenPGP digital signature