threads / rfc / 1266

RFC patchgit-rev-parse vs IFS

Subject: [RFC/PATCH] git-rev-parse vs IFS

## tl;dr

4 messages between Jul 17, 2005 and Jul 18, 2005. Diffs are folded; open one to read it.

replies: 3people: 1as markdown or json

Junio C Hamano· Jul 17, 2005, 06:59 UTC · lore

"git whatchanged" does not work with parameters that have an IFS character; this includes a filename with SP in it. A silly example:

    $ git-whatchanged --max-count=1
    diff-tree 98800286dffc76736549d7279622157ca2be9a3b (from 9171e0...
    Author: Junio C Hamano <junkio@cox.net>
    Date:   Sat Jul 16 23:13:03 2005 -0700
        add foo
    :000000 100644 0000000000000000000000000000000000000000 8b13789...
    $ git-whatchanged --max-count=1 'foo bar'
    $ ;# no output!

This is because the tail parameter to git-diff-tree used in git-whatchanged is $(git-rev-parse --no-revs "$@"), and the output from git-rev-parse is split at $IFS, so it ends up getting "foo" and "bar" separately).

If it were just embedded SP and TAB in the filnames, I would say we can set IFS to LF in the script that use git-rev-parse, declare that we do not support filenames with embedded LF, and be done with it. The thing is, for pickaxe, it is very natural to give a multi-line parameter, so setting IFS to LF is not a general workaround.

Doing this portably is possible but a bit of pain. I can add git-rev-parse ability to sq_quote its outputs, and change the callers to build a command line to "eval" using it. Here is an RFC patch that does it. My barf-o-tolerance for shell quoting and evaling is usually much higher than others, so while I feel this is not too ugly, it may offend your aesthetics.

------------ Help scripts that use git-rev-parse to grok args with SP/TAB/LF

The git-rev-parse command uses LF to separate each arguments it parses, so its users at least need to set IFS to LF to be able to handle filenames with embedded SPs and TABs. Some commands, however, can take and do expect arguments with embedded LF (notably, -Spickaxe of diff family), so even this workaround does not work for them.

When --sq flag to git-rev-parse is given, instead of showing one argument per line, it outputs arguments quoted for consumption with "eval" by the caller, to remedy this situation.

As an example, this patch converts git-whatchanged to use this new feature.

Signed-off-by: Junio C Hamano <junkio@cox.net>
---
 git-whatchanged |    9 ++++++---
 rev-parse.c     |   36 ++++++++++++++++++++++++++++++++++--
 2 files changed, 40 insertions(+), 5 deletions(-)
79cdb9505b0ee9a47969c3cbc8c8bf2a35b23ca8
Show changes to 2 files +40 −5

git-whatchanged, rev-parse.c

diff --git a/git-whatchanged b/git-whatchanged
--- a/git-whatchanged
+++ b/git-whatchanged
@@ -1,4 +1,7 @@
 #!/bin/sh
-git-rev-list $(git-rev-parse --default HEAD --revs-only "$@") |
-	git-diff-tree --stdin --pretty -r $(git-rev-parse --no-revs "$@") |
-	LESS="$LESS -S" ${PAGER:-less}
+rev_list_args=$(git-rev-parse --sq --default HEAD --revs-only "$@") &&
+diff_tree_args=$(git-rev-parse --sq --no-revs "$@") &&
+
+eval "git-rev-list $rev_list_args" |
+eval "git-diff-tree --stdin --pretty -r $diff_tree_args" |
+LESS="$LESS -S" ${PAGER:-less}
diff --git a/rev-parse.c b/rev-parse.c
--- a/rev-parse.c
+++ b/rev-parse.c
@@ -15,6 +15,7 @@ static int do_rev_argument = 1;
 static int output_revs = 0;
 static int flags_only = 0;
 static int no_flags = 0;
+static int output_sq = 0;
 
 #define NORMAL 0
 #define REVERSED 1
@@ -54,14 +55,41 @@ static void show_rev(int type, const uns
 	if (no_revs)
 		return;
 	output_revs++;
+
+	/* Hexadecimal string plus possibly a carret;
+	 * this does not have to be quoted even under output_sq.
+	 */
 	printf("%s%s\n", type == show_type ? "" : "^", sha1_to_hex(sha1));
 }
 
+static void show(const char *arg)
+{
+	if (output_sq) {
+		int sq = '\'';
+		const char *next;
+
+		putchar(sq);
+		while (*arg) {
+			next = strchr(arg, sq);
+			if (!next) {
+				fputs(arg, stdout);
+				break;
+			}
+			fputs("'\\''", stdout);
+			arg = next + 1;
+		}
+		putchar(sq);
+		putchar('\n');
+	}
+	else
+		puts(arg);
+}
+
 static void show_rev_arg(char *rev)
 {
 	if (no_revs)
 		return;
-	puts(rev);
+	show(rev);
 }
 
 static void show_norev(char *norev)
@@ -70,7 +98,7 @@ static void show_norev(char *norev)
 		return;
 	if (revs_only)
 		return;
-	puts(norev);
+	show(norev);
 }
 
 static void show_arg(char *arg)
@@ -328,6 +356,10 @@ int main(int argc, char **argv)
 				single_rev = 1;
 				continue;
 			}
+			if (!strcmp(arg, "--sq")) {
+				output_sq = 1;
+				continue;
+			}
 			if (!strcmp(arg, "--not")) {
 				show_type ^= REVERSED;
 				continue;
Junio C Hamano· Jul 17, 2005, 08:06 UTC · re: Junio C Hamano · lore

[PATCH] rev-parse --sq bugfixes.

We expect --sq output to be used to construct a string to be "eval"ed in shell scripts, so we should not be separating the parameter with LF, which would cause the current command to be ended.

Also there was a silly thinko for doing the shell quoting.
Signed-off-by: Junio C Hamano <junkio@cox.net>
---

*** This is a fix on top of the "git-rev-parse vs IFS" RFC *** patch.

 rev-parse.c |   29 ++++++++++++++++-------------
 1 files changed, 16 insertions(+), 13 deletions(-)
bf5a2b2f3cd5693560ab72974f9956c76a8fd667
Show changes to rev-parse.c +16 −13
diff --git a/rev-parse.c b/rev-parse.c
--- a/rev-parse.c
+++ b/rev-parse.c
@@ -50,18 +50,6 @@ static int is_rev_argument(const char *a
 	}
 }
 
-static void show_rev(int type, const unsigned char *sha1)
-{
-	if (no_revs)
-		return;
-	output_revs++;
-
-	/* Hexadecimal string plus possibly a carret;
-	 * this does not have to be quoted even under output_sq.
-	 */
-	printf("%s%s\n", type == show_type ? "" : "^", sha1_to_hex(sha1));
-}
-
 static void show(const char *arg)
 {
 	if (output_sq) {
@@ -75,16 +63,31 @@ static void show(const char *arg)
 				fputs(arg, stdout);
 				break;
 			}
+			while (arg < next)
+				putchar(*arg++);
 			fputs("'\\''", stdout);
 			arg = next + 1;
 		}
 		putchar(sq);
-		putchar('\n');
+		putchar(' ');
 	}
 	else
 		puts(arg);
 }
 
+static void show_rev(int type, const unsigned char *sha1)
+{
+	if (no_revs)
+		return;
+	output_revs++;
+
+	/* Hexadecimal string plus possibly a carret;
+	 * this does not have to be quoted even under output_sq.
+	 */
+	printf("%s%s%c", type == show_type ? "" : "^", sha1_to_hex(sha1),
+	       output_sq ? ' ' : '\n');
+}
+
 static void show_rev_arg(char *rev)
 {
 	if (no_revs)
Junio C Hamano· Jul 18, 2005, 17:27 UTC · re: Junio C Hamano · lore

[PATCH] Help scripts that use git-rev-parse to grok args with SP/TAB/LF

The git-rev-parse command uses LF to separate each arguments it parses, so its users at least need to set IFS to LF to be able to handle filenames with embedded SPs and TABs. Some commands, however, can take and do expect arguments with embedded LF (notably, -Spickaxe of diff family), so even this workaround does not work for them.

When --sq flag to git-rev-parse is given, instead of showing one argument per line, it outputs arguments quoted for consumption with "eval" by the caller, to remedy this situation.

As an example, this patch converts git-whatchanged to use this feature.

Signed-off-by: Junio C Hamano <junkio@cox.net>
---
*** This is a replacement for two patches I sent earlier:
***   [RFC/PATCH] git-rev-parse vs IFS
***   [PATCH] rev-parse --sq bugfixes.
 git-whatchanged |    9 ++++++---
 rev-parse.c     |   34 +++++++++++++++++++++++++++++++---
 2 files changed, 37 insertions(+), 6 deletions(-)
a99f0d64fae9a121a1ecbfd07d7667cf4aa0f19d
Show changes to 2 files +37 −6

git-whatchanged, rev-parse.c

diff --git a/git-whatchanged b/git-whatchanged
--- a/git-whatchanged
+++ b/git-whatchanged
@@ -1,4 +1,7 @@
 #!/bin/sh
-git-rev-list $(git-rev-parse --default HEAD --revs-only "$@") |
-	git-diff-tree --stdin --pretty -r $(git-rev-parse --no-revs "$@") |
-	LESS="$LESS -S" ${PAGER:-less}
+rev_list_args=$(git-rev-parse --sq --default HEAD --revs-only "$@") &&
+diff_tree_args=$(git-rev-parse --sq --no-revs "$@") &&
+
+eval "git-rev-list $rev_list_args" |
+eval "git-diff-tree --stdin --pretty -r $diff_tree_args" |
+LESS="$LESS -S" ${PAGER:-less}
diff --git a/rev-parse.c b/rev-parse.c
--- a/rev-parse.c
+++ b/rev-parse.c
@@ -15,6 +15,7 @@ static int do_rev_argument = 1;
 static int output_revs = 0;
 static int flags_only = 0;
 static int no_flags = 0;
+static int output_sq = 0;
 
 #define NORMAL 0
 #define REVERSED 1
@@ -49,19 +50,42 @@ static int is_rev_argument(const char *a
 	}
 }
 
+static void show(const char *arg)
+{
+	if (output_sq) {
+		int sq = '\'', ch;
+
+		putchar(sq);
+		while ((ch = *arg++)) {
+			if (ch == sq)
+				fputs("'\\'", stdout);
+			putchar(ch);
+		}
+		putchar(sq);
+		putchar(' ');
+	}
+	else
+		puts(arg);
+}
+
 static void show_rev(int type, const unsigned char *sha1)
 {
 	if (no_revs)
 		return;
 	output_revs++;
-	printf("%s%s\n", type == show_type ? "" : "^", sha1_to_hex(sha1));
+
+	/* Hexadecimal string plus possibly a carret;
+	 * this does not have to be quoted even under output_sq.
+	 */
+	printf("%s%s%c", type == show_type ? "" : "^", sha1_to_hex(sha1),
+	       output_sq ? ' ' : '\n');
 }
 
 static void show_rev_arg(char *rev)
 {
 	if (no_revs)
 		return;
-	puts(rev);
+	show(rev);
 }
 
 static void show_norev(char *norev)
@@ -70,7 +94,7 @@ static void show_norev(char *norev)
 		return;
 	if (revs_only)
 		return;
-	puts(norev);
+	show(norev);
 }
 
 static void show_arg(char *arg)
@@ -328,6 +352,10 @@ int main(int argc, char **argv)
 				single_rev = 1;
 				continue;
 			}
+			if (!strcmp(arg, "--sq")) {
+				output_sq = 1;
+				continue;
+			}
 			if (!strcmp(arg, "--not")) {
 				show_type ^= REVERSED;
 				continue;
Junio C Hamano· Jul 17, 2005, 08:45 UTC · re: Junio C Hamano · lore

[PATCH] Audit rev-parse users.

This patch changes rev-parse users that pass a single argument that is supposed to be a rev parameter to use "--verify".

Signed-off-by: Junio C Hamano <junkio@cox.net>
---
*** This does not have anything to do with the --sq flag.
 git-checkout-script |    2 +-
 git-cherry          |    8 ++++----
 git-commit-script   |    2 +-
 git-rebase-script   |    8 ++++----
 git-resolve-script  |    4 ++--
 5 files changed, 12 insertions(+), 12 deletions(-)
eee0f165b4762203c0d827bae7480daf41514f17
Show changes to 5 files +12 −12

git-checkout-script, git-cherry, git-commit-script, git-rebase-script, git-resolve-script

diff --git a/git-checkout-script b/git-checkout-script
--- a/git-checkout-script
+++ b/git-checkout-script
@@ -22,7 +22,7 @@ while [ "$#" != "0" ]; do
 		force=1
 		;;
 	*)
-		rev=$(git-rev-parse --verify --revs-only "$arg^0") || exit
+		rev=$(git-rev-parse --verify "$arg^0") || exit
 		if [ -z "$rev" ]; then
 			echo "unknown flag $arg"
 			exit 1
diff --git a/git-cherry b/git-cherry
--- a/git-cherry
+++ b/git-cherry
@@ -28,11 +28,11 @@ The output is intended to be used as:
 '
 
 case "$#" in
-1) linus=`git-rev-parse "$1"` &&
-   junio=`git-rev-parse HEAD` || exit
+1) linus=`git-rev-parse --verify "$1"` &&
+   junio=`git-rev-parse --verify HEAD` || exit
    ;;
-2) linus=`git-rev-parse "$1"` &&
-   junio=`git-rev-parse "$2"` || exit
+2) linus=`git-rev-parse --verify "$1"` &&
+   junio=`git-rev-parse --verify "$2"` || exit
    ;;
 *) echo >&2 "$usage"; exit 1 ;;
 esac
diff --git a/git-commit-script b/git-commit-script
--- a/git-commit-script
+++ b/git-commit-script
@@ -15,7 +15,7 @@ do
     -m) shift
         case "$#" in
 	0) usage ;;
-	*) use_commit=`git-rev-parse "$1"` ||
+	*) use_commit=`git-rev-parse --verify "$1"` ||
 	   exit ;;
 	esac
 	;;
diff --git a/git-rebase-script b/git-rebase-script
--- a/git-rebase-script
+++ b/git-rebase-script
@@ -11,11 +11,11 @@ upstream tree.'
 : ${GIT_DIR=.git}
 
 case "$#" in
-1) linus=`git-rev-parse "$1"` &&
-   junio=`git-rev-parse HEAD` || exit
+1) linus=`git-rev-parse --verify "$1"` &&
+   junio=`git-rev-parse --verify HEAD` || exit
    ;;
-2) linus=`git-rev-parse "$1"` &&
-   junio=`git-rev-parse "$2"` || exit
+2) linus=`git-rev-parse --verify "$1"` &&
+   junio=`git-rev-parse --verify "$2"` || exit
    ;;
 *) echo >&2 "$usage"; exit 1 ;;
 esac
diff --git a/git-resolve-script b/git-resolve-script
--- a/git-resolve-script
+++ b/git-resolve-script
@@ -6,8 +6,8 @@
 #
 . git-sh-setup-script || die "Not a git archive"
 
-head=$(git-rev-parse --revs-only "$1")
-merge=$(git-rev-parse --revs-only "$2")
+head=$(git-rev-parse --verify "$1")
+merge=$(git-rev-parse --verify "$2")
 merge_msg="$3"
 
 dropheads() {

← back to recent threads