From: Dominique Quatravaux <domq@google.com>
Date: Thu, 08 Mar 2012 11:57:02 GMT
Subject: Re: [PATCH 2/2] rebase -i: new option --name-rev
Message-ID: <CAJh6GrFKJw1506c6nOzh+EcNe7E_UM=NxV_yBPWx9BUCdpAvAg@mail.gmail.com>
In-Reply-To: <87399jnyxh.fsf@thomas.inf.ethz.ch>

On Thu, Mar 8, 2012 at 11:56 AM, Thomas Rast <trast@inf.ethz.ch> wrote:
> On a general note: you are submitting a completely new feature touching
> a heavily-used tool (and code path) during -rc0 time.  As a rule of
> thumb: Don't do that.  If you do it, don't Cc Junio unless it's his area
> of code.

[- gitster]
Sorry about that, I skimmed Junio's "What's cooking in git.git (Mar
2012, #03; Mon, 5)" and I thought I was in the "high value/damage
ratio" category.

> Hum.  I'm not sure yet if I find that very useful, since frequently the
> names will just be 'topic', 'topic~1', ...., 'topic~N'

Exactly, and when rebasing a branch with merges, the "foreign" commits
will stick out which is what I want.

> The --no- version is implicitly always supported, see gitcli(1).

Do you mean I should omit it from the doc?

>> +configuration variable `rebase.interactivenamerev`, this option can be
>
> You should spell it in a more readable way such as
> rebase.interactiveNameRev.

Will do.

>> -     shortsha1=$(echo $sha1 | cut -c1-7)
>> +     if test t = "$name_rev"
>> +     then
>> +             rev="$(git name-rev $sha1 | cut -d\  -f2)"
>> +     else
>> +             rev=$(echo $sha1 | cut -c1-7)
>> +     fi
>
> In the spirit of your previous patch, wouldn't it be faster to run 'git
> name-rev --stdin' within the pipeline?

I fear that it would also substitute hashes on the right-hand side (in
the commit messages), and also I would be piping data from and to git
name-rev from the same process which is a recipe for deadlocks.

> In line with the --autosquash concern, please write a test that uses
> both option (and verifies that *both* work!).

Will do.

-- 
  Dominique Quatravaux
  +41 79 609 40 72