unofficial mirror of emacs-devel@gnu.org 
 help / color / mirror / code / Atom feed
From: Stefan Monnier <monnier@iro.umontreal.ca>
To: joakim@verona.se
Cc: Eli Zaretskii <eliz@gnu.org>, emacs-devel@gnu.org
Subject: Re: imagemagick branch
Date: Tue, 18 May 2010 12:35:34 -0400	[thread overview]
Message-ID: <jwvd3wtusil.fsf-monnier+emacs@gnu.org> (raw)
In-Reply-To: <m37hn17aql.fsf@verona.se> (joakim@verona.se's message of "Tue, 18 May 2010 12:41:38 +0200")

> Yes, the code could probably safely be added to trunk. Even if one
> activates imagemagick with "configure --with-imagemagick=yes"
> imagemagick wont kick in unless you execute (imagemagick-register-types)

I just took a quick look at the code and I see the following nits to fix:
- obviously a merge will have to come with a good ChangeLog.
- also the merge will need to come with documentation.  Maybe not in the
  Texinfo form yet, but at least in the etc/NEWS with enough info that
  describes the `scale' and other such arguments that someone can start
  using them.
- the README talks about naming inconsistencies, I think these should be
  fixed before a first commit (should be straightforward).
- the "let" in image.el should not be followed by a line break and the while
  should be replaced by a dolist.
- the prototype of imagemagick_load_image has some odd indentation in
  its args, not sure what happened.
- a few lines in the C code break the 80columns limit.
- please use ANSI style function declarations rather than K&R for new code.
- you can get rid of the prototypes by reordering the code.
- the docstrings in DEFUN should not be indented (they'll display
  weirdly otherwise in C-h f).
- Some "{" are at the end of a for/if rather than on their own line.
- why use "*( imtypes + i)" rather than "imtypes[i]"?
- some "," lack a space after them.
- several "=" and "==" lack spaces around them.

> Another question then, how do I merge best to trunk? I'm reading:
> http://www.emacswiki.org/emacs/BzrForEmacsDevs#MergeToUpstream
> but it suggests removing my local branch after merging.

The branch's history is not really interesting it seems, so you might as
well do the first commit on trunk as a separate commit rather as a merge.

But if you prefer a merge, see Eli's answer.

> I would like to continue to add some more experimental functions to
> the branch, and later merge these to trunk. Would that also be ok?
> Or would it be better to create a new branch for this purpose?

No need for a new branch.


        Stefan



  parent reply	other threads:[~2010-05-18 16:35 UTC|newest]

Thread overview: 8+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2010-05-18  8:22 imagemagick branch joakim
2010-05-18  9:11 ` Eli Zaretskii
2010-05-18 10:41   ` joakim
2010-05-18 11:42     ` Eli Zaretskii
2010-05-18 15:37       ` Chong Yidong
2010-05-18 17:39         ` joakim
2010-05-18 16:35     ` Stefan Monnier [this message]
  -- strict thread matches above, loose matches on Subject: below --
2010-06-15 19:00 joakim

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: https://www.gnu.org/software/emacs/

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=jwvd3wtusil.fsf-monnier+emacs@gnu.org \
    --to=monnier@iro.umontreal.ca \
    --cc=eliz@gnu.org \
    --cc=emacs-devel@gnu.org \
    --cc=joakim@verona.se \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://git.savannah.gnu.org/cgit/emacs.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).