patch, 2 partsrebase -i: optimize the creation of the todo file

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(-)

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(-)

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:

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.

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

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.

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?

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

On Thu, Mar 8, 2012 at 11:56 AM, Thomas Rast <trast@inf.ethz.ch> wrote:

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

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

Do you mean I should omit it from the doc?

Will do.

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.

Will do.

Dominique Quatravaux <domq@google.com> writes:

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.

On Thu, Mar 8, 2012 at 11:56 AM, Thomas Rast <trast@inf.ethz.ch> wrote:

Dominique Quatravaux <domq@google.com> writes:

On Thu, Mar 8, 2012 at 7:58 PM, Junio C Hamano <gitster@pobox.com> wrote:

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

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

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.

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

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.

Am 3/8/2012 23:13, schrieb Junio C Hamano:

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

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

On Fri, Mar 9, 2012 at 8:22 AM, Johannes Sixt <j.sixt@viscovery.net> wrote:

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 think that Junio's suggestion fixes that (at the expense of 8 of the precious 80 columns).

--
  Dominique Quatravaux
  +41 79 609 40 72

Dominique Quatravaux <domq@google.com> writes:

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

Dominique Quatravaux <domq@google.com> writes:

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:

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

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

On Thu, Mar 8, 2012 at 11:48 AM, Thomas Rast <trast@inf.ethz.ch> wrote:

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

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

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.

Am 3/8/2012 11:42, schrieb Dominique Quatravaux:

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

-- Hannes

On Thu, Mar 8, 2012 at 12:20 PM, Johannes Sixt <j.sixt@viscovery.net> wrote:

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

On Thu, Mar 8, 2012 at 12:36 PM, Dominique Quatravaux <domq@google.com> wrote:

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.

Am 3/8/2012 12:41, schrieb Dominique Quatravaux:

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