Re: [PATCH 2/2] rebase -i: new option --name-rev

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.

Dominique Quatravaux [off-list ref] writes:

If set, the second column of the rebase todo contains named revisions (obtained
with git name-rev) instead of short SHA1s.

Hum.  I'm not sure yet if I find that very useful, since frequently the
names will just be 'topic', 'topic~1', ...., 'topic~N' if you are
rebasing a topic with N+1 commits not in master.  But you might, so who
am I to judge.

+--name-rev::
+--no-name-rev::

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

+configuration variable `rebase.interactivenamerev`, this option can be

You should spell it in a more readable way such as
rebase.interactiveNameRev.  The config machinery internally downcases
everything so the cosmetics won't prevent it from working.

-	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?

How does this interact with --autosquash?

+test_expect_success 'Symbolic revisions in --name-rev' '
+	exec > debug.log 2>&1 &&
+	FAKE_LINES="exec_cp_.git/rebase-merge/git-rebase-todo_rebase-todo 1 2" git rebase -i --name-rev HEAD~2 &&
+	test_cmp expect-rebase-todo rebase-todo
+'

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

-- 
Thomas Rast
trast@{inf,student}.ethz.ch

On Thu, Mar 8, 2012 at 11:56 AM, Thomas Rast [off-list ref] 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?

quoted

+configuration variable `rebase.interactivenamerev`, this option can be

You should spell it in a more readable way such as
rebase.interactiveNameRev.

Will do.

quoted

-     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