Re: [PATCH 2/5] ieot: default to not writing IEOT section

[Junio C Hamano]

Stefan Beller  writes:

>> > +static int record_ieot(void)
>> > +{
>> > + int val;
>> > +
>>
>> Initialize stack val to zero to ensure proper default.
>
> I don't think that is needed here, as we only use `val` when
> we first write to it via git_config_get_bool.

Yup.

Re: [ANNOUNCE] Git v2.20.0-rc1

[Junio C Hamano]

Elijah Newren  writes:

> If we don't set rebase.useBuiltin to false, then there is also a minor
> regression in the error message printed by the built-in rebase we may
> want to try to address.  I have a patch for it at
> <20181122044841.20993-2-new...@gmail.com>, but I don't know if that
> patch is acceptable as-is this close to a release since that'd not
> give translators much time to update.

For this particular one, I'd rather ship "rebase in C" with known
message glitch, with or without the "mark it experimental and make
it opt-in" last-time change.  Of course, turning it off by default
would let us worry about these message glitches even less, but at
this point, I'd be worried more about bugs that can affect the
actual operation and outcome recorded in the resulting history.

Re: [RFC] Introduce two new commands, switch-branch and restore-paths

[Junio C Hamano]

Stefan Beller  writes:

> Maybe we need to step back and consider what the command does.
> And from that I would name it "rewire-HEAD-and-update-index"
> and then simplify from there. For the beginner user, the concept of
> HEAD might be overwhelming, such that we don't want to have that
> in there.

I'd have to say that it is totally backwards.

Use of HEAD, ref, etc. is merely a means to what the end users want
to achieve, which is to switch to a "branch" (in air quotes, as the
word in this sentence means a bit broader than "a ref that is in
refs/heads/"), to choose which lineage of commits to work on to grow
or reshape the history.  Most of the time, you would be working on a
branch (that is a ref that is in refs/heads/), sometimes you would
be working on an unnamed "branch" (i.e. detached HEAD state, only
difference between being on it and a normal branch is whether it is
named---the history manipulation can happen exactly the same way).

In other words, your initial motivation of stepping back and
thinking about what the command is about is very good.  But in the
context of checking out a branch, the concept the end user works
with are at the level of "branch", not HEAD, ref, index, working
tree (all of which are underlying implementation details that let
you work on manipulating the history, represented by the "branch").

> "content-to-path", maybe(?) ...

Path is not a place.  A path t/Makefile is a shared name of a place
in a tree, the index, or in the working tree.  Renaming "git add" to
"content-to-path" because it adds the content to the index at path
may be equally OK, but it misses the essense (which is that it is to
add to the index and not to anythng else like a tree or the working
tree).

I actually think the easiest-to-understand shorthand for the
operation "checking out the contents at the path to the working
tree" is "checkout".  

These...

>   git tree-to-worktree # git checkout  -- 
>   git index-to-worktree # git checkout -- 

...are interesting and worth learning lessons from.  These would be
something people would suggest when users start making noises about
"restore-to-path" or "content-to-path" overloads three different
operations and is confusing.

I think "restore-to-path" that can take contents for individual
paths out of either a treeish or the index and update the index and
the working tree, depending on what the user tells it to do, is not
confusing to the end users.  At the conceptual level, the users need
to have a mental model that has three places (tree-ish, index and
working tree) that can hold contents to make use of these
operations, so it is only the matter of how to express it clearly
and concisely.

For that matter, "checkout ", "checkout  -- "
and "checkout -- " already is a trio of clear and cocncise way
to spell three distinct operations, so with the right mental model,
it may not be so confusing to the users.  And "checkout-branch",
"checkout-contents-from-tree", and "checkout-contents-from-index"
longhands may be a good way to help new users form the right mental
model, as they are more explicit in their names; once they form the
right mental model (that is, there are three places the contents
live, and there are a few operations to move contents from the two
places to the working tree, i.e. what traditionally is called
"checking things out"), they may gradulate to the shorthand form.

Re: [PATCH v1/RFC 1/1] 'git clone C:\cygwin\home\USER\repo' is working (again)

[Steven Penny]

On Mon, Nov 26, 2018 at 11:32 AM wrote:
> This is the first vesion of a patch.
> Is there a chance that you test it ?

I can confirm that this fixes the issue.

Thank you!

Re: Confusing behavior with ignored submodules and `git commit -a`

[Stefan Beller]

On Thu, Nov 15, 2018 at 4:31 PM Michael Forney  wrote:
>
> On 2018-11-15, Stefan Beller  wrote:
> > On Thu, Nov 15, 2018 at 1:33 PM Michael Forney  wrote:
> >> Well, currently the submodule config can be disabled in diff_flags by
> >> setting override_submodule_config=1. However, I'm thinking it may be
> >> simpler to selectively *enable* the submodule config in diff_flags
> >> where it is needed instead of disabling it everywhere else (i.e.
> >> use_submodule_config instead of override_submodule_config).
> >
> > This sounds like undoing the good(?) part of the series that introduced
> > this regression, as before that we selectively loaded the submodule
> > config, which lead to confusion when you forgot it. Selectively *enabling*
> > the submodule config sounds like that state before?
> >
> > Or do we *only* talk about enabling the ignore flag, while loading the
> > rest of the submodule config automatic?
>
> Yes, that is what I meant. I believe the automatic loading of
> submodule config is the right thing to do, it just uncovered cases
> where the current handling of override_submodule_config is not quite
> sufficient.

That sounds good.

>
> My suggestion of replacing override_submodule_config with
> use_submodule_config is because it seems like there are fewer places
> where we want to apply the ignore config than not. I think it should
> only apply in diffs against the working tree and when staging changes
> to the index (unless specified explicitly). The documentation just
> mentions the "diff family", but all but one of the possible values for
> submodule..ignore ("all") don't make sense unless comparing with
> the working tree. This is also how show/log -p behaved in git <2.15.
> So I think that clarifying that it is about modifications *to the
> working tree* would be a good idea.
>
> >> I'm also starting to see why this is tricky. The only difference that
> >> diff.c:run_diff_files sees between `git add inner` and `git add --all`
> >> is whether the index entry matched the pathspec exactly or not.
> >
> > Unrelated to the trickiness, I think we'd need to document the behavior
> > of the -a flag in git-add and git-commit better as adding the diff below
> > will depart from the "all" rule again, which I thought was a strong
> > motivator for Brandons series (IIRC).
>
> Can you explain what you mean by the "all" rule?

-a as short for --all in git commit, and I presumed it to be
'--all-content', but documentation tells me it's about files
only, so there is no really an "all" rule, but rather me misunderstanding
to what it applies.

Re: [ANNOUNCE] Git v2.20.0-rc1

[Johannes Schindelin]

Hi Ævar,

On Mon, 26 Nov 2018, Johannes Schindelin wrote:

> On Sat, 24 Nov 2018, Ævar Arnfjörð Bjarmason wrote:
> 
> > On Wed, Nov 21 2018, Junio C Hamano wrote:
> > 
> > >  * "git rebase" and "git rebase -i" have been reimplemented in C.
> > 
> > Here's another regression in the C version (and rc1), note: the
> > sha1collisiondetection is just a stand in for "some repo":
> > 
> > (
> > rm -rf /tmp/repo &&
> > git init /tmp/repo &&
> > cd /tmp/repo &&
> > for c in 1 2
> > do
> > touch $c &&
> > git add $c &&
> > git commit -m"add $c"
> > done &&
> > git remote add origin 
> > https://github.com/cr-marcstevens/sha1collisiondetection.git &&
> > git fetch &&
> > git branch --set-upstream-to origin/master &&
> > git rebase -i
> > )
> > 
> > The C version will die with "fatal: unable to read tree
> > ". Running this with
> > rebase.useBuiltin=false does the right thing and rebases as of the merge
> > base of the two (which here is the root of the history).
> 
> Sorry, this bug does not reproduce here:
> 
> $ git rebase -i
> Successfully rebased and updated refs/heads/master.
> 
> > I wasn't trying to stress test rebase. I was just wanting to rebase a
> > history I was about to force-push after cleaning it up, hardly an
> > obscure use-case. So [repeat last transmission in
> > https://public-inbox.org/git/87y39w1wc2@evledraar.gmail.com/ ]
> 
> Maybe you can give me the full details so that I can verify that this is
> indeed a bug in the builtin C and not just a regression caused by some
> random branches being merged together?
> 
> In short: please provide me with the exact URL and branch of your git.git
> fork to test. Then please make sure to specify the precise revision of the
> sha1collisiondetection/master rev, just in case that it matters.
> 
> Ideally, you would reduce the problem to a proper test case, say, for
> t3412 (it seems that you try to rebase onto an unrelated history, so it is
> *vaguely* related to "rebase-root").

So I was getting spooked enough by your half-complete bug report that I
did more digging (it is really quite a bit frustrating to have so little
hard evidence to go on, a wild goose chase is definitely not what I was
looking forward to after a day of fighting other fires, but you know,
built-in rebase is dear to me).

The error message you copied clearly comes from tree-walk.c, from
`fill_tree_descriptor()` (the other "unable to read tree" messages enclose
the hash in parentheses).

There are exactly 3 calls to said function in the built-in rebase/rebase
-i in the current `master`, a1598010f775 (Merge branch
'nd/per-worktree-ref-iteration', 2018-11-26):

$ git grep fill_tree_descriptor -- builtin/rebase*.c sequencer.[ch] 
rebase-interactive.[ch]
builtin/rebase.c:   if (!reset_hard && !fill_tree_descriptor([nr++], 
_oid)) {
builtin/rebase.c:   if (!fill_tree_descriptor([nr++], oid)) {
sequencer.c:if (!fill_tree_descriptor(, )) {

The last one of these is in `do_reset()`, i.e. handling a `reset` command
which you did not ask for, as you passed `-i` to `git rebase`, not `-ir`.

The first two *both* are in `reset_head()`. The first of them uses
`head_oid`, which is read directly via `get_oid("HEAD", _oid)`, so if
this is all zeroes for you, then it's not rebase's fault.

The second one uses the parameter `oid` passed into `reset_head()`. The
only calls to that function that do not pass `NULL` as `oid` (which would
trigger `oid` to be replaced by `_oid`, i.e again not all zeroes
unless your setup is broken) are:

- in the `--abort` code path
- in the `--autostash` code path
- in the fast-forwarding code path
- just after the "First, rewinding head" message in the *non*-interactive
  rebase

None of these apply to your script snippet.

Under the assumption that you might have forgotten to talk about
rebase.autostash=true and some dirty file, I tried to augment the script
snippet accordingly, but the built-in rebase as of current `master` still
works for me, plus: reading the autostash code path, it is hard to imagine
that the `lookup_commit_reference()` would return a pointer to a commit
object whose oid is all zeroes.

In short, even a thorough study of the code (keeping in mind the few
tidbits of information provided by you) leaves me really wondering which
code you run, because it sure does not look like current `master` to me.

And if it is not `master`, then I have to ask why you keep suggesting to
turn off the built-in rebase by default in `master`.

Ciao,
Johannes

P.S.: Maybe you have a hook you forgot to mention?

Re: What's cooking in git.git (Nov 2018, #06; Wed, 21)

[Junio C Hamano]

Stefan Beller  writes:

>> * sb/submodule-recursive-fetch-gets-the-tip (2018-10-31) 11 commits
>> [...]
>>
>>  "git fetch --recurse-submodules" may not fetch the necessary commit
>>  that is bound to the superproject, which is getting corrected.
>>
>>  Is the discussion on this topic over?  What was the outcome?
>
> Please don't merge down the topic in the current state.

Thanks.  It won't happen until the final anyway ;-)

Re: BUG: CR marker ^M doesn't show up in '-' lines of diffs when the ending of the removed line is CR+LF

[Junio C Hamano]

Johannes Sixt  writes:

> Am 26.11.18 um 04:04 schrieb Junio C Hamano:
>>
>> That does not sound right.  I would understand it if both lines
>> showed ^M at the end, and only the one on the postimage line had it
>> highlighted as a trailing-whitespace.
>
> I agree that this is a (small?) weakness. But...
>
>> When we are producing a colored output, we know we are *not* writing
>> for machines, so one way to do it would be to turn CR at the end of
>> the line into two letter "^" "M" sequence on our end, without relying
>> on the terminal and/or the pager.  I dunno.
>
> ... this goes too far, IMO. It is the pager's task to decode control
> characters.

It was tongue-in-cheek suggestion to split a CR into caret-em on our
end, but we'd get essentially the same visual effect if we added a
rule:

When producing a colored output (not limited to whitespace
error coloring of diff output), insert  before a CR
that comes immediately before a LF.

Then, what Frank saw in the troublesome output would become

 -something  CR  LF
 +something_new   CR  LF

and we'll let the existing pager+terminal magic turn that trailing
CR on the preimage line into caret-em, just like the trailing CR on
the postimage line is already shown as caret-em with the current
output.

And a good thing is that I do not think that new rule is doing any
decode of control chars on our end.  We are just producing colored
output normally.

Re: [RFC] Introduce two new commands, switch-branch and restore-paths

[Stefan Beller]

On Mon, Nov 26, 2018 at 8:01 AM Ævar Arnfjörð Bjarmason
 wrote:
>
>
> On Tue, Nov 20 2018, Duy Nguyen wrote:
>
> > On Mon, Nov 19, 2018 at 04:19:53PM +0100, Duy Nguyen wrote:
> >> I promise to come back with something better (at least it still
> >> sounds better in my mind). If that idea does not work out, we can
> >> come back and see if we can improve this.
> >
> > So this is it. The patch isn't pretty, mostly as a proof of
> > concept. Just look at the three functions at the bottom of checkout.c,
> > which is the main thing.
> >
> > This patch tries to split "git checkout" command in two new ones:
> >
> > - git switch-branch is all about switching branches
>
> Isn't this going to also take the other ref arguments 'git checkout'
> takes now? I.e. tags, detached HEADs etc? I'm reminded of the discussion
> about what "range-diff" should be called :)

Heh, good call. :-)
Note that the color of a bikeshed has fewer functional implications
than coming up with proper names in your API exposed to millions
of users, as cognitive associations playing mind tricks, can have a
huge impact on their productivity. ;-)

In a neighboring thread there is discussion of the concept of a
'change' (and evolving the change locally), which is yet another
thing in the refs-space.

'switch-branch' sounds like a good name for a beginner who is just
getting started, but as soon as they discover that there is more than
branches (detached HEAD via commits, tags,
remote tracking "branches"), this name may be confusing.
So it would not be a good choice for the intermediate Git user.
The old time power user would not care as they have 'checkout'
in their muscle memory, aliased to 'co', but maybe they'd find
it nice for explaining to new users.

So I'd be thrilled to find a name that serves users on all levels.

Maybe we need to step back and consider what the command does.
And from that I would name it "rewire-HEAD-and-update-index"
and then simplify from there. For the beginner user, the concept of
HEAD might be overwhelming, such that we don't want to have that
in there.

So I'd be tempted to suggest to call it "switch-to-ref", but that would
be wrong in the corner case as well: When using that with a remote
tracking branch, you don't "switch to it" by putting it into your HEAD,
but you merely checkout the commit that it's pointing at.



>
> > - git restore-paths (maybe restore-file is better) for checking out
> >   paths

"content-to-path", maybe(?) as it moves the content (as given by commit
or implicitly assuming the index when omitted) into that path(, again).
(I am not enthused about this, as you can similarly argue for
content-to-paths, content-to-worktree, which then could split up into
"index-to-worktree [pathspec]" as well as "tree-to-worktree ".
also the notion of X-to-Y seems a novel concept in our naming, so maybe
verb-noun is better, hence restore-path or "fix-paths" may be better)

> > Later on, we could start to add a bit more stuff in, e.g. some form of
> > disambiguation is no longer needed when running as switch-branch, or
> > restore-paths.
> >
> > So, what do you think?

The patch looks interestingly small :-)

> That "git checkout" does too many things is something that keeps coming
> up in online discussions about Git's UI. Two things:
>
> a) It would really help to have some comparison of cases where these
>split commands are much clearer or less ambiguous than
>git-checkout. I can think of some (e.g. branch with the same name as
>a file) but having some overall picture of what the new UI looks like
>with solved / not solved cases would be nice. Also a comparison with
>other SCMs people find less confusing (svn, hg, bzr, ...)

How do other SCMs solve this issue? (What is their design space?
How many commands do they have for what git-checkout does
all-in-one?)

> b) I think we really need to have some end-game where we'd actually
>switch away from "checkout" (which we could still auto-route to new
>commands in perpetuity, but print a warning or error). Otherwise
>we'll just end up with https://xkcd.com/927/ and more UI confusion
>for all.

Heh, that situation is only avoided when the new command has clear
advantages over the old, and ISTM that we can only compete on
UX and better defaults, so maybe I'd push for making it more logical,
maybe so:

  git tree-to-worktree # git checkout  -- 
  git index-to-worktree # git checkout -- 
  git rev-to-ref # git checkout 

Just food for thought, specifically the last one would be
hilarious if we'd end up with it.

Stefan

Re: [ANNOUNCE] Git v2.20.0-rc1

[Johannes Schindelin]

Hi Ævar,

On Sat, 24 Nov 2018, Ævar Arnfjörð Bjarmason wrote:

> On Wed, Nov 21 2018, Junio C Hamano wrote:
> 
> >  * "git rebase" and "git rebase -i" have been reimplemented in C.
> 
> Here's another regression in the C version (and rc1), note: the
> sha1collisiondetection is just a stand in for "some repo":
> 
> (
> rm -rf /tmp/repo &&
> git init /tmp/repo &&
> cd /tmp/repo &&
> for c in 1 2
> do
> touch $c &&
> git add $c &&
> git commit -m"add $c"
> done &&
> git remote add origin 
> https://github.com/cr-marcstevens/sha1collisiondetection.git &&
> git fetch &&
> git branch --set-upstream-to origin/master &&
> git rebase -i
> )
> 
> The C version will die with "fatal: unable to read tree
> ". Running this with
> rebase.useBuiltin=false does the right thing and rebases as of the merge
> base of the two (which here is the root of the history).

Sorry, this bug does not reproduce here:

$ git rebase -i
Successfully rebased and updated refs/heads/master.

> I wasn't trying to stress test rebase. I was just wanting to rebase a
> history I was about to force-push after cleaning it up, hardly an
> obscure use-case. So [repeat last transmission in
> https://public-inbox.org/git/87y39w1wc2@evledraar.gmail.com/ ]

Maybe you can give me the full details so that I can verify that this is
indeed a bug in the builtin C and not just a regression caused by some
random branches being merged together?

In short: please provide me with the exact URL and branch of your git.git
fork to test. Then please make sure to specify the precise revision of the
sha1collisiondetection/master rev, just in case that it matters.

Ideally, you would reduce the problem to a proper test case, say, for
t3412 (it seems that you try to rebase onto an unrelated history, so it is
*vaguely* related to "rebase-root").

Ciao,
Dscho

Re: [PATCH 2/5] ieot: default to not writing IEOT section

[Stefan Beller]

On Mon, Nov 26, 2018 at 1:48 PM Ben Peart  wrote:
>
>
>
> On 11/26/2018 2:59 PM, Stefan Beller wrote:
> >>> +static int record_ieot(void)
> >>> +{
> >>> + int val;
> >>> +
> >>
> >> Initialize stack val to zero to ensure proper default.
> >
> > I don't think that is needed here, as we only use `val` when
> > we first write to it via git_config_get_bool.
> >
> > Did you spot this via code review and thought of
> > defensive programming or is there a tool that
> > has a false positive here?
> >
>
> Code review and defensive programming.  I had to review the code in
> git_config_get_bool() to see if it always initialized the val even if it
> didn't find the requested config variable (esp since we don't pass in a
> default value for this function like we do others).
>

Ah, sorry to have sent out this email, which I found as one of the
earliest discussions in my mailbox. The later patches/discussions
became a lot more heated from my cursory skimming and sorted
out this as well.

It is interesting to notice that, as I also had to lookup how the config
machinery works (once? a couple times?) but now it is so hardcoded
in my brain to assume that if functions like git_config_* take the
branch, we can access the value that the config function was supposed
to read into.

Sorry for the noise,
Stefan

Re: What's cooking in git.git (Nov 2018, #06; Wed, 21)

[Stefan Beller]

>
> * sb/submodule-recursive-fetch-gets-the-tip (2018-10-31) 11 commits
> [...]
>
>  "git fetch --recurse-submodules" may not fetch the necessary commit
>  that is bound to the superproject, which is getting corrected.
>
>  Is the discussion on this topic over?  What was the outcome?

Please don't merge down the topic in the current state.

I have a local updated version sitting on my disk and plan to send
it once I made sure it addressed all comments of various revisions.

Re: [PATCH 2/5] ieot: default to not writing IEOT section

[Ben Peart]

On 11/26/2018 2:59 PM, Stefan Beller wrote:

+static int record_ieot(void)
+{
+ int val;
+


Initialize stack val to zero to ensure proper default.


I don't think that is needed here, as we only use `val` when
we first write to it via git_config_get_bool.

Did you spot this via code review and thought of
defensive programming or is there a tool that
has a false positive here?



Code review and defensive programming.  I had to review the code in 
git_config_get_bool() to see if it always initialized the val even if it 
didn't find the requested config variable (esp since we don't pass in a 
default value for this function like we do others).





+ if (!git_config_get_bool("index.recordoffsettable", ))
+ return val;
+ return 0;
+}

Re: [PATCH v2 0/9] diff --color-moved-ws fixes and enhancment

[Stefan Beller]

On Fri, Nov 23, 2018 at 3:17 AM Phillip Wood  wrote:
>
> From: Phillip Wood 
>
> Thanks to Stefan for his feedback on v1. I've updated patches 2 & 8 in
> response to those comments - see the range-diff below for details (the
> patch numbers are off by one in the range diff, I think because the
> first patch is unchanged and so it was used as the merge base by
> --range-diff=.

`git range-diff` accepts a three dotted "range" OLD...NEW
as an easy abbreviation for the arguments
"COMMON..OLD COMMON..NEW" and the common element is
computed as the last common element. It doesn't have knowledge
about where you started your topic branch.


> For some reason the range-diff also includes
> the notes even though I did not give --notes to format-patch)

This is interesting.
The existence of notes.rewrite. seems to work well
with the range-diff then, as the config would trigger the copy-over
of notes and then range-diff would diff the original notes to the new
notes.

>
> When trying out the new --color-moved-ws=allow-indentation-change I
> was disappointed to discover it did not work if the indentation
> contains a mix of spaces and tabs. This series reworks it so that it
> does.
>

The range-diff looks good to me.

Thanks,
Stefan

Re: [PATCH v1 1/2] log -G: Ignore binary files

[Stefan Beller]

On Wed, Nov 21, 2018 at 1:08 PM Thomas Braun
 wrote:
>
> The -G  option of log looks for the differences whose patch text
> contains added/removed lines that match regex.
>
> The concept of differences only makes sense for text files, therefore
> we need to ignore binary files when searching with -G  as well.

What about partial text/partial binary files?

I recall using text searching tools (not necessarily git machinery,
my memory is fuzzy) to check for strings in pdf files, which are
usually marked binary in context of git, such that we do not
see their diffs in `log -p`.

But I would expect a search with -G or -S to still work...
until I find the exception in the docs, only to wonder if
there is a switch to turn off this optimisation for this
corner case.

Stefan

Re: [PATCH 2/5] ieot: default to not writing IEOT section

[Stefan Beller]

> > +static int record_ieot(void)
> > +{
> > + int val;
> > +
>
> Initialize stack val to zero to ensure proper default.

I don't think that is needed here, as we only use `val` when
we first write to it via git_config_get_bool.

Did you spot this via code review and thought of
defensive programming or is there a tool that
has a false positive here?

>
> > + if (!git_config_get_bool("index.recordoffsettable", ))
> > + return val;
> > + return 0;
> > +}

Re: [PATCH] grep: use grep_opt->repo instead of explict repo argument

[Stefan Beller]

On Sun, Nov 18, 2018 at 8:38 AM Nguyễn Thái Ngọc Duy  wrote:
>
> This command is probably the first one that operates on a repository
> other than the_repository, in f9ee2fcdfa (grep: recurse in-process
> using 'struct repository' - 2017-08-02). An explicit 'struct
> repository *' was added in that commit to pass around the repository
> that we're supposed to grep from.
>
> Since 38bbc2ea39 (grep.c: remove implicit dependency on the_index -
> 2018-09-21). 'struct grep_opt *' carries in itself a repository
> parameter for grepping. We should now be able to reuse grep_opt to
> hold the submodule repo instead of a separate argument, which is just
> room for mistakes.

That makes sense. Assuming we did not make a mistake yet, the
test suite would not need changes (further assuming we do test for it,
but we do as we have explicit submodule grep tests).

>
> While at there, use the right reference instead of the_repository and
> the_index in this code. I was a bit careless in my attempt to kick
> the_repository / the_index out of library code. It's normally safe to
> just stick the_repository / the_index in bultin/ code, but it's not
> the case for grep.

Reviewed-by: Stefan Beller 

> Signed-off-by: Nguyễn Thái Ngọc Duy 

Re: BUG: CR marker ^M doesn't show up in '-' lines of diffs when the ending of the removed line is CR+LF

[Johannes Sixt]

Am 26.11.18 um 04:04 schrieb Junio C Hamano:

It appears to me that what Frank sees is not "^M highlighted for
whitespace breakage appears only on postimage lines, while ^M is
shown but not highlighted on preimage lines".  My reading of the
above is "Not just without highlight, ^M is just *NOT* shown on the
preimage line".

That does not sound right.  I would understand it if both lines
showed ^M at the end, and only the one on the postimage line had it
highlighted as a trailing-whitespace.


I agree that this is a (small?) weakness. But...


When we are producing a colored output, we know we are *not* writing
for machines, so one way to do it would be to turn CR at the end of
the line into two letter "^" "M" sequence on our end, without relying
on the terminal and/or the pager.  I dunno.


... this goes too far, IMO. It is the pager's task to decode control 
characters.


-- Hannes

Re: Did You Receive My Last Mail?

[Reem Al-Hashimi]

Hello,

My name is ms. Reem Al-Hashimi. The UAE minister of state for international 
cooparation. I got your contact from an email database from your country. I 
have a financial transaction i would like to discuss with you. Please reply to 
reem2...@daum.net, for more details if you are interested.

Regards,

Ms. Reem Al-Hashimi

Re: [PATCH v16 Part II 2/8] bisect--helper: `bisect_write` shell function in C

[Johannes Schindelin]

Hi Martin,

On Fri, 23 Nov 2018, Martin Ågren wrote:

> On Fri, 23 Nov 2018 at 11:13, Johannes Schindelin
>  wrote:
> > On Mon, 30 Oct 2017, Pranit Bauva wrote:
> >
> > > On Fri, Oct 27, 2017 at 10:58 PM, Martin Ågren  
> > > wrote:
> > > > On 27 October 2017 at 17:06, Pranit Bauva  
> > > > wrote:
> > > >> +static void free_terms(struct bisect_terms *terms)
> > > >> +{
> > > >> +   if (!terms->term_good)
> > > >> +   free((void *) terms->term_good);
> > > >> +   if (!terms->term_bad)
> > > >> +   free((void *) terms->term_bad);
> > > >> +}
> 
> > > > You leave the pointers dangling, but you're ok for now since this is the
> > > > last thing that happens in `cmd_bisect__helper()`. Your later patches
> > > > add more users, but they're also ok, since they immediately assign new
> > > > values.
> > > >
> > > > In case you (and others) find it useful, the below is a patch I've been
> > > > sitting on for a while as part of a series to plug various memory-leaks.
> > > > `FREE_AND_NULL_CONST()` would be useful in precisely these situations.
> > >
> > > Honestly, I wouldn't be the best person to judge this.
> >
> > Git's source code implicitly assumes that any `const` pointer refers to
> > memory owned by another code path. It is therefore probably not a good
> > idea to encourage `free()`ing a `const` pointer.
> 
> Yeah, I never submitted that patch as part of a real series. I remember
> having a funky feeling about it, and whatever use-case I had for this
> macro, I managed to solve the memory leak in some other way in a
> rewrite.
> 
> Thanks for a sanity check.

I am glad you agree, and it's just fair that I contribute a sanity check
on this here list when I have benefitted so many times from the same.

Ciao,
Johannes

Re: What's cooking in git.git (Nov 2018, #06; Wed, 21)

[Jeff King]

On Sun, Nov 25, 2018 at 11:02:05AM +0900, Junio C Hamano wrote:

> Jeff King  writes:
> 
> > I do also think in the long run we should be fixing the "unreachable
> > always become loose" issues.
> 
> I think I've seen an idea of collecting them into a garbage pack
> floated for at least a few times here.  What are the downsides?  We
> no longer will know when these unreachable ones were last accessed
> individually so we need to come up with a different policy around
> their expiration?  As the common traits among objects in such a
> garbage pack (iow the way we discover the objects that need to be
> placed in there) does not involve path information and we lose the
> ability to compress them well?

Yes, the main issue is handling the expiration/mtime.

We may lose some input to the delta heuristics, but:

  - the current alternative is non-delta loose objects (so just shoving
those in a pack is no worse for disk space, and probably better
because of less inode/file overhead)

  - if they were already packed we can often just retain the existing
deltas (and we get this basically for free with the existing code)

  - we could still walk unreachable bits of the graph, starting at
dangling commits, to find the path information (we do this to some
degree already to avoid dropping objects depended on by "unreachable
but recent" objects, but I don't know how systematic we are about
making sure to hit walk down from root trees first)

The most thorough discussion I know of in this direction is the one
around:

  
https://public-inbox.org/git/20170610080626.sjujpmgkli4mu...@sigill.intra.peff.net/

-Peff

Re: [PATCH v2 0/4] Port git-add--interactive.perl:status_cmd to C

[Slavica Djukic]

Hi Daniel,

On 16-May-17 6:00 AM, Daniel Ferreira wrote:

This is the second version of a patch series to start porting
git-add--interactive from Perl to C.

Series:
v1: 
https://public-inbox.org/git/1494009820-2090-1-git-send-email-bnm...@gmail.com/

Travis CI build: https://travis-ci.org/theiostream/git/builds/232669894

I have applied most of the changes suggested by Johannes and Ævar (thank
you!). The one exception was Johannes' request to make this a WIP patch
series with a sort-of "design" of what's next to come. I did not do
this because I still have no clue...! I'll begin experimenting on
update_cmd() to see if I gain some insight on this issue that I could
bring to this series. I do think this would be a good idea, though! :)



I am Slavica Đukic, Outreachy intern for Git (Dec-March 2018/19 round).

Project I'll be working on is "Turn git add -i to built-in".

Would you mind if I use your work so far as head start?




-- Daniel.

Daniel Ferreira (4):
   diff: export diffstat interface
   add--helper: create builtin helper for interactive add
   add--helper: implement interactive status command
   add--interactive: use add-interactive--helper for status_cmd

  .gitignore|   1 +
  Makefile  |   1 +
  builtin.h |   1 +
  builtin/add--helper.c | 285 ++
  diff.c|  17 +--
  diff.h|  19 +++-
  git-add--interactive.perl |   4 +-
  git.c |   1 +
  8 files changed, 309 insertions(+), 20 deletions(-)
  create mode 100644 builtin/add--helper.c

--
2.7.4 (Apple Git-66)




Thank you,
Slavica

Re: t5570 shaky for anyone ?

[Jeff King]

On Sun, Nov 25, 2018 at 11:22:47PM +0100, SZEDER Gábor wrote:

> > The following fixes it, but I am not sure if this is the ideal
> > solution.
> 
> Currently this is the only test that looks at 'daemon.log', but if we
> ever going to add another one, then that will be prone to the same
> issue.
> 
> I wonder whether it's really that useful to have the daemon log in the
> test script's output...  if the log was sent directly to daemon log,
> then the window for this race would be smaller, but still not
> completely closed.
> 
> Anyway, I added Peff to Cc, since he added that whole fifo-shell-loop
> thing.

Yeah, see my comments on the other part of the thread. If we did write
directly to a file, I think that would be enough here because git-daemon
writes this entry before running the sub-process. So by the time
ls-remote finishes, we know that it talked to upload-pack, and we know
that before upload-pack was run, git-daemon wrote the log entry. That
assumes git-daemon doesn't buffer its logs (but if it does, we should
probably fix that).

-Peff

Re: t5570 shaky for anyone ?

[Jeff King]

On Sun, Nov 25, 2018 at 10:01:38PM +, Thomas Gummerer wrote:

> On 11/25, Torsten Bögershausen wrote:
> > After running the  "Git 2.20-rc1" testsuite here on a raspi,
> > the only TC that failed was t5570.
> > When the "grep" was run on daemon.log, the file was empty (?).
> > When inspecting it later, it was filled, and grep would have found
> > the "extended.attribute" it was looking for.
> 
> I believe this has been reported before in
> https://public-inbox.org/git/1522783990.964448.1325338528.0d49c...@webmail.messagingengine.com/,
> but it seems like the thread never ended with actually fixing it.
> Reading the first reply Peff seemed to be fine with just removing the
> test completely, which would be the easiest solution ;)  Adding him to
> Cc: here.

Yes, I don't think there is a way to make this race-proof without
somehow convincing "cat" to flush (and let us know when it has). Which
really implies killing the daemon, and wait()ing on cat to process the
EOF and exit.  And that makes the tests a lot more expensive if we have
to start the daemon for each snippet.

So I'm still fine with just dropping this test.

> > diff --git a/t/t5570-git-daemon.sh b/t/t5570-git-daemon.sh
> > index 7466aad111..e259fee0ed 100755
> > --- a/t/t5570-git-daemon.sh
> > +++ b/t/t5570-git-daemon.sh
> > @@ -192,6 +192,7 @@ test_expect_success 'daemon log records all attributes' 
> > '
> > GIT_OVERRIDE_VIRTUAL_HOST=localhost \
> > git -c protocol.version=1 \
> > ls-remote "$GIT_DAEMON_URL/interp.git" &&
> > +   sleep 1 &&
> > grep -i extended.attribute daemon.log | cut -d" " -f2- >actual &&
> > test_cmp expect actual
> >  '
> > 
> > A slightly better approach may be to use a "sleep on demand":
> > 
> > +   ( grep -i -q extended.attribute daemon.log || sleep 1 ) &&

That doesn't really fix it, but just broadens the race window. I dunno.
Maybe that is enough in practice. We could do something like:

  repeat_with_timeout () {
local i=0
while test $i -lt 10
do
"$@" && return 0
sleep 1
done
# no success even after 10 seconds
return 1
  }

  repeat_with_timeout grep -i extended.attribute daemon.log

to make the pattern a bit more obvious (and make it easy to extend the
window arbitrarily; surely 10s is enough?).

-Peff

Re: [RFC] Introduce two new commands, switch-branch and restore-paths

[Duy Nguyen]

On Mon, Nov 26, 2018 at 5:01 PM Ævar Arnfjörð Bjarmason
 wrote:
> > So, what do you think?
>
> That "git checkout" does too many things is something that keeps coming
> up in online discussions about Git's UI. Two things:
>
> a) It would really help to have some comparison of cases where these
>split commands are much clearer or less ambiguous than
>git-checkout. I can think of some (e.g. branch with the same name as
>a file) but having some overall picture of what the new UI looks like
>with solved / not solved cases would be nice. Also a comparison with
>other SCMs people find less confusing (svn, hg, bzr, ...)

Less ambiguous is indeed one of the reasons I wanted to do this.

> b) I think we really need to have some end-game where we'd actually
>switch away from "checkout" (which we could still auto-route to new
>commands in perpetuity, but print a warning or error). Otherwise
>we'll just end up with https://xkcd.com/927/ and more UI confusion
>for all.

I'm not going to remove "git checkout". Not until the majority of
users are really in favor of the new ones. So my end-game plan is to
just promote the two new commands in man pages, tutorial, advice...
Perhaps at some point "git checkout" is also removed from "git help".
But that's about it. If you want to stick with "git checkout", it's
there (and not nagging you to move to new ones).
-- 
Duy

Re: [RFC PATCH] Introduce "precious" file concept

[Eckhard Maaß]

On Mon, Nov 12, 2018 at 11:22:09PM +, brian m. carlson wrote:
> This is going to totally hose automation.  My last job had files which
> might move from tracked to untracked (a file that had become generated),
> and long-running CI and build systems would need to be able to check out
> one status and switch to the other.  Your proposed change will prevent
> those systems from working, whereas they previously did.

Wouldn't those systems not use -f right now? And shouldn't Git have the
same semantic for -f to clobber everything in the proposed use case?
Like it does right now for untracked files which are not ignored. So to
be save going back and forth I would expect those systems to use -f
anyway. Have I missed something here?

Regards,
Eckhard

Re: [RFC] Introduce two new commands, switch-branch and restore-paths

[Ævar Arnfjörð Bjarmason]

On Tue, Nov 20 2018, Duy Nguyen wrote:

> On Mon, Nov 19, 2018 at 04:19:53PM +0100, Duy Nguyen wrote:
>> I promise to come back with something better (at least it still
>> sounds better in my mind). If that idea does not work out, we can
>> come back and see if we can improve this.
>
> So this is it. The patch isn't pretty, mostly as a proof of
> concept. Just look at the three functions at the bottom of checkout.c,
> which is the main thing.
>
> This patch tries to split "git checkout" command in two new ones:
>
> - git switch-branch is all about switching branches

Isn't this going to also take the other ref arguments 'git checkout'
takes now? I.e. tags, detached HEADs etc? I'm reminded of the discussion
about what "range-diff" should be called :)

> - git restore-paths (maybe restore-file is better) for checking out
>   paths

If it takes globs/dirs then a plural is probably better.

> The main idea is these two commands will co-exist with the good old
> 'git checkout', which will NOT be deprecated. Old timers will still
> use "git checkout". But new people should be introduced to the new two
> instead. And the new ones are just as capable as "git checkout".
>
> Since the three commands will co-exist (with duplicate functionality),
> maintenance cost must be kept to minimum. The way I did this is simply
> split the command line options into three pieces: common,
> switch-branch and checkout-paths. "git checkout" has all three, the
> other two have common and another piece.
>
> With this, a new option added to git checkout will be automatically
> available in either switch-branch or checkout-paths. Bug fixes apply
> to all relevant commands.
>
> Later on, we could start to add a bit more stuff in, e.g. some form of
> disambiguation is no longer needed when running as switch-branch, or
> restore-paths.
>
> So, what do you think?

That "git checkout" does too many things is something that keeps coming
up in online discussions about Git's UI. Two things:

a) It would really help to have some comparison of cases where these
   split commands are much clearer or less ambiguous than
   git-checkout. I can think of some (e.g. branch with the same name as
   a file) but having some overall picture of what the new UI looks like
   with solved / not solved cases would be nice. Also a comparison with
   other SCMs people find less confusing (svn, hg, bzr, ...)

b) I think we really need to have some end-game where we'd actually
   switch away from "checkout" (which we could still auto-route to new
   commands in perpetuity, but print a warning or error). Otherwise
   we'll just end up with https://xkcd.com/927/ and more UI confusion
   for all.

Re: [RFC PATCH] Introduce "precious" file concept

[Duy Nguyen]

On Mon, Nov 26, 2018 at 4:47 PM Ævar Arnfjörð Bjarmason
 wrote:
> >> >> How about something like this:
> >> >>
> >> >> 1. Introduce a concept with "garbage" files, which git is "permitted to
> >> >> delete" without prompting.
> >> >>
> >> >> 2. Retain the current default, i.e. "ignored files are garbage" for now,
> >> >> making the new behavior _opt in_ to avoid breaking automated
> >> >> systems/existing scripts for anyone. Put the setting for this behind a
> >> >> new core.* config flag.
> >> >>
> >> >> 3. In the plan for version 3.0 (a new major version where some breakage
> >> >> can be tolerable, according to Semantic Versioning), change the default
> >> >> so that "only explicit garbage is garbage". Include very clear notices
> >> >> of this in the release notes. The config flag is retained, but its
> >> >> default changes from true->false or vice versa. People who dislike the
> >> >> new behavior can easily change back to the 2.x semantics.
> >> >
> >> > How does this garbage thing interact with "git clean -x"? My
> >> > interpretation of this flag/attribute is that at version 3.0 by
> >> > default all ignored files are _not_ garbage, so "git clean -x" should
> >> > not remove any of them. Which is weird because most of ignored files
> >> > are like *.o that should be removed.
> >> >
> >> > I also need to mark "precious" on untracked or even tracked files (*).
> >> > Not sure how this "garbage" attribute interacts with that.
> >> >
> >> > (*) I was hoping I could get the idea [1] implemented in somewhat good
> >> > shape before presenting here. But I'm a bit slow on that front. So
> >> > yeah this "precious" on untracked/tracked thingy may be even
> >> > irrelevant if the patch series will be rejected.
> >>
> >> I think a garbage (or trashable) flag, if implemented, wouldn't need any
> >> special case in git-clean, i.e. -x would remove all untracked files,
> >> whether ignored or garbage/trashable. That's what my patch to implement
> >> it does:
> >> https://public-inbox.org/git/87zhuf3gs0@evledraar.gmail.com/
> >>
> >> I think that makes sense. Users running "git clean" have "--dry-run" and
> >> unlike "checkout a branch" or "merge this commit" where we'll now shred
> >> data implicitly it's obvious that git-clean is going to shred your data.
> >
> > Then that't not what I want. If I'm going to mark to keep "config.mak"
> > around, I'm not going to carefully move it away before doing "git
> > clean -fdx" then move it back. No "git clean --dry-run" telling me to
> > make a backup of config.mak is no good.
>
> Understood. I mean this in the context of solving the problem users have
> with running otherwise non-data-destroying commands like "checkout" and
> "merge" and getting their data destroyed, which is overwhelmingly why
> this topic gets resurrected.
>
> Some of the solutions overlap with this thing you want, but I think it's
> worth keeping the distinction between the two in mind.

On the other hand all use cases should be considered. It's going to be
a mess to have "trashable" attribute that applies to some commands
while "precious" to some others (and even worse when they overlap,
imagine having to define both in .gitattributes)

> I.e. I can
> imagine us finding some acceptable solution to the data shredding
> problem that doesn't implement this mode for "git-clean", or the other
> way around.
-- 
Duy

Re: [RFC PATCH] Introduce "precious" file concept

[Ævar Arnfjörð Bjarmason]

On Mon, Nov 26 2018, Duy Nguyen wrote:

> On Mon, Nov 26, 2018 at 4:34 PM Ævar Arnfjörð Bjarmason
>  wrote:
>>
>>
>> On Mon, Nov 26 2018, Duy Nguyen wrote:
>>
>> > On Mon, Nov 26, 2018 at 10:30 AM Per Lundberg  
>> > wrote:
>> >>
>> >> On 11/13/18 1:22 AM, brian m. carlson wrote:
>> >> > This is going to totally hose automation.  My last job had files which
>> >> > might move from tracked to untracked (a file that had become generated),
>> >> > and long-running CI and build systems would need to be able to check out
>> >> > one status and switch to the other.  Your proposed change will prevent
>> >> > those systems from working, whereas they previously did.
>> >> >
>> >> > I agree that your proposal would have been a better design originally,
>> >> > but breaking the way automated systems currently work is probably going
>> >> > to be a dealbreaker.
>> >>
>> >> How about something like this:
>> >>
>> >> 1. Introduce a concept with "garbage" files, which git is "permitted to
>> >> delete" without prompting.
>> >>
>> >> 2. Retain the current default, i.e. "ignored files are garbage" for now,
>> >> making the new behavior _opt in_ to avoid breaking automated
>> >> systems/existing scripts for anyone. Put the setting for this behind a
>> >> new core.* config flag.
>> >>
>> >> 3. In the plan for version 3.0 (a new major version where some breakage
>> >> can be tolerable, according to Semantic Versioning), change the default
>> >> so that "only explicit garbage is garbage". Include very clear notices
>> >> of this in the release notes. The config flag is retained, but its
>> >> default changes from true->false or vice versa. People who dislike the
>> >> new behavior can easily change back to the 2.x semantics.
>> >
>> > How does this garbage thing interact with "git clean -x"? My
>> > interpretation of this flag/attribute is that at version 3.0 by
>> > default all ignored files are _not_ garbage, so "git clean -x" should
>> > not remove any of them. Which is weird because most of ignored files
>> > are like *.o that should be removed.
>> >
>> > I also need to mark "precious" on untracked or even tracked files (*).
>> > Not sure how this "garbage" attribute interacts with that.
>> >
>> > (*) I was hoping I could get the idea [1] implemented in somewhat good
>> > shape before presenting here. But I'm a bit slow on that front. So
>> > yeah this "precious" on untracked/tracked thingy may be even
>> > irrelevant if the patch series will be rejected.
>>
>> I think a garbage (or trashable) flag, if implemented, wouldn't need any
>> special case in git-clean, i.e. -x would remove all untracked files,
>> whether ignored or garbage/trashable. That's what my patch to implement
>> it does:
>> https://public-inbox.org/git/87zhuf3gs0@evledraar.gmail.com/
>>
>> I think that makes sense. Users running "git clean" have "--dry-run" and
>> unlike "checkout a branch" or "merge this commit" where we'll now shred
>> data implicitly it's obvious that git-clean is going to shred your data.
>
> Then that't not what I want. If I'm going to mark to keep "config.mak"
> around, I'm not going to carefully move it away before doing "git
> clean -fdx" then move it back. No "git clean --dry-run" telling me to
> make a backup of config.mak is no good.

Understood. I mean this in the context of solving the problem users have
with running otherwise non-data-destroying commands like "checkout" and
"merge" and getting their data destroyed, which is overwhelmingly why
this topic gets resurrected.

Some of the solutions overlap with this thing you want, but I think it's
worth keeping the distinction between the two in mind. I.e. I can
imagine us finding some acceptable solution to the data shredding
problem that doesn't implement this mode for "git-clean", or the other
way around.

Re: [RFC PATCH 0/7] test: NetBSD support

[Ævar Arnfjörð Bjarmason]

On Sun, Nov 25 2018, Carlo Marcelo Arenas Belón wrote:

> Likely still missing changes as it only completes a run with a minimal
> number of dependencies but open for feedback
>
> Requires pkgsrc packages for gmake, perl, bash and curl and completes a run
>
>   $ gmake SHELL_PATH=/usr/pkg/bin/bash NO_PYTHON=1 CURL_DIR=/usr/pkg test

This looks good to me, and fixes most of the issues I've encountered on
NetBSD recently. See
https://gitlab.com/git-vcs/git-gitlab-ci/blob/9337ed6f99e3780d1d8dc087dff688f5a68805a4/ci/gitlab/run-on-gcc-farm.sh#L228-239
and https://gitlab.com/git-vcs/git-ci/-/jobs/125476939

Re: [ANNOUNCE] Git v2.20.0-rc1

[Elijah Newren]

On Sun, Nov 25, 2018 at 11:37 PM Junio C Hamano  wrote:
>
> Unless I hear otherwise in the next 24 hours, I am planning to
> merge the following topics to 'master' before cutting -rc2.  Please
> stop me on any of these topics.
>
>  - jc/postpone-rebase-in-c
>
>This may be the most controversial.  It demotes the C
>reimplementation of "git rebase" to an experimental opt-in
>feature that can only be enabled by setting rebase.useBuiltIn
>configuration that defaults to false.
>
>cf. 

If we don't set rebase.useBuiltin to false, then there is also a minor
regression in the error message printed by the built-in rebase we may
want to try to address.  I have a patch for it at
<20181122044841.20993-2-new...@gmail.com>, but I don't know if that
patch is acceptable as-is this close to a release since that'd not
give translators much time to update.

Re: [RFC PATCH] Introduce "precious" file concept

[Duy Nguyen]

On Mon, Nov 26, 2018 at 4:34 PM Ævar Arnfjörð Bjarmason
 wrote:
>
>
> On Mon, Nov 26 2018, Duy Nguyen wrote:
>
> > On Mon, Nov 26, 2018 at 10:30 AM Per Lundberg  wrote:
> >>
> >> On 11/13/18 1:22 AM, brian m. carlson wrote:
> >> > This is going to totally hose automation.  My last job had files which
> >> > might move from tracked to untracked (a file that had become generated),
> >> > and long-running CI and build systems would need to be able to check out
> >> > one status and switch to the other.  Your proposed change will prevent
> >> > those systems from working, whereas they previously did.
> >> >
> >> > I agree that your proposal would have been a better design originally,
> >> > but breaking the way automated systems currently work is probably going
> >> > to be a dealbreaker.
> >>
> >> How about something like this:
> >>
> >> 1. Introduce a concept with "garbage" files, which git is "permitted to
> >> delete" without prompting.
> >>
> >> 2. Retain the current default, i.e. "ignored files are garbage" for now,
> >> making the new behavior _opt in_ to avoid breaking automated
> >> systems/existing scripts for anyone. Put the setting for this behind a
> >> new core.* config flag.
> >>
> >> 3. In the plan for version 3.0 (a new major version where some breakage
> >> can be tolerable, according to Semantic Versioning), change the default
> >> so that "only explicit garbage is garbage". Include very clear notices
> >> of this in the release notes. The config flag is retained, but its
> >> default changes from true->false or vice versa. People who dislike the
> >> new behavior can easily change back to the 2.x semantics.
> >
> > How does this garbage thing interact with "git clean -x"? My
> > interpretation of this flag/attribute is that at version 3.0 by
> > default all ignored files are _not_ garbage, so "git clean -x" should
> > not remove any of them. Which is weird because most of ignored files
> > are like *.o that should be removed.
> >
> > I also need to mark "precious" on untracked or even tracked files (*).
> > Not sure how this "garbage" attribute interacts with that.
> >
> > (*) I was hoping I could get the idea [1] implemented in somewhat good
> > shape before presenting here. But I'm a bit slow on that front. So
> > yeah this "precious" on untracked/tracked thingy may be even
> > irrelevant if the patch series will be rejected.
>
> I think a garbage (or trashable) flag, if implemented, wouldn't need any
> special case in git-clean, i.e. -x would remove all untracked files,
> whether ignored or garbage/trashable. That's what my patch to implement
> it does:
> https://public-inbox.org/git/87zhuf3gs0@evledraar.gmail.com/
>
> I think that makes sense. Users running "git clean" have "--dry-run" and
> unlike "checkout a branch" or "merge this commit" where we'll now shred
> data implicitly it's obvious that git-clean is going to shred your data.

Then that't not what I want. If I'm going to mark to keep "config.mak"
around, I'm not going to carefully move it away before doing "git
clean -fdx" then move it back. No "git clean --dry-run" telling me to
make a backup of config.mak is no good.
-- 
Duy

Re: [RFC] Introduce two new commands, switch-branch and restore-paths

[Duy Nguyen]

On Mon, Nov 26, 2018 at 4:03 AM Junio C Hamano  wrote:
>
> Thomas Gummerer  writes:
>
> > I like the idea of splitting those commands up, in fact it is
> > something I've been considering working on myself.  I do think we
> > should consider if we want to change the behaviour of those new
> > commands in any way compared to 'git checkout', since we're starting
> > with a clean slate.

Better defaults? Hell yes!

> > One thing in particular that I have in mind is something I'm currently
> > working on, namely adding a --index flag to 'git checkout', which
> > would make 'git checkout' work in non-overlay mode (for more
> > discussion on that see also [*1*].
>
> Ah, thanks for reminding me of that.  That explains why I felt
> uneasy to see "restore" in the proposed command name.

About that name. I didn't want to start the command name with checkout
to avoid completion conflict (the obvious choice was checkout-path,
the function name behind it). And I didn't find any other good name,
so I picked "restore" out of git-checkout.txt's one-line description.
If we end up with a better command name, perhaps reword that line too.
-- 
Duy

Re: [RFC PATCH] Introduce "precious" file concept

[Ævar Arnfjörð Bjarmason]

On Mon, Nov 26 2018, Duy Nguyen wrote:

> On Mon, Nov 26, 2018 at 10:30 AM Per Lundberg  wrote:
>>
>> On 11/13/18 1:22 AM, brian m. carlson wrote:
>> > This is going to totally hose automation.  My last job had files which
>> > might move from tracked to untracked (a file that had become generated),
>> > and long-running CI and build systems would need to be able to check out
>> > one status and switch to the other.  Your proposed change will prevent
>> > those systems from working, whereas they previously did.
>> >
>> > I agree that your proposal would have been a better design originally,
>> > but breaking the way automated systems currently work is probably going
>> > to be a dealbreaker.
>>
>> How about something like this:
>>
>> 1. Introduce a concept with "garbage" files, which git is "permitted to
>> delete" without prompting.
>>
>> 2. Retain the current default, i.e. "ignored files are garbage" for now,
>> making the new behavior _opt in_ to avoid breaking automated
>> systems/existing scripts for anyone. Put the setting for this behind a
>> new core.* config flag.
>>
>> 3. In the plan for version 3.0 (a new major version where some breakage
>> can be tolerable, according to Semantic Versioning), change the default
>> so that "only explicit garbage is garbage". Include very clear notices
>> of this in the release notes. The config flag is retained, but its
>> default changes from true->false or vice versa. People who dislike the
>> new behavior can easily change back to the 2.x semantics.
>
> How does this garbage thing interact with "git clean -x"? My
> interpretation of this flag/attribute is that at version 3.0 by
> default all ignored files are _not_ garbage, so "git clean -x" should
> not remove any of them. Which is weird because most of ignored files
> are like *.o that should be removed.
>
> I also need to mark "precious" on untracked or even tracked files (*).
> Not sure how this "garbage" attribute interacts with that.
>
> (*) I was hoping I could get the idea [1] implemented in somewhat good
> shape before presenting here. But I'm a bit slow on that front. So
> yeah this "precious" on untracked/tracked thingy may be even
> irrelevant if the patch series will be rejected.

I think a garbage (or trashable) flag, if implemented, wouldn't need any
special case in git-clean, i.e. -x would remove all untracked files,
whether ignored or garbage/trashable. That's what my patch to implement
it does:
https://public-inbox.org/git/87zhuf3gs0@evledraar.gmail.com/

I think that makes sense. Users running "git clean" have "--dry-run" and
unlike "checkout a branch" or "merge this commit" where we'll now shred
data implicitly it's obvious that git-clean is going to shred your data.

Re: [RFC PATCH] Introduce "precious" file concept

[Duy Nguyen]

On Mon, Nov 26, 2018 at 10:30 AM Per Lundberg  wrote:
>
> On 11/13/18 1:22 AM, brian m. carlson wrote:
> > This is going to totally hose automation.  My last job had files which
> > might move from tracked to untracked (a file that had become generated),
> > and long-running CI and build systems would need to be able to check out
> > one status and switch to the other.  Your proposed change will prevent
> > those systems from working, whereas they previously did.
> >
> > I agree that your proposal would have been a better design originally,
> > but breaking the way automated systems currently work is probably going
> > to be a dealbreaker.
>
> How about something like this:
>
> 1. Introduce a concept with "garbage" files, which git is "permitted to
> delete" without prompting.
>
> 2. Retain the current default, i.e. "ignored files are garbage" for now,
> making the new behavior _opt in_ to avoid breaking automated
> systems/existing scripts for anyone. Put the setting for this behind a
> new core.* config flag.
>
> 3. In the plan for version 3.0 (a new major version where some breakage
> can be tolerable, according to Semantic Versioning), change the default
> so that "only explicit garbage is garbage". Include very clear notices
> of this in the release notes. The config flag is retained, but its
> default changes from true->false or vice versa. People who dislike the
> new behavior can easily change back to the 2.x semantics.

How does this garbage thing interact with "git clean -x"? My
interpretation of this flag/attribute is that at version 3.0 by
default all ignored files are _not_ garbage, so "git clean -x" should
not remove any of them. Which is weird because most of ignored files
are like *.o that should be removed.

I also need to mark "precious" on untracked or even tracked files (*).
Not sure how this "garbage" attribute interacts with that.

(*) I was hoping I could get the idea [1] implemented in somewhat good
shape before presenting here. But I'm a bit slow on that front. So
yeah this "precious" on untracked/tracked thingy may be even
irrelevant if the patch series will be rejected.

[1] 
https://public-inbox.org/git/cacsjy8c3rofv0kqejrwufqqzbnfu4msxjtpheybgmmrroff...@mail.gmail.com/

> Would this be a reasonable compromise for everybody?
-- 
Duy

Re: git bash lunching error

[Konstantin Khomoutov]

On Mon, Nov 26, 2018 at 08:32:27PM +0530, Chandra Shekhar wrote:

> cess: Resource temporarily unavailable (-1).
> DLL rebasing may be required; see 'rebaseall / rebase --help'.

This  ?

Please do not post screenshots, post text.
Also please copy and paste correctly when you do this.

Re: [RFC PATCH] Introduce "precious" file concept

[Junio C Hamano]

Per Lundberg  writes:

> How about something like this:
> ...
> Would this be a reasonable compromise for everybody?

I do not think you'd need to introduce such a deliberately breaking
change at all.  Just introduce a new "precious" class, perhaps mark
them with the atttribute mechanism, and that would be the endgame.
Early adopters would start marking ignored but not expendable paths
with the "precious" attribute and they won't be clobbered.  As the
mechanism becomes widely known and mature, more and more people use
it.  And even after that happens, early adopters do not have to change
any attribute setting, and late adopters would have plenty of examples
to imitate.  Those who do not need any "precious" class do not have
to do anything and you won't break any existing automation that way.

Re: [RFC PATCH] Introduce "precious" file concept

[Ævar Arnfjörð Bjarmason]

On Mon, Nov 26 2018, Per Lundberg wrote:

> On 11/13/18 1:22 AM, brian m. carlson wrote:
>> This is going to totally hose automation.  My last job had files which
>> might move from tracked to untracked (a file that had become generated),
>> and long-running CI and build systems would need to be able to check out
>> one status and switch to the other.  Your proposed change will prevent
>> those systems from working, whereas they previously did.
>>
>> I agree that your proposal would have been a better design originally,
>> but breaking the way automated systems currently work is probably going
>> to be a dealbreaker.
>
> How about something like this:
>
> 1. Introduce a concept with "garbage" files, which git is "permitted to
> delete" without prompting.
>
> 2. Retain the current default, i.e. "ignored files are garbage" for now,
> making the new behavior _opt in_ to avoid breaking automated
> systems/existing scripts for anyone. Put the setting for this behind a
> new core.* config flag.
>
> 3. In the plan for version 3.0 (a new major version where some breakage
> can be tolerable, according to Semantic Versioning), change the default
> so that "only explicit garbage is garbage". Include very clear notices
> of this in the release notes. The config flag is retained, but its
> default changes from true->false or vice versa. People who dislike the
> new behavior can easily change back to the 2.x semantics.
>
> Would this be a reasonable compromise for everybody?

Possibly, but I think there's an earlier step zero there for anyone
interested in pursuing this (and currently I can't make time for it),
which is to submit a patch with tests and documentation showing exactly
the sort of scenarios where we clobber or don't clobber existing files.

As my https://public-inbox.org/git/87zhuf3gs0@evledraar.gmail.com/
shows we have tests for this, but they're not explicit, and some want to
test some unrelated thing.

I.e. to test the cases where we clobber foo.c because foo.c now
explicitly exists, or cases where dir/foo.c is clobbered because "dir"
is now a tracked text file etc., are those the only two cases? I vaguely
suspect that there were other interesting cases, but at this point the
information has been paged out of the working set of my wetware. The
thread at
https://public-inbox.org/git/87o9au39s7@evledraar.gmail.com/ has
some notes about this.

Then as noted in
https://public-inbox.org/git/87wopj3661@evledraar.gmail.com/ the
reason we have this behavior seems to be something that grew organically
from a semi-related bugfix.

So I don't think we're at a point where we're all dug into our trenches
and some people want X and others want Y and in the name of backwards
compatibility we're going to stay with X. It may turn out that we just
want to retain 10% of X, and can get 99% of the safety of Y by doing
that.

Re: [RFC PATCH] Introduce "precious" file concept

[Per Lundberg]

On 11/13/18 1:22 AM, brian m. carlson wrote:
> This is going to totally hose automation.  My last job had files which
> might move from tracked to untracked (a file that had become generated),
> and long-running CI and build systems would need to be able to check out
> one status and switch to the other.  Your proposed change will prevent
> those systems from working, whereas they previously did.
> 
> I agree that your proposal would have been a better design originally,
> but breaking the way automated systems currently work is probably going
> to be a dealbreaker.

How about something like this:

1. Introduce a concept with "garbage" files, which git is "permitted to 
delete" without prompting.

2. Retain the current default, i.e. "ignored files are garbage" for now, 
making the new behavior _opt in_ to avoid breaking automated 
systems/existing scripts for anyone. Put the setting for this behind a 
new core.* config flag.

3. In the plan for version 3.0 (a new major version where some breakage 
can be tolerable, according to Semantic Versioning), change the default 
so that "only explicit garbage is garbage". Include very clear notices 
of this in the release notes. The config flag is retained, but its 
default changes from true->false or vice versa. People who dislike the 
new behavior can easily change back to the 2.x semantics.

Would this be a reasonable compromise for everybody?
-- 
Per Lundberg

Re: [PATCH v3 2/5] pretty: allow showing specific trailers

[Junio C Hamano]

Anders Waldenborg  writes:

> Would it feel less inconsistent if it did not set the 'only_trailers'
> option?

If %(trailers:key=...) did not automatically imply 'only', it would
be very consistent.

But as I already said, I think it would be less convenient, as I do
suspect that those who want specific keys would want to see only
those trailers with specific keys.

And if we want that convinience, we'd probably want a way to
optionally disable that 'only' bit when the user wants to.

And...

> --- a/Documentation/pretty-formats.txt
> +++ b/Documentation/pretty-formats.txt
> @@ -228,9 +228,9 @@ endif::git-rev-list[]
>  ** 'key=': only show trailers with specified key. Matching is done
> case-insensitively and trailing colon is optional. If option is
> given multiple times trailer lines matching any of the keys are
> -   shown. Non-trailer lines in the trailer block are also hidden
> -   (i.e. 'key' implies 'only'). E.g., `%(trailers:key=Reviewed-by)`
> -   shows trailer lines with key `Reviewed-by`.
> +   shown. Non-trailer lines in the trailer block are also hidden.
> +   E.g., `%(trailers:key=Reviewed-by)` shows trailer lines with key
> +   `Reviewed-by`.

... I do not think this change reduces the puzzlement felt by
readers who would have wondered how that implied 'only' can be
countermanded with the old text.  It just makes it look even less
explained to them.

If we assume that nobody would ever want to mix non-trailers when
asking specific keywords, then "them" in the above paragraph would
become an empty set, and we do not have to worry about them.  I am
not sure if Git is still such a small project to allow us rely on
such an assumption, though.

>  ** 'only': omit non-trailer lines from the trailer block.
>  ** 'unfold': make it behave as if interpret-trailer's `--unfold`
> option was given. E.g., `%(trailers:only,unfold)` unfolds and

Re: [PATCH v11 00/22] Convert "git stash" to C builtin

[Junio C Hamano]

Junio C Hamano  writes:

> Thomas Gummerer  writes:
>
>> Thanks for your work on this!  I have read through the range-diff and
>> the new patch of this last round, and this addresses all the comments
>> I had on v10 (and some more :)).  I consider it
>> Reviewed-by: Thomas Gummerer 
>
> Thanks.
>
> One thing that bothers me is that this seems to have been rebased on
> 'master', but as long as we are rebasing, the updated series must
> also take into account of the sd/stash-wo-user-name topic, i.e. if
> we are rebasing it, it should be rebased on top of the result of
>
>   git checkout -B ps/rebase-in-c master
>   git merge --no-ff sd/stash-wo-user-name
>
> I think.

https://travis-ci.org/git/git/builds/459619672 would show that this
C reimplementation now regresses from the scripted version due to
lack of such rebasing (i.e. porting a correction from scripted one).

Re: [ANNOUNCE] Git v2.20.0-rc1

[Junio C Hamano]

Unless I hear otherwise in the next 24 hours, I am planning to
merge the following topics to 'master' before cutting -rc2.  Please
stop me on any of these topics.

 - jc/postpone-rebase-in-c

   This may be the most controversial.  It demotes the C
   reimplementation of "git rebase" to an experimental opt-in
   feature that can only be enabled by setting rebase.useBuiltIn
   configuration that defaults to false.

   cf. 


 - ab/format-patch-rangediff-not-stat

   The "--rangediff" option recently added to "format-patch"
   interspersed a bogus and useless "--stat" information by mistake,
   which is being corrected.

   cf. 


The following three topics I do not need help in deciding; they are
all good and will be merged before -rc2.

 - jk/t5562-perl-path-fix

   This is to invoke a perl scriptlet with "$PERL_PATH" in one of
   the new tests, instead of relying on (an incorrect) she-bang line
   in the script file.

 - tb/clone-case-smashing-warning-test

   This enables the test to see the behaviour of "git clone" after
   cloning a project that has paths that are different only in case
   on MINGW (earlier it wanted CASE_INSENSITIVE_FS prerequisite and
   in addition not on MINGW).

 - nd/per-worktree-ref-iteration

   This fixes a "return function_that_returns_void(...)" in a
   function that returns void.


Thanks.

Re: [PATCH] doc: update diff-format.txt for removed ellipses in --raw

[Junio C Hamano]

Greg Hurrell  writes:

> Since 7cb6ac1e4b ("diff: diff_aligned_abbrev: remove ellipsis after
> abbreviated SHA-1 value", 2017-12-03), the "--raw" format of diff
> does not add ellipses in an attempt to align the output, but the
> documentation was not updated to reflect this.
>
> Signed-off-by: Greg Hurrell 
> ---
>
> The GIT_PRINT_SHA1_ELLIPSIS environment variable can be used, for now,
> to bring back the old output format, but that is already documented in
> git.txt, so I am not mentioning it here.

Thanks. Will queue.

Re: [PATCH v3 2/5] pretty: allow showing specific trailers

[Anders Waldenborg]

Junio C Hamano writes:
> I was confused by the "only" stuff.
>
> When you give a key (or two), they cannot possibly name non-trailer
> lines, so while it may be possible to ask "oh, by the way, I also
> want non-trailer lines in addition to signed-off-by and cc lines",
> the value of being able to make such a request is dubious.
>
> The value is dubious, but logically it makes it more consistent with
> the current %(trailers) that lack 'only', which is "oh by the way, I
> also want non-trailer lines in addition to the trailers with
> keyword", to allow a way to countermand the 'only' you flip on by
> default when keywords are given.


Would it feel less inconsistent if it did not set the 'only_trailers'
option?

Now that I look at it again setting 'only_trailers' is more of an
implementation trick/hack to make sure it doesn't take the fast-path in
format_trailer_info (and by documenting it it justifies that hack). If
instead the 'filter' option is checked in the relevant places there
would be no need to mix up 'only' with 'filter'.

That is, do you think something like this should be squashed in?

diff --git a/pretty.c b/pretty.c
index 302e67fa8..f45ccaf51 100644
--- a/pretty.c
+++ b/pretty.c
@@ -1360,7 +1360,6 @@ static size_t format_commit_one(struct strbuf *sb, /* in 
UTF-8 */

opts.filter = format_trailer_match_cb;
opts.filter_data = _list;
-   opts.only_trailers = 1;
} else
break;
}
diff --git a/trailer.c b/trailer.c
index 97c8f2762..07ca2b2c6 100644
--- a/trailer.c
+++ b/trailer.c
@@ -1132,7 +1132,7 @@ static void format_trailer_info(struct strbuf *out,
size_t i;

/* If we want the whole block untouched, we can take the fast path. */
-   if (!opts->only_trailers && !opts->unfold) {
+   if (!opts->only_trailers && !opts->unfold && !opts->filter) {
strbuf_add(out, info->trailer_start,
   info->trailer_end - info->trailer_start);
return;
@@ -1156,7 +1156,7 @@ static void format_trailer_info(struct strbuf *out,
strbuf_release();
strbuf_release();

-   } else if (!opts->only_trailers) {
+   } else if (!opts->only_trailers && !opts->filter) {
strbuf_addstr(out, trailer);
}
}
diff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt
index 7548e1d38..ea3cd5b28 100644
--- a/Documentation/pretty-formats.txt
+++ b/Documentation/pretty-formats.txt
@@ -228,9 +228,9 @@ endif::git-rev-list[]
 ** 'key=': only show trailers with specified key. Matching is done
case-insensitively and trailing colon is optional. If option is
given multiple times trailer lines matching any of the keys are
-   shown. Non-trailer lines in the trailer block are also hidden
-   (i.e. 'key' implies 'only'). E.g., `%(trailers:key=Reviewed-by)`
-   shows trailer lines with key `Reviewed-by`.
+   shown. Non-trailer lines in the trailer block are also hidden.
+   E.g., `%(trailers:key=Reviewed-by)` shows trailer lines with key
+   `Reviewed-by`.
 ** 'only': omit non-trailer lines from the trailer block.
 ** 'unfold': make it behave as if interpret-trailer's `--unfold`
option was given. E.g., `%(trailers:only,unfold)` unfolds and

Re: [RFC PATCH 7/7] config.mak.uname: use pkgsrc perl for NetBSD

[Junio C Hamano]

Carlo Marcelo Arenas Belón   writes:

> otherwise will default to /usr/bin/perl which wouldn't normally exist
>
> Signed-off-by: Carlo Marcelo Arenas Belón 
> ---
>  config.mak.uname | 1 +
>  1 file changed, 1 insertion(+)

I do not have experience with NetBSD so I take your words on face
value.  This patch makes sense to me.

Thanks.

> diff --git a/config.mak.uname b/config.mak.uname
> index 59ce03819b..d2edb723f4 100644
> --- a/config.mak.uname
> +++ b/config.mak.uname
> @@ -249,6 +249,7 @@ ifeq ($(uname_S),NetBSD)
>   OLD_ICONV = UnfortunatelyYes
>   BASIC_CFLAGS += -I/usr/pkg/include
>   BASIC_LDFLAGS += -L/usr/pkg/lib $(CC_LD_DYNPATH)/usr/pkg/lib
> + PERL_PATH = /usr/pkg/bin/perl
>   USE_ST_TIMESPEC = YesPlease
>   HAVE_PATHS_H = YesPlease
>   HAVE_BSD_SYSCTL = YesPlease

Re: [RFC PATCH 6/7] t5004: use GNU tar to avoid known issues with BSD tar

[Junio C Hamano]

Carlo Marcelo Arenas Belón   writes:

> 56ee96572a ("t5004: resurrect original empty tar archive test", 2013-05-09)
> added a test to try to detect and workaround issues with the standard tar
> from BSD, but at least in NetBSD would be better to instead require GNU tar
> which is available from pkgsrc
>
> Signed-off-by: Carlo Marcelo Arenas Belón 
> ---
>  t/t5004-archive-corner-cases.sh | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/t/t5004-archive-corner-cases.sh b/t/t5004-archive-corner-cases.sh
> index ced44355ca..baafc553f8 100755
> --- a/t/t5004-archive-corner-cases.sh
> +++ b/t/t5004-archive-corner-cases.sh
> @@ -31,6 +31,8 @@ test_lazy_prereq UNZIP_ZIP64_SUPPORT '
>   "$GIT_UNZIP" -v | grep ZIP64_SUPPORT
>  '
>  
> +test $uname_s = NetBSD && TAR="gtar"
> +

This smells wrong.

Isn't the top-level Makefile ask you to use TAR=gtar if that is the
usable implementation of tar on your platform, and isn't what you
specify as $(TAR) exported down to the t/Makefile to be used here?

>  # bsdtar/libarchive versions before 3.1.3 consider a tar file with a
>  # global pax header that is not followed by a file record as corrupt.
>  if "$TAR" tf "$TEST_DIRECTORY"/t5004/empty-with-pax-header.tar >/dev/null 
> 2>&1

Re: [RFC PATCH 5/7] test-lib: use pkgsrc provided unzip for NetBSD

[Junio C Hamano]

Carlo Marcelo Arenas Belón   writes:

> d98b2c5fce ("test-lib: on FreeBSD, look for unzip(1) in /usr/local/bin/",
> 2016-07-21) added an exception to the test suite for FreeBSD because the
> tests assume functionality not provided by its base unzip tool.
>
> NetBSD shares that limitation and provides a package that could be used
> instead so all tests from t5003 succeed
>
> Signed-off-by: Carlo Marcelo Arenas Belón 
> ---
>  t/test-lib.sh | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/t/test-lib.sh b/t/test-lib.sh
> index 6c6c0af7a1..2acb35f277 100644
> --- a/t/test-lib.sh
> +++ b/t/test-lib.sh
> @@ -1244,6 +1244,7 @@ test_lazy_prereq SANITY '
>  '
>  
>  test FreeBSD != $uname_s || GIT_UNZIP=${GIT_UNZIP:-/usr/local/bin/unzip}
> +test $uname_s = NetBSD && GIT_UNZIP=${GIT_UNZIP:-/usr/pkg/bin/unzip}

This is OK for now, but I wonder why this is done in test-lib.sh in
the first place, unlike $(TAR) that is set and configurable from the
top level.  The difference is that GIT_UNZIP happens to be used only
in tests, while TAR is used in the primary build procedure.

But I suspect that in the longer run, we should allow UNZIP to be
given next to TAR to the top-level Makefile and export it down.
That would allow the usual mechanism like config.mak.uname,
./configure and "make TAR=... UNZIP=..." command line override
to be used uniformly, without people having to worry about the
distinction.  The builders should *not* have to care that one is
used in the build and the other is only used in the test.

>  GIT_UNZIP=${GIT_UNZIP:-unzip}
>  test_lazy_prereq UNZIP '
>   "$GIT_UNZIP" -v

Re: [RFC PATCH 4/7] config.mak.uname: NetBSD uses old iconv interface

[Junio C Hamano]

Carlo Marcelo Arenas Belón   writes:

> prevents the following warning :

s/^/Doing so / or something to make it a complete sentence.

> ...
> it is set by optional configure at least since NetBSD 6.0

s/it/It/;


Again, makes sense, and thanks for tying this loose end.

> Signed-off-by: Carlo Marcelo Arenas Belón 
> ---
>  config.mak.uname | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/config.mak.uname b/config.mak.uname
> index 36c973c7e6..59ce03819b 100644
> --- a/config.mak.uname
> +++ b/config.mak.uname
> @@ -246,6 +246,7 @@ ifeq ($(uname_S),NetBSD)
>   ifeq ($(shell expr "$(uname_R)" : '[01]\.'),2)
>   NEEDS_LIBICONV = YesPlease
>   endif
> + OLD_ICONV = UnfortunatelyYes
>   BASIC_CFLAGS += -I/usr/pkg/include
>   BASIC_LDFLAGS += -L/usr/pkg/lib $(CC_LD_DYNPATH)/usr/pkg/lib
>   USE_ST_TIMESPEC = YesPlease

Re: [RFC PATCH 3/7] config.mak.uname: NetBSD uses BSD semantics with fread for directories

[Junio C Hamano]

Carlo Marcelo Arenas Belón   writes:

> this "fixes" test 23 (proper error on directory "files") from t1308
>
> other BSD (OpenBSD, MirBSD) likely also affected but they will be
> fixed in a different series
>
> the optional 'configure' sets this automatically and is probably what
> most users from this platform had been doing as a workaround

Yup, thanks for straightening this out.

>
> Signed-off-by: Carlo Marcelo Arenas Belón 
> ---
>  config.mak.uname | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/config.mak.uname b/config.mak.uname
> index 3ee7da0e23..36c973c7e6 100644
> --- a/config.mak.uname
> +++ b/config.mak.uname
> @@ -253,6 +253,7 @@ ifeq ($(uname_S),NetBSD)
>   HAVE_BSD_SYSCTL = YesPlease
>   HAVE_BSD_KERN_PROC_SYSCTL = YesPlease
>   PROCFS_EXECUTABLE_PATH = /proc/curproc/exe
> + FREAD_READS_DIRECTORIES = UnfortunatelyYes
>  endif
>  ifeq ($(uname_S),AIX)
>   DEFAULT_PAGER = more

Re: [RFC PATCH 2/7] t0301: remove trailing / for dir creation

[Junio C Hamano]

Carlo Marcelo Arenas Belón   writes:

> the semantics of how mkdir -p should work, specially when using -m are
> not standard and in this case NetBSD will assume that the permision
> should not be changed, breaking the test

This does not explain, except for the fuzzy "in this case", why we
want to lose trailing slash at all.  If what you wanted to say was

On NetBSD, "mkdir -p -m $bits path/to/dir/" ignores the
permission bits when creating the directory component 'dir', but
without the trailing slash at the end of "dir/", it works as
expected.

then that would be an understandable justification for the patch.
If you meant to say something else, then I couldn't read it from
what you wrote, so the log message needs updating to help future
readers.

> -p is technically not needed either, but will be cleared in a future
> patch eventhough it could be considered an alternative fix

I haven't seen the steps 3-7/7 yet, but if they remove "-p", then
"but" would be a strange thing to say here.  If they indeed do, then
perhaps:

The '-p' option is not needed in thse cases, as we know $HOME/
exists in the test environment and we are creating a new
directory directly under it.  It will be removed in a future
patch in the series.  Removing '-p' could be an alternative fix,
as the command then works as expected even on NetBSD with the
trailing slash after directory name.

On the other hand, if future changes make it necessary to create two
or more levels here, then

At this point in the series, '-p' is not needed in thse cases,
as we know $HOME/ exists in the test environment and we are
creating .git-credential-cache directory directly under it.
However, in later patches, we'll make it necessary for these
'mkdir -p' to create two or more levels, so removing '-p' would
be an alternative fix for this step, but would not work for the
series as a whole.

would also make sense.  I simply do not have enough information yet
to tell which at this point in the series.

> Signed-off-by: Carlo Marcelo Arenas Belón 
> ---
>  t/t0301-credential-cache.sh | 8 
>  1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/t/t0301-credential-cache.sh b/t/t0301-credential-cache.sh
> index fd92533acf..9529c612af 100755
> --- a/t/t0301-credential-cache.sh
> +++ b/t/t0301-credential-cache.sh
> @@ -77,9 +77,9 @@ test_expect_success "use custom XDG_CACHE_HOME even if xdg 
> socket exists" '
>  test_expect_success 'use user socket if user directory exists' '
>   test_when_finished "
>   git credential-cache exit &&
> - rmdir \"\$HOME/.git-credential-cache/\"
> + rmdir \"\$HOME/.git-credential-cache\"
>   " &&
> - mkdir -p -m 700 "$HOME/.git-credential-cache/" &&
> + mkdir -p -m 700 "$HOME/.git-credential-cache" &&
>   check approve cache <<-\EOF &&
>   protocol=https
>   host=example.com
> @@ -92,10 +92,10 @@ test_expect_success 'use user socket if user directory 
> exists' '
>  test_expect_success SYMLINKS 'use user socket if user directory is a symlink 
> to a directory' '
>   test_when_finished "
>   git credential-cache exit &&
> - rmdir \"\$HOME/dir/\" &&
> + rmdir \"\$HOME/dir\" &&
>   rm \"\$HOME/.git-credential-cache\"
>   " &&
> - mkdir -p -m 700 "$HOME/dir/" &&
> + mkdir -p -m 700 "$HOME/dir" &&
>   ln -s "$HOME/dir" "$HOME/.git-credential-cache" &&
>   check approve cache <<-\EOF &&
>   protocol=https

Re: [RFC PATCH 1/7] Documentation: update INSTALL for NetBSD

[Junio C Hamano]

Carlo Marcelo Arenas Belón   writes:

> NetBSD added a BSD licensed reimplementation of GNU libintl to
> its base at least since release 4.0 (mid 2012) and git can be
> configured to build with it.
>
> Signed-off-by: Carlo Marcelo Arenas Belón 
> ---
>  INSTALL | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)

Looks good.

> diff --git a/INSTALL b/INSTALL
> index c39006e8e7..100718989f 100644
> --- a/INSTALL
> +++ b/INSTALL
> @@ -154,11 +154,11 @@ Issues of note:
> git-gui, you can use NO_TCLTK.
>  
>   - A gettext library is used by default for localizing Git. The
> -   primary target is GNU libintl, but the Solaris gettext
> -   implementation also works.
> +   primary target is GNU libintl, but the Solaris and NetBSD gettext
> +   implementations also work.
>  
> We need a gettext.h on the system for C code, gettext.sh (or
> -   Solaris gettext(1)) for shell scripts, and libintl-perl for Perl
> +   gettext(1) utility) for shell scripts, and libintl-perl for Perl
> programs.
>  
> Set NO_GETTEXT to disable localization support and make Git only

[PATCH] rebase: mark the C reimplementation as an experimental opt-in feature (was Re: [ANNOUNCE] Git v2.20.0-rc1)

[Junio C Hamano]

Junio C Hamano  writes:

> Ævar Arnfjörð Bjarmason  writes:
>
>>>  * "git rebase" and "git rebase -i" have been reimplemented in C.
>>
>> Here's another regression in the C version (and rc1),...
>> I wasn't trying to stress test rebase. I was just wanting to rebase a
>> history I was about to force-push after cleaning it up, hardly an
>> obscure use-case. So [repeat last transmission in
>> https://public-inbox.org/git/87y39w1wc2@evledraar.gmail.com/ ]
>
> which, to those who are reading from sidelines:
>
> Given that we're still finding regressions bugs in the rebase-in-C
> version should we be considering reverting 5541bd5b8f ("rebase: default
> to using the builtin rebase", 2018-08-08)?
>
> I love the feature, but fear that the current list of known regressions
> serve as a canary for a larger list which we'd discover if we held off
> for another major release (and would re-enable rebase.useBuiltin=true in
> master right after 2.20 is out the door).
>
> I am fine with the proposed flip, but I'll have to see the extent of
> damage this late in the game so that I won't miss anything.  In
> addition to the one-liner below, we'd need to update the quoted
> release notes entry, and possibly adjust some tests (even though the
> "reimplementation" ought to be bug-to-bug compatible, it may not be).

So, in a more concrete form, what you want to see is something like
this in -rc2 and later?

-- >8 --
Subject: [PATCH] rebase: mark the C reimplementation as an experimental opt-in 
feature

It turns out to be a bit too early to unleash the reimplementation
to the general public.  Let's rewrite some documentation and make it
an opt-in feature.

Signed-off-by: Junio C Hamano 
---
 Documentation/config/rebase.txt | 16 ++--
 builtin/rebase.c|  2 +-
 t/README|  4 ++--
 3 files changed, 9 insertions(+), 13 deletions(-)

diff --git a/Documentation/config/rebase.txt b/Documentation/config/rebase.txt
index f079bf6b7e..af12623151 100644
--- a/Documentation/config/rebase.txt
+++ b/Documentation/config/rebase.txt
@@ -1,16 +1,12 @@
 rebase.useBuiltin::
-   Set to `false` to use the legacy shellscript implementation of
-   linkgit:git-rebase[1]. Is `true` by default, which means use
-   the built-in rewrite of it in C.
+   Set to `true` to use the experimental reimplementation of
+   linkgit:git-rebase[1] in C.  Defaults to `false`.
 +
 The C rewrite is first included with Git version 2.20. This option
-serves an an escape hatch to re-enable the legacy version in case any
-bugs are found in the rewrite. This option and the shellscript version
-of linkgit:git-rebase[1] will be removed in some future release.
-+
-If you find some reason to set this option to `false` other than
-one-off testing you should report the behavior difference as a bug in
-git.
+allows early adopters to opt into the experimental version to find
+bugs in the rewritten version. This option and the shellscript version
+of linkgit:git-rebase[1] will be removed in some future release once
+the reimplementation becomes more stable.
 
 rebase.stat::
Whether to show a diffstat of what changed upstream since the last
diff --git a/builtin/rebase.c b/builtin/rebase.c
index 5b3e5baec8..19ad97b177 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -59,7 +59,7 @@ static int use_builtin_rebase(void)
cp.git_cmd = 1;
if (capture_command(, , 6)) {
strbuf_release();
-   return 1;
+   return 0;
}
 
strbuf_trim();
diff --git a/t/README b/t/README
index 28711cc508..7e925e5fea 100644
--- a/t/README
+++ b/t/README
@@ -345,8 +345,8 @@ for the index version specified.  Can be set to any valid 
version
 GIT_TEST_PRELOAD_INDEX= exercises the preload-index code path
 by overriding the minimum number of cache entries required per thread.
 
-GIT_TEST_REBASE_USE_BUILTIN=, when false, disables the
-builtin version of git-rebase. See 'rebase.useBuiltin' in
+GIT_TEST_REBASE_USE_BUILTIN=, when true, forces the use of
+builtin version of git-rebase in the test. See 'rebase.useBuiltin' in
 git-config(1).
 
 GIT_TEST_INDEX_THREADS= enables exercising the multi-threaded loading
-- 
2.20.0-rc1

Re: [PATCH v11 00/22] Convert "git stash" to C builtin

[Junio C Hamano]

Thomas Gummerer  writes:

> Thanks for your work on this!  I have read through the range-diff and
> the new patch of this last round, and this addresses all the comments
> I had on v10 (and some more :)).  I consider it
> Reviewed-by: Thomas Gummerer 

Thanks.

One thing that bothers me is that this seems to have been rebased on
'master', but as long as we are rebasing, the updated series must
also take into account of the sd/stash-wo-user-name topic, i.e. if
we are rebasing it, it should be rebased on top of the result of

git checkout -B ps/rebase-in-c master
git merge --no-ff sd/stash-wo-user-name

I think.

Re: [PATCH v11 20/22] stash: convert `stash--helper.c` into `stash.c`

[Junio C Hamano]

Paul-Sebastian Ungureanu  writes:

> The old shell script `git-stash.sh`  was removed and replaced
> entirely by `builtin/stash.c`. In order to do that, `create` and
> `push` were adapted to work without `stash.sh`. For example, before
> this commit, `git stash create` called `git stash--helper create
> --message "$*"`. If it called `git stash--helper create "$@"`, then
> some of these changes wouldn't have been necessary.
>
> This commit also removes the word `helper` since now stash is
> called directly and not by a shell script.
>
> Signed-off-by: Paul-Sebastian Ungureanu 
> ---
>  .gitignore   |   1 -
>  Makefile |   3 +-
>  builtin.h|   2 +-
>  builtin/{stash--helper.c => stash.c} | 157 +++
>  git-stash.sh | 153 --
>  git.c|   2 +-
>  6 files changed, 92 insertions(+), 226 deletions(-)
>  rename builtin/{stash--helper.c => stash.c} (91%)
>  delete mode 100755 git-stash.sh

Seeing the recent trouble in "rebase in C" and how keeping the
scripted version as "git legacy-rebase" helped us postpone the
rewritten version without ripping the whole thing out, I wonder if
we can do the same here.

Also, the remaining two patches should be done _before_ this step, I
would think.  I can understand it if the reason you have those two
after this step is because you found the opportunity for these
improvements after you wrote this step, but in the larger picture
seen by the end users of the "stash in C" and those developers who
follow the evolution of the code, the logical place for this "now we
have everything in C, we retire the scripted version" step to happen
is at the very end.

Thanks.

Re: [PATCH] files-backend.c: fix build error on Solaris

[Junio C Hamano]

Carlo Arenas  writes:

> Signed-off-by: Carlo Marcelo Arenas Belón 

Do you mean Tested-by: (meaning, you actually saw the breakage with
SunCC without the patch and also saw the patch fixed the breakage)?

> clang with -Wpedantic also catch this (at least with Apple LLVM
> version 10.0.0); recent versions of gcc also include that flag and at
> least 8.2.0 shows a warning for it, so it might be worth adding it to
> developer mode (maybe under the pedantic DEVOPTS).

Nice to know.

> ..., would this be something I should be adding?, what are the expectations 
> around
> warnings and compilers?

It may be a worthwhile thing to discuss, and as a discussion
starter, a patch would help ;-).

Re: [PATCH] setup.c: remove needless argument passed to open in sanitize_stdfds

[Junio C Hamano]

pedrodel...@gmail.com writes:

> According to POSIX manual pages, the open() system call's mode
> argument specifies the file mode bits to be applied when a new
> file is created. If neither O_CREAT nor O_TMPFILE is specified,
> then mode is ignored.

Correct.  

While I would say two argument form would have been better if this
were a new code, this change is borderline Meh for existing code,
because passing 0 there already gives a reasonable sign that we know
this parameter does not matter.

If the ignored parameter were 0666 or some other more
plausible-as-perm-bits value, the story would be quite different,
though.

Thanks.

Re: BUG: CR marker ^M doesn't show up in '-' lines of diffs when the ending of the removed line is CR+LF

[Junio C Hamano]

Johannes Sixt  writes:

> But incorrect whitespace is never highlighted in removed lines, why
> should CR be an exception?
> ...
> Same here for other cases, for example
> 
> -something
> +something
>
> will not have on obvious indicator that whitespace was corrected.

All correct, but misses one point in Frank's original report, which
observed

-something
+something_new^M

with ^M highlighted for whitespace error.  The highlighting is
correct.  But notice lack of caret-em on the preimage line?

It turns out that we show something like this

-something CR LF

for the preimage line, while showing something like this

+something_new CR  LF

for the postimage line.  

Because CR on the postimage line, thanks to highlighting, appears
alone separate from the LF, it is shown as two-letter caret-em
sequence to the user.

On the other hand, because CR and LF appear next to each other on
the preimage line, the pager and/or the terminal behaves as if CR is
not even there and that is where Frank's complaint comes from, I think.

The code is doing the right thing by showing CR, but it is hidden by
the pager and/or the terminal.

Re: [PATCH v3 2/5] pretty: allow showing specific trailers

[Junio C Hamano]

Anders Waldenborg  writes:

> Junio C Hamano writes:
>> Also, use of 'key=' automatically turns on 'only' as described, and
>> I tend to agree that it would a convenient default mode (i.e. when
>> picking certain trailers only with this mechanism, it is likely that
>> the user is willing to use %(subject) etc. to fill in what was lost
>> by the implicit use of 'only'), but at the same time, it makes me
>> wonder if we need to have a way to countermand an 'only' (or
>> 'unfold' for that matter) that was given earlier, e.g.
>>
>>  %(trailers:key=signed-off-by,only=no)
>>
>> Thanks.
>
> I'm not sure what that would mean. The non-trailer lines in the trailer
> block doesn't match the key.

I was confused by the "only" stuff.

When you give a key (or two), they cannot possibly name non-trailer
lines, so while it may be possible to ask "oh, by the way, I also
want non-trailer lines in addition to signed-off-by and cc lines",
the value of being able to make such a request is dubious.

The value is dubious, but logically it makes it more consistent with
the current %(trailers) that lack 'only', which is "oh by the way, I
also want non-trailer lines in addition to the trailers with
keyword", to allow a way to countermand the 'only' you flip on by
default when keywords are given.

Re: [RFC] Introduce two new commands, switch-branch and restore-paths

[Junio C Hamano]

Thomas Gummerer  writes:

> I like the idea of splitting those commands up, in fact it is
> something I've been considering working on myself.  I do think we
> should consider if we want to change the behaviour of those new
> commands in any way compared to 'git checkout', since we're starting
> with a clean slate.
>
> One thing in particular that I have in mind is something I'm currently
> working on, namely adding a --index flag to 'git checkout', which
> would make 'git checkout' work in non-overlay mode (for more
> discussion on that see also [*1*].

Ah, thanks for reminding me of that.  That explains why I felt
uneasy to see "restore" in the proposed command name.  In short, I
think "checkout --index  ", i.e. if the 
matches a directory in  and the current index and/or the
working tree has tracked paths in that directory that do not exist
in , the operation _removes_ these paths so that the result
matches , should become the default of "I want to check out
these paths from the named tree-ish".  The current one is not
exactly "checking out the paths" in that it ignores and does not
check out the absense of paths in .  That operation sounds
more like "restoring paths out of a given tree".  If the tree does
not have some paths, these paths won't be "restored" from that tree,
so "restore" matches the current "overlay what's taken out of the
given tree on top of what is already in the index and the working
tree, without checking out the absense of paths" better from that
point of view.

Re: [PATCH] http-backend: enable cleaning up forked upload/receive-pack on exit

[Junio C Hamano]

Max Kirillov  writes:

> If http-backend dies because of errors, started upload-pack or
> receive-pack are not killed and waited, but rather stay running for somtime

"sometime" (will fix locally, no reason for a resend).

> until they exits because of closed stdin. It may be undesirable in working

"they exit" (ditto)

> environment, and it also causes occasional failure of t5562, because the
> processes keep opened act.err, and sometimes write there errors after next 
> test
> started using the file.
>
> Fix by enabling cleaning of the command at http-backed exit.

Thanks for a clear explanation.

Will queue.

> Reported-by: Carlo Arenas 
> Helped-by: Carlo Arenas 
> Signed-off-by: Max Kirillov 
> ---
> This seems to fix the issue at NetBSD. I verified it manually with strace but 
> could
> not catch the visible timing effect in tests at Linux. So no tests for it.
>
> the "t5562: do not reuse output files" patches are not needed then
>  http-backend.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/http-backend.c b/http-backend.c
> index 9e894f197f..29e68e38b5 100644
> --- a/http-backend.c
> +++ b/http-backend.c
> @@ -486,6 +486,8 @@ static void run_service(const char **argv, int 
> buffer_input)
>   if (buffer_input || gzipped_request || req_len >= 0)
>   cld.in = -1;
>   cld.git_cmd = 1;
> + cld.clean_on_exit = 1;
> + cld.wait_after_clean = 1;
>   if (start_command())
>   exit(1);

Re: [PATCH] t5562: do not reuse output files

[Junio C Hamano]

Max Kirillov  writes:

> On Sat, Nov 24, 2018 at 04:47:19PM +0900, Junio C Hamano wrote:
>> I do agree that forcing the parent to wait, like you described in
>> the comment, would be far more preferrable,
>
> It looks like it can be done as simple as:
>
> --- a/http-backend.c
> +++ b/http-backend.c
> @@ -486,6 +486,8 @@ static void run_service(const char **argv, int 
> buffer_input)
> if (buffer_input || gzipped_request || req_len >= 0)
> cld.in = -1;
> cld.git_cmd = 1;
> +   cld.clean_on_exit = 1;
> +   cld.wait_after_clean = 1;
> if (start_command())
> exit(1);
>
> at least according to strate it does what it should.

Sounds sane.

I am offhand not sure what the right value of wait_after_clean for
this codepath be, though.  46df6906 ("execv_dashed_external: wait
for child on signal death", 2017-01-06) made this non-default but
turned it on for dashed externals (especially to help the case where
they spawn a pager), as the parent has nothing other than to wait
for the child to exit in that codepath.  Does the same reasoning
apply here, too?

This is a meta point, but I wonder if there is an easy way to "grep"
for uses of run-command interface that do *not* set clean_on_exit.
The pager that can outlive us long after we exit, kept alive while
the user views our output, is an example cited by afe19ff7
("run-command: optionally kill children on exit", 2012-01-07), and
while I am wondering if the default should hae been to kill the
children instead, such a "grep" would have been very useful to know
what codepaths would be affected if we flipped the default.

Re: BUG: CR marker ^M doesn't show up in '-' lines of diffs when the ending of the removed line is CR+LF

[brian m. carlson]

On Sun, Nov 25, 2018 at 03:03:11PM +0100, Frank Schäfer wrote:
> Am 24.11.18 um 23:07 schrieb Johannes Sixt:
> > I don't think that there is anything to fix. If you have a file with
> > CRLF in it, but you did not declare to Git that CRLF is the expected
> > end-of-line indicator, then the CR *is* trailing whitespace (because
> > the line ends at LF), and 'git diff' highlights it. 
> 
> Sure, it's correct to highlight it.
> But it doesn't highlight it in removed lines, just in added lines.
> I can see no good reason why removed and added lines should be treated
> differently.

The default behavior is to highlight whitespace errors only in new
lines, because the assumption is that while you don't want to introduce
new errors, you can't do anything about old mistakes without rewriting
history.

I agree that in many circumstances, such as code review, this may be
undesirable.  In the past, I've done code reviews where I may let
existing trailing whitespace go but am strict about not introducing
new trailing whitespace, and being able to see both is helpful.

If you want to see whitespace errors in both the old and the new, use
--ws-error-highlight or set diff.wsErrorHighlight.
-- 
brian m. carlson: Houston, Texas, US
OpenPGP: https://keybase.io/bk2204


signature.asc
Description: PGP signature

Re: [PATCH v3 2/5] pretty: allow showing specific trailers

[Anders Waldenborg]

Junio C Hamano writes:
> Also, use of 'key=' automatically turns on 'only' as described, and
> I tend to agree that it would a convenient default mode (i.e. when
> picking certain trailers only with this mechanism, it is likely that
> the user is willing to use %(subject) etc. to fill in what was lost
> by the implicit use of 'only'), but at the same time, it makes me
> wonder if we need to have a way to countermand an 'only' (or
> 'unfold' for that matter) that was given earlier, e.g.
>
>   %(trailers:key=signed-off-by,only=no)
>
> Thanks.

I'm not sure what that would mean. The non-trailer lines in the trailer
block doesn't match the key.

Take this commit as an example:

$ git show -s --pretty=format:'%(trailers)' 
b4d065df03049bacfbc40467b60b13e804b7d289
Helped-by: Jeff King 
[jc: took idea and log message from peff]
Signed-off-by: Junio C Hamano 

With 'only' it shows:
$ git show -s --pretty=format:'%(trailers:only)' 
b4d065df03049bacfbc40467b60b13e804b7d289
Helped-by: Jeff King 
Signed-off-by: Junio C Hamano 

Now with a "key=signed-off-by" option I would imagine that as adding a
"| grep -i '^signed-off-by:'" to the end. In both cases (with and
without 'only') that would give the same result:
"Signed-off-by: Junio C Hamano "


 anders

Re: t5570 shaky for anyone ?

[SZEDER Gábor]

On Sun, Nov 25, 2018 at 09:52:23PM +0100, Torsten Bögershausen wrote:
> After running the  "Git 2.20-rc1" testsuite here on a raspi,
> the only TC that failed was t5570.
> When the "grep" was run on daemon.log, the file was empty (?).
> When inspecting it later, it was filled, and grep would have found
> the "extended.attribute" it was looking for.

I think I saw the same failure on Travis CI already before 2.19, so
it's not a new issue.  Here's the test's verbose output:

  + cat
  + 
  + GIT_OVERRIDE_VIRTUAL_HOST=localhost git -c protocol.version=1 ls-remote 
git://127.0.0.1:5570/interp.git
  b6752e52dd867264d12240028003f21e3e1dccabHEAD
  b6752e52dd867264d12240028003f21e3e1dccabrefs/heads/master
  + cut -d  -f2-
  + grep -i extended.attribute daemon.log
  + test_cmp expect actual
  + diff -u expect actual
  --- expect  2018-06-12 10:06:50.758357927 +
  +++ actual  2018-06-12 10:06:50.774365936 +
  @@ -1,2 +0,0 @@
  -Extended attribute "host": localhost
  -Extended attribute "protocol": version=1
  [10579] Connection from 127.0.0.1:45836
  [10579] Extended attribute "host": localhost
  [10579] Extended attribute "protocol": version=1
  error: last command exited with $?=1
  [10579] Request upload-pack for '/interp.git'
  [10579] Interpolated dir '/usr/src/git/t/trash
  dir.t5570/repo/localhost/interp.git'
  [10462] [10579] Disconnected
  not ok 21 - daemon log records all attributes

The thing is that 'git daemon's log is not written to 'daemon.log'
directly, but it goes through a fifo, which is read by a shell loop,
which then sends all log messages both to 'daemon.log' and to the test
script's standard error.  So there is certainly a race between log
messages going through the fifo and the loop before reaching
'daemon.log' and 'git ls-remote' exiting and 'grep' opening
'daemon.log'.

> The following fixes it, but I am not sure if this is the ideal
> solution.

Currently this is the only test that looks at 'daemon.log', but if we
ever going to add another one, then that will be prone to the same
issue.

I wonder whether it's really that useful to have the daemon log in the
test script's output...  if the log was sent directly to daemon log,
then the window for this race would be smaller, but still not
completely closed.

Anyway, I added Peff to Cc, since he added that whole fifo-shell-loop
thing.

> diff --git a/t/t5570-git-daemon.sh b/t/t5570-git-daemon.sh
> index 7466aad111..e259fee0ed 100755
> --- a/t/t5570-git-daemon.sh
> +++ b/t/t5570-git-daemon.sh
> @@ -192,6 +192,7 @@ test_expect_success 'daemon log records all attributes' '
>   GIT_OVERRIDE_VIRTUAL_HOST=localhost \
>   git -c protocol.version=1 \
>   ls-remote "$GIT_DAEMON_URL/interp.git" &&
> + sleep 1 &&
>   grep -i extended.attribute daemon.log | cut -d" " -f2- >actual &&
>   test_cmp expect actual
>  '
> 
> A slightly better approach may be to use a "sleep on demand":
> 
> + ( grep -i -q extended.attribute daemon.log || sleep 1 ) &&
> 

Re: [RFC] Introduce two new commands, switch-branch and restore-paths

[Thomas Gummerer]

On 11/20, Duy Nguyen wrote:
> On Mon, Nov 19, 2018 at 04:19:53PM +0100, Duy Nguyen wrote:
> > I promise to come back with something better (at least it still
> > sounds better in my mind). If that idea does not work out, we can
> > come back and see if we can improve this.
> 
> So this is it. The patch isn't pretty, mostly as a proof of
> concept. Just look at the three functions at the bottom of checkout.c,
> which is the main thing.
> 
> This patch tries to split "git checkout" command in two new ones:
> 
> - git switch-branch is all about switching branches
> - git restore-paths (maybe restore-file is better) for checking out
>   paths
> 
> The main idea is these two commands will co-exist with the good old
> 'git checkout', which will NOT be deprecated. Old timers will still
> use "git checkout". But new people should be introduced to the new two
> instead. And the new ones are just as capable as "git checkout".
> 
> Since the three commands will co-exist (with duplicate functionality),
> maintenance cost must be kept to minimum. The way I did this is simply
> split the command line options into three pieces: common,
> switch-branch and checkout-paths. "git checkout" has all three, the
> other two have common and another piece.
>
> With this, a new option added to git checkout will be automatically
> available in either switch-branch or checkout-paths. Bug fixes apply
> to all relevant commands.
> 
> Later on, we could start to add a bit more stuff in, e.g. some form of
> disambiguation is no longer needed when running as switch-branch, or
> restore-paths.
> 
> So, what do you think?

I like the idea of splitting those commands up, in fact it is
something I've been considering working on myself.  I do think we
should consider if we want to change the behaviour of those new
commands in any way compared to 'git checkout', since we're starting
with a clean slate.

One thing in particular that I have in mind is something I'm currently
working on, namely adding a --index flag to 'git checkout', which
would make 'git checkout' work in non-overlay mode (for more
discussion on that see also [*1*].  I got something working, that
needs to be polished a bit and am hoping to send that to the list
sometime soon.

I wonder if such the --index behaviour could be the default in
restore-paths command?

Most of the underlying machinery for 'checkout' could and should of
course still be shared between the commands.

*1*: 

> -- 8< --
> diff --git a/builtin.h b/builtin.h
> index 6538932e99..6e321ec8a4 100644
> --- a/builtin.h
> +++ b/builtin.h
> @@ -214,6 +214,7 @@ extern int cmd_remote_fd(int argc, const char **argv, 
> const char *prefix);
>  extern int cmd_repack(int argc, const char **argv, const char *prefix);
>  extern int cmd_rerere(int argc, const char **argv, const char *prefix);
>  extern int cmd_reset(int argc, const char **argv, const char *prefix);
> +extern int cmd_restore_paths(int argc, const char **argv, const char 
> *prefix);
>  extern int cmd_rev_list(int argc, const char **argv, const char *prefix);
>  extern int cmd_rev_parse(int argc, const char **argv, const char *prefix);
>  extern int cmd_revert(int argc, const char **argv, const char *prefix);
> @@ -227,6 +228,7 @@ extern int cmd_show_index(int argc, const char **argv, 
> const char *prefix);
>  extern int cmd_status(int argc, const char **argv, const char *prefix);
>  extern int cmd_stripspace(int argc, const char **argv, const char *prefix);
>  extern int cmd_submodule__helper(int argc, const char **argv, const char 
> *prefix);
> +extern int cmd_switch_branch(int argc, const char **argv, const char 
> *prefix);
>  extern int cmd_symbolic_ref(int argc, const char **argv, const char *prefix);
>  extern int cmd_tag(int argc, const char **argv, const char *prefix);
>  extern int cmd_tar_tree(int argc, const char **argv, const char *prefix);
> diff --git a/builtin/checkout.c b/builtin/checkout.c
> index acdafc6e4c..868ca3c223 100644
> --- a/builtin/checkout.c
> +++ b/builtin/checkout.c
> @@ -33,6 +33,16 @@ static const char * const checkout_usage[] = {
>   NULL,
>  };
>  
> +static const char * const switch_branch_usage[] = {
> + N_("git switch-branch [] "),
> + NULL,
> +};
> +
> +static const char * const restore_paths_usage[] = {
> + N_("git restore-paths [] [] -- ..."),
> + NULL,
> +};
> +
>  struct checkout_opts {
>   int patch_mode;
>   int quiet;
> @@ -44,6 +54,7 @@ struct checkout_opts {
>   int ignore_skipworktree;
>   int ignore_other_worktrees;
>   int show_progress;
> + int dwim_new_local_branch;
>   /*
>* If new checkout options are added, skip_merge_working_tree
>* should be updated accordingly.
> @@ -55,6 +66,7 @@ struct checkout_opts {
>   int new_branch_log;
>   enum branch_track track;
>   struct diff_options diff_options;
> + char *conflict_style;
>  
>   int branch_exists;
>   const char *prefix;
> @@ -1223,78 +1235,105 @@ static int 

Re: t5570 shaky for anyone ?

[Thomas Gummerer]

On 11/25, Torsten Bögershausen wrote:
> After running the  "Git 2.20-rc1" testsuite here on a raspi,
> the only TC that failed was t5570.
> When the "grep" was run on daemon.log, the file was empty (?).
> When inspecting it later, it was filled, and grep would have found
> the "extended.attribute" it was looking for.

I believe this has been reported before in
https://public-inbox.org/git/1522783990.964448.1325338528.0d49c...@webmail.messagingengine.com/,
but it seems like the thread never ended with actually fixing it.
Reading the first reply Peff seemed to be fine with just removing the
test completely, which would be the easiest solution ;)  Adding him to
Cc: here.  

> The following fixes it, but I am not sure if this is the ideal
> solution.
> 
> 
> diff --git a/t/t5570-git-daemon.sh b/t/t5570-git-daemon.sh
> index 7466aad111..e259fee0ed 100755
> --- a/t/t5570-git-daemon.sh
> +++ b/t/t5570-git-daemon.sh
> @@ -192,6 +192,7 @@ test_expect_success 'daemon log records all attributes' '
>   GIT_OVERRIDE_VIRTUAL_HOST=localhost \
>   git -c protocol.version=1 \
>   ls-remote "$GIT_DAEMON_URL/interp.git" &&
> + sleep 1 &&
>   grep -i extended.attribute daemon.log | cut -d" " -f2- >actual &&
>   test_cmp expect actual
>  '
> 
> A slightly better approach may be to use a "sleep on demand":
> 
> + ( grep -i -q extended.attribute daemon.log || sleep 1 ) &&
> 

Re: [PATCH v11 00/22] Convert "git stash" to C builtin

[Thomas Gummerer]

On 11/23, Paul-Sebastian Ungureanu wrote:
> Hello,
> 
> This is the 11th iteration of C git stash. Here are some of the changes,
> based on Thomas's and dscho's suggestions (from mailing list / pull request
> #495):

Thanks for your work on this!  I have read through the range-diff and
the new patch of this last round, and this addresses all the comments
I had on v10 (and some more :)).  I consider it
Reviewed-by: Thomas Gummerer 

> - improved memory management. Now, the callers of `do_create_stash()`
> are responsible of freeing the parameter they pass in. Moreover, the
> stash message is now a pointer to a buffer (in the previous iteration
> it was a pointer to a string). This should make it more clear who is
> responsible of freeing the memory.
> 
> - added `strbuf_insertf()` which inserts a format string at a given
> position in the buffer.
> 
> - some minor changes (changed "!oidcmp" to "oideq")
> 
> - fixed merge conflicts
> 
> Best regards,
> Paul
> 
> Joel Teichroeb (5):
>   stash: improve option parsing test coverage
>   stash: convert apply to builtin
>   stash: convert drop and clear to builtin
>   stash: convert branch to builtin
>   stash: convert pop to builtin
> 
> Paul-Sebastian Ungureanu (17):
>   sha1-name.c: add `get_oidf()` which acts like `get_oid()`
>   strbuf.c: add `strbuf_join_argv()`
>   strbuf.c: add `strbuf_insertf()` and `strbuf_vinsertf()`
>   t3903: modernize style
>   stash: rename test cases to be more descriptive
>   stash: add tests for `git stash show` config
>   stash: mention options in `show` synopsis
>   stash: convert list to builtin
>   stash: convert show to builtin
>   stash: convert store to builtin
>   stash: convert create to builtin
>   stash: convert push to builtin
>   stash: make push -q quiet
>   stash: convert save to builtin
>   stash: convert `stash--helper.c` into `stash.c`
>   stash: optimize `get_untracked_files()` and `check_changes()`
>   stash: replace all `write-tree` child processes with API calls
> 
>  Documentation/git-stash.txt  |4 +-
>  Makefile |2 +-
>  builtin.h|1 +
>  builtin/stash.c  | 1596 ++
>  cache.h  |1 +
>  git-stash.sh |  752 
>  git.c|1 +
>  sha1-name.c  |   19 +
>  strbuf.c |   51 ++
>  strbuf.h |   16 +
>  t/t3903-stash.sh |  192 ++--
>  t/t3907-stash-show-config.sh |   83 ++
>  12 files changed, 1897 insertions(+), 821 deletions(-)
>  create mode 100644 builtin/stash.c
>  delete mode 100755 git-stash.sh
>  create mode 100755 t/t3907-stash-show-config.sh
> 
> -- 
> 2.19.1.878.g0482332a22
> 

Re: [PATCH v11 03/22] strbuf.c: add `strbuf_insertf()` and `strbuf_vinsertf()`

[Thomas Gummerer]

On 11/23, Paul-Sebastian Ungureanu wrote:
> Implement `strbuf_insertf()` and `strbuf_vinsertf()` to
> insert data using a printf format string.
> 
> Original-idea-by: Johannes Schindelin 
> Signed-off-by: Paul-Sebastian Ungureanu 
> ---
>  strbuf.c | 36 
>  strbuf.h |  9 +
>  2 files changed, 45 insertions(+)
> 
> diff --git a/strbuf.c b/strbuf.c
> index 82e90f1dfe..bfbbdadbf3 100644
> --- a/strbuf.c
> +++ b/strbuf.c
> @@ -249,6 +249,42 @@ void strbuf_insert(struct strbuf *sb, size_t pos, const 
> void *data, size_t len)
>   strbuf_splice(sb, pos, 0, data, len);
>  }
>  
> +void strbuf_vinsertf(struct strbuf *sb, size_t pos, const char *fmt, va_list 
> ap)
> +{
> + int len, len2;
> + char save;
> + va_list cp;
> +
> + if (pos > sb->len)
> + die("`pos' is too far after the end of the buffer");

I was going to ask about translation of this and other messages in
'die()' calls, but I see other messages in strbuf.c are not marked for
translation either.  It may make sense to mark them all for
translation at some point in the future, but having them all
untranslated for now makes sense.

In the long run it may even be better to return an error here rather
than 'die()'ing, but again this is consistent with the rest of the
API, so this wouldn't be a good time to take that on.

> + va_copy(cp, ap);
> + len = vsnprintf(sb->buf + sb->len, 0, fmt, cp);

Here we're just getting the length of what we're trying to format
(excluding the final NUL).  As the second argument is 0, we do not
modify the strbuf at this point...

> + va_end(cp);
> + if (len < 0)
> + BUG("your vsnprintf is broken (returned %d)", len);
> + if (!len)
> + return; /* nothing to do */
> + if (unsigned_add_overflows(sb->len, len))
> + die("you want to use way too much memory");
> + strbuf_grow(sb, len);

... and then we grow the strbuf by the length we previously, which
excludes the NUL character, plus one extra character, so even if pos
== len we are sure to have enough space in the strbuf ...

> + memmove(sb->buf + pos + len, sb->buf + pos, sb->len - pos);
> + /* vsnprintf() will append a NUL, overwriting one of our characters */
> + save = sb->buf[pos + len];
> + len2 = vsnprintf(sb->buf + pos, sb->alloc - sb->len, fmt, ap);

... and we use vsnprintf to write the formatted string to the
beginning of the buffer.  sb->alloc - sb->len can be larger than
'len', which is fine as vsnprintf doesn't write anything after the NUL
character.  And as 'strbuf_grow' adds len + 1 bytes to the strbuf
we'll always have enough space for adding the formatted string ...

> + sb->buf[pos + len] = save;
> + if (len2 != len)
> + BUG("your vsnprintf is broken (returns inconsistent lengths)");
> + strbuf_setlen(sb, sb->len + len);

And finally we set the strbuf to the new length.  So all this is just
a very roundabout way to say that this function does the right thing
according to my reading (and tests).

> +}
> +
> +void strbuf_insertf(struct strbuf *sb, size_t pos, const char *fmt, ...)
> +{
> + va_list ap;
> + va_start(ap, fmt);
> + strbuf_vinsertf(sb, pos, fmt, ap);
> + va_end(ap);
> +}
> +
>  void strbuf_remove(struct strbuf *sb, size_t pos, size_t len)
>  {
>   strbuf_splice(sb, pos, len, "", 0);
> diff --git a/strbuf.h b/strbuf.h
> index be02150df3..8f8fe01e68 100644
> --- a/strbuf.h
> +++ b/strbuf.h
> @@ -244,6 +244,15 @@ void strbuf_addchars(struct strbuf *sb, int c, size_t n);
>   */
>  void strbuf_insert(struct strbuf *sb, size_t pos, const void *, size_t);
>  
> +/**
> + * Insert data to the given position of the buffer giving a printf format
> + * string. The contents will be shifted, not overwritten.
> + */
> +void strbuf_vinsertf(struct strbuf *sb, size_t pos, const char *fmt,
> +  va_list ap);
> +
> +void strbuf_insertf(struct strbuf *sb, size_t pos, const char *fmt, ...);
> +
>  /**
>   * Remove given amount of data from a given position of the buffer.
>   */
> -- 
> 2.19.1.878.g0482332a22
> 

Re: BUG: CR marker ^M doesn't show up in '-' lines of diffs when the ending of the removed line is CR+LF

[Johannes Sixt]

Am 25.11.18 um 15:03 schrieb Frank Schäfer:

Am 24.11.18 um 23:07 schrieb Johannes Sixt:

I don't think that there is anything to fix. If you have a file with
CRLF in it, but you did not declare to Git that CRLF is the expected
end-of-line indicator, then the CR *is* trailing whitespace (because
the line ends at LF), and 'git diff' highlights it.


Sure, it's correct to highlight it.
But it doesn't highlight it in removed lines, just in added lines.
I can see no good reason why removed and added lines should be treated
differently.


But incorrect whitespace is never highlighted in removed lines, why 
should CR be an exception?



1) If CR+LF line termination is used in a file, changing the content of
a line (but not its termination) currently produces a diff like

-something
+something_new^M

which causes the user to think he has changed the line ending (added a
CR) although he didn't.


But this is not limited to CR at EOL:

-something
+something_new

will also show the incorrect whitespace highlighted only for the + line.


2) If someone/something unintentionally changes the line termination
from CR+LF to LF, it doesn't show up in the diff:

-something
+something


Same here for other cases, for example

-something
+something

will not have on obvious indicator that whitespace was corrected.

If you are worried about a change in EOL style, you should better listen 
to your other tools. Either it is important, or it is not. If it is, 
they will report it to you. If it isn't, why care?


-- Hannes

Re: What's cooking in git.git (Sep 2018, #01; Tue, 4)

[Ævar Arnfjörð Bjarmason]

On Sun, Nov 25 2018, Torsten Bögershausen wrote:

> On Sun, Nov 25, 2018 at 05:28:35AM +0100, Torsten Bögershausen wrote:
>> On Sat, Nov 24, 2018 at 08:33:37PM +0100, Ævar Arnfjörð Bjarmason wrote:
>> >
>> > On Wed, Sep 05 2018, Ævar Arnfjörð Bjarmason wrote:
>> >
>> > > On Wed, Sep 05 2018, Eric Sunshine wrote:
>>
>> []
>>
>> > > SunCC used to be ahead of GCC & Clang when it came to certain classes of
>> > > warnings, but e.g. now everything it complains about is because it
>> > > doesn't understand C as well, e.g. we have quite a few compile warnings
>> > > due to code like this, which it claims is unreachable (but isn't):
>> > > https://github.com/git/git/blob/v2.19.0-rc2/read-cache.c#L950-L955
>> >
>>
>> Wait a second - even if the compiler claims something (wrong)...
>> there a still 1+1/2 questions from my side:
>>
>>
>> int verify_path(const char *path, unsigned mode)
>> {
>>  char c;
>>   ^
>>  /* Q1: should  "c" be initialized like this: */
>>  char c = *path;
>>
>>  if (has_dos_drive_prefix(path))
>>  return 0;
>>
>>  goto inside;
>>   /* Q2: and why do we need the "goto" here ? */
>>  for (;;) {
>>  if (!c)
>>  return 1;
>>  if (is_dir_sep(c)) {
>> inside:
>
> After some re-reading,
> I think that the "goto inside" was just hard to read
>
> Out of interest:
> would the following make the compiler happy ?
>
>
> diff --git a/read-cache.c b/read-cache.c
> index 49add63fe1..d574d58b9d 100644
> --- a/read-cache.c
> +++ b/read-cache.c
> @@ -951,17 +951,15 @@ static int verify_dotfile(const char *rest, unsigned 
> mode)
>
>  int verify_path(const char *path, unsigned mode)
>  {
> - char c;
> + char c = *path ? '/' : '\0';
>
>   if (has_dos_drive_prefix(path))
>   return 0;
>
> - goto inside;
>   for (;;) {
>   if (!c)
>   return 1;
>   if (is_dir_sep(c)) {
> -inside:
>   if (protect_hfs) {
>   if (is_hfs_dotgit(path))
>   return 0;

I haven't tested (it's tedious) but yes, I can tell you SunCC won't
whine about this. I've only seen its unreachability detector get
confused about goto inside a loop, not the the sort of code you've
replaced this with.

We should not be appeasing these old compiler warnings in cases where
they're wrong. You can check out the CI output I linked to to see 10-20
cases in the codebase where SunCC is wrong about unreliability.

Whether we should just fix this for its own sake is another question.

Re: BUG: CR marker ^M doesn't show up in '-' lines of diffs when the ending of the removed line is CR+LF

[Frank Schäfer]

Am 24.11.18 um 23:07 schrieb Johannes Sixt:
> I don't think that there is anything to fix. If you have a file with
> CRLF in it, but you did not declare to Git that CRLF is the expected
> end-of-line indicator, then the CR *is* trailing whitespace (because
> the line ends at LF), and 'git diff' highlights it. 

Sure, it's correct to highlight it.
But it doesn't highlight it in removed lines, just in added lines.
I can see no good reason why removed and added lines should be treated
differently.

Let me give you two real-life examples:
 
1) If CR+LF line termination is used in a file, changing the content of
a line (but not its termination) currently produces a diff like

-something
+something_new^M

which causes the user to think he has changed the line ending (added a
CR) although he didn't.

2) If someone/something unintentionally changes the line termination
from CR+LF to LF, it doesn't show up in the diff:

-something
+something

I don't think this behavior makes sense.
At least it's IMHO not what users expect to see.
They want to see what's really going on, not to get confused.

Regards,
Frank

Re: [PATCH] files-backend.c: fix build error on Solaris

[Duy Nguyen]

On Sun, Nov 25, 2018 at 11:19 AM Carlo Arenas  wrote:
>
> Signed-off-by: Carlo Marcelo Arenas Belón 
>
> clang with -Wpedantic also catch this (at least with Apple LLVM
> version 10.0.0); recent versions of gcc also include that flag and at
> least 8.2.0 shows a warning for it, so it might be worth adding it to
> developer mode (maybe under the pedantic DEVOPTS), would this be
> something I should be adding?, what are the expectations around
> warnings and compilers?

make DEVELOPER=1 DEVOPTS=pedantic (with either gcc 7.3.0 or 8.2.0) can
already catch this. I have no comment if this pedantic should be part
of default DEVELOPER=1. But at least in controlled environments like
Travis, we could turn it on to catch more.
-- 
Duy

Re: [PATCH] files-backend.c: fix build error on Solaris

[Carlo Arenas]

Signed-off-by: Carlo Marcelo Arenas Belón 

clang with -Wpedantic also catch this (at least with Apple LLVM
version 10.0.0); recent versions of gcc also include that flag and at
least 8.2.0 shows a warning for it, so it might be worth adding it to
developer mode (maybe under the pedantic DEVOPTS), would this be
something I should be adding?, what are the expectations around
warnings and compilers?

Carlo

Re: What's cooking in git.git (Sep 2018, #01; Tue, 4)

[Torsten Bögershausen]

On Sun, Nov 25, 2018 at 05:28:35AM +0100, Torsten Bögershausen wrote:
> On Sat, Nov 24, 2018 at 08:33:37PM +0100, Ævar Arnfjörð Bjarmason wrote:
> > 
> > On Wed, Sep 05 2018, Ævar Arnfjörð Bjarmason wrote:
> > 
> > > On Wed, Sep 05 2018, Eric Sunshine wrote:
> 
> []
> 
> > > SunCC used to be ahead of GCC & Clang when it came to certain classes of
> > > warnings, but e.g. now everything it complains about is because it
> > > doesn't understand C as well, e.g. we have quite a few compile warnings
> > > due to code like this, which it claims is unreachable (but isn't):
> > > https://github.com/git/git/blob/v2.19.0-rc2/read-cache.c#L950-L955
> > 
> 
> Wait a second - even if the compiler claims something (wrong)...
> there a still 1+1/2 questions from my side:
> 
> 
> int verify_path(const char *path, unsigned mode)
> {
>   char c;
>^
>   /* Q1: should  "c" be initialized like this: */
>   char c = *path;
> 
>   if (has_dos_drive_prefix(path))
>   return 0;
> 
>   goto inside;
>    /* Q2: and why do we need the "goto" here ? */
>   for (;;) {
>   if (!c)
>   return 1;
>   if (is_dir_sep(c)) {
> inside:

After some re-reading,
I think that the "goto inside" was just hard to read

Out of interest:
would the following make the compiler happy ?


diff --git a/read-cache.c b/read-cache.c
index 49add63fe1..d574d58b9d 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -951,17 +951,15 @@ static int verify_dotfile(const char *rest, unsigned mode)
 
 int verify_path(const char *path, unsigned mode)
 {
-   char c;
+   char c = *path ? '/' : '\0';
 
if (has_dos_drive_prefix(path))
return 0;
 
-   goto inside;
for (;;) {
if (!c)
return 1;
if (is_dir_sep(c)) {
-inside:
if (protect_hfs) {
if (is_hfs_dotgit(path))
return 0;

Re: What's cooking in git.git (Sep 2018, #01; Tue, 4)

[Torsten Bögershausen]

On Sat, Nov 24, 2018 at 08:33:37PM +0100, Ævar Arnfjörð Bjarmason wrote:
> 
> On Wed, Sep 05 2018, Ævar Arnfjörð Bjarmason wrote:
> 
> > On Wed, Sep 05 2018, Eric Sunshine wrote:

[]

> > SunCC used to be ahead of GCC & Clang when it came to certain classes of
> > warnings, but e.g. now everything it complains about is because it
> > doesn't understand C as well, e.g. we have quite a few compile warnings
> > due to code like this, which it claims is unreachable (but isn't):
> > https://github.com/git/git/blob/v2.19.0-rc2/read-cache.c#L950-L955
> 

Wait a second - even if the compiler claims something (wrong)...
there a still 1+1/2 questions from my side:


int verify_path(const char *path, unsigned mode)
{
char c;
 ^
/* Q1: should  "c" be initialized like this: */
char c = *path;

if (has_dos_drive_prefix(path))
return 0;

goto inside;
 /* Q2: and why do we need the "goto" here ? */
for (;;) {
if (!c)
return 1;
if (is_dir_sep(c)) {
inside:

Re: git overwriting local ignored files?

[David Mandelberg]

On 11/24/18 10:41 AM, Konstantin Khomoutov wrote:

On Sat, Nov 24, 2018 at 05:57:24PM +0300, Konstantin Khomoutov wrote:

On Sat, Nov 24, 2018 at 09:37:06AM -0500, David Mandelberg wrote:


It seems that git is overwriting my local files on merge if they're in
.gitignore.

[...]

The .gitignore file is to list "ignored and expendable" class of
files; there is no "ignored but precious class" in Git.

Ok. Would a patch be welcome? I have three ideas for how to implement it,
and I'm not sure which is better.

[...]

You might want to first investigate this recent thread [1] which AFAIK
was dedicated to exactly this problem.


Well, actually the thread is old, but its continuation [2] is recent.
The crux is that it discusses certain approaches to solve the apparent
problem and patches to do that.

1. https://public-inbox.org/git/4c6a1c5b.4030...@workspacewhiz.com/
2. https://public-inbox.org/git/871s8qdzph@evledraar.gmail.com/


Thanks for the pointers, and sorry to start a new thread. It looks like 
most of the work to do is in finding consensus on the right way forward, 
rather than in writing a patch. While I really would like there be a way 
to prevent git from overwriting ignored files, I don't have any strong 
opinions on what that way should look like. So I think I'll just wait 
and hope somebody else does the work.


--
https://david.mandelberg.org/

Re: What's cooking in git.git (Nov 2018, #06; Wed, 21)

[Junio C Hamano]

Jeff King  writes:

> I do also think in the long run we should be fixing the "unreachable
> always become loose" issues.

I think I've seen an idea of collecting them into a garbage pack
floated for at least a few times here.  What are the downsides?  We
no longer will know when these unreachable ones were last accessed
individually so we need to come up with a different policy around
their expiration?  As the common traits among objects in such a
garbage pack (iow the way we discover the objects that need to be
placed in there) does not involve path information and we lose the
ability to compress them well?

Re: [PATCH v3 3/8] refs: new ref types to make per-worktree refs visible to all worktrees

[Junio C Hamano]

Ævar Arnfjörð Bjarmason  writes:

> On Sun, Oct 21 2018, Nguyễn Thái Ngọc Duy wrote:
>
> This change has a regression in 2.20:
>
>> [...]
>>  static void files_reflog_path(struct files_ref_store *refs,
>>struct strbuf *sb,
>>const char *refname)
>> @@ -158,6 +178,9 @@ static void files_reflog_path(struct files_ref_store 
>> *refs,
>>  case REF_TYPE_PSEUDOREF:
>>  strbuf_addf(sb, "%s/logs/%s", refs->gitdir, refname);
>>  break;
>> +case REF_TYPE_OTHER_PSEUDOREF:
>> +case REF_TYPE_MAIN_PSEUDOREF:
>> +return files_reflog_path_other_worktrees(refs, sb, refname);
>>  case REF_TYPE_NORMAL:
>>  strbuf_addf(sb, "%s/logs/%s", refs->gitcommondir, refname);
>>  break;
>
> SunCC on Solaris hard errors on this:
>
> "refs/files-backend.c", line 183: void function cannot return value
>
> Needs to be files...(); break; instead.

True.

The caller itself returns "void", so it would be nice if this were a
mere warning() from practical usabliity's point of view, though ;-)

Re: How to efficiently backup a bare repository?

[Junio C Hamano]

Ævar Arnfjörð Bjarmason  writes:

> There's no easy out of the box way to do exactly what you've
> described. A few things come to mind:
> ...

Wouldn't it suffice to have a cron job that runs something like

D=$(date +"%Y-%m-%d")
git fetch $serving "refs/*:refs/backup-$D/*"

on the back-up box to fetch from the repository on the box the
end-users push into once a day?  In the back-up repository, the
refs/backup-2018-11-25/heads/master reference would be today's tip
of the master branch of the serving repository.  You can set the
expiry timeout to "now" (i.e. "gc" will immediately drop unreachable
objects, and that is fine because you expicitly have refs to pin
these objects anyway), get the dedup from "git fetch" for free,
repack the backup repository as a whole, and dropping the whole
refs/backup-2018-10-25/* hierarcy on 2018-11-25 is all you need to
expire the refs.

You may want to play with the ref-advertisement limiting options in
the recent Git, if it is too much to grow the amount of "have"s by
30x for the common ancestry negotiation.  But that is a small
implementation detail.

Re: [ANNOUNCE] Git v2.20.0-rc1

[Junio C Hamano]

Ævar Arnfjörð Bjarmason  writes:

>>  * "git rebase" and "git rebase -i" have been reimplemented in C.
>
> Here's another regression in the C version (and rc1),...
> I wasn't trying to stress test rebase. I was just wanting to rebase a
> history I was about to force-push after cleaning it up, hardly an
> obscure use-case. So [repeat last transmission in
> https://public-inbox.org/git/87y39w1wc2@evledraar.gmail.com/ ]

which, to those who are reading from sidelines:

Given that we're still finding regressions bugs in the rebase-in-C
version should we be considering reverting 5541bd5b8f ("rebase: default
to using the builtin rebase", 2018-08-08)?

I love the feature, but fear that the current list of known regressions
serve as a canary for a larger list which we'd discover if we held off
for another major release (and would re-enable rebase.useBuiltin=true in
master right after 2.20 is out the door).

I am fine with the proposed flip, but I'll have to see the extent of
damage this late in the game so that I won't miss anything.  In
addition to the one-liner below, we'd need to update the quoted
release notes entry, and possibly adjust some tests (even though the
"reimplementation" ought to be bug-to-bug compatible, it may not be).

diff --git b/builtin/rebase.c a/builtin/rebase.c
index 9dc8475cd3..60e357c735 100644
--- b/builtin/rebase.c
+++ a/builtin/rebase.c
@@ -54,7 +54,7 @@ static int use_builtin_rebase(void)
cp.git_cmd = 1;
if (capture_command(, , 6)) {
strbuf_release();
-   return 1;
+   return 0;
}
 
strbuf_trim();

Re: How to efficiently backup a bare repository?

[Ævar Arnfjörð Bjarmason]

On Fri, Nov 23 2018, Guilhem Bonnefille wrote:

> I'm managing many bare repositories for development teams.
>
> One service we want to offer is to let developers retrieve old state
> of the repository up to 30 days. For example, one developer
> (accidently) removed (push -f) a branch/tag and realize few days later
> (after vacations) that it was an error.
>
> What is the best approach to do this?
>
> Currently, we use a classical approach, backuping all the repo every
> day. But this is far from efficient as:
> - we accumulate 30th copies of the repository
> - due to packing logic of Git, even if the content is mostly similar,
> from one backup to another, there is no way to deduplicate.
>
> Is there any tricks based on reflog? Even for deleted refs (branch/tags)?
> Is there any tooling playing with the internal of git to offer such
> feature, like copying all refs in a timestamped refs directory to
> retain objects?
>
> Thanks in advance for any tips letting improve the backup.

There's no easy out of the box way to do exactly what you've
described. A few things come to mind:

a) If you can simply deny non-fast-forwards that's ideal. E.g. for some
   branches you care about, or tags. This is how most of us deal with
   this issue in practice. I.e. have some "blessed" refs that matter,
   and if someone rewinds their own topic branch that's their own
   problem.

b) You could as you touched upon have a post-receive hook that detects
   non-fast-forwards, and e.g. pushes a clobberd "master" or "v1.0" to
   some backup repo's 2018-11-24-23-39-04-master or whatever. Then users
   could grab old versions of refs from that repo. I do a similar thing
   at work to archive certain refs (old tags), but without renaming
   them.

   The advantage is that you get all refs ever, the disadvantage is that
   you're not going to get a copy of the repo as it was N days ago,
   it'll need to be manually pieced together.

c) Git could be made block-level de-duplication friendly. I was planning
   to work on it, but it's a small enough itch that I didn't care, but
   initial results look promising:
   https://public-inbox.org/git/20180125002942.ga21...@sigill.intra.peff.net/

d) Note that if you're e.g. rsyncing repos that are actively being
   pushed into you're likely to sometimes end up with corrupt repos
   unless you're very careful about what you grab and in what
   order. Best to backup repos with "git fetch".

e) If you're burned by one-off cases like this dev going away for 30
   days you could bump the default expiry that comes with git from 2
   weeks to e.g. 6 weeks. It's still a manual process to recover data
   (with fsck etc), but at least it's there.

Re: BUG: CR marker ^M doesn't show up in '-' lines of diffs when the ending of the removed line is CR+LF

[Johannes Sixt]

Am 24.11.18 um 15:51 schrieb Frank Schäfer:

Am 23.11.18 um 22:47 schrieb Johannes Sixt:

Am 23.11.18 um 19:19 schrieb Frank Schäfer:

The CR marker ^M doesn't show up in '-' lines of diffs when the ending
of the removed line is CR+LF.
It shows up as expected in '-' lines when the ending of the removed line
is CR only.
It also always shows up as expected in '+' lines.


Is your repository configured to (1) highlight whitespace errors in
diff output and (2) to leave CRLF alone in text files?

I'm using the default configuration, so whitespace is set to
trailing-space, but cr-at-eol is not set.



If so, then it is just a side-effect of this combination, an illusion,
so to say: The CR in the CRLF combo is trailing whitespace. The 'git
diff' marks it by inserting an escape sequence to switch the color
before ^M and another escape sequence to reset to color after ^M. This
breaks the CRLF combination apart, so that the pager does not process
it as a combined CRLF sequence; it displays the lone CR as ^M.

Urghh... so that needs to be fixed.
Why does it work correctly with '+' lines ?


I don't think that there is anything to fix. If you have a file with 
CRLF in it, but you did not declare to Git that CRLF is the expected 
end-of-line indicator, then the CR *is* trailing whitespace (because the 
line ends at LF), and 'git diff' highlights it.


-- Hannes

Re: [ANNOUNCE] Git v2.20.0-rc1

[Ævar Arnfjörð Bjarmason]

On Wed, Nov 21 2018, Junio C Hamano wrote:

>  * "git rebase" and "git rebase -i" have been reimplemented in C.

Here's another regression in the C version (and rc1), note: the
sha1collisiondetection is just a stand in for "some repo":

(
rm -rf /tmp/repo &&
git init /tmp/repo &&
cd /tmp/repo &&
for c in 1 2
do
touch $c &&
git add $c &&
git commit -m"add $c"
done &&
git remote add origin 
https://github.com/cr-marcstevens/sha1collisiondetection.git &&
git fetch &&
git branch --set-upstream-to origin/master &&
git rebase -i
)

The C version will die with "fatal: unable to read tree
". Running this with
rebase.useBuiltin=false does the right thing and rebases as of the merge
base of the two (which here is the root of the history).

I wasn't trying to stress test rebase. I was just wanting to rebase a
history I was about to force-push after cleaning it up, hardly an
obscure use-case. So [repeat last transmission in
https://public-inbox.org/git/87y39w1wc2@evledraar.gmail.com/ ]

Re: What's cooking in git.git (Sep 2018, #01; Tue, 4)

[Ævar Arnfjörð Bjarmason]

nlikely that we'll see someone develop new tests on
>> Solaris, so having 'chainlint' disabled there isn't likely to be a big
>> deal. Moreover, if someone does write a new test on Solaris which has
>> a broken &&-chain in a subshell, that breakage will be caught very
>> quickly once the test is run by anyone on Linux or MacOS (or Windows
>> or BSD or AIX), so it's not like the broken test will make it into the
>> project undetected.
>
> Since writing my initial message I see that the CSW package packaging
> git on Solaris just uses GNU userland:
> https://sourceforge.net/p/gar/code/HEAD/tree/csw/mgar/pkg/git/trunk/Makefile
>
> I.e. it puts /opt/csw/gnu in its $PATH, so this whole thing is probably
> a non-issue. I.e. if the near-as-you-can-get "official" package just
> compiles git against GNU tooling, perhaps we should stop caring about
> Solaris-native tooling.
>
> That does also mean that something like my patch above isn't OK, since
> we shouldn't be disabling GIT_TEST_CHAIN_LINT based on the uname, if it
> then turns out that GNU tooling is being used.
>
> So I think for the purposes of v2.19.0-rc2..v2.19.0 it's fine to just
> leave this as it is in master right now.
>
> It's somewhat of a shame that we're drifting to not supporing some of
> these more obscure POSIX systems "natively" though, but maybe that's the
> right move at this point, and maybe it isn't.
>
> When I was porting C code to Solaris ~10 years ago I'd often get SunCC
> turning up some interesting issues that were *real* issues, same with
> compiling with IBM xlc on AIX.
>
> Now when I'm re-visiting these systems much later I've yet to turn up
> some issue like that, just boring compatibility issues with their old
> tooling. E.g. one outstanding issue is that some test of ours fail on
> AIX because we use "readlink", but that command isn't in POSIX (only the
> C function is), so AIX doesn't implement it.
>
> SunCC used to be ahead of GCC & Clang when it came to certain classes of
> warnings, but e.g. now everything it complains about is because it
> doesn't understand C as well, e.g. we have quite a few compile warnings
> due to code like this, which it claims is unreachable (but isn't):
> https://github.com/git/git/blob/v2.19.0-rc2/read-cache.c#L950-L955

Correction. It is still useful for something:
https://public-inbox.org/git/87a7ly1djb@evledraar.gmail.com/

Re: [PATCH v3 3/8] refs: new ref types to make per-worktree refs visible to all worktrees

[Ævar Arnfjörð Bjarmason]

On Sun, Oct 21 2018, Nguyễn Thái Ngọc Duy wrote:

This change has a regression in 2.20:

> [...]
>  static void files_reflog_path(struct files_ref_store *refs,
> struct strbuf *sb,
> const char *refname)
> @@ -158,6 +178,9 @@ static void files_reflog_path(struct files_ref_store 
> *refs,
>   case REF_TYPE_PSEUDOREF:
>   strbuf_addf(sb, "%s/logs/%s", refs->gitdir, refname);
>   break;
> + case REF_TYPE_OTHER_PSEUDOREF:
> + case REF_TYPE_MAIN_PSEUDOREF:
> + return files_reflog_path_other_worktrees(refs, sb, refname);
>   case REF_TYPE_NORMAL:
>   strbuf_addf(sb, "%s/logs/%s", refs->gitcommondir, refname);
>   break;

SunCC on Solaris hard errors on this:

"refs/files-backend.c", line 183: void function cannot return value

Needs to be files...(); break; instead.

Re: git overwriting local ignored files?

[Konstantin Khomoutov]

On Sat, Nov 24, 2018 at 05:57:24PM +0300, Konstantin Khomoutov wrote:
> On Sat, Nov 24, 2018 at 09:37:06AM -0500, David Mandelberg wrote:
> 
> > > > It seems that git is overwriting my local files on merge if they're in
> > > > .gitignore.
> [...]
> > > The .gitignore file is to list "ignored and expendable" class of
> > > files; there is no "ignored but precious class" in Git.
> > Ok. Would a patch be welcome? I have three ideas for how to implement it,
> > and I'm not sure which is better.
> [...]
> 
> You might want to first investigate this recent thread [1] which AFAIK
> was dedicated to exactly this problem.

Well, actually the thread is old, but its continuation [2] is recent.
The crux is that it discusses certain approaches to solve the apparent
problem and patches to do that.

1. https://public-inbox.org/git/4c6a1c5b.4030...@workspacewhiz.com/
2. https://public-inbox.org/git/871s8qdzph@evledraar.gmail.com/

Re: BUG: CR marker ^M doesn't show up in '-' lines of diffs when the ending of the removed line is CR+LF

[Torsten Bögershausen]

On Sat, Nov 24, 2018 at 03:51:26PM +0100, Frank Schäfer wrote:
[]
> 
> Hmm... is CR-only line termination supported at all ?
> E.g. 'eol' can be set to 'lf' or 'crlf' but not 'cr'...
> 

No, CR-only is not supported, because:
Nobody was implementing it, and that is probably because
the only question abou CR-only (at least what I remember)
was a when an old Mac OS (not the Mac OS X)
was used (which used to use CR instead of LF).

And, such feature may be implemented by writing a filter,
replace CR with LF as "clean" and "LF" with "CR" for smudge.

Re: git overwriting local ignored files?

[Konstantin Khomoutov]

On Sat, Nov 24, 2018 at 09:37:06AM -0500, David Mandelberg wrote:

> > > It seems that git is overwriting my local files on merge if they're in
> > > .gitignore.
[...]
> > The .gitignore file is to list "ignored and expendable" class of
> > files; there is no "ignored but precious class" in Git.
> Ok. Would a patch be welcome? I have three ideas for how to implement it,
> and I'm not sure which is better.
[...]

You might want to first investigate this recent thread [1] which AFAIK
was dedicated to exactly this problem.

1. https://public-inbox.org/git/4c6a1c5b.4030...@workspacewhiz.com/

Re: BUG: CR marker ^M doesn't show up in '-' lines of diffs when the ending of the removed line is CR+LF

[Frank Schäfer]

Am 23.11.18 um 22:47 schrieb Johannes Sixt:
> Am 23.11.18 um 19:19 schrieb Frank Schäfer:
>> The CR marker ^M doesn't show up in '-' lines of diffs when the ending
>> of the removed line is CR+LF.
>> It shows up as expected in '-' lines when the ending of the removed line
>> is CR only.
>> It also always shows up as expected in '+' lines.
>
> Is your repository configured to (1) highlight whitespace errors in
> diff output and (2) to leave CRLF alone in text files?
I'm using the default configuration, so whitespace is set to
trailing-space, but cr-at-eol is not set.

>
> If so, then it is just a side-effect of this combination, an illusion,
> so to say: The CR in the CRLF combo is trailing whitespace. The 'git
> diff' marks it by inserting an escape sequence to switch the color
> before ^M and another escape sequence to reset to color after ^M. This
> breaks the CRLF combination apart, so that the pager does not process
> it as a combined CRLF sequence; it displays the lone CR as ^M.
Urghh... so that needs to be fixed.
Why does it work correctly with '+' lines ?

>
> It is easy to achieve the opposite effect, i.e., that ^M is not
> displayed. For example with these lines in .git/info/attributes or
> .gitattributes:
>
> *.cpp
> whitespace=trailing-space,cr-at-eol,indent-with-non-tab,space-before-tab
> *.h
> whitespace=trailing-space,cr-at-eol,indent-with-non-tab,space-before-tab
>
> Note the cr-at-eol. (There may be shorter versions to achieve a
> similar effect.)
With cr-at-eol, ^M indeed doesn't show up anymore in '+' lines with
CR+LF line endings. That's correct/expected.
'-' lines with CR+LF line endings are displayed correctly in this case, too.
However, ^M still shows up in '+' and '-' lines with CR line endings.

Hmm... is CR-only line termination supported at all ?
E.g. 'eol' can be set to 'lf' or 'crlf' but not 'cr'...

Regards,
Frank

>
> -- Hannes

Re: git overwriting local ignored files?

[David Mandelberg]

On 11/23/18 11:22 PM, Junio C Hamano wrote:

David Mandelberg  writes:


It seems that git is overwriting my local files on merge if they're in
.gitignore. See command transcript below. I searched `git help config`
and Google, but I couldn't find any way to prevent it. Am I missing
something? (The reason I care about ignored files is that I'm using
git with a working directory of $HOME to manage my dotfiles, and most
files in my $HOME are not tracked by git but are still important.)


The .gitignore file is to list "ignored and expendable" class of
files; there is no "ignored but precious class" in Git.


Ok. Would a patch be welcome? I have three ideas for how to implement 
it, and I'm not sure which is better.


1. Add a boolean config option called core.preserveIgnore (or similar). 
I'm guessing this is the least amount of work, but it's not very 
flexible. I'm also not sure how it should work with `git clean`.


2. Add some syntax to the .gitignore format so the same file can list 
both "ignored and expendable" and "ignored and precious". I could see 
using this to mark compiled files as ignored+expendable and per-repo 
editor/IDE config files as ignored+precious. Do you have any idea how 
big of a patch this would be? Or any idea for the new syntax? I think 
starting a line with "\x" where x is any character not currently escaped 
by a backslash would cause the least disruption to existing .gitignore 
files.


3. Like (2), but use a separate file instead of .gitignore (and the 
other git-ignore files).


Any thoughts?

--
https://david.mandelberg.org/

Re: [PATCH] t5562: do not reuse output files

[Max Kirillov]

On Sat, Nov 24, 2018 at 04:47:19PM +0900, Junio C Hamano wrote:
> I do agree that forcing the parent to wait, like you described in
> the comment, would be far more preferrable,

It looks like it can be done as simple as:

--- a/http-backend.c
+++ b/http-backend.c
@@ -486,6 +486,8 @@ static void run_service(const char **argv, int buffer_input)
if (buffer_input || gzipped_request || req_len >= 0)
cld.in = -1;
cld.git_cmd = 1;
+   cld.clean_on_exit = 1;
+   cld.wait_after_clean = 1;
if (start_command())
exit(1);

at least according to strate it does what it should.

Re: [PATCH] t5562: do not reuse output files

[Jeff King]

On Sat, Nov 24, 2018 at 04:47:19PM +0900, Junio C Hamano wrote:

> I do agree that forcing the parent to wait, like you described in
> the comment, would be far more preferrable, [...]

Stray processes can sometimes have funny effects on an outer test
harness, too. E.g., I think I've seen hangs running t5562 under prove,
because some process is holding open a pipe descriptor. This would
probably fix that, too.

> but until that happens,[...]

But if we can't do that immediately for some reason, I do agree with
everything else you said here. ;)

-Peff

Re: [PATCH] t5562: fix perl path

[Jeff King]

On Fri, Nov 23, 2018 at 01:38:21AM +0200, Max Kirillov wrote:

> From: Jeff King 
> 
> Some systems do not have perl installed to /usr/bin. Use the variable
> from the build settiings, and call perl directly than via shebang.
> 
> Signed-off-by: Max Kirillov 
> ---
> Submitting. Could you sign-off? Also removed shebang from the script as it is 
> not needed

Yep:

  Signed-off-by: Jeff King 

As Carlos mentioned, I think you could leave the shebang as
documentation, but I'm OK either way.

-Peff

Re: What's cooking in git.git (Nov 2018, #06; Wed, 21)

[Jeff King]

On Thu, Nov 22, 2018 at 07:36:54PM +0100, Ævar Arnfjörð Bjarmason wrote:

> > Yeah, my intent had been to circle back around to this, but I just
> > hadn't gotten to it. I'm still pondering a config option or similar,
> > though I remain unconvinced that the cases in which you've showed it
> > being slow are actually realistic or worth worrying about
> 
> FWIW those "used to be 2ms are now 20-40ms" pushes on ext4 are
> representative of the actual prod setup I'm mainly targeting. Now, I
> don't run on ext4 this patch helps there, but it seems plausible that it
> matters to someone who's counting on that performance.

Those are actually the more interesting cases, I think. The ones I'm
most skeptical of are the multi-second delays due to having 250K+
objects.

I do also think in the long run we should be fixing the "unreachable
always become loose" issues.

> Buh yeah, it's certainly obscure. I don't blame you if you don't want to
> hack on it, and not ejecting this out before 2.20 isn't going to break
> anything for me. But do you mind if I make it configurable as part of my
> post-2.20 "disable collisions?"

No, not at all.

> >  (and certainly having an obscure config option is not enough to help
> > most people). If we could have it kick in heuristically, that would be
> > better.
> 
> Aside from this specific scenario. I'd really prefer if we avoid having
> heuristic performance optimizations at all costs.
> 
> Database servers tend to do that sort of thing with their query planner,
> and it results in cases where your entire I/O profile changes overnight
> because you're now on the wrong side of some if/else heuristic about
> whather to use some index or not.

Yes, I agree that's a downside. I just think if we make everybody's
performance 10% slower, they're not really going to notice and seek out
a config option to flip to fix it.

> > However, note that the cache-load for finding abbreviations _must_ have
> > the complete list. And has been loading it for some time. So if you run
> > "git-fetch", for example, you've already been running this code for
> > months (and using the cache in more places is now a free speedup).
> 
> This is reminding me that I need to get around to re-submitting my
> core.validateAbbrev series, which addresses this part of the problem:
> https://public-inbox.org/git/20180608224136.20220-21-ava...@gmail.com/

Yeah, I still agree that is a reasonable thing to do.

> > At the very least, we'd want this patch on top, too. I also think René's
> > suggestion use access() is worth pursuing (though to some degree is
> > orthogonal to the cache).
> 
> I haven't had time to test that, and wasn't prioritizing it since I
> figured this was post-2.20. My hunch is it doesn't matter much if at all
> on NFS. The roundtrip time is what matters, whether that roundtrip is
> fstat() or access() probably not.

Yes, that line of reasoning makes sense to me (but I think an easy 2-3%
speedup on local filesystems may be worth it).

-Peff

Re: What's cooking in git.git (Nov 2018, #06; Wed, 21)

[Jeff King]

On Sat, Nov 24, 2018 at 11:11:36AM +0900, Junio C Hamano wrote:

> > However, note that the cache-load for finding abbreviations _must_ have
> > the complete list. And has been loading it for some time. So if you run
> > "git-fetch", for example, you've already been running this code for
> > months (and using the cache in more places is now a free speedup).
> >
> > At the very least, we'd want this patch on top, too. I also think René's
> > suggestion use access() is worth pursuing (though to some degree is
> > orthogonal to the cache).
> 
> OK, will queue, but I wonder where 66f0 comes from before silently
> doing s/66f04152be/3a2e08245c/ myself.

Whoops, I thought I made double-sure to pull the commit id from
origin/jk/loose-object-cache instead of my original local commits, but
obviously I didn't. Thanks for catching it.

-Peff

Re: [PATCH 1/2] format-patch: add a more exhaustive --range-diff test

[Ævar Arnfjörð Bjarmason]

On Sat, Nov 24 2018, Junio C Hamano wrote:

> Ævar Arnfjörð Bjarmason   writes:
>
>> Change the narrow test added in 31e2617a5f ("format-patch: add
>> --range-diff option to embed diff in cover letter", 2018-07-22) to
>> test the full output. This test would have spotted a regression in the
>> output if it wasn't beating around the bush and tested the full
>> output, let's do that.
>
> This looks wrong, given that we are declaring that showing the
> broken --stat in the output is wrong.  It makes us to expect a
> known-wrong output from the command.
>
> If the theme of the topic were "what we are giving by default is a
> sensible output when --stat is asked for, but it is rather too noisy
> and our default should be quieter---let's tone it down", then this
> patch in 1/2 and then a behaviour-change patch with updated
> expectation would make sense, but I do not think that is what you
> are aiming for with this series.
>
> Perhaps squash this into the real "fix" to show what the desired
> output should look like with the same patch?

I see you did that in 'pu'. I don't mind, looks good to me.

FWIW I split this up for ease of review, i.e. showing what the output
was before and what effect the code change had, but maybe that was
overdoing it and it's simpler just to have this all in one go.

>> Signed-off-by: Ævar Arnfjörð Bjarmason 
>> ---
>>  t/t3206-range-diff.sh | 27 ++-
>>  1 file changed, 22 insertions(+), 5 deletions(-)
>>
>> diff --git a/t/t3206-range-diff.sh b/t/t3206-range-diff.sh
>> index e497c1358f..0235c038be 100755
>> --- a/t/t3206-range-diff.sh
>> +++ b/t/t3206-range-diff.sh
>> @@ -249,11 +249,28 @@ for prev in topic master..topic
>>  do
>>  test_expect_success "format-patch --range-diff=$prev" '
>>  git format-patch --stdout --cover-letter --range-diff=$prev \
>> -master..unmodified >actual &&
>> -grep "= 1: .* s/5/A" actual &&
>> -grep "= 2: .* s/4/A" actual &&
>> -grep "= 3: .* s/11/B" actual &&
>> -grep "= 4: .* s/12/B" actual
>> +master..unmodified >actual.raw &&
>> +sed -e "s|^:||" -e "s|:$||" >expect <<-\EOF &&
>> +:1:  4de457d = 1:  35b9b25 s/5/A/
>> +: a => b | 0
>> +: 1 file changed, 0 insertions(+), 0 deletions(-)
>> +::
>> +:2:  fccce22 = 2:  de345ab s/4/A/
>> +: a => b | 0
>> +: 1 file changed, 0 insertions(+), 0 deletions(-)
>> +::
>> +:3:  147e64e = 3:  9af6654 s/11/B/
>> +: a => b | 0
>> +: 1 file changed, 0 insertions(+), 0 deletions(-)
>> +::
>> +:4:  a63e992 = 4:  2901f77 s/12/B/
>> +: a => b | 0
>> +: 1 file changed, 0 insertions(+), 0 deletions(-)
>> +::
>> +:-- :
>> +EOF
>> +sed -ne "/^1:/,/^--/p" actual &&
>> +test_cmp expect actual
>>  '
>>  done

Re: [PATCH] t5562: do not reuse output files

[Max Kirillov]

On Sat, Nov 24, 2018 at 04:47:19PM +0900, Junio C Hamano wrote:
> Junio C Hamano  writes:
> a better workaround might be to write into unique output filenames
> (act1.out, act2.out, etc.); that way, you do not have to worry about
> the output file for the next request getting clobbered by a stale
> process handling the previous request.

Yes I agree

> But at the same time,
> wouldn't this suggest that the test or the previous request may see
> an incomplete output,

Yes it may miss the child's message. But in this case of failed
http-backend process, there should be already one "fatal:"
message in the act.err from the parent, and missing another
one does not change the outcome.

Re: [PATCH] t5562: do not reuse output files

[Junio C Hamano]

Junio C Hamano  writes:

> Max Kirillov  writes:
>
>> diff --git a/t/t5562-http-backend-content-length.sh 
>> b/t/t5562-http-backend-content-length.sh
>> index 90d890d02f..bb53f82c0c 100755
>> --- a/t/t5562-http-backend-content-length.sh
>> +++ b/t/t5562-http-backend-content-length.sh
>> @@ -25,6 +25,8 @@ test_http_env() {
>>  handler_type="$1"
>>  request_body="$2"
>>  shift
>> +(rm -f act.out || true) &&
>> +(rm -f act.err || true) &&
>
> Why "||true"?  If the named file doesn't exist, "rm -f" would
> succeed, and if it does exist but somehow we fail to remove, then
> these added lines are not preveting the next part from reusing,
> i.e. they are not doing what they are supposed to be doing,...

Another thing.  The analysis in your log message talks about a stray
process holding open filehandles to these files.  An attempt to remove
them in such a stat would fail on some systems, so "||true" would not
help, would it?  It just hides the failure to remove, and when ||true
is useful in hiding th failure _is_ when such a stray process is still
there, waiting to corrupt the output of the next request and breaking
the test, no?

I do agree that forcing the parent to wait, like you described in
the comment, would be far more preferrable, but until that happens,
a better workaround might be to write into unique output filenames
(act1.out, act2.out, etc.); that way, you do not have to worry about
the output file for the next request getting clobbered by a stale
process handling the previous request.  But at the same time,
wouldn't this suggest that the test or the previous request may see
an incomplete output, as the analysed problem is that such a process
is writing to the output file very late, while we are preparing to
test the enxt request, which meas we have already checked the output
file for the previous request, right?  So even without ||true, I am
not sure how true a "solution" this change is to the issue.

Re: [PATCH] t5562: do not reuse output files

[Junio C Hamano]

Max Kirillov  writes:

> diff --git a/t/t5562-http-backend-content-length.sh 
> b/t/t5562-http-backend-content-length.sh
> index 90d890d02f..bb53f82c0c 100755
> --- a/t/t5562-http-backend-content-length.sh
> +++ b/t/t5562-http-backend-content-length.sh
> @@ -25,6 +25,8 @@ test_http_env() {
>   handler_type="$1"
>   request_body="$2"
>   shift
> + (rm -f act.out || true) &&
> + (rm -f act.err || true) &&

Why "||true"?  If the named file doesn't exist, "rm -f" would
succeed, and if it does exist but somehow we fail to remove, then
these added lines are not preveting the next part from reusing,
i.e. they are not doing what they are supposed to be doing, so we
should detect such a failure (if happens) as an error, no?

IOW, shouldn't it just be more like

+   rm -f act.out act.err &&

The same comment applies to the other hunk.


>   env \
>   CONTENT_TYPE="application/x-git-$handler_type-pack-request" \
>   QUERY_STRING="/repo.git/git-$handler_type-pack" \
> @@ -155,6 +157,8 @@ test_expect_success 'CONTENT_LENGTH overflow ssite_t' '
>  '
>  
>  test_expect_success 'empty CONTENT_LENGTH' '
> + (rm -f act.out || true) &&
> + (rm -f act.err || true) &&
>   env \
>   QUERY_STRING="service=git-receive-pack" \
>   PATH_TRANSLATED="$PWD"/.git/info/refs \

Re: git overwriting local ignored files?

[Junio C Hamano]

David Mandelberg  writes:

> It seems that git is overwriting my local files on merge if they're in
> .gitignore. See command transcript below. I searched `git help config`
> and Google, but I couldn't find any way to prevent it. Am I missing
> something? (The reason I care about ignored files is that I'm using
> git with a working directory of $HOME to manage my dotfiles, and most
> files in my $HOME are not tracked by git but are still important.)

The .gitignore file is to list "ignored and expendable" class of
files; there is no "ignored but precious class" in Git.