Re: [PATCH dim 1/2] dim: Add extract-tags command

From: Jani Nikula <jani.nikula@linux.intel.com>
To: "Ville Syrjälä" <ville.syrjala@linux.intel.com>
Cc: intel-gfx@lists.freedesktop.org
Subject: Re: [PATCH dim 1/2] dim: Add extract-tags command
Date: Wed, 15 Mar 2017 13:04:47 +0200	[thread overview]
Message-ID: <87lgs67qcw.fsf@intel.com> (raw)
In-Reply-To: <20170315105153.GS31595@intel.com>

On Wed, 15 Mar 2017, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> On Wed, Mar 15, 2017 at 11:11:29AM +0200, Jani Nikula wrote:
>> 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 `...`.
>
> dim is a bit of a hodge podge of varying styles. Which is why my
> copy-paste coding technique gave me all kinds of things ;)

I know... I've been trying to gradually clean it up, and I think I have
a branch fixing a bunch of shellcheck issues too. I'm sure some might
feel that enforcing a style or shellcheck cleanliness in a shell script
is overkill, but I'm thinking overgrown shell scripts like this will
become completely unmaintainable otherwise.

>> These are all in line with what we have all around, but makes me wonder
>
> I can drop the aliases here as well if people prefer. My .dimrc already
> has a boatload of these for all the other branches so having to move a
> few more there isn't a big deal.

Yeah, let's drop the aliases, at least for now. Having an alias
mechanism (that also bash autocompletes for user defined aliases) should
be good enough. Again, can be added later if there's demand.

>
>> if we should consider just working on the current branch in the future.
>
> Not sure there is any "current branch" as such when you have worktrees
> for everything.

Right.

BR,
Jani.

>
>> 
>> 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

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