On 2023/08/07 23:47:53 +1000, Mark Jamsek <mark@jamsek.com> wrote:
> Thanks, op! Sorry I didn't get this out last night.

not a problem at all!

> I've just added the other combinations to your diff so we test for -dPp,
> -dP, and -dp, and also added some 'log -x keyword' cases to the keywords
> regress.
> 
> The diff, however, also includes quite a few log regress copypasta
> errors where I cmp(1)'d stdout{,.expected} instead of stderr! I was
> very fortunate that we are producing the correct output; the tests and
> behaviour are all correct but I failed to check that it indeed was
> right. There was also one diff keyword case with the same problem
> although the test still wrongly checked for the original BASE output,
> which I have also now fixed.

oops!  sorry for not catching these while reviewing.

> I checked the many keyword tests I did around that time and it looks
> like I only committed this error repeatedly in the log tests and that
> last diff keyword test. I'm going to double check them again tomorrow
> with fresher eyes to be sure though.
> 
> I will _never_ copypasta regress scripts ever again; that was equal
> parts stupid and lucky!
> 
> btw, regarding log -dP, I was wondering if we should even make these
> mutually exclusive? It seems like it might be the right thing to do
> because it doesn't really make sense to ask for both as they both print
> the changed paths, just the diffstat annotates them with lines added
> and/or removed plus a summary total. It's fine as it is now the
> duplication is fixed but I was thinking about this when writing the
> test so thought I'd float the idea.

got tends to reject (partially) conflicting options instead of having
late flags overriding the previous, so declaring -d and -P conflicting
doesn't seem a bad idea to me, it would be somewhat coherent.

I don't have a strong opinion on this.  For the scripting /
interactive usage it's handier to handle also (partially) conflicting
flags, so that if you alias gl='got log -P' and later use `gl -d' it
works, but to simplify the internal handling making them conflicting
also makes sense.  Your call, I guess :-)

two suggestions below, it's ok op@ in any case.  Thanks!

> [...]
> --- regress/cmdline/log.sh
> +++ regress/cmdline/log.sh
> @@ -896,7 +896,43 @@ EOF
>  	ret=$?
>  	if [ $ret -ne 0 ]; then
>  		diff -u $testroot/stdout.expected $testroot/stdout
> +		test_done "$testroot" "$ret"
> +		return 1
>  	fi

since it's quite a few copypasta we could consider use a loop to
generate all the cases, for e.g.

	for flags in -d -dp -dP -dPp; do
	    (cd ... && got log $flags ... )
	    ... cmp ...
	done

> +	# try again with -dPp
> +	(cd $testroot/wt && got log -dPp | grep -A2 '^ [MDmA]' | \
> +	    sed '/^--/d' > $testroot/stdout)
> +
> +	cmp -s $testroot/stdout.expected $testroot/stdout
> +	ret=$?
> +	if [ $ret -ne 0 ]; then
> +		diff -u $testroot/stdout.expected $testroot/stdout
> +		test_done "$testroot" "$ret"
> +		return 1
> +	fi
> +
> +	# try again with -dP
> +	(cd $testroot/wt && got log -dP | grep -A2 '^ [MDmA]' | \
> +	    sed '/^--/d' > $testroot/stdout)
> +
> +	cmp -s $testroot/stdout.expected $testroot/stdout
> +	ret=$?
> +	if [ $ret -ne 0 ]; then
> +		diff -u $testroot/stdout.expected $testroot/stdout
> +		test_done "$testroot" "$ret"
> +		return 1
> +	fi
> +
> +	# try again with -dp
> +	(cd $testroot/wt && got log -dp | grep -A2 '^ [MDmA]' | \
> +	    sed '/^--/d' > $testroot/stdout)
> +
> +	cmp -s $testroot/stdout.expected $testroot/stdout
> +	ret=$?
> +	if [ $ret -ne 0 ]; then
> +		diff -u $testroot/stdout.expected $testroot/stdout
> +	fi
>  	test_done "$testroot" "$ret"
>  }
>  
> @@ -944,6 +980,17 @@ test_log_commit_keywords() {
>  		return 1
>  	fi
>  
> +	# request same set of commits now with log -x
> +	got log -r "$testroot/repo" -scmaster:- -xmaster:-15 > $testroot/stdout

not an issue, and during interactive usage i tend to join flags too,
but for readability in the regress I'd use the slightly more verbose

	got log -r "$testroot/repo" -s -cmaster:- ...

there are some more instances below.

> @@ -972,6 +1019,25 @@ test_log_commit_keywords() {
>  		return 1
>  	fi
>  
> +	# from head to the base commit using -x
> +	printf '%s %-7s commit number 16\n' "$d" "master" > \
> +	    $testroot/stdout.expected
> +	for i in $(seq 16 9); do
> +		printf '%s %.7s commit number %s\n' \
> +		    "$d" $(pop_idx $i $@) "$(( i-1 ))" \

not an issue but since $(pop_idx) was not quoted, $((i-1)) doesn't
need to be quoted either

(I'm not opposed to over-quoting things since how tricky word
splitting is on the bourne shell, pointed out only because pop_idx
wasn't quoted)

> +		    >> $testroot/stdout.expected
> +	done