[PATCH 1/1] Mark messages for translations

[Alexander Shopov]

Small changes in messages to fit the style and typography of rest
Reuse already translated messages if possible
Do not translate messages aimed at developers of git
Fix unit tests depending on the original string
Use `test_i18ngrep` for tests with translatable strings
Change and verifyrest of tests via `make GETTEXT_POISON=1 test`

Signed-off-by: Alexander Shopov 
---
 git.c  | 38 +-
 setup.c| 62 +-
 t/t0002-gitfile.sh |  4 +--
 t/t0008-ignores.sh |  2 +-
 t/t1506-rev-parse-diagnosis.sh |  2 +-
 5 files changed, 54 insertions(+), 54 deletions(-)

diff --git a/git.c b/git.c
index c870b9719..5ddcb75d4 100644
--- a/git.c
+++ b/git.c
@@ -5,11 +5,11 @@
 #include "run-command.h"
 
 const char git_usage_string[] =
-   "git [--version] [--help] [-C ] [-c name=value]\n"
-   "   [--exec-path[=]] [--html-path] [--man-path] 
[--info-path]\n"
-   "   [-p | --paginate | --no-pager] [--no-replace-objects] 
[--bare]\n"
-   "   [--git-dir=] [--work-tree=] 
[--namespace=]\n"
-   "[]";
+   N_("git [--version] [--help] [-C ] [-c =]\n"
+  "   [--exec-path[=]] [--html-path] [--man-path] 
[--info-path]\n"
+  "   [-p | --paginate | --no-pager] [--no-replace-objects] 
[--bare]\n"
+  "   [--git-dir=] [--work-tree=] 
[--namespace=]\n"
+  "[]");
 
 const char git_more_info_string[] =
N_("'git help -a' and 'git help -g' list available subcommands and 
some\n"
@@ -92,7 +92,7 @@ static int handle_options(const char ***argv, int *argc, int 
*envchanged)
*envchanged = 1;
} else if (!strcmp(cmd, "--git-dir")) {
if (*argc < 2) {
-   fprintf(stderr, "No directory given for 
--git-dir.\n" );
+   fprintf(stderr, _("no directory given for 
--git-dir\n" ));
usage(git_usage_string);
}
setenv(GIT_DIR_ENVIRONMENT, (*argv)[1], 1);
@@ -106,7 +106,7 @@ static int handle_options(const char ***argv, int *argc, 
int *envchanged)
*envchanged = 1;
} else if (!strcmp(cmd, "--namespace")) {
if (*argc < 2) {
-   fprintf(stderr, "No namespace given for 
--namespace.\n" );
+   fprintf(stderr, _("no namespace given for 
--namespace\n" ));
usage(git_usage_string);
}
setenv(GIT_NAMESPACE_ENVIRONMENT, (*argv)[1], 1);
@@ -120,7 +120,7 @@ static int handle_options(const char ***argv, int *argc, 
int *envchanged)
*envchanged = 1;
} else if (!strcmp(cmd, "--work-tree")) {
if (*argc < 2) {
-   fprintf(stderr, "No directory given for 
--work-tree.\n" );
+   fprintf(stderr, _("no directory given for 
--work-tree\n" ));
usage(git_usage_string);
}
setenv(GIT_WORK_TREE_ENVIRONMENT, (*argv)[1], 1);
@@ -134,7 +134,7 @@ static int handle_options(const char ***argv, int *argc, 
int *envchanged)
*envchanged = 1;
} else if (!strcmp(cmd, "--super-prefix")) {
if (*argc < 2) {
-   fprintf(stderr, "No prefix given for 
--super-prefix.\n" );
+   fprintf(stderr, _("no prefix given for 
--super-prefix\n" ));
usage(git_usage_string);
}
setenv(GIT_SUPER_PREFIX_ENVIRONMENT, (*argv)[1], 1);
@@ -156,7 +156,7 @@ static int handle_options(const char ***argv, int *argc, 
int *envchanged)
*envchanged = 1;
} else if (!strcmp(cmd, "-c")) {
if (*argc < 2) {
-   fprintf(stderr, "-c expects a configuration 
string\n" );
+   fprintf(stderr, _("-c expects a configuration 
string\n" ));
usage(git_usage_string);
}
git_config_push_parameter((*argv)[1]);
@@ -194,12 +194,12 @@ static int handle_options(const char ***argv, int *argc, 
int *envchanged)
*envchanged = 1;
} else if (!strcmp(cmd, "-C")) {
if (*argc < 2) {
-   fprintf(stderr, "No directory given for -C.\n" 
);
+   fprintf(stderr, _("no directory given for -C\n" 
));

[PATCH 0/1] Mark messages for translations

[Alexander Shopov]

Hi all,

Continuing with ths suggestions for improvements.

Racap since last time:

Eric Sunshine:
  1. Use 'test_i18ngrep' rather than 'grep'
Jeff King:
  2. Fix t1506
  
Previous changes:

Johannes Sixt:
  1. Lower-case letters at the beginning of error messages
  2. Past tense to present tense in some cases
Eric Sunshine:
  3. Fix `-cname=value` to say `-c =`
  4. Do not translate "BUG message"
Duy Nguyen:
  5. Fix parentheses on `_` macro

Kind regards:
al_shopov


Alexander Shopov (1):
  Mark messages for translations

 git.c  | 38 +-
 setup.c| 62 +-
 t/t0002-gitfile.sh |  4 +--
 t/t0008-ignores.sh |  2 +-
 t/t1506-rev-parse-diagnosis.sh |  2 +-
 5 files changed, 54 insertions(+), 54 deletions(-)

-- 
2.16.1

Re: [PATCH 5/8] rebase: introduce the --recreate-merges option

[Johannes Sixt]

Am 09.02.2018 um 07:11 schrieb Sergey Organov:

Johannes Schindelin  writes:

Let me explain the scenario which comes up plenty of times in my work with
Git for Windows. We have a thicket of some 70 branches on top of git.git's
latest release. These branches often include fixup! and squash! commits
and even more complicated constructs that rebase cannot handle at all at
the moment, such as reorder-before! and reorder-after! (for commits that
really need to go into a different branch).


I sympathize, but a solution that breaks even in simple cases can't be
used reliably to solve more complex problems, sorry. Being so deep
into your problems, I think you maybe just aren't seeing forest for the
trees [1].


Hold your horses! Dscho has a point here. --preserve-merges 
--first-parent works only as long as you don't tamper with the side 
branches. If you make changes in the side branches during the same 
rebase operation, this --first-parent mode would undo that change. (And, 
yes, its result would be called an "evil merge", and that scary name 
_should_ frighten you!)


-- Hannes

Re: [PATCH v2 17/17] fetch: make the --fetch-prune work with

[Eric Sunshine]

On Thu, Feb 8, 2018 at 11:19 AM, Ævar Arnfjörð Bjarmason
 wrote:
> fetch: make the --fetch-prune work with 

Do you mean s/--fetch-prune/--prune-tags/ ?

> Make the new --prune-tags option work properly when git-fetch is
> invoked with a  parameter instead of a 
> parameter.
>
> This change is split off from the introduction of --prune-tags due to
> the relative complexity of munging the incoming argv, which is easier
> to review as a separate change.
>
> Signed-off-by: Ævar Arnfjörð Bjarmason 

Re: [PATCH v2 16/17] fetch: add a --fetch-prune option and fetch.pruneTags config

[Eric Sunshine]

On Thu, Feb 8, 2018 at 11:19 AM, Ævar Arnfjörð Bjarmason
 wrote:
> Add a --fetch-prune option to git-fetch, along with fetch.pruneTags
> config option. [...]
>
> Signed-off-by: Ævar Arnfjörð Bjarmason 
> ---
> diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh
> @@ -592,6 +592,15 @@ test_configured_prune_type () {
> +   if test "$fetch_prune_tags" = 'true' ||
> +  test "$remote_origin_prune_tags" = 'true'
> +   then
> +   if ! printf '%s' "$cmdline" | grep -q 
> refs/remotes/origin/

Is $cmdline guaranteed to end with a newline? Historically, not all
'grep's would be able to match the last line if it was not properly
terminated. Perhaps you want '%s\n' instead?

> +   then
> +   new_cmdline="$new_cmdline 
> refs/tags/*:refs/tags/*"
> +   fi
> +   fi
> +
> @@ -705,6 +714,66 @@ test_configured_prune true  true  unset unset kept   
> pruned \
> +# When --prune-tags is supplied it's ignored if an explict refspec is

s/explict/explicit/

> +# given, same for the configuration options.
> +
> +# Pruning that also takes place if s!origin!!,
> +# or otherwise uses the file://-specific codepath. However, because
> +# there's no implicit +refs/heads/*:refs/remotes/origin/* refspec and
> +# supplying it on the command-line negate --prune-tags the branches

s/negate//
s/--prune-tags/&,/

> +# will not be pruned.

Re: [PATCH 5/8] rebase: introduce the --recreate-merges option

[Sergey Organov]

Johannes Schindelin  writes:

[...]

> With this patch, the goodness of the Git garden shears comes to `git
> rebase -i` itself. Passing the `--recreate-merges` option will generate
> a todo list that can be understood readily, and where it is obvious
> how to reorder commits. New branches can be introduced by inserting
> `label` commands and calling `merge -  `. And once this
> mode has become stable and universally accepted, we can deprecate the
> design mistake that was `--preserve-merges`.

This doesn't explain why you introduced this new --recreate-merges. Why
didn't you rather fix --preserve-merges to generate and use new todo
list format?

It doesn't seem likely that todo list created by one Git version is to
be ever used by another, right? Is there some hidden reason here? Some
tools outside of Git that use old todo list format, maybe?

Then, if new option indeed required, please look at the resulting manual:

--recreate-merges::
Recreate merge commits instead of flattening the history by replaying
merges. Merge conflict resolutions or manual amendments to merge
commits are not preserved.

-p::
--preserve-merges::
Recreate merge commits instead of flattening the history by replaying
commits a merge commit introduces. Merge conflict resolutions or manual
amendments to merge commits are not preserved.


Don't you think more explanations are needed there in the manual on
why do we have 2 separate options with almost the same yet subtly
different description? Is this subtle difference even important? How?

I also have trouble making sense of "Recreate merge commits instead of
flattening the history by replaying merges." Is it " instead of " or is it
rather " instead of ?

-- Sergey

Re: [PATCH 5/8] rebase: introduce the --recreate-merges option

[Sergey Organov]

Hi,

Johannes Schindelin  writes:
> On Wed, 7 Feb 2018, Sergey Organov wrote:
>> Johannes Schindelin  writes:
>> > +--recreate-merges::
>> > +  Recreate merge commits instead of flattening the history by replaying
>> > +  merges. Merge conflict resolutions or manual amendments to merge
>> > +  commits are not preserved.
>> 
>> I wonder why you guys still hold on replaying "merge-the-operation"
>> instead of replaying "merge-the-result"?
>
> This misses the point of rebasing: you want to replay the changes.

What this comment has to do with the statement to which it's supposed to
be a reply? Sounds like topic change to me. Please clarify if it isn't.

>
>> The latter, the merge commit itself, no matter how exactly it was
>> created in the first place, is the most valuable thing git keeps about
>> the merge, and you silently drop it entirely!
>
> You miss another very crucial point.

What was the first crucial point I miss? Do you rather agree that the
point you are replying to with this is very crucial one as well?

> I don't blame you, as you certainly have not used the Git garden
> shears for years.

Thanks a lot!

> Let me explain the scenario which comes up plenty of times in my work with
> Git for Windows. We have a thicket of some 70 branches on top of git.git's
> latest release. These branches often include fixup! and squash! commits
> and even more complicated constructs that rebase cannot handle at all at
> the moment, such as reorder-before! and reorder-after! (for commits that
> really need to go into a different branch).

I sympathize, but a solution that breaks even in simple cases can't be
used reliably to solve more complex problems, sorry. Being so deep
into your problems, I think you maybe just aren't seeing forest for the
trees [1].

> Even if you do not have such a complicated setup, it is quite possible
> that you need to include a commit in your development that needs to be
> dropped before contributing your work. Think e.g. removing the `-O2` flag
> when compiling with GCC because GDB gets utterly confused with executables
> compiled with `-O2` while single-stepping. This could be an initial commit
> called `TO-DROP` or some such.
>
> And guess what happens if you drop that `pick` line in your todo list and
> then the `merge` command simply tries to re-create the original merge
> commit's changes?
>
> Exactly. The merge will become an evil merge, and will introduce that very
> much not-wanted and therefore-dropped changes.

Okay, Houston, we've had a problem here.

I'm sure you'll be able to come-up with suitable solution once you start
to think about it positively, but automatic unguided silent re-merge is
still not the right answer, for the same reason of distortion of user
changes.

As for "evil merges"... I don't want to get too far from original
subject to even start discussing this.

>> OTOH, git keeps almost no information about "merge-the-operation", so
>> it's virtually impossible to reliably replay the operation
>> automatically, and yet you try to.
>
> That is true. However, the intended use case is not to allow you to
> recreate funny merges. Its use case is to allow you to recreate
> merges.

Then it at least should behave accordingly, e.g., stop after every such
occurrence, for user assistance. As an example, see what rerere does
when it fires, even though it's much more reliable than this blind
re-merge.

But the actual problem here is that almost any merge but those made with
pure "git merge", no options, no conflicts, no edits, no nothing,
becomes "funny" and is being destroyed, sometimes in a weird way, by
silently creating something different instead of original.

> At a later stage, I might introduce support to detect `-s ours` merges,
> because they are easy to detect. But even then, it will be an opt-in.

So you are going to fix one particular case that is "easy to detect"
(and fix). Does it mean you do realize it's a problem, but fail to see that
it's _fundamental_ problem with current approach?

I think you start from the wrong end. I think that any merge should be
made reproducible first (with possible guidance from the user when
required, as usual), and then advanced features for complex history
tweaking should come, not the other way around.

I feel that any solution that fails to exactly reproduce original
history, unless it is to be actually changed, is flawed, and we will
continue to hit our heads on sharp corners like "merge -s ours", most of
which will be not that simple to detect and fix, for an unforeseeable
future.

>
>> IMHO that was severe mistake in the original --preserve-merges, and you
>> bring with you to this new --recreate-merges... It's sad.
>
> Please refrain from drawing this discussion into an emotional direction.
> That is definitely not helpful.

I don't actually blame anybody for that original implementation in the
first place. It was made based on the actual needs, and that's 

Re: [PATCH v2] name-hash: properly fold directory names in adjust_dirname_case()

[Torsten Bögershausen]

On Thu, Feb 08, 2018 at 02:23:33PM -0500, Ben Peart wrote:

[]

> -
> +test_expect_success CASE_INSENSITIVE_FS 'add directory (with different 
> case)' '
> + git reset --hard initial &&
> + mkdir -p dir1/dir2 &&
> + echo > dir1/dir2/a &&
> + echo > dir1/dir2/b &&

Thanks for working on this-
One and a half style nits:
The ">" should be close to the file name:
echo >dir1/dir2/a &&
But then we don't even look into the file, so that the "\" produced by echo is 
not needed.
A simple 
>dir1/dir2/a &&
is all that is needed.

Re: [PATCH v2 13/17] git remote doc: correct dangerous lies about what prune does

[Eric Sunshine]

On Thu, Feb 8, 2018 at 11:19 AM, Ævar Arnfjörð Bjarmason
 wrote:
> The "git remote prune " command uses the same machinery as "git
> fetch  --prune", and shares all the same caveats, but its
> documentation has suggested that it'll just "delete stale
> remote-tracking branches under ".
>
> This isn't true, and hasn't been true since at least v1.8.5.6 (the
> oldest version I could be bothered to test).
>
> E.g. if "refs/tags/*:refs/tags/*" is explicitly set in the refspec of
> the remote it'll delete all local tags  doesn't know about.

Not worth a re-roll as this is only the commit message: s/remote/&,/

More below...

> Instead, briefly give the reader just enough of a hint that this
> option might constitute a shotgun aimed at their foot, and point them
> to the new PRUNING section in the git-fetch documentation which
> explains all the nuances of what this facility does.
> [...]
> Signed-off-by: Ævar Arnfjörð Bjarmason 
> ---
> diff --git a/Documentation/git-remote.txt b/Documentation/git-remote.txt
> @@ -172,10 +172,14 @@ With `-n` option, the remote heads are not queried 
> first with
> -Deletes all stale remote-tracking branches under .
> -These stale branches have already been removed from the remote repository
> -referenced by , but are still locally available in
> -"remotes/".
> +Deletes stale references associated with . By default stale

Nit: s/default/&,/

> +remote-tracking branches under , but depending on global
> +configuration and the configuration of the remote we might even prune
> +local tags that haven't been pushed there.

This is a bit difficult to digest grammatically due to the phrase
being incomplete. It could say either:

By default, stale ... under  _are deleted_, but
depending...

or:

Deletes stale ... ; by default, stale...

Re: [PATCH v2 12/17] git fetch doc: add a new section to explain the ins & outs of pruning

[Eric Sunshine]

On Thu, Feb 8, 2018 at 11:19 AM, Ævar Arnfjörð Bjarmason
 wrote:
> Add a new section to canonically explain how remote reference pruning
> works, and how users should be careful about using it in conjunction
> with tag refspecs in particular.
> [...]
> Signed-off-by: Ævar Arnfjörð Bjarmason 
> ---
> diff --git a/Documentation/git-fetch.txt b/Documentation/git-fetch.txt
> @@ -99,6 +99,55 @@ The latter use of the `remote..fetch` values 
> can be
> +PRUNING
> +---
> +[...]
> +If left to accumulate, these stale references might make performance
> +worse on big and busy repos that have a lot of branch churn, and
> +e.g. make the output of commands like `git branch -a --contains
> +` needlessly verbose, as well as impacting anything else
> +that'll work with the complete set of known references.
> +
> +These remote tracking references can be deleted as a one-off with

I think we call these "remote-tracking" (note the hyphen), which are
local but track something remote, rather than "remote tracking" (no
hyphen) which would themselves be remote.

> +either of:
> +
> +
> +# While fetching
> +$ git fetch --prune 
> +
> +# Only prune, don't fetch
> +$ git remote prune 
> +
> +
> +To prune references as part of your normal workflow without needing to
> +remember to run that set `fetch.prune` globally, or

s/that/&,/

> +`remote..prune` per-remote in the config. See
> +linkgit:git-config[1].

Re: [PATCH v2 11/17] fetch tests: fetch as well as fetch []

[Eric Sunshine]

On Thu, Feb 8, 2018 at 11:19 AM, Ævar Arnfjörð Bjarmason
 wrote:
> When a remote URL is supplied on the command-line the internals of the
> fetch are different, in particular the code in get_ref_map(). An
> earlier version of the subsequent fetch.pruneTags patch hid a segfault
> because the difference wasn't tested for.
>
> Now all the tests are run as both of the variants of:
>
> git fetch
> git -c [...] fetch $(git config remote.origin.url) $(git config 
> remote.origin.fetch)
>
> I'm using -c because while the [fetch] config just set by
> set_config_tristate will be picked up, the remote.origin.* config
> won't override it as intended.
>
> Work around that and turn this into a purely command-line test by
> always setting the variables on the command-line, and translate any
> setting of remote.origin.X into fetch.X.
> [...]
> Signed-off-by: Ævar Arnfjörð Bjarmason 
> ---
> diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh
> @@ -548,18 +548,52 @@ set_config_tristate () {
> *)
> git config "$1" "$2"
> +   key=$(echo $1 | sed -e 's/^remote\.origin/fetch/')

Faster (thus more Windows-friendly) assuming that $1 always starts
with "remote.origin":

key=fetch${u#remote.origin}

> +   git_fetch_c="$git_fetch_c -c $key=$2"
> ;;
> esac
>  }
>
> +test_configured_prune_type () {
> fetch_prune=$1
> remote_origin_prune=$2
> expected_branch=$3
> expected_tag=$4
> cmdline=$5
> -
> -   test_expect_success "prune fetch.prune=$1 remote.origin.prune=$2${5:+ 
> $5}; branch:$3 tag:$4" '
> +   mode=$6
> +
> +   if ! test -e prune-type-setup-done
> +   then
> +   test_expect_success 'prune_type setup' '
> +   git -C one config remote.origin.url >one.remote-url &&
> +   git -C one config remote.origin.fetch 
> >one.remote-fetch &&
> +   remote_url="file://$(cat one.remote-url)" &&
> +   remote_fetch="$(cat one.remote-fetch)" &&

Is there a reason that these values need to be captured to files
(which are otherwise not used) before being assigned to variables?
That is, wouldn't this work?

remote_url="file://$(git -C one config remote.origin.url)" &&
remote_fetch="$(git -C one config remote.origin.fetch)" &&

> +   cmdline_setup="\"$remote_url\" \"$remote_fetch\"" &&
> +   touch prune-type-setup-done

Why does "prune-type-setup-done" need to be a file rather than a
simple shell variable (which is global by default even when assigned
inside test_expect_success)?

Also, since the purpose of this code seems to compute 'cmdline_setup'
just once, can't you do away with 'prune-type-setup-done' altogether
and change:

if ! test -e prune-type-setup-done

to:

if test -z "$cmdline_setup"

> +   '
> +   fi

Re: [PATCH v2 02/17] fetch: trivially refactor assignment to ref_nr

[Eric Sunshine]

On Thu, Feb 8, 2018 at 11:19 AM, Ævar Arnfjörð Bjarmason
 wrote:
> Trivially refactor an assignment to make a subsequent patch
> smaller. The "ref_nr" variable is initialized to 0 earlier, just as
> "j" is, and "j" is only incremented in that loop, so this change isn't
> a logic error.
>
> This change makes a subsequent change which splits the incrementing of
> "ref_nr" into two blocks.

"This change _simplifies_ a subsequent..."

or

"This change make a subsequent change _easier_..."

> Signed-off-by: Ævar Arnfjörð Bjarmason 

Re: [PATCH v2 01/17] fetch: don't redundantly NULL something calloc() gave us

[Eric Sunshine]

On Thu, Feb 8, 2018 at 11:19 AM, Ævar Arnfjörð Bjarmason
 wrote:
> Stop redundantly NULL-ing the last element of the refs structure,
> which was retrieved via calloc(), and is thus guaranteed to be
> pre-NULL'd.
> [...]
> Signed-off-by: Ævar Arnfjörð Bjarmason 
> ---
> diff --git a/builtin/fetch.c b/builtin/fetch.c
> @@ -1302,7 +1302,6 @@ static int fetch_one(struct remote *remote, int argc, 
> const char **argv)
> } else
> refs[j++] = argv[i];
> }
> -   refs[j] = NULL;
> ref_nr = j;
> }

This is purely subjective, and I neglected to mention it as early as
v1, but I find that this change hurts readability. Specifically, as
I'm scanning or reading code, explicit termination conditions, like
this NULL assignment, are things I'm expecting to see; they're part of
the idiom of the language. When they're missing, I have to stop, go
back, and study the code more carefully to see if the "missing bit" is
a bug or is intentional. And, it's easy to misread xcalloc() as
xmalloc(), meaning that it's likely that one studying the code would
conclude that the missing NULL assignment is a bug.

If anything, if this patch wants to eliminate redundancy, I'd expect
it to change xcalloc() to xmalloc() and keep the NULL assignment, thus
leaving the idiom intact.

Re: [PATCH 3/3] t1404: use 'test_must_fail stderr='

[SZEDER Gábor]

On Fri, Feb 9, 2018 at 4:16 AM, Eric Sunshine  wrote:
> On Thu, Feb 8, 2018 at 9:42 PM, SZEDER Gábor  wrote:
>> Instead of 'test_must_fail git cmd... 2>output.err', which redirects
>> the standard error of the 'test_must_fail' helper function as well,
>> causing various issues as discussed in the previous patch.
>
> ECANTPARSE: This either wants to say:
>
> "Instead of , do ."

The "do " part is in the subject line.

> or:
>
> "'test_must_fail ... 2>...', which redirects ...
>  causes various issues discussed..."

Well, at this point I got the ECANTPARSE :)

> but fails to say either.
>
>> With this patch t1404 succeeds when run with '-x', even with shells
>> not supporting $BASH_XTRACEFD.
>>
>> Signed-off-by: SZEDER Gábor 

Re: [PATCH 3/3] t1404: use 'test_must_fail stderr='

[Eric Sunshine]

On Thu, Feb 8, 2018 at 9:42 PM, SZEDER Gábor  wrote:
> Instead of 'test_must_fail git cmd... 2>output.err', which redirects
> the standard error of the 'test_must_fail' helper function as well,
> causing various issues as discussed in the previous patch.

ECANTPARSE: This either wants to say:

"Instead of , do ."

or:

"'test_must_fail ... 2>...', which redirects ...
 causes various issues discussed..."

but fails to say either.

> With this patch t1404 succeeds when run with '-x', even with shells
> not supporting $BASH_XTRACEFD.
>
> Signed-off-by: SZEDER Gábor 

Re: [PATCH 2/3] t: teach 'test_must_fail' to save the command's stderr to a file

[Eric Sunshine]

On Thu, Feb 8, 2018 at 9:42 PM, SZEDER Gábor  wrote:
> To check that a git command fails and to inspect its error message we
> usually execute a command like this throughout our test suite:
>
>   test_must_fail git command --option 2>output.err
>
> Note that this command doesn't limit the redirection to the git
> command, but it redirects the standard error of the 'test_must_fail'
> helper function as well.  This is wrong for several reasons:
>
>   [...snip rationale...]
>
> Signed-off-by: SZEDER Gábor 
> ---
> diff --git a/t/README b/t/README
> @@ -430,6 +430,10 @@ Don't:
> use 'test_must_fail git cmd'.  This will signal a failure if git
> dies in an unexpected way (e.g. segfault).
>
> +   Don't redirect 'test_must_fail's standard error like
> +   'test_must_fail git cmd 2>err'.  Instead, run 'test_must_fail
> +   stderr=err git cmd'.

Perhaps not worth a re-roll, but it might be nice to cite a bit of the
rationale from the commit message for the "don't redirect" rule since
it's not necessarily obvious for readers of t/README why this
limitation exists (and it seems unlikely they would check this commit
message). The rationale here doesn't necessarily need to go into
exquisite detail of the commit message, but could be perhaps as simple
as:

Don't redirect 'test_must_fail's standard error to a file (e.g.
'test_must_fail git cmd 2>err') since error output from commands
run internally by 'test_must_fail' may pollute file "err".
Instead, run 'test_must_fail stderr=err git cmd'.

[PATCH 1/3] t: document 'test_must_fail ok='

[SZEDER Gábor]

Since 'test_might_fail' is implemented as a thin wrapper around
'test_must_fail', it also accepts the same options.  Mention this in
the docs as well.

Signed-off-by: SZEDER Gábor 
---
 t/README| 14 --
 t/test-lib-functions.sh | 10 ++
 2 files changed, 22 insertions(+), 2 deletions(-)

diff --git a/t/README b/t/README
index b3f7b449c3..1a1361a806 100644
--- a/t/README
+++ b/t/README
@@ -655,7 +655,7 @@ library for your script to use.
test_expect_code 1 git merge "merge msg" B master
'
 
- - test_must_fail 
+ - test_must_fail [] 
 
Run a git command and ensure it fails in a controlled way.  Use
this instead of "! ".  When git-command dies due to a
@@ -663,11 +663,21 @@ library for your script to use.
treats it as just another expected failure, which would let such a
bug go unnoticed.
 
- - test_might_fail 
+   Accepts the following options:
+
+ ok=[,<...>]:
+   Don't treat an exit caused by the given signal as error.
+   Multiple signals can be specified as a comma separated list.
+   Currently recognized signal names are: sigpipe, success.
+   (Don't use 'success', use 'test_might_fail' instead.)
+
+ - test_might_fail [] 
 
Similar to test_must_fail, but tolerate success, too.  Use this
instead of " || :" to catch failures due to segv.
 
+   Accepts the same options as test_must_fail.
+
  - test_cmp  
 
Check whether the content of the  file matches the
diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index 1701fe2a06..26b149ac1d 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -610,6 +610,14 @@ list_contains () {
 #
 # Writing this as "! git checkout ../outerspace" is wrong, because
 # the failure could be due to a segv.  We want a controlled failure.
+#
+# Accepts the following options:
+#
+#   ok=[,<...>]:
+# Don't treat an exit caused by the given signal as error.
+# Multiple signals can be specified as a comma separated list.
+# Currently recognized signal names are: sigpipe, success.
+# (Don't use 'success', use 'test_might_fail' instead.)
 
 test_must_fail () {
case "$1" in
@@ -656,6 +664,8 @@ test_must_fail () {
 #
 # Writing "git config --unset all.configuration || :" would be wrong,
 # because we want to notice if it fails due to segv.
+#
+# Accepts the same options as test_must_fail.
 
 test_might_fail () {
test_must_fail ok=success "$@"
-- 
2.16.1.180.g07550b0b1b

[PATCH 0/3] Teach 'test_must_fail' to save the command's stderr to a file

[SZEDER Gábor]

To check that a git command fails with the expected error message, we 
usually execute a command like this:

  test_must_fail git command --option 2>output.err
  
Alas, this command doesn't limit the redirection to the git command,
but it redirects the standard error of the 'test_must_fail' helper
function as well, causing various issues discussed in detail in the
second patch.  Therefore that patch introduces the 'test_must_fail
stderr=' option to save the executed git command's standard
error to the given file.

The last patch converts one test script to use 'test_must_fail
stderr=' to demonstrate its benefits: thereafter that script
will succeed with '-x'.  There are plenty more places to convert:

  $ git grep -E 'test_(must|might)_fail .* 2>' t/*.sh |wc -l
  430
  $ git grep --name-only -E 'test_(must|might)_fail .* 2>' t/*.sh |wc -l
  135

... and this doesn't even count commands spanning more lines, and
there are more in 'pu'.

I didn't convert more test scripts, because it's boring ;) but more
importantly because it could give us 135+ GSoC micro projects.

SZEDER Gábor (3):
  t: document 'test_must_fail ok='
  t: teach 'test_must_fail' to save the command's stderr to a file
  t1404: use 'test_must_fail stderr='

 t/README | 20 +--
 t/t1404-update-ref-errors.sh | 46 ++--
 t/test-lib-functions.sh  | 45 +--
 3 files changed, 76 insertions(+), 35 deletions(-)

-- 
2.16.1.180.g07550b0b1b

[PATCH 3/3] t1404: use 'test_must_fail stderr='

[SZEDER Gábor]

Instead of 'test_must_fail git cmd... 2>output.err', which redirects
the standard error of the 'test_must_fail' helper function as well,
causing various issues as discussed in the previous patch.

With this patch t1404 succeeds when run with '-x', even with shells
not supporting $BASH_XTRACEFD.

Signed-off-by: SZEDER Gábor 
---
 t/t1404-update-ref-errors.sh | 46 ++--
 1 file changed, 23 insertions(+), 23 deletions(-)

diff --git a/t/t1404-update-ref-errors.sh b/t/t1404-update-ref-errors.sh
index 3a887b5113..91250bc6a0 100755
--- a/t/t1404-update-ref-errors.sh
+++ b/t/t1404-update-ref-errors.sh
@@ -26,7 +26,7 @@ test_update_rejected () {
git pack-refs --all
fi &&
printf "create $prefix/%s $C\n" $create >input &&
-   test_must_fail git update-ref --stdin output.err &&
+   test_must_fail stderr=output.err git update-ref --stdin actual &&
test_cmp unchanged actual
@@ -102,7 +102,7 @@ df_test() {
else
printf "%s\n" "delete $delname" "create $addname $D"
fi >commands &&
-   test_must_fail git update-ref --stdin output.err &&
+   test_must_fail stderr=output.err git update-ref --stdin expected-refs &&
git for-each-ref --format="%(objectname) %(refname)" $prefix/r 
>actual-refs &&
@@ -337,7 +337,7 @@ test_expect_success 'missing old value blocks update' '
fatal: cannot lock ref $Q$prefix/foo$Q: unable to resolve reference 
$Q$prefix/foo$Q
EOF
printf "%s\n" "update $prefix/foo $E $D" |
-   test_must_fail git update-ref --stdin 2>output.err &&
+   test_must_fail stderr=output.err git update-ref --stdin &&
test_cmp expected output.err
 '
 
@@ -348,7 +348,7 @@ test_expect_success 'incorrect old value blocks update' '
fatal: cannot lock ref $Q$prefix/foo$Q: is at $C but expected $D
EOF
printf "%s\n" "update $prefix/foo $E $D" |
-   test_must_fail git update-ref --stdin 2>output.err &&
+   test_must_fail stderr=output.err git update-ref --stdin &&
test_cmp expected output.err
 '
 
@@ -359,7 +359,7 @@ test_expect_success 'existing old value blocks create' '
fatal: cannot lock ref $Q$prefix/foo$Q: reference already exists
EOF
printf "%s\n" "create $prefix/foo $E" |
-   test_must_fail git update-ref --stdin 2>output.err &&
+   test_must_fail stderr=output.err git update-ref --stdin &&
test_cmp expected output.err
 '
 
@@ -370,7 +370,7 @@ test_expect_success 'incorrect old value blocks delete' '
fatal: cannot lock ref $Q$prefix/foo$Q: is at $C but expected $D
EOF
printf "%s\n" "delete $prefix/foo $D" |
-   test_must_fail git update-ref --stdin 2>output.err &&
+   test_must_fail stderr=output.err git update-ref --stdin &&
test_cmp expected output.err
 '
 
@@ -381,7 +381,7 @@ test_expect_success 'missing old value blocks indirect 
update' '
fatal: cannot lock ref $Q$prefix/symref$Q: unable to resolve reference 
$Q$prefix/foo$Q
EOF
printf "%s\n" "update $prefix/symref $E $D" |
-   test_must_fail git update-ref --stdin 2>output.err &&
+   test_must_fail stderr=output.err git update-ref --stdin &&
test_cmp expected output.err
 '
 
@@ -393,7 +393,7 @@ test_expect_success 'incorrect old value blocks indirect 
update' '
fatal: cannot lock ref $Q$prefix/symref$Q: is at $C but expected $D
EOF
printf "%s\n" "update $prefix/symref $E $D" |
-   test_must_fail git update-ref --stdin 2>output.err &&
+   test_must_fail stderr=output.err git update-ref --stdin &&
test_cmp expected output.err
 '
 
@@ -405,7 +405,7 @@ test_expect_success 'existing old value blocks indirect 
create' '
fatal: cannot lock ref $Q$prefix/symref$Q: reference already exists
EOF
printf "%s\n" "create $prefix/symref $E" |
-   test_must_fail git update-ref --stdin 2>output.err &&
+   test_must_fail stderr=output.err git update-ref --stdin &&
test_cmp expected output.err
 '
 
@@ -417,7 +417,7 @@ test_expect_success 'incorrect old value blocks indirect 
delete' '
fatal: cannot lock ref $Q$prefix/symref$Q: is at $C but expected $D
EOF
printf "%s\n" "delete $prefix/symref $D" |
-   test_must_fail git update-ref --stdin 2>output.err &&
+   test_must_fail stderr=output.err git update-ref --stdin &&
test_cmp expected output.err
 '
 
@@ -428,7 +428,7 @@ test_expect_success 'missing old value blocks indirect 
no-deref update' '
fatal: cannot lock ref $Q$prefix/symref$Q: reference is missing but 
expected $D
EOF
printf "%s\n" "option no-deref" "update $prefix/symref $E $D" |
-   test_must_fail git update-ref --stdin 2>output.err &&
+   test_must_fail stderr=output.err git update-ref --stdin &&
test_cmp expected output.err
 '
 
@@ -440,7 +440,7 @@ test_expect_success 

[PATCH 2/3] t: teach 'test_must_fail' to save the command's stderr to a file

[SZEDER Gábor]

To check that a git command fails and to inspect its error message we
usually execute a command like this throughout our test suite:

  test_must_fail git command --option 2>output.err

Note that this command doesn't limit the redirection to the git
command, but it redirects the standard error of the 'test_must_fail'
helper function as well.  This is wrong for several reasons:

  - If that git command were to succeed or die for an unexpected
reason e.g. signal, then 'test_must_fail's helpful error message
would end up in the given file instead of on the terminal or in
't/test-results/$T.out', when the test is run with '-v' or
'--verbose-log', respectively.

  - If the test is run with '-x', the trace of executed commands in
'test_must_fail' will go to stderr as well (except for more recent
Bash versions supporting $BASH_XTRACEFD), and then in turn will
end up in the given file.

  - If the git command's error message is checked verbatim with
'test_cmp', then the '-x' trace will cause the test to fail.

  - Though rather unlikely, if the git command's error message is
checked with 'test_i18ngrep', then its pattern might match
'test_must_fail's error message (or '-x' trace) instead of the
given git command's, potentially hiding a bug.

Teach the 'test_must_fail' helper the 'stderr=' option to save
the executed git command's standard error to that file, so we can
avoid all the bad side effects of redirecting the whole thing's
standard error.

The same issues apply to the 'test_might_fail' helper function as
well, but since it's implemented as a thin wrapper around
'test_must_fail', no modifications were necessary and 'test_might_fail
stderr=output.err git command' will just work.

Signed-off-by: SZEDER Gábor 
---

Notes:
I considered adding an analogous 'stdout=' option as well, but in
the end haven't, because:

  - 'test_must_fail' doesn't print anything to its standard output, so a
plain '>out' redirection at the end of the line doesn't have any
undesirable side effects.

  - We would need four conditions to cover all possible combinations of
'stdout=' and 'stderr='.  Considering the above point, I don't think
it's worth it.

 t/README|  6 ++
 t/test-lib-functions.sh | 35 +--
 2 files changed, 31 insertions(+), 10 deletions(-)

diff --git a/t/README b/t/README
index 1a1361a806..2528359e9d 100644
--- a/t/README
+++ b/t/README
@@ -430,6 +430,10 @@ Don't:
use 'test_must_fail git cmd'.  This will signal a failure if git
dies in an unexpected way (e.g. segfault).
 
+   Don't redirect 'test_must_fail's standard error like
+   'test_must_fail git cmd 2>err'.  Instead, run 'test_must_fail
+   stderr=err git cmd'.
+
On the other hand, don't use test_must_fail for running regular
platform commands; just use '! cmd'.  We are not in the business
of verifying that the world given to us sanely works.
@@ -670,6 +674,8 @@ library for your script to use.
Multiple signals can be specified as a comma separated list.
Currently recognized signal names are: sigpipe, success.
(Don't use 'success', use 'test_might_fail' instead.)
+ stderr=:
+   Save 's standard error to .
 
  - test_might_fail [] 
 
diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index 26b149ac1d..d2f561477c 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -618,18 +618,33 @@ list_contains () {
 # Multiple signals can be specified as a comma separated list.
 # Currently recognized signal names are: sigpipe, success.
 # (Don't use 'success', use 'test_might_fail' instead.)
+#   stderr=:
+# Save the git command's standard error to .
 
 test_must_fail () {
-   case "$1" in
-   ok=*)
-   _test_ok=${1#ok=}
-   shift
-   ;;
-   *)
-   _test_ok=
-   ;;
-   esac
-   "$@"
+   _test_ok= _test_stderr=
+   while test $# -gt 0
+   do
+   case "$1" in
+   stderr=*)
+   _test_stderr=${1#stderr=}
+   shift
+   ;;
+   ok=*)
+   _test_ok=${1#ok=}
+   shift
+   ;;
+   *)
+   break
+   ;;
+   esac
+   done
+   if test -n "$_test_stderr"
+   then
+   "$@" 2>"$_test_stderr"
+   else
+   "$@"
+   fi
exit_code=$?
if test $exit_code -eq 0 && ! list_contains "$_test_ok" success
then
-- 
2.16.1.180.g07550b0b1b

Re: bad behaviour while using git rebase -i -p

[Junio C Hamano]

Jan Viktorin  writes:

> since Git 2.16.1, I've noticed a bad behaviour of git rebase -i -p. It
> screws up merge commits created with --log (or config merge.log = true)
> in my history.  A good merge commit with message like:
>
> Merge branch 'test'
> 
> * test:
>   c
>   b
>
> is changed after rebase (without touching that commit in any way) into:
>
> Merge branch 'test' a git-rebase-p-test.sh test: c b

I think we saw this problem independently discovered and a fix for
it posted today to the list.


https://public-inbox.org/git/20180208204241.19324-1-gregory.herr...@oracle.com/

Thanks.

I await your response for more close discussions.

[Mr Jeremiah]

Hello dear ,
I am Mr. Jeremiah Bworo , nationality of Ethiopia but currently In Benin 
Republic as a Principal Director with Federal Ministry Of Works and Housing 
here .

I will be delighted to invest with you there in your country into lucrative 
investments under your management as far as you could promise me of your 
country's economic stability . 

I await your response for more close discussions.

Regards,
Mr. Jeremiah Bworo
Principal director
Ministry of Works and Housing 
Benin Republic.

bad behaviour while using git rebase -i -p

[Jan Viktorin]

Hello,

since Git 2.16.1, I've noticed a bad behaviour of git rebase -i -p. It
screws up merge commits created with --log (or config merge.log = true)
in my history. A good merge commit with message like:

Merge branch 'test'

* test:
  c
  b

is changed after rebase (without touching that commit in any way) into:

Merge branch 'test' a git-rebase-p-test.sh test: c b

It seems, like the commit message is interpreted somehow - the '*'
character is expanded to the list of files in the current directory and
the original spacing is removed. This happens during my regular work.

Here is a code that seems to be reproducing this behaviour well:

git init

touch a
git add a
git commit -m a

git checkout -b test
# a new branch made to merge back to master later

touch b
git add b
git commit -m b

touch c
git add c
git commit -m c

git checkout master
git merge --no-edit --log test
git log -1
# everything looks good at this point

export GIT_SEQUENCE_EDITOR='sed "1s/pick/reword/" -i'
# we are rewording only the first commit...
export EDITOR='sed "s/b/x/" -i'
# ...and changing its message from "b" to "x"

git rebase -i HEAD^1 -p
git log -1
# here, you can see the bad merge commit message

Regards
Jan

Re: [PATCH v3 06/14] commit-graph: implement 'git-commit-graph read'

[Junio C Hamano]

Derrick Stolee  writes:

> +'read'::
> +
> +Read a graph file given by the graph-head file and output basic
> +details about the graph file.

"a graph file", assuming that there must be only one in the
specified place?  Or if there are more than one, read all of them?
Or is it an error to have more than one?

Do not answer questions in a message that is a response to _me_;
the purpose of a review is not to educate reviewers---it is to
improve the patch.

> +With `--graph-hash=` option, consider the graph file
> +graph-.graph in the pack directory.

I think it is more in line with how plumbing works to just let the
full pathname be specifiable (e.g. learn from how pack-objects takes
"pack-" prefix from the command line, even though in practice names
of all packs you see in any repos start from "pack-").

> +struct commit_graph *load_commit_graph_one(const char *graph_file, const 
> char *pack_dir)

This somehow smells like a screwed up API.  It gets a filename to
read from that is directly passed to git_open().  Why does an
instance of graph has to know and remember the path to the directory
(i.e. pack_dir) that was given when it was constructed?  "I am an
instance that holds commit topology learned from this object
database" is something it might want to know and remember, but "I am
told that I'll eventually be written back to there when I was
created" does not sound like a useful thing to have.

Re: [PATCH v3 03/14] commit-graph: create git-commit-graph builtin

[Junio C Hamano]

Derrick Stolee  writes:

> I wanted to have the smallest footprint as possible in the objects
> directory, and the .git/objects directory currently only holds
> folders.

When we cull stale files from pack directory, we rely on the related
files to share pack-.* pattern.  It is better not to contaminate
the directory with unrelated cruft.

As this is purely optional auxiliary information used for optimization,
perhaps .git/objects/info is a better place?  I dunno.

In any case, even if its default position ends up in .git/objects/pack/,
if this thing conceptually does not have any ties with packs
(i.e. it is not a corruption if the graph file also described
topologies including loose objects, and it is not a corruption if
the graph file did not cover objects in all packs), then the end
user visible option name shouldn't say "--pack-dir".  "--graph-dir"
that defaults to .git/objects/pack/ might be acceptable but it still
feels wrong.

Re: [PATCH v3 01/14] commit-graph: add format document

[Junio C Hamano]

Derrick Stolee  writes:

> You're right that this data is redundant. It is easy to describe the
> width of the tables using the OID length, so it is convenient to have
> that part of the format. Also, it is good to have 4-byte alignment
> here, so we are not wasting space.
>
> There isn't a strong reason to put that here, but I don't have a great
> reason to remove it, either.

Redundant information that can go out of sync is a great enough
reason not to have it in the first place.

Re: [PATCH] CodingGuidelines: mention "static" and "extern"

[Eric Sunshine]

On Thu, Feb 8, 2018 at 4:38 PM, Jeff King  wrote:
> Subject: [PATCH] CodingGuidelines: mention "static" and "extern"
> [...]
>
> Signed-off-by: Jeff King 
> ---
> diff --git a/Documentation/CodingGuidelines b/Documentation/CodingGuidelines
> @@ -386,6 +386,11 @@ For C programs:
> + - Variables and functions local to a given source file should be marked
> +   with "static". Variables that are visible to other source files
> +   must be declared with "extern" in header files. However, function
> +   declarations should not use "extern", as that is already the default.

Perhaps:

... as that is already the default, unless declarations in the
header are already "extern", in which case consistency
may favor mirroring existing usage.

or something.

Re: [PATCH] rebase -p: fix incorrect commit message when calling `git merge`.

[Junio C Hamano]

gregory.herr...@oracle.com writes:

> Since commit dd6fb0053 ("rebase -p: fix quoting when calling `git
> merge`"), commit message of the merge commit being rebased is passed to
> the merge command using a subshell executing 'git rev-parse --sq-quote'.
>
> Double quotes are needed around this subshell so that, newlines are
> kept for the git merge command.

Thanks for a fix.  Queued.

Re: [PATCH] color.h: document and modernize header

[Jeff King]

On Thu, Feb 08, 2018 at 05:26:33PM -0500, Eric Sunshine wrote:

> > I think the point is that "var" is a quad-state variable (yes, no, auto,
> > or "unknown") and we are converting to a boolean. This would probably be
> > a lot more clear if GIT_COLOR_* were all enum values and not #defines,
> > and this function took the matching enum type.
> >
> > So I think that's what you were trying to name with "constants as
> > returned by...", but it definitely took me some thinking to parse it.
> 
> Rather than talking about plural "constants" (which makes it more
> confusing), it would likely be helpful for it to say (explicitly) that
> the caller passes in the result of git_config_colorbool() as 'var'.
> 
> Or something like that.

That's not quite it, though. "var" is really the current program's idea
of whether color has been requested. So it's git_config_colorbool(), as
modified by things like "--color".

I think the best explanation is that it resolves an "enum color_bool"
into a single 0/1 boolean. Except that "enum color_bool" doesn't exist,
so we have no name for it.

-Peff

Re: [PATCH] color.h: document and modernize header

[Eric Sunshine]

On Thu, Feb 8, 2018 at 3:43 PM, Jeff King  wrote:
> On Thu, Feb 08, 2018 at 12:15:46PM -0800, Stefan Beller wrote:
>> +/*
>> + * Resolve the constants as returned by git_config_colorbool()
>> + * (specifically "auto") to a boolean answer.
>> + */
>> +extern int want_color(int var);
>
> This explanation left me even more confused about what should go in
> "var", and I think I'm the one who wrote the function. ;)

Agreed, this still fails to (directly) answer the question I asked in
[1] about what 'var' is.

> I think the point is that "var" is a quad-state variable (yes, no, auto,
> or "unknown") and we are converting to a boolean. This would probably be
> a lot more clear if GIT_COLOR_* were all enum values and not #defines,
> and this function took the matching enum type.
>
> So I think that's what you were trying to name with "constants as
> returned by...", but it definitely took me some thinking to parse it.

Rather than talking about plural "constants" (which makes it more
confusing), it would likely be helpful for it to say (explicitly) that
the caller passes in the result of git_config_colorbool() as 'var'.

Or something like that.

>> +/*
>> + * Output the formatted string in the specified color (and then reset to 
>> normal
>> + * color so subsequent output is uncolored). Omits the color encapsulation 
>> if
>> + * `color` is NULL. The `color_fprintf_ln` prints a new line after resetting
>> + * the color. The `color_print_strbuf` prints the given pre-formatted strbuf
>> + * instead, up to its first NUL character.
>> + */
>
> It probably doesn't matter much in practice, but the color_print_strbuf
> behavior sounds like a bug. Shouldn't it print the whole strbuf, even if
> it has an embedded NUL?

I (parenthetically) suggested[1] the same about fixing the
bug/misbehavior, though doing so is outside the scope of this
particular patch.

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

Re: Fetch-hooks

[Leo Gaspard]

On 02/08/2018 10:06 PM, Ævar Arnfjörð Bjarmason wrote:>> Hmm, OK, so I
guess I'll try to update the patch when I get some time to
>> delve into git's internals, as my use case (forbidding some fetches)
>> couldn't afaik be covered by a wrapper hook.
> 
> Per my reading of
> https://public-inbox.org/git/20111224234212.ga21...@gnu.kitenet.net/
> what Joey implemented is not what you described in your initial mail.
> 
> His is a *post*-fetch hook, we've already done the fetch and are just
> telling you as a courtesy what refs changed. You could also implement
> this as some cronjob that polls git for-each-ref but it's easier as a
> hook, fine.

I was thinking along the lines of
https://marc.info/?l=git=132486687023893=2
with high-level description at
https://marc.info/?l=git=132480559712592=2

With the high-level description given here, I'm pretty sure I can hack a
hook together to make things work as I want them to.

> What you're describing is something like a pre-fetch hook analogous to
> the pre-receive hooks, where you're fed refs updated on the remote on
> stdin, and can say you don't want some of those to be updated.
> 
> This may just be a lack of imagination on my part, but I don't see how
> that's sensible at all.
> 
> The refs we fetch are our *copy* of the remote refs, why would you not
> want to track the upstream remote. You're going to refuse some branches
> and what? Be further behind until some point in the future where the tip
> is GPG-signed and you accept it, at which poich you'll need to do more
> work than if you were up-to-date with the almost-GPG-signed version?

That's about it. I want all fetching to be blocked in case of the tip
not being signed. As there is a pre-push hook ensuring committers don't
forget to sign before pushing, the only case the tip could not be signed
is in case of an attack, which means it's better to just force-push
master because any git repo that fetched it is doomed anyway. Definitely
would not want to allow an untrusted revision get into anything that
could even remotely be taken as “endorsed” by the user.

(BTW, in order to avoid the case of someone forgetting to sign the
commit and not having installed the pre-push hook, there can be holes in
the commit-signing chain, the drawback being that the committer pushing
a signed commit takes responsibility for all unsigned commits directly
preceding his -- allowing them to recover in case of a mistaken push)

> I think you're confusing two things here. One is the reasonable concern
> of wanting to not have your local copy of remote refs have undesirable
> content, but a pre-fetch hook is not the way to accomplish that.

Well, a pre-fetch hook is a possible way of accomplishing that, and I
don't know of any better one?

> The other is e.g. to ensure that you never locally check out some "bad"
> ref, we don't have hook support for that, but could add it,
> e.g. git-checkout and git reset --hard could be taught about some
> pre-checkout hook.

Issue is, once we have to fix checkout and reset, all other commands
that potentially touch the worktree also have to be fixed (eg. I don't
know whether worktree add triggers pre-checkout?)

Also, this requires the hook to store a database of all the paths that
have been checked, because there is no logic in how one may choose to
checkout the repo. While having a tweak-fetch hook would make the
implementation straightforward, because at the time of invoking the hook
the “refname at remote” commit is already trusted, and the “object name”
is the commit whose validity we want to check, so we just have to check
the path between those two. (I don't know if you checked my current
scripts, but basically as the set of allowed PGP keys can change at any
commit, it's only possible to check a commit path, not a single commit
out-of-nowhere)

The only issue that could arise with a tweak-fetch hook is in case of a
force-fetch (and even maybe it's not even an actual issue, I haven't
given it real thought yet), but this can reasonably be banned, as once a
commit is signed it enters the “real” master branch, that should never
be moved backward, as it can't be the sign of an attack.

> You could also have some intermediate step between these two, where
> e.g. your refspec for "origin" is
> "+refs/heads/*:refs/remotes/origin-untrusted/*" instead of the default
> "+refs/heads/*:refs/remotes/origin/*", you fetch all refs to that
> location, then you move them (with some alias/hook) to
> "refs/remotes/origin/*" once they're seen to be "OK".

That is indeed another possibility, but then the idea is to make things
as transparent as possible for the end-user, not to completely change
their git workflow. As such, considering only signed commits to be part
of the upstream seems to make sense to me?

Cheers,
Leo

Re: Left-over COMMIT_EDITMSG file in gitdir

[Junio C Hamano]

Linus Torvalds  writes:

> In that rewrite, removing some temporary files seems to have been left
> out. At least one: .git/COMMIT_EDITMSG.
>
> In the original commit.sh shell script, we can find this:
>
>   rm -f "$GIT_DIR/COMMIT_MSG" "$GIT_DIR/COMMIT_EDITMSG" "$GIT_DIR/SQUASH_MSG"
>
> after creating the commit.  In the C implementation, we do have a
> number of "unlink(...)" calls:
> ...
>
> but no
>
> unlink(git_path_commit_editmsg());
>
> and that *seems* to be an oversight.
>
> Similarly, builtin/tag,c leaves a stale TAG_EDITMSG file behind.
> Again, that actually did exist in the original shell script, which
> used to do
>
>   trap 'rm -f "$GIT_DIR"/TAG_TMP* "$GIT_DIR"/TAG_FINALMSG
> "$GIT_DIR"/TAG_EDITMSG' 0
>
> which caused that file to be removed at exit.

I do not think it was intentional---just a bug in the rewrite.

Re: [PATCH v3 04/14] commit-graph: implement write_commit_graph()

[Junio C Hamano]

Derrick Stolee  writes:

> +char* get_commit_graph_filename_hash(const char *pack_dir,

Asterisk sticks to the identifier, not type, in our codebase.

> +  struct object_id *hash)
> +{
> + size_t len;
> + struct strbuf path = STRBUF_INIT;
> + strbuf_addstr(, pack_dir);
> + strbuf_addstr(, "/graph-");
> + strbuf_addstr(, oid_to_hex(hash));
> + strbuf_addstr(, ".graph");

Use of strbuf_addf() would make it easier to read and maintain, no?

> +
> + return strbuf_detach(, );
> +}
> +
> +static void write_graph_chunk_fanout(struct sha1file *f,
> +  struct commit **commits,
> +  int nr_commits)
> +{
> + uint32_t i, count = 0;
> + struct commit **list = commits;
> + struct commit **last = commits + nr_commits;
> +
> + /*
> +  * Write the first-level table (the list is sorted,
> +  * but we use a 256-entry lookup to be able to avoid
> +  * having to do eight extra binary search iterations).
> +  */
> + for (i = 0; i < 256; i++) {
> + while (list < last) {
> + if ((*list)->object.oid.hash[0] != i)
> + break;
> + count++;
> + list++;
> + }

If count and list are always incremented in unison, perhaps you do
not need an extra variable "last".  If typeof(nr_commits) is wider
than typeof(count), this loop and the next write-be32 is screwed
anyway ;-)

This comment probably applies equally to some other uses of the same
"compute last pointer to compare with running pointer for
termination" pattern in this patch.

> + sha1write_be32(f, count);
> + }
> +}

> +static int if_packed_commit_add_to_list(const struct object_id *oid,

That is a strange name.  "collect packed commits", perhaps?

> + struct packed_git *pack,
> + uint32_t pos,
> + void *data)
> +{
> + struct packed_oid_list *list = (struct packed_oid_list*)data;
> + enum object_type type;
> + unsigned long size;
> + void *inner_data;
> + off_t offset = nth_packed_object_offset(pack, pos);
> + inner_data = unpack_entry(pack, offset, , );
> +
> + if (inner_data)
> + free(inner_data);
> +
> + if (type != OBJ_COMMIT)
> + return 0;
> +
> + ALLOC_GROW(list->list, list->nr + 1, list->alloc);

This probably will become inefficient in large repositories.  You
know you'll be walking all the pack files, and total number of
objects in a packfile can be read cheaply, so it may make sense to
make a rough guestimate of the number of commits (e.g. 15-25% of the
total number of objects) in the repository and allocate the list
upfront, instead of a hardcoded 1024.

Left-over COMMIT_EDITMSG file in gitdir

[Linus Torvalds]

This may be intentional, but if so, it's not obvious..

Back long long ago, the original "git commit" shell script got rewritten in C.

In that rewrite, removing some temporary files seems to have been left
out. At least one: .git/COMMIT_EDITMSG.

In the original commit.sh shell script, we can find this:

  rm -f "$GIT_DIR/COMMIT_MSG" "$GIT_DIR/COMMIT_EDITMSG" "$GIT_DIR/SQUASH_MSG"

after creating the commit.  In the C implementation, we do have a
number of "unlink(...)" calls:

unlink(git_path_cherry_pick_head());
unlink(git_path_revert_head());
unlink(git_path_merge_head());
unlink(git_path_merge_msg());
unlink(git_path_merge_mode());
unlink(git_path_squash_msg());

but no

unlink(git_path_commit_editmsg());

and that *seems* to be an oversight.

Similarly, builtin/tag,c leaves a stale TAG_EDITMSG file behind.
Again, that actually did exist in the original shell script, which
used to do

  trap 'rm -f "$GIT_DIR"/TAG_TMP* "$GIT_DIR"/TAG_FINALMSG
"$GIT_DIR"/TAG_EDITMSG' 0

which caused that file to be removed at exit.

I guess I really don't care much, but those two files struck me when I
was doing a "git gc --prune=now" and looked at what was still left in
the .git directory..

If this is all intentional, never mind.

   Linus

Re: [PATCH] send-email: error out when relogin delay is missing

[Eric Sunshine]

On Thu, Feb 8, 2018 at 1:21 PM, Stefan Beller  wrote:
> On Thu, Feb 8, 2018 at 12:08 AM, Eric Sunshine  
> wrote:
>> On Wed, Feb 7, 2018 at 6:43 PM, Stefan Beller  wrote:
>>> +die __("When a batch size is given, the relogin delay must be set\n")
>>> +   if defined $relogin_delay and not defined $batch_size;
>>
>> This only makes sense is 'batch-size' is specified but not 'relogin'.
>> If the other way around, then the error is confusing. How about this
>> instead?
>> "--batch-size and --relogin must be specified together"
>> ...or something.
>
> I like this for its expressiveness as it would have helped me a lot.
> I dislike this because it is incorrect when you use the config options
> instead of command line arguments.

Perhaps:

"`batch-size` and `relogin` must be specified together
  (via command-line or configuration option)"

Re: [PATCH] CodingGuidelines: mention "static" and "extern"

[Stefan Beller]

On Thu, Feb 8, 2018 at 1:38 PM, Jeff King  wrote:
> On Thu, Feb 08, 2018 at 01:04:08PM -0800, Stefan Beller wrote:
>
>> You may sense a pattern here: I currently have the very firm understanding
>> we use the extern keyword in our codebase.
>>
>> And I can also attest that this was not always the case, as back in the
>> day I remember writing patches without the extern keyword only to be told:
>> (A) be similar to the function in the next lines
>> (B) the standard is to use extern
>> and I was convinced it was a bad decision to prefix declarations with
>> the extern keyword, but followed along as I don't want to have style
>> in the way of writing features.
>
> It definitely was the case that people used to suggest "extern". I think
> this was a Linus-ism from the early days, and I have been hating it for
> almost 12 years now. ;)
>
>>   $ cat Documentation/CodingGuidelines |grep extern
>>   $ # oh no it's empty!
>>
>> Care to add a section to our coding guidelines?
>
> Here's a patch.
>
> -- >8 --
> Subject: [PATCH] CodingGuidelines: mention "static" and "extern"
>
> It perhaps goes without saying that file-local stuff should
> be marked static, but it does not hurt to remind people.
>
> Less obvious is that we are settling on "do not include
> extern in function declarations". It is already the default
> unless the function was previously declared static (but if
> you are following a static declaration with an unmarked one,
> you should think about why you are declaring the thing
> twice). And so it just becomes an extra noise-word in our
> header files.
>
> We used to give the opposite advice, so there are quite a
> few "extern" markers in early Git code. But this at least
> makes a concrete suggestion that we can follow going
> forward.
>
> Signed-off-by: Jeff King 

Reviewed-by: Stefan Beller 

... and now I can resend that patch, after fixing it to
follow our style. :)

> ---
>  Documentation/CodingGuidelines | 5 +
>  1 file changed, 5 insertions(+)
>
> diff --git a/Documentation/CodingGuidelines b/Documentation/CodingGuidelines
> index c4cb5ff0d4..48aa4edfbd 100644
> --- a/Documentation/CodingGuidelines
> +++ b/Documentation/CodingGuidelines
> @@ -386,6 +386,11 @@ For C programs:
>   - Use Git's gettext wrappers to make the user interface
> translatable. See "Marking strings for translation" in po/README.
>
> + - Variables and functions local to a given source file should be marked
> +   with "static". Variables that are visible to other source files
> +   must be declared with "extern" in header files. However, function
> +   declarations should not use "extern", as that is already the default.
> +
>  For Perl programs:
>
>   - Most of the C guidelines above apply.
> --
> 2.16.1.365.g89f5777adf
>

[PATCH] CodingGuidelines: mention "static" and "extern"

[Jeff King]

On Thu, Feb 08, 2018 at 01:04:08PM -0800, Stefan Beller wrote:

> You may sense a pattern here: I currently have the very firm understanding
> we use the extern keyword in our codebase.
> 
> And I can also attest that this was not always the case, as back in the
> day I remember writing patches without the extern keyword only to be told:
> (A) be similar to the function in the next lines
> (B) the standard is to use extern
> and I was convinced it was a bad decision to prefix declarations with
> the extern keyword, but followed along as I don't want to have style
> in the way of writing features.

It definitely was the case that people used to suggest "extern". I think
this was a Linus-ism from the early days, and I have been hating it for
almost 12 years now. ;)

>   $ cat Documentation/CodingGuidelines |grep extern
>   $ # oh no it's empty!
> 
> Care to add a section to our coding guidelines?

Here's a patch.

-- >8 --
Subject: [PATCH] CodingGuidelines: mention "static" and "extern"

It perhaps goes without saying that file-local stuff should
be marked static, but it does not hurt to remind people.

Less obvious is that we are settling on "do not include
extern in function declarations". It is already the default
unless the function was previously declared static (but if
you are following a static declaration with an unmarked one,
you should think about why you are declaring the thing
twice). And so it just becomes an extra noise-word in our
header files.

We used to give the opposite advice, so there are quite a
few "extern" markers in early Git code. But this at least
makes a concrete suggestion that we can follow going
forward.

Signed-off-by: Jeff King 
---
 Documentation/CodingGuidelines | 5 +
 1 file changed, 5 insertions(+)

diff --git a/Documentation/CodingGuidelines b/Documentation/CodingGuidelines
index c4cb5ff0d4..48aa4edfbd 100644
--- a/Documentation/CodingGuidelines
+++ b/Documentation/CodingGuidelines
@@ -386,6 +386,11 @@ For C programs:
  - Use Git's gettext wrappers to make the user interface
translatable. See "Marking strings for translation" in po/README.
 
+ - Variables and functions local to a given source file should be marked
+   with "static". Variables that are visible to other source files
+   must be declared with "extern" in header files. However, function
+   declarations should not use "extern", as that is already the default.
+
 For Perl programs:
 
  - Most of the C guidelines above apply.
-- 
2.16.1.365.g89f5777adf

Re: [PATCH v3 03/14] commit-graph: create git-commit-graph builtin

[Derrick Stolee]

On 2/8/2018 4:27 PM, Junio C Hamano wrote:

Derrick Stolee  writes:


Teach git the 'commit-graph' builtin that will be used for writing and
reading packed graph files. The current implementation is mostly
empty, except for a '--pack-dir' option.

Why do we want to use "pack" dir, when this is specifically designed
not tied to packfile?  .git/objects/pack/ certainly is a possibility
in the sense that anywhere inside .git/objects/ would make sense,
but using the "pack" dir smells like signalling a wrong message to
users.



I wanted to have the smallest footprint as possible in the objects 
directory, and the .git/objects directory currently only holds folders.


I suppose this feature, along with the multi-pack-index (MIDX), extends 
the concept of the pack directory to be a "compressed data" directory, 
but keeps the "pack" name to be compatible with earlier versions.


Another option is to create a .git/objects/graph directory instead, but 
then we need to worry about that directory being present.


Thanks,
-Stolee

Re: [PATCH v3 01/14] commit-graph: add format document

[Derrick Stolee]

On 2/8/2018 4:21 PM, Junio C Hamano wrote:

Derrick Stolee  writes:


Add document specifying the binary format for commit graphs. This
format allows for:

* New versions.
* New hash functions and hash lengths.

It still is unclear, at least to me, why OID and OID length are
stored as if they can be independent.  If a reader does not
understand a new Object Id hash, is there anything the reader can
still do by knowing how long the hash (which it cannot recompute to
validate) is?  And if a reader does know what OID hashing scheme is
used to refer to the objects, it certainly would know how long the
OIDs are.

Giving length may make sense only when a reader can treat these OIDs
as completely opaque identifiers, without having to (re)hash from
the contents, but if that is the case, then there is no point saying
what exact hash function is used to compute OID.

So I'd understand storing only either one or the other, but not
both.  Am I missing something?


You're right that this data is redundant. It is easy to describe the 
width of the tables using the OID length, so it is convenient to have 
that part of the format. Also, it is good to have 4-byte alignment here, 
so we are not wasting space.


There isn't a strong reason to put that here, but I don't have a great 
reason to remove it, either.


Perhaps leave a byte blank for possible future use?




+The Git commit graph stores a list of commit OIDs and some associated
+metadata, including:
+
+- The generation number of the commit. Commits with no parents have
+  generation number 1; commits with parents have generation number
+  one more than the maximum generation number of its parents. We
+  reserve zero as special, and can be used to mark a generation
+  number invalid or as "not computed".

This "most natural" definition of generation number is stricter than
absolutely necessary (a looser definition that is sufficient is
"gennum of a child is larger than all of its parents'").  While I
personally think that is OK, some people who floated different ideas
in previous discussions on generation numbers may want to articulate
their ideas again.  One idea that I found clever was to use the
total number of commits that are ancestors of a commit instead (it
is far more expensive to compute than the most natural gennum, but
doing so may help other topology math, like "describe").


It is more difficult to compute the number of reachable commits, since 
you cannot learn that only by looking at the parents (you need to know 
how many commits are in the intersection of their reachable sets for a 
two-parent merge, or just walk all of the commits). This leads to a 
quadratic computation to discover the value for N commits.


I define it this rigidly now because I will submit a patch soon after 
this one lands that computes generation numbers and consumes them in 
paint_down_to_common(). I've got it sitting in my local repo ready for a 
rebase.





+CHUNK LOOKUP:
+
+  (C + 1) * 12 bytes listing the table of contents for the chunks:
+  First 4 bytes describe chunk id. Value 0 is a terminating label.
+  Other 8 bytes provide offset in current file for chunk to start.
+  (Chunks are ordered contiguously in the file, so you can infer
+  the length using the next chunk position if necessary.)

Aren't chunks numbered contiguously, starting from #1, thereby
making it unnecessary to store the 4-byte?

How does a reader obtain the length of the last chunk?  Ahh, that is
why there are C+1 entries in this table, not just C, so that the
reader knows where to stop while reading the last one.  Does that
mean that this table looks like this?

 { 1, offset_1 },

 { 2, offset_2 },
 ...
 { C, offset_C },
 { 0, offset_C+1 },

where where (offset_N+1 - offset_N) gives the length of chunk #N?


This is correct.




+  The remaining data in the body is described one chunk at a time, and
+  these chunks may be given in any order. Chunks are required unless
+  otherwise specified.
+
+CHUNK DATA:
+
+  OID Fanout (ID: {'O', 'I', 'D', 'F'}) (256 * 4 bytes)
+  The ith entry, F[i], stores the number of OIDs with first
+  byte at most i. Thus F[255] stores the total
+  number of commits (N).
+
+  OID Lookup (ID: {'O', 'I', 'D', 'L'}) (N * H bytes)
+  The OIDs for all commits in the graph, sorted in ascending order.
+
+  Commit Data (ID: {'C', 'G', 'E', 'T' }) (N * (H + 16) bytes)
+* The first H bytes are for the OID of the root tree.
+* The next 8 bytes are for the int-ids of the first two parents
+  of the ith commit. Stores value 0x if no parent in that
+  position. If there are more than two parents, the second value
+  has its most-significant bit on and the other bits store an array
+  position into the Large Edge List chunk.
+* The next 8 bytes store the generation number of the commit and
+  the commit time in seconds since EPOCH. The generation number
+  uses the 

Re: [PATCH v3 03/14] commit-graph: create git-commit-graph builtin

[Junio C Hamano]

Derrick Stolee  writes:

> Teach git the 'commit-graph' builtin that will be used for writing and
> reading packed graph files. The current implementation is mostly
> empty, except for a '--pack-dir' option.

Why do we want to use "pack" dir, when this is specifically designed
not tied to packfile?  .git/objects/pack/ certainly is a possibility
in the sense that anywhere inside .git/objects/ would make sense,
but using the "pack" dir smells like signalling a wrong message to
users.

Re: [PATCH v3 01/14] commit-graph: add format document

[Junio C Hamano]

Derrick Stolee  writes:

> Add document specifying the binary format for commit graphs. This
> format allows for:
>
> * New versions.
> * New hash functions and hash lengths.

It still is unclear, at least to me, why OID and OID length are
stored as if they can be independent.  If a reader does not
understand a new Object Id hash, is there anything the reader can
still do by knowing how long the hash (which it cannot recompute to
validate) is?  And if a reader does know what OID hashing scheme is
used to refer to the objects, it certainly would know how long the
OIDs are.

Giving length may make sense only when a reader can treat these OIDs
as completely opaque identifiers, without having to (re)hash from
the contents, but if that is the case, then there is no point saying
what exact hash function is used to compute OID.

So I'd understand storing only either one or the other, but not
both.  Am I missing something?

> +The Git commit graph stores a list of commit OIDs and some associated
> +metadata, including:
> +
> +- The generation number of the commit. Commits with no parents have
> +  generation number 1; commits with parents have generation number
> +  one more than the maximum generation number of its parents. We
> +  reserve zero as special, and can be used to mark a generation
> +  number invalid or as "not computed".

This "most natural" definition of generation number is stricter than
absolutely necessary (a looser definition that is sufficient is
"gennum of a child is larger than all of its parents'").  While I
personally think that is OK, some people who floated different ideas
in previous discussions on generation numbers may want to articulate
their ideas again.  One idea that I found clever was to use the
total number of commits that are ancestors of a commit instead (it
is far more expensive to compute than the most natural gennum, but
doing so may help other topology math, like "describe").

> +CHUNK LOOKUP:
> +
> +  (C + 1) * 12 bytes listing the table of contents for the chunks:
> +  First 4 bytes describe chunk id. Value 0 is a terminating label.
> +  Other 8 bytes provide offset in current file for chunk to start.
> +  (Chunks are ordered contiguously in the file, so you can infer
> +  the length using the next chunk position if necessary.)

Aren't chunks numbered contiguously, starting from #1, thereby
making it unnecessary to store the 4-byte?

How does a reader obtain the length of the last chunk?  Ahh, that is
why there are C+1 entries in this table, not just C, so that the
reader knows where to stop while reading the last one.  Does that
mean that this table looks like this?
   
{ 1, offset_1 },
{ 2, offset_2 },
...
{ C, offset_C },
{ 0, offset_C+1 },

where where (offset_N+1 - offset_N) gives the length of chunk #N?

> +  The remaining data in the body is described one chunk at a time, and
> +  these chunks may be given in any order. Chunks are required unless
> +  otherwise specified.
> +
> +CHUNK DATA:
> +
> +  OID Fanout (ID: {'O', 'I', 'D', 'F'}) (256 * 4 bytes)
> +  The ith entry, F[i], stores the number of OIDs with first
> +  byte at most i. Thus F[255] stores the total
> +  number of commits (N).
> +
> +  OID Lookup (ID: {'O', 'I', 'D', 'L'}) (N * H bytes)
> +  The OIDs for all commits in the graph, sorted in ascending order.
> +
> +  Commit Data (ID: {'C', 'G', 'E', 'T' }) (N * (H + 16) bytes)
> +* The first H bytes are for the OID of the root tree.
> +* The next 8 bytes are for the int-ids of the first two parents
> +  of the ith commit. Stores value 0x if no parent in that
> +  position. If there are more than two parents, the second value
> +  has its most-significant bit on and the other bits store an array
> +  position into the Large Edge List chunk.
> +* The next 8 bytes store the generation number of the commit and
> +  the commit time in seconds since EPOCH. The generation number
> +  uses the higher 30 bits of the first 4 bytes, while the commit
> +  time uses the 32 bits of the second 4 bytes, along with the lowest
> +  2 bits of the lowest byte, storing the 33rd and 34th bit of the
> +  commit time.
> +
> +  Large Edge List (ID: {'E', 'D', 'G', 'E'})
> +  This list of 4-byte values store the second through nth parents for
> +  all octopus merges. The second parent value in the commit data stores
> +  an array position within this list along with the most-significant bit
> +  on. Starting at that array position, iterate through this list of 
> int-ids
> +  for the parents until reaching a value with the most-significant bit 
> on.
> +  The other bits correspond to the int-id of the last parent. This chunk
> +  should always be present, but may be empty.

I am not convinced about the value of these 4-byte section IDs.  

They are useless unless you define what should happen when a reader
sees a block of data 

Re: [PATCHv3 2/4] builtin/blame: dim uninteresting metadata

[Stefan Beller]

On Mon, Jan 8, 2018 at 11:34 AM, Junio C Hamano  wrote:
> Stefan Beller  writes:
>
>> +color.blame.repeatedMeta::
>> + Use the customized color for the part of git-blame output that
>> + is repeated meta information per line (such as commit id,
>> + author name, date and timezone). Defaults to dark gray.
>> +
>> ...
>
> "Dark gray on default background" may alleviate worrries from those
> who prefer black ink on white paper display by hinting that both
> foreground and background colors can be configured.
>
> Do we want to make this overridable from the command line,
> i.e. --color-repeated-meta=gray?
>
>> +#define OUTPUT_COLOR_LINE02000
>
> The name of the macro implies that this is (or at least can be) a
> lot more generic UI request than merely "paint the same metadata on
> adjacent lines in a different color".
>
>> + OPT_BIT(0, "color-lines", _option, N_("color redundant 
>> metadata from previous line differently"), OUTPUT_COLOR_LINE),
>
> Should this eventually become "--color=" that is more
> usual and generic, possibly defaulting to "auto" in the future, I
> wonder?

This sounds like a good change to make, though what does "yes"
mean here? The following patches introduce other modes of
colorization, such that --color=
would make sense to me, with "lines" as the mode of this patch,
"fields" implemented in the next patch and "decayed" having the
meaning of the heating algorithm presented in the last patch.

However each of these (except no and auto) would want to have
extra parameters, which as mentioned above could go into its own
separate parameters, such that

  git blame --color=line --color-repeated-meta=cyan

would seem like a good UI. This might look like it could be a good
idea to have --color-repeated-meta imply --color= if no
--color is given. But as different coloring modes will have different
arguments to provide extra color information, I wonder what we
would do with

  git blame --color="mode1" --extra-color-arg-for-mode-2

So I don't think the idea of having separate options would be a
good idea as we'd have to think about the implications as mentioned
above. So maybe

  git blame "--color=lines,cyan yellow"

would work, if you want "cyan yellow" as the color configuration in
the "lines" mode? (bad choice of a background color btw)

>> diff --git a/color.h b/color.h
>> index 2e768a10c6..2df2f86698 100644
>> --- a/color.h
>> +++ b/color.h
>> @@ -30,6 +30,7 @@ struct strbuf;
>>  #define GIT_COLOR_BLUE   "\033[34m"
>>  #define GIT_COLOR_MAGENTA"\033[35m"
>>  #define GIT_COLOR_CYAN   "\033[36m"
>> +#define GIT_COLOR_DARK   "\033[1;30m"
>>  #define GIT_COLOR_BOLD_RED   "\033[1;31m"
>>  #define GIT_COLOR_BOLD_GREEN "\033[1;32m"
>>  #define GIT_COLOR_BOLD_YELLOW"\033[1;33m"
>
> How about using CYAN just like "diff" output uses it to paint the
> least interesting lines?  That way we will keep the "uninteresting
> is cyan" consistency for the default settings without adding a new
> color to the palette.

sounds good, I'll take that suggestion once we reach consensus on
the UI for the user.

Thanks,
Stefan

Re: Fetch-hooks

[Ævar Arnfjörð Bjarmason]

On Thu, Feb 08 2018, Leo Gaspard jotted:

> On 02/08/2018 04:30 PM, Joey Hess wrote:
>> Leo Gaspard wrote:
>>> That said, I just came upon [1] (esp. the description [2] and the patch
>>> [3]), and wondered: it looks like the patch was abandoned midway in
>>> favor of a hook refactoring. Would you happen to know whether the hook
>>> refactoring eventually took place, and/or whether this patch was
>>> resubmitted later, and/or whether it would still be possible to merge
>>> this now? (not having any experience with git's internals yet, I don't
>>> really know whether these are stupid questions or not)
>>>
>>> PS: Cc'ing Joey, as you most likely know best what eventually happened,
>>> if you can remember it?
>>
>> I don't remember it well, but reviewing the thread, I think it foundered
>> on this comment by Junio:
>>
>>> That use case sounds like that "git fetch" is called as a first class UI,
>>> which is covered by "git myfetch" (you can call it "git annex fetch")
>>> wrapper approach, the canonical example of a hook that we explicitly do
>> ^
>>> not want to add.
>>   ^^^
>>
>> While I still think a fetch hook would be a good idea for reasons of
>> composability, I then just went off and implemented such a wrapper for
>> my own particular use case, and the wrapper program then grew to cover
>> use cases that a hook would not have been able to cover, so ...
>
> Hmm, OK, so I guess I'll try to update the patch when I get some time to
> delve into git's internals, as my use case (forbidding some fetches)
> couldn't afaik be covered by a wrapper hook.

Per my reading of
https://public-inbox.org/git/20111224234212.ga21...@gnu.kitenet.net/
what Joey implemented is not what you described in your initial mail.

His is a *post*-fetch hook, we've already done the fetch and are just
telling you as a courtesy what refs changed. You could also implement
this as some cronjob that polls git for-each-ref but it's easier as a
hook, fine.

What you're describing is something like a pre-fetch hook analogous to
the pre-receive hooks, where you're fed refs updated on the remote on
stdin, and can say you don't want some of those to be updated.

This may just be a lack of imagination on my part, but I don't see how
that's sensible at all.

The refs we fetch are our *copy* of the remote refs, why would you not
want to track the upstream remote. You're going to refuse some branches
and what? Be further behind until some point in the future where the tip
is GPG-signed and you accept it, at which poich you'll need to do more
work than if you were up-to-date with the almost-GPG-signed version?

I think you're confusing two things here. One is the reasonable concern
of wanting to not have your local copy of remote refs have undesirable
content, but a pre-fetch hook is not the way to accomplish that.

The other is e.g. to ensure that you never locally check out some "bad"
ref, we don't have hook support for that, but could add it,
e.g. git-checkout and git reset --hard could be taught about some
pre-checkout hook.

You could also have some intermediate step between these two, where
e.g. your refspec for "origin" is
"+refs/heads/*:refs/remotes/origin-untrusted/*" instead of the default
"+refs/heads/*:refs/remotes/origin/*", you fetch all refs to that
location, then you move them (with some alias/hook) to
"refs/remotes/origin/*" once they're seen to be "OK".

Re: [PATCH] color.h: document and modernize header

[Stefan Beller]

On Thu, Feb 8, 2018 at 12:43 PM, Jeff King  wrote:
> On Thu, Feb 08, 2018 at 12:15:46PM -0800, Stefan Beller wrote:
>
>>  int color_fprintf(FILE *fp, const char *color, const char *fmt, ...)
>>  {
>>   va_list args;
>> diff --git a/color.h b/color.h
>> index fd2b688dfb..8c7e6c41c2 100644
>> --- a/color.h
>> +++ b/color.h
>> @@ -72,26 +72,50 @@ extern int color_stdout_is_tty;
>>   * Use the first one if you need only color config; the second is a 
>> convenience
>>   * if you are just going to change to git_default_config, too.
>>   */
>> -int git_color_config(const char *var, const char *value, void *cb);
>> -int git_color_default_config(const char *var, const char *value, void *cb);
>> +extern int git_color_config(const char *var, const char *value, void *cb);
>> +extern int git_color_default_config(const char *var, const char *value, 
>> void *cb);
>
> Hmph, I thought we weren't adding "extern" everywhere. See:
>
>   https://public-inbox.org/git/xmqq8tea5hxi@gitster.mtv.corp.google.com/
>
> Other than that, these changes mostly look like improvements. A few

...

> Those are all suggestions. Given that there's no documentation currently
> on most of these, I think even if you don't take any of my suggestions,
> this would still be a net improvement (modulo the "extern" thing).

A funny and sad rant about why clear communication matters:

[Once upon a time, maybe 2 years ago] I had the impression that the old
code is nicely written and was consistently marked extern in header files.
(which btw is consistent with variable declarations, they need the extern).
All the new code doesn't make use of extern, so I had this on my low prio
todo list, that eventually all code converges to have 'extern'
functions in headers.

C.f. the following commits, found via
  git log -p --author=Beller -S extern

  5ec8274b84 (xdiff-interface: export comparing and hashing strings,
  2017-10-25) adding new externs

  1ecbf31d02 (hashmap: migrate documentation from Documentation/technical
  into header, 2017-06-30), a cleanup, which doesn't touch externs

  a6d7eb2c7a (pull: optionally rebase submodules (remote submodule
  changes only), 2017-06-23) new code using externs

  bd26756112 (submodule.h: add extern keyword to functions, 2016-12-20)
  (The commit message is as accurate as it gets)

You may sense a pattern here: I currently have the very firm understanding
we use the extern keyword in our codebase.

And I can also attest that this was not always the case, as back in the
day I remember writing patches without the extern keyword only to be told:
(A) be similar to the function in the next lines
(B) the standard is to use extern
and I was convinced it was a bad decision to prefix declarations with
the extern keyword, but followed along as I don't want to have style
in the way of writing features.

  $ cat Documentation/CodingGuidelines |grep extern
  $ # oh no it's empty!

Care to add a section to our coding guidelines?

Thanks,
Stefan

[PATCH] rebase -p: fix incorrect commit message when calling `git merge`.

[gregory . herrero]

From: Gregory Herrero 

Since commit dd6fb0053 ("rebase -p: fix quoting when calling `git
merge`"), commit message of the merge commit being rebased is passed to
the merge command using a subshell executing 'git rev-parse --sq-quote'.

Double quotes are needed around this subshell so that, newlines are
kept for the git merge command.

Before this patch, following merge message:

"Merge mybranch into mynewbranch

Awesome commit."

becomes:

"Merge mybranch into mynewbranch Awesome commit."

after a rebase -p.

Fixes: "dd6fb0053 rebase -p: fix quoting when calling `git merge`"
Reported-by: Jamie Iles 
Suggested-by: Vegard Nossum 
Suggested-by: Quentin Casasnovas 
Signed-off-by: Gregory Herrero 
---
 git-rebase--interactive.sh   |  2 +-
 t/t3408-rebase-multi-line.sh | 26 +-
 2 files changed, 26 insertions(+), 2 deletions(-)

diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index d47bd2959..ab6a5883e 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -396,7 +396,7 @@ pick_one_preserving_merges () {
--sq-quote "$gpg_sign_opt")} \
$allow_rerere_autoupdate "$merge_args" \
"$strategy_args" \
-   -m $(git rev-parse --sq-quote "$msg_content") \
+   -m "$(git rev-parse --sq-quote "$msg_content")" 
\
"$new_parents"
then
printf "%s\n" "$msg_content" > 
"$GIT_DIR"/MERGE_MSG
diff --git a/t/t3408-rebase-multi-line.sh b/t/t3408-rebase-multi-line.sh
index 6b84e6042..e217fb4fb 100755
--- a/t/t3408-rebase-multi-line.sh
+++ b/t/t3408-rebase-multi-line.sh
@@ -24,8 +24,23 @@ But otherwise with a sane description." &&
>elif &&
git add elif &&
test_tick &&
-   git commit -m second
+   git commit -m second &&
 
+   git checkout -b side2 &&
+   >afile &&
+   git add afile &&
+   test_tick &&
+   git commit -m third &&
+   echo hello > afile &&
+   test_tick &&
+   git commit -a -m fourth &&
+   git checkout -b side-merge &&
+   git reset --hard HEAD^^ &&
+   git merge --no-ff -m "A merge commit log message that has a long
+summary that spills over multiple lines.
+
+But otherwise with a sane description." side2 &&
+   git branch side-merge-original
 '
 
 test_expect_success rebase '
@@ -36,6 +51,15 @@ test_expect_success rebase '
git cat-file commit side@{1} | sed -e "1,/^\$/d" >expect &&
test_cmp expect actual
 
+'
+test_expect_success rebasep '
+
+   git checkout side-merge &&
+   git rebase -p side &&
+   git cat-file commit HEAD | sed -e "1,/^\$/d" >actual &&
+   git cat-file commit side-merge-original | sed -e "1,/^\$/d" >expect &&
+   test_cmp expect actual
+
 '
 
 test_done
-- 
2.16.1

Re: [PATCH] color.h: document and modernize header

[Jeff King]

On Thu, Feb 08, 2018 at 12:15:46PM -0800, Stefan Beller wrote:

>  int color_fprintf(FILE *fp, const char *color, const char *fmt, ...)
>  {
>   va_list args;
> diff --git a/color.h b/color.h
> index fd2b688dfb..8c7e6c41c2 100644
> --- a/color.h
> +++ b/color.h
> @@ -72,26 +72,50 @@ extern int color_stdout_is_tty;
>   * Use the first one if you need only color config; the second is a 
> convenience
>   * if you are just going to change to git_default_config, too.
>   */
> -int git_color_config(const char *var, const char *value, void *cb);
> -int git_color_default_config(const char *var, const char *value, void *cb);
> +extern int git_color_config(const char *var, const char *value, void *cb);
> +extern int git_color_default_config(const char *var, const char *value, void 
> *cb);

Hmph, I thought we weren't adding "extern" everywhere. See:

  https://public-inbox.org/git/xmqq8tea5hxi@gitster.mtv.corp.google.com/

Other than that, these changes mostly look like improvements. A few
comments:

> +/*
> + * Resolve the constants as returned by git_config_colorbool()
> + * (specifically "auto") to a boolean answer.
> + */
> +extern int want_color(int var);

This explanation left me even more confused about what should go in
"var", and I think I'm the one who wrote the function. ;)

I think the point is that "var" is a quad-state variable (yes, no, auto,
or "unknown") and we are converting to a boolean. This would probably be
a lot more clear if GIT_COLOR_* were all enum values and not #defines,
and this function took the matching enum type.

So I think that's what you were trying to name with "constants as
returned by...", but it definitely took me some thinking to parse it.

> +/*
> + * Translate a Git color from 'value' into a string that the terminal can
> + * interpret and store it into 'dst'. The Git color values are of the form
> + * "foreground [background] [attr]" where fore- and background can be a color
> + * name ("red"), a RGB code (#0xFF) or a 256-color-mode from the 
> terminal.
> + */
> +extern int color_parse(const char *value, char *dst);
> +extern int color_parse_mem(const char *value, int len, char *dst);

The inputs here are called "value" mostly because we feed them from the
var/value pair of config. But maybe "colorspec", or even just "src"
would be more clear than "value".

> +/*
> + * Output the formatted string in the specified color (and then reset to 
> normal
> + * color so subsequent output is uncolored). Omits the color encapsulation if
> + * `color` is NULL. The `color_fprintf_ln` prints a new line after resetting
> + * the color. The `color_print_strbuf` prints the given pre-formatted strbuf
> + * instead, up to its first NUL character.
> + */

It probably doesn't matter much in practice, but the color_print_strbuf
behavior sounds like a bug. Shouldn't it print the whole strbuf, even if
it has an embedded NUL?

> +/*
> + * Check if the given color is GIT_COLOR_NIL that means "no color selected".
> + * The caller needs to replace the color with the actual desired color.
> + */
> +extern int color_is_nil(const char *color);

Is this a parsed color string, or a human-readable source? I think
consistent naming of the two variables would help (using a type doesn't
work since they're both "const char *").

> diff --git a/grep.c b/grep.c
> index 3d7cd0e96f..834b8eb439 100644
> --- a/grep.c
> +++ b/grep.c
> @@ -18,6 +18,11 @@ static void std_output(struct grep_opt *opt, const void 
> *buf, size_t size)
>   fwrite(buf, size, 1, stdout);
>  }
>  
> +static void color_set(char *dst, const char *color_bytes)
> +{
> + xsnprintf(dst, COLOR_MAXLEN, "%s", color_bytes);
> +}
> +

This part seems OK. I think we made color_set() globally available with
the notion that other callers might make use of it. But it's pretty
horrid as public interfaces go, requiring that the caller has created a
buffer of the appropriate size. We'd do better to have a "struct color"
with the correctly-sized buffer. But at this point I don't think it's worth
overhauling the color code.


Those are all suggestions. Given that there's no documentation currently
on most of these, I think even if you don't take any of my suggestions,
this would still be a net improvement (modulo the "extern" thing).

-Peff

[PATCH v3 09/14] commit-graph: implement --delete-expired

[Derrick Stolee]

Teach git-commit-graph to delete the graph files in the pack directory
that were not referenced by 'graph_head' during this process. This cleans
up space for the user while not causing race conditions with other running
Git processes that may be referencing the previous graph file.

To delete old graph files, a user (or managing process) would call

git commit-graph write --update-head --delete-expired

but there is some responsibility that the caller must consider. Specifically,
ensure that processes that started before a previous 'commit-graph write'
command have completed. Otherwise, they may have an open handle on a graph file
that will be deleted by the new call.

Signed-off-by: Derrick Stolee 
---
 Documentation/git-commit-graph.txt | 11 --
 builtin/commit-graph.c | 73 --
 t/t5318-commit-graph.sh|  7 ++--
 3 files changed, 84 insertions(+), 7 deletions(-)

diff --git a/Documentation/git-commit-graph.txt 
b/Documentation/git-commit-graph.txt
index 8c2cbbc923..7ae8f7484d 100644
--- a/Documentation/git-commit-graph.txt
+++ b/Documentation/git-commit-graph.txt
@@ -37,6 +37,11 @@ checksum hash of the written file.
 +
 With `--update-head` option, update the graph-head file to point
 to the written graph file.
++
+With the `--delete-expired` option, delete the graph files in the pack
+directory that are not referred to by the graph-head file. To avoid race
+conditions, do not delete the file previously referred to by the
+graph-head file if it is updated by the `--update-head` option.
 
 'read'::
 
@@ -60,11 +65,11 @@ EXAMPLES
 $ git commit-graph write
 
 
-* Write a graph file for the packed commits in your local .git folder
-* and update graph-head.
+* Write a graph file for the packed commits in your local .git folder,
+* update graph-head, and delete state graph files.
 +
 
-$ git commit-graph write --update-head
+$ git commit-graph write --update-head --delete-expired
 
 
 * Read basic information from a graph file.
diff --git a/builtin/commit-graph.c b/builtin/commit-graph.c
index 529cb80de6..15f647fd81 100644
--- a/builtin/commit-graph.c
+++ b/builtin/commit-graph.c
@@ -9,7 +9,7 @@ static char const * const builtin_commit_graph_usage[] = {
N_("git commit-graph [--pack-dir ]"),
N_("git commit-graph clear [--pack-dir ]"),
N_("git commit-graph read [--graph-hash=]"),
-   N_("git commit-graph write [--pack-dir ] [--update-head]"),
+   N_("git commit-graph write [--pack-dir ] [--update-head] 
[--delete-expired]"),
NULL
 };
 
@@ -24,7 +24,7 @@ static const char * const builtin_commit_graph_read_usage[] = 
{
 };
 
 static const char * const builtin_commit_graph_write_usage[] = {
-   N_("git commit-graph write [--pack-dir ] [--update-head]"),
+   N_("git commit-graph write [--pack-dir ] [--update-head] 
[--delete-expired]"),
NULL
 };
 
@@ -32,6 +32,7 @@ static struct opts_commit_graph {
const char *pack_dir;
const char *graph_hash;
int update_head;
+   int delete_expired;
 } opts;
 
 static int graph_clear(int argc, const char **argv)
@@ -153,9 +154,68 @@ static void update_head_file(const char *pack_dir, const 
struct object_id *graph
commit_lock_file();
 }
 
+/*
+ * To avoid race conditions and deleting graph files that are being
+ * used by other processes, look inside a pack directory for all files
+ * of the form "graph-.graph" that do not match the old or new
+ * graph hashes and delete them.
+ */
+static void do_delete_expired(const char *pack_dir,
+ struct object_id *old_graph_hash,
+ struct object_id *new_graph_hash)
+{
+   DIR *dir;
+   struct dirent *de;
+   int dirnamelen;
+   struct strbuf path = STRBUF_INIT;
+   char *old_graph_path, *new_graph_path;
+
+   if (old_graph_hash)
+   old_graph_path = get_commit_graph_filename_hash(pack_dir, 
old_graph_hash);
+   else
+   old_graph_path = NULL;
+   new_graph_path = get_commit_graph_filename_hash(pack_dir, 
new_graph_hash);
+
+   dir = opendir(pack_dir);
+   if (!dir) {
+   if (errno != ENOENT)
+   error_errno("unable to open object pack directory: %s",
+   pack_dir);
+   return;
+   }
+
+   strbuf_addstr(, pack_dir);
+   strbuf_addch(, '/');
+
+   dirnamelen = path.len;
+   while ((de = readdir(dir)) != NULL) {
+   size_t base_len;
+
+   if (is_dot_or_dotdot(de->d_name))
+   continue;
+
+   strbuf_setlen(, dirnamelen);
+   strbuf_addstr(, de->d_name);
+
+   base_len = path.len;
+   if (strip_suffix_mem(path.buf, _len, 

[PATCH v3 13/14] commit-graph: read only from specific pack-indexes

[Derrick Stolee]

Teach git-commit-graph to inspect the objects only in a certain list
of pack-indexes within the given pack directory. This allows updating
the commit graph iteratively, since we add all commits stored in a
previous commit graph.

Signed-off-by: Derrick Stolee 
---
 Documentation/git-commit-graph.txt | 11 +++
 builtin/commit-graph.c | 32 +---
 commit-graph.c | 25 +++--
 commit-graph.h |  4 +++-
 packfile.c |  4 ++--
 packfile.h |  2 ++
 t/t5318-commit-graph.sh| 13 +
 7 files changed, 83 insertions(+), 8 deletions(-)

diff --git a/Documentation/git-commit-graph.txt 
b/Documentation/git-commit-graph.txt
index 7ae8f7484d..727d5d70bb 100644
--- a/Documentation/git-commit-graph.txt
+++ b/Documentation/git-commit-graph.txt
@@ -42,6 +42,10 @@ With the `--delete-expired` option, delete the graph files 
in the pack
 directory that are not referred to by the graph-head file. To avoid race
 conditions, do not delete the file previously referred to by the
 graph-head file if it is updated by the `--update-head` option.
++
+With the `--stdin-packs` option, generate the new commit graph by
+walking objects only in the specified packfiles and any commits in
+the existing graph-head.
 
 'read'::
 
@@ -72,6 +76,13 @@ $ git commit-graph write
 $ git commit-graph write --update-head --delete-expired
 
 
+* Write a graph file, extending the current graph file using commits
+* in , update graph-head, and delete stale graph files.
++
+
+$ echo  | git commit-graph write --update-head --delete-expired 
--stdin-packs
+
+
 * Read basic information from a graph file.
 +
 
diff --git a/builtin/commit-graph.c b/builtin/commit-graph.c
index 15f647fd81..fe5f00551c 100644
--- a/builtin/commit-graph.c
+++ b/builtin/commit-graph.c
@@ -9,7 +9,7 @@ static char const * const builtin_commit_graph_usage[] = {
N_("git commit-graph [--pack-dir ]"),
N_("git commit-graph clear [--pack-dir ]"),
N_("git commit-graph read [--graph-hash=]"),
-   N_("git commit-graph write [--pack-dir ] [--update-head] 
[--delete-expired]"),
+   N_("git commit-graph write [--pack-dir ] [--update-head] 
[--delete-expired] [--stdin-packs]"),
NULL
 };
 
@@ -24,7 +24,7 @@ static const char * const builtin_commit_graph_read_usage[] = 
{
 };
 
 static const char * const builtin_commit_graph_write_usage[] = {
-   N_("git commit-graph write [--pack-dir ] [--update-head] 
[--delete-expired]"),
+   N_("git commit-graph write [--pack-dir ] [--update-head] 
[--delete-expired] [--stdin-packs]"),
NULL
 };
 
@@ -33,6 +33,7 @@ static struct opts_commit_graph {
const char *graph_hash;
int update_head;
int delete_expired;
+   int stdin_packs;
 } opts;
 
 static int graph_clear(int argc, const char **argv)
@@ -216,6 +217,11 @@ static int graph_write(int argc, const char **argv)
struct object_id *graph_hash;
struct object_id old_graph_hash;
int has_existing;
+   const char **pack_indexes = NULL;
+   int nr_packs = 0;
+   const char **lines = NULL;
+   int nr_lines = 0;
+   int alloc_lines = 0;
 
static struct option builtin_commit_graph_write_options[] = {
{ OPTION_STRING, 'p', "pack-dir", _dir,
@@ -225,6 +231,8 @@ static int graph_write(int argc, const char **argv)
N_("update graph-head to written graph file")),
OPT_BOOL('d', "delete-expired", _expired,
N_("delete expired head graph file")),
+   OPT_BOOL('s', "stdin-packs", _packs,
+   N_("only scan packfiles listed by stdin")),
OPT_END(),
};
 
@@ -241,7 +249,25 @@ static int graph_write(int argc, const char **argv)
 
has_existing = !!get_graph_head_hash(opts.pack_dir, _graph_hash);
 
-   graph_hash = write_commit_graph(opts.pack_dir);
+   if (opts.stdin_packs) {
+   struct strbuf buf = STRBUF_INIT;
+   nr_lines = 0;
+   alloc_lines = 128;
+   ALLOC_ARRAY(lines, alloc_lines);
+
+   while (strbuf_getline(, stdin) != EOF) {
+   ALLOC_GROW(lines, nr_lines + 1, alloc_lines);
+   lines[nr_lines++] = buf.buf;
+   strbuf_detach(, NULL);
+   }
+
+   pack_indexes = lines;
+   nr_packs = nr_lines;
+   }
+
+   graph_hash = write_commit_graph(opts.pack_dir,
+   pack_indexes,
+   nr_packs);
 
if (opts.update_head)

[PATCH v3 02/14] graph: add commit graph design document

[Derrick Stolee]

Add Documentation/technical/commit-graph.txt with details of the planned
commit graph feature, including future plans.

Signed-off-by: Derrick Stolee 
---
 Documentation/technical/commit-graph.txt | 189 +++
 1 file changed, 189 insertions(+)
 create mode 100644 Documentation/technical/commit-graph.txt

diff --git a/Documentation/technical/commit-graph.txt 
b/Documentation/technical/commit-graph.txt
new file mode 100644
index 00..fc86b06041
--- /dev/null
+++ b/Documentation/technical/commit-graph.txt
@@ -0,0 +1,189 @@
+Git Commit Graph Design Notes
+=
+
+Git walks the commit graph for many reasons, including:
+
+1. Listing and filtering commit history.
+2. Computing merge bases.
+
+These operations can become slow as the commit count grows. The merge
+base calculation shows up in many user-facing commands, such as 'merge-base'
+or 'status' and can take minutes to compute depending on history shape.
+
+There are two main costs here:
+
+1. Decompressing and parsing commits.
+2. Walking the entire graph to avoid topological order mistakes.
+
+The commit graph file is a supplemental data structure that accelerates
+commit graph walks. If a user downgrades or disables the 'core.commitGraph'
+config setting, then the existing ODB is sufficient. The file is stored
+next to packfiles either in the .git/objects/pack directory or in the pack
+directory of an alternate.
+
+The commit graph file stores the commit graph structure along with some
+extra metadata to speed up graph walks. By listing commit OIDs in lexi-
+cographic order, we can identify an integer position for each commit and
+refer to the parents of a commit using those integer positions. We use
+binary search to find initial commits and then use the integer positions
+for fast lookups during the walk.
+
+A consumer may load the following info for a commit from the graph:
+
+1. The commit OID.
+2. The list of parents, along with their integer position.
+3. The commit date.
+4. The root tree OID.
+5. The generation number (see definition below).
+
+Values 1-4 satisfy the requirements of parse_commit_gently().
+
+Define the "generation number" of a commit recursively as follows:
+
+ * A commit with no parents (a root commit) has generation number one.
+
+ * A commit with at least one parent has generation number one more than
+   the largest generation number among its parents.
+
+Equivalently, the generation number of a commit A is one more than the
+length of a longest path from A to a root commit. The recursive definition
+is easier to use for computation and observing the following property:
+
+If A and B are commits with generation numbers N and M, respectively,
+and N <= M, then A cannot reach B. That is, we know without searching
+that B is not an ancestor of A because it is further from a root commit
+than A.
+
+Conversely, when checking if A is an ancestor of B, then we only need
+to walk commits until all commits on the walk boundary have generation
+number at most N. If we walk commits using a priority queue seeded by
+generation numbers, then we always expand the boundary commit with highest
+generation number and can easily detect the stopping condition.
+
+This property can be used to significantly reduce the time it takes to
+walk commits and determine topological relationships. Without generation
+numbers, the general heuristic is the following:
+
+If A and B are commits with commit time X and Y, respectively, and
+X < Y, then A _probably_ cannot reach B.
+
+This heuristic is currently used whenever the computation can make
+mistakes with topological orders (such as "git log" with default order),
+but is not used when the topological order is required (such as merge
+base calculations, "git log --graph").
+
+In practice, we expect some commits to be created recently and not stored
+in the commit graph. We can treat these commits as having "infinite"
+generation number and walk until reaching commits with known generation
+number.
+
+Design Details
+--
+
+- A graph file is stored in a file named 'graph-.graph' in the pack
+  directory. This could be stored in an alternate.
+
+- The most-recent graph file hash is stored in a 'graph-head' file for
+  immediate access and storing backup graphs. This could be stored in an
+  alternate, and refers to a 'graph-.graph' file in the same pack
+  directory.
+
+- The core.commitGraph config setting must be on to consume graph files.
+
+- The file format includes parameters for the object id length and hash
+  algorithm, so a future change of hash algorithm does not require a change
+  in format.
+
+Current Limitations
+---
+
+- Only one graph file is used at one time. This allows the integer position
+  to seek into the single graph file. It is possible to extend the model
+  for multiple graph files, but that is currently not part of the design.

[PATCH v3 06/14] commit-graph: implement 'git-commit-graph read'

[Derrick Stolee]

Teach git-commit-graph to read commit graph files and summarize their contents.

Use the read subcommand to verify the contents of a commit graph file in the
tests.

Signed-off-by: Derrick Stolee 
---
 Documentation/git-commit-graph.txt |  16 
 builtin/commit-graph.c |  71 ++
 commit-graph.c | 147 +
 commit-graph.h |  23 ++
 t/t5318-commit-graph.sh|  34 +++--
 5 files changed, 286 insertions(+), 5 deletions(-)

diff --git a/Documentation/git-commit-graph.txt 
b/Documentation/git-commit-graph.txt
index 55dfe5c3d8..67e107f06a 100644
--- a/Documentation/git-commit-graph.txt
+++ b/Documentation/git-commit-graph.txt
@@ -9,6 +9,7 @@ git-commit-graph - Write and verify Git commit graphs (.graph 
files)
 SYNOPSIS
 
 [verse]
+'git commit-graph read'  [--pack-dir ]
 'git commit-graph write'  [--pack-dir ]
 
 
@@ -34,6 +35,15 @@ Includes all commits from the existing commit graph file. 
Outputs the
 checksum hash of the written file.
 
 
+'read'::
+
+Read a graph file given by the graph-head file and output basic
+details about the graph file.
++
+With `--graph-hash=` option, consider the graph file
+graph-.graph in the pack directory.
+
+
 EXAMPLES
 
 
@@ -43,6 +53,12 @@ EXAMPLES
 $ git commit-graph write
 
 
+* Read basic information from a graph file.
++
+
+$ git commit-graph read --graph-hash=
+
+
 
 GIT
 ---
diff --git a/builtin/commit-graph.c b/builtin/commit-graph.c
index 5dac033bfe..3ffa7ec433 100644
--- a/builtin/commit-graph.c
+++ b/builtin/commit-graph.c
@@ -5,10 +5,16 @@
 
 static char const * const builtin_commit_graph_usage[] = {
N_("git commit-graph [--pack-dir ]"),
+   N_("git commit-graph read [--graph-hash=]"),
N_("git commit-graph write [--pack-dir ]"),
NULL
 };
 
+static const char * const builtin_commit_graph_read_usage[] = {
+   N_("git commit-graph read [--pack-dir ]"),
+   NULL
+};
+
 static const char * const builtin_commit_graph_write_usage[] = {
N_("git commit-graph write [--pack-dir ]"),
NULL
@@ -16,8 +22,71 @@ static const char * const builtin_commit_graph_write_usage[] 
= {
 
 static struct opts_commit_graph {
const char *pack_dir;
+   const char *graph_hash;
 } opts;
 
+static int graph_read(int argc, const char **argv)
+{
+   struct object_id graph_hash;
+   struct commit_graph *graph = 0;
+   const char *graph_file;
+
+   static struct option builtin_commit_graph_read_options[] = {
+   { OPTION_STRING, 'p', "pack-dir", _dir,
+   N_("dir"),
+   N_("The pack directory to store the graph") },
+   { OPTION_STRING, 'H', "graph-hash", _hash,
+   N_("hash"),
+   N_("A hash for a specific graph file in the pack-dir."),
+   PARSE_OPT_OPTARG, NULL, (intptr_t) "" },
+   OPT_END(),
+   };
+
+   argc = parse_options(argc, argv, NULL,
+builtin_commit_graph_read_options,
+builtin_commit_graph_read_usage, 0);
+
+   if (!opts.pack_dir) {
+   struct strbuf path = STRBUF_INIT;
+   strbuf_addstr(, get_object_directory());
+   strbuf_addstr(, "/pack");
+   opts.pack_dir = strbuf_detach(, NULL);
+   }
+
+   if (opts.graph_hash && strlen(opts.graph_hash) == GIT_MAX_HEXSZ)
+   get_oid_hex(opts.graph_hash, _hash);
+   else
+   die("no graph hash specified");
+
+   graph_file = get_commit_graph_filename_hash(opts.pack_dir, _hash);
+   graph = load_commit_graph_one(graph_file, opts.pack_dir);
+
+   if (!graph)
+   die("graph file %s does not exist", graph_file);
+
+   printf("header: %08x %02x %02x %02x %02x\n",
+   ntohl(*(uint32_t*)graph->data),
+   *(unsigned char*)(graph->data + 4),
+   *(unsigned char*)(graph->data + 5),
+   graph->hash_len,
+   graph->num_chunks);
+   printf("num_commits: %u\n", graph->num_commits);
+   printf("chunks:");
+
+   if (graph->chunk_oid_fanout)
+   printf(" oid_fanout");
+   if (graph->chunk_oid_lookup)
+   printf(" oid_lookup");
+   if (graph->chunk_commit_data)
+   printf(" commit_metadata");
+   if (graph->chunk_large_edges)
+   printf(" large_edges");
+   printf("\n");
+
+   printf("pack_dir: %s\n", graph->pack_dir);
+   return 0;
+}
+
 static int graph_write(int argc, const char **argv)
 {
struct object_id *graph_hash;
@@ -70,6 +139,8 @@ int cmd_commit_graph(int argc, const char **argv, const char 
*prefix)
  

[PATCH v3 14/14] commit-graph: build graph from starting commits

[Derrick Stolee]

Teach git-commit-graph to read commits from stdin when the
--stdin-commits flag is specified. Commits reachable from these
commits are added to the graph. This is a much faster way to construct
the graph than inspecting all packed objects, but is restricted to
known tips.

For the Linux repository, 700,000+ commits were added to the graph
file starting from 'master' in 7-9 seconds, depending on the number
of packfiles in the repo (1, 24, or 120).

Signed-off-by: Derrick Stolee 
---
 Documentation/git-commit-graph.txt | 15 ++-
 builtin/commit-graph.c | 26 +-
 commit-graph.c | 26 --
 commit-graph.h |  4 +++-
 t/t5318-commit-graph.sh| 19 +++
 5 files changed, 81 insertions(+), 9 deletions(-)

diff --git a/Documentation/git-commit-graph.txt 
b/Documentation/git-commit-graph.txt
index 727d5d70bb..bd1c54025a 100644
--- a/Documentation/git-commit-graph.txt
+++ b/Documentation/git-commit-graph.txt
@@ -45,7 +45,12 @@ graph-head file if it is updated by the `--update-head` 
option.
 +
 With the `--stdin-packs` option, generate the new commit graph by
 walking objects only in the specified packfiles and any commits in
-the existing graph-head.
+the existing graph-head. (Cannot be combined with --stdin-commits.)
++
+With the `--stdin-commits` option, generate the new commit graph by
+walking commits starting at the commits specified in stdin as a list
+of OIDs in hex, one OID per line. (Cannot be combined with
+--stdin-packs.)
 
 'read'::
 
@@ -83,6 +88,14 @@ $ git commit-graph write --update-head --delete-expired
 $ echo  | git commit-graph write --update-head --delete-expired 
--stdin-packs
 
 
+* Write a graph file, extending the current graph file using all
+* commits reachable from refs/heads/*, update graph-head, and delete
+* stale graph files.
++
+
+$ git show-ref --heads -s | git commit-graph write --update-head 
--delete-expired --stdin-commits
+
+
 * Read basic information from a graph file.
 +
 
diff --git a/builtin/commit-graph.c b/builtin/commit-graph.c
index fe5f00551c..28d043b5a8 100644
--- a/builtin/commit-graph.c
+++ b/builtin/commit-graph.c
@@ -9,7 +9,7 @@ static char const * const builtin_commit_graph_usage[] = {
N_("git commit-graph [--pack-dir ]"),
N_("git commit-graph clear [--pack-dir ]"),
N_("git commit-graph read [--graph-hash=]"),
-   N_("git commit-graph write [--pack-dir ] [--update-head] 
[--delete-expired] [--stdin-packs]"),
+   N_("git commit-graph write [--pack-dir ] [--update-head] 
[--delete-expired] [--stdin-packs|--stdin-commits]"),
NULL
 };
 
@@ -24,7 +24,7 @@ static const char * const builtin_commit_graph_read_usage[] = 
{
 };
 
 static const char * const builtin_commit_graph_write_usage[] = {
-   N_("git commit-graph write [--pack-dir ] [--update-head] 
[--delete-expired] [--stdin-packs]"),
+   N_("git commit-graph write [--pack-dir ] [--update-head] 
[--delete-expired] [--stdin-packs|--stdin-commits]"),
NULL
 };
 
@@ -34,6 +34,7 @@ static struct opts_commit_graph {
int update_head;
int delete_expired;
int stdin_packs;
+   int stdin_commits;
 } opts;
 
 static int graph_clear(int argc, const char **argv)
@@ -219,6 +220,8 @@ static int graph_write(int argc, const char **argv)
int has_existing;
const char **pack_indexes = NULL;
int nr_packs = 0;
+   const char **commit_hex = NULL;
+   int nr_commits = 0;
const char **lines = NULL;
int nr_lines = 0;
int alloc_lines = 0;
@@ -233,6 +236,8 @@ static int graph_write(int argc, const char **argv)
N_("delete expired head graph file")),
OPT_BOOL('s', "stdin-packs", _packs,
N_("only scan packfiles listed by stdin")),
+   OPT_BOOL('C', "stdin-commits", _commits,
+   N_("start walk at commits listed by stdin")),
OPT_END(),
};
 
@@ -240,6 +245,9 @@ static int graph_write(int argc, const char **argv)
 builtin_commit_graph_write_options,
 builtin_commit_graph_write_usage, 0);
 
+   if (opts.stdin_packs && opts.stdin_commits)
+   die(_("cannot use both --stdin-commits and --stdin-packs"));
+
if (!opts.pack_dir) {
struct strbuf path = STRBUF_INIT;
strbuf_addstr(, get_object_directory());
@@ -261,13 +269,21 @@ static int graph_write(int argc, const char **argv)
strbuf_detach(, NULL);
}
 
-   pack_indexes = lines;
-   nr_packs = nr_lines;
+   if 

[PATCH v3 04/14] commit-graph: implement write_commit_graph()

[Derrick Stolee]

Teach Git to write a commit graph file by checking all packed objects
to see if they are commits, then store the file in the given pack
directory.

Signed-off-by: Derrick Stolee 
---
 Makefile   |   1 +
 commit-graph.c | 368 +
 commit-graph.h |  13 ++
 3 files changed, 382 insertions(+)
 create mode 100644 commit-graph.c
 create mode 100644 commit-graph.h

diff --git a/Makefile b/Makefile
index fc40b816dc..eeaeb6a745 100644
--- a/Makefile
+++ b/Makefile
@@ -761,6 +761,7 @@ LIB_OBJS += color.o
 LIB_OBJS += column.o
 LIB_OBJS += combine-diff.o
 LIB_OBJS += commit.o
+LIB_OBJS += commit-graph.o
 LIB_OBJS += compat/obstack.o
 LIB_OBJS += compat/terminal.o
 LIB_OBJS += config.o
diff --git a/commit-graph.c b/commit-graph.c
new file mode 100644
index 00..cb47b68871
--- /dev/null
+++ b/commit-graph.c
@@ -0,0 +1,368 @@
+#include "cache.h"
+#include "config.h"
+#include "git-compat-util.h"
+#include "pack.h"
+#include "packfile.h"
+#include "commit.h"
+#include "object.h"
+#include "revision.h"
+#include "sha1-lookup.h"
+#include "commit-graph.h"
+
+#define GRAPH_SIGNATURE 0x43475048 /* "CGPH" */
+#define GRAPH_CHUNKID_OIDFANOUT 0x4f494446 /* "OIDF" */
+#define GRAPH_CHUNKID_OIDLOOKUP 0x4f49444c /* "OIDL" */
+#define GRAPH_CHUNKID_DATA 0x43444154 /* "CDAT" */
+#define GRAPH_CHUNKID_LARGEEDGES 0x45444745 /* "EDGE" */
+
+#define GRAPH_DATA_WIDTH 36
+
+#define GRAPH_VERSION_1 0x1
+#define GRAPH_VERSION GRAPH_VERSION_1
+
+#define GRAPH_OID_VERSION_SHA1 1
+#define GRAPH_OID_LEN_SHA1 20
+#define GRAPH_OID_VERSION GRAPH_OID_VERSION_SHA1
+#define GRAPH_OID_LEN GRAPH_OID_LEN_SHA1
+
+#define GRAPH_LARGE_EDGES_NEEDED 0x8000
+#define GRAPH_PARENT_MISSING 0x7fff
+#define GRAPH_EDGE_LAST_MASK 0x7fff
+#define GRAPH_PARENT_NONE 0x7000
+
+#define GRAPH_LAST_EDGE 0x8000
+
+#define GRAPH_FANOUT_SIZE (4 * 256)
+#define GRAPH_CHUNKLOOKUP_WIDTH 12
+#define GRAPH_CHUNKLOOKUP_SIZE (5 * GRAPH_CHUNKLOOKUP_WIDTH)
+#define GRAPH_MIN_SIZE (GRAPH_CHUNKLOOKUP_SIZE + GRAPH_FANOUT_SIZE + \
+   GRAPH_OID_LEN + 8)
+
+char* get_commit_graph_filename_hash(const char *pack_dir,
+struct object_id *hash)
+{
+   size_t len;
+   struct strbuf path = STRBUF_INIT;
+   strbuf_addstr(, pack_dir);
+   strbuf_addstr(, "/graph-");
+   strbuf_addstr(, oid_to_hex(hash));
+   strbuf_addstr(, ".graph");
+
+   return strbuf_detach(, );
+}
+
+static void write_graph_chunk_fanout(struct sha1file *f,
+struct commit **commits,
+int nr_commits)
+{
+   uint32_t i, count = 0;
+   struct commit **list = commits;
+   struct commit **last = commits + nr_commits;
+
+   /*
+* Write the first-level table (the list is sorted,
+* but we use a 256-entry lookup to be able to avoid
+* having to do eight extra binary search iterations).
+*/
+   for (i = 0; i < 256; i++) {
+   while (list < last) {
+   if ((*list)->object.oid.hash[0] != i)
+   break;
+   count++;
+   list++;
+   }
+
+   sha1write_be32(f, count);
+   }
+}
+
+static void write_graph_chunk_oids(struct sha1file *f, int hash_len,
+  struct commit **commits, int nr_commits)
+{
+   struct commit **list, **last = commits + nr_commits;
+   for (list = commits; list < last; list++)
+   sha1write(f, (*list)->object.oid.hash, (int)hash_len);
+}
+
+static int commit_pos(struct commit **commits, int nr_commits,
+ const struct object_id *oid, uint32_t *pos)
+{
+   uint32_t first = 0, last = nr_commits;
+
+   while (first < last) {
+   uint32_t mid = first + (last - first) / 2;
+   struct object_id *current;
+   int cmp;
+
+   current = &(commits[mid]->object.oid);
+   cmp = oidcmp(oid, current);
+   if (!cmp) {
+   *pos = mid;
+   return 1;
+   }
+   if (cmp > 0) {
+   first = mid + 1;
+   continue;
+   }
+   last = mid;
+   }
+
+   *pos = first;
+   return 0;
+}
+
+static void write_graph_chunk_data(struct sha1file *f, int hash_len,
+  struct commit **commits, int nr_commits)
+{
+   struct commit **list = commits;
+   struct commit **last = commits + nr_commits;
+   uint32_t num_large_edges = 0;
+
+   while (list < last) {
+   struct commit_list *parent;
+   uint32_t int_id;
+   uint32_t packedDate[2];
+
+   parse_commit(*list);
+   sha1write(f, (*list)->tree->object.oid.hash, hash_len);
+
+   

[PATCH v3 05/14] commit-graph: implement 'git-commit-graph write'

[Derrick Stolee]

Teach git-commit-graph to write graph files. Create new test script to verify
this command succeeds without failure.

Signed-off-by: Derrick Stolee 
---
 Documentation/git-commit-graph.txt |  39 +
 builtin/commit-graph.c |  43 +++
 t/t5318-commit-graph.sh| 109 +
 3 files changed, 191 insertions(+)
 create mode 100755 t/t5318-commit-graph.sh

diff --git a/Documentation/git-commit-graph.txt 
b/Documentation/git-commit-graph.txt
index e1c3078ca1..55dfe5c3d8 100644
--- a/Documentation/git-commit-graph.txt
+++ b/Documentation/git-commit-graph.txt
@@ -5,6 +5,45 @@ NAME
 
 git-commit-graph - Write and verify Git commit graphs (.graph files)
 
+
+SYNOPSIS
+
+[verse]
+'git commit-graph write'  [--pack-dir ]
+
+
+DESCRIPTION
+---
+
+Manage the serialized commit graph file.
+
+
+OPTIONS
+---
+--pack-dir::
+   Use given directory for the location of packfiles, graph-head,
+   and graph files.
+
+
+COMMANDS
+
+'write'::
+
+Write a commit graph file based on the commits found in packfiles.
+Includes all commits from the existing commit graph file. Outputs the
+checksum hash of the written file.
+
+
+EXAMPLES
+
+
+* Write a commit graph file for the packed commits in your local .git folder.
++
+
+$ git commit-graph write
+
+
+
 GIT
 ---
 Part of the linkgit:git[1] suite
diff --git a/builtin/commit-graph.c b/builtin/commit-graph.c
index a01c5d9981..5dac033bfe 100644
--- a/builtin/commit-graph.c
+++ b/builtin/commit-graph.c
@@ -1,9 +1,16 @@
 #include "builtin.h"
 #include "config.h"
 #include "parse-options.h"
+#include "commit-graph.h"
 
 static char const * const builtin_commit_graph_usage[] = {
N_("git commit-graph [--pack-dir ]"),
+   N_("git commit-graph write [--pack-dir ]"),
+   NULL
+};
+
+static const char * const builtin_commit_graph_write_usage[] = {
+   N_("git commit-graph write [--pack-dir ]"),
NULL
 };
 
@@ -11,6 +18,37 @@ static struct opts_commit_graph {
const char *pack_dir;
 } opts;
 
+static int graph_write(int argc, const char **argv)
+{
+   struct object_id *graph_hash;
+
+   static struct option builtin_commit_graph_write_options[] = {
+   { OPTION_STRING, 'p', "pack-dir", _dir,
+   N_("dir"),
+   N_("The pack directory to store the graph") },
+   OPT_END(),
+   };
+
+   argc = parse_options(argc, argv, NULL,
+builtin_commit_graph_write_options,
+builtin_commit_graph_write_usage, 0);
+
+   if (!opts.pack_dir) {
+   struct strbuf path = STRBUF_INIT;
+   strbuf_addstr(, get_object_directory());
+   strbuf_addstr(, "/pack");
+   opts.pack_dir = strbuf_detach(, NULL);
+   }
+
+   graph_hash = write_commit_graph(opts.pack_dir);
+
+   if (graph_hash) {
+   printf("%s\n", oid_to_hex(graph_hash));
+   FREE_AND_NULL(graph_hash);
+   }
+
+   return 0;
+}
 
 int cmd_commit_graph(int argc, const char **argv, const char *prefix)
 {
@@ -31,6 +69,11 @@ int cmd_commit_graph(int argc, const char **argv, const char 
*prefix)
 builtin_commit_graph_usage,
 PARSE_OPT_STOP_AT_NON_OPTION);
 
+   if (argc > 0) {
+   if (!strcmp(argv[0], "write"))
+   return graph_write(argc, argv);
+   }
+
usage_with_options(builtin_commit_graph_usage,
   builtin_commit_graph_options);
 }
diff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh
new file mode 100755
index 00..b762587595
--- /dev/null
+++ b/t/t5318-commit-graph.sh
@@ -0,0 +1,109 @@
+#!/bin/sh
+
+test_description='commit graph'
+. ./test-lib.sh
+
+test_expect_success 'setup full repo' '
+   rm -rf .git &&
+   mkdir full &&
+   cd full &&
+   git init &&
+   packdir=".git/objects/pack"'
+
+test_expect_success 'write graph with no packs' '
+   git commit-graph write --pack-dir .'
+
+test_expect_success 'create commits and repack' '
+   for i in $(test_seq 3)
+   do
+   test_commit $i &&
+   git branch commits/$i
+   done &&
+   git repack'
+
+test_expect_success 'write graph' '
+   graph1=$(git commit-graph write) &&
+   test_path_is_file $packdir/graph-$graph1.graph'
+
+test_expect_success 'Add more commits' '
+   git reset --hard commits/1 &&
+   for i in $(test_seq 4 5)
+   do
+   test_commit $i &&
+   git branch commits/$i
+   done &&
+   git reset --hard commits/2 &&
+   for i in $(test_seq 6 7)
+   do
+   test_commit $i &&
+   git branch commits/$i
+   done &&
+   git 

[PATCH v3 10/14] commit-graph: add core.commitGraph setting

[Derrick Stolee]

The commit graph feature is controlled by the new core.commitGraph config
setting. This defaults to 0, so the feature is opt-in.

The intention of core.commitGraph is that a user can always stop checking
for or parsing commit graph files if core.commitGraph=0.

Signed-off-by: Derrick Stolee 
---
 Documentation/config.txt | 3 +++
 cache.h  | 1 +
 config.c | 5 +
 environment.c| 1 +
 4 files changed, 10 insertions(+)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 9593bfabaa..e90d0d1262 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -883,6 +883,9 @@ core.notesRef::
 This setting defaults to "refs/notes/commits", and it can be overridden by
 the `GIT_NOTES_REF` environment variable.  See linkgit:git-notes[1].
 
+core.commitGraph::
+   Enable git commit graph feature. Allows reading from .graph files.
+
 core.sparseCheckout::
Enable "sparse checkout" feature. See section "Sparse checkout" in
linkgit:git-read-tree[1] for more information.
diff --git a/cache.h b/cache.h
index 6440e2bf21..1063873316 100644
--- a/cache.h
+++ b/cache.h
@@ -771,6 +771,7 @@ extern char *git_replace_ref_base;
 
 extern int fsync_object_files;
 extern int core_preload_index;
+extern int core_commit_graph;
 extern int core_apply_sparse_checkout;
 extern int precomposed_unicode;
 extern int protect_hfs;
diff --git a/config.c b/config.c
index 41862d4a32..614cf59ac4 100644
--- a/config.c
+++ b/config.c
@@ -1213,6 +1213,11 @@ static int git_default_core_config(const char *var, 
const char *value)
return 0;
}
 
+   if (!strcmp(var, "core.commitgraph")) {
+   core_commit_graph = git_config_bool(var, value);
+   return 0;
+   }
+
if (!strcmp(var, "core.sparsecheckout")) {
core_apply_sparse_checkout = git_config_bool(var, value);
return 0;
diff --git a/environment.c b/environment.c
index 8289c25b44..81fed83c50 100644
--- a/environment.c
+++ b/environment.c
@@ -60,6 +60,7 @@ enum push_default_type push_default = 
PUSH_DEFAULT_UNSPECIFIED;
 enum object_creation_mode object_creation_mode = OBJECT_CREATION_MODE;
 char *notes_ref_name;
 int grafts_replace_parents = 1;
+int core_commit_graph;
 int core_apply_sparse_checkout;
 int merge_log_config = -1;
 int precomposed_unicode = -1; /* see probe_utf8_pathname_composition() */
-- 
2.15.1.45.g9b7079f

[PATCH v3 08/14] commit-graph: implement 'git-commit-graph clear'

[Derrick Stolee]

Teach Git to delete the current 'graph_head' file and the commit graph
it references. This is a good safety valve if somehow the file is
corrupted and needs to be recalculated. Since the commit graph is a
summary of contents already in the ODB, it can be regenerated.

Signed-off-by: Derrick Stolee 
---
 Documentation/git-commit-graph.txt | 11 +
 builtin/commit-graph.c | 50 ++
 commit-graph.c | 23 ++
 commit-graph.h |  2 ++
 t/t5318-commit-graph.sh|  5 
 5 files changed, 91 insertions(+)

diff --git a/Documentation/git-commit-graph.txt 
b/Documentation/git-commit-graph.txt
index 5e32c43b27..8c2cbbc923 100644
--- a/Documentation/git-commit-graph.txt
+++ b/Documentation/git-commit-graph.txt
@@ -9,6 +9,7 @@ git-commit-graph - Write and verify Git commit graphs (.graph 
files)
 SYNOPSIS
 
 [verse]
+'git commit-graph clear' [--pack-dir ]
 'git commit-graph read'  [--pack-dir ]
 'git commit-graph write'  [--pack-dir ]
 
@@ -45,6 +46,10 @@ details about the graph file.
 With `--graph-hash=` option, consider the graph file
 graph-.graph in the pack directory.
 
+'clear'::
+
+Delete the graph-head file and the graph file it references.
+
 
 EXAMPLES
 
@@ -68,6 +73,12 @@ $ git commit-graph write --update-head
 $ git commit-graph read --graph-hash=
 
 
+* Delete /graph-head and the file it references.
++
+
+$ git commit-graph clear --pack-dir=
+
+
 
 GIT
 ---
diff --git a/builtin/commit-graph.c b/builtin/commit-graph.c
index 776ca087e8..529cb80de6 100644
--- a/builtin/commit-graph.c
+++ b/builtin/commit-graph.c
@@ -1,16 +1,23 @@
 #include "builtin.h"
 #include "config.h"
+#include "dir.h"
 #include "lockfile.h"
 #include "parse-options.h"
 #include "commit-graph.h"
 
 static char const * const builtin_commit_graph_usage[] = {
N_("git commit-graph [--pack-dir ]"),
+   N_("git commit-graph clear [--pack-dir ]"),
N_("git commit-graph read [--graph-hash=]"),
N_("git commit-graph write [--pack-dir ] [--update-head]"),
NULL
 };
 
+static const char * const builtin_commit_graph_clear_usage[] = {
+   N_("git commit-graph clear [--pack-dir ]"),
+   NULL
+};
+
 static const char * const builtin_commit_graph_read_usage[] = {
N_("git commit-graph read [--pack-dir ]"),
NULL
@@ -27,6 +34,47 @@ static struct opts_commit_graph {
int update_head;
 } opts;
 
+static int graph_clear(int argc, const char **argv)
+{
+   char *old_path;
+   char *head_fname;
+   struct object_id old_graph_hash;
+
+   static struct option builtin_commit_graph_clear_options[] = {
+   { OPTION_STRING, 'p', "pack-dir", _dir,
+   N_("dir"),
+   N_("The pack directory to store the graph") },
+   OPT_END(),
+   };
+
+   argc = parse_options(argc, argv, NULL,
+builtin_commit_graph_clear_options,
+builtin_commit_graph_clear_usage, 0);
+
+   if (!opts.pack_dir) {
+   struct strbuf path = STRBUF_INIT;
+   strbuf_addstr(, get_object_directory());
+   strbuf_addstr(, "/pack");
+   opts.pack_dir = strbuf_detach(, NULL);
+   }
+
+   if (!get_graph_head_hash(opts.pack_dir, _graph_hash))
+   return 0;
+
+   head_fname = get_graph_head_filename(opts.pack_dir);
+   if (remove_path(head_fname))
+   die("failed to remove path %s", head_fname);
+   FREE_AND_NULL(head_fname);
+
+   old_path = get_commit_graph_filename_hash(opts.pack_dir,
+ _graph_hash);
+   if (remove_path(old_path))
+   die("failed to remove path %s", old_path);
+   free(old_path);
+
+   return 0;
+}
+
 static int graph_read(int argc, const char **argv)
 {
struct object_id graph_hash;
@@ -162,6 +210,8 @@ int cmd_commit_graph(int argc, const char **argv, const 
char *prefix)
 PARSE_OPT_STOP_AT_NON_OPTION);
 
if (argc > 0) {
+   if (!strcmp(argv[0], "clear"))
+   return graph_clear(argc, argv);
if (!strcmp(argv[0], "read"))
return graph_read(argc, argv);
if (!strcmp(argv[0], "write"))
diff --git a/commit-graph.c b/commit-graph.c
index 9789fe37f9..95b813c2c7 100644
--- a/commit-graph.c
+++ b/commit-graph.c
@@ -46,6 +46,29 @@ char *get_graph_head_filename(const char *pack_dir)
return strbuf_detach(, 0);
 }
 
+struct object_id *get_graph_head_hash(const char *pack_dir, struct object_id 
*hash)
+{
+   char hex[GIT_MAX_HEXSZ + 1];
+   char *fname;
+   FILE *f;
+
+   fname = 

[PATCH v3 07/14] commit-graph: update graph-head during write

[Derrick Stolee]

It is possible to have multiple commit graph files in a pack directory,
but only one is important at a time. Use a 'graph_head' file to point
to the important file. Teach git-commit-graph to write 'graph_head' upon
writing a new commit graph file.

Signed-off-by: Derrick Stolee 
---
 Documentation/git-commit-graph.txt | 11 ++-
 builtin/commit-graph.c | 27 +--
 commit-graph.c |  8 
 commit-graph.h |  1 +
 t/t5318-commit-graph.sh| 25 +++--
 5 files changed, 63 insertions(+), 9 deletions(-)

diff --git a/Documentation/git-commit-graph.txt 
b/Documentation/git-commit-graph.txt
index 67e107f06a..5e32c43b27 100644
--- a/Documentation/git-commit-graph.txt
+++ b/Documentation/git-commit-graph.txt
@@ -33,7 +33,9 @@ COMMANDS
 Write a commit graph file based on the commits found in packfiles.
 Includes all commits from the existing commit graph file. Outputs the
 checksum hash of the written file.
-
++
+With `--update-head` option, update the graph-head file to point
+to the written graph file.
 
 'read'::
 
@@ -53,6 +55,13 @@ EXAMPLES
 $ git commit-graph write
 
 
+* Write a graph file for the packed commits in your local .git folder
+* and update graph-head.
++
+
+$ git commit-graph write --update-head
+
+
 * Read basic information from a graph file.
 +
 
diff --git a/builtin/commit-graph.c b/builtin/commit-graph.c
index 3ffa7ec433..776ca087e8 100644
--- a/builtin/commit-graph.c
+++ b/builtin/commit-graph.c
@@ -1,12 +1,13 @@
 #include "builtin.h"
 #include "config.h"
+#include "lockfile.h"
 #include "parse-options.h"
 #include "commit-graph.h"
 
 static char const * const builtin_commit_graph_usage[] = {
N_("git commit-graph [--pack-dir ]"),
N_("git commit-graph read [--graph-hash=]"),
-   N_("git commit-graph write [--pack-dir ]"),
+   N_("git commit-graph write [--pack-dir ] [--update-head]"),
NULL
 };
 
@@ -16,13 +17,14 @@ static const char * const builtin_commit_graph_read_usage[] 
= {
 };
 
 static const char * const builtin_commit_graph_write_usage[] = {
-   N_("git commit-graph write [--pack-dir ]"),
+   N_("git commit-graph write [--pack-dir ] [--update-head]"),
NULL
 };
 
 static struct opts_commit_graph {
const char *pack_dir;
const char *graph_hash;
+   int update_head;
 } opts;
 
 static int graph_read(int argc, const char **argv)
@@ -87,6 +89,22 @@ static int graph_read(int argc, const char **argv)
return 0;
 }
 
+static void update_head_file(const char *pack_dir, const struct object_id 
*graph_hash)
+{
+   int fd;
+   struct lock_file lk = LOCK_INIT;
+   char *head_fname = get_graph_head_filename(pack_dir);
+
+   fd = hold_lock_file_for_update(, head_fname, LOCK_DIE_ON_ERROR);
+   FREE_AND_NULL(head_fname);
+
+   if (fd < 0)
+   die_errno("unable to open graph-head");
+
+   write_in_full(fd, oid_to_hex(graph_hash), GIT_MAX_HEXSZ);
+   commit_lock_file();
+}
+
 static int graph_write(int argc, const char **argv)
 {
struct object_id *graph_hash;
@@ -95,6 +113,8 @@ static int graph_write(int argc, const char **argv)
{ OPTION_STRING, 'p', "pack-dir", _dir,
N_("dir"),
N_("The pack directory to store the graph") },
+   OPT_BOOL('u', "update-head", _head,
+   N_("update graph-head to written graph file")),
OPT_END(),
};
 
@@ -111,6 +131,9 @@ static int graph_write(int argc, const char **argv)
 
graph_hash = write_commit_graph(opts.pack_dir);
 
+   if (opts.update_head)
+   update_head_file(opts.pack_dir, graph_hash);
+
if (graph_hash) {
printf("%s\n", oid_to_hex(graph_hash));
FREE_AND_NULL(graph_hash);
diff --git a/commit-graph.c b/commit-graph.c
index 9a337cea4d..9789fe37f9 100644
--- a/commit-graph.c
+++ b/commit-graph.c
@@ -38,6 +38,14 @@
 #define GRAPH_MIN_SIZE (GRAPH_CHUNKLOOKUP_SIZE + GRAPH_FANOUT_SIZE + \
GRAPH_OID_LEN + 8)
 
+char *get_graph_head_filename(const char *pack_dir)
+{
+   struct strbuf fname = STRBUF_INIT;
+   strbuf_addstr(, pack_dir);
+   strbuf_addstr(, "/graph-head");
+   return strbuf_detach(, 0);
+}
+
 char* get_commit_graph_filename_hash(const char *pack_dir,
 struct object_id *hash)
 {
diff --git a/commit-graph.h b/commit-graph.h
index c1608976b3..726b8aa0f4 100644
--- a/commit-graph.h
+++ b/commit-graph.h
@@ -4,6 +4,7 @@
 #include "git-compat-util.h"
 #include "commit.h"
 
+extern char *get_graph_head_filename(const char *pack_dir);
 extern char* 

[PATCH v3 12/14] commit-graph: close under reachability

[Derrick Stolee]

Teach write_commit_graph() to walk all parents from the commits
discovered in packfiles. This prevents gaps given by loose objects or
previously-missed packfiles.

Also automatically add commits from the existing graph file, if it
exists.

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

diff --git a/commit-graph.c b/commit-graph.c
index aff67c458e..d711a2cd81 100644
--- a/commit-graph.c
+++ b/commit-graph.c
@@ -633,6 +633,28 @@ static int if_packed_commit_add_to_list(const struct 
object_id *oid,
return 0;
 }
 
+static void close_reachable(struct packed_oid_list *oids)
+{
+   int i;
+   struct rev_info revs;
+   struct commit *commit;
+   init_revisions(, NULL);
+   for (i = 0; i < oids->nr; i++) {
+   commit = lookup_commit(oids->list[i]);
+   if (commit && !parse_commit(commit))
+   revs.commits = commit_list_insert(commit, 
);
+   }
+
+   if (prepare_revision_walk())
+   die(_("revision walk setup failed"));
+
+   while ((commit = get_revision()) != NULL) {
+   ALLOC_GROW(oids->list, oids->nr + 1, oids->alloc);
+   oids->list[oids->nr] = &(commit->object.oid);
+   (oids->nr)++;
+   }
+}
+
 struct object_id *write_commit_graph(const char *pack_dir)
 {
struct packed_oid_list oids;
@@ -650,12 +672,27 @@ struct object_id *write_commit_graph(const char *pack_dir)
char *fname;
struct commit_list *parent;
 
+   prepare_commit_graph();
+
oids.nr = 0;
oids.alloc = 1024;
+
+   if (commit_graph && oids.alloc < commit_graph->num_commits)
+   oids.alloc = commit_graph->num_commits;
+
ALLOC_ARRAY(oids.list, oids.alloc);
 
+   if (commit_graph) {
+   for (i = 0; i < commit_graph->num_commits; i++) {
+   oids.list[i] = malloc(sizeof(struct object_id));
+   get_nth_commit_oid(commit_graph, i, oids.list[i]);
+   }
+   oids.nr = commit_graph->num_commits;
+   }
+
for_each_packed_object(if_packed_commit_add_to_list, , 0);
 
+   close_reachable();
QSORT(oids.list, oids.nr, commit_compare);
 
count_distinct = 1;
-- 
2.15.1.45.g9b7079f

[PATCH v3 11/14] commit: integrate commit graph with commit parsing

[Derrick Stolee]

Teach Git to inspect a commit graph file to supply the contents of a
struct commit when calling parse_commit_gently(). This implementation
satisfies all post-conditions on the struct commit, including loading
parents, the root tree, and the commit date. The only loosely-expected
condition is that the commit buffer is loaded into the cache. This
was checked in log-tree.c:show_log(), but the "return;" on failure
produced unexpected results (i.e. the message line was never terminated).
The new behavior of loading the buffer when needed prevents the
unexpected behavior.

If core.commitGraph is false, then do not check graph files.

In test script t5318-commit-graph.sh, add output-matching conditions on
read-only graph operations.

By loading commits from the graph instead of parsing commit buffers, we
save a lot of time on long commit walks. Here are some performance
results for a copy of the Linux repository where 'master' has 704,766
reachable commits and is behind 'origin/master' by 19,610 commits.

| Command  | Before | After  | Rel % |
|--|||---|
| log --oneline --topo-order -1000 |  5.9s  |  0.7s  | -88%  |
| branch -vv   |  0.42s |  0.27s | -35%  |
| rev-list --all   |  6.4s  |  1.0s  | -84%  |
| rev-list --all --objects | 32.6s  | 27.6s  | -15%  |

Signed-off-by: Derrick Stolee 
---
 alloc.c |   1 +
 commit-graph.c  | 202 
 commit-graph.h  |  21 -
 commit.c|   3 +
 commit.h|   3 +
 log-tree.c  |   3 +-
 t/t5318-commit-graph.sh |  44 ++-
 7 files changed, 272 insertions(+), 5 deletions(-)

diff --git a/alloc.c b/alloc.c
index 12afadfacd..cf4f8b61e1 100644
--- a/alloc.c
+++ b/alloc.c
@@ -93,6 +93,7 @@ void *alloc_commit_node(void)
struct commit *c = alloc_node(_state, sizeof(struct commit));
c->object.type = OBJ_COMMIT;
c->index = alloc_commit_index();
+   c->graph_pos = COMMIT_NOT_FROM_GRAPH;
return c;
 }
 
diff --git a/commit-graph.c b/commit-graph.c
index 95b813c2c7..aff67c458e 100644
--- a/commit-graph.c
+++ b/commit-graph.c
@@ -38,6 +38,9 @@
 #define GRAPH_MIN_SIZE (GRAPH_CHUNKLOOKUP_SIZE + GRAPH_FANOUT_SIZE + \
GRAPH_OID_LEN + 8)
 
+/* global storage */
+struct commit_graph *commit_graph = NULL;
+
 char *get_graph_head_filename(const char *pack_dir)
 {
struct strbuf fname = STRBUF_INIT;
@@ -229,6 +232,205 @@ struct commit_graph *load_commit_graph_one(const char 
*graph_file, const char *p
return graph;
 }
 
+static void prepare_commit_graph_one(const char *obj_dir)
+{
+   char *graph_file;
+   struct object_id oid;
+   struct strbuf pack_dir = STRBUF_INIT;
+   strbuf_addstr(_dir, obj_dir);
+   strbuf_add(_dir, "/pack", 5);
+
+   if (!get_graph_head_hash(pack_dir.buf, ))
+   return;
+
+   graph_file = get_commit_graph_filename_hash(pack_dir.buf, );
+
+   commit_graph = load_commit_graph_one(graph_file, pack_dir.buf);
+   strbuf_release(_dir);
+}
+
+static int prepare_commit_graph_run_once = 0;
+void prepare_commit_graph(void)
+{
+   struct alternate_object_database *alt;
+   char *obj_dir;
+
+   if (prepare_commit_graph_run_once)
+   return;
+   prepare_commit_graph_run_once = 1;
+
+   obj_dir = get_object_directory();
+   prepare_commit_graph_one(obj_dir);
+   prepare_alt_odb();
+   for (alt = alt_odb_list; !commit_graph && alt; alt = alt->next)
+   prepare_commit_graph_one(alt->path);
+}
+
+static int bsearch_graph(struct commit_graph *g, struct object_id *oid, 
uint32_t *pos)
+{
+   int result = bsearch_hash(oid->hash, g->chunk_oid_fanout,
+ g->chunk_oid_lookup, g->hash_len);
+
+   if (result >= 0) {
+   *pos = result;
+   return 1;
+   } else {
+   *pos = -result - 1;
+   return 0;
+   }
+}
+
+struct object_id *get_nth_commit_oid(struct commit_graph *g,
+uint32_t n,
+struct object_id *oid)
+{
+   hashcpy(oid->hash, g->chunk_oid_lookup + g->hash_len * n);
+   return oid;
+}
+
+static struct commit_list **insert_parent_or_die(struct commit_graph *g,
+  int pos,
+  struct commit_list **pptr)
+{
+   struct commit *c;
+   struct object_id oid;
+   get_nth_commit_oid(g, pos, );
+   c = lookup_commit();
+   if (!c)
+   die("could not find commit %s", oid_to_hex());
+   c->graph_pos = pos;
+   return _list_insert(c, pptr)->next;
+}
+
+static int check_commit_parents(struct commit *item, struct commit_graph *g,
+   uint32_t pos, const 

[PATCH v3 03/14] commit-graph: create git-commit-graph builtin

[Derrick Stolee]

Teach git the 'commit-graph' builtin that will be used for writing and
reading packed graph files. The current implementation is mostly
empty, except for a '--pack-dir' option.

Signed-off-by: Derrick Stolee 
---
 .gitignore |  1 +
 Documentation/git-commit-graph.txt | 11 +++
 Makefile   |  1 +
 builtin.h  |  1 +
 builtin/commit-graph.c | 37 +
 command-list.txt   |  1 +
 git.c  |  1 +
 7 files changed, 53 insertions(+)
 create mode 100644 Documentation/git-commit-graph.txt
 create mode 100644 builtin/commit-graph.c

diff --git a/.gitignore b/.gitignore
index 833ef3b0b7..e82f90184d 100644
--- a/.gitignore
+++ b/.gitignore
@@ -34,6 +34,7 @@
 /git-clone
 /git-column
 /git-commit
+/git-commit-graph
 /git-commit-tree
 /git-config
 /git-count-objects
diff --git a/Documentation/git-commit-graph.txt 
b/Documentation/git-commit-graph.txt
new file mode 100644
index 00..e1c3078ca1
--- /dev/null
+++ b/Documentation/git-commit-graph.txt
@@ -0,0 +1,11 @@
+git-commit-graph(1)
+===
+
+NAME
+
+git-commit-graph - Write and verify Git commit graphs (.graph files)
+
+GIT
+---
+Part of the linkgit:git[1] suite
+
diff --git a/Makefile b/Makefile
index ee9d5eb11e..fc40b816dc 100644
--- a/Makefile
+++ b/Makefile
@@ -932,6 +932,7 @@ BUILTIN_OBJS += builtin/clone.o
 BUILTIN_OBJS += builtin/column.o
 BUILTIN_OBJS += builtin/commit-tree.o
 BUILTIN_OBJS += builtin/commit.o
+BUILTIN_OBJS += builtin/commit-graph.o
 BUILTIN_OBJS += builtin/config.o
 BUILTIN_OBJS += builtin/count-objects.o
 BUILTIN_OBJS += builtin/credential.o
diff --git a/builtin.h b/builtin.h
index 42378f3aa4..079855b6d4 100644
--- a/builtin.h
+++ b/builtin.h
@@ -149,6 +149,7 @@ extern int cmd_clone(int argc, const char **argv, const 
char *prefix);
 extern int cmd_clean(int argc, const char **argv, const char *prefix);
 extern int cmd_column(int argc, const char **argv, const char *prefix);
 extern int cmd_commit(int argc, const char **argv, const char *prefix);
+extern int cmd_commit_graph(int argc, const char **argv, const char *prefix);
 extern int cmd_commit_tree(int argc, const char **argv, const char *prefix);
 extern int cmd_config(int argc, const char **argv, const char *prefix);
 extern int cmd_count_objects(int argc, const char **argv, const char *prefix);
diff --git a/builtin/commit-graph.c b/builtin/commit-graph.c
new file mode 100644
index 00..a01c5d9981
--- /dev/null
+++ b/builtin/commit-graph.c
@@ -0,0 +1,37 @@
+#include "builtin.h"
+#include "config.h"
+#include "parse-options.h"
+
+static char const * const builtin_commit_graph_usage[] = {
+   N_("git commit-graph [--pack-dir ]"),
+   NULL
+};
+
+static struct opts_commit_graph {
+   const char *pack_dir;
+} opts;
+
+
+int cmd_commit_graph(int argc, const char **argv, const char *prefix)
+{
+   static struct option builtin_commit_graph_options[] = {
+   { OPTION_STRING, 'p', "pack-dir", _dir,
+   N_("dir"),
+   N_("The pack directory to store the graph") },
+   OPT_END(),
+   };
+
+   if (argc == 2 && !strcmp(argv[1], "-h"))
+   usage_with_options(builtin_commit_graph_usage,
+  builtin_commit_graph_options);
+
+   git_config(git_default_config, NULL);
+   argc = parse_options(argc, argv, prefix,
+builtin_commit_graph_options,
+builtin_commit_graph_usage,
+PARSE_OPT_STOP_AT_NON_OPTION);
+
+   usage_with_options(builtin_commit_graph_usage,
+  builtin_commit_graph_options);
+}
+
diff --git a/command-list.txt b/command-list.txt
index a1fad28fd8..835c5890be 100644
--- a/command-list.txt
+++ b/command-list.txt
@@ -34,6 +34,7 @@ git-clean   mainporcelain
 git-clone   mainporcelain   init
 git-column  purehelpers
 git-commit  mainporcelain   history
+git-commit-graphplumbingmanipulators
 git-commit-tree plumbingmanipulators
 git-config  ancillarymanipulators
 git-count-objects   ancillaryinterrogators
diff --git a/git.c b/git.c
index 9e96dd4090..d4832c1e0d 100644
--- a/git.c
+++ b/git.c
@@ -388,6 +388,7 @@ static struct cmd_struct commands[] = {
{ "clone", cmd_clone },
{ "column", cmd_column, RUN_SETUP_GENTLY },
{ "commit", cmd_commit, RUN_SETUP | NEED_WORK_TREE },
+   { "commit-graph", cmd_commit_graph, RUN_SETUP },
{ "commit-tree", cmd_commit_tree, RUN_SETUP },
{ "config", cmd_config, RUN_SETUP_GENTLY },
{ "count-objects", cmd_count_objects, RUN_SETUP },
-- 

[PATCH v3 00/14] Serialized Git Commit Graph

[Derrick Stolee]

Thanks to everyone who gave comments on v1 and v2.

Hopefully the following points have been addressed:

* Fixed inter-commit problems where certain fixes did not arrive until
  later commits.

* Converted from submode flags ("git commit-graph --write") to
  subcommands ("git commit-graph write").

* Fixed a bug where a non-commit OID would cause a segfault when using
  --stdin-commits. Added a test for an annotated tag.

* Numerous style issues, especially in the test script.

I also based my patches on the branch jt/binsearch-with-fanout to make
use of the bsearch_hash() method.

I look forward to your feedback.

Thanks,
-Stolee

-- >8 --

As promised [1], this patch contains a way to serialize the commit graph.
The current implementation defines a new file format to store the graph
structure (parent relationships) and basic commit metadata (commit date,
root tree OID) in order to prevent parsing raw commits while performing
basic graph walks. For example, we do not need to parse the full commit
when performing these walks:

* 'git log --topo-order -1000' walks all reachable commits to avoid
  incorrect topological orders, but only needs the commit message for
  the top 1000 commits.

* 'git merge-base  ' may walk many commits to find the correct
  boundary between the commits reachable from A and those reachable
  from B. No commit messages are needed.

* 'git branch -vv' checks ahead/behind status for all local branches
  compared to their upstream remote branches. This is essentially as
  hard as computing merge bases for each.

The current patch speeds up these calculations by injecting a check in
parse_commit_gently() to check if there is a graph file and using that
to provide the required metadata to the struct commit.

The file format has room to store generation numbers, which will be
provided as a patch after this framework is merged. Generation numbers
are referenced by the design document but not implemented in order to
make the current patch focus on the graph construction process. Once
that is stable, it will be easier to add generation numbers and make
graph walks aware of generation numbers one-by-one.

Here are some performance results for a copy of the Linux repository
where 'master' has 704,766 reachable commits and is behind 'origin/master'
by 19,610 commits.

| Command  | Before | After  | Rel % |
|--|||---|
| log --oneline --topo-order -1000 |  5.9s  |  0.7s  | -88%  |
| branch -vv   |  0.42s |  0.27s | -35%  |
| rev-list --all   |  6.4s  |  1.0s  | -84%  |
| rev-list --all --objects | 32.6s  | 27.6s  | -15%  |

To test this yourself, run the following on your repo:

  git config core.commitGraph true
  git show-ref -s | git commit-graph write --update-head --stdin-commits

The second command writes a commit graph file containing every commit
reachable from your refs. Now, all git commands that walk commits will
check your graph first before consulting the ODB. You can run your own
performance comparisions by toggling the 'core.commitgraph' setting.

[1] 
https://public-inbox.org/git/d154319e-bb9e-b300-7c37-27b1dcd2a...@jeffhostetler.com/
Re: What's cooking in git.git (Jan 2018, #03; Tue, 23)

[2] https://github.com/derrickstolee/git/pull/2
A GitHub pull request containing the latest version of this patch.

Derrick Stolee (14):
  commit-graph: add format document
  graph: add commit graph design document
  commit-graph: create git-commit-graph builtin
  commit-graph: implement write_commit_graph()
  commit-graph: implement 'git-commit-graph write'
  commit-graph: implement 'git-commit-graph read'
  commit-graph: update graph-head during write
  commit-graph: implement 'git-commit-graph clear'
  commit-graph: implement --delete-expired
  commit-graph: add core.commitGraph setting
  commit: integrate commit graph with commit parsing
  commit-graph: close under reachability
  commit-graph: read only from specific pack-indexes
  commit-graph: build graph from starting commits

 .gitignore  |   1 +
 Documentation/config.txt|   3 +
 Documentation/git-commit-graph.txt  | 115 
 Documentation/technical/commit-graph-format.txt |  91 +++
 Documentation/technical/commit-graph.txt| 189 ++
 Makefile|   2 +
 alloc.c |   1 +
 builtin.h   |   1 +
 builtin/commit-graph.c  | 335 ++
 cache.h |   1 +
 command-list.txt|   1 +
 commit-graph.c  | 828 
 commit-graph.h  |  60 ++
 commit.c|   3 +
 commit.h|   3 +
 

[PATCH v3 01/14] commit-graph: add format document

[Derrick Stolee]

Add document specifying the binary format for commit graphs. This
format allows for:

* New versions.
* New hash functions and hash lengths.
* Optional extensions.

Basic header information is followed by a binary table of contents
into "chunks" that include:

* An ordered list of commit object IDs.
* A 256-entry fanout into that list of OIDs.
* A list of metadata for the commits.
* A list of "large edges" to enable octopus merges.

The format automatically includes two parent positions for every
commit. This favors speed over space, since using only one position
per commit would cause an extra level of indirection for every merge
commit. (Octopus merges suffer from this indirection, but they are
very rare.)

Signed-off-by: Derrick Stolee 
---
 Documentation/technical/commit-graph-format.txt | 91 +
 1 file changed, 91 insertions(+)
 create mode 100644 Documentation/technical/commit-graph-format.txt

diff --git a/Documentation/technical/commit-graph-format.txt 
b/Documentation/technical/commit-graph-format.txt
new file mode 100644
index 00..349fa0c14c
--- /dev/null
+++ b/Documentation/technical/commit-graph-format.txt
@@ -0,0 +1,91 @@
+Git commit graph format
+===
+
+The Git commit graph stores a list of commit OIDs and some associated
+metadata, including:
+
+- The generation number of the commit. Commits with no parents have
+  generation number 1; commits with parents have generation number
+  one more than the maximum generation number of its parents. We
+  reserve zero as special, and can be used to mark a generation
+  number invalid or as "not computed".
+
+- The root tree OID.
+
+- The commit date.
+
+- The parents of the commit, stored using positional references within
+  the graph file.
+
+== graph-*.graph files have the following format:
+
+In order to allow extensions that add extra data to the graph, we organize
+the body into "chunks" and provide a binary lookup table at the beginning
+of the body. The header includes certain values, such as number of chunks,
+hash lengths and types.
+
+All 4-byte numbers are in network order.
+
+HEADER:
+
+  4-byte signature:
+  The signature is: {'C', 'G', 'P', 'H'}
+
+  1-byte version number:
+  Currently, the only valid version is 1.
+
+  1-byte Object Id Version (1 = SHA-1)
+
+  1-byte Object Id Length (H)
+
+  1-byte number (C) of "chunks"
+
+CHUNK LOOKUP:
+
+  (C + 1) * 12 bytes listing the table of contents for the chunks:
+  First 4 bytes describe chunk id. Value 0 is a terminating label.
+  Other 8 bytes provide offset in current file for chunk to start.
+  (Chunks are ordered contiguously in the file, so you can infer
+  the length using the next chunk position if necessary.)
+
+  The remaining data in the body is described one chunk at a time, and
+  these chunks may be given in any order. Chunks are required unless
+  otherwise specified.
+
+CHUNK DATA:
+
+  OID Fanout (ID: {'O', 'I', 'D', 'F'}) (256 * 4 bytes)
+  The ith entry, F[i], stores the number of OIDs with first
+  byte at most i. Thus F[255] stores the total
+  number of commits (N).
+
+  OID Lookup (ID: {'O', 'I', 'D', 'L'}) (N * H bytes)
+  The OIDs for all commits in the graph, sorted in ascending order.
+
+  Commit Data (ID: {'C', 'G', 'E', 'T' }) (N * (H + 16) bytes)
+* The first H bytes are for the OID of the root tree.
+* The next 8 bytes are for the int-ids of the first two parents
+  of the ith commit. Stores value 0x if no parent in that
+  position. If there are more than two parents, the second value
+  has its most-significant bit on and the other bits store an array
+  position into the Large Edge List chunk.
+* The next 8 bytes store the generation number of the commit and
+  the commit time in seconds since EPOCH. The generation number
+  uses the higher 30 bits of the first 4 bytes, while the commit
+  time uses the 32 bits of the second 4 bytes, along with the lowest
+  2 bits of the lowest byte, storing the 33rd and 34th bit of the
+  commit time.
+
+  Large Edge List (ID: {'E', 'D', 'G', 'E'})
+  This list of 4-byte values store the second through nth parents for
+  all octopus merges. The second parent value in the commit data stores
+  an array position within this list along with the most-significant bit
+  on. Starting at that array position, iterate through this list of int-ids
+  for the parents until reaching a value with the most-significant bit on.
+  The other bits correspond to the int-id of the last parent. This chunk
+  should always be present, but may be empty.
+
+TRAILER:
+
+   H-byte HASH-checksum of all of the above.
+
-- 
2.15.1.45.g9b7079f

Re: [PATCH] docs/interpret-trailers: fix agreement error

[Junio C Hamano]

Jonathan Tan  writes:

> On Thu,  8 Feb 2018 02:56:14 +
> "brian m. carlson"  wrote:
>
>>  Existing trailers are extracted from the input message by looking for
>> -a group of one or more lines that (i) are all trailers, or (ii) contains at
>> -least one Git-generated or user-configured trailer and consists of at
>> +a group of one or more lines that (i) are all trailers, or (ii) contain at
>> +least one Git-generated or user-configured trailer and consist of at
>>  least 25% trailers.
>>  The group must be preceded by one or more empty (or whitespace-only) lines.
>>  The group must either be at the end of the message or be the last
>
> Ah, good catch. Maybe "a group of one or more lines that (i) consists of all
> trailers, or (ii) contains ..."?

Your version reads better perhaps because it talks about "a group"
without placing undue stress on the fact that the member of the
group are usually multiple---I guess it is better over Brian's?

[PATCH] color.h: document and modernize header

[Stefan Beller]

Add documentation explaining the functions in color.h.
While at it, mark them extern and migrate the function `color_set`
into grep.c, where the only callers are.

Signed-off-by: Stefan Beller 
---

 This used to be part of  sb/blame-color, but I realized this is
 not strictly needed for that series, so I am sending it out alone.
 
 I would think I have addressed all concerns raised in 
 https://public-inbox.org/git/xmqqr2r088p3@gitster.mtv.corp.google.com/
 specifically the NEEDSWORK was dropped and this one function was just
 put into grep.c
 
 Thanks,
 Stefan

 color.c |  7 ---
 color.h | 52 ++--
 grep.c  |  5 +
 3 files changed, 43 insertions(+), 21 deletions(-)

diff --git a/color.c b/color.c
index d48dd947c9..f277e72e4c 100644
--- a/color.c
+++ b/color.c
@@ -161,11 +161,6 @@ int color_parse(const char *value, char *dst)
return color_parse_mem(value, strlen(value), dst);
 }
 
-void color_set(char *dst, const char *color_bytes)
-{
-   xsnprintf(dst, COLOR_MAXLEN, "%s", color_bytes);
-}
-
 /*
  * Write the ANSI color codes for "c" to "out"; the string should
  * already have the ANSI escape code in it. "out" should have enough
@@ -399,8 +394,6 @@ static int color_vfprintf(FILE *fp, const char *color, 
const char *fmt,
return r;
 }
 
-
-
 int color_fprintf(FILE *fp, const char *color, const char *fmt, ...)
 {
va_list args;
diff --git a/color.h b/color.h
index fd2b688dfb..8c7e6c41c2 100644
--- a/color.h
+++ b/color.h
@@ -72,26 +72,50 @@ extern int color_stdout_is_tty;
  * Use the first one if you need only color config; the second is a convenience
  * if you are just going to change to git_default_config, too.
  */
-int git_color_config(const char *var, const char *value, void *cb);
-int git_color_default_config(const char *var, const char *value, void *cb);
+extern int git_color_config(const char *var, const char *value, void *cb);
+extern int git_color_default_config(const char *var, const char *value, void 
*cb);
 
 /*
- * Set the color buffer (which must be COLOR_MAXLEN bytes)
- * to the raw color bytes; this is useful for initializing
- * default color variables.
+ * Parse a config option, which can be a boolean or one of
+ * "never", "auto", "always". Return a constant of
+ * GIT_COLOR_NEVER for "never" or negative boolean,
+ * GIT_COLOR_ALWAYS for "always" or a positive boolean,
+ * and GIT_COLOR_AUTO for "auto".
  */
-void color_set(char *dst, const char *color_bytes);
+extern int git_config_colorbool(const char *var, const char *value);
 
-int git_config_colorbool(const char *var, const char *value);
-int want_color(int var);
-int color_parse(const char *value, char *dst);
-int color_parse_mem(const char *value, int len, char *dst);
+/*
+ * Resolve the constants as returned by git_config_colorbool()
+ * (specifically "auto") to a boolean answer.
+ */
+extern int want_color(int var);
+
+/*
+ * Translate a Git color from 'value' into a string that the terminal can
+ * interpret and store it into 'dst'. The Git color values are of the form
+ * "foreground [background] [attr]" where fore- and background can be a color
+ * name ("red"), a RGB code (#0xFF) or a 256-color-mode from the terminal.
+ */
+extern int color_parse(const char *value, char *dst);
+extern int color_parse_mem(const char *value, int len, char *dst);
+
+/*
+ * Output the formatted string in the specified color (and then reset to normal
+ * color so subsequent output is uncolored). Omits the color encapsulation if
+ * `color` is NULL. The `color_fprintf_ln` prints a new line after resetting
+ * the color. The `color_print_strbuf` prints the given pre-formatted strbuf
+ * instead, up to its first NUL character.
+ */
 __attribute__((format (printf, 3, 4)))
-int color_fprintf(FILE *fp, const char *color, const char *fmt, ...);
+extern int color_fprintf(FILE *fp, const char *color, const char *fmt, ...);
 __attribute__((format (printf, 3, 4)))
-int color_fprintf_ln(FILE *fp, const char *color, const char *fmt, ...);
-void color_print_strbuf(FILE *fp, const char *color, const struct strbuf *sb);
+extern int color_fprintf_ln(FILE *fp, const char *color, const char *fmt, ...);
+extern void color_print_strbuf(FILE *fp, const char *color, const struct 
strbuf *sb);
 
-int color_is_nil(const char *color);
+/*
+ * Check if the given color is GIT_COLOR_NIL that means "no color selected".
+ * The caller needs to replace the color with the actual desired color.
+ */
+extern int color_is_nil(const char *color);
 
 #endif /* COLOR_H */
diff --git a/grep.c b/grep.c
index 3d7cd0e96f..834b8eb439 100644
--- a/grep.c
+++ b/grep.c
@@ -18,6 +18,11 @@ static void std_output(struct grep_opt *opt, const void 
*buf, size_t size)
fwrite(buf, size, 1, stdout);
 }
 
+static void color_set(char *dst, const char *color_bytes)
+{
+   xsnprintf(dst, COLOR_MAXLEN, "%s", color_bytes);
+}
+
 /*
  * Initialize the grep_defaults template with 

Re: [PATCH v2 00/17] document & test fetch pruning & add fetch.pruneTags

[Junio C Hamano]

Ævar Arnfjörð Bjarmason  writes:

> On Thu, Feb 08 2018, Ævar Arnfjörð Bjarmason jotted:
>
>> As noted in my 87h8quytmq@evledraar.gmail.com there was a bug I
>> noticed in v3 where it would segfault on some git-fetch invocations,
>> but there were not tests anywhere that caught that.
>
> ...and of course this whole submission this time around should be v4,
> not v2, but I didn't notice that I didn't adjust the subject prefix
> before sending. Junio: Hopefully you can pick it up anyway without too
> much trouble, sorry.
>
> FWIW I've deployed this to production @ work to some tens of k of
> machines (low "k" of which have users using git) without any issues.

Will replace.  I found that the resolution of conflicts necessary
with the jh/partial-clone topic is a bit different from the previous
version due to addition of an extra parameter to fetch_one(), and I
think I didn't botch it, but please double check when I push the
results out in a few hours.

Thanks.

[PATCH v2] name-hash: properly fold directory names in adjust_dirname_case()

[Ben Peart]

Correct the pointer arithmetic in adjust_dirname_case() so that it calls
find_dir_entry() with the correct string length.  Previously passing in
"dir1/foo" would pass a length of 6 instead of the correct 4.  This resulted in
find_dir_entry() never finding the entry and so the subsequent memcpy that would
fold the name to the version with the correct case never executed.

Add a test to validate the corrected behavior with name folding of directories.

Signed-off-by: Ben Peart 
---

Notes:
Base Ref: v2.16.1
Web-Diff: https://github.com/benpeart/git/commit/477da4602c
Checkout: git fetch https://github.com/benpeart/git adjust_dirname-v2 && 
git checkout 477da4602c

### Interdiff (v1..v2):

diff --git a/t/t0050-filesystem.sh b/t/t0050-filesystem.sh
index 219c96594c..0e4e51b79a 100755
--- a/t/t0050-filesystem.sh
+++ b/t/t0050-filesystem.sh
@@ -82,14 +82,18 @@ test_expect_success 'merge (case change)' '

 test_expect_success CASE_INSENSITIVE_FS 'add directory (with different 
case)' '
git reset --hard initial &&
-   mkdir -p dir1 &&
mkdir -p dir1/dir2 &&
-   touch dir1/dir2/a &&
-   touch dir1/dir2/b &&
+   echo > dir1/dir2/a &&
+   echo > dir1/dir2/b &&
git add dir1/dir2/a &&
git add dir1/DIR2/b &&
-   camel=$(git ls-files | grep dir2) &&
-   test $(echo "$camel" | wc -l) = 2
+   git ls-files >actual &&
+   cat >expected <<-\EOF &&
+   camelcase
+   dir1/dir2/a
+   dir1/dir2/b
+   EOF
+   test_cmp expected actual
 '

 test_expect_failure CASE_INSENSITIVE_FS 'add (with different case)' '

### Patches

 name-hash.c   |  6 +++---
 t/t0050-filesystem.sh | 16 +++-
 2 files changed, 18 insertions(+), 4 deletions(-)

diff --git a/name-hash.c b/name-hash.c
index 45c98db0a0..2ddbb72647 100644
--- a/name-hash.c
+++ b/name-hash.c
@@ -696,12 +696,12 @@ void adjust_dirname_case(struct index_state *istate, char 
*name)
if (*ptr == '/') {
struct dir_entry *dir;
 
-   ptr++;
-   dir = find_dir_entry(istate, name, ptr - name + 1);
+   dir = find_dir_entry(istate, name, ptr - name);
if (dir) {
memcpy((void *)startPtr, dir->name + (startPtr 
- name), ptr - startPtr);
-   startPtr = ptr;
+   startPtr = ptr + 1;
}
+   ptr++;
}
}
 }
diff --git a/t/t0050-filesystem.sh b/t/t0050-filesystem.sh
index b29d749bb7..0e4e51b79a 100755
--- a/t/t0050-filesystem.sh
+++ b/t/t0050-filesystem.sh
@@ -80,7 +80,21 @@ test_expect_success 'merge (case change)' '
git merge topic
 '
 
-
+test_expect_success CASE_INSENSITIVE_FS 'add directory (with different case)' '
+   git reset --hard initial &&
+   mkdir -p dir1/dir2 &&
+   echo > dir1/dir2/a &&
+   echo > dir1/dir2/b &&
+   git add dir1/dir2/a &&
+   git add dir1/DIR2/b &&
+   git ls-files >actual &&
+   cat >expected <<-\EOF &&
+   camelcase
+   dir1/dir2/a
+   dir1/dir2/b
+   EOF
+   test_cmp expected actual
+'
 
 test_expect_failure CASE_INSENSITIVE_FS 'add (with different case)' '
git reset --hard initial &&

base-commit: 8279ed033f703d4115bee620dccd32a9ec94d9aa
-- 
2.15.0.windows.1

Netgear X8 R8500

[Vita Beatae]

Hello, I’m very beginner and literally a grandma. Would like Tomatoes for my 
Netgear X8 R8500 router. Very misleading netgear product saying OpenVPN and had 
no idea about client or I would have to do all this stuff and the netgear 
security vulnerabilities are horrid. Was told to shut it down. I am shut dow 
working from my iPhone.  Been trying find a way to safely set up vpn for home 
with WiFi and cctv security cams plus other tv and device and need speed.  
Lower cost routers don’t offer speed and x8 looked like a good choice reviews 
were great said vpn very misleading. Netgear support non existent.  Ddwrt is 
looking scary to do myself Flashrouters called me very kindly said he would 
help me for basic support pkg that’s 100$ and I have to pay extra for guest 
vpn. Have lost many devices due to being hacked all my family pics gone. Ddwrt 
flash routers will help understandably for a fee. Every one has the right to 
explore and keep the lights on. I need to make sure if I spend any more money I 
will actually do this right and not see tomatoes available tomorrow for my 
model.  Can anyone help me make the right choices with this? Please.  I’m 
getting real broke very quick.  (I did get a RATTrap waiting for shipment to 
arrive. Have a drunk neighbor outside early am trying to guess passwords with a 
device. They also got under my house into the att line to my phone jack tech 
installed. I will pay respectfully but I need to really do this quick and 
obviously the best right way. Thank you.

Blessings,
Vita  

Sent from my iPhone

Re: [PATCH 2/2] always check for NULL return from packet_read_line()

[Jeff King]

On Thu, Feb 08, 2018 at 01:47:50PM -0500, Jon Simons wrote:

> The packet_read_line() function will die if it sees any
> protocol or socket errors. But it will return NULL for a
> flush packet; some callers which are not expecting this may
> dereference NULL if they get an unexpected flush. This would
> involve the other side breaking protocol, but we should
> flag the error rather than segfault.

As one might guess from the dual authorship on this series, Jon and I
discussed these off list. So this one is

  Reviewed-by: Jeff King 

And the other one, too, but I'm not sure that carries any weight. :)

-Peff

[PATCH 1/2] correct error messages for NULL packet_read_line()

[Jon Simons]

From: Jeff King 

The packet_read_line() function dies if it gets an
unexpected EOF. It only returns NULL if we get a flush
packet (or technically, a zero-length "0004" packet, but
nobody is supposed to send those, and they are
indistinguishable from a flush in this interface).

Let's correct error messages which claim an unexpected EOF;
it's really an unexpected flush packet.

While we're here, let's also check "!line" instead of
"!len" in the second case. The two events should always
coincide, but checking "!line" makes it more obvious that we
are not about to dereference NULL.

Signed-off-by: Jeff King 
---
 builtin/archive.c | 2 +-
 fetch-pack.c  | 4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/builtin/archive.c b/builtin/archive.c
index f863465..73971d0 100644
--- a/builtin/archive.c
+++ b/builtin/archive.c
@@ -55,7 +55,7 @@ static int run_remote_archiver(int argc, const char **argv,
 
buf = packet_read_line(fd[0], NULL);
if (!buf)
-   die(_("git archive: expected ACK/NAK, got EOF"));
+   die(_("git archive: expected ACK/NAK, got a flush packet"));
if (strcmp(buf, "ACK")) {
if (starts_with(buf, "NACK "))
die(_("git archive: NACK %s"), buf + 5);
diff --git a/fetch-pack.c b/fetch-pack.c
index a376b4e..1b7cd6b 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -262,8 +262,8 @@ static enum ack_type get_ack(int fd, struct object_id 
*result_oid)
char *line = packet_read_line(fd, );
const char *arg;
 
-   if (!len)
-   die(_("git fetch-pack: expected ACK/NAK, got EOF"));
+   if (!line)
+   die(_("git fetch-pack: expected ACK/NAK, got a flush packet"));
if (!strcmp(line, "NAK"))
return NAK;
if (skip_prefix(line, "ACK ", )) {
-- 
2.1.4

[PATCH 2/2] always check for NULL return from packet_read_line()

[Jon Simons]

The packet_read_line() function will die if it sees any
protocol or socket errors. But it will return NULL for a
flush packet; some callers which are not expecting this may
dereference NULL if they get an unexpected flush. This would
involve the other side breaking protocol, but we should
flag the error rather than segfault.

Signed-off-by: Jon Simons 
---
 remote-curl.c | 2 ++
 send-pack.c   | 2 ++
 2 files changed, 4 insertions(+)

diff --git a/remote-curl.c b/remote-curl.c
index 0053b09..9903077 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -339,6 +339,8 @@ static struct discovery *discover_refs(const char *service, 
int for_push)
 * pkt-line matches our request.
 */
line = packet_read_line_buf(>buf, >len, NULL);
+   if (!line)
+   die("invalid server response; expected service, got 
flush packet");
 
strbuf_reset();
strbuf_addf(, "# service=%s", service);
diff --git a/send-pack.c b/send-pack.c
index 11d6f3d..d37b265 100644
--- a/send-pack.c
+++ b/send-pack.c
@@ -147,6 +147,8 @@ static int pack_objects(int fd, struct ref *refs, struct 
oid_array *extra, struc
 static int receive_unpack_status(int in)
 {
const char *line = packet_read_line(in, NULL);
+   if (!line)
+   return error(_("unexpected flush packet while reading remote 
unpack status"));
if (!skip_prefix(line, "unpack ", ))
return error(_("unable to parse remote unpack status: %s"), 
line);
if (strcmp(line, "ok"))
-- 
2.1.4

[PATCH 0/2] Fix NULL checks for some packet_read_line call sites

[Jon Simons]

Included here are a couple of fixes and cleanups for
handling NULL return values from 'packet_read_line'.

Jeff King (1):
  correct error messages for NULL packet_read_line()

Jon Simons (1):
  always check for NULL return from packet_read_line()

 builtin/archive.c | 2 +-
 fetch-pack.c  | 4 ++--
 remote-curl.c | 2 ++
 send-pack.c   | 2 ++
 4 files changed, 7 insertions(+), 3 deletions(-)

-- 
2.1.4

Hello Dear Friend,

[ATIKOLA AWA]

Hello Dear Friend,
Greetings and how are you doing?
I want to know if you are kind to be my
partner in claiming the fund of  $13.6
MillionUSD left by a late client. If you're
interested please Revert for more details.
You can visit the web for more details.
http://newswww.bbc.net.uk/2/hi/uk_news/england/oxfordshire/4537663.stm
Dr.Atikola Awa,
atikola...@dr.com.

Re: [PATCH v1] name-hash: properly fold directory names in adjust_dirname_case()

[Junio C Hamano]

Torsten Bögershausen  writes:

> My personal favorite would be to spell out what we expect and run a diff.
> When it fails, we can see what fails, and the function would look
> like this:

I'd rather not to have the "sort" there; output from ls-files is
meant to be stable; passing it through sort would miss breakages.  I
agree that comparison between "expect" and "actual" is a good idea
nevertheless.

Speaking of styles, I'd prefer to reserve use of "touch" to cases
where resulting timestamp matters, and not "make sure it exists".

Thanks.

> test_expect_success CASE_INSENSITIVE_FS 'add directory (with different case)' 
> '
>   git reset --hard initial &&
>   mkdir -p dir1 &&
>   mkdir -p dir1/dir2 &&
>   touch dir1/dir2/a &&
>   touch dir1/dir2/b &&
>   git add dir1/dir2/a &&
>   git add dir1/DIR2/b &&
>   git ls-files | grep dir2 | sort >actual &&
>   cat >expected <<-\EOF &&
>   dir1/dir2/a
>   dir1/dir2/b
>   EOF
>   test_cmp expected actual
> '

Re: [PATCH] send-email: error out when relogin delay is missing

[Stefan Beller]

On Thu, Feb 8, 2018 at 12:08 AM, Eric Sunshine  wrote:
> On Wed, Feb 7, 2018 at 6:43 PM, Stefan Beller  wrote:
>> [...]
>> Error out for now instead of potentially confusing the user.
>> As 5453b83bdf (send-email: --batch-size to work around some SMTP
>> server limit, 2017-05-21) lays out, we rather want to not have this
>> interface anyway and would rather want to react on the server throttling
>> dynamically.
>>
>> Signed-off-by: Stefan Beller 
>> ---
>> diff --git a/git-send-email.perl b/git-send-email.perl
>> @@ -379,6 +379,9 @@ unless ($rc) {
>> +die __("When a batch size is given, the relogin delay must be set\n")
>> +   if defined $relogin_delay and not defined $batch_size;
>
> This only makes sense is 'batch-size' is specified but not 'relogin'.
> If the other way around, then the error is confusing. How about this
> instead?
>
> "--batch-size and --relogin must be specified together"
>
> ...or something.

I like this for its expressiveness as it would have helped me a lot.
I dislike this because it is incorrect when you use the config options
instead of command line arguments.

Stefan

Re: [PATCH v2 00/17] document & test fetch pruning & add fetch.pruneTags

[Ævar Arnfjörð Bjarmason]

On Thu, Feb 08 2018, Ævar Arnfjörð Bjarmason jotted:

> As noted in my 87h8quytmq@evledraar.gmail.com there was a bug I
> noticed in v3 where it would segfault on some git-fetch invocations,
> but there were not tests anywhere that caught that.

...and of course this whole submission this time around should be v4,
not v2, but I didn't notice that I didn't adjust the subject prefix
before sending. Junio: Hopefully you can pick it up anyway without too
much trouble, sorry.

FWIW I've deployed this to production @ work to some tens of k of
machines (low "k" of which have users using git) without any issues.

Re: [PATCH v1] name-hash: properly fold directory names in adjust_dirname_case()

[Junio C Hamano]

Ben Peart  writes:

> Correct the pointer arithmetic in adjust_dirname_case() so that it calls
> find_dir_entry() with the correct string length.  Previously passing in
> "dir1/foo" would pass a length of 6 instead of the correct 4.  This resulted 
> in
> find_dir_entry() never finding the entry and so the subsequent memcpy that 
> would
> fold the name to the version with the correct case never executed.
>
> Add a test to validate the corrected behavior with name folding of 
> directories.
>
> Signed-off-by: Ben Peart 
> ---

Thanks.

It appears that this codepath has been miscounting ever since it was
introduced in 41284eb0 ("name-hash: don't reuse cache_entry in
dir_entry", 2015-10-21).

> diff --git a/t/t0050-filesystem.sh b/t/t0050-filesystem.sh
> index b29d749bb7..219c96594c 100755
> --- a/t/t0050-filesystem.sh
> +++ b/t/t0050-filesystem.sh
> @@ -80,7 +80,17 @@ test_expect_success 'merge (case change)' '
> ...
> +test_expect_success CASE_INSENSITIVE_FS 'add directory (with different 
> case)' '
> + git reset --hard initial &&
> + mkdir -p dir1 &&
> + mkdir -p dir1/dir2 &&

A single "mkdir -p dir1/dir2" should be sufficient, thanks to "-p" ;-)

> + touch dir1/dir2/a &&
> + touch dir1/dir2/b &&

Hmph.

Re: [PATCH] docs/interpret-trailers: fix agreement error

[Jonathan Tan]

On Thu,  8 Feb 2018 02:56:14 +
"brian m. carlson"  wrote:

>  Existing trailers are extracted from the input message by looking for
> -a group of one or more lines that (i) are all trailers, or (ii) contains at
> -least one Git-generated or user-configured trailer and consists of at
> +a group of one or more lines that (i) are all trailers, or (ii) contain at
> +least one Git-generated or user-configured trailer and consist of at
>  least 25% trailers.
>  The group must be preceded by one or more empty (or whitespace-only) lines.
>  The group must either be at the end of the message or be the last

Ah, good catch. Maybe "a group of one or more lines that (i) consists of all
trailers, or (ii) contains ..."?

I'm also OK with the patch as-is.

Re: [PATCH v1] name-hash: properly fold directory names in adjust_dirname_case()

[Ben Peart]

On 2/8/2018 12:21 PM, Torsten Bögershausen wrote:

On Wed, Feb 07, 2018 at 07:41:56PM -0500, Ben Peart wrote:
[]


diff --git a/t/t0050-filesystem.sh b/t/t0050-filesystem.sh
index b29d749bb7..219c96594c 100755
--- a/t/t0050-filesystem.sh
+++ b/t/t0050-filesystem.sh
@@ -80,7 +80,17 @@ test_expect_success 'merge (case change)' '
git merge topic
  '
  
-

+test_expect_success CASE_INSENSITIVE_FS 'add directory (with different case)' '
+   git reset --hard initial &&
+   mkdir -p dir1 &&
+   mkdir -p dir1/dir2 &&
+   touch dir1/dir2/a &&
+   touch dir1/dir2/b &&
+   git add dir1/dir2/a &&
+   git add dir1/DIR2/b &&
+   camel=$(git ls-files | grep dir2) &&
+   test $(echo "$camel" | wc -l) = 2
+'
  


There is nothing wrong with with "wc -l", but:
a more new-style would probably use test_line_count() here.

My personal favorite would be to spell out what we expect and run a diff.
When it fails, we can see what fails, and the function would look
like this:



I agree with you completely that this is a better format and is easier 
to read.  All the new tests I've been writing follow this same pattern.


In this particular test file, I opted (for better and for worse) to 
stick with the style of all the other tests rather than 1) have this one 
test be very different than all the others or 2) rewriting all the 
existing tests in the new style.




test_expect_success CASE_INSENSITIVE_FS 'add directory (with different case)' '
git reset --hard initial &&
mkdir -p dir1 &&
mkdir -p dir1/dir2 &&
touch dir1/dir2/a &&
touch dir1/dir2/b &&
git add dir1/dir2/a &&
git add dir1/DIR2/b &&
git ls-files | grep dir2 | sort >actual &&
cat >expected <<-\EOF &&
dir1/dir2/a
dir1/dir2/b
EOF
test_cmp expected actual
'

Re: [PATCH v1] name-hash: properly fold directory names in adjust_dirname_case()

[Torsten Bögershausen]

On Wed, Feb 07, 2018 at 07:41:56PM -0500, Ben Peart wrote:
[]

> diff --git a/t/t0050-filesystem.sh b/t/t0050-filesystem.sh
> index b29d749bb7..219c96594c 100755
> --- a/t/t0050-filesystem.sh
> +++ b/t/t0050-filesystem.sh
> @@ -80,7 +80,17 @@ test_expect_success 'merge (case change)' '
>   git merge topic
>  '
>  
> -
> +test_expect_success CASE_INSENSITIVE_FS 'add directory (with different 
> case)' '
> + git reset --hard initial &&
> + mkdir -p dir1 &&
> + mkdir -p dir1/dir2 &&
> + touch dir1/dir2/a &&
> + touch dir1/dir2/b &&
> + git add dir1/dir2/a &&
> + git add dir1/DIR2/b &&
> + camel=$(git ls-files | grep dir2) &&
> + test $(echo "$camel" | wc -l) = 2
> +'
>  

There is nothing wrong with with "wc -l", but:
a more new-style would probably use test_line_count() here.

My personal favorite would be to spell out what we expect and run a diff.
When it fails, we can see what fails, and the function would look
like this:


test_expect_success CASE_INSENSITIVE_FS 'add directory (with different case)' '
git reset --hard initial &&
mkdir -p dir1 &&
mkdir -p dir1/dir2 &&
touch dir1/dir2/a &&
touch dir1/dir2/b &&
git add dir1/dir2/a &&
git add dir1/DIR2/b &&
git ls-files | grep dir2 | sort >actual &&
cat >expected <<-\EOF &&
dir1/dir2/a
dir1/dir2/b
EOF
test_cmp expected actual
'

Re: Fetch-hooks

[Leo Gaspard]

On 02/08/2018 04:30 PM, Joey Hess wrote:
> Leo Gaspard wrote:
>> That said, I just came upon [1] (esp. the description [2] and the patch
>> [3]), and wondered: it looks like the patch was abandoned midway in
>> favor of a hook refactoring. Would you happen to know whether the hook
>> refactoring eventually took place, and/or whether this patch was
>> resubmitted later, and/or whether it would still be possible to merge
>> this now? (not having any experience with git's internals yet, I don't
>> really know whether these are stupid questions or not)
>>
>> PS: Cc'ing Joey, as you most likely know best what eventually happened,
>> if you can remember it?
> 
> I don't remember it well, but reviewing the thread, I think it foundered
> on this comment by Junio:
> 
>> That use case sounds like that "git fetch" is called as a first class UI,
>> which is covered by "git myfetch" (you can call it "git annex fetch")
>> wrapper approach, the canonical example of a hook that we explicitly do
> ^
>> not want to add.
>   ^^^
> 
> While I still think a fetch hook would be a good idea for reasons of
> composability, I then just went off and implemented such a wrapper for
> my own particular use case, and the wrapper program then grew to cover
> use cases that a hook would not have been able to cover, so ...

Hmm, OK, so I guess I'll try to update the patch when I get some time to
delve into git's internals, as my use case (forbidding some fetches)
couldn't afaik be covered by a wrapper hook.

Thanks for the feedback!
Leo

Re: [PATCH v2 0/9] 'test_i18ngrep'-related fixes and improvements

[Jeff King]

On Thu, Feb 08, 2018 at 04:56:47PM +0100, SZEDER Gábor wrote:

> This is the second version of 'sg/test-i18ngrep'.
> 
> To recap, this patch series fixes a couple of bogus 'test_i18ngrep'
> invocations (patches 1-4), tries to prevent similar bugs in the future
> (patch 8), teaches 'test_i18ngrep' to be more informative on failure
> (patch 9), and a bit of cleanups in between (patches 5-7).
> 
> Changes since the previous version [1]:
> [...]

This round looks good to me. I left a very minor comment on patch 8, but
otherwise didn't find anything wrong. Thanks.

-Peff

Re: [PATCH v2 8/9] t: validate 'test_i18ngrep's parameters

[Jeff King]

On Thu, Feb 08, 2018 at 04:56:55PM +0100, SZEDER Gábor wrote:

> Prevent similar mistakes in the future by validating 'test_i18ngrep's
> parameters requiring that
> 
>   - The last parameter names an existing file to be read, effectively
> forbiding piping into 'test_i18ngrep'.

s/forbiding/forbidding/

> Note that this change will also forbid cases where 'test_i18ngrep'
> would legitimately read its standard input, e.g. when its standard
> input is redirected from a file, or when a git command's standard
> output is first written to an intermediate file, which is then
> preprocessed by a non-git command before the results are piped
> into 'test_i18ngrep'.  See two of the previous patches for the
> only such cases we had in our test suite.  However, reliably
> preventing the piping antipattern is arguably more important than
> supporting these cases, which can be easily worked around by
> opening the file directly or using an intermediate file anyway.
> 
>   - There are at least two parameters, not including the optional '!'
> to negate the pattern.  This ought to catch corner cases when
> 'test_i18ngrep' looks for the name of an existing file on its
> standard input; the above check would miss this case becase the
> filename as pattern would be the last parameter.
> 
> Note that this is not quite perfect, as it doesn't account for any
> 'grep --options' given as parameters.  However, doing so would be
> far too complicated, considering that patterns can start with
> dashes as well, and in the majority of the cases we don't use any
> such options anyway.

And most importantly, we never err on the side of complaining
unnecessarily. So our safety might not kick in, but as long as it kicks
in most of the time, we're fine.

I like this approach much better than the previous round.

> diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
> index 92ed029371..a1676e0386 100644
> --- a/t/test-lib-functions.sh
> +++ b/t/test-lib-functions.sh
> @@ -719,6 +719,18 @@ test_i18ncmp () {
>  # under GETTEXT_POISON this pretends that the command produced expected
>  # results.
>  test_i18ngrep () {
> + eval "last_arg=\"\${$#}\""

These embedded double-quotes are unnecessary, I think, because it's a
variable assignment: E.g.:

  set -- one two 'foo bar'
  eval "last_arg=\${$#}"
  echo $last_arg

should produce "foo bar".

Usually not a big deal, but because of the extra quoting it may make the
whole thing a bit more readable to drop them.

-Peff

Re: "git branch" issue in 2.16.1

[Lars Schneider]

> On 08 Feb 2018, at 17:19, Kevin Daudt  wrote:
> 
> On Thu, Feb 08, 2018 at 12:27:07PM +0100, Lars Schneider wrote:
>> 
>>> On 08 Feb 2018, at 12:13, Lars Schneider  wrote:
>>> 
>>> 
 On 08 Feb 2018, at 09:50, Jeff King  wrote:
 
 On Wed, Feb 07, 2018 at 11:20:08PM +0100, Lars Schneider wrote:
 
>> 1. You have $LESS in your environment (without "F") on one platform
>>  but not the other.
> 
> I think that's it. On my system LESS is defined to "-R".
> 
> This opens the pager:
> 
>   $ echo "TEST" | less
> 
> This does not open the pager:
> 
>   $ echo "TEST" | less -FRX
> 
> That means "F" works on macOS but Git doesn't set it because LESS is
> already in my environment.
> 
> Question is, why is LESS set that way on my system? I can't find
> it in .bashrc .bash_profile .zshrc and friends.
 
 There's also /etc/bash.bashrc, /etc/profile, etc. I don't know what's
 normal in the mac world. You can try running:
 
 bash -ix 2>&1 >>> 
 to see what your startup code is doing. I don't know of a good way to
 correlate that with the source files, though. Or even to ask bash which
 startup files it's looking in.
>>> 
>>> Unfortunately, this command doesn't work for me.
>>> 
>>> I ask around and most of my coworkers have LESS="-R".
>>> Only the coworker that doesn't really use his Mac and has
>>> no customizations does not have $LESS defined.
>>> 
>>> Therefore, I think it is likely some third party component
>>> that sets $LESS.
>>> 
>>> @Jason:
>>> Do you have homebrew, iTerm2, and/or oh-my-zsh installed?
>> 
>> Ha. I found it it! It is indeed oh-my-zsh:
>> https://github.com/robbyrussell/oh-my-zsh/blob/master/lib/misc.zsh#L23
>> 
>> Let's see if oh-my-zsh is willing to change that...
>> 
>> - Lars
> 
> I've just added unset LESS in my .zshrc, but for most users it would be
> better if they don't set it at all.

I proposed this instead:
https://github.com/robbyrussell/oh-my-zsh/pull/6611

Maybe you can chime in there?

Thanks,
Lars

Re: git gc --auto yelling at users where a repo legitimately has >6700 loose objects

[Ævar Arnfjörð Bjarmason]

On Thu, Jan 11 2018, Ævar Arnfjörð Bjarmason jotted:

> I recently disabled gc.auto=0 and my nightly aggressive repack script on
> our big monorepo across our infra, relying instead on git gc --auto in
> the background to just do its thing.
>
> I didn't want users to wait for git-gc, and I'd written this nightly
> cronjob before git-gc learned to detach to the background.
>
> But now I have git-gc on some servers yelling at users on every pull
> command:
>
> warning: There are too many unreachable loose objects; run 'git prune' to 
> remove them.
>
> The reason is that I have all the values at git's default settings, and
> there legitimately are >~6700 loose objects that were created in the
> last 2 weeks.
>
> For those rusty on git-gc's defaults, this is what it looks like in this
> scenario:
>
>  1. User runs "git pull"
>  2. git gc --auto is called, there are >6700 loose objects
>  3. it forks into the background, tries to prune and repack, objects
> older than gc.pruneExpire (2.weeks.ago) are pruned.
>  4. At the end of all this, we check *again* if we have >6700 objects,
> if we do we print "run 'git prune'" to .git/gc.log, and will just
> emit that error for the next day before trying again, at which point
> we unlink the gc.log and retry, see gc.logExpiry.
>
> Right now I've just worked around this by setting gc.pruneExpire to a
> lower value (4.days.ago). But there's a larger issue to be addressed
> here, and I'm not sure how.
>
> When the warning was added in [1] it didn't know to detach to the
> background yet, that came in [2], shortly after came gc.log in [3].
>
> We could add another gc.auto-like limit, which could be set at some
> higher value than gc.auto. "Hey if I have more than 6700 loose objects,
> prune the <2wks old ones, but if at the end there's still >6700 I don't
> want to hear about it unless there's >6700*N".
>
> I thought I'd just add that, but the details of how to pass that message
> around get nasty. With that solution we *also* don't want git gc to
> start churning in the background once we reach >6700 objects, so we need
> something like gc.logExpiry which defers the gc until the next day. We
> might need to create .git/gc-waitabit.marker, ew.
>
> More generally, these hard limits seem contrary to what the user cares
> about. E.g. I suspect that most of these loose objects come from
> branches since deleted in upstream, whose objects could have a different
> retention policy.
>
> Or we could say "I want 2 weeks of objects, but if that runs against the
> 6700 limit just keep the latest 6700/2".
>
> 1. a087cc9819 ("git-gc --auto: protect ourselves from accumulated
>cruft", 2007-09-17)
> 2. 9f673f9477 ("gc: config option for running --auto in background",
>2014-02-08)
> 3. 329e6e8794 ("gc: save log from daemonized gc --auto and print it next
>time", 2015-09-19)

My just-sent "How to produce a loose ref+size explosion via pruning +
git-gc", <87fu6bmr0j@evledraar.gmail.com>
(https://public-inbox.org/git/87fu6bmr0j@evledraar.gmail.com/),
shows an easy way to reproduce this.

After the steps outlined there git-gc --auto will end up in a state
where it'll start telling the user off for having too many loose
objects.

[PATCH v2 06/17] fetch tests: re-arrange arguments for future readability

[Ævar Arnfjörð Bjarmason]

Re-arrange the arguments to the test_configured_prune() function used
in this test to pass the arguments to --fetch last. A subsequent
change will test for more elaborate fetch arguments, including long
refspecs. It'll be more readable to be able to wrap those on a new
line of their own.

Signed-off-by: Ævar Arnfjörð Bjarmason 
---
 t/t5510-fetch.sh | 82 ++--
 1 file changed, 44 insertions(+), 38 deletions(-)

diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh
index 11da97f9b7..ab8b25344d 100755
--- a/t/t5510-fetch.sh
+++ b/t/t5510-fetch.sh
@@ -551,10 +551,10 @@ set_config_tristate () {
 test_configured_prune () {
fetch_prune=$1
remote_origin_prune=$2
-   cmdline=$3
-   expected_branch=$4
+   expected_branch=$3
+   cmdline=$4
 
-   test_expect_success "prune fetch.prune=$1 remote.origin.prune=$2${3:+ 
$3}; branch:$4" '
+   test_expect_success "prune fetch.prune=$1 remote.origin.prune=$2${4:+ 
$4}; branch:$3" '
# make sure a newbranch is there in . and also in one
git branch -f newbranch &&
(
@@ -587,41 +587,47 @@ test_configured_prune () {
'
 }
 
-test_configured_prune unset unset ""   kept
-test_configured_prune unset unset "--no-prune" kept
-test_configured_prune unset unset "--prune"pruned
-
-test_configured_prune false unset ""   kept
-test_configured_prune false unset "--no-prune" kept
-test_configured_prune false unset "--prune"pruned
-
-test_configured_prune true  unset ""   pruned
-test_configured_prune true  unset "--prune"pruned
-test_configured_prune true  unset "--no-prune" kept
-
-test_configured_prune unset false ""   kept
-test_configured_prune unset false "--no-prune" kept
-test_configured_prune unset false "--prune"pruned
-
-test_configured_prune false false ""   kept
-test_configured_prune false false "--no-prune" kept
-test_configured_prune false false "--prune"pruned
-
-test_configured_prune true  false ""   kept
-test_configured_prune true  false "--prune"pruned
-test_configured_prune true  false "--no-prune" kept
-
-test_configured_prune unset true  ""   pruned
-test_configured_prune unset true  "--no-prune" kept
-test_configured_prune unset true  "--prune"pruned
-
-test_configured_prune false true  ""   pruned
-test_configured_prune false true  "--no-prune" kept
-test_configured_prune false true  "--prune"pruned
-
-test_configured_prune true  true  ""   pruned
-test_configured_prune true  true  "--prune"pruned
-test_configured_prune true  true  "--no-prune" kept
+# $1 config: fetch.prune
+# $2 config: remote..prune
+# $3 expect: branch to be pruned?
+# $4 git-fetch $cmdline:
+#
+# $1$2$3 $4
+test_configured_prune unset unset kept   ""
+test_configured_prune unset unset kept   "--no-prune"
+test_configured_prune unset unset pruned "--prune"
+
+test_configured_prune false unset kept   ""
+test_configured_prune false unset kept   "--no-prune"
+test_configured_prune false unset pruned "--prune"
+
+test_configured_prune true  unset pruned ""
+test_configured_prune true  unset pruned "--prune"
+test_configured_prune true  unset kept   "--no-prune"
+
+test_configured_prune unset false kept   ""
+test_configured_prune unset false kept   "--no-prune"
+test_configured_prune unset false pruned "--prune"
+
+test_configured_prune false false kept   ""
+test_configured_prune false false kept   "--no-prune"
+test_configured_prune false false pruned "--prune"
+
+test_configured_prune true  false kept   ""
+test_configured_prune true  false pruned "--prune"
+test_configured_prune true  false kept   "--no-prune"
+
+test_configured_prune unset true  pruned ""
+test_configured_prune unset true  kept   "--no-prune"
+test_configured_prune unset true  pruned "--prune"
+
+test_configured_prune false true  pruned ""
+test_configured_prune false true  kept   "--no-prune"
+test_configured_prune false true  pruned "--prune"
+
+test_configured_prune true  true  pruned ""
+test_configured_prune true  true  pruned "--prune"
+test_configured_prune true  true  kept   "--no-prune"
 
 test_expect_success 'all boundary commits are excluded' '
test_commit base &&
-- 
2.15.1.424.g9478a66081

[PATCH v2 04/17] remote: add a macro for "refs/tags/*:refs/tags/*"

[Ævar Arnfjörð Bjarmason]

Add a macro with the refspec string "refs/tags/*:refs/tags/*". There's
been a pre-defined struct version of this since e0aaa29ff3 ("Have a
constant extern refspec for "--tags"", 2008-04-17), but nothing that
could be passed to e.g. add_fetch_refspec().

This will be used in subsequent commits to avoid hardcoding this
string in multiple places.

Signed-off-by: Ævar Arnfjörð Bjarmason 
---
 remote.c | 1 +
 remote.h | 2 ++
 2 files changed, 3 insertions(+)

diff --git a/remote.c b/remote.c
index 4e93753e19..356c123e3e 100644
--- a/remote.c
+++ b/remote.c
@@ -22,6 +22,7 @@ static struct refspec s_tag_refspec = {
"refs/tags/*"
 };
 
+/* See TAG_REFSPEC for the string version */
 const struct refspec *tag_refspec = _tag_refspec;
 
 struct counted_string {
diff --git a/remote.h b/remote.h
index 1f6611be21..80fea6dd11 100644
--- a/remote.h
+++ b/remote.h
@@ -297,4 +297,6 @@ extern int parseopt_push_cas_option(const struct option *, 
const char *arg, int
 extern int is_empty_cas(const struct push_cas_option *);
 void apply_push_cas(struct push_cas_option *, struct remote *, struct ref *);
 
+#define TAG_REFSPEC "refs/tags/*:refs/tags/*"
+
 #endif
-- 
2.15.1.424.g9478a66081

[PATCH v2 08/17] fetch tests: test --prune and refspec interaction

[Ævar Arnfjörð Bjarmason]

Add a test for the interaction between explicitly provided refspecs
and fetch.prune.

There's no point in adding this boilerplate to every combination of
unset/false/true, it's instructive and sufficient to show that no
matter if the variable is unset, false or true the refspec on the
command-line overrides any configuration variable.

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

diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh
index fad65bd885..dacdb8759c 100755
--- a/t/t5510-fetch.sh
+++ b/t/t5510-fetch.sh
@@ -609,6 +609,10 @@ test_configured_prune () {
 test_configured_prune unset unset kept   kept   ""
 test_configured_prune unset unset kept   kept   "--no-prune"
 test_configured_prune unset unset pruned kept   "--prune"
+test_configured_prune unset unset kept   pruned \
+   "--prune origin refs/tags/*:refs/tags/*"
+test_configured_prune unset unset pruned pruned \
+   "--prune origin refs/tags/*:refs/tags/* 
+refs/heads/*:refs/remotes/origin/*"
 
 test_configured_prune false unset kept   kept   ""
 test_configured_prune false unset kept   kept   "--no-prune"
@@ -625,6 +629,10 @@ test_configured_prune unset false pruned kept   "--prune"
 test_configured_prune false false kept   kept   ""
 test_configured_prune false false kept   kept   "--no-prune"
 test_configured_prune false false pruned kept   "--prune"
+test_configured_prune false false kept   pruned \
+   "--prune origin refs/tags/*:refs/tags/*"
+test_configured_prune false false pruned pruned \
+   "--prune origin refs/tags/*:refs/tags/* 
+refs/heads/*:refs/remotes/origin/*"
 
 test_configured_prune true  false kept   kept   ""
 test_configured_prune true  false pruned kept   "--prune"
@@ -641,6 +649,10 @@ test_configured_prune false true  pruned kept   "--prune"
 test_configured_prune true  true  pruned kept   ""
 test_configured_prune true  true  pruned kept   "--prune"
 test_configured_prune true  true  kept   kept   "--no-prune"
+test_configured_prune true  true  kept   pruned \
+   "--prune origin refs/tags/*:refs/tags/*"
+test_configured_prune true  true  pruned pruned \
+   "--prune origin refs/tags/*:refs/tags/* 
+refs/heads/*:refs/remotes/origin/*"
 
 test_expect_success 'all boundary commits are excluded' '
test_commit base &&
-- 
2.15.1.424.g9478a66081

[PATCH v2 09/17] fetch tests: double quote a variable for interpolation

[Ævar Arnfjörð Bjarmason]

If the $cmdline variable contains arguments with spaces they won't be
interpolated correctly, since the body of the test is single quoted,
and because test-lib.sh does its own eval().

This will be used in a subsequent commit to pass arguments that need
to be quoted to git-fetch, i.e. a file:// path to fetch, which will
have a space in it.

Signed-off-by: Ævar Arnfjörð Bjarmason 
---
 t/t5510-fetch.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh
index dacdb8759c..88d38e0819 100755
--- a/t/t5510-fetch.sh
+++ b/t/t5510-fetch.sh
@@ -578,7 +578,7 @@ test_configured_prune () {
set_config_tristate fetch.prune $fetch_prune &&
set_config_tristate remote.origin.prune 
$remote_origin_prune &&
 
-   git fetch $cmdline &&
+   git fetch '"$cmdline"' &&
case "$expected_branch" in
pruned)
test_must_fail git rev-parse --verify 
refs/remotes/origin/newbranch
-- 
2.15.1.424.g9478a66081

[PATCH v2 10/17] fetch tests: expand case/esac for later change

[Ævar Arnfjörð Bjarmason]

Expand a compact case/esac statement for a later change that'll add
more logic to the body of the "*" case. This is a whitespace-only
change.

Signed-off-by: Ævar Arnfjörð Bjarmason 
---
 t/t5510-fetch.sh | 8 ++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh
index 88d38e0819..dfc749f576 100755
--- a/t/t5510-fetch.sh
+++ b/t/t5510-fetch.sh
@@ -543,8 +543,12 @@ test_expect_success "should be able to fetch with 
duplicate refspecs" '
 set_config_tristate () {
# var=$1 val=$2
case "$2" in
-   unset)  test_unconfig "$1" ;;
-   *)  git config "$1" "$2" ;;
+   unset)
+   test_unconfig "$1"
+   ;;
+   *)
+   git config "$1" "$2"
+   ;;
esac
 }
 
-- 
2.15.1.424.g9478a66081

[PATCH v2 15/17] fetch tests: add scaffolding for the new fetch.pruneTags

[Ævar Arnfjörð Bjarmason]

The fetch.pruneTags configuration doesn't exist yet, but will be added
in a subsequent commit. Since testing for it requires adding new
parameters to the test_configured_prune function it's easier to review
this patch first to assert that no functional changes are introduced
yet.

Signed-off-by: Ævar Arnfjörð Bjarmason 
---
 t/t5510-fetch.sh | 92 ++--
 1 file changed, 50 insertions(+), 42 deletions(-)

diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh
index 73ba83454f..501d15dd42 100755
--- a/t/t5510-fetch.sh
+++ b/t/t5510-fetch.sh
@@ -562,10 +562,12 @@ test_configured_prune () {
 test_configured_prune_type () {
fetch_prune=$1
remote_origin_prune=$2
-   expected_branch=$3
-   expected_tag=$4
-   cmdline=$5
-   mode=$6
+   fetch_prune_tags=$3
+   remote_origin_prune_tags=$4
+   expected_branch=$5
+   expected_tag=$6
+   cmdline=$7
+   mode=$8
 
if ! test -e prune-type-setup-done
then
@@ -593,14 +595,16 @@ test_configured_prune_type () {
cmdline="$new_cmdline"
fi
 
-   test_expect_success "$mode prune fetch.prune=$1 
remote.origin.prune=$2${5:+ $5}; branch:$3 tag:$4" '
+   test_expect_success "$mode prune fetch.prune=$1 remote.origin.prune=$2 
fetch.pruneTags=$3 remote.origin.pruneTags=$4${7:+ $7}; branch:$5 tag:$6" '
# make sure a newbranch is there in . and also in one
git branch -f newbranch &&
git tag -f newtag &&
(
cd one &&
test_unconfig fetch.prune &&
+   test_unconfig fetch.pruneTags &&
test_unconfig remote.origin.prune &&
+   test_unconfig remote.origin.pruneTags &&
git fetch '"$cmdline_setup"' &&
git rev-parse --verify refs/remotes/origin/newbranch &&
git rev-parse --verify refs/tags/newtag
@@ -615,7 +619,9 @@ test_configured_prune_type () {
cd one &&
git_fetch_c="" &&
set_config_tristate fetch.prune $fetch_prune &&
+   set_config_tristate fetch.pruneTags $fetch_prune_tags &&
set_config_tristate remote.origin.prune 
$remote_origin_prune &&
+   set_config_tristate remote.origin.pruneTags 
$remote_origin_prune_tags &&
 
if test "$mode" != "link"
then
@@ -644,57 +650,59 @@ test_configured_prune_type () {
 
 # $1 config: fetch.prune
 # $2 config: remote..prune
-# $3 expect: branch to be pruned?
-# $4 expect: tag to be pruned?
-# $5 git-fetch $cmdline:
+# $3 config: fetch.pruneTags
+# $4 config: remote..pruneTags
+# $5 expect: branch to be pruned?
+# $6 expect: tag to be pruned?
+# $7 git-fetch $cmdline:
 #
-# $1$2$3 $4 $5
-test_configured_prune unset unset kept   kept   ""
-test_configured_prune unset unset kept   kept   "--no-prune"
-test_configured_prune unset unset pruned kept   "--prune"
-test_configured_prune unset unset kept   pruned \
+# $1$2$3$4$5 $6 $7
+test_configured_prune unset unset unset unset kept   kept   ""
+test_configured_prune unset unset unset unset kept   kept   "--no-prune"
+test_configured_prune unset unset unset unset pruned kept   "--prune"
+test_configured_prune unset unset unset unset kept   pruned \
"--prune origin refs/tags/*:refs/tags/*"
-test_configured_prune unset unset pruned pruned \
+test_configured_prune unset unset unset unset pruned pruned \
"--prune origin refs/tags/*:refs/tags/* 
+refs/heads/*:refs/remotes/origin/*"
 
-test_configured_prune false unset kept   kept   ""
-test_configured_prune false unset kept   kept   "--no-prune"
-test_configured_prune false unset pruned kept   "--prune"
+test_configured_prune false unset unset unset kept   kept   ""
+test_configured_prune false unset unset unset kept   kept   "--no-prune"
+test_configured_prune false unset unset unset pruned kept   "--prune"
 
-test_configured_prune true  unset pruned kept   ""
-test_configured_prune true  unset pruned kept   "--prune"
-test_configured_prune true  unset kept   kept   "--no-prune"
+test_configured_prune true  unset unset unset pruned kept   ""
+test_configured_prune true  unset unset unset pruned kept   "--prune"
+test_configured_prune true  unset unset unset kept   kept   "--no-prune"
 
-test_configured_prune unset false kept   kept   ""
-test_configured_prune unset false kept   kept   "--no-prune"
-test_configured_prune unset false pruned kept   "--prune"
+test_configured_prune unset false unset unset kept   kept   ""
+test_configured_prune unset false unset unset kept   kept   "--no-prune"
+test_configured_prune unset false unset unset pruned kept   "--prune"
 
-test_configured_prune 

[PATCH v2 11/17] fetch tests: fetch as well as fetch []

[Ævar Arnfjörð Bjarmason]

When a remote URL is supplied on the command-line the internals of the
fetch are different, in particular the code in get_ref_map(). An
earlier version of the subsequent fetch.pruneTags patch hid a segfault
because the difference wasn't tested for.

Now all the tests are run as both of the variants of:

git fetch
git -c [...] fetch $(git config remote.origin.url) $(git config 
remote.origin.fetch)

I'm using -c because while the [fetch] config just set by
set_config_tristate will be picked up, the remote.origin.* config
won't override it as intended.

Work around that and turn this into a purely command-line test by
always setting the variables on the command-line, and translate any
setting of remote.origin.X into fetch.X.

The reason for choosing the names "name" and "link" as opposed to
e.g. "named" and "url" is because they're the same length, which makes
the test output easier to read as it will be aligned.

Due to shellscript quoting madness it's not worthwhile to do all of
this within a test_expect_success, but do the parts that can easily be
done there, including the one-time setting of variables that don't
change between runs to be used by subsequent runs in the 'prune_type
setup' test.

Signed-off-by: Ævar Arnfjörð Bjarmason 
---
 t/t5510-fetch.sh | 47 +++
 1 file changed, 43 insertions(+), 4 deletions(-)

diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh
index dfc749f576..73ba83454f 100755
--- a/t/t5510-fetch.sh
+++ b/t/t5510-fetch.sh
@@ -548,18 +548,52 @@ set_config_tristate () {
;;
*)
git config "$1" "$2"
+   key=$(echo $1 | sed -e 's/^remote\.origin/fetch/')
+   git_fetch_c="$git_fetch_c -c $key=$2"
;;
esac
 }
 
 test_configured_prune () {
+   test_configured_prune_type "$@" "name"
+   test_configured_prune_type "$@" "link"
+}
+
+test_configured_prune_type () {
fetch_prune=$1
remote_origin_prune=$2
expected_branch=$3
expected_tag=$4
cmdline=$5
-
-   test_expect_success "prune fetch.prune=$1 remote.origin.prune=$2${5:+ 
$5}; branch:$3 tag:$4" '
+   mode=$6
+
+   if ! test -e prune-type-setup-done
+   then
+   test_expect_success 'prune_type setup' '
+   git -C one config remote.origin.url >one.remote-url &&
+   git -C one config remote.origin.fetch >one.remote-fetch 
&&
+   remote_url="file://$(cat one.remote-url)" &&
+   remote_fetch="$(cat one.remote-fetch)" &&
+   cmdline_setup="\"$remote_url\" \"$remote_fetch\"" &&
+   touch prune-type-setup-done
+   '
+   fi
+
+   if test "$mode" = 'link'
+   then
+   new_cmdline=""
+
+   if test "$cmdline" = ""
+   then
+   new_cmdline=$cmdline_setup
+   else
+   new_cmdline=$(printf "%s" "$cmdline" | perl -pe 
's[origin(?!/)]["'"$remote_url"'"]g')
+   fi
+
+   cmdline="$new_cmdline"
+   fi
+
+   test_expect_success "$mode prune fetch.prune=$1 
remote.origin.prune=$2${5:+ $5}; branch:$3 tag:$4" '
# make sure a newbranch is there in . and also in one
git branch -f newbranch &&
git tag -f newtag &&
@@ -567,7 +601,7 @@ test_configured_prune () {
cd one &&
test_unconfig fetch.prune &&
test_unconfig remote.origin.prune &&
-   git fetch &&
+   git fetch '"$cmdline_setup"' &&
git rev-parse --verify refs/remotes/origin/newbranch &&
git rev-parse --verify refs/tags/newtag
) &&
@@ -579,10 +613,15 @@ test_configured_prune () {
# then test
(
cd one &&
+   git_fetch_c="" &&
set_config_tristate fetch.prune $fetch_prune &&
set_config_tristate remote.origin.prune 
$remote_origin_prune &&
 
-   git fetch '"$cmdline"' &&
+   if test "$mode" != "link"
+   then
+   git_fetch_c=""
+   fi &&
+   git$git_fetch_c fetch '"$cmdline"' &&
case "$expected_branch" in
pruned)
test_must_fail git rev-parse --verify 
refs/remotes/origin/newbranch
-- 
2.15.1.424.g9478a66081

[PATCH v2 12/17] git fetch doc: add a new section to explain the ins & outs of pruning

[Ævar Arnfjörð Bjarmason]

Add a new section to canonically explain how remote reference pruning
works, and how users should be careful about using it in conjunction
with tag refspecs in particular.

A subsequent commit will update the git-remote documentation to refer
to this section, and details the motivation for writing this in the
first place.

Signed-off-by: Ævar Arnfjörð Bjarmason 
---
 Documentation/git-fetch.txt | 49 +
 1 file changed, 49 insertions(+)

diff --git a/Documentation/git-fetch.txt b/Documentation/git-fetch.txt
index b153aefa68..18fac0da2e 100644
--- a/Documentation/git-fetch.txt
+++ b/Documentation/git-fetch.txt
@@ -99,6 +99,55 @@ The latter use of the `remote..fetch` values can 
be
 overridden by giving the `--refmap=` parameter(s) on the
 command line.
 
+PRUNING
+---
+
+Git has a default disposition of keeping data unless it's explicitly
+thrown away; this extends to holding onto local references to branches
+on remotes that have themselves deleted those branches.
+
+If left to accumulate, these stale references might make performance
+worse on big and busy repos that have a lot of branch churn, and
+e.g. make the output of commands like `git branch -a --contains
+` needlessly verbose, as well as impacting anything else
+that'll work with the complete set of known references.
+
+These remote tracking references can be deleted as a one-off with
+either of:
+
+
+# While fetching
+$ git fetch --prune 
+
+# Only prune, don't fetch
+$ git remote prune 
+
+
+To prune references as part of your normal workflow without needing to
+remember to run that set `fetch.prune` globally, or
+`remote..prune` per-remote in the config. See
+linkgit:git-config[1].
+
+Here's where things get tricky and more specific. The pruning feature
+doesn't actually care about branches, instead it'll prune local <->
+remote references as a function of the refspec of the remote (see
+`` and <> above).
+
+Therefore if the refspec for the remote includes
+e.g. `refs/tags/*:refs/tags/*`, or you manually run e.g. `git fetch
+--prune  "refs/tags/*:refs/tags/*"` it won't be stale remote
+tracking branches that are deleted, but any local tag that doesn't
+exist on the remote.
+
+This might not be what you expect, i.e. you want to prune remote
+``, but also explicitly fetch tags from it, so when you fetch
+from it you delete all your local tags, most of which may not have
+come from the `` remote in the first place.
+
+So be careful when using this with a refspec like
+`refs/tags/*:refs/tags/*`, or any other refspec which might map
+references from multiple remotes to the same local namespace.
+
 OUTPUT
 --
 
-- 
2.15.1.424.g9478a66081

[PATCH v2 13/17] git remote doc: correct dangerous lies about what prune does

[Ævar Arnfjörð Bjarmason]

The "git remote prune " command uses the same machinery as "git
fetch  --prune", and shares all the same caveats, but its
documentation has suggested that it'll just "delete stale
remote-tracking branches under ".

This isn't true, and hasn't been true since at least v1.8.5.6 (the
oldest version I could be bothered to test).

E.g. if "refs/tags/*:refs/tags/*" is explicitly set in the refspec of
the remote it'll delete all local tags  doesn't know about.

Instead, briefly give the reader just enough of a hint that this
option might constitute a shotgun aimed at their foot, and point them
to the new PRUNING section in the git-fetch documentation which
explains all the nuances of what this facility does.

See "[BUG] git remote prune removes local tags, depending on fetch
config" (caci5s_39wnrbfjlfn0xhcy+uewtfn2ymnacrc86z6pjutjw...@mail.gmail.com)
by Michael Giuffrida for the initial report.

Reported-by: Michael Giuffrida 
Signed-off-by: Ævar Arnfjörð Bjarmason 
---
 Documentation/git-remote.txt | 14 +-
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/Documentation/git-remote.txt b/Documentation/git-remote.txt
index 577b969c1b..04e2410aec 100644
--- a/Documentation/git-remote.txt
+++ b/Documentation/git-remote.txt
@@ -172,10 +172,14 @@ With `-n` option, the remote heads are not queried first 
with
 
 'prune'::
 
-Deletes all stale remote-tracking branches under .
-These stale branches have already been removed from the remote repository
-referenced by , but are still locally available in
-"remotes/".
+Deletes stale references associated with . By default stale
+remote-tracking branches under , but depending on global
+configuration and the configuration of the remote we might even prune
+local tags that haven't been pushed there. Equivalent to `git fetch
+--prune `, except that no new references will be fetched.
++
+See the PRUNING section of linkgit:git-fetch[1] for what it'll prune
+depending on various configuration.
 +
 With `--dry-run` option, report what branches will be pruned, but do not
 actually prune them.
@@ -189,7 +193,7 @@ remotes.default is not defined, all remotes which do not 
have the
 configuration parameter remote..skipDefaultUpdate set to true will
 be updated.  (See linkgit:git-config[1]).
 +
-With `--prune` option, prune all the remotes that are updated.
+With `--prune` option, run pruning against all the remotes that are updated.
 
 
 DISCUSSION
-- 
2.15.1.424.g9478a66081

[PATCH v2 14/17] git-fetch & config doc: link to the new PRUNING section

[Ævar Arnfjörð Bjarmason]

Amend the documentation for fetch.prune, fetch..prune and
--prune to link to the recently added PRUNING section.

I'd have liked to link directly to it with "<>" from
fetch-options.txt, since it's included in git-fetch.txt (git-pull.txt
also includes it, but doesn't include that option). However making a
reference across files yields this error:

[...]/Documentation/git-fetch.xml:226: element xref: validity
error : IDREF attribute linkend references an unknown ID "PRUNING"

Signed-off-by: Ævar Arnfjörð Bjarmason 
---
 Documentation/config.txt| 6 +-
 Documentation/fetch-options.txt | 3 +++
 2 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 0e25b2c92b..0f27af5760 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1398,7 +1398,8 @@ fetch.unpackLimit::
 
 fetch.prune::
If true, fetch will automatically behave as if the `--prune`
-   option was given on the command line.  See also `remote..prune`.
+   option was given on the command line.  See also `remote..prune`
+   and the PRUNING section of linkgit:git-fetch[1].
 
 fetch.output::
Control how ref update status is printed. Valid values are
@@ -2944,6 +2945,9 @@ remote..prune::
remove any remote-tracking references that no longer exist on the
remote (as if the `--prune` option was given on the command line).
Overrides `fetch.prune` settings, if any.
++
+See also `remote..prune` and the PRUNING section of
+linkgit:git-fetch[1].
 
 remotes.::
The list of remotes which are fetched by "git remote update
diff --git a/Documentation/fetch-options.txt b/Documentation/fetch-options.txt
index fb6bebbc61..9f5c85ad96 100644
--- a/Documentation/fetch-options.txt
+++ b/Documentation/fetch-options.txt
@@ -74,6 +74,9 @@ ifndef::git-pull[]
line or in the remote configuration, for example if the remote
was cloned with the --mirror option), then they are also
subject to pruning.
++
+See the PRUNING section below for more details.
+
 endif::git-pull[]
 
 ifndef::git-pull[]
-- 
2.15.1.424.g9478a66081

[PATCH v2 17/17] fetch: make the --fetch-prune work with

[Ævar Arnfjörð Bjarmason]

Make the new --prune-tags option work properly when git-fetch is
invoked with a  parameter instead of a 
parameter.

This change is split off from the introduction of --prune-tags due to
the relative complexity of munging the incoming argv, which is easier
to review as a separate change.

Signed-off-by: Ævar Arnfjörð Bjarmason 
---
 Documentation/git-fetch.txt | 21 ++---
 builtin/fetch.c | 17 ++---
 t/t5510-fetch.sh|  6 +++---
 3 files changed, 23 insertions(+), 21 deletions(-)

diff --git a/Documentation/git-fetch.txt b/Documentation/git-fetch.txt
index 574206d139..03666f6215 100644
--- a/Documentation/git-fetch.txt
+++ b/Documentation/git-fetch.txt
@@ -175,24 +175,15 @@ It's reasonable to e.g. configure `fetch.pruneTags=true` 
in
 run, without making every invocation of `git fetch` without `--prune`
 an error.
 
-Another special case of `--prune-tags` is that
-`refs/tags/*:refs/tags/*` will not be implicitly provided if an URL is
-being fetched. I.e.:
-
-
-$ git fetch  --prune --prune-tags
-
-
-Will prune no tags, as opposed to:
+Pruning tags with `--prune-tags` also works when fetching a URL
+instead of a named remote. These will all prune tags not found on
+origin:
 
 
 $ git fetch origin --prune --prune-tags
-
-
-To prune tags given a URL supply the refspec explicitly:
-
-
-$ git fetch  --prune 'refs/tags/*:refs/tags/*'
+$ git fetch origin --prune 'refs/tags/*:refs/tags/*'
+$ git fetch  --prune --prune-tags
+$ git fetch  --prune 'refs/tags/*:refs/tags/*'
 
 
 OUTPUT
diff --git a/builtin/fetch.c b/builtin/fetch.c
index 55a0fc37be..c96f17a9a3 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -1283,7 +1283,10 @@ static int fetch_one(struct remote *remote, int argc, 
const char **argv, int pru
static const char **refs = NULL;
struct refspec *refspec;
int ref_nr = 0;
+   int j = 0;
int exit_code;
+   int maybe_prune_tags;
+   int remote_via_config = remote_is_configured(remote, 0);
 
if (!remote)
die(_("No remote repository specified.  Please, specify either 
a URL or a\n"
@@ -1311,13 +1314,21 @@ static int fetch_one(struct remote *remote, int argc, 
const char **argv, int pru
prune_tags = PRUNE_TAGS_BY_DEFAULT;
}
 
-   if (prune_tags_ok && prune_tags && remote_is_configured(remote, 0))
+   maybe_prune_tags = prune_tags_ok && prune_tags;
+   if (maybe_prune_tags && remote_via_config)
add_prune_tags_to_fetch_refspec(remote);
 
+   if (argc > 0 || (maybe_prune_tags && !remote_via_config)) {
+   size_t nr_alloc = st_add3(argc, maybe_prune_tags, 1);
+   refs = xcalloc(nr_alloc, sizeof(const char *));
+   if (maybe_prune_tags) {
+   refs[j++] = xstrdup("refs/tags/*:refs/tags/*");
+   ref_nr++;
+   }
+   }
+
if (argc > 0) {
-   int j = 0;
int i;
-   refs = xcalloc(st_add(argc, 1), sizeof(const char *));
for (i = 0; i < argc; i++) {
if (!strcmp(argv[i], "tag")) {
i++;
diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh
index 6f3ab7695a..297590814d 100755
--- a/t/t5510-fetch.sh
+++ b/t/t5510-fetch.sh
@@ -748,9 +748,9 @@ test_configured_prune unset unset unset true pruned  kept \
 test_configured_prune_type unset unset unset unset kept kept "origin 
--prune-tags" "name"
 test_configured_prune_type unset unset unset unset kept kept "origin 
--prune-tags" "link"
 test_configured_prune_type unset unset unset unset pruned pruned "origin 
--prune --prune-tags" "name"
-test_configured_prune_type unset unset unset unset kept   kept   "origin 
--prune --prune-tags" "link"
+test_configured_prune_type unset unset unset unset kept   pruned "origin 
--prune --prune-tags" "link"
 test_configured_prune_type unset unset unset unset pruned pruned "--prune 
--prune-tags origin" "name"
-test_configured_prune_type unset unset unset unset kept   kept   "--prune 
--prune-tags origin" "link"
+test_configured_prune_type unset unset unset unset kept   pruned "--prune 
--prune-tags origin" "link"
 test_configured_prune_type unset unset true  unset pruned pruned "--prune 
origin" "name"
 test_configured_prune_type unset unset true  unset kept   pruned "--prune 
origin" "link"
 test_configured_prune_type unset unset unset true  pruned pruned "--prune 
origin" "name"
@@ -772,7 +772,7 @@ test_expect_success 'remove remote.origin.fetch "one"' '
)
 '
 test_configured_prune_type unset unset unset unset kept pruned "origin --prune 

[PATCH v2 16/17] fetch: add a --fetch-prune option and fetch.pruneTags config

[Ævar Arnfjörð Bjarmason]

Add a --fetch-prune option to git-fetch, along with fetch.pruneTags
config option. This allows for doing any of:

git fetch -p -P
git fetch --prune --prune-tags
git fetch -p -P origin
git fetch --prune --prune-tags origin

Or simply:

git config fetch.prune true &&
git config fetch.pruneTags true &&
git fetch

Instead of the much more verbose:

git fetch --prune origin 'refs/tags/*:refs/tags/*' 
'+refs/heads/*:refs/remotes/origin/*'

Before this feature it was painful to support the use-case of pulling
from a repo which is having both its branches *and* tags deleted
regularly, and have our local references to reflect upstream.

At work we create deployment tags in the repo for each rollout, and
there's *lots* of those, so they're archived within weeks for
performance reasons.

Without this change it's hard to centrally configure such repos in
/etc/gitconfig (on servers that are only used for working with
them). You need to set fetch.prune=true globally, and then for each
repo:

git -C {} config --replace-all remote.origin.fetch 
"refs/tags/*:refs/tags/*" "^\+*refs/tags/\*:refs/tags/\*$"

Now I can simply set fetch.pruneTags=true in /etc/gitconfig as well,
and users running "git pull" will automatically get the pruning
semantics I want.

Even though "git remote" has corresponding "prune" and "update
--prune" subcommands I'm intentionally not adding a corresponding
prune-tags or "update --prune --prune-tags" mode to that command.

It's advertised (as noted in my recent "git remote doc: correct
dangerous lies about what prune does") as only modifying remote
tracking references, whereas any --prune-tags option is always going
to modify what from the user's perspective is a local copy of the tag,
since there's no such thing as a remote tracking tag.

Ideally add_prune_tags_to_fetch_refspec() would be something that
would use ALLOC_GROW() to grow the 'fetch` member of the 'remote'
struct. Instead I'm realloc-ing remote->fetch and adding the
tag_refspec to the end.

The reason is that parse_{fetch,push}_refspec which allocate the
refspec (ultimately remote->fetch) struct are called many places that
don't have access to a 'remote' struct. It would be hard to change all
their callsites to be amenable to carry around the bookkeeping
variables required for dynamic allocation.

All the other callers of the API first incrementally construct the
string version of the refspec in remote->fetch_refspec via
add_fetch_refspec(), before finally calling parse_fetch_refspec() via
some variation of remote_get().

It's less of a pain to deal with the one special case that needs to
modify already constructed refspecs than to chase down and change all
the other callsites. The API I'm adding is intentionally not
generalized because if we add more of these we'd probably want to
re-visit how this is done.

See my "Re: [BUG] git remote prune removes local tags, depending on
fetch config" (87po6ahx87@evledraar.gmail.com;
https://public-inbox.org/git/87po6ahx87@evledraar.gmail.com/) for
more background info.

Signed-off-by: Ævar Arnfjörð Bjarmason 
---
 Documentation/config.txt   | 14 +++
 Documentation/fetch-options.txt| 14 ++-
 Documentation/git-fetch.txt| 47 +++
 builtin/fetch.c| 32 ++--
 contrib/completion/git-completion.bash |  2 +-
 remote.c   | 14 +++
 remote.h   |  3 ++
 t/t5510-fetch.sh   | 69 ++
 8 files changed, 190 insertions(+), 5 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 0f27af5760..e254bfd531 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1401,6 +1401,14 @@ fetch.prune::
option was given on the command line.  See also `remote..prune`
and the PRUNING section of linkgit:git-fetch[1].
 
+fetch.pruneTags::
+   If true, fetch will automatically behave as if the
+   `refs/tags/*:refs/tags/*` refspec was provided when pruning,
+   if not set already. This allows for setting both this option
+   and `fetch.prune` to maintain a 1=1 mapping to upstream
+   refs. See also `remote..pruneTags` and the PRUNING
+   section of linkgit:git-fetch[1].
+
 fetch.output::
Control how ref update status is printed. Valid values are
`full` and `compact`. Default value is `full`. See section
@@ -2945,6 +2953,12 @@ remote..prune::
remove any remote-tracking references that no longer exist on the
remote (as if the `--prune` option was given on the command line).
Overrides `fetch.prune` settings, if any.
+
+remote..pruneTags::
+   When set to true, fetching from this remote by default will also
+   remove any local tags that no longer exist on the remote if pruning
+   is activated in general via `remote..prune`, `fetch.prune` 

[PATCH v2 05/17] fetch tests: refactor in preparation for testing tag pruning

[Ævar Arnfjörð Bjarmason]

In a subsequent commit this function will learn to test for tag
pruning, prepare for that by making space for more variables, and
making it clear that "expected" here refers to branches.

Signed-off-by: Ævar Arnfjörð Bjarmason 
---
 t/t5510-fetch.sh | 9 ++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh
index 668c54be41..11da97f9b7 100755
--- a/t/t5510-fetch.sh
+++ b/t/t5510-fetch.sh
@@ -549,9 +549,12 @@ set_config_tristate () {
 }
 
 test_configured_prune () {
-   fetch_prune=$1 remote_origin_prune=$2 cmdline=$3 expected=$4
+   fetch_prune=$1
+   remote_origin_prune=$2
+   cmdline=$3
+   expected_branch=$4
 
-   test_expect_success "prune fetch.prune=$1 remote.origin.prune=$2${3:+ 
$3}; $4" '
+   test_expect_success "prune fetch.prune=$1 remote.origin.prune=$2${3:+ 
$3}; branch:$4" '
# make sure a newbranch is there in . and also in one
git branch -f newbranch &&
(
@@ -572,7 +575,7 @@ test_configured_prune () {
set_config_tristate remote.origin.prune 
$remote_origin_prune &&
 
git fetch $cmdline &&
-   case "$expected" in
+   case "$expected_branch" in
pruned)
test_must_fail git rev-parse --verify 
refs/remotes/origin/newbranch
;;
-- 
2.15.1.424.g9478a66081

[PATCH v2 03/17] fetch: stop accessing "remote" variable indirectly

[Ævar Arnfjörð Bjarmason]

Access the "remote" variable passed to the fetch_one() directly rather
than through the gtransport wrapper struct constructed in this
function for other purposes.

This makes the code more readable, as it's now obvious that the remote
struct doesn't somehow get munged by the prepare_transport() function
above, which takes the "remote" struct as an argument and constructs
the "gtransport" struct, containing among other things the "remote"
struct.

A subsequent change will copy this pattern to access a new
remote->prune_tags field, but without the use of the gtransport
variable. It's useful once that change lands to see that the two
pieces of code behave exactly the same.

This pattern of accessing the container struct was added in
737c5a9cde ("fetch: make --prune configurable", 2013-07-13) when this
code was initially introduced.

Signed-off-by: Ævar Arnfjörð Bjarmason 
---
 builtin/fetch.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/builtin/fetch.c b/builtin/fetch.c
index 72085e30b9..a7705bc150 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -1280,8 +1280,8 @@ static int fetch_one(struct remote *remote, int argc, 
const char **argv)
 
if (prune < 0) {
/* no command line request */
-   if (0 <= gtransport->remote->prune)
-   prune = gtransport->remote->prune;
+   if (0 <= remote->prune)
+   prune = remote->prune;
else if (0 <= fetch_prune_config)
prune = fetch_prune_config;
else
-- 
2.15.1.424.g9478a66081

Re: "git branch" issue in 2.16.1

[Kevin Daudt]

On Thu, Feb 08, 2018 at 12:27:07PM +0100, Lars Schneider wrote:
> 
> > On 08 Feb 2018, at 12:13, Lars Schneider  wrote:
> > 
> > 
> >> On 08 Feb 2018, at 09:50, Jeff King  wrote:
> >> 
> >> On Wed, Feb 07, 2018 at 11:20:08PM +0100, Lars Schneider wrote:
> >> 
>  1. You have $LESS in your environment (without "F") on one platform
>    but not the other.
> >>> 
> >>> I think that's it. On my system LESS is defined to "-R".
> >>> 
> >>> This opens the pager:
> >>> 
> >>>   $ echo "TEST" | less
> >>> 
> >>> This does not open the pager:
> >>> 
> >>>   $ echo "TEST" | less -FRX
> >>> 
> >>> That means "F" works on macOS but Git doesn't set it because LESS is
> >>> already in my environment.
> >>> 
> >>> Question is, why is LESS set that way on my system? I can't find
> >>> it in .bashrc .bash_profile .zshrc and friends.
> >> 
> >> There's also /etc/bash.bashrc, /etc/profile, etc. I don't know what's
> >> normal in the mac world. You can try running:
> >> 
> >> bash -ix 2>&1  >> 
> >> to see what your startup code is doing. I don't know of a good way to
> >> correlate that with the source files, though. Or even to ask bash which
> >> startup files it's looking in.
> > 
> > Unfortunately, this command doesn't work for me.
> > 
> > I ask around and most of my coworkers have LESS="-R".
> > Only the coworker that doesn't really use his Mac and has
> > no customizations does not have $LESS defined.
> > 
> > Therefore, I think it is likely some third party component
> > that sets $LESS.
> > 
> > @Jason:
> > Do you have homebrew, iTerm2, and/or oh-my-zsh installed?
> 
> Ha. I found it it! It is indeed oh-my-zsh:
> https://github.com/robbyrussell/oh-my-zsh/blob/master/lib/misc.zsh#L23
> 
> Let's see if oh-my-zsh is willing to change that...
> 
> - Lars

I've just added unset LESS in my .zshrc, but for most users it would be
better if they don't set it at all.

[PATCH v2 07/17] fetch tests: add a tag to be deleted to the pruning tests

[Ævar Arnfjörð Bjarmason]

Add a tag to be deleted to the fetch --prune tests. The tag is always
kept for now, which is the expected behavior, but now I can add a test
for tag pruning in a later commit.

Signed-off-by: Ævar Arnfjörð Bjarmason 
---
 t/t5510-fetch.sh | 93 
 1 file changed, 53 insertions(+), 40 deletions(-)

diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh
index ab8b25344d..fad65bd885 100755
--- a/t/t5510-fetch.sh
+++ b/t/t5510-fetch.sh
@@ -552,21 +552,25 @@ test_configured_prune () {
fetch_prune=$1
remote_origin_prune=$2
expected_branch=$3
-   cmdline=$4
+   expected_tag=$4
+   cmdline=$5
 
-   test_expect_success "prune fetch.prune=$1 remote.origin.prune=$2${4:+ 
$4}; branch:$3" '
+   test_expect_success "prune fetch.prune=$1 remote.origin.prune=$2${5:+ 
$5}; branch:$3 tag:$4" '
# make sure a newbranch is there in . and also in one
git branch -f newbranch &&
+   git tag -f newtag &&
(
cd one &&
test_unconfig fetch.prune &&
test_unconfig remote.origin.prune &&
git fetch &&
-   git rev-parse --verify refs/remotes/origin/newbranch
+   git rev-parse --verify refs/remotes/origin/newbranch &&
+   git rev-parse --verify refs/tags/newtag
) &&
 
# now remove it
git branch -d newbranch &&
+   git tag -d newtag &&
 
# then test
(
@@ -582,6 +586,14 @@ test_configured_prune () {
kept)
git rev-parse --verify 
refs/remotes/origin/newbranch
;;
+   esac &&
+   case "$expected_tag" in
+   pruned)
+   test_must_fail git rev-parse --verify 
refs/tags/newtag
+   ;;
+   kept)
+   git rev-parse --verify refs/tags/newtag
+   ;;
esac
)
'
@@ -590,44 +602,45 @@ test_configured_prune () {
 # $1 config: fetch.prune
 # $2 config: remote..prune
 # $3 expect: branch to be pruned?
-# $4 git-fetch $cmdline:
+# $4 expect: tag to be pruned?
+# $5 git-fetch $cmdline:
 #
-# $1$2$3 $4
-test_configured_prune unset unset kept   ""
-test_configured_prune unset unset kept   "--no-prune"
-test_configured_prune unset unset pruned "--prune"
-
-test_configured_prune false unset kept   ""
-test_configured_prune false unset kept   "--no-prune"
-test_configured_prune false unset pruned "--prune"
-
-test_configured_prune true  unset pruned ""
-test_configured_prune true  unset pruned "--prune"
-test_configured_prune true  unset kept   "--no-prune"
-
-test_configured_prune unset false kept   ""
-test_configured_prune unset false kept   "--no-prune"
-test_configured_prune unset false pruned "--prune"
-
-test_configured_prune false false kept   ""
-test_configured_prune false false kept   "--no-prune"
-test_configured_prune false false pruned "--prune"
-
-test_configured_prune true  false kept   ""
-test_configured_prune true  false pruned "--prune"
-test_configured_prune true  false kept   "--no-prune"
-
-test_configured_prune unset true  pruned ""
-test_configured_prune unset true  kept   "--no-prune"
-test_configured_prune unset true  pruned "--prune"
-
-test_configured_prune false true  pruned ""
-test_configured_prune false true  kept   "--no-prune"
-test_configured_prune false true  pruned "--prune"
-
-test_configured_prune true  true  pruned ""
-test_configured_prune true  true  pruned "--prune"
-test_configured_prune true  true  kept   "--no-prune"
+# $1$2$3 $4 $5
+test_configured_prune unset unset kept   kept   ""
+test_configured_prune unset unset kept   kept   "--no-prune"
+test_configured_prune unset unset pruned kept   "--prune"
+
+test_configured_prune false unset kept   kept   ""
+test_configured_prune false unset kept   kept   "--no-prune"
+test_configured_prune false unset pruned kept   "--prune"
+
+test_configured_prune true  unset pruned kept   ""
+test_configured_prune true  unset pruned kept   "--prune"
+test_configured_prune true  unset kept   kept   "--no-prune"
+
+test_configured_prune unset false kept   kept   ""
+test_configured_prune unset false kept   kept   "--no-prune"
+test_configured_prune unset false pruned kept   "--prune"
+
+test_configured_prune false false kept   kept   ""
+test_configured_prune false false kept   kept   "--no-prune"
+test_configured_prune false false pruned kept   "--prune"
+
+test_configured_prune true  false kept   kept   ""
+test_configured_prune true  false pruned kept   "--prune"
+test_configured_prune 

[PATCH v2 00/17] document & test fetch pruning & add fetch.pruneTags

[Ævar Arnfjörð Bjarmason]

As noted in my 87h8quytmq@evledraar.gmail.com there was a bug I
noticed in v3 where it would segfault on some git-fetch invocations,
but there were not tests anywhere that caught that.

So in addition to fixing that issue, this fleshens out the testing
being set up as part of this series so we'll test those sorts of
invocations. It would segfault on some `git fetch `, not `git
fetch `.

Ævar Arnfjörð Bjarmason (17):
  fetch: don't redundantly NULL something calloc() gave us

Rephrased commit message.

  fetch: trivially refactor assignment to ref_nr

New, makes a subsequent change smaller.

  fetch: stop accessing "remote" variable indirectly

Typo fix in commit message noted by Junio.

  remote: add a macro for "refs/tags/*:refs/tags/*"

New, makes a subsequent change smaller.

  fetch tests: refactor in preparation for testing tag pruning
  fetch tests: re-arrange arguments for future readability
  fetch tests: add a tag to be deleted to the pruning tests

No changes.

  fetch tests: test --prune and refspec interaction

Changed +refs/tags/*:refs/tags/ to refs/tags/*:refs/tags/. No
functional difference, since git doesn't care. Just to be consistent
with the macro added earlier & doing the same in commit messages &
tests later in the series.

  fetch tests: double quote a variable for interpolation

Now back from an earlier version, needed for a later change.

  fetch tests: expand case/esac for later change

New, makes the next patch smaller / easier to review.

  fetch tests: fetch   as well as fetch []

For all `git fetch ` we now run another version of the test
where we test an equivalent `git fetch `. This sort of exhaustive
testing was missing in our whole test suite, and would have caught the
segfault in v3.

  git fetch doc: add a new section to explain the ins & outs of pruning
  git remote doc: correct dangerous lies about what prune does
  git-fetch & config doc: link to the new PRUNING section

No changes except omitting the "+" in front of refs/tags/[...] as
noted above.

  fetch tests: add scaffolding for the new fetch.pruneTags

Ditto "+" change + minor changes carried over from previous patches.

  fetch: add a --fetch-prune option and fetch.pruneTags config

The bug in v3 was that the remote->fetch variable needs to chaned in
lockstep with remote->fetch_refspec, but only the latter was
changed. Codepaths that fetched by URL would under --prune-tags expect
as many items in both, and segfault on the access to remote->fetch.

As explained in the amended commit message the API is not amenable to
ALLOC_GROW, so there's now a add_prune_tags_to_fetch_refspec()
function in remote.c which adds the new element to remote->fetch via
xrealloc() + memcpy().

Careful review of that most welcome.

There's lots more tests that catch the case where it segfaulted.

  fetch: make the --fetch-prune work with 

The previous patch was changed to document that this wouldn't work:

git fetch  --prune --prune-tag

This makes it work, at the cost of some complexity in fetch_one(). I
think it makes sense to keep this, I just wanted to split it off from
the previous patch to clearly show the hoops we need to jump through
for that one case.

 Documentation/config.txt   |  20 ++-
 Documentation/fetch-options.txt|  17 ++-
 Documentation/git-fetch.txt|  87 
 Documentation/git-remote.txt   |  14 +-
 builtin/fetch.c|  54 ++--
 contrib/completion/git-completion.bash |   2 +-
 remote.c   |  15 ++
 remote.h   |   5 +
 t/t5510-fetch.sh   | 242 +++--
 9 files changed, 395 insertions(+), 61 deletions(-)

-- 
2.15.1.424.g9478a66081