threads / patch / 13720

patchmake commit --interactive lock index

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

## tl;dr

12 messages between May 29, 2008 and Jun 2, 2008. Diffs are folded; open one to read it.

replies: 11people: 5as markdown or json

Paolo Bonzini· May 29, 2008, 08:09 UTC · lore

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.
Show changes to builtin-commit.c +3 −6
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
Johannes Schindelin· May 29, 2008, 12:43 UTC · re: Paolo Bonzini · lore

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

Hi,
On Thu, 29 May 2008, Paolo Bonzini wrote:
Show 5 quoted lines
> @@ -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

Paolo Bonzini· May 29, 2008, 13:12 UTC · re: Johannes Schindelin · lore

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

Johannes Schindelin wrote:
Show 12 quoted lines
> 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
Johannes Schindelin· May 29, 2008, 13:55 UTC · re: Paolo Bonzini · lore

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

Hi,
On Thu, 29 May 2008, Paolo Bonzini wrote:
Show 22 quoted lines
> 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

Paolo Bonzini· May 29, 2008, 14:40 UTC · re: Johannes Schindelin · lore

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

Show 16 quoted lines
>>>> +	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
Alex Riesen· May 29, 2008, 17:51 UTC · re: Paolo Bonzini · lore

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

Paolo Bonzini, Thu, May 29, 2008 16:40:57 +0200:
Show 21 quoted lines
>>>>> +	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?
Paolo Bonzini· May 29, 2008, 18:00 UTC · re: Alex Riesen · lore

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

Show 6 quoted lines
>> 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
Alex Riesen· May 29, 2008, 18:56 UTC · re: Paolo Bonzini · lore

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

Paolo Bonzini, Thu, May 29, 2008 20:00:55 +0200:
Show 10 quoted lines
>
>>> 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?
Paolo Bonzini· May 29, 2008, 19:17 UTC · re: Alex Riesen · lore

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

Alex Riesen wrote:
Show 10 quoted lines
> 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
Junio C Hamano· May 30, 2008, 05:29 UTC · re: Paolo Bonzini · lore

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

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?
Paolo Bonzini· May 30, 2008, 07:42 UTC · re: Junio C Hamano · lore

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

> 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
Paolo Bonzini· Jun 2, 2008, 13:52 UTC · re: Junio C Hamano · lore

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

> 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

← back to recent threads