[devel] [PATCH v4 1/2] Add diff exclude option for gear-rules

[devel] [PATCH v4 1/2] Add diff exclude option for gear-rules

[Vladimir D. Seleznev]

The option specifies a glob pattern that define files that should be
excluded from diff generation.

Signed-off-by: Vladimir D. Seleznev <vseleznv@altlinux.org>
---
 gear            | 17 +++++++++++++----
 gear-rules.5.in | 19 +++++++++++++++++++
 gear-store-tags |  4 ++++
 3 files changed, 36 insertions(+), 4 deletions(-)

diff --git a/gear b/gear
index 6282c92..7bdddc0 100755
--- a/gear
+++ b/gear
@@ -337,7 +337,7 @@ make_archive()
 	write_git_archive "$format" "$optional" "$cmd" "$tar_tree" "$dir_name" "$tar_name" "$tar_base" "$tar_suffix"
 }
 
-get_diff_name()
+get_diff_opts()
 {
 	local old_d="$1" && shift
 	local new_d="$1" && shift
@@ -376,6 +376,13 @@ get_diff_name()
 				;;
 			name=*) diff_name="${opt#name=}"
 				;;
+			exclude=*)
+				[ -z "$(printf %s "${opt#exclude=}" |tr -d '[:alnum:]_.?*-/![]')" ] ||
+					rules_error "Invalid diff exclude pattern specified: ${opt#exclude=}"
+				[ -n "${opt#exclude=}" ] ||
+					rules_error "Invalid diff exclude pattern specified: empty pattern"
+				diff_exclude="$diff_exclude :^${opt#exclude=}"
+				;;
 			*) rules_error "Unrecognized option: $opt"
 				;;
 		esac
@@ -402,6 +409,7 @@ write_git_diff()
 {
 	local optional="$1" && shift
 	local cmd="$1" && shift
+	local diff_exclude="$1" && shift
 	local old_tree="$1" && shift
 	local old_dir="$1" && shift
 	local new_tree="$1" && shift
@@ -423,7 +431,7 @@ write_git_diff()
 		}
 
 	git diff-tree --patch-with-stat --text --no-renames --no-ext-diff \
-		"$old_id" "$new_id" >"$outdir/$name"
+		"$old_id" "$new_id" -- $diff_exclude >"$outdir/$name"
 	verbose "Extracted diff: $name"
 
 	compress_file "$cmd" "$name" ''
@@ -449,14 +457,15 @@ make_difference()
 	diff_new_tree="${new_dir%%:*}"
 	new_dir="${new_dir#*:}"
 
-	get_diff_name "$old_dir" "$new_dir" "$@"
+	local diff_exclude=
+	get_diff_opts "$old_dir" "$new_dir" "$@"
 
 	diff_old_tree="$(resolve_tree_base_name "$diff_old_tree")" ||
 		rules_error "Invalid old tree: $diff_old_tree"
 	diff_new_tree="$(resolve_tree_base_name "$diff_new_tree")" ||
 		rules_error "Invalid new tree: $diff_new_tree"
 
-	write_git_diff "$optional" "$cmd" "$diff_old_tree" "$old_dir" "$diff_new_tree" "$new_dir" "$diff_name"
+	write_git_diff "$optional" "$cmd" "$diff_exclude" "$diff_old_tree" "$old_dir" "$diff_new_tree" "$new_dir" "$diff_name"
 }
 
 extract_pattern()
diff --git a/gear-rules.5.in b/gear-rules.5.in
index 5469199..ae79a35 100644
--- a/gear-rules.5.in
+++ b/gear-rules.5.in
@@ -380,6 +380,12 @@ unless the path part of
 .I new_tree_path
 is \*(lq.\*(rq, in which case the default is
 .BR @name@\-@version@\-@release@.patch .
+.TP
+.BI exclude= glob_pattern
+All the files that match the
+.B glob_pattern
+are excluded from the patch generation. This option can be specified multiple
+times.
 .RE
 .IP
 Value of the
@@ -541,6 +547,19 @@ However, if a prerelease version is packaged, parts like
 \*(lqpreN\*(rq or \*(lqrcN\*(rq should not be included in the package
 version, therefore the rule file will need some modifications for such
 versions.
+.PP
+You can also want to exclude
+.BR .gear
+subdirectory and spec file from diff generation:
+.RS 4
+.PP
+.ft CW
+.nf
+tar: v@version@:.
+diff: v@version@:. . exclude=.gear/** exclude=*.spec
+.fi
+.RE
+.PP
 .
 .SS "Archive with unmodified sources and separate patch files"
 If you prefer to maintain patch files for local modifications instead
diff --git a/gear-store-tags b/gear-store-tags
index a649b66..fdb82e1 100755
--- a/gear-store-tags
+++ b/gear-store-tags
@@ -162,6 +162,10 @@ find_tags_in_diff_options()
 				;;
 			name=*)
 				;;
+			exclude=*)
+				[ -z "$(printf %s "${opt#exclude=}" |tr -d '[:alnum:]_.?*-/![]')" ] ||
+					rules_info "Invalid diff exclude pattern specified: ${opt#exclude=}"
+				;;
 			*) rules_info "Unrecognized option \"$opt\" ignored"
 				;;
 		esac
-- 
2.29.2

[devel] [PATCH v4 2/2] tests: add test for diff exclude option

[Vladimir D. Seleznev]

Signed-off-by: Vladimir D. Seleznev <vseleznv@altlinux.org>
---
 tests/gear-rules-diff-tag-tree-exclude.test | 51 +++++++++++++++++++++
 1 file changed, 51 insertions(+)
 create mode 100644 tests/gear-rules-diff-tag-tree-exclude.test

diff --git a/tests/gear-rules-diff-tag-tree-exclude.test b/tests/gear-rules-diff-tag-tree-exclude.test
new file mode 100644
index 0000000..e9899ed
--- /dev/null
+++ b/tests/gear-rules-diff-tag-tree-exclude.test
@@ -0,0 +1,51 @@
+#!/bin/sh -efu
+
+gear_rules_diff_tag_tree_exclude() # test entry point
+{
+	local spec=file.spec
+	local n=foo
+	local v=1
+	local r=99
+	local src=a/bb/ccc/dddd/src
+	local dst=1/22/333/4444/55555/dst
+	local patch="${dst##*/}-$v-$r.patch${gear_directive#diff}"
+
+	printf 'Name:%s\nVersion:  %s\nRelease:   %s' "$n" "$v" "$r" >"$spec"
+	mkdir -p $src
+	echo one >$src/file
+	mkdir $src/sub
+	echo two >$src/sub/file2
+	git add "$spec" $src/file $src/sub/file2
+	git commit -q -a -m 1
+	git tag -a -m t1 v1
+	git rm -q -f $src/file $src/sub/file2
+	mkdir .gear
+	echo "$gear_directive: v1:$src $dst exclude=sub/** exclude=*3" > .gear/rules
+	mkdir -p $dst
+	echo two >$dst/file
+	echo three >$dst/file3
+	git add .gear/rules $dst/file $dst/file3
+	gear-store-tags -a
+	git commit -q -a -m 2
+
+	finalize_repo
+
+	$GEAR "$GEAR_OUT"
+	check_pkgtar_list "$spec
+./
+./$spec
+./$patch"
+	check_pkgtar_spec "$GEAR_OUT" "$spec"
+	unpack_pkgtar_to .out
+	result="$($decompress_prog ".out/$patch")"
+	assertEquals ' file | 2 +-
+ 1 file changed, 1 insertion(+), 1 deletion(-)
+
+diff --git a/file b/file
+index 5626abf..f719efd 100644
+--- a/file
++++ b/file
+@@ -1 +1 @@
+-one
++two' "$result"
+}
-- 
2.29.2

Re: [devel] [PATCH v4 1/2] Add diff exclude option for gear-rules

[Vladimir D. Seleznev]

Ping

On Fri, Nov 27, 2020 at 04:44:30PM +0300, Vladimir D. Seleznev wrote:
> The option specifies a glob pattern that define files that should be
> excluded from diff generation.
> 
> Signed-off-by: Vladimir D. Seleznev <vseleznv@altlinux.org>
> ---
>  gear            | 17 +++++++++++++----
>  gear-rules.5.in | 19 +++++++++++++++++++
>  gear-store-tags |  4 ++++
>  3 files changed, 36 insertions(+), 4 deletions(-)
> 
> diff --git a/gear b/gear
> index 6282c92..7bdddc0 100755
> --- a/gear
> +++ b/gear
> @@ -337,7 +337,7 @@ make_archive()
>  	write_git_archive "$format" "$optional" "$cmd" "$tar_tree" "$dir_name" "$tar_name" "$tar_base" "$tar_suffix"
>  }
>  
> -get_diff_name()
> +get_diff_opts()
>  {
>  	local old_d="$1" && shift
>  	local new_d="$1" && shift
> @@ -376,6 +376,13 @@ get_diff_name()
>  				;;
>  			name=*) diff_name="${opt#name=}"
>  				;;
> +			exclude=*)
> +				[ -z "$(printf %s "${opt#exclude=}" |tr -d '[:alnum:]_.?*-/![]')" ] ||
> +					rules_error "Invalid diff exclude pattern specified: ${opt#exclude=}"
> +				[ -n "${opt#exclude=}" ] ||
> +					rules_error "Invalid diff exclude pattern specified: empty pattern"
> +				diff_exclude="$diff_exclude :^${opt#exclude=}"
> +				;;
>  			*) rules_error "Unrecognized option: $opt"
>  				;;
>  		esac
> @@ -402,6 +409,7 @@ write_git_diff()
>  {
>  	local optional="$1" && shift
>  	local cmd="$1" && shift
> +	local diff_exclude="$1" && shift
>  	local old_tree="$1" && shift
>  	local old_dir="$1" && shift
>  	local new_tree="$1" && shift
> @@ -423,7 +431,7 @@ write_git_diff()
>  		}
>  
>  	git diff-tree --patch-with-stat --text --no-renames --no-ext-diff \
> -		"$old_id" "$new_id" >"$outdir/$name"
> +		"$old_id" "$new_id" -- $diff_exclude >"$outdir/$name"
>  	verbose "Extracted diff: $name"
>  
>  	compress_file "$cmd" "$name" ''
> @@ -449,14 +457,15 @@ make_difference()
>  	diff_new_tree="${new_dir%%:*}"
>  	new_dir="${new_dir#*:}"
>  
> -	get_diff_name "$old_dir" "$new_dir" "$@"
> +	local diff_exclude=
> +	get_diff_opts "$old_dir" "$new_dir" "$@"
>  
>  	diff_old_tree="$(resolve_tree_base_name "$diff_old_tree")" ||
>  		rules_error "Invalid old tree: $diff_old_tree"
>  	diff_new_tree="$(resolve_tree_base_name "$diff_new_tree")" ||
>  		rules_error "Invalid new tree: $diff_new_tree"
>  
> -	write_git_diff "$optional" "$cmd" "$diff_old_tree" "$old_dir" "$diff_new_tree" "$new_dir" "$diff_name"
> +	write_git_diff "$optional" "$cmd" "$diff_exclude" "$diff_old_tree" "$old_dir" "$diff_new_tree" "$new_dir" "$diff_name"
>  }
>  
>  extract_pattern()
> diff --git a/gear-rules.5.in b/gear-rules.5.in
> index 5469199..ae79a35 100644
> --- a/gear-rules.5.in
> +++ b/gear-rules.5.in
> @@ -380,6 +380,12 @@ unless the path part of
>  .I new_tree_path
>  is \*(lq.\*(rq, in which case the default is
>  .BR @name@\-@version@\-@release@.patch .
> +.TP
> +.BI exclude= glob_pattern
> +All the files that match the
> +.B glob_pattern
> +are excluded from the patch generation. This option can be specified multiple
> +times.
>  .RE
>  .IP
>  Value of the
> @@ -541,6 +547,19 @@ However, if a prerelease version is packaged, parts like
>  \*(lqpreN\*(rq or \*(lqrcN\*(rq should not be included in the package
>  version, therefore the rule file will need some modifications for such
>  versions.
> +.PP
> +You can also want to exclude
> +.BR .gear
> +subdirectory and spec file from diff generation:
> +.RS 4
> +.PP
> +.ft CW
> +.nf
> +tar: v@version@:.
> +diff: v@version@:. . exclude=.gear/** exclude=*.spec
> +.fi
> +.RE
> +.PP
>  .
>  .SS "Archive with unmodified sources and separate patch files"
>  If you prefer to maintain patch files for local modifications instead
> diff --git a/gear-store-tags b/gear-store-tags
> index a649b66..fdb82e1 100755
> --- a/gear-store-tags
> +++ b/gear-store-tags
> @@ -162,6 +162,10 @@ find_tags_in_diff_options()
>  				;;
>  			name=*)
>  				;;
> +			exclude=*)
> +				[ -z "$(printf %s "${opt#exclude=}" |tr -d '[:alnum:]_.?*-/![]')" ] ||
> +					rules_info "Invalid diff exclude pattern specified: ${opt#exclude=}"
> +				;;
>  			*) rules_info "Unrecognized option \"$opt\" ignored"
>  				;;
>  		esac
> -- 
> 2.29.2

-- 
   WBR,
   Vladimir D. Seleznev

Re: [devel] [PATCH v4 1/2] Add diff exclude option for gear-rules

[Dmitry V. Levin]

On Fri, Nov 27, 2020 at 04:44:30PM +0300, Vladimir D. Seleznev wrote:
> The option specifies a glob pattern that define files that should be

pattern that defineS

> excluded from diff generation.
> 
> Signed-off-by: Vladimir D. Seleznev <vseleznv@altlinux.org>
> ---
>  gear            | 17 +++++++++++++----
>  gear-rules.5.in | 19 +++++++++++++++++++
>  gear-store-tags |  4 ++++
>  3 files changed, 36 insertions(+), 4 deletions(-)
> 
> diff --git a/gear b/gear
> index 6282c92..7bdddc0 100755
> --- a/gear
> +++ b/gear
> @@ -337,7 +337,7 @@ make_archive()
>  	write_git_archive "$format" "$optional" "$cmd" "$tar_tree" "$dir_name" "$tar_name" "$tar_base" "$tar_suffix"
>  }
>  
> -get_diff_name()
> +get_diff_opts()
>  {
>  	local old_d="$1" && shift
>  	local new_d="$1" && shift

OK

> @@ -376,6 +376,13 @@ get_diff_name()
>  				;;
>  			name=*) diff_name="${opt#name=}"
>  				;;
> +			exclude=*)
> +				[ -z "$(printf %s "${opt#exclude=}" |tr -d '[:alnum:]_.?*-/![]')" ] ||
> +					rules_error "Invalid diff exclude pattern specified: ${opt#exclude=}"

Why '[:alnum:]_.?*-/![]' ?

> +				[ -n "${opt#exclude=}" ] ||
> +					rules_error "Invalid diff exclude pattern specified: empty pattern"
> +				diff_exclude="$diff_exclude :^${opt#exclude=}"
> +				;;

You're doing this ${opt#exclude=} thing 4 times,
I suggest saving the result to a variable and using it afterwards.


>  			*) rules_error "Unrecognized option: $opt"
>  				;;
>  		esac
> @@ -402,6 +409,7 @@ write_git_diff()
>  {
>  	local optional="$1" && shift
>  	local cmd="$1" && shift
> +	local diff_exclude="$1" && shift
>  	local old_tree="$1" && shift
>  	local old_dir="$1" && shift
>  	local new_tree="$1" && shift

OK

> @@ -423,7 +431,7 @@ write_git_diff()
>  		}
>  
>  	git diff-tree --patch-with-stat --text --no-renames --no-ext-diff \
> -		"$old_id" "$new_id" >"$outdir/$name"
> +		"$old_id" "$new_id" -- $diff_exclude >"$outdir/$name"
>  	verbose "Extracted diff: $name"
>  
>  	compress_file "$cmd" "$name" ''

OK

> @@ -449,14 +457,15 @@ make_difference()
>  	diff_new_tree="${new_dir%%:*}"
>  	new_dir="${new_dir#*:}"
>  
> -	get_diff_name "$old_dir" "$new_dir" "$@"
> +	local diff_exclude=
> +	get_diff_opts "$old_dir" "$new_dir" "$@"

OK

>  
>  	diff_old_tree="$(resolve_tree_base_name "$diff_old_tree")" ||
>  		rules_error "Invalid old tree: $diff_old_tree"
>  	diff_new_tree="$(resolve_tree_base_name "$diff_new_tree")" ||
>  		rules_error "Invalid new tree: $diff_new_tree"
>  
> -	write_git_diff "$optional" "$cmd" "$diff_old_tree" "$old_dir" "$diff_new_tree" "$new_dir" "$diff_name"
> +	write_git_diff "$optional" "$cmd" "$diff_exclude" "$diff_old_tree" "$old_dir" "$diff_new_tree" "$new_dir" "$diff_name"

The line was too long before the change, now it's way too long.

>  }
>  
>  extract_pattern()
> diff --git a/gear-rules.5.in b/gear-rules.5.in
> index 5469199..ae79a35 100644
> --- a/gear-rules.5.in
> +++ b/gear-rules.5.in
> @@ -380,6 +380,12 @@ unless the path part of
>  .I new_tree_path
>  is \*(lq.\*(rq, in which case the default is
>  .BR @name@\-@version@\-@release@.patch .
> +.TP
> +.BI exclude= glob_pattern
> +All the files that match the

All files matching the

> +.B glob_pattern

.I glob_pattern

> +are excluded from the patch generation. This option can be specified multiple
> +times.

are excluded from the generated diff file.

>  .RE
>  .IP
>  Value of the
> @@ -541,6 +547,19 @@ However, if a prerelease version is packaged, parts like
>  \*(lqpreN\*(rq or \*(lqrcN\*(rq should not be included in the package
>  version, therefore the rule file will need some modifications for such
>  versions.
> +.PP
> +You can also want to exclude

You may want to exclude

> +.BR .gear
> +subdirectory and spec file from diff generation:
> +.RS 4
> +.PP
> +.ft CW
> +.nf
> +tar: v@version@:.
> +diff: v@version@:. . exclude=.gear/** exclude=*.spec
> +.fi
> +.RE
> +.PP
>  .
>  .SS "Archive with unmodified sources and separate patch files"
>  If you prefer to maintain patch files for local modifications instead
> diff --git a/gear-store-tags b/gear-store-tags
> index a649b66..fdb82e1 100755
> --- a/gear-store-tags
> +++ b/gear-store-tags
> @@ -162,6 +162,10 @@ find_tags_in_diff_options()
>  				;;
>  			name=*)
>  				;;
> +			exclude=*)
> +				[ -z "$(printf %s "${opt#exclude=}" |tr -d '[:alnum:]_.?*-/![]')" ] ||
> +					rules_info "Invalid diff exclude pattern specified: ${opt#exclude=}"
> +				;;

exclude= is unused here, why bother about its correctness?


-- 
ldv

Re: [devel] [PATCH v4 1/2] Add diff exclude option for gear-rules

[Alexey Gladkov]

On Mon, Dec 07, 2020 at 07:58:09PM +0300, Dmitry V. Levin wrote:
> On Fri, Nov 27, 2020 at 04:44:30PM +0300, Vladimir D. Seleznev wrote:
> > The option specifies a glob pattern that define files that should be
> 
> pattern that defineS
> 
> > excluded from diff generation.
> > 
> > Signed-off-by: Vladimir D. Seleznev <vseleznv@altlinux.org>
> > ---
> >  gear            | 17 +++++++++++++----
> >  gear-rules.5.in | 19 +++++++++++++++++++
> >  gear-store-tags |  4 ++++
> >  3 files changed, 36 insertions(+), 4 deletions(-)
> > 
> > diff --git a/gear b/gear
> > index 6282c92..7bdddc0 100755
> > --- a/gear
> > +++ b/gear
> > @@ -337,7 +337,7 @@ make_archive()
> >  	write_git_archive "$format" "$optional" "$cmd" "$tar_tree" "$dir_name" "$tar_name" "$tar_base" "$tar_suffix"
> >  }
> >  
> > -get_diff_name()
> > +get_diff_opts()
> >  {
> >  	local old_d="$1" && shift
> >  	local new_d="$1" && shift
> 
> OK
> 
> > @@ -376,6 +376,13 @@ get_diff_name()
> >  				;;
> >  			name=*) diff_name="${opt#name=}"
> >  				;;
> > +			exclude=*)
> > +				[ -z "$(printf %s "${opt#exclude=}" |tr -d '[:alnum:]_.?*-/![]')" ] ||
> > +					rules_error "Invalid diff exclude pattern specified: ${opt#exclude=}"
> 
> Why '[:alnum:]_.?*-/![]' ?

because glob patterns have these symbols.

> > +				[ -n "${opt#exclude=}" ] ||
> > +					rules_error "Invalid diff exclude pattern specified: empty pattern"
> > +				diff_exclude="$diff_exclude :^${opt#exclude=}"
> > +				;;
> 
> You're doing this ${opt#exclude=} thing 4 times,
> I suggest saving the result to a variable and using it afterwards.
> 
> 
> >  			*) rules_error "Unrecognized option: $opt"
> >  				;;
> >  		esac
> > @@ -402,6 +409,7 @@ write_git_diff()
> >  {
> >  	local optional="$1" && shift
> >  	local cmd="$1" && shift
> > +	local diff_exclude="$1" && shift
> >  	local old_tree="$1" && shift
> >  	local old_dir="$1" && shift
> >  	local new_tree="$1" && shift
> 
> OK
> 
> > @@ -423,7 +431,7 @@ write_git_diff()
> >  		}
> >  
> >  	git diff-tree --patch-with-stat --text --no-renames --no-ext-diff \
> > -		"$old_id" "$new_id" >"$outdir/$name"
> > +		"$old_id" "$new_id" -- $diff_exclude >"$outdir/$name"
> >  	verbose "Extracted diff: $name"
> >  
> >  	compress_file "$cmd" "$name" ''
> 
> OK
> 
> > @@ -449,14 +457,15 @@ make_difference()
> >  	diff_new_tree="${new_dir%%:*}"
> >  	new_dir="${new_dir#*:}"
> >  
> > -	get_diff_name "$old_dir" "$new_dir" "$@"
> > +	local diff_exclude=
> > +	get_diff_opts "$old_dir" "$new_dir" "$@"
> 
> OK
> 
> >  
> >  	diff_old_tree="$(resolve_tree_base_name "$diff_old_tree")" ||
> >  		rules_error "Invalid old tree: $diff_old_tree"
> >  	diff_new_tree="$(resolve_tree_base_name "$diff_new_tree")" ||
> >  		rules_error "Invalid new tree: $diff_new_tree"
> >  
> > -	write_git_diff "$optional" "$cmd" "$diff_old_tree" "$old_dir" "$diff_new_tree" "$new_dir" "$diff_name"
> > +	write_git_diff "$optional" "$cmd" "$diff_exclude" "$diff_old_tree" "$old_dir" "$diff_new_tree" "$new_dir" "$diff_name"
> 
> The line was too long before the change, now it's way too long.
> 
> >  }
> >  
> >  extract_pattern()
> > diff --git a/gear-rules.5.in b/gear-rules.5.in
> > index 5469199..ae79a35 100644
> > --- a/gear-rules.5.in
> > +++ b/gear-rules.5.in
> > @@ -380,6 +380,12 @@ unless the path part of
> >  .I new_tree_path
> >  is \*(lq.\*(rq, in which case the default is
> >  .BR @name@\-@version@\-@release@.patch .
> > +.TP
> > +.BI exclude= glob_pattern
> > +All the files that match the
> 
> All files matching the
> 
> > +.B glob_pattern
> 
> .I glob_pattern
> 
> > +are excluded from the patch generation. This option can be specified multiple
> > +times.
> 
> are excluded from the generated diff file.
> 
> >  .RE
> >  .IP
> >  Value of the
> > @@ -541,6 +547,19 @@ However, if a prerelease version is packaged, parts like
> >  \*(lqpreN\*(rq or \*(lqrcN\*(rq should not be included in the package
> >  version, therefore the rule file will need some modifications for such
> >  versions.
> > +.PP
> > +You can also want to exclude
> 
> You may want to exclude
> 
> > +.BR .gear
> > +subdirectory and spec file from diff generation:
> > +.RS 4
> > +.PP
> > +.ft CW
> > +.nf
> > +tar: v@version@:.
> > +diff: v@version@:. . exclude=.gear/** exclude=*.spec
> > +.fi
> > +.RE
> > +.PP
> >  .
> >  .SS "Archive with unmodified sources and separate patch files"
> >  If you prefer to maintain patch files for local modifications instead
> > diff --git a/gear-store-tags b/gear-store-tags
> > index a649b66..fdb82e1 100755
> > --- a/gear-store-tags
> > +++ b/gear-store-tags
> > @@ -162,6 +162,10 @@ find_tags_in_diff_options()
> >  				;;
> >  			name=*)
> >  				;;
> > +			exclude=*)
> > +				[ -z "$(printf %s "${opt#exclude=}" |tr -d '[:alnum:]_.?*-/![]')" ] ||
> > +					rules_info "Invalid diff exclude pattern specified: ${opt#exclude=}"
> > +				;;
> 
> exclude= is unused here, why bother about its correctness?
> 
> 
> -- 
> ldv
> 

-- 
Rgrds, legion

Re: [devel] [PATCH v4 1/2] Add diff exclude option for gear-rules

[Dmitry V. Levin]

On Mon, Dec 07, 2020 at 06:07:41PM +0100, Alexey Gladkov wrote:
> On Mon, Dec 07, 2020 at 07:58:09PM +0300, Dmitry V. Levin wrote:
> > On Fri, Nov 27, 2020 at 04:44:30PM +0300, Vladimir D. Seleznev wrote:
[...]
> > > @@ -376,6 +376,13 @@ get_diff_name()
> > >  				;;
> > >  			name=*) diff_name="${opt#name=}"
> > >  				;;
> > > +			exclude=*)
> > > +				[ -z "$(printf %s "${opt#exclude=}" |tr -d '[:alnum:]_.?*-/![]')" ] ||
> > > +					rules_error "Invalid diff exclude pattern specified: ${opt#exclude=}"
> > 
> > Why '[:alnum:]_.?*-/![]' ?
> 
> because glob patterns have these symbols.

This doesn't match the pattern check in extract_pattern(),
should it be updated?

Would it be useful to have a single definition of this set somewhere?


-- 
ldv

Re: [devel] [PATCH v4 1/2] Add diff exclude option for gear-rules

[Alexey Gladkov]

On Mon, Dec 07, 2020 at 08:19:25PM +0300, Dmitry V. Levin wrote:
> On Mon, Dec 07, 2020 at 06:07:41PM +0100, Alexey Gladkov wrote:
> > On Mon, Dec 07, 2020 at 07:58:09PM +0300, Dmitry V. Levin wrote:
> > > On Fri, Nov 27, 2020 at 04:44:30PM +0300, Vladimir D. Seleznev wrote:
> [...]
> > > > @@ -376,6 +376,13 @@ get_diff_name()
> > > >  				;;
> > > >  			name=*) diff_name="${opt#name=}"
> > > >  				;;
> > > > +			exclude=*)
> > > > +				[ -z "$(printf %s "${opt#exclude=}" |tr -d '[:alnum:]_.?*-/![]')" ] ||
> > > > +					rules_error "Invalid diff exclude pattern specified: ${opt#exclude=}"
> > > 
> > > Why '[:alnum:]_.?*-/![]' ?
> > 
> > because glob patterns have these symbols.
> 
> This doesn't match the pattern check in extract_pattern(),
> should it be updated?

This is a different issue. I told you about this on November 26, 2020.

> Would it be useful to have a single definition of this set somewhere?

Yep.

-- 
Rgrds, legion

Re: [devel] [PATCH v4 1/2] Add diff exclude option for gear-rules

[Vladimir D. Seleznev]

On Mon, Dec 07, 2020 at 07:58:09PM +0300, Dmitry V. Levin wrote:
> On Fri, Nov 27, 2020 at 04:44:30PM +0300, Vladimir D. Seleznev wrote:
> > The option specifies a glob pattern that define files that should be
> 
> pattern that defineS
> 
> > excluded from diff generation.
> > 
> > Signed-off-by: Vladimir D. Seleznev <vseleznv@altlinux.org>
> > ---
> >  gear            | 17 +++++++++++++----
> >  gear-rules.5.in | 19 +++++++++++++++++++
> >  gear-store-tags |  4 ++++
> >  3 files changed, 36 insertions(+), 4 deletions(-)
> > 
> > diff --git a/gear b/gear
> > index 6282c92..7bdddc0 100755
> > --- a/gear
> > +++ b/gear
> > @@ -337,7 +337,7 @@ make_archive()
> >  	write_git_archive "$format" "$optional" "$cmd" "$tar_tree" "$dir_name" "$tar_name" "$tar_base" "$tar_suffix"
> >  }
> >  
> > -get_diff_name()
> > +get_diff_opts()
> >  {
> >  	local old_d="$1" && shift
> >  	local new_d="$1" && shift
> 
> OK
> 
> > @@ -376,6 +376,13 @@ get_diff_name()
> >  				;;
> >  			name=*) diff_name="${opt#name=}"
> >  				;;
> > +			exclude=*)
> > +				[ -z "$(printf %s "${opt#exclude=}" |tr -d '[:alnum:]_.?*-/![]')" ] ||
> > +					rules_error "Invalid diff exclude pattern specified: ${opt#exclude=}"
> 
> Why '[:alnum:]_.?*-/![]' ?
> 
> > +				[ -n "${opt#exclude=}" ] ||
> > +					rules_error "Invalid diff exclude pattern specified: empty pattern"
> > +				diff_exclude="$diff_exclude :^${opt#exclude=}"
> > +				;;
> 
> You're doing this ${opt#exclude=} thing 4 times,
> I suggest saving the result to a variable and using it afterwards.

https://lists.altlinux.org/pipermail/devel/2020-November/212766.html

On Fri, Nov 27, 2020 at 02:28:08PM +0100, Alexey Gladkov wrote:
> Здесь и в gear-store-tags не требуется заводить новую переменную только
> для того чтобы переложить $opt без префикса в $diff_exclude.
> 
> С другими опциями это оправдано так как значения этих опций используются
> в функции ниже.
> 
> Предлагаю написать просто: opt="${opt#exclude=}"

-- 
   WBR,
   Vladimir D. Seleznev