[PATCH] make commit --interactive lock index

[PATCH] make commit --interactive lock index

[Paolo Bonzini]

I noticed that the way "git commit --interactive" sets up the commit
is different from the way a normal "git commit" does it.  Commit
2888605c changed one, but not the other.  This makes the behavior
equivalent in the two cases.

Signed-off-by: Paolo Bonzini <bonzini@gnu.org>
---
 builtin-commit.c |    9 +++------
 1 files changed, 3 insertions(+), 6 deletions(-)

	I am sending this patch for review as I don't know if it's
	necessary.  The code is a tad cleaner, but it does cause more
	system calls in the commit --interactive case, because of the
	additional read of the index.

	The assert tests that, in the interactive case, we'll go down to
	the COMMIT_ASIS case.

	The patch is on top of the previous change I sent for signal
	handling in git-commit.

diff --git a/builtin-commit.c b/builtin-commit.c
index ef8b1f0..5a5f9a3 100644
--- a/builtin-commit.c
+++ b/builtin-commit.c
@@ -219,13 +219,8 @@ static char *prepare_index(int argc, const char **argv, const char *prefix)
 	struct path_list partial;
 	const char **pathspec = NULL;
 
-	if (interactive) {
+	if (interactive)
 		interactive_add(argc, argv, prefix);
-		if (read_cache() < 0)
-			die("index file corrupt");
-		commit_style = COMMIT_AS_IS;
-		return get_index_file();
-	}
 
 	if (read_cache() < 0)
 		die("index file corrupt");
@@ -233,6 +228,8 @@ static char *prepare_index(int argc, const char **argv, const char *prefix)
 	if (*argv)
 		pathspec = get_pathspec(prefix, argv);
 
+	assert (!(interactive && pathspec && *pathspec));
+
 	signal (SIGINT, rollback_on_signal);
 	signal (SIGHUP, rollback_on_signal);
 	signal (SIGTERM, rollback_on_signal);
-- 
1.5.5

Re: [PATCH] make commit --interactive lock index

[Johannes Schindelin]

Hi,

On Thu, 29 May 2008, Paolo Bonzini wrote:

> @@ -233,6 +228,8 @@ static char *prepare_index(int argc, const char **argv, const char *prefix)
>  	if (*argv)
>  		pathspec = get_pathspec(prefix, argv);
>  
> +	assert (!(interactive && pathspec && *pathspec));

As pathspec is specified indirectly by the user, I think an assert() here 
is actively wrong.

Ciao,
Dscho

Re: [PATCH] make commit --interactive lock index

[Paolo Bonzini]

Johannes Schindelin wrote:
> Hi,
> 
> On Thu, 29 May 2008, Paolo Bonzini wrote:
> 
>> @@ -233,6 +228,8 @@ static char *prepare_index(int argc, const char **argv, const char *prefix)
>>  	if (*argv)
>>  		pathspec = get_pathspec(prefix, argv);
>>  
>> +	assert (!(interactive && pathspec && *pathspec));
> 
> As pathspec is specified indirectly by the user, I think an assert() here 
> is actively wrong.

But the program may still guarantee a condition by checking it 
elsewhere.  I don't need to teach you about that, do I?  In particular, 
the assert checks that this:

if (interactive && argc > 0)
         die("Paths with --interactive does not make sense.");

... is equivalent to !pathspec || !*pathspec.

Paolo

Re: [PATCH] make commit --interactive lock index

[Johannes Schindelin]

Hi,

On Thu, 29 May 2008, Paolo Bonzini wrote:

> Johannes Schindelin wrote:
> 
> > On Thu, 29 May 2008, Paolo Bonzini wrote:
> > 
> > > @@ -233,6 +228,8 @@ static char *prepare_index(int argc, const char
> > > **argv, const char *prefix)
> > >   if (*argv)
> > >    pathspec = get_pathspec(prefix, argv);
> > > 
> > > +	assert (!(interactive && pathspec && *pathspec));
> > 
> > As pathspec is specified indirectly by the user, I think an assert() 
> > here is actively wrong.
> 
> But the program may still guarantee a condition by checking it 
> elsewhere.  I don't need to teach you about that, do I?  In particular, 
> the assert checks that this:
> 
> if (interactive && argc > 0)
>         die("Paths with --interactive does not make sense.");
> 
> ... is equivalent to !pathspec || !*pathspec.

Okay, I have to spell it out:

I think that the assert() here is not helpful at all, and that you should 
rather do the "if () die()" thingie.

Ciao,
Dscho

Re: [PATCH] make commit --interactive lock index

[Paolo Bonzini]

>>>> +	assert (!(interactive && pathspec && *pathspec));
>>> As pathspec is specified indirectly by the user, I think an assert() 
>>> here is actively wrong.
>> But the program may still guarantee a condition by checking it 
>> elsewhere.  I don't need to teach you about that, do I?  In particular, 
>> the assert checks that this:
>>
>> if (interactive && argc > 0)
>>         die("Paths with --interactive does not make sense.");
>>
>> ... is equivalent to !pathspec || !*pathspec.
> 
> Okay, I have to spell it out:
> 
> I think that the assert() here is not helpful at all, and that you should 
> rather do the "if () die()" thingie.

The "if() die ()" thingie is already in builtin-commit.c, so we won't 
ever get a pathspec in the "add --interactive" case.  If we do, 
something else has already been done incorrectly before -- not by the 
user but by the programmer.

Paolo

Re: [PATCH] make commit --interactive lock index

[Alex Riesen]

Paolo Bonzini, Thu, May 29, 2008 16:40:57 +0200:
>>>>> +	assert (!(interactive && pathspec && *pathspec));
>>>> As pathspec is specified indirectly by the user, I think an 
>>>> assert() here is actively wrong.
>>> But the program may still guarantee a condition by checking it  
>>> elsewhere.  I don't need to teach you about that, do I?  In 
>>> particular, the assert checks that this:
>>>
>>> if (interactive && argc > 0)
>>>         die("Paths with --interactive does not make sense.");
>>>
>>> ... is equivalent to !pathspec || !*pathspec.
>>
>> Okay, I have to spell it out:
>>
>> I think that the assert() here is not helpful at all, and that you 
>> should rather do the "if () die()" thingie.
>
> The "if() die ()" thingie is already in builtin-commit.c, so we won't  
> ever get a pathspec in the "add --interactive" case.  If we do,  
> something else has already been done incorrectly before -- not by the  
> user but by the programmer.

What could that be?

Re: [PATCH] make commit --interactive lock index

[Paolo Bonzini]

>> The "if() die ()" thingie is already in builtin-commit.c, so we won't  
>> ever get a pathspec in the "add --interactive" case.  If we do,  
>> something else has already been done incorrectly before -- not by the  
>> user but by the programmer.
> 
> What could that be?

Nothing, but it documents to whoever reads the code what is the path 
that will be taken.  Anyway if it happened it would be very bad.

Paolo

Re: [PATCH] make commit --interactive lock index

[Alex Riesen]

Paolo Bonzini, Thu, May 29, 2008 20:00:55 +0200:
>
>>> The "if() die ()" thingie is already in builtin-commit.c, so we won't 
>>>  ever get a pathspec in the "add --interactive" case.  If we do,   
>>> something else has already been done incorrectly before -- not by the 
>>>  user but by the programmer.
>>
>> What could that be?
>
> Nothing, but it documents to whoever reads the code what is the path  
> that will be taken.  Anyway if it happened it would be very bad.

Why is a comment not enough?

Re: [PATCH] make commit --interactive lock index

[Paolo Bonzini]

Alex Riesen wrote:
> Paolo Bonzini, Thu, May 29, 2008 20:00:55 +0200:
>>>> The "if() die ()" thingie is already in builtin-commit.c, so we won't 
>>>>  ever get a pathspec in the "add --interactive" case.  If we do,   
>>>> something else has already been done incorrectly before -- not by the 
>>>>  user but by the programmer.
>>> What could that be?
>> Nothing, but it documents to whoever reads the code what is the path  
>> that will be taken.  Anyway if it happened it would be very bad.
> 
> Why is a comment not enough?

I tend to use comment if it is more interesting to express the condition 
in English, and asserts if it is more interesting to express it as code.

Paolo

Re: [PATCH] make commit --interactive lock index

[Junio C Hamano]

Paolo Bonzini <bonzini@gnu.org> writes:

> I noticed that the way "git commit --interactive" sets up the commit
> is different from the way a normal "git commit" does it.  Commit
> 2888605c changed one, but not the other.  This makes the behavior
> equivalent in the two cases.

Sorry, you need to defend this change much better than that.

I fail to see why it is a bad thing to have differences among the
codepaths that do different things.  The quoted commit 2888605
(builtin-commit: fix partial-commit support, 2007-11-18) was _not_ about
"doing it the same way".  It was simply "commit has three cases, AS_IS,
NORMAL and PARTIAL; the codepath that implements PARTIAL case is not done
right, so fix it to behave exactly the same as the old scripted version".

When interactive_add() returns, the index has already been updated, and
there is no reason to take the index lock, refresh the index, nor anything
that the normal "as-is" commit codepath needs to do (let alone the
alternate index dance forced upon the partial commit codepath).

If your change were so that "git commit --interactive" reverts the index
when one of the hooks exited non-zero just like COMMIT_NORMAL case (as
opposed to the current code which does not revert the index), I would
understand the need to change what's inside "if (interactive)" block.  But
that is not what the patch is about.  Changing the codepath so that it
does not return from that block made me follow unnecessary and unrelated
codepath to convince myself that this patch is mostly a no-op, except that
you are doing an extra refresh_cache() on the index that interactive_add()
has already refreshed before giving the control back to you.

Can you explain why this is an improvement?

Re: [PATCH] make commit --interactive lock index

[Paolo Bonzini]

> Can you explain why this is an improvement?

No, I was just wondering if this was a bug, possibly one that shows in 
weird cases such as when the index is updated under git-commit's feet. 
As I said in the cover letter, I sent this patch only for review from 
more knowledgeable people.

Paolo

Re: [PATCH] make commit --interactive lock index

[Paolo Bonzini]

> If your change were so that "git commit --interactive" reverts the index
> when one of the hooks exited non-zero just like COMMIT_NORMAL case (as
> opposed to the current code which does not revert the index), I would
> understand the need to change what's inside "if (interactive)" block.

So why does the COMMIT_AS_IS path need to lock?  The comment is not clear:

          * The caller should run hooks on the real index, and run
          * hooks on the real index, and create commit from the_index.
          * We still need to refresh the index here.

It seems to me that the lock+refresh+write+commit done by plain "git 
commit" is useless too, since it also runs in the case "pathspec && 
*pathspec".

I guess the patch should be withdrawn, and so I didn't want to follow up 
anymore, but the weird comment made me change my mind...

(The other patch I sent -- which is at 
http://permalink.gmane.org/gmane.comp.version-control.git/83209 -- is 
not withdrawn and not related to this one).

Paolo