date:20180227

On Wed, Feb 28, 2018 at 07:42:51AM +, Eric Wong wrote:

> > > >  a) We could override the meaning of die() in Git.pm.  This feels
> > > > ugly but if it works, it would be a very small patch.
> > > 
> > > Unlikely to work since I think we use eval {} to trap exceptions
> > > from die.
> > > 
> > > >  b) We could forbid use of die() and use some git_die() instead (but
> > > > with a better name) for our own error handling.
> > > 
> > > Call sites may be dual-use: "die" can either be caught by an
> > > eval or used to show an error message to the user.
> 
> 
> 
> > > >  d) We could wrap each command in an eval {...} block to convert the
> > > > result from die() to exit 128.
> > > 
> > > I prefer option d)
> > 
> > FWIW, I agree with all of that. You can do (d) without an enclosing eval
> > block by just hooking the __DIE__ handler, like:
> > 
> > $SIG{__DIE__} = sub {
> >   print STDERR "fatal: @_\n";
> >   exit 128;
> > };
> 
> Looks like it has the same problems I pointed out with a) and b).

You're right. I cut down my example too much and dropped the necessary
eval magic. Try this:

-- >8 --
SIG{__DIE__} = sub {
  CORE::die @_ if $^S || !defined($^S);
  print STDERR "fatal: @_";
  exit 128;
};

eval {
  die "inside eval";
};
print "eval status: $@" if $@;

die "outside eval";
-- 8< --

Running that should produce:

$ perl foo.pl; echo $?
eval status: inside eval at foo.pl line 8.
fatal: outside eval at foo.pl line 12.
128

It may be getting a little too black-magic, though. Embedding in an eval
is at least straightforward, if a bit more invasive.

-Peff

Darlehen

2018-02-27 Thread Scotwest Credit Union Limited



Gre an Dich,

GELD VERFGBAR ZU VERLEIHEN. Holen Sie sich das Geld / Darlehen, das Sie 
bei Scotwest Credit Union Limited bentigen. Wir sind private Kreditgeber 
/ Investoren und bieten sowohl Privatdarlehen, Startdarlehen, Bildungs- / 
Agrarkredit, Immobilien- / Baudarlehen, Immobilienkredit, schuldenfreie 
Kredite, Mobilheimkredit, Hartgeldkredit, besicherte / ungesicherte Kredite, 
Investitionsfinanzierung, Expansion Darlehen, JV-Kapital / Reha-Darlehen, 
Eigenkapital / Refinanzierungsdarlehen usw. an interessierte Personen aus jedem 
Land. Wir bieten Darlehen an Personen national / international zu einem 
niedrigen Zinssatz von 3%. Wenn Sie von Banken oder anderen Kreditgebern 
abgelehnt wurden, ist Scotwest Credit Union Limited hier, um Ihnen bei der 
Archivierung Ihres Ziels zu helfen. Wenn Sie ein Darlehen jeder Art 
bentigen, kontaktieren Sie uns ber die unten stehende 
E-Mail-Adresse und wir sind hier, um Ihnen zu helfen, was Sie brauchen: 
scot.wes...@gmail.com

Ihre vollstndigen Namen:
Adresse:
Telefonnummer:
Kreditbetrag bentigt:
Dauer:
Besetzung:
Monatliches Einkommen
Geschlecht:
Geburtsdatum:
Zustand:
Land:
Postleitzahl:
Zweck:

Sobald Sie diese Informationen zur Verfgung stellen, knnen wir 
Ihnen die Rckzahlung des Darlehens auf monatlicher Basis anbieten

Wir erwarten Ihre schnelle Antwort.


Vielen Dank.
Herr R. Ashley.

Re: [Problem] test_must_fail makes possibly questionable assumptions about exit_code.

2018-02-27 Thread Eric Wong

Jeff King  wrote:
> On Wed, Feb 28, 2018 at 04:07:18AM +, Eric Wong wrote:
> 
> > > In the rest of git, die() makes a command exit with status 128.  The
> > > trouble here is that our code in Perl is assuming the same meaning for
> > > die() but using perl's die builtin instead.  That suggests a few
> > > options:
> > > 
> > >  a) We could override the meaning of die() in Git.pm.  This feels
> > > ugly but if it works, it would be a very small patch.
> > 
> > Unlikely to work since I think we use eval {} to trap exceptions
> > from die.
> > 
> > >  b) We could forbid use of die() and use some git_die() instead (but
> > > with a better name) for our own error handling.
> > 
> > Call sites may be dual-use: "die" can either be caught by an
> > eval or used to show an error message to the user.



> > >  d) We could wrap each command in an eval {...} block to convert the
> > > result from die() to exit 128.
> > 
> > I prefer option d)
> 
> FWIW, I agree with all of that. You can do (d) without an enclosing eval
> block by just hooking the __DIE__ handler, like:
> 
> $SIG{__DIE__} = sub {
>   print STDERR "fatal: @_\n";
>   exit 128;
> };

Looks like it has the same problems I pointed out with a) and b).

Re: [PATCH] revision.c: reduce object database queries

On Tue, Feb 27, 2018 at 03:16:58PM -0800, Junio C Hamano wrote:

> >> This code comes originally form 454fbbcde3 (git-rev-list: allow missing
> >> objects when the parent is marked UNINTERESTING, 2005-07-10). But later,
> >> in aeeae1b771 (revision traversal: allow UNINTERESTING objects to be
> >> missing, 2009-01-27), we marked dealt with calling parse_object() on the
> >> parents more directly.
> >>
> >> So what I wonder is whether this code is simply redundant and can go
> >> away entirely. That would save the has_object_file() call in all cases.
> 
> Hmm, interesting. I forgot all what I did around this area, but you
> are right.

I'll leave it to Stolee whether he wants to dig into removing the
has_object_file() call. I think it would do the right thing, but the
most interesting bit would be how it impacts the timings.

> > There's a similar case for trees. ...
> > though technically the existing code allows _missing_ trees, but
> > not on corrupt ones.
> 
> True, but the intention of these "do not care too much about missing
> stuff while marking uninteresting" effort is aligned better with
> ignoring corrupt ones, too, I would think, as "missing" in that
> sentence is in fact about "not availble", and stuff that exists in
> corrupt form is still not available anyway.  So I do not think it
> makes a bad change to start allowing corrupt ones.

Agreed. Here it is in patch form, though as we both said, it probably
doesn't matter that much in practice. So I'd be OK dropping it out of
a sense of conservatism.

-- >8 --
Subject: [PATCH] mark_tree_contents_uninteresting: drop has_object check

It's generally acceptable for UNINTERESTING objects in a
traversal to be unavailable (e.g., see aeeae1b771). When
marking trees UNINTERESTING, we access the object database
twice: once to check if the object is missing (and return
quietly if it is), and then again to actually parse it.

We can instead just try to parse; if that fails, we can then
return quietly. That halves the effort we spend on locating
the object.

Note that this isn't _exactly_ the same as the original
behavior, as the parse failure could be due to other
problems than a missing object: it could be corrupted, in
which case the original code would have died. But the new
behavior is arguably better, as it covers the object being
unavailable for any reason. We'll also still issue a warning
to stderr in such a case.

Signed-off-by: Jeff King 
---
 revision.c | 5 +
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/revision.c b/revision.c
index 5ce9b93baa..221d62c52b 100644
--- a/revision.c
+++ b/revision.c
@@ -51,12 +51,9 @@ static void mark_tree_contents_uninteresting(struct tree 
*tree)
 {
struct tree_desc desc;
struct name_entry entry;
-   struct object *obj = >object;

-   if (!has_object_file(>oid))
+   if (parse_tree_gently(tree, 1) < 0)
return;
-   if (parse_tree(tree) < 0)
-   die("bad tree %s", oid_to_hex(>oid));

init_tree_desc(, tree->buffer, tree->size);
while (tree_entry(, )) {
-- 
2.16.2.582.ge2c16ac3c4

Re: [RFC] Rebasing merges: a jorney to the ultimate solution (Road Clear)

Igor Djordjevic  writes:

> On 28/02/2018 03:12, Igor Djordjevic wrote:
>> 
>> Would additional step as suggested in [1] (using R1 and R2 to "catch" 
>> interactive rebase additions/amendments/drops, on top of U1' and 
>> U2'), make more sense (or provide an additional clue, at least)?
>> 
>> [1] 
>> https://public-inbox.org/git/8829c395-fb84-2db0-9288-f7b28fa0d...@gmail.com/
>
> Heh, no sleeping tonight :P
>
> Anyway, from (yet another) ad hoc test, additional step mentioned in 
> [1] above seems to handle this case, too (merge with `-s ours` 
> dropping B* patches, plus B1 cherry-picked between X1..X2):
>
> On 28/02/2018 00:27, Johannes Schindelin wrote:
>> 
>> - One of the promises was that the new way would also handle merge
>>   strategies other than recursive. What would happen, for example, if M
>>   was generated using `-s ours` (read: dropping the B* patches' changes)
>>   and if B1 had been cherry-picked into the history between X1..X2?
>> 
>>   Reverting R would obviously revert those B1 changes, even if B1' would
>>   obviously not even be part of the rebased history!
>> 
>> Yes, I agree that this `-s ours` example is quite concocted, but the point
>> of this example is not how plausible it is, but how easy it is to come up
>> with a scenario where this design to "rebase merge commits" results in
>> very, very unwanted behavior.
>
> And not just that - it worked with additional interactive rebase 
> adding, amending and removing commits, on top of all this still 
> preserving original `-s ours` merge commit evil-merge amendment, too, 
> all as expected (or so it seems) :P

Great! I do believe that once we start from some sensible approach, many
kinds of improvements are possible. I'll definitely need to take close
look at what you came up with, thanks!

I'd like to say though that nobody should expect miracles from automated
rebasing of merges when we get to complex history editing. It will need
to retreat to manual merge, sooner or later.

-- Sergey

Re: [RFC] Rebasing merges: a jorney to the ultimate solution (Road Clear)

Jacob Keller  writes:

> On Tue, Feb 27, 2018 at 10:14 AM, Junio C Hamano  wrote:
>> Sergey Organov  writes:
>>
>>> You've already bit this poor thingy to death. Please rather try your
>>> teeth on the proposed Trivial Merge (TM) method.
>>
>> Whatever you do, do *NOT* call any part of your proposal "trivial
>> merge", unless you are actually using the term to mean what Git
>> calls "trivial merge".  The phrase has an established meaning in Git
>> and your attempt to abuse it to mean something entirely different is
>> adding unnecessary hindrance for other people to understand what you
>> want to perform.
>
> Agreed, I think we need better terminology here, the current words for
> (TM) are definitely *not* trivial merges. Same for "angel merge", I
> don't think that term really works well either.

Agreed.

How do we call a merge that introduces no differences on either side of
the merge then? Is there some English for even more trivial than what
Git calls "trivial merge"?

-- Sergey

Re: [RFC] Rebasing merges: a jorney to the ultimate solution (Road Clear)

Hi Johannes,

Johannes Schindelin  writes:
> Hi Buga,
>
> thank you for making this a lot more understandable to this thick
> developer.
>
> On Tue, 27 Feb 2018, Igor Djordjevic wrote:
>
>> On 27/02/2018 19:55, Igor Djordjevic wrote:
>> > 
>> > It would be more along the lines of "(1) rebase old merge commit parents, 
>> > (2) generate separate diff between old merge commit and each of its 
>> > parents, (3) apply each diff to their corresponding newly rebased 
>> > parent respectively (as a temporary commit, one per rebased parent), 
>> > (4) merge these temporary commits to generate 'rebased' merge commit, 
>> > (5) drop temporary commits, recording their parents as parents of 
>> > 'rebased' merge commit (instead of dropped temporary commits)".
>> > 
>> > Implementation wise, steps (2) and (3) could also be done by simply 
>> > copying old merge commit _snapshot_ on top of each of its parents as 
>> > a temporary, non-merge commit, then rebasing (cherry-picking) these 
>> > temporary commits on top of their rebased parent commits to produce 
>> > rebased temporary commits (to be merged for generating 'rebased' 
>> > merge commit in step (4)).
>> 
>> For those still tagging along (and still confused), here are some 
>> diagrams (following what Sergey originally described). Note that 
>> actual implementation might be even simpler, but I believe it`s a bit 
>> easier to understand like this, using some "temporary" commits approach.
>> 
>> Here`s our starting position:
>> 
>> (0) ---X1---o---o---o---o---o---X2 (master)
>>|\
>>| A1---A2---A3
>>| \
>>|  M (topic)
>>| /
>>\-B1---B2---B3
>> 
>> 
>> Now, we want to rebase merge commit M from X1 onto X2. First, rebase
>> merge commit parents as usual:
>> 
>> (1) ---X1---o---o---o---o---o---X2
>>|\   |\
>>| A1---A2---A3   | A1'--A2'--A3'
>>| \  |
>>|  M |
>>| /  |
>>\-B1---B2---B3   \-B1'--B2'--B3'
>> 
>> 
>> That was commonly understandable part.
>
> Good. Let's assume that I want to do this interactively (because let's
> face it, rebase is boring unless we shake up things a little). And let's
> assume that A1 is my only change to the README, and that I realized that
> it was incorrect and I do not want the world to see it, so I drop A1'.
>
> Let's see how things go from here:
>
>> Now, for "rebasing" the merge commit (keeping possible amendments), we
>> do some extra work. First, we make two temporary commits on top of old
>> merge parents, by using exact tree (snapshot) of commit M:
>> 
>> (2) ---X1---o---o---o---o---o---X2
>>|\   |\
>>| A1---A2---A3---U1  | A1'--A2'--A3'
>>| \  |
>>|  M |
>>| /  |
>>\-B1---B2---B3---U2  \-B1'--B2'--B3'
>
> Okay, everything would still be the same except that I still have dropped
> A1'.
>
>> So here, in terms of _snapshots_ (trees, not diffs), U1 = U2 = M.
>> 
>> Now, we rebase these temporary commits, too:
>> 
>> (3) ---X1---o---o---o---o---o---X2
>>|\   |\
>>| A1---A2---A3---U1  | A1'--A2'--A3'--U1'
>>| \  |
>>|  M |
>>| /  |
>>\-B1---B2---B3---U2  \-B1'--B2'--B3'--U2'
>
> I still want to drop A1 in this rebase, so A1' is still missing.
>
> And now it starts to get interesting.
>
> The diff between A3 and U1 does not touch the README, of course, as I said
> that only A1 changed the README.  But the diff between B3 and U2 does
> change the README, thanks to M containing A1 change.
>
> Therefore, the diff between B3' and U2' will also have this change to the
> README. That change that I wanted to drop.
>
>> As a next step, we merge these temporary commits to produce our
>> "rebased" merged commit M:
>> 
>> (4) ---X1---o---o---o---o---o---X2
>>|\   |\
>>| A1---A2---A3---U1  | A1'--A2'--A3'--U1'
>>| \  |  \
>>|  M |   M'
>>| /  |  /
>>\-B1---B2---B3---U2  \-B1'--B2'--B3'--U2'
>
> And here, thanks to B3'..U2' changing the README, M' will also have that
> change that I wanted to see dropped.
>
> Note that A1' is still dropped in my example.
>
>> Finally, we drop temporary commits, and record rebased commits A3' 
>> and B3' as our "rebased" merge commit parents instead (merge commit 
>> M' keeps its same tree/snapshot state, just gets parents replaced):
>> 
>> (5) ---X1---o---o---o---o---o---X2
>>|\   |\
>>| A1---A2---A3---U1  | A1'--A2'--A3'
>>| \  | \

Re: [RFC] Rebasing merges: a jorney to the ultimate solution (Road Clear)

Junio C Hamano  writes:

> Igor Djordjevic  writes:
>
>> On 27/02/2018 20:59, Igor Djordjevic wrote:
>>> 
>>> (3) ---X1---o---o---o---o---o---X2
>>>|\   |\
>>>| A1---A2---A3---U1  | A1'--A2'--A3'--U1'
>>>| \  |
>>>|  M |
>>>| /  |
>>>\-B1---B2---B3---U2  \-B1'--B2'--B3'--U2'
>>> 
>>
>> Meh, I hope I`m rushing it now, but for example, if we had decided to 
>> drop commit A2 during an interactive rebase (so losing A2' from 
>> diagram above), wouldn`t U2' still introduce those changes back, once 
>> U1' and U2' are merged, being incorrect/unwanted behavior...? :/
>>
>> p.s. Looks like Johannes already elaborated on this in the meantime, 
>> let`s see... (goes reading that other e-mail[1])
>
> As long as we are talking about rebase that allows us users to
> adjust and make changes ("rebase -i" being the prime and most
> flexible example), it is easy to imagine that A1'--A3' and B1'--B3'
> have almost no relation to their original counterparts.  After all,
> mechanical merge won't be able to guess the intention of the change
> humans make, so depending on what happend during X1 and X2 that
> happend outside of these two topics, what's required to bring series
> A and B to series A' and B' can be unlimited.

You discuss some different method here. In my original proposal U1' and
U2' are to be merged. Nothing should be replayed on top of them. I.e.,
U1' is _already_ the result of replaying difference between M and A3 on
top of A3'.

> So from that alone, it should be clear that replaying difference
> between M and A3 (and M and B3) on top of U1' and U2' is hopeless as a
> general solution.

Getting U1' from U1 is the same complexity as getting A3' from A3, with
the same caveats. So, as general solution, it's as good as rebase of
non-merge commit.

> It is acceptable as long as a solution fails loudly when it does the
> wrong thing, but I do not think the apporach can produce incorrect
> result silently, as your example shows above.

I still believe the method handles simple cases automatically and
correctly and allows to immediately stop for amendment should something
suspect appear.

-- Sergey

Re: [RFC] Rebasing merges: a jorney to the ultimate solution (Road Clear)

Igor Djordjevic  writes:

> On 28/02/2018 01:36, Jacob Keller wrote:
>> 
>> > > (3) ---X1---o---o---o---o---o---X2
>> > >|\   |\
>> > >| A1---A2---A3---U1  | A1'--A2'--A3'--U1'
>> > >| \  |
>> > >|  M |
>> > >| /  |
>> > >\-B1---B2---B3---U2  \-B1'--B2'--B3'--U2'
>> > >
>> >
>> > Meh, I hope I`m rushing it now, but for example, if we had decided to
>> > drop commit A2 during an interactive rebase (so losing A2' from
>> > diagram above), wouldn`t U2' still introduce those changes back, once
>> > U1' and U2' are merged, being incorrect/unwanted behavior...? :/
>> 
>> In that case, the method won't work well at all, so I think we need a
>> different approach.
>> 
>
> Hmm, still rushing it, but what about adding an additional step, 
> something like this:

I think it's unneeded, as it should work fine without it, see another
reply.

-- Sergey

Re: [RFC] Rebasing merges: a jorney to the ultimate solution (Road Clear)

Igor Djordjevic  writes:

> On 27/02/2018 20:59, Igor Djordjevic wrote:
>> 
>> (3) ---X1---o---o---o---o---o---X2
>>|\   |\
>>| A1---A2---A3---U1  | A1'--A2'--A3'--U1'
>>| \  |
>>|  M |
>>| /  |
>>\-B1---B2---B3---U2  \-B1'--B2'--B3'--U2'
>> 
>
> Meh, I hope I`m rushing it now, but for example, if we had decided to 
> drop commit A2 during an interactive rebase (so losing A2' from 
> diagram above), wouldn`t U2' still introduce those changes back, once 
> U1' and U2' are merged, being incorrect/unwanted behavior...? :/

I think the method will handle this nicely.

When you "drop" A2, will A3' change, or stay intact?

If it changes, say, to A3'', the U1' will change to U1'', and the method
will propagate the change automatically.

If it A3' doesn't change, then there are no changes to take.

-- Sergey

Re: [Problem] test_must_fail makes possibly questionable assumptions about exit_code.

On Wed, Feb 28, 2018 at 04:07:18AM +, Eric Wong wrote:

> > In the rest of git, die() makes a command exit with status 128.  The
> > trouble here is that our code in Perl is assuming the same meaning for
> > die() but using perl's die builtin instead.  That suggests a few
> > options:
> > 
> >  a) We could override the meaning of die() in Git.pm.  This feels
> > ugly but if it works, it would be a very small patch.
> 
> Unlikely to work since I think we use eval {} to trap exceptions
> from die.
> 
> >  b) We could forbid use of die() and use some git_die() instead (but
> > with a better name) for our own error handling.
> 
> Call sites may be dual-use: "die" can either be caught by an
> eval or used to show an error message to the user.
> 
> >  c) We could have a special different exit code convention for
> > commands written in Perl.  And then change expectations whenever a
> > command is rewritten in C.  As you might expect, I don't like this
> > option.
> 
> I don't like it, either.
> 
> >  d) We could wrap each command in an eval {...} block to convert the
> > result from die() to exit 128.
> 
> I prefer option d)

FWIW, I agree with all of that. You can do (d) without an enclosing eval
block by just hooking the __DIE__ handler, like:

$SIG{__DIE__} = sub {
  print STDERR "fatal: @_\n";
  exit 128;
};

-Peff

Re: [RFC] Rebasing merges: a jorney to the ultimate solution (Road Clear)

Junio C Hamano  writes:

> Sergey Organov  writes:
>
>> You've already bit this poor thingy to death. Please rather try your
>> teeth on the proposed Trivial Merge (TM) method.
>
> Whatever you do, do *NOT* call any part of your proposal "trivial
> merge", unless you are actually using the term to mean what Git
> calls "trivial merge".  The phrase has an established meaning in Git
> and your attempt to abuse it to mean something entirely different is
> adding unnecessary hindrance for other people to understand what you
> want to perform.

Yeah, got it. It's confusing indeed.

-- Sergey

[PATCH] hooks/pre-auto-gc-battery: allow gc to run on non-laptops

2018-02-27 Thread Adam Borowski

Desktops and servers tend to have no power sensor, thus on_ac_power returns
255 ("unknown").

If that tool returns "unknown", there's no point in querying other sources
as it already queried them, and is smarter than us (can handle multiple
adapters).

Reported by: Xin Li 
Signed-off-by: Adam Borowski 
---
 contrib/hooks/pre-auto-gc-battery | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/contrib/hooks/pre-auto-gc-battery 
b/contrib/hooks/pre-auto-gc-battery
index 6a2cdebdb..7ba78c4df 100755
--- a/contrib/hooks/pre-auto-gc-battery
+++ b/contrib/hooks/pre-auto-gc-battery
@@ -17,7 +17,7 @@
 # ln -sf /usr/share/git-core/contrib/hooks/pre-auto-gc-battery \
 #  hooks/pre-auto-gc
 
-if test -x /sbin/on_ac_power && /sbin/on_ac_power
+if test -x /sbin/on_ac_power && (/sbin/on_ac_power;test $? -ne 1)
 then
exit 0
 elif test "$(cat /sys/class/power_supply/AC/online 2>/dev/null)" = 1
-- 
2.16.2

Re: [RFC] Rebasing merges: a jorney to the ultimate solution (Road Clear)

On 28/02/2018 03:12, Igor Djordjevic wrote:
> 
> Would additional step as suggested in [1] (using R1 and R2 to "catch" 
> interactive rebase additions/amendments/drops, on top of U1' and 
> U2'), make more sense (or provide an additional clue, at least)?
> 
> [1] 
> https://public-inbox.org/git/8829c395-fb84-2db0-9288-f7b28fa0d...@gmail.com/

Heh, no sleeping tonight :P

Anyway, from (yet another) ad hoc test, additional step mentioned in 
[1] above seems to handle this case, too (merge with `-s ours` 
dropping B* patches, plus B1 cherry-picked between X1..X2):

On 28/02/2018 00:27, Johannes Schindelin wrote:
> 
> - One of the promises was that the new way would also handle merge
>   strategies other than recursive. What would happen, for example, if M
>   was generated using `-s ours` (read: dropping the B* patches' changes)
>   and if B1 had been cherry-picked into the history between X1..X2?
> 
>   Reverting R would obviously revert those B1 changes, even if B1' would
>   obviously not even be part of the rebased history!
> 
> Yes, I agree that this `-s ours` example is quite concocted, but the point
> of this example is not how plausible it is, but how easy it is to come up
> with a scenario where this design to "rebase merge commits" results in
> very, very unwanted behavior.

And not just that - it worked with additional interactive rebase 
adding, amending and removing commits, on top of all this still 
preserving original `-s ours` merge commit evil-merge amendment, too, 
all as expected (or so it seems) :P

Again, for the brave ones, here`s another messy test script (not 
tightly related to the last discussed diagrams, but based on my 
original "demonstration" script[2])... Hopefully I get to make a 
proper (and sane) sample soon... if not missing something here in the 
first place ;)

Regards, Buga

[2] https://public-inbox.org/git/bbe64321-4d3a-d3fe-8bb9-58b600fab...@gmail.com/

-- 8< --
#!/bin/sh

# rm -rf ./.git
# rm -f ./test.txt

git init

touch ./test.txt
git add -- test.txt

for i in {1..20}
do
echo A$i >>test.txt
git commit -am "A$i"
done

git checkout -b b1
sed -i '3iB11' test.txt
git commit -am "B11"
sed -i '7iB12' test.txt
git commit -am "B12"

git checkout -b b2 HEAD^
sed -i '16iB21' test.txt
git commit -am "B21"
sed -i '18iB22' test.txt
git commit -am "B22"

git checkout -b merge b1
git merge -s ours --no-commit b2
sed -i '12iX' test.txt # amend merge commit
git commit -am "M"
git tag original-merge

git checkout master
git cherry-pick b2
for i in {1..5}
do
j=`expr "$i" + 20`
sed -i "${i}iA${j}" test.txt
git commit -am "A$j"
done

# simple/naive demonstration of proposed merge rebasing logic
# using described "Trivial Merge" (TM, or "Angel Merge"),
# preserving merge commit manual amendments, but still respecting
# interactively rebased added/modified/dropped commits :)

# read -p "Press enter to continue"
git checkout b1
git cherry-pick -m1 original-merge && git tag U1
git reset --hard HEAD^^ # drop U1 and last b1 commit
sed -i '/B11/c\B' test.txt
git commit -a --amend --no-edit
git rebase master
git cherry-pick U1 && git tag U1-prime

# read -p "Press enter to continue"
git checkout b2
git cherry-pick -m2 original-merge && git tag U2
git reset --hard HEAD^ # drop U2
git rebase master
sed -i '20iBX' test.txt
git commit -am "BX" # add new commit
git cherry-pick U2 && git tag U2-prime

git diff U1 U1-prime | git apply --3way && git commit -m "U2-second" && git tag 
U2-second
git checkout b1
git diff U2 U2-prime | git apply --3way && git commit -m "U1-second" && git tag 
U1-second

# read -p "Press enter to continue"
git branch -f merge b1
git checkout merge
git merge b2 --no-commit
git commit -a --reuse-message original-merge
git tag angel-merge

# read -p "Press enter to continue"
git reset --hard b1^
git read-tree --reset angel-merge
git update-ref refs/heads/merge "$(git show -s --format=%B original-merge | git 
commit-tree "$(git write-tree)" -p "$(git rev-parse b1^^)" -p "$(git rev-parse 
b2^^)")"
git tag -f angel-merge
git checkout angel-merge .
git branch -f b1 b1^^
git branch -f b2 b2^^

# show resulting graph
echo
git log --all --decorate --oneline --graph

# comparison between original merge and rebased merge,
# showing merge commit amendment "X" being preserved during rebase
# (not shown in diff)
echo
echo 'diff original-merge angel-merge:'
git diff original-merge angel-merge

F.LLI PISTOLESI Snc Company

2018-02-27 Thread .F.LLI PISTOLESI

Hello , 
 


I am looking for a reliable supplier /manufacturer of products for sell in 
Europe.

I came across your listing and wanted to get some information regarding minimum 
Order Quantities, FOB pricing and also the possibility of packaging including 
payments terms.

So could you please get back to be with the above informations as soon as 
possible .

My email is :tm6428...@gmail.com

Many thanks and i looking forward to hearing from you and hopefully placing an 
order with you company.

Best Regards
Lorenzo Delleani.

F.LLI PISTOLESI Snc Company P.O. box 205
2740 AE Waddinxveen
The Netherlands

Re: [Problem] test_must_fail makes possibly questionable assumptions about exit_code.

2018-02-27 Thread Eric Wong

Jonathan Nieder  wrote:
> The fundamental thing is the actual Git commands, not the tests in the
> testsuite, no?

Right.  I've never been picky about exit codes; only that a
non-zero happens on errors.

> In the rest of git, die() makes a command exit with status 128.  The
> trouble here is that our code in Perl is assuming the same meaning for
> die() but using perl's die builtin instead.  That suggests a few
> options:
> 
>  a) We could override the meaning of die() in Git.pm.  This feels
> ugly but if it works, it would be a very small patch.

Unlikely to work since I think we use eval {} to trap exceptions
from die.

>  b) We could forbid use of die() and use some git_die() instead (but
> with a better name) for our own error handling.

Call sites may be dual-use: "die" can either be caught by an
eval or used to show an error message to the user.

>  c) We could have a special different exit code convention for
> commands written in Perl.  And then change expectations whenever a
> command is rewritten in C.  As you might expect, I don't like this
> option.

I don't like it, either.

>  d) We could wrap each command in an eval {...} block to convert the
> result from die() to exit 128.

I prefer option d)

Re: [RFC] Rebasing merges: a jorney to the ultimate solution (Road Clear)

Hi Junio,

On 28/02/2018 01:10, Junio C Hamano wrote:
> 
> > > (3) ---X1---o---o---o---o---o---X2
> > >|\   |\
> > >| A1---A2---A3---U1  | A1'--A2'--A3'--U1'
> > >| \  |
> > >|  M |
> > >| /  |
> > >\-B1---B2---B3---U2  \-B1'--B2'--B3'--U2'
> > >
> >
> > Meh, I hope I`m rushing it now, but for example, if we had decided to 
> > drop commit A2 during an interactive rebase (so losing A2' from 
> > diagram above), wouldn`t U2' still introduce those changes back, once 
> > U1' and U2' are merged, being incorrect/unwanted behavior...? :/
> 
> As long as we are talking about rebase that allows us users to
> adjust and make changes ("rebase -i" being the prime and most
> flexible example), it is easy to imagine that A1'--A3' and B1'--B3'
> have almost no relation to their original counterparts.  After all,
> mechanical merge won't be able to guess the intention of the change
> humans make, so depending on what happend during X1 and X2 that
> happend outside of these two topics, what's required to bring series
> A and B to series A' and B' can be unlimited.  So from that alone,
> it should be clear that replaying difference between M and A3 (and M
> and B3) on top of U1' and U2' is hopeless as a general solution.

Yeah, I`ve encountered it in my (trivial) test case :(

> It is acceptable as long as a solution fails loudly when it does the
> wrong thing, but I do not think the apporach can produce incorrect
> result silently, as your example shows above.

Hmm, I think my example doesn`t even try to prevent failing, but it 
should otherwise be perfectly capable of doing so (and doing it 
loudly) - for example, it`s enough to diff U1' and U2' - if not the 
same, user might want to confirm the "rebased" merge outcome, as 
either something went wrong, or interactive rebase happened... or 
both :) (it`s what Sergey originally explained, seeming to be a solid 
safety net, though more testing would be good)

> What you _COULD_ learn from an old merge is to compute:
> 
> - a trial and mechanical merge between A3 and B3; call that pre-M
> 
> - diff to bring pre-M to M (call that patch evil-M); which is
>   what the person who made M did to resolve the textual and
>   semantic conflicts necessary to merge these two topics.
> 
> Then when merging A3' and B3', you could try to mechanically merge
> them (call that pre-M'), and apply evil-M, which may apply cleanly
> on top of pre-M', or it may not.  When there aren't so huge a
> difference between series A and A' (and series B and B'), the result
> would probably be a moral equivalent of Sergay's "replay" (so this
> approach will also silently produce a wrong result without human
> supervision).  One edge the evil-M approach has over Sergey's "dual
> cherry pick" is that it separates and highlights non-mechanical
> conflict resolution out of mechanical merges in a human readable
> form (i.e. the patch evil-M).

This seems to be what Johannes wrote about[1], too, but also 
something I think would be good to avoid, if possible, not 
complicating it too much :P

Maybe something like that latest thought[2] could help, using 
additional R1 and R2 commits to handle interactive rebase 
additions/amendments/drops, on top of U1' and U2'? Yeah, and 
that`s not complicating it... ;) :D

Regards, Buga

[1] 
https://public-inbox.org/git/nycvar.qro.7.76.6.1802272330290...@zvavag-6oxh6da.rhebcr.pbec.zvpebfbsg.pbz/
[2] https://public-inbox.org/git/8829c395-fb84-2db0-9288-f7b28fa0d...@gmail.com/

Re: [PATCH] t: send verbose test-helper output to fd 4

On Thu, Feb 22, 2018 at 7:48 AM, Jeff King  wrote:
> This is a repost of the two patches from:
>
>   https://public-inbox.org/git/20180209185710.ga23...@sigill.intra.peff.net/
>
> (now just one patch, since sg/test-i18ngrep graduated and we can do it
> all in one step). The idea got positive feedback, but nobody commented
> on patches and I didn't see them in "What's cooking".
>
> -- >8 --
> Test helper functions like test_must_fail may produce
> messages to stderr when they see a problem. When the tests
> are run with "--verbose", this ends up on the test script's
> stderr, and the user can read it.
>
> But there's a problem. Some tests record stderr as part of
> the test, like:
>
>   test_must_fail git foo 2>output &&
>   test_i18ngrep expected.message output
>
> In this case the error text goes into "output". This makes
> the --verbose output less useful (it also means we might
> accidentally match it in the second, though in practice we
> tend to produce these messages only on error, so we'd abort
> the test when the first command fails).
>
> Let's instead send this user-facing output directly to
> descriptor 4, which always points to the original stderr (or
> /dev/null in non-verbose mode). And it's already forbidden
> to redirect descriptor 4, since we use it for BASH_XTRACEFD,
> as explained in 9be795fbce (t5615: avoid re-using descriptor
> 4, 2017-12-08).
>
> Signed-off-by: Jeff King 
> ---
>  t/test-lib-functions.sh | 20 ++--
>  1 file changed, 10 insertions(+), 10 deletions(-)
>
> diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
> index 67b5994afb..aabee13e5d 100644
> --- a/t/test-lib-functions.sh
> +++ b/t/test-lib-functions.sh
> @@ -625,22 +625,22 @@ test_must_fail () {
> exit_code=$?
> if test $exit_code -eq 0 && ! list_contains "$_test_ok" success
> then
> -   echo >&2 "test_must_fail: command succeeded: $*"
> +   echo >&4 "test_must_fail: command succeeded: $*"
> return 1
> elif test_match_signal 13 $exit_code && list_contains "$_test_ok" 
> sigpipe
> then
> return 0
> elif test $exit_code -gt 129 && test $exit_code -le 192
> then
> -   echo >&2 "test_must_fail: died by signal $(($exit_code - 
> 128)): $*"
> +   echo >&4 "test_must_fail: died by signal $(($exit_code - 
> 128)): $*"
> return 1
> elif test $exit_code -eq 127
> then
> -   echo >&2 "test_must_fail: command not found: $*"
> +   echo >&4 "test_must_fail: command not found: $*"
> return 1
> elif test $exit_code -eq 126
> then
> -   echo >&2 "test_must_fail: valgrind error: $*"
> +   echo >&4 "test_must_fail: valgrind error: $*"
> return 1
> fi
> return 0
> @@ -678,7 +678,7 @@ test_expect_code () {
> return 0
> fi
>
> -   echo >&2 "test_expect_code: command exited with $exit_code, we wanted 
> $want_code $*"
> +   echo >&4 "test_expect_code: command exited with $exit_code, we wanted 
> $want_code $*"
> return 1
>  }

The parts above changing the fds used for error messages in
'test_must_fail' and 'test_expect_code' will become unnecessary if we
take my patch from

  https://public-inbox.org/git/20180225134015.26964-1-szeder@gmail.com/

because that patch also ensures that error messages from this function
will go to fd 4 even if the stderr of the functions is redirected in the
test.  Though, arguably, your patch makes it more readily visible that
those error messages go to fd 4, so maybe it's still worth having.

> @@ -742,18 +742,18 @@ test_i18ngrep () {
> shift
> ! grep "$@" && return 0
>
> -   echo >&2 "error: '! grep $@' did find a match in:"
> +   echo >&4 "error: '! grep $@' did find a match in:"
> else
> grep "$@" && return 0
>
> -   echo >&2 "error: 'grep $@' didn't find a match in:"
> +   echo >&4 "error: 'grep $@' didn't find a match in:"
> fi
>
> if test -s "$last_arg"
> then
> -   cat >&2 "$last_arg"
> +   cat >&4 "$last_arg"
> else
> -   echo >&2 ""
> +   echo >&4 ""
> fi
>
> return 1
> @@ -764,7 +764,7 @@ test_i18ngrep () {
>  # not output anything when they fail.
>  verbose () {
> "$@" && return 0
> -   echo >&2 "command failed: $(git rev-parse --sq-quote "$@")"
> +   echo >&4 "command failed: $(git rev-parse --sq-quote "$@")"
> return 1
>  }

I'm not so sure about these changes to 'test_18ngrep' and 'verbose'.  It
seems that they are the result of a simple s/>&2/>&4/ on
'test-lib-functions.sh'?  The problem described in the commit message
doesn't really applies to them, because their _only_ output to stderr
are the

Re: [PATCH 00/11] Moving global state into the repository object (part 2)

2018-02-27 Thread Duy Nguyen

On Tue, Feb 27, 2018 at 05:05:57PM -0800, Stefan Beller wrote:
> This applies on top of origin/sb/object-store and is the continuation of
> that series, adding the repository as a context argument to functions.
> 
> This series focusses on packfile handling, exposing (re)prepare_packed_git
> and find_pack_entry to a repository argument.

It looks good.

Looking at the full-series diff though, it makes me wonder if we
should keep prepare_packed_git() private (i.e. how we initialize the
object store, packfile included, is a private matter). How about
something like this on top?

-- 8< --
Subject: [PATCH] packfile: keep raw_object_store.packed_git{,_mru} fields 
private

These fields are initialized lazily via prepare_packed_git(). All
access to these must call that function first but unless you know the
implementation details, you may not see the connection.

Keep that connection internal. These fields should only be accessed
via get_packed_git() and get_packed_git_mru() outside packfile.c.

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 builtin/count-objects.c  |  4 +---
 builtin/fsck.c   |  6 ++
 builtin/gc.c |  3 +--
 builtin/pack-objects.c   | 21 +++--
 builtin/pack-redundant.c |  6 ++
 fast-import.c|  7 ++-
 http-backend.c   |  5 ++---
 object-store.h   |  4 ++--
 pack-bitmap.c|  3 +--
 packfile.c   | 15 ++-
 packfile.h   |  6 +-
 server-info.c|  5 ++---
 sha1_name.c  |  6 ++
 13 files changed, 47 insertions(+), 44 deletions(-)

diff --git a/builtin/count-objects.c b/builtin/count-objects.c
index d480301763..088309945b 100644
--- a/builtin/count-objects.c
+++ b/builtin/count-objects.c
@@ -121,9 +121,7 @@ int cmd_count_objects(int argc, const char **argv, const 
char *prefix)
struct strbuf loose_buf = STRBUF_INIT;
struct strbuf pack_buf = STRBUF_INIT;
struct strbuf garbage_buf = STRBUF_INIT;
-   if (!the_repository->objects.packed_git)
-   prepare_packed_git(the_repository);
-   for (p = the_repository->objects.packed_git; p; p = p->next) {
+   for (p = get_packed_git(the_repository); p; p = p->next) {
if (!p->pack_local)
continue;
if (open_pack_index(p))
diff --git a/builtin/fsck.c b/builtin/fsck.c
index 0a043a43c2..6d86f2581a 100644
--- a/builtin/fsck.c
+++ b/builtin/fsck.c
@@ -732,10 +732,8 @@ int cmd_fsck(int argc, const char **argv, const char 
*prefix)
uint32_t total = 0, count = 0;
struct progress *progress = NULL;
 
-   prepare_packed_git(the_repository);
-
if (show_progress) {
-   for (p = the_repository->objects.packed_git; p;
+   for (p = get_packed_git(the_repository); p;
 p = p->next) {
if (open_pack_index(p))
continue;
@@ -744,7 +742,7 @@ int cmd_fsck(int argc, const char **argv, const char 
*prefix)
 
progress = start_progress(_("Checking 
objects"), total);
}
-   for (p = the_repository->objects.packed_git; p;
+   for (p = get_packed_git(the_repository); p;
 p = p->next) {
/* verify gives error messages itself */
if (verify_pack(p, fsck_obj_buffer,
diff --git a/builtin/gc.c b/builtin/gc.c
index 80d19c54d5..be63bec09c 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -173,8 +173,7 @@ static int too_many_packs(void)
if (gc_auto_pack_limit <= 0)
return 0;
 
-   prepare_packed_git(the_repository);
-   for (cnt = 0, p = the_repository->objects.packed_git; p; p = p->next) {
+   for (cnt = 0, p = get_packed_git(the_repository); p; p = p->next) {
if (!p->pack_local)
continue;
if (p->pack_keep)
diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index 5e2590f882..a305f50100 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -1026,7 +1026,7 @@ static int want_object_in_pack(const struct object_id 
*oid,
if (want != -1)
return want;
}
-   list_for_each(pos, _repository->objects.packed_git_mru) {
+   list_for_each(pos, get_packed_git_mru(the_repository)) {
struct packed_git *p = list_entry(pos, struct packed_git, mru);
off_t offset;
 
@@ -1045,7 +1045,7 @@ static int want_object_in_pack(const struct object_id 
*oid,
want = want_found_object(exclude, p);

Re: [RFC] Rebasing merges: a jorney to the ultimate solution (Road Clear)

Hi Johannes,

On 28/02/2018 00:27, Johannes Schindelin wrote:
> 
> thank you for making this a lot more understandable to this thick
> developer.

Hehe, no problem, it primarily served fighting my own thickness ;)

> > Finally, we drop temporary commits, and record rebased commits A3' 
> > and B3' as our "rebased" merge commit parents instead (merge commit 
> > M' keeps its same tree/snapshot state, just gets parents replaced):
> >
> > (5) ---X1---o---o---o---o---o---X2
> >|\   |\
> >| A1---A2---A3---U1  | A1'--A2'--A3'
> >| \  | \
> >|  M |  M'
> >| /  | /
> >\-B1---B2---B3---U2  \-B1'--B2'--B3'
> 
> ...
> 
> In my example, where I dropped A1' specifically so that that embarrasingly
> incorrect change to the README would not be seen by the world, though, the
> evil merge would be truly evil: it would show said change to the world.
> The exact opposite of what I wanted.

Yeah, I`m afraid that`s what my testing produced as well :( Back to 
the drawing board...

> It would have been nice to have such a simple solution ;-)

Eh, the worst thing is the feeling I have, like it`s just around the 
corner, but we`re somehow missing it :P

> So the most obvious way to try to fix this design would be to recreate the
> original merge first, even with merge conflicts, and then trying to use the
> diff between that and the actual original merge commit.

For simplicity sake, this is something I would like to avoid (if 
possible), and also for the reasons you mentioned yourself:

> Now, would this work?
> 
> I doubt it, for at least two reasons:
> 
> - if there are merge conflicts between A3/B3 and between A3'/B3', those
>   merge conflicts will very likely look very different, and the conflicts
>   when reverting R will contain those nested conflicts: utterly confusing.
>   And those conflicts will look even more confusing if a patch (such as
>   A1') was dropped during an interactive rebase.
> 
> - One of the promises was that the new way would also handle merge
>   strategies other than recursive. What would happen, for example, if M
>   was generated using `-s ours` (read: dropping the B* patches' changes)
>   and if B1 had been cherry-picked into the history between X1..X2?
> 
>   Reverting R would obviously revert those B1 changes, even if B1' would
>   obviously not even be part of the rebased history!
> 
> ...
> 
> But maybe I missed something obvious, and the design can still be fixed
> somehow?

Would additional step as suggested in [1] (using R1 and R2 to "catch" 
interactive rebase additions/amendments/drops, on top of U1' and 
U2'), make more sense (or provide an additional clue, at least)?

It`s late here, and I`m really rushing it now, so please forgive me if 
it`s a stupid one... :$

Regards, Buga

[1] https://public-inbox.org/git/8829c395-fb84-2db0-9288-f7b28fa0d...@gmail.com/

Re: [RFC] Rebasing merges: a jorney to the ultimate solution (Road Clear)

On 28/02/2018 02:33, Igor Djordjevic wrote:
> 
> This seems to be working inside my (too trivial?) test case, for 
> interactive adding, dropping, and amending of rebased commits, 
> resulting "rebased" merge containing all the added/modified/dropped 
> changes, plus the original merge amendment, all as expected :P

In case anyone has a wish to examine my (now pretty messy) test 
script, here it is - sorry for not having time to clean it up! :(

What I get when I diff original and "rebased" merge is this:

diff --git a/test.txt b/test.txt
index a82470b..d458032 100644
--- a/test.txt
+++ b/test.txt
@@ -1,10 +1,14 @@
+A21
+A22
+A23
+A24
+A25
 A1
 A2
-B11
+B
 A3
 A4
 A5
-B12
 A6
 A7
 A8
@@ -14,6 +18,7 @@ A10
 A11
 A12
 A13
+BX
 A14
 B2
 A15


... where A21 to A25 are additions due to new base, B11 was 
interactively amended to B, B12 was interactively dropped, and BX 
interactively added :)

We don`t see line X here, being an "evil merge" amendment being 
correctly preserved from original merge commit (thus not a 
difference). If we do `git show` of the "rebased" merge, we get this, 
as expected:

diff --cc test.txt
index b173cef,fad39a8..d458032
--- a/test.txt
+++ b/test.txt
@@@ -13,6 -13,6 +13,7 @@@ A
  A7
  A8
  A9
++X
  A10
  A11
  A12


Regards, Buga

-- 8< --
#!/bin/sh

# rm -rf ./.git
# rm -f ./test.txt

git init

touch ./test.txt
git add -- test.txt

for i in {1..20}
do
echo A$i >>test.txt
git commit -am "A$i"
done

git checkout -b b1
sed -i '3iB11' test.txt
git commit -am "B11"
sed -i '7iB12' test.txt
git commit -am "B12"

git checkout -b b2 HEAD^
sed -i '16iB2' test.txt
git commit -am "B2"

git checkout -b merge b1
git merge --no-commit b2
sed -i '12iX' test.txt # amend merge commit
git commit -am "M"
git tag original-merge

git checkout master
for i in {1..5}
do
j=`expr "$i" + 20`
sed -i "${i}iA${j}" test.txt
git commit -am "A$j"
done

# simple/naive demonstration of proposed merge rebasing logic
# using described "Trivial Merge" (TM, or "Angel Merge"),
# preserving merge commit manual amendments, but still respecting
# interactively rebased added/modified/dropped commits :)

# read -p "Press enter to continue"
git checkout b1
git cherry-pick -m1 original-merge && git tag U1
git reset --hard HEAD^^ # drop U1 and last b1 commit
sed -i '/B11/c\B' test.txt
git commit -a --amend --no-edit
git rebase master
git cherry-pick U1 && git tag U1-prime

# read -p "Press enter to continue"
git checkout b2
git cherry-pick -m2 original-merge && git tag U2
git reset --hard HEAD^ # drop U2
git rebase master
sed -i '20iBX' test.txt
git commit -am "BX" # add new commit
git cherry-pick U2 && git tag U2-prime

git diff U1 U1-prime | git apply --3way && git commit -m "U2-second" && git tag 
U2-second
git checkout b1
git diff U2 U2-prime | git apply --3way && git commit -m "U1-second" && git tag 
U1-second

# read -p "Press enter to continue"
git branch -f merge b1
git checkout merge
git merge b2 --no-commit
git commit -a --reuse-message original-merge
git tag angel-merge

# read -p "Press enter to continue"
git reset --hard b1^
git read-tree --reset angel-merge
git update-ref refs/heads/merge "$(git show -s --format=%B original-merge | git 
commit-tree "$(git write-tree)" -p "$(git rev-parse b1^^)" -p "$(git rev-parse 
b2^^)")"
git tag -f angel-merge
git checkout angel-merge .
git branch -f b1 b1^^
git branch -f b2 b2^^

# show resulting graph
echo
git log --all --decorate --oneline --graph

# comparison between original merge and rebased merge,
# showing merge commit amendment "X" being preserved during rebase
# (not shown in diff)
echo
echo 'diff original-merge angel-merge:'
git diff original-merge angel-merge

[PATCH v3 3/4] sha1_file.c: move delayed getenv(altdb) back to setup_git_env()

getenv() is supposed to work on the main repository only. This delayed
getenv() code in sha1_file.c makes it more difficult to convert
sha1_file.c to a generic object store that could be used by both
submodule and main repositories.

Move the getenv() back in setup_git_env() where other env vars are
also fetched.

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 environment.c  | 1 +
 object-store.h | 5 -
 object.c   | 1 +
 repository.c   | 2 ++
 repository.h   | 1 +
 sha1_file.c| 6 +-
 6 files changed, 10 insertions(+), 6 deletions(-)

diff --git a/environment.c b/environment.c
index 454e435bed..b2128c1188 100644
--- a/environment.c
+++ b/environment.c
@@ -175,6 +175,7 @@ void setup_git_env(const char *git_dir)
args.object_dir = getenv_safe(_free, DB_ENVIRONMENT);
args.graft_file = getenv_safe(_free, GRAFT_ENVIRONMENT);
args.index_file = getenv_safe(_free, INDEX_ENVIRONMENT);
+   args.alternate_db = getenv_safe(_free, ALTERNATE_DB_ENVIRONMENT);
repo_set_gitdir(the_repository, git_dir, );
argv_array_clear(_free);
 
diff --git a/object-store.h b/object-store.h
index afe2f93459..9b1303549b 100644
--- a/object-store.h
+++ b/object-store.h
@@ -87,6 +87,9 @@ struct raw_object_store {
 */
char *objectdir;
 
+   /* Path to extra alternate object database if not NULL */
+   char *alternate_db;
+
struct packed_git *packed_git;
/* A most-recently-used ordered version of the packed_git list. */
struct list_head packed_git_mru;
@@ -109,7 +112,7 @@ struct raw_object_store {
unsigned packed_git_initialized : 1;
 };
 
-#define RAW_OBJECT_STORE_INIT(o) { NULL, NULL, 
LIST_HEAD_INIT(o.packed_git_mru), NULL, NULL, 0, 0, 0 }
+#define RAW_OBJECT_STORE_INIT(o) { NULL, NULL, NULL, 
LIST_HEAD_INIT(o.packed_git_mru), NULL, NULL, 0, 0, 0 }
 
 void raw_object_store_clear(struct raw_object_store *o);
 
diff --git a/object.c b/object.c
index a7c238339b..5317cfc390 100644
--- a/object.c
+++ b/object.c
@@ -464,6 +464,7 @@ static void free_alt_odbs(struct raw_object_store *o)
 void raw_object_store_clear(struct raw_object_store *o)
 {
FREE_AND_NULL(o->objectdir);
+   FREE_AND_NULL(o->alternate_db);
 
free_alt_odbs(o);
o->alt_odb_tail = NULL;
diff --git a/repository.c b/repository.c
index 89e76173a3..b5306ddaa2 100644
--- a/repository.c
+++ b/repository.c
@@ -61,6 +61,8 @@ void repo_set_gitdir(struct repository *repo,
repo_set_commondir(repo, o->commondir);
expand_base_dir(>objects.objectdir, o->object_dir,
repo->commondir, "objects");
+   free(repo->objects.alternate_db);
+   repo->objects.alternate_db = xstrdup_or_null(o->alternate_db);
expand_base_dir(>graft_file, o->graft_file,
repo->commondir, "info/grafts");
expand_base_dir(>index_file, o->index_file,
diff --git a/repository.h b/repository.h
index f917baa584..1b6afd0926 100644
--- a/repository.h
+++ b/repository.h
@@ -94,6 +94,7 @@ struct set_gitdir_args {
const char *object_dir;
const char *graft_file;
const char *index_file;
+   const char *alternate_db;
 };
 
 extern void repo_set_gitdir(struct repository *repo,
diff --git a/sha1_file.c b/sha1_file.c
index dfc8deec38..ad1cd441e6 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -673,15 +673,11 @@ int foreach_alt_odb(alt_odb_fn fn, void *cb)
 
 void prepare_alt_odb(struct repository *r)
 {
-   const char *alt;
-
if (r->objects.alt_odb_tail)
return;
 
-   alt = getenv(ALTERNATE_DB_ENVIRONMENT);
-
r->objects.alt_odb_tail = >objects.alt_odb_list;
-   link_alt_odb_entries(r, alt, PATH_SEP, NULL, 0);
+   link_alt_odb_entries(r, r->objects.alternate_db, PATH_SEP, NULL, 0);
 
read_info_alternates(r, r->objects.objectdir, 0);
 }
-- 
2.16.1.399.g632f88eed1

[PATCH v3 4/4] repository: delete ignore_env member

This variable was added because the repo_set_gitdir() was created to
cover both submodule and main repos, but these two are initialized a
bit differently so ignore_env == 0 means main repo, while ignore_env
!= 0 is submodules.

Since the difference part (env variables) has been moved out of
repo_set_gitdir(), this function works the same way for both repo
types and ignore_env is not needed anymore.

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 repository.c | 4 +---
 repository.h | 9 -
 2 files changed, 1 insertion(+), 12 deletions(-)

diff --git a/repository.c b/repository.c
index b5306ddaa2..4f44384dde 100644
--- a/repository.c
+++ b/repository.c
@@ -12,7 +12,7 @@ static struct repository the_repo = {
NULL, NULL, NULL,
_index,
_algos[GIT_HASH_SHA1],
-   0, 0
+   0
 };
 struct repository *the_repository = _repo;
 
@@ -139,8 +139,6 @@ int repo_init(struct repository *repo, const char *gitdir, 
const char *worktree)
struct repository_format format;
memset(repo, 0, sizeof(*repo));
 
-   repo->ignore_env = 1;
-
INIT_LIST_HEAD(>objects.packed_git_mru);
 
if (repo_init_gitdir(repo, gitdir))
diff --git a/repository.h b/repository.h
index 1b6afd0926..e05a77a099 100644
--- a/repository.h
+++ b/repository.h
@@ -73,15 +73,6 @@ struct repository {
const struct git_hash_algo *hash_algo;
 
/* Configurations */
-   /*
-* Bit used during initialization to indicate if repository state (like
-* the location of the 'objectdir') should be read from the
-* environment.  By default this bit will be set at the begining of
-* 'repo_init()' so that all repositories will ignore the environment.
-* The exception to this is 'the_repository', which doesn't go through
-* the normal 'repo_init()' process.
-*/
-   unsigned ignore_env:1;
 
/* Indicate if a repository has a different 'commondir' from 'gitdir' */
unsigned different_commondir:1;
-- 
2.16.1.399.g632f88eed1

[PATCH v3 0/4] Delete ignore_env member in struct repository

v3 fixes comment style. Also since Brandon raised a question about
shared_root, it's obviously not a good name, so I renamed it to
commondir.

I still keep the delete patch 2/4, but I move the repo_setup_env()
deletion back to 1/4 so all env logic is in one patch (the
introduction of new helper functions in 1/4 and deletion in 2/4 are
still diff noise if 2/4 is completely merged back).

Interdiff:

diff --git a/environment.c b/environment.c
index 47c6e31559..b2128c1188 100644
--- a/environment.c
+++ b/environment.c
@@ -149,7 +149,8 @@ static char *expand_namespace(const char *raw_namespace)
return strbuf_detach(, NULL);
 }
 
-/* Wrapper of getenv() that returns a strdup value. This value is kept
+/*
+ * Wrapper of getenv() that returns a strdup value. This value is kept
  * in argv to be freed later.
  */
 static const char *getenv_safe(struct argv_array *argv, const char *name)
@@ -170,7 +171,7 @@ void setup_git_env(const char *git_dir)
struct set_gitdir_args args = { NULL };
struct argv_array to_free = ARGV_ARRAY_INIT;
 
-   args.shared_root = getenv_safe(_free, GIT_COMMON_DIR_ENVIRONMENT);
+   args.commondir = getenv_safe(_free, GIT_COMMON_DIR_ENVIRONMENT);
args.object_dir = getenv_safe(_free, DB_ENVIRONMENT);
args.graft_file = getenv_safe(_free, GRAFT_ENVIRONMENT);
args.index_file = getenv_safe(_free, INDEX_ENVIRONMENT);
diff --git a/repository.c b/repository.c
index c555dacad2..4f44384dde 100644
--- a/repository.c
+++ b/repository.c
@@ -27,15 +27,15 @@ static void expand_base_dir(char **out, const char *in,
 }
 
 static void repo_set_commondir(struct repository *repo,
-  const char *shared_root)
+  const char *commondir)
 {
struct strbuf sb = STRBUF_INIT;
 
free(repo->commondir);
 
-   if (shared_root) {
+   if (commondir) {
repo->different_commondir = 1;
-   repo->commondir = xstrdup(shared_root);
+   repo->commondir = xstrdup(commondir);
return;
}
 
@@ -58,7 +58,7 @@ void repo_set_gitdir(struct repository *repo,
repo->gitdir = xstrdup(gitfile ? gitfile : root);
free(old_gitdir);
 
-   repo_set_commondir(repo, o->shared_root);
+   repo_set_commondir(repo, o->commondir);
expand_base_dir(>objects.objectdir, o->object_dir,
repo->commondir, "objects");
free(repo->objects.alternate_db);
diff --git a/repository.h b/repository.h
index 07e8971428..e05a77a099 100644
--- a/repository.h
+++ b/repository.h
@@ -81,7 +81,7 @@ struct repository {
 extern struct repository *the_repository;
 
 struct set_gitdir_args {
-   const char *shared_root;
+   const char *commondir;
const char *object_dir;
const char *graft_file;
const char *index_file;

Nguyễn Thái Ngọc Duy (4):
  repository.c: move env-related setup code back to environment.c
  repository.c: delete dead functions
  sha1_file.c: move delayed getenv(altdb) back to setup_git_env()
  repository: delete ignore_env member

 cache.h|  2 +-
 environment.c  | 31 +--
 object-store.h |  5 ++-
 object.c   |  1 +
 repository.c   | 84 --
 repository.h   | 21 +++--
 setup.c|  3 +-
 sha1_file.c|  6 +---
 8 files changed, 87 insertions(+), 66 deletions(-)

-- 
2.16.1.399.g632f88eed1

[PATCH v3 1/4] repository.c: move env-related setup code back to environment.c

It does not make sense that generic repository code contains handling
of environment variables, which are specific for the main repository
only. Refactor repo_set_gitdir() function to take $GIT_DIR and
optionally _all_ other customizable paths. These optional paths can be
NULL and will be calculated according to the default directory layout.

Note that some dead functions are left behind to reduce diff
noise. They will be deleted in the next patch.

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 cache.h   |  2 +-
 environment.c | 30 +++---
 repository.c  | 59 ++-
 repository.h  | 11 +-
 setup.c   |  3 +--
 5 files changed, 79 insertions(+), 26 deletions(-)

diff --git a/cache.h b/cache.h
index 5717399183..b164a407eb 100644
--- a/cache.h
+++ b/cache.h
@@ -459,7 +459,7 @@ static inline enum object_type object_type(unsigned int 
mode)
  */
 extern const char * const local_repo_env[];
 
-extern void setup_git_env(void);
+extern void setup_git_env(const char *git_dir);
 
 /*
  * Returns true iff we have a configured git repository (either via
diff --git a/environment.c b/environment.c
index ec10b062e6..454e435bed 100644
--- a/environment.c
+++ b/environment.c
@@ -14,6 +14,7 @@
 #include "fmt-merge-msg.h"
 #include "commit.h"
 #include "object-store.h"
+#include "argv-array.h"
 
 int trust_executable_bit = 1;
 int trust_ctime = 1;
@@ -148,10 +149,34 @@ static char *expand_namespace(const char *raw_namespace)
return strbuf_detach(, NULL);
 }
 
-void setup_git_env(void)
+/*
+ * Wrapper of getenv() that returns a strdup value. This value is kept
+ * in argv to be freed later.
+ */
+static const char *getenv_safe(struct argv_array *argv, const char *name)
+{
+   const char *value = getenv(name);
+
+   if (!value)
+   return NULL;
+
+   argv_array_push(argv, value);
+   return argv->argv[argv->argc - 1];
+}
+
+void setup_git_env(const char *git_dir)
 {
const char *shallow_file;
const char *replace_ref_base;
+   struct set_gitdir_args args = { NULL };
+   struct argv_array to_free = ARGV_ARRAY_INIT;
+
+   args.commondir = getenv_safe(_free, GIT_COMMON_DIR_ENVIRONMENT);
+   args.object_dir = getenv_safe(_free, DB_ENVIRONMENT);
+   args.graft_file = getenv_safe(_free, GRAFT_ENVIRONMENT);
+   args.index_file = getenv_safe(_free, INDEX_ENVIRONMENT);
+   repo_set_gitdir(the_repository, git_dir, );
+   argv_array_clear(_free);
 
if (getenv(NO_REPLACE_OBJECTS_ENVIRONMENT))
check_replace_refs = 0;
@@ -301,8 +326,7 @@ int set_git_dir(const char *path)
 {
if (setenv(GIT_DIR_ENVIRONMENT, path, 1))
return error("Could not set GIT_DIR to '%s'", path);
-   repo_set_gitdir(the_repository, path);
-   setup_git_env();
+   setup_git_env(path);
return 0;
 }
 
diff --git a/repository.c b/repository.c
index a069b1b640..ae117efbf0 100644
--- a/repository.c
+++ b/repository.c
@@ -41,35 +41,55 @@ static int find_common_dir(struct strbuf *sb, const char 
*gitdir, int fromenv)
return get_common_dir_noenv(sb, gitdir);
 }
 
-static void repo_setup_env(struct repository *repo)
+static void expand_base_dir(char **out, const char *in,
+   const char *base_dir, const char *def_in)
+{
+   free(*out);
+   if (in)
+   *out = xstrdup(in);
+   else
+   *out = xstrfmt("%s/%s", base_dir, def_in);
+}
+
+static void repo_set_commondir(struct repository *repo,
+  const char *commondir)
 {
struct strbuf sb = STRBUF_INIT;
 
-   repo->different_commondir = find_common_dir(, repo->gitdir,
-   !repo->ignore_env);
free(repo->commondir);
+
+   if (commondir) {
+   repo->different_commondir = 1;
+   repo->commondir = xstrdup(commondir);
+   return;
+   }
+
+   repo->different_commondir = get_common_dir_noenv(, repo->gitdir);
repo->commondir = strbuf_detach(, NULL);
-   raw_object_store_clear(>objects);
-   repo->objects.objectdir =
-   git_path_from_env(DB_ENVIRONMENT, repo->commondir,
- "objects", !repo->ignore_env);
-   free(repo->graft_file);
-   repo->graft_file = git_path_from_env(GRAFT_ENVIRONMENT, repo->commondir,
-"info/grafts", !repo->ignore_env);
-   free(repo->index_file);
-   repo->index_file = git_path_from_env(INDEX_ENVIRONMENT, repo->gitdir,
-"index", !repo->ignore_env);
 }
 
-void repo_set_gitdir(struct repository *repo, const char *path)
+void repo_set_gitdir(struct repository *repo,
+const char *root,
+const struct set_gitdir_args *o)
 {
-   const char *gitfile = read_gitfile(path);
+

[PATCH v3 2/4] repository.c: delete dead functions

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 repository.c | 25 -
 1 file changed, 25 deletions(-)

diff --git a/repository.c b/repository.c
index ae117efbf0..89e76173a3 100644
--- a/repository.c
+++ b/repository.c
@@ -16,31 +16,6 @@ static struct repository the_repo = {
 };
 struct repository *the_repository = _repo;
 
-static char *git_path_from_env(const char *envvar, const char *git_dir,
-  const char *path, int fromenv)
-{
-   if (fromenv) {
-   const char *value = getenv(envvar);
-   if (value)
-   return xstrdup(value);
-   }
-
-   return xstrfmt("%s/%s", git_dir, path);
-}
-
-static int find_common_dir(struct strbuf *sb, const char *gitdir, int fromenv)
-{
-   if (fromenv) {
-   const char *value = getenv(GIT_COMMON_DIR_ENVIRONMENT);
-   if (value) {
-   strbuf_addstr(sb, value);
-   return 1;
-   }
-   }
-
-   return get_common_dir_noenv(sb, gitdir);
-}
-
 static void expand_base_dir(char **out, const char *in,
const char *base_dir, const char *def_in)
 {
-- 
2.16.1.399.g632f88eed1

Re: [RFC] Rebasing merges: a jorney to the ultimate solution (Road Clear)

On 28/02/2018 01:36, Jacob Keller wrote:
> 
> > > (3) ---X1---o---o---o---o---o---X2
> > >|\   |\
> > >| A1---A2---A3---U1  | A1'--A2'--A3'--U1'
> > >| \  |
> > >|  M |
> > >| /  |
> > >\-B1---B2---B3---U2  \-B1'--B2'--B3'--U2'
> > >
> >
> > Meh, I hope I`m rushing it now, but for example, if we had decided to
> > drop commit A2 during an interactive rebase (so losing A2' from
> > diagram above), wouldn`t U2' still introduce those changes back, once
> > U1' and U2' are merged, being incorrect/unwanted behavior...? :/
> 
> In that case, the method won't work well at all, so I think we need a
> different approach.
> 

Hmm, still rushing it, but what about adding an additional step, 
something like this: 
 
 (4) ---X1---o---o---o---o---o---X2
|\   |\
| A1---A2---A3---U1  | A1'--A2'--A3'--U1'--R1
| \  |
|  M |
| /  |
\-B1---B2---B3---U2  \-B1'--B2'--B3'--U2'--R2


... where:

  R1 = git diff U2 U2' | git apply --3way
  R2 = git diff U1 U1' | git apply --3way

(this is just to explain the point, might be there is a better way to 
produce Rx)

So, we still use Ux' to preserve merge commit M amendments, but also 
Rx to catch any changes happening between Ux and Ux' caused by 
interactive rebase commit manipulation (add/amend/drop).

Note that R*1* is produced by applying diff from U*2*' side, and vice 
versa (as it`s the other side that can erroneously introduce dropped 
commit changes back, like U2' in case of dropped A2').

>From here we continue as before - merging R1 and R2, then rewriting 
merge commit parents to point to A3' and B3' (dropping Ux` and Rx).

This seems to be working inside my (too trivial?) test case, for 
interactive adding, dropping, and amending of rebased commits, 
resulting "rebased" merge containing all the added/modified/dropped 
changes, plus the original merge amendment, all as expected :P

Regards, Buga

Re: [PATCH] protocol: treat unrecognized protocol.version setting as 0

Duy Nguyen wrote:
> On Wed, Feb 28, 2018 at 8:02 AM, Brandon Williams  wrote:
>> On 02/27, Jonathan Nieder wrote:

>>> If I share my .gitconfig or .git/config file between multiple machines
>>> (or between multiple Git versions on a single machine) and set
>>>
>>>   [protocol]
>>>   version = 2
>>>
>>> then running "git fetch" with a Git version that does not support
>>> protocol v2 errors out with
>>>
>>>   fatal: unknown value for config 'protocol.version': 2
>>>
>>> In the spirit of v1.7.6-rc0~77^2~1 (Improve error handling when
>>> parsing dirstat parameters, 2011-04-29), it is better to (perhaps
>>> after warning the user) ignore the unrecognized protocol version.
>>> After all, future Git versions might add even more protocol versions,
>>> and using two different Git versions with the same Git repo, machine,
>>> or home directory should not cripple the older Git version just
>>> because of a parameter that is only understood by a more recent Git
>>> version.
>
> I wonder if it's better to specify multiple versions. If v2 is not
> recognized by this git but v0 is, then it can pick that up. But if you
> explicitly tell it to choose between v2 and v3 only and it does not
> understand either, then it dies. Not sure if this is a good idea
> though.

Interesting thought.  Something roughly like this (on top of the patch
this is a reply to)?

diff --git i/protocol.c w/protocol.c
index ce9c634a3a..6aa8857a11 100644
--- i/protocol.c
+++ w/protocol.c
@@ -1,4 +1,5 @@
 #include "cache.h"
+#include "string-list.h"
 #include "config.h"
 #include "protocol.h"
 
@@ -14,14 +15,18 @@ static enum protocol_version parse_protocol_version(const 
char *value)
 
 enum protocol_version get_protocol_version_config(void)
 {
-   const char *value;
-   if (!git_config_get_string_const("protocol.version", )) {
-   enum protocol_version version = parse_protocol_version(value);
-   if (version != protocol_unknown_version)
-   return version;
+   const struct string_list *values;
+   const struct string_list_item *value;
+   enum protocol_version result = protocol_v0;
+
+   values = git_config_get_value_multi("protocol.version");
+   for_each_string_list_item(value, values) {
+   enum protocol_version v = parse_protocol_version(value->string);
+   if (v != protocol_unknown_version)
+   result = v;
}
 
-   return protocol_v0;
+   return result;
 }
 
 enum protocol_version determine_protocol_version_server(void)

Re: [PATCH] protocol: treat unrecognized protocol.version setting as 0

2018-02-27 Thread Brandon Williams

On 02/28, Duy Nguyen wrote:
> On Wed, Feb 28, 2018 at 8:02 AM, Brandon Williams  wrote:
> > On 02/27, Jonathan Nieder wrote:
> >> If I share my .gitconfig or .git/config file between multiple machines
> >> (or between multiple Git versions on a single machine) and set
> >>
> >>   [protocol]
> >>   version = 2
> >>
> >> then running "git fetch" with a Git version that does not support
> >> protocol v2 errors out with
> >>
> >>   fatal: unknown value for config 'protocol.version': 2
> >>
> >> In the spirit of v1.7.6-rc0~77^2~1 (Improve error handling when
> >> parsing dirstat parameters, 2011-04-29), it is better to (perhaps
> >> after warning the user) ignore the unrecognized protocol version.
> >> After all, future Git versions might add even more protocol versions,
> >> and using two different Git versions with the same Git repo, machine,
> >> or home directory should not cripple the older Git version just
> >> because of a parameter that is only understood by a more recent Git
> >> version.
> 
> I wonder if it's better to specify multiple versions. If v2 is not
> recognized by this git but v0 is, then it can pick that up. But if you
> explicitly tell it to choose between v2 and v3 only and it does not
> understand either, then it dies. Not sure if this is a good idea
> though.

I mean that's definitely a possibility, but I don't think its worth the
effort to get that working until we actually need it.  I'm hoping we
really don't bump version numbers often.

-- 
Brandon Williams

Re: [PATCH] protocol: treat unrecognized protocol.version setting as 0

2018-02-27 Thread Duy Nguyen

On Wed, Feb 28, 2018 at 8:02 AM, Brandon Williams  wrote:
> On 02/27, Jonathan Nieder wrote:
>> If I share my .gitconfig or .git/config file between multiple machines
>> (or between multiple Git versions on a single machine) and set
>>
>>   [protocol]
>>   version = 2
>>
>> then running "git fetch" with a Git version that does not support
>> protocol v2 errors out with
>>
>>   fatal: unknown value for config 'protocol.version': 2
>>
>> In the spirit of v1.7.6-rc0~77^2~1 (Improve error handling when
>> parsing dirstat parameters, 2011-04-29), it is better to (perhaps
>> after warning the user) ignore the unrecognized protocol version.
>> After all, future Git versions might add even more protocol versions,
>> and using two different Git versions with the same Git repo, machine,
>> or home directory should not cripple the older Git version just
>> because of a parameter that is only understood by a more recent Git
>> version.

I wonder if it's better to specify multiple versions. If v2 is not
recognized by this git but v0 is, then it can pick that up. But if you
explicitly tell it to choose between v2 and v3 only and it does not
understand either, then it dies. Not sure if this is a good idea
though.
-- 
Duy

Re: [PATCH v3 29/35] pkt-line: add packet_buf_write_len function

2018-02-27 Thread Brandon Williams

On 02/27, Jonathan Nieder wrote:
> Brandon Williams wrote:
> 
> > Add the 'packet_buf_write_len()' function which allows for writing an
> > arbitrary length buffer into a 'struct strbuf' and formatting it in
> > packet-line format.
> 
> Makes sense.
> 
> [...]
> > --- a/pkt-line.h
> > +++ b/pkt-line.h
> > @@ -26,6 +26,7 @@ void packet_buf_flush(struct strbuf *buf);
> >  void packet_buf_delim(struct strbuf *buf);
> >  void packet_write(int fd_out, const char *buf, size_t size);
> >  void packet_buf_write(struct strbuf *buf, const char *fmt, ...) 
> > __attribute__((format (printf, 2, 3)));
> > +void packet_buf_write_len(struct strbuf *buf, const char *data, size_t 
> > len);
> 
> I wonder if we should rename packet_buf_write to something like
> packet_buf_writef.  Right now there's a kind of confusing collection of
> functions without much symmetry.
> 
> Alternatively, the _buf_ ones could become strbuf_* functions:
> 
>   strbuf_add_packet(, data, len);
>   strbuf_addf_packet(, fmt, ...);
> 
> That would make it clearer that these append to buf.
> 
> I'm just thinking out loud.  For this series, the API you have here
> looks fine, even if it is a bit inconsistent.  (In other words, even
> if you agree with me, this would probably be best addressed as a patch
> on top.)

Yeah I agree that an api change is needed, but yeah it can be done on
top of this series.

> 
> [...]
> > --- a/pkt-line.c
> > +++ b/pkt-line.c
> > @@ -215,6 +215,22 @@ void packet_buf_write(struct strbuf *buf, const char 
> > *fmt, ...)
> > va_end(args);
> >  }
> >  
> > +void packet_buf_write_len(struct strbuf *buf, const char *data, size_t len)
> > +{
> > +   size_t orig_len, n;
> > +
> > +   orig_len = buf->len;
> > +   strbuf_addstr(buf, "");
> > +   strbuf_add(buf, data, len);
> > +   n = buf->len - orig_len;
> > +
> > +   if (n > LARGE_PACKET_MAX)
> > +   die("protocol error: impossibly long line");
> 
> Could the error message describe the long line (e.g.
> 
>   ...impossibly long line %.*s...", 256, data);
> 

I was reusing the error msg as it appears in another part of this file.

> )?
> 
> > +
> > +   set_packet_header(>buf[orig_len], n);
> > +   packet_trace(buf->buf + orig_len + 4, n - 4, 1);
> 
> Could do, more simply:
> 
>   packet_trace(data, len, 1);

I'll change this.

> 
> Thanks,
> Jonathan

-- 
Brandon Williams

[PATCH 11/11] packfile: allow find_pack_entry to handle arbitrary repositories

Signed-off-by: Stefan Beller 
---
 packfile.c | 10 +-
 packfile.h |  3 +--
 2 files changed, 6 insertions(+), 7 deletions(-)

diff --git a/packfile.c b/packfile.c
index e16f8252233..399c59e640f 100644
--- a/packfile.c
+++ b/packfile.c
@@ -1835,18 +1835,18 @@ static int fill_pack_entry(const unsigned char *sha1,
return 1;
 }
 
-int find_pack_entry_the_repository(const unsigned char *sha1, struct 
pack_entry *e)
+int find_pack_entry(struct repository *r, const unsigned char *sha1, struct 
pack_entry *e)
 {
struct list_head *pos;
 
-   prepare_packed_git(the_repository);
-   if (!the_repository->objects.packed_git)
+   prepare_packed_git(r);
+   if (!r->objects.packed_git)
return 0;
 
-   list_for_each(pos, _repository->objects.packed_git_mru) {
+   list_for_each(pos, >objects.packed_git_mru) {
struct packed_git *p = list_entry(pos, struct packed_git, mru);
if (fill_pack_entry(sha1, e, p)) {
-   list_move(>mru, 
_repository->objects.packed_git_mru);
+   list_move(>mru, >objects.packed_git_mru);
return 1;
}
}
diff --git a/packfile.h b/packfile.h
index feeabd6f031..6f7b9e91d66 100644
--- a/packfile.h
+++ b/packfile.h
@@ -124,8 +124,7 @@ extern const struct packed_git *has_packed_and_bad(const 
unsigned char *sha1);
  * Iff a pack file in the given repository contains the object named by sha1,
  * return true and store its location to e.
  */
-#define find_pack_entry(r, s, e) find_pack_entry_##r(s, e)
-extern int find_pack_entry_the_repository(const unsigned char *sha1, struct 
pack_entry *e);
+extern int find_pack_entry(struct repository *r, const unsigned char *sha1, 
struct pack_entry *e);
 
 extern int has_sha1_pack(const unsigned char *sha1);
 
-- 
2.16.2.395.g2e18187dfd-goog

[PATCH 09/11] packfile: allow reprepare_packed_git to handle arbitrary repositories

Signed-off-by: Stefan Beller 
---
 packfile.c | 8 
 packfile.h | 3 +--
 2 files changed, 5 insertions(+), 6 deletions(-)

diff --git a/packfile.c b/packfile.c
index 9a3efc01555..b12fc0789e0 100644
--- a/packfile.c
+++ b/packfile.c
@@ -898,11 +898,11 @@ void prepare_packed_git(struct repository *r)
r->objects.packed_git_initialized = 1;
 }
 
-void reprepare_packed_git_the_repository(void)
+void reprepare_packed_git(struct repository *r)
 {
-   the_repository->objects.approximate_object_count_valid = 0;
-   the_repository->objects.packed_git_initialized = 0;
-   prepare_packed_git(the_repository);
+   r->objects.approximate_object_count_valid = 0;
+   r->objects.packed_git_initialized = 0;
+   prepare_packed_git(r);
 }
 
 unsigned long unpack_object_header_buffer(const unsigned char *buf,
diff --git a/packfile.h b/packfile.h
index 9142866c8ae..99f4741bd95 100644
--- a/packfile.h
+++ b/packfile.h
@@ -35,8 +35,7 @@ extern struct packed_git *parse_pack_index(unsigned char 
*sha1, const char *idx_
 extern void (*report_garbage)(unsigned seen_bits, const char *path);
 
 extern void prepare_packed_git(struct repository *r);
-#define reprepare_packed_git(r) reprepare_packed_git_##r()
-extern void reprepare_packed_git_the_repository(void);
+extern void reprepare_packed_git(struct repository *r);
 extern void install_packed_git(struct repository *r, struct packed_git *pack);
 
 /*
-- 
2.16.2.395.g2e18187dfd-goog

[PATCH 10/11] packfile: add repository argument to find_pack_entry

While at it move the documentation to the header and mention which pack
files are searched.

Signed-off-by: Stefan Beller 
---
 packfile.c  | 8 ++--
 packfile.h  | 7 ++-
 sha1_file.c | 6 +++---
 3 files changed, 11 insertions(+), 10 deletions(-)

diff --git a/packfile.c b/packfile.c
index b12fc0789e0..e16f8252233 100644
--- a/packfile.c
+++ b/packfile.c
@@ -1835,11 +1835,7 @@ static int fill_pack_entry(const unsigned char *sha1,
return 1;
 }
 
-/*
- * Iff a pack file contains the object named by sha1, return true and
- * store its location to e.
- */
-int find_pack_entry(const unsigned char *sha1, struct pack_entry *e)
+int find_pack_entry_the_repository(const unsigned char *sha1, struct 
pack_entry *e)
 {
struct list_head *pos;
 
@@ -1860,7 +1856,7 @@ int find_pack_entry(const unsigned char *sha1, struct 
pack_entry *e)
 int has_sha1_pack(const unsigned char *sha1)
 {
struct pack_entry e;
-   return find_pack_entry(sha1, );
+   return find_pack_entry(the_repository, sha1, );
 }
 
 int has_pack_index(const unsigned char *sha1)
diff --git a/packfile.h b/packfile.h
index 99f4741bd95..feeabd6f031 100644
--- a/packfile.h
+++ b/packfile.h
@@ -120,7 +120,12 @@ extern int packed_object_info(struct packed_git *pack, 
off_t offset, struct obje
 extern void mark_bad_packed_object(struct packed_git *p, const unsigned char 
*sha1);
 extern const struct packed_git *has_packed_and_bad(const unsigned char *sha1);
 
-extern int find_pack_entry(const unsigned char *sha1, struct pack_entry *e);
+/*
+ * Iff a pack file in the given repository contains the object named by sha1,
+ * return true and store its location to e.
+ */
+#define find_pack_entry(r, s, e) find_pack_entry_##r(s, e)
+extern int find_pack_entry_the_repository(const unsigned char *sha1, struct 
pack_entry *e);
 
 extern int has_sha1_pack(const unsigned char *sha1);
 
diff --git a/sha1_file.c b/sha1_file.c
index 0b9fefaaf02..d498b3e90b9 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -1273,7 +1273,7 @@ int sha1_object_info_extended(const unsigned char *sha1, 
struct object_info *oi,
}
 
while (1) {
-   if (find_pack_entry(real, ))
+   if (find_pack_entry(the_repository, real, ))
break;
 
/* Most likely it's a loose object. */
@@ -1282,7 +1282,7 @@ int sha1_object_info_extended(const unsigned char *sha1, 
struct object_info *oi,
 
/* Not a loose object; someone else may have just packed it. */
reprepare_packed_git(the_repository);
-   if (find_pack_entry(real, ))
+   if (find_pack_entry(the_repository, real, ))
break;
 
/* Check if it is a missing object */
@@ -1662,7 +1662,7 @@ static int freshen_loose_object(const unsigned char *sha1)
 static int freshen_packed_object(const unsigned char *sha1)
 {
struct pack_entry e;
-   if (!find_pack_entry(sha1, ))
+   if (!find_pack_entry(the_repository, sha1, ))
return 0;
if (e.p->freshened)
return 1;
-- 
2.16.2.395.g2e18187dfd-goog

[PATCH 04/11] packfile: add repository argument to prepare_packed_git_one

Signed-off-by: Stefan Beller 
---
 packfile.c | 7 ---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/packfile.c b/packfile.c
index 1c24b02cc84..8bb158fc84e 100644
--- a/packfile.c
+++ b/packfile.c
@@ -735,7 +735,8 @@ static void report_pack_garbage(struct string_list *list)
report_helper(list, seen_bits, first, list->nr);
 }
 
-static void prepare_packed_git_one(char *objdir, int local)
+#define prepare_packed_git_one(r, o, l) prepare_packed_git_one_##r(o, l)
+static void prepare_packed_git_one_the_repository(char *objdir, int local)
 {
struct strbuf path = STRBUF_INIT;
size_t dirnamelen;
@@ -889,10 +890,10 @@ void prepare_packed_git(void)
 
if (the_repository->objects.packed_git_initialized)
return;
-   prepare_packed_git_one(get_object_directory(), 1);
+   prepare_packed_git_one(the_repository, get_object_directory(), 1);
prepare_alt_odb(the_repository);
for (alt = the_repository->objects.alt_odb_list; alt; alt = alt->next)
-   prepare_packed_git_one(alt->path, 0);
+   prepare_packed_git_one(the_repository, alt->path, 0);
rearrange_packed_git(the_repository);
prepare_packed_git_mru(the_repository);
the_repository->objects.packed_git_initialized = 1;
-- 
2.16.2.395.g2e18187dfd-goog

[PATCH 06/11] packfile: add repository argument to reprepare_packed_git

See previous patch for explanation.

Signed-off-by: Stefan Beller 
---
 builtin/gc.c   | 2 +-
 builtin/receive-pack.c | 3 ++-
 bulk-checkin.c | 3 ++-
 fetch-pack.c   | 3 ++-
 packfile.c | 2 +-
 packfile.h | 3 ++-
 sha1_file.c| 2 +-
 7 files changed, 11 insertions(+), 7 deletions(-)

diff --git a/builtin/gc.c b/builtin/gc.c
index 97f34ae9fe0..c16020ef42a 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -478,7 +478,7 @@ int cmd_gc(int argc, const char **argv, const char *prefix)
return error(FAILED_RUN, rerere.argv[0]);
 
report_garbage = report_pack_garbage;
-   reprepare_packed_git();
+   reprepare_packed_git(the_repository);
if (pack_garbage.nr > 0)
clean_pack_garbage();
 
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 954fc72c7cb..8b03a6e03dc 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -1,4 +1,5 @@
 #include "builtin.h"
+#include "repository.h"
 #include "config.h"
 #include "lockfile.h"
 #include "pack.h"
@@ -1778,7 +1779,7 @@ static const char *unpack(int err_fd, struct shallow_info 
*si)
status = finish_command();
if (status)
return "index-pack abnormal exit";
-   reprepare_packed_git();
+   reprepare_packed_git(the_repository);
}
return NULL;
 }
diff --git a/bulk-checkin.c b/bulk-checkin.c
index 3310fd210a1..eadc2d51720 100644
--- a/bulk-checkin.c
+++ b/bulk-checkin.c
@@ -3,6 +3,7 @@
  */
 #include "cache.h"
 #include "bulk-checkin.h"
+#include "repository.h"
 #include "csum-file.h"
 #include "pack.h"
 #include "strbuf.h"
@@ -57,7 +58,7 @@ static void finish_bulk_checkin(struct bulk_checkin_state 
*state)
 
strbuf_release();
/* Make objects we just wrote available to ourselves */
-   reprepare_packed_git();
+   reprepare_packed_git(the_repository);
 }
 
 static int already_written(struct bulk_checkin_state *state, unsigned char 
sha1[])
diff --git a/fetch-pack.c b/fetch-pack.c
index 8253d746e0c..eac5928a27b 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -1,4 +1,5 @@
 #include "cache.h"
+#include "repository.h"
 #include "config.h"
 #include "lockfile.h"
 #include "refs.h"
@@ -1192,7 +1193,7 @@ struct ref *fetch_pack(struct fetch_pack_args *args,
prepare_shallow_info(, shallow);
ref_cpy = do_fetch_pack(args, fd, ref, sought, nr_sought,
, pack_lockfile);
-   reprepare_packed_git();
+   reprepare_packed_git(the_repository);
update_shallow(args, sought, nr_sought, );
clear_shallow_info();
return ref_cpy;
diff --git a/packfile.c b/packfile.c
index 6e8b98ab405..6ea50230aab 100644
--- a/packfile.c
+++ b/packfile.c
@@ -899,7 +899,7 @@ void prepare_packed_git_the_repository(void)
the_repository->objects.packed_git_initialized = 1;
 }
 
-void reprepare_packed_git(void)
+void reprepare_packed_git_the_repository(void)
 {
the_repository->objects.approximate_object_count_valid = 0;
the_repository->objects.packed_git_initialized = 0;
diff --git a/packfile.h b/packfile.h
index 39eb590b2ae..9afbf73657e 100644
--- a/packfile.h
+++ b/packfile.h
@@ -36,7 +36,8 @@ extern void (*report_garbage)(unsigned seen_bits, const char 
*path);
 
 #define prepare_packed_git(r) prepare_packed_git_##r()
 extern void prepare_packed_git_the_repository(void);
-extern void reprepare_packed_git(void);
+#define reprepare_packed_git(r) reprepare_packed_git_##r()
+extern void reprepare_packed_git_the_repository(void);
 extern void install_packed_git(struct repository *r, struct packed_git *pack);
 
 /*
diff --git a/sha1_file.c b/sha1_file.c
index 36282acb1a8..0b9fefaaf02 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -1281,7 +1281,7 @@ int sha1_object_info_extended(const unsigned char *sha1, 
struct object_info *oi,
return 0;
 
/* Not a loose object; someone else may have just packed it. */
-   reprepare_packed_git();
+   reprepare_packed_git(the_repository);
if (find_pack_entry(real, ))
break;
 
-- 
2.16.2.395.g2e18187dfd-goog

[PATCH 01/11] packfile: allow prepare_packed_git_mru to handle arbitrary repositories

This conversion was done without the #define trick used in the earlier
series refactoring to have better repository access, because this function
is easy to review, as all lines are converted and it has only one caller

Signed-off-by: Stefan Beller 
---
 packfile.c | 10 +-
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/packfile.c b/packfile.c
index ccfe084642b..b844b653265 100644
--- a/packfile.c
+++ b/packfile.c
@@ -873,14 +873,14 @@ static void rearrange_packed_git(void)
set_next_packed_git, sort_pack);
 }
 
-static void prepare_packed_git_mru(void)
+static void prepare_packed_git_mru(struct repository *r)
 {
struct packed_git *p;
 
-   INIT_LIST_HEAD(_repository->objects.packed_git_mru);
+   INIT_LIST_HEAD(>objects.packed_git_mru);
 
-   for (p = the_repository->objects.packed_git; p; p = p->next)
-   list_add_tail(>mru, _repository->objects.packed_git_mru);
+   for (p = r->objects.packed_git; p; p = p->next)
+   list_add_tail(>mru, >objects.packed_git_mru);
 }
 
 void prepare_packed_git(void)
@@ -894,7 +894,7 @@ void prepare_packed_git(void)
for (alt = the_repository->objects.alt_odb_list; alt; alt = alt->next)
prepare_packed_git_one(alt->path, 0);
rearrange_packed_git();
-   prepare_packed_git_mru();
+   prepare_packed_git_mru(the_repository);
the_repository->objects.packed_git_initialized = 1;
 }
 
-- 
2.16.2.395.g2e18187dfd-goog

[PATCH 03/11] packfile: allow install_packed_git to handle arbitrary repositories

This conversion was done without the #define trick used in the earlier
series refactoring to have better repository access, because this function
is easy to review, as it only has one caller and all lines but the first
two are converted.

We must not convert 'pack_open_fds' to be a repository specific variable,
as it is used to monitor resource usage of the machine that Git executes
on.

Signed-off-by: Stefan Beller 
---
 fast-import.c | 2 +-
 http.c| 2 +-
 packfile.c| 8 
 packfile.h| 2 +-
 4 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/fast-import.c b/fast-import.c
index 1685fe59a20..67550584da4 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -1038,7 +1038,7 @@ static void end_packfile(void)
if (!new_p)
die("core git rejected index %s", idx_name);
all_packs[pack_id] = new_p;
-   install_packed_git(new_p);
+   install_packed_git(the_repository, new_p);
free(idx_name);
 
/* Print the boundary */
diff --git a/http.c b/http.c
index afe2707e86b..f3c2302f84b 100644
--- a/http.c
+++ b/http.c
@@ -2134,7 +2134,7 @@ int finish_http_pack_request(struct http_pack_request 
*preq)
return -1;
}
 
-   install_packed_git(p);
+   install_packed_git(the_repository, p);
free(tmp_idx);
return 0;
 }
diff --git a/packfile.c b/packfile.c
index 5ccce419354..1c24b02cc84 100644
--- a/packfile.c
+++ b/packfile.c
@@ -680,13 +680,13 @@ struct packed_git *add_packed_git(const char *path, 
size_t path_len, int local)
return p;
 }
 
-void install_packed_git(struct packed_git *pack)
+void install_packed_git(struct repository *r, struct packed_git *pack)
 {
if (pack->pack_fd != -1)
pack_open_fds++;
 
-   pack->next = the_repository->objects.packed_git;
-   the_repository->objects.packed_git = pack;
+   pack->next = r->objects.packed_git;
+   r->objects.packed_git = pack;
 }
 
 void (*report_garbage)(unsigned seen_bits, const char *path);
@@ -782,7 +782,7 @@ static void prepare_packed_git_one(char *objdir, int local)
 * corresponding .pack file that we can map.
 */
(p = add_packed_git(path.buf, path.len, local)) != 
NULL)
-   install_packed_git(p);
+   install_packed_git(the_repository, p);
}
 
if (!report_garbage)
diff --git a/packfile.h b/packfile.h
index 6a2c57045ca..cc3bf67ab50 100644
--- a/packfile.h
+++ b/packfile.h
@@ -36,7 +36,7 @@ extern void (*report_garbage)(unsigned seen_bits, const char 
*path);
 
 extern void prepare_packed_git(void);
 extern void reprepare_packed_git(void);
-extern void install_packed_git(struct packed_git *pack);
+extern void install_packed_git(struct repository *r, struct packed_git *pack);
 
 /*
  * Give a rough count of objects in the repository. This sacrifices accuracy
-- 
2.16.2.395.g2e18187dfd-goog

[PATCH 05/11] packfile: add repository argument to prepare_packed_git

Add a repository argument to allow prepare_packed_git callers to
be more specific about which repository to handle. See c28d027a52c
(sha1_file: add repository argument to link_alt_odb_entry, 2018-02-20)
for an explanation of the #define trick.

Signed-off-by: Stefan Beller 
---
 builtin/count-objects.c  |  2 +-
 builtin/fsck.c   |  2 +-
 builtin/gc.c |  2 +-
 builtin/pack-objects.c   |  2 +-
 builtin/pack-redundant.c |  2 +-
 fast-import.c|  2 +-
 http-backend.c   |  2 +-
 pack-bitmap.c|  2 +-
 packfile.c   | 10 +-
 packfile.h   |  3 ++-
 server-info.c|  2 +-
 sha1_name.c  |  4 ++--
 12 files changed, 18 insertions(+), 17 deletions(-)

diff --git a/builtin/count-objects.c b/builtin/count-objects.c
index 9334648dccf..b262327bf6d 100644
--- a/builtin/count-objects.c
+++ b/builtin/count-objects.c
@@ -123,7 +123,7 @@ int cmd_count_objects(int argc, const char **argv, const 
char *prefix)
struct strbuf pack_buf = STRBUF_INIT;
struct strbuf garbage_buf = STRBUF_INIT;
if (!the_repository->objects.packed_git)
-   prepare_packed_git();
+   prepare_packed_git(the_repository);
for (p = the_repository->objects.packed_git; p; p = p->next) {
if (!p->pack_local)
continue;
diff --git a/builtin/fsck.c b/builtin/fsck.c
index 76c94e9b5af..27f580352c5 100644
--- a/builtin/fsck.c
+++ b/builtin/fsck.c
@@ -727,7 +727,7 @@ int cmd_fsck(int argc, const char **argv, const char 
*prefix)
uint32_t total = 0, count = 0;
struct progress *progress = NULL;
 
-   prepare_packed_git();
+   prepare_packed_git(the_repository);
 
if (show_progress) {
for (p = the_repository->objects.packed_git; p;
diff --git a/builtin/gc.c b/builtin/gc.c
index 96ff7908677..97f34ae9fe0 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -174,7 +174,7 @@ static int too_many_packs(void)
if (gc_auto_pack_limit <= 0)
return 0;
 
-   prepare_packed_git();
+   prepare_packed_git(the_repository);
for (cnt = 0, p = the_repository->objects.packed_git; p; p = p->next) {
if (!p->pack_local)
continue;
diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index 7f376c2aefb..84e940a53ba 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -3151,7 +3151,7 @@ int cmd_pack_objects(int argc, const char **argv, const 
char *prefix)
if (progress && all_progress_implied)
progress = 2;
 
-   prepare_packed_git();
+   prepare_packed_git(the_repository);
if (ignore_packed_keep) {
struct packed_git *p;
for (p = the_repository->objects.packed_git; p; p = p->next)
diff --git a/builtin/pack-redundant.c b/builtin/pack-redundant.c
index 55462bc826a..83d2395dae9 100644
--- a/builtin/pack-redundant.c
+++ b/builtin/pack-redundant.c
@@ -631,7 +631,7 @@ int cmd_pack_redundant(int argc, const char **argv, const 
char *prefix)
break;
}
 
-   prepare_packed_git();
+   prepare_packed_git(the_repository);
 
if (load_all_packs)
load_all();
diff --git a/fast-import.c b/fast-import.c
index 67550584da4..39cd0b7540d 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -3471,7 +3471,7 @@ int cmd_main(int argc, const char **argv)
rc_free[i].next = _free[i + 1];
rc_free[cmd_save - 1].next = NULL;
 
-   prepare_packed_git();
+   prepare_packed_git(the_repository);
start_packfile();
set_die_routine(die_nicely);
set_checkpoint_signal();
diff --git a/http-backend.c b/http-backend.c
index 4d93126c15f..4950078c936 100644
--- a/http-backend.c
+++ b/http-backend.c
@@ -519,7 +519,7 @@ static void get_info_packs(struct strbuf *hdr, char *arg)
size_t cnt = 0;
 
select_getanyfile(hdr);
-   prepare_packed_git();
+   prepare_packed_git(the_repository);
for (p = the_repository->objects.packed_git; p; p = p->next) {
if (p->pack_local)
cnt++;
diff --git a/pack-bitmap.c b/pack-bitmap.c
index d51172b1d5b..273bdfb631f 100644
--- a/pack-bitmap.c
+++ b/pack-bitmap.c
@@ -336,7 +336,7 @@ static int open_pack_bitmap(void)
 
assert(!bitmap_git.map && !bitmap_git.loaded);
 
-   prepare_packed_git();
+   prepare_packed_git(the_repository);
for (p = the_repository->objects.packed_git; p; p = p->next) {
if (open_pack_bitmap_1(p) == 0)
ret = 0;
diff --git a/packfile.c b/packfile.c
index 8bb158fc84e..6e8b98ab405 100644
--- a/packfile.c
+++ b/packfile.c
@@ -817,7 +817,7 @@ unsigned long

[PATCH 08/11] packfile: allow prepare_packed_git to handle arbitrary repositories

Signed-off-by: Stefan Beller 
---
 packfile.c | 18 +-
 packfile.h |  3 +--
 2 files changed, 10 insertions(+), 11 deletions(-)

diff --git a/packfile.c b/packfile.c
index c45516acd41..9a3efc01555 100644
--- a/packfile.c
+++ b/packfile.c
@@ -883,19 +883,19 @@ static void prepare_packed_git_mru(struct repository *r)
list_add_tail(>mru, >objects.packed_git_mru);
 }
 
-void prepare_packed_git_the_repository(void)
+void prepare_packed_git(struct repository *r)
 {
struct alternate_object_database *alt;
 
-   if (the_repository->objects.packed_git_initialized)
+   if (r->objects.packed_git_initialized)
return;
-   prepare_packed_git_one(the_repository, get_object_directory(), 1);
-   prepare_alt_odb(the_repository);
-   for (alt = the_repository->objects.alt_odb_list; alt; alt = alt->next)
-   prepare_packed_git_one(the_repository, alt->path, 0);
-   rearrange_packed_git(the_repository);
-   prepare_packed_git_mru(the_repository);
-   the_repository->objects.packed_git_initialized = 1;
+   prepare_packed_git_one(r, get_object_directory(), 1);
+   prepare_alt_odb(r);
+   for (alt = r->objects.alt_odb_list; alt; alt = alt->next)
+   prepare_packed_git_one(r, alt->path, 0);
+   rearrange_packed_git(r);
+   prepare_packed_git_mru(r);
+   r->objects.packed_git_initialized = 1;
 }
 
 void reprepare_packed_git_the_repository(void)
diff --git a/packfile.h b/packfile.h
index 9afbf73657e..9142866c8ae 100644
--- a/packfile.h
+++ b/packfile.h
@@ -34,8 +34,7 @@ extern struct packed_git *parse_pack_index(unsigned char 
*sha1, const char *idx_
 #define PACKDIR_FILE_GARBAGE 4
 extern void (*report_garbage)(unsigned seen_bits, const char *path);
 
-#define prepare_packed_git(r) prepare_packed_git_##r()
-extern void prepare_packed_git_the_repository(void);
+extern void prepare_packed_git(struct repository *r);
 #define reprepare_packed_git(r) reprepare_packed_git_##r()
 extern void reprepare_packed_git_the_repository(void);
 extern void install_packed_git(struct repository *r, struct packed_git *pack);
-- 
2.16.2.395.g2e18187dfd-goog

[PATCH 00/11] Moving global state into the repository object (part 2)

This applies on top of origin/sb/object-store and is the continuation of
that series, adding the repository as a context argument to functions.

This series focusses on packfile handling, exposing (re)prepare_packed_git
and find_pack_entry to a repository argument.

Looking at the diffstat of "Delete ignore_env member in struct repository"[1]
and "Fix initializing the_hash_algo"[2], which also build on 
origin/sb/object-store, 
this series looks rather orthogonal to those, so I would not a lot of merge
conflicts.

[1] https://public-inbox.org/git/20180227095846.9238-1-pclo...@gmail.com/
[2] https://public-inbox.org/git/20180225111840.16421-1-pclo...@gmail.com/

The third series (after this one) will focus on object replacement,
such that we can migrate sha1_object_info_extended at the end of the
third series.

Thanks,
Stefan

Stefan Beller (11):
  packfile: allow prepare_packed_git_mru to handle arbitrary
repositories
  packfile: allow rearrange_packed_git to handle arbitrary repositories
  packfile: allow install_packed_git to handle arbitrary repositories
  packfile: add repository argument to prepare_packed_git_one
  packfile: add repository argument to prepare_packed_git
  packfile: add repository argument to reprepare_packed_git
  packfile: allow prepare_packed_git_one to handle arbitrary
repositories
  packfile: allow prepare_packed_git to handle arbitrary repositories
  packfile: allow reprepare_packed_git to handle arbitrary repositories
  packfile: add repository argument to find_pack_entry
  packfile: allow find_pack_entry to handle arbitrary repositories

 builtin/count-objects.c  |  2 +-
 builtin/fsck.c   |  2 +-
 builtin/gc.c |  4 +--
 builtin/pack-objects.c   |  2 +-
 builtin/pack-redundant.c |  2 +-
 builtin/receive-pack.c   |  3 +-
 bulk-checkin.c   |  3 +-
 fast-import.c|  4 +--
 fetch-pack.c |  3 +-
 http-backend.c   |  2 +-
 http.c   |  2 +-
 pack-bitmap.c|  2 +-
 packfile.c   | 72 +++-
 packfile.h   | 12 ---
 server-info.c|  2 +-
 sha1_file.c  |  8 ++---
 sha1_name.c  |  4 +--
 17 files changed, 66 insertions(+), 63 deletions(-)

-- 
2.16.2.395.g2e18187dfd-goog

[PATCH 07/11] packfile: allow prepare_packed_git_one to handle arbitrary repositories

Signed-off-by: Stefan Beller 
---
 packfile.c | 7 +++
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/packfile.c b/packfile.c
index 6ea50230aab..c45516acd41 100644
--- a/packfile.c
+++ b/packfile.c
@@ -735,8 +735,7 @@ static void report_pack_garbage(struct string_list *list)
report_helper(list, seen_bits, first, list->nr);
 }
 
-#define prepare_packed_git_one(r, o, l) prepare_packed_git_one_##r(o, l)
-static void prepare_packed_git_one_the_repository(char *objdir, int local)
+static void prepare_packed_git_one(struct repository *r, char *objdir, int 
local)
 {
struct strbuf path = STRBUF_INIT;
size_t dirnamelen;
@@ -769,7 +768,7 @@ static void prepare_packed_git_one_the_repository(char 
*objdir, int local)
base_len = path.len;
if (strip_suffix_mem(path.buf, _len, ".idx")) {
/* Don't reopen a pack we already have. */
-   for (p = the_repository->objects.packed_git; p;
+   for (p = r->objects.packed_git; p;
 p = p->next) {
size_t len;
if (strip_suffix(p->pack_name, ".pack", ) &&
@@ -783,7 +782,7 @@ static void prepare_packed_git_one_the_repository(char 
*objdir, int local)
 * corresponding .pack file that we can map.
 */
(p = add_packed_git(path.buf, path.len, local)) != 
NULL)
-   install_packed_git(the_repository, p);
+   install_packed_git(r, p);
}
 
if (!report_garbage)
-- 
2.16.2.395.g2e18187dfd-goog

[PATCH 02/11] packfile: allow rearrange_packed_git to handle arbitrary repositories

Signed-off-by: Stefan Beller 
---
 packfile.c | 8 
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/packfile.c b/packfile.c
index b844b653265..5ccce419354 100644
--- a/packfile.c
+++ b/packfile.c
@@ -866,10 +866,10 @@ static int sort_pack(const void *a_, const void *b_)
return -1;
 }
 
-static void rearrange_packed_git(void)
+static void rearrange_packed_git(struct repository *r)
 {
-   the_repository->objects.packed_git = llist_mergesort(
-   the_repository->objects.packed_git, get_next_packed_git,
+   r->objects.packed_git = llist_mergesort(
+   r->objects.packed_git, get_next_packed_git,
set_next_packed_git, sort_pack);
 }
 
@@ -893,7 +893,7 @@ void prepare_packed_git(void)
prepare_alt_odb(the_repository);
for (alt = the_repository->objects.alt_odb_list; alt; alt = alt->next)
prepare_packed_git_one(alt->path, 0);
-   rearrange_packed_git();
+   rearrange_packed_git(the_repository);
prepare_packed_git_mru(the_repository);
the_repository->objects.packed_git_initialized = 1;
 }
-- 
2.16.2.395.g2e18187dfd-goog

Re: [PATCH] protocol: treat unrecognized protocol.version setting as 0

2018-02-27 Thread Brandon Williams

On 02/27, Jonathan Nieder wrote:
> If I share my .gitconfig or .git/config file between multiple machines
> (or between multiple Git versions on a single machine) and set
> 
>   [protocol]
>   version = 2
> 
> then running "git fetch" with a Git version that does not support
> protocol v2 errors out with
> 
>   fatal: unknown value for config 'protocol.version': 2
> 
> In the spirit of v1.7.6-rc0~77^2~1 (Improve error handling when
> parsing dirstat parameters, 2011-04-29), it is better to (perhaps
> after warning the user) ignore the unrecognized protocol version.
> After all, future Git versions might add even more protocol versions,
> and using two different Git versions with the same Git repo, machine,
> or home directory should not cripple the older Git version just
> because of a parameter that is only understood by a more recent Git
> version.
> 
> So ignore the unrecognized value.  It may be useful for spell checking
> (for instance, if I put "version = v1" intending "version = 1") to
> warn about such settings, but this patch does not, since at least in
> these early days for protocol v2 it is expected for configurations
> that want to opportunistically use protocol v2 if available not to be
> unusual.
> 
> Signed-off-by: Jonathan Nieder 
> ---
> Google has been running with a patch like this internally for a while,
> since we have been changing the protocol.version number to a new value
> like 20180226 each time a minor tweak to the protocolv2 RFC occured.
> 
> The bit I have doubts about is whether to warn.  What do you think?

Patch looks good to me.  And I don't have a strong preference either way
for whether to warn or not.


-- 
Brandon Williams

Re: [PATCH v2 3/4] sha1_file.c: move delayed getenv(altdb) back to setup_git_env()

2018-02-27 Thread Duy Nguyen

On Wed, Feb 28, 2018 at 3:12 AM, Brandon Williams  wrote:
> On 02/27, Nguyễn Thái Ngọc Duy wrote:
>> diff --git a/repository.c b/repository.c
>> index 7654b8ada9..e326f0fcbc 100644
>> --- a/repository.c
>> +++ b/repository.c
>> @@ -61,6 +61,8 @@ void repo_set_gitdir(struct repository *repo,
>>   repo_set_commondir(repo, o->shared_root);
>>   expand_base_dir(>objects.objectdir, o->object_dir,
>>   repo->commondir, "objects");
>> + free(repo->objects.alternate_db);
>> + repo->objects.alternate_db = xstrdup_or_null(o->alternate_db);
>
> Just a note that this only works because the object store is embedded in
> the repository struct, it isn't a pointer to an object store.

It is possible to make it work even if object store is not embedded
though. From my point of view, this function is about "give me
$GIT_DIR and optionally the where all other parts are, if you ignore
default repo layout and tear the repository apart". We could
initialize the object store later when it's created and pass the
relevant paths to it. Exactly where it's safe to "create and
initialize object store" is harder to see now because the whole main
repo setup is spread out over many many functions.
-- 
Duy

[PATCH] protocol: treat unrecognized protocol.version setting as 0

If I share my .gitconfig or .git/config file between multiple machines
(or between multiple Git versions on a single machine) and set

[protocol]
version = 2

then running "git fetch" with a Git version that does not support
protocol v2 errors out with

fatal: unknown value for config 'protocol.version': 2

In the spirit of v1.7.6-rc0~77^2~1 (Improve error handling when
parsing dirstat parameters, 2011-04-29), it is better to (perhaps
after warning the user) ignore the unrecognized protocol version.
After all, future Git versions might add even more protocol versions,
and using two different Git versions with the same Git repo, machine,
or home directory should not cripple the older Git version just
because of a parameter that is only understood by a more recent Git
version.

So ignore the unrecognized value.  It may be useful for spell checking
(for instance, if I put "version = v1" intending "version = 1") to
warn about such settings, but this patch does not, since at least in
these early days for protocol v2 it is expected for configurations
that want to opportunistically use protocol v2 if available not to be
unusual.

Signed-off-by: Jonathan Nieder 
---
Google has been running with a patch like this internally for a while,
since we have been changing the protocol.version number to a new value
like 20180226 each time a minor tweak to the protocolv2 RFC occured.

The bit I have doubts about is whether to warn.  What do you think?

Thanks,
Jonathan

 protocol.c |  8 ++--
 t/t5700-protocol-v1.sh | 12 
 2 files changed, 14 insertions(+), 6 deletions(-)

diff --git a/protocol.c b/protocol.c
index 43012b7eb6..ce9c634a3a 100644
--- a/protocol.c
+++ b/protocol.c
@@ -17,12 +17,8 @@ enum protocol_version get_protocol_version_config(void)
const char *value;
if (!git_config_get_string_const("protocol.version", )) {
enum protocol_version version = parse_protocol_version(value);
-
-   if (version == protocol_unknown_version)
-   die("unknown value for config 'protocol.version': %s",
-   value);
-
-   return version;
+   if (version != protocol_unknown_version)
+   return version;
}
 
return protocol_v0;
diff --git a/t/t5700-protocol-v1.sh b/t/t5700-protocol-v1.sh
index ba86a44eb1..c35767ab01 100755
--- a/t/t5700-protocol-v1.sh
+++ b/t/t5700-protocol-v1.sh
@@ -31,6 +31,18 @@ test_expect_success 'clone with git:// using protocol v1' '
grep "clone< version 1" log
 '
 
+test_expect_success 'unrecognized protocol versions fall back to v0' '
+   GIT_TRACE_PACKET=1 git -c protocol.version= \
+   clone "$GIT_DAEMON_URL/parent" v 2>log &&
+
+   git -C daemon_child log -1 --format=%s >actual &&
+   git -C "$daemon_parent" log -1 --format=%s >expect &&
+   test_cmp expect actual &&
+
+   # Client requested and server responded using protocol v0
+   ! grep version log
+'
+
 test_expect_success 'fetch with git:// using protocol v1' '
test_commit -C "$daemon_parent" two &&
 
-- 
2.16.2.395.g2e18187dfd

Re: [PATCH 04/11] t3030-merge-recursive: don't check the stderr of a subshell

On Tue, Feb 27, 2018 at 10:17 PM, Junio C Hamano  wrote:
> SZEDER Gábor  writes:
>
>> + git read-tree -i -m $c3 2>actual-err &&
>> + test_must_be_empty expected-err &&
>> + git update-index --ignore-missing --refresh 2>>actual-err &&
>> + test_must_be_empty expected-err &&
>> + git merge-recursive $c0 -- $c3 $c7 2>>actual-err &&
>> + test_must_be_empty expected-err &&
>> + git ls-files -s >actual-files 2>>actual-err &&
>> + test_must_be_empty expected-err
>
> Also, with the error output of individual steps tested like this
> (assuming that test-must-be-empty checks are to be done on
> the actual-err file, not ecpected-err that nobody creates), I do not
> see a point in appending to the file.  So perhaps squash this in?

Agreed again.


>  t/t3030-merge-recursive.sh | 22 +++---
>  1 file changed, 11 insertions(+), 11 deletions(-)
>
> diff --git a/t/t3030-merge-recursive.sh b/t/t3030-merge-recursive.sh
> index cbeea1cf94..3563e77b37 100755
> --- a/t/t3030-merge-recursive.sh
> +++ b/t/t3030-merge-recursive.sh
> @@ -526,13 +526,13 @@ test_expect_success 'merge-recursive w/ empty work tree 
> - ours has rename' '
> export GIT_INDEX_FILE &&
> mkdir "$GIT_WORK_TREE" &&
> git read-tree -i -m $c7 2>actual-err &&
> -   test_must_be_empty expected-err &&
> +   test_must_be_empty actual-err &&
> git update-index --ignore-missing --refresh 2>actual-err &&
> -   test_must_be_empty expected-err &&
> +   test_must_be_empty actual-err &&
> git merge-recursive $c0 -- $c7 $c3 2>actual-err &&
> -   test_must_be_empty expected-err &&
> +   test_must_be_empty actual-err &&
> git ls-files -s >actual-files 2>actual-err &&
> -   test_must_be_empty expected-err
> +   test_must_be_empty actual-err
> ) &&
> cat >expected-files <<-EOF &&
> 100644 $o3 0b/c
> @@ -551,13 +551,13 @@ test_expect_success 'merge-recursive w/ empty work tree 
> - theirs has rename' '
> export GIT_INDEX_FILE &&
> mkdir "$GIT_WORK_TREE" &&
> git read-tree -i -m $c3 2>actual-err &&
> -   test_must_be_empty expected-err &&
> -   git update-index --ignore-missing --refresh 2>>actual-err &&
> -   test_must_be_empty expected-err &&
> -   git merge-recursive $c0 -- $c3 $c7 2>>actual-err &&
> -   test_must_be_empty expected-err &&
> -   git ls-files -s >actual-files 2>>actual-err &&
> -   test_must_be_empty expected-err
> +   test_must_be_empty actual-err &&
> +   git update-index --ignore-missing --refresh 2>actual-err &&
> +   test_must_be_empty actual-err &&
> +   git merge-recursive $c0 -- $c3 $c7 2>actual-err &&
> +   test_must_be_empty actual-err &&
> +   git ls-files -s >actual-files 2>actual-err &&
> +   test_must_be_empty actual-err
> ) &&
> cat >expected-files <<-EOF &&
> 100644 $o3 0b/c

Re: [PATCH 2/2] t5556: replace test_i18ngrep with a simple grep

On Wed, Feb 28, 2018 at 12:47 AM, Ramsay Jones
 wrote:
>
>
> On 27/02/18 22:05, Junio C Hamano wrote:
>> Junio C Hamano  writes:
>>
>>> OK, somehow I had the version from Ramsay on a topic branch that was
>>> not merged to 'pu'.  Here is the replacement for 2/2 I'd be queuing.
>>>
>>> We'd need SZEDER to sign it off (optionally correcting mistakes in
>>> the log message) if we are going with this solution.
>>>
>>> Thanks.
>>
>> I guess I missed Ramsay's v2 which already did this
>>
>> <550fb3f4-8d25-c5c4-0ecd-3a4e61ea1...@ramsayjones.plus.com>
>
> Yes, and as I said in the cover letter, I wasn't too sure that
> I had passed that patch along correctly. ;-)
>
>> so I'll use that version.  We still want sign-off from Szeder,
>> though.
>
> I would be happy with either version, or maybe Szeder would like
> to tweak the commit message. In any event, it would be good to
> get sign-off from Szeder.

Certainly, here you go:

Signed-off-by: SZEDER Gábor 

However, I'm not sure about the authorship and taking credit for the
patch.  We ended up taking my patch, sure, but I think Ramsay did all
the real hard work, i.e. writing the commit message and, most
importantly, realizing that something is wrong with that '...| sort' at
the end of the line.

Re: [RFC] Rebasing merges: a jorney to the ultimate solution (Road Clear)

2018-02-27 Thread Jacob Keller

On Tue, Feb 27, 2018 at 3:40 PM, Igor Djordjevic
 wrote:
> On 27/02/2018 20:59, Igor Djordjevic wrote:
>>
>> (3) ---X1---o---o---o---o---o---X2
>>|\   |\
>>| A1---A2---A3---U1  | A1'--A2'--A3'--U1'
>>| \  |
>>|  M |
>>| /  |
>>\-B1---B2---B3---U2  \-B1'--B2'--B3'--U2'
>>
>
> Meh, I hope I`m rushing it now, but for example, if we had decided to
> drop commit A2 during an interactive rebase (so losing A2' from
> diagram above), wouldn`t U2' still introduce those changes back, once
> U1' and U2' are merged, being incorrect/unwanted behavior...? :/
>
> p.s. Looks like Johannes already elaborated on this in the meantime,
> let`s see... (goes reading that other e-mail[1])
>
> [1] 
> https://public-inbox.org/git/nycvar.qro.7.76.6.1802272330290...@zvavag-6oxh6da.rhebcr.pbec.zvpebfbsg.pbz/


In that case, the method won't work well at all, so I think we need a
different approach.

Thanks,
Jake

Re: [RFC] Rebasing merges: a jorney to the ultimate solution (Road Clear)

2018-02-27 Thread Jacob Keller

On Tue, Feb 27, 2018 at 10:14 AM, Junio C Hamano  wrote:
> Sergey Organov  writes:
>
>> You've already bit this poor thingy to death. Please rather try your
>> teeth on the proposed Trivial Merge (TM) method.
>
> Whatever you do, do *NOT* call any part of your proposal "trivial
> merge", unless you are actually using the term to mean what Git
> calls "trivial merge".  The phrase has an established meaning in Git
> and your attempt to abuse it to mean something entirely different is
> adding unnecessary hindrance for other people to understand what you
> want to perform.

Agreed, I think we need better terminology here, the current words for
(TM) are definitely *not* trivial merges. Same for "angel merge", I
don't think that term really works well either.

The goal of the process is to split the merge apart to its components
for each side branch and then bring them back together after applying
them to the newly rebased branches.

Re: [RFC] Rebasing merges: a jorney to the ultimate solution (Road Clear)

2018-02-27 Thread Jacob Keller

On Tue, Feb 27, 2018 at 8:21 AM, Johannes Schindelin
 wrote:
> Hi Jake,
>
> On Mon, 26 Feb 2018, Jacob Keller wrote:
>
>> On Mon, Feb 26, 2018 at 4:07 PM, Johannes Schindelin
>>  wrote:
>> >
>> > On Tue, 20 Feb 2018, Igor Djordjevic wrote:
>> >
>> >> I`m really interested in this topic, which seems to (try to) address the
>> >> only "bad feeling" I had with rebasing merges - being afraid of silently
>> >> losing amendments by actually trying to "replay" the merge (where
>> >> additional and possibly important context is missing), instead of really
>> >> "rebasing" it (somehow).
>> >
>> > If those amendments are what you are worried about, why not address them
>> > specifically?
>> >
>> > In other words, rather than performing the equivalent of
>> >
>> > git show ^! | git apply
>> >
>> > (which would of course completely ignore if the rewritten ^2
>> > dropped a patch, amended a patch, or even added a new patch), what you
>> > really want is to figure out what changes the user made when merging, and
>> > what merge strategy was used to begin with.
>> >
>> > To see what I mean, look at the output of `git show 0fd90daba8`: it shows
>> > how conflicts were resolved. By necessity, this is more complicated than a
>> > simple diff: it is *not* as simple as taking a diff between two revisions
>> > and applying that diff to a third revision. There were (at least) three
>> > revisions involved in the original merge commit, and recreating that merge
>> > commit faithfully means to represent the essence of the merge commit
>> > faithfully enough to be able to replay it on a new set of at least three
>> > revisions.  That can be simplified to two-way diffs only in very, very
>> > special circumstances, and in all other cases this simplification will
>> > simply fall on its nose.
>> >
>> > If the proposed solution was to extend `git apply` to process combined
>> > diffs, I would agree that we're on to something. That is not the proposed
>> > solution, though.
>> >
>> > In short: while I am sympathetic to the desire to keep things simple,
>> > the idea to somehow side-step replaying the original merge seems to be
>> > *prone* to be flawed. Any system that cannot accommodate
>> > dropped/changed/added commits on either side of a merge is likely to be
>> > too limited to be useful.
>> >
>>
>>
>> The reason Sergey's solution works is because he cherry picks the
>> merge using each parent first, and then merges the result of those. So
>> each branch of the merge gets one, and then you merge the result of
>> those cherry-picks. This preservers amendments and changes properly,
>> and should result in a good solution.
>
> I saw your patch trying to add a minimal example, and I really want to run
> away screaming.
>
> Do you have any way to describe the idea in a simple, 3-5 lines long
> paragraph?
>
> So far, I just know that it is some sort of confusing criss-cross
> cherry-picking and merging and stuff, but nothing in those steps shouts
> out to me what the *idea* is.
>

Sergey's posted explained it more in detail, at
https://public-inbox.org/git/87y3jtqdyg@javad.com/

I was mostly just attempting to re-create it in a test case to show
that it could work.

> If it would be something like "recreate the old merge, with merge
> conflicts and all, then generate the diff to the actual tree of the merge
> commit, then apply that to the newly-generated merge", I would understand.
>

It's more or less:

Rebase each parent, then cherry-pick -m the original merge to that
parent, then you merge the result of each cherry-pick, then use the
resulting final merged tree to create the merge pointing at the real
parents instead of the cherry-pick merges.

> I would still suspect that -s ours would be a hard nut for that method,
> but I would understand that idea.
>

The goal of the process isn't to know or understand the "-s ours"
strategy, but simply re-create the contents of the original merge
faithfully, while still preserving the changes done when rebasing the
side branches. Thus it should re-create the contents generated by "-s
ours" the first time, but it doesn't need to do or know anything
special about how the content was created.

> Thanks,
> Dscho

Re: [Problem] test_must_fail makes possibly questionable assumptions about exit_code.

Hi,

Randall S. Becker wrote:

> After months of arguing with some platform developers on this subject, the
> perl spec was held over my head repeatedly about a few lines that are
> causing issues. The basic problem is this line (test-lib-functions.sh, line
> 633, still in ffa952497)
>
>>elif test $exit_code -gt 129 && test $exit_code -le 192
>>   then
>>   echo >&2 "test_must_fail: died by signal $(($exit_code - 128)):
>
> According to the perl spec http://perldoc.perl.org/functions/die.html, die
> basically takes whatever errno is, mods it with 256 and there you go. EBADF
> is what is used when perl reads from stdin and calls die - that's standard
> perl. In most systems, you end up with something useful, when EBADF is 9.
> But when it is 4009, you get a garbage answer (4009 mod 256 a.k.a. 169).
> However, only 128-165 are technically reserved for signals, rather than all
> the way up to 192, which may be true in some places but not everywhere.
>
> The advice (I'm putting that nicely) I received was to use exit so that the
> result is predictable - unlikely to be useful in the 15K test suites in git.

The fundamental thing is the actual Git commands, not the tests in the
testsuite, no?

In the rest of git, die() makes a command exit with status 128.  The
trouble here is that our code in Perl is assuming the same meaning for
die() but using perl's die builtin instead.  That suggests a few
options:

 a) We could override the meaning of die() in Git.pm.  This feels
ugly but if it works, it would be a very small patch.

 b) We could forbid use of die() and use some git_die() instead (but
with a better name) for our own error handling.

 c) We could have a special different exit code convention for
commands written in Perl.  And then change expectations whenever a
command is rewritten in C.  As you might expect, I don't like this
option.

 d) We could wrap each command in an eval {...} block to convert the
result from die() to exit 128.

Option (b) feels simplest to me.

Thoughts?

Thanks,
Jonathan

Re: [RFC] Rebasing merges: a jorney to the ultimate solution (Road Clear)

Igor Djordjevic  writes:

> On 27/02/2018 20:59, Igor Djordjevic wrote:
>> 
>> (3) ---X1---o---o---o---o---o---X2
>>|\   |\
>>| A1---A2---A3---U1  | A1'--A2'--A3'--U1'
>>| \  |
>>|  M |
>>| /  |
>>\-B1---B2---B3---U2  \-B1'--B2'--B3'--U2'
>> 
>
> Meh, I hope I`m rushing it now, but for example, if we had decided to 
> drop commit A2 during an interactive rebase (so losing A2' from 
> diagram above), wouldn`t U2' still introduce those changes back, once 
> U1' and U2' are merged, being incorrect/unwanted behavior...? :/
>
> p.s. Looks like Johannes already elaborated on this in the meantime, 
> let`s see... (goes reading that other e-mail[1])

As long as we are talking about rebase that allows us users to
adjust and make changes ("rebase -i" being the prime and most
flexible example), it is easy to imagine that A1'--A3' and B1'--B3'
have almost no relation to their original counterparts.  After all,
mechanical merge won't be able to guess the intention of the change
humans make, so depending on what happend during X1 and X2 that
happend outside of these two topics, what's required to bring series
A and B to series A' and B' can be unlimited.  So from that alone,
it should be clear that replaying difference between M and A3 (and M
and B3) on top of U1' and U2' is hopeless as a general solution.

It is acceptable as long as a solution fails loudly when it does the
wrong thing, but I do not think the apporach can produce incorrect
result silently, as your example shows above.

What you _COULD_ learn from an old merge is to compute:

- a trial and mechanical merge between A3 and B3; call that pre-M

- diff to bring pre-M to M (call that patch evil-M); which is
  what the person who made M did to resolve the textual and
  semantic conflicts necessary to merge these two topics.

Then when merging A3' and B3', you could try to mechanically merge
them (call that pre-M'), and apply evil-M, which may apply cleanly
on top of pre-M', or it may not.  When there aren't so huge a
difference between series A and A' (and series B and B'), the result
would probably be a moral equivalent of Sergay's "replay" (so this
approach will also silently produce a wrong result without human
supervision).  One edge the evil-M approach has over Sergey's "dual
cherry pick" is that it separates and highlights non-mechanical
conflict resolution out of mechanical merges in a human readable
form (i.e. the patch evil-M).

Re: [PATCH v3 34/35] remote-curl: implement stateless-connect command

Hi,

Brandon Williams wrote:

> Teach remote-curl the 'stateless-connect' command which is used to
> establish a stateless connection with servers which support protocol
> version 2.  This allows remote-curl to act as a proxy, allowing the git
> client to communicate natively with a remote end, simply using
> remote-curl as a pass through to convert requests to http.

Cool!  I better look at the spec for that first.

*looks at the previous patch*

Oh, there is no documented spec. :/  I'll muddle through this instead,
then.

[...]
> --- a/remote-curl.c
> +++ b/remote-curl.c
> @@ -188,7 +188,10 @@ static struct ref *parse_git_refs(struct discovery 
> *heads, int for_push)
>   heads->version = discover_version();
>   switch (heads->version) {
>   case protocol_v2:
> - die("support for protocol v2 not implemented yet");
> + /*
> +  * Do nothing.  Client should run 'stateless-connect' and
> +  * request the refs themselves.
> +  */
>   break;

This is the 'list' command, right?  Since we expect the client to run
'stateless-connect' instead, can we make it error out?

[...]
> @@ -1082,6 +1085,184 @@ static void parse_push(struct strbuf *buf)
>   free(specs);
>  }
>  
> +struct proxy_state {
> + char *service_name;
> + char *service_url;
> + struct curl_slist *headers;
> + struct strbuf request_buffer;
> + int in;
> + int out;
> + struct packet_reader reader;
> + size_t pos;
> + int seen_flush;
> +};

Can this have a comment describing what it is/does?  It's not obvious
to me at first glance.

It doesn't have to go in a lot of detail since this is an internal
implementation detail, but something saying e.g. that this represents
a connection to an HTTP server (that's an example; I'm not saying
that's what it represents :)) would help.

> +
> +static void proxy_state_init(struct proxy_state *p, const char *service_name,
> +  enum protocol_version version)
[...]
> +static void proxy_state_clear(struct proxy_state *p)

Looks sensible.

[...]
> +static size_t proxy_in(char *buffer, size_t eltsize,
> +size_t nmemb, void *userdata)

Can this have a comment describing the interface?  (E.g. does it read
a single pkt_line?  How is the caller expected to use it?  Does it
satisfy the interface of some callback?)

libcurl's example https://curl.haxx.se/libcurl/c/ftpupload.html just
calls this read_callback.  Such a name plus a pointer to
CURLOPT_READFUNCTION should do the trick; bonus points if the comment 
says what our implementation of the callback does.

Is this about having peek ability?

> +{
> + size_t max = eltsize * nmemb;

Can this overflow?  st_mult can avoid having to worry about that.

> + struct proxy_state *p = userdata;
> + size_t avail = p->request_buffer.len - p->pos;
> +
> + if (!avail) {
> + if (p->seen_flush) {
> + p->seen_flush = 0;
> + return 0;
> + }
> +
> + strbuf_reset(>request_buffer);
> + switch (packet_reader_read(>reader)) {
> + case PACKET_READ_EOF:
> + die("unexpected EOF when reading from parent process");
> + case PACKET_READ_NORMAL:
> + packet_buf_write_len(>request_buffer, p->reader.line,
> +  p->reader.pktlen);
> + break;
> + case PACKET_READ_DELIM:
> + packet_buf_delim(>request_buffer);
> + break;
> + case PACKET_READ_FLUSH:
> + packet_buf_flush(>request_buffer);
> + p->seen_flush = 1;
> + break;
> + }
> + p->pos = 0;
> + avail = p->request_buffer.len;
> + }
> +
> + if (max < avail)
> + avail = max;
> + memcpy(buffer, p->request_buffer.buf + p->pos, avail);
> + p->pos += avail;
> + return avail;

This is a number of bytes, but CURLOPT_READFUNCTION expects a number
of items, fread-style.  That is:

if (avail < eltsize)
... handle somehow, maybe by reading in more? ...

avail_memb = avail / eltsize;
memcpy(buffer,
   p->request_buffer.buf + p->pos,
   st_mult(avail_memb, eltsize));
p->pos += st_mult(avail_memb, eltsize);
return avail_memb;

But https://curl.haxx.se/libcurl/c/CURLOPT_READFUNCTION.html says

Your function must then return the actual number of bytes that
it stored in that memory area.

Does this mean eltsize is always 1?  This is super confusing...

... ok, a quick grep for fread_func in libcurl reveals that eltsize is
indeed always 1.  Can we add an assertion so we notice if that
changes?

if (eltsize != 1)
BUG("curl read callback called with size = %zu != 1", eltsize);

[Problem] test_must_fail makes possibly questionable assumptions about exit_code.

2018-02-27 Thread Randall S. Becker

Hi all,

After months of arguing with some platform developers on this subject, the
perl spec was held over my head repeatedly about a few lines that are
causing issues. The basic problem is this line (test-lib-functions.sh, line
633, still in ffa952497)

>elif test $exit_code -gt 129 && test $exit_code -le 192
>   then
>   echo >&2 "test_must_fail: died by signal $(($exit_code -
128)):

According to the perl spec http://perldoc.perl.org/functions/die.html, die
basically takes whatever errno is, mods it with 256 and there you go. EBADF
is what is used when perl reads from stdin and calls die - that's standard
perl. In most systems, you end up with something useful, when EBADF is 9.
But when it is 4009, you get a garbage answer (4009 mod 256 a.k.a. 169).
However, only 128-165 are technically reserved for signals, rather than all
the way up to 192, which may be true in some places but not everywhere.

The advice (I'm putting that nicely) I received was to use exit so that the
result is predictable - unlikely to be useful in the 15K test suites in git.
However, dropping this to 165 conditionally might help.

I'm looking for what approach to take here, because I don't think I'm going
to get perl fixed any time soon, or the error number range on the platform
fixed ... ever.

This is causing only two breaks that I have lived with and probably still
could. Consider me begging for a suggestion.

Sincerest,
Randall

-- Brief whoami:
 NonStop developer since approximately 2112884442
 UNIX developer since approximately 421664400
-- In my real life, I talk too much.

Re: [PATCH 2/2] t5556: replace test_i18ngrep with a simple grep

2018-02-27 Thread Ramsay Jones

On 27/02/18 22:05, Junio C Hamano wrote:
> Junio C Hamano  writes:
> 
>> OK, somehow I had the version from Ramsay on a topic branch that was
>> not merged to 'pu'.  Here is the replacement for 2/2 I'd be queuing.
>>
>> We'd need SZEDER to sign it off (optionally correcting mistakes in
>> the log message) if we are going with this solution.
>>
>> Thanks.
> 
> I guess I missed Ramsay's v2 which already did this
> 
> <550fb3f4-8d25-c5c4-0ecd-3a4e61ea1...@ramsayjones.plus.com>

Yes, and as I said in the cover letter, I wasn't too sure that
I had passed that patch along correctly. ;-)

> so I'll use that version.  We still want sign-off from Szeder,
> though.

I would be happy with either version, or maybe Szeder would like
to tweak the commit message. In any event, it would be good to
get sign-off from Szeder.

Thanks!

ATB,
Ramsay Jones

Re: [PATCH 0/5] roll back locks in various code paths

Martin Ågren  writes:

> Patches 2-4 are the actual fixes where I teach some functions to always
> roll back the lock they're holding. Notably, these are all in "libgit".
>
> Patch 1 is a "while at it" to use locks on the stack instead of having
> them be static. Patch 5 removes code to roll back locks which are
> already rolled back.
>
> I've based this on maint. There's a conflict on pu, with c7d4394111
> (sequencer: avoid using errno clobbered by rollback_lock_file(),
> 2018-02-11). The conflict resolution would be to take my version for the
> "could not lock HEAD"-hunk.

Thanks for running a trial merge before sending your patches out.  I
wish there were more contributors like you ;-)

The changes looked reasonable.

Re: [RFC] Rebasing merges: a jorney to the ultimate solution (Road Clear)

On 27/02/2018 20:59, Igor Djordjevic wrote:
> 
> (3) ---X1---o---o---o---o---o---X2
>|\   |\
>| A1---A2---A3---U1  | A1'--A2'--A3'--U1'
>| \  |
>|  M |
>| /  |
>\-B1---B2---B3---U2  \-B1'--B2'--B3'--U2'
> 

Meh, I hope I`m rushing it now, but for example, if we had decided to 
drop commit A2 during an interactive rebase (so losing A2' from 
diagram above), wouldn`t U2' still introduce those changes back, once 
U1' and U2' are merged, being incorrect/unwanted behavior...? :/

p.s. Looks like Johannes already elaborated on this in the meantime, 
let`s see... (goes reading that other e-mail[1])

[1] 
https://public-inbox.org/git/nycvar.qro.7.76.6.1802272330290...@zvavag-6oxh6da.rhebcr.pbec.zvpebfbsg.pbz/

Re: [PATCH v3 14/35] connect: request remote refs using v2

Jeff King  writes:

>> struct strs {...};
>> 
>> void strs_init(struct strs *);
>> void strs_push(struct strs *, const char *);
>> void strs_pushf(struct strs *, const char *fmt, ...);
>> void strs_pushl(struct strs *, ...);
>> void strs_pushv(struct strs *, const char **);
>> void strs_pop(struct strs *);
>> void strs_clear(struct strs *);
>> const char **strs_detach(struct strs *);
>> 
>> ...is short, feels pretty natural, and doesn't require understanding
>> "v" for "vector".
>
> Not bad. The "v" carries the information that it _is_ a NULL-terminated
> vector and not some other list-like structure (and so is suitable for
> feeding to execv, etc). But that may just be obvious from looking at its
> uses and documentation.

And with "v", it probably is obvious without looking at its uses and
documentation, so... ;-)

Re: [PATCH v3 28/35] transport-helper: introduce stateless-connect

Brandon Williams wrote:

> Introduce the transport-helper capability 'stateless-connect'.  This
> capability indicates that the transport-helper can be requested to run
> the 'stateless-connect' command which should attempt to make a
> stateless connection with a remote end.  Once established, the
> connection can be used by the git client to communicate with
> the remote end natively in a stateless-rpc manner as supported by
> protocol v2.  This means that the client must send everything the server
> needs in a single request as the client must not assume any
> state-storing on the part of the server or transport.
>
> If a stateless connection cannot be established then the remote-helper
> will respond in the same manner as the 'connect' command indicating that
> the client should fallback to using the dumb remote-helper commands.
>
> Signed-off-by: Brandon Williams 
> ---
>  transport-helper.c | 8 
>  transport.c| 1 +
>  transport.h| 6 ++
>  3 files changed, 15 insertions(+)

Please add documentation for this command to
Documentation/gitremote-helpers.txt.

That helps reviewers, since it means reviewers can get a sense of what
the interface is meant to be.  It helps remote helper implementers as
well: it tells them what they can rely on and what can't rely on in
this interface.  For the same reason it helpers remote helper callers
as well.

Thanks,
Jonathan

Re: [RFC] Rebasing merges: a jorney to the ultimate solution (Road Clear)

2018-02-27 Thread Johannes Schindelin

Hi Buga,

thank you for making this a lot more understandable to this thick
developer.

On Tue, 27 Feb 2018, Igor Djordjevic wrote:

> On 27/02/2018 19:55, Igor Djordjevic wrote:
> > 
> > It would be more along the lines of "(1) rebase old merge commit parents, 
> > (2) generate separate diff between old merge commit and each of its 
> > parents, (3) apply each diff to their corresponding newly rebased 
> > parent respectively (as a temporary commit, one per rebased parent), 
> > (4) merge these temporary commits to generate 'rebased' merge commit, 
> > (5) drop temporary commits, recording their parents as parents of 
> > 'rebased' merge commit (instead of dropped temporary commits)".
> > 
> > Implementation wise, steps (2) and (3) could also be done by simply 
> > copying old merge commit _snapshot_ on top of each of its parents as 
> > a temporary, non-merge commit, then rebasing (cherry-picking) these 
> > temporary commits on top of their rebased parent commits to produce 
> > rebased temporary commits (to be merged for generating 'rebased' 
> > merge commit in step (4)).
> 
> For those still tagging along (and still confused), here are some 
> diagrams (following what Sergey originally described). Note that 
> actual implementation might be even simpler, but I believe it`s a bit 
> easier to understand like this, using some "temporary" commits approach.
> 
> Here`s our starting position:
> 
> (0) ---X1---o---o---o---o---o---X2 (master)
>|\
>| A1---A2---A3
>| \
>|  M (topic)
>| /
>\-B1---B2---B3
> 
> 
> Now, we want to rebase merge commit M from X1 onto X2. First, rebase
> merge commit parents as usual:
> 
> (1) ---X1---o---o---o---o---o---X2
>|\   |\
>| A1---A2---A3   | A1'--A2'--A3'
>| \  |
>|  M |
>| /  |
>\-B1---B2---B3   \-B1'--B2'--B3'
> 
> 
> That was commonly understandable part.

Good. Let's assume that I want to do this interactively (because let's
face it, rebase is boring unless we shake up things a little). And let's
assume that A1 is my only change to the README, and that I realized that
it was incorrect and I do not want the world to see it, so I drop A1'.

Let's see how things go from here:

> Now, for "rebasing" the merge commit (keeping possible amendments), we
> do some extra work. First, we make two temporary commits on top of old
> merge parents, by using exact tree (snapshot) of commit M:
> 
> (2) ---X1---o---o---o---o---o---X2
>|\   |\
>| A1---A2---A3---U1  | A1'--A2'--A3'
>| \  |
>|  M |
>| /  |
>\-B1---B2---B3---U2  \-B1'--B2'--B3'

Okay, everything would still be the same except that I still have dropped
A1'.

> So here, in terms of _snapshots_ (trees, not diffs), U1 = U2 = M.
> 
> Now, we rebase these temporary commits, too:
> 
> (3) ---X1---o---o---o---o---o---X2
>|\   |\
>| A1---A2---A3---U1  | A1'--A2'--A3'--U1'
>| \  |
>|  M |
>| /  |
>\-B1---B2---B3---U2  \-B1'--B2'--B3'--U2'

I still want to drop A1 in this rebase, so A1' is still missing.

And now it starts to get interesting.

The diff between A3 and U1 does not touch the README, of course, as I said
that only A1 changed the README.  But the diff between B3 and U2 does
change the README, thanks to M containing A1 change.

Therefore, the diff between B3' and U2' will also have this change to the
README. That change that I wanted to drop.

> As a next step, we merge these temporary commits to produce our
> "rebased" merged commit M:
> 
> (4) ---X1---o---o---o---o---o---X2
>|\   |\
>| A1---A2---A3---U1  | A1'--A2'--A3'--U1'
>| \  |  \
>|  M |   M'
>| /  |  /
>\-B1---B2---B3---U2  \-B1'--B2'--B3'--U2'

And here, thanks to B3'..U2' changing the README, M' will also have that
change that I wanted to see dropped.

Note that A1' is still dropped in my example.

> Finally, we drop temporary commits, and record rebased commits A3' 
> and B3' as our "rebased" merge commit parents instead (merge commit 
> M' keeps its same tree/snapshot state, just gets parents replaced):
> 
> (5) ---X1---o---o---o---o---o---X2
>|\   |\
>| A1---A2---A3---U1  | A1'--A2'--A3'
>| \  | \
>|  M |  M'
>| /  | /
>\-B1---B2---B3---U2  \-B1'--B2'--B3'

Now, thanks to U2' being dropped (and A1' *still* being

Re: [PATCH v5 0/2] diff: add --compact-summary (aka nd/diff-stat-with-summary)

Nguyễn Thái Ngọc Duy   writes:

> ... and v5 fixes the commit message of 2/2 where in v4 it still
> mentions --stat-with-summary instead of --compact-summary. Sorry.
>
> Nguyễn Thái Ngọc Duy (2):
>   diff.c: refactor pprint_rename() to use strbuf
>   diff: add --compact-summary

Thanks, will queue.  I guess we all run out of paints of different
colours, and it's a good time to go incremental by merging the topic
to 'next'.

>
>  Documentation/diff-options.txt|  8 ++
>  diff.c| 96 ---
>  diff.h|  1 +
>  t/t4013-diff-various.sh   |  5 +
>  ...ty_--root_--stat_--compact-summary_initial | 12 +++
>  ...-R_--root_--stat_--compact-summary_initial | 12 +++
>  ...tree_--stat_--compact-summary_initial_mode |  4 +
>  ...e_-R_--stat_--compact-summary_initial_mode |  4 +
>  8 files changed, 109 insertions(+), 33 deletions(-)
>  create mode 100644 
> t/t4013/diff.diff-tree_--pretty_--root_--stat_--compact-summary_initial
>  create mode 100644 
> t/t4013/diff.diff-tree_--pretty_-R_--root_--stat_--compact-summary_initial
>  create mode 100644 
> t/t4013/diff.diff-tree_--stat_--compact-summary_initial_mode
>  create mode 100644 
> t/t4013/diff.diff-tree_-R_--stat_--compact-summary_initial_mode

Re: [PATCH v3 31/35] remote-curl: store the protocol version the server responded with

Brandon Williams wrote:

> Store the protocol version the server responded with when performing
> discovery.  This will be used in a future patch to either change the
> 'Git-Protocol' header sent in subsequent requests or to determine if a
> client needs to fallback to using a different protocol version.

nit: s/fallback/fall back/ (fallback is the noun/adjective, fall back
the verb)

> Signed-off-by: Brandon Williams 

With or without that tweak,
Reviewed-by: Jonathan Nieder 

Thanks.

Re: [PATCH] revision.c: reduce object database queries

Jeff King  writes:

>> This code comes originally form 454fbbcde3 (git-rev-list: allow missing
>> objects when the parent is marked UNINTERESTING, 2005-07-10). But later,
>> in aeeae1b771 (revision traversal: allow UNINTERESTING objects to be
>> missing, 2009-01-27), we marked dealt with calling parse_object() on the
>> parents more directly.
>>
>> So what I wonder is whether this code is simply redundant and can go
>> away entirely. That would save the has_object_file() call in all cases.

Hmm, interesting. I forgot all what I did around this area, but you
are right.  

> There's a similar case for trees. ...
> though technically the existing code allows _missing_ trees, but
> not on corrupt ones.

True, but the intention of these "do not care too much about missing
stuff while marking uninteresting" effort is aligned better with
ignoring corrupt ones, too, I would think, as "missing" in that
sentence is in fact about "not availble", and stuff that exists in
corrupt form is still not available anyway.  So I do not think it
makes a bad change to start allowing corrupt ones.

> I guess this is perhaps less interesting, because we only mark trees
> directly fed from the pending array, not every tree of commits that we
> traverse. Though if you had a really gigantic tree, it might be
> measurable.

I tend to agree that this is less interesting case than commits.

A huge tree with millions of entries in a single level would spend
quite a lot of cycle in slurping the tree data to in-core buffer,
but we do not actually parse these million entries upon opening the
tree, so it may not be too bad.

Re: [PATCH v3 29/35] pkt-line: add packet_buf_write_len function

Brandon Williams wrote:

> Add the 'packet_buf_write_len()' function which allows for writing an
> arbitrary length buffer into a 'struct strbuf' and formatting it in
> packet-line format.

Makes sense.

[...]
> --- a/pkt-line.h
> +++ b/pkt-line.h
> @@ -26,6 +26,7 @@ void packet_buf_flush(struct strbuf *buf);
>  void packet_buf_delim(struct strbuf *buf);
>  void packet_write(int fd_out, const char *buf, size_t size);
>  void packet_buf_write(struct strbuf *buf, const char *fmt, ...) 
> __attribute__((format (printf, 2, 3)));
> +void packet_buf_write_len(struct strbuf *buf, const char *data, size_t len);

I wonder if we should rename packet_buf_write to something like
packet_buf_writef.  Right now there's a kind of confusing collection of
functions without much symmetry.

Alternatively, the _buf_ ones could become strbuf_* functions:

strbuf_add_packet(, data, len);
strbuf_addf_packet(, fmt, ...);

That would make it clearer that these append to buf.

I'm just thinking out loud.  For this series, the API you have here
looks fine, even if it is a bit inconsistent.  (In other words, even
if you agree with me, this would probably be best addressed as a patch
on top.)

[...]
> --- a/pkt-line.c
> +++ b/pkt-line.c
> @@ -215,6 +215,22 @@ void packet_buf_write(struct strbuf *buf, const char 
> *fmt, ...)
>   va_end(args);
>  }
>  
> +void packet_buf_write_len(struct strbuf *buf, const char *data, size_t len)
> +{
> + size_t orig_len, n;
> +
> + orig_len = buf->len;
> + strbuf_addstr(buf, "");
> + strbuf_add(buf, data, len);
> + n = buf->len - orig_len;
> +
> + if (n > LARGE_PACKET_MAX)
> + die("protocol error: impossibly long line");

Could the error message describe the long line (e.g.

...impossibly long line %.*s...", 256, data);

)?

> +
> + set_packet_header(>buf[orig_len], n);
> + packet_trace(buf->buf + orig_len + 4, n - 4, 1);

Could do, more simply:

packet_trace(data, len, 1);

Thanks,
Jonathan

Re: [PATCH v3 26/35] transport-helper: remove name parameter

Brandon Williams wrote:

> Commit 266f1fdfa (transport-helper: be quiet on read errors from
> helpers, 2013-06-21) removed a call to 'die()' which printed the name of
> the remote helper passed in to the 'recvline_fh()' function using the
> 'name' parameter.  Once the call to 'die()' was removed the parameter
> was no longer necessary but wasn't removed.  Clean up 'recvline_fh()'
> parameter list by removing the 'name' parameter.
>
> Signed-off-by: Brandon Williams 
> ---
>  transport-helper.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)

Nice.

Reviewed-by: Jonathan Nieder

Re: [PATCH v3 4/9] t3701: don't hard code sha1 hash values

Phillip Wood  writes:

>  t/t3701-add-interactive.sh | 30 --
>  1 file changed, 20 insertions(+), 10 deletions(-)
>
> diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh
> index bdd1f292a9..46d655038f 100755
> --- a/t/t3701-add-interactive.sh
> +++ b/t/t3701-add-interactive.sh
> @@ -10,6 +10,16 @@ then
>   test_done
>  fi
>  
> +diff_cmp () {
> + for x
> + do
> + sed  -e '/^index/s/[0-9a-f]*[1-9a-f][0-9a-f]*\.\./1234567../' \
> +  -e '/^index/s/\.\.[0-9a-f]*[1-9a-f][0-9a-f]*/..9abcdef/' \
> +  "$x" >"$x.filtered"

Interesting ;-)  You require .. and on the left hand side you want
to see a run of hexdec with at least one non-zero hexdigit, which is
filtered to fixed-length 1234567; right hand side is the same deal.

Which sounds like a reasonable way to future-proof the comparison.

If 7 zeros are expected in the result, and the actual output had 8
zeros, the filter does not touch either so they compare differently,
which is somewhat unfortunate.  Perhaps something like

/^index/s/^00*\.\./000../
/^index/s/\([^0-9a-f]\)00*\.\./\1000../
/^index/s/\.\.00*$/..000/
/^index/s/\.\.00*\([^0-9a-f]\)/..000\1/

after the above two patterns help?

Re: [PATCH v2] sha1_name: fix uninitialized memory errors

On Tue, Feb 27, 2018 at 02:30:39PM -0800, Junio C Hamano wrote:

> -- >8 --
> From: Derrick Stolee 
> Date: Tue, 27 Feb 2018 06:47:04 -0500
> Subject: [PATCH] sha1_name: fix uninitialized memory errors
> 
> During abbreviation checks, we navigate to the position within a
> pack-index that an OID would be inserted and check surrounding OIDs
> for the maximum matching prefix. This position may be beyond the
> last position, because the given OID is lexicographically larger
> than every OID in the pack. Then nth_packed_object_oid() does not
> initialize "oid".
> 
> Use the return value of nth_packed_object_oid() to prevent these
> errors.
> 
> Also the comment about checking near-by objects miscounts the
> neighbours.  If we have a hit at "first", we check "first-1" and
> "first+1" to make sure we have sufficiently long abbreviation not to
> match either.  If we do not have a hit, "first" is the smallest
> among the objects that are larger than what we want to name, so we
> check that and "first-1" to make sure we have sufficiently long
> abbreviation not to match either.  In either case, we only check up
> to two near-by objects.

Yep, this looks good to me. Thanks for wrapping it up.

-Peff

Re: [PATCH/RFC 1/1] Auto diff of UTF-16 files in UTF-8

On Mon, Feb 26, 2018 at 06:27:06PM +0100, tbo...@web.de wrote:

> @@ -3611,8 +3615,25 @@ int diff_populate_filespec(struct diff_filespec *s, 
> unsigned int flags)
>   s->size = size;
>   s->should_free = 1;
>   }
> - }
> - else {
> + if (!s->binary && buffer_is_binary(s->data, s->size) &&
> + buffer_has_utf16_bom(s->data, s->size)) {
> + int outsz = 0;
> + char *outbuf;
> + outbuf = reencode_string_len(s->data, (int)s->size,
> +  "UTF-8", "UTF-16", );
> + if (outbuf) {
> + if (s->should_free)
> + free(s->data);
> + if (s->should_munmap)
> + munmap(s->data, s->size);
> + s->should_munmap = 0;
> + s->data = outbuf;
> + s->size = outsz;
> + s->reencoded_from_utf16 = 1;
> + s->should_free = 1;
> + }
> + }
> + } else {

I don't think it makes sense to do the conversion deep inside
diff_populate_filespec(), because it will kick in much more than you'd
want (e.g., "format-patch | am" would stop working with this patch, I
think).

I think you'd want to hook this in at the same level as fill_textconv().
In fact, one way to do it would be to have the get_textconv() stack just
fill in a special driver that does the auto-detection. This is similar
to my earlier patch, but it avoids overriding the user-facing config for
non-textconv things (and naturally any actual configured textconv filter
would override the auto-detection).

-Peff

Re: [PATCH v3 2/9] t3701: indent here documents

Phillip Wood  writes:

> From: Phillip Wood 
>
> Indent here documents in line with the current style for tests.
>
> Signed-off-by: Phillip Wood 
> ---

This loses the hand-edit-while-queuing done based on Eric's comment
for the previous round (see what has been queued on 'pu' for quite a
while), which is not very much appreciated.

Also you lost the title fix done while queuing on 6/9 (see 'pu').

Re: [PATCH v2] sha1_name: fix uninitialized memory errors

Jeff King  writes:

> Thanks, this looks good to me.
>
> Semi-related, I wondered also at the weird asymmetry in the if-else,
> ...
> So I think the code is right, but the comment is wrong.

-- >8 --
From: Derrick Stolee 
Date: Tue, 27 Feb 2018 06:47:04 -0500
Subject: [PATCH] sha1_name: fix uninitialized memory errors

During abbreviation checks, we navigate to the position within a
pack-index that an OID would be inserted and check surrounding OIDs
for the maximum matching prefix. This position may be beyond the
last position, because the given OID is lexicographically larger
than every OID in the pack. Then nth_packed_object_oid() does not
initialize "oid".

Use the return value of nth_packed_object_oid() to prevent these
errors.

Also the comment about checking near-by objects miscounts the
neighbours.  If we have a hit at "first", we check "first-1" and
"first+1" to make sure we have sufficiently long abbreviation not to
match either.  If we do not have a hit, "first" is the smallest
among the objects that are larger than what we want to name, so we
check that and "first-1" to make sure we have sufficiently long
abbreviation not to match either.  In either case, we only check up
to two near-by objects.

Reported-by: Christian Couder 
Signed-off-by: Derrick Stolee 
Signed-off-by: Junio C Hamano 
---
 sha1_name.c | 14 +++---
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/sha1_name.c b/sha1_name.c
index 05a635911b..f1c3d37a6d 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -542,20 +542,20 @@ static void find_abbrev_len_for_pack(struct packed_git *p,
/*
 * first is now the position in the packfile where we would insert
 * mad->hash if it does not exist (or the position of mad->hash if
-* it does exist). Hence, we consider a maximum of three objects
+* it does exist). Hence, we consider a maximum of two objects
 * nearby for the abbreviation length.
 */
mad->init_len = 0;
if (!match) {
-   nth_packed_object_oid(, p, first);
-   extend_abbrev_len(, mad);
+   if (nth_packed_object_oid(, p, first))
+   extend_abbrev_len(, mad);
} else if (first < num - 1) {
-   nth_packed_object_oid(, p, first + 1);
-   extend_abbrev_len(, mad);
+   if (nth_packed_object_oid(, p, first + 1))
+   extend_abbrev_len(, mad);
}
if (first > 0) {
-   nth_packed_object_oid(, p, first - 1);
-   extend_abbrev_len(, mad);
+   if (nth_packed_object_oid(, p, first - 1))
+   extend_abbrev_len(, mad);
}
mad->init_len = mad->cur_len;
 }
-- 
2.16.2-325-g2fc74f41c5

Re: [PATCH v7 0/7] convert: add support for different encodings

On Tue, Feb 27, 2018 at 02:10:20PM -0800, Junio C Hamano wrote:

> > I thought it solved that by the hosting folks never seeing the strange
> > binary-looking data. They see only utf8, which diffs well.
> 
> Ah, OK, that is a "fix" in a wider context (in a narrower context,
> "work around" is a more appropriate term ;-).
> 
> The reason why I have been nudging people toward considering in-repo
> encoding attribute is because forcing projects that already have
> their contents in a strange binary-looking encoding to switch is
> costly.  But perhaps having them pay one-time conversion pain is a
> better investment longer term.

Yeah, thanks for mentioning that. It should have gone in my "relative
merits" list. The conversion flag-day is definitely going to be a pain
for users, and doesn't help with diffing older versions.

-Peff

Re: [PATCH v3 14/35] connect: request remote refs using v2

On Tue, Feb 27, 2018 at 05:10:09PM -0500, Eric Sunshine wrote:

> > That would be fine with me. Though I would love it if we could find a
> > shorter name for the associated functions. For example,
> > argv_array_pushf() can make lines quite long, and something like
> > argv_pushf() is easier to read (in my opinion). And that might work
> > because "argv" is pretty unique by itself, but "string" is not.
> >
> > Some one-word name like "strarray" might work, though I find that is not
> > quite catchy. I guess "strv" is short if you assume that people know the
> > "v" suffix means "vector".
> 
> struct strs {...};
> 
> void strs_init(struct strs *);
> void strs_push(struct strs *, const char *);
> void strs_pushf(struct strs *, const char *fmt, ...);
> void strs_pushl(struct strs *, ...);
> void strs_pushv(struct strs *, const char **);
> void strs_pop(struct strs *);
> void strs_clear(struct strs *);
> const char **strs_detach(struct strs *);
> 
> ...is short, feels pretty natural, and doesn't require understanding
> "v" for "vector".

Not bad. The "v" carries the information that it _is_ a NULL-terminated
vector and not some other list-like structure (and so is suitable for
feeding to execv, etc). But that may just be obvious from looking at its
uses and documentation.

-Peff

Re: [PATCH 2/5] sequencer: always roll back lock in `do_recursive_merge()`

On Tue, Feb 27, 2018 at 11:08:12PM +0100, Martin Ågren wrote:

> > So I think it's correct as-is, but I wonder if writing it as:
> >
> >   if (!active_cache_changed)
> > rollback_lock_file(_lock);
> >   else if (write_locked_index(_index, _lock, COMMIT_LOCK))
> > return error(...);
> >
> > might be easier to follow. I'm OK with leaving it, too, but thought I'd
> > mention it in case it confused other reviewers.
> 
> I also hesitated at that one. There are some similar instances elsewhere, 
> e.g.,
> in builtin/merge.c. There's also rerere.c, which does a variant of your
> suggestion.

Hmm, yeah, grepping shows quite a few of various forms.

I wonder if it is worth a helper like:

  /* like write_locked_index(), but optimize out unchanged writes */
  static int maybe_write_locked_index(struct index *index,
  struct lock_file *lock,
  unsigned flags)
  {
if (!index->cached_changed) {
if (flags & COMMIT_LOCK)
rollback_lock_file(lock);
return 0;
}
return write_locked_index(index, lock, flags);
  }

Alternatively, it could just be a flag to write_locked_index() to enable
the optimization.

I actually suspect that most callers would prefer to have it kick in by
default (with an optional flag to disable it if some caller really needs
to), but that would possibly be a subtle breakage for the caller that
needs the flag.

-Peff

Re: [PATCH v7 0/7] convert: add support for different encodings

Jeff King  writes:

> On Tue, Feb 27, 2018 at 01:55:02PM -0800, Junio C Hamano wrote:
>
>> Jeff King  writes:
>> 
>> > Of the three solutions, I think the relative merits are something like
>> > this:
>> > ...
>> >   3. w-t-e (Lars's patch)
>> 
>> I thought Lars's w-t-e was about keeping the in-repo contents in
>> UTF-8 and externalize in whatever encoding (e.g. UTF-16), so it
>> won't help the issue hosting folks want to deal with, i.e. showing
>> in-repo data that is stored in a strange binary-looking encoding in
>> a more reasonable encodign while diffing, no?
>
> I thought it solved that by the hosting folks never seeing the strange
> binary-looking data. They see only utf8, which diffs well.

Ah, OK, that is a "fix" in a wider context (in a narrower context,
"work around" is a more appropriate term ;-).

The reason why I have been nudging people toward considering in-repo
encoding attribute is because forcing projects that already have
their contents in a strange binary-looking encoding to switch is
costly.  But perhaps having them pay one-time conversion pain is a
better investment longer term.

Re: [PATCH v3 14/35] connect: request remote refs using v2

2018-02-27 Thread Eric Sunshine

On Tue, Feb 27, 2018 at 5:04 PM, Jeff King  wrote:
> On Tue, Feb 27, 2018 at 01:58:00PM -0800, Junio C Hamano wrote:
>> So are we looking for a natural name to call an array of trings?  I
>> personally do not mind argv_array too much, but perhaps we can call
>> it a string_array and then everybody will be happy?
>
> That would be fine with me. Though I would love it if we could find a
> shorter name for the associated functions. For example,
> argv_array_pushf() can make lines quite long, and something like
> argv_pushf() is easier to read (in my opinion). And that might work
> because "argv" is pretty unique by itself, but "string" is not.
>
> Some one-word name like "strarray" might work, though I find that is not
> quite catchy. I guess "strv" is short if you assume that people know the
> "v" suffix means "vector".

struct strs {...};

void strs_init(struct strs *);
void strs_push(struct strs *, const char *);
void strs_pushf(struct strs *, const char *fmt, ...);
void strs_pushl(struct strs *, ...);
void strs_pushv(struct strs *, const char **);
void strs_pop(struct strs *);
void strs_clear(struct strs *);
const char **strs_detach(struct strs *);

...is short, feels pretty natural, and doesn't require understanding
"v" for "vector".

Re: [PATCH 2/5] sequencer: always roll back lock in `do_recursive_merge()`

On 27 February 2018 at 22:44, Jeff King  wrote:
> I want to note one thing that confused me while reviewing. While looking
> to see if there were other returns, I noticed that the lines right near
> the end of your context are funny:
>
> if (active_cache_changed &&
>   write_locked_index(_index, _lock, COMMIT_LOCK))
>   /*
>* TRANSLATORS: %s will be "revert", "cherry-pick" or
>* "rebase -i".
>*/
>   return error(_("%s: Unable to write new index file"),
>   _(action_name(opts)));
>   rollback_lock_file(_lock);
>
> At first I thought that rollback was a noop, since write_locked_index()
> would always either commit or rollback. But it's needed for the case
> when we active_cache_changed isn't true.
>
> So I think it's correct as-is, but I wonder if writing it as:
>
>   if (!active_cache_changed)
> rollback_lock_file(_lock);
>   else if (write_locked_index(_index, _lock, COMMIT_LOCK))
> return error(...);
>
> might be easier to follow. I'm OK with leaving it, too, but thought I'd
> mention it in case it confused other reviewers.

I also hesitated at that one. There are some similar instances elsewhere, e.g.,
in builtin/merge.c. There's also rerere.c, which does a variant of your
suggestion.

Martin

Re: [PATCH] test_must_be_empty: make sure the file exists, not just empty

On Tue, Feb 27, 2018 at 01:27:29PM -0800, Junio C Hamano wrote:

> The helper function test_must_be_empty is meant to make sure the
> given file is empty, but its implementation is:
> 
>   if test -s "$1"
>   then
>   ... not empty, we detected a failure ...
>   fi
> 
> Surely, the file having non-zero size is a sign that the condition
> "the file must be empty" is violated, but it misses the case where
> the file does not even exist.  It is an accident waiting to happen
> with a buggy test like this:
> 
>   git frotz 2>error-message &&
>   test_must_be_empty errro-message
> 
> that won't get caught until you deliberately break 'git frotz' and
> notice why the test does not fail.
> 
> Signed-off-by: Junio C Hamano 

This seems like a huge and obvious improvement to me. I'm amazed it
hasn't come up before (and that this doesn't reveal any existing typos
like the one you showed).

-Peff

Re: Is offloading to GPU a worthwhile feature?

On Tue, Feb 27, 2018 at 12:52 PM, Konstantin Ryabitsev
wrote:
> compression offload

Currently there is a series under review that introduces a commit graph
file[1], which would allow to not need decompressing objects for a rev walk, but
have the walking information as-needed on disk.

Once walking (as part of negotiation) is done,
we'd have to pack a pack file to return to the client,
which maybe can be improved by GPU acceleration[2].

Though once upon a time Junio had proposed to change
this part of the protocol as well. Instead of having a packfile
catered to a specific user/request, the server would store
multiple pack files, which are temporally sorted, e.g.
one "old" packfile containing everything that is roughly older
than 4 weeks ago, then a "medium pack file" that is up to last
weekend, and then a "new" pack that is just this weeks work,
followed by the on-demand pack that is just the latest
generated on the fly.

The server would just dump these different packfiles
(concatenated?) at the user, and would need to refresh
its packfiles occasionally every week or so.

[1]
https://public-inbox.org/git/1519698787-190494-1-git-send-email-dsto...@microsoft.com/
[2]
http://on-demand.gputechconf.com/gtc/2014/presentations/S4459-parallel-lossless-compression-using-gpus.pdf

> I realize this would be silly amounts of work. But, if it's worth it,
> perhaps we can benefit from all the GPU computation libs written for
> cryptocoin mining and use them for something good. :)

Currently there is work being done on "protocol v2"[3], which
is also motivated by the desire to have easy extensibility in the protocol,
so if you want to put in a cryptocoin-secret-handshake [into the protocol]
that improves the cost of compute or the bandwidth required for your
typical use case, it will be possible to do so with ease.

[3] https://public-inbox.org/git/20180207011312.189834-1-bmw...@google.com/

I wonder if the bitmap code can be sped up using GPUs. Every once in a while
the discussion floats up bloom filters or inverse bloom filters for
the negotiation
part, and a quick search shows that those could also be sped up using GPUs.

Stefan

Re: [PATCH 2/2] t5556: replace test_i18ngrep with a simple grep

Junio C Hamano  writes:

> OK, somehow I had the version from Ramsay on a topic branch that was
> not merged to 'pu'.  Here is the replacement for 2/2 I'd be queuing.
>
> We'd need SZEDER to sign it off (optionally correcting mistakes in
> the log message) if we are going with this solution.
>
> Thanks.

I guess I missed Ramsay's v2 which already did this

<550fb3f4-8d25-c5c4-0ecd-3a4e61ea1...@ramsayjones.plus.com>

so I'll use that version.  We still want sign-off from Szeder,
though.

Re: [PATCH v3 14/35] connect: request remote refs using v2

On Tue, Feb 27, 2018 at 01:58:00PM -0800, Junio C Hamano wrote:

> Jonathan Nieder  writes:
> 
> > Jonathan Tan wrote:
> >> On Thu, 22 Feb 2018 13:26:58 -0500
> >> Jeff King  wrote:
> >
> >>> I agree that it shouldn't matter much here. But if the name argv_array
> >>> is standing in the way of using it, I think we should consider giving it
> >>> a more general name. I picked that not to evoke "this must be arguments"
> >>> but "this is terminated by a single NULL".
> > [...]
> >> This sounds reasonable - I withdraw my comment about using struct
> >> string_list.
> >
> > Marking with #leftoverbits as a reminder to think about what such a
> > more general name would be (or what kind of docs to put in
> > argv-array.h) and make it so the next time I do a search for that
> > keyword.
> 
> So are we looking for a natural name to call an array of trings?  I
> personally do not mind argv_array too much, but perhaps we can call
> it a string_array and then everybody will be happy?

That would be fine with me. Though I would love it if we could find a
shorter name for the associated functions. For example,
argv_array_pushf() can make lines quite long, and something like
argv_pushf() is easier to read (in my opinion). And that might work
because "argv" is pretty unique by itself, but "string" is not.

Some one-word name like "strarray" might work, though I find that is not
quite catchy. I guess "strv" is short if you assume that people know the
"v" suffix means "vector".

It may not be worth worrying too much about, though. We already have
24-character monstrosities like string_list_append_nodup(). ;)

-Peff

Re: [PATCH v7 0/7] convert: add support for different encodings

On Tue, Feb 27, 2018 at 01:55:02PM -0800, Junio C Hamano wrote:

> Jeff King  writes:
> 
> > Of the three solutions, I think the relative merits are something like
> > this:
> > ...
> >   3. w-t-e (Lars's patch)
> 
> I thought Lars's w-t-e was about keeping the in-repo contents in
> UTF-8 and externalize in whatever encoding (e.g. UTF-16), so it
> won't help the issue hosting folks want to deal with, i.e. showing
> in-repo data that is stored in a strange binary-looking encoding in
> a more reasonable encodign while diffing, no?

I thought it solved that by the hosting folks never seeing the strange
binary-looking data. They see only utf8, which diffs well.

-Peff

Re: [PATCH v3 14/35] connect: request remote refs using v2

Jonathan Nieder  writes:

> Jonathan Tan wrote:
>> On Thu, 22 Feb 2018 13:26:58 -0500
>> Jeff King  wrote:
>
>>> I agree that it shouldn't matter much here. But if the name argv_array
>>> is standing in the way of using it, I think we should consider giving it
>>> a more general name. I picked that not to evoke "this must be arguments"
>>> but "this is terminated by a single NULL".
> [...]
>> This sounds reasonable - I withdraw my comment about using struct
>> string_list.
>
> Marking with #leftoverbits as a reminder to think about what such a
> more general name would be (or what kind of docs to put in
> argv-array.h) and make it so the next time I do a search for that
> keyword.

So are we looking for a natural name to call an array of trings?  I
personally do not mind argv_array too much, but perhaps we can call
it a string_array and then everybody will be happy?

Re: [PATCH v7 0/7] convert: add support for different encodings

Jeff King  writes:

> Of the three solutions, I think the relative merits are something like
> this:
> ...
>   3. w-t-e (Lars's patch)

I thought Lars's w-t-e was about keeping the in-repo contents in
UTF-8 and externalize in whatever encoding (e.g. UTF-16), so it
won't help the issue hosting folks want to deal with, i.e. showing
in-repo data that is stored in a strange binary-looking encoding in
a more reasonable encodign while diffing, no?

Usually we only work in-repo encoding when producing a diff and show
the result in in-repo encoding, but I can imagine a new attribute,
when set, we first convert in-repo to the specified encoding before
passing the result to xdiff machinery.  Then convert it back to
in-repo encoding before showing the diff (or just show the result in
that encoding xdiff machinery processed---I do not know which one
should be the default).

Re: [PATCH 0/5] roll back locks in various code paths

On Tue, Feb 27, 2018 at 10:30:08PM +0100, Martin Ågren wrote:

> Patches 2-4 are the actual fixes where I teach some functions to always
> roll back the lock they're holding. Notably, these are all in "libgit".
> 
> Patch 1 is a "while at it" to use locks on the stack instead of having
> them be static. Patch 5 removes code to roll back locks which are
> already rolled back.

These all look good to me. Happy to see us dropping the "static" from
more lockfiles, too.

-Peff

Re: [PATCH 2/5] sequencer: always roll back lock in `do_recursive_merge()`

On Tue, Feb 27, 2018 at 10:30:10PM +0100, Martin Ågren wrote:

> diff --git a/sequencer.c b/sequencer.c
> index 90807c4559..e6bac4692a 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -465,8 +465,10 @@ static int do_recursive_merge(struct commit *base, 
> struct commit *next,
>   fputs(o.obuf.buf, stdout);
>   strbuf_release();
>   diff_warn_rename_limit("merge.renamelimit", o.needed_rename_limit, 0);
> - if (clean < 0)
> + if (clean < 0) {
> + rollback_lock_file(_lock);
>   return clean;
> + }
>  
>   if (active_cache_changed &&
>   write_locked_index(_index, _lock, COMMIT_LOCK))

This addition is obviously correct.

I want to note one thing that confused me while reviewing. While looking
to see if there were other returns, I noticed that the lines right near
the end of your context are funny:

if (active_cache_changed &&
  write_locked_index(_index, _lock, COMMIT_LOCK))
  /*
   * TRANSLATORS: %s will be "revert", "cherry-pick" or
   * "rebase -i".
   */
  return error(_("%s: Unable to write new index file"),
  _(action_name(opts)));
  rollback_lock_file(_lock);

At first I thought that rollback was a noop, since write_locked_index()
would always either commit or rollback. But it's needed for the case
when we active_cache_changed isn't true.

So I think it's correct as-is, but I wonder if writing it as:

  if (!active_cache_changed)
rollback_lock_file(_lock);
  else if (write_locked_index(_index, _lock, COMMIT_LOCK))
return error(...);

might be easier to follow. I'm OK with leaving it, too, but thought I'd
mention it in case it confused other reviewers.

-Peff

Re: [PATCH] test_must_be_empty: make sure the file exists, not just empty

On Tue, Feb 27, 2018 at 1:27 PM, Junio C Hamano  wrote:
> The helper function test_must_be_empty is meant to make sure the
> given file is empty, but its implementation is:
>
> if test -s "$1"
> then
> ... not empty, we detected a failure ...
> fi
>
> Surely, the file having non-zero size is a sign that the condition
> "the file must be empty" is violated, but it misses the case where
> the file does not even exist.  It is an accident waiting to happen
> with a buggy test like this:
>
> git frotz 2>error-message &&
> test_must_be_empty errro-message
>
> that won't get caught until you deliberately break 'git frotz' and
> notice why the test does not fail.
>
> Signed-off-by: Junio C Hamano 

Reviewed-by: Stefan Beller

Re: [PATCH v2] sha1_name: fix uninitialized memory errors

On Tue, Feb 27, 2018 at 06:47:04AM -0500, Derrick Stolee wrote:

> Peff made an excellent point about the nested if statements. This
> goes back to Christian's original recommendation.
> 
> -- >8 --
> 
> During abbreviation checks, we navigate to the position within a
> pack-index that an OID would be inserted and check surrounding OIDs
> for the maximum matching prefix. This position may be beyond the
> last position, because the given OID is lexicographically larger
> than every OID in the pack. Then nth_packed_object_oid() does not
> initialize "oid".
> 
> Use the return value of nth_packed_object_oid() to prevent these
> errors.
> 
> Reported-by: Christian Couder 
> Signed-off-by: Derrick Stolee 

Thanks, this looks good to me.

Semi-related, I wondered also at the weird asymmetry in the if-else,
which is:

  if ...
  else if ...
  if ...

but the comment directly above says: "we consider a maximum of three
objects nearby". I think it's actually two, because you can only trigger
one of the first two conditionals.

Is that right?

Let's imagine we're looking for object 1234abcd.  If we didn't find a
match, then we might have:

  1234abcc 
  1234abce <-- first points here

in which case we need to check both first-1 and first. And we do.

If we do have a match, then we might see:

  1234abcc
  1234abcd <-- first points here
  1234abce

and we must check first-1 and first+1, but _not_ first.

So I think the code is right, but the comment is wrong.

-Peff

Re: [PATCH v5 01/12] sequencer: avoid using errno clobbered by rollback_lock_file()

On 26 February 2018 at 22:29, Johannes Schindelin
 wrote:
> As pointed out in a review of the `--recreate-merges` patch series,
> `rollback_lock_file()` clobbers errno. Therefore, we have to report the
> error message that uses errno before calling said function.
>
> Signed-off-by: Johannes Schindelin 
> ---
>  sequencer.c | 13 -
>  1 file changed, 8 insertions(+), 5 deletions(-)
>
> diff --git a/sequencer.c b/sequencer.c
> index e9baaf59bd9..5aa3dc3c95c 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -345,12 +345,14 @@ static int write_message(const void *buf, size_t len, 
> const char *filename,
> if (msg_fd < 0)
> return error_errno(_("could not lock '%s'"), filename);
> if (write_in_full(msg_fd, buf, len) < 0) {
> +   error_errno(_("could not write to '%s'"), filename);
> rollback_lock_file(_file);
> -   return error_errno(_("could not write to '%s'"), filename);
> +   return -1;
> }
> if (append_eol && write(msg_fd, "\n", 1) < 0) {
> +   error_errno(_("could not write eol to '%s'"), filename);
> rollback_lock_file(_file);
> -   return error_errno(_("could not write eol to '%s'"), 
> filename);
> +   return -1;
> }
> if (commit_lock_file(_file) < 0) {
> rollback_lock_file(_file);
> @@ -2106,16 +2108,17 @@ static int save_head(const char *head)
>
> fd = hold_lock_file_for_update(_lock, git_path_head_file(), 0);
> if (fd < 0) {
> +   error_errno(_("could not lock HEAD"));
> rollback_lock_file(_lock);
> -   return error_errno(_("could not lock HEAD"));
> +   return -1;
> }

I just noticed this when test-merging my series of lockfile-fixes to pu.
This `rollback_lock_file()` is not needed, since failure to take the
lock leaves it unlocked. If one wants to roll back the lock "for
clarity" or "just to be safe", then the same should arguably be done in
`write_message()`, just barely visible at the top of this diff.

Perhaps not worth a reroll. The conflict resolution between this and my
patch would be to take my hunk.

https://public-inbox.org/git/cover.1519763396.git.martin.ag...@gmail.com/T/#t

Martin

[PATCH 3/5] merge-recursive: always roll back lock in `merge_recursive_generic()`

If we return early, we forget to roll back the lockfile. Do so.

Signed-off-by: Martin Ågren 
---
 merge-recursive.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/merge-recursive.c b/merge-recursive.c
index cc5fa0a949..7345125691 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -2198,8 +2198,10 @@ int merge_recursive_generic(struct merge_options *o,
hold_locked_index(, LOCK_DIE_ON_ERROR);
clean = merge_recursive(o, head_commit, next_commit, ca,
result);
-   if (clean < 0)
+   if (clean < 0) {
+   rollback_lock_file();
return clean;
+   }
 
if (active_cache_changed &&
write_locked_index(_index, , COMMIT_LOCK))
-- 
2.16.2.246.ga4ee8f

[PATCH 5/5] sequencer: do not roll back lockfile unnecessarily

If `commit_lock_file()` or `hold_lock_file_for_update()` fail, there is
no need to call `rollback_lock_file()` on the lockfile. It doesn't hurt
either, but it does make different callers in this file inconsistent,
which might be confusing.

While at it, remove a trailing '.' from a recurring error message.

Signed-off-by: Martin Ågren 
---
 sequencer.c | 18 ++
 1 file changed, 6 insertions(+), 12 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index e6bac4692a..0e6d04e4ce 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -303,10 +303,8 @@ static int write_message(const void *buf, size_t len, 
const char *filename,
rollback_lock_file(_file);
return error_errno(_("could not write eol to '%s'"), filename);
}
-   if (commit_lock_file(_file) < 0) {
-   rollback_lock_file(_file);
-   return error(_("failed to finalize '%s'."), filename);
-   }
+   if (commit_lock_file(_file) < 0)
+   return error(_("failed to finalize '%s'"), filename);
 
return 0;
 }
@@ -1585,10 +1583,8 @@ static int save_head(const char *head)
ssize_t written;
 
fd = hold_lock_file_for_update(_lock, git_path_head_file(), 0);
-   if (fd < 0) {
-   rollback_lock_file(_lock);
+   if (fd < 0)
return error_errno(_("could not lock HEAD"));
-   }
strbuf_addf(, "%s\n", head);
written = write_in_full(fd, buf.buf, buf.len);
strbuf_release();
@@ -1597,10 +1593,8 @@ static int save_head(const char *head)
return error_errno(_("could not write to '%s'"),
   git_path_head_file());
}
-   if (commit_lock_file(_lock) < 0) {
-   rollback_lock_file(_lock);
-   return error(_("failed to finalize '%s'."), 
git_path_head_file());
-   }
+   if (commit_lock_file(_lock) < 0)
+   return error(_("failed to finalize '%s'"), 
git_path_head_file());
return 0;
 }
 
@@ -1724,7 +1718,7 @@ static int save_todo(struct todo_list *todo_list, struct 
replay_opts *opts)
todo_list->buf.len - offset) < 0)
return error_errno(_("could not write to '%s'"), todo_path);
if (commit_lock_file(_lock) < 0)
-   return error(_("failed to finalize '%s'."), todo_path);
+   return error(_("failed to finalize '%s'"), todo_path);
 
if (is_rebase_i(opts)) {
const char *done_path = rebase_path_done();
-- 
2.16.2.246.ga4ee8f

[PATCH 2/5] sequencer: always roll back lock in `do_recursive_merge()`

If we return early, we forget to roll back the lockfile. Do so.

Signed-off-by: Martin Ågren 
---
 sequencer.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/sequencer.c b/sequencer.c
index 90807c4559..e6bac4692a 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -465,8 +465,10 @@ static int do_recursive_merge(struct commit *base, struct 
commit *next,
fputs(o.obuf.buf, stdout);
strbuf_release();
diff_warn_rename_limit("merge.renamelimit", o.needed_rename_limit, 0);
-   if (clean < 0)
+   if (clean < 0) {
+   rollback_lock_file(_lock);
return clean;
+   }
 
if (active_cache_changed &&
write_locked_index(_index, _lock, COMMIT_LOCK))
-- 
2.16.2.246.ga4ee8f

[PATCH 4/5] merge: always roll back lock in `checkout_fast_forward()`

This function originated in builtin/merge.c. It was moved to merge.c in
commit db699a8a1f (Move try_merge_command and checkout_fast_forward to
libgit.a, 2012-10-26), but was used from sequencer.c even before that.

If a problem occurs, the function returns without rolling back the
lockfile. Teach it to do so.

Signed-off-by: Martin Ågren 
---
 merge.c | 12 +---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/merge.c b/merge.c
index 195b578700..f06a4773d4 100644
--- a/merge.c
+++ b/merge.c
@@ -113,17 +113,23 @@ int checkout_fast_forward(const struct object_id *head,
setup_unpack_trees_porcelain(, "merge");
 
trees[nr_trees] = parse_tree_indirect(head);
-   if (!trees[nr_trees++])
+   if (!trees[nr_trees++]) {
+   rollback_lock_file(_file);
return -1;
+   }
trees[nr_trees] = parse_tree_indirect(remote);
-   if (!trees[nr_trees++])
+   if (!trees[nr_trees++]) {
+   rollback_lock_file(_file);
return -1;
+   }
for (i = 0; i < nr_trees; i++) {
parse_tree(trees[i]);
init_tree_desc(t+i, trees[i]->buffer, trees[i]->size);
}
-   if (unpack_trees(nr_trees, t, ))
+   if (unpack_trees(nr_trees, t, )) {
+   rollback_lock_file(_file);
return -1;
+   }
if (write_locked_index(_index, _file, COMMIT_LOCK))
return error(_("unable to write new index file"));
return 0;
-- 
2.16.2.246.ga4ee8f

[PATCH 1/5] sequencer: make lockfiles non-static

After 076aa2cbd (tempfile: auto-allocate tempfiles on heap, 2017-09-05),
we can have lockfiles on the stack.

One of these functions fails to always roll back the lock. That will be
fixed in the next commit.

Signed-off-by: Martin Ågren 
---
 sequencer.c | 10 +-
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index 4d3f60594c..90807c4559 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -290,7 +290,7 @@ static void print_advice(int show_hint, struct replay_opts 
*opts)
 static int write_message(const void *buf, size_t len, const char *filename,
 int append_eol)
 {
-   static struct lock_file msg_file;
+   struct lock_file msg_file = LOCK_INIT;
 
int msg_fd = hold_lock_file_for_update(_file, filename, 0);
if (msg_fd < 0)
@@ -436,7 +436,7 @@ static int do_recursive_merge(struct commit *base, struct 
commit *next,
struct tree *result, *next_tree, *base_tree, *head_tree;
int clean;
char **xopt;
-   static struct lock_file index_lock;
+   struct lock_file index_lock = LOCK_INIT;
 
if (hold_locked_index(_lock, LOCK_REPORT_ON_ERROR) < 0)
return -1;
@@ -1183,7 +1183,7 @@ static int prepare_revs(struct replay_opts *opts)
 
 static int read_and_refresh_cache(struct replay_opts *opts)
 {
-   static struct lock_file index_lock;
+   struct lock_file index_lock = LOCK_INIT;
int index_fd = hold_locked_index(_lock, 0);
if (read_index_preload(_index, NULL) < 0) {
rollback_lock_file(_lock);
@@ -1577,7 +1577,7 @@ static int create_seq_dir(void)
 
 static int save_head(const char *head)
 {
-   static struct lock_file head_lock;
+   struct lock_file head_lock = LOCK_INIT;
struct strbuf buf = STRBUF_INIT;
int fd;
ssize_t written;
@@ -1702,7 +1702,7 @@ int sequencer_rollback(struct replay_opts *opts)
 
 static int save_todo(struct todo_list *todo_list, struct replay_opts *opts)
 {
-   static struct lock_file todo_lock;
+   struct lock_file todo_lock = LOCK_INIT;
const char *todo_path = get_todo_path(opts);
int next = todo_list->current, offset, fd;
 
-- 
2.16.2.246.ga4ee8f

[PATCH 0/5] roll back locks in various code paths

Patches 2-4 are the actual fixes where I teach some functions to always
roll back the lock they're holding. Notably, these are all in "libgit".

Patch 1 is a "while at it" to use locks on the stack instead of having
them be static. Patch 5 removes code to roll back locks which are
already rolled back.

I've based this on maint. There's a conflict on pu, with c7d4394111
(sequencer: avoid using errno clobbered by rollback_lock_file(),
2018-02-11). The conflict resolution would be to take my version for the
"could not lock HEAD"-hunk.

Martin

Martin Ågren (5):
  sequencer: make lockfiles non-static
  sequencer: always roll back lock in `do_recursive_merge()`
  merge-recursive: always roll back lock in `merge_recursive_generic()`
  merge: always roll back lock in `checkout_fast_forward()`
  sequencer: do not roll back lockfile unnecessarily

 merge-recursive.c |  4 +++-
 merge.c   | 12 +---
 sequencer.c   | 32 ++--
 3 files changed, 26 insertions(+), 22 deletions(-)

-- 
2.16.2.246.ga4ee8f

Re: [PATCH 04/11] t3030-merge-recursive: don't check the stderr of a subshell

On Tue, Feb 27, 2018 at 10:10 PM, Junio C Hamano  wrote:
> Junio C Hamano  writes:
>
>> SZEDER Gábor  writes:
>>
>>> The two test checking 'git mmerge-recursive' in an empty worktree in
>>> ...
>>>  GIT_INDEX_FILE="$PWD/ours-has-rename-index" &&
>>>  export GIT_INDEX_FILE &&
>>>  mkdir "$GIT_WORK_TREE" &&
>>> -git read-tree -i -m $c7 &&
>>> -git update-index --ignore-missing --refresh &&
>>> -git merge-recursive $c0 -- $c7 $c3 &&
>>> -git ls-files -s >actual-files
>>> -) 2>actual-err &&
>>> ->expected-err &&
>>> +git read-tree -i -m $c7 2>actual-err &&
>>> +test_must_be_empty expected-err &&
>>> +git update-index --ignore-missing --refresh 2>actual-err &&
>>> +test_must_be_empty expected-err &&
>>> +git merge-recursive $c0 -- $c7 $c3 2>actual-err &&
>>> +test_must_be_empty expected-err &&
>>> +git ls-files -s >actual-files 2>actual-err &&
>>> +test_must_be_empty expected-err
>>
>> Where do the contents of all of these expected-err files come from?
>> Should all of the test_must_be_empty checks be checking actual-err
>> instead?

Ugh, I messed that up.

> And the reason why your pre-submission testing did not catch may be
> because test_must_be_empty is broken?  I wonder if this is a good
> way forward to catch a possible bug like this.

Yeah.  'test -s file' means "exists and has a size greater than zero",
so the missing file doesn't trigger the error code path.

> Of course, if somebody was using the helepr for "must be either
> missing or empty", this change will break it, but I somehow doubt
> it.

FWIW, I just run the test suite with this change added, and there were
no failures.  I think it's a good change.

>  A program that creates/opens and writes an error message only
> when an error is detected is certainly possible, and could be tested
> with the current test_must_be_empty this way:
>
> rm -f actual-err &&
> git frotz --error-to=actual-err &&
> test_must_be_empty actual-err
>
> but then the last step in such a test like the above is more natural
> to check if actual-err _exists_ in the first place anyway, so...
>
>  t/test-lib-functions.sh | 6 +-
>  1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
> index 37eb34044a..6cfbee60e4 100644
> --- a/t/test-lib-functions.sh
> +++ b/t/test-lib-functions.sh
> @@ -772,7 +772,11 @@ verbose () {
>  # otherwise.
>
>  test_must_be_empty () {
> -   if test -s "$1"
> +   if ! test -f "$1"
> +   then
> +   echo "'$1' is missing"
> +   return 1
> +   elif test -s "$1"
> then
> echo "'$1' is not empty, it contains:"
> cat "$1"

[PATCH] test_must_be_empty: make sure the file exists, not just empty

The helper function test_must_be_empty is meant to make sure the
given file is empty, but its implementation is:

if test -s "$1"
then
... not empty, we detected a failure ...
fi

Surely, the file having non-zero size is a sign that the condition
"the file must be empty" is violated, but it misses the case where
the file does not even exist.  It is an accident waiting to happen
with a buggy test like this:

git frotz 2>error-message &&
test_must_be_empty errro-message

that won't get caught until you deliberately break 'git frotz' and
notice why the test does not fail.

Signed-off-by: Junio C Hamano 
---
 t/test-lib-functions.sh | 6 +-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index 37eb34044a..6cfbee60e4 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -772,7 +772,11 @@ verbose () {
 # otherwise.
 
 test_must_be_empty () {
-   if test -s "$1"
+   if ! test -f "$1"
+   then
+   echo "'$1' is missing"
+   return 1
+   elif test -s "$1"
then
echo "'$1' is not empty, it contains:"
cat "$1"
-- 
2.16.2-325-g2fc74f41c5

Re: [PATCH v7 0/7] convert: add support for different encodings

On Tue, Feb 27, 2018 at 10:05:17PM +0100, Torsten Bögershausen wrote:

> The other question is:
> Would this help showing diffs of UTF-16 encoded files on a "git hoster",
> github/bitbucket/ ?

Almost. There's probably one more thing needed. We don't currently read
in-tree .gitattributes when doing a diff in a bare repository. And most
hosting sites will store bare repositories.

And of course it would require the users to actually set the attributes
themselves.

> Or would the auto-magic UTF-16 avoid binary patch that I send out be more 
> helpful ?
> Or both ?
> Or the w-t-e encoding ?

Of the three solutions, I think the relative merits are something like
this:

  1. baked-in textconv (my patch)

 - reuses an existing diff feature, so minimal code and not likely to
   break things

 - requires people to add a .gitattributes entry

 - needs work to make bare-repo .gitattributes work (though I think
   this would be useful for other features, too)

 - has a run-time cost at each diff to do the conversion

 - may sometimes annoy people when it doesn't kick in (e.g.,
   emailed patches from format-patch won't have a readable diff)

 - doesn't combine with other custom-diff config (e.g., utf-16
   storing C code should still use diff=c funcname rules, but
   wouldn't with my patch)

  2. auto-detect utf-16 (your patch)
 - Just Works for existing repositories storing utf-16

 - carries some risk of kicking in when people would like it not to
   (e.g., when they really do want a binary patch that can be
   applied).

   I think it would probably be OK if this kicked in only when
   ALLOW_TEXTCONV is set (the default for porcelain), and --binary
   is not (i.e., when we would otherwise just say "binary
   files differ").

 - similar to (1), carries a run-time cost for each diff, and users
   may sometimes still see binary diffs

  3. w-t-e (Lars's patch)

 - requires no server-side modifications; the diff is plain vanilla

 - works everywhere you diff, plumbing and porcelain

 - does require people to add a .gitattributes entry

 - run-time cost is per-checkout, not per-diff

So I can see room for (3) to co-exist alongside the others. Between (1)
and (2), I think (2) is probably the better direction.

-Peff

[PATCH v8 30/29] fixup! merge-recursive: apply necessary modifications for directory renames

2018-02-27 Thread Elijah Newren

Use is_null_oid() instead of is_null_sha1()
---
This is just a fixup to patch 23/29 in my v8 series for detecting directory 
renames;
should squash cleanly.

 merge-recursive.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/merge-recursive.c b/merge-recursive.c
index ffe1d0d117..6e6ec90e9e 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -667,9 +667,9 @@ static int update_stages_for_stage_data(struct 
merge_options *opt,
oidcpy(, _data->stages[3].oid);
 
return update_stages(opt, path,
-is_null_sha1(o.oid.hash) ? NULL : ,
-is_null_sha1(a.oid.hash) ? NULL : ,
-is_null_sha1(b.oid.hash) ? NULL : );
+is_null_oid() ? NULL : ,
+is_null_oid() ? NULL : ,
+is_null_oid() ? NULL : );
 }
 
 static void update_entry(struct stage_data *entry,
-- 
2.16.1.232.gbf538760f8

Re: [PATCH 04/11] t3030-merge-recursive: don't check the stderr of a subshell