[GUILT PATCH 0/4] Add guards to guilt

[GUILT PATCH 0/4] Add guards to guilt

[Eric Lesh]

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 introduces the command `get_guarded_series`, which just lists
patches that are to be applied based on the guards.  It also makes
        eidx=`wc -l < $applied`
inaccurate if you're using it as an index into get_series.

If you change guards on a patch or select a different guard while
patches are applied, some commands might get confused. guilt pop -a will fix
everything though.  Usually, it's best to pop -a before fiddling with
guards anyway.

This is an RFC, but I have tested it and things seem to be working
well.

[PATCH 1/4] 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/4] guilt-guard: Assign guards to patches in series
       This adds the guilt-guard command and utility functions to
       guilt.

[PATCH 3/4] 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/4] Use guards information and functions
       This changes guilt-header, guilt-next, guilt-push and guilt-unapplied to
       use the guards information properly.

       After the guilt-push change, header, next, and unapplied get
       confused and break, so I rolled their fixes into this patch
       instead of separate ones.

 Documentation/guilt-guards.txt |   40 +++++++++++++++++++++++++
 Documentation/guilt-select.txt |   42 ++++++++++++++++++++++++++
 guilt                          |   62 ++++++++++++++++++++++++++++++++++++++-
 guilt-guards                   |   63 ++++++++++++++++++++++++++++++++++++++++
 guilt-header                   |    7 ++--
 guilt-next                     |    2 +-
 guilt-push                     |    8 ++--
 guilt-select                   |   36 +++++++++++++++++++++++
 guilt-unapplied                |    2 +-
 9 files changed, 252 insertions(+), 10 deletions(-)
 create mode 100644 Documentation/guilt-guards.txt
 create mode 100644 Documentation/guilt-select.txt
 create mode 100755 guilt-guards
 create mode 100755 guilt-select

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

[Eric Lesh]

Signed-off-by: Eric Lesh <eclesh@ucla.edu>

--
---
 guilt |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/guilt b/guilt
index f67bfb5..774909e 100755
--- a/guilt
+++ b/guilt
@@ -178,7 +178,8 @@ get_series()
 	#	- whitespace only
 	#	- optional whitespace followed by '#' followed by more
 	#	  optional whitespace
-	grep -ve '^[[:space:]]*\(#.*\)*$' "$series"
+	# also remove comments from end of lines
+	grep -ve '^[[:space:]]*\(#.*\)*$' < "$series" | sed -e 's/[[:space:]]*#.*$//'
 }
 
 # usage: do_make_header <hash>
-- 
1.5.2

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

[Josef Sipek]

On Sun, Jul 29, 2007 at 12:50:15AM -0700, Eric Lesh wrote:
... 
> diff --git a/guilt b/guilt
> index f67bfb5..774909e 100755
> --- a/guilt
> +++ b/guilt
> @@ -178,7 +178,8 @@ get_series()
>  	#	- whitespace only
>  	#	- optional whitespace followed by '#' followed by more
>  	#	  optional whitespace
> -	grep -ve '^[[:space:]]*\(#.*\)*$' "$series"
> +	# also remove comments from end of lines
> +	grep -ve '^[[:space:]]*\(#.*\)*$' < "$series" | sed -e 's/[[:space:]]*#.*$//'

I'd be tempted to replace the whole thing with one sed script...something
like (not tested):

"
/^[[:space:]]*#/ ! {
	s/[[:space:]]*#.*$//

	p
}
"

Regardless of the other 3 patches, this one makes sense to include.

Josef 'Jeff' Sipek.

-- 
Once you have their hardware. Never give it back.
(The First Rule of Hardware Acquisition)

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

[Eric Lesh]

[ Do you mind if these messages go to both your email addresses, or
should I remove one or the other? ]

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

> On Sun, Jul 29, 2007 at 12:50:15AM -0700, Eric Lesh wrote:
> ... 
>> diff --git a/guilt b/guilt
>> index f67bfb5..774909e 100755
>> --- a/guilt
>> +++ b/guilt
>> @@ -178,7 +178,8 @@ get_series()
>>  	#	- whitespace only
>>  	#	- optional whitespace followed by '#' followed by more
>>  	#	  optional whitespace
>> -	grep -ve '^[[:space:]]*\(#.*\)*$' "$series"
>> +	# also remove comments from end of lines
>> +	grep -ve '^[[:space:]]*\(#.*\)*$' < "$series" | sed -e 's/[[:space:]]*#.*$//'
>
> I'd be tempted to replace the whole thing with one sed script...something
> like (not tested):
>
> "
> /^[[:space:]]*#/ ! {
> 	s/[[:space:]]*#.*$//
>
> 	p
> }
> "
>

sed -e "/^[[:space:]]*\(#.*\)*$/d
	/^[[:space:]]*\(#.*\)*$/!{
	s/[[:space:]]*#.*$//
	}
	" $series

is the best I can do.

sed -e "/^[[:space:]]*\(#.*\)*$/d" -e "s/[[:space:]]*#.*$//" $series

works too, and is maybe more readable.

My sed-foo is weak, though.

	Eric

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

[Josef Sipek]

On Sun, Jul 29, 2007 at 10:15:54PM -0700, Eric Lesh wrote:
> 
> [ Do you mind if these messages go to both your email addresses, or
> should I remove one or the other? ]
 
I really don't care if I get duplicates. It'll happen anyway when the git
mailing list is cc'd. As for which address I prefer, it really doesn't
matter to me which one stays in Cc. I'm slowly trying to move everything
over to @cs.

> Josef Sipek <jsipek@fsl.cs.sunysb.edu> writes:
> 
> > On Sun, Jul 29, 2007 at 12:50:15AM -0700, Eric Lesh wrote:
> > ... 
> >> diff --git a/guilt b/guilt
> >> index f67bfb5..774909e 100755
> >> --- a/guilt
> >> +++ b/guilt
> >> @@ -178,7 +178,8 @@ get_series()
> >>  	#	- whitespace only
> >>  	#	- optional whitespace followed by '#' followed by more
> >>  	#	  optional whitespace
> >> -	grep -ve '^[[:space:]]*\(#.*\)*$' "$series"
> >> +	# also remove comments from end of lines
> >> +	grep -ve '^[[:space:]]*\(#.*\)*$' < "$series" | sed -e 's/[[:space:]]*#.*$//'
> >
> > I'd be tempted to replace the whole thing with one sed script...something
> > like (not tested):
> >
> > "
> > /^[[:space:]]*#/ ! {
> > 	s/[[:space:]]*#.*$//
> >
> > 	p
> > }
> > "
> >
> 
> sed -e "/^[[:space:]]*\(#.*\)*$/d
> 	/^[[:space:]]*\(#.*\)*$/!{
> 	s/[[:space:]]*#.*$//
> 	}
> 	" $series
> 
> is the best I can do.

I think the script I wrote is a bit cleaner as it more easily translates to:

if (!ignore_line) {
	strip comment
	print
}

to make it work, you'd need to run sed with -n to not implicitly print the
line.

Jeff.

-- 
I'm somewhere between geek and normal.
		- Linus Torvalds

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

[Eric Lesh]

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

>
> I think the script I wrote is a bit cleaner as it more easily translates to:
>
> if (!ignore_line) {
> 	strip comment
> 	print
> }
>
> to make it work, you'd need to run sed with -n to not implicitly print the
> line.
>

Told you my sed-foo sucks ;-)  You're right again, of course.

	Eric

[GUILT PATCH 2/4] 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.

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

diff --git a/Documentation/guilt-guards.txt b/Documentation/guilt-guards.txt
new file mode 100644
index 0000000..f5ac537
--- /dev/null
+++ b/Documentation/guilt-guards.txt
@@ -0,0 +1,40 @@
+guilt-guards(1)
+===============
+
+NAME
+----
+guilt-guards - Assign guards to patches
+
+SYNOPSIS
+--------
+include::usage-guilt-guards.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 774909e..b2767ea 100755
--- a/guilt
+++ b/guilt
@@ -182,6 +182,64 @@ get_series()
 	grep -ve '^[[:space:]]*\(#.*\)*$' < "$series" | sed -e 's/[[:space:]]*#.*$//'
 }
 
+get_guarded_series()
+{
+	get_series | while read p
+	do
+		[ -z `check_guards $p` ] && echo "$p"
+	done
+}
+
+# usage: check_guards <patch>
+# Returns t if the patch should be skipped
+check_guards()
+{
+        get_guards "$1" | while read guard
+        do
+                pos=`echo $guard | grep -e "^+"`
+                guard=`echo $guard | sed -e 's/[+-]//'`
+                if [ $pos ]; then
+                        # Push +guard *only if* guard selected
+                        push=`grep -e "^$guard\$" "$guards_file" > /dev/null; echo $?`
+                        [ $push -ne 0 ] && echo t
+                else
+                        # Push -guard *unless* guard selected
+                        push=`grep -e "^$guard\$" "$guards_file" > /dev/null; echo $?`
+                        [ $push -eq 0 ] && echo t
+                fi
+        done
+}
+
+# usage: get_guards <patch>
+get_guards()
+{
+	grep -e "^$1[[:space:]]*#" < "$series" | sed -e "s/^$1 //" -e 's/#[^+-]*//g'
+}
+
+# usage: set_guards <patch> <guards>
+set_guards()
+{
+	p="$1"
+	shift
+	for x in "$@"; do
+		if [ -z $(echo "$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-guards b/guilt-guards
new file mode 100755
index 0000000..71df4f8
--- /dev/null
+++ b/guilt-guards
@@ -0,0 +1,63 @@
+#!/bin/sh
+#
+# Copyright (c) Eric Lesh, 2007
+#
+
+USAGE="[-l|--list] [-n|--none] [<patchname>] [+<guard>] [-<guard>]"
+. 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 $(echo $1 | grep -e '^[+-]') ]; then
+			if [ -z $(get_series | grep -e "^$1\$") ]; then
+				die "Patch $1 does not exist"
+			else
+				print_guards "$1"
+			fi
+		else
+			p=`get_top`
+			unset_guards "$p" `get_guards "$p"`
+			set_guards "$p" "$1"
+		fi
+		;;
+	*)
+		if [ -z $(echo $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`
+		fi
+		unset_guards "$patch" `get_guards "$patch"`
+		set_guards "$patch" "$@"
+		;;
+esac
-- 
1.5.2

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

[Josef Sipek]

On Sun, Jul 29, 2007 at 12:50:16AM -0700, Eric Lesh wrote:
> 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.
> 
> Signed-off-by: Eric Lesh <eclesh@ucla.edu>
> ---
>  Documentation/guilt-guards.txt |   40 +++++++++++++++++++++++++
>  guilt                          |   58 ++++++++++++++++++++++++++++++++++++
>  guilt-guards                   |   63 ++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 161 insertions(+), 0 deletions(-)
>  create mode 100644 Documentation/guilt-guards.txt
>  create mode 100755 guilt-guards
> 
> diff --git a/Documentation/guilt-guards.txt b/Documentation/guilt-guards.txt
> new file mode 100644
> index 0000000..f5ac537
> --- /dev/null
> +++ b/Documentation/guilt-guards.txt
> @@ -0,0 +1,40 @@
> +guilt-guards(1)
> +===============
> +
> +NAME
> +----
> +guilt-guards - Assign guards to patches
> +
> +SYNOPSIS
> +--------
> +include::usage-guilt-guards.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 774909e..b2767ea 100755
> --- a/guilt
> +++ b/guilt
> @@ -182,6 +182,64 @@ get_series()
>  	grep -ve '^[[:space:]]*\(#.*\)*$' < "$series" | sed -e 's/[[:space:]]*#.*$//'
>  }
>  
> +get_guarded_series()
> +{
> +	get_series | while read p
> +	do
> +		[ -z `check_guards $p` ] && echo "$p"

Having check_guards return 0 or 1 makes things cleaner:

check_guards "$p" && echo "$p"

> +	done
> +}
> +
> +# usage: check_guards <patch>
> +# Returns t if the patch should be skipped
> +check_guards()
> +{
> +        get_guards "$1" | while read guard
> +        do
> +                pos=`echo $guard | grep -e "^+"`
> +                guard=`echo $guard | sed -e 's/[+-]//'`
> +                if [ $pos ]; then
> +                        # Push +guard *only if* guard selected
> +                        push=`grep -e "^$guard\$" "$guards_file" > /dev/null; echo $?`
> +                        [ $push -ne 0 ] && echo t

			   [ $push -ne 0 ] && return 1

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

ditto

> +                fi
> +        done

	return 0
> +}
> +
> +# usage: get_guards <patch>
> +get_guards()
> +{
> +	grep -e "^$1[[:space:]]*#" < "$series" | sed -e "s/^$1 //" -e 's/#[^+-]*//g'
> +}
> +
> +# usage: set_guards <patch> <guards>

I'd try to make it clearer that multiple guards can be specified.

> +set_guards()
> +{
> +	p="$1"
> +	shift
> +	for x in "$@"; do
> +		if [ -z $(echo "$x" | grep -e "^[+-]") ]; then

Is that the only restriction on the guard name?

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

ditto.

> +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-guards b/guilt-guards
> new file mode 100755
> index 0000000..71df4f8
> --- /dev/null
> +++ b/guilt-guards
> @@ -0,0 +1,63 @@
> +#!/bin/sh
> +#
> +# Copyright (c) Eric Lesh, 2007
> +#
> +
> +USAGE="[-l|--list] [-n|--none] [<patchname>] [+<guard>] [-<guard>]"

Since -l and -n are mutually exclusive, shouldn't it be something like:

[-l|--list|-n|--none|[<patchname>] [(+|-)guard...]]

> +. 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 $(echo $1 | grep -e '^[+-]') ]; then
> +			if [ -z $(get_series | grep -e "^$1\$") ]; then
> +				die "Patch $1 does not exist"
> +			else
> +				print_guards "$1"
> +			fi
> +		else
> +			p=`get_top`
> +			unset_guards "$p" `get_guards "$p"`
> +			set_guards "$p" "$1"
> +		fi
> +		;;
> +	*)
> +		if [ -z $(echo $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`
> +		fi
> +		unset_guards "$patch" `get_guards "$patch"`
> +		set_guards "$patch" "$@"
> +		;;
> +esac
> -- 
> 1.5.2

-- 
Note 96.3% of all statistics are fiction.

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

[Eric Lesh]

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

[...]

>> +get_guarded_series()
>> +{
>> +	get_series | while read p
>> +	do
>> +		[ -z `check_guards $p` ] && echo "$p"
>
> Having check_guards return 0 or 1 makes things cleaner:
>
> check_guards "$p" && echo "$p"
>
>> +	done
>> +}
>> +
>> +# usage: check_guards <patch>
>> +# Returns t if the patch should be skipped
>> +check_guards()
>> +{
>> +        get_guards "$1" | while read guard
>> +        do
>> +                pos=`echo $guard | grep -e "^+"`
>> +                guard=`echo $guard | sed -e 's/[+-]//'`
>> +                if [ $pos ]; then
>> +                        # Push +guard *only if* guard selected
>> +                        push=`grep -e "^$guard\$" "$guards_file" > /dev/null; echo $?`
>> +                        [ $push -ne 0 ] && echo t
>
> 			   [ $push -ne 0 ] && return 1
>

This returns from the subshell created by the pipe and the while loop,
right?

So I'm using:

check_guards()
{
	get_guards "$1" | while read guard
	do
		pos=`echo $guard | grep -e "^+"`
		guard=`echo $guard | sed -e 's/^[+-]//'`
		if [ $pos ]; then
			# Push +guard *only if* guard selected
			push=`grep -e "^$guard\$" "$guards_file" > /dev/null; echo $?`
			[ $push -ne 0 ] && return 1
		else
			# Push -guard *unless* guard selected
			push=`grep -e "^$guard\$" "$guards_file" > /dev/null; echo $?`
			[ $push -eq 0 ] && return 1
		fi
                return 0
	done
	return $?
}

where 1 means push.

>> +# usage: get_guards <patch>
>> +get_guards()
>> +{
>> +	grep -e "^$1[[:space:]]*#" < "$series" | sed -e "s/^$1 //" -e 's/#[^+-]*//g'
>> +}

Should this also be one sed script instead of a grep + sed?

>> +
>> +# usage: set_guards <patch> <guards>
>
> I'd try to make it clearer that multiple guards can be specified.
>

Done with <guards...> now.

>> +set_guards()
>> +{
>> +	p="$1"
>> +	shift
>> +	for x in "$@"; do
>> +		if [ -z $(echo "$x" | grep -e "^[+-]") ]; then
>
> Is that the only restriction on the guard name?
>

Yes.  On patches, you put a '+guard' or '-guard'.  When selecting with
guilt-select, it's just 'guard'.  The + or - just means 'apply when
selected' or 'apply unless selected'.  You can edit things manually to
make guards with a space in the name, but the mechanism will work even
in that case.

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

[...]

The rest I'll do.  Thanks for the review.

	Eric

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

[Josef Sipek]

On Sun, Jul 29, 2007 at 11:41:52PM -0700, Eric Lesh wrote:
> Josef Sipek <jsipek@fsl.cs.sunysb.edu> writes:
> 
> [...]
> 
> >> +get_guarded_series()
> >> +{
> >> +	get_series | while read p
> >> +	do
> >> +		[ -z `check_guards $p` ] && echo "$p"
> >
> > Having check_guards return 0 or 1 makes things cleaner:
> >
> > check_guards "$p" && echo "$p"
> >
> >> +	done
> >> +}
> >> +
> >> +# usage: check_guards <patch>
> >> +# Returns t if the patch should be skipped
> >> +check_guards()
> >> +{
> >> +        get_guards "$1" | while read guard
> >> +        do
> >> +                pos=`echo $guard | grep -e "^+"`
> >> +                guard=`echo $guard | sed -e 's/[+-]//'`
> >> +                if [ $pos ]; then
> >> +                        # Push +guard *only if* guard selected
> >> +                        push=`grep -e "^$guard\$" "$guards_file" > /dev/null; echo $?`
> >> +                        [ $push -ne 0 ] && echo t
> >
> > 			   [ $push -ne 0 ] && return 1
> >
> 
> This returns from the subshell created by the pipe and the while loop,
> right?

Right, sorry.

> So I'm using:
> 
> check_guards()
> {
> 	get_guards "$1" | while read guard
> 	do
> 		pos=`echo $guard | grep -e "^+"`
> 		guard=`echo $guard | sed -e 's/^[+-]//'`
> 		if [ $pos ]; then
> 			# Push +guard *only if* guard selected
> 			push=`grep -e "^$guard\$" "$guards_file" > /dev/null; echo $?`
> 			[ $push -ne 0 ] && return 1
> 		else
> 			# Push -guard *unless* guard selected
> 			push=`grep -e "^$guard\$" "$guards_file" > /dev/null; echo $?`
> 			[ $push -eq 0 ] && return 1
> 		fi
>                 return 0

Beware of whitespace :)

> 	done
> 	return $?
> }
> 
> where 1 means push.
> 
> >> +# usage: get_guards <patch>
> >> +get_guards()
> >> +{
> >> +	grep -e "^$1[[:space:]]*#" < "$series" | sed -e "s/^$1 //" -e 's/#[^+-]*//g'
> >> +}
> 
> Should this also be one sed script instead of a grep + sed?

I'm all for more complex sed/awk scripts to replace lots of forks and pipes.

> >> +
> >> +# usage: set_guards <patch> <guards>
> >
> > I'd try to make it clearer that multiple guards can be specified.
> >
> 
> Done with <guards...> now.
> 
> >> +set_guards()
> >> +{
> >> +	p="$1"
> >> +	shift
> >> +	for x in "$@"; do
> >> +		if [ -z $(echo "$x" | grep -e "^[+-]") ]; then
> >
> > Is that the only restriction on the guard name?
> >
> 
> Yes.  On patches, you put a '+guard' or '-guard'.  When selecting with
> guilt-select, it's just 'guard'.  The + or - just means 'apply when
> selected' or 'apply unless selected'.  You can edit things manually to
> make guards with a space in the name, but the mechanism will work even
> in that case.

I am thinking that it _might_ make sense to have some validate_guard_name
function - I am not sure if it would be used enough to make it useful
instead of just obfuscating the code.

> >> +			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>
> >
> 
> [...]
> 
> The rest I'll do.  Thanks for the review.

Thanks for the patches :)

Jeff.

-- 
The obvious mathematical breakthrough would be development of an easy way to
factor large prime numbers.
		- Bill Gates, The Road Ahead, pg. 265

[GUILT PATCH 3/4] 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 |   42 ++++++++++++++++++++++++++++++++++++++++
 guilt                          |    1 +
 guilt-select                   |   36 ++++++++++++++++++++++++++++++++++
 3 files changed, 79 insertions(+), 0 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..8e18f26
--- /dev/null
+++ b/Documentation/guilt-select.txt
@@ -0,0 +1,42 @@
+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
+-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/guilt b/guilt
index b2767ea..3882962 100755
--- a/guilt
+++ b/guilt
@@ -641,6 +641,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..f237ef0
--- /dev/null
+++ b/guilt-select
@@ -0,0 +1,36 @@
+#!/bin/sh
+#
+# Copyright (c) Eric Lesh, 2007
+#
+
+USAGE="[-n|--none] [-s|--series] [<guard>]"
+ . `dirname $0`/guilt
+
+if [ $# == 0 ]; then
+	if [ -s "$guards_file" ]; then
+		cat "$guards_file"
+	else
+		echo "No guards applied"
+	fi
+	exit 0
+fi
+
+case $1 in
+	-n|--none)
+		rm -f "$guards_file"
+		touch "$guards_file"
+		;;
+	-s|--series)
+		(get_series | while read patch; do
+			get_guards "$patch"
+		done) | sed -e 's/ /\n/g' | sort | uniq
+		;;
+	*)
+		for x in "$@"; do
+			if [ $(echo $x | grep -e "^[+-]") ]; then
+				die "'$x' is not a valid guard name"
+			fi
+		done
+		echo "$@" | sed -e 's/ /\n/g' | sort | uniq > "$guards_file"
+		;;
+esac
-- 
1.5.2

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

[Josef Sipek]

On Sun, Jul 29, 2007 at 12:50:17AM -0700, Eric Lesh wrote:
> 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 |   42 ++++++++++++++++++++++++++++++++++++++++
>  guilt                          |    1 +
>  guilt-select                   |   36 ++++++++++++++++++++++++++++++++++
>  3 files changed, 79 insertions(+), 0 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..8e18f26
> --- /dev/null
> +++ b/Documentation/guilt-select.txt

You might want to update the guilt(7) page's description of the patch
directory.

> @@ -0,0 +1,42 @@
> +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
> +-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/guilt b/guilt
> index b2767ea..3882962 100755
> --- a/guilt
> +++ b/guilt
> @@ -641,6 +641,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..f237ef0
> --- /dev/null
> +++ b/guilt-select
> @@ -0,0 +1,36 @@
> +#!/bin/sh
> +#
> +# Copyright (c) Eric Lesh, 2007
> +#
> +
> +USAGE="[-n|--none] [-s|--series] [<guard>]"
> + . `dirname $0`/guilt
> +
> +if [ $# == 0 ]; then
> +	if [ -s "$guards_file" ]; then
> +		cat "$guards_file"

Later on, for the -s option processing, you sort (presumably to have uniq do
the right thing), should we sort here too to be consitent?

> +	else
> +		echo "No guards applied"

I think outputing this message to stderr might be better; it'll allow for
redirection more easily.

> +	fi
> +	exit 0
> +fi
> +
> +case $1 in
> +	-n|--none)
> +		rm -f "$guards_file"
> +		touch "$guards_file"

Since guilt-init doesn't create the guards file, I'm thinking that this
should be just a rm -f ...

> +		;;
> +	-s|--series)
> +		(get_series | while read patch; do
> +			get_guards "$patch"
> +		done) | sed -e 's/ /\n/g' | sort | uniq
> +		;;
> +	*)
> +		for x in "$@"; do
> +			if [ $(echo $x | grep -e "^[+-]") ]; then
> +				die "'$x' is not a valid guard name"
> +			fi
> +		done
> +		echo "$@" | sed -e 's/ /\n/g' | sort | uniq > "$guards_file"
> +		;;
> +esac
> -- 
> 1.5.2

-- 
The box said "Windows XP or better required". So I installed Linux.

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

[Eric Lesh]

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

[...]

>> +if [ $# == 0 ]; then
>> +	if [ -s "$guards_file" ]; then
>> +		cat "$guards_file"
>
> Later on, for the -s option processing, you sort (presumably to have uniq do
> the right thing), should we sort here too to be consitent?
>

The $guards_file isn't really meant to be handed edited, and
guilt-select itself sorts before it stores them in the guards file.  I could
sort it again on printing, but don't think it's necessary.

>> +
>> +case $1 in
>> +	-n|--none)
>> +		rm -f "$guards_file"
>> +		touch "$guards_file"
>
> Since guilt-init doesn't create the guards file, I'm thinking that this
> should be just a rm -f ...

Should guilt-init create it?  I added $guards_file to guilt(7), so not
seeing it might freak Documentation-conscious readers out?

	Eric

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

[Josef Sipek]

On Mon, Jul 30, 2007 at 12:02:26AM -0700, Eric Lesh wrote:
> Josef Sipek <jsipek@fsl.cs.sunysb.edu> writes:
> 
> [...]
> 
> >> +if [ $# == 0 ]; then
> >> +	if [ -s "$guards_file" ]; then
> >> +		cat "$guards_file"
> >
> > Later on, for the -s option processing, you sort (presumably to have uniq do
> > the right thing), should we sort here too to be consitent?
> >
> 
> The $guards_file isn't really meant to be handed edited, and
> guilt-select itself sorts before it stores them in the guards file.  I could
> sort it again on printing, but don't think it's necessary.
 
Duh. No need to re-sort.

> >> +
> >> +case $1 in
> >> +	-n|--none)
> >> +		rm -f "$guards_file"
> >> +		touch "$guards_file"
> >
> > Since guilt-init doesn't create the guards file, I'm thinking that this
> > should be just a rm -f ...
> 
> Should guilt-init create it?  I added $guards_file to guilt(7), so not
> seeing it might freak Documentation-conscious readers out?

I'm thinking that it would be nice to have the file created when the first
guard is set, and removed when the last guard is removed. This way, if you
don't care about guards, you don't have to ignore the file (if you have your
patches dir under version control). This also happens to be the nicer way to
transition from pre-guard patch dirs to ones with guards - in a way it's
cheating around "upgrading" the repo :)

I'd like this lazy creation to be documented, of course to not confuse the
handful that actually read the docs :)

Jeff.

-- 
Linux, n.:
  Generous programmers from around the world all join forces to help
  you shoot yourself in the foot for free. 

[GUILT PATCH 4/4] Use guards information and functions

[Eric Lesh]

Make guilt-push respect guards. Also teach guilt-header, guilt-next,
and guilt-unapplied to grok patches that are skipped because of
guards.

Signed-off-by: Eric Lesh <eclesh@ucla.edu>
---
 guilt-header    |    7 ++++---
 guilt-next      |    2 +-
 guilt-push      |    8 ++++----
 guilt-unapplied |    2 +-
 4 files changed, 10 insertions(+), 9 deletions(-)

diff --git a/guilt-header b/guilt-header
index d07e2be..ef7f55e 100755
--- a/guilt-header
+++ b/guilt-header
@@ -15,15 +15,16 @@ patch="$1"
 if [ -z "$patch" ]; then
 	# use the patch that's on the top of the stack
 
-	eidx=`wc -l < $applied`
-	if [ $eidx -eq 0 ]; then
+	patch=`get_top`
+	if [ -z "$patch" ]; then
 		die "Status file is empty"
 	fi
+	eidx=`get_series | grep -ne "^$patch\$" | cut -d: -f1`
 else
 	# use the specified patch
 
 	eidx=`get_series | grep -ne "^$patch\$" | cut -d: -f1`
-	if [ $eidx -eq 0 ]; then
+	if [ -z "$eidx" ]; then
 		die "Patch $patch is not in the series"
 	fi
 fi
diff --git a/guilt-next b/guilt-next
index f38f1cc..38f57fa 100755
--- a/guilt-next
+++ b/guilt-next
@@ -13,5 +13,5 @@ fi
 n=`wc -l < $applied`
 n=$(($n + 1))
 
-get_series | awk "{ if (NR == $n) print \$0}"
+get_guarded_series | awk "{ if (NR == $n) print \$0}"
 
diff --git a/guilt-push b/guilt-push
index ad3616b..ce928e3 100755
--- a/guilt-push
+++ b/guilt-push
@@ -24,7 +24,7 @@ if [ "$patch" = "--all" ] || [ "$patch" = "-a" ]; then
 	# we are supposed to push all patches, get the last one out of
 	# series
 
-	eidx=`get_series | wc -l`
+	eidx=`get_guarded_series | wc -l`
 	if [ $eidx -eq 0 ]; then
 		die "There are no patches to push"
 	fi
@@ -37,9 +37,9 @@ else
 	# we're supposed to push only up to a patch, make sure the patch is
 	# in the series
 
-	eidx=`get_series | grep -ne "^$patch\$" | cut -d: -f1`
+	eidx=`get_guarded_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
 
@@ -52,7 +52,7 @@ fi
 sidx=`wc -l < $applied`
 sidx=`expr $sidx + 1`
 
-get_series | sed -n -e "${sidx},${eidx}p" | while read p
+get_guarded_series | sed -n -e "${sidx},${eidx}p" | while read p
 do
 	echo "Applying patch..$p"
 	if [ ! -f "$GUILT_DIR/$branch/$p" ]; then
diff --git a/guilt-unapplied b/guilt-unapplied
index 192a7e5..6904360 100755
--- a/guilt-unapplied
+++ b/guilt-unapplied
@@ -13,4 +13,4 @@ fi
 n=`wc -l < $applied`
 n=`expr $n + 1`
 
-get_series | sed -n -e "$n,\$p"
+get_guarded_series | sed -n -e "$n,\$p"
-- 
1.5.2

Re: [GUILT PATCH 4/4] Use guards information and functions

[Josef Sipek]

On Sun, Jul 29, 2007 at 12:50:18AM -0700, Eric Lesh wrote:
> Make guilt-push respect guards. Also teach guilt-header, guilt-next,
> and guilt-unapplied to grok patches that are skipped because of
> guards.
> 
> Signed-off-by: Eric Lesh <eclesh@ucla.edu>
> ---
>  guilt-header    |    7 ++++---
>  guilt-next      |    2 +-
>  guilt-push      |    8 ++++----
>  guilt-unapplied |    2 +-
>  4 files changed, 10 insertions(+), 9 deletions(-)
> 
> diff --git a/guilt-header b/guilt-header
> index d07e2be..ef7f55e 100755
> --- a/guilt-header
> +++ b/guilt-header
> @@ -15,15 +15,16 @@ patch="$1"
>  if [ -z "$patch" ]; then
>  	# use the patch that's on the top of the stack
>  
> -	eidx=`wc -l < $applied`
> -	if [ $eidx -eq 0 ]; then
> +	patch=`get_top`
> +	if [ -z "$patch" ]; then
>  		die "Status file is empty"
>  	fi
> +	eidx=`get_series | grep -ne "^$patch\$" | cut -d: -f1`
>  else
>  	# use the specified patch
>  
>  	eidx=`get_series | grep -ne "^$patch\$" | cut -d: -f1`
> -	if [ $eidx -eq 0 ]; then
> +	if [ -z "$eidx" ]; then
>  		die "Patch $patch is not in the series"
>  	fi
>  fi
> diff --git a/guilt-next b/guilt-next
> index f38f1cc..38f57fa 100755
> --- a/guilt-next
> +++ b/guilt-next
> @@ -13,5 +13,5 @@ fi
>  n=`wc -l < $applied`
>  n=$(($n + 1))
>  
> -get_series | awk "{ if (NR == $n) print \$0}"
> +get_guarded_series | awk "{ if (NR == $n) print \$0}"

Seeing this almost makes me thing that get_series should give you the
guarded series unless you poke it the right way...something like:

get_series --full

or

get_full_series


The guarded series is what most commands care about, right?

> diff --git a/guilt-push b/guilt-push
> index ad3616b..ce928e3 100755
> --- a/guilt-push
> +++ b/guilt-push
> @@ -24,7 +24,7 @@ if [ "$patch" = "--all" ] || [ "$patch" = "-a" ]; then
>  	# we are supposed to push all patches, get the last one out of
>  	# series
>  
> -	eidx=`get_series | wc -l`
> +	eidx=`get_guarded_series | wc -l`
>  	if [ $eidx -eq 0 ]; then
>  		die "There are no patches to push"
>  	fi
> @@ -37,9 +37,9 @@ else
>  	# we're supposed to push only up to a patch, make sure the patch is
>  	# in the series
>  
> -	eidx=`get_series | grep -ne "^$patch\$" | cut -d: -f1`
> +	eidx=`get_guarded_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
>  
> @@ -52,7 +52,7 @@ fi
>  sidx=`wc -l < $applied`
>  sidx=`expr $sidx + 1`
>  
> -get_series | sed -n -e "${sidx},${eidx}p" | while read p
> +get_guarded_series | sed -n -e "${sidx},${eidx}p" | while read p
>  do
>  	echo "Applying patch..$p"
>  	if [ ! -f "$GUILT_DIR/$branch/$p" ]; then
> diff --git a/guilt-unapplied b/guilt-unapplied
> index 192a7e5..6904360 100755
> --- a/guilt-unapplied
> +++ b/guilt-unapplied
> @@ -13,4 +13,4 @@ fi
>  n=`wc -l < $applied`
>  n=`expr $n + 1`
>  
> -get_series | sed -n -e "$n,\$p"
> +get_guarded_series | sed -n -e "$n,\$p"
> -- 
> 1.5.2

-- 
Debugging is twice as hard as writing the code in the first place.
Therefore, if you write the code as cleverly as possible, you are, by
definition, not smart enough to debug it.
		- Brian W. Kernighan 

Re: [GUILT PATCH 4/4] Use guards information and functions

[Eric Lesh]

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

>> -get_series | awk "{ if (NR == $n) print \$0}"
>> +get_guarded_series | awk "{ if (NR == $n) print \$0}"
>
> Seeing this almost makes me thing that get_series should give you the
> guarded series unless you poke it the right way...something like:
>
> get_series --full
>
> or
>
> get_full_series
>
>
> The guarded series is what most commands care about, right?
>

You're right.  I'll do it with get_full_series.

	Eric

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

[Josef Sipek]

On Sun, Jul 29, 2007 at 12:50:14AM -0700, Eric Lesh wrote:
> 
> 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.

Neat. I actually never looked into guards, but you forced me to :) Very
interesting concept. I think it would be worth it having such functionality
in guilt.

I know I've been bad about forcing even myself to write new regression
tests. Your patches modify things significantly enough, that I'd like to see
some regression tests cases to make sure that user's data is not eaten
(e.g., a bug in the guard setting code could blow away the series file =>
very bad).

> This introduces the command `get_guarded_series`, which just lists
> patches that are to be applied based on the guards.  It also makes
>         eidx=`wc -l < $applied`
> inaccurate if you're using it as an index into get_series.

The index-based patch finding is a bit nasty anyway.

> If you change guards on a patch or select a different guard while
> patches are applied, some commands might get confused. guilt pop -a will fix
> everything though.  Usually, it's best to pop -a before fiddling with
> guards anyway.

Is this a problem with other projects' implementations of guards as well?
Perhaps printing a warning if a new guard is set when there are applied
patches would be in order?

> This is an RFC, but I have tested it and things seem to be working
> well.

Great!

I'm going to reply to each of the patches separately with any comments.

Josef 'Jeff' Sipek.

-- 
Penguin : Linux version 2.4.20-46.9.legacysmp on an i386 machine (2778.72 BogoMips).

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

[Eric Lesh]

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

>
> I know I've been bad about forcing even myself to write new regression
> tests. Your patches modify things significantly enough, that I'd like to see
> some regression tests cases to make sure that user's data is not eaten
> (e.g., a bug in the guard setting code could blow away the series file =>
> very bad).
>

Yeah, I'll try and make one.  070-guards.sh to test guilt-guard and
guilt-select plus pushing and popping?

There's also sed -i in a few places. For integrity purposes, are a cp
and sed better?

>> This introduces the command `get_guarded_series`, which just lists
>> patches that are to be applied based on the guards.  It also makes
>>         eidx=`wc -l < $applied`
>> inaccurate if you're using it as an index into get_series.
>
> The index-based patch finding is a bit nasty anyway.
>
>> If you change guards on a patch or select a different guard while
>> patches are applied, some commands might get confused. guilt pop -a will fix
>> everything though.  Usually, it's best to pop -a before fiddling with
>> guards anyway.
>
> Is this a problem with other projects' implementations of guards as well?
> Perhaps printing a warning if a new guard is set when there are applied
> patches would be in order?
>

Yeah, they have this problem too, but tell you so when you select, so
guilt should too. I'll fix that up.  Mercurial also has two options
which do the popping and reapplying for you, which I'll try and implement also.

Thanks a lot for the review.  Things were pretty ugly, but with your
help it should look much better.

	Eric

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

[Josef Sipek]

On Mon, Jul 30, 2007 at 01:32:53AM -0700, Eric Lesh wrote:
> Josef Sipek <jsipek@fsl.cs.sunysb.edu> writes:
> 
> >
> > I know I've been bad about forcing even myself to write new regression
> > tests. Your patches modify things significantly enough, that I'd like to see
> > some regression tests cases to make sure that user's data is not eaten
> > (e.g., a bug in the guard setting code could blow away the series file =>
> > very bad).
> >
> 
> Yeah, I'll try and make one.  070-guards.sh to test guilt-guard and
> guilt-select plus pushing and popping?

Sounds good.

> There's also sed -i in a few places. For integrity purposes, are a cp
> and sed better?

I like the fact that sed -i makes the code cleaner, BUT I don't want the
users to come after me if they patches disappear. Perhaps having a wrapper
(a function) for sed that's nicely paranoid and handles errors as well as
possible would be in order:

safe_sed [<sed options...>]

> > Is this a problem with other projects' implementations of guards as well?
> > Perhaps printing a warning if a new guard is set when there are applied
> > patches would be in order?
> >
> 
> Yeah, they have this problem too,

Good :)

> but tell you so when you select, so guilt should too.

Agreed.

> Mercurial also has two options which do the popping and reapplying for
> you, which I'll try and implement also.

As in:

t=`guilt-top`
guilt-pop -a
guilt-push "$t"

? Beware that "$t" might not be in the new guarded series.

> Thanks a lot for the review.  Things were pretty ugly, but with your
> help it should look much better.

Nah...just a few nit-picks, that's all :)

Josef 'Jeff' Sipek.

-- 
I think there is a world market for maybe five computers.
		- Thomas Watson, chairman of IBM, 1943.