Re: hunting for lost highly interactive browser based git tutorial

[Jeff King]

On Wed, Oct 03, 2018 at 01:28:20AM -0400, _g e r r y _ _l o w r y _ wrote:

> The tutorial "may" have be called something like "Try Git";
> however, I can not find it at try.github.io and other places where i have 
> looked.

Unfortunately, Try Git seems to have been shut down. I don't know of any
official announcement, but there's some discussion here:

  https://github.com/Try-Git/try_git/issues/24#issuecomment-420784845

-Peff

hunting for lost highly interactive browser based git tutorial

[_g e r r y _ _l o w r y _]

Help, please and thank you.

i have spent > one hour searching via Google and by visiting git-scm, 
BitBucket, github, et cetera, for an excellent tutorial for
beginners and refresher for one who has not touched git for quite a while.

if you've done the same tutorial, you will recognize its features:

-- browser based
-- highly interactive (use commands that are not necessarily "the next step", 
i.e., spontaneous self-guided review and trying
variations not necessarily in the tutorial's script)
-- teaches Git command line not GUI

The tutorial "may" have be called something like "Try Git";
however, I can not find it at try.github.io and other places where i have 
looked.

it's out there somewhere.

The good news is i have found many of my favourite Git resources-if only i 
could find my favourite Git beginner's interactive
tutorial, i'd be one happy homo sapiens (probably i need a life).

Thank you for taking time to read this request.

B-)

Gerry (Lowry)
Wasaga Beach
Ontario
Canada

Re: [BUG] A part of an edge from an octopus merge gets colored, even with --color=never

[Jeff King]

On Mon, Sep 24, 2018 at 08:27:47PM -0400, Noam Postavsky wrote:

> > So I think this is doing the right thing. I'm not sure if there's a
> > better way to explain "dashless" or not.
> 
> I've updated the comments and renamed a few variables, see if that helps.

Yeah, I really like your explanations/diagrams in the comments. It makes
the logic very clear.

> > Do you feel comfortable trying to add something to the test suite for
> > this?
> 
> Um, sort of. I managed to recast my script into the framework of the
> other tests (see attached t4299-octopus.sh); it seems like it should
> go into t4202-log.sh, but it's not clear to me how I can do this
> without breaking all the other tests which expect a certain sequence
> of commits.

It's OK to start a new script if you have some tricky or complicated
setup. Probably it ought to be t4214-log-graph-octopus to keep it near
the other log tests. And it should be added in the actual patch, but I
assume you just kept it out here since you weren't sure where to put it
yet.

I'll try to comment on the test script itself.

> #!/bin/sh
> 
> test_description='git log'

This should actually describe what's going on in the test. Usually a
one-sentence is OK, but I think it might be good to specifically mention
that we're handling this special octopus case.

> . ./test-lib.sh
> 
> make_octopus_merge () {
>   for i ; do
>   git checkout master -b $i || return $?
>   # Make tag name different from branch name.
>   test_commit $i $i $i tag$i || return $?
>   done

Please use:

  for i in "$@"

which is a bit less subtle (there's also only one caller of this
function, so it could be inlined; note that it's OK to use "return" in a
test_expect block).

Why do we need the tag name to be different?

>   git checkout 1 -b merge &&

This is assuming we just made a branch called "1", but that's one of the
arguments. Probably this should be "$1" (or the whole thing should just
be inlined so it is clear that what the set of parameters we expect is).

>   test_tick &&
>   git merge -m octopus-merge "$@"

Good use of test_tick so that we have predictable traversal ordering.

> test_expect_success 'set up merge history' '
>   test_commit initial &&
>   make_octopus_merge 1 2 3 4 &&
>   git checkout 1 -b L &&
>   test_commit left
> '

It might actually be worth setting up the uncolored expect file as part
of this, since it so neatly diagrams the graph you're trying to produce.

I.e., something like (completely untested; note that the leading
indentation is all tabs, which will be removed by the "<<-" operator):

test_expect_success 'set up merge history' '
# This is the graph we expect to generate here.
cat >expect.uncolored <<-\EOF &&
* left
| *---.   octopus-merge
| |\ \ \
|/ / / /
| | | * 4
| | * | 3
| | |/
| * | 2
| |/
* | 1
|/
* initial
EOF
for i in 1 2 3 4; do
git checkout -b $i $master || return $?
# Make tag name different from branch name.
test_commit $i $i $i tag$i || return $?
done &&
git checkout -b merge 1 &&
test_tick &&
git merge -m octopus-merge 1 2 3 4
'

> cat > expect.colors <<\EOF

A few style bits: we prefer to keep even setup steps like this inside a
test_expect block (though you may see some very old tests which have not
been fixed yet). Also, we omit the space after ">".

> * left
> | *---. 
>   octopus-merge
> | |\ \ \
> |/ / / /
> | | | * 4
> | | * | 3
> | | |/
> | * | 2
> | |/
> * | 1
> |/
> * initial

Yikes. :) This one is pretty hard to read. I'm not sure if there's a
good alternative. If you pipe the output of test_decode through
this:

  sed '
s/./R/g;
s/./B/g;
s/./M/g;
s/./Y/g;
  '

you get this:

  * left
  R *BBMM   octopus-merge
  R RY B M
  RR Y B M
  R Y B * 4
  R Y * M 3
  R Y MM
  R * M 2
  R MM
  * M 1
  MM
  * initial

which is admittedly pretty horrible, too, but at least resembles a
graph. I dunno.

I'm also not thrilled that we depend on the exact sequence of default
colors, but I suspect it's not the first time. And it wouldn't be too
hard to update it if that default changes.

> test_expect_success 'log --graph with tricky octopus merge' '
>   git log --color=always --graph --date-order --pretty=tformat:%s --all |
>   test_decode_color | sed "s/ *\$//" >actual &&

Try not to put "git" on the left-hand side of a pipe, since it means
we'll miss its exit code (and especially we'd miss its death due to ASan
or Valgrind problems, which I think was one of the major ways of
detecting the original problem). So:

  git log ... >actual.raw &&
  test_decode_color actual &&
  test_cmp expect.colors actual

> cat > expect <<\EOF
> * left
> | *---.   octopus-merge
> | |\ \ \
> |/ / / /
> | | | * 4
> | | * | 3
> | | |/
> | * | 2
> | |/
> 

gitk doesn't quit

[Alan Aversa]

To quit gitk, I have to invoke ctrl-c from the command line. I get this
error when trying to quit gitk with ctrl-q or from the GUI menu:

-- 
☧


error writing "stdout": I/O error
error writing "stdout": I/O error
while executing
"puts "Error saving config: $err""
(procedure "savestuff" line 90)
invoked from within
"savestuff ."
(procedure "doquit" line 6)
invoked from within
"doquit"
invoked from within
".#bar.#bar#file invoke active"
("uplevel" body line 1)
invoked from within
"uplevel #0 [list $w invoke active]"
(procedure "tk::MenuInvoke" line 50)
invoked from within
"tk::MenuInvoke .#bar.#bar#file 1"
(command bound to event)


signature.asc
Description: OpenPGP digital signature

Re: wrong output on fail

[Jeff King]

On Sat, Sep 29, 2018 at 03:30:21PM +1200, Paul Wratt wrote:

> --
> ...
> Total 21 (delta 8), reused 0 (delta 0)
> error: RPC failed; result=56, HTTP code = 0
> fatal: The remote end hung up unexpectedly
> fatal: The remote end hung up unexpectedly
> Everything up-to-date
> --
> 
> I am getting the above from "git push".
> 
> I am having intermittent HTTPS connection failures with my ISP
> (Vodafone NZ, mobile), although HTTP works fine. It may even be a
> peering issue.
> 
> The failures above are accounted for, but that last line is definitely wrong

Yes, that's unfortunate. Usually we'd complain if the connection drops,
but I wonder if this has to do with the extra trip through
git-remote-https. I.e., that program sees the failure and dies, but we
don't manage to pass back the error code to the main git-push process.

It sounds like this is repeatable? If so, can you try with GIT_TRACE=1
and GIT_TRANSPORT_HELPER_DEBUG=1 set in the environment?

-Peff

Re: [PATCH v2 3/3] git-help.txt: document "git help cmd" vs "git cmd --help" for aliases

[Jeff King]

On Mon, Oct 01, 2018 at 01:21:07PM +0200, Rasmus Villemoes wrote:

> This documents the existing behaviour of "git help cmd" when cmd is an
> alias, as well as providing a hint to use the "git cmd --help" form to
> be taken directly to the man page for the aliased command.

Good idea.

> diff --git a/Documentation/git-help.txt b/Documentation/git-help.txt
> index 83d25d825a..37e85868fd 100644
> --- a/Documentation/git-help.txt
> +++ b/Documentation/git-help.txt
> @@ -29,6 +29,10 @@ guide is brought up. The 'man' program is used by default 
> for this
>  purpose, but this can be overridden by other options or configuration
>  variables.
>  
> +If an alias is given, git prints a note explaining what it is an alias
> +for on standard output. To get the manual page for the aliased
> +command, use `git COMMAND --help`.

Funny English: "what it is an...". Maybe:

  If an alias is given, git shows the definition of the alias on
  standard output. To get the manual page...


-Peff

Re: [PATCH v2 2/3] git.c: handle_alias: prepend alias info when first argument is -h

[Jeff King]

On Mon, Oct 01, 2018 at 01:21:06PM +0200, Rasmus Villemoes wrote:

> Most git commands respond to -h anywhere in the command line, or at
> least as a first and lone argument, by printing the usage
> information. For aliases, we can provide a little more information that
> might be useful in interpreting/understanding the following output by
> prepending a line telling that the command is an alias, and for what.
> 
> When one invokes a simple alias, such as "cp = cherry-pick"
> with -h, this results in
> 
> $ git cp -h
> 'cp' is aliased to 'cherry-pick'
> usage: git cherry-pick [] ...
> ...
> 
> When the alias consists of more than one word, this provides the
> additional benefit of informing the user which options are implicit in
> using the alias, e.g. with "cp = cherry-pick -n":
> 
> $ git cp -h
> 'cp' is aliased to 'cherry-pick -n'
> usage: git cherry-pick [] ...
> ...
> 
> For shell commands, we cannot know how it responds to -h, but printing
> this line to stderr should not hurt, and can help in figuring out what
> is happening in a case like
> 
> $ git sc -h
> 'sc' is aliased to '!somecommand'
> somecommand: invalid option '-h'

Nicely explained.

> diff --git a/git.c b/git.c
> index a6f4b44af5..0211c2d4c0 100644
> --- a/git.c
> +++ b/git.c
> @@ -318,6 +318,9 @@ static int handle_alias(int *argcp, const char ***argv)
>   alias_command = (*argv)[0];
>   alias_string = alias_lookup(alias_command);
>   if (alias_string) {
> + if (*argcp > 1 && !strcmp((*argv)[1], "-h"))
> + fprintf_ln(stderr, _("'%s' is aliased to '%s'"),
> +alias_command, alias_string);
>   if (alias_string[0] == '!') {
>   struct child_process child = CHILD_PROCESS_INIT;
>   int nongit_ok;

And the implementation makes sense.

-Peff

Re: [PATCH v2 1/3] help: redirect to aliased commands for "git cmd --help"

[Jeff King]

On Mon, Oct 01, 2018 at 01:21:05PM +0200, Rasmus Villemoes wrote:

> As discussed in the thread for v1 of this patch [1] [2], this changes the
> rules for "git foo --help" when foo is an alias.
> 
> (0) When invoked as "git help foo", we continue to print the "foo is
> aliased to bar" message and nothing else.
> 
> (1) If foo is an alias for a shell command, print "foo is aliased to
> !bar" as usual.
> 
> (2) Otherwise, break the alias string into words, and pretend that "git
> word0 --help" was called.
> 
> At least for me, getting the man page for git-cherry-pick directly with
> "git cp --help" is more useful (and how I expect an alias to behave)
> than the short "is aliased to" notice. It is also consistent with
> "--help" generally providing more comprehensive help than "-h".

Makes sense.

> I believe that printing the "is aliased to" message also in case (2) has
> value: Depending on pager setup, or if the user has help.format=web, the
> message is still present immediately above the prompt when the user
> quits the pager/returns to the terminal. That serves as an explanation
> for why one was redirected to "man git-cherry-pick" from "git cp
> --help", and if cp is actually 'cherry-pick -n', it reminds the user
> that using cp has some flag implicitly set before firing off the next
> command.
> 
> It also provides some useful info in case we end up erroring out, either
> in the "bad alias string" check, or in the "No manual entry for gitbar"
> case.

OK, I buy that line of reasoning. And in the other cases, it shouldn't
_hurt_ anything.

> diff --git a/builtin/help.c b/builtin/help.c
> index 8d4f6dd301..4802a06f37 100644
> --- a/builtin/help.c
> +++ b/builtin/help.c
> @@ -415,9 +415,29 @@ static const char *check_git_cmd(const char* cmd)
>  
>   alias = alias_lookup(cmd);
>   if (alias) {
> - printf_ln(_("'%s' is aliased to '%s'"), cmd, alias);
> - free(alias);
> - exit(0);
> + const char **argv;
> + int count;
> +
> + /*
> +  * If we were invoked as "git help cmd", or cmd is an
> +  * alias for a shell command, we inform the user what
> +  * cmd is an alias for and do nothing else.
> +  */
> + if (!exclude_guides || alias[0] == '!') {
> + printf_ln(_("'%s' is aliased to '%s'"), cmd, alias);
> + free(alias);
> + exit(0);
> + }

I'm not sure I understand why exclude_guides is relevant. We check it
below when we know that we _don't_ have an alias. Hrm. I guess you're
using it here as a proxy for "git foo --help" being used instead of "git
help foo". The comment probably needs to spell out that exclude_guides
is the same as your "we were invoked as...".

I wonder if we could change the name of that option. It is an
undocumented, hidden option that we use internally, so it should be OK
to do so (or we could always add another one). That might prevent
somebody in the future from using --exclude-guides in more places and
breaking your assumption here.

> + /*
> +  * Otherwise, we pretend that the command was "git
> +  * word0 --help.
> +  */
> + fprintf_ln(stderr, _("'%s' is aliased to '%s'"), cmd, alias);
> + count = split_cmdline(alias, );
> + if (count < 0)
> + die(_("bad alias.%s string: %s"), cmd,
> + split_cmdline_strerror(count));
> + return alias;

So we split only to find argv[0] here. But then we don't return it. That
works because the split is done in place, meaning we must have inserted
a NUL in alias. That's sufficiently subtle that it might be worth
spelling it out in a comment.

We don't need to free alias here as we do above, because we're passing
it back. We should free argv, though, I think (not its elements, just
the array itself).

Unfortunately the caller is going to leak our returned "alias", but I'm
not sure we can do much about it. I'm not overly concerned with the
memory, but it is going to trigger leak-checkers (and we're trying to
quiet them down, not go the other way). I think it may be OK to overlook
that and just UNLEAK() it in cmd_help().

-Peff

Die Kontaktdaten des Pfarrer sind wie folgt

[Monika Stefen]

Lieber,

mitteile ich Ihnen dass, ich das Geschaeft mit Erfolg zu Ende 
geschaft. Und ich bin im Momentan in Venezuela. Aber ich moechte Sie 
mit der Summe USD350,000.00 beloehnen fuer Ihre Unterstuetzung und 
Beistand waehrend unserer Zussammenarbeit. Auf diesen Grund habe ich 
einen Pfarrer eingeschaltet um das Geld wie Provison-USD350,000.00 auf 
Ihr Konto zu ueberweisen.

Der Pfarrer hat auch die Moeglichkeit das Geld per CHEQUE an Sie per 
DHL-Service schicken oder Sie koennen selber das Cheque bei dem Pfarrer 
abholen. Auf duesen Punkt, mussen Sie kontakt mit dem Pfarrer aufnehmen 
um zu wissen wie Sie das Geld/Cheque von USD350,000.00 abholen koennen.

Die Kontaktdaten des Pfarrer sind wie folgt/

Sein Name ist  Pfarrer Melvyn Davis
Email: rev.mel...@yahoo.com
Telefonnummer (00225 54 76 28 89)

Mit freundlichen Gruessen, 
Monika Stefen

Re: [BUG] A part of an edge from an octopus merge gets colored, even with --color=never

[Noam Postavsky]

On Mon, 24 Sep 2018 at 20:27, Noam Postavsky
 wrote:
>
> On Sat, 8 Sep 2018 at 12:13, Jeff King  wrote:
>
> > Great (and sorry for the delayed response).
>
> No problem, I know it's not the most urgent bug ever :)

Ping. :)

> I managed to recast my script into the framework of the
> other tests (see attached t4299-octopus.sh); it seems like it should
> go into t4202-log.sh, but it's not clear to me how I can do this
> without breaking all the other tests which expect a certain sequence
> of commits.

Re: [PATCH] more oideq/hasheq conversions

[Jacob Keller]

On Tue, Oct 2, 2018 at 2:19 PM Jeff King  wrote:
>
> We added faster equality-comparison functions for hashes in
> 14438c4497 (introduce hasheq() and oideq(), 2018-08-28). A
> few topics were in-flight at the time, and can now be
> converted. This covers all spots found by "make coccicheck"
> in master (the coccicheck results were tweaked by hand for
> style).
>
> Signed-off-by: Jeff King 
> ---
> Jake: I was surprised that this was not a "patch 2" on top of your
> earlier coccicheck patch. Apologies if you were planning to send it out.
>

Nope, I hadn't gotten around to this yet, thanks!

The conversions also look good to me :)

Regards,
Jake

Re: [PATCH v4 3/4] transport.c: introduce core.alternateRefsCommand

[Jeff King]

On Mon, Oct 01, 2018 at 07:23:58PM -0700, Taylor Blau wrote:

> +core.alternateRefsCommand::
> + When advertising tips of available history from an alternate, use the 
> shell to
> + execute the specified command instead of linkgit:git-for-each-ref[1]. 
> The
> + first argument is the absolute path of the alternate. Output must 
> contain one
> + hex object id per line (i.e., the same as produce by `git for-each-ref
> + --format='%(objectname)'`).
> ++
> +This is useful when a repository only wishes to advertise some of its
> +alternate's references as `.have`'s. For example, to only advertise branch
> +heads, configure `core.alternateRefsCommand` to the path of a script which 
> runs
> +`git --git-dir="$1" for-each-ref --format='%(objectname)' refs/heads`.
> ++
> +Note that the configured value is executed in a shell, and thus
> +linkgit:git-for-each-ref[1] by itself does not work, as scripts have to 
> handle
> +the path argument specially.

This last paragraph is trying to fix the wrong-impression that we
discussed in the last round. But I'm not sure it doesn't make things
more confusing. ;)

Specifically, the problem isn't the shell. The issue is that we pass the
repo path as an argument to the command. So either:

  - it's a real command that we run, in which case git-for-each-ref does
not take a repo path argument and so doesn't work; or

  - it's a shell snippet, in which case the argument is appended to the
snippet (and here's where you can get into a rabbit hole of
explaining how our shell invocation works, and we should avoid that)

Can we just say:

  Note that you cannot generally put `git for-each-ref` directly into
  the config value, as it does not take a repository path as an argument
  (but you can wrap the command above in a shell script).

> [...]

The rest of the patch looks good to me, along with the other three
(modulo the "expect" fixup you already sent).

-Peff

[PATCH v4 4/4] transport.c: introduce core.alternateRefsPrefixes

[Taylor Blau]

On Tue, Oct 02, 2018 at 04:13:13PM +0100, Ramsay Jones wrote:
>
> On 02/10/18 03:24, Taylor Blau wrote:
> [snip]
> > diff --git a/t/t5410-receive-pack-alternates.sh 
> > b/t/t5410-receive-pack-alternates.sh
> > index 49d0fe44fb..94794c35da 100755
> > --- a/t/t5410-receive-pack-alternates.sh
> > +++ b/t/t5410-receive-pack-alternates.sh
> > @@ -30,4 +30,12 @@ test_expect_success 'with core.alternateRefsCommand' '
> > test_cmp expect actual.haves
> >  '
> >
> > +test_expect_success 'with core.alternateRefsPrefixes' '
> > +   test_config -C fork core.alternateRefsPrefixes "refs/heads/private" &&
> > +   git rev-parse private/branch expect &&
>
> s/expect/>expect/ ?

Ah, certainly. Thanks for catching my mistake. I've resent 4/4 as below.

Junio -- if you find this re-roll to be acceptable, please queue this
patch instead of the one that it is in reply to.

-- >8 --

The recently-introduced "core.alternateRefsCommand" allows callers to
specify with high flexibility the tips that they wish to advertise from
alternates. This flexibility comes at the cost of some inconvenience
when the caller only wishes to limit the advertisement to one or more
prefixes.

For example, to advertise only tags, a caller using
'core.alternateRefsCommand' would have to do:

  $ git config core.alternateRefsCommand ' \
  f() { git -C "$1" for-each-ref \
  refs/tags --format="%(objectname)" }; f "$@"'

The above is cumbersome to write, so let's introduce a
"core.alternateRefsPrefixes" to address this common case. Instead, the
caller can run:

  $ git config core.alternateRefsPrefixes 'refs/tags'

Which will behave identically to the longer example using
"core.alternateRefsCommand".

Since the value of "core.alternateRefsPrefixes" is appended to 'git
for-each-ref' and then executed, include a "--" before taking the
configured value to avoid misinterpreting arguments as flags to 'git
for-each-ref'.

In the case that the caller wishes to specify multiple prefixes, they
may separate them by whitespace. If "core.alternateRefsCommand" is set,
it will take precedence over "core.alternateRefsPrefixes".

Signed-off-by: Taylor Blau 
---
 Documentation/config.txt   | 7 +++
 t/t5410-receive-pack-alternates.sh | 8 
 transport.c| 5 +
 3 files changed, 20 insertions(+)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index ac0577d288..1dc5eb3cfa 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -632,6 +632,13 @@ Note that the configured value is executed in a shell, and 
thus
 linkgit:git-for-each-ref[1] by itself does not work, as scripts have to handle
 the path argument specially.

+core.alternateRefsPrefixes::
+   When listing references from an alternate, list only references that 
begin
+   with the given prefix. Prefixes match as if they were given as 
arguments to
+   linkgit:git-for-each-ref[1]. To list multiple prefixes, separate them 
with
+   whitespace. If `core.alternateRefsCommand` is set, setting
+   `core.alternateRefsPrefixes` has no effect.
+
 core.bare::
If true this repository is assumed to be 'bare' and has no
working directory associated with it.  If this is the case a
diff --git a/t/t5410-receive-pack-alternates.sh 
b/t/t5410-receive-pack-alternates.sh
index 49d0fe44fb..457c20c2a5 100755
--- a/t/t5410-receive-pack-alternates.sh
+++ b/t/t5410-receive-pack-alternates.sh
@@ -30,4 +30,12 @@ test_expect_success 'with core.alternateRefsCommand' '
test_cmp expect actual.haves
 '

+test_expect_success 'with core.alternateRefsPrefixes' '
+   test_config -C fork core.alternateRefsPrefixes "refs/heads/private" &&
+   git rev-parse private/branch >expect &&
+   printf "" | git receive-pack fork >actual &&
+   extract_haves actual.haves &&
+   test_cmp expect actual.haves
+'
+
 test_done
diff --git a/transport.c b/transport.c
index e271b66603..83474add28 100644
--- a/transport.c
+++ b/transport.c
@@ -1341,6 +1341,11 @@ static void fill_alternate_refs_command(struct 
child_process *cmd,
argv_array_pushf(>args, "--git-dir=%s", repo_path);
argv_array_push(>args, "for-each-ref");
argv_array_push(>args, "--format=%(objectname)");
+
+   if (!git_config_get_value("core.alternateRefsPrefixes", 
)) {
+   argv_array_push(>args, "--");
+   argv_array_split(>args, value);
+   }
}

cmd->env = local_repo_env;
--
2.19.0.221.g150f307af

Re: Git Evolve

[Taylor Blau]

On Tue, Oct 02, 2018 at 11:11:11AM +0200, Ævar Arnfjörð Bjarmason wrote:

You timed this email quite well ;-).

> On Tue, Oct 02 2018, Taylor Blau wrote:
>
> > Hi Stefan,
> >
> > On Sat, Sep 29, 2018 at 04:00:04PM -0700, Stefan Xenos wrote:
> >> Hello, List!
> >>
> >> I'm interested in porting something like Mercurial's evolve command to
> >> Git.
> >
> > Welcome to Git :-). I think that the discussion in this thread is good,
> > but it's not why I'm replying. I have also wanted a Mercurial feature in
> > Git, but a different one than yours.
> >
> > Specifically, I've wanted the 'hg absorb' command. My understanding of
> > the commands functionality is that it builds a sort of flamegraph-esque
> > view of the blame, and then cascades downwards parts of a change. I am
> > sure that I'm not doing the command justice, so I'll defer to [1] where
> > it is explained in more detail.
> >
> > The benefit of this command is that it gives you a way to--without
> > ambiguity--absorb changes into earlier commits, and in fact, the
> > earliest commit that they make sense to belong to.
> >
> > This would simplify my workflow greatly when re-rolling patches, as I
> > often want to rewrite a part of an earlier commit. This is certainly
> > possible by a number of different `git rebase` invocations (e.g., (1)
> > create fixup commits, and then re-order them, or (2) mark points in your
> > history as 'edit', and rewrite them in a detached state, and I'm sure
> > many more).
> >
> > I'm curious if you or anyone else has thought about how this might work
> > in Git.
>
> I've wanted a "git absorb" for a while, but have done no actual work on
> it, I just found out about it.
>
> I think a combination of these two heuristics would probably do the
> trick:
>
>  1. If a change in your "git diff" output has a hunk whose lines overlap
> with an earlier commit in the @{u}.. range, we do the equivalent of
> "git add -p", select that hunk, and "git commit --fixup  commit>". We fixup the most recent commit that matches (otherwise
> commit>we'd conflict).

I had imagined this working slightly differently. I think about it in
terms of a flamegraph-shape, where each line is affected by gravity.
Consider this:

   [---]
  L0   L1   L2   L3   L4   L5   L6   L7

Here's a line in a diff that affects L1-L4. Were we to create a
``fixup'' to L3-L4, it would look like this:

 [-] --|
   [-|-] <-|
  L0   L1   L2   L3   L4   L5   L6   L7

The commit owning the adjacent edit hunk is the one that gets the
changes applied to it.

Consider instead the case where we have two overlapping parts of a
change:

 [-|]
   [-|-]
  L0   L1   L2   L3   L4   L5   L6   L7

The left-hand side of the top-most hunk belongs to the hunk below it,
but the right-hand side is ``affected by gravity'' down to the base:

 [-|
   [-|-]|---]
  L0   L1   L2   L3   L4   L5   L6   L7

And thus could be reapplied anywhere. I have not spent time proving this,
but I believe that this resolves appropriate bases without ambiguity
(or, at least can detect when finding a base introduces ambiguity).

>  2. Have some mode where we fall back from #1 and consider changes to
> entire files, if that's unambiguous.
>
> The neat thing about this would be that you could tweak how promiscuous
> #1 would be via the -U option to git-diff, and #2 would just be a
> special case of -U9 (we should really add a -Uinf...).
>
> Then once you ran this you could run "git rebase -i --autosquash" to see
> how the TODO list would look, and optionally have some "git absorb
> --now" or whatever to do the "git add -p", "git commit --fixup" and "git
> rebase --autosquash" all in one go.

That's cool. I hadn't known about '--autosquash' before, but it now
makes sense to me why I have often seen patches that begin with
"fixup!".

Another thought that I am reminded of was an off-list discussion with
Peff, where we thought that a particularly Git-like way to implement
something like this would be to generate a set of commits that would be
immediately '--autosquash'-able.

> > [1]: http://files.lihdd.net/hgabsorb-note.pdf

Thanks,
Taylor

Re: git clone works in ide but not git bash CLI

[Johannes Schindelin]

Hi David,

On Mon, 24 Sep 2018, David Brown wrote:

> Howdy, I have a conundrum:
> 
> App: Spring Cloud Config Server
> envvars: GIT_URL and SSH_KEY
> IDE: Intellij 2018.2.4 Ultimate
> 
> When I use the IDE to assign the SSH_KEY value all is copacetic.
> 
> If I assign the envvar at the Git Bash CLI:
> 
> com.jcraft.jsch.JSchException: Auth fail
> 
> Any guesses?

This sounds like you use JGit somehow? Whereas "Git Bash CLI" suggests
that you actually want to use Git for Windows?

Ciao,
Johannes

> Thanks and regards,
> 
> 

Re: cannot dowload from your amazon server a windows git distro

[Taylor Blau]

Hi,

On Tue, Oct 02, 2018 at 01:59:51PM +0700, WEB0 - Helmut wrote:
> hi,
> you amazon download is not working.
> do you have some free location to download please?

I was able to download both 32- and 64-bit copies of the non-portable
Windows installer for version 2.19.0. I used the link at [1] to do so.

Can you try again and see if it is a reproducible failure? If so, either
the website is sending you to the wrong place, or there is a problem
with GitHub (in which case you should contact supp...@github.com, which
will route it to the right place.)

Thanks,
Taylor

[1]: https://git-scm.com/download/win

Re: [PATCH 1/2] commit-graph: clean up leaked memory during write

[Stefan Beller]

>
> My preference is to avoid them in the name of simplicity. If you're
> using "make SANITIZE=leak test" to check for leaks, it will skip these
> cases. If you're using valgrind, I think these may be reported as
> "reachable". But that number already isn't useful for finding real
> leaks, because it includes cases like the "foo" above as well as
> program-lifetime globals.
>
> The only argument (IMHO) for such an UNLEAK() is that it annotates the
> location for when somebody later changes the function to "return -1"
> instead of dying. But if we are going to do such annotation, we may as
> well actually call free(), which is what the "return" version will
> ultimately have to do.

Heh, that was part of my reasoning why we'd want to have *something*.

> I'd actually be _more_ favorable to calling free() instead of UNLEAK()
> there, but I'm still mildly negative, just because it may go stale (and
> our leak-checking wouldn't usefully notice these cases). Anybody
> converting that die() to a return needs to re-analyze the function for
> what might need to be released (and that includes non-memory bits like
> descriptors, too).

Sounds reasonable, so then the consensus (between Martin, you and me)
is to drop the UNLEAK.

Re: [PATCH 1/2] commit-graph: clean up leaked memory during write

[Jeff King]

On Tue, Oct 02, 2018 at 10:59:28AM -0700, Stefan Beller wrote:

> > Generally speaking, it
> > seems impossible to UNLEAK when dying, since we don't know what we have
> > allocated higher up in the call-stack.
> 
> I do not understand; I thought UNLEAK was specifically for the purpose of
> die() calls without imposing extra overhead; rereading 0e5bba53af
> (add UNLEAK annotation for reducing leak false positives, 2017-09-08)
> doesn't provide an example for prematurely die()ing, only for regular
> program exit.

I responded elsewhere, but as the author of UNLEAK, let me comment here:
it was intended only for program exit. That's why there are no such
examples. :)

If you're using it anywhere except the return from a cmd_* function, or
a static-local helper that's called from a cmd_*, you should probably
actually be freeing the memory.

-Peff

Re: [PATCH 1/2] commit-graph: clean up leaked memory during write

[Jeff King]

On Tue, Oct 02, 2018 at 12:44:09PM -0700, Stefan Beller wrote:

> > My worry is that one of these would seem to be true:
> >
> > * UNLEAK is unsuitable for the job. Whenever we have a `die()` as we do
> >   here, we can UNLEAK the variables we know of, but we can't do anything
> >   about the allocations we have made higher up the call-chain.
> 
> IMHO that is the issue of the functions higher up the call chain and ought
> to not affect this patch. By doing the right thing here locally the code base
> will approach a good state eventually.

But it's impossible. If I do this:

  foo = xstrdup(bar);
  subfunction(foo);

then I cannot protect myself from leaking "foo" when subfunction() calls
die(). It must be valid when I enter the function, and I have no
opportunity to run code when it leaves (because it never does).

> > * We add code with no purpose. In this case, we're not talking a lot of
> >   lines, but across the code base, if they bring no gain, they are bound
> >   to provide a negative net value given enough time.
> 
> I see. I did not estimate its negative impact to be high enough, as the
> UNLEAK near a die() call was obvious good thing (locally).
> 
> I don't know what the best way to proceed is in this case.

My preference is to avoid them in the name of simplicity. If you're
using "make SANITIZE=leak test" to check for leaks, it will skip these
cases. If you're using valgrind, I think these may be reported as
"reachable". But that number already isn't useful for finding real
leaks, because it includes cases like the "foo" above as well as
program-lifetime globals.

The only argument (IMHO) for such an UNLEAK() is that it annotates the
location for when somebody later changes the function to "return -1"
instead of dying. But if we are going to do such annotation, we may as
well actually call free(), which is what the "return" version will
ultimately have to do.

I'd actually be _more_ favorable to calling free() instead of UNLEAK()
there, but I'm still mildly negative, just because it may go stale (and
our leak-checking wouldn't usefully notice these cases). Anybody
converting that die() to a return needs to re-analyze the function for
what might need to be released (and that includes non-memory bits like
descriptors, too).

-Peff

Re: Git Evolve

[Kyle Meyer]

Ævar Arnfjörð Bjarmason  writes:

> On Tue, Oct 02 2018, Taylor Blau wrote:

[...]

>> Specifically, I've wanted the 'hg absorb' command. My understanding of
>> the commands functionality is that it builds a sort of flamegraph-esque
>> view of the blame, and then cascades downwards parts of a change. I am
>> sure that I'm not doing the command justice, so I'll defer to [1] where
>> it is explained in more detail.
>>
>> The benefit of this command is that it gives you a way to--without
>> ambiguity--absorb changes into earlier commits, and in fact, the
>> earliest commit that they make sense to belong to.
>>
>> This would simplify my workflow greatly when re-rolling patches, as I
>> often want to rewrite a part of an earlier commit. This is certainly
>> possible by a number of different `git rebase` invocations (e.g., (1)
>> create fixup commits, and then re-order them, or (2) mark points in your
>> history as 'edit', and rewrite them in a detached state, and I'm sure
>> many more).
>>
>> I'm curious if you or anyone else has thought about how this might work
>> in Git.
>
> I've wanted a "git absorb" for a while, but have done no actual work on
> it, I just found out about it.

It may be worth looking into git-autofixup [0] (author cc'd).  I learned
about it when Magit used it in its magit-commit-absorb command, but I
haven't used it yet myself.

[0] https://github.com/torbiak/git-autofixup

-- 
Kyle

Re: [PATCH 1/1] protocol: limit max protocol version per service

[Stefan Beller]

On Tue, Oct 2, 2018 at 3:00 PM Josh Steadmon  wrote:
>
> For services other than git-receive-pack, clients currently advertise
> that they support the version set in the protocol.version config,
> regardless of whether or not there is actually an implementation of that
> service for the given protocol version. This causes backwards-
> compatibility problems when a new implementation for the given
> protocol version is added.
>
> This patch sets maximum allowed protocol versions for git-receive-pack,
> git-upload-archive, and git-upload-pack.
>
> Previously, git-receive-pack would downgrade from v2 to v0, but would
> allow v1 if set in protocol.version. Now, it will downgrade from v2 to
> v1.

But does git-receive-pack understand v1?
As to my understanding we have not even defined v1
for push (receive-pack) and archive --remote (upload-archive).
v1 is only known to fetch (upload-pack).

> +enum protocol_version determine_maximum_protocol_version(
> +   const char *service, enum protocol_version default_version)
> +{
> +   if (!strcmp(service, "git-receive-pack"))
> +   return protocol_v1;
> +   else if (!strcmp(service, "git-upload-archive"))
> +   return protocol_v1;

so I would think these two would be _v0.
... goes and checks ...
aa9bab29b8 (upload-pack, receive-pack: introduce protocol version 1,
2017-10-16) seems to actually teach v1 to receive-pack as well,
but upload-archive was completely off radar, so I think returning
(v1, v0, v2 in the order as in the code) would make sense?

Asides from this, I thought there was a deliberate decision
that we'd want to avoid a strict order on the protocol versions,
but I could not find prior discussion on list to back up this claim. :/

For example we'd go with e.g. enums instead of integers
for version numbers, as then some internal setup could
also have things like protocol_v2018-10-02 or protocol_vWhatever;
some protocol version may be advantageous to the client, some to
the server, and we'd need to negotiate the best version that both
are happy with. (e.g. the server may like version 0, 2 and 3, and
the client may like 0,2,4 as 3 is bad security wise for the client,
so both would negotiate to 2 as their best case)

>From a maintenance perspective, do we want to keep
this part of the code central, as it ties protocol (as proxied
by service name) to the max version number?
I would think that we'd rather have the decision local to the
code, i.e. builtin/fetch would need to tell protocol.c that it
can do (0,1,2) and builtin/push can do (0,1), and then the
networking layers of code would figure out by the input
from the caller and the input from the user (configured
protocol.version) what is the best to go forward from
then on.

But I guess having the central place here is not to
bad either. How will it cope with the desire of protocol v2
to have only one end point (c.f. serve.{c,h} via builtin/serve
as "git serve") ?

Stefan

[PATCH 1/1] protocol: limit max protocol version per service

[Josh Steadmon]

For services other than git-receive-pack, clients currently advertise
that they support the version set in the protocol.version config,
regardless of whether or not there is actually an implementation of that
service for the given protocol version. This causes backwards-
compatibility problems when a new implementation for the given
protocol version is added.

This patch sets maximum allowed protocol versions for git-receive-pack,
git-upload-archive, and git-upload-pack.

Previously, git-receive-pack would downgrade from v2 to v0, but would
allow v1 if set in protocol.version. Now, it will downgrade from v2 to
v1.

Signed-off-by: Josh Steadmon 
---
 connect.c | 11 ---
 protocol.c| 13 +
 protocol.h|  7 +++
 remote-curl.c | 11 ---
 4 files changed, 28 insertions(+), 14 deletions(-)

diff --git a/connect.c b/connect.c
index 94547e5056..4a8cd78239 100644
--- a/connect.c
+++ b/connect.c
@@ -1228,14 +1228,11 @@ struct child_process *git_connect(int fd[2], const char 
*url,
struct child_process *conn;
enum protocol protocol;
enum protocol_version version = get_protocol_version_config();
+   enum protocol_version max_version;
 
-   /*
-* NEEDSWORK: If we are trying to use protocol v2 and we are planning
-* to perform a push, then fallback to v0 since the client doesn't know
-* how to push yet using v2.
-*/
-   if (version == protocol_v2 && !strcmp("git-receive-pack", prog))
-   version = protocol_v0;
+   max_version = determine_maximum_protocol_version(prog, version);
+   if (version > max_version)
+   version = max_version;
 
/* Without this we cannot rely on waitpid() to tell
 * what happened to our children.
diff --git a/protocol.c b/protocol.c
index 5e636785d1..1c553d8b99 100644
--- a/protocol.c
+++ b/protocol.c
@@ -79,3 +79,16 @@ enum protocol_version 
determine_protocol_version_client(const char *server_respo
 
return version;
 }
+
+enum protocol_version determine_maximum_protocol_version(
+   const char *service, enum protocol_version default_version)
+{
+   if (!strcmp(service, "git-receive-pack"))
+   return protocol_v1;
+   else if (!strcmp(service, "git-upload-archive"))
+   return protocol_v1;
+   else if (!strcmp(service, "git-upload-pack"))
+   return protocol_v2;
+
+   return default_version;
+}
diff --git a/protocol.h b/protocol.h
index 2ad35e433c..eabc0c5fab 100644
--- a/protocol.h
+++ b/protocol.h
@@ -31,4 +31,11 @@ extern enum protocol_version 
determine_protocol_version_server(void);
  */
 extern enum protocol_version determine_protocol_version_client(const char 
*server_response);
 
+/*
+ * Used by a client to determine the maximum protocol version to advertise for 
a
+ * particular service. If the service is unrecognized, return default_version.
+ */
+extern enum protocol_version determine_maximum_protocol_version(
+   const char *service, enum protocol_version default_version);
+
 #endif /* PROTOCOL_H */
diff --git a/remote-curl.c b/remote-curl.c
index fb28309e85..028adb76ae 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -344,6 +344,7 @@ static struct discovery *discover_refs(const char *service, 
int for_push)
int http_ret, maybe_smart = 0;
struct http_get_options http_options;
enum protocol_version version = get_protocol_version_config();
+   enum protocol_version max_version;
 
if (last && !strcmp(service, last->service))
return last;
@@ -360,13 +361,9 @@ static struct discovery *discover_refs(const char 
*service, int for_push)
strbuf_addf(_url, "service=%s", service);
}
 
-   /*
-* NEEDSWORK: If we are trying to use protocol v2 and we are planning
-* to perform a push, then fallback to v0 since the client doesn't know
-* how to push yet using v2.
-*/
-   if (version == protocol_v2 && !strcmp("git-receive-pack", service))
-   version = protocol_v0;
+   max_version = determine_maximum_protocol_version(service, version);
+   if (version > max_version)
+   version = max_version;
 
/* Add the extra Git-Protocol header */
if (get_protocol_http_header(version, _header))
-- 
2.19.0.605.g01d371f741-goog

[PATCH 0/1] Limit client version advertisements

[Josh Steadmon]

As discussed in [1], clients will incorrectly advertise support for
protocol version 2 even when the service in question does not have a v2
implementation. This patch sets maximum protocol versions for
git-receive-pack, git-upload-archive, and git-upload-pack.

[1]: https://public-inbox.org/git/20180927012455.234876-1-stead...@google.com/

Josh Steadmon (1):
  protocol: limit max protocol version per service

 connect.c | 11 ---
 protocol.c| 13 +
 protocol.h|  7 +++
 remote-curl.c | 11 ---
 4 files changed, 28 insertions(+), 14 deletions(-)

-- 
2.19.0.605.g01d371f741-goog

Re: Git 2.19.01 on Windows crasesh during GC

[Johannes Schindelin]

Hi,

On Fri, 14 Sep 2018, Michal Fita wrote:

> Problem signature:
>   Problem Event Name: APPCRASH
>   Application Name:   git.exe
>   Application Version:2.19.0.1
>   Application Timestamp:  5b980bc7
>   Fault Module Name:  ntdll.dll
>   Fault Module Version:   6.1.7601.24117
>   Fault Module Timestamp: 5add228d
>   Exception Code: c005
>   Exception Offset:   00032964
>   OS Version: 6.1.7601.2.1.0.256.48
>   Locale ID:  2057
>   Additional Information 1:   335e
>   Additional Information 2:   335ee83054d6c615e4a7142c362e3dd4
>   Additional Information 3:   5184
>   Additional Information 4:   518485c5adbc52c624cc6890056919a6

Sadly, this is hardly enough information to even start a wild goose chase
of a debugging spree.

Happily, I *think* that another report will help you, too:
https://github.com/git-for-windows/git/issues/1839

In short: please test with a new snapshot from
https://wingit.blob.core.windows.net/files/index.html

Ciao,
Johannes

Re: [PATCH] more oideq/hasheq conversions

[Derrick Stolee]

On 10/2/2018 5:19 PM, Jeff King wrote:

We added faster equality-comparison functions for hashes in
14438c4497 (introduce hasheq() and oideq(), 2018-08-28). A
few topics were in-flight at the time, and can now be
converted. This covers all spots found by "make coccicheck"
in master (the coccicheck results were tweaked by hand for
style).

Signed-off-by: Jeff King 
---
Jake: I was surprised that this was not a "patch 2" on top of your
earlier coccicheck patch. Apologies if you were planning to send it out.

This doesn't conflict with anything in "pu", so it's a reasonable time
to apply it. There are a few lingering cases in pu, so another option is
to wait a week or two and see if they get merged.

These conversions look good to me!

Reviewed-by: Derrick Stolee 

[PATCH] more oideq/hasheq conversions

[Jeff King]

We added faster equality-comparison functions for hashes in
14438c4497 (introduce hasheq() and oideq(), 2018-08-28). A
few topics were in-flight at the time, and can now be
converted. This covers all spots found by "make coccicheck"
in master (the coccicheck results were tweaked by hand for
style).

Signed-off-by: Jeff King 
---
Jake: I was surprised that this was not a "patch 2" on top of your
earlier coccicheck patch. Apologies if you were planning to send it out.

This doesn't conflict with anything in "pu", so it's a reasonable time
to apply it. There are a few lingering cases in pu, so another option is
to wait a week or two and see if they get merged.

 builtin/checkout.c | 3 ++-
 cache-tree.c   | 2 +-
 commit-reach.c | 2 +-
 midx.c | 8 
 4 files changed, 8 insertions(+), 7 deletions(-)

diff --git a/builtin/checkout.c b/builtin/checkout.c
index b30b48767e..902c06702c 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -497,7 +497,8 @@ static int skip_merge_working_tree(const struct 
checkout_opts *opts,
 * We must do the merge if we are actually moving to a new commit.
 */
if (!old_branch_info->commit || !new_branch_info->commit ||
-   oidcmp(_branch_info->commit->object.oid, 
_branch_info->commit->object.oid))
+   !oideq(_branch_info->commit->object.oid,
+  _branch_info->commit->object.oid))
return 0;
 
/*
diff --git a/cache-tree.c b/cache-tree.c
index 5ce51468f0..9c5cf2cc4f 100644
--- a/cache-tree.c
+++ b/cache-tree.c
@@ -781,7 +781,7 @@ static void verify_one(struct index_state *istate,
strbuf_add(_buf, oid->hash, the_hash_algo->rawsz);
}
hash_object_file(tree_buf.buf, tree_buf.len, tree_type, _oid);
-   if (oidcmp(_oid, >oid))
+   if (!oideq(_oid, >oid))
BUG("cache-tree for path %.*s does not match. "
"Expected %s got %s", len, path->buf,
oid_to_hex(_oid), oid_to_hex(>oid));
diff --git a/commit-reach.c b/commit-reach.c
index 00e5ceee6f..a7808430e1 100644
--- a/commit-reach.c
+++ b/commit-reach.c
@@ -426,7 +426,7 @@ struct contains_stack {
 static int in_commit_list(const struct commit_list *want, struct commit *c)
 {
for (; want; want = want->next)
-   if (!oidcmp(>item->object.oid, >object.oid))
+   if (oideq(>item->object.oid, >object.oid))
return 1;
return 0;
 }
diff --git a/midx.c b/midx.c
index f3e8dbc108..06aef56400 100644
--- a/midx.c
+++ b/midx.c
@@ -285,8 +285,8 @@ static int nth_midxed_pack_entry(struct multi_pack_index 
*m, struct pack_entry *
struct object_id oid;
nth_midxed_object_oid(, m, pos);
for (i = 0; i < p->num_bad_objects; i++)
-   if (!hashcmp(oid.hash,
-p->bad_object_sha1 + the_hash_algo->rawsz 
* i))
+   if (hasheq(oid.hash,
+  p->bad_object_sha1 + the_hash_algo->rawsz * 
i))
return 0;
}
 
@@ -583,8 +583,8 @@ static struct pack_midx_entry *get_sorted_entries(struct 
multi_pack_index *m,
 * Take only the first duplicate.
 */
for (cur_object = 0; cur_object < nr_fanout; cur_object++) {
-   if (cur_object && !oidcmp(_by_fanout[cur_object 
- 1].oid,
- 
_by_fanout[cur_object].oid))
+   if (cur_object && oideq(_by_fanout[cur_object - 
1].oid,
+   
_by_fanout[cur_object].oid))
continue;
 
ALLOC_GROW(deduplicated_entries, *nr_objects + 1, 
alloc_objects);
-- 
2.19.0.489.g7c40b6af8e

Re: [PATCH] coccicheck: process every source file at once

[Jeff King]

On Tue, Oct 02, 2018 at 01:58:10PM -0700, Jacob Keller wrote:

> On Tue, Oct 2, 2018 at 1:31 PM Jeff King  wrote:
> > Actually, I guess we do not need to save $? at all, since we have only a
> > single process to care about. So even simpler:
> >
> >   spatch ... 2>$@+ 2>$@.log ||
> >   {
> > cat $@.log
> > exit 1
> >   }
> >   # if we get here, we were successful
> >   mv $@+ $@ ;# etc
> >
> > would work. That's missing all the Makefile=required backslashes and
> > semicolons, of course. ;)
> >
> 
> I opted to drop to just save the return, immediately after calling.
> It's a bit less code change, and I think the result is as clear as the
> above would be. This way we do drop the subshell, not that it matters
> much in the end...

Yeah. To be clear, I'm fine with any of the versions discussed in this
thread. Thanks for working on this!

-Peff

Re: [PATCH v9 19/21] stash: convert `stash--helper.c` into `stash.c`

[Thomas Gummerer]

On 09/26, Paul-Sebastian Ungureanu wrote:
> The old shell script `git-stash.sh`  was removed and replaced
> entirely by `builtin/stash.c`. In order to do that, `create` and
> `push` were adapted to work without `stash.sh`. For example, before
> this commit, `git stash create` called `git stash--helper create
> --message "$*"`. If it called `git stash--helper create "$@"`, then
> some of these changes wouldn't have been necessary.
> 
> This commit also removes the word `helper` since now stash is
> called directly and not by a shell script.
> 
> Signed-off-by: Paul-Sebastian Ungureanu 
> ---
>  .gitignore   |   1 -
>  Makefile |   3 +-
>  builtin.h|   2 +-
>  builtin/{stash--helper.c => stash.c} | 162 ---
>  git-stash.sh | 153 -
>  git.c|   2 +-
>  6 files changed, 98 insertions(+), 225 deletions(-)
>  rename builtin/{stash--helper.c => stash.c} (90%)
>  delete mode 100755 git-stash.sh
>
> [...]
>
> @@ -1571,7 +1562,44 @@ int cmd_stash__helper(int argc, const char **argv, 
> const char *prefix)
>   return !!push_stash(argc, argv, prefix);
>   else if (!strcmp(argv[0], "save"))
>   return !!save_stash(argc, argv, prefix);
> + else if (*argv[0] != '-')
> + usage_msg_opt(xstrfmt(_("unknown subcommand: %s"), argv[0]),
> +   git_stash_usage, options);
> +
> + if (strcmp(argv[0], "-p")) {
> + while (++i < argc && strcmp(argv[i], "--")) {
> + /*
> +  * `akpqu` is a string which contains all short options,
> +  * except `-m` which is verified separately.
> +  */
> + if ((strlen(argv[i]) == 2) && *argv[i] == '-' &&
> + strchr("akpqu", argv[i][1]))
> + continue;
> +
> + if (!strcmp(argv[i], "--all") ||
> + !strcmp(argv[i], "--keep-index") ||
> + !strcmp(argv[i], "--no-keep-index") ||
> + !strcmp(argv[i], "--patch") ||
> + !strcmp(argv[i], "--quiet") ||
> + !strcmp(argv[i], "--include-untracked"))
> + continue;
> +
> + /*
> +  * `-m` and `--message=` are verified separately because
> +  * they need to be immediately followed by a string
> +  * (i.e.`-m"foobar"` or `--message="foobar"`).
> +  */
> + if ((strlen(argv[i]) > 2 &&
> +  !strncmp(argv[i], "-m", 2)) ||
> + (strlen(argv[i]) > 10 &&
> +  !strncmp(argv[i], "--message=", 10)))

These 'strlen && !strncmp' calls could be replaced with
'starts_with()'.

> + continue;
> +
> + usage_with_options(git_stash_usage, options);
> + }
> + }

This is a bit more complex than what we used to have, which was just
"if it starts with a "-" it's an option", but I don't think it hurts
being more explicit here either.

>  
> - usage_msg_opt(xstrfmt(_("unknown subcommand: %s"), argv[0]),
> -   git_stash_helper_usage, options);
> + argv_array_push(, "push");
> + argv_array_pushv(, argv);
> + return !!push_stash(args.argc, args.argv, prefix);
>  }

Re: git projects with submodules in different sites - in txt format (:+(

[Philip Oakley]

On 02/10/2018 06:47, Michele Hallak wrote:

Hi,

I am getting out of idea about how to change the methodology we are using in 
order to ease our integration process... Close to despair, I am throwing the 
question to you...

We have 6 infrastructure repositories [A, B, C, D, E, F ?].



Each project [W,X,Y,Z] is composed of 4 repositories [1-4], each one using one 
or two infrastructure repositories as sub-modules. (Not the same)


e.g. W1-W4; with say B & D as submodules


The infrastructure repositories are common to several projects and in the case 
we have to make change in the infrastructure for a specific project, we are 
doing it on a specific branch until properly merged.


Do you also have remotes setup that provide backup and central authority 
to the projects..?


Everything is fine (more or less) and somehow working.


Good..


Now, we have one project that will be developed in another site and with 
another git server physically separated from the main site.


Is it networked? Internal control, external internet, sneakernet?


I copied the infrastructure repositories in the new site and removed and add 
the sub-modules in order for them to point to the url in the separated git 
server.

Every 2 weeks, the remotely developed code has to be integrated back in the 
main site.
My idea was to format GIT patches, integrate in the main site, tag the whole 
thing and ship back the integrated tagged code to the remote site.
... and now the nightmare starts:
yep, you have lost the validation & verification capability of Git's 
sha1/oid and DAG.




Since the .gitmodules is different, I cannot have the same SHA and then same 
tag and I am never sure that the integrated code is proper.


Remotes, remotes...


May be there is a simple solution that I don't know about to my problem? Is 
there something else than GIT patches? Should I simply ship to the remote site 
the code as is and change the submodules each time?



I think the solution you need is `git bundle` 
https://git-scm.com/docs/git-bundle. This is designed for the case where 
you do not have the regular git transport infrastructure. Instead it 
records the expected data that would be 'on the wire', which is then 
read in at the far end. The bundle can contain excess data to ensure 
overlap between site transmissions.


You just run the projects in the same way but add the courier step for 
shipping the CD, or some password protected archive as per your security 
needs.


Everything is should be just fine (more or less) and somehow it will 
just work. ;-)


--
Philip
https://stackoverflow.com/questions/11792671/how-to-git-bundle-a-complete-repo

Re: [PATCH] coccicheck: process every source file at once

[Jacob Keller]

On Tue, Oct 2, 2018 at 1:31 PM Jeff King  wrote:
> Actually, I guess we do not need to save $? at all, since we have only a
> single process to care about. So even simpler:
>
>   spatch ... 2>$@+ 2>$@.log ||
>   {
> cat $@.log
> exit 1
>   }
>   # if we get here, we were successful
>   mv $@+ $@ ;# etc
>
> would work. That's missing all the Makefile=required backslashes and
> semicolons, of course. ;)
>

I opted to drop to just save the return, immediately after calling.
It's a bit less code change, and I think the result is as clear as the
above would be. This way we do drop the subshell, not that it matters
much in the end...

Thanks,
Jake

> -Peff

Re: [PATCH] branch: colorize branches checked out in a linked working tree the same way as the current branch is colorized

[Johannes Schindelin]

Hi Ævar,

On Thu, 27 Sep 2018, Ævar Arnfjörð Bjarmason wrote:

> On Thu, Sep 27 2018, Nickolai Belakovski wrote:
> 
> > Will do re: screenshot when I get home, although it's pretty easy to
> > imagine, the git branch output will have one other branch colored in green,
> > bit without the asterisk (for one linked worktree) :)
> >
> > Also will do re: changing comments to /**/ (didn't know // was from C++,
> > TIL) and I'll clean up the comments to remove some of the more obvious
> > ones, but I'll try to keep a comment explaining the basic flow of creating
> > a nest if statement to evaluate worktree refs for color.
> >
> > And yes, I copy/pasted into gmail. I was having trouble setting up
> > send-email, but I think I may have it figured out now. Should I create a
> > new thread with send-email? Or maybe reply to this one (I can do that by
> > specifying the Message-ID to reply to right?
> 
> You'd run git format-patch master..your-topic with
> --subject-prefix="PATCH v2" and
> --in-reply-to="".
>  Then
> it'll show up in reply to your v1.

There is also a nice tutorial in
https://github.com/git-for-windows/git/blob/master/CONTRIBUTING.md#submit-your-patch
(which, contrary to the location, is useful for non-Windows developers,
too.)

> You can also for an easier experience do this via GitGitGadget, see
> https://github.com/gitgitgadget/gitgitgadget looking at its code it
> seems to have some way to reference a Message-ID, but I don't know how
> to trigger that.

IIRC GitGitGadget has no facility yet to reply to any mail it did not
generate itself (i.e. if you did not generate v1 using GitGitGadget, then
it cannot generate a v2 that replies to the previous iteration).

This might change at some stage, but I have other priorities for now.
(Which should not stop any contributor from opening a PR to scratch their
own favorite itch.)

Ciao,
Johannes

> 
> > This is my first time using this workflow, so I appreciate your
> > patience :) )?
> 
> No worries, happy to help.
> 
> > On Thu, Sep 27, 2018 at 8:33 AM Ævar Arnfjörð Bjarmason 
> > wrote:
> >
> >>
> >> On Thu, Sep 27 2018, Nickolai Belakovski wrote:
> >>
> >> > In order to more clearly display which branches are active, the output
> >> > of git branch is modified to colorize branches checked out in any linked
> >> > worktrees with the same color as the current branch.
> >> >
> >> > This is meant to simplify workflows related to worktree, particularly
> >> > due to the limitations of not being able to check out the same branch in
> >> > two worktrees and the inability to delete a branch checked out in a
> >> > worktree. When performing branch operations like checkout and delete, it
> >> > would be useful to know more readily if the branches in which the user
> >> > is interested are already checked out in a worktree.
> >> >
> >> > The git worktree list command contains the relevant information, however
> >> > this is a much less frquently used command than git branch.
> >> >
> >> > Signed-off-by: Nickolai Belakovski 
> >>
> >> Sounds cool, b.t.w. would be neat-o to have some screenshot uploaded to
> >> imgur or whatever just to skim what it looks like before/after.
> >>
> >> > diff --git a/builtin/branch.c b/builtin/branch.c
> >> > index 4fc55c350..65b58ff7c 100644
> >> > --- a/builtin/branch.c
> >> > +++ b/builtin/branch.c
> >> > @@ -334,11 +334,36 @@ static char *build_format(struct ref_filter
> >> > *filter, int maxwidth, const char *r
> >> > struct strbuf local = STRBUF_INIT;
> >> > struct strbuf remote = STRBUF_INIT;
> >> >
> >> > -   strbuf_addf(, "%%(if)%%(HEAD)%%(then)* %s%%(else)
> >> %s%%(end)",
> >> > -   branch_get_color(BRANCH_COLOR_CURRENT),
> >> > -   branch_get_color(BRANCH_COLOR_LOCAL));
> >> > -   strbuf_addf(, "  %s",
> >> > -   branch_get_color(BRANCH_COLOR_REMOTE));
> >> > +   // Prepend the current branch of this worktree with "* " and
> >> > all other branches with "  "
> >>
> >>
> >> We use /* ... */ C comments, not C++-style // (well, it's in C now, but
> >> not the ancient versions we need to support).
> >>
> >> It also seems all of this patch was copy/pasted into GMail or something,
> >> it has wrapping and doesn't apply with "git am".
> >>
> >> Also most/all of these comments I'd say we could better do without,
> >> i.e. the ones explaining basic code flow that's easy to see from the
> >> code itself.
> >>
> 

Re: [PATCH v9 16/21] stash: convert push to builtin

[Thomas Gummerer]

On 09/26, Paul-Sebastian Ungureanu wrote:
> Add stash push to the helper.
> 
> Signed-off-by: Paul-Sebastian Ungureanu 
> ---
>  builtin/stash--helper.c | 244 +++-
>  git-stash.sh|   6 +-
>  2 files changed, 244 insertions(+), 6 deletions(-)
> 
> diff --git a/builtin/stash--helper.c b/builtin/stash--helper.c
> index 49b05f2458..d79233d7ec 100644
> --- a/builtin/stash--helper.c
> +++ b/builtin/stash--helper.c
> @@ -23,6 +23,9 @@ static const char * const git_stash_helper_usage[] = {
>   N_("git stash--helper ( pop | apply ) [--index] [-q|--quiet] 
> []"),
>   N_("git stash--helper branch  []"),
>   N_("git stash--helper clear"),
> + N_("git stash--helper [push [-p|--patch] [-k|--[no-]keep-index] 
> [-q|--quiet]\n"
> +"  [-u|--include-untracked] [-a|--all] [-m|--message 
> ]\n"
> +"  [--] [...]]"),
>   NULL
>  };
>  
> @@ -71,6 +74,13 @@ static const char * const git_stash_helper_create_usage[] 
> = {
>   NULL
>  };
>  
> +static const char * const git_stash_helper_push_usage[] = {
> + N_("git stash--helper [push [-p|--patch] [-k|--[no-]keep-index] 
> [-q|--quiet]\n"
> +"  [-u|--include-untracked] [-a|--all] [-m|--message 
> ]\n"
> +"  [--] [...]]"),
> + NULL
> +};
> +
>  static const char *ref_stash = "refs/stash";
>  static struct strbuf stash_index_path = STRBUF_INIT;
>  
> @@ -1088,7 +1098,7 @@ static int stash_working_tree(struct stash_info *info, 
> struct pathspec ps)
>  
>  static int do_create_stash(struct pathspec ps, char **stash_msg,
>  int include_untracked, int patch_mode,
> -struct stash_info *info)
> +struct stash_info *info, struct strbuf *patch)
>  {
>   int ret = 0;
>   int flags = 0;
> @@ -1102,7 +1112,6 @@ static int do_create_stash(struct pathspec ps, char 
> **stash_msg,
>   struct strbuf commit_tree_label = STRBUF_INIT;
>   struct strbuf untracked_files = STRBUF_INIT;
>   struct strbuf stash_msg_buf = STRBUF_INIT;
> - struct strbuf patch = STRBUF_INIT;
>  
>   read_cache_preload(NULL);
>   refresh_cache(REFRESH_QUIET);
> @@ -1152,7 +1161,7 @@ static int do_create_stash(struct pathspec ps, char 
> **stash_msg,
>   untracked_commit_option = 1;
>   }
>   if (patch_mode) {
> - ret = stash_patch(info, ps, );
> + ret = stash_patch(info, ps, patch);
>   *stash_msg = NULL;
>   if (ret < 0) {
>   fprintf_ln(stderr, _("Cannot save the current worktree 
> state"));
> @@ -1221,7 +1230,8 @@ static int create_stash(int argc, const char **argv, 
> const char *prefix)
>0);
>  
>   memset(, 0, sizeof(ps));
> - ret = do_create_stash(ps, _msg, include_untracked, 0, );
> + ret = do_create_stash(ps, _msg, include_untracked, 0, ,
> +   NULL);
>  
>   if (!ret)
>   printf_ln("%s", oid_to_hex(_commit));
> @@ -1234,6 +1244,230 @@ static int create_stash(int argc, const char **argv, 
> const char *prefix)
>   return ret < 0;
>  }
>  
> +static int do_push_stash(struct pathspec ps, char *stash_msg, int quiet,
> +  int keep_index, int patch_mode, int include_untracked)
> +{
> + int ret = 0;
> + struct stash_info info;
> + struct strbuf patch = STRBUF_INIT;
> +
> + if (patch_mode && keep_index == -1)
> + keep_index = 1;
> +
> + if (patch_mode && include_untracked) {
> + fprintf_ln(stderr, _("Can't use --patch and --include-untracked"
> +  " or --all at the same time"));
> + ret = -1;

We should set "stash_msg" to NULL here, otherwise we'll get an invalid
free if these options are used together and a message is given.

In general I find this API and the do_create_stash API a bit
cumbersome to use.  Maybe it would help if we'd 'xstrdup' the string
before passing it in, so we can free it unconditionally, without
setting it to NULL everywhere.  As it's only a single strdup for each
command that's run that isn't very costly compared to what else we're
doing here, and I think the readability we're gaining would be worth
it.

> + goto done;
> + }
> +
> + read_cache_preload(NULL);
> + if (!include_untracked && ps.nr) {
> + int i;
> + char *ps_matched = xcalloc(ps.nr, 1);
> +
> + for (i = 0; i < active_nr; i++)
> + ce_path_match(_index, active_cache[i], ,
> +   ps_matched);
> +
> + if (report_path_error(ps_matched, , NULL)) {
> + fprintf_ln(stderr, _("Did you forget to 'git add'?"));
> + stash_msg = NULL;
> + ret = -1;

'ps_matched' is not being free'd in this error case.

> + goto done;
> + 

Re: [PATCH] coccicheck: process every source file at once

[Jeff King]

On Tue, Oct 02, 2018 at 01:00:21PM -0700, Jacob Keller wrote:

> > > This is nearly a 4x decrease in the time required to run make
> > > coccicheck. This is due to the overhead of restarting spatch for every
> > > file. By processing all files at once, we can amortize this startup cost
> > > across the total number of files, rather than paying it once per file.
> >
> > One minor side effect is that we lose the opportunity to parallelize
> > quite as much. However, I think the reduction in total CPU makes it well
> > worth that (and we still have 8 cocci files and growing, which should
> > keep at least 8 cores busy).
> >
> 
> I don't think we do any less than previously, because we are doing
> each file in a for-loop, so those would all be serial anyways.

Yeah, sorry to be unclear. This is a strict improvement on what was
there before. We had floated some patches to do the full NxM
parallelization, but I think this path is better because of the
reduction in actual CPU used (and if more recent versions of spatch can
parallelize internally, we'll eventually get the best of both worlds).

> > > diff --git a/Makefile b/Makefile
> > > index df1df9db78da..b9947f3f51ec 100644
> > > --- a/Makefile
> > > +++ b/Makefile
> > > @@ -2715,10 +2715,8 @@ endif
> > >  %.cocci.patch: %.cocci $(COCCI_SOURCES)
> > >   @echo '' SPATCH $<; \
> > >   ret=0; \
> > > - for f in $(COCCI_SOURCES); do \
> > > - $(SPATCH) --sp-file $< $$f $(SPATCH_FLAGS) || \
> > > - { ret=$$?; break; }; \
> > > - done >$@+ 2>$@.log; \
> > > + ( $(SPATCH) --sp-file $< $(COCCI_SOURCES) $(SPATCH_FLAGS) || \
> > > + { ret=$$?; }; ) >$@+ 2>$@.log; \
> >
> > This looks pretty straight-forward. I wondered if we could get rid of
> > the "ret" handling, since we don't need to pass the error back out of
> > the loop. But it's also used for the "show the log only on error" logic
> > below:
> >
> 
> We could probably get rid of it by doing the spatch without an ||, and
> then just save $?.

Actually, I guess we do not need to save $? at all, since we have only a
single process to care about. So even simpler:

  spatch ... 2>$@+ 2>$@.log ||
  {
cat $@.log
exit 1
  }
  # if we get here, we were successful
  mv $@+ $@ ;# etc

would work. That's missing all the Makefile=required backslashes and
semicolons, of course. ;)

-Peff

Re: v2.19.0 Git install doesn't allow VS Code as an editor

[Johannes Schindelin]

Hi,

On Fri, 14 Sep 2018, Taylor Blau wrote:

> Hi Zachary,
> 
> On Fri, Sep 14, 2018 at 09:43:43AM -0500, Zachary Bryant wrote:
> > When the installer asks for a default editor, it defaults to vim and
> > when I select either VS Code option, it won't allow me to proceed.
> 
> It sounds like this is an issue pertaining to Git for Windows, which
> uses an issue tracker that is separate from the mailing list here. Their
> tracker is at [1], but I'm cc-ing the maintainer here to let him know.

see https://github.com/git-for-windows/build-extra/pull/200

Ciao,
Johannes

Re: [PATCH v9 15/21] stash: convert create to builtin

[Thomas Gummerer]

On 09/26, Paul-Sebastian Ungureanu wrote:
> Add stash create to the helper.
> 
> Signed-off-by: Paul-Sebastian Ungureanu 
> ---
>  builtin/stash--helper.c | 450 
>  git-stash.sh|   2 +-
>  2 files changed, 451 insertions(+), 1 deletion(-)
> 
> diff --git a/builtin/stash--helper.c b/builtin/stash--helper.c
> index b7421b68aa..49b05f2458 100644
> --- a/builtin/stash--helper.c
> +++ b/builtin/stash--helper.c
> @@ -12,6 +12,9 @@
>  #include "rerere.h"
>  #include "revision.h"
>  #include "log-tree.h"
> +#include "diffcore.h"
> +
> +#define INCLUDE_ALL_FILES 2
>  
>  static const char * const git_stash_helper_usage[] = {
>   N_("git stash--helper list []"),
> @@ -63,6 +66,11 @@ static const char * const git_stash_helper_store_usage[] = 
> {
>   NULL
>  };
>  
> +static const char * const git_stash_helper_create_usage[] = {
> + N_("git stash--helper create []"),
> + NULL
> +};
> +
>  static const char *ref_stash = "refs/stash";
>  static struct strbuf stash_index_path = STRBUF_INIT;
>  
> @@ -289,6 +297,24 @@ static int reset_head(void)
>   return run_command();
>  }
>  
> +static void add_diff_to_buf(struct diff_queue_struct *q,
> + struct diff_options *options,
> + void *data)
> +{
> + int i;
> +
> + for (i = 0; i < q->nr; i++) {
> + strbuf_addstr(data, q->queue[i]->one->path);
> +
> + /*
> +  * The reason we add "0" at the end of this strbuf
> +  * is because we will pass the output further to
> +  * "git update-index -z ...".
> +  */
> + strbuf_addch(data, 0);

I'd find it slightly clearer to pass '\0' instead of 0 here.  That
makes it immediately clear to the reader that we mean the NUL
character (even though it all ends up being the same in the end).

> + }
> +}
> +
>  static int get_newly_staged(struct strbuf *out, struct object_id *c_tree)
>  {
>   struct child_process cp = CHILD_PROCESS_INIT;
> @@ -786,6 +812,428 @@ static int store_stash(int argc, const char **argv, 
> const char *prefix)
>   return do_store_stash(, stash_msg, quiet);
>  }
>  
> +static void add_pathspecs(struct argv_array *args,
> +struct pathspec ps) {

This indentation looks a bit weird.

> + int i;
> +
> + for (i = 0; i < ps.nr; i++)
> + argv_array_push(args, ps.items[i].match);
> +}
> +
> +/*
> + * `untracked_files` will be filled with the names of untracked files.
> + * The return value is:
> + *
> + * = 0 if there are not any untracked files
> + * > 0 if there are untracked files
> + */
> +static int get_untracked_files(struct pathspec ps, int include_untracked,
> +struct strbuf *untracked_files)
> +{
> + int i;
> + int max_len;
> + int found = 0;
> + char *seen;
> + struct dir_struct dir;
> +
> + memset(, 0, sizeof(dir));
> + if (include_untracked != INCLUDE_ALL_FILES)
> + setup_standard_excludes();
> +
> + seen = xcalloc(ps.nr, 1);
> +
> + max_len = fill_directory(, the_repository->index, );
> + for (i = 0; i < dir.nr; i++) {
> + struct dir_entry *ent = dir.entries[i];
> + if (dir_path_match(_index, ent, , max_len, seen)) {
> + found++;
> + strbuf_addstr(untracked_files, ent->name);
> + /* NUL-terminate: will be fed to update-index -z */
> + strbuf_addch(untracked_files, 0);
> + }
> + free(ent);
> + }
> +
> + free(seen);
> + free(dir.entries);
> + free(dir.ignored);
> + clear_directory();
> + return found;
> +}
> +
> +/*
> + * The return value of `check_changes()` can be:
> + *
> + * < 0 if there was an error
> + * = 0 if there are no changes.
> + * > 0 if there are changes.
> + */
> +static int check_changes(struct pathspec ps, int include_untracked)
> +{
> + int result;
> + int ret = 0;

This variable is unused, so compilation with -Werror breaks at this
patch.

> + struct rev_info rev;
> + struct object_id dummy;
> + struct strbuf out = STRBUF_INIT;
> +
> + /* No initial commit. */
> + if (get_oid("HEAD", ))
> + return -1;
> +
> + if (read_cache() < 0)
> + return -1;
> +
> + init_revisions(, NULL);
> + rev.prune_data = ps;
> +
> + rev.diffopt.flags.quick = 1;
> + rev.diffopt.flags.ignore_submodules = 1;
> + rev.abbrev = 0;
> +
> + add_head_to_pending();
> + diff_setup_done();
> +
> + result = run_diff_index(, 1);
> + if (diff_result_code(, result))
> + return 1;
> +
> + object_array_clear();
> + result = run_diff_files(, 0);
> + if (diff_result_code(, result))
> + return 1;
> +
> + if (include_untracked && get_untracked_files(ps, include_untracked,
> + 

Re: [PATCH v3] coccicheck: process every source file at once

[Jacob Keller]

On Tue, Oct 2, 2018 at 1:07 PM Jacob Keller  wrote:
>
> From: Jacob Keller 
>
> make coccicheck is used in order to apply coccinelle semantic patches,
> and see if any of the transformations found within contrib/coccinelle/
> can be applied to the current code base.
>
> Pass every file to a single invocation of spatch, instead of running
> spatch once per source file.
>
> This reduces the time required to run make coccicheck by a significant
> amount of time:
>
> Prior timing of make coccicheck
>   real6m14.090s
>   user25m2.606s
>   sys 1m22.919s
>
> New timing of make coccicheck
>   real1m36.580s
>   user7m55.933s
>   sys 0m18.219s
>
> This is nearly a 4x decrease in the time required to run make
> coccicheck. This is due to the overhead of restarting spatch for every
> file. By processing all files at once, we can amortize this startup cost
> across the total number of files, rather than paying it once per file.
>
> Signed-off-by: Jacob Keller 
> ---

Forgot to add what changed. I dropped the subshell and "||" bit around
invoking spatch.

Thanks,
Jake


>  Makefile | 6 ++
>  1 file changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/Makefile b/Makefile
> index df1df9db78da..da692ece9e12 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -2715,10 +2715,8 @@ endif
>  %.cocci.patch: %.cocci $(COCCI_SOURCES)
> @echo '' SPATCH $<; \
> ret=0; \
> -   for f in $(COCCI_SOURCES); do \
> -   $(SPATCH) --sp-file $< $$f $(SPATCH_FLAGS) || \
> -   { ret=$$?; break; }; \
> -   done >$@+ 2>$@.log; \
> +   $(SPATCH) --sp-file $< $(COCCI_SOURCES) $(SPATCH_FLAGS) >$@+ 
> 2>$@.log; \
> +   ret=$$?; \
> if test $$ret != 0; \
> then \
> cat $@.log; \
> --
> 2.18.0.219.gaf81d287a9da
>

[PATCH v3] coccicheck: process every source file at once

[Jacob Keller]

From: Jacob Keller 

make coccicheck is used in order to apply coccinelle semantic patches,
and see if any of the transformations found within contrib/coccinelle/
can be applied to the current code base.

Pass every file to a single invocation of spatch, instead of running
spatch once per source file.

This reduces the time required to run make coccicheck by a significant
amount of time:

Prior timing of make coccicheck
  real6m14.090s
  user25m2.606s
  sys 1m22.919s

New timing of make coccicheck
  real1m36.580s
  user7m55.933s
  sys 0m18.219s

This is nearly a 4x decrease in the time required to run make
coccicheck. This is due to the overhead of restarting spatch for every
file. By processing all files at once, we can amortize this startup cost
across the total number of files, rather than paying it once per file.

Signed-off-by: Jacob Keller 
---
 Makefile | 6 ++
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/Makefile b/Makefile
index df1df9db78da..da692ece9e12 100644
--- a/Makefile
+++ b/Makefile
@@ -2715,10 +2715,8 @@ endif
 %.cocci.patch: %.cocci $(COCCI_SOURCES)
@echo '' SPATCH $<; \
ret=0; \
-   for f in $(COCCI_SOURCES); do \
-   $(SPATCH) --sp-file $< $$f $(SPATCH_FLAGS) || \
-   { ret=$$?; break; }; \
-   done >$@+ 2>$@.log; \
+   $(SPATCH) --sp-file $< $(COCCI_SOURCES) $(SPATCH_FLAGS) >$@+ 2>$@.log; \
+   ret=$$?; \
if test $$ret != 0; \
then \
cat $@.log; \
-- 
2.18.0.219.gaf81d287a9da

Re: [PATCH v2] coccicheck: process every source file at once

[Jacob Keller]

On Tue, Oct 2, 2018 at 1:03 PM Jacob Keller  wrote:
>
> From: Jacob Keller 
>
> make coccicheck is used in order to apply coccinelle semantic patches,
> and see if any of the transformations found within contrib/coccinelle/
> can be applied to the current code base.
>
> Pass every file to a single invocation of spatch, instead of running
> spatch once per source file.
>
> This reduces the time required to run make coccicheck by a significant
> amount of time:
>
> Prior timing of make coccicheck
>   real6m14.090s
>   user25m2.606s
>   sys 1m22.919s
>
> New timing of make coccicheck
>   real1m36.580s
>   user7m55.933s
>   sys 0m18.219s
>
> This is nearly a 4x decrease in the time required to run make
> coccicheck. This is due to the overhead of restarting spatch for every
> file. By processing all files at once, we can amortize this startup cost
> across the total number of files, rather than paying it once per file.
>
> Signed-off-by: Jacob Keller 
> ---

Woops, ignore this version, it doesn't quite work.

Thanks,
Jake

[PATCH v2] coccicheck: process every source file at once

[Jacob Keller]

From: Jacob Keller 

make coccicheck is used in order to apply coccinelle semantic patches,
and see if any of the transformations found within contrib/coccinelle/
can be applied to the current code base.

Pass every file to a single invocation of spatch, instead of running
spatch once per source file.

This reduces the time required to run make coccicheck by a significant
amount of time:

Prior timing of make coccicheck
  real6m14.090s
  user25m2.606s
  sys 1m22.919s

New timing of make coccicheck
  real1m36.580s
  user7m55.933s
  sys 0m18.219s

This is nearly a 4x decrease in the time required to run make
coccicheck. This is due to the overhead of restarting spatch for every
file. By processing all files at once, we can amortize this startup cost
across the total number of files, rather than paying it once per file.

Signed-off-by: Jacob Keller 
---
 Makefile | 6 ++
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/Makefile b/Makefile
index df1df9db78da..b9947f3f51ec 100644
--- a/Makefile
+++ b/Makefile
@@ -2715,10 +2715,8 @@ endif
 %.cocci.patch: %.cocci $(COCCI_SOURCES)
@echo '' SPATCH $<; \
ret=0; \
-   for f in $(COCCI_SOURCES); do \
-   $(SPATCH) --sp-file $< $$f $(SPATCH_FLAGS) || \
-   { ret=$$?; break; }; \
-   done >$@+ 2>$@.log; \
+   ( $(SPATCH) --sp-file $< $(COCCI_SOURCES) $(SPATCH_FLAGS) || \
+   { ret=$$?; }; ) >$@+ 2>$@.log; \
if test $$ret != 0; \
then \
cat $@.log; \
-- 
2.18.0.219.gaf81d287a9da

Re: [PATCH] coccicheck: process every source file at once

[Jacob Keller]

On Tue, Oct 2, 2018 at 12:55 PM Jeff King  wrote:
>
> On Tue, Oct 02, 2018 at 12:16:42PM -0700, Jacob Keller wrote:
>
> > make coccicheck is used in order to apply coccinelle semantic patches,
> > and see if any of the transformations found within contrib/coccinelle/
> > can be applied to the current code base.
> >
> > Pass every file to a single invocation of spatch, instead of running
> > spatch once per source file.
> >
> > This reduces the time required to run make coccicheck by a significant
> > amount of time:
> >
> > Prior timing of make coccicheck
> >   real6m14.090s
> >   user25m2.606s
> >   sys 1m22.919s
> >
> > New timing of make coccicheck
> >   real1m36.580s
> >   user7m55.933s
> >   sys 0m18.219s
>
> Yay! This is a nice result.
>
> It's also one of the things that Julia suggested in an earlier thread.
> One thing I wasn't quite sure about after digging into the various
> versions (1.0.4 on Debian stable/unstable, 1.0.6 in experimental, and
> pre-1.0.7 at the bleeding edge) was whether the old versions would be
> similarly helped (or work at all).
>
> I just replicated your results with 1.0.4.deb-3+b2 from Debian stable.
> It's possible there are older versions floating around, but for
> something developer-only like this, I think "in Debian stable" is a
> reasonable enough cutoff.
>

Good. I hadn't checked back too far, but I know support for multiple
files has existed since quite a while.

> > This is nearly a 4x decrease in the time required to run make
> > coccicheck. This is due to the overhead of restarting spatch for every
> > file. By processing all files at once, we can amortize this startup cost
> > across the total number of files, rather than paying it once per file.
>
> One minor side effect is that we lose the opportunity to parallelize
> quite as much. However, I think the reduction in total CPU makes it well
> worth that (and we still have 8 cocci files and growing, which should
> keep at least 8 cores busy).
>

I don't think we do any less than previously, because we are doing
each file in a for-loop, so those would all be serial anyways.

> I think recent versions of Coccinelle will actually parallelize
> internally, too, but my 1.0.4 doesn't seem to. That's probably what I
> was thinking of earlier (but this is still a win even without that).
>
> It looks like this also fixes a problem I ran into when doing the oideq
> work, which is that the patch for a header file would get shown multiple
> times (once for each file that includes it). So this is faster _and_
> more correct. Double-yay.
>

Woot :D

> > diff --git a/Makefile b/Makefile
> > index df1df9db78da..b9947f3f51ec 100644
> > --- a/Makefile
> > +++ b/Makefile
> > @@ -2715,10 +2715,8 @@ endif
> >  %.cocci.patch: %.cocci $(COCCI_SOURCES)
> >   @echo '' SPATCH $<; \
> >   ret=0; \
> > - for f in $(COCCI_SOURCES); do \
> > - $(SPATCH) --sp-file $< $$f $(SPATCH_FLAGS) || \
> > - { ret=$$?; break; }; \
> > - done >$@+ 2>$@.log; \
> > + ( $(SPATCH) --sp-file $< $(COCCI_SOURCES) $(SPATCH_FLAGS) || \
> > + { ret=$$?; }; ) >$@+ 2>$@.log; \
>
> This looks pretty straight-forward. I wondered if we could get rid of
> the "ret" handling, since we don't need to pass the error back out of
> the loop. But it's also used for the "show the log only on error" logic
> below:
>

We could probably get rid of it by doing the spatch without an ||, and
then just save $?.

> >   if test $$ret != 0; \
> >   then \
> >   cat $@.log; \
>
> The subshell could be a {}, though, I think, but it's not that big a
> deal either way.
>
> -Peff

Re: [PATCH] coccicheck: process every source file at once

[Jeff King]

On Tue, Oct 02, 2018 at 12:16:42PM -0700, Jacob Keller wrote:

> make coccicheck is used in order to apply coccinelle semantic patches,
> and see if any of the transformations found within contrib/coccinelle/
> can be applied to the current code base.
> 
> Pass every file to a single invocation of spatch, instead of running
> spatch once per source file.
> 
> This reduces the time required to run make coccicheck by a significant
> amount of time:
> 
> Prior timing of make coccicheck
>   real6m14.090s
>   user25m2.606s
>   sys 1m22.919s
> 
> New timing of make coccicheck
>   real1m36.580s
>   user7m55.933s
>   sys 0m18.219s

Yay! This is a nice result.

It's also one of the things that Julia suggested in an earlier thread.
One thing I wasn't quite sure about after digging into the various
versions (1.0.4 on Debian stable/unstable, 1.0.6 in experimental, and
pre-1.0.7 at the bleeding edge) was whether the old versions would be
similarly helped (or work at all).

I just replicated your results with 1.0.4.deb-3+b2 from Debian stable.
It's possible there are older versions floating around, but for
something developer-only like this, I think "in Debian stable" is a
reasonable enough cutoff.

> This is nearly a 4x decrease in the time required to run make
> coccicheck. This is due to the overhead of restarting spatch for every
> file. By processing all files at once, we can amortize this startup cost
> across the total number of files, rather than paying it once per file.

One minor side effect is that we lose the opportunity to parallelize
quite as much. However, I think the reduction in total CPU makes it well
worth that (and we still have 8 cocci files and growing, which should
keep at least 8 cores busy).

I think recent versions of Coccinelle will actually parallelize
internally, too, but my 1.0.4 doesn't seem to. That's probably what I
was thinking of earlier (but this is still a win even without that).

It looks like this also fixes a problem I ran into when doing the oideq
work, which is that the patch for a header file would get shown multiple
times (once for each file that includes it). So this is faster _and_
more correct. Double-yay.

> diff --git a/Makefile b/Makefile
> index df1df9db78da..b9947f3f51ec 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -2715,10 +2715,8 @@ endif
>  %.cocci.patch: %.cocci $(COCCI_SOURCES)
>   @echo '' SPATCH $<; \
>   ret=0; \
> - for f in $(COCCI_SOURCES); do \
> - $(SPATCH) --sp-file $< $$f $(SPATCH_FLAGS) || \
> - { ret=$$?; break; }; \
> - done >$@+ 2>$@.log; \
> + ( $(SPATCH) --sp-file $< $(COCCI_SOURCES) $(SPATCH_FLAGS) || \
> + { ret=$$?; }; ) >$@+ 2>$@.log; \

This looks pretty straight-forward. I wondered if we could get rid of
the "ret" handling, since we don't need to pass the error back out of
the loop. But it's also used for the "show the log only on error" logic
below:

>   if test $$ret != 0; \
>   then \
>   cat $@.log; \

The subshell could be a {}, though, I think, but it's not that big a
deal either way.

-Peff

Re: [PATCH 1/2] commit-graph: clean up leaked memory during write

[Stefan Beller]

On Tue, Oct 2, 2018 at 12:09 PM Martin Ågren  wrote:
>
> On Tue, 2 Oct 2018 at 19:59, Stefan Beller  wrote:
> > > > +
> > > > +   string_list_clear(, 0);
> > > >  }
> > >
> > > Nit: The blank line adds some asymmetry, IMVHO.
> >
> > I think these blank lines are super common, as in:
> >
> > {
> >   declarations;
> >
> >   multiple;
> >   lines(of);
> >   code;
> >
> >   cleanup;
> >   and_frees;
> > }
> >
> > (c.f. display_table in column.c, which I admit to have
> > cherry-picked as an example).
> >
> > While in nit territory, I would rather move the string list init
> > into the first block:
> >
> >   {
> > struct string_list list = STRING_LIST_INIT_DUP;
> >
> > for_each_ref(add_ref_to_list, );
> > write_commit_graph(obj_dir, NULL, , append);
> >
> > string_list_clear(, 0);
> >   }
>
> Now this looks very symmetrical. :-)
>
> > > >  void write_commit_graph(const char *obj_dir,
> > > > @@ -846,9 +848,11 @@ void write_commit_graph(const char *obj_dir,
> > > > compute_generation_numbers(, report_progress);
> > > >
> > > > graph_name = get_commit_graph_filename(obj_dir);
> > > > -   if (safe_create_leading_directories(graph_name))
> > > > +   if (safe_create_leading_directories(graph_name)) {
> > > > +   UNLEAK(graph_name);
> > > > die_errno(_("unable to create leading directories of 
> > > > %s"),
> > > >   graph_name);
> > > > +   }
> > >
> > > Do you really need this hunk?
> >
> > graph_name is produced via xstrfmt in get_commit_graph_filename,
> > so it needs to be free'd in any return/exit path.
>
> Agreed. Although I am questioning that `die()` and its siblings count.
>
> > > In my testing with LeakSanitizer and
> > > valgrind, I don't need this hunk to be leak-free.
> >
> >
> > > Generally speaking, it
> > > seems impossible to UNLEAK when dying, since we don't know what we have
> > > allocated higher up in the call-stack.
> >
> > I do not understand; I thought UNLEAK was specifically for the purpose of
> > die() calls without imposing extra overhead; rereading 0e5bba53af
> > (add UNLEAK annotation for reducing leak false positives, 2017-09-08)
> > doesn't provide an example for prematurely die()ing, only for regular
> > program exit.
> >
> > > [...] With this hunk, I am
> > > puzzled and feel uneasy, both about having to UNLEAK before dying and
> > > about having to UNLEAK outside of builtin/.
> >
> > I am not uneasy about an UNLEAK before dying, but about dying outside
> > builtin/ in general
>
> Yeah, not dying would be even better (out of scope for this patch).
>
> > (but having a die call accompanied by UNLEAK seems
> > to be the right thing). Can you explain the worries you have regarding the
> > allocations on the call stack, as xstrfmt is allocating on the heap and we
> > only UNLEAK the pointer to that?
>
> I think we agree that leaking things "allocat[ed] on the call stack"
> isn't much of a worry. The reason I mentioned the call stack is that
> we've got any number of calls behind us on it, and we might have made
> all sorts of allocations on the heap, and at this point, we have no
> idea about what we should be UNLEAK-ing.

Wouldn't that be the responsibility of each function to make sure things
are UNLEAK'd or free'd before the function is either over or stopped
intermittently (by a subroutine dying) ?

In an ideal world we'd only ever exit/die in the functions high up
the call chain (which are in builtin/) and all other code would gracefully
return error codes or messages instead or even cope with some failure
conditions?

> My worry is that one of these would seem to be true:
>
> * UNLEAK is unsuitable for the job. Whenever we have a `die()` as we do
>   here, we can UNLEAK the variables we know of, but we can't do anything
>   about the allocations we have made higher up the call-chain.

IMHO that is the issue of the functions higher up the call chain and ought
to not affect this patch. By doing the right thing here locally the code base
will approach a good state eventually.

> Our test
>   suite obviously provokes lots of calls to `die()` -- imagine that each
>   of those leaves a few leaked allocations behind. We'd have a semi-huge
>   number of leaks being reported. While we could mark with UNLEAK to
>   reduce that number, we wouldn't be able to bring the number of leaks
>   down to anywhere near manageable where we'd be able to find the last
>   few true positives.

Makes sense.

> * We add code with no purpose. In this case, we're not talking a lot of
>   lines, but across the code base, if they bring no gain, they are bound
>   to provide a negative net value given enough time.

I see. I did not estimate its negative impact to be high enough, as the
UNLEAK near a die() call was obvious good thing (locally).

I don't know what the best way to proceed is in this case.

Thanks,
Stefan

Re: [PATCH v9 13/21] stash: mention options in `show` synopsis.

[Thomas Gummerer]

> Subject: stash: mention options in `show` synopsis.

Really minor point, but the '.' in the end should be dropped.

Also as this is fixing a pre-existing problem I would have put this
patch near the beginning of the series, rather than in between
conversions of functions, and just incorporated the bits that changed
in the helper into the patch that converts 'git stash show'.

The rest of the patch looks good to me.

On 09/26, Paul-Sebastian Ungureanu wrote:
> Mention in the usage text and in the documentation, that `show`
> accepts any option known to `git diff`.
> 
> Signed-off-by: Paul-Sebastian Ungureanu 
> ---
>  Documentation/git-stash.txt | 4 ++--
>  builtin/stash--helper.c | 4 ++--
>  2 files changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/Documentation/git-stash.txt b/Documentation/git-stash.txt
> index 7ef8c47911..e31ea7d303 100644
> --- a/Documentation/git-stash.txt
> +++ b/Documentation/git-stash.txt
> @@ -9,7 +9,7 @@ SYNOPSIS
>  
>  [verse]
>  'git stash' list []
> -'git stash' show []
> +'git stash' show [] []
>  'git stash' drop [-q|--quiet] []
>  'git stash' ( pop | apply ) [--index] [-q|--quiet] []
>  'git stash' branch  []
> @@ -106,7 +106,7 @@ stash@{1}: On master: 9cc0589... Add git-stash
>  The command takes options applicable to the 'git log'
>  command to control what is shown and how. See linkgit:git-log[1].
>  
> -show []::
> +show [] []::
>  
>   Show the changes recorded in the stash entry as a diff between the
>   stashed contents and the commit back when the stash entry was first
> diff --git a/builtin/stash--helper.c b/builtin/stash--helper.c
> index 1bc838ee6b..1f02f5f2e9 100644
> --- a/builtin/stash--helper.c
> +++ b/builtin/stash--helper.c
> @@ -15,7 +15,7 @@
>  
>  static const char * const git_stash_helper_usage[] = {
>   N_("git stash--helper list []"),
> - N_("git stash--helper show []"),
> + N_("git stash--helper show [] []"),
>   N_("git stash--helper drop [-q|--quiet] []"),
>   N_("git stash--helper ( pop | apply ) [--index] [-q|--quiet] 
> []"),
>   N_("git stash--helper branch  []"),
> @@ -29,7 +29,7 @@ static const char * const git_stash_helper_list_usage[] = {
>  };
>  
>  static const char * const git_stash_helper_show_usage[] = {
> - N_("git stash--helper show []"),
> + N_("git stash--helper show [] []"),
>   NULL
>  };
>  
> -- 
> 2.19.0.rc0.23.g1fb9f40d88
> 

Re: Git Evolve

[Stefan Xenos]

If you're curious how the Mercurial absorb command works, here's the code:

https://www.mercurial-scm.org/repo/hg/file/tip/hgext/absorb.py

It's GPL 2:

https://www.mercurial-scm.org/repo/hg/file/tip/COPYING



On Tue, Oct 2, 2018 at 2:11 AM, Ævar Arnfjörð Bjarmason
 wrote:
>
> On Tue, Oct 02 2018, Taylor Blau wrote:
>
>> Hi Stefan,
>>
>> On Sat, Sep 29, 2018 at 04:00:04PM -0700, Stefan Xenos wrote:
>>> Hello, List!
>>>
>>> I'm interested in porting something like Mercurial's evolve command to
>>> Git.
>>
>> Welcome to Git :-). I think that the discussion in this thread is good,
>> but it's not why I'm replying. I have also wanted a Mercurial feature in
>> Git, but a different one than yours.
>>
>> Specifically, I've wanted the 'hg absorb' command. My understanding of
>> the commands functionality is that it builds a sort of flamegraph-esque
>> view of the blame, and then cascades downwards parts of a change. I am
>> sure that I'm not doing the command justice, so I'll defer to [1] where
>> it is explained in more detail.
>>
>> The benefit of this command is that it gives you a way to--without
>> ambiguity--absorb changes into earlier commits, and in fact, the
>> earliest commit that they make sense to belong to.
>>
>> This would simplify my workflow greatly when re-rolling patches, as I
>> often want to rewrite a part of an earlier commit. This is certainly
>> possible by a number of different `git rebase` invocations (e.g., (1)
>> create fixup commits, and then re-order them, or (2) mark points in your
>> history as 'edit', and rewrite them in a detached state, and I'm sure
>> many more).
>>
>> I'm curious if you or anyone else has thought about how this might work
>> in Git.
>
> I've wanted a "git absorb" for a while, but have done no actual work on
> it, I just found out about it.
>
> I think a combination of these two heuristics would probably do the
> trick:
>
>  1. If a change in your "git diff" output has a hunk whose lines overlap
> with an earlier commit in the @{u}.. range, we do the equivalent of
> "git add -p", select that hunk, and "git commit --fixup  commit>". We fixup the most recent commit that matches (otherwise
> commit>we'd conflict).
>
>  2. Have some mode where we fall back from #1 and consider changes to
> entire files, if that's unambiguous.
>
> The neat thing about this would be that you could tweak how promiscuous
> #1 would be via the -U option to git-diff, and #2 would just be a
> special case of -U9 (we should really add a -Uinf...).
>
> Then once you ran this you could run "git rebase -i --autosquash" to see
> how the TODO list would look, and optionally have some "git absorb
> --now" or whatever to do the "git add -p", "git commit --fixup" and "git
> rebase --autosquash" all in one go.
>
>> [1]: http://files.lihdd.net/hgabsorb-note.pdf

Re: [PATCH 2/2] fsck: use oidset for skiplist

[Jeff King]

On Tue, Oct 02, 2018 at 09:05:32PM +0200, René Scharfe wrote:

> > The reason hashmap.c was added was to avoid open addressing. ;)
> Because efficient removal of elements is easier to implement with
> chaining, according to 6a364ced49 (add a hashtable implementation that
> supports O(1) removal).  khash.h deletes using its flags bitmap.  We
> didn't compare their performance when entries are removed so far.

I think it may depend on your workload. Open-addressing generally uses a
tombstone, so you're still dealing with the "deleted" entries until the
next table resize. I suspect that's fine in most cases, but I also am
sure you could find a benchmark that favors the chained approach (I
think in most cases we actually never delete at all -- we simply fill up
a table and then eventually clear it).

> > So yeah, I think it could perhaps be improved, but in my mind talking
> > about "hashmap.c" is fundamentally talking about chained buckets.
> 
> Admittedly I wouldn't touch hashmap.c, as I find its interface too
> complex to wrap my head around.  But perhaps I just didn't try hard
> enough, yet.

FWIW, it's not just you. ;)

> > Yeah. And if it really does perform better, I think we should stick with
> > it in the code base. I wonder if we could stand to clean up the
> > interfaces a little.  E.g., I had a hard time declaring a hash in one
> > place, and then defining it somewhere else.
> 
> You can't use KHASH_DECLARE and KHASH_INIT together, as both declare
> the same structs.  So I guess the idea is to have a header file with
> KHASH_DECLARE and a .c file with KHASH_INIT, the latter *not* including
> the former, but both including khash.h.  I didn't actually try that,
> though.

Yeah, that seems weird. You'd want to include one from the other to make
sure that they both match.

By the way, if you do want to pursue changes, I have no problem at all
hacking up khash into something that can't be merged with its upstream.
It's nice that it's a well-used and tested library, but I'd much rather
have something that we on this project understand (and that matches our
conventions and style).

> > This is kind of a layering violation, too. You're assuming that struct
> > assignment is sufficient to make one kh struct freeable from another
> > pointer. That's probably reasonable, since you're just destroying them
> > both (e.g., some of our FLEX structs point into their own struct memory,
> > making a hidden dependency; but they obviously would not need to free
> > such a field).
> 
> Fair enough.  How about this on top?  (The khash.h part would go in
> first in a separate patch in a proper series.)

Yes, much nicer, and the khash change wasn't too painful.

-Peff

[PATCH] coccicheck: process every source file at once

[Jacob Keller]

From: Jacob Keller 

make coccicheck is used in order to apply coccinelle semantic patches,
and see if any of the transformations found within contrib/coccinelle/
can be applied to the current code base.

Pass every file to a single invocation of spatch, instead of running
spatch once per source file.

This reduces the time required to run make coccicheck by a significant
amount of time:

Prior timing of make coccicheck
  real6m14.090s
  user25m2.606s
  sys 1m22.919s

New timing of make coccicheck
  real1m36.580s
  user7m55.933s
  sys 0m18.219s

This is nearly a 4x decrease in the time required to run make
coccicheck. This is due to the overhead of restarting spatch for every
file. By processing all files at once, we can amortize this startup cost
across the total number of files, rather than paying it once per file.

Signed-off-by: Jacob Keller 
---
 Makefile | 6 ++
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/Makefile b/Makefile
index df1df9db78da..b9947f3f51ec 100644
--- a/Makefile
+++ b/Makefile
@@ -2715,10 +2715,8 @@ endif
 %.cocci.patch: %.cocci $(COCCI_SOURCES)
@echo '' SPATCH $<; \
ret=0; \
-   for f in $(COCCI_SOURCES); do \
-   $(SPATCH) --sp-file $< $$f $(SPATCH_FLAGS) || \
-   { ret=$$?; break; }; \
-   done >$@+ 2>$@.log; \
+   ( $(SPATCH) --sp-file $< $(COCCI_SOURCES) $(SPATCH_FLAGS) || \
+   { ret=$$?; }; ) >$@+ 2>$@.log; \
if test $$ret != 0; \
then \
cat $@.log; \
-- 
2.18.0.219.gaf81d287a9da

Re: [PATCH 5/5] diff --color-moved: fix a memory leak

[Stefan Beller]

On Tue, Oct 2, 2018 at 10:55 AM Phillip Wood  wrote:
>
> From: Phillip Wood 
>
> Free the hashmap items as well as the hashmap itself. This was found
> with asan.
>
> Signed-off-by: Phillip Wood 
> ---
>  diff.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/diff.c b/diff.c
> index 4464feacf8..94cc1b5592 100644
> --- a/diff.c
> +++ b/diff.c
> @@ -5770,8 +5770,8 @@ static void diff_flush_patch_all_file_pairs(struct 
> diff_options *o)
> if (o->color_moved == COLOR_MOVED_ZEBRA_DIM)
> dim_moved_lines(o);
>
> -   hashmap_free(_lines, 0);
> -   hashmap_free(_lines, 0);
> +   hashmap_free(_lines, 1);
> +   hashmap_free(_lines, 1);

And this is sufficient as the entries themselves are
moved_entry, which only contain pointers (to
o->emitted_symbols), which are freed later.
There is nothing in there that needs extra free-ing.

Thanks,
Stefan

Re: [PATCH v7 7/7] read-cache: load cache entries on worker threads

[Ben Peart]

On 10/1/2018 1:09 PM, Duy Nguyen wrote:

On Mon, Oct 1, 2018 at 3:46 PM Ben Peart  wrote:

+/*
+ * A helper function that will load the specified range of cache entries
+ * from the memory mapped file and add them to the given index.
+ */
+static unsigned long load_cache_entry_block(struct index_state *istate,
+   struct mem_pool *ce_mem_pool, int offset, int nr, const 
char *mmap,


Please use unsigned long for offset (here and in the thread_data
struct). We should use off_t instead, but that's out of scope. At
least keep offset type consistent in here.



Unfortunately, this code is littered with different types for size and 
offset.  "int" is the most common but there are also off_t, size_t and 
some unsigned long as well.  Currently all of them are at least 32 bits 
so until we need to have an index larger than 32 bits, we should be OK. 
I agree, fixing them all is outside the scope of this patch.



+   unsigned long start_offset, const struct cache_entry 
*previous_ce)


I don't think you want to pass previous_ce in. You always pass NULL
anyway. And if this function is about loading a block (i.e. at block
boundary) then initial previous_ce _must_ be NULL or things break
horribly.



The function as written can load any arbitrary subset of cache entries 
as long as previous_ce is set correctly.  I currently only use it on 
block boundaries but I don't see any good reason to limit its 
capabilities by moving what code passes the NULL in one function deeper.



@@ -1959,20 +2007,125 @@ static void *load_index_extensions(void *_data)

  #define THREAD_COST(1)

+struct load_cache_entries_thread_data
+{
+   pthread_t pthread;
+   struct index_state *istate;
+   struct mem_pool *ce_mem_pool;
+   int offset;
+   const char *mmap;
+   struct index_entry_offset_table *ieot;
+   int ieot_offset;/* starting index into the ieot array */


If it's an index, maybe just name it ieot_index and we can get rid of
the comment.


+   int ieot_work;  /* count of ieot entries to process */


Maybe instead of saving the whole "ieot" table here. Add

  struct index_entry_offset *blocks;

which points to the starting block for this thread and rename that
mysterious (to me) ieot_work to nr_blocks. The thread will have access
from blocks[0] to blocks[nr_blocks - 1]



Meh. Either way you have to figure out there are a block of entries and 
each thread is going to process some subset of those entries.  You can 
do the base + offset math here or down in the calling function but it 
has to happen (and be understood) either way.


I'll rename ieot_offset to ieot_start and ieot_work to ieot_blocks which 
should hopefully help make it more obvious what they do.



+   unsigned long consumed; /* return # of bytes in index file processed */
+};
+
+/*
+ * A thread proc to run the load_cache_entries() computation
+ * across multiple background threads.
+ */
+static void *load_cache_entries_thread(void *_data)
+{
+   struct load_cache_entries_thread_data *p = _data;
+   int i;
+
+   /* iterate across all ieot blocks assigned to this thread */
+   for (i = p->ieot_offset; i < p->ieot_offset + p->ieot_work; i++) {
+   p->consumed += load_cache_entry_block(p->istate, p->ce_mem_pool, p->offset, 
p->ieot->entries[i].nr, p->mmap, p->ieot->entries[i].offset, NULL);


Please wrap this long line.


+   p->offset += p->ieot->entries[i].nr;
+   }
+   return NULL;
+}
+
+static unsigned long load_cache_entries_threaded(struct index_state *istate, 
const char *mmap, size_t mmap_size,
+   unsigned long src_offset, int nr_threads, struct 
index_entry_offset_table *ieot)
+{
+   int i, offset, ieot_work, ieot_offset, err;
+   struct load_cache_entries_thread_data *data;
+   unsigned long consumed = 0;
+   int nr;
+
+   /* a little sanity checking */
+   if (istate->name_hash_initialized)
+   BUG("the name hash isn't thread safe");
+
+   mem_pool_init(>ce_mem_pool, 0);
+   data = xcalloc(nr_threads, sizeof(struct 
load_cache_entries_thread_data));


we normally use sizeof(*data) instead of sizeof(struct ...)


+
+   /* ensure we have no more threads than we have blocks to process */
+   if (nr_threads > ieot->nr)
+   nr_threads = ieot->nr;
+   data = xcalloc(nr_threads, sizeof(struct 
load_cache_entries_thread_data));


eh.. reallocate the same "data"?



Thanks, good catch - I hate leaky code.


+
+   offset = ieot_offset = 0;
+   ieot_work = DIV_ROUND_UP(ieot->nr, nr_threads);
+   for (i = 0; i < nr_threads; i++) {
+   struct load_cache_entries_thread_data *p = [i];
+   int j;
+
+   if (ieot_offset + ieot_work > ieot->nr)
+   ieot_work = ieot->nr - ieot_offset;
+
+   p->istate = istate;
+   p->offset = offset;
+  

Re: [PATCH 4/5] diff --color-moved-ws: fix another memory leak

[Stefan Beller]

On Tue, Oct 2, 2018 at 10:55 AM Phillip Wood  wrote:
>
> From: Phillip Wood 
>
> This is obvious in retrospect, it was found with asan.

Indeed. :/

Thanks,
Stefan

Re: [PATCH 1/2] commit-graph: clean up leaked memory during write

[Martin Ågren]

On Tue, 2 Oct 2018 at 19:59, Stefan Beller  wrote:
> > > +
> > > +   string_list_clear(, 0);
> > >  }
> >
> > Nit: The blank line adds some asymmetry, IMVHO.
>
> I think these blank lines are super common, as in:
>
> {
>   declarations;
>
>   multiple;
>   lines(of);
>   code;
>
>   cleanup;
>   and_frees;
> }
>
> (c.f. display_table in column.c, which I admit to have
> cherry-picked as an example).
>
> While in nit territory, I would rather move the string list init
> into the first block:
>
>   {
> struct string_list list = STRING_LIST_INIT_DUP;
>
> for_each_ref(add_ref_to_list, );
> write_commit_graph(obj_dir, NULL, , append);
>
> string_list_clear(, 0);
>   }

Now this looks very symmetrical. :-)

> > >  void write_commit_graph(const char *obj_dir,
> > > @@ -846,9 +848,11 @@ void write_commit_graph(const char *obj_dir,
> > > compute_generation_numbers(, report_progress);
> > >
> > > graph_name = get_commit_graph_filename(obj_dir);
> > > -   if (safe_create_leading_directories(graph_name))
> > > +   if (safe_create_leading_directories(graph_name)) {
> > > +   UNLEAK(graph_name);
> > > die_errno(_("unable to create leading directories of %s"),
> > >   graph_name);
> > > +   }
> >
> > Do you really need this hunk?
>
> graph_name is produced via xstrfmt in get_commit_graph_filename,
> so it needs to be free'd in any return/exit path.

Agreed. Although I am questioning that `die()` and its siblings count.

> > In my testing with LeakSanitizer and
> > valgrind, I don't need this hunk to be leak-free.
>
>
> > Generally speaking, it
> > seems impossible to UNLEAK when dying, since we don't know what we have
> > allocated higher up in the call-stack.
>
> I do not understand; I thought UNLEAK was specifically for the purpose of
> die() calls without imposing extra overhead; rereading 0e5bba53af
> (add UNLEAK annotation for reducing leak false positives, 2017-09-08)
> doesn't provide an example for prematurely die()ing, only for regular
> program exit.
>
> > [...] With this hunk, I am
> > puzzled and feel uneasy, both about having to UNLEAK before dying and
> > about having to UNLEAK outside of builtin/.
>
> I am not uneasy about an UNLEAK before dying, but about dying outside
> builtin/ in general

Yeah, not dying would be even better (out of scope for this patch).

> (but having a die call accompanied by UNLEAK seems
> to be the right thing). Can you explain the worries you have regarding the
> allocations on the call stack, as xstrfmt is allocating on the heap and we
> only UNLEAK the pointer to that?

I think we agree that leaking things "allocat[ed] on the call stack"
isn't much of a worry. The reason I mentioned the call stack is that
we've got any number of calls behind us on it, and we might have made
all sorts of allocations on the heap, and at this point, we have no
idea about what we should be UNLEAK-ing.

My worry is that one of these would seem to be true:

* UNLEAK is unsuitable for the job. Whenever we have a `die()` as we do
  here, we can UNLEAK the variables we know of, but we can't do anything
  about the allocations we have made higher up the call-chain. Our test
  suite obviously provokes lots of calls to `die()` -- imagine that each
  of those leaves a few leaked allocations behind. We'd have a semi-huge
  number of leaks being reported. While we could mark with UNLEAK to
  reduce that number, we wouldn't be able to bring the number of leaks
  down to anywhere near manageable where we'd be able to find the last
  few true positives.

* We add code with no purpose. In this case, we're not talking a lot of
  lines, but across the code base, if they bring no gain, they are bound
  to provide a negative net value given enough time.

Martin

Re: [PATCH 3/5] diff --color-moved-ws: fix a memory leak

[Stefan Beller]

On Tue, Oct 2, 2018 at 10:55 AM Phillip Wood  wrote:
>
> From: Phillip Wood 
>
> Don't duplicate the indentation string if we're not going to use it.
> This was found with asan.

Makes sense,

Thanks,
Stefan

With compute_ws_delta growing bigger here (and having only one caller),
I wonder if we want to pass 'match' instead of match->es (and pmb_nr),
such that the function can also take care of
pmb[pmb_nr++].match = match;

Then the function is not a mere compute_ws_delta, but a whole
setup_new_pmb(...) thing. Undecided.

Re: [PATCH 2/2] fsck: use oidset for skiplist

[René Scharfe]

Am 01.10.2018 um 22:26 schrieb Jeff King:
> On Mon, Oct 01, 2018 at 09:15:53PM +0200, René Scharfe wrote:
> The reason hashmap.c was added was to avoid open addressing. ;)
Because efficient removal of elements is easier to implement with
chaining, according to 6a364ced49 (add a hashtable implementation that
supports O(1) removal).  khash.h deletes using its flags bitmap.  We
didn't compare their performance when entries are removed so far.

> So yeah, I think it could perhaps be improved, but in my mind talking
> about "hashmap.c" is fundamentally talking about chained buckets.

Admittedly I wouldn't touch hashmap.c, as I find its interface too
complex to wrap my head around.  But perhaps I just didn't try hard
enough, yet.

>> But I like how khash.h is both already in the tree and also really easy
>> to deploy, as it's just a single header file.  It's a tasty low-hanging
>> fruit.
> 
> Yeah. And if it really does perform better, I think we should stick with
> it in the code base. I wonder if we could stand to clean up the
> interfaces a little.  E.g., I had a hard time declaring a hash in one
> place, and then defining it somewhere else.

You can't use KHASH_DECLARE and KHASH_INIT together, as both declare
the same structs.  So I guess the idea is to have a header file with
KHASH_DECLARE and a .c file with KHASH_INIT, the latter *not* including
the former, but both including khash.h.  I didn't actually try that,
though.

> And I think as you found
> that it insists on heap-allocating the hash-table struct itself, which
> does not match our usual style.

Perhaps we can fix that with little effort (see below).

>> This is straight-forward, except for oidset_clear(), which needs to
>> allocate a kh_oid_t on the heap in order to be able to feed it to
>> kh_destroy_oid() for release it.  Alternatively we could open-code the
>> relevant parts of the latter, but that would be a layering violation.
> 
> This is kind of a layering violation, too. You're assuming that struct
> assignment is sufficient to make one kh struct freeable from another
> pointer. That's probably reasonable, since you're just destroying them
> both (e.g., some of our FLEX structs point into their own struct memory,
> making a hidden dependency; but they obviously would not need to free
> such a field).

Fair enough.  How about this on top?  (The khash.h part would go in
first in a separate patch in a proper series.)

NB: I stuck to the 4-spaces-tabs formatting in khash.h here.

---
 khash.h  | 9 +++--
 oidset.c | 4 +---
 2 files changed, 8 insertions(+), 5 deletions(-)

diff --git a/khash.h b/khash.h
index 07b4cc2e67..d10caa0c35 100644
--- a/khash.h
+++ b/khash.h
@@ -82,11 +82,16 @@ static const double __ac_HASH_UPPER = 0.77;
SCOPE kh_##name##_t *kh_init_##name(void) { 
\
return (kh_##name##_t*)xcalloc(1, sizeof(kh_##name##_t));   
\
}   
\
+   SCOPE void kh_release_##name(kh_##name##_t *h)  
\
+   {   
\
+   free(h->flags); 
\
+   free((void *)h->keys);  
\
+   free((void *)h->vals);  
\
+   }   
\
SCOPE void kh_destroy_##name(kh_##name##_t *h)  
\
{   
\
if (h) {
\
-   free((void *)h->keys); free(h->flags);  
\
-   free((void *)h->vals);  
\
+   kh_release_##name(h);   
\
free(h);
\
}   
\
}   
  

Re: [PATCH 2/5] diff --color-moved-ws: fix out of bounds string access

[Stefan Beller]

On Tue, Oct 2, 2018 at 10:55 AM Phillip Wood  wrote:
>
> From: Phillip Wood 
>
> When adjusting the start of the string to take account of the change
> in indentation the code was not checking that the string being
> adjusted was in fact longer than the indentation change. This was
> detected by asan.
>
> Signed-off-by: Phillip Wood 
> ---
>  diff.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/diff.c b/diff.c
> index 5a08d64497..0096bdc339 100644
> --- a/diff.c
> +++ b/diff.c
> @@ -841,7 +841,7 @@ static int cmp_in_block_with_wsd(const struct 
> diff_options *o,
> al -= wslen;
> }
>
> -   if (strcmp(a, c))
> +   if (al < 0 || al != cl || memcmp(a, c, al))

If (al < 0) then al != cl, so I would think we could drop the first
part, and only check with al != cl || memcmp

In theory we should use xdiff_compare_lines here
as the rest of the lines could have more white space issues,
but we catch that earlier via a die("color-moved-ws:
allow-indentation-change cannot be combined with other
white space modes"), so memcmp is fine.

Side note: There are comments above this code that seem
to be also obsolete after patch 1 ("...carried forward in
pmb->wsd; ...)

Re: [PATCH 1/5] diff --color-moved-ws: fix double free crash

[Stefan Beller]

On Tue, Oct 2, 2018 at 10:55 AM Phillip Wood  wrote:

> The solution is to store the ws_delta in the array of potential moved
> blocks rather than with the lines. This means that it no longer needs
> to be copied around and one block cannot overwrite the ws_delta of
> another. Additionally it saves some malloc/free calls as we don't keep
> allocating and freeing ws_deltas.

Another solution would be to duplicate the copy-arounds, that it only
fixes the double free, but having another layer of abstraction
(moved block vs line) makes sense as then we don't need to copy
it forward.

With this patch applied the diff as mentioned works and having the
ws deltas with the blocks instead of the

>
> Signed-off-by: Phillip Wood 



>  static void pmb_advance_or_null_multi_match(struct diff_options *o,
[...]
> for (i = 0; i < pmb_nr; i++) {
> if (got_match[i]) {
> /* Carry the white space delta forward */

I would think this comment is obsolete as well with this patch?

With or without that nit addressed, this patch is
Reviewed-by: Stefan Beller 

Re: git fetch behaves weirdely when run in a worktree

[Kaartic Sivaraam]

Hi,

Sorry for the delay. Got a little busy over the weekend. I seem to have
found the reason behind the issue in the mean time :-)


On Wed, 2018-09-26 at 10:05 -0700, Junio C Hamano wrote:
> Duy Nguyen  writes:
> 
> > On Wed, Sep 26, 2018 at 05:24:14PM +0200, Duy Nguyen wrote:
> > > On Wed, Sep 26, 2018 at 6:46 AM Kaartic Sivaraam
> > >  wrote:
> > > > This is the most interesting part of the issue. I **did not** run
> > > > 'git fetch ...' in between those cat commands. I was surprised by
> > > > how the contents of FETCH_HEAD are changing without me spawning any
> > > > git processes that might change it. Am I missing something here? As
> > > > far as i could remember, there wasn't any IDE running that might
> > > > automatically spawn git commands especially in that work
> > > > tree. Weird.
> > 
> > Maybe something like this could help track down that rogue "git fetch"
> > process (it's definitely _some_ process writing to the wrong file; or
> > some file synchronization thingy is going on). You can log more of
> > course, but this is where FETCH_HEAD is updated.
> 

Thanks for the patch! It really helped me identify the issue.

The actual culprit here doesn't seem to be Git itself. It was the "git-
prompt" bash plug-in[1] I was using. It seems to be spawning "git
fetch" for every command I type on shell. That answers why the
FETCH_HEAD was being updated even though I wasn't explicitly running
it. The git bash plug-in seems to be fetching changes for *all* the
upstream branches. That answers why there FETCH_HEAD was populated with
info about all the branches when I explicitly requested for the next
branch. I've also verified that `git fetch origin next` works as
intended (FETCH_HEAD has info only about orgin/next) when I turn-off
the plug-in which confirms that it's the culprit. A cursory search
found me a related issue[2] but I'm not sure if it's the exact same
one.

I could identify the culprit only with the help of Duy's patch. Thanks
Duy! Sorry for not realising this earlier. I almost forgot I'm using it
as I've been accustomed to it a lot.


> Well, a background-ish thing could be some vendor-provided copy of
> Git that has nothing to do with what Kaartic would be compiling with
> this patch X-<.

Fortunately, it wasn't the case here as the plug-in was using my
manually-built version of Git :-)

Thanks for the help!

Tag-line: Sometimes tools become part of our workflow so much that we
really don't realise their presence. It's an indication of a good tool
but we should be aware of suspecting them when an issue arises!
Something which I should have done to realise the issue ealier x-<


References:
[1]: https://github.com/magicmonty/bash-git-prompt
[2]: https://github.com/magicmonty/bash-git-prompt/issues/125

Thanks,
Sivaraam

Re: [PATCH 1/2] commit-graph: clean up leaked memory during write

[Stefan Beller]

On Tue, Oct 2, 2018 at 8:40 AM Martin Ågren  wrote:
>
> On Tue, 2 Oct 2018 at 17:01, Derrick Stolee via GitGitGadget
>  wrote:
> > diff --git a/commit-graph.c b/commit-graph.c
> > index 2a24eb8b5a..7226bd6b58 100644
> > --- a/commit-graph.c
> > +++ b/commit-graph.c
> > @@ -698,6 +698,8 @@ void write_commit_graph_reachable(const char *obj_dir, 
> > int append,
> > string_list_init(, 1);
> > for_each_ref(add_ref_to_list, );
> > write_commit_graph(obj_dir, NULL, , append, report_progress);
> > +
> > +   string_list_clear(, 0);
> >  }
>
> Nit: The blank line adds some asymmetry, IMVHO.

I think these blank lines are super common, as in:

{
  declarations;

  multiple;
  lines(of);
  code;

  cleanup;
  and_frees;
}

(c.f. display_table in column.c, which I admit to have
cherry-picked as an example).

While in nit territory, I would rather move the string list init
into the first block:

  {
struct string_list list = STRING_LIST_INIT_DUP;

for_each_ref(add_ref_to_list, );
write_commit_graph(obj_dir, NULL, , append);

string_list_clear(, 0);
  }




>
> >  void write_commit_graph(const char *obj_dir,
> > @@ -846,9 +848,11 @@ void write_commit_graph(const char *obj_dir,
> > compute_generation_numbers(, report_progress);
> >
> > graph_name = get_commit_graph_filename(obj_dir);
> > -   if (safe_create_leading_directories(graph_name))
> > +   if (safe_create_leading_directories(graph_name)) {
> > +   UNLEAK(graph_name);
> > die_errno(_("unable to create leading directories of %s"),
> >   graph_name);
> > +   }
>
> Do you really need this hunk?

graph_name is produced via xstrfmt in get_commit_graph_filename,
so it needs to be free'd in any return/exit path.

> In my testing with LeakSanitizer and
> valgrind, I don't need this hunk to be leak-free.


> Generally speaking, it
> seems impossible to UNLEAK when dying, since we don't know what we have
> allocated higher up in the call-stack.

I do not understand; I thought UNLEAK was specifically for the purpose of
die() calls without imposing extra overhead; rereading 0e5bba53af
(add UNLEAK annotation for reducing leak false positives, 2017-09-08)
doesn't provide an example for prematurely die()ing, only for regular
program exit.

> Reviewed-by: Martin Ågren 
>
> as I've verified the leaks before and after. With this hunk, I am
> puzzled and feel uneasy, both about having to UNLEAK before dying and
> about having to UNLEAK outside of builtin/.

I am not uneasy about an UNLEAK before dying, but about dying outside
builtin/ in general (but having a die call accompanied by UNLEAK seems
to be the right thing). Can you explain the worries you have regarding the
allocations on the call stack, as xstrfmt is allocating on the heap and we
only UNLEAK the pointer to that?

Stefan

[PATCH 5/5] diff --color-moved: fix a memory leak

[Phillip Wood]

From: Phillip Wood 

Free the hashmap items as well as the hashmap itself. This was found
with asan.

Signed-off-by: Phillip Wood 
---
 diff.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/diff.c b/diff.c
index 4464feacf8..94cc1b5592 100644
--- a/diff.c
+++ b/diff.c
@@ -5770,8 +5770,8 @@ static void diff_flush_patch_all_file_pairs(struct 
diff_options *o)
if (o->color_moved == COLOR_MOVED_ZEBRA_DIM)
dim_moved_lines(o);
 
-   hashmap_free(_lines, 0);
-   hashmap_free(_lines, 0);
+   hashmap_free(_lines, 1);
+   hashmap_free(_lines, 1);
}
 
for (i = 0; i < esm.nr; i++)
-- 
2.19.0

[PATCH 1/5] diff --color-moved-ws: fix double free crash

[Phillip Wood]

From: Phillip Wood 

Running
  git diff --color-moved-ws=allow-indentation-change v2.18.0 v2.19.0
results in a crash due to a double free. This happens when two
potential moved blocks start with consecutive lines. As
pmb_advance_or_null_multi_match() advances it copies the ws_delta from
the last matching line to the next. When the first of our consecutive
lines is advanced its ws_delta well be copied to the second,
overwriting the ws_delta of the block containing the second line. Then
when the second line is advanced it will copy the new ws_delta to the
line below it and so on. Eventually one of these blocks will stop
matching and the ws_delta will be freed. From then on the other block
is in a use-after-free state and when it stops matching it will try to
free the ws_delta that has already been freed by the other block.

The solution is to store the ws_delta in the array of potential moved
blocks rather than with the lines. This means that it no longer needs
to be copied around and one block cannot overwrite the ws_delta of
another. Additionally it saves some malloc/free calls as we don't keep
allocating and freeing ws_deltas.

Signed-off-by: Phillip Wood 
---
 diff.c | 73 +-
 1 file changed, 41 insertions(+), 32 deletions(-)

diff --git a/diff.c b/diff.c
index 9393993e33..5a08d64497 100644
--- a/diff.c
+++ b/diff.c
@@ -751,7 +751,6 @@ struct moved_entry {
struct hashmap_entry ent;
const struct emitted_diff_symbol *es;
struct moved_entry *next_line;
-   struct ws_delta *wsd;
 };
 
 /**
@@ -768,6 +767,17 @@ struct ws_delta {
 };
 #define WS_DELTA_INIT { NULL, 0 }
 
+struct moved_block {
+   struct moved_entry *match;
+   struct ws_delta wsd;
+};
+
+static void moved_block_clear(struct moved_block *b)
+{
+   FREE_AND_NULL(b->wsd.string);
+   b->match = NULL;
+}
+
 static int compute_ws_delta(const struct emitted_diff_symbol *a,
 const struct emitted_diff_symbol *b,
 struct ws_delta *out)
@@ -785,7 +795,7 @@ static int compute_ws_delta(const struct 
emitted_diff_symbol *a,
 static int cmp_in_block_with_wsd(const struct diff_options *o,
 const struct moved_entry *cur,
 const struct moved_entry *match,
-struct moved_entry *pmb,
+struct moved_block *pmb,
 int n)
 {
struct emitted_diff_symbol *l = >emitted_symbols->buf[n];
@@ -805,7 +815,7 @@ static int cmp_in_block_with_wsd(const struct diff_options 
*o,
if (strcmp(a, b))
return 1;
 
-   if (!pmb->wsd)
+   if (!pmb->wsd.string)
/*
 * No white space delta was carried forward? This can happen
 * when we exit early in this function and do not carry
@@ -822,8 +832,8 @@ static int cmp_in_block_with_wsd(const struct diff_options 
*o,
 * one of them for the white spaces, depending which was longer.
 */
 
-   wslen = strlen(pmb->wsd->string);
-   if (pmb->wsd->current_longer) {
+   wslen = strlen(pmb->wsd.string);
+   if (pmb->wsd.current_longer) {
c += wslen;
cl -= wslen;
} else {
@@ -873,7 +883,6 @@ static struct moved_entry *prepare_entry(struct 
diff_options *o,
ret->ent.hash = xdiff_hash_string(l->line, l->len, flags);
ret->es = l;
ret->next_line = NULL;
-   ret->wsd = NULL;
 
return ret;
 }
@@ -913,76 +922,72 @@ static void add_lines_to_move_detection(struct 
diff_options *o,
 static void pmb_advance_or_null(struct diff_options *o,
struct moved_entry *match,
struct hashmap *hm,
-   struct moved_entry **pmb,
+   struct moved_block *pmb,
int pmb_nr)
 {
int i;
for (i = 0; i < pmb_nr; i++) {
-   struct moved_entry *prev = pmb[i];
+   struct moved_entry *prev = pmb[i].match;
struct moved_entry *cur = (prev && prev->next_line) ?
prev->next_line : NULL;
if (cur && !hm->cmpfn(o, cur, match, NULL)) {
-   pmb[i] = cur;
+   pmb[i].match = cur;
} else {
-   pmb[i] = NULL;
+   pmb[i].match = NULL;
}
}
 }
 
 static void pmb_advance_or_null_multi_match(struct diff_options *o,
struct moved_entry *match,
struct hashmap *hm,
-   struct moved_entry **pmb,
+   struct moved_block *pmb,
int pmb_nr, int n)

[PATCH 4/5] diff --color-moved-ws: fix another memory leak

[Phillip Wood]

From: Phillip Wood 

This is obvious in retrospect, it was found with asan.

Signed-off-by: Phillip Wood 
---
 diff.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/diff.c b/diff.c
index efadd05c90..4464feacf8 100644
--- a/diff.c
+++ b/diff.c
@@ -971,6 +971,8 @@ static void pmb_advance_or_null_multi_match(struct 
diff_options *o,
moved_block_clear([i]);
}
}
+
+   free(got_match);
 }
 
 static int shrink_potential_moved_blocks(struct moved_block *pmb,
-- 
2.19.0

[PATCH 3/5] diff --color-moved-ws: fix a memory leak

[Phillip Wood]

From: Phillip Wood 

Don't duplicate the indentation string if we're not going to use it.
This was found with asan.

Signed-off-by: Phillip Wood 
---
 diff.c | 6 +-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/diff.c b/diff.c
index 0096bdc339..efadd05c90 100644
--- a/diff.c
+++ b/diff.c
@@ -785,11 +785,15 @@ static int compute_ws_delta(const struct 
emitted_diff_symbol *a,
const struct emitted_diff_symbol *longer =  a->len > b->len ? a : b;
const struct emitted_diff_symbol *shorter = a->len > b->len ? b : a;
int d = longer->len - shorter->len;
+   int ret = !strncmp(longer->line + d, shorter->line, shorter->len);
+
+   if (!ret)
+   return ret;
 
out->string = xmemdupz(longer->line, d);
out->current_longer = (a == longer);
 
-   return !strncmp(longer->line + d, shorter->line, shorter->len);
+   return ret;
 }
 
 static int cmp_in_block_with_wsd(const struct diff_options *o,
-- 
2.19.0

[PATCH 2/5] diff --color-moved-ws: fix out of bounds string access

[Phillip Wood]

From: Phillip Wood 

When adjusting the start of the string to take account of the change
in indentation the code was not checking that the string being
adjusted was in fact longer than the indentation change. This was
detected by asan.

Signed-off-by: Phillip Wood 
---
 diff.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/diff.c b/diff.c
index 5a08d64497..0096bdc339 100644
--- a/diff.c
+++ b/diff.c
@@ -841,7 +841,7 @@ static int cmp_in_block_with_wsd(const struct diff_options 
*o,
al -= wslen;
}
 
-   if (strcmp(a, c))
+   if (al < 0 || al != cl || memcmp(a, c, al))
return 1;
 
return 0;
-- 
2.19.0

Re: [PATCH v7 6/7] ieot: add Index Entry Offset Table (IEOT) extension

[Duy Nguyen]

On Tue, Oct 2, 2018 at 6:34 PM Ben Peart  wrote:
> >> +   offset = lseek(newfd, 0, SEEK_CUR) + 
> >> write_buffer_len;
> >
> > This only works correctly if the ce_write_entry() from the last
> > iteration has flushed everything to out to newfd. Maybe it does, but
> > it's error prone to rely on that in my opinion. Maybe we need an
> > explicit ce_write_flush() here to make sure.
> >
>
> This logic already takes any unflushed data into account - the offset is
> what has been flushed to disk (lseek) plus the amount still in the
> buffer (write_buffer_len) waiting to be flushed.  I don't see any need
> to force an additional flush and adding one could have a negative impact
> on performance.

Eck! How did I miss that write_buffer_len :P
-- 
Duy

Re: [PATCH v7 6/7] ieot: add Index Entry Offset Table (IEOT) extension

[Ben Peart]

On 10/1/2018 12:27 PM, Duy Nguyen wrote:

On Mon, Oct 1, 2018 at 3:46 PM Ben Peart  wrote:

@@ -1888,6 +1890,23 @@ static size_t estimate_cache_size(size_t ondisk_size, 
unsigned int entries)
 return ondisk_size + entries * per_entry;
  }

+struct index_entry_offset
+{
+   /* starting byte offset into index file, count of index entries in this 
block */
+   int offset, nr;


uint32_t?


+};
+
+struct index_entry_offset_table
+{
+   int nr;
+   struct index_entry_offset entries[0];


Use FLEX_ARRAY. Some compilers are not happy with an array of zero
items if I remember correctly.



Thanks for the warning, I'll update that.


@@ -2523,6 +2551,9 @@ static int do_write_index(struct index_state *istate, 
struct tempfile *tempfile,
 struct strbuf previous_name_buf = STRBUF_INIT, *previous_name;
 int drop_cache_tree = istate->drop_cache_tree;
 off_t offset;
+   int ieot_work = 1;
+   struct index_entry_offset_table *ieot = NULL;
+   int nr;


There are a bunch of stuff going on in this function, maybe rename
this to nr_threads or nr_blocks to be less generic.



I can add a nr_threads variable to make this more obvious.



 for (i = removed = extended = 0; i < entries; i++) {
 if (cache[i]->ce_flags & CE_REMOVE)
@@ -2556,7 +2587,38 @@ static int do_write_index(struct index_state *istate, 
struct tempfile *tempfile,
 if (ce_write(, newfd, , sizeof(hdr)) < 0)
 return -1;

+#ifndef NO_PTHREADS
+   if ((nr = git_config_get_index_threads()) != 1) {


Maybe keep this assignment out of "if".


+   int ieot_blocks, cpus;
+
+   /*
+* ensure default number of ieot blocks maps evenly to the
+* default number of threads that will process them
+*/
+   if (!nr) {
+   ieot_blocks = istate->cache_nr / THREAD_COST;
+   cpus = online_cpus();
+   if (ieot_blocks > cpus - 1)
+   ieot_blocks = cpus - 1;


The " - 1" here is for extension thread, yes? Probably worth a comment.


+   } else {
+   ieot_blocks = nr;
+   }
+
+   /*
+* no reason to write out the IEOT extension if we don't
+* have enough blocks to utilize multi-threading
+*/
+   if (ieot_blocks > 1) {
+   ieot = xcalloc(1, sizeof(struct 
index_entry_offset_table)
+   + (ieot_blocks * sizeof(struct 
index_entry_offset)));


Use FLEX_ALLOC_MEM() after you declare ..._table with FLEX_ARRAY.



FLEX_ALLOC_MEM() is focused on variable length "char" data.  All uses of 
FLEX_ARRAY with non char data did the allocation themselves to avoid the 
unnecessary memcpy() that comes with FLEX_ALLOC_MEM.



This ieot needs to be freed also and should be before any "return -1"
in this function.



Good catch. Will do.


+   ieot->nr = 0;
+   ieot_work = DIV_ROUND_UP(entries, ieot_blocks);


Perhaps a better name for ioet_work? This looks like the number of
cache entries per block.


+   }
+   }
+#endif
+
 offset = lseek(newfd, 0, SEEK_CUR) + write_buffer_len;
+   nr = 0;


Eh.. repurpose nr to count cache entries now? It's kinda hard to follow.


 previous_name = (hdr_version == 4) ? _name_buf : NULL;

 for (i = 0; i < entries; i++) {
@@ -2578,11 +2640,31 @@ static int do_write_index(struct index_state *istate, 
struct tempfile *tempfile,

 drop_cache_tree = 1;
 }
+   if (ieot && i && (i % ieot_work == 0)) {
+   ieot->entries[ieot->nr].nr = nr;
+   ieot->entries[ieot->nr].offset = offset;
+   ieot->nr++;
+   /*
+* If we have a V4 index, set the first byte to an 
invalid
+* character to ensure there is nothing common with the 
previous
+* entry
+*/
+   if (previous_name)
+   previous_name->buf[0] = 0;
+   nr = 0;
+   offset = lseek(newfd, 0, SEEK_CUR) + write_buffer_len;


This only works correctly if the ce_write_entry() from the last
iteration has flushed everything to out to newfd. Maybe it does, but
it's error prone to rely on that in my opinion. Maybe we need an
explicit ce_write_flush() here to make sure.



This logic already takes any unflushed data into account - the offset is 
what has been flushed to disk (lseek) plus the amount still in the 
buffer (write_buffer_len) waiting to be flushed.  I don't see any need 
to force an additional flush and adding one could have a negative impact 
on performance.



+   }
 if 

Re: [PATCH v2 8/8] reflog expire: cover reflog from all worktrees

[Duy Nguyen]

On Sun, Sep 30, 2018 at 7:36 AM Eric Sunshine  wrote:
>
> On Sat, Sep 29, 2018 at 3:11 PM Nguyễn Thái Ngọc Duy  
> wrote:
> > Signed-off-by: Nguyễn Thái Ngọc Duy 
> > ---
> > diff --git a/Documentation/git-reflog.txt b/Documentation/git-reflog.txt
> > @@ -72,6 +72,11 @@ Options for `expire`
> > +--single-worktree::
> > +   By default when `--all` is specified, reflogs from all working
> > +   trees are processed. This option limits the processing to reflogs
> > +   from the current working tree only.
>
> Bikeshedding: I wonder if this should be named "--this-worktree" or
> "--this-worktree-only" or if it should somehow be orthogonal to --all
> rather than modifying it. (Genuine questions. I don't have the
> answers.)

It follows a precedent (made by me :p) which is rev-list
--single-worktree. I doubt that option is widely used though so we
could still rename it if there's a better name. I made
--single-worktree to contrast "all worktrees" by default. Even if it's
"this/current worktree" it still has to somehow say "everything in
this worktree" so I felt modifying --all was a good idea.

> > diff --git a/builtin/reflog.c b/builtin/reflog.c
> > @@ -577,10 +585,18 @@ static int cmd_reflog_expire(int argc, const char 
> > **argv, const char *prefix)
> > if (do_all) {
> > struct collect_reflog_cb collected;
> > +   struct worktree **worktrees, **p;
> > int i;
> >
> > memset(, 0, sizeof(collected));
> > -   for_each_reflog(collect_reflog, );
> > +   worktrees = get_worktrees(0);
> > +   for (p = worktrees; *p; p++) {
> > +   if (!all_worktrees && !(*p)->is_current)
> > +   continue;
> > +   collected.wt = *p;
> > +   for_each_reflog(collect_reflog, );
> > +   }
> > +   free_worktrees(worktrees);
>
> Should this have a test in the test suite?

Of course. I was partly lazy/tired near the end, and anticipated more
comments anyway so I did not do it :D
-- 
Duy

[PATCH v3 1/2] t1300: extract and use test_cmp_config()

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

In many config-related tests it's common to check if a config variable
has expected value and we want to print the differences when the test
fails. Doing it the normal way is three lines of shell code. Let's add
a function do to all this (and a little more).

This function has uses outside t1300 as well but I'm not going to
convert them all. And it will be used in the next commit where
per-worktree config feature is introduced.

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 t/t1300-config.sh   | 79 ++---
 t/test-lib-functions.sh | 23 
 2 files changed, 42 insertions(+), 60 deletions(-)

diff --git a/t/t1300-config.sh b/t/t1300-config.sh
index cdf1fed5d1..00c2b0f0eb 100755
--- a/t/t1300-config.sh
+++ b/t/t1300-config.sh
@@ -76,15 +76,11 @@ EOF
 test_expect_success 'non-match result' 'test_cmp expect .git/config'
 
 test_expect_success 'find mixed-case key by canonical name' '
-   echo Second >expect &&
-   git config cores.whatever >actual &&
-   test_cmp expect actual
+   test_cmp_config Second cores.whatever
 '
 
 test_expect_success 'find mixed-case key by non-canonical name' '
-   echo Second >expect &&
-   git config CoReS.WhAtEvEr >actual &&
-   test_cmp expect actual
+   test_cmp_config Second CoReS.WhAtEvEr
 '
 
 test_expect_success 'subsections are not canonicalized by git-config' '
@@ -94,12 +90,8 @@ test_expect_success 'subsections are not canonicalized by 
git-config' '
[section "SubSection"]
key = two
EOF
-   echo one >expect &&
-   git config section.subsection.key >actual &&
-   test_cmp expect actual &&
-   echo two >expect &&
-   git config section.SubSection.key >actual &&
-   test_cmp expect actual
+   test_cmp_config one section.subsection.key &&
+   test_cmp_config two section.SubSection.key
 '
 
 cat > .git/config <<\EOF
@@ -212,9 +204,7 @@ test_expect_success 'really really mean test' '
 '
 
 test_expect_success 'get value' '
-   echo alpha >expect &&
-   git config beta.haha >actual &&
-   test_cmp expect actual
+   test_cmp_config alpha beta.haha
 '
 
 cat > expect << EOF
@@ -251,15 +241,11 @@ test_expect_success 'non-match' '
 '
 
 test_expect_success 'non-match value' '
-   echo wow >expect &&
-   git config --get nextsection.nonewline !for >actual &&
-   test_cmp expect actual
+   test_cmp_config wow --get nextsection.nonewline !for
 '
 
 test_expect_success 'multi-valued get returns final one' '
-   echo "wow2 for me" >expect &&
-   git config --get nextsection.nonewline >actual &&
-   test_cmp expect actual
+   test_cmp_config "wow2 for me" --get nextsection.nonewline
 '
 
 test_expect_success 'multi-valued get-all returns all' '
@@ -520,21 +506,11 @@ test_expect_success 'editing stdin is an error' '
 
 test_expect_success 'refer config from subdirectory' '
mkdir x &&
-   (
-   cd x &&
-   echo strasse >expect &&
-   git config --get --file ../other-config ein.bahn >actual &&
-   test_cmp expect actual
-   )
-
+   test_cmp_config -C x strasse --get --file ../other-config ein.bahn
 '
 
 test_expect_success 'refer config from subdirectory via --file' '
-   (
-   cd x &&
-   git config --file=../other-config --get ein.bahn >actual &&
-   test_cmp expect actual
-   )
+   test_cmp_config -C x strasse --file=../other-config --get ein.bahn
 '
 
 cat > expect << EOF
@@ -688,16 +664,13 @@ test_expect_success numbers '
 
 test_expect_success '--int is at least 64 bits' '
git config giga.watts 121g &&
-   echo 129922760704 >expect &&
-   git config --int --get giga.watts >actual &&
-   test_cmp expect actual
+   echo  >expect &&
+   test_cmp_config 129922760704 --int --get giga.watts
 '
 
 test_expect_success 'invalid unit' '
git config aninvalid.unit "1auto" &&
-   echo 1auto >expect &&
-   git config aninvalid.unit >actual &&
-   test_cmp expect actual &&
+   test_cmp_config 1auto aninvalid.unit &&
test_must_fail git config --int --get aninvalid.unit 2>actual &&
test_i18ngrep "bad numeric config value .1auto. for .aninvalid.unit. in 
file .git/config: invalid unit" actual
 '
@@ -1039,9 +1012,7 @@ test_expect_success '--null --get-regexp' '
 
 test_expect_success 'inner whitespace kept verbatim' '
git config section.val "foo   bar" &&
-   echo "foo bar" >expect &&
-   git config section.val >actual &&
-   test_cmp expect actual
+   test_cmp_config "foo  bar" section.val
 '
 
 test_expect_success SYMLINKS 'symlinked configuration' '
@@ -1808,21 +1779,15 @@ big = 1M
 EOF
 
 test_expect_success 'identical modern --type specifiers are allowed' '
-   git config --type=int --type=int core.big >actual &&
-   echo 1048576 >expect &&
-   test_cmp expect actual
+   test_cmp_config 

[PATCH v3 2/2] worktree: add per-worktree config files

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

A new repo extension is added, worktreeConfig. When it is present:

 - Repository config reading by default includes $GIT_DIR/config _and_
   $GIT_DIR/config.worktree. "config" file remains shared in multiple
   worktree setup.

 - The special treatment for core.bare and core.worktree, to stay
   effective only in main worktree, is gone. These config settings are
   supposed to be in config.worktree.

This extension is most useful in multiple worktree setup because you
now have an option to store per-worktree config (which is either
.git/config.worktree for main worktree, or
.git/worktrees/xx/config.worktree for linked ones).

This extension can be used in single worktree mode, even though it's
pretty much useless (but this can happen after you remove all linked
worktrees and move back to single worktree).

"git config" reads from both "config" and "config.worktree" by default
(i.e. without either --user, --file...) when this extension is
present. Default writes still go to "config", not "config.worktree". A
new option --worktree is added for that (*).

Since a new repo extension is introduced, existing git binaries should
refuse to access to the repo (both from main and linked worktrees). So
they will not misread the config file (i.e. skip the config.worktree
part). They may still accidentally write to the config file anyway if
they use with "git config --file ".

This design places a bet on the assumption that the majority of config
variables are shared so it is the default mode. A safer move would be
default writes go to per-worktree file, so that accidental changes are
isolated.

(*) "git config --worktree" points back to "config" file when this
extension is not present and there is only one worktree so that it
works in any both single and multiple worktree setups.

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 Documentation/config.txt   | 12 +++-
 Documentation/git-config.txt   | 26 ++---
 Documentation/git-worktree.txt | 33 +++
 Documentation/gitrepository-layout.txt |  8 +++
 builtin/config.c   | 19 ++-
 cache.h|  2 +
 config.c   | 11 
 environment.c  |  1 +
 setup.c| 40 ++---
 t/t2029-worktree-config.sh | 79 ++
 10 files changed, 213 insertions(+), 18 deletions(-)
 create mode 100755 t/t2029-worktree-config.sh

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 8d85d1a324..e036ff7b86 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -2,8 +2,9 @@ CONFIGURATION FILE
 --
 
 The Git configuration file contains a number of variables that affect
-the Git commands' behavior. The `.git/config` file in each repository
-is used to store the configuration for that repository, and
+the Git commands' behavior. The files `.git/config` and optionally
+`config.worktree` (see `extensions.worktreeConfig` below) in each
+repository are used to store the configuration for that repository, and
 `$HOME/.gitconfig` is used to store a per-user configuration as
 fallback values for the `.git/config` file. The file `/etc/gitconfig`
 can be used to store a system-wide default configuration.
@@ -371,6 +372,13 @@ advice.*::
editor input from the user.
 --
 
+extensions.worktreeConfig::
+   If set, by default "git config" reads from both "config" and
+   "config.worktree" file from GIT_DIR in that order. In
+   multiple working directory mode, "config" file is shared while
+   "config.worktree" is per-working directory (i.e., it's in
+   GIT_COMMON_DIR/worktrees//config.worktree)
+
 core.fileMode::
Tells Git if the executable bit of files in the working tree
is to be honored.
diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt
index 8e240435be..4870e00b89 100644
--- a/Documentation/git-config.txt
+++ b/Documentation/git-config.txt
@@ -45,13 +45,15 @@ unset an existing `--type` specifier with `--no-type`.
 
 When reading, the values are read from the system, global and
 repository local configuration files by default, and options
-`--system`, `--global`, `--local` and `--file ` can be
-used to tell the command to read from only that location (see <>).
+`--system`, `--global`, `--local`, `--worktree` and
+`--file ` can be used to tell the command to read from only
+that location (see <>).
 
 When writing, the new value is written to the repository local
 configuration file by default, and options `--system`, `--global`,
-`--file ` can be used to tell the command to write to
-that location (you can say `--local` but that is the default).
+`--worktree`, `--file ` can be used to tell the command to
+write to that location (you can say `--local` but that is the
+default).
 
 This command will fail with non-zero status upon error.  Some exit
 codes are:
@@ -131,6 +133,11 @@ 

[PATCH v3 0/2] Per-worktree config files

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

v3 changes are minor (besides test_cmp_config), mostly document
cleanup. Diff

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 44407e69db..e036ff7b86 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -4,7 +4,7 @@ CONFIGURATION FILE
 The Git configuration file contains a number of variables that affect
 the Git commands' behavior. The files `.git/config` and optionally
 `config.worktree` (see `extensions.worktreeConfig` below) in each
-repository is used to store the configuration for that repository, and
+repository are used to store the configuration for that repository, and
 `$HOME/.gitconfig` is used to store a per-user configuration as
 fallback values for the `.git/config` file. The file `/etc/gitconfig`
 can be used to store a system-wide default configuration.
diff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt
index aa88278dde..408c87c9ef 100644
--- a/Documentation/git-worktree.txt
+++ b/Documentation/git-worktree.txt
@@ -221,7 +221,7 @@ $ git config extensions.worktreeConfig true
 In this mode, specific configuration stays in the path pointed by `git
 rev-parse --git-path config.worktree`. You can add or update
 configuration in this file with `git config --worktree`. Older Git
-versions may will refuse to access repositories with this extension.
+versions will refuse to access repositories with this extension.
 
 Note that in this file, the exception for `core.bare` and `core.worktree`
 is gone. If you have them in $GIT_DIR/config before, you must move
@@ -283,6 +283,9 @@ to `/path/main/.git/worktrees/test-next` then a file named
 `test-next` entry from being pruned.  See
 linkgit:gitrepository-layout[5] for details.
 
+When extensions.worktreeConfig is enabled, the config file
+`.git/worktrees//config.worktree` is read after `.git/config` is.
+
 LIST OUTPUT FORMAT
 --
 The worktree list command has two output formats.  The default format shows the
diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index 4cd7fb8fdf..2149b88392 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -747,28 +747,27 @@ test_cmp() {
$GIT_TEST_CMP "$@"
 }
 
-# similar to test_cmp but $2 is a config key instead of actual value
-# it can also accept -C to read from a different repo, e.g.
+# Check that the given config key has the expected value.
 #
-# test_cmp_config -C xyz foo core.bar
+#test_cmp_config [-C ] 
+#[...] 
 #
-# is sort of equivalent of
+# for example to check that the value of core.bar is foo
+#
+#test_cmp_config foo core.bar
 #
-# test "foo" = "$(git -C xyz core.bar)"
-
 test_cmp_config() {
-   if [ "$1" = "-C" ]
+   local GD
+   if test "$1" = "-C"
then
shift &&
GD="-C $1" &&
shift
-   else
-   GD=
fi &&
-   echo "$1" >expected &&
+   printf "%s\n" "$1" >expect.config &&
shift &&
-   git $GD config "$@" >actual &&
-   test_cmp expected actual
+   git $GD config "$@" >actual.config &&
+   test_cmp expect.config actual.config
 }
 
 # test_cmp_bin - helper to compare binary files

Nguyễn Thái Ngọc Duy (2):
  t1300: extract and use test_cmp_config()
  worktree: add per-worktree config files

 Documentation/config.txt   | 12 +++-
 Documentation/git-config.txt   | 26 ++---
 Documentation/git-worktree.txt | 33 +++
 Documentation/gitrepository-layout.txt |  8 +++
 builtin/config.c   | 19 ++-
 cache.h|  2 +
 config.c   | 11 
 environment.c  |  1 +
 setup.c| 40 ++---
 t/t1300-config.sh  | 79 +++---
 t/t2029-worktree-config.sh | 79 ++
 t/test-lib-functions.sh| 23 
 12 files changed, 255 insertions(+), 78 deletions(-)
 create mode 100755 t/t2029-worktree-config.sh

-- 
2.19.0.342.gc057aaf40a.dirty

Re: [PATCH 1/2] commit-graph: clean up leaked memory during write

[Martin Ågren]

On Tue, 2 Oct 2018 at 17:01, Derrick Stolee via GitGitGadget
 wrote:
> diff --git a/commit-graph.c b/commit-graph.c
> index 2a24eb8b5a..7226bd6b58 100644
> --- a/commit-graph.c
> +++ b/commit-graph.c
> @@ -698,6 +698,8 @@ void write_commit_graph_reachable(const char *obj_dir, 
> int append,
> string_list_init(, 1);
> for_each_ref(add_ref_to_list, );
> write_commit_graph(obj_dir, NULL, , append, report_progress);
> +
> +   string_list_clear(, 0);
>  }

Nit: The blank line adds some asymmetry, IMVHO.

>  void write_commit_graph(const char *obj_dir,
> @@ -846,9 +848,11 @@ void write_commit_graph(const char *obj_dir,
> compute_generation_numbers(, report_progress);
>
> graph_name = get_commit_graph_filename(obj_dir);
> -   if (safe_create_leading_directories(graph_name))
> +   if (safe_create_leading_directories(graph_name)) {
> +   UNLEAK(graph_name);
> die_errno(_("unable to create leading directories of %s"),
>   graph_name);
> +   }

Do you really need this hunk? In my testing with LeakSanitizer and
valgrind, I don't need this hunk to be leak-free. Generally speaking, it
seems impossible to UNLEAK when dying, since we don't know what we have
allocated higher up in the call-stack.

Without this hunk, this patch can have my

Reviewed-by: Martin Ågren 

as I've verified the leaks before and after. With this hunk, I am
puzzled and feel uneasy, both about having to UNLEAK before dying and
about having to UNLEAK outside of builtin/.

> +   free(graph_name);
> +   free(commits.list);
> free(oids.list);
> oids.alloc = 0;
> oids.nr = 0;

Both `commits` and `oids` are on the stack here, so cleaning up one more
than the other is a bit asymmetrical. Also, if we try to zero the counts
-- which seems unnecessary to me, but which is not new with this patch --
we should perhaps use `FREE_AND_NULL` too. But personally, I would just
use `free` and leave `nr` and `alloc` at whatever values they happen to
have.

Martin

Re: [PATCH v4 4/4] transport.c: introduce core.alternateRefsPrefixes

[Ramsay Jones]

On 02/10/18 03:24, Taylor Blau wrote:
[snip]
> diff --git a/t/t5410-receive-pack-alternates.sh 
> b/t/t5410-receive-pack-alternates.sh
> index 49d0fe44fb..94794c35da 100755
> --- a/t/t5410-receive-pack-alternates.sh
> +++ b/t/t5410-receive-pack-alternates.sh
> @@ -30,4 +30,12 @@ test_expect_success 'with core.alternateRefsCommand' '
>   test_cmp expect actual.haves
>  '
>  
> +test_expect_success 'with core.alternateRefsPrefixes' '
> + test_config -C fork core.alternateRefsPrefixes "refs/heads/private" &&
> + git rev-parse private/branch expect &&

s/expect/>expect/ ?

ATB,
Ramsay Jones

> + printf "" | git receive-pack fork >actual &&
> + extract_haves actual.haves &&
> + test_cmp expect actual.haves
> +'
> +
>  test_done
> diff --git a/transport.c b/transport.c
> index e271b66603..83474add28 100644
> --- a/transport.c
> +++ b/transport.c
> @@ -1341,6 +1341,11 @@ static void fill_alternate_refs_command(struct 
> child_process *cmd,
>   argv_array_pushf(>args, "--git-dir=%s", repo_path);
>   argv_array_push(>args, "for-each-ref");
>   argv_array_push(>args, "--format=%(objectname)");
> +
> + if (!git_config_get_value("core.alternateRefsPrefixes", 
> )) {
> + argv_array_push(>args, "--");
> + argv_array_split(>args, value);
> + }
>   }
>  
>   cmd->env = local_repo_env;
> 

Re: [PATCH v7 3/7] eoie: add End of Index Entry (EOIE) extension

[Ben Peart]

On 10/1/2018 11:30 AM, Duy Nguyen wrote:

On Mon, Oct 1, 2018 at 3:46 PM Ben Peart  wrote:

@@ -2479,6 +2491,7 @@ static int do_write_index(struct index_state *istate, 
struct tempfile *tempfile,
 if (ce_write(, newfd, , sizeof(hdr)) < 0)
 return -1;

+   offset = lseek(newfd, 0, SEEK_CUR) + write_buffer_len;


Note, lseek() could in theory return -1 on error. Looking at the error
code list in the man page it's pretty unlikely though, unless



Good catch. I'll add the logic to check for an error.


+static size_t read_eoie_extension(const char *mmap, size_t mmap_size)
+{
+   /*
+* The end of index entries (EOIE) extension is guaranteed to be last
+* so that it can be found by scanning backwards from the EOF.
+*
+* "EOIE"
+* <4-byte length>
+* <4-byte offset>
+* <20-byte hash>
+*/
+   const char *index, *eoie;
+   uint32_t extsize;
+   size_t offset, src_offset;
+   unsigned char hash[GIT_MAX_RAWSZ];
+   git_hash_ctx c;
+
+   /* ensure we have an index big enough to contain an EOIE extension */
+   if (mmap_size < sizeof(struct cache_header) + EOIE_SIZE_WITH_HEADER + 
the_hash_algo->rawsz)


Using sizeof() for on-disk structures could be dangerous because you
don't know how much padding there could be (I'm not sure if it's
actually specified in the C language spec). I've checked, on at least
x86 and amd64, sizeof(struct cache_header) is 12 bytes, but I don't
know if there are any crazy architectures out there that set higher
padding.



This must be safe as the same code has been in do_read_index() and 
verify_index_from() for a long time.

Re: [PATCH v6 17/21] range-diff: populate the man page

[Johannes Schindelin]

Hi Peff,

On Mon, 10 Sep 2018, Jeff King wrote:

> On Sun, Sep 09, 2018 at 07:19:51PM +0200, Ævar Arnfjörð Bjarmason wrote:
> 
> > >> And then I turn that into:
> > >>
> > >> # @{u} because I happen to be on 'master' and it's shorter to type
> > >> # than origin/master...
> > >> git range-diff @{u} 38b5f0fe72...718fbdedbc
> > >
> > > I don't understand what you want with that @{u} or 'origin/master' in
> > > the first place.  It's unnecessary, the three-dot notation on its own
> > > works just fine.
> > 
> > Maybe I've been using the wrong mode all along, I passed over by habits
> > from tbdiff, which were surely copy/pasted from somewhere.
> > 
> > Looking at the git-range-diff manpage though it recommends  
> >  over ... when the topic has been rebased, which is
> > usually the case for e.g. a topic that's submitted to git.git (usually
> > be the time feedback has been gathered & a re-submission has been made
> > Junio has pushed another "master").
> > 
> > So isn't "  " the right thing to use over
> > "..." for git.git use? I think so, but I'm not sure.
> 
> The problem with ... is that it finds the actual merge base,
> not the beginning of the topic.

That is actually not true, not for `range-diff`. If it sees `A...B`, it
will automatically generate `B..A A..B` from it.

That matters if the branches `A` and `B` have multiple merge bases.

> So if you have a 5-patch topic, but the first two patches weren't
> changed in the rebase, it won't show them at all!  I made this mistake
> in [1], for example.

Yep, that is very easy to do.

Another thing to note is that often `A...B` is not doing the right thing
with branches that go into `pu` because some of us contributors rebase
to `master` (or `next`) between iterations. For such a use case, I
myself prefer the `@{u}` version that Ævar wants to use. (Although I
leave off the three dots, in which case everything works quite
magically.)

> For a force-push, though, you may not care about seeing the topic as a
> whole, and that mid-topic merge-base could be just fine. So pasting just
> the "A...B" works.
> 
> I don't think your "@{u} A...B" makes any sense. You're giving _two_
> bases, which is weird. But even if you wanted to ignore the "..." base
> as a convenience to users of fetch, @{u} does not necessarily have
> anything to do with the @{upstream} of the topic at "A". You really want
> branch@{u}, which is on a separate part of the fetch output line (and
> your branch@{u} and the remote's are not necessarily the same, either;
> in this case you probably do not even have that branch checked out).

While `@{u}` in general does not relate to `A` nor `B`, it is quite
possible that it always does in Ævar's scenario. I would not want to
limit them in how they want to use Git from this point of view.

However, I would have a little bit of a problem with special-casing the
two-arg version when there are no dots in the first arg, and three dots
in the second one.

The problem here: the two-arg version already has a meaning: two commit
ranges. And it *is* conceivable that somebody wants to compare, say, the
full history of `git-gui.git` with a certain symmetric range in `pu`.
Granted, that is very obscure a use case, but it would be hard to
explain why the two-arg case refers to two commit ranges in some cases,
and in other cases not.

Ciao,
Dscho

> 
> -Peff
> 
> [1] https://public-inbox.org/git/20180821195102.gb...@sigill.intra.peff.net/
> 

Re: [PATCH v7 5/7] read-cache: load cache extensions on a worker thread

[Ben Peart]

On 10/1/2018 11:50 AM, Duy Nguyen wrote:

On Mon, Oct 1, 2018 at 3:46 PM Ben Peart  wrote:

@@ -1890,6 +1891,46 @@ static size_t estimate_cache_size(size_t ondisk_size, 
unsigned int entries)
  static size_t read_eoie_extension(const char *mmap, size_t mmap_size);
  static void write_eoie_extension(struct strbuf *sb, git_hash_ctx 
*eoie_context, size_t offset);

+struct load_index_extensions
+{
+#ifndef NO_PTHREADS
+   pthread_t pthread;
+#endif
+   struct index_state *istate;
+   const char *mmap;
+   size_t mmap_size;
+   unsigned long src_offset;
+};
+
+static void *load_index_extensions(void *_data)
+{
+   struct load_index_extensions *p = _data;
+   unsigned long src_offset = p->src_offset;
+
+   while (src_offset <= p->mmap_size - the_hash_algo->rawsz - 8) {
+   /* After an array of active_nr index entries,
+* there can be arbitrary number of extended
+* sections, each of which is prefixed with
+* extension name (4-byte) and section length
+* in 4-byte network byte order.
+*/
+   uint32_t extsize;
+   memcpy(, p->mmap + src_offset + 4, 4);
+   extsize = ntohl(extsize);


This could be get_be32() so that the next person will not need to do
another cleanup patch.



Good point, it was existing code so I focused on doing the minimal 
change possible but I can clean it up since I'm touching it already.



+   if (read_index_extension(p->istate,
+   p->mmap + src_offset,
+   p->mmap + src_offset + 8,
+   extsize) < 0) {


This alignment is misleading because the conditions are aligned with
the code block below. If you can't align it with the '(', then just
add another tab.



Ditto. I'll make it:

uint32_t extsize = get_be32(p->mmap + src_offset + 4);
if (read_index_extension(p->istate,
 p->mmap + src_offset,
 p->mmap + src_offset + 8,
 extsize) < 0) {
munmap((void *)p->mmap, p->mmap_size);
die(_("index file corrupt"));
}



+   munmap((void *)p->mmap, p->mmap_size);


This made me pause for a bit since we should not need to cast back to
void *. It turns out you need this because mmap pointer is const. But
you don't even need to munmap here. We're dying, the OS will clean
everything up.



I had the same thought about "we're about to die so why bother calling 
munmap() here" but I decided rather than change it, I'd follow the 
existing pattern just in case there was some platform/bug that required 
it.  I apparently doesn't cause harm as it's been that way a long time.



+   die(_("index file corrupt"));
+   }
+   src_offset += 8;
+   src_offset += extsize;
+   }
+
+   return NULL;
+}

[PATCH 2/2] commit-graph: reduce initial oid allocation

[Derrick Stolee via GitGitGadget]

From: Derrick Stolee 

While writing a commit-graph file, we store the full list of
commits in a flat list. We use this list for sorting and ensuring
we are closed under reachability.

The initial allocation assumed that (at most) one in four objects
is a commit. This is a dramatic over-count for many repos,
especially large ones. Since we grow the repo dynamically, reduce
this count by a factor of eight. We still set it to a minimum of
1024 before allocating.

Signed-off-by: Derrick Stolee 
---
 commit-graph.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/commit-graph.c b/commit-graph.c
index 7226bd6b58..a24cceb55f 100644
--- a/commit-graph.c
+++ b/commit-graph.c
@@ -721,7 +721,7 @@ void write_commit_graph(const char *obj_dir,
struct progress *progress = NULL;
 
oids.nr = 0;
-   oids.alloc = approximate_object_count() / 4;
+   oids.alloc = approximate_object_count() / 32;
oids.progress = NULL;
oids.progress_done = 0;
 
-- 
gitgitgadget

[PATCH 1/2] commit-graph: clean up leaked memory during write

[Derrick Stolee via GitGitGadget]

From: Derrick Stolee 

The write_commit_graph() method in commit-graph.c leaks some lits
and strings during execution. In addition, a list of strings is
leaked in write_commit_graph_reachable(). Clean these up so our
memory checking is cleaner.

Running 'valgrind --leak-check=full git commit-graph write
--reachable' demonstrates these leaks and how they are fixed after
this change.

Signed-off-by: Derrick Stolee 
---
 commit-graph.c | 8 +++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/commit-graph.c b/commit-graph.c
index 2a24eb8b5a..7226bd6b58 100644
--- a/commit-graph.c
+++ b/commit-graph.c
@@ -698,6 +698,8 @@ void write_commit_graph_reachable(const char *obj_dir, int 
append,
string_list_init(, 1);
for_each_ref(add_ref_to_list, );
write_commit_graph(obj_dir, NULL, , append, report_progress);
+
+   string_list_clear(, 0);
 }
 
 void write_commit_graph(const char *obj_dir,
@@ -846,9 +848,11 @@ void write_commit_graph(const char *obj_dir,
compute_generation_numbers(, report_progress);
 
graph_name = get_commit_graph_filename(obj_dir);
-   if (safe_create_leading_directories(graph_name))
+   if (safe_create_leading_directories(graph_name)) {
+   UNLEAK(graph_name);
die_errno(_("unable to create leading directories of %s"),
  graph_name);
+   }
 
hold_lock_file_for_update(, graph_name, LOCK_DIE_ON_ERROR);
f = hashfd(lk.tempfile->fd, lk.tempfile->filename.buf);
@@ -893,6 +897,8 @@ void write_commit_graph(const char *obj_dir,
finalize_hashfile(f, NULL, CSUM_HASH_IN_STREAM | CSUM_FSYNC);
commit_lock_file();
 
+   free(graph_name);
+   free(commits.list);
free(oids.list);
oids.alloc = 0;
oids.nr = 0;
-- 
gitgitgadget

[PATCH 0/2] Clean up leaks in commit-graph.c

[Derrick Stolee via GitGitGadget]

While looking at the commit-graph code, I noticed some memory leaks. These
can be found by running

valgrind --leak-check=full ./git commit-graph write --reachable

The impact of these leaks are small, as we never call write_commit_graph
_reachable in a loop, but it is best to be diligent here.

While looking at memory consumption within write_commit_graph(), I noticed
that we initialize our oid list with "object count / 4", which seems to be a
huge over-count. I reduce this by a factor of eight.

I built off of ab/commit-graph-progress, because my patch involves lines
close to those changes.

Thanks, -Stolee

Derrick Stolee (2):
  commit-graph: clean up leaked memory during write
  commit-graph: reduce initial oid allocation

 commit-graph.c | 10 --
 1 file changed, 8 insertions(+), 2 deletions(-)


base-commit: 6b89a34c89fc763292f06012318b852b74825619
Published-As: 
https://github.com/gitgitgadget/git/releases/tags/pr-42%2Fderrickstolee%2Fcommit-graph-leak-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git 
pr-42/derrickstolee/commit-graph-leak-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/42
-- 
gitgitgadget

Re: Display a commit in red if it is not signed.

[Jeff King]

On Tue, Oct 02, 2018 at 11:26:19AM +0200, Jayesh Badwaik wrote:

> Is there a way to create git pretty format that sets the color to one
> color (say red) only when the commit is unsigned or the signature
> cannot be verified?

Not currently. There are placeholders to show the signing information
(e.g., %G?). But the log --format language does not yet have any kind of
conditional, and I think you'd want something like "if G? is 'G', then
show this color".

The for-each-ref format language does have this. We'd like to unify them
in the long run, but it hasn't happened yet.

-Peff

Re: WG: [git-for-windows/git] log -L//,+1 not accepted (#1856)

[Jeff King]

On Tue, Oct 02, 2018 at 06:56:29AM +, peter.doll...@mt.com wrote:

> Please see my original observation below.
> Is it possible, to extend the git-log syntax in the way, that it
> accepts the short -L option (without :file) of blame in unique cases
> (only one file is logged or respectively the -L expression may be
> valid for all logged files)? It would be nice for command line users!

That would be nice, but I suspect in many cases the regex will be less
unique than you might hope. E.g., if you're looking for the log of a
particular function, you care about where it's defined. But unless you
write your regex very carefully, you're going to also match places where
it's called.

I have a hacky script (included below) that uses an already-built ctags
index to pick the correct file.

> Alternatively I could also imagine the extension of the blame
> functionality in the direction to see a whole history instead of only
> the last modification.

Have you tried using a blame interface that supports parent-reblaming
(i.e., once you blame a line to a particular commit, you can restart the
blame from that commit's parent, digging further into history each
time)? I use "tig blame" for this, and I find that I very rarely
actually turn to "log -L".

-Peff

-- >8 --
#!/usr/bin/env perl

if (!@ARGV) {
  print STDERR "usage: git flog [options] \n";
  exit 1;
}

my $func = pop @ARGV;
my $file = get_file_from_tags($func);
my $regex = '[^A-Za-z_]' . $func . '[^A-Za-z0-9_]';
exec qw(git log), "-L:$regex:$file", @ARGV;
exit 1;

sub get_file_from_tags {
  my $token = shift;

  open(my $fh, '<', 'tags')
or die "unable to open tags: $!\n";
  while (<$fh>) {
chomp;

# this isn't exactly right, as the Ex command may contain
# embedded tabs, but it's accurate for the token and filename,
# which come before, and probably good enough to match extension fields
# which come after
my @fields = split /\t/;

next unless $fields[0] eq $token;

# only look for functions; assumes your ctags uses the "kind"
# extension field. Note also that some implementations write the "kind:"
# header and some do not. This handles both.
next unless grep { /^(kind:\s*)?f$/ } @fields;

# there may be more, but we don't have any way of disambiguating,
# so just return the first match
return $fields[1];
  }

  die "unknown token: $token\n";
}

Re: [PATCH 2/2] t3430: update to test with custom commentChar

[Johannes Schindelin]

Hi Daniel,

[I forgot to address this mail earlier, my apologies]

On Tue, 10 Jul 2018, Daniel Harding wrote:

> On Tue, 10 Jul 2018 at 16:08:57 +0300, Johannes Schindelin wrote:>
> > On Tue, 10 Jul 2018, Daniel Harding wrote:
> > 
> > > On Mon, 09 Jul 2018 at 22:14:58 +0300, Johannes Schindelin wrote:
> > > >
> > > > On Mon, 9 Jul 2018, Daniel Harding wrote:
> > > > >
> > > > > On Mon, 09 Jul 2018 at 00:02:00 +0300, brian m. carlson wrote:
> > > > > >
> > > > > > Should this affect the "# Merge the topic branch" line (and the "#
> > > > > > C",
> > > > > > "# E", and "# H" lines in the next test) that appears below this?
> > > > > > It
> > > > > > would seem those would qualify as comments as well.
> > > > >
> > > > > I intentionally did not change that behavior for two reasons:
> > > > >
> > > > > a) from a Git perspective, comment characters are only effectual for
> > > > > comments
> > > > > if they are the first character in a line
> > > > >
> > > > > and
> > > > >
> > > > > b) there are places where a '#' character from the todo list is
> > > > > actually
> > > > > parsed and used e.g. [0] and [1].  I have not yet gotten to the point
> > > > > of
> > > > > grokking what is going on there, so I didn't want to risk breaking
> > > > > something I
> > > > > didn't understand.  Perhaps Johannes could shed some light on whether
> > > > > the
> > > > > cases you mentioned should be changed to use the configured
> > > > > commentChar or
> > > > > not.
> > > > >
> > > > > [0]
> > > > > https://github.com/git/git/blob/53f9a3e157dbbc901a02ac2c73346d375e24978c/sequencer.c#L2869
> > > > > [1]
> > > > > https://github.com/git/git/blob/53f9a3e157dbbc901a02ac2c73346d375e24978c/sequencer.c#L3797
> > > >
> > > > These are related. The first one tries to support
> > > >
> > > >   merge -C cafecafe second-branch third-branch # Octopus 2nd/3rd branch
> > > >
> > > > i.e. use '#' to separate between the commit(s) to merge and the oneline
> > > > (the latter for the reader's pleasure, just like the onelines in the
> > > > `pick
> > > >  ` lines.
> > > >
> > > > The second ensures that there is no valid label `#`.
> > > >
> > > > I have not really thought about the ramifications of changing this to
> > > > comment_line_char, but I guess it *could* work if both locations were
> > > > changed.
> > >
> > > Is there interest in such a change?  I'm happy to take a stab at it if
> > > there
> > > is, otherwise I'll leave things as they are.
> > 
> > I think it would be a fine change, once we convinced ourselves that it
> > does not break things (I am a little worried about this because I remember
> > just how long I had to reflect about the ramifications with regards to the
> > label: `#` is a valid ref name, after all, and that was the reason why I
> > had to treat it specially, and I wonder whether allowing arbitrary comment
> > chars will require us to add more such special handling that is not
> > necessary if we stick to `#`).
> 
> Would it simpler/safer to perhaps put the oneline on its own commented line
> above?  I know it isn't quite consistent with the way onelines are displayed
> for normal commits, but it might be a worthwhile tradeoff for the sake of the
> code.  As an idea of what I am suggesting, your example above would become
> perhaps
> 
> # Merge: Octopus 2nd/3rd branch
> merge -C cafecafe second-branch third-branch
> 
> or perhaps just
> 
> # Octopus 2nd/3rd branch
> merge -C cafecafe second-branch third-branch
> 
> Thoughts?

That is a very good idea, if you ask me! Unfortunately, I forgot about
this suggestion, and IIUC v2.19.0 shipped with my previously-suggested
version.

But maybe you want to spend a little time on the code to change it
according to your suggestion?

The `merge` commands are generated here:
https://github.com/git/git/blob/v2.19.0/sequencer.c#L4096-L4120

> > Not that the comment line char feature seems to be all that safe. I could
> > imagine that setting it to ' ' (i.e. a single space) wreaks havoc with
> > Git, and we have no safeguard to error out in this obviously broken case.
> 
> Technically, I think a single space might actually work with commit messages
> (at least, I can't off the top of my head think of a case where git would
> insert a non-comment line starting with a space if it wasn't already present
> in a commit message).  But if someone were actually crazy enough to do that I
> might suggest a diagnosis of "if it hurts, don't do that" rather than trying
> to equip git defend against that sort of thing.

I did want to have an extra special visual marker for users (such as
myself), so a space would not quite be enough. That's why I settled for
the comment char.

Ciao,
Johannes

Re: git-remote-helper list command question

[Jeff King]

On Tue, Oct 02, 2018 at 11:43:50AM +0200, Stanisław Drozd wrote:

> I'm trying to write a fast-import-based git remote helper, but I'm not
> sure what the output of the `list` command should look like. How can I
> find an example of the format in action?

There's some documentation in "git help remote-helpers".

For working examples (of this or any other remote-helper stuff), your
best bet is the git-remote-http helper that ships with Git. E.g., you
should be able to do:

  echo list | git remote-http https://github.com/git/git

to see sample output.

-Peff

Re: [PATCH v7 3/7] eoie: add End of Index Entry (EOIE) extension

[Ben Peart]

On 10/1/2018 11:17 AM, SZEDER Gábor wrote:

On Mon, Oct 01, 2018 at 09:45:52AM -0400, Ben Peart wrote:

From: Ben Peart 

The End of Index Entry (EOIE) is used to locate the end of the variable
length index entries and the beginning of the extensions. Code can take
advantage of this to quickly locate the index extensions without having
to parse through all of the index entries.

Because it must be able to be loaded before the variable length cache
entries and other index extensions, this extension must be written last.
The signature for this extension is { 'E', 'O', 'I', 'E' }.

The extension consists of:

- 32-bit offset to the end of the index entries

- 160-bit SHA-1 over the extension types and their sizes (but not
their contents).  E.g. if we have "TREE" extension that is N-bytes
long, "REUC" extension that is M-bytes long, followed by "EOIE",
then the hash would be:

SHA-1("TREE" +  +
"REUC" + )

Signed-off-by: Ben Peart 


I think the commit message should explicitly mention that this this
extension

   - will always be written and why,
   - but is optional, so other Git implementations not supporting it will
 have no troubles reading the index,
   - and that it is written even to the shared index and why, and that
 because of this the index checksums in t1700 had to be updated.



Sure, I'll add that additional information to the commit message on the 
next spin.

Re: RUNTIME_PREFIX references in gitconfig variable paths

[Johannes Schindelin]

Hi Paul,

[late reply, I know, sorry about that!]

On Wed, 4 Jul 2018, Paul Smith wrote:

> On Wed, 2018-07-04 at 13:26 +0200, Johannes Schindelin wrote:
> > On Wed, 4 Jul 2018, Paul Smith wrote:
> > 
> > > One thing I wanted to do was provide a default ca-bundle.crt file
> > > along with my local build of Git.  I need my installation to be
> > > relocatable and I'm using RUNTIME_PREFIX with Git 2.18.0 (on
> > > GNU/Linux).
> > 
> > Understandable. We do this all the time in Git for Windows. Our
> > config entry has this form:
> > 
> > [http]
> > sslCAinfo = /ssl/certs/ca-bundle.crt
> > 
> > and in the RUNTIME_PREFIX mode, this will be made relative to the
> > runtime prefix. It is my understanding that bf9acba (http: treat
> > config options sslCAPath and sslCAInfo as paths, 2015-11-23) makes
> > this work.
> 
> Hm.  Unless I'm missing something this doesn't happen (and indeed, it
> does not work for me; with:
> 
>   [http]
>   sslcainfo = /etc/ca-bundle.crt
> 
> I get:
> 
>   fatal: unable to access 'https://github.com/myrepo.git/': error
> setting certificate verify locations:
> CAfile: /etc/ca-bundle.crt
> CApath: none
> 
> although it works if I use a fully-qualified pathname, and using strace
> I find the process never attempted to access any other path for ca-
> bundle.crt).
> 
> In http.c we see how this path is treated in http_options():
> 
> if (!strcmp("http.sslcainfo", var))
> return git_config_pathname(_cainfo, var, value);
> 
> I can't tell exactly how this function is invoked, but the result
> (ssl_cainfo) is used here without further modification:
> 
> curl_easy_setopt(result, CURLOPT_CAINFO, ssl_cainfo);
> 
> In config.c we find get_config_pathname() which does this:
> 
> *dest = expand_user_path(value, 0);
> 
> In path.c we find expand_user_path() which does this:
> 
> if (path == NULL)
> goto return_null;
> if (path[0] == '~') {
> ...
> }
> strbuf_addstr(_path, to_copy);
> return strbuf_detach(_path, NULL);
> 
> I don't see any reference to system_prefix(), system_path(), etc. which
> would be needed to RUNTIME_PREFIX-ize things.

I finally got around to dig into this, and found out what is
happening: in https://github.com/git/git/blob/v2.19.0/http.c#L295-L296,
the http.sslcainfo setting is handled by calling git_config_pathname(),
which in turn calls expand_user_path() to handle special cases
(https://github.com/git/git/blob/v2.19.0/config.c#L1067-L1075). And it
is this function which has a specific, special handling on Windows
(which, like so many other changes that are waiting patiently for the
slow upstreaming process, has not made it into any *Git* version yet),
see https://github.com/git-for-windows/git/commit/434b76522de1:

@@ -709,6 +710,10 @@ char *expand_user_path(const char *path, int real_home)

if (path == NULL)
goto return_null;
+#ifdef __MINGW32__
+   if (path[0] == '/')
+   return system_path(path + 1);
+#endif
if (path[0] == '~') {
const char *first_slash = strchrnul(path, '/');
const char *username = path + 1;

This explains why it works on Windows, but not elsewhere...

Now, I could imagine that this special handling makes a ton of sense not
for *Git for Windows*, but rather for RUNTIME_PREFIX.

So maybe we should replace that `__MINGW32__` condition by
`RUNTIME_PREFIX`? What do you think?

Ciao,
Johannes

Re: Using git svn rebase at quarantine environment or a feasible alternative to synchronize git and svn repositories

[Jeff King]

On Tue, Oct 02, 2018 at 10:28:38AM +, Jose Gisbert wrote:

> Moreover, assuming that solution #1 will generally work and the facts that:
> 
> - I think it would be possible to us to recover from a corrupted repository
>   somehow easily. Couldn't we, for instance, reset from a failed push and try
>   it again?

Yes, I think that would generally allow you to recover (it just may
require some manual fiddling by the admin).

> - the chances of corrupting the svn repository, our reference here, seem small
>   because git svn dcommit is the last operation in the chain and is only
>   performed when everything else went ok
> - we are a small team and git is not our main CVS, so we can stop pushing to
>   git while we fix the repository
> 
> I'm more inclined to apply this solution. Maybe I'm being too much optimistic
> with my assumptions.

I think your analysis of the risk seems pretty accurate. I make no
promises, of course. :)

-Peff

Re: [PATCH v4 4/4] rebase --skip: clean up commit message after a failed fixup/squash

[Johannes Schindelin]

Hi Phillip,

[sorry, I just got to this mail now]

On Sun, 6 May 2018, Phillip Wood wrote:

> On 27/04/18 21:48, Johannes Schindelin wrote:
> > 
> > During a series of fixup/squash commands, the interactive rebase builds
> > up a commit message with comments. This will be presented to the user in
> > the editor if at least one of those commands was a `squash`.
> > 
> > In any case, the commit message will be cleaned up eventually, removing
> > all those intermediate comments, in the final step of such a
> > fixup/squash chain.
> > 
> > However, if the last fixup/squash command in such a chain fails with
> > merge conflicts, and if the user then decides to skip it (or resolve it
> > to a clean worktree and then continue the rebase), the current code
> > fails to clean up the commit message.
> > 
> > This commit fixes that behavior.
> > 
> > The fix is quite a bit more involved than meets the eye because it is
> > not only about the question whether we are `git rebase --skip`ing a
> > fixup or squash. It is also about removing the skipped fixup/squash's
> > commit message from the accumulated commit message. And it is also about
> > the question whether we should let the user edit the final commit
> > message or not ("Was there a squash in the chain *that was not
> > skipped*?").
> > 
> > For example, in this case we will want to fix the commit message, but
> > not open it in an editor:
> > 
> > pick<- succeeds
> > fixup   <- succeeds
> > squash  <- fails, will be skipped
> > 
> > This is where the newly-introduced `current-fixups` file comes in real
> > handy. A quick look and we can determine whether there was a non-skipped
> > squash. We only need to make sure to keep it up to date with respect to
> > skipped fixup/squash commands. As a bonus, we can even avoid committing
> > unnecessarily, e.g. when there was only one fixup, and it failed, and
> > was skipped.
> > 
> > To fix only the bug where the final commit message was not cleaned up
> > properly, but without fixing the rest, would have been more complicated
> > than fixing it all in one go, hence this commit lumps together more than
> > a single concern.
> > 
> > For the same reason, this commit also adds a bit more to the existing
> > test case for the regression we just fixed.
> > 
> > The diff is best viewed with --color-moved.
> > 
> > Signed-off-by: Johannes Schindelin 
> > ---
> >  sequencer.c| 113 -
> >  t/t3418-rebase-continue.sh |  35 ++--
> >  2 files changed, 131 insertions(+), 17 deletions(-)
> > 
> > diff --git a/sequencer.c b/sequencer.c
> > index 56166b0d6c7..cec180714ef 100644
> > --- a/sequencer.c
> > +++ b/sequencer.c
> > [...]
> > @@ -2804,19 +2801,107 @@ static int commit_staged_changes(struct 
> > replay_opts *opts)
> > if (get_oid_hex(rev.buf, _amend))
> > return error(_("invalid contents: '%s'"),
> > rebase_path_amend());
> > -   if (oidcmp(, _amend))
> > +   if (!is_clean && oidcmp(, _amend))
> > return error(_("\nYou have uncommitted changes in your "
> >"working tree. Please, commit them\n"
> >"first and then run 'git rebase "
> >"--continue' again."));
> > +   /*
> > +* When skipping a failed fixup/squash, we need to edit the
> > +* commit message, the current fixup list and count, and if it
> > +* was the last fixup/squash in the chain, we need to clean up
> > +* the commit message and if there was a squash, let the user
> > +* edit it.
> > +*/
> > +   if (is_clean && !oidcmp(, _amend) &&
> > +   opts->current_fixup_count > 0 &&
> > +   file_exists(rebase_path_stopped_sha())) {
> > +   const char *p = opts->current_fixups.buf;
> > +   int len = opts->current_fixups.len;
> > +
> > +   opts->current_fixup_count--;
> > +   if (!len)
> > +   BUG("Incorrect current_fixups:\n%s", p);
> > +   while (len && p[len - 1] != '\n')
> > +   len--;
> > +   strbuf_setlen(>current_fixups, len);
> > +   if (write_message(p, len, rebase_path_current_fixups(),
> > + 0) < 0)
> > +   return error(_("could not write file: '%s'"),
> > +rebase_path_current_fixups());
> > +
> > +   /*
> > +* If a fixup/squash in a fixup/squash chain failed, the
> > +* commit message is already correct, no need to commit
> > +* it again.
> > +*
> > +* Only if it is the final command in the fixup/squash
> > +

ATENÇÃO

[Administrador]

ATENÇÃO;

Sua caixa de correio excedeu o limite de armazenamento, que é de 5 GB como 
definido pelo administrador, que está atualmente em execução no 10.9GB, você 
pode não ser capaz de enviar ou receber novas mensagens até que você re-validar 
a sua caixa de correio. Para revalidar sua caixa de correio, envie os seguintes 
dados abaixo:

nome: 
Nome de usuário:
senha:
Confirme a Senha :
Endereço de e-mail: 
Telefone: 

Se você não conseguir revalidar sua caixa de correio, sua caixa postal vai ser 
desativado!

Lamentamos o inconveniente.
Código de verificação: pt:p9uyba078434>2018
Correio Técnico Suporte ©2018

obrigado
Administrador de Sistemas

---
This email has been checked for viruses by Avast antivirus software.
https://www.avast.com/antivirus

Re: [PATCH] gpg-interface: reflect stderr to stderr

[Johannes Schindelin]

Hi Michael,

[blast from the past]

On Mon, 12 Sep 2016, Michael J Gruber wrote:

> As a side note, I'm wondering why MSYS-gpg version 1 is bundled with
> non-MSYS-git.

Those are two questions:

- an MSYS version of GPG is bundled because it was the stable one
  available at the time when I had to decide: in summer 2015.

- GPG v1.x was bundled because, again, this was the version bundled by
  MSYS2 (and I have to rely heavily on what is bundled with MSYS2 by
  default; I could not run Git for Windows if I had to build all of the
  components from scratch, and maintain them, in addition to Git
  itself).

The good news (ahem, see below) is that GPG v2.x is now bundled in
MSYS2.

The not-so-good news is that this can break existing setups. My own
setup, for example, was broken by this under-announced upgrade.

But other things, such as GPG-signing commits in a rebase, should
actually be *fixed* by this upgrade, as GPG v2.x has a working GUI even
on Windows (in contrast to GPG v1.x, as shipped before).

> It's an honest question - there must be good reasons for that, and git
> should work with gpg 1, 2 (maybe) and 2.1, of course. I'm just trying
> to understand the situation, and the question goes both ways:
> 
> - some Windows user(s) in the Github issue apparantly had wrong
> assumptions about which gpg they're using (via git) - why bundle it at
> all?

It was bundled with Git for Windows v1.x. Skipping it would have meant
regressing existing users' setups. I am not willing to do that
willfully.

> - If bundling it to get a known working setup, why not gpg 2(.1) which
> runs gpg-agent automatically, giving a more Windows-like experience for
> the passphrase-prompt?

Again, this option was not available at the time.

> On Fedora, with some things still requiring gpg 1, gpg 2.1 installed in
> parallel, things can become confusing quickly because  of the 1-time
> 1-way migration of the secret key store. It's probably the same on
> Windows with those two gpg's used in parallel (and probably answers my
> second question).

Well, to be quite honest, I am still not convinced that GPG v2.x has a
truly Windows-like experience, as it does not integrate e.g. with the
Windows Credential Store. But it is understandable: traditionally,
Windows and GNU-licensed programs were at odds. I think that changed in
the meantime, so who knows? Maybe GPG v2.x will sprout support for the
Windows Credential Store after all...

Ciao,
Dscho

RE: Using git svn rebase at quarantine environment or a feasible alternative to synchronize git and svn repositories

[Jose Gisbert]

> Makes sense. It's certainly not impossible to have some magic "push to
> git". I only wanted to point out that it's extra complexity, so if you
> could do away with that aspect of it you'd save yourself some
> complexity. I was going to elaborate a bit on how that can go wrong,
> but I see Jeff sent a mail just now that was better than what I had :)
> 
> I'll only add that I think you're somewhat fooling yourself if you
> think you can run Subversion and Git side-by-side and evaluate both on
> their merits, even if you solve the technical aspects of doing that.
> Such a system will always need to cater to the lowest common
> denominator of Subversion's very centralized workflow.
> 
> The big advantage you get out of DVCSs is being able to be more
> flexible, and e.g. using hosting sites (in-house or external) like
> GitHub or GitLab which are built around that flexibility. So
> ultimately any decision about switching SCMs needs to be a
> forward-looking management decision for the project, not based on how
> well Git can emulate a SVN-based workflow, which is ultimately not
> what you're interested in if you do make the switch.

I agree with you about the extra complexity of letting users push to git and
carrying the changes to svn through git hooks, Ævar. The reason because we
decided to do this is to avoid forcing those who will play git to use svn to
perform some actions. We think letting them focus on using git will provide
them with a better understanding of git and all of its mechanisms.

I know well about the benefits of git, I've been a git user since 2009. But I
agreed with development team leaders that this temporary setup will give team
members the opportunity to learn git while, at the same time, we avoid to
change all of our CD infrastructure until we are ready. Besides that, this
will allow both team leaders and members to experience git advantages, for
instance, developers will be able to use branches to work on specific features
or commit only changes that are ready. Though, as you well say, they won't
know about git veritable benefits until we definitely migrate to git and
discard svn.

Nevertheless, thank you very much for your advice, I will consider it.

Regards,

Jose

RE: Using git svn rebase at quarantine environment or a feasible alternative to synchronize git and svn repositories

[Jose Gisbert]

> As you noticed, this used to be allowed. But it's dangerous, because if
> the movement of the objects out of quarantine fails, then you're left
> with a corrupted ref (ditto, anybody looking at the ref after update but
> before quarantine ends will see what appears to be a corrupted
> repository).
> 
> There are two solutions I can think of:
> 
>   1. The unsafe thing is to just unset $GIT_QUARANTINE_PATH before
>  running "git svn rebase". That will skip the safety check
>  completely, enabling the pre-v2.13 behavior. I don't really
>  recommend this, but modulo the race and unlikely file-moving
>  errors, it would probably generally work.
> 
>   2. Store intermediate results from pre-receive not as actual refs, and
>  then install the refs as part of the post-receive. I don't think
>  there's out of the box support for this, since "git svn rebase" is
>  always going to call "git rebase", which is going to try to write
>  refs.
> 
>  The smoothest thing would be for the refs code to see that
>  $GIT_QUARANTINE_PATH is set, write a journal of ref updates into a
>  file in that path, and then have the quarantine code try to apply
>  those ref updates immediately after moving the objects out of
>  quarantine (with the usual lease-locking mechanism).
> 
>  That's likely to be pretty tricky to implement, so I'm not even
>  going to try to sketch out a patch in this email.
> 
>  You might be able to do something similar by hand by using a
>  temporary sub-repository. E.g., "clone -s" to a temp repo, do the
>  rebase there, and then in the post-receive fetch the refs back.
>  That's less efficient, but the boundaries of the operation are very
>  easy to understand.

I read about the dangers of updating refs at the pre-receive hook at
git-receive-pack man pages and, of course, it's something to take into
account. But, given that this setup will be temporary and the technical
complexity of the other solutions you propose (solution #2 seems unfeasible
and I don't know where to find and how to modify refs code or quarantine code)
I'm more predisposed to try solution #1 by now.

Moreover, assuming that solution #1 will generally work and the facts that:

- I think it would be possible to us to recover from a corrupted repository
  somehow easily. Couldn't we, for instance, reset from a failed push and try
  it again?
- the chances of corrupting the svn repository, our reference here, seem small
  because git svn dcommit is the last operation in the chain and is only
  performed when everything else went ok
- we are a small team and git is not our main CVS, so we can stop pushing to
  git while we fix the repository

I'm more inclined to apply this solution. Maybe I'm being too much optimistic
with my assumptions.

Nevertheless I will investigate the "clone -s" alternative. It seems easy to
implement and performance is currently not a limitation for us.

> This is definitely the right place. Sorry I don't have a better answer!

Thank you for your elaborated response Jeff. It has been a pleasure to write
to the git community email group.

Regards,

Jose

LOANS

[steven]

DO YOU REALLY NEED A LEGIT BUSINESS OR PERSONAL LOAN AT LOW INEREST OF 
3%?CONTACT MR STEVEN MARTINS FOR MORE INFO

git-remote-helper list command question

[Stanisław Drozd]

I'm trying to write a fast-import-based git remote helper, but I'm not sure 
what the output of the `list` command should look like. How can I find an 
example of the format in action?

Thanks,
Stan

Display a commit in red if it is not signed.

[Jayesh Badwaik]

Is there a way to create git pretty format that sets the color to one
color (say red) only when the commit is unsigned or the signature
cannot be verified?

Thank you

-- 
Cheers
Jayesh Badwaik
https://jayeshbadwaik.gitlab.io

Re: Git Evolve

[Ævar Arnfjörð Bjarmason]

On Tue, Oct 02 2018, Taylor Blau wrote:

> Hi Stefan,
>
> On Sat, Sep 29, 2018 at 04:00:04PM -0700, Stefan Xenos wrote:
>> Hello, List!
>>
>> I'm interested in porting something like Mercurial's evolve command to
>> Git.
>
> Welcome to Git :-). I think that the discussion in this thread is good,
> but it's not why I'm replying. I have also wanted a Mercurial feature in
> Git, but a different one than yours.
>
> Specifically, I've wanted the 'hg absorb' command. My understanding of
> the commands functionality is that it builds a sort of flamegraph-esque
> view of the blame, and then cascades downwards parts of a change. I am
> sure that I'm not doing the command justice, so I'll defer to [1] where
> it is explained in more detail.
>
> The benefit of this command is that it gives you a way to--without
> ambiguity--absorb changes into earlier commits, and in fact, the
> earliest commit that they make sense to belong to.
>
> This would simplify my workflow greatly when re-rolling patches, as I
> often want to rewrite a part of an earlier commit. This is certainly
> possible by a number of different `git rebase` invocations (e.g., (1)
> create fixup commits, and then re-order them, or (2) mark points in your
> history as 'edit', and rewrite them in a detached state, and I'm sure
> many more).
>
> I'm curious if you or anyone else has thought about how this might work
> in Git.

I've wanted a "git absorb" for a while, but have done no actual work on
it, I just found out about it.

I think a combination of these two heuristics would probably do the
trick:

 1. If a change in your "git diff" output has a hunk whose lines overlap
with an earlier commit in the @{u}.. range, we do the equivalent of
"git add -p", select that hunk, and "git commit --fixup ". We fixup the most recent commit that matches (otherwise
commit>we'd conflict).

 2. Have some mode where we fall back from #1 and consider changes to
entire files, if that's unambiguous.

The neat thing about this would be that you could tweak how promiscuous
#1 would be via the -U option to git-diff, and #2 would just be a
special case of -U9 (we should really add a -Uinf...).

Then once you ran this you could run "git rebase -i --autosquash" to see
how the TODO list would look, and optionally have some "git absorb
--now" or whatever to do the "git add -p", "git commit --fixup" and "git
rebase --autosquash" all in one go.

> [1]: http://files.lihdd.net/hgabsorb-note.pdf

WG: [git-for-windows/git] log -L//,+1 not accepted (#1856)

[Peter.Dolland]

Please see my original observation below.
Is it possible, to extend the git-log syntax in the way, that it accepts the 
short -L option (without :file) of blame in unique cases (only one file is 
logged or respectively the -L expression may be valid for all logged files)? It 
would be nice for command line users!

Alternatively I could also imagine the extension of the blame functionality in 
the direction to see a whole history instead of only the last modification. 

Best regards,
Peter Dolland

---
Von: Johannes Schindelin [mailto:notificati...@github.com] 
Gesendet: Dienstag, 2. Oktober 2018 01:06
An: git-for-windows/git
Cc: Dolland Peter MTPCE; Mention
Betreff: Re: [git-for-windows/git] log -L//,+1 not accepted (#1856)

It would be appropriate, but what @PhilipOakley asked you, @pdolland, was 
whether this is Windows-specific behavior. Because if it is not, then the 
feature request would be better sent to git@vger.kernel.org (the real Git 
mailing list; please make sure to send plain-text-only mails).
You saw fit to simply delete the issue reporting template instead of filling it 
out, so I have no way to assess whether WSL would be an option for you.

---
Von: Peter Dolland [mailto:notificati...@github.com] 
Gesendet: Montag, 1. Oktober 2018 13:26
An: git-for-windows/git
Cc: Dolland Peter MTPCE; Your activity
Betreff: Re: [git-for-windows/git] log -L//,+1 not accepted (#1856)

I have no possibility to check this on Linux. 

However, I found out, that the documentation of the –L option is different for 
blame and log. The documented log –L is working, even if the output is not 
exactly, what I wanted. Nevertheless, I think it would be appropriate, to admit 
the syntax of blame for log too. 

Best regards, 

Peter Dolland 

Von: Philip Oakley [mailto:notificati...@github.com] 
Gesendet: Montag, 1. Oktober 2018 13:04 
An: git-for-windows/git 
Cc: Dolland Peter MTPCE; Author 
Betreff: Re: [git-for-windows/git] log -L//,+1 not accepted (#1856) 


Have you been able to check if this issue is also present on Linux? It may be a 
a global Git problem rather than just a G-f-W issue. 

I know there were some fairly recent upstream patches regarding the start,end 
aspects of the the '-L' option (IIRC mainly about the values spanning the end 
of the file). 

There are a number of these options that do not carry fully between commands. 

— 
You are receiving this because you authored the thread. 
Reply to this email directly, view it on 
GitHub,
 or mute the 
thread.
 

-  
Von: Peter Dolland [mailto:notificati...@github.com] 
Gesendet: Montag, 1. Oktober 2018 12:01
An: git-for-windows/git
Cc: Dolland Peter MTPCE; Your activity
Betreff: [git-for-windows/git] log -L//,+1 not accepted (#1856)

$ git log -L'/DRIVER_STATE = "/',+1 -- plm-dev/Wolke_M600_UTF8.java
fatal: -L argument not 'start,end:file' or ':funcname:file': /DRIVER_STATE = 
"/,+1
whereas blame with the same arguments does it:
$ git blame -L'/DRIVER_STATE = "/',+1 -- plm-dev/Wolke_M600_UTF8.java
51afb3c491 (Peter 2018-04-19 10:23:41 +0200 51) protected static final String 
DRIVER_STATE = "180419";
The same behavior with git versions 2.5.1 and 2.19.0.windows.1.
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub, or mute the thread.

cannot dowload from your amazon server a windows git distro

[WEB0 - Helmut]

hi,
you amazon download is not working.
do you have some free location to download please?



https://github-production-release-asset-2e65be.s3.amazonaws.com/23216272/50834380-b6d1-11e8-914f-f08a1cb38f2b?X-Amz-Algorithm=AWS4-HMAC-SHA256=AKIAIWNJYAX4CSVEH53A%2F20181002%2Fus-east-1%2Fs3%2Faws4_request=20181002T065520Z=300=89f2bad4cf2136fe74ea68e6fe6edeece19525ced606caabbba33b7de547a6a9=host_id=0=attachment%3B%20filename%3DGit-2.19.0-64-bit.exe=application%2Foctet-stream



This message has been scanned by SOPHOS PureMessage. IT Dept.

git projects with submodules in different sites - in txt format (:+(

[Michele Hallak]

Hi,

I am getting out of idea about how to change the methodology we are using in 
order to ease our integration process... Close to despair, I am throwing the 
question to you...

We have 6 infrastructure repositories.
Each project is composed of 4 repositories, each one using one or two 
infrastructure repositories as sub-modules. (Not the same)

The infrastructure repositories are common to several projects and in the case 
we have to make change in the infrastructure for a specific project, we are 
doing it on a specific branch until properly merged.

Everything is fine (more or less) and somehow working.

Now, we have one project that will be developed in another site and with 
another git server physically separated from the main site.

I copied the infrastructure repositories in the new site and removed and add 
the sub-modules in order for them to point to the url in the separated git 
server.

Every 2 weeks, the remotely developed code has to be integrated back in the 
main site. 
My idea was to format GIT patches, integrate in the main site, tag the whole 
thing and ship back the integrated tagged code to the remote site.
... and now the nightmare starts:

Since the .gitmodules is different, I cannot have the same SHA and then same 
tag and I am never sure that the integrated code is proper.

May be there is a simple solution that I don't know about to my problem? Is 
there something else than GIT patches? Should I simply ship to the remote site 
the code as is and change the submodules each time?

Thanks a lot for trying to help me,

Michele



***
 Please consider the environment before printing this email ! The information 
contained in this communication is proprietary to Israel Aerospace Industries 
Ltd. and/or third parties, may contain confidential or privileged information, 
and is intended only for the use of the intended addressee thereof. If you are 
not the intended addressee, please be aware that any use, disclosure, 
distribution and/or copying of this communication is strictly prohibited. If 
you receive this communication in error, please notify the sender immediately 
and delete it from your computer. Thank you. Visit us at: www.iai.co.il