date:20181024

Uwe Kleine-König  writes:

> Without passing --allow-unrelated-histories the command sequence
> fails as intended since commit e379fdf34fee ("merge: refuse to create
> too cool a merge by default"). To setup a subtree merging unrelated
> histories is normal, so add the option to the howto document.
>

Thanks.  We should have been more careful when we tightened "git
merge".

Will apply.

> Signed-off-by: Uwe Kleine-König 
> ---
>  Documentation/howto/using-merge-subtree.txt | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/Documentation/howto/using-merge-subtree.txt 
> b/Documentation/howto/using-merge-subtree.txt
> index 1ae8d1214ec0..a499a94ac228 100644
> --- a/Documentation/howto/using-merge-subtree.txt
> +++ b/Documentation/howto/using-merge-subtree.txt
> @@ -33,7 +33,7 @@ Here is the command sequence you need:
>  
>  
>  $ git remote add -f Bproject /path/to/B <1>
> -$ git merge -s ours --no-commit Bproject/master <2>
> +$ git merge -s ours --no-commit --allow-unrelated-histories Bproject/master 
> <2>
>  $ git read-tree --prefix=dir-B/ -u Bproject/master <3>
>  $ git commit -m "Merge B project as our subdirectory" <4>

Re: New semantic patches vs. in-flight topics [was: Re: [PATCH 00/19] Bring more repository handles into our code base]

On Mon, Oct 22, 2018 at 07:39:35PM +0200, SZEDER Gábor wrote:

> I don't really like how this or the previous RFC patch series deal
> with semantic patches (or how some past patch series dealt with them,
> for that matter), for various reasons:
> [..]

I am a little late to this thread, but it really seems to me that the
crux of the issue here:

>   - 'make coccicheck' won't run clean (and the static analysis build
> job on Travis CI will fail) for all commits following adding the
> new semantic patches but before applying the resulting
> transformations.
> 
>   - These semantic patches interact badly with 'pu' and 'next',
> because those integration branches can contain topic branches
> adding new code that should be transformed by these semanic
> patches.  Consequently, 'make coccicheck' won't run clean and the
> static analysis build job will fail until all those topics reach
> 'master', and the remaining transformations are applied on top.

Is that we are considering it a failure for "pu" to have pending
coccinelle patches. Are there any coccicheck transformations that aren't
just "we prefer to do it like X instead of Y"?

Changes that actually break things should be caught by the compiler
(either by their nature, or because we take care to rename or change
interfaces when making a semantic change to a function).

I think that's getting at what you're saying here:

>   - Is it really necessary to carry these semantic patches in-tree?
> Let me ellaborate.  There are basically two main use cases for
> semantic patches:
> 
>   - To avoid undesirable code patterns, e.g. we should not use
> [...]
>   - To perform one-off code transformations, e.g. to modify a

I am mostly thinking of your first type here. And I think it makes sense
for people to avoid submitting code that does not pass "make coccicheck"
_as it was at the state of their branch_. But for cocci changes that are
in flight at the same time as their branch, I do not see any need to
strictly adhere to them. The code is "undesirable", not "wrong".

For the second type, I agree that we do not necessarily need to carry
them in-tree. Eventually the transformation happens, and nobody would
use the "old" way because it doesn't work anymore. Problem solved.

I do not mind carrying them for a while as a convenience to people who
will need to fix up their topics in flight (or more likely to the
maintainer, who will need to fixup the merge). But we should make sure
not to forget to remove them when their usefulness has passed.

Likewise, we should not forget to occasionally run "make coccicheck" on
master to make sure people have a clean base to build on. If Junio is
able to do that, then great. But other people can also do so and submit
patches (to go on their respective topics, or to be a new mass-cleanup
topic).

I guess there is some lag there if Junio pushes out a master branch that
fails coccicheck, because contributors may build on it before somebody
gets around to fixing it.

> Having said that, it's certainly easier to double-check the
> resulting transformations when one can apply the semantic
> patches locally, and doing so is easier when the semantic
> patches are in tree than when they must be copy-pasted from a
> commit message.

I've wondered if we could have a script that pulls a coccinelle snippet
from a commit message and runs it. It may be a hassle to find the right
commit, though (you'd start the procedure from "oops, my compile now
seems broken; what was the change that I need to apply to adapt?").

>   - A new semantic patch should be added as "pending", e.g. to the
> file 'the_repository.pending.cocci', together with the resulting
> transformations in the same commit.
> 
> This way neither 'make coccicheck' nor the static analysis build
> job would complain in the topic branch or in the two integration
> branches.  And if they do complain, then we would know right away
> that they complain because of a well-established semantic patch.
> Yet, anyone interested could run 'make coccicheck-pending' to see
> where are we heading.

OK, makes sense.

>   - The author of the "pending" semanting patch should then keep an
> eye on already cooking topics: whether any of them contain new
> code that should be transformed, and how they progress to
> 'master', and sending followup patch(es) with the remaining
> transformations when applicable.
> 
> Futhermore, the author should also pay attention to any new topics
> that branch off after the "pending" semantic patch, and whether
> any of them introduce code to be transformed, warning their
> authors as necessary.

This part seems tricky, though. There's a race condition between
promoting the patch from pending to not-pending and other topics
branching off. And remember that we do not always see other people's
branches, which they may work on in

Re: the opposite of .gitignore, whitelist

"lhf...@163.com"  writes:

> I have a good idea, add a file to git that is the opposite of .gitignore...,

Do negative patterns in .gitignore file help without inventing
anything new?

Re: [PATCH] sequencer: cleanup for gcc 8.2.1 warning

Carlo Marcelo Arenas Belón   writes:

> sequencer.c: In function ‘write_basic_state’:
> sequencer.c:2392:37: warning: zero-length gnu_printf format string 
> [-Wformat-zero-length]
>write_file(rebase_path_verbose(), "");

The change may squelch the above warning, but doesn't it change the
behaviour?  You need to explain what the changed behaviour is and
why it is OK to change that behaviour in this space, in addition to
show what warning you are working around.

I _think_ the intent of this code is to create an empty file,
because that is what the original version of "git rebase" did.
write_file() does use strbuf_complete_line() to avoid creating
a file with incomplete last line, but it does allow us to create
an empty file.  By adding a LF, the created file is no longer an
empty one.

Does it matter?  IOW, does it cause a regression if we start adding
an LF to the file?

By reading master:git-rebase.sh::read_basic_state(), I _think_ it is
safe to do so, as the side that consumes this $state_dir/verbose
only checks file's existence, and does not care what it contains,
even if somehow the scripted version of "git rebase" ends up reading
the state file created by this newer version of "git rebase" in C.
Also next:sequencer.c::read_populate_opts() only checks for file's
existence, so the newer version of "git rebase" is also safe.

So as an immediate workaround for 'next', this patch happens to be
OK, but that is only true because the consumer happens to ignore the
distinction between an empty file and a file with a single LF in it.

I'd have to say that the ability to create an empty file is more
important in the longer term.  Can't the workaround be done to
write_file() instead?  I actually do not mind if the solution were
to introduce a newhelper "write_empty_file()", but the way it is
written in the code before this patch, i.e.

write_file(FILENAME, "")

is so obvious a way to create an empty file, so if we do not have to
resort to such a hackery to special case an empty file, that would
be preferrable.

Thanks.

>
> Signed-off-by: Carlo Marcelo Arenas Belón 
> ---
>  sequencer.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/sequencer.c b/sequencer.c
> index 8dd6db5a01..358e83bf6b 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -2335,7 +2335,7 @@ int write_basic_state(struct replay_opts *opts, const 
> char *head_name,
>   write_file(rebase_path_quiet(), "\n");
>  
>   if (opts->verbose)
> - write_file(rebase_path_verbose(), "");
> + write_file(rebase_path_verbose(), "\n");
>   if (opts->strategy)
>   write_file(rebase_path_strategy(), "%s\n", opts->strategy);
>   if (opts->xopts_nr > 0)

Re: Git trademark status and policy

On Wed, Oct 24, 2018 at 12:55:33AM -0700, David Aguilar wrote:

> > So I think we should generally recommend against such generic names
> > during the naming phase. At this point, I'm not sure the pain of
> > changing now is any less than the pain of changing later if and when
> > there's a conflict.
> [...]
> 
> Thanks for the recommendation.  I'm open to changing the name in a
> future major release.  For users that already use the short "dag" name,
> we can transition over to something else if it's relatively short and
> sweet.

Going from my paragraph above, I think it is probably OK to just leave
it for now (unless you prefer to use a major version boundary to do the
change rather than later possibly having to deal with it on a shorter
timeframe).

I have no real opinion on a replacement name. :)

> There's also one more script, but it's never installed in the users's
> $PATH and is more of an internal implementation detail.  Git Cola
> includes a GIT_SEQUENCE_EDITOR-compatible "git-xbase" command that
> provides a visual interactive rebase feature.  That command should
> probably be renamed to "cola-git-seq-editor" to make that clearer, and
> also to open up the possibility of installing it in bin/ in the future
> since it is useful on its own.

Yeah, agreed. If it's not in the PATH, then it doesn't need to be git-*
at all, does it?

> The rationale for two commands is that worktree diff+commit and history
> inspection are our two primary use-cases.  Everything else is provided
> as a sub-command, "git cola rebase", "git cola stash", etc. so there's
> not much pressure to add more top-level names, just these two.

Makes sense.

> Thoughts?

Everything you said seems pretty reasonable to me. Thanks for being
conscientious about the naming issues.

-Peff

Re: [PATCH v4 2/3] reset: add new reset.quiet config setting

Ramsay Jones  writes:

>> diff --git a/Documentation/config.txt b/Documentation/config.txt
>> index f6f4c21a54..a2d1b8b116 100644
>> --- a/Documentation/config.txt
>> +++ b/Documentation/config.txt
>> @@ -2728,6 +2728,9 @@ rerere.enabled::
>>  `$GIT_DIR`, e.g. if "rerere" was previously used in the
>>  repository.
>>  
>> +reset.quiet::
>> +When set to true, 'git reset' will default to the '--quiet' option.
>
> Mention that this 'Defaults to false'?

Perhaps.

>>  -q::
>>  --quiet::
>> -Be quiet, only report errors.
>> +--no-quiet::
>> +Be quiet, only report errors. The default behavior is set by the
>> +`reset.quiet` config option. `--quiet` and `--no-quiet` will
>> +override the default behavior.
>
> Better than last time, but how about something like:
>
>  -q::
>  --quiet::
>  --no-quiet::
>   Be quiet, only report errors. The default behaviour of the
>   command, which is to not be quiet, can be specified by the
>   `reset.quiet` configuration variable. The `--quiet` and
>   `--no-quiet` options can be used to override any configured
>   default.
>
> Hmm, I am not sure that is any better! :-D

To be honest, I find the second sentence in your rewrite even more
confusing.  It reads as if `reset.quiet` configuration variable 
can be used to restore the "show what is yet to be added"
behaviour, due to the parenthetical mention of the default behaviour
without any configuration.

The command reports what is yet to be added to the index
after `reset` by default.  It can be made to only report
errors with the `--quiet` option, or setting `reset.quiet`
configuration variable to `true` (the latter can be
overriden with `--no-quiet`).

That may not be much better, though X-<.

Re: [PATCH v2 1/3] [Outreachy] t3903-stash: test without configured user name

Eric Sunshine  writes:

>> +test_commit 1 &&
>> +test_config user.useconfigonly true &&
>> +test_config stash.usebuiltin true &&
>> +sane_unset GIT_AUTHOR_NAME &&
>> +sane_unset GIT_AUTHOR_EMAIL &&
>> +sane_unset GIT_COMMITTER_NAME &&
>> +sane_unset GIT_COMMITTER_EMAIL &&
>> +test_must_fail git config user.email &&
>
> Instead of simply asserting that 'user.email' is not set here, you
> could instead proactively ensure that it is not set. That is, instead
> of the test_must_fail(), do this:
>
> test_unconfig user.email &&
> test_unconfig user.name &&

Yes, it would be more in line with what is done to the environment
variables and to other configuration variables in the same block.

Not that I think that this inconsistency is end of the world ;-)

Thanks.

>> +echo changed >1.t &&
>> +git stash
>> +'
>> +
>>  test_done
>> --
>> 2.19.1.windows.1
>>

Re: [PATCH 1/3] [Outreachy] t3903-stash: test without configured user name

Slavica  writes:

> On 23-Oct-18 8:52 PM, Christian Couder wrote:
>> On Tue, Oct 23, 2018 at 6:35 PM Slavica  wrote:
>>> This is part of enhancement request that ask for `git stash` to work even 
>>> if `user.name` is not configured.
>>> The issue is discussed here: 
>>> https://public-inbox.org/git/87o9debty4@evledraar.gmail.com/T/#u.
>> We prefer commit messages that contain as much as possible all the
>> information necessary to understand the patch without links to other
>> places.
>>
>> It seems that only this email from you reached me. Did you send other
>> emails for patches 2/3 and 3/3?
>>
>> [...]
>
> Okay, I will change that. This is my first patch and I am still adapting.
>
> Emails for patches 2/3 and 3/3 because aren't there because I am still
> preparing them.
>
> (I didn't know if I had 3 patches in plan that they should be sent at
> almost the same time.)

It is more efficient for everybody involved.

 - You may discover that 1/3 you just (thought) finished was not
   sufficient while working on 2/3 and 3/3, and by the time you are
   pretty close to finishing 2/3 and 3/3, you may want to update 1/3
   in a big way.  Sending a premature version and having others to
   review is wasting everbody's time.

 - Your 1/3 might become perfect alone with help from others'
   reviews and your updates, but after that everybody may forget
   about it when you are ready to send out 2/3 and 3/3; if these
   three are truly related patches in a single topic, you would want
   to have what 1/3 did fresh in your reviewers' minds.  You'd have
   to find the old message of 1/3 and make 2/3 and 3/3 responses to
   it to keep them properly threaded (which may take your time), and
   reviewers need to refresh their memory by going back to 1/3
   before reviewing 2/3 and 3/3

One thing I learned twice while working in this project is that open
source development is not a race to produce and show your product as
quickly as possible.

When I was an individual contributor, the project was young and
there were many people with good and competing ideas working to
achieve more-or-less the same goal.  It felt like a competition
to get *MY* version of the vision, design and implementation over
others' adopted and one way to stay in the competition was to send
things as quickly as possible.  I didn't know better, and I think I
ended up wasting many people's time that way.

That changed when I became the maintainer, as (1) I no longer had to
race with anybody ;-), and (2) I introduced the 'pu' (proposed
update) system so that anything that was queued early can be
discarded and replaced when a better thing come within a reasonable
timeframe.

And then I re-learned the same "this is not a race" lesson a couple
of years ago, when I started working in a timezone several hours
away from the most active participants for a few months at a time.
I do not have to respond to a message I see on the list immediately,
as it is too late to catch the sender who is already in bed ;-)

So take your time and make sure what you are sending out can be
reviewed the most efficiently.  Completing 2/3 and 3/3 before
sending 1/3 out to avoid having to redo 1/3 and avoid having
reviewers to spend their time piecemeal is one thing.  Making sure
that the patch does not have style issues that distract reviewers'
attention is another.

Sitting on what you think you have completed for a few days allows
you to review your product with fresh eyes before sending them out,
which is another benefit of trying not to rush.

Re: [PATCH 1/3] [Outreachy] t3903-stash: test without configured user name

Johannes Schindelin  writes:

>> HOME is set to TRASH_DIRECTORY in t/test-lib.sh already, and we do
>> so to avoid getting affected by the real $HOME/.gitconfig of the
>> user who happens to be running the test suite.
>
> My bad. I should have checked. I was under the impression that we set
> `HOME` to the `t/` directory and initialized it. But you are right, of
> course, and that subshell as well as the override of `HOME` are absolutely
> unnecessary.

I was afraid that I may be missing some future plans to update
$TRASH_DIRECTORY/.gitconfig with "git config --global user.name Foo"
etc. in an earlier part of the test script, which would have made
the subshell and moving HOME elsewhere perfectly good ways to future
proof the new test being added (in which case, in-code comment to
say that near the assignment to HOME would have been a good
improvement).

Not that having them breaks the logic, but they distract the
readers by making them wonder what is going on, so I think we can do
without the subshell and assignment to HOME.

Thanks.

Re: [PATCH v2 0/2] Work around case-insensitivity issues with cwd on Windows

Johannes Schindelin  writes:

> On Wed, 24 Oct 2018, Junio C Hamano wrote:
>
>> "Johannes Schindelin via GitGitGadget" 
>> writes:
>> 
>> > Changes since v1:
>> >
>> >  * Fixed a grammar mistake in the second commit message.
>> 
>> Thanks.  I think this matches what I earlier queued on 'pu' with
>> Stephen's typofix squashed in, so we are good already.
>
> Perfect. Sorry that I forgot to check.

Thanks for double-checking, and I do appreciate the final version,
even if it matches the version I would have in 'pu' if I perfectly
followed all what was said in the review thread, as I am not always
perfect.

Re: Recommended configurations (was Re: [PATCH v1 2/2] reset: add new reset.quietDefault config setting)

Jeff King  writes:

> I do hope that some options will just be no-brainers to enable always,
> though (e.g., I think in the long run commit-graph should just default
> to "on"; it's cheap to keep up to date and helps proportionally to the
> repo size).

Same here.

We should strive to make any feature to optimize for repositories
with a particular trait not to hurt too much to repositories without
that trait, so that we can start such a feature as opt-in but later
can make it the default for everybody.  Sometimes it may not be
possible, but my gut feeling is that features aiming for optimizing
big repositories should fundamentally need only very small overhead
when enabled in a small repository.

So I view them not as a set of million "if your repository matches
this criterion, turn it on" knobs.  Rather, they are "we haven't
tested it fully, but you can opt into the experiment a new way to do
the same operation, which is designed to optimize for repositories
with this trait. Enabling it even when your repository does not have
that trait and reporting regression is also very welcome, as it is a
good indication that the new way has rough edges at its corners".

Thanks.

Re: [PATCH] Poison gettext with the Ook language

Duy Nguyen  writes:

> The person who writes
>
>  printf(_("%s"), getenv("foo"));
>
> may not go through the same thought process as with complexFunction().
> If _() calls getenv(), because you the order of parameter evaluation
> is unspecified, you cannot be sure if getenv("foo") will be called
> before or after the one inside _(). One of them may screw up the
> other.

Yup, sometimes we've been sloppy but we should strive to mimick
efforts like f4ef5173 ("determine_author_info(): copy getenv
output", 2014-08-27).

[PATCH] http: give curl version warnings consistently

When a requested feature cannot be activated because the version of
cURL library used to build Git with is too old, most of the codepaths
give a warning like "$Feature is not supported with cURL < $Version",
marked for l10n.  A few of them, however, did not follow that pattern
and said things like "$Feature is not activated, your curl version is
too old (>= $Version)", and without marking them for l10n.

Update these to match the style of the majority of warnings and mark
them for l10n.

Signed-off-by: Junio C Hamano 
---

 > I have a clean-up suggestion related to this but is orthogonal to
 > this three-patch series (after the fix-up is applied, anyway), which
 > I'll be sending out separately.

 So, here it is.

 http.c | 6 ++
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/http.c b/http.c
index 43e75ac583..2214100e3b 100644
--- a/http.c
+++ b/http.c
@@ -834,8 +834,7 @@ static CURL *get_curl_handle(void)
 #if LIBCURL_VERSION_NUM >= 0x072c00
curl_easy_setopt(result, CURLOPT_SSL_OPTIONS, 
CURLSSLOPT_NO_REVOKE);
 #else
-   warning("CURLSSLOPT_NO_REVOKE not applied to curl SSL options 
because\n"
-   "your curl version is too old (< 7.44.0)");
+   warning(_("CURLSSLOPT_NO_REVOKE not suported with cURL < 
7.44.0"));
 #endif
}
 
@@ -908,8 +907,7 @@ static CURL *get_curl_handle(void)
curl_easy_setopt(result, CURLOPT_PROTOCOLS,
 get_curl_allowed_protocols(-1));
 #else
-   warning("protocol restrictions not applied to curl redirects because\n"
-   "your curl version is too old (>= 7.19.4)");
+   warning(_("Protocol restrictions not supported with cURL < 7.19.4"));
 #endif
if (getenv("GIT_CURL_VERBOSE"))
curl_easy_setopt(result, CURLOPT_VERBOSE, 1L);
-- 
2.19.1-542-gc4df23f792

Re: [PATCH 2/3] http: add support for disabling SSL revocation checks in cURL

Eric Sunshine  writes:

> On Mon, Oct 15, 2018 at 6:14 AM Brendan Forster via GitGitGadget
>  wrote:
>> This config value is only used if http.sslBackend is set to "schannel",
>> which forces cURL to use the Windows Certificate Store when validating
>> server certificates associated with a remote server.
>>
>> This is only supported in cURL 7.44 or later.
>> [...]
>> Signed-off-by: Brendan Forster 
>> ---
>> diff --git a/http.c b/http.c
>> @@ -811,6 +818,16 @@ static CURL *get_curl_handle(void)
>> +   if (http_ssl_backend && !strcmp("schannel", http_ssl_backend) &&
>> +   !http_schannel_check_revoke) {
>> +#if LIBCURL_VERSION_NUM >= 0x072c00
>> +   curl_easy_setopt(result, CURLOPT_SSL_OPTIONS, 
>> CURLSSLOPT_NO_REVOKE);
>> +#else
>> +   warning("CURLSSLOPT_NO_REVOKE not applied to curl SSL 
>> options because\n"
>> +   "your curl version is too old (>= 7.44.0)");
>
> This message is confusing. If your curl is too old, shouldn't the ">=" be a 
> "<"?

I do not think I saw any update to correct this, and worse yet I do
not offhand recall if there was any other issue raised on the
series.

So assuming that this is the only remaining one, I'll squash the
following to step 2/3 of this three-patch series and plan to merge
it down to 'next' in the coming few days.

I have a clean-up suggestion related to this but is orthogonal to
this three-patch series (after the fix-up is applied, anyway), which
I'll be sending out separately.

Thanks.

 http.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/http.c b/http.c
index 0ebf8f77a6..43e75ac583 100644
--- a/http.c
+++ b/http.c
@@ -835,7 +835,7 @@ static CURL *get_curl_handle(void)
curl_easy_setopt(result, CURLOPT_SSL_OPTIONS, 
CURLSSLOPT_NO_REVOKE);
 #else
warning("CURLSSLOPT_NO_REVOKE not applied to curl SSL options 
because\n"
-   "your curl version is too old (>= 7.44.0)");
+   "your curl version is too old (< 7.44.0)");
 #endif
}
 
-- 
2.19.1-542-gc4df23f792

Re: [PATCH v4 10/12] Add a base implementation of SHA-256 support

2018-10-24 Thread Carlo Arenas

On Wed, Oct 24, 2018 at 7:41 PM brian m. carlson
 wrote:
> diff --git a/sha256/block/sha256.h b/sha256/block/sha256.h
> new file mode 100644
> index 00..38f02f7e6c
> --- /dev/null
> +++ b/sha256/block/sha256.h
> @@ -0,0 +1,26 @@
> +#ifndef SHA256_BLOCK_SHA256_H
> +#define SHA256_BLOCK_SHA256_H
> +
> +#include "git-compat-util.h"

this shouldn't be needed and might be discouraged as per the
instructions in Documentation/CodingGuidelines

Carlo

[PATCH v4 09/12] commit-graph: convert to using the_hash_algo

Instead of using hard-coded constants for object sizes, use
the_hash_algo to look them up.  In addition, use a function call to look
up the object ID version and produce the correct value.  For now, we use
version 1, which means to use the default algorithm used in the rest of
the repository.

Signed-off-by: brian m. carlson 
---
 commit-graph.c | 33 +
 1 file changed, 17 insertions(+), 16 deletions(-)

diff --git a/commit-graph.c b/commit-graph.c
index 40c855f185..6763d19288 100644
--- a/commit-graph.c
+++ b/commit-graph.c
@@ -23,16 +23,11 @@
 #define GRAPH_CHUNKID_DATA 0x43444154 /* "CDAT" */
 #define GRAPH_CHUNKID_LARGEEDGES 0x45444745 /* "EDGE" */
 
-#define GRAPH_DATA_WIDTH 36
+#define GRAPH_DATA_WIDTH (the_hash_algo->rawsz + 16)
 
 #define GRAPH_VERSION_1 0x1
 #define GRAPH_VERSION GRAPH_VERSION_1
 
-#define GRAPH_OID_VERSION_SHA1 1
-#define GRAPH_OID_LEN_SHA1 GIT_SHA1_RAWSZ
-#define GRAPH_OID_VERSION GRAPH_OID_VERSION_SHA1
-#define GRAPH_OID_LEN GRAPH_OID_LEN_SHA1
-
 #define GRAPH_OCTOPUS_EDGES_NEEDED 0x8000
 #define GRAPH_PARENT_MISSING 0x7fff
 #define GRAPH_EDGE_LAST_MASK 0x7fff
@@ -44,13 +39,18 @@
 #define GRAPH_FANOUT_SIZE (4 * 256)
 #define GRAPH_CHUNKLOOKUP_WIDTH 12
 #define GRAPH_MIN_SIZE (GRAPH_HEADER_SIZE + 4 * GRAPH_CHUNKLOOKUP_WIDTH \
-   + GRAPH_FANOUT_SIZE + GRAPH_OID_LEN)
+   + GRAPH_FANOUT_SIZE + the_hash_algo->rawsz)
 
 char *get_commit_graph_filename(const char *obj_dir)
 {
return xstrfmt("%s/info/commit-graph", obj_dir);
 }
 
+static uint8_t oid_version(void)
+{
+   return 1;
+}
+
 static struct commit_graph *alloc_commit_graph(void)
 {
struct commit_graph *g = xcalloc(1, sizeof(*g));
@@ -125,15 +125,15 @@ struct commit_graph *load_commit_graph_one(const char 
*graph_file)
}
 
hash_version = *(unsigned char*)(data + 5);
-   if (hash_version != GRAPH_OID_VERSION) {
+   if (hash_version != oid_version()) {
error(_("hash version %X does not match version %X"),
- hash_version, GRAPH_OID_VERSION);
+ hash_version, oid_version());
goto cleanup_fail;
}
 
graph = alloc_commit_graph();
 
-   graph->hash_len = GRAPH_OID_LEN;
+   graph->hash_len = the_hash_algo->rawsz;
graph->num_chunks = *(unsigned char*)(data + 6);
graph->graph_fd = fd;
graph->data = graph_map;
@@ -149,7 +149,7 @@ struct commit_graph *load_commit_graph_one(const char 
*graph_file)
 
chunk_lookup += GRAPH_CHUNKLOOKUP_WIDTH;
 
-   if (chunk_offset > graph_size - GIT_MAX_RAWSZ) {
+   if (chunk_offset > graph_size - the_hash_algo->rawsz) {
error(_("improper chunk offset %08x%08x"), 
(uint32_t)(chunk_offset >> 32),
  (uint32_t)chunk_offset);
goto cleanup_fail;
@@ -764,6 +764,7 @@ void write_commit_graph(const char *obj_dir,
int num_extra_edges;
struct commit_list *parent;
struct progress *progress = NULL;
+   const unsigned hashsz = the_hash_algo->rawsz;
 
if (!commit_graph_compatible(the_repository))
return;
@@ -909,7 +910,7 @@ void write_commit_graph(const char *obj_dir,
hashwrite_be32(f, GRAPH_SIGNATURE);
 
hashwrite_u8(f, GRAPH_VERSION);
-   hashwrite_u8(f, GRAPH_OID_VERSION);
+   hashwrite_u8(f, oid_version());
hashwrite_u8(f, num_chunks);
hashwrite_u8(f, 0); /* unused padding byte */
 
@@ -924,8 +925,8 @@ void write_commit_graph(const char *obj_dir,
 
chunk_offsets[0] = 8 + (num_chunks + 1) * GRAPH_CHUNKLOOKUP_WIDTH;
chunk_offsets[1] = chunk_offsets[0] + GRAPH_FANOUT_SIZE;
-   chunk_offsets[2] = chunk_offsets[1] + GRAPH_OID_LEN * commits.nr;
-   chunk_offsets[3] = chunk_offsets[2] + (GRAPH_OID_LEN + 16) * commits.nr;
+   chunk_offsets[2] = chunk_offsets[1] + hashsz * commits.nr;
+   chunk_offsets[3] = chunk_offsets[2] + (hashsz + 16) * commits.nr;
chunk_offsets[4] = chunk_offsets[3] + 4 * num_extra_edges;
 
for (i = 0; i <= num_chunks; i++) {
@@ -938,8 +939,8 @@ void write_commit_graph(const char *obj_dir,
}
 
write_graph_chunk_fanout(f, commits.list, commits.nr);
-   write_graph_chunk_oids(f, GRAPH_OID_LEN, commits.list, commits.nr);
-   write_graph_chunk_data(f, GRAPH_OID_LEN, commits.list, commits.nr);
+   write_graph_chunk_oids(f, hashsz, commits.list, commits.nr);
+   write_graph_chunk_data(f, hashsz, commits.list, commits.nr);
write_graph_chunk_large_edges(f, commits.list, commits.nr);
 
close_commit_graph(the_repository);

[PATCH v4 01/12] sha1-file: rename algorithm to "sha1"

The transition plan anticipates us using a syntax such as "^{sha1}" for
disambiguation.  Since this is a syntax some people will be typing a
lot, it makes sense to provide a short, easy-to-type syntax.  Omitting
the dash doesn't create any ambiguity; however, it does make the syntax
shorter and easier to type, especially for touch typists.  In addition,
the transition plan already uses "sha1" in this context.

Rename the name of SHA-1 implementation to "sha1".

Note that this change creates no backwards compatibility concerns, since
we haven't yet used this field in any configuration settings.

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

diff --git a/sha1-file.c b/sha1-file.c
index dd0b6aa873..91311ebb3d 100644
--- a/sha1-file.c
+++ b/sha1-file.c
@@ -97,7 +97,7 @@ const struct git_hash_algo hash_algos[GIT_HASH_NALGOS] = {
NULL,
},
{
-   "sha-1",
+   "sha1",
/* "sha1", big-endian */
0x73686131,
GIT_SHA1_RAWSZ,

[PATCH v4 11/12] sha256: add an SHA-256 implementation using libgcrypt

Generally, one gets better performance out of cryptographic routines
written in assembly than C, and this is also true for SHA-256.  In
addition, most Linux distributions cannot distribute Git linked against
OpenSSL for licensing reasons.

Most systems with GnuPG will also have libgcrypt, since it is a
dependency of GnuPG.  libgcrypt is also faster than the SHA1DC
implementation for messages of a few KiB and larger.

For comparison, on a Core i7-6600U, this implementation processes 16 KiB
chunks at 355 MiB/s while SHA1DC processes equivalent chunks at 337
MiB/s.

In addition, libgcrypt is licensed under the LGPL 2.1, which is
compatible with the GPL.  Add an implementation of SHA-256 that uses
libgcrypt.

Signed-off-by: brian m. carlson 
---
 Makefile| 13 +++--
 hash.h  |  4 
 sha256/gcrypt.h | 30 ++
 3 files changed, 45 insertions(+), 2 deletions(-)
 create mode 100644 sha256/gcrypt.h

diff --git a/Makefile b/Makefile
index e99b7712f6..5a07e03100 100644
--- a/Makefile
+++ b/Makefile
@@ -179,6 +179,10 @@ all::
 # in one call to the platform's SHA1_Update(). e.g. APPLE_COMMON_CRYPTO
 # wants 'SHA1_MAX_BLOCK_SIZE=1024L*1024L*1024L' defined.
 #
+# Define BLK_SHA256 to use the built-in SHA-256 routines.
+#
+# Define GCRYPT_SHA256 to use the SHA-256 routines in libgcrypt.
+#
 # Define NEEDS_CRYPTO_WITH_SSL if you need -lcrypto when using -lssl (Darwin).
 #
 # Define NEEDS_SSL_WITH_CRYPTO if you need -lssl when using -lcrypto (Darwin).
@@ -1634,8 +1638,13 @@ endif
 endif
 endif
 
-LIB_OBJS += sha256/block/sha256.o
-BASIC_CFLAGS += -DSHA256_BLK
+ifdef GCRYPT_SHA256
+   BASIC_CFLAGS += -DSHA256_GCRYPT
+   EXTLIBS += -lgcrypt
+else
+   LIB_OBJS += sha256/block/sha256.o
+   BASIC_CFLAGS += -DSHA256_BLK
+endif
 
 ifdef SHA1_MAX_BLOCK_SIZE
LIB_OBJS += compat/sha1-chunked.o
diff --git a/hash.h b/hash.h
index a9bc624020..2ef098052d 100644
--- a/hash.h
+++ b/hash.h
@@ -15,7 +15,11 @@
 #include "block-sha1/sha1.h"
 #endif
 
+#if defined(SHA256_GCRYPT)
+#include "sha256/gcrypt.h"
+#else
 #include "sha256/block/sha256.h"
+#endif
 
 #ifndef platform_SHA_CTX
 /*
diff --git a/sha256/gcrypt.h b/sha256/gcrypt.h
new file mode 100644
index 00..09bd8bb200
--- /dev/null
+++ b/sha256/gcrypt.h
@@ -0,0 +1,30 @@
+#ifndef SHA256_GCRYPT_H
+#define SHA256_GCRYPT_H
+
+#include 
+
+#define SHA256_DIGEST_SIZE 32
+
+typedef gcry_md_hd_t gcrypt_SHA256_CTX;
+
+inline void gcrypt_SHA256_Init(gcrypt_SHA256_CTX *ctx)
+{
+   gcry_md_open(ctx, GCRY_MD_SHA256, 0);
+}
+
+inline void gcrypt_SHA256_Update(gcrypt_SHA256_CTX *ctx, const void *data, 
size_t len)
+{
+   gcry_md_write(*ctx, data, len);
+}
+
+inline void gcrypt_SHA256_Final(unsigned char *digest, gcrypt_SHA256_CTX *ctx)
+{
+   memcpy(digest, gcry_md_read(*ctx, GCRY_MD_SHA256), SHA256_DIGEST_SIZE);
+}
+
+#define platform_SHA256_CTX gcrypt_SHA256_CTX
+#define platform_SHA256_Init gcrypt_SHA256_Init
+#define platform_SHA256_Update gcrypt_SHA256_Update
+#define platform_SHA256_Final gcrypt_SHA256_Final
+
+#endif

[PATCH v4 08/12] t/helper: add a test helper to compute hash speed

Add a utility (which is less for the testsuite and more for developers)
that can compute hash speeds for whatever hash algorithms are
implemented.  This allows developers to test their personal systems to
determine the performance characteristics of various algorithms.

Signed-off-by: brian m. carlson 
---
 Makefile   |  1 +
 t/helper/test-hash-speed.c | 61 ++
 t/helper/test-tool.c   |  1 +
 t/helper/test-tool.h   |  1 +
 4 files changed, 64 insertions(+)
 create mode 100644 t/helper/test-hash-speed.c

diff --git a/Makefile b/Makefile
index 81dc9ac819..68169a7abb 100644
--- a/Makefile
+++ b/Makefile
@@ -716,6 +716,7 @@ TEST_BUILTINS_OBJS += test-example-decorate.o
 TEST_BUILTINS_OBJS += test-genrandom.o
 TEST_BUILTINS_OBJS += test-hash.o
 TEST_BUILTINS_OBJS += test-hashmap.o
+TEST_BUILTINS_OBJS += test-hash-speed.o
 TEST_BUILTINS_OBJS += test-index-version.o
 TEST_BUILTINS_OBJS += test-json-writer.o
 TEST_BUILTINS_OBJS += test-lazy-init-name-hash.o
diff --git a/t/helper/test-hash-speed.c b/t/helper/test-hash-speed.c
new file mode 100644
index 00..432233c7f0
--- /dev/null
+++ b/t/helper/test-hash-speed.c
@@ -0,0 +1,61 @@
+#include "test-tool.h"
+#include "cache.h"
+
+#define NUM_SECONDS 3
+
+static inline void compute_hash(const struct git_hash_algo *algo, git_hash_ctx 
*ctx, uint8_t *final, const void *p, size_t len)
+{
+   algo->init_fn(ctx);
+   algo->update_fn(ctx, p, len);
+   algo->final_fn(final, ctx);
+}
+
+int cmd__hash_speed(int ac, const char **av)
+{
+   git_hash_ctx ctx;
+   unsigned char hash[GIT_MAX_RAWSZ];
+   clock_t initial, start, end;
+   unsigned bufsizes[] = { 64, 256, 1024, 8192, 16384 };
+   int i;
+   void *p;
+   const struct git_hash_algo *algo = NULL;
+
+   if (ac == 2) {
+   for (i = 1; i < GIT_HASH_NALGOS; i++) {
+   if (!strcmp(av[1], hash_algos[i].name)) {
+   algo = _algos[i];
+   break;
+   }
+   }
+   }
+   if (!algo)
+   die("usage: test-tool hash-speed algo_name");
+
+   /* Use this as an offset to make overflow less likely. */
+   initial = clock();
+
+   printf("algo: %s\n", algo->name);
+
+   for (i = 0; i < ARRAY_SIZE(bufsizes); i++) {
+   unsigned long j, kb;
+   double kb_per_sec;
+   p = xcalloc(1, bufsizes[i]);
+   start = end = clock() - initial;
+   for (j = 0; ((end - start) / CLOCKS_PER_SEC) < NUM_SECONDS; 
j++) {
+   compute_hash(algo, , hash, p, bufsizes[i]);
+
+   /*
+* Only check elapsed time every 128 iterations to avoid
+* dominating the runtime with system calls.
+*/
+   if (!(j & 127))
+   end = clock() - initial;
+   }
+   kb = j * bufsizes[i];
+   kb_per_sec = kb / (1024 * ((double)end - start) / 
CLOCKS_PER_SEC);
+   printf("size %u: %lu iters; %lu KiB; %0.2f KiB/s\n", 
bufsizes[i], j, kb, kb_per_sec);
+   free(p);
+   }
+
+   exit(0);
+}
diff --git a/t/helper/test-tool.c b/t/helper/test-tool.c
index 6b5836dc1b..e009c8186d 100644
--- a/t/helper/test-tool.c
+++ b/t/helper/test-tool.c
@@ -20,6 +20,7 @@ static struct test_cmd cmds[] = {
{ "example-decorate", cmd__example_decorate },
{ "genrandom", cmd__genrandom },
{ "hashmap", cmd__hashmap },
+   { "hash-speed", cmd__hash_speed },
{ "index-version", cmd__index_version },
{ "json-writer", cmd__json_writer },
{ "lazy-init-name-hash", cmd__lazy_init_name_hash },
diff --git a/t/helper/test-tool.h b/t/helper/test-tool.h
index 29ac7b0b0d..19a7e8332a 100644
--- a/t/helper/test-tool.h
+++ b/t/helper/test-tool.h
@@ -16,6 +16,7 @@ int cmd__dump_untracked_cache(int argc, const char **argv);
 int cmd__example_decorate(int argc, const char **argv);
 int cmd__genrandom(int argc, const char **argv);
 int cmd__hashmap(int argc, const char **argv);
+int cmd__hash_speed(int argc, const char **argv);
 int cmd__index_version(int argc, const char **argv);
 int cmd__json_writer(int argc, const char **argv);
 int cmd__lazy_init_name_hash(int argc, const char **argv);

[PATCH v4 00/12] Base SHA-256 implementation

This series provides a functional SHA-256 implementation and wires it
up, along with some housekeeping patches to make it suitable for
testing.

While I was fixing the macros, I wondered if I could make the code a bit
cleaner by using inline functions.  I tried it and found that not only
did it make the code cleaner, it made the code significantly faster
across all sizes of output, with larger gains on larger chunks (e.g.,
214 MiB/s on 16 KiB chunks vs 151 MiB/s).  I'm unsure why this effect
occurs, but I figured nobody would complain about improved performance.

Changes from v3:
* Switch to using inline functions instead of macros in many cases.
* Undefine remaining macros at the top.

Changes from v2:
* Improve commit messages to include timing and performance information.
* Improve commit messages to be less ambiguous and more friendly to a
  wider variety of English speakers.
* Prefer functions taking struct git_hash_algo in hex.c.
* Port pieces of the block-sha1 implementation over to the block-sha256
  implementation for better compatibility.
* Drop patch 13 in favor of further discussion about the best way
  forward for versioning commit graph.
* Rename the test so as to have a different number from other tests.
* Rebase on master.

Changes from v1:
* Add a hash_to_hex function mirroring sha1_to_hex, but for
  the_hash_algo.
* Strip commit message explanation about why we chose SHA-256.
* Rebase on master
* Strip leading whitespace from commit message.
* Improve commit-graph patch to cover new code added since v1.
* Be more honest about the scope of work involved in porting the SHA-256
  implementation out of libtomcrypt.
* Revert change to limit hashcmp to 20 bytes.

brian m. carlson (12):
  sha1-file: rename algorithm to "sha1"
  sha1-file: provide functions to look up hash algorithms
  hex: introduce functions to print arbitrary hashes
  cache: make hashcmp and hasheq work with larger hashes
  t: add basic tests for our SHA-1 implementation
  t: make the sha1 test-tool helper generic
  sha1-file: add a constant for hash block size
  t/helper: add a test helper to compute hash speed
  commit-graph: convert to using the_hash_algo
  Add a base implementation of SHA-256 support
  sha256: add an SHA-256 implementation using libgcrypt
  hash: add an SHA-256 implementation using OpenSSL

 Makefile  |  22 +++
 cache.h   |  51 ---
 commit-graph.c|  33 +++--
 hash.h|  41 +-
 hex.c |  32 +++-
 sha1-file.c   |  70 -
 sha256/block/sha256.c | 201 ++
 sha256/block/sha256.h |  26 
 sha256/gcrypt.h   |  30 
 t/helper/test-hash-speed.c|  61 
 t/helper/{test-sha1.c => test-hash.c} |  19 +--
 t/helper/test-sha1.c  |  52 +--
 t/helper/test-sha256.c|   7 +
 t/helper/test-tool.c  |   2 +
 t/helper/test-tool.h  |   4 +
 t/t0015-hash.sh   |  54 +++
 16 files changed, 601 insertions(+), 104 deletions(-)
 create mode 100644 sha256/block/sha256.c
 create mode 100644 sha256/block/sha256.h
 create mode 100644 sha256/gcrypt.h
 create mode 100644 t/helper/test-hash-speed.c
 copy t/helper/{test-sha1.c => test-hash.c} (65%)
 create mode 100644 t/helper/test-sha256.c
 create mode 100755 t/t0015-hash.sh

Range-diff against v3:
 1:  a004a4c982 <  -:  -- :hash-impl
 2:  cf9f7f5620 =  1:  cf9f7f5620 sha1-file: rename algorithm to "sha1"
 3:  0144deaebe =  2:  0144deaebe sha1-file: provide functions to look up hash 
algorithms
 4:  b74858fb03 =  3:  b74858fb03 hex: introduce functions to print arbitrary 
hashes
 5:  e9703017a4 =  4:  e9703017a4 cache: make hashcmp and hasheq work with 
larger hashes
 6:  ab85a834fd =  5:  ab85a834fd t: add basic tests for our SHA-1 
implementation
 7:  962f6d8903 =  6:  962f6d8903 t: make the sha1 test-tool helper generic
 8:  53addf4d58 =  7:  53addf4d58 sha1-file: add a constant for hash block size
 9:  9ace10faa2 =  8:  9ace10faa2 t/helper: add a test helper to compute hash 
speed
10:  9adc56d01e =  9:  9adc56d01e commit-graph: convert to using the_hash_algo
11:  8e82cb0dfb ! 10:  f48cb1ad27 Add a base implementation of SHA-256 support
@@ -207,6 +207,9 @@
 +#include "git-compat-util.h"
 +#include "./sha256.h"
 +
++#undef RND
++#undef BLKSIZE
++
 +#define BLKSIZE blk_SHA256_BLKSIZE
 +
 +void blk_SHA256_Init(blk_SHA256_CTX *ctx)
@@ -228,14 +231,35 @@
 +  return (x >> n) | (x << (32 - n));
 +}
 +
-+#define Ch(x,y,z)   (z ^ (x & (y ^ z)))
-+#define Maj(x,y,z)  (((x | y) & z) | (x & y))
-+#define S(x, n) ror((x),(n))
-+#define R(x, n) ((x)>>(n))
-+#define Sigma0(x)   (S(x, 2) ^ S(x, 13) ^ S(x, 22))
-+#define

[PATCH v4 10/12] Add a base implementation of SHA-256 support

SHA-1 is weak and we need to transition to a new hash function.  For
some time, we have referred to this new function as NewHash.  Recently,
we decided to pick SHA-256 as NewHash.

Add a basic implementation of SHA-256 based off libtomcrypt, which is in
the public domain.  Optimize it and restructure it to meet our coding
standards.  Pull in the update and final functions from the SHA-1 block
implementation, as we know these function correctly with all compilers.
This implementation is slower than SHA-1, but more performant
implementations will be introduced in future commits.

Wire up SHA-256 in the list of hash algorithms, and add a test that the
algorithm works correctly.

Note that with this patch, it is still not possible to switch to using
SHA-256 in Git.  Additional patches are needed to prepare the code to
handle a larger hash algorithm and further test fixes are needed.

Signed-off-by: brian m. carlson 
---
 Makefile   |   4 +
 cache.h|  12 ++-
 hash.h |  19 +++-
 sha1-file.c|  45 +
 sha256/block/sha256.c  | 201 +
 sha256/block/sha256.h  |  26 ++
 t/helper/test-sha256.c |   7 ++
 t/helper/test-tool.c   |   1 +
 t/helper/test-tool.h   |   1 +
 t/t0015-hash.sh|  25 +
 10 files changed, 337 insertions(+), 4 deletions(-)
 create mode 100644 sha256/block/sha256.c
 create mode 100644 sha256/block/sha256.h
 create mode 100644 t/helper/test-sha256.c

diff --git a/Makefile b/Makefile
index 68169a7abb..e99b7712f6 100644
--- a/Makefile
+++ b/Makefile
@@ -739,6 +739,7 @@ TEST_BUILTINS_OBJS += test-run-command.o
 TEST_BUILTINS_OBJS += test-scrap-cache-tree.o
 TEST_BUILTINS_OBJS += test-sha1.o
 TEST_BUILTINS_OBJS += test-sha1-array.o
+TEST_BUILTINS_OBJS += test-sha256.o
 TEST_BUILTINS_OBJS += test-sigchain.o
 TEST_BUILTINS_OBJS += test-strcmp-offset.o
 TEST_BUILTINS_OBJS += test-string-list.o
@@ -1633,6 +1634,9 @@ endif
 endif
 endif
 
+LIB_OBJS += sha256/block/sha256.o
+BASIC_CFLAGS += -DSHA256_BLK
+
 ifdef SHA1_MAX_BLOCK_SIZE
LIB_OBJS += compat/sha1-chunked.o
BASIC_CFLAGS += -DSHA1_MAX_BLOCK_SIZE="$(SHA1_MAX_BLOCK_SIZE)"
diff --git a/cache.h b/cache.h
index 9e5d1dd85a..48ce1565e6 100644
--- a/cache.h
+++ b/cache.h
@@ -48,11 +48,17 @@ unsigned long git_deflate_bound(git_zstream *, unsigned 
long);
 /* The block size of SHA-1. */
 #define GIT_SHA1_BLKSZ 64
 
+/* The length in bytes and in hex digits of an object name (SHA-256 value). */
+#define GIT_SHA256_RAWSZ 32
+#define GIT_SHA256_HEXSZ (2 * GIT_SHA256_RAWSZ)
+/* The block size of SHA-256. */
+#define GIT_SHA256_BLKSZ 64
+
 /* The length in byte and in hex digits of the largest possible hash value. */
-#define GIT_MAX_RAWSZ GIT_SHA1_RAWSZ
-#define GIT_MAX_HEXSZ GIT_SHA1_HEXSZ
+#define GIT_MAX_RAWSZ GIT_SHA256_RAWSZ
+#define GIT_MAX_HEXSZ GIT_SHA256_HEXSZ
 /* The largest possible block size for any supported hash. */
-#define GIT_MAX_BLKSZ GIT_SHA1_BLKSZ
+#define GIT_MAX_BLKSZ GIT_SHA256_BLKSZ
 
 struct object_id {
unsigned char hash[GIT_MAX_RAWSZ];
diff --git a/hash.h b/hash.h
index 1bcf7ab6fd..a9bc624020 100644
--- a/hash.h
+++ b/hash.h
@@ -15,6 +15,8 @@
 #include "block-sha1/sha1.h"
 #endif
 
+#include "sha256/block/sha256.h"
+
 #ifndef platform_SHA_CTX
 /*
  * platform's underlying implementation of SHA-1; could be OpenSSL,
@@ -34,6 +36,18 @@
 #define git_SHA1_Updateplatform_SHA1_Update
 #define git_SHA1_Final platform_SHA1_Final
 
+#ifndef platform_SHA256_CTX
+#define platform_SHA256_CTXSHA256_CTX
+#define platform_SHA256_Init   SHA256_Init
+#define platform_SHA256_Update SHA256_Update
+#define platform_SHA256_Final  SHA256_Final
+#endif
+
+#define git_SHA256_CTX platform_SHA256_CTX
+#define git_SHA256_Initplatform_SHA256_Init
+#define git_SHA256_Update  platform_SHA256_Update
+#define git_SHA256_Final   platform_SHA256_Final
+
 #ifdef SHA1_MAX_BLOCK_SIZE
 #include "compat/sha1-chunked.h"
 #undef git_SHA1_Update
@@ -52,12 +66,15 @@
 #define GIT_HASH_UNKNOWN 0
 /* SHA-1 */
 #define GIT_HASH_SHA1 1
+/* SHA-256  */
+#define GIT_HASH_SHA256 2
 /* Number of algorithms supported (including unknown). */
-#define GIT_HASH_NALGOS (GIT_HASH_SHA1 + 1)
+#define GIT_HASH_NALGOS (GIT_HASH_SHA256 + 1)
 
 /* A suitably aligned type for stack allocations of hash contexts. */
 union git_hash_ctx {
git_SHA_CTX sha1;
+   git_SHA256_CTX sha256;
 };
 typedef union git_hash_ctx git_hash_ctx;
 
diff --git a/sha1-file.c b/sha1-file.c
index 9bdd04ea45..c97d93a14a 100644
--- a/sha1-file.c
+++ b/sha1-file.c
@@ -40,10 +40,20 @@
 #define EMPTY_TREE_SHA1_BIN_LITERAL \
 "\x4b\x82\x5d\xc6\x42\xcb\x6e\xb9\xa0\x60" \
 "\xe5\x4b\xf8\xd6\x92\x88\xfb\xee\x49\x04"
+#define EMPTY_TREE_SHA256_BIN_LITERAL \
+   "\x6e\xf1\x9b\x41\x22\x5c\x53\x69\xf1\xc1" \
+   "\x04\xd4\x5d\x8d\x85\xef\xa9\xb0\x57\xb5" \
+   "\x3b\x14\xb4\xb9\xb9\x39\xdd\x74\xde\xcc" \
+

[PATCH v4 06/12] t: make the sha1 test-tool helper generic

Since we're going to have multiple hash algorithms to test, it makes
sense to share as much of the test code as possible.  Convert the sha1
helper for the test-tool to be generic and move it out into its own
module.  This will allow us to share most of this code with our NewHash
implementation.

Signed-off-by: brian m. carlson 
---
 Makefile  |  1 +
 t/helper/{test-sha1.c => test-hash.c} | 19 +-
 t/helper/test-sha1.c  | 52 +--
 t/helper/test-tool.h  |  2 ++
 4 files changed, 14 insertions(+), 60 deletions(-)
 copy t/helper/{test-sha1.c => test-hash.c} (65%)

diff --git a/Makefile b/Makefile
index d18ab0fe78..81dc9ac819 100644
--- a/Makefile
+++ b/Makefile
@@ -714,6 +714,7 @@ TEST_BUILTINS_OBJS += test-dump-split-index.o
 TEST_BUILTINS_OBJS += test-dump-untracked-cache.o
 TEST_BUILTINS_OBJS += test-example-decorate.o
 TEST_BUILTINS_OBJS += test-genrandom.o
+TEST_BUILTINS_OBJS += test-hash.o
 TEST_BUILTINS_OBJS += test-hashmap.o
 TEST_BUILTINS_OBJS += test-index-version.o
 TEST_BUILTINS_OBJS += test-json-writer.o
diff --git a/t/helper/test-sha1.c b/t/helper/test-hash.c
similarity index 65%
copy from t/helper/test-sha1.c
copy to t/helper/test-hash.c
index 1ba0675c75..0a31de66f3 100644
--- a/t/helper/test-sha1.c
+++ b/t/helper/test-hash.c
@@ -1,13 +1,14 @@
 #include "test-tool.h"
 #include "cache.h"
 
-int cmd__sha1(int ac, const char **av)
+int cmd_hash_impl(int ac, const char **av, int algo)
 {
-   git_SHA_CTX ctx;
-   unsigned char sha1[20];
+   git_hash_ctx ctx;
+   unsigned char hash[GIT_MAX_HEXSZ];
unsigned bufsz = 8192;
int binary = 0;
char *buffer;
+   const struct git_hash_algo *algop = _algos[algo];
 
if (ac == 2) {
if (!strcmp(av[1], "-b"))
@@ -26,7 +27,7 @@ int cmd__sha1(int ac, const char **av)
die("OOPS");
}
 
-   git_SHA1_Init();
+   algop->init_fn();
 
while (1) {
ssize_t sz, this_sz;
@@ -38,20 +39,20 @@ int cmd__sha1(int ac, const char **av)
if (sz == 0)
break;
if (sz < 0)
-   die_errno("test-sha1");
+   die_errno("test-hash");
this_sz += sz;
cp += sz;
room -= sz;
}
if (this_sz == 0)
break;
-   git_SHA1_Update(, buffer, this_sz);
+   algop->update_fn(, buffer, this_sz);
}
-   git_SHA1_Final(sha1, );
+   algop->final_fn(hash, );
 
if (binary)
-   fwrite(sha1, 1, 20, stdout);
+   fwrite(hash, 1, algop->rawsz, stdout);
else
-   puts(sha1_to_hex(sha1));
+   puts(hash_to_hex_algop(hash, algop));
exit(0);
 }
diff --git a/t/helper/test-sha1.c b/t/helper/test-sha1.c
index 1ba0675c75..d860c387c3 100644
--- a/t/helper/test-sha1.c
+++ b/t/helper/test-sha1.c
@@ -3,55 +3,5 @@
 
 int cmd__sha1(int ac, const char **av)
 {
-   git_SHA_CTX ctx;
-   unsigned char sha1[20];
-   unsigned bufsz = 8192;
-   int binary = 0;
-   char *buffer;
-
-   if (ac == 2) {
-   if (!strcmp(av[1], "-b"))
-   binary = 1;
-   else
-   bufsz = strtoul(av[1], NULL, 10) * 1024 * 1024;
-   }
-
-   if (!bufsz)
-   bufsz = 8192;
-
-   while ((buffer = malloc(bufsz)) == NULL) {
-   fprintf(stderr, "bufsz %u is too big, halving...\n", bufsz);
-   bufsz /= 2;
-   if (bufsz < 1024)
-   die("OOPS");
-   }
-
-   git_SHA1_Init();
-
-   while (1) {
-   ssize_t sz, this_sz;
-   char *cp = buffer;
-   unsigned room = bufsz;
-   this_sz = 0;
-   while (room) {
-   sz = xread(0, cp, room);
-   if (sz == 0)
-   break;
-   if (sz < 0)
-   die_errno("test-sha1");
-   this_sz += sz;
-   cp += sz;
-   room -= sz;
-   }
-   if (this_sz == 0)
-   break;
-   git_SHA1_Update(, buffer, this_sz);
-   }
-   git_SHA1_Final(sha1, );
-
-   if (binary)
-   fwrite(sha1, 1, 20, stdout);
-   else
-   puts(sha1_to_hex(sha1));
-   exit(0);
+   return cmd_hash_impl(ac, av, GIT_HASH_SHA1);
 }
diff --git a/t/helper/test-tool.h b/t/helper/test-tool.h
index e4890566da..29ac7b0b0d 100644
--- a/t/helper/test-tool.h
+++ b/t/helper/test-tool.h
@@ -50,4 +50,6 @@ int cmd__windows_named_pipe(int argc, const char **argv);
 #endif
 int cmd__write_cache(int argc, const char **argv);

[PATCH v4 05/12] t: add basic tests for our SHA-1 implementation

We have in the past had some unfortunate endianness issues with some
SHA-1 implementations we ship, especially on big-endian machines.  Add
an explicit test using the test helper to catch these issues and point
them out prominently.  This test can also be used as a staging ground
for people testing additional algorithms to verify that their
implementations are working as expected.

Signed-off-by: brian m. carlson 
---
 t/t0015-hash.sh | 29 +
 1 file changed, 29 insertions(+)
 create mode 100755 t/t0015-hash.sh

diff --git a/t/t0015-hash.sh b/t/t0015-hash.sh
new file mode 100755
index 00..8e763c2c3d
--- /dev/null
+++ b/t/t0015-hash.sh
@@ -0,0 +1,29 @@
+#!/bin/sh
+
+test_description='test basic hash implementation'
+. ./test-lib.sh
+
+
+test_expect_success 'test basic SHA-1 hash values' '
+   test-tool sha1 actual &&
+   grep da39a3ee5e6b4b0d3255bfef95601890afd80709 actual &&
+   printf "a" | test-tool sha1 >actual &&
+   grep 86f7e437faa5a7fce15d1ddcb9eaeaea377667b8 actual &&
+   printf "abc" | test-tool sha1 >actual &&
+   grep a9993e364706816aba3e25717850c26c9cd0d89d actual &&
+   printf "message digest" | test-tool sha1 >actual &&
+   grep c12252ceda8be8994d5fa0290a47231c1d16aae3 actual &&
+   printf "abcdefghijklmnopqrstuvwxyz" | test-tool sha1 >actual &&
+   grep 32d10c7b8cf96570ca04ce37f2a19d84240d3a89 actual &&
+   perl -E "for (1..10) { print q{aa}; }" | \
+   test-tool sha1 >actual &&
+   grep 34aa973cd4c4daa4f61eeb2bdbad27316534016f actual &&
+   printf "blob 0\0" | test-tool sha1 >actual &&
+   grep e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 actual &&
+   printf "blob 3\0abc" | test-tool sha1 >actual &&
+   grep f2ba8f84ab5c1bce84a7b441cb1959cfc7093b7f actual &&
+   printf "tree 0\0" | test-tool sha1 >actual &&
+   grep 4b825dc642cb6eb9a060e54bf8d69288fbee4904 actual
+'
+
+test_done

[PATCH v4 04/12] cache: make hashcmp and hasheq work with larger hashes

In 183a638b7d ("hashcmp: assert constant hash size", 2018-08-23), we
modified hashcmp to assert that the hash size was always 20 to help it
optimize and inline calls to memcmp.  In a future series, we replaced
many calls to hashcmp and oidcmp with calls to hasheq and oideq to
improve inlining further.

However, we want to support hash algorithms other than SHA-1, namely
SHA-256.  When doing so, we must handle the case where these values are
32 bytes long as well as 20.  Adjust hashcmp to handle two cases:
20-byte matches, and maximum-size matches.  Therefore, when we include
SHA-256, we'll automatically handle it properly, while at the same time
teaching the compiler that there are only two possible options to
consider.  This will allow the compiler to write the most efficient
possible code.

Copy similar code into hasheq and perform an identical transformation.
At least with GCC 8.2.0, making hasheq defer to hashcmp when there are
two branches prevents the compiler from inlining the comparison, while
the code in this patch is inlined properly.  Add a comment to avoid an
accidental performance regression from well-intentioned refactoring.

Signed-off-by: brian m. carlson 
---
 cache.h | 22 --
 1 file changed, 12 insertions(+), 10 deletions(-)

diff --git a/cache.h b/cache.h
index 51580c4b77..bab8e8964f 100644
--- a/cache.h
+++ b/cache.h
@@ -1027,16 +1027,12 @@ extern const struct object_id null_oid;
 static inline int hashcmp(const unsigned char *sha1, const unsigned char *sha2)
 {
/*
-* This is a temporary optimization hack. By asserting the size here,
-* we let the compiler know that it's always going to be 20, which lets
-* it turn this fixed-size memcmp into a few inline instructions.
-*
-* This will need to be extended or ripped out when we learn about
-* hashes of different sizes.
+* Teach the compiler that there are only two possibilities of hash size
+* here, so that it can optimize for this case as much as possible.
 */
-   if (the_hash_algo->rawsz != 20)
-   BUG("hash size not yet supported by hashcmp");
-   return memcmp(sha1, sha2, the_hash_algo->rawsz);
+   if (the_hash_algo->rawsz == GIT_MAX_RAWSZ)
+   return memcmp(sha1, sha2, GIT_MAX_RAWSZ);
+   return memcmp(sha1, sha2, GIT_SHA1_RAWSZ);
 }
 
 static inline int oidcmp(const struct object_id *oid1, const struct object_id 
*oid2)
@@ -1046,7 +1042,13 @@ static inline int oidcmp(const struct object_id *oid1, 
const struct object_id *o
 
 static inline int hasheq(const unsigned char *sha1, const unsigned char *sha2)
 {
-   return !hashcmp(sha1, sha2);
+   /*
+* We write this here instead of deferring to hashcmp so that the
+* compiler can properly inline it and avoid calling memcmp.
+*/
+   if (the_hash_algo->rawsz == GIT_MAX_RAWSZ)
+   return !memcmp(sha1, sha2, GIT_MAX_RAWSZ);
+   return !memcmp(sha1, sha2, GIT_SHA1_RAWSZ);
 }
 
 static inline int oideq(const struct object_id *oid1, const struct object_id 
*oid2)

[PATCH v4 02/12] sha1-file: provide functions to look up hash algorithms

There are several ways we might refer to a hash algorithm: by name, such
as in the config file; by format ID, such as in a pack; or internally,
by a pointer to the hash_algos array.  Provide functions to look up hash
algorithms based on these various forms and return the internal constant
used for them.  If conversion to another form is necessary, this
internal constant can be used to look up the proper data in the
hash_algos array.

Signed-off-by: brian m. carlson 
---
 hash.h  | 13 +
 sha1-file.c | 21 +
 2 files changed, 34 insertions(+)

diff --git a/hash.h b/hash.h
index 7c8238bc2e..80881eea47 100644
--- a/hash.h
+++ b/hash.h
@@ -98,4 +98,17 @@ struct git_hash_algo {
 };
 extern const struct git_hash_algo hash_algos[GIT_HASH_NALGOS];
 
+/*
+ * Return a GIT_HASH_* constant based on the name.  Returns GIT_HASH_UNKNOWN if
+ * the name doesn't match a known algorithm.
+ */
+int hash_algo_by_name(const char *name);
+/* Identical, except based on the format ID. */
+int hash_algo_by_id(uint32_t format_id);
+/* Identical, except for a pointer to struct git_hash_algo. */
+static inline int hash_algo_by_ptr(const struct git_hash_algo *p)
+{
+   return p - hash_algos;
+}
+
 #endif
diff --git a/sha1-file.c b/sha1-file.c
index 91311ebb3d..7e9dedc744 100644
--- a/sha1-file.c
+++ b/sha1-file.c
@@ -122,6 +122,27 @@ const char *empty_blob_oid_hex(void)
return oid_to_hex_r(buf, the_hash_algo->empty_blob);
 }
 
+int hash_algo_by_name(const char *name)
+{
+   int i;
+   if (!name)
+   return GIT_HASH_UNKNOWN;
+   for (i = 1; i < GIT_HASH_NALGOS; i++)
+   if (!strcmp(name, hash_algos[i].name))
+   return i;
+   return GIT_HASH_UNKNOWN;
+}
+
+int hash_algo_by_id(uint32_t format_id)
+{
+   int i;
+   for (i = 1; i < GIT_HASH_NALGOS; i++)
+   if (format_id == hash_algos[i].format_id)
+   return i;
+   return GIT_HASH_UNKNOWN;
+}
+
+
 /*
  * This is meant to hold a *small* number of objects that you would
  * want read_sha1_file() to be able to return, but yet you do not want

[PATCH v4 03/12] hex: introduce functions to print arbitrary hashes

Currently, we have functions that turn an arbitrary SHA-1 value or an
object ID into hex format, either using a static buffer or with a
user-provided buffer.  Add variants of these functions that can handle
an arbitrary hash algorithm, specified by constant.  Update the
documentation as well.

While we're at it, remove the "extern" declaration from this family of
functions, since it's not needed and our style now recommends against
it.

We use the variant taking the algorithm structure pointer as the
internal variant, since taking an algorithm pointer is the easiest way
to handle all of the variants in use.

Note that we maintain these functions because there are hashes which
must change based on the hash algorithm in use but are not object IDs
(such as pack checksums).

Signed-off-by: brian m. carlson 
---
 cache.h | 15 +--
 hex.c   | 32 
 2 files changed, 33 insertions(+), 14 deletions(-)

diff --git a/cache.h b/cache.h
index 59c8a93046..51580c4b77 100644
--- a/cache.h
+++ b/cache.h
@@ -1364,9 +1364,9 @@ extern int get_oid_hex(const char *hex, struct object_id 
*sha1);
 extern int hex_to_bytes(unsigned char *binary, const char *hex, size_t len);
 
 /*
- * Convert a binary sha1 to its hex equivalent. The `_r` variant is reentrant,
+ * Convert a binary hash to its hex equivalent. The `_r` variant is reentrant,
  * and writes the NUL-terminated output to the buffer `out`, which must be at
- * least `GIT_SHA1_HEXSZ + 1` bytes, and returns a pointer to out for
+ * least `GIT_MAX_HEXSZ + 1` bytes, and returns a pointer to out for
  * convenience.
  *
  * The non-`_r` variant returns a static buffer, but uses a ring of 4
@@ -1374,10 +1374,13 @@ extern int hex_to_bytes(unsigned char *binary, const 
char *hex, size_t len);
  *
  *   printf("%s -> %s", sha1_to_hex(one), sha1_to_hex(two));
  */
-extern char *sha1_to_hex_r(char *out, const unsigned char *sha1);
-extern char *oid_to_hex_r(char *out, const struct object_id *oid);
-extern char *sha1_to_hex(const unsigned char *sha1);   /* static buffer 
result! */
-extern char *oid_to_hex(const struct object_id *oid);  /* same static buffer 
as sha1_to_hex */
+char *hash_to_hex_algop_r(char *buffer, const unsigned char *hash, const 
struct git_hash_algo *);
+char *sha1_to_hex_r(char *out, const unsigned char *sha1);
+char *oid_to_hex_r(char *out, const struct object_id *oid);
+char *hash_to_hex_algop(const unsigned char *hash, const struct git_hash_algo 
*);  /* static buffer result! */
+char *sha1_to_hex(const unsigned char *sha1);  
/* same static buffer */
+char *hash_to_hex(const unsigned char *hash);  
/* same static buffer */
+char *oid_to_hex(const struct object_id *oid); 
/* same static buffer */
 
 /*
  * Parse a 40-character hexadecimal object ID starting from hex, updating the
diff --git a/hex.c b/hex.c
index 10af1a29e8..d2e8bb9540 100644
--- a/hex.c
+++ b/hex.c
@@ -73,14 +73,15 @@ int parse_oid_hex(const char *hex, struct object_id *oid, 
const char **end)
return ret;
 }
 
-char *sha1_to_hex_r(char *buffer, const unsigned char *sha1)
+inline char *hash_to_hex_algop_r(char *buffer, const unsigned char *hash,
+   const struct git_hash_algo *algop)
 {
static const char hex[] = "0123456789abcdef";
char *buf = buffer;
int i;
 
-   for (i = 0; i < the_hash_algo->rawsz; i++) {
-   unsigned int val = *sha1++;
+   for (i = 0; i < algop->rawsz; i++) {
+   unsigned int val = *hash++;
*buf++ = hex[val >> 4];
*buf++ = hex[val & 0xf];
}
@@ -89,20 +90,35 @@ char *sha1_to_hex_r(char *buffer, const unsigned char *sha1)
return buffer;
 }
 
-char *oid_to_hex_r(char *buffer, const struct object_id *oid)
+char *sha1_to_hex_r(char *buffer, const unsigned char *sha1)
 {
-   return sha1_to_hex_r(buffer, oid->hash);
+   return hash_to_hex_algop_r(buffer, sha1, _algos[GIT_HASH_SHA1]);
 }
 
-char *sha1_to_hex(const unsigned char *sha1)
+char *oid_to_hex_r(char *buffer, const struct object_id *oid)
+{
+   return hash_to_hex_algop_r(buffer, oid->hash, the_hash_algo);
+}
+
+char *hash_to_hex_algop(const unsigned char *hash, const struct git_hash_algo 
*algop)
 {
static int bufno;
static char hexbuffer[4][GIT_MAX_HEXSZ + 1];
bufno = (bufno + 1) % ARRAY_SIZE(hexbuffer);
-   return sha1_to_hex_r(hexbuffer[bufno], sha1);
+   return hash_to_hex_algop_r(hexbuffer[bufno], hash, algop);
+}
+
+char *sha1_to_hex(const unsigned char *sha1)
+{
+   return hash_to_hex_algop(sha1, _algos[GIT_HASH_SHA1]);
+}
+
+char *hash_to_hex(const unsigned char *hash)
+{
+   return hash_to_hex_algop(hash, the_hash_algo);
 }
 
 char *oid_to_hex(const struct object_id *oid)
 {
-   return sha1_to_hex(oid->hash);
+   return

[PATCH v4 12/12] hash: add an SHA-256 implementation using OpenSSL

We already have OpenSSL routines available for SHA-1, so add routines
for SHA-256 as well.

On a Core i7-6600U, this SHA-256 implementation compares favorably to
the SHA1DC SHA-1 implementation:

SHA-1: 157 MiB/s (64 byte chunks); 337 MiB/s (16 KiB chunks)
SHA-256: 165 MiB/s (64 byte chunks); 408 MiB/s (16 KiB chunks)

Signed-off-by: brian m. carlson 
---
 Makefile | 7 +++
 hash.h   | 2 ++
 2 files changed, 9 insertions(+)

diff --git a/Makefile b/Makefile
index 5a07e03100..36fd3a149b 100644
--- a/Makefile
+++ b/Makefile
@@ -183,6 +183,8 @@ all::
 #
 # Define GCRYPT_SHA256 to use the SHA-256 routines in libgcrypt.
 #
+# Define OPENSSL_SHA256 to use the SHA-256 routines in OpenSSL.
+#
 # Define NEEDS_CRYPTO_WITH_SSL if you need -lcrypto when using -lssl (Darwin).
 #
 # Define NEEDS_SSL_WITH_CRYPTO if you need -lssl when using -lcrypto (Darwin).
@@ -1638,6 +1640,10 @@ endif
 endif
 endif
 
+ifdef OPENSSL_SHA256
+   EXTLIBS += $(LIB_4_CRYPTO)
+   BASIC_CFLAGS += -DSHA256_OPENSSL
+else
 ifdef GCRYPT_SHA256
BASIC_CFLAGS += -DSHA256_GCRYPT
EXTLIBS += -lgcrypt
@@ -1645,6 +1651,7 @@ else
LIB_OBJS += sha256/block/sha256.o
BASIC_CFLAGS += -DSHA256_BLK
 endif
+endif
 
 ifdef SHA1_MAX_BLOCK_SIZE
LIB_OBJS += compat/sha1-chunked.o
diff --git a/hash.h b/hash.h
index 2ef098052d..adde708cf2 100644
--- a/hash.h
+++ b/hash.h
@@ -17,6 +17,8 @@
 
 #if defined(SHA256_GCRYPT)
 #include "sha256/gcrypt.h"
+#elif defined(SHA256_OPENSSL)
+#include 
 #else
 #include "sha256/block/sha256.h"
 #endif

[PATCH v4 07/12] sha1-file: add a constant for hash block size

There is one place we need the hash algorithm block size: the HMAC code
for push certs.  Expose this constant in struct git_hash_algo and expose
values for SHA-1 and for the largest value of any hash.

Signed-off-by: brian m. carlson 
---
 cache.h | 4 
 hash.h  | 3 +++
 sha1-file.c | 2 ++
 3 files changed, 9 insertions(+)

diff --git a/cache.h b/cache.h
index bab8e8964f..9e5d1dd85a 100644
--- a/cache.h
+++ b/cache.h
@@ -45,10 +45,14 @@ unsigned long git_deflate_bound(git_zstream *, unsigned 
long);
 /* The length in bytes and in hex digits of an object name (SHA-1 value). */
 #define GIT_SHA1_RAWSZ 20
 #define GIT_SHA1_HEXSZ (2 * GIT_SHA1_RAWSZ)
+/* The block size of SHA-1. */
+#define GIT_SHA1_BLKSZ 64
 
 /* The length in byte and in hex digits of the largest possible hash value. */
 #define GIT_MAX_RAWSZ GIT_SHA1_RAWSZ
 #define GIT_MAX_HEXSZ GIT_SHA1_HEXSZ
+/* The largest possible block size for any supported hash. */
+#define GIT_MAX_BLKSZ GIT_SHA1_BLKSZ
 
 struct object_id {
unsigned char hash[GIT_MAX_RAWSZ];
diff --git a/hash.h b/hash.h
index 80881eea47..1bcf7ab6fd 100644
--- a/hash.h
+++ b/hash.h
@@ -81,6 +81,9 @@ struct git_hash_algo {
/* The length of the hash in hex characters. */
size_t hexsz;
 
+   /* The block size of the hash. */
+   size_t blksz;
+
/* The hash initialization function. */
git_hash_init_fn init_fn;
 
diff --git a/sha1-file.c b/sha1-file.c
index 7e9dedc744..9bdd04ea45 100644
--- a/sha1-file.c
+++ b/sha1-file.c
@@ -90,6 +90,7 @@ const struct git_hash_algo hash_algos[GIT_HASH_NALGOS] = {
0x,
0,
0,
+   0,
git_hash_unknown_init,
git_hash_unknown_update,
git_hash_unknown_final,
@@ -102,6 +103,7 @@ const struct git_hash_algo hash_algos[GIT_HASH_NALGOS] = {
0x73686131,
GIT_SHA1_RAWSZ,
GIT_SHA1_HEXSZ,
+   GIT_SHA1_BLKSZ,
git_hash_sha1_init,
git_hash_sha1_update,
git_hash_sha1_final,

the opposite of .gitignore, whitelist

2018-10-24 Thread lhf...@163.com

I have a good idea, add a file to git that is the opposite of .gitignore, 
whitelist, the code in the development directory can be submitted to git 
version control, you can only submit the source code in the src directory, 
without concern for development tools and operations.System and other files, 
after all, the type of each project code is fixed, I am in the community I did 
not find a way to submit requirements in the community, so
--
lhf...@163.com

Re: New semantic patches vs. in-flight topics [was: Re: [PATCH 00/19] Bring more repository handles into our code base]

2018-10-24 Thread SZEDER Gábor

On Mon, Oct 22, 2018 at 11:54:06AM -0700, Stefan Beller wrote:

> For the sake of a good history, I would think running 'make coccicheck'
> and applying the resulting patches would be best as part of the (dirty)
> merge of any topic that proposes new semantic patches, but that would
> add load to Junio as it would be an extra step during the merge.
> 
> One could argue that the step of applying such transformations into
> the dirty merge is cheaper than resolving merge conflicts that are
> had when the topic includes the transformation.

Please consider that merge commits' have uglier diffs than regular
commits, and that merge commits cause additional complications when
'git bisect' points the finger at them, both of which are exacerbated
by additional changes squeezed into evil merges.

> > Consequently, 'make coccicheck' won't run clean and the
> > static analysis build job will fail until all those topics reach
> > 'master', and the remaining transformations are applied on top.
> >
> > This was (and still is!) an issue with the hasheq()/oideq() series
> > as well: that series was added on 2018-08-28, and the static
> > analysis build job is red on 'pu' ever since.  See the follow-up
> > patch e43d2dcce1 (more oideq/hasheq conversions, 2018-10-02), and
> > one more follow-up will be necessary after the builtin stash topic
> > is merged to 'master'.
> 
> In my understanding this follow up is a feature, as it helps to avoid
> merge conflicts with other topics in flight.

I don't see how such a follow up patch helps to avoid merge conflicts.

There were topics that branched off before the introduction of oideq()
into 'master', therefore they couldn't make use of this new function
until they were merged to 'master' as well, so they added their own
!oidcmp() calls.  That follow up patch was necessary to transform
these new !oidcmp() calls after those topics reached 'master'.  Merge
conflicts had nothing to do with it.

So this follow up patch is not a feature, but rather an inherent
consequence of the project's branching model, with lots of parallel
running topics branching off at different points and progressing at
different speeds.

> > This makes it harder to review other patch series.
> 
> as 'make coccicheck' is an integral part of your review?

Erm, right, "review" was not the right word here.  Anyway, as it is,
'make coccicheck' is an integral part of our automated tests, not only
on Travis CI but on the upcoming Azure thing as well.  I just try to
pay attention to its results and the results of a bunch of my
additional builds, and complain or even send a fix when something goes
reproducibly wrong.  This has certainly became more cumbersome with
the permanently failing static analysis build job in the last couple
of weeks.

> > How about introducing the concept of "pending" semantic patches,
> > stored in 'contrib/coccinelle/.pending.cocci' files, modifying
> > 'make coccicheck' to skip them, and adding the new 'make
> > coccicheck-pending' target to make it convenient to apply them, e.g.
> > something like the simple patch at the end.
> >
> > So the process would go something like this:
> >
> >   - A new semantic patch should be added as "pending", e.g. to the
> > file 'the_repository.pending.cocci', together with the resulting
> > transformations in the same commit.
> >
> > This way neither 'make coccicheck' nor the static analysis build
> > job would complain in the topic branch or in the two integration
> > branches.  And if they do complain, then we would know right away
> > that they complain because of a well-established semantic patch.
> > Yet, anyone interested could run 'make coccicheck-pending' to see
> > where are we heading.
> >
> >   - The author of the "pending" semanting patch should then keep an
> > eye on already cooking topics: whether any of them contain new
> > code that should be transformed, and how they progress to
> > 'master', and sending followup patch(es) with the remaining
> > transformations when applicable.
> >
> > Futhermore, the author should also pay attention to any new topics
> > that branch off after the "pending" semantic patch, and whether
> > any of them introduce code to be transformed, warning their
> > authors as necessary.
> >
> >   - Finally, after all the dust settled, the dev should follow up with
> > a patch to:
> >
> >   - promote the "penging" patch to '.cocci', if its purpose
> > is to avoid undesirable code patterns in the future, or
> >
> >   - remove the semantic patch, if it was used in a one-off
> > transformation.
> >
> > Thoughts?
> 
> I like the approach of having separate classes of semantic patches:
> (a) the regular "we need to keep checking these" as they address
> undesirable code patterns, which is what we currently have,
> and what 'make coccicheck' would complain about.
> (b) The pending patches as

Re: [PATCH] i18n: make GETTEXT_POISON a runtime option

2018-10-24 Thread Ramsay Jones




On 25/10/2018 02:09, Jeff King wrote:
> On Thu, Oct 25, 2018 at 10:00:31AM +0900, Junio C Hamano wrote:
> 
>> Jeff King  writes:
>>
>>> but then you lose the default handling. I think if we added a new
>>> option, it would either be:
>>>
>>>   # interpret a value directly; use default on empty, I guess?
>>>   git config --default=false --type=bool --interpret-value 
>>> "$GIT_WHATEVER_ENV"
>>>
>>> or
>>>
>>>   # less flexible, but the --default semantics are more obvious
>>>   git config --default=false --type=bool --get-env GIT_WHATEVER_ENV
>>
>> Yeah, my thinko.  The latter would be closer to what this patch
>> wants to have, but obviously the former would be more flexible and
>> useful in wider context.  Both have the "Huh?" factor---what they
>> are doing has little to do with "config", but I did not think of a
>> better kitchen-sink (and our default kitchen-sink "rev-parse" is
>> even further than "config", I would think, for this one).
> 
> Heh, I thought through the exact sequence in your paragraph when writing
> my other message. That's probably a good sign that we should probably
> not pursue this further unless we see the use case come up again a few
> more times (and if we do, then consider "config" the least-bad place to
> do it).

I was thinking:

  $ git var -e GIT_WHATEVER_ENV

[-e for environment].

... but that is really no different than git-config. ;-)

ATB,
Ramsay Jones

Re: [PATCH 1/2] t5410: use longer path for sample script

Johannes Schindelin  writes:

> On Wed, 24 Oct 2018, Jeff King wrote:
>
>> t5410 creates a sample script "alternate-refs", and sets
>> core.alternateRefsCommand to just "alternate-refs". That
>> shouldn't work, as "." is not in our $PATH, and so we should
>> not find it.
>> 
>> However, due to a bug in run-command.c, we sometimes find it
>> anyway! Even more confusing, this bug is only in the
>> fork-based version of run-command. So the test passes on
>> Linux (etc), but fails on Windows.
>> 
>> In preparation for fixing the run-command bug, let's use a
>> more complete path here.
>> 
>> Reported-by: Johannes Schindelin 
>> Signed-off-by: Jeff King 
>> ---
>
> Thank you for the fix! I can confirm that the patch works, and the commit
> message is stellar, as per usual for your contributions.
>
> BTW since this breaks every single one of our Continuous Builds on
> Windows, I would be very much in favor of fast-tracking this to `master`.
>
> Thanks,
> Dscho

I should note to the public that this one, and the companion patch
2/2, owe greatly to you and Peff's efforts.

Thanks, both.

>
>>  t/t5410-receive-pack-alternates.sh | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>> 
>> diff --git a/t/t5410-receive-pack-alternates.sh 
>> b/t/t5410-receive-pack-alternates.sh
>> index 457c20c2a5..f00d0da860 100755
>> --- a/t/t5410-receive-pack-alternates.sh
>> +++ b/t/t5410-receive-pack-alternates.sh
>> @@ -23,7 +23,7 @@ test_expect_success 'with core.alternateRefsCommand' '
>>  --format="%(objectname)" \
>>  refs/heads/public/
>>  EOF
>> -test_config -C fork core.alternateRefsCommand alternate-refs &&
>> +test_config -C fork core.alternateRefsCommand ./alternate-refs &&
>>  git rev-parse public/branch >expect &&
>>  printf "" | git receive-pack fork >actual &&
>>  extract_haves actual.haves &&
>> -- 
>> 2.19.1.1094.gd480080bf6
>> 
>>

Re: [PATCH v3 2/3] reset: add new reset.quiet config setting

Duy Nguyen  writes:

> OK. Just to be sure we're on the same page. Am I waiting for all
> config changes to land in 'master', or do I rebase my series on
> 'next'? I usually base on 'master' but the mention of 'next' here
> confuses me a bit.

I was hoping that you can do something like:

  $ git fetch https://github.com/gitster/git \
--refmap=refs/heads/*:refs/remotes/broken-out/* \
bp/reset-quiet master
  $ git checkout broken-out/master^0
  $ git merge broekn-out/bp/reset-quiet
  $ git rebase HEAD np/config-split

once it is clear to everybody that Ben's reset series is ready to be
merged to 'next' (or it actually hits 'next').

Re: [PATCH] i18n: make GETTEXT_POISON a runtime option

On Thu, Oct 25, 2018 at 10:00:31AM +0900, Junio C Hamano wrote:

> Jeff King  writes:
> 
> > but then you lose the default handling. I think if we added a new
> > option, it would either be:
> >
> >   # interpret a value directly; use default on empty, I guess?
> >   git config --default=false --type=bool --interpret-value 
> > "$GIT_WHATEVER_ENV"
> >
> > or
> >
> >   # less flexible, but the --default semantics are more obvious
> >   git config --default=false --type=bool --get-env GIT_WHATEVER_ENV
> 
> Yeah, my thinko.  The latter would be closer to what this patch
> wants to have, but obviously the former would be more flexible and
> useful in wider context.  Both have the "Huh?" factor---what they
> are doing has little to do with "config", but I did not think of a
> better kitchen-sink (and our default kitchen-sink "rev-parse" is
> even further than "config", I would think, for this one).

Heh, I thought through the exact sequence in your paragraph when writing
my other message. That's probably a good sign that we should probably
not pursue this further unless we see the use case come up again a few
more times (and if we do, then consider "config" the least-bad place to
do it).

-Peff

[PATCH] l10n: vi.po: fix typo in pack-objects

2018-10-24 Thread Minh Nguyen


---
 po/vi.po | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/po/vi.po b/po/vi.po
index bc79319b6..e646825ed 100644
--- a/po/vi.po
+++ b/po/vi.po
@@ -13663,7 +13663,7 @@ msgstr "Đánh số các đối tượng"
 #: builtin/pack-objects.c:3382
 #, c-format
 msgid "Total % (delta %), reused % (delta 
%)"
-msgstr "Tỏng % (delta %), dùng lại % (delta 
%)"
+msgstr "Tổng % (delta %), dùng lại % (delta 
%)"


 #: builtin/pack-refs.c:7
 msgid "git pack-refs []"
--
2.18.0

Re: [PATCH] i18n: make GETTEXT_POISON a runtime option

Jeff King  writes:

> but then you lose the default handling. I think if we added a new
> option, it would either be:
>
>   # interpret a value directly; use default on empty, I guess?
>   git config --default=false --type=bool --interpret-value "$GIT_WHATEVER_ENV"
>
> or
>
>   # less flexible, but the --default semantics are more obvious
>   git config --default=false --type=bool --get-env GIT_WHATEVER_ENV

Yeah, my thinko.  The latter would be closer to what this patch
wants to have, but obviously the former would be more flexible and
useful in wider context.  Both have the "Huh?" factor---what they
are doing has little to do with "config", but I did not think of a
better kitchen-sink (and our default kitchen-sink "rev-parse" is
even further than "config", I would think, for this one).

[PATCH] sequencer: cleanup for gcc 8.2.1 warning

2018-10-24 Thread Carlo Marcelo Arenas Belón

sequencer.c: In function ‘write_basic_state’:
sequencer.c:2392:37: warning: zero-length gnu_printf format string 
[-Wformat-zero-length]
   write_file(rebase_path_verbose(), "");

Signed-off-by: Carlo Marcelo Arenas Belón 
---
 sequencer.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/sequencer.c b/sequencer.c
index 8dd6db5a01..358e83bf6b 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -2335,7 +2335,7 @@ int write_basic_state(struct replay_opts *opts, const 
char *head_name,
write_file(rebase_path_quiet(), "\n");
 
if (opts->verbose)
-   write_file(rebase_path_verbose(), "");
+   write_file(rebase_path_verbose(), "\n");
if (opts->strategy)
write_file(rebase_path_strategy(), "%s\n", opts->strategy);
if (opts->xopts_nr > 0)
-- 
2.19.1

Re: Recommended configurations (was Re: [PATCH v1 2/2] reset: add new reset.quietDefault config setting)

On Wed, Oct 24, 2018 at 11:48:20AM -0400, Derrick Stolee wrote:

> Generally, there are a lot of config settings that are likely in the "if you
> have a big repo, then you should use this" category. However, there is
> rarely a one-size-fits-all solution to these problems, just like there are
> different ways a repo can be "big" (working directory? number of commits?
> submodules?).
> [...]
> All of this is to say: it is probably a good idea to have some "recommended
> configuration" for big repos, but there will always be power users who want
> to tweak each and every one of these settings. I'm open to design ideas of
> how to store a list of recommended configurations and how to set a group of
> config settings with one command (say, a "git recommended-config
> [small|large|submodules]" builtin that fills the local config with the
> important settings).

Maybe it would be useful to teach git-sizer[1] to recommend particular
settings based on the actual definitions of "big" that it measures.

I do hope that some options will just be no-brainers to enable always,
though (e.g., I think in the long run commit-graph should just default
to "on"; it's cheap to keep up to date and helps proportionally to the
repo size).

[1] https://github.com/github/git-sizer

Re: [PATCH 2/3] mingw: replace MSVCRT's fstat() with a Win32-based implementation

On Wed, Oct 24, 2018 at 09:37:43AM +0200, Johannes Schindelin wrote:
> Hi brian,
> 
> On Wed, 24 Oct 2018, brian m. carlson wrote:
> > These lines strike me as a bit odd.  As far as I'm aware, Unix systems
> > don't return anything useful in this field when calling fstat on a pipe.
> > Is there a reason we fill this in on Windows?  If so, could the commit
> > message explain what that is?
> 
> AFAICT the idea was to imitate MSVCRT's fstat() in these cases.
> 
> But a quick web search suggests that you are right:
> https://bugzilla.redhat.com/show_bug.cgi?id=58768#c4 (I could not find any
> official documentation talking about fstat() and pipes, but I trust Alan
> to know their stuff).

Yeah, that behavior is quite old.  I'm surprised that Linux ever did
that.

> Do note, please, that according to the issue described in that link, at
> least *some* glibc/Linux combinations behave in exactly the way this patch
> implements it.
> 
> At this point, I am wary of changing this, too, as the code in question
> has been in production (read: tested thoroughly) in the current form for
> *years*, and I am really loathe to introduce a bug where even
> Windows-specific code in compat/ might rely on this behavior. (And no, I
> do not trust our test suite to find all of those use cases.)

I don't feel strongly either way.  I feel confident the rest of Git
doesn't use that field, so I don't see any downsides to keeping it other
than the slight overhead of populating it.  I just thought I'd ask in
case there was something important I was missing.
-- 
brian m. carlson: Houston, Texas, US
OpenPGP: https://keybase.io/bk2204


signature.asc
Description: PGP signature

Re: [PATCH v2 1/3] [Outreachy] t3903-stash: test without configured user name

2018-10-24 Thread Eric Sunshine

On Wed, Oct 24, 2018 at 4:06 PM Slavica Djukic
 wrote:
> This is part of enhancement request that ask for 'git stash' to work
> even if 'user.name' and 'user.email' are not configured.
> Due to an implementation detail, git-stash undesirably requires
> 'user.name' and 'user.email' to be set, but shouldn't.

Thanks for re-rolling. This version looks better. One comment below...

> Signed-off-by: Slavica Djukic 
> ---
> diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
> @@ -1156,4 +1156,17 @@ test_expect_success 'stash --  works with 
> binary files' '
> +test_expect_failure 'stash works when user.name and user.email are not set' '
> +test_commit 1 &&
> +test_config user.useconfigonly true &&
> +test_config stash.usebuiltin true &&
> +sane_unset GIT_AUTHOR_NAME &&
> +sane_unset GIT_AUTHOR_EMAIL &&
> +sane_unset GIT_COMMITTER_NAME &&
> +sane_unset GIT_COMMITTER_EMAIL &&
> +test_must_fail git config user.email &&

Instead of simply asserting that 'user.email' is not set here, you
could instead proactively ensure that it is not set. That is, instead
of the test_must_fail(), do this:

test_unconfig user.email &&
test_unconfig user.name &&

> +echo changed >1.t &&
> +git stash
> +'
> +
>  test_done
> --
> 2.19.1.windows.1
>

[PATCH v2 1/3] [Outreachy] t3903-stash: test without configured user name

2018-10-24 Thread Slavica Djukic

From: Slavica 

This is part of enhancement request that ask for 'git stash' to work
even if 'user.name' and 'user.email' are not configured.
Due to an implementation detail, git-stash undesirably requires
'user.name' and 'user.email' to be set, but shouldn't.
The issue is discussed here:
https://public-inbox.org/git/87o9debty4@evledraar.gmail.com/T/#u.

Signed-off-by: Slavica Djukic 
---
 t/t3903-stash.sh | 13 +
 1 file changed, 13 insertions(+)

diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
index 9e06494ba0..048998d5ce 100755
--- a/t/t3903-stash.sh
+++ b/t/t3903-stash.sh
@@ -1156,4 +1156,17 @@ test_expect_success 'stash --  works with binary 
files' '
test_path_is_file subdir/untracked
 '
 
+test_expect_failure 'stash works when user.name and user.email are not set' '
+test_commit 1 &&
+test_config user.useconfigonly true &&
+test_config stash.usebuiltin true &&
+sane_unset GIT_AUTHOR_NAME &&
+sane_unset GIT_AUTHOR_EMAIL &&
+sane_unset GIT_COMMITTER_NAME &&
+sane_unset GIT_COMMITTER_EMAIL &&
+test_must_fail git config user.email &&
+echo changed >1.t &&
+git stash
+'
+
 test_done
-- 
2.19.1.windows.1

[PATCH v2 1/3] [Outreachy] t3903-stash: test without configured user name

2018-10-24 Thread Slavica Djukic

Changes since v1:

*changed test title
*removed subshell and HOME override
*fixed weird identation
*unset() replaced with sane_unset()

Slavica (1):
  [Outreachy] t3903-stash: test without configured user name

 t/t3903-stash.sh | 13 +
 1 file changed, 13 insertions(+)

-- 
2.19.1.windows.1

[PATCH v3] range-diff: allow to diff files regardless of submodule config

2018-10-24 Thread Lucas De Marchi

If we have `submodule.diff = log' in the configuration file
or `--submodule=log' is given as argument, range-diff fails
to compare both diffs and we only get the following output:

Submodule a 000...000 (new submodule)

Even if the repository doesn't have any submodule.

That's because the mode in diff_filespec is not correct and when
flushing the diff, down in builtin_diff() we will enter the condition:

if (o->submodule_format == DIFF_SUBMODULE_LOG &&
(!one->mode || S_ISGITLINK(one->mode)) &&
(!two->mode || S_ISGITLINK(two->mode))) {
show_submodule_summary(o, one->path ? one->path : two->path,
>oid, >oid,
two->dirty_submodule);
return;

It turns out that S_ISGITLINK will return true (mode == 016 here).
Similar thing happens if submodule.diff is "diff".

Do like it's done in grep.c when calling fill_filespec() and force it to
be recognized as a file by adding S_IFREG to the mode.

Signed-off-by: Lucas De Marchi 
Acked-by: Johannes Schindelin 
---

v2: Add test to make sure we don't regress

v3: Extend commit mesage with better explanation and base it on maint branch
since it's a bug fix

 range-diff.c  |  2 +-
 t/t3206-range-diff.sh | 29 +
 2 files changed, 30 insertions(+), 1 deletion(-)

diff --git a/range-diff.c b/range-diff.c
index b6b9abac26..3d6aa2330a 100644
--- a/range-diff.c
+++ b/range-diff.c
@@ -334,7 +334,7 @@ static struct diff_filespec *get_filespec(const char *name, 
const char *p)
 {
struct diff_filespec *spec = alloc_filespec(name);
 
-   fill_filespec(spec, _oid, 0, 0644);
+   fill_filespec(spec, _oid, 0, 0100644);
spec->data = (char *)p;
spec->size = strlen(p);
spec->should_munmap = 0;
diff --git a/t/t3206-range-diff.sh b/t/t3206-range-diff.sh
index 2237c7f4af..518c9a527d 100755
--- a/t/t3206-range-diff.sh
+++ b/t/t3206-range-diff.sh
@@ -122,6 +122,35 @@ test_expect_success 'changed commit' '
test_cmp expected actual
 '
 
+test_expect_success 'changed commit with sm config' '
+   git range-diff --no-color --submodule=log topic...changed >actual &&
+   cat >expected <<-EOF &&
+   1:  4de457d = 1:  a4b s/5/A/
+   2:  fccce22 = 2:  f51d370 s/4/A/
+   3:  147e64e ! 3:  0559556 s/11/B/
+   @@ -10,7 +10,7 @@
+ 9
+ 10
+-11
+   -+B
+   ++BB
+ 12
+ 13
+ 14
+   4:  a63e992 ! 4:  d966c5c s/12/B/
+   @@ -8,7 +8,7 @@
+@@
+ 9
+ 10
+   - B
+   + BB
+-12
++B
+ 13
+   EOF
+   test_cmp expected actual
+'
+
 test_expect_success 'changed message' '
git range-diff --no-color topic...changed-message >actual &&
sed s/Z/\ /g >expected <<-EOF &&
-- 
2.19.1.543.g4e5b40e2e8

Re: [PATCH] Poison gettext with the Ook language

2018-10-24 Thread Ævar Arnfjörð Bjarmason

On Wed, Oct 24 2018, Duy Nguyen wrote:

> On Tue, Oct 23, 2018 at 6:45 PM Ævar Arnfjörð Bjarmason
>  wrote:
>> >> The effect of what I'm suggesting here, and which my WIP patch in
>> >> <875zxtd59e@evledraar.gmail.com> implements is that we'd do a
>> >> one-time getenv() for each process that prints a _() message that we
>> >> aren't doing now, and for each message call a function that would check
>> >> a boolean "are we in poison mode" static global variable.
>> >
>> > Just don't do the getenv() inside _() even if you just do it one time.
>> > getenv() may invalidate whatever value from the previous call. I would
>> > not want to find out my code broke because of an getenv() inside some
>> > innocent _()...
>>
>> How would any code break? We have various getenv("GIT_TEST_*...")
>> already that work the same way. Unless you set that in the environment
>> the function is idempotent, and I don't see how anyone would do that
>> accidentally.
>
> I didn't check those GIT_TEST_ but I think the difference is in
> complexity. When you write
>
>  var = getenv("foo");
>  complexFunction();
>
> you probably start to feel scary and wrap getenv() with a strdup()
> because you usually don't know exactly what complexFunction() can call
> (and even if you do, you can't be sure what it may call in the
> future).
>
> The person who writes
>
>  printf(_("%s"), getenv("foo"));
>
> may not go through the same thought process as with complexFunction().
> If _() calls getenv(), because you the order of parameter evaluation
> is unspecified, you cannot be sure if getenv("foo") will be called
> before or after the one inside _(). One of them may screw up the
> other.

Ah, you mean because getenv() is not reentrant such a construct may
cause us to return something else entirely for getenv("bar") (e.g. in
this case the value for getenv("bar")).

Is that something you or anyone else has seen in practice? My intuition
is that while POSIX doesn't make that promise it isn't likely that
there's any implementation that would mutate the env purely on a
getenv(), but setenv() (munging some static "env" area in-place) might
be another matter.

But we could always call use_gettext_poison() really early from
git_setup_gettext() (called from our main()) if we're worried about
this, it would then set the static "poison_requested" variable and we
wouldn't call getenv() again, e.g. if we're later calling it in the
middle of multithreaded code, or within the same same sequence point.

>> > And we should still respect NO_GETTEXT, not doing any extra work when
>> > it's defined.
>>
>> This is not how any of our NO_* defines work. *Sometimes* defining them
>> guarantees you do no extra work, but in other cases we've decided that
>> bending over backwards with #ifdef in various places isn't worth it.
>
> I guess it boils down to "worth it". For me #ifdef NO_GETTEXT would be
> limited to gettext.h and it's not that much work. But you do the
> actual work. You decide.

Yeah the ifdef is pretty small and not really worth worrynig about, the
main benefit is being able to run tests in this mode without
recompiling.

I hadn't been running with GETTEXT_POISON when I build git because I
hadn't been bothered to build twice just for it, but am now running it
alongside other GIT_TEST_* modes.

Re: [PATCH] range-diff: allow to diff files regardless submodule

2018-10-24 Thread Lucas De Marchi

On Wed, Oct 24, 2018 at 02:18:43PM +0900, Junio C Hamano wrote:
> Junio C Hamano  writes:
> 
> > Lucas De Marchi  writes:
> >
> >> Your reply arrived just a little after I sent the v2, so I thought it
> >> was just the race and you would end up seeing the unread email in the
> >> same thread. Sorry for not including the msg id:
> >> 20181011081750.24240-1-lucas.demar...@intel.com
> >
> > OK, then I am not surprised that I do not recall seeing such an old
> > message ;-)  Let me take a look.
> 
> Heh, it turns out that that is the version that has been queued in
> 'next' for about a week already.
> 
> One issue that was pointed out in v1 was that the log message was
> unclear; it seems v2 didn't address that at all, though.

ok, let me try to expand it.

thanks
Lucas De Marchi

> 
> Thanks.
>

[PATCH v3 3/3] doc: document diff/merge.guitool config keys

Signed-off-by: Denton Liu 
---
 Documentation/diff-config.txt  | 8 
 Documentation/merge-config.txt | 6 ++
 2 files changed, 14 insertions(+)

diff --git a/Documentation/diff-config.txt b/Documentation/diff-config.txt
index 85bca83c3..e64d983c3 100644
--- a/Documentation/diff-config.txt
+++ b/Documentation/diff-config.txt
@@ -177,6 +177,14 @@ diff.tool::
Any other value is treated as a custom diff tool and requires
that a corresponding difftool..cmd variable is defined.
 
+diff.guitool::
+   Controls which diff tool is used by linkgit:git-difftool[1] when
+   the -g/--gui flag is specified. This variable overrides the value
+   configured in `merge.guitool`. The list below shows the valid
+   built-in values. Any other value is treated as a custom diff tool
+   and requires that a corresponding difftool..cmd variable
+   is defined.
+
 include::mergetools-diff.txt[]
 
 diff.indentHeuristic::
diff --git a/Documentation/merge-config.txt b/Documentation/merge-config.txt
index 662c2713c..a7f4ea90c 100644
--- a/Documentation/merge-config.txt
+++ b/Documentation/merge-config.txt
@@ -63,6 +63,12 @@ merge.tool::
Any other value is treated as a custom merge tool and requires
that a corresponding mergetool..cmd variable is defined.
 
+merge.guitool::
+   Controls which merge tool is used by linkgit:git-mergetool[1] when the
+   -g/--gui flag is specified. The list below shows the valid built-in 
values.
+   Any other value is treated as a custom merge tool and requires that a
+   corresponding mergetool..cmd variable is defined.
+
 include::mergetools-merge.txt[]
 
 merge.verbosity::
-- 
2.19.1.544.ge0b0585a1.dirty

[PATCH v3 2/3] completion: support `git mergetool --[no-]gui`

Signed-off-by: Denton Liu 
Signed-off-by: Anmol Mago 
Signed-off-by: Brian Ho 
Signed-off-by: David Lu 
Signed-off-by: Ryan Wang 
---
 contrib/completion/git-completion.bash | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/contrib/completion/git-completion.bash 
b/contrib/completion/git-completion.bash
index db7fd87b6..a45b4a050 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -1833,7 +1833,7 @@ _git_mergetool ()
return
;;
--*)
-   __gitcomp "--tool= --prompt --no-prompt"
+   __gitcomp "--tool= --prompt --no-prompt --gui --no-gui"
return
;;
esac
-- 
2.19.1.544.ge0b0585a1.dirty

[PATCH v3 1/3] mergetool: accept -g/--[no-]gui as arguments

In line with how difftool accepts a -g/--[no-]gui option, make mergetool
accept the same option in order to use the `merge.guitool` variable to
find the default mergetool instead of `merge.tool`.

Signed-off-by: Denton Liu 
Signed-off-by: Anmol Mago 
Signed-off-by: Brian Ho 
Signed-off-by: David Lu 
Signed-off-by: Ryan Wang 
Acked-by: David Aguilar 
---
 Documentation/git-mergetool.txt | 11 +++
 git-mergetool--lib.sh   | 16 +++-
 git-mergetool.sh| 11 +--
 3 files changed, 31 insertions(+), 7 deletions(-)

diff --git a/Documentation/git-mergetool.txt b/Documentation/git-mergetool.txt
index 3622d6648..0c7975a05 100644
--- a/Documentation/git-mergetool.txt
+++ b/Documentation/git-mergetool.txt
@@ -79,6 +79,17 @@ success of the resolution after the custom tool has exited.
Prompt before each invocation of the merge resolution program
to give the user a chance to skip the path.
 
+-g::
+--gui::
+   When 'git-mergetool' is invoked with the `-g` or `--gui` option
+   the default merge tool will be read from the configured
+   `merge.guitool` variable instead of `merge.tool`.
+
+--no-gui::
+   This overrides a previous `-g` or `--gui` setting and reads the
+   default merge tool will be read from the configured `merge.tool`
+   variable.
+
 -O::
Process files in the order specified in the
, which has one shell glob pattern per line.
diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh
index 9a8b97a2a..83bf52494 100644
--- a/git-mergetool--lib.sh
+++ b/git-mergetool--lib.sh
@@ -350,17 +350,23 @@ guess_merge_tool () {
 }
 
 get_configured_merge_tool () {
-   # Diff mode first tries diff.tool and falls back to merge.tool.
-   # Merge mode only checks merge.tool
+   # If first argument is true, find the guitool instead
+   if test "$1" = true
+   then
+   gui_prefix=gui
+   fi
+
+   # Diff mode first tries diff.(gui)tool and falls back to 
merge.(gui)tool.
+   # Merge mode only checks merge.(gui)tool
if diff_mode
then
-   merge_tool=$(git config diff.tool || git config merge.tool)
+   merge_tool=$(git config diff.${gui_prefix}tool || git config 
merge.${gui_prefix}tool)
else
-   merge_tool=$(git config merge.tool)
+   merge_tool=$(git config merge.${gui_prefix}tool)
fi
if test -n "$merge_tool" && ! valid_tool "$merge_tool"
then
-   echo >&2 "git config option $TOOL_MODE.tool set to unknown 
tool: $merge_tool"
+   echo >&2 "git config option $TOOL_MODE.${gui_prefix}tool set to 
unknown tool: $merge_tool"
echo >&2 "Resetting to default..."
return 1
fi
diff --git a/git-mergetool.sh b/git-mergetool.sh
index d07c7f387..01b9ad59b 100755
--- a/git-mergetool.sh
+++ b/git-mergetool.sh
@@ -9,7 +9,7 @@
 # at the discretion of Junio C Hamano.
 #
 
-USAGE='[--tool=tool] [--tool-help] [-y|--no-prompt|--prompt] [-O] 
[file to merge] ...'
+USAGE='[--tool=tool] [--tool-help] [-y|--no-prompt|--prompt] 
[-g|--gui|--no-gui] [-O] [file to merge] ...'
 SUBDIRECTORY_OK=Yes
 NONGIT_OK=Yes
 OPTIONS_SPEC=
@@ -389,6 +389,7 @@ print_noop_and_exit () {
 
 main () {
prompt=$(git config --bool mergetool.prompt)
+   gui_tool=false
guessed_merge_tool=false
orderfile=
 
@@ -414,6 +415,12 @@ main () {
shift ;;
esac
;;
+   --no-gui)
+   gui_tool=false
+   ;;
+   -g|--gui)
+   gui_tool=true
+   ;;
-y|--no-prompt)
prompt=false
;;
@@ -443,7 +450,7 @@ main () {
if test -z "$merge_tool"
then
# Check if a merge tool has been configured
-   merge_tool=$(get_configured_merge_tool)
+   merge_tool=$(get_configured_merge_tool $gui_tool)
# Try to guess an appropriate merge tool if no tool has been 
set.
if test -z "$merge_tool"
then
-- 
2.19.1.544.ge0b0585a1.dirty

[PATCH v3 0/3] Add --gui to mergetool

This adds another patch on top of the existing patchset in order to document the
guitool keys for `git config`. This way, the completions script will now be able
to complete these key values as well.

Denton Liu (3):
  mergetool: accept -g/--[no-]gui as arguments
  completion: support `git mergetool --[no-]gui`
  doc: document diff/merge.guitool config keys

 Documentation/diff-config.txt  |  8 
 Documentation/git-mergetool.txt| 11 +++
 Documentation/merge-config.txt |  6 ++
 contrib/completion/git-completion.bash |  2 +-
 git-mergetool--lib.sh  | 16 +++-
 git-mergetool.sh   | 11 +--
 6 files changed, 46 insertions(+), 8 deletions(-)

-- 
2.19.1.544.ge0b0585a1.dirty

Re: [PATCH v3 2/3] shallow: offer to prune only non-existing entries

Hi,

On Wed, 24 Oct 2018, Johannes Schindelin wrote:

> On Wed, 24 Oct 2018, Junio C Hamano wrote:
> 
> > Jonathan, do you see any issues with the use of lookup_commit() in
> > this change wrt lazy clone?  I am wondering what happens when the
> > commit in question is at, an immediate parent of, or an immediate
> > child of a promisor object.  I _think_ this change won't make it
> > worse for two features in playing together, but thought that it
> > would be better to double check.
> 
> Good point.
> 
> Instinctively, I would say that no promised object can be a shallow
> commit. The entire idea of a shallow commit is that it *is* present, but
> none of its parents.
> 
> Also, I would claim that the shallow feature does not make sense with lazy
> clones, as lazy clones offer a superset of shallow clone's functionality.
> 
> However, I am curious whether there is a better way to check for the
> presence of a local commit? Do we have an API function for that, that I
> missed? (I do not really want to parse the commit, after all, just verify
> that it is not pruned.)

Okay, I looked around a bit. It seems that there is an
`is_promisor_object(oid)` function in `pu` to see whether an object was
promised. If need be (and I am still not convinced that there is a need),
then we can always add a call to that function to the condition.

Coming back to my question whether there is a better way to check for the
presence of a local commit, I figured that I can use `has_object_file()`
instead of looking up and parsing the commit, as this code does not really
need to verify that the shallow entry refers to a commit, but only that it
refers to a local object.

So I'll send another iteration shortly, with this diff applied to 2/3:

-- snip --
diff --git a/shallow.c b/shallow.c
index 9c3faa8af243..02fdbfc554c4 100644
--- a/shallow.c
+++ b/shallow.c
@@ -263,8 +263,7 @@ static int write_one_shallow(const struct commit_graft 
*graft, void *cb_data)
if (graft->nr_parent != -1)
return 0;
if (data->flags & QUICK) {
-   struct commit *c = lookup_commit(the_repository, >oid);
-   if (!c || parse_commit(c))
+   if (!has_object_file(>oid))
return 0;
} else if (data->flags & SEEN_ONLY) {
struct commit *c = lookup_commit(the_repository, >oid);
--snap --

Ciao,
Dscho

> 
> > 
> > "Johannes Schindelin via GitGitGadget" 
> > writes:
> > 
> > > From: Johannes Schindelin 
> > >
> > > The `prune_shallow()` function wants a full reachability check to be
> > > completed before it goes to work, to ensure that all unreachable entries
> > > are removed from the shallow file.
> > >
> > > However, in the upcoming patch we do not even want to go that far. We
> > > really only need to remove entries corresponding to pruned commits, i.e.
> > > to commits that no longer exist.
> > >
> > > Let's support that use case.
> > >
> > > Signed-off-by: Johannes Schindelin 
> > > ---
> > >  builtin/prune.c |  2 +-
> > >  commit.h|  2 +-
> > >  shallow.c   | 22 +-
> > >  3 files changed, 19 insertions(+), 7 deletions(-)
> > >
> > > diff --git a/builtin/prune.c b/builtin/prune.c
> > > index 41230f821..6d6ab6cf1 100644
> > > --- a/builtin/prune.c
> > > +++ b/builtin/prune.c
> > > @@ -161,7 +161,7 @@ int cmd_prune(int argc, const char **argv, const char 
> > > *prefix)
> > >   free(s);
> > >  
> > >   if (is_repository_shallow(the_repository))
> > > - prune_shallow(show_only);
> > > + prune_shallow(show_only, 0);
> > >  
> > >   return 0;
> > >  }
> > > diff --git a/commit.h b/commit.h
> > > index 1d260d62f..ff34447ab 100644
> > > --- a/commit.h
> > > +++ b/commit.h
> > > @@ -249,7 +249,7 @@ extern void assign_shallow_commits_to_refs(struct 
> > > shallow_info *info,
> > >  uint32_t **used,
> > >  int *ref_status);
> > >  extern int delayed_reachability_test(struct shallow_info *si, int c);
> > > -extern void prune_shallow(int show_only);
> > > +extern void prune_shallow(int show_only, int quick_prune);
> > >  extern struct trace_key trace_shallow;
> > >  
> > >  extern int interactive_add(int argc, const char **argv, const char 
> > > *prefix, int patch);
> > > diff --git a/shallow.c b/shallow.c
> > > index 732e18d54..0a2671bc2 100644
> > > --- a/shallow.c
> > > +++ b/shallow.c
> > > @@ -247,6 +247,7 @@ static void check_shallow_file_for_update(struct 
> > > repository *r)
> > >  
> > >  #define SEEN_ONLY 1
> > >  #define VERBOSE   2
> > > +#define QUICK_PRUNE 4
> > >  
> > >  struct write_shallow_data {
> > >   struct strbuf *out;
> > > @@ -261,7 +262,11 @@ static int write_one_shallow(const struct 
> > > commit_graft *graft, void *cb_data)
> > >   const char *hex = oid_to_hex(>oid);
> > >   if (graft->nr_parent != -1)
> > >   return 0;
> > > - if (data->flags & SEEN_ONLY) {
> > > + if (data->flags & QUICK_PRUNE) {
> > > +

[PATCH v4 3/3] repack -ad: prune the list of shallow commits

2018-10-24 Thread Johannes Schindelin via GitGitGadget

From: Johannes Schindelin 

`git repack` can drop unreachable commits without further warning,
making the corresponding entries in `.git/shallow` invalid, which causes
serious problems when deepening the branches.

One scenario where unreachable commits are dropped by `git repack` is
when a `git fetch --prune` (or even a `git fetch` when a ref was
force-pushed in the meantime) can make a commit unreachable that was
reachable before.

Therefore it is not safe to assume that a `git repack -adlf` will keep
unreachable commits alone (under the assumption that they had not been
packed in the first place, which is an assumption at least some of Git's
code seems to make).

This is particularly important to keep in mind when looking at the
`.git/shallow` file: if any commits listed in that file become
unreachable, it is not a problem, but if they go missing, it *is* a
problem. One symptom of this problem is that a deepening fetch may now
fail with

fatal: error in object: unshallow 

To avoid this problem, let's prune the shallow list in `git repack` when
the `-d` option is passed, unless `-A` is passed, too (which would force
the now-unreachable objects to be turned into loose objects instead of
being deleted). Additionally, we also need to take `--keep-reachable`
and `--unpack-unreachable=` into account.

Note: an alternative solution discussed during the review of this patch
was to teach `git fetch` to simply ignore entries in .git/shallow if the
corresponding commits do not exist locally. A quick test, however,
revealed that the .git/shallow file is written during a shallow *clone*,
in which case the commits do not exist, either, but the "shallow" line
*does* need to be sent. Therefore, this approach would be a lot more
finicky than the approach presented by the this patch.

Signed-off-by: Johannes Schindelin 
---
 builtin/repack.c | 6 ++
 t/t5537-fetch-shallow.sh | 2 +-
 2 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/builtin/repack.c b/builtin/repack.c
index c6a7943d5..acd43c75d 100644
--- a/builtin/repack.c
+++ b/builtin/repack.c
@@ -549,6 +549,12 @@ int cmd_repack(int argc, const char **argv, const char 
*prefix)
if (!po_args.quiet && isatty(2))
opts |= PRUNE_PACKED_VERBOSE;
prune_packed_objects(opts);
+
+   if (!keep_unreachable &&
+   (!(pack_everything & LOOSEN_UNREACHABLE) ||
+unpack_unreachable) &&
+   is_repository_shallow(the_repository))
+   prune_shallow(PRUNE_QUICK);
}
 
if (!no_update_server_info)
diff --git a/t/t5537-fetch-shallow.sh b/t/t5537-fetch-shallow.sh
index 686fdc928..6faf17e17 100755
--- a/t/t5537-fetch-shallow.sh
+++ b/t/t5537-fetch-shallow.sh
@@ -186,7 +186,7 @@ EOF
test_cmp expect actual
 '
 
-test_expect_failure '.git/shallow is edited by repack' '
+test_expect_success '.git/shallow is edited by repack' '
git init shallow-server &&
test_commit -C shallow-server A &&
test_commit -C shallow-server B &&
-- 
gitgitgadget

[PATCH v4 2/3] shallow: offer to prune only non-existing entries

2018-10-24 Thread Johannes Schindelin via GitGitGadget

From: Johannes Schindelin 

The `prune_shallow()` function wants a full reachability check to be
completed before it goes to work, to ensure that all unreachable entries
are removed from the shallow file.

However, in the upcoming patch we do not even want to go that far. We
really only need to remove entries corresponding to pruned commits, i.e.
to commits that no longer exist.

Let's support that use case.

Rather than extending the signature of `prune_shallow()` to accept
another Boolean, let's turn it into a bit field and declare constants,
for readability.

Signed-off-by: Johannes Schindelin 
---
 builtin/prune.c |  2 +-
 commit.h|  4 +++-
 shallow.c   | 23 +--
 3 files changed, 21 insertions(+), 8 deletions(-)

diff --git a/builtin/prune.c b/builtin/prune.c
index 41230f821..1ec9ddd75 100644
--- a/builtin/prune.c
+++ b/builtin/prune.c
@@ -161,7 +161,7 @@ int cmd_prune(int argc, const char **argv, const char 
*prefix)
free(s);
 
if (is_repository_shallow(the_repository))
-   prune_shallow(show_only);
+   prune_shallow(show_only ? PRUNE_SHOW_ONLY : 0);
 
return 0;
 }
diff --git a/commit.h b/commit.h
index 1d260d62f..b80d6fdb5 100644
--- a/commit.h
+++ b/commit.h
@@ -249,7 +249,9 @@ extern void assign_shallow_commits_to_refs(struct 
shallow_info *info,
   uint32_t **used,
   int *ref_status);
 extern int delayed_reachability_test(struct shallow_info *si, int c);
-extern void prune_shallow(int show_only);
+#define PRUNE_SHOW_ONLY 1
+#define PRUNE_QUICK 2
+extern void prune_shallow(unsigned options);
 extern struct trace_key trace_shallow;
 
 extern int interactive_add(int argc, const char **argv, const char *prefix, 
int patch);
diff --git a/shallow.c b/shallow.c
index 732e18d54..02fdbfc55 100644
--- a/shallow.c
+++ b/shallow.c
@@ -247,6 +247,7 @@ static void check_shallow_file_for_update(struct repository 
*r)
 
 #define SEEN_ONLY 1
 #define VERBOSE   2
+#define QUICK 4
 
 struct write_shallow_data {
struct strbuf *out;
@@ -261,7 +262,10 @@ static int write_one_shallow(const struct commit_graft 
*graft, void *cb_data)
const char *hex = oid_to_hex(>oid);
if (graft->nr_parent != -1)
return 0;
-   if (data->flags & SEEN_ONLY) {
+   if (data->flags & QUICK) {
+   if (!has_object_file(>oid))
+   return 0;
+   } else if (data->flags & SEEN_ONLY) {
struct commit *c = lookup_commit(the_repository, >oid);
if (!c || !(c->object.flags & SEEN)) {
if (data->flags & VERBOSE)
@@ -371,16 +375,23 @@ void advertise_shallow_grafts(int fd)
 
 /*
  * mark_reachable_objects() should have been run prior to this and all
- * reachable commits marked as "SEEN".
+ * reachable commits marked as "SEEN", except when quick_prune is non-zero,
+ * in which case lines are excised from the shallow file if they refer to
+ * commits that do not exist (any longer).
  */
-void prune_shallow(int show_only)
+void prune_shallow(unsigned options)
 {
struct lock_file shallow_lock = LOCK_INIT;
struct strbuf sb = STRBUF_INIT;
+   unsigned flags = SEEN_ONLY;
int fd;
 
-   if (show_only) {
-   write_shallow_commits_1(, 0, NULL, SEEN_ONLY | VERBOSE);
+   if (options & PRUNE_QUICK)
+   flags |= QUICK;
+
+   if (options & PRUNE_SHOW_ONLY) {
+   flags |= VERBOSE;
+   write_shallow_commits_1(, 0, NULL, flags);
strbuf_release();
return;
}
@@ -388,7 +399,7 @@ void prune_shallow(int show_only)
   git_path_shallow(the_repository),
   LOCK_DIE_ON_ERROR);
check_shallow_file_for_update(the_repository);
-   if (write_shallow_commits_1(, 0, NULL, SEEN_ONLY)) {
+   if (write_shallow_commits_1(, 0, NULL, flags)) {
if (write_in_full(fd, sb.buf, sb.len) < 0)
die_errno("failed to write to %s",
  get_lock_file_path(_lock));
-- 
gitgitgadget

[PATCH v4 0/3] repack -ad: fix after fetch --prune in a shallow repository

2018-10-24 Thread Johannes Schindelin via GitGitGadget

Under certain circumstances, commits that were reachable can be made
unreachable, e.g. via git fetch --prune. These commits might have been
packed already, in which case git repack -adlf will just drop them without
giving them the usual grace period before an eventual git prune (via git gc)
prunes them.

This is a problem when the shallow file still lists them, which is the
reason why git prune edits that file. And with the proposed changes, git
repack -ad will now do the same.

Reported by Alejandro Pauly.

Changes since v3:

 * Made the regression test less confusing with regards to tags that might
   need to be dereferenced.
 * Introduced new global constants PRUNE_SHOW_ONLY and PRUNE_QUICK instead
   of extending the signature of prune_shallow() with Boolean parameters.
 * While at it, renamed the file-local constant from QUICK_PRUNE to QUICK.
 * Replaced the lookup_commit() && parse_commit() cadence (that wants to
   test for the existence of a commit) to has_object_file(), for ease of
   readability, and also to make it more obvious how to add a call to 
   is_promisor_object() if we ever figure out that that would be necessary.

Changes since v2:

 * Fixed a typo in the last commit message.
 * Added an explanation to the last commit message why we do not simply skip
   non-existing shallow commits at fetch time.
 * Introduced a new, "quick prune" mode where prune_shallow() does not try
   to drop unreachable commits, but only non-existing ones.
 * Rebased to current master because there were too many merge conflicts for
   my liking otherwise.

Changes since v1:

 * Also trigger prune_shallow() when --unpack-unreachable= was
   passed to git repack.
 * No need to trigger prune_shallow() when git repack was called with -k.

Johannes Schindelin (3):
  repack: point out a bug handling stale shallow info
  shallow: offer to prune only non-existing entries
  repack -ad: prune the list of shallow commits

 builtin/prune.c  |  2 +-
 builtin/repack.c |  6 ++
 commit.h |  4 +++-
 shallow.c| 23 +--
 t/t5537-fetch-shallow.sh | 27 +++
 5 files changed, 54 insertions(+), 8 deletions(-)


base-commit: c4df23f7927d8d00e666a3c8d1b3375f1dc8a3c1
Published-As: 
https://github.com/gitgitgadget/git/releases/tags/pr-9%2Fdscho%2Fshallow-and-fetch-prune-v4
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git 
pr-9/dscho/shallow-and-fetch-prune-v4
Pull-Request: https://github.com/gitgitgadget/git/pull/9

Range-diff vs v3:

 1:  ed8559b91 ! 1:  d9720bd25 repack: point out a bug handling stale shallow 
info
 @@ -29,7 +29,7 @@
  + test_commit -C shallow-server C &&
  + test_commit -C shallow-server E &&
  + test_commit -C shallow-server D &&
 -+ d="$(git -C shallow-server rev-parse --verify D)" &&
 ++ d="$(git -C shallow-server rev-parse --verify D^0)" &&
  + git -C shallow-server checkout master &&
  +
  + git clone --depth=1 --no-tags --no-single-branch \
 @@ -43,7 +43,7 @@
  + test_must_fail git -C shallow-client rev-parse --verify $d^0 &&
  + ! grep $d shallow-client/.git/shallow &&
  +
 -+ git -C shallow-server branch branch-orig D^0 &&
 ++ git -C shallow-server branch branch-orig $d &&
  + git -C shallow-client fetch --prune --depth=2 \
  + origin "+refs/heads/*:refs/remotes/origin/*"
  +'
 2:  f085eb4f7 ! 2:  18308c13e shallow: offer to prune only non-existing entries
 @@ -12,6 +12,10 @@
  
  Let's support that use case.
  
 +Rather than extending the signature of `prune_shallow()` to accept
 +another Boolean, let's turn it into a bit field and declare constants,
 +for readability.
 +
  Signed-off-by: Johannes Schindelin 
  
  diff --git a/builtin/prune.c b/builtin/prune.c
 @@ -22,7 +26,7 @@
   
if (is_repository_shallow(the_repository))
  - prune_shallow(show_only);
 -+ prune_shallow(show_only, 0);
 ++ prune_shallow(show_only ? PRUNE_SHOW_ONLY : 0);
   
return 0;
   }
 @@ -35,7 +39,9 @@
   int *ref_status);
   extern int delayed_reachability_test(struct shallow_info *si, int c);
  -extern void prune_shallow(int show_only);
 -+extern void prune_shallow(int show_only, int quick_prune);
 ++#define PRUNE_SHOW_ONLY 1
 ++#define PRUNE_QUICK 2
 ++extern void prune_shallow(unsigned options);
   extern struct trace_key trace_shallow;
   
   extern int interactive_add(int argc, const char **argv, const char 
*prefix, int patch);
 @@ -47,7 +53,7 @@
   
   #define SEEN_ONLY 1
   #define VERBOSE   2
 -+#define QUICK_PRUNE 4
 ++#define QUICK 4
   
   struct write_shallow_data {
struct strbuf *out;
 @@ -56,9 +62,8 @@
if (graft->nr_parent != -1)
return 0;
  - if (data->flags

[PATCH v4 1/3] repack: point out a bug handling stale shallow info

2018-10-24 Thread Johannes Schindelin via GitGitGadget

From: Johannes Schindelin 

A `git fetch --prune` can turn previously-reachable objects unreachable,
even commits that are in the `shallow` list. A subsequent `git repack
-ad` will then unceremoniously drop those unreachable commits, and the
`shallow` list will become stale. This means that when we try to fetch
with a larger `--depth` the next time, we may end up with:

fatal: error in object: unshallow 

Reported by Alejandro Pauly.

Signed-off-by: Johannes Schindelin 
---
 t/t5537-fetch-shallow.sh | 27 +++
 1 file changed, 27 insertions(+)

diff --git a/t/t5537-fetch-shallow.sh b/t/t5537-fetch-shallow.sh
index 7045685e2..686fdc928 100755
--- a/t/t5537-fetch-shallow.sh
+++ b/t/t5537-fetch-shallow.sh
@@ -186,6 +186,33 @@ EOF
test_cmp expect actual
 '
 
+test_expect_failure '.git/shallow is edited by repack' '
+   git init shallow-server &&
+   test_commit -C shallow-server A &&
+   test_commit -C shallow-server B &&
+   git -C shallow-server checkout -b branch &&
+   test_commit -C shallow-server C &&
+   test_commit -C shallow-server E &&
+   test_commit -C shallow-server D &&
+   d="$(git -C shallow-server rev-parse --verify D^0)" &&
+   git -C shallow-server checkout master &&
+
+   git clone --depth=1 --no-tags --no-single-branch \
+   "file://$PWD/shallow-server" shallow-client &&
+
+   : now remove the branch and fetch with prune &&
+   git -C shallow-server branch -D branch &&
+   git -C shallow-client fetch --prune --depth=1 \
+   origin "+refs/heads/*:refs/remotes/origin/*" &&
+   git -C shallow-client repack -adfl &&
+   test_must_fail git -C shallow-client rev-parse --verify $d^0 &&
+   ! grep $d shallow-client/.git/shallow &&
+
+   git -C shallow-server branch branch-orig $d &&
+   git -C shallow-client fetch --prune --depth=2 \
+   origin "+refs/heads/*:refs/remotes/origin/*"
+'
+
 . "$TEST_DIRECTORY"/lib-httpd.sh
 start_httpd
 
-- 
gitgitgadget

Recommended configurations (was Re: [PATCH v1 2/2] reset: add new reset.quietDefault config setting)

2018-10-24 Thread Derrick Stolee

On 10/23/2018 4:03 PM, Ævar Arnfjörð Bjarmason wrote:

[snip]
The --ahead-behind config setting stalled on-list before:
https://public-inbox.org/git/36e3a9c3-f7e2-4100-1bfc-647b809a0...@jeffhostetler.com/

Now we have this similarly themed thing.

I think we need to be mindful of how changes like this can add up to
very confusing UI. I.e. in this case I can see a "how take make git fast
on large repos" post on stackoverflow in our future where the answer is
setting a bunch of seemingly irrelevant config options like reset.quiet
and status.aheadbehind=false etc.

So maybe we should take a step back and consider if the real thing we
want is just some way for the user to tell git "don't work so hard at
coming up with these values".

That can also be smart, e.g. some "auto" setting that tweaks it based on
estimated repo size so even with the same config your tiny dotfiles.git
will get "ahead/behind" reporting, but not when you cd into windows.git.

Generally, there are a lot of config settings that are likely in the "if
you have a big repo, then you should use this" category. However, there
is rarely a one-size-fits-all solution to these problems, just like
there are different ways a repo can be "big" (working directory? number
of commits? submodules?).

I would typically expect that users with _really_ big repos have the
resources to have an expert tweak the settings that are best for that
data shape and share those settings with all the users. In VFS for Git,
we turn certain config settings on by default when mounting the repo
[1], but others are either signaled through warning messages (like the
status.aheadBehind config setting [2]).

We never upstreamed the status.aheadBehind config setting [2], but we
_did_ upstream the command-line option as fd9b544 "status: add
--[no-]ahead-behind to status and commit for V2 format". We didn't want
to change the expected output permanently, so we didn't add the config
setting to our list of "required" settings, but instead created a list
of optional settings [3]; these settings don't override the existing
settings so users can opt-out. (Now that we have the commit-graph
enabled and kept up-to-date, it may be time to revisit the importance of
this setting.)

All of this is to say: it is probably a good idea to have some
"recommended configuration" for big repos, but there will always be
power users who want to tweak each and every one of these settings. I'm
open to design ideas of how to store a list of recommended
configurations and how to set a group of config settings with one
command (say, a "git recommended-config [small|large|submodules]"
builtin that fills the local config with the important settings).

Thanks,
-Stolee

[1]
https://github.com/Microsoft/VFSForGit/blob/7daa9f1133764a4e4bd87014833fc2091e6702c1/GVFS/GVFS/CommandLine/GVFSVerb.cs#L79-L104

Code in VFS for Git that enables "required" config settings.

[2]
https://github.com/Microsoft/git/commit/0cbe9e6b23e4d9008d4a1676e1dd6a87bdcd6ed5

status: add status.aheadbehind setting

[3]
https://github.com/Microsoft/VFSForGit/blob/7daa9f1133764a4e4bd87014833fc2091e6702c1/GVFS/GVFS/CommandLine/GVFSVerb.cs#L120-L123

Code in VFS for Git that enables "optional" config settings.

Re: bug?: git grep HEAD with exclude in pathspec not taken into account

2018-10-24 Thread Christophe Bliard

In fact, per 
https://github.com/git/git/commit/859b7f1d0e742493d2a9396794cd9040213ad846,
having only a negative pattern is like having a catch-all positive
pattern and the negative pattern (since git 2.13.0).

Thus, adding the positive pattern yields the same results:

> git --no-pager grep foo HEAD -- ':!fileA' .
HEAD:fileB:foo is false+
> git --no-pager grep foo HEAD -- ':!fileB' .
HEAD:fileA:foo
HEAD:fileB:foo is false+

Le mer. 24 oct. 2018 à 17:14, Duy Nguyen  a écrit :
>
> On Wed, Oct 24, 2018 at 4:55 PM Christophe Bliard
>  wrote:
> >
> > Hi,
> >
> > I observed an unexpected behavior while using git grep with both git
> > 2.19.1 and 2.14.3. Here is how to reproduce it:
> >
> > > git init
> > Initialized empty Git repository in /private/tmp/hello/.git/
> > > echo foo > fileA
> > > echo 'foo is false+' > fileB
> > > git add fileA
> > > git commit -m fileA
> > [master (root-commit) f2c83e7] fileA
> >  1 file changed, 1 insertion(+)
> >  create mode 100644 fileA
> > > git add fileB
> > > git commit -m fileB
> > [master e35df26] fileB
> >  1 file changed, 1 insertion(+)
> >  create mode 100644 fileB
> > > git --no-pager grep foo HEAD -- ':!fileA'
> > HEAD:fileB:foo is false+
> > > git --no-pager grep foo HEAD -- ':!fileB'
> > HEAD:fileA:foo
> > HEAD:fileB:foo is false+
> >
> > In the last command, fileB appears in grep results but it should have
> > been excluded.
> >
> > If the HEAD parameter is removed, it works as expected:
>
> It's probably a bug. We have different code paths for matching things
> with or without HEAD, roughly speaking. I'll look into to this more on
> the weekend, unless somebody (is welcome to) beats me first.
>
> Another thing that might also be a problem is, negative patterns are
> supposed to exclude stuff from "something" but you don't specify that
> "something" (i.e. positive patterns) in your grep commands above. If
> "grep foo HEAD -- :/ ':!fileB'" works, then you probably just run into
> some undefined corner case.
> --
> Duy

Re: [PATCH v2 0/2] Work around case-insensitivity issues with cwd on Windows

Hi Junio,

On Wed, 24 Oct 2018, Junio C Hamano wrote:

> "Johannes Schindelin via GitGitGadget" 
> writes:
> 
> > Changes since v1:
> >
> >  * Fixed a grammar mistake in the second commit message.
> 
> Thanks.  I think this matches what I earlier queued on 'pu' with
> Stephen's typofix squashed in, so we are good already.

Perfect. Sorry that I forgot to check.

Ciao,
Dscho

Re: [PATCH 1/3] [Outreachy] t3903-stash: test without configured user name

Hi Junio,

On Wed, 24 Oct 2018, Junio C Hamano wrote:

> Johannes Schindelin  writes:
> 
> > On Wed, 24 Oct 2018, Junio C Hamano wrote:
> >
> >> Slavica  writes:
> >> 
> >> > +test_expect_failure 'stash with HOME as non-existing directory' '
> >> > +test_commit 1 &&
> >> > +test_config user.useconfigonly true &&
> >> > +test_config stash.usebuiltin true &&
> >> > +(
> >> > +HOME=$(pwd)/none &&
> >> > +export HOME &&
> >> 
> >> What is the reason why this test needs to move HOME away from
> >> TRASH_DIRECTORY (set in t/test-lib.sh)?
> >
> > This is to make sure that no user.name nor user.email is configured. That
> > was my idea, hence I answer your question.
> 
> HOME is set to TRASH_DIRECTORY in t/test-lib.sh already, and we do
> so to avoid getting affected by the real $HOME/.gitconfig of the
> user who happens to be running the test suite.

My bad. I should have checked. I was under the impression that we set
`HOME` to the `t/` directory and initialized it. But you are right, of
course, and that subshell as well as the override of `HOME` are absolutely
unnecessary.

Thanks,
Dscho

> 
> With that in place for ages, this test still moves HOME away from
> TRASH_DIRECTORY, and that is totally unnecessary if it is only done
> to avoid getting affected by the real $HOME/.gitconfig of the user.
> After all, this single test is not unique in its need to avoid
> reading from user's $HOME/.gitconfig---so I expected there may be
> other reasons.
> 
> That is why I asked, and your response is not quite answering that
> question.
>

Re: bug?: git grep HEAD with exclude in pathspec not taken into account

On Wed, Oct 24, 2018 at 4:55 PM Christophe Bliard
 wrote:
>
> Hi,
>
> I observed an unexpected behavior while using git grep with both git
> 2.19.1 and 2.14.3. Here is how to reproduce it:
>
> > git init
> Initialized empty Git repository in /private/tmp/hello/.git/
> > echo foo > fileA
> > echo 'foo is false+' > fileB
> > git add fileA
> > git commit -m fileA
> [master (root-commit) f2c83e7] fileA
>  1 file changed, 1 insertion(+)
>  create mode 100644 fileA
> > git add fileB
> > git commit -m fileB
> [master e35df26] fileB
>  1 file changed, 1 insertion(+)
>  create mode 100644 fileB
> > git --no-pager grep foo HEAD -- ':!fileA'
> HEAD:fileB:foo is false+
> > git --no-pager grep foo HEAD -- ':!fileB'
> HEAD:fileA:foo
> HEAD:fileB:foo is false+
>
> In the last command, fileB appears in grep results but it should have
> been excluded.
>
> If the HEAD parameter is removed, it works as expected:

It's probably a bug. We have different code paths for matching things
with or without HEAD, roughly speaking. I'll look into to this more on
the weekend, unless somebody (is welcome to) beats me first.

Another thing that might also be a problem is, negative patterns are
supposed to exclude stuff from "something" but you don't specify that
"something" (i.e. positive patterns) in your grep commands above. If
"grep foo HEAD -- :/ ':!fileB'" works, then you probably just run into
some undefined corner case.
-- 
Duy

Re: Translate ProGit v2 into vietnamese

On Wed, Oct 24, 2018 at 2:16 PM rk42_gg  wrote:
>
> Dear Git Developer,
>
> I have started translating Pro Git v2 book into Vietnamese at
> https://github.com/rocket42/progit2-vi

Great!

> Pls show me how to add it to "*Translations started for*" section in
> https://git-scm.com/book/en/v2

This is the wrong place for this though because the book is maintained
separately. At the bottom left of that page you can see the link
"hosted on GitHub", which leads me to

https://github.com/progit/progit2/blob/master/TRANSLATING.md

Start from there and maybe open pull requests (or issues for support)
on that project. Good luck.
-- 
Duy

Re: [PATCH v3 2/3] reset: add new reset.quiet config setting

On Wed, Oct 24, 2018 at 4:56 AM Junio C Hamano  wrote:
> How we should get there is a different story.  I think Duy's series
> needs at least another update to move the split pieces into its own
> subdirectory of Documentation/, and it is not all that urgent, while
> this three-patch series (with the advice.* bit added) for "reset" is
> pretty much ready to go 'next', so my gut feeling is that it is best
> to keep the description here, and to ask Duy to base the updated
> version of config-split topic on top.

OK. Just to be sure we're on the same page. Am I waiting for all
config changes to land in 'master', or do I rebase my series on
'next'? I usually base on 'master' but the mention of 'next' here
confuses me a bit.
-- 
Duy

Re: bug?: git grep HEAD with exclude in pathspec not taken into account

2018-10-24 Thread Christophe Bliard

Hi,

I observed an unexpected behavior while using git grep with both git
2.19.1 and 2.14.3. Here is how to reproduce it:

> git init
Initialized empty Git repository in /private/tmp/hello/.git/
> echo foo > fileA
> echo 'foo is false+' > fileB
> git add fileA
> git commit -m fileA
[master (root-commit) f2c83e7] fileA
 1 file changed, 1 insertion(+)
 create mode 100644 fileA
> git add fileB
> git commit -m fileB
[master e35df26] fileB
 1 file changed, 1 insertion(+)
 create mode 100644 fileB
> git --no-pager grep foo HEAD -- ':!fileA'
HEAD:fileB:foo is false+
> git --no-pager grep foo HEAD -- ':!fileB'
HEAD:fileA:foo
HEAD:fileB:foo is false+

In the last command, fileB appears in grep results but it should have
been excluded.

If the HEAD parameter is removed, it works as expected:

> git --no-pager grep foo -- ':!fileB'
fileA:foo

If giving the revision, it does not work either

> git --no-pager grep foo e35df26 -- ':!fileB'
e35df26:fileA:foo
e35df26:fileB:foo is false+

The same behavior can be seen with git archive:

> git archive --verbose master ':(top)'
fileA
fileB
pax_global_header66600064133641017230014512gustar00rootroot0052
comment=e35df26c65f3c0b303e78743496598b8b6a566e9
fileA66441336410172300120130ustar00rootroot00foo
fileB664000161336410172300120170ustar00rootroot00foo
is false+
> /usr/local/bin/git archive --verbose master ':(top)' ':(exclude)fileA'
fileB
pax_global_header66600064133641017230014512gustar00rootroot0052
comment=e35df26c65f3c0b303e78743496598b8b6a566e9
fileB664000161336410172300120170ustar00rootroot00foo
is false+
> /usr/local/bin/git archive --verbose master ':(top)' ':(exclude)fileB'
fileA
fileB
pax_global_header66600064133641017230014512gustar00rootroot0052
comment=e35df26c65f3c0b303e78743496598b8b6a566e9
fileA66441336410172300120130ustar00rootroot00foo
fileB664000161336410172300120170ustar00rootroot00foo
is false+

fileA can be excluded, but not fileB.

Is it a bug?

Thanks

--
Christophe Bliard

Re: [PATCH v3 2/3] reset: add new reset.quiet config setting

On Tue, Oct 23, 2018 at 8:47 PM Ben Peart  wrote:
>
>
>
> On 10/22/2018 10:45 AM, Duy Nguyen wrote:
> > On Mon, Oct 22, 2018 at 3:38 PM Ben Peart  wrote:
> >>
> >> From: Ben Peart 
> >>
> >> Add a reset.quiet config setting that sets the default value of the --quiet
> >> flag when running the reset command.  This enables users to change the
> >> default behavior to take advantage of the performance advantages of
> >> avoiding the scan for unstaged changes after reset.  Defaults to false.
> >>
> >> Signed-off-by: Ben Peart 
> >> ---
> >>   Documentation/config.txt| 3 +++
> >>   Documentation/git-reset.txt | 4 +++-
> >>   builtin/reset.c | 1 +
> >>   3 files changed, 7 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/Documentation/config.txt b/Documentation/config.txt
> >> index f6f4c21a54..a2d1b8b116 100644
> >> --- a/Documentation/config.txt
> >> +++ b/Documentation/config.txt
> >> @@ -2728,6 +2728,9 @@ rerere.enabled::
> >>  `$GIT_DIR`, e.g. if "rerere" was previously used in the
> >>  repository.
> >>
> >> +reset.quiet::
> >> +   When set to true, 'git reset' will default to the '--quiet' option.
> >> +
> >
> > With 'nd/config-split' topic moving pretty much all config keys out of
> > config.txt, you probably want to do the same for this series: add this
> > in a new file called Documentation/reset-config.txt then include the
> > file here like the sendemail one below.
> >
>
> Seems a bit overkill to pull a line of documentation into a separate
> file and replace it with a line of 'import' logic.  Perhaps if/when
> there is more documentation to pull out that would make more sense.

There are a couple benefits of having all config keys stored in the
same way (i.e. in separate files). Searching will be easier, you
_know_ reset.stuff will be in reset-config.txt. If you mix both ways,
you may need to look in config.txt as well as searching
reset-config.txt. This single config key also stands out when you look
at the end result of nd/config-split. The config split also opens up
an opportunity to include command-specific config in individual
command man page if done consistently.
-- 
Duy

Re: [PATCH] Poison gettext with the Ook language

On Tue, Oct 23, 2018 at 6:45 PM Ævar Arnfjörð Bjarmason
 wrote:
> >> The effect of what I'm suggesting here, and which my WIP patch in
> >> <875zxtd59e@evledraar.gmail.com> implements is that we'd do a
> >> one-time getenv() for each process that prints a _() message that we
> >> aren't doing now, and for each message call a function that would check
> >> a boolean "are we in poison mode" static global variable.
> >
> > Just don't do the getenv() inside _() even if you just do it one time.
> > getenv() may invalidate whatever value from the previous call. I would
> > not want to find out my code broke because of an getenv() inside some
> > innocent _()...
>
> How would any code break? We have various getenv("GIT_TEST_*...")
> already that work the same way. Unless you set that in the environment
> the function is idempotent, and I don't see how anyone would do that
> accidentally.

I didn't check those GIT_TEST_ but I think the difference is in
complexity. When you write

 var = getenv("foo");
 complexFunction();

you probably start to feel scary and wrap getenv() with a strdup()
because you usually don't know exactly what complexFunction() can call
(and even if you do, you can't be sure what it may call in the
future).

The person who writes

 printf(_("%s"), getenv("foo"));

may not go through the same thought process as with complexFunction().
If _() calls getenv(), because you the order of parameter evaluation
is unspecified, you cannot be sure if getenv("foo") will be called
before or after the one inside _(). One of them may screw up the
other.

> > And we should still respect NO_GETTEXT, not doing any extra work when
> > it's defined.
>
> This is not how any of our NO_* defines work. *Sometimes* defining them
> guarantees you do no extra work, but in other cases we've decided that
> bending over backwards with #ifdef in various places isn't worth it.

I guess it boils down to "worth it". For me #ifdef NO_GETTEXT would be
limited to gettext.h and it's not that much work. But you do the
actual work. You decide.
-- 
Duy

Re: [PATCH 1/3] [Outreachy] t3903-stash: test without configured user name

2018-10-24 Thread Slavica




On 23-Oct-18 8:52 PM, Christian Couder wrote:

On Tue, Oct 23, 2018 at 6:35 PM Slavica  wrote:

This is part of enhancement request that ask for `git stash` to work even if 
`user.name` is not configured.
The issue is discussed here: 
https://public-inbox.org/git/87o9debty4@evledraar.gmail.com/T/#u.

We prefer commit messages that contain as much as possible all the
information necessary to understand the patch without links to other
places.

It seems that only this email from you reached me. Did you send other
emails for patches 2/3 and 3/3?

[...]


Okay, I will change that. This is my first patch and I am still adapting.

Emails for patches 2/3 and 3/3 because aren't there because I am still 
preparing them.


(I didn't know if I had 3 patches in plan that they should be sent at 
almost the same time.)





+(
+HOME=$(pwd)/none &&
+export HOME &&
+unset GIT_AUTHOR_NAME &&
+unset GIT_AUTHOR_EMAIL &&
+unset GIT_COMMITTER_NAME &&
+unset GIT_COMMITTER_EMAIL &&
+test_must_fail git config user.email &&
+echo changed >1.t &&
+   git stash

It seems that the above line is not indented like the previous ones.
I don't know what is the reason, in my IDE everything seems fine, but 
I'll fix it.



+)
+'

Thanks for contributing,
Christian.


You are welcome,

Slavica

Re: [PATCH v3 09/12] commit-graph: convert to using the_hash_algo

2018-10-24 Thread Derrick Stolee


On 10/21/2018 10:43 PM, brian m. carlson wrote:

Instead of using hard-coded constants for object sizes, use
the_hash_algo to look them up.  In addition, use a function call to look
up the object ID version and produce the correct value.  For now, we use
version 1, which means to use the default algorithm used in the rest of
the repository.


This patch looks good to me. Later, we will want to delete the 
commit-graph file during the "upgrade the repo to newhash" process, but 
that can wait.


Thanks,
-Stolee

Re: [PATCH] commit-reach: fix sorting commits by generation

2018-10-24 Thread Derrick Stolee


On 10/23/2018 4:32 PM, Thomas Gummerer wrote:

On 10/22, René Scharfe wrote:

Am 22.10.2018 um 23:10 schrieb Thomas Gummerer:

Anyway, your implied question was discussed back then.  Derrick wrote:

The reason to sort is to hopefully minimize the amount we walk by
exploring the "lower" commits first. This is a performance-only thing,
not a correctness issue (which is why the bug exists). Even then, it is
just a heuristic.

Thanks for pointing that out!


Does b6723e4671 in pu (commit-reach: fix first-parent heuristic) change
that picture?  Did a quick test and found no performance difference with
and without the fix on top, i.e. proper sorting didn't seem to matter.

I just gave 'test-tool reach can_all_from_reach' a try and got the
same results, with or without the fix the times are very similar.  I
haven't had time to follow the commit-graph series though, so I'm not
sure I used it correctly.


Thanks for your attention here. I've been thinking a lot about this 
heuristic and have concluded the following two things are true:


(1) When we return 1, the order that we explore the 'from' commits does 
not change the explored set of commits.


(2) When we return 0, the order that we explore the 'to' commits will 
change the explored set, but it is difficult to say that the heuristic 
helps more than it hurts.


Item (1) is contrary to what I had thought when I first created the 
heuristic.


The details are tricky, but essentially each DFS starting at a 'from' 
commit may be short-circuited due to a prior walk, but swapping the 
order of those two 'from' commits would lead to the same set of commits 
to be explored (with the short-circuit happening in the other commit). 
The only change is that we can terminate early if we fully explore a 
'from' commit and do not find a commit marked with 'with_flag'. In this 
sense, it would be best to explore the commits that are "closest" to the 
generation number cutoff first, as we can maybe find a negative answer 
earlier in the search.


In this sense, we could remove the sort entirely and probably not have 
much performance hit. But since the set of 'from' commits is probably 
much smaller than the set of commits to explore, the sort is likely 
inexpensive.


In conclusion: I cannot say that this sort is super-important. As for 
the potential benefits in (2), I'll leave that to people who run git as 
a server who may have telemetry around fetch negotiation. How often do 
we actually say we need more rounds of negotiation? What kinds of data 
shapes matter there?


Thanks,
-Stolee

Re: [RFC] cherry-pick notes to find out cherry-picks from the origin

2018-10-24 Thread Tejun Heo

Ping, thanks.

-- 
tejun

Translate ProGit v2 into vietnamese

2018-10-24 Thread rk42_gg


Dear Git Developer,

I have started translating Pro Git v2 book into Vietnamese at 
https://github.com/rocket42/progit2-vi


Pls show me how to add it to "*Translations started for*" section in 
https://git-scm.com/book/en/v2


Thank you very much!

--


Full Name: Nguyễn Văn Hùng
Gmail/Skype/yahoo:hung.rocket42
Address: 259/35/2 Phú diễn street - Bắc Từ Liêm district- Hà Nội city
Mobile:+84986451381

Re: [PATCHv2 0/3] git-p4: improved unshelving - small fixes

2018-10-24 Thread Luke Diamand

On Tue, 16 Oct 2018 at 05:27, Junio C Hamano  wrote:
>
> Luke Diamand  writes:
>
> > This is the same as my earlier patch other than fixing the
> > documentation to reflect the change to the destination branch,
> > as noticed by Junio.
>
> Also you set up when-finished driver for clean-up before running
> clone, which I think is a good change, too.
>
> Will replace.  Thanks.

At the moment the parent commit file revisions are constructed by
p4-printing the relevant files at the required version. That's because
it's easy to do - I can just make use of the existing infrastructure
for grabbing P4 changes.

However, in most cases we will already have these files present in the
git repo - it's just that we don't know where. I think it ought to be
possible to look at the revisions of each file, and then search
through the git revision history for that file to find the magic
comment marker that git-p4 inserts (which has the P4 changelist
embedded in it) and then use that - rather than going back to the
Perforce depot.

In most cases that ought to be quite a bit faster, especially for large files.

For now I'm not proposing to do this as it's quite a bit more work
(this isn't really my day job!) and for the typical use case (at least
the ones I encounter) the performance of unshelving isn't really that
important - the fact that it does it at all is quite amazing.

But anyway, if someone somewhere finds that git-p4 unshelve
performance is too slow, then that's the change to make.

Luke

[PATCH v2] i18n: make GETTEXT_POISON a runtime option

2018-10-24 Thread Ævar Arnfjörð Bjarmason

Change the GETTEXT_POISON compile-time + runtime GIT_GETTEXT_POISON
test parameter to only be a GIT_TEST_GETTEXT_POISON= runtime
parameter, to be consistent with other parameters documented in
"Running tests with special setups" in t/README.

When I added GETTEXT_POISON in bb946bba76 ("i18n: add GETTEXT_POISON
to simulate unfriendly translator", 2011-02-22) I was concerned with
ensuring that the _() function would get constant folded if NO_GETTEXT
was defined or if it wasn't and GETTEXT_POISON wasn't defined.

But as the benchmark in my [1] shows doing a one-off runtime
getenv("GIT_TEST_[...]") is trivial, and since GETTEXT_POISON was
originally added this has become the common idiom in the codebase for
turning on special test setups.

So change GETTEXT_POISON to work the same way. Now the
GETTEXT_POISON=YesPlease compile-time option is gone, and running the
tests with GIT_TEST_GETTEXT_POISON=[true|false] can be toggled on/off
without recompiling.

This allows for conditionally amending tests to test with/without
poison, similar to what 859fdc0c3c ("commit-graph: define
GIT_TEST_COMMIT_GRAPH", 2018-08-29) did for GIT_TEST_COMMIT_GRAPH. Do
some of that, now we e.g. always run the t0205-gettext-poison.sh test.

I did enough there to remove the GETTEXT_POISON prerequisite, but its
inverse C_LOCALE_OUTPUT is still around, and surely some tests using
it can be converted to e.g. always set GIT_TEST_GETTEXT_POISON=false.

Notes on the implementation:

 * The only reason we need a new "git-sh-i18n--helper" is to expose
   git_env_bool() to shellscripts, since git-sh-i18n and friends need
   to inspect the $GIT_TEST_GETTEXT_POISON variable.

   We only call it if $GIT_TEST_GETTEXT_POISON is set, or in the test
   suite. This code can go away for non-test code once the last
   i18n-using shellscript is rewritten in C, or if "git config" learns
   to combine --bool and a value fed into it, see [2].

 * We still compile a dedicated GETTEXT_POISON build in Travis CI,
   this is probably the wrong thing to do and should be followed-up
   with something similar to ae59a4e44f ("travis: run tests with
   GIT_TEST_SPLIT_INDEX", 2018-01-07) to re-use an existing test setup
   for running in the GIT_TEST_GETTEXT_POISON mode.

 * The reason for not doing:

   test_lazy_prereq C_LOCALE_OUTPUT [...]

   in test-lib.sh is because there's some interpolation problem with
   that syntax which makes t6040-tracking-info.sh fail. Hence using
   the simpler test_set_prereq. It'll fail with:

   + git branch -vv
   + sed -n -e
   mkdir -p "$TRASH_DIRECTORY/prereq-test-dir" &&
   (
   cd "$TRASH_DIRECTORY/prereq-test-dir" &&! git sh-i18n--helper 
--git-test-gettext-poison
   )
   sed: -e expression #1, char 2: unknown command: `m'
   error: last command exited with $?=1
   not ok 3 - branch -vv

   This is some interpolation problem with how test_lazy_prereq works
   that doesn't affect test_set_prereq.

 * We now skip a test in t-basic.sh under
   GIT_TEST_GETTEXT_POISON=true that wasn't skipped before. This test
   relies on C locale output, but due to an edge case in how the
   previous implementation of GETTEXT_POISON worked (reading it from
   GIT-BUILD-OPTIONS) wasn't enabling poison correctly. Now it does,
   and needs to be skipped.

See also
https://public-inbox.org/git/878t2pd6yu@evledraar.gmail.com/ for
more discussion.

1. https://public-inbox.org/git/871s8gd32p@evledraar.gmail.com/

2. https://public-inbox.org/git/20181024074400.ga31...@sigill.intra.peff.net/

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

On Wed, Oct 24 2018, Junio C Hamano wrote:

> Ævar Arnfjörð Bjarmason   writes:
>
>> Notes on the implementation:
>>
>>  * The only reason we need a new "git-sh-i18n--helper" and the
>>corresponding "test-tool gettext-poison" is to expose
>>git_env_bool() to shellscripts, since git-sh-i18n and friends need
>>to inspect the $GIT_TEST_GETTEXT_POISON variable.
>>
>>We only call these if $GIT_TEST_GETTEXT_POISON is set, or in the
>>test suite, and this code can go away for non-test code once the
>>last i18n-using shellscript is rewritten in C.
>
> Makes me wonder if we want to teach "git config" a new "--env-bool"
> option that can be used by third-party scripts.  Or would it be just
> the matter of writing
>
>   git config --default=false --type=bool "$GIT_WHATEVER_ENV"
>
> in these third-party scripts and we do not need to add such a thing?

As Jeff notes in a follow-up reply git-config can't quite do this
yet. So in lieu of implementing that I have a more minimal helper,
which could be migrated to something like the --get-env mode Jeff is
talking about if we add that.

>>  * The reason for not doing:
>>
>>test_lazy_prereq GETTEXT_POISON 'test-tool gettext-poison'
>>test_lazy_prereq C_LOCALE_OUTPUT '! test-tool gettext-poison'
>>
>>In test-lib.sh is because there's some interpolation problem with
>>that syntax which

Contact Mr. Alfred Brunson (view email attachment)

2018-10-24 Thread Le Quang Minh

 <> 
 


Congratulation.docx
Description: Binary data

[ANNOUNCE] Git Rev News edition 44

2018-10-24 Thread Christian Couder

Hi everyone,

The 44th edition of Git Rev News is now published:

  https://git.github.io/rev_news/2018/10/24/edition-44/

Enjoy,
Christian, Jakub, Markus and Gabriel.

Re: [PATCH v2 0/2] Work around case-insensitivity issues with cwd on Windows

"Johannes Schindelin via GitGitGadget" 
writes:

> Changes since v1:
>
>  * Fixed a grammar mistake in the second commit message.

Thanks.  I think this matches what I earlier queued on 'pu' with
Stephen's typofix squashed in, so we are good already.

Re: [PATCH 1/3] [Outreachy] t3903-stash: test without configured user name

Johannes Schindelin  writes:

> Hi Junio,
>
> On Wed, 24 Oct 2018, Junio C Hamano wrote:
>
>> Slavica  writes:
>> 
>> > +test_expect_failure 'stash with HOME as non-existing directory' '
>> > +test_commit 1 &&
>> > +test_config user.useconfigonly true &&
>> > +test_config stash.usebuiltin true &&
>> > +(
>> > +HOME=$(pwd)/none &&
>> > +export HOME &&
>> 
>> What is the reason why this test needs to move HOME away from
>> TRASH_DIRECTORY (set in t/test-lib.sh)?
>
> This is to make sure that no user.name nor user.email is configured. That
> was my idea, hence I answer your question.

HOME is set to TRASH_DIRECTORY in t/test-lib.sh already, and we do
so to avoid getting affected by the real $HOME/.gitconfig of the
user who happens to be running the test suite.

With that in place for ages, this test still moves HOME away from
TRASH_DIRECTORY, and that is totally unnecessary if it is only done
to avoid getting affected by the real $HOME/.gitconfig of the user.
After all, this single test is not unique in its need to avoid
reading from user's $HOME/.gitconfig---so I expected there may be
other reasons.

That is why I asked, and your response is not quite answering that
question.

Re: [PATCH] Clear --exclude list after 'git rev-parse --all'

2018-10-24 Thread Andreas Gruenbacher

On Wed, 24 Oct 2018 at 11:24, Junio C Hamano  wrote:
> Andreas Gruenbacher  writes:
> >> All other glob options do show_reference with for_each_ref_in() and
> >> then calls clear_ref_exclusion(), and logically the patch makes
> >> sense.
> >>
> >> What is the "problem" this patch fixes, though?  Is it easy to add a
> >> new test to t/6018-rev-list-glob.sh to demonstrate that "--glob" (or
> >> whatever that clears exclusion list without this patch) works
> >> correctly but "--all" misbehaves without this change?
> >
> > The test suite doesn't cover clearing the exclusion list for any of
> > those rev-parse options and I also didn't write such a test case. I
> > ran into this inconsistency during code review.
>
> That is why I asked what "problem" this patch fixes.  Without
> answering that question, it is unclear if the patch is completing
> missing coverage for "--all", or it is cargo culting an useless
> clearing done for "--glob" and friends to code for "--all" that did
> not do the same useless clearing.

This is how the --exclude option is described:

   --exclude=
   Do not include refs matching  that the next
--all, --branches,
   --tags, --remotes, or --glob would otherwise consider.
Repetitions of this
   option accumulate exclusion patterns up to the next --all,
--branches, --tags,
   --remotes, or --glob option (other options or arguments do not clear
   accumulated patterns).

I'm assuming that some thought has gone into making the options behave
in this particular way. The implementation in revision.c follows this
pattern, but the implementation in builtin/rev-parse.c only almost
does.

Andreas

Re: [PATCH 2/2] run-command: mark path lookup errors with ENOENT

Hi Peff,

On Wed, 24 Oct 2018, Jeff King wrote:

> On Wed, Oct 24, 2018 at 11:01:54AM +0200, Johannes Schindelin wrote:
> 
> > > @@ -910,6 +921,7 @@ int start_command(struct child_process *cmd)
> > >  }
> > >  #endif
> > >  
> > > +end_of_spawn:
> > 
> > Sadly, this fails to build on Windows:
> > 
> > run-command.c: In function 'start_command':
> > run-command.c:924:1: error: label 'end_of_spawn' defined but not used 
> > [-Werror=unused-label]
> >  end_of_spawn:
> >  ^~~~
> 
> Doh. I didn't think of that.
> 
> > How about squashing in this diff:
> > 
> > -- snip --
> > diff --git a/run-command.c b/run-command.c
> > index 639ea5ac3366..3f03795a5995 100644
> > --- a/run-command.c
> > +++ b/run-command.c
> > @@ -918,6 +918,8 @@ int start_command(struct child_process *cmd)
> > close(fhout);
> > if (fherr != 2)
> > close(fherr);
> > +
> > +   goto end_of_spawn;
> >  }
> >  #endif
> >  
> > -- snap --
> > 
> > I can confirm that the result compiles and passes t0061.
> 
> That leaves the Windows side of the #else with a funny, useless goto
> (and without even a matching useless one on the Unix side).  Let's put
> it instead inside the half of the #if that actually uses it. Like so
> (actually courtesy of Jonathan Nieder):
> 
> diff --git a/run-command.c b/run-command.c
> index 639ea5ac33..d679cc267c 100644
> --- a/run-command.c
> +++ b/run-command.c
> @@ -868,6 +868,8 @@ int start_command(struct child_process *cmd)
>   argv_array_clear();
>   free(childenv);
>  }
> +end_of_spawn:
> +
>  #else
>  {
>   int fhin = 0, fhout = 1, fherr = 2;
> @@ -921,7 +923,6 @@ int start_command(struct child_process *cmd)
>  }
>  #endif
>  
> -end_of_spawn:
>   if (cmd->pid < 0) {
>   if (need_in)
>   close_pair(fdin);

That works for me, too.

> Thanks for your review!

You're welcome!

Thanks,
Dscho

> 
> -Peff
>

Re: [PATCH] Clear --exclude list after 'git rev-parse --all'

Andreas Gruenbacher  writes:

>> All other glob options do show_reference with for_each_ref_in() and
>> then calls clear_ref_exclusion(), and logically the patch makes
>> sense.
>>
>> What is the "problem" this patch fixes, though?  Is it easy to add a
>> new test to t/6018-rev-list-glob.sh to demonstrate that "--glob" (or
>> whatever that clears exclusion list without this patch) works
>> correctly but "--all" misbehaves without this change?
>
> The test suite doesn't cover clearing the exclusion list for any of
> those rev-parse options and I also didn't write such a test case. I
> ran into this inconsistency during code review.

That is why I asked what "problem" this patch fixes.  Without
answering that question, it is unclear if the patch is completing
missing coverage for "--all", or it is cargo culting an useless
clearing done for "--glob" and friends to code for "--all" that did
not do the same useless clearing.  IOW, there are two ways to address
the "inconsistency", and the proposed log message (nor your answer
above) does not make a convincing argument why adding the same code
to the "--all" side is the right way to achieve consistency---rather
than removing the call to clear from the existing ones.

Thanks.

[PATCH v2 1/2] mingw: ensure `getcwd()` reports the correct case

2018-10-24 Thread Johannes Schindelin via GitGitGadget

From: Johannes Schindelin 

When switching the current working directory, say, in PowerShell, it is
quite possible to use a different capitalization than the one that is
recorded on disk. While doing the same in `cmd.exe` adjusts the
capitalization magically, that does not happen in PowerShell so that
`getcwd()` returns the current directory in a different way than is
recorded on disk.

Typically this creates no problems except when you call

git log .

in a subdirectory called, say, "GIT/" but you switched to "Git/" and
your `getcwd()` reports the latter, then Git won't understand that you
wanted to see the history as per the `GIT/` subdirectory but it thinks you
wanted to see the history of some directory that may have existed in the
past (but actually never did).

So let's be extra careful to adjust the capitalization of the current
directory before working with it.

Reported by a few PowerShell power users ;-)

Signed-off-by: Johannes Schindelin 
---
 compat/mingw.c | 11 +--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/compat/mingw.c b/compat/mingw.c
index 18caf2196..2c3e27ce9 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -917,8 +917,15 @@ struct tm *localtime_r(const time_t *timep, struct tm 
*result)
 
 char *mingw_getcwd(char *pointer, int len)
 {
-   wchar_t wpointer[MAX_PATH];
-   if (!_wgetcwd(wpointer, ARRAY_SIZE(wpointer)))
+   wchar_t cwd[MAX_PATH], wpointer[MAX_PATH];
+   DWORD ret = GetCurrentDirectoryW(ARRAY_SIZE(cwd), cwd);
+
+   if (!ret || ret >= ARRAY_SIZE(cwd)) {
+   errno = ret ? ENAMETOOLONG : err_win_to_posix(GetLastError());
+   return NULL;
+   }
+   ret = GetLongPathNameW(cwd, wpointer, ARRAY_SIZE(wpointer));
+   if (!ret || ret >= ARRAY_SIZE(wpointer))
return NULL;
if (xwcstoutf(pointer, wpointer, len) < 0)
return NULL;
-- 
gitgitgadget

[PATCH v2 0/2] Work around case-insensitivity issues with cwd on Windows

2018-10-24 Thread Johannes Schindelin via GitGitGadget

On Windows, file names are recorded case-sensitively, but looked up
case-insensitively. Therefore, it is possible to switch to a directory by
using incorrect case, e.g. cd documentation will still get you into the 
Documentation subdirectory.

In Powershell, doing so will however report the current directory with the
specified spelling rather than the one recorded on disk, and Git will get
confused.

To remedy that, we fixed this in Git for Windows more than three years ago,
and needed only a small fix a couple of months later to accommodate for the
diverse scenarios encountered by the many Git for Windows users.

Not only to keep the story closer to what happened historically, but also to
make it easier to follow, I refrained from squashing these two patches.

Side note: the second patch is technically not battle-tested for that long:
it uses an API function that requires Windows Vista or later, and we only
recently started to clean up Git for Windows' code to drop fallbacks for
Windows XP. Read: this code used to load the GetFinalPathNameByHandle() 
function dynamically, and that is the only difference to the code that has
been "battle-tested" for close to three years.

Changes since v1:

 * Fixed a grammar mistake in the second commit message.

Anton Serbulov (1):
  mingw: fix getcwd when the parent directory cannot be queried

Johannes Schindelin (1):
  mingw: ensure `getcwd()` reports the correct case

 compat/mingw.c | 50 --
 1 file changed, 48 insertions(+), 2 deletions(-)


base-commit: c4df23f7927d8d00e666a3c8d1b3375f1dc8a3c1
Published-As: 
https://github.com/gitgitgadget/git/releases/tags/pr-54%2Fdscho%2Fmingw-getcwd-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git 
pr-54/dscho/mingw-getcwd-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/54

Range-diff vs v1:

 1:  e13ae2338 = 1:  e13ae2338 mingw: ensure `getcwd()` reports the correct case
 2:  3e3b1c3b1 ! 2:  87ef9ac63 mingw: fix getcwd when the parent directory 
cannot be queried
 @@ -4,7 +4,7 @@
  
  `GetLongPathName()` function may fail when it is unable to query
  the parent directory of a path component to determine the long name
 -for that component. It happens, because of it uses `FindFirstFile()`
 +for that component. It happens, because it uses `FindFirstFile()`
  function for each next short part of path. The `FindFirstFile()`
  requires `List Directory` and `Synchronize` desired access for a 
calling
  process.

-- 
gitgitgadget

[PATCH v2 2/2] mingw: fix getcwd when the parent directory cannot be queried

2018-10-24 Thread Anton Serbulov via GitGitGadget

From: Anton Serbulov 

`GetLongPathName()` function may fail when it is unable to query
the parent directory of a path component to determine the long name
for that component. It happens, because it uses `FindFirstFile()`
function for each next short part of path. The `FindFirstFile()`
requires `List Directory` and `Synchronize` desired access for a calling
process.

In case of lacking such permission for some part of path,
the `GetLongPathName()` returns 0 as result and `GetLastError()`
returns ERROR_ACCESS_DENIED.

`GetFinalPathNameByHandle()` function can help in such cases, because
it requires `Read Attributes` and `Synchronize` desired access to the
target path only.

The `GetFinalPathNameByHandle()` function was introduced on
`Windows Server 2008/Windows Vista`. So we need to load it dynamically.

`CreateFile()` parameters:
`lpFileName` = path to the current directory
`dwDesiredAccess` = 0 (it means `Read Attributes` and `Synchronize`)
`dwShareMode` = FILE_SHARE_READ | FILE_SHARE_WRITE | FILE_SHARE_DELETE
(it prevents `Sharing Violation`)
`lpSecurityAttributes` = NULL (default security attributes)
`dwCreationDisposition` = OPEN_EXISTING
  (required to obtain a directory handle)
`dwFlagsAndAttributes` = FILE_FLAG_BACKUP_SEMANTICS
 (required to obtain a directory handle)
`hTemplateFile` = NULL (when opening an existing file or directory,
`CreateFile` ignores this parameter)

The string that is returned by `GetFinalPathNameByHandle()` function
uses the \\?\ syntax. To skip the prefix and convert backslashes
to slashes, the `normalize_ntpath()` mingw function will be used.

Note: `GetFinalPathNameByHandle()` function returns a final path.
It is the path that is returned when a path is fully resolved.
For example, for a symbolic link named "C:\tmp\mydir" that points to
"D:\yourdir", the final path would be "D:\yourdir".

Signed-off-by: Anton Serbulov 
Signed-off-by: Johannes Schindelin 
---
 compat/mingw.c | 39 +++
 1 file changed, 39 insertions(+)

diff --git a/compat/mingw.c b/compat/mingw.c
index 2c3e27ce9..19addfa5d 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -202,6 +202,31 @@ static int ask_yes_no_if_possible(const char *format, ...)
}
 }
 
+/* Normalizes NT paths as returned by some low-level APIs. */
+static wchar_t *normalize_ntpath(wchar_t *wbuf)
+{
+   int i;
+   /* fix absolute path prefixes */
+   if (wbuf[0] == '\\') {
+   /* strip NT namespace prefixes */
+   if (!wcsncmp(wbuf, L"\\??\\", 4) ||
+   !wcsncmp(wbuf, L"?\\", 4))
+   wbuf += 4;
+   else if (!wcsnicmp(wbuf, L"\\DosDevices\\", 12))
+   wbuf += 12;
+   /* replace remaining '...UNC\' with '\\' */
+   if (!wcsnicmp(wbuf, L"UNC\\", 4)) {
+   wbuf += 2;
+   *wbuf = '\\';
+   }
+   }
+   /* convert backslashes to slashes */
+   for (i = 0; wbuf[i]; i++)
+   if (wbuf[i] == '\\')
+   wbuf[i] = '/';
+   return wbuf;
+}
+
 int mingw_unlink(const char *pathname)
 {
int ret, tries = 0;
@@ -925,6 +950,20 @@ char *mingw_getcwd(char *pointer, int len)
return NULL;
}
ret = GetLongPathNameW(cwd, wpointer, ARRAY_SIZE(wpointer));
+   if (!ret && GetLastError() == ERROR_ACCESS_DENIED) {
+   HANDLE hnd = CreateFileW(cwd, 0,
+   FILE_SHARE_READ | FILE_SHARE_WRITE | FILE_SHARE_DELETE, 
NULL,
+   OPEN_EXISTING, FILE_FLAG_BACKUP_SEMANTICS, NULL);
+   if (hnd == INVALID_HANDLE_VALUE)
+   return NULL;
+   ret = GetFinalPathNameByHandleW(hnd, wpointer, 
ARRAY_SIZE(wpointer), 0);
+   CloseHandle(hnd);
+   if (!ret || ret >= ARRAY_SIZE(wpointer))
+   return NULL;
+   if (xwcstoutf(pointer, normalize_ntpath(wpointer), len) < 0)
+   return NULL;
+   return pointer;
+   }
if (!ret || ret >= ARRAY_SIZE(wpointer))
return NULL;
if (xwcstoutf(pointer, wpointer, len) < 0)
-- 
gitgitgadget

Re: [PATCH 2/2] mingw: fix getcwd when the parent directory cannot be queried

Hi Stephen,

On Tue, 23 Oct 2018, Stephen & Linda Smith wrote:

> On Tuesday, October 23, 2018 3:52:49 AM MST you wrote:
> > From: Anton Serbulov 
> > 
> > `GetLongPathName()` function may fail when it is unable to query
> > the parent directory of a path component to determine the long name
> > for that component. It happens, because of it uses `FindFirstFile()`
> s/of it/it/

Thanks, fixed in v2,
Dscho

> 
> > function for each next short part of path. The `FindFirstFile()`
> > requires `List Directory` and `Synchronize` desired access for a calling
> > process.
> 
> 
> 
> 
> 
>

Re: [PATCH 2/2] run-command: mark path lookup errors with ENOENT

On Wed, Oct 24, 2018 at 11:01:54AM +0200, Johannes Schindelin wrote:

> > @@ -910,6 +921,7 @@ int start_command(struct child_process *cmd)
> >  }
> >  #endif
> >  
> > +end_of_spawn:
> 
> Sadly, this fails to build on Windows:
> 
>   run-command.c: In function 'start_command':
>   run-command.c:924:1: error: label 'end_of_spawn' defined but not used 
> [-Werror=unused-label]
>end_of_spawn:
>^~~~

Doh. I didn't think of that.

> How about squashing in this diff:
> 
> -- snip --
> diff --git a/run-command.c b/run-command.c
> index 639ea5ac3366..3f03795a5995 100644
> --- a/run-command.c
> +++ b/run-command.c
> @@ -918,6 +918,8 @@ int start_command(struct child_process *cmd)
>   close(fhout);
>   if (fherr != 2)
>   close(fherr);
> +
> + goto end_of_spawn;
>  }
>  #endif
>  
> -- snap --
> 
> I can confirm that the result compiles and passes t0061.

That leaves the Windows side of the #else with a funny, useless goto
(and without even a matching useless one on the Unix side).  Let's put
it instead inside the half of the #if that actually uses it. Like so
(actually courtesy of Jonathan Nieder):

diff --git a/run-command.c b/run-command.c
index 639ea5ac33..d679cc267c 100644
--- a/run-command.c
+++ b/run-command.c
@@ -868,6 +868,8 @@ int start_command(struct child_process *cmd)
argv_array_clear();
free(childenv);
 }
+end_of_spawn:
+
 #else
 {
int fhin = 0, fhout = 1, fherr = 2;
@@ -921,7 +923,6 @@ int start_command(struct child_process *cmd)
 }
 #endif
 
-end_of_spawn:
if (cmd->pid < 0) {
if (need_in)
close_pair(fdin);

Thanks for your review!

-Peff

Re: [PATCH 2/2] run-command: mark path lookup errors with ENOENT

Hi Peff,

On Wed, 24 Oct 2018, Jeff King wrote:

> Since commit e3a434468f (run-command: use the
> async-signal-safe execv instead of execvp, 2017-04-19),
> prepare_cmd() does its own PATH lookup for any commands we
> run (on non-Windows platforms).
> 
> However, its logic does not match the old execvp call when
> we fail to find a matching entry in the PATH. Instead of
> feeding the name directly to execv, execvp would consider
> that an ENOENT error. By continuing and passing the name
> directly to execv, we effectively behave as if "." was
> included at the end of the PATH. This can have confusing and
> even dangerous results.

For the record, I tried to come up with an attack vector to exploit this,
and failed to find one.

> The fix itself is pretty straight-forward. There's a new
> test in t0061 to cover this explicitly, and I've also added
> a duplicate of the ENOENT test to ensure that we return the
> correct errno for this case.
> 
> Signed-off-by: Jeff King 
> ---
>  run-command.c  | 20 
>  t/t0061-run-command.sh | 13 -
>  2 files changed, 28 insertions(+), 5 deletions(-)
> 
> diff --git a/run-command.c b/run-command.c
> index 84b883c213..639ea5ac33 100644
> --- a/run-command.c
> +++ b/run-command.c
> @@ -380,7 +380,7 @@ static void child_err_spew(struct child_process *cmd, 
> struct child_err *cerr)
>   set_error_routine(old_errfn);
>  }
>  
> -static void prepare_cmd(struct argv_array *out, const struct child_process 
> *cmd)
> +static int prepare_cmd(struct argv_array *out, const struct child_process 
> *cmd)

I always like when we change functions to return a value that can then
indicate an error, making the libification effort so much easier.

>  {
>   if (!cmd->argv[0])
>   BUG("command is empty");
> @@ -403,16 +403,22 @@ static void prepare_cmd(struct argv_array *out, const 
> struct child_process *cmd)
>   /*
>* If there are no '/' characters in the command then perform a path
>* lookup and use the resolved path as the command to exec.  If there
> -  * are no '/' characters or if the command wasn't found in the path,
> -  * have exec attempt to invoke the command directly.
> +  * are '/' characters, we have exec attempt to invoke the command
> +  * directly.

Nice. I would have probably forgotten about that comment.

>*/
>   if (!strchr(out->argv[1], '/')) {
>   char *program = locate_in_PATH(out->argv[1]);
>   if (program) {
>   free((char *)out->argv[1]);
>   out->argv[1] = program;
> + } else {
> + argv_array_clear(out);
> + errno = ENOENT;
> + return -1;
>   }
>   }
> +
> + return 0;
>  }
>  
>  static char **prep_childenv(const char *const *deltaenv)
> @@ -719,6 +725,12 @@ int start_command(struct child_process *cmd)
>   struct child_err cerr;
>   struct atfork_state as;
>  
> + if (prepare_cmd(, cmd) < 0) {
> + failed_errno = errno;
> + cmd->pid = -1;
> + goto end_of_spawn;
> + }
> +
>   if (pipe(notify_pipe))
>   notify_pipe[0] = notify_pipe[1] = -1;
>  
> @@ -729,7 +741,6 @@ int start_command(struct child_process *cmd)
>   set_cloexec(null_fd);
>   }
>  
> - prepare_cmd(, cmd);
>   childenv = prep_childenv(cmd->env);
>   atfork_prepare();
>  
> @@ -910,6 +921,7 @@ int start_command(struct child_process *cmd)
>  }
>  #endif
>  
> +end_of_spawn:

Sadly, this fails to build on Windows:

run-command.c: In function 'start_command':
run-command.c:924:1: error: label 'end_of_spawn' defined but not used 
[-Werror=unused-label]
 end_of_spawn:
 ^~~~

How about squashing in this diff:

-- snip --
diff --git a/run-command.c b/run-command.c
index 639ea5ac3366..3f03795a5995 100644
--- a/run-command.c
+++ b/run-command.c
@@ -918,6 +918,8 @@ int start_command(struct child_process *cmd)
close(fhout);
if (fherr != 2)
close(fherr);
+
+   goto end_of_spawn;
 }
 #endif
 
-- snap --

I can confirm that the result compiles and passes t0061.

Thanks,
Dscho

>   if (cmd->pid < 0) {
>   if (need_in)
>   close_pair(fdin);
> diff --git a/t/t0061-run-command.sh b/t/t0061-run-command.sh
> index 3e131c5325..cf932c8514 100755
> --- a/t/t0061-run-command.sh
> +++ b/t/t0061-run-command.sh
> @@ -12,10 +12,14 @@ cat >hello-script <<-EOF
>   cat hello-script
>  EOF
>  
> -test_expect_success 'start_command reports ENOENT' '
> +test_expect_success 'start_command reports ENOENT (slash)' '
>   test-tool run-command start-command-ENOENT ./does-not-exist
>  '
>  
> +test_expect_success 'start_command reports ENOENT (no slash)' '
> + test-tool run-command start-command-ENOENT does-not-exist
> +'
> +
>  test_expect_success 'run_command can

Re: [PATCH] Clear --exclude list after 'git rev-parse --all'

2018-10-24 Thread Andreas Gruenbacher

On Wed, 24 Oct 2018 at 06:35, Junio C Hamano  wrote:
> Andreas Gruenbacher  writes:
>
> > Commit [1] added the --exclude option to revision.c.  The --all,
> > --branches, --tags, --remotes, and --glob options clear the exclude
> > list.  Shortly therafter, commit [2] added the same to 'git rev-parse',
> > but without clearing the exclude list for the --all option.  Fix that.
> >
> > [1] e7b432c52 ("revision: introduce --exclude= to tame wildcards", 
> > 2013-08-30)
> > [2] 9dc01bf06 ("rev-parse: introduce --exclude= to tame wildcards", 
> > 2013-11-01)
> >
> > Signed-off-by: Andreas Gruenbacher 
> > ---
> >  builtin/rev-parse.c | 1 +
> >  1 file changed, 1 insertion(+)
>
> All other glob options do show_reference with for_each_ref_in() and
> then calls clear_ref_exclusion(), and logically the patch makes
> sense.
>
> What is the "problem" this patch fixes, though?  Is it easy to add a
> new test to t/6018-rev-list-glob.sh to demonstrate that "--glob" (or
> whatever that clears exclusion list without this patch) works
> correctly but "--all" misbehaves without this change?

The test suite doesn't cover clearing the exclusion list for any of
those rev-parse options and I also didn't write such a test case. I
ran into this inconsistency during code review.

Andreas

Re: [PATCH 1/2] t5410: use longer path for sample script

Hi Peff,

On Wed, 24 Oct 2018, Jeff King wrote:

> t5410 creates a sample script "alternate-refs", and sets
> core.alternateRefsCommand to just "alternate-refs". That
> shouldn't work, as "." is not in our $PATH, and so we should
> not find it.
> 
> However, due to a bug in run-command.c, we sometimes find it
> anyway! Even more confusing, this bug is only in the
> fork-based version of run-command. So the test passes on
> Linux (etc), but fails on Windows.
> 
> In preparation for fixing the run-command bug, let's use a
> more complete path here.
> 
> Reported-by: Johannes Schindelin 
> Signed-off-by: Jeff King 
> ---

Thank you for the fix! I can confirm that the patch works, and the commit
message is stellar, as per usual for your contributions.

BTW since this breaks every single one of our Continuous Builds on
Windows, I would be very much in favor of fast-tracking this to `master`.

Thanks,
Dscho

>  t/t5410-receive-pack-alternates.sh | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/t/t5410-receive-pack-alternates.sh 
> b/t/t5410-receive-pack-alternates.sh
> index 457c20c2a5..f00d0da860 100755
> --- a/t/t5410-receive-pack-alternates.sh
> +++ b/t/t5410-receive-pack-alternates.sh
> @@ -23,7 +23,7 @@ test_expect_success 'with core.alternateRefsCommand' '
>   --format="%(objectname)" \
>   refs/heads/public/
>   EOF
> - test_config -C fork core.alternateRefsCommand alternate-refs &&
> + test_config -C fork core.alternateRefsCommand ./alternate-refs &&
>   git rev-parse public/branch >expect &&
>   printf "" | git receive-pack fork >actual &&
>   extract_haves actual.haves &&
> -- 
> 2.19.1.1094.gd480080bf6
> 
>

Re: [PATCH] upload-pack: fix broken if/else chain in config callback

Hi Peff,

On Wed, 24 Oct 2018, Jeff King wrote:

> The upload_pack_config() callback uses an if/else chain
> like:
> 
>   if (!strcmp(var, "a"))
>  ...
>   else if (!strcmp(var, "b"))
>  ...
>   etc
> 
> This works as long as the conditions are mutually exclusive,
> but one of them is not. 20b20a22f8 (upload-pack: provide a
> hook for running pack-objects, 2016-05-18) added:
> 
>   else if (current_config_scope() != CONFIG_SCOPE_REPO) {
>  ... check some more options ...
>   }
> 
> That was fine in that commit, because it came at the end of
> the chain.  But later, 10ac85c785 (upload-pack: add object
> filtering for partial clone, 2017-12-08) did this:
> 
>   else if (current_config_scope() != CONFIG_SCOPE_REPO) {
>  ... check some more options ...
>   } else if (!strcmp("uploadpack.allowfilter", var))
>  ...
> 
> We'd always check the scope condition first, meaning we'd
> _only_ respect allowfilter when it's in the repo config. You
> can see this with:
> 
>   git -c uploadpack.allowfilter=true upload-pack . | head -1
> 
> which will not advertise the filter capability (but will
> after this patch). We never noticed because:
> 
>   - our tests always set it in the repo config
> 
>   - in protocol v2, we use a different code path that
> actually calls repo_config_get_bool() separately, so
> that _does_ work. Real-world people experimenting with
> this may be using v2.
> 
> The more recent uploadpack.allowrefinwant option is in the
> same boat.
> 
> There are a few possible fixes:
> 
>   1. Bump the scope conditional back to the bottom of the
>  chain. But that just means somebody else is likely to
>  make the same mistake later.
> 
>   2. Make the conditional more like the others. I.e.:
> 
>else if (!current_config_scope() != CONFIG_SCOPE_REPO &&
> !strcmp(var, "uploadpack.notallowedinrepo"))
> 
>  This works, but the idea of the original structure was
>  that we may grow multiple sensitive options like this.
> 
>   3. Pull it out of the chain entirely. The chain mostly
>  serves to avoid extra strcmp() calls after we've found
>  a match. But it's not worth caring about those. In the
>  worst case, when there isn't a match, we're already
>  hitting every strcmp (and this happens regularly for
>  stuff like "core.bare", etc).
> 
> This patch does (3).
> 
> Signed-off-by: Jeff King 
> ---
> Phew. That was a lot of explanation for a small patch, but
> this was sufficiently subtle I thought it worth it. And
> also, I was really surprised the bug managed to exist for
> this long without anybody noticing.

Maybe a lot of explanation, but definitely a good one. The explanation and
the patch look good to me.

Thanks,
Dscho

> 
>  upload-pack.c | 9 ++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/upload-pack.c b/upload-pack.c
> index 540778d1dd..489c18e222 100644
> --- a/upload-pack.c
> +++ b/upload-pack.c
> @@ -1028,14 +1028,17 @@ static int upload_pack_config(const char *var, const 
> char *value, void *unused)
>   keepalive = git_config_int(var, value);
>   if (!keepalive)
>   keepalive = -1;
> - } else if (current_config_scope() != CONFIG_SCOPE_REPO) {
> - if (!strcmp("uploadpack.packobjectshook", var))
> - return git_config_string(_objects_hook, var, 
> value);
>   } else if (!strcmp("uploadpack.allowfilter", var)) {
>   allow_filter = git_config_bool(var, value);
>   } else if (!strcmp("uploadpack.allowrefinwant", var)) {
>   allow_ref_in_want = git_config_bool(var, value);
>   }
> +
> + if (current_config_scope() != CONFIG_SCOPE_REPO) {
> + if (!strcmp("uploadpack.packobjectshook", var))
> + return git_config_string(_objects_hook, var, 
> value);
> + }
> +
>   return parse_hide_refs_config(var, value, "uploadpack");
>  }
>  
> -- 
> 2.19.1.1094.gd480080bf6
>

Re: [PATCH v3 1/3] repack: point out a bug handling stale shallow info

Hi Junio,

me again. I was wrong.

On Wed, 24 Oct 2018, Johannes Schindelin wrote:

> On Wed, 24 Oct 2018, Junio C Hamano wrote:
> 
> > "Johannes Schindelin via GitGitGadget" 
> > writes:
> > 
> > > +...
> > > + d="$(git -C shallow-server rev-parse --verify D)" &&
> > > + git -C shallow-server checkout master &&
> > > +
> > > +...
> > > + ! grep $d shallow-client/.git/shallow &&
> > 
> > We know D (and $d) is not a tag,
> 
> Actually, it is... the `test_commit D` creates that tag, and that is what
> I use here.
> 
> > but perhaps the place that assigns to $d (way above) can do the
> > rev-parse of D^0, not D, to make it more clear what is going on,
> > especially given that...
> 
> ... that the `grep` really wants to test for the absence of the *commit*,
> not the *tag* in .git/shallow?
> 
> Yes, you are right ;-)
> 
> So why did my test do the right thing, if it looked at a tag, but did not
> dereference it via ^0? The answer is: the `test_commit` function creates
> light-weight tags, i.e. no tag objects. And therefore, the $d^0 you found
> below, that's just confusing.

What I was referring to was the call

test_must_fail git -C shallow-client rev-parse --verify $d^0

However, here we *have* to append ^0, otherwise `rev-parse --verify` will
(and quite confusingly so) *succeed*. I *repeatedly* fall into that trap
that `git rev-parse --verify `
will succeed. Why? Because that is a valid 40-digit hex string. Not
because the object exists. Because it does not.

So I managed to confuse myself again into believing that I only need to
append ^0 to the earlier rev-parse call, but can remove it from this one,
when in reality, I have to append it to both. In the first case, to avoid
having to think about dereferencing a tag, in the second case, to force
rev-parse to look for the object.

Ciao,
Dscho

> 
> I will change it so that the `rev-parse` call uses ^0 (even if it is
> technically not necessary), to avoid said confusion.
> 
> Thanks,
> Dscho
> 
> > 
> > > + git -C shallow-server branch branch-orig D^0 &&
> > 
> > ... this does that D^0 thing here to makes us wonder if D needs
> > unwrapping before using it as a commit (not commit-ish). 
> > 
> > If we did so, we could use $d here instead of D^0.
> > 
> > > + git -C shallow-client fetch --prune --depth=2 \
> > > + origin "+refs/heads/*:refs/remotes/origin/*"
> > > +'
> > > +
> > >  . "$TEST_DIRECTORY"/lib-httpd.sh
> > >  start_httpd
> > 
> > 
>

Re: How to start contributing

On Sat, Oct 20, 2018 at 08:47:35AM +0200, Christian Couder wrote:

> > Make “git tag –contains ” less chatty if  is invalid
> > “git tag – contains ” prints the whole help text if  is
> > invalid. It should only show the error message instead.
> > Thread: 
> > https://public-inbox.org/git/20160118215433.gb24...@sigill.intra.peff.net/
> >
> > This bug is solved using the 3rd option, but I suspect that it’s still
> > here because the 2nd option is preferred.
> 
> I think it should probably have been removed from the micro-project
> list. I am CC'ing Peff as he wrote the email listing the different
> options to solve the original issue and may think otherwise.

Sorry for the reply.

Yeah, I I think the solution in 3bb0923f06 (parse-options: do not show
usage upon invalid option value, 2018-03-22) is fine. Even though it may
not have been what _I_ would have done personally, I don't think it is
worth anybody spending more brain cycles on it.

-Peff

Re: [PATCH v3 1/3] repack: point out a bug handling stale shallow info

Hi Junio,

On Wed, 24 Oct 2018, Junio C Hamano wrote:

> "Johannes Schindelin via GitGitGadget" 
> writes:
> 
> > +...
> > +   d="$(git -C shallow-server rev-parse --verify D)" &&
> > +   git -C shallow-server checkout master &&
> > +
> > +...
> > +   ! grep $d shallow-client/.git/shallow &&
> 
> We know D (and $d) is not a tag,

Actually, it is... the `test_commit D` creates that tag, and that is what
I use here.

> but perhaps the place that assigns to $d (way above) can do the
> rev-parse of D^0, not D, to make it more clear what is going on,
> especially given that...

... that the `grep` really wants to test for the absence of the *commit*,
not the *tag* in .git/shallow?

Yes, you are right ;-)

So why did my test do the right thing, if it looked at a tag, but did not
dereference it via ^0? The answer is: the `test_commit` function creates
light-weight tags, i.e. no tag objects. And therefore, the $d^0 you found
below, that's just confusing.

I will change it so that the `rev-parse` call uses ^0 (even if it is
technically not necessary), to avoid said confusion.

Thanks,
Dscho

> 
> > +   git -C shallow-server branch branch-orig D^0 &&
> 
> ... this does that D^0 thing here to makes us wonder if D needs
> unwrapping before using it as a commit (not commit-ish). 
> 
> If we did so, we could use $d here instead of D^0.
> 
> > +   git -C shallow-client fetch --prune --depth=2 \
> > +   origin "+refs/heads/*:refs/remotes/origin/*"
> > +'
> > +
> >  . "$TEST_DIRECTORY"/lib-httpd.sh
> >  start_httpd
> 
>

Re: [PATCH] worktree: populate lock_reason in get_worktrees and light refactor/cleanup in worktree files

2018-10-24 Thread Eric Sunshine

On Wed, Oct 24, 2018 at 2:39 AM  wrote:
> lock_reason is now populated during the execution of get_worktrees
>
> is_worktree_locked has been simplified, renamed, and changed to internal
> linkage. It is simplified to only return the lock reason (or NULL in case
> there is no lock reason) and to not have any side effects on the inputs.
> As such it made sense to rename it since it only returns the reason.
>
> Since this function was now being used to populate the worktree struct's
> lock_reason field, it made sense to move the function to internal
> linkage and have callers refer to the lock_reason field. The
> lock_reason_valid field was removed since a NULL/non-NULL value of
> lock_reason accomplishes the same effect.
>
> Some unused variables within worktree source code were removed.

Thanks for the submission.

One thing which isn't clear from this commit message is _why_ this
change is desirable at this time, aside from the obvious
simplification of the code and client interaction (or perhaps those
are the _why_?).

Although I had envisioned populating the "reason" field greedily in
the way this patch does, not everyone agrees that doing so is
desirable. In particular, Junio argued[1,2] for populating it lazily,
which accounts for the current implementation. That's why I ask about
the _why_ of this change since it will likely need to be justified in
a such a way to convince Junio to change his mind.

Thanks.

[1]: https://public-inbox.org/git/xmqq8tyq5czn@gitster.mtv.corp.google.com/
[2]: https://public-inbox.org/git/xmqq4m9d0w6v@gitster.mtv.corp.google.com/

[PATCH] howto/using-merge-subtree: mention --allow-unrelated-histories

2018-10-24 Thread Uwe Kleine-König

Without passing --allow-unrelated-histories the command sequence
fails as intended since commit e379fdf34fee ("merge: refuse to create
too cool a merge by default"). To setup a subtree merging unrelated
histories is normal, so add the option to the howto document.

Signed-off-by: Uwe Kleine-König 
---
 Documentation/howto/using-merge-subtree.txt | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Documentation/howto/using-merge-subtree.txt 
b/Documentation/howto/using-merge-subtree.txt
index 1ae8d1214ec0..a499a94ac228 100644
--- a/Documentation/howto/using-merge-subtree.txt
+++ b/Documentation/howto/using-merge-subtree.txt
@@ -33,7 +33,7 @@ Here is the command sequence you need:
 
 
 $ git remote add -f Bproject /path/to/B <1>
-$ git merge -s ours --no-commit Bproject/master <2>
+$ git merge -s ours --no-commit --allow-unrelated-histories Bproject/master <2>
 $ git read-tree --prefix=dir-B/ -u Bproject/master <3>
 $ git commit -m "Merge B project as our subdirectory" <4>
 
-- 
2.19.1

Re: [PATCH v3 3/3] repack -ad: prune the list of shallow commits

Hi Junio,

On Wed, 24 Oct 2018, Junio C Hamano wrote:

> "Johannes Schindelin via GitGitGadget" 
> writes:
> 
> > diff --git a/builtin/repack.c b/builtin/repack.c
> > index c6a7943d5..9217fc832 100644
> > --- a/builtin/repack.c
> > +++ b/builtin/repack.c
> > @@ -549,6 +549,12 @@ int cmd_repack(int argc, const char **argv, const char 
> > *prefix)
> > if (!po_args.quiet && isatty(2))
> > opts |= PRUNE_PACKED_VERBOSE;
> > prune_packed_objects(opts);
> > +
> > +   if (!keep_unreachable &&
> > +   (!(pack_everything & LOOSEN_UNREACHABLE) ||
> > +unpack_unreachable) &&
> > +   is_repository_shallow(the_repository))
> > +   prune_shallow(0, 1);
> 
> The logic looks correct (and the reasoning behind it, which was
> explained in the log message, was sound).  prune_shallow(0, 1)
> however is not easily understandable.
> 
> There are only two callsites of this function after these three
> patches, and the areas of the code that have these calls are in
> relatively quiescent parts in the whole tree, so it shouldn't be too
> distracting to do an update to make the function take a flag word,
> so that we can make callsites more immediately obvious, i.e.
> 
>   prune_shallow(PRUNE_SHALLOW_QUICK)
> 
> It of course can be left as a low-hanging fruit loose-end.

I almost did that, but then decided not to clutter the previous patch with
this change (and global constant).

Having said that, since you also had this idea, I will make that change.
It will read nicer, you are right.

Ciao,
Dscho

> 
> > diff --git a/t/t5537-fetch-shallow.sh b/t/t5537-fetch-shallow.sh
> > index 2d0031703..777c9d1dc 100755
> > --- a/t/t5537-fetch-shallow.sh
> > +++ b/t/t5537-fetch-shallow.sh
> > @@ -186,7 +186,7 @@ EOF
> > test_cmp expect actual
> >  '
> >  
> > -test_expect_failure '.git/shallow is edited by repack' '
> > +test_expect_success '.git/shallow is edited by repack' '
> > git init shallow-server &&
> > test_commit -C shallow-server A &&
> > test_commit -C shallow-server B &&
>

Re: [PATCH v3 2/3] shallow: offer to prune only non-existing entries

Hi Junio & Jonathan,

On Wed, 24 Oct 2018, Junio C Hamano wrote:

> Jonathan, do you see any issues with the use of lookup_commit() in
> this change wrt lazy clone?  I am wondering what happens when the
> commit in question is at, an immediate parent of, or an immediate
> child of a promisor object.  I _think_ this change won't make it
> worse for two features in playing together, but thought that it
> would be better to double check.

Good point.

Instinctively, I would say that no promised object can be a shallow
commit. The entire idea of a shallow commit is that it *is* present, but
none of its parents.

Also, I would claim that the shallow feature does not make sense with lazy
clones, as lazy clones offer a superset of shallow clone's functionality.

However, I am curious whether there is a better way to check for the
presence of a local commit? Do we have an API function for that, that I
missed? (I do not really want to parse the commit, after all, just verify
that it is not pruned.)

Thanks,
Dscho

> 
> "Johannes Schindelin via GitGitGadget" 
> writes:
> 
> > From: Johannes Schindelin 
> >
> > The `prune_shallow()` function wants a full reachability check to be
> > completed before it goes to work, to ensure that all unreachable entries
> > are removed from the shallow file.
> >
> > However, in the upcoming patch we do not even want to go that far. We
> > really only need to remove entries corresponding to pruned commits, i.e.
> > to commits that no longer exist.
> >
> > Let's support that use case.
> >
> > Signed-off-by: Johannes Schindelin 
> > ---
> >  builtin/prune.c |  2 +-
> >  commit.h|  2 +-
> >  shallow.c   | 22 +-
> >  3 files changed, 19 insertions(+), 7 deletions(-)
> >
> > diff --git a/builtin/prune.c b/builtin/prune.c
> > index 41230f821..6d6ab6cf1 100644
> > --- a/builtin/prune.c
> > +++ b/builtin/prune.c
> > @@ -161,7 +161,7 @@ int cmd_prune(int argc, const char **argv, const char 
> > *prefix)
> > free(s);
> >  
> > if (is_repository_shallow(the_repository))
> > -   prune_shallow(show_only);
> > +   prune_shallow(show_only, 0);
> >  
> > return 0;
> >  }
> > diff --git a/commit.h b/commit.h
> > index 1d260d62f..ff34447ab 100644
> > --- a/commit.h
> > +++ b/commit.h
> > @@ -249,7 +249,7 @@ extern void assign_shallow_commits_to_refs(struct 
> > shallow_info *info,
> >uint32_t **used,
> >int *ref_status);
> >  extern int delayed_reachability_test(struct shallow_info *si, int c);
> > -extern void prune_shallow(int show_only);
> > +extern void prune_shallow(int show_only, int quick_prune);
> >  extern struct trace_key trace_shallow;
> >  
> >  extern int interactive_add(int argc, const char **argv, const char 
> > *prefix, int patch);
> > diff --git a/shallow.c b/shallow.c
> > index 732e18d54..0a2671bc2 100644
> > --- a/shallow.c
> > +++ b/shallow.c
> > @@ -247,6 +247,7 @@ static void check_shallow_file_for_update(struct 
> > repository *r)
> >  
> >  #define SEEN_ONLY 1
> >  #define VERBOSE   2
> > +#define QUICK_PRUNE 4
> >  
> >  struct write_shallow_data {
> > struct strbuf *out;
> > @@ -261,7 +262,11 @@ static int write_one_shallow(const struct commit_graft 
> > *graft, void *cb_data)
> > const char *hex = oid_to_hex(>oid);
> > if (graft->nr_parent != -1)
> > return 0;
> > -   if (data->flags & SEEN_ONLY) {
> > +   if (data->flags & QUICK_PRUNE) {
> > +   struct commit *c = lookup_commit(the_repository, >oid);
> > +   if (!c || parse_commit(c))
> > +   return 0;
> > +   } else if (data->flags & SEEN_ONLY) {
> > struct commit *c = lookup_commit(the_repository, >oid);
> > if (!c || !(c->object.flags & SEEN)) {
> > if (data->flags & VERBOSE)
> > @@ -371,16 +376,23 @@ void advertise_shallow_grafts(int fd)
> >  
> >  /*
> >   * mark_reachable_objects() should have been run prior to this and all
> > - * reachable commits marked as "SEEN".
> > + * reachable commits marked as "SEEN", except when quick_prune is non-zero,
> > + * in which case lines are excised from the shallow file if they refer to
> > + * commits that do not exist (any longer).
> >   */
> > -void prune_shallow(int show_only)
> > +void prune_shallow(int show_only, int quick_prune)
> >  {
> > struct lock_file shallow_lock = LOCK_INIT;
> > struct strbuf sb = STRBUF_INIT;
> > +   unsigned flags = SEEN_ONLY;
> > int fd;
> >  
> > +   if (quick_prune)
> > +   flags |= QUICK_PRUNE;
> > +
> > if (show_only) {
> > -   write_shallow_commits_1(, 0, NULL, SEEN_ONLY | VERBOSE);
> > +   flags |= VERBOSE;
> > +   write_shallow_commits_1(, 0, NULL, flags);
> > strbuf_release();
> > return;
> > }
> > @@ -388,7 +400,7 @@ void prune_shallow(int show_only)
> >git_path_shallow(the_repository),
> >

Re: Git trademark status and policy

2018-10-24 Thread David Aguilar

On Tue, Sep 18, 2018 at 02:22:22PM -0400, Jeff King wrote:
> On Mon, Sep 17, 2018 at 11:25:31AM +0200, Christian Couder wrote:
> 
> > > (Also, to be clear, this is all _only_ about "Git Cola". The "git-cola"
> > > command is explicitly OK in the policy because that's how commands
> > > work).
> > 
> > I agree about "git-cola" though I wonder about "git-dag" as this is
> > another command used by the project that is more generic. For example
> > I could imagine that, if we wanted to provide a shortcut for `git log
> > --graph --decorate --oneline`, we might want to use `git dag`.
> > 
> > I guess we can still recommend to change it if possible, though we can
> > also acknowledge that, as our recommendation comes very late (too
> > late?), it is just a "weak" recommendation.
> 
> Yeah, I agree with you, though I think it is a separate issue. "git-dag"
> is explicitly OK in the trademark policy, and they are not using "Git
> Dag" in any recognizable way.
> 
> So I think there is no trademark issue, but "git-dag" is probably just
> not a great idea in general, because the namespace is open and it is
> likely to get stomped by some other project. Or git itself. Or it may
> even be annoying for users who have a "git dag" alias (on-disk commands
> always override aliases).
> 
> So I think we should generally recommend against such generic names
> during the naming phase. At this point, I'm not sure the pain of
> changing now is any less than the pain of changing later if and when
> there's a conflict.
> 
> I think I'm actually violently agreeing with you, but I wanted to make
> it clear. :) (And everything else in your email seemed sensible, too).
> 
> -Peff

Thanks for the recommendation.  I'm open to changing the name in a
future major release.  For users that already use the short "dag" name,
we can transition over to something else if it's relatively short and
sweet.

Maybe a better name would be "git-kcola" (a nod to gitk), or "git-vdag"
for "visual DAG"?  Any sugs?  I'm terrible at naming things, but I do
refrain from using additional "git-*" names beyond these two for the
project.  I kinda like "vdag" since it's easy to type, and nearby the
existing "dag" name.

There's also one more script, but it's never installed in the users's
$PATH and is more of an internal implementation detail.  Git Cola
includes a GIT_SEQUENCE_EDITOR-compatible "git-xbase" command that
provides a visual interactive rebase feature.  That command should
probably be renamed to "cola-git-seq-editor" to make that clearer, and
also to open up the possibility of installing it in bin/ in the future
since it is useful on its own.

The rationale for two commands is that worktree diff+commit and history
inspection are our two primary use-cases.  Everything else is provided
as a sub-command, "git cola rebase", "git cola stash", etc. so there's
not much pressure to add more top-level names, just these two.

Thoughts?
-- 
David

Re: [PATCH] i18n: make GETTEXT_POISON a runtime option

On Wed, Oct 24, 2018 at 02:45:49PM +0900, Junio C Hamano wrote:

> Ævar Arnfjörð Bjarmason   writes:
> 
> > Notes on the implementation:
> >
> >  * The only reason we need a new "git-sh-i18n--helper" and the
> >corresponding "test-tool gettext-poison" is to expose
> >git_env_bool() to shellscripts, since git-sh-i18n and friends need
> >to inspect the $GIT_TEST_GETTEXT_POISON variable.
> >
> >We only call these if $GIT_TEST_GETTEXT_POISON is set, or in the
> >test suite, and this code can go away for non-test code once the
> >last i18n-using shellscript is rewritten in C.
> 
> Makes me wonder if we want to teach "git config" a new "--env-bool"
> option that can be used by third-party scripts.  Or would it be just
> the matter of writing
> 
>   git config --default=false --type=bool "$GIT_WHATEVER_ENV"
> 
> in these third-party scripts and we do not need to add such a thing?

The config command only takes names, not values. So you'd have to write
something like:

  git -c env.bool="$GIT_WHATEVER_ENV" config --type=bool env.bool

but then you lose the default handling. I think if we added a new
option, it would either be:

  # interpret a value directly; use default on empty, I guess?
  git config --default=false --type=bool --interpret-value "$GIT_WHATEVER_ENV"

or

  # less flexible, but the --default semantics are more obvious
  git config --default=false --type=bool --get-env GIT_WHATEVER_ENV

-Peff

Re: [PATCH 1/3] [Outreachy] t3903-stash: test without configured user name

Hi Junio,

On Wed, 24 Oct 2018, Junio C Hamano wrote:

> Slavica  writes:
> 
> > +test_expect_failure 'stash with HOME as non-existing directory' '
> > +test_commit 1 &&
> > +test_config user.useconfigonly true &&
> > +test_config stash.usebuiltin true &&
> > +(
> > +HOME=$(pwd)/none &&
> > +export HOME &&
> 
> What is the reason why this test needs to move HOME away from
> TRASH_DIRECTORY (set in t/test-lib.sh)?

This is to make sure that no user.name nor user.email is configured. That
was my idea, hence I answer your question.

Ciao,
Dscho

> 
> > +unset GIT_AUTHOR_NAME &&
> > +unset GIT_AUTHOR_EMAIL &&
> > +unset GIT_COMMITTER_NAME &&
> > +unset GIT_COMMITTER_EMAIL &&
> > +test_must_fail git config user.email &&
> > +echo changed >1.t &&
> > +   git stash
> > +)
> > +'
> > +
> >  test_done
>

[PATCH 2/2] run-command: mark path lookup errors with ENOENT