diff mbox

[maintainer-tools] dim: Remove git commit --amend from dim_apply

Message ID 1447416339-11419-1-git-send-email-ander.conselvan.de.oliveira@intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

Ander Conselvan de Oliveira Nov. 13, 2015, 12:05 p.m. UTC
Calling git --amend invokes the editor, which will not run if it relies
on the terminal for input. So don't do that from dim_apply.

Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
---
 dim | 2 --
 1 file changed, 2 deletions(-)

Comments

Chris Wilson Nov. 13, 2015, 12:16 p.m. UTC | #1
On Fri, Nov 13, 2015 at 02:05:39PM +0200, Ander Conselvan de Oliveira wrote:
> Calling git --amend invokes the editor, which will not run if it relies
> on the terminal for input. So don't do that from dim_apply.
> 
> Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>

Presumable the ammend is there for a good reason and removing it keeps
the state dirty? So maybe --amend --no-edit?
-Chris
Tvrtko Ursulin Nov. 13, 2015, 12:31 p.m. UTC | #2
On 13/11/15 12:16, Chris Wilson wrote:
> On Fri, Nov 13, 2015 at 02:05:39PM +0200, Ander Conselvan de Oliveira wrote:
>> Calling git --amend invokes the editor, which will not run if it relies
>> on the terminal for input. So don't do that from dim_apply.
>>
>> Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
>
> Presumable the ammend is there for a good reason and removing it keeps
> the state dirty? So maybe --amend --no-edit?

In either case can also consider "test -t 0" which is like isatty(0).

Regards,

Tvrtko
Ander Conselvan de Oliveira Nov. 13, 2015, 12:33 p.m. UTC | #3
On Fri, 2015-11-13 at 12:16 +0000, Chris Wilson wrote:
> On Fri, Nov 13, 2015 at 02:05:39PM +0200, Ander Conselvan de Oliveira wrote:
> > Calling git --amend invokes the editor, which will not run if it relies
> > on the terminal for input. So don't do that from dim_apply.
> > 
> > Signed-off-by: Ander Conselvan de Oliveira <
> > ander.conselvan.de.oliveira@intel.com>
> 
> Presumable the ammend is there for a good reason and removing it keeps
> the state dirty? So maybe --amend --no-edit?

I'm fine with that, but I'd like to have a definitive answer to the question
before I resend. Maybe Daniel knows?

Ander
Ville Syrjala Nov. 13, 2015, 12:42 p.m. UTC | #4
On Fri, Nov 13, 2015 at 12:16:59PM +0000, Chris Wilson wrote:
> On Fri, Nov 13, 2015 at 02:05:39PM +0200, Ander Conselvan de Oliveira wrote:
> > Calling git --amend invokes the editor, which will not run if it relies
> > on the terminal for input. So don't do that from dim_apply.
> > 
> > Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
> 
> Presumable the ammend is there for a good reason and removing it keeps
> the state dirty? So maybe --amend --no-edit?

I don't think there's any reason other than Daniel's gvim workflow.
Jani Nikula Nov. 13, 2015, 2:33 p.m. UTC | #5
On Fri, 13 Nov 2015, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> On Fri, Nov 13, 2015 at 12:16:59PM +0000, Chris Wilson wrote:
>> On Fri, Nov 13, 2015 at 02:05:39PM +0200, Ander Conselvan de Oliveira wrote:
>> > Calling git --amend invokes the editor, which will not run if it relies
>> > on the terminal for input. So don't do that from dim_apply.
>> > 
>> > Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
>> 
>> Presumable the ammend is there for a good reason and removing it keeps
>> the state dirty? So maybe --amend --no-edit?
>
> I don't think there's any reason other than Daniel's gvim workflow.

Yes, that's it, there's no functional reason other than to fire up an
editor to edit the commit message (add r-b, etc.) This always fails for
me and I just do this in the cli separately.

Basically three options, just remove it, add tty test suggested by
Tvrtko, or add some DIM_POST_APPLY_ACTION configuration. I'm fine with
any of them.


BR,
Jani.
Ander Conselvan de Oliveira Nov. 13, 2015, 2:40 p.m. UTC | #6
On Fri, 2015-11-13 at 12:31 +0000, Tvrtko Ursulin wrote:
> On 13/11/15 12:16, Chris Wilson wrote:
> > On Fri, Nov 13, 2015 at 02:05:39PM +0200, Ander Conselvan de Oliveira wrote:
> > > Calling git --amend invokes the editor, which will not run if it relies
> > > on the terminal for input. So don't do that from dim_apply.
> > > 
> > > Signed-off-by: Ander Conselvan de Oliveira <
> > > ander.conselvan.de.oliveira@intel.com>
> > 
> > Presumable the ammend is there for a good reason and removing it keeps
> > the state dirty? So maybe --amend --no-edit?
> 
> In either case can also consider "test -t 0" which is like isatty(0).

I think that will always evaluate to false since you are supposed to cat the
mbox to dim apply.

Ander
Ville Syrjala Nov. 13, 2015, 2:42 p.m. UTC | #7
On Fri, Nov 13, 2015 at 04:33:50PM +0200, Jani Nikula wrote:
> On Fri, 13 Nov 2015, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> > On Fri, Nov 13, 2015 at 12:16:59PM +0000, Chris Wilson wrote:
> >> On Fri, Nov 13, 2015 at 02:05:39PM +0200, Ander Conselvan de Oliveira wrote:
> >> > Calling git --amend invokes the editor, which will not run if it relies
> >> > on the terminal for input. So don't do that from dim_apply.
> >> > 
> >> > Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
> >> 
> >> Presumable the ammend is there for a good reason and removing it keeps
> >> the state dirty? So maybe --amend --no-edit?
> >
> > I don't think there's any reason other than Daniel's gvim workflow.
> 
> Yes, that's it, there's no functional reason other than to fire up an
> editor to edit the commit message (add r-b, etc.) This always fails for
> me and I just do this in the cli separately.
> 
> Basically three options, just remove it, add tty test suggested by
> Tvrtko, or add some DIM_POST_APPLY_ACTION configuration. I'm fine with
> any of them.

I don't want to amend the commit at this point in my workflow. "dim xt"
(should upstream that somehow one of these days...) is what I do after
"dim aq", so anything that fires up an editor is no good for me.
Tvrtko Ursulin Nov. 13, 2015, 3:07 p.m. UTC | #8
On 13/11/15 14:40, Ander Conselvan De Oliveira wrote:
> On Fri, 2015-11-13 at 12:31 +0000, Tvrtko Ursulin wrote:
>> On 13/11/15 12:16, Chris Wilson wrote:
>>> On Fri, Nov 13, 2015 at 02:05:39PM +0200, Ander Conselvan de Oliveira wrote:
>>>> Calling git --amend invokes the editor, which will not run if it relies
>>>> on the terminal for input. So don't do that from dim_apply.
>>>>
>>>> Signed-off-by: Ander Conselvan de Oliveira <
>>>> ander.conselvan.de.oliveira@intel.com>
>>>
>>> Presumable the ammend is there for a good reason and removing it keeps
>>> the state dirty? So maybe --amend --no-edit?
>>
>> In either case can also consider "test -t 0" which is like isatty(0).
>
> I think that will always evaluate to false since you are supposed to cat the
> mbox to dim apply.

Question is then how does it work for Daniel. :)

Regards,

Tvrtko
Ander Conselvan de Oliveira Nov. 13, 2015, 3:11 p.m. UTC | #9
On Fri, 2015-11-13 at 15:07 +0000, Tvrtko Ursulin wrote:
> On 13/11/15 14:40, Ander Conselvan De Oliveira wrote:
> > On Fri, 2015-11-13 at 12:31 +0000, Tvrtko Ursulin wrote:
> > > On 13/11/15 12:16, Chris Wilson wrote:
> > > > On Fri, Nov 13, 2015 at 02:05:39PM +0200, Ander Conselvan de Oliveira
> > > > wrote:
> > > > > Calling git --amend invokes the editor, which will not run if it
> > > > > relies
> > > > > on the terminal for input. So don't do that from dim_apply.
> > > > > 
> > > > > Signed-off-by: Ander Conselvan de Oliveira <
> > > > > ander.conselvan.de.oliveira@intel.com>
> > > > 
> > > > Presumable the ammend is there for a good reason and removing it keeps
> > > > the state dirty? So maybe --amend --no-edit?
> > > 
> > > In either case can also consider "test -t 0" which is like isatty(0).
> > 
> > I think that will always evaluate to false since you are supposed to cat the
> > mbox to dim apply.
> 
> Question is then how does it work for Daniel. :)

If I understood correctly he uses gvim, which doesn't run on a terminal.

Ander
Daniel Vetter Nov. 18, 2015, 3:51 p.m. UTC | #10
On Fri, Nov 13, 2015 at 05:11:58PM +0200, Ander Conselvan De Oliveira wrote:
> On Fri, 2015-11-13 at 15:07 +0000, Tvrtko Ursulin wrote:
> > On 13/11/15 14:40, Ander Conselvan De Oliveira wrote:
> > > On Fri, 2015-11-13 at 12:31 +0000, Tvrtko Ursulin wrote:
> > > > On 13/11/15 12:16, Chris Wilson wrote:
> > > > > On Fri, Nov 13, 2015 at 02:05:39PM +0200, Ander Conselvan de Oliveira
> > > > > wrote:
> > > > > > Calling git --amend invokes the editor, which will not run if it
> > > > > > relies
> > > > > > on the terminal for input. So don't do that from dim_apply.
> > > > > > 
> > > > > > Signed-off-by: Ander Conselvan de Oliveira <
> > > > > > ander.conselvan.de.oliveira@intel.com>
> > > > > 
> > > > > Presumable the ammend is there for a good reason and removing it keeps
> > > > > the state dirty? So maybe --amend --no-edit?
> > > > 
> > > > In either case can also consider "test -t 0" which is like isatty(0).
> > > 
> > > I think that will always evaluate to false since you are supposed to cat the
> > > mbox to dim apply.
> > 
> > Question is then how does it work for Daniel. :)
> 
> If I understood correctly he uses gvim, which doesn't run on a terminal.

Yup. And I use git commit --amend to paste in all the r-b/t-b tags and all
that stuff. Maybe we need a DIM_POST_APPLY_CMD in .dimrc?
-Daniel
diff mbox

Patch

diff --git a/dim b/dim
index db92c57..893906b 100755
--- a/dim
+++ b/dim
@@ -382,8 +382,6 @@  function dim_apply
 	if [ -n $message_id ]; then
 		commit_add_tag "Link" "http://patchwork.freedesktop.org/patch/msgid/$message_id"
 	fi
-
-	git commit --amend &
 }
 
 function magic_patch