Re: [PATCHv3] git-rebase--interactive.sh: extend "edit" command to be more useful

From: Kevin Ballard <kevin@sb.org>
Date: Dec 3, 2010, 08:16 UTC
Message-ID: <048EACFB-2038-4D49-B6C3-7E7354F62171@sb.org>
In-Reply-To: <20101203080603.GC18202@burratino>

On Dec 3, 2010, at 12:06 AM, Jonathan Nieder wrote:

Show 11 quoted lines

> Hi,
> 
> Kevin Ballard wrote:
> 
>> [Subject: [PATCHv3] git-rebase--interactive.sh: extend "edit" command to be more useful
> 
> Maybe something like
> 
> 	rebase-i: treat "edit" without sha1 as a request to amend previous commit
> 
> would make the meaning more obvious in a shortlog.

That seems a bit misleading, though. This command really has nothing to do with amending the previous commit. You can do anything you want once you break back to the shell. I personally used it to run git-merge at that point in the history. For this reason I'm a bit uneasy about overloading "edit", but it does have the benefit that people already know "edit" brings them to the shell.

Show 16 quoted lines

>> Extend the "edit" command to simply stop for editing if no sha1 is
>> given or if the sha1 is equal to "-". This behaves the same as "x false"
>> but is a bit friendlier for the user.
> 
> Nice.  I like the semantics.
> 
>> * Picked up the extended explanation suggested by Jonathan Nieder.
>>  I left off the last line about "noop" as that doesn't seem related.
> 
> Right, please feel free to remind me if I forget to pick that up again.
> 
>> * If the line given is "edit - some comments", emit "some comments" when
>>  stopped. This is undocumented
> 
> I think that's okay for now (though of course it would be best to explain
> some example uses in Documentation/git-rebase.txt in the form of examples).

Yep, I definitely need to add documentation.

Show 18 quoted lines

>> --- a/git-rebase--interactive.sh
>> +++ b/git-rebase--interactive.sh
>> @@ -469,12 +469,29 @@ do_next () {
>> +			comment="$rest"
>> +			line=$(git rev-list --pretty=oneline -1 --abbrev-commit --abbrev=7 HEAD)
> 
> Hmm, the script seems to assume rev-list will not fail throughout.  :/
> Ok.
> 
>> +			sha1="${line%% *}"
>> +			rest="${line#* }"
>> +			echo "$sha1" > "$DOTEST"/stopped-sha
> 
> Maybe this can be done without relying on details of --pretty=oneline
> format?
> 
> 			sha1=$(git rev-parse --short HEAD)
> 			rest=$(git show -s --format=%s HEAD)

Does this not similarly assume that rev-parse and show will not fail? Or was the above comment only meant to point out this potential issue without suggesting that it needed to be fixed?

Show 20 quoted lines

> (Yes, elsewhere the script uses
> 
> 	git rev-list --no-merges --pretty=oneline --abbrev-commit \
> 		--abbrev=7 --reverse --left-right --topo-order "$@" |
> 	sed -n "s/^>//p" |
> 	while read -r shortsha1 rest
> 
> but in that loop, avoiding an extra exec seems more important.)
> 
>> +		fi
>> 		git rev-parse --verify HEAD > "$AMEND"
>> 		warn "Stopped at $sha1... $rest"
>> +		if test -n "$comment"; then
>> +			warn
>> +			warn "	$comment"
>> +			warn
> 
> Thanks, looks good to me.
> 
> Ideas for tests?  (see t3404 for inspiration)

I'll look into that. I wasn't really sure how to test this before, but t3404 does have some examples of testing the edit command already.

-Kevin Ballard