[PATCH 1/2] rebase -i: optimize the creation of the todo file

[PATCH 1/2] rebase -i: optimize the creation of the todo file

[Dominique Quatravaux]

Instead of obtaining short SHA1's from "git rev-list" and hitting the repository
once more with "git rev-parse" for the full-size SHA1's, obtain long SHA1's from
"git rev-list" and truncate them with "cut".
---
 git-rebase--interactive.sh |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 5812222..8dcb8b0 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -774,17 +774,17 @@ else
 	revisions=$onto...$orig_head
 	shortrevisions=$shorthead
 fi
-git rev-list $merges_option --pretty=oneline --abbrev-commit \
-	--abbrev=7 --reverse --left-right --topo-order \
+git rev-list $merges_option --pretty=oneline --no-abbrev-commit \
+	--reverse --left-right --topo-order \
 	$revisions | \
 	sed -n "s/^>//p" |
-while read -r shortsha1 rest
+while read -r sha1 rest
 do
+	shortsha1=$(echo $sha1 | cut -c1-7)
 	if test t != "$preserve_merges"
 	then
 		printf '%s\n' "pick $shortsha1 $rest" >> "$todo"
 	else
-		sha1=$(git rev-parse $shortsha1)
 		if test -z "$rebase_root"
 		then
 			preserve=t
-- 
1.7.7.3

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

[Dominique Quatravaux]

If set, the second column of the rebase todo contains named revisions (obtained
with git name-rev) instead of short SHA1s.
---
 Documentation/git-rebase.txt  |   11 +++++++++++
 git-rebase--interactive.sh    |   11 ++++++++---
 git-rebase.sh                 |   10 ++++++++++
 t/t3404-rebase-interactive.sh |   11 +++++++++++
 4 files changed, 40 insertions(+), 3 deletions(-)

diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
index 504945c..e7ecd2c 100644
--- a/Documentation/git-rebase.txt
+++ b/Documentation/git-rebase.txt
@@ -365,6 +365,17 @@ If the '--autosquash' option is enabled by default using the
 configuration variable `rebase.autosquash`, this option can be
 used to override and disable this setting.
 
+--name-rev::
+--no-name-rev::
+	Instead of showing short SHA1 hashes in the todo list, show
+	human-readable revisions obtained with linkgit:git-name-rev[1].
++
+This option is only valid when the '--interactive' option is used.
++
+If the '--name-rev' option is enabled by default using the
+configuration variable `rebase.interactivenamerev`, this option can be
+used to override and disable this setting.
+
 --no-ff::
 	With --interactive, cherry-pick all rebased commits instead of
 	fast-forwarding over the unchanged ones.  This ensures that the
diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 8dcb8b0..5583dcb 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -780,10 +780,15 @@ git rev-list $merges_option --pretty=oneline --no-abbrev-commit \
 	sed -n "s/^>//p" |
 while read -r sha1 rest
 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
 	if test t != "$preserve_merges"
 	then
-		printf '%s\n' "pick $shortsha1 $rest" >> "$todo"
+		printf '%s\n' "pick $rev $rest" >> "$todo"
 	else
 		if test -z "$rebase_root"
 		then
@@ -801,7 +806,7 @@ do
 		if test f = "$preserve"
 		then
 			touch "$rewritten"/$sha1
-			printf '%s\n' "pick $shortsha1 $rest" >> "$todo"
+			printf '%s\n' "pick $rev $rest" >> "$todo"
 		fi
 	fi
 done
diff --git a/git-rebase.sh b/git-rebase.sh
index 69c1374..9330be3 100755
--- a/git-rebase.sh
+++ b/git-rebase.sh
@@ -43,6 +43,8 @@ s,strategy=!       use the given merge strategy
 no-ff!             cherry-pick all commits, even if unchanged
 m,merge!           use merging strategies to rebase
 i,interactive!     let the user edit the list of commits to rebase
+name-rev           show revisions by name in the list of commits
+no-name-rev        show revisions by short SHA1 in the list (default)
 f,force-rebase!    force rebase even if branch is up to date
 X,strategy-option=! pass the argument through to the merge strategy
 stat!              display a diffstat of what changed upstream
@@ -98,6 +100,8 @@ action=
 preserve_merges=
 autosquash=
 test "$(git config --bool rebase.autosquash)" = "true" && autosquash=t
+name_rev=
+test "$(git config --bool rebase.interactivenamerev)" = "true" && name_rev=t
 
 read_basic_state () {
 	head_name=$(cat "$state_dir"/head-name) &&
@@ -287,6 +291,12 @@ do
 	-f|--no-ff)
 		force_rebase=t
 		;;
+	--name-rev)
+		name_rev=t
+		;;
+	--no-name-rev)
+		name_rev=
+		;;
 	--rerere-autoupdate|--no-rerere-autoupdate)
 		allow_rerere_autoupdate="$1"
 		;;
diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index b981572..299ce40 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -163,6 +163,17 @@ test_expect_success 'exchange two commits' '
 	test G = $(git cat-file commit HEAD | sed -ne \$p)
 '
 
+cat > expect-rebase-todo <<EOF
+pick branch1~1 H
+pick branch1 G
+EOF
+
+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
+'
+
 cat > expect << EOF
 diff --git a/file1 b/file1
 index f70f10e..fd79235 100644
-- 
1.7.7.3

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

[Thomas Rast]

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 <domq@google.com> 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

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

[Dominique Quatravaux]

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

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

[Junio C Hamano]

Dominique Quatravaux <domq@google.com> writes:

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

Well, these days, nothing that comes without prior discussion on
pain points before the feature freeze, be it from seasoned list
regulars or from people new to this list, can be of so high-value
that it cannot wait until the next cycle.

The only handful of exceptions I can think of are:

 (1) an obvious fix to a high-risk bug (new or old) that was
     recently discovered as a potential attack vector;

 (2) an obvious fix to a bug in a new feature added in the upcoming
     release; or

 (3) an obviously necessary tweak and adjustment to parts of the
     system related to a new feature added in the upcoming release.

(1) is called "security fix" and (2) is "brown paper bag fix".

I do not think of a good word to explain (3), but for example, if a
release candidate added a feature to "git merge" so that a signed
tag can be merged while recording the signature in the resulting
commit, but the feature did not trigger when the merge is made via
"git pull" because it unwrapped the fetched tag to a commit before
calling "git merge", an update to "git pull" and probably "git
fmt-merge-msg" that is used by "git pull" may have to be made after
we enter the soft feature freeze (-rc0) for the new feature in "git
merge" to be useful.

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

[Dominique Quatravaux]

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. [..]

Dominique Quatravaux <domq@google.com> writes:
>> 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.

On Thu, Mar 8, 2012 at 7:58 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Well, these days, nothing that comes without prior discussion on
> pain points before the feature freeze, be it from seasoned list
> regulars or from people new to this list, can be of so high-value
> that it cannot wait until the next cycle.

ACK. I'm cool with skipping a cycle, they are so fast anyways!

-- 
  Dominique Quatravaux
  +41 79 609 40 72

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

[Junio C Hamano]

Thomas Rast <trast@inf.ethz.ch> writes:

> Dominique Quatravaux <domq@google.com> 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.

I think the only use case where this might be useful is when you
have totally undescriptive one-line description to your commits that
they alone do not help distinguishing the commits being picked, e.g.

  pick 47a02ff add frotz
  pick d41489a fix frotz
  pick 00c8fd4 fix frotz more
  pick 090ea12 again fix frotz
  pick 74775a0 fix frotz one more time
  pick 6f7f3be at least now frotz compiles

As we treat the one-line subject merely as a human aid, I would
probably do not mind if a workaround for such an issue were done to
produce something like this instead:

  pick 47a02ff (1) add frotz
  pick d41489a (2) fix frotz
  pick 00c8fd4 (3) fix frotz more
  pick 090ea12 (4) again fix frotz
  pick 74775a0 (5) fix frotz one more time
  pick 6f7f3be (6) at least now frotz compiles

It is not a problem with the current rebase-i that detaches HEAD
while working, but using object names like "topic~7" that is
relative to something inherently fluid (i.e. a branch) makes me feel
nervous as an end user from aesthetics point of view.

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

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Thomas Rast <trast@inf.ethz.ch> writes:
>
>> Dominique Quatravaux <domq@google.com> 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.
>
> I think the only use case where this might be useful is when you
> have totally undescriptive one-line description to your commits that
> they alone do not help distinguishing the commits being picked, e.g.
> ...

This may need a bit of clarification for readers from the future.
If you _were_ somehow interactively rebasing changes made on two or
more branches into a single branch, knowing which branch each commit
came from may have value, even if your commit titles are descriptive
enough.

Today's "git rebase -i" wouldn't do something like that, and we will
not know how the user would interact with such a yet-to-be-written
tool, so it is too early to judge if using "topic~1" is the desired
improvement or not at this point.

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

[Johannes Sixt]

Am 3/8/2012 23:13, schrieb Junio C Hamano:
> Junio C Hamano <gitster@pobox.com> writes:
> 
>> Thomas Rast <trast@inf.ethz.ch> writes:
>>
>>> Dominique Quatravaux <domq@google.com> 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.
>>
>> I think the only use case where this might be useful is when you
>> have totally undescriptive one-line description to your commits that
>> they alone do not help distinguishing the commits being picked, e.g.
>> ...
> 
> This may need a bit of clarification for readers from the future.
> If you _were_ somehow interactively rebasing changes made on two or
> more branches into a single branch, knowing which branch each commit
> came from may have value, even if your commit titles are descriptive
> enough.
> 
> Today's "git rebase -i" wouldn't do something like that, and we will
> not know how the user would interact with such a yet-to-be-written
> tool, so it is too early to judge if using "topic~1" is the desired
> improvement or not at this point.

Yet-to-be-written? Rebase -i happily linearizes mergy history, so this
does have some merits even today.

I do share your concerns that naming to-be-rebased commits with a relative
specifier such as "topic~1" could be dangerous. However, this is a problem
only when the rebase -i is not completed timely, so that you have
sufficient time to mess with the ref "topic" from a different terminal.
You would have to run "git branch -f", "git fetch", or "git push" (the
latter could even happen from remote) that involve the ref name. Can't we
just declare this as "don't do that then"? (We do say this more often than
not since recently :-)

-- Hannes

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

[Dominique Quatravaux]

Junio C Hamano <gitster@pobox.com> writes:
>> Today's "git rebase -i" wouldn't do something like that, and we will
>> not know how the user would interact with such a yet-to-be-written
>> tool, so it is too early to judge if using "topic~1" is the desired
>> improvement or not at this point.
>
On Fri, Mar 9, 2012 at 8:22 AM, Johannes Sixt <j.sixt@viscovery.net> wrote:
> Yet-to-be-written? Rebase -i happily linearizes mergy history, so this
> does have some merits even today.

Right, my personal itch is to "transplant" topic branches without their merge
history getting in the way, eg go from


   M1 ------ M2 ----- M3 ---- M4                master
     \                  \
      \           B1 --- B2 --- B3              topicB
       \         /
        A1 --- A2 --- A3                        topicA

to

   M1 ---- M2 ---- M3 ---- M4                   master
     \                       \
      \                       B1 ---- B3        topicB
       \
        A1 ---- A2 ---- A3                      topicA


If I "git rebase -i --onto master topicA topicB", the rebase todo might go like

  pick 1234abc Cool shiny new stuff
  pick 234abc1 Something something master
  pick 34abc12 Fix something something
  pick 4abc123 Fix shiny new stuff

With my patch (combined with Junio's suggestion, and some whitespace padding
for extra niceness) we would get instead

  pick 1234abc (topicB~2) Cool shiny new stuff
  pick 234abc1 (master~2) Something something master
  pick 34abc12 (master~1) Fix something something
  pick 4abc123 (topicB)   Fix shiny new stuff

Snip the lines that don't m/topicB/ with your text editor, save file, done.

> I do share your concerns that naming to-be-rebased commits with a relative
> specifier such as "topic~1" could be dangerous. However, this is a problem
> only when the rebase -i is not completed timely, so that you have
> sufficient time to mess with the ref "topic" from a different terminal.

I think that Junio's suggestion fixes that (at the expense of 8 of the precious
80 columns).

--
  Dominique Quatravaux
  +41 79 609 40 72

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

[Junio C Hamano]

Dominique Quatravaux <domq@google.com> writes:

> On Fri, Mar 9, 2012 at 8:22 AM, Johannes Sixt <j.sixt@viscovery.net> wrote:
>> Rebase -i happily linearizes mergy history, so this
>> does have some merits even today.
>
> Right, my personal itch is to "transplant" topic branches without their merge
> history getting in the way,...
> ...
>   pick 1234abc (topicB~2) Cool shiny new stuff
>   pick 234abc1 (master~2) Something something master
>   pick 34abc12 (master~1) Fix something something
>   pick 4abc123 (topicB)   Fix shiny new stuff

Ok. I didn't consider the "flattening" aspect of the rebase, and the
above makes sense to me.

Re: [PATCH 1/2] rebase -i: optimize the creation of the todo file

[Thomas Rast]

Dominique Quatravaux <domq@google.com> writes:

> Instead of obtaining short SHA1's from "git rev-list" and hitting the repository
> once more with "git rev-parse" for the full-size SHA1's, obtain long SHA1's from
> "git rev-list" and truncate them with "cut".

That doesn't work if there are 7-digit SHA1 collisions in the repo.
Even my git.git has a bunch of them, as checked with

  git rev-list --objects --all | cut -c1-7 | sort | uniq -d

and I expect a bigger project would have collisions beyond the 9th
digit.

What you can, however, do:

> -git rev-list $merges_option --pretty=oneline --abbrev-commit \
> -	--abbrev=7 --reverse --left-right --topo-order \
> +git rev-list $merges_option --pretty=oneline --no-abbrev-commit \
> +	--reverse --left-right --topo-order \
>  	$revisions | \

rev-list can give you *both* the abbreviated and full SHA1s if you say

  git rev-list $merges_option --format="%>%h %H %s" <...etc...>

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

Re: [PATCH 1/2] rebase -i: optimize the creation of the todo file

[Dominique Quatravaux]

On Thu, Mar 8, 2012 at 11:48 AM, Thomas Rast <trast@inf.ethz.ch> wrote:
> Dominique Quatravaux <domq@google.com> writes:
>
>> Instead of obtaining short SHA1's from "git rev-list" and hitting the repository
>> once more with "git rev-parse" for the full-size SHA1's, obtain long SHA1's from
>> "git rev-list" and truncate them with "cut".
>
> That doesn't work if there are 7-digit SHA1 collisions in the repo.

I don't see how my patch changes the picture wrt SHA1 collisions. In
the current state, the rebase todo already uses short SHA1s.

> Even my git.git has a bunch of them, as checked with
>
>  git rev-list --objects --all | cut -c1-7 | sort | uniq -d
>
> and I expect a bigger project would have collisions beyond the 9th
> digit.
>
> What you can, however, do:
>
>> -git rev-list $merges_option --pretty=oneline --abbrev-commit \
>> -     --abbrev=7 --reverse --left-right --topo-order \
>> +git rev-list $merges_option --pretty=oneline --no-abbrev-commit \
>> +     --reverse --left-right --topo-order \
>>       $revisions | \
>
> rev-list can give you *both* the abbreviated and full SHA1s if you say
>
>  git rev-list $merges_option --format="%>%h %H %s" <...etc...>

Interesting, thanks! It seems that %h works with find_unique_abbrev
which according to the name, should Do The Right Thing. I'll update my
patch.


-- 
  Dominique Quatravaux
  +41 79 609 40 72

Re: [PATCH 1/2] rebase -i: optimize the creation of the todo file

[Thomas Rast]

Dominique Quatravaux <domq@google.com> writes:

> On Thu, Mar 8, 2012 at 11:48 AM, Thomas Rast <trast@inf.ethz.ch> wrote:
>> Dominique Quatravaux <domq@google.com> writes:
>>
>>> Instead of obtaining short SHA1's from "git rev-list" and hitting the repository
>>> once more with "git rev-parse" for the full-size SHA1's, obtain long SHA1's from
>>> "git rev-list" and truncate them with "cut".
>>
>> That doesn't work if there are 7-digit SHA1 collisions in the repo.
>
> I don't see how my patch changes the picture wrt SHA1 collisions. In
> the current state, the rebase todo already uses short SHA1s.

But it gets them from --abbrev-commit, which makes sure that the
abbreviation is unique.  The 7 is just a request for the minimal number.

You can easily verify this in git.git with e.g.

  git show --abbrev-commit --abbrev=7 \
      2539a7b64dbc7e214704f6b879749a94685fb99e

Note how the output uses an 8-digit SHA1 in the first line.  The 7-digit
colliding commit is 2539a7bdb81dcc920c97837a9bfb9d4c4e1ac280.

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

Re: [PATCH 1/2] rebase -i: optimize the creation of the todo file

[Johannes Sixt]

Am 3/8/2012 11:42, schrieb Dominique Quatravaux:
> +	shortsha1=$(echo $sha1 | cut -c1-7)

> -		sha1=$(git rev-parse $shortsha1)

Why do you call it "optimization" when you spend two or three subprocesses
instead of one?

-- Hannes

Re: [PATCH 1/2] rebase -i: optimize the creation of the todo file

[Dominique Quatravaux]

On Thu, Mar 8, 2012 at 12:20 PM, Johannes Sixt <j.sixt@viscovery.net> wrote:
> Am 3/8/2012 11:42, schrieb Dominique Quatravaux:
>> +     shortsha1=$(echo $sha1 | cut -c1-7)
>
>> -             sha1=$(git rev-parse $shortsha1)
>
> Why do you call it "optimization" when you spend two or three subprocesses
> instead of one?

echo is a shell internal. "git rev-parse" is two processes just as
"cut" and a pipe. The difference is that cut doesn't hit the git
repository.

But the real purpose of this first change is to lay the ground for the
next one, so I'd be happy to change the patch description...


-- 
  Dominique Quatravaux
  +41 79 609 40 72

Re: [PATCH 1/2] rebase -i: optimize the creation of the todo file

[Dominique Quatravaux]

On Thu, Mar 8, 2012 at 12:36 PM, Dominique Quatravaux <domq@google.com> wrote:
> On Thu, Mar 8, 2012 at 12:20 PM, Johannes Sixt <j.sixt@viscovery.net> wrote:
>> Am 3/8/2012 11:42, schrieb Dominique Quatravaux:
>>> +     shortsha1=$(echo $sha1 | cut -c1-7)
>>
>>> -             sha1=$(git rev-parse $shortsha1)
>>
>> Why do you call it "optimization" when you spend two or three subprocesses
>> instead of one?
>
> echo is a shell internal. "git rev-parse" is two processes just as
> "cut" and a pipe.

My mistake, strace git rev-parse revals that this is only one process.
Still, I think that saving a bunch of filesystem access beats saving
one fork() (one of the two processes in my patched version is a shell,
so no execve() there) but I admit I haven't benchmarked this.


-- 
  Dominique Quatravaux
  +41 79 609 40 72

Re: [PATCH 1/2] rebase -i: optimize the creation of the todo file

[Johannes Sixt]

Am 3/8/2012 12:41, schrieb Dominique Quatravaux:
> On Thu, Mar 8, 2012 at 12:36 PM, Dominique Quatravaux <domq@google.com> wrote:
>> On Thu, Mar 8, 2012 at 12:20 PM, Johannes Sixt <j.sixt@viscovery.net> wrote:
>>> Am 3/8/2012 11:42, schrieb Dominique Quatravaux:
>>>> +     shortsha1=$(echo $sha1 | cut -c1-7)
>>>
>>>> -             sha1=$(git rev-parse $shortsha1)
>>>
>>> Why do you call it "optimization" when you spend two or three subprocesses
>>> instead of one?
>>
>> echo is a shell internal. "git rev-parse" is two processes just as
>> "cut" and a pipe.
> 
> My mistake, strace git rev-parse revals that this is only one process.
> Still, I think that saving a bunch of filesystem access beats saving
> one fork()... 

Not so on Windows.

But you must look at the repository in any case to avoid truncating the
SHA1 too much, as Thomas pointed out.

-- Hannes