[maintainer-tools PATCH 1/2] dim: Accept .mbox and .patch files as apply-branch optional argument.

[maintainer-tools PATCH 1/2] dim: Accept .mbox and .patch files as apply-branch optional argument.

[Rodrigo Vivi]

Instead of forcing users to cat .patch or .mbox let's accept them
as optional argument for dim apply-branches.

Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Jani Nikula <jani.nikula@intel.com>
Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
---
 dim     | 10 +++++++++-
 dim.rst |  2 +-
 2 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/dim b/dim
index 11aa675cc3bc..e98d23b24ec0 100755
--- a/dim
+++ b/dim
@@ -771,7 +771,15 @@ function dim_apply_branch
 	assert_branch $branch
 	assert_repo_clean
 
-	cat > $file
+	case $1 in
+		*".patch" | *".mbox")
+			cat $1 > $file
+			shift
+			;;
+		*)
+			cat > $file
+			;;
+	esac
 
 	message_id=$(message_get_id $file)
 
diff --git a/dim.rst b/dim.rst
index 802c776e03f9..7f492edc4c04 100644
--- a/dim.rst
+++ b/dim.rst
@@ -79,7 +79,7 @@ first need to check out the right branch using::
 
 Applying patches is done in the main repository with::
 
-    $ cat patch.mbox | dim apply-branch <branch>
+    $ dim apply-branch <branch> patch.mbox
 
 This works like a glorified version of git apply-mbox and does basic patch
 checking and adds stuff like patchwork links of the merged patch. It is
-- 
2.13.2

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[maintainer-tools PATCH 2/2] dim: Accept patchwork URLs as apply-branch optional argument.

[Rodrigo Vivi]

Instead of having to manually download mbox from patchwork
let's make dim to do it directly.

Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Jani Nikula <jani.nikula@intel.com>
Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
---
 dim | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/dim b/dim
index e98d23b24ec0..73b48da7f436 100755
--- a/dim
+++ b/dim
@@ -756,6 +756,16 @@ function dim_push
 	dim_push_branch $(git_current_branch) "$@"
 }
 
+function download_mbox
+{
+	wget -q --spider ${1}
+	if [ $? -ne "0" ]; then
+	    echoerr "URL ${1} not found."
+	    exit 1
+	fi
+	wget -q ${1} -O $2
+}
+
 # ensure we're on branch $1, and apply patches. the rest of the arguments are
 # passed to git am.
 dim_alias_ab=apply-branch
@@ -772,6 +782,14 @@ function dim_apply_branch
 	assert_repo_clean
 
 	case $1 in
+		*"patchwork.freedesktop.org"*"mbox")
+			download_mbox $1 $file
+			shift
+			;;
+		*"patchwork.freedesktop.org"*)
+			download_mbox $1/mbox $file
+			shift
+			;;
 		*".patch" | *".mbox")
 			cat $1 > $file
 			shift
-- 
2.13.2

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [maintainer-tools PATCH 2/2] dim: Accept patchwork URLs as apply-branch optional argument.

[Rodrigo Vivi]

On Wed, Aug 16, 2017 at 11:13 AM, Rodrigo Vivi <rodrigo.vivi@intel.com> wrote:
> Instead of having to manually download mbox from patchwork
> let's make dim to do it directly.
>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Jani Nikula <jani.nikula@intel.com>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> ---
>  dim | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
>
> diff --git a/dim b/dim
> index e98d23b24ec0..73b48da7f436 100755
> --- a/dim
> +++ b/dim
> @@ -756,6 +756,16 @@ function dim_push
>         dim_push_branch $(git_current_branch) "$@"
>  }
>
> +function download_mbox
> +{
> +       wget -q --spider ${1}
> +       if [ $? -ne "0" ]; then
> +           echoerr "URL ${1} not found."
> +           exit 1
> +       fi
> +       wget -q ${1} -O $2
> +}
> +
>  # ensure we're on branch $1, and apply patches. the rest of the arguments are
>  # passed to git am.
>  dim_alias_ab=apply-branch
> @@ -772,6 +782,14 @@ function dim_apply_branch
>         assert_repo_clean
>
>         case $1 in
> +               *"patchwork.freedesktop.org"*"mbox")
> +                       download_mbox $1 $file
> +                       shift
> +                       ;;
> +               *"patchwork.freedesktop.org"*)

Another thing that I'd like to do is to be able to give the patchwork
id directly, but I don't want to mess with the $@ going to git
directly so I'm not sure which way would be better...
maybe parse for something like
"pw="*)
download_mbox ${1#pw=} $file
so we could use
dim aq pw=170802

?
suggestions?


> +                       download_mbox $1/mbox $file
> +                       shift
> +                       ;;
>                 *".patch" | *".mbox")
>                         cat $1 > $file
>                         shift
> --
> 2.13.2
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Rodrigo Vivi
Blog: http://blog.vivi.eng.br
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [maintainer-tools PATCH 1/2] dim: Accept .mbox and .patch files as apply-branch optional argument.

[Jani Nikula]

On Wed, 16 Aug 2017, Rodrigo Vivi <rodrigo.vivi@intel.com> wrote:
> Instead of forcing users to cat .patch or .mbox let's accept them
> as optional argument for dim apply-branches.

Well, that's a useless use of cat anyway. You could do

$ dim apply-branch branch < patch.mbox

> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Jani Nikula <jani.nikula@intel.com>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> ---
>  dim     | 10 +++++++++-
>  dim.rst |  2 +-
>  2 files changed, 10 insertions(+), 2 deletions(-)
>
> diff --git a/dim b/dim
> index 11aa675cc3bc..e98d23b24ec0 100755
> --- a/dim
> +++ b/dim
> @@ -771,7 +771,15 @@ function dim_apply_branch
>  	assert_branch $branch
>  	assert_repo_clean
>  
> -	cat > $file
> +	case $1 in
> +		*".patch" | *".mbox")
> +			cat $1 > $file
> +			shift
> +			;;
> +		*)
> +			cat > $file
> +			;;
> +	esac

This would really be a surprising interface, argument parsing based on
file suffixes. I don't approve.

You'll need to make this handle options before the branch argument,
something like:

Usage: dim apply-branch [apply-branch options] branch [--] [git options]

Is stdin redirection really such a bad thing?

BR,
Jani.


>  
>  	message_id=$(message_get_id $file)
>  
> diff --git a/dim.rst b/dim.rst
> index 802c776e03f9..7f492edc4c04 100644
> --- a/dim.rst
> +++ b/dim.rst
> @@ -79,7 +79,7 @@ first need to check out the right branch using::
>  
>  Applying patches is done in the main repository with::
>  
> -    $ cat patch.mbox | dim apply-branch <branch>
> +    $ dim apply-branch <branch> patch.mbox
>  
>  This works like a glorified version of git apply-mbox and does basic patch
>  checking and adds stuff like patchwork links of the merged patch. It is

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

Re: [maintainer-tools PATCH 2/2] dim: Accept patchwork URLs as apply-branch optional argument.

[Jani Nikula]

On Wed, 16 Aug 2017, Rodrigo Vivi <rodrigo.vivi@intel.com> wrote:
> Instead of having to manually download mbox from patchwork
> let's make dim to do it directly.
>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Jani Nikula <jani.nikula@intel.com>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> ---
>  dim | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
>
> diff --git a/dim b/dim
> index e98d23b24ec0..73b48da7f436 100755
> --- a/dim
> +++ b/dim
> @@ -756,6 +756,16 @@ function dim_push
>  	dim_push_branch $(git_current_branch) "$@"
>  }
>  
> +function download_mbox
> +{
> +	wget -q --spider ${1}

What's the benefit of doing this first?

> +	if [ $? -ne "0" ]; then
> +	    echoerr "URL ${1} not found."
> +	    exit 1

Always just return 1 on errors, and set -e will handle the abort. This
way the caller can decide to use foo || true to ignore the error.

> +	fi
> +	wget -q ${1} -O $2
> +}
> +
>  # ensure we're on branch $1, and apply patches. the rest of the arguments are
>  # passed to git am.
>  dim_alias_ab=apply-branch
> @@ -772,6 +782,14 @@ function dim_apply_branch
>  	assert_repo_clean
>  
>  	case $1 in
> +		*"patchwork.freedesktop.org"*"mbox")
> +			download_mbox $1 $file
> +			shift
> +			;;
> +		*"patchwork.freedesktop.org"*)
> +			download_mbox $1/mbox $file
> +			shift
> +			;;

Really, the interface gets worse and worse here!

---

I may sound like a pedant looking after style and consistency in a shell
script, but this one has grown beyond the size where we can just ignore
its maintenance. I've put in quite a bit of effort since the user base
went from 1 to 2 to keep it together.


BR,
Jani.


>  		*".patch" | *".mbox")
>  			cat $1 > $file
>  			shift

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

Re: [maintainer-tools PATCH 2/2] dim: Accept patchwork URLs as apply-branch optional argument.

[Jani Nikula]

On Wed, 16 Aug 2017, Rodrigo Vivi <rodrigo.vivi@gmail.com> wrote:
> On Wed, Aug 16, 2017 at 11:13 AM, Rodrigo Vivi <rodrigo.vivi@intel.com> wrote:
>> Instead of having to manually download mbox from patchwork
>> let's make dim to do it directly.
>>
>> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
>> Cc: Jani Nikula <jani.nikula@intel.com>
>> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
>> ---
>>  dim | 18 ++++++++++++++++++
>>  1 file changed, 18 insertions(+)
>>
>> diff --git a/dim b/dim
>> index e98d23b24ec0..73b48da7f436 100755
>> --- a/dim
>> +++ b/dim
>> @@ -756,6 +756,16 @@ function dim_push
>>         dim_push_branch $(git_current_branch) "$@"
>>  }
>>
>> +function download_mbox
>> +{
>> +       wget -q --spider ${1}
>> +       if [ $? -ne "0" ]; then
>> +           echoerr "URL ${1} not found."
>> +           exit 1
>> +       fi
>> +       wget -q ${1} -O $2
>> +}
>> +
>>  # ensure we're on branch $1, and apply patches. the rest of the arguments are
>>  # passed to git am.
>>  dim_alias_ab=apply-branch
>> @@ -772,6 +782,14 @@ function dim_apply_branch
>>         assert_repo_clean
>>
>>         case $1 in
>> +               *"patchwork.freedesktop.org"*"mbox")
>> +                       download_mbox $1 $file
>> +                       shift
>> +                       ;;
>> +               *"patchwork.freedesktop.org"*)
>
> Another thing that I'd like to do is to be able to give the patchwork
> id directly, but I don't want to mess with the $@ going to git
> directly so I'm not sure which way would be better...

Personally I prefer using message-id based patchwork references:

http://patchwork.freedesktop.org/patch/msgid/20170811113907.6716-1-jani.nikula@intel.com

> maybe parse for something like
> "pw="*)
> download_mbox ${1#pw=} $file
> so we could use
> dim aq pw=170802
>
> ?
> suggestions?

The ways around this are argument parsing in apply-branch or adding
another dim subcommand.

Btw one missing piece is handling series mboxes, which do apply, but
only the last commit from an mbox gets all the checks and Link: tags
etc.


BR,
Jani.


>
>
>> +                       download_mbox $1/mbox $file
>> +                       shift
>> +                       ;;
>>                 *".patch" | *".mbox")
>>                         cat $1 > $file
>>                         shift
>> --
>> 2.13.2
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

Re: [maintainer-tools PATCH 1/2] dim: Accept .mbox and .patch files as apply-branch optional argument.

[Daniel Vetter]

On Thu, Aug 17, 2017 at 10:30:02AM +0300, Jani Nikula wrote:
> On Wed, 16 Aug 2017, Rodrigo Vivi <rodrigo.vivi@intel.com> wrote:
> > Instead of forcing users to cat .patch or .mbox let's accept them
> > as optional argument for dim apply-branches.
> 
> Well, that's a useless use of cat anyway. You could do
> 
> $ dim apply-branch branch < patch.mbox
> 
> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> > Cc: Jani Nikula <jani.nikula@intel.com>
> > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > ---
> >  dim     | 10 +++++++++-
> >  dim.rst |  2 +-
> >  2 files changed, 10 insertions(+), 2 deletions(-)
> >
> > diff --git a/dim b/dim
> > index 11aa675cc3bc..e98d23b24ec0 100755
> > --- a/dim
> > +++ b/dim
> > @@ -771,7 +771,15 @@ function dim_apply_branch
> >  	assert_branch $branch
> >  	assert_repo_clean
> >  
> > -	cat > $file
> > +	case $1 in
> > +		*".patch" | *".mbox")
> > +			cat $1 > $file
> > +			shift
> > +			;;
> > +		*)
> > +			cat > $file
> > +			;;
> > +	esac
> 
> This would really be a surprising interface, argument parsing based on
> file suffixes. I don't approve.
> 
> You'll need to make this handle options before the branch argument,
> something like:
> 
> Usage: dim apply-branch [apply-branch options] branch [--] [git options]
> 
> Is stdin redirection really such a bad thing?

+1 on that. If I pick it from patchwork or an mbox, I just < patch.mbox or
curl patchwork.url | dim apply. I don't see the value in baking that into
the script ...

What we could do (and we have a jira for that already) is to check
patchwork for the updated .mbox with all the r-b/t-b/a-b tags auto-added.
That would greatly improve at least my workflow.

And I also agree with Jani on keeping the dim design clean. We can add
stuff where it makes sense, and where dim really can add value (with
additional safety checks and convenience features).

One thing we maybe could be doing is a more fancy aliasing feature, along
the lines of git aliases, where you can do whatever you want. Then you
could do a dim apply-curl alias which resolves to curl $arg | dim apply.
Not sure how to implement that though ...
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [maintainer-tools PATCH 1/2] dim: Accept .mbox and .patch files as apply-branch optional argument.

[Jani Nikula]

On Thu, 17 Aug 2017, Daniel Vetter <daniel@ffwll.ch> wrote:
> One thing we maybe could be doing is a more fancy aliasing feature, along
> the lines of git aliases, where you can do whatever you want. Then you
> could do a dim apply-curl alias which resolves to curl $arg | dim apply.
> Not sure how to implement that though ...

We *already* have a fancy aliasing feature. You can add arbitrary
subcommands of your own, and you can override existing subcommands. Try
this in your ~/.dimrc:

dim_my_fancy_list_aliases()
{
	echo "Hello world!"
	dim_list_aliases
}

dim_alias_list_aliases=my-fancy-list-aliases

This first part will give you a new "dim my-fancy-list-aliases"
subcommand, and the second part will use it for "dim list-aliases".

Go wild. ;)


BR,
Jani.


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

Re: [maintainer-tools PATCH 1/2] dim: Accept .mbox and .patch files as apply-branch optional argument.

[Daniel Vetter]

On Thu, Aug 17, 2017 at 10:12 AM, Jani Nikula <jani.nikula@intel.com> wrote:
> On Thu, 17 Aug 2017, Daniel Vetter <daniel@ffwll.ch> wrote:
>> One thing we maybe could be doing is a more fancy aliasing feature, along
>> the lines of git aliases, where you can do whatever you want. Then you
>> could do a dim apply-curl alias which resolves to curl $arg | dim apply.
>> Not sure how to implement that though ...
>
> We *already* have a fancy aliasing feature. You can add arbitrary
> subcommands of your own, and you can override existing subcommands. Try
> this in your ~/.dimrc:
>
> dim_my_fancy_list_aliases()
> {
>         echo "Hello world!"
>         dim_list_aliases
> }
>
> dim_alias_list_aliases=my-fancy-list-aliases
>
> This first part will give you a new "dim my-fancy-list-aliases"
> subcommand, and the second part will use it for "dim list-aliases".
>
> Go wild. ;)

Maybe we just need to document this better? Rodrigo, you're up for a
patch to dim.rst, with the above example in a new ALIASES sections?
I'd put it right above ENVIRONMENT.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [maintainer-tools PATCH 2/2] dim: Accept patchwork URLs as apply-branch optional argument.

[Rodrigo Vivi]

On Thu, Aug 17, 2017 at 12:37 AM, Jani Nikula <jani.nikula@intel.com> wrote:
> On Wed, 16 Aug 2017, Rodrigo Vivi <rodrigo.vivi@intel.com> wrote:
>> Instead of having to manually download mbox from patchwork
>> let's make dim to do it directly.
>>
>> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
>> Cc: Jani Nikula <jani.nikula@intel.com>
>> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
>> ---
>>  dim | 18 ++++++++++++++++++
>>  1 file changed, 18 insertions(+)
>>
>> diff --git a/dim b/dim
>> index e98d23b24ec0..73b48da7f436 100755
>> --- a/dim
>> +++ b/dim
>> @@ -756,6 +756,16 @@ function dim_push
>>       dim_push_branch $(git_current_branch) "$@"
>>  }
>>
>> +function download_mbox
>> +{
>> +     wget -q --spider ${1}
>
> What's the benefit of doing this first?

it check if links exist before trying to download anything.
at least it returns fast when link is not valid....

>
>> +     if [ $? -ne "0" ]; then
>> +         echoerr "URL ${1} not found."
>> +         exit 1
>
> Always just return 1 on errors, and set -e will handle the abort. This
> way the caller can decide to use foo || true to ignore the error.

oh of course!

>
>> +     fi
>> +     wget -q ${1} -O $2
>> +}
>> +
>>  # ensure we're on branch $1, and apply patches. the rest of the arguments are
>>  # passed to git am.
>>  dim_alias_ab=apply-branch
>> @@ -772,6 +782,14 @@ function dim_apply_branch
>>       assert_repo_clean
>>
>>       case $1 in
>> +             *"patchwork.freedesktop.org"*"mbox")
>> +                     download_mbox $1 $file
>> +                     shift
>> +                     ;;
>> +             *"patchwork.freedesktop.org"*)
>> +                     download_mbox $1/mbox $file
>> +                     shift
>> +                     ;;
>
> Really, the interface gets worse and worse here!

actually was the other way around...
I made this bad one and decided to extend to files :P

>
> ---
>
> I may sound like a pedant looking after style and consistency in a shell
> script, but this one has grown beyond the size where we can just ignore
> its maintenance. I've put in quite a bit of effort since the user base
> went from 1 to 2 to keep it together.

you are absolutely right. it is not just a shell script... it is our
tool and we should care!

>
>
> BR,
> Jani.
>
>
>>               *".patch" | *".mbox")
>>                       cat $1 > $file
>>                       shift
>
> --
> Jani Nikula, Intel Open Source Technology Center
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Rodrigo Vivi
Blog: http://blog.vivi.eng.br
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [maintainer-tools PATCH 2/2] dim: Accept patchwork URLs as apply-branch optional argument.

[Rodrigo Vivi]

On Thu, Aug 17, 2017 at 12:45 AM, Jani Nikula <jani.nikula@intel.com> wrote:
> On Wed, 16 Aug 2017, Rodrigo Vivi <rodrigo.vivi@gmail.com> wrote:
>> On Wed, Aug 16, 2017 at 11:13 AM, Rodrigo Vivi <rodrigo.vivi@intel.com> wrote:
>>> Instead of having to manually download mbox from patchwork
>>> let's make dim to do it directly.
>>>
>>> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
>>> Cc: Jani Nikula <jani.nikula@intel.com>
>>> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
>>> ---
>>>  dim | 18 ++++++++++++++++++
>>>  1 file changed, 18 insertions(+)
>>>
>>> diff --git a/dim b/dim
>>> index e98d23b24ec0..73b48da7f436 100755
>>> --- a/dim
>>> +++ b/dim
>>> @@ -756,6 +756,16 @@ function dim_push
>>>         dim_push_branch $(git_current_branch) "$@"
>>>  }
>>>
>>> +function download_mbox
>>> +{
>>> +       wget -q --spider ${1}
>>> +       if [ $? -ne "0" ]; then
>>> +           echoerr "URL ${1} not found."
>>> +           exit 1
>>> +       fi
>>> +       wget -q ${1} -O $2
>>> +}
>>> +
>>>  # ensure we're on branch $1, and apply patches. the rest of the arguments are
>>>  # passed to git am.
>>>  dim_alias_ab=apply-branch
>>> @@ -772,6 +782,14 @@ function dim_apply_branch
>>>         assert_repo_clean
>>>
>>>         case $1 in
>>> +               *"patchwork.freedesktop.org"*"mbox")
>>> +                       download_mbox $1 $file
>>> +                       shift
>>> +                       ;;
>>> +               *"patchwork.freedesktop.org"*)
>>
>> Another thing that I'd like to do is to be able to give the patchwork
>> id directly, but I don't want to mess with the $@ going to git
>> directly so I'm not sure which way would be better...
>
> Personally I prefer using message-id based patchwork references:
>
> http://patchwork.freedesktop.org/patch/msgid/20170811113907.6716-1-jani.nikula@intel.com

well remembered...
in the end it gets converted to
https://patchwork.freedesktop.org/patch/171432/

so we can get
https://patchwork.freedesktop.org/patch/171432/mbox

>
>> maybe parse for something like
>> "pw="*)
>> download_mbox ${1#pw=} $file
>> so we could use
>> dim aq pw=170802
>>
>> ?
>> suggestions?
>
> The ways around this are argument parsing in apply-branch or adding
> another dim subcommand.

Usage: dim apply-branch [apply-branch options] branch [--] [git options]

-i  <patchwork-id>
-m <Message-id>
-u <url>

hmm but not sure it goes well with the aliases were we would need to convert

dim aq -m 20170811113907.6716-1-jani.nikula@intel.com [git options]
to
dim apply-branch -m 20170811113907.6716-1-jani.nikula@intel.com
drm-intel-next-queued [git options]

>
> Btw one missing piece is handling series mboxes, which do apply, but
> only the last commit from an mbox gets all the checks and Link: tags
> etc.

yeap! well remembered! I suffered from this behaviour already and seen
other folks also complaining about it.

So, for this should we iterate over patches inside mbox somehow and
git apply one by one or should we apply and then rebase && amend ?
not sure how to deal whit this in a cleaner way...

>
>
> BR,
> Jani.
>
>
>>
>>
>>> +                       download_mbox $1/mbox $file
>>> +                       shift
>>> +                       ;;
>>>                 *".patch" | *".mbox")
>>>                         cat $1 > $file
>>>                         shift
>>> --
>>> 2.13.2
>>>
>>> _______________________________________________
>>> Intel-gfx mailing list
>>> Intel-gfx@lists.freedesktop.org
>>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
> --
> Jani Nikula, Intel Open Source Technology Center



-- 
Rodrigo Vivi
Blog: http://blog.vivi.eng.br
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [maintainer-tools PATCH 1/2] dim: Accept .mbox and .patch files as apply-branch optional argument.

[Rodrigo Vivi]

On Thu, Aug 17, 2017 at 12:57 AM, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Thu, Aug 17, 2017 at 10:30:02AM +0300, Jani Nikula wrote:
>> On Wed, 16 Aug 2017, Rodrigo Vivi <rodrigo.vivi@intel.com> wrote:
>> > Instead of forcing users to cat .patch or .mbox let's accept them
>> > as optional argument for dim apply-branches.
>>
>> Well, that's a useless use of cat anyway. You could do
>>
>> $ dim apply-branch branch < patch.mbox
>>
>> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
>> > Cc: Jani Nikula <jani.nikula@intel.com>
>> > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
>> > ---
>> >  dim     | 10 +++++++++-
>> >  dim.rst |  2 +-
>> >  2 files changed, 10 insertions(+), 2 deletions(-)
>> >
>> > diff --git a/dim b/dim
>> > index 11aa675cc3bc..e98d23b24ec0 100755
>> > --- a/dim
>> > +++ b/dim
>> > @@ -771,7 +771,15 @@ function dim_apply_branch
>> >     assert_branch $branch
>> >     assert_repo_clean
>> >
>> > -   cat > $file
>> > +   case $1 in
>> > +           *".patch" | *".mbox")
>> > +                   cat $1 > $file
>> > +                   shift
>> > +                   ;;
>> > +           *)
>> > +                   cat > $file
>> > +                   ;;
>> > +   esac
>>
>> This would really be a surprising interface, argument parsing based on
>> file suffixes. I don't approve.
>>
>> You'll need to make this handle options before the branch argument,
>> something like:
>>
>> Usage: dim apply-branch [apply-branch options] branch [--] [git options]
>>
>> Is stdin redirection really such a bad thing?
>
> +1 on that. If I pick it from patchwork or an mbox, I just < patch.mbox or
> curl patchwork.url | dim apply. I don't see the value in baking that into
> the script ...

oh, I had missed this before I commented on the other one.
good idea to use curl...

>
> What we could do (and we have a jira for that already) is to check
> patchwork for the updated .mbox with all the r-b/t-b/a-b tags auto-added.
> That would greatly improve at least my workflow.
>
> And I also agree with Jani on keeping the dim design clean. We can add
> stuff where it makes sense, and where dim really can add value (with
> additional safety checks and convenience features).

agree!

>
> One thing we maybe could be doing is a more fancy aliasing feature, along
> the lines of git aliases, where you can do whatever you want. Then you
> could do a dim apply-curl alias which resolves to curl $arg | dim apply.
> Not sure how to implement that though ...

I will try to use and to create my own alias and document it..



> -Daniel
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Rodrigo Vivi
Blog: http://blog.vivi.eng.br
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [maintainer-tools PATCH 2/2] dim: Accept patchwork URLs as apply-branch optional argument.

[Rodrigo Vivi]

On Thu, Aug 17, 2017 at 9:18 AM, Rodrigo Vivi <rodrigo.vivi@gmail.com> wrote:
> On Thu, Aug 17, 2017 at 12:45 AM, Jani Nikula <jani.nikula@intel.com> wrote:
>> On Wed, 16 Aug 2017, Rodrigo Vivi <rodrigo.vivi@gmail.com> wrote:
>>> On Wed, Aug 16, 2017 at 11:13 AM, Rodrigo Vivi <rodrigo.vivi@intel.com> wrote:
>>>> Instead of having to manually download mbox from patchwork
>>>> let's make dim to do it directly.
>>>>
>>>> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
>>>> Cc: Jani Nikula <jani.nikula@intel.com>
>>>> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
>>>> ---
>>>>  dim | 18 ++++++++++++++++++
>>>>  1 file changed, 18 insertions(+)
>>>>
>>>> diff --git a/dim b/dim
>>>> index e98d23b24ec0..73b48da7f436 100755
>>>> --- a/dim
>>>> +++ b/dim
>>>> @@ -756,6 +756,16 @@ function dim_push
>>>>         dim_push_branch $(git_current_branch) "$@"
>>>>  }
>>>>
>>>> +function download_mbox
>>>> +{
>>>> +       wget -q --spider ${1}
>>>> +       if [ $? -ne "0" ]; then
>>>> +           echoerr "URL ${1} not found."
>>>> +           exit 1
>>>> +       fi
>>>> +       wget -q ${1} -O $2
>>>> +}
>>>> +
>>>>  # ensure we're on branch $1, and apply patches. the rest of the arguments are
>>>>  # passed to git am.
>>>>  dim_alias_ab=apply-branch
>>>> @@ -772,6 +782,14 @@ function dim_apply_branch
>>>>         assert_repo_clean
>>>>
>>>>         case $1 in
>>>> +               *"patchwork.freedesktop.org"*"mbox")
>>>> +                       download_mbox $1 $file
>>>> +                       shift
>>>> +                       ;;
>>>> +               *"patchwork.freedesktop.org"*)
>>>
>>> Another thing that I'd like to do is to be able to give the patchwork
>>> id directly, but I don't want to mess with the $@ going to git
>>> directly so I'm not sure which way would be better...
>>
>> Personally I prefer using message-id based patchwork references:
>>
>> http://patchwork.freedesktop.org/patch/msgid/20170811113907.6716-1-jani.nikula@intel.com
>
> well remembered...
> in the end it gets converted to
> https://patchwork.freedesktop.org/patch/171432/
>
> so we can get
> https://patchwork.freedesktop.org/patch/171432/mbox
>
>>
>>> maybe parse for something like
>>> "pw="*)
>>> download_mbox ${1#pw=} $file
>>> so we could use
>>> dim aq pw=170802
>>>
>>> ?
>>> suggestions?
>>
>> The ways around this are argument parsing in apply-branch or adding
>> another dim subcommand.
>
> Usage: dim apply-branch [apply-branch options] branch [--] [git options]
>
> -i  <patchwork-id>
> -m <Message-id>
> -u <url>
>
> hmm but not sure it goes well with the aliases were we would need to convert
>
> dim aq -m 20170811113907.6716-1-jani.nikula@intel.com [git options]
> to
> dim apply-branch -m 20170811113907.6716-1-jani.nikula@intel.com
> drm-intel-next-queued [git options]

nevermind... please entirely ignore this..

I solved my own needs with:

dim_pwaq()
{
        if [ -n "$1" ]; then
                curl https://patchwork.freedesktop.org/patch/$1/mbox/
| dim_apply_queued
        else
                echo "Give me a patchwork id"
        fi
}

using as:
$ dim pwaq 172141

>
>>
>> Btw one missing piece is handling series mboxes, which do apply, but
>> only the last commit from an mbox gets all the checks and Link: tags
>> etc.
>
> yeap! well remembered! I suffered from this behaviour already and seen
> other folks also complaining about it.
>
> So, for this should we iterate over patches inside mbox somehow and
> git apply one by one or should we apply and then rebase && amend ?
> not sure how to deal whit this in a cleaner way...

this part is still valid..

>
>>
>>
>> BR,
>> Jani.
>>
>>
>>>
>>>
>>>> +                       download_mbox $1/mbox $file
>>>> +                       shift
>>>> +                       ;;
>>>>                 *".patch" | *".mbox")
>>>>                         cat $1 > $file
>>>>                         shift
>>>> --
>>>> 2.13.2
>>>>
>>>> _______________________________________________
>>>> Intel-gfx mailing list
>>>> Intel-gfx@lists.freedesktop.org
>>>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
>>
>> --
>> Jani Nikula, Intel Open Source Technology Center
>
>
>
> --
> Rodrigo Vivi
> Blog: http://blog.vivi.eng.br



-- 
Rodrigo Vivi
Blog: http://blog.vivi.eng.br
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx