[GUILT PATCH v2 0/5] Add guards to guilt

[GUILT PATCH v2 0/5] Add guards to guilt

[Eric Lesh]

Changes in v2
        - get_series returns guarded series, get_full_series gives
          full series.  Most commands want the guarded series anyway.
        - more options for guilt-select (better hg compatability)
        - rename guilt-guards to guilt-guard (better hg compatability)
        - a couple bug fixes
        - an extra patch that adds a test suite

This series adds Mercurial Queues-like guards to guilt.  It allows you
to assign guards to related patches in the series file to selectively
push patches.

See Chapter 13 of the HG Book for more info.
(http://hgbook.red-bean.com/hgbookch13.html)

Guards are appended as "#+foo" or "#-foo" to the end of the patch name
in the series file, so they are compatible with quilt. The
guard setting and unsetting functions also respect comments on the
patch line, so they aren't mangled when you use guards.

This changes `get_series` to return the guarded series of patches that
will be actually be applied.  `get_full_series` returns the full
series file.

If you change guards on a patch or select a different guard while
patches are applied, some commands might get confused.  See
`guilt-select --pop` and `guilt-select --reapply` to avoid having to
`guilt-pop -a` yourself.

I did everything (maybe?) you suggested except implement a
safe_sed wrapper and a validate_guards function.  The sed stuff is
maybe better for a later patch that covers most sed usage in guilt,
and the validate_guards isn't really necessary given the small number
of times it's called.  If you would prefer either of those to get
done, let me know.

[PATCH 1/5] get_series: Remove comments from end of series lines
       This just strips everything but the patch name from get_series,
       to hide comments or guards on the line.

[PATCH 2/5] guilt-guard: Assign guards to patches in series
       This adds the guilt-guard command and utility functions to
       guilt.  Introduces get_guarded_series, which is removed in
       patch 4.

[PATCH 3/5] guilt-select: Select guards to apply when pushing patches
       This puts selected guards in .git/patch/$branch/guards, and
       adds a $guards_file variable to guilt.

[PATCH 4/5] get_series: return guarded patches only
       Actually distinguish between guarded and non-guarded patches in
       a number of commands by making get_series return the guarded
       series, and introducing get_full_series to return the unguarded
       series.  Lots of changes.
       If anything breaks, it's in this patch.  Hopefully.

[PATCH 5/5] Guards test suite
       Test stuff that uses get_series or get_full_series.

[GUILT PATCH 1/5] get_series: Remove comments from end of series lines

[Eric Lesh]

Also make sure that series_remove_patch and series_rename_patch do the
right thing if there are comments on the line.

Signed-off-by: Eric Lesh <eclesh@ucla.edu>
---
 guilt                    |   15 ++++++++++++---
 regression/050-series.sh |    2 +-
 2 files changed, 13 insertions(+), 4 deletions(-)

diff --git a/guilt b/guilt
index 7f2b93c..700c167 100755
--- a/guilt
+++ b/guilt
@@ -178,7 +178,13 @@ get_series()
 	#	- whitespace only
 	#	- optional whitespace followed by '#' followed by more
 	#	  optional whitespace
-	grep -ve '^[[:space:]]*\(#.*\)*$' "$series"
+	# also remove comments from end of lines
+	sed -n -e "/^[[:space:]]*\(#.*\)*\$/ ! {
+		s/[[:space:]]*#.*\$//
+
+		p
+		}
+		" $series
 }
 
 # usage: do_make_header <hash>
@@ -290,14 +296,17 @@ series_insert_patch()
 # usage: series_remove_patch <patchname>
 series_remove_patch()
 {
-	grep -v "^$1\$" < "$series" > "$series.tmp"
+	grep -v "^$1[[:space:]]*#*" < "$series" > "$series.tmp"
 	mv "$series.tmp" "$series"
 }
 
 # usage: series_rename_patch <oldname> <newname>
 series_rename_patch()
 {
-	awk -v old="$1" -v new="$2" \
+	# Rename the patch, but preserve comments on the line
+	old=$(grep -e "^$1[[:space:]]*" $series)
+	new=$(echo "$old" | sed -e "s,^$1,$2,")
+	awk -v old="$old" -v new="$new" \
 		'{ if ($0 == old) print new; else print $0 }' \
 		"$series" > "$series.tmp"
 
diff --git a/regression/050-series.sh b/regression/050-series.sh
index eb23540..4c47e9d 100755
--- a/regression/050-series.sh
+++ b/regression/050-series.sh
@@ -26,7 +26,7 @@ modify
 add
 
 remove
-mode
+mode # and text
 #sure
 DONE
 }
-- 
1.5.2

Re: [GUILT PATCH 1/5] get_series: Remove comments from end of series lines

[Josef Sipek]

On Mon, Jul 30, 2007 at 08:11:17PM -0700, Eric Lesh wrote:
...
> @@ -290,14 +296,17 @@ series_insert_patch()
>  # usage: series_remove_patch <patchname>
>  series_remove_patch()
>  {
> -	grep -v "^$1\$" < "$series" > "$series.tmp"
> +	grep -v "^$1[[:space:]]*#*" < "$series" > "$series.tmp"
>  	mv "$series.tmp" "$series"
>  }

I haven't tested it, but I believe that this change would be very
bad...suppose you have series with patches "foo" and "foo-2". Now if you try
to remove foo, the regexp would end up being:

"^foo[ ]*#*"

This would match both patches!
  
>  # usage: series_rename_patch <oldname> <newname>
>  series_rename_patch()
>  {
> -	awk -v old="$1" -v new="$2" \
> +	# Rename the patch, but preserve comments on the line
> +	old=$(grep -e "^$1[[:space:]]*" $series)

This could match multiple lines.

> +	new=$(echo "$old" | sed -e "s,^$1,$2,")

One more comment about sed...there are a number of place in guilt that take
patch names and create regexps from them, but don't make sure things get
properly escaped. I guess all the sed changes should be done in one go.

Also, beware that this is in a function, so setting $new and $old affects
the _all_ of guilt. One way around it would be a force subshell to execute
using parenthesis around the contents of the function body.

> +	awk -v old="$old" -v new="$new" \
>  		'{ if ($0 == old) print new; else print $0 }' \
>  		"$series" > "$series.tmp"
>  
> diff --git a/regression/050-series.sh b/regression/050-series.sh
> index eb23540..4c47e9d 100755
> --- a/regression/050-series.sh
> +++ b/regression/050-series.sh
> @@ -26,7 +26,7 @@ modify
>  add
>  
>  remove
> -mode
> +mode # and text
>  #sure
>  DONE
>  }

Good :)

Jeff.

-- 
NT is to UNIX what a doughnut is to a particle accelerator.

[GUILT PATCH 2/5] guilt-guard: Assign guards to patches in series

[Eric Lesh]

guilt-guard will assign guards to a patch.  They work so that:

    * Patches with no guards are always pushed.

    * Patches with positive guards (i.e. +foo) are pushed *only if* the
      guard is selected.

    * Patches with negative guards (i.e. -foo) are pushed *unless* the
      guard is selected.

This also introduces a number of guard-related utility functions into
guilt.  get_guarded_series is used to get the list of patches that are
applicable given the current guard status.  It replaces get_series in
a later patch.

Signed-off-by: Eric Lesh <eclesh@ucla.edu>
---
 Documentation/guilt-guard.txt |   40 +++++++++++++++++++++++
 guilt                         |   66 +++++++++++++++++++++++++++++++++++++++
 guilt-guard                   |   69 +++++++++++++++++++++++++++++++++++++++++
 3 files changed, 175 insertions(+), 0 deletions(-)
 create mode 100644 Documentation/guilt-guard.txt
 create mode 100755 guilt-guard

diff --git a/Documentation/guilt-guard.txt b/Documentation/guilt-guard.txt
new file mode 100644
index 0000000..6290bf7
--- /dev/null
+++ b/Documentation/guilt-guard.txt
@@ -0,0 +1,40 @@
+guilt-guard(1)
+===============
+
+NAME
+----
+guilt-guard - Assign guards to patches
+
+SYNOPSIS
+--------
+include::usage-guilt-guard.txt[]
+
+DESCRIPTION
+-----------
+Assign guards to the specified patch, or to the patch on top of the
+stack if no patch is given on the command line.
+
+An unguarded patch is always pushed.
+
+A positive guard begins with a +. A patch with a positive guard is
+pushed *only if* the guard is selected.
+
+A negative guard begins with a -. A patch with a negative guard is
+always pushed, *unless* the guard is selected.
+
+OPTIONS
+-------
+-l|--list::
+        List all patches and their guards
+-n|--none::
+        Remove all guards from a patch
+
+Author
+------
+Written by Eric Lesh <eclesh@ucla.edu>
+
+Documentation
+-------------
+Documentation by Eric Lesh <eclesh@ucla.edu>
+
+include::footer.txt[]
diff --git a/guilt b/guilt
index 700c167..6af590c 100755
--- a/guilt
+++ b/guilt
@@ -187,6 +187,72 @@ get_series()
 		" $series
 }
 
+get_guarded_series()
+{
+	get_series | while read p
+	do
+		check_guards "$p" && echo "$p"
+	done
+}
+
+# usage: check_guards <patch>
+# Returns 0 if the patch should be pushed
+check_guards()
+{
+	get_guards "$1" | while read guard
+	do
+		pos=`printf %s $guard | grep -e "^+"`
+		guard=`printf %s $guard | sed -e 's/^[+-]//'`
+		if [ "$pos" ]; then
+			# Push +guard *only if* guard selected
+			push=`grep -e "^$guard\$" "$guards_file" >/dev/null 2>/dev/null; echo $?`
+			[ $push -ne 0 ] && return 1
+		else
+			# Push -guard *unless* guard selected
+			push=`grep -e "^$guard\$" "$guards_file" >/dev/null 2>/dev/null; echo $?`
+			[ $push -eq 0 ] && return 1
+		fi
+		return 0
+	done
+	return $?
+}
+
+# usage: get_guards <patch>
+get_guards()
+{
+	sed -n -e "\,^$1[[:space:]]*#, {
+		s,^$1[[:space:]]*,,
+		s,#[^+-]*,,g
+
+		p
+		}
+		" $series
+}
+
+# usage: set_guards <patch> <guards...>
+set_guards()
+{
+	p="$1"
+	shift
+	for x in "$@"; do
+		if [ -z $(printf %s "$x" | grep -e "^[+-]") ]; then
+			echo "'$x' is not a valid guard name"
+		else
+			sed -i -e "s,^\($p[[:space:]]*.*\)$,\1 #$x," "$series"
+		fi
+	done
+}
+
+# usage: unset_guards <patch> <guards...>
+unset_guards()
+{
+	p="$1"
+	shift
+	for x in "$@"; do
+		sed -i -e "/^$p[[:space:]]/s/ #$x//" "$series"
+	done
+}
+
 # usage: do_make_header <hash>
 do_make_header()
 {
diff --git a/guilt-guard b/guilt-guard
new file mode 100755
index 0000000..a0cac2e
--- /dev/null
+++ b/guilt-guard
@@ -0,0 +1,69 @@
+#!/bin/sh
+#
+# Copyright (c) Eric Lesh, 2007
+#
+
+USAGE="[-l|--list|-n|--none|[<patchname>] [(+|-)<guard>...]]"
+. `dirname $0`/guilt
+
+print_guards()
+{
+	guards=`get_guards "$1"`
+	echo "$1: $guards"
+}
+
+if [ "$1" == "-l" ] || [ "$1" == "--list" ]; then
+	get_series | while read patch; do
+		print_guards "$patch"
+	done
+	exit 0
+elif [ "$1" == "-n" ] || [ "$1" == "--none" ]; then
+	patch="$2"
+	if [ -z "$patch" ]; then
+		patch=`get_top`
+	fi
+	unset_guards "$patch" `get_guards "$patch"`
+	exit 0
+fi
+
+case $# in
+	0)
+		if [ ! -s "$applied" ]; then
+			die "No patches applied."
+		fi
+		print_guards `get_top`
+		;;
+	1)
+		if [ -z $(printf %s "$1" | grep -e '^[+-]') ]; then
+			if [ -z $(get_series | grep -e "^$1\$") ]; then
+				die "Patch $1 does not exist."
+			else
+				print_guards "$1"
+			fi
+		else
+			patch=`get_top`
+			if [ -z "$patch" ]; then
+				die "You must specify a patch."
+			fi
+			unset_guards "$patch" `get_guards "$patch"`
+			set_guards "$patch" "$1"
+		fi
+		;;
+	*)
+		if [ -z $(printf %s "$1" | grep -e '^[+-]') ]; then
+			if [ -z $(get_series | grep -e "^$1\$") ]; then
+				die "Patch $1 does not exist."
+			else
+				patch="$1"
+			fi
+			shift
+		else
+			patch=`get_top`
+			if [ -z "$patch" ]; then
+				die "You must specify a patch."
+			fi
+		fi
+		unset_guards "$patch" `get_guards "$patch"`
+		set_guards "$patch" "$@"
+		;;
+esac
-- 
1.5.2

Re: [GUILT PATCH 2/5] guilt-guard: Assign guards to patches in series

[Josef Sipek]

On Mon, Jul 30, 2007 at 08:11:18PM -0700, Eric Lesh wrote:
...
> diff --git a/Documentation/guilt-guard.txt b/Documentation/guilt-guard.txt
> new file mode 100644
> index 0000000..6290bf7
> --- /dev/null
> +++ b/Documentation/guilt-guard.txt
> @@ -0,0 +1,40 @@
> +guilt-guard(1)
> +===============

Extra = ;)

> +
> +NAME
> +----
> +guilt-guard - Assign guards to patches
> +
> +SYNOPSIS
> +--------
> +include::usage-guilt-guard.txt[]
> +
> +DESCRIPTION
> +-----------
> +Assign guards to the specified patch, or to the patch on top of the
> +stack if no patch is given on the command line.
> +
> +An unguarded patch is always pushed.
> +
> +A positive guard begins with a +. A patch with a positive guard is
> +pushed *only if* the guard is selected.
> +
> +A negative guard begins with a -. A patch with a negative guard is
> +always pushed, *unless* the guard is selected.
> +
> +OPTIONS
> +-------
> +-l|--list::
> +        List all patches and their guards
> +-n|--none::
> +        Remove all guards from a patch

I generally try to keep an empty line between the options. I don't think it
affects rendering, but just for consistency.

> +
> +Author
> +------
> +Written by Eric Lesh <eclesh@ucla.edu>
> +
> +Documentation
> +-------------
> +Documentation by Eric Lesh <eclesh@ucla.edu>
> +
> +include::footer.txt[]
> diff --git a/guilt b/guilt
> index 700c167..6af590c 100755
> --- a/guilt
> +++ b/guilt
> @@ -187,6 +187,72 @@ get_series()
>  		" $series
>  }
>  
> +get_guarded_series()
> +{
> +	get_series | while read p
> +	do
> +		check_guards "$p" && echo "$p"
> +	done
> +}
> +
> +# usage: check_guards <patch>
> +# Returns 0 if the patch should be pushed
> +check_guards()
> +{
> +	get_guards "$1" | while read guard
> +	do
> +		pos=`printf %s $guard | grep -e "^+"`
> +		guard=`printf %s $guard | sed -e 's/^[+-]//'`
> +		if [ "$pos" ]; then
> +			# Push +guard *only if* guard selected
> +			push=`grep -e "^$guard\$" "$guards_file" >/dev/null 2>/dev/null; echo $?`
> +			[ $push -ne 0 ] && return 1

			grep -e "^$guard\$" "$guards_file" >/dev/null 2>/dev/null
			[ $? -ne 0 ] && return 1

Much cleaner looking :) Or even:

			grep -e ....... || return 1

> +		else
> +			# Push -guard *unless* guard selected
> +			push=`grep -e "^$guard\$" "$guards_file" >/dev/null 2>/dev/null; echo $?`
> +			[ $push -eq 0 ] && return 1

Ditto.

> +		fi
> +		return 0
> +	done
> +	return $?

I'd throw in a small comment above the outter return to make sure no one
tries to clean things up and remove it.

> +}
> +
> +# usage: get_guards <patch>
> +get_guards()
> +{
> +	sed -n -e "\,^$1[[:space:]]*#, {
> +		s,^$1[[:space:]]*,,
> +		s,#[^+-]*,,g
> +
> +		p
> +		}
> +		" $series

Three things...

1) I generally preserve the sed script indentation

2) I like having the whole script start at col 0, syntax highlighting makes
it readable enough.

3) quote the "$series" in case there's whitespace in the repo path or branch
name


This is what I'd make it look like if I were to write it, but that's me just
nitpicking at this point :)

	sed -n -e "
line 1	{
	line 2

	line 4
}
" "$series"


> +}
> +
> +# usage: set_guards <patch> <guards...>
> +set_guards()
> +{
> +	p="$1"

Again, be careful about namespace polution.

> +	shift
> +	for x in "$@"; do
> +		if [ -z $(printf %s "$x" | grep -e "^[+-]") ]; then

Out of curiosity, why printf and not echo?

> +			echo "'$x' is not a valid guard name"
> +		else
> +			sed -i -e "s,^\($p[[:space:]]*.*\)$,\1 #$x," "$series"

The regexp is in double quotes, so you should escape the $ (EOL), as well as
all the \. Yep, this is shell scripting at its worst.

> +		fi
> +	done
> +}
> +
> +# usage: unset_guards <patch> <guards...>
> +unset_guards()
> +{
> +	p="$1"

Namespace..

> +	shift
> +	for x in "$@"; do
> +		sed -i -e "/^$p[[:space:]]/s/ #$x//" "$series"
> +	done
> +}
> +
>  # usage: do_make_header <hash>
>  do_make_header()
>  {
> diff --git a/guilt-guard b/guilt-guard
> new file mode 100755
> index 0000000..a0cac2e
> --- /dev/null
> +++ b/guilt-guard
> @@ -0,0 +1,69 @@
> +#!/bin/sh
> +#
> +# Copyright (c) Eric Lesh, 2007
> +#
> +
> +USAGE="[-l|--list|-n|--none|[<patchname>] [(+|-)<guard>...]]"
> +. `dirname $0`/guilt
> +
> +print_guards()
> +{
> +	guards=`get_guards "$1"`
> +	echo "$1: $guards"

	echo "$1: `get_guards \"$1\"`"

Eliminates assignment & namespace polution :) Not really a problem here as
this is a guilt-guard only function.

> +}
> +
> +if [ "$1" == "-l" ] || [ "$1" == "--list" ]; then
> +	get_series | while read patch; do
> +		print_guards "$patch"
> +	done
> +	exit 0
> +elif [ "$1" == "-n" ] || [ "$1" == "--none" ]; then
> +	patch="$2"
> +	if [ -z "$patch" ]; then
> +		patch=`get_top`
> +	fi
> +	unset_guards "$patch" `get_guards "$patch"`
> +	exit 0
> +fi
> +
> +case $# in
> +	0)
> +		if [ ! -s "$applied" ]; then
> +			die "No patches applied."
> +		fi
> +		print_guards `get_top`

Quote the `get_top` in case the topmost patch has whitespace in it. Yeah,
it's annoying, but safety first :)

> +		;;
> +	1)
> +		if [ -z $(printf %s "$1" | grep -e '^[+-]') ]; then
> +			if [ -z $(get_series | grep -e "^$1\$") ]; then
> +				die "Patch $1 does not exist."
> +			else
> +				print_guards "$1"
> +			fi
> +		else
> +			patch=`get_top`
> +			if [ -z "$patch" ]; then
> +				die "You must specify a patch."
> +			fi
> +			unset_guards "$patch" `get_guards "$patch"`
> +			set_guards "$patch" "$1"
> +		fi
> +		;;
> +	*)
> +		if [ -z $(printf %s "$1" | grep -e '^[+-]') ]; then
> +			if [ -z $(get_series | grep -e "^$1\$") ]; then
> +				die "Patch $1 does not exist."
> +			else
> +				patch="$1"
> +			fi
> +			shift
> +		else
> +			patch=`get_top`
> +			if [ -z "$patch" ]; then
> +				die "You must specify a patch."
> +			fi
> +		fi
> +		unset_guards "$patch" `get_guards "$patch"`
> +		set_guards "$patch" "$@"
> +		;;
> +esac
> -- 
> 1.5.2

Josef 'Jeff' Sipek, falling asleep after a loooong day.

-- 
We have joy, we have fun, we have Linux on a Sun...

Re: [GUILT PATCH 2/5] guilt-guard: Assign guards to patches in series

[Eric Lesh]

[ I'm finally back to this.  Thanks for your comments. ]

Josef Sipek <jsipek@fsl.cs.sunysb.edu> writes:

[...]

>> +}
>> +
>> +# usage: set_guards <patch> <guards...>
>> +set_guards()
>> +{
>> +	p="$1"
>
> Again, be careful about namespace polution.
>

Can I use "local", or is it a bashism?  If not, use parentheses around
the function body?

>> +	shift
>> +	for x in "$@"; do
>> +		if [ -z $(printf %s "$x" | grep -e "^[+-]") ]; then
>
> Out of curiosity, why printf and not echo?
>

For guards named '-e' or other funky things echo doesn't like and can't
process with echo --.

>> +			echo "'$x' is not a valid guard name"
>> +		else
>> +			sed -i -e "s,^\($p[[:space:]]*.*\)$,\1 #$x," "$series"
>
> The regexp is in double quotes, so you should escape the $ (EOL), as well as
> all the \. Yep, this is shell scripting at its worst.
>

Yikes.

[...]

I'm trying to clean the rest and get it ready again. This whole series
will definitely need to incubate for a while once there's a
reasonable-looking version, to make sure nothing goes crazy.  Hopefully
it ends up being useful somewhere!

	Eric

Re: [GUILT PATCH 2/5] guilt-guard: Assign guards to patches in series

[David Kastrup]

Eric Lesh <eclesh@ucla.edu> writes:

> [ I'm finally back to this.  Thanks for your comments. ]
>
> Josef Sipek <jsipek@fsl.cs.sunysb.edu> writes:
>
> [...]
>
>>> +}
>>> +
>>> +# usage: set_guards <patch> <guards...>
>>> +set_guards()
>>> +{
>>> +	p="$1"
>>
>> Again, be careful about namespace polution.
>>
>
> Can I use "local", or is it a bashism?  If not, use parentheses around
> the function body?
>
>>> +	shift
>>> +	for x in "$@"; do
>>> +		if [ -z $(printf %s "$x" | grep -e "^[+-]") ]; then
>>> +			echo "'$x' is not a valid guard name"
>>> +		else
>>> +			sed -i -e "s,^\($p[[:space:]]*.*\)$,\1 #$x," "$series"
>>
>> Out of curiosity, why printf and not echo?
>>
>
> For guards named '-e' or other funky things echo doesn't like and
> can't process with echo --.

The problem with the above is that it reacts strangely to multiline
options.

Should be much better (and faster on shells without builtin printf) to
use

case "$x" in
   [+-]*)
     sed -i -e ...  ;;
       *)
     echo "'$x' is not ...
esac

and this runs portably without forking on shells that are 30 years
old.  Shell script programmers _really_ should know "case" inside out.

Also, instead of 'for x in "$@"' one can just write "for x'

>> The regexp is in double quotes, so you should escape the $ (EOL),
>> as well as all the \. Yep, this is shell scripting at its worst.

\ does not need to be escaped in double quotes except before \, $ and `.
You can write

    sed -i -e "s,^\($p[[:space:]]*.*\)\$,\1 #$x," "$series"

and that's fine.

-- 
David Kastrup

Re: [GUILT PATCH 2/5] guilt-guard: Assign guards to patches in series

[Thomas Adam]

On 09/08/07, David Kastrup <dak@gnu.org> wrote:
>     sed -i -e "s,^\($p[[:space:]]*.*\)\$,\1 #$x," "$series"
>
> and that's fine.

I'm surprised to see 'sed -i' being at all, it's certainly non-portable.

-- Thomas Adam

Re: [GUILT PATCH 2/5] guilt-guard: Assign guards to patches in series

[David Kastrup]

"Thomas Adam" <thomas.adam22@gmail.com> writes:

> On 09/08/07, David Kastrup <dak@gnu.org> wrote:
>>     sed -i -e "s,^\($p[[:space:]]*.*\)\$,\1 #$x," "$series"
>>
>> and that's fine.
>
> I'm surprised to see 'sed -i' being at all, it's certainly non-portable.

Yes.  Neither is [[:space:]].  The above actually is pretty much
equivalent to

      $(RM) "$series+"
      sed "/^$p/s/\$/ #$x/" "$series" >"$series+"
      $(MV) "$series+" "$series"

Which is probably not what was intended (the whole [[:space:]]
construct is irrelevant).  More likely it was intended to do something
like

      $(RM) "$series+"
      sed "/^$p[ 	]/s/\$/ #$x/" "$series" >"$series+"
      $(MV) "$series+" "$series"

or if $p can contain slashes but not commata,

      $(RM) "$series+"
      sed "\,^$p[ 	],s,\$, #$x," "$series" >"$series+"
      $(MV) "$series+" "$series"

Note that the included tab character will get safely from any shell to
any sed.

-- 
David Kastrup

Re: [GUILT PATCH 2/5] guilt-guard: Assign guards to patches in series

[Eric Lesh]

"Thomas Adam" <thomas.adam22@gmail.com> writes:

> On 09/08/07, David Kastrup <dak@gnu.org> wrote:
>>     sed -i -e "s,^\($p[[:space:]]*.*\)\$,\1 #$x," "$series"
>>
>> and that's fine.
>
> I'm surprised to see 'sed -i' being at all, it's certainly non-portable.
>
> -- Thomas Adam

You're definitely right. I've been asked to change this a couple of
times and keep forgetting.  Thanks for the reminder.

	Eric

Re: [GUILT PATCH 2/5] guilt-guard: Assign guards to patches in series

[Eric Lesh]

David Kastrup <dak@gnu.org> writes:

>>>> +	shift
>>>> +	for x in "$@"; do
>>>> +		if [ -z $(printf %s "$x" | grep -e "^[+-]") ]; then
>>>> +			echo "'$x' is not a valid guard name"
>>>> +		else
>>>> +			sed -i -e "s,^\($p[[:space:]]*.*\)$,\1 #$x," "$series"
>>>
>>> Out of curiosity, why printf and not echo?
>>>
>>
>> For guards named '-e' or other funky things echo doesn't like and
>> can't process with echo --.
>
> The problem with the above is that it reacts strangely to multiline
> options.
>

There shouldn't be multiline options passed to this function, so it
might not be a problem.

> Should be much better (and faster on shells without builtin printf) to
> use
>
> case "$x" in
>    [+-]*)
>      sed -i -e ...  ;;
>        *)
>      echo "'$x' is not ...
> esac
>
> and this runs portably without forking on shells that are 30 years
> old.  Shell script programmers _really_ should know "case" inside out.
>

Heh, as you may have noticed, I'm no shell programmer :-)  Thanks for
the advice though.

> Also, instead of 'for x in "$@"' one can just write "for x'
>

Nice.

>>> The regexp is in double quotes, so you should escape the $ (EOL),
>>> as well as all the \. Yep, this is shell scripting at its worst.
>
> \ does not need to be escaped in double quotes except before \, $ and `.
> You can write
>
>     sed -i -e "s,^\($p[[:space:]]*.*\)\$,\1 #$x," "$series"
>
> and that's fine.

Yeah.  That one made itself clear.  The sed -i needs to go too, as
Thomas observed.  The regexp itself also needs cleansing.

Lots of work to do...

	Eric

Re: [GUILT PATCH 2/5] guilt-guard: Assign guards to patches in series

[Josef Sipek]

On Thu, Aug 09, 2007 at 12:34:48AM -0700, Eric Lesh wrote:
> [ I'm finally back to this.  Thanks for your comments. ]

Good. I was starting to get worried :)

> Josef Sipek <jsipek@fsl.cs.sunysb.edu> writes:
> 
> [...]
> 
> >> +}
> >> +
> >> +# usage: set_guards <patch> <guards...>
> >> +set_guards()
> >> +{
> >> +	p="$1"
> >
> > Again, be careful about namespace polution.
> >
> 
> Can I use "local", or is it a bashism?  If not, use parentheses around
> the function body?

Right, "local" is a bashism therefore you must use a subshell (paretheses).

> >> +	shift
> >> +	for x in "$@"; do
> >> +		if [ -z $(printf %s "$x" | grep -e "^[+-]") ]; then
> >
> > Out of curiosity, why printf and not echo?
> >
> 
> For guards named '-e' or other funky things echo doesn't like and can't
> process with echo --.

Good enough reason :)

...
> I'm trying to clean the rest and get it ready again. This whole series
> will definitely need to incubate for a while once there's a
> reasonable-looking version, to make sure nothing goes crazy.  Hopefully
> it ends up being useful somewhere!

I'd use it at times. For certain scenarios (2 series that are mostly
identical) using guards makes more sense than different branches.

Thanks,

Josef 'Jeff' Sipek.

-- 
Humans were created by water to transport it upward.

[GUILT PATCH 3/5] guilt-select: Select guards to apply when pushing patches

[Eric Lesh]

guilt-select chooses guards that alter which patches will be applied
with a guilt-push.  The selected guards are stored in
.git/patches/$branch/guards.

Signed-off-by: Eric Lesh <eclesh@ucla.edu>
---
 Documentation/guilt-select.txt |   47 ++++++++++++++++++++++++++++++++++++
 Documentation/guilt.txt        |    5 +++-
 guilt                          |    1 +
 guilt-select                   |   52 ++++++++++++++++++++++++++++++++++++++++
 4 files changed, 104 insertions(+), 1 deletions(-)
 create mode 100644 Documentation/guilt-select.txt
 create mode 100755 guilt-select

diff --git a/Documentation/guilt-select.txt b/Documentation/guilt-select.txt
new file mode 100644
index 0000000..12f19b4
--- /dev/null
+++ b/Documentation/guilt-select.txt
@@ -0,0 +1,47 @@
+guilt-select(1)
+===============
+
+NAME
+----
+guilt-select - Select guards to apply when pushing patches
+
+SYNOPSIS
+--------
+include::usage-guilt-select.txt[]
+
+DESCRIPTION
+-----------
+Select guards to apply when pushing patches.
+
+Guards are selected without the + or - prefix.  Patches are applied in
+the following way:
+
+* An unguarded patch is always applied.
+
+* A patch with a positive guard is applied *only* if the guard is
+selected with guilt-select.
+
+* A patch with a negative guard is applied *unless* the guard is
+selected with guilt-select.
+
+OPTIONS
+-------
+-n|--none::
+        Remove all selected guards
+--pop::
+        Pop back to the first guarded patch
+--reapply::
+        Pop back to first guarded patch, select a new guard, and
+        push
+-s|--series::
+        List all guards listed in the series file
+
+Author
+------
+Written by Eric Lesh <eclesh@ucla.edu>
+
+Documentation
+-------------
+Documentation by Eric Lesh <eclesh@ucla.edu>
+
+include::footer.txt[]
diff --git a/Documentation/guilt.txt b/Documentation/guilt.txt
index 31dbc0e..11c2ca9 100644
--- a/Documentation/guilt.txt
+++ b/Documentation/guilt.txt
@@ -33,7 +33,10 @@ PATCHES DIRECTORY
 In Guilt, all the patches are stored in .git/patches/$branch/, where $branch
 is the name of the branch being worked on. This means that one can have a
 independent series of patches for each branch present in the repository.
-Each of these per-branch directories contains 2 special files:
+Each of these per-branch directories contains 3 special files:
+
+guards: This file contains any guards that should be applied to the
+series when pushing. It is only present when guards are selected.
 
 series: This file contains a list of all the patch filenames relative to the
 per-branch patch directory. Empty and commented out lines are ignored.
diff --git a/guilt b/guilt
index 6af590c..b289026 100755
--- a/guilt
+++ b/guilt
@@ -666,6 +666,7 @@ fi
 # very useful files
 series="$GUILT_DIR/$branch/series"
 applied="$GUILT_DIR/$branch/status"
+guards_file="$GUILT_DIR/$branch/guards"
 
 # determine an editor to use for anything interactive (fall back to vi)
 editor="vi"
diff --git a/guilt-select b/guilt-select
new file mode 100755
index 0000000..378ca98
--- /dev/null
+++ b/guilt-select
@@ -0,0 +1,52 @@
+#!/bin/sh
+#
+# Copyright (c) Eric Lesh, 2007
+#
+
+USAGE="[-n|--none|-s|--series|[--pop|--reapply] <guards...>]"
+. `dirname $0`/guilt
+
+select_guards()
+{
+	for x in "$@"; do
+		if [ $(printf %s "$x" | grep -e "^[+-]") ]; then
+			die "'$x' cannot begin with + or -."
+		fi
+	done
+	echo "$@" | sed -e 's/ /\n/g' | sort | uniq > "$guards_file"
+}
+
+if [ $# == 0 ]; then
+	if [ -s "$guards_file" ]; then
+		cat "$guards_file"
+	else
+		echo >&2 "No guards applied"
+	fi
+	exit 0
+fi
+
+case $1 in
+	-n|--none)
+		rm -f "$guards_file"
+		;;
+	--pop)
+		guilt-pop -a
+		shift
+		select_guards "$@"
+		;;
+	--reapply)
+		top=`get_top`
+		guilt-pop -a
+		shift
+		select_guards "$@"
+		guilt-push "$top"
+		;;
+	-s|--series)
+		(get_series | while read patch; do
+			get_guards "$patch"
+		done) | sed -e 's/ /\n/g' | sort | uniq
+		;;
+	*)
+		select_guards "$@"
+		;;
+esac
-- 
1.5.2

[GUILT PATCH 4/5] get_series: return guarded patches only

[Eric Lesh]

This renames get_guarded_series to get_series, and introduces
get_full_series for when the full series is needed.  Many guilt
scripts Just Work with that change.  Those that don't are fixed up.

With this patch, guards are respected everywhere in guilt.

Signed-off-by: Eric Lesh <eclesh@ucla.edu>
---
 guilt        |    6 +++---
 guilt-export |    2 +-
 guilt-guard  |    6 +++---
 guilt-header |    4 ++--
 guilt-push   |    4 ++--
 guilt-select |    2 +-
 guilt-series |    6 ++++--
 7 files changed, 16 insertions(+), 14 deletions(-)

diff --git a/guilt b/guilt
index b289026..d618980 100755
--- a/guilt
+++ b/guilt
@@ -171,7 +171,7 @@ get_prev()
 	fi
 }
 
-get_series()
+get_full_series()
 {
 	# ignore all lines matching:
 	#	- empty lines
@@ -187,9 +187,9 @@ get_series()
 		" $series
 }
 
-get_guarded_series()
+get_series()
 {
-	get_series | while read p
+	get_full_series | while read p
 	do
 		check_guards "$p" && echo "$p"
 	done
diff --git a/guilt-export b/guilt-export
index 2c0a9fd..c872d7f 100755
--- a/guilt-export
+++ b/guilt-export
@@ -18,7 +18,7 @@ fi
 trap "rm -rf \"$target_dir\"" 0
 mkdir -p "$target_dir"
 
-get_series | tee "$target_dir/series" | while read p; do
+get_full_series | tee "$target_dir/series" | while read p; do
 	silent mkdir -p "`dirname $target_dir/$p`" || true
 	cp "$GUILT_DIR/$branch/$p" "$target_dir/$p"
 done
diff --git a/guilt-guard b/guilt-guard
index a0cac2e..a427e25 100755
--- a/guilt-guard
+++ b/guilt-guard
@@ -13,7 +13,7 @@ print_guards()
 }
 
 if [ "$1" == "-l" ] || [ "$1" == "--list" ]; then
-	get_series | while read patch; do
+	get_full_series | while read patch; do
 		print_guards "$patch"
 	done
 	exit 0
@@ -35,7 +35,7 @@ case $# in
 		;;
 	1)
 		if [ -z $(printf %s "$1" | grep -e '^[+-]') ]; then
-			if [ -z $(get_series | grep -e "^$1\$") ]; then
+			if [ -z $(get_full_series | grep -e "^$1\$") ]; then
 				die "Patch $1 does not exist."
 			else
 				print_guards "$1"
@@ -51,7 +51,7 @@ case $# in
 		;;
 	*)
 		if [ -z $(printf %s "$1" | grep -e '^[+-]') ]; then
-			if [ -z $(get_series | grep -e "^$1\$") ]; then
+			if [ -z $(get_full_series | grep -e "^$1\$") ]; then
 				die "Patch $1 does not exist."
 			else
 				patch="$1"
diff --git a/guilt-header b/guilt-header
index 5716265..540cf2a 100755
--- a/guilt-header
+++ b/guilt-header
@@ -31,8 +31,8 @@ esac
 [ -z "$patch" ] && die "No patches applied."
 
 # check that patch exists in the series
-ret=`get_series | grep -ne "^$patch\$" | cut -d: -f1`
-if [ $ret -eq 0 ]; then
+ret=`get_full_series | grep -ne "^$patch\$" | cut -d: -f1`
+if [ -z "$ret" ]; then
 	die "Patch $patch is not in the series"
 fi
 
diff --git a/guilt-push b/guilt-push
index ad3616b..e4004e0 100755
--- a/guilt-push
+++ b/guilt-push
@@ -26,7 +26,7 @@ if [ "$patch" = "--all" ] || [ "$patch" = "-a" ]; then
 
 	eidx=`get_series | wc -l`
 	if [ $eidx -eq 0 ]; then
-		die "There are no patches to push"
+		die "There are no patches to push."
 	fi
 elif [ -z "$patch" ]; then
 	# we are supposed to push only the next patch onto the stack
@@ -39,7 +39,7 @@ else
 
 	eidx=`get_series | grep -ne "^$patch\$" | cut -d: -f1`
 	if [ -z "$eidx" ]; then
-		die "Patch $patch is not in the series"
+		die "Patch $patch is not in the series or is guarded."
 	fi
 fi
 
diff --git a/guilt-select b/guilt-select
index 378ca98..57373c7 100755
--- a/guilt-select
+++ b/guilt-select
@@ -42,7 +42,7 @@ case $1 in
 		guilt-push "$top"
 		;;
 	-s|--series)
-		(get_series | while read patch; do
+		(get_full_series | while read patch; do
 			get_guards "$patch"
 		done) | sed -e 's/ /\n/g' | sort | uniq
 		;;
diff --git a/guilt-series b/guilt-series
index 9c34a08..85ef15d 100755
--- a/guilt-series
+++ b/guilt-series
@@ -36,12 +36,12 @@ elif [ ! -z "$gui" ]; then
 
 	gitk $range
 elif [ -z "$verbose" ]; then
-	get_series
+	get_full_series
 else
 	prefix="+"
 	top=`get_top`
 
-	get_series |
+	get_full_series |
 	while read patch; do
 		if [ -z "$top" ]; then
 			echo "  $patch"
@@ -49,6 +49,8 @@ else
 			if [ "$patch" = "$top" ]; then
 				echo "= $patch"
 				prefix=" "
+                        elif [ $(check_guards "$patch"; echo $?) -eq 1 ]; then
+				echo "  $patch"
 			else
 				echo "$prefix $patch"
 			fi
-- 
1.5.2

[GUILT PATCH 5/5] Guards test suite

[Eric Lesh]

The guards patches touch a lot of guilt, so make sure they don't muck
anything up.

Signed-off-by: Eric Lesh <eclesh@ucla.edu>
---
 regression/070-guards.sh |  184 ++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 184 insertions(+), 0 deletions(-)
 create mode 100755 regression/070-guards.sh

diff --git a/regression/070-guards.sh b/regression/070-guards.sh
new file mode 100755
index 0000000..d6917f2
--- /dev/null
+++ b/regression/070-guards.sh
@@ -0,0 +1,184 @@
+#!/bin/bash
+#
+# Test the commands that use get_*_series, while applying guards
+#
+
+source scaffold
+source generic_test_data
+
+function prepare_for_tests
+{
+	# generic_test_data's patches all depend on each other
+	# that's no good for guards testing
+
+	echo "abc" > def
+	git-add def
+	git-commit -s -m "initial" 2> /dev/null > /dev/null
+
+	cat << DONE > .git/patches/master/first
+diff --git a/first b/first
+new file mode 100644
+index 0000000..9c59e24
+--- /dev/null
++++ b/first
+@@ -0,0 +1 @@
++first
+DONE
+
+	cat << DONE > .git/patches/master/second
+diff --git a/second b/second
+new file mode 100644
+index 0000000..e019be0
+--- /dev/null
++++ b/second
+@@ -0,0 +1 @@
++second
+DONE
+
+	cat << DONE > .git/patches/master/third
+diff --git a/third b/third
+new file mode 100644
+index 0000000..234496b
+--- /dev/null
++++ b/third
+@@ -0,0 +1 @@
++third
+DONE
+
+	cat << DONE > .git/patches/master/fourth
+diff --git a/fourth b/fourth
+new file mode 100644
+index 0000000..285a4e6
+--- /dev/null
++++ b/fourth
+@@ -0,0 +1 @@
++fourth
+DONE
+
+	cat << DONE > .git/patches/master/series
+first
+second
+third
+fourth
+DONE
+
+	touch -d "$GIT_COMMITTER_DATE" .git/patches/master/first
+	touch -d "$GIT_COMMITTER_DATE" .git/patches/master/second
+	touch -d "$GIT_COMMITTER_DATE" .git/patches/master/third
+	touch -d "$GIT_COMMITTER_DATE" .git/patches/master/fourth
+}
+
+function expected_applied_none
+{
+	echo "first"
+	echo "second"
+	echo "third"
+	echo "fourth"
+}
+
+function expected_applied_positive
+{
+	echo "second"
+	echo "third"
+	echo "fourth"
+}
+
+function expected_applied_positive_selected
+{
+	echo "first"
+	echo "second"
+	echo "third"
+	echo "fourth"
+}
+
+function expected_applied_negative
+{
+	echo "first"
+	echo "second"
+	echo "third"
+	echo "fourth"
+}
+
+function expected_applied_negative_selected
+{
+	echo "second"
+	echo "third"
+	echo "fourth"
+}
+
+function expected_next
+{
+	echo "second"
+}
+
+function expected_unapplied
+{
+	echo "second"
+	echo "third"
+	echo "fourth"
+}
+
+empty_repo
+cd $REPODIR
+guilt-init
+
+prepare_for_tests
+
+# test with no guarded patches and no guards selected
+guilt-push -a > /dev/null
+guilt-applied > /tmp/reg.$$
+expected_applied_none | diff -u - /tmp/reg.$$
+echo -n "[none] "
+
+# test with one positive guarded patch and no guards selected
+guilt-pop -a > /dev/null
+guilt-guard first +foo
+guilt-push -a > /dev/null
+guilt-applied > /tmp/reg.$$
+expected_applied_positive | diff -u - /tmp/reg.$$
+echo -n "[positive] "
+
+# test with one positive guarded patch with that guard selected
+guilt-pop -a > /dev/null
+guilt-select foo
+guilt-push -a > /dev/null
+guilt-applied > /tmp/reg.$$
+expected_applied_positive_selected | diff -u - /tmp/reg.$$
+echo -n "[positive selected] "
+
+# test with one negative guarded patch and no guards selected
+guilt pop -a > /dev/null
+guilt-select -n
+guilt-guard first -foo
+guilt-push -a > /dev/null
+guilt-applied > /tmp/reg.$$
+expected_applied_negative | diff -u - /tmp/reg.$$
+echo -n "[negative] "
+
+# test with one negative guarded patch with that guard selected
+guilt pop -a > /dev/null
+guilt-select foo
+guilt-push -a > /dev/null
+guilt-applied > /tmp/reg.$$
+expected_applied_negative_selected | diff -u - /tmp/reg.$$
+echo -n "[negative selected] "
+
+# test that guilt-next works
+guilt-pop -a > /dev/null
+guilt-select -n
+guilt-guard first +foo
+guilt-next > /tmp/reg.$$
+expected_next | diff -u - /tmp/reg.$$
+echo -n "[next] "
+
+# test that guilt-unapplied works
+guilt-pop -a > /dev/null
+guilt-select -n
+guilt-guard first +foo
+guilt-unapplied > /tmp/reg.$$
+expected_unapplied | diff -u - /tmp/reg.$$
+echo -n "[unapplied] "
+
+rm -f /tmp/reg.$$
+
+complete_test
-- 
1.5.2

Re: [GUILT PATCH v2 0/5] Add guards to guilt

[Josef Sipek]

On Mon, Jul 30, 2007 at 08:11:16PM -0700, Eric Lesh wrote:
...
> I did everything (maybe?) you suggested except implement a
> safe_sed wrapper and a validate_guards function.  The sed stuff is
> maybe better for a later patch that covers most sed usage in guilt,

Agreed.

> and the validate_guards isn't really necessary given the small number
> of times it's called.  If you would prefer either of those to get
> done, let me know.

I'll look at the patches and complain there if it seems like something
that should be done. :)

Josef 'Jeff' Sipek.

-- 
I already backed up the [server] once, I can do it again.