threads / patch / 9285

patch, 4 partsget_series: Remove comments from end of series lines

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

## tl;dr

20 messages between Jul 29, 2007 and Jul 30, 2007. Diffs are folded; open one to read it.

replies: 19people: 2as markdown or json

Eric Lesh· Jul 29, 2007, 07:50 UTC · lore

[GUILT PATCH 0/4] Add guards to guilt

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
Eric Lesh· Jul 29, 2007, 07:50 UTC · re: Eric Lesh · lore
Signed-off-by: Eric Lesh <eclesh@ucla.edu>
--
---
 guilt |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)
Show changes to guilt +2 −1
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
Josef Sipek· Jul 30, 2007, 03:54 UTC · re: Eric Lesh · lore

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

On Sun, Jul 29, 2007 at 12:50:15AM -0700, Eric Lesh wrote: ...

Show 11 quoted lines
> 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)
Eric Lesh· Jul 30, 2007, 05:15 UTC · re: Josef Sipek · lore

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

[ 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:
Show 25 quoted lines
> 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
Josef Sipek· Jul 30, 2007, 05:26 UTC · re: Eric Lesh · lore

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

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.
Show 35 quoted lines
> 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
Eric Lesh· Jul 30, 2007, 07:07 UTC · re: Josef Sipek · lore

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

Josef Sipek <jsipek@fsl.cs.sunysb.edu> writes:
Show 11 quoted lines
>
> 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
Eric Lesh· Jul 29, 2007, 07:50 UTC · re: Eric Lesh · lore

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

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
Show changes to 3 files +161 −0

Documentation/guilt-guards.txt, guilt, 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
Josef Sipek· Jul 30, 2007, 04:06 UTC · re: Eric Lesh · lore

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

On Sun, Jul 29, 2007 at 12:50:16AM -0700, Eric Lesh wrote:
Show 78 quoted lines
> 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"
Show 15 quoted lines
> +	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
Show 9 quoted lines
> +}
> +
> +# 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.
Show 6 quoted lines
> +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?
Show 8 quoted lines
> +			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.
Show 24 quoted lines
> +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...]]
Show 59 quoted lines
> +. 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.
Eric Lesh· Jul 30, 2007, 06:41 UTC · re: Josef Sipek · lore

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

Josef Sipek <jsipek@fsl.cs.sunysb.edu> writes:
[...]
Show 28 quoted lines
>> +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.
Show 5 quoted lines
>> +# 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?
Show 5 quoted lines
>> +
>> +# usage: set_guards <patch> <guards>
>
> I'd try to make it clearer that multiple guards can be specified.
>
Done with <guards...> now.
Show 9 quoted lines
>> +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.

Show 9 quoted lines
>> +			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
Josef Sipek· Jul 30, 2007, 19:28 UTC · re: Eric Lesh · lore

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

On Sun, Jul 29, 2007 at 11:41:52PM -0700, Eric Lesh wrote:
Show 35 quoted lines
> 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.
Show 18 quoted lines
> 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 :)
Show 13 quoted lines
> 	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.
Show 23 quoted lines
> >> +
> >> +# 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.

Show 13 quoted lines
> >> +			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
Eric Lesh· Jul 29, 2007, 07:50 UTC · re: Eric Lesh · lore

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

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
Show changes to 3 files +79 −0

Documentation/guilt-select.txt, guilt, 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
Josef Sipek· Jul 30, 2007, 04:12 UTC · re: Eric Lesh · lore

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

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

Show 72 quoted lines
> @@ -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.

Show 8 quoted lines
> +	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 ...

Show 17 quoted lines
> +		;;
> +	-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.
Eric Lesh· Jul 30, 2007, 07:02 UTC · re: Josef Sipek · lore

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

Josef Sipek <jsipek@fsl.cs.sunysb.edu> writes:
[...]
Show 7 quoted lines
>> +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.

Show 8 quoted lines
>> +
>> +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
Josef Sipek· Jul 30, 2007, 19:34 UTC · re: Eric Lesh · lore

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

On Mon, Jul 30, 2007 at 12:02:26AM -0700, Eric Lesh wrote:
Show 15 quoted lines
> 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.
Show 11 quoted lines
> >> +
> >> +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. 
Eric Lesh· Jul 29, 2007, 07:50 UTC · re: Eric Lesh · lore

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

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(-)
Show changes to 4 files +10 −9

guilt-header, guilt-next, guilt-push, guilt-unapplied

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
Josef Sipek· Jul 30, 2007, 04:15 UTC · re: Eric Lesh · lore

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

On Sun, Jul 29, 2007 at 12:50:18AM -0700, Eric Lesh wrote:
Show 46 quoted lines
> 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?
Show 46 quoted lines
> 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 
Eric Lesh· Jul 30, 2007, 07:06 UTC · re: Josef Sipek · lore

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

Josef Sipek <jsipek@fsl.cs.sunysb.edu> writes:
Show 15 quoted lines
>> -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
Josef Sipek· Jul 30, 2007, 03:54 UTC · re: Eric Lesh · lore

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

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).
Eric Lesh· Jul 30, 2007, 08:32 UTC · re: Josef Sipek · lore

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

Josef Sipek <jsipek@fsl.cs.sunysb.edu> writes:
Show 7 quoted lines
>
> 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?

Show 16 quoted lines
>> 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
Josef Sipek· Jul 30, 2007, 19:20 UTC · re: Eric Lesh · lore

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

On Mon, Jul 30, 2007 at 01:32:53AM -0700, Eric Lesh wrote:
Show 12 quoted lines
> 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...>]
Show 6 quoted lines
> > 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.

← back to recent threads