Re: [PATCH 1/3 v3] Clean up t6016-rev-list-graph-simplify-history

[Antonio Russo <antonio.e.russo@gmail.com>]

On 5/1/20 11:10 AM, Junio C Hamano wrote:
> Antonio Russo <antonio.e.russo@gmail.com> writes:
> 
>> +check_graph () {
>> +	cat >expected &&
> 
> Not a new issue, but we may want to fix this to align to majority of
> tests by calling it "expect".
> 
>> +	git rev-list --graph "$@" | sed "$(cat sedscript)" > actual &&
> 
> Style. No SP between > (or < for that matter) and the filename.
> 
> The "sed" utility can be told to read its script from a file with
> its "-f" option.
> 
> Correctness.  Never run "git" command that is the target being
> tested on the left side of a pipe.  It will hide the exit status.

>> -	'
>> +	echo "s/ *$//;" > sedscript &&
>> +	( for tag in $(git tag -l) ; do echo "s/$(git rev-parse --verify $tag)/$tag/;" >> sedscript ; done )
> 
> Avoid unreadable one-liner with needless subshell.
> 
> I suspect that this is a task for-each-ref was designed for,
> something along the lines of...
> 
> 	git for-each-ref --format='s|%(objectname)|%(refname:short)|' \
> 		refs/tags/ >>sedScript
> 

Thanks, I've fixed these.

> 
> Much nicer to see.
> 
> Having said all that, I am not sure if this change of design is
> sound.
> 
> The original approach would have worked even if two or more of these
> tags pointed at the same object.  Your version will pick one of
> them.  If two tags, say A5 and C8, pointed at the same commit, and
> the illustration given to check_graph helper from its standard
> output labeled a commit as C8, wouldn't the actual output converted
> to show A5 with your sedScript approach?
> 
> I think it is salvageable by changing the direction you munge.
> Instead of munging the rev-list output, you can store it as-is in
> "actual", and instead pass the illustration that comes from the
> standard input of the check_graph helper through sed to expand the
> symbolic names to actual object names before comparing. i.e.
> 
> 	check_graph () {
> 		sed -f expand_tag_to_objects.sed >expect &&
> 		git rev-list --graph "$@" >actual &&
> 		test_cmp expect actual
> 	}
> 
> Note that I renamed the overly generic "sedscript" to a name that
> reflects the purpose of the file (and the contents being of a
> certain type is conveyed by .sed suffix, just like you'd use
> suffixes like .c, .txt).  A good discipline to learn, I would say.

I also fixed these.  I originally chose the other direction
(oid->refname) to make the output more human readable.

If you think that's a worthwhile goal, I can figure something out.

That said, the patchset as it stands scratches my immediate need,
and addressing D. Stolee's comments (in particular, making it work
with revs->limited==0) unfortunately has to be a lower priority
for me.

I am not abandoning the patchset, but may be more delayed
responding to review.

Thanks,
Antonio