Re: [PATCH 1/3] git-compat-util: introduce skip_to_opt_val()

[Christian Couder]

On Sun, Dec 3, 2017 at 11:48 PM, Junio C Hamano  wrote:
> Christian Couder  writes:
>
>> Anyway there is a design choice to be made. Adding a "const char
>> *default" argument makes the function more generic,...
>
> I didn't suggest anything of that sort, and I do not understand why
> you are repeatedly talking about "default" that you considered and
> rejected, as if it were an alternative viable option.  I agree that
> "default" is not yet a good idea and it is a solution to a problem
> that is not yet shown to exist.
>
> On the other hand, just assigning NULL to *arg when you did not see
> a delimiting '=', on the other hand, is an alternative option that
> is viable.

What I am saying is that I'd rather have a lot of code like:

if (skip_to_optional_val(arg, "--key", , "") /* the last
argument is the default value */
do_something(arg);

than a lot of code like this:

if (skip_to_optional_val(arg, "--key", ) /* no default can
be passed, NULL is the default */
do_something(arg ? arg : "");

because in the former case the `arg ? arg : ""` pattern is captured
inside the function, so the code is simpler.

In the few cases where do_something() accepts NULL and does something
different with it, the former can be changed to:

if (skip_to_optional_val(arg, "--key", , NULL) /* the last
argument is the default value */
do_something(arg);

So yeah I rejected it, but my preference is not strong and I never
said or thought that it was not viable. I just think that there are
few cases that might benefit. So the benefits are not big and it might
be better for consistency and simplicity of the UI to nudge people to
make "--key" and "--key=" behave the same. That's why having "" as the
default and no default argument is a little better in my opinion.

>>  I think setting
>> "arg" to NULL increases the risk of crashes and makes it too easy to
>> make "--key" and "--key=" behave differently which I think is not
>> consistent and not intuitive.
>
> So now this is very specific to the need of command line argument
> parsing and is not a generic thing?  You cannot have your cake and
> eat it too, though.

I think that even when we are not parsing options, it is probably good
in general for UI consistency and simplicity that "key" and "key=" are
interpreted in the same way.

AW: [PATCH v6 1/2] http-backend: respect CONTENT_LENGTH as specified by rfc3875

[Florian Manschwetus]

Hi All,
I could provide a bash script I used in between to make this working with IIS, 
without fixing http-backend binary, maybe this helps to understand how this 
cases might be handled.

Mit freundlichen Grüßen / With kind regards
Florian Manschwetus



> -Ursprüngliche Nachricht-
> Von: Junio C Hamano [mailto:gits...@pobox.com]
> Gesendet: Sonntag, 3. Dezember 2017 07:07
> An: Jeff King
> Cc: Max Kirillov; Eric Sunshine; Florian Manschwetus; Chris Packham;
> Konstantin Khomoutov; git@vger.kernel.org
> Betreff: Re: [PATCH v6 1/2] http-backend: respect CONTENT_LENGTH as
> specified by rfc3875
> 
> Jeff King  writes:
> 
> > ... Eventually our fill() will block trying to get data that is not
> > there. On an Apache server, the webserver would know there is nothing
> > left to send us and close() the pipe, and we'd get EOF.
> > But on IIS, I think the pipe remains open and we'd just block
> > indefinitely trying to read().
> 
> Ah, yeah, under that scenario, trusting content-length and trying to read,
> waiting for input that would never come, will be a problem, and it would
> probably want to get documented.

Re: [PATCH v5 4/4] builtin/branch: strip refs/heads/ using skip_prefix

[SZEDER Gábor]

On Fri, Dec 1, 2017 at 6:59 AM, Kaartic Sivaraam
 wrote:
> Sorry, missed a ';' in v4.
>
> The surprising thing I discovered in the TravisCI build for v4
> was that apart from the 'Documentation' build the 'Static Analysis'
> build passed, with the following output,
>
> -- 
> $ ci/run-static-analysis.sh
> GIT_VERSION = 2.13.1.1972.g6ced3f745
>  SPATCH contrib/coccinelle/array.cocci
>  SPATCH result: contrib/coccinelle/array.cocci.patch
>  SPATCH contrib/coccinelle/free.cocci
>  SPATCH contrib/coccinelle/object_id.cocci
>  SPATCH contrib/coccinelle/qsort.cocci
>  SPATCH contrib/coccinelle/strbuf.cocci
>  SPATCH result: contrib/coccinelle/strbuf.cocci.patch
>  SPATCH contrib/coccinelle/swap.cocci
>  SPATCH contrib/coccinelle/xstrdup_or_null.cocci
>
> The command "ci/run-static-analysis.sh" exited with 0.

Perhaps Coccinelle should have errored out, or perhaps its 0 exit code
means "I didn't find any code matching any of the semantic patches that
required transformation".

> I guess static analysis tools make an assumption that the source
> code is syntactically valid for them to work correctly. So, I guess
> we should at least make sure the code 'compiles' before running
> the static analysis tool even though we don't build it completely.
> I'm not sure if it's a bad thing to run the static analysis on code
> that isn't syntactically valid, though.

Travis CI already runs 6 build jobs compiling Git.  And that is in
addition to the one that you should have run yourself before even
thinking about submitting v4 ;)  That's plenty to catch errors like
these.  And if any of those builds fail because Git can't be built or
because of a test failure, then Coccinelle's success doesn't matter at
all, because the commit is toast anyway.


Gábor

Re: [SCRIPT/RFC 0/3] git-commit --onto-parent (three-way merge, no working tree file changes)

[Igor Djordjevic]

Hi Hannes,

On 01/12/2017 18:23, Johannes Sixt wrote:
> 
> > To work with `--onto-parent` and be able to commit on top of any of
> > the topic branches, you would need a situation like this instead:
> >
> >   (1)  ...C  <- topic C
> >   |
> >  ...A |  <- topic A
> >  \|
> >...o I<- integration
> >  /|
> >  ...B |  <- topic B
> >   |
> >...D  <- topic D
> 
> This is a very, VERY exotic workflow, I would say. How would you
> construct commit I when three or more topics have conflicts?
> merge-octopus does not support this use-case.

But I`m not interested in constructing commit I in the first place, 
only help working with it once it`s already there (which shouldn`t be 
too uncommon in case of unrelated, non-conflicting topics) - but I 
already agreed my example could have been a bit too esoteric, 
distracting attention from the important part :)

I`ll continue on this further below, where you commented on those 
more common scenarios.

Here, let`s try to look at the whole situation from a "patch queue" 
perspective instead, starting from something like this:

(5) ---O---I  <- integration <- HEAD

... and then making progress like this - first, commit A1 onto O, 
starting "topicA" branch at the same time, even, maybe using syntax 
like `git commit --onto-parent O -b topicA`:

(6) ---O---J  <- integration <- HEAD   [ J = I + A1 ]
\ /
 A1   <- topicA

..., then commit B1 onto O to start "topicB" branch:

(7)  B1   <- topicB
/ \
---O---K  <- integration <- HEAD   [ K = J + B1 ]
\ /
 A1   <- topicA

..., then add one more commit (patch) onto B1:

(8)  B1---B2   <- topicB
/  \
---OL  <- integration <- HEAD  [ L = K + B2 ]
\  /
 A1---/<- topicA

..., and one more, B3:

(9)  B1---B2---B3   <- topicB
/   \
---O-M  <- integration <- HEAD [ M = L + B3 ]
\   /
 A1/<- topicA

We can also start a new topic branch (queue), commit C1:

(10) B1---B2---B3   <- topicB
/   \
---O-N  <- integration <- HEAD [ N = M + C1 ]
   |\   /|
   | A1/ /  <- topicA
   \/
C1-/<- topicC

And lets make one more "topicA" related commit A2:

(11) B1---B2---B3   <- topicB
/   \
---O-P  <- integration <- HEAD [ P = N + A2 ]
   |\   /|
   | A1---A2---/ /  <- topicA
   \/
C1-/<- topicC


Notice how HEAD never leaves "integration" branch, and underlying 
commit is recreated each time? It`s like a live branch we`re working 
on, but we`re not actually committing to.

No branch switching (and no working tree file changes caused by it), 
and we`re working on multiple topics/branches simultaneously, being 
able to instantly test their mutual interaction as we go, but also 
creating their separate (and "clean") histories at the same time.

I guess this would make most sense with simple topics we _could_ 
practically work on at the same time without making our life too 
complicated - what stands for "git add --patch", too, when working on 
multiple commits at the same time.

Once satisfied, of course each topic branch would need to be tested 
separately, and each commit, even - all the same as with "git add 
--patch" commits.

And "git add --patch" can still be used here, too, to distribute 
partial changes, currently existing together inside the working tree, 
to different topic branches, at the time of making the commit itself.

Does this approach make more sense in regards to "git commit 
--onto-parent" functionality I`m having in mind? Or I`m dreaming too 
much here...? :)

> > Once there, starting from your initial position:
> >
> > > ...A...C<- topics A, C
> > > \   \ E
> > >   ---o---o---o---o I<- integration <- HEAD
> > > /   /
> > > ...B...D<- topics B, D
> >
> > ... and doing something like `git commit --onto B --merge` would
> > yield:
> > 
> > (3) ...A...C<- topics A, C
> >   \   \ E
> > ---o---o---o---o I'   <- integration
> >   /   /|
> >   ...B...D |  <- topic D
> >   \|
> >f---'  <- topic B
> >
> > ... where (I' = I + f) is still true.
> 
> I am not used to this picture. I would not think that it is totally
> unacceptable, but it still has a hmm-factor.

Main idea is not to pile up uninteresting merge commits inside 
(throwaway) integration branch, needlessly complicating history, but 
pretend as we just made the integration merge, where fixup commit f 
was already existing in its topic branch prior the merge.

> > If that`s preferred in some
> > cases, it could even look like this 

RE: [RFE] Inverted sparseness

[Randall S. Becker]

On December 3, 2017 6:14 PM, Philip Oakley wrote a nugget of wisdom: 
>From: "Randall S. Becker" 
>Sent: Friday, December 01, 2017 6:31 PM
>> On December 1, 2017 1:19 PM, Jeff Hostetler wrote:
>>>On 12/1/2017 12:21 PM, Randall S. Becker wrote:
 I recently encountered a really strange use-case relating to sparse 
 clone/fetch that is really backwards from the discussion that has 
 been going on, and well, I'm a bit embarrassed to bring it up, but I 
 have no good solution including building a separate data store that 
 will end up inconsistent with repositories (a bad solution).  The 
 use-case is as
 follows:

 Given a backbone of multiple git repositories spread across an 
 organization with a server farm and upstream vendors.
 The vendor delivers code by having the client perform git pull into 
 a specific branch.
 The customer may take the code as is or merge in customizations.
 The vendor wants to know exactly what commit of theirs is installed 
 on each server, in near real time.
 The customer is willing to push the commit-ish to the vendor's 
 upstream repo but does not want, by default, to share the actual 
 commit contents for security reasons.
 Realistically, the vendor needs to know that their own commit id was 
 put somewhere (process exists to track this, so not part of the 
 use-case) and whether there is a subsequent commit contributed >by 
 the customer, but the content is not relevant initially.

 After some time, the vendor may request the commit contents from the 
 customer in order to satisfy support requirements - a.k.a. a defect 
 was found but has to be resolved.
 The customer would then perform a deeper push that looks a lot like 
 a "slightly" symmetrical operation of a deep fetch following a prior 
 sparse fetch to supply the vendor with the specific commit(s).
>>
>>>Perhaps I'm not understanding the subtleties of what you're 
>>>describing, but could you do this with stock git functionality.
>>
>>>Let the vendor publish a "well known branch" for the client.
>>>Let the client pull that and build.
>>>Let the client create a branch set to the same commit that they fetched.
>>>Let the client push that branch as a client-specific branch to the 
>>>vendor to indicate that that is the official release they are based on.
>>
>>>Then the vendor would know the official commit that the client was using.
>> This is the easy part, and it doesn't require anything sparse to exist.
>>
>>>If the client makes local changes, does the vendor really need the SHA 
>>>of those -- without the actual content?
>>>I mean any SHA would do right?  Perhaps let the client create a second 
>>>client-specific branch (set to  the same commit as the first) to 
>>>indicate they had mods.
>>>Later, when the vendor needs the actual client changes, the client 
>>>does a normal push to this 2nd client-specific branch at the vendor.
>>>This would send everything that the client has done to the code since 
>>>the official release.
>>
>> What I should have added to the use-case was that there is a strong 
>> audit requirement (regulatory, actually) involved that the SHA is 
>> exact, immutable, and cannot be substitute or forged (one of the 
>> reasons git is in such high regard). So, no I can't arrange a fake SHA 
>> to represent a SHA to be named later. It SHA of the installed commit 
>> is part of the official record of what happened on the specific server, so 
>> I'm stuck with it.
>>
>>>I'm not sure what you mean about "it is inside a tree".
>>
>> m---a---b---c---H1
>>  `---d---H2
>>
>> d would be at a head. b would be inside. Determining content of c is 
>> problematic if b is sparse, so I'm really unsure that any of this is 
>> possible.

>I think I get the jist of your use case. Would I be right that you don't have 
>a true working
>solution yet? i.e. that it's a problem that is almost sorted but falls down at 
>the last step.

>If one pretended that this was a single development shop, and the various 
>vendors, clients
>and customers as being independent devolopers, each of whom is over protective 
>of their
>code, it may give a better view that maps onto classic feature development 
>diagrams.
>(i.e draw the answer for local devs, then mark where the splits happen)

>In particular, I think you could use a notional regulator's view that the 
>whole code base is
>part of a large Git heirarchy of branches and merges, and that some of the 
>feature loops
>are only available via the particular developer that worked on that feature.

>This would mean that from a regulatory overview there is a merge commit in the 
>'main'
>(master) heirachy that has the main and feature commits listed, and the 
>feature commit
>is probably an --allow-empty commit (that has an empty tree if they are that 
>paranoid) that
>says 'function X released' (and probably tagged), and that release 

Re: [PATCH] send-email: extract email-parsing code into a subroutine

[Nathan PAYRE]

I've tested your code, and after few changes it's works perfectly!
The code looks better now.
Thanks a lot for your review.

2017-12-03 23:00 GMT+01:00 Ævar Arnfjörð Bjarmason :
>
> On Sat, Dec 02 2017, Payre Nathan jotted:
>
>> From: Nathan Payre 
>>
>> The existing code mixes parsing of email header with regular
>> expression and actual code. Extract the parsing code into a new
>> subroutine 'parse_header_line()'. This improves the code readability
>> and make parse_header_line reusable in other place.
>>
>> Signed-off-by: Nathan Payre 
>> Signed-off-by: Matthieu Moy 
>> Signed-off-by: Timothee Albertin 
>> Signed-off-by: Daniel Bensoussan 
>> ---
>>
>> This patch is a first step to implement a new feature.
>> See new feature discussion here: 
>> https://public-inbox.org/git/20171030223444.5052-1-nathan.pa...@etu.univ-lyon1.fr/
>>
>>  git-send-email.perl | 106 
>> +++-
>>  1 file changed, 80 insertions(+), 26 deletions(-)
>>
>> diff --git a/git-send-email.perl b/git-send-email.perl
>> index 2208dcc21..98c2e461c 100755
>> --- a/git-send-email.perl
>> +++ b/git-send-email.perl
>> @@ -715,41 +715,64 @@ EOT3
>>   if (!defined $compose_encoding) {
>>   $compose_encoding = "UTF-8";
>>   }
>> - while(<$c>) {
>> +
>> + my %parsed_email;
>> + while (<$c>) {
>>   next if m/^GIT:/;
>> - if ($in_body) {
>> - $summary_empty = 0 unless (/^\n$/);
>> - } elsif (/^\n$/) {
>> - $in_body = 1;
>> - if ($need_8bit_cte) {
>> + parse_header_line($_, \%parsed_email);
>> + if (/^\n$/i) {
>> + while (my $row = <$c>) {
>> + if (!($row =~ m/^GIT:/)) {
>> + $parsed_email{'body'} = 
>> $parsed_email{'body'} . $row;
>> + }
>> + }
>> + }
>> + }
>> + if ($parsed_email{'from'}) {
>> + $sender = $parsed_email{'from'};
>> + }
>> + if ($parsed_email{'in_reply_to'}) {
>> + $initial_reply_to = $parsed_email{'in_reply_to'};
>> + }
>> + if ($parsed_email{'subject'}) {
>> + $initial_subject = $parsed_email{'subject'};
>> + print $c2 "Subject: " .
>> + quote_subject($parsed_email{'subject'}, 
>> $compose_encoding) .
>> + "\n";
>> + }
>> + if ($parsed_email{'mime-version'}) {
>> + $need_8bit_cte = 0;
>> + }
>> + if ($need_8bit_cte) {
>> + if ($parsed_email{'content-type'}) {
>> + print $c2 "MIME-Version: 1.0\n",
>> +  "Content-Type: 
>> $parsed_email{'content-type'};",
>> +  "Content-Transfer-Encoding: 8bit\n";
>> + } else {
>>   print $c2 "MIME-Version: 1.0\n",
>>"Content-Type: text/plain; ",
>> -"charset=$compose_encoding\n",
>> +  "charset=$compose_encoding\n",
>>"Content-Transfer-Encoding: 8bit\n";
>>   }
>> - } elsif (/^MIME-Version:/i) {
>> - $need_8bit_cte = 0;
>> - } elsif (/^Subject:\s*(.+)\s*$/i) {
>> - $initial_subject = $1;
>> - my $subject = $initial_subject;
>> - $_ = "Subject: " .
>> - quote_subject($subject, $compose_encoding) .
>> - "\n";
>> - } elsif (/^In-Reply-To:\s*(.+)\s*$/i) {
>> - $initial_reply_to = $1;
>> - next;
>> - } elsif (/^From:\s*(.+)\s*$/i) {
>> - $sender = $1;
>> - next;
>> - } elsif (/^(?:To|Cc|Bcc):/i) {
>> - print __("To/Cc/Bcc fields are not interpreted yet, 
>> they have been ignored\n");
>> - next;
>> - }
>> - print $c2 $_;
>>   }
>> + if ($parsed_email{'body'}) {
>> + $summary_empty = 0;
>> + print $c2 "\n$parsed_email{'body'}\n";
>> + }
>> +
>>   close $c;
>>   close $c2;
>>
>> + open $c2, "<", $compose_filename . ".final"
>> + or die sprintf(__("Failed to open %s.final: %s"), 
>> $compose_filename, $!);
>> +
>> + print "affichage : \n";
>> + while (<$c2>) {
>> + print $_;
>> + }
>> +
>> + close $c2;
>> +
>>   if ($summary_empty) {
>>   print __("Summary email is empty, skipping it\n");

Re: bug deleting "unmerged" branch (2.12.3)

[Philip Oakley]

From: "Junio C Hamano" 

"Philip Oakley"  writes:


I think it was that currently you are on M, and neither A nor B are
ancestors (i.e. merged) of M.

As Junio said:- "branch -d" protects branches that are yet to be
merged to the **current branch**.


Actually, I think people loosened this over time and removal of
branch X is not rejected even if the range HEAD..X is not empty, as
long as X is marked to integrate with/build on something else with
branch.X.{remote,merge} and the range X@{upstream}..X is empty.

So the stress of "current branch" above you added is a bit of a
white lie.


Ah, thanks. [I haven't had chance to check the code]

The man page does say:
.-d
.Delete a branch. The branch must be fully merged in its upstream
.branch, or in HEAD if no upstream was set with --track 
.or --set-upstream.


It's whether or not Ulrich had joined the two aspects together, and if the
doc was sufficient to help recognise the 'unmerged' issue. Ulrich?
--
Philip

Re: [RFE] Inverted sparseness

[Philip Oakley]

From: "Randall S. Becker" 
Sent: Friday, December 01, 2017 6:31 PM

On December 1, 2017 1:19 PM, Jeff Hostetler wrote:

On 12/1/2017 12:21 PM, Randall S. Becker wrote:
I recently encountered a really strange use-case relating to sparse 
clone/fetch that is really backwards from the discussion that has been 
going on, and well, I'm a bit embarrassed to bring it up, but I have no 
good solution including building a separate data store that will end up 
inconsistent with repositories (a bad solution).  The use-case is as 
follows:


Given a backbone of multiple git repositories spread across an 
organization with a server farm and upstream vendors.
The vendor delivers code by having the client perform git pull into a 
specific branch.

The customer may take the code as is or merge in customizations.
The vendor wants to know exactly what commit of theirs is installed on 
each server, in near real time.
The customer is willing to push the commit-ish to the vendor's upstream 
repo but does not want, by default, to share the actual commit contents 
for security reasons.
Realistically, the vendor needs to know that their own commit id was put 
somewhere (process exists to track this, so not part of the use-case) 
and whether there is a subsequent commit contributed >by the customer, 
but the content is not relevant initially.


After some time, the vendor may request the commit contents from the 
customer in order to satisfy support requirements - a.k.a. a defect was 
found but has to be resolved.
The customer would then perform a deeper push that looks a lot like a 
"slightly" symmetrical operation of a deep fetch following a prior 
sparse fetch to supply the vendor with the specific commit(s).


Perhaps I'm not understanding the subtleties of what you're describing, 
but could you do this with stock git functionality.



Let the vendor publish a "well known branch" for the client.
Let the client pull that and build.
Let the client create a branch set to the same commit that they fetched.
Let the client push that branch as a client-specific branch to the vendor 
to indicate that that is the official release they are based on.



Then the vendor would know the official commit that the client was using.

This is the easy part, and it doesn't require anything sparse to exist.

If the client makes local changes, does the vendor really need the SHA of 
those -- without the actual content?
I mean any SHA would do right?  Perhaps let the client create a second 
client-specific branch (set to

the same commit as the first) to indicate they had mods.
Later, when the vendor needs the actual client changes, the client does a 
normal push to this 2nd client-specific branch at the vendor.
This would send everything that the client has done to the code since the 
official release.


What I should have added to the use-case was that there is a strong audit 
requirement (regulatory, actually) involved that the SHA is exact, 
immutable, and cannot be substitute or forged (one of the reasons git is 
in such high regard). So, no I can't arrange a fake SHA to represent a SHA 
to be named later. It SHA of the installed commit is part of the official 
record of what happened on the specific server, so I'm stuck with it.



I'm not sure what you mean about "it is inside a tree".


m---a---b---c---H1
 `---d---H2

d would be at a head. b would be inside. Determining content of c is 
problematic if b is sparse, so I'm really unsure that any of this is 
possible.


Cheers,
Randall

-- Brief whoami: NonStop developer since approximately 
UNIX(421664400)/NonStop(2112884442)

-- In my real life, I talk too much.


I think I get the jist of your use case. Would I be right that you don't 
have a true working solution yet? i.e. that it's a problem that is almost 
sorted but falls down at the last step.


If one pretended that this was a single development shop, and the various 
vendors, clients and customers as being independent devolopers, each of whom 
is over protective of their code, it may give a better view that maps onto 
classic feature development diagrams. (i.e draw the answer for local devs, 
then mark where the splits happen)


In particular, I think you could use a notional regulator's view that the 
whole code base is part of a large Git heirarchy of branches and merges, and 
that some of the feature loops are only available via the particular 
developer that worked on that feature.


This would mean that from a regulatory overview there is a merge commit in 
the 'main' (master) heirachy that has the main and feature commits listed, 
and the feature commit is probably an --allow-empty commit (that has an 
empty tree if they are that paranoid) that says 'function X released' (and 
probably tagged), and that release commit then has, as its parent, the true 
release commit, with the true code tree. The latter commit isn't actually 
being shown to you!


At this point the potential for using 

Re: [SCRIPT/RFC 0/3] git-commit --onto-parent (three-way merge, no working tree file changes)

[Igor Djordjevic]

Hi Chris,

On 30/11/2017 23:40, Chris Nerwert wrote:
> 
> I'm actually doing the described workflow quite often with git rebase
> when working on a topic. Given the following structure:
> 
>   ---o   (master)
>   \
>o---A---B---C (topic)
> 
> When I want to make changes to commit A one option is to make them
> directly on topic, then do "git commit --fixup A", and then eventual
> interactive rebase onto master will clean them up:
> 
>   ---o (master)
>   \
>o---A---B---C---f!A (topic)
> 
> However, sometimes this breaks when changes in B or C conflict
> somehow with A (which may happen quite a lot during development of a
> topic), so the rebase will not apply cleanly. So sometimes I make a
> temporary branch from A, commit the fixup there:
> 
>   ---o   (master)
>   \
>o---A---B---C (topic)
> \
>  f!A (temp)
> 
> and then use "git rebase --onto temp A topic" to move the topic back
> on track:
> 
>   ---o (master)
>   \
>o---A---f!A (temp)
>   \
>B'---C' (topic)
> 
> after which the final cleanup rebase is much easier to do.
> 
> Obviously, all the branch switching and rebasing does take its tall
> on file modifications.

>From use case you described (and which I often experience myself), it 
seems plain "git commit --onto A" would be of help here, committing 
fixup onto A directly, without a need to switch to it (branch or 
not), a case I`m discussing with Hannes in that other sub-thread[1] of 
this e-mail, too.

But from there, your flow takes a different direction, using rebase, 
while this whole thread started around some merge-like functionality.

I can imagine a user interface doing what you (and I) would like, 
something like:

(1) git commit --onto A --rebase

..., where your changes would first be committed onto commit A, and 
then commits from A (excluded) to HEAD (included) rebased onto this 
new commit.

BUT, as far as it seems to me, rebase currently touches working tree 
for each operation (am I wrong here?), so once the rebase sequence is 
initiated, it would internally still need to checkout to your new 
fixup commit (on top of A), and then proceed applying changes and 
changing working tree with each commit being rebased, overall failing 
to address your main concern - needless (untouched) file 
modifications, even in case of no conflicts.

I find this scenario quite interesting as well, but I`m afraid it may 
currently be out of scope of what I`m trying to accomplish with "git 
commit --onto[-parent]", for the most part because it looks like it 
would need "index only rebase" first (not touching working tree, that 
is)...?

If we had that, it would/should be pretty easy to add it into the mix 
with "git commit --onto" here, ending up with something as imagined 
in command line (1) above :) I`ll make a note of it, thanks.

Any further help appreciated, of course :)

Regards, Buga

[1] 
https://public-inbox.org/git/0f30bef8-a9f7-43b9-bc89-4b9cd7af3...@gmail.com/T/#me830a80d745df60ae8bd6a2e67eee4bd4dabf56c

Re: [PATCH 1/3] git-compat-util: introduce skip_to_opt_val()

[Junio C Hamano]

Christian Couder  writes:

> Anyway there is a design choice to be made. Adding a "const char
> *default" argument makes the function more generic,...

I didn't suggest anything of that sort, and I do not understand why
you are repeatedly talking about "default" that you considered and
rejected, as if it were an alternative viable option.  I agree that
"default" is not yet a good idea and it is a solution to a problem
that is not yet shown to exist.  

On the other hand, just assigning NULL to *arg when you did not see
a delimiting '=', on the other hand, is an alternative option that
is viable.

>  I think setting
> "arg" to NULL increases the risk of crashes and makes it too easy to
> make "--key" and "--key=" behave differently which I think is not
> consistent and not intuitive.

So now this is very specific to the need of command line argument
parsing and is not a generic thing?  You cannot have your cake and
eat it too, though.

[PATCH v2 9/9] t3404: add test case for abbreviated commands

[Liam Beguin]

Make sure the todo list ends up using single-letter command
abbreviations when the rebase.abbreviateCommands is enabled.
This configuration option should not change anything else.

Signed-off-by: Liam Beguin 
---
 t/t3404-rebase-interactive.sh | 22 ++
 1 file changed, 22 insertions(+)

diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index 6a82d1ed876d..481a3500900d 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -1260,6 +1260,28 @@ test_expect_success 'rebase -i respects 
rebase.missingCommitsCheck = error' '
test B = $(git cat-file commit HEAD^ | sed -ne \$p)
 '
 
+test_expect_success 'respects rebase.abbreviateCommands with fixup, squash and 
exec' '
+   rebase_setup_and_clean abbrevcmd &&
+   test_commit "first" file1.txt "first line" first &&
+   test_commit "second" file1.txt "another line" second &&
+   test_commit "fixup! first" file2.txt "first line again" first_fixup &&
+   test_commit "squash! second" file1.txt "another line here" 
second_squash &&
+   cat >expected <<-EOF &&
+   p $(git rev-list --abbrev-commit -1 first) first
+   f $(git rev-list --abbrev-commit -1 first_fixup) fixup! first
+   x git show HEAD
+   p $(git rev-list --abbrev-commit -1 second) second
+   s $(git rev-list --abbrev-commit -1 second_squash) squash! second
+   x git show HEAD
+   EOF
+   git checkout abbrevcmd &&
+   set_cat_todo_editor &&
+   test_config rebase.abbreviateCommands true &&
+   test_must_fail git rebase -i --exec "git show HEAD" \
+   --autosquash master >actual &&
+   test_cmp expected actual
+'
+
 test_expect_success 'static check of bad command' '
rebase_setup_and_clean bad-cmd &&
set_fake_editor &&
-- 
2.15.1.280.g10402c1f5b5c

[PATCH v2 6/9] rebase -i: update functions to use a flags parameter

[Liam Beguin]

Update functions used in the rebase--helper so that they take a generic
'flags' parameter instead of a growing list of options.

Signed-off-by: Liam Beguin 
---
 builtin/rebase--helper.c | 13 +++--
 sequencer.c  |  9 +
 sequencer.h  |  8 +---
 3 files changed, 17 insertions(+), 13 deletions(-)

diff --git a/builtin/rebase--helper.c b/builtin/rebase--helper.c
index af0f91164fd0..fe814bf7229e 100644
--- a/builtin/rebase--helper.c
+++ b/builtin/rebase--helper.c
@@ -12,7 +12,7 @@ static const char * const builtin_rebase_helper_usage[] = {
 int cmd_rebase__helper(int argc, const char **argv, const char *prefix)
 {
struct replay_opts opts = REPLAY_OPTS_INIT;
-   int keep_empty = 0;
+   unsigned flags = 0, keep_empty = 0;
enum {
CONTINUE = 1, ABORT, MAKE_SCRIPT, SHORTEN_OIDS, EXPAND_OIDS,
CHECK_TODO_LIST, SKIP_UNNECESSARY_PICKS, REARRANGE_SQUASH
@@ -48,16 +48,17 @@ int cmd_rebase__helper(int argc, const char **argv, const 
char *prefix)
argc = parse_options(argc, argv, NULL, options,
builtin_rebase_helper_usage, PARSE_OPT_KEEP_ARGV0);
 
+   flags |= keep_empty ? TODO_LIST_KEEP_EMPTY : 0;
+   flags |= command == SHORTEN_OIDS ? TODO_LIST_SHORTED_IDS : 0;
+
if (command == CONTINUE && argc == 1)
return !!sequencer_continue();
if (command == ABORT && argc == 1)
return !!sequencer_remove_state();
if (command == MAKE_SCRIPT && argc > 1)
-   return !!sequencer_make_script(keep_empty, stdout, argc, argv);
-   if (command == SHORTEN_OIDS && argc == 1)
-   return !!transform_todo_insn(1);
-   if (command == EXPAND_OIDS && argc == 1)
-   return !!transform_todo_insn(0);
+   return !!sequencer_make_script(stdout, argc, argv, flags);
+   if ((command == SHORTEN_OIDS || command == EXPAND_OIDS) && argc == 1)
+   return !!transform_todo_insn(flags);
if (command == CHECK_TODO_LIST && argc == 1)
return !!check_todo_list();
if (command == SKIP_UNNECESSARY_PICKS && argc == 1)
diff --git a/sequencer.c b/sequencer.c
index 0ff3c90e44bf..7d712811e9d1 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -2444,14 +2444,15 @@ void append_signoff(struct strbuf *msgbuf, int 
ignore_footer, unsigned flag)
strbuf_release();
 }
 
-int sequencer_make_script(int keep_empty, FILE *out,
-   int argc, const char **argv)
+int sequencer_make_script(FILE *out, int argc, const char **argv,
+ unsigned flags)
 {
char *format = NULL;
struct pretty_print_context pp = {0};
struct strbuf buf = STRBUF_INIT;
struct rev_info revs;
struct commit *commit;
+   int keep_empty = flags & TODO_LIST_KEEP_EMPTY;
 
init_revisions(, NULL);
revs.verbose_header = 1;
@@ -2494,7 +2495,7 @@ int sequencer_make_script(int keep_empty, FILE *out,
 }
 
 
-int transform_todo_insn(int shorten_ids)
+int transform_todo_insn(unsigned flags)
 {
const char *todo_file = rebase_path_todo();
struct todo_list todo_list = TODO_LIST_INIT;
@@ -2522,7 +2523,7 @@ int transform_todo_insn(int shorten_ids)
 
/* add commit id */
if (item->commit) {
-   const char *oid = shorten_ids ?
+   const char *oid = flags & TODO_LIST_SHORTED_IDS ?
  short_commit_name(item->commit) :
  oid_to_hex(>commit->object.oid);
 
diff --git a/sequencer.h b/sequencer.h
index 4e444e3bf1c4..3bb6b0658192 100644
--- a/sequencer.h
+++ b/sequencer.h
@@ -45,10 +45,12 @@ int sequencer_continue(struct replay_opts *opts);
 int sequencer_rollback(struct replay_opts *opts);
 int sequencer_remove_state(struct replay_opts *opts);
 
-int sequencer_make_script(int keep_empty, FILE *out,
-   int argc, const char **argv);
+#define TODO_LIST_KEEP_EMPTY (1U << 0)
+#define TODO_LIST_SHORTED_IDS (1U << 1)
+int sequencer_make_script(FILE *out, int argc, const char **argv,
+ unsigned flags);
 
-int transform_todo_insn(int shorten_ids);
+int transform_todo_insn(unsigned flags);
 int check_todo_list(void);
 int skip_unnecessary_picks(void);
 int rearrange_squash(void);
-- 
2.15.1.280.g10402c1f5b5c

[PATCH v2 3/9] rebase -i: set commit to null in exec commands

[Liam Beguin]

Make sure commit is set to NULL when parsing exec instructions
from the todo list. If not, we may try to access an uninitialized
address later while updating the todo list.

Signed-off-by: Liam Beguin 
---
 sequencer.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/sequencer.c b/sequencer.c
index fa94ed652d2c..5033b049d995 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -1268,6 +1268,7 @@ static int parse_insn_line(struct todo_item *item, const 
char *bol, char *eol)
bol += padding;
 
if (item->command == TODO_EXEC) {
+   item->commit = NULL;
item->arg = bol;
item->arg_len = (int)(eol - bol);
return 0;
-- 
2.15.1.280.g10402c1f5b5c

[PATCH v2 8/9] rebase -i: learn to abbreviate command names

[Liam Beguin]

`git rebase -i` already know how to interpret single-letter command
names. Teach it to generate the todo list with these same abbreviated
names.

Based-on-patch-by: Johannes Schindelin 
Signed-off-by: Liam Beguin 
---
 Documentation/rebase-config.txt | 20 
 builtin/rebase--helper.c|  3 +++
 sequencer.c | 16 ++--
 sequencer.h |  1 +
 4 files changed, 38 insertions(+), 2 deletions(-)

diff --git a/Documentation/rebase-config.txt b/Documentation/rebase-config.txt
index 30ae08cb5a4b..42e1ba757564 100644
--- a/Documentation/rebase-config.txt
+++ b/Documentation/rebase-config.txt
@@ -30,3 +30,23 @@ rebase.instructionFormat::
A format string, as specified in linkgit:git-log[1], to be used for the
todo list during an interactive rebase.  The format will
automatically have the long commit hash prepended to the format.
+
+rebase.abbreviateCommands::
+   If set to true, `git rebase` will use abbreviated command names in the
+   todo list resulting in something like this:
++
+---
+   p deadbee The oneline of the commit
+   p fa1afe1 The oneline of the next commit
+   ...
+---
++
+instead of:
++
+---
+   pick deadbee The oneline of the commit
+   pick fa1afe1 The oneline of the next commit
+   ...
+---
++
+Defaults to false.
diff --git a/builtin/rebase--helper.c b/builtin/rebase--helper.c
index 03337e1484a2..2c51ddcfd3dd 100644
--- a/builtin/rebase--helper.c
+++ b/builtin/rebase--helper.c
@@ -13,6 +13,7 @@ int cmd_rebase__helper(int argc, const char **argv, const 
char *prefix)
 {
struct replay_opts opts = REPLAY_OPTS_INIT;
unsigned flags = 0, keep_empty = 0;
+   int abbreviate_commands = 0;
enum {
CONTINUE = 1, ABORT, MAKE_SCRIPT, SHORTEN_OIDS, EXPAND_OIDS,
CHECK_TODO_LIST, SKIP_UNNECESSARY_PICKS, REARRANGE_SQUASH,
@@ -43,6 +44,7 @@ int cmd_rebase__helper(int argc, const char **argv, const 
char *prefix)
};
 
git_config(git_default_config, NULL);
+   git_config_get_bool("rebase.abbreviatecommands", _commands);
 
opts.action = REPLAY_INTERACTIVE_REBASE;
opts.allow_ff = 1;
@@ -52,6 +54,7 @@ int cmd_rebase__helper(int argc, const char **argv, const 
char *prefix)
builtin_rebase_helper_usage, PARSE_OPT_KEEP_ARGV0);
 
flags |= keep_empty ? TODO_LIST_KEEP_EMPTY : 0;
+   flags |= abbreviate_commands ? TODO_LIST_ABBREVIATE_CMDS : 0;
flags |= command == SHORTEN_OIDS ? TODO_LIST_SHORTED_IDS : 0;
 
if (command == CONTINUE && argc == 1)
diff --git a/sequencer.c b/sequencer.c
index bd047737082d..b752dcc52982 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -795,6 +795,13 @@ static const char *command_to_string(const enum 
todo_command command)
die("Unknown command: %d", command);
 }
 
+static const char command_to_char(const enum todo_command command)
+{
+   if (command < TODO_COMMENT && todo_command_info[command].c)
+   return todo_command_info[command].c;
+   return comment_line_char;
+}
+
 static int is_noop(const enum todo_command command)
 {
return TODO_NOOP <= command;
@@ -2453,6 +2460,7 @@ int sequencer_make_script(FILE *out, int argc, const char 
**argv,
struct rev_info revs;
struct commit *commit;
int keep_empty = flags & TODO_LIST_KEEP_EMPTY;
+   const char *insn = flags & TODO_LIST_ABBREVIATE_CMDS ? "p" : "pick";
 
init_revisions(, NULL);
revs.verbose_header = 1;
@@ -2485,7 +2493,8 @@ int sequencer_make_script(FILE *out, int argc, const char 
**argv,
strbuf_reset();
if (!keep_empty && is_original_commit_empty(commit))
strbuf_addf(, "%c ", comment_line_char);
-   strbuf_addf(, "pick %s ", oid_to_hex(>object.oid));
+   strbuf_addf(, "%s %s ", insn,
+   oid_to_hex(>object.oid));
pretty_print_commit(, commit, );
strbuf_addch(, '\n');
fputs(buf.buf, out);
@@ -2558,7 +2567,10 @@ int transform_todo_insn(unsigned flags)
}
 
/* add command to the buffer */
-   strbuf_addstr(, command_to_string(item->command));
+   if (flags & TODO_LIST_ABBREVIATE_CMDS)
+   strbuf_addch(, command_to_char(item->command));
+   else
+   strbuf_addstr(, command_to_string(item->command));
 
/* add commit id */
if (item->commit) {
diff --git a/sequencer.h b/sequencer.h
index e4a9d2419883..468ee79fb72d 100644
--- a/sequencer.h
+++ b/sequencer.h
@@ -47,6 +47,7 @@ int sequencer_remove_state(struct 

[PATCH v2 4/9] rebase -i: refactor transform_todo_ids

[Liam Beguin]

The transform_todo_ids function is a little hard to read. Lets try
to make it easier by using more of the strbuf API. Also, since we'll
soon be adding command abbreviations, let's rename the function so
it's name reflects that change.

Signed-off-by: Liam Beguin 
---
 builtin/rebase--helper.c |  4 +--
 sequencer.c  | 69 
 sequencer.h  |  2 +-
 3 files changed, 31 insertions(+), 44 deletions(-)

diff --git a/builtin/rebase--helper.c b/builtin/rebase--helper.c
index f8519363a393..7c06a27de821 100644
--- a/builtin/rebase--helper.c
+++ b/builtin/rebase--helper.c
@@ -55,9 +55,9 @@ int cmd_rebase__helper(int argc, const char **argv, const 
char *prefix)
if (command == MAKE_SCRIPT && argc > 1)
return !!sequencer_make_script(keep_empty, stdout, argc, argv);
if (command == SHORTEN_SHA1S && argc == 1)
-   return !!transform_todo_ids(1);
+   return !!transform_todo_insn(1);
if (command == EXPAND_SHA1S && argc == 1)
-   return !!transform_todo_ids(0);
+   return !!transform_todo_insn(0);
if (command == CHECK_TODO_LIST && argc == 1)
return !!check_todo_list();
if (command == SKIP_UNNECESSARY_PICKS && argc == 1)
diff --git a/sequencer.c b/sequencer.c
index 5033b049d995..0ff3c90e44bf 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -2494,60 +2494,47 @@ int sequencer_make_script(int keep_empty, FILE *out,
 }
 
 
-int transform_todo_ids(int shorten_ids)
+int transform_todo_insn(int shorten_ids)
 {
const char *todo_file = rebase_path_todo();
struct todo_list todo_list = TODO_LIST_INIT;
-   int fd, res, i;
-   FILE *out;
+   struct strbuf buf = STRBUF_INIT;
+   struct todo_item *item;
+   int i;
 
-   strbuf_reset(_list.buf);
-   fd = open(todo_file, O_RDONLY);
-   if (fd < 0)
-   return error_errno(_("could not open '%s'"), todo_file);
-   if (strbuf_read(_list.buf, fd, 0) < 0) {
-   close(fd);
+   if (strbuf_read_file(_list.buf, todo_file, 0) < 0)
return error(_("could not read '%s'."), todo_file);
-   }
-   close(fd);
 
-   res = parse_insn_buffer(todo_list.buf.buf, _list);
-   if (res) {
+   if (parse_insn_buffer(todo_list.buf.buf, _list)) {
todo_list_release(_list);
return error(_("unusable todo list: '%s'"), todo_file);
}
 
-   out = fopen(todo_file, "w");
-   if (!out) {
-   todo_list_release(_list);
-   return error(_("unable to open '%s' for writing"), todo_file);
-   }
-   for (i = 0; i < todo_list.nr; i++) {
-   struct todo_item *item = todo_list.items + i;
-   int bol = item->offset_in_buf;
-   const char *p = todo_list.buf.buf + bol;
-   int eol = i + 1 < todo_list.nr ?
-   todo_list.items[i + 1].offset_in_buf :
-   todo_list.buf.len;
-
-   if (item->command >= TODO_EXEC && item->command != TODO_DROP)
-   fwrite(p, eol - bol, 1, out);
-   else {
-   const char *id = shorten_ids ?
-   short_commit_name(item->commit) :
-   oid_to_hex(>commit->object.oid);
-   int len;
-
-   p += strspn(p, " \t"); /* left-trim command */
-   len = strcspn(p, " \t"); /* length of command */
-
-   fprintf(out, "%.*s %s %.*s\n",
-   len, p, id, item->arg_len, item->arg);
+   for (item = todo_list.items, i = 0; i < todo_list.nr; i++, item++) {
+   /* if the item is not a command write it and continue */
+   if (item->command >= TODO_COMMENT) {
+   strbuf_addf(, "%.*s\n", item->arg_len, item->arg);
+   continue;
+   }
+
+   /* add command to the buffer */
+   strbuf_addstr(, command_to_string(item->command));
+
+   /* add commit id */
+   if (item->commit) {
+   const char *oid = shorten_ids ?
+ short_commit_name(item->commit) :
+ oid_to_hex(>commit->object.oid);
+
+   strbuf_addf(, " %s", oid);
}
+   /* add all the rest */
+   strbuf_addf(, " %.*s\n", item->arg_len, item->arg);
}
-   fclose(out);
+
+   i = write_message(buf.buf, buf.len, todo_file, 0);
todo_list_release(_list);
-   return 0;
+   return i;
 }
 
 enum check_level {
diff --git a/sequencer.h b/sequencer.h
index 6f3d3df82c0a..4e444e3bf1c4 100644
--- a/sequencer.h
+++ b/sequencer.h
@@ -48,7 +48,7 @@ int sequencer_remove_state(struct replay_opts 

[PATCH v2 7/9] rebase -i -x: add exec commands via the rebase--helper

[Liam Beguin]

Recent work on `git-rebase--interactive` aims to convert shell code to
C. Even if this is most likely not a big performance enhancement, let's
convert it too since a coming change to abbreviate command names
requires it to be updated.

Signed-off-by: Liam Beguin 
---
 builtin/rebase--helper.c   |  7 ++-
 git-rebase--interactive.sh | 23 +--
 sequencer.c| 39 +++
 sequencer.h|  1 +
 4 files changed, 47 insertions(+), 23 deletions(-)

diff --git a/builtin/rebase--helper.c b/builtin/rebase--helper.c
index fe814bf7229e..03337e1484a2 100644
--- a/builtin/rebase--helper.c
+++ b/builtin/rebase--helper.c
@@ -15,7 +15,8 @@ int cmd_rebase__helper(int argc, const char **argv, const 
char *prefix)
unsigned flags = 0, keep_empty = 0;
enum {
CONTINUE = 1, ABORT, MAKE_SCRIPT, SHORTEN_OIDS, EXPAND_OIDS,
-   CHECK_TODO_LIST, SKIP_UNNECESSARY_PICKS, REARRANGE_SQUASH
+   CHECK_TODO_LIST, SKIP_UNNECESSARY_PICKS, REARRANGE_SQUASH,
+   ADD_EXEC
} command = 0;
struct option options[] = {
OPT_BOOL(0, "ff", _ff, N_("allow fast-forward")),
@@ -36,6 +37,8 @@ int cmd_rebase__helper(int argc, const char **argv, const 
char *prefix)
N_("skip unnecessary picks"), SKIP_UNNECESSARY_PICKS),
OPT_CMDMODE(0, "rearrange-squash", ,
N_("rearrange fixup/squash lines"), REARRANGE_SQUASH),
+   OPT_CMDMODE(0, "add-exec-commands", ,
+   N_("insert exec commands in todo list"), ADD_EXEC),
OPT_END()
};
 
@@ -65,5 +68,7 @@ int cmd_rebase__helper(int argc, const char **argv, const 
char *prefix)
return !!skip_unnecessary_picks();
if (command == REARRANGE_SQUASH && argc == 1)
return !!rearrange_squash();
+   if (command == ADD_EXEC && argc == 2)
+   return !!sequencer_add_exec_commands(argv[1]);
usage_with_options(builtin_rebase_helper_usage, options);
 }
diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 437815669f00..e3f5a0abf3c7 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -722,27 +722,6 @@ collapse_todo_ids() {
git rebase--helper --shorten-ids
 }
 
-# Add commands after a pick or after a squash/fixup series
-# in the todo list.
-add_exec_commands () {
-   {
-   first=t
-   while read -r insn rest
-   do
-   case $insn in
-   pick)
-   test -n "$first" ||
-   printf "%s" "$cmd"
-   ;;
-   esac
-   printf "%s %s\n" "$insn" "$rest"
-   first=
-   done
-   printf "%s" "$cmd"
-   } <"$1" >"$1.new" &&
-   mv "$1.new" "$1"
-}
-
 # Switch to the branch in $into and notify it in the reflog
 checkout_onto () {
GIT_REFLOG_ACTION="$GIT_REFLOG_ACTION: checkout $onto_name"
@@ -982,7 +961,7 @@ fi
 
 test -s "$todo" || echo noop >> "$todo"
 test -z "$autosquash" || git rebase--helper --rearrange-squash || exit
-test -n "$cmd" && add_exec_commands "$todo"
+test -n "$cmd" && git rebase--helper --add-exec-commands "$cmd"
 
 todocount=$(git stripspace --strip-comments <"$todo" | wc -l)
 todocount=${todocount##* }
diff --git a/sequencer.c b/sequencer.c
index 7d712811e9d1..bd047737082d 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -2494,6 +2494,45 @@ int sequencer_make_script(FILE *out, int argc, const 
char **argv,
return 0;
 }
 
+/*
+ * Add commands after pick and (series of) squash/fixup commands
+ * in the todo list.
+ */
+int sequencer_add_exec_commands(const char *commands)
+{
+   const char *todo_file = rebase_path_todo();
+   struct todo_list todo_list = TODO_LIST_INIT;
+   struct todo_item *item;
+   struct strbuf *buf = _list.buf;
+   size_t offset = 0, commands_len = strlen(commands);
+   int i, first;
+
+   if (strbuf_read_file(_list.buf, todo_file, 0) < 0)
+   return error(_("could not read '%s'."), todo_file);
+
+   if (parse_insn_buffer(todo_list.buf.buf, _list)) {
+   todo_list_release(_list);
+   return error(_("unusable todo list: '%s'"), todo_file);
+   }
+
+   first = 1;
+   /* insert  before every pick except the first one */
+   for (item = todo_list.items, i = 0; i < todo_list.nr; i++, item++) {
+   if (item->command == TODO_PICK && !first) {
+   strbuf_insert(buf, item->offset_in_buf + offset,
+ commands, commands_len);
+   offset += commands_len;
+   }
+   first = 0;
+   }
+
+   /* append final  */
+   strbuf_add(buf, 

[PATCH v2 0/9] rebase -i: add config to abbreviate command names

[Liam Beguin]

Hi everyone,

This series will add the 'rebase.abbreviateCommands' configuration
option to allow `git rebase -i` to default to the single-letter command
names when generating 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 in V2:
- Refactor and rename 'transform_todo_ids'
- Replace SHA-1 by OID in rebase--helper.c
- Update todo list related functions to take a generic 'flags' parameter
- Rename 'add_exec_commands' function to 'sequencer_add_exec_commands'
- Rename 'add-exec' option to 'add-exec-commands'
- Use 'strbur_read_file' instead of rewriting it
- Make 'command_to_char' return 'comment_char_line' if no single-letter
  command name is defined
- Combine both tests into a single test case
- Update commit messages

Liam Beguin (9):
  Documentation: move rebase.* configs to new file
  Documentation: use preferred name for the 'todo list' script
  rebase -i: set commit to null in exec commands
  rebase -i: refactor transform_todo_ids
  rebase -i: replace reference to sha1 with oid
  rebase -i: update functions to use a flags parameter
  rebase -i -x: add exec commands via the rebase--helper
  rebase -i: learn to abbreviate command names
  t3404: add test case for abbreviated commands

 Documentation/config.txt|  31 +---
 Documentation/git-rebase.txt|  19 +
 Documentation/rebase-config.txt |  52 +
 builtin/rebase--helper.c|  29 +---
 git-rebase--interactive.sh  |  23 +-
 sequencer.c | 126 +---
 sequencer.h |  10 ++-
 t/t3404-rebase-interactive.sh   |  22 ++
 8 files changed, 186 insertions(+), 126 deletions(-)
 create mode 100644 Documentation/rebase-config.txt

-- 
2.15.1.280.g10402c1f5b5c

[PATCH v2 1/9] Documentation: move rebase.* configs to new file

[Liam Beguin]

Move all rebase.* configuration variables to a separate file in order to
remove duplicates, and include it in config.txt and git-rebase.txt.  The
new descriptions are mostly taken from config.txt as they are more
verbose.

Signed-off-by: Liam Beguin 
---
 Documentation/config.txt| 31 +--
 Documentation/git-rebase.txt| 19 +--
 Documentation/rebase-config.txt | 32 
 3 files changed, 34 insertions(+), 48 deletions(-)
 create mode 100644 Documentation/rebase-config.txt

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 531649cb40ea..e424b7de90b5 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -2691,36 +2691,7 @@ push.recurseSubmodules::
is retained. You may override this configuration at time of push by
specifying '--recurse-submodules=check|on-demand|no'.
 
-rebase.stat::
-   Whether to show a diffstat of what changed upstream since the last
-   rebase. False by default.
-
-rebase.autoSquash::
-   If set to true enable `--autosquash` option by default.
-
-rebase.autoStash::
-   When set to true, automatically create a temporary stash entry
-   before the operation begins, and apply it after the operation
-   ends.  This means that you can run rebase on a dirty worktree.
-   However, use with care: the final stash application after a
-   successful rebase might result in non-trivial conflicts.
-   Defaults to false.
-
-rebase.missingCommitsCheck::
-   If set to "warn", git rebase -i will print a warning if some
-   commits are removed (e.g. a line was deleted), however the
-   rebase will still proceed. If set to "error", it will print
-   the previous warning and stop the rebase, 'git rebase
-   --edit-todo' can then be used to correct the error. If set to
-   "ignore", no checking is done.
-   To drop a commit without warning or error, use the `drop`
-   command in the todo-list.
-   Defaults to "ignore".
-
-rebase.instructionFormat::
-   A format string, as specified in linkgit:git-log[1], to be used for
-   the instruction list during an interactive rebase.  The format will 
automatically
-   have the long commit hash prepended to the format.
+include::rebase-config.txt[]
 
 receive.advertiseAtomic::
By default, git-receive-pack will advertise the atomic push
diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
index 3cedfb0fd22b..8a861c1e0d69 100644
--- a/Documentation/git-rebase.txt
+++ b/Documentation/git-rebase.txt
@@ -203,24 +203,7 @@ Alternatively, you can undo the 'git rebase' with
 CONFIGURATION
 -
 
-rebase.stat::
-   Whether to show a diffstat of what changed upstream since the last
-   rebase. False by default.
-
-rebase.autoSquash::
-   If set to true enable `--autosquash` option by default.
-
-rebase.autoStash::
-   If set to true enable `--autostash` option by default.
-
-rebase.missingCommitsCheck::
-   If set to "warn", print warnings about removed commits in
-   interactive mode. If set to "error", print the warnings and
-   stop the rebase. If set to "ignore", no checking is
-   done. "ignore" by default.
-
-rebase.instructionFormat::
-   Custom commit list format to use during an `--interactive` rebase.
+include::rebase-config.txt[]
 
 OPTIONS
 ---
diff --git a/Documentation/rebase-config.txt b/Documentation/rebase-config.txt
new file mode 100644
index ..dba088d7c68f
--- /dev/null
+++ b/Documentation/rebase-config.txt
@@ -0,0 +1,32 @@
+rebase.stat::
+   Whether to show a diffstat of what changed upstream since the last
+   rebase. False by default.
+
+rebase.autoSquash::
+   If set to true enable `--autosquash` option by default.
+
+rebase.autoStash::
+   When set to true, automatically create a temporary stash entry
+   before the operation begins, and apply it after the operation
+   ends.  This means that you can run rebase on a dirty worktree.
+   However, use with care: the final stash application after a
+   successful rebase might result in non-trivial conflicts.
+   This option can be overridden by the `--no-autostash` and
+   `--autostash` options of linkgit:git-rebase[1].
+   Defaults to false.
+
+rebase.missingCommitsCheck::
+   If set to "warn", git rebase -i will print a warning if some
+   commits are removed (e.g. a line was deleted), however the
+   rebase will still proceed. If set to "error", it will print
+   the previous warning and stop the rebase, 'git rebase
+   --edit-todo' can then be used to correct the error. If set to
+   "ignore", no checking is done.
+   To drop a commit without warning or error, use the `drop`
+   command in the todo-list.
+   Defaults to "ignore".
+
+rebase.instructionFormat::
+   A format string, as specified in 

[PATCH v2 2/9] Documentation: use preferred name for the 'todo list' script

[Liam Beguin]

Use "todo list" instead of "instruction list" or "todo-list" to
reduce further confusion regarding the name of this script.

Signed-off-by: Liam Beguin 
---
 Documentation/rebase-config.txt | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/Documentation/rebase-config.txt b/Documentation/rebase-config.txt
index dba088d7c68f..30ae08cb5a4b 100644
--- a/Documentation/rebase-config.txt
+++ b/Documentation/rebase-config.txt
@@ -23,10 +23,10 @@ rebase.missingCommitsCheck::
--edit-todo' can then be used to correct the error. If set to
"ignore", no checking is done.
To drop a commit without warning or error, use the `drop`
-   command in the todo-list.
+   command in the todo list.
Defaults to "ignore".
 
 rebase.instructionFormat::
A format string, as specified in linkgit:git-log[1], to be used for the
-   instruction list during an interactive rebase.  The format will
+   todo list during an interactive rebase.  The format will
automatically have the long commit hash prepended to the format.
-- 
2.15.1.280.g10402c1f5b5c

[PATCH v2 5/9] rebase -i: replace reference to sha1 with oid

[Liam Beguin]

Since we are trying to abstract the hash function name elsewhere in the
code base, lets use OID instead of SHA-1 in the rebase--helper too.

Signed-off-by: Liam Beguin 
---
 builtin/rebase--helper.c | 10 +-
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/builtin/rebase--helper.c b/builtin/rebase--helper.c
index 7c06a27de821..af0f91164fd0 100644
--- a/builtin/rebase--helper.c
+++ b/builtin/rebase--helper.c
@@ -14,7 +14,7 @@ int cmd_rebase__helper(int argc, const char **argv, const 
char *prefix)
struct replay_opts opts = REPLAY_OPTS_INIT;
int keep_empty = 0;
enum {
-   CONTINUE = 1, ABORT, MAKE_SCRIPT, SHORTEN_SHA1S, EXPAND_SHA1S,
+   CONTINUE = 1, ABORT, MAKE_SCRIPT, SHORTEN_OIDS, EXPAND_OIDS,
CHECK_TODO_LIST, SKIP_UNNECESSARY_PICKS, REARRANGE_SQUASH
} command = 0;
struct option options[] = {
@@ -27,9 +27,9 @@ int cmd_rebase__helper(int argc, const char **argv, const 
char *prefix)
OPT_CMDMODE(0, "make-script", ,
N_("make rebase script"), MAKE_SCRIPT),
OPT_CMDMODE(0, "shorten-ids", ,
-   N_("shorten SHA-1s in the todo list"), SHORTEN_SHA1S),
+   N_("shorten commit ids in the todo list"), 
SHORTEN_OIDS),
OPT_CMDMODE(0, "expand-ids", ,
-   N_("expand SHA-1s in the todo list"), EXPAND_SHA1S),
+   N_("expand commit ids in the todo list"), EXPAND_OIDS),
OPT_CMDMODE(0, "check-todo-list", ,
N_("check the todo list"), CHECK_TODO_LIST),
OPT_CMDMODE(0, "skip-unnecessary-picks", ,
@@ -54,9 +54,9 @@ int cmd_rebase__helper(int argc, const char **argv, const 
char *prefix)
return !!sequencer_remove_state();
if (command == MAKE_SCRIPT && argc > 1)
return !!sequencer_make_script(keep_empty, stdout, argc, argv);
-   if (command == SHORTEN_SHA1S && argc == 1)
+   if (command == SHORTEN_OIDS && argc == 1)
return !!transform_todo_insn(1);
-   if (command == EXPAND_SHA1S && argc == 1)
+   if (command == EXPAND_OIDS && argc == 1)
return !!transform_todo_insn(0);
if (command == CHECK_TODO_LIST && argc == 1)
return !!check_todo_list();
-- 
2.15.1.280.g10402c1f5b5c

Re: [PATCH] send-email: extract email-parsing code into a subroutine

[Ævar Arnfjörð Bjarmason]

On Sat, Dec 02 2017, Payre Nathan jotted:

> From: Nathan Payre 
>
> The existing code mixes parsing of email header with regular
> expression and actual code. Extract the parsing code into a new
> subroutine 'parse_header_line()'. This improves the code readability
> and make parse_header_line reusable in other place.
>
> Signed-off-by: Nathan Payre 
> Signed-off-by: Matthieu Moy 
> Signed-off-by: Timothee Albertin 
> Signed-off-by: Daniel Bensoussan 
> ---
>
> This patch is a first step to implement a new feature.
> See new feature discussion here: 
> https://public-inbox.org/git/20171030223444.5052-1-nathan.pa...@etu.univ-lyon1.fr/
>
>  git-send-email.perl | 106 
> +++-
>  1 file changed, 80 insertions(+), 26 deletions(-)
>
> diff --git a/git-send-email.perl b/git-send-email.perl
> index 2208dcc21..98c2e461c 100755
> --- a/git-send-email.perl
> +++ b/git-send-email.perl
> @@ -715,41 +715,64 @@ EOT3
>   if (!defined $compose_encoding) {
>   $compose_encoding = "UTF-8";
>   }
> - while(<$c>) {
> +
> + my %parsed_email;
> + while (<$c>) {
>   next if m/^GIT:/;
> - if ($in_body) {
> - $summary_empty = 0 unless (/^\n$/);
> - } elsif (/^\n$/) {
> - $in_body = 1;
> - if ($need_8bit_cte) {
> + parse_header_line($_, \%parsed_email);
> + if (/^\n$/i) {
> + while (my $row = <$c>) {
> + if (!($row =~ m/^GIT:/)) {
> + $parsed_email{'body'} = 
> $parsed_email{'body'} . $row;
> + }
> + }
> + }
> + }
> + if ($parsed_email{'from'}) {
> + $sender = $parsed_email{'from'};
> + }
> + if ($parsed_email{'in_reply_to'}) {
> + $initial_reply_to = $parsed_email{'in_reply_to'};
> + }
> + if ($parsed_email{'subject'}) {
> + $initial_subject = $parsed_email{'subject'};
> + print $c2 "Subject: " .
> + quote_subject($parsed_email{'subject'}, 
> $compose_encoding) .
> + "\n";
> + }
> + if ($parsed_email{'mime-version'}) {
> + $need_8bit_cte = 0;
> + }
> + if ($need_8bit_cte) {
> + if ($parsed_email{'content-type'}) {
> + print $c2 "MIME-Version: 1.0\n",
> +  "Content-Type: 
> $parsed_email{'content-type'};",
> +  "Content-Transfer-Encoding: 8bit\n";
> + } else {
>   print $c2 "MIME-Version: 1.0\n",
>"Content-Type: text/plain; ",
> -"charset=$compose_encoding\n",
> +  "charset=$compose_encoding\n",
>"Content-Transfer-Encoding: 8bit\n";
>   }
> - } elsif (/^MIME-Version:/i) {
> - $need_8bit_cte = 0;
> - } elsif (/^Subject:\s*(.+)\s*$/i) {
> - $initial_subject = $1;
> - my $subject = $initial_subject;
> - $_ = "Subject: " .
> - quote_subject($subject, $compose_encoding) .
> - "\n";
> - } elsif (/^In-Reply-To:\s*(.+)\s*$/i) {
> - $initial_reply_to = $1;
> - next;
> - } elsif (/^From:\s*(.+)\s*$/i) {
> - $sender = $1;
> - next;
> - } elsif (/^(?:To|Cc|Bcc):/i) {
> - print __("To/Cc/Bcc fields are not interpreted yet, 
> they have been ignored\n");
> - next;
> - }
> - print $c2 $_;
>   }
> + if ($parsed_email{'body'}) {
> + $summary_empty = 0;
> + print $c2 "\n$parsed_email{'body'}\n";
> + }
> +
>   close $c;
>   close $c2;
>
> + open $c2, "<", $compose_filename . ".final"
> + or die sprintf(__("Failed to open %s.final: %s"), 
> $compose_filename, $!);
> +
> + print "affichage : \n";
> + while (<$c2>) {
> + print $_;
> + }
> +
> + close $c2;
> +
>   if ($summary_empty) {
>   print __("Summary email is empty, skipping it\n");
>   $compose = -1;
> @@ -792,6 +815,37 @@ sub ask {
>   return;
>  }
>
> +sub parse_header_line {
> + my $lines = shift;
> + my $parsed_line = shift;
> +
> + foreach (split(/\n/, $lines)) {
> + if (/^From:\s*(.+)$/i) {
> + $parsed_line->{'from'} = $1;
> + 

Re: [PATCH] send-email: extract email-parsing code into a subroutine

[Matthieu Moy]

Nathan PAYRE  writes:

> I found a mistake in my signed-off-by, please replace
>  by 

I think you want exactly the opposite of this. You're contributing as a
Lyon 1 student, hence your identity is @etu.univ-lyon1.fr. Your Gmail
adress is used only for technical reasons.

OTOH, you are missing the first line From: ... @..univ-lyon1.fr in your
message.

See how you did it:

https://public-inbox.org/git/20171012091727.30759-1-second.pa...@gmail.com/

(The sign-off was wrong in this one, but the From was OK)

-- 
Matthieu Moy
https://matthieu-moy.fr/

Re: [PATCH 1/3] git-compat-util: introduce skip_to_opt_val()

[Christian Couder]

On Sun, Dec 3, 2017 at 7:45 PM, Junio C Hamano  wrote:
> Christian Couder  writes:
>
>> From: Christian Couder 
>>
>> We often accept both a "--key" option and a "--key=" option.
>>
>> These options currently are parsed using something like:
>>
>> if (!strcmp(arg, "--key")) {
>>   /* do something */
>> } else if (skip_prefix(arg, "--key=", )) {
>>   /* do something with arg */
>> }
>>
>> which is a bit cumbersome compared to just:
>>
>> if (skip_to_opt_val(arg, "--key", )) {
>>   /* do something with arg */
>> }
>
> Sounds sensible; especially if there are many existing code that can
> be shortened by using this helper, that is.
>
>> Note that, when using skip_to_opt_val(), it is not possible any more
>> to do something different when the first argument is exactly "--key"
>> than when it is exactly "--key=", but in most cases we already don't
>> make any difference, which is a good thing.
>
> Note that "it is not possible" is misleading.  skip_to_opt_val()
> could be written to allow the caller to tell the difference if you
> chose to, but *you* made it impossible by assigning "" to arg upon
> seeing "--key".  You could assign NULL to arg in that case, and
> "--key" and "--key=" can be differenciated by checking arg; the
> former will give you NULL and the latter "".

Yeah, what I meant was "the design of the function in this patch makes
it impossible..."
That's why I wrote after the diffstat:

"""Another possibility would be to add a "const char *default"
argument to the function, and to do:

  if (!*p) {
  *val = default;
  return 1;
  }

This could make the function more useful in some cases."""

But yeah I could have added the above to the commit message and it
hopefully would have made it clear what I meant.

Anyway there is a design choice to be made. Adding a "const char
*default" argument makes the function more generic, but this might not
be very useful as there are few cases that might benefit. And if we
want to make the command line interface consistent and perhaps avoid
user errors, we might want to have a rule that says that "--key" and
"--key=" should always give the same result. In this case we may also
want to make it easy to implement options that follow the rule.

So my preference is to not add the "const char *default" argument to
the function. But it is not a strong preference.

> Not that I am convinced that it is a bad idea to deliberately lose
> information like this implementation does.  At least not yet.
>
> If there will be no conceivable caller that wants to differenticate
> between the two, the implementation that "loses information" can
> claim to be easier to use, as the callers do not have to be forced
> to write something like:
>
> if (skip_to_optional_val(arg, "--key", )
> do_something(arg ? arg : "");
>
> to treat them the same.
>
> Having said all that, I would expect skip_to_optional_val() (as a
> name of the helper I suspect skip_to_optional_arg() is more
> appropriate, though)

Ok I will rename it skip_to_optional_arg().

> to store NULL in the arg thing if there is no
> optional argument given.  Also, the caller does not have to even do
> the 'arg ? arg : ""' dance if it is so common and natural for the
> application to treat "--key" and "--key=" equivalently, as it is
> expected that the actual code to work on the arg, i.e. do_something()
> in the above example, _should_ be prepared to take NULL and "" and
> treat them the same way (that is the definition of "it is so common
> and natural for the application to treat them the same).

I'd rather add the "const char *default" argument to the function
rather than have the function just set "arg" to NULL. I think setting
"arg" to NULL increases the risk of crashes and makes it too easy to
make "--key" and "--key=" behave differently which I think is not
consistent and not intuitive.

> So, I think you identified interesting pattern that can be cleaned
> up by introducing a helper, but I am not sure if I agree with the
> exact design of the helper.

Ok, maybe the above explains the design a bit better.

>> Note that "opt" in the function name actually means "optional" as
>> the function can be used to parse any "key=value" string where "key"
>> is also considered as valid, not just command line options.
>
> Yup.  This paragraph is a good thing to have in the proposed log
> message, to explain the reason why you force callers of this helper
> to spell out the leading dashes like "--key" (not "key").  I think
> that it is a sane design of the input side of this function---it
> does not limit it to an overly narrow case of command line option
> processing.  For the same reason, I think it is saner design of the
> output side to allow callers to tell between "key=" and "key".
>
> While staring at this helper and writing "does not limit to command
> line option processing", it occurs 

Re: RFC: Native clean/smudge filter for UTF-16 files

[Lars Schneider]

> On 24 Nov 2017, at 19:04, Jeff King  wrote:
> 
> On Thu, Nov 23, 2017 at 04:18:59PM +0100, Lars Schneider wrote:
> 
>> Alternatively, I could add a native attribute to Git that translates UTF-16 
>> to UTF-8 and back. A conversion function is already available in "mingw.h" 
>> [3]
>> on Windows. Limiting this feature to Windows wouldn't be a problem from my
>> point of view as UTF-16 is only relevant on Windows anyways. The attribute 
>> could look like this:
>> 
>>*.txttext encoding=utf-16
>> 
>> There was a previous discussion on the topic and Jonathan already suggested
>> a "native" clean/smudge filter in 2010 [4]. Also the "encoding" attribute
>> is already present but, as far as I can tell, is only used by the git gui
>> for viewing [5].
> 
> I would not want to see a proliferation of built-in filters, but it
> really seems like text-encoding conversion is a broad and practical one
> that many people might benefit from. So just like line-ending
> conversion, which _could_ be done by generic filters, it makes sense to
> me to support it natively for speed and simplicity.
> 
>> Do you think a patch that converts UTF-16 files to UTF-8 via an attribute
>> "encoding=utf-16" on Windows would have a chance to get accepted?
> 
> You haven't fully specified the semantics here, so let me sketch out
> what I think it ought to look like:

Thank you :-)


> - declare utf8 the "canonical" in-repo representation, just as we have
>   declared LF for line endings (alternatively this could be
>   configurable, but if we can get away with declaring utf8 the one true
>   encoding, that cuts out a lot of corner cases).

100% agree on UTF-8 as canonical representation and I wouldn't make 
that configurable as it would open a can of worms.


> - if core.convertEncoding is true, then for any file with an
>   encoding=foo attribute, internally run iconv(foo, utf8) in
>   convert_to_git(), and likewise iconv(utf8, foo) in
>   convert_to_working_tree.
> 
> - I'm not sure if core.convertEncoding should be enabled by default. If
>   it's a noop as long as there's no encoding attribute, then it's
>   probably fine. But I would not want accidental conversion or any
>   slowdown for the common case that the user wants no conversion.

I think we should mimic the behavior of "eol=crlf/lf" attribute.

AFAIK whenever I set "*.ext text eol=crlf", then I can be sure the 
file is checked out with CRLF independent of any of my local config
settings. Isn't that correct? I would expect a similar behavior if
"*.ext text encoding=utf16" is set. Wouldn't that mean that we do
not need a "core.convertEncoding" config?

I do 100% agree that it must be a noop if there is no encoding 
attribute present.


> - I doubt we'd want a "core.autoEncoding" similar to "core.autocrlf". I
>   don't think people consistently have all utf-16 files (the way they
>   might have all CRLF files) rather a few files that must be utf-16.

Agreed!


> - I have actually seen two types of utf-16 in git repos in the wild:
>   files which really must be utf-16 (because some tool demands it) and
>   files which happen to be utf-16, but could just as easily be utf-8
>   (and the user simply does not notice and commits utf-16, but doesn't
>   realize it until much later when their diffs are unreadable).
> 
>   For the first case, the "encoding" thing above would work fine. For
>   the second case, in theory we could have an option that takes any
>   file with a "text" attribute and no "encoding" attribute, and
>   converts it to utf-8.

TBH I would label that a "non-goal". Because AFAIK we cannot reliability
detect the encoding automatically.


>   I suspect that's opening a can of worms for false positives similar
>   to core.autocrlf. And performance drops as we try to guess the
>   encoding and convert all incoming data.
> 
>   So I mention it mostly as a direction I think we probably _don't_
>   want to go. Anybody with the "this could have been utf-8 all along"
>   type of file can remedy it by converting and committing the result.

100 % agree.


> Omitting all of the "we shouldn't do this" bullet points, it seems
> pretty simple and sane to me.
> 
> There is one other approach, which is to really store utf-16 in the
> repository and better teach the diff tools to handle it (which are
> really the main thing in git that cares about looking into the blob
> contents). You can do this already with a textconv filter, but:
> 
>  1. It's slow (though cacheable).
> 
>  2. It doesn't work unless each repo configures the filter (so not on
> sites like GitHub, unless we define a micro-format that diff=utf16
> should be textconv'd on display, and get all implementations to
> respect that).

Actually, rendering diffs on Git hosting sites such as GitHub is one
of my goals. Therefore, storing content as UTF-16 wouldn't be a solution
for me.


>  3. Textconv patches look good, but can't be applied. This occasionally
> makes things awkward, 

Re: [PATCH 1/3] git-compat-util: introduce skip_to_opt_val()

[Junio C Hamano]

Christian Couder  writes:

> From: Christian Couder 
>
> We often accept both a "--key" option and a "--key=" option.
>
> These options currently are parsed using something like:
>
> if (!strcmp(arg, "--key")) {
>   /* do something */
> } else if (skip_prefix(arg, "--key=", )) {
>   /* do something with arg */
> }
>
> which is a bit cumbersome compared to just:
>
> if (skip_to_opt_val(arg, "--key", )) {
>   /* do something with arg */
> }

Sounds sensible; especially if there are many existing code that can
be shortened by using this helper, that is.

> Note that, when using skip_to_opt_val(), it is not possible any more
> to do something different when the first argument is exactly "--key"
> than when it is exactly "--key=", but in most cases we already don't
> make any difference, which is a good thing.

Note that "it is not possible" is misleading.  skip_to_opt_val()
could be written to allow the caller to tell the difference if you
chose to, but *you* made it impossible by assigning "" to arg upon
seeing "--key".  You could assign NULL to arg in that case, and
"--key" and "--key=" can be differenciated by checking arg; the
former will give you NULL and the latter "".

Not that I am convinced that it is a bad idea to deliberately lose
information like this implementation does.  At least not yet.

If there will be no conceivable caller that wants to differenticate
between the two, the implementation that "loses information" can
claim to be easier to use, as the callers do not have to be forced
to write something like:

if (skip_to_optional_val(arg, "--key", )
do_something(arg ? arg : "");

to treat them the same.

Having said all that, I would expect skip_to_optional_val() (as a
name of the helper I suspect skip_to_optional_arg() is more
appropriate, though) to store NULL in the arg thing if there is no
optional argument given.  Also, the caller does not have to even do
the 'arg ? arg : ""' dance if it is so common and natural for the
application to treat "--key" and "--key=" equivalently, as it is
expected that the actual code to work on the arg, i.e. do_something()
in the above example, _should_ be prepared to take NULL and "" and
treat them the same way (that is the definition of "it is so common
and natural for the application to treat them the same).

So, I think you identified interesting pattern that can be cleaned
up by introducing a helper, but I am not sure if I agree with the
exact design of the helper.

> Note that "opt" in the function name actually means "optional" as
> the function can be used to parse any "key=value" string where "key"
> is also considered as valid, not just command line options.

Yup.  This paragraph is a good thing to have in the proposed log
message, to explain the reason why you force callers of this helper
to spell out the leading dashes like "--key" (not "key").  I think
that it is a sane design of the input side of this function---it
does not limit it to an overly narrow case of command line option
processing.  For the same reason, I think it is saner design of the
output side to allow callers to tell between "key=" and "key".

While staring at this helper and writing "does not limit to command
line option processing", it occurs to me that a helper that allows
you to look for an optional ':' (instead of '=') may also be useful,
so the final version might become a pair of functions, perhaps like
so:

int skip_to_optional_delim(const char *s,
   const char *prefix, char delim,
   const char **rest)
{
const char *p;

if (!skip_prefix(str, prefix, ))
return 0;
if (!*p)
*rest = NULL;
else if (*p != delim)
return 0;
else
*rest = p + 1;
return 1;
}

int skip_to_optional_arg(const char *s,
 const char *prefix,
 const char **arg)
{
return skip_to_optional_delim(s, prefix, '=', arg);
}

As the first one would not (by definition) have any callers
initially after your series, it can be static to a file that
implements both of them and it is OK to expose only the latter to
the public.  

I do think it is a premature optimization to inline this function,
whose initial callers will be (from the look of the remainder of
your patches) command line parsing that sits farthest from any
performance critical code.

>
> Signed-off-by: Christian Couder 
> ---
>  git-compat-util.h | 35 +++
>  1 file changed, 35 insertions(+)
>
> Another possibility would be to add a "const char *default"
> argument to the function, and to do: 
>
>   if (!*p) {
>   *val = default;
>   return 1;
>   }
>
> This could make the function more useful in some cases.
>
> I also wonder 

[PATCH 2/3] index-pack: use skip_to_opt_val()

[Christian Couder]

From: Christian Couder 

Let's simplify index-pack option parsing using skip_to_opt_val().

Signed-off-by: Christian Couder 
---
 builtin/index-pack.c | 11 +++
 1 file changed, 3 insertions(+), 8 deletions(-)

diff --git a/builtin/index-pack.c b/builtin/index-pack.c
index 8ec459f522..5cf252c885 100644
--- a/builtin/index-pack.c
+++ b/builtin/index-pack.c
@@ -1660,10 +1660,7 @@ int cmd_index_pack(int argc, const char **argv, const 
char *prefix)
from_stdin = 1;
} else if (!strcmp(arg, "--fix-thin")) {
fix_thin_pack = 1;
-   } else if (!strcmp(arg, "--strict")) {
-   strict = 1;
-   do_fsck_object = 1;
-   } else if (skip_prefix(arg, "--strict=", )) {
+   } else if (skip_to_opt_val(arg, "--strict", )) {
strict = 1;
do_fsck_object = 1;
fsck_set_msg_types(_options, arg);
@@ -1679,10 +1676,8 @@ int cmd_index_pack(int argc, const char **argv, const 
char *prefix)
verify = 1;
show_stat = 1;
stat_only = 1;
-   } else if (!strcmp(arg, "--keep")) {
-   keep_msg = "";
-   } else if (starts_with(arg, "--keep=")) {
-   keep_msg = arg + 7;
+   } else if (skip_to_opt_val(arg, "--keep", _msg)) {
+   ; /* nothing to do */
} else if (starts_with(arg, "--threads=")) {
char *end;
nr_threads = strtoul(arg+10, , 0);
-- 
2.15.1.271.g1a4e40aa5d.dirty

[PATCH 1/3] git-compat-util: introduce skip_to_opt_val()

[Christian Couder]

From: Christian Couder 

We often accept both a "--key" option and a "--key=" option.

These options currently are parsed using something like:

if (!strcmp(arg, "--key")) {
/* do something */
} else if (skip_prefix(arg, "--key=", )) {
/* do something with arg */
}

which is a bit cumbersome compared to just:

if (skip_to_opt_val(arg, "--key", )) {
/* do something with arg */
}

Note that, when using skip_to_opt_val(), it is not possible any more
to do something different when the first argument is exactly "--key"
than when it is exactly "--key=", but in most cases we already don't
make any difference, which is a good thing.

Note that "opt" in the function name actually means "optional" as
the function can be used to parse any "key=value" string where "key"
is also considered as valid, not just command line options.

Signed-off-by: Christian Couder 
---
 git-compat-util.h | 35 +++
 1 file changed, 35 insertions(+)

Another possibility would be to add a "const char *default"
argument to the function, and to do: 

if (!*p) {
*val = default;
return 1;
}

This could make the function more useful in some cases.

I also wonder if the function is too big to be inlined, and
in that case, in which file it should be added. 

diff --git a/git-compat-util.h b/git-compat-util.h
index cedad4d581..7ee040388f 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -534,6 +534,41 @@ static inline int ends_with(const char *str, const char 
*suffix)
return strip_suffix(str, suffix, );
 }
 
+/*
+ * If the string "str" is the same as the string in "prefix", then the "val"
+ * parameter is set to the empty string and 1 is returned.
+ * If the string "str" begins with the string found in "prefix" and then a
+ * "=" sign, then the "val" parameter is set to "str + strlen(prefix) + 1"
+ * (i.e., to the point in the string right after the prefix and the "=" sign),
+ * and 1 is returned.
+ *
+ * Otherwise, return 0 and leave "val" untouched.
+ *
+ * When we accept both a "--key" and a "--key=" option, this function
+ * can be used instead of !strcmp(arg, "--key") and then
+ * skip_prefix(arg, "--key=", ) to parse such an option.
+ */
+static inline int skip_to_opt_val(const char *str, const char *prefix,
+ const char **val)
+{
+   const char *p;
+
+   if (!skip_prefix(str, prefix, ))
+   return 0;
+
+   if (!*p) {
+   *val = "";
+   return 1;
+   }
+
+   if (*p == '=') {
+   *val = p + 1;
+   return 1;
+   }
+
+   return 0;
+}
+
 #define SWAP(a, b) do {\
void *_swap_a_ptr = &(a);   \
void *_swap_b_ptr = &(b);   \
-- 
2.15.1.271.g1a4e40aa5d.dirty

[PATCH 3/3] diff: use skip_to_opt_val()

[Christian Couder]

From: Christian Couder 

Let's simplify diff option parsing using skip_to_opt_val().

Signed-off-by: Christian Couder 
---
 diff.c | 22 --
 1 file changed, 8 insertions(+), 14 deletions(-)

diff --git a/diff.c b/diff.c
index 2ebe2227b4..067b498187 100644
--- a/diff.c
+++ b/diff.c
@@ -4508,17 +4508,11 @@ int diff_opt_parse(struct diff_options *options,
options->output_format |= DIFF_FORMAT_NUMSTAT;
else if (!strcmp(arg, "--shortstat"))
options->output_format |= DIFF_FORMAT_SHORTSTAT;
-   else if (!strcmp(arg, "-X") || !strcmp(arg, "--dirstat"))
-   return parse_dirstat_opt(options, "");
-   else if (skip_prefix(arg, "-X", ))
-   return parse_dirstat_opt(options, arg);
-   else if (skip_prefix(arg, "--dirstat=", ))
+   else if (skip_prefix(arg, "-X", ) || skip_to_opt_val(arg, 
"--dirstat", ))
return parse_dirstat_opt(options, arg);
else if (!strcmp(arg, "--cumulative"))
return parse_dirstat_opt(options, "cumulative");
-   else if (!strcmp(arg, "--dirstat-by-file"))
-   return parse_dirstat_opt(options, "files");
-   else if (skip_prefix(arg, "--dirstat-by-file=", )) {
+   else if (skip_to_opt_val(arg, "--dirstat-by-file", )) {
parse_dirstat_opt(options, "files");
return parse_dirstat_opt(options, arg);
}
@@ -4540,13 +4534,13 @@ int diff_opt_parse(struct diff_options *options,
return stat_opt(options, av);
 
/* renames options */
-   else if (starts_with(arg, "-B") || starts_with(arg, 
"--break-rewrites=") ||
-!strcmp(arg, "--break-rewrites")) {
+   else if (starts_with(arg, "-B") ||
+skip_to_opt_val(arg, "--break-rewrites", )) {
if ((options->break_opt = diff_scoreopt_parse(arg)) == -1)
return error("invalid argument to -B: %s", arg+2);
}
-   else if (starts_with(arg, "-M") || starts_with(arg, "--find-renames=") 
||
-!strcmp(arg, "--find-renames")) {
+   else if (starts_with(arg, "-M") ||
+skip_to_opt_val(arg, "--find-renames", )) {
if ((options->rename_score = diff_scoreopt_parse(arg)) == -1)
return error("invalid argument to -M: %s", arg+2);
options->detect_rename = DIFF_DETECT_RENAME;
@@ -4554,8 +4548,8 @@ int diff_opt_parse(struct diff_options *options,
else if (!strcmp(arg, "-D") || !strcmp(arg, "--irreversible-delete")) {
options->irreversible_delete = 1;
}
-   else if (starts_with(arg, "-C") || starts_with(arg, "--find-copies=") ||
-!strcmp(arg, "--find-copies")) {
+   else if (starts_with(arg, "-C") ||
+skip_to_opt_val(arg, "--find-copies", )) {
if (options->detect_rename == DIFF_DETECT_COPY)
options->flags.find_copies_harder = 1;
if ((options->rename_score = diff_scoreopt_parse(arg)) == -1)
-- 
2.15.1.271.g1a4e40aa5d.dirty

Re: [PATCH v4 2/2] launch_editor(): indicate that Git waits for user input

[Lars Schneider]

> On 02 Dec 2017, at 04:45, Kaartic Sivaraam  wrote:
> 
> On Friday 01 December 2017 11:59 PM, Jeff King wrote:
>> On Fri, Dec 01, 2017 at 01:52:14PM +0100, Lars Schneider wrote:
>>> 
>>> Thanks for the review :-)
>> Actually, I meant to bikeshed one part but forgot. ;)
>>> +   fprintf(stderr, _("hint: Waiting for your editor 
>>> input..."));
>> I found "waiting for editor input" to be a funny way of saying this. I
>> input to the editor, the editor does not input to Git. :)
>> Maybe "waiting for your editor finish" or something would make more
>> sense?
> 
> May be the good "Launched editor. Waiting ..." message, that was used in a 
> previous version, itself makes sense?

Perfect bikeshed topic :-)

I would like to add "for your input" or "for you" to convey 
that Git is not waiting for the machine but for the user.

"hint: Launched editor. Waiting for your input..."

Would that work for you?

Thanks,
Lars

Re: [PATCH] git-gui: allow Ctrl+T to toggle multiple paths (Re: [BUG] git gui can't commit multiple files)

[Timon]

On 12/2/17, Jonathan Nieder  wrote:
> Jonathan Nieder wrote:
>
>> From: Johannes Schindelin 
>> Subject: git-gui: allow Ctrl+T to toggle multiple paths
>>
>> In the Unstaged Changes panel, selecting multiple lines (using
>> shift+click) and pressing ctrl+t to stage them causes one file to be
>> staged instead of all of the selected files.  The same also happens
>> when unstaging files.
>>
>> This regression appears to have been introduced by gitgui-0.21.0~7^2~1
>> (Allow keyboard control to work in the staging widgets, 2016-10-01).
>>
>> Also reported by zosrothko as a Git-for-Windows issue:
>> https://github.com/git-for-windows/git/issues/1012
>>
>> [jn: fleshed out commit message]
>>
>> Reported-by: Timon 
>> Signed-off-by: Johannes Schindelin 
>> Signed-off-by: Jonathan Nieder 
>
> Gah, this should say:
>
> Signed-off-by; Jonathan Nieder 
>
> [...]
>>> I applied it locally to git-2.15.0 and can confirm it fixed the problem
>>> for me.
>>> If you need any more info/help from me, or would like me to test
>>> anything, feel free to ask.
>>
>> Thanks for this pointer.  I'm including the patch here so the project
>> can consider applying it (it doesn't appear to have been sent upstream
>> before).  I have not tested it or verified the claim it makes about
>> what change introduced this regression --- if you're able to help with
>> that, that would be welcome.
>
> Can you bisect?  That is:
>
>  git clone git://repo.or.cz/git-gui
>  cd git-gui
>  git bisect start
>  git bisect good gitgui-0.20.0
>  git bisect bad gitgui-0.21.0
>
> Then cut to the chase:
>
>  git checkout gitgui-0.21.0~7^2~1
>  ... test test test ...
>  git bisect (good|bad)
>
>  git checkout gitgui-0.21.0~7^2~1^
>  ... test test test ...
>  git bisect (good|bad)
>
> and follow the instructions if it suggests testing additional versions.
>
> Then I'll be happy to re-send the patch with the results from your
> testing.
>
> Thanks again,
> Jonathan
>

Did the testing and it went smoothly with the outcome:

088ad75dc279614849f92e5ae0a2b579b26719eb is the first bad commit
commit 088ad75dc279614849f92e5ae0a2b579b26719eb
Author: Pat Thoyts 
Date:   Sat Oct 1 22:04:39 2016 +0100

Allow keyboard control to work in the staging widgets.

Keyboard focus was restricted to the commit message widget and users were
forced to use the mouse to select files in the workdir widget and only then
could use a key combination to stage the file.
It is now possible to use key navigation (Ctrl-Tab, arrow keys and Ctrl-T
or Ctrl-U) to stage and unstage files.
Suggested by @koppor in git-for-window/git issue #859

Signed-off-by: Pat Thoyts 

:100755 100755 11048c7a0e94f598b168de98d18fda9aea420c7d
ec1cc43e8f73acd3d52047ece4dfab5edeb6ffb4 M  git-gui.sh

DARPA icon test

[Jacob Chassereau]

https://www.w3.org/Icons/DARPA/inline.html


Sent from my iPhone

[PATCH] list-objects-filter-options: fix 'keword' typo in comment

[Christian Couder]

From: Christian Couder 

Signed-off-by: Christian Couder 

---
 list-objects-filter-options.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/list-objects-filter-options.c b/list-objects-filter-options.c
index 50b98109eb..6729a57dee 100644
--- a/list-objects-filter-options.c
+++ b/list-objects-filter-options.c
@@ -8,7 +8,7 @@
 #include "list-objects-filter-options.h"
 
 /*
- * Parse value of the argument to the "filter" keword.
+ * Parse value of the argument to the "filter" keyword.
  * On the command line this looks like:
  *   --filter=
  * and in the pack protocol as:
-- 
2.15.0.16.g80a053294d

Re: [PATCH v4 2/2] launch_editor(): indicate that Git waits for user input

[Lars Schneider]

> On 03 Dec 2017, at 06:15, Junio C Hamano  wrote:
> 
> Jeff King  writes:
> 
>> I tried to think of ways this "show a message and then delete it" could
>> go wrong. It should work OK with editors that just do curses-like
>> things, taking over the terminal and then restoring it at the end.
>> ...
> 
> I think that it is not worth to special-case "dumb" terminals like
> this round of the patches do.  If it so much disturbs reviewers that
> "\e[K" may not work everywhere, we can do without the "then delete
> it" part.  It was merely trying to be extra nice, and the more
> important part of the "feature" is to be noticeable, and I do think
> that not showing anything on "dumb", only because the message cannot
> be retracted, is putting the cart before the horse.  
> 
> Since especially now people are hiding this behind an advise.*
> thing, I think it is OK to show a message and waste a line, even.

Well, my reasoning was to minimize the risk of bothering people:
People using "dumb" terminals wouldn't be bothered because nothing
changes and people using "smart" terminals would see the message
only temporarily. Of course, emacsclient users would be the
exception. They would always be bothered and therefore I added
the "advice" switch.

That being said, I can also follow your logic. If we have such
a feature then we shouldn't surprise the user. We should make it 
consistently available in all environments.

I am on the fence here. I like consistency but I don't want to
bother Git users.

@Peff: Do you lean into either direction? Could you imagine that
novice/regular users are bothered? (I don't expect experts to be
bothered too much as they would likely be able to find and set 
the advice config).


Thanks,
Lars

[PATCH v2] Makefile: replace perl/Makefile.PL with simple make rules

[Ævar Arnfjörð Bjarmason]

Replace the perl/Makefile.PL and the fallback perl/Makefile used under
NO_PERL_MAKEMAKER=NoThanks with a much simpler implementation heavily
inspired by how the i18n infrastructure's build process works[1].

The reason for having the Makefile.PL in the first place is that it
was initially[2] building a perl C binding to interface with libgit,
this functionality, that was removed[3] before Git.pm ever made it to
the master branch.

We've since since started maintaining a fallback perl/Makefile, as
MakeMaker wouldn't work on some platforms[4]. That's just the tip of
the iceberg. We have the PM.stamp hack in the top-level Makefile[5] to
detect whether we need to regenerate the perl/perl.mak, which I fixed
just recently to deal with issues like the perl version changing from
under us[6].

There is absolutely no reason for why this needs to be so complex
anymore. All we're getting out of this elaborate Rube Goldberg machine
was copying perl/* to perl/blib/* as we do a string-replacement on
the *.pm files to hardcode @@LOCALEDIR@@ in the source, as well as
pod2man-ing Git.pm & friends.

So replace the whole thing with something that's pretty much a copy of
how we generate po/build/**.mo from po/*.po, just with a small sed(1)
command instead of msgfmt. As that's being done rename the files
from *.pm to *.pmc just to indicate that they're genreated (see
"perldoc -f require").

While I'm at it, change the fallback for Error.pm from being something
where we'll ship our own Error.pm if one doesn't exist at build time
to one where we just use a Git::Error wrapper that'll always prefer
the system-wide Error.pm, only falling back to our own copy if it
really doesn't exist at runtime. It's now shipped as
Git::FromCPAN::Error, making it easy to add other modules to
Git::FromCPAN::* in the future if that's needed.

Functional changes:

 * This will not always install into perl's idea of its global
   "installsitelib". This only potentially matters for packagers that
   need to expose Git.pm for non-git use, and as explained in the
   INSTALL file there's a trivial workaround.

 * The scripts themselves will 'use lib' the target directory, but if
   INSTLIBDIR is set it overrides it. It doesn't have to be this way,
   it could be set in addition to INSTLIBDIR, but my reading of [7] is
   that this is the desired behavior.

 * We don't man pages for all of the perl modules as we used t, only
   Git(3pm). As discussed on-list[8] that we were building installed
   manpages for purely internal APIs like Git::I18N or
   private-Error.pm was always a bug anyway, and all the Git::SVN::*
   ones say they're internal APIs.

   There are apparently external users of Git.pm, but I don't expect
   there to be any of the others.

   As a side-effect of these general changes the perl documentation
   now only installed by install-{doc,man}, not a mere "install" as
   before.

1. 5e9637c629 ("i18n: add infrastructure for translating Git with
   gettext", 2011-11-18)

2. b1edc53d06 ("Introduce Git.pm (v4)", 2006-06-24)

3. 18b0fc1ce1 ("Git.pm: Kill Git.xs for now", 2006-09-23)

4. f848718a69 ("Make perl/ build procedure ActiveState friendly.",
   2006-12-04)

5. ee9be06770 ("perl: detect new files in MakeMaker builds",
   2012-07-27)

6. c59c4939c2 ("perl: regenerate perl.mak if perl -V changes",
   2017-03-29)

7. 0386dd37b1 ("Makefile: add PERLLIB_EXTRA variable that adds to
   default perl path", 2013-11-15)

8. 87bmjjv1pu@evledraar.booking.com ("Re: [PATCH] Makefile:
   replace perl/Makefile.PL with simple make rules"

Signed-off-by: Ævar Arnfjörð Bjarmason 
---

Per my reading of the list discussion this should be OK to apply. We
now build the manpage for Git.pm (but no other *.pm, as discussed).

Aside from the improved commit message & POD building I updated the
INSTALL instructions & we now build stuff in perl/build/{lib,man}
instead of just PM stuff in perl/build/lib. tbdiff with v2 follows:

1: 6eae4593dc ! 1: 90ee965ea6 Makefile: replace perl/Makefile.PL with 
simple make rules
@@ -21,7 +21,8 @@
 There is absolutely no reason for why this needs to be so complex
 anymore. All we're getting out of this elaborate Rube Goldberg 
machine
 was copying perl/* to perl/blib/* as we do a string-replacement on
-the *.pm files to hardcode @@LOCALEDIR@@ in the source.
+the *.pm files to hardcode @@LOCALEDIR@@ in the source, as well as
+pod2man-ing Git.pm & friends.
 
 So replace the whole thing with something that's pretty much a 
copy of
 how we generate po/build/**.mo from po/*.po, just with a small 
sed(1)
@@ -49,18 +50,18 @@
it could be set in addition to INSTLIBDIR, but my reading of 
[7] is
that this is the desired behavior.
 
- * We don't build the Git(3) Git::I18N(3) etc. man pages from the
-   embedded perldoc. I 

Re: [PATCH v4 1/4] Makefile: generate Perl header from template file

[Ævar Arnfjörð Bjarmason]

On Sun, Dec 03 2017, Junio C. Hamano jotted:

> Johannes Sixt  writes:
>
>>> +   sed -e 's=@@PATHSEP@@=$(pathsep)=g' \
>>
>> This doesn't work, unfortunately. When $(pathsep) is ';', we get an
>> incomplete sed expression because ';' is also a command separator in
>> the sed language.
>
> It is correct that ';' can be and does get used in place of LF when
> writing a script on a single line, but even then, as part of a
> string argument to 's' command (and also others), there is no need
> to quote ';' or otherwise treat it any specially, as the commands
> know what their syntax is (e.g. 's=string=replacement=' after seeing
> the first '=' knows that it needs to find one unquoted '=' to find
> the end of the first argument, and another to find the end of the
> replacement string, and ';' seen during that scanning would not have
> any special meaning).
>
> If your sed is so broken and does not satisfy the above expectation,
> t6023 would not work for you, I would gess.
>
> t/t6023-merge-file.sh:sed -e "s/deerit.\$/deerit;/" -e "s/me;\$/me./" < 
> new5.txt > new6.txt
> t/t6023-merge-file.sh:sed -e "s/deerit.\$/deerit,/" -e "s/me;\$/me,/" < 
> new5.txt > new7.txt
> t/t6023-merge-file.sh:sed -e 's/deerit./&/' -e "s/locavit,/locavit;/"< 
> new6.txt | tr '%' '\012' > new8.txt

Since this whole thing is guarded by "ifndef NO_PERL" Dan could just be
using "perl -pe" here instead of fiddling around with the portability
edge cases of sed, e.g.:

perl -pe 's[foo][bar[g' out