From: Paolo Bonzini <bonzini@gnu.org>
Date: Mon, 04 Feb 2008 17:14:19 GMT
Subject: Re: [PATCH 3/4] git-commit: Refactor creation of log message.
Message-ID: <47A747EB.9020909@gnu.org>
In-Reply-To: <alpine.LSU.1.00.0802041646461.7372@racer.site>

Johannes Schindelin wrote:
> Hi,
> 
> On Mon, 21 Jan 2008, Paolo Bonzini wrote:
> 
>> This means that: 1) the commit may be aborted after editing the message
>> if there is a problem writing out the tree object (slight disadvantage);
> 
> I consider this more than a slight disadvantage.  I regularly take ages 
> coming up with a good commit message, because I think that the overall 
> time balance is better with me spending more time on the message, but 
> every reader spending less time to guess what I meant.
> 
> So I would be quite annoyed to edit a message, only to find out that for 
> whatever reason the commit was not successful.

Just to make it clearer, the piece of code that would have to fail, for 
the behavior to change, is this:

         discard_cache();
         read_cache_from(index_file);
         if (!active_cache_tree)
                 active_cache_tree = cache_tree();
         if (cache_tree_update(active_cache_tree,
                               active_cache, active_nr, 0, 0) < 0) {
                 rollback_index_files();
                 die("Error building trees");
         }

There are a couple of failure modes hidden in update_one (in cache-tree.c):

         sub = find_subtree(it, path + baselen, entlen, 0);
         if (!sub)
                 die("cache-tree.c: '%.*s' in '%s' not found",
                     entlen, path + baselen, path);

         ...

         if (mode != S_IFGITLINK && !missing_ok && !has_sha1_file(sha1))
                 return error("invalid object %s", sha1_to_hex(sha1));

but I don't really understand how they could happen.  If you do, I 
appreciate being taught.  :-)

Also note that some problems writing the tree object (I cannot think of 
anything but running out of diskspace -- permission errors could be 
caught creating the index and logmessage too) could also happen writing 
the commit object.  I don't think in practice the disadvantage I 
mentioned can happen.

> Are you _sure_ you need 3/4 for 4/4?

Probably no, but it makes it much easier to avoid duplicated code (all 
the cases are already enumerated in prepare_log_message).  I tried it 
first and did not finish because the code was really really ugly.

Thanks for the review.

Paolo