Re: [PATCH 2/4] Documentation: list all type specifiers in config prose

[Junio C Hamano]

Jeff King  writes:

>> +The type specifier can be either `--int` or `--bool`, to make 'git config'
>> +ensure that the variable(s) are of the given type and convert the value to 
>> the
>> +canonical form (simple decimal number for int, a "true" or "false" string 
>> for
>> +bool, either of for --bool-or-int), or `--path`, which does some path 
>> expansion
>> +(see `--path` below), or `--expiry-date` (see `--expiry-date` below).  If no
>> +type specifier is passed, no checks or transformations are performed on the
>> +value.
>
> Perhaps it's time to switch to a list format for these?

A very sensible suggestion.  The original was already bad enough but
with complete set, it does become quite hard to read.  Perhaps along
the lines of...

A type specifier option can be used to force interpretation of
values and conversion to canonical form.  Currently supported
type specifiers are:

`--int`::
The value is taken as an integer.

`--bool`::
The value is taken as a yes/no value, and are shown as
"true" or "false".

...

Re: [PATCH 1/4] builtin/config: introduce `--default`

[Eric Sunshine]

On Tue, Mar 6, 2018 at 1:52 AM, Jeff King  wrote:
> On Mon, Mar 05, 2018 at 06:17:26PM -0800, Taylor Blau wrote:
>> In an aim to replace:
>>   $ git config --get-color slot [default] [...]
>> with:
>>   $ git config --default default --color slot [...]
>> introduce `--defualt` to behave as if the given default were present and
>> assigned to slot in the case that that slot does not exist.
>
> I think this motivation skips over the beginning part of the story,
> which is why we want "--color --default". :)
>
> IMHO, the reason we want --default is two-fold:
>
>   1. Callers have to handle parsing defaults themselves, like:
>
>foo=$(git config core.foo || echo 1234)
>
>  For an integer, that's not too bad, since you can write "1048576"
>  instead of "1M". For colors, it's abominable, which is why we added
>  "--get-color". But as we add more types that are hard to parse
>  (like --expiry-date), it would be nice for them to get the same
>  defaulting feature without adding --get-expiry-date, etc.
>
>   2. --get-color is a one-off unlike all of the other types. That's bad
>  interface design, but the inconsistency also makes it harder to add
>  features which treat the types uniformly (like, say, a --stdin
>  query mode).
>
> And perhaps minor, but it's also easier to correctly error-check
> --default, since the "foo" example above would do the wrong thing if
> git-config encountered a fatal error.

Thanks. For someone (me) who didn't follow the earlier discussion
closely, this motivating explanation really helps; especially since
the commit message mentions only color, which seems to already allow
for a default value, so it wasn't clear what benefit the new --default
provides.

>> +test_expect_success 'marshals default value as bool-or-int' '
>> + echo "1
>> +true" >expect &&
>> + git config --default 1 --bool-or-int core.foo >actual &&
>> + git config --default true --bool-or-int core.foo >>actual &&
>> + test_cmp expect actual
>> +'
>
> Funny indentation. Use:
>
>   {
> echo 1 &&
> echo true
>   } >expect &&
>
> or
>
>   cat >expect <<-\EOF
>   1
>   true
>   EOF

Or, simpler:

test_write_lines 1 true >expect

Re: [PATCH 1/4] builtin/config: introduce `--default`

[Eric Sunshine]

On Mon, Mar 5, 2018 at 9:17 PM, Taylor Blau  wrote:
> In an aim to replace:
>
>   $ git config --get-color slot [default] [...]
>
> with:
>
>   $ git config --default default --color slot [...]
>
> introduce `--defualt` to behave as if the given default were present and

s/--defualt/--default/

> assigned to slot in the case that that slot does not exist.
>
> Values filled by `--default` behave exactly as if they were present in
> the affected configuration file; they will be parsed by type specifiers
> without the knowledge that they are not themselves present in the
> configuraion.
>
> Specifically, this means that the following will work:
>
>   $ git config --int --default 1M does.not.exist
>   1048576
>
> In subsequent commits, we will offer `--color`, which (in conjunction
> with `--default`) will be sufficient to replace `--get-color`.
>
> Signed-off-by: Taylor Blau 

Re: [PATCH 4/4] builtin/config: introduce `--color` type specifier

[Jeff King]

On Mon, Mar 05, 2018 at 06:17:29PM -0800, Taylor Blau wrote:

> As of this commit, the cannonical way to retreive an ANSI-compatible
> color escape sequence from a configuration file is with the
> `--get-color` action.
> 
> This is to allow Git to "fall back" on a default value for the color
> should the given section not exist in the specified configuration(s).
> 
> With the addition of `--default`, this is no longer needed since:
> 
>   $ git config --default red --color core.section
> 
> will be have exactly as:
> 
>   $ git config --get-color core.section red
> 
> For consistency, let's introduce `--color` and encourage `--color`,
> `--default` together over `--get-color` alone.

Sounds good. Do we want to explicitly mark "--get-color" as historical
and/or deprecated in the git-config manpage? We won't remove it for a
long time, but this would start the cycle.

> @@ -168,6 +170,12 @@ static int format_config(struct strbuf *buf, const char 
> *key_, const char *value
>   if (git_config_expiry_date(, key_, value_) < 0)
>   return -1;
>   strbuf_addf(buf, "%"PRItime, t);
> + } else if (types == TYPE_COLOR) {
> + char *v = xmalloc(COLOR_MAXLEN);
> + if (git_config_color(, key_, value_) < 0)
> + return -1;
> + strbuf_addstr(buf, v);
> + free((char *) v);

No need to cast the argument to free, unless you're getting rid of a
"const" (but here "v" already has type "char *").

However, do we need heap allocation here at all? I think:

  char v[COLOR_MAXLEN];
  if (git_config_color(v, key_, value_) < 0)
return -1;
  strbuf_addstr(buf, v);

would suffice (and would also fix the leak when we return on error).

Ditto for the call in normalize_value().

> @@ -313,6 +321,15 @@ static char *normalize_value(const char *key, const char 
> *value)
>   else
>   return xstrdup(v ? "true" : "false");
>   }
> + if (types == TYPE_COLOR) {
> + char *v = xmalloc(COLOR_MAXLEN);
> + if (!git_config_color(, key, value)) {
> + free((char *) v);
> + return xstrdup(value);
> + }
> + free((char *) v);
> + die("cannot parse color '%s'", value);
> + }
>  
>   die("BUG: cannot normalize type %d", types);
>  }
> diff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh
> index 4f8e6f5fd..c03f54fbe 100755
> --- a/t/t1300-repo-config.sh
> +++ b/t/t1300-repo-config.sh
> @@ -931,6 +931,22 @@ test_expect_success 'get --expiry-date' '
>   test_must_fail git config --expiry-date date.invalid1
>  '
>  
> +cat >expect < +[foo]
> + color = red
> +EOF
> +
> +test_expect_success 'get --color' '
> + rm .git/config &&
> + git config --color foo.color "red" &&
> + test_cmp expect .git/config
> +'
> +
> +test_expect_success 'get --color barfs on non-color' '
> + echo "[foo]bar=not-a-color" >.git/config &&
> + test_must_fail git config --get --color foo.bar
> +'

Looks good. The out-of-block setup of expect violates our modern style,
but matches the surrounding code.

-Peff

Re: [PATCH 3/4] config.c: introduce 'git_config_color' to parse ANSI colors

[Jeff King]

On Mon, Mar 05, 2018 at 06:17:28PM -0800, Taylor Blau wrote:

> In preparation for adding `--color` to the `git-config(1)` builtin,
> let's introduce a color parsing utility, `git_config_color` in a similar
> fashion to `git_config_`.

Sounds good...

> @@ -1000,6 +1001,15 @@ int git_config_expiry_date(timestamp_t *timestamp, 
> const char *var, const char *
>   return 0;
>  }
>  
> +int git_config_color(char **dest, const char *var, const char *value)
> +{
> + if (!value)
> + return config_error_nonbool(var);
> + if (color_parse(value, *dest) < 0)
> + return -1;
> + return 0;
> +}

Why do we take a pointer-to-pointer here? We don't allocate like
git_config_string() does, but instead fill in the existing buffer.

-Peff

Re: [PATCH 2/4] Documentation: list all type specifiers in config prose

[Jeff King]

On Mon, Mar 05, 2018 at 06:17:27PM -0800, Taylor Blau wrote:

> The documentation for the `git-config(1)` builtin has not been recently
> updated to include new types, such as `--bool-or-int`, and
> `--expiry-date`. To ensure completeness when adding a new type
> specifier, let's update the existing documentation to include the new
> types.
> 
> This commit also prepares for the new type specifier `--color`, so that
> this section will not lag behind when yet more new specifiers are added.

Good catch. Thanks for cleaning this up.

> -The type specifier can be either `--int` or `--bool`, to make
> -'git config' ensure that the variable(s) are of the given type and
> -convert the value to the canonical form (simple decimal number for int,
> -a "true" or "false" string for bool), or `--path`, which does some
> -path expansion (see `--path` below).  If no type specifier is passed, no
> -checks or transformations are performed on the value.
> +The type specifier can be either `--int` or `--bool`, to make 'git config'
> +ensure that the variable(s) are of the given type and convert the value to 
> the
> +canonical form (simple decimal number for int, a "true" or "false" string for
> +bool, either of for --bool-or-int), or `--path`, which does some path 
> expansion
> +(see `--path` below), or `--expiry-date` (see `--expiry-date` below).  If no
> +type specifier is passed, no checks or transformations are performed on the
> +value.

Perhaps it's time to switch to a list format for these?

-Peff

Re: [PATCH 1/4] builtin/config: introduce `--default`

[Jeff King]

On Mon, Mar 05, 2018 at 06:17:26PM -0800, Taylor Blau wrote:

> In an aim to replace:
> 
>   $ git config --get-color slot [default] [...]
> 
> with:
> 
>   $ git config --default default --color slot [...]
> 
> introduce `--defualt` to behave as if the given default were present and
> assigned to slot in the case that that slot does not exist.

I think this motivation skips over the beginning part of the story,
which is why we want "--color --default". :)

IMHO, the reason we want --default is two-fold:

  1. Callers have to handle parsing defaults themselves, like:

   foo=$(git config core.foo || echo 1234)

 For an integer, that's not too bad, since you can write "1048576"
 instead of "1M". For colors, it's abominable, which is why we added
 "--get-color". But as we add more types that are hard to parse
 (like --expiry-date), it would be nice for them to get the same
 defaulting feature without adding --get-expiry-date, etc.

  2. --get-color is a one-off unlike all of the other types. That's bad
 interface design, but the inconsistency also makes it harder to add
 features which treat the types uniformly (like, say, a --stdin
 query mode).

And perhaps minor, but it's also easier to correctly error-check
--default, since the "foo" example above would do the wrong thing if
git-config encountered a fatal error.

> diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt
> index 14da5fc15..390b49831 100644
> --- a/Documentation/git-config.txt
> +++ b/Documentation/git-config.txt
> @@ -233,6 +233,10 @@ See also <>.
>   using `--file`, `--global`, etc) and `on` when searching all
>   config files.
>  
> +--default value::
> +  When using `--get`, `--get-all`, and `--get-regexp`, behave as
> +  if value were the value assigned to the given slot.

I had thought about this in the context of --get, where a single value
makes sense.

For --get-all, would we want to be able to specify a list of objects?
E.g.:

  git config --default foo --default bar --get-all core.slot

and behave as if we found two entries, "foo" and "bar"?

I'm not really sure what semantics would be most useful.

Ditto for --get-regexp.

This isn't necessarily an objection. I'm just not sure what people would
expect. So it might make sense to start more limited and wait for a real
use case to pop up. But I'm also open to arguments about plausible use
cases. ;)

> diff --git a/builtin/config.c b/builtin/config.c
> index ab5f95476..76edefc07 100644
> --- a/builtin/config.c
> +++ b/builtin/config.c

The general design of the implementation looks good. There's one funny
thing:

> + if (!values.nr && default_value) {
> + struct strbuf *item;
> +
> + ALLOC_GROW(values.items, values.nr + 1, values.alloc);
> + item = [values.nr++];
> + if (format_config(item, key_, default_value) < 0) {
> + values.nr = 0;
> + }

We never initialize the strbuf data here, and we can't count on
ALLOC_GROW to even zero it (which I suspect would work, but still isn't
how strbufs are meant to be used).

Do you need:

  strbuf_init(item, 0);

here, similar to what collect_config does?

(As an aside, it seems like this whole thing might be simpler with a
string_list, but that's certainly not a problem that you're introducing
here).

> +test_expect_success 'marshals default value as bool-or-int' '
> + echo "1
> +true" >expect &&
> + git config --default 1 --bool-or-int core.foo >actual &&
> + git config --default true --bool-or-int core.foo >>actual &&
> + test_cmp expect actual
> +'

Funny indentation. Use:

  {
echo 1 &&
echo true
  } >expect &&

or

  cat >expect <<-\EOF
  1
  true
  EOF

-Peff

Re: [PATCH v3 12/35] serve: introduce git-serve

[Jeff King]

On Mon, Mar 05, 2018 at 01:36:49PM -0800, Jonathan Nieder wrote:

> > I agree that would be a lot more pleasant for adding protocol features.
> > But I just worry that the stateful protocols get a lot less efficient.
> > I'm having trouble coming up with an easy reproduction, but my
> > recollection is that http has some nasty corner cases, because each
> > round of "have" lines sent to the server has to summarize the previous
> > conversation. So you can get a case where the client's requests keep
> > getting bigger and bigger during the negotiation (and eventually getting
> > large enough to cause problems).
> 
> That's not so much a corner case as just how negotiation works over
> http.

Sure. What I meant more was "there are corner cases where it gets out of
control and doesn't work".

I have had to give the advice in the past "if your fetch over http
doesn't work, try it over ssh". If we change the ssh protocol to be
stateless, too, then that closes that escape hatch.

I haven't had to give that advice for a while, though. Maybe tweaks to
the parameters or just larger buffers have made the problem go away over
the years?

> We want to do better (e.g. see [1]) but that's a bigger change than
> the initial protocol v2.
> 
> As Brandon explained it to me, we really do want to use stateless-rpc
> semantics by default, since that's just better for maintainability.
> Instead of having two protocols, one that is sane and one that
> struggles to hoist that into stateless-rpc, there would be one
> stateless baseline plus capabilities to make use of state.

Yes, I think that would be a nice end-game. It just wasn't clear to me
where we'd be in the interim.

-Peff

GOOD FRIEND?

[MOHAMED ABDUL]

Dear Sir
i have this bellow funds for investment in oil and real estate money in uae 
shearing 40% 60%  
total funs $50,000,000 USD

Your Private Mobile No:
REPLY HERE :  borisputinmarketingoiland...@gmail.com

MR MOHAMED ABDUL

Re: [PATCH] http.c: shell command evaluation for extraheader

[Colin Arnott]

Johannes,

On March 5, 2018 1:47 PM, Johannes Schindelin  
wrote:

> As the credential-helper is already intended for sensitive data, and as it
> already allows to interact with a helper, I would strongly assume that it
> would make more sense to try to extend that feature (instead of the simple
> extraHeader one).

To confirm you are suggesting that the credential struct, defined in 
credential.h, be extended to include a headers array, like so:
--- a/credential.h
+++ b/credential.h
@@ -15,6 +15,7 @@ struct credential {
char *protocol;
char *host;
char *path;
+   char **headers
 };
 
 #define CREDENTIAL_INIT { STRING_LIST_INIT_DUP }

> This would also help alleviate all the quoting/dequoting issues involved
> with shell scripting.
> 
> Besides, the http.extraHeader feature was designed to accommodate all
> kinds of extra headers, not only authentication ones (and indeed, the
> authentication was only intended for use in build agents, where both
> environment and logging can be controlled rather tightly).

I realise that my examples are scoped for auth, but I can conceive of other 
mutating headers that are not explicitly authentication related, and could 
benefit from shell execution before fetch, pull, push actions.

> I also see that in your implementation, only the extraHeader value is
> evaluated, without any access to the rest of the metadata (such as URL,
> and optionally specified user).
>
> It would probably get a little more complicated than a shell script to
> write a credential-helper that will always be asked to generate an
> authentication, but I think even a moderate-level Perl script could be
> used for that, and it would know the URL and user for which the
> credentials are intended...

You are correct; the scope provided by http..* is enough to meet my use 
cases, however I agree the lack of access to metadata limits what can be done 
within in the context of the shell, and makes the case for a credential-helper 
implementation stronger. I think there is something to be said about the 
simplicity and user-friendliness of allowing shell scripts for semi-complex 
config options, but authentication is a task that should be handled well and 
centrally, thus extending the credential-api makes sense.

​Without Wax,
Colin Arnott​

[PATCH v2] git.el: handle default excludesfile properly

[Dorab Patel]

The previous version only looked at core.excludesfile for locating the
excludesfile.  So, when core.excludesfile was not defined, it did not
use the relevant default locations for the global excludes file.

The issue is in git-get-exclude-files().  Investigation shows that
git-get-exclude-files() is only called from
git-run-ls-files-with-excludes().  Modifying
git-run-ls-files-with-excludes() to use the "--exclude-standard"
option to git-run-ls-files() obviates the need for
git-get-exclude-files() altogether, which is now removed.  In
addition, the "--exclude-per-directory" option to git-run-ls-files()
is used only when git-per-dir-ignore-file is not the default
(.gitignore), since the default case is handled by the
"--exclude-standard" option.

Looking at the history shows that git-get-exclude-files() was
implemented by commit 274e13e0e9 (git.el: Take into account the
core.excludesfile config option., 2007-07-31), whereas the
"--exclude-standard" option was introduced by commit 8e7b07c8a7
(git-ls-files: add --exclude-standard, 2007-11-15), three and a half
months later.  This explains why the "--exclude-standard" option was
not used in the original code.

Signed-off-by: Dorab Patel 
---

Notes:
The original patch[1] V1 attempted to add code to
git-get-exclude-files() to handle the case when core.excludesfile was
not defined.  This involved code to check for the env variable
XDG_CONFIG_HOME and related processing to find the value of the
default excludesfile.  Further investigation showed that using the
"--exclude-standard" option to git-run-ls-files-with-excludes()
already does similar processing.  Hence the V2 patch uses the
"--exclude-standard" option and does away with
git-get-exclude-files().

[1] 
https://public-inbox.org/git/20180303034803.21589-1-dorabpa...@gmail.com/

 contrib/emacs/git.el | 21 ++---
 1 file changed, 6 insertions(+), 15 deletions(-)

diff --git a/contrib/emacs/git.el b/contrib/emacs/git.el
index 97919f2d7..cef42f1de 100644
--- a/contrib/emacs/git.el
+++ b/contrib/emacs/git.el
@@ -755,22 +755,13 @@ Return the list of files that haven't been handled."
   (setq unmerged-files (nreverse unmerged-files))  ;; assume it is sorted 
already
   (git-set-filenames-state status unmerged-files 'unmerged
 
-(defun git-get-exclude-files ()
-  "Get the list of exclude files to pass to git-ls-files."
-  (let (files
-(config (git-config "core.excludesfile")))
-(when (file-readable-p ".git/info/exclude")
-  (push ".git/info/exclude" files))
-(when (and config (file-readable-p config))
-  (push config files))
-files))
-
 (defun git-run-ls-files-with-excludes (status files default-state  
options)
-  "Run git-ls-files on FILES with appropriate --exclude-from options."
-  (let ((exclude-files (git-get-exclude-files)))
-(apply #'git-run-ls-files status files default-state "--directory" 
"--no-empty-directory"
-   (concat "--exclude-per-directory=" git-per-dir-ignore-file)
-   (append options (mapcar (lambda (f) (concat "--exclude-from=" f)) 
exclude-files)
+  "Run git-ls-files on FILES with appropriate exclude options."
+  (apply #'git-run-ls-files status files default-state
+"--directory" "--no-empty-directory" "--exclude-standard"
+(append (unless (string-equal git-per-dir-ignore-file ".gitignore") ; 
handled by --exclude-standard
+  (list (concat "--exclude-per-directory=" 
git-per-dir-ignore-file)))
+options)))
 
 (defun git-update-status-files ( files mark-files)
   "Update the status of FILES from the index.
-- 
2.16.2

Re: [PATCH v5 00/12] rebase -i: offer to recreate merge commits

[Igor Djordjevic]

Hi Johannes,

On 26/02/2018 22:29, Johannes Schindelin wrote:
> 
> Once upon a time, I dreamt of an interactive rebase that would not
> flatten branch structure, but instead recreate the commit topology
> faithfully.
> 
> My original attempt was --preserve-merges, but that design was so
> limited that I did not even enable it in interactive mode.
> 
> Subsequently, it *was* enabled in interactive mode, with the predictable
> consequences: as the --preserve-merges design does not allow for
> specifying the parents of merge commits explicitly, all the new commits'
> parents are defined *implicitly* by the previous commit history, and
> hence it is *not possible to even reorder commits*.
> 
> This design flaw cannot be fixed. Not without a complete re-design, at
> least. This patch series offers such a re-design.
> 
> Think of --recreate-merges as "--preserve-merges done right".

First of all, thanks for this wonderful improvement to existing `git 
rebase` functionality, I`m really excited to have this in the mainline! :)

But in the light of "--preserve-merges done right", I would like to 
hear your opinion on a topic that might be considered more or less 
important, and thus tackled in a few different ways... :$

Rebasing amended merges :( Even though documentation is quite clear 
about merge conflicts and manual amendments not recreated 
automatically, this might be considered quite an issue (a bug, even), 
as even in case of non-interactive rebase, amended content will be 
dropped - and even worse, it all happens silently, without alerting 
the user (for whom we presume to know what he`s doing, I guess).

Now, might be this is considered the least interesting use case, in 
comparison to all the power of more serious interactive rebases, but 
I would argue it could be the one most used by less adventurous users 
that would simply like to stay up-to-date with upstream, rebasing their 
current work on top of it (think `git pull --rebase=recreate`, even).

As it currently is, and that was the case with `--preserve-merges`, 
too, this will cause them to silently lose their work (amended merge 
content). And while documentation is clear about it, these might be 
less knowledgeable users, too, and thus potentially be the ones we 
should (try to) protect even more, if possible.

Now, in the light of that other, ongoing "merge rebasing" topic[1], 
it seems we really might be able to do much better, actually 
_rebasing_ merges (and keeping manual conflict resolutions/amendments), 
instead of _recreating_ them (and silently loosing content), and doing 
so reliably (or stopping for possible user inspection, but not silently 
doing the wrong thing, even if documented).

This concerns non-interactive rebase the most, but I have ideas on 
making it aligned with interactive one, too, where user could 
actually decide whether to rebase or (re)create the merge (rebase 
becoming the default, intuitively aligned with non-interactive rebase).

But before elaborating, I would like to hear your opinion on whether 
you find it worth to pursue that goal here, before `--recreate-merges` 
hits the mainstream, or it might be just fine as a possible later
improvement, too (if accepted, that is).

My main concern, and why I raised the question inside this topic in 
the first place, is default behavior. With `--recreate-merges` just 
being introduced, we have no backwards compatibility to think about, 
being a unique chance to make default behave as needed (not to say 
"correct", even), and might be really ticking one more of 
"--preserve-merges done right" boxes, and could be a pretty important 
one, too.

But once this becomes widely available, I guess it will be hard to 
improve (fix?) this merge rebasing silent content losing behavior 
(even if we would acknowledge it as a bug), without introducing 
additional options - and without a possibility to make possibly 
"right" behavior a default one, thus further complicating user 
experience.

So, I wanted to hear your stance on this :(

Knowing how much this means to you, it is really not my wish to drag 
this topic further, and if you find it that we`re good here as it is, 
I wouldn`t have any objections - I guess later new `--rebase-merges` 
option is a possibility, too, might be a wrapper around 
`--recreate-merges`, but with actual merge rebasing being a default 
(where merge recreation would still be possible, too)...

Otherwise, if you have any interest in it now, I can further elaborate 
what I`m thinking about, where it might help improve both user 
experience and rebase possibilities, for what might not be too much 
extra work... hopefully :P

Whatever ends up being your response, I`m really grateful for your 
work on this matter so far, and thank you for everything you did.

p.s. lol, now that I said it, and after writing all this, I might 
actually even like the idea of (later) having `--rebase-merges` 
alongside `--recreate-merges`, too, each one clearly communicating 
its 

Re: [PATCH 0/4] Teach `--default` to `git-config(1)`

[Taylor Blau]

On Mon, Mar 05, 2018 at 06:17:25PM -0800, Taylor Blau wrote:
> Attached is a patch series to introduce `--default` and `--color` to the
> `git-config(1)` builtin with the aim of introducing a consistent interface to
> replace `--get-color`.

This series draws significant inspiration from Soukaina Nait Hmid's work
in:

  
https://public-inbox.org/git/0102015fb0bf2f74-cb456171-fe65-4d83-8784-b553c7c9e584-000...@eu-west-1.amazonses.com/

--
- Taylor

[PATCH 2/4] Documentation: list all type specifiers in config prose

[Taylor Blau]

The documentation for the `git-config(1)` builtin has not been recently
updated to include new types, such as `--bool-or-int`, and
`--expiry-date`. To ensure completeness when adding a new type
specifier, let's update the existing documentation to include the new
types.

This commit also prepares for the new type specifier `--color`, so that
this section will not lag behind when yet more new specifiers are added.

Signed-off-by: Taylor Blau 
---
 Documentation/git-config.txt | 13 +++--
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt
index 390b49831..28d84ded9 100644
--- a/Documentation/git-config.txt
+++ b/Documentation/git-config.txt
@@ -38,12 +38,13 @@ existing values that match the regexp are updated or unset. 
 If
 you want to handle the lines that do *not* match the regex, just
 prepend a single exclamation mark in front (see also <>).
 
-The type specifier can be either `--int` or `--bool`, to make
-'git config' ensure that the variable(s) are of the given type and
-convert the value to the canonical form (simple decimal number for int,
-a "true" or "false" string for bool), or `--path`, which does some
-path expansion (see `--path` below).  If no type specifier is passed, no
-checks or transformations are performed on the value.
+The type specifier can be either `--int` or `--bool`, to make 'git config'
+ensure that the variable(s) are of the given type and convert the value to the
+canonical form (simple decimal number for int, a "true" or "false" string for
+bool, either of for --bool-or-int), or `--path`, which does some path expansion
+(see `--path` below), or `--expiry-date` (see `--expiry-date` below).  If no
+type specifier is passed, no checks or transformations are performed on the
+value.
 
 When reading, the values are read from the system, global and
 repository local configuration files by default, and options
-- 
2.15.1.354.g95ec6b1b3

[PATCH 4/4] builtin/config: introduce `--color` type specifier

[Taylor Blau]

As of this commit, the cannonical way to retreive an ANSI-compatible
color escape sequence from a configuration file is with the
`--get-color` action.

This is to allow Git to "fall back" on a default value for the color
should the given section not exist in the specified configuration(s).

With the addition of `--default`, this is no longer needed since:

  $ git config --default red --color core.section

will be have exactly as:

  $ git config --get-color core.section red

For consistency, let's introduce `--color` and encourage `--color`,
`--default` together over `--get-color` alone.

Signed-off-by: Taylor Blau 
---
 Documentation/git-config.txt | 10 +++---
 builtin/config.c | 17 +
 t/t1300-repo-config.sh   | 16 
 3 files changed, 40 insertions(+), 3 deletions(-)

diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt
index 28d84ded9..0dfb20324 100644
--- a/Documentation/git-config.txt
+++ b/Documentation/git-config.txt
@@ -42,9 +42,9 @@ The type specifier can be either `--int` or `--bool`, to make 
'git config'
 ensure that the variable(s) are of the given type and convert the value to the
 canonical form (simple decimal number for int, a "true" or "false" string for
 bool, either of for --bool-or-int), or `--path`, which does some path expansion
-(see `--path` below), or `--expiry-date` (see `--expiry-date` below).  If no
-type specifier is passed, no checks or transformations are performed on the
-value.
+(see `--path` below), or `--expiry-date` (see `--expiry-date` below), or
+`--color` (see `--color` below).  If no type specifier is passed, no checks or
+transformations are performed on the value.
 
 When reading, the values are read from the system, global and
 repository local configuration files by default, and options
@@ -186,6 +186,10 @@ See also <>.
a fixed or relative date-string to a timestamp. This option
has no effect when setting the value.
 
+--color::
+  `git config` will ensure that the output is converted to an
+  ANSI color escape sequence.
+
 -z::
 --null::
For all options that output values and/or keys, always
diff --git a/builtin/config.c b/builtin/config.c
index 76edefc07..05f97f2cb 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -54,6 +54,7 @@ static int show_origin;
 #define TYPE_BOOL_OR_INT (1<<2)
 #define TYPE_PATH (1<<3)
 #define TYPE_EXPIRY_DATE (1<<4)
+#define TYPE_COLOR (1<<5)
 
 static struct option builtin_config_options[] = {
OPT_GROUP(N_("Config file location")),
@@ -83,6 +84,7 @@ static struct option builtin_config_options[] = {
OPT_BIT(0, "bool-or-int", , N_("value is --bool or --int"), 
TYPE_BOOL_OR_INT),
OPT_BIT(0, "path", , N_("value is a path (file or directory 
name)"), TYPE_PATH),
OPT_BIT(0, "expiry-date", , N_("value is an expiry date"), 
TYPE_EXPIRY_DATE),
+   OPT_BIT(0, "color", , N_("value is a color"), TYPE_COLOR),
OPT_GROUP(N_("Other")),
OPT_BOOL('z', "null", _null, N_("terminate values with NUL byte")),
OPT_BOOL(0, "name-only", _values, N_("show variable names only")),
@@ -168,6 +170,12 @@ static int format_config(struct strbuf *buf, const char 
*key_, const char *value
if (git_config_expiry_date(, key_, value_) < 0)
return -1;
strbuf_addf(buf, "%"PRItime, t);
+   } else if (types == TYPE_COLOR) {
+   char *v = xmalloc(COLOR_MAXLEN);
+   if (git_config_color(, key_, value_) < 0)
+   return -1;
+   strbuf_addstr(buf, v);
+   free((char *) v);
} else if (value_) {
strbuf_addstr(buf, value_);
} else {
@@ -313,6 +321,15 @@ static char *normalize_value(const char *key, const char 
*value)
else
return xstrdup(v ? "true" : "false");
}
+   if (types == TYPE_COLOR) {
+   char *v = xmalloc(COLOR_MAXLEN);
+   if (!git_config_color(, key, value)) {
+   free((char *) v);
+   return xstrdup(value);
+   }
+   free((char *) v);
+   die("cannot parse color '%s'", value);
+   }
 
die("BUG: cannot normalize type %d", types);
 }
diff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh
index 4f8e6f5fd..c03f54fbe 100755
--- a/t/t1300-repo-config.sh
+++ b/t/t1300-repo-config.sh
@@ -931,6 +931,22 @@ test_expect_success 'get --expiry-date' '
test_must_fail git config --expiry-date date.invalid1
 '
 
+cat >expect <.git/config &&
+   test_must_fail git config --get --color foo.bar
+'
+
 cat > expect << EOF
 [quote]
leading = " test"
-- 
2.15.1.354.g95ec6b1b3

[PATCH 1/4] builtin/config: introduce `--default`

[Taylor Blau]

In an aim to replace:

  $ git config --get-color slot [default] [...]

with:

  $ git config --default default --color slot [...]

introduce `--defualt` to behave as if the given default were present and
assigned to slot in the case that that slot does not exist.

Values filled by `--default` behave exactly as if they were present in
the affected configuration file; they will be parsed by type specifiers
without the knowledge that they are not themselves present in the
configuraion.

Specifically, this means that the following will work:

  $ git config --int --default 1M does.not.exist
  1048576

In subsequent commits, we will offer `--color`, which (in conjunction
with `--default`) will be sufficient to replace `--get-color`.

Signed-off-by: Taylor Blau 
---
 Documentation/git-config.txt |   4 ++
 builtin/config.c |  19 +++
 t/t1310-config-default.sh| 119 +++
 3 files changed, 142 insertions(+)
 create mode 100755 t/t1310-config-default.sh

diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt
index 14da5fc15..390b49831 100644
--- a/Documentation/git-config.txt
+++ b/Documentation/git-config.txt
@@ -233,6 +233,10 @@ See also <>.
using `--file`, `--global`, etc) and `on` when searching all
config files.
 
+--default value::
+  When using `--get`, `--get-all`, and `--get-regexp`, behave as
+  if value were the value assigned to the given slot.
+
 [[FILES]]
 FILES
 -
diff --git a/builtin/config.c b/builtin/config.c
index ab5f95476..76edefc07 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -26,6 +26,7 @@ static char term = '\n';
 static int use_global_config, use_system_config, use_local_config;
 static struct git_config_source given_config_source;
 static int actions, types;
+static char *default_value;
 static int end_null;
 static int respect_includes_opt = -1;
 static struct config_options config_options;
@@ -87,6 +88,7 @@ static struct option builtin_config_options[] = {
OPT_BOOL(0, "name-only", _values, N_("show variable names only")),
OPT_BOOL(0, "includes", _includes_opt, N_("respect include 
directives on lookup")),
OPT_BOOL(0, "show-origin", _origin, N_("show origin of config 
(file, standard input, blob, command line)")),
+   OPT_STRING(0, "default", _value, N_("value"), N_("use default 
value with missing entry")),
OPT_END(),
 };
 
@@ -251,6 +253,16 @@ static int get_value(const char *key_, const char *regex_)
config_with_options(collect_config, ,
_config_source, _options);
 
+   if (!values.nr && default_value) {
+   struct strbuf *item;
+
+   ALLOC_GROW(values.items, values.nr + 1, values.alloc);
+   item = [values.nr++];
+   if (format_config(item, key_, default_value) < 0) {
+   values.nr = 0;
+   }
+   }
+
ret = !values.nr;
 
for (i = 0; i < values.nr; i++) {
@@ -594,6 +606,13 @@ int cmd_config(int argc, const char **argv, const char 
*prefix)
usage_with_options(builtin_config_usage, 
builtin_config_options);
}
 
+   if (default_value && !(actions &
+   (ACTION_GET|ACTION_GET_ALL|ACTION_GET_REGEXP))) {
+   error("--default is only applicable to --get, --get-all, "
+   "and --get-regexp.");
+   usage_with_options(builtin_config_usage, 
builtin_config_options);
+   }
+
if (actions == ACTION_LIST) {
check_argc(argc, 0, 0);
if (config_with_options(show_all_config, NULL,
diff --git a/t/t1310-config-default.sh b/t/t1310-config-default.sh
new file mode 100755
index 0..57fe63295
--- /dev/null
+++ b/t/t1310-config-default.sh
@@ -0,0 +1,119 @@
+#!/bin/sh
+
+test_description='Test git config in different settings (with --default)'
+
+. ./test-lib.sh
+
+test_expect_success 'clear default config' '
+   rm -f .git/config
+'
+
+test_expect_success 'uses default when missing entry' '
+   echo quux >expect &&
+   git config --default quux core.foo >actual &&
+   test_cmp expect actual
+'
+
+test_expect_success 'uses entry when available' '
+   echo bar >expect &&
+   git config --add core.foo bar &&
+   git config --default baz core.foo >actual &&
+   git config --unset core.foo &&
+   test_cmp expect actual
+'
+
+test_expect_success 'marshals default value as bool' '
+   echo true >expect &&
+   git config --default true --bool core.foo >actual &&
+   test_cmp expect actual
+'
+
+test_expect_success 'marshals default value as int' '
+   echo 810 >expect &&
+   git config --default 810 --int core.foo >actual &&
+   test_cmp expect actual
+'
+
+test_expect_success 'marshals default value as int (storage unit)' '
+   echo 1048576 >expect &&
+   git config --default 1M --int core.foo >actual &&
+   test_cmp 

[PATCH 3/4] config.c: introduce 'git_config_color' to parse ANSI colors

[Taylor Blau]

In preparation for adding `--color` to the `git-config(1)` builtin,
let's introduce a color parsing utility, `git_config_color` in a similar
fashion to `git_config_`.

Signed-off-by: Taylor Blau 
---
 config.c | 10 ++
 config.h |  1 +
 2 files changed, 11 insertions(+)

diff --git a/config.c b/config.c
index b0c20e6cb..fb1f41ab3 100644
--- a/config.c
+++ b/config.c
@@ -16,6 +16,7 @@
 #include "string-list.h"
 #include "utf8.h"
 #include "dir.h"
+#include "color.h"
 
 struct config_source {
struct config_source *prev;
@@ -1000,6 +1001,15 @@ int git_config_expiry_date(timestamp_t *timestamp, const 
char *var, const char *
return 0;
 }
 
+int git_config_color(char **dest, const char *var, const char *value)
+{
+   if (!value)
+   return config_error_nonbool(var);
+   if (color_parse(value, *dest) < 0)
+   return -1;
+   return 0;
+}
+
 static int git_default_core_config(const char *var, const char *value)
 {
/* This needs a better name */
diff --git a/config.h b/config.h
index ef70a9cac..48f8e7684 100644
--- a/config.h
+++ b/config.h
@@ -59,6 +59,7 @@ extern int git_config_bool(const char *, const char *);
 extern int git_config_string(const char **, const char *, const char *);
 extern int git_config_pathname(const char **, const char *, const char *);
 extern int git_config_expiry_date(timestamp_t *, const char *, const char *);
+extern int git_config_color(char **, const char *, const char *);
 extern int git_config_set_in_file_gently(const char *, const char *, const 
char *);
 extern void git_config_set_in_file(const char *, const char *, const char *);
 extern int git_config_set_gently(const char *, const char *);
-- 
2.15.1.354.g95ec6b1b3

[PATCH 0/4] Teach `--default` to `git-config(1)`

[Taylor Blau]

Hi,

Attached is a patch series to introduce `--default` and `--color` to the
`git-config(1)` builtin with the aim of introducing a consistent interface to
replace `--get-color`.

Thank you in advance for reviewing this series; I will be more than happy to
respond to any feedback seeing as I am still quite new to working on Git itself.

Thanks,
Taylor

Taylor Blau (4):
  builtin/config: introduce `--default`
  Documentation: list all type specifiers in config prose
  config.c: introduce 'git_config_color' to parse ANSI colors
  builtin/config: introduce `--color` type specifier

 Documentation/git-config.txt |  21 +---
 builtin/config.c |  36 +
 config.c |  10 
 config.h |   1 +
 t/t1300-repo-config.sh   |  16 ++
 t/t1310-config-default.sh| 119 +++
 6 files changed, 197 insertions(+), 6 deletions(-)
 create mode 100755 t/t1310-config-default.sh

--
2.15.1.354.g95ec6b1b3

Re: git help clone: questions

[Junio C Hamano]

kalle  writes:

> -In the explanation of the option --reference: shouldn't there be
> written '' instead of  'reference repository'?

"Shouldn't X be Y?" is not an effective way to communicate; it
solicits a "no, the current one is fine." without any explanation.

If you think X should be Y for some reason, please say "I think X
should be Y BECAUSE Z" instead.  Without stating why you think
differently from what those who wrote the current text, it is hard
for people to respond either with "Yeah, you're right---I agree
with Z" or with "No, Z does not hold because..."

Re: [GSoC] [PATCH] travis-ci: added clang static analysis

[Junio C Hamano]

SiddharthaMishra  writes:

> Added a job to run clang static code analysis on the master and maint branch
>
> Signed-off-by: SiddharthaMishra 
> ---

Why on 'master' and 'maint' and not others?  Quite frankly, I find
this choice of branches rather odd, as these two branches are not
where the real development happens.  If we do not want to increase
the number of jobs and limit the test only to a single branch, I
would probably pick 'next', and if we can afford two, probably
'pu' and 'next'.

Re: [PATCH v9 6/8] convert: check for detectable errors in UTF encodings

[Junio C Hamano]

Lars Schneider  writes:

>> On 05 Mar 2018, at 22:50, Junio C Hamano  wrote:
>> 
>> lars.schnei...@autodesk.com writes:
>> 
>>> +static int validate_encoding(const char *path, const char *enc,
>>> + const char *data, size_t len, int die_on_error)
>>> +{
>>> +   if (!memcmp("UTF-", enc, 4)) {
>> 
>> Does the caller already know that enc is sufficiently long that
>> using memcmp is safe?
>
> No :-(
>
> Would you be willing to squash that in?
>
> if (strlen(enc) > 4 && !memcmp("UTF-", enc, 4)) {
>
> I deliberately used "> 4" as plain "UTF-" is not even valid.

I'd rather not.  The code does not have to even look at 6th and
later bytes in the enc[] even if it wanted to reject "UTF-" followed
by nothing, but use of strlen() forces it to look at everything.

Stepping back, shouldn't

if (starts_with(enc, "UTF-") 

be sufficient?  If you really care about the case where "UTF-" alone
comes here, you could write

if (starts_with(enc, "UTF-") && enc[4])

but I do not think "&& enc[4]" is even needed.  The functions called
from this block would not consider "UTF-" alone as something valid
anyway, so with that "&& enf[4]" we would be piling more code only
for invalid/rare case.

Re: [GSoC][PATCH v3] Make options that expect object ids less chatty if id is invalid

[Junio C Hamano]

Paul-Sebastian Ungureanu  writes:

> Usually, the usage should be shown only if the user does not know what
> options are available. If the user specifies an invalid value, the user
> is already aware of the available options. In this case, there is no
> point in displaying the usage anymore.
>
> This patch applies to "git tag --contains", "git branch --contains",
> "git branch --points-at", "git for-each-ref --contains" and many more.
>
> Signed-off-by: Paul-Sebastian Ungureanu 
> ---

I am guessing that this was sent as a replacement for fcfba373
("ref-filter: make "--contains " less chatty if  is
invalid", 2018-02-23) that was merged to 'next' at 9623d681 ("Merge
branch 'ps/contains-id-error-message' into next", 2018-02-27).

In general, we do not drop and replace what is already merged to
'next' with a new version; once a topic is merged to 'next', we go
incremental and further refinements are made on top instead.

I however strongly suspect that the approach taken by this round is
a lot better, and it is sufficiently different that an "incremental"
that applies on top of the previous patches would essentially revert
them and builds what we see in this message afresh.

So I am tempted to revert the previous one out of 'next' and then
treat this one as if it were a new/different topic.


> diff --git a/builtin/update-index.c b/builtin/update-index.c
> index 58d1c2d28..1c170 100644
> --- a/builtin/update-index.c
> +++ b/builtin/update-index.c
> @@ -1060,6 +1060,8 @@ int cmd_update_index(int argc, const char **argv, const 
> char *prefix)
>   switch (parseopt_state) {
>   case PARSE_OPT_HELP:
>   exit(129);
> + case PARSE_OPT_ERROR:
> + exit(1);

OK, so things like

$ git update-index --index-version
$ git update-index --index-version NOT_AN_INTEGER

used to give the full usage (just like your primary and original
focus that was what "tag --contains" etc. did), but with this
change, they just throw an error and stop.  I guess this is a very
good thing ;-)

Also the exit status is changed from 129 to 1.  It is not clear if
that is a desirable change (I am not yet saying we shouldn't change
it, though).  Calling scripts probably only care about non-zero ness
of the exit status, so this change probably does not hurt people in
practice, I guess.

> @@ -504,7 +503,7 @@ int parse_options_step(struct parse_opt_ctx_t *ctx,
>   goto show_usage;
>   switch (parse_long_opt(ctx, arg + 2, options)) {
>   case -1:
> - goto show_usage_error;
> + return PARSE_OPT_ERROR;

An error return -1 from parse_long_opt() unfortunately includes the
case where the given string is ambiguous.  The test-parse-options
command in t/helpers e.g. has --string and --string2 option, that
take a string argument, so "test-parse-options --stri" says "error:
Ambigous option: stri (could be --string or --string2)", and without
this patch, it goes on to show the usage help.  With the patch,
however, we no longer get the help, and I think that is a regression;
the user likely wants to see the help text that describes these two
potential options to decide between the two.

Of course, "test-parse-options --string" fails with "error: options
`string' requires a value" and stops without the usage help---and
that is definitely an improvement.

Taking these together, I _think_ this patch is moving things in the
right direction, in that it allows callers of parse_options_step()
to tell "user knew what option s/he wanted, and sufficient error
message has already be given" and "user gave us a nonsense option
and would be helped by usage text" cases apart by introducing a new
value PARSE_OPT_ERROR, but in order to be able to correctly give
PARSE_OPT_ERROR back to the caller, parse_long_opt() and
parse_short_opt() (possibly, but I didn't check) would need a bit of
tweak to help their callers in this function.

>  test_expect_success 'non ambiguous option (after two options it 
> abbreviates)' '
> @@ -291,6 +291,7 @@ test_expect_success 'OPT_CALLBACK() and OPT_BIT() work' '
>  test_expect_success 'OPT_CALLBACK() and callback errors work' '
>   test_must_fail test-parse-options --no-length >output 2>output.err &&
>   test_i18ncmp expect output &&
> + >expect.err &&
>   test_i18ncmp expect.err output.err
>  '

The way the existing script sets test vectors up is not that great,
but this "expect.err" file is created by getting the usage output at
the very beginning of the test (into "expect") and a few tests refer
to it, expecting it to have the usage help (see check_unknown_i18n
helper).  We should treat its contents as a constant, and shouldn't
touch it like this.  Instead, perhaps do

test_must_fail ... 2>actual.err &&
test_must_be_empty actual.err

here.

> diff --git 

Re: [PATCH/RFC 1/1] gc --auto: exclude the largest giant pack in low-memory config

[Duy Nguyen]

On Mon, Mar 5, 2018 at 9:00 PM, Ævar Arnfjörð Bjarmason
 wrote:
>
> On Thu, Mar 01 2018, Nguyễn Thái Ngọc Duy jotted:
>
>> pack-objects could be a big memory hog especially on large repos,
>> everybody knows that. The suggestion to stick a .keep file on the
>> largest pack to avoid this problem is also known for a long time.
>>
>> Let's do the suggestion automatically instead of waiting for people to
>> come to Git mailing list and get the advice. When a certain condition
>> is met, gc --auto create a .keep file temporary before repack is run,
>> then remove it afterward.
>>
>> gc --auto does this based on an estimation of pack-objects memory
>> usage and whether that fits in one third of system memory (the
>> assumption here is for desktop environment where there are many other
>> applications running).
>>
>> Since the estimation may be inaccurate and that 1/3 threshold is
>> arbitrary, give the user a finer control over this mechanism as well:
>> if the largest pack is larger than gc.bigPackThreshold, it's kept.
>
> This is very promising. Saves lots of memory on my ad-hoc testing of
> adding a *.keep file on an in-house repo.

The good news for you is when we run external rev-list on top of this,
memory consumption seems even better (and I think even peak memory
should be a bit lower too, but I'll need to verify that).

>> + if (big_pack_threshold)
>> + return pack->pack_size >= big_pack_threshold;
>> +
>> + /* First we have to scan through at least one pack */
>> + mem_want = pack->pack_size + pack->index_size;
>> + /* then pack-objects needs lots more for book keeping */
>> + mem_want += sizeof(struct object_entry) * nr_objects;
>> + /*
>> +  * internal rev-list --all --objects takes up some memory too,
>> +  * let's say half of it is for blobs
>> +  */
>> + mem_want += sizeof(struct blob) * nr_objects / 2;
>> + /*
>> +  * and the other half is for trees (commits and tags are
>> +  * usually insignificant)
>> +  */
>> + mem_want += sizeof(struct tree) * nr_objects / 2;
>> + /* and then obj_hash[], underestimated in fact */
>> + mem_want += sizeof(struct object *) * nr_objects;
>> + /*
>> +  * read_sha1_file() (either at delta calculation phase, or
>> +  * writing phase) also fills up the delta base cache
>> +  */
>> + mem_want += delta_base_cache_limit;
>> + /* and of course pack-objects has its own delta cache */
>> + mem_want += max_delta_cache_size;
>
> I'm not familiar enough with this part to say, but isn't this assuming a
> lot about the distribution of objects in a way that will cause is not to
> repack in some pathological cases?

Yeah this assumes a "normal" case. When the estimation is really off,
either we exclude the base pack or repack everything unnecessarily,
but we always repack. A wrong decision here can only affect
performance, not correctness.

There's one case I probably should address though. This "exclude the
base pack" will create two packs in the end, one big and one small.
But if the second pack is getting as big as the first one, it's time
we merge both into one.

> Probably worth documenting...
>
>> + /* Only allow 1/3 of memory for pack-objects */
>> + mem_have = total_ram() / 3;
>
> Would be great to have this be a configurable variable, so you could set
> it to e.g. 33% (like here), 50% etc.

Hmm.. isn't gc.bigPackThreshold enough? I mean in a controlled
environment, you probably already know how much ram is available, and
much of this estimation is based on pack size (well the number of
objects in the pack) anyway, you could avoid all this heuristics by
saying "when the base pack is larger than 1GB, always exclude it in
repack". This estimation should only needed when people do not
configure anything (and still expect reasonable defaults). Or when you
plan multiple 'gc' runs on the same machine?
-- 
Duy

Re: [PATCH v9 6/8] convert: check for detectable errors in UTF encodings

[Lars Schneider]

> On 05 Mar 2018, at 22:50, Junio C Hamano  wrote:
> 
> lars.schnei...@autodesk.com writes:
> 
>> +static int validate_encoding(const char *path, const char *enc,
>> +  const char *data, size_t len, int die_on_error)
>> +{
>> +if (!memcmp("UTF-", enc, 4)) {
> 
> Does the caller already know that enc is sufficiently long that
> using memcmp is safe?

No :-(

Would you be willing to squash that in?

if (strlen(enc) > 4 && !memcmp("UTF-", enc, 4)) {

I deliberately used "> 4" as plain "UTF-" is not even valid.


Thanks for spotting this,
Lars

Re: man gittutorial: patch proposal

[Jonathan Nieder]

Hi,

kalle wrote:

> see attachment.

Thanks for writing.  A few questions:

 1. Can you say a little more about the rationale for this change?
E.g. is the current formatting sending a confusing message?  Is the
current formatting intending to use '' as quotes and using italics
instead?  If so, should this use "" to fix it?

 2. May we forge your sign-off? See the section "Certify your work"
in Documentation/SubmittingPatches for more about what this means.

Thanks,
Jonathan

user-manual: patch proposals and questions

[kalle]

The patches are attached.
Further some questions:

-see the explanations of the branch command, ca. line 280: wouldn't it
be better to use other words than 'references'?
-sentence "it shows all commits reachable from the parent commit": it
seems wrong to me. The last commit is also shown.
- chapter "Browsing revisions": it seems counterintuitive to me to have
two different logics for the meaning of "branch1..branch2" and
"branch1...branch2", according to whether it's the argument of `git log'
or `git diff'
-section "Check whether two branches point at the same history": 'git
diff origin..master' -> shouldn't it be rather 'git diff
branch1..branch2'? … or rewrite the example with branch1=origin and
branch2=master.

greetings,
kalle
>From 19061a0dd4363edbf8757a5e9eee8ace210f4029 Mon Sep 17 00:00:00 2001
From: kalledaballe 
Date: Fri, 9 Feb 2018 20:46:52 +0100
Subject: [PATCH 2/5] 3 small formulation changes in
 Documentation/user-manual.txt

---
 Documentation/user-manual.txt | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/Documentation/user-manual.txt b/Documentation/user-manual.txt
index eff7890..e3efc26 100644
--- a/Documentation/user-manual.txt
+++ b/Documentation/user-manual.txt
@@ -322,7 +322,7 @@ do so (now or later) by using -b with the checkout command again. Example:
 HEAD is now at 427abfa Linux v2.6.17
 
 
-The HEAD then refers to the SHA-1 of the commit instead of to a branch,
+If you haven't created a new branch yet, the HEAD then refers to the SHA-1 of the commit instead of to a branch,
 and git branch shows that you are no longer on a branch:
 
 
@@ -370,7 +370,7 @@ be updated by `git fetch` (hence `git pull`) and `git push`. See
 <> for details.
 
 You might want to build on one of these remote-tracking branches
-on a branch of your own, just as you would for a tag:
+a branch of your own, just as you would for a tag:
 
 
 $ git checkout -b my-todo-copy origin/todo
@@ -404,7 +404,7 @@ they may also be packed together in a single file; see
 linkgit:git-pack-refs[1]).
 
 As another useful shortcut, the "HEAD" of a repository can be referred
-to just using the name of that repository.  So, for example, "origin"
+to by just using the name of that repository.  So, for example, "origin"
 is usually a shortcut for the HEAD branch in the repository "origin".
 
 For the complete list of paths which Git checks for references, and
-- 
2.1.4

>From 3917eeaf8d21b8b90a773c46ee5b9d12eac901e3 Mon Sep 17 00:00:00 2001
From: kalledaballe 
Date: Sat, 10 Feb 2018 16:08:45 +0100
Subject: [PATCH 3/5] I changed the sequence of 2 sentences.

---
 Documentation/user-manual.txt | 10 +++---
 1 file changed, 3 insertions(+), 7 deletions(-)

diff --git a/Documentation/user-manual.txt b/Documentation/user-manual.txt
index e3efc26..b9dc17a 100644
--- a/Documentation/user-manual.txt
+++ b/Documentation/user-manual.txt
@@ -609,13 +609,9 @@ $ git show HEAD^2   # show the second parent of HEAD
 In addition to HEAD, there are several other special names for
 commits:
 
-Merges (to be discussed later), as well as operations such as
-`git reset`, which change the currently checked-out commit, generally
-set ORIG_HEAD to the value HEAD had before the current operation.
+ORIG_HEAD is set to the value HEAD before merging (to be discussed later) operations as well as operations such as `git reset'. which change the currently checked-out commit.
 
-The `git fetch` operation always stores the head of the last fetched
-branch in FETCH_HEAD.  For example, if you run `git fetch` without
-specifying a local branch as the target of the operation
+FETCH_HEAD after a `git fetch' operation stores the head of the last fetched branch. For example, if you run `git fetch' without specifying a local branch as the target of the operation.
 
 -
 $ git fetch git://example.com/proj.git theirbranch
@@ -664,7 +660,7 @@ can also make more specific requests:
 
 -
 $ git log v2.5..	# commits since (not reachable from) v2.5
-$ git log test..master	# commits reachable from master but not test
+$ git log ..master	# commits reachable from master but not from test
 $ git log master..test	# ...reachable from test but not master
 $ git log master...test	# ...reachable from either test or master,
 			#but not both
-- 
2.1.4

>From a084b699ed7a0768530f9112ae8469fd5e297356 Mon Sep 17 00:00:00 2001
From: kalledaballe 
Date: Sat, 10 Feb 2018 16:14:25 +0100
Subject: [PATCH 4/5] I errouneously have deleted one word too much in the last
 commit

---
 Documentation/user-manual.txt | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Documentation/user-manual.txt b/Documentation/user-manual.txt
index 

user-manual: patch proposals and questions

[kalle]

The patches are attached.
Further some questions:

-see the explanations of the branch command, ca. line 280: wouldn't it
be better to use other words than 'references'?
-sentence "it shows all commits reachable from the parent commit": it
seems wrong to me. The last commit is also shown.
- chapter "Browsing revisions": it seems counterintuitive to me to have
two different logics for the meaning of "branch1..branch2" and
"branch1...branch2", according to whether it's the argument of `git log'
or `git diff'
-section "Check whether two branches point at the same history": 'git
diff origin..master' -> shouldn't it be rather 'git diff
branch1..branch2'? … or rewrite the example with branch1=origin and
branch2=master.

greetings,
kalle

git help clone: questions

[kalle]

-In the explanation of the option --reference: shouldn't there be
written '' instead of  'reference repository'?

greetings,
kalle

man gittutorial: patch proposal

[kalle]

see attachment.
greetings,
kalle
>From ed466d29733a14acf3b2071d3d35aa829e09b1d7 Mon Sep 17 00:00:00 2001
From: kalledaballe 
Date: Thu, 8 Feb 2018 18:53:54 +0100
Subject: [PATCH 1/5] I corrected some few quotation mistakes

---
 Documentation/gittutorial.txt | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/Documentation/gittutorial.txt b/Documentation/gittutorial.txt
index 242de31..d04f8ac 100644
--- a/Documentation/gittutorial.txt
+++ b/Documentation/gittutorial.txt
@@ -372,7 +372,7 @@ her work-in-progress on top of the resulting history.
 
 When you are working in a small closely knit group, it is not
 unusual to interact with the same repository over and over
-again.  By defining 'remote' repository shorthand, you can make
+again.  By defining remote repository shorthand, you can make
 it easier:
 
 
@@ -406,8 +406,8 @@ could merge the changes into her master branch:
 alice$ git merge bob/master
 -
 
-This `merge` can also be done by 'pulling from her own remote-tracking
-branch', like this:
+This merge can also be done by pulling from her own remote-tracking
+branch, like this:
 
 -
 alice$ git pull . remotes/bob/master
-- 
2.1.4

Re: [PATCH 2/2] git-svn: allow empty email-address in authors-prog and authors-file

[Eric Sunshine]

On Mon, Mar 5, 2018 at 4:37 AM, Andreas Heiduk  wrote:
> 2018-03-05 2:42 GMT+01:00 Eric Sunshine :
>> On Sun, Mar 4, 2018 at 6:22 AM, Andreas Heiduk  wrote:
>>> The email address in --authors-file and --authors-prog can be empty but
>>> git-svn translated it into a syntethic email address in the form
>>> $USERNAME@$REPO_UUID. Now git-svn behaves like git-commit: If the email
>>> is explicitly set to the empty string, the commit does not contain
>>> an email address.
>>
>> Falling back to "$name@$uuid" was clearly an intentional choice, so
>> this seems like a rather significant change of behavior. How likely is
>> it that users or scripts relying upon the existing behavior will break
>> with this change? If the likelihood is high, should this behavior be
>> opt-in?
>
> I don't know nor understand the intial choice. Didn' git-commit support
> empty emails once upon a time? Or is the SVN-UID important for some
> SVK/SVM workflows?

I don't know the answer to either question. As I've only ever used
git-svn a few times many years ago, I can't speak of the reason behind
the name@uuid fallback, but that it was intentional suggests that
there may have been a good reason for it.

> My reasoning was: If authors-prog is NOT used, the behaviour
> is unchanged - the UUID will be generated. If a Script IS used, then I
> assume that a valid email was generated and this change will not
> break these scripts. Only scripts intentionally not generating emails
> will break. But again - was would be the purpose of such a script?
> And such a script can be easily changed to add the UUID again.

As I'm not the author of every script in the wild, I can't answer as
to the purpose of a script working in such a way, but that there may
be such scripts makes me cautious. We don't know how easy it would be
for a script to be "fixed" for this new behavior or even how soon the
"breakage" would be noticed for scripts which have been working
properly and quietly in the background for years.

> So I think adding an explicit opt-in does not pay off.

I defer judgment to Eric W.'s area expertise.

>>> if ($author =~ /^\s*(.+?)\s*<(.*)>\s*$/) {
>>> my ($name, $email) = ($1, $2);
>>> -   $email = undef if length $2 == 0;
>>> return [$name, $email];
>>
>> Mental note: existing behavior intentionally makes $email undefined if
>> not present in $author; revised behavior leaves it defined.
>
> But only if the data comes from authors-prog. authors-file is unaffected.
>
>>
>>> } else {
>>> die "Author: $orig_author: $::_authors_prog returned "
>>> @@ -2020,8 +2019,8 @@ sub make_log_entry {
>>> -   $email ||= "$author\@$uuid";
>>> -   $commit_email ||= "$author\@$uuid";
>>> +   $email //= "$author\@$uuid";
>>> +   $commit_email //= "$author\@$uuid";
>>
>> With the revised behavior (above), $email is unconditionally defined,
>> so these //= expressions will _never_ assign "$author\@$uuid" to
>> $email. Am I reading that correctly? If so, then isn't this now just
>> dead code? Wouldn't it be clearer to remove these lines altogether?
>
> The olf behaviour still kicks in if
>  - neither authors-file nor authors-prog is used
>  - only authors-file is used

Okay.

>> (However, there has lately been some talk[1] about bumping the minimum
>> Perl version to 5.10.)
>>
>> [1]: https://public-inbox.org/git/20171223174400.26668-1-ava...@gmail.com/
>
> Did the thread result in some actual action?

Not to my knowledge. Scanning the thread, it looks like the issue is
still hanging.

Ad-hoc pre contributor summit dinner & drinks on the 6th

[Ævar Arnfjörð Bjarmason]

On Sat, Mar 03 2018, Jeff King jotted:

> The rest of this email is all logistics for attendees, so if you're not
> coming, you can stop reading. :)

I'll be arriving in BCN around 17:00 tomorrow. If someone wants to grab
pre-dinner beer or dinner, preferably in the Eixample district (just
across the street from the conference venue) before or at Spanish dinner
time (around 21-22) add me on WhatsApp @ +31 611 763 987.

If there's more than one person interested I'll create a WhatsApp group
for it so we can keep adding people. Or reply here if you prefer E-Mail,
but I'm thinking subscribers to the ML who aren't there tomorrow will
thank us...

Re: [PATCH 2/2] add--interactive: detect bogus diffFilter output

[Ævar Arnfjörð Bjarmason]

On Mon, Mar 5, 2018 at 11:09 PM, Junio C Hamano  wrote:
> Ævar Arnfjörð Bjarmason  writes:
>
>> On Sat, Mar 03 2018, Jeff King jotted:
>>
>>> +if (@colored && @colored != @diff) {
>>
>> nit: should just be:
>>
>> if (@colored != @diff) {
>>
>> It's not possible for @arrays in scalar context to be undefined.
>
> It is true that @array can not be undef, but your rewrite I think is
> wrong.
>
> The first "do the comparison only @colored is true" is not about
> definedness.  It is "@colored can be an empty array when the user is
> not using separate 'show these colored lines to the end user, feed
> these noncolored lines to git-apply command' feature, so @colored==0
> and @diff > 0 is perfectly fine".

Yes, sorry for the noise. I misread the intent of the code.

Re: Contributor Summit planning

[Ævar Arnfjörð Bjarmason]

On Mon, Mar 05 2018, Jonathan Nieder jotted:

> Lars Schneider wrote:
>> - error reporting: Git is distributed and therefore lots of errors are only
>>   reported locally. That makes it hard for administrators in larger
>>   companies to see trouble. Would it make sense to add a config option that
>>   would push recent errors along with "git push" to the server?
>
> I'm interested in instrumentation but worried about the privacy
> ramifications of this particular proposal.  I'd be happy to see some
> built-in instrumentation hooks (or even a standard instrumentation
> approach, if the mailing list comes up with a good one that respects
> privacy).

I have this use-case as well, and figured a good approach would be:q

 1. Add corresponding config variables for GIT_TRACE_* so you could
config them in /etc/gitconfig (or elsewhere). Similar to
e.g. user.name & GIT_AUTHOR_NAME

 2. Add some new trace like e.g. GIT_TRACE_COMMANDS, make it take a
format string in GIT_TRACE_COMMANDS_FORMAT (or usually via
config). Thus setting GIT_TRACE_COMMANDS to a file would e.g. spew
your current repo path, subcommand, or even the absolute command
line to the file.

 3. Have some cronjob or other monitoring thingy pick up the file &
submit to central logging.

Of course you could overdo the format specifiers in #2 and e.g. send the
full commands along, but it seems to me that it would be sufficient for
privacy concerns to document that caveat with some examples.

After all, for this use-case we're talking about us somehow guarding
against a sysadmin who can just install a /etc/profile.d/git_wrapper
anyway that'll log everything you do with git, or even provide a custom
git binary, so it's always going to be left to their best judgement.

Re: [PATCH 2/2] add--interactive: detect bogus diffFilter output

[Junio C Hamano]

Ævar Arnfjörð Bjarmason  writes:

> On Sat, Mar 03 2018, Jeff King jotted:
>
>> +if (@colored && @colored != @diff) {
>
> nit: should just be:
>
> if (@colored != @diff) {
>
> It's not possible for @arrays in scalar context to be undefined.

It is true that @array can not be undef, but your rewrite I think is
wrong.

The first "do the comparison only @colored is true" is not about
definedness.  It is "@colored can be an empty array when the user is
not using separate 'show these colored lines to the end user, feed
these noncolored lines to git-apply command' feature, so @colored==0
and @diff > 0 is perfectly fine".

Re: Contributor Summit planning

[Ævar Arnfjörð Bjarmason]

On Mon, Mar 05 2018, Lars Schneider jotted:

>> On 03 Mar 2018, at 11:39, Jeff King  wrote:
>>
>> On Sat, Mar 03, 2018 at 05:30:10AM -0500, Jeff King wrote:
>>
>>> As in past years, I plan to run it like an unconference. Attendees are
>>> expected to bring topics for group discussion. Short presentations are
>>> also welcome. We'll put the topics on a whiteboard in the morning, and
>>> pick whichever ones people are interested in.
>>>
>>> Feel free to reply to this thread if you want to make plans or discuss
>>> any proposed topics before the summit. Input or questions from
>>> non-attendees is welcome here.
>>
>> I'll plan to offer two topics:
>>
>> - a round-up of the current state and past year's activities of Git as
>>   a member project of Software Freedom Conservancy
>>
>> - some updates on the state of the git-scm.com since my report last
>>   year
>>
>> As with last year, I'll try to send a written report to the list for
>> those who aren't at the summit in person.
>
> Thanks for starting this. I would like to propose the following topics:
>
> - hooks: Discuss a proposal for multiple local Git hooks of the same type.

I'm assuming you mean having stuff like pre-receive.d/* in addition to
pre-receive:

I had a WIP series for this that I didn't end up pursuing after getting
discouraged at:
https://public-inbox.org/git/cacbzzx6j6q2dun_z-pnent1u714dvnpfbrl_pieqylmczlu...@mail.gmail.com/

There's various bolt-on solutions that do this for subsets of the hooks,
e.g. GitLab now has this at
https://docs.gitlab.com/ee/administration/custom_hooks.html and this
stand-alone solution:
https://gist.github.com/carlos-jenkins/89da9dcf9e0d528ac978311938aade43

I still think this would be great to have, but Junio's objection being:

> Junio: And I have to say that a sequential execution that always
> Junio: short-circuits at the first failure is below that threshold.
> Junio:
> Junio: One reason I care about allowing the users to specify "do not
> Junio: shortcut" is that I anticipate that people would want to have a
> Junio: logging of the result at the end of the chain.

Got me discouraged, it would have made the implementation a bit more
complex, and I found other solutions to the problem I was trying to
solve.

Now we use Gitlab's implementation of this which has the semantics I
proposed at the time, and you just put log hooks at the beginning, but
of course that's server-side only. Having this be generally usable in
git would be great.

Re: Bug report: "Use of uninitialized value $_ in print"

[Jonathan Nieder]

Jeff King wrote:
> On Fri, Mar 02, 2018 at 09:15:44AM -0800, Junio C Hamano wrote:
>> Jeff King  writes:

>>> That's probably a reasonable sanity check, but I think we need to abort
>>> and not just have a too-small DISPLAY array. Because later code like the
>>> hunk-splitting is going to assume that there's a 1:1 line
>>> correspondence. We definitely don't want to end up in a situation where
>>> we show one thing but apply another.
>>
>> Yes, agreed completely.
>
> Let's add this sanity check while we're thinking about it. Here's a
> series.
>
>   [1/2]: t3701: add a test for interactive.diffFilter
>   [2/2]: add--interactive: detect bogus diffFilter output
>
>  git-add--interactive.perl  |  8 
>  t/t3701-add-interactive.sh | 20 
>  2 files changed, 28 insertions(+)

With or without the tweak Ævar Arnfjörð Bjarmason suggested,
Reviewed-by: Jonathan Nieder 

Thanks.  It's probably also worth adding Sam's reported-by to patch 2/2:
Reported-by: Sam Kuper 

Re: [PATCH v9 6/8] convert: check for detectable errors in UTF encodings

[Junio C Hamano]

lars.schnei...@autodesk.com writes:

> +static int validate_encoding(const char *path, const char *enc,
> +   const char *data, size_t len, int die_on_error)
> +{
> + if (!memcmp("UTF-", enc, 4)) {

Does the caller already know that enc is sufficiently long that
using memcmp is safe?

Re: [PATCH v3 12/35] serve: introduce git-serve

[Jonathan Nieder]

Hi,

Jeff King wrote:

> I agree that would be a lot more pleasant for adding protocol features.
> But I just worry that the stateful protocols get a lot less efficient.
> I'm having trouble coming up with an easy reproduction, but my
> recollection is that http has some nasty corner cases, because each
> round of "have" lines sent to the server has to summarize the previous
> conversation. So you can get a case where the client's requests keep
> getting bigger and bigger during the negotiation (and eventually getting
> large enough to cause problems).

That's not so much a corner case as just how negotiation works over
http.

We want to do better (e.g. see [1]) but that's a bigger change than
the initial protocol v2.

As Brandon explained it to me, we really do want to use stateless-rpc
semantics by default, since that's just better for maintainability.
Instead of having two protocols, one that is sane and one that
struggles to hoist that into stateless-rpc, there would be one
stateless baseline plus capabilities to make use of state.

For example, it would be nice to have a capability to remember
negotiation state between rounds, to get around exactly the problem
you're describing when using a stateful protocol.  Stateless backends
would just not advertise such a capability.  But doing that without [1]
still sort of feels like a cop-out.  If we can get a reasonable
baseline using ideas like [1] and then have a capability to keep
server-side state as icing on the cake instead of having a negotiation
process that only really makes sense when you have server-side state,
then that would be even better.

> If anything, I wish we could push the http protocol in a more stateful
> direction with something like websockets. But I suspect that's an
> unrealistic dream, just because not everybody's http setup (proxies,
> etc) will be able to handle that.

Agreed.  I think we have to continue to deal with stateless-rpc
semantics, at least for the near future.

Jonathan

[1] 
https://public-inbox.org/git/20180227054638.gb65...@aiede.svl.corp.google.com/

Re: t006 broken under Mac OS

[Junio C Hamano]

Jeff King  writes:

> Yeah, I think the ref_array_item.refname flex-parameter is not valid.
> This is the same issue Ramsay mentioned in:
>
>   
> https://public-inbox.org/git/58b2bdcd-d621-fd21-ab4d-6a9478319...@ramsayjones.plus.com/
>
> Junio, I think it probably makes sense to eject ot/cat-batch-format from
> pu for now. That series is on pause for a bit while Olga works on some
> other refactoring, and it's causing problems for people who test pu
> regularly.

Yup, thanks for a reminder.

Re: [PATCH 2/2] add--interactive: detect bogus diffFilter output

[Ævar Arnfjörð Bjarmason]

On Sat, Mar 03 2018, Jeff King jotted:

> + if (@colored && @colored != @diff) {

nit: should just be:

if (@colored != @diff) {

It's not possible for @arrays in scalar context to be undefined.

Re: [PATCH v3] run-command: add hint when a hook is ignored

[Junio C Hamano]

Jeff King  writes:

>> True.  The user could tell the server operator to rename them or
>> remove them because they are not doing anything useful, but then
>> as long as everybody knows they are not doing anything, it is OK
>> to leave that sleeping dog lie, as they are not doing anything
>> harmful anyway.
>> 
>> That brings us one step further back to question if the hints are
>> useful in the first place, though ;-).
>
> Yes, that last paragraph definitely crossed my mind. Do we have an
> opinion on doing anything here? (E.g., declaring the hints not worth the
> trouble and reverting them versus just living with it)?

I am tempted to declare that the "hints" was not a good idea and
suggest reverting them.

Re: [PATCH 00/11] Make the test suite pass with '-x' and /bin/sh

[SZEDER Gábor]

On Sat, Mar 3, 2018 at 8:12 AM, Jeff King  wrote:
> On Sat, Feb 24, 2018 at 12:39:40AM +0100, SZEDER Gábor wrote:
>
>> The first patch is the most important: with a couple of well-placed file
>> descriptor redirections it ensures that the stderr of the test helper
>> functions running git commands only contain the stderr of the tested
>> command, thereby resolving over 90% of the failures resulting from
>> running the test suite with '-x' and /bin/sh.
>
> I dunno. It seems like this requires a lot of caveats for people using
> subshells and shell functions, and I suspect it's going to be an
> on-going maintenance burden.

After finally figuring out the redirections in the first patch, I was
quite surprised by how few failing tests remained.  We only gathered 28
such tests over all these years; if it continues at this rate, that
probably won't be that much of a burden.  And the second patch provides
an escape hatch, should it ever be needed.

The current situation, however, is a burden much more frequently,
because the idiosyncrasies of TEST_SHELL_PATH and/or '--verbose-log' pop
up whenever trying to run any test script with '-x' that has such a test
in it.

I think this is the right tradeoff.

> That said, I'm not opposed if you want to do the work to try to get the
> whole test-suite clean, and we can see how it goes from there. It
> shouldn't be hurting anything, I don't think, aside from some
> mysterious-looking redirects (but your commit messages seem to explain
> it, so anybody can dig).
>
> Does it make descriptor 7 magical, and something that scripts should
> avoid touching? That would mean we have 2 magical descriptors now.

Tests can still use fd 7 as long as they don't intend to attach it
directly to that particular git command that is run inside one of these
test helper functions.

I settled on fd 7 because that fd is already used as stderr for the
'test_pause' and 'debug' helper functions and it isn't used in any of
our tests.

Re: [PATCH 2/2] add--interactive: detect bogus diffFilter output

[Jeff King]

On Mon, Mar 05, 2018 at 12:53:07PM -0800, Junio C Hamano wrote:

> Jeff King  writes:
> 
> > It's important that the diff-filter only filter the
> > individual lines, and that there remain a one-to-one mapping
> > between the input and output lines. Otherwise, things like
> > hunk-splitting will behave quite unexpectedly (e.g., you
> > think you are splitting at one point, but it has a different
> > effect in the text patch we apply).
> >
> > We can't detect all problematic cases, but we can at least
> > catch the obvious case where we don't even have the correct
> > number of lines.
> 
> Will queue.  We could probably also make sure that each of the
> corresponding line pair begins with the same '-/ /+' letter, but we
> need to draw a line and stop somewhere, and I think the number of
> lines is probably a good enough place.

I think that would break things like diff-so-fancy, which actually
removes the -/+ in favor of pure coloring (not something I care for
myself, but it seems a legitimate use case). So it's probably best not
to look at the content, not just from a "we can only go so far"
perspective but also because we actively don't know what the filter's
output is supposed to look like.

-Peff

Re: [PATCH 2/2] add--interactive: detect bogus diffFilter output

[Junio C Hamano]

Jeff King  writes:

> It's important that the diff-filter only filter the
> individual lines, and that there remain a one-to-one mapping
> between the input and output lines. Otherwise, things like
> hunk-splitting will behave quite unexpectedly (e.g., you
> think you are splitting at one point, but it has a different
> effect in the text patch we apply).
>
> We can't detect all problematic cases, but we can at least
> catch the obvious case where we don't even have the correct
> number of lines.

Will queue.  We could probably also make sure that each of the
corresponding line pair begins with the same '-/ /+' letter, but we
need to draw a line and stop somewhere, and I think the number of
lines is probably a good enough place.

>
> Signed-off-by: Jeff King 
> ---
>  git-add--interactive.perl  | 8 
>  t/t3701-add-interactive.sh | 8 
>  2 files changed, 16 insertions(+)
>
> diff --git a/git-add--interactive.perl b/git-add--interactive.perl
> index 964c3a7542..ff02008abe 100755
> --- a/git-add--interactive.perl
> +++ b/git-add--interactive.perl
> @@ -705,6 +705,14 @@ sub parse_diff {
>   }
>   my (@hunk) = { TEXT => [], DISPLAY => [], TYPE => 'header' };
>  
> + if (@colored && @colored != @diff) {
> + print STDERR
> +   "fatal: mismatched output from interactive.diffFilter\n",
> +   "hint: Your filter must maintain a one-to-one 
> correspondence\n",
> +   "hint: between its input and output lines.\n";
> + exit 1;
> + }
> +
>   for (my $i = 0; $i < @diff; $i++) {
>   if ($diff[$i] =~ /^@@ /) {
>   push @hunk, { TEXT => [], DISPLAY => [],
> diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh
> index 64fe56c3d5..9bb17f91b2 100755
> --- a/t/t3701-add-interactive.sh
> +++ b/t/t3701-add-interactive.sh
> @@ -404,6 +404,14 @@ test_expect_success TTY 'diffFilter filters diff' '
>   grep foo:.*content output
>  '
>  
> +test_expect_success TTY 'detect bogus diffFilter output' '
> + git reset --hard &&
> +
> + echo content >test &&
> + test_config interactive.diffFilter "echo too-short" &&
> + printf y | test_must_fail test_terminal git add -p
> +'
> +
>  test_expect_success 'patch-mode via -i prompts for files' '
>   git reset --hard &&

Re: [PATCH v3 12/35] serve: introduce git-serve

[Jeff King]

On Mon, Mar 05, 2018 at 10:43:21AM -0800, Brandon Williams wrote:

> In the current protocol http has a lot of additional stuff that's had to
> be done to it to get it to work with a protocol that was designed to be
> stateful first.  What I want is for the protocol to be designed
> stateless first so that http functions essentially the same as ssh or
> file or git transports and we don't have to do any hackery to get it to
> work.  This also makes it very simple to implement a new feature in the
> protocol because you only need to think about implementing it once
> instead of twice like you kind of have to do with v0.  So in the most
> recent series everything is a chain of request/response pairs even in
> the non-http cases.

I agree that would be a lot more pleasant for adding protocol features.
But I just worry that the stateful protocols get a lot less efficient.
I'm having trouble coming up with an easy reproduction, but my
recollection is that http has some nasty corner cases, because each
round of "have" lines sent to the server has to summarize the previous
conversation. So you can get a case where the client's requests keep
getting bigger and bigger during the negotiation (and eventually getting
large enough to cause problems).

If anything, I wish we could push the http protocol in a more stateful
direction with something like websockets. But I suspect that's an
unrealistic dream, just because not everybody's http setup (proxies,
etc) will be able to handle that.

-Peff

Re: [PATCH v4 13/35] ls-refs: introduce ls-refs server command

[Jeff King]

On Mon, Mar 05, 2018 at 10:29:14AM -0800, Jonathan Nieder wrote:

> >> It also accepts "refs/h*" to get "refs/heads" and "refs/hello".  I think
> >> it's worth going for the most-restrictive thing to start with, since
> >> that enables a lot more server operations without worrying about
> >> breaking compatibility.
> >
> > And just to clarify what do you see as being the most-restrictive case
> > of patterns that would should use?
> 
> Peff, can you say a little more about the downsides of accepting
> refs/h*?
> 
> IIRC the "git push" command already accepts such refspecs, so there's a
> benefit to accepting them.  Reftable and packed-refs support such
> queries about as efficiently as refs/heads/*.  For loose refs, readdir
> doesn't provide a way to restrict which files you look at, but loose
> refs are always slow anyway. :)
> 
> In other words, I see real benefits and I don't see much in the way of
> costs, so I'm not seeing why not to support this.

"git for-each-ref" only handles "/" boundaries. I think we used to have
similar problems with the internal for_each_ref(), but I just checked
and I think it's more flexible these days.  One could imagine a more
trie-like storage, though I agree that is stretching it with a
hypothetical.

Mostly my point was that I don't see any big upside, and the choice
seemed rather arbitrary. And as it is generally easier to loosen the
patterns later than tighten them, it makes sense to go with the tightest
option at first unless there is a compelling reason not to.

-Peff

Re: [PATCH v4 13/35] ls-refs: introduce ls-refs server command

[Jeff King]

On Mon, Mar 05, 2018 at 10:21:55AM -0800, Brandon Williams wrote:

> > Hmm, so this would accept stuff like "refs/heads/*/foo" but quietly
> > ignore the "/foo" part.
> 
> Yeah that's true...this should probably not do that.  Since
> "refs/heads/*/foo" violates what the spec is, really this should error
> out as an invalid pattern.

Yeah, that would be better, I think.

> > It also accepts "refs/h*" to get "refs/heads" and "refs/hello".  I think
> > it's worth going for the most-restrictive thing to start with, since
> > that enables a lot more server operations without worrying about
> > breaking compatibility.
> 
> And just to clarify what do you see as being the most-restrictive case
> of patterns that would should use?

I mean only accepting "*" at a "/" boundary (or just allowing a trailing
slash to imply recursion, like "refs/heads/", or even just always
assuming recursion to allow "refs/heads").

-Peff

Re: [RFC] Contributing to Git (on Windows)

[Junio C Hamano]

Johannes Schindelin  writes:

>> > "What if they say my code is not good enough?"
>> 
>> Sure, though there is something implied in what is Junio is saying
>> that is useful for such people.
>> 
>> It is patience.  It is the message that if you miss a portability bug,
>> we won't be disappointed in you, and it in fact happens all the time
>> to the best of contributors.
>> 
>> If there's a straightforward way to convey that in the text, I agree
>> with Junio that it's worth conveying.
>
> That is not how I read Junio's statement. I read it more like "If your
> code is flawed, we'll let you know."

I think you are talking about this part?

In fact, portability issues in a patch originally written for a
platform is rather quickly discovered if the original platform
is more minor than the others, so while it is good advice to
test your ware before you make it public, singling out the
portability issues may not add much value.

It's more like "Just like everybody else you are expected to be
imperfect, but those on list can and will help spot and fix if you
made mistakes.  Do not worry too much about things like portability
over to a system you usually do not work on."

The other side of the coin is that we are expected to be imperfect,
so even if your code is flawed, we may not even notice, so we may
not let you know.  It's mutual process in which we try to help each
other.

Re: [PATCH 2/2] git-svn: allow empty email-address in authors-prog and authors-file

[Eric Wong]

Andreas Heiduk  wrote:
> 2018-03-05 2:42 GMT+01:00 Eric Sunshine :
> > Doesn't such a behavior change deserve being documented (and possibly 
> > tests)?
> 
> The old behaviour was neither documented nor tested - the
> change did not break any test after all.

I consider that too low of a bar to justify a behavior change
which can negatively affect users.

Re: [PATCH 1/2] git-svn: search --authors-prog in PATH too

[Andreas Heiduk]

Am 05.03.2018 um 20:48 schrieb Andreas Heiduk:
> Am 05.03.2018 um 18:52 schrieb Eric Wong:
>> Thanks both, I've queued 1/2 up with Eric S's edits at svn/authors-prog.
>> I'm not yet convinced 2/2 is a good change, however.
> 
> I'm not sure which direction your argument points to: Do you object to a
> $PATH search at all? Or would you like to remove the legacy code too?
> Or is the code/doc/... in bad shape?

OK, I've mismatched your reply and the patch number. So my new questions
are:

Do you object to the feature as such? Or would you like to remove the
legacy code too? Or is the code/doc/... in bad shape beyond what E.S.
already mentioned?

Right now my team performs a hefty "git filter-branch" after each "git
svn fetch" followed by black magic to set the new object ids in the
caches / rev-maps of git-svn. Official support for "no synthetic email"
would be much easier.

[GSoC] [PATCH] travis-ci: added clang static analysis

[SiddharthaMishra]

Added a job to run clang static code analysis on the master and maint branch

Signed-off-by: SiddharthaMishra 
---
 .travis.yml   | 17 -
 ci/run-static-analysis.sh |  9 -
 2 files changed, 24 insertions(+), 2 deletions(-)

diff --git a/.travis.yml b/.travis.yml
index 4684b3f4f..9b891d182 100644
--- a/.travis.yml
+++ b/.travis.yml
@@ -48,7 +48,7 @@ matrix:
   before_install:
   before_script:
   script: ci/run-linux32-docker.sh
-- env: jobname=StaticAnalysis
+- env: jobname=CocciStaticAnalysis
   os: linux
   compiler:
   addons:
@@ -59,6 +59,21 @@ matrix:
   before_script:
   script: ci/run-static-analysis.sh
   after_failure:
+- if: branch IN (master, maint)
+  env: jobname=ClangStaticAnalysis
+  os: linux
+  compiler:
+  add_ons:
+apt:
+  sources:
+  - ubuntu-toolchain-r-test
+  - llvm-toolchain-trusty
+  packages:
+  - clang
+  before_install:
+  before_script:
+  script: ci/run-static-analysis.sh
+  after_failure:
 - env: jobname=Documentation
   os: linux
   compiler:
diff --git a/ci/run-static-analysis.sh b/ci/run-static-analysis.sh
index fe4ee4e06..6ae032f54 100755
--- a/ci/run-static-analysis.sh
+++ b/ci/run-static-analysis.sh
@@ -5,6 +5,13 @@
 
 . ${0%/*}/lib-travisci.sh
 
-make coccicheck
+case "$jobname" in
+ClangStaticAnalysis)
+   scan-build -analyze-headers --status-bugs make
+   ;;
+CocciStaticAnalysis)
+   make coccicheck
+   ;;
+esac
 
 save_good_tree
-- 
2.16.2.248.ge2408a6f7.dirty

Re: [PATCH 1/2] git-svn: search --authors-prog in PATH too

[Andreas Heiduk]

Am 05.03.2018 um 18:52 schrieb Eric Wong:
> 
> Thanks both, I've queued 1/2 up with Eric S's edits at svn/authors-prog.
> I'm not yet convinced 2/2 is a good change, however.

I'm not sure which direction your argument points to: Do you object to a
$PATH search at all? Or would you like to remove the legacy code too?
Or is the code/doc/... in bad shape?

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

[Brandon Williams]

On 03/02, Johannes Schindelin wrote:
> Hi Brandon,
> 
> On Wed, 28 Feb 2018, Brandon Williams wrote:
> 
> > diff --git a/remote-curl.c b/remote-curl.c
> > index 66a53f74b..3f882d766 100644
> > --- a/remote-curl.c
> > +++ b/remote-curl.c
> > @@ -188,7 +188,12 @@ static struct ref *parse_git_refs(struct discovery 
> > *heads, int for_push)
> > [...]
> > +static size_t proxy_in(char *buffer, size_t eltsize,
> > +  size_t nmemb, void *userdata)
> > +{
> > +   size_t max;
> > +   struct proxy_state *p = userdata;
> > +   size_t avail = p->request_buffer.len - p->pos;
> > +
> > +
> > +   if (eltsize != 1)
> > +   BUG("curl read callback called with size = %zu != 1", eltsize);
> 
> The format specified %z is not actually portable. Please use PRIuMAX and
> cast to (uintmax_t) instead.
> 
> This breaks the Windows build of `pu` (before that, there was still a test
> failure that I did not have the time to chase down).

Oh sorry, Looks like Junio put a patch ontop in pu to fix this.  I'll
squash that fix into this patch.

Thanks for catching this.

> 
> Ciao,
> Dscho

-- 
Brandon Williams

Re: [PATCH 00/44] reroll nd/remove-ignore-env.. sb/object-store and sb/packfiles..

[Junio C Hamano]

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

> 01/44 - 05/44: nd/remove-ignore-env-field
>
>   This series is moved up top. After this the patch that touch
>   alternate-db in sha1_file.c looks natural because no env is involved
>   anymore

Yes, I do like having this upfront and being able to merge it before
having to wait for the rest of the huge pile.

>   I also take this opportunity to introduce a new patch 01/44 to avoid
>   struct initialization that makes it hard to read and update. Later
>   patches are also simplified thanks to this.
>
> 06/44 - 32/44: sb/object-store
>
>   06/44 is updated to introduce raw_object_store_init() instead of
>   RAW_OBJECT_STORE_INIT macro. This function is now used to initialize
>   both main repo and submodule ones.
>
>   10/44 (moving "packed_git") also introduces two new access wrapper
>   get_packed_git() and get_packed_git_mru()

I haven't studied individual patches in this round for these, but
the interdiff you show below looks quite sensible.

Thanks, will take a bit deeper look at the rest.

Re: [PATCH v3 00/13] various perl fixes

[Junio C Hamano]

Ævar Arnfjörð Bjarmason   writes:

> Same as v2 except rebased on master & a couple of commit message
> fixes, thanks to Eric Sunshine (thanks!). tbdiff with v2:

Noting that the series was rebased is a good thing to do, but a
statement of the fact alone without justification makes readers
wonder if there was a valid reason why it was rebased, or it was
just because the 'master' saw more commits that do not have any
impact on what the series does.  For the former, saying something
like "A recently graduated $name topic added another instance of the
same kind of glitch this series addresses, so the series has been
rebased to cover that codepath as well." would make it very helpful.

Thanks.  Will queue.  After scanning it again, I think this is ready
for "next".

Re: [RFC] Contributing to Git (on Windows)

[Eric Sunshine]

On Mon, Mar 5, 2018 at 1:26 PM, Jonathan Nieder  wrote:
> Johannes Schindelin wrote:
>> The Google gang (you & Junio included) uses Linux. Peff uses Linux. From
>> what I can see Duy, Eric and Jake use Linux. That covers already the most
>> active reviewers right there.
>
> We are not as uniform as it may seem.  The Google gang all uses Linux
> on workstations but some use Mac laptops as well.  We deal with user
> reports all the time from all three popular platforms.

And, Eric uses Mac, not Linux, though he does test his submissions on
Linux and BSD VM's.

Re: Contributor Summit planning

[Jonathan Nieder]

Lars Schneider wrote:

> Thanks for starting this. I would like to propose the following topics:

Cool!  Do you mind starting threads for these so people who aren't there
can provide input into the discussion, too?  In other words, I'm
imagining

 1. Thread starts on mailing list

 2. Contributor summit: in-person presentation, discussion, etc lead to
people having better ideas

 3. On-list thread goes really well as a result of aforementioned
in-person discussion

Quick feedback:

> - hooks: Discuss a proposal for multiple local Git hooks of the same type.

I'd be happy to weigh in on a mailing list thread about this.  It's
also related to
https://public-inbox.org/git/20171002234517.gv19...@aiede.mtv.corp.google.com/
which is an interest of mine.

> - error reporting: Git is distributed and therefore lots of errors are only
>   reported locally. That makes it hard for administrators in larger
>   companies to see trouble. Would it make sense to add a config option that
>   would push recent errors along with "git push" to the server?

I'm interested in instrumentation but worried about the privacy
ramifications of this particular proposal.  I'd be happy to see some
built-in instrumentation hooks (or even a standard instrumentation
approach, if the mailing list comes up with a good one that respects
privacy).

> - fuzzing: Would it make sense to register Git to Google's OSS fuzzing
>   program https://github.com/google/oss-fuzz ?

Of course!

Alongside the obvious security benefit, there is money available to
support someone working on this:
https://opensource.googleblog.com/2017/05/oss-fuzz-five-months-later-and.html
https://www.google.com/about/appsecurity/patch-rewards/ clarifies that
the reward goes to the contributor, so you don't even necessarily have
to share your reward with the Git project. ;-)

Thanks,
Jonathan

Re: [PATCH v5 0/9] Correct offsets of hunks when one is skipped

[Junio C Hamano]

Phillip Wood  writes:

> From: Phillip Wood 
>
> I've updated these to clean up the perl style in response to Junio's
> comments on v4.

I've considered the use of "apply --recount" in this script (eh,
rather, the existence of that option in "apply" command itself ;-))
a bug that need to be eventually fixed for a long time, so the copy
of earlier rounds I've been carrying in my tree were forked from
'maint'.

I'll queue this round on the same base commit as before to replace;
I _think_ this is ready for 'next'.

Thanks for working on this.

Re: [PATCH v3 11/35] test-pkt-line: introduce a packet-line test helper

[Brandon Williams]

On 03/02, Jeff King wrote:
> On Fri, Feb 23, 2018 at 01:22:31PM -0800, Brandon Williams wrote:
> 
> > On 02/22, Stefan Beller wrote:
> > > On Tue, Feb 6, 2018 at 5:12 PM, Brandon Williams  
> > > wrote:
> > > 
> > > > +static void pack_line(const char *line)
> > > > +{
> > > > +   if (!strcmp(line, "") || !strcmp(line, "\n"))
> > > 
> > > From our in-office discussion:
> > > v1/v0 packs pktlines twice in http, which is not possible to
> > > construct using this test helper when using the same string
> > > for the packed and unpacked representation of flush and delim packets,
> > > i.e. test-pkt-line --pack $(test-pkt-line --pack ) would produce
> > > '' instead of '0009\n'.
> > > To fix it we'd have to replace the unpacked versions of these pkts to
> > > something else such as "FLUSH" "DELIM".
> > > 
> > > However as we do not anticipate the test helper to be used in further
> > > tests for v0, this ought to be no big issue.
> > > Maybe someone else cares though?
> > 
> > I'm going to punt and say, if someone cares enough they can update this
> > test-helper when they want to use it for v1/v0 stuff.
> 
> I recently add packetize and depacketize helpers for testing v0 streams;
> see 4414a15002 (t/lib-git-daemon: add network-protocol helpers,
> 2018-01-24). Is it worth folding these together?

I didn't know something like that existed! (of course if it was just
added this year then it didn't exist when I started working on this
stuff).  Yeah its probably a good idea to fold these together, I can
take a look at how your packetize and depacketize helpers work and add
the small amount of functionality that I'd need to replace the helper I
made.

> 
> -Peff

-- 
Brandon Williams

Re: [PATCH] t2028: fix minor error and issues in newly-added "worktree move" tests

[Eric Sunshine]

On Mon, Mar 5, 2018 at 1:37 PM, Junio C Hamano  wrote:
> SZEDER Gábor  writes:
>
>> Could you please save 'git worktree's output into an intermediate
>> file, and run 'grep' on the file's contents?
>
> Here is what I tentatively came up with, while deciding what should
> be queued based on Eric's patch, as a possible squash/fixup.

Thanks for saving a round-trip. One comment below...

> diff --git a/t/t2028-worktree-move.sh b/t/t2028-worktree-move.sh
> @@ -74,8 +75,10 @@ test_expect_success 'move worktree' '
> toplevel="$(pwd)" &&
> git worktree move source destination &&
> test_path_is_missing source &&
> -   git worktree list --porcelain | grep "^worktree.*/destination" &&
> -   ! git worktree list --porcelain | grep "^worktree.*/source" &&
> +   git worktree list --porcelain >out &&
> +   grep "^worktree.*/destination" out &&
> +   git worktree list --porcelain >out &&
> +   ! grep "^worktree.*/source" out &&

The second "git worktree list --porcelain >out" can be dropped,
leaving the two grep's back-to-back since it they can test the same
'out' file

> git -C destination log --format=%s >actual2 &&
> echo init >expected2 &&
> test_cmp expected2 actual2

Re: [PATCH v3 12/35] serve: introduce git-serve

[Brandon Williams]

On 03/02, Jeff King wrote:
> On Fri, Feb 23, 2018 at 01:45:57PM -0800, Brandon Williams wrote:
> 
> > I think this is the price of extending the protocol in a backward
> > compatible way.  If we don't want to be backwards compatible (allowing
> > for graceful fallback to v1) then we could design this differently.
> > Even so we're not completely out of luck just yet.
> > 
> > Back when I introduced the GIT_PROTOCOL side-channel I was able to
> > demonstrate that arbitrary data could be sent to the server and it would
> > only respect the stuff it knows about.  This means that we can do a
> > follow up to v2 at some point to introduce an optimization where we can
> > stuff a request into GIT_PROTOCOL and short-circuit the first round-trip
> > if the server supports it.
> 
> If that's our end-game, it does make me wonder if we'd be happier just
> jumping to that at first. Before you started the v2 protocol work, I had
> a rough patch series passing what I called "early capabilities". The
> idea was to let the client speak a few optional capabilities before the
> ref advertisement, and be ready for the server to ignore them
> completely. That doesn't clean up all the warts with the v0 protocol,
> but it handles the major one (allowing more efficient ref
> advertisements).

I didn't really want to get to that just yet, simply because I want to
try and keep the scope of this smaller while still being able to fix
most of the issues we have with v0.

> I dunno. There's a lot more going on here in v2 and I'm not sure I've
> fully digested it.

I tried to keep it similar enough to v0 such that it wouldn't be that
big of a leap (small steps).  For example negotiation is really done the
same as it is in v0 during fetch (a next step would be to actually
improve that).  We can definitely talk about all this in more detail
later this week too.

> 
> > The great thing about this is that from the POV of the git-client, it
> > doesn't care if its speaking using the git://, ssh://, file://, or
> > http:// transport; it's all the same protocol.  In my next re-roll I'll
> > even drop the "# service" bit from the http server response and then the
> > responses will truly be identical in all cases.
> 
> This part has me a little confused still. The big difference between
> http and the other protocols is that the other ones are full-duplex, and
> http is a series of stateless request/response pairs.
> 
> Are the other protocols becoming stateless request/response pairs, too?
> Or will they be "the same protocol" only in the sense of using the same
> transport?
> 
> (There are a lot of reasons not to like the stateless pair thing; it has
> some horrid corner cases during want/have negotiation).

Junio made a comment on the Spec in the most recent version of the
series about how I state that v2 is stateless and "MUST NOT" rely on
state being stored on the server side.  In reality I think this needs to
be tweaked a bit because when you do have a full-duplex connection you
may probably want to use that to reduce the amount of data that you send
in some cases.

In the current protocol http has a lot of additional stuff that's had to
be done to it to get it to work with a protocol that was designed to be
stateful first.  What I want is for the protocol to be designed
stateless first so that http functions essentially the same as ssh or
file or git transports and we don't have to do any hackery to get it to
work.  This also makes it very simple to implement a new feature in the
protocol because you only need to think about implementing it once
instead of twice like you kind of have to do with v0.  So in the most
recent series everything is a chain of request/response pairs even in
the non-http cases.

In a previous version of the series I had each command being able to
last any number of rounds and having a 'stateless' capability indicating
if the command needed to be run stateless.  I didn't think that was a
good design because by default you are still designing the stateful
thing first and the http (stateless) case can be an afterthought.  So
instead maybe we'll need commands which can benefit from state to have a
'stateful' feature that can be advertised when a full-duplex connection
is possible.  This still gives you the opportunity to not advertise that
and have the same behavior over ssh as http.  I actually remember
hearing someone talk about how they would like to allow for ssh
connections to their server and just have it be a proxy for http and
this would enable that.

-- 
Brandon Williams

Re: [PATCH] t2028: fix minor error and issues in newly-added "worktree move" tests

[Junio C Hamano]

SZEDER Gábor  writes:

> There is one more issue in these tests.
> ...
> The main purpose of this test script is to test the 'git worktree'
> command, but these pipes hide its exit code.
> Could you please save 'git worktree's output into an intermediate
> file, and run 'grep' on the file's contents?

Here is what I tentatively came up with, while deciding what should
be queued based on Eric's patch, as a possible squash/fixup.

 t/t2028-worktree-move.sh | 12 
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/t/t2028-worktree-move.sh b/t/t2028-worktree-move.sh
index d70d13dabe..1c391f370e 100755
--- a/t/t2028-worktree-move.sh
+++ b/t/t2028-worktree-move.sh
@@ -7,7 +7,8 @@ test_description='test git worktree move, remove, lock and 
unlock'
 test_expect_success 'setup' '
test_commit init &&
git worktree add source &&
-   git worktree list --porcelain | grep "^worktree" >actual &&
+   git worktree list --porcelain >out &&
+   grep "^worktree" out >actual &&
cat <<-EOF >expected &&
worktree $(pwd)
worktree $(pwd)/source
@@ -74,8 +75,10 @@ test_expect_success 'move worktree' '
toplevel="$(pwd)" &&
git worktree move source destination &&
test_path_is_missing source &&
-   git worktree list --porcelain | grep "^worktree.*/destination" &&
-   ! git worktree list --porcelain | grep "^worktree.*/source" &&
+   git worktree list --porcelain >out &&
+   grep "^worktree.*/destination" out &&
+   git worktree list --porcelain >out &&
+   ! grep "^worktree.*/source" out &&
git -C destination log --format=%s >actual2 &&
echo init >expected2 &&
test_cmp expected2 actual2
@@ -90,7 +93,8 @@ test_expect_success 'move worktree to another dir' '
git worktree move destination some-dir &&
test_when_finished "git worktree move some-dir/destination destination" 
&&
test_path_is_missing destination &&
-   git worktree list --porcelain | grep "^worktree.*/some-dir/destination" 
&&
+   git worktree list --porcelain >out &&
+   grep "^worktree.*/some-dir/destination" out &&
git -C some-dir/destination log --format=%s >actual2 &&
echo init >expected2 &&
test_cmp expected2 actual2

Re: Contributor Summit planning

[Lars Schneider]

> On 03 Mar 2018, at 11:39, Jeff King  wrote:
> 
> On Sat, Mar 03, 2018 at 05:30:10AM -0500, Jeff King wrote:
> 
>> As in past years, I plan to run it like an unconference. Attendees are
>> expected to bring topics for group discussion. Short presentations are
>> also welcome. We'll put the topics on a whiteboard in the morning, and
>> pick whichever ones people are interested in.
>> 
>> Feel free to reply to this thread if you want to make plans or discuss
>> any proposed topics before the summit. Input or questions from
>> non-attendees is welcome here.
> 
> I'll plan to offer two topics:
> 
> - a round-up of the current state and past year's activities of Git as
>   a member project of Software Freedom Conservancy
> 
> - some updates on the state of the git-scm.com since my report last
>   year
> 
> As with last year, I'll try to send a written report to the list for
> those who aren't at the summit in person.

Thanks for starting this. I would like to propose the following topics:

- hooks: Discuss a proposal for multiple local Git hooks of the same type.

- error reporting: Git is distributed and therefore lots of errors are only
  reported locally. That makes it hard for administrators in larger 
  companies to see trouble. Would it make sense to add a config option that 
  would push recent errors along with "git push" to the server?

- fuzzing: Would it make sense to register Git to Google's OSS fuzzing
  program https://github.com/google/oss-fuzz ?


- Lars

Re: [PATCH v4 13/35] ls-refs: introduce ls-refs server command

[Jonathan Nieder]

Hi,

On Mon, Mar 05, 2018 at 10:21:55AM -0800, Brandon Williams wrote:
> On 03/02, Jeff King wrote:

>> It also accepts "refs/h*" to get "refs/heads" and "refs/hello".  I think
>> it's worth going for the most-restrictive thing to start with, since
>> that enables a lot more server operations without worrying about
>> breaking compatibility.
>
> And just to clarify what do you see as being the most-restrictive case
> of patterns that would should use?

Peff, can you say a little more about the downsides of accepting
refs/h*?

IIRC the "git push" command already accepts such refspecs, so there's a
benefit to accepting them.  Reftable and packed-refs support such
queries about as efficiently as refs/heads/*.  For loose refs, readdir
doesn't provide a way to restrict which files you look at, but loose
refs are always slow anyway. :)

In other words, I see real benefits and I don't see much in the way of
costs, so I'm not seeing why not to support this.

Thanks,
Jonathan

Re: [RFC] Contributing to Git (on Windows)

[Jonathan Nieder]

Hi again,

Back on topic, some quick clarifications to tie up loose ends.

Also I want to thank you for continuing to push the project to work
better (especially to work better for newcomers).  We don't seem to
have a habit of saying often enough how important that goal is.  Of
course we'll disagree from time to time in minor ways about particular
aspects of how to change the development workflow, but the progress
you've already made (e.g. via tools like SubmitGit) is a huge deal.

[...]
Johannes Schindelin wrote:
> On Sat, 3 Mar 2018, Jonathan Nieder wrote:

>>  1. Approximately 1/2 of the differences you describe apply to Mac as
>> well as Windows.
>
> No. The executable bit, the native line endings, most of the issues I
> listed do not catch macOS-based developers off guard.

Yeah, my wording was really sloppy.

I meant that one of the differences you described half-applies to Mac
and the rest don't apply to Mac.  I should have proofread.

[...]
> Stolee agreed in the PR to mention alternatives to Hyper-V, such as
> VirtualBox, which would help macOS-based developers here.

I have no opinion about that (maybe it will make the text too long and
overwhelming, or maybe it will help people on balance).

[...]
> The Google gang (you & Junio included) uses Linux. Peff uses Linux. From
> what I can see Duy, Eric and Jake use Linux. That covers already the most
> active reviewers right there.

We are not as uniform as it may seem.  The Google gang all uses Linux
on workstations but some use Mac laptops as well.  We deal with user
reports all the time from all three popular platforms.

IIRC then Junio has a test setup that tests on Linux, FreeBSD, and
some other platforms.  IIRC Microsoft provides a way to run a Windows
VM for development purposes that he could use for testing in the same
way as he tests on FreeBSD if there are clear enough instructions for
doing it (hint, hint). :)

Of course it's even better if there is some public shared build/test
dashboard that we can all work together to at least keep green.

[...]
> So where is that formatter call that fixes your code?

There's "make style", and people still continue to work on improving
its configuration (thanks!).

[...]
> However, VSTS is free for teams up to five, meaning that any developer can
> register a project, and once I manage to get build definitions in shape, I
> can make them public and anybody can test their code on the major three
> platforms, in their personal VSTS account.

Thanks.  That sounds potentially useful.  (Though a shared dashboard
that we all keep green might be even more so.)

[...]
> So everything is a legal text.

Yeah.  In this context I should have instead said something like
"lawyer-reviewed text".

[...]
> Put another way: I indulged in veering off on a tangent. You know, like we
> do for fun here ;-)

Feel free to call me on it when my tangents are hurting, or when
they're helping for that matter.  That way I have training data to
improve my model of how to make a good tangent. :)

Sincerely,
Jonathan

Re: [PATCH v4 13/35] ls-refs: introduce ls-refs server command

[Brandon Williams]

On 03/02, Jeff King wrote:
> On Wed, Feb 28, 2018 at 03:22:30PM -0800, Brandon Williams wrote:
> 
> > +static void add_pattern(struct pattern_list *patterns, const char *pattern)
> > +{
> > +   struct ref_pattern p;
> > +   const char *wildcard;
> > +
> > +   p.pattern = strdup(pattern);
> 
> xstrdup?
> 
> > +   wildcard = strchr(pattern, '*');
> > +   if (wildcard) {
> > +   p.wildcard_pos = wildcard - pattern;
> > +   } else {
> > +   p.wildcard_pos = -1;
> > +   }
> 
> Hmm, so this would accept stuff like "refs/heads/*/foo" but quietly
> ignore the "/foo" part.

Yeah that's true...this should probably not do that.  Since
"refs/heads/*/foo" violates what the spec is, really this should error
out as an invalid pattern.

> 
> It also accepts "refs/h*" to get "refs/heads" and "refs/hello".  I think
> it's worth going for the most-restrictive thing to start with, since
> that enables a lot more server operations without worrying about
> breaking compatibility.

And just to clarify what do you see as being the most-restrictive case
of patterns that would should use?

-- 
Brandon Williams

Re: [RFC] Contributing to Git (on Windows)

[Jonathan Nieder]

Derrick Stolee wrote:
> Dereck Stolee wrote:
>
> nit: s/Dereck/Derrick/ Is my outgoing email name misspelled, or do you have
> a misspelled contact info for me?

A manual typo.  Sorry about that.

[... a bunch snipped ...]
> I have a habit of being too loose in language around lawyer-speak. I should
> not have attempted to summarize what "Signed-off-by:" means and will use
> that helpful link for the description instead.

No worries.  I make that kind of mistake all the time but just thought
it worth pointing out.

BTW, thanks again for writing and submitting this document.  It can't
land soon enough. :)

Jonathan

Re: [RFC] Contributing to Git (on Windows)

[Jonathan Nieder]

Hi Dscho,

Johannes Schindelin wrote:
> On Sat, 3 Mar 2018, Jonathan Nieder wrote:

>> Hopefully the clarifications and suggestions higher in this message
>> help.  If they don't, then I'm nervous about our ability to understand
>> each other.
>
> Okay, let me state what I think the goal of this document should be:
>
>   To help Windows-based developers who want to contribute their first
>   patch to the Git project.
>
> Whatever makes it easier to contributors and is easily explained, should
> go in, in my opinion.
>
> Wishful thinking, and philosophical considerations, should probably stay
> out.

I think this conversation has gone way off the rails.

I certainly share some blame for that: in
https://public-inbox.org/git/20180303182723.ga76...@aiede.svl.corp.google.com/
I let my emotions get the better of me and let my bafflement show
instead of focusing on my gratitude for the context and clarifications
you were providing.  And I am grateful for those.

What went wrong is that I somehow turned it into a debate.  That's not
the point of a patch review.  After all, we have the same goals for
this document!  So I am happy to continue to work with Derrick Stolee
(and anyone else interested) on whatever improvements he would like to
pursue, but I am going to bow out of the arguing with you part, if
that's okay.

Jonathan

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

[Johannes Schindelin]

Hi Phillip,

On Fri, 2 Mar 2018, Phillip Wood wrote:

> On 01/03/18 05:39, Sergey Organov wrote:
> > 
> > Igor Djordjevic  writes:
> > 
> >> On 28/02/2018 06:19, Sergey Organov wrote:
> >>>
> > (3) ---X1---o---o---o---o---o---X2
> >|\   |\
> >| A1---A2---A3---U1  | A1'--A2'--A3'--U1'
> >| \  |
> >|  M |
> >| /  |
> >\-B1---B2---B3---U2  \-B1'--B2'--B3'--U2'
> >
> 
>  Meh, I hope I`m rushing it now, but for example, if we had decided to 
>  drop commit A2 during an interactive rebase (so losing A2' from 
>  diagram above), wouldn`t U2' still introduce those changes back, once 
>  U1' and U2' are merged, being incorrect/unwanted behavior...? :/
> >>>
> >>> I think the method will handle this nicely.
> >>
> >> That`s what I thought as well. And then I made a test. And then the 
> >> test broke... Now, might be the test was flawed in the first place, 
> >> but thinking about it a bit more, it does seem to make sense not to 
> >> handle this case nicely :/
> > 
> > Yeah, I now see it myself. I'm sorry for being lazy and not inspecting
> > this more carefully in the first place.
> > 
> > [...]
> > 
> >> So while your original proposal currently seems like it could be 
> >> working nicely for non-interactive rebase (and might be some simpler 
> >> interactive ones), now hitting/acknowledging its first real use 
> >> limit, my additional quick attempt[1] just tries to aid pretty 
> >> interesting case of complicated interactive rebase, too, where we 
> >> might be able to do better as well, still using you original proposal 
> >> as a base idea :)
> > 
> > Yes, thank you for pushing me back to reality! :-) The work and thoughts
> > you are putting into solving the puzzle are greatly appreciated!
> > 
> > Thinking about it overnight, I now suspect that original proposal had a
> > mistake in the final merge step. I think that what you did is a way to
> > fix it, and I want to try to figure what exactly was wrong in the
> > original proposal and to find simpler way of doing it right.
> > 
> > The likely solution is to use original UM as a merge-base for final
> > 3-way merge of U1' and U2', but I'm not sure yet. Sounds pretty natural
> > though, as that's exactly UM from which both U1' and U2' have diverged
> > due to rebasing and other history editing.
> 
> Hi Sergey, I've been following this discussion from the sidelines,
> though I haven't had time to study all the posts in this thread in
> detail. I wonder if it would be helpful to think of rebasing a merge as
> merging the changes in the parents due to the rebase back into the
> original merge. So for a merge M with parents A B C that are rebased to
> A' B' C' the rebased merge M' would be constructed by (ignoring shell
> quoting issues)
> 
> git checkout --detach M
> git merge-recursive A -- M A'
> tree=$(git write-tree)
> git merge-recursive B -- $tree B'
> tree=$(git write-tree)
> git merge-recursive C -- $tree C'
> tree=$(git write-tree)
> M'=$(git log --pretty=%B -1 M | git commit-tree -pA' -pB' -pC')

(The $tree obviously wants to be passed as parameter to commit-tree, but
it's easy to get the idea.)

> This should pull in all the changes from the parents while preserving
> any evil conflict resolution in the original merge. It superficially
> reminds me of incremental merging [1] but it's so long since I looked at
> that I'm not sure if there are any significant similarities.

Interesting.

Basically, this is pretending that A'/B'/C' were not the result of
rebases, but of merges between A/B/C and upstream, and then performing an
octopus merge of M, A', B' and C'.

It is a beautiful application of the duality between merges and rebases:
ideally, they both result in the same tree [*1*].

That is a pretty clean and simple-to-describe paradigm to work off of, and
to reason about.

Ciao,
Dscho

Footnote *1*: I frequently use that duality in work to rebase "clean"
patches on top of upstream, and use that rebased branch to figure out how
to resolve merge conflicts when merging upstream into a long-running
branch (whose tree is identical to the pre-rebase version of the clean
patches). And yes, I *think* that this is essentially Michael Haggerty's
`git-imerge` idea.

Re: [PATCH 1/2] git-svn: search --authors-prog in PATH too

[Eric Wong]

Eric Sunshine  wrote:
> On Sun, Mar 4, 2018 at 6:22 AM, Andreas Heiduk  wrote:
> > In 36db1eddf9 ("git-svn: add --authors-prog option", 2009-05-14) the path
> > to authors-prog was made absolute because git-svn changes the current
> > directoy in some situations. This makes sense if the program is part of
> 
> s/directoy/directory/
> 
> > the repository but prevents searching via $PATH.
> >
> > The old behaviour is still retained, but if the file does not exists, then
> > authors-prog is search in $PATH as any other command.
> 
> s/search/searched for/
> 
> > Signed-off-by: Andreas Heiduk 

Thanks both, I've queued 1/2 up with Eric S's edits at svn/authors-prog.
I'm not yet convinced 2/2 is a good change, however.

The following changes since commit 7e31236f652ad9db221511eaf157ce0ef55585d6:

  Sixth batch for 2.17 (2018-02-28 13:39:24 -0800)

are available in the Git repository at:

  git://bogomips.org/git-svn.git svn/authors-prog

for you to fetch changes up to e99652c412701cf988770e5cfaa253712a39221a:

  git-svn: search --authors-prog in PATH too (2018-03-05 02:09:57 +)


Andreas Heiduk (1):
  git-svn: search --authors-prog in PATH too

 Documentation/git-svn.txt | 5 +
 git-svn.perl  | 3 ++-
 2 files changed, 7 insertions(+), 1 deletion(-)

Re: [FEATURE] git-gui: Staging path(s) should re-select a new path in "Unstaged Changes"

[Birger Skogeng Pedersen]

My apologies.

Seems there was some error causing this, I see now that paths are
automatically re-selected by default. Can't reproduce the error.

Please disregard this thread.


Birger

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

[Johannes Schindelin]

Hi Buga,

On Sat, 3 Mar 2018, Igor Djordjevic wrote:

> By the way, is there documentation for `git merge-recursive` 
> anywhere, besides the code itself...? :$

I am not aware of any. The commit message adding the command is not very
illuminating (https://github.com/git-for-windows/git/commit/720d150c4):

Add a new merge strategy by Fredrik Kuivinen.

I really wanted to try this out, instead of asking for an adjustment
to the 'git merge' driver and waiting.  For now the new strategy is
called 'fredrik' and not in the list of default strategies to be tried.

The script wants Python 2.4 so this commit also adjusts Debian and RPM
build procecure files.

Digging through https://public-inbox.org/git/ during that time frame comes
up with this hit, though:

https://public-inbox.org/git/20050907164734.ga20...@c165.ib.student.liu.se/

which is still not a good documentation of the algorithm. You can probably
dig further yourself, but I think I can describe it very quickly here:

To merge two commits recursively, you first have to find their "merge
bases". If there was an obvious branch point, then that is the merge base.
But when you start a branch off of master, then work a bit, then merge
master, you already have two merge bases.

The trick about the recursive merge is to reduce the number of merge bases
iteratively to one. It does that by taking two merge bases, and performing
a recursive merge on them, which generates a "virtual" commit, the
condensed merge base. That one is then merged recursively with the next
merge base, until there is only one left.

A recursive merge of two commits with exactly one merge base is simply a
three-way merge.

I vaguely remember that there was something funny about the order in which
order you want to process the merge bases: if you did it in one
(chronological) direction, it worked beautifully, in the other direction
it would generate tons of merge conflicts or something like that.

Ciao,
Johannes

[RFC PATCH 5/5] merge-recursive: improve handling for add/add conflicts

[Elijah Newren]

This makes add/add conflicts use the new handle_file_collision()
function.  This leaves the handling of the index the same, but
modifies how the working tree is handled: instead of always doing
a two-way merge of the file contents and recording them at the
collision path name, we instead first estimate the similarity of the
two files involved.  If they are not similar, we instead record the
file contents into two separate files for the user to inspect.

Several testcases had to be modified to either expect files to be
written to different locations, or for the two test colliding files
to be modified so that they were similar.

Signed-off-by: Elijah Newren 
---
 merge-recursive.c| 25 -
 t/t2023-checkout-m.sh|  2 +-
 t/t3418-rebase-continue.sh   | 27 +++
 t/t3504-cherry-pick-rerere.sh| 19 ++-
 t/t4200-rerere.sh| 12 ++--
 t/t6020-merge-df.sh  |  4 ++--
 t/t6024-recursive-merge.sh   | 35 +--
 t/t6025-merge-symlinks.sh|  9 ++---
 t/t6031-merge-filemode.sh|  4 ++--
 t/t6042-merge-rename-corner-cases.sh |  2 +-
 t/t6043-merge-rename-directories.sh  | 15 ---
 t/t7060-wtstatus.sh  |  1 +
 t/t7064-wtstatus-pv2.sh  |  4 ++--
 t/t7506-status-submodule.sh  | 11 +++
 t/t7610-mergetool.sh | 28 ++--
 15 files changed, 128 insertions(+), 70 deletions(-)

diff --git a/merge-recursive.c b/merge-recursive.c
index 96f0e9cee2..edba4fb11c 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -3070,11 +3070,26 @@ static int process_entry(struct merge_options *o,
clean_merge = -1;
}
} else if (a_oid && b_oid) {
-   /* Case C: Added in both (check for same permissions) and */
-   /* case D: Modified in both, but differently. */
-   clean_merge = merge_content(o, path, 0 /* file_in_way */,
-   o_oid, o_mode, a_oid, a_mode, 
b_oid, b_mode,
-   NULL);
+   if (!o_oid) {
+   /* Case C: Added in both (check for same permissions) */
+   output(o, 1,
+  _("CONFLICT (add/add): Merge conflict in %s"),
+  path);
+   clean_merge = handle_file_collision(o,
+   path, NULL, NULL,
+   o->branch1,
+   o->branch2,
+   a_oid, a_mode,
+   b_oid, b_mode,
+   0);
+   } else
+   /* case D: Modified in both, but differently. */
+   clean_merge = merge_content(o, path,
+   0 /* file_in_way */,
+   o_oid, o_mode,
+   a_oid, a_mode,
+   b_oid, b_mode,
+   NULL);
} else if (!o_oid && !a_oid && !b_oid) {
/*
 * this entry was deleted altogether. a_mode == 0 means
diff --git a/t/t2023-checkout-m.sh b/t/t2023-checkout-m.sh
index 7e18985134..2f8ea52318 100755
--- a/t/t2023-checkout-m.sh
+++ b/t/t2023-checkout-m.sh
@@ -27,7 +27,7 @@ clean_branchnames () {
 }
 
 test_expect_success '-m restores 2-way conflicted+resolved file' '
-   cp each.txt each.txt.conflicted &&
+   test_must_fail git merge-file -p each.txt~HEAD /dev/null 
each.txt~master >each.txt.conflicted &&
echo resolved >each.txt &&
git add each.txt &&
git checkout -m -- each.txt &&
diff --git a/t/t3418-rebase-continue.sh b/t/t3418-rebase-continue.sh
index 7c91a85f43..cf3a82d512 100755
--- a/t/t3418-rebase-continue.sh
+++ b/t/t3418-rebase-continue.sh
@@ -10,10 +10,17 @@ set_fake_editor
 
 test_expect_success 'setup' '
test_commit "commit-new-file-F1" F1 1 &&
-   test_commit "commit-new-file-F2" F2 2 &&
+   printf "1\n2\n2\n" >F2 &&
+   git add F2 &&
+   test_tick &&
+   git commit -m "commit-new-file-F2" &&
 
git checkout -b topic HEAD^ &&
-   test_commit "commit-new-file-F2-on-topic-branch" F2 22 &&
+   printf "1\n2\n22\n" >F2 &&
+   git add F2 &&
+   test_tick &&
+   git commit -m "commit-new-file-F2-on-topic-branch" &&
+   git tag "commit-new-file-F2-on-topic-branch" &&
 
git checkout master
 '
@@ -48,7 +55,13 @@ 

[RFC PATCH 3/5] merge-recursive: fix rename/add conflict handling

[Elijah Newren]

This makes the rename/add conflict handling make use of the new
handle_file_collision() function, which fixes several bugs and improves
things for the rename/add case significantly.  Previously, rename/add
would:

  * Not leave any higher order stage entries in the index, making it
appear as if there were no conflict.
  * Would place the rename file at the colliding path, and move the
added file elsewhere, which combined with the lack of higher order
stage entries felt really odd.  It's not clear to me why the
rename should take precedence over the add; if one should be moved
out of the way, they both probably should.
  * In the recursive case, it would do a two way merge of the added
file and the version of the renamed file on the renamed side,
completely excluding modifications to the renamed file on the
unrenamed side of history.

Using the new handle_file_collision() fixes all of these issues, and
adds smarts to allow two-way merge OR move colliding files to separate
paths depending on the similarity of the colliding files.

Signed-off-by: Elijah Newren 
---
 merge-recursive.c | 137 +++---
 t/t6036-recursive-corner-cases.sh |  19 +++---
 2 files changed, 108 insertions(+), 48 deletions(-)

diff --git a/merge-recursive.c b/merge-recursive.c
index c54b918dc8..403c0006dc 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -181,6 +181,7 @@ static int oid_eq(const struct object_id *a, const struct 
object_id *b)
 enum rename_type {
RENAME_NORMAL = 0,
RENAME_DIR,
+   RENAME_ADD,
RENAME_DELETE,
RENAME_ONE_FILE_TO_ONE,
RENAME_ONE_FILE_TO_TWO,
@@ -223,6 +224,7 @@ static inline void setup_rename_conflict_info(enum 
rename_type rename_type,
  struct stage_data *src_entry1,
  struct stage_data *src_entry2)
 {
+   int ostage1, ostage2;
struct rename_conflict_info *ci = xcalloc(1, sizeof(struct 
rename_conflict_info));
ci->rename_type = rename_type;
ci->pair1 = pair1;
@@ -240,18 +242,22 @@ static inline void setup_rename_conflict_info(enum 
rename_type rename_type,
dst_entry2->rename_conflict_info = ci;
}
 
-   if (rename_type == RENAME_TWO_FILES_TO_ONE) {
-   /*
-* For each rename, there could have been
-* modifications on the side of history where that
-* file was not renamed.
-*/
-   int ostage1 = o->branch1 == branch1 ? 3 : 2;
-   int ostage2 = ostage1 ^ 1;
+   /*
+* For each rename, there could have been
+* modifications on the side of history where that
+* file was not renamed.
+*/
+   if (rename_type == RENAME_ADD ||
+   rename_type == RENAME_TWO_FILES_TO_ONE) {
+   ostage1 = o->branch1 == branch1 ? 3 : 2;
 
ci->ren1_other.path = pair1->one->path;
oidcpy(>ren1_other.oid, _entry1->stages[ostage1].oid);
ci->ren1_other.mode = src_entry1->stages[ostage1].mode;
+   }
+
+   if (rename_type == RENAME_TWO_FILES_TO_ONE) {
+   ostage2 = ostage1 ^ 1;
 
ci->ren2_other.path = pair2->one->path;
oidcpy(>ren2_other.oid, _entry2->stages[ostage2].oid);
@@ -1119,6 +1125,18 @@ static int merge_file_special_markers(struct 
merge_options *o,
char *side2 = NULL;
int ret;
 
+   if (o->branch1 != branch1) {
+   /*
+* It's weird getting a reverse merge with HEAD on the bottom
+* and the other branch on the top.  Fix that.
+*/
+   return merge_file_special_markers(o,
+ one, b, a,
+ branch2, filename2,
+ branch1, filename1,
+ mfi);
+   }
+
if (filename1)
side1 = xstrfmt("%s:%s", branch1, filename1);
if (filename2)
@@ -1290,7 +1308,6 @@ static struct diff_filespec *filespec_from_entry(struct 
diff_filespec *target,
return target;
 }
 
-#if 0 // #if-0-ing avoids unused function warning; will make live in next 
commit
 static int handle_file_collision(struct merge_options *o,
 const char *collide_path,
 const char *prev_path1,
@@ -1307,6 +1324,21 @@ static int handle_file_collision(struct merge_options *o,
int minimum_score;
char *new_path1, *new_path2;
 
+   /*
+* It's easiest to get the correct things into stage 2 and 3, and
+* to make sure that the content merge puts HEAD before the other
+* branch if we just ensure that branch1 == o->branch1.  So, simply
+* 

[RFC PATCH 2/5] merge-recursive: new function for better colliding conflict resolutions

[Elijah Newren]

There are three conflict types that represent two (possibly entirely
unrelated) files colliding at the same location:
  * add/add
  * rename/add
  * rename/rename(2to1)

These three conflict types already share more similarity than might be
immediately apparent from their description: (1) the handling of the
rename variants already involves removing any entries from the index
corresponding to the original file names[*], thus only leaving entries
in the index for the colliding path; (2) likewise, any trace of the
original file name in the working tree is also removed.  So, in all
three cases we're left with how to represent two colliding files in both
the index and the working copy.

[*] Technically, this isn't quite true because rename/rename(2to1)
conflicts in the recursive (o->call_depth > 0) case do an "unrename"
since about seven years ago.  But even in that case, Junio felt
compelled to explain that my decision to "unrename" wasn't necessarily
the only or right answer -- search for "Comment from Junio" in t6036 for
details.

My initial motivation for looking at these three conflict types was that
if the handling of these three conflict types is the same, at least in
the limited set of cases where a renamed file is unmodified on the side
of history where the file is not renamed, then a significant performance
improvement for rename detection during merges is possible.  However,
while that served as motivation to look at these three types of
conflicts, the actual goal of this new function is to try to improve the
handling for all three cases, not to merely make them the same as each
other in that special circumstance.

=== Handling the working tree ===

The previous behavior for these conflict types in regards to the
working tree (assuming the file collision occurs at 'foo') was:
  * add/add does a two-way merge of the two files and records it as 'foo'.
  * rename/rename(2to1) records the two different files into two new
uniquely named files (foo~HEAD and foo~$MERGE), while removing 'foo'
from the working tree.
  * rename/add records the two different files into two different
locations, recording the add at foo~$SIDE and, oddly, recording
the rename at foo (why is the rename more important than the add?)

So, the question for what to write to the working tree boils down to
whether the two colliding files should be two-way merged and recorded in
place, or recorded into separate files.  If the files are similar enough,
the two-way merge is probably preferable, but if they're not similar,
recording as separate files is probably preferable.  (The same logic that
applies for the working directory here would also apply to the recursive
case, i.e. the o->call_depth > 0 case, as well.)  The code handling the
different types of conflicts appear to have been written with different
assumptions about whether the colliding files would be similar.

But, rather than make an assumption about whether the two files will be
similar, why not just check?  If we simply call estimate_similarity(),
we can two-way merge the files if they are similar, and otherwise record
the two files at different locations.

=== Handling of the index ===

For a typical rename, unpack_trees() would set up the index in the
following fashion:
   old_path  new_path
   stage1: 5ca1ab1e  
   stage2: f005ba11  
   stage3:   b0a710ad
And merge-recursive would rewrite this to
   new_path
   stage1: 5ca1ab1e
   stage2: f005ba11
   stage3: b0a710ad
Removing old_path from the index means the user won't have to `git rm
old_path` manually every time a renamed path has a content conflict.
It also means they can use `git checkout [--ours|--theirs|--conflict|-m]
new_path`, `git diff [--ours|--theirs]` and various other commands that
would be difficult otherwise.

This strategy becomes a problem when we have a rename/add or
rename/rename(2to1) conflict, however, because then we have only three
slots to store blob sha1s and we need either four or six.  Previously,
this was handled by continuing to delete old_path from the index, and
just outright ignoring any blob shas from old_path.  That had the
downside of deleting any trace of changes made to old_path on the other
side of history.  This function instead does a three-way content merge of
the renamed file, and stores the blob sha1 for that at either stage2 or
stage3 for new_path (depending on which side the rename came from).  That
has the advantage of bringing information about changes on both sides and
still allows for easy resolution (no need to git rm old_path, etc.), but
does have the downside that if the content merge had conflict markers,
then what we store in the index is the sha1 of a blob with conflict
markers.  While that is a downside, it seems less problematic than the
downsides of any obvious alternatives, and certainly makes more sense
than the previous handling.

Signed-off-by: Elijah Newren 
---
 diff.h|   4 

[RFC PATCH 4/5] merge-recursive: improve handling for rename/rename(2to1) conflicts

[Elijah Newren]

This makes the rename/rename(2to1) conflicts use the new
handle_file_collision() function.  Since that function was based
originally on the rename/rename(2to1) handling code, the main
differences here are in what was added.  In particular:

  * If the two colliding files are similar, instead of being stored
at collide_path~HEAD and collide_path~MERGE, the files are two-way
merged and recorded at collide_path.
  * Instead of recording the version of the renamed file that existed
on the renamed side in the index (thus ignoring any changes that
were made to the file on the side of history without the rename),
we do a three-way content merge on the renamed path, then store
that at either stage 2 or stage 3.
  * Note that if either of the three-way content merges done for each
rename have conflicts, we do NOT try to estimate the similarity of
the resulting two files and just automatically consider them to be
dissimilar.  This is done to avoid foisting conflicts-of-conflicts
on the user.

Signed-off-by: Elijah Newren 
---
 merge-recursive.c| 101 +--
 t/t6042-merge-rename-corner-cases.sh |   2 +-
 2 files changed, 14 insertions(+), 89 deletions(-)

diff --git a/merge-recursive.c b/merge-recursive.c
index 403c0006dc..96f0e9cee2 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -657,27 +657,6 @@ static int update_stages(struct merge_options *opt, const 
char *path,
return 0;
 }
 
-static int update_stages_for_stage_data(struct merge_options *opt,
-   const char *path,
-   const struct stage_data *stage_data)
-{
-   struct diff_filespec o, a, b;
-
-   o.mode = stage_data->stages[1].mode;
-   oidcpy(, _data->stages[1].oid);
-
-   a.mode = stage_data->stages[2].mode;
-   oidcpy(, _data->stages[2].oid);
-
-   b.mode = stage_data->stages[3].mode;
-   oidcpy(, _data->stages[3].oid);
-
-   return update_stages(opt, path,
-is_null_oid() ? NULL : ,
-is_null_oid() ? NULL : ,
-is_null_oid() ? NULL : );
-}
-
 static void update_entry(struct stage_data *entry,
 struct diff_filespec *o,
 struct diff_filespec *a,
@@ -1615,7 +1594,6 @@ static int conflict_rename_rename_2to1(struct 
merge_options *o,
char *path = c1->path; /* == c2->path */
struct merge_file_info mfi_c1;
struct merge_file_info mfi_c2;
-   int ret;
 
output(o, 1, _("CONFLICT (rename/rename): "
   "Rename %s->%s in %s. "
@@ -1623,9 +1601,6 @@ static int conflict_rename_rename_2to1(struct 
merge_options *o,
   a->path, c1->path, ci->branch1,
   b->path, c2->path, ci->branch2);
 
-   remove_file(o, 1, a->path, o->call_depth || 
would_lose_untracked(a->path));
-   remove_file(o, 1, b->path, o->call_depth || 
would_lose_untracked(b->path));
-
if (merge_file_special_markers(o, a, c1, >ren1_other,
   o->branch1, c1->path,
   o->branch2, ci->ren1_other.path, 
_c1) ||
@@ -1634,66 +1609,11 @@ static int conflict_rename_rename_2to1(struct 
merge_options *o,
   o->branch2, c2->path, _c2))
return -1;
 
-   if (o->call_depth) {
-   /*
-* If mfi_c1.clean && mfi_c2.clean, then it might make
-* sense to do a two-way merge of those results.  But, I
-* think in all cases, it makes sense to have the virtual
-* merge base just undo the renames; they can be detected
-* again later for the non-recursive merge.
-*/
-   remove_file(o, 0, path, 0);
-   ret = update_file(o, 0, _c1.oid, mfi_c1.mode, a->path);
-   if (!ret)
-   ret = update_file(o, 0, _c2.oid, mfi_c2.mode,
- b->path);
-   } else {
-   char *new_path1 = unique_path(o, path, ci->branch1);
-   char *new_path2 = unique_path(o, path, ci->branch2);
-   output(o, 1, _("Renaming %s to %s and %s to %s instead"),
-  a->path, new_path1, b->path, new_path2);
-   if (was_dirty(o, path))
-   output(o, 1, _("Refusing to lose dirty file at %s"),
-  path);
-   else if (would_lose_untracked(path))
-   /*
-* Only way we get here is if both renames were from
-* a directory rename AND user had an untracked file
-* at the location where both files end up after the
-* two directory renames.  See testcase 10d of t6043.
- 

[RFC PATCH 1/5] Add testcases for improved file collision conflict handling

[Elijah Newren]

Adds testcases dealing with file collisions for the following types of
conflicts:
  * add/add
  * rename/add
  * rename/rename(2to1)
These tests include expectations for proposed smarter behavior which has
not yet been implemented and thus are currently expected to fail.
Subsequent commits will correct that and explain the new behavior.

Signed-off-by: Elijah Newren 
---
 t/t6042-merge-rename-corner-cases.sh | 220 +++
 1 file changed, 220 insertions(+)

diff --git a/t/t6042-merge-rename-corner-cases.sh 
b/t/t6042-merge-rename-corner-cases.sh
index 411550d2b6..a6c151ef95 100755
--- a/t/t6042-merge-rename-corner-cases.sh
+++ b/t/t6042-merge-rename-corner-cases.sh
@@ -575,4 +575,224 @@ test_expect_success 'rename/rename/add-dest merge still 
knows about conflicting
test ! -f c
 '
 
+test_conflicts_with_adds_and_renames() {
+   test $1 != 0 && side1=rename || side1=add
+   test $2 != 0 && side2=rename || side2=add
+
+   # Setup:
+   #  L
+   # / \
+   #   master   ?
+   # \ /
+   #  R
+   #
+   # Where:
+   #   Both L and R have files named 'three-unrelated' and
+   #   'three-related' which collide (i.e. 4 files colliding at two
+   #   pathnames).  Each of the colliding files could have been
+   #   involved in a rename, in which case there was a file named
+   #   'one-[un]related' or 'two-[un]related' that was modified on the
+   #   opposite side of history and renamed into the collision on this
+   #   side of history.
+   #
+   # Questions for both sets of collisions:
+   #   1) The index should contain both a stage 2 and stage 3 entry
+   #  for the colliding file.  Does it?
+   #   2) When renames are involved, the content merges are clean, so
+   #  the index should reflect the content merges, not merely the
+   #  version of the colliding file from the prior commit.  Does
+   #  it?
+   #
+   # Questions for three-unrelated:
+   #   3) There should be files in the worktree named
+   #  'three-unrelated~HEAD' and 'three-unrelated~R^0' with the
+   #  (content-merged) version of 'three-unrelated' from the
+   #  appropriate side of the merge.  Are they present?
+   #   4) There should be no file named 'three-unrelated' in the
+   #  working tree.  That'd make it too likely that users would
+   #  use it instead of carefully looking at both
+   #  three-unrelated~HEAD and three-unrelated~R^0.  Is it
+   #  correctly missing?
+   #
+   # Questions for three-related:
+   #   3) There should be a file in the worktree named three-related
+   #  containing the two-way merged contents of the content-merged
+   #  versions of three-related from each of the two colliding
+   #  files.  Is it present?
+   #   4) There should not be any three-related~* files in the working
+   #  tree.
+   test_expect_success "setup simple $side1/$side2 conflict" '
+   test_create_repo simple_${side1}_${side2} &&
+   (
+   cd simple_${side1}_${side2} &&
+
+   # Create a simple file with 10 lines
+   ten="0 1 2 3 4 5 6 7 8 9" &&
+   for i in $ten
+   do
+   echo line $i in a sample file
+   done >unrelated1_v1 &&
+   # Create a 2nd version of same file with one more line
+   cat unrelated1_v1 >unrelated1_v2 &&
+   echo another line >>unrelated1_v2 &&
+
+   # Create an unrelated simple file with 10 lines
+   for i in $ten
+   do
+   echo line $i in another sample file
+   done >unrelated2_v1 &&
+   # Create a 2nd version of same file with one more line
+   cat unrelated2_v1 >unrelated2_v2 &&
+   echo another line >>unrelated2_v2 &&
+
+   # Create some related files now
+   for i in $ten
+   do
+   echo Random base content line $i
+   done >related1_v1 &&
+   cp -a related1_v1 related1_v2 &&
+   echo modification >>related1_v2 &&
+
+   cp -a related1_v1 related2_v1 &&
+   echo more stuff >>related2_v1 &&
+   cp -a related2_v1 related2_v2 &&
+   echo yet more stuff >>related2_v2 &&
+
+   # Use a tag to record both these files for simple
+   # access, and clean out these untracked files
+   git tag unrelated1_v1 `git hash-object -w 

[RFC PATCH 0/5] Improve path collision conflict resolutions

[Elijah Newren]

This series improves conflict resolutions for conflict types that involve
two (possibly entirely unrelated) files colliding at the same path.  These
conflict types are:
  * add/add
  * rename/add
  * rename/rename(2to1)

Improving these conflict types has some subtle (though significant)
performance ramifications, but for now, I am just concentrating on
providing the user with better information for their conflict resolution.
Performance considerations will be addressed in a future patch series.

Before mentioning the improvements, it may be worth noting that these three
types are actually more similar than might at first be apparent: for the
cases involving renames, any trace of the rename-source path is deleted
from both the index and the working copy (modulo a small technicality
mentioned in patch 2), leaving just the question of how to represent two
colliding files in both the index and the working copy for all three cases.

There are three important changes this patch series introduces:

  1) Consolidating the code for these three types of conflict resolutions
 into a single function that all three code paths can call.

  2) Doing content merges for a rename before looking at the path collision
 between a rename and some other file.  In particular (in what I most
 suspect others might have an objection to from this series), record
 that content-merged file -- which may have conflict markers -- in the
 index at the appropriate higher stage.

  3) Smarter behavior for recording the conflict in the working tree: first
 checking whether the two colliding files are similar, and then based
 on that, deciding whether to handle the path collision via a two-way
 merge of the different files or to instead record the two different
 files at separate temporary paths.

In more detail:

1)

The consolidation seems fairly self-explanatory, but it had a bigger effect
than you'd expect: it made it clear that the rename/add conflict resolution
is broken in multiple ways, and it also made it easier to reason about what
_should_ be done for rename/add conflicts (something I had struggled with
when I looked at that particular conflict type in the past).  See patch 3
for more details.

Sidenote: I was kind of surprised that rename/add could have been broken
for this long, unnoticed.  Does no one ever hit that conflict in real life?
It looks like we did not have very good test coverage for rename/add
conflicts; a brief search seems to show that we only had a few testcases
triggering that conflict type, and all of them were added by me.  Patch 1
tries to address the testcase problem by adding some tests that try to
check the index and working copy more strictly for all three conflict
types.

2)

Previously, rename/rename(2to1) conflict resolution for the colliding path
would just accept the index changes made by unpack_trees(), meaning that
each of the higher order stages in the index for the path collision would
implicitly ignore any changes to each renamed file from the other side of
history.  Since, as noted above, all traces of the rename-source path were
removed from both the index and the working tree, this meant that the index
was missing information about changes to such files.  If the user tried to
resolve the conflict using the index rather than the working copy, they
would end up with a silent loss of changes.

I "fixed" this by doing the three-way content merge for each renamed-file,
and then recorded THAT in the index at either stage 2 or 3 as appropriate.
Since that merge might have conflict markers, that could mean recording in
the index a file with conflict markers as though it were a given side.
(See patch 2 for a more detailed explanation.)  I figure this might be the
most controversial change I made.  I can think of a few alternatives, but I
liked all of them less.  Opinions?

This change did not require any significant changes to the testsuite; the
difference between the old and new behavior was essentially untested.

(rename/add was even worse: not recording _any_ higher order stages in the
index, and thus partially hiding the fact that the path was involved in a
conflict at all.)

3)

Given the similarity between the conflict types, the big question for
handling the conflict in the working tree was whether the two colliding
files should be two-way merged and recorded in place (as add/add does,
which seems to be preferable if the two files are similar), or whether the
two files should be recorded into separate files (as rename/add and
rename/rename(2to1) do, which seems to be preferable if the two files are
dissimilar).  The code handling the different types of conflicts appear to
have been written with different assumptions about whether the colliding
files would be similar.

But, rather than make an assumption about whether the two files will be
similar, why not just check and then make the best choice based on that?
Thus, this code makes use of 

Re: Contributor Summit planning

[Brandon Williams]

On 03/03, Jeff King wrote:
> On Sat, Mar 03, 2018 at 05:30:10AM -0500, Jeff King wrote:
> 
> > As in past years, I plan to run it like an unconference. Attendees are
> > expected to bring topics for group discussion. Short presentations are
> > also welcome. We'll put the topics on a whiteboard in the morning, and
> > pick whichever ones people are interested in.
> > 
> > Feel free to reply to this thread if you want to make plans or discuss
> > any proposed topics before the summit. Input or questions from
> > non-attendees is welcome here.
> 
> I'll plan to offer two topics:
> 
>  - a round-up of the current state and past year's activities of Git as
>a member project of Software Freedom Conservancy
> 
>  - some updates on the state of the git-scm.com since my report last
>year
> 
> As with last year, I'll try to send a written report to the list for
> those who aren't at the summit in person.

Thanks for kicking things off!

Since I've been working on protocol stuff I'd like to spend a bit of
time discussing protocol v2 :)

-- 
Brandon Williams

Re: [PATCH] git-gui: Add hotkeys to change focus between ui widgets

[Johannes Schindelin]

Hi Birger,

On Wed, 28 Feb 2018, Birger Skogeng Pedersen wrote:

> The user cannot change focus between the list of files, the diff view
> and the commit message widgets without using the mouse (clicking either of
> the four widgets ).
> 
> Hotkeys CTRL/CMD+number (1-4) now focuses the first file of either the
> "Unstaged Changes" or "Staged Changes", the diff view or the
> commit message dialog widgets, respectively. This enables the user to
> select/unselect files, view the diff and create a commit in git-gui
> using keyboard-only.

I like this!

> diff --git a/git-gui/git-gui.sh b/git-gui/git-gui.sh
> index 91c00e648..f96c0a6b8 100755
> 
> (This is my first patch ever, any feedback is highly appreciated)

I am not an expert in Tcl/Tk, but I'll do my best to comment on this
patch.

> --- a/git-gui/git-gui.sh
> +++ b/git-gui/git-gui.sh
> @@ -2664,6 +2664,38 @@ proc show_less_context {} {
>   }
>  }
>  
> +proc select_first_path {w} {
> + global file_lists last_clicked selected_paths
> + if {[llength $file_lists($w)] > 0} {
> + focus $w
> + set last_clicked [list $w 1]
> + set path [lindex $file_lists($w) 0]
> + array unset selected_paths
> + set selected_paths($path) 1
> + show_diff $path $w
> + }
> +}

Do you think there is a way to focus on the last-selected path? That would
make this feature even more convenient, I think.

I am not sure that this information is still there if switching back from
another component...

> +proc select_first_unstaged_changes_path {} {
> + global ui_workdir
> + select_first_path $ui_workdir
> +}
> +
> +proc select_first_staged_changes_path {} {
> + global ui_index
> + select_first_path $ui_index
> +}
> +
> +proc focus_diff {} {
> + global ui_diff
> + focus $ui_diff
> +}
> +
> +proc focus_commit_message {} {
> + global ui_comm
> + focus $ui_comm
> +}
> +
>  ##
>  ##
>  ## ui construction
> @@ -3876,6 +3908,11 @@ foreach i [list $ui_index $ui_workdir] {
>  }
>  unset i
>  
> +bind . <$M1B-Key-1> {select_first_unstaged_changes_path}
> +bind . <$M1B-Key-2> {select_first_staged_changes_path}
> +bind . <$M1B-Key-3> {focus_diff}
> +bind . <$M1B-Key-4> {focus_commit_message}
> +
>  set file_lists($ui_index) [list]
>  set file_lists($ui_workdir) [list]

Looks good!

We are currently without an active Git GUI maintainer, so I hope that
Junio (the Git maintainer) will pick this up.

Ciao,
Johannes

Re: [RFC] Contributing to Git (on Windows)

[Johannes Schindelin]

Hi Jonathan,

On Sat, 3 Mar 2018, Jonathan Nieder wrote:

> Johannes Schindelin wrote:
> >> Jonathan Nieder  writes:
> >>> Dereck Stolee wrote:
> 
>  +Test Your Changes on Linux
>  +--
>  +
>  +It can be important to work directly on the [core Git
>  codebase](https://github.com/git/git), +such as a recent commit into
>  the `master` or `next` branch that has not been incorporated +into
>  Git for Windows. Also, it can help to run functional and performance
>  tests on your +code in Linux before submitting patches to the
>  Linux-focused mailing list.
> >>>
> >>> I'm surprised at this advice.  Does it actually come up?
> >
> > Yes.
> >
> > I personally set up the automated builds on Windows, Linux and macOS for
> > our team, and as a rule we always open an internal Pull Request on our
> > topic branches as we develop them, and you would probably not believe the
> > number of issues we caught before sending the patches to this list. Issues
> > including
> [nice list snipped]
> 
> Thanks for explaining.  I still am going to push back on the wording
> here, and here is why:
> 
>  1. Approximately 1/2 of the differences you describe apply to Mac as
> well as Windows.

No. The executable bit, the native line endings, most of the issues I
listed do not catch macOS-based developers off guard.

> The advice certainly does not apply on Mac.

That is true.

Stolee agreed in the PR to mention alternatives to Hyper-V, such as
VirtualBox, which would help macOS-based developers here.

> You might object: Mac readers would not be reading this text!  But
> that is not how humans work: if project documentation (e.g. the
> CONTRIBUTING.md on GitHub!) says that the project is Linux-focused
> and if you don't test on Linux then you might as well not bother,
> then people are going to believe it.

I thought the document encourages to test on Linux. It does not claim that
you can forget about getting your patches accepted if you don't test on
Linux.

>  2. It is not unusual for Linux users to make portability mistakes that
> are quickly pointed out on list.  If anything, the advice to test on
> other platforms should apply to contributors on Linux even more.
> This happens especially often to new contributors, who sometimes use
> bashisms, etc that get quickly pointed out.

True. Maybe this document is not for Linux-based developers, then. I might
over-generalize here, but in my experience, Windows-based developers were
particularly uneasy about getting what they perceived as criticized, and
for those developers it is that I want tools to tell them what is wrong.
It makes it much easier to accept.

>  3. I do not *want* Git to be a Linux-focused project; I want the code
> to perform well on all popular platforms and for people not to be
> afraid to make it so.

Me, too.

Realistically, though, way more than half of the reviewers on the Git
mailing list use Linux, and rarely switch to any other OS even for
testing.

The Google gang (you & Junio included) uses Linux. Peff uses Linux. From
what I can see Duy, Eric and Jake use Linux. That covers already the most
active reviewers right there.

So in order to get patches accepted (and that is the goal of a
contributor), chances can be improved by making stuff work on Linux.

> If the docs say that all we care about is Linux, then people are
> likely to be too scared to do the necessary and valuable work of
> making it work well on Mac, Windows, etc.  The actual mix of
> contributors doesn't bear it out anyway: a large number of
> contributors are already on Mac or Windows.

This document targets Windows-based developers. So they will already have
made sure that it works well on Windows.

Also, putting the burden of testing portability on first-time contributors
is really what will scare them away. So don't pretend that you want to
avoid scaring them if you talk about portability.

> Fortunately this is pretty straightforward to fix in the doc: it could
> say something like "to the multi-platform focused mailing list", for
> example.

You know what? I'd rather not. Seriously. I am already uneasy with
suggesting to install a full-fledged Linux VM, but I think it will
ultimately help contributors *not* to get scared away already by the first
volley of reviewer comments.

> [...]
> > To my chagrin, this idea of making most of the boring stuff (and I include
> > formatting in that category, but I am probably special in that respect) as
> > automatable, and as little of an issue for review as possible, leaving
> > most brain cycles to work on the correctness of the patches instead, did
> > not really receive a lot of traction on this mailing list.
> 
> Huh?  I'm confident that this is a pretty widely supported idea within
> the Git project.
> 
> I get the feeling you must have misread something or misinterpreted a
> different response.

So 

Re: [BUG] [git 2.16.1] yeeek ... my files are gone .. by git pull

[Jeff King]

On Mon, Mar 05, 2018 at 03:49:13PM +0100, Johannes Schindelin wrote:

> > I think that is doing the right thing for half of the problem. But
> > there's something else funny where we do not include the "upstream"
> > commits from the split history (i.e., we rebase onto nothing,
> > whereas a normal "git rebase" with a split history will graft the two
> > together).
> 
> Let me ask to make sure I am understanding you correctly. Are you
> referring to "split history" as the case where the commit graph has *two*
> root commits?

Yes, I mean two root commits. But especially when one is in the history
to be rebased, and the other is in the "upstream" history.

So as a concrete example, if I have this repo:

  git init
  >one && git add one && git commit -m one
  git checkout --orphan other
  git mv one two && git commit -m two

and I do this:

  git rebase master

I end up with a two-commit history, with "two" on top of "one". That
makes sense to me. Similarly if I instead do:

  git rebase -i master

the todo list has "pick two", and if I leave it as-is then I get the
same history. But if I do:

  git rebase --preserve-merges master

I end up with a single-commit history, with only commit "one". That's
wrong, because it threw away the history on the "other" branch.

If I apply the patch I showed earlier, then I get a single-branch
history, but this time it contains only "two". That also seems wrong,
because we didn't build on top of "master". I'd expect this command to
give the same results as a non-merge-preserving rebase.

Does that make more sense?

-Peff

Re: [RFC] Contributing to Git (on Windows)

[Derrick Stolee]

I really appreciate the feedback on this document, Jonathan.

On 3/3/2018 1:27 PM, Jonathan Nieder wrote:

Hi Dscho,

Johannes Schindelin wrote:

Jonathan Nieder  writes:

Dereck Stolee wrote:


nit: s/Dereck/Derrick/ Is my outgoing email name misspelled, or do you 
have a misspelled contact info for me?



+Test Your Changes on Linux
+--
+
+It can be important to work directly on the [core Git
codebase](https://github.com/git/git), +such as a recent commit into
the `master` or `next` branch that has not been incorporated +into
Git for Windows. Also, it can help to run functional and performance
tests on your +code in Linux before submitting patches to the
Linux-focused mailing list.

I'm surprised at this advice.  Does it actually come up?

Yes.

I personally set up the automated builds on Windows, Linux and macOS for
our team, and as a rule we always open an internal Pull Request on our
topic branches as we develop them, and you would probably not believe the
number of issues we caught before sending the patches to this list. Issues
including

[nice list snipped]

Thanks for explaining.  I still am going to push back on the wording
here, and here is why:

  1. Approximately 1/2 of the differences you describe apply to Mac as
 well as Windows.  The advice certainly does not apply on Mac.

 You might object: Mac readers would not be reading this text!  But
 that is not how humans work: if project documentation (e.g. the
 CONTRIBUTING.md on GitHub!) says that the project is Linux-focused
 and if you don't test on Linux then you might as well not bother,
 then people are going to believe it.

  2. It is not unusual for Linux users to make portability mistakes that
 are quickly pointed out on list.  If anything, the advice to test on
 other platforms should apply to contributors on Linux even more.
 This happens especially often to new contributors, who sometimes use
 bashisms, etc that get quickly pointed out.

  3. I do not *want* Git to be a Linux-focused project; I want the code
 to perform well on all popular platforms and for people not to be
 afraid to make it so.

 If the docs say that all we care about is Linux, then people are
 likely to be too scared to do the necessary and valuable work of
 making it work well on Mac, Windows, etc.  The actual mix of
 contributors doesn't bear it out anyway: a large number of
 contributors are already on Mac or Windows.

Fortunately this is pretty straightforward to fix in the doc: it could
say something like "to the multi-platform focused mailing list", for
example.


I like the "multi-platform" wording, and I'll try to use it as often as 
possible. I'll keep the Linux instructions because it is free to set up 
a Linux environment and testing in Windows and Linux will catch most of 
the cross-platform issues.




[...]

To my chagrin, this idea of making most of the boring stuff (and I include
formatting in that category, but I am probably special in that respect) as
automatable, and as little of an issue for review as possible, leaving
most brain cycles to work on the correctness of the patches instead, did
not really receive a lot of traction on this mailing list.

Huh?  I'm confident that this is a pretty widely supported idea within
the Git project.

I get the feeling you must have misread something or misinterpreted a
different response.

[...]

No, this advice comes straight from my personal observation that the
reviewers on the Git mailing list are Linux-centric.

Hopefully the clarifications and suggestions higher in this message
help.  If they don't, then I'm nervous about our ability to understand
each other.

[...]

Now, how reasonable do I think it is to ask those contributors to purchase
an Apple machine to test their patches on macOS (you cannot just download
an .iso nor would it be legal to run a macOS VM on anything but Apple
hardware)? You probably guessed my answer: not at all.

Agreed, this is something that needs to be automated (and not via a
CONTRIBUTING.md file).  As a stopgap, having a section in the
contribution instructions about testing using Windows's Linux
subsystem is a valuable thing, and thanks for that; I never meant to
imply otherwise.

[...]

On Fri, 2 Mar 2018, Junio C Hamano wrote:

In fact, portability issues in a patch originally written for a platform
is rather quickly discovered if the original platform is more minor than
the others, so while it is good advice to test your ware before you make
it public, singling out the portability issues may not add much value.
The fewer number of obvious bugs remaining, the fewer number of
iterations it would take for a series to get in a good shape.

[...]

For you, Junio, however, the task *now* is to put yourself into the shoes
of a Computer Science student in their 2nd year who wants to become an
Open Source contributor and is a little afraid to talk directly to "the

Re: [BUG] [git 2.16.1] yeeek ... my files are gone .. by git pull

[Johannes Schindelin]

Hi Peff,

On Wed, 28 Feb 2018, Jeff King wrote:

> On Tue, Feb 27, 2018 at 12:33:56AM +0100, Johannes Schindelin wrote:
> 
> > > So something like this helps:
> > > 
> > > diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
> > > index 81c5b42875..71e6cbb388 100644
> > > --- a/git-rebase--interactive.sh
> > > +++ b/git-rebase--interactive.sh
> > > @@ -921,15 +921,20 @@ else
> > >  
> > >   if test -z "$rebase_root"
> > >   then
> > >   preserve=t
> > > + p=
> > >   for p in $(git rev-list --parents -1 $sha1 | cut -d' ' 
> > > -s -f2-)
> > >   do
> > >   if test -f "$rewritten"/$p
> > >   then
> > >   preserve=f
> > >   fi
> > >   done
> > > + if test -z "$p"
> > > + then
> > > + preserve=f
> > > + fi
> > >   else
> > >   preserve=f
> > >   fi
> > >   if test f = "$preserve"
> > > 
> > > Because it at least adds "two" to the list of commits to pick. But
> > > oddly, it picks it directly as a root commit again. Whereas a rebase
> > > without --preserve-merges (and even "-i") picks it on top of commit
> > > "one" (which is what I'd expect).
> > > 
> > > +cc Dscho, as the --preserve-merges guru.
> > 
> > Your analysis makes sense to me. Please note, though, that I would not
> > consider myself a guru on preserve-merges. I think this mode is broken by
> > design (you can blame me if you want).
> 
> I think that is doing the right thing for half of the problem. But
> there's something else funny where we do not include the "upstream"
> commits from the split history (i.e., we rebase onto nothing,
> whereas a normal "git rebase" with a split history will graft the two
> together).

Let me ask to make sure I am understanding you correctly. Are you
referring to "split history" as the case where the commit graph has *two*
root commits?

If so: when you perform a merge-preserving rebase, then those two root
commits will be recreated as new root commits, by design.

The non-merge-preserving mode cannot create two root commits, as it does
not allow for introducing merge commits (and you'd need that to combine
the two strands).

It is quite possible that I misunderstand completely, though. Care to
enlighten me?

Ciao,
Dscho

Re: Contributor Summit planning

[Ævar Arnfjörð Bjarmason]

On Sat, Mar 03 2018, Jeff King jotted:

> Feel free to reply to this thread if you want to make plans or discuss
> any proposed topics before the summit. Input or questions from
> non-attendees is welcome here.

1)

Last year we discussed the general topic of performance. That's of
course very general, and can touch multiple unrelated areas of git
(worktree 'status' performance, all things protocol, storing big
objects, graph traversal, etc.).

No doubt others will propose other topics we'd want to carve out into
their own discussions (e.g. I'm expecting the v2 protocol to be one such
topic), but it'll be nice to have something that brings everyone
up-to-date on what the status is for purposes of coordination &
collaboration.

2)

I doubt many are interested in these as topics during the contributor
summit (but will propose them on the board if others care), but I have a
couple of things that touch quite a lot of things that I'm going to get
around to working on (or have in some WIP state):

 - PCRE-ification & PCRE convert:

   a) Use the interface recently added to PCRE to use it also for basic &
  extended POSIX regexps. This speeds things up a lot

   b) See if we can use PCRE (or at least extended regexp) consistently in
  the rest of git via single interface. Relevant code:

  git grep '\b(regcomp|regexec)\b' -- '*.[ch]'

   c) Maybe if we like a) && b) enough for performance reasons we'd be
 happy to make PCRE a hard dependency, and would then ship with a
 compat/pcre2 (~80k lines) instead of the current ancient (and hard
 to update) compat/regex/* (~10k lines).

 - Adding the ability to optionally read a .gitconfig shipped with the
   repo, see
   https://public-inbox.org/git/87zi6eakkt@evledraar.gmail.com/

Re: Contributor Summit planning

[Derrick Stolee]

On 3/3/2018 5:39 AM, Jeff King wrote:

On Sat, Mar 03, 2018 at 05:30:10AM -0500, Jeff King wrote:


As in past years, I plan to run it like an unconference. Attendees are
expected to bring topics for group discussion. Short presentations are
also welcome. We'll put the topics on a whiteboard in the morning, and
pick whichever ones people are interested in.

Feel free to reply to this thread if you want to make plans or discuss
any proposed topics before the summit. Input or questions from
non-attendees is welcome here.

I'll plan to offer two topics:

  - a round-up of the current state and past year's activities of Git as
a member project of Software Freedom Conservancy

  - some updates on the state of the git-scm.com since my report last
year

As with last year, I'll try to send a written report to the list for
those who aren't at the summit in person.

-Peff


Thanks for putting this together, Peff.

I'll be ready to talk about the serialized commit graph [1], generation 
numbers, and other commit-walk optimizations.


Thanks,
-Stolee

[1] 
https://public-inbox.org/git/1519698787-190494-1-git-send-email-dsto...@microsoft.com/T/#u

Re: [Bug] git log --show-signature print extra carriage return ^M

[Johannes Schindelin]

Hi Larry,

On Sun, 4 Mar 2018, Larry Hunter wrote:

> There is bug using "git log --show-signature" in my installation
> 
> git 2.16.2.windows.1
> gpg (GnuPG) 2.2.4
> libgcrypt 1.8.2

The gpg.exe shipped in Git for Windows should say something like this:

$ gpg --version
gpg (GnuPG) 1.4.22
Copyright (C) 2015 Free Software Foundation, Inc.
License GPLv3+: GNU GPL version 3 or later

This is free software: you are free to change and redistribute it.
There is NO WARRANTY, to the extent permitted by law.

Home: ~/.gnupg
Supported algorithms:
Pubkey: RSA, RSA-E, RSA-S, ELG-E, DSA
Cipher: IDEA, 3DES, CAST5, BLOWFISH, AES, AES192, AES256, TWOFISH,
CAMELLIA128, CAMELLIA192, CAMELLIA256
Hash: MD5, SHA1, RIPEMD160, SHA256, SHA384, SHA512, SHA224
Compression: Uncompressed, ZIP, ZLIB, BZIP2

Therefore, the GNU Privacy Guard version you use is not the one shipped
and supported by the Git for Windows project.

> that prints (with colors) an extra ^M (carriage return?) at the end of
> the gpg lines. As an example, the output of "git log --show-signature
> HEAD" looks like:
> 
> $ git log --show-signature HEAD
> commit 46c490188ebd216f20c454ee61108e51b481844e (HEAD -> master)
> gpg: Signature made 03/04/18 16:53:06 ora solare Europa occidentale^M
> gpg:using RSA key ...^M
> gpg: Good signature from "..." [ultimate]^M
> Author: ... <...>
> Date:   Sun Mar 4 16:53:06 2018 +0100
> ...
> 
> To help find a fix, I tested the command "git verify-commit HEAD" that
> prints (without colors) the same lines without extra ^M characters.
> 
> $ git verify-commit HEAD
> gpg: Signature made 03/04/18 16:53:06 ora solare Europa occidentale
> gpg:using RSA key ...
> gpg: Good signature from "..." [ultimate]

My guess is that the latter command simply does not go through the pager
while the former does.

Do you see the ^M in the output of `git -p verify-commit HEAD`?

Ciao,
Johannes

Re: [PATCH/RFC 1/1] gc --auto: exclude the largest giant pack in low-memory config

[Ævar Arnfjörð Bjarmason]

On Thu, Mar 01 2018, Nguyễn Thái Ngọc Duy jotted:

> pack-objects could be a big memory hog especially on large repos,
> everybody knows that. The suggestion to stick a .keep file on the
> largest pack to avoid this problem is also known for a long time.
>
> Let's do the suggestion automatically instead of waiting for people to
> come to Git mailing list and get the advice. When a certain condition
> is met, gc --auto create a .keep file temporary before repack is run,
> then remove it afterward.
>
> gc --auto does this based on an estimation of pack-objects memory
> usage and whether that fits in one third of system memory (the
> assumption here is for desktop environment where there are many other
> applications running).
>
> Since the estimation may be inaccurate and that 1/3 threshold is
> arbitrary, give the user a finer control over this mechanism as well:
> if the largest pack is larger than gc.bigPackThreshold, it's kept.

This is very promising. Saves lots of memory on my ad-hoc testing of
adding a *.keep file on an in-house repo.

> + if (big_pack_threshold)
> + return pack->pack_size >= big_pack_threshold;
> +
> + /* First we have to scan through at least one pack */
> + mem_want = pack->pack_size + pack->index_size;
> + /* then pack-objects needs lots more for book keeping */
> + mem_want += sizeof(struct object_entry) * nr_objects;
> + /*
> +  * internal rev-list --all --objects takes up some memory too,
> +  * let's say half of it is for blobs
> +  */
> + mem_want += sizeof(struct blob) * nr_objects / 2;
> + /*
> +  * and the other half is for trees (commits and tags are
> +  * usually insignificant)
> +  */
> + mem_want += sizeof(struct tree) * nr_objects / 2;
> + /* and then obj_hash[], underestimated in fact */
> + mem_want += sizeof(struct object *) * nr_objects;
> + /*
> +  * read_sha1_file() (either at delta calculation phase, or
> +  * writing phase) also fills up the delta base cache
> +  */
> + mem_want += delta_base_cache_limit;
> + /* and of course pack-objects has its own delta cache */
> + mem_want += max_delta_cache_size;

I'm not familiar enough with this part to say, but isn't this assuming a
lot about the distribution of objects in a way that will cause is not to
repack in some pathological cases?

Probably worth documenting...

> + /* Only allow 1/3 of memory for pack-objects */
> + mem_have = total_ram() / 3;

Would be great to have this be a configurable variable, so you could set
it to e.g. 33% (like here), 50% etc.

Re: [PATCH] http.c: shell command evaluation for extraheader

[Johannes Schindelin]

Hi Colin,

On Sun, 4 Mar 2018, Colin Arnott wrote:

> The http.extraHeader config parameter currently only supports storing
> constant values. There are two main use cases where this fails:
> 
>   0. Sensitive payloads: frequently this config parameter is used to pass
>  authentication credentials in place of or in addition to the
>  Authorization header, however since this value is required to be in
>  the clear this can create security issues.
> 
>   1. Mutating headers: some headers, especially new authentication
>  schemes, leverage short lived tokens that change over time.
> 
> There do exist solutions with current tools for these use cases, however
> none are optimal:
> 
>   0. Shell alias: by aliasing over git with a call to git that includes the
>  config directive and evaluates the header value inline, you can
>  fake the desired mutability:
>`alias git='git -c http.extraHeader="$(gpg -d crypt.gpg)"'`
>  This presents two problems:
>  a. aliasing over commands can be confusing to new users, since git
> config information is stored in shell configs
>  b. this solution scales only to your shell, not all shells
> 
>   1. Global hook: you could implement a hook that writes the config
>  entry before fetch / pull actions, so that it is up to date, but
>  this does nothing to secure it.
> 
>   2. git-credential-helper: the credential helper interface already
>  supports shelling out to arbitrary binaries or scripts, however
>  this interface can only be used to populate the Authorization
>  header.

As the credential-helper is already intended for sensitive data, and as it
already allows to interact with a helper, I would strongly assume that it
would make more sense to try to extend that feature (instead of the simple
extraHeader one).

This would also help alleviate all the quoting/dequoting issues involved
with shell scripting.

Besides, the http.extraHeader feature was designed to accommodate all
kinds of extra headers, not only authentication ones (and indeed, the
authentication was only intended for use in build agents, where both
environment and logging can be controlled rather tightly).

I also see that in your implementation, only the extraHeader value is
evaluated, without any access to the rest of the metadata (such as URL,
and optionally specified user).

It would probably get a little more complicated than a shell script to
write a credential-helper that will always be asked to generate an
authentication, but I think even a moderate-level Perl script could be
used for that, and it *would* know the URL and user for which the
credentials are intended...

Ciao,
Johannes

Re: [PATCH] t2028: fix minor error and issues in newly-added "worktree move" tests

[SZEDER Gábor]

> Recently-added "git worktree move" tests include a minor error and a few
> small issues. Specifically:
> 
> * checking non-existence of wrong file ("source" instead of
>   "destination")
> 
> * unneeded redirect (">empty")
> 
> * unused variable ("toplevel")
> 
> * restoring a worktree location by means of a separate test somewhat
>   distant from the test which moved it rather than using
>   test_when_finished() to restore it in a self-contained fashion

There is one more issue in these tests.
 

> diff --git a/t/t2028-worktree-move.sh b/t/t2028-worktree-move.sh
> index 082368d8c6..d70d13dabe 100755
> --- a/t/t2028-worktree-move.sh
> +++ b/t/t2028-worktree-move.sh
> @@ -75,7 +75,7 @@ test_expect_success 'move worktree' '
>   git worktree move source destination &&
>   test_path_is_missing source &&
>   git worktree list --porcelain | grep "^worktree.*/destination" &&
> - ! git worktree list --porcelain | grep "^worktree.*/source" >empty &&
> + ! git worktree list --porcelain | grep "^worktree.*/source" &&

The main purpose of this test script is to test the 'git worktree'
command, but these pipes hide its exit code.
Could you please save 'git worktree's output into an intermediate
file, and run 'grep' on the file's contents?

This also applies to two other tests in this test script.

>   git -C destination log --format=%s >actual2 &&
>   echo init >expected2 &&
>   test_cmp expected2 actual2
> @@ -86,10 +86,10 @@ test_expect_success 'move main worktree' '
>  '
>  
>  test_expect_success 'move worktree to another dir' '
> - toplevel="$(pwd)" &&
>   mkdir some-dir &&
>   git worktree move destination some-dir &&
> - test_path_is_missing source &&
> + test_when_finished "git worktree move some-dir/destination destination" 
> &&
> + test_path_is_missing destination &&
>   git worktree list --porcelain | grep "^worktree.*/some-dir/destination" 
> &&
>   git -C some-dir/destination log --format=%s >actual2 &&
>   echo init >expected2 &&
> @@ -100,10 +100,6 @@ test_expect_success 'remove main worktree' '
>   test_must_fail git worktree remove .
>  '
>  
> -test_expect_success 'move some-dir/destination back' '
> - git worktree move some-dir/destination destination
> -'
> -
>  test_expect_success 'remove locked worktree' '
>   git worktree lock destination &&
>   test_when_finished "git worktree unlock destination" &&
> -- 
> 2.16.2.660.g709887971b
> 
> 

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

[SZEDER Gábor]

On Mon, Mar 5, 2018 at 11:59 AM, Phillip Wood  wrote:
> I did wonder about putting this function in a library when I first wrote
> it but decided to wait and see if it is useful instead. As Junio points
> out it would need to be improved to act as a generic filter, so I'll
> take the easy option and leave it where it is at the moment.

Makes sense.  I just pointed it out, because I could have used it
already in its current form for a test I was working on last week.

Re: [PATCH 0/3] git worktree prune improvements

[Duy Nguyen]

On Sat, Mar 3, 2018 at 9:21 PM, Randall S. Becker
 wrote:
> On March 2, 2018 10:39 PM, Nguy?n Thái Ng?c Duy wrote:
>> This is something we could do to improve the situation when a user manually
>> moves a worktree and not follow the update process (we have had the first
>> reported case [1]). Plus a bit cleanup in gc.
>>
>> I think this is something we should do until we somehow make the user
>> aware that the worktree is broken as soon as they move a worktree
>> manually. But there's some more work to get there.
>>
>> [1] http://public-inbox.org/git/%3Caa98f187-4b1a-176d-2a1b-
>> 826c99577...@aegee.org%3E
>
> I wonder whether the OT thread discussion about branch annotation may have 
> some value here. For some repositories I manage, I have received questions 
> about whether there was some way to know that a branch in the clone was 
> associated with a worktree "at any point in the past", which, once the 
> worktree has been pruned, is not derivable in a formal computational sense - 
> there may be specific conditions where it is. Perhaps, if that line of 
> development moves forward, that we should considering annotating the 
> worktree-created branch to help with our pruning process and to identify 
> where the branch originated.
>

I think for pruning, we already have that information. If a branch is
associated to a worktree, its HEAD must say so and we must not prune
anything reachable from _any_ HEAD. I made that mistake actually.
Still in process of fixing it (and fsck).
-- 
Duy

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

[Phillip Wood]

On 02/03/18 16:09, Junio C Hamano wrote:
> SZEDER Gábor  writes:
> 
>>> +diff_cmp () {
>>> +   for x
>>> +   do
>>> +   sed  -e '/^index/s/[0-9a-f]*[1-9a-f][0-9a-f]*\.\./1234567../' \
>>> +-e '/^index/s/\.\.[0-9a-f]*[1-9a-f][0-9a-f]*/..9abcdef/' \
>>> +-e '/^index/s/ 00*\.\./ 000../' \
>>> +-e '/^index/s/\.\.00*$/..000/' \
>>> +-e '/^index/s/\.\.00* /..000 /' \
>>> +"$x" >"$x.filtered"
>>> +   done
>>> +   test_cmp "$1.filtered" "$2.filtered"
>>> +}
>>
>> t3701 is not the only test script comparing diffs, many other
>> tests do so:
>>
>>   $ git grep 'diff --git' t/ |wc -l
>>   835
>>
>> It's totally inaccurate, but a good ballpark figure.
>>
>> I think this function should be a widely available test helper
>> function in 'test-lib-functions.sh', perhaps renamed to
>> 'test_cmp_diff', so those other tests could use it as well.
> 
> I am on the fence.  
> 
> The above does not redact enough (e.g. rename/copy score should be
> redacted while comparing) to serve as a generic filter.
> 
> We could certainly extend it when we make code in other tests use
> the helper, but then we can do the moving to test-lib-functions when
> that happens, too.
> 
> Those who will be writing new tests that need to compare two diff
> outputs are either (1) people who are careful and try to find
> existing tests that do the same, to locate the helper we already
> have to be reused in theirs, or (2) people who don't and roll their
> own.  The latter camp cannot be helped either way, but the former
> will run "git grep 'diff --git' t/" and find the helper whether it
> is in this script or in test-lib-functions, I would think.
> 
> So, I certainly do not mind a reroll to move it to a more generic
> place, but I do not think I would terribly mind if we leave it in
> its current place, later to be moved by the first new caller that
> wants to use it from outside this script.
> 

I did wonder about putting this function in a library when I first wrote
it but decided to wait and see if it is useful instead. As Junio points
out it would need to be improved to act as a generic filter, so I'll
take the easy option and leave it where it is at the moment.

Best Wishes

Phillip

[PATCH v5 9/9] add -p: don't rely on apply's '--recount' option

[Phillip Wood]

From: Phillip Wood 

Now that add -p counts patches properly it should be possible to turn
off the '--recount' option when invoking 'git apply'

Signed-off-by: Phillip Wood 
---

Notes:
I can't think of a reason why this shouldn't be OK but I can't help
feeling slightly nervous about it. I've made it a separate patch so it
can be easily dropped or reverted if I've missed something.

 git-add--interactive.perl | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/git-add--interactive.perl b/git-add--interactive.perl
index d1b550d4d8..f83e7450ad 100755
--- a/git-add--interactive.perl
+++ b/git-add--interactive.perl
@@ -677,7 +677,7 @@ sub add_untracked_cmd {
 sub run_git_apply {
my $cmd = shift;
my $fh;
-   open $fh, '| git ' . $cmd . " --recount --allow-overlap";
+   open $fh, '| git ' . $cmd . " --allow-overlap";
print $fh @_;
return close $fh;
 }
-- 
2.16.2

[PATCH v5 7/9] add -p: calculate offset delta for edited patches

[Phillip Wood]

From: Phillip Wood 

Recount the number of preimage and postimage lines in a hunk after it
has been edited so any change in the number of insertions or deletions
can be used to adjust the offsets of subsequent hunks. If an edited
hunk is subsequently split then the offset correction will be lost. It
would be possible to fix this if it is a problem, however the code
here is still an improvement on the status quo for the common case
where an edited hunk is applied without being split.

This is also a necessary step to removing '--recount' and
'--allow-overlap' from the invocation of 'git apply'. Before
'--recount' can be removed the splitting and coalescing counting needs
to be fixed to handle a missing newline at the end of a file. In order
to remove '--allow-overlap' there needs to be i) some way of verifying
the offset data in the edited hunk (probably by correlating the
preimage (or postimage if the patch is going to be applied in reverse)
lines of the edited and unedited versions to see if they are offset or
if any leading/trailing context lines have been removed) and ii) a way of
dealing with edited hunks that change context lines that are shared
with neighbouring hunks.

Signed-off-by: Phillip Wood 
---

Notes:
changes since v4:
 - replaced instances of
 <> and <>;
   with
 if (<>) {
<>;
 }

changes since v1
 - edited hunks are now recounted before seeing if they apply in
   preparation for removing '--recount' when invoking 'git apply'
 - added sentence to commit message about the offset data being lost
   if an edited hunk is split

 git-add--interactive.perl  | 59 +++---
 t/t3701-add-interactive.sh |  2 +-
 2 files changed, 51 insertions(+), 10 deletions(-)

diff --git a/git-add--interactive.perl b/git-add--interactive.perl
index 7a0a5896bb..f80372d3ad 100755
--- a/git-add--interactive.perl
+++ b/git-add--interactive.perl
@@ -938,13 +938,23 @@ sub coalesce_overlapping_hunks {
parse_hunk_header($text->[0]);
unless ($_->{USE}) {
$ofs_delta += $o_cnt - $n_cnt;
+   # If this hunk has been edited then subtract
+   # the delta that is due to the edit.
+   if ($_->{OFS_DELTA}) {
+   $ofs_delta -= $_->{OFS_DELTA};
+   }
next;
}
if ($ofs_delta) {
$n_ofs += $ofs_delta;
$_->{TEXT}->[0] = format_hunk_header($o_ofs, $o_cnt,
 $n_ofs, $n_cnt);
}
+   # If this hunk was edited then adjust the offset delta
+   # to reflect the edit.
+   if ($_->{OFS_DELTA}) {
+   $ofs_delta += $_->{OFS_DELTA};
+   }
if (defined $last_o_ctx &&
$o_ofs <= $last_o_ctx &&
!$_->{DIRTY} &&
@@ -1016,6 +1026,30 @@ marked for discarding."),
 marked for applying."),
 );
 
+sub recount_edited_hunk {
+   local $_;
+   my ($oldtext, $newtext) = @_;
+   my ($o_cnt, $n_cnt) = (0, 0);
+   for (@{$newtext}[1..$#{$newtext}]) {
+   my $mode = substr($_, 0, 1);
+   if ($mode eq '-') {
+   $o_cnt++;
+   } elsif ($mode eq '+') {
+   $n_cnt++;
+   } elsif ($mode eq ' ') {
+   $o_cnt++;
+   $n_cnt++;
+   }
+   }
+   my ($o_ofs, undef, $n_ofs, undef) =
+   parse_hunk_header($newtext->[0]);
+   $newtext->[0] = format_hunk_header($o_ofs, $o_cnt, $n_ofs, $n_cnt);
+   my (undef, $orig_o_cnt, undef, $orig_n_cnt) =
+   parse_hunk_header($oldtext->[0]);
+   # Return the change in the number of lines inserted by this hunk
+   return $orig_o_cnt - $orig_n_cnt - $o_cnt + $n_cnt;
+}
+
 sub edit_hunk_manually {
my ($oldtext) = @_;
 
@@ -1114,25 +1148,32 @@ sub prompt_yesno {
 }
 
 sub edit_hunk_loop {
-   my ($head, $hunk, $ix) = @_;
-   my $text = $hunk->[$ix]->{TEXT};
+   my ($head, $hunks, $ix) = @_;
+   my $hunk = $hunks->[$ix];
+   my $text = $hunk->{TEXT};
 
while (1) {
-   $text = edit_hunk_manually($text);
-   if (!defined $text) {
+   my $newtext = edit_hunk_manually($text);
+   if (!defined $newtext) {
return undef;
}
my $newhunk = {
-   TEXT => $text,
-   TYPE => $hunk->[$ix]->{TYPE},
+   TEXT => $newtext,
+   TYPE => $hunk->{TYPE},

[PATCH v5 6/9] add -p: adjust offsets of subsequent hunks when one is skipped

[Phillip Wood]

From: Phillip Wood 

Since commit 8cbd431082 ("git-add--interactive: replace hunk
recounting with apply --recount", 2008-7-2) if a hunk is skipped then
we rely on the context lines to apply subsequent hunks in the right
place. While this works most of the time it is possible for hunks to
end up being applied in the wrong place. To fix this adjust the offset
of subsequent hunks to correct for any change in the number of
insertions or deletions due to the skipped hunk. The change in offset
due to edited hunks that have the number of insertions or deletions
changed is ignored here, it will be fixed in the next commit.

Signed-off-by: Phillip Wood 
---
 git-add--interactive.perl  | 15 +--
 t/t3701-add-interactive.sh |  2 +-
 2 files changed, 14 insertions(+), 3 deletions(-)

diff --git a/git-add--interactive.perl b/git-add--interactive.perl
index 8ababa6453..7a0a5896bb 100755
--- a/git-add--interactive.perl
+++ b/git-add--interactive.perl
@@ -926,14 +926,25 @@ sub coalesce_overlapping_hunks {
my @out = ();
 
my ($last_o_ctx, $last_was_dirty);
+   my $ofs_delta = 0;
 
-   for (grep { $_->{USE} } @in) {
+   for (@in) {
if ($_->{TYPE} ne 'hunk') {
push @out, $_;
next;
}
my $text = $_->{TEXT};
-   my ($o_ofs) = parse_hunk_header($text->[0]);
+   my ($o_ofs, $o_cnt, $n_ofs, $n_cnt) =
+   parse_hunk_header($text->[0]);
+   unless ($_->{USE}) {
+   $ofs_delta += $o_cnt - $n_cnt;
+   next;
+   }
+   if ($ofs_delta) {
+   $n_ofs += $ofs_delta;
+   $_->{TEXT}->[0] = format_hunk_header($o_ofs, $o_cnt,
+$n_ofs, $n_cnt);
+   }
if (defined $last_o_ctx &&
$o_ofs <= $last_o_ctx &&
!$_->{DIRTY} &&
diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh
index 094eeca405..e3150a4e07 100755
--- a/t/t3701-add-interactive.sh
+++ b/t/t3701-add-interactive.sh
@@ -544,7 +544,7 @@ test_expect_success 'set up pathological context' '
test_write_lines +b " a" >patch
 '
 
-test_expect_failure 'add -p works with pathological context lines' '
+test_expect_success 'add -p works with pathological context lines' '
git reset &&
printf "%s\n" n y |
git add -p &&
-- 
2.16.2

[PATCH v5 5/9] t3701: add failing test for pathological context lines

[Phillip Wood]

From: Phillip Wood 

When a hunk is skipped by add -i the offsets of subsequent hunks are
not adjusted to account for any missing insertions due to the skipped
hunk. Most of the time this does not matter as apply uses the context
lines to apply the subsequent hunks in the correct place, however in
pathological cases the context lines will match at the now incorrect
offset and the hunk will be applied in the wrong place. The offsets of
hunks following an edited hunk that has had the number of insertions
or deletions changed also need to be updated in the same way. Add
failing tests to demonstrate this.

Signed-off-by: Phillip Wood 
---

Notes:
changes since v2:
 - removed test_set_editor as it is already set

changes since v1:
 - changed edit test to use the existing fake editor and to strip
   the hunk header and some context lines as well as deleting an
   insertion
 - style fixes

 t/t3701-add-interactive.sh | 30 ++
 1 file changed, 30 insertions(+)

diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh
index f95714230b..094eeca405 100755
--- a/t/t3701-add-interactive.sh
+++ b/t/t3701-add-interactive.sh
@@ -531,4 +531,34 @@ test_expect_success 'status ignores dirty submodules 
(except HEAD)' '
! grep dirty-otherwise output
 '
 
+test_expect_success 'set up pathological context' '
+   git reset --hard &&
+   test_write_lines a a a a a a a a a a a >a &&
+   git add a &&
+   git commit -m a &&
+   test_write_lines c b a a a a a a a b a a a a >a &&
+   test_write_lines a a a a a a a b a a a a >expected-1 &&
+   test_write_lines   b a a a a a a a b a a a a >expected-2 &&
+   # check editing can cope with missing header and deleted context lines
+   # as well as changes to other lines
+   test_write_lines +b " a" >patch
+'
+
+test_expect_failure 'add -p works with pathological context lines' '
+   git reset &&
+   printf "%s\n" n y |
+   git add -p &&
+   git cat-file blob :a >actual &&
+   test_cmp expected-1 actual
+'
+
+test_expect_failure 'add -p patch editing works with pathological context 
lines' '
+   git reset &&
+   # n q q below is in case edit fails
+   printf "%s\n" e yn q q |
+   git add -p &&
+   git cat-file blob :a >actual &&
+   test_cmp expected-2 actual
+'
+
 test_done
-- 
2.16.2

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

[Phillip Wood]

From: Phillip Wood 

Use a filter when comparing diffs to fix the value of non-zero hashes
in diff index lines so we're not hard coding sha1 hash values in the
expected output. This makes it easier to change the expected output if
a test is edited as we don't need to worry about the exact hash value
and means the tests will work when the hash algorithm is transitioned
away from sha1.

Thanks-to: Junio C Hamano 
Signed-off-by: Phillip Wood 
---

Notes:
changes since v3:
 - fix zero hash values to seven digits
changes since v2:
 - fix hash values in index lines rather than removing the line
 - reworded commit message

 t/t3701-add-interactive.sh | 33 +++--
 1 file changed, 23 insertions(+), 10 deletions(-)

diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh
index 836ce346ed..f95714230b 100755
--- a/t/t3701-add-interactive.sh
+++ b/t/t3701-add-interactive.sh
@@ -10,6 +10,19 @@ then
test_done
 fi
 
+diff_cmp () {
+   for x
+   do
+   sed  -e '/^index/s/[0-9a-f]*[1-9a-f][0-9a-f]*\.\./1234567../' \
+-e '/^index/s/\.\.[0-9a-f]*[1-9a-f][0-9a-f]*/..9abcdef/' \
+-e '/^index/s/ 00*\.\./ 000../' \
+-e '/^index/s/\.\.00*$/..000/' \
+-e '/^index/s/\.\.00* /..000 /' \
+"$x" >"$x.filtered"
+   done
+   test_cmp "$1.filtered" "$2.filtered"
+}
+
 test_expect_success 'setup (initial)' '
echo content >file &&
git add file &&
@@ -35,7 +48,7 @@ test_expect_success 'setup expected' '
 test_expect_success 'diff works (initial)' '
(echo d; echo 1) | git add -i >output &&
sed -ne "/new file/,/content/p" diff &&
-   test_cmp expected diff
+   diff_cmp expected diff
 '
 test_expect_success 'revert works (initial)' '
git add file &&
@@ -72,7 +85,7 @@ test_expect_success 'setup expected' '
 test_expect_success 'diff works (commit)' '
(echo d; echo 1) | git add -i >output &&
sed -ne "/^index/,/content/p" diff &&
-   test_cmp expected diff
+   diff_cmp expected diff
 '
 test_expect_success 'revert works (commit)' '
git add file &&
@@ -91,7 +104,7 @@ test_expect_success 'dummy edit works' '
test_set_editor : &&
(echo e; echo a) | git add -p &&
git diff > diff &&
-   test_cmp expected diff
+   diff_cmp expected diff
 '
 
 test_expect_success 'setup patch' '
@@ -159,7 +172,7 @@ test_expect_success 'setup expected' '
 test_expect_success 'real edit works' '
(echo e; echo n; echo d) | git add -p &&
git diff >output &&
-   test_cmp expected output
+   diff_cmp expected output
 '
 
 test_expect_success 'skip files similarly as commit -a' '
@@ -171,7 +184,7 @@ test_expect_success 'skip files similarly as commit -a' '
git reset &&
git commit -am commit &&
git diff >expected &&
-   test_cmp expected output &&
+   diff_cmp expected output &&
git reset --hard HEAD^
 '
 rm -f .gitignore
@@ -248,7 +261,7 @@ test_expect_success 'add first line works' '
git apply patch &&
(echo s; echo y; echo y) | git add -p file &&
git diff --cached > diff &&
-   test_cmp expected diff
+   diff_cmp expected diff
 '
 
 test_expect_success 'setup expected' '
@@ -271,7 +284,7 @@ test_expect_success 'deleting a non-empty file' '
rm non-empty &&
echo y | git add -p non-empty &&
git diff --cached >diff &&
-   test_cmp expected diff
+   diff_cmp expected diff
 '
 
 test_expect_success 'setup expected' '
@@ -290,7 +303,7 @@ test_expect_success 'deleting an empty file' '
rm empty &&
echo y | git add -p empty &&
git diff --cached >diff &&
-   test_cmp expected diff
+   diff_cmp expected diff
 '
 
 test_expect_success 'split hunk setup' '
@@ -355,7 +368,7 @@ test_expect_success 'patch mode ignores unmerged entries' '
+changed
EOF
git diff --cached >diff &&
-   test_cmp expected diff
+   diff_cmp expected diff
 '
 
 test_expect_success TTY 'diffs can be colorized' '
@@ -384,7 +397,7 @@ test_expect_success 'patch-mode via -i prompts for files' '
 
echo test >expect &&
git diff --cached --name-only >actual &&
-   test_cmp expect actual
+   diff_cmp expect actual
 '
 
 test_expect_success 'add -p handles globs' '
-- 
2.16.2

[PATCH v5 8/9] add -p: fix counting when splitting and coalescing

[Phillip Wood]

From: Phillip Wood 

When a file has no trailing new line at the end diff records this by
appending "\ No newline at end of file" below the last line of the
file. This line should not be counted in the hunk header. Fix the
splitting and coalescing code to count files without a trailing new line
properly and change one of the tests to test splitting without a
trailing new line.

Signed-off-by: Phillip Wood 
---

Notes:
changes since v4:
 - use a separate if clause to handle with "\ No newline at end of
   file" lines rather than crowbarring them into the "+" line
   handling.

 git-add--interactive.perl  | 11 +++
 t/t3701-add-interactive.sh | 31 +++
 2 files changed, 34 insertions(+), 8 deletions(-)

diff --git a/git-add--interactive.perl b/git-add--interactive.perl
index f80372d3ad..d1b550d4d8 100755
--- a/git-add--interactive.perl
+++ b/git-add--interactive.perl
@@ -793,6 +793,11 @@ sub split_hunk {
while (++$i < @$text) {
my $line = $text->[$i];
my $display = $display->[$i];
+   if ($line =~ /^\\/) {
+   push @{$this->{TEXT}}, $line;
+   push @{$this->{DISPLAY}}, $display;
+   next;
+   }
if ($line =~ /^ /) {
if ($this->{ADDDEL} &&
!defined $next_hunk_start) {
@@ -891,6 +896,9 @@ sub merge_hunk {
$n_cnt++;
push @line, $line;
next;
+   } elsif ($line =~ /^\\/) {
+   push @line, $line;
+   next;
}
 
last if ($o1_ofs <= $ofs);
@@ -909,6 +917,9 @@ sub merge_hunk {
$n_cnt++;
push @line, $line;
next;
+   } elsif ($line =~ /^\\/) {
+   push @line, $line;
+   next;
}
$ofs++;
$o_cnt++;
diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh
index 4cc9517eda..a9a9478a29 100755
--- a/t/t3701-add-interactive.sh
+++ b/t/t3701-add-interactive.sh
@@ -237,31 +237,46 @@ test_expect_success 'setup patch' '
 baseline
 content
+lastline
+   \ No newline at end of file
EOF
 '
 
-# Expected output, similar to the patch but w/ diff at the top
+# Expected output, diff is similar to the patch but w/ diff at the top
 test_expect_success 'setup expected' '
-   cat >expected <<-\EOF
-   diff --git a/file b/file
-   index b6f2c08..61b9053 100755
+   echo diff --git a/file b/file >expected &&
+   cat patch |sed "/^index/s/ 100644/ 100755/" >>expected &&
+   cat >expected-output <<-\EOF
--- a/file
+++ b/file
@@ -1,2 +1,4 @@
+firstline
 baseline
 content
+lastline
+   \ No newline at end of file
+   @@ -1,2 +1,3 @@
+   +firstline
+baseline
+content
+   @@ -1,2 +2,3 @@
+baseline
+content
+   +lastline
+   \ No newline at end of file
EOF
 '
 
 # Test splitting the first patch, then adding both
-test_expect_success 'add first line works' '
+test_expect_success C_LOCALE_OUTPUT 'add first line works' '
git commit -am "clear local changes" &&
git apply patch &&
-   (echo s; echo y; echo y) | git add -p file &&
-   git diff --cached > diff &&
-   diff_cmp expected diff
+   printf "%s\n" s y y | git add -p file 2>error |
+   sed -n -e "s/^Stage this hunk[^@]*\(@@ .*\)/\1/" \
+  -e "/^[-+@ ]"/p  >output &&
+   test_must_be_empty error &&
+   git diff --cached >diff &&
+   diff_cmp expected diff &&
+   test_cmp expected-output output
 '
 
 test_expect_success 'setup expected' '
-- 
2.16.2

[PATCH v5 1/9] add -i: add function to format hunk header

[Phillip Wood]

From: Phillip Wood 

This code is duplicated in a couple of places so make it into a
function as we're going to add some more callers shortly.

Signed-off-by: Phillip Wood 
---
 git-add--interactive.perl | 21 +++--
 1 file changed, 11 insertions(+), 10 deletions(-)

diff --git a/git-add--interactive.perl b/git-add--interactive.perl
index 964c3a7542..8ababa6453 100755
--- a/git-add--interactive.perl
+++ b/git-add--interactive.perl
@@ -751,6 +751,15 @@ sub parse_hunk_header {
return ($o_ofs, $o_cnt, $n_ofs, $n_cnt);
 }
 
+sub format_hunk_header {
+   my ($o_ofs, $o_cnt, $n_ofs, $n_cnt) = @_;
+   return ("@@ -$o_ofs" .
+   (($o_cnt != 1) ? ",$o_cnt" : '') .
+   " +$n_ofs" .
+   (($n_cnt != 1) ? ",$n_cnt" : '') .
+   " @@\n");
+}
+
 sub split_hunk {
my ($text, $display) = @_;
my @split = ();
@@ -838,11 +847,7 @@ sub split_hunk {
my $o_cnt = $hunk->{OCNT};
my $n_cnt = $hunk->{NCNT};
 
-   my $head = ("@@ -$o_ofs" .
-   (($o_cnt != 1) ? ",$o_cnt" : '') .
-   " +$n_ofs" .
-   (($n_cnt != 1) ? ",$n_cnt" : '') .
-   " @@\n");
+   my $head = format_hunk_header($o_ofs, $o_cnt, $n_ofs, $n_cnt);
my $display_head = $head;
unshift @{$hunk->{TEXT}}, $head;
if ($diff_use_color) {
@@ -912,11 +917,7 @@ sub merge_hunk {
}
push @line, $line;
}
-   my $head = ("@@ -$o0_ofs" .
-   (($o_cnt != 1) ? ",$o_cnt" : '') .
-   " +$n0_ofs" .
-   (($n_cnt != 1) ? ",$n_cnt" : '') .
-   " @@\n");
+   my $head = format_hunk_header($o0_ofs, $o_cnt, $n0_ofs, $n_cnt);
@{$prev->{TEXT}} = ($head, @line);
 }
 
-- 
2.16.2