Re: [PATCH v2 2/6] grep.c: take column number as argument to show_line()

[Ævar Arnfjörð Bjarmason]

On Mon, Apr 23 2018, Eric Sunshine wrote:

> On Sun, Apr 22, 2018 at 9:17 PM, Taylor Blau  wrote:
> One important issue I noticed is that patch 3/7 neglects to update
> grep.c:init_grep_defaults() to initialize opt.color_columnno.

I think this is fine for fields that are 0 by default, since the struct
 is already zero'd out. See my e62ba43244 ("grep: remove redundant
 double assignment to 0", 2017-06-29) for some prior art.

> Looking at the tests again, are you gaining anything by placing them
> inside that for-loop? (Genuine question.)

The tests in that loop are just to stress-test grep with/without a
working tree. Even though we can see from the implementation that it's
the same in both cases here, I think it makes sense to add new stuff to
that loop by default to stress that case.

Re: [PATCH v2 2/6] grep.c: take column number as argument to show_line()

[Eric Sunshine]

On Mon, Apr 23, 2018 at 3:27 AM, Ævar Arnfjörð Bjarmason
 wrote:
> On Mon, Apr 23 2018, Eric Sunshine wrote:
>> One important issue I noticed is that patch 3/7 neglects to update
>> grep.c:init_grep_defaults() to initialize opt.color_columnno.
>
> I think this is fine for fields that are 0 by default, since the struct
>  is already zero'd out. See my e62ba43244 ("grep: remove redundant
>  double assignment to 0", 2017-06-29) for some prior art.

Indeed, I wasn't worried about opt.columnnum, which is fine being
zero'd out by the memset(). What I was concerned about was
opt.color_columnno; the corresponding opt.color_lineno is handled
explicitly in init_grep_defaults():

color_set(opt->color_lineno, "");

I'd expect opt.color_columnno to be treated likewise.

Re: [PATCH v3 6/6] help: use command-list.txt for the source of guides

[Eric Sunshine]

On Sat, Apr 21, 2018 at 12:54 PM, Nguyễn Thái Ngọc Duy
 wrote:
> The help command currently hard codes the list of guides and their
> summary in C. Let's move this list to command-list.txt. This lets us
> extract summary lines from Documentation/git*.txt. This also
> potentially lets us lists guides in git.txt, but I'll leave that for
> now.
>
> Signed-off-by: Nguyễn Thái Ngọc Duy 
> ---
> diff --git a/generate-cmdlist.sh b/generate-cmdlist.sh
> @@ -58,7 +58,11 @@ command_list "$1" |
>  do
> -   prefix=git-
> +   if [ "$category" = guide ]; then

Style:

if test "$category" = guide
then
...

> +   prefix=git
> +   else
> +   prefix=git-
> +   fi

Re: [PATCH v2 3/6] generate-cmdlist.sh: keep all information in common-cmds.h

[Øystein Walle]

Junio C Hamano  writes:

> Nguyễn Thái Ngọc Duy   writes:
> 
> > +category_list () {
> > +   command_list "$1" | awk '{print $2;}' | sort | uniq
> > +}
> 
> Piping output of awk to sort/uniq, instead of processing all inside
> awk within the END block of the script, means that we are wasting
> two processes---I do not think we care too much about it, but some
> people might.
> 

Can be written as:

command_list "$1" | awk '!seen[$2]++ {print $2}'

This doesn't actually sort it, though, which I'm not sure whether is a
good thing or a bad thing in this case. But it is less work, and being fast is
nice for completion scripts.

Øsse

Re: [PATCH v2 2/6] grep.c: take column number as argument to show_line()

[Ævar Arnfjörð Bjarmason]

On Mon, Apr 23 2018, Taylor Blau wrote:

> For your consideration: https://github.com/ttaylorr/git/compare/tb/grep-colno

Looks good to me aside from two minor issues I noticed:

 * In "grep.c: display column number of first match" you use a comment
   style we don't normally use, i.e. /**\n not /*\n. See "Multi-line
   comments" in Documentation/CodingGuidelines.

 * You're not updating contrib/git-jump/README to document the new
   output format. It just refers to the old format, but now depending on
   if you use "grep" or not it'll use this new thing. It also makes
   sense to update the example added in 007d06aa57 ("contrib/git-jump:
   allow to configure the grep command", 2017-11-20) which seems to have
   added jump.grepCmd as a workaround for not having this.

But also, after just looking at this the second time around; Is there a
reason we shouldn't just call this --column, not --column-number? I
realize the former works because of the lazyness of our getopt parsing
(also --colu though..).

I think when we add features to git-grep we should be as close to GNU
grep as possible (e.g. not add this -m alias meaning something different
as in your v1), but if GNU grep doesn't have something go with the trend
of other grep tools, as noted at
https://beyondgrep.com/feature-comparison/ (and I found another one that
has this: https://github.com/beyondgrep/website/pull/83), so there's
already 3 prominent grep tools that call this just --column.

I think we should just go with that.

Also, as a bonus question, since you're poking at this column code
anyway, interested in implementing -o (--only-matching)? That would be
super-useful (see
https://public-inbox.org/git/87in9ucsbb@evledraar.gmail.com/) :)

Re: [PATCH v2 3/6] generate-cmdlist.sh: keep all information in common-cmds.h

[SZEDER Gábor]

> Junio C Hamano  writes:
> 
> > Nguyễn Thái Ngọc Duy   writes:
> > 
> > > +category_list () {
> > > + command_list "$1" | awk '{print $2;}' | sort | uniq
> > > +}
> > 
> > Piping output of awk to sort/uniq, instead of processing all inside
> > awk within the END block of the script, means that we are wasting
> > two processes---I do not think we care too much about it, but some
> > people might.
> > 
> 
> Can be written as:
> 
> command_list "$1" | awk '!seen[$2]++ {print $2}'
> 
> This doesn't actually sort it, though, which I'm not sure whether is a
> good thing or a bad thing in this case. But it is less work, and being fast is
> nice for completion scripts.

This script is run during the build process, not during completion.

(The order wouldn't matter for completion, because the shell would
sort possible completion words anyway.)

Re: Fw: New Defects reported by Coverity Scan for git [argv_array: offer to split a string by whitespace]

[Johannes Schindelin]

Hi Philip,

On Sun, 22 Apr 2018, Philip Oakley wrote:

> is this part of your series "argv_array: offer to split a string by
> whitespace"?
> 
> https://public-inbox.org/git/capig+ctdbttuefymkntm773ebge14tpic4g4xefusvwsypd...@mail.gmail.com/
> 
> - Original Message - From: 
> Sent: Saturday, April 21, 2018 10:53 AM
> Subject: New Defects reported by Coverity Scan for git
> 
> > New defect(s) Reported-by: Coverity Scan
> > Showing 1 of 1 defect(s)
> >
> >
> > ** CID 1434982:  Memory - corruptions  (OVERRUN)
> >
> >
> > 
> > *** CID 1434982:  Memory - corruptions  (OVERRUN)
> > /builtin/replace.c: 475 in convert_graft_file()
> > 469
> > 470 while (strbuf_getline(, fp) != EOF) {
> > 471 if (*buf.buf == '#')
> > 472 continue;
> > 473
> > 474 argv_array_split(, buf.buf);
> > > > > CID 1434982:  Memory - corruptions  (OVERRUN)
> > > > > Overrunning buffer pointed to by "args.argv" of 8 bytes by passing
> > > > > it to a function which accesses it at byte offset 8.
> > 475 if (args.argc && create_graft(args.argc, args.argv, force))
> > 476 strbuf_addf(, "\n\t%s", buf.buf);
> > 477 argv_array_clear();
> > 478 }
> > 479
> > 480 strbuf_release();

Yes, it is. Coverity has problems to figure out what is really happening
here, and it has the exact same problems with strbufs.

We initialize both of these structs using static initializers, with
specific, empty arrays. When we need to reallocate, we figure out that the
empty array was still there and replace it with a NULL so we can realloc.
So there is no buffer overrun, but Coverity cannot figure that out, and as
much as I tried, I could not come up with a "template" to shut up
Coverity.

Ciao,
Dscho

Re: [PATCH v8 06/16] sequencer: introduce the `merge` command

[Johannes Schindelin]

Hi Phillip,

On Sun, 22 Apr 2018, Phillip Wood wrote:

> On 21/04/18 16:56, Phillip Wood wrote:
> > On 21/04/18 11:33, Johannes Schindelin wrote:
> >> This patch is part of the effort to reimplement `--preserve-merges` with
> >> a substantially improved design, a design that has been developed in the
> >> Git for Windows project to maintain the dozens of Windows-specific patch
> >> series on top of upstream Git.
> >>
> >> The previous patch implemented the `label` and `reset` commands to label
> >> commits and to reset to labeled commits. This patch adds the `merge`
> >> command, with the following syntax:
> > 
> > The two patches seem to have been fused together in this series.
> > 
> > If the reset command fails because it would overwrite untracked files it
> > says
> > 
> > error: Untracked working tree file 'b' would be overwritten by merge.
> > 
> > Followed by the hint to edit the todo file. Saying 'merge' rather
> > 'reset' is possibly confusing to users. Perhaps it could call
> > setup_unpack_trees_porcelain(), though that would need to be extended to
> > handle 'reset'.
> 
> 
> > Also it currently refuses to overwrite ignored files
> > which is either annoying or safe depending on one's point of view.
> 
> Looking at the existing code this is consistent with (most) of the rest
> of the sequencer. The code to fast-forward commits will overwrite
> ignored files, and I think the initial checkout will as well but the
> rest (picking commits and the new merge command) will not.

I never thought about that... but then, I never came close to encountering
such an issue, as I do not typically turn ignored files into tracked ones
;-)

Ciao,
Dscho

Re: [PATCH v1 1/2] merge: Add merge.renames config setting

[Ben Peart]

On 4/20/2018 1:26 PM, Elijah Newren wrote:

On Fri, Apr 20, 2018 at 10:02 AM, Elijah Newren  wrote:

On Fri, Apr 20, 2018 at 6:36 AM, Ben Peart  wrote:

--- a/Documentation/merge-config.txt
+++ b/Documentation/merge-config.txt
@@ -37,6 +37,11 @@ merge.renameLimit::
 during a merge; if not specified, defaults to the value of
 diff.renameLimit.

+merge.renames::
+   Whether and how Git detects renames.  If set to "false",
+   rename detection is disabled. If set to "true", basic rename
+   detection is enabled. This is the default.


One can already control o->detect_rename via the -Xno-renames and
-Xfind-renames options.  I think the documentation should mention that
"false" is the same as passing -Xno-renames, and "true" is the same as
passing -Xfind-renames.  However, find-renames does take similarity
threshold as a parameter, so there's a question whether this option
should provide some way to do the same.  I'm not sure the answer to
that; it may be that we'd want a separate config option for that, and
we can wait to add it until someone actually wants it.


I just realized another issue, though it also affects -Xno-renames.
Even if rename detection is turned off for the merge, it is
unconditionally turned on for the diffstat.  In builtin/merge.c,
function finish(), there is the code:

 if (new_head && show_diffstat) {
 ...
 opts.detect_rename = DIFF_DETECT_RENAME;

It seems that this option should affect that line as well.  (Do you
have diffstat turned off by chance?  If not, you may be able to
improve your performance even more...)



Seems reasonable to me.  I'll update the patch to do that.

Re: [PATCH v1 1/2] merge: Add merge.renames config setting

[Ben Peart]

On 4/22/2018 8:07 AM, Eckhard Maaß wrote:

On Fri, Apr 20, 2018 at 11:34:25AM -0700, Elijah Newren wrote:

Sorry, I think I wasn't being clear.  The documentation for the config
options for e.g. diff.renameLimit, fetch.prune, log.abbrevCommit, and
merge.ff all mention the equivalent command line parameters.  Your
patch doesn't do that for merge.renames, but I think it would be
helpful if it did.


I wonder here what the relation to the diff.* options should be in this
regard anyway. There is already diff.renames. Naively, I would assume
that these options are in sync, that is you control the behavior of both
the normal diff family like git show and git merge. The reasoning, at
least for me, is to keep consistency between the outcome of rename
detection while merging and a later simple "git show MERGE_BASE..HEAD".
I would expect those to give me the same style of rename detection.

Hence, I would like to use diff.renames and maybe enhance this option to
also carry the score in backward compatible way (or introduce a second
configuration option?). Is this idea going in a good direction? If yes,
I will try to submit a patch for this.


It's a fair question.  If you look at all the options in 
Documentation/merge-config.txt, you will see many merge specific 
settings.  I think the ability to control these settings separately is 
pretty well established.


In commit 2a2ac926547 when merge.renamelimit was added, it was decided 
to have separate settings for merge and diff to give users the ability 
to control that behavior.  In this particular case, it will default to 
the value of diff.renamelimit when it isn't set.  That isn't consistent 
with the other merge settings.


Changing that behavior across the rest of the merge settings is outside 
the scope of this patch.  I don't have a strong opinion as to whether 
that is a good or bad thing.




Ah, by the way: for people that have not touched diff.renames there will
be no visible change in how Git behaves - the default for diff.renames
is a rename with 50% score with is the same for merge. So it will only
change if one has tweaked diff.renames already. But I wonder if one does
that and expect the merge to use a different rename detection anyway.

Greetings,
Eckhard

Re: [PATCH v1 1/2] merge: Add merge.renames config setting

[Ben Peart]

On 4/20/2018 2:34 PM, Elijah Newren wrote:

Hi Ben,

On Fri, Apr 20, 2018 at 10:59 AM, Ben Peart  wrote:


On 4/20/2018 1:02 PM, Elijah Newren wrote:


On Fri, Apr 20, 2018 at 6:36 AM, Ben Peart 
wrote:


--- a/Documentation/merge-config.txt
+++ b/Documentation/merge-config.txt
@@ -37,6 +37,11 @@ merge.renameLimit::
  during a merge; if not specified, defaults to the value of
  diff.renameLimit.

+merge.renames::
+   Whether and how Git detects renames.  If set to "false",
+   rename detection is disabled. If set to "true", basic rename
+   detection is enabled. This is the default.



One can already control o->detect_rename via the -Xno-renames and
-Xfind-renames options.


This statement wasn't meant to be independent of the sentence that
followed it...


Yes, but that requires people to know they need to do that and then remember
to pass it on the command line every time.  We've found that doesn't
typically happen, we just get someone complaining about slow merges. :)

That is why we added them as config options which change the default. That
way we can then set them on the repo and the default behavior gives them
better performance.  They can still always override the config setting with
the command line options.


Sorry, I think I wasn't being clear.  The documentation for the config
options for e.g. diff.renameLimit, fetch.prune, log.abbrevCommit, and
merge.ff all mention the equivalent command line parameters.  Your
patch doesn't do that for merge.renames, but I think it would be
helpful if it did.

Also, a link in the documentation the other way, from
Documentation/merge-strategies.txt under the entries for -Xno-renames
and -Xfind-renames should probably mention this new merge.renames
config setting (much like the -Xno-renormalize flag mentions the
merge.renomralize config option).



I'm all in favor of having more information in the documentation.  I'm 
of the opinion that if someone has made the effort to actually _read_ 
the documentation, we should be as descriptive and complete as possible.


I'll take a cut at adding the things you have pointed out would be helpful.


I agree that's the pre-existing behavior, but prior to this patch
turning off rename detection could only be done manually with every
invocation.  I'm slightly concerned that users might be confused if
merge.renames was set to false somewhere -- perhaps even in a global
/etc/gitconfig that they had no knowledge of or control over -- and in
an attempt to get rename detection to work they started passing larger
and larger values for renameLimit all to no avail.

The easy fix here may just be documenting the diff.renameLimit and
merge.renameLimit options that they have no effect if rename detection
is turned off.


I can add this additional documentation as well.  While some might think 
it is stating the obvious, I'm sure someone will benefit from it being 
explicitly called out.




Or maybe I'm just worrying too much, but we (folks at $dayjob) were
bit pretty hard by renameLimit silently being capped at a value less
than the user specified and in a way that wasn't documented anywhere.

Re: [PATCH v3 4/6] git.c: implement --list-cmds=porcelain

[SZEDER Gábor]

On Sat, Apr 21, 2018 at 6:54 PM, Nguyễn Thái Ngọc Duy  wrote:
> This is useful for git-completion.bash because it needs this set of
> commands. Right now we have to maintain a separate command category in
> there.

I don't really understand this paragraph, in particular its second
sentence.  I would have described this change like this:

  To get the list of commands offered after 'git ', the
  completion script filters out plumbing commands from the list of all
  git commands.  To do so, it contains a long hard-coded list of the
  names of all known plumbing commands, which is redundant with the
  categorization in 'command-list.txt', is a maintenance burden, and
  tends to get out-of-sync when new plumbing commands are added.

  Implement 'git --list-cmds=porcelain' to list only commands
  categorized as porcelains, so we can get rid of that long hard-coded
  command list from the completion script.

But then I noticed that it's not an accurate description of the
current situation, because there is a wide grey area between
porcelains and plumbing, and the completion script doesn't "filter out
plumbing commands", but rather filters out commands that can be
considered too low-level to be useful for "general" usage.
Consequently, after 'git ' we also list:

  - some 'ancillaryinterrogators': blame, annotate, difftool, fsck,
help
  - some 'ancillarymanipulators': config, mergetool, remote
  - some 'foreignscminterface': p4, request-pull, svn, send-email
  - even some plumbing: apply, name-rev (though 'name-rev' could be
omitted; we have 'git describe')
  - and also all "unknown" 'git-foo' commands that can be found in
$PATH, which can be the user's own git scripts or other
git-related tools ('git-annex', Git LFS, etc.).

With this change we wouldn't list any of the above commands, but only
those that are explicitly categorized as 'mainporcelain'.  I'd much
prefer the current behaviour.


> Note that the current completion script incorrectly classifies
> filter-branch as porcelain and t9902 tests this behavior. We keep it
> this way in t9902 because this test does not really care which
> particular command is porcelain or plubmbing, they're just names.
>
> Signed-off-by: Nguyễn Thái Ngọc Duy 
> ---
>  contrib/completion/git-completion.bash | 106 +++--
>  git.c  |   2 +
>  help.c |  12 +++
>  help.h |   1 +
>  t/t9902-completion.sh  |   6 +-
>  5 files changed, 31 insertions(+), 96 deletions(-)
>
> diff --git a/contrib/completion/git-completion.bash 
> b/contrib/completion/git-completion.bash
> index a5f13ade20..7d17ca23f6 100644
> --- a/contrib/completion/git-completion.bash
> +++ b/contrib/completion/git-completion.bash


> @@ -859,101 +864,14 @@ __git_all_commands=
>  __git_compute_all_commands ()
>  {
> test -n "$__git_all_commands" ||
> -   __git_all_commands=$(__git_list_all_commands)
> -}
> -
> -__git_list_porcelain_commands ()
> -{
> -   local i IFS=" "$'\n'
> -   __git_compute_all_commands
> -   for i in $__git_all_commands
> -   do
> -   case $i in
> -   *--*) : helper pattern;;
> -   applymbox): ask gittus;;
<...>
> -   verify-tag)   : plumbing;;

I will be glad to see this enormous list go.

> -   *) echo $i;;
> -   esac
> -   done
> +   __git_all_commands=$(__git_list_commands all)
>  }

Re: Silly "git gc" UI issue.

[Junio C Hamano]

Christian Couder  writes:

> On Sat, Apr 21, 2018 at 5:13 AM, Junio C Hamano  wrote:
>
>> @@ -388,6 +389,9 @@ int cmd_gc(int argc, const char **argv, const char 
>> *prefix)
>> if (argc > 0)
>> usage_with_options(builtin_gc_usage, builtin_gc_options);
>>
>> +   if (prune_expire && parse_expiry_date(prune_expire, ))
>> +   die(_("Failed to parse prune expiry value %s"), 
>> prune_expire);
>
> Micronit: I thought we prefer error messages to start with a lower
> case letter, like:
>
>die(_("failed to parse prune expiry value %s"), prune_expire);

Thanks.

There is an existing "Failed..." already before the pre-context of
this hunk, which I'll fix with a preliminary clean-up patch.

Re: [PATCH v3 2/6] git.c: implement --list-cmds=all and use it in git-completion.bash

[SZEDER Gábor]

On Sat, Apr 21, 2018 at 6:54 PM, Nguyễn Thái Ngọc Duy  wrote:
> Signed-off-by: Nguyễn Thái Ngọc Duy 

Oh, look, an empty commit message! :)
So what does this option do and why use it in the completion script?

Re: [PATCH v8 06/16] sequencer: introduce the `merge` command

[Johannes Schindelin]

Hi Phillip,

On Sat, 21 Apr 2018, Phillip Wood wrote:

> On 21/04/18 11:33, Johannes Schindelin wrote:
> > This patch is part of the effort to reimplement `--preserve-merges` with
> > a substantially improved design, a design that has been developed in the
> > Git for Windows project to maintain the dozens of Windows-specific patch
> > series on top of upstream Git.
> > 
> > The previous patch implemented the `label` and `reset` commands to label
> > commits and to reset to labeled commits. This patch adds the `merge`
> > command, with the following syntax:
> 
> The two patches seem to have been fused together in this series.

Indeed. I have yet to investigate further how that happened, it could be a
bug in my series after all.

> If the reset command fails because it would overwrite untracked files it
> says
> 
> error: Untracked working tree file 'b' would be overwritten by merge.
> 
> Followed by the hint to edit the todo file. Saying 'merge' rather 'reset' is
> possibly confusing to users. Perhaps it could call
> setup_unpack_trees_porcelain(), though that would need to be extended to
> handle 'reset'.

Yes, and it changes global state :-(

Maybe we can leave it as-is for now? After all, it should be clear to the
user what is happening. The most important part is the "Untracked working
tree file"...

> Also it currently refuses to overwrite ignored files which is either
> annoying or safe depending on one's point of view.

Let me think about that. My gut feeling says: if it is easy to do, then
let's nuke ignored files, but keep untracked files. But I do not think
that the unpack-trees machinery was taught to know about .gitignore...

Seeing as `label` and `reset` really are mostly about revisions we see
along the lines, I think that the common case will *not* overwrite any
untracked files, ever. You would have to use `reset` on a
not-previously-seen commit and/or add `exec` commands designed to
interfere with the `reset`.

So I tend to want to not bother with discerning between untracked and
ignored files here.

Ciao,
Dscho

Re: [PATCH v8 06/16] sequencer: introduce the `merge` command

[Johannes Schindelin]

Hi Philip,

On Sun, 22 Apr 2018, Philip Oakley wrote:

> From: "Johannes Schindelin" 
> > This patch is part of the effort to reimplement `--preserve-merges` with
> > a substantially improved design, a design that has been developed in the
> > Git for Windows project to maintain the dozens of Windows-specific patch
> > series on top of upstream Git.
> >
> > The previous patch implemented the `label` and `reset` commands to label
> 
> The previous patch was [Patch 05/16] git-rebase--interactive: clarify
> arguments, so this statement doesn't appear to be true. Has a patch been
> missed or re-ordered? Or should it be simply "This patch implements" ?
> Likewise the patch subject would be updated.

As Phillip guessed correctly, it was a mistaken `git commit --amend`.

> > commits and to reset to labeled commits. This patch adds the `merge`
> 
> s/adds/also adds/ ?

No, as I really want to keep those two commits separate. I disentangled
them.

> > command, with the following syntax:
> >
> > merge [-C ]  # 
> >
> > The  parameter in this instance is the *original* merge commit,
> > whose author and message will be used for the merge commit that is about
> > to be created.
> >
> > The  parameter refers to the (possibly rewritten) revision to
> > merge. Let's see an example of a todo list:
> >
> The example ought to also note that `label onto` is to
> `# label current HEAD with a name`, seeing as this is the first occurance.
> It may be obvious in retrospect, but not at first reading.

I added some sentence to describe what `label onto` does and why.

> > label onto
> >
> > # Branch abc
> > reset onto
> 
> Is this reset strictly necessary. We are already there @head.

No, this is not strictly necessary, but

- it makes it easier to auto-generate (otherwise you would have to keep
  track of the "current HEAD" while generating that todo list, and

- if I keep the `reset onto` there, then it is *a lot* easier to reorder
  topic branches.

Ciao,
Dscho

Re: What's cooking in git.git (Apr 2018, #02; Tue, 17)

[Junio C Hamano]

Taylor Blau  writes:

> For now, the Message-ID that I was referring to is:
> 20180410001826.GB67209@syl.local. [1]

Thanks.

Re: [PATCH v1 0/5] Allocate cache entries from memory pool

[Jameson Miller]

On 04/17/2018 02:39 PM, Ben Peart wrote:



On 4/17/2018 12:34 PM, Jameson Miller wrote:


100K

Test   baseline [4] block_allocation

0002.1: read_cache/discard_cache 1 times   0.03(0.01+0.01) 
0.02(0.01+0.01) -33.3%


1M:

Test   baseline block_allocation

0002.1: read_cache/discard_cache 1 times   0.23(0.12+0.11) 
0.17(0.07+0.09) -26.1%


2M:

Test   baseline block_allocation

0002.1: read_cache/discard_cache 1 times   0.45(0.26+0.19) 
0.39(0.17+0.20) -13.3%



100K is not a large enough sample size to show the perf impact of this
change, but we can see a perf improvement with 1M and 2M entries.


I see a 33% change with 100K files which is a substantial improvement 
even in the 100K case.  I do see that the actual wall clock savings 
aren't nearly as much with a small repo as it is with the larger repos 
which makes sense.


You are correct - I should have been more careful in my wording. What I 
meant is that the wall time savings with 100K is not large, because this 
operation is already very fast.

Hi

[Flordevilla Pingkian]

 I am Flordevilla Pingkian,Please reconfirm this massage and get back
to me.via (flord_p...@usa.com)
 Regards segt Flordevilla

Re: [PATCH v3 6/9] commit: use generation numbers for in_merge_bases()

[Derrick Stolee]

On 4/18/2018 6:15 PM, Jakub Narebski wrote:

Derrick Stolee  writes:


The containment algorithm for 'git branch --contains' is different
from that for 'git tag --contains' in that it uses is_descendant_of()
instead of contains_tag_algo(). The expensive portion of the branch
algorithm is computing merge bases.

When a commit-graph file exists with generation numbers computed,
we can avoid this merge-base calculation when the target commit has
a larger generation number than the target commits.

You have "target" twice in above paragraph; one of those should probably
be something else.


Thanks. Second "target" should be "initial".


[...]

+
+   if (commit->generation > min_generation)
+   return 0;

Why not use "return ret;" instead of "return 0;", like the rest of the
code [cryptically] does, that is:

   +if (commit->generation > min_generation)
   +return ret;


Sure.

Thanks,
-Stolee

Re: [PATCH v3 7/9] commit: add short-circuit to paint_down_to_common()

[Derrick Stolee]

On 4/18/2018 7:19 PM, Jakub Narebski wrote:

Derrick Stolee  writes:


[...]

[...], and this saves time during 'git branch --contains' queries
that would otherwise walk "around" the commit we are inspecting.

If I understand the code properly, what happens is that we can now
short-circuit if all commits that are left are lower than the target
commit.

This is because max-order priority queue is used: if the commit with
maximum generation number is below generation number of target commit,
then target commit is not reachable from any commit in the priority
queue (all of which has generation number less or equal than the commit
at head of queue, i.e. all are same level or deeper); compare what I
have written in [1]

[1]: https://public-inbox.org/git/866052dkju@gmail.com/

Do I have that right?  If so, it looks all right to me.


Yes, the priority queue needs to compare via generation number first or 
there will be errors. This is why we could not use commit time before.





For a copy of the Linux repository, where HEAD is checked out at
v4.13~100, we get the following performance improvement for
'git branch --contains' over the previous commit:

Before: 0.21s
After:  0.13s
Rel %: -38%

[...]

flags = commit->object.flags & (PARENT1 | PARENT2 | STALE);
if (flags == (PARENT1 | PARENT2)) {
if (!(commit->object.flags & RESULT)) {
@@ -876,7 +886,7 @@ static struct commit_list *merge_bases_many(struct commit 
*one, int n, struct co
return NULL;
}
  
-	list = paint_down_to_common(one, n, twos);

+   list = paint_down_to_common(one, n, twos, 0);
  
  	while (list) {

struct commit *commit = pop_commit();
@@ -943,7 +953,7 @@ static int remove_redundant(struct commit **array, int cnt)
filled_index[filled] = j;
work[filled++] = array[j];
}
-   common = paint_down_to_common(array[i], filled, work);
+   common = paint_down_to_common(array[i], filled, work, 0);
if (array[i]->object.flags & PARENT2)
redundant[i] = 1;
for (j = 0; j < filled; j++)
@@ -1067,7 +1077,7 @@ int in_merge_bases_many(struct commit *commit, int 
nr_reference, struct commit *
if (commit->generation > min_generation)
return 0;
  
-	bases = paint_down_to_common(commit, nr_reference, reference);

+   bases = paint_down_to_common(commit, nr_reference, reference, 
commit->generation);

Is it the only case where we would call paint_down_to_common() with
non-zero last parameter?  Would we always use commit->generation where
commit is the first parameter of paint_down_to_common()?

If both are true and will remain true, then in my humble opinion it is
not necessary to change the signature of this function.


We need to change the signature some way, but maybe the way I chose is 
not the best.


To elaborate: paint_down_to_common() is used for multiple purposes. The 
caller here that supplies 'commit->generation' is used only to compute 
reachability (by testing if the flag PARENT2 exists on the commit, then 
clears all flags). The other callers expect the full walk down to the 
common commits, and keeps those PARENT1, PARENT2, and STALE flags for 
future use (such as reporting merge bases). Usually the call to 
paint_down_to_common() is followed by a revision walk that only halts 
when reaching root commits or commits with both PARENT1 and PARENT2 
flags on, so always short-circuiting on generations would break the 
functionality; this is confirmed by the t5318-commit-graph.sh.


An alternative to the signature change is to add a boolean parameter 
"use_cutoff" or something, that specifies "don't walk beyond the 
commit". This may give a more of a clear description of what it will do 
with the generation value, but since we are already performing 
generation comparisons before calling paint_down_to_common() I find this 
simple enough.


Thanks,
-Stolee

Re: [PATCH v3 8/9] commit-graph: always load commit-graph information

[Derrick Stolee]

On 4/18/2018 8:02 PM, Jakub Narebski wrote:

Derrick Stolee  writes:


Most code paths load commits using lookup_commit() and then
parse_commit(). In some cases, including some branch lookups, the commit
is parsed using parse_object_buffer() which side-steps parse_commit() in
favor of parse_commit_buffer().

With generation numbers in the commit-graph, we need to ensure that any
commit that exists in the commit-graph file has its generation number
loaded.

All right, that is nice explanation of the why behind this change.


Create new load_commit_graph_info() method to fill in the information
for a commit that exists only in the commit-graph file. Call it from
parse_commit_buffer() after loading the other commit information from
the given buffer. Only fill this information when specified by the
'check_graph' parameter. This avoids duplicate work when we already
checked the graph in parse_commit_gently() or when simply checking the
buffer contents in check_commit().

Couldn't this 'check_graph' parameter be a global variable similar to
the 'commit_graph' variable?  Maybe I am not understanding it.


See the two callers at the bottom of the patch. They have different 
purposes: one needs to fill in a valid commit struct, the other needs to 
check the commit buffer is valid (then throws away the struct). They 
have different values for 'check_graph'. Also, in parse_commit_gently() 
we check parse_commit_in_graph() before we call parse_commit_buffer, so 
we do not want to repeat work; in the case of a valid commit-graph file, 
but the commit is not in the commit-graph, we would repeat our binary 
search for the same commit.





Signed-off-by: Derrick Stolee 
---
  commit-graph.c | 51 --
  commit-graph.h |  8 
  commit.c   |  7 +--
  commit.h   |  2 +-
  object.c   |  2 +-
  sha1_file.c|  2 +-
  6 files changed, 49 insertions(+), 23 deletions(-)

diff --git a/commit-graph.c b/commit-graph.c
index 688d5b1801..21e853c21a 100644
--- a/commit-graph.c
+++ b/commit-graph.c
@@ -245,13 +245,19 @@ static struct commit_list **insert_parent_or_die(struct 
commit_graph *g,
return _list_insert(c, pptr)->next;
  }
  
+static void fill_commit_graph_info(struct commit *item, struct commit_graph *g, uint32_t pos)

+{
+   const unsigned char *commit_data = g->chunk_commit_data + 
GRAPH_DATA_WIDTH * pos;
+   item->generation = get_be32(commit_data + g->hash_len + 8) >> 2;
+}
+
  static int fill_commit_in_graph(struct commit *item, struct commit_graph *g, 
uint32_t pos)
  {
uint32_t edge_value;
uint32_t *parent_data_ptr;
uint64_t date_low, date_high;
struct commit_list **pptr;
-   const unsigned char *commit_data = g->chunk_commit_data + (g->hash_len 
+ 16) * pos;
+   const unsigned char *commit_data = g->chunk_commit_data + 
GRAPH_DATA_WIDTH * pos;

I'm probably wrong, but isn't it unrelated change?


You're right. I saw this while I was in here, and there was a similar 
comment on this change in a different patch. Probably best to keep these 
cleanup things in a separate commit.



item->object.parsed = 1;
item->graph_pos = pos;
@@ -292,31 +298,40 @@ static int fill_commit_in_graph(struct commit *item, 
struct commit_graph *g, uin
return 1;
  }
  
+static int find_commit_in_graph(struct commit *item, struct commit_graph *g, uint32_t *pos)

+{
+   if (item->graph_pos != COMMIT_NOT_FROM_GRAPH) {
+   *pos = item->graph_pos;
+   return 1;
+   } else {
+   return bsearch_graph(commit_graph, &(item->object.oid), pos);
+   }
+}

All right (after the fix).


+
  int parse_commit_in_graph(struct commit *item)
  {
+   uint32_t pos;
+
+   if (item->object.parsed)
+   return 0;
if (!core_commit_graph)
return 0;
-   if (item->object.parsed)
-   return 1;

Hmmm... previously the function returned 1 if item->object.parsed, now
it returns 0 for this situation.  I don't understand this change.


The good news is that this change is unimportant (the only caller is 
parse_commit_gently() which checks item->object.parsed before calling 
parse_commit_in_graph()). I wonder why I reordered those things, anyway. 
I'll revert to simplify the patch.





-
prepare_commit_graph();
-   if (commit_graph) {
-   uint32_t pos;
-   int found;
-   if (item->graph_pos != COMMIT_NOT_FROM_GRAPH) {
-   pos = item->graph_pos;
-   found = 1;
-   } else {
-   found = bsearch_graph(commit_graph, &(item->object.oid), 
);
-   }
-
-   if (found)
-   return fill_commit_in_graph(item, commit_graph, pos);
-   }
-
+   if (commit_graph && find_commit_in_graph(item, commit_graph, ))
+   

Re: [PATCH v2 1/1] completion: load completion file for external subcommand

[SZEDER Gábor]

On Thu, Apr 19, 2018 at 9:07 PM, Florian Gamböck  wrote:
> On 2018-04-18 21:51, SZEDER Gábor wrote:
>>
>> On Tue, Apr 10, 2018 at 10:28 PM, Florian Gamböck  wrote:
>>>
>>> Adding external subcommands to Git is as easy as to put an executable
>>> file git-foo into PATH. Packaging such subcommands for a Linux distribution
>>> can be achieved by unpacking the executable into /usr/bin of the user's
>>> system. Adding system-wide completion scripts for new subcommands, however,
>>> can be a bit tricky.
>>>
>>> Since bash-completion started to use dynamical loading of completion
>>> scripts since v1.90 (preview of v2.0),
>>
>>
>> I believe the main bash-completion repository can be found at:
>>
>>  https://github.com/scop/bash-completion.git
>>
>> This repository still contains the branch 'dynamic-loading'; for the
>> record it points to 3b029892f6f9db3b7210a7f66d636be3e5ec5fa2.
>>
>> Two commits on that branch are worth mentioning:
>>
>>   20c05b43 (Load completions in separate files dynamically, get rid of
>> have()., 2011-10-12)
>>   5baebf81 (Add _xfunc for loading and calling functions on demand,
>> use it in apt-get, cvsps, rsync, and sshfs., 2011-10-13)
>
>
> Nice, thanks for the pointers!
>
>>> (...)
>>>
>>> I think the easiest method is to use a function that is defined by
>>> bash-completion v2.0+, namely __load_completion.
>>
>>
>> This is wrong, __load_completion() was introduced in cad3abfc
>> (__load_completion: New function, use in _completion_loader and _xfunc,
>> 2015-07-15), and the first release tag containg it is '2.2' from 2016-03-03.
>
>
> Dang, I thought it was introduced at the same time. Sorry for that. I guess,
> 2016 is a bit too young to take it for granted then?
>
>> The release tags '1.90' and '2.0' are from 2011-11-03 and 2012-06-17,
>> respectively.  This leaves a couple of years long hole where completions
>> were already loaded dynamically but there was no __load_completion()
>> function.
>>
>> Would it be possible to use _xfunc() instead to plug that hole?  It seems
>> the be tricky, because that function not only sources but also _calls_ the
>> completion function.
>
>
> But isn't this exactly what we want?

No, that's definitely not what we want.

The bash-completion project can get away with it, because they only
use their _xfunc() to source a file they themselves ship and to call a
completion function they know that that file contains.

We, however, would like to load a file that might not exist and to
call a function that might not be defined.  Git has a lot of plumbing
commands with neither a corresponding _git_plumbing_cmd() completion
function in our completion script nor a corresponding
'git-plumbing-cmd' file that could be sourced dynamically to provide
that function.  The same applies to any 'git-foo' command in the
user's $PATH (the user's own scripts or various git-related tools,
e.g. Git LFS).

So if someone tries e.g. 'git diff-index ' to complete files in
the current directory, then it would result in an error message like

  _git_diff_index: command not found

Furthermore, earlier versions of _xfunc(), I think until the
introduction  of __load_completion(), tried to source the file given
as parameter without checking its existence beforehand, so on whatever
LTS I happen to be currently using I would also get an error like

  bash: /usr/share/bash-completion/completions/git-diff-index: No such
file or directory

Finally, since all this is running in the user's interactive shell,
Bash will even run the 'command_not_found_handle', if it's set (and
most Linux distros do set it in their default configuration (OK, maybe
not most, but at least some of the more popular do)), which may or may
not have any suggestions, but at the very least it takes quite a while
to scan through its database.


> Lucky us, we can replace the whole
> if-fi block with a simpler:
>
>_xfunc git-$command $completion_func 2>/dev/null && return
>
> If _xfunc is not defined -- as in, bashcomp is not installed / loaded --
> then the return will not get called and the original completion will
> continue:
>
>declare -f $completion_func >/dev/null 2>/dev/null &&
>$completion_func && return
>
> Since this would be redundant, we could define a fall-back for _xfunc like
> so:
>
>declare -f _xfunc || _xfunc() {
>declare -f $completion_func >/dev/null 2>/dev/null &&
>$completion_func && return
>}
>
> This way, we retain the "old" behavior and get dynamic loading if bashcomp
> is available. The actual call to get the completions would just be _xfunc
> like in my first example above.
>
> What do you think?
>
> --
> Regards
>
> Florian

Re: [PATCH v2 0/6] Teach '--column-number' to 'git-grep(1)'

[Junio C Hamano]

Taylor Blau  writes:

>> It seems that these two used to be "even when it is configured not
>> to show linenumber, with -n it is shown and without -n it is not,
>> when the new --column-number feature forces the command to show the
>> "filename plus colon plus location info plus coon" header.  I'm
>> guessing that the reason why these got changed this round is because
>> the old way was found too defensive (perhaps nobody sets
>> grep.linenumber in .git/config in the tests that come before these)?
> ...
> Do you think that it is worth testing --column-number with and without
> grep.columnnumber as above?

I view the "git -c var.val=foo cmd" as a reasonable way to make sure
that the test is not affected by any stale state in .git/config left
behind by an earlier test that did "git config var.val bar" and then
failed to clean itself up, but I do not think it is all that
intereseting to test the inter-changeability between config and
command line option "-n" while checking its interaction with another
option e.g. "--column".

Re: [PATCH v3 5/9] ref-filter: use generation number for --contains

[Derrick Stolee]

On 4/18/2018 5:02 PM, Jakub Narebski wrote:

Here I can offer only the cursory examination, as I don't know this area
of code in question.

Derrick Stolee  writes:


A commit A can reach a commit B only if the generation number of A
is larger than the generation number of B. This condition allows
significantly short-circuiting commit-graph walks.

Use generation number for 'git tag --contains' queries.

On a copy of the Linux repository where HEAD is containd in v4.13
but no earlier tag, the command 'git tag --contains HEAD' had the
following peformance improvement:

Before: 0.81s
After:  0.04s
Rel %:  -95%

A question: what is the performance after if the "commit-graph" feature
is disabled, or there is no commit-graph file?  Is there performance
regression in this case, or is the difference negligible?


Negligible, since we are adding a small number of integer comparisons 
and the main cost is in commit parsing. More on commit parsing in 
response to your comments below.





Helped-by: Jeff King 
Signed-off-by: Derrick Stolee 
---
  ref-filter.c | 23 +++
  1 file changed, 19 insertions(+), 4 deletions(-)

diff --git a/ref-filter.c b/ref-filter.c
index cffd8bf3ce..e2fea6d635 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -1587,7 +1587,8 @@ static int in_commit_list(const struct commit_list *want, 
struct commit *c)
  /*
   * Test whether the candidate or one of its parents is contained in the list.

  ^

Sidenote: when examining the code after the change, I have noticed that
the above part of commit header for the comtains_test() function is no
longer entirely correct, as the function only checks the candidate
commit, and in no place it access its parents.

But that is not your problem.


I'll add a commit in the next version that fixes this comment before I 
make any changes to the method.





   * Do not recurse to find out, though, but return -1 if inconclusive.
   */
  static enum contains_result contains_test(struct commit *candidate,
  const struct commit_list *want,
- struct contains_cache *cache)
+ struct contains_cache *cache,
+ uint32_t cutoff)
  {
enum contains_result *cached = contains_cache_at(cache, candidate);
  
@@ -1603,6 +1604,10 @@ static enum contains_result contains_test(struct commit *candidate,
  
  	/* Otherwise, we don't know; prepare to recurse */

parse_commit_or_die(candidate);
+
+   if (candidate->generation < cutoff)
+   return CONTAINS_NO;
+

Looks good to me.

The only [minor] question may be whether to define separate type for
generation numbers, and whether to future proof the tests - though the
latter would be almost certainly overengineering, and the former
probablt too.


If we have multiple notions of generation, then we can refactor all 
references to the "generation" member.





return CONTAINS_UNKNOWN;
  }
  
@@ -1618,8 +1623,18 @@ static enum contains_result contains_tag_algo(struct commit *candidate,

  struct contains_cache *cache)
  {
struct contains_stack contains_stack = { 0, 0, NULL };
-   enum contains_result result = contains_test(candidate, want, cache);
+   enum contains_result result;
+   uint32_t cutoff = GENERATION_NUMBER_INFINITY;
+   const struct commit_list *p;
+
+   for (p = want; p; p = p->next) {
+   struct commit *c = p->item;
+   parse_commit_or_die(c);
+   if (c->generation < cutoff)
+   cutoff = c->generation;
+   }

Sholdn't the above be made conditional on the ability to get generation
numbers from the commit-graph file (feature is turned on and file
exists)?  Otherwise here after the change contains_tag_algo() now parses
each commit in 'want', which I think was not done previously.

With commit-graph file parsing is [probably] cheap.  Without it, not
necessary.

But I might be worrying about nothing.


Not nothing. This parses the "wants" when we previously did not parse 
the wants. Further: this parsing happens before we do the simple check 
of comparing the OID of the candidate against the wants.


The question is: are these parsed commits significant compared to the 
walk that will parse many more commits? It is certainly possible.


One way to fix this is to call 'prepare_commit_graph()' directly and 
then test that 'commit_graph' is non-null before performing any parses. 
I'm not thrilled with how that couples the commit-graph implementation 
to this feature, but that may be necessary to avoid regressions in the 
non-commit-graph case.




  
+	result = contains_test(candidate, want, cache, cutoff);

Other than the question about possible performace regression if

Re: [PATCH 0/6] Compute and consume generation numbers

[Derrick Stolee]

On 4/21/2018 4:44 PM, Jakub Narebski wrote:

Jakub Narebski  writes:

Derrick Stolee  writes:

On 4/11/2018 3:32 PM, Jakub Narebski wrote:

What would you suggest as a good test that could imply performance? The
Google Colab notebook linked to above includes a function to count
number of commits (nodes / vertices in the commit graph) walked,
currently in the worst case scenario.

The two main questions to consider are:

1. Can X reach Y?

That is easy to do.  The function generic_is_reachable() does
that... though using direct translation of the pseudocode for
"Algorithm 3: Reachable" from FELINE paper, which is recursive and
doesn't check if vertex was already visited was not good idea for large
graphs such as Linux kernel commit graph, oops.  That is why
generic_is_reachable_large() was created.

[...]


And the thing to measure is a commit count. If possible, it would be
good to count commits walked (commits whose parent list is enumerated)
and commits inspected (commits that were listed as a parent of some
walked commit). Walked commits require a commit parse -- albeit from
the commit-graph instead of the ODB now -- while inspected commits
only check the in-memory cache.

[...]

For git.git and Linux, I like to use the release tags as tests. They
provide a realistic view of the linear history, and maintenance
releases have their own history from the major releases.

Hmmm... testing for v4.9-rc5..v4.9 in Linux kernel commit graphs, the
FELINE index does not bring any improvements over using just level
(generation number) filter.  But that may be caused by narrowing od
commit DAG around releases.

I try do do the same between commits in wide part, with many commits
with the same level (same generation number) both for source and for
target commit.  Though this may be unfair to level filter, though...


Note however that FELINE index is not unabiguous, like generation
numbers are (modulo decision whether to start at 0 or at 1); it depends
on the topological ordering chosen for the X elements.

One can now test reachability on git.git repository; there is a form
where one can plug source and destination revisions at
https://colab.research.google.com/drive/1V-U7_slu5Z3s5iEEMFKhLXtaxSu5xyzg#scrollTo=svNUnSA9O_NK=2=1

I have tried the case that is quite unfair to the generation numbers
filter, namely the check between one of recent tags, and one commit that
shares generation number among largest number of other commits.

Here level = generation number-1 (as it starts at 0 for root commit, not
1).

The results are:
  * src = 468165c1d = v2.17.0
  * dst = 66d2e04ec = v2.0.5-5-g66d2e04ec

  * 468165c1d has level 18418 which it shares with 6 commits
  * 66d2e04ec has level 14776 which it shares with 93 commits
  * gen(468165c1d) - gen(66d2e04ec) = 3642

  algorithm  | access  | walk   | maxdepth | visited | level-f  | FELINE-f  |
  ---+-++--+-+--+---+
  naive  | 48865   | 39599  | 244  | 9200|  |   |
  level  |  3086   |  2492  | 113  |  528| 285  |   |
  FELINE |   283   |   216  |  68  |0|  |  25   |
  lev+FELINE |   282   |   215  |  68  |0|   5  |  24   |
  ---+-++--+-+--+---+
  lev+FEL+mpi|79   |59  |  21  |0|   0  |   0   |

Here we have:
* 'naive' implementation means simple DFS walk, without any filters (cut-offs)
* 'level' means using levels / generation numbers based negative-cut filter
* 'FELINE' means using FELINE index based negative-cut filter
* 'lev+FELINE' means combining generation numbers filter with FELINE filter
* 'mpi' means min-post [smanning-tree] intervals for positive-cut filter;
   note that the code does not walk the path after cut, but it is easy to do

The stats have the following meaning:
* 'access' means accessing the node
* 'walk' is actual walking the node
* 'maxdepth' is maximum depth of the stack used for DFS
* 'level-f' and 'FELINE-f' is number of times levels filter or FELINE filter
   were used for negative-cut; note that those are not disjoint; node can
   be rejected by both level filter and FELINE filter

For v2.17.0 and v2.17.0-rc2 the numbers are much less in FELINE favor:
the results are the same, with 5 commits accessed and 6 walked compared
to 61574 accessed in naive algorithm.

The git.git commit graph has 53128 nodes and 66124 edges, 4 tips / heads
(different child-less commits) and 9 roots, and has average clustering
coefficient 0.000409217.


Thanks for these results. Now, write a patch. I'm sticking to generation 
numbers for my patch because of the simplified computation, but you can 
contribute a FELINE implementation.



P.S. Would it be better to move the discussion about possible extensions
to the commit-graph in the form of new chunks (topological order, FELINE
index, min-post 

Re: [PATCH v3 0/9] Compute and consume generation numbers

[Derrick Stolee]

On 4/18/2018 8:04 PM, Jakub Narebski wrote:

Derrick Stolee  writes:


-- >8 --

This is the one of several "small" patches that follow the serialized
Git commit graph patch (ds/commit-graph) and lazy-loading trees
(ds/lazy-load-trees).

As described in Documentation/technical/commit-graph.txt, the generation
number of a commit is one more than the maximum generation number among
its parents (trivially, a commit with no parents has generation number
one). This section is expanded to describe the interaction with special
generation numbers GENERATION_NUMBER_INFINITY (commits not in the commit-graph
file) and *_ZERO (commits in a commit-graph file written before generation
numbers were implemented).

This series makes the computation of generation numbers part of the
commit-graph write process.

Finally, generation numbers are used to order commits in the priority
queue in paint_down_to_common(). This allows a short-circuit mechanism
to improve performance of `git branch --contains`.

Further, use generation numbers for 'git tag --contains', providing a
significant speedup (at least 95% for some cases).

A more substantial refactoring of revision.c is required before making
'git log --graph' use generation numbers effectively.

This patch series is build on ds/lazy-load-trees.

Derrick Stolee (9):
   commit: add generation number to struct commmit

Nice and short patch. Looks good to me.


   commit-graph: compute generation numbers

Another quite easy to understand patch. LGTM.


   commit: use generations in paint_down_to_common()

Nice and short patch; minor typo in comment in code.
Otherwise it looks good to me.


   commit-graph.txt: update design document

I see that diagram got removed in this version; maybe it could be
replaced with relationship table?

Anyway, it looks good to me.


The diagrams and tables seemed to cause more confusion than clarity. I 
think the reader should create their own mental model from the 
definitions and description and we should avoid trying to make a summary.





   ref-filter: use generation number for --contains

A question: how performance looks like after the change if commit-graph
is not available?


The performance issue is minor, but will be fixed in v4.




   commit: use generation numbers for in_merge_bases()

Possible typo in the commit message, and stylistic inconsistence in
in_merge_bases() - though actually more clear than existing code.

Short, simple, and gives good performance improvenebts.


   commit: add short-circuit to paint_down_to_common()

Looks good to me; ignore [mostly] what I have written in response to the
patch in question.


   commit-graph: always load commit-graph information

Looks all right; question: parameter or one more global variable.


I responded to say that the global variable approach is incorrect. 
Parameter is important to functionality and performance.





   merge: check config before loading commits

This looks good to me.


  Documentation/technical/commit-graph.txt | 30 +--
  alloc.c  |  1 +
  builtin/merge.c  |  5 +-
  commit-graph.c   | 99 +++-
  commit-graph.h   |  8 ++
  commit.c | 54 +++--
  commit.h |  7 +-
  object.c |  2 +-
  ref-filter.c | 23 +-
  sha1_file.c  |  2 +-
  t/t5318-commit-graph.sh  |  9 +++
  11 files changed, 199 insertions(+), 41 deletions(-)


base-commit: 7b8a21dba1bce44d64bd86427d3d92437adc4707

Feature Request: option for git difftool to show changes in submodule contents

[Oshaben Nicholas]

Hello, 

A coworker of mine was trying to use git difftool in a repository with 
submodules and we found that it only shows the change in the subproject commit 
hash. The option to show the changes in the contents of the submodules exists 
for git diff. Can this be added to difftool as well?

Thank you,

Nicholas Oshaben
nicholas.osha...@bwigroup.com

Re: [PATCH v8 06/16] sequencer: introduce the `merge` command

[Phillip Wood]

On 23/04/18 13:20, Johannes Schindelin wrote:

Hi Phillip,

On Sat, 21 Apr 2018, Phillip Wood wrote:


On 21/04/18 11:33, Johannes Schindelin wrote:

This patch is part of the effort to reimplement `--preserve-merges` with
a substantially improved design, a design that has been developed in the
Git for Windows project to maintain the dozens of Windows-specific patch
series on top of upstream Git.

The previous patch implemented the `label` and `reset` commands to label
commits and to reset to labeled commits. This patch adds the `merge`
command, with the following syntax:


The two patches seem to have been fused together in this series.


Indeed. I have yet to investigate further how that happened, it could be a
bug in my series after all.


If the reset command fails because it would overwrite untracked files it
says

error: Untracked working tree file 'b' would be overwritten by merge.

Followed by the hint to edit the todo file. Saying 'merge' rather 'reset' is
possibly confusing to users. Perhaps it could call
setup_unpack_trees_porcelain(), though that would need to be extended to
handle 'reset'.


Yes, and it changes global state :-(

Maybe we can leave it as-is for now? After all, it should be clear to the
user what is happening. The most important part is the "Untracked working
tree file"...


I'm fine with leaving it, I've might get round to doing a small series 
to clean things up slightly in a few weeks. At the moment 
setup_unpack_trees_porcelain() leaks memory as it is called for each 
merge and allocates new strings each time. It would also be nice if the 
error messages reflected the command, so it said 'cherry-pick', 'revert' 
or 'reset' rather than 'merge'



Also it currently refuses to overwrite ignored files which is either
annoying or safe depending on one's point of view.


Let me think about that. My gut feeling says: if it is easy to do, then
let's nuke ignored files, but keep untracked files. But I do not think
that the unpack-trees machinery was taught to know about .gitignore...

Seeing as `label` and `reset` really are mostly about revisions we see
along the lines, I think that the common case will *not* overwrite any
untracked files, ever. You would have to use `reset` on a
not-previously-seen commit and/or add `exec` commands designed to
interfere with the `reset`.

So I tend to want to not bother with discerning between untracked and
ignored files here.


I don't think it's a pressing concern. In the past I once had a patch 
series that removed some tracked files in favor of having the build 
system generate them and added them to .gitignore. Each time I rebased I 
had to manually remove them at some stage which was annoying but that is 
quite a rare occurrence.


Best Wishes

Phillip



Ciao,
Dscho

Re: [PATCH v1 0/5] Allocate cache entries from memory pool

[Jameson Miller]

On 04/18/2018 12:49 AM, Junio C Hamano wrote:

Jameson Miller  writes:


This patch series improves the performance of loading indexes by
reducing the number of malloc() calls. ...

Jameson Miller (5):
   read-cache: teach refresh_cache_entry to take istate
   Add an API creating / discarding cache_entry structs
   mem-pool: fill out functionality
   Allocate cache entries from memory pools
   Add optional memory validations around cache_entry lifecyle

  apply.c|  26 +++---
  blame.c|   5 +-
  builtin/checkout.c |   8 +-
  builtin/difftool.c |   8 +-
  builtin/reset.c|   6 +-
  builtin/update-index.c |  26 +++---
  cache.h|  40 -
  git.c  |   3 +
  mem-pool.c | 136 -
  mem-pool.h |  34 
  merge-recursive.c  |   4 +-
  read-cache.c   | 229 +++--
  resolve-undo.c |   6 +-
  split-index.c  |  31 +--
  tree.c |   4 +-
  unpack-trees.c |  27 +++---
  16 files changed, 476 insertions(+), 117 deletions(-)


base-commit: cafaccae98f749ebf33495aec42ea25060de8682

I couldn't quite figure out what these five patches were based on,
even with this line.  Basing on and referring to a commit that is
not part of our published history with "base-commit" is not all that
useful to others.

My apologies - this patch series should be applied to the 'next'
branch.  It applies cleanly on top of
b46fe60e1d ("merge-fix/ps/test-chmtime-get", 2018-04-20), which
is a commit in the 'next' branch.

Offhand without applying these patches and viewing the changes in
wider contexts, one thing that makes me wonder is how the two
allocation schemes can be used with two implementations of free().
Upon add_index_entry() that replaces an index entry for an existing
path, we'd discard an entry that was originally read as part of
read_cache().  If we do that again, the second add_index_entry()
will be discading, in its call to replace_index_entry(), the entry
that was allocated by the caller of the first add_index_entry()
call.  And replace_index_entry() would not have a way to know from
which allocation the entry's memory came from.

Perhaps it is not how you are using the "transient" stuff, and from
the comment in 2/5, it is for "entries that are not meant to go into
the index", but then higher stage index entries in unpack_trees seem
to be allocated via the "transient" stuff, so I am not sure what the
plans are for things like merge_recursive() that uses unpack_trees()
to prepare the index and then later "fix it up" by further futzing
around the index to adjust for renames it finds, etc.


Good points. The intention with this logic is that any entries
that *could* go into an index are allocated from the memory
pool. The "transient" entries only exist for a short period of
time. These have a defined lifetime and we can always trace the
corresponding "free" call. make_transient_cache_entry() is only
used to construct a temporary cache_entry to pass to the
checkout_entry() / write_entry(). There is a note in checkout.c
indicating that write_entry() needs to be re-factored to not take
a cache_entry.

The cache_entry type could gain knowledge about where it was
allocated from. This would allow us to only have a
single "free()" function, which could inspect the cache_entry to
see if it was allocated from a mem_pool (and possibly which
mem_pool) and take the appropriate action. The downside of this
approach is that cache_entry would need to gain a field to track
this information, and I *think* all of the bit field spots are
taken.


Let me read it fully once we know where these patches are to be
applied, but before that I cannot say much about them X-<.

Thanks.

Re: Is support for 10.8 dropped?

[Igor Korot]

Hi, Eric,

On Sun, Apr 22, 2018 at 12:37 AM, Eric Sunshine  wrote:
> On Sun, Apr 22, 2018 at 1:15 AM, Igor Korot  wrote:
>> MyMac:git-2.17.0 igorkorot$ cat config.mak
>> NO_GETTEXT=Yes
>> NO_OPENSSL=Yes
>>
>> MyMac:dbhandler igorkorot$ /Users/igorkorot/git-2.17.0/git pull
>> fatal: unable to access
>> 'https://github.com/oneeyeman1/dbhandler.git/': error:1407742E:SSL
>> routines:SSL23_GET_SERVER_HELLO:tlsv1 alert protocol version
>> MyMac:dbhandler igorkorot$
>
> Try re-building with OpenSSL enabled (remove NO_OPENSSL from
> config.make). You may need to build/install OpenSSL yourself to get
> this to work.

2 things:
1. Is the file name "config.mak" or "config.make"?
2. Do I have to do "make clean" or just remove the line and o "make"?

Thank you.

Re: [PATCH v1 0/5] Allocate cache entries from memory pool

[Jameson Miller]

On 04/20/2018 01:49 PM, Stefan Beller wrote:

base-commit: cafaccae98f749ebf33495aec42ea25060de8682


I couldn't quite figure out what these five patches were based on,
even with this line.  Basing on and referring to a commit that is
not part of our published history with "base-commit" is not all that
useful to others.


I'd like to second this. In the object store refactoring, I am at a point where
I'd want to migrate the memory management of {object, tree, commit, tag}.c
which currently is done in alloc.c to a memory pool, that has a dedicated
pointer to it.

So I'd either have to refactor alloc.c to take the_repository[1] or
I'd play around with the mem_pool to manage memory in the
object layer. I guess this playing around can happen with
what is at origin/jm/mem-pool, however the life cycle management
part of the third patch[2] would allow for stopping memleaks there.
So I am interested in this series as well.



Sorry about the confusion here. This patch series can be applied to the 
next branch. They apply cleanly on [3]. The lifecycle functions are 
re-introduced in [4], which we could incorporate sooner if useful. I 
didn't have a consumer for the calls in the original patch series, and 
so deferred them until there was a caller. I would be interested to 
understand how the mem_pool would fit your needs, and if it is 
sufficient or needs modification for your use cases.



[1] proof of concept in patches nearby
https://public-inbox.org/git/20180206001749.218943-31-sbel...@google.com/

[2] https://public-inbox.org/git/20180417163400.3875-5-jam...@microsoft.com/

Thanks,
Stefan



[3] b46fe60e1d ("merge-fix/ps/test-chmtime-get", 2018-04-20)

[4]
https://public-inbox.org/git/20180417163400.3875-5-jam...@microsoft.com/

Re: [PATCH v1 1/2] merge: Add merge.renames config setting

[Ben Peart]

On 4/21/2018 12:23 AM, Junio C Hamano wrote:

Elijah Newren  writes:


+merge.renames::
+   Whether and how Git detects renames.  If set to "false",
+   rename detection is disabled. If set to "true", basic rename
+   detection is enabled. This is the default.



One can already control o->detect_rename via the -Xno-renames and
-Xfind-renames options.

...
Sorry, I think I wasn't being clear.  The documentation for the config
options for e.g. diff.renameLimit, fetch.prune, log.abbrevCommit, and
merge.ff all mention the equivalent command line parameters.  Your
patch doesn't do that for merge.renames, but I think it would be
helpful if it did.


Yes, and if we are adding a new configuration, we should do so in
such a way that we do not have to come back and extend it when we
know what the command line option does and the configuration being
proposed is less capable already.


Between all the different command line options, config settings, merge 
strategies and the interactions between the diff and merge versions, I
was trying to keep things as simple and consistent as possible.  To that 
end 'merge.renames' was modeled after the existing 'diff.renames.'


I wonder if we can just add a

single configuration whose value can be "never" to pretend as if
"--Xno-renames" were given, and some similarity score like "50" to
pretend as if "--Xfind-renames=50" were given.

That is, merge.renames does not have to a simple "yes-no to control
the --Xno-renames option".  And it would of course be better to
document it.



With the existing differences in how these options are passed on the 
command line, I'm hesitant to add yet another pattern in the config 
settings that combines 'renames' and '--find-renames[=]'.


I _have_ wondered why this all isn't configured via find-renames with 
find-renames=0 meaning renames=false (instead of mapping 0 to 32K).  I 
think that could have eliminated the need for splitting rename across 
two different settings (which is what I think you are proposing above). 
I'd then want the config setting and command line option to be the same 
syntax and behavior.


Moving the existing settings to this model and updating the config and 
command line options to be consistent without breaking backwards 
compatibility is outside the intended scope of this patch.



I also had to wonder how "merge -s resolve" faired, if the project
is not interested in renamed paths at all.



To be clear, it isn't that we're not interested in detecting renamed 
files and paths.  We're just opposed to it taking an hour to figure that 
out!



Thanks.

Re: Is support for 10.8 dropped?

[Igor Korot]

Hi, Brian,

On Sun, Apr 22, 2018 at 12:49 PM, brian m. carlson
 wrote:
> On Sun, Apr 22, 2018 at 12:10:20AM -0700, Perry Hutchison wrote:
>> Eric Sunshine  wrote:
>> > On Sun, Apr 22, 2018 at 1:15 AM, Igor Korot  wrote:
>> > > MyMac:git-2.17.0 igorkorot$ cat config.mak
>> > > NO_GETTEXT=Yes
>> > > NO_OPENSSL=Yes
>> > >
>> > > MyMac:dbhandler igorkorot$ /Users/igorkorot/git-2.17.0/git pull
>> > > fatal: unable to access
>> > > 'https://github.com/oneeyeman1/dbhandler.git/': error:1407742E:SSL
>> > > routines:SSL23_GET_SERVER_HELLO:tlsv1 alert protocol version
>> > > MyMac:dbhandler igorkorot$
>> >
>> > Try re-building with OpenSSL enabled (remove NO_OPENSSL from
>> > config.make). You may need to build/install OpenSSL yourself to get
>> > this to work.
>>
>> Explanation:  "tlsv1 alert protocol version" means that the server
>> requires its clients to use a newer version of TLS than what was
>> used in the local build of git.
>
> I think the issue here is that you're using a crypto library which may
> only support TLS 1.0 on 10.8[0].  GitHub requires TLS 1.2 as of
> recently.  So this isn't really a problem with Git, so much as it's an
> incompatibility between the version of the crypto library you're using
> and GitHub.
>
> I expect that due to the PCI DSS rules prohibiting new deployment of TLS
> 1.0, you'll continue to run into this issue more and more unless you
> upgrade to an OS or crypto library that supports TLS 1.2.  As of June
> 30, TLS 1.0 will be pretty much dead on the Internet.

Yes, openSSL on OSX 10.8 is old. I will t to update it tonight.
But as a developer I'm trying to stay with whatever OS provides.
Because upgrading will actually means that I will have to recompile an re-test
everything I made, since I will be working on a brand new system.

This is however is an unfortunate event when I will have to upgrade...

Than you.

>
> [0] I surmised this from https://www.ssllabs.com/ssltest/clients.html,
> but I don't use macOS so can't speak for certain.
> --
> brian m. carlson: Houston, Texas, US
> OpenPGP: https://keybase.io/bk2204

SEC_E_BUFFER_TOO_SMALL on Windows

[Jason B. Nance]

Hello all,

We are seeing intermittent errors with Git 2.16.2.windows.1 on Windows 7 
connecting to TFS 2017 (running in a Jenkins slave process):

ERROR: Error cloning remote repo 'origin' hudson.plugins.git.GitException: 
Command "C:\tools\Git\bin\git.exe fetch --tags --progress 
https://internal-tfs-server/tfs/project/_git/repo 
+refs/heads/*:refs/remotes/origin/*" returned status code 128:
stdout: 
stderr: fatal: unable to access 
'https://internal-tfs-server/tfs/project/_git/repo/': schannel: next 
InitializeSecurityContext failed: SEC_E_BUFFER_TOO_SMALL (0x80090321) - The 
buffers supplied to a function was too small.

I found the following thread from 2015 on a cURL list that seems to be similar:

https://curl.haxx.se/mail/lib-2015-04/0136.html

However, it looks like a patch was released for that issue:

https://curl.haxx.se/mail/lib-2015-04/0152.html

Rebooting the Windows client appears to resolve the issue for a time.

Has anyone else experienced this and found a resolution or workaround?

Thank you,

j

Re: SEC_E_BUFFER_TOO_SMALL on Windows

[Beat Bolli <bbo...@ewanet.ch>]

On Mon, Apr 23, 2018 at 11:13:41AM -0500, Jason B. Nance wrote:
> Hello all,
> 
> We are seeing intermittent errors with Git 2.16.2.windows.1 on Windows
> 7 connecting to TFS 2017 (running in a Jenkins slave process):
> 
> ERROR: Error cloning remote repo 'origin'
> hudson.plugins.git.GitException: Command "C:\tools\Git\bin\git.exe
> fetch --tags --progress
> https://internal-tfs-server/tfs/project/_git/repo
> +refs/heads/*:refs/remotes/origin/*" returned status code 128:
> stdout: stderr: fatal: unable to access
> 'https://internal-tfs-server/tfs/project/_git/repo/': schannel:
> next InitializeSecurityContext failed: SEC_E_BUFFER_TOO_SMALL
> (0x80090321) - The buffers supplied to a function was too small.
> 
> I found the following thread from 2015 on a cURL list that seems to be
> similar:
> 
> https://curl.haxx.se/mail/lib-2015-04/0136.html
> 
> However, it looks like a patch was released for that issue:
> 
> https://curl.haxx.se/mail/lib-2015-04/0152.html
> 
> Rebooting the Windows client appears to resolve the issue for a time.
> 
> Has anyone else experienced this and found a resolution or workaround?

This answer seems relevant: https://stackoverflow.com/a/39217099/232775 .
The link in the answer is no longer available; the current link is
https://developer.microsoft.com/en-us/microsoft-edge/platform/issues/4906705/ .

The obvious workaround would be to retry the request, because the error
happens randomly depending on the value of a Diffie-Hellman ephemeral
key parameter.

Cheers,
Beat


signature.asc
Description: PGP signature

Re: [RFC 00/10] Make .the gitmodules file path configurable

[Jonathan Nieder]

Hi,

Antonio Ospite wrote:

> vcsh[1] uses bare git repositories and detached work-trees to manage
> *distinct* sets of configuration files directly into $HOME.

Cool!  I like the tooling you're creating for this, though keep in mind
that Git has some weaknesses as a tool for deployment.

In particular, keep in mind that when git updates a file, there is a
period of time while it is missing from the filesystem, which can be
problematic for dotfiles.

[...]
> However when multiple repositories take turns using the same directory
> as their work-tree, and more than one of them want to use submodules,
> there could still be conflicts about the '.gitmodules' file because git
> hardcodes this path.
>
> For comparison, in case of '.gitignore' a similar conflict might arise,
> but git has alternative ways to specify exclude files, so vcsh solves
> this by setting core.excludesFile for each repository and track ignored
> files somewhere else (in ~/.gitignore.d/$VCSH_REPO_NAME).

For reference:

core.excludesFile
Specifies the pathname to the file that contains
patterns to describe paths that are not meant to be
tracked, in addition to .gitignore (per-directory) and
.git/info/exclude. Defaults to
$XDG_CONFIG_HOME/git/ignore. If $XDG_CONFIG_HOME is
either not set or empty, $HOME/.config/git/ignore is
used instead. See gitignore(5).

Using this as a substitute for /.gitignore is a bit of a
hack.  It happens to work, though, so reading on. :)

[...]
> So this series proposes a mechanism to set an alternative path for the
> submodules configuration file (from now on "gitmodules file").

I am nervous about this.  I wonder if there is another way to
accomplish the goal.

One possibility would be to handle the case where .gitmodules is
excluded by a sparse checkout specification and use .gitmodules from
the index in that case.  Would that work for you?

Thanks,
Jonathan

Re: [PATCH v1 3/5] mem-pool: fill out functionality

[Jonathan Tan]

On Mon, 23 Apr 2018 13:27:09 -0400
Jameson Miller  wrote:

> > This seems overly complicated - the struct mem_pool already has a linked
> > list of pages, so couldn't you create a custom page and insert it behind
> > the current front page instead whenever you needed a large-size page?
> 
> Yes - that is another option. However, the linked list of pages includes 
> memory that *could* have space for an allocation, while the "custom" 
> region will never have left over memory that can be used for other 
> allocations. When searching pages for memory to satisfy a request, there 
> is no reason to search through the "custom" pages. There is a trade-off 
> between complexity and implementation, so I am open to suggestions.
> 
> This was discussed in [1], where it originally was implemented closer to 
> what you describe here.
> 
> > Also, when combining, there could be some wasted space on one of the
> > pages. I'm not sure if that's worth calling out, though.
> 
> Yes, we bring over the whole page. However, these pages are now 
> available for new allocations.
> 
> [1]
> https://public-inbox.org/git/xmqqk1u2k91l@gitster-ct.c.googlers.com/

Ah, I didn't realize that the plan was to search over all pages when
allocating memory from the pool, instead of only searching the last
page. This seems like a departure from the fast-import.c way, where as
far as I can tell, new_object() searches only one page. If we do plan to
do this, searching all pages doesn't seem like a good idea to me,
especially since the objects we're storing in the pool are of similar
size.

If we decide to go ahead with searching all the pages, though, the
"custom" pages should probably be another linked list instead of an
array.

Re: [PATCH v10 00/36] Add directory rename detection to git

[Elijah Newren]

Hi Junio,

On Thu, Apr 19, 2018 at 8:05 PM, Junio C Hamano  wrote:
>> On Thu, Apr 19, 2018 at 10:57 AM, Elijah Newren  wrote:
>>> This series is a reboot of the directory rename detection series that was
>>> merged to master and then reverted due to the final patch having a buggy
>>> can-skip-update check, as noted at
>>>   https://public-inbox.org/git/xmqqmuya43cs@gitster-ct.c.googlers.com/
>>> This series based on top of master.
>
> The series as-is is fine, I think, from the maintainer's point of
> view.  Thanks.

Sorry to be a pest, but now I'm unsure how I should handle the next
round.  I've got:
- two minor fixup commits that can be trivially squashed in (not yet
sent), affecting just the final few patches
- a "year" vs "years" typo in commit message of patch 32 (which is now
in pu as commit 3daa9b3eb6dd)
- an (independent-ish) unpack_trees fix (Message-ID:
20180421193736.12722-1-new...@gmail.com), possibly to be supplemented
by another fix/improvement suggested by Duy

Should I...
- send out a reroll of everything, and include the unpack_trees
fix(es) in the series?
- just resend patches 32-36 with the fixes, and renumber the patches
to include the unpack_trees stuff in the middle?
- just send the two fixup commits, ignore the minor typo, and keep the
unpack_trees fix(es) as a separate topic that we'll just want to
advance first?
- something else?

Thanks,
Elijah

Re: [RFC PATCH v10 32.5/36] unpack_trees: fix memory corruption with split_index when src != dst

[Elijah Newren]

On Mon, Apr 23, 2018 at 10:37 AM, Duy Nguyen  wrote:
>>> [1] To me the second parameter should be src_index, not dst_index.
>>> We're copying entries from _source_ index to "result" and we should
>>> also copy extensions from the source index. That line happens to work
>>> only when dst_index is the same as src_index, which is the common use
>>> case so far.
>>
>> That makes sense; this sounds like another fix that should be
>> submitted.  Did you want to submit a patch making that change?  Do you
>> want me to?
>
> I did not look careful enough to make sure it was right and submit a
> patch. But it sounds like it could be another regression if dst_index
> is now not the same as src_index (sorry I didn't look at your whole
> stories and don't if dst_index != src_index is a new thing or not). If
> dst_index is new, moving extensions from that to result index is
> basically no-op, in other words we fail to copy necessary extensions
> over.

Ah, got it, sounds like it should be included in this patch.

A quick summary for you so you don't have to review my whole series:
- All callers of unpack_trees() have dst_index == src_index, until my
  series.
- My series makes merge-recursive.c call unpack_trees() with
  dst_index != src_index (all other callsites unchanged)
- In merge-recursive.c, dst_index points to an entirely new index, so
  yeah we'd be dropping the extensions from the original src_index.

I think all the relevant parts of my series as far as this change is
concerned is the first few diff hunks at
  https://public-inbox.org/git/20180419175823.7946-34-new...@gmail.com/

Re: [PATCH v3 0/4] rebase -i: avoid stale "# This is a combination of" in commit messages

[Stefan Beller]

On Sat, Apr 21, 2018 at 12:34 AM, Johannes Schindelin
 wrote:
> Eric Sunshine pointed out that I had such a commit message in
> https://public-inbox.org/git/CAPig+cRrS0_nYJJY=o6cbov630snqhpv5qgrqdd8mw-syzn...@mail.gmail.com/
> and I went on a hunt to figure out how the heck this happened.
>
> Turns out that if there is a fixup/squash chain where the *last* command
> fails with merge conflicts, and we either --skip ahead or resolve the
> conflict to a clean tree and then --continue, our code does not do a
> final cleanup.
>
> Contrary to my initial gut feeling, this bug was not introduced by my
> rewrite in C of the core parts of rebase -i, but it looks to me as if
> that bug was with us for a very long time (at least the --skip part).
>
> The developer (read: user of rebase -i) in me says that we would want to
> fast-track this, but the author of rebase -i in me says that we should
> be cautious and cook this in `next` for a while.

I looked through the patches again and think this series is good to go.

Thanks,
Stefan

Re: [PATCH v1 3/5] mem-pool: fill out functionality

[Jameson Miller]

On 04/23/2018 01:49 PM, Jonathan Tan wrote:

On Mon, 23 Apr 2018 13:27:09 -0400
Jameson Miller  wrote:


This seems overly complicated - the struct mem_pool already has a linked
list of pages, so couldn't you create a custom page and insert it behind
the current front page instead whenever you needed a large-size page?


Yes - that is another option. However, the linked list of pages includes
memory that *could* have space for an allocation, while the "custom"
region will never have left over memory that can be used for other
allocations. When searching pages for memory to satisfy a request, there
is no reason to search through the "custom" pages. There is a trade-off
between complexity and implementation, so I am open to suggestions.

This was discussed in [1], where it originally was implemented closer to
what you describe here.


Also, when combining, there could be some wasted space on one of the
pages. I'm not sure if that's worth calling out, though.


Yes, we bring over the whole page. However, these pages are now
available for new allocations.

[1]
https://public-inbox.org/git/xmqqk1u2k91l@gitster-ct.c.googlers.com/


Ah, I didn't realize that the plan was to search over all pages when
allocating memory from the pool, instead of only searching the last
page. This seems like a departure from the fast-import.c way, where as
far as I can tell, new_object() searches only one page. If we do plan to
do this, searching all pages doesn't seem like a good idea to me,
especially since the objects we're storing in the pool are of similar
size.


I see. However, the new_object() logic in fast-import is a different 
than the logic mem_pool was abstracting, and is not covered by the 
mem_pool functionality. The behavior of searching over all pages for one 
to satisfy the request existed previously and was not changed in the 
mem_pool implementation.




If we decide to go ahead with searching all the pages, though, the
"custom" pages should probably be another linked list instead of an
array.



This is an option - I went with the current design because we only need 
pointers to a block of memory (as well tracking how large the allocation 
is for verification purposes). We don't necessarily need the extra 
overhead of a structure to track the linked list nodes, when it is 
provided by the existing array manipulation functions. I am open to 
feedback on this point, however.

Re: Is support for 10.8 dropped?

[Eric Sunshine]

On Mon, Apr 23, 2018 at 12:31 PM, Igor Korot  wrote:
> 1. Is the file name "config.mak" or "config.make"?

"config.mak"

> 2. Do I have to do "make clean" or just remove the line and o "make"?

"make clean" would not hurt.

Re: [RFC PATCH v10 32.5/36] unpack_trees: fix memory corruption with split_index when src != dst

[Elijah Newren]

Hi,

On Sun, Apr 22, 2018 at 5:38 AM, Duy Nguyen  wrote:
>>   - there's a better, more performant fix or there is some way to actually
>> share a split_index between two independent index_state objects.
>
> A cleaner way of doing this would be something to the line [1]
>
> move_index_extensions(>result, o->dst_index);
>
> near the end of this function. This could be where we compare the
> result index with the source index's shared file and see if it's worth
> keeping the shared index or not. Shared index is designed to work with
> huge index files though, any operations that go through all index
> entries will usually not be cheap. But at least it's safer.

Yeah, it looks like move_index_extensions() currently has no logic for
the split_index.  Adding it sounds to me like a patch series of its
own, and I'm keen to limit additional changes since my patch series
already broke things pretty badly once already.

>> However, with this fix, all the tests pass both normally and under
>> GIT_TEST_SPLIT_INDEX=DareISayYes.  Without this patch, when
>> GIT_TEST_SPLIT_INDEX is set, my directory rename detection series will fail
>> several tests, as reported by SZEDER.
>
> Yes, the change looks good.

Great, thanks for looking over it.

> [1] To me the second parameter should be src_index, not dst_index.
> We're copying entries from _source_ index to "result" and we should
> also copy extensions from the source index. That line happens to work
> only when dst_index is the same as src_index, which is the common use
> case so far.

That makes sense; this sounds like another fix that should be
submitted.  Did you want to submit a patch making that change?  Do you
want me to?

Elijah

Re: [PATCH v10 00/36] Add directory rename detection to git

[Elijah Newren]

On Thu, Apr 19, 2018 at 10:57 AM, Elijah Newren  wrote:
> Additional testing:
>
>   * I've re-merged all ~13k merge commits in git.git with both
> git-2.17.0 and this version of git, comparing the results to each
> other in detail.  (Including stdout & stderr, as well as the output
> of subsequent commands like `git status`, `git ls-files -s`, `git
> diff -M`, `git diff -M --staged`).  The only differences were in 23
> merges of either git-gui or gitk which involved directory renames
> (e.g. git-2.17.0's merge would result in files like 'lib/tools.tcl'
> or 'po/ru.po' instead of the expected 'git-gui/lib/tools.tcl' or
> 'gitk-git/po/ru.po')
>
>   * I'm trying to do the same with linux.git, but it looks like that will
> take nearly a week to complete...

Results after restarting[1] and throwing some big hardware at it to
get faster completion:

Out of 53288 merge commits with exactly two parents in linux.git:
  - 48491 merged identically
  - 4737 merged the same other than a few different "Auto-merging
" output lines (as expected due to patch 35/36)
  - 53 merged the same other than different "Checking out files: ..."
output (I just did a plain merge; no flags like --no-progress)
  - the remaining 7 commits had non-trivial merge differences, all
attributable to directory rename detection kicking in

So, it looks good to me.  If anyone has suggestions for other testing
to do, let me know.

[1] Restarted so it could include my unpack_trees fix (from
message-id20180421193736.12722-1-new...@gmail.com) plus a couple minor
fixup commits (fixing some testcase nits and a comment typo).

Re: [PATCH v1 0/5] Allocate cache entries from memory pool

[Jameson Miller]

On 04/20/2018 07:34 PM, Jonathan Tan wrote:

On Tue, 17 Apr 2018 16:34:39 +
Jameson Miller  wrote:


Jameson Miller (5):
   read-cache: teach refresh_cache_entry to take istate
   Add an API creating / discarding cache_entry structs
   mem-pool: fill out functionality
   Allocate cache entries from memory pools
   Add optional memory validations around cache_entry lifecyle


In this patch set, there is no enforcement that the cache entry created
by make_index_cache_entry() goes into the correct index when
add_index_entry() is invoked. (Junio described similar things, I
believe, in [1].) This might be an issue when we bring up and drop
multiple indexes, and dropping one index causes a cache entry in another
to become invalidated.


Correct - it is up to the caller here to coordinate this. The code 
should be set up so this is not a problem here. In the case of a 
split-index, the cache entries should be allocated from the memory pool 
associated with the "most common" / base index. If you found a place I 
missed or seems questionable, or have suggestions, I would be glad to 
look into it.




One solution is to store the index for which the cache entry was created
in the cache entry itself, but that does increase its size. Another is


Yes, this is an option. For this initial patch series, I decided to not 
add extra fields to the cache_entry type, but I think incorporating this 
in cache_entry is a viable option, and has some positive properties.



to change the API such that a cache entry is created and added in the
same function, and then have some rollback if the cache entry turns out
to be invalid (to support add-empty-entry -> fill -> verify), but I
don't know if this is feasible. Anyway, all these alternatives should be
at least discussed in the commit message, I think.


I can include a discussion of these in the commit message. Thanks.



The make_transient_cache_entry() function might be poorly named, since
as far as I can tell, the entries produced by that function are actually
the longest lasting, since they are never freed.


They should always be freed (and are usually freed close to where they 
are allocated, or by the calling function). If you see an instance where 
this is not the case, please point it out, because that is not the 
intention.




Along those lines, I was slightly surprised to find out in patch 4 that
cache entry freeing is a no-op. That's fine, but in that case, it would
be better to delete all the calls to the "discard" function, and
document in the others that the entries they create will only be freed
when the memory pool itself is discarded.


I can add a comment inside the function body. In the next commit, I do 
add logic to perform extra (optional) verification in the discard 
function. I did wrestle with this fact, but I feel there is value in 
having the locations where it is appropriate to free these entries in 
code, even if this particular implementation is not utilizing it right 
now. Hopefully the verification logic added in 5/5 is enough to justify 
keeping this function around.




[1] https://public-inbox.org/git/xmqqwox5i0f7@gitster-ct.c.googlers.com/

Re: [PATCH v1 0/5] Allocate cache entries from memory pool

[Stefan Beller]

On Mon, Apr 23, 2018 at 9:44 AM, Jameson Miller
 wrote:
> I would be interested to understand how the
> mem_pool would fit your needs, and if it is sufficient or needs modification
> for your use cases.
>
>> [1] proof of concept in patches nearby
>> https://public-inbox.org/git/20180206001749.218943-31-sbel...@google.com/
>>

Currenlty the parsed objects are loaded into memory and never freed.
See alloc.c which implements a specialized memory allocator for this
object loading.
When working with submodules, their objects are also just put into this
globally-namespaced object store. (See struct object **obj_hash in
object.c)

I want to make the object store a per-repository object, such that
when working with submodules, you can free up all submodule objects
once you are done with a given submodule.

To do so, the memory allocation needs to manage the whole life cycle,
while preserving the efficiency of alloc.c. See 855419f764
(Add specialized object allocator, 2006-06-19). The mem-pool that
you propose can allocate large slabs of memory and we can put
objects in there without alignment overhead, such that we preserve
the memory efficiency while being able to track all the memory.

So I would think it is sufficient as-is with this series, maybe we need
a little tweaking there, but nothing large IMHO.

Thanks,
Stefan

Re: [PATCH v1 3/5] mem-pool: fill out functionality

[Jameson Miller]

On 04/20/2018 07:21 PM, Jonathan Tan wrote:

On Tue, 17 Apr 2018 16:34:42 +
Jameson Miller  wrote:


@@ -19,8 +19,27 @@ struct mem_pool {
  
  	/* The total amount of memory allocated by the pool. */

size_t pool_alloc;
+
+   /*
+* Array of pointers to "custom size" memory allocations.
+* This is used for "large" memory allocations.
+* The *_end variables are used to track the range of memory
+* allocated.
+*/
+   void **custom, **custom_end;
+   int nr, nr_end, alloc, alloc_end;


This seems overly complicated - the struct mem_pool already has a linked
list of pages, so couldn't you create a custom page and insert it behind
the current front page instead whenever you needed a large-size page?


Yes - that is another option. However, the linked list of pages includes 
memory that *could* have space for an allocation, while the "custom" 
region will never have left over memory that can be used for other 
allocations. When searching pages for memory to satisfy a request, there 
is no reason to search through the "custom" pages. There is a trade-off 
between complexity and implementation, so I am open to suggestions.


This was discussed in [1], where it originally was implemented closer to 
what you describe here.




Also, when combining, there could be some wasted space on one of the
pages. I'm not sure if that's worth calling out, though.



Yes, we bring over the whole page. However, these pages are now 
available for new allocations.


[1]
https://public-inbox.org/git/xmqqk1u2k91l@gitster-ct.c.googlers.com/

Re: [PATCH v2 1/1] completion: load completion file for external subcommand

[Florian Gamböck]

On 2018-04-23 17:12, SZEDER Gábor wrote:
On Thu, Apr 19, 2018 at 9:07 PM, Florian Gamböck  
wrote:

On 2018-04-18 21:51, SZEDER Gábor wrote:
Would it be possible to use _xfunc() instead to plug that hole?  It 
seems the be tricky, because that function not only sources but also 
_calls_ the completion function.


But isn't this exactly what we want?


No, that's definitely not what we want.

The bash-completion project can get away with it, because they only 
use their _xfunc() to source a file they themselves ship and to call a 
completion function they know that that file contains.


We, however, would like to load a file that might not exist and to 
call a function that might not be defined.  Git has a lot of plumbing 
commands with neither a corresponding _git_plumbing_cmd() completion 
function in our completion script nor a corresponding 
'git-plumbing-cmd' file that could be sourced dynamically to provide 
that function.  The same applies to any 'git-foo' command in the 
user's $PATH (the user's own scripts or various git-related tools, 
e.g. Git LFS).


So if someone tries e.g. 'git diff-index ' to complete files in 
the current directory, then it would result in an error message like


 _git_diff_index: command not found

Furthermore, earlier versions of _xfunc(), I think until the 
introduction  of __load_completion(), tried to source the file given 
as parameter without checking its existence beforehand, so on whatever 
LTS I happen to be currently using I would also get an error like


 bash: /usr/share/bash-completion/completions/git-diff-index: No such 
 file or directory


Finally, since all this is running in the user's interactive shell, 
Bash will even run the 'command_not_found_handle', if it's set (and 
most Linux distros do set it in their default configuration (OK, maybe 
not most, but at least some of the more popular do)), which may or may 
not have any suggestions, but at the very least it takes quite a while 
to scan through its database.


You're right, this could be a problem.

Then how about the following patch? This is one of my very first 
iterations of this patch, before even sending it to the mailing list. 
Actually this is similar to what the original _xfunc did, plus existence 
checking and minus premature function calling. In the directory of the 
git completion script, if there is an appropriate script for the current 
subcommand, if it is a regular file (or a valid symlink) and if it is 
readable, then source it and test the existence of the completion 
function again. Likewise for a possible alias.


-- >8 --
diff --git a/contrib/completion/git-completion.bash 
b/contrib/completion/git-completion.bash
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -2615,10 +2615,21 @@ __git_main ()
local completion_func="_git_${command//-/_}"
declare -f $completion_func >/dev/null && $completion_func && return

+   local completion_dir="$(dirname ${BASH_SOURCE[0]})"
+   local completion_file="$completion_dir/git-$command"
+   [ -f "$completion_file" -a -r "$completion_file" ] && . 
"$completion_file"
+   declare -f $completion_func >/dev/null && $completion_func && return
+
local expansion=$(__git_aliased_command "$command")
if [ -n "$expansion" ]; then
words[1]=$expansion
+
completion_func="_git_${expansion//-/_}"
+   declare -f $completion_func >/dev/null && $completion_func && 
return
+
+   completion_file="$completion_dir/git-$expansion"
+   [ -f "$completion_file" -a -r "$completion_file" ] &&
+   . "$completion_file"
declare -f $completion_func >/dev/null && $completion_func
fi
}

Re: [RFC PATCH v10 32.5/36] unpack_trees: fix memory corruption with split_index when src != dst

[Duy Nguyen]

On Mon, Apr 23, 2018 at 7:09 PM, Elijah Newren  wrote:
> Hi,
>
> On Sun, Apr 22, 2018 at 5:38 AM, Duy Nguyen  wrote:
>>>   - there's a better, more performant fix or there is some way to actually
>>> share a split_index between two independent index_state objects.
>>
>> A cleaner way of doing this would be something to the line [1]
>>
>> move_index_extensions(>result, o->dst_index);
>>
>> near the end of this function. This could be where we compare the
>> result index with the source index's shared file and see if it's worth
>> keeping the shared index or not. Shared index is designed to work with
>> huge index files though, any operations that go through all index
>> entries will usually not be cheap. But at least it's safer.
>
> Yeah, it looks like move_index_extensions() currently has no logic for
> the split_index.  Adding it sounds to me like a patch series of its
> own, and I'm keen to limit additional changes since my patch series
> already broke things pretty badly once already.

Oh I'm not suggesting that you do it. I was simply pointing out
something I saw while I looked at this patch and surrounding area. And
it's definitely should be done separately (by whoever) since merge
logic is quite twisted as I understand it (then top it off with rename
logic)

>> [1] To me the second parameter should be src_index, not dst_index.
>> We're copying entries from _source_ index to "result" and we should
>> also copy extensions from the source index. That line happens to work
>> only when dst_index is the same as src_index, which is the common use
>> case so far.
>
> That makes sense; this sounds like another fix that should be
> submitted.  Did you want to submit a patch making that change?  Do you
> want me to?

I did not look careful enough to make sure it was right and submit a
patch. But it sounds like it could be another regression if dst_index
is now not the same as src_index (sorry I didn't look at your whole
stories and don't if dst_index != src_index is a new thing or not). If
dst_index is new, moving extensions from that to result index is
basically no-op, in other words we fail to copy necessary extensions
over.
-- 
Duy

[PATCH 1/2] Makefile: remove unused @@PERLLIBDIR@@ substitution variable

[Jonathan Nieder]

Junio noticed that this variable is not quoted correctly when it is
passed to sed.  As a shell-quoted string, it should be inside
single-quotes like $(perllibdir_relative_SQ), not outside them like
$INSTLIBDIR.

In fact, this substitution variable is not used.  Simplify by removing
it.

Reported-by: Junio C Hamano 
Signed-off-by: Jonathan Nieder 
---
An unrelated cleanup noticed while looking over this code.

 Makefile | 1 -
 1 file changed, 1 deletion(-)

diff --git a/Makefile b/Makefile
index 154929f1c8..8f4cb506ff 100644
--- a/Makefile
+++ b/Makefile
@@ -2109,7 +2109,6 @@ GIT-PERL-HEADER: $(PERL_HEADER_TEMPLATE) GIT-PERL-DEFINES 
Makefile
INSTLIBDIR="$$INSTLIBDIR$${INSTLIBDIR_EXTRA:+:$$INSTLIBDIR_EXTRA}" && \
sed -e 's=@@PATHSEP@@=$(pathsep)=g' \
-e 's=@@INSTLIBDIR@@='$$INSTLIBDIR'=g' \
-   -e 's=@@PERLLIBDIR@@='$(perllibdir_SQ)'=g' \
-e 's=@@PERLLIBDIR_REL@@=$(perllibdir_relative_SQ)=g' \
-e 's=@@GITEXECDIR_REL@@=$(gitexecdir_relative_SQ)=g' \
-e 's=@@LOCALEDIR_REL@@=$(localedir_relative_SQ)=g' \
-- 
2.17.0.441.gb46fe60e1d

[PATCH 7/9] packfile: add repository argument to unpack_entry

[Stefan Beller]

Add a repository argument to allow the callers of unpack_entry
to be more specific about which repository to act on. This is a small
mechanical change; it doesn't change the implementation to handle
repositories other than the_repository yet.

As with the previous commits, use a macro to catch callers passing a
repository other than the_repository at compile time.

Signed-off-by: Jonathan Nieder 
Signed-off-by: Stefan Beller 
---
 fast-import.c | 2 +-
 pack-check.c  | 3 ++-
 packfile.c| 7 ---
 packfile.h| 3 ++-
 4 files changed, 9 insertions(+), 6 deletions(-)

diff --git a/fast-import.c b/fast-import.c
index afe06bd7c1..b009353e93 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -1376,7 +1376,7 @@ static void *gfi_unpack_entry(
 */
p->pack_size = pack_size + the_hash_algo->rawsz;
}
-   return unpack_entry(p, oe->idx.offset, , sizep);
+   return unpack_entry(the_repository, p, oe->idx.offset, , sizep);
 }
 
 static const char *get_mode(const char *str, uint16_t *modep)
diff --git a/pack-check.c b/pack-check.c
index 385d964bdd..d3a57df34f 100644
--- a/pack-check.c
+++ b/pack-check.c
@@ -1,4 +1,5 @@
 #include "cache.h"
+#include "repository.h"
 #include "pack.h"
 #include "pack-revindex.h"
 #include "progress.h"
@@ -134,7 +135,7 @@ static int verify_packfile(struct packed_git *p,
data = NULL;
data_valid = 0;
} else {
-   data = unpack_entry(p, entries[i].offset, , );
+   data = unpack_entry(the_repository, p, 
entries[i].offset, , );
data_valid = 1;
}
 
diff --git a/packfile.c b/packfile.c
index 2876e04bb1..d5ac48ef18 100644
--- a/packfile.c
+++ b/packfile.c
@@ -1279,7 +1279,7 @@ static void *cache_or_unpack_entry(struct packed_git *p, 
off_t base_offset,
 
ent = get_delta_base_cache_entry(p, base_offset);
if (!ent)
-   return unpack_entry(p, base_offset, type, base_size);
+   return unpack_entry(the_repository, p, base_offset, type, 
base_size);
 
if (type)
*type = ent->type;
@@ -1485,8 +1485,9 @@ static void *read_object_the_repository(const struct 
object_id *oid,
return content;
 }
 
-void *unpack_entry(struct packed_git *p, off_t obj_offset,
-  enum object_type *final_type, unsigned long *final_size)
+void *unpack_entry_the_repository(struct packed_git *p, off_t obj_offset,
+ enum object_type *final_type,
+ unsigned long *final_size)
 {
struct pack_window *w_curs = NULL;
off_t curpos = obj_offset;
diff --git a/packfile.h b/packfile.h
index bc8d840b1b..1efa57a90e 100644
--- a/packfile.h
+++ b/packfile.h
@@ -115,7 +115,8 @@ extern off_t nth_packed_object_offset(const struct 
packed_git *, uint32_t n);
 extern off_t find_pack_entry_one(const unsigned char *sha1, struct packed_git 
*);
 
 extern int is_pack_valid(struct packed_git *);
-extern void *unpack_entry(struct packed_git *, off_t, enum object_type *, 
unsigned long *);
+#define unpack_entry(r, p, of, ot, s) unpack_entry_##r(p, of, ot, s)
+extern void *unpack_entry_the_repository(struct packed_git *, off_t, enum 
object_type *, unsigned long *);
 extern unsigned long unpack_object_header_buffer(const unsigned char *buf, 
unsigned long len, enum object_type *type, unsigned long *sizep);
 extern unsigned long get_size_from_delta(struct packed_git *, struct 
pack_window **, off_t);
 extern int unpack_object_header(struct packed_git *, struct pack_window **, 
off_t *, unsigned long *);
-- 
2.17.0.484.g0c8726318c-goog

[PATCH 9/9] cache.h: allow sha1_object_info to handle arbitrary repositories

[Stefan Beller]

This involves also adapting sha1_object_info_extended and a some
internal functions that are used to implement these. It all has to
happen in one patch, because of a single recursive chain of calls visits
all these functions.

Signed-off-by: Jonathan Nieder 
Signed-off-by: Stefan Beller 
---
 cache.h |  9 -
 packfile.c  | 58 ++---
 packfile.h  |  8 
 sha1_file.c | 25 +--
 4 files changed, 50 insertions(+), 50 deletions(-)

diff --git a/cache.h b/cache.h
index 6340b2c572..3a4d80e92b 100644
--- a/cache.h
+++ b/cache.h
@@ -1192,8 +1192,7 @@ static inline void *read_object_file(const struct 
object_id *oid, enum object_ty
 }
 
 /* Read and unpack an object file into memory, write memory to an object file 
*/
-#define oid_object_info(r, o, f) oid_object_info_##r(o, f)
-int oid_object_info_the_repository(const struct object_id *, unsigned long *);
+int oid_object_info(struct repository *r, const struct object_id *, unsigned 
long *);
 
 extern int hash_object_file(const void *buf, unsigned long len,
const char *type, struct object_id *oid);
@@ -1675,9 +1674,9 @@ struct object_info {
 /* Do not check loose object */
 #define OBJECT_INFO_IGNORE_LOOSE 16
 
-#define oid_object_info_extended(r, oid, oi, flags) \
-   oid_object_info_extended_##r(oid, oi, flags)
-int oid_object_info_extended_the_repository(const struct object_id *, struct 
object_info *, unsigned flags);
+int oid_object_info_extended(struct repository *r,
+const struct object_id *,
+struct object_info *, unsigned flags);
 
 /*
  * Set this to 0 to prevent sha1_object_info_extended() from fetching missing
diff --git a/packfile.c b/packfile.c
index 8de87f904b..55d383ed0a 100644
--- a/packfile.c
+++ b/packfile.c
@@ -1104,9 +1104,9 @@ static const unsigned char *get_delta_base_sha1(struct 
packed_git *p,
return NULL;
 }
 
-#define retry_bad_packed_offset(r, p, o) \
-   retry_bad_packed_offset_##r(p, o)
-static int retry_bad_packed_offset_the_repository(struct packed_git *p, off_t 
obj_offset)
+static int retry_bad_packed_offset(struct repository *r,
+  struct packed_git *p,
+  off_t obj_offset)
 {
int type;
struct revindex_entry *revidx;
@@ -1116,7 +1116,7 @@ static int retry_bad_packed_offset_the_repository(struct 
packed_git *p, off_t ob
return OBJ_BAD;
nth_packed_object_oid(, p, revidx->nr);
mark_bad_packed_object(p, oid.hash);
-   type = oid_object_info(the_repository, , NULL);
+   type = oid_object_info(r, , NULL);
if (type <= OBJ_NONE)
return OBJ_BAD;
return type;
@@ -1124,13 +1124,12 @@ static int 
retry_bad_packed_offset_the_repository(struct packed_git *p, off_t ob
 
 #define POI_STACK_PREALLOC 64
 
-#define packed_to_object_type(r, p, o, t, w, c) \
-   packed_to_object_type_##r(p, o, t, w, c)
-static enum object_type packed_to_object_type_the_repository(struct packed_git 
*p,
-off_t obj_offset,
-enum object_type 
type,
-struct pack_window 
**w_curs,
-off_t curpos)
+static enum object_type packed_to_object_type(struct repository *r,
+ struct packed_git *p,
+ off_t obj_offset,
+ enum object_type type,
+ struct pack_window **w_curs,
+ off_t curpos)
 {
off_t small_poi_stack[POI_STACK_PREALLOC];
off_t *poi_stack = small_poi_stack;
@@ -1157,7 +1156,7 @@ static enum object_type 
packed_to_object_type_the_repository(struct packed_git *
if (type <= OBJ_NONE) {
/* If getting the base itself fails, we first
 * retry the base, otherwise unwind */
-   type = retry_bad_packed_offset(the_repository, p, 
base_offset);
+   type = retry_bad_packed_offset(r, p, base_offset);
if (type > OBJ_NONE)
goto out;
goto unwind;
@@ -1185,7 +1184,7 @@ static enum object_type 
packed_to_object_type_the_repository(struct packed_git *
 unwind:
while (poi_stack_nr) {
obj_offset = poi_stack[--poi_stack_nr];
-   type = retry_bad_packed_offset(the_repository, p, obj_offset);
+   type = retry_bad_packed_offset(r, p, obj_offset);
if (type > OBJ_NONE)
goto out;
   

[PATCH 4/9] packfile: add repository argument to packed_to_object_type

[Stefan Beller]

Add a repository argument to allow the callers of packed_to_object_type
to be more specific about which repository to handle. This is a small
mechanical change; it doesn't change the implementation to handle
repositories other than the_repository yet.

As with the previous commits, use a macro to catch callers passing a
repository other than the_repository at compile time.

Signed-off-by: Jonathan Nieder 
Signed-off-by: Stefan Beller 
---
 packfile.c | 16 +---
 1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/packfile.c b/packfile.c
index d2b3f3503b..3ecfba66af 100644
--- a/packfile.c
+++ b/packfile.c
@@ -1124,11 +1124,13 @@ static int 
retry_bad_packed_offset_the_repository(struct packed_git *p, off_t ob
 
 #define POI_STACK_PREALLOC 64
 
-static enum object_type packed_to_object_type(struct packed_git *p,
- off_t obj_offset,
- enum object_type type,
- struct pack_window **w_curs,
- off_t curpos)
+#define packed_to_object_type(r, p, o, t, w, c) \
+   packed_to_object_type_##r(p, o, t, w, c)
+static enum object_type packed_to_object_type_the_repository(struct packed_git 
*p,
+off_t obj_offset,
+enum object_type 
type,
+struct pack_window 
**w_curs,
+off_t curpos)
 {
off_t small_poi_stack[POI_STACK_PREALLOC];
off_t *poi_stack = small_poi_stack;
@@ -1378,8 +1380,8 @@ int packed_object_info(struct packed_git *p, off_t 
obj_offset,
 
if (oi->typep || oi->type_name) {
enum object_type ptot;
-   ptot = packed_to_object_type(p, obj_offset, type, _curs,
-curpos);
+   ptot = packed_to_object_type(the_repository, p, obj_offset,
+type, _curs, curpos);
if (oi->typep)
*oi->typep = ptot;
if (oi->type_name) {
-- 
2.17.0.484.g0c8726318c-goog

[PATCH 8/9] packfile: add repository argument to cache_or_unpack_entry

[Stefan Beller]

Add a repository argument to allow the callers of cache_or_unpack_entry
to be more specific about which repository to act on. This is a small
mechanical change; it doesn't change the implementation to handle
repositories other than the_repository yet.

As with the previous commits, use a macro to catch callers passing a
repository other than the_repository at compile time.

Signed-off-by: Jonathan Nieder 
Signed-off-by: Stefan Beller 
---
 packfile.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/packfile.c b/packfile.c
index d5ac48ef18..8de87f904b 100644
--- a/packfile.c
+++ b/packfile.c
@@ -1272,7 +1272,8 @@ static void detach_delta_base_cache_entry(struct 
delta_base_cache_entry *ent)
free(ent);
 }
 
-static void *cache_or_unpack_entry(struct packed_git *p, off_t base_offset,
+#define cache_or_unpack_entry(r, p, bo, bs, t) cache_or_unpack_entry_##r(p, 
bo, bs, t)
+static void *cache_or_unpack_entry_the_repository(struct packed_git *p, off_t 
base_offset,
unsigned long *base_size, enum object_type *type)
 {
struct delta_base_cache_entry *ent;
@@ -1346,7 +1347,7 @@ int packed_object_info_the_repository(struct packed_git 
*p, off_t obj_offset,
 * a "real" type later if the caller is interested.
 */
if (oi->contentp) {
-   *oi->contentp = cache_or_unpack_entry(p, obj_offset, oi->sizep,
+   *oi->contentp = cache_or_unpack_entry(the_repository, p, 
obj_offset, oi->sizep,
  );
if (!*oi->contentp)
type = OBJ_BAD;
-- 
2.17.0.484.g0c8726318c-goog

[PATCH 5/9] packfile: add repository argument to packed_object_info

[Stefan Beller]

From: Jonathan Nieder 

Add a repository argument to allow callers of packed_object_info to be
more specific about which repository to handle. This is a small
mechanical change; it doesn't change the implementation to handle
repositories other than the_repository yet.

As with the previous commits, use a macro to catch callers passing a
repository other than the_repository at compile time.

Signed-off-by: Jonathan Nieder 
Signed-off-by: Stefan Beller 
---
 builtin/pack-objects.c | 3 ++-
 packfile.c | 4 ++--
 packfile.h | 3 ++-
 sha1_file.c| 3 ++-
 4 files changed, 8 insertions(+), 5 deletions(-)

diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index 8d4111f748..d65eb4a947 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -1572,7 +1572,8 @@ static void drop_reused_delta(struct object_entry *entry)
 
oi.sizep = >size;
oi.typep = >type;
-   if (packed_object_info(entry->in_pack, entry->in_pack_offset, ) < 0) 
{
+   if (packed_object_info(the_repository, entry->in_pack,
+  entry->in_pack_offset, ) < 0) {
/*
 * We failed to get the info from this pack for some reason;
 * fall back to sha1_object_info, which may find another copy.
diff --git a/packfile.c b/packfile.c
index 3ecfba66af..5fa7d27d3b 100644
--- a/packfile.c
+++ b/packfile.c
@@ -1333,8 +1333,8 @@ static void add_delta_base_cache(struct packed_git *p, 
off_t base_offset,
hashmap_add(_base_cache, ent);
 }
 
-int packed_object_info(struct packed_git *p, off_t obj_offset,
-  struct object_info *oi)
+int packed_object_info_the_repository(struct packed_git *p, off_t obj_offset,
+ struct object_info *oi)
 {
struct pack_window *w_curs = NULL;
unsigned long size;
diff --git a/packfile.h b/packfile.h
index a92c0b241c..bc8d840b1b 100644
--- a/packfile.h
+++ b/packfile.h
@@ -125,7 +125,8 @@ extern void release_pack_memory(size_t);
 /* global flag to enable extra checks when accessing packed objects */
 extern int do_check_packed_object_crc;
 
-extern int packed_object_info(struct packed_git *pack, off_t offset, struct 
object_info *);
+#define packed_object_info(r, p, o, oi) packed_object_info_##r(p, o, oi)
+extern int packed_object_info_the_repository(struct packed_git *pack, off_t 
offset, struct object_info *);
 
 extern void mark_bad_packed_object(struct packed_git *p, const unsigned char 
*sha1);
 extern const struct packed_git *has_packed_and_bad(const unsigned char *sha1);
diff --git a/sha1_file.c b/sha1_file.c
index 93f25c6c6a..b292e04fd3 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -1307,7 +1307,8 @@ int oid_object_info_extended_the_repository(const struct 
object_id *oid, struct
 * information below, so return early.
 */
return 0;
-   rtype = packed_object_info(e.p, e.offset, oi);
+
+   rtype = packed_object_info(the_repository, e.p, e.offset, oi);
if (rtype < 0) {
mark_bad_packed_object(e.p, real->hash);
return oid_object_info_extended(the_repository, real, oi, 0);
-- 
2.17.0.484.g0c8726318c-goog

Re: [PATCH v10 00/36] Add directory rename detection to git

[Elijah Newren]

On Mon, Apr 23, 2018 at 4:46 PM, Junio C Hamano  wrote:
> Elijah Newren  writes:
>
>> Out of 53288 merge commits with exactly two parents in linux.git:
>>   - 48491 merged identically
>>   - 4737 merged the same other than a few different "Auto-merging
>> " output lines (as expected due to patch 35/36)
>>   - 53 merged the same other than different "Checking out files: ..."
>> output (I just did a plain merge; no flags like --no-progress)
>>   - the remaining 7 commits had non-trivial merge differences, all
>> attributable to directory rename detection kicking in
>>
>> So, it looks good to me.  If anyone has suggestions for other testing
>> to do, let me know.
>
> There must have been some merges that stopped due to conflicts among
> those 50k, and I am interested to hear how they were different.  Or
> are they included in the above numbers (e.g. among 48491 there were
> ones that stopped with conflicts, but the results these conflictted
> merge left in the working tree and the index were identical)?

They are included in the categories listed above.  What my comparison
did was for each of the 53288 commits:

1) Do the merge, capture stdout and stderr, and the exit status
2) Record output of 'git ls-files -s'
3) Record output of 'git status | grep -v detached'
4) Record contents of every untracked file (could be created e.g. due
to D/F conflicts)
5) Record contents of 'git diff -M --staged'
6) Record contents of 'git diff -M'
(all of this stuff in 1-6 is recorded into a single text file with
some nice headers to split the sections up).

7) Repeat steps 1-6 with the new version of git, but recording into a
different filename
8) Compare the two text files to see what was different between the
two merges, if anything.
(If they are different, save the files somewhere for me to look at later.)


Then after each merge, there's a bunch of cleanup to make sure things
are in a pristine state for the next merge.

Re: [PATCH v4 00/11] Deprecate .git/info/grafts

[Stefan Beller]

On Sat, Apr 21, 2018 at 2:43 AM, Johannes Schindelin
 wrote:
> It is fragile, as there is no way for the revision machinery to say "but
> now I want to traverse the graph ignoring the graft file" e.g. when
> pushing commits to a remote repository (which, as a consequence, can
> miss commits).
>
> And we already have a better solution with `git replace --graft 
> [...]`.
>

Apart from some technical questions on patch 4 [1]
this series looks good to me,.

Thanks,
Stefan

[1] 
https://public-inbox.org/git/CAGZ79ka=BLGCCTOw848m0SE9O+ZKhQfiW9RUz99W4=gdg+7...@mail.gmail.com/

Re: Git Bash Completion Not Working In Emacs

[Marko Vasic]

Thank you Andreas,

Is there a way to use git bash completion with "M-x shell"?

Marko

On Mon, Apr 16, 2018 at 11:25 AM, Andreas Schwab  wrote:
> On Apr 16 2018, Marko Vasic  wrote:
>
>> Git bash completion script works perfectly under the terminal,
>> however, it does not work in Emacs (neither shell nor gui mode). Did
>> anyone encounter this issues and how can it be solved?
>
> Depend on how you run the command in Emacs.  If you use M-x shell then
> Emacs handles special keys like TAB itself (and the started shell
> usually disables its own command line editing).  If you use M-x term
> then Emacs emulates a terminal and lets the running process handle all
> special keys.
>
> Andreas.
>
> --
> Andreas Schwab, sch...@linux-m68k.org
> GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
> "And now for something completely different."

Re: [PATCH v3 0/4] rebase -i: avoid stale "# This is a combinationof" in commit messages

[Phillip Wood]

On 23/04/18 19:11, Stefan Beller wrote:


On Sat, Apr 21, 2018 at 12:34 AM, Johannes Schindelin
 wrote:

Eric Sunshine pointed out that I had such a commit message in
https://public-inbox.org/git/CAPig+cRrS0_nYJJY=o6cbov630snqhpv5qgrqdd8mw-syzn...@mail.gmail.com/
and I went on a hunt to figure out how the heck this happened.

Turns out that if there is a fixup/squash chain where the *last* command
fails with merge conflicts, and we either --skip ahead or resolve the
conflict to a clean tree and then --continue, our code does not do a
final cleanup.

Contrary to my initial gut feeling, this bug was not introduced by my
rewrite in C of the core parts of rebase -i, but it looks to me as if
that bug was with us for a very long time (at least the --skip part).

The developer (read: user of rebase -i) in me says that we would want to
fast-track this, but the author of rebase -i in me says that we should
be cautious and cook this in `next` for a while.


I looked through the patches again and think this series is good to go.


I've just realized I commented on an outdated version as the new version 
was posted there rather than as a reply to v1. I've just looked through 
it and I'm not sure it addresses the unnecessary editing of the commit 
message of the previous commit if a single squash command is skipped as 
outlined in 
https://public-inbox.org/git/b6512eae-e214-9699-4d69-77117a0da...@talktalk.net/


Best Wishes

Phillip


Thanks,
Stefan

Re: [PATCH v1 1/2] merge: Add merge.renames config setting

[Eckhard Maaß]

On Mon, Apr 23, 2018 at 09:15:09AM -0400, Ben Peart wrote:
> In commit 2a2ac926547 when merge.renamelimit was added, it was decided to
> have separate settings for merge and diff to give users the ability to
> control that behavior.  In this particular case, it will default to the
> value of diff.renamelimit when it isn't set.  That isn't consistent with the
> other merge settings.

However, it seems like a desirable way to do it.

Maybe let me throw in some code for discussion (test and documentation
is missing, mainly to form an idea what the change in options should
be). I admit the patch below is concerned only with diff.renames, but
whatever we come up with for merge should be reflected there, too,
doesn't it?

Greetings,
Eckhard

-- >8 --

>From e8a88111f2aaf338a4c19e83251c7178f7152129 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Eckhard=20S=2E=20Maa=C3=9F?= 
Date: Sun, 22 Apr 2018 23:29:08 +0200
Subject: [PATCH] diff: enhance diff.renames to be able to set rename score
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Signed-off-by: Eckhard S. Maaß 
---
 diff.c | 35 ---
 1 file changed, 28 insertions(+), 7 deletions(-)

diff --git a/diff.c b/diff.c
index 1289df4b1f..a3cedad5cf 100644
--- a/diff.c
+++ b/diff.c
@@ -30,6 +30,7 @@
 #endif
 
 static int diff_detect_rename_default;
+static int diff_rename_score_default;
 static int diff_indent_heuristic = 1;
 static int diff_rename_limit_default = 400;
 static int diff_suppress_blank_empty;
@@ -177,13 +178,33 @@ static int parse_submodule_params(struct diff_options 
*options, const char *valu
return 0;
 }
 
+int parse_rename_score(const char **cp_p);
+
+static int git_config_rename_score(const char *value)
+{
+   int parsed_rename_score = parse_rename_score();
+   if (parsed_rename_score == -1)
+   return error("invalid argument to diff.renamescore: %s", value);
+   diff_rename_score_default = parsed_rename_score;
+   return 0;
+}
+
 static int git_config_rename(const char *var, const char *value)
 {
-   if (!value)
-   return DIFF_DETECT_RENAME;
-   if (!strcasecmp(value, "copies") || !strcasecmp(value, "copy"))
-   return  DIFF_DETECT_COPY;
-   return git_config_bool(var,value) ? DIFF_DETECT_RENAME : 0;
+   if (!value) {
+   diff_detect_rename_default = DIFF_DETECT_RENAME;
+   return 0;
+   }
+   if (skip_to_optional_arg(value, "copies", ) || 
skip_to_optional_arg(value, "copy", )) {
+   diff_detect_rename_default = DIFF_DETECT_COPY;
+   return git_config_rename_score(value);
+   }
+   if (skip_to_optional_arg(value, "renames", ) || 
skip_to_optional_arg(value, "rename", )) {
+   diff_detect_rename_default = DIFF_DETECT_RENAME;
+   return git_config_rename_score(value);
+   }
+   diff_detect_rename_default = git_config_bool(var,value) ? 
DIFF_DETECT_RENAME : 0;
+   return 0;
 }
 
 long parse_algorithm_value(const char *value)
@@ -307,8 +328,7 @@ int git_diff_ui_config(const char *var, const char *value, 
void *cb)
return 0;
}
if (!strcmp(var, "diff.renames")) {
-   diff_detect_rename_default = git_config_rename(var, value);
-   return 0;
+   return git_config_rename(var, value);
}
if (!strcmp(var, "diff.autorefreshindex")) {
diff_auto_refresh_index = git_config_bool(var, value);
@@ -4116,6 +4136,7 @@ void diff_setup(struct diff_options *options)
options->add_remove = diff_addremove;
options->use_color = diff_use_color_default;
options->detect_rename = diff_detect_rename_default;
+   options->rename_score = diff_rename_score_default;
options->xdl_opts |= diff_algorithm;
if (diff_indent_heuristic)
DIFF_XDL_SET(options, INDENT_HEURISTIC);
-- 
2.17.0.252.gfe0a9eaf31

[PATCH v8 0/4] worktree: teach "add" to check out existing branches

[Thomas Gummerer]

Thanks Eric and Junio for the review the suggestions in the last
round.

Previous rounds are at <20180121120208.12760-1-t.gumme...@gmail.com>,
<20180204221305.28300-1-t.gumme...@gmail.com>,
<20180317220830.30963-1-t.gumme...@gmail.com>,
<2018031719.4940-1-t.gumme...@gmail.com>,
<20180325134947.25828-1-t.gumme...@gmail.com>,
<20180331151804.30380-1-t.gumme...@gmail.com> and
<20180415202917.4360-1-t.gumme...@gmail.com>.

This round updates the output for "resetting branch ..." to not have
braces embedded inside of another pair of braces, and is not correctly
printing "checking out ''" when 'git worktree add 
' is used.  Both these changes are in patch 2/4, the
other patches are the same as in the previous round.

Interdiff below:

diff --git a/builtin/worktree.c b/builtin/worktree.c
index f5a5283b39..d52495f312 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -353,7 +353,8 @@ static int add_worktree(const char *path, const char 
*refname,
return ret;
 }
 
-static void print_preparing_worktree_line(const char *branch,
+static void print_preparing_worktree_line(int detach,
+ const char *branch,
  const char *new_branch,
  const char *new_branch_force,
  int checkout_existing_branch)
@@ -365,19 +366,27 @@ static void print_preparing_worktree_line(const char 
*branch,
if (!commit)
printf_ln(_("Preparing worktree (new branch '%s')"), 
new_branch_force);
else
-   printf_ln(_("Preparing worktree (resetting branch '%s' 
(was at %s))"),
+   printf_ln(_("Preparing worktree (resetting branch '%s'; 
was at %s)"),
  new_branch_force,
  find_unique_abbrev(commit->object.oid.hash,
 DEFAULT_ABBREV));
} else if (new_branch) {
printf_ln(_("Preparing worktree (new branch '%s')"), 
new_branch);
} else {
-   struct commit *commit = lookup_commit_reference_by_name(branch);
-   if (!commit)
-   die(_("invalid reference: %s"), branch);
-   printf_ln(_("Preparing worktree (detached HEAD %s)"),
- find_unique_abbrev(commit->object.oid.hash,
-DEFAULT_ABBREV));
+   struct strbuf s = STRBUF_INIT;
+   if (!detach && !strbuf_check_branch_ref(, branch) &&
+   ref_exists(s.buf))
+   printf_ln(_("Preparing worktree (checking out '%s')"),
+ branch);
+   else {
+   struct commit *commit = 
lookup_commit_reference_by_name(branch);
+   if (!commit)
+   die(_("invalid reference: %s"), branch);
+   printf_ln(_("Preparing worktree (detached HEAD %s)"),
+ find_unique_abbrev(commit->object.oid.hash,
+DEFAULT_ABBREV));
+   }
+   strbuf_release();
}
 }
 
@@ -481,7 +490,7 @@ static int add(int ac, const char **av, const char *prefix)
}
}
 
-   print_preparing_worktree_line(branch, new_branch, new_branch_force,
+   print_preparing_worktree_line(opts.detach, branch, new_branch, 
new_branch_force,
  checkout_existing_branch);
 
if (new_branch) {

Thomas Gummerer (4):
  worktree: remove extra members from struct add_opts
  worktree: improve message when creating a new worktree
  worktree: factor out dwim_branch function
  worktree: teach "add" to check out existing branches

 Documentation/git-worktree.txt |   9 +++-
 builtin/worktree.c | 111 +++--
 t/t2025-worktree-add.sh|  26 +++---
 3 files changed, 110 insertions(+), 36 deletions(-)

-- 
2.16.1.74.g7afd1c25cc.dirty

[PATCH v8 1/4] worktree: remove extra members from struct add_opts

[Thomas Gummerer]

There are two members of 'struct add_opts', which are only used inside
the 'add()' function, but being part of 'struct add_opts' they are
needlessly also passed to the 'add_worktree' function.

Make them local to the 'add()' function to make it clearer where they
are used.

Signed-off-by: Thomas Gummerer 
---
 builtin/worktree.c | 32 +++-
 1 file changed, 15 insertions(+), 17 deletions(-)

diff --git a/builtin/worktree.c b/builtin/worktree.c
index 7cef5b120b..4d96a21a45 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -27,8 +27,6 @@ struct add_opts {
int detach;
int checkout;
int keep_locked;
-   const char *new_branch;
-   int force_new_branch;
 };
 
 static int show_only;
@@ -363,10 +361,11 @@ static int add(int ac, const char **av, const char 
*prefix)
const char *new_branch_force = NULL;
char *path;
const char *branch;
+   const char *new_branch = NULL;
const char *opt_track = NULL;
struct option options[] = {
OPT__FORCE(, N_("checkout  even if already 
checked out in other worktree")),
-   OPT_STRING('b', NULL, _branch, N_("branch"),
+   OPT_STRING('b', NULL, _branch, N_("branch"),
   N_("create a new branch")),
OPT_STRING('B', NULL, _branch_force, N_("branch"),
   N_("create or reset a branch")),
@@ -384,7 +383,7 @@ static int add(int ac, const char **av, const char *prefix)
memset(, 0, sizeof(opts));
opts.checkout = 1;
ac = parse_options(ac, av, prefix, options, worktree_usage, 0);
-   if (!!opts.detach + !!opts.new_branch + !!new_branch_force > 1)
+   if (!!opts.detach + !!new_branch + !!new_branch_force > 1)
die(_("-b, -B, and --detach are mutually exclusive"));
if (ac < 1 || ac > 2)
usage_with_options(worktree_usage, options);
@@ -395,33 +394,32 @@ static int add(int ac, const char **av, const char 
*prefix)
if (!strcmp(branch, "-"))
branch = "@{-1}";
 
-   opts.force_new_branch = !!new_branch_force;
-   if (opts.force_new_branch) {
+   if (new_branch_force) {
struct strbuf symref = STRBUF_INIT;
 
-   opts.new_branch = new_branch_force;
+   new_branch = new_branch_force;
 
if (!opts.force &&
-   !strbuf_check_branch_ref(, opts.new_branch) &&
+   !strbuf_check_branch_ref(, new_branch) &&
ref_exists(symref.buf))
die_if_checked_out(symref.buf, 0);
strbuf_release();
}
 
-   if (ac < 2 && !opts.new_branch && !opts.detach) {
+   if (ac < 2 && !new_branch && !opts.detach) {
int n;
const char *s = worktree_basename(path, );
-   opts.new_branch = xstrndup(s, n);
+   new_branch = xstrndup(s, n);
if (guess_remote) {
struct object_id oid;
const char *remote =
-   unique_tracking_name(opts.new_branch, );
+   unique_tracking_name(new_branch, );
if (remote)
branch = remote;
}
}
 
-   if (ac == 2 && !opts.new_branch && !opts.detach) {
+   if (ac == 2 && !new_branch && !opts.detach) {
struct object_id oid;
struct commit *commit;
const char *remote;
@@ -430,25 +428,25 @@ static int add(int ac, const char **av, const char 
*prefix)
if (!commit) {
remote = unique_tracking_name(branch, );
if (remote) {
-   opts.new_branch = branch;
+   new_branch = branch;
branch = remote;
}
}
}
 
-   if (opts.new_branch) {
+   if (new_branch) {
struct child_process cp = CHILD_PROCESS_INIT;
cp.git_cmd = 1;
argv_array_push(, "branch");
-   if (opts.force_new_branch)
+   if (new_branch_force)
argv_array_push(, "--force");
-   argv_array_push(, opts.new_branch);
+   argv_array_push(, new_branch);
argv_array_push(, branch);
if (opt_track)
argv_array_push(, opt_track);
if (run_command())
return -1;
-   branch = opts.new_branch;
+   branch = new_branch;
} else if (opt_track) {
die(_("--[no-]track can only be used if a new branch is 
created"));
}
-- 
2.16.1.74.g7afd1c25cc.dirty

Re: Is support for 10.8 dropped?

[Igor Korot]

Eric,

On Mon, Apr 23, 2018 at 4:04 PM, Eric Sunshine  wrote:
> On Mon, Apr 23, 2018 at 3:58 PM, Totsten Bögershausen  wrote:
>> On 2018-04-23 18:53, Eric Sunshine wrote:
>>> On Mon, Apr 23, 2018 at 12:31 PM, Igor Korot  wrote:
 1. Is the file name "config.mak" or "config.make"?
>>>
>>> "config.mak"
>>
>> I am confused, I have these file in my tree:
>> config.mak.in  config.mak.uname   Makefile
>> Setting options is documentend in Makefile
>
> You can place custom build settings in a file named "config.mak" which
> you create; it's not part of the distribution. The other files are not
> intended for modification.
>
>> I would actually try to install openssl / openssl-dev (or however it is
>> called) via "mac ports" or "home brew".
>>
>> Both should work (but I don't have a system to test on)
>
> Igor indicated earlier[1] in the thread that he wanted to avoid
> Homebrew (and Macports, etc., presumably). If he'd be willing to use
> Homebrew, then easiest would be simply to install Git itself via
> Homebrew: "brew install git"

Yes.
This laptop is old and doesn't have too big of a hard drive.
And I'm trying to create a big program

Thank you.

>
> [1]: 
> https://public-inbox.org/git/CA+FnnTzfJMBuSMAD7PgUurRu8jOpirEgM6=+=i91zdglwmf...@mail.gmail.com/

Re: [PATCH v8 06/16] sequencer: introduce the `merge` command

[Philip Oakley]

From: "Johannes Schindelin"  : Monday, April 23, 
2018 1:03 PM

Subject: Re: [PATCH v8 06/16] sequencer: introduce the `merge` command



Hi Philip,


[...]



> label onto
>
> # Branch abc
> reset onto

Is this reset strictly necessary. We are already there @head.


No, this is not strictly necessary, but


I've realised my misunderstanding. I was thinking this (and others) was 
equivalent to


$  git reset  # maybe even --hard,

i.e. affecting the worktree

rather that just being a movement of the Head rev (though I may be having 
brain fade here regarding untracked files etc..)




- it makes it easier to auto-generate (otherwise you would have to keep
 track of the "current HEAD" while generating that todo list, and

- if I keep the `reset onto` there, then it is *a lot* easier to reorder
 topic branches.

Ciao,
Dscho


Thanks

Philip 

Re: [PATCH v4 04/11] replace: "libify" create_graft() and callees

[Stefan Beller]

Hi Johannes,

On Fri, Apr 20, 2018 at 3:21 PM, Johannes Schindelin
 wrote:
> @@ -250,27 +257,38 @@ static void import_object(struct object_id *oid, enum 
> object_type type,
> -   if (strbuf_read(, cmd.out, 41) < 0)
> -   die_errno("unable to read from mktree");
> +   if (strbuf_read(, cmd.out, 41) < 0) {
> +   close(fd);
> +   close(cmd.out);
> +   return error_errno("unable to read from mktree");

So before the errno is coming directly from strbuf_read,
which will set errno on error to the desired errno.
(It will come from an underlying read())

However close() may fail and clobber errno,
so I would think we'd need to

if (strbuf_read(, cmd.out, 41) < 0) {
  int err =  errno; /* close shall not clobber errno */
  close(fd);
  close(cmd.out);
  errno = err;
  return error_errno(...);
}

> -   if (fstat(fd, ) < 0)
> -   die_errno("unable to fstat %s", filename);
> +   if (fstat(fd, ) < 0) {
> +   close(fd);
> +   return error_errno("unable to fstat %s", filename);
> +   }

Same here?

An alternative would be to do
ret = error_errno(...)
close (..)
return ret;


> @@ -288,19 +307,23 @@ static int edit_and_replace(const char *object_ref, int 
> force, int raw)
> struct strbuf ref = STRBUF_INIT;
>
> if (get_oid(object_ref, _oid) < 0)
> -   die("Not a valid object name: '%s'", object_ref);
> +   return error("Not a valid object name: '%s'", object_ref);
>
> type = oid_object_info(_oid, NULL);
> if (type < 0)
> -   die("unable to get object type for %s", oid_to_hex(_oid));
> +   return error("unable to get object type for %s",
> +oid_to_hex(_oid));
>
> -   check_ref_valid(_oid, , , force);
> +   if (check_ref_valid(_oid, , , force))
> +   return -1;
> strbuf_release();
>
> -   export_object(_oid, type, raw, tmpfile);
> +   if (export_object(_oid, type, raw, tmpfile))
> +   return -1;
> if (launch_editor(tmpfile, NULL, NULL) < 0)
> -   die("editing object file failed");
> -   import_object(_oid, type, raw, tmpfile);
> +   return error("editing object file failed");
> +   if (import_object(_oid, type, raw, tmpfile))
> +   return -1;
>
> free(tmpfile);

Do we need to free tmpfile in previous returns?

> @@ -394,24 +422,29 @@ static int create_graft(int argc, const char **argv, 
> int force)
> unsigned long size;
>
> if (get_oid(old_ref, _oid) < 0)
> -   die(_("Not a valid object name: '%s'"), old_ref);
> -   commit = lookup_commit_or_die(_oid, old_ref);
> +   return error(_("Not a valid object name: '%s'"), old_ref);
> +   commit = lookup_commit_reference(_oid);
> +   if (!commit)
> +   return error(_("could not parse %s"), old_ref);
>
> buffer = get_commit_buffer(commit, );
> strbuf_add(, buffer, size);
> unuse_commit_buffer(commit, buffer);
>
> -   replace_parents(, argc - 1, [1]);
> +   if (replace_parents(, argc - 1, [1]) < 0)
> +   return -1;
>
> if (remove_signature()) {
> warning(_("the original commit '%s' has a gpg signature."), 
> old_ref);
> warning(_("the signature will be removed in the replacement 
> commit!"));
> }
>
> -   check_mergetags(commit, argc, argv);
> +   if (check_mergetags(commit, argc, argv))
> +   return -1;
>
> if (write_object_file(buf.buf, buf.len, commit_type, _oid))
> -   die(_("could not write replacement commit for: '%s'"), 
> old_ref);
> +   return error(_("could not write replacement commit for: 
> '%s'"),
> +old_ref);
>
> strbuf_release();

Release  in the other return cases, too?

Thanks,
Stefan

Re: [RFC PATCH] checkout: Force matching mtime between files

[Robin H. Johnson]

On Fri, Apr 13, 2018 at 07:01:29PM +0200, Michał Górny wrote:
> Currently git does not control mtimes of files being checked out.  This
> means that the only assumption you could make is that all files created
> or modified within a single checkout action will have mtime between
> start time and end time of this checkout.  The relations between mtimes
> of different files depend on the order in which they are checked out,
> filesystem speed and timestamp precision.
> ...
Junio: ping for review or inclusion of this patch?

-- 
Robin Hugh Johnson
Gentoo Linux: Dev, Infra Lead, Foundation Treasurer
E-Mail   : robb...@gentoo.org
GnuPG FP : 11ACBA4F 4778E3F6 E4EDF38E B27B944E 34884E85
GnuPG FP : 7D0B3CEB E9B85B1F 825BCECF EE05E6F6 A48F6136


signature.asc
Description: Digital signature

Re: Is support for 10.8 dropped?

[Eric Sunshine]

On Mon, Apr 23, 2018 at 3:58 PM, Totsten Bögershausen  wrote:
> On 2018-04-23 18:53, Eric Sunshine wrote:
>> On Mon, Apr 23, 2018 at 12:31 PM, Igor Korot  wrote:
>>> 1. Is the file name "config.mak" or "config.make"?
>>
>> "config.mak"
>
> I am confused, I have these file in my tree:
> config.mak.in  config.mak.uname   Makefile
> Setting options is documentend in Makefile

You can place custom build settings in a file named "config.mak" which
you create; it's not part of the distribution. The other files are not
intended for modification.

> I would actually try to install openssl / openssl-dev (or however it is
> called) via "mac ports" or "home brew".
>
> Both should work (but I don't have a system to test on)

Igor indicated earlier[1] in the thread that he wanted to avoid
Homebrew (and Macports, etc., presumably). If he'd be willing to use
Homebrew, then easiest would be simply to install Git itself via
Homebrew: "brew install git"

[1]: 
https://public-inbox.org/git/CA+FnnTzfJMBuSMAD7PgUurRu8jOpirEgM6=+=i91zdglwmf...@mail.gmail.com/

[PATCH v8 4/4] worktree: teach "add" to check out existing branches

[Thomas Gummerer]

Currently 'git worktree add ' creates a new branch named after the
basename of the path by default.  If a branch with that name already
exists, the command refuses to do anything, unless the '--force' option
is given.

However we can do a little better than that, and check the branch out if
it is not checked out anywhere else.  This will help users who just want
to check an existing branch out into a new worktree, and save a few
keystrokes.

As the current behaviour is to simply 'die()' when a branch with the name
of the basename of the path already exists, there are no backwards
compatibility worries here.

We will still 'die()' if the branch is checked out in another worktree,
unless the --force flag is passed.

Signed-off-by: Thomas Gummerer 
---
 Documentation/git-worktree.txt |  9 +++--
 builtin/worktree.c | 30 --
 t/t2025-worktree-add.sh| 26 +++---
 3 files changed, 50 insertions(+), 15 deletions(-)

diff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt
index 41585f535d..5d54f36a71 100644
--- a/Documentation/git-worktree.txt
+++ b/Documentation/git-worktree.txt
@@ -61,8 +61,13 @@ $ git worktree add --track -b   
/
 
 +
 If `` is omitted and neither `-b` nor `-B` nor `--detach` used,
-then, as a convenience, a new branch based at HEAD is created automatically,
-as if `-b $(basename )` was specified.
+then, as a convenience, the new worktree is associated with a branch
+(call it ``) named after `$(basename )`.  If ``
+doesn't exist, a new branch based on HEAD is automatically created as
+if `-b ` was given.  If `` does exist, it will be
+checked out in the new worktree, if it's not checked out anywhere
+else, otherwise the command will refuse to create the worktree (unless
+`--force` is used).
 
 list::
 
diff --git a/builtin/worktree.c b/builtin/worktree.c
index bd4cf4583f..d52495f312 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -356,9 +356,12 @@ static int add_worktree(const char *path, const char 
*refname,
 static void print_preparing_worktree_line(int detach,
  const char *branch,
  const char *new_branch,
- const char *new_branch_force)
+ const char *new_branch_force,
+ int checkout_existing_branch)
 {
-   if (new_branch_force) {
+   if (checkout_existing_branch) {
+   printf_ln(_("Preparing worktree (checking out '%s')"), branch);
+   } else if (new_branch_force) {
struct commit *commit = 
lookup_commit_reference_by_name(new_branch_force);
if (!commit)
printf_ln(_("Preparing worktree (new branch '%s')"), 
new_branch_force);
@@ -387,11 +390,23 @@ static void print_preparing_worktree_line(int detach,
}
 }
 
-static const char *dwim_branch(const char *path, const char **new_branch)
+static const char *dwim_branch(const char *path, const char **new_branch,
+  int *checkout_existing_branch)
 {
int n;
const char *s = worktree_basename(path, );
-   *new_branch = xstrndup(s, n);
+   const char *branchname = xstrndup(s, n);
+   struct strbuf ref = STRBUF_INIT;
+
+   if (!strbuf_check_branch_ref(, branchname) &&
+   ref_exists(ref.buf)) {
+   *checkout_existing_branch = 1;
+   strbuf_release();
+   UNLEAK(branchname);
+   return branchname;
+   }
+
+   *new_branch = branchname;
if (guess_remote) {
struct object_id oid;
const char *remote =
@@ -406,6 +421,7 @@ static int add(int ac, const char **av, const char *prefix)
struct add_opts opts;
const char *new_branch_force = NULL;
char *path;
+   int checkout_existing_branch = 0;
const char *branch;
const char *new_branch = NULL;
const char *opt_track = NULL;
@@ -453,7 +469,8 @@ static int add(int ac, const char **av, const char *prefix)
}
 
if (ac < 2 && !new_branch && !opts.detach) {
-   const char *s = dwim_branch(path, _branch);
+   const char *s = dwim_branch(path, _branch,
+   _existing_branch);
if (s)
branch = s;
}
@@ -473,7 +490,8 @@ static int add(int ac, const char **av, const char *prefix)
}
}
 
-   print_preparing_worktree_line(opts.detach, branch, new_branch, 
new_branch_force);
+   print_preparing_worktree_line(opts.detach, branch, new_branch, 
new_branch_force,
+ checkout_existing_branch);
 
if (new_branch) {
struct child_process cp = CHILD_PROCESS_INIT;
diff --git a/t/t2025-worktree-add.sh 

[PATCH v8 3/4] worktree: factor out dwim_branch function

[Thomas Gummerer]

Factor out a dwim_branch function, which takes care of the dwim'ery in
'git worktree add '.  It's not too much code currently, but we're
adding a new kind of dwim in a subsequent patch, at which point it makes
more sense to have it as a separate function.

Factor it out now to reduce the patch noise in the next patch.

Signed-off-by: Thomas Gummerer 
---
 builtin/worktree.c | 27 +--
 1 file changed, 17 insertions(+), 10 deletions(-)

diff --git a/builtin/worktree.c b/builtin/worktree.c
index d348101216..bd4cf4583f 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -387,6 +387,20 @@ static void print_preparing_worktree_line(int detach,
}
 }
 
+static const char *dwim_branch(const char *path, const char **new_branch)
+{
+   int n;
+   const char *s = worktree_basename(path, );
+   *new_branch = xstrndup(s, n);
+   if (guess_remote) {
+   struct object_id oid;
+   const char *remote =
+   unique_tracking_name(*new_branch, );
+   return remote;
+   }
+   return NULL;
+}
+
 static int add(int ac, const char **av, const char *prefix)
 {
struct add_opts opts;
@@ -439,16 +453,9 @@ static int add(int ac, const char **av, const char *prefix)
}
 
if (ac < 2 && !new_branch && !opts.detach) {
-   int n;
-   const char *s = worktree_basename(path, );
-   new_branch = xstrndup(s, n);
-   if (guess_remote) {
-   struct object_id oid;
-   const char *remote =
-   unique_tracking_name(new_branch, );
-   if (remote)
-   branch = remote;
-   }
+   const char *s = dwim_branch(path, _branch);
+   if (s)
+   branch = s;
}
 
if (ac == 2 && !new_branch && !opts.detach) {
-- 
2.16.1.74.g7afd1c25cc.dirty

[PATCH v8 2/4] worktree: improve message when creating a new worktree

[Thomas Gummerer]

Currently 'git worktree add' produces output like the following:

Preparing ../foo (identifier foo)
HEAD is now at 26da330922 

The '../foo' is the path where the worktree is created, which the user
has just given on the command line.  The identifier is an internal
implementation detail, which is not particularly relevant for the user
and indeed isn't mentioned explicitly anywhere in the man page.

Instead of this message, print a message that gives the user a bit more
detail of what exactly 'git worktree' is doing.  There are various dwim
modes which perform some magic under the hood, which should be
helpful to users.  Just from the output of the command it is not always
visible to users what exactly has happened.

Help the users a bit more by modifying the "Preparing ..." message and
adding some additional information of what 'git worktree add' did under
the hood, while not displaying the identifier anymore.

Currently this ends up in three different cases:

  - 'git worktree add -b ...' or 'git worktree add ', both of
which create a new branch, either through the user explicitly
requesting it, or through 'git worktree add' implicitly creating
it.  This will end up with the following output:

  Preparing worktree (new branch '')
  HEAD is now at 26da330922 

  - 'git worktree add -B ...', which may either create a new branch if
the branch with the given name does not exist yet, or resets an
existing branch to the current HEAD, or the commit-ish given.
Depending on which action is taken, we'll end up with the following
output:

  Preparing worktree (resetting branch 'next'; was at caa68db14)
  HEAD is now at 26da330922 

or:

  Preparing worktree (new branch '')
  HEAD is now at 26da330922 

  - 'git worktree add --detach' or 'git worktree add 
', both of which create a new worktree with a detached
HEAD, for which we will print the following output:

  Preparing worktree (detached HEAD 26da330922)
  HEAD is now at 26da330922 

  - 'git worktree add  ', which checks out the
branch prints the following output:

  Preparing worktree (checking out '')
  HEAD is now at 47007d5 

Additionally currently the "Preparing ..." line is printed to stderr,
while the "HEAD is now at ..." line is printed to stdout by 'git reset
--hard', which is used internally by 'git worktree add'.  Fix this
inconsistency by printing the "Preparing ..." message to stdout as
well.  As "Preparing ..." is not an error, stdout also seems like the
more appropriate output stream.

Helped-by: Eric Sunshine 
Signed-off-by: Thomas Gummerer 
---
 builtin/worktree.c | 38 --
 1 file changed, 36 insertions(+), 2 deletions(-)

diff --git a/builtin/worktree.c b/builtin/worktree.c
index 4d96a21a45..d348101216 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -301,8 +301,6 @@ static int add_worktree(const char *path, const char 
*refname,
strbuf_addf(, "%s/commondir", sb_repo.buf);
write_file(sb.buf, "../..");
 
-   fprintf_ln(stderr, _("Preparing %s (identifier %s)"), path, name);
-
argv_array_pushf(_env, "%s=%s", GIT_DIR_ENVIRONMENT, sb_git.buf);
argv_array_pushf(_env, "%s=%s", GIT_WORK_TREE_ENVIRONMENT, path);
cp.git_cmd = 1;
@@ -355,6 +353,40 @@ static int add_worktree(const char *path, const char 
*refname,
return ret;
 }
 
+static void print_preparing_worktree_line(int detach,
+ const char *branch,
+ const char *new_branch,
+ const char *new_branch_force)
+{
+   if (new_branch_force) {
+   struct commit *commit = 
lookup_commit_reference_by_name(new_branch_force);
+   if (!commit)
+   printf_ln(_("Preparing worktree (new branch '%s')"), 
new_branch_force);
+   else
+   printf_ln(_("Preparing worktree (resetting branch '%s'; 
was at %s)"),
+ new_branch_force,
+ find_unique_abbrev(commit->object.oid.hash,
+DEFAULT_ABBREV));
+   } else if (new_branch) {
+   printf_ln(_("Preparing worktree (new branch '%s')"), 
new_branch);
+   } else {
+   struct strbuf s = STRBUF_INIT;
+   if (!detach && !strbuf_check_branch_ref(, branch) &&
+   ref_exists(s.buf))
+   printf_ln(_("Preparing worktree (checking out '%s')"),
+ branch);
+   else {
+   struct commit *commit = 
lookup_commit_reference_by_name(branch);
+   if (!commit)
+   die(_("invalid reference: %s"), branch);
+   printf_ln(_("Preparing worktree 

Re: Feature Request: option for git difftool to show changes in submodule contents

[Stefan Beller]

Hi Oshaben!

On Mon, Apr 23, 2018 at 7:49 AM, Oshaben Nicholas
 wrote:
> Hello,
>
> A coworker of mine was trying to use git difftool in a repository with 
> submodules and we found that it only shows the change in the subproject 
> commit hash. The option to show the changes in the contents of the submodules 
> exists for git diff. Can this be added to difftool as well?

Yes, of course it can be added. Care to write a patch?

See lines 432-443 in builtin/difftool.c:
https://github.com/git/git/blob/master/builtin/difftool.c#L432

I'd think you'd want to compute some output
(Similar to the git-diff output? Then we could just
refactor the submodule diff stuff to be reusable here)
and put it into the strbuf there.

Feel free to ask away or read the docs Documentation/SubmittingPatches

Thanks,
Stefan

Re: Is support for 10.8 dropped?

[Totsten Bögershausen]

On 2018-04-23 18:53, Eric Sunshine wrote:

On Mon, Apr 23, 2018 at 12:31 PM, Igor Korot  wrote:

1. Is the file name "config.mak" or "config.make"?


"config.mak"


I am confused, I have these file in my tree:
config.mak.in  config.mak.uname   Makefile
Setting options is documentend in Makefile




2. Do I have to do "make clean" or just remove the line and o "make"?


"make clean" would not hurt.


I would actually try to install openssl / openssl-dev (or however it is 
called) via "mac ports" or "home brew".


Both should work (but I don't have a system to test on)

Re: [PATCH v3 7/9] commit: add short-circuit to paint_down_to_common()

[Jakub Narebski]

Derrick Stolee  writes:

> On 4/18/2018 7:19 PM, Jakub Narebski wrote:
>> Derrick Stolee  writes:
>>
> [...]
>>> [...], and this saves time during 'git branch --contains' queries
>>> that would otherwise walk "around" the commit we are inspecting.
>>>
>> If I understand the code properly, what happens is that we can now
>> short-circuit if all commits that are left are lower than the target
>> commit.
>>
>> This is because max-order priority queue is used: if the commit with
>> maximum generation number is below generation number of target commit,
>> then target commit is not reachable from any commit in the priority
>> queue (all of which has generation number less or equal than the commit
>> at head of queue, i.e. all are same level or deeper); compare what I
>> have written in [1]
>>
>> [1]: https://public-inbox.org/git/866052dkju@gmail.com/
>>
>> Do I have that right?  If so, it looks all right to me.
>
> Yes, the priority queue needs to compare via generation number first
> or there will be errors. This is why we could not use commit time
> before.

I was more concerned about getting right the order in the priority queue
(does it return minimal or maximal generation number).

I understand that the cutoff could not be used without generation
numbers because of the possibility of clock skew - using cutoff on dates
could lead to wrong results.

>>> For a copy of the Linux repository, where HEAD is checked out at
>>> v4.13~100, we get the following performance improvement for
>>> 'git branch --contains' over the previous commit:
>>>
>>> Before: 0.21s
>>> After:  0.13s
>>> Rel %: -38%
>> [...]
>>> flags = commit->object.flags & (PARENT1 | PARENT2 | STALE);
>>> if (flags == (PARENT1 | PARENT2)) {
>>> if (!(commit->object.flags & RESULT)) {
>>> @@ -876,7 +886,7 @@ static struct commit_list *merge_bases_many(struct 
>>> commit *one, int n, struct co
>>> return NULL;
>>> }
>>>   - list = paint_down_to_common(one, n, twos);
>>> +   list = paint_down_to_common(one, n, twos, 0);
>>> while (list) {
>>> struct commit *commit = pop_commit();
>>> @@ -943,7 +953,7 @@ static int remove_redundant(struct commit **array, int 
>>> cnt)
>>> filled_index[filled] = j;
>>> work[filled++] = array[j];
>>> }
>>> -   common = paint_down_to_common(array[i], filled, work);
>>> +   common = paint_down_to_common(array[i], filled, work, 0);
>>> if (array[i]->object.flags & PARENT2)
>>> redundant[i] = 1;
>>> for (j = 0; j < filled; j++)
>>> @@ -1067,7 +1077,7 @@ int in_merge_bases_many(struct commit *commit, int 
>>> nr_reference, struct commit *
>>> if (commit->generation > min_generation)
>>> return 0;
>>>   - bases = paint_down_to_common(commit, nr_reference, reference);
>>> +   bases = paint_down_to_common(commit, nr_reference, reference, 
>>> commit->generation);
>>
>> Is it the only case where we would call paint_down_to_common() with
>> non-zero last parameter?  Would we always use commit->generation where
>> commit is the first parameter of paint_down_to_common()?
>>
>> If both are true and will remain true, then in my humble opinion it is
>> not necessary to change the signature of this function.
>
> We need to change the signature some way, but maybe the way I chose is
> not the best.

No, after taking longer I think the new signature is a good choice.

> To elaborate: paint_down_to_common() is used for multiple
> purposes. The caller here that supplies 'commit->generation' is used
> only to compute reachability (by testing if the flag PARENT2 exists on
> the commit, then clears all flags). The other callers expect the full
> walk down to the common commits, and keeps those PARENT1, PARENT2, and
> STALE flags for future use (such as reporting merge bases). Usually
> the call to paint_down_to_common() is followed by a revision walk that
> only halts when reaching root commits or commits with both PARENT1 and
> PARENT2 flags on, so always short-circuiting on generations would
> break the functionality; this is confirmed by the
> t5318-commit-graph.sh.

Right.

I have realized that just after sending the email.  I'm sorry about this.

>
> An alternative to the signature change is to add a boolean parameter
> "use_cutoff" or something, that specifies "don't walk beyond the
> commit". This may give a more of a clear description of what it will
> do with the generation value, but since we are already performing
> generation comparisons before calling paint_down_to_common() I find
> this simple enough.

Two things:

1. The signature proposed in the patch is more generic.  The cutoff does
   not need to be equal to the generation number of the commit, though
   currently it always (all of one time the new mechanism is used) is.

   So now I think the new signature of 

GSoC students and mentors in 2018

[Stefan Beller]

Hi Git community,

This year we'll participate once again in Google Summer or Code!
We'll have 3 students and 3 mentors, which is more than in recent years.

Paul-Sebastian Ungureanu mentored by DScho, wants to convert git-stash
into a builtin.

Alban Gruin and Pratik Karki want to convert parts of git-rebase into
a builtin. Both are mentored by Christian and myself.

The slots were just announced today, please join me in welcoming them
to the Git mailing list! (Although you may remember them from the
micro projects[1,2,3])

[1] 
https://public-inbox.org/git/20180319155929.7000-1-ungureanupaulsebast...@gmail.com/
[2] https://public-inbox.org/git/2018030907.17607-1-alban.gr...@gmail.com/
[3] https://public-inbox.org/git/20180327173137.5970-1-predatoram...@gmail.com/

Thanks,
Stefan

[PATCH 2/9] cache.h: add repository argument to oid_object_info

[Stefan Beller]

Add a repository argument to allow the callers of oid_object_info
to be more specific about which repository to handle. This is a small
mechanical change; it doesn't change the implementation to handle
repositories other than the_repository yet.

In the expanded macro the identifier `the_repository` is not actually used,
so the compiler does not catch if the repository.h header is not included
at the call site. call sites needing that #include were identified by
changing the macro to definition to

  #define sha1_object_info(r, sha1, size) \
  (r, sha1_object_info_##r(sha1, size)).

This produces a compiler warning about the left hand side of the comma
operator being unused, which can be suppressed using -Wno-unused-value.

To avoid breaking bisection, do not include this trick in the patch.

Signed-off-by: Stefan Beller 
---
 archive-tar.c|  2 +-
 archive-zip.c|  3 ++-
 blame.c  |  4 ++--
 builtin/blame.c  |  2 +-
 builtin/cat-file.c   |  6 +++---
 builtin/describe.c   |  2 +-
 builtin/fast-export.c|  2 +-
 builtin/fetch.c  |  2 +-
 builtin/fsck.c   |  3 ++-
 builtin/index-pack.c |  4 ++--
 builtin/ls-tree.c|  2 +-
 builtin/mktree.c |  2 +-
 builtin/pack-objects.c   |  8 +---
 builtin/prune.c  |  3 ++-
 builtin/replace.c| 11 ++-
 builtin/tag.c|  4 ++--
 builtin/unpack-objects.c |  2 +-
 cache.h  |  3 ++-
 diff.c   |  3 ++-
 fast-import.c| 14 +-
 list-objects-filter.c|  2 +-
 object.c |  2 +-
 pack-bitmap-write.c  |  3 ++-
 packfile.c   |  2 +-
 reachable.c  |  2 +-
 refs.c   |  2 +-
 remote.c |  2 +-
 sequencer.c  |  3 ++-
 sha1_file.c  |  4 ++--
 sha1_name.c  | 12 ++--
 submodule.c  |  2 +-
 tag.c|  2 +-
 32 files changed, 67 insertions(+), 53 deletions(-)

diff --git a/archive-tar.c b/archive-tar.c
index 3563bcb9f2..f93409324f 100644
--- a/archive-tar.c
+++ b/archive-tar.c
@@ -276,7 +276,7 @@ static int write_tar_entry(struct archiver_args *args,
memcpy(header.name, path, pathlen);
 
if (S_ISREG(mode) && !args->convert &&
-   oid_object_info(oid, ) == OBJ_BLOB &&
+   oid_object_info(the_repository, oid, ) == OBJ_BLOB &&
size > big_file_threshold)
buffer = NULL;
else if (S_ISLNK(mode) || S_ISREG(mode)) {
diff --git a/archive-zip.c b/archive-zip.c
index 6b20bce4d1..74f3fe9103 100644
--- a/archive-zip.c
+++ b/archive-zip.c
@@ -325,7 +325,8 @@ static int write_zip_entry(struct archiver_args *args,
compressed_size = 0;
buffer = NULL;
} else if (S_ISREG(mode) || S_ISLNK(mode)) {
-   enum object_type type = oid_object_info(oid, );
+   enum object_type type = oid_object_info(the_repository, oid,
+   );
 
method = 0;
attr2 = S_ISLNK(mode) ? ((mode | 0777) << 16) :
diff --git a/blame.c b/blame.c
index 78c9808bd1..dfa24473dc 100644
--- a/blame.c
+++ b/blame.c
@@ -81,7 +81,7 @@ static void verify_working_tree_path(struct commit 
*work_tree, const char *path)
unsigned mode;
 
if (!get_tree_entry(commit_oid, path, _oid, ) &&
-   oid_object_info(_oid, NULL) == OBJ_BLOB)
+   oid_object_info(the_repository, _oid, NULL) == 
OBJ_BLOB)
return;
}
 
@@ -504,7 +504,7 @@ static int fill_blob_sha1_and_mode(struct blame_origin 
*origin)
return 0;
if (get_tree_entry(>commit->object.oid, origin->path, 
>blob_oid, >mode))
goto error_out;
-   if (oid_object_info(>blob_oid, NULL) != OBJ_BLOB)
+   if (oid_object_info(the_repository, >blob_oid, NULL) != 
OBJ_BLOB)
goto error_out;
return 0;
  error_out:
diff --git a/builtin/blame.c b/builtin/blame.c
index db38c0b307..bfdf7cc132 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -655,7 +655,7 @@ static int is_a_rev(const char *name)
 
if (get_oid(name, ))
return 0;
-   return OBJ_NONE < oid_object_info(, NULL);
+   return OBJ_NONE < oid_object_info(the_repository, , NULL);
 }
 
 int cmd_blame(int argc, const char **argv, const char *prefix)
diff --git a/builtin/cat-file.c b/builtin/cat-file.c
index 4ecdb9ff54..b8ecbea98e 100644
--- a/builtin/cat-file.c
+++ b/builtin/cat-file.c
@@ -116,7 +116,7 @@ static int cat_one_file(int opt, const char *exp_type, 
const char *obj_name,
/* else fallthrough */
 
case 'p':
-   type = oid_object_info(, NULL);
+   type = oid_object_info(the_repository, , NULL);
if (type < 0)

[PATCH 0/9] object store: oid_object_info is the next contender

[Stefan Beller]

This applies on top of origin/sb/object-store-replace and is available as
https://github.com/stefanbeller/git/tree/oid_object_info

This continues the work of sb/packfiles-in-repository,
extending the layer at which we have to pass in an explicit
repository object to oid_object_info.

A test merge to next shows only a minor merge conflicit (adding
different #include lines in one c file), so this might be a good next
step for the object store series.

Notes on further object store series:
I plan on converting the "parsed object store" next,
which would be {alloc, object, tree, commit, tag}.c as that is a prerequisite
for migrating shallow (which is intermingled with grafts) information to the
object store.

There is currently work going on in allocation (mempool - Jameson Miller)
and grafts (deprecate grafts - DScho), which is why I am sending this
series first. I think it can go in parallel to the "parsed object store"
that is coming next.

Thanks,
Stefan

Jonathan Nieder (1):
  packfile: add repository argument to packed_object_info

Stefan Beller (8):
  cache.h: add repository argument to oid_object_info_extended
  cache.h: add repository argument to oid_object_info
  packfile: add repository argument to retry_bad_packed_offset
  packfile: add repository argument to packed_to_object_type
  packfile: add repository argument to read_object
  packfile: add repository argument to unpack_entry
  packfile: add repository argument to cache_or_unpack_entry
  cache.h: allow sha1_object_info to handle arbitrary repositories

 archive-tar.c|  2 +-
 archive-zip.c|  3 ++-
 blame.c  |  4 ++--
 builtin/blame.c  |  2 +-
 builtin/cat-file.c   | 12 ++--
 builtin/describe.c   |  2 +-
 builtin/fast-export.c|  2 +-
 builtin/fetch.c  |  2 +-
 builtin/fsck.c   |  3 ++-
 builtin/index-pack.c |  4 ++--
 builtin/ls-tree.c|  2 +-
 builtin/mktree.c |  2 +-
 builtin/pack-objects.c   | 11 +++
 builtin/prune.c  |  3 ++-
 builtin/replace.c| 11 ++-
 builtin/tag.c|  4 ++--
 builtin/unpack-objects.c |  2 +-
 cache.h  |  7 +--
 diff.c   |  3 ++-
 fast-import.c| 16 ++--
 list-objects-filter.c|  2 +-
 object.c |  2 +-
 pack-bitmap-write.c  |  3 ++-
 pack-check.c |  3 ++-
 packfile.c   | 40 +++-
 packfile.h   |  6 --
 reachable.c  |  2 +-
 refs.c   |  2 +-
 remote.c |  2 +-
 sequencer.c  |  3 ++-
 sha1_file.c  | 32 ++--
 sha1_name.c  | 12 ++--
 streaming.c  |  2 +-
 submodule.c  |  2 +-
 tag.c|  2 +-
 35 files changed, 121 insertions(+), 91 deletions(-)

-- 
2.17.0.484.g0c8726318c-goog

[PATCH 1/9] cache.h: add repository argument to oid_object_info_extended

[Stefan Beller]

Add a repository argument to allow oid_object_info_extended callers
to be more specific about which repository to act on. This is a small
mechanical change; it doesn't change the implementation to handle
repositories other than the_repository yet.

Signed-off-by: Stefan Beller 
---
 builtin/cat-file.c |  6 +++---
 cache.h|  5 -
 packfile.c |  2 +-
 sha1_file.c| 10 +-
 streaming.c|  2 +-
 5 files changed, 14 insertions(+), 11 deletions(-)

diff --git a/builtin/cat-file.c b/builtin/cat-file.c
index 2c46d257cd..4ecdb9ff54 100644
--- a/builtin/cat-file.c
+++ b/builtin/cat-file.c
@@ -77,7 +77,7 @@ static int cat_one_file(int opt, const char *exp_type, const 
char *obj_name,
switch (opt) {
case 't':
oi.type_name = 
-   if (oid_object_info_extended(, , flags) < 0)
+   if (oid_object_info_extended(the_repository, , , flags) 
< 0)
die("git cat-file: could not get object info");
if (sb.len) {
printf("%s\n", sb.buf);
@@ -88,7 +88,7 @@ static int cat_one_file(int opt, const char *exp_type, const 
char *obj_name,
 
case 's':
oi.sizep = 
-   if (oid_object_info_extended(, , flags) < 0)
+   if (oid_object_info_extended(the_repository, , , flags) 
< 0)
die("git cat-file: could not get object info");
printf("%lu\n", size);
return 0;
@@ -342,7 +342,7 @@ static void batch_object_write(const char *obj_name, struct 
batch_options *opt,
struct strbuf buf = STRBUF_INIT;
 
if (!data->skip_object_info &&
-   oid_object_info_extended(>oid, >info,
+   oid_object_info_extended(the_repository, >oid, >info,
 OBJECT_INFO_LOOKUP_REPLACE) < 0) {
printf("%s missing\n",
   obj_name ? obj_name : oid_to_hex(>oid));
diff --git a/cache.h b/cache.h
index 027bd7ffc8..588c4fff9a 100644
--- a/cache.h
+++ b/cache.h
@@ -1673,7 +1673,10 @@ struct object_info {
 #define OBJECT_INFO_QUICK 8
 /* Do not check loose object */
 #define OBJECT_INFO_IGNORE_LOOSE 16
-extern int oid_object_info_extended(const struct object_id *, struct 
object_info *, unsigned flags);
+
+#define oid_object_info_extended(r, oid, oi, flags) \
+   oid_object_info_extended_##r(oid, oi, flags)
+int oid_object_info_extended_the_repository(const struct object_id *, struct 
object_info *, unsigned flags);
 
 /*
  * Set this to 0 to prevent sha1_object_info_extended() from fetching missing
diff --git a/packfile.c b/packfile.c
index 0bc67d0e00..d9914ba723 100644
--- a/packfile.c
+++ b/packfile.c
@@ -1474,7 +1474,7 @@ static void *read_object(const struct object_id *oid, 
enum object_type *type,
oi.sizep = size;
oi.contentp = 
 
-   if (oid_object_info_extended(oid, , 0) < 0)
+   if (oid_object_info_extended(the_repository, oid, , 0) < 0)
return NULL;
return content;
 }
diff --git a/sha1_file.c b/sha1_file.c
index 64a5bd7d87..50a2dc5f0a 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -1231,7 +1231,7 @@ static int sha1_loose_object_info(struct repository *r,
 
 int fetch_if_missing = 1;
 
-int oid_object_info_extended(const struct object_id *oid, struct object_info 
*oi, unsigned flags)
+int oid_object_info_extended_the_repository(const struct object_id *oid, 
struct object_info *oi, unsigned flags)
 {
static struct object_info blank_oi = OBJECT_INFO_INIT;
struct pack_entry e;
@@ -1310,7 +1310,7 @@ int oid_object_info_extended(const struct object_id *oid, 
struct object_info *oi
rtype = packed_object_info(e.p, e.offset, oi);
if (rtype < 0) {
mark_bad_packed_object(e.p, real->hash);
-   return oid_object_info_extended(real, oi, 0);
+   return oid_object_info_extended(the_repository, real, oi, 0);
} else if (oi->whence == OI_PACKED) {
oi->u.packed.offset = e.offset;
oi->u.packed.pack = e.p;
@@ -1329,7 +1329,7 @@ int oid_object_info(const struct object_id *oid, unsigned 
long *sizep)
 
oi.typep = 
oi.sizep = sizep;
-   if (oid_object_info_extended(oid, ,
+   if (oid_object_info_extended(the_repository, oid, ,
 OBJECT_INFO_LOOKUP_REPLACE) < 0)
return -1;
return type;
@@ -1347,7 +1347,7 @@ static void *read_object(const unsigned char *sha1, enum 
object_type *type,
 
hashcpy(oid.hash, sha1);
 
-   if (oid_object_info_extended(, , 0) < 0)
+   if (oid_object_info_extended(the_repository, , , 0) < 0)
return NULL;
return content;
 }
@@ -1745,7 +1745,7 @@ int has_sha1_file_with_flags(const unsigned char *sha1, 
int flags)
if (!startup_info->have_repository)
return 0;
hashcpy(oid.hash, sha1);
-  

[PATCH 11/41] tree-walk: avoid hard-coded 20 constant

[brian m. carlson]

Use the_hash_algo to look up the length of our current hash instead of
hard-coding the value 20.

Signed-off-by: brian m. carlson 
---
 tree-walk.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tree-walk.c b/tree-walk.c
index e11b3063af..27797c5406 100644
--- a/tree-walk.c
+++ b/tree-walk.c
@@ -105,7 +105,7 @@ static void entry_extract(struct tree_desc *t, struct 
name_entry *a)
 static int update_tree_entry_internal(struct tree_desc *desc, struct strbuf 
*err)
 {
const void *buf = desc->buffer;
-   const unsigned char *end = desc->entry.oid->hash + 20;
+   const unsigned char *end = desc->entry.oid->hash + the_hash_algo->rawsz;
unsigned long size = desc->size;
unsigned long len = end - (const unsigned char *)buf;
 

[PATCH 12/41] tree-walk: convert get_tree_entry_follow_symlinks to object_id

[brian m. carlson]

Since the only caller of this function already uses struct object_id,
update get_tree_entry_follow_symlinks to use it in parameters and
internally.

Signed-off-by: brian m. carlson 
---
 sha1_name.c |  4 ++--
 tree-walk.c | 16 
 tree-walk.h |  2 +-
 3 files changed, 11 insertions(+), 11 deletions(-)

diff --git a/sha1_name.c b/sha1_name.c
index 7043652a24..7c2d08a202 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -1685,8 +1685,8 @@ static int get_oid_with_context_1(const char *name,
if (new_filename)
filename = new_filename;
if (flags & GET_OID_FOLLOW_SYMLINKS) {
-   ret = 
get_tree_entry_follow_symlinks(tree_oid.hash,
-   filename, oid->hash, >symlink_path,
+   ret = get_tree_entry_follow_symlinks(_oid,
+   filename, oid, >symlink_path,
>mode);
} else {
ret = get_tree_entry(_oid, filename, oid,
diff --git a/tree-walk.c b/tree-walk.c
index 27797c5406..8f5090862b 100644
--- a/tree-walk.c
+++ b/tree-walk.c
@@ -488,7 +488,7 @@ int traverse_trees(int n, struct tree_desc *t, struct 
traverse_info *info)
 struct dir_state {
void *tree;
unsigned long size;
-   unsigned char sha1[20];
+   struct object_id oid;
 };
 
 static int find_tree_entry(struct tree_desc *t, const char *name, struct 
object_id *result, unsigned *mode)
@@ -576,7 +576,7 @@ int get_tree_entry(const struct object_id *tree_oid, const 
char *name, struct ob
  * See the code for enum follow_symlink_result for a description of
  * the return values.
  */
-enum follow_symlinks_result get_tree_entry_follow_symlinks(unsigned char 
*tree_sha1, const char *name, unsigned char *result, struct strbuf 
*result_path, unsigned *mode)
+enum follow_symlinks_result get_tree_entry_follow_symlinks(struct object_id 
*tree_oid, const char *name, struct object_id *result, struct strbuf 
*result_path, unsigned *mode)
 {
int retval = MISSING_OBJECT;
struct dir_state *parents = NULL;
@@ -589,7 +589,7 @@ enum follow_symlinks_result 
get_tree_entry_follow_symlinks(unsigned char *tree_s
 
init_tree_desc(, NULL, 0UL);
strbuf_addstr(, name);
-   hashcpy(current_tree_oid.hash, tree_sha1);
+   oidcpy(_tree_oid, tree_oid);
 
while (1) {
int find_result;
@@ -609,11 +609,11 @@ enum follow_symlinks_result 
get_tree_entry_follow_symlinks(unsigned char *tree_s
ALLOC_GROW(parents, parents_nr + 1, parents_alloc);
parents[parents_nr].tree = tree;
parents[parents_nr].size = size;
-   hashcpy(parents[parents_nr].sha1, root.hash);
+   oidcpy([parents_nr].oid, );
parents_nr++;
 
if (namebuf.buf[0] == '\0') {
-   hashcpy(result, root.hash);
+   oidcpy(result, );
retval = FOUND;
goto done;
}
@@ -663,7 +663,7 @@ enum follow_symlinks_result 
get_tree_entry_follow_symlinks(unsigned char *tree_s
 
/* We could end up here via a symlink to dir/.. */
if (namebuf.buf[0] == '\0') {
-   hashcpy(result, parents[parents_nr - 1].sha1);
+   oidcpy(result, [parents_nr - 1].oid);
retval = FOUND;
goto done;
}
@@ -677,7 +677,7 @@ enum follow_symlinks_result 
get_tree_entry_follow_symlinks(unsigned char *tree_s
 
if (S_ISDIR(*mode)) {
if (!remainder) {
-   hashcpy(result, current_tree_oid.hash);
+   oidcpy(result, _tree_oid);
retval = FOUND;
goto done;
}
@@ -687,7 +687,7 @@ enum follow_symlinks_result 
get_tree_entry_follow_symlinks(unsigned char *tree_s
  1 + first_slash - namebuf.buf);
} else if (S_ISREG(*mode)) {
if (!remainder) {
-   hashcpy(result, current_tree_oid.hash);
+   oidcpy(result, _tree_oid);
retval = FOUND;
} else {
retval = NOT_DIR;
diff --git a/tree-walk.h b/tree-walk.h
index 4617deeb0e..805f58f00f 100644
--- a/tree-walk.h
+++ b/tree-walk.h
@@ -64,7 +64,7 @@ enum follow_symlinks_result {
   */
 };
 
-enum follow_symlinks_result get_tree_entry_follow_symlinks(unsigned char 
*tree_sha1, const char *name, unsigned char 

[PATCH 08/41] packfile: abstract away hash constant values

[brian m. carlson]

There are several instances of the constant 20 and 20-based values in
the packfile code.  Abstract away dependence on SHA-1 by using the
values from the_hash_algo instead.

Use unsigned values for temporary constants to provide the compiler with
more information about what kinds of values it should expect.

Signed-off-by: brian m. carlson 
---
 packfile.c | 66 ++
 1 file changed, 37 insertions(+), 29 deletions(-)

diff --git a/packfile.c b/packfile.c
index 84acd405e0..b7bc4eab17 100644
--- a/packfile.c
+++ b/packfile.c
@@ -84,6 +84,7 @@ static int check_packed_git_idx(const char *path, struct 
packed_git *p)
uint32_t version, nr, i, *index;
int fd = git_open(path);
struct stat st;
+   const unsigned int hashsz = the_hash_algo->rawsz;
 
if (fd < 0)
return -1;
@@ -92,7 +93,7 @@ static int check_packed_git_idx(const char *path, struct 
packed_git *p)
return -1;
}
idx_size = xsize_t(st.st_size);
-   if (idx_size < 4 * 256 + 20 + 20) {
+   if (idx_size < 4 * 256 + hashsz + hashsz) {
close(fd);
return error("index file %s is too small", path);
}
@@ -129,11 +130,11 @@ static int check_packed_git_idx(const char *path, struct 
packed_git *p)
/*
 * Total size:
 *  - 256 index entries 4 bytes each
-*  - 24-byte entries * nr (20-byte sha1 + 4-byte offset)
-*  - 20-byte SHA1 of the packfile
-*  - 20-byte SHA1 file checksum
+*  - 24-byte entries * nr (object ID + 4-byte offset)
+*  - hash of the packfile
+*  - file checksum
 */
-   if (idx_size != 4*256 + nr * 24 + 20 + 20) {
+   if (idx_size != 4*256 + nr * (hashsz + 4) + hashsz + hashsz) {
munmap(idx_map, idx_size);
return error("wrong index v1 file size in %s", path);
}
@@ -142,16 +143,16 @@ static int check_packed_git_idx(const char *path, struct 
packed_git *p)
 * Minimum size:
 *  - 8 bytes of header
 *  - 256 index entries 4 bytes each
-*  - 20-byte sha1 entry * nr
+*  - object ID entry * nr
 *  - 4-byte crc entry * nr
 *  - 4-byte offset entry * nr
-*  - 20-byte SHA1 of the packfile
-*  - 20-byte SHA1 file checksum
+*  - hash of the packfile
+*  - file checksum
 * And after the 4-byte offset table might be a
 * variable sized table containing 8-byte entries
 * for offsets larger than 2^31.
 */
-   unsigned long min_size = 8 + 4*256 + nr*(20 + 4 + 4) + 20 + 20;
+   unsigned long min_size = 8 + 4*256 + nr*(hashsz + 4 + 4) + 
hashsz + hashsz;
unsigned long max_size = min_size;
if (nr)
max_size += (nr - 1)*8;
@@ -444,10 +445,11 @@ static int open_packed_git_1(struct packed_git *p)
 {
struct stat st;
struct pack_header hdr;
-   unsigned char sha1[20];
-   unsigned char *idx_sha1;
+   unsigned char hash[GIT_MAX_RAWSZ];
+   unsigned char *idx_hash;
long fd_flag;
ssize_t read_result;
+   const unsigned hashsz = the_hash_algo->rawsz;
 
if (!p->index_data && open_pack_index(p))
return error("packfile %s index unavailable", p->pack_name);
@@ -507,15 +509,15 @@ static int open_packed_git_1(struct packed_git *p)
 " while index indicates %"PRIu32" objects",
 p->pack_name, ntohl(hdr.hdr_entries),
 p->num_objects);
-   if (lseek(p->pack_fd, p->pack_size - sizeof(sha1), SEEK_SET) == -1)
+   if (lseek(p->pack_fd, p->pack_size - hashsz, SEEK_SET) == -1)
return error("end of packfile %s is unavailable", p->pack_name);
-   read_result = read_in_full(p->pack_fd, sha1, sizeof(sha1));
+   read_result = read_in_full(p->pack_fd, hash, hashsz);
if (read_result < 0)
return error_errno("error reading from %s", p->pack_name);
-   if (read_result != sizeof(sha1))
+   if (read_result != hashsz)
return error("packfile %s signature is unavailable", 
p->pack_name);
-   idx_sha1 = ((unsigned char *)p->index_data) + p->index_size - 40;
-   if (hashcmp(sha1, idx_sha1))
+   idx_hash = ((unsigned char *)p->index_data) + p->index_size - hashsz * 
2;
+   if (hashcmp(hash, idx_hash))
return error("packfile %s does not match index", p->pack_name);
return 0;
 }
@@ -530,7 +532,7 @@ static int open_packed_git(struct packed_git *p)
 
 static int in_window(struct 

[PATCH 3/9] packfile: add repository argument to retry_bad_packed_offset

[Stefan Beller]

Add a repository argument to allow the callers of retry_bad_packed_offset
to be more specific about which repository to handle. This is a small
mechanical change; it doesn't change the implementation to handle
repositories other than the_repository yet.

As with the previous commits, use a macro to catch callers passing a
repository other than the_repository at compile time.

Signed-off-by: Jonathan Nieder 
Signed-off-by: Stefan Beller 
---
 packfile.c | 8 +---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/packfile.c b/packfile.c
index 80c7fa734f..d2b3f3503b 100644
--- a/packfile.c
+++ b/packfile.c
@@ -1104,7 +1104,9 @@ static const unsigned char *get_delta_base_sha1(struct 
packed_git *p,
return NULL;
 }
 
-static int retry_bad_packed_offset(struct packed_git *p, off_t obj_offset)
+#define retry_bad_packed_offset(r, p, o) \
+   retry_bad_packed_offset_##r(p, o)
+static int retry_bad_packed_offset_the_repository(struct packed_git *p, off_t 
obj_offset)
 {
int type;
struct revindex_entry *revidx;
@@ -1153,7 +1155,7 @@ static enum object_type packed_to_object_type(struct 
packed_git *p,
if (type <= OBJ_NONE) {
/* If getting the base itself fails, we first
 * retry the base, otherwise unwind */
-   type = retry_bad_packed_offset(p, base_offset);
+   type = retry_bad_packed_offset(the_repository, p, 
base_offset);
if (type > OBJ_NONE)
goto out;
goto unwind;
@@ -1181,7 +1183,7 @@ static enum object_type packed_to_object_type(struct 
packed_git *p,
 unwind:
while (poi_stack_nr) {
obj_offset = poi_stack[--poi_stack_nr];
-   type = retry_bad_packed_offset(p, obj_offset);
+   type = retry_bad_packed_offset(the_repository, p, obj_offset);
if (type > OBJ_NONE)
goto out;
}
-- 
2.17.0.484.g0c8726318c-goog

[PATCH 6/9] packfile: add repository argument to read_object

[Stefan Beller]

Add a repository argument to allow the callers of read_object
to be more specific about which repository to act on. This is a small
mechanical change; it doesn't change the implementation to handle
repositories other than the_repository yet.

As with the previous commits, use a macro to catch callers passing a
repository other than the_repository at compile time.

Signed-off-by: Stefan Beller 
---
 packfile.c | 8 +---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/packfile.c b/packfile.c
index 5fa7d27d3b..2876e04bb1 100644
--- a/packfile.c
+++ b/packfile.c
@@ -1469,8 +1469,10 @@ struct unpack_entry_stack_ent {
unsigned long size;
 };
 
-static void *read_object(const struct object_id *oid, enum object_type *type,
-unsigned long *size)
+#define read_object(r, o, t, s) read_object_##r(o, t, s)
+static void *read_object_the_repository(const struct object_id *oid,
+   enum object_type *type,
+   unsigned long *size)
 {
struct object_info oi = OBJECT_INFO_INIT;
void *content;
@@ -1614,7 +1616,7 @@ void *unpack_entry(struct packed_git *p, off_t obj_offset,
  oid_to_hex(_oid), 
(uintmax_t)obj_offset,
  p->pack_name);
mark_bad_packed_object(p, base_oid.hash);
-   base = read_object(_oid, , 
_size);
+   base = read_object(the_repository, _oid, 
, _size);
external_base = base;
}
}
-- 
2.17.0.484.g0c8726318c-goog

[PATCH 10/41] pack-redundant: abstract away hash algorithm

[brian m. carlson]

Instead of using hard-coded instances of the constant 20, use
the_hash_algo to look up the correct constant.

Signed-off-by: brian m. carlson 
---
 builtin/pack-redundant.c | 12 +++-
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/builtin/pack-redundant.c b/builtin/pack-redundant.c
index 354478a127..0fe1ff3cb7 100644
--- a/builtin/pack-redundant.c
+++ b/builtin/pack-redundant.c
@@ -252,13 +252,14 @@ static void cmp_two_packs(struct pack_list *p1, struct 
pack_list *p2)
unsigned long p1_off = 0, p2_off = 0, p1_step, p2_step;
const unsigned char *p1_base, *p2_base;
struct llist_item *p1_hint = NULL, *p2_hint = NULL;
+   const unsigned int hashsz = the_hash_algo->rawsz;
 
p1_base = p1->pack->index_data;
p2_base = p2->pack->index_data;
p1_base += 256 * 4 + ((p1->pack->index_version < 2) ? 4 : 8);
p2_base += 256 * 4 + ((p2->pack->index_version < 2) ? 4 : 8);
-   p1_step = (p1->pack->index_version < 2) ? 24 : 20;
-   p2_step = (p2->pack->index_version < 2) ? 24 : 20;
+   p1_step = hashsz + ((p1->pack->index_version < 2) ? 4 : 0);
+   p2_step = hashsz + ((p2->pack->index_version < 2) ? 4 : 0);
 
while (p1_off < p1->pack->num_objects * p1_step &&
   p2_off < p2->pack->num_objects * p2_step)
@@ -359,13 +360,14 @@ static size_t sizeof_union(struct packed_git *p1, struct 
packed_git *p2)
size_t ret = 0;
unsigned long p1_off = 0, p2_off = 0, p1_step, p2_step;
const unsigned char *p1_base, *p2_base;
+   const unsigned int hashsz = the_hash_algo->rawsz;
 
p1_base = p1->index_data;
p2_base = p2->index_data;
p1_base += 256 * 4 + ((p1->index_version < 2) ? 4 : 8);
p2_base += 256 * 4 + ((p2->index_version < 2) ? 4 : 8);
-   p1_step = (p1->index_version < 2) ? 24 : 20;
-   p2_step = (p2->index_version < 2) ? 24 : 20;
+   p1_step = hashsz + ((p1->index_version < 2) ? 4 : 0);
+   p2_step = hashsz + ((p2->index_version < 2) ? 4 : 0);
 
while (p1_off < p1->num_objects * p1_step &&
   p2_off < p2->num_objects * p2_step)
@@ -558,7 +560,7 @@ static struct pack_list * add_pack(struct packed_git *p)
 
base = p->index_data;
base += 256 * 4 + ((p->index_version < 2) ? 4 : 8);
-   step = (p->index_version < 2) ? 24 : 20;
+   step = the_hash_algo->rawsz + ((p->index_version < 2) ? 4 : 0);
while (off < p->num_objects * step) {
llist_insert_back(l.all_objects, base + off);
off += step;

Re: [PATCH 9/9] cache.h: allow sha1_object_info to handle arbitrary repositories

[Stefan Beller]

On Mon, Apr 23, 2018 at 5:34 PM, brian m. carlson
 wrote:
> On Mon, Apr 23, 2018 at 04:43:27PM -0700, Stefan Beller wrote:
>> This involves also adapting sha1_object_info_extended and a some
>> internal functions that are used to implement these. It all has to
>> happen in one patch, because of a single recursive chain of calls visits
>> all these functions.
>
> Ah, yes, I remember that recursive call chain.
>
> Anyway, other than the one item I mentioned earlier, this series looked
> good to me.

Thanks for the quick review!

Well, this very paragraph that you quote also mentions
*sha1*_object_info_extended, so I'll fix that, too

I'll wait until tomorrow for a resend to give others
the opportunity to weigh in as well.

Thanks,
Stefan

[PATCH 2/3] ls-remote: send server options when using protocol v2

[Brandon Williams]

Teach ls-remote to optionally accept server options by specifying them
on the cmdline via '-o' or '--server-option'.  These server options are
sent to the remote end when querying for the remote end's refs using
protocol version 2.

If communicating using a protocol other than v2 the provided options are
ignored and not sent to the remote end.

Signed-off-by: Brandon Williams 
---
 Documentation/git-ls-remote.txt |  8 
 builtin/ls-remote.c |  4 
 connect.c   |  9 -
 remote.h|  4 +++-
 t/t5702-protocol-v2.sh  | 16 
 transport.c |  2 +-
 transport.h |  6 ++
 7 files changed, 46 insertions(+), 3 deletions(-)

diff --git a/Documentation/git-ls-remote.txt b/Documentation/git-ls-remote.txt
index 5f2628c8f..e5defb1b2 100644
--- a/Documentation/git-ls-remote.txt
+++ b/Documentation/git-ls-remote.txt
@@ -60,6 +60,14 @@ OPTIONS
upload-pack only shows the symref HEAD, so it will be the only
one shown by ls-remote.
 
+-o ::
+--server-option=::
+   Transmit the given string to the server when communicating using
+   protocol version 2.  The given string must not contain a NUL or LF
+   character.
+   When multiple `--server-option=` are given, they are all
+   sent to the other side in the order listed on the command line.
+
 ::
The "remote" repository to query.  This parameter can be
either a URL or the name of a remote (see the GIT URLS and
diff --git a/builtin/ls-remote.c b/builtin/ls-remote.c
index 380c18027..3150bfb92 100644
--- a/builtin/ls-remote.c
+++ b/builtin/ls-remote.c
@@ -45,6 +45,7 @@ int cmd_ls_remote(int argc, const char **argv, const char 
*prefix)
const char *uploadpack = NULL;
const char **pattern = NULL;
struct argv_array ref_prefixes = ARGV_ARRAY_INIT;
+   struct string_list server_options = STRING_LIST_INIT_DUP;
 
struct remote *remote;
struct transport *transport;
@@ -67,6 +68,7 @@ int cmd_ls_remote(int argc, const char **argv, const char 
*prefix)
  2, PARSE_OPT_NOCOMPLETE),
OPT_BOOL(0, "symref", _symref_target,
 N_("show underlying ref in addition to the object 
pointed by it")),
+   OPT_STRING_LIST('o', "server-option", _options, 
N_("server-specific"), N_("option to transmit")),
OPT_END()
};
 
@@ -107,6 +109,8 @@ int cmd_ls_remote(int argc, const char **argv, const char 
*prefix)
transport = transport_get(remote, NULL);
if (uploadpack != NULL)
transport_set_option(transport, TRANS_OPT_UPLOADPACK, 
uploadpack);
+   if (server_options.nr)
+   transport->server_options = _options;
 
ref = transport_get_remote_refs(transport, _prefixes);
if (transport_disconnect(transport))
diff --git a/connect.c b/connect.c
index 54971166a..3000768c7 100644
--- a/connect.c
+++ b/connect.c
@@ -408,7 +408,8 @@ static int process_ref_v2(const char *line, struct ref 
***list)
 
 struct ref **get_remote_refs(int fd_out, struct packet_reader *reader,
 struct ref **list, int for_push,
-const struct argv_array *ref_prefixes)
+const struct argv_array *ref_prefixes,
+const struct string_list *server_options)
 {
int i;
*list = NULL;
@@ -419,6 +420,12 @@ struct ref **get_remote_refs(int fd_out, struct 
packet_reader *reader,
if (server_supports_v2("agent", 0))
packet_write_fmt(fd_out, "agent=%s", 
git_user_agent_sanitized());
 
+   if (server_options && server_options->nr &&
+   server_supports_v2("server-option", 1))
+   for (i = 0; i < server_options->nr; i++)
+   packet_write_fmt(fd_out, "server-option=%s",
+server_options->items[i].string);
+
packet_delim(fd_out);
/* When pushing we don't want to request the peeled tags */
if (!for_push)
diff --git a/remote.h b/remote.h
index 2b3180f94..93dd97e25 100644
--- a/remote.h
+++ b/remote.h
@@ -153,6 +153,7 @@ void free_refs(struct ref *ref);
 struct oid_array;
 struct packet_reader;
 struct argv_array;
+struct string_list;
 extern struct ref **get_remote_heads(struct packet_reader *reader,
 struct ref **list, unsigned int flags,
 struct oid_array *extra_have,
@@ -161,7 +162,8 @@ extern struct ref **get_remote_heads(struct packet_reader 
*reader,
 /* Used for protocol v2 in order to retrieve refs from a remote */
 extern struct ref **get_remote_refs(int fd_out, struct packet_reader *reader,
struct ref **list, int for_push,
-   const struct argv_array *ref_prefixes);

[PATCH 1/3] serve: introduce the server-option capability

[Brandon Williams]

Introduce the "server-option" capability to protocol version 2.  This
enables future clients the ability to send server specific options in
command requests when using protocol version 2.

Signed-off-by: Brandon Williams 
---
 Documentation/technical/protocol-v2.txt | 10 ++
 serve.c |  1 +
 t/t5701-git-serve.sh| 21 +
 3 files changed, 32 insertions(+)

diff --git a/Documentation/technical/protocol-v2.txt 
b/Documentation/technical/protocol-v2.txt
index 136179d7d..d7b6f38e0 100644
--- a/Documentation/technical/protocol-v2.txt
+++ b/Documentation/technical/protocol-v2.txt
@@ -393,3 +393,13 @@ header.
1 - pack data
2 - progress messages
3 - fatal error message just before stream aborts
+
+ server-option
+~~~
+
+If advertised, indicates that any number of server specific options can be
+included in a request.  This is done by sending each option as a
+"server-option=" capability line in the capability-list section of
+a request.
+
+The provided options must not contain a NUL or LF character.
diff --git a/serve.c b/serve.c
index a5a7b2f7d..bda085f09 100644
--- a/serve.c
+++ b/serve.c
@@ -56,6 +56,7 @@ static struct protocol_capability capabilities[] = {
{ "agent", agent_advertise, NULL },
{ "ls-refs", always_advertise, ls_refs },
{ "fetch", upload_pack_advertise, upload_pack_v2 },
+   { "server-option", always_advertise, NULL },
 };
 
 static void advertise_capabilities(void)
diff --git a/t/t5701-git-serve.sh b/t/t5701-git-serve.sh
index 72d7bc562..011a5796d 100755
--- a/t/t5701-git-serve.sh
+++ b/t/t5701-git-serve.sh
@@ -10,6 +10,7 @@ test_expect_success 'test capability advertisement' '
agent=git/$(git version | cut -d" " -f3)
ls-refs
fetch=shallow
+   server-option

EOF
 
@@ -173,4 +174,24 @@ test_expect_success 'symrefs parameter' '
test_cmp actual expect
 '
 
+test_expect_success 'sending server-options' '
+   test-pkt-line pack >in <<-EOF &&
+   command=ls-refs
+   server-option=hello
+   server-option=world
+   0001
+   ref-prefix HEAD
+   
+   EOF
+
+   cat >expect <<-EOF &&
+   $(git rev-parse HEAD) HEAD
+   
+   EOF
+
+   git serve --stateless-rpc out &&
+   test-pkt-line unpack actual &&
+   test_cmp actual expect
+'
+
 test_done
-- 
2.17.0.484.g0c8726318c-goog

[PATCH 0/3] optionally send server-options when using v2

[Brandon Williams]

Building on top of protocol version 2 this series adds the ability to
optionally send server specific options when using protocol v2. This
resembles the "push-options" feature except server options are sent as
capability lines during a command request allowing for all current and
future commands to benefit from sending arbitrary server options (and
not requiring that sending server specific options be re-implemented for
each and every command that may want to make use of them in the future).

These options can be provided by the user via the command line by giving
"-o " or "--server-option=" to either ls-remote or
fetch.

Command request example:

command=fetch
server-option=hello
server-option=world
0001
want A
want B
have X
have Y


These options are only transmitted to the remote end when communicating
using protocol version 2.

Brandon Williams (3):
  serve: introduce the server-option capability
  ls-remote: send server options when using protocol v2
  fetch: send server options when using protocol v2

 Documentation/fetch-options.txt |  8 +++
 Documentation/git-ls-remote.txt |  8 +++
 Documentation/technical/protocol-v2.txt | 10 
 builtin/fetch.c |  5 
 builtin/ls-remote.c |  4 
 connect.c   |  9 ++-
 fetch-pack.c|  7 ++
 fetch-pack.h|  1 +
 remote.h|  4 +++-
 serve.c |  1 +
 t/t5701-git-serve.sh| 21 
 t/t5702-protocol-v2.sh  | 32 +
 transport.c |  3 ++-
 transport.h |  6 +
 14 files changed, 116 insertions(+), 3 deletions(-)

-- 
2.17.0.484.g0c8726318c-goog

[PATCH 3/3] fetch: send server options when using protocol v2

[Brandon Williams]

Teach fetch to optionally accept server options by specifying them on
the cmdline via '-o' or '--server-option'.  These server options are
sent to the remote end when performing a fetch communicating using
protocol version 2.

If communicating using a protocol other than v2 the provided options are
ignored and not sent to the remote end.

Signed-off-by: Brandon Williams 
---
 Documentation/fetch-options.txt |  8 
 builtin/fetch.c |  5 +
 fetch-pack.c|  7 +++
 fetch-pack.h|  1 +
 t/t5702-protocol-v2.sh  | 16 
 transport.c |  1 +
 6 files changed, 38 insertions(+)

diff --git a/Documentation/fetch-options.txt b/Documentation/fetch-options.txt
index 8631e365f..97d3217df 100644
--- a/Documentation/fetch-options.txt
+++ b/Documentation/fetch-options.txt
@@ -188,6 +188,14 @@ endif::git-pull[]
is specified. This flag forces progress status even if the
standard error stream is not directed to a terminal.
 
+-o ::
+--server-option=::
+   Transmit the given string to the server when communicating using
+   protocol version 2.  The given string must not contain a NUL or LF
+   character.
+   When multiple `--server-option=` are given, they are all
+   sent to the other side in the order listed on the command line.
+
 -4::
 --ipv4::
Use IPv4 addresses only, ignoring IPv6 addresses.
diff --git a/builtin/fetch.c b/builtin/fetch.c
index 7ee83ac0f..5a6f6b2dc 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -62,6 +62,7 @@ static int shown_url = 0;
 static int refmap_alloc, refmap_nr;
 static const char **refmap_array;
 static struct list_objects_filter_options filter_options;
+static struct string_list server_options = STRING_LIST_INIT_DUP;
 
 static int git_fetch_config(const char *k, const char *v, void *cb)
 {
@@ -170,6 +171,7 @@ static struct option builtin_fetch_options[] = {
 N_("accept refs that update .git/shallow")),
{ OPTION_CALLBACK, 0, "refmap", NULL, N_("refmap"),
  N_("specify fetch refmap"), PARSE_OPT_NONEG, parse_refmap_arg },
+   OPT_STRING_LIST('o', "server-option", _options, 
N_("server-specific"), N_("option to transmit")),
OPT_SET_INT('4', "ipv4", , N_("use IPv4 addresses only"),
TRANSPORT_FAMILY_IPV4),
OPT_SET_INT('6', "ipv6", , N_("use IPv6 addresses only"),
@@ -1417,6 +1419,9 @@ static int fetch_one(struct remote *remote, int argc, 
const char **argv, int pru
}
}
 
+   if (server_options.nr)
+   gtransport->server_options = _options;
+
sigchain_push_common(unlock_pack_on_signal);
atexit(unlock_pack);
refspec = parse_fetch_refspec(ref_nr, refs);
diff --git a/fetch-pack.c b/fetch-pack.c
index 216d1368b..199eb8a1d 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -1174,6 +1174,13 @@ static int send_fetch_request(int fd_out, const struct 
fetch_pack_args *args,
packet_buf_write(_buf, "command=fetch");
if (server_supports_v2("agent", 0))
packet_buf_write(_buf, "agent=%s", 
git_user_agent_sanitized());
+   if (args->server_options && args->server_options->nr &&
+   server_supports_v2("server-option", 1)) {
+   int i;
+   for (i = 0; i < args->server_options->nr; i++)
+   packet_write_fmt(fd_out, "server-option=%s",
+args->server_options->items[i].string);
+   }
 
packet_buf_delim(_buf);
if (args->use_thin_pack)
diff --git a/fetch-pack.h b/fetch-pack.h
index 667024a76..f4ba851c6 100644
--- a/fetch-pack.h
+++ b/fetch-pack.h
@@ -15,6 +15,7 @@ struct fetch_pack_args {
const char *deepen_since;
const struct string_list *deepen_not;
struct list_objects_filter_options filter_options;
+   const struct string_list *server_options;
unsigned deepen_relative:1;
unsigned quiet:1;
unsigned keep_pack:1;
diff --git a/t/t5702-protocol-v2.sh b/t/t5702-protocol-v2.sh
index 71ef1aee1..dbfd0691c 100755
--- a/t/t5702-protocol-v2.sh
+++ b/t/t5702-protocol-v2.sh
@@ -217,6 +217,22 @@ test_expect_success 'ref advertisment is filtered during 
fetch using protocol v2
! grep "refs/tags/three" log
 '
 
+test_expect_success 'server-options are sent when fetching' '
+   test_when_finished "rm -f log" &&
+
+   test_commit -C file_parent four &&
+
+   GIT_TRACE_PACKET="$(pwd)/log" git -C file_child -c protocol.version=2 \
+   fetch -o hello -o world origin master &&
+
+   git -C file_child log -1 --format=%s origin/master >actual &&
+   git -C file_parent log -1 --format=%s >expect &&
+   test_cmp expect actual &&
+
+   grep "server-option=hello" log &&
+   grep "server-option=world" log
+'
+
 # Test protocol v2 with 'http://' transport
 #
 . 

[PATCH dj/runtime-prefix 0/2] Handle $IFS in $INSTLIBDIR

[Jonathan Nieder]

Hi,

Johannes Sixt wrote:
> Am 05.12.2017 um 22:35 schrieb Junio C Hamano:
> > Dan Jacques  writes:

>>> Thanks for checking! The patch that you quoted above looks like it's from
>>> this "v4" thread; however, the patch that you are diffing against in your
>>> latest reply seems like it is from an earlier version.
>>>
>>> I believe that the $(pathsep) changes in your proposed patch are already
>>> present in v4,...
>>
>> You're of course right.  The patches I had in my tree are outdated.
>>
>> Will replace, even though I won't be merging them to 'pu' while we
>> wait for Ævar's perl build procedure update to stabilize.
>
> The updated series works for me now. Nevertheless, I suggest to squash
> in the following change to protect against IFS and globbing characters in
> $INSTLIBDIR.
>
> diff --git a/Makefile b/Makefile
> index 7ac4458f11..08c78a1a63 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -2072,7 +2072,7 @@ GIT-PERL-HEADER: $(PERL_HEADER_TEMPLATE) 
> GIT-PERL-DEFINES perl/perl.mak Makefile
>   INSTLIBDIR_EXTRA='$(PERLLIB_EXTRA_SQ)' && \
>   INSTLIBDIR="$$INSTLIBDIR$${INSTLIBDIR_EXTRA:+:$$INSTLIBDIR_EXTRA}" && \
>   sed -e 's=@@PATHSEP@@=$(pathsep)=g' \
> - -e 's=@@INSTLIBDIR@@='$$INSTLIBDIR'=g' \
> + -e 's=@@INSTLIBDIR@@='"$$INSTLIBDIR"'=g' \
>   -e 's=@@GITEXECDIR@@=$(gitexecdir_relative_SQ)=g' \
>   -e 's=@@PERLLIBDIR@@=$(perllibdir_relative_SQ)=g' \

I just ran into this.  Here's a pair of patches to fix it.

Thanks,
Jonathan Nieder (2):
  Makefile: remove unused substitution variable @@PERLLIBDIR@@
  Makefile: quote $INSTLIBDIR when passing it to sed

 Makefile | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

-- 
2.17.0.441.gb46fe60e1d

Re: [PATCH v1 1/2] merge: Add merge.renames config setting

[Junio C Hamano]

Ben Peart  writes:

>> I also had to wonder how "merge -s resolve" faired, if the project
>> is not interested in renamed paths at all.
>>
>
> To be clear, it isn't that we're not interested in detecting renamed
> files and paths.  We're just opposed to it taking an hour to figure
> that out!

Yeah, but as opposed to passing "oh, let's see if we can get a
reasonable result without rename detection just this time" from the
command line, configuring merge.renames=false in would mean exactly
that: "we don't need rename detection, just want to skip the cycles
spent for it".  That is why I wondered how well the resolve strategy
would have fit your needs.

[PATCH 20/41] dir: convert struct untracked_cache_dir to object_id

[brian m. carlson]

Convert the exclude_sha1 member of struct untracked_cache_dir and rename
it to exclude_oid.  Eliminate several hard-coded integral constants, and
update a function name that referred to SHA-1.

Signed-off-by: brian m. carlson 
---
 dir.c| 23 ---
 dir.h|  5 +++--
 t/helper/test-dump-untracked-cache.c |  2 +-
 3 files changed, 16 insertions(+), 14 deletions(-)

diff --git a/dir.c b/dir.c
index 63a917be45..06f4c4a8bf 100644
--- a/dir.c
+++ b/dir.c
@@ -1240,11 +1240,11 @@ static void prep_exclude(struct dir_struct *dir,
(!untracked || !untracked->valid ||
 /*
  * .. and .gitignore does not exist before
- * (i.e. null exclude_sha1). Then we can skip
+ * (i.e. null exclude_oid). Then we can skip
  * loading .gitignore, which would result in
  * ENOENT anyway.
  */
-!is_null_sha1(untracked->exclude_sha1))) {
+!is_null_oid(>exclude_oid))) {
/*
 * dir->basebuf gets reused by the traversal, but we
 * need fname to remain unchanged to ensure the src
@@ -1275,9 +1275,9 @@ static void prep_exclude(struct dir_struct *dir,
 * order, though, if you do that.
 */
if (untracked &&
-   hashcmp(oid_stat.oid.hash, untracked->exclude_sha1)) {
+   oidcmp(_stat.oid, >exclude_oid)) {
invalidate_gitignore(dir->untracked, untracked);
-   hashcpy(untracked->exclude_sha1, oid_stat.oid.hash);
+   oidcpy(>exclude_oid, _stat.oid);
}
dir->exclude_stack = stk;
current = stk->baselen;
@@ -2622,9 +2622,10 @@ static void write_one_dir(struct untracked_cache_dir 
*untracked,
stat_data_to_disk(_data, >stat_data);
strbuf_add(>sb_stat, _data, sizeof(stat_data));
}
-   if (!is_null_sha1(untracked->exclude_sha1)) {
+   if (!is_null_oid(>exclude_oid)) {
ewah_set(wd->sha1_valid, i);
-   strbuf_add(>sb_sha1, untracked->exclude_sha1, 20);
+   strbuf_add(>sb_sha1, untracked->exclude_oid.hash,
+  the_hash_algo->rawsz);
}
 
intlen = encode_varint(untracked->untracked_nr, intbuf);
@@ -2825,16 +2826,16 @@ static void read_stat(size_t pos, void *cb)
ud->valid = 1;
 }
 
-static void read_sha1(size_t pos, void *cb)
+static void read_oid(size_t pos, void *cb)
 {
struct read_data *rd = cb;
struct untracked_cache_dir *ud = rd->ucd[pos];
-   if (rd->data + 20 > rd->end) {
+   if (rd->data + the_hash_algo->rawsz > rd->end) {
rd->data = rd->end + 1;
return;
}
-   hashcpy(ud->exclude_sha1, rd->data);
-   rd->data += 20;
+   hashcpy(ud->exclude_oid.hash, rd->data);
+   rd->data += the_hash_algo->rawsz;
 }
 
 static void load_oid_stat(struct oid_stat *oid_stat, const unsigned char *data,
@@ -2917,7 +2918,7 @@ struct untracked_cache *read_untracked_extension(const 
void *data, unsigned long
ewah_each_bit(rd.check_only, set_check_only, );
rd.data = next + len;
ewah_each_bit(rd.valid, read_stat, );
-   ewah_each_bit(rd.sha1_valid, read_sha1, );
+   ewah_each_bit(rd.sha1_valid, read_oid, );
next = rd.data;
 
 done:
diff --git a/dir.h b/dir.h
index b0758b82a2..de66be9f4e 100644
--- a/dir.h
+++ b/dir.h
@@ -3,6 +3,7 @@
 
 /* See Documentation/technical/api-directory-listing.txt */
 
+#include "cache.h"
 #include "strbuf.h"
 
 struct dir_entry {
@@ -118,8 +119,8 @@ struct untracked_cache_dir {
/* all data except 'dirs' in this struct are good */
unsigned int valid : 1;
unsigned int recurse : 1;
-   /* null SHA-1 means this directory does not have .gitignore */
-   unsigned char exclude_sha1[20];
+   /* null object ID means this directory does not have .gitignore */
+   struct object_id exclude_oid;
char name[FLEX_ARRAY];
 };
 
diff --git a/t/helper/test-dump-untracked-cache.c 
b/t/helper/test-dump-untracked-cache.c
index d7c55c2355..bd92fb305a 100644
--- a/t/helper/test-dump-untracked-cache.c
+++ b/t/helper/test-dump-untracked-cache.c
@@ -23,7 +23,7 @@ static void dump(struct untracked_cache_dir *ucd, struct 
strbuf *base)
len = base->len;
strbuf_addf(base, "%s/", ucd->name);
printf("%s %s", base->buf,
-  sha1_to_hex(ucd->exclude_sha1));
+  oid_to_hex(>exclude_oid));
if (ucd->recurse)
fputs(" recurse", stdout);
if (ucd->check_only)

[PATCH 28/41] merge: convert empty tree constant to the_hash_algo

[brian m. carlson]

To avoid dependency on a particular hash algorithm, convert a use of
EMPTY_TREE_SHA1_HEX to use the_hash_algo->empty_tree instead.  Since
both branches now use oid_to_hex, condense the if statement into a
ternary.

Signed-off-by: brian m. carlson 
---
 merge.c | 5 +
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/merge.c b/merge.c
index f06a4773d4..5186cb6156 100644
--- a/merge.c
+++ b/merge.c
@@ -11,10 +11,7 @@
 
 static const char *merge_argument(struct commit *commit)
 {
-   if (commit)
-   return oid_to_hex(>object.oid);
-   else
-   return EMPTY_TREE_SHA1_HEX;
+   return oid_to_hex(commit ? >object.oid : 
the_hash_algo->empty_tree);
 }
 
 int index_has_changes(struct strbuf *sb)

[PATCH 17/41] pack-redundant: convert linked lists to use struct object_id

[brian m. carlson]

Convert struct llist_item and the rest of the linked list code to use
struct object_id.  Add a use of GIT_MAX_HEXSZ to avoid a dependency on a
hard-coded constant.

Signed-off-by: brian m. carlson 
---
 builtin/pack-redundant.c | 50 +---
 1 file changed, 26 insertions(+), 24 deletions(-)

diff --git a/builtin/pack-redundant.c b/builtin/pack-redundant.c
index 0fe1ff3cb7..0494dceff7 100644
--- a/builtin/pack-redundant.c
+++ b/builtin/pack-redundant.c
@@ -20,7 +20,7 @@ static int load_all_packs, verbose, alt_odb;
 
 struct llist_item {
struct llist_item *next;
-   const unsigned char *sha1;
+   const struct object_id *oid;
 };
 static struct llist {
struct llist_item *front;
@@ -90,14 +90,14 @@ static struct llist * llist_copy(struct llist *list)
return ret;
 
new_item = ret->front = llist_item_get();
-   new_item->sha1 = list->front->sha1;
+   new_item->oid = list->front->oid;
 
old_item = list->front->next;
while (old_item) {
prev = new_item;
new_item = llist_item_get();
prev->next = new_item;
-   new_item->sha1 = old_item->sha1;
+   new_item->oid = old_item->oid;
old_item = old_item->next;
}
new_item->next = NULL;
@@ -108,10 +108,10 @@ static struct llist * llist_copy(struct llist *list)
 
 static inline struct llist_item *llist_insert(struct llist *list,
  struct llist_item *after,
-  const unsigned char *sha1)
+ const struct object_id *oid)
 {
struct llist_item *new_item = llist_item_get();
-   new_item->sha1 = sha1;
+   new_item->oid = oid;
new_item->next = NULL;
 
if (after != NULL) {
@@ -131,21 +131,21 @@ static inline struct llist_item *llist_insert(struct 
llist *list,
 }
 
 static inline struct llist_item *llist_insert_back(struct llist *list,
-  const unsigned char *sha1)
+  const struct object_id *oid)
 {
-   return llist_insert(list, list->back, sha1);
+   return llist_insert(list, list->back, oid);
 }
 
 static inline struct llist_item *llist_insert_sorted_unique(struct llist *list,
-   const unsigned char *sha1, struct llist_item *hint)
+   const struct object_id *oid, struct llist_item *hint)
 {
struct llist_item *prev = NULL, *l;
 
l = (hint == NULL) ? list->front : hint;
while (l) {
-   int cmp = hashcmp(l->sha1, sha1);
+   int cmp = oidcmp(l->oid, oid);
if (cmp > 0) { /* we insert before this entry */
-   return llist_insert(list, prev, sha1);
+   return llist_insert(list, prev, oid);
}
if (!cmp) { /* already exists */
return l;
@@ -154,11 +154,11 @@ static inline struct llist_item 
*llist_insert_sorted_unique(struct llist *list,
l = l->next;
}
/* insert at the end */
-   return llist_insert_back(list, sha1);
+   return llist_insert_back(list, oid);
 }
 
 /* returns a pointer to an item in front of sha1 */
-static inline struct llist_item * llist_sorted_remove(struct llist *list, 
const unsigned char *sha1, struct llist_item *hint)
+static inline struct llist_item * llist_sorted_remove(struct llist *list, 
const struct object_id *oid, struct llist_item *hint)
 {
struct llist_item *prev, *l;
 
@@ -166,7 +166,7 @@ static inline struct llist_item * 
llist_sorted_remove(struct llist *list, const
l = (hint == NULL) ? list->front : hint;
prev = NULL;
while (l) {
-   int cmp = hashcmp(l->sha1, sha1);
+   int cmp = oidcmp(l->oid, oid);
if (cmp > 0) /* not in list, since sorted */
return prev;
if (!cmp) { /* found */
@@ -201,7 +201,7 @@ static void llist_sorted_difference_inplace(struct llist *A,
b = B->front;
 
while (b) {
-   hint = llist_sorted_remove(A, b->sha1, hint);
+   hint = llist_sorted_remove(A, b->oid, hint);
b = b->next;
}
 }
@@ -268,9 +268,11 @@ static void cmp_two_packs(struct pack_list *p1, struct 
pack_list *p2)
/* cmp ~ p1 - p2 */
if (cmp == 0) {
p1_hint = llist_sorted_remove(p1->unique_objects,
-   p1_base + p1_off, p1_hint);
+   (const struct object_id *)(p1_base + 
p1_off),
+   p1_hint);
p2_hint = llist_sorted_remove(p2->unique_objects,
-   p1_base 

[PATCH 36/41] sequencer: use the_hash_algo for empty tree object ID

[brian m. carlson]

To ensure that we are hash algorithm agnostic, use the_hash_algo to look
up the object ID for the empty tree instead of using the empty_tree_oid
variable.

Signed-off-by: brian m. carlson 
---
 sequencer.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/sequencer.c b/sequencer.c
index b879593486..12c1e1cdbb 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -1119,7 +1119,7 @@ static int try_to_commit(struct strbuf *msg, const char 
*author,
 
if (!(flags & ALLOW_EMPTY) && !oidcmp(current_head ?
  _head->tree->object.oid :
- _tree_oid, )) {
+ the_hash_algo->empty_tree, 
)) {
res = 1; /* run 'git commit' to display error message */
goto out;
}

[PATCH 21/41] http: eliminate hard-coded constants

[brian m. carlson]

Use the_hash_algo to find the right size for parsing pack names.

Signed-off-by: brian m. carlson 
---
 http.c | 11 ++-
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/http.c b/http.c
index 3034d10b68..ec70676748 100644
--- a/http.c
+++ b/http.c
@@ -2047,7 +2047,8 @@ int http_get_info_packs(const char *base_url, struct 
packed_git **packs_head)
int ret = 0, i = 0;
char *url, *data;
struct strbuf buf = STRBUF_INIT;
-   unsigned char sha1[20];
+   unsigned char hash[GIT_MAX_RAWSZ];
+   const unsigned hexsz = the_hash_algo->hexsz;
 
end_url_with_slash(, base_url);
strbuf_addstr(, "objects/info/packs");
@@ -2063,11 +2064,11 @@ int http_get_info_packs(const char *base_url, struct 
packed_git **packs_head)
switch (data[i]) {
case 'P':
i++;
-   if (i + 52 <= buf.len &&
+   if (i + hexsz + 12 <= buf.len &&
starts_with(data + i, " pack-") &&
-   starts_with(data + i + 46, ".pack\n")) {
-   get_sha1_hex(data + i + 6, sha1);
-   fetch_and_setup_pack_index(packs_head, sha1,
+   starts_with(data + i + hexsz + 6, ".pack\n")) {
+   get_sha1_hex(data + i + 6, hash);
+   fetch_and_setup_pack_index(packs_head, hash,
  base_url);
i += 51;
break;

[PATCH 39/41] Update shell scripts to compute empty tree object ID

[brian m. carlson]

Several of our shell scripts hard-code the object ID of the empty tree.
To avoid any problems when changing hashes, compute this value on
startup of the script.  For performance, store the value in a variable
and reuse it throughout the life of the script.

Signed-off-by: brian m. carlson 
---
 git-filter-branch.sh   | 4 +++-
 git-rebase--interactive.sh | 4 +++-
 templates/hooks--pre-commit.sample | 2 +-
 3 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/git-filter-branch.sh b/git-filter-branch.sh
index 64f21547c1..ccceaf19a7 100755
--- a/git-filter-branch.sh
+++ b/git-filter-branch.sh
@@ -11,6 +11,8 @@
 # The following functions will also be available in the commit filter:
 
 functions=$(cat << \EOF
+EMPTY_TREE=$(git hash-object -t tree /dev/null)
+
 warn () {
echo "$*" >&2
 }
@@ -46,7 +48,7 @@ git_commit_non_empty_tree()
 {
if test $# = 3 && test "$1" = $(git rev-parse "$3^{tree}"); then
map "$3"
-   elif test $# = 1 && test "$1" = 
4b825dc642cb6eb9a060e54bf8d69288fbee4904; then
+   elif test $# = 1 && test "$1" = $EMPTY_TREE; then
:
else
git commit-tree "$@"
diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 50323fc273..cc873d630d 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -81,6 +81,8 @@ rewritten_pending="$state_dir"/rewritten-pending
 # and leaves CR at the end instead.
 cr=$(printf "\015")
 
+empty_tree=$(git hash-object -t tree /dev/null)
+
 strategy_args=${strategy:+--strategy=$strategy}
 test -n "$strategy_opts" &&
 eval '
@@ -238,7 +240,7 @@ is_empty_commit() {
die "$(eval_gettext "\$sha1: not a commit that can be picked")"
}
ptree=$(git rev-parse -q --verify "$1"^^{tree} 2>/dev/null) ||
-   ptree=4b825dc642cb6eb9a060e54bf8d69288fbee4904
+   ptree=$empty_tree
test "$tree" = "$ptree"
 }
 
diff --git a/templates/hooks--pre-commit.sample 
b/templates/hooks--pre-commit.sample
index 68d62d5446..6a75641638 100755
--- a/templates/hooks--pre-commit.sample
+++ b/templates/hooks--pre-commit.sample
@@ -12,7 +12,7 @@ then
against=HEAD
 else
# Initial commit: diff against an empty tree object
-   against=4b825dc642cb6eb9a060e54bf8d69288fbee4904
+   against=$(git hash-object -t tree /dev/null)
 fi
 
 # If you want to allow non-ASCII filenames set this variable to true.

[PATCH 04/41] packfile: remove unused member from struct pack_entry

[brian m. carlson]

The sha1 member in struct pack_entry is unused except for one instance
in which we store a value in it.  Since nobody ever reads this value,
don't bother to compute it and remove the member from struct pack_entry.

Signed-off-by: brian m. carlson 
---
 cache.h| 1 -
 packfile.c | 1 -
 2 files changed, 2 deletions(-)

diff --git a/cache.h b/cache.h
index 11a989319d..dd1a9c6094 100644
--- a/cache.h
+++ b/cache.h
@@ -1572,7 +1572,6 @@ struct pack_window {
 
 struct pack_entry {
off_t offset;
-   unsigned char sha1[20];
struct packed_git *p;
 };
 
diff --git a/packfile.c b/packfile.c
index 0bc67d0e00..5c219d0229 100644
--- a/packfile.c
+++ b/packfile.c
@@ -1833,7 +1833,6 @@ static int fill_pack_entry(const unsigned char *sha1,
return 0;
e->offset = offset;
e->p = p;
-   hashcpy(e->sha1, sha1);
return 1;
 }
 

[PATCH 07/41] packfile: convert find_pack_entry to object_id

[brian m. carlson]

Convert find_pack_entry and the static function fill_pack_entry to take
pointers to struct object_id.

Signed-off-by: brian m. carlson 
---
 packfile.c  | 12 ++--
 packfile.h  |  2 +-
 sha1_file.c |  6 +++---
 3 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/packfile.c b/packfile.c
index e65f943664..84acd405e0 100644
--- a/packfile.c
+++ b/packfile.c
@@ -1805,7 +1805,7 @@ struct packed_git *find_sha1_pack(const unsigned char 
*sha1,
 
 }
 
-static int fill_pack_entry(const unsigned char *sha1,
+static int fill_pack_entry(const struct object_id *oid,
   struct pack_entry *e,
   struct packed_git *p)
 {
@@ -1814,11 +1814,11 @@ static int fill_pack_entry(const unsigned char *sha1,
if (p->num_bad_objects) {
unsigned i;
for (i = 0; i < p->num_bad_objects; i++)
-   if (!hashcmp(sha1, p->bad_object_sha1 + 20 * i))
+   if (!hashcmp(oid->hash, p->bad_object_sha1 + 20 * i))
return 0;
}
 
-   offset = find_pack_entry_one(sha1, p);
+   offset = find_pack_entry_one(oid->hash, p);
if (!offset)
return 0;
 
@@ -1836,7 +1836,7 @@ static int fill_pack_entry(const unsigned char *sha1,
return 1;
 }
 
-int find_pack_entry(struct repository *r, const unsigned char *sha1, struct 
pack_entry *e)
+int find_pack_entry(struct repository *r, const struct object_id *oid, struct 
pack_entry *e)
 {
struct list_head *pos;
 
@@ -1846,7 +1846,7 @@ int find_pack_entry(struct repository *r, const unsigned 
char *sha1, struct pack
 
list_for_each(pos, >objects->packed_git_mru) {
struct packed_git *p = list_entry(pos, struct packed_git, mru);
-   if (fill_pack_entry(sha1, e, p)) {
+   if (fill_pack_entry(oid, e, p)) {
list_move(>mru, >objects->packed_git_mru);
return 1;
}
@@ -1857,7 +1857,7 @@ int find_pack_entry(struct repository *r, const unsigned 
char *sha1, struct pack
 int has_object_pack(const struct object_id *oid)
 {
struct pack_entry e;
-   return find_pack_entry(the_repository, oid->hash, );
+   return find_pack_entry(the_repository, oid, );
 }
 
 int has_pack_index(const unsigned char *sha1)
diff --git a/packfile.h b/packfile.h
index 14ca34bcbd..782029ed07 100644
--- a/packfile.h
+++ b/packfile.h
@@ -134,7 +134,7 @@ extern const struct packed_git *has_packed_and_bad(const 
unsigned char *sha1);
  * Iff a pack file in the given repository contains the object named by sha1,
  * return true and store its location to e.
  */
-extern int find_pack_entry(struct repository *r, const unsigned char *sha1, 
struct pack_entry *e);
+extern int find_pack_entry(struct repository *r, const struct object_id *oid, 
struct pack_entry *e);
 
 extern int has_object_pack(const struct object_id *oid);
 
diff --git a/sha1_file.c b/sha1_file.c
index 1617e25495..4328c61285 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -1268,7 +1268,7 @@ int oid_object_info_extended(const struct object_id *oid, 
struct object_info *oi
}
 
while (1) {
-   if (find_pack_entry(the_repository, real->hash, ))
+   if (find_pack_entry(the_repository, real, ))
break;
 
if (flags & OBJECT_INFO_IGNORE_LOOSE)
@@ -1281,7 +1281,7 @@ int oid_object_info_extended(const struct object_id *oid, 
struct object_info *oi
/* Not a loose object; someone else may have just packed it. */
if (!(flags & OBJECT_INFO_QUICK)) {
reprepare_packed_git(the_repository);
-   if (find_pack_entry(the_repository, real->hash, ))
+   if (find_pack_entry(the_repository, real, ))
break;
}
 
@@ -1669,7 +1669,7 @@ static int freshen_loose_object(const struct object_id 
*oid)
 static int freshen_packed_object(const struct object_id *oid)
 {
struct pack_entry e;
-   if (!find_pack_entry(the_repository, oid->hash, ))
+   if (!find_pack_entry(the_repository, oid, ))
return 0;
if (e.p->freshened)
return 1;

[PATCH 02/41] server-info: remove unused members from struct pack_info

[brian m. carlson]

The head member of struct pack_info is completely unused and the
nr_heads member is used only in one place, which is an assignment.
Since these structure members are not useful, remove them.

Signed-off-by: brian m. carlson 
---
 server-info.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/server-info.c b/server-info.c
index 83460ec0d6..828ec5e538 100644
--- a/server-info.c
+++ b/server-info.c
@@ -92,8 +92,6 @@ static struct pack_info {
int old_num;
int new_num;
int nr_alloc;
-   int nr_heads;
-   unsigned char (*head)[20];
 } **info;
 static int num_pack;
 static const char *objdir;
@@ -228,7 +226,6 @@ static void init_pack_info(const char *infofile, int force)
for (i = 0; i < num_pack; i++) {
if (stale) {
info[i]->old_num = -1;
-   info[i]->nr_heads = 0;
}
}