Re: [PATCH 3/4] git-commit: Refactor creation of log message.

From: Paolo Bonzini <bonzini@gnu.org>
Date: Feb 4, 2008, 17:14 UTC
Message-ID: <47A747EB.9020909@gnu.org>
In-Reply-To: <alpine.LSU.1.00.0802041646461.7372@racer.site>

Johannes Schindelin wrote:

Show 14 quoted lines

> 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