threads / patch / 23795

patchgit-rebase--interactive: Handle commit messages that end in backslash

Subject: [PATCH] git-rebase--interactive: Handle commit messages that end in backslash

## tl;dr

4 messages between May 13, 2010 and May 14, 2010. Diffs are folded; open one to read it.

replies: 3people: 3as markdown or json

Chris Lamb· May 13, 2010, 12:39 UTC · lore

This patch fixes three issues with handling commits that end in a backslash character ("\"), two of which had potential for silent data "loss".

First, if you had two commits
  abcde...   Remove trailing \
  01234...   Another commit
then git rebase -i formatted this in your editor like so:
 pick abcde Remove trailing pick 01234: Another commit

Assuming you don't spot this visually, git-rebase will then happily skip over 01234 upon processing, removing that commit disappear from your HEAD (!).

Secondly, when skipping over commits whose parents were unchanged, if any of these commits ended in a backslash, it would try and ignore it.

Lastly, this patch fixes "edit"ing and "squash"ing of commits that end in a backslash; git-rebase would previously blow up.

Signed-off-by: Chris Lamb <lamby@debian.org>
---
 git-rebase--interactive.sh |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)
Regards,
-- 
      ,''`.
     : :'  :     Chris Lamb
     `. `'`      lamby@debian.org
       `-


From 63a17f07dc8eba199f59699a055eebd6e696f27f Mon Sep 17 00:00:00 2001
From: Chris Lamb <lamby@debian.org>
Date: Thu, 13 May 2010 13:36:45 +0100
Subject: [PATCH] git-rebase--interactive: Handle commit messages that end in backslash

This patch fixes three issues with handling commits that end in a backslash
character ("\"), two of which had potential for silent data "loss".

First, if you had two commits

  abcde...   Remove trailing \
  01234...   Another commit

then git rebase -i formatted this in your editor like so:

 pick abcde Remove trailing pick 01234: Another commit

Assuming you don't spot this visually, git-rebase will then happily skip
over 01234 upon processing, removing that commit disappear from your HEAD
(!).

Secondly, when skipping over commits whose parents were unchanged, if any
of these commits ended in a backslash, it would try and ignore it.

Lastly, this patch fixes "edit"ing and "squash"ing of commits that end in a
backslash; git-rebase would previously blow up.

Signed-off-by: Chris Lamb <lamby@debian.org>
---
 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 436b7f5..31c5fd2 100755
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -450,7 +450,7 @@ record_in_rewritten() {
 
 do_next () {
 	rm -f "$MSG" "$AUTHOR_SCRIPT" "$AMEND" || exit
-	read command sha1 rest < "$TODO"
+	sed -e 's/\\$/\\\\/g' "$TODO" | read command sha1 rest
 	case "$command" in
 	'#'*|''|noop)
 		mark_action_done
@@ -591,7 +591,7 @@ do_rest () {
 # skip picking commits whose parents are unchanged
 skip_unnecessary_picks () {
 	fd=3
-	while read command sha1 rest
+	sed -e 's/\\$/\\\\/g' "$TODO" | while read command sha1 rest
 	do
 		# fd=3 means we skip the command
 		case "$fd,$command,$(git rev-parse --verify --quiet $sha1^)" in
@@ -607,7 +607,7 @@ skip_unnecessary_picks () {
 			;;
 		esac
 		echo "$command${sha1:+ }$sha1${rest:+ }$rest" >&$fd
-	done <"$TODO" >"$TODO.new" 3>>"$DONE" &&
+	done >"$TODO.new" 3>>"$DONE" &&
 	mv -f "$TODO".new "$TODO" &&
 	case "$(peek_next_command)" in
 	squash|s|fixup|f)
@@ -890,7 +890,7 @@ first and then run 'git rebase --continue' again."
 		git rev-list $MERGES_OPTION --pretty=oneline --abbrev-commit \
 			--abbrev=7 --reverse --left-right --topo-order \
 			$REVISIONS | \
-			sed -n "s/^>//p" | while read shortsha1 rest
+			sed -n "s/^>//p" | sed -e 's/\\$/\\\\/g' | while read shortsha1 rest
 		do
 			if test t != "$PRESERVE_MERGES"
 			then
-- 
1.7.1
Chris Lamb· May 13, 2010, 12:58 UTC · re: Chris Lamb · lore

Re: [PATCH] git-rebase--interactive: Handle commit messages that end in backslash

Chris Lamb wrote:
 
> This patch fixes three issues with handling commits that end in a
> backslash character ("\"), two of which had potential for silent data
> "loss".

Disregard the patch; it has some funky side-effect which breaks ONTO parsing and I don't know why.

(The problem still exists though.)
Regards,
-- 
      ,''`.
     : :'  :     Chris Lamb
     `. `'`      lamby@debian.org
       `-
Jeff King· May 13, 2010, 14:33 UTC · re: Chris Lamb · lore

Re: [PATCH] git-rebase--interactive: Handle commit messages that end in backslash

On Thu, May 13, 2010 at 01:58:07PM +0100, Chris Lamb wrote:
> Disregard the patch; it has some funky side-effect which breaks ONTO
> parsing and I don't know why.
>
> (The problem still exists though.)
Hmm. It looks like there is more weirdness, too. Try
  git commit -m 'magic \\n newline'

I get a stray newline in the git-rebase-todo file. IIRC, we had this problem before because some shells' "echo" will interpret, and some will not (I think bash will not, but dash will). So there are a few "echo" calls that need to be "printf %s" in git-rebase--interactive.

But I wonder if we would do better to simply sanitize the subject line of metacharacters. As I understand it[1], the subject line shown is for human consumption only, and the actual operations happen on the included sha1. So we could munge it in a non-reversible way without breaking anything.

-Peff

[1] I may be wrong. This is not code I'm too familiar with, and I do remember a proposal at some point for automagically handling subjects with "wip" or "squash" in them. Still, a light munging may work.

Thomas Rast· May 14, 2010, 10:15 UTC · re: Jeff King · lore

Re: [PATCH] git-rebase--interactive: Handle commit messages that end in backslash

Jeff King wrote:
Show 10 quoted lines
> As I understand it[1], the subject line shown is for
> human consumption only, and the actual operations happen on the included
> sha1. So we could munge it in a non-reversible way without breaking
> anything.
> 
> -Peff
> 
> [1] I may be wrong. This is not code I'm too familiar with, and I do
> remember a proposal at some point for automagically handling subjects
> with "wip" or "squash" in them. Still, a light munging may work.

During the actual rebasing, only the 'cmd sha' portion of the line matters, as evidenced by the code doing

  do_next () {
          rm -f "$MSG" "$AUTHOR_SCRIPT" "$AMEND" || exit
          read command sha1 rest < "$TODO"
and then not using $rest except in status messages.

The automatic handling you are referring to is --autosquash, which happens in rearrange_squash() which is a preprocessing stage. AFAICS that function is still okay iff the munging maps prefixes to prefixes.

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

← back to recent threads