Patch submitting procedures

[Jeroen Demeyer]

This is a reminder on how to properly submit a patch to Trac.
Recently, I find a lot of small administrative issues. Please keep in
mind the following:

1) When submitting a patch, make sure there is reasonable *commit
message* (use hg qrefresh -e to set the message). The message is
allowed to span multiple lines, but the first line should stand by
itself (this will be displayed by hg log). Also the correct(!) ticket
number should appear on the first line of the commit message. Long
lines should be wrapped.

2) Patches should be made using *hg export tip*, and not hg diff.

3) When authoring or reviewing a ticket, add your *real name* as
Author/Reviewer. Also add your name to
http://trac.sagemath.org/sage_trac/#AccountNamesmappedtoRealNames

4) Write instructions for merging the patches (which patches to apply,
which dependencies there are, URLs of spkgs) in the *ticket
description*, not only in a comment.

Thanks,
Jeroen.

[slabbe]

> 1) When submitting a patch, make sure there is reasonable *commit
> message* (use hg qrefresh -e to set the message).

or

hg qrefresh -m "#XXXX: Commit message"

to do it from the command line. I think if one does

hg qrefresh -e "#XXXX: Commit message"

it erases the patch and replaces it by the string "#XXXX: Commit
message".... so beware.

Sébastien

[Jason Grout]

On 3/28/11 4:36 AM, Jeroen Demeyer wrote:
> Also the correct(!) ticket
> number should appear on the first line of the commit message.

Does the patch merging script not automatically do this? I thought at
one time somebody added this. Again, the patch merging script already
knows this, and if the script did it, the number would be uniformly
formatted.

Thanks,

Jason

[Jeroen Demeyer]

On 2011-03-29 13:15, Jason Grout wrote:
> On 3/28/11 4:36 AM, Jeroen Demeyer wrote:
>> Also the correct(!) ticket
>> number should appear on the first line of the commit message.
>
> Does the patch merging script not automatically do this?

This is not the case. It would be possible to do this automatically.
But then it's not clear how to handle the case where the commit message
already contains the ticket number. Even more difficult is the case
when the author adds a *wrong* ticket number to the commit message.
Currently, I can easily check this. Because of this, I choose not to
automatically add a ticket number to a commit message.

Jeroen.

[Jason Grout]

If the script automatically prepended:

Trac #xxxx:

to the start of every commit message, then:

1. the ticket number text would be uniform

2. if a wrong ticket number was indicated, it would be obvious which was
the correct ticket number, as it's always first in the standard format

3. people would eventually get in the habit of not adding the ticket
number, and we wouldn't have to deal with inconsistent formatting of the
number, wrong ticket numbers, forgetting to add ticket numbers, etc.

Thanks,

Jason

[David Roe]

If the script automatically prepended:

Trac #xxxx:

to the start of every commit message, then:

1. the ticket number text would be uniform

2. if a wrong ticket number was indicated, it would be obvious which was the correct ticket number, as it's always first in the standard format

3. people would eventually get in the habit of not adding the ticket number, and we wouldn't have to deal with inconsistent formatting of the number, wrong ticket numbers, forgetting to add ticket numbers, etc.

+1
David

[Ivan Andrus]

On Mar 28, 2011, at 11:36 AM, Jeroen Demeyer wrote:

> 2) Patches should be made using *hg export tip*, and not hg diff.

Don't forget that you need to use --git if you have touched any binary files. It may be best to add this to your .hgrc (I think this was mentioned in another thread recently).

-Ivan

[Keshav Kini]

Indeed. Mercurial's workflow is not really supposed to be carried out
by sending patches to people (the encouraged behavior is to use `hg
pull` from other people's repositories), so the default patch / export
format only includes a subset of the total information so that it can
be backwards compatible with ancient diff formats. Adding "[diff]\ngit
= true" to your ~/.hgrc is really necessary in order to make your
patches accurately reflect your changes (if using mq) or commits (if
not). Besides binary files, this also exports information about file
permission changes, for example.

-Keshav

[William Stein]

On Sat, Apr 2, 2011 at 5:25 PM, Keshav Kini <kesha...@gmail.com> wrote:
> Indeed. Mercurial's workflow is not really supposed to be carried out
> by sending patches to people (the encouraged behavior is to use `hg
> pull` from other people's repositories), so the default patch / export
> format only includes a subset of the total information so that it can
> be backwards compatible with ancient diff formats. Adding "[diff]\ngit
> = true" to your ~/.hgrc is really necessary in order to make your
> patches accurately reflect your changes (if using mq) or commits (if
> not). Besides binary files, this also exports information about file
> permission changes, for example.

Could we change the default to "git = true" as a patch to the version
of hg we distribute with Sage? Or, alternatively, have a patch that
prints a warning
whenever Sage's hg is used but git = false? The second option would
be safer, since a user might accidentally switch between using the
Sage hg and
a system-wide one...

>
> -Keshav
>
> On Mar 31, 4:00 pm, Ivan Andrus <darthand...@gmail.com> wrote:
>> On Mar 28, 2011, at 11:36 AM, Jeroen Demeyer wrote:
>>
>> > 2) Patches should be made using *hg export tip*, and not hg diff.
>>
>> Don't forget that you need to use --git if you have touched any binary files.  It may be best to add this to your .hgrc (I think this was mentioned in another thread recently).
>>
>> -Ivan
>

> --
> To post to this group, send an email to sage-...@googlegroups.com
> To unsubscribe from this group, send an email to sage-devel+...@googlegroups.com
> For more options, visit this group at http://groups.google.com/group/sage-devel
> URL: http://www.sagemath.org
>

--
William Stein
Professor of Mathematics
University of Washington
http://wstein.org

[Keshav Kini]

On Apr 4, 3:23 am, William Stein <wst...@gmail.com> wrote:
> Could we change the default to "git = true" as a patch to the version
> of hg we distribute with Sage?   Or, alternatively, have a patch that
> prints a warning
> whenever Sage's hg is used but git = false?   The second option would
> be safer, since a user might accidentally switch between using the
> Sage hg and
> a system-wide one...

`sage -hg` is only different from `hg` in that the particular `hg`
executable run is determined by the new $PATH set by `. sage-env`.
Unfortunately mercurial reads its config files mostly from fixed
locations (whether relative to the repository or absolute) and not
from environment variables except $HOME which it uses to find ~/.hgrc,
and `. sage-env` doesn't modify $HOME.

Personally I think the best solution is to create $SAGE_ROOT/local/etc/
mercurial/hgrc and put "[diff]\ngit=true" in it. This file will be
read by `sage -hg` but not the system `hg` due to the location of our
"hg" executable file, and though it will be overruled by "[diff]
\ngit=false" in either your ~/.hgrc or your /etc/mercurial/hgrc,
nobody will be putting that in those files since false is the default
value anyway. In any case, this is only a problem for developers who
can be reminded to remove or change the git diff setting in their
~/.hgrc or /etc/mercurial/hgrc if necessary, though I doubt this will
happen. So I don't think we need to go so far as to actually patch the
mercurial source code.

I just tested the above and it works. It can be implemented by just
putting the following in the spkg-install for mercurial:

mkdir -p $SAGE_LOCAL/etc/mercurial/
echo """[diff]
git=true""" > $SAGE_LOCAL/etc/mercurial/hgrc

-Keshav

[William Stein]

Great idea! Post a ticket.

[Keshav Kini]

On Apr 4, 1:13 pm, William Stein <wst...@gmail.com> wrote:
> Great idea!  Post a ticket.

This is now #11121 and awaits review :)

-Keshav

[Jeroen Demeyer]

On 2011-03-29 14:26, Jason Grout wrote:
> If the script automatically prepended:
>
> Trac #xxxx:
>
> to the start of every commit message, then:

Done for sage-4.7.alpha5

Of course you get lots of silly commit messages like

Trac #11141: #11141: add PolyGUI and cygdb to SAGE_LOCAL/bin/.hgignore

[Jason Grout]

Of course, temporarily the script could see if there was a ticket number
already starting the commit message, maybe a regular expression
something like (note: I haven't tested the following; it's likely that
I'm misremembering regexp syntax):

^([Tt]rac)?\w* (#)?\w*xxxx(:)?

If that is found, the script could replace it with the automated trac
number.

Thanks,

Jason

[Jeroen Demeyer]

On 2011-04-11 16:03, Jason Grout wrote:
> Of course, temporarily the script could see if there was a ticket number
> already starting the commit message, maybe a regular expression
> something like (note: I haven't tested the following; it's likely that
> I'm misremembering regexp syntax):
>
> ^([Tt]rac)?\w* (#)?\w*xxxx(:)?

If you're able to come up with a good (Perl-style) regular expression
which does The Right Thing, then I'm happy to use it. Unfortunately,
there is currently no standard way to specify ticket numbers in the
commit message, so it's not so clear what the regular expression should do.

Jeroen.