Code review comment for lp:~vila/bzr/673637-temp-commit-file

Revision history for this message
Vincent Ladeuil (vila) wrote :

>>>>> John A Meinel <email address hidden> writes:

    > Review: Approve
    > I'm "ok" with this fix. It seems to solve a problem that someone was having.

    > The concerns are:

    > 1) it seems like the problem is actually in emacs, and we are
    > papering around it in bzr

Right, I chose to avoid the finger syndrom, hence the 'using the working
directory for a temporary file is not needed there' in the cover letter.

    > 2) It means that you won't use the cwd for staging the commit log,
    > in case someone wants to save it off somewhere for later (in case
    > of failure to commit/rollback/etc.)

I think we want to address this differently: both qbzr and bzr-gtk are
already saving it in branch.conf so:
- we need the savng to be done by bzr itself,
- we really want to save it in tree.conf instead.

In the mean time, this fix avoids the infamous OLMD (OS Locks Must
Die). I'm prety sure there is at least one bug asking for the above so
we won't forget.

    > 3) I don't have any personal stake, because I always use "-m" and
    > shell as my commit message editor. (With the other benefit that
    > 'bzr diff' always continues to work.) As such, I don't have a feel
    > for how this will impact people, since I don't use it.

Same here.

    > 4) I think this might also solve a bug people were having, where
    > they wanted to commit it /etc, and had rights to /etc/.bzr but
    > *not* rights to /etc/ itself. (sysadmins not wanting to commit as
    > root, I guess.)

Indeed.

    > On the whole, I think I'm for it, just concerned about possible fallout.

I was too, that's why I kept the test and tweaked it instead of just
removing it.

« Back to merge proposal