[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