[dim PATCH 4/7] dim: abstract helper for listing unmerged tags

[Daniel Vetter]

On Fri, Dec 7, 2018 at 11:00 AM Jani Nikula <jani.nikula at intel.com> wrote:
>
> On Thu, 06 Dec 2018, Daniel Vetter <daniel at ffwll.ch> wrote:
> > On Wed, Dec 05, 2018 at 05:02:56PM +0200, Jani Nikula wrote:
> >> Make the pull request code a bit easier to grasp. Use git log pretty
> >> format for easier extraction of the tags.
> >>
> >> Signed-off-by: Jani Nikula <jani.nikula at intel.com>
> >> ---
> >>  dim | 18 +++++++++++++++++-
> >>  1 file changed, 17 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/dim b/dim
> >> index b227f0db99dc..e0f96f78c858 100755
> >> --- a/dim
> >> +++ b/dim
> >> @@ -345,6 +345,22 @@ function git_branch_exists # branch
> >>      fi
> >>  }
> >>
> >> +# $1: branch
> >> +# $2: upstream
> >> +function git_unmerged_tags
> >> +{
> >> +    local branch upstream
> >> +
> >> +    branch=$1
> >> +    upstream=$2
> >> +
> >> +    # assume branch based tag names
> >> +    git log --decorate --pretty=%D "$branch@{upstream}" ^$upstream |\
> >> +            grep -o "tag: $branch-[0-9-]\+" |\
> >
> > This drops the () which contain the branches, slightly increasing the risk
> > we misparse. I'm still baffled that there's no better way to get a list of
> > tags for a commit range.
>
> Note how I use --pretty=%D so I can be sure the git log output only
> contains ref names, and nothing else. I think this *decreases* the
> chances of misparsing.

Oh, entirely missed that. Much better than my cargo-cult :-)

> >> +            sed -e "s/^tag: //" |\
> >> +            tr "[:space:]" " "
> >
> > Not clear to me why we need to do space translations with your pipeline.
>
> Yeah, *sigh*.
>
> It's not strictly about this pipeline, it's about robustness in general.
>
> Assigning the output to a variable would include the newlines... but
> $drm_intel_next_tags references would have the newlines expanded to
> spaces while quoted "$drm_intel_next_tags" would retain the newlines. As
> the follow-up patches need to be able to pick the first from the list, I
> opted to this so I can use ${x%% *} to do that, without fear that
> quoting would mess that up.
>
> This was another one of those cases where I paused to contemplate the
> relative merits of converting the entire thing to python.

*shudder*

Reluctantly Reviewed-by: Daniel Vetter <daniel.vetter at ffwll.ch>

Maybe add a comment about this in the code that some quoting
translates the newlines to spaces while others doesn't, so need to
make it consistent manually. Also perhaps tr only newlines, to make it
more clear what we're doing.
-Daniel


> > I'd go with the old pipeline exactly. Not because it's better, but because
> > we know it works. At least until we have a real way to list tags ...
>
> Alas the old one doesn't work as-is for the general case. The
> drm-intel-next- grep&sed works only because the drm-intel-next branch
> does not contain tags such as drm-intel-next-fixes- which would also
> match.
>
> BR,
> Jani.
>
> > -Daniel
> >
> >> +}
> >> +
> >>  function git_committer_email
> >>  {
> >>      if ! committer_email=$(git config --get user.email) ; then
> >> @@ -1930,7 +1946,7 @@ function dim_pull_request
> >>
> >>      if [ "$branch" = "drm-intel-next" ]; then
> >>              # drm-intel-next pulls have been tagged using dim update-next
> >> -            drm_intel_next_tags=$(git log "$branch@{upstream}" ^$upstream --decorate | grep "(.*tag: drm-intel-next-" | sed -e "s/^.*(.*tag: \(drm-intel-next-[^ ,]*\).*)$/\1/")
> >> +            drm_intel_next_tags=$(git_unmerged_tags "$branch" "$upstream")
> >>              prep_pull_mail $req_file $drm_intel_next_tags
> >>              tag=$(git describe --all --exact "$branch@{upstream}")
> >>
> >> --
> >> 2.11.0
> >>
> >> _______________________________________________
> >> dim-tools mailing list
> >> dim-tools at lists.freedesktop.org
> >> https://lists.freedesktop.org/mailman/listinfo/dim-tools
>
> --
> Jani Nikula, Intel Open Source Graphics Center



--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch