On 5/1/20 11:10 AM, Junio C Hamano wrote:
> Antonio Russo <antonio.e.russo@xxxxxxxxx> 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
