All of lore.kernel.org
 help / color / mirror / Atom feed
From: Jani Nikula <jani.nikula@linux.intel.com>
To: ville.syrjala@linux.intel.com, intel-gfx@lists.freedesktop.org
Subject: Re: [PATCH dim 1/2] dim: Add extract-tags command
Date: Wed, 15 Mar 2017 11:11:29 +0200	[thread overview]
Message-ID: <87a88m9a66.fsf@intel.com> (raw)
In-Reply-To: <20170314185014.8635-1-ville.syrjala@linux.intel.com>

On Tue, 14 Mar 2017, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> Add a command for extracting various tags (eg. Reviwed-by:) from
> emails. You can give the comamnd a rangeish to add the tags from
> the same email to multiple already applied patches.
>
> The regexp used to pick up tags is purposefully quite broad. People
> tend to typo these things, or add extra whitespace etc. However the
> broad regexp does mean this occasionally picks up stuff that isn't
> a tag. So manually amending the commit is probably a wise idea,
> and so I simply decided to also leave a '--- extracted tags ---'
> separator in the commit message just before the extracted tags,
> which can be cleaned up manually when verifying that the tags look
> correct.
>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  dim | 60 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 60 insertions(+)
>
> diff --git a/dim b/dim
> index 6d3b9734b348..4110642b2f4a 100755
> --- a/dim
> +++ b/dim
> @@ -333,6 +333,28 @@ if message_id is not None:
>  EOF
>  }
>  
> +message_print_body ()
> +{
> +	python2 <<EOF
> +import email
> +
> +def print_part(part):
> +    mtype = part.get_content_maintype()
> +    if mtype == 'text':
> +        print(part.get_payload(decode=True))
> +
> +def print_msg(file):
> +    msg = email.message_from_file(file)
> +    if msg.is_multipart():
> +        for part in msg.get_payload():
> +            print_part(part)
> +    else:
> +        print_part(msg)
> +
> +print_msg(open('$1', 'r'))
> +EOF
> +}
> +
>  # append all arguments as tags at the end of the commit message of HEAD
>  function dim_commit_add_tag
>  {
> @@ -973,6 +995,44 @@ function rangeish()
>  	fi
>  }
>  
> +dim_alias_xt=extract-tags
> +function dim_extract_tags
> +{
> +	local branch=$1
> +	shift
> +	local range=$(rangeish "$1")
> +	local file=`mktemp`

So shell has an annoying feature that the 'set -e' has no effect on
commands failing when used in an initialization within the local
variable declaration. Simple assignments would be okay, but for clarity
I've opted to put all local variable declarations in a single line,
folled by a blank line and then assignments. See elsewhere in dim.

I also prefer $(...) over `...`.

> +
> +	assert_branch $branch
> +	assert_repo_clean
> +
> +	cat > $file
> +
> +	echo "message_print_body \"$file\""

Leftover debug message?

> +	local tags=$(message_print_body "$file" | grep -ai '^[^>]*[A-Za-z-]\+: [^ ]')

That's indeed quite liberal in accepting stuff. But perhaps it's okay
for starters, and we can restrict it more if it gets to be annoying.

> +	test -n "$tags" || return 0

Maybe it's just me trying to maintain a 1.5k line shell script, but I'd
like to be more verbose with the if statements. I'd write this as

	if [[ -n "$tags" ]]; then
		...

> +	local tags=$(printf -- "--- extracted tags ---\n%s" "$tags")

Lose the local here. Nitpick, AFAICT most git commands (like
format-patch) wrap placeholders in *** rather than ---.

An interesting observation, if you put # in front of the line, you can
actually have that as part of the commit message. But after you do 'git
commit --amend' on it, the line gets dropped as a comment
automatically. In short, that would save a couple of keystrokes perhaps,
even if you'd have to move the other tags around.

> +	git filter-branch -f --msg-filter "cat ; echo \"$tags\"" $range

I wonder what I screwed up when I was trying to use that for
dim_commit_add_tag and failed; the current implementation of it is
hidous. :/

> +}
> +
> +dim_alias_xq=extract-queued
> +function dim_extract_queued
> +{
> +	dim_extract_tags drm-intel-next-queued "$@"
> +}
> +
> +dim_alias_xf=extract-fixes
> +function dim_extract_fixes
> +{
> +	dim_extract_tags drm-intel-fixes "$@"
> +}
> +
> +dim_alias_xnf=extract-next-fixes
> +function dim_extract_next_fixes
> +{
> +	dim_extract_tags drm-intel-next-fixes "$@"
> +}

These are all in line with what we have all around, but makes me wonder
if we should consider just working on the current branch in the future.

Thanks for doing this, regardless of the nitpicks above. ;)

BR,
Jani.

> +
>  dim_alias_check_patch=checkpatch
>  dim_alias_cp=checkpatch
>  function dim_checkpatch

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

  parent reply	other threads:[~2017-03-15  9:11 UTC|newest]

Thread overview: 15+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-03-14 18:50 [PATCH dim 1/2] dim: Add extract-tags command ville.syrjala
2017-03-14 18:50 ` [PATCH dim 2/2] dim: Add add-link command ville.syrjala
2017-03-15  9:17   ` Jani Nikula
2017-03-15  9:18     ` Jani Nikula
2017-03-15 10:53       ` Ville Syrjälä
2017-03-15 11:08         ` Jani Nikula
2017-03-15 15:09   ` [PATCH dim v2 " ville.syrjala
2017-03-15  9:11 ` Jani Nikula [this message]
2017-03-15 10:51   ` [PATCH dim 1/2] dim: Add extract-tags command Ville Syrjälä
2017-03-15 11:04     ` Jani Nikula
2017-03-15 15:09 ` [PATCH dim v2 " ville.syrjala
2017-03-15 15:15   ` Chris Wilson
2017-03-16  7:40     ` Jani Nikula
2017-03-16  9:49       ` Ville Syrjälä
2017-03-16 13:33         ` Jani Nikula

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=87a88m9a66.fsf@intel.com \
    --to=jani.nikula@linux.intel.com \
    --cc=intel-gfx@lists.freedesktop.org \
    --cc=ville.syrjala@linux.intel.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.