Re: [PATCH] tests: avoid syntax triggering old dash bug

[Torsten Bögershausen]

On Wed, Nov 28, 2018 at 01:47:41AM +, brian m. carlson wrote:
> On Tue, Nov 27, 2018 at 05:42:53PM +0100, Ævar Arnfjörð Bjarmason wrote:
> > Avoid a bug in dash that's been fixed ever since its
> > ec2c84d ("[PARSER] Fix clobbering of checkkwd", 2011-03-15)[1] first
> > released with dash v0.5.7 in July 2011.
> > 
> > This fixes 1/2 tests failing on Debian Lenny & Squeeze. The other
> > failure is due to 1b42f45255 ("git-svn: apply "svn.pathnameencoding"
> > before URL encoding", 2016-02-09).
> 
> Are people still using such versions of Debian?  I only see wheezy (7)
> on the mirrors, not squeeze (6) or lenny (5).  It might be better for us
> to encourage users to upgrade to an OS that has security support rather
> than work around bugs in obsolete OSes.

Yes, I have an old PowerPC box to test if code handle endians right.
And to ask people to upgrade does not conflict with supporting older
versions (if that is as easy as this patch).
I think we can have both.

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

[Per Lundberg]

On 11/28/18 3:21 AM, brian m. carlson wrote:

Thanks for the elaboration, Brian - good to get things down to a 
practical, real-world level.

 > [...]
 >
> I point this out to underscore how fundamental this change is.  People
> overwhelmingly do not read the release notes, so expecting people to
> realize that a change has been made, especially when many people only
> upgrade Git because of a security issue, may result in unexpected
> consequences.

This is one of the more important things of software engineering. _Don't 
mix security fixes with breaking changes_. They are very different 
things and like you say, we can't really expect people to real release 
notes for every little incremental release we do.

That's an important part of the SemVer guarantee: a minor version 
bump/patch level increase means "bug fix" or "added functionality in a 
backwards-compatible way". So: no changing of default behavior or 
semantics, but adding a new behavior which is opt-in is perfectly fine.

> Just because we don't think of this use of Git as normal or desirable > 
> doesn't mean people don't do it and don't expect it to keep working.

In other words, we need to be "bug-by-bug" compatible with previous 
versions. :-) What some people would consider a bug, others would 
consider a feature.

> I think any change we make here has to be opt-in, at least until Git
> 3.0.  A config knob would probably be the right way to go. 

Agree. It's less than optimal but I think it's something that we all 
could live with. Deciding to switching the default (or not) is then 
rightfully postponed to a later time, and we can revisit the pros and 
cons then. The important thing now is to get the functionality 
implemented in a good way, tested and eventually merged.
--
Per Lundberg

Re: [PATCH] advice: don't pointlessly suggest --convert-graft-file

[Junio C Hamano]

Ævar Arnfjörð Bjarmason   writes:

> The advice to run 'git replace --convert-graft-file' added in
> f9f99b3f7d ("Deprecate support for .git/info/grafts", 2018-04-29)
> didn't add an exception for the 'git replace --convert-graft-file'
> codepath itself.
>
> As a result we'd suggest running --convert-graft-file while the user
> was running --convert-graft-file, which makes no sense. Before:
>
> $ git replace --convert-graft-file
> hint: Support for /info/grafts is deprecated
> hint: and will be removed in a future Git version.
> hint:
> hint: Please use "git replace --convert-graft-file"
> hint: to convert the grafts into replace refs.
> hint:
> hint: Turn this message off by running
> hint: "git config advice.graftFileDeprecated false"

That's a good one.  The glitch is real, the improvement is obvious,
and the implementation seems to be straight-forward and sensible.

How did you find one, is the real question ;-)

Re: Git pull confusing output

[Junio C Hamano]

Will  writes:

> I’m far from being a guru, but I consider myself a competent Git
> user. Yet, here’s my understanding of the output of one the most-used
> commands, `git push`:
>> Counting objects: 6, done.
> No idea what an “object” is. Apparently there’s 6 of them
> here. What does “counting” them means? Should I care?

You vaguely recall that the last time you pushed you saw ~400
objects counted there, so you get the feeling how active you were
since then.

It is up to you if you are interested in such a feel of the level of
activity.  "git fetch" (hence "git pull") would also give you a
similar "feel", e.g. "the last fetch was ~1200 objects and today's
is mere ~200, so it seems it is a relatively slow day".

As to "what is an object?", there are plenty of Git tutorials and
books to learn the basics from.  Again, it is up to you if you care.

>> Delta compression using up to 4 threads.
> No idea what is “delta compression”, I suppose something is being
> compressed. It’s using anything between 1 and 4 threads, which is not
> a very precise or useful information. Should I care?

Likewise.

>> Compressing objects: 100% (6/6), done.
> I still don’t know what objects are, but I appreciate having feedback
> on progress

Exactly.

>> Writing objects: 100% (6/6), 656 bytes | 656.00 KiB/s, done.
> Writing what, where? Should I care? Still good to have feedback

You are pushing the data in commits you wrote, modifications you
made to files, etc., to the other side, so that is what is written
to the other side.  Is there any other thing you might suspect that
is written in this context, to make you think a clarification is
needed in the above message?

>> Total 6 (delta 4), reused 0 (delta 0)
> No idea what any of those numbers mean. Should I care?

It is up to you to get interested in these details and learn what
they mean.  In this case, among these 6 objects transferred, Git
managed to find that 4 are similar to other objects the other side
already has or being sent by this push and can be transferred very
efficiently by sending only the difference, which is what "delta"
means.

>> remote: Resolving deltas: 100% (4/4), completed with 4 local objects.
> I do know what’s a remote, but I don’t know what “resolving
> deltas” means. There’s local objects now? I don’t understand what
> happened to those local objects, are they the byproduct of the delta
> resolving or the input or something else? Should I care?

The "remote:" prefix is "the other side said the following".  IOW,
you are seeing the message from the receiving end.  As you sent 4
objects as mere "difference" (not the whole data needed to know
every byte of the file or directory), the receiving side needed to
find the object the "difference" was relative to, and reassemble
what you would have sent if there weren't delta compression.  These
4 local objects were local from the point of view of the other side,
i.e. the repository that received your push.

The information density of this one is much lower than the previous
progress output lines.  This one is primarily to give you the feeling 
of relative speed (you've seen how fast the "writing" phase which is
constrained mostly by over-the-wire speed already, and now you are
observing how many more seconds are spent to post-process the data
sent over the wire) and avoiding to get you bored.

I think we have "--quiet" option for those who do not care.

Re: [PATCH v2 7/7] Suggest other commands instead of "git checkout"

[Junio C Hamano]

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

> The assumption made is here
>
> - "git checkout" is a horrible monster that should only be touched
>   with a two-meter pole
>
> - there are other commands that can achieve the same thing

Thanks for clearly spelling out the assumptions.  It is good that
this step cames at the end, as the earlier 6 steps looked reasonable
to me.

Thanks.


>
> Signed-off-by: Nguyễn Thái Ngọc Duy 
> ---
>  Documentation/git-branch.txt   |  8 ++--
>  Documentation/git-check-ref-format.txt |  2 +-
>  Documentation/git-format-patch.txt |  2 +-
>  Documentation/git-merge-base.txt   |  2 +-
>  Documentation/git-rebase.txt   |  2 +-
>  Documentation/git-remote.txt   |  2 +-
>  Documentation/git-rerere.txt   | 10 ++---
>  Documentation/git-reset.txt| 18 -
>  Documentation/git-revert.txt   |  2 +-
>  Documentation/git-stash.txt|  6 +--
>  Documentation/gitattributes.txt|  2 +-
>  Documentation/gitcli.txt   |  4 +-
>  Documentation/gitcore-tutorial.txt | 18 -
>  Documentation/giteveryday.txt  | 24 ++--
>  Documentation/githooks.txt |  5 ++-
>  Documentation/gittutorial-2.txt|  2 +-
>  Documentation/gittutorial.txt  |  4 +-
>  Documentation/revisions.txt|  2 +-
>  Documentation/user-manual.txt  | 54 +-
>  advice.c   |  2 +-
>  sha1-name.c|  2 +-
>  wt-status.c|  2 +-
>  22 files changed, 88 insertions(+), 87 deletions(-)
>
> diff --git a/Documentation/git-branch.txt b/Documentation/git-branch.txt
> index bf5316ffa9..1564df47d2 100644
> --- a/Documentation/git-branch.txt
> +++ b/Documentation/git-branch.txt
> @@ -48,7 +48,7 @@ The command's second form creates a new branch head named 
> 
>  which points to the current `HEAD`, or  if given.
>  
>  Note that this will create the new branch, but it will not switch the
> -working tree to it; use "git checkout " to switch to the
> +working tree to it; use "git switch-branch " to switch to the
>  new branch.
>  
>  When a local branch is started off a remote-tracking branch, Git sets up the
> @@ -194,7 +194,7 @@ This option is only applicable in non-verbose mode.
>  +
>  This behavior is the default when the start point is a remote-tracking 
> branch.
>  Set the branch.autoSetupMerge configuration variable to `false` if you
> -want `git checkout` and `git branch` to always behave as if `--no-track`
> +want `git switch-branch` and `git branch` to always behave as if `--no-track`
>  were given. Set it to `always` if you want this behavior when the
>  start-point is either a local or remote-tracking branch.
>  
> @@ -293,7 +293,7 @@ Start development from a known tag::
>  $ git clone git://git.kernel.org/pub/scm/.../linux-2.6 my2.6
>  $ cd my2.6
>  $ git branch my2.6.14 v2.6.14   <1>
> -$ git checkout my2.6.14
> +$ git switch-branch my2.6.14
>  
>  +
>  <1> This step and the next one could be combined into a single step with
> @@ -319,7 +319,7 @@ NOTES
>  -
>  
>  If you are creating a branch that you want to checkout immediately, it is
> -easier to use the git checkout command with its `-b` option to create
> +easier to use the "git switch-branch" command with its `-b` option to create
>  a branch and check it out with a single command.
>  
>  The options `--contains`, `--no-contains`, `--merged` and `--no-merged`
> diff --git a/Documentation/git-check-ref-format.txt 
> b/Documentation/git-check-ref-format.txt
> index d9de992585..38c2169d7a 100644
> --- a/Documentation/git-check-ref-format.txt
> +++ b/Documentation/git-check-ref-format.txt
> @@ -88,7 +88,7 @@ but it is explicitly forbidden at the beginning of a branch 
> name).
>  When run with `--branch` option in a repository, the input is first
>  expanded for the ``previous checkout syntax''
>  `@{-n}`.  For example, `@{-1}` is a way to refer the last thing that
> -was checked out using "git checkout" operation. This option should be
> +was checked out using "git switch-branch" operation. This option should be
>  used by porcelains to accept this syntax anywhere a branch name is
>  expected, so they can act as if you typed the branch name. As an
>  exception note that, the ``previous checkout operation'' might result
> diff --git a/Documentation/git-format-patch.txt 
> b/Documentation/git-format-patch.txt
> index aba4c5febe..0ceaa1173c 100644
> --- a/Documentation/git-format-patch.txt
> +++ b/Documentation/git-format-patch.txt
> @@ -416,7 +416,7 @@ One way to test if your MUA is set up correctly is:
>  * Apply it:
>  
>  $ git fetch  master:test-apply
> -$ git checkout test-apply
> +$ git switch-branch test-apply
>  $ git reset --hard
>  $ git am a.patch
>  
> diff --git a/Documentation/git-merge-base.txt 
> b/Documentation/git-merge-base.txt
> index 9f07f4f6ed..1b25e5d530 100644
> --- 

Re: [PATCH v2 6/7] checkout: split into switch-branch and checkout-files

[Junio C Hamano]

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

> The good old "git checkout" command is still here and will be until
> all (or most of users) are sick of it.

Two comments on the goal (the implementation looked reasonable
assuming the reader agrees with the gaol).

At least to me, the verb "switch" needs two things to switch
between, i.e. "switch A and B", unless it is "switch to X".
Either "switch-to-branch" or simply "switch-to", perhaps?

As I already hinted in my response to Stefan (?) about
checkout-from-tree vs checkout-from-index, a command with multiple
modes of operation is not confusing to people with the right mental
model, and I suspect that having two separate commands for "checking
out a branch" and "checking out paths" that is done by this step
would help users to form the right mental model.  So I tend to think
these two are "training wheels", and suspect that once they got it,
nobody will become "sick of" the single "checkout" command that can
be used to do either.  It's just the matter of being aware what can
be done (which requires the right mental model) and how to tell Git
what the user wants it do (two separate commands, operating mode
option, or just the implied command line syntax---once the user
knows what s/he is doing, these do not make that much a difference).

As to the implementation,

>   int dwim_new_local_branch;
> + int accept_pathspec;
> + int empty_arg_ok;

I think this is a reasonable way to completely avoid the codepath
that considers an unknown arg being parsed could be a pathspec
element (e.g. switch-to-that-branch mode will never want to see
pathspec, so disambiguation logic and/or hint should not kick in).

At the beginning of cmd_switch_branches(), please "explicitly"
assign 0 to accept_pathspec field, after memset(0) clears the whole
structure.  When a reader sees memset(0), it is read as "giving a
clean and bland slate to futz with with a reasonable default", but
for this particular field, i.e. accept_pathspec, there is NO
reasoanble default.  It's not like switch-to-branch mode is the heir
to checkout and the other sibling checkout-files is doing a
non-default behaviour.

Same comment probably would apply to the other one, although I
didn't think too deeply about that one.


>   /*
>* If new checkout options are added, skip_merge_working_tree
> @@ -1056,7 +1068,7 @@ static int parse_branchname_arg(int argc, const char 
> **argv,
>   arg = argv[0];
>   dash_dash_pos = -1;
>   for (i = 0; i < argc; i++) {
> - if (!strcmp(argv[i], "--")) {
> + if (opts->accept_pathspec && !strcmp(argv[i], "--")) {
>   dash_dash_pos = i;
>   break;
>   }
> @@ -1067,6 +1079,8 @@ static int parse_branchname_arg(int argc, const char 
> **argv,
>   has_dash_dash = 1; /* case (3) or (1) */
>   else if (dash_dash_pos >= 2)
>   die(_("only one reference expected, %d given."), dash_dash_pos);
> + else if (!opts->accept_pathspec)
> + has_dash_dash = 1;
>  
>   if (!strcmp(arg, "-"))
>   arg = "@{-1}";
> @@ -1291,30 +1305,23 @@ static struct option 
> *add_checkout_path_options(struct checkout_opts *opts,
>   return newopts;
>  }
>  
> -int cmd_checkout(int argc, const char **argv, const char *prefix)
> +static int checkout_main(int argc, const char **argv, const char *prefix,
> +  struct checkout_opts *opts, struct option *options,
> +  const char * const usagestr[])
>  {
> - struct checkout_opts real_opts;
> - struct checkout_opts *opts = _opts;
>   struct branch_info new_branch_info;
>   int dwim_remotes_matched = 0;
> - struct option *options = NULL;
>  
> - memset(opts, 0, sizeof(*opts));
>   memset(_branch_info, 0, sizeof(new_branch_info));
>   opts->overwrite_ignore = 1;
>   opts->prefix = prefix;
>   opts->show_progress = -1;
> - opts->dwim_new_local_branch = 1;
>  
>   git_config(git_checkout_config, opts);
>  
>   opts->track = BRANCH_TRACK_UNSPECIFIED;
>  
> - options = add_common_options(opts, options);
> - options = add_switch_branch_options(opts, options);
> - options = add_checkout_path_options(opts, options);
> -
> - argc = parse_options(argc, argv, prefix, options, checkout_usage,
> + argc = parse_options(argc, argv, prefix, options, usagestr,
>PARSE_OPT_KEEP_DASHDASH);
>  
>   if (opts->show_progress < 0) {
> @@ -1381,7 +1388,8 @@ int cmd_checkout(int argc, const char **argv, const 
> char *prefix)
>_remotes_matched);
>   argv += n;
>   argc -= n;
> - }
> + } else if (!opts->empty_arg_ok)
> + usage_with_options(usagestr, options);
>  
>   if (argc) {
>   parse_pathspec(>pathspec, 0,
> @@ -1443,3 +1451,60 @@ int cmd_checkout(int argc, const char **argv, const 
> char 

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

[J.H. van de Water]

> > me@work /cygdrive
> > $ ls
> > c  d
> >
> > So `/cygdrive` *is* a valid directory in Cygwin.
> 
> That supports the code that does not special case a path that begins
> with /cygdrive/ and simply treats it as a full path and freely use
> relative path, I guess.  Very good point.

Please read

https://cygwin.com/cygwin-ug-net/using.html#cygdrive
( The cygdrive path prefix )

 you can access arbitary drives on your system by using the cygdrive path
prefix. The default value for this prefix is /cygdrive ...


The cygdrive prefix is a >>> virtual directory <<< under which all drives on
a system are subsumed ...


The cygdrive prefix may be CHANGED in the fstab file as outlined above !


To simplify scripting, Cygwin also provides a /proc/cygdrive symlink, ...

=

Re: [PATCH v2 1/7] parse-options: allow parse_options_concat(NULL, options)

[Junio C Hamano]

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

> There is currently no caller that calls this function with "a" being
> NULL. But it will be introduced shortly. It is used to construct the
> option array from scratch, e.g.
>
>struct parse_options opts = NULL;

Missing asterisk somewhere?

>opts = parse_options_concat(opts, opts_1);
>opts = parse_options_concat(opts, opts_2);
>
> Signed-off-by: Nguyễn Thái Ngọc Duy 
> ---
>  parse-options-cb.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/parse-options-cb.c b/parse-options-cb.c
> index 8c9edce52f..c609d52926 100644
> --- a/parse-options-cb.c
> +++ b/parse-options-cb.c
> @@ -126,7 +126,7 @@ struct option *parse_options_concat(struct option *a, 
> struct option *b)
>   struct option *ret;
>   size_t i, a_len = 0, b_len = 0;
>  
> - for (i = 0; a[i].type != OPTION_END; i++)
> + for (i = 0; a && a[i].type != OPTION_END; i++)
>   a_len++;
>   for (i = 0; b[i].type != OPTION_END; i++)
>   b_len++;

Re: [PATCH] tests: avoid syntax triggering old dash bug

[Junio C Hamano]

Eric Sunshine  writes:

> On Tue, Nov 27, 2018 at 11:43 AM Ævar Arnfjörð Bjarmason
>  wrote:
>> Avoid a bug in dash that's been fixed ever since its
>> ec2c84d ("[PARSER] Fix clobbering of checkkwd", 2011-03-15)[1] first
>> released with dash v0.5.7 in July 2011.
>
> Perhaps enhance the commit message to explain the nature of the bug
> itself. It is not at all obvious from reading the above or from
> looking at the diff itself what the actual problem is that the patch
> is fixing. (And it wasn't even immediately obvious by looking at the
> commit message of ec2c84d in the dash repository.) To help readers of
> this patch avoid re-introducing this problem or diagnose such a
> failure, it might be a good idea to give an example of the syntax
> which trips up old dash (i.e. a here-doc followed immediately by a
> {...} expression) and the actual error message 'Syntax error: "}"
> unexpected'.

Indeed.  From the patch text, I would not have even guessed.  I was
wondering if there were interactions with "" and $() inside it.

If having {...} immediately after a here-doc is a problem, then the
patch should not touch existing code at all, but instead insert a
new line, perhaps like

: avoid open brace immediately after here-doc for old dash

immediately before {...}; that would have made it easier to grok.

>@@ -892,8 +892,9 @@ test_expect_success 'get --expiry-date' '
>   1510348087
>   0
>   EOF
>+  date_valid1=$(git config --expiry-date date.valid1) &&
>   {
>-  echo "$rel_out $(git config --expiry-date date.valid1)"
>+  echo "$rel_out $date_valid1"
>   git config --expiry-date date.valid2 &&
>   git config --expiry-date date.valid3 &&
>   git config --expiry-date date.valid4 &&

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

[Jonathan Nieder]

Hi,

Junio C Hamano wrote:
>> Ævar Arnfjörð Bjarmason  writes:

>>> 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).
[...]
> 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(-)

I thought I should weigh in on how this would affect Debian's and
Google's deployments.

First of all, I've looked over the revert patch carefully and it is
well written and does what it says on the tin.

At https://bugs.debian.org/914695 is a report of a test regression in
an outside project that is very likely to have been triggered by the
new faster rebase code.  The issue has not been triaged, so I don't
know yet whether it's a problem in rebase-in-c or a manifestation of a
bug in the test.

That said, Google has been running with the new rebase since ~1 month
ago when it became the default, with no issues reported by users.  As
a result, I am confident that it can cope with what most users of
"next" throw at it, which means that if we are to find more issues to
polish it better, it will need all the exposure it can get.

In the Google deployment, we will keep using rebase-in-c even if it
gets disabled by default, in order to help with that.

>From the Debian point of view, it's only a matter of time before
rebase-in-c becomes the default: even if it's not the default in 2.20,
it would presumably be so in 2.21 or 2.22.  That means the community's
attention when resolving security and reliability bugs would be on the
rebase-in-c implementation.  As a result, the Debian package will most
likely enable rebase-in-c by default even if upstream disables it, in
order to increase the package's shelf life (i.e. to ease the
maintenance burden of supporting whichever version of the package ends
up in the next Debian stable).

So with either hat on, it doesn't matter whether you apply this patch
upstream.

Having two pretty different deployments end up with the same
conclusion leads me to suspect that it's best for upstream not to
apply the revert patch, unless either

  (a) we have a concrete regression to address and then try again, or
  (b) we have a test or other plan to follow before trying again.

Thanks and hope that helps,
Jonathan

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

[Max Kirillov]

On Mon, Nov 26, 2018 at 11:06:40AM +0900, Junio C Hamano wrote:
> 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?

As far as I understand, the reason to _not_ wait was that
the child might still be expecting closed stdin even after
receiving SIGTERM, so parent would wait forever. But this is
not clearly the case here. And otherwise there should be no
reason to not wait, as long as we are interested in
synchronous exiting of the child.

In my Linux experiments the child was exiting because of signal
earlier than because of closed stdin, but who knows, maybe
with some bad luck the signal would be delivered after the
closed stdin, and we would still have the issue. After all,
at Linux it was mostly working even without fix.

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

[Junio C Hamano]

Johannes Schindelin  writes:

> It takes a little folding and knotting of the brain to understand that
> this `!skip_dos_drive_prefix()` has *nothing* to do with the comment
> `unc paths` nor with the test whether the paths starts with two directory
> separators.
>
> As a consequence, I would highly suggest to turn this into:
>
>   if (skip_dos_drive_prefix())
>   ; /* absolute path with DOS drive prefix */
>   /* unc paths */
>   else if (is_dir_sep(pos[0]) && is_dir_sep(pos[1])) {
>
> That makes the code a lot easier to understand, and as a consequence a lot
> harder to mess up in the future.

Excellent.  With or without "unc paths" comment, the separation does
make the logic more clear.

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

[Junio C Hamano]

Johannes Schindelin  writes:

>> Sorry, but I fail to see the point the last example wants to make.
>
> I agree. For me, the real test is this:
>
> me@work ~
> $ cd /cygdrive
>
> me@work /cygdrive
> $ ls
> c  d
>
> So `/cygdrive` *is* a valid directory in Cygwin.

That supports the code that does not special case a path that begins
with /cygdrive/ and simply treats it as a full path and freely use
relative path, I guess.  Very good point.

Re: [ANNOUNCE] Git v2.20.0-rc1

[Junio C Hamano]

Johannes Schindelin  writes:

> ...
> 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?

Any response?  Or can I retract jc/postpone-rebase-in-c that was
prepared as a reaction to this?

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

[Junio C Hamano]

Ævar Arnfjörð Bjarmason  writes:

> What do you think about some patch like that which retains the plumbing
> behavior for things like read-tree, doesn't introduce "precious" or
> "trashable", and just makes you specify "[checkout|merge|...] --force"
> in cases where we'd have clobbering?

Whether you like it or not, don't people's automation use tons of
invocations of "git merge", "git checkout", etc.?  You'd be breaking
them by such a change.  Other than that, if we never had Git before
and do not have to worry about existing users, I'd think it would be
a lot closer to the ideal than today's system if "checkout 
foo.o" rejected overwriting "foo.o" that is not tracked in the
current index but matches an ignore pattern, and required a
"--force" option to overwrite it.

A user, during a conflict resolution, may say "I want this 'git
checkout foo/' to ignore conflicted paths in that directory, so I
would give "--force" option to it, but now "--force" also implies
that I am willing to clobber ignored paths, which means I cannot use
it".

I would think that a proper automation needs per-path hint from the
user and/or the project, not just a single-size-fits-all --force
option, and "unlike all the *.o ignored files that are expendable,
this vendor-supplied-object.o is not" is one way to give such a
per-path hint.

> This would give scripts which relied on our stable plumbing consistent
> behavior, while helping users who're using our main porcelain not to
> lose data. I could then add a --force option to the likes of read-tree
> (on by default), so you could get porcelain-like behavior with
> --no-force.

At that low level, I suspect that a single size fits all "--force"
would work even less well.

Re: Forcing GC to always fail

[Bryan Turner]

On Tue, Nov 27, 2018 at 5:55 PM Elijah Newren  wrote:
>
> On Tue, Nov 27, 2018 at 4:16 PM Ævar Arnfjörð Bjarmason
>  wrote:
> >
> > On Wed, Nov 28 2018, Bryan Turner wrote:
> >
> > > On Tue, Nov 27, 2018 at 3:47 PM Ævar Arnfjörð Bjarmason
> > >  wrote:
> > >>
> > >> On Tue, Nov 27 2018, Bryan Turner wrote:
> > >>
> > >> >
> > >> > Is there anything I can set, perhaps some invalid configuration
> > >> > option/value, that will make "git gc" (most important) and "git
> > >> > reflog" (ideal, but less important) fail when they're run in our
> > >> > repositories? Hopefully at that point customers will reach out to us
> > >> > for help _before_ they corrupt their repositories.
> > >>
> > >> $ stahp='Bryan.Turner.will.hunt.you.down.if.you.manually.run.gc' && 
> > >> git -c gc.pruneExpire=$stahp gc; git -c gc.reflogExpire=$stahp reflog 
> > >> expire
> > >> error: Invalid gc.pruneexpire: 
> > >> 'Bryan.Turner.will.hunt.you.down.if.you.manually.run.gc'
> > >> fatal: unable to parse 'gc.pruneexpire' from command-line config
> > >> error: 'Bryan.Turner.will.hunt.you.down.if.you.manually.run.gc' for 
> > >> 'gc.reflogexpire' is not a valid timestamp
> > >> fatal: unable to parse 'gc.reflogexpire' from command-line config
> > >
> > > Thanks for that! It looks like that does block both "git gc" and "git
> > > reflog expire" without blocking "git pack-refs", "git repack" or "git
> > > prune". Fantastic! The fact that it shows the invalid value means it
> > > might also be possible to at least provide a useful hint that manual
> > > GC is not safe.
> > >
> > > I appreciate your help, Ævar.
> >
> > No problem. I was going to add that you can set
> > e.g. pack.windowMemory='some.message' to make this go for git-repack
> > too, but it sounds like you don't want that.
> >
> > Is there a reason for why BitBucket isn't using 'git-gc'? Some other
> > hosting providers use it, and if you don't run it with "expire now" or
> > similarly aggressive non-default values on an active repository it won't
> > corrupt anything.

We did use "git gc" (sans options; its configuration was applied via
"config") for several years, but there were some rough edges.

The biggest one was that "git gc" has a canned set of steps it runs
that can't be disabled, even when they add no value (or are actively
detrimental).

For us, the biggest issue was "git gc"'s insistence on trying to run
"git reflog expire". That triggers locking behaviors that resulted in
very frequent GC failures--and the only reflogs Bitbucket Server (by
default) creates are all configured to never ex[ire or be pruned, so
the effort is all wasted anyway.

Counting objects: 3, done.
Delta compression using up to 8 threads.
Compressing objects: 100% (2/2), done.
Writing objects: 100% (3/3), 270 bytes | 0 bytes/s, done.
Total 3 (delta 1), reused 0 (delta 0)
remote: error: cannot lock ref 'stash-refs/pull-requests/13996/merge':
ref 'stash-refs/pull-requests/13996/merge' is at
2ec6a74b453d76f7a5247baa9f396361027ffdf but expected
1678f303202010c6d7e6201226df08dc8fc49ae3
remote: error: cannot lock ref 'stash-refs/pull-requests/13996/to':
ref 'stash-refs/pull-requests/13996/to' is at
bd32d53e4fe63b15be029085f1b6d795d526adbc but expected
f55ec06e89a11b8bdbcd97680a900361307d28c4
remote: error: cannot lock ref 'stash-refs/pull-requests/14006/merge':
ref 'stash-refs/pull-requests/14006/merge' is at
1cc907e2a7082033d70a164f222d3cce17a453a9 but expected
ae057003d7ed7d096b5b952191d784113f25b982
remote: error: cannot lock ref 'stash-refs/pull-requests/14006/to':
ref 'stash-refs/pull-requests/14006/to' is at
bd32d53e4fe63b15be029085f1b6d795d526adbc but expected
f55ec06e89a11b8bdbcd97680a900361307d28c4
remote: error: cannot lock ref 'stash-refs/pull-requests/14043/merge':
ref 'stash-refs/pull-requests/14043/merge' is at
a2e510b1b2b583b273f6d6d28e13151619e8d143 but expected
7735a5bde21815d307c68244e8fd2d67a09d5a39
remote: error: cannot lock ref 'stash-refs/pull-requests/14043/to':
ref 'stash-refs/pull-requests/14043/to' is at
bd32d53e4fe63b15be029085f1b6d795d526adbc but expected
f55ec06e89a11b8bdbcd97680a900361307d28c4
remote: error: cannot lock ref 'stash-refs/pull-requests/14047/merge':
ref 'stash-refs/pull-requests/14047/merge' is at
bd4f0e9bcbed34fd9befa65763f5aee6c9ebd8ce but expected
649dea948e8e6b54506615e5d61c6779c242d5af
remote: error: cannot lock ref 'stash-refs/pull-requests/14047/to':
ref 'stash-refs/pull-requests/14047/to' is at
bd32d53e4fe63b15be029085f1b6d795d526adbc but expected
f55ec06e89a11b8bdbcd97680a900361307d28c4
remote: error: failed to run reflog
To https://...
   f55ec06..bd32d53  master -> master

(Note: That example was from when "git gc --auto" was running attached
to a push, because auto-detach doesn't always detach, but our explicit
"git gc" processing would fail with the same "cannot lock ref"
messages.)

The worktree and rerere cleanup "git gc" does is also unnecessary
overhead, but was less of a concern because it wouldn't make GC
_fail_.

Re: Forcing GC to always fail

[Elijah Newren]

On Tue, Nov 27, 2018 at 4:16 PM Ævar Arnfjörð Bjarmason
 wrote:
>
> On Wed, Nov 28 2018, Bryan Turner wrote:
>
> > On Tue, Nov 27, 2018 at 3:47 PM Ævar Arnfjörð Bjarmason
> >  wrote:
> >>
> >> On Tue, Nov 27 2018, Bryan Turner wrote:
> >>
> >> >
> >> > Is there anything I can set, perhaps some invalid configuration
> >> > option/value, that will make "git gc" (most important) and "git
> >> > reflog" (ideal, but less important) fail when they're run in our
> >> > repositories? Hopefully at that point customers will reach out to us
> >> > for help _before_ they corrupt their repositories.
> >>
> >> $ stahp='Bryan.Turner.will.hunt.you.down.if.you.manually.run.gc' && 
> >> git -c gc.pruneExpire=$stahp gc; git -c gc.reflogExpire=$stahp reflog 
> >> expire
> >> error: Invalid gc.pruneexpire: 
> >> 'Bryan.Turner.will.hunt.you.down.if.you.manually.run.gc'
> >> fatal: unable to parse 'gc.pruneexpire' from command-line config
> >> error: 'Bryan.Turner.will.hunt.you.down.if.you.manually.run.gc' for 
> >> 'gc.reflogexpire' is not a valid timestamp
> >> fatal: unable to parse 'gc.reflogexpire' from command-line config
> >
> > Thanks for that! It looks like that does block both "git gc" and "git
> > reflog expire" without blocking "git pack-refs", "git repack" or "git
> > prune". Fantastic! The fact that it shows the invalid value means it
> > might also be possible to at least provide a useful hint that manual
> > GC is not safe.
> >
> > I appreciate your help, Ævar.
>
> No problem. I was going to add that you can set
> e.g. pack.windowMemory='some.message' to make this go for git-repack
> too, but it sounds like you don't want that.
>
> Is there a reason for why BitBucket isn't using 'git-gc'? Some other
> hosting providers use it, and if you don't run it with "expire now" or
> similarly aggressive non-default values on an active repository it won't
> corrupt anything.

...assuming no other repo has this one as an alternate, which I
suspect is the issue at play.  (I wrote an alternate-aware gc script
years ago when using Atlassian Stash to try to workaround this issue,
but think I only used it for a couple repos and never got around to
deploying it in prod for continuous use, probably worried I had missed
a corner case.  Had meant to, but at some point the powers that be
decided to push us toward a different repository manager tool, and
I've long since forgotten most details.)

Re: [PATCH] tests: avoid syntax triggering old dash bug

[brian m. carlson]

On Tue, Nov 27, 2018 at 05:42:53PM +0100, Ævar Arnfjörð Bjarmason wrote:
> Avoid a bug in dash that's been fixed ever since its
> ec2c84d ("[PARSER] Fix clobbering of checkkwd", 2011-03-15)[1] first
> released with dash v0.5.7 in July 2011.
> 
> This fixes 1/2 tests failing on Debian Lenny & Squeeze. The other
> failure is due to 1b42f45255 ("git-svn: apply "svn.pathnameencoding"
> before URL encoding", 2016-02-09).

Are people still using such versions of Debian?  I only see wheezy (7)
on the mirrors, not squeeze (6) or lenny (5).  It might be better for us
to encourage users to upgrade to an OS that has security support rather
than work around bugs in obsolete OSes.
-- 
brian m. carlson: Houston, Texas, US
OpenPGP: https://keybase.io/bk2204


signature.asc
Description: PGP signature

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

[brian m. carlson]

On Tue, Nov 27, 2018 at 02:50:34PM +, Per Lundberg wrote:
> I agree strongly with this personally; if we must choose between "might
> break automation" and "might delete non-garbage files", I would say the
> former is the lesser evil of the two.
> 
> But, if I had 10 000 000 servers set up using automated scripts that
> would break because of this, I might think differently. Quite likely so,
> in fact.
> 
> What are these automation scenarios _more specifically_? Junio or Brian,
> would you care to elaborate? Is it for build servers where you want "git
> clean -dfx" to always reset the working copy to a pristine state or are
> we talking about some other scenarios?

We had long-running CI servers, since bootstrapping a new system took an
hour.  These would check out the branch to test and run some process
(essentially, a "make" and "make test").  Then, another branch would be
tested, and so on.  The old branch would likely not be merged at this
point.

The scenario I'm thinking of is when a file (say a CSS file) became
built instead of stored in the repository.  Then the file would be added
to .gitignore in the new commit, and it would be generated as part of
the make step.  It would be important to blow away that file away when
checking out a new commit, because not doing so would mean that the CI
system would simply fail to work and require manual intervention.

Moreover, a CI job might fail, due to either a flaky test or a
legitimate failures, so the job might need to be re-run multiple times.
Requiring human intervention, especially when such jobs might be running
at odd hours, would be undesirable.

Another thing we did was to use a specially named gitignore file in our
build step.  We created a new repository, copied the special gitignore
file in as .gitignore, copied in the source and build products, ran git
add and git commit, and then ran git clean -dfx to remove proprietary
source code, packaging the result.  A change to the behavior of git
clean -dfx would be devastating here.

I point this out to underscore how fundamental this change is.  People
overwhelmingly do not read the release notes, so expecting people to
realize that a change has been made, especially when many people only
upgrade Git because of a security issue, may result in unexpected
consequences.  Just because we don't think of this use of Git as normal
or desirable doesn't mean people don't do it and don't expect it to
keep working.  People do read and rely on our documentation.

I think any change we make here has to be opt-in, at least until Git
3.0.  A config knob would probably be the right way to go.  I realize
that may not provide all the benefits we want out of the box, but it
lets people turn the option on once and forget about it.  It also lets
people who don't desire this new behavior explicitly turn it off.
-- 
brian m. carlson: Houston, Texas, US
OpenPGP: https://keybase.io/bk2204


signature.asc
Description: PGP signature

Re: Forcing GC to always fail

[Ævar Arnfjörð Bjarmason]

On Wed, Nov 28 2018, Bryan Turner wrote:

> On Tue, Nov 27, 2018 at 3:47 PM Ævar Arnfjörð Bjarmason
>  wrote:
>>
>>
>> On Tue, Nov 27 2018, Bryan Turner wrote:
>>
>> >
>> > Is there anything I can set, perhaps some invalid configuration
>> > option/value, that will make "git gc" (most important) and "git
>> > reflog" (ideal, but less important) fail when they're run in our
>> > repositories? Hopefully at that point customers will reach out to us
>> > for help _before_ they corrupt their repositories.
>>
>> You could fix this and so many other issues by just hanging up a "Is
>> This Good For The Company?" banner up in Atlassian offices .
>
> Not sure I understand what this means, or what your goal was in saying
> it. No one inside Atlassian is running these commands. I'm trying to
> help save administrators from themselves, which reduces real world
> end-user pain that comes from decisions made without fully
> understanding the consequences. It feels like this comment is mocking
> my earnest desire to help, and my genuine question looking for any
> insight people more familiar with the code might be able to offer.
> Perhaps I'm just missing the joke, but if it's an Office Space
> reference it feels like it's in pretty poor taste.

I (mis)read 'administrators' as being other people at Atlassian. Yeah
it's a reference to Office Space. I meaning to poke some fun at the
situation of having to defensively configure tools least co-workers run
them the wrong way, which I'm sure we've all had to do at some point. I
didn't mean any offense by it.

>>
>> But more seriously:
>>
>> $ stahp='Bryan.Turner.will.hunt.you.down.if.you.manually.run.gc' && git 
>> -c gc.pruneExpire=$stahp gc; git -c gc.reflogExpire=$stahp reflog expire
>> error: Invalid gc.pruneexpire: 
>> 'Bryan.Turner.will.hunt.you.down.if.you.manually.run.gc'
>> fatal: unable to parse 'gc.pruneexpire' from command-line config
>> error: 'Bryan.Turner.will.hunt.you.down.if.you.manually.run.gc' for 
>> 'gc.reflogexpire' is not a valid timestamp
>> fatal: unable to parse 'gc.reflogexpire' from command-line config
>
> Thanks for that! It looks like that does block both "git gc" and "git
> reflog expire" without blocking "git pack-refs", "git repack" or "git
> prune". Fantastic! The fact that it shows the invalid value means it
> might also be possible to at least provide a useful hint that manual
> GC is not safe.
>
> I appreciate your help, Ævar.

No problem. I was going to add that you can set
e.g. pack.windowMemory='some.message' to make this go for git-repack
too, but it sounds like you don't want that.

Is there a reason for why BitBucket isn't using 'git-gc'? Some other
hosting providers use it, and if you don't run it with "expire now" or
similarly aggressive non-default values on an active repository it won't
corrupt anything.

Re: Forcing GC to always fail

[Bryan Turner]

On Tue, Nov 27, 2018 at 3:47 PM Ævar Arnfjörð Bjarmason
 wrote:
>
>
> On Tue, Nov 27 2018, Bryan Turner wrote:
>
> >
> > Is there anything I can set, perhaps some invalid configuration
> > option/value, that will make "git gc" (most important) and "git
> > reflog" (ideal, but less important) fail when they're run in our
> > repositories? Hopefully at that point customers will reach out to us
> > for help _before_ they corrupt their repositories.
>
> You could fix this and so many other issues by just hanging up a "Is
> This Good For The Company?" banner up in Atlassian offices .

Not sure I understand what this means, or what your goal was in saying
it. No one inside Atlassian is running these commands. I'm trying to
help save administrators from themselves, which reduces real world
end-user pain that comes from decisions made without fully
understanding the consequences. It feels like this comment is mocking
my earnest desire to help, and my genuine question looking for any
insight people more familiar with the code might be able to offer.
Perhaps I'm just missing the joke, but if it's an Office Space
reference it feels like it's in pretty poor taste.

>
> But more seriously:
>
> $ stahp='Bryan.Turner.will.hunt.you.down.if.you.manually.run.gc' && git 
> -c gc.pruneExpire=$stahp gc; git -c gc.reflogExpire=$stahp reflog expire
> error: Invalid gc.pruneexpire: 
> 'Bryan.Turner.will.hunt.you.down.if.you.manually.run.gc'
> fatal: unable to parse 'gc.pruneexpire' from command-line config
> error: 'Bryan.Turner.will.hunt.you.down.if.you.manually.run.gc' for 
> 'gc.reflogexpire' is not a valid timestamp
> fatal: unable to parse 'gc.reflogexpire' from command-line config

Thanks for that! It looks like that does block both "git gc" and "git
reflog expire" without blocking "git pack-refs", "git repack" or "git
prune". Fantastic! The fact that it shows the invalid value means it
might also be possible to at least provide a useful hint that manual
GC is not safe.

I appreciate your help, Ævar.

Bryan

Re: Forcing GC to always fail

[Ævar Arnfjörð Bjarmason]

On Tue, Nov 27 2018, Bryan Turner wrote:

> Something of an odd question, but is there something I can do in the
> configuration for a repository that forces any "git gc" run in that
> repository to always fail without doing anything? (Ideally I'd like to
> make "git reflog expire" _also_ fail.)
>
> Background: For Bitbucket Server, we have a fairly recurrent issue
> where administrators decide they know how to manage garbage collection
> for our repositories better than we do, so they jump on the server and
> start running things like this:
>
> git reflog expire --expire=now –all
> git gc --prune=now
> git repack -adf --window=200 --depth=200
>
> They then come running to us with their corrupted repository expecting
> and/or hoping that we can fix it (often without proper backups).
>
> Bitbucket Server itself never runs "git gc" (or "git reflog expire").
> We've configured how reflog expiry should be handled, but of course
> that's overridden by explicit command line options like
> "--expire=now". We _do_ run "git pack-refs", "git repack" and "git
> prune" (with various options), so those commands need to continue to
> work.
>
> Is there anything I can set, perhaps some invalid configuration
> option/value, that will make "git gc" (most important) and "git
> reflog" (ideal, but less important) fail when they're run in our
> repositories? Hopefully at that point customers will reach out to us
> for help _before_ they corrupt their repositories.

You could fix this and so many other issues by just hanging up a "Is
This Good For The Company?" banner up in Atlassian offices .

But more seriously:

$ stahp='Bryan.Turner.will.hunt.you.down.if.you.manually.run.gc' && git -c 
gc.pruneExpire=$stahp gc; git -c gc.reflogExpire=$stahp reflog expire
error: Invalid gc.pruneexpire: 
'Bryan.Turner.will.hunt.you.down.if.you.manually.run.gc'
fatal: unable to parse 'gc.pruneexpire' from command-line config
error: 'Bryan.Turner.will.hunt.you.down.if.you.manually.run.gc' for 
'gc.reflogexpire' is not a valid timestamp
fatal: unable to parse 'gc.reflogexpire' from command-line config

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

[Ævar Arnfjörð Bjarmason]

On Tue, Nov 27 2018, Johannes Schindelin wrote:

> Hi Junio & Paul,
>
> On Mon, 26 Nov 2018, Junio C Hamano wrote:
>
>> 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.
>
> Feel very free to cherry-pick
> https://github.com/git-for-windows/git/commit/004da7e7faa36c872868ae938e06594ea1c2f01c
> and
> https://github.com/git-for-windows/git/commit/cedfcd39f5a4e4beb33e16fa67c4659fd4bdabf6
> which is what we carry in Git for Windows.

...and then something similar to 62c23938fa ("tests: add a special setup
where rebase.useBuiltin is off", 2018-11-14) so those of us who're
smoking next for bugs can test both and report if some of the test
setups (odd OS's etc) show a difference in behavior.

I did some of this the last time around, but then I had to e.g. smoke
next against pu, and look at the general fallout there and see what was
due to stash-in-C, it would be much better to have a
GIT_TEST_STASH_USE_BUILTIN.


>> 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: Git pull confusing output

[Ævar Arnfjörð Bjarmason]

On Tue, Nov 27 2018, Will wrote:

> On 27 Nov 2018, at 19:24, Stefan Beller wrote:
>
>> The different phases taking each one line takes up precious
>> screen real estate, so another approach would be delete the line
>> after one phase is finished, such that you'd only see the currently
>> active phase (that can be useful for debugging as in "The phase of
>> 'Writing objects' takes very long" -> slow network connection).
>
> I like this idea
>
 Pushing to github.com:williamdclt/some-repo.git… done
 1ca9aaa..4320d30  master -> master
>>>
>>>
>>> I’d be more than happy to work on this (`git push` is an example
>>> amongst so many other), but want the mailing list’s opinion on it. Am
>>> I wrong in thinking that this output is not something users want, am I
>>> fighting windmills or maybe just being ignorant?
>>
>> I think this would be a useful patch, but it could get complicated
>> quickly: push uses other low level git commands to prepare the
>> packfile to be sent to the server, currently it only needs to pipe
>> through the output of the low level command (or even have the
>> low level command directly write to the terminal).
>>
>> The output of those low level commands should not be changed
>> when run on their own, I would assume.
>
> Agreed. I didn’t expect it to be so subtle, but I’ll look into it
> and see if that’s something within my reach.

It's not *quite* the same topic, but a related WIP patch that got
dropped (and it would be great if someone looking at this area could
pick it up) is
https://public-inbox.org/git/20180902085503.ga25...@sigill.intra.peff.net/

I think it's also worth looking into making the progress code able to
emit stuff like:

Counting / Compressing / Writing objects A% (X_1/Y_2) B% (X_2/Y_2) C% 
(X_3/Y_3)

That would allow for splitting up some of these cases where our progress
bars are overly verbose across multiple lines without losing info by
completely erasing the line, and would also support other cases where
sometimes we do this stuff concurrently. See e.g. my recent commit-graph
progress patches for something that would benefit from this type of
thing.

It would need a patch to the progress code to make it able to juggle N
number of progress bars with some format to stitch them all together.

[PATCH 3/5] t/README: re-flow a paragraph

[Ævar Arnfjörð Bjarmason]

An earlier change changed this paragraph to make the first line quite
short as to produce a more minimal diff. Let's re-flow it. There's no
changes here if diffed with --word-diff.

Signed-off-by: Ævar Arnfjörð Bjarmason 
---
 t/README | 9 -
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/t/README b/t/README
index 3139f4330a..c03b268813 100644
--- a/t/README
+++ b/t/README
@@ -216,11 +216,10 @@ Or tests matching a glob:
 
 $ GIT_SKIP_TESTS='t[0-4]??? t91?? t9200.8' make
 
-The value of the environment variable is a
-SP separated list of patterns that tells which tests to skip,
-and either can match the "t[0-9]{4}" part to skip the whole
-test, or t[0-9]{4} followed by ".$number" to say which
-particular test to skip.
+The value of the environment variable is a SP separated list of
+patterns that tells which tests to skip, and either can match the
+"t[0-9]{4}" part to skip the whole test, or t[0-9]{4} followed by
+".$number" to say which particular test to skip.
 
 For an individual test suite --run could be used to specify that
 only some tests should be run or that some tests should be
-- 
2.20.0.rc1.379.g1dd7ef354c

[PATCH 2/5] t/README: modernize description of GIT_SKIP_TESTS

[Ævar Arnfjörð Bjarmason]

When the GIT_SKIP_TESTS documentation was added in
fbd458a3f6 ("t/README: Add 'Skipping Tests' section below 'Running
Tests'", 2008-06-20) there was no way to declare test prerequisites,
that came later in a7bb394037 ("test-lib: Infrastructure to test and
check for prerequisites", 2009-03-01).

The docs were newer updated, and have been saying that you might want
to use GIT_SKIP_TESTS for a use-case which we'd never use them for,
skipping tests because 'unzip' isn't there. For that we'd use the
UNZIP prerequisite added in 552a26c8c0 ("Use prerequisites to skip
tests that need unzip", 2009-03-16). Fix the docs accordingly.

Signed-off-by: Ævar Arnfjörð Bjarmason 
---
 t/README | 15 ---
 1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/t/README b/t/README
index b6ec28f634..3139f4330a 100644
--- a/t/README
+++ b/t/README
@@ -202,20 +202,21 @@ GIT_TEST_EXEC_PATH defaults to `$GIT_TEST_INSTALLED/git 
--exec-path`.
 Skipping Tests
 --
 
-In some environments, certain tests have no way of succeeding
-due to platform limitation, such as lack of 'unzip' program, or
-filesystem that do not allow arbitrary sequence of non-NUL bytes
-as pathnames.
+Certain tests may fail intermittently or entirely. These should
+ideally be reported as bugs and fixed, or guarded by a prerequisite
+(see "Using test prerequisites" below). But until then they can be
+skipped.
 
-You should be able to say something like
+To skip tests, set the GIT_SKIP_TESTS variable. Individual tests can
+be skipped:
 
 $ GIT_SKIP_TESTS=t9200.8 sh ./t9200-git-cvsexport-commit.sh
 
-and even:
+Or tests matching a glob:
 
 $ GIT_SKIP_TESTS='t[0-4]??? t91?? t9200.8' make
 
-to omit such tests.  The value of the environment variable is a
+The value of the environment variable is a
 SP separated list of patterns that tells which tests to skip,
 and either can match the "t[0-9]{4}" part to skip the whole
 test, or t[0-9]{4} followed by ".$number" to say which
-- 
2.20.0.rc1.379.g1dd7ef354c

[PATCH 5/5] test-lib: add support for GIT_TODO_TESTS

[Ævar Arnfjörð Bjarmason]

As noted in the updated t/README documentation being added here
setting this new GIT_TODO_TESTS variable is usually better than
GIT_SKIP_TESTS.

My use-case for this is to get feedback from the CI infrastructure[1]
about which tests are passing due to fixes that have trickled into
git.git.

With the GIT_SKIP_TESTS variable this use-case is painful, you need to
do an occasional manual run without GIT_SKIP_TESTS set. It's much
better to use GIT_TODO_TESTS and get a report of passing TODO tests
from prove(1) at the bottom of the test output. Once those passing
TODO tests have trickled down to 'master' the relevant glob (set for
all of master/next/pu) can be removed.

As seen in the "GIT_TODO_TESTS mixed failure" test the lack of
interaction with existing tests marked as TODO by the test suite
itself is intentional. There's no need to print out something saying
they matched GIT_TODO_TESTS if they're already TODO tests.

1. https://public-inbox.org/git/875zwm15k2@evledraar.gmail.com/

Signed-off-by: Ævar Arnfjörð Bjarmason 
---
 ci/lib-travisci.sh |  2 +-
 t/README   | 18 +--
 t/t-basic.sh   | 81 +++---
 t/test-lib.sh  | 31 +++---
 4 files changed, 112 insertions(+), 20 deletions(-)

diff --git a/ci/lib-travisci.sh b/ci/lib-travisci.sh
index 69dff4d1ec..ad8290bfdb 100755
--- a/ci/lib-travisci.sh
+++ b/ci/lib-travisci.sh
@@ -121,7 +121,7 @@ osx-clang|osx-gcc)
# t9810 occasionally fails on Travis CI OS X
# t9816 occasionally fails with "TAP out of sequence errors" on
# Travis CI OS X
-   export GIT_SKIP_TESTS="t9810 t9816"
+   export GIT_TODO_TESTS="t9810 t9816"
;;
 GIT_TEST_GETTEXT_POISON)
export GIT_TEST_GETTEXT_POISON=YesPlease
diff --git a/t/README b/t/README
index c03b268813..922b4fb3bf 100644
--- a/t/README
+++ b/t/README
@@ -207,8 +207,19 @@ ideally be reported as bugs and fixed, or guarded by a 
prerequisite
 (see "Using test prerequisites" below). But until then they can be
 skipped.
 
-To skip tests, set the GIT_SKIP_TESTS variable. Individual tests can
-be skipped:
+To skip tests, set either the GIT_SKIP_TESTS or GIT_TODO_TESTS
+variables. The difference is that with SKIP the tests won't be run at
+all, whereas they will be run with TODO, but in success or failure
+annotated as a TODO test.
+
+It's usually preferrable to use TODO, since the test suite will report
+those tests that unexpectedly succeed, which may indicate that
+whatever bug broke the test in the past has been fixed, and the test
+should be un-TODO'd. There's no such feedback loop with
+GIT_SKIP_TESTS.
+
+The GIT_SKIP_TESTS and GIT_TODO_TESTS take the same values. Individual
+tests can be skipped:
 
 $ GIT_SKIP_TESTS=t9200.8 sh ./t9200-git-cvsexport-commit.sh
 
@@ -223,7 +234,8 @@ patterns that tells which tests to skip, and either can 
match the
 
 For an individual test suite --run could be used to specify that
 only some tests should be run or that some tests should be
-excluded from a run.
+excluded from a run. The --run option is a shorthand for setting
+a GIT_SKIP_TESTS pattern.
 
 The argument for --run is a list of individual test numbers or
 ranges with an optional negation prefix that define what tests in
diff --git a/t/t-basic.sh b/t/t-basic.sh
index b87a8f18c2..34c9c5c2a3 100755
--- a/t/t-basic.sh
+++ b/t/t-basic.sh
@@ -324,9 +324,10 @@ test_expect_success 'test --verbose-only' '
EOF
 '
 
-test_expect_success 'GIT_SKIP_TESTS' "
+test_expect_success 'GIT_{SKIP,TODO}_TESTS' "
(
GIT_SKIP_TESTS='git.2' && export GIT_SKIP_TESTS &&
+   GIT_TODO_TESTS='git.3' && export GIT_TODO_TESTS &&
run_sub_test_lib_test git-skip-tests-basic \
'GIT_SKIP_TESTS' <<-\\EOF &&
for i in 1 2 3
@@ -338,16 +339,17 @@ test_expect_success 'GIT_SKIP_TESTS' "
check_sub_test_lib_test git-skip-tests-basic <<-\\EOF
> ok 1 - passing test #1
> ok 2 # skip passing test #2 (GIT_SKIP_TESTS)
-   > ok 3 - passing test #3
-   > # passed all 3 test(s)
+   > ok 3 - passing test #3 # TODO (GIT_TODO_TESTS)
+   > # passed all 3 test(s) (including 1/0 ok/failing TODO test(s))
> 1..3
EOF
)
 "
 
-test_expect_success 'GIT_SKIP_TESTS several tests' "
+test_expect_success 'GIT_{SKIP,TODO}_TESTS several tests' "
(
GIT_SKIP_TESTS='git.2 git.5' && export GIT_SKIP_TESTS &&
+   GIT_TODO_TESTS='git.3 git.6' && export GIT_TODO_TESTS &&
run_sub_test_lib_test git-skip-tests-several \
'GIT_SKIP_TESTS several tests' <<-\\EOF &&
for i in 1 2 3 4 5 6
@@ -359,22 +361,25 @@ test_expect_success 'GIT_SKIP_TESTS several tests' "
check_sub_test_lib_test git-skip-tests-several <<-\\EOF
> ok 1 - 

[PATCH 1/5] t/README: make the 'Skipping tests' section less confusing

[Ævar Arnfjörð Bjarmason]

I added this section in b5500d16cd ("t/README: Add a section about
skipping tests", 2010-07-02), but apparently didn't notice that there
was an existing "Skipping Tests" section added in
fbd458a3f6 ("t/README: Add 'Skipping Tests' section below 'Running
Tests'", 2008-06-20).

Then since 20873f45e7 ("t/README: Document the do's and don'ts of
tests", 2010-07-02) and 0445e6f0a1 ("test-lib: '--run' to run only
specific tests", 2014-04-30) we've started to refer to "Skipping
tests" or "Skipping Tests" sections in prose elsewhere.

Let's clean up this confusion by renaming the section, and while we're
at it improve the example. Usually we don't use the PERL prerequisite,
and we should accurately describe why you'd want to use prerequisites,
as opposed to GIT_SKIP_TESTS. So let's say "Tests that depend[...]"
instead of "If you need to skip tests[...]" here.

Signed-off-by: Ævar Arnfjörð Bjarmason 
---
 t/README | 22 +++---
 1 file changed, 11 insertions(+), 11 deletions(-)

diff --git a/t/README b/t/README
index 28711cc508..b6ec28f634 100644
--- a/t/README
+++ b/t/README
@@ -494,7 +494,7 @@ And here are the "don'ts:"
 
The harness will catch this as a programming error of the test.
Use test_done instead if you need to stop the tests early (see
-   "Skipping tests" below).
+   "Using test prerequisites" below).
 
  - Don't use '! git cmd' when you want to make sure the git command
exits with failure in a controlled way by calling "die()".  Instead,
@@ -587,28 +587,28 @@ And here are the "don'ts:"
it'll complain if anything is amiss.
 
 
-Skipping tests
---
+Using test prerequisites
+
 
-If you need to skip tests you should do so by using the three-arg form
-of the test_* functions (see the "Test harness library" section
-below), e.g.:
+Tests that depend on something which may not be present on the system
+should use the three-arg form of the test_* functions (see the "Test
+harness library" section below), e.g.:
 
-test_expect_success PERL 'I need Perl' '
-perl -e "hlagh() if unf_unf()"
+test_expect_success SYMLINKS 'setup' '
+ln -s target link
 '
 
 The advantage of skipping tests like this is that platforms that don't
-have the PERL and other optional dependencies get an indication of how
+have the SYMLINKS and other optional dependencies get an indication of how
 many tests they're missing.
 
 If the test code is too hairy for that (i.e. does a lot of setup work
 outside test assertions) you can also skip all remaining tests by
 setting skip_all and immediately call test_done:
 
-   if ! test_have_prereq PERL
+   if ! test_have_prereq SYMLINKS
then
-   skip_all='skipping perl interface tests, perl not available'
+   skip_all="skipping symlink tests, the filesystem doesn't support it"
test_done
fi
 
-- 
2.20.0.rc1.379.g1dd7ef354c

[PATCH 4/5] test-lib: add more exhaustive GIT_SKIP_TESTS testing

[Ævar Arnfjörð Bjarmason]

Add a test for the when GIT_SKIP_TESTS is used to skip entire test
files. Support for this was added back in 04ece59399 ("GIT_SKIP_TESTS:
allow users to omit tests that are known to break", 2006-12-28), but
never tested for.

Signed-off-by: Ævar Arnfjörð Bjarmason 
---
 t/t-basic.sh | 17 +
 1 file changed, 17 insertions(+)

diff --git a/t/t-basic.sh b/t/t-basic.sh
index b6566003dd..b87a8f18c2 100755
--- a/t/t-basic.sh
+++ b/t/t-basic.sh
@@ -393,6 +393,23 @@ test_expect_success 'GIT_SKIP_TESTS sh pattern' "
)
 "
 
+test_expect_success 'GIT_SKIP_TESTS entire file' "
+   (
+   GIT_SKIP_TESTS='git' && export GIT_SKIP_TESTS &&
+   run_sub_test_lib_test git-skip-tests-entire-file \
+   'GIT_SKIP_TESTS' <<-\\EOF &&
+   for i in 1 2 3
+   do
+   test_expect_success \"passing test #\$i\" 'true'
+   done
+   test_done
+   EOF
+   check_sub_test_lib_test git-skip-tests-entire-file <<-\\EOF
+   1..0 # SKIP skip all tests in git
+   EOF
+   )
+"
+
 test_expect_success '--run basic' "
run_sub_test_lib_test run-basic \
'--run basic' --run='1 3 5' <<-\\EOF &&
-- 
2.20.0.rc1.379.g1dd7ef354c

Re: Git pull confusing output

[Will]

On 27 Nov 2018, at 19:24, Stefan Beller wrote:

> The different phases taking each one line takes up precious
> screen real estate, so another approach would be delete the line
> after one phase is finished, such that you'd only see the currently
> active phase (that can be useful for debugging as in "The phase of
> 'Writing objects' takes very long" -> slow network connection).

I like this idea

>>> Pushing to github.com:williamdclt/some-repo.git… done
>>> 1ca9aaa..4320d30  master -> master
>>
>>
>> I’d be more than happy to work on this (`git push` is an example
>> amongst so many other), but want the mailing list’s opinion on it. Am
>> I wrong in thinking that this output is not something users want, am I
>> fighting windmills or maybe just being ignorant?
>
> I think this would be a useful patch, but it could get complicated
> quickly: push uses other low level git commands to prepare the
> packfile to be sent to the server, currently it only needs to pipe
> through the output of the low level command (or even have the
> low level command directly write to the terminal).
>
> The output of those low level commands should not be changed
> when run on their own, I would assume.

Agreed. I didn’t expect it to be so subtle, but I’ll look into it
and see if that’s something within my reach.

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

[Thomas Gummerer]

On 11/27, Johannes Schindelin wrote:
> Hi,
> 
> On Sun, 25 Nov 2018, Thomas Gummerer wrote:
> 
> > 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.
> 
> I guess I was too overzealous in my copying. These conditions really
> indicate bugs in the caller... So I'd actually rather change that die() to
> BUG().
> 
> But then, the original code in strbuf_vaddf() calls die() and would have
> to be changed, too.

Right, making these 'BUG()' makes sense to me.  But at this stage of
the series it's probably better to just aim for consistency with the
surrounding code without starting to do more cleanups that were not
included in earlier iterations.  I think that's best left for patches
on top.

> > > + 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.
> 
> It is not actually the beginning of the buffer, but possibly the middle of
> the buffer ;-)

Oops, you're right of course :) 

> > 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).
> 
> :-)
> 
> It seems that Junio likes this way of reviewing, giving him confidence
> that the review was thorough.
>
> Thanks!
> Dscho
> 
> > > +}
> > > +
> > > +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 

Forcing GC to always fail

[Bryan Turner]

Something of an odd question, but is there something I can do in the
configuration for a repository that forces any "git gc" run in that
repository to always fail without doing anything? (Ideally I'd like to
make "git reflog expire" _also_ fail.)

Background: For Bitbucket Server, we have a fairly recurrent issue
where administrators decide they know how to manage garbage collection
for our repositories better than we do, so they jump on the server and
start running things like this:

git reflog expire --expire=now –all
git gc --prune=now
git repack -adf --window=200 --depth=200

They then come running to us with their corrupted repository expecting
and/or hoping that we can fix it (often without proper backups).

Bitbucket Server itself never runs "git gc" (or "git reflog expire").
We've configured how reflog expiry should be handled, but of course
that's overridden by explicit command line options like
"--expire=now". We _do_ run "git pack-refs", "git repack" and "git
prune" (with various options), so those commands need to continue to
work.

Is there anything I can set, perhaps some invalid configuration
option/value, that will make "git gc" (most important) and "git
reflog" (ideal, but less important) fail when they're run in our
repositories? Hopefully at that point customers will reach out to us
for help _before_ they corrupt their repositories.

Bryan

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

[Phillip Wood]

Hi Stefan

On 26/11/2018 21:20, Stefan Beller wrote:

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.


I was using the new --range-diff option to format-patch, I think I 
should have given --range-diff=@{u}...



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.


Yes, but I think with format-patch it should only diff the notes when 
--notes is given.



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.


That's good, thanks for your comments on the previous iterations.

Best Wishes

Phillip

Thanks,
Stefan

Re: [PATCH 8/9] sha1-file: use loose object cache for quick existence check

[René Scharfe]

Am 12.11.2018 um 15:54 schrieb Jeff King:
> diff --git a/sha1-file.c b/sha1-file.c
> index 4aae716a37..e53da0b701 100644
> --- a/sha1-file.c
> +++ b/sha1-file.c
> @@ -921,6 +921,24 @@ static int open_sha1_file(struct repository *r,
>   return -1;
>  }
>  
> +static int quick_has_loose(struct repository *r,
> +const unsigned char *sha1)
> +{
> + int subdir_nr = sha1[0];
> + struct object_id oid;
> + struct object_directory *odb;
> +
> + hashcpy(oid.hash, sha1);
> +
> + prepare_alt_odb(r);
> + for (odb = r->objects->odb; odb; odb = odb->next) {
> + odb_load_loose_cache(odb, subdir_nr);

Is this thread-safe?  What happens if e.g. one index-pack thread resizes
the array while another one sorts it?

Loading the cache explicitly up-front would avoid that, and improves
performance a bit in my (very limited) tests on an SSD.  Demo patch for
next at the bottom.  How does it do against your test cases?

> + if (oid_array_lookup(>loose_objects_cache, ) >= 0)
> + return 1;
> + }
> + return 0;
> +}
> +
>  /*
>   * Map the loose object at "path" if it is not NULL, or the path found by
>   * searching for a loose object named "sha1".
> @@ -1171,6 +1189,8 @@ static int sha1_loose_object_info(struct repository *r,
>   if (!oi->typep && !oi->type_name && !oi->sizep && !oi->contentp) {
>   const char *path;
>   struct stat st;
> + if (!oi->disk_sizep && (flags & OBJECT_INFO_QUICK))
> + return quick_has_loose(r, sha1) ? 0 : -1;
>   if (stat_sha1_file(r, sha1, , ) < 0)
>   return -1;
>   if (oi->disk_sizep)
> 

 builtin/fetch.c  |  2 ++
 builtin/index-pack.c |  2 ++
 fetch-pack.c |  2 ++
 object-store.h   |  1 +
 sha1-file.c  | 30 +++---
 5 files changed, 34 insertions(+), 3 deletions(-)

diff --git a/builtin/fetch.c b/builtin/fetch.c
index e0140327aa..4b031f5da5 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -301,6 +301,8 @@ static void find_non_local_tags(const struct ref *refs,
refname_hash_init(_refs);
refname_hash_init(_refs);
 
+   repo_load_loose_cache(the_repository);
+
for_each_ref(add_one_refname, _refs);
for (ref = refs; ref; ref = ref->next) {
if (!starts_with(ref->name, "refs/tags/"))
diff --git a/builtin/index-pack.c b/builtin/index-pack.c
index ac1f4ea9a7..7fc6321c77 100644
--- a/builtin/index-pack.c
+++ b/builtin/index-pack.c
@@ -1772,6 +1772,8 @@ int cmd_index_pack(int argc, const char **argv, const 
char *prefix)
if (show_stat)
obj_stat = xcalloc(st_add(nr_objects, 1), sizeof(struct 
object_stat));
ofs_deltas = xcalloc(nr_objects, sizeof(struct ofs_delta_entry));
+   if (startup_info->have_repository)
+   repo_load_loose_cache(the_repository);
parse_pack_objects(pack_hash);
if (report_end_of_input)
write_in_full(2, "\0", 1);
diff --git a/fetch-pack.c b/fetch-pack.c
index dd6700bda9..96c4624d9e 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -656,6 +656,8 @@ static void mark_complete_and_common_ref(struct 
fetch_negotiator *negotiator,
 
save_commit_buffer = 0;
 
+   repo_load_loose_cache(the_repository);
+
for (ref = *refs; ref; ref = ref->next) {
struct object *o;
 
diff --git a/object-store.h b/object-store.h
index 8dceed0f31..f98dd3c857 100644
--- a/object-store.h
+++ b/object-store.h
@@ -53,6 +53,7 @@ void add_to_alternates_memory(const char *dir);
  * from 0 to 255 inclusive).
  */
 void odb_load_loose_cache(struct object_directory *odb, int subdir_nr);
+void repo_load_loose_cache(struct repository *r);
 
 struct packed_git {
struct packed_git *next;
diff --git a/sha1-file.c b/sha1-file.c
index 05f63dfd4e..ae12f0a198 100644
--- a/sha1-file.c
+++ b/sha1-file.c
@@ -921,10 +921,19 @@ static int open_sha1_file(struct repository *r,
return -1;
 }
 
+static int quick_has_loose_odb(struct object_directory *odb,
+  const struct object_id *oid)
+{
+   int subdir_nr = oid->hash[0];
+
+   if (odb->loose_objects_subdir_seen[subdir_nr])
+   return oid_array_lookup(>loose_objects_cache, oid) >= 0;
+   return check_and_freshen_odb(odb, oid, 0);
+}
+
 static int quick_has_loose(struct repository *r,
   const unsigned char *sha1)
 {
-   int subdir_nr = sha1[0];
struct object_id oid;
struct object_directory *odb;
 
@@ -932,8 +941,7 @@ static int quick_has_loose(struct repository *r,
 
prepare_alt_odb(r);
for (odb = r->objects->odb; odb; odb = odb->next) {
-   odb_load_loose_cache(odb, subdir_nr);
-   if (oid_array_lookup(>loose_objects_cache, ) >= 0)
+   if (quick_has_loose_odb(odb, ))
return 1;
}

git difftool --dir-diff not saving new files copied from LOCAL to REMOTE

[Moon, John]

Hi all,

I’m using vim DirDiff plugin to compare 2 branches.  This works great for the 
most part except:

When I compare a branch to the current branch (git difftool -t vimDirDiff 
--dir-diff master)

If there is a file that exists in $LOCAL that is not in $REMOTE I copy the file 
into $REMOTE, but when I exit the difftool the file is not in the current 
branch.  I would expect the new file to be there so I can add it.

Is there a way to make it sync the new files in the temp folder with the 
current branch?  Or even to use the current directory for $REMOTE

Thanks. 

[PATCH] advice: don't pointlessly suggest --convert-graft-file

[Ævar Arnfjörð Bjarmason]

The advice to run 'git replace --convert-graft-file' added in
f9f99b3f7d ("Deprecate support for .git/info/grafts", 2018-04-29)
didn't add an exception for the 'git replace --convert-graft-file'
codepath itself.

As a result we'd suggest running --convert-graft-file while the user
was running --convert-graft-file, which makes no sense. Before:

$ git replace --convert-graft-file
hint: Support for /info/grafts is deprecated
hint: and will be removed in a future Git version.
hint:
hint: Please use "git replace --convert-graft-file"
hint: to convert the grafts into replace refs.
hint:
hint: Turn this message off by running
hint: "git config advice.graftFileDeprecated false"

Add a check for that case and skip printing the advice while the user
is busy following our advice.

Signed-off-by: Ævar Arnfjörð Bjarmason 
---
 builtin/replace.c  | 1 +
 t/t6050-replace.sh | 5 -
 2 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/builtin/replace.c b/builtin/replace.c
index a58b9c6d13..affcdfb416 100644
--- a/builtin/replace.c
+++ b/builtin/replace.c
@@ -495,6 +495,7 @@ static int convert_graft_file(int force)
if (!fp)
return -1;
 
+   advice_graft_file_deprecated = 0;
while (strbuf_getline(, fp) != EOF) {
if (*buf.buf == '#')
continue;
diff --git a/t/t6050-replace.sh b/t/t6050-replace.sh
index 86374a9c52..5d6d3184ac 100755
--- a/t/t6050-replace.sh
+++ b/t/t6050-replace.sh
@@ -461,7 +461,10 @@ test_expect_success '--convert-graft-file' '
printf "%s\n%s %s\n\n# comment\n%s\n" \
$(git rev-parse HEAD^^ HEAD^ HEAD^^ HEAD^2) \
>.git/info/grafts &&
-   git replace --convert-graft-file &&
+   git status 2>stderr &&
+   test_i18ngrep "hint:.*grafts is deprecated" stderr &&
+   git replace --convert-graft-file 2>stderr &&
+   test_i18ngrep ! "hint:.*grafts is deprecated" stderr &&
test_path_is_missing .git/info/grafts &&
 
: verify that the history is now "grafted" &&
-- 
2.20.0.rc1.379.g1dd7ef354c

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

[Achim Gratz]

Junio C Hamano writes:
> I wonder if it makes the rest of the code simpler if we stripped
> things like /cygdrive/c here exactly the sam way as we strip C:
> For that, has_dos_drive_prefix() needs to know /cygdrive/[a-z],
> which may not be a bad thing, I guess.  Let's read on.

The cygdrive prefix can be configured by the user to something
arbitrarily different, so if you're hoping to simplify the string
handling this way you'll most likely be disappointed.  It is exactly
that fact that led to the introduction of /proc/cygdrive as an
alternative prefix which doesn't depend on any configuration.


Regards,
Achim.
-- 
+<[Q+ Matrix-12 WAVE#46+305 Neuron microQkb Andromeda XTk Blofeld]>+

Factory and User Sound Singles for Waldorf Q+, Q and microQ:
http://Synth.Stromeko.net/Downloads.html#WaldorfSounds

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 27.11.18 um 19:15 schrieb Johannes Sixt:
> Am 27.11.18 um 00:31 schrieb Junio C Hamano:
>> Johannes Sixt  writes:
>>> Am 26.11.18 um 04:04 schrieb Junio C Hamano:
>>> ... 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.
>
> I wouldn't want that to happen for all output (context lines, - lines,
> + lines): I really am not interested to see all the CRs in my CRLF files.
>
>> 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.
>
> But we already have it, as Brian pointed out:
>
>    git diff --ws-error-highlight=old,new
>
> or by setting diff.wsErrorHighlight accordingly.
... but that also turns on displaying normal space/tab errors in removed
lines.
So to make both of us happy, we would need separate switches.

Any chance this can be done easily enough ?

Regards,
Frank

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

[Achim Gratz]

tbo...@web.de writes:
> The solution is to implement has_dos_drive_prefix(), skip_dos_drive_prefix()
> is_dir_sep(), offset_1st_component() and convert_slashes() for cygwin
> in the same way as it is done in 'Git for Windows' in compat/mingw.[ch]

Please use the Cygwin API path conversion functions for C code and the
cygpath program for shell code instead of trying to re-implement your
own handling (which is prone to introduce subtle bugs or at least
different heuristics from what cygwin itself uses).

https://cygwin.com/cygwin-api/cygwin-functions.html#func-cygwin-path


Regards,
Achim.
-- 
+<[Q+ Matrix-12 WAVE#46+305 Neuron microQkb Andromeda XTk Blofeld]>+

SD adaptation for Waldorf microQ V2.22R2:
http://Synth.Stromeko.net/Downloads.html#WaldorfSDada

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 27.11.18 um 00:31 schrieb 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.

Hmm... I think I now understand what caused the confusion here.
It was my mistake: I didn't consider that EOL characters are whitespace
characters, too. :/

Nevertheless, I still think that eol (CR, LF) and "normal" whitespace
(space, tab) should be distinguished and marked/displayed differently,
because they are playing different roles.
Your suggestion seems to be a good solution for that.

Regards,
Frank

[PATCH v1] mem_pool: add GIT_TRACE_MEMPOOL support

[Ben Peart]

From: Ben Peart 

Add tracing around initializing and discarding mempools. In discard report
on the amount of memory unused in the current block to help tune setting
the initial_size.

Signed-off-by: Ben Peart 
---

Notes:
Base Ref: * git-trace-mempool
Web-Diff: https://github.com/benpeart/git/commit/9ac84bbca2
Checkout: git fetch https://github.com/benpeart/git git-trace-mempool-v1 && 
git checkout 9ac84bbca2

 mem-pool.c | 5 +
 1 file changed, 5 insertions(+)

diff --git a/mem-pool.c b/mem-pool.c
index a2841a4a9a..065389aaec 100644
--- a/mem-pool.c
+++ b/mem-pool.c
@@ -5,6 +5,7 @@
 #include "cache.h"
 #include "mem-pool.h"
 
+static struct trace_key trace_mem_pool = TRACE_KEY_INIT(MEMPOOL);
 #define BLOCK_GROWTH_SIZE 1024*1024 - sizeof(struct mp_block);
 
 /*
@@ -48,12 +49,16 @@ void mem_pool_init(struct mem_pool **mem_pool, size_t 
initial_size)
mem_pool_alloc_block(pool, initial_size, NULL);
 
*mem_pool = pool;
+   trace_printf_key(_mem_pool, "mem_pool (%p): init (%"PRIuMAX") 
initial size\n",
+   pool, (uintmax_t)initial_size);
 }
 
 void mem_pool_discard(struct mem_pool *mem_pool, int invalidate_memory)
 {
struct mp_block *block, *block_to_free;
 
+   trace_printf_key(_mem_pool, "mem_pool (%p): discard (%"PRIuMAX") 
unused\n",
+   mem_pool, (uintmax_t)(mem_pool->mp_block->end - 
mem_pool->mp_block->next_free));
block = mem_pool->mp_block;
while (block)
{

base-commit: bb75be6cb916297f271c846f2f9caa3daaaec718
-- 
2.18.0.windows.1

Re: [PATCH v2 4/7] checkout: move dwim_new_local_branch to checkout_opts

[Stefan Beller]

On Tue, Nov 27, 2018 at 8:53 AM Nguyễn Thái Ngọc Duy  wrote:
>
> Signed-off-by: Nguyễn Thái Ngọc Duy 

I would not mind to have this squashed into the previous patch
but keeping it separated is fine, too.
(Reason for squashing: it makes it clearer that we do not
care about one specific option, but have to treat all the loose
options the same way.)

[PATCH v1] teach git to support a virtual (partially populated) work directory

[Ben Peart]

From: Ben Peart 

To make git perform well on the very largest repos, we must make git
operations O(modified) instead of O(size of repo).  This takes advantage of
the fact that the number of files a developer has modified (especially
in very large repos) is typically a tiny fraction of the overall repo size.

We accomplished this by utilizing the existing internal logic for the skip
worktree bit and excludes to tell git to ignore all files and folders other
than those that have been modified.  This logic is driven by an external
process that monitors writes to the repo and communicates the list of files
and folders with changes to git via the virtual work directory hook in this
patch.

The external process maintains a list of files and folders that have been
modified.  When git runs, it requests the list of files and folders that
have been modified via the virtual work directory hook.  Git then sets/clears
the skip-worktree bit on the cache entries and builds a hashmap of the
modified files/folders that is used by the excludes logic to avoid scanning
the entire repo looking for changes and untracked files.

With this system, we have been able to make local git command performance on
extremely large repos (millions of files, 1/2 million folders) entirely
manageable (30 second checkout, 3.5 seconds status, 4 second add, 7 second
commit, etc).

On index load, clear/set the skip worktree bits based on the virtual
work directory data. Use virtual work directory data to update skip-worktree
bit in unpack-trees. Use virtual work directory data to exclude files and
folders not explicitly requested.

Signed-off-by: Ben Peart 
---

I believe I've incorporated all the feedback from the RFC.  Renaming the
feature, updating the setting to be a boolean with a hard coded hook name,
labeling the feature "experimental," and only calling get_dtype() if the
feature is turned on.

If there are other suggestions on how to ensure this is a useful and general
purpose feature please let me know.

Notes:
Base Ref: master
Web-Diff: https://github.com/benpeart/git/commit/65c3ca2e5f
Checkout: git fetch https://github.com/benpeart/git virtual-workdir-v1 && 
git checkout 65c3ca2e5f

 Documentation/config/core.txt |   9 +
 Documentation/githooks.txt|  23 ++
 Makefile  |   1 +
 cache.h   |   1 +
 config.c  |  32 ++-
 config.h  |   1 +
 dir.c |  26 ++-
 environment.c |   1 +
 read-cache.c  |   2 +
 t/t1092-virtualworkdir.sh | 393 ++
 unpack-trees.c|  23 +-
 virtualworkdir.c  | 314 +++
 virtualworkdir.h  |  25 +++
 13 files changed, 843 insertions(+), 8 deletions(-)
 create mode 100755 t/t1092-virtualworkdir.sh
 create mode 100644 virtualworkdir.c
 create mode 100644 virtualworkdir.h

diff --git a/Documentation/config/core.txt b/Documentation/config/core.txt
index d0e6635fe0..49b7699a4e 100644
--- a/Documentation/config/core.txt
+++ b/Documentation/config/core.txt
@@ -68,6 +68,15 @@ core.fsmonitor::
avoiding unnecessary processing of files that have not changed.
See the "fsmonitor-watchman" section of linkgit:githooks[5].
 
+core.virtualWorkDir::
+   Please regard this as an experimental feature.
+   If set to true, utilize the virtual-work-dir hook to identify all
+   files and directories that are present in the working directory.
+   Git will only track and update files listed in the virtual work
+   directory.  Using the virtual work directory will supersede the
+   sparse-checkout settings which will be ignored.
+   See the "virtual-work-dir" section of linkgit:githooks[6].
+
 core.trustctime::
If false, the ctime differences between the index and the
working tree are ignored; useful when the inode change time
diff --git a/Documentation/githooks.txt b/Documentation/githooks.txt
index 959044347e..9888d504b4 100644
--- a/Documentation/githooks.txt
+++ b/Documentation/githooks.txt
@@ -485,6 +485,29 @@ The exit status determines whether git will use the data 
from the
 hook to limit its search.  On error, it will fall back to verifying
 all files and folders.
 
+virtual-work-dir
+
+
+Please regard this as an experimental feature.
+
+The "Virtual Work Directory" hook allows populating the working directory
+sparsely. The virtual work directory data is typically automatically
+generated by an external process.  Git will limit what files it checks for
+changes as well as which directories are checked for untracked files based
+on the path names given. Git will also only update those files listed in the
+virtual work directory.
+
+The hook is invoked when the configuration option core.virtualWorkDir is
+set to true.  The hook takes one argument, a version (currently 1).
+
+The hook should output to stdout the list of 

Re: [PATCH v2 3/7] checkout: move 'confict_style' to checkout_opts

[Stefan Beller]

On Tue, Nov 27, 2018 at 8:53 AM Nguyễn Thái Ngọc Duy  wrote:
>

The last patches seemed self explanatory after the first RFC
and their commit message. This one is harder to reason about,
as --conflict is documented as "The same as --merge option
above, but ..." and --merge is "When switching branches, ..."
so ... ah my mind wandered off, expecting this to be a preparation
for the separate commands already, but this is just about ensuring
everything is in opts such that the split can be done later?

Re: [PATCH v2 1/7] parse-options: allow parse_options_concat(NULL, options)

[Stefan Beller]

On Tue, Nov 27, 2018 at 8:53 AM Nguyễn Thái Ngọc Duy  wrote:
>
> There is currently no caller that calls this function with "a" being
> NULL. But it will be introduced shortly. It is used to construct the
> option array from scratch, e.g.
>
>struct parse_options opts = NULL;
>opts = parse_options_concat(opts, opts_1);
>opts = parse_options_concat(opts, opts_2);

While this addresses the immediate needs, I'd prefer to think
about the API exposure of parse_options_concat,
(related: do we want to have docs in its header file?)
and I'd recommend to make it symmetrical, i.e.
allow the second argument to also be NULL?

In the example given here, you'd just short it to

struct parse_options opts = opts_1;
opts = parse_options_concat(opts, opts_2);

if not for this patch. Are opts_{1,2} ever be NULL?

Re: [PATCH] tests: avoid syntax triggering old dash bug

[Ævar Arnfjörð Bjarmason]

On Tue, Nov 27 2018, Eric Sunshine wrote:

> On Tue, Nov 27, 2018 at 11:43 AM Ævar Arnfjörð Bjarmason
>  wrote:
>> Avoid a bug in dash that's been fixed ever since its
>> ec2c84d ("[PARSER] Fix clobbering of checkkwd", 2011-03-15)[1] first
>> released with dash v0.5.7 in July 2011.
>
> Perhaps enhance the commit message to explain the nature of the bug
> itself. It is not at all obvious from reading the above or from
> looking at the diff itself what the actual problem is that the patch
> is fixing. (And it wasn't even immediately obvious by looking at the
> commit message of ec2c84d in the dash repository.) To help readers of
> this patch avoid re-introducing this problem or diagnose such a
> failure, it might be a good idea to give an example of the syntax
> which trips up old dash (i.e. a here-doc followed immediately by a
> {...} expression) and the actual error message 'Syntax error: "}"
> unexpected'.

I haven't taken the time to understand the bug either. Our entire test
suite had one instance of this, so I think it's obscure enough that it's
fine to just fix it as a one-off and not spend any more time on making
sure it doesn't happen again or add some lint for detecting it.

>> This fixes 1/2 tests failing on Debian Lenny & Squeeze. The other
>> failure is due to 1b42f45255 ("git-svn: apply "svn.pathnameencoding"
>> before URL encoding", 2016-02-09).
>>
>> This particular test has been failing since 5f9674243d ("config: add
>> --expiry-date", 2017-11-18).
>>
>> Signed-off-by: Ævar Arnfjörð Bjarmason 

Re: [PATCH] tests: avoid syntax triggering old dash bug

[Eric Sunshine]

On Tue, Nov 27, 2018 at 11:43 AM Ævar Arnfjörð Bjarmason
 wrote:
> Avoid a bug in dash that's been fixed ever since its
> ec2c84d ("[PARSER] Fix clobbering of checkkwd", 2011-03-15)[1] first
> released with dash v0.5.7 in July 2011.

Perhaps enhance the commit message to explain the nature of the bug
itself. It is not at all obvious from reading the above or from
looking at the diff itself what the actual problem is that the patch
is fixing. (And it wasn't even immediately obvious by looking at the
commit message of ec2c84d in the dash repository.) To help readers of
this patch avoid re-introducing this problem or diagnose such a
failure, it might be a good idea to give an example of the syntax
which trips up old dash (i.e. a here-doc followed immediately by a
{...} expression) and the actual error message 'Syntax error: "}"
unexpected'.

Thanks.

> This fixes 1/2 tests failing on Debian Lenny & Squeeze. The other
> failure is due to 1b42f45255 ("git-svn: apply "svn.pathnameencoding"
> before URL encoding", 2016-02-09).
>
> This particular test has been failing since 5f9674243d ("config: add
> --expiry-date", 2017-11-18).
>
> Signed-off-by: Ævar Arnfjörð Bjarmason 

Re: Git pull confusing output

[Stefan Beller]

On Tue, Nov 27, 2018 at 8:52 AM Will  wrote:

> And even them, do they need this info every time they push?

I agree that we should make the output a bit more user friendly,
which means we'd only want to output relevant data for the user.

The different phases taking each one line takes up precious
screen real estate, so another approach would be delete the line
after one phase is finished, such that you'd only see the currently
active phase (that can be useful for debugging as in "The phase of
'Writing objects' takes very long" -> slow network connection).

> I feel like a less intimidating output would help, while showing info
> about objects and deltas with the verbose flag:

I agree that most information in pushing is not very useful
and could be omitted. This helps in multiple ways:
* it keeps the focus on the actually important information,
   see bf1a11f0a1 (sideband: highlight keywords in remote
   sideband output, 2018-08-07)
* less space in a terminal wasted, such that you can scroll over
   it better

> > Compressing… done

After the push succeeded this information would not be useful
any more, it is only useful during the compression phase
(Does it progress quickly enough? or does it error out?)

Slightly related (but applies mostly to fetch, for which this
discussion can also be had):
When fetching, these informations are generated on the
remote side (as the server needs to create the packfile
according to your local state that you negotiated with the
server), which takes some time. Sending over this
information also keeps the connection alive. This is only
relevant in corner cases depending on the setup of the
hosting provider/repository, but it led to commits such as
https://eclipse.googlesource.com/jgit/jgit/+/a38531b21c7e2b0dc956e0ed1bfc9513f604273c
in the java implementation of Git.

> > Pushing to github.com:williamdclt/some-repo.git… done
> > 1ca9aaa..4320d30  master -> master
>
>
> I’d be more than happy to work on this (`git push` is an example
> amongst so many other), but want the mailing list’s opinion on it. Am
> I wrong in thinking that this output is not something users want, am I
> fighting windmills or maybe just being ignorant?

I think this would be a useful patch, but it could get complicated
quickly: push uses other low level git commands to prepare the
packfile to be sent to the server, currently it only needs to pipe
through the output of the low level command (or even have the
low level command directly write to the terminal).

The output of those low level commands should not be changed
when run on their own, I would assume.

So maybe the best way to dive into understanding what happens
under the hood in git-push is to run

  GIT_TRACE=1 git push ...

and see what child processes are invoked (e.g.
run_command: git pack-objects --all-progress-implied)
and then we'd need to change the output of iff the
specific progress flag is given.

Stefan

Re: [PATCH] config.mak.dev: enable -Wpedantic in clang

[Eric Sunshine]

On Tue, Nov 27, 2018 at 10:48 AM Carlo Arenas  wrote:
> On Tue, Nov 27, 2018 at 2:53 AM Eric Sunshine  wrote:
> > On Tue, Nov 27, 2018 at 5:06 AM Carlo Marcelo Arenas Belón
> > > +ifneq ($(filter clang10,$(COMPILER_FEATURES)),)
> > > +CFLAGS += -Wpedantic
> > > +endif
> >
> > Should this condition be tightened to match only for OSX since there
> > is no such clang version number in the rest world outside of Apple?
>
> instead, I changed it to clang4 and tested it in a FreeBSD 11 box I
> was able to get a hold off, would that work better?
> will update patch accordingly then

Playing Devi's Advocate, what if Apple's clang "8" was, in reality,
real-world clang 3? Then this condition would incorrectly enable the
compiler option on Apple for a (real) clang version below 4. For this
reason, it seems we shouldn't be trusting only the clang version
number when dealing with Apple.

(I suspect that it won't make a difference in actual practice, so it
may be reasonable to punt on this issue until/if someone complains.)

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 27.11.18 um 00:31 schrieb Junio C Hamano:

Johannes Sixt  writes:

Am 26.11.18 um 04:04 schrieb Junio C Hamano:
... 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.


I wouldn't want that to happen for all output (context lines, - lines, + 
lines): I really am not interested to see all the CRs in my CRLF files.



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.


But we already have it, as Brian pointed out:

   git diff --ws-error-highlight=old,new

or by setting diff.wsErrorHighlight accordingly.

-- Hannes

[PATCH v2 3/7] checkout: move 'confict_style' to checkout_opts

[Nguyễn Thái Ngọc Duy]

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 builtin/checkout.c | 8 
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/builtin/checkout.c b/builtin/checkout.c
index 31245c1eb4..211a347a0c 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -55,6 +55,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;
@@ -1228,7 +1229,6 @@ int cmd_checkout(int argc, const char **argv, const char 
*prefix)
struct checkout_opts real_opts;
struct checkout_opts *opts = _opts;
struct branch_info new_branch_info;
-   char *conflict_style = NULL;
int dwim_new_local_branch = 1;
int dwim_remotes_matched = 0;
struct option options[] = {
@@ -1254,7 +1254,7 @@ int cmd_checkout(int argc, const char **argv, const char 
*prefix)
OPT_BOOL_F(0, "overwrite-ignore", >overwrite_ignore,
   N_("update ignored files (default)"),
   PARSE_OPT_NOCOMPLETE),
-   OPT_STRING(0, "conflict", _style, N_("style"),
+   OPT_STRING(0, "conflict", >conflict_style, N_("style"),
   N_("conflict style (merge or diff3)")),
OPT_BOOL('p', "patch", >patch_mode, N_("select hunks 
interactively")),
OPT_BOOL(0, "ignore-skip-worktree-bits", 
>ignore_skipworktree,
@@ -1290,9 +1290,9 @@ int cmd_checkout(int argc, const char **argv, const char 
*prefix)
opts->show_progress = isatty(2);
}
 
-   if (conflict_style) {
+   if (opts->conflict_style) {
opts->merge = 1; /* implied */
-   git_xmerge_config("merge.conflictstyle", conflict_style, NULL);
+   git_xmerge_config("merge.conflictstyle", opts->conflict_style, 
NULL);
}
 
if ((!!opts->new_branch + !!opts->new_branch_force + 
!!opts->new_orphan_branch) > 1)
-- 
2.19.1.1327.g328c130451.dirty

[PATCH v2 6/7] checkout: split into switch-branch and checkout-files

[Nguyễn Thái Ngọc Duy]

"git checkout" doing too many things is a source of confusion for many
users (and it even bites old timers sometimes). To rememdy that, the
command is now split in two: switch-branch and checkout-files.

The switch-branch command is all about switching branches, detaching,
DWIM-ing new branch... It does not accept pathspecs and it always
requires a ref (in contrast, "git checkout" without arguments works)

The checkout-files command on the other hand is all about resetting
certain files in worktree, either from the index or from a specific
tree. It could accept a tree-ish, but it will never touch HEAD or the
ref it points to.

The good old "git checkout" command is still here and will be until
all (or most of users) are sick of it.

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 builtin.h  |  2 +
 builtin/checkout.c | 91 +++---
 git.c  |  2 +
 3 files changed, 82 insertions(+), 13 deletions(-)

diff --git a/builtin.h b/builtin.h
index 6538932e99..d4a66e5f79 100644
--- a/builtin.h
+++ b/builtin.h
@@ -138,6 +138,7 @@ extern int cmd_branch(int argc, const char **argv, const 
char *prefix);
 extern int cmd_bundle(int argc, const char **argv, const char *prefix);
 extern int cmd_cat_file(int argc, const char **argv, const char *prefix);
 extern int cmd_checkout(int argc, const char **argv, const char *prefix);
+extern int cmd_checkout_files(int argc, const char **argv, const char *prefix);
 extern int cmd_checkout_index(int argc, const char **argv, const char *prefix);
 extern int cmd_check_attr(int argc, const char **argv, const char *prefix);
 extern int cmd_check_ignore(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 d9dbd2d40d..c09d2da47a 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 checkout_files_usage[] = {
+   N_("git checkout-files [] [] -- ..."),
+   NULL,
+};
+
 struct checkout_opts {
int patch_mode;
int quiet;
@@ -45,6 +55,8 @@ struct checkout_opts {
int ignore_other_worktrees;
int show_progress;
int dwim_new_local_branch;
+   int accept_pathspec;
+   int empty_arg_ok;
 
/*
 * If new checkout options are added, skip_merge_working_tree
@@ -1056,7 +1068,7 @@ static int parse_branchname_arg(int argc, const char 
**argv,
arg = argv[0];
dash_dash_pos = -1;
for (i = 0; i < argc; i++) {
-   if (!strcmp(argv[i], "--")) {
+   if (opts->accept_pathspec && !strcmp(argv[i], "--")) {
dash_dash_pos = i;
break;
}
@@ -1067,6 +1079,8 @@ static int parse_branchname_arg(int argc, const char 
**argv,
has_dash_dash = 1; /* case (3) or (1) */
else if (dash_dash_pos >= 2)
die(_("only one reference expected, %d given."), dash_dash_pos);
+   else if (!opts->accept_pathspec)
+   has_dash_dash = 1;
 
if (!strcmp(arg, "-"))
arg = "@{-1}";
@@ -1291,30 +1305,23 @@ static struct option *add_checkout_path_options(struct 
checkout_opts *opts,
return newopts;
 }
 
-int cmd_checkout(int argc, const char **argv, const char *prefix)
+static int checkout_main(int argc, const char **argv, const char *prefix,
+struct checkout_opts *opts, struct option *options,
+const char * const usagestr[])
 {
-   struct checkout_opts real_opts;
-   struct checkout_opts *opts = _opts;
struct branch_info new_branch_info;
int dwim_remotes_matched = 0;
-   struct option *options = NULL;
 
-   memset(opts, 0, sizeof(*opts));
memset(_branch_info, 0, sizeof(new_branch_info));
opts->overwrite_ignore = 1;
opts->prefix = prefix;
opts->show_progress = -1;
-   opts->dwim_new_local_branch = 1;
 
git_config(git_checkout_config, opts);
 
opts->track = BRANCH_TRACK_UNSPECIFIED;
 
-   options = add_common_options(opts, options);
-   options = add_switch_branch_options(opts, options);
-   

[PATCH v2 5/7] checkout: split options[] array in three pieces

[Nguyễn Thái Ngọc Duy]

This is a preparation step for introducing new commands that do parts
of what checkout does. There will be two new commands, one is about
switching branches, detaching HEAD... one about checking out
paths. These share the a subset of command line options. The rest of
command line options are separate.

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 builtin/checkout.c | 80 --
 1 file changed, 56 insertions(+), 24 deletions(-)

diff --git a/builtin/checkout.c b/builtin/checkout.c
index a50c51f287..d9dbd2d40d 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -1226,14 +1226,32 @@ static int checkout_branch(struct checkout_opts *opts,
return switch_branches(opts, new_branch_info);
 }
 
-int cmd_checkout(int argc, const char **argv, const char *prefix)
+static struct option *add_common_options(struct checkout_opts *opts,
+struct option *prevopts)
 {
-   struct checkout_opts real_opts;
-   struct checkout_opts *opts = _opts;
-   struct branch_info new_branch_info;
-   int dwim_remotes_matched = 0;
struct option options[] = {
OPT__QUIET(>quiet, N_("suppress progress reporting")),
+   OPT_BOOL(0, "ignore-skip-worktree-bits", 
>ignore_skipworktree,
+N_("do not limit pathspecs to sparse entries only")),
+   { OPTION_CALLBACK, 0, "recurse-submodules", NULL,
+   "checkout", "control recursive updating of 
submodules",
+   PARSE_OPT_OPTARG, 
option_parse_recurse_submodules_worktree_updater },
+   OPT_BOOL(0, "progress", >show_progress, N_("force 
progress reporting")),
+   OPT__FORCE(>force, N_("force checkout (throw away local 
modifications)"),
+  PARSE_OPT_NOCOMPLETE),
+   OPT_STRING(0, "conflict", >conflict_style, N_("style"),
+  N_("conflict style (merge or diff3)")),
+   OPT_END()
+   };
+   struct option *newopts = parse_options_concat(prevopts, options);
+   free(prevopts);
+   return newopts;
+}
+
+static struct option *add_switch_branch_options(struct checkout_opts *opts,
+   struct option *prevopts)
+{
+   struct option options[] = {
OPT_STRING('b', NULL, >new_branch, N_("branch"),
   N_("create and checkout a new branch")),
OPT_STRING('B', NULL, >new_branch_force, N_("branch"),
@@ -1243,33 +1261,43 @@ int cmd_checkout(int argc, const char **argv, const 
char *prefix)
OPT_SET_INT('t', "track",  >track, N_("set upstream info 
for new branch"),
BRANCH_TRACK_EXPLICIT),
OPT_STRING(0, "orphan", >new_orphan_branch, 
N_("new-branch"), N_("new unparented branch")),
+   OPT_BOOL('m', "merge", >merge, N_("perform a 3-way merge 
with the new branch")),
+   OPT_HIDDEN_BOOL(0, "guess", >dwim_new_local_branch,
+   N_("second guess 'git checkout 
'")),
+   OPT_BOOL(0, "ignore-other-worktrees", 
>ignore_other_worktrees,
+N_("do not check if another worktree is holding the 
given ref")),
+   OPT_END()
+   };
+   struct option *newopts = parse_options_concat(prevopts, options);
+   free(prevopts);
+   return newopts;
+}
+
+static struct option *add_checkout_path_options(struct checkout_opts *opts,
+   struct option *prevopts)
+{
+   struct option options[] = {
OPT_SET_INT_F('2', "ours", >writeout_stage,
  N_("checkout our version for unmerged files"),
  2, PARSE_OPT_NONEG),
OPT_SET_INT_F('3', "theirs", >writeout_stage,
  N_("checkout their version for unmerged files"),
  3, PARSE_OPT_NONEG),
-   OPT__FORCE(>force, N_("force checkout (throw away local 
modifications)"),
-  PARSE_OPT_NOCOMPLETE),
-   OPT_BOOL('m', "merge", >merge, N_("perform a 3-way merge 
with the new branch")),
-   OPT_BOOL_F(0, "overwrite-ignore", >overwrite_ignore,
-  N_("update ignored files (default)"),
-  PARSE_OPT_NOCOMPLETE),
-   OPT_STRING(0, "conflict", >conflict_style, N_("style"),
-  N_("conflict style (merge or diff3)")),
OPT_BOOL('p', "patch", >patch_mode, N_("select hunks 
interactively")),
-   OPT_BOOL(0, "ignore-skip-worktree-bits", 
>ignore_skipworktree,
-N_("do not limit pathspecs to sparse entries only")),
-   OPT_HIDDEN_BOOL(0, "guess", >dwim_new_local_branch,
-   N_("second guess 'git checkout 
'")),
-

[PATCH v2 2/7] checkout: make "opts" in cmd_checkout() a pointer

[Nguyễn Thái Ngọc Duy]

"opts" will soon be moved out of cmd_checkout(). To keep changes in
that patch smaller, convert "opts" to a pointer and keep the real
thing behind "real_opts".

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 builtin/checkout.c | 109 +++--
 1 file changed, 55 insertions(+), 54 deletions(-)

diff --git a/builtin/checkout.c b/builtin/checkout.c
index acdafc6e4c..31245c1eb4 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -1225,76 +1225,77 @@ static int checkout_branch(struct checkout_opts *opts,
 
 int cmd_checkout(int argc, const char **argv, const char *prefix)
 {
-   struct checkout_opts opts;
+   struct checkout_opts real_opts;
+   struct checkout_opts *opts = _opts;
struct branch_info new_branch_info;
char *conflict_style = NULL;
int dwim_new_local_branch = 1;
int dwim_remotes_matched = 0;
struct option options[] = {
-   OPT__QUIET(, N_("suppress progress reporting")),
-   OPT_STRING('b', NULL, _branch, N_("branch"),
+   OPT__QUIET(>quiet, N_("suppress progress reporting")),
+   OPT_STRING('b', NULL, >new_branch, N_("branch"),
   N_("create and checkout a new branch")),
-   OPT_STRING('B', NULL, _branch_force, N_("branch"),
+   OPT_STRING('B', NULL, >new_branch_force, N_("branch"),
   N_("create/reset and checkout a branch")),
-   OPT_BOOL('l', NULL, _branch_log, N_("create reflog for 
new branch")),
-   OPT_BOOL(0, "detach", _detach, N_("detach HEAD at 
named commit")),
-   OPT_SET_INT('t', "track",  , N_("set upstream info 
for new branch"),
+   OPT_BOOL('l', NULL, >new_branch_log, N_("create reflog 
for new branch")),
+   OPT_BOOL(0, "detach", >force_detach, N_("detach HEAD at 
named commit")),
+   OPT_SET_INT('t', "track",  >track, N_("set upstream info 
for new branch"),
BRANCH_TRACK_EXPLICIT),
-   OPT_STRING(0, "orphan", _orphan_branch, 
N_("new-branch"), N_("new unparented branch")),
-   OPT_SET_INT_F('2', "ours", _stage,
+   OPT_STRING(0, "orphan", >new_orphan_branch, 
N_("new-branch"), N_("new unparented branch")),
+   OPT_SET_INT_F('2', "ours", >writeout_stage,
  N_("checkout our version for unmerged files"),
  2, PARSE_OPT_NONEG),
-   OPT_SET_INT_F('3', "theirs", _stage,
+   OPT_SET_INT_F('3', "theirs", >writeout_stage,
  N_("checkout their version for unmerged files"),
  3, PARSE_OPT_NONEG),
-   OPT__FORCE(, N_("force checkout (throw away local 
modifications)"),
+   OPT__FORCE(>force, N_("force checkout (throw away local 
modifications)"),
   PARSE_OPT_NOCOMPLETE),
-   OPT_BOOL('m', "merge", , N_("perform a 3-way merge 
with the new branch")),
-   OPT_BOOL_F(0, "overwrite-ignore", _ignore,
+   OPT_BOOL('m', "merge", >merge, N_("perform a 3-way merge 
with the new branch")),
+   OPT_BOOL_F(0, "overwrite-ignore", >overwrite_ignore,
   N_("update ignored files (default)"),
   PARSE_OPT_NOCOMPLETE),
OPT_STRING(0, "conflict", _style, N_("style"),
   N_("conflict style (merge or diff3)")),
-   OPT_BOOL('p', "patch", _mode, N_("select hunks 
interactively")),
-   OPT_BOOL(0, "ignore-skip-worktree-bits", 
_skipworktree,
+   OPT_BOOL('p', "patch", >patch_mode, N_("select hunks 
interactively")),
+   OPT_BOOL(0, "ignore-skip-worktree-bits", 
>ignore_skipworktree,
 N_("do not limit pathspecs to sparse entries only")),
OPT_HIDDEN_BOOL(0, "guess", _new_local_branch,
N_("second guess 'git checkout 
'")),
-   OPT_BOOL(0, "ignore-other-worktrees", 
_other_worktrees,
+   OPT_BOOL(0, "ignore-other-worktrees", 
>ignore_other_worktrees,
 N_("do not check if another worktree is holding the 
given ref")),
{ OPTION_CALLBACK, 0, "recurse-submodules", NULL,
"checkout", "control recursive updating of 
submodules",
PARSE_OPT_OPTARG, 
option_parse_recurse_submodules_worktree_updater },
-   OPT_BOOL(0, "progress", _progress, N_("force progress 
reporting")),
+   OPT_BOOL(0, "progress", >show_progress, N_("force 
progress reporting")),
OPT_END(),
};
 
-   memset(, 0, sizeof(opts));
+   memset(opts, 0, sizeof(*opts));
memset(_branch_info, 0, sizeof(new_branch_info));
-   opts.overwrite_ignore = 1;
-   opts.prefix = prefix;
-   

[PATCH v2 1/7] parse-options: allow parse_options_concat(NULL, options)

[Nguyễn Thái Ngọc Duy]

There is currently no caller that calls this function with "a" being
NULL. But it will be introduced shortly. It is used to construct the
option array from scratch, e.g.

   struct parse_options opts = NULL;
   opts = parse_options_concat(opts, opts_1);
   opts = parse_options_concat(opts, opts_2);

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 parse-options-cb.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/parse-options-cb.c b/parse-options-cb.c
index 8c9edce52f..c609d52926 100644
--- a/parse-options-cb.c
+++ b/parse-options-cb.c
@@ -126,7 +126,7 @@ struct option *parse_options_concat(struct option *a, 
struct option *b)
struct option *ret;
size_t i, a_len = 0, b_len = 0;
 
-   for (i = 0; a[i].type != OPTION_END; i++)
+   for (i = 0; a && a[i].type != OPTION_END; i++)
a_len++;
for (i = 0; b[i].type != OPTION_END; i++)
b_len++;
-- 
2.19.1.1327.g328c130451.dirty

[PATCH/RFC v2 0/7] Introduce new commands switch-branch and checkout-files

[Nguyễn Thái Ngọc Duy]

v2 is just a bit better to look at than v1. This is by no means final.
If you think the command name is bad, the default behavior should
change, or something else, speak up. It's still very "RFC".

v2 breaks down the giant patch in v1 and starts adding some changes in
these new commands:

- restore-paths is renamed to checkout-paths. I wrote I didn't like
  "checkout" because of completion conflict. But who am I kidding,
  I'll use aliases anyway. "-files" instead of "-paths" because we
  already have ls-files.
- both commands will not accept no arguments. There is no "git
  checkout" equivalent.
- ambiguation rules are now aware that "switch-branch" for example
  can't take pathspec...
- the last patch tries to hide "git checkout" away. The command
  example updates show how these will be used. Which probably helps
  figure better names or defaults for them too

One thing I notice that we often use "git checkout -- " and
rarely "git checkout  -- ". Which makes me think
perhaps "git checkout-files" should not use "--" to separate the two.
We'll have this instead

git checkout-files [--from=] 


Oh and of course I'll be waiting for the new --index from Thomas
before submitting submitting any thing serious for 'next'. We still
have plenty of time.

Nguyễn Thái Ngọc Duy (7):
  parse-options: allow parse_options_concat(NULL, options)
  checkout: make "opts" in cmd_checkout() a pointer
  checkout: move 'confict_style' to checkout_opts
  checkout: move dwim_new_local_branch to checkout_opts
  checkout: split options[] array in three pieces
  checkout: split into switch-branch and checkout-files
  Suggest other commands instead of "git checkout"

 Documentation/git-branch.txt   |   8 +-
 Documentation/git-check-ref-format.txt |   2 +-
 Documentation/git-format-patch.txt |   2 +-
 Documentation/git-merge-base.txt   |   2 +-
 Documentation/git-rebase.txt   |   2 +-
 Documentation/git-remote.txt   |   2 +-
 Documentation/git-rerere.txt   |  10 +-
 Documentation/git-reset.txt|  18 +-
 Documentation/git-revert.txt   |   2 +-
 Documentation/git-stash.txt|   6 +-
 Documentation/gitattributes.txt|   2 +-
 Documentation/gitcli.txt   |   4 +-
 Documentation/gitcore-tutorial.txt |  18 +-
 Documentation/giteveryday.txt  |  24 +--
 Documentation/githooks.txt |   5 +-
 Documentation/gittutorial-2.txt|   2 +-
 Documentation/gittutorial.txt  |   4 +-
 Documentation/revisions.txt|   2 +-
 Documentation/user-manual.txt  |  54 +++---
 advice.c   |   2 +-
 builtin.h  |   2 +
 builtin/checkout.c | 256 +
 git.c  |   2 +
 parse-options-cb.c |   2 +-
 sha1-name.c|   2 +-
 wt-status.c|   2 +-
 26 files changed, 271 insertions(+), 166 deletions(-)

-- 
2.19.1.1327.g328c130451.dirty

[PATCH v2 7/7] Suggest other commands instead of "git checkout"

[Nguyễn Thái Ngọc Duy]

The assumption made is here

- "git checkout" is a horrible monster that should only be touched
  with a two-meter pole

- there are other commands that can achieve the same thing

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 Documentation/git-branch.txt   |  8 ++--
 Documentation/git-check-ref-format.txt |  2 +-
 Documentation/git-format-patch.txt |  2 +-
 Documentation/git-merge-base.txt   |  2 +-
 Documentation/git-rebase.txt   |  2 +-
 Documentation/git-remote.txt   |  2 +-
 Documentation/git-rerere.txt   | 10 ++---
 Documentation/git-reset.txt| 18 -
 Documentation/git-revert.txt   |  2 +-
 Documentation/git-stash.txt|  6 +--
 Documentation/gitattributes.txt|  2 +-
 Documentation/gitcli.txt   |  4 +-
 Documentation/gitcore-tutorial.txt | 18 -
 Documentation/giteveryday.txt  | 24 ++--
 Documentation/githooks.txt |  5 ++-
 Documentation/gittutorial-2.txt|  2 +-
 Documentation/gittutorial.txt  |  4 +-
 Documentation/revisions.txt|  2 +-
 Documentation/user-manual.txt  | 54 +-
 advice.c   |  2 +-
 sha1-name.c|  2 +-
 wt-status.c|  2 +-
 22 files changed, 88 insertions(+), 87 deletions(-)

diff --git a/Documentation/git-branch.txt b/Documentation/git-branch.txt
index bf5316ffa9..1564df47d2 100644
--- a/Documentation/git-branch.txt
+++ b/Documentation/git-branch.txt
@@ -48,7 +48,7 @@ The command's second form creates a new branch head named 

 which points to the current `HEAD`, or  if given.
 
 Note that this will create the new branch, but it will not switch the
-working tree to it; use "git checkout " to switch to the
+working tree to it; use "git switch-branch " to switch to the
 new branch.
 
 When a local branch is started off a remote-tracking branch, Git sets up the
@@ -194,7 +194,7 @@ This option is only applicable in non-verbose mode.
 +
 This behavior is the default when the start point is a remote-tracking branch.
 Set the branch.autoSetupMerge configuration variable to `false` if you
-want `git checkout` and `git branch` to always behave as if `--no-track`
+want `git switch-branch` and `git branch` to always behave as if `--no-track`
 were given. Set it to `always` if you want this behavior when the
 start-point is either a local or remote-tracking branch.
 
@@ -293,7 +293,7 @@ Start development from a known tag::
 $ git clone git://git.kernel.org/pub/scm/.../linux-2.6 my2.6
 $ cd my2.6
 $ git branch my2.6.14 v2.6.14   <1>
-$ git checkout my2.6.14
+$ git switch-branch my2.6.14
 
 +
 <1> This step and the next one could be combined into a single step with
@@ -319,7 +319,7 @@ NOTES
 -
 
 If you are creating a branch that you want to checkout immediately, it is
-easier to use the git checkout command with its `-b` option to create
+easier to use the "git switch-branch" command with its `-b` option to create
 a branch and check it out with a single command.
 
 The options `--contains`, `--no-contains`, `--merged` and `--no-merged`
diff --git a/Documentation/git-check-ref-format.txt 
b/Documentation/git-check-ref-format.txt
index d9de992585..38c2169d7a 100644
--- a/Documentation/git-check-ref-format.txt
+++ b/Documentation/git-check-ref-format.txt
@@ -88,7 +88,7 @@ but it is explicitly forbidden at the beginning of a branch 
name).
 When run with `--branch` option in a repository, the input is first
 expanded for the ``previous checkout syntax''
 `@{-n}`.  For example, `@{-1}` is a way to refer the last thing that
-was checked out using "git checkout" operation. This option should be
+was checked out using "git switch-branch" operation. This option should be
 used by porcelains to accept this syntax anywhere a branch name is
 expected, so they can act as if you typed the branch name. As an
 exception note that, the ``previous checkout operation'' might result
diff --git a/Documentation/git-format-patch.txt 
b/Documentation/git-format-patch.txt
index aba4c5febe..0ceaa1173c 100644
--- a/Documentation/git-format-patch.txt
+++ b/Documentation/git-format-patch.txt
@@ -416,7 +416,7 @@ One way to test if your MUA is set up correctly is:
 * Apply it:
 
 $ git fetch  master:test-apply
-$ git checkout test-apply
+$ git switch-branch test-apply
 $ git reset --hard
 $ git am a.patch
 
diff --git a/Documentation/git-merge-base.txt b/Documentation/git-merge-base.txt
index 9f07f4f6ed..1b25e5d530 100644
--- a/Documentation/git-merge-base.txt
+++ b/Documentation/git-merge-base.txt
@@ -149,7 +149,7 @@ instead.
 Discussion on fork-point mode
 -
 
-After working on the `topic` branch created with `git checkout -b
+After working on the `topic` branch created with `git switch-branch -b
 topic origin/master`, the history of remote-tracking branch
 `origin/master` may have been 

[PATCH v2 4/7] checkout: move dwim_new_local_branch to checkout_opts

[Nguyễn Thái Ngọc Duy]

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 builtin/checkout.c | 8 +---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/builtin/checkout.c b/builtin/checkout.c
index 211a347a0c..a50c51f287 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -44,6 +44,8 @@ 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.
@@ -1229,7 +1231,6 @@ int cmd_checkout(int argc, const char **argv, const char 
*prefix)
struct checkout_opts real_opts;
struct checkout_opts *opts = _opts;
struct branch_info new_branch_info;
-   int dwim_new_local_branch = 1;
int dwim_remotes_matched = 0;
struct option options[] = {
OPT__QUIET(>quiet, N_("suppress progress reporting")),
@@ -1259,7 +1260,7 @@ int cmd_checkout(int argc, const char **argv, const char 
*prefix)
OPT_BOOL('p', "patch", >patch_mode, N_("select hunks 
interactively")),
OPT_BOOL(0, "ignore-skip-worktree-bits", 
>ignore_skipworktree,
 N_("do not limit pathspecs to sparse entries only")),
-   OPT_HIDDEN_BOOL(0, "guess", _new_local_branch,
+   OPT_HIDDEN_BOOL(0, "guess", >dwim_new_local_branch,
N_("second guess 'git checkout 
'")),
OPT_BOOL(0, "ignore-other-worktrees", 
>ignore_other_worktrees,
 N_("do not check if another worktree is holding the 
given ref")),
@@ -1275,6 +1276,7 @@ int cmd_checkout(int argc, const char **argv, const char 
*prefix)
opts->overwrite_ignore = 1;
opts->prefix = prefix;
opts->show_progress = -1;
+   opts->dwim_new_local_branch = 1;
 
git_config(git_checkout_config, opts);
 
@@ -1339,7 +1341,7 @@ int cmd_checkout(int argc, const char **argv, const char 
*prefix)
struct object_id rev;
int dwim_ok =
!opts->patch_mode &&
-   dwim_new_local_branch &&
+   opts->dwim_new_local_branch &&
opts->track == BRANCH_TRACK_UNSPECIFIED &&
!opts->new_branch;
int n = parse_branchname_arg(argc, argv, dwim_ok,
-- 
2.19.1.1327.g328c130451.dirty

Git pull confusing output

[Will]

I’m far from being a guru, but I consider myself a competent Git user. 
Yet, here’s my understanding of the output of one the most-used 
commands, `git push`:

Counting objects: 6, done.
No idea what an “object” is. Apparently there’s 6 of them here. 
What does “counting” them means? Should I care?

Delta compression using up to 4 threads.
No idea what is “delta compression”, I suppose something is being 
compressed. It’s using anything between 1 and 4 threads, which is not 
a very precise or useful information. Should I care?

Compressing objects: 100% (6/6), done.
I still don’t know what objects are, but I appreciate having feedback 
on progress

Writing objects: 100% (6/6), 656 bytes | 656.00 KiB/s, done.

Writing what, where? Should I care? Still good to have feedback

Total 6 (delta 4), reused 0 (delta 0)

No idea what any of those numbers mean. Should I care?

remote: Resolving deltas: 100% (4/4), completed with 4 local objects.
I do know what’s a remote, but I don’t know what “resolving 
deltas” means. There’s local objects now? I don’t understand what 
happened to those local objects, are they the byproduct of the delta 
resolving or the input or something else? Should I care?

To github.com:williamdclt/some-repo.git

Fair enough

1ca9aaa..4320d30  master -> master

Fair enough


All in all, I didn’t understand most of what I’ve been told, and 
don’t seem to care about it. Don’t take my sassiness for disrespect, 
I really appreciate (and am impressed by) everything that happens here, 
but I feel like a less confusing UI is such a low-hanging fruit. How 
many devs understand what all of this means, 1%-2% if even that? And 
even them, do they need this info every time they push?


I feel like a less intimidating output would help, while showing info 
about objects and deltas with the verbose flag:

Compressing… done
Pushing to github.com:williamdclt/some-repo.git… done
1ca9aaa..4320d30  master -> master



I’d be more than happy to work on this (`git push` is an example 
amongst so many other), but want the mailing list’s opinion on it. Am 
I wrong in thinking that this output is not something users want, am I 
fighting windmills or maybe just being ignorant?

[PATCH] tests: avoid syntax triggering old dash bug

[Ævar Arnfjörð Bjarmason]

Avoid a bug in dash that's been fixed ever since its
ec2c84d ("[PARSER] Fix clobbering of checkkwd", 2011-03-15)[1] first
released with dash v0.5.7 in July 2011.

This fixes 1/2 tests failing on Debian Lenny & Squeeze. The other
failure is due to 1b42f45255 ("git-svn: apply "svn.pathnameencoding"
before URL encoding", 2016-02-09).

This particular test has been failing since 5f9674243d ("config: add
--expiry-date", 2017-11-18).

1. https://git.kernel.org/pub/scm/utils/dash/dash.git/

Signed-off-by: Ævar Arnfjörð Bjarmason 
---
 t/t1300-config.sh | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/t/t1300-config.sh b/t/t1300-config.sh
index 9652b241c7..7690b518b8 100755
--- a/t/t1300-config.sh
+++ b/t/t1300-config.sh
@@ -892,8 +892,9 @@ test_expect_success 'get --expiry-date' '
1510348087
0
EOF
+   date_valid1=$(git config --expiry-date date.valid1) &&
{
-   echo "$rel_out $(git config --expiry-date date.valid1)"
+   echo "$rel_out $date_valid1"
git config --expiry-date date.valid2 &&
git config --expiry-date date.valid3 &&
git config --expiry-date date.valid4 &&
-- 
2.20.0.rc1.379.g1dd7ef354c

Re: [PATCH] config.mak.dev: enable -Wpedantic in clang

[Carlo Arenas]

On Tue, Nov 27, 2018 at 2:53 AM Eric Sunshine  wrote:
> On Tue, Nov 27, 2018 at 5:06 AM Carlo Marcelo Arenas Belón
> > +ifneq ($(filter clang10,$(COMPILER_FEATURES)),)
> > +CFLAGS += -Wpedantic
> > +endif
>
> Should this condition be tightened to match only for OSX since there
> is no such clang version number in the rest world outside of Apple?

instead, I changed it to clang4 and tested it in a FreeBSD 11 box I
was able to get a hold off, would that work better?
will update patch accordingly then

Carlo

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

[Duy Nguyen]

On Tue, Nov 27, 2018 at 1:56 PM Jacob Keller  wrote:
>
> On Tue, Nov 27, 2018 at 1:45 AM Per Lundberg  wrote:
> >
> > On 11/26/18 5:55 PM, Duy Nguyen wrote:
> > > On Mon, Nov 26, 2018 at 4:47 PM Ævar Arnfjörð Bjarmason
> > >  wrote:
> > >> 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)
> >
> > Agree - I think it would be a very bad idea to have a "mix" of both
> > trashable and precious. IMO, we should try to find which one of these
> > concepts suits most general use cases best and causes less churn for
> > existing scripts/users' existing "semantic expectations", and pick that one.
> > --
> > Per Lundberg
>
> Personally, I would rather err on the side which requires the least
> interaction from users to avoid silently clobbering an ignored file.
>
> Either Duy's solution with a sort of "untracked" reflog, or the
> garbage/trashable notion.
>
> I don't like the idea of precious because it means people have to know
> and remember to opt in, and it's quite possible they will not do so
> until after they've lost real data.
>
> I'd only have trashable apply in the case where it was implicit. i.e.
> git clean -fdx would still delete them, as this is an explicit
> operation that (hopefully?) users know will delete data.

Yes I know it will delete ignored files. But I don't want it to delete
some files. There is no way I can tell Git to do that.

It's the same with merge/checkout's overwriting problem. Once the
initial surprise is over, I want control over what files I want Git to
just delete and not annoy me, what Git should not delete.
-- 
Duy

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

[Ævar Arnfjörð Bjarmason]

On Mon, Nov 26 2018, Junio C Hamano wrote:

> 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.

The patch I submitted in <87zhuf3gs0@evledraar.gmail.com>[1] changed
the behavior for read-tree & checkout & merge etc.

It was an RFC more in the spirit of showing what in our current tests
had to change to spur some discussion.

But I'm very sympathetic to this line of argument. I.e. in my patch I'm
changing the semantics of read-tree, which is plumbing.

What do you think about some patch like that which retains the plumbing
behavior for things like read-tree, doesn't introduce "precious" or
"trashable", and just makes you specify "[checkout|merge|...] --force"
in cases where we'd have clobbering?

This would give scripts which relied on our stable plumbing consistent
behavior, while helping users who're using our main porcelain not to
lose data. I could then add a --force option to the likes of read-tree
(on by default), so you could get porcelain-like behavior with
--no-force.

1. https://public-inbox.org/git/87zhuf3gs0@evledraar.gmail.com/

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

[Per Lundberg]

On 11/27/18 2:55 PM, Jacob Keller wrote:

> Personally, I would rather err on the side which requires the least
> interaction from users to avoid silently clobbering an ignored file.
> 
 > [...]
> 
> I don't like the idea of precious because it means people have to know
> and remember to opt in, and it's quite possible they will not do so
> until after they've lost real data.

I agree strongly with this personally; if we must choose between "might 
break automation" and "might delete non-garbage files", I would say the 
former is the lesser evil of the two.

But, if I had 10 000 000 servers set up using automated scripts that 
would break because of this, I might think differently. Quite likely so, 
in fact.

What are these automation scenarios _more specifically_? Junio or Brian, 
would you care to elaborate? Is it for build servers where you want "git 
clean -dfx" to always reset the working copy to a pristine state or are 
we talking about some other scenarios?

> I'd only have trashable apply in the case where it was implicit. i.e.
> git clean -fdx would still delete them, as this is an explicit
> operation that (hopefully?) users know will delete data.

This is one of the tougher calls, unfortunately.

If I was a user (which I am), and I was typing "git clean -dfx", what 
would I expect?

The help text (currently) states "-x   remove ignored files, too".

Would it be safe to assume that people would understand that "ignored 
_does not_ mean trashable when doing "git checkout some-ref" BUT it 
_does_ mean trashable in the "git clean -dfx" context"? I'm not so 
certain. It would be one of those perceived inconsistencies that would 
make people scream in anger because they _presumed_ that with the new 
"trashable" concept, "git clean -dfx" would no longer hit them in the leg.

And the other way around: if we change "git clean -dfx" to _not_ treat 
"ignored == trashable", it is likely to "hose automation" as it has been 
previously stated. People who might be using this syntax and _want_ it 
to remove ignored files would be upset, and rightfully so.

So in my POV, it's a tough decision between two, less-than-optimal 
alternatives.

But I would perhaps be able to live with the current semantics for "git 
clean -dfx" _as long as we update the help text_ so that "-x" indicates 
more clearly that non-trashable files can be deleted. It doesn't make 
things _worse_ than they currently are and if this is what it takes to 
get the trashable concept implemented and accepted by the community, 
it's a compromise I'd be willing to make.
--
Per Lundberg

GREAT ILLUMINATI SOCIETY

[ILLUMINATE/BROTHERHOOD]

The Illuminati operates in defense of you and all humans, in all places, and of 
all generations. Our duty to this planet has spanned across centuries and 
survived even the most established government entities. But the cultivation of 
trillions of human lives is a daunting responsibility, and while the human 
would not exist today without our protection, many uninformed masses mistake 
our guidance for a restriction of liberty.

Every human desires to be free of oppression, free of hardship, free of 
poverty, free of hunger, free of rules and laws — but as you understand, the 
nature of your species leaves true freedom impossible.

You may never understand how your life can be free while guided by our 
organization. You may never fully comprehend our purpose and why you are safest 
and happiest with us. Simply open your mind and release your apprehensions, and 
you will find the relief of truth.The truth can be told when its revealed to 
you the secrete of life.

CONTACT EMAIL: i_illumin...@yahoo.com

on how to claim your form for membership of the (GREAT ILLUMINATI SOCIETY) 

GREAT ILLUMINATI SOCIETY

[ILLUMINATE/BROTHERHOOD]

The Illuminati operates in defense of you and all humans, in all places, and of 
all generations. Our duty to this planet has spanned across centuries and 
survived even the most established government entities. But the cultivation of 
trillions of human lives is a daunting responsibility, and while the human 
would not exist today without our protection, many uninformed masses mistake 
our guidance for a restriction of liberty.

Every human desires to be free of oppression, free of hardship, free of 
poverty, free of hunger, free of rules and laws — but as you understand, the 
nature of your species leaves true freedom impossible.

You may never understand how your life can be free while guided by our 
organization. You may never fully comprehend our purpose and why you are safest 
and happiest with us. Simply open your mind and release your apprehensions, and 
you will find the relief of truth.The truth can be told when its revealed to 
you the secrete of life.

CONTACT EMAIL: i_illumin...@yahoo.com

on how to claim your form for membership of the (GREAT ILLUMINATI SOCIETY) 

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

[Johannes Schindelin]

Hi Junio & Paul,

On Mon, 26 Nov 2018, Junio C Hamano wrote:

> 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.

Feel very free to cherry-pick
https://github.com/git-for-windows/git/commit/004da7e7faa36c872868ae938e06594ea1c2f01c
and
https://github.com/git-for-windows/git/commit/cedfcd39f5a4e4beb33e16fa67c4659fd4bdabf6
which is what we carry in Git for Windows.

Ciao,
Dscho

> 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 v11 03/22] strbuf.c: add `strbuf_insertf()` and `strbuf_vinsertf()`

[Johannes Schindelin]

Hi,

On Sun, 25 Nov 2018, Thomas Gummerer wrote:

> 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.

I guess I was too overzealous in my copying. These conditions really
indicate bugs in the caller... So I'd actually rather change that die() to
BUG().

But then, the original code in strbuf_vaddf() calls die() and would have
to be changed, too.

> > +   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.

It is not actually the beginning of the buffer, but possibly the middle 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).

:-)

It seems that Junio likes this way of reviewing, giving him confidence
that the review was thorough.

Thanks!
Dscho

> > +}
> > +
> > +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
> > 
> 

Git for Windows v2.19.2 demoted to "pre-release"

[Johannes Schindelin]

Team,

sorry for the inconvenience, but I had to "pull" Git for Windows v2.19.2
because of two major bugs: 32-bit Git Bash simply failed to start, and all
operations using git:// URLs were broken.

This close to v2.20.0, I was uncomfortable pushing yet another upgrade to
users that would have been made obsolete a mere week from now.

I apologize for the trouble,
Johannes

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

[Johannes Schindelin]

Hi Junio,

On Tue, 27 Nov 2018, Junio C Hamano wrote:

> Steven Penny  writes:
> 
> > If you strip the drive, you can still navigate within the same drive:
> >
> > $ cd 'C:\Users'
> > $ pwd
> > /cygdrive/c/Users
> >
> > $ cd '\Windows'
> > $ pwd
> > /cygdrive/c/Windows
> >
> > but you can no longer traverse drives:
> >
> > $ cd '\Testing'
> > sh: cd: \Testing: No such file or directory
> 
> Sorry, but I fail to see the point the last example wants to make.

I agree. For me, the real test is this:

me@work ~
$ cd /cygdrive

me@work /cygdrive
$ ls
c  d

So `/cygdrive` *is* a valid directory in Cygwin.

> > I would say these could be merged into a "win.h" or similar. Cygwin 
> > typically
> > leans toward the "/unix/style" while MINGW has been more tolerant of
> > "C:\Windows\Style" and "C:/Mixed/Style" paths, i dont see that changing.
> 
> I'd defer to Windows folks to decide if a unified win.h is a good
> idea.

We already have such a thing, but it is not just `win.h`, it is
`compat/win32/`. I would think that the best idea would be to move the
MINGW variants to `compat/win32/path-utils.c` and declare them in
`compat/win32/path-utils.h`, renaming them from `mingw_*()` to
`win32_*()`.

Ciao,
Dscho

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

[Jacob Keller]

On Tue, Nov 27, 2018 at 1:45 AM Per Lundberg  wrote:
>
> On 11/26/18 5:55 PM, Duy Nguyen wrote:
> > On Mon, Nov 26, 2018 at 4:47 PM Ævar Arnfjörð Bjarmason
> >  wrote:
> >> 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)
>
> Agree - I think it would be a very bad idea to have a "mix" of both
> trashable and precious. IMO, we should try to find which one of these
> concepts suits most general use cases best and causes less churn for
> existing scripts/users' existing "semantic expectations", and pick that one.
> --
> Per Lundberg

Personally, I would rather err on the side which requires the least
interaction from users to avoid silently clobbering an ignored file.

Either Duy's solution with a sort of "untracked" reflog, or the
garbage/trashable notion.

I don't like the idea of precious because it means people have to know
and remember to opt in, and it's quite possible they will not do so
until after they've lost real data.

I'd only have trashable apply in the case where it was implicit. i.e.
git clean -fdx would still delete them, as this is an explicit
operation that (hopefully?) users know will delete data.

Thanks,
Jake

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

[Johannes Schindelin]

Hi Torsten,

On Mon, 26 Nov 2018, tbo...@web.de wrote:

> diff --git a/compat/cygwin.c b/compat/cygwin.c
> index b9862d606d..c4a10cb5a1 100644
> --- a/compat/cygwin.c
> +++ b/compat/cygwin.c
> @@ -1,19 +1,29 @@
>  #include "../git-compat-util.h"
>  #include "../cache.h"
>  
> +int cygwin_skip_dos_drive_prefix(char **path)
> +{
> + int ret = has_dos_drive_prefix(*path);
> + *path += ret;
> + return ret;
> +}
> +
>  int cygwin_offset_1st_component(const char *path)
>  {
> - const char *pos = path;
> + char *pos = (char *)path;
> +
>   /* unc paths */
> - if (is_dir_sep(pos[0]) && is_dir_sep(pos[1])) {
> + if (!skip_dos_drive_prefix() &&
> + is_dir_sep(pos[0]) && is_dir_sep(pos[1])) {

It takes a little folding and knotting of the brain to understand that
this `!skip_dos_drive_prefix()` has *nothing* to do with the comment
`unc paths` nor with the test whether the paths starts with two directory
separators.

As a consequence, I would highly suggest to turn this into:

if (skip_dos_drive_prefix())
; /* absolute path with DOS drive prefix */
/* unc paths */
else if (is_dir_sep(pos[0]) && is_dir_sep(pos[1])) {

That makes the code a lot easier to understand, and as a consequence a lot
harder to mess up in the future.

Thanks,
Dscho

>   /* skip server name */
> - pos = strchr(pos + 2, '/');
> + pos = strpbrk(pos + 2, "\\/");
>   if (!pos)
>   return 0; /* Error: malformed unc path */
>  
>   do {
>   pos++;
> - } while (*pos && pos[0] != '/');
> + } while (*pos && !is_dir_sep(*pos));
>   }
> +
>   return pos + is_dir_sep(*pos) - path;
>  }
> diff --git a/compat/cygwin.h b/compat/cygwin.h
> index 8e52de4644..46f29c0a90 100644
> --- a/compat/cygwin.h
> +++ b/compat/cygwin.h
> @@ -1,2 +1,34 @@
> +#define has_dos_drive_prefix(path) \
> + (isalpha(*(path)) && (path)[1] == ':' ? 2 : 0)
> +
> +
> +int cygwin_offset_1st_component(const char *path);
> +#define offset_1st_component cygwin_offset_1st_component
> +
> +
> +#define has_dos_drive_prefix(path) \
> + (isalpha(*(path)) && (path)[1] == ':' ? 2 : 0)
> +int cygwin_skip_dos_drive_prefix(char **path);
> +#define skip_dos_drive_prefix cygwin_skip_dos_drive_prefix
> +static inline int cygwin_is_dir_sep(int c)
> +{
> + return c == '/' || c == '\\';
> +}
> +#define is_dir_sep cygwin_is_dir_sep
> +static inline char *cygwin_find_last_dir_sep(const char *path)
> +{
> + char *ret = NULL;
> + for (; *path; ++path)
> + if (is_dir_sep(*path))
> + ret = (char *)path;
> + return ret;
> +}
> +static inline void convert_slashes(char *path)
> +{
> + for (; *path; path++)
> + if (*path == '\\')
> + *path = '/';
> +}
> +#define find_last_dir_sep cygwin_find_last_dir_sep
>  int cygwin_offset_1st_component(const char *path);
>  #define offset_1st_component cygwin_offset_1st_component
> -- 
> 2.19.0.271.gfe8321ec05
> 
> 

[no subject]

[Roya Moaddel]

We need to make sure everyone's contact information is up to date,
please login with your corporate login and verify the information is correct:
Click here
We need to have this list updated within 24hrs, so please make this a priority.
Thank You

[PATCH 1/1] rebase --stat: fix when rebasing to an unrelated history

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin 

When rebasing to a commit history that has no common commits with the
current branch, there is no merge base. The scripted version of the `git
rebase` command was not prepared for that and spewed out

fatal: ambiguous argument '': unknown revision or path not in
the working tree.

but then continued (due to lack of error checking).

The built-in version of the `git rebase` command blindly translated that
shell script code, assuming that there is no need to test whether there
*was* a merge base, and due to its better error checking, exited with a
fatal error (because it tried to read the object with hash ...
as a tree).

Fix both scripted and built-in versions to output something sensibly,
and add a regression test to keep this working in all eternity.

Reported-by: Ævar Arnfjörð Bjarmason 
Signed-off-by: Johannes Schindelin 
---
 builtin/rebase.c  |  8 +---
 git-legacy-rebase.sh  |  6 --
 t/t3406-rebase-message.sh | 10 ++
 3 files changed, 19 insertions(+), 5 deletions(-)

diff --git a/builtin/rebase.c b/builtin/rebase.c
index 5b3e5baec8..9e4b0b564f 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -1483,7 +1483,8 @@ int cmd_rebase(int argc, const char **argv, const char 
*prefix)
 
if (options.flags & REBASE_VERBOSE)
printf(_("Changes from %s to %s:\n"),
-   oid_to_hex(_base),
+   is_null_oid(_base) ?
+   "(empty)" : oid_to_hex(_base),
oid_to_hex(>object.oid));
 
/* We want color (if set), but no pager */
@@ -1494,8 +1495,9 @@ int cmd_rebase(int argc, const char **argv, const char 
*prefix)
DIFF_FORMAT_SUMMARY | DIFF_FORMAT_DIFFSTAT;
opts.detect_rename = DIFF_DETECT_RENAME;
diff_setup_done();
-   diff_tree_oid(_base, >object.oid,
- "", );
+   diff_tree_oid(is_null_oid(_base) ?
+ the_hash_algo->empty_tree : _base,
+ >object.oid, "", );
diffcore_std();
diff_flush();
}
diff --git a/git-legacy-rebase.sh b/git-legacy-rebase.sh
index b97ffdc9dd..be3b241676 100755
--- a/git-legacy-rebase.sh
+++ b/git-legacy-rebase.sh
@@ -718,10 +718,12 @@ if test -n "$diffstat"
 then
if test -n "$verbose"
then
-   echo "$(eval_gettext "Changes from \$mb to \$onto:")"
+   mb_display="${mb:-(empty)}"
+   echo "$(eval_gettext "Changes from \$mb_display to \$onto:")"
fi
+   mb_tree="${mb:-$(git hash-object -t tree /dev/null)}"
# We want color (if set), but no pager
-   GIT_PAGER='' git diff --stat --summary "$mb" "$onto"
+   GIT_PAGER='' git diff --stat --summary "$mb_tree" "$onto"
 fi
 
 test -n "$interactive_rebase" && run_specific_rebase
diff --git a/t/t3406-rebase-message.sh b/t/t3406-rebase-message.sh
index 38bd876cab..a1ee912118 100755
--- a/t/t3406-rebase-message.sh
+++ b/t/t3406-rebase-message.sh
@@ -91,4 +91,14 @@ test_expect_success 'error out early upon -C or 
--whitespace=' '
test_i18ngrep "Invalid whitespace option" err
 '
 
+test_expect_success 'rebase -i onto unrelated history' '
+   git init unrelated &&
+   test_commit -C unrelated 1 &&
+   git -C unrelated remote add -f origin "$PWD" &&
+   git -C unrelated branch --set-upstream-to=origin/master &&
+   git -C unrelated -c core.editor=true rebase -i -v --stat >actual &&
+   test_i18ngrep "Changes from (empty)" actual &&
+   test_i18ngrep "5 files changed" actual
+'
+
 test_done
-- 
gitgitgadget

[PATCH 0/1] Fix git rebase --stat -i

[Johannes Schindelin via GitGitGadget]

We're really entering obscure territory here, I would say.

To trigger the bug, two things have to come together: the user must have
asked for a diffstat afterwards, and the commits need to have been rebased
onto a completely unrelated commit history (i.e. there must exist no merge
base between the pre-rebase HEAD and the post-rebase HEAD).

Please note that this bug existed already in the scripted rebase, but it was
never detected because the scripted version would not even perform any error
checking.

It will make Junio very happy that I squashed the regression test in to the
patch that fixes it. The reason, however, was not to make Junio happy (I
hope to make him happy by fixing bugs), but simply that an earlier iteration
of test would only fail with the built-in rebase, but not with the scripted
version. The current version would fail with the scripted version, but I'll
save the time to split the patch again now.

Johannes Schindelin (1):
  rebase --stat: fix when rebasing to an unrelated history

 builtin/rebase.c  |  8 +---
 git-legacy-rebase.sh  |  6 --
 t/t3406-rebase-message.sh | 10 ++
 3 files changed, 19 insertions(+), 5 deletions(-)


base-commit: a1598010f775d82b5adf12c29d0f5bc9b41434c6
Published-As: 
https://github.com/gitgitgadget/git/releases/tags/pr-88%2Fdscho%2Frebase-stat-fix-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git 
pr-88/dscho/rebase-stat-fix-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/88
-- 
gitgitgadget

HELLO

[Aisha Gaddafi]

Assalamu alaikum,

I have a business Proposal for you and I need mutual respect, trust,
honesty, transparency, adequate support and assistance, Hope to hear
from you for more details.

Warmest regards
Mrs Aisha Gaddafi

السلام عليكم،

لدي اقتراح عمل لك وأنا بحاجة إلى الاحترام المتبادل والثقة والأمانة
والشفافية والدعم الكافي والمساعدة ، ونأمل أن نسمع منك لمزيد من
التفاصيل.

أحر التحيات
السيدة عائشة القذافي

Re: [PATCH] config.mak.dev: enable -Wpedantic in clang

[Eric Sunshine]

On Tue, Nov 27, 2018 at 5:06 AM Carlo Marcelo Arenas Belón
 wrote:
> [...]
> -Wpedantic is only enabled for clang 10 or higher (only available in macOS
> with latest Xcode) but this restriction should be relaxed further as more
> environments are tested

We know from [1] that the clang version number "10" is an Apple
fiction/invention. Perhaps this commit message can be worded a bit
more carefully to avoid confusing readers who are not clued in to that
fact.

[1]: 
https://public-inbox.org/git/capig+crqxq_dows2dsc1x3tagjjnwig7p4eys4kq+c2piad...@mail.gmail.com/

> Signed-off-by: Carlo Marcelo Arenas Belón 
> ---
> +ifneq ($(filter clang10,$(COMPILER_FEATURES)),)
> +CFLAGS += -Wpedantic
> +endif

Should this condition be tightened to match only for OSX since there
is no such clang version number in the rest world outside of Apple?

[PATCH] config.mak.dev: enable -Wpedantic in clang

[Carlo Marcelo Arenas Belón]

DEVOPTS=pedantic adds -pedantic to the compiler flags, but misses on some
diagnostics when using clang, and that are only enabled with -Wpedantic

46c0eb5843 ("files-backend.c: fix build error on Solaris", 2018-11-25)
fixes an issue that was visible also with gcc but not clang so correct
that with the hope that in the future CI could be used for early detection
of similar issues

-Wpedantic is only enabled for clang 10 or higher (only available in macOS
with latest Xcode) but this restriction should be relaxed further as more
environments are tested

Signed-off-by: Carlo Marcelo Arenas Belón 
---
 config.mak.dev | 11 +++
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/config.mak.dev b/config.mak.dev
index bbeeff44fe..ad25beacd8 100644
--- a/config.mak.dev
+++ b/config.mak.dev
@@ -1,8 +1,15 @@
+ifndef COMPILER_FEATURES
+COMPILER_FEATURES := $(shell ./detect-compiler $(CC))
+endif
+
 ifeq ($(filter no-error,$(DEVOPTS)),)
 CFLAGS += -Werror
 endif
 ifneq ($(filter pedantic,$(DEVOPTS)),)
 CFLAGS += -pedantic
+ifneq ($(filter clang10,$(COMPILER_FEATURES)),)
+CFLAGS += -Wpedantic
+endif
 # don't warn for each N_ use
 CFLAGS += -DUSE_PARENS_AROUND_GETTEXT_N=0
 endif
@@ -16,10 +23,6 @@ CFLAGS += -Wstrict-prototypes
 CFLAGS += -Wunused
 CFLAGS += -Wvla
 
-ifndef COMPILER_FEATURES
-COMPILER_FEATURES := $(shell ./detect-compiler $(CC))
-endif
-
 ifneq ($(filter clang4,$(COMPILER_FEATURES)),)
 CFLAGS += -Wtautological-constant-out-of-range-compare
 endif
-- 
2.20.0.rc1

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

[Per Lundberg]

On 11/26/18 5:55 PM, Duy Nguyen wrote:
> On Mon, Nov 26, 2018 at 4:47 PM Ævar Arnfjörð Bjarmason
>  wrote:
>> 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)

Agree - I think it would be a very bad idea to have a "mix" of both 
trashable and precious. IMO, we should try to find which one of these 
concepts suits most general use cases best and causes less churn for 
existing scripts/users' existing "semantic expectations", and pick that one.
--
Per Lundberg

PLEASE OPEN ATTACH

[Daniel Simba]

From. Mr. Daniel and Brother Emmanuel Simba

Email: danielsimba...@gmail.com

Telephone number: +27 782641230


PLEASE OPEN ATTACH FOR THE PROPOSAL

THANKS AND GOD BLESS YOU
Mr. Daniel and Brother Emmanuel Simba
(FOR THE FAMILY)

From Daniel.docx
Description: MS-Word 2007 document