Re: [PATCH 5/7] gitk: Make cherry-pick call git-citool on conflicts.

From: Alexander Gavrilov <angavrilov@gmail.com>
Date: Oct 9, 2008, 08:24 UTC
Message-ID: <bb6f213e0810090124i62a51e92n45836ee02577331a@mail.gmail.com>
In-Reply-To: <18669.46569.45285.170033@cargo.ozlabs.ibm.com>

On Thu, Oct 9, 2008 at 11:42 AM, Paul Mackerras <paulus@samba.org> wrote:

Show 5 quoted lines

>> +proc exec_citool {args {baseid {}}} {
>
> I'm a little nervous about you having a parameter called "args", since
> that specific name has a special meaning in Tcl; it's how Tcl handles
> variable-length argument lists.

Yes, I'd better change that. Probably it works only because that name is special only as the last parameter.

Show 6 quoted lines

> If we can assume the existence of a shell (which we do elsewhere), we
> can perhaps do this more simply by putting the environment variable
> settings in the command before the command name.  It's a pity that git
> citool won't take the author name/email/date as command-line arguments
> or from a file, since it ends up being pretty verbose doing it the way
> you have.

If I understand correctly, using a shell would require composing the command as a string, which itself requires quoting the author name & email, and other argument strings. I did not feel confident enough to do that, so chose a dumb but safe solution.

Show 14 quoted lines

>> @@ -7861,7 +7908,17 @@ proc cherrypick {} {
>>      # no error occurs, and exec takes that as an indication of error...
>>      if {[catch {exec sh -c "git cherry-pick -r $rowmenuid 2>&1"} err]} {
>>       notbusy cherrypick
>> -     error_popup $err
>> +     if {[regexp -line \
>> +         {Entry '(.*)' would be overwritten by merge} $err msg fname]} {
>> +         error_popup [mc "Cherry-pick failed: file '%s' had local modifications.
>> +Your working directory is in an inconsistent state." $fname]
>
> That message seems a bit too scary.  It's not inconsistent, it's just
> got local modifications.  If I remember correctly, in this situation
> git cherry-pick will back out all the changes it did and leave the
> working directory as it was before.

Yes, I'll have to reword it.

> [list] as an idiom for the empty list is a little unusual (here and
> elsewhere in your patches); {} would be more usual.

I'm more used to languages where lists and strings are very different types... Even in perl you have to use [] for an empty list ref, and '' for an empty string.

Alexander