Omar Polo <op@omarpolo.com> wrote:
> 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!
> 
> > ...
> > 
> > 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 :-)

Thanks for sharing your thoughts about this. After further consideration,
I think it might be better to keep things as is; I'm not sure we'll gain
anything by rejecting -dP. It was very simple to handle this case and I
don't anticipate any substantial changes to this interface so rejecting
them now doesn't seem to benefit us. But, as you point out, it could
lead to unexpected changes for users and for no real reason.

If anything, purely for the sake of less options, I'd probably lean more
toward dropping -d (or -P because 'd' is easier to type), and showing
the diffstat by default if -P (or -d) is specified. But that means we're
computing the diff when the user may just want the path list, and on
slower machines that cost may be noticeable.

That said, if anyone thinks we should, I certainly won't object.

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

Thanks, op! I've committed this with your suggestions :)

> > [...]
> > --- 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


-- 
Mark Jamsek <https://bsdbox.org>
GPG: F2FF 13DE 6A06 C471 CA80  E6E2 2930 DC66 86EE CF68