date:20171122

On Thu, Nov 23, 2017 at 11:41:25AM +0900, Junio C Hamano wrote:

> Jeff King  writes:
> 
> > Right, I came to the same conclusion (we may even have discussed this on
> > the list, I don't remember). The current "todo" format says that only
> > the command and sha1 matter, and we'd be changing that. Maybe that's not
> > so bad if the user has to enable the feature themselves (and clearly it
> > would be incompatible with a custom format option).
> 
> If you are in the habit of always writing 4 or more lines, the
> chance that you would make a typo on the line you can correct in the
> "todo" list with such a feature is at most 25% ;-).

You'd think, but the distribution of typos doesn't seem to be uniform.
:)

I think what actually happens is that I quite often do my final
proofread in my MUA, where the subject is displayed apart from the rest
of the message. So I tend to gloss over it, and any errors there are
more likely to make it to the list.

> I think one downside is that a mere presence of such an option hints
> that we somehow encourage people to commit with a title-only message.

Yeah, though it might be handy for me I think it just opens up too many
funny questions like this.

-Peff

Re: [PATCH 0/3] Introduce BUG_ON(cond, msg) MACRO

On Thu, Nov 23, 2017 at 10:38:07AM +0900, Junio C Hamano wrote:

> > FWIW I think we've done fine at using assert so far.  But if I
> > understand correctly, the point of this series is to stop having to
> > worry about it.
> 
> I recalled that there was at least one, and "log -Sassert" piped to
> "less" that looks for "/^[ ^I]*assert\(" caught me one recent one.
> 
> 08414938 ("mailinfo.c: move side-effects outside of assert", 2016-12-19)

Thanks, I forgot about that one. There's some discussion about NDEBUG in
the surrounding thread if anybody is interested:

https://public-inbox.org/git/900a55073f78a9f19daca67e468d334@3c843fe6ba8f3c586a21345a2783aa0/

(but it's long and there's really no resolution, so you may want to skip
it).

> Even though I do not personally mind
> 
>   assert(flags & EXPECTED_BIT);
>   assert(remaining_doshes == 0);
> 
> left as a reminder primarily for coders, we can do just as well do
> so with
> 
>   if (remaining_doshes != 0)
>   BUG("the gostak did not distim all doshes???");

Yeah, agreed. The reason I do not mind the assert() form is that if you
have nothing useful to say in the BUG() sentence, it's a bit more
compact.

> So I am fine if we want to move to reduce the use of assert()s or
> get rid of them.  I personally prefer (like Peff, if I am not
> mistaken) an explicit use of the usual control structure, as it is
> easy to follow.

To clarify my position, I think BUG_ON(cond, msg) from this series
provides basically no value over "if (cond) BUG(msg)". But I could see
value in "BUG_ON(cond)" that allows the compact form but doesn't respect
NDEBUG.

-Peff

Re: [PATCH] defer expensive load_ref_decorations until needed

On Thu, Nov 23, 2017 at 11:19:42AM +0900, Junio C Hamano wrote:

> Jeff King  writes:
> 
> > And lazy-load wouldn't help you there for a normal:
> >
> >   git log
> >
> > But what's interesting in your command is the pretty-format. Even though
> > decoration is turned on, your format doesn't show any. So we never
> > actually ask "is this commit decorated" and the lazy-load helps.
> 
> Hmph, I wonder if we can detect this case and not make a call to
> load decorations in the first place.  That would remove the need to
> remember the options when load is called so that we can use it when
> we load decorations lazily later.

Probably userformat_find_requirements() could (and should) be taught to
report on decoration flags, like it does for notes. As it is now we call
load_ref_decorations() repeatedly while processing the commits (which
works because it's a noop after the first call).

And once we can do that, it would be easy to do something like:

  if (decoration_style && !want->decorations)
decoration_style = 0;

But I think that may only be part of the story. Do all of the output
formats show decorations? I think --format=email, for instance, does
not.

So the real question is not just "does the user format want
decorations", but "does the pretty format want decorations". Which
requires knowing things about each format that might get out of sync
with the rest of the code. That might be OK if it lives in pretty.c. But
the lazy-load thing make sit just work without having to duplicate the
logic at all.

-Peff

Re: [PATCH] xdiff/xpatience: support anchoring line(s)

Junio C Hamano  writes:

> What do we do when
>
> git diff --histogram --patience
>
> is given?  Do we warn?  If we don't, perhaps it may not be too bad
> if 
>
> git diff --histogram --patience-anchor=foo
> git diff --patience-anchor=foo --histogram
>
> did not get any warning.  Instead we just implicitly turn any
> occurence of --patience-anchor=foo into --patience followed by the
> same option, and assume that the user wanted the usual "last one
> wins" semantics.  It would turn patience on for the former, and
> ignore the anchor for the latter and use historgram.

Thinking about this a bit more, I do like the basic idea of the UI
even better.  What we could do is to sell this to the end users as a
new kind of diff algorithm choice (i.e. myers, patience, ... will
gain a new friend) that internally happens to be implemented by
piggybacking on patience (just like minimal is piggybacking on
myers) and call it "anchor".  Then just like this command line

git diff --histogram --patience

makes the last one win without complaint, it is sane that these
command lines

git diff --histogram --anchored=
git diff --anchored= --histogram

make the last one win without complaint, either.

Hmm?

Re: [PATCH 1/5] p5550: factor our nonsense-pack creation

Jeff King  writes:

> Right, I came to the same conclusion (we may even have discussed this on
> the list, I don't remember). The current "todo" format says that only
> the command and sha1 matter, and we'd be changing that. Maybe that's not
> so bad if the user has to enable the feature themselves (and clearly it
> would be incompatible with a custom format option).

If you are in the habit of always writing 4 or more lines, the
chance that you would make a typo on the line you can correct in the
"todo" list with such a feature is at most 25% ;-).

I think one downside is that a mere presence of such an option hints
that we somehow encourage people to commit with a title-only message.

Re: [PATCH 5/5] sha1_file: don't re-scan pack directory for null sha1

Jeff King  writes:

> So there are four cases we care about for this call in fetch:
>
>   1. We fed a real sha1 and got a commit (or peeled to one).
>
>   2. We fed a real sha1 which resolved to a non-commit, and we got NULL.
>
>   3. We fed a real sha1 and the object was missing or corrupted, and we
>  got NULL.
>
>   4. We fed a null sha1 and got NULL.
>
> Right now we lump cases 2-4 together as "do not do a fast-forward
> check". That's fine for 2 and 4, but probably not for 3. We can easily
> catch case 4 ourselves (if we care to), but distinguishing case 3 from
> the others is hard. How should lookup_commit_reference_gently() signal
> it to us?

Not limiting us to the caller in the "fetch" codepath, I think the
expectation by callers of lookup_commit_reference_gently() in the
ideal world would be:

 - It has an object name, and wants to use it as point in the commit
   DAG to define the traversal over the DAG, if it refers to a
   commit known to us.

 - It does not know if these object names represent a tag object, a
   commit object, or some other object.  It does not know if the
   local repository actually has them (e.g. we received a "have"
   from the other side---missing is expected).

 - Hence, it would happily accept a NULL as "we do not have it" and
   "we do have it, but it is not a commit-ish".

And from that point of view, 2, 3a (missing), and 4 (0{40}) to yield
NULL is perfectly fine.  3b (exists but broken) may be a noteworthy
event, but for the purpose of the caller, it may want to proceed as
if the object is missing from our end, so it might deserve warning()
but not die(), at least as the default behaviour.

Re: [PATCH v2] gitcli: tweak "man gitcli" for clarity

Jeff King  writes:

> I read the rest of the thread, and I think the question here is not
> about Git's behavior, but about parsing this sentence.
>
> Without a "--" Git can sometimes do what you want. Or it may error out,
> if what you asked for is ambiguous. And that sentence is trying to cover
> those cases separately, and the "can" only applies to the ambiguous
> case.
>
> It's pretty clear to me as it is, but maybe we can write it differently.
> Like:
>
>   Without a disambiguating `--`, Git makes a reasonable guess. If it
>   cannot guess (because your request is ambiguous), then it will error
>   out.

Splitting it into two sentences like you did makes it even clearer,
I would think.

Re: [PATCH] defer expensive load_ref_decorations until needed

Jeff King  writes:

> And lazy-load wouldn't help you there for a normal:
>
>   git log
>
> But what's interesting in your command is the pretty-format. Even though
> decoration is turned on, your format doesn't show any. So we never
> actually ask "is this commit decorated" and the lazy-load helps.

Hmph, I wonder if we can detect this case and not make a call to
load decorations in the first place.  That would remove the need to
remember the options when load is called so that we can use it when
we load decorations lazily later.

Re: [PATCH] xdiff/xpatience: support anchoring line(s)

Junio C Hamano  writes:

> Jonathan Tan  writes:
>
>> One thing that might help is to warn if --anchor is used without
>> --patience, but I couldn't find a good place to put that warning. Let me
>> know if you know of a good place.
>
> How about dropping --anchor option and do it as "--patience="?

Well, that may not be an optimal suggestion, as it is always a pain
to have to deal with an option with optional argument.

I understand that the case you would really want to warn against is

git diff --histogram --anchor=foo

and not

git diff --anchor=foo

as it is sensible to turn --patience on implicitly.  Perhaps a good
starting point might be to rename the option to include "patience"
somewhere in its name ("--patience-anchor="?) to make it
more obvious that it is about helping the patience algorithm.  And
then make "--patience-anchor=" without any other algorithm
selection option to behave as if "--patience" was also given.

What do we do when

git diff --histogram --patience

is given?  Do we warn?  If we don't, perhaps it may not be too bad
if 

git diff --histogram --patience-anchor=foo
git diff --patience-anchor=foo --histogram

did not get any warning.  Instead we just implicitly turn any
occurence of --patience-anchor=foo into --patience followed by the
same option, and assume that the user wanted the usual "last one
wins" semantics.  It would turn patience on for the former, and
ignore the anchor for the latter and use historgram.

Re: [PATCH] xdiff/xpatience: support anchoring line(s)

Jonathan Tan  writes:

> One thing that might help is to warn if --anchor is used without
> --patience, but I couldn't find a good place to put that warning. Let me
> know if you know of a good place.

How about dropping --anchor option and do it as "--patience="?

Re: [PATCH 0/3] Introduce BUG_ON(cond, msg) MACRO

Jonathan Nieder  writes:

> FWIW I think we've done fine at using assert so far.  But if I
> understand correctly, the point of this series is to stop having to
> worry about it.

I recalled that there was at least one, and "log -Sassert" piped to
"less" that looks for "/^[ ^I]*assert\(" caught me one recent one.

08414938 ("mailinfo.c: move side-effects outside of assert", 2016-12-19)

Even though I do not personally mind

assert(flags & EXPECTED_BIT);
assert(remaining_doshes == 0);

left as a reminder primarily for coders, we can do just as well do
so with

if (remaining_doshes != 0)
BUG("the gostak did not distim all doshes???");

So I am fine if we want to move to reduce the use of assert()s or
get rid of them.  I personally prefer (like Peff, if I am not
mistaken) an explicit use of the usual control structure, as it is
easy to follow.  BUG_ON() would become another thing readers need to
get used to, if we were to use it, and my gut feeling is that it may
not be worth it.

A few more random things related to this topic that comes to my
mind:

 - If we had a good set of tools to tell us if an expression is free
   of side-effects, then assert() would be less
   problematic---we could mechanically check if an assert() that is
   left as a reminder for coders/readers is safe.

 - Even if we had such a check, using the check only on new changes
   when a patch is accepted is not good enough.  An assert(distim())
   may have been safe back when it was added because distim() used
   to be free of side-effects, but a later update to it may add side
   effects to it.

 - The issue that is caused by "this function used to be pure but
   lately it gained side-effects" is not limited to assert().  Using
   it in "if (condition) BUG(...)" or "BUG_ON(condition,...)" will
   not sidestep the fact that such a change will alter behaviour of
   callers of the function.  It's just that assert(condition) is
   conditionally compiled, which makes the issue a worse one.

[ANNOUNCE] Bug tracker for Git

Hi,

As discussed at [1], I've occasionally wanted to have a place to keep
track of bugs I'm working on in Git.  Some kind people on the Chromium
project helped me set an issue tracker up, so now we have one.

 https://crbug.com/git

Feel free to file bugs, feature requests, and leftover bits there.
I'll be happy to triage them to keep the list of bugs meaningful.
Anyone else wanting to help with bug management can feel free to
contact me and I'll grant you permissions to edit issues.

This particular implementation of an issue tracker is Monorail
.
It is similar to the issue tracker that used to run at
code.google.com.  If you find things you don't like about the way it
works, they accept patches. :)

It accepts replies to the emails it sends.

This uses Google accounts to authenticate people filing a bug.  If you
would like a Google account for your existing email address, you can
get one at https://accounts.google.com/SignUpWithoutGmail.

There is an API for programmatic access to the issue tracker: [2].
One thing I would appreciate help with is using that API to make
regular backups.  The chromium project has their own backups using
database dumps but I would be happier if multiple people are making
backups independently.

This is an experiment --- I don't know whether it will stick.  The
configuration will likely change as we get experience with it.  I
expect to be using it to track my own work for the forseeable future.
I'd be happy if it is useful to others as well.

Thanks,
Jonathan

[1] 
https://public-inbox.org/git/20170919160753.ga75...@aiede.mtv.corp.google.com/
https://public-inbox.org/git/vpqiovpubmh@anie.imag.fr/
https://public-inbox.org/git/7vhay9tqs6@alter.siamese.dyndns.org/
https://public-inbox.org/git/7vehzjugdz@alter.siamese.dyndns.org/
https://public-inbox.org/git/4a1fb1de.3070...@op5.se/
[2] 
https://chromium.googlesource.com/infra/infra/+/master/appengine/monorail/doc/api.md

Re: [PATCH] xdiff/xpatience: support anchoring line(s)

On Wed, Nov 22, 2017 at 3:41 PM, Jonathan Tan  wrote:
> Teach the patience diff to attempt preventing user-specified lines from
> appearing as a deletion or addition in the end result. The end user can
> use this by specifying "--anchor=" one or more times when using
> Git commands like "diff" and "show".
>
> Signed-off-by: Jonathan Tan 
> ---
> Actual patch instead of RFC.
>
> One thing that might help is to warn if --anchor is used without
> --patience, but I couldn't find a good place to put that warning. Let me
> know if you know of a good place.

Would it make sense to have `--anchor` imply patience?
(not necessarily in this patch, might be a "yes, let's do
it in a year when users complain")

> Replying to Stefan's and Junio's comments:
>
>> The solution you provide is a good thing to experiment with, but
>> longer term, I would want to have huge record of configs in which
>> humans selected the best diff, such that we can use that data
>> to reason about better automatic diff generation.
>> The diff heuristic was based on a lot of human generated data,
>> that was generated by Michael at the time. I wonder if we want to
>> permanently store the anchor so the data collection will happen
>> automatically over time.
>
> I think machine learning is beyond the scope of this patch :-)

agreed; I just wanted to share what I think we could do in the future
to select sane default. For that we'd want to collect some "most useful"
configurations.

When I proposed separate flags for the move detection regarding
ignoring whitespaces, the question "how is the user sanely select
from so many flags?" came up. And in that spirit I would want think
adding this rather fundamental flag, and then machine learn (e.g. the
weights in traversing the diff matrix) off of this collected data later
might be a viable approach.

>> or rather: "c is not moved, we don't care how the diff actually looks
>> like",
>> so maybe
>>   ! grep "+c" diff
>
> I think it's less error-prone to show "a" moving. With this, if the
> command somehow prints nothing, the test would still pass.

Makes sense.

> diff --git a/t/t4033-diff-patience.sh b/t/t4033-diff-patience.sh
> index 113304dc5..2d00d1056 100755
> --- a/t/t4033-diff-patience.sh
> +++ b/t/t4033-diff-patience.sh

I was waiting for

test_expect_success 'one --anchor anchors many lines' '
printf "a\nb\na\nc\na\n" >file && # many 'a's

--anchor=a
...

Thanks for writing this patch,
I hope we can make use of this addition eventually a lot. :)

Stefan

Re: [PATCH 0/3] Introduce BUG_ON(cond, msg) MACRO

On Wed, Nov 22, 2017 at 04:08:39PM -0800, Jonathan Nieder wrote:

> > For the record, I'm totally fine with banning assert() in favor of a
> > custom equivalent. I just don't think we've seen any real problems with
> > assert in our codebase so far.
> 
> It sounds like we basically agree, then. :)

Yeah, I didn't quite understand your argument at first. If it's that
asserts are potentially dangerous, I buy that. They haven't bit us, but
they could.

That would be a good thing for Stefan to put in his commit message. ;)

-Peff

Re: [PATCH 0/3] Introduce BUG_ON(cond, msg) MACRO

Jeff King wrote:
> On Wed, Nov 22, 2017 at 03:45:32PM -0800, Jonathan Nieder wrote:

>> It lets you build with NDEBUG.
>
> But why do you want to build with NDEBUG if nothing uses assert()? ;)

No idea, but some distros (not Debian) have done it before and I don't
want to be burned again.

> I'm being a little glib, but I think there really is a chicken-and-egg
> question here. Why are people building with NDEBUG if they don't want to
> disable asserts?

It's because they want to disable asserts.

The semantics of assert is that they're allowed to be compiled out.  A
person setting NDEBUG is perfectly within their rights to assume that
the author of a program has followed those semantics.  Of course this
makes assert a terrible API from the point of view of Rusty Russel's
API rating scheme
 but
it's what C gives us.

FWIW I think we've done fine at using assert so far.  But if I
understand correctly, the point of this series is to stop having to
worry about it.

[...]
>> It also goes through our own die()
>> handler, which means that e.g. the error message gets propagated over
>> remote transports.
>
> BUG() doesn't go through our die handler. It hits vreportf(), which does
> do some minor munging, but it always goes to stderr. Error message
> propagation over the remote protocol happens either:
>
>   1. From a parent process muxing our stderr onto the sideband.
>
>   2. Via special wrappers like receive-pack's rp_error().
>
> The only program I know that sets a custom die handler to change the
> reporting is git-http-backend (and there it only applies to die("BUG"),
> not BUG(), so with the latter you'd get a hard hangup instead of an
> attempt at an http 500).

Ah, interesting.  That could be worth changing, though I don't think
it's too important.

[...]
> Yes, obviously side effects in an assert() are horrible. But I haven't
> noticed that being a problem in our code base.
>
> For the record, I'm totally fine with banning assert() in favor of a
> custom equivalent. I just don't think we've seen any real problems with
> assert in our codebase so far.

It sounds like we basically agree, then. :)

Jonathan

Re: [PATCH v2] gitcli: tweak "man gitcli" for clarity

On Tue, Nov 21, 2017 at 10:45:52PM +0100, Kevin Daudt wrote:

> > - * Without disambiguating `--`, Git makes a reasonable guess, but errors
> > -   out and asking you to disambiguate when ambiguous.  E.g. if you have a
> > + * Without a disambiguating `--`, Git makes a reasonable guess, but can
> > +   error out, asking you to disambiguate when ambiguous.  E.g. if you have 
> > a
> 
> 'Can' error out implies that it sometimes would not error out when there
> is ambiguity. Are there situation where git does not error out in that
> case?

I read the rest of the thread, and I think the question here is not
about Git's behavior, but about parsing this sentence.

Without a "--" Git can sometimes do what you want. Or it may error out,
if what you asked for is ambiguous. And that sentence is trying to cover
those cases separately, and the "can" only applies to the ambiguous
case.

It's pretty clear to me as it is, but maybe we can write it differently.
Like:

  Without a disambiguating `--`, Git makes a reasonable guess. If it
  cannot guess (because your request is ambiguous), then it will error
  out.

-Peff

Re: [PATCH 0/3] Introduce BUG_ON(cond, msg) MACRO

On Wed, Nov 22, 2017 at 03:45:32PM -0800, Jonathan Nieder wrote:

> Jeff King wrote:
> 
> > Yes. I'd be fine having a single-argument BUG_ON() like that. But then,
> > I'm not sure what it's buying us over assert().
> 
> It lets you build with NDEBUG.

But why do you want to build with NDEBUG if nothing uses assert()? ;)

I'm being a little glib, but I think there really is a chicken-and-egg
question here. Why are people building with NDEBUG if they don't want to
disable asserts? Do they do it out of habit? A misguided sense of
performance optimization? Does it do something else I'm not aware of?

> It also goes through our own die()
> handler, which means that e.g. the error message gets propagated over
> remote transports.

BUG() doesn't go through our die handler. It hits vreportf(), which does
do some minor munging, but it always goes to stderr. Error message
propagation over the remote protocol happens either:

  1. From a parent process muxing our stderr onto the sideband.

  2. Via special wrappers like receive-pack's rp_error().

The only program I know that sets a custom die handler to change the
reporting is git-http-backend (and there it only applies to die("BUG"),
not BUG(), so with the latter you'd get a hard hangup instead of an
attempt at an http 500).

> Please please please, don't rely on side-effects from assert().  They
> will cause me to run into pain over time.  This issue alone might be
> worth banning use of assert() in the codebase, if we can't trust
> reviewers to catch problematic examples (fortunately reviewers have
> been pretty good about that so far, but banning it would free up their
> attention to focus on other aspects of a patch).

Yes, obviously side effects in an assert() are horrible. But I haven't
noticed that being a problem in our code base.

For the record, I'm totally fine with banning assert() in favor of a
custom equivalent. I just don't think we've seen any real problems with
assert in our codebase so far.

-Peff

Re: [PATCH 1/3] Documentation/CodingGuidelines: explain why assert is bad

Stefan Beller wrote:
> On Wed, Nov 22, 2017 at 2:59 PM, Jonathan Nieder  wrote:

>> In a certain ideal world, the preference would be reversed: you'd want
>> to use assert() wherever you can and require the compiler to check
>> that all assert()s are verifiable at compile time.  A check that a
>> static analyzer can verify is more valuable than a run-time check.
>> When a compile-time check is not possible, you'd have to fall back to
>> BUG_ON().
>
> Linux has BUILT_BUG_ON as well, which we may desire?

I thought so until I tried to use it:
https://public-inbox.org/git/20170830065631.gh153...@aiede.mtv.corp.google.com/

So I think we are stuck with run-time checking for now.

Thanks,
Jonathan

Re: [PATCH] defer expensive load_ref_decorations until needed

On Wed, Nov 22, 2017 at 03:21:06PM -0800, Phil Hord wrote:

> Hm. I think I was confused.
> 
> I wrote v1 of this patch a few months ago. Clearly I was wrong about
> rev-parse being afflicted.  We have a script that was suffering and it
> uses both "git log --format=%h" and "git rev-parse" to get hashes; I
> remember testing both, but I can't find it in my $zsh_history; my
> memory and my commit-message must be faulty.

OK, that makes more sense (that log would see it).

> However, "git log" does not need any --decorate option to trigger this lag.
> 
> $ git for-each-ref| wc -l
> 24172
> $ time git log --format=%h -1
> git log --format=%h -1   0.47s user 0.04s system 99% cpu 0.509 total
>
> I grepped the code just now, too, and I see the same as you, though;
> it seems to hold off unless !!decoration_style.  Nevertheless, gdb
> shows me decoration_style=1 with this command:
> 
> GIT_CONFIG=/dev/null cgdb --args git log -1 --format="%h"
> 

Right, the default these days is "auto decorate", so it's enabled if
your output is to a terminal. So "git log --no-decorate" should be cheap
again (or you may want to set log.decorate=false in your config).

And lazy-load wouldn't help you there for a normal:

  git log

But what's interesting in your command is the pretty-format. Even though
decoration is turned on, your format doesn't show any. So we never
actually ask "is this commit decorated" and the lazy-load helps.

So I think your patch is doing the right thing, but the explanation
should probably cover that it is really helping non-decorating formats.

> Here are timing tests on this repo without this change:
> 
> git log --format=%h -1 0.54s user 0.05s system 99% cpu
> 0.597 total
> git log --format=%h -1 --decorate  0.54s user 0.04s system 98% cpu
> 0.590 total
> git log --format=%h%d -1   0.53s user 0.05s system 99% cpu
> 0.578 total
> 
> And the same commands with this change:
> 
> git log --format=%h -1  0.01s user 0.01s system 71%
> cpu 0.017 total
> git log --format=%h -1 --decorate   0.00s user 0.01s system 92%
> cpu 0.009 total
> git log --format=%h%d -10.53s user 0.09s system 88%
> cpu 0.699 total

Yeah, that's consistent with what I'd expect.

> > I have definitely seen "rev-parse HEAD" be O(# of refs), but that is
> > mostly attributable to having all the refs packed (and until v2.15.0,
> > the packed-refs code would read the whole file into memory).
> 
> Hm.  Could this be why rev-parse was slow for me?  My original problem
> showed up on v1.9 (build machine) and I patched it on v2.14.0-rc1.
> But, no; testing on 1.9, 2.11 and 2.14 still doesn't show me the lag
> in rev-parse.  I remain befuddled.

Doing "rev-parse HEAD" would still have to load the packed refs if the
thing that HEAD points to is in there. Perhaps your current HEAD is
detached, or you have a loose ref for the current branch? Try "git
pack-refs --all --prune" and then re-time.

-Peff

Re: [PATCH 0/3] Introduce BUG_ON(cond, msg) MACRO

Jeff King wrote:

> Yes. I'd be fine having a single-argument BUG_ON() like that. But then,
> I'm not sure what it's buying us over assert().

It lets you build with NDEBUG.  It also goes through our own die()
handler, which means that e.g. the error message gets propagated over
remote transports.

Please please please, don't rely on side-effects from assert().  They
will cause me to run into pain over time.  This issue alone might be
worth banning use of assert() in the codebase, if we can't trust
reviewers to catch problematic examples (fortunately reviewers have
been pretty good about that so far, but banning it would free up their
attention to focus on other aspects of a patch).

Jonathan

[PATCH] xdiff/xpatience: support anchoring line(s)

2017-11-22 Thread Jonathan Tan

Teach the patience diff to attempt preventing user-specified lines from
appearing as a deletion or addition in the end result. The end user can
use this by specifying "--anchor=" one or more times when using
Git commands like "diff" and "show".

Signed-off-by: Jonathan Tan 
---
Actual patch instead of RFC.

One thing that might help is to warn if --anchor is used without
--patience, but I couldn't find a good place to put that warning. Let me
know if you know of a good place.

Replying to Stefan's and Junio's comments:

> The solution you provide is a good thing to experiment with, but
> longer term, I would want to have huge record of configs in which
> humans selected the best diff, such that we can use that data
> to reason about better automatic diff generation.
> The diff heuristic was based on a lot of human generated data,
> that was generated by Michael at the time. I wonder if we want to
> permanently store the anchor so the data collection will happen
> automatically over time.

I think machine learning is beyond the scope of this patch :-)

> or rather: "c is not moved, we don't care how the diff actually looks
> like",
> so maybe
>   ! grep "+c" diff

I think it's less error-prone to show "a" moving. With this, if the
command somehow prints nothing, the test would still pass.

> While this is RFC, I should not expect comments, though it would
> be nice to have them in the final series. ;-)

Added comments :-)

> This is a natural extension of the idea the patience algorithm is
> built upon.  If this were a cumulative command line option that can
> be given to specify multiple lines and can be used across the diff
> family, it would make a welcome addition, I would think.

Specifying multiple lines - done.

By diff family, do you mean "diff", "show", and others? If yes, that has
also been done in this patch.
---
 Documentation/diff-options.txt |  9 +++
 diff.c |  8 ++
 diff.h |  4 +++
 t/t4033-diff-patience.sh   | 59 ++
 xdiff/xdiff.h  |  4 +++
 xdiff/xpatience.c  | 42 ++
 6 files changed, 121 insertions(+), 5 deletions(-)

diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt
index dd0dba5b1..b17d62696 100644
--- a/Documentation/diff-options.txt
+++ b/Documentation/diff-options.txt
@@ -100,6 +100,15 @@ For instance, if you configured diff.algorithm variable to 
a
 non-default value and want to use the default one, then you
 have to use `--diff-algorithm=default` option.
 
+--anchor=::
+   This option may be specified more than once.
++
+If the "patience diff" algorithm is used, Git attempts to prevent lines
+starting with this text from appearing as a deletion or addition in the
+output.
++
+If another diff algorithm is used, this has no effect.
+
 --stat[=[,[,]]]::
Generate a diffstat. By default, as much space as necessary
will be used for the filename part, and the rest for the graph
diff --git a/diff.c b/diff.c
index 0763e8926..29a7176ee 100644
--- a/diff.c
+++ b/diff.c
@@ -3210,6 +3210,8 @@ static void builtin_diff(const char *name_a,
ecbdata.opt = o;
ecbdata.header = header.len ?  : NULL;
xpp.flags = o->xdl_opts;
+   xpp.anchors = o->anchors;
+   xpp.anchors_nr = o->anchors_nr;
xecfg.ctxlen = o->context;
xecfg.interhunkctxlen = o->interhunkcontext;
xecfg.flags = XDL_EMIT_FUNCNAMES;
@@ -3302,6 +3304,8 @@ static void builtin_diffstat(const char *name_a, const 
char *name_b,
memset(, 0, sizeof(xpp));
memset(, 0, sizeof(xecfg));
xpp.flags = o->xdl_opts;
+   xpp.anchors = o->anchors;
+   xpp.anchors_nr = o->anchors_nr;
xecfg.ctxlen = o->context;
xecfg.interhunkctxlen = o->interhunkcontext;
if (xdi_diff_outf(, , diffstat_consume, diffstat,
@@ -4608,6 +4612,10 @@ int diff_opt_parse(struct diff_options *options,
options->xdl_opts &= ~XDF_DIFF_ALGORITHM_MASK;
options->xdl_opts |= value;
return argcount;
+   } else if (skip_prefix(arg, "--anchor=", )) {
+   ALLOC_GROW(options->anchors, options->anchors_nr + 1,
+  options->anchors_alloc);
+   options->anchors[options->anchors_nr++] = xstrdup(arg);
}
 
/* flags options */
diff --git a/diff.h b/diff.h
index 0fb18dd73..7cf276f07 100644
--- a/diff.h
+++ b/diff.h
@@ -166,6 +166,10 @@ struct diff_options {
const char *stat_sep;
long xdl_opts;
 
+   /* see Documentation/diff-options.txt */
+   char **anchors;
+   size_t anchors_nr, anchors_alloc;
+
int stat_width;
int stat_name_width;
int stat_graph_width;
diff --git

Re: [PATCH v3 5/5] Testing: provide tests requiring them with ellipses after SHA-1 values

2017-11-22 Thread Philip Oakley

From: "Junio C Hamano" 

"Philip Oakley"  writes:

From: "Junio C Hamano" 

Ann T Ropea  writes:

*1* We are being overly generous in t4013-diff-various.sh because we do
not want to destroy/take apart the here-document.  Given that all this 
a

temporary measure, we should get away with it.

So, the need to reformat the test for the future post-deprecation
period is being deferred to the time that the PRINT_SHA1_ELLIPSIS env
variable, and all ellipis, is removed - is that the case? Maybe it
just needs saying plainly.

And if we say it that way, it is clear that with this series, we are
shipping a new feature with a test that does not protect the output
format we claim to be the improved and preferred one.  That sounds
quite bad.

Having said that, I have already queued this to 'pu' and I do not
terribly mind to merge it down to 'next', leaving the test updates
to cover the new output format as well as the backward compatible
one at the same time for a later follow-up patch.

I'd agree. I just wanted to ensure that I had the right understanding.

I'd however hate it if I have to carry the topic in the current
shape in 'next' forever, waiting for such an update to come, that
may never materialize, and be forced to do it myself without being
explicitly asked by (and thanked for) anybody, especially because
this is not exactly my itch X-<.

True.

Or is the env variable being retained as a fallback 'forever'? I'm
half guessing that it may tend toward the latter as it's an easier
backward compatibility decision.

We do not know until this change is released to the wild, at which
time we will hear noises about the lack of expected ellipses their
(poorly written) scripts rely on and tell them to set the workaround
environment variable.  We may not hear from such people at all, in
which case we may be able to remove it within a year or so, but it
is too early to tell.

I was wondering if there should be a small documentation change for the env 
variable and states that it is a temporary measure for short term 
compatibility. Though I'm not sure where the 'right' place would be for it.

Re: [PATCH 0/3] Introduce BUG_ON(cond, msg) MACRO

On Wed, Nov 22, 2017 at 03:28:14PM -0800, Jonathan Nieder wrote:

> > I do like human readable messages. But sometimes such a message just
> > makes the code harder to read (and to write). E.g., is there any real
> > value in:
> >
> >   BUG_ON(!foo, "called bar() with a foo!");
> >
> > over:
> >
> >   assert(foo);
> 
> I think you're hinting at wanting
> 
>   BUG_ON(!foo);
> 
> which is something that the Linux kernel has (and which is not done in
> this series).

Yes. I'd be fine having a single-argument BUG_ON() like that. But then,
I'm not sure what it's buying us over assert().

I get why the kernel cannot use the default "dump core and exit"
behavior of assert(), but that's basically what our BUG() does.

-Peff

Re: [PATCH 2/3] git-compat: introduce BUG_ON(condition, fmt, ...) macro

On Wed, Nov 22, 2017 at 03:02:39PM -0800, Jonathan Nieder wrote:

> > --- a/git-compat-util.h
> > +++ b/git-compat-util.h
> > @@ -1092,9 +1092,13 @@ static inline int regexec_buf(const regex_t *preg, 
> > const char *buf, size_t size,
> >  __attribute__((format (printf, 3, 4))) NORETURN
> >  void BUG_fl(const char *file, int line, const char *fmt, ...);
> >  #define BUG(...) BUG_fl(__FILE__, __LINE__, __VA_ARGS__)
> > +#define BUG_ON(condition, ...) do { if (condition) BUG(__VA_ARGS__); } 
> > while (0)
> >  #else
> >  __attribute__((format (printf, 1, 2))) NORETURN
> >  void BUG(const char *fmt, ...);
> > +
> > +__attribute__((format (printf, 2, 3)))
> > +void BUG_ON(int condition, const char *fmt, ...);
> >  #endif
> 
> I worry that these definitions are mildly incompatible: the macro
> accepts anything that can go in an 'if', including pointers, and the
> function only accepts an int.

I suspect this would cause real latent problems. Doing:

  const char *foo = NULL;

  ...
  BUG_ON(foo, "foo was set twice!");
  foo = xstrdup(bar);

would compile fine on modern systems, but issue a pointer-to-int
conversion warning if you didn't have variadic macros. So most git devs
would be unlikely to see it, until the trap springs on some poor soul
building on ancient AIX or something.

On the other hand, I wouldn't be surprised if our friends on less-abled
compilers see a lot of pointless warnings anyway, and almost certainly
can't deal with the equivalent of "-Werror".

I'm also not sure which compilers that even encompasses these days.
Maybe we should add variadic macros to our list of weather-balloon c99
features to test for.

> Is there a way for the macro to typecheck that its argument is an
> integer to avoid that?

I couldn't think of one.

It would be nice to just "!!condition" in the non-macro case, but of
course we can't do that automatically without a macro.

You're asking for the opposite: force the macro version to require an
int, so that the caller has to remember to do "!!condition". I can't
think of a way to do that, but I'm also not sure I like the direction,
as it pushes the effort into the callsite.

-Peff

Re: [PATCH 0/3] Introduce BUG_ON(cond, msg) MACRO

Hi,

Jeff King wrote:
> On Wed, Nov 22, 2017 at 02:38:24PM -0800, Stefan Beller wrote:

>> On reviewing [1] I wondered why there are so many asserts and wondered
>> if these asserts could have been prevented by a better functionality around
>> bug reporting in our code.
>>
>> Introduce a BUG_ON macro, which is superior to assert() by
>>  * being always there, even when compiled with NDEBUG and
>>  * providind an additional human readable error message, like BUG()
>
> I'm not sure I agree with the aim of the series.
>
> If people want to compile with NDEBUG, that's their business, I guess.
> I don't see much _point_ in it for Git, since most of our assertions do
> not respect NDEBUG, and I don't think we tend to assert in expensive
> ways anyway.
>
> I do like human readable messages. But sometimes such a message just
> makes the code harder to read (and to write). E.g., is there any real
> value in:
>
>   BUG_ON(!foo, "called bar() with a foo!");
>
> over:
>
>   assert(foo);

I think you're hinting at wanting

BUG_ON(!foo);

which is something that the Linux kernel has (and which is not done in
this series).

[...]
> I also find (as your third patch switches):
>
>   if (!foo)
>   BUG("foo has not been setup");
>
> more readable than the BUG_ON() version, if only because it uses
> traditional control flow.

Yes, I think you're right.

Thanks,
Jonathan

Re: [PATCH 0/3] Introduce BUG_ON(cond, msg) MACRO

On Wed, Nov 22, 2017 at 02:38:24PM -0800, Stefan Beller wrote:

> On reviewing [1] I wondered why there are so many asserts and wondered
> if these asserts could have been prevented by a better functionality around
> bug reporting in our code.
> 
> Introduce a BUG_ON macro, which is superior to assert() by
>  * being always there, even when compiled with NDEBUG and
>  * providind an additional human readable error message, like BUG()

I'm not sure I agree with the aim of the series.

If people want to compile with NDEBUG, that's their business, I guess.
I don't see much _point_ in it for Git, since most of our assertions do
not respect NDEBUG, and I don't think we tend to assert in expensive
ways anyway.

I do like human readable messages. But sometimes such a message just
makes the code harder to read (and to write). E.g., is there any real
value in:

  BUG_ON(!foo, "called bar() with a foo!");

over:

  assert(foo);

? The error message you'd get from the latter is rather sparse, but the
file and line number information it contains should be enough to find
the original source line. And after all, it's not _supposed_ to happen,
so if it does you're likely going to need to dig into the source anyway.

The human-readable BUG messages I find useful add some context or
summarize the situation. E.g. (pulled from random grepping):

  BUG: color parsing ran out of space

is way better than:

  assert failed: len < 2

Likewise, in this code:

  if (hashmap_put(map, alloc_ref_store_hash_entry(name, refs)))
die("BUG: %s ref_store '%s' initialized twice", type, name);

we get a lot of extra information:

  - the type is mentioned
  - the name variable is dereferenced
  - the implication of "initialized twice" is made clear by the author,
which would not be immediately obvious just from seeing the failed
call

So I _like_ good messages, but I also think a lot of assertions don't
really lend themselves to good messages. And we should shoot for just
making them easy to read and write.

I also find (as your third patch switches):

  if (!foo)
BUG("foo has not been setup");

more readable than the BUG_ON() version, if only because it uses
traditional control flow. But that may just be because I'm used to it.
I'm sure kernel folks are used to BUG_ON() at this point, and we'd grow
used to it, too.

-Peff

Re: [PATCH] defer expensive load_ref_decorations until needed

On Wed, Nov 22, 2017 at 1:27 PM, Jeff King  wrote:
> On Tue, Nov 21, 2017 at 03:43:36PM -0800, Phil Hord wrote:
>
>> With many thousands of references, a simple `git rev-parse HEAD` may take
>> more than a second to return because it first loads all the refs into
>> memory even though it will never use them.
>
> The overall goal of lazy-loading seems reasonable, but I'm slightly
> confused: how and why does "git rev-parse HEAD" load ref decorations?
>
> Grepping around I find that we mostly load them only when appropriate
> (when the "log" family sees a decorate option, when we see %d/%D in a
> pretty format, or with --simplify-by-decoration in a traversal). And
> poking at "rev-parse HEAD" in gdb seems to confirm that it does not hit
> that function.

Hm. I think I was confused.

I wrote v1 of this patch a few months ago. Clearly I was wrong about
rev-parse being afflicted.  We have a script that was suffering and it
uses both "git log --format=%h" and "git rev-parse" to get hashes; I
remember testing both, but I can't find it in my $zsh_history; my
memory and my commit-message must be faulty.

However, "git log" does not need any --decorate option to trigger this lag.

$ git for-each-ref| wc -l
24172
$ time git log --format=%h -1
git log --format=%h -1   0.47s user 0.04s system 99% cpu 0.509 total

I grepped the code just now, too, and I see the same as you, though;
it seems to hold off unless !!decoration_style.  Nevertheless, gdb
shows me decoration_style=1 with this command:

GIT_CONFIG=/dev/null cgdb --args git log -1 --format="%h"

Here are timing tests on this repo without this change:

git log --format=%h -1 0.54s user 0.05s system 99% cpu
0.597 total
git log --format=%h -1 --decorate  0.54s user 0.04s system 98% cpu
0.590 total
git log --format=%h%d -1   0.53s user 0.05s system 99% cpu
0.578 total

And the same commands with this change:

git log --format=%h -1  0.01s user 0.01s system 71%
cpu 0.017 total
git log --format=%h -1 --decorate   0.00s user 0.01s system 92%
cpu 0.009 total
git log --format=%h%d -10.53s user 0.09s system 88%
cpu 0.699 total

> I have definitely seen "rev-parse HEAD" be O(# of refs), but that is
> mostly attributable to having all the refs packed (and until v2.15.0,
> the packed-refs code would read the whole file into memory).

Hm.  Could this be why rev-parse was slow for me?  My original problem
showed up on v1.9 (build machine) and I patched it on v2.14.0-rc1.
But, no; testing on 1.9, 2.11 and 2.14 still doesn't show me the lag
in rev-parse.  I remain befuddled.

> I've also
> seen unnecessary ref lookups due to replace refs (we load al of the
> packed refs to find out that no, there's nothing in refs/replace).

I haven't seen this in the code, but I have had refs/replace hacks in
the past. Is that enough to wake this up?

Phil

Re: [PATCH 1/3] Documentation/CodingGuidelines: explain why assert is bad

On Wed, Nov 22, 2017 at 2:59 PM, Jonathan Nieder  wrote:

> In a certain ideal world, the preference would be reversed: you'd want
> to use assert() wherever you can and require the compiler to check
> that all assert()s are verifiable at compile time.  A check that a
> static analyzer can verify is more valuable than a run-time check.
> When a compile-time check is not possible, you'd have to fall back to
> BUG_ON().

Linux has BUILT_BUG_ON as well, which we may desire?

Re: [PATCH 2/3] git-compat: introduce BUG_ON(condition, fmt, ...) macro

Hi,

Stefan Beller wrote:

> --- a/git-compat-util.h
> +++ b/git-compat-util.h
> @@ -1092,9 +1092,13 @@ static inline int regexec_buf(const regex_t *preg, 
> const char *buf, size_t size,
>  __attribute__((format (printf, 3, 4))) NORETURN
>  void BUG_fl(const char *file, int line, const char *fmt, ...);
>  #define BUG(...) BUG_fl(__FILE__, __LINE__, __VA_ARGS__)
> +#define BUG_ON(condition, ...) do { if (condition) BUG(__VA_ARGS__); } while 
> (0)
>  #else
>  __attribute__((format (printf, 1, 2))) NORETURN
>  void BUG(const char *fmt, ...);
> +
> +__attribute__((format (printf, 2, 3)))
> +void BUG_ON(int condition, const char *fmt, ...);
>  #endif

I worry that these definitions are mildly incompatible: the macro
accepts anything that can go in an 'if', including pointers, and the
function only accepts an int.

Is there a way for the macro to typecheck that its argument is an
integer to avoid that?

Thanks,
Jonathan

Re: [PATCH 1/3] Documentation/CodingGuidelines: explain why assert is bad

Hi,

Stefan Beller wrote:

> --- a/Documentation/CodingGuidelines
> +++ b/Documentation/CodingGuidelines
> @@ -386,6 +386,9 @@ For C programs:
>   - Use Git's gettext wrappers to make the user interface
> translatable. See "Marking strings for translation" in po/README.
>  
> + - Prefer the BUG() macro over asserts, as asserts requires that the
> +   NDEBUG flag is unset on compilation to work.

nit: is there some logical place in the list of C guidelines this
should go at, instead of the last item?  Maybe near the top, since
this is one of those straightforward cases and we're just saying that
this is the startegy for asserting invariants that this project
prefers.

Separately: I am not sure we currently universally prefer BUG_ON()
over assert().  In theory, assert() is fine wherever you don't care
whether the assertion is checked at run-time --- in other words, it is
a fancy form of comment.  BUG_ON() is useful for defensive checks that
you *expect* to never trip but want to rely on afterwards.

In a certain ideal world, the preference would be reversed: you'd want
to use assert() wherever you can and require the compiler to check
that all assert()s are verifiable at compile time.  A check that a
static analyzer can verify is more valuable than a run-time check.
When a compile-time check is not possible, you'd have to fall back to
BUG_ON().

But we don't live in that ideal world.  I'm not aware of any widely
available tools for checking assert()s.  So maybe it makes sense to
forbid assert() in our codebase entirely.

That could be enforced using something like check-non-portable-shell.pl,
except

- run on C files instead of sh files
- run on Git's source code instead of t/

What do you think?

Thanks,
Jonathan

Re: [PATCH v5 4/6] list-objects: filter objects in traverse_commit_list

On Tue, Nov 21, 2017 at 12:58 PM, Jeff Hostetler  wrote:
> +   assert(arg);
> +   assert(!unset);

I count 16 asserts in this patch. Is that really needed?
Either omit them or use BUG if we want to rely on user
bug reports when these conditions trigger, as assert is unreliable
due to its dependence on the NDEBUG flag.

Stefan

[PATCH 3/3] contrib/coccinelle: convert all conditional bugs to bug_on

Add a coccinelle patch that detects a conditional BUG and converts it to
BUG_ON.  Also apply that semantic patch.

Signed-off-by: Stefan Beller 
---
 builtin/merge.c |  3 +--
 contrib/coccinelle/bug_on.cocci |  8 
 environment.c   | 22 ++
 notes.c |  9 +
 refs.c  |  7 +++
 refs/files-backend.c| 14 ++
 refs/packed-backend.c   | 13 +
 sha1_file.c |  4 ++--
 tempfile.c  | 34 --
 9 files changed, 56 insertions(+), 58 deletions(-)
 create mode 100644 contrib/coccinelle/bug_on.cocci

diff --git a/builtin/merge.c b/builtin/merge.c
index 612dd7bfb6..df5884b4c1 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -770,8 +770,7 @@ static void prepare_to_commit(struct commit_list 
*remoteheads)
struct strbuf msg = STRBUF_INIT;
strbuf_addbuf(, _msg);
strbuf_addch(, '\n');
-   if (squash)
-   BUG("the control must not reach here under --squash");
+   BUG_ON(squash, "the control must not reach here under --squash");
if (0 < option_edit)
strbuf_commented_addf(, _(merge_editor_comment), 
comment_line_char);
if (signoff)
diff --git a/contrib/coccinelle/bug_on.cocci b/contrib/coccinelle/bug_on.cocci
new file mode 100644
index 00..e778d19e3c
--- /dev/null
+++ b/contrib/coccinelle/bug_on.cocci
@@ -0,0 +1,8 @@
+@@
+expression E;
+@@
+- if (E)
+-  BUG(
++ BUG_ON(E,
+   ...);
+
diff --git a/environment.c b/environment.c
index 8fa032f307..9ba01a85ec 100644
--- a/environment.c
+++ b/environment.c
@@ -177,22 +177,20 @@ int have_git_dir(void)
 
 const char *get_git_dir(void)
 {
-   if (!the_repository->gitdir)
-   BUG("git environment hasn't been setup");
+   BUG_ON(!the_repository->gitdir, "git environment hasn't been setup");
return the_repository->gitdir;
 }
 
 const char *get_git_common_dir(void)
 {
-   if (!the_repository->commondir)
-   BUG("git environment hasn't been setup");
+   BUG_ON(!the_repository->commondir,
+  "git environment hasn't been setup");
return the_repository->commondir;
 }
 
 const char *get_git_namespace(void)
 {
-   if (!namespace)
-   BUG("git environment hasn't been setup");
+   BUG_ON(!namespace, "git environment hasn't been setup");
return namespace;
 }
 
@@ -242,8 +240,8 @@ const char *get_git_work_tree(void)
 
 char *get_object_directory(void)
 {
-   if (!the_repository->objectdir)
-   BUG("git environment hasn't been setup");
+   BUG_ON(!the_repository->objectdir,
+  "git environment hasn't been setup");
return the_repository->objectdir;
 }
 
@@ -282,15 +280,15 @@ int odb_pack_keep(const char *name)
 
 char *get_index_file(void)
 {
-   if (!the_repository->index_file)
-   BUG("git environment hasn't been setup");
+   BUG_ON(!the_repository->index_file,
+  "git environment hasn't been setup");
return the_repository->index_file;
 }
 
 char *get_graft_file(void)
 {
-   if (!the_repository->graft_file)
-   BUG("git environment hasn't been setup");
+   BUG_ON(!the_repository->graft_file,
+  "git environment hasn't been setup");
return the_repository->graft_file;
 }
 
diff --git a/notes.c b/notes.c
index c7f21fae44..cae8fa0657 100644
--- a/notes.c
+++ b/notes.c
@@ -400,10 +400,11 @@ static void load_subtree(struct notes_tree *t, struct 
leaf_node *subtree,
 oid_to_hex(>val_oid));
 
prefix_len = subtree->key_oid.hash[KEY_INDEX];
-   if (prefix_len >= GIT_SHA1_RAWSZ)
-   BUG("prefix_len (%"PRIuMAX") is out of range", 
(uintmax_t)prefix_len);
-   if (prefix_len * 2 < n)
-   BUG("prefix_len (%"PRIuMAX") is too small", 
(uintmax_t)prefix_len);
+   BUG_ON(prefix_len >= GIT_SHA1_RAWSZ,
+  "prefix_len (%"PRIuMAX") is out of range",
+  (uintmax_t)prefix_len);
+   BUG_ON(prefix_len * 2 < n, "prefix_len (%"PRIuMAX") is too small",
+  (uintmax_t)prefix_len);
memcpy(object_oid.hash, subtree->key_oid.hash, prefix_len);
while (tree_entry(, )) {
unsigned char type;
diff --git a/refs.c b/refs.c
index 339d4318ee..1a5473fbe3 100644
--- a/refs.c
+++ b/refs.c
@@ -937,8 +937,8 @@ int ref_transaction_update(struct ref_transaction 
*transaction,
return -1;
}
 
-   if (flags & ~REF_TRANSACTION_UPDATE_ALLOWED_FLAGS)
-   BUG("illegal flags 0x%x passed to ref_transaction_update()", 
flags);
+   BUG_ON(flags & ~REF_TRANSACTION_UPDATE_ALLOWED_FLAGS,
+  "illegal flags 0x%x passed to ref_transaction_update()", flags);
 
flags |= (new_oid ? REF_HAVE_NEW : 0) | (old_oid ? REF_HAVE_OLD : 0);

[PATCH 0/3] Introduce BUG_ON(cond, msg) MACRO

On reviewing [1] I wondered why there are so many asserts and wondered
if these asserts could have been prevented by a better functionality around
bug reporting in our code.

Introduce a BUG_ON macro, which is superior to assert() by
 * being always there, even when compiled with NDEBUG and
 * providind an additional human readable error message, like BUG()
 
Opinions?

Thanks,
Stefan

[1] https://public-inbox.org/git/20171121205852.15731-5-...@jeffhostetler.com/

Stefan Beller (3):
  Documentation/CodingGuidelines: explain why assert is bad
  git-compat: introduce BUG_ON(condition, fmt, ...) macro
  contrib/coccinelle: convert all conditional bugs to bug_on

 Documentation/CodingGuidelines  |  3 +++
 builtin/merge.c |  3 +--
 contrib/coccinelle/bug_on.cocci |  8 
 environment.c   | 22 ++
 git-compat-util.h   |  4 
 notes.c |  9 +
 refs.c  |  7 +++
 refs/files-backend.c| 14 ++
 refs/packed-backend.c   | 13 +
 sha1_file.c |  4 ++--
 tempfile.c  | 34 --
 usage.c | 12 +++-
 12 files changed, 74 insertions(+), 59 deletions(-)
 create mode 100644 contrib/coccinelle/bug_on.cocci

-- 
2.15.0.448.gf294e3d99a-goog

[PATCH 2/3] git-compat: introduce BUG_ON(condition, fmt, ...) macro

A lot of BUG() invocations are in the form of
if (condition)
BUG()
so moving the condition into the same line as the macro will result in
more readable code.  The conversion to use this MACRO will happen in a
later patch.

This macro in name and spirit is borrowed from linux, which defines it as

#define BUG_ON(condition) do { if (unlikely(condition)) BUG(); } while (0)

in include/asm-generic/bug.h, however Git prefers to have a more specific
message in BUG() calls that we include as well.

In case the user doesn't have HAVE_VARIADIC_MACROS, I could not come up
with some MACRO trickery to transport the message down to BUG conditionally
such that BUG_ON is a function just like BUG() as well.

Signed-off-by: Stefan Beller 
---
 git-compat-util.h |  4 
 usage.c   | 12 +++-
 2 files changed, 15 insertions(+), 1 deletion(-)

diff --git a/git-compat-util.h b/git-compat-util.h
index cedad4d581..4fec462e30 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -1092,9 +1092,13 @@ static inline int regexec_buf(const regex_t *preg, const 
char *buf, size_t size,
 __attribute__((format (printf, 3, 4))) NORETURN
 void BUG_fl(const char *file, int line, const char *fmt, ...);
 #define BUG(...) BUG_fl(__FILE__, __LINE__, __VA_ARGS__)
+#define BUG_ON(condition, ...) do { if (condition) BUG(__VA_ARGS__); } while 
(0)
 #else
 __attribute__((format (printf, 1, 2))) NORETURN
 void BUG(const char *fmt, ...);
+
+__attribute__((format (printf, 2, 3)))
+void BUG_ON(int condition, const char *fmt, ...);
 #endif
 
 /*
diff --git a/usage.c b/usage.c
index cdd534c9df..3aed669181 100644
--- a/usage.c
+++ b/usage.c
@@ -240,7 +240,17 @@ NORETURN void BUG(const char *fmt, ...)
BUG_vfl(NULL, 0, fmt, ap);
va_end(ap);
 }
-#endif
+
+void BUG_ON(int condition, const char *fmt, ...)
+{
+   if (condition) {
+   va_list ap;
+   va_start(ap, fmt);
+   BUG_vfl(NULL, 0, fmt, ap);
+   va_end(ap);
+   }
+}
+#endif /* HAVE_VARIADIC_MACROS */
 
 #ifdef SUPPRESS_ANNOTATED_LEAKS
 void unleak_memory(const void *ptr, size_t len)
-- 
2.15.0.448.gf294e3d99a-goog

[PATCH 1/3] Documentation/CodingGuidelines: explain why assert is bad

Signed-off-by: Stefan Beller 
---
 Documentation/CodingGuidelines | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/Documentation/CodingGuidelines b/Documentation/CodingGuidelines
index c4cb5ff0d4..4f8791895b 100644
--- a/Documentation/CodingGuidelines
+++ b/Documentation/CodingGuidelines
@@ -386,6 +386,9 @@ For C programs:
  - Use Git's gettext wrappers to make the user interface
translatable. See "Marking strings for translation" in po/README.
 
+ - Prefer the BUG() macro over asserts, as asserts requires that the
+   NDEBUG flag is unset on compilation to work.
+
 For Perl programs:
 
  - Most of the C guidelines above apply.
-- 
2.15.0.448.gf294e3d99a-goog

Re: [PATCH 1/5] p5550: factor our nonsense-pack creation

On Tue, Nov 21, 2017 at 04:32:42PM -0800, Stefan Beller wrote:

> > Heh, yes. I even fixed it once, but I have the funny habit of noticing
> > such typos while reading the "todo" list of "rebase -i" and fixing them
> > there. Which of course has no impact whatsoever on the commit. :-/
> 
> That happened to me a couple of times as well before.
> This sounds like a UX bug on first thought.
> 
> On second thought the text after the abbreviated hash can be
> user dependent IIRC, by setting some format option how to populate the
> rebase instruction sheet using log, so it would not be easy to take
> fixes from that line into the commit for a fixup.

Right, I came to the same conclusion (we may even have discussed this on
the list, I don't remember). The current "todo" format says that only
the command and sha1 matter, and we'd be changing that. Maybe that's not
so bad if the user has to enable the feature themselves (and clearly it
would be incompatible with a custom format option). But I think in the
end it probably just makes sense to retrain my expectation, and remember
to "reword" instead of "pick".

-Peff

Re: [PATCH 5/5] sha1_file: don't re-scan pack directory for null sha1

On Wed, Nov 22, 2017 at 10:42:30AM +0900, Junio C Hamano wrote:

> Jeff King  writes:
> 
> > I'm not sure what the right behavior is, but I'm pretty sure that's not
> > it. Probably one of:
> >
> >   - skip updating the ref when we see the breakage
> >
> >   - ditto, but terminate the whole operation, since we might be deleting
> > other refs and in a broken repo we're probably best to make as few
> > changes as possible
> >
> >   - behave as if it was a non-ff, which would allow "--force" to
> > overwrite the broken ref. Maybe convenient for fixing things, but
> > possibly surprising (and it's not that hard to just delete the
> > broken refs manually before proceeding).
> 
> Perhaps the last one would be the ideal endgame, but the second one
> may be a good stopping point in the shorter term.

This turns out to be a lot trickier than I expected. The crux of the
matter is that the case we care about is hidden inside
lookup_commit_reference_gently(), which doesn't distinguish between
corruption and "not a commit".

So there are four cases we care about for this call in fetch:

  1. We fed a real sha1 and got a commit (or peeled to one).

  2. We fed a real sha1 which resolved to a non-commit, and we got NULL.

  3. We fed a real sha1 and the object was missing or corrupted, and we
 got NULL.

  4. We fed a null sha1 and got NULL.

Right now we lump cases 2-4 together as "do not do a fast-forward
check". That's fine for 2 and 4, but probably not for 3. We can easily
catch case 4 ourselves (if we care to), but distinguishing case 3 from
the others is hard. How should lookup_commit_reference_gently() signal
it to us?

Or should lookup_commit_reference_gently() die on corruption? That's not
very "gentle", but I think the "gently" here is really about "it might
not be a commit", not "the repo might be corrupted". But I think even
that may be the tip of the iceberg. The next thing we do is feed the
commits to in_merge_bases(), which will happily return "nope" if the old
commit cannot be parsed (because it has only a boolean return value).

So I dunno. Maybe it is a losing battle to try to pass this kind of
corruption information up the stack.  I'm tempted to say that there
should just be a "paranoid" flag to globally die() whenever we see a
corruption (and you could run with it normally, but relax it whenever
you're investigating a broken repo). But I doubt even that works. Not
having the "old_oid" object at all would be a repo corruption here, but
how are the low-level routines supposed to know when a missing object is
a corruption and when it is not?

-Peff

[PATCH v4 0/4] make git worktree add dwim more

The previous rounds were at
https://public-inbox.org/git/20171112134305.3949-1-t.gumme...@gmail.com/,
https://public-inbox.org/git/20171118181103.28354-1-t.gumme...@gmail.com/ and
https://public-inbox.org/git/20171118224706.13810-1-t.gumme...@gmail.com/.

Thanks Eric for the review of the previous round.

The main change is that the --[no-]track flag now works generally
instead of just disabling the new dwim mode.

Interdiff below:
diff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt
index eedead2c4c..abc8f1f50d 100644
--- a/Documentation/git-worktree.txt
+++ b/Documentation/git-worktree.txt
@@ -56,22 +56,23 @@ If  is not found, and neither `-b` nor `-B` nor 
`--detach` are
 used, but there does exist a tracking branch in exactly one remote
 (call it ) with a matching name, treat as equivalent to
 
-$ git worktree add -b   /
+$ git worktree add --track -b   /
 
 +
 If `` is omitted and neither `-b` nor `-B` nor `--detach` used,
 then, as a convenience, if there exists a tracking branch in exactly
-one remote (call it ) matching the basename of the path
-(call it ), treat it as equivalent to
+one remote (call it ``) matching the basename of the path
+(call it ``), treat it as equivalent to
 
-$ git worktree add -b   /
+$ git worktree add --track -b   /
 
-If no tracking branch exists in exactly one remote,  is
+If no tracking branch exists in exactly one remote, `` is
 created based on HEAD, as if `-b $(basename )` was specified.
 +
 To disable the behaviour of trying to match the basename of  to
 a remote, and always create a new branch from HEAD, the `--no-track`
 flag can be passed to `git worktree add`.
+
 list::
 
 List details of each worktree.  The main worktree is listed first, followed by
@@ -123,6 +124,12 @@ OPTIONS
such as configuring sparse-checkout. See "Sparse checkout"
in linkgit:git-read-tree[1].
 
+--[no-]track::
+   With `--track` `` is set as "tracking" branch for
+   ``.  This is the default if `` is a remote
+   tracking branch, and can be suppressed with `--no-track`.  See
+   also linkgit:git-branch[1].
+
 --lock::
Keep the working tree locked after creation. This is the
equivalent of `git worktree lock` after `git worktree add`,
diff --git a/builtin/worktree.c b/builtin/worktree.c
index b2a6dd020c..cbcceb0385 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -342,7 +342,7 @@ static int add(int ac, const char **av, const char *prefix)
const char *new_branch_force = NULL;
char *path;
const char *branch;
-   int track_dwim = 1;
+   const char *opt_track = NULL;
struct option options[] = {
OPT__FORCE(, N_("checkout  even if already 
checked out in other worktree")),
OPT_STRING('b', NULL, _branch, N_("branch"),
@@ -352,7 +352,9 @@ static int add(int ac, const char **av, const char *prefix)
OPT_BOOL(0, "detach", , N_("detach HEAD at named 
commit")),
OPT_BOOL(0, "checkout", , N_("populate the new 
working tree")),
OPT_BOOL(0, "lock", _locked, N_("keep the new working 
tree locked")),
-   OPT_BOOL(0, "track", _dwim, N_("checkout upstream branch 
if there's a unique match")),
+   OPT_PASSTHRU(0, "track", _track, NULL,
+N_("set up tracking mode (see git-branch(1))"),
+PARSE_OPT_NOARG | PARSE_OPT_OPTARG),
OPT_END()
};
 
@@ -387,7 +389,7 @@ static int add(int ac, const char **av, const char *prefix)
int n;
const char *s = worktree_basename(path, );
opts.new_branch = xstrndup(s, n);
-   if (track_dwim) {
+   if (!opt_track || strcmp(opt_track, "--no-track")) {
struct object_id oid;
const char *remote =
unique_tracking_name(opts.new_branch, );
@@ -402,11 +404,12 @@ static int add(int ac, const char **av, const char 
*prefix)
const char *remote;
 
commit = lookup_commit_reference_by_name(branch);
-   if (!commit)
+   if (!commit) {
remote = unique_tracking_name(branch, );
-   if (!commit && remote) {
-   opts.new_branch = branch;
-   branch = remote;
+   if (remote) {
+   opts.new_branch = branch;
+   branch = remote;
+   }
}
}
 
@@ -418,9 +421,13 @@ static int add(int ac, const char **av, const char *prefix)
argv_array_push(, "--force");
argv_array_push(, opts.new_branch);
argv_array_push(, branch);
+   if (opt_track)
+   argv_array_push(, opt_track);

[PATCH v4 2/4] worktree: add --[no-]track option to the add subcommand

Currently 'git worktree add' sets up tracking branches if '' is
a remote tracking branch, and doesn't set them up otherwise, as is the
default for 'git branch'.

This may or may not be what the user wants.  Allow overriding this
behaviour with a --[no-]track flag that gets passed through to 'git
branch'.

We already respect branch.autoSetupMerge, as 'git worktree' just calls
'git branch' internally.

Signed-off-by: Thomas Gummerer 
---
 Documentation/git-worktree.txt |  6 +
 builtin/worktree.c |  8 ++
 t/t2025-worktree-add.sh| 55 ++
 3 files changed, 69 insertions(+)

diff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt
index b472acc356..798a642f84 100644
--- a/Documentation/git-worktree.txt
+++ b/Documentation/git-worktree.txt
@@ -107,6 +107,12 @@ OPTIONS
such as configuring sparse-checkout. See "Sparse checkout"
in linkgit:git-read-tree[1].
 
+--[no-]track::
+   With `--track` `` is set as "tracking" branch for
+   ``.  This is the default if `` is a remote
+   tracking branch, and can be suppressed with `--no-track`.  See
+   also linkgit:git-branch[1].
+
 --lock::
Keep the working tree locked after creation. This is the
equivalent of `git worktree lock` after `git worktree add`,
diff --git a/builtin/worktree.c b/builtin/worktree.c
index 7b9307aa58..8f9446d43c 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -341,6 +341,7 @@ static int add(int ac, const char **av, const char *prefix)
const char *new_branch_force = NULL;
char *path;
const char *branch;
+   const char *opt_track = NULL;
struct option options[] = {
OPT__FORCE(, N_("checkout  even if already 
checked out in other worktree")),
OPT_STRING('b', NULL, _branch, N_("branch"),
@@ -350,6 +351,9 @@ static int add(int ac, const char **av, const char *prefix)
OPT_BOOL(0, "detach", , N_("detach HEAD at named 
commit")),
OPT_BOOL(0, "checkout", , N_("populate the new 
working tree")),
OPT_BOOL(0, "lock", _locked, N_("keep the new working 
tree locked")),
+   OPT_PASSTHRU(0, "track", _track, NULL,
+N_("set up tracking mode (see git-branch(1))"),
+PARSE_OPT_NOARG | PARSE_OPT_OPTARG),
OPT_END()
};
 
@@ -394,9 +398,13 @@ static int add(int ac, const char **av, const char *prefix)
argv_array_push(, "--force");
argv_array_push(, opts.new_branch);
argv_array_push(, branch);
+   if (opt_track)
+   argv_array_push(, opt_track);
if (run_command())
return -1;
branch = opts.new_branch;
+   } else if (opt_track) {
+   die(_("--[no-]track can only be used if a new branch is 
created"));
}
 
UNLEAK(path);
diff --git a/t/t2025-worktree-add.sh b/t/t2025-worktree-add.sh
index b5c47ac602..53042ce565 100755
--- a/t/t2025-worktree-add.sh
+++ b/t/t2025-worktree-add.sh
@@ -313,5 +313,60 @@ test_expect_success 'checkout a branch under bisect' '
 test_expect_success 'rename a branch under bisect not allowed' '
test_must_fail git branch -M under-bisect bisect-with-new-name
 '
+# Is branch "refs/heads/$1" set to pull from "$2/$3"?
+test_branch_upstream () {
+   printf "%s\n" "$2" "refs/heads/$3" >expect.upstream &&
+   {
+   git config "branch.$1.remote" &&
+   git config "branch.$1.merge"
+   } >actual.upstream &&
+   test_cmp expect.upstream actual.upstream
+}
+
+test_expect_success '--track sets up tracking' '
+   test_when_finished rm -rf track &&
+   git worktree add --track -b track track master &&
+   git config "branch.track.merge" &&
+   (
+   test_branch_upstream track . master
+   )
+'
+
+# setup remote repository $1 and repository $2 with $1 set up as
+# remote.  The remote has two branches, master and foo.
+setup_remote_repo () {
+   git init $1 &&
+   (
+   cd $1 &&
+   test_commit $1_master &&
+   git checkout -b foo &&
+   test_commit upstream_foo
+   ) &&
+   git init $2 &&
+   (
+   cd $2 &&
+   test_commit $2_master &&
+   git remote add $1 ../$1 &&
+   git config remote.$1.fetch \
+   "refs/heads/*:refs/remotes/$1/*" &&
+   git fetch --all
+   )
+}
+
+test_expect_success '--no-track avoids setting up tracking' '
+   test_when_finished rm -rf repo_upstream repo_local foo &&
+   setup_remote_repo repo_upstream repo_local &&
+   (
+   cd repo_local &&
+   git worktree add --no-track -b foo ../foo repo_upstream/foo
+   ) &&
+   (
+

[PATCH v4 1/4] checkout: factor out functions to new lib file

Factor the functions out, so they can be re-used from other places.  In
particular these functions will be re-used in builtin/worktree.c to make
git worktree add dwim more.

While there add some docs to the function.

Signed-off-by: Thomas Gummerer 
---
 Makefile   |  1 +
 builtin/checkout.c | 41 +
 checkout.c | 42 ++
 checkout.h | 13 +
 4 files changed, 57 insertions(+), 40 deletions(-)
 create mode 100644 checkout.c
 create mode 100644 checkout.h

diff --git a/Makefile b/Makefile
index cd75985991..8d603c7443 100644
--- a/Makefile
+++ b/Makefile
@@ -757,6 +757,7 @@ LIB_OBJS += branch.o
 LIB_OBJS += bulk-checkin.o
 LIB_OBJS += bundle.o
 LIB_OBJS += cache-tree.o
+LIB_OBJS += checkout.o
 LIB_OBJS += color.o
 LIB_OBJS += column.o
 LIB_OBJS += combine-diff.o
diff --git a/builtin/checkout.c b/builtin/checkout.c
index fc4f8fd2ea..9e1cfd10b3 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -1,5 +1,6 @@
 #include "builtin.h"
 #include "config.h"
+#include "checkout.h"
 #include "lockfile.h"
 #include "parse-options.h"
 #include "refs.h"
@@ -874,46 +875,6 @@ static int git_checkout_config(const char *var, const char 
*value, void *cb)
return git_xmerge_config(var, value, NULL);
 }
 
-struct tracking_name_data {
-   /* const */ char *src_ref;
-   char *dst_ref;
-   struct object_id *dst_oid;
-   int unique;
-};
-
-static int check_tracking_name(struct remote *remote, void *cb_data)
-{
-   struct tracking_name_data *cb = cb_data;
-   struct refspec query;
-   memset(, 0, sizeof(struct refspec));
-   query.src = cb->src_ref;
-   if (remote_find_tracking(remote, ) ||
-   get_oid(query.dst, cb->dst_oid)) {
-   free(query.dst);
-   return 0;
-   }
-   if (cb->dst_ref) {
-   free(query.dst);
-   cb->unique = 0;
-   return 0;
-   }
-   cb->dst_ref = query.dst;
-   return 0;
-}
-
-static const char *unique_tracking_name(const char *name, struct object_id 
*oid)
-{
-   struct tracking_name_data cb_data = { NULL, NULL, NULL, 1 };
-   cb_data.src_ref = xstrfmt("refs/heads/%s", name);
-   cb_data.dst_oid = oid;
-   for_each_remote(check_tracking_name, _data);
-   free(cb_data.src_ref);
-   if (cb_data.unique)
-   return cb_data.dst_ref;
-   free(cb_data.dst_ref);
-   return NULL;
-}
-
 static int parse_branchname_arg(int argc, const char **argv,
int dwim_new_local_branch_ok,
struct branch_info *new,
diff --git a/checkout.c b/checkout.c
new file mode 100644
index 00..b0c744d37a
--- /dev/null
+++ b/checkout.c
@@ -0,0 +1,42 @@
+#include "cache.h"
+#include "remote.h"
+
+struct tracking_name_data {
+   /* const */ char *src_ref;
+   char *dst_ref;
+   struct object_id *dst_oid;
+   int unique;
+};
+
+static int check_tracking_name(struct remote *remote, void *cb_data)
+{
+   struct tracking_name_data *cb = cb_data;
+   struct refspec query;
+   memset(, 0, sizeof(struct refspec));
+   query.src = cb->src_ref;
+   if (remote_find_tracking(remote, ) ||
+   get_oid(query.dst, cb->dst_oid)) {
+   free(query.dst);
+   return 0;
+   }
+   if (cb->dst_ref) {
+   free(query.dst);
+   cb->unique = 0;
+   return 0;
+   }
+   cb->dst_ref = query.dst;
+   return 0;
+}
+
+const char *unique_tracking_name(const char *name, struct object_id *oid)
+{
+   struct tracking_name_data cb_data = { NULL, NULL, NULL, 1 };
+   cb_data.src_ref = xstrfmt("refs/heads/%s", name);
+   cb_data.dst_oid = oid;
+   for_each_remote(check_tracking_name, _data);
+   free(cb_data.src_ref);
+   if (cb_data.unique)
+   return cb_data.dst_ref;
+   free(cb_data.dst_ref);
+   return NULL;
+}
diff --git a/checkout.h b/checkout.h
new file mode 100644
index 00..9980711179
--- /dev/null
+++ b/checkout.h
@@ -0,0 +1,13 @@
+#ifndef CHECKOUT_H
+#define CHECKOUT_H
+
+#include "cache.h"
+
+/*
+ * Check if the branch name uniquely matches a branch name on a remote
+ * tracking branch.  Return the name of the remote if such a branch
+ * exists, NULL otherwise.
+ */
+extern const char *unique_tracking_name(const char *name, struct object_id 
*oid);
+
+#endif /* CHECKOUT_H */
-- 
2.15.0.345.gf926f18f3

[PATCH v4 3/4] worktree: make add dwim

Currently 'git worktree add  ', errors out when 'branch'
is not a local branch.   It has no additional dwim'ing features that one
might expect.

Make it behave more like 'git checkout ' when the branch doesn't
exist locally, but a remote tracking branch uniquely matches the desired
branch name, i.e. create a new branch from the remote tracking branch
and set the upstream to the remote tracking branch.

As 'git worktree add' currently just dies in this situation, there are
no backwards compatibility worries when introducing this feature.

Signed-off-by: Thomas Gummerer 
---
 Documentation/git-worktree.txt |  7 +++
 builtin/worktree.c | 16 
 t/t2025-worktree-add.sh| 21 +
 3 files changed, 44 insertions(+)

diff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt
index 798a642f84..45642d3b7f 100644
--- a/Documentation/git-worktree.txt
+++ b/Documentation/git-worktree.txt
@@ -52,6 +52,13 @@ is linked to the current repository, sharing everything 
except working
 directory specific files such as HEAD, index, etc. `-` may also be
 specified as ``; it is synonymous with `@{-1}`.
 +
+If  is not found, and neither `-b` nor `-B` nor `--detach` are
+used, but there does exist a tracking branch in exactly one remote
+(call it ) with a matching name, treat as equivalent to
+
+$ git worktree add --track -b   /
+
++
 If `` is omitted and neither `-b` nor `-B` nor `--detach` used,
 then, as a convenience, a new branch based at HEAD is created automatically,
 as if `-b $(basename )` was specified.
diff --git a/builtin/worktree.c b/builtin/worktree.c
index 8f9446d43c..e9cc3f3872 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -1,4 +1,5 @@
 #include "cache.h"
+#include "checkout.h"
 #include "config.h"
 #include "builtin.h"
 #include "dir.h"
@@ -390,6 +391,21 @@ static int add(int ac, const char **av, const char *prefix)
opts.new_branch = xstrndup(s, n);
}
 
+   if (ac == 2 && !opts.new_branch && !opts.detach) {
+   struct object_id oid;
+   struct commit *commit;
+   const char *remote;
+
+   commit = lookup_commit_reference_by_name(branch);
+   if (!commit) {
+   remote = unique_tracking_name(branch, );
+   if (remote) {
+   opts.new_branch = branch;
+   branch = remote;
+   }
+   }
+   }
+
if (opts.new_branch) {
struct child_process cp = CHILD_PROCESS_INIT;
cp.git_cmd = 1;
diff --git a/t/t2025-worktree-add.sh b/t/t2025-worktree-add.sh
index 53042ce565..ea144938a9 100755
--- a/t/t2025-worktree-add.sh
+++ b/t/t2025-worktree-add.sh
@@ -369,4 +369,25 @@ test_expect_success '--no-track avoids setting up 
tracking' '
)
 '
 
+test_expect_success '"add"   fails' '
+   test_must_fail git worktree add foo non-existent
+'
+
+test_expect_success '"add"   dwims' '
+   test_when_finished rm -rf repo_upstream repo_dwim foo &&
+   setup_remote_repo repo_upstream repo_dwim &&
+   git init repo_dwim &&
+   (
+   cd repo_dwim &&
+   git worktree add ../foo foo
+   ) &&
+   (
+   cd foo &&
+   test_branch_upstream foo repo_upstream foo &&
+   git rev-parse repo_upstream/foo >expect &&
+   git rev-parse foo >actual &&
+   test_cmp expect actual
+   )
+'
+
 test_done
-- 
2.15.0.345.gf926f18f3

[PATCH v4 4/4] worktree: make add dwim

Currently 'git worktree add ' creates a new branch named after the
basename of the , that matches the HEAD of whichever worktree we
were on when calling "git worktree add ".

Make 'git worktree add  behave more like the dwim machinery in
'git checkout ', i.e. check if the new branch name uniquely
matches the branch name of a remote tracking branch, and if so check out
that branch and set the upstream to the remote tracking branch.

This is a change of behaviour compared to the current behaviour, where
we create a new branch matching HEAD.  However as 'git worktree' is
still an experimental feature, and it's easy to notice/correct the
behaviour in case it's not what the user desired it's probably okay to
break existing behaviour here.

In order to also satisfy users who want the current behaviour of
creating a new branch from HEAD, add a '--no-track' flag, which disables
the new behaviour, and keeps the old behaviour of creating a new branch
from the head of the current worktree.

Signed-off-by: Thomas Gummerer 
---
 Documentation/git-worktree.txt | 14 --
 builtin/worktree.c |  7 +++
 t/t2025-worktree-add.sh| 32 
 3 files changed, 51 insertions(+), 2 deletions(-)

diff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt
index 45642d3b7f..abc8f1f50d 100644
--- a/Documentation/git-worktree.txt
+++ b/Documentation/git-worktree.txt
@@ -60,8 +60,18 @@ $ git worktree add --track -b   
/
 
 +
 If `` is omitted and neither `-b` nor `-B` nor `--detach` used,
-then, as a convenience, a new branch based at HEAD is created automatically,
-as if `-b $(basename )` was specified.
+then, as a convenience, if there exists a tracking branch in exactly
+one remote (call it ``) matching the basename of the path
+(call it ``), treat it as equivalent to
+
+$ git worktree add --track -b   /
+
+If no tracking branch exists in exactly one remote, `` is
+created based on HEAD, as if `-b $(basename )` was specified.
++
+To disable the behaviour of trying to match the basename of  to
+a remote, and always create a new branch from HEAD, the `--no-track`
+flag can be passed to `git worktree add`.
 
 list::
 
diff --git a/builtin/worktree.c b/builtin/worktree.c
index e9cc3f3872..cbcceb0385 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -389,6 +389,13 @@ static int add(int ac, const char **av, const char *prefix)
int n;
const char *s = worktree_basename(path, );
opts.new_branch = xstrndup(s, n);
+   if (!opt_track || strcmp(opt_track, "--no-track")) {
+   struct object_id oid;
+   const char *remote =
+   unique_tracking_name(opts.new_branch, );
+   if (remote)
+   branch = remote;
+   }
}
 
if (ac == 2 && !opts.new_branch && !opts.detach) {
diff --git a/t/t2025-worktree-add.sh b/t/t2025-worktree-add.sh
index ea144938a9..6fd3da4036 100755
--- a/t/t2025-worktree-add.sh
+++ b/t/t2025-worktree-add.sh
@@ -390,4 +390,36 @@ test_expect_success '"add"   dwims' '
)
 '
 
+test_expect_success 'git worktree add --no-track does not set up tracking' '
+   test_when_finished rm -rf repo_a repo_b foo &&
+   setup_remote_repo repo_a repo_b &&
+   (
+   cd repo_b &&
+   git worktree add --no-track ../foo
+   ) &&
+   (
+   cd foo &&
+   ! test_branch_upstream foo repo_a foo &&
+   git rev-parse repo_a/foo >expect &&
+   git rev-parse foo >actual &&
+   ! test_cmp expect actual
+   )
+'
+
+test_expect_success 'git worktree add sets up tracking' '
+   test_when_finished rm -rf repo_a repo_b &&
+   setup_remote_repo repo_a repo_b &&
+   (
+   cd repo_b &&
+   git worktree add ../foo
+   ) &&
+   (
+   cd foo &&
+   test_branch_upstream foo repo_a foo &&
+   git rev-parse repo_a/foo >expect &&
+   git rev-parse foo >actual &&
+   test_cmp expect actual
+   )
+'
+
 test_done
-- 
2.15.0.345.gf926f18f3

Re: git status always modifies index?

On Wed, Nov 22, 2017 at 01:56:35PM -0800, Jonathan Nieder wrote:

> Jeff King wrote:
> > On Wed, Nov 22, 2017 at 12:27:20PM -0800, Jonathan Nieder wrote:
> 
> >> That said, I wonder if this use case is an illustration that a name
> >> like --no-lock-index (as was used in Git for Windows when this feature
> >> first appeared) or --no-refresh-on-disk-index (sorry, I am terrible at
> >> coming up with option names) would make the feature easier to
> >> discover.
> [...]
> > Or maybe just living with the minor philosophical rough edges,
> > since it seems OK in practice.
> 
> To be clear, my concern is not philosophical but practical: I'm saying
> if it's a "git status" option (or at least shows up in the "git
> status" manpage) and it is memorably about $GIT_DIR/index (at least
> mentions that in its description) then it is more likely to help
> people.

Right, I went a little off track of your original point.

What I was trying to get at is that naming it "status --no-lock-index"
would not be the same thing (even though with the current implementation
it would behave the same). IOW, can we improve the documentation of
"status" to point to make it easier to discover this use case.

-Peff

Re: [PATCH v2] Teach stash to parse -m/--message like commit does

On Wed, Nov 22, 2017 at 01:20:30PM -0800, Phil Hord wrote:

> `git stash push -m foo` uses "foo" as the message for the stash. But
> `git stash push -m"foo"` does not parse successfully.  Similarly
> `git stash push --message="My stash message"` also fails.  The stash
> documentation doesn't suggest this syntax should work, but gitcli
> does and my fingers have learned this pattern long ago for `commit`.
> 
> Teach `git stash` and `git store` to parse -mFoo and --message=Foo
> the same as `git commit` would do.  Even though it's an internal
> function, add similar support to create_stash() for consistency.

I definitely approve of the goal. The implementation looks pretty
straightforward given the current parsing scheme.

Many of our other scripts lean on "rev-parse --parseopt" to handle
options.  E.g.:

OPTIONS="\
git foo [options]
--
m,message= stash message
"
foo() {
for i in "$@"; do echo " pre: $i"; done
eval "$(echo -n "$OPTIONS" | git rev-parse --parseopt -- "$@")"
for i in "$@"; do echo "post: $i"; done
}
foo -mmsg
foo -m msg
foo --message=msg
foo --message msg

should convert each of those into "-m msg". It also handles unique
partial options like "--mess", though IMHO that is not that big a deal.

Would it be possible to convert stash to use --parseopt? I'm fine if the
answer is "no", or even "yes, but it's tricky so let's do this in the
meantime". But I think that's the endgame we should be shooting for (or,
of course, doing the whole thing in C, which I think somebody else is
working on).

-Peff

Re: git status always modifies index?

Jeff King wrote:
> On Wed, Nov 22, 2017 at 12:27:20PM -0800, Jonathan Nieder wrote:

>> That said, I wonder if this use case is an illustration that a name
>> like --no-lock-index (as was used in Git for Windows when this feature
>> first appeared) or --no-refresh-on-disk-index (sorry, I am terrible at
>> coming up with option names) would make the feature easier to
>> discover.
[...]
> Or maybe just living with the minor philosophical rough edges,
> since it seems OK in practice.

To be clear, my concern is not philosophical but practical: I'm saying
if it's a "git status" option (or at least shows up in the "git
status" manpage) and it is memorably about $GIT_DIR/index (at least
mentions that in its description) then it is more likely to help
people.

Thanks,
Jonathan

Re: [PATCH v5 00/10] Partial clone part 2: fsck and promisors

2017-11-22 Thread Jonathan Tan

On Wed, Nov 22, 2017 at 10:00 AM, Jonathan Tan  wrote:
> On Wed, 22 Nov 2017 14:25:13 +0900
> Junio C Hamano  wrote:
>
>> Thanks, will replace/queue all three series.  I am getting a feeling
>> that the first one is already ready for 'next', while the other two
>> may want to see a bit more comments?
>
> Yes, I think so too.
>
> Jeff Hostetler and I noticed some issues occuring when some other Git
> commands dynamically fetch objects due to the fact that those commands
> and fetch-pack use overlapping object flags. At the very least, we
> should look at that before it goes into next.

In the end, making fetch-pack refrain from setting object flags when
being used to dynamically fetch objects works (see the new "introduce
fetch-object: fetch one promisor object" commit that I just pushed to
[1]). I'll coordinate with Jeff Hostetler about sending v6 to the list
(most likely after the U.S. Thanksgiving holidays).

[1] https://github.com/jonathantanmy/git/commits/pc20171122

Re: Bisect marking new commits incorrectly

On Wed, Nov 22, 2017 at 05:01:29PM +, Adam Dinwoodie wrote:

> > So if you test a commit that git bisect asks you to test, and it
> > appears that this commit is "new", then you can just discard the
> > previous "new" commit because it will give you less information than
> > the new "new" one.
> > (The old "new" will not let you discard any commits that the new "new"
> > commit allows you to discard, because it is a descendant of the new
> > "new" commit.)
> > 
> > If you don't test the commit that git bisect asks you to test, then
> > git bisect assumes that you know better and discards the old "new".
> 
> Ah, that makes sense, thank you for the explanation.
> 
> In my case, I knew two commits were "new", but didn't know which would
> provide more information to the bisect algorithm; I'd assumed if I
> provided both, Git would work out the appropriate thing to do with the
> combined information without me needing to work it out.

Yeah, I really would have expected this to work, too. Should we be
taking the merge base of the set of "new" commits, and using that as the
true "new"?

I.e., with this graph:

  A--B--C--D
  \
   E--F

if I mark D and F as bad, can we assume that B, C, and E are "new", too,
and treat "B" as the new single "new"? That breaks down if the bug was
introduced independently on each branch, but I think bisect explicitly
doesn't handle such a case.

What might give it trouble is if there are multiple merge bases, like:

G--H-
   /  \  \
  A--B--C--D  \
  \\
   EF

The flip to "new" could have been on the G-H branch, or in B.

So maybe that's not workable.

(I've never really dug into the bisect algorithm, and this is written
largely off the cuff, so I'm fine if the response is "nope, you have no
idea what you are talking about").

-Peff

[PATCH v2 2/2] stash-store: add failing test for same-ref

stash-store cannot create a new stash with the same ref as stash@{0}. No
error is returned even though no new stash log is created. Add a failing
test to track.

Signed-off-by: Phil Hord 
---
 t/t3903-stash.sh | 11 +++
 1 file changed, 11 insertions(+)

diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
index 279e31717..7d511afd3 100755
--- a/t/t3903-stash.sh
+++ b/t/t3903-stash.sh
@@ -813,6 +813,17 @@ test_expect_success 'push -m also works without space' '
test_cmp expect actual
 '
 
+test_expect_failure 'store same ref twice' '
+   >foo &&
+   git add foo &&
+   STASH_ID=$(git stash create) &&
+   git stash store -m "original message" $STASH_ID &&
+   git stash store -m "custom message" $STASH_ID &&
+   echo "stash@{0}: custom message" >expect &&
+   git stash list -1 >actual &&
+   test_cmp expect actual
+'
+
 test_expect_success 'store -m foo shows right message' '
>foo &&
git add foo &&
-- 
2.15.0.471.g17a719cfe.dirty

Re: [PATCH] defer expensive load_ref_decorations until needed

On Tue, Nov 21, 2017 at 03:43:36PM -0800, Phil Hord wrote:

> With many thousands of references, a simple `git rev-parse HEAD` may take
> more than a second to return because it first loads all the refs into
> memory even though it will never use them.

The overall goal of lazy-loading seems reasonable, but I'm slightly
confused: how and why does "git rev-parse HEAD" load ref decorations?

Grepping around I find that we mostly load them only when appropriate
(when the "log" family sees a decorate option, when we see %d/%D in a
pretty format, or with --simplify-by-decoration in a traversal). And
poking at "rev-parse HEAD" in gdb seems to confirm that it does not hit
that function.

I have definitely seen "rev-parse HEAD" be O(# of refs), but that is
mostly attributable to having all the refs packed (and until v2.15.0,
the packed-refs code would read the whole file into memory). I've also
seen unnecessary ref lookups due to replace refs (we load al of the
packed refs to find out that no, there's nothing in refs/replace).

-Peff

[PATCH v2] Teach stash to parse -m/--message like commit does

`git stash push -m foo` uses "foo" as the message for the stash. But
`git stash push -m"foo"` does not parse successfully.  Similarly
`git stash push --message="My stash message"` also fails.  The stash
documentation doesn't suggest this syntax should work, but gitcli
does and my fingers have learned this pattern long ago for `commit`.

Teach `git stash` and `git store` to parse -mFoo and --message=Foo
the same as `git commit` would do.  Even though it's an internal
function, add similar support to create_stash() for consistency.

Reviewd-by: Junio C Hamano 
Signed-off-by: Phil Hord 
---

Added tests for 'stash push' and 'stash store'.
Added a note that create_stash is included but unnecessary.

 git-stash.sh | 18 +++
 t/t3903-stash.sh | 93 
 2 files changed, 111 insertions(+)

diff --git a/git-stash.sh b/git-stash.sh
index 4b7495144..1114005ce 100755
--- a/git-stash.sh
+++ b/git-stash.sh
@@ -76,6 +76,12 @@ create_stash () {
shift
stash_msg=${1?"BUG: create_stash () -m requires an 
argument"}
;;
+   -m*)
+   stash_msg=${1#-m}
+   ;;
+   --message=*)
+   stash_msg=${1#--message=}
+   ;;
-u|--include-untracked)
shift
untracked=${1?"BUG: create_stash () -u requires an 
argument"}
@@ -193,6 +199,12 @@ store_stash () {
shift
stash_msg="$1"
;;
+   -m*)
+   stash_msg=${1#-m}
+   ;;
+   --message=*)
+   stash_msg=${1#--message=}
+   ;;
-q|--quiet)
quiet=t
;;
@@ -251,6 +263,12 @@ push_stash () {
test -z ${1+x} && usage
stash_msg=$1
;;
+   -m*)
+   stash_msg=${1#-m}
+   ;;
+   --message=*)
+   stash_msg=${1#--message=}
+   ;;
--help)
show_help
;;
diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
index 3b1ac1971..39c7f2ebd 100755
--- a/t/t3903-stash.sh
+++ b/t/t3903-stash.sh
@@ -804,6 +804,99 @@ test_expect_success 'push -m shows right message' '
test_cmp expect actual
 '
 
+test_expect_success 'push -m also works without space' '
+   >foo &&
+   git add foo &&
+   git stash push -m"unspaced test message" &&
+   echo "stash@{0}: On master: unspaced test message" >expect &&
+   git stash list -1 >actual &&
+   test_cmp expect actual
+'
+
+test_expect_success 'store -m foo shows right message' '
+   git stash clear &&
+   git reset --hard &&
+   echo quux >bazzy &&
+   git add bazzy &&
+   STASH_ID=$(git stash create) &&
+   git stash store -m "store m" $STASH_ID &&
+   echo "stash@{0}: store m" >expect &&
+   git stash list -1 >actual &&
+   test_cmp expect actual
+'
+
+test_expect_success 'store -mfoo shows right message' '
+   git stash clear &&
+   git reset --hard &&
+   echo quux >bazzy &&
+   git add bazzy &&
+   STASH_ID=$(git stash create) &&
+   git stash store -m"store mfoo" $STASH_ID &&
+   echo "stash@{0}: store mfoo" >expect &&
+   git stash list -1 >actual &&
+   test_cmp expect actual
+'
+
+test_expect_success 'store --message=foo shows right message' '
+   git stash clear &&
+   git reset --hard &&
+   echo quux >bazzy &&
+   git add bazzy &&
+   STASH_ID=$(git stash create) &&
+   git stash store --message="store message=foo" $STASH_ID &&
+   echo "stash@{0}: store message=foo" >expect &&
+   git stash list -1 >actual &&
+   test_cmp expect actual
+'
+
+test_expect_success 'store --message foo shows right message' '
+   git stash clear &&
+   git reset --hard &&
+   echo quux >bazzy &&
+   git add bazzy &&
+   STASH_ID=$(git stash create) &&
+   git stash store --message "store message foo" $STASH_ID &&
+   echo "stash@{0}: store message foo" >expect &&
+   git stash list -1 >actual &&
+   test_cmp expect actual
+'
+
+test_expect_success 'push -mfoo uses right message' '
+   >foo &&
+   git add foo &&
+   git stash push -m"test mfoo" &&
+   echo "stash@{0}: On master: test mfoo" >expect &&
+   git stash list -1 >actual &&
+   test_cmp expect actual
+'
+
+test_expect_success 'push --message foo is synonym for -mfoo' '
+   >foo &&
+   git add foo &&
+   git stash push --message "test message foo" &&
+   echo "stash@{0}: On master: test message foo" >expect &&
+   git stash

Re: git status always modifies index?

On Wed, Nov 22, 2017 at 12:27:20PM -0800, Jonathan Nieder wrote:

> Nathan Neulinger wrote[1]:
> 
> > I just got an answer to my stackoverflow question on this,
> > apparently it's already implemented:
> >
> > https://stackoverflow.com/questions/47436939/how-to-run-git-status-without-modifying-git-index-such-as-in-a-prompt-command
> >
> > There is a "--no-optional-locks" command in 2.15 that looks like it
> > does exactly what I need.
> 
> I was about to point to
> https://public-inbox.org/git/20170921043214.pyhdsrpy4omy5...@sigill.intra.peff.net/
> about exactly this thing. :)
> 
> That said, I wonder if this use case is an illustration that a name
> like --no-lock-index (as was used in Git for Windows when this feature
> first appeared) or --no-refresh-on-disk-index (sorry, I am terrible at
> coming up with option names) would make the feature easier to
> discover.

Yeah, it's interesting that Nathan does not care about the simultaneous
locking here, but rather about the effect of writing to the repo for
what would otherwise be a read-only operation.

Under the original intent of --no-optional-locks I think if we could
somehow magically update the on-disk index without lock contention, it
would be OK to do so. But that would make it no longer work for this
particular case.

And I would also not be surprised if there are other cases where we
write in a lockless way that would best be avoided in a multi-user
setup. I'm thinking specifically of the way that some merge-y operations
may write out intermediate objects, even though they're only needed
inside the process. It _should_ be a read-only operation to ask "can
these two things be merged cleanly", and you should be able to ask that
without accidentally creating root-owned files in .git/objects.

So I actually think what Nathan wants is not exactly the same as
--no-optional-locks in the first place. But in practice, for a limited
set of operations and with the way that locks work in Git, it
accomplishes the same thing. Maybe that points to having a broader
option. Or maybe having two separate options that largely have the same
effect. Or maybe just living with the minor philosophical rough edges,
since it seems OK in practice.

-Peff

RE: Beneficiary

2017-11-22 Thread PLEDGES




Good Day my name is Mrs Alice Walton I have a Present Package for you contact  
me Via:alicewaltso...@gmail.com

Re: [PATCH] fix french translation


Thanks!

Bye

Le mer 22 nov 2017 à 21:17, Jean-Noël AVILA  a 
écrit :

On Wednesday, 22 November 2017 18:24:40 CET Louis Bettens wrote:

 Signed-off-by: Louis Bettens 
 ---
  po/fr.po | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

 diff --git a/po/fr.po b/po/fr.po
 index 4deae3318..a12a2ae37 100644
 --- a/po/fr.po
 +++ b/po/fr.po
 @@ -14331,7 +14331,7 @@ msgstr "git worktree add [] 
 []"


  #: builtin/worktree.c:17
  msgid "git worktree list []"
 -msgstr "git worktree prune []"
 +msgstr "git worktree list []"

  #: builtin/worktree.c:18
  msgid "git worktree lock [] "



Good catch! I guess the segment switched directly from fuzzy to 
translated.


I'll queue it for the next version which we should not wait for long.

Thanks

Re: git status always modifies index?

Hi,

Nathan Neulinger wrote[1]:

> I just got an answer to my stackoverflow question on this,
> apparently it's already implemented:
>
> https://stackoverflow.com/questions/47436939/how-to-run-git-status-without-modifying-git-index-such-as-in-a-prompt-command
>
> There is a "--no-optional-locks" command in 2.15 that looks like it
> does exactly what I need.

I was about to point to
https://public-inbox.org/git/20170921043214.pyhdsrpy4omy5...@sigill.intra.peff.net/
about exactly this thing. :)

That said, I wonder if this use case is an illustration that a name
like --no-lock-index (as was used in Git for Windows when this feature
first appeared) or --no-refresh-on-disk-index (sorry, I am terrible at
coming up with option names) would make the feature easier to
discover.

Thanks,
Jonathan

[1] 
https://public-inbox.org/git/dfbf4af3-e87c-bdcb-7544-685572925...@neulinger.org/

Re: [PATCH 1/2] test-lib: add LIBPCRE1 & LIBPCRE2 prerequisites

Hi,

Ævar Arnfjörð Bjarmason wrote:

> Add LIBPCRE1 and LIBPCRE2 prerequisites which are true when git is
> compiled with USE_LIBPCRE1=YesPlease or USE_LIBPCRE2=YesPlease,
> respectively.
>
> The syntax of PCRE1 and PCRE2 isn't the same in all cases (see
> pcresyntax(3) and pcre2syntax(3)). If test are added that test for
> those they'll need to be guarded by these new prerequisites.
>
> Signed-off-by: Ævar Arnfjörð Bjarmason 
> ---
>  t/README  | 12 
>  t/test-lib.sh |  2 ++
>  2 files changed, 14 insertions(+)
> 
> diff --git a/t/README b/t/README
> index 4b079e4494..599cd9808c 100644
> --- a/t/README
> +++ b/t/README
> @@ -808,6 +808,18 @@ use these, and "test_set_prereq" for how to define your 
> own.
> Git was compiled with support for PCRE. Wrap any tests
> that use git-grep --perl-regexp or git-grep -P in these.
>  
> + - LIBPCRE1
> +
> +   Git was compiled with PCRE v1 support via
> +   USE_LIBPCRE1=YesPlease. Wrap any PCRE using tests that for some
> +   reason need v1 of the PCRE library instead of v2 in these.

Are there plans to use the LIBPCRE1 prereq?  It might be simpler to
only have LIBPCRE2, and LIBPCRE1 can still be expressed as

PCRE,!LIBPCRE2

which I think is clearer about the intent.

Thanks,
Jonathan

Re: [PATCH] fix french translation

2017-11-22 Thread Jean-Noël AVILA

On Wednesday, 22 November 2017 18:24:40 CET Louis Bettens wrote:
> Signed-off-by: Louis Bettens 
> ---
>  po/fr.po | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/po/fr.po b/po/fr.po
> index 4deae3318..a12a2ae37 100644
> --- a/po/fr.po
> +++ b/po/fr.po
> @@ -14331,7 +14331,7 @@ msgstr "git worktree add []  
> []"
>  
>  #: builtin/worktree.c:17
>  msgid "git worktree list []"
> -msgstr "git worktree prune []"
> +msgstr "git worktree list []"
>  
>  #: builtin/worktree.c:18
>  msgid "git worktree lock [] "
> 

Good catch! I guess the segment switched directly from fuzzy to translated.

I'll queue it for the next version which we should not wait for long.

Thanks

Re: [PATCH v5 5/6] rev-list: add list-objects filtering support

Hi,

Jeff Hostetler wrote:

> Teach rev-list to use the filtering provided by the
> traverse_commit_list_filtered() interface to omit
> unwanted objects from the result.
>
> Object filtering is only allowed when one of the "--objects*"
> options are used.

micronit: the line widths seem to be uneven in these commit messages,
which is a bit distracting when reading.

> When the "--filter-print-omitted" option is used, the omitted
> objects are printed at the end.  These are marked with a "~".
> This option can be combined with "--quiet" to get a list of
> just the omitted objects.

Neat.  Can you give a quick example?

Using --quiet for this feels a bit odd, since it previously meant
to print nothing to stdout.  I wonder if there's another way ---
e.g.

--print-omitted=(yes|no|only)

If I wanted to list all objects matching a filter, even objects
that are not reachable from any ref, is there a way to do that?
(Just curious, trying to think about this interface.)

> Add t6112 test.

This part doesn't need to be in the commit message.  More generally,
anything I could more easily learn from the code or diffstat doesn't
need to be in the commit message: the commit message is about the
"why" more than the details of what in the code changed.

> In the future, we will introduce a "partial clone" mechanism
> wherein an object in a repo, obtained from a remote, may
> reference a missing object that can be dynamically fetched from
> that remote once needed.  This "partial clone" mechanism will
> have a way, sometimes slow, of determining if a missing link
> is one of the links expected to be produced by this mechanism.

Does this mean the s will be part of the wire protocol?
I'll look more carefully at them below with that in mind.

> This patch introduces handling of missing objects to help
> debugging and development of the "partial clone" mechanism,
> and once the mechanism is implemented, for a power user to
> perform operations that are missing-object aware without
> incurring the cost of checking if a missing link is expected.

I had trouble understanding what this paragraph is about.  Can you
give an example?

> Signed-off-by: Jeff Hostetler 
> ---
>  Documentation/git-rev-list.txt  |   4 +-
>  Documentation/rev-list-options.txt  |  36 ++
>  builtin/rev-list.c  | 108 -
>  t/t6112-rev-list-filters-objects.sh | 225 
> 
>  4 files changed, 370 insertions(+), 3 deletions(-)
>  create mode 100755 t/t6112-rev-list-filters-objects.sh

Looks reasonably concise, good.

[...]
> --- a/Documentation/git-rev-list.txt
> +++ b/Documentation/git-rev-list.txt
> @@ -47,7 +47,9 @@ SYNOPSIS
>[ --fixed-strings | -F ]
>[ --date=]
>[ [ --objects | --objects-edge | --objects-edge-aggressive ]
> -[ --unpacked ] ]
> +[ --unpacked ]
> +[ --filter= [ --filter-print-omitted ] ] ]

Does this mean --filter is only useful with --objects?  E.g. I can't
use it to filter commits?

> +  [ --missing= ]

--missing=(error|allow-any|print) would be more informative and about
equally concise.

Since this is mainly for debugging, does it have a different
compatibility guarantee from other options?  Could it be named
accordingly to set expectations?

[...]
> +The form '--filter=blob:none' omits all blobs.

Sounds sensible.

> ++
> +The form '--filter=blob:limit=[kmg]' omits blobs larger than n bytes
> +or units.  The value may be zero.

On second thought, doesn't blob:limit=0 mean blob:none is not needed?
Is it for future consistency with tree:none?

What units do [kmg] use? Are they GB, GiB, or one of the variants in
between?

> ++
> +The form '--filter=sparse:oid=' uses a sparse-checkout
> +specification contained in the object (or the object that the expression
> +evaluates to) to omit blobs that would not be not required for a
> +sparse checkout on the requested refs.

This one makes me a little nervous because it would mean we're
planning on adding sparse-checkout specifications to the wire
protocol.  Maybe that's okay --- they're already part of the on-disk
format --- but it makes me nervous because the sparse-checkout format
is not so great, as I believe MS has already noticed.

What is an ?  Can it just say ?  How would this one
work when passed over the wire?

> ++
> +The form '--filter=sparse:path=' similarly uses a sparse-checkout
> +specification contained in .

Is this  relative to the cwd of the caller, or is it within some
commit?

Sorry it took so long to send this feedback / these questions.
Hopefully it's useful nevertheless.

Thanks and hope that helps,
Jonathan

Re: [PATCH v3 3/3] worktree: make add dwim

On 11/22, Junio C Hamano wrote:
> Thomas Gummerer  writes:
> 
> > I didn't consider that, I think you are right, and the flag should
> > apply in that case as well.  I think at that point we may as well pass
> > this flag through to the 'git branch' call, and let users set up
> > tracking if they want to, the same way it works in 'git branch'.
> 
> OK, so tracking is set up by default in the current implementation
> of "git worktree" (even without your proposed update), but we will
> stop doing so, and instead take an explicit "--track" option (or
> "--no-track" to countermand an earlier "--track" on the command line
> and/or a default configured with branch.autosetupmerge) just like
> "git branch" does?

I was a bit brief in the above.  The full story is that tracking is
set up by default if the '' given is a remote tracking branch,
and isn't set up otherwise, the same way as 'git branch' behaves.

What I'm planning to do is introduce a --[no-]track flag to override
this behaviour.  As 'git worktree' really just calls 'git branch'
internally, the branch.autoSetupMerge configuration is also
respected.

> I think that it is very sensible thing to make sure that "branch",
> "checkout -b" and "worktree", i.e. the three ways to create a branch
> to work on (the latter two being short-hands), behave consistently.

So in summary "branch" and "worktree" already behave consistently, the
plan is just to introduce the same "--[no-]track" flag as branch to
allow users to override the behaviour the same way as they are allowed
in "branch".

> Thanks.

Re: [PATCH v3 00/33] Add directory rename detection to git

On Tue, Nov 21, 2017 at 5:12 PM, Elijah Newren  wrote:
> On Tue, Nov 21, 2017 at 4:42 PM, Stefan Beller  wrote:
>> On Tue, Nov 21, 2017 at 12:00 AM, Elijah Newren  wrote:
>>> This patchset introduces directory rename detection to merge-recursive; I'm
>>> resubmitting just a few hours after my PATCHv2 because I didn't know about
>>> the DEVELOPER=1 flag previously, and my code had a number of
>>> warnings/errors.  I would have just submitted fixup/squash patches, but
>>> when I checked, there sadly they cause merge conflicts when rebasing
>>>
>>> See https://public-inbox.org/git/20171110190550.27059-1-new...@gmail.com/
>>> for the first series, design considerations, etc, and
>>> https://public-inbox.org/git/20171120220209.15111-1-new...@gmail.com/ for
>>> v2.
>>
>> Thanks, I'll take a look!
>>
>> Protip: To make it easy for reviewers add an interdiff[1] between the 
>> different
>> versions of the patch series, this can be done via tbdiff[2] for example,
>> or in case you still have the old branch around or Junio has it queued 
>> already,
>> you can do a diff against that branch.
>
> Thanks!
>
> Interesting; tbdiff looks cool.  Junio hasn't queued this series yet,
> but it's easy enough to reconstruct the old one.  It does weigh in
> pretty heavy, and I'm slighly worried about gmail mangling all the
> lines, but I'm going to give it a shot anyway.  If it's too mangled,
> I'll try to repost using git-send-email.  It does weigh in at over
> 1600 lines, so it's not small.

In my first round of review I only looked over the tests to see if I'd
find the behavior intuitive, I spared the implementation, as Junio seemed
to have reviewed a couple patches of the v1 implementation.

Now I also looked over the implementation and quite like it, though
I'd be happy if others would also have a look.

All but one comment were minor style nits, which are no big deal;
the other remark that I was musing about was whether we want to use
strbufs in the new code instead of e.g. sprintfs to extend strings.
And I'd think we would want to use them unless there are compelling
reasons not to.

Thanks,
Stefan

Re: Documentation of post-receive hook

2017-11-22 Thread Christoph Michelbach

On Wed, 2017-11-22 at 10:14 +0900, Junio C Hamano wrote:
> Your suggesting to mention that particular message hints at me that
> you feel that the users may not necessarily understand that push did
> not result in any update of references on the other side when they
> see it.  If the message was clear enough to them, "when it reacts to
> push and updates" ought to be clear enough description, too.
> 
> And if that indeed is the case (and I would not be surprised if it
> is, but I suspect that most users are clueful enough), it is not the
> documentation, but the "Already up-to-date" message, that needs to
> be clarified, no?
> 
> Besides, we'd rather not cast the end-user facing message in stone
> in the documentation like that (especially when the message has
> known room for improvement and will change).

You're right (with everything you wrote). Your proposed text sounds good.

Re: [PATCH v3 25/33] merge-recursive: check for file level conflicts then get new name

On Tue, Nov 21, 2017 at 12:00 AM, Elijah Newren  wrote:
> Before trying to apply directory renames to paths within the given
> directories, we want to make sure that there aren't conflicts at the
> file level either.  If there aren't any, then get the new name from
> any directory renames.
>
> Signed-off-by: Elijah Newren 
> ---
>  merge-recursive.c   | 192 
> ++--
>  t/t6043-merge-rename-directories.sh |   2 +-
>  2 files changed, 185 insertions(+), 9 deletions(-)
>
> diff --git a/merge-recursive.c b/merge-recursive.c
> index b8c7d6dce3..5bc207b819 100644
> --- a/merge-recursive.c
> +++ b/merge-recursive.c
> @@ -1496,6 +1496,109 @@ static void get_renamed_dir_portion(const char 
> *old_path, const char *new_path,
> }
>  }
>
> +/*
> + * Write:
> + *   element1, element2, element3, ..., elementN
> + * to str.  If only one element, just write "element1" to str.
> + */
> +static void comma_separated_list(char *str, struct string_list *slist)

This is quite a low level function, so I wondered if we have such functionality
already, but neither string-list.h nor strbuf.h present a drop-in replacement.
Speaking of strbufs, this might be another "big thing" to use in this series as
strbufs make using strings (and its memory management) easier in git.

This functionality could look like this (in strbuf.c):

/*
 * Adds all strings of a string list to the strbuf, separated by the
given separator.
 * For example a list of ("a", "b") with sep=";" would result in "a;b" added
 * to the strbuf.
 */
void strbuf_add_separated_string_list(struct strbuf *sb, struct
string_list *slist, const char *sep)
{
int add_sep = 0;
struct string_list_item *item;

for_each_string_list_item(item, s_list) {
if (add_sep)
strbuf_addstr(sb, sep);
strbuf_addstr(item->string);
add_sep = 1;
}
}

> +{
> +   int i;
> +
> +   for (i = 0; i < slist->nr; i++) {
> +   str += sprintf(str, "%s", slist->items[i].string);
> +   if (i < slist->nr-1)

style nit: blanks before and after '-';

> +   str += sprintf(str, ", ");
> +   }
> +}

Re: [PATCH v3 22/33] merge-recursive: check for directory level conflicts

> +   /* We can assume it's not rename/rename(1to1) because
> +* that was case (1), already checked above.  So we
> +* know that head_ent->new_dir and merge_ent->new_dir
> +* are different strings.
> +*/

micro nit:

/* we prefer comments like this */

/*
 * or like this
 */

/* but not
 * like this.
 */

Re: [PATCH v3 11/33] directory rename detection: testcases exploring possibly suboptimal merges

> +# Note: Both x and y got renamed and it'd be nice to detect both, and we do
> +# better with directory rename detection than git did previously, but the
> +# simple rule from section 5 prevents me from handling this as optimally as
> +# we potentially could.

...previously...

> +# Testcase 8e, Both sides rename, one side adds to original directory
> +#   Commit O: z/{b,c}
> +#   Commit A: y/{b,c}
> +#   Commit B: w/{b,c}, z/d
> +#
> +# Possible Resolutions:
> +#   Previous git: z/d, CONFLICT(z/b -> y/b vs. w/b), CONFLICT(z/c -> y/c vs. 
> w/c)

"Previous git" may be hard to understand when reviewing this code in 2 years.
The future proof term is "git without dir rename detection" or such.
(This is only a small nit, which on its own doesn't require a reroll;
I'll keep reading.)

Re: [PATCH 2/2] grep: fix segfault under -P + PCRE2 + (*NO_JIT)

2017-11-22 Thread Eric Sunshine

On Wed, Nov 22, 2017 at 8:36 AM, Ævar Arnfjörð Bjarmason
 wrote:
> Fix a bug in the compilation of PCRE2 patterns under JIT (the most
> common runtime configuration), any pattern with a (*NO_JIT) verb would
> segfault. This bug dates back to my 94da9193a6 ("grep: add support for
> PCRE v2", 2017-06-01):
>
> $ git grep -P '(*NO_JIT)hi.*there'
> Segmentation fault
>
> As explained ad more length in the comment being added here it isn't

s/ad/at/
s/here/here,/

> sufficient to just check pcre2_config() to see whether the JIT should
> be used, pcre2_pattern_info() also has to be asked.
>
> This is something I discovered myself when fiddling around with PCRE2
> verbs in patterns passed to git. I don't expect that any user of git
> has encountered this given the obscurity of passing PCRE2 verbs
> through to the library, along with the relative obscurity of (*NO_JIT)
> itself.
>
> Signed-off-by: Ævar Arnfjörð Bjarmason

Re: [RFC PATCH] builtin/worktree: enhance worktree removal

2017-11-22 Thread Eric Sunshine

On Wed, Nov 22, 2017 at 12:09 PM, Kaartic Sivaraam
 wrote:
> On Wednesday 22 November 2017 03:07 AM, Eric Sunshine wrote:
>> [1]: Excerpt from:
>> https://public-inbox.org/git/capig+cttrv2c7jlu1dr4+n8xo+7yq+deiwlda835wbgd6fh...@mail.gmail.com/
>>
>> Other information which would be nice to display for each worktree
>> [by the 'list' command] (possibly controlled by a --verbose flag):
>>
>> * the checked out branch or detached head
>> * whether it is locked
>>  - the lock reason (if available)
>>  - and whether the worktree is currently accessible
>>  * whether it can be pruned
>>  - and the prune reason if so
>
> It would nice to see this information. It would be very useful but 'list'
> doesn't seem to be the right sub-sub-command to give such information. Maybe
> there should be a new sub-sub-command named 'info' or something to give such
> information?

I'm perfectly fine with having 'git worktree list' provide this extra
information; that was the intention from the start. The "quick"
summary of all worktrees provided by 'git worktree list' is exactly
the sort of place where you're most likely to notice something
unexpected, such as the missing worktree directory. And, this extra
information doesn't have to be noisy:

--- 8< ---
% git worktree list
giggle 89ea799ffc [master]
../bobble  f172cb543d [feature1]  locked
../fumple  6453c84b7d (detached HEAD)  prunable
--- 8< ---

And, the verbose case:

--- 8< ---
% git worktree list -v
giggle 89ea799ffc [master]
../bobble  f172cb543d [feature1]
locked: worktree on removable media
../fumple  6453c84b7d (detached HEAD)
prunable: directory does not exist
--- 8< ---

An "info" command you speak of might also have value, but that's
orthogonal to this extra information that 'git worktree list' could
provide.

Re: [PATCH][l10n-fr] list translated to prune in command

Hi,

Louis Bettens wrote:

> "$ git worktree" when in a french locale shows an incorrect usage
> summary.  This comes down to this trivial issue in the i18n.

Good catch.  This comes from v2.7.0-rc3~4^2~7^2~2^2 (l10n: fr v2.7.0
round 1 (2477t), 2015-12-18).

For next time, you can send these three emails in a single email:

Subject: l10n: fr.po: "worktree list" mistranslated as prune

"$ git worktree" when in a french locale shows an incorrect usage
summary.  This comes down to this trivial issue in the i18n.

Signed-off-by: ...
---
Also it seems this is the only .po that has this particular quirk:

$ grep -c "worktree prune" po/*.po
po/bg.po:2
po/ca.po:2
po/de.po:2
po/es.po:2
po/fr.po:3  # outlier
po/is.po:0
po/it.po:0
po/ko.po:2
po/pt_PT.po:2
po/ru.po:2
po/sv.po:2
po/vi.po:2
po/zh_CN.po:2

zero lines -> translation missing, OK
two lines -> msgid and msgstr, OK
three lines -> something wrong. In this case, the present issue.

 po/fr.po | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/po/fr.po b/po/fr.po
index 4deae3318..a12a2ae37 100644
--- a/po/fr.po
+++ b/po/fr.po
[...]

That way, Jean-Noel Avila or Jiang Xin can apply the patch with a
single "git am" command and the explanation (above the "---" line)
shows up in "git log".

I assume they can handle this one fine as-is.  Just pointing it out
for the future.

See the DISCUSSION section in "git help format-patch" for more details.

Thanks,
Jonathan

Re: [PATCH v5 00/10] Partial clone part 2: fsck and promisors

2017-11-22 Thread Jonathan Tan

On Wed, 22 Nov 2017 14:25:13 +0900
Junio C Hamano  wrote:

> Jeff Hostetler  writes:
> 
> > From: Jeff Hostetler 
> >
> > This is V5 of part 2 of partial clone.  This assumes V5 of part 1
> > is already present.  V5 includes minor cleanup over V4 and better
> > separates the --exclude-promisor-objects and --missing arguments.
> >
> > Part 2 is concerned with fsck, gc, initial support for dynamic
> > object fetching, and tracking promisor objects.  Jonathan Tan
> > originally developed this code.  I have moved it on top of
> > part 1 and updated it slightly.
> 
> Thanks, will replace/queue all three series.  I am getting a feeling
> that the first one is already ready for 'next', while the other two
> may want to see a bit more comments?

Yes, I think so too.

Jeff Hostetler and I noticed some issues occuring when some other Git
commands dynamically fetch objects due to the fact that those commands
and fetch-pack use overlapping object flags. At the very least, we
should look at that before it goes into next.

Re: [PATCH v2] launch_editor(): indicate that Git waits for user input


On Wednesday 22 November 2017 10:25 PM, Lars Schneider wrote:



On 20 Nov 2017, at 01:11, Junio C Hamano  wrote:

Kaartic Sivaraam  writes:

It might be a good thing to keep the notice but I think it would be
better to have that error message in a "new line". I'm not sure if
it's possible or not.


Of course it is possible, if you really wanted to.  The code knows
if it gave the "we launched and waiting for you" notice, so it can
maintain not just one (i.e. "how I close the notice?") but another
one (i.e. "how I do so upon an error?") and use it in the error
codepath.


I think a newline makes sense. I'll look into this for v3.



If I remember correctly, I don't think it's as simple as printing a 
newline character in case of an error.  That's because the error message 
that shows up in the same line as "Launched your ..." comes from a 
different function (possibly finish_command() though I'm not sure)



Thanks,
Lars

Re: [RFC PATCH] builtin/worktree: enhance worktree removal



Kaartic: Regarding the actual patch, rather than silencing
validate_worktree() (which seems an unfortunate thing to do), isn't it
possible simply to do a quick test to see if the worktree directory
exists before calling validate_worktree()? If it doesn't exist, then
just skip down to the part of the code which does the 'prune'
operation.



Nice suggestion. It seems to be a much better thing to do than silencing 
'validate_worktree' (it was an "ad-hoc" patch, you see). I'll send an 
update incorporating this suggestion (of course, with the suggested 
commit message), when I find time.


Thanks,
Kaartic

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


On Wednesday 22 November 2017 07:39 AM, Junio C Hamano wrote:

Eric Sunshine  writes:


On Tue, Nov 21, 2017 at 2:12 PM, Kaartic Sivaraam
 wrote:

On Wednesday 22 November 2017 12:08 AM, Eric Sunshine wrote:

The original code unconditionally uses "+ 11", which says that the
prefix is _always_ present. This commit message muddies the waters [...]


That muddiness of that statement is a consequence of my recent encounter[1]
in which the assumption (that the prefix(refs/heads/ always exists) of that
code failed. I had a little suspicion, when I wrote that commit message,
that there might be other cases in which assumption might fail. The issue
has been resolved only in 3/4 of jc/branch-name-sanity but that was only
after I wrote the commit message initially.  So, it does make sense to
remove that muddiness now. Thanks for noting that.

[1]: Note the 'warning: ' message in the following mail. That ugly '|�?' was
a consequence of the assumption that the 'prefix' always existed!
https://public-inbox.org/git/1509209933.2256.4.ca...@gmail.com/


Thanks for the explanation and history reference.


I have a suspicion that it wasn't the case.  The ugly uninitialized
byte sequence came from an error codepath of a function that is asked
to fill a strbuf with "refs/heads/$something" returning early when it
found an error in the input, without realizing that the caller still
looks at the strbuf even when it got an error.  That caller-callee pair
was updated.



You seem to be right when viewing from the perspective of the callee 
(strbuf_check_branch_ref). IOW, you *seem* to be telling that the 
"callee" should have known that the caller was expecting the 'prefix' 
even in case of an error and should have "inserted" it regardless of the 
error (I thought the strbuf was initialized and contained the result of 
strbuf_branchname even in the case of an error) ?


I thought that the 'caller' should have known that the 'callee' would 
not insert the prefix when there was an error in the branch name thus it 
should have anticipated that there would be a case in which the prefix 
(refs/heads/) doesn't exist in the buf (the assumption).




It is just a bug and +11 is always correct; passing the data that
does not begin with "refs/heads/" there, including the case where an
uninitialized buffer is passed, is simply a bug.


Let me see if I could correlate your statement with that error. AFAIK, I 
don't think the buffer was uninitialized in the case of that error. It 
had in it the result of interpretation of 'strbuf_branchname', didn't 
it? So that clears one case and leads me to the interpretation that,


"Passing the data (at an offset of 11 bytes from the beginning of the 
buf) that does not begin with "refs/heads/" is a bug"


Which seems to be in line with my statement that it was wrong (in the 
part of the "caller") to assume that "strbuf_check_branch_ref" always 
returns a buf with the prefix "refs/heads/" ?





In other words, skip_prefix() is a good change, but if we are to use
it, we should also check the result and error out if we do not see
"refs/heads/" there--you already bothered to spend extra cycles for
that.



Makes sense. I should have better done this initially instead of giving 
a muddy justification for not doing it.



Thanks,
Kaartic

Re: [PATCH] defer expensive load_ref_decorations until needed

On Tue, Nov 21, 2017, Junio C Hamano  wrote:
> Junio C Hamano  writes:
>
> I am not sure if "maybe_" is a good name here, though.  If anything,
> you are making the semantics of "load_ref_decorations()" to "maybe"
> (but I do not suggest renaming that one).
>
> How about calling it to load_ref_decorations_lazily() or something?

I groped about for something conventional, but "..._gently" didn't fit
the bill, so I went with "maybe".  I like "lazily" better for this
case. I will change it for v2.

>> Other than that, I like what this patch attempts to do.  A nicely
>> identified low-hanging fruit ;-).
>
> Having said that, this will have a bad interaction with another
> topic in flight: <20171121213341.13939-1-rafa.al...@gmail.com>
>
> Perhaps this should wait until the other topic lands and stabilizes.
> We'd need to rethink if the approach taken by this patch, i.e. to
> still pass the info to load() but holding onto it until the time
> lazy_load() actually uses it, is a sensible way forward, or we would
> want to change the calling convention to help making it easier to
> implement the lazy loading.

I noticed that after just after cleaning this one up, but didn't look
closely yet.  I'll hold this in my local queue until ra lands.

P

Re: [PATCH] fix french translation


Also it seems this is the only .po that has this particular quirk:

$ grep -c "worktree prune" po/*.po
po/bg.po:2
po/ca.po:2
po/de.po:2
po/es.po:2
po/fr.po:3  # outlier
po/is.po:0
po/it.po:0
po/ko.po:2
po/pt_PT.po:2
po/ru.po:2
po/sv.po:2
po/vi.po:2
po/zh_CN.po:2

zero lines -> translation missing, OK
two lines -> msgid and msgstr, OK
three lines -> something wrong. In this case, the present issue.


Le mer 22 nov 2017 à 18:24, Louis Bettens  a 
écrit :

Signed-off-by: Louis Bettens 
---
 po/fr.po | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/po/fr.po b/po/fr.po
index 4deae3318..a12a2ae37 100644
--- a/po/fr.po
+++ b/po/fr.po
@@ -14331,7 +14331,7 @@ msgstr "git worktree add []  
[]"


 #: builtin/worktree.c:17
 msgid "git worktree list []"
-msgstr "git worktree prune []"
+msgstr "git worktree list []"

 #: builtin/worktree.c:18
 msgid "git worktree lock [] "
--
2.15.0

Re: [PATCH v2] launch_editor(): indicate that Git waits for user input

2017-11-22 Thread Eric Sunshine

On Wed, Nov 22, 2017 at 11:53 AM, Lars Schneider
 wrote:
>> On 17 Nov 2017, at 20:41, Eric Sunshine  wrote:
>> * emacsclient already prints its own message ("Waiting for Emacs...",
>> which runs together with Git's message). Perhaps treat emacsclient as
>> a special case and skip printing this message if emacsclient is in
>> use: if (strstr(...,"emacsclient"))
>
> If Junio et al. are ok with the special handling of emacs, then I am happy
> to add this change in v3. If we look for "emacsclient", then would this
> cover emacs on Linux and Windows, too? (I am no emacs user)

Yes, searching for "emacsclient" should work on all platforms (Linux,
MacOS, Windows, BSD). Most of the time, the full value of EDITOR is
just "emacsclient", but sometimes emacsclient is located at an
out-of-the-way location, not in PATH, such as in my example
(/long/path/to/emacsclient). On Windows, it's actually called
"emacsclientw", but that should be matched just fine when searching
for "emacsclient".

[PATCH] fix french translation

Signed-off-by: Louis Bettens 
---
 po/fr.po | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/po/fr.po b/po/fr.po
index 4deae3318..a12a2ae37 100644
--- a/po/fr.po
+++ b/po/fr.po
@@ -14331,7 +14331,7 @@ msgstr "git worktree add []  
[]"
 
 #: builtin/worktree.c:17
 msgid "git worktree list []"
-msgstr "git worktree prune []"
+msgstr "git worktree list []"
 
 #: builtin/worktree.c:18
 msgid "git worktree lock [] "
-- 
2.15.0

[PATCH][l10n-fr] list translated to prune in command

"$ git worktree" when in a french locale shows an incorrect usage
summary.  This comes down to this trivial issue in the i18n.

Re: [RFC PATCH] builtin/worktree: enhance worktree removal


On Wednesday 22 November 2017 09:25 AM, Junio C Hamano wrote:

Eric Sunshine  writes:

So, Kaatic's patch is intended to address that oversight (though I
haven't examined the implementation closely; I was just trying to
understand the reason for the patch).


OK, so the proposed log message was a bit confusing for those who
are *not* the person who wrote it (who knew why existing behaviour
was inadequate and did not describe how "worktree remove" would fail
under such a scenario to illustrate it, incorrectly assuming that
everybody who reads the proposed log message already *knows* how it
would fail).


I shouldn't have made the log message as 'ad hoc' as I made the patch, 
sorry :-(




"git worktree remove" removes both the named worktree
directory and the administrative information for it after
checking that there is no local modifications that would be
lost (which is a handy safety measure).  It however refuses
to work if the worktree directory is _already_ removed.

The user could use "git worktree prune" after seeing the
error and realizing the situation, but at that point, there
is nothing gained by leaving only the administrative data
behind.  Teach "git worktree remove" to go ahead and remove
the trace of the worktree in such a case.

or soemthing like that?



Much better!

Re: [RFC PATCH] builtin/worktree: enhance worktree removal

On Wednesday 22 November 2017 03:07 AM, Eric Sunshine wrote:

On Tue, Nov 21, 2017 at 10:09 AM, Kaartic Sivaraam
wrote:

The new feature to 'remove' worktree was handy to remove specific
worktrees. It didn't cover one particular case of removal. Specifically,
if there is an "entry" (a directory in /.git/worktrees)
for a worktree but the worktree repository itself does not exist then
it means that the "entry" is stale and it could just be removed.

So, in case there's a "worktree entry" but not "worktree direectory"
then just remove the 'stale' entry.

Let me see if I understand. Sometimes you know that you've deleted the
worktree directory, in which case 'git worktree prune' is the obvious
choice. However, there may be cases when you've forgotten that you
deleted the worktree directory (or it got deleted some other way), yet
it still shows up in "git worktree list" output with no indication
that it has been deleted (though, ideally, it should tell you so[1]).
In this case, you see a worktree that you know you no longer want, so
you invoke "git worktree remove" but that errors out because the
actual directory is already gone. This patch make the operation
succeed, despite the missing directory. Is that correct?

Yes. My primary motive for sending this is that, I thought it didn't
make much sense for 'remove' to fail just because the directory didn't
exist. Further, I thought it would be more flexible to allow for
'selective' pruning of worktrees. The use case in which, I thought, this
flexibility might prove helpful is that of 'scripts'.

Consider a hypothetical script that plays around with a git repository.
Further, consider that spawns a new worktree for temporary use. It's
quite natural for the script to consider cleaning up it's own mess and
thus it might consider removing the worktree it created. With the
'remove' command it is as easy is,

git worktree remove

But what if it was the case that the directory might/might not exist
when that part of the script is reached. In case the directory didn't
exist the 'remove' command errors out that it didn't exist instead of
just removing the worktree "entry" for that . I thought it
wasn't a good thing and thus this script.

After writing this, I feel I haven't justified it well (impostor
syndrome, possibly). Anyways, at the end of the day this "ad-hoc" patch
was just a result of my gut feeling that, "It didn't make sense for the
'remove' command to fail when the directory didn't exist". The
implementation is just a "sloppy" illustration of how it could be
achieved and should "better" not be used as such.

[1]: Excerpt from:
https://public-inbox.org/git/capig+cttrv2c7jlu1dr4+n8xo+7yq+deiwlda835wbgd6fh...@mail.gmail.com/

Other information which would be nice to display for each worktree
[by the 'list' command] (possibly controlled by a --verbose flag):

* the checked out branch or detached head
* whether it is locked
- the lock reason (if available)
- and whether the worktree is currently accessible
* whether it can be pruned
- and the prune reason if so

It would nice to see this information. It would be very useful but
'list' doesn't seem to be the right sub-sub-command to give such
information. Maybe there should be a new sub-sub-command named 'info' or
something to give such information?

Re: Bisect marking new commits incorrectly

2017-11-22 Thread Adam Dinwoodie

On Wednesday 22 November 2017 at 05:21 pm +0100, Christian Couder wrote:
> On Wed, Nov 22, 2017 at 3:39 PM, Adam Dinwoodie  wrote:
> > In trying to do a bisect on the Git repository, I seem to have come
> > across surprising behavior where the order in which `git bisect` appears
> > to forget that previous commits were marked as new.
> 
> Yeah, the algorithm uses many "old" commits and only one "new" commit.
> 
> [...]
> 
> > As you can see, in both cases, only the most recent "new" command
> > appears to have any effect.  I'd have expected that both commits would
> > have been marked as "new", and the bisect run would use both facts to
> > work out which commit is the target of the bisection.
> 
> I didn't look at your test case, but the algorithm to find another
> commit to test is described here:
> 
> https://git-scm.com/docs/git-bisect-lk2009.html#_bisection_algorithm
> 
> and you can see that the first rule of the algorithm is to keep only
> the commits that are ancestors of the "new" commit (including the
> "new" commit itself).
> 
> The reason for this rule is that other commits cannot have introduced
> the behavior we are looking for. The result of this rule is that git
> bisect will always ask you to test a commit that is an ancestor of the
> "new" commit.
> 
> So if you test a commit that git bisect asks you to test, and it
> appears that this commit is "new", then you can just discard the
> previous "new" commit because it will give you less information than
> the new "new" one.
> (The old "new" will not let you discard any commits that the new "new"
> commit allows you to discard, because it is a descendant of the new
> "new" commit.)
> 
> If you don't test the commit that git bisect asks you to test, then
> git bisect assumes that you know better and discards the old "new".

Ah, that makes sense, thank you for the explanation.

In my case, I knew two commits were "new", but didn't know which would
provide more information to the bisect algorithm; I'd assumed if I
provided both, Git would work out the appropriate thing to do with the
combined information without me needing to work it out.

> > This is using v2.15.0.  It's possibly relevant that 95a731ce is a
> > direct parent of 14c63a9d.
> >
> > Is this a bug, or intentional behaviour?  Am I missing something that
> > means it's actually sensible to have Git silently discard some bisect
> > commands in this sort of circumstance?
> 
> Well, instead of silently discarding a the old "new" commit when the
> new "new" commit is not an ancestor of it, git bisect could perhaps
> warn or ask you to confirm that you know what you are doing.

Warning the user seems a sensible suggestion to me.  I'll add it to my
list of things to spend some time on at some point in the future :)

Re: [PATCHv5 7/7] builtin/describe.c: describe a blob

On Tue, Nov 21, 2017 at 11:55 PM, Junio C Hamano  wrote:
> Stefan Beller  writes:
>
>> ...
>> fixed.
>> ...
>> fixed the text...
>> ...
>> I am not fully convinced all descriptions are in recent history, but I
>> tend to agree that most are, so probably the trade off is a wash.
>
> So what do we want with this topic?  I think the "teach 'git log' to
> highlight commits whose changes involve the given blob" is a more or
> less an orthogonal thing,

Well, both of them solve our immediate needs, so I'd be fine with pursuing
just one of them, but I do not oppose taking both.

> and I suspect that it is something users
> may (although I personally do not) find valuable to have a related
> but different feature in "git describe".

agreed.

Re: [PATCH v3 0/4] Hash abstraction

On Tue, Nov 21, 2017 at 11:50 PM, Junio C Hamano  wrote:
> Junio C Hamano  writes:
>
>> "brian m. carlson"  writes:
>>
>>> This is a series proposing a basic abstraction for hash functions.
>>>
>>> The full description of the series can be found here:
>>> https://public-inbox.org/git/20171028181239.59458-1-sand...@crustytoothpaste.net/
>>>
>>> At the bottom of this message is the output of git tbdiff between v2 and
>>> v3.
>>>
>>> Changes from v2:
>>> * Remove inline.
>>> * Add dummy functions that call die for unknown hash implementation.
>>> * Move structure definitions to hash.h and include git-compat-util.h in
>>>   hash.h.
>>> * Rename current_hash to the_hash_algo.
>>> * Use repo_set_hash_algo everywhere we set the hash algorithm for a
>>>   struct repository.
>>
>> Change for all of the above in this series looked sensible to me.
>> Thank, will queue.
>
> So... is everybody happy enough with this version that nobody minds
> more codebase to be adjusted on the infrastructure this series lays
> out?  I think this is ready for 'next', but just in case I missed
> some discussions...

yeah I was happy at the time of review; sorry for not saying so.

Re: [PATCH v2] launch_editor(): indicate that Git waits for user input


> On 20 Nov 2017, at 01:11, Junio C Hamano  wrote:
> 
> Kaartic Sivaraam  writes:
> 
 However, it's not clear how much benefit you gain from stashing this
 away in a static variable. Premature optimization?
>>> 
>>> The variable being "static" could be (but it was done primarily
>>> because it allowed me not to worry about freeing),
> 
> The current code happens to be safe because I do not allocate.  I do
> not know what others will do to the code in the future, and at that
> point, instinct kicks in to futureproof against the worst ;-).
> 
 Should printing of close_notice be done before the error()? Otherwise,
 you get this:
 
 --- 8< ---
 Launched your editor (...) ...There was a problem...
 --- 8< ---
>>> 
>>> In my version with a far shorter message, I deliberately chose not
>>> to clear the notice.  We ran the editor, and we saw a problem.  That
>>> is what happened and that is what will be left on the terminal.
>>> 
>> 
>> It might be a good thing to keep the notice but I think it would be
>> better to have that error message in a "new line". I'm not sure if
>> it's possible or not.
> 
> Of course it is possible, if you really wanted to.  The code knows
> if it gave the "we launched and waiting for you" notice, so it can
> maintain not just one (i.e. "how I close the notice?") but another
> one (i.e. "how I do so upon an error?") and use it in the error
> codepath.

I think a newline makes sense. I'll look into this for v3.

Thanks,
Lars

Re: [PATCH v2] launch_editor(): indicate that Git waits for user input


> On 17 Nov 2017, at 20:41, Eric Sunshine  wrote:
> 
> On Fri, Nov 17, 2017 at 8:51 AM,   wrote:
> 
>> +   char *term = getenv("TERM");
>> +
>> +   if (term && strcmp(term, "dumb"))
>> +   /*
>> +* go back to the beginning and erase the
>> +* entire line if the terminal is capable
>> +* to do so, to avoid wasting the vertical
>> +* space.
>> +*/
>> +   close_notice = "\r\033[K";
>> +   else
>> +   /* otherwise, complete and waste the line */
>> +   close_notice = "done.\n";
>> +   }
>> +
>> +   if (close_notice) {
>> +   fprintf(
>> +   stderr,
>> +   "Launched your editor ('%s'). Adjust, save, 
>> and close the "
>> +   "file to continue. Waiting for your input... 
>> ", editor
>> +   );
> 
> Here's what this looks like for me:
> 
> --- 8< ---
> Launched your editor
> ('/Applications/Emacs.app/Contents/MacOS/bin/emacsclient'). Adjust,
> save, and close the file to continue. Waiting for your input...
> Waiting for Emacs...
> --- 8< ---
> 
> Very, very noisy, so much so that it's almost unreadable. There are at
> least three reasons for the noise:
> 
> * The raw message itself is already overly long. Do we really need to
> assume that newcomers are so clueless that they need it spelled out to
> such a level of detail? "Launched editor" should be enough for most
> people, and one would hope that "Launched editor; waiting for
> input..." would be enough for the rest.
> 
> * Does not take into consideration that EDITOR might be very long;
> perhaps you could just print the basename and strip arguments (i.e.
> "/my/long/path/edit -x --foo --zap" becomes "edit"). Or, just omit the
> editor altogether.

Agreed already in another reply here:
https://public-inbox.org/git/0dd1ee8a-47f1-41aa-8f86-c9fdf99d2...@gmail.com/

> 
> * emacsclient already prints its own message ("Waiting for Emacs...",
> which runs together with Git's message). Perhaps treat emacsclient as
> a special case and skip printing this message if emacsclient is in
> use: if (strstr(...,"emacsclient"))

If Junio et al. are ok with the special handling of emacs, then I am happy 
to add this change in v3. If we look for "emacsclient", then would this 
cover emacs on Linux and Windows, too? (I am no emacs user)


> 
> And, of course, with a "dumb" terminal, it's even noisier with the
> extra "done." at the end:
> 
> --- 8< ---
> Launched your editor
> ('/Applications/Emacs.app/Contents/MacOS/bin/emacsclient'). Adjust,
> save, and close the file to continue. Waiting for your input...
> Waiting for Emacs...
> done.
> --- 8< ---
> 
> As Junio pointed out in [1], emacsclient has already emitted a
> newline, so the clear-line sequence is ineffective; likewise, for a
> dumb terminal, "done." ends up on its own line. Aside from the noise,
> this also suggests making a special case for emacsclient.
> 
> And, as Junio pointed out in [2], with a message so long, once it has
> wrapped, the clear-line sequence does not work as intended. For those
> of us with 80-column terminals, we're left with a bunch of noise on
> the screen.
> 
>> +   fflush(stderr);
>> +   }
>> 
>>p.argv = args;
>>p.env = env;
>> @@ -53,11 +79,14 @@ int launch_editor(const char *path, struct strbuf 
>> *buffer, const char *const *en
>>sig = ret - 128;
>>sigchain_pop(SIGINT);
>>sigchain_pop(SIGQUIT);
>> +
>>if (sig == SIGINT || sig == SIGQUIT)
>>raise(sig);
>>if (ret)
>>return error("There was a problem with the editor 
>> '%s'.",
>>editor);
>> +   if (close_notice)
>> +   fputs(close_notice, stderr);
> 
> Should printing of close_notice be done before the error()? Otherwise,
> you get this:
> 
> --- 8< ---
> Launched your editor (...) ...There was a problem...
> --- 8< ---

I think we should keep it as I agree with Junio's argument here:
https://public-inbox.org/git/xmqqh8tsqs83@gitster.mtv.corp.google.com/

- Lars

Re: [PATCH v2] launch_editor(): indicate that Git waits for user input


> On 17 Nov 2017, at 19:40, Junio C Hamano  wrote:
> 
> lars.schnei...@autodesk.com writes:
> 
>> Junio posted the original version of this patch [1] as response to my RFC 
>> [2].
>> I took Junio's patch and slightly changed the commit message as well as the
>> message printed to the user after GIT_EDITOR is invoked [3].
>> 
>> Thanks,
>> Lars
> 
> Thanks.
> 
>> diff --git a/editor.c b/editor.c
>> index 7519edecdc..23db92d8c6 100644
>> --- a/editor.c
>> +++ b/editor.c
>> @@ -40,6 +40,32 @@ int launch_editor(const char *path, struct strbuf...
>> ...
>> +if (close_notice) {
>> +fprintf(
>> +stderr,
>> +"Launched your editor ('%s'). Adjust, save, and 
>> close the "
>> +"file to continue. Waiting for your input... ", 
>> editor
> 
> How wide is your typical terminal window?  With message this long, a
> sample standalone program I used while developing the prototype of
> this feature no longer can retract this "temporary" message.
> 
> Would something shorter like "Waiting for you to finish editing..."
> work well enough?

Yeah, Eric criticized the verbosity elsewhere, too. I understand your point
of view. Let's revert it to your initial short version. 

- Lars

> 
> -- -- --
> #include 
> 
> int main(void)
> {
>   const char *EL = "\033[K"; /* Erase in Line */
>   const char *editor = "emacsclient";
> 
>   fprintf(
>   stderr,
>   "Launched your editor ('%s'). Adjust, save, and close the "
>   "file to continue. Waiting for your input... ", editor);
>   fflush(stderr);
>   sleep(2);
>   fprintf(stderr, "\r%s", EL);
>   fflush(stderr);
>   return 0;
> }
> -- -- --
>

Re: [ANNOUNCE] Git Rev News edition 33


> On 22 Nov 2017, at 15:00, Christian Couder  wrote:
> 
> Hi everyone,
> 
> The 33rd edition of Git Rev News is now published:
> 
>  https://git.github.io/rev_news/2017/11/22/edition-33/
> 
> Thanks a lot to all the contributors and helpers!
> 
> Enjoy,
> Christian, Thomas, Jakub and Markus.

Thanks Christian! I really enjoy reading your summaries and, in particular,
the developer spotlight!

- Lars

Re: doc: prefer 'stash push' over 'stash save'

You probably already noticed this was my fault for filtering the patch
through Gmail's GUI.  I did also push a replacement which hopefully
does apply.

On Tue, Nov 21, 2017 at 8:39 PM, Junio C Hamano  wrote:
> Jonathan Nieder  writes:
>
>> Phil Hord wrote:
>>
>>> Although `git stash save` was deprecated recently, some parts of the
>>> documentation still refer to it instead of `push`.
>>>
>>> Signed-off-by: Phil Hord 
>>> ---
>>>  Documentation/git-stash.txt | 4 ++--
>>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> Reviewed-by: Jonathan Nieder 
>> Thanks.
>
> Heh, this does not even apply to 8be661007 that it claims to apply
> on top of, which is contained in fd2ebf14 ("stash: mark "git stash
> save" deprecated in the man page", 2017-10-22).
>
> I've wiggled it in, so there is no need to resend, but next time
> please be careful when sending the patch and also when sending a
> reviewed-by.

Re: git status always modifies index?

2017-11-22 Thread Santiago Torres

Ah, my bad. I missed this patch...

Good luck!
-Santiago.


signature.asc
Description: PGP signature

Re: Bisect marking new commits incorrectly

2017-11-22 Thread Christian Couder

On Wed, Nov 22, 2017 at 3:39 PM, Adam Dinwoodie  wrote:
> In trying to do a bisect on the Git repository, I seem to have come
> across surprising behavior where the order in which `git bisect` appears
> to forget that previous commits were marked as new.

Yeah, the algorithm uses many "old" commits and only one "new" commit.

[...]

> As you can see, in both cases, only the most recent "new" command
> appears to have any effect.  I'd have expected that both commits would
> have been marked as "new", and the bisect run would use both facts to
> work out which commit is the target of the bisection.

I didn't look at your test case, but the algorithm to find another
commit to test is described here:

https://git-scm.com/docs/git-bisect-lk2009.html#_bisection_algorithm

and you can see that the first rule of the algorithm is to keep only
the commits that are ancestors of the "new" commit (including the
"new" commit itself).

The reason for this rule is that other commits cannot have introduced
the behavior we are looking for. The result of this rule is that git
bisect will always ask you to test a commit that is an ancestor of the
"new" commit.

So if you test a commit that git bisect asks you to test, and it
appears that this commit is "new", then you can just discard the
previous "new" commit because it will give you less information than
the new "new" one.
(The old "new" will not let you discard any commits that the new "new"
commit allows you to discard, because it is a descendant of the new
"new" commit.)

If you don't test the commit that git bisect asks you to test, then
git bisect assumes that you know better and discards the old "new".

> This is using v2.15.0.  It's possibly relevant that 95a731ce is a
> direct parent of 14c63a9d.
>
> Is this a bug, or intentional behaviour?  Am I missing something that
> means it's actually sensible to have Git silently discard some bisect
> commands in this sort of circumstance?

Well, instead of silently discarding a the old "new" commit when the
new "new" commit is not an ancestor of it, git bisect could perhaps
warn or ask you to confirm that you know what you are doing.

Re: git status always modifies index?

2017-11-22 Thread Nathan Neulinger

I just got an answer to my stackoverflow question on this, apparently it's
already implemented:

https://stackoverflow.com/questions/47436939/how-to-run-git-status-without-modifying-git-index-such-as-in-a-prompt-command

There is a "--no-optional-locks" command in 2.15 that looks like it does
exactly what I need.

-- Nathan

On 11/22/17 10:10 AM, Santiago Torres wrote:

On Wed, Nov 22, 2017 at 09:37:09AM -0600, Nathan Neulinger wrote:

What I'm meaning is - why does it need to write the index back out to disk?

From looking at the code in builtin/commit.c it looks like it takes a lock
on the index, collects the status, and then unconditionally rewrites the
index file.

Hmm, I just took a look at those lines and I see what you mean. From
what I understand, this is to cache the result of the index computation
for ensuing git calls.

I'm proposing that the update_index_if_able call not actually be issued if
it would result in a ownership change on the underlying file - such as a
simple case of root user or other privileged account issuing 'git status' in
a directory.

I don't think this would be a desire-able default behavior. I'd wager
that running git status using different accounts is not a great idea ---
specially interacting with a user repository as root.

I understand completely that it would be expected to be altered if the
privileged user did a commit/add or any other operation that was inherently
a 'write' operation, but doesn't seem like status should be one of those
cases.

I think it's because of the reasons above. That being said, I don't know
what the rest of the community would think of something akin to a
--no-update-index type flag.

Cheers!
-Santiago.

Nathan Neulinger nn...@neulinger.org
Neulinger Consulting (573) 612-1412

Re: git status always modifies index?

2017-11-22 Thread Santiago Torres

On Wed, Nov 22, 2017 at 09:37:09AM -0600, Nathan Neulinger wrote:
> What I'm meaning is - why does it need to write the index back out to disk?
> 
> From looking at the code in builtin/commit.c it looks like it takes a lock
> on the index, collects the status, and then unconditionally rewrites the
> index file.
>
Hmm, I just took a look at those lines and I see what you mean. From
what I understand, this is to cache the result of the index computation
for ensuing git calls.

> I'm proposing that the update_index_if_able call not actually be issued if
> it would result in a ownership change on the underlying file - such as a
> simple case of root user or other privileged account issuing 'git status' in
> a directory.

I don't think this would be a desire-able default behavior. I'd wager
that running git status using different accounts is not a great idea ---
specially interacting with a user repository as root.

> I understand completely that it would be expected to be altered if the
> privileged user did a commit/add or any other operation that was inherently
> a 'write' operation, but doesn't seem like status should be one of those
> cases.

I think it's because of the reasons above. That being said, I don't know
what the rest of the community would think of something akin to a
--no-update-index type flag.

Cheers!
-Santiago.


signature.asc
Description: PGP signature

Re: [PATCH v2] doc: Add missing "-n" (dry-run) option to reflog man page

"Robert P. J. Day"  writes:

>   i don't believe "git reflog" supports "-v", only "--verbose", or am
> i misreading the source?

Ah, then at least you are being (locally) consistent.  Sorry for a
remembo.

As to (global) consistency, in the oldest time we tried to list
every options (but only in the fully spelled form, IIRC) in the
synopsis section, but later with the proliferation of options, it
became more common to use the generic "git subcmd ..." form
in synopsis and leave not just the description but also the
enumeration of the options to the description part of the docs, so
we ended up with a mixture in the documentation set.  Aiming for
global consistency is a very good thing, and we need to decide what
model to adhere to and stick to it to the end.  If you want to carry
the torch and declare that we would list every option and every
short-hand (but not "--v", "--ve", "--ver", ..."--verbose" for
obvious reasons), I personally am fine with that.

RE: [PATCH 1/2] Documentation about triangular workflow

2017-11-22 Thread ALBERTIN TIMOTHEE p1514771



Daniel Bensoussan  writes:

>> +TRIANGULAR WORKFLOW
>> +---
>> +
>> +Introduction
>> +
>> +
>> +In some projects, contributors cannot push directly to the project but
>> +have to suggest their commits to the maintainer (e.g. pull requests).
>> +For these projects, it's common to use what's called a *triangular
>> +workflow*:
>> + ...
>> +Motivations
>> +~~~
>> +
>> +* Allows contributors to work with Git even if they don't have
>> +write access to **UPSTREAM**.
>> +
>> +Indeed, in a centralized workflow, a contributor without write access
>> +could write some code but could not send it by itself.  The contributor
>> +was forced to create a mail which shows the difference between the
>> +new and the old code, and then send it to a maintainer to commit
>> +and push it.  This isn't convenient at all, neither for the
>> +contributor, neither for the maintainer. With the triangular
>> +workflow, the contributors have the write access on **PUBLISH**
>> +so they don't have to pass upon maintainer(s).  And only the
>> +maintainer(s) can push from **PUBLISH** to **UPSTREAM**.
>> +This is called a distributed workflow (See "DISTRIBUTED WORKFLOWS"
>> +above).

>I probably should not be judging if these additions to
>gitworkflows.txt is a good idea in the first place without seeing
>any explanation as to why this patch is here, but I think it misses
>the place where "triangular" sits in a larger picture.

There already have been a discussion about this documentation:
https://public-inbox.org/git/e83a9439-54c8-4925-8ee3-6aeedd941...@grenoble-inp.org/
We forgot to add it to the commit message, it will be in the next
commit message.

>The workflow to contrast against to illustrate the motivation is a
>centralized workflow, where everybody pushes their updates to a
>single place.  It does have problems inherent to its structure
>(e.g. "review before integration" is much harder, if possible), and
>also has its merits (e.g. it is simpler to explain and reason
>about).

>If you want to wean a project off of the centralized model, you'd
>need to use the "distributed workflow".  The workflow to review and
>apply mailed patches in public, and the workflow to have the project
>pull from many publish repositories individual contributor has, are
>two that allows the project to go distributed.  These two are
>complementary choices with pros and cons, and it is not like one is
>an improvement of the other.  Projects like the kernel even uses
>hybrid of the two---the patches are reviewed in public at central
>places (i.e. subsystem mailing lists) in an e-mail form and go
>through iterations getting polished, and the polished results are
>collected by (sub)maintainers and sent upwards, either as a request
>to pull from publish repositories maintained by (sub)maintainers, or
>relayed again in e-mail form (the last mile being e-mail primarily
>serves as a transport vehicle for changes proven to be good, not as
>material to be further reviewed).

>The reason why projects make these choices is because there are pros
>and cons.  A large collection of changes is far easier to integrate
>with one command (i.e. "git pull") and with a need to resolve merge
>conflicts just once, than applying many small changes as e-mailed
>patches, having to resolve many conflicts along the way.  In order
>to ensure quality of the individual changes, however, the changes
>need to be reviewed and polished, and the reality of the life is
>that there are far fewer people who are qualified to adequately
>review and help polishing the changes than those who make changes.
>Asking reviewers to go to different repositories (whose number
>scales with the number of contributors) and leave comments in the
>webforms is much less efficient and more costly for the project
>overall, than asking them to subscribe to relevant mailing lists
>(whose number scales only with the number of areas of interest) and
>conduct reviews there.  Other factors like "offline access" also
>count when considering the two models as "choices".

>As long as the document uses phrases like "forced to", "isn't
>convenient at all", etc., it is clear that it starts from a wrong
>premise, "one is an improvement over the other".

We will take this into account.
We didn't know there were hybrid workflows.

Thank you for your time

Timothée Albertin

Re: [PATCH 1/1] doc: Mention info/attributes in gitrepository-layout