Re: [PATCH v3 0/6] rebase -i: add config to abbreviate command-names

[Liam Beguin]

Hi,

On Mon, 2017-05-08 at 09:27 +0900, Junio C Hamano wrote:
> Liam Beguin  writes:
> 
> > Sorry for the delay, I don't mind switching to C but it would probably
> > be easier to see if the scripted version gets approved first.
> > If it does, I could then get started on the C implementation.
> > If you prefer I could also forget about the scripted version, make a C
> > implementation work and see if that gets approved.
> 
> I am not sure what "approved" would mean in the context of this
> project, though ;-) Your patch to the scripted version would
> certainly not be in the upcoming release.  If you define the
> "approval" as "it is queued to my tree somewhere", the patch would
> start its life like everybody else by getting merged to the 'pu'
> branch, where there already is a topic that removes the code you
> patch your enhancement into.
> 

By "approved", I guess I meant the list reaches an agreement. 

> The list _can_ agree that it is a good idea to have an option to
> populate the todo list with shortened insn words from the beginning
> (instead of merely accepting a short-hand while executing), which is
> what your patch wants to do, without actually having the updated
> scripted "rebase -i" merged in any of the integration branches in my
> tree.  If you meant by "approval" to have such a list concensus, I
> think you may already have one.  I personally do not think it is a
> great idea but I do not think it is a horrible one, either.  As long
> as it is an opt-in feature that many people find useful (which may
> be the case already, judging from the list traffic), I do not mind
> ;-)
> 

Ok, based on this, I'll send a new series based on the 'pu' branch.

Thanks again, 
Liam

Re: [PATCH v3 0/6] rebase -i: add config to abbreviate command-names

[Junio C Hamano]

Liam Beguin  writes:

> Sorry for the delay, I don't mind switching to C but it would probably
> be easier to see if the scripted version gets approved first.
> If it does, I could then get started on the C implementation.
> If you prefer I could also forget about the scripted version, make a C
> implementation work and see if that gets approved.

I am not sure what "approved" would mean in the context of this
project, though ;-) Your patch to the scripted version would
certainly not be in the upcoming release.  If you define the
"approval" as "it is queued to my tree somewhere", the patch would
start its life like everybody else by getting merged to the 'pu'
branch, where there already is a topic that removes the code you
patch your enhancement into.

The list _can_ agree that it is a good idea to have an option to
populate the todo list with shortened insn words from the beginning
(instead of merely accepting a short-hand while executing), which is
what your patch wants to do, without actually having the updated
scripted "rebase -i" merged in any of the integration branches in my
tree.  If you meant by "approval" to have such a list concensus, I
think you may already have one.  I personally do not think it is a
great idea but I do not think it is a horrible one, either.  As long
as it is an opt-in feature that many people find useful (which may
be the case already, judging from the list traffic), I do not mind
;-)

Re: [PATCH v3 0/6] rebase -i: add config to abbreviate command-names

[Liam Beguin]

Hi Junio,

On Wed, 2017-05-03 at 22:04 -0700, Junio C Hamano wrote:
> Johannes Schindelin  writes:
> 
> > > If 'git-rebase--interactive.sh' is bound to be replaced, I could
> > > just shrink this to the Documentation cleanup (patches 4 and 5)
> > > and rework the rest on top of your new implementation.
> > 
> > I kind of hoped that Junio would chime in with his verdict. That would be
> > the ultimate deciding factor, I think.
> 
> What I can predict is that within two or three release cycles
> (unless you completely lose interest) the todo-list generation will
> be all in C and that I anticipate that the C version may even be
> capable of generating different kind of todo command (e.g. to
> support tools like your Garden Shears more natively), so the
> mid-term direction definitely is that any enhancement would in the
> end needs to happen on top of or in coordination with the C rewrite
> we've been discussing recently.
> 
> I didn't know what the comfort levels of Liam working with scripted
> vs C code, and the "vertict" depends on that, I would think.  We may
> want to discuss the enhancement in the original scripted form Liam
> did with new tests while the C rewrite is still cooking, and either
> Liam, you or somebody else can make it work with your C rewrite when
> both are ready.  If Liam feels comfortable working with you and the
> code after the C rewrite that is still in-flight, it is also fine if
> the next iteration from Liam were on top of your series.
> 
> 

Sorry for the delay, I don't mind switching to C but it would probably
be easier to see if the scripted version gets approved first.
If it does, I could then get started on the C implementation.
If you prefer I could also forget about the scripted version, make a C
implementation work and see if that gets approved.

Thanks,
Liam

Re: [PATCH v3 0/6] rebase -i: add config to abbreviate command-names

[Junio C Hamano]

Johannes Schindelin  writes:

>> If 'git-rebase--interactive.sh' is bound to be replaced, I could
>> just shrink this to the Documentation cleanup (patches 4 and 5)
>> and rework the rest on top of your new implementation.
>
> I kind of hoped that Junio would chime in with his verdict. That would be
> the ultimate deciding factor, I think.

What I can predict is that within two or three release cycles
(unless you completely lose interest) the todo-list generation will
be all in C and that I anticipate that the C version may even be
capable of generating different kind of todo command (e.g. to
support tools like your Garden Shears more natively), so the
mid-term direction definitely is that any enhancement would in the
end needs to happen on top of or in coordination with the C rewrite
we've been discussing recently.

I didn't know what the comfort levels of Liam working with scripted
vs C code, and the "vertict" depends on that, I would think.  We may
want to discuss the enhancement in the original scripted form Liam
did with new tests while the C rewrite is still cooking, and either
Liam, you or somebody else can make it work with your C rewrite when
both are ready.  If Liam feels comfortable working with you and the
code after the C rewrite that is still in-flight, it is also fine if
the next iteration from Liam were on top of your series.

Re: [PATCH v3 0/6] rebase -i: add config to abbreviate command-names

[Johannes Schindelin]

Hi Liam,

On Tue, 2 May 2017, Liam Beguin wrote:

> On Tue, 2017-05-02 at 17:48 +0200, Johannes Schindelin wrote:
> 
> > I offered a couple of comments, my biggest one probably being that
> > this patch series is crossing paths with my patch series that tries to
> > move more functionality out of the git-rebase--interactive.sh script
> > into the git-rebase--helper that is written in C, closely followed by
> > my suggestion to fold at least part of the functionality into the
> > SHA-1 collapsing/expanding.
> 
> I've seen a few messages about this migration, but I'm not yet sure I
> understand the difference between the shell and the C implementations.
> Is the C version going to replace 'git-rebase--interactive.sh'?

The C version has already started to replace the shell script, yes. In
adding and using git-rebase--helper, there are already parts of the
interactive rebase functionality that are run using C code only. The idea
is to move more and more functionality over (separating out the
--preserve-merges handling into a different shell script, as I have no
plans to convert that code to C, and as far as I can see nobody else wants
to step up to that task, either). Eventually, we may be able to finish
that gigantic task of having git-rebase be all builtin C code.

> > If your patch series "wins", I can easily forward-port your changes to
> > the rebase-i-extra branch, but it may actually make sense to build on
> > top of the rebase-i-extra branch to begin with. If you agree: I pushed
> > the proposed change to the `rebase-i-extra+abbrev` branch at
> > https://github.com/dscho/git.
> 
> If 'git-rebase--interactive.sh' is bound to be replaced, I could
> just shrink this to the Documentation cleanup (patches 4 and 5)
> and rework the rest on top of your new implementation.

I kind of hoped that Junio would chime in with his verdict. That would be
the ultimate deciding factor, I think.

Ciao,
Johannes

Re: [PATCH v3 0/6] rebase -i: add config to abbreviate command-names

[Liam Beguin]

Hi Johannes, 

On Tue, 2017-05-02 at 17:48 +0200, Johannes Schindelin wrote:
> Hi Liam,
> 
> On Tue, 2 May 2017, Liam Beguin wrote:
> 
> > Add the 'rebase.abbreviateCommands' configuration option to allow `git
> > rebase -i` to default to the single-letter command-names in the todo
> > list.
> > 
> > Using single-letter command-names can present two benefits.  First, it
> > makes it easier to change the action since you only need to replace a
> > single character (i.e.: in vim "r" instead of
> > "ciw").  Second, using this with a large enough value of
> > 'core.abbrev' enables the lines of the todo list to remain aligned
> > making the files easier to read.
> > 
> > Changes from v1 to v2:
> >  - Improve Documentation and commit message
> > 
> > Changes from v2 to v3:
> >  - Transform a single patch into a series
> >  - change option name from 'rebase.abbrevCmd' to 'rebase.abbreviateCommands'
> >  - abbreviate all commands (not just pick)
> >  - teach `git rebase -i --autosquash` to recognise single-letter 
> > command-names
> >  - move rebase configuration documentation to 
> > Documentation/rebase-config.txt
> >  - update Documentation to use the preferred naming for the todo list
> >  - update Documentation and commit messages according to feedback
> 
> Thank you for this pleasant read. It is an excellent contribution, and the
> way you communicate what you changed and why is very welcome.
> 

Thanks! and thank you for the support and help.

> I offered a couple of comments, my biggest one probably being that this
> patch series is crossing paths with my patch series that tries to move
> more functionality out of the git-rebase--interactive.sh script into the
> git-rebase--helper that is written in C, closely followed by my suggestion
> to fold at least part of the functionality into the SHA-1
> collapsing/expanding.
> 

I've seen a few messages about this migration, but I'm not yet sure I understand
the difference between the shell and the C implementations. Is the C version 
going
to replace 'git-rebase--interactive.sh'?

> If your patch series "wins", I can easily forward-port your changes to the
> rebase-i-extra branch, but it may actually make sense to build on top of
> the rebase-i-extra branch to begin with. If you agree: I pushed the
> proposed change to the `rebase-i-extra+abbrev` branch at
> https://github.com/dscho/git.
> 

If 'git-rebase--interactive.sh' is bound to be replaced, I could
just shrink this to the Documentation cleanup (patches 4 and 5)
and rework the rest on top of your new implementation.

> I look forward to see this story unfold!
> 
> Ciao,
> Johannes

Thanks, 
Liam

Re: [PATCH v3 0/6] rebase -i: add config to abbreviate command-names

[Johannes Schindelin]

Hi Liam,

On Tue, 2 May 2017, Liam Beguin wrote:

> Add the 'rebase.abbreviateCommands' configuration option to allow `git
> rebase -i` to default to the single-letter command-names in the todo
> list.
> 
> Using single-letter command-names can present two benefits.  First, it
> makes it easier to change the action since you only need to replace a
> single character (i.e.: in vim "r" instead of
> "ciw").  Second, using this with a large enough value of
> 'core.abbrev' enables the lines of the todo list to remain aligned
> making the files easier to read.
> 
> Changes from v1 to v2:
>  - Improve Documentation and commit message
> 
> Changes from v2 to v3:
>  - Transform a single patch into a series
>  - change option name from 'rebase.abbrevCmd' to 'rebase.abbreviateCommands'
>  - abbreviate all commands (not just pick)
>  - teach `git rebase -i --autosquash` to recognise single-letter command-names
>  - move rebase configuration documentation to Documentation/rebase-config.txt
>  - update Documentation to use the preferred naming for the todo list
>  - update Documentation and commit messages according to feedback

Thank you for this pleasant read. It is an excellent contribution, and the
way you communicate what you changed and why is very welcome.

I offered a couple of comments, my biggest one probably being that this
patch series is crossing paths with my patch series that tries to move
more functionality out of the git-rebase--interactive.sh script into the
git-rebase--helper that is written in C, closely followed by my suggestion
to fold at least part of the functionality into the SHA-1
collapsing/expanding.

If your patch series "wins", I can easily forward-port your changes to the
rebase-i-extra branch, but it may actually make sense to build on top of
the rebase-i-extra branch to begin with. If you agree: I pushed the
proposed change to the `rebase-i-extra+abbrev` branch at
https://github.com/dscho/git.

I look forward to see this story unfold!

Ciao,
Johannes

Re: [PATCH v3 0/6] rebase -i: add config to abbreviate command-names

[Johannes Schindelin]

Hi,


On Tue, 2 May 2017, Ævar Arnfjörð Bjarmason wrote:

> I locally rebased this into just 3 patches, i.e. in this sequence:
> 
> - Documentation: move rebase.* config variables to a separate 
> rebase-config.txt
> - Documentation: use preferred name for the 'todo list' script
> - *all the rest of this squashed*

I think that makes a lot of sense. (I would drop the part about f!/s!, as
I pointed out, though)

Ciao,
Johannes

Re: [PATCH v3 0/6] rebase -i: add config to abbreviate command-names

[Ævar Arnfjörð Bjarmason]

On Tue, May 2, 2017 at 6:00 AM, Liam Beguin  wrote:
> Add the 'rebase.abbreviateCommands' configuration option to allow
> `git rebase -i` to default to the single-letter command-names in
> the todo list.
>
> Using single-letter command-names can present two benefits.
> First, it makes it easier to change the action since you only need to
> replace a single character (i.e.: in vim "r" instead of
> "ciw").
> Second, using this with a large enough value of 'core.abbrev' enables the
> lines of the todo list to remain aligned making the files easier to
> read.
>
> Changes from v1 to v2:
>  - Improve Documentation and commit message
>
> Changes from v2 to v3:
>  - Transform a single patch into a series
>  - change option name from 'rebase.abbrevCmd' to 'rebase.abbreviateCommands'
>  - abbreviate all commands (not just pick)
>  - teach `git rebase -i --autosquash` to recognise single-letter command-names
>  - move rebase configuration documentation to Documentation/rebase-config.txt
>  - update Documentation to use the preferred naming for the todo list
>  - update Documentation and commit messages according to feedback
>
> Liam Beguin (6):
>   rebase -i: add abbreviated command-names handling
>   rebase -i: add abbreviate_commands function
>   rebase -i: add short command-name in --autosquash
>   Documentation: move rebase.* config variables to a separate
> rebase-config.txt
>   Documentation: use prefered name for the 'todo list' script
>   Documentation: document the rebase.abbreviateCommands option

I locally rebased this into just 3 patches, i.e. in this sequence:

- Documentation: move rebase.* config variables to a separate rebase-config.txt
- Documentation: use preferred name for the 'todo list' script
- *all the rest of this squashed*

I think that's much less confusing than having 3x "rebase -i" patches.
If you look at any one of those you have very little context for
what's going on, and there seems to be no point in splitting them
since the end result is tiny (3 files changed, 45 insertions(+), 4
deletions(-)).

I think with that this looks good, but it also needs tests, if you
apply your series and then comment out the new calls to
abbreviate_commands all tests still pass, if you look at git-config(1)
and search for the other rebase.* commands & grep the test suite for
those you can see how they're tested for.

I don't think this needs a lot of testing since it's a rather trivial
feature, but just one test to make sure that the todo list ends up as
"p ..." "e  ..." instead of "pick ..." "exec ..." etc. would be good.

>  Documentation/config.txt| 31 +---
>  Documentation/git-rebase.txt| 21 +++-
>  Documentation/rebase-config.txt | 53 
> +
>  git-rebase--interactive.sh  | 24 
>  4 files changed, 78 insertions(+), 52 deletions(-)
>  create mode 100644 Documentation/rebase-config.txt
>
> --
> 2.9.3
>