[MERGE] Add a way to specify a template commit message

John Arbash Meinel john at arbash-meinel.com
Mon Feb 5 18:29:16 GMT 2007


James Westby wrote:
> On (04/02/07 12:38), James Westby wrote:
>> On (04/02/07 07:29), Robert Collins wrote:
>>> Robert Collins has voted +1 (conditional).
>>> Status is now: Semi-approved
>>> Comment:
>>> The check for 'start_message != ""' seems wrong to me - an empty start 
>>> message is still a start message.
>> I was just following the lead of the template message code. That
>> excludes an empty template message. I can see that that might be
>> different though, you can save on the separator adding removing if it is
>> empty. I'll make the change as you add.
>>
>>> Just checking for None appears 
>>> sufficient to me. Other than that, the test for using start_message 
>>> should be a separate test function, not an extension of the current one 
>>> - its a new test, and the mutter() call seems entirely spurious to me.
>>>
>> Again I was just following what was there. I was trying to be cautious
>> to avoid problems on things like win32. I'll factor out the code to do
>> the editor thing and make it a separate test.
>>
> 
> And here is the bundle.
> 
> Thanks,
> 
> James
> 

...

I'm pretty sure that msgfile = os.close() is bogus (I don't think it
returns anything useful, and certainly it is never used).

I think it would be better to just do:

os.close(tmp_fileno)
msgfile = None

...
if msgfile is None:
    msgfile = file(msgfilename, "w")

Maybe at one point it was doing
msgfile = os.fdopen(tmp_fileno)

>          msgfile = os.close(tmp_fileno)
> +        havefile = False
> +        if start_message is not None:
> +            havefile = True
> +            msgfile = file(msgfilename, "w")
> +            msgfile.write("%s\n" % start_message.encode(
> +                                     bzrlib.user_encoding, 'replace'))
>          if infotext is not None and infotext != "":
>              hasinfo = True
> -            msgfile = file(msgfilename, "w")
> +            if not havefile:
> +              msgfile = file(msgfilename, "w")
> +              havefile = True
>              msgfile.write("\n\n%s\n\n%s" % (ignoreline,
>                  infotext.encode(bzrlib.user_encoding, 'replace')))
> -            msgfile.close()
>          else:
>              hasinfo = False
>  
> +        if havefile:
> +          msgfile.close()
> +

^- This is indented incorrectly, it should be 4 spaces, not 2.

Otherwise it seems reasonable.

John
=:->



More information about the bazaar mailing list